diff mbox series

[v1,1/5] dt-bindings: PCI: qcom-ep: Add support for SA8775P SoC

Message ID 1695218113-31198-2-git-send-email-quic_msarkar@quicinc.com
State New
Headers
Series arm64: qcom: sa8775p: add support for EP PCIe |

Commit Message

Mrinmay Sarkar Sept. 20, 2023, 1:55 p.m. UTC 
  Add devicetree bindings support for SA8775P SoC.
Define reg and interrupt per platform.

Signed-off-by: Mrinmay Sarkar <quic_msarkar@quicinc.com>
---
 .../devicetree/bindings/pci/qcom,pcie-ep.yaml      | 130 +++++++++++++++++----
 1 file changed, 108 insertions(+), 22 deletions(-)

Comments

Manivannan Sadhasivam Sept. 21, 2023, 8:11 a.m. UTC | #1 
On Wed, Sep 20, 2023 at 07:25:08PM +0530, Mrinmay Sarkar wrote:
> Add devicetree bindings support for SA8775P SoC.
> Define reg and interrupt per platform.
> 
> Signed-off-by: Mrinmay Sarkar <quic_msarkar@quicinc.com>
> ---
>  .../devicetree/bindings/pci/qcom,pcie-ep.yaml      | 130 +++++++++++++++++----
>  1 file changed, 108 insertions(+), 22 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml
> index a223ce0..e860e8f 100644
> --- a/Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml
> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml
> @@ -13,6 +13,7 @@ properties:
>    compatible:
>      oneOf:
>        - enum:
> +          - qcom,sa8775p-pcie-ep
>            - qcom,sdx55-pcie-ep
>            - qcom,sm8450-pcie-ep
>        - items:
> @@ -20,29 +21,19 @@ properties:
>            - const: qcom,sdx55-pcie-ep
>  
>    reg:
> -    items:
> -      - description: Qualcomm-specific PARF configuration registers
> -      - description: DesignWare PCIe registers
> -      - description: External local bus interface registers
> -      - description: Address Translation Unit (ATU) registers
> -      - description: Memory region used to map remote RC address space
> -      - description: BAR memory region
> +    minItems: 6
> +    maxItems: 7
>  
>    reg-names:
> -    items:
> -      - const: parf
> -      - const: dbi
> -      - const: elbi
> -      - const: atu
> -      - const: addr_space
> -      - const: mmio
> +    minItems: 6
> +    maxItems: 7
>  
>    clocks:
> -    minItems: 7
> +    minItems: 5
>      maxItems: 8
>  
>    clock-names:
> -    minItems: 7
> +    minItems: 5
>      maxItems: 8
>  
>    qcom,perst-regs:
> @@ -57,14 +48,12 @@ properties:
>            - description: Perst separation enable offset
>  
>    interrupts:
> -    items:
> -      - description: PCIe Global interrupt
> -      - description: PCIe Doorbell interrupt
> +    minItems: 2
> +    maxItems: 3
>  
>    interrupt-names:
> -    items:
> -      - const: global
> -      - const: doorbell
> +    minItems: 2
> +    maxItems: 3
>  
>    reset-gpios:
>      description: GPIO used as PERST# input signal
> @@ -122,6 +111,51 @@ allOf:
>          compatible:
>            contains:
>              enum:
> +              - qcom,sa8775p-pcie-ep
> +    then:
> +      properties:
> +        reg:
> +          items:
> +            - description: Qualcomm-specific PARF configuration registers
> +            - description: DesignWare PCIe registers
> +            - description: External local bus interface registers
> +            - description: Address Translation Unit (ATU) registers
> +            - description: Memory region used to map remote RC address space
> +            - description: BAR memory region
> +            - description: DMA memory region

It should be described as "DMA register space" or something, because this could
be misinterpreted as memory region for doing DMA.

- Mani
Rob Herring Sept. 21, 2023, 6:38 p.m. UTC | #2 
On Wed, Sep 20, 2023 at 07:25:08PM +0530, Mrinmay Sarkar wrote:
> Add devicetree bindings support for SA8775P SoC.
> Define reg and interrupt per platform.
> 
> Signed-off-by: Mrinmay Sarkar <quic_msarkar@quicinc.com>
> ---
>  .../devicetree/bindings/pci/qcom,pcie-ep.yaml      | 130 +++++++++++++++++----
>  1 file changed, 108 insertions(+), 22 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml
> index a223ce0..e860e8f 100644
> --- a/Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml
> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml
> @@ -13,6 +13,7 @@ properties:
>    compatible:
>      oneOf:
>        - enum:
> +          - qcom,sa8775p-pcie-ep
>            - qcom,sdx55-pcie-ep
>            - qcom,sm8450-pcie-ep
>        - items:
> @@ -20,29 +21,19 @@ properties:
>            - const: qcom,sdx55-pcie-ep
>  
>    reg:
> -    items:
> -      - description: Qualcomm-specific PARF configuration registers
> -      - description: DesignWare PCIe registers
> -      - description: External local bus interface registers
> -      - description: Address Translation Unit (ATU) registers
> -      - description: Memory region used to map remote RC address space
> -      - description: BAR memory region
> +    minItems: 6
> +    maxItems: 7
>  
>    reg-names:
> -    items:
> -      - const: parf
> -      - const: dbi
> -      - const: elbi
> -      - const: atu
> -      - const: addr_space
> -      - const: mmio
> +    minItems: 6
> +    maxItems: 7

Don't move these into if/then schemas. Then we are duplicating the 
names, and there is no reason to keep them aligned for new compatibles.

Rob
Shazad Hussain Oct. 6, 2023, 10:54 a.m. UTC | #3 
On 9/22/2023 12:08 AM, Rob Herring wrote:
> On Wed, Sep 20, 2023 at 07:25:08PM +0530, Mrinmay Sarkar wrote:
>> Add devicetree bindings support for SA8775P SoC.
>> Define reg and interrupt per platform.
>>
>> Signed-off-by: Mrinmay Sarkar <quic_msarkar@quicinc.com>
>> ---
>>   .../devicetree/bindings/pci/qcom,pcie-ep.yaml      | 130 +++++++++++++++++----
>>   1 file changed, 108 insertions(+), 22 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml
>> index a223ce0..e860e8f 100644
>> --- a/Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml
>> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml
>> @@ -13,6 +13,7 @@ properties:
>>     compatible:
>>       oneOf:
>>         - enum:
>> +          - qcom,sa8775p-pcie-ep
>>             - qcom,sdx55-pcie-ep
>>             - qcom,sm8450-pcie-ep
>>         - items:
>> @@ -20,29 +21,19 @@ properties:
>>             - const: qcom,sdx55-pcie-ep
>>   
>>     reg:
>> -    items:
>> -      - description: Qualcomm-specific PARF configuration registers
>> -      - description: DesignWare PCIe registers
>> -      - description: External local bus interface registers
>> -      - description: Address Translation Unit (ATU) registers
>> -      - description: Memory region used to map remote RC address space
>> -      - description: BAR memory region
>> +    minItems: 6
>> +    maxItems: 7
>>   
>>     reg-names:
>> -    items:
>> -      - const: parf
>> -      - const: dbi
>> -      - const: elbi
>> -      - const: atu
>> -      - const: addr_space
>> -      - const: mmio
>> +    minItems: 6
>> +    maxItems: 7
> 
> Don't move these into if/then schemas. Then we are duplicating the
> names, and there is no reason to keep them aligned for new compatibles.
> 
> Rob

Hi Rob,
As we have one extra reg property (dma) required for sa8775p-pcie-ep,
isn't it expected to be moved in if/then as per number of regs
required. Anyways we would have duplication of some properties for new
compatibles where the member numbers differs for a property.

Are you suggesting to add the extra reg property (dma) in the existing 
reg and reg-names list, and add minItems/maxItems for all compatibles 
present in this file ?

-Shazad
Mrinmay Sarkar Oct. 11, 2023, 11:13 a.m. UTC | #4 
On 10/6/2023 4:24 PM, Shazad Hussain wrote:
>
>
> On 9/22/2023 12:08 AM, Rob Herring wrote:
>> On Wed, Sep 20, 2023 at 07:25:08PM +0530, Mrinmay Sarkar wrote:
>>> Add devicetree bindings support for SA8775P SoC.
>>> Define reg and interrupt per platform.
>>>
>>> Signed-off-by: Mrinmay Sarkar <quic_msarkar@quicinc.com>
>>> ---
>>>   .../devicetree/bindings/pci/qcom,pcie-ep.yaml      | 130 
>>> +++++++++++++++++----
>>>   1 file changed, 108 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml 
>>> b/Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml
>>> index a223ce0..e860e8f 100644
>>> --- a/Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml
>>> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml
>>> @@ -13,6 +13,7 @@ properties:
>>>     compatible:
>>>       oneOf:
>>>         - enum:
>>> +          - qcom,sa8775p-pcie-ep
>>>             - qcom,sdx55-pcie-ep
>>>             - qcom,sm8450-pcie-ep
>>>         - items:
>>> @@ -20,29 +21,19 @@ properties:
>>>             - const: qcom,sdx55-pcie-ep
>>>       reg:
>>> -    items:
>>> -      - description: Qualcomm-specific PARF configuration registers
>>> -      - description: DesignWare PCIe registers
>>> -      - description: External local bus interface registers
>>> -      - description: Address Translation Unit (ATU) registers
>>> -      - description: Memory region used to map remote RC address space
>>> -      - description: BAR memory region
>>> +    minItems: 6
>>> +    maxItems: 7
>>>       reg-names:
>>> -    items:
>>> -      - const: parf
>>> -      - const: dbi
>>> -      - const: elbi
>>> -      - const: atu
>>> -      - const: addr_space
>>> -      - const: mmio
>>> +    minItems: 6
>>> +    maxItems: 7
>>
>> Don't move these into if/then schemas. Then we are duplicating the
>> names, and there is no reason to keep them aligned for new compatibles.
>>
>> Rob
>
> Hi Rob,
> As we have one extra reg property (dma) required for sa8775p-pcie-ep,
> isn't it expected to be moved in if/then as per number of regs
> required. Anyways we would have duplication of some properties for new
> compatibles where the member numbers differs for a property.
>
> Are you suggesting to add the extra reg property (dma) in the existing 
> reg and reg-names list, and add minItems/maxItems for all compatibles 
> present in this file ?
>
> -Shazad

Here we have defined reg and interrupt per platform as clocks is defined.

-Mrinmay
Dmitry Baryshkov Oct. 11, 2023, 11:43 a.m. UTC | #5 
On Wed, 11 Oct 2023 at 14:14, Mrinmay Sarkar <quic_msarkar@quicinc.com> wrote:
>
>
> On 10/6/2023 4:24 PM, Shazad Hussain wrote:
> >
> >
> > On 9/22/2023 12:08 AM, Rob Herring wrote:
> >> On Wed, Sep 20, 2023 at 07:25:08PM +0530, Mrinmay Sarkar wrote:
> >>> Add devicetree bindings support for SA8775P SoC.
> >>> Define reg and interrupt per platform.
> >>>
> >>> Signed-off-by: Mrinmay Sarkar <quic_msarkar@quicinc.com>
> >>> ---
> >>>   .../devicetree/bindings/pci/qcom,pcie-ep.yaml      | 130
> >>> +++++++++++++++++----
> >>>   1 file changed, 108 insertions(+), 22 deletions(-)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml
> >>> b/Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml
> >>> index a223ce0..e860e8f 100644
> >>> --- a/Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml
> >>> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml
> >>> @@ -13,6 +13,7 @@ properties:
> >>>     compatible:
> >>>       oneOf:
> >>>         - enum:
> >>> +          - qcom,sa8775p-pcie-ep
> >>>             - qcom,sdx55-pcie-ep
> >>>             - qcom,sm8450-pcie-ep
> >>>         - items:
> >>> @@ -20,29 +21,19 @@ properties:
> >>>             - const: qcom,sdx55-pcie-ep
> >>>       reg:
> >>> -    items:
> >>> -      - description: Qualcomm-specific PARF configuration registers
> >>> -      - description: DesignWare PCIe registers
> >>> -      - description: External local bus interface registers
> >>> -      - description: Address Translation Unit (ATU) registers
> >>> -      - description: Memory region used to map remote RC address space
> >>> -      - description: BAR memory region
> >>> +    minItems: 6
> >>> +    maxItems: 7
> >>>       reg-names:
> >>> -    items:
> >>> -      - const: parf
> >>> -      - const: dbi
> >>> -      - const: elbi
> >>> -      - const: atu
> >>> -      - const: addr_space
> >>> -      - const: mmio
> >>> +    minItems: 6
> >>> +    maxItems: 7
> >>
> >> Don't move these into if/then schemas. Then we are duplicating the
> >> names, and there is no reason to keep them aligned for new compatibles.
> >>
> >> Rob
> >
> > Hi Rob,
> > As we have one extra reg property (dma) required for sa8775p-pcie-ep,
> > isn't it expected to be moved in if/then as per number of regs
> > required. Anyways we would have duplication of some properties for new
> > compatibles where the member numbers differs for a property.
> >
> > Are you suggesting to add the extra reg property (dma) in the existing
> > reg and reg-names list, and add minItems/maxItems for all compatibles
> > present in this file ?

This is what we have been doing in other cases: if the list is an
extension of the current list, there is no need to duplicate it. One
can use min/maxItems instead.

> >
> > -Shazad
>
> Here we have defined reg and interrupt per platform as clocks is defined.
>
> -Mrinmay
>
Mrinmay Sarkar Oct. 13, 2023, 12:55 p.m. UTC | #6 
On 10/11/2023 5:13 PM, Dmitry Baryshkov wrote:
> On Wed, 11 Oct 2023 at 14:14, Mrinmay Sarkar <quic_msarkar@quicinc.com> wrote:
>>
>> On 10/6/2023 4:24 PM, Shazad Hussain wrote:
>>>
>>> On 9/22/2023 12:08 AM, Rob Herring wrote:
>>>> On Wed, Sep 20, 2023 at 07:25:08PM +0530, Mrinmay Sarkar wrote:
>>>>> Add devicetree bindings support for SA8775P SoC.
>>>>> Define reg and interrupt per platform.
>>>>>
>>>>> Signed-off-by: Mrinmay Sarkar <quic_msarkar@quicinc.com>
>>>>> ---
>>>>>    .../devicetree/bindings/pci/qcom,pcie-ep.yaml      | 130
>>>>> +++++++++++++++++----
>>>>>    1 file changed, 108 insertions(+), 22 deletions(-)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml
>>>>> b/Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml
>>>>> index a223ce0..e860e8f 100644
>>>>> --- a/Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml
>>>>> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml
>>>>> @@ -13,6 +13,7 @@ properties:
>>>>>      compatible:
>>>>>        oneOf:
>>>>>          - enum:
>>>>> +          - qcom,sa8775p-pcie-ep
>>>>>              - qcom,sdx55-pcie-ep
>>>>>              - qcom,sm8450-pcie-ep
>>>>>          - items:
>>>>> @@ -20,29 +21,19 @@ properties:
>>>>>              - const: qcom,sdx55-pcie-ep
>>>>>        reg:
>>>>> -    items:
>>>>> -      - description: Qualcomm-specific PARF configuration registers
>>>>> -      - description: DesignWare PCIe registers
>>>>> -      - description: External local bus interface registers
>>>>> -      - description: Address Translation Unit (ATU) registers
>>>>> -      - description: Memory region used to map remote RC address space
>>>>> -      - description: BAR memory region
>>>>> +    minItems: 6
>>>>> +    maxItems: 7
>>>>>        reg-names:
>>>>> -    items:
>>>>> -      - const: parf
>>>>> -      - const: dbi
>>>>> -      - const: elbi
>>>>> -      - const: atu
>>>>> -      - const: addr_space
>>>>> -      - const: mmio
>>>>> +    minItems: 6
>>>>> +    maxItems: 7
>>>> Don't move these into if/then schemas. Then we are duplicating the
>>>> names, and there is no reason to keep them aligned for new compatibles.
>>>>
>>>> Rob
>>> Hi Rob,
>>> As we have one extra reg property (dma) required for sa8775p-pcie-ep,
>>> isn't it expected to be moved in if/then as per number of regs
>>> required. Anyways we would have duplication of some properties for new
>>> compatibles where the member numbers differs for a property.
>>>
>>> Are you suggesting to add the extra reg property (dma) in the existing
>>> reg and reg-names list, and add minItems/maxItems for all compatibles
>>> present in this file ?
> This is what we have been doing in other cases: if the list is an
> extension of the current list, there is no need to duplicate it. One
> can use min/maxItems instead.
Hi Dmitry

we have tried using min/maxItems rather than duplicating but somehow
catch up with some warnings in dt_bindings check

//local/mnt/workspace/Mrinmay/lemans/next-20230914/linux-next/out/Documentation/devicetree/bindings/pci/qcom,pcie-ep.example.dtb: 
pcie-ep@1c00000: reg: [[29360128, 12288], [1073741824, 3869], 
[1073745696, 200], [1073745920, 4096], [1073750016, 4096], [29372416, 
12288]] is too short//
//        from schema $id: 
http://devicetree.org/schemas/pci/qcom,pcie-ep.yaml#//
///local/mnt/workspace/Mrinmay/lemans/next-20230914/linux-next/out/Documentation/devicetree/bindings/pci/qcom,pcie-ep.example.dtb: 
pcie-ep@1c00000: reg-names: ['parf', 'dbi', 'elbi', 'atu', 'addr_space', 
'mmio'] is too short//
//        from schema $id: 
http://devicetree.org/schemas/pci/qcom,pcie-ep.yaml#//
///local/mnt/workspace/Mrinmay/lemans/next-20230914/linux-next/out/Documentation/devicetree/bindings/pci/qcom,pcie-ep.example.dtb: 
pcie-ep@1c00000: interrupts: [[0, 140, 4], [0, 145, 4]] is too short//
//        from schema $id: 
http://devicetree.org/schemas/pci/qcom,pcie-ep.yaml#//
///local/mnt/workspace/Mrinmay/lemans/next-20230914/linux-next/out/Documentation/devicetree/bindings/pci/qcom,pcie-ep.example.dtb: 
pcie-ep@1c00000: interrupt-names: ['global', 'doorbell'] is too short//
//        from schema $id: 
http://devicetree.org/schemas/pci/qcom,pcie-ep.yaml#//
/

//local/mnt/workspace/Mrinmay/lemans/next-20230914/linux-next/out/Documentation/devicetree/bindings/pci/qcom,pcie-ep.example.dtb: 
pcie-ep@1c00000: interrupt-names: ['global', 'doorbell'] is too short/

added the patch in attachment.

--Mrinmay

>>> -Shazad
>> Here we have defined reg and interrupt per platform as clocks is defined.
>>
>> -Mrinmay
>>
>
From 520653ae6996366942f21a8942b5d8ac33e30ee3 Mon Sep 17 00:00:00 2001
From: Mrinmay Sarkar <quic_msarkar@quicinc.com>
Date: Fri, 13 Oct 2023 18:09:56 +0530
Subject: [PATCH] dt-bindings: PCI: qcom-ep: Add support for SA8775P SoC

Add devicetree bindings support for SA8775P SoC.

Signed-off-by: Mrinmay Sarkar <quic_msarkar@quicinc.com>
---
 .../devicetree/bindings/pci/qcom,pcie-ep.yaml | 73 ++++++++++++++++++-
 1 file changed, 70 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml
index a223ce029cab..00eef92685a2 100644
--- a/Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml
+++ b/Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml
@@ -13,9 +13,11 @@ properties:
   compatible:
     oneOf:
       - enum:
+          - qcom,sa8775p-pcie-ep
           - qcom,sdx55-pcie-ep
           - qcom,sm8450-pcie-ep
       - items:
+          - const: qcom,sa8775p-pcie-ep
           - const: qcom,sdx65-pcie-ep
           - const: qcom,sdx55-pcie-ep
 
@@ -27,6 +29,7 @@ properties:
       - description: Address Translation Unit (ATU) registers
       - description: Memory region used to map remote RC address space
       - description: BAR memory region
+      - description: DMA register space
 
   reg-names:
     items:
@@ -36,13 +39,14 @@ properties:
       - const: atu
       - const: addr_space
       - const: mmio
+      - const: dma
 
   clocks:
-    minItems: 7
+    minItems: 5
     maxItems: 8
 
   clock-names:
-    minItems: 7
+    minItems: 5
     maxItems: 8
 
   qcom,perst-regs:
@@ -60,11 +64,13 @@ properties:
     items:
       - description: PCIe Global interrupt
       - description: PCIe Doorbell interrupt
+      - description: DMA interrupt
 
   interrupt-names:
     items:
       - const: global
       - const: doorbell
+      - const: dma
 
   reset-gpios:
     description: GPIO used as PERST# input signal
@@ -125,7 +131,13 @@ allOf:
               - qcom,sdx55-pcie-ep
     then:
       properties:
-        clocks:
+        reg:
+          maxItems: 6
+          minItems: 6
+        reg-names:
+          maxItems: 6
+          minItems: 6
+          clocks:
           items:
             - description: PCIe Auxiliary clock
             - description: PCIe CFG AHB clock
@@ -143,6 +155,12 @@ allOf:
             - const: slave_q2a
             - const: sleep
             - const: ref
+        interrupts:
+          maxItems: 2
+          minItems: 2
+        interrupt-names:
+          maxItems: 3
+          minItems: 3
 
   - if:
       properties:
@@ -152,6 +170,13 @@ allOf:
               - qcom,sm8450-pcie-ep
     then:
       properties:
+        properties:
+          reg:
+            maxItems: 6
+            minItems: 6
+          reg-names:
+            maxItems: 6
+            minItems: 6
         clocks:
           items:
             - description: PCIe Auxiliary clock
@@ -172,6 +197,48 @@ allOf:
             - const: ref
             - const: ddrss_sf_tbu
             - const: aggre_noc_axi
+        interrupts:
+          maxItems: 2
+          minItems: 2
+        interrupt-names:
+          maxItems: 3
+          minItems: 3
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - qcom,sa8775p-pcie-ep
+    then:
+      properties:
+        properties:
+        reg:
+          maxItems: 7
+          minItems: 7
+        reg-names:
+          maxItems: 7
+          minItems: 7
+        clocks:
+          items:
+            - description: PCIe Auxiliary clock
+            - description: PCIe CFG AHB clock
+            - description: PCIe Master AXI clock
+            - description: PCIe Slave AXI clock
+            - description: PCIe Slave Q2A AXI clock
+        clock-names:
+          items:
+            - const: aux
+            - const: cfg
+            - const: bus_master
+            - const: bus_slave
+            - const: slave_q2a
+        interrupts:
+          maxItems: 3
+          minItems: 3
+        interrupt-names:
+          maxItems: 3
+          minItems: 3
 
 unevaluatedProperties: false
Dmitry Baryshkov Oct. 13, 2023, 4:38 p.m. UTC | #7 
On Fri, 13 Oct 2023 at 15:55, Mrinmay Sarkar <quic_msarkar@quicinc.com> wrote:
>
>
> On 10/11/2023 5:13 PM, Dmitry Baryshkov wrote:
> > On Wed, 11 Oct 2023 at 14:14, Mrinmay Sarkar <quic_msarkar@quicinc.com> wrote:
> >>
> >> On 10/6/2023 4:24 PM, Shazad Hussain wrote:
> >>>
> >>> On 9/22/2023 12:08 AM, Rob Herring wrote:
> >>>> On Wed, Sep 20, 2023 at 07:25:08PM +0530, Mrinmay Sarkar wrote:
> >>>>> Add devicetree bindings support for SA8775P SoC.
> >>>>> Define reg and interrupt per platform.
> >>>>>
> >>>>> Signed-off-by: Mrinmay Sarkar <quic_msarkar@quicinc.com>
> >>>>> ---
> >>>>>    .../devicetree/bindings/pci/qcom,pcie-ep.yaml      | 130
> >>>>> +++++++++++++++++----
> >>>>>    1 file changed, 108 insertions(+), 22 deletions(-)
> >>>>>
> >>>>> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml
> >>>>> b/Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml
> >>>>> index a223ce0..e860e8f 100644
> >>>>> --- a/Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml
> >>>>> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml
> >>>>> @@ -13,6 +13,7 @@ properties:
> >>>>>      compatible:
> >>>>>        oneOf:
> >>>>>          - enum:
> >>>>> +          - qcom,sa8775p-pcie-ep
> >>>>>              - qcom,sdx55-pcie-ep
> >>>>>              - qcom,sm8450-pcie-ep
> >>>>>          - items:
> >>>>> @@ -20,29 +21,19 @@ properties:
> >>>>>              - const: qcom,sdx55-pcie-ep
> >>>>>        reg:
> >>>>> -    items:
> >>>>> -      - description: Qualcomm-specific PARF configuration registers
> >>>>> -      - description: DesignWare PCIe registers
> >>>>> -      - description: External local bus interface registers
> >>>>> -      - description: Address Translation Unit (ATU) registers
> >>>>> -      - description: Memory region used to map remote RC address space
> >>>>> -      - description: BAR memory region
> >>>>> +    minItems: 6
> >>>>> +    maxItems: 7
> >>>>>        reg-names:
> >>>>> -    items:
> >>>>> -      - const: parf
> >>>>> -      - const: dbi
> >>>>> -      - const: elbi
> >>>>> -      - const: atu
> >>>>> -      - const: addr_space
> >>>>> -      - const: mmio
> >>>>> +    minItems: 6
> >>>>> +    maxItems: 7
> >>>> Don't move these into if/then schemas. Then we are duplicating the
> >>>> names, and there is no reason to keep them aligned for new compatibles.
> >>>>
> >>>> Rob
> >>> Hi Rob,
> >>> As we have one extra reg property (dma) required for sa8775p-pcie-ep,
> >>> isn't it expected to be moved in if/then as per number of regs
> >>> required. Anyways we would have duplication of some properties for new
> >>> compatibles where the member numbers differs for a property.
> >>>
> >>> Are you suggesting to add the extra reg property (dma) in the existing
> >>> reg and reg-names list, and add minItems/maxItems for all compatibles
> >>> present in this file ?
> > This is what we have been doing in other cases: if the list is an
> > extension of the current list, there is no need to duplicate it. One
> > can use min/maxItems instead.
> Hi Dmitry
>
> we have tried using min/maxItems rather than duplicating but somehow
> catch up with some warnings in dt_bindings check
>
> //local/mnt/workspace/Mrinmay/lemans/next-20230914/linux-next/out/Documentation/devicetree/bindings/pci/qcom,pcie-ep.example.dtb:
> pcie-ep@1c00000: reg: [[29360128, 12288], [1073741824, 3869],
> [1073745696, 200], [1073745920, 4096], [1073750016, 4096], [29372416,
> 12288]] is too short//
> //        from schema $id:
> http://devicetree.org/schemas/pci/qcom,pcie-ep.yaml#//
> ///local/mnt/workspace/Mrinmay/lemans/next-20230914/linux-next/out/Documentation/devicetree/bindings/pci/qcom,pcie-ep.example.dtb:
> pcie-ep@1c00000: reg-names: ['parf', 'dbi', 'elbi', 'atu', 'addr_space',
> 'mmio'] is too short//
> //        from schema $id:

missing min/maxItems for reg and reg-names

> http://devicetree.org/schemas/pci/qcom,pcie-ep.yaml#//
> ///local/mnt/workspace/Mrinmay/lemans/next-20230914/linux-next/out/Documentation/devicetree/bindings/pci/qcom,pcie-ep.example.dtb:
> pcie-ep@1c00000: interrupts: [[0, 140, 4], [0, 145, 4]] is too short//
> //        from schema $id:
> http://devicetree.org/schemas/pci/qcom,pcie-ep.yaml#//
> ///local/mnt/workspace/Mrinmay/lemans/next-20230914/linux-next/out/Documentation/devicetree/bindings/pci/qcom,pcie-ep.example.dtb:
> pcie-ep@1c00000: interrupt-names: ['global', 'doorbell'] is too short//
> //        from schema $id:
> http://devicetree.org/schemas/pci/qcom,pcie-ep.yaml#//

incorrect min/maxItems for interrupts.

> //local/mnt/workspace/Mrinmay/lemans/next-20230914/linux-next/out/Documentation/devicetree/bindings/pci/qcom,pcie-ep.example.dtb:
> pcie-ep@1c00000: interrupt-names: ['global', 'doorbell'] is too short/
>
> added the patch in attachment.
>
> --Mrinmay
>
> >>> -Shazad
> >> Here we have defined reg and interrupt per platform as clocks is defined.
> >>
> >> -Mrinmay
> >>
> >
Mrinmay Sarkar Oct. 16, 2023, 4:24 a.m. UTC | #8 
On 10/13/2023 10:08 PM, Dmitry Baryshkov wrote:
> On Fri, 13 Oct 2023 at 15:55, Mrinmay Sarkar <quic_msarkar@quicinc.com> wrote:
>>
>> On 10/11/2023 5:13 PM, Dmitry Baryshkov wrote:
>>> On Wed, 11 Oct 2023 at 14:14, Mrinmay Sarkar <quic_msarkar@quicinc.com> wrote:
>>>> On 10/6/2023 4:24 PM, Shazad Hussain wrote:
>>>>> On 9/22/2023 12:08 AM, Rob Herring wrote:
>>>>>> On Wed, Sep 20, 2023 at 07:25:08PM +0530, Mrinmay Sarkar wrote:
>>>>>>> Add devicetree bindings support for SA8775P SoC.
>>>>>>> Define reg and interrupt per platform.
>>>>>>>
>>>>>>> Signed-off-by: Mrinmay Sarkar <quic_msarkar@quicinc.com>
>>>>>>> ---
>>>>>>>     .../devicetree/bindings/pci/qcom,pcie-ep.yaml      | 130
>>>>>>> +++++++++++++++++----
>>>>>>>     1 file changed, 108 insertions(+), 22 deletions(-)
>>>>>>>
>>>>>>> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml
>>>>>>> b/Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml
>>>>>>> index a223ce0..e860e8f 100644
>>>>>>> --- a/Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml
>>>>>>> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml
>>>>>>> @@ -13,6 +13,7 @@ properties:
>>>>>>>       compatible:
>>>>>>>         oneOf:
>>>>>>>           - enum:
>>>>>>> +          - qcom,sa8775p-pcie-ep
>>>>>>>               - qcom,sdx55-pcie-ep
>>>>>>>               - qcom,sm8450-pcie-ep
>>>>>>>           - items:
>>>>>>> @@ -20,29 +21,19 @@ properties:
>>>>>>>               - const: qcom,sdx55-pcie-ep
>>>>>>>         reg:
>>>>>>> -    items:
>>>>>>> -      - description: Qualcomm-specific PARF configuration registers
>>>>>>> -      - description: DesignWare PCIe registers
>>>>>>> -      - description: External local bus interface registers
>>>>>>> -      - description: Address Translation Unit (ATU) registers
>>>>>>> -      - description: Memory region used to map remote RC address space
>>>>>>> -      - description: BAR memory region
>>>>>>> +    minItems: 6
>>>>>>> +    maxItems: 7
>>>>>>>         reg-names:
>>>>>>> -    items:
>>>>>>> -      - const: parf
>>>>>>> -      - const: dbi
>>>>>>> -      - const: elbi
>>>>>>> -      - const: atu
>>>>>>> -      - const: addr_space
>>>>>>> -      - const: mmio
>>>>>>> +    minItems: 6
>>>>>>> +    maxItems: 7
>>>>>> Don't move these into if/then schemas. Then we are duplicating the
>>>>>> names, and there is no reason to keep them aligned for new compatibles.
>>>>>>
>>>>>> Rob
>>>>> Hi Rob,
>>>>> As we have one extra reg property (dma) required for sa8775p-pcie-ep,
>>>>> isn't it expected to be moved in if/then as per number of regs
>>>>> required. Anyways we would have duplication of some properties for new
>>>>> compatibles where the member numbers differs for a property.
>>>>>
>>>>> Are you suggesting to add the extra reg property (dma) in the existing
>>>>> reg and reg-names list, and add minItems/maxItems for all compatibles
>>>>> present in this file ?
>>> This is what we have been doing in other cases: if the list is an
>>> extension of the current list, there is no need to duplicate it. One
>>> can use min/maxItems instead.
>> Hi Dmitry
>>
>> we have tried using min/maxItems rather than duplicating but somehow
>> catch up with some warnings in dt_bindings check
>>
>> //local/mnt/workspace/Mrinmay/lemans/next-20230914/linux-next/out/Documentation/devicetree/bindings/pci/qcom,pcie-ep.example.dtb:
>> pcie-ep@1c00000: reg: [[29360128, 12288], [1073741824, 3869],
>> [1073745696, 200], [1073745920, 4096], [1073750016, 4096], [29372416,
>> 12288]] is too short//
>> //        from schema $id:
>> http://devicetree.org/schemas/pci/qcom,pcie-ep.yaml#//
>> ///local/mnt/workspace/Mrinmay/lemans/next-20230914/linux-next/out/Documentation/devicetree/bindings/pci/qcom,pcie-ep.example.dtb:
>> pcie-ep@1c00000: reg-names: ['parf', 'dbi', 'elbi', 'atu', 'addr_space',
>> 'mmio'] is too short//
>> //        from schema $id:
> missing min/maxItems for reg and reg-names
>
>> http://devicetree.org/schemas/pci/qcom,pcie-ep.yaml#//
>> ///local/mnt/workspace/Mrinmay/lemans/next-20230914/linux-next/out/Documentation/devicetree/bindings/pci/qcom,pcie-ep.example.dtb:
>> pcie-ep@1c00000: interrupts: [[0, 140, 4], [0, 145, 4]] is too short//
>> //        from schema $id:
>> http://devicetree.org/schemas/pci/qcom,pcie-ep.yaml#//
>> ///local/mnt/workspace/Mrinmay/lemans/next-20230914/linux-next/out/Documentation/devicetree/bindings/pci/qcom,pcie-ep.example.dtb:
>> pcie-ep@1c00000: interrupt-names: ['global', 'doorbell'] is too short//
>> //        from schema $id:
>> http://devicetree.org/schemas/pci/qcom,pcie-ep.yaml#//
> incorrect min/maxItems for interrupts.
I am getting the same warnings even after correcting the min/maxItems 
for interrupt.
> -Mrinmay
>> //local/mnt/workspace/Mrinmay/lemans/next-20230914/linux-next/out/Documentation/devicetree/bindings/pci/qcom,pcie-ep.example.dtb:
>> pcie-ep@1c00000: interrupt-names: ['global', 'doorbell'] is too short/
>>
>> added the patch in attachment.
>>
>> --Mrinmay
>>
>>>>> -Shazad
>>>> Here we have defined reg and interrupt per platform as clocks is defined.
>>>>
>>>> -Mrinmay
>>>>
>
>
Dmitry Baryshkov Oct. 16, 2023, 5:19 a.m. UTC | #9 
On Fri, 13 Oct 2023 at 15:55, Mrinmay Sarkar <quic_msarkar@quicinc.com> wrote:
>
>
> On 10/11/2023 5:13 PM, Dmitry Baryshkov wrote:
> > On Wed, 11 Oct 2023 at 14:14, Mrinmay Sarkar <quic_msarkar@quicinc.com> wrote:
> >>
> >> On 10/6/2023 4:24 PM, Shazad Hussain wrote:
> >>>
> >>> On 9/22/2023 12:08 AM, Rob Herring wrote:
> >>>> On Wed, Sep 20, 2023 at 07:25:08PM +0530, Mrinmay Sarkar wrote:
> >>>>> Add devicetree bindings support for SA8775P SoC.
> >>>>> Define reg and interrupt per platform.
> >>>>>
> >>>>> Signed-off-by: Mrinmay Sarkar <quic_msarkar@quicinc.com>
> >>>>> ---
> >>>>>    .../devicetree/bindings/pci/qcom,pcie-ep.yaml      | 130
> >>>>> +++++++++++++++++----
> >>>>>    1 file changed, 108 insertions(+), 22 deletions(-)
> >>>>>
> >>>>> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml
> >>>>> b/Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml
> >>>>> index a223ce0..e860e8f 100644
> >>>>> --- a/Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml
> >>>>> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml
> >>>>> @@ -13,6 +13,7 @@ properties:
> >>>>>      compatible:
> >>>>>        oneOf:
> >>>>>          - enum:
> >>>>> +          - qcom,sa8775p-pcie-ep
> >>>>>              - qcom,sdx55-pcie-ep
> >>>>>              - qcom,sm8450-pcie-ep
> >>>>>          - items:
> >>>>> @@ -20,29 +21,19 @@ properties:
> >>>>>              - const: qcom,sdx55-pcie-ep
> >>>>>        reg:
> >>>>> -    items:
> >>>>> -      - description: Qualcomm-specific PARF configuration registers
> >>>>> -      - description: DesignWare PCIe registers
> >>>>> -      - description: External local bus interface registers
> >>>>> -      - description: Address Translation Unit (ATU) registers
> >>>>> -      - description: Memory region used to map remote RC address space
> >>>>> -      - description: BAR memory region
> >>>>> +    minItems: 6
> >>>>> +    maxItems: 7
> >>>>>        reg-names:
> >>>>> -    items:
> >>>>> -      - const: parf
> >>>>> -      - const: dbi
> >>>>> -      - const: elbi
> >>>>> -      - const: atu
> >>>>> -      - const: addr_space
> >>>>> -      - const: mmio
> >>>>> +    minItems: 6
> >>>>> +    maxItems: 7
> >>>> Don't move these into if/then schemas. Then we are duplicating the
> >>>> names, and there is no reason to keep them aligned for new compatibles.
> >>>>
> >>>> Rob
> >>> Hi Rob,
> >>> As we have one extra reg property (dma) required for sa8775p-pcie-ep,
> >>> isn't it expected to be moved in if/then as per number of regs
> >>> required. Anyways we would have duplication of some properties for new
> >>> compatibles where the member numbers differs for a property.
> >>>
> >>> Are you suggesting to add the extra reg property (dma) in the existing
> >>> reg and reg-names list, and add minItems/maxItems for all compatibles
> >>> present in this file ?
> > This is what we have been doing in other cases: if the list is an
> > extension of the current list, there is no need to duplicate it. One
> > can use min/maxItems instead.
> Hi Dmitry
>
> we have tried using min/maxItems rather than duplicating but somehow
> catch up with some warnings in dt_bindings check
>
> //local/mnt/workspace/Mrinmay/lemans/next-20230914/linux-next/out/Documentation/devicetree/bindings/pci/qcom,pcie-ep.example.dtb:
> pcie-ep@1c00000: reg: [[29360128, 12288], [1073741824, 3869],
> [1073745696, 200], [1073745920, 4096], [1073750016, 4096], [29372416,
> 12288]] is too short//
> //        from schema $id:
> http://devicetree.org/schemas/pci/qcom,pcie-ep.yaml#//
> ///local/mnt/workspace/Mrinmay/lemans/next-20230914/linux-next/out/Documentation/devicetree/bindings/pci/qcom,pcie-ep.example.dtb:
> pcie-ep@1c00000: reg-names: ['parf', 'dbi', 'elbi', 'atu', 'addr_space',
> 'mmio'] is too short//
> //        from schema $id:
> http://devicetree.org/schemas/pci/qcom,pcie-ep.yaml#//
> ///local/mnt/workspace/Mrinmay/lemans/next-20230914/linux-next/out/Documentation/devicetree/bindings/pci/qcom,pcie-ep.example.dtb:
> pcie-ep@1c00000: interrupts: [[0, 140, 4], [0, 145, 4]] is too short//
> //        from schema $id:
> http://devicetree.org/schemas/pci/qcom,pcie-ep.yaml#//
> ///local/mnt/workspace/Mrinmay/lemans/next-20230914/linux-next/out/Documentation/devicetree/bindings/pci/qcom,pcie-ep.example.dtb:
> pcie-ep@1c00000: interrupt-names: ['global', 'doorbell'] is too short//
> //        from schema $id:
> http://devicetree.org/schemas/pci/qcom,pcie-ep.yaml#//
> /
>
> //local/mnt/workspace/Mrinmay/lemans/next-20230914/linux-next/out/Documentation/devicetree/bindings/pci/qcom,pcie-ep.example.dtb:
> pcie-ep@1c00000: interrupt-names: ['global', 'doorbell'] is too short/
>
> added the patch in attachment.

Please, don't send patches as attachments. It is impossible to comment on it.

So, few points I had to fix to make your patch to work:

- Please, understand the difference between enum and items. You'd need
to add your compat string to only one of them. Or to a new entry. But
adding it to both entries is a definite mistake.

- You have extended items for existing platforms (reg, reg-names,
interrupts, interrupt-names). However you failed to add corresponding
minItems, allowing existing platforms to use the list with less items
in it.

- You do not need to have maxItems:N, minItems:N with the same value.
Please drop these minItems, it is the default.

- You haven't reviewed the patch on your own. You have erroneously
nested 'properties' clauses in two places.

$ git diff --stat
 Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml | 33
+++++++++++----------------------
 1 file changed, 11 insertions(+), 22 deletions(-)

Hope this helps.
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml
index a223ce0..e860e8f 100644
--- a/Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml
+++ b/Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml
@@ -13,6 +13,7 @@  properties:
   compatible:
     oneOf:
       - enum:
+          - qcom,sa8775p-pcie-ep
           - qcom,sdx55-pcie-ep
           - qcom,sm8450-pcie-ep
       - items:
@@ -20,29 +21,19 @@  properties:
           - const: qcom,sdx55-pcie-ep
 
   reg:
-    items:
-      - description: Qualcomm-specific PARF configuration registers
-      - description: DesignWare PCIe registers
-      - description: External local bus interface registers
-      - description: Address Translation Unit (ATU) registers
-      - description: Memory region used to map remote RC address space
-      - description: BAR memory region
+    minItems: 6
+    maxItems: 7
 
   reg-names:
-    items:
-      - const: parf
-      - const: dbi
-      - const: elbi
-      - const: atu
-      - const: addr_space
-      - const: mmio
+    minItems: 6
+    maxItems: 7
 
   clocks:
-    minItems: 7
+    minItems: 5
     maxItems: 8
 
   clock-names:
-    minItems: 7
+    minItems: 5
     maxItems: 8
 
   qcom,perst-regs:
@@ -57,14 +48,12 @@  properties:
           - description: Perst separation enable offset
 
   interrupts:
-    items:
-      - description: PCIe Global interrupt
-      - description: PCIe Doorbell interrupt
+    minItems: 2
+    maxItems: 3
 
   interrupt-names:
-    items:
-      - const: global
-      - const: doorbell
+    minItems: 2
+    maxItems: 3
 
   reset-gpios:
     description: GPIO used as PERST# input signal
@@ -122,6 +111,51 @@  allOf:
         compatible:
           contains:
             enum:
+              - qcom,sa8775p-pcie-ep
+    then:
+      properties:
+        reg:
+          items:
+            - description: Qualcomm-specific PARF configuration registers
+            - description: DesignWare PCIe registers
+            - description: External local bus interface registers
+            - description: Address Translation Unit (ATU) registers
+            - description: Memory region used to map remote RC address space
+            - description: BAR memory region
+            - description: DMA memory region
+        reg-names:
+          items:
+            - const: parf
+            - const: dbi
+            - const: elbi
+            - const: atu
+            - const: addr_space
+            - const: mmio
+            - const: dma
+    else:
+      properties:
+        reg:
+          items:
+            - description: Qualcomm-specific PARF configuration registers
+            - description: DesignWare PCIe registers
+            - description: External local bus interface registers
+            - description: Address Translation Unit (ATU) registers
+            - description: Memory region used to map remote RC address space
+            - description: BAR memory region
+        reg-names:
+          items:
+            - const: parf
+            - const: dbi
+            - const: elbi
+            - const: atu
+            - const: addr_space
+            - const: mmio
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
               - qcom,sdx55-pcie-ep
     then:
       properties:
@@ -173,6 +207,58 @@  allOf:
             - const: ddrss_sf_tbu
             - const: aggre_noc_axi
 
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - qcom,sa8775-pcie-ep
+    then:
+      properties:
+        clocks:
+          items:
+            - description: PCIe Auxiliary clock
+            - description: PCIe CFG AHB clock
+            - description: PCIe Master AXI clock
+            - description: PCIe Slave AXI clock
+            - description: PCIe Slave Q2A AXI clock
+        clock-names:
+          items:
+            - const: aux
+            - const: cfg
+            - const: bus_master
+            - const: bus_slave
+            - const: slave_q2a
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - qcom,sa8775p-pcie-ep
+    then:
+      properties:
+        interrupts:
+          items:
+            - description: PCIe Global interrupt
+            - description: PCIe Doorbell interrupt
+            - description: DMA interrupt
+        interrupt-names:
+          items:
+            - const: global
+            - const: doorbell
+            - const: dma
+    else:
+      properties:
+        interrupts:
+          items:
+            - description: PCIe Global interrupt
+            - description: PCIe Doorbell interrupt
+        interrupt-names:
+          items:
+            - const: global
+            - const: doorbell
+
 unevaluatedProperties: false
 
 examples: