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 <t...@semihalf.com>
---
 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