[PATCH 0/2] DSA switch fixes

[PATCH 0/2] DSA switch fixes

[Vladimir Oltean]

This small patch set contains some bug fixes. The first is for the
recently introduced ->port_probe() DSA switch method, and the second is
for the recently introduced SJA1105 driver.

Vladimir Oltean (2):
  net: dsa: fix phydev->speed being uninitialized for the CPU port fixed
    PHY
  net: dsa: sja1105: fix device id detection

 drivers/net/sja1105.c |  6 ------
 net/dsa-uclass.c      | 11 +++++++++++
 2 files changed, 11 insertions(+), 6 deletions(-)

-- 
2.25.1

[PATCH 1/2] net: dsa: fix phydev->speed being uninitialized for the CPU port fixed PHY

[Vladimir Oltean]

If the DSA API is going to allow drivers to do things such as:

- phy_config in dsa_ops :: port_probe
- phy_startup in dsa_ops :: port_enable

then it would actually be good if the ->port_probe() method would
actually be called in all cases before the ->port_enable() is.

Currently this is true for user ports, but not true for the CPU port,
because the CPU port does not have a udevice registered for it (this is
all part of DSA's design). So the current issue is that after
phy_startup has finished for the CPU port, its phydev->speed is an
uninitialized value, because phy_config() was never called for the
priv->cpu_port_fixed_phy, and it is precisely phy_config() who copies
the speed into the phydev in the case of the fixed PHY driver.

So we need to simulate a probing event for the CPU port by manually
calling the driver's ->port_probe() method for the CPU port.

Fixes: 8a2982574854 ("net: dsa: introduce a .port_probe() method in struct dsa_ops")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 net/dsa-uclass.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/net/dsa-uclass.c b/net/dsa-uclass.c
index 606b1539a776..9ff55a02fb23 100644
--- a/net/dsa-uclass.c
+++ b/net/dsa-uclass.c
@@ -466,6 +466,8 @@ static int dsa_pre_probe(struct udevice *dev)
 {
 	struct dsa_pdata *pdata = dev_get_uclass_plat(dev);
 	struct dsa_priv *priv = dev_get_uclass_priv(dev);
+	struct dsa_ops *ops = dsa_get_ops(dev);
+	int err;
 
 	priv->num_ports = pdata->num_ports;
 	priv->cpu_port = pdata->cpu_port;
@@ -477,6 +479,15 @@ static int dsa_pre_probe(struct udevice *dev)
 
 	uclass_find_device_by_ofnode(UCLASS_ETH, pdata->master_node,
 				     &priv->master_dev);
+
+	/* Simulate a probing event for the CPU port */
+	if (ops->port_probe) {
+		err = ops->port_probe(dev, priv->cpu_port,
+				      priv->cpu_port_fixed_phy);
+		if (err)
+			return err;
+	}
+
 	return 0;
 }
 
-- 
2.25.1

Re: [PATCH 1/2] net: dsa: fix phydev->speed being uninitialized for the CPU port fixed PHY

[Ramon Fried]

On Sun, Dec 5, 2021 at 1:01 AM Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
>
> If the DSA API is going to allow drivers to do things such as:
>
> - phy_config in dsa_ops :: port_probe
> - phy_startup in dsa_ops :: port_enable
>
> then it would actually be good if the ->port_probe() method would
> actually be called in all cases before the ->port_enable() is.
>
> Currently this is true for user ports, but not true for the CPU port,
> because the CPU port does not have a udevice registered for it (this is
> all part of DSA's design). So the current issue is that after
> phy_startup has finished for the CPU port, its phydev->speed is an
> uninitialized value, because phy_config() was never called for the
> priv->cpu_port_fixed_phy, and it is precisely phy_config() who copies
> the speed into the phydev in the case of the fixed PHY driver.
>
> So we need to simulate a probing event for the CPU port by manually
> calling the driver's ->port_probe() method for the CPU port.
>
> Fixes: 8a2982574854 ("net: dsa: introduce a .port_probe() method in struct dsa_ops")
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
>  net/dsa-uclass.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/net/dsa-uclass.c b/net/dsa-uclass.c
> index 606b1539a776..9ff55a02fb23 100644
> --- a/net/dsa-uclass.c
> +++ b/net/dsa-uclass.c
> @@ -466,6 +466,8 @@ static int dsa_pre_probe(struct udevice *dev)
>  {
>         struct dsa_pdata *pdata = dev_get_uclass_plat(dev);
>         struct dsa_priv *priv = dev_get_uclass_priv(dev);
> +       struct dsa_ops *ops = dsa_get_ops(dev);
> +       int err;
>
>         priv->num_ports = pdata->num_ports;
>         priv->cpu_port = pdata->cpu_port;
> @@ -477,6 +479,15 @@ static int dsa_pre_probe(struct udevice *dev)
>
>         uclass_find_device_by_ofnode(UCLASS_ETH, pdata->master_node,
>                                      &priv->master_dev);
> +
> +       /* Simulate a probing event for the CPU port */
> +       if (ops->port_probe) {
> +               err = ops->port_probe(dev, priv->cpu_port,
> +                                     priv->cpu_port_fixed_phy);
> +               if (err)
> +                       return err;
> +       }
> +
>         return 0;
>  }
>
> --
> 2.25.1
>
Reviewed-by: Ramon Fried <rfried.dev@gmail.com>

Re: [PATCH 1/2] net: dsa: fix phydev->speed being uninitialized for the CPU port fixed PHY

[Ramon Fried]

On Sat, Jan 15, 2022 at 6:46 PM Ramon Fried <rfried.dev@gmail.com> wrote:
>
> On Sun, Dec 5, 2021 at 1:01 AM Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
> >
> > If the DSA API is going to allow drivers to do things such as:
> >
> > - phy_config in dsa_ops :: port_probe
> > - phy_startup in dsa_ops :: port_enable
> >
> > then it would actually be good if the ->port_probe() method would
> > actually be called in all cases before the ->port_enable() is.
> >
> > Currently this is true for user ports, but not true for the CPU port,
> > because the CPU port does not have a udevice registered for it (this is
> > all part of DSA's design). So the current issue is that after
> > phy_startup has finished for the CPU port, its phydev->speed is an
> > uninitialized value, because phy_config() was never called for the
> > priv->cpu_port_fixed_phy, and it is precisely phy_config() who copies
> > the speed into the phydev in the case of the fixed PHY driver.
> >
> > So we need to simulate a probing event for the CPU port by manually
> > calling the driver's ->port_probe() method for the CPU port.
> >
> > Fixes: 8a2982574854 ("net: dsa: introduce a .port_probe() method in struct dsa_ops")
> > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> > ---
> >  net/dsa-uclass.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> >
> > diff --git a/net/dsa-uclass.c b/net/dsa-uclass.c
> > index 606b1539a776..9ff55a02fb23 100644
> > --- a/net/dsa-uclass.c
> > +++ b/net/dsa-uclass.c
> > @@ -466,6 +466,8 @@ static int dsa_pre_probe(struct udevice *dev)
> >  {
> >         struct dsa_pdata *pdata = dev_get_uclass_plat(dev);
> >         struct dsa_priv *priv = dev_get_uclass_priv(dev);
> > +       struct dsa_ops *ops = dsa_get_ops(dev);
> > +       int err;
> >
> >         priv->num_ports = pdata->num_ports;
> >         priv->cpu_port = pdata->cpu_port;
> > @@ -477,6 +479,15 @@ static int dsa_pre_probe(struct udevice *dev)
> >
> >         uclass_find_device_by_ofnode(UCLASS_ETH, pdata->master_node,
> >                                      &priv->master_dev);
> > +
> > +       /* Simulate a probing event for the CPU port */
> > +       if (ops->port_probe) {
> > +               err = ops->port_probe(dev, priv->cpu_port,
> > +                                     priv->cpu_port_fixed_phy);
> > +               if (err)
> > +                       return err;
> > +       }
> > +
> >         return 0;
> >  }
> >
> > --
> > 2.25.1
> >
> Reviewed-by: Ramon Fried <rfried.dev@gmail.com>
Applied to u-boot-net/next,
Thanks !

Re: [PATCH 1/2] net: dsa: fix phydev->speed being uninitialized for the CPU port fixed PHY

[Tim Harvey]

On Sat, Dec 4, 2021 at 3:01 PM Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
>
> If the DSA API is going to allow drivers to do things such as:
>
> - phy_config in dsa_ops :: port_probe
> - phy_startup in dsa_ops :: port_enable
>
> then it would actually be good if the ->port_probe() method would
> actually be called in all cases before the ->port_enable() is.
>
> Currently this is true for user ports, but not true for the CPU port,
> because the CPU port does not have a udevice registered for it (this is
> all part of DSA's design). So the current issue is that after
> phy_startup has finished for the CPU port, its phydev->speed is an
> uninitialized value, because phy_config() was never called for the
> priv->cpu_port_fixed_phy, and it is precisely phy_config() who copies
> the speed into the phydev in the case of the fixed PHY driver.
>
> So we need to simulate a probing event for the CPU port by manually
> calling the driver's ->port_probe() method for the CPU port.
>
> Fixes: 8a2982574854 ("net: dsa: introduce a .port_probe() method in struct dsa_ops")
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
>  net/dsa-uclass.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/net/dsa-uclass.c b/net/dsa-uclass.c
> index 606b1539a776..9ff55a02fb23 100644
> --- a/net/dsa-uclass.c
> +++ b/net/dsa-uclass.c
> @@ -466,6 +466,8 @@ static int dsa_pre_probe(struct udevice *dev)
>  {
>         struct dsa_pdata *pdata = dev_get_uclass_plat(dev);
>         struct dsa_priv *priv = dev_get_uclass_priv(dev);
> +       struct dsa_ops *ops = dsa_get_ops(dev);
> +       int err;
>
>         priv->num_ports = pdata->num_ports;
>         priv->cpu_port = pdata->cpu_port;
> @@ -477,6 +479,15 @@ static int dsa_pre_probe(struct udevice *dev)
>
>         uclass_find_device_by_ofnode(UCLASS_ETH, pdata->master_node,
>                                      &priv->master_dev);
> +
> +       /* Simulate a probing event for the CPU port */
> +       if (ops->port_probe) {
> +               err = ops->port_probe(dev, priv->cpu_port,
> +                                     priv->cpu_port_fixed_phy);
> +               if (err)
> +                       return err;
> +       }
> +
>         return 0;
>  }
>
> --
> 2.25.1
>

Vladimir,

I came across this while looking for the best place to configure cpu
port interface mode (ie rgmii id) for the mv88e61xx dsa driver I'm
working on. Note that this patch causes port_probe to be called on the
cpu port 'before' the master device has been probed. I'm not sure if
this is intended or not?

In my case I was looking to configure the cpu port interface mode when
the port was probed but I can't do that because it happens before the
switch is probed because of some things that need to happen there.
Best Regards,

Tim

Re: [PATCH 1/2] net: dsa: fix phydev->speed being uninitialized for the CPU port fixed PHY

[Vladimir Oltean]

Hi Tim,

On Fri, Mar 25, 2022 at 09:53:20AM -0700, Tim Harvey wrote:
> Vladimir,
> 
> I came across this while looking for the best place to configure cpu
> port interface mode (ie rgmii id) for the mv88e61xx dsa driver I'm
> working on. Note that this patch causes port_probe to be called on the
> cpu port 'before' the master device has been probed. I'm not sure if
> this is intended or not?
> 
> In my case I was looking to configure the cpu port interface mode when
> the port was probed but I can't do that because it happens before the
> switch is probed because of some things that need to happen there.
> Best Regards,
> 
> Tim

You're past the DM_MDIO probing issues now? Glad to hear that.
Probing the DSA CPU port before the master wasn't necessarily the
intention, but then again, I'm not sure that there's a strict ordering
guarantee between the two that needs to be satisfied?

What do you need exactly? We could probably always reverse the
device_probe(master) call with the probing of the CPU port if the
ordering turns out to be a real problem. I can regression-test the
change on my boards, but I'd like to understand the need you have, maybe
even document it so that future changes are aware of it.

Re: [PATCH 1/2] net: dsa: fix phydev->speed being uninitialized for the CPU port fixed PHY

[Tim Harvey]

On Fri, Mar 25, 2022 at 11:07 AM Vladimir Oltean
<vladimir.oltean@nxp.com> wrote:
>
> Hi Tim,
>
> On Fri, Mar 25, 2022 at 09:53:20AM -0700, Tim Harvey wrote:
> > Vladimir,
> >
> > I came across this while looking for the best place to configure cpu
> > port interface mode (ie rgmii id) for the mv88e61xx dsa driver I'm
> > working on. Note that this patch causes port_probe to be called on the
> > cpu port 'before' the master device has been probed. I'm not sure if
> > this is intended or not?
> >
> > In my case I was looking to configure the cpu port interface mode when
> > the port was probed but I can't do that because it happens before the
> > switch is probed because of some things that need to happen there.
> > Best Regards,
> >
> > Tim
>
> You're past the DM_MDIO probing issues now? Glad to hear that.
> Probing the DSA CPU port before the master wasn't necessarily the
> intention, but then again, I'm not sure that there's a strict ordering
> guarantee between the two that needs to be satisfied?
>
> What do you need exactly? We could probably always reverse the
> device_probe(master) call with the probing of the CPU port if the
> ordering turns out to be a real problem. I can regression-test the
> change on my boards, but I'd like to understand the need you have, maybe
> even document it so that future changes are aware of it.

Yes, I've got the mdio probing working now. The mv88e61xx driver
supports several chips that have different register offsets that need
to be known before indirect read/write and port read/write can be
used. That detection happens early on so I have it in the dsa_probe. I
would prefer to configure the cpu port interface mode (mac mode, link
speed/duplex etc) once instead of doing it every time the cpu port
gets enabled so I want to put that in the dsa_probe but at that time I
don't have the phy_device to determine interface mode (I suppose I
could get it from dt?). I noticed dsa_class calls ops->port_probe for
the cpu port early so that seemed like a great place to do all that,
but then I found my switch dsa_probe hadn't been called yet so I
haven't identified the switch and set the register offsets yet.

I have worked around the fact that the port_probe is called for the
cpu port before the switch is probed I just wasn't sure if something
in this patch should be changed in case others fall into this trap in
the future. dsa_port_probe probes the master before calling
ops->port_probe with the comment 'We depend on the master device for
proper operation' so I just figured the same should be done for the
pre_probe.

I hope to send a series in the next few days but I do have a few items
I still need to fix:

1. my board currently uses the mv88e61xx_hw_reset weak override to
configure LEDs, GPIO's using direct register writes as I need one of
the GPIO's to be configured as a 125MHz RGMII_RCLK and configure MAC
interface mode(rgmii delays). I've moved the mac interface config into
the driver based on the dt props (phy-mode and fixed-link subnode) but
am still not sure how to go about dealing with the 'very custom' LED
and GPIO config without the hassle of defining new dt bindings. I was
hoping to use board_phy_config() but when that is called for the
switch the phy_id is a generic PHY_FIXED_ID and when called for the
ports the phydev->bus uses indirect register writes which can't be
used to configure the gpios.
2. While my switch mdio bus is probing now as well as my fec_dm_mdio
bus I'm not clear how to properly get the struct mii_dev * associated
with the fec_dm_mdio from the dsa switch. Currently I'm using
miiphy_get_dev_by_name("mdio") which is horrible. How do I get the
mii_bus or even the udevice of an dm_mdio bus associated with a dm_eth
device?

Best Regards,

Tim

Re: [PATCH 1/2] net: dsa: fix phydev->speed being uninitialized for the CPU port fixed PHY

[Vladimir Oltean]

On Fri, Mar 25, 2022 at 02:03:56PM -0700, Tim Harvey wrote:
> On Fri, Mar 25, 2022 at 11:07 AM Vladimir Oltean
> <vladimir.oltean@nxp.com> wrote:
> >
> > Hi Tim,
> >
> > On Fri, Mar 25, 2022 at 09:53:20AM -0700, Tim Harvey wrote:
> > > Vladimir,
> > >
> > > I came across this while looking for the best place to configure cpu
> > > port interface mode (ie rgmii id) for the mv88e61xx dsa driver I'm
> > > working on. Note that this patch causes port_probe to be called on the
> > > cpu port 'before' the master device has been probed. I'm not sure if
> > > this is intended or not?
> > >
> > > In my case I was looking to configure the cpu port interface mode when
> > > the port was probed but I can't do that because it happens before the
> > > switch is probed because of some things that need to happen there.
> > > Best Regards,
> > >
> > > Tim
> >
> > You're past the DM_MDIO probing issues now? Glad to hear that.
> > Probing the DSA CPU port before the master wasn't necessarily the
> > intention, but then again, I'm not sure that there's a strict ordering
> > guarantee between the two that needs to be satisfied?
> >
> > What do you need exactly? We could probably always reverse the
> > device_probe(master) call with the probing of the CPU port if the
> > ordering turns out to be a real problem. I can regression-test the
> > change on my boards, but I'd like to understand the need you have, maybe
> > even document it so that future changes are aware of it.
> 
> Yes, I've got the mdio probing working now. The mv88e61xx driver
> supports several chips that have different register offsets that need
> to be known before indirect read/write and port read/write can be
> used. That detection happens early on so I have it in the dsa_probe. I
> would prefer to configure the cpu port interface mode (mac mode, link
> speed/duplex etc) once instead of doing it every time the cpu port
> gets enabled so I want to put that in the dsa_probe but at that time I
> don't have the phy_device to determine interface mode (I suppose I
> could get it from dt?). I noticed dsa_class calls ops->port_probe for
> the cpu port early so that seemed like a great place to do all that,
> but then I found my switch dsa_probe hadn't been called yet so I
> haven't identified the switch and set the register offsets yet.
> 
> I have worked around the fact that the port_probe is called for the
> cpu port before the switch is probed I just wasn't sure if something
> in this patch should be changed in case others fall into this trap in
> the future. dsa_port_probe probes the master before calling
> ops->port_probe with the comment 'We depend on the master device for
> proper operation' so I just figured the same should be done for the
> pre_probe.

Sorry, that was quite a blunder, we should definitely ensure that
U_BOOT_DRIVER :: probe gets called before dsa_ops :: port_probe.
I've made a change moving the port_probe call for the CPU port to
dsa_post_probe(), tested it in the sandbox, and it appears to work.

> I hope to send a series in the next few days but I do have a few items
> I still need to fix:
> 
> 1. my board currently uses the mv88e61xx_hw_reset weak override to
> configure LEDs, GPIO's using direct register writes as I need one of
> the GPIO's to be configured as a 125MHz RGMII_RCLK and configure MAC
> interface mode(rgmii delays). I've moved the mac interface config into
> the driver based on the dt props (phy-mode and fixed-link subnode) but
> am still not sure how to go about dealing with the 'very custom' LED
> and GPIO config without the hassle of defining new dt bindings. I was
> hoping to use board_phy_config() but when that is called for the
> switch the phy_id is a generic PHY_FIXED_ID and when called for the
> ports the phydev->bus uses indirect register writes which can't be
> used to configure the gpios.

How are these configs handled in Linux?

> 2. While my switch mdio bus is probing now as well as my fec_dm_mdio
> bus I'm not clear how to properly get the struct mii_dev * associated
> with the fec_dm_mdio from the dsa switch. Currently I'm using
> miiphy_get_dev_by_name("mdio") which is horrible. How do I get the
> mii_bus or even the udevice of an dm_mdio bus associated with a dm_eth
> device?

Hmm, isn't the MDIO bus udevice the dev->parent of the switch?
Assuming that the reason you need this is for MDIO input/output towards
the switch. Although my suggestion would be to wrap this I/O behind
dm_mdio_read(struct udevice *dev /* MDIO child device */) and
dm_mdio_write(struct udevice *dev), rather than poking inside struct
udevice from the mv88e61xx driver.

Re: [PATCH 1/2] net: dsa: fix phydev->speed being uninitialized for the CPU port fixed PHY

[Tim Harvey]

On Mon, Mar 28, 2022 at 2:26 AM Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
>
> On Fri, Mar 25, 2022 at 02:03:56PM -0700, Tim Harvey wrote:
> > On Fri, Mar 25, 2022 at 11:07 AM Vladimir Oltean
> > <vladimir.oltean@nxp.com> wrote:
> > >
> > > Hi Tim,
> > >
> > > On Fri, Mar 25, 2022 at 09:53:20AM -0700, Tim Harvey wrote:
> > > > Vladimir,
> > > >
> > > > I came across this while looking for the best place to configure cpu
> > > > port interface mode (ie rgmii id) for the mv88e61xx dsa driver I'm
> > > > working on. Note that this patch causes port_probe to be called on the
> > > > cpu port 'before' the master device has been probed. I'm not sure if
> > > > this is intended or not?
> > > >
> > > > In my case I was looking to configure the cpu port interface mode when
> > > > the port was probed but I can't do that because it happens before the
> > > > switch is probed because of some things that need to happen there.
> > > > Best Regards,
> > > >
> > > > Tim
> > >
> > > You're past the DM_MDIO probing issues now? Glad to hear that.
> > > Probing the DSA CPU port before the master wasn't necessarily the
> > > intention, but then again, I'm not sure that there's a strict ordering
> > > guarantee between the two that needs to be satisfied?
> > >
> > > What do you need exactly? We could probably always reverse the
> > > device_probe(master) call with the probing of the CPU port if the
> > > ordering turns out to be a real problem. I can regression-test the
> > > change on my boards, but I'd like to understand the need you have, maybe
> > > even document it so that future changes are aware of it.
> >
> > Yes, I've got the mdio probing working now. The mv88e61xx driver
> > supports several chips that have different register offsets that need
> > to be known before indirect read/write and port read/write can be
> > used. That detection happens early on so I have it in the dsa_probe. I
> > would prefer to configure the cpu port interface mode (mac mode, link
> > speed/duplex etc) once instead of doing it every time the cpu port
> > gets enabled so I want to put that in the dsa_probe but at that time I
> > don't have the phy_device to determine interface mode (I suppose I
> > could get it from dt?). I noticed dsa_class calls ops->port_probe for
> > the cpu port early so that seemed like a great place to do all that,
> > but then I found my switch dsa_probe hadn't been called yet so I
> > haven't identified the switch and set the register offsets yet.
> >
> > I have worked around the fact that the port_probe is called for the
> > cpu port before the switch is probed I just wasn't sure if something
> > in this patch should be changed in case others fall into this trap in
> > the future. dsa_port_probe probes the master before calling
> > ops->port_probe with the comment 'We depend on the master device for
> > proper operation' so I just figured the same should be done for the
> > pre_probe.
>
> Sorry, that was quite a blunder, we should definitely ensure that
> U_BOOT_DRIVER :: probe gets called before dsa_ops :: port_probe.
> I've made a change moving the port_probe call for the CPU port to
> dsa_post_probe(), tested it in the sandbox, and it appears to work.
>

Ok, sounds good - did you submit this someplace? I haven't seen it.

> > I hope to send a series in the next few days but I do have a few items
> > I still need to fix:
> >
> > 1. my board currently uses the mv88e61xx_hw_reset weak override to
> > configure LEDs, GPIO's using direct register writes as I need one of
> > the GPIO's to be configured as a 125MHz RGMII_RCLK and configure MAC
> > interface mode(rgmii delays). I've moved the mac interface config into
> > the driver based on the dt props (phy-mode and fixed-link subnode) but
> > am still not sure how to go about dealing with the 'very custom' LED
> > and GPIO config without the hassle of defining new dt bindings. I was
> > hoping to use board_phy_config() but when that is called for the
> > switch the phy_id is a generic PHY_FIXED_ID and when called for the
> > ports the phydev->bus uses indirect register writes which can't be
> > used to configure the gpios.
>
> How are these configs handled in Linux?

Each of the 14 GPIO's for MV88E61xx can be configured as GPIO,
PTP_TRIG (PTP output trigger), PTP_EVREQ (PTP event request input),
PTP_EXTCLK (PTP ext clock input), SE_RCLK0 (SyncE RX clock 0 output),
SE_RCLK1 (SyncE RX clock 1 output), or CLK125 (125 MHz clock output).
Currently the only config handled in Linux is PTP_EVREQ for PTP
support.

Each port has 2 LED's which can be configured for 16 different
functions. The LED config is not handled at all the the Linux driver
(PHY LED config is spotty in general).

I think it makes sense to handle this in board_config_phy() and I can
do and avoid the issue of direct register access with something like:

int board_phy_config(struct phy_device *phydev)
{
        unsigned short val;

        /* Fixed PHY: for GW5904/GW5909 this is Marvell 88E6176 GbE Switch */
        if (phydev->phy_id == 0xa5a55a5a &&
                ((board_type == GW5904) || (board_type == GW5909))) {
               /* get the mdio parent bus so we have direct register access */
               struct mii_dev *bus = miiphy_get_dev_by_name("mdio");

                puts("MV88E61XX ");

                /* GPIO[0] output CLK125 for RGMII_REFCLK:
                 * GPIO[7:0] Direction register is 0x62 of Scratch
registers: 1=input 0=output
                 * GPIO[0] Pin Control register is 0x68 of Scratch
registers: 7=CLK125 (Free running 125MHz clock output)
                 * Scratch register access is at 0x1a of GLOBAL2 register (0x1c)
                 */
                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)
                 *  LED Control is register 0x16 of port registers
                 *  port registers are at 0x10 + port
                 */
                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);

        }
}

>
> > 2. While my switch mdio bus is probing now as well as my fec_dm_mdio
> > bus I'm not clear how to properly get the struct mii_dev * associated
> > with the fec_dm_mdio from the dsa switch. Currently I'm using
> > miiphy_get_dev_by_name("mdio") which is horrible. How do I get the
> > mii_bus or even the udevice of an dm_mdio bus associated with a dm_eth
> > device?
>
> Hmm, isn't the MDIO bus udevice the dev->parent of the switch?

My dt is:

&fec {
        pinctrl-names = "default";
        pinctrl-0 = <&pinctrl_enet>;
        phy-mode = "rgmii-id";
        status = "okay";

        fixed-link {
                speed = <1000>;
                full-duplex;
        };

        mdio {
                #address-cells = <1>;
                #size-cells = <0>;

                switch@0 {
                        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>;

                                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;
                                        };
                                };
                        };
                };
        };
};

Assuming this looks correct to you the dm_fec_mdio UCLASS_MDIO gets
bound to the 'mdio' node and its parent is the fec eth device. So in
my switch probe the parent of the switch is the dm_fec_mdio device but
I'm not clear how to get to that udevice's strict mii_dev*.

> Assuming that the reason you need this is for MDIO input/output towards
> the switch. Although my suggestion would be to wrap this I/O behind
> dm_mdio_read(struct udevice *dev /* MDIO child device */) and
> dm_mdio_write(struct udevice *dev), rather than poking inside struct
> udevice from the mv88e61xx driver.

I'm not clear what you mean by the above?

My current pass at adding DSA support to the mv88e61xx driver is to
change the struct phy_device *phydev argument to struct udevice *dev
throughout (which actually isn't as messy as it seems) and I store the
struct mii_dev* in the switch priv structure to be used by the low
level functions needing mii.

Best Regards,

Tim

Re: [PATCH 1/2] net: dsa: fix phydev->speed being uninitialized for the CPU port fixed PHY

[Vladimir Oltean]

On Mon, Mar 28, 2022 at 03:23:02PM -0700, Tim Harvey wrote:
> On Mon, Mar 28, 2022 at 2:26 AM Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
> >
> > On Fri, Mar 25, 2022 at 02:03:56PM -0700, Tim Harvey wrote:
> > > On Fri, Mar 25, 2022 at 11:07 AM Vladimir Oltean
> > > <vladimir.oltean@nxp.com> wrote:
> > > >
> > > > Hi Tim,
> > > >
> > > > On Fri, Mar 25, 2022 at 09:53:20AM -0700, Tim Harvey wrote:
> > > > > Vladimir,
> > > > >
> > > > > I came across this while looking for the best place to configure cpu
> > > > > port interface mode (ie rgmii id) for the mv88e61xx dsa driver I'm
> > > > > working on. Note that this patch causes port_probe to be called on the
> > > > > cpu port 'before' the master device has been probed. I'm not sure if
> > > > > this is intended or not?
> > > > >
> > > > > In my case I was looking to configure the cpu port interface mode when
> > > > > the port was probed but I can't do that because it happens before the
> > > > > switch is probed because of some things that need to happen there.
> > > > > Best Regards,
> > > > >
> > > > > Tim
> > > >
> > > > You're past the DM_MDIO probing issues now? Glad to hear that.
> > > > Probing the DSA CPU port before the master wasn't necessarily the
> > > > intention, but then again, I'm not sure that there's a strict ordering
> > > > guarantee between the two that needs to be satisfied?
> > > >
> > > > What do you need exactly? We could probably always reverse the
> > > > device_probe(master) call with the probing of the CPU port if the
> > > > ordering turns out to be a real problem. I can regression-test the
> > > > change on my boards, but I'd like to understand the need you have, maybe
> > > > even document it so that future changes are aware of it.
> > >
> > > Yes, I've got the mdio probing working now. The mv88e61xx driver
> > > supports several chips that have different register offsets that need
> > > to be known before indirect read/write and port read/write can be
> > > used. That detection happens early on so I have it in the dsa_probe. I
> > > would prefer to configure the cpu port interface mode (mac mode, link
> > > speed/duplex etc) once instead of doing it every time the cpu port
> > > gets enabled so I want to put that in the dsa_probe but at that time I
> > > don't have the phy_device to determine interface mode (I suppose I
> > > could get it from dt?). I noticed dsa_class calls ops->port_probe for
> > > the cpu port early so that seemed like a great place to do all that,
> > > but then I found my switch dsa_probe hadn't been called yet so I
> > > haven't identified the switch and set the register offsets yet.
> > >
> > > I have worked around the fact that the port_probe is called for the
> > > cpu port before the switch is probed I just wasn't sure if something
> > > in this patch should be changed in case others fall into this trap in
> > > the future. dsa_port_probe probes the master before calling
> > > ops->port_probe with the comment 'We depend on the master device for
> > > proper operation' so I just figured the same should be done for the
> > > pre_probe.
> >
> > Sorry, that was quite a blunder, we should definitely ensure that
> > U_BOOT_DRIVER :: probe gets called before dsa_ops :: port_probe.
> > I've made a change moving the port_probe call for the CPU port to
> > dsa_post_probe(), tested it in the sandbox, and it appears to work.
> >
> 
> Ok, sounds good - did you submit this someplace? I haven't seen it.

No, I didn't submit it anywhere. My thinking is that since the existing
DSA drivers aren't critically affected by the calling order, you could
include a change along those lines in your work for Marvell switches.

> > > I hope to send a series in the next few days but I do have a few items
> > > I still need to fix:
> > >
> > > 1. my board currently uses the mv88e61xx_hw_reset weak override to
> > > configure LEDs, GPIO's using direct register writes as I need one of
> > > the GPIO's to be configured as a 125MHz RGMII_RCLK and configure MAC
> > > interface mode(rgmii delays). I've moved the mac interface config into
> > > the driver based on the dt props (phy-mode and fixed-link subnode) but
> > > am still not sure how to go about dealing with the 'very custom' LED
> > > and GPIO config without the hassle of defining new dt bindings. I was
> > > hoping to use board_phy_config() but when that is called for the
> > > switch the phy_id is a generic PHY_FIXED_ID and when called for the
> > > ports the phydev->bus uses indirect register writes which can't be
> > > used to configure the gpios.
> >
> > How are these configs handled in Linux?
> 
> Each of the 14 GPIO's for MV88E61xx can be configured as GPIO,
> PTP_TRIG (PTP output trigger), PTP_EVREQ (PTP event request input),
> PTP_EXTCLK (PTP ext clock input), SE_RCLK0 (SyncE RX clock 0 output),
> SE_RCLK1 (SyncE RX clock 1 output), or CLK125 (125 MHz clock output).
> Currently the only config handled in Linux is PTP_EVREQ for PTP
> support.
> 
> Each port has 2 LED's which can be configured for 16 different
> functions. The LED config is not handled at all the the Linux driver
> (PHY LED config is spotty in general).
> 
> I think it makes sense to handle this in board_config_phy() and I can
> do and avoid the issue of direct register access with something like:
> 
> int board_phy_config(struct phy_device *phydev)
> {
>         unsigned short val;
> 
>         /* Fixed PHY: for GW5904/GW5909 this is Marvell 88E6176 GbE Switch */
>         if (phydev->phy_id == 0xa5a55a5a &&
>                 ((board_type == GW5904) || (board_type == GW5909))) {
>                /* get the mdio parent bus so we have direct register access */
>                struct mii_dev *bus = miiphy_get_dev_by_name("mdio");
> 
>                 puts("MV88E61XX ");
> 
>                 /* GPIO[0] output CLK125 for RGMII_REFCLK:
>                  * GPIO[7:0] Direction register is 0x62 of Scratch registers: 1=input 0=output
>                  * GPIO[0] Pin Control register is 0x68 of Scratch registers: 7=CLK125 (Free running 125MHz clock output)
>                  * Scratch register access is at 0x1a of GLOBAL2 register (0x1c)
>                  */
>                 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)
>                  *  LED Control is register 0x16 of port registers
>                  *  port registers are at 0x10 + port
>                  */
>                 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);
> 
>         }
> }
> 
> >
> > > 2. While my switch mdio bus is probing now as well as my fec_dm_mdio
> > > bus I'm not clear how to properly get the struct mii_dev * associated
> > > with the fec_dm_mdio from the dsa switch. Currently I'm using
> > > miiphy_get_dev_by_name("mdio") which is horrible. How do I get the
> > > mii_bus or even the udevice of an dm_mdio bus associated with a dm_eth
> > > device?
> >
> > Hmm, isn't the MDIO bus udevice the dev->parent of the switch?
> 
> My dt is:
> 
> &fec {
>         pinctrl-names = "default";
>         pinctrl-0 = <&pinctrl_enet>;
>         phy-mode = "rgmii-id";
>         status = "okay";
> 
>         fixed-link {
>                 speed = <1000>;
>                 full-duplex;
>         };
> 
>         mdio {
>                 #address-cells = <1>;
>                 #size-cells = <0>;
> 
>                 switch@0 {
>                         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>;
> 
>                                 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;
>                                         };
>                                 };
>                         };
>                 };
>         };
> };
> 
> Assuming this looks correct to you the dm_fec_mdio UCLASS_MDIO gets
> bound to the 'mdio' node and its parent is the fec eth device. So in
> my switch probe the parent of the switch is the dm_fec_mdio device but
> I'm not clear how to get to that udevice's strict mii_dev*.

My confusion comes from the fact that I don't really understand why you
want to get to the struct mii_dev. In DM world, that structure would be
at dev_get_uclass_priv(dev->parent)->mii_bus from the perspective of the
DM_DSA driver, but see below.

> > Assuming that the reason you need this is for MDIO input/output towards
> > the switch. Although my suggestion would be to wrap this I/O behind
> > dm_mdio_read(struct udevice *dev /* MDIO child device */) and
> > dm_mdio_write(struct udevice *dev), rather than poking inside struct
> > udevice from the mv88e61xx driver.
> 
> I'm not clear what you mean by the above?

What I meant by the above was that I was assuming you wouldn't attempt
to configure the switch in any way outside of its DM_DSA driver, that is
kind of the point of the device model. Which is also the point of me
pointing out that the MDIO bus udevice is the dev->parent of the DM_DSA
udevice, with no implications whatsoever to what you should do to access
the switch "cleaner" from board/ files.

I know of no clean way to mix DM code with board code, maybe others
reading this could chime in.

For LED control, perhaps you could come up with some "one size fits all"
configuration which is done from the DM_DSA driver rather than the board
files. Although you're probably doing this in the first place so that
you don't have to do it in Linux, case in which...

Generally the stance in the ARM DT world has been that Linux takes care
of initializing the hardware up to the state it needs, assuming a blank
slate, and the bootloader minimally initializes the peripherals it needs
for a successful boot process.

For the 125 MHz clock, "qca,clk-out-frequency" from drivers/net/phy/atheros.c
seems to me like a close enough precedent that's supported in U-Boot as
well. I would propose some DT bindings and certainly make sure to run
them by the Linux community first, even as an RFC, then do the same here
once there is basic agreement.

> My current pass at adding DSA support to the mv88e61xx driver is to
> change the struct phy_device *phydev argument to struct udevice *dev
> throughout (which actually isn't as messy as it seems) and I store the
> struct mii_dev* in the switch priv structure to be used by the low
> level functions needing mii.

That is quite interesting, I would have expected quite the contrary.
Since DSA uses the device model, and your switch is the first driver to
probe on an MDIO bus (=> which also uses the device model, otherwise it
wouldn't probe), the expectation would be to not make use of
struct mii_dev, but rather go through the struct mdio_ops :: read and
write methods of the DM_MDIO udevice (for which I see no existing
wrapper functions, hence the prior reference to dm_mdio_read and
dm_mdio_write as naming suggestions for these wrapper functions).
The lack of these wrappers is probably explained due to the traditional
lack of DM devices sitting on MDIO buses. This is probably why the code
conversion doesn't seem messy to you.

Re: [PATCH 1/2] net: dsa: fix phydev->speed being uninitialized for the CPU port fixed PHY

[Tim Harvey]

On Mon, Mar 28, 2022 at 5:03 PM Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
>
> On Mon, Mar 28, 2022 at 03:23:02PM -0700, Tim Harvey wrote:
> > On Mon, Mar 28, 2022 at 2:26 AM Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
> > >
> > > On Fri, Mar 25, 2022 at 02:03:56PM -0700, Tim Harvey wrote:
> > > > On Fri, Mar 25, 2022 at 11:07 AM Vladimir Oltean
> > > > <vladimir.oltean@nxp.com> wrote:
> > > > >
> > > > > Hi Tim,
> > > > >
> > > > > On Fri, Mar 25, 2022 at 09:53:20AM -0700, Tim Harvey wrote:
> > > > > > Vladimir,
> > > > > >
> > > > > > I came across this while looking for the best place to configure cpu
> > > > > > port interface mode (ie rgmii id) for the mv88e61xx dsa driver I'm
> > > > > > working on. Note that this patch causes port_probe to be called on the
> > > > > > cpu port 'before' the master device has been probed. I'm not sure if
> > > > > > this is intended or not?
> > > > > >
> > > > > > In my case I was looking to configure the cpu port interface mode when
> > > > > > the port was probed but I can't do that because it happens before the
> > > > > > switch is probed because of some things that need to happen there.
> > > > > > Best Regards,
> > > > > >
> > > > > > Tim
> > > > >
> > > > > You're past the DM_MDIO probing issues now? Glad to hear that.
> > > > > Probing the DSA CPU port before the master wasn't necessarily the
> > > > > intention, but then again, I'm not sure that there's a strict ordering
> > > > > guarantee between the two that needs to be satisfied?
> > > > >
> > > > > What do you need exactly? We could probably always reverse the
> > > > > device_probe(master) call with the probing of the CPU port if the
> > > > > ordering turns out to be a real problem. I can regression-test the
> > > > > change on my boards, but I'd like to understand the need you have, maybe
> > > > > even document it so that future changes are aware of it.
> > > >
> > > > Yes, I've got the mdio probing working now. The mv88e61xx driver
> > > > supports several chips that have different register offsets that need
> > > > to be known before indirect read/write and port read/write can be
> > > > used. That detection happens early on so I have it in the dsa_probe. I
> > > > would prefer to configure the cpu port interface mode (mac mode, link
> > > > speed/duplex etc) once instead of doing it every time the cpu port
> > > > gets enabled so I want to put that in the dsa_probe but at that time I
> > > > don't have the phy_device to determine interface mode (I suppose I
> > > > could get it from dt?). I noticed dsa_class calls ops->port_probe for
> > > > the cpu port early so that seemed like a great place to do all that,
> > > > but then I found my switch dsa_probe hadn't been called yet so I
> > > > haven't identified the switch and set the register offsets yet.
> > > >
> > > > I have worked around the fact that the port_probe is called for the
> > > > cpu port before the switch is probed I just wasn't sure if something
> > > > in this patch should be changed in case others fall into this trap in
> > > > the future. dsa_port_probe probes the master before calling
> > > > ops->port_probe with the comment 'We depend on the master device for
> > > > proper operation' so I just figured the same should be done for the
> > > > pre_probe.
> > >
> > > Sorry, that was quite a blunder, we should definitely ensure that
> > > U_BOOT_DRIVER :: probe gets called before dsa_ops :: port_probe.
> > > I've made a change moving the port_probe call for the CPU port to
> > > dsa_post_probe(), tested it in the sandbox, and it appears to work.
> > >
> >
> > Ok, sounds good - did you submit this someplace? I haven't seen it.
>
> No, I didn't submit it anywhere. My thinking is that since the existing
> DSA drivers aren't critically affected by the calling order, you could
> include a change along those lines in your work for Marvell switches.
>

Ok, I'll add this to my series

> > > > I hope to send a series in the next few days but I do have a few items
> > > > I still need to fix:
> > > >
> > > > 1. my board currently uses the mv88e61xx_hw_reset weak override to
> > > > configure LEDs, GPIO's using direct register writes as I need one of
> > > > the GPIO's to be configured as a 125MHz RGMII_RCLK and configure MAC
> > > > interface mode(rgmii delays). I've moved the mac interface config into
> > > > the driver based on the dt props (phy-mode and fixed-link subnode) but
> > > > am still not sure how to go about dealing with the 'very custom' LED
> > > > and GPIO config without the hassle of defining new dt bindings. I was
> > > > hoping to use board_phy_config() but when that is called for the
> > > > switch the phy_id is a generic PHY_FIXED_ID and when called for the
> > > > ports the phydev->bus uses indirect register writes which can't be
> > > > used to configure the gpios.
> > >
> > > How are these configs handled in Linux?
> >
> > Each of the 14 GPIO's for MV88E61xx can be configured as GPIO,
> > PTP_TRIG (PTP output trigger), PTP_EVREQ (PTP event request input),
> > PTP_EXTCLK (PTP ext clock input), SE_RCLK0 (SyncE RX clock 0 output),
> > SE_RCLK1 (SyncE RX clock 1 output), or CLK125 (125 MHz clock output).
> > Currently the only config handled in Linux is PTP_EVREQ for PTP
> > support.
> >
> > Each port has 2 LED's which can be configured for 16 different
> > functions. The LED config is not handled at all the the Linux driver
> > (PHY LED config is spotty in general).
> >
> > I think it makes sense to handle this in board_config_phy() and I can
> > do and avoid the issue of direct register access with something like:
> >
> > int board_phy_config(struct phy_device *phydev)
> > {
> >         unsigned short val;
> >
> >         /* Fixed PHY: for GW5904/GW5909 this is Marvell 88E6176 GbE Switch */
> >         if (phydev->phy_id == 0xa5a55a5a &&
> >                 ((board_type == GW5904) || (board_type == GW5909))) {
> >                /* get the mdio parent bus so we have direct register access */
> >                struct mii_dev *bus = miiphy_get_dev_by_name("mdio");
> >
> >                 puts("MV88E61XX ");
> >
> >                 /* GPIO[0] output CLK125 for RGMII_REFCLK:
> >                  * GPIO[7:0] Direction register is 0x62 of Scratch registers: 1=input 0=output
> >                  * GPIO[0] Pin Control register is 0x68 of Scratch registers: 7=CLK125 (Free running 125MHz clock output)
> >                  * Scratch register access is at 0x1a of GLOBAL2 register (0x1c)
> >                  */
> >                 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)
> >                  *  LED Control is register 0x16 of port registers
> >                  *  port registers are at 0x10 + port
> >                  */
> >                 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);
> >
> >         }
> > }
> >
> > >
> > > > 2. While my switch mdio bus is probing now as well as my fec_dm_mdio
> > > > bus I'm not clear how to properly get the struct mii_dev * associated
> > > > with the fec_dm_mdio from the dsa switch. Currently I'm using
> > > > miiphy_get_dev_by_name("mdio") which is horrible. How do I get the
> > > > mii_bus or even the udevice of an dm_mdio bus associated with a dm_eth
> > > > device?
> > >
> > > Hmm, isn't the MDIO bus udevice the dev->parent of the switch?
> >
> > My dt is:
> >
> > &fec {
> >         pinctrl-names = "default";
> >         pinctrl-0 = <&pinctrl_enet>;
> >         phy-mode = "rgmii-id";
> >         status = "okay";
> >
> >         fixed-link {
> >                 speed = <1000>;
> >                 full-duplex;
> >         };
> >
> >         mdio {
> >                 #address-cells = <1>;
> >                 #size-cells = <0>;
> >
> >                 switch@0 {
> >                         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>;
> >
> >                                 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;
> >                                         };
> >                                 };
> >                         };
> >                 };
> >         };
> > };
> >
> > Assuming this looks correct to you the dm_fec_mdio UCLASS_MDIO gets
> > bound to the 'mdio' node and its parent is the fec eth device. So in
> > my switch probe the parent of the switch is the dm_fec_mdio device but
> > I'm not clear how to get to that udevice's strict mii_dev*.
>
> My confusion comes from the fact that I don't really understand why you
> want to get to the struct mii_dev. In DM world, that structure would be
> at dev_get_uclass_priv(dev->parent)->mii_bus from the perspective of the
> DM_DSA driver, but see below.
>

ok, I finally understand what you mean. I was confused when you
mentioned dm_mdio_read/dm_mdio_write as those don't exist until I
finally noticed Marek's patch to add them:
https://patchwork.ozlabs.org/project/uboot/patch/20220318145035.10148-5-kabel@kernel.org/

Now that I understand and agree that dropping the notion of struct
mii_dev is the way to go.

> > > Assuming that the reason you need this is for MDIO input/output towards
> > > the switch. Although my suggestion would be to wrap this I/O behind
> > > dm_mdio_read(struct udevice *dev /* MDIO child device */) and
> > > dm_mdio_write(struct udevice *dev), rather than poking inside struct
> > > udevice from the mv88e61xx driver.
> >
> > I'm not clear what you mean by the above?
>
> What I meant by the above was that I was assuming you wouldn't attempt
> to configure the switch in any way outside of its DM_DSA driver, that is
> kind of the point of the device model. Which is also the point of me
> pointing out that the MDIO bus udevice is the dev->parent of the DM_DSA
> udevice, with no implications whatsoever to what you should do to access
> the switch "cleaner" from board/ files.
>
> I know of no clean way to mix DM code with board code, maybe others
> reading this could chime in.
>
> For LED control, perhaps you could come up with some "one size fits all"
> configuration which is done from the DM_DSA driver rather than the board
> files. Although you're probably doing this in the first place so that
> you don't have to do it in Linux, case in which...

I'm not interested in solving the one size fits all PHY LED
configuration (or GPIO as clock output for that matter). There are
people working on something like that and I'll wait and see where that
goes.

For now I'll just use board_phy_config which is working fine.

>
> Generally the stance in the ARM DT world has been that Linux takes care
> of initializing the hardware up to the state it needs, assuming a blank
> slate, and the bootloader minimally initializes the peripherals it needs
> for a successful boot process.
>
> For the 125 MHz clock, "qca,clk-out-frequency" from drivers/net/phy/atheros.c
> seems to me like a close enough precedent that's supported in U-Boot as
> well. I would propose some DT bindings and certainly make sure to run
> them by the Linux community first, even as an RFC, then do the same here
> once there is basic agreement.
>
> > My current pass at adding DSA support to the mv88e61xx driver is to
> > change the struct phy_device *phydev argument to struct udevice *dev
> > throughout (which actually isn't as messy as it seems) and I store the
> > struct mii_dev* in the switch priv structure to be used by the low
> > level functions needing mii.
>
> That is quite interesting, I would have expected quite the contrary.
> Since DSA uses the device model, and your switch is the first driver to
> probe on an MDIO bus (=> which also uses the device model, otherwise it
> wouldn't probe), the expectation would be to not make use of
> struct mii_dev, but rather go through the struct mdio_ops :: read and
> write methods of the DM_MDIO udevice (for which I see no existing
> wrapper functions, hence the prior reference to dm_mdio_read and
> dm_mdio_write as naming suggestions for these wrapper functions).
> The lack of these wrappers is probably explained due to the traditional
> lack of DM devices sitting on MDIO buses. This is probably why the code
> conversion doesn't seem messy to you.

You may be right that it's cleaner with a completely new driver. I
switched back to that and during review its easy to side-by-side diff
my dsa driver with the old one to see the differences as I've kept the
structure the same.

I thought I was done with the DM_MDIO conversion for fex_mxc but after
some additional testing I see that I can't convert it to use
MDIO_UCLASS for all suers. There are several boards using fec_mxc with
DM_MDIO defined that don't have a proper dt to support it (either
missing the mdio subnode so nothing to bind to, or missing the
phy-mode/phy-handle/fixed-phy nodes so dm_eth_phy_connect would fail.
So I need to make sure fec_mxc binds a DM_MDIO bus and 'attempts'
dm_eth_phy_connect and uses the existing non DM_MDIO as a fallback if
it fails. There are only a couple of eth drivers in U-Boot currently
using UCLASS_MDIO and none of them do this but I think it's likely
because they are all drivers for newere boards with proper dts?

At any rate... getting close now.

Best Regards,

Tim

[PATCH 2/2] net: dsa: sja1105: fix device id detection

[Vladimir Oltean]

The sja1105_check_device_id() function contains logic to work without
changing the device tree on reworked boards, one of which I have (the
NXP LS1021A-TSN normally has a SJA1105T, but I have a version with a
resoldered SJA1105Q which is pin compatible). This logic is taken from
the Linux driver.

However this logic gets shortcircuited in U-Boot by an earlier check for
the exact device ID specified in the device tree. So the reworked board
does not probe the SJA1105Q switch. Remove this duplicated logic and let
the automatic device ID detection do its job.

Fixes: f24b666b2204 ("net: dsa: add driver for NXP SJA1105 L2 switch")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/sja1105.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/net/sja1105.c b/drivers/net/sja1105.c
index 17bab33eddb7..4ca8709e347c 100644
--- a/drivers/net/sja1105.c
+++ b/drivers/net/sja1105.c
@@ -3276,12 +3276,6 @@ static int sja1105_check_device_id(struct sja1105_private *priv)
 	sja1105_packing(packed_buf, &device_id, 31, 0, SJA1105_SIZE_DEVICE_ID,
 			UNPACK);
 
-	if (device_id != priv->info->device_id) {
-		printf("Expected device ID 0x%llx but read 0x%llx\n",
-		       priv->info->device_id, device_id);
-		return -ENODEV;
-	}
-
 	rc = sja1105_xfer_buf(priv, SPI_READ, regs->prod_id, packed_buf,
 			      SJA1105_SIZE_DEVICE_ID);
 	if (rc < 0)
-- 
2.25.1

Re: [PATCH 2/2] net: dsa: sja1105: fix device id detection

[Ramon Fried]

On Sun, Dec 5, 2021 at 1:01 AM Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
>
> The sja1105_check_device_id() function contains logic to work without
> changing the device tree on reworked boards, one of which I have (the
> NXP LS1021A-TSN normally has a SJA1105T, but I have a version with a
> resoldered SJA1105Q which is pin compatible). This logic is taken from
> the Linux driver.
>
> However this logic gets shortcircuited in U-Boot by an earlier check for
> the exact device ID specified in the device tree. So the reworked board
> does not probe the SJA1105Q switch. Remove this duplicated logic and let
> the automatic device ID detection do its job.
>
> Fixes: f24b666b2204 ("net: dsa: add driver for NXP SJA1105 L2 switch")
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
>  drivers/net/sja1105.c | 6 ------
>  1 file changed, 6 deletions(-)
>
> diff --git a/drivers/net/sja1105.c b/drivers/net/sja1105.c
> index 17bab33eddb7..4ca8709e347c 100644
> --- a/drivers/net/sja1105.c
> +++ b/drivers/net/sja1105.c
> @@ -3276,12 +3276,6 @@ static int sja1105_check_device_id(struct sja1105_private *priv)
>         sja1105_packing(packed_buf, &device_id, 31, 0, SJA1105_SIZE_DEVICE_ID,
>                         UNPACK);
>
> -       if (device_id != priv->info->device_id) {
> -               printf("Expected device ID 0x%llx but read 0x%llx\n",
> -                      priv->info->device_id, device_id);
> -               return -ENODEV;
> -       }
> -
>         rc = sja1105_xfer_buf(priv, SPI_READ, regs->prod_id, packed_buf,
>                               SJA1105_SIZE_DEVICE_ID);
>         if (rc < 0)
> --
> 2.25.1
>
Reviewed-by: Ramon Fried <rfried.dev@gmail.com>

Re: [PATCH 2/2] net: dsa: sja1105: fix device id detection

[Ramon Fried]

On Sat, Jan 15, 2022 at 6:48 PM Ramon Fried <rfried.dev@gmail.com> wrote:
>
> On Sun, Dec 5, 2021 at 1:01 AM Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
> >
> > The sja1105_check_device_id() function contains logic to work without
> > changing the device tree on reworked boards, one of which I have (the
> > NXP LS1021A-TSN normally has a SJA1105T, but I have a version with a
> > resoldered SJA1105Q which is pin compatible). This logic is taken from
> > the Linux driver.
> >
> > However this logic gets shortcircuited in U-Boot by an earlier check for
> > the exact device ID specified in the device tree. So the reworked board
> > does not probe the SJA1105Q switch. Remove this duplicated logic and let
> > the automatic device ID detection do its job.
> >
> > Fixes: f24b666b2204 ("net: dsa: add driver for NXP SJA1105 L2 switch")
> > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> > ---
> >  drivers/net/sja1105.c | 6 ------
> >  1 file changed, 6 deletions(-)
> >
> > diff --git a/drivers/net/sja1105.c b/drivers/net/sja1105.c
> > index 17bab33eddb7..4ca8709e347c 100644
> > --- a/drivers/net/sja1105.c
> > +++ b/drivers/net/sja1105.c
> > @@ -3276,12 +3276,6 @@ static int sja1105_check_device_id(struct sja1105_private *priv)
> >         sja1105_packing(packed_buf, &device_id, 31, 0, SJA1105_SIZE_DEVICE_ID,
> >                         UNPACK);
> >
> > -       if (device_id != priv->info->device_id) {
> > -               printf("Expected device ID 0x%llx but read 0x%llx\n",
> > -                      priv->info->device_id, device_id);
> > -               return -ENODEV;
> > -       }
> > -
> >         rc = sja1105_xfer_buf(priv, SPI_READ, regs->prod_id, packed_buf,
> >                               SJA1105_SIZE_DEVICE_ID);
> >         if (rc < 0)
> > --
> > 2.25.1
> >
> Reviewed-by: Ramon Fried <rfried.dev@gmail.com>
Applied to u-boot-net/next
Thanks !