linux-kernel - RE: [PATCH v4 2/5] net: macb: add support for sgmii MAC-PHY 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:   Mon, 24 Jun 2019 06:35:44 +0000
From:   Parshuram Raju Thombare <pthombar@...ence.com>
To:     Russell King - ARM Linux admin <linux@...linux.org.uk>
CC:     "andrew@...n.ch" <andrew@...n.ch>,
        "nicolas.ferre@...rochip.com" <nicolas.ferre@...rochip.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "f.fainelli@...il.com" <f.fainelli@...il.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "hkallweit1@...il.com" <hkallweit1@...il.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Rafal Ciepiela <rafalc@...ence.com>,
        Anil Joy Varughese <aniljoy@...ence.com>,
        Piotr Sroka <piotrs@...ence.com>
Subject: RE: [PATCH v4 2/5] net: macb: add support for sgmii MAC-PHY interface


>> +	if (change_interface) {
>> +		if (bp->phy_interface == PHY_INTERFACE_MODE_SGMII) {
>> +			gem_writel(bp, NCFGR, ~GEM_BIT(SGMIIEN) &
>> +				   ~GEM_BIT(PCSSEL) &
>> +				   gem_readl(bp, NCFGR));
>> +			gem_writel(bp, NCR, ~GEM_BIT(TWO_PT_FIVE_GIG) &
>> +				   gem_readl(bp, NCR));
>> +			gem_writel(bp, PCS_CTRL, gem_readl(bp, PCS_CTRL) |
>> +				   GEM_BIT(PCS_CTRL_RST));
>> +		}
>I still don't think this makes much sense, splitting the interface
>configuration between here and below.
Do you mean splitting mac_config in two *_configure functions ?
This was done as per Andrew's suggestion to make code mode readable
and easy to manage by splitting MAC configuration for different interfaces.

>> +		bp->phy_interface = state->interface;
>> +	}
>> +
>>  	if (!phylink_autoneg_inband(mode) &&
>>  	    (bp->speed != state->speed ||
>> -	     bp->duplex != state->duplex)) {
>> +	     bp->duplex != state->duplex ||
>> +	     change_interface)) {
>>  		u32 reg;
>>
>>  		reg = macb_readl(bp, NCFGR);
>>  		reg &= ~(MACB_BIT(SPD) | MACB_BIT(FD));
>>  		if (macb_is_gem(bp))
>>  			reg &= ~GEM_BIT(GBE);
>> +		macb_or_gem_writel(bp, NCFGR, reg);
>> +
>> +		if (bp->phy_interface == PHY_INTERFACE_MODE_SGMII)
>> +			gem_writel(bp, NCFGR, GEM_BIT(SGMIIEN) |
>> +				   GEM_BIT(PCSSEL) |
>> +				   gem_readl(bp, NCFGR));
>This will only be executed when we are not using inband mode, which
>basically means it's not possible to switch to SGMII in-band mode.
SGMII is used in default PHY mode. And above code is to program MAC to 
select PCS and SGMII interface.

>> +
>> +		if (!interface_supported) {
>> +			netdev_err(dev, "Phy mode %s not supported",
>> +				   phy_modes(phy_mode));
>> +			goto err_out_free_netdev;
>> +		}
>> +
>>  		bp->phy_interface = phy_mode;
>> +	} else {
>> +		bp->phy_interface = phy_mode;
>> +	}
>If bp->phy_interface is PHY_INTERFACE_MODE_SGMII here, and mac_config()
>is called with state->interface = PHY_INTERFACE_MODE_SGMII, then
>mac_config() won't configure the MAC for the interface type - is that
>intentional?
In mac_config configure MAC for non in-band mode, there is also check for speed, duplex
changes. bp->speed and bp->duplex are initialized to SPEED_UNKNOWN and DUPLEX_UNKNOWN
values so it is expected that for non in band mode state contains valid speed and duplex mode
which are different from *_UNKNOWN values.

Regards,
Parshuram Thombare