[+cc linux-pci, sorry, due to the @google.com DMARC hassles I can't reply
directly to mail sent to bhelgaas@xxxxxxxxxx or to helgaas@xxxxxxxxxx
(which is currently forwarded to bhelgaas@xxxxxxxxxx). I *can* reply to
linux-pci mail because I have a non google.com account subscribed there.
Sorry again, I know this is TMI. I'm going to manually insert Thomas'
response so I can respond.]
Thomas Petazzoni wrote:
> On Wed, Apr 27, 2016 at 05:58:27PM -0500, Bjorn Helgaas wrote:
>> Hi Thomas & Liviu,
>>
>> You added pci_ioremap_set_mem_type(int mem_type) with 1c8c3cf0b523
>> ("ARM: 8060/1: mm: allow sub-architectures to override PCI I/O memory
>> type").
>>
>> I see this patch on the list: "[PATCH 3/3] ARM: mvebu: implement
>> L2/PCIe deadlock workaround"
>> (http://lists.infradead.org/pipermail/linux-arm-kernel/2014-March/242784.html)
>> that does call pci_ioremap_set_mem_type(), but it doesn't look like
>> that patch ever got merged.
>>
>> Is it still useful to have pci_ioremap_set_mem_type() even though
>> nobody calls it?
> I am actually about to send a patch that makes use of it, I discussed
> it with Arnd a few days ago / last week.
>
> Essentially the story was the following. On Cortex-A9 based Marvell
> SoCs, when HW I/O coherency is enabled, you need all non-RAM space to
> be mapped strongly ordered. To this effect, I submitted this patch
> series that introduces pci_ioremap_mem_type() to allow
> platform-specific code to override the memory type used to map PCI I/O
> space.
>
> The reaction of Arnd after this patch series was to suggest that maybe
> I should just change the memory type for all platforms, so I sent
> another patch doing that.
>
> But in the end, Russell merged the implementation of
> pci_ioremap_mem_type() from the previous version of the patch series.
> And the remaining patch fell through the cracks. Since PCI I/O is
> rarely used these days, it wasn't noticed.
>
> But right now, I have this in my stack of patches to be sent:
> ...
> diff --git a/arch/arm/mach-mvebu/coherency.c b/arch/arm/mach-mvebu/coherency.c
> index 7e989d6..2d1f06d 100644
> --- a/arch/arm/mach-mvebu/coherency.c
> +++ b/arch/arm/mach-mvebu/coherency.c
> ...
> @@ -186,7 +180,8 @@ static void __init armada_375_380_coherency_init(struct device_node *np)
> struct device_node *cache_dn;
>
> coherency_cpu_base = of_iomap(np, 0);
> - arch_ioremap_caller = armada_pcie_wa_ioremap_caller;
> + arch_ioremap_caller = armada_wa_ioremap_caller;
> + pci_ioremap_set_mem_type(MT_UNCACHED);
This makes sense to me because I think this changes ioremap() to do
the right thing. But my concern is that ioremap_page_range() doesn't
actually use ioremap(), so pci_ioremap_set_mem_type() has no effect on
that path. pci_remap_iospace() uses ioremap_page_range(), so I
suspect it will use the wrong attributes.
>> I'm looking at the issue of how we ioremap memory-mapped ioport
>> spaces, and pci_ioremap_mem_type is currently used in the arm-specific
>> pci_ioremap_io():
>>
>> int pci_ioremap_io(unsigned int offset, phys_addr_t phys_addr)
>> {
>> BUG_ON(offset + SZ_64K > IO_SPACE_LIMIT);
>>
>> return ioremap_page_range(PCI_IO_VIRT_BASE + offset,
>> PCI_IO_VIRT_BASE + offset + SZ_64K,
>> phys_addr,
>> __pgprot(get_mem_type(pci_ioremap_mem_type)->prot_pte));
>> }
>>
>> Also, what about pci_remap_iospace(), added by 8b921acfeffd ("PCI: Add
>> pci_remap_iospace() to map bus I/O resources")?
>>
>> int __weak pci_remap_iospace(const struct resource *res, phys_addr_t phys_addr)
>> {
>> #if defined(PCI_IOBASE) && defined(CONFIG_MMU)
>> unsigned long vaddr = (unsigned long)PCI_IOBASE + res->start;
>>
>> if (!(res->flags & IORESOURCE_IO))
>> return -EINVAL;
>>
>> if (res->end > IO_SPACE_LIMIT)
>> return -EINVAL;
>>
>> return ioremap_page_range(vaddr, vaddr + resource_size(res), phys_addr,
>> pgprot_device(PAGE_KERNEL));
>> ...
> Yes, I was also surprised to see two functions doing pretty much the
> same, this probably needs to be refactored. There are 11 users of
> pci_ioremap_io() (6 of them being Marvell related), and
> pci_ioremap_io() is provided by ARM specific code.
>
> On the other hand, pci_remap_iospace() is provided by
> architecture-independent code, and use in 6 PCI controller drivers.
>
> What about:
>
> 1/ Providing an alternate version of pci_remap_iospace() that allows
> to pass the memory attribute to be used.
I'd rather fix ioremap_page_range() somehow, because that's an
exported interface, and if we only fix pci_remap_iospace(), we still
have to worry about ioremap_page_range() doing the wrong thing.
> 2/ Convert all users of pci_ioremap_io() to pci_remap_iospace().
>
> 3/ Get rid of pci_ioremap_io().
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
