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

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAKfTPtDTnzyksa4Om1HgZTJX7dGeM_vYiEV2eQnEi9AmZK7KEw@mail.gmail.com>
Date: Wed, 17 Sep 2025 19:21:03 +0200
From: Vincent Guittot <vincent.guittot@...aro.org>
To: Niklas Cassel <cassel@...nel.org>
Cc: chester62515@...il.com, mbrugger@...e.com, ghennadi.procopciuc@....nxp.com, 
	s32@....com, lpieralisi@...nel.org, kwilczynski@...nel.org, mani@...nel.org, 
	robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org, 
	Ionut.Vicovan@....com, larisa.grigore@....com, Ghennadi.Procopciuc@....com, 
	ciprianmarian.costea@....com, bogdan.hamciuc@....com, 
	linux-arm-kernel@...ts.infradead.org, linux-pci@...r.kernel.org, 
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/4] dt-bindings: pcie: Add the NXP PCIe controller

Hi Niklas,

On Wed, 17 Sept 2025 at 10:42, Niklas Cassel <cassel@...nel.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