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

From: Milind Parab
Date: Tue Dec 10 2019 - 06:21:16 EST


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