RE: [PATCH v4 2/5] net: macb: add support for sgmii MAC-PHY interface

[Parshuram Raju Thombare]

>> >I still don't think this makes much sense, splitting the interface
>> > configuration between here and below.
>> Do you mean splitting mac_config in two *_configure functions ?
>> This was done as per Andrew's suggestion to make code mode readable
>> and easy to manage by splitting MAC configuration for different interfaces.
>No, I mean here you disable SGMII if we're switching away from SGMII
>mode.... (note, this means there is more to come for this sentence)
Sorry, I misunderstood your original question. I think disabling old interface
and enabling new one can be done in single place. I will do this change.

>> >This will only be executed when we are not using inband mode, which
>> >basically means it's not possible to switch to SGMII in-band mode.
>> SGMII is used in default PHY mode. And above code is to program MAC to
>> select PCS and SGMII interface.
>... and here you enable it for SGMII mode, but only for non-inband
>modes.
>
>Why not:
>	if (change_interface)  {
>		if (state->interface == PHY_INTERFACE_MODE_SGMII) {
>			// Enable SGMII mode and PCS
>			gem_writel(bp, NCFGR, ncfgr | GEM_BIT(SGMIIEN) |
>				   GEM_BIT(PCSSEL));
>		} else {
>			// Disable SGMII mode and PCS
>			gem_writel(bp, NCFGR, ncfgr & ~(GEM_BIT(SGMIIEN)
>				   GEM_BIT(PCSSEL)));
>			// Reset PCS
>			gem_writel(bp, PCS_CTRL, gem_readl(bp, PCS_CTRL)
>				   GEM_BIT(PCS_CTRL_RST));
>		}
>	}
>	if (!phylink_autoneg_inband(mode) &&
>	    (bp->speed != state->speed || bp->duplex != state->duplex)) {
>?
Ok

>> >> +
>> >> +		if (!interface_supported) {
>> >> +			netdev_err(dev, "Phy mode %s not supported",
>> >> +				   phy_modes(phy_mode));
>> >> +			goto err_out_free_netdev;
>> >> +		}
>> >> +
>> >>  		bp->phy_interface = phy_mode;
>> >> +	} else {
>> >> +		bp->phy_interface = phy_mode;
>> >> +	}
>> >If bp->phy_interface is PHY_INTERFACE_MODE_SGMII here, and
>> > mac_config()
>> >is called with state->interface = PHY_INTERFACE_MODE_SGMII, then
>> >mac_config() won't configure the MAC for the interface type - is that
>> >intentional?
>> In mac_config configure MAC for non in-band mode, there is also check for
>> speed, duplex
>> changes. bp->speed and bp->duplex are initialized to SPEED_UNKNOWN
>> and DUPLEX_UNKNOWN
>> values so it is expected that for non in band mode state contains valid speed
>> and duplex mode
>> which are different from *_UNKNOWN values.

>Sorry, this reply doesn't answer my question.  I'm not asking about
>bp->speed and bp->duplex.  I'm asking:
>1) why you are initialising bp->phy_interface here
>2) you to consider the impact that has on the mac_config() implementation
>  you are proposing
> because I think it's buggy.
bp->phy_interface is to store phy mode value from device tree. This is used later 
to know what phy interface user has selected for PHY-MAC. Same is used
to configure MAC correctly and based on your suggestion code is
added to handle PHY dynamically changing phy interface, in which 
case bp->phy_interface is also updated. Though it may not be what user want, 
if phy interface is totally decided by PHY and is anyway going to be different from what user
has selected in DT, initializing it here doesn't make sense.
But in case of PHY not changing phy_interface dynamically bp->phy_interface need to be
initialized with value from DT.

Regards,
Parshuram Thombare