[PATCH v8 2/3] dt-bindings: PCI: pci-imx6: Add external reference clock input

[Hongxing Zhu]

> -----Original Message-----
> From: Conor Dooley <conor at kernel.org>
> Sent: 2025年10月28日 1:06
> To: Hongxing Zhu <hongxing.zhu at nxp.com>
> Cc: robh at kernel.org; krzk+dt at kernel.org; conor+dt at kernel.org;
> bhelgaas at google.com; Frank Li <frank.li at nxp.com>; l.stach at pengutronix.de;
> lpieralisi at kernel.org; kwilczynski at kernel.org; mani at kernel.org;
> shawnguo at kernel.org; s.hauer at pengutronix.de; kernel at pengutronix.de;
> festevam at gmail.com; linux-pci at vger.kernel.org;
> linux-arm-kernel at lists.infradead.org; devicetree at vger.kernel.org;
> imx at lists.linux.dev; linux-kernel at vger.kernel.org
> Subject: Re: [PATCH v8 2/3] dt-bindings: PCI: pci-imx6: Add external reference
> clock input
> 
> On Mon, Oct 27, 2025 at 06:36:32AM +0000, Hongxing Zhu wrote:
> > > -----Original Message-----
> > > From: Conor Dooley <conor at kernel.org>
> > > Sent: 2025年10月25日 1:07
> > > To: Hongxing Zhu <hongxing.zhu at nxp.com>
> > > Cc: robh at kernel.org; krzk+dt at kernel.org; conor+dt at kernel.org;
> > > bhelgaas at google.com; Frank Li <frank.li at nxp.com>;
> > > l.stach at pengutronix.de; lpieralisi at kernel.org;
> > > kwilczynski at kernel.org; mani at kernel.org; shawnguo at kernel.org;
> > > s.hauer at pengutronix.de; kernel at pengutronix.de; festevam at gmail.com;
> > > linux-pci at vger.kernel.org; linux-arm-kernel at lists.infradead.org;
> > > devicetree at vger.kernel.org; imx at lists.linux.dev;
> > > linux-kernel at vger.kernel.org
> > > Subject: Re: [PATCH v8 2/3] dt-bindings: PCI: pci-imx6: Add external
> > > reference clock input
> > >
> > > On Fri, Oct 24, 2025 at 10:40:12AM +0800, Richard Zhu wrote:
> > > > i.MX95 PCIes have two reference clock inputs: one from internal
> > > > PLL, the other from off chip crystal oscillator. The "extref"
> > > > clock refers to a reference clock from an external crystal oscillator.
> > > >
> > > > Add external reference clock input for i.MX95 PCIes.
> > > >
> > > > Signed-off-by: Richard Zhu <hongxing.zhu at nxp.com>
> > > > Reviewed-by: Frank Li <Frank.Li at nxp.com>
> > > > ---
> > > >  Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > >
> > > > diff --git
> > > > a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml
> > > > b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml
> > > > index ca5f2970f217c..b4c40d0573dce 100644
> > > > --- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml
> > > > +++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml
> > > > @@ -212,14 +212,17 @@ allOf:
> > > >      then:
> > > >        properties:
> > > >          clocks:
> > > > +          minItems: 4
> > > >            maxItems: 5
> > > >          clock-names:
> > > > +          minItems: 4
> > > >            items:
> > > >              - const: pcie
> > >
> > > 1
> > >
> > > >              - const: pcie_bus
> > >
> > > 2
> > >
> > > >              - const: pcie_phy
> > >
> > > 3
> > >
> > > >              - const: pcie_aux
> > >
> > > 4
> > >
> > > >              - const: ref
> > >
> > > 5
> > >
> > > > +            - const: extref  # Optional
> > >
> > > 6
> > >
> > > There are 6 clocks here, but clocks and clock-names in this binding
> > > do not permit 6:
> > > |  clocks:
> > > |    minItems: 3
> > > |    items:
> > > |      - description: PCIe bridge clock.
> > > |      - description: PCIe bus clock.
> > > |      - description: PCIe PHY clock.
> > > |      - description: Additional required clock entry for imx6sx-pcie,
> > > |           imx6sx-pcie-ep, imx8mq-pcie, imx8mq-pcie-ep.
> > > |      - description: PCIe reference clock.
> > > |
> > > |  clock-names:
> > > |    minItems: 3
> > > |    maxItems: 5
> > >
> > > AFAICT, what this patch actually did is make "ref" an optional
> > > clock, but the claim in the patch is that extref is optional. With
> > > this patch applied, you can have a) no reference clocks or b) only "ref".
> "extref"
> > > is never allowed.
> > Hi Conor:
> > Thanks for your review comments.
> > Just same to "extref", the "ref" should be marked as optional clock too.
> 
> Right, your commit message should then mention that.
> 
Hi Conor:
It's my mistake. My previous understanding is wrong.
Only "extref" is an optional clock.

> > > Is it supposed to be possible to have "ref" and "extref"?
> > > Or "extref" without "ref"?
> > > Neither "ref" or "extref"?
> > "ref" and "extref" have an exclusive relationship of choosing one of
> > the two,  and they cannot all exist simultaneously.
> 
> Right, please go test what you've produced, because extref is never
> permitted by this binding.
"ref" and "extref" doesn't have an exclusive relationship. There are two
 options. one is "ref", the other one is "ref, extref".

If "ref" present, the internal system PLL clock is used as PCIe reference
 clock source. If both of them present, the PCIe reference clock would come
 from an off-chip crystal oscillator.

In the end, the maxItem of clocks should be '6'. I would post the another
 patch-set after correct the maxItem of clocks from '5' to '6'.

Sorry again to my misleading.

Best Regards
Richard Zhu
> 
> > > I don't know the answer to that question because you're doing things
> > > that are contradictory in your patch and the commit message isn't clear.
> > >
> > Sorry for causing you confusion.