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

[Parshuram Raju Thombare <pthombar@cadence.com>]

>From: Russell King - ARM Linux admin <linux@armlinux.org.uk>
>
>On Wed, Jun 19, 2019 at 09:40:46AM +0100, Parshuram Thombare wrote:
>
>> This patch add support for SGMII interface) and
>
>> 2.5Gbps MAC in Cadence ethernet controller driver.

>>  	switch (state->interface) {
>
>> +	case PHY_INTERFACE_MODE_SGMII:
>
>> +		if (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE)
>
>> +			phylink_set(mask, 2500baseT_Full);
>
>
>
>This doesn't look correct to me.  SGMII as defined by Cisco only
>supports 1G, 100M and 10M speeds, not 2.5G.

Cadence MAC support 2.5G SGMII by using higher clock frequency.
Even 
>
>Even so, SGMII is not limited to just base-T - PHYs are free to offer
>base-X to SGMII conversion too.

Ok, I will make change to allow 1000BASE-X and 2500BASE-X
In SGMII mode.

>> +	case PHY_INTERFACE_MODE_2500BASEX:
>
>> +		if (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE)
>
>> +			phylink_set(mask, 2500baseX_Full);
>
>> +	/* fallthrough */
>
>> +	case PHY_INTERFACE_MODE_1000BASEX:
>
>> +		if (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE)
>
>> +			phylink_set(mask, 1000baseX_Full);
>
>> +		break;
>
>
>
>Please see how other drivers which use phylink deal with the validate()
>format, and please read the phylink documentation:
>
> * Note that the PHY may be able to transform from one connection
> * technology to another, so, eg, don't clear 1000BaseX just
> * because the MAC is unable to BaseX mode. This is more about
> * clearing unsupported speeds and duplex settings.
>

There are some configs used in this driver which limits MAC speed.
Above checks just to make sure this use case does not break.


>> +		state->duplex = MACB_BFEXT(DUPLEX, macb_readl(bp,
>NSR));
>> +		state->link = MACB_BFEXT(NSR_LINK, macb_readl(bp, NSR));
>> +	} else if (bp->phy_interface == PHY_INTERFACE_MODE_2500BASEX) {
>> +		state->speed = SPEED_2500;
>> +		state->duplex = MACB_BFEXT(DUPLEX, macb_readl(bp,
>NSR));
>> +		state->link = MACB_BFEXT(NSR_LINK, macb_readl(bp, NSR));
>> +	} else if (bp->phy_interface == PHY_INTERFACE_MODE_1000BASEX) {
>> +		state->speed = SPEED_1000;
>> +		state->duplex = MACB_BFEXT(DUPLEX, macb_readl(bp,
>NSR));
>> +		state->link = MACB_BFEXT(NSR_LINK, macb_readl(bp, NSR));
>> +	}
>
>
>
>So if the phy_interface type is not one listed, we leave state alone?
>That doesn't seem good.  It looks like you should at least simply set
>state->duplex and state->link according to the NSR register content,
>and always derive the speed.

Ok, I will make that change

>
>It would also be good to set state->lp_advertising if you have access
>to that so ethtool can report the link partner's abilities.  Current
>Marvell drivers that use phylink don't do that because that information
>is not available from the hardware.
>
Link partner ability information is available only for SGMII, I will add 
change to populate state->lp_ advertising for SGMII.

>> +	if (bp->phy_interface == PHY_INTERFACE_MODE_SGMII ||
>> +	    bp->phy_interface == PHY_INTERFACE_MODE_1000BASEX ||
>> +	    bp->phy_interface == PHY_INTERFACE_MODE_2500BASEX) {
>> +		gem_writel(bp, PCS_CTRL, gem_readl(bp, PCS_CTRL) |
>> +			   GEM_BIT(PCS_CTRL_RESTART_AN));
>> +	}
>This will only be called for 802.3z link modes, so you don't need these
>checks.

Ok, I will remove these checks.

>> @@ -506,18 +563,26 @@ static void gem_mac_config(struct phylink_config
>*pl_config, unsigned int mode,
>>  		switch (state->speed) {
>> +		case SPEED_2500:
>> +			gem_writel(bp, NCFGR, GEM_BIT(GBE) |
>> +				   gem_readl(bp, NCFGR));
>>  		}
>> -		macb_or_gem_writel(bp, NCFGR, reg);
>>
>>  		bp->speed = state->speed;
>>  		bp->duplex = state->duplex;
>
>
>
>This is not going to work for 802.3z nor SGMII properly when in-band
>negotiation is used.  We don't know ahead of time what the speed and
>duplex will be.  Please see existing drivers for examples showing
>how mac_config() should be implemented (there's good reason why its
>laid out as it is in those drivers.)
>
Ok, Here I will configure MAC only for FIXED and PHY mode.

>
>> @@ -555,6 +620,7 @@ static void gem_mac_link_down(struct
>phylink_config *pl_config,
>
>>  static const struct phylink_mac_ops gem_phylink_ops = {
>
>>  	.validate = gem_phylink_validate,
>
>>  	.mac_link_state = gem_phylink_mac_link_state,
>
>> +	.mac_an_restart = gem_mac_an_restart,
>
>>  	.mac_config = gem_mac_config,
>
>>  	.mac_link_up = gem_mac_link_up,
>
>>  	.mac_link_down = gem_mac_link_down,
>
>> @@ -2248,7 +2314,9 @@ static void macb_init_hw(struct macb *bp)
>
>>  	macb_set_hwaddr(bp);
>
>>
>
>>  	config = macb_mdc_clk_div(bp);
>
>> -	if (bp->phy_interface == PHY_INTERFACE_MODE_SGMII)
>
>> +	if (bp->phy_interface == PHY_INTERFACE_MODE_SGMII ||
>
>> +	    bp->phy_interface == PHY_INTERFACE_MODE_1000BASEX ||
>
>> +	    bp->phy_interface == PHY_INTERFACE_MODE_2500BASEX)
>
>>  		config |= GEM_BIT(SGMIIEN) | GEM_BIT(PCSSEL);
>
>
>
>Configuration of the phy interface mode should be done in mac_config()
>as previously mentioned, some PHYs can change their link mode at run
>time.  Hotplugging SFPs can change the link mode between SGMII and
>base-X too.
Ok

>> +	if (bp->phy_interface == PHY_INTERFACE_MODE_SGMII ||
>
>> +	    bp->phy_interface == PHY_INTERFACE_MODE_1000BASEX ||
>
>> +	    bp->phy_interface == PHY_INTERFACE_MODE_2500BASEX) {
>
>> +		//Enable PCS AN
>
>> +		gem_writel(bp, PCS_CTRL, gem_readl(bp, PCS_CTRL) |
>
>> +			   GEM_BIT(PCS_CTRL_EN_AN));
>
>> +		//Reset PCS block
>
>> +		gem_writel(bp, PCS_CTRL, gem_readl(bp, PCS_CTRL) |
>
>> +			   GEM_BIT(PCS_CTRL_RST));
>
>> +	}
>
>> +
>
>
>
>Should be in mac_config.
>
Ok

>
>> -	if (bp->phy_interface == PHY_INTERFACE_MODE_SGMII)
>
>> +	if (bp->phy_interface == PHY_INTERFACE_MODE_SGMII ||
>
>> +	    bp->phy_interface == PHY_INTERFACE_MODE_1000BASEX ||
>
>> +	    bp->phy_interface == PHY_INTERFACE_MODE_2500BASEX)
>
>>  		val |= GEM_BIT(SGMIIEN) | GEM_BIT(PCSSEL);
>
>
>
>Should be in mac_config.
>
Ok

Regards,
Parshuram Thombare