-----Original Message-----
From: Bjorn Helgaas <helgaas@kernel.org>
Sent: 2025年9月26日 5:58
To: Hongxing Zhu <hongxing.zhu@nxp.com>
Cc: Frank Li <frank.li@nxp.com>; jingoohan1@gmail.com;
l.stach@pengutronix.de; lpieralisi@kernel.org; kwilczynski@kernel.org;
mani@kernel.org; robh@kernel.org; bhelgaas@google.com;
shawnguo@kernel.org; s.hauer@pengutronix.de; kernel@pengutronix.de;
festevam@gmail.com; linux-pci@vger.kernel.org;
linux-arm-kernel@lists.infradead.org; imx@lists.linux.dev;
linux-kernel@vger.kernel.org
Subject: Re: [PATCH v5 2/2] PCI: imx6: Add a method to handle CLKREQ#
override active low

On Tue, Sep 23, 2025 at 03:39:13PM +0800, Richard Zhu wrote:

quoted

The CLKREQ# is an open drain, active low signal that is driven low by
the card to request reference clock. It's an optional signal added in
PCIe CEM r4.0, sec 2. Thus, this signal wouldn't be driven low if it's
reserved.

Since the reference clock controlled by CLKREQ# may be required by
i.MX PCIe host too. To make sure this clock is ready even when the
CLKREQ# isn't driven low by the card(e.x the scenario described
above), force CLKREQ# override active low for i.MX PCIe host during

initialization.

quoted

The CLKREQ# override can be cleared safely when supports-clkreq is
present and PCIe link is up later. Because the CLKREQ# would be driven
low by the card at this time.

Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/pci/controller/dwc/pci-imx6.c | 43
++++++++++++++++++++++++++-
 1 file changed, 42 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c

b/drivers/pci/controller/dwc/pci-imx6.c
index 80e48746bbaf..6b03b1111d06 100644

--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c

@@ -52,6 +52,8 @@
 #define IMX95_PCIE_REF_CLKEN			BIT(23)
 #define IMX95_PCIE_PHY_CR_PARA_SEL		BIT(9)
 #define IMX95_PCIE_SS_RW_REG_1			0xf4
+#define IMX95_PCIE_CLKREQ_OVERRIDE_EN		BIT(8)
+#define IMX95_PCIE_CLKREQ_OVERRIDE_VAL		BIT(9)
 #define IMX95_PCIE_SYS_AUX_PWR_DET		BIT(31)

 #define IMX95_PE0_GEN_CTRL_1			0x1050

@@ -136,6 +138,7 @@ struct imx_pcie_drvdata {
 	int (*enable_ref_clk)(struct imx_pcie *pcie, bool enable);
 	int (*core_reset)(struct imx_pcie *pcie, bool assert);
 	int (*wait_pll_lock)(struct imx_pcie *pcie);
+	void (*clr_clkreq_override)(struct imx_pcie *pcie);
 	const struct dw_pcie_host_ops *ops;
 };

@@ -149,6 +152,7 @@ struct imx_pcie {
 	struct gpio_desc	*reset_gpiod;
 	struct clk_bulk_data	*clks;
 	int			num_clks;
+	bool			supports_clkreq;
 	struct regmap		*iomuxc_gpr;
 	u16			msi_ctrl;
 	u32			controller_id;

@@ -239,6 +243,16 @@ static unsigned int imx_pcie_grp_offset(const struct

imx_pcie *imx_pcie)

quoted

 	return imx_pcie->controller_id == 1 ? IOMUXC_GPR16 : IOMUXC_GPR14;
}

+static void  imx95_pcie_clkreq_override(struct imx_pcie *imx_pcie,
+bool enable) {
+	regmap_update_bits(imx_pcie->iomuxc_gpr, IMX95_PCIE_SS_RW_REG_1,
+			   IMX95_PCIE_CLKREQ_OVERRIDE_EN,
+			   enable ? IMX95_PCIE_CLKREQ_OVERRIDE_EN : 0);
+	regmap_update_bits(imx_pcie->iomuxc_gpr, IMX95_PCIE_SS_RW_REG_1,
+			   IMX95_PCIE_CLKREQ_OVERRIDE_VAL,
+			   enable ? IMX95_PCIE_CLKREQ_OVERRIDE_VAL : 0); }
+
 static int imx95_pcie_init_phy(struct imx_pcie *imx_pcie)  {
 	/*

@@ -685,7 +699,7 @@ static int imx6q_pcie_enable_ref_clk(struct imx_pcie

*imx_pcie, bool enable)

quoted

 	return 0;
 }

-static int imx8mm_pcie_enable_ref_clk(struct imx_pcie *imx_pcie, bool
enable)
+static void imx8mm_pcie_clkreq_override(struct imx_pcie *imx_pcie,
+bool enable)
 {
 	int offset = imx_pcie_grp_offset(imx_pcie);

@@ -695,6 +709,11 @@ static int imx8mm_pcie_enable_ref_clk(struct

imx_pcie *imx_pcie, bool enable)

quoted

 	regmap_update_bits(imx_pcie->iomuxc_gpr, offset,
 			   IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE_EN,
 			   enable ? IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE_EN : 0);
+}
+
+static int imx8mm_pcie_enable_ref_clk(struct imx_pcie *imx_pcie, bool
+enable) {
+	imx8mm_pcie_clkreq_override(imx_pcie, enable);
 	return 0;
 }

@@ -1298,6 +1317,16 @@ static void imx_pcie_host_exit(struct dw_pcie_rp

*pp)

quoted

 		regulator_disable(imx_pcie->vpcie);
 }

+static void imx8mm_pcie_clr_clkreq_override(struct imx_pcie
+*imx_pcie) {
+	imx8mm_pcie_clkreq_override(imx_pcie, false); }
+
+static void imx95_pcie_clr_clkreq_override(struct imx_pcie *imx_pcie)
+{
+	imx95_pcie_clkreq_override(imx_pcie, false); }
+
 static void imx_pcie_host_post_init(struct dw_pcie_rp *pp)  {
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp); @@ -1322,6 +1351,12

@@

quoted

static void imx_pcie_host_post_init(struct dw_pcie_rp *pp)
 		dw_pcie_writel_dbi(pci, GEN3_RELATED_OFF, val);
 		dw_pcie_dbi_ro_wr_dis(pci);
 	}
+
+	/* Clear CLKREQ# override if supports_clkreq is true and link is up */
+	if (dw_pcie_link_up(pci) && imx_pcie->supports_clkreq) {
+		if (imx_pcie->drvdata->clr_clkreq_override)
+			imx_pcie->drvdata->clr_clkreq_override(imx_pcie);
+	}
 }

 /*

@@ -1745,6 +1780,8 @@ static int imx_pcie_probe(struct platform_device

*pdev)

quoted

 	pci->max_link_speed = 1;
 	of_property_read_u32(node, "fsl,max-link-speed",
&pci->max_link_speed);

+	imx_pcie->supports_clkreq =
+		of_property_read_bool(node, "supports-clkreq");
 	imx_pcie->vpcie = devm_regulator_get_optional(&pdev->dev, "vpcie");
 	if (IS_ERR(imx_pcie->vpcie)) {
 		if (PTR_ERR(imx_pcie->vpcie) != -ENODEV) @@ -1873,6 +1910,7 @@
static const struct imx_pcie_drvdata drvdata[] = {
 		.mode_mask[1] = IMX8MQ_GPR12_PCIE2_CTRL_DEVICE_TYPE,
 		.init_phy = imx8mq_pcie_init_phy,
 		.enable_ref_clk = imx8mm_pcie_enable_ref_clk,
+		.clr_clkreq_override = imx8mm_pcie_clr_clkreq_override,
 	},
 	[IMX8MM] = {
 		.variant = IMX8MM,

@@ -1883,6 +1921,7 @@ static const struct imx_pcie_drvdata drvdata[] = {
 		.mode_off[0] = IOMUXC_GPR12,
 		.mode_mask[0] = IMX6Q_GPR12_DEVICE_TYPE,
 		.enable_ref_clk = imx8mm_pcie_enable_ref_clk,
+		.clr_clkreq_override = imx8mm_pcie_clr_clkreq_override,
 	},
 	[IMX8MP] = {
 		.variant = IMX8MP,

@@ -1893,6 +1932,7 @@ static const struct imx_pcie_drvdata drvdata[] = {
 		.mode_off[0] = IOMUXC_GPR12,
 		.mode_mask[0] = IMX6Q_GPR12_DEVICE_TYPE,
 		.enable_ref_clk = imx8mm_pcie_enable_ref_clk,
+		.clr_clkreq_override = imx8mm_pcie_clr_clkreq_override,

imx8mm_pcie_enable_ref_clk() and imx8mm_pcie_clr_clkreq_override() both
call imx8mm_pcie_clkreq_override().

All these devices that use imx8mm_pcie_clr_clkreq_override() also enable the
refclk in .init() by overriding clkreq, so .post_init() clears that override.

On i.MX8M, the override clkreq# operations are wrapped as .enable_ref_clk().
This would block L1ss feature enablement, if the override active low is not
 cleared. This patch-set paves the path to support L1ss feature on i.MX8M PCIes.

quoted

 	},
 	[IMX8Q] = {
 		.variant = IMX8Q,

@@ -1913,6 +1953,7 @@ static const struct imx_pcie_drvdata drvdata[] = {
 		.core_reset = imx95_pcie_core_reset,
 		.init_phy = imx95_pcie_init_phy,
 		.wait_pll_lock = imx95_pcie_wait_for_phy_pll_lock,
+		.clr_clkreq_override = imx95_pcie_clr_clkreq_override,

But here there's no .enable_ref_clk() method, so I guess this device must
override clkreq automatically and the driver doesn't have to enable that?

Wow, it's my fault that I made a mistake when sent out the v4 after v3 review
 around. "imx95_pcie_clkreq_override(imx_pcie, true);" should be added when I
 pack the codes into a new function in v4.

https://lore.kernel.org/imx/hsmebnz6opoj45zztdd2svmdtrwwwrngjaidpltbunnkdvvdqz@lhyejtlwkkes/ (local)

--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c

@@ -281,6 +281,8 @@ static int imx95_pcie_init_phy(struct imx_pcie *imx_pcie)
                           IMX95_PCIE_REF_CLKEN,
                           IMX95_PCIE_REF_CLKEN);

+       imx95_pcie_clkreq_override(imx_pcie, true);
+

Thanks a lot for your carefully review here.

Hi Mani:
The v5 patch-set miss the changes listed above, please backout the applied v5
 patch-set. Should I re-post next version?
Sorry to bring inconvenience to you.

Best Regards
Richard Zhu

quoted

 	},
 	[IMX8MQ_EP] = {
 		.variant = IMX8MQ_EP,
--
2.37.1