diff mbox series

[v3,04/17] dt-bindings: PCI: dwc: Add max-link-speed common property

Message ID 20220610085706.15741-5-Sergey.Semin@baikalelectronics.ru (mailing list archive)
State Superseded
Headers show
Series PCI: dwc: Add generic resources and Baikal-T1 support | expand

Commit Message

Serge Semin June 10, 2022, 8:56 a.m. UTC
In accordance with [1] DW PCIe controllers support up to Gen5 link speed.
Let's add the max-link-speed property upper bound to 5 then. The DT
bindings of the particular devices are expected to setup more strict
constraint on that parameter.

[1] Synopsys DesignWare Cores PCI Express Controller Databook, Version
5.40a, March 2019, p. 27

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>

---

Changelog v3:
- This is a new patch unpinned from the next one:
  https://lore.kernel.org/linux-pci/20220503214638.1895-2-Sergey.Semin@baikalelectronics.ru/
  by the Rob' request. (@Rob)
---
 Documentation/devicetree/bindings/pci/snps,dw-pcie-common.yaml | 3 +++
 Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml     | 2 ++
 Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml        | 1 +
 3 files changed, 6 insertions(+)

Comments

Rob Herring (Arm) June 15, 2022, 2:55 p.m. UTC | #1
On Fri, Jun 10, 2022 at 11:56:52AM +0300, Serge Semin wrote:
> In accordance with [1] DW PCIe controllers support up to Gen5 link speed.
> Let's add the max-link-speed property upper bound to 5 then. The DT
> bindings of the particular devices are expected to setup more strict
> constraint on that parameter.
> 
> [1] Synopsys DesignWare Cores PCI Express Controller Databook, Version
> 5.40a, March 2019, p. 27
> 
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> 
> ---
> 
> Changelog v3:
> - This is a new patch unpinned from the next one:
>   https://lore.kernel.org/linux-pci/20220503214638.1895-2-Sergey.Semin@baikalelectronics.ru/
>   by the Rob' request. (@Rob)
> ---
>  Documentation/devicetree/bindings/pci/snps,dw-pcie-common.yaml | 3 +++
>  Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml     | 2 ++
>  Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml        | 1 +
>  3 files 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 627a5d6625ba..b2fbe886981b 100644
> --- a/Documentation/devicetree/bindings/pci/snps,dw-pcie-common.yaml
> +++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie-common.yaml
> @@ -45,6 +45,9 @@ properties:
>        the peripheral devices available on the PCIe bus.
>      maxItems: 1
>  
> +  max-link-speed:
> +    maximum: 5

Unless the default is less than the max, shouldn't the max here be 1 
less than the h/w max?

> +
>    num-lanes:
>      description:
>        Number of PCIe link lanes to use. Can be omitted should the already
> diff --git a/Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml b/Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml
> index dcd521aed213..fc3b5d4ac245 100644
> --- a/Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml
> +++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml
> @@ -55,4 +55,6 @@ examples:
>  
>        phys = <&pcie_phy0>, <&pcie_phy1>, <&pcie_phy2>, <&pcie_phy3>;
>        phy-names = "pcie0", "pcie1", "pcie2", "pcie3";
> +
> +      max-link-speed = <3>;
>      };
> diff --git a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> index 4a5c8b933b52..01cedf51e0f8 100644
> --- a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> +++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> @@ -74,4 +74,5 @@ examples:
>        phy-names = "pcie";
>  
>        num-lanes = <1>;
> +      max-link-speed = <3>;

This should give you an error because pci-bus.yaml only goes up to 4. 

I'm not really sure that limiting it in the common schema is too useful. 
We're going to be updating it one step at a time. Limiting it is really 
only helpful for specific implementations.

Patch 1 didn't apply for me, so none of the checks ran.

Rob
Serge Semin June 19, 2022, 2:27 p.m. UTC | #2
On Wed, Jun 15, 2022 at 08:55:50AM -0600, Rob Herring wrote:
> On Fri, Jun 10, 2022 at 11:56:52AM +0300, Serge Semin wrote:
> > In accordance with [1] DW PCIe controllers support up to Gen5 link speed.
> > Let's add the max-link-speed property upper bound to 5 then. The DT
> > bindings of the particular devices are expected to setup more strict
> > constraint on that parameter.
> > 
> > [1] Synopsys DesignWare Cores PCI Express Controller Databook, Version
> > 5.40a, March 2019, p. 27
> > 
> > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > 
> > ---
> > 
> > Changelog v3:
> > - This is a new patch unpinned from the next one:
> >   https://lore.kernel.org/linux-pci/20220503214638.1895-2-Sergey.Semin@baikalelectronics.ru/
> >   by the Rob' request. (@Rob)
> > ---
> >  Documentation/devicetree/bindings/pci/snps,dw-pcie-common.yaml | 3 +++
> >  Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml     | 2 ++
> >  Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml        | 1 +
> >  3 files 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 627a5d6625ba..b2fbe886981b 100644
> > --- a/Documentation/devicetree/bindings/pci/snps,dw-pcie-common.yaml
> > +++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie-common.yaml
> > @@ -45,6 +45,9 @@ properties:
> >        the peripheral devices available on the PCIe bus.
> >      maxItems: 1
> >  
> > +  max-link-speed:
> > +    maximum: 5
> 

> Unless the default is less than the max, shouldn't the max here be 1 
> less than the h/w max?

Why? AFAIU max-link-speed semantics it works as less-than-or-equal
operator isn't it? The modern DW PCIe Root ports and Endpoints
IP-cores support up to Gen5 PCIe speed including the Gen5 mode (see
the CX_MAX_PCIE_SPEED IP-core synthesize paramter). It's reasonable to
set the max-link-speed here to be in coherency with the IP-core
reference manual.

> 
> > +
> >    num-lanes:
> >      description:
> >        Number of PCIe link lanes to use. Can be omitted should the already
> > diff --git a/Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml b/Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml
> > index dcd521aed213..fc3b5d4ac245 100644
> > --- a/Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml
> > +++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml
> > @@ -55,4 +55,6 @@ examples:
> >  
> >        phys = <&pcie_phy0>, <&pcie_phy1>, <&pcie_phy2>, <&pcie_phy3>;
> >        phy-names = "pcie0", "pcie1", "pcie2", "pcie3";
> > +
> > +      max-link-speed = <3>;
> >      };
> > diff --git a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > index 4a5c8b933b52..01cedf51e0f8 100644
> > --- a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > +++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > @@ -74,4 +74,5 @@ examples:
> >        phy-names = "pcie";
> >  
> >        num-lanes = <1>;
> > +      max-link-speed = <3>;
> 

> This should give you an error because pci-bus.yaml only goes up to 4. 

I've set max-link-speed to "3" here. So no error will be caused neither
by this schema nor by the pci-bus.yaml bindings.

* Though these examples won't be evaluated because the generic DW PCIe
RP and EP schemas have been marked as "select: false".

> 
> I'm not really sure that limiting it in the common schema is too useful. 
> We're going to be updating it one step at a time. Limiting it is really 
> only helpful for specific implementations.
> 

I disagree. As I said above the max PCIe speed limit set here has been
taken from the HW reference manual so it describes the modern DW PCIe
controllers capability. No mater what value is set by the pci-bus.yaml
schema (eventually we'll get to have it increased to Gen5 too) we can
use the DW PCIe-specific limitation here as a known upper capabilities
bound.

> Patch 1 didn't apply for me, so none of the checks ran.

I see. The series will be re-based onto 5.19-rc1 in the next patchset
revision.

-Sergey

> 
> Rob
Serge Semin June 28, 2022, 12:15 p.m. UTC | #3
Rob,
Could you please get your attention back to this this thread?

-Sergey

On Sun, Jun 19, 2022 at 05:27:20PM +0300, Serge Semin wrote:
> On Wed, Jun 15, 2022 at 08:55:50AM -0600, Rob Herring wrote:
> > On Fri, Jun 10, 2022 at 11:56:52AM +0300, Serge Semin wrote:
> > > In accordance with [1] DW PCIe controllers support up to Gen5 link speed.
> > > Let's add the max-link-speed property upper bound to 5 then. The DT
> > > bindings of the particular devices are expected to setup more strict
> > > constraint on that parameter.
> > > 
> > > [1] Synopsys DesignWare Cores PCI Express Controller Databook, Version
> > > 5.40a, March 2019, p. 27
> > > 
> > > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > > 
> > > ---
> > > 
> > > Changelog v3:
> > > - This is a new patch unpinned from the next one:
> > >   https://lore.kernel.org/linux-pci/20220503214638.1895-2-Sergey.Semin@baikalelectronics.ru/
> > >   by the Rob' request. (@Rob)
> > > ---
> > >  Documentation/devicetree/bindings/pci/snps,dw-pcie-common.yaml | 3 +++
> > >  Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml     | 2 ++
> > >  Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml        | 1 +
> > >  3 files 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 627a5d6625ba..b2fbe886981b 100644
> > > --- a/Documentation/devicetree/bindings/pci/snps,dw-pcie-common.yaml
> > > +++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie-common.yaml
> > > @@ -45,6 +45,9 @@ properties:
> > >        the peripheral devices available on the PCIe bus.
> > >      maxItems: 1
> > >  
> > > +  max-link-speed:
> > > +    maximum: 5
> > 
> 
> > Unless the default is less than the max, shouldn't the max here be 1 
> > less than the h/w max?
> 
> Why? AFAIU max-link-speed semantics it works as less-than-or-equal
> operator isn't it? The modern DW PCIe Root ports and Endpoints
> IP-cores support up to Gen5 PCIe speed including the Gen5 mode (see
> the CX_MAX_PCIE_SPEED IP-core synthesize paramter). It's reasonable to
> set the max-link-speed here to be in coherency with the IP-core
> reference manual.
> 
> > 
> > > +
> > >    num-lanes:
> > >      description:
> > >        Number of PCIe link lanes to use. Can be omitted should the already
> > > diff --git a/Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml b/Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml
> > > index dcd521aed213..fc3b5d4ac245 100644
> > > --- a/Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml
> > > +++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml
> > > @@ -55,4 +55,6 @@ examples:
> > >  
> > >        phys = <&pcie_phy0>, <&pcie_phy1>, <&pcie_phy2>, <&pcie_phy3>;
> > >        phy-names = "pcie0", "pcie1", "pcie2", "pcie3";
> > > +
> > > +      max-link-speed = <3>;
> > >      };
> > > diff --git a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > > index 4a5c8b933b52..01cedf51e0f8 100644
> > > --- a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > > +++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > > @@ -74,4 +74,5 @@ examples:
> > >        phy-names = "pcie";
> > >  
> > >        num-lanes = <1>;
> > > +      max-link-speed = <3>;
> > 
> 
> > This should give you an error because pci-bus.yaml only goes up to 4. 
> 
> I've set max-link-speed to "3" here. So no error will be caused neither
> by this schema nor by the pci-bus.yaml bindings.
> 
> * Though these examples won't be evaluated because the generic DW PCIe
> RP and EP schemas have been marked as "select: false".
> 
> > 
> > I'm not really sure that limiting it in the common schema is too useful. 
> > We're going to be updating it one step at a time. Limiting it is really 
> > only helpful for specific implementations.
> > 
> 
> I disagree. As I said above the max PCIe speed limit set here has been
> taken from the HW reference manual so it describes the modern DW PCIe
> controllers capability. No mater what value is set by the pci-bus.yaml
> schema (eventually we'll get to have it increased to Gen5 too) we can
> use the DW PCIe-specific limitation here as a known upper capabilities
> bound.
> 
> > Patch 1 didn't apply for me, so none of the checks ran.
> 
> I see. The series will be re-based onto 5.19-rc1 in the next patchset
> revision.
> 
> -Sergey
> 
> > 
> > Rob
Rob Herring (Arm) June 28, 2022, 2:56 p.m. UTC | #4
On Tue, Jun 28, 2022 at 6:15 AM Serge Semin <fancer.lancer@gmail.com> wrote:
>
> Rob,
> Could you please get your attention back to this this thread?

I'll get to it. I'm catching up from a week of vacation.

Rob
Serge Semin June 29, 2022, 1:50 a.m. UTC | #5
On Tue, Jun 28, 2022 at 08:56:38AM -0600, Rob Herring wrote:
> On Tue, Jun 28, 2022 at 6:15 AM Serge Semin <fancer.lancer@gmail.com> wrote:
> >
> > Rob,
> > Could you please get your attention back to this this thread?
> 

> I'll get to it. I'm catching up from a week of vacation.

Ok. Sorry for bothering.

-Sergey

> 
> Rob
Rob Herring (Arm) July 1, 2022, 2:44 p.m. UTC | #6
On Sun, Jun 19, 2022 at 05:27:20PM +0300, Serge Semin wrote:
> On Wed, Jun 15, 2022 at 08:55:50AM -0600, Rob Herring wrote:
> > On Fri, Jun 10, 2022 at 11:56:52AM +0300, Serge Semin wrote:
> > > In accordance with [1] DW PCIe controllers support up to Gen5 link speed.
> > > Let's add the max-link-speed property upper bound to 5 then. The DT
> > > bindings of the particular devices are expected to setup more strict
> > > constraint on that parameter.
> > > 
> > > [1] Synopsys DesignWare Cores PCI Express Controller Databook, Version
> > > 5.40a, March 2019, p. 27
> > > 
> > > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > > 
> > > ---
> > > 
> > > Changelog v3:
> > > - This is a new patch unpinned from the next one:
> > >   https://lore.kernel.org/linux-pci/20220503214638.1895-2-Sergey.Semin@baikalelectronics.ru/
> > >   by the Rob' request. (@Rob)
> > > ---
> > >  Documentation/devicetree/bindings/pci/snps,dw-pcie-common.yaml | 3 +++
> > >  Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml     | 2 ++
> > >  Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml        | 1 +
> > >  3 files 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 627a5d6625ba..b2fbe886981b 100644
> > > --- a/Documentation/devicetree/bindings/pci/snps,dw-pcie-common.yaml
> > > +++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie-common.yaml
> > > @@ -45,6 +45,9 @@ properties:
> > >        the peripheral devices available on the PCIe bus.
> > >      maxItems: 1
> > >  
> > > +  max-link-speed:
> > > +    maximum: 5
> > 
> 
> > Unless the default is less than the max, shouldn't the max here be 1 
> > less than the h/w max?
> 
> Why? AFAIU max-link-speed semantics it works as less-than-or-equal
> operator isn't it? The modern DW PCIe Root ports and Endpoints
> IP-cores support up to Gen5 PCIe speed including the Gen5 mode (see
> the CX_MAX_PCIE_SPEED IP-core synthesize paramter). It's reasonable to
> set the max-link-speed here to be in coherency with the IP-core
> reference manual.

It is supposed to be an override for the default. Wouldn't the default 
always be the max the IP supports?

In any case, this max is only accurate for the current/latest version of 
the IP. For older versions, it's not an accurate limit. I think it 
should just be dropped and each controller needs to provide a limit.


> > >    num-lanes:
> > >      description:
> > >        Number of PCIe link lanes to use. Can be omitted should the already
> > > diff --git a/Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml b/Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml
> > > index dcd521aed213..fc3b5d4ac245 100644
> > > --- a/Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml
> > > +++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml
> > > @@ -55,4 +55,6 @@ examples:
> > >  
> > >        phys = <&pcie_phy0>, <&pcie_phy1>, <&pcie_phy2>, <&pcie_phy3>;
> > >        phy-names = "pcie0", "pcie1", "pcie2", "pcie3";
> > > +
> > > +      max-link-speed = <3>;
> > >      };
> > > diff --git a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > > index 4a5c8b933b52..01cedf51e0f8 100644
> > > --- a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > > +++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > > @@ -74,4 +74,5 @@ examples:
> > >        phy-names = "pcie";
> > >  
> > >        num-lanes = <1>;
> > > +      max-link-speed = <3>;
> > 
> 
> > This should give you an error because pci-bus.yaml only goes up to 4. 
> 
> I've set max-link-speed to "3" here. So no error will be caused neither
> by this schema nor by the pci-bus.yaml bindings.
> 
> * Though these examples won't be evaluated because the generic DW PCIe
> RP and EP schemas have been marked as "select: false".

Uh, I don't know what I was thinking...

> 
> > 
> > I'm not really sure that limiting it in the common schema is too useful. 
> > We're going to be updating it one step at a time. Limiting it is really 
> > only helpful for specific implementations.
> > 
> 
> I disagree. As I said above the max PCIe speed limit set here has been
> taken from the HW reference manual so it describes the modern DW PCIe
> controllers capability. No mater what value is set by the pci-bus.yaml
> schema (eventually we'll get to have it increased to Gen5 too) we can
> use the DW PCIe-specific limitation here as a known upper capabilities
> bound.

The latest DWC IP is likely going to support the latest gen within some 
amount of time. With each bump, we're going to have to update 
pci-bus.yaml and then this one too. Yet neither really has any impact 
if we provide a per controller limit.

Rob
Serge Semin July 7, 2022, 7:02 p.m. UTC | #7
On Fri, Jul 01, 2022 at 08:44:24AM -0600, Rob Herring wrote:
> On Sun, Jun 19, 2022 at 05:27:20PM +0300, Serge Semin wrote:
> > On Wed, Jun 15, 2022 at 08:55:50AM -0600, Rob Herring wrote:
> > > On Fri, Jun 10, 2022 at 11:56:52AM +0300, Serge Semin wrote:
> > > > In accordance with [1] DW PCIe controllers support up to Gen5 link speed.
> > > > Let's add the max-link-speed property upper bound to 5 then. The DT
> > > > bindings of the particular devices are expected to setup more strict
> > > > constraint on that parameter.
> > > > 
> > > > [1] Synopsys DesignWare Cores PCI Express Controller Databook, Version
> > > > 5.40a, March 2019, p. 27
> > > > 
> > > > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > > > 
> > > > ---
> > > > 
> > > > Changelog v3:
> > > > - This is a new patch unpinned from the next one:
> > > >   https://lore.kernel.org/linux-pci/20220503214638.1895-2-Sergey.Semin@baikalelectronics.ru/
> > > >   by the Rob' request. (@Rob)
> > > > ---
> > > >  Documentation/devicetree/bindings/pci/snps,dw-pcie-common.yaml | 3 +++
> > > >  Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml     | 2 ++
> > > >  Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml        | 1 +
> > > >  3 files 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 627a5d6625ba..b2fbe886981b 100644
> > > > --- a/Documentation/devicetree/bindings/pci/snps,dw-pcie-common.yaml
> > > > +++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie-common.yaml
> > > > @@ -45,6 +45,9 @@ properties:
> > > >        the peripheral devices available on the PCIe bus.
> > > >      maxItems: 1
> > > >  
> > > > +  max-link-speed:
> > > > +    maximum: 5
> > > 
> > 
> > > Unless the default is less than the max, shouldn't the max here be 1 
> > > less than the h/w max?
> > 
> > Why? AFAIU max-link-speed semantics it works as less-than-or-equal
> > operator isn't it? The modern DW PCIe Root ports and Endpoints
> > IP-cores support up to Gen5 PCIe speed including the Gen5 mode (see
> > the CX_MAX_PCIE_SPEED IP-core synthesize paramter). It's reasonable to
> > set the max-link-speed here to be in coherency with the IP-core
> > reference manual.
> 

> It is supposed to be an override for the default. Wouldn't the default 
> always be the max the IP supports?

No. It depends on the controller implementation, link-partner
capability and results of the negotiation procedure. In our case by
default the link can't be established higher than Gen.2. In order to
rise it up to maximum possible Gen.3, the link must be re-established
with special flag set in the controller CSR.

> In any case, this max is only accurate for the current/latest version of 
> the IP. For older versions, it's not an accurate limit. I think it 
> should just be dropped and each controller needs to provide a limit.

The generic bindings can have some more relaxed constraints than older
IP-core released would have. It's absolutely normal to have the
IP-core evolve in time: new standard support is added, new properties,
new clocks sources or reset controls are added, etc. All of that IMO
should be taken into account in the generic DT-schema as it's supposed
to be as generic as possible a priori. It can have some properties
defined, which particular device release can lack of (a brightest
example is the clock/reset-names). While the particular device
DT-schema shall have a more rigor constraints so the corresponding
DT-node would have only the supported properties.

> 
> 
> > > >    num-lanes:
> > > >      description:
> > > >        Number of PCIe link lanes to use. Can be omitted should the already
> > > > diff --git a/Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml b/Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml
> > > > index dcd521aed213..fc3b5d4ac245 100644
> > > > --- a/Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml
> > > > +++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml
> > > > @@ -55,4 +55,6 @@ examples:
> > > >  
> > > >        phys = <&pcie_phy0>, <&pcie_phy1>, <&pcie_phy2>, <&pcie_phy3>;
> > > >        phy-names = "pcie0", "pcie1", "pcie2", "pcie3";
> > > > +
> > > > +      max-link-speed = <3>;
> > > >      };
> > > > diff --git a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > > > index 4a5c8b933b52..01cedf51e0f8 100644
> > > > --- a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > > > +++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > > > @@ -74,4 +74,5 @@ examples:
> > > >        phy-names = "pcie";
> > > >  
> > > >        num-lanes = <1>;
> > > > +      max-link-speed = <3>;
> > > 
> > 
> > > This should give you an error because pci-bus.yaml only goes up to 4. 
> > 
> > I've set max-link-speed to "3" here. So no error will be caused neither
> > by this schema nor by the pci-bus.yaml bindings.
> > 
> > * Though these examples won't be evaluated because the generic DW PCIe
> > RP and EP schemas have been marked as "select: false".
> 
> Uh, I don't know what I was thinking...
> 
> > 
> > > 
> > > I'm not really sure that limiting it in the common schema is too useful. 
> > > We're going to be updating it one step at a time. Limiting it is really 
> > > only helpful for specific implementations.
> > > 
> > 
> > I disagree. As I said above the max PCIe speed limit set here has been
> > taken from the HW reference manual so it describes the modern DW PCIe
> > controllers capability. No mater what value is set by the pci-bus.yaml
> > schema (eventually we'll get to have it increased to Gen5 too) we can
> > use the DW PCIe-specific limitation here as a known upper capabilities
> > bound.
> 

> The latest DWC IP is likely going to support the latest gen within some 
> amount of time.

Well, in general not necessarily. New incompatible IP-core might get
released instead by the vendor. Vendor can stop supporting the DW PCIe
controller IP-core, etc.

> With each bump, we're going to have to update 
> pci-bus.yaml and then this one too.

I don't see much problem in updating the common property when the new
IP-core and its instances are released.
pci-bus.yaml is supposed to contain a generic max PCIe speed defined
by the standards (although currently it doesn't).
snps,dw-pcie-common.yaml will contain the maximum PCIe speed supported
by the known device instances.
These schemas most likely won't be updated synchronously especially
seeing pci-bus.yaml currently defines max PCIe bus speed of Gen.3,
which isn't the maximum the recent standard defines.

> Yet neither really has any impact 
> if we provide a per controller limit.

If we don't provide the max-link-speed property constraint in the
generic DW PCIe DT-schema, the bindings will be less accurate.
What if the vendor-specific bindings miss the max-link-speed property
constraint while the pci-bus.yaml has greater upper limit than the
IP-core will ever support?..

Anyway I think I've made my point more than clear. I have nothing to
add in this matter. Let's finally finish the discussion. If I wasn't
persuasive enough for you, just explicitly tell me that you'd rather
dropped the patch. So what is your conclusion?

-Sergey

> 
> Rob
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/pci/snps,dw-pcie-common.yaml b/Documentation/devicetree/bindings/pci/snps,dw-pcie-common.yaml
index 627a5d6625ba..b2fbe886981b 100644
--- a/Documentation/devicetree/bindings/pci/snps,dw-pcie-common.yaml
+++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie-common.yaml
@@ -45,6 +45,9 @@  properties:
       the peripheral devices available on the PCIe bus.
     maxItems: 1
 
+  max-link-speed:
+    maximum: 5
+
   num-lanes:
     description:
       Number of PCIe link lanes to use. Can be omitted should the already
diff --git a/Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml b/Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml
index dcd521aed213..fc3b5d4ac245 100644
--- a/Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml
+++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml
@@ -55,4 +55,6 @@  examples:
 
       phys = <&pcie_phy0>, <&pcie_phy1>, <&pcie_phy2>, <&pcie_phy3>;
       phy-names = "pcie0", "pcie1", "pcie2", "pcie3";
+
+      max-link-speed = <3>;
     };
diff --git a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
index 4a5c8b933b52..01cedf51e0f8 100644
--- a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
+++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
@@ -74,4 +74,5 @@  examples:
       phy-names = "pcie";
 
       num-lanes = <1>;
+      max-link-speed = <3>;
     };