On Thu, Mar 10, 2022 at 02:35:21PM -0800, Tim Harvey wrote: > On Thu, Mar 10, 2022 at 8:50 AM Vladimir Oltean <olte...@gmail.com> wrote: > > > > Hello Tim, > > > > On Thu, Mar 10, 2022 at 08:16:13AM -0800, Tim Harvey wrote: > > > Greetings, > > > > > > I wanted to take a stab at adding dsa support for the mv88e61xx which > > > currently has a driver in drivers/net/phy [1]. The board I have > > > available to me is the gw5904 which has a mv88e6085 with the upstream > > > port connected to the IMX6 FEC MAC over RGMII. This currently works > > > with the existing mv88e61xx driver with the following defined in > > > gwventana_gw5904_defconfig: > > > > > > CONFIG_MV88E61XX_SWITCH=y > > > CONFIG_MV88E61XX_CPU_PORT=5 > > > CONFIG_MV88E61XX_PHY_PORTS=0xf > > > CONFIG_MV88E61XX_FIXED_PORTS=0x0 > > > > > > The device-tree is arch/arm/dts/imx6qdl-gw5904.dtsi [2] which I > > > believe is proper and works in Linux with the Linux driver in > > > drivers/net/dsa/mv88e6xxx [3]. > > > > > > &fec { > > > pinctrl-names = "default"; > > > pinctrl-0 = <&pinctrl_enet>; > > > phy-mode = "rgmii-id"; > > > phy-reset-gpios = <&gpio1 30 GPIO_ACTIVE_LOW>; > > > phy-reset-duration = <10>; > > > phy-reset-post-delay = <100>; > > > status = "okay"; > > > > > > fixed-link { > > > speed = <1000>; > > > full-duplex; > > > }; > > > > > > mdio { > > > #address-cells = <1>; > > > #size-cells = <0>; > > > > > > switch@0 { > > > compatible = "marvell,mv88e6085"; > > > reg = <0>; > > > > > > ports { > > > #address-cells = <1>; > > > #size-cells = <0>; > > > > > > port@0 { > > > reg = <0>; > > > label = "lan4"; > > > }; > > > > > > port@1 { > > > reg = <1>; > > > label = "lan3"; > > > }; > > > > > > port@2 { > > > reg = <2>; > > > label = "lan2"; > > > }; > > > > > > port@3 { > > > reg = <3>; > > > label = "lan1"; > > > }; > > > > > > port@5 { > > > reg = <5>; > > > label = "cpu"; > > > ethernet = <&fec>; > > > }; > > > }; > > > }; > > > }; > > > }; > > > > > > My motivation for doing this is to be able to drop > > > gwventana_gw5904_defconfig as it is the same defconfig as > > > gwventana_emmc_defconfig with the switch added and with get_phy_id > > > overridden by the current mv88e61xx driver that config won't work with > > > boards that lack the switch. > > > > > > My first approach was to just put a #if !defined(CONFIG_DM_DSA) around > > > mv88e61xx get_phy_id and add a skeleton driver with an of_match of > > > compatible = "marvell,mv88e6085" but the driver does not probe with > > > the above dt fragment. > > > > > > Any ideas why the driver won't probe and advise on how I should > > > proceed with this? I'm not clear yet if I can just modify the existing > > > driver or if I should create a new one. > > > > > > Best Regards, > > > > > > Tim > > > [1] > > > https://elixir.bootlin.com/u-boot/latest/source/drivers/net/phy/mv88e61xx.c > > > [2] > > > https://elixir.bootlin.com/u-boot/latest/source/arch/arm/dts/imx6qdl-gw5904.dtsi > > > [3] > > > https://elixir.bootlin.com/linux/latest/source/drivers/net/dsa/mv88e6xxx/chip.c > > > > I spent about 3 minutes looking at mv88e61xx.c, so take what I'm about > > to say with a grain of salt. > > > > The biggest issue with reusing that code seems to be that it uses struct > > phy_device throughout. A DM_DSA driver would need to work around a > > struct udevice. I'd probably create a new driver, make it easy for > > others to use, then delete old driver. I see there are 5 occurrences of > > CONFIG_MV88E61XX_PHY_PORTS in defconfigs, all added within the past 4 > > years. One is yours. Could have been a lot worse. > > > > As to why your driver is not probing. I think the fec_mxc parent MDIO > > bus must be a udevice as well, registered using DM_MDIO? > > Vladimir, > > Ok, I see that even though fec_mxc is DM_ETH it doesn't use DM_MDIO. I > suppose my ksz9477 dsa driver probed because that switch was hanging > off I2C and DM_I2C was in use as opposed to this time the switch is > hanging off of MDIO. > > With changes to fec_mxc to use DM_MDIO/UCLASS_MDIO for the its mdio > bus, I see my UCLASS_MDIO bus getting probed and registering the bus > but the call to dm_eth_phy_connect(fec) fails. > > For the situation I have where the fec_mxc provides the MDIO bus > connected to the switch as well as the MAC that's connected via rgmii > to the upstream port I believe I need a 'phy-handle' in my fec node vs > a 'fixed-link' subnode otherwise dm_eth_connect_phy_handle() won't > probe my mdio device. But then I find that dm_eth_phy_connect(fec) > calls dm_eth_connect_phy_handle() which probes my UCLASS_MDIO but the > call to dm_mdio_phy_connect() fails and my UCLASS_DSA device never > gets probed. > > So I guess I'm confused if I should be using fixed-link or not and if > so, how does the UCLASS_MDIO device get probed?
You should definitely use a fixed-link and not a phy-handle to describe the MAC-to-MAC connection between the FEC and the switch's CPU port. If the MDIO bus udevice cannot get created/bound to a driver in lack of a compatible string, maybe you need to force a call to device_bind_driver_to_node() from the FEC driver for its "mdio" subnode? > > &fec { > pinctrl-names = "default" > #if 1 // required for dm_eth_connect_phy_handle to probe the UCLASS_MDIO > device > phy-handle = "switch"; > #endif > phy-mode = "rgmii-id"; > status = "okay"; > > #if 0 // if present phy_connect is called immediate in > dm_eth_connect_phy_handle and the UCLASS_MDIO device is not probed > fixed-link { > speed = <1000>; > full-duplex; > }; > #endif > > mdio { > #address-cells = <1>; > #size-cells = <0>; > > switch: switch@0 { > compatible = "marvell,mv88e6085"; > reg = <0>; > > ports { > #address-cells = <1>; > #size-cells = <0>; > > port@0 { > reg = <0>; > label = "lan4"; > phy-mode = "internal"; > }; > > ... > port@5 { > reg = <5>; > label = "cpu"; > ethernet = <&fec>; > phy-mode = "rgmii-id"; > > fixed-link { > speed = <1000>; > full-duplex; > }; > }; > }; > }; > }; > }; > > Best regards, > > Tim