Re: [PATCH] net: phy: add qca8081 ethernet phy driver

[Jie Luo <luoj@codeaurora.org>]

On 8/17/2021 9:33 PM, Andrew Lunn wrote:
> On Tue, Aug 17, 2021 at 09:10:44PM +0800, Jie Luo wrote:
>> On 8/16/2021 9:56 PM, Andrew Lunn wrote:
>>> On Mon, Aug 16, 2021 at 07:34:40PM +0800, Luo Jie wrote:
>>>> qca8081 is industry’s lowest cost and power 1-port 2.5G/1G Ethernet PHY
>>>> chip, which implements SGMII/SGMII+ for interface to SoC.
>>> Hi Luo
>>>
>>> No Marketing claims in the commit message please. Even if it is
>>> correct now, it will soon be wrong with newer generations of
>>> devices.
>>>
>>> And what is SGMII+? Please reference a document. Is it actually
>>> 2500BaseX?
>> Hi Andrew,
>>
>> thanks for the comments, will remove the claims in the next patch.
>>
>> SGMII+ is for 2500BaseX, which is same as SGMII, but the clock frequency of
>> SGMII+ is 2.5 times SGMII.
> 25000BaseX is not SGMII over clocked at 2.5GHz.
>
> If it is using 2500BaseX then call it 2500BaseX, because 2500BaseX is
> well defined in the standards, and SGMII overclocked to 2.5G is not
> standardised.

will update it to use 2500BaseX in the next patch, thanks for this comment.

>
>>> A lot of these registers look the same as the at803x. So i'm thinking
>>> you should merge these two drivers. There is a lot of code which is
>>> identical, or very similar, which you can share.
>> Hi Andrew,
>>
>> qca8081 supports IEEE1588 feature, the IEEE1588 code may be submitted in the
>> near future,
>>
>> so it may be a good idea to keep it out from at803x code.
> Please merge it. A lot of the code is the same, and a lot of the new
> code you are adding will go away once you use the helpers. And
> probably you can improve the features of the older PHYs at the same
> time, where features are the same between them.
thanks for the comment, will update the patch to merge it into at803x code.
>
>>>> +static int qca808x_phy_ms_seed_enable(struct phy_device *phydev, bool enable)
>>>> +{
>>>> +	u16 seed_enable = 0;
>>>> +
>>>> +	if (enable)
>>>> +		seed_enable = QCA808X_MASTER_SLAVE_SEED_ENABLE;
>>>> +
>>>> +	return qca808x_debug_reg_modify(phydev, QCA808X_PHY_DEBUG_LOCAL_SEED,
>>>> +			QCA808X_MASTER_SLAVE_SEED_ENABLE, seed_enable);
>>>> +}
>>> This is interesting. I've not seen any other PHY does this. is there
>>> documentation? Is the datasheet available?
>> this piece of code is for configuring the random seed to a lower value to
>> make the PHY linked
>>
>> as the SLAVE mode for fixing some IOT issue, for master/slave
>> auto-negotiation, please refer to
>>
>> https://www.ieee802.org/3/an/public/jul04/lynskey_2_0704.pdf.
> And what happens when this device is used in an Ethernet switch? A
> next generation of a qca8k? Take a look at
> genphy_setup_master_slave().  Use MASTER_SLAVE_CFG_MASTER_PREFERRED or
> MASTER_SLAVE_CFG_SLAVE_PREFERRED to decide how to bias the
> master/slave decision.
>
>       Andrew

Hi Andrew, thanks for this comment, qca8081 is mainly used in the IPQ 
SOC chip currently.

qca801 is configured as MASTER_SLAVE_CFG_SLAVE_PREFERRED by default.

the SEED random lower value is for making the qca8081 linked as SLAVE 
mode when the link

partner is also configured as MASTER_SLAVE_CFG_SLAVE_PREFERRED in 
auto-negotiation.

.