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

Hongxing Zhu
2024-10-24T07:43:01Z

> -----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
> >
>
> --
> மணிவண்ணன் சதாசிவம்