* dsa driver for mv88e61xx
@ 2022-03-10 16:16 Tim Harvey
2022-03-10 16:50 ` Vladimir Oltean
0 siblings, 1 reply; 6+ messages in thread
From: Tim Harvey @ 2022-03-10 16:16 UTC (permalink / raw)
To: Vladimir Oltean, u-boot
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: dsa driver for mv88e61xx
2022-03-10 16:16 dsa driver for mv88e61xx Tim Harvey
@ 2022-03-10 16:50 ` Vladimir Oltean
2022-03-10 22:35 ` Tim Harvey
0 siblings, 1 reply; 6+ messages in thread
From: Vladimir Oltean @ 2022-03-10 16:50 UTC (permalink / raw)
To: Tim Harvey; +Cc: u-boot
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?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: dsa driver for mv88e61xx
2022-03-10 16:50 ` Vladimir Oltean
@ 2022-03-10 22:35 ` Tim Harvey
2022-03-10 23:18 ` Vladimir Oltean
0 siblings, 1 reply; 6+ messages in thread
From: Tim Harvey @ 2022-03-10 22:35 UTC (permalink / raw)
To: Vladimir Oltean; +Cc: u-boot
On Thu, Mar 10, 2022 at 8:50 AM Vladimir Oltean <olteanv@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?
&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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: dsa driver for mv88e61xx
2022-03-10 22:35 ` Tim Harvey
@ 2022-03-10 23:18 ` Vladimir Oltean
2022-03-11 16:41 ` Tim Harvey
0 siblings, 1 reply; 6+ messages in thread
From: Vladimir Oltean @ 2022-03-10 23:18 UTC (permalink / raw)
To: Tim Harvey; +Cc: u-boot
On Thu, Mar 10, 2022 at 02:35:21PM -0800, Tim Harvey wrote:
> On Thu, Mar 10, 2022 at 8:50 AM Vladimir Oltean <olteanv@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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: dsa driver for mv88e61xx
2022-03-10 23:18 ` Vladimir Oltean
@ 2022-03-11 16:41 ` Tim Harvey
2022-03-11 18:43 ` Vladimir Oltean
0 siblings, 1 reply; 6+ messages in thread
From: Tim Harvey @ 2022-03-11 16:41 UTC (permalink / raw)
To: Vladimir Oltean; +Cc: u-boot
On Thu, Mar 10, 2022 at 3:18 PM Vladimir Oltean <olteanv@gmail.com> wrote:
>
> On Thu, Mar 10, 2022 at 02:35:21PM -0800, Tim Harvey wrote:
> > On Thu, Mar 10, 2022 at 8:50 AM Vladimir Oltean <olteanv@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?
>
Yes, the FEC MAC's internal MDIO lacks its own compatible string so I
can in the fec driver bind the MDIO driver to the mdio node and probe
it via uclass_get_device_by_ofnode(UCLASS_MDIO, node, &mdiodev).
Now the UCLASS_MDIO driver is probed and it works with boards that
have MAC->PHY (non-fixed PHY's) but for a MAC<->MAC fixed-link
phy_connect calls phy_connect_fixed with a null bus and does not scan
the mdio bus and the dsa driver still does not probe. I'm still back
to not understanding how to get the dsa switch driver to probe when
it's a child of an mdio node.
Maybe the answer is similar to your above answer in that I need to
make the fec driver continue to probe all children in the mdio bus
node. I feel like this is quite a burden on the mac driver and that
functionality should be handled in uclass-mdio somehow? I'm not sure
if there is a push to convert MDIO drivers to UCLASS_MDIO. It seems
like there is only a handful compared to all the drivers that call
mdio_register so perhaps this issue just hasn't come up yet. Are we
sure that in order to use UCLASS_DSA you must have a UCLASS_MDIO mdio
bus?
Best Regards,
Tim
> >
> > &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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: dsa driver for mv88e61xx
2022-03-11 16:41 ` Tim Harvey
@ 2022-03-11 18:43 ` Vladimir Oltean
0 siblings, 0 replies; 6+ messages in thread
From: Vladimir Oltean @ 2022-03-11 18:43 UTC (permalink / raw)
To: Tim Harvey; +Cc: u-boot
Hello Tim,
On Fri, Mar 11, 2022 at 08:41:48AM -0800, Tim Harvey wrote:
> On Thu, Mar 10, 2022 at 3:18 PM Vladimir Oltean <olteanv@gmail.com> wrote:
> >
> > On Thu, Mar 10, 2022 at 02:35:21PM -0800, Tim Harvey wrote:
> > > On Thu, Mar 10, 2022 at 8:50 AM Vladimir Oltean <olteanv@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?
> >
>
> Yes, the FEC MAC's internal MDIO lacks its own compatible string so I
> can in the fec driver bind the MDIO driver to the mdio node and probe
> it via uclass_get_device_by_ofnode(UCLASS_MDIO, node, &mdiodev).
>
> Now the UCLASS_MDIO driver is probed and it works with boards that
> have MAC->PHY (non-fixed PHY's) but for a MAC<->MAC fixed-link
> phy_connect calls phy_connect_fixed with a null bus and does not scan
> the mdio bus and the dsa driver still does not probe. I'm still back
> to not understanding how to get the dsa switch driver to probe when
> it's a child of an mdio node.
Well, it certainly isn't any form of phy_connect() call that is supposed
to probe the MDIO bus's children. And by the way, if you call
dm_eth_phy_connect(), that deals with both "phy-handle" and "fixed-link"
cases for you.
>
> Maybe the answer is similar to your above answer in that I need to
> make the fec driver continue to probe all children in the mdio bus
> node. I feel like this is quite a burden on the mac driver and that
> functionality should be handled in uclass-mdio somehow? I'm not sure
> if there is a push to convert MDIO drivers to UCLASS_MDIO. It seems
> like there is only a handful compared to all the drivers that call
> mdio_register so perhaps this issue just hasn't come up yet. Are we
> sure that in order to use UCLASS_DSA you must have a UCLASS_MDIO mdio
> bus?
Yup, pretty sure that the bus needs to follow the device model before
DM-capable devices under it will get probed.
Necessary, but not sufficient, it seems.
I spent 5 more minutes trying to see what is different between
UCLASS_SPI and UCLASS_MDIO, and it looks like this is what's different:
UCLASS_MDIO lacks a call to dm_scan_fdt_dev(). I've added this, and
booted the U-Boot sandbox while moving the DSA sandbox driver under the
MDIO sandbox driver. With the changes below, it probes as expected.
Hope this helps.
From 157cc32109348e8c0fb0763e7fe5c728cfd81eb3 Mon Sep 17 00:00:00 2001
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Date: Fri, 11 Mar 2022 20:38:29 +0200
Subject: [PATCH] make the dsa sandbox driver probe under DM_MDIO
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
arch/sandbox/dts/test.dts | 72 +++++++++++++++++++--------------------
net/mdio-uclass.c | 4 +++
2 files changed, 40 insertions(+), 36 deletions(-)
diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts
index 3d206fdb3cf8..b0329b672a43 100644
--- a/arch/sandbox/dts/test.dts
+++ b/arch/sandbox/dts/test.dts
@@ -533,42 +533,6 @@
fake-host-hwaddr = [00 00 66 44 22 66];
};
- dsa-test {
- compatible = "sandbox,dsa";
-
- ports {
- #address-cells = <1>;
- #size-cells = <0>;
- swp_0: port@0 {
- reg = <0>;
- label = "lan0";
- phy-mode = "rgmii-rxid";
-
- fixed-link {
- speed = <100>;
- full-duplex;
- };
- };
-
- swp_1: port@1 {
- reg = <1>;
- label = "lan1";
- phy-mode = "rgmii-txid";
- fixed-link = <0 1 100 0 0>;
- };
-
- port@2 {
- reg = <2>;
- ethernet = <&dsa_eth0>;
-
- fixed-link {
- speed = <1000>;
- full-duplex;
- };
- };
- };
- };
-
firmware {
sandbox_firmware: sandbox-firmware {
compatible = "sandbox,firmware";
@@ -1552,6 +1516,42 @@
mdio: mdio-test {
compatible = "sandbox,mdio";
+
+ dsa-test {
+ compatible = "sandbox,dsa";
+
+ ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ swp_0: port@0 {
+ reg = <0>;
+ label = "lan0";
+ phy-mode = "rgmii-rxid";
+
+ fixed-link {
+ speed = <100>;
+ full-duplex;
+ };
+ };
+
+ swp_1: port@1 {
+ reg = <1>;
+ label = "lan1";
+ phy-mode = "rgmii-txid";
+ fixed-link = <0 1 100 0 0>;
+ };
+
+ port@2 {
+ reg = <2>;
+ ethernet = <&dsa_eth0>;
+
+ fixed-link {
+ speed = <1000>;
+ full-duplex;
+ };
+ };
+ };
+ };
};
pm-bus-test {
diff --git a/net/mdio-uclass.c b/net/mdio-uclass.c
index e74e34f78f9c..190cb08b31d8 100644
--- a/net/mdio-uclass.c
+++ b/net/mdio-uclass.c
@@ -59,7 +59,11 @@ static int dm_mdio_post_bind(struct udevice *dev)
return -EINVAL;
}
+#if CONFIG_IS_ENABLED(OF_REAL)
+ return dm_scan_fdt_dev(dev);
+#else
return 0;
+#endif
}
/*
--
2.25.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-03-11 18:43 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-10 16:16 dsa driver for mv88e61xx Tim Harvey
2022-03-10 16:50 ` Vladimir Oltean
2022-03-10 22:35 ` Tim Harvey
2022-03-10 23:18 ` Vladimir Oltean
2022-03-11 16:41 ` Tim Harvey
2022-03-11 18:43 ` Vladimir Oltean
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.