Re: [PATCH 1/3] net: ethernet: add support for PCS and 2.5G speed

[<Nicolas.Ferre@microchip.com>]

$subject should begin with "net: macb: "

Parshuram,

Sorry but NACK on the series.

David,

This patch series seem pretty intrusive, so I would like that you wait 
for my explicit ACK before applying even next versions of it.

More comments below...


On 25/02/2019 at 18:21, Florian Fainelli wrote:
> On 2/25/19 1:11 AM, Parshuram Raju Thombare wrote:
>>> Le 2/22/19 à 12:12 PM, Parshuram Thombare a écrit :
>>>> This patch add support for PCS (for SGMII interface) and 2.5Gbps MAC
>>>> in Cadence ethernet controller driver.
>>>
>>> At a high level you don't seem to be making use of PHYLINK so which 2.5Gbps
>>> interfaces do you actually support?
>>>
>>
>> New ethernet controller have MAC which support 2.5G speed.
>> Also there is addition of PCS and SGMII interface.
> 
> I should have asked this more clearly: have you tested with SFP modules
> for instance? If you want to be able to reliably support 2500baseT
> and/or 2500baseX with hot plugging of such modules, you need to
> implement PHYLINK for that network driver, there is no other way around.
> 
>>
>>>>
>>>> Signed-off-by: Parshuram Thombare <pthombar@cadence.com>
>>>> ---
>>>

[..]

>>>>
>>>> -	switch (speed) {
>>>> -	case SPEED_10:
>>>> +	if (interface == PHY_INTERFACE_MODE_GMII ||
>>>> +	    interface == PHY_INTERFACE_MODE_MII) {
>>>> +		switch (speed) {
>>>> +		case SPEED_10:>  		rate = 2500000;
>>>
>>> You need to add one tab to align rate and break.
>>
>> Do you mean a tab each for rate and break lines ?
>> All switch statements are aligned at a tab.  I am not sure how does case and rate got on same line.
> 
> It should look like this:
> 
> switch (cond) {
> case cond1:
> 	do_something();
> 	break;
> 
> etc.

Read the coding style documentation, everything is well explained there.


>>>>   		break;
>>>> -	case SPEED_100:
>>>> +		case SPEED_100:
>>>>   		rate = 25000000;
>>>>   		break;
>>>> -	case SPEED_1000:
>>>> +		case SPEED_1000:
>>>>   		rate = 125000000;
>>>>   		break;
>>>> -	default:
>>>> +		default:
>>>> +		return;
>>>> +		}
>>>> +	} else if (interface == PHY_INTERFACE_MODE_SGMII) {
>>>> +		switch (speed) {
>>>> +		case SPEED_10:
>>>> +		rate = 1250000;
>>>> +		break;
>>>> +		case SPEED_100:
>>>> +		rate = 12500000;
>>>> +		break;
>>>> +		case SPEED_1000:
>>>> +		rate = 125000000;
>>>> +		break;
>>>> +		case SPEED_2500:
>>>> +		rate = 312500000;
>>>> +		break;
>>>> +		default:
>>>> +		return;
>>>
>>> The indentation is broken here and you can greatly simplify this with a simple
>>> function that returns speed * 1250 and does an initial check for unsupported
>>> speeds.
>>>
>>
>> I ran checkpatch.pl and all indentation issues were cleared. But I think having function
>> is better option, I will make that change.
>>
>>>> +		}
>>>> +	} else {
>>>>   		return;
>>>>   	}

[..]

>>>> int macb_mii_probe(struct net_device *dev)
>>>>   	}
>>>>
>>>>   	/* mask with MAC supported features */
>>>> -	if (macb_is_gem(bp) && bp->caps &
>>> MACB_CAPS_GIGABIT_MODE_AVAILABLE)
>>>> -		phy_set_max_speed(phydev, SPEED_1000);
>>>> -	else
>>>> -		phy_set_max_speed(phydev, SPEED_100);
>>>> +	if (macb_is_gem(bp)) {
>>>
>>> You have changed the previous logic that also checked for
>>> MACB_CAPS_GIGABIT_MODE_AVAILABLE, why?
>>
>> My understanding is all GEM (ID >= 0x2) support GIGABIT mode.
>> Was there any other reason for this check ?

We use (ID >= 0x2) and only 10/100 mode on sama5d4/sama5d2 for more than 
5 years, as seen in their respective struct macb_config, weren't you aware?

> Well, if anyone would know, it would be you, I don't work for Cadence
> nor have access to the internal IP documentation.

I agree with Florian, and what you modify makes me feel that the rest of 
the patch might break my use of the Cadence IP in my products. That's 
not a good sign, to say the least.

I'm expecting that Cadence don't break software used by their customers 
on products already deployed on the field.
For next series, I would like that you report that you actually tested 
your changes on older IP revisions and that non-regression tests are 
passed successfully for some of the long time users of this IP (namely 
Microchip/Atmel and Xilinx products).



>>>> +		linkmode_copy(phydev->supported, PHY_GBIT_FEATURES);
>>>> +		if (bp->caps & MACB_CAPS_TWO_PT_FIVE_GIG_SPEED)
>>>> +
>>> 	linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT,
>>>> +					 phydev->supported);
>>>> +	} else {
>>>> +		linkmode_copy(phydev->supported, PHY_BASIC_FEATURES);
>>>> +	}
>>>> +
>>>> +	linkmode_copy(phydev->advertising, phydev->supported);
>>>>
>>>>   	if (bp->caps & MACB_CAPS_NO_GIGABIT_HALF)
>>>>   		phy_remove_link_mode(phydev,
>>>> @@ -2217,8 +2267,6 @@ 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)
>>>> -		config |= GEM_BIT(SGMIIEN) | GEM_BIT(PCSSEL);
>>>>   	config |= MACB_BF(RBOF, NET_IP_ALIGN);	/* Make eth data
>>> aligned */
>>>>   	config |= MACB_BIT(PAE);		/* PAuse Enable */
>>>>   	config |= MACB_BIT(DRFCS);		/* Discard Rx FCS */
>>>> @@ -3255,6 +3303,23 @@ static void macb_configure_caps(struct macb *bp,
>>>>   		dcfg = gem_readl(bp, DCFG1);
>>>>   		if (GEM_BFEXT(IRQCOR, dcfg) == 0)
>>>>   			bp->caps |= MACB_CAPS_ISR_CLEAR_ON_WRITE;
>>>> +		if (GEM_BFEXT(NO_PCS, dcfg) == 0)
>>>> +			bp->caps |= MACB_CAPS_PCS;
>>>> +		switch (MACB_BFEXT(IDNUM, macb_readl(bp, MID))) {
>>>> +		case MACB_GEM7016_IDNUM:
>>>> +		case MACB_GEM7017_IDNUM:
>>>> +		case MACB_GEM7017A_IDNUM:
>>>> +		case MACB_GEM7020_IDNUM:
>>>> +		case MACB_GEM7021_IDNUM:
>>>> +		case MACB_GEM7021A_IDNUM:
>>>> +		case MACB_GEM7022_IDNUM:
>>>> +		if (bp->caps & MACB_CAPS_PCS)
>>>> +			bp->caps |= MACB_CAPS_TWO_PT_FIVE_GIG_SPEED;
>>>> +		break;
>>>> +
>>>> +		default:
>>>> +		break;
>>>> +		}
>>>>   		dcfg = gem_readl(bp, DCFG2);
>>>>   		if ((dcfg & (GEM_BIT(RX_PKT_BUFF) | GEM_BIT(TX_PKT_BUFF)))
>>> == 0)
>>>>   			bp->caps |= MACB_CAPS_FIFO_MODE;
>>>> @@ -4110,7 +4175,28 @@ static int macb_probe(struct platform_device
>>> *pdev)
>>>>   		else
>>>>   			bp->phy_interface = PHY_INTERFACE_MODE_MII;
>>>>   	} else {
>>>> +		switch (err) {
>>>> +		case PHY_INTERFACE_MODE_SGMII:
>>>> +		if (bp->caps & MACB_CAPS_PCS) {
>>>> +			bp->phy_interface = PHY_INTERFACE_MODE_SGMII;
>>>> +			break;
>>>> +		}
>>>
>>> If SGMII was selected on a version of the IP that does not support it, then falling
>>> back to GMII or MII does not sound correct, this is a hard error that must be
>>> handled as such.
>>> --
>>> Florian
>>
>> My intention was to continue (instead of failing) with whatever functionality is available.
>> Can we have some error message and continue with what is available ?
> 
> This is probably not going to help anyone, imagine you incorrectly
> specified a 'phy-mode' property in the Device Tree and you have to dig
> into the driver in the function that sets the clock rate to find out
> what you just defaulted to 1Gbits/GMII for instance. This is not user
> friendly at all and will increase the support burden on your end as
> well, if SGMII is specified and the IP does not support it, detect it
> and return an error, how that propagates, either as a probe failure or
> the inability to open the network device, your call.
> 

Best regards,
-- 
Nicolas Ferre