[PATCH] dt-bindings: PCI: pci-ep: Add ref-clk-mode

[PATCH] dt-bindings: PCI: pci-ep: Add ref-clk-mode

[Niklas Cassel]

While some boards designs support multiple reference clocking schemes
(e.g. Common Clock and SRNS), and can choose the clocking scheme using
e.g. a DIP switch, most boards designs only support a single clocking
scheme (even if the SoC might support multiple clocking schemes).

This property is needed such that the PCI controller driver, in endpoint
mode, can set the proper bits, e.g. the Common Clock Configuration bit and
the SRIS Clocking bit, in the PCIe Link Control Register (Offset 10h).
(Sometimes, there are also specific bits that needs to be set in the PHY.)

Some device tree bindings have already implemented vendor specific
properties to handle this, e.g. "nvidia,enable-ext-refclk" (Common Clock)
and "nvidia,enable-srns" (SRNS). However, since this property is common
for all PCI controllers running in endpoint mode, this really ought to be
a property in the common pcie-ep.yaml device tree binding.

Add a new ref-clk-mode property that describes the reference clocking
scheme used by the endpoint. (We do not add a common-clk-ssc option, since
we cannot know/control if the common clock provided by the host uses SSC.)

Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 Documentation/devicetree/bindings/pci/pci-ep.yaml | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/pci-ep.yaml b/Documentation/devicetree/bindings/pci/pci-ep.yaml
index f75000e3093d..206c1dc2ab82 100644
--- a/Documentation/devicetree/bindings/pci/pci-ep.yaml
+++ b/Documentation/devicetree/bindings/pci/pci-ep.yaml
@@ -42,6 +42,15 @@ properties:
     default: 1
     maximum: 16
 
+  ref-clk-mode:
+    description: Reference clocking architechture
+    enum:
+      - common-clk        # Common Reference Clock (provided by RC side)
+      - common-clk-ep     # Common Reference Clock (provided by EP side)
+      - common-clk-ep-ssc # Common Reference Clock With Spread (provided by EP side)
+      - srns              # Separate Reference Clocks No Spread
+      - sris              # Separate Reference Clocks Independent Spread
+
   linux,pci-domain:
     description:
       If present this property assigns a fixed PCI domain number to a PCI
-- 
2.49.0

Re: [PATCH] dt-bindings: PCI: pci-ep: Add ref-clk-mode

[Manivannan Sadhasivam]

On Fri, Apr 25, 2025 at 11:20:12AM +0200, Niklas Cassel wrote:
> While some boards designs support multiple reference clocking schemes
> (e.g. Common Clock and SRNS), and can choose the clocking scheme using
> e.g. a DIP switch, most boards designs only support a single clocking
> scheme (even if the SoC might support multiple clocking schemes).
> 
> This property is needed such that the PCI controller driver, in endpoint
> mode, can set the proper bits, e.g. the Common Clock Configuration bit and
> the SRIS Clocking bit, in the PCIe Link Control Register (Offset 10h).
> (Sometimes, there are also specific bits that needs to be set in the PHY.)
> 

Thanks for adding the property. I did plan to submit something similar to allow
Qcom PCIe EP controllers to run in SRIS mode.

> Some device tree bindings have already implemented vendor specific
> properties to handle this, e.g. "nvidia,enable-ext-refclk" (Common Clock)
> and "nvidia,enable-srns" (SRNS). However, since this property is common
> for all PCI controllers running in endpoint mode, this really ought to be
> a property in the common pcie-ep.yaml device tree binding.
> 

We should also mark the nvidia specific properties deprecated and use this one.
But that's for another follow up series.

> Add a new ref-clk-mode property that describes the reference clocking
> scheme used by the endpoint. (We do not add a common-clk-ssc option, since
> we cannot know/control if the common clock provided by the host uses SSC.)
> 
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> ---
>  Documentation/devicetree/bindings/pci/pci-ep.yaml | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pci/pci-ep.yaml b/Documentation/devicetree/bindings/pci/pci-ep.yaml
> index f75000e3093d..206c1dc2ab82 100644
> --- a/Documentation/devicetree/bindings/pci/pci-ep.yaml
> +++ b/Documentation/devicetree/bindings/pci/pci-ep.yaml
> @@ -42,6 +42,15 @@ properties:
>      default: 1
>      maximum: 16
>  
> +  ref-clk-mode:

How about 'refclk-mode' instead of 'ref-clk-mode'? 'refclk' is the most widely
used terminology in the bindings.

> +    description: Reference clocking architechture
> +    enum:
> +      - common-clk        # Common Reference Clock (provided by RC side)

Can we use 'common-clk-host' so that it is explicit that the clock is coming
from the host side?

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH] dt-bindings: PCI: pci-ep: Add ref-clk-mode

[Niklas Cassel]

Hello Mani,

On Wed, Apr 30, 2025 at 12:35:18PM +0530, Manivannan Sadhasivam wrote:
> On Fri, Apr 25, 2025 at 11:20:12AM +0200, Niklas Cassel wrote:
> > While some boards designs support multiple reference clocking schemes
> > (e.g. Common Clock and SRNS), and can choose the clocking scheme using
> > e.g. a DIP switch, most boards designs only support a single clocking
> > scheme (even if the SoC might support multiple clocking schemes).
> > 
> > This property is needed such that the PCI controller driver, in endpoint
> > mode, can set the proper bits, e.g. the Common Clock Configuration bit and
> > the SRIS Clocking bit, in the PCIe Link Control Register (Offset 10h).
> > (Sometimes, there are also specific bits that needs to be set in the PHY.)
> > 
> 
> Thanks for adding the property. I did plan to submit something similar to allow
> Qcom PCIe EP controllers to run in SRIS mode.
> 
> > Some device tree bindings have already implemented vendor specific
> > properties to handle this, e.g. "nvidia,enable-ext-refclk" (Common Clock)
> > and "nvidia,enable-srns" (SRNS). However, since this property is common
> > for all PCI controllers running in endpoint mode, this really ought to be
> > a property in the common pcie-ep.yaml device tree binding.
> > 
> 
> We should also mark the nvidia specific properties deprecated and use this one.
> But that's for another follow up series.
> 
> > Add a new ref-clk-mode property that describes the reference clocking
> > scheme used by the endpoint. (We do not add a common-clk-ssc option, since
> > we cannot know/control if the common clock provided by the host uses SSC.)
> > 
> > Signed-off-by: Niklas Cassel <cassel@kernel.org>
> > ---
> >  Documentation/devicetree/bindings/pci/pci-ep.yaml | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/pci/pci-ep.yaml b/Documentation/devicetree/bindings/pci/pci-ep.yaml
> > index f75000e3093d..206c1dc2ab82 100644
> > --- a/Documentation/devicetree/bindings/pci/pci-ep.yaml
> > +++ b/Documentation/devicetree/bindings/pci/pci-ep.yaml
> > @@ -42,6 +42,15 @@ properties:
> >      default: 1
> >      maximum: 16
> >  
> > +  ref-clk-mode:
> 
> How about 'refclk-mode' instead of 'ref-clk-mode'? 'refclk' is the most widely
> used terminology in the bindings.

I does seem that way.
Will use your suggestion in V2.


> 
> > +    description: Reference clocking architechture
> > +    enum:
> > +      - common-clk        # Common Reference Clock (provided by RC side)
> 
> Can we use 'common-clk-host' so that it is explicit that the clock is coming
> from the host side?

Sure.

I take it that you prefer 'common-clk-host' over 'common-clk-rc' ?


Kind regards,
Niklas

Re: [PATCH] dt-bindings: PCI: pci-ep: Add ref-clk-mode

[Krzysztof Kozlowski]

On 30/04/2025 09:05, Manivannan Sadhasivam wrote:
>> Some device tree bindings have already implemented vendor specific
>> properties to handle this, e.g. "nvidia,enable-ext-refclk" (Common Clock)
>> and "nvidia,enable-srns" (SRNS). However, since this property is common
>> for all PCI controllers running in endpoint mode, this really ought to be
>> a property in the common pcie-ep.yaml device tree binding.
>>
> 
> We should also mark the nvidia specific properties deprecated and use this one.
> But that's for another follow up series.

There should be an user of this property in this patchset (or linked
somehow), otherwise why do we need new binding?


Best regards,
Krzysztof

Re: [PATCH] dt-bindings: PCI: pci-ep: Add ref-clk-mode

[Manivannan Sadhasivam]

On Wed, Apr 30, 2025 at 09:16:56AM +0200, Niklas Cassel wrote:
> Hello Mani,
> 
> On Wed, Apr 30, 2025 at 12:35:18PM +0530, Manivannan Sadhasivam wrote:
> > On Fri, Apr 25, 2025 at 11:20:12AM +0200, Niklas Cassel wrote:
> > > While some boards designs support multiple reference clocking schemes
> > > (e.g. Common Clock and SRNS), and can choose the clocking scheme using
> > > e.g. a DIP switch, most boards designs only support a single clocking
> > > scheme (even if the SoC might support multiple clocking schemes).
> > > 
> > > This property is needed such that the PCI controller driver, in endpoint
> > > mode, can set the proper bits, e.g. the Common Clock Configuration bit and
> > > the SRIS Clocking bit, in the PCIe Link Control Register (Offset 10h).
> > > (Sometimes, there are also specific bits that needs to be set in the PHY.)
> > > 
> > 
> > Thanks for adding the property. I did plan to submit something similar to allow
> > Qcom PCIe EP controllers to run in SRIS mode.
> > 
> > > Some device tree bindings have already implemented vendor specific
> > > properties to handle this, e.g. "nvidia,enable-ext-refclk" (Common Clock)
> > > and "nvidia,enable-srns" (SRNS). However, since this property is common
> > > for all PCI controllers running in endpoint mode, this really ought to be
> > > a property in the common pcie-ep.yaml device tree binding.
> > > 
> > 
> > We should also mark the nvidia specific properties deprecated and use this one.
> > But that's for another follow up series.
> > 
> > > Add a new ref-clk-mode property that describes the reference clocking
> > > scheme used by the endpoint. (We do not add a common-clk-ssc option, since
> > > we cannot know/control if the common clock provided by the host uses SSC.)
> > > 
> > > Signed-off-by: Niklas Cassel <cassel@kernel.org>
> > > ---
> > >  Documentation/devicetree/bindings/pci/pci-ep.yaml | 9 +++++++++
> > >  1 file changed, 9 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/pci/pci-ep.yaml b/Documentation/devicetree/bindings/pci/pci-ep.yaml
> > > index f75000e3093d..206c1dc2ab82 100644
> > > --- a/Documentation/devicetree/bindings/pci/pci-ep.yaml
> > > +++ b/Documentation/devicetree/bindings/pci/pci-ep.yaml
> > > @@ -42,6 +42,15 @@ properties:
> > >      default: 1
> > >      maximum: 16
> > >  
> > > +  ref-clk-mode:
> > 
> > How about 'refclk-mode' instead of 'ref-clk-mode'? 'refclk' is the most widely
> > used terminology in the bindings.
> 
> I does seem that way.
> Will use your suggestion in V2.
> 
> 
> > 
> > > +    description: Reference clocking architechture
> > > +    enum:
> > > +      - common-clk        # Common Reference Clock (provided by RC side)
> > 
> > Can we use 'common-clk-host' so that it is explicit that the clock is coming
> > from the host side?
> 
> Sure.
> 
> I take it that you prefer 'common-clk-host' over 'common-clk-rc' ?
> 

That's what I intended previously, but thinking more, I feel that we should
stick to '-rc'i, as that's what the PCIe spec uses.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH] dt-bindings: PCI: pci-ep: Add ref-clk-mode

[Rob Herring]

On Wed, Apr 30, 2025 at 01:23:03PM +0530, Manivannan Sadhasivam wrote:
> On Wed, Apr 30, 2025 at 09:16:56AM +0200, Niklas Cassel wrote:
> > Hello Mani,
> > 
> > On Wed, Apr 30, 2025 at 12:35:18PM +0530, Manivannan Sadhasivam wrote:
> > > On Fri, Apr 25, 2025 at 11:20:12AM +0200, Niklas Cassel wrote:
> > > > While some boards designs support multiple reference clocking schemes
> > > > (e.g. Common Clock and SRNS), and can choose the clocking scheme using
> > > > e.g. a DIP switch, most boards designs only support a single clocking
> > > > scheme (even if the SoC might support multiple clocking schemes).
> > > > 
> > > > This property is needed such that the PCI controller driver, in endpoint
> > > > mode, can set the proper bits, e.g. the Common Clock Configuration bit and
> > > > the SRIS Clocking bit, in the PCIe Link Control Register (Offset 10h).
> > > > (Sometimes, there are also specific bits that needs to be set in the PHY.)
> > > > 
> > > 
> > > Thanks for adding the property. I did plan to submit something similar to allow
> > > Qcom PCIe EP controllers to run in SRIS mode.
> > > 
> > > > Some device tree bindings have already implemented vendor specific
> > > > properties to handle this, e.g. "nvidia,enable-ext-refclk" (Common Clock)
> > > > and "nvidia,enable-srns" (SRNS). However, since this property is common
> > > > for all PCI controllers running in endpoint mode, this really ought to be
> > > > a property in the common pcie-ep.yaml device tree binding.
> > > > 
> > > 
> > > We should also mark the nvidia specific properties deprecated and use this one.
> > > But that's for another follow up series.
> > > 
> > > > Add a new ref-clk-mode property that describes the reference clocking
> > > > scheme used by the endpoint. (We do not add a common-clk-ssc option, since
> > > > we cannot know/control if the common clock provided by the host uses SSC.)
> > > > 
> > > > Signed-off-by: Niklas Cassel <cassel@kernel.org>
> > > > ---
> > > >  Documentation/devicetree/bindings/pci/pci-ep.yaml | 9 +++++++++
> > > >  1 file changed, 9 insertions(+)
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/pci/pci-ep.yaml b/Documentation/devicetree/bindings/pci/pci-ep.yaml
> > > > index f75000e3093d..206c1dc2ab82 100644
> > > > --- a/Documentation/devicetree/bindings/pci/pci-ep.yaml
> > > > +++ b/Documentation/devicetree/bindings/pci/pci-ep.yaml
> > > > @@ -42,6 +42,15 @@ properties:
> > > >      default: 1
> > > >      maximum: 16
> > > >  
> > > > +  ref-clk-mode:
> > > 
> > > How about 'refclk-mode' instead of 'ref-clk-mode'? 'refclk' is the most widely
> > > used terminology in the bindings.
> > 
> > I does seem that way.
> > Will use your suggestion in V2.
> > 
> > 
> > > 
> > > > +    description: Reference clocking architechture
> > > > +    enum:
> > > > +      - common-clk        # Common Reference Clock (provided by RC side)
> > > 
> > > Can we use 'common-clk-host' so that it is explicit that the clock is coming
> > > from the host side?
> > 
> > Sure.
> > 
> > I take it that you prefer 'common-clk-host' over 'common-clk-rc' ?
> > 
> 
> That's what I intended previously, but thinking more, I feel that we should
> stick to '-rc'i, as that's what the PCIe spec uses.

Couldn't this apply to any link, not just a RC? Is there PCIe 
terminology for upstream and downstream ends of a link?

The 'common-clk' part seems redundant to me with '-rc' or whatever we 
end up with added.

Finally, this[1] seems related. Figure out a common solution.

Rob

[1] https://lore.kernel.org/all/20250406144822.21784-2-marek.vasut+renesas@mailbox.org

Re: [PATCH] dt-bindings: PCI: pci-ep: Add ref-clk-mode

[Manivannan Sadhasivam]

On Fri, May 09, 2025 at 01:18:27PM -0500, Rob Herring wrote:
> On Wed, Apr 30, 2025 at 01:23:03PM +0530, Manivannan Sadhasivam wrote:
> > On Wed, Apr 30, 2025 at 09:16:56AM +0200, Niklas Cassel wrote:
> > > Hello Mani,
> > > 
> > > On Wed, Apr 30, 2025 at 12:35:18PM +0530, Manivannan Sadhasivam wrote:
> > > > On Fri, Apr 25, 2025 at 11:20:12AM +0200, Niklas Cassel wrote:
> > > > > While some boards designs support multiple reference clocking schemes
> > > > > (e.g. Common Clock and SRNS), and can choose the clocking scheme using
> > > > > e.g. a DIP switch, most boards designs only support a single clocking
> > > > > scheme (even if the SoC might support multiple clocking schemes).
> > > > > 
> > > > > This property is needed such that the PCI controller driver, in endpoint
> > > > > mode, can set the proper bits, e.g. the Common Clock Configuration bit and
> > > > > the SRIS Clocking bit, in the PCIe Link Control Register (Offset 10h).
> > > > > (Sometimes, there are also specific bits that needs to be set in the PHY.)
> > > > > 
> > > > 
> > > > Thanks for adding the property. I did plan to submit something similar to allow
> > > > Qcom PCIe EP controllers to run in SRIS mode.
> > > > 
> > > > > Some device tree bindings have already implemented vendor specific
> > > > > properties to handle this, e.g. "nvidia,enable-ext-refclk" (Common Clock)
> > > > > and "nvidia,enable-srns" (SRNS). However, since this property is common
> > > > > for all PCI controllers running in endpoint mode, this really ought to be
> > > > > a property in the common pcie-ep.yaml device tree binding.
> > > > > 
> > > > 
> > > > We should also mark the nvidia specific properties deprecated and use this one.
> > > > But that's for another follow up series.
> > > > 
> > > > > Add a new ref-clk-mode property that describes the reference clocking
> > > > > scheme used by the endpoint. (We do not add a common-clk-ssc option, since
> > > > > we cannot know/control if the common clock provided by the host uses SSC.)
> > > > > 
> > > > > Signed-off-by: Niklas Cassel <cassel@kernel.org>
> > > > > ---
> > > > >  Documentation/devicetree/bindings/pci/pci-ep.yaml | 9 +++++++++
> > > > >  1 file changed, 9 insertions(+)
> > > > > 
> > > > > diff --git a/Documentation/devicetree/bindings/pci/pci-ep.yaml b/Documentation/devicetree/bindings/pci/pci-ep.yaml
> > > > > index f75000e3093d..206c1dc2ab82 100644
> > > > > --- a/Documentation/devicetree/bindings/pci/pci-ep.yaml
> > > > > +++ b/Documentation/devicetree/bindings/pci/pci-ep.yaml
> > > > > @@ -42,6 +42,15 @@ properties:
> > > > >      default: 1
> > > > >      maximum: 16
> > > > >  
> > > > > +  ref-clk-mode:
> > > > 
> > > > How about 'refclk-mode' instead of 'ref-clk-mode'? 'refclk' is the most widely
> > > > used terminology in the bindings.
> > > 
> > > I does seem that way.
> > > Will use your suggestion in V2.
> > > 
> > > 
> > > > 
> > > > > +    description: Reference clocking architechture
> > > > > +    enum:
> > > > > +      - common-clk        # Common Reference Clock (provided by RC side)
> > > > 
> > > > Can we use 'common-clk-host' so that it is explicit that the clock is coming
> > > > from the host side?
> > > 
> > > Sure.
> > > 
> > > I take it that you prefer 'common-clk-host' over 'common-clk-rc' ?
> > > 
> > 
> > That's what I intended previously, but thinking more, I feel that we should
> > stick to '-rc'i, as that's what the PCIe spec uses.
> 
> Couldn't this apply to any link, not just a RC? Is there PCIe 
> terminology for upstream and downstream ends of a link?
> 

Usually, the refclk comes from the host machine to the endpoint, but doesn't
necessarily from the root complex. Since the refclk source could very well be
from the motherboard or the host system PCB, controlled by the host software.

> The 'common-clk' part seems redundant to me with '-rc' or whatever we 
> end up with added.
> 

No. It could be the other way around. We can drop the '-rc' suffix if it seem
redundant. Maybe that is a valid argument also since root complex doesn't
necessarily provide refclk and the common refclk usually comes from the host.

> Finally, this[1] seems related. Figure out a common solution.
> 

Thanks for pointing it out. I think the patch also refers to the 'refclk' though
there was never a mention of 'reference clock'. I will respond there.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH] dt-bindings: PCI: pci-ep: Add ref-clk-mode

[Niklas Cassel]

On Sat, May 10, 2025 at 01:01:51AM +0530, Manivannan Sadhasivam wrote:
> On Fri, May 09, 2025 at 01:18:27PM -0500, Rob Herring wrote:
> > > > > 
> > > > > > +    description: Reference clocking architechture
> > > > > > +    enum:
> > > > > > +      - common-clk        # Common Reference Clock (provided by RC side)
> > > > > 
> > > > > Can we use 'common-clk-host' so that it is explicit that the clock is coming
> > > > > from the host side?
> > > > 
> > > > Sure.
> > > > 
> > > > I take it that you prefer 'common-clk-host' over 'common-clk-rc' ?
> > > > 
> > > 
> > > That's what I intended previously, but thinking more, I feel that we should
> > > stick to '-rc'i, as that's what the PCIe spec uses.
> > 
> > Couldn't this apply to any link, not just a RC? Is there PCIe 
> > terminology for upstream and downstream ends of a link?
> > 
> 
> Usually, the refclk comes from the host machine to the endpoint, but doesn't
> necessarily from the root complex. Since the refclk source could very well be
> from the motherboard or the host system PCB, controlled by the host software.
> 
> > The 'common-clk' part seems redundant to me with '-rc' or whatever we 
> > end up with added.
> > 
> 
> No. It could be the other way around. We can drop the '-rc' suffix if it seem
> redundant. Maybe that is a valid argument also since root complex doesn't
> necessarily provide refclk and the common refclk usually comes from the host.

When the RC and EP uses a common clock (rather than separate clocks),
the clock can either be provided by the host side or the EP side.

The most common by far (if using a common clock) is that it the common
clock is provided by the host side. That is why my patch just named it
'common-clk' instead of 'common-clk-host' or 'common-clk-rc'.

I can use whatever name we agree on. I indend to send out V2 of this
patch as part of a series that adds SRIS support to the dw-rockchip
driver, in order to address Krzysztof's comment.


> 
> > Finally, this[1] seems related. Figure out a common solution.

I don't see the connection.

https://lore.kernel.org/all/20250406144822.21784-2-marek.vasut+renesas@mailbox.org/

does specify a reference clock, but that is in a host side DT binding.


This patch adds a refclk-mode property to an endpoint side DT binding.

This property is needed such that the endpoint can configure the bits
in its own PCIe Link Control Register before starting the link.

Perhaps the host side could also make use of a similar property, but I'm not
sure, you don't know from the host side which endpoint will be plugged in.

From the EP side, you do know if your SoC only supports common-clock or
SRNS/SRIS, since that depends on if the board can source the clock from
the PCIe slot or not (of all the DWC based drivers, only Qcom and Tegra
can do so, rest uses SRNS/SRIS), so this property definitely makes sense
in an EP side DT binding.


Kind regards,
Niklas

Re: [PATCH] dt-bindings: PCI: pci-ep: Add ref-clk-mode

[Rob Herring]

On Sat, May 10, 2025 at 01:04:16PM +0200, Niklas Cassel wrote:
> On Sat, May 10, 2025 at 01:01:51AM +0530, Manivannan Sadhasivam wrote:
> > On Fri, May 09, 2025 at 01:18:27PM -0500, Rob Herring wrote:
> > > > > > 
> > > > > > > +    description: Reference clocking architechture
> > > > > > > +    enum:
> > > > > > > +      - common-clk        # Common Reference Clock (provided by RC side)
> > > > > > 
> > > > > > Can we use 'common-clk-host' so that it is explicit that the clock is coming
> > > > > > from the host side?
> > > > > 
> > > > > Sure.
> > > > > 
> > > > > I take it that you prefer 'common-clk-host' over 'common-clk-rc' ?
> > > > > 
> > > > 
> > > > That's what I intended previously, but thinking more, I feel that we should
> > > > stick to '-rc'i, as that's what the PCIe spec uses.
> > > 
> > > Couldn't this apply to any link, not just a RC? Is there PCIe 
> > > terminology for upstream and downstream ends of a link?
> > > 
> > 
> > Usually, the refclk comes from the host machine to the endpoint, but doesn't
> > necessarily from the root complex. Since the refclk source could very well be
> > from the motherboard or the host system PCB, controlled by the host software.
> > 
> > > The 'common-clk' part seems redundant to me with '-rc' or whatever we 
> > > end up with added.
> > > 
> > 
> > No. It could be the other way around. We can drop the '-rc' suffix if it seem
> > redundant. Maybe that is a valid argument also since root complex doesn't
> > necessarily provide refclk and the common refclk usually comes from the host.
> 
> When the RC and EP uses a common clock (rather than separate clocks),
> the clock can either be provided by the host side or the EP side.
> 
> The most common by far (if using a common clock) is that it the common
> clock is provided by the host side. That is why my patch just named it
> 'common-clk' instead of 'common-clk-host' or 'common-clk-rc'.
> 
> I can use whatever name we agree on. I indend to send out V2 of this
> patch as part of a series that adds SRIS support to the dw-rockchip
> driver, in order to address Krzysztof's comment.
> 
> 
> > 
> > > Finally, this[1] seems related. Figure out a common solution.
> 
> I don't see the connection.
> 
> https://lore.kernel.org/all/20250406144822.21784-2-marek.vasut+renesas@mailbox.org/
> 
> does specify a reference clock, but that is in a host side DT binding.
> 
> 
> This patch adds a refclk-mode property to an endpoint side DT binding.

If we are dealing with the same property of the link, it doesn't matter 
which side. What we don't need is 2 different solutions.

> This property is needed such that the endpoint can configure the bits
> in its own PCIe Link Control Register before starting the link.
> 
> Perhaps the host side could also make use of a similar property, but I'm not
> sure, you don't know from the host side which endpoint will be plugged in.
> 
> >From the EP side, you do know if your SoC only supports common-clock or
> SRNS/SRIS, since that depends on if the board can source the clock from
> the PCIe slot or not (of all the DWC based drivers, only Qcom and Tegra
> can do so, rest uses SRNS/SRIS), so this property definitely makes sense
> in an EP side DT binding.

I don't understand why we need this in DT in the first place. Seems like 
needing to specify this breaks discoverability? Perhaps this information 
is only relevant after initial link is up and the host can read the EP 
registers?

Rob

Re: [PATCH] dt-bindings: PCI: pci-ep: Add ref-clk-mode

[Niklas Cassel]

On Mon, May 12, 2025 at 08:59:09AM -0500, Rob Herring wrote:
> > 
> > This patch adds a refclk-mode property to an endpoint side DT binding.
> 
> If we are dealing with the same property of the link, it doesn't matter 
> which side. What we don't need is 2 different solutions.

It is not really a property of the link though.

The RC could be running with a Separate Reference clock without spread
spectrum clocking (SSC), while the EP could be running with a Separate
Reference clock with SSC.

The link would still be classified as SRIS, even though only one end of
the link is using SSC.

So AFAICT there cannot be property "per link".


> 
> > This property is needed such that the endpoint can configure the bits
> > in its own PCIe Link Control Register before starting the link.
> > 
> > Perhaps the host side could also make use of a similar property, but I'm not
> > sure, you don't know from the host side which endpoint will be plugged in.
> > 
> > >From the EP side, you do know if your SoC only supports common-clock or
> > SRNS/SRIS, since that depends on if the board can source the clock from
> > the PCIe slot or not (of all the DWC based drivers, only Qcom and Tegra
> > can do so, rest uses SRNS/SRIS), so this property definitely makes sense
> > in an EP side DT binding.
> 
> I don't understand why we need this in DT in the first place. Seems like 
> needing to specify this breaks discoverability? Perhaps this information 
> is only relevant after initial link is up and the host can read the EP 
> registers?

If we take the RK3588 SoC as an example, per the TRM, the SoC supports both
SRNS and Common Clock. However, on the Rock 5b board (which uses the RK3588
SoC), the refclock when running is EP mode can only be sourced from the
clock generator on the board itself (Separate Reference clock), it is not
possible to source the refclock from the PCIe slot itself (Common Clock).

However, this is a design limitation of the board, not of the SoC.

E.g. Rockchip might have a development board that uses the RK3588 SoC,
which allows you select where to source the clock from using a mux, either
from the PCIe slot, or from the on board clock generator.

Some development boards I have seen have a DIP switch on the board that
allows you to select if you want to source the clock from the PCIe slot or
not. However, not all boards have this nice feature.

And even if you do have a DIP switch for that, and a GPIO which you can
read the DIP switch value from, when running in Separate Reference clock
mode, you can either run with or without SSC (i.e. SRNS mode or SRIS mode).

When running in SRIS mode, to enable SSC, we need to write registers both
in the PHY and in the controller, before even starting link training.

I do realize that, for boards supporting more than a single mode (Common
Clock/SRNS/SRIS), this device tree property is basically a configuration
option. For boards only supporting a single mode, it is actually describing
the hardware.

E.g. Rock 5b can run in both SRNS and SRIS mode (Common Clock is not
supported), and since this has to be configured before starting the link,
I cannot think of a better way to control this than a device tree property.

In my specific case, I will also need to add a SSC property to the PCIe PHY
DT binding, to control if SSC should be enabled or not (needed when running
in SRIS mode).

Sure, perhaps it could be possible to use phy_set_mode() from the PCIe
controller driver or similar, that conveys this information to the PHY
driver... But with all the possible PCI bifurcation DT properties that
already exist in the PCIe PHY DT binding, I'm not sure if making use of
phy_set_mode() is feasible, or if I will be forced to add a property to the
PCIe PHY DT binding.


Kind regards,
Niklas

Re: [PATCH] dt-bindings: PCI: pci-ep: Add ref-clk-mode

[Niklas Cassel]

Rob,

On Tue, May 13, 2025 at 07:25:50PM +0200, Niklas Cassel wrote:
> On Mon, May 12, 2025 at 08:59:09AM -0500, Rob Herring wrote:
> 
> I do realize that, for boards supporting more than a single mode (Common
> Clock/SRNS/SRIS), this device tree property is basically a configuration
> option. For boards only supporting a single mode, it is actually describing
> the hardware.
> 
> E.g. Rock 5b can run in both SRNS and SRIS mode (Common Clock is not
> supported), and since this has to be configured before starting the link,
> I cannot think of a better way to control this than a device tree property.

Just to provide some additional context (although it was mentioned in the
commit log), basically what I am trying to accomplish is to replace the
vendor specific properties
"nvidia,enable-ext-refclk" (Common Clock) and "nvidia,enable-srns" (SRNS)
(the tegra SoC does not support SRIS, that is why it doesn't have a
property for that) with a generic property that can be used by all PCIe
endpoint controller drivers.

IMO, there is really no reason to have two properties for this, it is
cleaner to just have a single property which is basically an enum
(as suggested by this patch).

To bring some additional insight of how it will be used, one could grep
for nvidia,enable-ext-refclk and nvidia,enable-srns in the tree.

My plan was to get some feedback on this binding before implementing
support for SRIS in the dw-rockchip driver, but I can also modify the
tegra driver to also support this new property in V2.


Kind regards,
Niklas

Re: [PATCH] dt-bindings: PCI: pci-ep: Add ref-clk-mode

[Rob Herring]

On Tue, May 13, 2025 at 07:25:45PM +0200, Niklas Cassel wrote:
> On Mon, May 12, 2025 at 08:59:09AM -0500, Rob Herring wrote:
> > > 
> > > This patch adds a refclk-mode property to an endpoint side DT binding.
> > 
> > If we are dealing with the same property of the link, it doesn't matter 
> > which side. What we don't need is 2 different solutions.
> 
> It is not really a property of the link though.
> 
> The RC could be running with a Separate Reference clock without spread
> spectrum clocking (SSC), while the EP could be running with a Separate
> Reference clock with SSC.
> 
> The link would still be classified as SRIS, even though only one end of
> the link is using SSC.

In the SSC cases, that's only relevant to the end you need to enable SSC 
or not, right? IOW, whether the RC is using SSC or not is irrelevant to 
the EP. And for the end with SSC, that's only if 

There's also this discussion[1] about common SSC handling. In general, 
it seems ON/OFF is not sufficient for configuring spread-spectrum.

> 
> So AFAICT there cannot be property "per link".

Fair enough.

I think we're mixing SSC and clock sources which are mostly independent 
things to describe. SSC is configuration on top of what the clocking 
topology looks like.

For clock sources, the clock can come from the slot or locally. For 
locally, that could be internal to the SoC or some source on the board. 
Does it make sense to describe this with the clock binding? 

Let's assume we define a 'clocks' entry to represent the PCIe clock. We 
could define no clock to mean 'using the slot clock'. Anything else is a 
local clock source which is where it gets interesting. There's probably 
all sorts of ways that can look between the PCIe controller, PHY, and 
board level components. Given the variability there, using the clock 
binding is probably the right thing to do. 

Then if we have a clock defined and figure out the common 
spread-spectrum handling for the clock binding, you would have 
everything you need.


> > > This property is needed such that the endpoint can configure the bits
> > > in its own PCIe Link Control Register before starting the link.
> > > 
> > > Perhaps the host side could also make use of a similar property, but I'm not
> > > sure, you don't know from the host side which endpoint will be plugged in.
> > > 
> > > >From the EP side, you do know if your SoC only supports common-clock or
> > > SRNS/SRIS, since that depends on if the board can source the clock from
> > > the PCIe slot or not (of all the DWC based drivers, only Qcom and Tegra
> > > can do so, rest uses SRNS/SRIS), so this property definitely makes sense
> > > in an EP side DT binding.
> > 
> > I don't understand why we need this in DT in the first place. Seems like 
> > needing to specify this breaks discoverability? Perhaps this information 
> > is only relevant after initial link is up and the host can read the EP 
> > registers?
> 
> If we take the RK3588 SoC as an example, per the TRM, the SoC supports both
> SRNS and Common Clock. However, on the Rock 5b board (which uses the RK3588
> SoC), the refclock when running is EP mode can only be sourced from the
> clock generator on the board itself (Separate Reference clock), it is not
> possible to source the refclock from the PCIe slot itself (Common Clock).
> 
> However, this is a design limitation of the board, not of the SoC.
> 
> E.g. Rockchip might have a development board that uses the RK3588 SoC,
> which allows you select where to source the clock from using a mux, either
> from the PCIe slot, or from the on board clock generator.
> 
> Some development boards I have seen have a DIP switch on the board that
> allows you to select if you want to source the clock from the PCIe slot or
> not. However, not all boards have this nice feature.
> 
> And even if you do have a DIP switch for that, and a GPIO which you can
> read the DIP switch value from, when running in Separate Reference clock
> mode, you can either run with or without SSC (i.e. SRNS mode or SRIS mode).
> 
> When running in SRIS mode, to enable SSC, we need to write registers both
> in the PHY and in the controller, before even starting link training.
> 
> I do realize that, for boards supporting more than a single mode (Common
> Clock/SRNS/SRIS), this device tree property is basically a configuration
> option. For boards only supporting a single mode, it is actually describing
> the hardware.

Okay, I understand now and agree it is needed. Even a DIP switch isn't 
too helpful. The meaning of each state is board specific and not 
discoverable, so we're not going to have that in the kernel. You'd need 
some board specific code in firmware to set some a common DT 
property (because requiring users to set a DT property based on some 
DIP switch isn't very user friendly), or the DIP switch has to be set 
one way and users have to read that bit of documentation.

> E.g. Rock 5b can run in both SRNS and SRIS mode (Common Clock is not
> supported), and since this has to be configured before starting the link,
> I cannot think of a better way to control this than a device tree property.
> 
> In my specific case, I will also need to add a SSC property to the PCIe PHY
> DT binding, to control if SSC should be enabled or not (needed when running
> in SRIS mode).
> 
> Sure, perhaps it could be possible to use phy_set_mode() from the PCIe
> controller driver or similar, that conveys this information to the PHY
> driver... But with all the possible PCI bifurcation DT properties that
> already exist in the PCIe PHY DT binding, I'm not sure if making use of
> phy_set_mode() is feasible, or if I will be forced to add a property to the
> PCIe PHY DT binding.

Don't let kernel implementation define the binding. As long as the 
information is in DT, the kernel can extract that and provide it to 
whatever components need it.

Rob

[1] https://github.com/devicetree-org/dt-schema/pull/154