| Message ID | 20220430130347.15190-5-linux@fw-web.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Support mt7531 on BPI-R2 Pro | expand |
Am Samstag, 30. April 2022, 15:03:47 CEST schrieb Frank Wunderlich: > From: Frank Wunderlich <frank-w@public-files.de> > > Add Device Tree node for mt7531 switch connected to gmac0. > > Signed-off-by: Frank Wunderlich <frank-w@public-files.de> > --- > v2: > - drop status=disabled > --- > .../boot/dts/rockchip/rk3568-bpi-r2-pro.dts | 48 +++++++++++++++++++ > 1 file changed, 48 insertions(+) > > diff --git a/arch/arm64/boot/dts/rockchip/rk3568-bpi-r2-pro.dts b/arch/arm64/boot/dts/rockchip/rk3568-bpi-r2-pro.dts > index ed2f2bd9a016..f0ffbe818170 100644 > --- a/arch/arm64/boot/dts/rockchip/rk3568-bpi-r2-pro.dts > +++ b/arch/arm64/boot/dts/rockchip/rk3568-bpi-r2-pro.dts > @@ -437,6 +437,54 @@ &i2c5 { > status = "disabled"; > }; > > +&mdio0 { > + #address-cells = <1>; > + #size-cells = <0>; > + > + switch@0 { > + compatible = "mediatek,mt7531"; > + reg = <0>; > + > + ports { > + #address-cells = <1>; > + #size-cells = <0>; > + > + port@1 { > + reg = <1>; > + label = "lan0"; > + }; > + > + port@2 { > + reg = <2>; > + label = "lan1"; > + }; > + > + port@3 { > + reg = <3>; > + label = "lan2"; > + }; > + > + port@4 { > + reg = <4>; > + label = "lan3"; > + }; > + > + port@5 { > + reg = <5>; > + label = "cpu"; > + ethernet = <&gmac0>; > + phy-mode = "rgmii"; - phy-mode: String, the following values are acceptable for port labeled "cpu": If compatible mediatek,mt7530 or mediatek,mt7621 is set, must be either "trgmii" or "rgmii" If compatible mediatek,mt7531 is set, must be either "sgmii", "1000base-x" or "2500base-x" So I guess the phy-mode needs to change? Heiko > + > + fixed-link { > + speed = <1000>; > + full-duplex; > + pause; > + }; > + }; > + }; > + }; > +}; > + > &mdio1 { > rgmii_phy1: ethernet-phy@0 { > compatible = "ethernet-phy-ieee802.3-c22"; >
Am 30. April 2022 16:05:06 MESZ schrieb Heiko Stuebner <heiko@sntech.de>: >Am Samstag, 30. April 2022, 15:03:47 CEST schrieb Frank Wunderlich: >> --- a/arch/arm64/boot/dts/rockchip/rk3568-bpi-r2-pro.dts >> +++ b/arch/arm64/boot/dts/rockchip/rk3568-bpi-r2-pro.dts >> @@ -437,6 +437,54 @@ &i2c5 { >> status = "disabled"; >> }; >> >> + port@5 { >> + reg = <5>; >> + label = "cpu"; >> + ethernet = <&gmac0>; >> + phy-mode = "rgmii"; >- phy-mode: String, the following values are acceptable for port >labeled > "cpu": > If compatible mediatek,mt7530 or mediatek,mt7621 is set, > must be either "trgmii" or "rgmii" > If compatible mediatek,mt7531 is set, > must be either "sgmii", "1000base-x" or "2500base-x" > >So I guess the phy-mode needs to change? This results from current (before my cpu-port patches) implementation in driver where cpu-port is fixed to port 6. On Mt7530 port 6 supports rgmii and trgmii. On mt7531 port6 only supports sgmii (which is basicly 2500base-x). Afaik it does not support 1G. Port 5 on mt7531 supports rgmii or sgmii (dual sgmii mode) but seems not reflected yet in txt. On thing more to change when converting txt to yaml. regards Frank
On Sat, Apr 30, 2022 at 03:03:47PM +0200, Frank Wunderlich wrote: > From: Frank Wunderlich <frank-w@public-files.de> > > Add Device Tree node for mt7531 switch connected to gmac0. > > Signed-off-by: Frank Wunderlich <frank-w@public-files.de> > --- > v2: > - drop status=disabled > --- > .../boot/dts/rockchip/rk3568-bpi-r2-pro.dts | 48 +++++++++++++++++++ > 1 file changed, 48 insertions(+) > > diff --git a/arch/arm64/boot/dts/rockchip/rk3568-bpi-r2-pro.dts b/arch/arm64/boot/dts/rockchip/rk3568-bpi-r2-pro.dts > index ed2f2bd9a016..f0ffbe818170 100644 > --- a/arch/arm64/boot/dts/rockchip/rk3568-bpi-r2-pro.dts > +++ b/arch/arm64/boot/dts/rockchip/rk3568-bpi-r2-pro.dts > @@ -437,6 +437,54 @@ &i2c5 { > status = "disabled"; > }; > > +&mdio0 { > + #address-cells = <1>; > + #size-cells = <0>; > + > + switch@0 { I think the preferable names are the newer "ethernet-switch@0", "ethernet-ports", "ethernet-port@0". Otherwise Reviewed-by: Vladimir Oltean <olteanv@gmail.com> > + compatible = "mediatek,mt7531"; > + reg = <0>; > + > + ports { > + #address-cells = <1>; > + #size-cells = <0>; > + > + port@1 { > + reg = <1>; > + label = "lan0"; > + }; > + > + port@2 { > + reg = <2>; > + label = "lan1"; > + }; > + > + port@3 { > + reg = <3>; > + label = "lan2"; > + }; > + > + port@4 { > + reg = <4>; > + label = "lan3"; > + }; > + > + port@5 { > + reg = <5>; > + label = "cpu"; > + ethernet = <&gmac0>; > + phy-mode = "rgmii"; > + > + fixed-link { > + speed = <1000>; > + full-duplex; > + pause; > + }; > + }; > + }; > + }; > +}; > + > &mdio1 { > rgmii_phy1: ethernet-phy@0 { > compatible = "ethernet-phy-ieee802.3-c22"; > -- > 2.25.1 >
Hi, thanks for review > Gesendet: Mittwoch, 04. Mai 2022 um 17:24 Uhr > Von: "Vladimir Oltean" <olteanv@gmail.com> > > +&mdio0 { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + switch@0 { > > I think the preferable names are the newer "ethernet-switch@0", > "ethernet-ports", "ethernet-port@0". > > Otherwise > > Reviewed-by: Vladimir Oltean <olteanv@gmail.com> current device-tree nodes using "switch" and "ports" see discussioon here about make it fixed to "ports" property instead of PatternProperties including optional "ethernet-" https://patchwork.kernel.org/project/linux-mediatek/patch/20220502153238.85090-1-linux@fw-web.de/#24843155 > > > + compatible = "mediatek,mt7531"; > > + reg = <0>; > > + > > + ports { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + port@1 { > > + reg = <1>; > > + label = "lan0"; > > + }; > > + > > + port@2 { > > + reg = <2>; > > + label = "lan1"; > > + }; > > + > > + port@3 { > > + reg = <3>; > > + label = "lan2"; > > + }; > > + > > + port@4 { > > + reg = <4>; > > + label = "lan3"; > > + }; > > + > > + port@5 { > > + reg = <5>; > > + label = "cpu"; > > + ethernet = <&gmac0>; > > + phy-mode = "rgmii"; > > + > > + fixed-link { > > + speed = <1000>; > > + full-duplex; > > + pause; > > + }; > > + }; > > + }; > > + };
On Wed, May 04, 2022 at 05:33:20PM +0200, Frank Wunderlich wrote: > Hi, > > thanks for review > > > Gesendet: Mittwoch, 04. Mai 2022 um 17:24 Uhr > > Von: "Vladimir Oltean" <olteanv@gmail.com> > > > > +&mdio0 { > > > + #address-cells = <1>; > > > + #size-cells = <0>; > > > + > > > + switch@0 { > > > > I think the preferable names are the newer "ethernet-switch@0", > > "ethernet-ports", "ethernet-port@0". > > > > Otherwise > > > > Reviewed-by: Vladimir Oltean <olteanv@gmail.com> > > current device-tree nodes using "switch" and "ports" > > see discussioon here about make it fixed to "ports" property instead of PatternProperties including optional "ethernet-" > > https://patchwork.kernel.org/project/linux-mediatek/patch/20220502153238.85090-1-linux@fw-web.de/#24843155 Hmm, I don't get why Krzysztof said to just keep what is used in existing device trees. The schema validator should describe what is valid, and since the mt7530 driver does not care one way or another (some drivers do explicitly parse the "ports"/"ethernet-ports" node), then whatever is valid for the DSA core is also valid for the mt7530 bindings. And "ethernet-ports" is valid too, so I think it should be accepted by mediatek.yaml...
On 04/05/2022 17:47, Vladimir Oltean wrote: >> >> current device-tree nodes using "switch" and "ports" >> >> see discussioon here about make it fixed to "ports" property instead of PatternProperties including optional "ethernet-" >> >> https://patchwork.kernel.org/project/linux-mediatek/patch/20220502153238.85090-1-linux@fw-web.de/#24843155 > > Hmm, I don't get why Krzysztof said to just keep what is used in > existing device trees. The schema validator should describe what is > valid, These were talks about bindings which describe hardware. The node name, except Devicetree spec asking for generic names, does not matter here actually. > and since the mt7530 driver does not care one way or another > (some drivers do explicitly parse the "ports"/"ethernet-ports" node), > then whatever is valid for the DSA core is also valid for the mt7530 > bindings. And "ethernet-ports" is valid too, so I think it should be > accepted by mediatek.yaml... You can make it "(ethernet-)?ports" as well. My comment was purely to make it simpler, for bindings (goes into properties, not patternProperties) and for us. If you prefer to keep it like DSA core, also fine. Best regards, Krzysztof
Am 5. Mai 2022 10:46:55 MESZ schrieb Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>: >On 04/05/2022 17:47, Vladimir Oltean wrote: >>> >>> current device-tree nodes using "switch" and "ports" >>> >>> see discussioon here about make it fixed to "ports" property instead >of PatternProperties including optional "ethernet-" >>> >>> >https://patchwork.kernel.org/project/linux-mediatek/patch/20220502153238.85090-1-linux@fw-web.de/#24843155 >> >> Hmm, I don't get why Krzysztof said to just keep what is used in >> existing device trees. The schema validator should describe what is >> valid, > >These were talks about bindings which describe hardware. The node name, >except Devicetree spec asking for generic names, does not matter here >actually. > >> and since the mt7530 driver does not care one way or another >> (some drivers do explicitly parse the "ports"/"ethernet-ports" node), >> then whatever is valid for the DSA core is also valid for the mt7530 >> bindings. And "ethernet-ports" is valid too, so I think it should be >> accepted by mediatek.yaml... > >You can make it "(ethernet-)?ports" as well. My comment was purely to >make it simpler, for bindings (goes into properties, not >patternProperties) and for us. If you prefer to keep it like DSA core, >also fine. Ok, i'm also thinking, the dsa-definition will be the right way (pattern-properties with optional "ethernet-") in binding. Should i use "ethernet-ports" instead of "ports" here? Current dts with mt7530/mt7531 switches using "ports" so i would use it here too. If dsa prefer ethernet-ports now it should be changed in other files too. >Best regards, >Krzysztof Hi, regards Frank
On 05/05/2022 11:06, Frank Wunderlich wrote: >> You can make it "(ethernet-)?ports" as well. My comment was purely to >> make it simpler, for bindings (goes into properties, not >> patternProperties) and for us. If you prefer to keep it like DSA core, >> also fine. > > Ok, i'm also thinking, the dsa-definition will be the right way (pattern-properties with optional "ethernet-") in binding. > > Should i use "ethernet-ports" instead of "ports" here? Current dts with mt7530/mt7531 switches using "ports" so i would use it here too. If dsa prefer ethernet-ports now it should be changed in other files too. I think bindings allow both, so choose something consistent with existing DTS sources. Best regards, Krzysztof
diff --git a/arch/arm64/boot/dts/rockchip/rk3568-bpi-r2-pro.dts b/arch/arm64/boot/dts/rockchip/rk3568-bpi-r2-pro.dts index ed2f2bd9a016..f0ffbe818170 100644 --- a/arch/arm64/boot/dts/rockchip/rk3568-bpi-r2-pro.dts +++ b/arch/arm64/boot/dts/rockchip/rk3568-bpi-r2-pro.dts @@ -437,6 +437,54 @@ &i2c5 { status = "disabled"; }; +&mdio0 { + #address-cells = <1>; + #size-cells = <0>; + + switch@0 { + compatible = "mediatek,mt7531"; + reg = <0>; + + ports { + #address-cells = <1>; + #size-cells = <0>; + + port@1 { + reg = <1>; + label = "lan0"; + }; + + port@2 { + reg = <2>; + label = "lan1"; + }; + + port@3 { + reg = <3>; + label = "lan2"; + }; + + port@4 { + reg = <4>; + label = "lan3"; + }; + + port@5 { + reg = <5>; + label = "cpu"; + ethernet = <&gmac0>; + phy-mode = "rgmii"; + + fixed-link { + speed = <1000>; + full-duplex; + pause; + }; + }; + }; + }; +}; + &mdio1 { rgmii_phy1: ethernet-phy@0 { compatible = "ethernet-phy-ieee802.3-c22";