Re: [PATCH v3 8/8] board: gw_ventana: enable MV88E61XX DSA support

[Vladimir Oltean <vladimir.oltean@nxp.com>]

On Mon, May 23, 2022 at 11:25:49AM -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>
> ---
> v3:
>  - move mdio's mdio@0 subnode
> v2: no changes
> ---
>  arch/arm/dts/imx6qdl-gw5904.dtsi        | 41 ++++++++++++++++++++
>  board/gateworks/gw_ventana/gw_ventana.c | 50 +++++++++----------------
>  configs/gwventana_gw5904_defconfig      |  7 ++--
>  3 files changed, 62 insertions(+), 36 deletions(-)
> 
> diff --git a/arch/arm/dts/imx6qdl-gw5904.dtsi b/arch/arm/dts/imx6qdl-gw5904.dtsi
> index 286c7a9924c2..1b2f70d1ccb2 100644
> --- a/arch/arm/dts/imx6qdl-gw5904.dtsi
> +++ b/arch/arm/dts/imx6qdl-gw5904.dtsi
> @@ -219,6 +219,33 @@
>  			compatible = "marvell,mv88e6085";
>  			reg = <0>;
>  
> +			mdios {
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +
> +				mdio@0 {
> +					reg = <0>;
> +					#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 +253,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 &&

PHY_FIXED_ID, but see below.

> +		 ((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);
> +	}
> +

There's nothing too board specific about this configuration, why do you
feel it does not belong to the DSA driver?

Some macros hiding away magic register addresses and values would be
good either way.

>  	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
> -- 
> 2.17.1
>