RE: [PATCH 3/3] net: macb: add support for high speed interface

[Milind Parab]

>> This patch add support for high speed USXGMII PCS and 10G
>> speed in Cadence ethernet controller driver.
>
>How has this been tested?
>

This patch is tested in 10G fixed mode on SFP+ module. 

In our own lab, we have various hardware test platforms supporting SGMII (through a TI PHY and another build that connects to a Marvell 1G PHY), GMII (through a Marvell PHY), 10GBASE-R (direct connection to SFP+), USXGMII (currently we can emulate this using an SFP+ connection from/to our own hardware)

>> @@ -81,6 +81,18 @@ struct sifive_fu540_macb_mgmt {
>>  #define MACB_WOL_HAS_MAGIC_PACKET	(0x1 << 0)
>>  #define MACB_WOL_ENABLED		(0x1 << 1)
>>
>> +enum {
>> +	HS_MAC_SPEED_100M,
>> +	HS_MAC_SPEED_1000M,
>> +	HS_MAC_SPEED_2500M,
>> +	HS_MAC_SPEED_5000M,
>> +	HS_MAC_SPEED_10000M,
>
>Are these chip register definitions?  Shouldn't you be relying on fixed
>values for these, rather than their position in an enumerated list?
>
Okay, yes it is safe to use fixed values instead of enum. I will change this.

>
>> +	gem_writel(bp, USX_CONTROL, config | GEM_BIT(TX_EN));
>> +	config = gem_readl(bp, USX_CONTROL);
>> +	gem_writel(bp, USX_CONTROL, config | GEM_BIT(SIGNAL_OK));
>> +	ret = readx_poll_timeout(GEM_READ_USX_STATUS, bp, val,
>> +				 val & GEM_BIT(USX_BLOCK_LOCK),
>> +				 1, MACB_USX_BLOCK_LOCK_TIMEOUT);
>
>What if there's no signal to lock on to?  That's treated as link down
>and is not a failure.
>
Yes, if there is no signal to lock on, that is treated as link down.
Is this okay or should there be some additional error handling needed?

>> +
>> +	switch (state->speed) {
>> +	case SPEED_10000:
>> +		speed = HS_MAC_SPEED_10000M;
>> +		break;
>> +	case SPEED_5000:
>> +		speed = HS_MAC_SPEED_5000M;
>> +		break;
>> +	case SPEED_2500:
>> +		speed = HS_MAC_SPEED_2500M;
>> +		break;
>> +	case SPEED_1000:
>> +		speed = HS_MAC_SPEED_1000M;
>> +		break;
>> +	default:
>> +	case SPEED_100:
>> +		speed = HS_MAC_SPEED_100M;
>> +		break;
>> +	}

>So you only support fixed-mode (phy and fixed links) and not in-band
>links here.
>
Yes, this is right.