Message ID | 20241101070610.1267391-3-hongxing.zhu@nxp.com (mailing list archive) |
---|---|
State | New |
Headers | show |
Series | A bunch of changes to refine i.MX PCIe driver | expand |
On Fri, Nov 01, 2024 at 03:06:02PM +0800, Richard Zhu wrote: > Add "ref" clock to enable reference clock. To avoid the DT > compatibility, i.MX95 REF clock might be optional. Your wording is not correct. Perhaps you wanted to say, "To avoid breaking DT backwards compatibility"? > Replace the > devm_clk_bulk_get() by devm_clk_bulk_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 | 19 +++++++++++++------ > 1 file changed, 13 insertions(+), 6 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c > index 808d1f105417..bc8567677a67 100644 > --- a/drivers/pci/controller/dwc/pci-imx6.c > +++ b/drivers/pci/controller/dwc/pci-imx6.c > @@ -82,6 +82,7 @@ enum imx_pcie_variants { > #define IMX_PCIE_FLAG_HAS_SERDES BIT(6) > #define IMX_PCIE_FLAG_SUPPORT_64BIT BIT(7) > #define IMX_PCIE_FLAG_CPU_ADDR_FIXUP BIT(8) > +#define IMX_PCIE_FLAG_CUSTOM_PME_TURNOFF BIT(9) > > #define imx_check_flag(pci, val) (pci->drvdata->flags & val) > > @@ -98,6 +99,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]; > @@ -1278,9 +1280,8 @@ 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 ret, i, req_cnt; > u16 val; > - int i; > > imx_pcie = devm_kzalloc(dev, sizeof(*imx_pcie), GFP_KERNEL); > if (!imx_pcie) > @@ -1330,7 +1331,10 @@ static int imx_pcie_probe(struct platform_device *pdev) > 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; > + ret = devm_clk_bulk_get(dev, req_cnt, imx_pcie->clks); > + ret |= devm_clk_bulk_get_optional(dev, imx_pcie->drvdata->clks_optional_cnt, > + imx_pcie->clks + req_cnt); Why do you need to use 'clk_bulk' API to get a single reference clock? Just use devm_clk_get_optional(dev, "ref") And who is going to supply the reference clock in the absence of this clockn in DT? - Mani > if (ret) > return ret; > > @@ -1480,6 +1484,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"}; > > static const struct imx_pcie_drvdata drvdata[] = { > [IMX6Q] = { > @@ -1592,9 +1597,11 @@ static const struct imx_pcie_drvdata drvdata[] = { > }, > [IMX95] = { > .variant = IMX95, > - .flags = IMX_PCIE_FLAG_HAS_SERDES, > - .clk_names = imx8mq_clks, > - .clks_cnt = ARRAY_SIZE(imx8mq_clks), > + .flags = IMX_PCIE_FLAG_HAS_SERDES | > + IMX_PCIE_FLAG_SUPPORTS_SUSPEND, > + .clk_names = imx95_clks, > + .clks_cnt = ARRAY_SIZE(imx95_clks), > + .clks_optional_cnt = 1, > .ltssm_off = IMX95_PE0_GEN_CTRL_3, > .ltssm_mask = IMX95_PCIE_LTSSM_EN, > .mode_off[0] = IMX95_PE0_GEN_CTRL_1, > -- > 2.37.1 >
> -----Original Message----- > From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > Sent: 2024年11月15日 14:38 > 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 <frank.li@nxp.com>; > 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 v6 02/10] PCI: imx6: Add ref clock for i.MX95 PCIe > > On Fri, Nov 01, 2024 at 03:06:02PM +0800, Richard Zhu wrote: > > Add "ref" clock to enable reference clock. To avoid the DT > > compatibility, i.MX95 REF clock might be optional. > > Your wording is not correct. Perhaps you wanted to say, "To avoid breaking > DT backwards compatibility"? > Yes, you're right. Thanks. > > Replace the > > devm_clk_bulk_get() by devm_clk_bulk_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 | 19 +++++++++++++------ > > 1 file changed, 13 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/pci/controller/dwc/pci-imx6.c > > b/drivers/pci/controller/dwc/pci-imx6.c > > index 808d1f105417..bc8567677a67 100644 > > --- a/drivers/pci/controller/dwc/pci-imx6.c > > +++ b/drivers/pci/controller/dwc/pci-imx6.c > > @@ -82,6 +82,7 @@ enum imx_pcie_variants { > > #define IMX_PCIE_FLAG_HAS_SERDES BIT(6) > > #define IMX_PCIE_FLAG_SUPPORT_64BIT BIT(7) > > #define IMX_PCIE_FLAG_CPU_ADDR_FIXUP BIT(8) > > +#define IMX_PCIE_FLAG_CUSTOM_PME_TURNOFF BIT(9) > > > > #define imx_check_flag(pci, val) (pci->drvdata->flags & val) > > > > @@ -98,6 +99,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]; @@ -1278,9 +1280,8 > @@ > > 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 ret, i, req_cnt; > > u16 val; > > - int i; > > > > imx_pcie = devm_kzalloc(dev, sizeof(*imx_pcie), GFP_KERNEL); > > if (!imx_pcie) > > @@ -1330,7 +1331,10 @@ static int imx_pcie_probe(struct > platform_device *pdev) > > 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; > > + ret = devm_clk_bulk_get(dev, req_cnt, imx_pcie->clks); > > + ret |= devm_clk_bulk_get_optional(dev, > imx_pcie->drvdata->clks_optional_cnt, > > + imx_pcie->clks + req_cnt); > > Why do you need to use 'clk_bulk' API to get a single reference clock? Just > use devm_clk_get_optional(dev, "ref") It's easier to add more optional clks in future. I can change to use devm_clk_get_optional(dev, "ref") here if you insistent. > > And who is going to supply the reference clock in the absence of this clockn in > DT? When the "preview" version firmware is used, this clock is gated on in default. In this case, hiso-blk-ctrl would gated on this clock in default state. Best Regards Richard Zhu > > - Mani > > > if (ret) > > return ret; > > > > @@ -1480,6 +1484,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"}; > > > > static const struct imx_pcie_drvdata drvdata[] = { > > [IMX6Q] = { > > @@ -1592,9 +1597,11 @@ static const struct imx_pcie_drvdata drvdata[] = > { > > }, > > [IMX95] = { > > .variant = IMX95, > > - .flags = IMX_PCIE_FLAG_HAS_SERDES, > > - .clk_names = imx8mq_clks, > > - .clks_cnt = ARRAY_SIZE(imx8mq_clks), > > + .flags = IMX_PCIE_FLAG_HAS_SERDES | > > + IMX_PCIE_FLAG_SUPPORTS_SUSPEND, > > + .clk_names = imx95_clks, > > + .clks_cnt = ARRAY_SIZE(imx95_clks), > > + .clks_optional_cnt = 1, > > .ltssm_off = IMX95_PE0_GEN_CTRL_3, > > .ltssm_mask = IMX95_PCIE_LTSSM_EN, > > .mode_off[0] = IMX95_PE0_GEN_CTRL_1, > > -- > > 2.37.1 > > > > -- > மணிவண்ணன் சதாசிவம்
> -----Original Message----- > From: Hongxing Zhu > Sent: 2024年11月18日 10:59 > To: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > 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 <frank.li@nxp.com>; 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 v6 02/10] PCI: imx6: Add ref clock for i.MX95 PCIe > > > -----Original Message----- > > From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > Sent: 2024年11月15日 14:38 > > 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 > > <frank.li@nxp.com>; 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 v6 02/10] PCI: imx6: Add ref clock for i.MX95 PCIe > > > > On Fri, Nov 01, 2024 at 03:06:02PM +0800, Richard Zhu wrote: > > > Add "ref" clock to enable reference clock. To avoid the DT > > > compatibility, i.MX95 REF clock might be optional. > > > > Your wording is not correct. Perhaps you wanted to say, "To avoid > > breaking DT backwards compatibility"? > > > Yes, you're right. Thanks. > > > > Replace the > > > devm_clk_bulk_get() by devm_clk_bulk_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 | 19 +++++++++++++------ > > > 1 file changed, 13 insertions(+), 6 deletions(-) > > > > > > diff --git a/drivers/pci/controller/dwc/pci-imx6.c > > > b/drivers/pci/controller/dwc/pci-imx6.c > > > index 808d1f105417..bc8567677a67 100644 > > > --- a/drivers/pci/controller/dwc/pci-imx6.c > > > +++ b/drivers/pci/controller/dwc/pci-imx6.c > > > @@ -82,6 +82,7 @@ enum imx_pcie_variants { > > > #define IMX_PCIE_FLAG_HAS_SERDES BIT(6) > > > #define IMX_PCIE_FLAG_SUPPORT_64BIT BIT(7) > > > #define IMX_PCIE_FLAG_CPU_ADDR_FIXUP BIT(8) > > > +#define IMX_PCIE_FLAG_CUSTOM_PME_TURNOFF BIT(9) > > > > > > #define imx_check_flag(pci, val) (pci->drvdata->flags & val) > > > > > > @@ -98,6 +99,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]; @@ -1278,9 > +1280,8 > > @@ > > > 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 ret, i, req_cnt; > > > u16 val; > > > - int i; > > > > > > imx_pcie = devm_kzalloc(dev, sizeof(*imx_pcie), GFP_KERNEL); > > > if (!imx_pcie) > > > @@ -1330,7 +1331,10 @@ static int imx_pcie_probe(struct > > platform_device *pdev) > > > 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; > > > + ret = devm_clk_bulk_get(dev, req_cnt, imx_pcie->clks); > > > + ret |= devm_clk_bulk_get_optional(dev, > > imx_pcie->drvdata->clks_optional_cnt, > > > + imx_pcie->clks + req_cnt); > > > > Why do you need to use 'clk_bulk' API to get a single reference clock? > > Just use devm_clk_get_optional(dev, "ref") > It's easier to add more optional clks in future. I can change to use > devm_clk_get_optional(dev, "ref") here if you insistent. Since the clock fetch is not distinguished by platforms explicitly. devm_clk_get_optional(dev, "ref") can be used only when i.MX95 specification is added. - ret |= devm_clk_bulk_get_optional(dev, imx_pcie->drvdata->clks_optional_cnt, - imx_pcie->clks + req_cnt); if (ret) return ret; + for (i = 0; i < imx_pcie->drvdata->clks_optional_cnt; i++) { + imx_pcie->clks[req_cnt + i].clk = devm_clk_get_optional(dev, + imx_pcie->drvdata->clk_names[req_cnt + i]); + if (IS_ERR(imx_pcie->clks[req_cnt + i].clk)) + return PTR_ERR(imx_pcie->clks[req_cnt + i].clk); + } Or - ret |= devm_clk_bulk_get_optional(dev, imx_pcie->drvdata->clks_optional_cnt, - imx_pcie->clks + req_cnt); if (ret) return ret; + if (imx_pcie->drvdata->variant == IMX95) { + 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); + } Which one is better of these two changes? Or To keep codes simple and aligned, how about to keep the original one? Best Regards Richard Zhu > > > > > And who is going to supply the reference clock in the absence of this > > clockn in DT? > When the "preview" version firmware is used, this clock is gated on in default. > In this case, hiso-blk-ctrl would gated on this clock in default state. > > Best Regards > Richard Zhu > > > > - Mani > > > > > if (ret) > > > return ret; > > > > > > @@ -1480,6 +1484,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"}; > > > > > > static const struct imx_pcie_drvdata drvdata[] = { > > > [IMX6Q] = { > > > @@ -1592,9 +1597,11 @@ static const struct imx_pcie_drvdata > > > drvdata[] = > > { > > > }, > > > [IMX95] = { > > > .variant = IMX95, > > > - .flags = IMX_PCIE_FLAG_HAS_SERDES, > > > - .clk_names = imx8mq_clks, > > > - .clks_cnt = ARRAY_SIZE(imx8mq_clks), > > > + .flags = IMX_PCIE_FLAG_HAS_SERDES | > > > + IMX_PCIE_FLAG_SUPPORTS_SUSPEND, > > > + .clk_names = imx95_clks, > > > + .clks_cnt = ARRAY_SIZE(imx95_clks), > > > + .clks_optional_cnt = 1, > > > .ltssm_off = IMX95_PE0_GEN_CTRL_3, > > > .ltssm_mask = IMX95_PCIE_LTSSM_EN, > > > .mode_off[0] = IMX95_PE0_GEN_CTRL_1, > > > -- > > > 2.37.1 > > > > > > > -- > > மணிவண்ணன் சதாசிவம்
diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c index 808d1f105417..bc8567677a67 100644 --- a/drivers/pci/controller/dwc/pci-imx6.c +++ b/drivers/pci/controller/dwc/pci-imx6.c @@ -82,6 +82,7 @@ enum imx_pcie_variants { #define IMX_PCIE_FLAG_HAS_SERDES BIT(6) #define IMX_PCIE_FLAG_SUPPORT_64BIT BIT(7) #define IMX_PCIE_FLAG_CPU_ADDR_FIXUP BIT(8) +#define IMX_PCIE_FLAG_CUSTOM_PME_TURNOFF BIT(9) #define imx_check_flag(pci, val) (pci->drvdata->flags & val) @@ -98,6 +99,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]; @@ -1278,9 +1280,8 @@ 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 ret, i, req_cnt; u16 val; - int i; imx_pcie = devm_kzalloc(dev, sizeof(*imx_pcie), GFP_KERNEL); if (!imx_pcie) @@ -1330,7 +1331,10 @@ static int imx_pcie_probe(struct platform_device *pdev) 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; + ret = devm_clk_bulk_get(dev, req_cnt, imx_pcie->clks); + ret |= devm_clk_bulk_get_optional(dev, imx_pcie->drvdata->clks_optional_cnt, + imx_pcie->clks + req_cnt); if (ret) return ret; @@ -1480,6 +1484,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"}; static const struct imx_pcie_drvdata drvdata[] = { [IMX6Q] = { @@ -1592,9 +1597,11 @@ static const struct imx_pcie_drvdata drvdata[] = { }, [IMX95] = { .variant = IMX95, - .flags = IMX_PCIE_FLAG_HAS_SERDES, - .clk_names = imx8mq_clks, - .clks_cnt = ARRAY_SIZE(imx8mq_clks), + .flags = IMX_PCIE_FLAG_HAS_SERDES | + IMX_PCIE_FLAG_SUPPORTS_SUSPEND, + .clk_names = imx95_clks, + .clks_cnt = ARRAY_SIZE(imx95_clks), + .clks_optional_cnt = 1, .ltssm_off = IMX95_PE0_GEN_CTRL_3, .ltssm_mask = IMX95_PCIE_LTSSM_EN, .mode_off[0] = IMX95_PE0_GEN_CTRL_1,