Re: [PATCH V6 3/5] PCI: thunder-pem: Allow to probe PEM-specific register range for ACPI case

[Tomasz Nowicki]

On 19.09.2016 20:09, Bjorn Helgaas wrote:
> On Fri, Sep 09, 2016 at 09:24:05PM +0200, Tomasz Nowicki wrote:
> > thunder-pem driver stands for being ACPI based PCI host controller.
> > However, there is no standard way to describe its PEM-specific register
> > ranges in ACPI tables. Thus we add thunder_pem_init() ACPI extension
> > to obtain hardcoded addresses from static resource array.
> > Although it is not pretty, it prevents from creating standard mechanism to
> > handle similar cases in future.
> >
> > Signed-off-by: Tomasz Nowicki <tn@xxxxxxxxxxxx>
> > ---
> >  drivers/pci/host/pci-thunder-pem.c | 61 ++++++++++++++++++++++++++++++--------
> >  1 file changed, 48 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/pci/host/pci-thunder-pem.c b/drivers/pci/host/pci-thunder-pem.c
> > index 6abaf80..b048761 100644
> > --- a/drivers/pci/host/pci-thunder-pem.c
> > +++ b/drivers/pci/host/pci-thunder-pem.c
> > @@ -18,6 +18,7 @@
> >  #include <linux/init.h>
> >  #include <linux/of_address.h>
> >  #include <linux/of_pci.h>
> > +#include <linux/pci-acpi.h>
> >  #include <linux/pci-ecam.h>
> >  #include <linux/platform_device.h>
> >
> > @@ -284,6 +285,40 @@ static int thunder_pem_config_write(struct pci_bus *bus, unsigned int devfn,
> >  	return pci_generic_config_write(bus, devfn, where, size, val);
> >  }
> >
> > +#ifdef CONFIG_ACPI
> > +static struct resource thunder_pem_reg_res[] = {
> > +	[4] = DEFINE_RES_MEM(0x87e0c0000000UL, SZ_16M),
> > +	[5] = DEFINE_RES_MEM(0x87e0c1000000UL, SZ_16M),
> > +	[6] = DEFINE_RES_MEM(0x87e0c2000000UL, SZ_16M),
> > +	[7] = DEFINE_RES_MEM(0x87e0c3000000UL, SZ_16M),
> > +	[8] = DEFINE_RES_MEM(0x87e0c4000000UL, SZ_16M),
> > +	[9] = DEFINE_RES_MEM(0x87e0c5000000UL, SZ_16M),
> > +	[14] = DEFINE_RES_MEM(0x97e0c0000000UL, SZ_16M),
> > +	[15] = DEFINE_RES_MEM(0x97e0c1000000UL, SZ_16M),
> > +	[16] = DEFINE_RES_MEM(0x97e0c2000000UL, SZ_16M),
> > +	[17] = DEFINE_RES_MEM(0x97e0c3000000UL, SZ_16M),
> > +	[18] = DEFINE_RES_MEM(0x97e0c4000000UL, SZ_16M),
> > +	[19] = DEFINE_RES_MEM(0x97e0c5000000UL, SZ_16M),
>
> 1) The "correct" way to discover the resources consumed by an ACPI
>    device is to use the _CRS method.  I know there are some issues
>    there for bridges (not the fault of ThunderX!) because there's not
>    a good way to distinguish windows from resources consumed directly
>    by the bridge.
>
>    But we should either do this correctly, or include a comment about
>    why we're doing it wrong, so we don't give the impression that this
>    is the right way to do it.
>
>    I seem to recall some discussion about why we're doing it this way,
>    but I don't remember the details.  It'd be nice to include a
>    summary here.

OK I will. The reason why we cannot use _CRS for this case is that 
CONSUMER flag was not use consistently for the bridge so far.

> 2) This is a little weird because here we define the resource size as
>    16MB, in the OF case we get the resource size from OF, in either
>    case we ioremap 64K of it, and then as far as I can tell, we only
>    ever access PEM_CFG_WR and PEM_CFG_RD, at offsets 0x28 and 0x30
>    into the space.
>
>    If the hardware actually decodes the entire 16MB, we should ioremap
>    the whole 16MB.  (Strictly speaking, drivers only need to ioremap
>    the parts they're using, but in this case nobody claims the entire
>    resource because of deficiencies in the ACPI and OF cores, so the
>    driver should ioremap the entire thing to help prevent conflicts
>    with other devices.)
>
>    It'd be nice if we didn't have the 64KB magic number.  I think
>    using devm_ioremap_resource() would be nice.

I agree.

David, is there anything which prevents us from using 
devm_ioremap_resource() here with SZ_16M size?

Tomasz