-----Original Message-----
From: Manivannan Sadhasivam <redacted>
Sent: 2025年1月19日 15:03
To: Hongxing Zhu <hongxing.zhu@nxp.com>
Cc: l.stach@pengutronix.de; bhelgaas@google.com; lpieralisi@kernel.org;
kw@linux.com; robh@kernel.org; krzk+dt@kernel.org; conor+dt@kernel.org;
shawnguo@kernel.org; Frank Li [off-list ref]; s.hauer@pengutronix.de;
festevam@gmail.com; imx@lists.linux.dev; kernel@pengutronix.de;
linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH v7 02/10] PCI: imx6: Add ref clock for i.MX95 PCIe

On Tue, Nov 26, 2024 at 03:56:54PM +0800, Richard Zhu wrote:

quoted

Add "ref" clock to enable reference clock. To avoid breaking DT
backwards compatibility, i.MX95 REF clock might be optional. Use
devm_clk_get_optional() to fetch i.MX95 PCIe optional clocks in driver.

If use external clock, ref clock should point to external reference.

If use internal clock, CREF_EN in LAST_TO_REG controls reference
output, which implement in drivers/clk/imx/clk-imx95-blk-ctl.c.

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

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

b/drivers/pci/controller/dwc/pci-imx6.c
index 385f6323e3ca..f7e928e0a018 100644

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

@@ -103,6 +103,7 @@ struct imx_pcie_drvdata {
 	const char *gpr;
 	const char * const *clk_names;
 	const u32 clks_cnt;
+	const u32 clks_optional_cnt;
 	const u32 ltssm_off;
 	const u32 ltssm_mask;
 	const u32 mode_off[IMX_PCIE_MAX_INSTANCES]; @@ -1306,9 +1307,8

@@

quoted

static int imx_pcie_probe(struct platform_device *pdev)
 	struct device_node *np;
 	struct resource *dbi_base;
 	struct device_node *node = dev->of_node;
-	int ret;
+	int i, ret, req_cnt;
 	u16 val;
-	int i;

 	imx_pcie = devm_kzalloc(dev, sizeof(*imx_pcie), GFP_KERNEL);
 	if (!imx_pcie)

@@ -1358,9 +1358,13 @@ static int imx_pcie_probe(struct platform_device

*pdev)

quoted

 		imx_pcie->clks[i].id = imx_pcie->drvdata->clk_names[i];

 	/* Fetch clocks */
-	ret = devm_clk_bulk_get(dev, imx_pcie->drvdata->clks_cnt, imx_pcie->clks);
+	req_cnt = imx_pcie->drvdata->clks_cnt -

imx_pcie->drvdata->clks_optional_cnt;

quoted

+	ret = devm_clk_bulk_get(dev, req_cnt, imx_pcie->clks);
 	if (ret)
 		return ret;
+	imx_pcie->clks[req_cnt].clk = devm_clk_get_optional(dev, "ref");
+	if (IS_ERR(imx_pcie->clks[req_cnt].clk))
+		return PTR_ERR(imx_pcie->clks[req_cnt].clk);

I think you should just switch to devm_clk_bulk_get_all() instead of getting the
clks separately. As I told previously, the DT binding should ensure that correct
clocks for the platforms are defined in DT and the driver has no business in
validating it. Driver should trust the DT instead (unless there is a valid reason to not
do so).

quoted

 	if (imx_check_flag(imx_pcie, IMX_PCIE_FLAG_HAS_PHYDRV)) {
 		imx_pcie->phy = devm_phy_get(dev, "pcie-phy"); @@ -1509,6 +1513,7

@@ static const char * const imx8mm_clks[] = {"pcie_bus", "pcie",

"pcie_aux"};  static const char * const imx8mq_clks[] = {"pcie_bus",
"pcie", "pcie_phy", "pcie_aux"};  static const char * const
imx6sx_clks[] = {"pcie_bus", "pcie", "pcie_phy", "pcie_inbound_axi"};
static const char * const imx8q_clks[] = {"mstr", "slv", "dbi"};
+static const char * const imx95_clks[] = {"pcie_bus", "pcie",
+"pcie_phy", "pcie_aux", "ref"};

And these static clock defines will go away too.

Hi Mani:
Thanks for your comments.
The suggestions are very nice. How about kick off further optimization later?
 Since the changes would impact all i.MX PCIes.
Meanwhile, I'm a little worry about that there is no consensus yet on relying
 entirely on the dt binding check.

Best Regards
Richard Zhu

- Mani

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