diff mbox series

[v4,2/9] PCI: imx6: Add ref clock for i.MX95 PCIe

Message ID 1728981213-8771-3-git-send-email-hongxing.zhu@nxp.com (mailing list archive)
State Superseded
Headers show
Series A bunch of changes to refine i.MX PCIe driver | expand

Commit Message

Hongxing Zhu Oct. 15, 2024, 8:33 a.m. UTC 
Add "ref" clock to enable reference clock.

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 | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

Manivannan Sadhasivam Oct. 22, 2024, 4:46 p.m. UTC | #1 
On Tue, Oct 15, 2024 at 04:33:26PM +0800, Richard Zhu wrote:
> Add "ref" clock to enable reference clock.
> 
> 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.
> 

So this means the driver won't work with old devicetrees. Am I right? Then you
are breaking the DT compatibility.

You should make the clock optional in the driver.

- Mani

> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> Reviewed-by: Frank Li <Frank.Li@nxp.com>
> ---
>  drivers/pci/controller/dwc/pci-imx6.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 808d1f105417..52a8b2dc828a 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -1480,6 +1480,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] = {
> @@ -1593,8 +1594,8 @@ 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),
> +		.clk_names = imx95_clks,
> +		.clks_cnt = ARRAY_SIZE(imx95_clks),
>  		.ltssm_off = IMX95_PE0_GEN_CTRL_3,
>  		.ltssm_mask = IMX95_PCIE_LTSSM_EN,
>  		.mode_off[0]  = IMX95_PE0_GEN_CTRL_1,
> -- 
> 2.37.1
>
Hongxing Zhu Oct. 24, 2024, 7:42 a.m. UTC | #2 
> -----Original Message-----
> From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Sent: 2024年10月23日 0:46
> To: Hongxing Zhu <hongxing.zhu@nxp.com>
> Cc: kw@linux.com; bhelgaas@google.com; lpieralisi@kernel.org; Frank Li
> <frank.li@nxp.com>; l.stach@pengutronix.de; robh+dt@kernel.org;
> conor+dt@kernel.org; shawnguo@kernel.org;
> krzysztof.kozlowski+dt@linaro.org; festevam@gmail.com;
> s.hauer@pengutronix.de; linux-pci@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> devicetree@vger.kernel.org; kernel@pengutronix.de; imx@lists.linux.dev
> Subject: Re: [PATCH v4 2/9] PCI: imx6: Add ref clock for i.MX95 PCIe
> 
> On Tue, Oct 15, 2024 at 04:33:26PM +0800, Richard Zhu wrote:
> > Add "ref" clock to enable reference clock.
> >
> > 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.
> >
> 
> So this means the driver won't work with old devicetrees. Am I right? Then
> you are breaking the DT compatibility.
> 
> You should make the clock optional in the driver.
Anyway, old DTBs can't work even probe is complete successfully. Since this
 bit would be gated off by released boot firmware.

i.MX95 is a pretty new chip, it's my fault that I didn't figure out this bit
can gate the clock out when internal PLL is used as reference clock in the
initial i.MX95 PCIe support upstream.
 
So, there is no different results whatever this commit is applied or not, when
old DTBs are used.
 
How about just keep this commit?
Since the ref clock is not optional for i.MX95 PCIe from HW view actually.

At end, the commit of this patch is updated as below.
"
PCI: imx6: Add ref clock support for i.MX95 PCIe
 
Add "ref" clock to enable the PCIe reference clock on i.MX95, despite breaking
DT compatibility. This change addresses issues with older DTBs, which would not 
work even if probing was successful, as the reference clock bit is gated off by 
the production boot firmware.
 
For systems using an external clock, the ref clock should point to the external 
reference. For systems using the internal clock, the CREF_EN bit in LAST_TO_REG 
will control the reference output, which is implemented in the 
drivers/clk/imx/clk-imx95-blk-ctl.c driver.
"
Best Regards
Richard Zhu
> 
> - Mani
> 
> > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> > Reviewed-by: Frank Li <Frank.Li@nxp.com>
> > ---
> >  drivers/pci/controller/dwc/pci-imx6.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> > b/drivers/pci/controller/dwc/pci-imx6.c
> > index 808d1f105417..52a8b2dc828a 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -1480,6 +1480,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] = {
> > @@ -1593,8 +1594,8 @@ 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),
> > +		.clk_names = imx95_clks,
> > +		.clks_cnt = ARRAY_SIZE(imx95_clks),
> >  		.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 mbox series

Patch

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 808d1f105417..52a8b2dc828a 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -1480,6 +1480,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] = {
@@ -1593,8 +1594,8 @@  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),
+		.clk_names = imx95_clks,
+		.clks_cnt = ARRAY_SIZE(imx95_clks),
 		.ltssm_off = IMX95_PE0_GEN_CTRL_3,
 		.ltssm_mask = IMX95_PCIE_LTSSM_EN,
 		.mode_off[0]  = IMX95_PE0_GEN_CTRL_1,