[PATCH v1] PCI: imx6: Update MPLLB bandwidth for i.MX95 PCIe Gen3 stability

[PATCH v1] PCI: imx6: Update MPLLB bandwidth for i.MX95 PCIe Gen3 stability

[hongxing.zhu]

From: Richard Zhu <hongxing.zhu@nxp.com>

Bandwidth marginality was observed during i.MX95 Gen3 PCIe tests with
the default MPLLB_BINDWIDTH value. This margin degradation worsens
across voltage and temperature (VT) variations and different test
matrices, potentially causing link stability issues.

Testing with MPLLB_BINDWIDTH value of 140 (0x8c) shows significant
improvement in bandwidth margins across all VT conditions and test
scenarios.

Implement PHY register write helper function and configure:
- MPLLB_BW_OVRD_IN = 140 (0x8c) for improved bandwidth margin
- MPLLB_BW_OVRD_EN to enable the override

This ensures robust PCIe Gen3 performance across all operating
conditions.

Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
---
 drivers/pci/controller/dwc/pci-imx6.c | 31 +++++++++++++++++++++++----
 1 file changed, 27 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 6924a06bde305..023145b0dd2c7 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -80,6 +80,15 @@
 #define IMX95_SID_MASK				GENMASK(5, 0)
 #define IMX95_MAX_LUT				32
 
+#define IMX95_PCIE_PHY_REG_ADDR			0x3008
+#define IMX95_PCIE_PHY_REG_EN			BIT(31)
+#define IMX95_PCIE_PHY_REG_ADDR_MASK		GENMASK(15, 0)
+#define IMX95_PCIE_PHY_REG_DATA			0x300c
+#define IMX95_PCIE_PHY_MPLLB_OVRD_IN		0x2004
+#define IMX95_PCIE_PHY_MPLLB_BW_OVRD_EN		0x400
+#define IMX95_PCIE_PHY_MPLLB_BW_OVRD_IN		0x2005
+#define IMX95_PCIE_PHY_MPLLB_BW			0x8c
+
 #define IMX95_PCIE_RST_CTRL			0x3010
 #define IMX95_PCIE_COLD_RST			BIT(0)
 
@@ -269,6 +278,20 @@ static int imx95_pcie_select_ref_clk_src(struct imx_pcie *imx_pcie)
 	return 0;
 }
 
+static void imx95_pcie_phy_write(struct imx_pcie *imx_pcie, int addr, u16 data)
+{
+	regmap_update_bits(imx_pcie->iomuxc_gpr, IMX95_PCIE_SS_RW_REG_0,
+			   IMX95_PCIE_PHY_CR_PARA_SEL,
+			   IMX95_PCIE_PHY_CR_PARA_SEL);
+
+	fsleep(200);
+	regmap_update_bits(imx_pcie->iomuxc_gpr, IMX95_PCIE_PHY_REG_ADDR,
+			   IMX95_PCIE_PHY_REG_EN, IMX95_PCIE_PHY_REG_EN);
+	regmap_update_bits(imx_pcie->iomuxc_gpr, IMX95_PCIE_PHY_REG_ADDR,
+			   IMX95_PCIE_PHY_REG_ADDR_MASK, addr);
+	regmap_write(imx_pcie->iomuxc_gpr, IMX95_PCIE_PHY_REG_DATA, data);
+}
+
 static int imx95_pcie_init_phy(struct imx_pcie *imx_pcie)
 {
 	/*
@@ -284,10 +307,10 @@ static int imx95_pcie_init_phy(struct imx_pcie *imx_pcie)
 	regmap_set_bits(imx_pcie->iomuxc_gpr, IMX95_PCIE_SS_RW_REG_1,
 			IMX95_PCIE_SYS_AUX_PWR_DET);
 
-	regmap_update_bits(imx_pcie->iomuxc_gpr,
-			IMX95_PCIE_SS_RW_REG_0,
-			IMX95_PCIE_PHY_CR_PARA_SEL,
-			IMX95_PCIE_PHY_CR_PARA_SEL);
+	imx95_pcie_phy_write(imx_pcie, IMX95_PCIE_PHY_MPLLB_BW_OVRD_IN,
+			     IMX95_PCIE_PHY_MPLLB_BW);
+	imx95_pcie_phy_write(imx_pcie, IMX95_PCIE_PHY_MPLLB_OVRD_IN,
+			     IMX95_PCIE_PHY_MPLLB_BW_OVRD_EN);
 
 	return 0;
 }
-- 
2.34.1

Re: [PATCH v1] PCI: imx6: Update MPLLB bandwidth for i.MX95 PCIe Gen3 stability

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Sleep in atomic context (NOIRQ) during PHY initialization causes a kernel panic.
- [Medium] Non-atomic configuration of PHY register address and enable bits potentially triggering spurious hardware accesses.
- [Low] Redundant initialization and arbitrary delay (fsleep) executed on every PHY register write.
--

commit d6071da15c88ccfbb08be55ce1c9198ec88fd68c
Author: Richard Zhu <hongxing.zhu@nxp.com>

PCI: imx6: Update MPLLB bandwidth for i.MX95 PCIe Gen3 stability

Bandwidth marginality was observed during i.MX95 Gen3 PCIe tests with
the default MPLLB_BINDWIDTH value. This margin degradation worsens
across voltage and temperature (VT) variations and different test
matrices, potentially causing link stability issues.

> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 98e1db751132a..e65def1620441 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c

[ ... ]

> @@ -269,6 +278,20 @@ static int imx95_pcie_select_ref_clk_src(struct imx_pcie *imx_pcie)
>  	return 0;
>  }
>  
> +static void imx95_pcie_phy_write(struct imx_pcie *imx_pcie, int addr, u16 data)
> +{
> +	regmap_update_bits(imx_pcie->iomuxc_gpr, IMX95_PCIE_SS_RW_REG_0,
> +			   IMX95_PCIE_PHY_CR_PARA_SEL,
> +			   IMX95_PCIE_PHY_CR_PARA_SEL);
> +
> +	fsleep(200);

[Severity: Low]
Does this execute redundant initialization and an arbitrary delay on every single
PHY register write?

Because imx95_pcie_init_phy() calls imx95_pcie_phy_write() twice, this
IMX95_PCIE_PHY_CR_PARA_SEL configuration and 200us delay are performed on every
register write.

Could this be a one-time initialization prior to the write sequence rather than
per-write overhead?


[Severity: High]
Does this fsleep() cause a sleep-in-atomic bug during system resume?

The imx_pcie_resume_noirq() callback runs with local interrupts disabled, and it
calls dw_pcie_resume_noirq() -> imx_pcie_host_init() -> imx95_pcie_init_phy() ->
imx95_pcie_phy_write().

For 200us, fsleep() resolves to usleep_range() which will attempt to schedule
a sleep in this atomic context.

Should this use udelay(200) instead to safely wait?

> +	regmap_update_bits(imx_pcie->iomuxc_gpr, IMX95_PCIE_PHY_REG_ADDR,
> +			   IMX95_PCIE_PHY_REG_EN, IMX95_PCIE_PHY_REG_EN);
> +	regmap_update_bits(imx_pcie->iomuxc_gpr, IMX95_PCIE_PHY_REG_ADDR,
> +			   IMX95_PCIE_PHY_REG_ADDR_MASK, addr);

[Severity: Medium]
Will this sequence trigger a spurious hardware access using the old address?

By setting IMX95_PCIE_PHY_REG_EN in IMX95_PCIE_PHY_REG_ADDR before updating the
target address (IMX95_PCIE_PHY_REG_ADDR_MASK), the hardware might trigger an
access immediately upon the enable bit being set, using whatever old address was
left over in the register.

Should these bits be updated atomically in a single regmap_update_bits() call?

> +	regmap_write(imx_pcie->iomuxc_gpr, IMX95_PCIE_PHY_REG_DATA, data);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260706070340.3603150-1-hongxing.zhu@oss.nxp.com?part=1

Re: [PATCH v1] PCI: imx6: Update MPLLB bandwidth for i.MX95 PCIe Gen3 stability

[Frank Li]

On Mon, Jul 06, 2026 at 03:03:40PM +0800, hongxing.zhu@oss.nxp.com wrote:
> From: Richard Zhu <hongxing.zhu@nxp.com>

subject:
PCI: imx6: Update MPLLB bandwidth to improve i.MX95 Gen3 PCIe stability

>
> Bandwidth marginality was observed during i.MX95 Gen3 PCIe tests with
> the default MPLLB_BINDWIDTH value. This margin degradation worsens
> across voltage and temperature (VT) variations and different test
> matrices, potentially causing link stability issues.
>
> Testing with MPLLB_BINDWIDTH value of 140 (0x8c) shows significant
> improvement in bandwidth margins across all VT conditions and test
> scenarios.
>
> Implement PHY register write helper function and configure:
> - MPLLB_BW_OVRD_IN = 140 (0x8c) for improved bandwidth margin
> - MPLLB_BW_OVRD_EN to enable the override
>
> This ensures robust PCIe Gen3 performance across all operating
> conditions.
>
> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> ---
>  drivers/pci/controller/dwc/pci-imx6.c | 31 +++++++++++++++++++++++----
>  1 file changed, 27 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 6924a06bde305..023145b0dd2c7 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -80,6 +80,15 @@
>  #define IMX95_SID_MASK				GENMASK(5, 0)
>  #define IMX95_MAX_LUT				32
>
> +#define IMX95_PCIE_PHY_REG_ADDR			0x3008
> +#define IMX95_PCIE_PHY_REG_EN			BIT(31)
> +#define IMX95_PCIE_PHY_REG_ADDR_MASK		GENMASK(15, 0)
> +#define IMX95_PCIE_PHY_REG_DATA			0x300c
> +#define IMX95_PCIE_PHY_MPLLB_OVRD_IN		0x2004
> +#define IMX95_PCIE_PHY_MPLLB_BW_OVRD_EN		0x400

Not sure how spec descript it. but BW_OVRD_EN is bit of OVRD_IN register

look like IMX95_PCIE_PHY_MPLLB_OVRD_BW_EN is more clean to avoid confuse
it is a bit of register BW_OVRD_IN.

> +#define IMX95_PCIE_PHY_MPLLB_BW_OVRD_IN		0x2005
> +#define IMX95_PCIE_PHY_MPLLB_BW			0x8c
> +
>  #define IMX95_PCIE_RST_CTRL			0x3010
>  #define IMX95_PCIE_COLD_RST			BIT(0)
>
> @@ -269,6 +278,20 @@ static int imx95_pcie_select_ref_clk_src(struct imx_pcie *imx_pcie)
>  	return 0;
>  }
>
> +static void imx95_pcie_phy_write(struct imx_pcie *imx_pcie, int addr, u16 data)
> +{
> +	regmap_update_bits(imx_pcie->iomuxc_gpr, IMX95_PCIE_SS_RW_REG_0,
> +			   IMX95_PCIE_PHY_CR_PARA_SEL,
> +			   IMX95_PCIE_PHY_CR_PARA_SEL);
> +
> +	fsleep(200);
> +	regmap_update_bits(imx_pcie->iomuxc_gpr, IMX95_PCIE_PHY_REG_ADDR,
> +			   IMX95_PCIE_PHY_REG_EN, IMX95_PCIE_PHY_REG_EN);
> +	regmap_update_bits(imx_pcie->iomuxc_gpr, IMX95_PCIE_PHY_REG_ADDR,
> +			   IMX95_PCIE_PHY_REG_ADDR_MASK, addr);
> +	regmap_write(imx_pcie->iomuxc_gpr, IMX95_PCIE_PHY_REG_DATA, data);
> +}
> +
>  static int imx95_pcie_init_phy(struct imx_pcie *imx_pcie)
>  {
>  	/*
> @@ -284,10 +307,10 @@ static int imx95_pcie_init_phy(struct imx_pcie *imx_pcie)
>  	regmap_set_bits(imx_pcie->iomuxc_gpr, IMX95_PCIE_SS_RW_REG_1,
>  			IMX95_PCIE_SYS_AUX_PWR_DET);
>
> -	regmap_update_bits(imx_pcie->iomuxc_gpr,
> -			IMX95_PCIE_SS_RW_REG_0,
> -			IMX95_PCIE_PHY_CR_PARA_SEL,
> -			IMX95_PCIE_PHY_CR_PARA_SEL);

why need remove this part?

Frank

> +	imx95_pcie_phy_write(imx_pcie, IMX95_PCIE_PHY_MPLLB_BW_OVRD_IN,
> +			     IMX95_PCIE_PHY_MPLLB_BW);
> +	imx95_pcie_phy_write(imx_pcie, IMX95_PCIE_PHY_MPLLB_OVRD_IN,
> +			     IMX95_PCIE_PHY_MPLLB_BW_OVRD_EN);
>
>  	return 0;
>  }
> --
> 2.34.1
>
>