Re: [RFC PATCH net-next 6/8] net: dsa: mt7530: simplify mt7530_setup_port6() and change to void

[Arınç ÜNAL]

On 16.01.2024 00:37, Vladimir Oltean wrote:
> On Sat, Jan 13, 2024 at 01:25:27PM +0300, Arınç ÜNAL wrote:
> > diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> > index 3ce4e0bb04dd..3a02308763ca 100644
> > --- a/drivers/net/dsa/mt7530.c
> > +++ b/drivers/net/dsa/mt7530.c
> > @@ -414,72 +414,56 @@ mt753x_preferred_default_local_cpu_port(struct dsa_switch *ds)
> >   }
> >   
> >   /* Setup port 6 interface mode and TRGMII TX circuit */
> > -static int
> > +static void
> >   mt7530_setup_port6(struct dsa_switch *ds, phy_interface_t interface)
> >   {
> >   	struct mt7530_priv *priv = ds->priv;
> > -	u32 ncpo1, ssc_delta, trgint, xtal;
> > +	u32 ncpo1, ssc_delta, xtal;
> >   
> >   	mt7530_clear(priv, MT7530_MHWTRAP, MHWTRAP_P6_DIS);
> >   
> > +	if (interface == PHY_INTERFACE_MODE_RGMII)
> > +		return;
>
> It would be good to add a comment here which states that the port comes
> out of reset with values good for RGMII.
>
> Also, there's a built-in assumption in this patch, that dynamically
> switching between RGMII and TRGMII is not possible. This is because
> phylink mac_config() is not necesarily called only once immediately
> after reset, but after each major_config().
>
> The fact that the driver sets both PHY_INTERFACE_MODE_RGMII and
> PHY_INTERFACE_MODE_TRGMII at once in config->supported_interfaces does
> not disprove that dynamic reconfiguration is possible. Normally,
> interfaces for which it doesn't make sense to dynamically reconfigure
> (they are wired to fixed pinout) have a single bit set in
> supported_interfaces. Is this switching something that makes any sense
> at all, given that port 6 is internal? It's not something that phylink
> knows to do today, but if this is theoretically possible and remotely
> useful, someone might end up wanting, in the future, to revert this patch.

Do you mean by internal port that the port does not have MII pinout? Port 6
of the MT7530 switch do. It is possible to have an external PHY wired to
it. So it would make sense to design mt7530_setup_port6() in the sense that
dynamic reconfiguration is possible.

I've tested to see that the core operations for TRGMII does not interfere
so no need to undo them when the interface changes from TRGMII to RGMII.

I'll do below on this patch:

	if (interface == PHY_INTERFACE_MODE_RGMII) {
		mt7530_rmw(priv, MT7530_P6ECR, P6_INTF_MODE_MASK,
			   P6_INTF_MODE(0));
		return;
	}

Arınç