| Message ID | 20220512002003.16886-9-tharvey@gateworks.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | Ramon Fried |
| Headers | show |
| Series | Add MV88E61xx DSA driver and use on gwventana | expand |
On Wed, May 11, 2022 at 05:20:03PM -0700, Tim Harvey wrote: > Add MV88E61XX DSA support: > - update dt: U-Boot dsa driver requires different device-tree syntax > than the linux driver in order to link the dsa ports to the mdio bus. > - update defconfig > - replace mv88e61xx_hw_reset weak override with board_phy_config support > for mv88e61xx configuration that is outside the scope of the DSA driver > > Signed-off-by: Tim Harvey <tharvey@gateworks.com> > --- > v2: no changes > --- > arch/arm/dts/imx6qdl-gw5904.dtsi | 35 +++++++++++++++++ > board/gateworks/gw_ventana/gw_ventana.c | 50 +++++++++---------------- > configs/gwventana_gw5904_defconfig | 7 ++-- > 3 files changed, 56 insertions(+), 36 deletions(-) > > diff --git a/arch/arm/dts/imx6qdl-gw5904.dtsi b/arch/arm/dts/imx6qdl-gw5904.dtsi > index 286c7a9924c2..63590a2debc7 100644 > --- a/arch/arm/dts/imx6qdl-gw5904.dtsi > +++ b/arch/arm/dts/imx6qdl-gw5904.dtsi > @@ -219,6 +219,27 @@ > compatible = "marvell,mv88e6085"; > reg = <0>; > > + mdios { I'm thinking either "mdios" is a container node for other "mdio" nodes, or you have the "mdio" node directly. Are you sure you want this structure? We might have problems if we pass this DT as-is up to Linux. Whereas the support for an "mdios" subnode could be upstreamed, I think. The current DT bindings document says: - mdio : Container of PHY and devices on the switches MDIO bus. - mdio? : Container of PHYs and devices on the external MDIO bus. The node must contains a compatible string of "marvell,mv88e6xxx-mdio-external" You have an "mdios" which theoretically matches "mdio?" but only unintendedly so, since it is the internal MDIO bus and not the external one. Easiest way out right now is to not introduce something which isn't parsed by the current Linux driver. So the internal MDIO bus would be named "mdio", and the external one would be named whatever, and would be identified by compatible string. > + #address-cells = <1>; > + #size-cells = <0>; > + > + sw_phy0: ethernet-phy@0 { > + reg = <0x0>; > + }; > + > + sw_phy1: ethernet-phy@1 { > + reg = <0x1>; > + }; > + > + sw_phy2: ethernet-phy@2 { > + reg = <0x2>; > + }; > + > + sw_phy3: ethernet-phy@3 { > + reg = <0x3>; > + }; > + }; > + > ports { > #address-cells = <1>; > #size-cells = <0>;
On Wed, May 18, 2022 at 7:41 AM Vladimir Oltean <vladimir.oltean@nxp.com> wrote: > > On Wed, May 11, 2022 at 05:20:03PM -0700, Tim Harvey wrote: > > Add MV88E61XX DSA support: > > - update dt: U-Boot dsa driver requires different device-tree syntax > > than the linux driver in order to link the dsa ports to the mdio bus. > > - update defconfig > > - replace mv88e61xx_hw_reset weak override with board_phy_config support > > for mv88e61xx configuration that is outside the scope of the DSA driver > > > > Signed-off-by: Tim Harvey <tharvey@gateworks.com> > > --- > > v2: no changes > > --- > > arch/arm/dts/imx6qdl-gw5904.dtsi | 35 +++++++++++++++++ > > board/gateworks/gw_ventana/gw_ventana.c | 50 +++++++++---------------- > > configs/gwventana_gw5904_defconfig | 7 ++-- > > 3 files changed, 56 insertions(+), 36 deletions(-) > > > > diff --git a/arch/arm/dts/imx6qdl-gw5904.dtsi b/arch/arm/dts/imx6qdl-gw5904.dtsi > > index 286c7a9924c2..63590a2debc7 100644 > > --- a/arch/arm/dts/imx6qdl-gw5904.dtsi > > +++ b/arch/arm/dts/imx6qdl-gw5904.dtsi > > @@ -219,6 +219,27 @@ > > compatible = "marvell,mv88e6085"; > > reg = <0>; > > > > + mdios { > > I'm thinking either "mdios" is a container node for other "mdio" nodes, > or you have the "mdio" node directly. Are you sure you want this > structure? We might have problems if we pass this DT as-is up to Linux. > Whereas the support for an "mdios" subnode could be upstreamed, I think. > The current DT bindings document says: > > - mdio : Container of PHY and devices on the switches MDIO > bus. > - mdio? : Container of PHYs and devices on the external MDIO > bus. The node must contains a compatible string of > "marvell,mv88e6xxx-mdio-external" > > You have an "mdios" which theoretically matches "mdio?" but only > unintendedly so, since it is the internal MDIO bus and not the external > one. > > Easiest way out right now is to not introduce something which isn't > parsed by the current Linux driver. So the internal MDIO bus would be > named "mdio", and the external one would be named whatever, and would be > identified by compatible string. > > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + sw_phy0: ethernet-phy@0 { > > + reg = <0x0>; > > + }; > > + > > + sw_phy1: ethernet-phy@1 { > > + reg = <0x1>; > > + }; > > + > > + sw_phy2: ethernet-phy@2 { > > + reg = <0x2>; > > + }; > > + > > + sw_phy3: ethernet-phy@3 { > > + reg = <0x3>; > > + }; > > + }; > > + > > ports { > > #address-cells = <1>; > > #size-cells = <0>; Vladimir, I'm not sure I follow your suggestion correctly. I agree this is not ideal and not at all what I wanted. The dt prior to this patch is what exists and works in Linux but U-Boot dsa (more correctly DM_MDIO dm_eth_phy_connect()) requires the phy-mode and phy-handle in the ports and I'm not clear how to define the mdio's for those to point to here. Recall I did the same thing for gw7901 in commit 1cb87b929e1e ("arm: dts: imx8mm-venice-gw7901.dts: fix dsa switch configuration") which I didn't feel was correct either as the previous dt there is what is in Linux and works. Can you give me an example? Best Regards, Tim
Hi Tim, On Wed, May 18, 2022 at 09:15:26AM -0700, Tim Harvey wrote: > On Wed, May 18, 2022 at 7:41 AM Vladimir Oltean <vladimir.oltean@nxp.com> wrote: > > > > On Wed, May 11, 2022 at 05:20:03PM -0700, Tim Harvey wrote: > > > Add MV88E61XX DSA support: > > > - update dt: U-Boot dsa driver requires different device-tree syntax > > > than the linux driver in order to link the dsa ports to the mdio bus. > > > - update defconfig > > > - replace mv88e61xx_hw_reset weak override with board_phy_config support > > > for mv88e61xx configuration that is outside the scope of the DSA driver > > > > > > Signed-off-by: Tim Harvey <tharvey@gateworks.com> > > > --- > > > v2: no changes > > > --- > > > arch/arm/dts/imx6qdl-gw5904.dtsi | 35 +++++++++++++++++ > > > board/gateworks/gw_ventana/gw_ventana.c | 50 +++++++++---------------- > > > configs/gwventana_gw5904_defconfig | 7 ++-- > > > 3 files changed, 56 insertions(+), 36 deletions(-) > > > > > > diff --git a/arch/arm/dts/imx6qdl-gw5904.dtsi b/arch/arm/dts/imx6qdl-gw5904.dtsi > > > index 286c7a9924c2..63590a2debc7 100644 > > > --- a/arch/arm/dts/imx6qdl-gw5904.dtsi > > > +++ b/arch/arm/dts/imx6qdl-gw5904.dtsi > > > @@ -219,6 +219,27 @@ > > > compatible = "marvell,mv88e6085"; > > > reg = <0>; > > > > > > + mdios { > > > > I'm thinking either "mdios" is a container node for other "mdio" nodes, > > or you have the "mdio" node directly. Are you sure you want this > > structure? We might have problems if we pass this DT as-is up to Linux. > > Whereas the support for an "mdios" subnode could be upstreamed, I think. > > The current DT bindings document says: > > > > - mdio : Container of PHY and devices on the switches MDIO > > bus. > > - mdio? : Container of PHYs and devices on the external MDIO > > bus. The node must contains a compatible string of > > "marvell,mv88e6xxx-mdio-external" > > > > You have an "mdios" which theoretically matches "mdio?" but only > > unintendedly so, since it is the internal MDIO bus and not the external > > one. > > > > Easiest way out right now is to not introduce something which isn't > > parsed by the current Linux driver. So the internal MDIO bus would be > > named "mdio", and the external one would be named whatever, and would be > > identified by compatible string. > > > > > + #address-cells = <1>; > > > + #size-cells = <0>; > > > + > > > + sw_phy0: ethernet-phy@0 { > > > + reg = <0x0>; > > > + }; > > > + > > > + sw_phy1: ethernet-phy@1 { > > > + reg = <0x1>; > > > + }; > > > + > > > + sw_phy2: ethernet-phy@2 { > > > + reg = <0x2>; > > > + }; > > > + > > > + sw_phy3: ethernet-phy@3 { > > > + reg = <0x3>; > > > + }; > > > + }; > > > + > > > ports { > > > #address-cells = <1>; > > > #size-cells = <0>; > > Vladimir, > > I'm not sure I follow your suggestion correctly. I agree this is not > ideal and not at all what I wanted. The dt prior to this patch is what > exists and works in Linux but U-Boot dsa (more correctly DM_MDIO > dm_eth_phy_connect()) requires the phy-mode and phy-handle in the > ports and I'm not clear how to define the mdio's for those to point to > here. > > Recall I did the same thing for gw7901 in commit 1cb87b929e1e ("arm: > dts: imx8mm-venice-gw7901.dts: fix dsa switch configuration") which I > didn't feel was correct either as the previous dt there is what is in > Linux and works. > > Can you give me an example? My considerations are the following. The Linux Documentation/devicetree/bindings/net/mdio.yaml file says that the following are valid node names for an MDIO controller: properties: $nodename: pattern: "^mdio(@.*)?" In simple terms, "mdio" or "mdio@something". When you put an "mdio" controller on the same level as the "ports" node, you cannot use the "mdio@something" syntax, because the #address-cells and #size-cells must be 0 (otherwise said, if you say "mdio@something", you are also forced to say "ports@something"). This is not a problem if there is a single MDIO controller, because you can just use "mdio" and call it a day. But when you have multiple MDIO controllers, naming the second one becomes problematic. The Linux mv88e6xxx driver expects secondary (external) MDIO controllers to be named whatever (mdio1 for example) and just looks at their compatible strings. This is fine, but the "mdio1" name is neither "mdio" nor "mdio@something". So mdio.yaml won't really validate these nodes. This is why some other drivers have an "mdios" container node on the same level as "ports", and make "mdios" have #address-cells of 1, so that children of "mdios" can be "mdio@something", and all children will be validated by mdio.yaml. The mv88e6xxx driver does not support parsing an "mdios" container node, but code for that could be added, if we ever wanted to improve the bindings for this switch. All that I'm saying is that your approach is the weird mix where you are neither benefitting from having a container node for MDIO controllers, nor complying to mdio.yaml, nor following what the Linux driver expects. You are free to make a choice whether to simply follow the non-compliant Linux way of having "mdio", "mdio1", etc, or to introduce a proper "mdios" container node here _and_ in Linux. But what you've proposed currently doesn't really make sense.
diff --git a/arch/arm/dts/imx6qdl-gw5904.dtsi b/arch/arm/dts/imx6qdl-gw5904.dtsi index 286c7a9924c2..63590a2debc7 100644 --- a/arch/arm/dts/imx6qdl-gw5904.dtsi +++ b/arch/arm/dts/imx6qdl-gw5904.dtsi @@ -219,6 +219,27 @@ compatible = "marvell,mv88e6085"; reg = <0>; + mdios { + #address-cells = <1>; + #size-cells = <0>; + + sw_phy0: ethernet-phy@0 { + reg = <0x0>; + }; + + sw_phy1: ethernet-phy@1 { + reg = <0x1>; + }; + + sw_phy2: ethernet-phy@2 { + reg = <0x2>; + }; + + sw_phy3: ethernet-phy@3 { + reg = <0x3>; + }; + }; + ports { #address-cells = <1>; #size-cells = <0>; @@ -226,27 +247,41 @@ port@0 { reg = <0>; label = "lan4"; + phy-mode = "internal"; + phy-handle = <&sw_phy0>; }; port@1 { reg = <1>; label = "lan3"; + phy-mode = "internal"; + phy-handle = <&sw_phy1>; }; port@2 { reg = <2>; label = "lan2"; + phy-mode = "internal"; + phy-handle = <&sw_phy2>; }; port@3 { reg = <3>; label = "lan1"; + phy-mode = "internal"; + phy-handle = <&sw_phy3>; }; port@5 { reg = <5>; label = "cpu"; ethernet = <&fec>; + phy-mode = "rgmii-id"; + + fixed-link { + speed = <1000>; + full-duplex; + }; }; }; }; diff --git a/board/gateworks/gw_ventana/gw_ventana.c b/board/gateworks/gw_ventana/gw_ventana.c index c06630a66b66..bef3f7ef0d2b 100644 --- a/board/gateworks/gw_ventana/gw_ventana.c +++ b/board/gateworks/gw_ventana/gw_ventana.c @@ -68,44 +68,30 @@ int board_phy_config(struct phy_device *phydev) phy_write(phydev, MDIO_DEVAD_NONE, 14, val); } + /* Fixed PHY: for GW5904/GW5909 this is Marvell 88E6176 GbE Switch */ + else if (phydev->phy_id == 0xa5a55a5a && + ((board_type == GW5904) || (board_type == GW5909))) { + struct mii_dev *bus = miiphy_get_dev_by_name("mdio"); + + puts("MV88E61XX "); + /* GPIO[0] output CLK125 for RGMII_REFCLK */ + bus->write(bus, 0x1c, 0, 0x1a, (1 << 15) | (0x62 << 8) | 0xfe); + bus->write(bus, 0x1c, 0, 0x1a, (1 << 15) | (0x68 << 8) | 7); + + /* Port 0-3 LED configuration: Table 80/82 */ + /* LED configuration: 7:4-green (8=Activity) 3:0 amber (8=Link) */ + bus->write(bus, 0x10, 0, 0x16, 0x8088); + bus->write(bus, 0x11, 0, 0x16, 0x8088); + bus->write(bus, 0x12, 0, 0x16, 0x8088); + bus->write(bus, 0x13, 0, 0x16, 0x8088); + } + if (phydev->drv->config) phydev->drv->config(phydev); return 0; } -#ifdef CONFIG_MV88E61XX_SWITCH -int mv88e61xx_hw_reset(struct phy_device *phydev) -{ - struct mii_dev *bus = phydev->bus; - - /* GPIO[0] output, CLK125 */ - debug("enabling RGMII_REFCLK\n"); - bus->write(bus, 0x1c /*MV_GLOBAL2*/, 0, - 0x1a /*MV_SCRATCH_MISC*/, - (1 << 15) | (0x62 /*MV_GPIO_DIR*/ << 8) | 0xfe); - bus->write(bus, 0x1c /*MV_GLOBAL2*/, 0, - 0x1a /*MV_SCRATCH_MISC*/, - (1 << 15) | (0x68 /*MV_GPIO01_CNTL*/ << 8) | 7); - - /* RGMII delay - Physical Control register bit[15:14] */ - debug("setting port%d RGMII rx/tx delay\n", CONFIG_MV88E61XX_CPU_PORT); - /* forced 1000mbps full-duplex link */ - bus->write(bus, 0x10 + CONFIG_MV88E61XX_CPU_PORT, 0, 1, 0xc0fe); - phydev->autoneg = AUTONEG_DISABLE; - phydev->speed = SPEED_1000; - phydev->duplex = DUPLEX_FULL; - - /* LED configuration: 7:4-green (8=Activity) 3:0 amber (8=Link) */ - bus->write(bus, 0x10, 0, 0x16, 0x8088); - bus->write(bus, 0x11, 0, 0x16, 0x8088); - bus->write(bus, 0x12, 0, 0x16, 0x8088); - bus->write(bus, 0x13, 0, 0x16, 0x8088); - - return 0; -} -#endif // CONFIG_MV88E61XX_SWITCH - #if defined(CONFIG_VIDEO_IPUV3) static void enable_hdmi(struct display_info_t const *dev) { diff --git a/configs/gwventana_gw5904_defconfig b/configs/gwventana_gw5904_defconfig index d25a8324b1df..9809bc691508 100644 --- a/configs/gwventana_gw5904_defconfig +++ b/configs/gwventana_gw5904_defconfig @@ -103,12 +103,11 @@ CONFIG_SUPPORT_EMMC_BOOT=y CONFIG_FSL_USDHC=y CONFIG_MTD=y CONFIG_PHYLIB=y -CONFIG_MV88E61XX_SWITCH=y -CONFIG_MV88E61XX_CPU_PORT=5 -CONFIG_MV88E61XX_PHY_PORTS=0xf -CONFIG_MV88E61XX_FIXED_PORTS=0x0 +CONFIG_MV88E61XX=y +CONFIG_PHY_FIXED=y CONFIG_DM_ETH=y CONFIG_DM_MDIO=y +CONFIG_DM_DSA=y CONFIG_E1000=y CONFIG_FEC_MXC=y CONFIG_MII=y
Add MV88E61XX DSA support: - update dt: U-Boot dsa driver requires different device-tree syntax than the linux driver in order to link the dsa ports to the mdio bus. - update defconfig - replace mv88e61xx_hw_reset weak override with board_phy_config support for mv88e61xx configuration that is outside the scope of the DSA driver Signed-off-by: Tim Harvey <tharvey@gateworks.com> --- v2: no changes --- arch/arm/dts/imx6qdl-gw5904.dtsi | 35 +++++++++++++++++ board/gateworks/gw_ventana/gw_ventana.c | 50 +++++++++---------------- configs/gwventana_gw5904_defconfig | 7 ++-- 3 files changed, 56 insertions(+), 36 deletions(-)