On Fri, Mar 14, 2014 at 03:34:27PM +0000, Liviu Dudau wrote:
> Some architectures do not share x86 simple view of the PCI I/O space
> and instead use a range of addresses that map to bus addresses. For
> some architectures these ranges will be expressed by OF bindings
> in a device tree file.
It's true that the current Linux "x86 view of PCI I/O space" is pretty
simple and limited. But I don't think that's a fundamental x86 limitation
(other than the fact that the actual INB/OUTB/etc. CPU instructions
themselves are limited to a single 64K I/O port space).
Host bridges on x86 could have MMIO apertures that turn CPU memory accesses
into PCI port accesses. We could implement any number of I/O port spaces
this way, by making the kernel inb()/outb()/etc. interfaces smart enough to
use the memory-mapped space instead of (or in addition to) the
INB/OUTB/etc. instructions.
ia64 does this (see arch/ia64/include/asm/io.h for a little description)
and I think maybe one or two other arches have something similar.
> Introduce a pci_register_io_range() helper function that can be used
> by the architecture code to keep track of the I/O ranges described by the
> PCI bindings. If the PCI_IOBASE macro is not defined that signals
> lack of support for PCI and we return an error.
I don't quite see how you intend to use this, because this series doesn't
include any non-stub implementation of pci_register_io_range().
Is this anything like the ia64 strategy I mentioned above? If so, it would
be really nice to unify some of this stuff.
> Signed-off-by: Liviu Dudau <[email protected]>
> Acked-by: Grant Likely <[email protected]>
> Tested-by: Tanmay Inamdar <[email protected]>
> ---
> drivers/of/address.c | 9 +++++++++
> include/linux/of_address.h | 1 +
> 2 files changed, 10 insertions(+)
>
> diff --git a/drivers/of/address.c b/drivers/of/address.c
> index 1a54f1f..be958ed 100644
> --- a/drivers/of/address.c
> +++ b/drivers/of/address.c
> @@ -619,6 +619,15 @@ const __be32 *of_get_address(struct device_node *dev, int index, u64 *size,
> }
> EXPORT_SYMBOL(of_get_address);
>
> +int __weak pci_register_io_range(phys_addr_t addr, resource_size_t size)
> +{
> +#ifndef PCI_IOBASE
> + return -EINVAL;
> +#else
> + return 0;
> +#endif
> +}
> +
> unsigned long __weak pci_address_to_pio(phys_addr_t address)
> {
> if (address > IO_SPACE_LIMIT)
> diff --git a/include/linux/of_address.h b/include/linux/of_address.h
> index 5f6ed6b..40c418d 100644
> --- a/include/linux/of_address.h
> +++ b/include/linux/of_address.h
> @@ -56,6 +56,7 @@ extern void __iomem *of_iomap(struct device_node *device, int index);
> extern const __be32 *of_get_address(struct device_node *dev, int index,
> u64 *size, unsigned int *flags);
>
> +extern int pci_register_io_range(phys_addr_t addr, resource_size_t size);
> extern unsigned long pci_address_to_pio(phys_addr_t addr);
>
> extern int of_pci_range_parser_init(struct of_pci_range_parser *parser,
> --
> 1.9.0
>
On Fri, 2014-04-04 at 18:19 -0600, Bjorn Helgaas wrote:
> > Introduce a pci_register_io_range() helper function that can be used
> > by the architecture code to keep track of the I/O ranges described by the
> > PCI bindings. If the PCI_IOBASE macro is not defined that signals
> > lack of support for PCI and we return an error.
>
> I don't quite see how you intend to use this, because this series doesn't
> include any non-stub implementation of pci_register_io_range().
>
> Is this anything like the ia64 strategy I mentioned above? If so, it would
> be really nice to unify some of this stuff.
We also use two different strategies on ppc32 and ppc64
- On ppc32, inb/outb turn into an MMIO access to _IO_BASE + port
That _IO_BASE is a variable which is initialized to the ioremapped address
of the IO space MMIO aperture of the first bridge we discover. Then port
numbers are "fixed up" on all other bridges so that the addition _IO_BASE + port
fits the ioremapped address of the IO space on that bridge. A bit messy... and breaks
whenever drivers copy port numbers into variables of the wrong type such as shorts.
- On ppc64, we have more virtual space, so instead we reserve a range
of address space (fixed) for IO space, it's always the same. Bridges IO spaces
are then mapped into that range, so we always have a positive offset from _IO_BASE
which makes things a bit more robust and less "surprising" than ppc32. Additionally,
the first 64k are reserved. They are only mapped if we see an ISA bridge (which some
older machines have). Otherwise it's left unmapped, so crappy drivers trying to
hard code x86 IO ports will blow up immediately which I deem better than silently
whacking the wrong hardware. In addition, we have a mechanism we use on powernv to
re-route accesses to that first 64k to the power8 built-in LPC bus which can
have some legacy IOs on it such as a UART or a RTC.
Cheers,
Ben.
On Sat, Apr 05, 2014 at 01:19:53AM +0100, Bjorn Helgaas wrote:
> On Fri, Mar 14, 2014 at 03:34:27PM +0000, Liviu Dudau wrote:
> > Some architectures do not share x86 simple view of the PCI I/O space
> > and instead use a range of addresses that map to bus addresses. For
> > some architectures these ranges will be expressed by OF bindings
> > in a device tree file.
>
> It's true that the current Linux "x86 view of PCI I/O space" is pretty
> simple and limited. But I don't think that's a fundamental x86 limitation
> (other than the fact that the actual INB/OUTB/etc. CPU instructions
> themselves are limited to a single 64K I/O port space).
Hi Bjorn,
Thanks for reviewing this series.
I might've taken a too dim view of x86 world. I tend to split the existing
architectures into the ones that have special I/O instructions and the ones
that map a region of memory into CPU space and do I/O transactions there as
simple read/writes.
>
> Host bridges on x86 could have MMIO apertures that turn CPU memory accesses
> into PCI port accesses. We could implement any number of I/O port spaces
> this way, by making the kernel inb()/outb()/etc. interfaces smart enough to
> use the memory-mapped space instead of (or in addition to) the
> INB/OUTB/etc. instructions.
Right, sorry for my ignorance then: how does *currently* the device driver do
the I/O transfer transparent of the implementation mechanism? Or they have
intimate knowledge of wether the device is behind a host bridge and can do MMIO
or is on an ISA or CF bus and then it needs INB/OUTB ? And if we make inb/outb
smarter, does that mean that we need to change the drivers?
>
> ia64 does this (see arch/ia64/include/asm/io.h for a little description)
> and I think maybe one or two other arches have something similar.
>
> > Introduce a pci_register_io_range() helper function that can be used
> > by the architecture code to keep track of the I/O ranges described by the
> > PCI bindings. If the PCI_IOBASE macro is not defined that signals
> > lack of support for PCI and we return an error.
>
> I don't quite see how you intend to use this, because this series doesn't
> include any non-stub implementation of pci_register_io_range().
>
> Is this anything like the ia64 strategy I mentioned above? If so, it would
> be really nice to unify some of this stuff.
After discussions with Arnd and Catalin I know have a new series that moves
some of the code from arm64 series into this one. I am putting it through
testing right know as I am going to have to depend on another series that
makes PCI_IOBASE defined only for architectures that do MMIO in order to
choose the correct default implementation for these functions. My hope is
that I will be able to send the series this week.
Best regards,
Liviu
>
> > Signed-off-by: Liviu Dudau <[email protected]>
> > Acked-by: Grant Likely <[email protected]>
> > Tested-by: Tanmay Inamdar <[email protected]>
> > ---
> > drivers/of/address.c | 9 +++++++++
> > include/linux/of_address.h | 1 +
> > 2 files changed, 10 insertions(+)
> >
> > diff --git a/drivers/of/address.c b/drivers/of/address.c
> > index 1a54f1f..be958ed 100644
> > --- a/drivers/of/address.c
> > +++ b/drivers/of/address.c
> > @@ -619,6 +619,15 @@ const __be32 *of_get_address(struct device_node *dev, int index, u64 *size,
> > }
> > EXPORT_SYMBOL(of_get_address);
> >
> > +int __weak pci_register_io_range(phys_addr_t addr, resource_size_t size)
> > +{
> > +#ifndef PCI_IOBASE
> > + return -EINVAL;
> > +#else
> > + return 0;
> > +#endif
> > +}
> > +
> > unsigned long __weak pci_address_to_pio(phys_addr_t address)
> > {
> > if (address > IO_SPACE_LIMIT)
> > diff --git a/include/linux/of_address.h b/include/linux/of_address.h
> > index 5f6ed6b..40c418d 100644
> > --- a/include/linux/of_address.h
> > +++ b/include/linux/of_address.h
> > @@ -56,6 +56,7 @@ extern void __iomem *of_iomap(struct device_node *device, int index);
> > extern const __be32 *of_get_address(struct device_node *dev, int index,
> > u64 *size, unsigned int *flags);
> >
> > +extern int pci_register_io_range(phys_addr_t addr, resource_size_t size);
> > extern unsigned long pci_address_to_pio(phys_addr_t addr);
> >
> > extern int of_pci_range_parser_init(struct of_pci_range_parser *parser,
> > --
> > 1.9.0
> >
>
--
====================
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
¯\_(ツ)_/¯
On Sun, Apr 06, 2014 at 10:49:53AM +0100, Benjamin Herrenschmidt wrote:
> On Fri, 2014-04-04 at 18:19 -0600, Bjorn Helgaas wrote:
> > > Introduce a pci_register_io_range() helper function that can be used
> > > by the architecture code to keep track of the I/O ranges described by the
> > > PCI bindings. If the PCI_IOBASE macro is not defined that signals
> > > lack of support for PCI and we return an error.
> >
> > I don't quite see how you intend to use this, because this series doesn't
> > include any non-stub implementation of pci_register_io_range().
> >
> > Is this anything like the ia64 strategy I mentioned above? If so, it would
> > be really nice to unify some of this stuff.
>
> We also use two different strategies on ppc32 and ppc64
>
> - On ppc32, inb/outb turn into an MMIO access to _IO_BASE + port
>
> That _IO_BASE is a variable which is initialized to the ioremapped address
> of the IO space MMIO aperture of the first bridge we discover. Then port
> numbers are "fixed up" on all other bridges so that the addition _IO_BASE + port
> fits the ioremapped address of the IO space on that bridge. A bit messy... and breaks
> whenever drivers copy port numbers into variables of the wrong type such as shorts.
>
> - On ppc64, we have more virtual space, so instead we reserve a range
> of address space (fixed) for IO space, it's always the same. Bridges IO spaces
> are then mapped into that range, so we always have a positive offset from _IO_BASE
> which makes things a bit more robust and less "surprising" than ppc32. Additionally,
> the first 64k are reserved. They are only mapped if we see an ISA bridge (which some
> older machines have). Otherwise it's left unmapped, so crappy drivers trying to
> hard code x86 IO ports will blow up immediately which I deem better than silently
> whacking the wrong hardware. In addition, we have a mechanism we use on powernv to
> re-route accesses to that first 64k to the power8 built-in LPC bus which can
> have some legacy IOs on it such as a UART or a RTC.
>
> Cheers,
> Ben.
>
Hi Benjamin,
Thanks for the summary, is really useful as I was recently looking into code in that
area. One thing I was trying to understand is why ppc needs _IO_BASE at all rather
than using the generic PCI_IOBASE?
Best regards,
Liviu
--
====================
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
¯\_(ツ)_/¯
On Mon, 2014-04-07 at 09:35 +0100, Liviu Dudau wrote:
> Thanks for the summary, is really useful as I was recently looking
> into code in that
> area. One thing I was trying to understand is why ppc needs _IO_BASE
> at all rather
> than using the generic PCI_IOBASE?
Perhaps because our code predates it ? :-) I haven't looked much into
the semantics of PCI_IOBASE yet...
Cheers,
Ben.
On Monday 07 April 2014 19:13:28 Benjamin Herrenschmidt wrote:
> On Mon, 2014-04-07 at 09:35 +0100, Liviu Dudau wrote:
> > Thanks for the summary, is really useful as I was recently looking
> > into code in that
> > area. One thing I was trying to understand is why ppc needs _IO_BASE
> > at all rather
> > than using the generic PCI_IOBASE?
>
> Perhaps because our code predates it ? I haven't looked much into
> the semantics of PCI_IOBASE yet...
Yes, I'm pretty sure that's all there is to it. PCI_IOBASE just
happened to be an identifier we picked for asm-generic, but the
use on PowerPC is much older than the generic file.
Arnd
On Monday 07 April 2014 09:31:20 Liviu Dudau wrote:
> On Sat, Apr 05, 2014 at 01:19:53AM +0100, Bjorn Helgaas wrote:
>
> > Host bridges on x86 could have MMIO apertures that turn CPU memory accesses
> > into PCI port accesses. We could implement any number of I/O port spaces
> > this way, by making the kernel inb()/outb()/etc. interfaces smart enough to
> > use the memory-mapped space instead of (or in addition to) the
> > INB/OUTB/etc. instructions.
PowerPC actually has this already, as CONFIG_PPC_INDIRECT_PIO meaning that
access to PIO registers is bus specific, and there is also CONFIG_PPC_INDIRECT_MMIO
for the case where MMIO access is not native.
> Right, sorry for my ignorance then: how does *currently* the device driver do
> the I/O transfer transparent of the implementation mechanism? Or they have
> intimate knowledge of wether the device is behind a host bridge and can do MMIO
> or is on an ISA or CF bus and then it needs INB/OUTB ? And if we make inb/outb
> smarter, does that mean that we need to change the drivers?
The idea of that would be to not change drivers.
My preference here would be to only have a generic function for those
architectures that have the simple MMIO access all the time.
> > ia64 does this (see arch/ia64/include/asm/io.h for a little description)
> > and I think maybe one or two other arches have something similar.
> >
> > > Introduce a pci_register_io_range() helper function that can be used
> > > by the architecture code to keep track of the I/O ranges described by the
> > > PCI bindings. If the PCI_IOBASE macro is not defined that signals
> > > lack of support for PCI and we return an error.
> >
> > I don't quite see how you intend to use this, because this series doesn't
> > include any non-stub implementation of pci_register_io_range().
> >
> > Is this anything like the ia64 strategy I mentioned above? If so, it would
> > be really nice to unify some of this stuff.
>
> After discussions with Arnd and Catalin I know have a new series that moves
> some of the code from arm64 series into this one. I am putting it through
> testing right know as I am going to have to depend on another series that
> makes PCI_IOBASE defined only for architectures that do MMIO in order to
> choose the correct default implementation for these functions. My hope is
> that I will be able to send the series this week.
I think migrating other architectures to use the same code should be
a separate effort from adding a generic implementation that can be
used by arm64. It's probably a good idea to have patches to convert
arm32 and/or microblaze.
Arnd
On Mon, Apr 07, 2014 at 12:36:15PM +0100, Arnd Bergmann wrote:
> On Monday 07 April 2014 09:31:20 Liviu Dudau wrote:
> > On Sat, Apr 05, 2014 at 01:19:53AM +0100, Bjorn Helgaas wrote:
> >
> > > Host bridges on x86 could have MMIO apertures that turn CPU memory accesses
> > > into PCI port accesses. We could implement any number of I/O port spaces
> > > this way, by making the kernel inb()/outb()/etc. interfaces smart enough to
> > > use the memory-mapped space instead of (or in addition to) the
> > > INB/OUTB/etc. instructions.
>
> PowerPC actually has this already, as CONFIG_PPC_INDIRECT_PIO meaning that
> access to PIO registers is bus specific, and there is also CONFIG_PPC_INDIRECT_MMIO
> for the case where MMIO access is not native.
>
> > Right, sorry for my ignorance then: how does *currently* the device driver do
> > the I/O transfer transparent of the implementation mechanism? Or they have
> > intimate knowledge of wether the device is behind a host bridge and can do MMIO
> > or is on an ISA or CF bus and then it needs INB/OUTB ? And if we make inb/outb
> > smarter, does that mean that we need to change the drivers?
>
> The idea of that would be to not change drivers.
>
> My preference here would be to only have a generic function for those
> architectures that have the simple MMIO access all the time.
>
> > > ia64 does this (see arch/ia64/include/asm/io.h for a little description)
> > > and I think maybe one or two other arches have something similar.
> > >
> > > > Introduce a pci_register_io_range() helper function that can be used
> > > > by the architecture code to keep track of the I/O ranges described by the
> > > > PCI bindings. If the PCI_IOBASE macro is not defined that signals
> > > > lack of support for PCI and we return an error.
> > >
> > > I don't quite see how you intend to use this, because this series doesn't
> > > include any non-stub implementation of pci_register_io_range().
> > >
> > > Is this anything like the ia64 strategy I mentioned above? If so, it would
> > > be really nice to unify some of this stuff.
> >
> > After discussions with Arnd and Catalin I know have a new series that moves
> > some of the code from arm64 series into this one. I am putting it through
> > testing right know as I am going to have to depend on another series that
> > makes PCI_IOBASE defined only for architectures that do MMIO in order to
> > choose the correct default implementation for these functions. My hope is
> > that I will be able to send the series this week.
>
> I think migrating other architectures to use the same code should be
> a separate effort from adding a generic implementation that can be
> used by arm64. It's probably a good idea to have patches to convert
> arm32 and/or microblaze.
Agree. My updated series only moves the arm64 code into framework to make the
arm64 part a noop.
Liviu
>
> Arnd
>
>
--
====================
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
¯\_(ツ)_/¯
On Mon, Apr 7, 2014 at 5:36 AM, Arnd Bergmann <[email protected]> wrote:
> I think migrating other architectures to use the same code should be
> a separate effort from adding a generic implementation that can be
> used by arm64. It's probably a good idea to have patches to convert
> arm32 and/or microblaze.
Let me reiterate that I am 100% in favor of replacing arch-specific
code with more generic implementations.
However, I am not 100% in favor of doing it as separate efforts
(although maybe I could be convinced). The reasons I hesitate are
that (1) if only one architecture uses a new "generic" implementation,
we really don't know whether it is generic enough, (2) until I see the
patches to convert other architectures, I have to assume that I'm the
one who will write them, and (3) as soon as we add the code to
drivers/pci, it becomes partly my headache to maintain it, even if
only one arch benefits from it.
Please don't think I'm questioning anyone's intent or good will. It's
just that I understand the business pressures, and I know how hard it
can be to justify this sort of work to one's management, especially
after the immediate problem has been solved.
Bjorn
On Mon, Apr 07, 2014 at 06:58:24PM +0100, Bjorn Helgaas wrote:
> On Mon, Apr 7, 2014 at 5:36 AM, Arnd Bergmann <[email protected]> wrote:
>
> > I think migrating other architectures to use the same code should be
> > a separate effort from adding a generic implementation that can be
> > used by arm64. It's probably a good idea to have patches to convert
> > arm32 and/or microblaze.
>
> Let me reiterate that I am 100% in favor of replacing arch-specific
> code with more generic implementations.
>
> However, I am not 100% in favor of doing it as separate efforts
> (although maybe I could be convinced). The reasons I hesitate are
> that (1) if only one architecture uses a new "generic" implementation,
> we really don't know whether it is generic enough, (2) until I see the
> patches to convert other architectures, I have to assume that I'm the
> one who will write them, and (3) as soon as we add the code to
> drivers/pci, it becomes partly my headache to maintain it, even if
> only one arch benefits from it.
>
> Please don't think I'm questioning anyone's intent or good will. It's
> just that I understand the business pressures, and I know how hard it
> can be to justify this sort of work to one's management, especially
> after the immediate problem has been solved.
I understand your concern. I guess the only way to prove my good intentions
is to shut up and show the code.
Liviu
>
> Bjorn
>
--
====================
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
¯\_(ツ)_/¯
On Tuesday 08 April 2014 10:50:39 Liviu Dudau wrote:
> On Mon, Apr 07, 2014 at 06:58:24PM +0100, Bjorn Helgaas wrote:
> > On Mon, Apr 7, 2014 at 5:36 AM, Arnd Bergmann <[email protected]> wrote:
> >
> > > I think migrating other architectures to use the same code should be
> > > a separate effort from adding a generic implementation that can be
> > > used by arm64. It's probably a good idea to have patches to convert
> > > arm32 and/or microblaze.
> >
> > Let me reiterate that I am 100% in favor of replacing arch-specific
> > code with more generic implementations.
> >
> > However, I am not 100% in favor of doing it as separate efforts
> > (although maybe I could be convinced). The reasons I hesitate are
> > that (1) if only one architecture uses a new "generic" implementation,
> > we really don't know whether it is generic enough, (2) until I see the
> > patches to convert other architectures, I have to assume that I'm the
> > one who will write them, and (3) as soon as we add the code to
> > drivers/pci, it becomes partly my headache to maintain it, even if
> > only one arch benefits from it.
Fair enough.
My approach to the asm-generic infrastruction has mostly been to ensure
that whoever adds a new architecture has to make things easier for the
next person. For the PCI code it's clearly your call to pick whatever
works best for you.
> > Please don't think I'm questioning anyone's intent or good will. It's
> > just that I understand the business pressures, and I know how hard it
> > can be to justify this sort of work to one's management, especially
> > after the immediate problem has been solved.
>
> I understand your concern. I guess the only way to prove my good intentions
> is to shut up and show the code.
I'd suggest looking at architectures in this order then:
* microblaze (this one is easy and wants to share code with us)
* arm32-multiplatform (obviously interesting, but not as easy as microblaze)
* powerpc64 (they are keen on sharing, code is similar to what you have)
* mips (this is really platform specific, some want to share drivers with
arm32, others should keep their current code. Note that platform selection
on mips is compile-time only, they don't have to do it all the same way)
* powerpc32 (their code is currently different, might not be worth it)
My preference would be to have only the first two done initially and leave
the other ones up to architecture maintainers, but Bjorn should say
how much he wants to see get done.
Arnd
On Tue, Apr 8, 2014 at 4:22 AM, Arnd Bergmann <[email protected]> wrote:
> On Tuesday 08 April 2014 10:50:39 Liviu Dudau wrote:
>> On Mon, Apr 07, 2014 at 06:58:24PM +0100, Bjorn Helgaas wrote:
>> > On Mon, Apr 7, 2014 at 5:36 AM, Arnd Bergmann <[email protected]> wrote:
>> >
>> > > I think migrating other architectures to use the same code should be
>> > > a separate effort from adding a generic implementation that can be
>> > > used by arm64. It's probably a good idea to have patches to convert
>> > > arm32 and/or microblaze.
>> >
>> > Let me reiterate that I am 100% in favor of replacing arch-specific
>> > code with more generic implementations.
>> >
>> > However, I am not 100% in favor of doing it as separate efforts
>> > (although maybe I could be convinced). The reasons I hesitate are
>> > that (1) if only one architecture uses a new "generic" implementation,
>> > we really don't know whether it is generic enough, (2) until I see the
>> > patches to convert other architectures, I have to assume that I'm the
>> > one who will write them, and (3) as soon as we add the code to
>> > drivers/pci, it becomes partly my headache to maintain it, even if
>> > only one arch benefits from it.
>
> Fair enough.
>
> My approach to the asm-generic infrastruction has mostly been to ensure
> that whoever adds a new architecture has to make things easier for the
> next person.
That's a good rule. But if we add a generic implementation used only
by one architecture, the overall complexity has increased (we added
new unshared code), so the next person has to look at N+1 existing
implementations. If we even convert one existing arch, that seems
like an improvement: we have N implementations with one being used by
at least two arches.
Bjorn
(sorry for replying to a months old thread)
On Mon, Apr 07, 2014 at 06:58:24PM +0100, Bjorn Helgaas wrote:
> On Mon, Apr 7, 2014 at 5:36 AM, Arnd Bergmann <[email protected]> wrote:
>
> > I think migrating other architectures to use the same code should be
> > a separate effort from adding a generic implementation that can be
> > used by arm64. It's probably a good idea to have patches to convert
> > arm32 and/or microblaze.
>
> Let me reiterate that I am 100% in favor of replacing arch-specific
> code with more generic implementations.
>
> However, I am not 100% in favor of doing it as separate efforts
> (although maybe I could be convinced). The reasons I hesitate are
> that (1) if only one architecture uses a new "generic" implementation,
> we really don't know whether it is generic enough, (2) until I see the
> patches to convert other architectures, I have to assume that I'm the
> one who will write them, and (3) as soon as we add the code to
> drivers/pci, it becomes partly my headache to maintain it, even if
> only one arch benefits from it.
I agree and understand your point.
> Please don't think I'm questioning anyone's intent or good will. It's
> just that I understand the business pressures, and I know how hard it
> can be to justify this sort of work to one's management, especially
> after the immediate problem has been solved.
But, unfortunately, that's something we failed to address in reasonable
time (even though I was one of the proponents of the generic PCIe
implementation). This work is very likely to slip further into the late
part of this year and I am aware that several ARM partners are blocked
on the (upstream) availability of PCIe support for the arm64 kernel.
Although a bit late, I'm raising this now and hopefully we'll come to a
conclusion soon. Delaying arm64 PCIe support even further is not a real
option, which leaves us with:
1. Someone else (with enough PCIe knowledge) volunteering to take over
soon or
2. Dropping Liviu's work and going for an arm64-specific implementation
(most likely based on the arm32 implementation, see below)
First option is ideal but there is work to do as laid out by Arnd here:
http://article.gmane.org/gmane.linux.kernel/1679304
The latest patches from Liviu are here (they only target arm64 and there
are additional comments to be addressed from the above thread):
http://linux-arm.org/git?p=linux-ld.git;a=shortlog;h=refs/heads/for-upstream/pci-next
The main reason for the second option is timing. We could temporarily
move Liviu's code under arch/arm64 with the hope that we generalise it
later. However, the risk is even higher that once the code is in
mainline, the generic implementation won't happen. In which case, I
don't see much point in departing from the arm32 PCI API, making bios32
clone the best second option.
For the alternative implementation above, we already have a heavily cut
down version of the arm32 PCI support but only tested in a virtual
environment so far:
https://git.kernel.org/cgit/linux/kernel/git/will/linux.git/log/?h=pci/bios32
In conclusion, unless someone volunteers for the first option fairly
soon, we'll post the alternative patches for review and take it from
there.
Thanks.
--
Catalin
On Thu, Jun 26, 2014 at 09:59:26AM +0100, Catalin Marinas wrote:
> (sorry for replying to a months old thread)
>
> On Mon, Apr 07, 2014 at 06:58:24PM +0100, Bjorn Helgaas wrote:
> > On Mon, Apr 7, 2014 at 5:36 AM, Arnd Bergmann <[email protected]> wrote:
> >
> > > I think migrating other architectures to use the same code should be
> > > a separate effort from adding a generic implementation that can be
> > > used by arm64. It's probably a good idea to have patches to convert
> > > arm32 and/or microblaze.
> >
> > Let me reiterate that I am 100% in favor of replacing arch-specific
> > code with more generic implementations.
> >
> > However, I am not 100% in favor of doing it as separate efforts
> > (although maybe I could be convinced). The reasons I hesitate are
> > that (1) if only one architecture uses a new "generic" implementation,
> > we really don't know whether it is generic enough, (2) until I see the
> > patches to convert other architectures, I have to assume that I'm the
> > one who will write them, and (3) as soon as we add the code to
> > drivers/pci, it becomes partly my headache to maintain it, even if
> > only one arch benefits from it.
>
> I agree and understand your point.
>
> > Please don't think I'm questioning anyone's intent or good will. It's
> > just that I understand the business pressures, and I know how hard it
> > can be to justify this sort of work to one's management, especially
> > after the immediate problem has been solved.
>
> But, unfortunately, that's something we failed to address in reasonable
> time (even though I was one of the proponents of the generic PCIe
> implementation). This work is very likely to slip further into the late
> part of this year and I am aware that several ARM partners are blocked
> on the (upstream) availability of PCIe support for the arm64 kernel.
>
> Although a bit late, I'm raising this now and hopefully we'll come to a
> conclusion soon. Delaying arm64 PCIe support even further is not a real
> option, which leaves us with:
>
> 1. Someone else (with enough PCIe knowledge) volunteering to take over
> soon or
> 2. Dropping Liviu's work and going for an arm64-specific implementation
> (most likely based on the arm32 implementation, see below)
>
> First option is ideal but there is work to do as laid out by Arnd here:
>
> http://article.gmane.org/gmane.linux.kernel/1679304
>
> The latest patches from Liviu are here (they only target arm64 and there
> are additional comments to be addressed from the above thread):
>
> http://linux-arm.org/git?p=linux-ld.git;a=shortlog;h=refs/heads/for-upstream/pci-next
>
> The main reason for the second option is timing. We could temporarily
> move Liviu's code under arch/arm64 with the hope that we generalise it
> later. However, the risk is even higher that once the code is in
> mainline, the generic implementation won't happen. In which case, I
> don't see much point in departing from the arm32 PCI API, making bios32
> clone the best second option.
>
> For the alternative implementation above, we already have a heavily cut
> down version of the arm32 PCI support but only tested in a virtual
> environment so far:
>
> https://git.kernel.org/cgit/linux/kernel/git/will/linux.git/log/?h=pci/bios32
>
> In conclusion, unless someone volunteers for the first option fairly
> soon, we'll post the alternative patches for review and take it from
> there.
That would be a huge step backwards IMO and a huge dissapointment. If
you go with the alternative patches from Will you will basically reset
every partner's implementation that has been built on top of my
patches (when they did so with the understanding that my series will be
the one ARM will support and publish) *and* make anyone's attempt to
create a generic implementation harder, as they will have to undo this
code to remove the arch-specific parts.
While I have part of a personal blame to carry as I have dragged some
of the work for too long, there are things that I could not have done
differently due to internal pressure inside the project I work on. It
is my intent to resume work on it as soon as possible, but life is
such at the moment that I have to dedicate my time to other things.
Best regards,
Liviu
>
> Thanks.
>
> --
> Catalin
--
====================
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
¯\_(ツ)_/¯
On Thu, Jun 26, 2014 at 10:30:29AM +0100, Liviu Dudau wrote:
> On Thu, Jun 26, 2014 at 09:59:26AM +0100, Catalin Marinas wrote:
> > Although a bit late, I'm raising this now and hopefully we'll come to a
> > conclusion soon. Delaying arm64 PCIe support even further is not a real
> > option, which leaves us with:
> >
> > 1. Someone else (with enough PCIe knowledge) volunteering to take over
> > soon or
> > 2. Dropping Liviu's work and going for an arm64-specific implementation
> > (most likely based on the arm32 implementation, see below)
[...]
> > In conclusion, unless someone volunteers for the first option fairly
> > soon, we'll post the alternative patches for review and take it from
> > there.
>
> That would be a huge step backwards IMO and a huge dissapointment. If
> you go with the alternative patches from Will you will basically reset
> every partner's implementation that has been built on top of my
> patches (when they did so with the understanding that my series will be
> the one ARM will support and publish) *and* make anyone's attempt to
> create a generic implementation harder, as they will have to undo this
> code to remove the arch-specific parts.
I fully agree and the alternative patchset is definitely _not_ my
preferred solution. You can read this email as a request for help to
complete the work (whether it comes from ARM, Linaro or other interested
parties). I don't mean taking over the whole patchset but potentially
helping with other arch conversion (microblaze, arm multi-platform).
(however, if the generic PCIe work won't happen in reasonable time, we
need to set some deadline rather than keeping the patchset out of tree
indefinitely)
--
Catalin
On Thu, Jun 26, 2014 at 03:11:38PM +0100, Catalin Marinas wrote:
> On Thu, Jun 26, 2014 at 10:30:29AM +0100, Liviu Dudau wrote:
> > On Thu, Jun 26, 2014 at 09:59:26AM +0100, Catalin Marinas wrote:
> > > Although a bit late, I'm raising this now and hopefully we'll come to a
> > > conclusion soon. Delaying arm64 PCIe support even further is not a real
> > > option, which leaves us with:
> > >
> > > 1. Someone else (with enough PCIe knowledge) volunteering to take over
> > > soon or
> > > 2. Dropping Liviu's work and going for an arm64-specific implementation
> > > (most likely based on the arm32 implementation, see below)
> [...]
> > > In conclusion, unless someone volunteers for the first option fairly
> > > soon, we'll post the alternative patches for review and take it from
> > > there.
> >
> > That would be a huge step backwards IMO and a huge dissapointment. If
> > you go with the alternative patches from Will you will basically reset
> > every partner's implementation that has been built on top of my
> > patches (when they did so with the understanding that my series will be
> > the one ARM will support and publish) *and* make anyone's attempt to
> > create a generic implementation harder, as they will have to undo this
> > code to remove the arch-specific parts.
>
> I fully agree and the alternative patchset is definitely _not_ my
> preferred solution. You can read this email as a request for help to
> complete the work (whether it comes from ARM, Linaro or other interested
> parties). I don't mean taking over the whole patchset but potentially
> helping with other arch conversion (microblaze, arm multi-platform).
I feel it's also worth pointing out that I didn't write that code with the
intention of getting it merged, nor as a competing solution to what Liviu
was proposing at the time. It was merely a development tool to enable some
of the SMMU and GICv3 work that Marc and I have been working on recently.
Will
Adding Michael Simek...
On Thu, Jun 26, 2014 at 3:59 AM, Catalin Marinas
<[email protected]> wrote:
> (sorry for replying to a months old thread)
>
> On Mon, Apr 07, 2014 at 06:58:24PM +0100, Bjorn Helgaas wrote:
>> On Mon, Apr 7, 2014 at 5:36 AM, Arnd Bergmann <[email protected]> wrote:
>>
>> > I think migrating other architectures to use the same code should be
>> > a separate effort from adding a generic implementation that can be
>> > used by arm64. It's probably a good idea to have patches to convert
>> > arm32 and/or microblaze.
>>
>> Let me reiterate that I am 100% in favor of replacing arch-specific
>> code with more generic implementations.
>>
>> However, I am not 100% in favor of doing it as separate efforts
>> (although maybe I could be convinced). The reasons I hesitate are
>> that (1) if only one architecture uses a new "generic" implementation,
>> we really don't know whether it is generic enough, (2) until I see the
>> patches to convert other architectures, I have to assume that I'm the
>> one who will write them, and (3) as soon as we add the code to
>> drivers/pci, it becomes partly my headache to maintain it, even if
>> only one arch benefits from it.
>
> I agree and understand your point.
>
>> Please don't think I'm questioning anyone's intent or good will. It's
>> just that I understand the business pressures, and I know how hard it
>> can be to justify this sort of work to one's management, especially
>> after the immediate problem has been solved.
>
> But, unfortunately, that's something we failed to address in reasonable
> time (even though I was one of the proponents of the generic PCIe
> implementation). This work is very likely to slip further into the late
> part of this year and I am aware that several ARM partners are blocked
> on the (upstream) availability of PCIe support for the arm64 kernel.
>
> Although a bit late, I'm raising this now and hopefully we'll come to a
> conclusion soon. Delaying arm64 PCIe support even further is not a real
> option, which leaves us with:
>
> 1. Someone else (with enough PCIe knowledge) volunteering to take over
> soon or
Well, I might have 2 months ago, but now I'm pretty booked up.
> 2. Dropping Liviu's work and going for an arm64-specific implementation
> (most likely based on the arm32 implementation, see below)
3. Keeping Liviu's patches leaving some of the architecture specific
bits. I know Arnd and I both commented on it still needing more common
pieces, but compared to option 2 that would be way better.
Let's look at the patches in question:
3e71867 pci: Introduce pci_register_io_range() helper function.
6681dff pci: OF: Fix the conversion of IO ranges into IO resources.
Both OF patches. I'll happily merge them.
2d5dd85 pci: Create pci_host_bridge before its associated bus in
pci_create_root_bus.
f6f2854 pci: Introduce a domain number for pci_host_bridge.
524a9f5 pci: Export find_pci_host_bridge() function.
These don't seem to be too controversial.
fb75718 pci: of: Parse and map the IRQ when adding the PCI device.
6 LOC. Hardly controversial.
920a685 pci: Add support for creating a generic host_bridge from device tree
This function could be moved to drivers/of/of_pci.c if having it in
drivers/pci is too much maintenance burden. However, nearly the same
code is already being duplicated in every DT enabled ARM PCI host
driver and will continue as more PCI hosts are added. So this isn't
really a question of converting other architectures to common PCI host
infrastructure, but converting DT based PCI hosts to common
infrastructure. ARM is the only arch moving host drivers to
drivers/pci ATM. Until other architectures start doing that,
converting them is pointless.
bcf5c10 Fix ioport_map() for !CONFIG_GENERIC_IOMAP cases.
Seems like an independent fix that should be applied regardless.
7cfde80 arm64: Add architecture support for PCI
What is here is really just a function of which option we pick.
> First option is ideal but there is work to do as laid out by Arnd here:
>
> http://article.gmane.org/gmane.linux.kernel/1679304
I don't agree arm32 is harder than microblaze. Yes, converting ALL of
arm would be, but that is not necessary. With Liviu's latest branch
the hacks I previously needed are gone (thanks!), and this is all I
need to get Versatile PCI working (under QEMU):
diff --git a/arch/arm/include/asm/io.h b/arch/arm/include/asm/io.h
index 3d23418..22b7529 100644
--- a/arch/arm/include/asm/io.h
+++ b/arch/arm/include/asm/io.h
@@ -178,6 +178,7 @@ static inline void __iomem *__typesafe_io(unsigned
long addr)
/* PCI fixed i/o mapping */
#define PCI_IO_VIRT_BASE 0xfee00000
+#define PCI_IOBASE PCI_IO_VIRT_BASE
#if defined(CONFIG_PCI)
void pci_ioremap_set_mem_type(int mem_type);
Hum, so I guess now I've converted ARM...
Here's a branch with my changes[1]. And BTW, it also has
multi-platform support for Versatile as moving the PCI host to DT (and
drivers/pci/host) is about the last remaining obstacle.
Rob
[1] git://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git
versatile-pci-v2
On Thursday 26 June 2014 19:44:21 Rob Herring wrote:
> > Although a bit late, I'm raising this now and hopefully we'll come to a
> > conclusion soon. Delaying arm64 PCIe support even further is not a real
> > option, which leaves us with:
> >
> > 1. Someone else (with enough PCIe knowledge) volunteering to take over
> > soon or
>
> Well, I might have 2 months ago, but now I'm pretty booked up.
>
> > 2. Dropping Liviu's work and going for an arm64-specific implementation
> > (most likely based on the arm32 implementation, see below)
>
> 3. Keeping Liviu's patches leaving some of the architecture specific
> bits. I know Arnd and I both commented on it still needing more common
> pieces, but compared to option 2 that would be way better.
Agreed.
> 920a685 pci: Add support for creating a generic host_bridge from device tree
>
> This function could be moved to drivers/of/of_pci.c if having it in
> drivers/pci is too much maintenance burden. However, nearly the same
> code is already being duplicated in every DT enabled ARM PCI host
> driver and will continue as more PCI hosts are added. So this isn't
> really a question of converting other architectures to common PCI host
> infrastructure, but converting DT based PCI hosts to common
> infrastructure. ARM is the only arch moving host drivers to
> drivers/pci ATM. Until other architectures start doing that,
> converting them is pointless.
This is the most important part in my mind. Every implementation gets
this code wrong at least initially, and we have to provide some generic
helper to get the broken code out of host drivers. I don't care whether
that helper is part of the PCI core or part of the OF core.
> 7cfde80 arm64: Add architecture support for PCI
>
> What is here is really just a function of which option we pick.
>
>
> > First option is ideal but there is work to do as laid out by Arnd here:
> >
> > http://article.gmane.org/gmane.linux.kernel/1679304
>
> I don't agree arm32 is harder than microblaze. Yes, converting ALL of
> arm would be, but that is not necessary. With Liviu's latest branch
> the hacks I previously needed are gone (thanks!), and this is all I
> need to get Versatile PCI working (under QEMU):
I meant converting all of arm32 would be harder, but I agree we don't
have to do it. It would be nice to convert all of the drivers/pci/host
drivers though, iow all multiplatform-enabled ones, and leaving the
current arm32 pci implementation for the platforms we don't want to
convert to multiplatform anyway (footbridge, iop, ixp4xx, ks8695 (?),
pxa, sa1100).
Arnd
On Fri, Jun 27, 2014 at 12:03:34PM +0100, Arnd Bergmann wrote:
> On Thursday 26 June 2014 19:44:21 Rob Herring wrote:
> > I don't agree arm32 is harder than microblaze. Yes, converting ALL of
> > arm would be, but that is not necessary. With Liviu's latest branch
> > the hacks I previously needed are gone (thanks!), and this is all I
> > need to get Versatile PCI working (under QEMU):
>
> I meant converting all of arm32 would be harder, but I agree we don't
> have to do it. It would be nice to convert all of the drivers/pci/host
> drivers though, iow all multiplatform-enabled ones, and leaving the
> current arm32 pci implementation for the platforms we don't want to
> convert to multiplatform anyway (footbridge, iop, ixp4xx, ks8695 (?),
> pxa, sa1100).
I'm more than happy to convert the generic host controller we merged
recently, but I'd probably want the core changes merged first so that I
know I'm not wasting my time!
Will
On Friday 27 June 2014 13:49:49 Will Deacon wrote:
> On Fri, Jun 27, 2014 at 12:03:34PM +0100, Arnd Bergmann wrote:
> > On Thursday 26 June 2014 19:44:21 Rob Herring wrote:
> > > I don't agree arm32 is harder than microblaze. Yes, converting ALL of
> > > arm would be, but that is not necessary. With Liviu's latest branch
> > > the hacks I previously needed are gone (thanks!), and this is all I
> > > need to get Versatile PCI working (under QEMU):
> >
> > I meant converting all of arm32 would be harder, but I agree we don't
> > have to do it. It would be nice to convert all of the drivers/pci/host
> > drivers though, iow all multiplatform-enabled ones, and leaving the
> > current arm32 pci implementation for the platforms we don't want to
> > convert to multiplatform anyway (footbridge, iop, ixp4xx, ks8695 (?),
> > pxa, sa1100).
>
> I'm more than happy to convert the generic host controller we merged
> recently, but I'd probably want the core changes merged first so that I
> know I'm not wasting my time!
That is definitely fine with me, but it's Bjorn's decision.
Arnd
On Fri, Jun 27, 2014 at 02:16:28PM +0100, Arnd Bergmann wrote:
> On Friday 27 June 2014 13:49:49 Will Deacon wrote:
> > On Fri, Jun 27, 2014 at 12:03:34PM +0100, Arnd Bergmann wrote:
> > > On Thursday 26 June 2014 19:44:21 Rob Herring wrote:
> > > > I don't agree arm32 is harder than microblaze. Yes, converting ALL of
> > > > arm would be, but that is not necessary. With Liviu's latest branch
> > > > the hacks I previously needed are gone (thanks!), and this is all I
> > > > need to get Versatile PCI working (under QEMU):
> > >
> > > I meant converting all of arm32 would be harder, but I agree we don't
> > > have to do it. It would be nice to convert all of the drivers/pci/host
> > > drivers though, iow all multiplatform-enabled ones, and leaving the
> > > current arm32 pci implementation for the platforms we don't want to
> > > convert to multiplatform anyway (footbridge, iop, ixp4xx, ks8695 (?),
> > > pxa, sa1100).
> >
> > I'm more than happy to convert the generic host controller we merged
> > recently, but I'd probably want the core changes merged first so that I
> > know I'm not wasting my time!
>
> That is definitely fine with me, but it's Bjorn's decision.
Or the changes to the generic host controller can be part of Liviu's
patch series (maybe together with other PCI host drivers like
designware, as time allows).
--
Catalin
On Fri, Jun 27, 2014 at 01:44:21AM +0100, Rob Herring wrote:
> On Thu, Jun 26, 2014 at 3:59 AM, Catalin Marinas
> <[email protected]> wrote:
> > Although a bit late, I'm raising this now and hopefully we'll come to a
> > conclusion soon. Delaying arm64 PCIe support even further is not a real
> > option, which leaves us with:
> >
> > 1. Someone else (with enough PCIe knowledge) volunteering to take over
> > soon or
> > 2. Dropping Liviu's work and going for an arm64-specific implementation
> > (most likely based on the arm32 implementation, see below)
>
> 3. Keeping Liviu's patches leaving some of the architecture specific
> bits. I know Arnd and I both commented on it still needing more common
> pieces, but compared to option 2 that would be way better.
>
> Let's look at the patches in question:
>
> 3e71867 pci: Introduce pci_register_io_range() helper function.
> 6681dff pci: OF: Fix the conversion of IO ranges into IO resources.
>
> Both OF patches. I'll happily merge them.
We just need to make sure they don't break other users of
of_pci_range_to_resource() that the second patch introduces.
> 2d5dd85 pci: Create pci_host_bridge before its associated bus in
> pci_create_root_bus.
> f6f2854 pci: Introduce a domain number for pci_host_bridge.
> 524a9f5 pci: Export find_pci_host_bridge() function.
>
> These don't seem to be too controversial.
I think here there were discussions around introducing domain_nr to
pci_host_bridge, particularly to the pci_create_root_bus_in_domain() API
change. I don't think we reached any conclusion.
> fb75718 pci: of: Parse and map the IRQ when adding the PCI device.
>
> 6 LOC. Hardly controversial.
I agree.
> 920a685 pci: Add support for creating a generic host_bridge from device tree
>
> This function could be moved to drivers/of/of_pci.c if having it in
> drivers/pci is too much maintenance burden.
I think it makes sense. Currently drivers/pci/host-bridge.c doesn't have
anything OF related, so of_pci.c looks more appropriate.
> However, nearly the same
> code is already being duplicated in every DT enabled ARM PCI host
> driver and will continue as more PCI hosts are added. So this isn't
> really a question of converting other architectures to common PCI host
> infrastructure, but converting DT based PCI hosts to common
> infrastructure. ARM is the only arch moving host drivers to
> drivers/pci ATM. Until other architectures start doing that,
> converting them is pointless.
I agree. It's probably more important to convert some of the
drivers/pci/host implementations to using the common parsing rather than
a new architecture (this way we avoid even more code duplication).
> bcf5c10 Fix ioport_map() for !CONFIG_GENERIC_IOMAP cases.
>
> Seems like an independent fix that should be applied regardless.
Indeed. But it got stuck at the top of this series and hasn't been
pushed upstream.
> 7cfde80 arm64: Add architecture support for PCI
>
> What is here is really just a function of which option we pick.
With Liviu's latest version (not posted) and with
of_create_pci_host_bridge() function moved to of_pci.c, I don't think
there is much new functionality added to drivers/pci/. What I think we
need is clarifying the domain_nr patch (and API change) and more users
of the new generic code. As you said, it doesn't need to be a separate
architecture but rather existing pci host drivers under drivers/pci. Of
course, other arch conversion should follow shortly as well but even
without an immediate conversion, I don't see too much additional
maintenance burden for the core PCIe code (and code sharing between new
PCIe host drivers is even more beneficial).
--
Catalin
On Fri, Jun 27, 2014 at 8:14 AM, Catalin Marinas
<[email protected]> wrote:
> ...
> With Liviu's latest version (not posted) and with
> of_create_pci_host_bridge() function moved to of_pci.c, I don't think
> there is much new functionality added to drivers/pci/. What I think we
> need is clarifying the domain_nr patch (and API change) and more users
> of the new generic code. As you said, it doesn't need to be a separate
> architecture but rather existing pci host drivers under drivers/pci. Of
> course, other arch conversion should follow shortly as well but even
> without an immediate conversion, I don't see too much additional
> maintenance burden for the core PCIe code (and code sharing between new
> PCIe host drivers is even more beneficial).
Sorry, I haven't had time to follow this. It sounds like there are
several pieces we could get out of the way easily. How about posting
the actual patches again? Maybe re-order them so the easy pieces are
first so they can get applied even if there are issues with later
ones?
Bjorn
On Fri, Jun 27, 2014 at 03:55:04PM +0100, Bjorn Helgaas wrote:
> On Fri, Jun 27, 2014 at 8:14 AM, Catalin Marinas
> <[email protected]> wrote:
> > ...
> > With Liviu's latest version (not posted) and with
> > of_create_pci_host_bridge() function moved to of_pci.c, I don't think
> > there is much new functionality added to drivers/pci/. What I think we
> > need is clarifying the domain_nr patch (and API change) and more users
> > of the new generic code. As you said, it doesn't need to be a separate
> > architecture but rather existing pci host drivers under drivers/pci. Of
> > course, other arch conversion should follow shortly as well but even
> > without an immediate conversion, I don't see too much additional
> > maintenance burden for the core PCIe code (and code sharing between new
> > PCIe host drivers is even more beneficial).
>
> Sorry, I haven't had time to follow this. It sounds like there are
> several pieces we could get out of the way easily. How about posting
> the actual patches again? Maybe re-order them so the easy pieces are
> first so they can get applied even if there are issues with later
> ones?
OK, I will post a new series on Monday.
Thanks,
Liviu
>
> Bjorn
>
--
====================
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
¯\_(ツ)_/¯
On 06/27/2014 07:49 AM, Will Deacon wrote:
> On Fri, Jun 27, 2014 at 12:03:34PM +0100, Arnd Bergmann wrote:
>> On Thursday 26 June 2014 19:44:21 Rob Herring wrote:
>>> I don't agree arm32 is harder than microblaze. Yes, converting ALL of
>>> arm would be, but that is not necessary. With Liviu's latest branch
>>> the hacks I previously needed are gone (thanks!), and this is all I
>>> need to get Versatile PCI working (under QEMU):
>>
>> I meant converting all of arm32 would be harder, but I agree we don't
>> have to do it. It would be nice to convert all of the drivers/pci/host
>> drivers though, iow all multiplatform-enabled ones, and leaving the
>> current arm32 pci implementation for the platforms we don't want to
>> convert to multiplatform anyway (footbridge, iop, ixp4xx, ks8695 (?),
>> pxa, sa1100).
>
> I'm more than happy to convert the generic host controller we merged
> recently, but I'd probably want the core changes merged first so that I
> know I'm not wasting my time!
Something like this untested patch...
Another issue I found still present is pci_ioremap_io needs some work
to unify with the arm implementation. That's a matter of changing the
function from an offset to i/o resource. That should be a pretty
mechanical change.
Also, there is a potential for memory leak because there is no undo
for of_create_pci_host_bridge.
Rob
8<------------------------------------------------------------------
>From 301b402631b2867ced38d5533586da0bd888045c Mon Sep 17 00:00:00 2001
From: Rob Herring <[email protected]>
Date: Fri, 27 Jun 2014 09:46:09 -0500
Subject: [PATCH] pci: generic-host: refactor to use common range parsing
Signed-off-by: Rob Herring <[email protected]>
---
drivers/pci/host/pci-host-generic.c | 191 ++++++------------------------------
1 file changed, 29 insertions(+), 162 deletions(-)
diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c
index 44fe6aa..57fd1e1 100644
--- a/drivers/pci/host/pci-host-generic.c
+++ b/drivers/pci/host/pci-host-generic.c
@@ -32,16 +32,14 @@ struct gen_pci_cfg_bus_ops {
struct gen_pci_cfg_windows {
struct resource res;
- struct resource bus_range;
void __iomem **win;
const struct gen_pci_cfg_bus_ops *ops;
};
struct gen_pci {
- struct pci_host_bridge host;
+ struct pci_host_bridge *host;
struct gen_pci_cfg_windows cfg;
- struct list_head resources;
};
static void __iomem *gen_pci_map_cfg_bus_cam(struct pci_bus *bus,
@@ -50,7 +48,7 @@ static void __iomem *gen_pci_map_cfg_bus_cam(struct pci_bus *bus,
{
struct pci_sys_data *sys = bus->sysdata;
struct gen_pci *pci = sys->private_data;
- resource_size_t idx = bus->number - pci->cfg.bus_range.start;
+ resource_size_t idx = bus->number - bus->busn_res.start;
return pci->cfg.win[idx] + ((devfn << 8) | where);
}
@@ -66,7 +64,7 @@ static void __iomem *gen_pci_map_cfg_bus_ecam(struct pci_bus *bus,
{
struct pci_sys_data *sys = bus->sysdata;
struct gen_pci *pci = sys->private_data;
- resource_size_t idx = bus->number - pci->cfg.bus_range.start;
+ resource_size_t idx = bus->number - bus->busn_res.start;
return pci->cfg.win[idx] + ((devfn << 12) | where);
}
@@ -138,154 +136,32 @@ static const struct of_device_id gen_pci_of_match[] = {
};
MODULE_DEVICE_TABLE(of, gen_pci_of_match);
-static int gen_pci_calc_io_offset(struct device *dev,
- struct of_pci_range *range,
- struct resource *res,
- resource_size_t *offset)
-{
- static atomic_t wins = ATOMIC_INIT(0);
- int err, idx, max_win;
- unsigned int window;
-
- if (!PAGE_ALIGNED(range->cpu_addr))
- return -EINVAL;
-
- max_win = (IO_SPACE_LIMIT + 1) / SZ_64K;
- idx = atomic_inc_return(&wins);
- if (idx > max_win)
- return -ENOSPC;
-
- window = (idx - 1) * SZ_64K;
- err = pci_ioremap_io(window, range->cpu_addr);
- if (err)
- return err;
-
- of_pci_range_to_resource(range, dev->of_node, res);
- res->start = window;
- res->end = res->start + range->size - 1;
- *offset = window - range->pci_addr;
- return 0;
-}
-
-static int gen_pci_calc_mem_offset(struct device *dev,
- struct of_pci_range *range,
- struct resource *res,
- resource_size_t *offset)
-{
- of_pci_range_to_resource(range, dev->of_node, res);
- *offset = range->cpu_addr - range->pci_addr;
- return 0;
-}
-
-static void gen_pci_release_of_pci_ranges(struct gen_pci *pci)
-{
- struct pci_host_bridge_window *win;
-
- list_for_each_entry(win, &pci->resources, list)
- release_resource(win->res);
-
- pci_free_resource_list(&pci->resources);
-}
-
-static int gen_pci_parse_request_of_pci_ranges(struct gen_pci *pci)
-{
- struct of_pci_range range;
- struct of_pci_range_parser parser;
- int err, res_valid = 0;
- struct device *dev = pci->host.dev.parent;
- struct device_node *np = dev->of_node;
-
- if (of_pci_range_parser_init(&parser, np)) {
- dev_err(dev, "missing \"ranges\" property\n");
- return -EINVAL;
- }
-
- for_each_of_pci_range(&parser, &range) {
- struct resource *parent, *res;
- resource_size_t offset;
- u32 restype = range.flags & IORESOURCE_TYPE_BITS;
-
- res = devm_kmalloc(dev, sizeof(*res), GFP_KERNEL);
- if (!res) {
- err = -ENOMEM;
- goto out_release_res;
- }
-
- switch (restype) {
- case IORESOURCE_IO:
- parent = &ioport_resource;
- err = gen_pci_calc_io_offset(dev, &range, res, &offset);
- break;
- case IORESOURCE_MEM:
- parent = &iomem_resource;
- err = gen_pci_calc_mem_offset(dev, &range, res, &offset);
- res_valid |= !(res->flags & IORESOURCE_PREFETCH || err);
- break;
- default:
- err = -EINVAL;
- continue;
- }
-
- if (err) {
- dev_warn(dev,
- "error %d: failed to add resource [type 0x%x, %lld bytes]\n",
- err, restype, range.size);
- continue;
- }
-
- err = request_resource(parent, res);
- if (err)
- goto out_release_res;
-
- pci_add_resource_offset(&pci->resources, res, offset);
- }
-
- if (!res_valid) {
- dev_err(dev, "non-prefetchable memory resource required\n");
- err = -EINVAL;
- goto out_release_res;
- }
-
- return 0;
-
-out_release_res:
- gen_pci_release_of_pci_ranges(pci);
- return err;
-}
-
static int gen_pci_parse_map_cfg_windows(struct gen_pci *pci)
{
int err;
u8 bus_max;
resource_size_t busn;
- struct resource *bus_range;
- struct device *dev = pci->host.dev.parent;
+ struct pci_bus *bus = pci->host->bus;
+ struct resource *bus_range = &bus->busn_res;
+ struct device *dev = pci->host->dev.parent;
struct device_node *np = dev->of_node;
- if (of_pci_parse_bus_range(np, &pci->cfg.bus_range))
- pci->cfg.bus_range = (struct resource) {
- .name = np->name,
- .start = 0,
- .end = 0xff,
- .flags = IORESOURCE_BUS,
- };
-
err = of_address_to_resource(np, 0, &pci->cfg.res);
if (err) {
dev_err(dev, "missing \"reg\" property\n");
return err;
}
- pci->cfg.win = devm_kcalloc(dev, resource_size(&pci->cfg.bus_range),
+ pci->cfg.win = devm_kcalloc(dev, resource_size(bus_range),
sizeof(*pci->cfg.win), GFP_KERNEL);
if (!pci->cfg.win)
return -ENOMEM;
/* Limit the bus-range to fit within reg */
- bus_max = pci->cfg.bus_range.start +
+ bus_max = bus_range->start +
(resource_size(&pci->cfg.res) >> pci->cfg.ops->bus_shift) - 1;
- pci->cfg.bus_range.end = min_t(resource_size_t, pci->cfg.bus_range.end,
- bus_max);
+ pci_bus_update_busn_res_end(bus, min_t(resource_size_t,
+ bus_range->end, bus_max));
/* Map our Configuration Space windows */
if (!devm_request_mem_region(dev, pci->cfg.res.start,
@@ -293,7 +169,6 @@ static int gen_pci_parse_map_cfg_windows(struct gen_pci *pci)
"Configuration Space"))
return -ENOMEM;
- bus_range = &pci->cfg.bus_range;
for (busn = bus_range->start; busn <= bus_range->end; ++busn) {
u32 idx = busn - bus_range->start;
u32 sz = 1 << pci->cfg.ops->bus_shift;
@@ -305,34 +180,21 @@ static int gen_pci_parse_map_cfg_windows(struct gen_pci *pci)
return -ENOMEM;
}
- /* Register bus resource */
- pci_add_resource(&pci->resources, bus_range);
return 0;
}
-static int gen_pci_setup(int nr, struct pci_sys_data *sys)
-{
- struct gen_pci *pci = sys->private_data;
- list_splice_init(&pci->resources, &sys->resources);
- return 1;
-}
+/* Unused, temporary to satisfy ARM arch code */
+static struct pci_sys_data sys;
static int gen_pci_probe(struct platform_device *pdev)
{
- int err;
+ int err, lastbus;
const char *type;
const struct of_device_id *of_id;
const int *prop;
struct device *dev = &pdev->dev;
struct device_node *np = dev->of_node;
struct gen_pci *pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL);
- struct hw_pci hw = {
- .nr_controllers = 1,
- .private_data = (void **)&pci,
- .setup = gen_pci_setup,
- .map_irq = of_irq_parse_and_map_pci,
- .ops = &gen_pci_ops,
- };
if (!pci)
return -ENOMEM;
@@ -353,23 +215,28 @@ static int gen_pci_probe(struct platform_device *pdev)
of_id = of_match_node(gen_pci_of_match, np);
pci->cfg.ops = of_id->data;
- pci->host.dev.parent = dev;
- INIT_LIST_HEAD(&pci->host.windows);
- INIT_LIST_HEAD(&pci->resources);
- /* Parse our PCI ranges and request their resources */
- err = gen_pci_parse_request_of_pci_ranges(pci);
- if (err)
- return err;
+ pci->host = of_create_pci_host_bridge(&pdev->dev, &gen_pci_ops, &sys);
+ if (PTR_ERR(pci->host))
+ return PTR_ERR(pci->host);
/* Parse and map our Configuration Space windows */
err = gen_pci_parse_map_cfg_windows(pci);
- if (err) {
- gen_pci_release_of_pci_ranges(pci);
+ if (err)
return err;
- }
- pci_common_init_dev(dev, &hw);
+ pci_ioremap_io(0, pci->host->io_base);
+
+ pci_add_flags(PCI_ENABLE_PROC_DOMAINS);
+ pci_add_flags(PCI_REASSIGN_ALL_BUS | PCI_REASSIGN_ALL_RSRC);
+
+ lastbus = pci_scan_child_bus(pci->host->bus);
+ pci_bus_update_busn_res_end(pci->host->bus, lastbus);
+
+ pci_assign_unassigned_bus_resources(pci->host->bus);
+
+ pci_bus_add_devices(pci->host->bus);
+
return 0;
}
--
1.9.1
Hi Rob,
Nice work!
On Fri, Jun 27, 2014 at 05:15:53PM +0100, Rob Herring wrote:
> On 06/27/2014 07:49 AM, Will Deacon wrote:
> > On Fri, Jun 27, 2014 at 12:03:34PM +0100, Arnd Bergmann wrote:
> >> On Thursday 26 June 2014 19:44:21 Rob Herring wrote:
> >>> I don't agree arm32 is harder than microblaze. Yes, converting ALL of
> >>> arm would be, but that is not necessary. With Liviu's latest branch
> >>> the hacks I previously needed are gone (thanks!), and this is all I
> >>> need to get Versatile PCI working (under QEMU):
> >>
> >> I meant converting all of arm32 would be harder, but I agree we don't
> >> have to do it. It would be nice to convert all of the drivers/pci/host
> >> drivers though, iow all multiplatform-enabled ones, and leaving the
> >> current arm32 pci implementation for the platforms we don't want to
> >> convert to multiplatform anyway (footbridge, iop, ixp4xx, ks8695 (?),
> >> pxa, sa1100).
> >
> > I'm more than happy to convert the generic host controller we merged
> > recently, but I'd probably want the core changes merged first so that I
> > know I'm not wasting my time!
>
> Something like this untested patch...
>
> Another issue I found still present is pci_ioremap_io needs some work
> to unify with the arm implementation. That's a matter of changing the
> function from an offset to i/o resource. That should be a pretty
> mechanical change.
>
> Also, there is a potential for memory leak because there is no undo
> for of_create_pci_host_bridge.
Once Liviu reposts his series, I'll take a look at
of_create_pci_host_bridge, as I'm not sure that it provides all the
behaviour that we get from gen_pci_parse_request_of_pci_ranges right now
(e.g. required a non-prefetchable mem resource).
Will