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

[Jie Luo]

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.

.