From: Hou Zhiqiang <[email protected]>
On FSL Layerscape SoCs, the number of lanes assigned to PCIe
controller is not fixed, it is determined by the selected
SerDes protocol. The current num-lanes indicates the max lanes
PCIe controller can support up to, instead of the lanes assigned
to the PCIe controller. This can result in PCIe link training fail
after hot-reset.
Hou Zhiqiang (4):
dt-bindings: PCI: designware: Remove the num-lanes from Required
properties
PCI: dwc: Return directly when num-lanes is not found
ARM: dts: ls1021a: Remove num-lanes property from PCIe nodes
arm64: dts: fsl: Remove num-lanes property from PCIe nodes
Documentation/devicetree/bindings/pci/designware-pcie.txt | 1 -
arch/arm/boot/dts/ls1021a.dtsi | 2 --
arch/arm64/boot/dts/freescale/fsl-ls1012a.dtsi | 1 -
arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi | 3 ---
arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi | 6 ------
arch/arm64/boot/dts/freescale/fsl-ls1088a.dtsi | 3 ---
arch/arm64/boot/dts/freescale/fsl-ls208xa.dtsi | 4 ----
drivers/pci/controller/dwc/pcie-designware.c | 6 ++++--
8 files changed, 4 insertions(+), 22 deletions(-)
--
2.17.1
From: Hou Zhiqiang <[email protected]>
The num-lanes is not a mandatory property, e.g. on FSL
Layerscape SoCs, the PCIe link training is completed
automatically base on the selected SerDes protocol, it
doesn't need the num-lanes to set-up the link width.
It is previously in both Required and Optional properties,
let's remove it from the Required properties.
Signed-off-by: Hou Zhiqiang <[email protected]>
---
V2:
- Reworded the change log and subject.
- Fixed a typo in subject.
Documentation/devicetree/bindings/pci/designware-pcie.txt | 1 -
1 file changed, 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/pci/designware-pcie.txt b/Documentation/devicetree/bindings/pci/designware-pcie.txt
index 5561a1c060d0..bd880df39a79 100644
--- a/Documentation/devicetree/bindings/pci/designware-pcie.txt
+++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt
@@ -11,7 +11,6 @@ Required properties:
the ATU address space.
(The old way of getting the configuration address space from "ranges"
is deprecated and should be avoided.)
-- num-lanes: number of lanes to use
RC mode:
- #address-cells: set to <3>
- #size-cells: set to <2>
--
2.17.1
From: Hou Zhiqiang <[email protected]>
Remove the num-lanes to avoid the driver setting the link width.
On FSL Layerscape SoCs, the number of lanes assigned to PCIe
controller is not fixed, it is determined by the selected SerDes
protocol in the RCW (Reset Configuration Word), and the PCIe link
training is completed automatically base on the selected SerDes
protocol, and the link width set-up is updated by hardware after
power on reset. So the num-lanes is not needed for Layerscape PCIe.
The current num-lanes was added erroneously, which actually indicates
the max lanes PCIe controller can support up to, instead of the lanes
assigned to the PCIe controller. And the link width set by SerDes
protocol will be overridden by the num-lanes, hence the subsequent
re-taining will fail when the assigned lanes does not equal to
num-lanes.
Signed-off-by: Hou Zhiqiang <[email protected]>
Reviewed-by: Andrew Murray <[email protected]>
---
V2:
- Reworded the change log.
arch/arm/boot/dts/ls1021a.dtsi | 2 --
1 file changed, 2 deletions(-)
diff --git a/arch/arm/boot/dts/ls1021a.dtsi b/arch/arm/boot/dts/ls1021a.dtsi
index 464df4290ffc..2f6977ada447 100644
--- a/arch/arm/boot/dts/ls1021a.dtsi
+++ b/arch/arm/boot/dts/ls1021a.dtsi
@@ -874,7 +874,6 @@
#address-cells = <3>;
#size-cells = <2>;
device_type = "pci";
- num-lanes = <4>;
num-viewport = <6>;
bus-range = <0x0 0xff>;
ranges = <0x81000000 0x0 0x00000000 0x40 0x00010000 0x0 0x00010000 /* downstream I/O */
@@ -899,7 +898,6 @@
#address-cells = <3>;
#size-cells = <2>;
device_type = "pci";
- num-lanes = <4>;
num-viewport = <6>;
bus-range = <0x0 0xff>;
ranges = <0x81000000 0x0 0x00000000 0x48 0x00010000 0x0 0x00010000 /* downstream I/O */
--
2.17.1
From: Hou Zhiqiang <[email protected]>
Remove the num-lanes to avoid the driver setting the link width.
On FSL Layerscape SoCs, the number of lanes assigned to PCIe
controller is not fixed, it is determined by the selected SerDes
protocol in the RCW (Reset Configuration Word), and the PCIe link
training is completed automatically base on the selected SerDes
protocol, and the link width set-up is updated by hardware after
power on reset. So the num-lanes is not needed for Layerscape PCIe.
The current num-lanes was added erroneously, which actually indicates
the max lanes PCIe controller can support up to, instead of the lanes
assigned to the PCIe controller. And the link width set by SerDes
protocol will be overridden by the num-lanes, hence the subsequent
re-taining will fail when the assigned lanes does not equal to
num-lanes.
Signed-off-by: Hou Zhiqiang <[email protected]>
Reviewed-by: Andrew Murray <[email protected]>
---
V2:
- Reworded the change log.
arch/arm64/boot/dts/freescale/fsl-ls1012a.dtsi | 1 -
arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi | 3 ---
arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi | 6 ------
arch/arm64/boot/dts/freescale/fsl-ls1088a.dtsi | 3 ---
arch/arm64/boot/dts/freescale/fsl-ls208xa.dtsi | 4 ----
5 files changed, 17 deletions(-)
diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1012a.dtsi b/arch/arm64/boot/dts/freescale/fsl-ls1012a.dtsi
index ec6257a5b251..119c597ca867 100644
--- a/arch/arm64/boot/dts/freescale/fsl-ls1012a.dtsi
+++ b/arch/arm64/boot/dts/freescale/fsl-ls1012a.dtsi
@@ -486,7 +486,6 @@
#address-cells = <3>;
#size-cells = <2>;
device_type = "pci";
- num-lanes = <4>;
num-viewport = <2>;
bus-range = <0x0 0xff>;
ranges = <0x81000000 0x0 0x00000000 0x40 0x00010000 0x0 0x00010000 /* downstream I/O */
diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi b/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi
index 71d9ed9ff985..c084c7a4b6a6 100644
--- a/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi
+++ b/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi
@@ -677,7 +677,6 @@
#size-cells = <2>;
device_type = "pci";
dma-coherent;
- num-lanes = <4>;
num-viewport = <6>;
bus-range = <0x0 0xff>;
ranges = <0x81000000 0x0 0x00000000 0x40 0x00010000 0x0 0x00010000 /* downstream I/O */
@@ -704,7 +703,6 @@
#size-cells = <2>;
device_type = "pci";
dma-coherent;
- num-lanes = <2>;
num-viewport = <6>;
bus-range = <0x0 0xff>;
ranges = <0x81000000 0x0 0x00000000 0x48 0x00010000 0x0 0x00010000 /* downstream I/O */
@@ -731,7 +729,6 @@
#size-cells = <2>;
device_type = "pci";
dma-coherent;
- num-lanes = <2>;
num-viewport = <6>;
bus-range = <0x0 0xff>;
ranges = <0x81000000 0x0 0x00000000 0x50 0x00010000 0x0 0x00010000 /* downstream I/O */
diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi b/arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi
index b0ef08b090dd..d4c1da3d4bde 100644
--- a/arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi
+++ b/arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi
@@ -649,7 +649,6 @@
#size-cells = <2>;
device_type = "pci";
dma-coherent;
- num-lanes = <4>;
num-viewport = <8>;
bus-range = <0x0 0xff>;
ranges = <0x81000000 0x0 0x00000000 0x40 0x00010000 0x0 0x00010000 /* downstream I/O */
@@ -671,7 +670,6 @@
reg-names = "regs", "addr_space";
num-ib-windows = <6>;
num-ob-windows = <8>;
- num-lanes = <2>;
status = "disabled";
};
@@ -687,7 +685,6 @@
#size-cells = <2>;
device_type = "pci";
dma-coherent;
- num-lanes = <2>;
num-viewport = <8>;
bus-range = <0x0 0xff>;
ranges = <0x81000000 0x0 0x00000000 0x48 0x00010000 0x0 0x00010000 /* downstream I/O */
@@ -709,7 +706,6 @@
reg-names = "regs", "addr_space";
num-ib-windows = <6>;
num-ob-windows = <8>;
- num-lanes = <2>;
status = "disabled";
};
@@ -725,7 +721,6 @@
#size-cells = <2>;
device_type = "pci";
dma-coherent;
- num-lanes = <2>;
num-viewport = <8>;
bus-range = <0x0 0xff>;
ranges = <0x81000000 0x0 0x00000000 0x50 0x00010000 0x0 0x00010000 /* downstream I/O */
@@ -747,7 +742,6 @@
reg-names = "regs", "addr_space";
num-ib-windows = <6>;
num-ob-windows = <8>;
- num-lanes = <2>;
status = "disabled";
};
diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1088a.dtsi b/arch/arm64/boot/dts/freescale/fsl-ls1088a.dtsi
index dfbead405783..76c87afeba1e 100644
--- a/arch/arm64/boot/dts/freescale/fsl-ls1088a.dtsi
+++ b/arch/arm64/boot/dts/freescale/fsl-ls1088a.dtsi
@@ -456,7 +456,6 @@
#size-cells = <2>;
device_type = "pci";
dma-coherent;
- num-lanes = <4>;
num-viewport = <256>;
bus-range = <0x0 0xff>;
ranges = <0x81000000 0x0 0x00000000 0x20 0x00010000 0x0 0x00010000 /* downstream I/O */
@@ -482,7 +481,6 @@
#size-cells = <2>;
device_type = "pci";
dma-coherent;
- num-lanes = <4>;
num-viewport = <6>;
bus-range = <0x0 0xff>;
ranges = <0x81000000 0x0 0x00000000 0x28 0x00010000 0x0 0x00010000 /* downstream I/O */
@@ -508,7 +506,6 @@
#size-cells = <2>;
device_type = "pci";
dma-coherent;
- num-lanes = <8>;
num-viewport = <6>;
bus-range = <0x0 0xff>;
ranges = <0x81000000 0x0 0x00000000 0x30 0x00010000 0x0 0x00010000 /* downstream I/O */
diff --git a/arch/arm64/boot/dts/freescale/fsl-ls208xa.dtsi b/arch/arm64/boot/dts/freescale/fsl-ls208xa.dtsi
index 64101c9962ce..7a0be8eaa84a 100644
--- a/arch/arm64/boot/dts/freescale/fsl-ls208xa.dtsi
+++ b/arch/arm64/boot/dts/freescale/fsl-ls208xa.dtsi
@@ -639,7 +639,6 @@
#size-cells = <2>;
device_type = "pci";
dma-coherent;
- num-lanes = <4>;
num-viewport = <6>;
bus-range = <0x0 0xff>;
msi-parent = <&its>;
@@ -661,7 +660,6 @@
#size-cells = <2>;
device_type = "pci";
dma-coherent;
- num-lanes = <4>;
num-viewport = <6>;
bus-range = <0x0 0xff>;
msi-parent = <&its>;
@@ -683,7 +681,6 @@
#size-cells = <2>;
device_type = "pci";
dma-coherent;
- num-lanes = <8>;
num-viewport = <256>;
bus-range = <0x0 0xff>;
msi-parent = <&its>;
@@ -705,7 +702,6 @@
#size-cells = <2>;
device_type = "pci";
dma-coherent;
- num-lanes = <4>;
num-viewport = <6>;
bus-range = <0x0 0xff>;
msi-parent = <&its>;
--
2.17.1
From: Hou Zhiqiang <[email protected]>
The num-lanes is optional, so probably it isn't added
on some platforms. The subsequent programming is base
on the num-lanes, hence return when it is not found.
Signed-off-by: Hou Zhiqiang <[email protected]>
Reviewed-by: Andrew Murray <[email protected]>
---
V2:
- No change.
drivers/pci/controller/dwc/pcie-designware.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
index 7d25102c304c..0a89bfd1636e 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -423,8 +423,10 @@ void dw_pcie_setup(struct dw_pcie *pci)
ret = of_property_read_u32(np, "num-lanes", &lanes);
- if (ret)
- lanes = 0;
+ if (ret) {
+ dev_dbg(pci->dev, "property num-lanes isn't found\n");
+ return;
+ }
/* Set the number of lanes */
val = dw_pcie_readl_dbi(pci, PCIE_PORT_LINK_CONTROL);
--
2.17.1
On Tue, Aug 20, 2019 at 07:28:43AM +0000, Z.q. Hou wrote:
> From: Hou Zhiqiang <[email protected]>
>
> The num-lanes is not a mandatory property, e.g. on FSL
> Layerscape SoCs, the PCIe link training is completed
> automatically base on the selected SerDes protocol, it
> doesn't need the num-lanes to set-up the link width.
>
> It is previously in both Required and Optional properties,
> let's remove it from the Required properties.
>
> Signed-off-by: Hou Zhiqiang <[email protected]>
> ---
Reviewed-by: Andrew Murray <[email protected]>
> V2:
> - Reworded the change log and subject.
> - Fixed a typo in subject.
>
> Documentation/devicetree/bindings/pci/designware-pcie.txt | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/pci/designware-pcie.txt b/Documentation/devicetree/bindings/pci/designware-pcie.txt
> index 5561a1c060d0..bd880df39a79 100644
> --- a/Documentation/devicetree/bindings/pci/designware-pcie.txt
> +++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt
> @@ -11,7 +11,6 @@ Required properties:
> the ATU address space.
> (The old way of getting the configuration address space from "ranges"
> is deprecated and should be avoided.)
> -- num-lanes: number of lanes to use
> RC mode:
> - #address-cells: set to <3>
> - #size-cells: set to <2>
> --
> 2.17.1
>
Hi Andrew,
Thanks a lot for your review!
Thanks,
Zhiqiang
> -----Original Message-----
> From: Andrew Murray <[email protected]>
> Sent: 2019??8??20?? 17:23
> To: Z.q. Hou <[email protected]>
> Cc: [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; Leo Li
> <[email protected]>; [email protected]; M.h. Lian
> <[email protected]>
> Subject: Re: [PATCHv2 1/4] dt-bindings: PCI: designware: Remove the
> num-lanes from Required properties
>
> On Tue, Aug 20, 2019 at 07:28:43AM +0000, Z.q. Hou wrote:
> > From: Hou Zhiqiang <[email protected]>
> >
> > The num-lanes is not a mandatory property, e.g. on FSL Layerscape
> > SoCs, the PCIe link training is completed automatically base on the
> > selected SerDes protocol, it doesn't need the num-lanes to set-up the
> > link width.
> >
> > It is previously in both Required and Optional properties, let's
> > remove it from the Required properties.
> >
> > Signed-off-by: Hou Zhiqiang <[email protected]>
> > ---
>
> Reviewed-by: Andrew Murray <[email protected]>
>
>
> > V2:
> > - Reworded the change log and subject.
> > - Fixed a typo in subject.
> >
> > Documentation/devicetree/bindings/pci/designware-pcie.txt | 1 -
> > 1 file changed, 1 deletion(-)
> >
> > diff --git a/Documentation/devicetree/bindings/pci/designware-pcie.txt
> > b/Documentation/devicetree/bindings/pci/designware-pcie.txt
> > index 5561a1c060d0..bd880df39a79 100644
> > --- a/Documentation/devicetree/bindings/pci/designware-pcie.txt
> > +++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt
> > @@ -11,7 +11,6 @@ Required properties:
> > the ATU address space.
> > (The old way of getting the configuration address space from
> "ranges"
> > is deprecated and should be avoided.)
> > -- num-lanes: number of lanes to use
> > RC mode:
> > - #address-cells: set to <3>
> > - #size-cells: set to <2>
> > --
> > 2.17.1
> >
On Tue, Aug 20, 2019 at 07:28:37AM +0000, Z.q. Hou wrote:
> From: Hou Zhiqiang <[email protected]>
>
> On FSL Layerscape SoCs, the number of lanes assigned to PCIe
> controller is not fixed, it is determined by the selected
> SerDes protocol. The current num-lanes indicates the max lanes
> PCIe controller can support up to, instead of the lanes assigned
> to the PCIe controller. This can result in PCIe link training fail
> after hot-reset.
>
> Hou Zhiqiang (4):
> dt-bindings: PCI: designware: Remove the num-lanes from Required
> properties
> PCI: dwc: Return directly when num-lanes is not found
> ARM: dts: ls1021a: Remove num-lanes property from PCIe nodes
> arm64: dts: fsl: Remove num-lanes property from PCIe nodes
>
> Documentation/devicetree/bindings/pci/designware-pcie.txt | 1 -
> arch/arm/boot/dts/ls1021a.dtsi | 2 --
> arch/arm64/boot/dts/freescale/fsl-ls1012a.dtsi | 1 -
> arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi | 3 ---
> arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi | 6 ------
> arch/arm64/boot/dts/freescale/fsl-ls1088a.dtsi | 3 ---
> arch/arm64/boot/dts/freescale/fsl-ls208xa.dtsi | 4 ----
> drivers/pci/controller/dwc/pcie-designware.c | 6 ++++--
> 8 files changed, 4 insertions(+), 22 deletions(-)
What a mess.
I am going to apply these but first if anyone can explain to
me what commit 907fce090253 was _supposed_ to to I would
be grateful, I read it multiple times but I still have not
understood it. This series does the right thing but why things
are they way they are in the mainline honestly I have no
idea, this does not make any sense in the slightest:
ret = of_property_read_u32(np, "num-lanes", &lanes);
if (ret)
lanes = 0;
/* Set the number of lanes */
val = dw_pcie_readl_dbi(pci, PCIE_PORT_LINK_CONTROL);
val &= ~PORT_LINK_MODE_MASK;
switch (lanes) {
case 1:
val |= PORT_LINK_MODE_1_LANES;
break;
case 2:
val |= PORT_LINK_MODE_2_LANES;
break;
case 4:
val |= PORT_LINK_MODE_4_LANES;
break;
case 8:
val |= PORT_LINK_MODE_8_LANES;
break;
default:
dev_err(pci->dev, "num-lanes %u: invalid value\n", lanes);
return;
}
why do we need to set lanes to 0 if num-lanes is not present ? To print
an error message ?
I really do not understand this code.
Lorenzo
On Tue, Aug 20, 2019 at 07:28:43AM +0000, Z.q. Hou wrote:
> From: Hou Zhiqiang <[email protected]>
>
> The num-lanes is not a mandatory property, e.g. on FSL
> Layerscape SoCs, the PCIe link training is completed
> automatically base on the selected SerDes protocol, it
> doesn't need the num-lanes to set-up the link width.
>
> It is previously in both Required and Optional properties,
> let's remove it from the Required properties.
>
> Signed-off-by: Hou Zhiqiang <[email protected]>
> ---
> V2:
> - Reworded the change log and subject.
> - Fixed a typo in subject.
>
> Documentation/devicetree/bindings/pci/designware-pcie.txt | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/pci/designware-pcie.txt b/Documentation/devicetree/bindings/pci/designware-pcie.txt
> index 5561a1c060d0..bd880df39a79 100644
> --- a/Documentation/devicetree/bindings/pci/designware-pcie.txt
> +++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt
> @@ -11,7 +11,6 @@ Required properties:
> the ATU address space.
> (The old way of getting the configuration address space from "ranges"
> is deprecated and should be avoided.)
> -- num-lanes: number of lanes to use
> RC mode:
> - #address-cells: set to <3>
> - #size-cells: set to <2>
The patch is fine but if I were to be picky, we should update the
"Optional" entry to remove the "this property should be specified
unless the link is brought already up in BIOS" AFAIK in this HW
"BIOS" does not really play a role (and honestly the sentence above
is vague enough to make it useless if not harmful in a DT binding).
Lorenzo
On Thu, Aug 22, 2019 at 05:48:15PM +0100, Lorenzo Pieralisi wrote:
> On Tue, Aug 20, 2019 at 07:28:37AM +0000, Z.q. Hou wrote:
> > From: Hou Zhiqiang <[email protected]>
> >
> > On FSL Layerscape SoCs, the number of lanes assigned to PCIe
> > controller is not fixed, it is determined by the selected
> > SerDes protocol. The current num-lanes indicates the max lanes
> > PCIe controller can support up to, instead of the lanes assigned
> > to the PCIe controller. This can result in PCIe link training fail
> > after hot-reset.
> >
> > Hou Zhiqiang (4):
> > dt-bindings: PCI: designware: Remove the num-lanes from Required
> > properties
> > PCI: dwc: Return directly when num-lanes is not found
> > ARM: dts: ls1021a: Remove num-lanes property from PCIe nodes
> > arm64: dts: fsl: Remove num-lanes property from PCIe nodes
> >
> > Documentation/devicetree/bindings/pci/designware-pcie.txt | 1 -
> > arch/arm/boot/dts/ls1021a.dtsi | 2 --
> > arch/arm64/boot/dts/freescale/fsl-ls1012a.dtsi | 1 -
> > arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi | 3 ---
> > arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi | 6 ------
> > arch/arm64/boot/dts/freescale/fsl-ls1088a.dtsi | 3 ---
> > arch/arm64/boot/dts/freescale/fsl-ls208xa.dtsi | 4 ----
> > drivers/pci/controller/dwc/pcie-designware.c | 6 ++++--
> > 8 files changed, 4 insertions(+), 22 deletions(-)
>
> What a mess.
>
> I am going to apply these but first if anyone can explain to
> me what commit 907fce090253 was _supposed_ to to I would
> be grateful, I read it multiple times but I still have not
> understood it. This series does the right thing but why things
The DWC controller drivers all implement a .host_init callback -
some of the drivers choose to call dw_pcie_setup_rc from their
callback which, amongst other things will set up/train the link.
As far as I can tell, dw_pcie_setup_rc is the only user of pp->lanes.
Therefore for hardware where the link is already set up by firmware
and thus dw_pcie_setup_rc is never called - it is unnecessary to
read the DT value for pp->lanes. So the first hunk in 907fce090253
gets rid of the error and makes the num-lanes property optional.
However this opens up the possibility of a DT misconfiguration for
other controllers that do call dw_pcie_setup_rc, i.e. they set
num-lanes to 0 when it is required. Therefore the second hunk
ensures that an error is emitted when num-lanes was needed but not
provided.
> are they way they are in the mainline honestly I have no
> idea, this does not make any sense in the slightest:
>
> ret = of_property_read_u32(np, "num-lanes", &lanes);
> if (ret)
> lanes = 0;
Please note that the code below is in a different function to the
code above.
>
> /* Set the number of lanes */
> val = dw_pcie_readl_dbi(pci, PCIE_PORT_LINK_CONTROL);
> val &= ~PORT_LINK_MODE_MASK;
> switch (lanes) {
> case 1:
> val |= PORT_LINK_MODE_1_LANES;
> break;
> case 2:
> val |= PORT_LINK_MODE_2_LANES;
> break;
> case 4:
> val |= PORT_LINK_MODE_4_LANES;
> break;
> case 8:
> val |= PORT_LINK_MODE_8_LANES;
> break;
> default:
> dev_err(pci->dev, "num-lanes %u: invalid value\n", lanes);
> return;
> }
>
> why do we need to set lanes to 0 if num-lanes is not present ? To print
> an error message ?
At this point in time, the controller is trying to train the link but
it doesn't know how many lanes, so we need to error. We don't error when
reading the device tree earlier - because at that point in time we don't
know if num-lanes is optional or not.
Thanks,
Andrew Murray
>
> I really do not understand this code.
>
> Lorenzo
On Fri, Aug 23, 2019 at 10:44:25AM +0100, Andrew Murray wrote:
> On Thu, Aug 22, 2019 at 05:48:15PM +0100, Lorenzo Pieralisi wrote:
> > On Tue, Aug 20, 2019 at 07:28:37AM +0000, Z.q. Hou wrote:
> > > From: Hou Zhiqiang <[email protected]>
> > >
> > > On FSL Layerscape SoCs, the number of lanes assigned to PCIe
> > > controller is not fixed, it is determined by the selected
> > > SerDes protocol. The current num-lanes indicates the max lanes
> > > PCIe controller can support up to, instead of the lanes assigned
> > > to the PCIe controller. This can result in PCIe link training fail
> > > after hot-reset.
> > >
> > > Hou Zhiqiang (4):
> > > dt-bindings: PCI: designware: Remove the num-lanes from Required
> > > properties
> > > PCI: dwc: Return directly when num-lanes is not found
> > > ARM: dts: ls1021a: Remove num-lanes property from PCIe nodes
> > > arm64: dts: fsl: Remove num-lanes property from PCIe nodes
> > >
> > > Documentation/devicetree/bindings/pci/designware-pcie.txt | 1 -
> > > arch/arm/boot/dts/ls1021a.dtsi | 2 --
> > > arch/arm64/boot/dts/freescale/fsl-ls1012a.dtsi | 1 -
> > > arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi | 3 ---
> > > arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi | 6 ------
> > > arch/arm64/boot/dts/freescale/fsl-ls1088a.dtsi | 3 ---
> > > arch/arm64/boot/dts/freescale/fsl-ls208xa.dtsi | 4 ----
> > > drivers/pci/controller/dwc/pcie-designware.c | 6 ++++--
> > > 8 files changed, 4 insertions(+), 22 deletions(-)
> >
> > What a mess.
> >
> > I am going to apply these but first if anyone can explain to
> > me what commit 907fce090253 was _supposed_ to to I would
> > be grateful, I read it multiple times but I still have not
> > understood it. This series does the right thing but why things
>
> The DWC controller drivers all implement a .host_init callback -
> some of the drivers choose to call dw_pcie_setup_rc from their
> callback which, amongst other things will set up/train the link.
>
> As far as I can tell, dw_pcie_setup_rc is the only user of pp->lanes.
> Therefore for hardware where the link is already set up by firmware
> and thus dw_pcie_setup_rc is never called - it is unnecessary to
> read the DT value for pp->lanes. So the first hunk in 907fce090253
> gets rid of the error and makes the num-lanes property optional.
>
> However this opens up the possibility of a DT misconfiguration for
> other controllers that do call dw_pcie_setup_rc, i.e. they set
> num-lanes to 0 when it is required. Therefore the second hunk
> ensures that an error is emitted when num-lanes was needed but not
> provided.
Yes, the problem is not 907fce090253, it is subsequent changes
(ie feb85d9b1c47 AFAICS).
> > are they way they are in the mainline honestly I have no
> > idea, this does not make any sense in the slightest:
> >
> > ret = of_property_read_u32(np, "num-lanes", &lanes);
> > if (ret)
> > lanes = 0;
>
> Please note that the code below is in a different function to the
> code above.
In the mainline kernel they are in the same function
ie dw_pcie_setup() and as reported here current code
does not make any sense.
Anyway merging these patches, thanks for having a look.
Lorenzo
> > /* Set the number of lanes */
> > val = dw_pcie_readl_dbi(pci, PCIE_PORT_LINK_CONTROL);
> > val &= ~PORT_LINK_MODE_MASK;
> > switch (lanes) {
> > case 1:
> > val |= PORT_LINK_MODE_1_LANES;
> > break;
> > case 2:
> > val |= PORT_LINK_MODE_2_LANES;
> > break;
> > case 4:
> > val |= PORT_LINK_MODE_4_LANES;
> > break;
> > case 8:
> > val |= PORT_LINK_MODE_8_LANES;
> > break;
> > default:
> > dev_err(pci->dev, "num-lanes %u: invalid value\n", lanes);
> > return;
> > }
> >
> > why do we need to set lanes to 0 if num-lanes is not present ? To print
> > an error message ?
>
> At this point in time, the controller is trying to train the link but
> it doesn't know how many lanes, so we need to error. We don't error when
> reading the device tree earlier - because at that point in time we don't
> know if num-lanes is optional or not.
>
> Thanks,
>
> Andrew Murray
>
> >
> > I really do not understand this code.
> >
> > Lorenzo
On Tue, Aug 20, 2019 at 07:28:37AM +0000, Z.q. Hou wrote:
> From: Hou Zhiqiang <[email protected]>
>
> On FSL Layerscape SoCs, the number of lanes assigned to PCIe
> controller is not fixed, it is determined by the selected
> SerDes protocol. The current num-lanes indicates the max lanes
> PCIe controller can support up to, instead of the lanes assigned
> to the PCIe controller. This can result in PCIe link training fail
> after hot-reset.
>
> Hou Zhiqiang (4):
> dt-bindings: PCI: designware: Remove the num-lanes from Required
> properties
> PCI: dwc: Return directly when num-lanes is not found
> ARM: dts: ls1021a: Remove num-lanes property from PCIe nodes
> arm64: dts: fsl: Remove num-lanes property from PCIe nodes
>
> Documentation/devicetree/bindings/pci/designware-pcie.txt | 1 -
> arch/arm/boot/dts/ls1021a.dtsi | 2 --
> arch/arm64/boot/dts/freescale/fsl-ls1012a.dtsi | 1 -
> arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi | 3 ---
> arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi | 6 ------
> arch/arm64/boot/dts/freescale/fsl-ls1088a.dtsi | 3 ---
> arch/arm64/boot/dts/freescale/fsl-ls208xa.dtsi | 4 ----
> drivers/pci/controller/dwc/pcie-designware.c | 6 ++++--
> 8 files changed, 4 insertions(+), 22 deletions(-)
Applied to pci/dwc for v5.4, thanks.
Lorenzo
On Tue, 20 Aug 2019 07:28:43 +0000, "Z.q. Hou" wrote:
> From: Hou Zhiqiang <[email protected]>
>
> The num-lanes is not a mandatory property, e.g. on FSL
> Layerscape SoCs, the PCIe link training is completed
> automatically base on the selected SerDes protocol, it
> doesn't need the num-lanes to set-up the link width.
>
> It is previously in both Required and Optional properties,
> let's remove it from the Required properties.
>
> Signed-off-by: Hou Zhiqiang <[email protected]>
> ---
> V2:
> - Reworded the change log and subject.
> - Fixed a typo in subject.
>
> Documentation/devicetree/bindings/pci/designware-pcie.txt | 1 -
> 1 file changed, 1 deletion(-)
>
Reviewed-by: Rob Herring <[email protected]>
Hi Rob,
Thanks a lot for your review!
Thanks,
Zhiqiang
> -----Original Message-----
> From: Rob Herring <[email protected]>
> Sent: 2019??8??28?? 0:58
> To: Z.q. Hou <[email protected]>
> Cc: [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; Leo Li
> <[email protected]>; [email protected];
> [email protected]; M.h. Lian <[email protected]>; Z.q. Hou
> <[email protected]>
> Subject: Re: [PATCHv2 1/4] dt-bindings: PCI: designware: Remove the
> num-lanes from Required properties
>
> On Tue, 20 Aug 2019 07:28:43 +0000, "Z.q. Hou" wrote:
> > From: Hou Zhiqiang <[email protected]>
> >
> > The num-lanes is not a mandatory property, e.g. on FSL Layerscape
> > SoCs, the PCIe link training is completed automatically base on the
> > selected SerDes protocol, it doesn't need the num-lanes to set-up the
> > link width.
> >
> > It is previously in both Required and Optional properties, let's
> > remove it from the Required properties.
> >
> > Signed-off-by: Hou Zhiqiang <[email protected]>
> > ---
> > V2:
> > - Reworded the change log and subject.
> > - Fixed a typo in subject.
> >
> > Documentation/devicetree/bindings/pci/designware-pcie.txt | 1 -
> > 1 file changed, 1 deletion(-)
> >
>
> Reviewed-by: Rob Herring <[email protected]>