Message-ID: <aKZLSSsEacH01suz@lizhi-Precision-Tower-5810>
Date: Wed, 20 Aug 2025 18:25:13 -0400
From: Frank Li <Frank.li@....com>
To: Bjorn Helgaas <helgaas@...nel.org>
Cc: Richard Zhu <hongxing.zhu@....com>, l.stach@...gutronix.de,
	lpieralisi@...nel.org, kwilczynski@...nel.org, mani@...nel.org,
	robh@...nel.org, bhelgaas@...gle.com, shawnguo@...nel.org,
	s.hauer@...gutronix.de, kernel@...gutronix.de, festevam@...il.com,
	linux-pci@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	imx@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 2/2] PCI: imx6: Add a method to handle CLKREQ#
 override active low

On Wed, Aug 20, 2025 at 04:40:10PM -0500, Bjorn Helgaas wrote:
> On Wed, Aug 20, 2025 at 04:10:48PM +0800, Richard Zhu wrote:
> > The CLKREQ# is an open drain, active low signal that is driven low by
> > the card to request reference clock.
> >
> > Since the reference clock may be required by i.MX PCIe host too. To make
> > sure this clock is available even when the CLKREQ# isn't driven low by
> > the card(e.x no card connected), force CLKREQ# override active low for
> > i.MX PCIe host during initialization.
> >
> > 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 in this case.
> >
> > Signed-off-by: Richard Zhu <hongxing.zhu@....com>
> > ---
> >  drivers/pci/controller/dwc/pci-imx6.c | 35 +++++++++++++++++++++++++++
> >  1 file changed, 35 insertions(+)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> > index 80e48746bbaf6..a73632b47e2d3 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;
> > @@ -267,6 +271,13 @@ static int imx95_pcie_init_phy(struct imx_pcie *imx_pcie)
> >  			   IMX95_PCIE_REF_CLKEN,
> >  			   IMX95_PCIE_REF_CLKEN);
> >
> > +	/* Force CLKREQ# low by override */
> > +	regmap_update_bits(imx_pcie->iomuxc_gpr,
> > +			   IMX95_PCIE_SS_RW_REG_1,
> > +			   IMX95_PCIE_CLKREQ_OVERRIDE_EN |
> > +			   IMX95_PCIE_CLKREQ_OVERRIDE_VAL,
> > +			   IMX95_PCIE_CLKREQ_OVERRIDE_EN |
> > +			   IMX95_PCIE_CLKREQ_OVERRIDE_VAL);
> >  	return 0;
> >  }
> >
> > @@ -1298,6 +1309,18 @@ static void imx_pcie_host_exit(struct dw_pcie_rp *pp)
> >  		regulator_disable(imx_pcie->vpcie);
> >  }
> >
> > +static void imx8mm_pcie_clr_clkreq_override(struct imx_pcie *imx_pcie)
> > +{
> > +	imx8mm_pcie_enable_ref_clk(imx_pcie, false);
> > +}
> > +
> > +static void imx95_pcie_clr_clkreq_override(struct imx_pcie *imx_pcie)
> > +{
> > +	regmap_update_bits(imx_pcie->iomuxc_gpr, IMX95_PCIE_SS_RW_REG_1,
> > +			   IMX95_PCIE_CLKREQ_OVERRIDE_EN |
> > +			   IMX95_PCIE_CLKREQ_OVERRIDE_VAL, 0);
> > +}
> > +
> >  static void imx_pcie_host_post_init(struct dw_pcie_rp *pp)
> >  {
> >  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > @@ -1322,6 +1345,12 @@ 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 +1774,8 @@ static int imx_pcie_probe(struct platform_device *pdev)
> >  	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");
>
> Is there a DT binding update that goes along with this?  I see
> "supports-clkreq" mentioned in
> Documentation/devicetree/bindings/pci/nvidia,tegra194-pcie.yaml, but
> nowhere else.

It is already defined dtschema/schemas/pci/pci-bus-common.yaml although I
like it should "clkreq-broken" because most system support it now.

>
> Should this be some kind of generic thing?  CLKREQ# itself is part of
> the PCIe base spec, so it's definitely not device-specific.
>
> But I'm not sure what this property is telling us.  Based on the
> patch:

It is quite long story about clkreq signal. The old version before L1SS

PCIe controller #CLKREQ (#CLKREQ_RC)
EP #CLKREQ (#CLKREQ_EP)

There are OR gate in board design OR(#CLKREQ_RC, #CLKREQ_EP) to controller
ref clk gate.

In this type board design, supports-clkreq should be set false.

After L1SS support, there are ECN at PCIe. Require connect RC and EP's #clkreq
(open drain) together and use one pull up register, see below diagram.

           VCC
            ─┬──
             │
             │
            ┌┴┐
            │ │
            │ │ 10K
            │ │
            └┬┘
┌────────┐   │ #clkreq┌───────────┐
│ RC     ┼───┼────────┼  EP       │
│        │   │        │           │
└────────┘   │        └───────────┘
             │
         ┌────────────┐
         │ CLK Gate   │
         │   │        │
         └───┴────────┘

So RC's phy part can power on/off by #clkreq signal beside ref clock gate.

Rollback more years, there are #clkreq signal at stardard pcie slots.
https://en.wikipedia.org/wiki/PCI_Express, PIN12 CLKREQ# is added by ECN
(maybe after miniPCIe). Before ECN, PIN12 is reversed.

>
>   - We assert CLKREQ# on IMX95 (and on IMX95_EP, which seems sort of
>     weird) regardless of "supports-clkreq".
>
>   - We call .clr_clkreq_override() to stop asserting CLKREQ# on
>     IMX8MQ, IMX8MM, IMX8MP, and IMX95 (but not IMX95_EP), but only if
>     they have "supports-clkreq" in devicetree.
>
> So I don't know whether or how CLKREQ# is asserted on IMX8MQ, IMX8MM,
> and IMX8MP.
>
> And I don't know why we assert CLKREQ# on IMX95_EP but apparently
> never stop asserting it.

It is default for EP by spec define. We have not support L1SS for EP side
yet. But it should not neccessary to overwrite it because IP default pull
clkreq to low when work at EP mode. but not do that at RC mode.

>
> And I don't know why we assert CLKREQ# regardless of "supports-clkreq"
> but only stop asserting it if "supports-clkreq" is present.

See above diagram, ref clk should be alway running after power up. clkreq
is power saving feature. Only board design match #clkreq ECN requirement,
we can enough such power saving feature.

Before enable L1SS, EP card always pull #clkreq to low. But there are
"broken" design, other from board or EP card, which product before ECN
public.

Frank

>
> >  	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 +1904,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 +1915,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 +1926,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,
> >  	},
> >  	[IMX8Q] = {
> >  		.variant = IMX8Q,
> > @@ -1913,6 +1947,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,
> >  	},
> >  	[IMX8MQ_EP] = {
> >  		.variant = IMX8MQ_EP,
> > --
> > 2.37.1
> >

Powered by blists - more mailing lists