linux-kernel - Re: [PATCH v3 2/2] PCI: imx6: Add a method to handle CLKREQ# override active low

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <hsmebnz6opoj45zztdd2svmdtrwwwrngjaidpltbunnkdvvdqz@lhyejtlwkkes>
Date: Mon, 22 Sep 2025 11:42:36 +0530
From: Manivannan Sadhasivam <mani@...nel.org>
To: Richard Zhu <hongxing.zhu@....com>
Cc: frank.li@....com, jingoohan1@...il.com, l.stach@...gutronix.de, 
	lpieralisi@...nel.org, kwilczynski@...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 v3 2/2] PCI: imx6: Add a method to handle CLKREQ#
 override active low

On Mon, Sep 22, 2025 at 10:37:41AM +0800, Richard Zhu wrote:
> 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.
> 
> 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@....com>
> Reviewed-by: Frank Li <Frank.Li@....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 80e48746bbaf..a73632b47e2d 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);

This should be:

	imx95_pcie_clkreq_override(imx_pcie, true);

refer below...

>  	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);

Just noticed this discrepancy. 'imx8mm_pcie_enable_ref_clk(, false)' is enabling
the CLKREQ# override, thereby enabling the refclk. But only for imx8mm, this
helper is called as imx8mm_pcie_enable_ref_clk(). But for imx95, the equivalent
function is called as imx95_pcie_clr_clkreq_override(). This is causing
confusion.

Maybe you should just call both functions as:

	imx8mm_pcie_clkreq_override(imx_pcie, bool enable);
	imx95_pcie_clkreq_override(imx_pcie, bool enable);

Then,

	imx8mm_pcie_clr_clkreq_override(struct imx_pcie *imx_pcie)
	{
		imx8mm_pcie_clkreq_override(imx_pcie, false)
	}

	imx95_pcie_clr_clkreq_override(struct imx_pcie *imx_pcie)
	{
		imx95_pcie_clkreq_override(imx_pcie, false)
	}

and populate the clr_clkreq_override() callback.

- Mani

-- 
மணிவண்ணன் சதாசிவம்