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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 10 Dec 2019 11:20:38 +0000
From:   Milind Parab <mparab@...ence.com>
To:     Russell King - ARM Linux admin <linux@...linux.org.uk>
CC:     "nicolas.nerre@...rochip.com" <nicolas.nerre@...rochip.com>,
        "andrew@...n.ch" <andrew@...n.ch>,
        "antoine.tenart@...tlin.com" <antoine.tenart@...tlin.com>,
        "f.fainelli@...il.com" <f.fainelli@...il.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "hkallweit1@...il.com" <hkallweit1@...il.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Dhananjay Vilasrao Kangude <dkangude@...ence.com>,
        "a.fatoum@...gutronix.de" <a.fatoum@...gutronix.de>,
        "brad.mouring@...com" <brad.mouring@...com>,
        Parshuram Raju Thombare <pthombar@...ence.com>
Subject: RE: [PATCH 3/3] net: macb: add support for high speed interface

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