Re: [PATCH net-next 4/9] net: dsa: microchip: remove setup_rgmii_delay() KSZ operation

[Bastien Curutchet]

Hi Paolo,

On 6/1/26 10:49 AM, Paolo Abeni wrote:
> On 5/26/26 11:47 AM, Bastien Curutchet (Schneider Electric) wrote:
> > setup_rgmii_delay() operation is only used once during the common phylink
> > MAC configuration. Only the lan937x switch implements this
> > setup_rgmii_delay().
> >
> > Remove the setup_rgmii_delay operation from ksz_dev_ops.
> > Implement a lan937x-specific phylink MAC configuration that does this
> > RGMII delay setup.
> > Export ksz_set_xmii since it's needed by the lan937x implementation.
> >
> > Signed-off-by: Bastien Curutchet (Schneider Electric) <bastien.curutchet@xxxxxxxxxxx>
> > ---
> >   drivers/net/dsa/microchip/ksz_common.c   |  6 +-----
> >   drivers/net/dsa/microchip/ksz_common.h   |  2 +-
> >   drivers/net/dsa/microchip/lan937x_main.c | 29 +++++++++++++++++++++++++++--
> >   3 files changed, 29 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
> > index 5fb151fbf4f3..d6511fcebcf2 100644
> > --- a/drivers/net/dsa/microchip/ksz_common.c
> > +++ b/drivers/net/dsa/microchip/ksz_common.c
> > @@ -3127,8 +3127,7 @@ int ksz_set_mac_eee(struct dsa_switch *ds, int port,
> >   	return 0;
> >   }
> >   
> > -static void ksz_set_xmii(struct ksz_device *dev, int port,
> > -			 phy_interface_t interface)
> > +void ksz_set_xmii(struct ksz_device *dev, int port, phy_interface_t interface)
> >   {
> >   	const u8 *bitval = dev->info->xmii_ctrl1;
> >   	struct ksz_port *p = &dev->ports[port];
> > @@ -3233,9 +3232,6 @@ void ksz_phylink_mac_config(struct phylink_config *config,
> >   	}
> >   
> >   	ksz_set_xmii(dev, port, state->interface);
> > -
> > -	if (dev->dev_ops->setup_rgmii_delay)
> > -		dev->dev_ops->setup_rgmii_delay(dev, port);
> >   }
> >   
> >   bool ksz_get_gbit(struct ksz_device *dev, int port)
> > diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
> > index 804912fdd2db..ab08c98ed257 100644
> > --- a/drivers/net/dsa/microchip/ksz_common.h
> > +++ b/drivers/net/dsa/microchip/ksz_common.h
> > @@ -410,7 +410,6 @@ struct ksz_dev_ops {
> >   			   u8 data);
> >   	void (*freeze_mib)(struct ksz_device *dev, int port, bool freeze);
> >   	void (*port_init_cnt)(struct ksz_device *dev, int port);
> > -	void (*setup_rgmii_delay)(struct ksz_device *dev, int port);
> >   	int (*tc_cbs_set_cinc)(struct ksz_device *dev, int port, u32 val);
> >   	int (*init)(struct ksz_device *dev);
> >   };
> > @@ -468,6 +467,7 @@ void ksz_phylink_get_caps(struct dsa_switch *ds, int port,
> >   void ksz_phylink_mac_disable_tx_lpi(struct phylink_config *config);
> >   int ksz_phylink_mac_enable_tx_lpi(struct phylink_config *config,
> >   				  u32 timer, bool tx_clock_stop);
> > +void ksz_set_xmii(struct ksz_device *dev, int port, phy_interface_t interface);
> >   void ksz_phylink_mac_config(struct phylink_config *config,
> >   			    unsigned int mode,
> >   			    const struct phylink_link_state *state);
> > diff --git a/drivers/net/dsa/microchip/lan937x_main.c b/drivers/net/dsa/microchip/lan937x_main.c
> > index 57e289637193..f8f220242f7a 100644
> > --- a/drivers/net/dsa/microchip/lan937x_main.c
> > +++ b/drivers/net/dsa/microchip/lan937x_main.c
> > @@ -643,6 +643,32 @@ static void lan937x_setup_rgmii_delay(struct ksz_device *dev, int port)
> >   	}
> >   }
> >   
> > +static void lan937x_phylink_mac_config(struct phylink_config *config,
> > +				       unsigned int mode,
> > +				       const struct phylink_link_state *state)
> > +{
> > +	struct dsa_port *dp = dsa_phylink_to_port(config);
> > +	struct ksz_device *dev = dp->ds->priv;
> > +	int port = dp->index;
> > +
> > +	/* Internal PHYs */
> > +	if (dev->info->internal_phy[port])
> > +		return;
> > +
> > +	/* No need to configure XMII control register when using SGMII. */
> > +	if (ksz_is_sgmii_port(dev, port))
> > +		return;
> > +
> > +	if (phylink_autoneg_inband(mode)) {
> > +		dev_err(dev->dev, "In-band AN not supported!\n");
> > +		return;
> > +	}
> > +
> > +	ksz_set_xmii(dev, port, state->interface);
> > +
> > +	lan937x_setup_rgmii_delay(dev, port);
>
> I think you can avoid some code duplication with something alike the
> following:
>
> - factor out of ksz_phylink_mac_config() a  __ksz_phylink_mac_config(),
> helper, with the latter doing all the work and returning true when
> actually calling ksz_set_xmii()
>
> - implement lan937x_phylink_mac_config() on top of
> __ksz_phylink_mac_config():
>
> 	if (__ksz_phylink_mac_config())
> 		lan937x_setup_rgmii_delay(dev, port);

Indeed, I'll do it in next iteration, thank you.


Best regards,
Bastien