| Message ID | 20220222155030.988-5-pali@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | PCI: mvebu: subsystem ids, AER and INTx | expand |
On Tue, Feb 22, 2022 at 04:50:22PM +0100, Pali Rohár wrote: > Controller driver needs to correctly configure PCIe link if it contains 1 > or 4 SerDes PCIe lanes. Therefore add a new 'num-lanes' DT property for > mvebu PCIe controller. Property 'num-lanes' seems to be de-facto standard > way how number of lanes is specified in other PCIe controllers. > > Signed-off-by: Pali Rohár <pali@kernel.org> > Acked-by: Rob Herring <robh@kernel.org> > --- > Documentation/devicetree/bindings/pci/mvebu-pci.txt | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/Documentation/devicetree/bindings/pci/mvebu-pci.txt b/Documentation/devicetree/bindings/pci/mvebu-pci.txt > index 6173af6885f8..24225852bce0 100644 > --- a/Documentation/devicetree/bindings/pci/mvebu-pci.txt > +++ b/Documentation/devicetree/bindings/pci/mvebu-pci.txt > @@ -77,6 +77,7 @@ and the following optional properties: > - marvell,pcie-lane: the physical PCIe lane number, for ports having > multiple lanes. If this property is not found, we assume that the > value is 0. > +- num-lanes: number of SerDes PCIe lanes for this link (1 or 4) > - reset-gpios: optional GPIO to PERST# > - reset-delay-us: delay in us to wait after reset de-assertion, if not > specified will default to 100ms, as required by the PCIe specification. > @@ -141,6 +142,7 @@ pcie-controller { > interrupt-map = <0 0 0 0 &mpic 58>; > marvell,pcie-port = <0>; > marvell,pcie-lane = <0>; > + num-lanes = <1>; Is this patch really necessary? AFAICS, the related driver change only sets "port->is_x4 = true" when "num-lanes = <4>", and in all other cases it defaults to a Max Link Width of 1: lnkcap |= (port->is_x4 ? 4 : 1) << 4; I don't see the point of adding a value that we don't validate or do anything with. E.g., I don't see an error message that would catch "num-lanes = <3>". Bjorn
On Thursday 24 February 2022 18:02:26 Bjorn Helgaas wrote: > On Tue, Feb 22, 2022 at 04:50:22PM +0100, Pali Rohár wrote: > > Controller driver needs to correctly configure PCIe link if it contains 1 > > or 4 SerDes PCIe lanes. Therefore add a new 'num-lanes' DT property for > > mvebu PCIe controller. Property 'num-lanes' seems to be de-facto standard > > way how number of lanes is specified in other PCIe controllers. > > > > Signed-off-by: Pali Rohár <pali@kernel.org> > > Acked-by: Rob Herring <robh@kernel.org> > > --- > > Documentation/devicetree/bindings/pci/mvebu-pci.txt | 11 +++++++++++ > > 1 file changed, 11 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/pci/mvebu-pci.txt b/Documentation/devicetree/bindings/pci/mvebu-pci.txt > > index 6173af6885f8..24225852bce0 100644 > > --- a/Documentation/devicetree/bindings/pci/mvebu-pci.txt > > +++ b/Documentation/devicetree/bindings/pci/mvebu-pci.txt > > @@ -77,6 +77,7 @@ and the following optional properties: > > - marvell,pcie-lane: the physical PCIe lane number, for ports having > > multiple lanes. If this property is not found, we assume that the > > value is 0. > > +- num-lanes: number of SerDes PCIe lanes for this link (1 or 4) > > - reset-gpios: optional GPIO to PERST# > > - reset-delay-us: delay in us to wait after reset de-assertion, if not > > specified will default to 100ms, as required by the PCIe specification. > > @@ -141,6 +142,7 @@ pcie-controller { > > interrupt-map = <0 0 0 0 &mpic 58>; > > marvell,pcie-port = <0>; > > marvell,pcie-lane = <0>; > > + num-lanes = <1>; > > Is this patch really necessary? This is just documentation patch. And I think that documentation is always important. > AFAICS, the related driver change > only sets "port->is_x4 = true" when "num-lanes = <4>", and in all > other cases it defaults to a Max Link Width of 1: > > lnkcap |= (port->is_x4 ? 4 : 1) << 4; Yes! And this registers configures number of lanes in HW. > I don't see the point of adding a value that we don't validate or do > anything with. E.g., I don't see an error message that would catch > "num-lanes = <3>". > > Bjorn In past I was told that kernel should not do validation of DT properties and it is job of some DT schema validation. That is why I did not added code into kernel which show error message when value different than 1 and 4 is specified in DT. But issue here is that there is no DT schema for pci-mvebu as above .txt file was not converted to YAML schema yet. This is something which should be improved...
On Fri, Feb 25, 2022 at 01:58:10PM +0100, Pali Rohár wrote: > On Thursday 24 February 2022 18:02:26 Bjorn Helgaas wrote: > > On Tue, Feb 22, 2022 at 04:50:22PM +0100, Pali Rohár wrote: > > > Controller driver needs to correctly configure PCIe link if it contains 1 > > > or 4 SerDes PCIe lanes. Therefore add a new 'num-lanes' DT property for > > > mvebu PCIe controller. Property 'num-lanes' seems to be de-facto standard > > > way how number of lanes is specified in other PCIe controllers. > > > > > > Signed-off-by: Pali Rohár <pali@kernel.org> > > > Acked-by: Rob Herring <robh@kernel.org> > > > --- > > > Documentation/devicetree/bindings/pci/mvebu-pci.txt | 11 +++++++++++ > > > 1 file changed, 11 insertions(+) > > > > > > diff --git a/Documentation/devicetree/bindings/pci/mvebu-pci.txt b/Documentation/devicetree/bindings/pci/mvebu-pci.txt > > > index 6173af6885f8..24225852bce0 100644 > > > --- a/Documentation/devicetree/bindings/pci/mvebu-pci.txt > > > +++ b/Documentation/devicetree/bindings/pci/mvebu-pci.txt > > > @@ -77,6 +77,7 @@ and the following optional properties: > > > - marvell,pcie-lane: the physical PCIe lane number, for ports having > > > multiple lanes. If this property is not found, we assume that the > > > value is 0. > > > +- num-lanes: number of SerDes PCIe lanes for this link (1 or 4) > > > - reset-gpios: optional GPIO to PERST# > > > - reset-delay-us: delay in us to wait after reset de-assertion, if not > > > specified will default to 100ms, as required by the PCIe specification. > > > @@ -141,6 +142,7 @@ pcie-controller { > > > interrupt-map = <0 0 0 0 &mpic 58>; > > > marvell,pcie-port = <0>; > > > marvell,pcie-lane = <0>; > > > + num-lanes = <1>; > > > > Is this patch really necessary? > > This is just documentation patch. And I think that documentation is > always important. > > > AFAICS, the related driver change > > only sets "port->is_x4 = true" when "num-lanes = <4>", and in all > > other cases it defaults to a Max Link Width of 1: > > > > lnkcap |= (port->is_x4 ? 4 : 1) << 4; > > Yes! > > And this registers configures number of lanes in HW. > > > I don't see the point of adding a value that we don't validate or do > > anything with. E.g., I don't see an error message that would catch > > "num-lanes = <3>". > > > > Bjorn > > In past I was told that kernel should not do validation of DT properties > and it is job of some DT schema validation. That is why I did not added > code into kernel which show error message when value different than 1 > and 4 is specified in DT. > > But issue here is that there is no DT schema for pci-mvebu as above > .txt file was not converted to YAML schema yet. This is something which > should be improved... I'm OK with this patch as-is, especially since Rob acked it. Bjorn
diff --git a/Documentation/devicetree/bindings/pci/mvebu-pci.txt b/Documentation/devicetree/bindings/pci/mvebu-pci.txt index 6173af6885f8..24225852bce0 100644 --- a/Documentation/devicetree/bindings/pci/mvebu-pci.txt +++ b/Documentation/devicetree/bindings/pci/mvebu-pci.txt @@ -77,6 +77,7 @@ and the following optional properties: - marvell,pcie-lane: the physical PCIe lane number, for ports having multiple lanes. If this property is not found, we assume that the value is 0. +- num-lanes: number of SerDes PCIe lanes for this link (1 or 4) - reset-gpios: optional GPIO to PERST# - reset-delay-us: delay in us to wait after reset de-assertion, if not specified will default to 100ms, as required by the PCIe specification. @@ -141,6 +142,7 @@ pcie-controller { interrupt-map = <0 0 0 0 &mpic 58>; marvell,pcie-port = <0>; marvell,pcie-lane = <0>; + num-lanes = <1>; /* low-active PERST# reset on GPIO 25 */ reset-gpios = <&gpio0 25 1>; /* wait 20ms for device settle after reset deassertion */ @@ -161,6 +163,7 @@ pcie-controller { interrupt-map = <0 0 0 0 &mpic 59>; marvell,pcie-port = <0>; marvell,pcie-lane = <1>; + num-lanes = <1>; clocks = <&gateclk 6>; }; @@ -177,6 +180,7 @@ pcie-controller { interrupt-map = <0 0 0 0 &mpic 60>; marvell,pcie-port = <0>; marvell,pcie-lane = <2>; + num-lanes = <1>; clocks = <&gateclk 7>; }; @@ -193,6 +197,7 @@ pcie-controller { interrupt-map = <0 0 0 0 &mpic 61>; marvell,pcie-port = <0>; marvell,pcie-lane = <3>; + num-lanes = <1>; clocks = <&gateclk 8>; }; @@ -209,6 +214,7 @@ pcie-controller { interrupt-map = <0 0 0 0 &mpic 62>; marvell,pcie-port = <1>; marvell,pcie-lane = <0>; + num-lanes = <1>; clocks = <&gateclk 9>; }; @@ -225,6 +231,7 @@ pcie-controller { interrupt-map = <0 0 0 0 &mpic 63>; marvell,pcie-port = <1>; marvell,pcie-lane = <1>; + num-lanes = <1>; clocks = <&gateclk 10>; }; @@ -241,6 +248,7 @@ pcie-controller { interrupt-map = <0 0 0 0 &mpic 64>; marvell,pcie-port = <1>; marvell,pcie-lane = <2>; + num-lanes = <1>; clocks = <&gateclk 11>; }; @@ -257,6 +265,7 @@ pcie-controller { interrupt-map = <0 0 0 0 &mpic 65>; marvell,pcie-port = <1>; marvell,pcie-lane = <3>; + num-lanes = <1>; clocks = <&gateclk 12>; }; @@ -273,6 +282,7 @@ pcie-controller { interrupt-map = <0 0 0 0 &mpic 99>; marvell,pcie-port = <2>; marvell,pcie-lane = <0>; + num-lanes = <1>; clocks = <&gateclk 26>; }; @@ -289,6 +299,7 @@ pcie-controller { interrupt-map = <0 0 0 0 &mpic 103>; marvell,pcie-port = <3>; marvell,pcie-lane = <0>; + num-lanes = <1>; clocks = <&gateclk 27>; }; };