Re: [PATCH v2 1/2] openrisc: Add pci bus support

[Arnd Bergmann]

On Thu, Jul 14, 2022 at 2:41 PM Stafford Horne <shorne@xxxxxxxxx> wrote:
> On Thu, Jul 14, 2022 at 09:56:44AM +0200, Arnd Bergmann wrote:
> > > diff --git a/arch/openrisc/Kconfig b/arch/openrisc/Kconfig
> > > index e814df4c483c..327241988819 100644
> > > --- a/arch/openrisc/Kconfig
> > > +++ b/arch/openrisc/Kconfig
> > > @@ -21,7 +21,9 @@ config OPENRISC
> > >         select GENERIC_IRQ_PROBE
> > >         select GENERIC_IRQ_SHOW
> > >         select GENERIC_IOMAP
> > > +       select GENERIC_PCI_IOMAP
> > >         select GENERIC_CPU_DEVICES
> >
> > > @@ -46,9 +50,6 @@ config MMU
> > >  config GENERIC_HWEIGHT
> > >         def_bool y
> > >
> > > -config NO_IOPORT_MAP
> > > -       def_bool y
> > > -
>
> I tried the below patch on top of this but I get failures, as the __pci_ioport_map
> uses ioport_map.
>
>     lib/pci_iomap.c: In function 'pci_iomap_range':
>       CC      drivers/i2c/i2c-core-base.o
>     ./include/asm-generic/pci_iomap.h:29:41: error: implicit declaration of function 'ioport_map'; did you mean 'ioremap'? [-Werror=implicit-function-declaration]
>        29 | #define __pci_ioport_map(dev, port, nr) ioport_map((port), (nr))
>           |                                         ^~~~~~~~~~
>     lib/pci_iomap.c:44:24: note: in expansion of macro '__pci_ioport_map'
>        44 |                 return __pci_ioport_map(dev, start, len);
>           |                        ^~~~~~~~~~~~~~~~
>

Ah, I see. So setting NO_IOPORT_MAP without GENERIC_PCI_IOMAP
probably just works, but then you'd also build all the driver that use
ioport_map() when they cannot work.

Maybe add another

#define __pci_ioport_map(dev, port, nr) NULL

case to include/asm-generic/pci_iomap.h, or add an #ifdef to lib
pci_iomap_range() to not call it in this case.

> > GENERIC_IOMAP makes no sense without PIO, and I think you also
> > need to keep the NO_IOPORT_MAP. I think you still want
> > GENERIC_PCI_IOMAP, which in the absence of the other two
> > should turn just return an __iomem pointer for memory resource
> > and NULL for i/o resources.
>
> OK.
>
> If we keep NO_IOPORT_MAP, it causes HAS_IOPORT_MAP to be false and it removes
> the definition of ioport_map which still seems to be needed at link time.  Maybe
> thats an issue though.

This is the intention of CONFIG_NO_IOPORT_MAP, it's meant to be set
on architectures that have no way of defining ioport_map() in a sensible
way.

        Arnd