[PATCH] PCI: Designware: make num-lanes an optional DT property

[PATCH] PCI: Designware: make num-lanes an optional DT property

[Gabriele Paoloni]

From: gabriele paoloni <gabriele.paoloni@huawei.com>

Currently num-lanes is read in dw_pcie_host_init().
for A SoC that performs the link-up operation in UEFI num-lanes
is not needed in the DTS but it has to be specified to any value;
otherwise dw_pcie_host_init will fail.
If the link is brought up in BIOS dw_pcie_setup_rc should not
be called.
This patch rework dw_pcie_host_init() so that, if num-lanes is
not specified, it sets pp->lanes = 0. If later on dw_pcie_setup_rc
is called and pp->num_lanes is not set to a valid value,
dw_pcie_setup_rc will send an error message and return.

This patch is a follow-up of
http://www.spinics.net/lists/linux-pci/msg44394.html

Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com>
---
 Documentation/devicetree/bindings/pci/designware-pcie.txt |  3 ++-
 drivers/pci/host/pcie-designware.c                        | 10 ++++++----
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/pci/designware-pcie.txt b/Documentation/devicetree/bindings/pci/designware-pcie.txt
index 9f4faa8..0036ab3 100644
--- a/Documentation/devicetree/bindings/pci/designware-pcie.txt
+++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt
@@ -14,7 +14,6 @@ Required properties:
 - interrupt-map-mask and interrupt-map: standard PCI properties
 	to define the mapping of the PCIe interface to interrupt
 	numbers.
-- num-lanes: number of lanes to use
 - clocks: Must contain an entry for each entry in clock-names.
 	See ../clocks/clock-bindings.txt for details.
 - clock-names: Must include the following entries:
@@ -22,6 +21,8 @@ Required properties:
 	- "pcie_bus"
 
 Optional properties:
+- num-lanes: number of lanes to use (this property should be specified unless
+  the link is brought already up in BIOS)
 - reset-gpio: gpio pin number of power good signal
 - bus-range: PCI bus numbers covered (it is recommended for new devicetrees to
   specify this property, to keep backwards compatibility a range of 0x00-0xff
diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
index 52aa6e3..b0b40ed 100644
--- a/drivers/pci/host/pcie-designware.c
+++ b/drivers/pci/host/pcie-designware.c
@@ -482,10 +482,9 @@ int dw_pcie_host_init(struct pcie_port *pp)
 		}
 	}
 
-	if (of_property_read_u32(np, "num-lanes", &pp->lanes)) {
-		dev_err(pp->dev, "Failed to parse the number of lanes\n");
-		return -EINVAL;
-	}
+	ret = of_property_read_u32(np, "num-lanes", &pp->lanes);
+	if (ret)
+		pp->lanes = 0;
 
 	if (IS_ENABLED(CONFIG_PCI_MSI)) {
 		if (!pp->ops->msi_host_init) {
@@ -764,6 +763,9 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
 	case 8:
 		val |= PORT_LINK_MODE_8_LANES;
 		break;
+	default:
+		dev_err(pp->dev, "invalid pp->lanes value\n");
+		return;
 	}
 	dw_pcie_writel_rc(pp, val, PCIE_PORT_LINK_CONTROL);
 
-- 
1.9.1

Re: [PATCH] PCI: Designware: make num-lanes an optional DT property

[Bjorn Helgaas]

Hi Gabriele,

On Fri, Sep 11, 2015 at 01:02:26AM +0800, Gabriele Paoloni wrote:
> From: gabriele paoloni <gabriele.paoloni@huawei.com>
> 
> Currently num-lanes is read in dw_pcie_host_init().
> for A SoC that performs the link-up operation in UEFI num-lanes
> is not needed in the DTS but it has to be specified to any value;
> otherwise dw_pcie_host_init will fail.
> If the link is brought up in BIOS dw_pcie_setup_rc should not
> be called.
> This patch rework dw_pcie_host_init() so that, if num-lanes is
> not specified, it sets pp->lanes = 0. If later on dw_pcie_setup_rc
> is called and pp->num_lanes is not set to a valid value,
> dw_pcie_setup_rc will send an error message and return.

Which systems don't call dw_pcie_setup_rc()?  I don't have time for an
exhaustive search, but it looks like most or all designware drivers do call
dw_pcie_setup_rc(), so if any left out num-lanes, they would report an
error.

> This patch is a follow-up of
> http://www.spinics.net/lists/linux-pci/msg44394.html
> 
> Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com>
> ---
>  Documentation/devicetree/bindings/pci/designware-pcie.txt |  3 ++-
>  drivers/pci/host/pcie-designware.c                        | 10 ++++++----
>  2 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pci/designware-pcie.txt b/Documentation/devicetree/bindings/pci/designware-pcie.txt
> index 9f4faa8..0036ab3 100644
> --- a/Documentation/devicetree/bindings/pci/designware-pcie.txt
> +++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt
> @@ -14,7 +14,6 @@ Required properties:
>  - interrupt-map-mask and interrupt-map: standard PCI properties
>  	to define the mapping of the PCIe interface to interrupt
>  	numbers.
> -- num-lanes: number of lanes to use
>  - clocks: Must contain an entry for each entry in clock-names.
>  	See ../clocks/clock-bindings.txt for details.
>  - clock-names: Must include the following entries:
> @@ -22,6 +21,8 @@ Required properties:
>  	- "pcie_bus"
>  
>  Optional properties:
> +- num-lanes: number of lanes to use (this property should be specified unless
> +  the link is brought already up in BIOS)
>  - reset-gpio: gpio pin number of power good signal
>  - bus-range: PCI bus numbers covered (it is recommended for new devicetrees to
>    specify this property, to keep backwards compatibility a range of 0x00-0xff
> diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
> index 52aa6e3..b0b40ed 100644
> --- a/drivers/pci/host/pcie-designware.c
> +++ b/drivers/pci/host/pcie-designware.c
> @@ -482,10 +482,9 @@ int dw_pcie_host_init(struct pcie_port *pp)
>  		}
>  	}
>  
> -	if (of_property_read_u32(np, "num-lanes", &pp->lanes)) {
> -		dev_err(pp->dev, "Failed to parse the number of lanes\n");
> -		return -EINVAL;
> -	}
> +	ret = of_property_read_u32(np, "num-lanes", &pp->lanes);
> +	if (ret)
> +		pp->lanes = 0;
>  
>  	if (IS_ENABLED(CONFIG_PCI_MSI)) {
>  		if (!pp->ops->msi_host_init) {
> @@ -764,6 +763,9 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
>  	case 8:
>  		val |= PORT_LINK_MODE_8_LANES;
>  		break;
> +	default:
> +		dev_err(pp->dev, "invalid pp->lanes value\n");

Maybe print the invalid value here, labelled "num-lanes"?

Bjorn

> +		return;
>  	}
>  	dw_pcie_writel_rc(pp, val, PCIE_PORT_LINK_CONTROL);
>  
> -- 
> 1.9.1
> 
> --
> 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

RE: [PATCH] PCI: Designware: make num-lanes an optional DT property

[Gabriele Paoloni]

> -----Original Message-----
> From: Bjorn Helgaas [mailto:bhelgaas@google.com]
> Sent: Friday, September 18, 2015 11:55 PM
> To: Gabriele Paoloni
> Cc: jingoohan1@gmail.com; pratyush.anand@gmail.com; linux-
> pci@vger.kernel.org; Wangzhou (B); Yuanzhichang; Zhudacai; zhangjukuo;
> qiuzhenfa; Liguozhu (Kenneth)
> Subject: Re: [PATCH] PCI: Designware: make num-lanes an optional DT
> property
> 
> Hi Gabriele,
> 
> On Fri, Sep 11, 2015 at 01:02:26AM +0800, Gabriele Paoloni wrote:
> > From: gabriele paoloni <gabriele.paoloni@huawei.com>
> >
> > Currently num-lanes is read in dw_pcie_host_init().
> > for A SoC that performs the link-up operation in UEFI num-lanes
> > is not needed in the DTS but it has to be specified to any value;
> > otherwise dw_pcie_host_init will fail.
> > If the link is brought up in BIOS dw_pcie_setup_rc should not
> > be called.
> > This patch rework dw_pcie_host_init() so that, if num-lanes is
> > not specified, it sets pp->lanes = 0. If later on dw_pcie_setup_rc
> > is called and pp->num_lanes is not set to a valid value,
> > dw_pcie_setup_rc will send an error message and return.
> 
> Which systems don't call dw_pcie_setup_rc()?  I don't have time for an
> exhaustive search, but it looks like most or all designware drivers do
> call
> dw_pcie_setup_rc(), so if any left out num-lanes, they would report an
> error.

All of them currently call dw_pcie_setup_rc(), however the call is not needed
if the link-up is done in BIOS. For instance if you look at
exynos_pcie_establish_link(), you see that it will first check if the link is
already up; if so the function will return as the BIOS has already brought
up the link and dw_pcie_setup_rc is not called in this case. Therefore num-lanes
is not mandatory for a platform with link-up setup in BIOS.

> 
> > This patch is a follow-up of
> > http://www.spinics.net/lists/linux-pci/msg44394.html
> >
> > Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com>
> > ---
> >  Documentation/devicetree/bindings/pci/designware-pcie.txt |  3 ++-
> >  drivers/pci/host/pcie-designware.c                        | 10
> ++++++----
> >  2 files changed, 8 insertions(+), 5 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/pci/designware-
> pcie.txt b/Documentation/devicetree/bindings/pci/designware-pcie.txt
> > index 9f4faa8..0036ab3 100644
> > --- a/Documentation/devicetree/bindings/pci/designware-pcie.txt
> > +++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt
> > @@ -14,7 +14,6 @@ Required properties:
> >  - interrupt-map-mask and interrupt-map: standard PCI properties
> >  	to define the mapping of the PCIe interface to interrupt
> >  	numbers.
> > -- num-lanes: number of lanes to use
> >  - clocks: Must contain an entry for each entry in clock-names.
> >  	See ../clocks/clock-bindings.txt for details.
> >  - clock-names: Must include the following entries:
> > @@ -22,6 +21,8 @@ Required properties:
> >  	- "pcie_bus"
> >
> >  Optional properties:
> > +- num-lanes: number of lanes to use (this property should be
> specified unless
> > +  the link is brought already up in BIOS)
> >  - reset-gpio: gpio pin number of power good signal
> >  - bus-range: PCI bus numbers covered (it is recommended for new
> devicetrees to
> >    specify this property, to keep backwards compatibility a range of
> 0x00-0xff
> > diff --git a/drivers/pci/host/pcie-designware.c
> b/drivers/pci/host/pcie-designware.c
> > index 52aa6e3..b0b40ed 100644
> > --- a/drivers/pci/host/pcie-designware.c
> > +++ b/drivers/pci/host/pcie-designware.c
> > @@ -482,10 +482,9 @@ int dw_pcie_host_init(struct pcie_port *pp)
> >  		}
> >  	}
> >
> > -	if (of_property_read_u32(np, "num-lanes", &pp->lanes)) {
> > -		dev_err(pp->dev, "Failed to parse the number of lanes\n");
> > -		return -EINVAL;
> > -	}
> > +	ret = of_property_read_u32(np, "num-lanes", &pp->lanes);
> > +	if (ret)
> > +		pp->lanes = 0;
> >
> >  	if (IS_ENABLED(CONFIG_PCI_MSI)) {
> >  		if (!pp->ops->msi_host_init) {
> > @@ -764,6 +763,9 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
> >  	case 8:
> >  		val |= PORT_LINK_MODE_8_LANES;
> >  		break;
> > +	default:
> > +		dev_err(pp->dev, "invalid pp->lanes value\n");
> 
> Maybe print the invalid value here, labelled "num-lanes"?

Agreed, will fix in v2

> 
> Bjorn
> 
> > +		return;
> >  	}
> >  	dw_pcie_writel_rc(pp, val, PCIE_PORT_LINK_CONTROL);
> >
> > --
> > 1.9.1
> >
> > --
> > 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