[PATCH net] net: dsa: microchip: keep compatibility with device tree blobs with no phy-mode

[PATCH net] net: dsa: microchip: keep compatibility with device tree blobs with no phy-mode

[Vladimir Oltean]

DSA has multiple ways of specifying a MAC connection to an internal PHY.
One requires a DT description like this:

	port@0 {
		reg = <0>;
		phy-handle = <&internal_phy>;
		phy-mode = "internal";
	};

(which is IMO the recommended approach, as it is the clearest
description)

but it is also possible to leave the specification as just:

	port@0 {
		reg = <0>;
	}

and if the driver implements ds->ops->phy_read and ds->ops->phy_write,
the DSA framework "knows" it should create a ds->slave_mii_bus, and it
should connect to a non-OF-based internal PHY on this MDIO bus, at an
MDIO address equal to the port address.

There is also an intermediary way of describing things:

	port@0 {
		reg = <0>;
		phy-handle = <&internal_phy>;
	};

In case 2, DSA calls phylink_connect_phy() and in case 3, it calls
phylink_of_phy_connect(). In both cases, phylink_create() has been
called with a phy_interface_t of PHY_INTERFACE_MODE_NA, and in both
cases, PHY_INTERFACE_MODE_NA is translated into phy->interface.

It is important to note that phy_device_create() initializes
dev->interface = PHY_INTERFACE_MODE_GMII, and so, when we use
phylink_create(PHY_INTERFACE_MODE_NA), no one will override this, and we
will end up with a PHY_INTERFACE_MODE_GMII interface inherited from the
PHY.

All this means that in order to maintain compatibility with device tree
blobs where the phy-mode property is missing, we need to allow the
"gmii" phy-mode and treat it as "internal".

Fixes: 2c709e0bdad4 ("net: dsa: microchip: ksz8795: add phylink support")
Link: https://bugzilla.kernel.org/show_bug.cgi?id=216320
Reported-by: Craig McQueen <craig@mcqueen.id.au>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/microchip/ksz_common.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index ed7d137cba99..7461272a6d41 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -803,9 +803,15 @@ static void ksz_phylink_get_caps(struct dsa_switch *ds, int port,
 	if (dev->info->supports_rgmii[port])
 		phy_interface_set_rgmii(config->supported_interfaces);
 
-	if (dev->info->internal_phy[port])
+	if (dev->info->internal_phy[port]) {
 		__set_bit(PHY_INTERFACE_MODE_INTERNAL,
 			  config->supported_interfaces);
+		/* Compatibility for phylib's default interface type when the
+		 * phy-mode property is absent
+		 */
+		__set_bit(PHY_INTERFACE_MODE_GMII,
+			  config->supported_interfaces);
+	}
 
 	if (dev->dev_ops->get_caps)
 		dev->dev_ops->get_caps(dev, port, config);
-- 
2.34.1

Re: [PATCH net] net: dsa: microchip: keep compatibility with device tree blobs with no phy-mode

[Alvin Šipraga]

On Thu, Aug 18, 2022 at 05:32:50PM +0300, Vladimir Oltean wrote:
> DSA has multiple ways of specifying a MAC connection to an internal PHY.
> One requires a DT description like this:
> 
> 	port@0 {
> 		reg = <0>;
> 		phy-handle = <&internal_phy>;
> 		phy-mode = "internal";
> 	};
> 
> (which is IMO the recommended approach, as it is the clearest
> description)
> 
> but it is also possible to leave the specification as just:
> 
> 	port@0 {
> 		reg = <0>;
> 	}
> 
> and if the driver implements ds->ops->phy_read and ds->ops->phy_write,
> the DSA framework "knows" it should create a ds->slave_mii_bus, and it
> should connect to a non-OF-based internal PHY on this MDIO bus, at an
> MDIO address equal to the port address.
> 
> There is also an intermediary way of describing things:
> 
> 	port@0 {
> 		reg = <0>;
> 		phy-handle = <&internal_phy>;
> 	};
> 
> In case 2, DSA calls phylink_connect_phy() and in case 3, it calls
> phylink_of_phy_connect(). In both cases, phylink_create() has been
> called with a phy_interface_t of PHY_INTERFACE_MODE_NA, and in both
> cases, PHY_INTERFACE_MODE_NA is translated into phy->interface.
> 
> It is important to note that phy_device_create() initializes
> dev->interface = PHY_INTERFACE_MODE_GMII, and so, when we use
> phylink_create(PHY_INTERFACE_MODE_NA), no one will override this, and we
> will end up with a PHY_INTERFACE_MODE_GMII interface inherited from the
> PHY.
> 
> All this means that in order to maintain compatibility with device tree
> blobs where the phy-mode property is missing, we need to allow the
> "gmii" phy-mode and treat it as "internal".
> 
> Fixes: 2c709e0bdad4 ("net: dsa: microchip: ksz8795: add phylink support")
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=216320
> Reported-by: Craig McQueen <craig@mcqueen.id.au>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
>  drivers/net/dsa/microchip/ksz_common.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)

Reviewed-by: Alvin Šipraga <alsi@bang-olufsen.dk>

Some quick grepping shows at least a few other drivers which do not set
PHY_INTERFACE_MODE_GMII for their ports with internal PHY:

    bcm_sf2
    ar9331 (*)
    lantiq_gswip

Should these be "fixed" too? Or only if somebody reports a regression?

(*) I note that ar9331 ought not to rely on DSA workarounds, per your
other patchset, so I there is actually no need to "fix" that one, since
the new validation you are introducing will require a phy-mode to be
specified for those switches' ports anyway.

> 
> diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
> index ed7d137cba99..7461272a6d41 100644
> --- a/drivers/net/dsa/microchip/ksz_common.c
> +++ b/drivers/net/dsa/microchip/ksz_common.c
> @@ -803,9 +803,15 @@ static void ksz_phylink_get_caps(struct dsa_switch *ds, int port,
>  	if (dev->info->supports_rgmii[port])
>  		phy_interface_set_rgmii(config->supported_interfaces);
>  
> -	if (dev->info->internal_phy[port])
> +	if (dev->info->internal_phy[port]) {
>  		__set_bit(PHY_INTERFACE_MODE_INTERNAL,
>  			  config->supported_interfaces);
> +		/* Compatibility for phylib's default interface type when the
> +		 * phy-mode property is absent
> +		 */
> +		__set_bit(PHY_INTERFACE_MODE_GMII,
> +			  config->supported_interfaces);
> +	}
>  
>  	if (dev->dev_ops->get_caps)
>  		dev->dev_ops->get_caps(dev, port, config);
> -- 
> 2.34.1
>

Re: [PATCH net] net: dsa: microchip: keep compatibility with device tree blobs with no phy-mode

[Vladimir Oltean]

On Thu, Aug 18, 2022 at 03:06:44PM +0000, Alvin Šipraga wrote:
> Reviewed-by: Alvin Šipraga <alsi@bang-olufsen.dk>

Thanks.

> Some quick grepping shows at least a few other drivers which do not set
> PHY_INTERFACE_MODE_GMII for their ports with internal PHY:
> 
>     bcm_sf2
>     ar9331 (*)
>     lantiq_gswip
> 
> Should these be "fixed" too? Or only if somebody reports a regression?
> 
> (*) I note that ar9331 ought not to rely on DSA workarounds, per your
> other patchset, so I there is actually no need to "fix" that one, since
> the new validation you are introducing will require a phy-mode to be
> specified for those switches' ports anyway.

Note that my patch set which you are talking about
https://patchwork.kernel.org/project/netdevbpf/cover/20220818115500.2592578-1-vladimir.oltean@nxp.com/
only enforces phy-mode presence for CPU and DSA ports. Whereas internal
PHYs are more typical on user ports. So the problem is equally
applicable to ar9331, as long as it won't specify a phy-mode but rely on
DSA's assumption that user ports with lacking DT descriptions want to
connect to an internal PHY.

IDK, if I had infinite time I'd go fix every driver. I don't know why
some have GMII and some don't, and I'm not exactly keen to go study
every driver's subtleties unless a problem is reported.

Re: [PATCH net] net: dsa: microchip: keep compatibility with device tree blobs with no phy-mode

[Alvin Šipraga]

On Thu, Aug 18, 2022 at 06:18:06PM +0300, Vladimir Oltean wrote:
> On Thu, Aug 18, 2022 at 03:06:44PM +0000, Alvin Šipraga wrote:
> > Reviewed-by: Alvin Šipraga <alsi@bang-olufsen.dk>
> 
> Thanks.
> 
> > Some quick grepping shows at least a few other drivers which do not set
> > PHY_INTERFACE_MODE_GMII for their ports with internal PHY:
> > 
> >     bcm_sf2
> >     ar9331 (*)
> >     lantiq_gswip
> > 
> > Should these be "fixed" too? Or only if somebody reports a regression?
> > 
> > (*) I note that ar9331 ought not to rely on DSA workarounds, per your
> > other patchset, so I there is actually no need to "fix" that one, since
> > the new validation you are introducing will require a phy-mode to be
> > specified for those switches' ports anyway.
> 
> Note that my patch set which you are talking about
> https://patchwork.kernel.org/project/netdevbpf/cover/20220818115500.2592578-1-vladimir.oltean@nxp.com/
> only enforces phy-mode presence for CPU and DSA ports. Whereas internal
> PHYs are more typical on user ports. So the problem is equally
> applicable to ar9331, as long as it won't specify a phy-mode but rely on
> DSA's assumption that user ports with lacking DT descriptions want to
> connect to an internal PHY.

Thanks for the clarification.

> 
> IDK, if I had infinite time I'd go fix every driver. I don't know why
> some have GMII and some don't, and I'm not exactly keen to go study
> every driver's subtleties unless a problem is reported.

Yes, fair enough!

Kind regards,
Alvin

Re: [PATCH net] net: dsa: microchip: keep compatibility with device tree blobs with no phy-mode

[Andrew Lunn]

> It is important to note that phy_device_create() initializes
> dev->interface = PHY_INTERFACE_MODE_GMII, and so, when we use
> phylink_create(PHY_INTERFACE_MODE_NA), no one will override this, and we
> will end up with a PHY_INTERFACE_MODE_GMII interface inherited from the
> PHY.

Is this actually a bug?

With pure phylib, you should call one of the connect functions, which
underneath calls phy_attach_direct() which has a phy_interface_t. So
the default in practice does not matter.

> All this means that in order to maintain compatibility with device tree
> blobs where the phy-mode property is missing, we need to allow the
> "gmii" phy-mode and treat it as "internal".

of_get_phy_mode() returns PHY_INTERFACE_MODE_NA if the property is
missing, which also suggests this is a bug.

I wonder if we have any ports which actually rely on
PHY_INTERFACE_MODE_GMII?

	 Andrew

Re: [PATCH net] net: dsa: microchip: keep compatibility with device tree blobs with no phy-mode

[Vladimir Oltean]

On Thu, Aug 18, 2022 at 05:13:51PM +0200, Andrew Lunn wrote:
> > It is important to note that phy_device_create() initializes
> > dev->interface = PHY_INTERFACE_MODE_GMII, and so, when we use
> > phylink_create(PHY_INTERFACE_MODE_NA), no one will override this, and we
> > will end up with a PHY_INTERFACE_MODE_GMII interface inherited from the
> > PHY.
> 
> Is this actually a bug?
> 
> With pure phylib, you should call one of the connect functions, which
> underneath calls phy_attach_direct() which has a phy_interface_t. So
> the default in practice does not matter.

What do you consider "bug"? I think here is where phylink inherits what
phy_device_create() set as default:

int phylink_connect_phy(struct phylink *pl, struct phy_device *phy)
{
	int ret;

	/* Use PHY device/driver interface */
	if (pl->link_interface == PHY_INTERFACE_MODE_NA) {
		pl->link_interface = phy->interface;
		pl->link_config.interface = pl->link_interface; <- here
	}

	ret = phylink_attach_phy(pl, phy, pl->link_interface); <- here is the phy_attach_direct()
	if (ret < 0)
		return ret;

	ret = phylink_bringup_phy(pl, phy, pl->link_config.interface);
	if (ret)
		phy_detach(phy);

	return ret;
}

So yes, although you're right in that if you call phy_attach_direct()
this doesn't happen, but if you go through phylink_connect_phy() I think
it's expected that it will.

> > All this means that in order to maintain compatibility with device tree
> > blobs where the phy-mode property is missing, we need to allow the
> > "gmii" phy-mode and treat it as "internal".
> 
> of_get_phy_mode() returns PHY_INTERFACE_MODE_NA if the property is
> missing, which also suggests this is a bug.
> 
> I wonder if we have any ports which actually rely on
> PHY_INTERFACE_MODE_GMII?

What do you mean, you mean actual GMII wired to chip pinout?

Re: [PATCH net] net: dsa: microchip: keep compatibility with device tree blobs with no phy-mode

[Russell King (Oracle)]

On Thu, Aug 18, 2022 at 05:13:51PM +0200, Andrew Lunn wrote:
> > It is important to note that phy_device_create() initializes
> > dev->interface = PHY_INTERFACE_MODE_GMII, and so, when we use
> > phylink_create(PHY_INTERFACE_MODE_NA), no one will override this, and we
> > will end up with a PHY_INTERFACE_MODE_GMII interface inherited from the
> > PHY.
> 
> Is this actually a bug?
> 
> With pure phylib, you should call one of the connect functions, which
> underneath calls phy_attach_direct() which has a phy_interface_t. So
> the default in practice does not matter.
> 
> > All this means that in order to maintain compatibility with device tree
> > blobs where the phy-mode property is missing, we need to allow the
> > "gmii" phy-mode and treat it as "internal".
> 
> of_get_phy_mode() returns PHY_INTERFACE_MODE_NA if the property is
> missing, which also suggests this is a bug.
> 
> I wonder if we have any ports which actually rely on
> PHY_INTERFACE_MODE_GMII?

Loads now. Florian contributed the code to phylink that detects when
DSA initialises phylink with PHY_INTERFACE_MODE_NA, and then looks at
phy->interface _before_ calling phy_attach_direct() - and this is how
we end up with PHY_INTERFACE_MODE_GMII.

See:

commit 4904b6ea1f9dbf47107f50b1c502a22d0160712d
Author: Florian Fainelli <f.fainelli@gmail.com>
Date:   Tue Dec 12 16:00:26 2017 -0800

    net: phy: phylink: Use PHY device interface if N/A

    We may not always be able to resolve a correct phy_interface_t value before
    actually connecting to the PHY device, when that happens, just have
    phylink_connect_phy() utilize what the PHY device/driver provided.

    Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
    Signed-off-by: David S. Miller <davem@davemloft.net>

submitted for 4.16-rc1. DSA then used this in:

commit aab9c4067d2389d0adfc9c53806437df7b0fe3d5
Author: Florian Fainelli <f.fainelli@gmail.com>
Date:   Thu May 10 13:17:36 2018 -0700

    net: dsa: Plug in PHYLINK support
...

for 4.18-rc1 to connect to its PHYs:

static int dsa_slave_phy_connect(struct net_device *slave_dev, int addr)
{
        struct dsa_port *dp = dsa_slave_to_port(slave_dev);
        struct dsa_switch *ds = dp->ds;

        slave_dev->phydev = mdiobus_get_phy(ds->slave_mii_bus, addr);
        if (!slave_dev->phydev) {
                netdev_err(slave_dev, "no phy at %d\n", addr);
                return -ENODEV;
        }

        return phylink_connect_phy(dp->pl, slave_dev->phydev);
}
...
        ret = phylink_of_phy_connect(dp->pl, port_dn, phy_flags);
        if (ret == -ENODEV) {
                /* We could not connect to a designated PHY or SFP, so use the
                 * switch internal MDIO bus instead
                 */
                ret = dsa_slave_phy_connect(slave_dev, dp->index);
                if (ret) {
                        netdev_err(slave_dev,
                                   "failed to connect to port %d: %d\n",
                                   dp->index, ret);
                        phylink_destroy(dp->pl);
                        return ret;
                }
        }

which will be used when there is no phy-handle property.

I extended the change in 4904b6ea1f9dbf47107f50b1c502a22d0160712d to also
apply to fwnode setups in:

commit a18e6521a7d95dae8c65b5b0ef6bbe624fbe808c
Author: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
Date:   Fri Nov 19 16:28:06 2021 +0000

    net: phylink: handle NA interface mode in phylink_fwnode_phy_connect()

because we were ending up with DSA drivers using PHY_INTERFACE_MODE_NA
inside phylink, and phylink has always special-cased that and drivers
have taken it to mean "give me all interface modes that are supported"
which is not what DSA should be using. It also gives a uniform and
understandable behaviour from phylink for DSA, rather than having
phylink behave one way (where no phy-mode and no phy-handle are
specified) but in a completely different way when phy-handle without
phy-mode is specified. It seemed to be the sensible thing to do, and
Florian agreed at the time.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net] net: dsa: microchip: keep compatibility with device tree blobs with no phy-mode

[Rasmus Villemoes]

On 18/08/2022 16.32, Vladimir Oltean wrote:
> DSA has multiple ways of specifying a MAC connection to an internal PHY.
> One requires a DT description like this:
> 
> 	port@0 {
> 		reg = <0>;
> 		phy-handle = <&internal_phy>;
> 		phy-mode = "internal";
> 	};
> 
> (which is IMO the recommended approach, as it is the clearest
> description)
> 
> but it is also possible to leave the specification as just:
> 
> 	port@0 {
> 		reg = <0>;
> 	}
> 
> and if the driver implements ds->ops->phy_read and ds->ops->phy_write,
> the DSA framework "knows" it should create a ds->slave_mii_bus, and it
> should connect to a non-OF-based internal PHY on this MDIO bus, at an
> MDIO address equal to the port address.
> 
> There is also an intermediary way of describing things:
> 
> 	port@0 {
> 		reg = <0>;
> 		phy-handle = <&internal_phy>;
> 	};

Well, there's also e.g. arch/arm/boot/dts/at91-sama5d3_ksz9477_evb.dts
which sets the phy-mode but not the phy-handle:

                        port@0 {
                                reg = <0>;
                                label = "lan1";
                                phy-mode = "internal";
                        };

And doing that in my case seems to fix things (I wouldn't know what
phy-handle to point at anyway), so since we're still in development, I
think I'll do that. But if I want to follow the new-world-order to the
letter, should I also figure out a way to point at a phy-handle?
> Fixes: 2c709e0bdad4 ("net: dsa: microchip: ksz8795: add phylink support")
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=216320
> Reported-by: Craig McQueen <craig@mcqueen.id.au>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

I've also tested this patch on top of v5.19 without altering my .dts,
and that also seems to fix things, so I suppose you can add

Fixes: 65ac79e18120 ("net: dsa: microchip: add the phylink get_caps")
Tested-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>

Thanks,
Rasmus

Re: [PATCH net] net: dsa: microchip: keep compatibility with device tree blobs with no phy-mode

[Vladimir Oltean]

On Fri, Aug 19, 2022 at 12:19:12PM +0200, Rasmus Villemoes wrote:
> Well, there's also e.g. arch/arm/boot/dts/at91-sama5d3_ksz9477_evb.dts
> which sets the phy-mode but not the phy-handle:
> 
>                         port@0 {
>                                 reg = <0>;
>                                 label = "lan1";
>                                 phy-mode = "internal";
>                         };

Yeah, it looks like there's also that variation, curious.

> And doing that in my case seems to fix things (I wouldn't know what
> phy-handle to point at anyway), so since we're still in development, I
> think I'll do that. But if I want to follow the new-world-order to the
> letter, should I also figure out a way to point at a phy-handle?

So if by "new world order" you mean
https://patchwork.kernel.org/project/netdevbpf/cover/20220818115500.2592578-1-vladimir.oltean@nxp.com/
then no, that patch set doesn't change the requirements for *user* ports
(what you have here) but for CPU and DSA ports, where no phy-handle/
fixed-link/phy-mode means something entirely different than it means for
user ports.

To give you an idea of how things work for user ports. If a user port
has a phy-handle, DSA will connect to that, irrespective of what OF-based
MDIO bus that is on. If not, DSA looks at whether ds->slave_mii_bus is
populated with a struct mii_bus by the driver. If it is, it connects in
a non-OF based way to a PHY address equal to the port number. If
ds->slave_mii_bus doesn't exist but the driver provides
ds->ops->phy_read and ds->ops->phy_write, DSA automatically creates
ds->slave_mii_bus where its ops are the driver provided phy_read and
phy_write, and it then does the same thing of connecting to the PHY in
that non-OF based way.

So to convert a driver that currently relies on DSA allocating the
ds->slave_mii_bus, you need to allocate/register it yourself (using the
of_mdiobus_* variants), and populate ds->slave_mii_bus with it. Look at
lan937x_mdio_register() for instance.

For existing device trees which connect in a non-OF based way, you still
need to keep ds->ops->phy_read and ds->ops->phy_write, and let DSA
create the ds->slave_mii_bus. The phy_read and phy_write can be the same
between your MDIO bus ops and DSA's MDIO bus ops.

> > Fixes: 2c709e0bdad4 ("net: dsa: microchip: ksz8795: add phylink support")
> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=216320
> > Reported-by: Craig McQueen <craig@mcqueen.id.au>
> > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> I've also tested this patch on top of v5.19 without altering my .dts,
> and that also seems to fix things, so I suppose you can add
> 
> Fixes: 65ac79e18120 ("net: dsa: microchip: add the phylink get_caps")
> Tested-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>

So I don't intend to make you modify your device tree in this case, but
there is something to be said about U-Boot compatibility. In U-Boot,
with DM_DSA, I don't intend to support any unnecessary complexity and
alternative ways of describing the same thing, so there, phy-mode and
one of phy-handle or fixed-link are mandatory for all ports. And since
U-Boot can pass its own device tree to Linux, it means Linux DSA drivers
might need to gradually gain support for OF-based phy-handle on user
ports as well. So see what Tim Harvey has done in drivers/net/ksz9477.c
in the U-Boot source code, and try to work with his device tree format,
as a starting point.

Re: [PATCH net] net: dsa: microchip: keep compatibility with device tree blobs with no phy-mode

[Rasmus Villemoes]

On 19/08/2022 12.57, Vladimir Oltean wrote:
> On Fri, Aug 19, 2022 at 12:19:12PM +0200, Rasmus Villemoes wrote:
>> Well, there's also e.g. arch/arm/boot/dts/at91-sama5d3_ksz9477_evb.dts
>> which sets the phy-mode but not the phy-handle:
>>
>>                         port@0 {
>>                                 reg = <0>;
>>                                 label = "lan1";
>>                                 phy-mode = "internal";
>>                         };
> 
> Yeah, it looks like there's also that variation, curious.
> 
>> And doing that in my case seems to fix things (I wouldn't know what
>> phy-handle to point at anyway), so since we're still in development, I
>> think I'll do that. But if I want to follow the new-world-order to the
>> letter, should I also figure out a way to point at a phy-handle?
> 
> So if by "new world order" you mean
> https://patchwork.kernel.org/project/netdevbpf/cover/20220818115500.2592578-1-vladimir.oltean@nxp.com/
> then no, that patch set doesn't change the requirements for *user* ports
> (what you have here) but for CPU and DSA ports, where no phy-handle/
> fixed-link/phy-mode means something entirely different than it means for
> user ports.

No, I meant the new world order as of the things merged into v5.19.
Which does seem to require an explicit phy-mode, and I'm happy to add that.

> To give you an idea of how things work for user ports. If a user port
> has a phy-handle, DSA will connect to that, irrespective of what OF-based
> MDIO bus that is on. If not, DSA looks at whether ds->slave_mii_bus is
> populated with a struct mii_bus by the driver. If it is, it connects in
> a non-OF based way to a PHY address equal to the port number. If
> ds->slave_mii_bus doesn't exist but the driver provides
> ds->ops->phy_read and ds->ops->phy_write, DSA automatically creates
> ds->slave_mii_bus where its ops are the driver provided phy_read and
> phy_write, and it then does the same thing of connecting to the PHY in
> that non-OF based way.

Thanks, that's quite useful. From quick grepping, it seems that ksz9567
currently falls into the latter category?

> So to convert a driver that currently relies on DSA allocating the
> ds->slave_mii_bus, you need to allocate/register it yourself (using the
> of_mdiobus_* variants), and populate ds->slave_mii_bus with it. Look at
> lan937x_mdio_register() for instance.
> 
> For existing device trees which connect in a non-OF based way, you still
> need to keep ds->ops->phy_read and ds->ops->phy_write, and let DSA
> create the ds->slave_mii_bus. The phy_read and phy_write can be the same
> between your MDIO bus ops and DSA's MDIO bus ops.
> 
>>> Fixes: 2c709e0bdad4 ("net: dsa: microchip: ksz8795: add phylink support")
>>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=216320
>>> Reported-by: Craig McQueen <craig@mcqueen.id.au>
>>> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
>>
>> I've also tested this patch on top of v5.19 without altering my .dts,
>> and that also seems to fix things, so I suppose you can add
>>
>> Fixes: 65ac79e18120 ("net: dsa: microchip: add the phylink get_caps")
>> Tested-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> 
> So I don't intend to make you modify your device tree in this case, 

No, but it's actually easier for me to just do that rather than carry an
extra patch until the mainline fix hits 5.19.y.

but
> there is something to be said about U-Boot compatibility. In U-Boot,
> with DM_DSA, I don't intend to support any unnecessary complexity and
> alternative ways of describing the same thing, so there, phy-mode and
> one of phy-handle or fixed-link are mandatory for all ports. 

OK. I suppose that means the linux driver for the ksz9477 family  should
learn to check if there's an "mdio" subnode and if so populate
ds->slave_mii_bus, but unlike lan937x, absence of that node should not
be fatal?

And since
> U-Boot can pass its own device tree to Linux, it means Linux DSA drivers
> might need to gradually gain support for OF-based phy-handle on user
> ports as well. So see what Tim Harvey has done in drivers/net/ksz9477.c
> in the U-Boot source code, and try to work with his device tree format,
> as a starting point.

Hm. It does seem like that driver has the mdio bus optional (as in,
probe doesn't fail just because the subnode isn't present). But I'm
curious why ksz_probe_mdio() looks for a subnode called "mdios" rather
than "mdio". Tim, just a typo?

Thanks,
Rasmus

Re: [PATCH net] net: dsa: microchip: keep compatibility with device tree blobs with no phy-mode

[Vladimir Oltean]

On Fri, Aug 19, 2022 at 01:47:51PM +0200, Rasmus Villemoes wrote:
> > To give you an idea of how things work for user ports. If a user port
> > has a phy-handle, DSA will connect to that, irrespective of what OF-based
> > MDIO bus that is on. If not, DSA looks at whether ds->slave_mii_bus is
> > populated with a struct mii_bus by the driver. If it is, it connects in
> > a non-OF based way to a PHY address equal to the port number. If
> > ds->slave_mii_bus doesn't exist but the driver provides
> > ds->ops->phy_read and ds->ops->phy_write, DSA automatically creates
> > ds->slave_mii_bus where its ops are the driver provided phy_read and
> > phy_write, and it then does the same thing of connecting to the PHY in
> > that non-OF based way.
> 
> Thanks, that's quite useful. From quick grepping, it seems that ksz9567
> currently falls into the latter category?

So it would appear.

> No, but it's actually easier for me to just do that rather than carry an
> extra patch until the mainline fix hits 5.19.y.

Whatever suits you best.

> > there is something to be said about U-Boot compatibility. In U-Boot,
> > with DM_DSA, I don't intend to support any unnecessary complexity and
> > alternative ways of describing the same thing, so there, phy-mode and
> > one of phy-handle or fixed-link are mandatory for all ports. 
> 
> OK. I suppose that means the linux driver for the ksz9477 family  should
> learn to check if there's an "mdio" subnode and if so populate
> ds->slave_mii_bus, but unlike lan937x, absence of that node should not
> be fatal?

Yes.

> > U-Boot can pass its own device tree to Linux, it means Linux DSA drivers
> > might need to gradually gain support for OF-based phy-handle on user
> > ports as well. So see what Tim Harvey has done in drivers/net/ksz9477.c
> > in the U-Boot source code, and try to work with his device tree format,
> > as a starting point.
> 
> Hm. It does seem like that driver has the mdio bus optional (as in,
> probe doesn't fail just because the subnode isn't present). But I'm
> curious why ksz_probe_mdio() looks for a subnode called "mdios" rather
> than "mdio". Tim, just a typo?

No, definitely not a typo. I have to explain this for what seems like
the millionth time already, but the idea with an "mdios" container was
copied from the sja1105 driver, where there are actually multiple MDIO
buses. Documentation/devicetree/bindings/net/mdio.yaml wants the $nodename
to have the "^mdio(@.*)?" pattern, and:
- if you use "mdio" you can have a single node
- if you don't put the MDIO nodes under a container with an
  #address-cells != 0, you can't use the "mdio@something" syntax

https://lore.kernel.org/all/20210621234930.espjau5l2t5dr75y@skbuf/T/#me43cf6b1976a3c3aec8357f19ab967f98eea1f73

What I'm actually less clear about is whether ksz9477 actually needs this.
Luckily U-Boot should have less compatibility issues to worry about,
since the DT is appended to the bootloader image, so some corrections
can be made if necessary.

Re: [PATCH net] net: dsa: microchip: keep compatibility with device tree blobs with no phy-mode

[Tim Harvey]

On Fri, Aug 19, 2022 at 9:42 AM Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
>
> On Fri, Aug 19, 2022 at 01:47:51PM +0200, Rasmus Villemoes wrote:
> > > To give you an idea of how things work for user ports. If a user port
> > > has a phy-handle, DSA will connect to that, irrespective of what OF-based
> > > MDIO bus that is on. If not, DSA looks at whether ds->slave_mii_bus is
> > > populated with a struct mii_bus by the driver. If it is, it connects in
> > > a non-OF based way to a PHY address equal to the port number. If
> > > ds->slave_mii_bus doesn't exist but the driver provides
> > > ds->ops->phy_read and ds->ops->phy_write, DSA automatically creates
> > > ds->slave_mii_bus where its ops are the driver provided phy_read and
> > > phy_write, and it then does the same thing of connecting to the PHY in
> > > that non-OF based way.
> >
> > Thanks, that's quite useful. From quick grepping, it seems that ksz9567
> > currently falls into the latter category?
>
> So it would appear.
>
> > No, but it's actually easier for me to just do that rather than carry an
> > extra patch until the mainline fix hits 5.19.y.
>
> Whatever suits you best.
>
> > > there is something to be said about U-Boot compatibility. In U-Boot,
> > > with DM_DSA, I don't intend to support any unnecessary complexity and
> > > alternative ways of describing the same thing, so there, phy-mode and
> > > one of phy-handle or fixed-link are mandatory for all ports.
> >
> > OK. I suppose that means the linux driver for the ksz9477 family  should
> > learn to check if there's an "mdio" subnode and if so populate
> > ds->slave_mii_bus, but unlike lan937x, absence of that node should not
> > be fatal?
>
> Yes.
>
> > > U-Boot can pass its own device tree to Linux, it means Linux DSA drivers
> > > might need to gradually gain support for OF-based phy-handle on user
> > > ports as well. So see what Tim Harvey has done in drivers/net/ksz9477.c
> > > in the U-Boot source code, and try to work with his device tree format,
> > > as a starting point.
> >
> > Hm. It does seem like that driver has the mdio bus optional (as in,
> > probe doesn't fail just because the subnode isn't present). But I'm
> > curious why ksz_probe_mdio() looks for a subnode called "mdios" rather
> > than "mdio". Tim, just a typo?
>
> No, definitely not a typo. I have to explain this for what seems like
> the millionth time already, but the idea with an "mdios" container was
> copied from the sja1105 driver, where there are actually multiple MDIO
> buses. Documentation/devicetree/bindings/net/mdio.yaml wants the $nodename
> to have the "^mdio(@.*)?" pattern, and:
> - if you use "mdio" you can have a single node
> - if you don't put the MDIO nodes under a container with an
>   #address-cells != 0, you can't use the "mdio@something" syntax
>
> https://lore.kernel.org/all/20210621234930.espjau5l2t5dr75y@skbuf/T/#me43cf6b1976a3c3aec8357f19ab967f98eea1f73
>
> What I'm actually less clear about is whether ksz9477 actually needs this.
> Luckily U-Boot should have less compatibility issues to worry about,
> since the DT is appended to the bootloader image, so some corrections
> can be made if necessary.

Vladimir,

The linux ksz9477 driver does not need the 'mdios' subnode or
phy-handle's to function properly. I added these nodes to the U-Boot
dts for imx8mm-venice-gw7901.dts and imx8mp-venice-gw74xx.dts in order
for the U-Boot ksz9477 driver to work properly. So now I have
out-of-sync dts files for those that will cause an issue the next time
someone sync's dts from Linux back to U-Boot:

diff --git a/arch/arm64/boot/dts/freescale/imx8mp-venice-gw74xx.dts b/arch/arm64
/boot/dts/freescale/imx8mp-venice-gw74xx.dts
index 8469d5ddf960..f0bd67f12bad 100644
--- a/arch/arm64/boot/dts/freescale/imx8mp-venice-gw74xx.dts
+++ b/arch/arm64/boot/dts/freescale/imx8mp-venice-gw74xx.dts
@@ -500,6 +500,7 @@ ports {
                        lan1: port@0 {
                                reg = <0>;
                                label = "lan1";
+                               phy-handle = <&sw_phy0>;
                                phy-mode = "internal";
                                local-mac-address = [00 00 00 00 00 00];
                        };
@@ -507,6 +508,7 @@ lan1: port@0 {
                        lan2: port@1 {
                                reg = <1>;
                                label = "lan2";
+                               phy-handle = <&sw_phy1>;
                                phy-mode = "internal";
                                local-mac-address = [00 00 00 00 00 00];
                        };
@@ -514,6 +516,7 @@ lan2: port@1 {
                        lan3: port@2 {
                                reg = <2>;
                                label = "lan3";
+                               phy-handle = <&sw_phy2>;
                                phy-mode = "internal";
                                local-mac-address = [00 00 00 00 00 00];
                        };
@@ -521,6 +524,7 @@ lan3: port@2 {
                        lan4: port@3 {
                                reg = <3>;
                                label = "lan4";
+                               phy-handle = <&sw_phy3>;
                                phy-mode = "internal";
                                local-mac-address = [00 00 00 00 00 00];
                        };
@@ -528,6 +532,7 @@ lan4: port@3 {
                        lan5: port@4 {
                                reg = <4>;
                                label = "lan5";
+                               phy-handle = <&sw_phy4>;
                                phy-mode = "internal";
                                local-mac-address = [00 00 00 00 00 00];
                        };
@@ -544,6 +549,38 @@ fixed-link {
                                };
                        };
                };
+
+               mdios {
+                       #address-cells = <1>;
+                       #size-cells = <0>;
+
+                       mdio@0 {
+                               reg = <0>;
+                               compatible = "microchip,ksz-mdio";
+                               #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>;
+                               };
+
+                               sw_phy4: ethernet-phy@4 {
+                                       reg = <0x4>;
+                               };
+                       };
+               };
        };
 };

I believe you are still of the opinion that support for parsing the
mdios node above be added to the Linux DSA drivers and dt-bindings or
were you suggesting just the bindings be added? I don't think bindings
would get ack'd unless there was code actually using them for
something and its not clear to me exactly what you were asking for.

Best Regards,

Tim

Re: [PATCH net] net: dsa: microchip: keep compatibility with device tree blobs with no phy-mode

[Jakub Kicinski]

On Thu, 18 Aug 2022 17:32:50 +0300 Vladimir Oltean wrote:
> DSA has multiple ways of specifying a MAC connection to an internal PHY.
> One requires a DT description like this:

Too much side-conversations going on here for me to grasp, this is good
to go in, correct?

Re: [PATCH net] net: dsa: microchip: keep compatibility with device tree blobs with no phy-mode

[Vladimir Oltean]

On Fri, Aug 19, 2022 at 04:32:11PM -0700, Jakub Kicinski wrote:
> On Thu, 18 Aug 2022 17:32:50 +0300 Vladimir Oltean wrote:
> > DSA has multiple ways of specifying a MAC connection to an internal PHY.
> > One requires a DT description like this:
> 
> Too much side-conversations going on here for me to grasp, this is good
> to go in, correct?

Admittedly I'm not an impartial party, but yes please.

Re: [PATCH net] net: dsa: microchip: keep compatibility with device tree blobs with no phy-mode

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (master)
by Jakub Kicinski <kuba@kernel.org>:

On Thu, 18 Aug 2022 17:32:50 +0300 you wrote:
> DSA has multiple ways of specifying a MAC connection to an internal PHY.
> One requires a DT description like this:
> 
> 	port@0 {
> 		reg = <0>;
> 		phy-handle = <&internal_phy>;
> 		phy-mode = "internal";
> 	};
> 
> [...]

Here is the summary with links:
  - [net] net: dsa: microchip: keep compatibility with device tree blobs with no phy-mode
    https://git.kernel.org/netdev/net/c/5fbb08eb7f94

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html