[RESEND PATCH 0/3] phy: zynqmp: fix SERDES scrambler register handling and enable for USB

[RESEND PATCH 0/3] phy: zynqmp: fix SERDES scrambler register handling and enable for USB

[Radhey Shyam Pandey]

This series fixes three related issues in the ZynqMP SERDES PHY
scrambler/encoder bypass path:

1. The L0_TM_DISABLE_SCRAMBLE_ENCODER mask incorrectly included bit 2
   of L0_TX_DIG_61, which is a reserved read-only field. Correct the
   mask to (BIT(3) | GENMASK(1, 0)).

2. xpsgtr_bypass_scrambler_8b10b() used xpsgtr_write_phy() which
   performs a full register write, clobbering unrelated bits. Switch
   to xpsgtr_clr_set_phy() with clr=mask, set=mask to preserve other
   register fields.

3. USB Gen1 requires PHY-side scrambling and 8b/10b encoding as
   mandated by the USB 3.x specification. The driver was incorrectly
   bypassing these for USB, the same as SATA and SGMII where encoding
   is handled in the controller.

Issues reported by sashiko[1] are pre-existing issues and will
taken as separate followup series.

[1]: 
https://lore.kernel.org/all/20260512213513.852EEC2BCB0@smtp.kernel.org
https://lore.kernel.org/all/20260512223142.1AFC4C2BCB0@smtp.kernel.org

Nava kishore Manne (3):
  phy: zynqmp: fix L0_TM_DISABLE_SCRAMBLE_ENCODER mask
  phy: zynqmp: use read-modify-write for SERDES scrambler bypass
  phy: zynqmp: keep SERDES scrambler and 8b/10b enabled for USB

 drivers/phy/xilinx/phy-zynqmp.c | 37 ++++++++++++++++++++++++++-------
 1 file changed, 30 insertions(+), 7 deletions(-)


base-commit: 3d5670d672ae08b8c534b7beed6f57c8b44e7b43
-- 
2.43.0


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[RESEND PATCH 1/3] phy: zynqmp: fix L0_TM_DISABLE_SCRAMBLE_ENCODER mask

[Radhey Shyam Pandey]

From: Nava kishore Manne <nava.kishore.manne@amd.com>

The L0_TX_DIG_61 register bit 2 is a reserved read-only field.
The previous mask value 0x0f incorrectly included bit 2, causing
unintended writes to a reserved bit on every scrambler bypass
operation.

Correct the mask to (BIT(3) | GENMASK(1, 0)) to cover only the
valid scramble bypass control bits.

Fixes: 4a33bea00314 ("phy: zynqmp: Add PHY driver for the Xilinx ZynqMP Gigabit Transceiver")
Cc: stable@vger.kernel.org
Signed-off-by: Nava kishore Manne <nava.kishore.manne@amd.com>
Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
---
 drivers/phy/xilinx/phy-zynqmp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/phy/xilinx/phy-zynqmp.c b/drivers/phy/xilinx/phy-zynqmp.c
index fe6b4925d166..c037d7c13d48 100644
--- a/drivers/phy/xilinx/phy-zynqmp.c
+++ b/drivers/phy/xilinx/phy-zynqmp.c
@@ -53,7 +53,7 @@
 #define L0_TM_DIG_6			0x106c
 #define L0_TM_DIS_DESCRAMBLE_DECODER	0x0f
 #define L0_TX_DIG_61			0x00f4
-#define L0_TM_DISABLE_SCRAMBLE_ENCODER	0x0f
+#define L0_TM_DISABLE_SCRAMBLE_ENCODER	(BIT(3) | GENMASK(1, 0))
 
 /* PLL Test Mode register parameters */
 #define L0_TM_PLL_DIG_37		0x2094
-- 
2.43.0


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [RESEND PATCH 1/3] phy: zynqmp: fix L0_TM_DISABLE_SCRAMBLE_ENCODER mask

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] The return value of clk_prepare_enable() is not captured, leading to a masked error and potential hardware faults.
- [Medium] Missing clk_disable_unprepare() on error paths causes a clock reference count leak.
--

commit b562ec219de1a77ec468ae7c48ab00117de6c7d1
Author: Nava kishore Manne <nava.kishore.manne@amd.com>

phy: zynqmp: fix L0_TM_DISABLE_SCRAMBLE_ENCODER mask

This patch fixes the L0_TM_DISABLE_SCRAMBLE_ENCODER mask in the ZynqMP
PHY driver. The previous mask incorrectly included a reserved read-only
field, which has now been excluded.

> diff --git a/drivers/phy/xilinx/phy-zynqmp.c b/drivers/phy/xilinx/phy-zynqmp.c
> index fe6b4925d1662..c037d7c13d485 100644
> --- a/drivers/phy/xilinx/phy-zynqmp.c
> +++ b/drivers/phy/xilinx/phy-zynqmp.c
> @@ -53,7 +53,7 @@
>  #define L0_TM_DIG_6			0x106c
>  #define L0_TM_DIS_DESCRAMBLE_DECODER	0x0f
>  #define L0_TX_DIG_61			0x00f4
> -#define L0_TM_DISABLE_SCRAMBLE_ENCODER	0x0f
> +#define L0_TM_DISABLE_SCRAMBLE_ENCODER	(BIT(3) | GENMASK(1, 0))
>  
>  /* PLL Test Mode register parameters */
>  #define L0_TM_PLL_DIG_37		0x2094

[Severity: High]
This is a pre-existing issue, but does xpsgtr_phy_init() improperly mask
errors if the clock fails to enable?

drivers/phy/xilinx/phy-zynqmp.c:xpsgtr_phy_init() {
    ...
	/* Configure and enable the clock when peripheral phy_init call */
	if (clk_prepare_enable(gtr_dev->clk[gtr_phy->refclk]))
		goto out;
    ...
out:
	mutex_unlock(&gtr_dev->gtr_mutex);
	return ret;
}

Since ret is initialized to 0 and the error from clk_prepare_enable() isn't
assigned to it, this function will return 0 on failure.

This could cause the PHY framework to mistakenly believe the PHY is
initialized, potentially leading to unclocked register accesses and system
hangs.

[Severity: Medium]
This is also a pre-existing issue, but could this result in a clock
reference count leak on error paths?

drivers/phy/xilinx/phy-zynqmp.c:xpsgtr_phy_init() {
    ...
	if (clk_prepare_enable(gtr_dev->clk[gtr_phy->refclk]))
		goto out;

	/* Skip initialization if not required. */
	if (!xpsgtr_phy_init_required(gtr_phy))
		goto out;

	if (gtr_dev->tx_term_fix) {
		ret = xpsgtr_phy_tx_term_fix(gtr_phy);
		if (ret < 0)
			goto out;
    ...

If a subsequent step like xpsgtr_phy_tx_term_fix() or xpsgtr_configure_pll()
fails, the function jumps to out and returns an error without calling
clk_disable_unprepare().

Because the initialization failed, phy_exit() will not be called by the
framework, leaving the clock prepared and enabled indefinitely.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260627155229.2791113-1-radhey.shyam.pandey@amd.com?part=1

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[RESEND PATCH 2/3] phy: zynqmp: use read-modify-write for SERDES scrambler bypass

[Radhey Shyam Pandey]

From: Nava kishore Manne <nava.kishore.manne@amd.com>

xpsgtr_bypass_scrambler_8b10b() used xpsgtr_write_phy() which performs
a full register write, silently clearing any bits beyond the intended
bypass control fields.

Switch to xpsgtr_clr_set_phy() with clr=mask, set=mask to set only
the bypass bits while preserving the remaining bits in each register.

Fixes: 4a33bea00314 ("phy: zynqmp: Add PHY driver for the Xilinx ZynqMP Gigabit Transceiver")
Cc: stable@vger.kernel.org
Signed-off-by: Nava kishore Manne <nava.kishore.manne@amd.com>
Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
---
 drivers/phy/xilinx/phy-zynqmp.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/phy/xilinx/phy-zynqmp.c b/drivers/phy/xilinx/phy-zynqmp.c
index c037d7c13d48..6c56c4df8523 100644
--- a/drivers/phy/xilinx/phy-zynqmp.c
+++ b/drivers/phy/xilinx/phy-zynqmp.c
@@ -505,8 +505,12 @@ static void xpsgtr_lane_set_protocol(struct xpsgtr_phy *gtr_phy)
 /* Bypass (de)scrambler and 8b/10b decoder and encoder. */
 static void xpsgtr_bypass_scrambler_8b10b(struct xpsgtr_phy *gtr_phy)
 {
-	xpsgtr_write_phy(gtr_phy, L0_TM_DIG_6, L0_TM_DIS_DESCRAMBLE_DECODER);
-	xpsgtr_write_phy(gtr_phy, L0_TX_DIG_61, L0_TM_DISABLE_SCRAMBLE_ENCODER);
+	xpsgtr_clr_set_phy(gtr_phy, L0_TM_DIG_6,
+			   L0_TM_DIS_DESCRAMBLE_DECODER,
+			   L0_TM_DIS_DESCRAMBLE_DECODER);
+	xpsgtr_clr_set_phy(gtr_phy, L0_TX_DIG_61,
+			   L0_TM_DISABLE_SCRAMBLE_ENCODER,
+			   L0_TM_DISABLE_SCRAMBLE_ENCODER);
 }
 
 /* DP-specific initialization. */
-- 
2.43.0


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [RESEND PATCH 2/3] phy: zynqmp: use read-modify-write for SERDES scrambler bypass

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] The error code from clk_prepare_enable() is not propagated, causing a false success return.
- [High] Missing clk_disable_unprepare() on error paths causes a clock resource leak.
--

commit 5cbba5539e34550743b9636376f1a41a65d04bb8
Author: Nava kishore Manne <nava.kishore.manne@amd.com>

phy: zynqmp: use read-modify-write for SERDES scrambler bypass

This commit switches xpsgtr_bypass_scrambler_8b10b() to use 
xpsgtr_clr_set_phy() instead of xpsgtr_write_phy(). This modifies only the
bypass bits while preserving the rest of the register fields.

> diff --git a/drivers/phy/xilinx/phy-zynqmp.c b/drivers/phy/xilinx/phy-zynqmp.c
> index c037d7c13d485..6c56c4df8523c 100644
> --- a/drivers/phy/xilinx/phy-zynqmp.c
> +++ b/drivers/phy/xilinx/phy-zynqmp.c
> @@ -505,8 +505,12 @@ static void xpsgtr_lane_set_protocol(struct xpsgtr_phy *gtr_phy)
>  /* Bypass (de)scrambler and 8b/10b decoder and encoder. */
>  static void xpsgtr_bypass_scrambler_8b10b(struct xpsgtr_phy *gtr_phy)
>  {

[ ... ]

> static int xpsgtr_phy_init(struct phy *phy)
> {
> 	struct xpsgtr_phy *gtr_phy = phy_get_drvdata(phy);
> 	struct xpsgtr_dev *gtr_dev = gtr_phy->dev;
> 	int ret = 0;
> 
> 	mutex_lock(&gtr_dev->gtr_mutex);
> 
> 	/* Configure and enable the clock when peripheral phy_init call */
> 	if (clk_prepare_enable(gtr_dev->clk[gtr_phy->refclk]))
> 		goto out;

[Severity: High]
This is a pre-existing issue, but does this error path fail to set the
return code? 

When clk_prepare_enable() fails, it jumps to 'out' while 'ret'
is still 0, leading to a false success return. This can cause the PHY
framework to assume the PHY is initialized.

> 
> 	/* Skip initialization if not required. */
> 	if (!xpsgtr_phy_init_required(gtr_phy))
> 		goto out;
> 
> 	if (gtr_dev->tx_term_fix) {
> 		ret = xpsgtr_phy_tx_term_fix(gtr_phy);
> 		if (ret < 0)
> 			goto out;

[Severity: High]
This is a pre-existing issue, but does this error path jump to 'out' without
disabling the previously enabled clock?

Because 'ret' is negative here, the PHY framework considers the
initialization failed and won't call xpsgtr_phy_exit(). This results in
a permanent clock resource leak.

> 
> 		gtr_dev->tx_term_fix = false;
> 	}
> 
> 	/* Enable coarse code saturation limiting logic. */
> 	xpsgtr_write_phy(gtr_phy, L0_TM_PLL_DIG_37, L0_TM_COARSE_CODE_LIMIT);
> 
> 	/*
> 	 * Configure the PLL, the lane protocol, and perform protocol-specific
> 	 * initialization.
> 	 */
> 	ret = xpsgtr_configure_pll(gtr_phy);
> 	if (ret)
> 		goto out;

[Severity: High]
This is a pre-existing issue, but does this error path also leak the clock?

Like the tx_term_fix failure above, this jumps to 'out' without calling
clk_disable_unprepare() on the clock enabled at the start of the function.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260627155229.2791113-1-radhey.shyam.pandey@amd.com?part=2

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[RESEND PATCH 3/3] phy: zynqmp: keep SERDES scrambler and 8b/10b enabled for USB

[Radhey Shyam Pandey]

From: Nava kishore Manne <nava.kishore.manne@amd.com>

USB Gen1 requires scrambling and 8b/10b encoding to be performed in the
physical layer. Do not bypass PHY-side scrambler or encoder/decoder for
USB operation, as mandated by the USB 3.x specification.

Scrambler and 8b/10b bypass remain restricted to SATA and SGMII
modes, where encoding is handled in the controller.

Fixes: 4a33bea00314 ("phy: zynqmp: Add PHY driver for the Xilinx ZynqMP Gigabit Transceiver")
Cc: stable@vger.kernel.org
Signed-off-by: Nava kishore Manne <nava.kishore.manne@amd.com>
Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
---
 drivers/phy/xilinx/phy-zynqmp.c | 39 ++++++++++++++++++++++++---------
 1 file changed, 29 insertions(+), 10 deletions(-)

diff --git a/drivers/phy/xilinx/phy-zynqmp.c b/drivers/phy/xilinx/phy-zynqmp.c
index 6c56c4df8523..087fe402e4e2 100644
--- a/drivers/phy/xilinx/phy-zynqmp.c
+++ b/drivers/phy/xilinx/phy-zynqmp.c
@@ -502,15 +502,30 @@ static void xpsgtr_lane_set_protocol(struct xpsgtr_phy *gtr_phy)
 	}
 }
 
-/* Bypass (de)scrambler and 8b/10b decoder and encoder. */
-static void xpsgtr_bypass_scrambler_8b10b(struct xpsgtr_phy *gtr_phy)
+/**
+ * xpsgtr_bypass_scrambler_8b10b - Configure scrambler/encoder behavior
+ * @gtr_phy: pointer to lane context
+ * @bypass: true to enable scrambler/encoder bypass (SATA/SGMII),
+ *          false to disable scrambler/encoder bypass (USB3)
+ *
+ * Uses RMW to preserve reserved and unrelated register fields.
+ */
+static void xpsgtr_bypass_scrambler_8b10b(struct xpsgtr_phy *gtr_phy,
+					  bool bypass)
 {
-	xpsgtr_clr_set_phy(gtr_phy, L0_TM_DIG_6,
-			   L0_TM_DIS_DESCRAMBLE_DECODER,
-			   L0_TM_DIS_DESCRAMBLE_DECODER);
-	xpsgtr_clr_set_phy(gtr_phy, L0_TX_DIG_61,
-			   L0_TM_DISABLE_SCRAMBLE_ENCODER,
-			   L0_TM_DISABLE_SCRAMBLE_ENCODER);
+	if (bypass) {
+		xpsgtr_clr_set_phy(gtr_phy, L0_TM_DIG_6,
+				   L0_TM_DIS_DESCRAMBLE_DECODER,
+				   L0_TM_DIS_DESCRAMBLE_DECODER);
+		xpsgtr_clr_set_phy(gtr_phy, L0_TX_DIG_61,
+				   L0_TM_DISABLE_SCRAMBLE_ENCODER,
+				   L0_TM_DISABLE_SCRAMBLE_ENCODER);
+	} else {
+		xpsgtr_clr_set_phy(gtr_phy, L0_TM_DIG_6,
+				   L0_TM_DIS_DESCRAMBLE_DECODER, 0);
+		xpsgtr_clr_set_phy(gtr_phy, L0_TX_DIG_61,
+				   L0_TM_DISABLE_SCRAMBLE_ENCODER, 0);
+	}
 }
 
 /* DP-specific initialization. */
@@ -531,7 +546,7 @@ static void xpsgtr_phy_init_sata(struct xpsgtr_phy *gtr_phy)
 {
 	struct xpsgtr_dev *gtr_dev = gtr_phy->dev;
 
-	xpsgtr_bypass_scrambler_8b10b(gtr_phy);
+	xpsgtr_bypass_scrambler_8b10b(gtr_phy, true);
 
 	writel(gtr_phy->lane, gtr_dev->siou + SATA_CONTROL_OFFSET);
 }
@@ -547,7 +562,7 @@ static void xpsgtr_phy_init_sgmii(struct xpsgtr_phy *gtr_phy)
 	xpsgtr_clr_set(gtr_dev, TX_PROT_BUS_WIDTH, mask, val);
 	xpsgtr_clr_set(gtr_dev, RX_PROT_BUS_WIDTH, mask, val);
 
-	xpsgtr_bypass_scrambler_8b10b(gtr_phy);
+	xpsgtr_bypass_scrambler_8b10b(gtr_phy, true);
 }
 
 /* Configure TX de-emphasis and margining for DP. */
@@ -707,6 +722,10 @@ static int xpsgtr_phy_init(struct phy *phy)
 	case ICM_PROTOCOL_SGMII:
 		xpsgtr_phy_init_sgmii(gtr_phy);
 		break;
+
+	case ICM_PROTOCOL_USB:
+		xpsgtr_bypass_scrambler_8b10b(gtr_phy, false);
+		break;
 	}
 
 out:
-- 
2.43.0


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [RESEND PATCH 3/3] phy: zynqmp: keep SERDES scrambler and 8b/10b enabled for USB

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] PCIe and DisplayPort (DP) protocols lack explicit initialization of the scrambler/encoder bypass state, leaving them vulnerable to state pollution.
- [High] Silently ignores clock enable failure, masking the error and causing unbalanced clock disables later.
- [High] Bus width registers modified by SGMII are not reset by other protocols, risking state inheritance bugs.
--

commit e6d5f06ce31b70c624ec3e6a69c5b7e7ef55746a
Author: Nava kishore Manne <nava.kishore.manne@amd.com>
Subject: phy: zynqmp: keep SERDES scrambler and 8b/10b enabled for USB

This commit ensures that the scrambler and 8b/10b encoder/decoder are not
bypassed for USB operation, as mandated by the USB 3.x specification. The
bypass functionality is restricted to SATA and SGMII modes where encoding is
handled in the controller.

Cc: stable@vger.kernel.org

> diff --git a/drivers/phy/xilinx/phy-zynqmp.c b/drivers/phy/xilinx/phy-zynqmp.c
> index 6c56c4df8523c..087fe402e4e20 100644
> --- a/drivers/phy/xilinx/phy-zynqmp.c
> +++ b/drivers/phy/xilinx/phy-zynqmp.c
[ ... ]
> @@ -547,7 +562,7 @@ static void xpsgtr_phy_init_sgmii(struct xpsgtr_phy *gtr_phy)
>  	xpsgtr_clr_set(gtr_dev, TX_PROT_BUS_WIDTH, mask, val);
>  	xpsgtr_clr_set(gtr_dev, RX_PROT_BUS_WIDTH, mask, val);

[Severity: High]
This isn't a bug introduced by this patch, but SGMII alters the global bus
width registers here without other protocols guaranteeing they are restored to
default states.

If a lane previously used for SGMII is reassigned to another protocol (e.g.,
via warm reboot or bootloader handoff), will the 10-bit bus width setting
persist in the hardware and silently break other protocols (like USB or PCIe)
that expect their default bus widths?

>  
> -	xpsgtr_bypass_scrambler_8b10b(gtr_phy);
> +	xpsgtr_bypass_scrambler_8b10b(gtr_phy, true);
>  }
[ ... ]
> @@ -707,6 +722,10 @@ static int xpsgtr_phy_init(struct phy *phy)
>  	case ICM_PROTOCOL_SGMII:
>  		xpsgtr_phy_init_sgmii(gtr_phy);
>  		break;
> +
> +	case ICM_PROTOCOL_USB:
> +		xpsgtr_bypass_scrambler_8b10b(gtr_phy, false);
> +		break;

[Severity: High]
This isn't a bug introduced by this patch, but the switch statement in
xpsgtr_phy_init() misses explicit bypass configuration for DP and PCIe.

If a lane is dynamically reassigned or left bypassed by previous boot stages,
will DP and PCIe silently fail as they inherit a stale bypass = true state?

>  	}
>  
>  out:

[Severity: High]
This isn't a bug introduced by this patch, but looking at xpsgtr_phy_init(),
could the clock error path cause issues here?

drivers/phy/xilinx/phy-zynqmp.c:xpsgtr_phy_init() {
    int ret = 0;
    ...
    if (clk_prepare_enable(gtr_dev->clk[gtr_phy->refclk]))
        goto out;
    ...
out:
    mutex_unlock(&gtr_dev->gtr_mutex);
    return ret;
}

If clk_prepare_enable() fails, we branch to 'out', but 'ret' remains 0. Will
this silently ignore the clock enable failure, mask the error, and cause
unbalanced clock disables later when xpsgtr_phy_exit() calls
clk_disable_unprepare() on a clock that was never successfully enabled?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260627155229.2791113-1-radhey.shyam.pandey@amd.com?part=3

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy