[PATCH v2 phy 15/16] phy: lynx-28g: truly power the lanes up or down

Vladimir Oltean posted 16 patches 1 week, 1 day ago
There is a newer version of this series
[PATCH v2 phy 15/16] phy: lynx-28g: truly power the lanes up or down
Posted by Vladimir Oltean 1 week, 1 day ago
The current procedure for power_off() and power_on() is the same as the
one used for major lane reconfiguration, aka halting.

But one would expect that a powered off lane causes the CDR (clock and
data recovery) loop of the link partner to lose lock onto its RX stream
(which suggests there are no longer any bit transitions => the channel
is inactive). However, this does not take place (the CDR lock is still
there), so a halted lane is still active.

Implement the procedure mentioned in the block guide for powering down
a lane, and then back on.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v1->v2: slight commit message reword

 drivers/phy/freescale/phy-fsl-lynx-28g.c | 78 ++++++++++++++++++------
 1 file changed, 60 insertions(+), 18 deletions(-)

diff --git a/drivers/phy/freescale/phy-fsl-lynx-28g.c b/drivers/phy/freescale/phy-fsl-lynx-28g.c
index 6f2078721aca..798343b55ec7 100644
--- a/drivers/phy/freescale/phy-fsl-lynx-28g.c
+++ b/drivers/phy/freescale/phy-fsl-lynx-28g.c
@@ -73,9 +73,11 @@
 
 /* Lane a Tx Reset Control Register */
 #define LNaTRSTCTL(lane)			(0x800 + (lane) * 0x100 + 0x20)
-#define LNaTRSTCTL_HLT_REQ			BIT(27)
-#define LNaTRSTCTL_RST_DONE			BIT(30)
 #define LNaTRSTCTL_RST_REQ			BIT(31)
+#define LNaTRSTCTL_RST_DONE			BIT(30)
+#define LNaTRSTCTL_HLT_REQ			BIT(27)
+#define LNaTRSTCTL_STP_REQ			BIT(26)
+#define LNaTRSTCTL_DIS				BIT(24)
 
 /* Lane a Tx General Control Register */
 #define LNaTGCR0(lane)				(0x800 + (lane) * 0x100 + 0x24)
@@ -102,9 +104,11 @@
 
 /* Lane a Rx Reset Control Register */
 #define LNaRRSTCTL(lane)			(0x800 + (lane) * 0x100 + 0x40)
-#define LNaRRSTCTL_HLT_REQ			BIT(27)
-#define LNaRRSTCTL_RST_DONE			BIT(30)
 #define LNaRRSTCTL_RST_REQ			BIT(31)
+#define LNaRRSTCTL_RST_DONE			BIT(30)
+#define LNaRRSTCTL_HLT_REQ			BIT(27)
+#define LNaRRSTCTL_STP_REQ			BIT(26)
+#define LNaRRSTCTL_DIS				BIT(24)
 #define LNaRRSTCTL_CDR_LOCK			BIT(12)
 
 /* Lane a Rx General Control Register */
@@ -672,14 +676,12 @@ static void lynx_28g_lane_set_pll(struct lynx_28g_lane *lane,
 	}
 }
 
-static int lynx_28g_power_off(struct phy *phy)
+/* Halting puts the lane in a mode in which it can be reconfigured */
+static void lynx_28g_lane_halt(struct phy *phy)
 {
 	struct lynx_28g_lane *lane = phy_get_drvdata(phy);
 	u32 trstctl, rrstctl;
 
-	if (!lane->powered_up)
-		return 0;
-
 	/* Issue a halt request */
 	lynx_28g_lane_rmw(lane, LNaTRSTCTL, LNaTRSTCTL_HLT_REQ,
 			  LNaTRSTCTL_HLT_REQ);
@@ -692,20 +694,13 @@ static int lynx_28g_power_off(struct phy *phy)
 		rrstctl = lynx_28g_lane_read(lane, LNaRRSTCTL);
 	} while ((trstctl & LNaTRSTCTL_HLT_REQ) ||
 		 (rrstctl & LNaRRSTCTL_HLT_REQ));
-
-	lane->powered_up = false;
-
-	return 0;
 }
 
-static int lynx_28g_power_on(struct phy *phy)
+static void lynx_28g_lane_reset(struct phy *phy)
 {
 	struct lynx_28g_lane *lane = phy_get_drvdata(phy);
 	u32 trstctl, rrstctl;
 
-	if (lane->powered_up)
-		return 0;
-
 	/* Issue a reset request on the lane */
 	lynx_28g_lane_rmw(lane, LNaTRSTCTL, LNaTRSTCTL_RST_REQ,
 			  LNaTRSTCTL_RST_REQ);
@@ -718,6 +713,52 @@ static int lynx_28g_power_on(struct phy *phy)
 		rrstctl = lynx_28g_lane_read(lane, LNaRRSTCTL);
 	} while (!(trstctl & LNaTRSTCTL_RST_DONE) ||
 		 !(rrstctl & LNaRRSTCTL_RST_DONE));
+}
+
+static int lynx_28g_power_off(struct phy *phy)
+{
+	struct lynx_28g_lane *lane = phy_get_drvdata(phy);
+	u32 trstctl, rrstctl;
+
+	if (!lane->powered_up)
+		return 0;
+
+	/* Issue a stop request */
+	lynx_28g_lane_rmw(lane, LNaTRSTCTL, LNaTRSTCTL_STP_REQ,
+			  LNaTRSTCTL_STP_REQ);
+	lynx_28g_lane_rmw(lane, LNaRRSTCTL, LNaRRSTCTL_STP_REQ,
+			  LNaRRSTCTL_STP_REQ);
+
+	/* Wait until the stop process is complete */
+	do {
+		trstctl = lynx_28g_lane_read(lane, LNaTRSTCTL);
+		rrstctl = lynx_28g_lane_read(lane, LNaRRSTCTL);
+	} while ((trstctl & LNaTRSTCTL_STP_REQ) ||
+		 (rrstctl & LNaRRSTCTL_STP_REQ));
+
+	/* Power down the RX and TX portions of the lane */
+	lynx_28g_lane_rmw(lane, LNaRRSTCTL, LNaRRSTCTL_DIS,
+			  LNaRRSTCTL_DIS);
+	lynx_28g_lane_rmw(lane, LNaTRSTCTL, LNaTRSTCTL_DIS,
+			  LNaTRSTCTL_DIS);
+
+	lane->powered_up = false;
+
+	return 0;
+}
+
+static int lynx_28g_power_on(struct phy *phy)
+{
+	struct lynx_28g_lane *lane = phy_get_drvdata(phy);
+
+	if (lane->powered_up)
+		return 0;
+
+	/* Power up the RX and TX portions of the lane */
+	lynx_28g_lane_rmw(lane, LNaRRSTCTL, 0, LNaRRSTCTL_DIS);
+	lynx_28g_lane_rmw(lane, LNaTRSTCTL, 0, LNaTRSTCTL_DIS);
+
+	lynx_28g_lane_reset(phy);
 
 	lane->powered_up = true;
 
@@ -1132,7 +1173,7 @@ static int lynx_28g_set_mode(struct phy *phy, enum phy_mode mode, int submode)
 	 * the reconfiguration is being done.
 	 */
 	if (powered_up)
-		lynx_28g_power_off(phy);
+		lynx_28g_lane_halt(phy);
 
 	err = lynx_28g_lane_disable_pcvt(lane, lane->mode);
 	if (err)
@@ -1145,8 +1186,9 @@ static int lynx_28g_set_mode(struct phy *phy, enum phy_mode mode, int submode)
 	lane->mode = lane_mode;
 
 out:
+	/* Reset the lane if necessary */
 	if (powered_up)
-		lynx_28g_power_on(phy);
+		lynx_28g_lane_reset(phy);
 
 	return err;
 }
-- 
2.34.1
Re: [PATCH v2 phy 15/16] phy: lynx-28g: truly power the lanes up or down
Posted by Josua Mayer 1 week ago
Am 23.09.25 um 21:44 schrieb Vladimir Oltean:
> The current procedure for power_off() and power_on() is the same as the
> one used for major lane reconfiguration, aka halting.
>
> But one would expect that a powered off lane causes the CDR (clock and
> data recovery) loop of the link partner to lose lock onto its RX stream
> (which suggests there are no longer any bit transitions => the channel
> is inactive). However, this does not take place (the CDR lock is still
> there), so a halted lane is still active.
>
> Implement the procedure mentioned in the block guide for powering down
> a lane, and then back on.
>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
> v1->v2: slight commit message reword
>
>  drivers/phy/freescale/phy-fsl-lynx-28g.c | 78 ++++++++++++++++++------
>  1 file changed, 60 insertions(+), 18 deletions(-)
cut
> +static int lynx_28g_power_off(struct phy *phy)
> +{
> +	struct lynx_28g_lane *lane = phy_get_drvdata(phy);
> +	u32 trstctl, rrstctl;
> +
> +	if (!lane->powered_up)
> +		return 0;
> +
> +	/* Issue a stop request */
> +	lynx_28g_lane_rmw(lane, LNaTRSTCTL, LNaTRSTCTL_STP_REQ,
> +			  LNaTRSTCTL_STP_REQ);
> +	lynx_28g_lane_rmw(lane, LNaRRSTCTL, LNaRRSTCTL_STP_REQ,
> +			  LNaRRSTCTL_STP_REQ);
> +
> +	/* Wait until the stop process is complete */
> +	do {
> +		trstctl = lynx_28g_lane_read(lane, LNaTRSTCTL);
> +		rrstctl = lynx_28g_lane_read(lane, LNaRRSTCTL);
> +	} while ((trstctl & LNaTRSTCTL_STP_REQ) ||
> +		 (rrstctl & LNaRRSTCTL_STP_REQ));

Unbounded loop, perhaps use timeout.

This can fail on unbalanced calls as you discovered,
but also e.g. when a pll is unstable.

See below for when this came up previously:

https://lore.kernel.org/all/20240218-lynx28g-infinite-loop-v1-1-59cc5cef8367@solid-run.com/

Re: [PATCH v2 phy 15/16] phy: lynx-28g: truly power the lanes up or down
Posted by Vladimir Oltean 1 week ago
On Wed, Sep 24, 2025 at 10:09:26AM +0000, Josua Mayer wrote:
> Unbounded loop, perhaps use timeout.
> 
> This can fail on unbalanced calls as you discovered,
> but also e.g. when a pll is unstable.
> 
> See below for when this came up previously:
> 
> https://lore.kernel.org/all/20240218-lynx28g-infinite-loop-v1-1-59cc5cef8367@solid-run.com/

Ok, I can make this a 17-patch set, no problem.

What happened to your patch? Did it get lost? Do you mind if I write a
new one myself, with a single read_poll_timeout() call for both
LNaTRSTCTL and LNaRRSTCTL, to keep the functionality same as before?
Re: [PATCH v2 phy 15/16] phy: lynx-28g: truly power the lanes up or down
Posted by Josua Mayer 1 week ago
Am 24.09.25 um 15:06 schrieb Vladimir Oltean:
> On Wed, Sep 24, 2025 at 10:09:26AM +0000, Josua Mayer wrote:
>> Unbounded loop, perhaps use timeout.
>>
>> This can fail on unbalanced calls as you discovered,
>> but also e.g. when a pll is unstable.
>>
>> See below for when this came up previously:
>>
>> https://lore.kernel.org/all/20240218-lynx28g-infinite-loop-v1-1-59cc5cef8367@solid-run.com/
> Ok, I can make this a 17-patch set, no problem.
>
> What happened to your patch? Did it get lost?
I suspect I got distracted and never sent a v2.
> Do you mind if I write a
> new one myself, with a single read_poll_timeout() call for both
> LNaTRSTCTL and LNaRRSTCTL, to keep the functionality same as before?
Sure, feel free.
Since tx and rx are in different registers you will still need to poll each one?