RE: [PATCH V3 net-next 3/4] net: phy: realtek: add dt property to enable ALDPS mode

[Joakim Zhang]

Hi Jisheng,

> -----Original Message-----
> From: Jisheng Zhang <Jisheng.Zhang@xxxxxxxxxxxxx>
> Sent: 2021年6月9日 9:57
> To: Joakim Zhang <qiangqing.zhang@xxxxxxx>
> Cc: davem@xxxxxxxxxxxxx; kuba@xxxxxxxxxx; robh+dt@xxxxxxxxxx;
> andrew@xxxxxxx; hkallweit1@xxxxxxxxx; linux@xxxxxxxxxxxxxxx;
> f.fainelli@xxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx>;
> netdev@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH V3 net-next 3/4] net: phy: realtek: add dt property to
> enable ALDPS mode
> 
> On Tue, 8 Jun 2021 10:14:40 +0000
> Joakim Zhang <qiangqing.zhang@xxxxxxx> wrote:
> 
> 
> >
> >
> > Hi Jisheng,
> 
> Hi,
> 
> >
> > > -----Original Message-----
> > > From: Jisheng Zhang <Jisheng.Zhang@xxxxxxxxxxxxx>
> > > Sent: 2021年6月8日 17:51
> > > To: Joakim Zhang <qiangqing.zhang@xxxxxxx>
> > > Cc: davem@xxxxxxxxxxxxx; kuba@xxxxxxxxxx; robh+dt@xxxxxxxxxx;
> > > andrew@xxxxxxx; hkallweit1@xxxxxxxxx; linux@xxxxxxxxxxxxxxx;
> > > f.fainelli@xxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx>;
> > > netdev@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx;
> > > linux-kernel@xxxxxxxxxxxxxxx
> > > Subject: Re: [PATCH V3 net-next 3/4] net: phy: realtek: add dt
> > > property to enable ALDPS mode
> > >
> > > On Tue,  8 Jun 2021 11:15:34 +0800
> > > Joakim Zhang <qiangqing.zhang@xxxxxxx> wrote:
> > >
> > >
> > > >
> > > >
> > > > If enable Advance Link Down Power Saving (ALDPS) mode, it will
> > > > change crystal/clock behavior, which cause RXC clock stop for
> > > > dozens to hundreds of miliseconds. This is comfirmed by Realtek
> > > > engineer. For some MACs, it needs RXC clock to support RX logic,
> > > > after this patch, PHY can generate continuous RXC clock during
> auto-negotiation.
> > > >
> > > > ALDPS default is disabled after hardware reset, it's more
> > > > reasonable to add a property to enable this feature, since ALDPS
> > > > would introduce side
> > > effect.
> > > > This patch adds dt property "realtek,aldps-enable" to enable ALDPS
> > > > mode per users' requirement.
> > > >
> > > > Jisheng Zhang enables this feature, changes the default behavior.
> > > > Since mine patch breaks the rule that new implementation should
> > > > not break existing design, so Cc'ed let him know to see if it can be
> accepted.
> > > >
> > > > Cc: Jisheng Zhang <Jisheng.Zhang@xxxxxxxxxxxxx>
> > > > Signed-off-by: Joakim Zhang <qiangqing.zhang@xxxxxxx>
> > > > ---
> > > >  drivers/net/phy/realtek.c | 20 +++++++++++++++++---
> > > >  1 file changed, 17 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
> > > > index ca258f2a9613..79dc55bb4091 100644
> > > > --- a/drivers/net/phy/realtek.c
> > > > +++ b/drivers/net/phy/realtek.c
> > > > @@ -76,6 +76,7 @@ MODULE_AUTHOR("Johnson Leung");
> > > > MODULE_LICENSE("GPL");
> > > >
> > > >  struct rtl821x_priv {
> > > > +       u16 phycr1;
> > > >         u16 phycr2;
> > > >  };
> > > >
> > > > @@ -98,6 +99,14 @@ static int rtl821x_probe(struct phy_device
> *phydev)
> > > >         if (!priv)
> > > >                 return -ENOMEM;
> > > >
> > > > +       priv->phycr1 = phy_read_paged(phydev, 0xa43,
> > > RTL8211F_PHYCR1);
> > > > +       if (priv->phycr1 < 0)
> > > > +               return priv->phycr1;
> > > > +
> > > > +       priv->phycr1 &= (RTL8211F_ALDPS_PLL_OFF |
> > > > + RTL8211F_ALDPS_ENABLE | RTL8211F_ALDPS_XTAL_OFF);
> 
> I believe your intention is
> 
> priv->phycr1 &= ~(RTL8211F_ALDPS_PLL_OFF | RTL8211F_ALDPS_ENABLE |
> priv->RTL8211F_ALDPS_XTAL_OFF);
> However, this is not necessary. See below.

No, mine intention is to read back this three bits what the register contained.

> > >
> > > priv->phycr1 is 0 by default, so above 5 LoCs can be removed
> >
> > The intention of this is to take bootloader into account. Such as uboot
> configure the PHY before.
> 
> The last param "set" of phy_modify_paged_changed() means *bit mask of bits
> to set* If we don't want to enable ALDPS, 0 is enough.
> 
> Even if uboot configured the PHY before linux, I believe
> phy_modify_paged_changed() can clear ALDPS bits w/o above 5 LoCs.

The logic is:
1) read back these three bits from the register.
2) if linux set "realtek,aldps-enable", assert these three bit; if not, keep these three bits read before.
3) call phy_modify_paged_changed() to configure, "mask" parameter to clear these three bits first, "set" parameter to assert these three bits per the result of step 2.

So, if step 1 read back the value is that these three bits are asserted, then in step 3, it will first clear these three bits and assert these three bits again. The result is ALDPS is enabled even without " realtek,aldps-enable " in DT.

Best Regards,
Joakim Zhang
> Thanks
> 
> >
> > Best Regards,
> > Joakim Zhang
> > > > +       if (of_property_read_bool(dev->of_node,
> "realtek,aldps-enable"))
> > > > +               priv->phycr1 |= RTL8211F_ALDPS_PLL_OFF |
> > > > + RTL8211F_ALDPS_ENABLE | RTL8211F_ALDPS_XTAL_OFF;
> > > > +
> > > >         priv->phycr2 = phy_read_paged(phydev, 0xa43,
> > > RTL8211F_PHYCR2);
> > > >         if (priv->phycr2 < 0)
> > > >                 return priv->phycr2; @@ -324,11 +333,16 @@ static
> > > > int rtl8211f_config_init(struct phy_device
> > > *phydev)
> > > >         struct rtl821x_priv *priv = phydev->priv;
> > > >         struct device *dev = &phydev->mdio.dev;
> > > >         u16 val_txdly, val_rxdly;
> > > > -       u16 val;
> > > >         int ret;
> > > >
> > > > -       val = RTL8211F_ALDPS_ENABLE | RTL8211F_ALDPS_PLL_OFF |
> > > RTL8211F_ALDPS_XTAL_OFF;
> > > > -       phy_modify_paged_changed(phydev, 0xa43, RTL8211F_PHYCR1,
> val,
> > > val);
> > > > +       ret = phy_modify_paged_changed(phydev, 0xa43,
> > > RTL8211F_PHYCR1,
> > > > +
> RTL8211F_ALDPS_PLL_OFF |
> > > RTL8211F_ALDPS_ENABLE | RTL8211F_ALDPS_XTAL_OFF,
> > > > +                                      priv->phycr1);
> > > > +       if (ret < 0) {
> > > > +               dev_err(dev, "aldps mode  configuration
> failed: %pe\n",
> > > > +                       ERR_PTR(ret));
> > > > +               return ret;
> > > > +       }
> > > >
> > > >         switch (phydev->interface) {
> > > >         case PHY_INTERFACE_MODE_RGMII:
> > > > --
> > > > 2.17.1
> > > >
> >