diff mbox

[v3,1/4] PCI: designware: Configuration space should be specified in 'reg'

Message ID 1405587643-13808-2-git-send-email-kishon@ti.com (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show

Commit Message

Kishon Vijay Abraham I July 17, 2014, 9 a.m. UTC 
The configuration address space has so far been specified in *ranges*,
however it should be specified in *reg* making it a platform MEM resource.
Hence used 'platform_get_resource_*' API to get configuration address
space in the designware driver.

Cc: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Marek Vasut <marex@denx.de>
Cc: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
Acked-by: Mohit Kumar <mohit.kumar@st.com>
Acked-by: Jingoo Han <jg1.han@samsung.com>
---
 .../devicetree/bindings/pci/designware-pcie.txt    |    4 ++++
 drivers/pci/host/pcie-designware.c                 |   17 +++++++++++++++--
 2 files changed, 19 insertions(+), 2 deletions(-)

Comments

Bjorn Helgaas July 22, 2014, 8:53 p.m. UTC | #1 
On Thu, Jul 17, 2014 at 02:30:40PM +0530, Kishon Vijay Abraham I wrote:
> The configuration address space has so far been specified in *ranges*,
> however it should be specified in *reg* making it a platform MEM resource.
> Hence used 'platform_get_resource_*' API to get configuration address
> space in the designware driver.
> 
> Cc: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Marek Vasut <marex@denx.de>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> Acked-by: Mohit Kumar <mohit.kumar@st.com>
> Acked-by: Jingoo Han <jg1.han@samsung.com>
> ---
>  .../devicetree/bindings/pci/designware-pcie.txt    |    4 ++++
>  drivers/pci/host/pcie-designware.c                 |   17 +++++++++++++++--
>  2 files changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pci/designware-pcie.txt b/Documentation/devicetree/bindings/pci/designware-pcie.txt
> index d0d15ee..ed0d9b9 100644
> --- a/Documentation/devicetree/bindings/pci/designware-pcie.txt
> +++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt
> @@ -2,6 +2,10 @@
>  
>  Required properties:
>  - compatible: should contain "snps,dw-pcie" to identify the core.
> +- reg: Should contain the configuration address space.
> +- reg-names: Must be "config" for the PCIe configuration space.
> +    (The old way of getting the configuration address space from "ranges"
> +    is deprecated and should be avoided.)

I'm not a devicetree person, so please excuse my floundering here.
There doesn't seem to be consistent usage of "reg" or "reg-names".
Here's what I found in the existing Documentation/devicetree/bindings/pci
stuff:

  83xx-512x-pci.txt
    - reg: should contain two address length tuples
	The first is for the internal pci bridge registers
	The second is for the pci config space access registers

  fsl,imx6q-pcie.txt
    - reg: base addresse and length of the pcie controller

  host-generic-pci.txt
    - reg            : The Configuration Space base address and size, as accessed
		       from the parent bus.

  nvidia,tegra20-pcie.txt
    - reg: A list of physical base address and length for each set of controller
      registers. Must contain an entry for each entry in the reg-names property.
    - reg-names: Must include the following entries:
      "pads": PADS registers
      "afi": AFI registers
      "cs": configuration space region

  pci-rcar-gen2.txt
    - reg:  A list of physical regions to access the device: the first is
	    the operational registers for the OHCI/EHCI controllers and the
	    second is for the bridge configuration and control registers.

  ralink,rt3883-pci.txt
     - reg: specifies the physical base address of the controller and
       the length of the memory mapped region.

  rcar-pci.txt
    - reg: base address and length of the pcie controller registers.

  samsung,exynos5440-pcie.txt
    - reg: base addresses and lengths of the pcie controller,
	  the phy controller, additional register for the phy controller.

  v3-v360epc-pci.txt
    - reg: should contain the base address of the V3 adapter.

So my questions are:

1) Is "reg" the right place for configuration space?  I assume this is
CAM or ECAM.  Only 83xx-512x-pci.txt, host-generic-pci.txt, and
nvidia,tegra20-pcie.txt mention using "reg" for config space; all the
others use "reg" for register space for the PCIe controller itself.

2) Is "config" the correct value for "reg-names"?  Only
nvidia,tegra20-pcie.txt has something similar, and it wants "cs".
Should these be the same?

3) Doesn't this add a revlock (kernels with this patch require a new
device tree that adds "reg" and "reg-names"?  I know both Mohit and
Jingoo acked this [1,2], so I'm just double-checking that this is
expected and desired.

Bjorn

[1] http://lkml.kernel.org/r/2CC2A0A4A178534D93D5159BF3BCB6619C5F94EFC8@EAPEX1MAIL1.st.com
[2] http://marc.info/?l=linux-pci&m=140482389231290&w=2


>  - #address-cells: set to <3>
>  - #size-cells: set to <2>
>  - device_type: set to "pci"
> diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
> index 1eaf4df..0b7b455 100644
> --- a/drivers/pci/host/pcie-designware.c
> +++ b/drivers/pci/host/pcie-designware.c
> @@ -20,6 +20,7 @@
>  #include <linux/of_pci.h>
>  #include <linux/pci.h>
>  #include <linux/pci_regs.h>
> +#include <linux/platform_device.h>
>  #include <linux/types.h>
>  
>  #include "pcie-designware.h"
> @@ -396,11 +397,23 @@ static const struct irq_domain_ops msi_domain_ops = {
>  int __init dw_pcie_host_init(struct pcie_port *pp)
>  {
>  	struct device_node *np = pp->dev->of_node;
> +	struct platform_device *pdev = to_platform_device(pp->dev);
>  	struct of_pci_range range;
>  	struct of_pci_range_parser parser;
> +	struct resource *cfg_res;
>  	u32 val;
>  	int i;
>  
> +	cfg_res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "config");
> +	if (cfg_res) {
> +		pp->config.cfg0_size = resource_size(cfg_res)/2;
> +		pp->config.cfg1_size = resource_size(cfg_res)/2;
> +		pp->cfg0_base = cfg_res->start;
> +		pp->cfg1_base = cfg_res->start + pp->config.cfg0_size;
> +	} else {
> +		dev_err(pp->dev, "missing *config* reg space\n");
> +	}
> +
>  	if (of_pci_range_parser_init(&parser, np)) {
>  		dev_err(pp->dev, "missing ranges property\n");
>  		return -EINVAL;
> @@ -433,6 +446,8 @@ int __init dw_pcie_host_init(struct pcie_port *pp)
>  			of_pci_range_to_resource(&range, np, &pp->cfg);
>  			pp->config.cfg0_size = resource_size(&pp->cfg)/2;
>  			pp->config.cfg1_size = resource_size(&pp->cfg)/2;
> +			pp->cfg0_base = pp->cfg.start;
> +			pp->cfg1_base = pp->cfg.start + pp->config.cfg0_size;
>  		}
>  	}
>  
> @@ -445,8 +460,6 @@ int __init dw_pcie_host_init(struct pcie_port *pp)
>  		}
>  	}
>  
> -	pp->cfg0_base = pp->cfg.start;
> -	pp->cfg1_base = pp->cfg.start + pp->config.cfg0_size;
>  	pp->mem_base = pp->mem.start;
>  
>  	pp->va_cfg0_base = devm_ioremap(pp->dev, pp->cfg0_base,
> -- 
> 1.7.9.5
> 
--
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 July 22, 2014, 9:11 p.m. UTC | #2 
On Tuesday 22 July 2014 14:53:26 Bjorn Helgaas wrote:
> > @@ -2,6 +2,10 @@
> >  
> >  Required properties:
> >  - compatible: should contain "snps,dw-pcie" to identify the core.
> > +- reg: Should contain the configuration address space.
> > +- reg-names: Must be "config" for the PCIe configuration space.
> > +    (The old way of getting the configuration address space from "ranges"
> > +    is deprecated and should be avoided.)
> 
> I'm not a devicetree person, so please excuse my floundering here.
> There doesn't seem to be consistent usage of "reg" or "reg-names".

I think the above is ok, given the constraints we hae.

> Here's what I found in the existing Documentation/devicetree/bindings/pci
> stuff:
>
>   83xx-512x-pci.txt
>     - reg: should contain two address length tuples
>         The first is for the internal pci bridge registers
>         The second is for the pci config space access registers

This is fine

>   fsl,imx6q-pcie.txt
>     - reg: base addresse and length of the pcie controller

This should have included an entry for the config space, it was a
mistake

>   host-generic-pci.txt
>     - reg            : The Configuration Space base address and size, as accessed
>                        from the parent bus.

This is fine.

>   nvidia,tegra20-pcie.txt
>     - reg: A list of physical base address and length for each set of controller
>       registers. Must contain an entry for each entry in the reg-names property.
>     - reg-names: Must include the following entries:
>       "pads": PADS registers
>       "afi": AFI registers
>       "cs": configuration space region

The names are a bit strange, but it's also ok. In general we should mandate
the order of the entries as well.

>   pci-rcar-gen2.txt
>     - reg:  A list of physical regions to access the device: the first is
>             the operational registers for the OHCI/EHCI controllers and the
>             second is for the bridge configuration and control registers.

This one is wrong, as previously discussed. The first region should
have been listed in the 'ranges' property, because these are the registers
of the child nodes (the actual PCI devices) rather than the host controller
itself.

>   ralink,rt3883-pci.txt
>      - reg: specifies the physical base address of the controller and
>        the length of the memory mapped region.

The wording is a bit strange but it seems otherwise correct.

>   rcar-pci.txt
>     - reg: base address and length of the pcie controller registers.

This is ok

>   samsung,exynos5440-pcie.txt
>     - reg: base addresses and lengths of the pcie controller,
>           the phy controller, additional register for the phy controller.

Could be a bit more verbose, but is not wrong. In order to work
with the "snps,dw-pcie" binding, this will need a reg-names property.

>   v3-v360epc-pci.txt
>     - reg: should contain the base address of the V3 adapter.

This has the same mistake that the pcie-dw driver has and should
be fixed by putting the config space in there.

> So my questions are:
> 
> 1) Is "reg" the right place for configuration space?  I assume this is
> CAM or ECAM.  Only 83xx-512x-pci.txt, host-generic-pci.txt, and
> nvidia,tegra20-pcie.txt mention using "reg" for config space; all the
> others use "reg" for register space for the PCIe controller itself.

We had long debates about this. The conclusion was that we should
have put them in 'reg' from the start and not used 'ranges' here.

> 2) Is "config" the correct value for "reg-names"?  Only
> nvidia,tegra20-pcie.txt has something similar, and it wants "cs".
> Should these be the same?

It would be better if they were the same, but it's not important.
The names are private to the device driver, and the register layout
within this region is driver specific, so we can't use common code
to parse them anyway.

> 3) Doesn't this add a revlock (kernels with this patch require a new
> device tree that adds "reg" and "reg-names"?  I know both Mohit and
> Jingoo acked this [1,2], so I'm just double-checking that this is
> expected and desired.

My understanding is that the patch still allows the old method as
a fallback if no reg with the name "config" is present.

	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
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/pci/designware-pcie.txt b/Documentation/devicetree/bindings/pci/designware-pcie.txt
index d0d15ee..ed0d9b9 100644
--- a/Documentation/devicetree/bindings/pci/designware-pcie.txt
+++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt
@@ -2,6 +2,10 @@ 
 
 Required properties:
 - compatible: should contain "snps,dw-pcie" to identify the core.
+- reg: Should contain the configuration address space.
+- reg-names: Must be "config" for the PCIe configuration space.
+    (The old way of getting the configuration address space from "ranges"
+    is deprecated and should be avoided.)
 - #address-cells: set to <3>
 - #size-cells: set to <2>
 - device_type: set to "pci"
diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
index 1eaf4df..0b7b455 100644
--- a/drivers/pci/host/pcie-designware.c
+++ b/drivers/pci/host/pcie-designware.c
@@ -20,6 +20,7 @@ 
 #include <linux/of_pci.h>
 #include <linux/pci.h>
 #include <linux/pci_regs.h>
+#include <linux/platform_device.h>
 #include <linux/types.h>
 
 #include "pcie-designware.h"
@@ -396,11 +397,23 @@  static const struct irq_domain_ops msi_domain_ops = {
 int __init dw_pcie_host_init(struct pcie_port *pp)
 {
 	struct device_node *np = pp->dev->of_node;
+	struct platform_device *pdev = to_platform_device(pp->dev);
 	struct of_pci_range range;
 	struct of_pci_range_parser parser;
+	struct resource *cfg_res;
 	u32 val;
 	int i;
 
+	cfg_res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "config");
+	if (cfg_res) {
+		pp->config.cfg0_size = resource_size(cfg_res)/2;
+		pp->config.cfg1_size = resource_size(cfg_res)/2;
+		pp->cfg0_base = cfg_res->start;
+		pp->cfg1_base = cfg_res->start + pp->config.cfg0_size;
+	} else {
+		dev_err(pp->dev, "missing *config* reg space\n");
+	}
+
 	if (of_pci_range_parser_init(&parser, np)) {
 		dev_err(pp->dev, "missing ranges property\n");
 		return -EINVAL;
@@ -433,6 +446,8 @@  int __init dw_pcie_host_init(struct pcie_port *pp)
 			of_pci_range_to_resource(&range, np, &pp->cfg);
 			pp->config.cfg0_size = resource_size(&pp->cfg)/2;
 			pp->config.cfg1_size = resource_size(&pp->cfg)/2;
+			pp->cfg0_base = pp->cfg.start;
+			pp->cfg1_base = pp->cfg.start + pp->config.cfg0_size;
 		}
 	}
 
@@ -445,8 +460,6 @@  int __init dw_pcie_host_init(struct pcie_port *pp)
 		}
 	}
 
-	pp->cfg0_base = pp->cfg.start;
-	pp->cfg1_base = pp->cfg.start + pp->config.cfg0_size;
 	pp->mem_base = pp->mem.start;
 
 	pp->va_cfg0_base = devm_ioremap(pp->dev, pp->cfg0_base,