[PATCH] phy: xilinx: zynqmp: Fix bus width setting for SGMII

[PATCH] phy: xilinx: zynqmp: Fix bus width setting for SGMII

[Robert Hancock]

TX_PROT_BUS_WIDTH and RX_PROT_BUS_WIDTH are single registers with
separate bit fields for each lane. The code in xpsgtr_phy_init_sgmii was
not preserving the existing register value for other lanes, so enabling
the PHY in SGMII mode on one lane zeroed out the settings for all other
lanes, causing other PS-GTR peripherals such as USB3 to malfunction.

Use xpsgtr_clr_set to only manipulate the desired bits in the register.

Fixes: 4a33bea00314 ("phy: zynqmp: Add PHY driver for the Xilinx ZynqMP Gigabit Transceiver")
Signed-off-by: Robert Hancock <robert.hancock@calian.com>
---
 drivers/phy/xilinx/phy-zynqmp.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/phy/xilinx/phy-zynqmp.c b/drivers/phy/xilinx/phy-zynqmp.c
index f478d8a17115..9be9535ad7ab 100644
--- a/drivers/phy/xilinx/phy-zynqmp.c
+++ b/drivers/phy/xilinx/phy-zynqmp.c
@@ -134,7 +134,8 @@
 #define PROT_BUS_WIDTH_10		0x0
 #define PROT_BUS_WIDTH_20		0x1
 #define PROT_BUS_WIDTH_40		0x2
-#define PROT_BUS_WIDTH_SHIFT		2
+#define PROT_BUS_WIDTH_SHIFT(n)		((n) * 2)
+#define PROT_BUS_WIDTH_MASK(n)		GENMASK((n) * 2 + 1, (n) * 2)
 
 /* Number of GT lanes */
 #define NUM_LANES			4
@@ -445,12 +446,12 @@ static void xpsgtr_phy_init_sata(struct xpsgtr_phy *gtr_phy)
 static void xpsgtr_phy_init_sgmii(struct xpsgtr_phy *gtr_phy)
 {
 	struct xpsgtr_dev *gtr_dev = gtr_phy->dev;
+	u32 mask = PROT_BUS_WIDTH_MASK(gtr_phy->lane);
+	u32 val = PROT_BUS_WIDTH_10 << PROT_BUS_WIDTH_SHIFT(gtr_phy->lane);
 
 	/* Set SGMII protocol TX and RX bus width to 10 bits. */
-	xpsgtr_write(gtr_dev, TX_PROT_BUS_WIDTH,
-		     PROT_BUS_WIDTH_10 << (gtr_phy->lane * PROT_BUS_WIDTH_SHIFT));
-	xpsgtr_write(gtr_dev, RX_PROT_BUS_WIDTH,
-		     PROT_BUS_WIDTH_10 << (gtr_phy->lane * PROT_BUS_WIDTH_SHIFT));
+	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);
 }
-- 
2.31.1


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

Re: [PATCH] phy: xilinx: zynqmp: Fix bus width setting for SGMII

[Laurent Pinchart]

Hi Robert,

Thank you for the patch.

On Tue, Jan 25, 2022 at 06:16:00PM -0600, Robert Hancock wrote:
> TX_PROT_BUS_WIDTH and RX_PROT_BUS_WIDTH are single registers with
> separate bit fields for each lane. The code in xpsgtr_phy_init_sgmii was
> not preserving the existing register value for other lanes, so enabling
> the PHY in SGMII mode on one lane zeroed out the settings for all other
> lanes, causing other PS-GTR peripherals such as USB3 to malfunction.
> 
> Use xpsgtr_clr_set to only manipulate the desired bits in the register.
> 
> Fixes: 4a33bea00314 ("phy: zynqmp: Add PHY driver for the Xilinx ZynqMP Gigabit Transceiver")
> Signed-off-by: Robert Hancock <robert.hancock@calian.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/phy/xilinx/phy-zynqmp.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/phy/xilinx/phy-zynqmp.c b/drivers/phy/xilinx/phy-zynqmp.c
> index f478d8a17115..9be9535ad7ab 100644
> --- a/drivers/phy/xilinx/phy-zynqmp.c
> +++ b/drivers/phy/xilinx/phy-zynqmp.c
> @@ -134,7 +134,8 @@
>  #define PROT_BUS_WIDTH_10		0x0
>  #define PROT_BUS_WIDTH_20		0x1
>  #define PROT_BUS_WIDTH_40		0x2
> -#define PROT_BUS_WIDTH_SHIFT		2
> +#define PROT_BUS_WIDTH_SHIFT(n)		((n) * 2)
> +#define PROT_BUS_WIDTH_MASK(n)		GENMASK((n) * 2 + 1, (n) * 2)
>  
>  /* Number of GT lanes */
>  #define NUM_LANES			4
> @@ -445,12 +446,12 @@ static void xpsgtr_phy_init_sata(struct xpsgtr_phy *gtr_phy)
>  static void xpsgtr_phy_init_sgmii(struct xpsgtr_phy *gtr_phy)
>  {
>  	struct xpsgtr_dev *gtr_dev = gtr_phy->dev;
> +	u32 mask = PROT_BUS_WIDTH_MASK(gtr_phy->lane);
> +	u32 val = PROT_BUS_WIDTH_10 << PROT_BUS_WIDTH_SHIFT(gtr_phy->lane);
>  
>  	/* Set SGMII protocol TX and RX bus width to 10 bits. */
> -	xpsgtr_write(gtr_dev, TX_PROT_BUS_WIDTH,
> -		     PROT_BUS_WIDTH_10 << (gtr_phy->lane * PROT_BUS_WIDTH_SHIFT));
> -	xpsgtr_write(gtr_dev, RX_PROT_BUS_WIDTH,
> -		     PROT_BUS_WIDTH_10 << (gtr_phy->lane * PROT_BUS_WIDTH_SHIFT));
> +	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);
>  }

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH] phy: xilinx: zynqmp: Fix bus width setting for SGMII

[Michal Simek]

On 1/26/22 01:16, Robert Hancock wrote:
> TX_PROT_BUS_WIDTH and RX_PROT_BUS_WIDTH are single registers with
> separate bit fields for each lane. The code in xpsgtr_phy_init_sgmii was
> not preserving the existing register value for other lanes, so enabling
> the PHY in SGMII mode on one lane zeroed out the settings for all other
> lanes, causing other PS-GTR peripherals such as USB3 to malfunction.
> 
> Use xpsgtr_clr_set to only manipulate the desired bits in the register.
> 
> Fixes: 4a33bea00314 ("phy: zynqmp: Add PHY driver for the Xilinx ZynqMP Gigabit Transceiver")
> Signed-off-by: Robert Hancock <robert.hancock@calian.com>
> ---
>   drivers/phy/xilinx/phy-zynqmp.c | 11 ++++++-----
>   1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/phy/xilinx/phy-zynqmp.c b/drivers/phy/xilinx/phy-zynqmp.c
> index f478d8a17115..9be9535ad7ab 100644
> --- a/drivers/phy/xilinx/phy-zynqmp.c
> +++ b/drivers/phy/xilinx/phy-zynqmp.c
> @@ -134,7 +134,8 @@
>   #define PROT_BUS_WIDTH_10		0x0
>   #define PROT_BUS_WIDTH_20		0x1
>   #define PROT_BUS_WIDTH_40		0x2
> -#define PROT_BUS_WIDTH_SHIFT		2
> +#define PROT_BUS_WIDTH_SHIFT(n)		((n) * 2)
> +#define PROT_BUS_WIDTH_MASK(n)		GENMASK((n) * 2 + 1, (n) * 2)
>   
>   /* Number of GT lanes */
>   #define NUM_LANES			4
> @@ -445,12 +446,12 @@ static void xpsgtr_phy_init_sata(struct xpsgtr_phy *gtr_phy)
>   static void xpsgtr_phy_init_sgmii(struct xpsgtr_phy *gtr_phy)
>   {
>   	struct xpsgtr_dev *gtr_dev = gtr_phy->dev;
> +	u32 mask = PROT_BUS_WIDTH_MASK(gtr_phy->lane);
> +	u32 val = PROT_BUS_WIDTH_10 << PROT_BUS_WIDTH_SHIFT(gtr_phy->lane);
>   
>   	/* Set SGMII protocol TX and RX bus width to 10 bits. */
> -	xpsgtr_write(gtr_dev, TX_PROT_BUS_WIDTH,
> -		     PROT_BUS_WIDTH_10 << (gtr_phy->lane * PROT_BUS_WIDTH_SHIFT));
> -	xpsgtr_write(gtr_dev, RX_PROT_BUS_WIDTH,
> -		     PROT_BUS_WIDTH_10 << (gtr_phy->lane * PROT_BUS_WIDTH_SHIFT));
> +	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);
>   }

I have found this issue while I was developing u-boot driver for the same.

Acked-by: Michal Simek <michal.simek@xilinx.com>

Thanks,
Michal



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

Re: [PATCH] phy: xilinx: zynqmp: Fix bus width setting for SGMII

[Vinod Koul]

On 25-01-22, 18:16, Robert Hancock wrote:
> TX_PROT_BUS_WIDTH and RX_PROT_BUS_WIDTH are single registers with
> separate bit fields for each lane. The code in xpsgtr_phy_init_sgmii was
> not preserving the existing register value for other lanes, so enabling
> the PHY in SGMII mode on one lane zeroed out the settings for all other
> lanes, causing other PS-GTR peripherals such as USB3 to malfunction.
> 
> Use xpsgtr_clr_set to only manipulate the desired bits in the register.

Applied, thanks

-- 
~Vinod

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