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 <olte...@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 <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?
> >
>
> 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.olt...@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.olt...@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
