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

[Parshuram Raju Thombare]

>> +	if (change_interface) {
>> +		if (bp->phy_interface == PHY_INTERFACE_MODE_SGMII) {
>> +			gem_writel(bp, NCFGR, ~GEM_BIT(SGMIIEN) &
>> +				   ~GEM_BIT(PCSSEL) &
>> +				   gem_readl(bp, NCFGR));
>> +			gem_writel(bp, NCR, ~GEM_BIT(TWO_PT_FIVE_GIG) &
>> +				   gem_readl(bp, NCR));
>> +			gem_writel(bp, PCS_CTRL, gem_readl(bp, PCS_CTRL) |
>> +				   GEM_BIT(PCS_CTRL_RST));
>> +		}
>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.

>> +		bp->phy_interface = state->interface;
>> +	}
>> +
>>  	if (!phylink_autoneg_inband(mode) &&
>>  	    (bp->speed != state->speed ||
>> -	     bp->duplex != state->duplex)) {
>> +	     bp->duplex != state->duplex ||
>> +	     change_interface)) {
>>  		u32 reg;
>>
>>  		reg = macb_readl(bp, NCFGR);
>>  		reg &= ~(MACB_BIT(SPD) | MACB_BIT(FD));
>>  		if (macb_is_gem(bp))
>>  			reg &= ~GEM_BIT(GBE);
>> +		macb_or_gem_writel(bp, NCFGR, reg);
>> +
>> +		if (bp->phy_interface == PHY_INTERFACE_MODE_SGMII)
>> +			gem_writel(bp, NCFGR, GEM_BIT(SGMIIEN) |
>> +				   GEM_BIT(PCSSEL) |
>> +				   gem_readl(bp, NCFGR));
>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.

>> +
>> +		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.

Regards,
Parshuram Thombare