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

Vincent Guittot
2025-09-17T17:21:25Z

Hi Niklas,

On Wed, 17 Sept 2025 at 10:42, Niklas Cassel <cassel@kernel.org> wrote:
>
> Hello Vincent,
>
> Nice to see you sending some PCIe patches :)
>
> Quite different from the scheduler and power management patches that you
> usually work on :)

Yeah, It's always interesting to explore different areas

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

I agree. Thanks for having shared the email threads on the subject.

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

yes, they are all the same

- common or separate clock
- Spread spectrum or not

and finally which clock to use as the reference behind internal or external

In my case, I only need to know the first 2 items

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

Having a clock binding to define where the clock(s) comes from could
be a good way to describe the various ways to provide the ref clock
and an empty "ref" clock can suggest using an internal clock for those
which have one.

But I don't see an easy way to describe common vs separate and with or
without spread spectrum.

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

I tend to agree that getting the common/separate and w/ or w/o spread
spectrum is not straightforward.

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

The above provides much more than what we need as it is mainly a
boolean for pcie than characterizing the spread spectrum itself

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

I agree.

Regards,
Vincent

>
>
> Kind regards,
> Niklas