diff mbox

[v2,03/18] PCI: designware: Configuration space should be specified in 'reg'

Message ID 1401345500-20188-4-git-send-email-kishon@ti.com
State New, archived
Headers show

Commit Message

Kishon Vijay Abraham I May 29, 2014, 6:38 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: Mohit Kumar <mohit.kumar@st.com>
Cc: Jingoo Han <jg1.han@samsung.com>
Cc: Marek Vasut <marex@denx.de>
Cc: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 .../devicetree/bindings/pci/designware-pcie.txt    |    1 +
 drivers/pci/host/pcie-designware.c                 |   17 +++++++++++++++--
 2 files changed, 16 insertions(+), 2 deletions(-)

Comments

Mohit KUMAR DCG May 29, 2014, 7:11 a.m. UTC | #1
Hello Kishon,

> -----Original Message-----
> From: Kishon Vijay Abraham I [mailto:kishon@ti.com]
> Sent: Thursday, May 29, 2014 12:08 PM
> To: devicetree@vger.kernel.org; linux-doc@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; linux-omap@vger.kernel.org; linux-
> pci@vger.kernel.org; linux-kernel@vger.kernel.org
> Cc: arnd@arndb.de; tony@atomide.com; jg1.han@samsung.com;
> kishon@ti.com; Jason Gunthorpe; Bjorn Helgaas; Mohit KUMAR DCG; Marek
> Vasut
> Subject: [PATCH v2 03/18] PCI: designware: Configuration space should be
> specified in 'reg'
> 
> 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: Mohit Kumar <mohit.kumar@st.com>
> Cc: Jingoo Han <jg1.han@samsung.com>
> Cc: Marek Vasut <marex@denx.de>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  .../devicetree/bindings/pci/designware-pcie.txt    |    1 +
>  drivers/pci/host/pcie-designware.c                 |   17 +++++++++++++++--
>  2 files changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pci/designware-pcie.txt
> b/Documentation/devicetree/bindings/pci/designware-pcie.txt
> index d6fae13..8314360 100644
> --- a/Documentation/devicetree/bindings/pci/designware-pcie.txt
> +++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt
> @@ -6,6 +6,7 @@ Required properties:
>  	as "samsung,exynos5440-pcie" or "fsl,imx6q-pcie".
>  - reg: base addresses and lengths of the pcie controller,
>  	the phy controller, additional register for the phy controller.
> +	The configuration address space should also be specified here.
>  - interrupts: interrupt values for level interrupt,
>  	pulse interrupt, special interrupt.
>  - clocks: from common clock binding: handle to pci clock.
> diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-
> designware.c
> index c4e3732..603b386 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"
> @@ -392,11 +393,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;
> @@ -429,6 +442,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;

- As you are getting cfg address space as MEM resource, so remove above code that
 gets the configuration space from dt range. Also correct dt for pcie cfg space for the platforms
 based on this driver.

Otherwise looks fine to me.

Thanks
Mohit

>  		}
>  	}
> 
> @@ -441,8 +456,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
Kishon Vijay Abraham I May 29, 2014, 1:16 p.m. UTC | #2
Hi,

On Thursday 29 May 2014 12:41 PM, Mohit KUMAR DCG wrote:
> Hello Kishon,
> 
>> -----Original Message-----
>> From: Kishon Vijay Abraham I [mailto:kishon@ti.com]
>> Sent: Thursday, May 29, 2014 12:08 PM
>> To: devicetree@vger.kernel.org; linux-doc@vger.kernel.org; linux-arm-
>> kernel@lists.infradead.org; linux-omap@vger.kernel.org; linux-
>> pci@vger.kernel.org; linux-kernel@vger.kernel.org
>> Cc: arnd@arndb.de; tony@atomide.com; jg1.han@samsung.com;
>> kishon@ti.com; Jason Gunthorpe; Bjorn Helgaas; Mohit KUMAR DCG; Marek
>> Vasut
>> Subject: [PATCH v2 03/18] PCI: designware: Configuration space should be
>> specified in 'reg'
>>
>> 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: Mohit Kumar <mohit.kumar@st.com>
>> Cc: Jingoo Han <jg1.han@samsung.com>
>> Cc: Marek Vasut <marex@denx.de>
>> Cc: Arnd Bergmann <arnd@arndb.de>
>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>> ---
>>  .../devicetree/bindings/pci/designware-pcie.txt    |    1 +
>>  drivers/pci/host/pcie-designware.c                 |   17 +++++++++++++++--
>>  2 files changed, 16 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/pci/designware-pcie.txt
>> b/Documentation/devicetree/bindings/pci/designware-pcie.txt
>> index d6fae13..8314360 100644
>> --- a/Documentation/devicetree/bindings/pci/designware-pcie.txt
>> +++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt
>> @@ -6,6 +6,7 @@ Required properties:
>>  	as "samsung,exynos5440-pcie" or "fsl,imx6q-pcie".
>>  - reg: base addresses and lengths of the pcie controller,
>>  	the phy controller, additional register for the phy controller.
>> +	The configuration address space should also be specified here.
>>  - interrupts: interrupt values for level interrupt,
>>  	pulse interrupt, special interrupt.
>>  - clocks: from common clock binding: handle to pci clock.
>> diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-
>> designware.c
>> index c4e3732..603b386 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"
>> @@ -392,11 +393,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;
>> @@ -429,6 +442,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;
> 
> - As you are getting cfg address space as MEM resource, so remove above code that
>  gets the configuration space from dt range. Also correct dt for pcie cfg space for the platforms
>  based on this driver.

Ok. Will send that as a separate patch converting all platforms to use MEM
resource.

Thanks
Kishon
--
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
Kumar Gala May 29, 2014, 3:03 p.m. UTC | #3
On May 29, 2014, at 1:38 AM, Kishon Vijay Abraham I <kishon@ti.com> 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: Mohit Kumar <mohit.kumar@st.com>
> Cc: Jingoo Han <jg1.han@samsung.com>
> Cc: Marek Vasut <marex@denx.de>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
> .../devicetree/bindings/pci/designware-pcie.txt    |    1 +
> drivers/pci/host/pcie-designware.c                 |   17 +++++++++++++++--
> 2 files changed, 16 insertions(+), 2 deletions(-)

Why should the cfg space be defined in *reg* instead of ranges?

- k
Liviu Dudau May 29, 2014, 3:18 p.m. UTC | #4
On Thu, May 29, 2014 at 10:03:54AM -0500, Kumar Gala wrote:
> 
> On May 29, 2014, at 1:38 AM, Kishon Vijay Abraham I <kishon@ti.com> 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: Mohit Kumar <mohit.kumar@st.com>
> > Cc: Jingoo Han <jg1.han@samsung.com>
> > Cc: Marek Vasut <marex@denx.de>
> > Cc: Arnd Bergmann <arnd@arndb.de>
> > Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> > ---
> > .../devicetree/bindings/pci/designware-pcie.txt    |    1 +
> > drivers/pci/host/pcie-designware.c                 |   17 +++++++++++++++--
> > 2 files changed, 16 insertions(+), 2 deletions(-)
> 
> Why should the cfg space be defined in *reg* instead of ranges?

Because what you end up using is a struct resource to represent the cfg space and
the conversion between ranges and resources breaks down for CFG space (we don't
have a flag in the resource flags to say this is CFG resource). Specifying it
as a *reg* property makes it a MEM resource and no special casing is needed.

Best regards,
Liviu

> 
> - k
> 
> -- 
> Employee of Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
> 
> --
> 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
>
Kumar Gala May 29, 2014, 4:03 p.m. UTC | #5
On May 29, 2014, at 10:18 AM, Liviu Dudau <liviu@dudau.co.uk> wrote:

> On Thu, May 29, 2014 at 10:03:54AM -0500, Kumar Gala wrote:
>> 
>> On May 29, 2014, at 1:38 AM, Kishon Vijay Abraham I <kishon@ti.com> 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: Mohit Kumar <mohit.kumar@st.com>
>>> Cc: Jingoo Han <jg1.han@samsung.com>
>>> Cc: Marek Vasut <marex@denx.de>
>>> Cc: Arnd Bergmann <arnd@arndb.de>
>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>>> ---
>>> .../devicetree/bindings/pci/designware-pcie.txt    |    1 +
>>> drivers/pci/host/pcie-designware.c                 |   17 +++++++++++++++--
>>> 2 files changed, 16 insertions(+), 2 deletions(-)
>> 
>> Why should the cfg space be defined in *reg* instead of ranges?
> 
> Because what you end up using is a struct resource to represent the cfg space and
> the conversion between ranges and resources breaks down for CFG space (we don't
> have a flag in the resource flags to say this is CFG resource). Specifying it
> as a *reg* property makes it a MEM resource and no special casing is needed.
> 
> Best regards,
> Liviu

Just because the kernel doesn’t handle this is NO reason to change the way the DT works.

We are probably better of changing of_bus_pci_get_flags() to set IORESOURCE_MEM for cfg type.  Will send a patch for this.

- k
Jason Gunthorpe May 29, 2014, 4:30 p.m. UTC | #6
On Thu, May 29, 2014 at 11:03:36AM -0500, Kumar Gala wrote:

> Just because the kernel doesn’t handle this is NO reason to change
> the way the DT works.

The OF specs do not specify how to process a config type ranges entry,
and we all mutually agreed that the only sane interpretation for such
a thing would be to describe an ECAM memory space so generic code
could potentially make use of it.

Since designware is not ECAM it should not use config ranges.

This has come up multiple times now, and the above is the consensus.

Jason
--
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
Murali Karicheri May 29, 2014, 4:32 p.m. UTC | #7
On 5/29/2014 2:38 AM, ABRAHAM, KISHON VIJAY 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: Mohit Kumar <mohit.kumar@st.com>
> Cc: Jingoo Han <jg1.han@samsung.com>
> Cc: Marek Vasut <marex@denx.de>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>   .../devicetree/bindings/pci/designware-pcie.txt    |    1 +
>   drivers/pci/host/pcie-designware.c                 |   17 +++++++++++++++--
>   2 files changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/pci/designware-pcie.txt b/Documentation/devicetree/bindings/pci/designware-pcie.txt
> index d6fae13..8314360 100644
> --- a/Documentation/devicetree/bindings/pci/designware-pcie.txt
> +++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt
> @@ -6,6 +6,7 @@ Required properties:
>   	as "samsung,exynos5440-pcie" or "fsl,imx6q-pcie".
>   - reg: base addresses and lengths of the pcie controller,
>   	the phy controller, additional register for the phy controller.
> +	The configuration address space should also be specified here.
Kishon,

I am working on the Keystone PCI driver for which v1 is already posted. 
Want to clarify
following.
1. Original text for reg states "base addresses and lengths of the pcie 
controller,
         the phy controller, additional register for the phy controller" 
and you added
         "The configuration address space should also be specified here"

    and the code below added resource name "config"

Does PCI designware follow some convention? Does it mean after applying 
this patch
config name is mandatory or optional? Below code you are not returning 
error. Can you
or author of PCI designware clarify what is expected to be present as 
mandatory and
what is optional.

Does config refers to RC's config space or EP's config space or both? 
The code below divide
the size by 2. So it appears to be RC's + EP's config space. Please clarify.

>   - interrupts: interrupt values for level interrupt,
>   	pulse interrupt, special interrupt.
>   - clocks: from common clock binding: handle to pci clock.
> diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
> index c4e3732..603b386 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"
> @@ -392,11 +393,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");
This should return error -EINVAL.

> +	}
> +
>   	if (of_pci_range_parser_init(&parser, np)) {
>   		dev_err(pp->dev, "missing ranges property\n");
>   		return -EINVAL;
> @@ -429,6 +442,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;
>   		}
>   	}
>   
> @@ -441,8 +456,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,
BTW, Please also review my Keystone series so that we could discuss this 
topic in that context
as well.

--
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
Kumar Gala May 29, 2014, 4:51 p.m. UTC | #8
On May 29, 2014, at 11:30 AM, Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote:

> On Thu, May 29, 2014 at 11:03:36AM -0500, Kumar Gala wrote:
> 
>> Just because the kernel doesn’t handle this is NO reason to change
>> the way the DT works.
> 
> The OF specs do not specify how to process a config type ranges entry,
> and we all mutually agreed that the only sane interpretation for such
> a thing would be to describe an ECAM memory space so generic code
> could potentially make use of it.
> 
> Since designware is not ECAM it should not use config ranges.
> 
> This has come up multiple times now, and the above is the consensus.
> 
> Jason

Well the designware controller does support ECAM, just that the current in kernel users don’t do cfg space that way.

So do we continue to support the current users that use a cfg range for a non-ECAM space?  Or break their DT and convert them to using regs?

- k
Kishon Vijay Abraham I May 30, 2014, 5:30 a.m. UTC | #9
Hi,

On Thursday 29 May 2014 10:02 PM, Murali Karicheri wrote:
> On 5/29/2014 2:38 AM, ABRAHAM, KISHON VIJAY 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: Mohit Kumar <mohit.kumar@st.com>
>> Cc: Jingoo Han <jg1.han@samsung.com>
>> Cc: Marek Vasut <marex@denx.de>
>> Cc: Arnd Bergmann <arnd@arndb.de>
>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>> ---
>>   .../devicetree/bindings/pci/designware-pcie.txt    |    1 +
>>   drivers/pci/host/pcie-designware.c                 |   17 +++++++++++++++--
>>   2 files changed, 16 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/pci/designware-pcie.txt
>> b/Documentation/devicetree/bindings/pci/designware-pcie.txt
>> index d6fae13..8314360 100644
>> --- a/Documentation/devicetree/bindings/pci/designware-pcie.txt
>> +++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt
>> @@ -6,6 +6,7 @@ Required properties:
>>       as "samsung,exynos5440-pcie" or "fsl,imx6q-pcie".
>>   - reg: base addresses and lengths of the pcie controller,
>>       the phy controller, additional register for the phy controller.
>> +    The configuration address space should also be specified here.
> Kishon,
> 
> I am working on the Keystone PCI driver for which v1 is already posted. Want to
> clarify
> following.
> 1. Original text for reg states "base addresses and lengths of the pcie
> controller,
>         the phy controller, additional register for the phy controller" and you
> added
>         "The configuration address space should also be specified here"
> 
>    and the code below added resource name "config"
> 
> Does PCI designware follow some convention? Does it mean after applying this patch
> config name is mandatory or optional? Below code you are not returning error.
> Can you
> or author of PCI designware clarify what is expected to be present as mandatory
> and
> what is optional.

From whatever I could make out from the comments for my previous version,
'config' is mandatory for all new platforms adding support for PCIe DW. However
since there already exists platforms that use 'ranges', I'm not returning
error. Once all the platforms that use DW is modified to use 'reg', will return
error.
> 
> Does config refers to RC's config space or EP's config space or both? The code
> below divide

In the case of DRA7, it's the space from where you read the configuration space
contents of the EP (we have separate address space for the configuration space
of RC denoted by *rc_dbics* in this patch series). But there are other
platforms where RC does not have a separate configuration address space.
> the size by 2. So it appears to be RC's + EP's config space. Please clarify.

No. divide by 2 is for cfg1 and cfg1 is used by PCIe bridges.
> 
>>   - interrupts: interrupt values for level interrupt,
>>       pulse interrupt, special interrupt.
>>   - clocks: from common clock binding: handle to pci clock.
>> diff --git a/drivers/pci/host/pcie-designware.c
>> b/drivers/pci/host/pcie-designware.c
>> index c4e3732..603b386 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"
>> @@ -392,11 +393,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");
> This should return error -EINVAL.

ah.. it'll break for other platforms. It should be part of a different patch
once we convert all users to 8reg*.
> 
>> +    }
>> +
>>       if (of_pci_range_parser_init(&parser, np)) {
>>           dev_err(pp->dev, "missing ranges property\n");
>>           return -EINVAL;
>> @@ -429,6 +442,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;
>>           }
>>       }
>>   @@ -441,8 +456,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,
> BTW, Please also review my Keystone series so that we could discuss this topic
> in that context
> as well.

sure..

Cheers
Kishon
--
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
Murali Karicheri May 30, 2014, 2:15 p.m. UTC | #10
>-----Original Message-----
>From: Murali Karicheri [mailto:m-karicheri2@ti.com]
>Sent: Thursday, May 29, 2014 12:32 PM
>To: ABRAHAM, KISHON VIJAY
>Cc: devicetree@vger.kernel.org; linux-doc@vger.kernel.org; linux-arm-
>kernel@lists.infradead.org; linux-omap@vger.kernel.org; linux-pci@vger.kernel.org; linux-
>kernel@vger.kernel.org; arnd@arndb.de; tony@atomide.com; jg1.han@samsung.com;
>Jason Gunthorpe; Bjorn Helgaas; Mohit Kumar; Marek Vasut
>Subject: Re: [PATCH v2 03/18] PCI: designware: Configuration space should be specified
>in 'reg'
>
>On 5/29/2014 2:38 AM, ABRAHAM, KISHON VIJAY 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: Mohit Kumar <mohit.kumar@st.com>
>> Cc: Jingoo Han <jg1.han@samsung.com>
>> Cc: Marek Vasut <marex@denx.de>
>> Cc: Arnd Bergmann <arnd@arndb.de>
>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>> ---
>>   .../devicetree/bindings/pci/designware-pcie.txt    |    1 +
>>   drivers/pci/host/pcie-designware.c                 |   17 +++++++++++++++--
>>   2 files changed, 16 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/pci/designware-pcie.txt
>> b/Documentation/devicetree/bindings/pci/designware-pcie.txt
>> index d6fae13..8314360 100644
>> --- a/Documentation/devicetree/bindings/pci/designware-pcie.txt
>> +++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt
>> @@ -6,6 +6,7 @@ Required properties:
>>   	as "samsung,exynos5440-pcie" or "fsl,imx6q-pcie".
>>   - reg: base addresses and lengths of the pcie controller,
>>   	the phy controller, additional register for the phy controller.
>> +	The configuration address space should also be specified here.
>Kishon,
>
>I am working on the Keystone PCI driver for which v1 is already posted.
>Want to clarify
>following.
>1. Original text for reg states "base addresses and lengths of the pcie controller,
>         the phy controller, additional register for the phy controller"
>and you added
>         "The configuration address space should also be specified here"
>
>    and the code below added resource name "config"
>
>Does PCI designware follow some convention? Does it mean after applying this patch
>config name is mandatory or optional? Below code you are not returning error. Can you or
>author of PCI designware clarify what is expected to be present as mandatory and what is
>optional.
>
>Does config refers to RC's config space or EP's config space or both?
>The code below divide
>the size by 2. So it appears to be RC's + EP's config space. Please clarify.
>
>>   - interrupts: interrupt values for level interrupt,
>>   	pulse interrupt, special interrupt.
>>   - clocks: from common clock binding: handle to pci clock.
>> diff --git a/drivers/pci/host/pcie-designware.c
>> b/drivers/pci/host/pcie-designware.c
>> index c4e3732..603b386 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"
>> @@ -392,11 +393,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");
>This should return error -EINVAL.
>
>> +	}
>> +
>>   	if (of_pci_range_parser_init(&parser, np)) {
>>   		dev_err(pp->dev, "missing ranges property\n");
>>   		return -EINVAL;
>> @@ -429,6 +442,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;
>>   		}
>>   	}
>>
>> @@ -441,8 +456,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,
>BTW, Please also review my Keystone series so that we could discuss this topic in that
>context as well.

--
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
Kishon Vijay Abraham I June 18, 2014, 9:14 a.m. UTC | #11
Hi,

On Friday 30 May 2014 07:45 PM, Karicheri, Muralidharan wrote:
>> -----Original Message-----
>> From: Murali Karicheri [mailto:m-karicheri2@ti.com]
>> Sent: Thursday, May 29, 2014 12:32 PM
>> To: ABRAHAM, KISHON VIJAY
>> Cc: devicetree@vger.kernel.org; linux-doc@vger.kernel.org; linux-arm-
>> kernel@lists.infradead.org; linux-omap@vger.kernel.org; linux-pci@vger.kernel.org; linux-
>> kernel@vger.kernel.org; arnd@arndb.de; tony@atomide.com; jg1.han@samsung.com;
>> Jason Gunthorpe; Bjorn Helgaas; Mohit Kumar; Marek Vasut
>> Subject: Re: [PATCH v2 03/18] PCI: designware: Configuration space should be specified
>> in 'reg'
>>
>> On 5/29/2014 2:38 AM, ABRAHAM, KISHON VIJAY 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: Mohit Kumar <mohit.kumar@st.com>
>>> Cc: Jingoo Han <jg1.han@samsung.com>
>>> Cc: Marek Vasut <marex@denx.de>
>>> Cc: Arnd Bergmann <arnd@arndb.de>
>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>>> ---
>>>   .../devicetree/bindings/pci/designware-pcie.txt    |    1 +
>>>   drivers/pci/host/pcie-designware.c                 |   17 +++++++++++++++--
>>>   2 files changed, 16 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/pci/designware-pcie.txt
>>> b/Documentation/devicetree/bindings/pci/designware-pcie.txt
>>> index d6fae13..8314360 100644
>>> --- a/Documentation/devicetree/bindings/pci/designware-pcie.txt
>>> +++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt
>>> @@ -6,6 +6,7 @@ Required properties:
>>>   	as "samsung,exynos5440-pcie" or "fsl,imx6q-pcie".
>>>   - reg: base addresses and lengths of the pcie controller,
>>>   	the phy controller, additional register for the phy controller.
>>> +	The configuration address space should also be specified here.
>> Kishon,
>>
>> I am working on the Keystone PCI driver for which v1 is already posted.
>> Want to clarify
>> following.
>> 1. Original text for reg states "base addresses and lengths of the pcie controller,
>>         the phy controller, additional register for the phy controller"
>> and you added
>>         "The configuration address space should also be specified here"
>>
>>    and the code below added resource name "config"
>>
>> Does PCI designware follow some convention? Does it mean after applying this patch
>> config name is mandatory or optional? Below code you are not returning error. Can you or
>> author of PCI designware clarify what is expected to be present as mandatory and what is
>> optional.
>>
>> Does config refers to RC's config space or EP's config space or both?
>> The code below divide
>> the size by 2. So it appears to be RC's + EP's config space. Please clarify.
>>
>>>   - interrupts: interrupt values for level interrupt,
>>>   	pulse interrupt, special interrupt.
>>>   - clocks: from common clock binding: handle to pci clock.
>>> diff --git a/drivers/pci/host/pcie-designware.c
>>> b/drivers/pci/host/pcie-designware.c
>>> index c4e3732..603b386 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"
>>> @@ -392,11 +393,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");
>> This should return error -EINVAL.

Just read the other thread and Grant Likely suggested the host controller
driver should be backward compatible [1]. So we can't return -EINVAL here.
So I'd assume this patch is fine as is? Arnd? Jingoo?

[1] -> https://lkml.org/lkml/2014/6/3/124

Thanks
Kishon
--
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
Jingoo Han June 18, 2014, 9:27 a.m. UTC | #12
On Wednesday, June 18, 2014 6:15 PM, Kishon Vijay Abraham I wrote:
> On Friday 30 May 2014 07:45 PM, Karicheri, Muralidharan wrote:
> >> On 5/29/2014 2:38 AM, ABRAHAM, KISHON VIJAY 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: Mohit Kumar <mohit.kumar@st.com>
> >>> Cc: Jingoo Han <jg1.han@samsung.com>
> >>> Cc: Marek Vasut <marex@denx.de>
> >>> Cc: Arnd Bergmann <arnd@arndb.de>
> >>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> >>> ---
> >>>   .../devicetree/bindings/pci/designware-pcie.txt    |    1 +
> >>>   drivers/pci/host/pcie-designware.c                 |   17 +++++++++++++++--
> >>>   2 files changed, 16 insertions(+), 2 deletions(-)

[...]

> >>> +	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");
> >> This should return error -EINVAL.
> 
> Just read the other thread and Grant Likely suggested the host controller
> driver should be backward compatible [1]. So we can't return -EINVAL here.
> So I'd assume this patch is fine as is? Arnd? Jingoo?

Yes, you're right. The driver should keep backward compatibility for
legacy DT binding. However, after enough time goes by, these legacy
DT handling can be removed.

Best regards,
Jingoo Han

> 
> [1] -> https://lkml.org/lkml/2014/6/3/124
> 
> Thanks
> Kishon

--
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 d6fae13..8314360 100644
--- a/Documentation/devicetree/bindings/pci/designware-pcie.txt
+++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt
@@ -6,6 +6,7 @@  Required properties:
 	as "samsung,exynos5440-pcie" or "fsl,imx6q-pcie".
 - reg: base addresses and lengths of the pcie controller,
 	the phy controller, additional register for the phy controller.
+	The configuration address space should also be specified here.
 - interrupts: interrupt values for level interrupt,
 	pulse interrupt, special interrupt.
 - clocks: from common clock binding: handle to pci clock.
diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
index c4e3732..603b386 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"
@@ -392,11 +393,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;
@@ -429,6 +442,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;
 		}
 	}
 
@@ -441,8 +456,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,