Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Sender: luoj=codeaurora.org@mg.codeaurora.org
DMARC-Filter: OpenDMARC Filter v1.4.1 smtp.codeaurora.org 0AE05C4338F
Subject: Re: [PATCH] net: phy: add qca8081 ethernet phy driver
To:     Andrew Lunn <andrew@lunn.ch>
Cc:     hkallweit1@gmail.com, linux@armlinux.org.uk, davem@davemloft.net,
        kuba@kernel.org, linux-kernel@vger.kernel.org,
        netdev@vger.kernel.org, sricharan@codeaurora.org
References: <20210816113440.22290-1-luoj@codeaurora.org>
 <YRpuhIcwN2IsaHzy@lunn.ch>
 <6856a839-0fa0-1240-47cd-ae8536294bcd@codeaurora.org>
 <YRu6n49p/Evecd8P@lunn.ch>
From:   Jie Luo <luoj@codeaurora.org>
Message-ID: <92c80a0b-87ad-252b-7a9a-30176e7f06cf@codeaurora.org>
Date:   Wed, 18 Aug 2021 21:21:45 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101
 Thunderbird/78.13.0
MIME-Version: 1.0
In-Reply-To: <YRu6n49p/Evecd8P@lunn.ch>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Precedence: bulk


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.

.