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

[Parshuram Raju Thombare]

>From: Russell King - ARM Linux admin <linux@xxxxxxxxxxxxxxx>
>
>On Wed, Jun 19, 2019 at 11:23:01AM +0000, Parshuram Raju Thombare wrote:
>
>> >From: Russell King - ARM Linux admin <linux@xxxxxxxxxxxxxxx>
>
>> >
>
>> >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.
>
>
>
>Ok, so why not set 2.5GBASE-X too?  Does the MAC handle auto-detecting
>
>the SGMII/BASE-X speed itself or does it need to be programmed?  If it
>
>needs to be programmed, you need additional handling in the validate
>
>callback to deal with that.

No, currently MAC can't auto detect it, it need to be programmed.
But I think programming speed/duplex mode is already done for non in-band
modes in mac_config.
For in band mode, I see two places to config MAC speed
and duplex mode, 1. mac_link_state 2. mac_link_up. In mac_link_up, though state
read from mac_link_state is passed, it is only used for printing log and updating
pl->cur_interface, so if configuring MAC speed/duplex mode in mac_link_up is correct, 
these parameters will need to read again from HW.

>> >> +	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.
>
>
>
>That's not what I'm saying.
>
>
>
>By way of example, you're offering 1000BASE-T just because the MAC
>
>connection supports it.  However, the MAC doesn't _actually_ support
>
>1000BASE-T, it supports a connection to a PHY that _happens_ to
>
>convert the MAC connection to 1000BASE-T.  It could equally well
>
>convert the MAC connection to 1000BASE-X.
>
>
>
>So, only setting 1000BASE-X when you have a PHY connection using
>
>1000BASE-X is fundamentally incorrect.
>
>
>
>For example, you could have a MAC <-> PHY link using standard 1.25Gbps
>	
>SGMII, and the PHY offers 1000BASE-T _and_ 1000BASE-X connections on
>
>a first-link-up basis.  An example of a PHY that does this are the
>
>Marvell 1G PHYs (eg, 88E151x).
>
>
>
>This point is detailed in the PHYLINK documentation, which I quoted
>
>above.
Ok, I will not clear 1000/2500BASE-T for PHY connection is just 1000/2500BASE-X
Also I will keep 1000/2500BASE-X link modes for SGMII/GMII modes.

>
>
>> >> @@ -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.
>
>
>
>As you are not the only one who has made this error, I'm considering
>
>splitting mac_config() into mac_config_fixed() and mac_config_inband()
>
>so that it's clearer what is required.  Maybe even taking separate
>
>structures so that it's impossible to access members that should not
>
>be used.
>
For in band mode, I see two places to config MAC speed
and duplex mode - 1. mac_link_state 2. mac_link_up. 
In mac_link_up, though state read from mac_link_state is passed, 
it is only used for printing log and updating pl->cur_interface, 
so if configuring MAC speed/duplex mode in mac_link_up is correct, 
these parameters will need to read again from HW.
>
>--
>
>RMK's Patch system: https://urldefense.proofpoint.com/v2/url?u=https-
>3A__www.armlinux.org.uk_developer_patches_&d=DwIBAg&c=aUq983L2pue2F
>qKFoP6PGHMJQyoJ7kl3s3GZ-_haXqY&r=GTefrem3hiBCnsjCOqAuapQHRN8-
>rKC1FRbk0it-LDs&m=qYg0cUy9RXzvJcQIwLNjHCC8tbUg_-
>k2oqUIMDpStiA&s=xUkYplnpxrywxVfsk-J5c2Z6_K96ELTBkgC5g37OXTE&e=
>
>FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
>
>According to speedtest.net: 11.9Mbps down 500kbps up

Regards,
Parshuram Thombare