Re: [PATCH net-next v8 6/9] net: phy: c22: migrate to genphy_c45_write_eee_adv()

[Guenter Roeck]

On 2/24/23 22:08, Oleksij Rempel wrote:
> On Fri, Feb 24, 2023 at 04:09:40PM -0800, Guenter Roeck wrote:
> > On 2/24/23 12:02, Oleksij Rempel wrote:
> > [ ... ]
> > > > For cubieboard:
> > > >
> > > > MDIO_PCS_EEE_ABLE = 0x0000
> > > >
> > > > qemu reports attempts to access unsupported registers.
> > > >
> > > > I had a look at the Allwinner mdio driver. There is no indication suggesting
> > > > what the real hardware would return when trying to access unsupported registers,
> > > > and the Ethernet controller datasheet is not public.
> > >
> > > These are PHY accesses over MDIO bus. Ethernet controller should not
> > > care about content of this operations. But on qemu side, it is implemented as
> > > part of Ethernet controller emulation...
> > >
> > > Since MDIO_PCS_EEE_ABLE == 0x0000, phydev->supported_eee should prevent
> > > other EEE related operations. But may be actual phy_read_mmd() went
> > > wrong. It is a combination of simple phy_read/write to different
> > > registers.
> >
> > Adding MDD read/write support in qemu doesn't help. Something else in your patch
> > prevents the PHY from coming up. After reverting your patch, I see
> >
> > sun4i-emac 1c0b000.ethernet eth0: Link is Up - 100Mbps/Full - flow control off
> > IPv6: ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready
> >
> > in the log. This is missing with your patch in place.
> >
> > Anyway, the key difference is not really the qemu emulation, but the added
> > unconditional call to genphy_c45_write_eee_adv() in your patch. If you look
> > closely into that function, you may notice that the 'changed' variable is
> > never set to 0.
> >
> > diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
> > index 3813b86689d0..fee514b96ab1 100644
> > --- a/drivers/net/phy/phy-c45.c
> > +++ b/drivers/net/phy/phy-c45.c
> > @@ -672,7 +672,7 @@ EXPORT_SYMBOL_GPL(genphy_c45_read_mdix);
> >    */
> >   int genphy_c45_write_eee_adv(struct phy_device *phydev, unsigned long *adv)
> >   {
> > -       int val, changed;
> > +       int val, changed = 0;
> >
> >          if (linkmode_intersects(phydev->supported_eee, PHY_EEE_CAP1_FEATURES)) {
> >                  val = linkmode_to_mii_eee_cap1_t(adv);
> >
> > fixes the problem, both for cubieboard and xtensa.
>
> Good point! Thx for finding it!
>
> Do you wont to send the fix against net?

No, please go ahead and do it.

Guenter