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

Hi Jisheng,

-----Original Message-----
From: Jisheng Zhang <redacted>
Sent: 2021年6月9日 11:04
To: Joakim Zhang <redacted>
Cc: davem@davemloft.net; kuba@kernel.org; robh+dt@kernel.org;
andrew@lunn.ch; hkallweit1@gmail.com; linux@armlinux.org.uk;
f.fainelli@gmail.com; dl-linux-imx [off-list ref];
netdev@vger.kernel.org; devicetree@vger.kernel.org;
linux-kernel@vger.kernel.org
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 [off-list ref] wrote:

quoted

CAUTION: Email originated externally, do not click links or open attachments

unless you recognize the sender and know the content is safe.

quoted

Hi Jisheng,

quoted

-----Original Message-----
From: Jisheng Zhang <redacted>
Sent: 2021年6月9日 9:57
To: Joakim Zhang <redacted>
Cc: davem@davemloft.net; kuba@kernel.org; robh+dt@kernel.org;
andrew@lunn.ch; hkallweit1@gmail.com; linux@armlinux.org.uk;
f.fainelli@gmail.com; dl-linux-imx [off-list ref];
netdev@vger.kernel.org; devicetree@vger.kernel.org;
linux-kernel@vger.kernel.org
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 [off-list ref] wrote:

quoted

Hi Jisheng,

Hi,

quoted

quoted

-----Original Message-----
From: Jisheng Zhang <redacted>
Sent: 2021年6月8日 17:51
To: Joakim Zhang <redacted>
Cc: davem@davemloft.net; kuba@kernel.org; robh+dt@kernel.org;
andrew@lunn.ch; hkallweit1@gmail.com; linux@armlinux.org.uk;
f.fainelli@gmail.com; dl-linux-imx [off-list ref];
netdev@vger.kernel.org; devicetree@vger.kernel.org;
linux-kernel@vger.kernel.org
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
[off-list ref] wrote:

quoted

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.

quoted

quoted

quoted

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.

quoted

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.

quoted

quoted

quoted

Cc: Jisheng Zhang <redacted>
Signed-off-by: Joakim Zhang <redacted>
---
 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)

quoted

quoted

quoted

        if (!priv)
                return -ENOMEM;

+       priv->phycr1 = phy_read_paged(phydev, 0xa43,

RTL8211F_PHYCR1);

quoted

+       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.

quoted

quoted

quoted

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.

quoted

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.

quoted

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.

quoted

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