Message ID | 1405587643-13808-2-git-send-email-kishon@ti.com |
---|---|
State | Superseded, archived |
Headers | show |
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 devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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,