[net-next,v5,5/6] dsa: lan9303: Determine CPU port based on dsa_switch ptr

  In preparing to move the adjust_link logic into the phylink_mac_link_up
api, change the macro used to check for the cpu port.

Signed-off-by: Jerry Ray <jerry.ray@microchip.com>
---
 drivers/net/dsa/lan9303-core.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
  

On Fri, Dec 09, 2022 at 04:47:12PM -0600, Jerry Ray wrote:
> In preparing to move the adjust_link logic into the phylink_mac_link_up
> api, change the macro used to check for the cpu port.

No justification given for why this change is made.

As a counter argument, I could point out that DSA can support configurations
where the CPU port is one of the 100BASE-TX PHY ports, and the MII port
can be used as a regular user port where a PHY has been connected. Some
people are already doing this, for example connecting a Beaglebone Black
(can also be Raspberry Pi or what have you) over SPI to a VSC7512 switch
evaluation board.

The LAN9303 documentation makes it rather clear to me that such a
configuration would be possible, because the Switch Engine Ingress Port
Type Register allows any of the 3 switch ports to expect DSA tags. It's
lan9303_setup_tagging() the one who hardcodes the driver configuration
to be that where port 0 is the only acceptable CPU port (as well as the
early check from lan9303_setup()).

DSA's understanding of a CPU port is only that - a port which carries
DSA-tagged traffic, and is not exposed as a net_device to user space.
Nothing about MII vs internal PHY ports - that is a separate classification,
and a dsa_is_cpu_port() test is simply not something relevant from
phylink's perspective, to put it simply.

What we see in other device drivers for phylink handling is that there
is driver-level knowledge as to which ports have internal PHYs and which
have xMII pins. See priv->info->supports_mii[] in sja1105, dev->info->supports_mii[]
in the ksz drivers, mv88e6xxx_phy_is_internal() in mv88e6xxx,
felix->info->port_modes[] in the ocelot drivers, etc etc. Hardcoding
port number 0 is also better from my perspective than looking for the
CPU port. That's because the switch IP was built with the idea in mind
that port 0 is MII.

I would very much appreciate if this driver did not make any assumptions
that the internal PHY ports cannot carry DSA-tagged traffic. This
assumption was true when the driver was introduced, but it changed with
commit 0e27921816ad ("net: dsa: Use PHYLINK for the CPU/DSA ports").
Coincidentally, that is also the commit which started prompting the
lan9303 driver for an update, via the dmesg warnings you are seeing.

It looks like there is potentially more code to unlock than this simple
API change, which is something you could look at.

> 
> Signed-off-by: Jerry Ray <jerry.ray@microchip.com>
> ---
>  drivers/net/dsa/lan9303-core.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
> index 694249aa1f19..1d22e4b74308 100644
> --- a/drivers/net/dsa/lan9303-core.c
> +++ b/drivers/net/dsa/lan9303-core.c
> @@ -1064,7 +1064,11 @@ static void lan9303_adjust_link(struct dsa_switch *ds, int port,
>  	struct lan9303 *chip = ds->priv;
>  	int ctl;
>  
> -	if (!phy_is_pseudo_fixed_link(phydev))
> +	/* On this device, we are only interested in doing something here if
> +	 * this is the CPU port. All other ports are 10/100 phys using MDIO
> +	 * to control there link settings.
> +	 */
> +	if (!dsa_is_cpu_port(ds, port))
>  		return;
>  
>  	ctl = lan9303_phy_read(ds, port, MII_BMCR);
> -- 
> 2.17.1
>
  

> > In preparing to move the adjust_link logic into the phylink_mac_link_up
> > api, change the macro used to check for the cpu port.
> 
> No justification given for why this change is made.
>

The phydev parameter being passed into phylink_mac_link_up for the CPU port
was NULL, so I have to move to something else.
 
> As a counter argument, I could point out that DSA can support configurations
> where the CPU port is one of the 100BASE-TX PHY ports, and the MII port
> can be used as a regular user port where a PHY has been connected. Some
> people are already doing this, for example connecting a Beaglebone Black
> (can also be Raspberry Pi or what have you) over SPI to a VSC7512 switch
> evaluation board.
> 
> The LAN9303 documentation makes it rather clear to me that such a
> configuration would be possible, because the Switch Engine Ingress Port
> Type Register allows any of the 3 switch ports to expect DSA tags. It's
> lan9303_setup_tagging() the one who hardcodes the driver configuration
> to be that where port 0 is the only acceptable CPU port (as well as the
> early check from lan9303_setup()).
> 
> DSA's understanding of a CPU port is only that - a port which carries
> DSA-tagged traffic, and is not exposed as a net_device to user space.
> Nothing about MII vs internal PHY ports - that is a separate classification,
> and a dsa_is_cpu_port() test is simply not something relevant from
> phylink's perspective, to put it simply.
> 
> What we see in other device drivers for phylink handling is that there
> is driver-level knowledge as to which ports have internal PHYs and which
> have xMII pins. See priv->info->supports_mii[] in sja1105, dev->info->supports_mii[]
> in the ksz drivers, mv88e6xxx_phy_is_internal() in mv88e6xxx,
> felix->info->port_modes[] in the ocelot drivers, etc etc. Hardcoding
> port number 0 is also better from my perspective than looking for the
> CPU port. That's because the switch IP was built with the idea in mind
> that port 0 is MII.
> 
> I would very much appreciate if this driver did not make any assumptions
> that the internal PHY ports cannot carry DSA-tagged traffic. This
> assumption was true when the driver was introduced, but it changed with
> commit 0e27921816ad ("net: dsa: Use PHYLINK for the CPU/DSA ports").
> Coincidentally, that is also the commit which started prompting the
> lan9303 driver for an update, via the dmesg warnings you are seeing.
> 
> It looks like there is potentially more code to unlock than this simple
> API change, which is something you could look at.
>

I understand your point. The LAN9303 is very flexible, supporting an I2C
method for managing the switch and allowing the xMII to operate as either
MAC or PHY.

The original driver was written targeting the primary configuration most
widely used by our customers. The host CPU has an xMII interface and the
MDIO bus is used for control. Adding in the flexibility to support other
configurations is something I will investigate as I expand the driver to
support LAN9353/LAN9354/LAN9355 devices. Adding the
private->info->supports_mii[] as was done in the ksz drivers is the most
likely approach. I see this as a separate patch series.  Would you agree?

I will hardcode for port 0 at this point rather than looking at CPU port.

Regards,
Jerry.

> >
> > Signed-off-by: Jerry Ray <jerry.ray@microchip.com>
> > ---
> >  drivers/net/dsa/lan9303-core.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
> > index 694249aa1f19..1d22e4b74308 100644
> > --- a/drivers/net/dsa/lan9303-core.c
> > +++ b/drivers/net/dsa/lan9303-core.c
> > @@ -1064,7 +1064,11 @@ static void lan9303_adjust_link(struct dsa_switch *ds, int port,
> >       struct lan9303 *chip = ds->priv;
> >       int ctl;
> >
> > -     if (!phy_is_pseudo_fixed_link(phydev))
> > +     /* On this device, we are only interested in doing something here if
> > +      * this is the CPU port. All other ports are 10/100 phys using MDIO
> > +      * to control there link settings.
> > +      */
> > +     if (!dsa_is_cpu_port(ds, port))
> >               return;
> >
> >       ctl = lan9303_phy_read(ds, port, MII_BMCR);
> > --
> > 2.17.1
> >
>
  

On Mon, Dec 12, 2022 at 05:42:01PM +0000, Jerry.Ray@microchip.com wrote:
> > It looks like there is potentially more code to unlock than this simple
> > API change, which is something you could look at.
> 
> I understand your point. The LAN9303 is very flexible, supporting an I2C
> method for managing the switch and allowing the xMII to operate as either
> MAC or PHY.
> 
> The original driver was written targeting the primary configuration most
> widely used by our customers. The host CPU has an xMII interface and the
> MDIO bus is used for control. Adding in the flexibility to support other
> configurations is something I will investigate as I expand the driver to
> support LAN9353/LAN9354/LAN9355 devices. Adding the
> private->info->supports_mii[] as was done in the ksz drivers is the most
> likely approach. I see this as a separate patch series.  Would you agree?
> 
> I will hardcode for port 0 at this point rather than looking at CPU port.

Yes, I think that would be ok. Thanks for at least not baking in any
more assumptions in the driver that already exist.