Re: [PATCH] phylib: add driver for aquantia phy

[Florian Fainelli <f.fainelli@gmail.com>]

On 27/07/15 01:30, Shaohui Xie wrote:
>> -----Original Message-----
>> From: Florian Fainelli [mailto:f.fainelli@gmail.com]
>> Sent: Friday, July 24, 2015 12:39 PM
>> To: shh.xie@gmail.com; netdev@vger.kernel.org; davem@davemloft.net
>> Cc: Xie Shaohui-B21989
>> Subject: Re: [PATCH] phylib: add driver for aquantia phy
>>
>> Le 07/23/15 20:46, shh.xie@gmail.com a écrit :
>>> From: Shaohui Xie <Shaohui.Xie@freescale.com>
>>>
>>> This patch added driver to support Aquantia PHYs AQ1202, AQ2104,
>>> AQR105, AQR405, which accessed through clause 45.
>>
>> Could you prefix your patches with "net: phy: " in the future to be
>> consistent with what is typically used?
> [S.H] OK.
> 
>>
>> See comments below
>>
>>>
>>> Signed-off-by: Shaohui Xie <Shaohui.Xie@freescale.com>
>>> ---
>>
>> [snip]
>>
>>> +static int aquantia_read_status(struct phy_device *phydev) {
>>> +	int reg;
>>> +
>>> +	phydev->speed = SPEED_10000;
>>> +	phydev->duplex = DUPLEX_FULL;
>>> +
>>> +	reg = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_STAT1);
>>> +	reg = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_STAT1);
>>> +	if (reg & MDIO_STAT1_LSTATUS)
>>> +		phydev->link = 1;
>>> +	else
>>> +		phydev->link = 0;
>>> +
>>> +	reg = phy_read_mmd(phydev, MDIO_MMD_AN, 0xc800);
>>> +	mdelay(10);
>>> +	reg = phy_read_mmd(phydev, MDIO_MMD_AN, 0xc800);
>>> +	if (reg == 0x9)
>>> +		phydev->speed = SPEED_2500;
>>> +	else if (reg == 0x5)
>>> +		phydev->speed = SPEED_1000;
>>> +	else if (reg == 0x3)
>>> +		phydev->speed = SPEED_100;
>>
>> Could we use a switch/case here? 
> [S.H] OK.
> 
> How about 10Mbits/sec and duplex are we
>> guaranteed to be full-duplex at e.g: 100 or 10Mbits/sec?
> [S.H] The PHY does not support 10M bits/sec. 
> When link to 100M. the phy is full-duplex.

Ok, that means you need to restrict the supported flags accordingly not
to advertise these modes as being supported in the first place, see below:

> 
>>
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static struct phy_driver aquantia_driver[] = { {
>>> +	.phy_id		= PHY_ID_AQ1202,
>>> +	.phy_id_mask	= 0xfffffff0,
>>> +	.name		= "Aquantia AQ1202",
>>> +	.features	= PHY_GBIT_FEATURES,
>>
>> If these are 10GbE PHYs, should not we start defining a new features
>> bitmask here to reflect that accordingly? That way MAC
> [S.H] there are several defines for 10G PHYs, should be used by a given 10G PHY. 
> 
> for this Aquantia PHY, SUPPORTED_10000baseT_Full is a valid define, should I set it as below:
> .features	= PHY_GBIT_FEATURES | SUPPORTED_10000baseT_Full,

PHY_GBIT_FEATURES means 10/100/1000 half and full-duplex are supported,
which are not supported as you indicated above, I would go with adding
only the supported modes here, this is really important since this is
the contract between the PHY driver and the Ethernet MAC using it
through the PHY library.

Thanks!
-- 
Florian