Messages in this thread | | | From | Milind Parab <> | Subject | RE: [PATCH 3/3] net: macb: add support for high speed interface | Date | Tue, 10 Dec 2019 11:20:38 +0000 |
| |
>> 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.
| |