linux-kernel - Re: [PATCH v4 1/3] dt-bindings: PCI: dwc: Add one more reference clock

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <aGKm41I/FsrWN8jN@lizhi-Precision-Tower-5810>
Date: Mon, 30 Jun 2025 11:08:50 -0400
From: Frank Li <Frank.li@....com>
To: Krzysztof Kozlowski <krzk@...nel.org>
Cc: Richard Zhu <hongxing.zhu@....com>, l.stach@...gutronix.de,
	lpieralisi@...nel.org, kwilczynski@...nel.org, mani@...nel.org,
	robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org,
	bhelgaas@...gle.com, shawnguo@...nel.org, s.hauer@...gutronix.de,
	kernel@...gutronix.de, festevam@...il.com,
	linux-pci@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	devicetree@...r.kernel.org, imx@...ts.linux.dev,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 1/3] dt-bindings: PCI: dwc: Add one more reference
 clock

On Mon, Jun 30, 2025 at 10:25:43AM +0200, Krzysztof Kozlowski wrote:
> On 28/06/2025 17:33, Frank Li wrote:
> > On Sat, Jun 28, 2025 at 02:34:12PM +0200, Krzysztof Kozlowski wrote:
> >> On Fri, Jun 27, 2025 at 04:09:49PM -0400, Frank Li wrote:
> >>> On Fri, Jun 27, 2025 at 08:54:46AM +0200, Krzysztof Kozlowski wrote:
> >>>> On Thu, Jun 26, 2025 at 03:38:02PM +0800, Richard Zhu wrote:
> >>>>> Add one more reference clock "extref" to be onhalf the reference clock
> >>>>> that comes from external crystal oscillator.
> >>>>>
> >>>>> Signed-off-by: Richard Zhu <hongxing.zhu@....com>
> >>>>> ---
> >>>>>  .../devicetree/bindings/pci/snps,dw-pcie-common.yaml        | 6 ++++++
> >>>>>  1 file changed, 6 insertions(+)
> >>>>>
> >>>>> diff --git a/Documentation/devicetree/bindings/pci/snps,dw-pcie-common.yaml b/Documentation/devicetree/bindings/pci/snps,dw-pcie-common.yaml
> >>>>> index 34594972d8db..ee09e0d3bbab 100644
> >>>>> --- a/Documentation/devicetree/bindings/pci/snps,dw-pcie-common.yaml
> >>>>> +++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie-common.yaml
> >>>>> @@ -105,6 +105,12 @@ properties:
> >>>>>              define it with this name (for instance pipe, core and aux can
> >>>>>              be connected to a single source of the periodic signal).
> >>>>>            const: ref
> >>>>> +        - description:
> >>>>> +            Some dwc wrappers (like i.MX95 PCIes) have two reference clock
> >>>>> +            inputs, one from internal PLL, the other from off chip crystal
> >>>>> +            oscillator. Use extref clock name to be onhalf of the reference
> >>>>> +            clock comes form external crystal oscillator.
> >>>>
> >>>> How internal PLL can be represented as 'ref' clock? Internal means it is
> >>>> not outside, so impossible to represent.
> >>>
> >>> Internal means in side SoC, but outside PCIe controller.
> >>
> >> So external... It does not matter for PCIe controller whether clock is
> >> coming from SoC or from some crystal.  It is still input pin. Same input
> >> pin.
> >
> > It is NOT the same pin. It is TWO pins, there are mux inside in PCI
> > controller.
> >
> > There are similar cases in s32 rtc, there are 4 input source[0,1,2,3]
> > https://lore.kernel.org/imx/20241127144322.GA3454134-robh@kernel.org/
> > Only one provide.
> >
> >>
> >>>
> >>>>
> >>>> Where is the DTS so we can look at big picture?
> >>>
> >>> imx94 pci's upstream is still on going, which quite similar with imx95.
> >>> Just board design choose external crystal.
> >>>
> >>> pcie_ref_clk: clock-pcie-ref {
> >>>                 compatible = "gpio-gate-clock";
> >>>                 clocks = <&xtal25m>;
> >>>                 #clock-cells = <0>;
> >>>                 enable-gpios = <&pca9670_i2c3 7 GPIO_ACTIVE_LOW>;
> >>> };
> >>>
> >>> &pcie0 {
> >>>         pinctrl-0 = <&pinctrl_pcie0>;
> >>>         pinctrl-names = "default";
> >>>         clocks = <&scmi_clk IMX94_CLK_HSIO>,
> >>>                  <&scmi_clk IMX94_CLK_HSIOPLL>,
> >>>                  <&scmi_clk IMX94_CLK_HSIOPLL_VCO>,
> >>>                  <&scmi_clk IMX94_CLK_HSIOPCIEAUX>,
> >>>                  <&pcie_ref_clk>;
> >>>         clock-names = "pcie", "pcie_bus", "pcie_phy", "pcie_aux", "ext-ref";
> >>
> >> So this is totally faked hardware property.
> >>
> >> No, it is the same clock signal, not different. You write bindings from
> >> this device point of view, not for your board.
> >
> > No the same clock signal. There are two sources, "ext-ref" or "ref".
> > PCI controller need know which one provide clocks.
>
> OK, this should be clearly expressed not some vague play of the words
> what is internal and external...

Does bjorn's suggest description is clear enough?

  "Some dwc wrappers (like i.MX95 PCIes) have two reference clock
  inputs, one from an internal PLL, the other from an off-chip crystal
  oscillator. If present, 'extref' refers to a reference clock from
  an external oscillator."

Frank
>
> >
> > There are mux inside PCI controller, DT need provide information which on
> > provide.
> >
> > Maybe my example dts miss-lead you. Altherate descript is
> >   clock-names = "pcie", "pcie_bus", "pcie_phy", "pcie_aux", "ref", "ext-ref";
> >
> >   But we thinks if ext-ref provide, "ref" is not neccesary need be turn on.
> >   So remove it from the list.
>
> If the ref clock is actually wired it should be there. You describe here
> hardware, not what is necessary.
>
> Best regards,
> Krzysztof