Re: [PATCH net-next v2 1/3] net: ethernet: ti: am65-cpsw: Move mode specific config to mac_config()

[Russell King (Oracle)]

On Mon, Apr 03, 2023 at 07:20:21PM +0530, Siddharth Vadapalli wrote:
> 
> 
> On 03-04-2023 18:43, Russell King (Oracle) wrote:
> > On Mon, Apr 03, 2023 at 06:31:52PM +0530, Siddharth Vadapalli wrote:
> >>
> >>
> >> On 03-04-2023 16:38, Russell King (Oracle) wrote:
> >>> On Mon, Apr 03, 2023 at 04:31:04PM +0530, Siddharth Vadapalli wrote:
> >>>> Move the interface mode specific configuration to the mac_config()
> >>>> callback am65_cpsw_nuss_mac_config().
> >>>>
> >>>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@xxxxxx>
> >>>> ---
> >>>>  drivers/net/ethernet/ti/am65-cpsw-nuss.c | 10 +++++++---
> >>>>  1 file changed, 7 insertions(+), 3 deletions(-)
> >>>>
> >>>> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> >>>> index d17757ecbf42..74e099828978 100644
> >>>> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> >>>> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> >>>> @@ -1504,12 +1504,17 @@ static void am65_cpsw_nuss_mac_config(struct phylink_config *config, unsigned in
> >>>>  							  phylink_config);
> >>>>  	struct am65_cpsw_port *port = container_of(slave, struct am65_cpsw_port, slave);
> >>>>  	struct am65_cpsw_common *common = port->common;
> >>>> +	u32 mac_control = 0;
> >>>>  
> >>>>  	if (common->pdata.extra_modes & BIT(state->interface)) {
> >>>> -		if (state->interface == PHY_INTERFACE_MODE_SGMII)
> >>>> +		if (state->interface == PHY_INTERFACE_MODE_SGMII) {
> >>>> +			mac_control |= CPSW_SL_CTL_EXT_EN;
> >>>>  			writel(ADVERTISE_SGMII,
> >>>>  			       port->sgmii_base + AM65_CPSW_SGMII_MR_ADV_ABILITY_REG);
> >>>> +		}
> >>>>  
> >>>> +		if (mac_control)
> >>>> +			cpsw_sl_ctl_set(port->slave.mac_sl, mac_control);
> >>>>  		writel(AM65_CPSW_SGMII_CONTROL_MR_AN_ENABLE,
> >>>>  		       port->sgmii_base + AM65_CPSW_SGMII_CONTROL_REG);
> >>>>  	}
> >>>> @@ -1553,8 +1558,7 @@ static void am65_cpsw_nuss_mac_link_up(struct phylink_config *config, struct phy
> >>>>  
> >>>>  	if (speed == SPEED_1000)
> >>>>  		mac_control |= CPSW_SL_CTL_GIG;
> >>>> -	if (interface == PHY_INTERFACE_MODE_SGMII)
> >>>> -		mac_control |= CPSW_SL_CTL_EXT_EN;
> >>>> +	/* TODO: Verify whether in-band is necessary for 10 Mbps RGMII */
> >>>>  	if (speed == SPEED_10 && phy_interface_mode_is_rgmii(interface))
> >>>>  		/* Can be used with in band mode only */
> >>>>  		mac_control |= CPSW_SL_CTL_EXT_EN;
> >>>
> >>> I'm afraid I can see you haven't thought this patch through properly.
> >>>
> >>> am65_cpsw_nuss_mac_link_down() will call
> >>> cpsw_sl_ctl_reset(port->slave.mac_sl); which has the effect of clearing
> >>> to zero the entire MAC control register. This will clear
> >>> CPSW_SL_CTL_EXT_EN that was set in am65_cpsw_nuss_mac_config() which is
> >>> not what you want to be doing.
> >>>
> >>> Given that we have the 10Mbps issue with RGMII, I think what you want
> >>> to be doing is:
> >>>
> >>> 1. Set CPSW_SL_CTL_EXT_EN in am65_cpsw_nuss_mac_config() if in SGMII
> >>>    mode, otherwise clear this bit.
> >>>
> >>> 2. Clear the mac_control register in am65_cpsw_nuss_mac_link_down()
> >>>    if in RMGII mode, otherwise preserve the state of
> >>>    CPSW_SL_CTL_EXT_EN but clear all other bits.
> >>>
> >>> 3. Set CPSW_SL_CTL_EXT_EN in am65_cpsw_nuss_mac_link_up() if in
> >>>    RGMII mode and 10Mbps.
> >>
> >> I plan to implement it as follows:
> >> 1. Add a member "u32 mode_config" to "struct am65_cpsw_slave_data" in
> >> "am65-cpsw-nuss.h".
> >> 2. In am65_cpsw_nuss_mac_config(), store the value of mac_control in
> >> "port->slave.mode_config".
> >> 3. In am65_cpsw_nuss_mac_link_down(), after the reset via
> >> cpsw_sl_ctl_reset(), execute:
> >> cpsw_sl_ctl_set(port->slave.mac_sl, port->slave.mode_config) in order to
> >> restore the configuration performed in am65_cpsw_nuss_mac_config().
> >>
> >> Please let me know in case of any suggestions to implement it in a
> >> better manner.
> > 
> > Do you think this complexity is really worth it?
> > 
> > Let's look at what's available:
> > 
> > cpsw_sl_ctl_set() - sets bits in the mac control register
> > cpsw_sl_ctl_clr() - clears bits in the mac control register
> > cpsw_sl_ctl_reset() - sets the mac control register to zero
> > 
> > So, in mac_config(), we can do:
> > 
> > 	if (interface == SGMII)
> > 		cpsw_sl_ctl_set(CPSW_SL_CTL_EXT_EN);
> > 	else
> > 		cpsw_sl_ctl_clr(CPSW_SL_CTL_EXT_EN);
> 
> While this will work for patch 1/3, once I add support for USXGMII mode
> as in patch 3/3, I believe that I have to invert it, beginning by
> invoking a cpsw_sl_ctl_clr(CPSW_SL_CTL_EXT_EN) at the start in
> mac_config() followed by switching through the modes. If the mode is
> SGMII, then I invoke cpsw_sl_ctl_set(CPSW_SL_CTL_EXT_EN), along with the
> write to the MR_ADV_ABILITY_REG register to advertise SGMII. If the mode
> is USXGMII, then I invoke:
> cpsw_sl_ctl_set(CPSW_SL_CTL_XGIG | CPSW_SL_CTL_XGMII_EN)

For patch 1, I did leave out the write for MR_ADV_ABILITY_REG, I had
assumed you'd get the idea on that and merge the if() condition you
already had with my suggestion above (which isn't literal code!)

In patch 3, you simply need to add:

	if (interface == USXGMII)
		cpsw_sl_ctl_set(CPSW_SL_CTL_XGIG | CPSW_SL_CTL_XGMII_EN);
	else
		cpsw_sl_ctl_clr(CPSW_SL_CTL_XGIG | CPSW_SL_CTL_XGMII_EN);

Thanks.

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