Re: [PATCH 1/4] dt-bindings: pcie: Add the NXP PCIe controller

[Niklas Cassel <cassel@kernel.org>]

Hello Vincent,

Nice to see you sending some PCIe patches :)

Quite different from the scheduler and power management patches that you
usually work on :)

(snip)

> +  nxp,phy-mode:
> +    $ref: /schemas/types.yaml#/definitions/string
> +    description: Select PHY mode for PCIe controller
> +    enum:
> +      - crns  # Common Reference Clock, No Spread Spectrum
> +      - crss  # Common Reference Clock, Spread Spectrum
> +      - srns  # Separate reference Clock, No Spread Spectrum
> +      - sris  # Separate Reference Clock, Independent Spread Spectrum

This does not seem to be anything NXP specific, so I really think that this
should be some kind of generic property.


Note that I tried to add a similar property, but for the PCIe endpoint side
rather that the PCIe root complex side:
https://lore.kernel.org/linux-pci/20250425092012.95418-2-cassel@kernel.org/

However, due to shifting priorities, I haven't been able to follow up with
a new version/proposal.

My problem is not exactly the same, but even for a root complex, the PCI
specification allows the endpoint side to provide the common clock, which
means that the root complex would source the refclk from the PCIe slot,
so I would say that our problems are quite similar.

Rob Herring suggested to use the clock binding rather than an enum.
I can see his point of view, but personally I'm not convinced that his
suggestion of not having a clock specified means "source the refclock from
the slot" is better than a simple enum.

To me, it seems way clearer to explicitly specify the mode in device tree,
rather than the mode implictly being set if a "clk" phandle is there or not.
That approach seems way easier to misunderstand, as the user would need to
know that the clocking mode is inferred from a "clk" phandle being there or
not.


I also note that Rob Herring was not really a fan of having separate spread
spectrum options. Instead, it seems like he wanted a separate way to define
if SSC was used or not.

I have seen the following patch merged:
https://github.com/devicetree-org/dt-schema/pull/154
https://github.com/devicetree-org/dt-schema/commit/d7c9156d46bd287f21a5ed3303bea8a4d66d452a

So I'm not sure if that is the intended way they want SSC to be defined or
not.


I apologize for bringing up my own problem in this discussion, but at least
it is clear to me that we cannot continue with each PCIe driver adding their
own vendor specific properties (with completely different names) for this.
Some kind of generic solution is needed, at least for new drivers.


Kind regards,
Niklas