Re: [PATCH v2 02/15] net: phy: adin: hook genphy_read_abilities() to get_features

[Ardelean, Alexandru]

On Thu, 2019-08-08 at 21:32 +0200, Heiner Kallweit wrote:
> [External]
> 
> On 08.08.2019 17:24, Andrew Lunn wrote:
> > On Thu, Aug 08, 2019 at 03:30:13PM +0300, Alexandru Ardelean wrote:
> > > The ADIN PHYs can operate with Clause 45, however they are not typical for
> > > how phylib considers Clause 45 PHYs.
> > > 
> > > If the `features` field & the `get_features` hook are unspecified, and the
> > > device wants to operate via Clause 45, it would also try to read features
> > > via the `genphy_c45_pma_read_abilities()`, which will try to read PMA regs
> > > that are unsupported.
> > > 
> > > Hooking the `genphy_read_abilities()` function to the `get_features` hook
> > > will ensure that this does not happen and the PHY features are read
> > > correctly regardless of Clause 22 or Clause 45 operation.
> > 
> > I think we need to stop and think about a PHY which supports both C22
> > and C45.
> > 
> > How does bus enumeration work? Is it discovered twice?  I've always
> > considered phydev->is_c45 means everything is c45, not that some
> > registers can be accessed via c45. But the driver is mixing c22 and
> > c45. Does the driver actually require c45? Are some features which are
> > only accessibly via C45? What does C45 actually bring us for this
> > device?
> > 

Hmm, I can't answer [all] these questions.

These PHYs seem to be a bit different from the rest that I looked at in drivers/net/phy with regard to C45 & C22.
And C45 seems to be more/closer related to 10G PHYs [from what I can tell].

The ADIN PHYs can operate only in C22.
The only thing that is needed [and a bit special] is EEE, which [for C22] requires the translation layer to convert C45
reg addresses to internal C22 equivalent.

> genphy_c45_pma_read_abilities() is only called if phydev->is_c45 is set.
> And this flag means that the PHY complies with Clause 45 incl. all the
> standard devices like PMA. In the case here only some vendor-specific
> registers can be accessed via Clause 45 and therefore is_c45 shouldn't
> bet set. As a consequence this patch isn't needed.

ack, will drop patch

> 
> >      Andrew
> > 
> Heiner
>