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

[Krzysztof Kozlowski <krzk@kernel.org>]

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@nxp.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...

> 
> 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