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日 11:04
> 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 Wed, 9 Jun 2021 02:51:11 +0000
> Joakim Zhang <qiangqing.zhang@xxxxxxx> wrote:
> 
> > CAUTION: Email originated externally, do not click links or open attachments
> unless you recognize the sender and know the content is safe.
> >
> >
> > 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.
> >
> 
> Aha, I see you want to keep the ALDPS bits(maybe configured by prelinux env)
> untouched.
> If ALDPS has been enabled by prelinux env, even there's no
> "realtek,aldps-enable"
> in DT, the ALDPS may be keep enabled in linux. Thus the ALDPS behavior rely on
> the prelinux env. I'm not sure whether this is correct or not.
> 
> IMHO, the "realtek,aldps-enable" is a "yes" or "no" bool. If it's set, ALDPS will
> be enabled in linux; If it's no, ALDPS will be disabled in linux. We should not rely
> on prelinux env.

You can refer to below V1 comments, we may not take enough cases into account, others may care about boot loader. 
https://lkml.org/lkml/2021/6/1/264

Anyway, keep the original value in registers should not have side effects. Thanks for your reviewing, and I saw this patch set has been
accepted, if you want to improve this code, I am happy to review and test your patch.

Best Regards,
Joakim Zhang
> Thanks