diff mbox

[v7,1/6] pci: Introduce pci_register_io_range() helper function.

Message ID 1394811272-1547-2-git-send-email-Liviu.Dudau@arm.com
State Changes Requested
Headers show

Commit Message

Liviu Dudau March 14, 2014, 3:34 p.m. UTC 
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.

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.

Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
Acked-by: Grant Likely <grant.likely@linaro.org>
Tested-by: Tanmay Inamdar <tinamdar@apm.com>
---
 drivers/of/address.c       | 9 +++++++++
 include/linux/of_address.h | 1 +
 2 files changed, 10 insertions(+)

Comments

Bjorn Helgaas April 5, 2014, 12:19 a.m. UTC | #1 
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 <Liviu.Dudau@arm.com>
> Acked-by: Grant Likely <grant.likely@linaro.org>
> Tested-by: Tanmay Inamdar <tinamdar@apm.com>
> ---
>  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
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Benjamin Herrenschmidt April 6, 2014, 9:49 a.m. UTC | #2 
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.


--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Liviu Dudau April 7, 2014, 8:31 a.m. UTC | #3 
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 <Liviu.Dudau@arm.com>
> > Acked-by: Grant Likely <grant.likely@linaro.org>
> > Tested-by: Tanmay Inamdar <tinamdar@apm.com>
> > ---
> >  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
> > 
>
Liviu Dudau April 7, 2014, 8:35 a.m. UTC | #4 
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
Benjamin Herrenschmidt April 7, 2014, 9:13 a.m. UTC | #5 
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.


--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann April 7, 2014, 11:16 a.m. UTC | #6 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann April 7, 2014, 11:36 a.m. UTC | #7 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Liviu Dudau April 7, 2014, 1:42 p.m. UTC | #8 
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
> 
>
Bjorn Helgaas April 7, 2014, 5:58 p.m. UTC | #9 
On Mon, Apr 7, 2014 at 5:36 AM, Arnd Bergmann <arnd@arndb.de> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Helgaas April 7, 2014, 11:21 p.m. UTC | #10 
On Fri, Mar 14, 2014 at 9:34 AM, Liviu Dudau <Liviu.Dudau@arm.com> 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.
>
> 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.
>
> Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
> Acked-by: Grant Likely <grant.likely@linaro.org>
> Tested-by: Tanmay Inamdar <tinamdar@apm.com>
> ---
>  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
> +}

This isn't PCI code, so I'm fine with it in that sense, but I'm not
sure the idea of a PCI_IOBASE #define is really what we need.  It's
not really determined by the processor architecture, it's determined
by the platform.  And a single address isn't enough in general,
either, because if there are multiple host bridges, there's no reason
the apertures that generate PCI I/O transactions need to be contiguous
on the CPU side.

That's just a long way of saying that if we ever came up with a more
generic way to handle I/O port spaces, PCI_IOBASE might go away.  And
I guess part of that rework could be changing this use of it along
with the others.

>  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
>
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann April 8, 2014, 7:12 a.m. UTC | #11 
On Monday 07 April 2014 17:21:51 Bjorn Helgaas wrote:
> On Fri, Mar 14, 2014 at 9:34 AM, Liviu Dudau <Liviu.Dudau@arm.com> 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.
> >
> > 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.
> >
> > Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
> > Acked-by: Grant Likely <grant.likely@linaro.org>
> > Tested-by: Tanmay Inamdar <tinamdar@apm.com>
> > ---
> >  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
> > +}
> 
> This isn't PCI code, so I'm fine with it in that sense, but I'm not
> sure the idea of a PCI_IOBASE #define is really what we need.  It's
> not really determined by the processor architecture, it's determined
> by the platform.  And a single address isn't enough in general,
> either, because if there are multiple host bridges, there's no reason
> the apertures that generate PCI I/O transactions need to be contiguous
> on the CPU side.
> 
> That's just a long way of saying that if we ever came up with a more
> generic way to handle I/O port spaces, PCI_IOBASE might go away.  And
> I guess part of that rework could be changing this use of it along
> with the others.

I'd rather not add a generic implementation of this at all, but
keep it all within the host resource scanning code.

If we do add a generic implementation, my preference would be
to use the version introduced for arm64, with a fallback of
returning -EINVAL if the architecture doesn't implement it.

There is no way ever that returning '0' makes sense here: Either
the architecture supports memory mapped I/O spaces and then we
should be able to find an appropriate io_offset for it, or it
doesn't support memory mapped I/O spaces and we should never
even call this function.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Liviu Dudau April 8, 2014, 9:49 a.m. UTC | #12 
On Tue, Apr 08, 2014 at 12:21:51AM +0100, Bjorn Helgaas wrote:
> On Fri, Mar 14, 2014 at 9:34 AM, Liviu Dudau <Liviu.Dudau@arm.com> 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.
> >
> > 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.
> >
> > Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
> > Acked-by: Grant Likely <grant.likely@linaro.org>
> > Tested-by: Tanmay Inamdar <tinamdar@apm.com>
> > ---
> >  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
> > +}
> 
> This isn't PCI code, so I'm fine with it in that sense, but I'm not
> sure the idea of a PCI_IOBASE #define is really what we need.  It's
> not really determined by the processor architecture, it's determined
> by the platform.  And a single address isn't enough in general,
> either, because if there are multiple host bridges, there's no reason
> the apertures that generate PCI I/O transactions need to be contiguous
> on the CPU side.

It should not be only platform's choice if the architecture doesn't support it.
To my mind PCI_IOBASE means "I support MMIO operations and this is the
start of the virtual address where my I/O ranges are mapped." It's the
same as ppc's _IO_BASE. And pci_address_to_pio() will take care to give
you the correct io_offset in the presence of multiple host bridges,
while keeping the io resource in the range [0 .. host_bridge_io_range_size - 1]

> 
> That's just a long way of saying that if we ever came up with a more
> generic way to handle I/O port spaces, PCI_IOBASE might go away.  And
> I guess part of that rework could be changing this use of it along
> with the others.

And I have a patch series that #defines PCI_IOBASE only in those architectures
that support MMIO, where this macro makes sense. Also notice that the
arm64 series has a patch that I'm going to roll into this one where ioport_map()
gets fixed to include PCI_IOBASE when !CONFIG_GENERIC_MAP.

Best regards,
Liviu

> 
> >  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
> >
>
Liviu Dudau April 8, 2014, 9:50 a.m. UTC | #13 
On Mon, Apr 07, 2014 at 06:58:24PM +0100, Bjorn Helgaas wrote:
> On Mon, Apr 7, 2014 at 5:36 AM, Arnd Bergmann <arnd@arndb.de> 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
>
Arnd Bergmann April 8, 2014, 10:11 a.m. UTC | #14 
On Tuesday 08 April 2014 10:49:33 Liviu Dudau wrote:
> On Tue, Apr 08, 2014 at 12:21:51AM +0100, Bjorn Helgaas wrote:
> > On Fri, Mar 14, 2014 at 9:34 AM, Liviu Dudau <Liviu.Dudau@arm.com> 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.
> > >
> > > 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.
> > >
> > > Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
> > > Acked-by: Grant Likely <grant.likely@linaro.org>
> > > Tested-by: Tanmay Inamdar <tinamdar@apm.com>
> > > ---
> > >  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
> > > +}
> > 
> > This isn't PCI code, so I'm fine with it in that sense, but I'm not
> > sure the idea of a PCI_IOBASE #define is really what we need.  It's
> > not really determined by the processor architecture, it's determined
> > by the platform.  And a single address isn't enough in general,
> > either, because if there are multiple host bridges, there's no reason
> > the apertures that generate PCI I/O transactions need to be contiguous
> > on the CPU side.
> 
> It should not be only platform's choice if the architecture doesn't support it.
> To my mind PCI_IOBASE means "I support MMIO operations and this is the
> start of the virtual address where my I/O ranges are mapped." It's the
> same as ppc's _IO_BASE. And pci_address_to_pio() will take care to give
> you the correct io_offset in the presence of multiple host bridges,
> while keeping the io resource in the range [0 .. host_bridge_io_range_size - 1]

There is a wide range of implementations across architectures:

a) no access to I/O ports at all (tile, s390, ...)
b) access to I/O ports only through special instructions (x86, ...)
c) all MMIO is mapped virtual contiguous to PCI_IOBASE or _IO_BASE
   (most ppc64, arm32 with MMU, arm64, ...)
d) only one PCI host can have an I/O space (mips, microblaze, ...)
e) each host controller can have its own method (ppc64 with indirect pio)
f) PIO token equals virtual address plus offset (some legacy ARM platforms,
   probably some other architectures), or physical address (sparc)
g) PIO token encodes address space number plus offset (ia64)

a) and b) are trivially handled by any implementation that falls
back to 'return -EINVAL'.
I believe that c) is the most appropriate solution and we should be
able to adopt it by most of the architectures that have an MMU and
make it the default implementation.
d) seems like a good fallback for noMMU architectures: While we do
need to support I/O spaces, we need to support multiple PCI domains,
and we need to support noMMU, the combination of all three should
be extremely rare, and I'd leave it up to the architecture to support
that if there is a real use case, rather than trying to put that into
common code.
Anything that currently requires e), f) or g) I think should keep
doing that and not try to use the generic implementation.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann April 8, 2014, 10:22 a.m. UTC | #15 
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 <arnd@arndb.de> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Helgaas April 8, 2014, 4:48 p.m. UTC | #16 
On Tue, Apr 8, 2014 at 4:11 AM, Arnd Bergmann <arnd@arndb.de> wrote:

> There is a wide range of implementations across architectures:
>
> a) no access to I/O ports at all (tile, s390, ...)
> b) access to I/O ports only through special instructions (x86, ...)
> c) all MMIO is mapped virtual contiguous to PCI_IOBASE or _IO_BASE
>    (most ppc64, arm32 with MMU, arm64, ...)
> d) only one PCI host can have an I/O space (mips, microblaze, ...)
> e) each host controller can have its own method (ppc64 with indirect pio)
> f) PIO token equals virtual address plus offset (some legacy ARM platforms,
>    probably some other architectures), or physical address (sparc)
> g) PIO token encodes address space number plus offset (ia64)
>
> a) and b) are trivially handled by any implementation that falls
> back to 'return -EINVAL'.
> I believe that c) is the most appropriate solution and we should be
> able to adopt it by most of the architectures that have an MMU and
> make it the default implementation.
> d) seems like a good fallback for noMMU architectures: While we do
> need to support I/O spaces, we need to support multiple PCI domains,
> and we need to support noMMU, the combination of all three should
> be extremely rare, and I'd leave it up to the architecture to support
> that if there is a real use case, rather than trying to put that into
> common code.
> Anything that currently requires e), f) or g) I think should keep
> doing that and not try to use the generic implementation.

Thanks for the nice summary.  That's way more than I had figured out myself.

I don't know whether it'd be worth it, especially for something that's
so close to obsolete, but it seems like it should be *possible* to
generalize and unify this somewhat.  I would argue that g) (which I
wrote, so I know it better than the others) could fairly easily
subsume c), d), and f), since it maps an ioport number to a virtual
address for an MMIO access, but it doesn't assume that all the MMIO
spaces are contiguous.

b), e), and maybe a) could be handled with an exception, e.g., inside
inb(), look up the struct io_space (e.g., similar to what ia64 does in
__ia64_mk_io_addr()), and if that struct contains a non-zero ops
pointer, use that instead of doing the MMIO access.  The ops pointer
functions could use the x86 INB instruction or do the indirect PIO
thing or whatever.

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Helgaas April 8, 2014, 4:54 p.m. UTC | #17 
On Tue, Apr 8, 2014 at 4:22 AM, Arnd Bergmann <arnd@arndb.de> 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 <arnd@arndb.de> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Catalin Marinas June 26, 2014, 8:59 a.m. UTC | #18 
(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 <arnd@arndb.de> 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.
Liviu Dudau June 26, 2014, 9:30 a.m. UTC | #19 
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 <arnd@arndb.de> 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
Catalin Marinas June 26, 2014, 2:11 p.m. UTC | #20 
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)
Will Deacon June 26, 2014, 2:14 p.m. UTC | #21 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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,