| Message ID | 20210819170416.2252210-1-vladimir.oltean@nxp.com |
|---|---|
| State | Accepted |
| Commit | 0181f6f19c6c35b24f1516d8db22f3bbce762633 |
| Headers | show |
| Series | [devicetree,1/2] MIPS: mscc: ocelot: disable all switch ports by default | expand |
On Thu, Aug 19, 2021 at 08:04:16PM +0300, Vladimir Oltean wrote: > The ocelot driver was converted to phylink, and that expects a valid > phy_interface_t. Without a phy-mode, of_get_phy_mode returns > PHY_INTERFACE_MODE_NA, which is not ideal because phylink rejects that. > > The ocelot driver was patched to treat PHY_INTERFACE_MODE_NA as > PHY_INTERFACE_MODE_INTERNAL to work with the broken DT blobs, but we > should fix the device trees and specify the phy-mode too. > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> > --- Please note that the pre-phylink driver has this check: switch (ocelot_port->phy_mode) { case PHY_INTERFACE_MODE_NA: (...) case PHY_INTERFACE_MODE_SGMII: (...) case PHY_INTERFACE_MODE_QSGMII: (...) default: dev_err(ocelot->dev, "invalid phy mode for port%d, (Q)SGMII only\n", port); of_node_put(portnp); err = -EINVAL; goto out_teardown; } So it does not actually expect PHY_INTERFACE_MODE_INTERNAL and will error out. Are we okay with the new device tree blobs breaking the old kernel? Should we instead wait for a few more kernel release cycles before changing the device tree in this regard?
On Thu, Aug 19, 2021 at 08:04:15PM +0300, Vladimir Oltean wrote: > The ocelot switch driver used to ignore ports which do not have a > phy-handle property and not probe those, but this is not quite ok since > it is valid to not have a phy-handle property if there is a fixed-link. > > It seems that checking for a phy-handle was a proxy for the proper check > which is for the status, but that doesn't make a lot of sense, since the > ocelot driver already iterates using for_each_available_child_of_node > which skips the disabled ports, so I have no idea. > > Anyway, a widespread pattern in device trees is for a SoC dtsi to > disable by default all hardware, and let board dts files enable what is > used. So let's do that and enable only the ports with a phy-handle in > the pcb120 and pcb123 device tree files. > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> > --- > arch/mips/boot/dts/mscc/ocelot.dtsi | 11 +++++++++++ > arch/mips/boot/dts/mscc/ocelot_pcb120.dts | 8 ++++++++ > arch/mips/boot/dts/mscc/ocelot_pcb123.dts | 4 ++++ > 3 files changed, 23 insertions(+) applied to mips-next. Thomas. -- Crap can work. Given enough thrust pigs will fly, but it's not necessarily a good idea. [ RFC1925, 2.3 ]
diff --git a/arch/mips/boot/dts/mscc/ocelot.dtsi b/arch/mips/boot/dts/mscc/ocelot.dtsi index 535a98284dcb..e51db651af13 100644 --- a/arch/mips/boot/dts/mscc/ocelot.dtsi +++ b/arch/mips/boot/dts/mscc/ocelot.dtsi @@ -150,36 +150,47 @@ ethernet-ports { port0: port@0 { reg = <0>; + status = "disabled"; }; port1: port@1 { reg = <1>; + status = "disabled"; }; port2: port@2 { reg = <2>; + status = "disabled"; }; port3: port@3 { reg = <3>; + status = "disabled"; }; port4: port@4 { reg = <4>; + status = "disabled"; }; port5: port@5 { reg = <5>; + status = "disabled"; }; port6: port@6 { reg = <6>; + status = "disabled"; }; port7: port@7 { reg = <7>; + status = "disabled"; }; port8: port@8 { reg = <8>; + status = "disabled"; }; port9: port@9 { reg = <9>; + status = "disabled"; }; port10: port@10 { reg = <10>; + status = "disabled"; }; }; }; diff --git a/arch/mips/boot/dts/mscc/ocelot_pcb120.dts b/arch/mips/boot/dts/mscc/ocelot_pcb120.dts index 897de5025d7f..d2dc6b3d923c 100644 --- a/arch/mips/boot/dts/mscc/ocelot_pcb120.dts +++ b/arch/mips/boot/dts/mscc/ocelot_pcb120.dts @@ -69,40 +69,48 @@ phy4: ethernet-phy@3 { }; &port0 { + status = "okay"; phy-handle = <&phy0>; }; &port1 { + status = "okay"; phy-handle = <&phy1>; }; &port2 { + status = "okay"; phy-handle = <&phy2>; }; &port3 { + status = "okay"; phy-handle = <&phy3>; }; &port4 { + status = "okay"; phy-handle = <&phy7>; phy-mode = "sgmii"; phys = <&serdes 4 SERDES1G(2)>; }; &port5 { + status = "okay"; phy-handle = <&phy4>; phy-mode = "sgmii"; phys = <&serdes 5 SERDES1G(5)>; }; &port6 { + status = "okay"; phy-handle = <&phy6>; phy-mode = "sgmii"; phys = <&serdes 6 SERDES1G(3)>; }; &port9 { + status = "okay"; phy-handle = <&phy5>; phy-mode = "sgmii"; phys = <&serdes 9 SERDES1G(4)>; diff --git a/arch/mips/boot/dts/mscc/ocelot_pcb123.dts b/arch/mips/boot/dts/mscc/ocelot_pcb123.dts index ef852f382da8..7d7e638791dd 100644 --- a/arch/mips/boot/dts/mscc/ocelot_pcb123.dts +++ b/arch/mips/boot/dts/mscc/ocelot_pcb123.dts @@ -47,17 +47,21 @@ &mdio0 { }; &port0 { + status = "okay"; phy-handle = <&phy0>; }; &port1 { + status = "okay"; phy-handle = <&phy1>; }; &port2 { + status = "okay"; phy-handle = <&phy2>; }; &port3 { + status = "okay"; phy-handle = <&phy3>; };
The ocelot switch driver used to ignore ports which do not have a phy-handle property and not probe those, but this is not quite ok since it is valid to not have a phy-handle property if there is a fixed-link. It seems that checking for a phy-handle was a proxy for the proper check which is for the status, but that doesn't make a lot of sense, since the ocelot driver already iterates using for_each_available_child_of_node which skips the disabled ports, so I have no idea. Anyway, a widespread pattern in device trees is for a SoC dtsi to disable by default all hardware, and let board dts files enable what is used. So let's do that and enable only the ports with a phy-handle in the pcb120 and pcb123 device tree files. Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> --- arch/mips/boot/dts/mscc/ocelot.dtsi | 11 +++++++++++ arch/mips/boot/dts/mscc/ocelot_pcb120.dts | 8 ++++++++ arch/mips/boot/dts/mscc/ocelot_pcb123.dts | 4 ++++ 3 files changed, 23 insertions(+)