[PATCH v2 0/2] rework mtk pcie-gen3 bindings and support mt7986

[PATCH v2 0/2] rework mtk pcie-gen3 bindings and support mt7986

[Frank Wunderlich]

From: Frank Wunderlich <frank-w@public-files.de>

This Series prepares support for mt7986 PCIe which is basicly gen3 PCIe
but with slightly differnt clock configuration.

To make differences better to read i split the exiting bindings which
has already different settings with a compatible switch and then add a
new one for mt7986.

v2:
- fixed typo in part 1 (SoC based config)
- squashed part2+3 (compatible and clock config for mt7986)

Frank Wunderlich (2):
  dt-bindings: PCI: mediatek-gen3: add SoC based clock config
  dt-bindings: PCI: mediatek-gen3: add support for mt7986

 .../bindings/pci/mediatek-pcie-gen3.yaml      | 65 +++++++++++++++----
 1 file changed, 53 insertions(+), 12 deletions(-)

-- 
2.34.1

[PATCH v2 1/2] dt-bindings: PCI: mediatek-gen3: add SoC based clock config

[Frank Wunderlich]

From: Frank Wunderlich <frank-w@public-files.de>

The PCIe driver covers different SOC which needing different clock
configs. Define them based on compatible.

Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
Reviewed-by: Rob Herring <robh@kernel.org>
---
v2:
- fix typo in mediatek,mt8192-pcie
---
 .../bindings/pci/mediatek-pcie-gen3.yaml      | 48 ++++++++++++++-----
 1 file changed, 36 insertions(+), 12 deletions(-)

diff --git a/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml b/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml
index c00be39af64e..98d3f0f1cd76 100644
--- a/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml
+++ b/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml
@@ -43,9 +43,6 @@ description: |+
   each set has its own address for MSI message, and supports 32 MSI vectors
   to generate interrupt.
 
-allOf:
-  - $ref: /schemas/pci/pci-bus.yaml#
-
 properties:
   compatible:
     oneOf:
@@ -84,15 +81,7 @@ properties:
     maxItems: 6
 
   clock-names:
-    items:
-      - const: pl_250m
-      - const: tl_26m
-      - const: tl_96m
-      - const: tl_32k
-      - const: peri_26m
-      - enum:
-          - top_133m        # for MT8192
-          - peri_mem        # for MT8188/MT8195
+    maxItems: 6
 
   assigned-clocks:
     maxItems: 1
@@ -138,6 +127,41 @@ required:
   - '#interrupt-cells'
   - interrupt-controller
 
+allOf:
+  - $ref: /schemas/pci/pci-bus.yaml#
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: mediatek,mt8192-pcie
+    then:
+      properties:
+        clock-names:
+          items:
+            - const: pl_250m
+            - const: tl_26m
+            - const: tl_96m
+            - const: tl_32k
+            - const: peri_26m
+            - const: top_133m
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - mediatek,mt8188-pcie
+              - mediatek,mt8195-pcie
+    then:
+      properties:
+        clock-names:
+          items:
+            - const: pl_250m
+            - const: tl_26m
+            - const: tl_96m
+            - const: tl_32k
+            - const: peri_26m
+            - const: peri_mem
+
 unevaluatedProperties: false
 
 examples:
-- 
2.34.1

Re: [PATCH v2 1/2] dt-bindings: PCI: mediatek-gen3: add SoC based clock config

[Jianjun Wang]

Hi Frank,

After apply this patch, we found some dtbs_check error with the
following patch which adds the PCIe node for MT8195:

https://lore.kernel.org/linux-pci/20221020111925.30002-3-tinghan.shen@mediatek.com/

arch/arm64/boot/dts/mediatek/mt8195-cherry-tomato-r2.dtb: pcie@112f0000
: clock-names:        5: 'top_133m' was expected
    From schema: Documentation/devicetree/bindings/pci/mediatek-pcie-
gen3.yaml
arch/arm64/boot/dts/mediatek/mt8195-cherry-tomato-r2.dtb: pcie@112f8000
: clock-names:        5: 'top_133m' was expected
    From schema: Documentation/devicetree/bindings/pci/mediatek-pcie-
gen3.yaml

Did you get the same error when adding the PCIe node for MT7986?

Thanks. 

On Tue, 2022-10-25 at 09:28 +0200, Frank Wunderlich wrote:
> From: Frank Wunderlich <frank-w@public-files.de>
> 
> The PCIe driver covers different SOC which needing different clock
> configs. Define them based on compatible.
> 
> Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
> Reviewed-by: Rob Herring <robh@kernel.org>
> ---
> v2:
> - fix typo in mediatek,mt8192-pcie
> ---
>  .../bindings/pci/mediatek-pcie-gen3.yaml      | 48 ++++++++++++++---
> --
>  1 file changed, 36 insertions(+), 12 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pci/mediatek-pcie-
> gen3.yaml b/Documentation/devicetree/bindings/pci/mediatek-pcie-
> gen3.yaml
> index c00be39af64e..98d3f0f1cd76 100644
> --- a/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml
> +++ b/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml
> @@ -43,9 +43,6 @@ description: |+
>    each set has its own address for MSI message, and supports 32 MSI
> vectors
>    to generate interrupt.
>  
> -allOf:
> -  - $ref: /schemas/pci/pci-bus.yaml#
> -
>  properties:
>    compatible:
>      oneOf:
> @@ -84,15 +81,7 @@ properties:
>      maxItems: 6
>  
>    clock-names:
> -    items:
> -      - const: pl_250m
> -      - const: tl_26m
> -      - const: tl_96m
> -      - const: tl_32k
> -      - const: peri_26m
> -      - enum:
> -          - top_133m        # for MT8192
> -          - peri_mem        # for MT8188/MT8195
> +    maxItems: 6
>  
>    assigned-clocks:
>      maxItems: 1
> @@ -138,6 +127,41 @@ required:
>    - '#interrupt-cells'
>    - interrupt-controller
>  
> +allOf:
> +  - $ref: /schemas/pci/pci-bus.yaml#
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: mediatek,mt8192-pcie
> +    then:
> +      properties:
> +        clock-names:
> +          items:
> +            - const: pl_250m
> +            - const: tl_26m
> +            - const: tl_96m
> +            - const: tl_32k
> +            - const: peri_26m
> +            - const: top_133m
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - mediatek,mt8188-pcie
> +              - mediatek,mt8195-pcie
> +    then:
> +      properties:
> +        clock-names:
> +          items:
> +            - const: pl_250m
> +            - const: tl_26m
> +            - const: tl_96m
> +            - const: tl_32k
> +            - const: peri_26m
> +            - const: peri_mem
> +
>  unevaluatedProperties: false
>  
>  examples:

Re: [PATCH v2 1/2] dt-bindings: PCI: mediatek-gen3: add SoC based clock config

[Frank Wunderlich]

Am 28. Oktober 2022 11:24:36 MESZ schrieb Jianjun Wang <jianjun.wang@mediatek.com>:
>Hi Frank,
>
>After apply this patch, we found some dtbs_check error with the
>following patch which adds the PCIe node for MT8195:
>
>https://lore.kernel.org/linux-pci/20221020111925.30002-3-tinghan.shen@mediatek.com/
>
>arch/arm64/boot/dts/mediatek/mt8195-cherry-tomato-r2.dtb: pcie@112f0000
>: clock-names:        5: 'top_133m' was expected
>    From schema: Documentation/devicetree/bindings/pci/mediatek-pcie-
>gen3.yaml
>arch/arm64/boot/dts/mediatek/mt8195-cherry-tomato-r2.dtb: pcie@112f8000
>: clock-names:        5: 'top_133m' was expected
>    From schema: Documentation/devicetree/bindings/pci/mediatek-pcie-
>gen3.yaml
>
>Did you get the same error when adding the PCIe node for MT7986?
>
>Thanks. 
>
>On Tue, 2022-10-25 at 09:28 +0200, Frank Wunderlich wrote:
>> From: Frank Wunderlich <frank-w@public-files.de>
>> 

As far as i see the problem is the fallback-node which requires different clockconfig than the main compatible.

6th clock was defined as this enum
  - top_133m        # for MT8192
  - peri_mem        # for MT8188/MT8195

By using lower compatible as main compatible and first one as fallback you cannot success all parts of allOf.

>>    clock-names:
>> -    items:
>> -      - const: pl_250m
>> -      - const: tl_26m
>> -      - const: tl_96m
>> -      - const: tl_32k
>> -      - const: peri_26m
>> -      - enum:
>> -          - top_133m        # for MT8192
>> -          - peri_mem        # for MT8188/MT8195

From my PoV the dts is wrong as the 2 SoC are not compatible to each other...mt8192 needs top_133m as 6th clock whereas mt8195 needs peri_mem. Of course we can change it back to enum in both branches,but imho fallback does not match to main compatible in the dts.

>> +allOf:
>> +  - $ref: /schemas/pci/pci-bus.yaml#
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            const: mediatek,mt8192-pcie
>> +    then:
>> +      properties:
>> +        clock-names:
>> +          items:
>> +            - const: pl_250m
>> +            - const: tl_26m
>> +            - const: tl_96m
>> +            - const: tl_32k
>> +            - const: peri_26m
>> +            - const: top_133m
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            enum:
>> +              - mediatek,mt8188-pcie
>> +              - mediatek,mt8195-pcie
>> +    then:
>> +      properties:
>> +        clock-names:
>> +          items:
>> +            - const: pl_250m
>> +            - const: tl_26m
>> +            - const: tl_96m
>> +            - const: tl_32k
>> +            - const: peri_26m
>> +            - const: peri_mem
>> +
>>  unevaluatedProperties: false
>>  
>>  examples:
>


regards Frank

Aw: Re: [PATCH v2 1/2] dt-bindings: PCI: mediatek-gen3: add SoC based clock config

[Frank Wunderlich]

Hi
> Gesendet: Freitag, 28. Oktober 2022 um 11:24 Uhr
> Von: "Jianjun Wang" <jianjun.wang@mediatek.com>
> An: "Frank Wunderlich" <linux@fw-web.de>, linux-mediatek@lists.infradead.org
> Cc: "Frank Wunderlich" <frank-w@public-files.de>, "Ryder Lee" <ryder.lee@mediatek.com>, "Bjorn Helgaas" <bhelgaas@google.com>, "Rob Herring" <robh+dt@kernel.org>, "Krzysztof Kozlowski" <krzysztof.kozlowski+dt@linaro.org>, "Matthias Brugger" <matthias.bgg@gmail.com>, linux-pci@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, "Rob Herring" <robh@kernel.org>
> Betreff: Re: [PATCH v2 1/2] dt-bindings: PCI: mediatek-gen3: add SoC based clock config
>
> Hi Frank,
>
> After apply this patch, we found some dtbs_check error with the
> following patch which adds the PCIe node for MT8195:
>
> https://lore.kernel.org/linux-pci/20221020111925.30002-3-tinghan.shen@mediatek.com/
>
> arch/arm64/boot/dts/mediatek/mt8195-cherry-tomato-r2.dtb: pcie@112f0000
> : clock-names:        5: 'top_133m' was expected
>     From schema: Documentation/devicetree/bindings/pci/mediatek-pcie-
> gen3.yaml
> arch/arm64/boot/dts/mediatek/mt8195-cherry-tomato-r2.dtb: pcie@112f8000
> : clock-names:        5: 'top_133m' was expected
>     From schema: Documentation/devicetree/bindings/pci/mediatek-pcie-
> gen3.yaml
>
> Did you get the same error when adding the PCIe node for MT7986?

i'm sure i had tested the yaml and did not get any error, but on my retest i get same error for mt7986 too...

maybe the right way is to remove the contains in the mediatek,mt8192-pcie if condition (to match only if this condition is no fallback),
then it is clean for me. Can you test it with your patches?

allOf:
  - $ref: /schemas/pci/pci-bus.yaml#
  - if:
      properties:
        compatible:
          #contains:
          const: mediatek,mt8192-pcie
    then:
      properties:
        clock-names:
          items:
            - const: pl_250m
            - const: tl_26m
            - const: tl_96m
            - const: tl_32k
            - const: peri_26m
            - const: top_133m


regards Frank

regards Frank

Re: Aw: Re: [PATCH v2 1/2] dt-bindings: PCI: mediatek-gen3: add SoC based clock config

[Jianjun Wang (王建军)]

Hi Frank,

On Fri, 2022-10-28 at 18:42 +0200, Frank Wunderlich wrote:
> Hi
> > Gesendet: Freitag, 28. Oktober 2022 um 11:24 Uhr
> > Von: "Jianjun Wang" <jianjun.wang@mediatek.com>
> > An: "Frank Wunderlich" <linux@fw-web.de>, 
> > linux-mediatek@lists.infradead.org
> > Cc: "Frank Wunderlich" <frank-w@public-files.de>, "Ryder Lee" <
> > ryder.lee@mediatek.com>, "Bjorn Helgaas" <bhelgaas@google.com>,
> > "Rob Herring" <robh+dt@kernel.org>, "Krzysztof Kozlowski" <
> > krzysztof.kozlowski+dt@linaro.org>, "Matthias Brugger" <
> > matthias.bgg@gmail.com>, linux-pci@vger.kernel.org, 
> > devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, 
> > linux-arm-kernel@lists.infradead.org, "Rob Herring" <
> > robh@kernel.org>
> > Betreff: Re: [PATCH v2 1/2] dt-bindings: PCI: mediatek-gen3: add
> > SoC based clock config
> > 
> > Hi Frank,
> > 
> > After apply this patch, we found some dtbs_check error with the
> > following patch which adds the PCIe node for MT8195:
> > 
> > 
https://lore.kernel.org/linux-pci/20221020111925.30002-3-tinghan.shen@mediatek.com/
> > 
> > arch/arm64/boot/dts/mediatek/mt8195-cherry-tomato-r2.dtb: 
> > pcie@112f0000
> > : clock-names:        5: 'top_133m' was expected
> >     From schema: Documentation/devicetree/bindings/pci/mediatek-
> > pcie-
> > gen3.yaml
> > arch/arm64/boot/dts/mediatek/mt8195-cherry-tomato-r2.dtb: 
> > pcie@112f8000
> > : clock-names:        5: 'top_133m' was expected
> >     From schema: Documentation/devicetree/bindings/pci/mediatek-
> > pcie-
> > gen3.yaml
> > 
> > Did you get the same error when adding the PCIe node for MT7986?
> 
> i'm sure i had tested the yaml and did not get any error, but on my
> retest i get same error for mt7986 too...
> 
> maybe the right way is to remove the contains in the mediatek,mt8192-
> pcie if condition (to match only if this condition is no fallback),
> then it is clean for me. Can you test it with your patches?

Sorry for the late response, I have tested with the patch for MT8195,
and it works, thanks!

> 
> allOf:
>   - $ref: /schemas/pci/pci-bus.yaml#
>   - if:
>       properties:
>         compatible:
>           #contains:
>           const: mediatek,mt8192-pcie
>     then:
>       properties:
>         clock-names:
>           items:
>             - const: pl_250m
>             - const: tl_26m
>             - const: tl_96m
>             - const: tl_32k
>             - const: peri_26m
>             - const: top_133m
> 
> 
> regards Frank
> 
> regards Frank

[PATCH v2 2/2] dt-bindings: PCI: mediatek-gen3: add support for mt7986

[Frank Wunderlich]

From: Frank Wunderlich <frank-w@public-files.de>

Add compatible string and clock-definition for mt7986. It needs 4 clocks
for PCIe, define them in binding.

Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
---
v2:
- squashed patch 2+3 (compatible and clock definition)
---
 .../bindings/pci/mediatek-pcie-gen3.yaml        | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml b/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml
index 98d3f0f1cd76..57d0e84253e9 100644
--- a/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml
+++ b/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml
@@ -48,6 +48,7 @@ properties:
     oneOf:
       - items:
           - enum:
+              - mediatek,mt7986-pcie
               - mediatek,mt8188-pcie
               - mediatek,mt8195-pcie
           - const: mediatek,mt8192-pcie
@@ -78,9 +79,11 @@ properties:
       - const: mac
 
   clocks:
+    minItems: 4
     maxItems: 6
 
   clock-names:
+    minItems: 4
     maxItems: 6
 
   assigned-clocks:
@@ -161,6 +164,20 @@ allOf:
             - const: tl_32k
             - const: peri_26m
             - const: peri_mem
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - mediatek,mt7986-pcie
+    then:
+      properties:
+        clock-names:
+          items:
+            - const: pl_250m
+            - const: tl_26m
+            - const: peri_26m
+            - const: top_133m
 
 unevaluatedProperties: false
 
-- 
2.34.1

Re: [PATCH v2 2/2] dt-bindings: PCI: mediatek-gen3: add support for mt7986

[AngeloGioacchino Del Regno]

Il 25/10/22 09:28, Frank Wunderlich ha scritto:
> From: Frank Wunderlich <frank-w@public-files.de>
> 
> Add compatible string and clock-definition for mt7986. It needs 4 clocks
> for PCIe, define them in binding.
> 
> Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
> ---
> v2:
> - squashed patch 2+3 (compatible and clock definition)
> ---
>   .../bindings/pci/mediatek-pcie-gen3.yaml        | 17 +++++++++++++++++
>   1 file changed, 17 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml b/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml
> index 98d3f0f1cd76..57d0e84253e9 100644
> --- a/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml
> +++ b/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml
> @@ -48,6 +48,7 @@ properties:
>       oneOf:
>         - items:
>             - enum:
> +              - mediatek,mt7986-pcie
>                 - mediatek,mt8188-pcie
>                 - mediatek,mt8195-pcie
>             - const: mediatek,mt8192-pcie
> @@ -78,9 +79,11 @@ properties:
>         - const: mac
>   
>     clocks:
> +    minItems: 4
>       maxItems: 6

I'm not sure that this is really correct.
If you have a list of items (const or description, doesn't matter), you don't have
to specify maxItems, as it is implicitly equal to the number of items.
Also, specifying minItems means that you're "setting" one or more items as
optional.

It looks like you're specifying a specific and definite list of items for both
clocks and clock-names... and for all of the supported SoCs, so, I don't think
that having {min,max}Items globally specified on clocks, clock-names make any
kind of sense.

.....but I'd like for someone else to give an opinion on this as well.

Krzysztof, please?

Cheers,
Angelo

>   
>     clock-names:
> +    minItems: 4
>       maxItems: 6
>   
>     assigned-clocks:
> @@ -161,6 +164,20 @@ allOf:
>               - const: tl_32k
>               - const: peri_26m
>               - const: peri_mem
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - mediatek,mt7986-pcie
> +    then:
> +      properties:
> +        clock-names:
> +          items:
> +            - const: pl_250m
> +            - const: tl_26m
> +            - const: peri_26m
> +            - const: top_133m
>   
>   unevaluatedProperties: false
>   
> 

Re: [PATCH v2 2/2] dt-bindings: PCI: mediatek-gen3: add support for mt7986

[Rob Herring]

On Tue, Oct 25, 2022 at 01:10:56PM +0200, AngeloGioacchino Del Regno wrote:
> Il 25/10/22 09:28, Frank Wunderlich ha scritto:
> > From: Frank Wunderlich <frank-w@public-files.de>
> > 
> > Add compatible string and clock-definition for mt7986. It needs 4 clocks
> > for PCIe, define them in binding.
> > 
> > Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
> > ---
> > v2:
> > - squashed patch 2+3 (compatible and clock definition)
> > ---
> >   .../bindings/pci/mediatek-pcie-gen3.yaml        | 17 +++++++++++++++++
> >   1 file changed, 17 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml b/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml
> > index 98d3f0f1cd76..57d0e84253e9 100644
> > --- a/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml
> > +++ b/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml
> > @@ -48,6 +48,7 @@ properties:
> >       oneOf:
> >         - items:
> >             - enum:
> > +              - mediatek,mt7986-pcie
> >                 - mediatek,mt8188-pcie
> >                 - mediatek,mt8195-pcie
> >             - const: mediatek,mt8192-pcie
> > @@ -78,9 +79,11 @@ properties:
> >         - const: mac
> >     clocks:
> > +    minItems: 4
> >       maxItems: 6
> 
> I'm not sure that this is really correct.
> If you have a list of items (const or description, doesn't matter), you don't have
> to specify maxItems, as it is implicitly equal to the number of items.
> Also, specifying minItems means that you're "setting" one or more items as
> optional.
> 
> It looks like you're specifying a specific and definite list of items for both
> clocks and clock-names... and for all of the supported SoCs, so, I don't think
> that having {min,max}Items globally specified on clocks, clock-names make any
> kind of sense.
> 
> .....but I'd like for someone else to give an opinion on this as well.

It does make sense. Globally, we have the full range of possible clocks 
and then the if/then schemas further restrict it.

Rob

Re: [PATCH v2 2/2] dt-bindings: PCI: mediatek-gen3: add support for mt7986

[Rob Herring]

On Tue, 25 Oct 2022 09:28:37 +0200, Frank Wunderlich wrote:
> From: Frank Wunderlich <frank-w@public-files.de>
> 
> Add compatible string and clock-definition for mt7986. It needs 4 clocks
> for PCIe, define them in binding.
> 
> Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
> ---
> v2:
> - squashed patch 2+3 (compatible and clock definition)
> ---
>  .../bindings/pci/mediatek-pcie-gen3.yaml        | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>