On Fri, Mar 14, 2014 at 9:34 AM, Liviu Dudau <[email protected]> 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 <[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
> +}
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
>
On Monday 07 April 2014 17:21:51 Bjorn Helgaas wrote:
> On Fri, Mar 14, 2014 at 9:34 AM, Liviu Dudau <[email protected]> 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 <[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
> > +}
>
> 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
On Tue, Apr 08, 2014 at 12:21:51AM +0100, Bjorn Helgaas wrote:
> On Fri, Mar 14, 2014 at 9:34 AM, Liviu Dudau <[email protected]> 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 <[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
> > +}
>
> 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
> >
>
--
====================
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
¯\_(ツ)_/¯
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 <[email protected]> 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 <[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
> > > +}
> >
> > 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
On Tue, Apr 8, 2014 at 4:11 AM, Arnd Bergmann <[email protected]> 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