diff mbox series

[1/7] dt-bindings: PCI: qcom: Add IPQ9574 specific compatible

Message ID 20230214164135.17039-2-quic_devipriy@quicinc.com
State New
Headers
Series Add PCIe support for IPQ9574 |

Commit Message

Devi Priya Feb. 14, 2023, 4:41 p.m. UTC 
  Document the compatible for IPQ9574

Signed-off-by: Devi Priya <quic_devipriy@quicinc.com>
---
 .../devicetree/bindings/pci/qcom,pcie.yaml    | 72 ++++++++++++++++++-
 1 file changed, 70 insertions(+), 2 deletions(-)

Comments

Krzysztof Kozlowski Feb. 16, 2023, 10:29 a.m. UTC | #1 
On 14/02/2023 17:41, Devi Priya wrote:
> Document the compatible for IPQ9574
> 
> Signed-off-by: Devi Priya <quic_devipriy@quicinc.com>
> ---
>  .../devicetree/bindings/pci/qcom,pcie.yaml    | 72 ++++++++++++++++++-
>  1 file changed, 70 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
> index 872817d6d2bd..dabdf2684e2d 100644
> --- a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
> @@ -26,6 +26,7 @@ properties:
>            - qcom,pcie-ipq8064-v2
>            - qcom,pcie-ipq8074
>            - qcom,pcie-ipq8074-gen3
> +          - qcom,pcie-ipq9574
>            - qcom,pcie-msm8996
>            - qcom,pcie-qcs404
>            - qcom,pcie-sa8540p
> @@ -44,11 +45,11 @@ properties:
>  
>    reg:
>      minItems: 4
> -    maxItems: 5
> +    maxItems: 6
>  
>    reg-names:
>      minItems: 4
> -    maxItems: 5
> +    maxItems: 6
>  
>    interrupts:
>      minItems: 1
> @@ -105,6 +106,8 @@ properties:
>      items:
>        - const: pciephy
>  
> +  msi-parent: true
> +
>    power-domains:
>      maxItems: 1
>  
> @@ -173,6 +176,27 @@ allOf:
>              - const: parf # Qualcomm specific registers
>              - const: config # PCIe configuration space
>  
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - qcom,pcie-ipq9574
> +    then:
> +      properties:
> +        reg:
> +          minItems: 5
> +          maxItems: 6
> +        reg-names:
> +          minItems: 5
> +          items:
> +            - const: dbi # DesignWare PCIe registers
> +            - const: elbi # External local bus interface registers
> +            - const: atu # ATU address space
> +            - const: parf # Qualcomm specific registers
> +            - const: config # PCIe configuration space
> +            - const: aggr_noc #PCIe aggr_noc

Why last one is optional? I would assume device either has it or has not.


Best regards,
Krzysztof
Devi Priya Feb. 20, 2023, 1:29 p.m. UTC | #2 
Hi Krzysztof,
Thanks for taking time to review the patch!

On 2/16/2023 3:59 PM, Krzysztof Kozlowski wrote:
> On 14/02/2023 17:41, Devi Priya wrote:
>> Document the compatible for IPQ9574
>>
>> Signed-off-by: Devi Priya <quic_devipriy@quicinc.com>
>> ---
>>   .../devicetree/bindings/pci/qcom,pcie.yaml    | 72 ++++++++++++++++++-
>>   1 file changed, 70 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
>> index 872817d6d2bd..dabdf2684e2d 100644
>> --- a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
>> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
>> @@ -26,6 +26,7 @@ properties:
>>             - qcom,pcie-ipq8064-v2
>>             - qcom,pcie-ipq8074
>>             - qcom,pcie-ipq8074-gen3
>> +          - qcom,pcie-ipq9574
>>             - qcom,pcie-msm8996
>>             - qcom,pcie-qcs404
>>             - qcom,pcie-sa8540p
>> @@ -44,11 +45,11 @@ properties:
>>   
>>     reg:
>>       minItems: 4
>> -    maxItems: 5
>> +    maxItems: 6
>>   
>>     reg-names:
>>       minItems: 4
>> -    maxItems: 5
>> +    maxItems: 6
>>   
>>     interrupts:
>>       minItems: 1
>> @@ -105,6 +106,8 @@ properties:
>>       items:
>>         - const: pciephy
>>   
>> +  msi-parent: true
>> +
>>     power-domains:
>>       maxItems: 1
>>   
>> @@ -173,6 +176,27 @@ allOf:
>>               - const: parf # Qualcomm specific registers
>>               - const: config # PCIe configuration space
>>   
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            enum:
>> +              - qcom,pcie-ipq9574
>> +    then:
>> +      properties:
>> +        reg:
>> +          minItems: 5
>> +          maxItems: 6
>> +        reg-names:
>> +          minItems: 5
>> +          items:
>> +            - const: dbi # DesignWare PCIe registers
>> +            - const: elbi # External local bus interface registers
>> +            - const: atu # ATU address space
>> +            - const: parf # Qualcomm specific registers
>> +            - const: config # PCIe configuration space
>> +            - const: aggr_noc #PCIe aggr_noc
> 
> Why last one is optional? I would assume device either has it or has not.
> 
Yes right, the device has aggr_noc.
The rate adapter update was required only for 1-lane PCIe
But will check and update this accordingly in the next spin.

> 
> Best regards,
> Krzysztof
> 
Best Regards,
Devi Priya
Manivannan Sadhasivam Feb. 24, 2023, 8:23 a.m. UTC | #3 
On Tue, Feb 14, 2023 at 10:11:29PM +0530, Devi Priya wrote:
> Document the compatible for IPQ9574
> 

You didn't mention about the "msi-parent" property that is being added
by this patch.

> Signed-off-by: Devi Priya <quic_devipriy@quicinc.com>
> ---
>  .../devicetree/bindings/pci/qcom,pcie.yaml    | 72 ++++++++++++++++++-
>  1 file changed, 70 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
> index 872817d6d2bd..dabdf2684e2d 100644
> --- a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
> @@ -26,6 +26,7 @@ properties:
>            - qcom,pcie-ipq8064-v2
>            - qcom,pcie-ipq8074
>            - qcom,pcie-ipq8074-gen3
> +          - qcom,pcie-ipq9574
>            - qcom,pcie-msm8996
>            - qcom,pcie-qcs404
>            - qcom,pcie-sa8540p
> @@ -44,11 +45,11 @@ properties:
>  
>    reg:
>      minItems: 4
> -    maxItems: 5
> +    maxItems: 6
>  
>    reg-names:
>      minItems: 4
> -    maxItems: 5
> +    maxItems: 6
>  
>    interrupts:
>      minItems: 1
> @@ -105,6 +106,8 @@ properties:
>      items:
>        - const: pciephy
>  
> +  msi-parent: true
> +
>    power-domains:
>      maxItems: 1
>  
> @@ -173,6 +176,27 @@ allOf:
>              - const: parf # Qualcomm specific registers
>              - const: config # PCIe configuration space
>  
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - qcom,pcie-ipq9574
> +    then:
> +      properties:
> +        reg:
> +          minItems: 5
> +          maxItems: 6
> +        reg-names:
> +          minItems: 5
> +          items:
> +            - const: dbi # DesignWare PCIe registers
> +            - const: elbi # External local bus interface registers
> +            - const: atu # ATU address space
> +            - const: parf # Qualcomm specific registers
> +            - const: config # PCIe configuration space
> +            - const: aggr_noc #PCIe aggr_noc

Why do you need this region unlike other SoCs? Is the driver making use of it?

Thanks,
Mani

> +
>    - if:
>        properties:
>          compatible:
> @@ -365,6 +389,39 @@ allOf:
>              - const: ahb # AHB Reset
>              - const: axi_m_sticky # AXI Master Sticky reset
>  
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - qcom,pcie-ipq9574
> +    then:
> +      properties:
> +        clocks:
> +          minItems: 6
> +          maxItems: 6
> +        clock-names:
> +          items:
> +            - const: ahb  # AHB clock
> +            - const: aux  # Auxiliary clock
> +            - const: axi_m # AXI Master clock
> +            - const: axi_s # AXI Slave clock
> +            - const: axi_bridge # AXI bridge clock
> +            - const: rchng
> +        resets:
> +          minItems: 8
> +          maxItems: 8
> +        reset-names:
> +          items:
> +            - const: pipe # PIPE reset
> +            - const: sticky # Core Sticky reset
> +            - const: axi_s_sticky # AXI Slave Sticky reset
> +            - const: axi_s # AXI Slave reset
> +            - const: axi_m_sticky # AXI Master Sticky reset
> +            - const: axi_m # AXI Master reset
> +            - const: aux # AUX Reset
> +            - const: ahb # AHB Reset
> +
>    - if:
>        properties:
>          compatible:
> @@ -681,6 +738,16 @@ allOf:
>          - interconnects
>          - interconnect-names
>  
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - qcom,pcie-ipq9574
> +    then:
> +      required:
> +        - msi-parent
> +
>    - if:
>        not:
>          properties:
> @@ -693,6 +760,7 @@ allOf:
>                  - qcom,pcie-ipq8064v2
>                  - qcom,pcie-ipq8074
>                  - qcom,pcie-ipq8074-gen3
> +                - qcom,pcie-ipq9574
>                  - qcom,pcie-qcs404
>      then:
>        required:
> -- 
> 2.17.1
>
Devi Priya Feb. 28, 2023, 5:26 a.m. UTC | #4 
On 2/24/2023 1:53 PM, Manivannan Sadhasivam wrote:
> On Tue, Feb 14, 2023 at 10:11:29PM +0530, Devi Priya wrote:
>> Document the compatible for IPQ9574
>>
Hi Mani, Thanks for taking time to review the patch.
> 
> You didn't mention about the "msi-parent" property that is being added
> by this patch
Sure, will update the commit message in the next spin
> 
>> Signed-off-by: Devi Priya <quic_devipriy@quicinc.com>
>> ---
>>   .../devicetree/bindings/pci/qcom,pcie.yaml    | 72 ++++++++++++++++++-
>>   1 file changed, 70 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
>> index 872817d6d2bd..dabdf2684e2d 100644
>> --- a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
>> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
>> @@ -26,6 +26,7 @@ properties:
>>             - qcom,pcie-ipq8064-v2
>>             - qcom,pcie-ipq8074
>>             - qcom,pcie-ipq8074-gen3
>> +          - qcom,pcie-ipq9574
>>             - qcom,pcie-msm8996
>>             - qcom,pcie-qcs404
>>             - qcom,pcie-sa8540p
>> @@ -44,11 +45,11 @@ properties:
>>   
>>     reg:
>>       minItems: 4
>> -    maxItems: 5
>> +    maxItems: 6
>>   
>>     reg-names:
>>       minItems: 4
>> -    maxItems: 5
>> +    maxItems: 6
>>   
>>     interrupts:
>>       minItems: 1
>> @@ -105,6 +106,8 @@ properties:
>>       items:
>>         - const: pciephy
>>   
>> +  msi-parent: true
>> +
>>     power-domains:
>>       maxItems: 1
>>   
>> @@ -173,6 +176,27 @@ allOf:
>>               - const: parf # Qualcomm specific registers
>>               - const: config # PCIe configuration space
>>   
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            enum:
>> +              - qcom,pcie-ipq9574
>> +    then:
>> +      properties:
>> +        reg:
>> +          minItems: 5
>> +          maxItems: 6
>> +        reg-names:
>> +          minItems: 5
>> +          items:
>> +            - const: dbi # DesignWare PCIe registers
>> +            - const: elbi # External local bus interface registers
>> +            - const: atu # ATU address space
>> +            - const: parf # Qualcomm specific registers
>> +            - const: config # PCIe configuration space
>> +            - const: aggr_noc #PCIe aggr_noc
> 
> Why do you need this region unlike other SoCs? Is the driver making use of it?
We have the aggr_noc region in ipq9574 to achieve higher throughput & to 
handle multiple PCIe instances. The driver uses it to rate adapt 1-lane 
PCIe clocks. My bad, missed it. Will add the driver changes in V2.
> 
> Thanks,
> Mani
> 
>> +
>>     - if:
>>         properties:
>>           compatible:
>> @@ -365,6 +389,39 @@ allOf:
>>               - const: ahb # AHB Reset
>>               - const: axi_m_sticky # AXI Master Sticky reset
>>   
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            enum:
>> +              - qcom,pcie-ipq9574
>> +    then:
>> +      properties:
>> +        clocks:
>> +          minItems: 6
>> +          maxItems: 6
>> +        clock-names:
>> +          items:
>> +            - const: ahb  # AHB clock
>> +            - const: aux  # Auxiliary clock
>> +            - const: axi_m # AXI Master clock
>> +            - const: axi_s # AXI Slave clock
>> +            - const: axi_bridge # AXI bridge clock
>> +            - const: rchng
>> +        resets:
>> +          minItems: 8
>> +          maxItems: 8
>> +        reset-names:
>> +          items:
>> +            - const: pipe # PIPE reset
>> +            - const: sticky # Core Sticky reset
>> +            - const: axi_s_sticky # AXI Slave Sticky reset
>> +            - const: axi_s # AXI Slave reset
>> +            - const: axi_m_sticky # AXI Master Sticky reset
>> +            - const: axi_m # AXI Master reset
>> +            - const: aux # AUX Reset
>> +            - const: ahb # AHB Reset
>> +
>>     - if:
>>         properties:
>>           compatible:
>> @@ -681,6 +738,16 @@ allOf:
>>           - interconnects
>>           - interconnect-names
>>   
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            enum:
>> +              - qcom,pcie-ipq9574
>> +    then:
>> +      required:
>> +        - msi-parent
>> +
>>     - if:
>>         not:
>>           properties:
>> @@ -693,6 +760,7 @@ allOf:
>>                   - qcom,pcie-ipq8064v2
>>                   - qcom,pcie-ipq8074
>>                   - qcom,pcie-ipq8074-gen3
>> +                - qcom,pcie-ipq9574
>>                   - qcom,pcie-qcs404
>>       then:
>>         required:
>> -- 
>> 2.17.1
>>
> 
Thanks,
Devi Priya
Manivannan Sadhasivam Feb. 28, 2023, 6:33 a.m. UTC | #5 
On Tue, Feb 28, 2023 at 10:56:53AM +0530, Devi Priya wrote:
> 
> 
> On 2/24/2023 1:53 PM, Manivannan Sadhasivam wrote:
> > On Tue, Feb 14, 2023 at 10:11:29PM +0530, Devi Priya wrote:
> > > Document the compatible for IPQ9574
> > > 
> Hi Mani, Thanks for taking time to review the patch.
> > 
> > You didn't mention about the "msi-parent" property that is being added
> > by this patch
> Sure, will update the commit message in the next spin
> > 
> > > Signed-off-by: Devi Priya <quic_devipriy@quicinc.com>
> > > ---
> > >   .../devicetree/bindings/pci/qcom,pcie.yaml    | 72 ++++++++++++++++++-
> > >   1 file changed, 70 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
> > > index 872817d6d2bd..dabdf2684e2d 100644
> > > --- a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
> > > +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
> > > @@ -26,6 +26,7 @@ properties:
> > >             - qcom,pcie-ipq8064-v2
> > >             - qcom,pcie-ipq8074
> > >             - qcom,pcie-ipq8074-gen3
> > > +          - qcom,pcie-ipq9574
> > >             - qcom,pcie-msm8996
> > >             - qcom,pcie-qcs404
> > >             - qcom,pcie-sa8540p
> > > @@ -44,11 +45,11 @@ properties:
> > >     reg:
> > >       minItems: 4
> > > -    maxItems: 5
> > > +    maxItems: 6
> > >     reg-names:
> > >       minItems: 4
> > > -    maxItems: 5
> > > +    maxItems: 6
> > >     interrupts:
> > >       minItems: 1
> > > @@ -105,6 +106,8 @@ properties:
> > >       items:
> > >         - const: pciephy
> > > +  msi-parent: true
> > > +
> > >     power-domains:
> > >       maxItems: 1
> > > @@ -173,6 +176,27 @@ allOf:
> > >               - const: parf # Qualcomm specific registers
> > >               - const: config # PCIe configuration space
> > > +  - if:
> > > +      properties:
> > > +        compatible:
> > > +          contains:
> > > +            enum:
> > > +              - qcom,pcie-ipq9574
> > > +    then:
> > > +      properties:
> > > +        reg:
> > > +          minItems: 5
> > > +          maxItems: 6
> > > +        reg-names:
> > > +          minItems: 5
> > > +          items:
> > > +            - const: dbi # DesignWare PCIe registers
> > > +            - const: elbi # External local bus interface registers
> > > +            - const: atu # ATU address space
> > > +            - const: parf # Qualcomm specific registers
> > > +            - const: config # PCIe configuration space
> > > +            - const: aggr_noc #PCIe aggr_noc
> > 
> > Why do you need this region unlike other SoCs? Is the driver making use of it?
> We have the aggr_noc region in ipq9574 to achieve higher throughput & to
> handle multiple PCIe instances. The driver uses it to rate adapt 1-lane PCIe
> clocks. My bad, missed it. Will add the driver changes in V2.

Hmm, this is something new. How can you achieve higher throughput with this
region? Can you explain more on how it is used?

Thanks,
Mani

> > 
> > Thanks,
> > Mani
> > 
> > > +
> > >     - if:
> > >         properties:
> > >           compatible:
> > > @@ -365,6 +389,39 @@ allOf:
> > >               - const: ahb # AHB Reset
> > >               - const: axi_m_sticky # AXI Master Sticky reset
> > > +  - if:
> > > +      properties:
> > > +        compatible:
> > > +          contains:
> > > +            enum:
> > > +              - qcom,pcie-ipq9574
> > > +    then:
> > > +      properties:
> > > +        clocks:
> > > +          minItems: 6
> > > +          maxItems: 6
> > > +        clock-names:
> > > +          items:
> > > +            - const: ahb  # AHB clock
> > > +            - const: aux  # Auxiliary clock
> > > +            - const: axi_m # AXI Master clock
> > > +            - const: axi_s # AXI Slave clock
> > > +            - const: axi_bridge # AXI bridge clock
> > > +            - const: rchng
> > > +        resets:
> > > +          minItems: 8
> > > +          maxItems: 8
> > > +        reset-names:
> > > +          items:
> > > +            - const: pipe # PIPE reset
> > > +            - const: sticky # Core Sticky reset
> > > +            - const: axi_s_sticky # AXI Slave Sticky reset
> > > +            - const: axi_s # AXI Slave reset
> > > +            - const: axi_m_sticky # AXI Master Sticky reset
> > > +            - const: axi_m # AXI Master reset
> > > +            - const: aux # AUX Reset
> > > +            - const: ahb # AHB Reset
> > > +
> > >     - if:
> > >         properties:
> > >           compatible:
> > > @@ -681,6 +738,16 @@ allOf:
> > >           - interconnects
> > >           - interconnect-names
> > > +  - if:
> > > +      properties:
> > > +        compatible:
> > > +          contains:
> > > +            enum:
> > > +              - qcom,pcie-ipq9574
> > > +    then:
> > > +      required:
> > > +        - msi-parent
> > > +
> > >     - if:
> > >         not:
> > >           properties:
> > > @@ -693,6 +760,7 @@ allOf:
> > >                   - qcom,pcie-ipq8064v2
> > >                   - qcom,pcie-ipq8074
> > >                   - qcom,pcie-ipq8074-gen3
> > > +                - qcom,pcie-ipq9574
> > >                   - qcom,pcie-qcs404
> > >       then:
> > >         required:
> > > -- 
> > > 2.17.1
> > > 
> > 
> Thanks,
> Devi Priya
Dmitry Baryshkov March 3, 2023, 3:16 p.m. UTC | #6 
28 февраля 2023 г. 08:33:58 GMT+02:00, Manivannan Sadhasivam <mani@kernel.org> пишет:
>On Tue, Feb 28, 2023 at 10:56:53AM +0530, Devi Priya wrote:
>> 
>> 
>> On 2/24/2023 1:53 PM, Manivannan Sadhasivam wrote:
>> > On Tue, Feb 14, 2023 at 10:11:29PM +0530, Devi Priya wrote:
>> > > Document the compatible for IPQ9574
>> > > 
>> Hi Mani, Thanks for taking time to review the patch.
>> > 
>> > You didn't mention about the "msi-parent" property that is being added
>> > by this patch
>> Sure, will update the commit message in the next spin
>> > 
>> > > Signed-off-by: Devi Priya <quic_devipriy@quicinc.com>
>> > > ---
>> > >   .../devicetree/bindings/pci/qcom,pcie.yaml    | 72 ++++++++++++++++++-
>> > >   1 file changed, 70 insertions(+), 2 deletions(-)
>> > > 
>> > > diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
>> > > index 872817d6d2bd..dabdf2684e2d 100644
>> > > --- a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
>> > > +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
>> > > @@ -26,6 +26,7 @@ properties:
>> > >             - qcom,pcie-ipq8064-v2
>> > >             - qcom,pcie-ipq8074
>> > >             - qcom,pcie-ipq8074-gen3
>> > > +          - qcom,pcie-ipq9574
>> > >             - qcom,pcie-msm8996
>> > >             - qcom,pcie-qcs404
>> > >             - qcom,pcie-sa8540p
>> > > @@ -44,11 +45,11 @@ properties:
>> > >     reg:
>> > >       minItems: 4
>> > > -    maxItems: 5
>> > > +    maxItems: 6
>> > >     reg-names:
>> > >       minItems: 4
>> > > -    maxItems: 5
>> > > +    maxItems: 6
>> > >     interrupts:
>> > >       minItems: 1
>> > > @@ -105,6 +106,8 @@ properties:
>> > >       items:
>> > >         - const: pciephy
>> > > +  msi-parent: true
>> > > +
>> > >     power-domains:
>> > >       maxItems: 1
>> > > @@ -173,6 +176,27 @@ allOf:
>> > >               - const: parf # Qualcomm specific registers
>> > >               - const: config # PCIe configuration space
>> > > +  - if:
>> > > +      properties:
>> > > +        compatible:
>> > > +          contains:
>> > > +            enum:
>> > > +              - qcom,pcie-ipq9574
>> > > +    then:
>> > > +      properties:
>> > > +        reg:
>> > > +          minItems: 5
>> > > +          maxItems: 6
>> > > +        reg-names:
>> > > +          minItems: 5
>> > > +          items:
>> > > +            - const: dbi # DesignWare PCIe registers
>> > > +            - const: elbi # External local bus interface registers
>> > > +            - const: atu # ATU address space
>> > > +            - const: parf # Qualcomm specific registers
>> > > +            - const: config # PCIe configuration space
>> > > +            - const: aggr_noc #PCIe aggr_noc
>> > 
>> > Why do you need this region unlike other SoCs? Is the driver making use of it?
>> We have the aggr_noc region in ipq9574 to achieve higher throughput & to
>> handle multiple PCIe instances. The driver uses it to rate adapt 1-lane PCIe
>> clocks. My bad, missed it. Will add the driver changes in V2.
>
>Hmm, this is something new. How can you achieve higher throughput with this
>region? Can you explain more on how it is used?

Based on the name of the region, it looks like it is an interconnect region. 

Devi, if this is the case, then you have to handle it through the interconnect driver, rather than poking directly into these registers.


>
>Thanks,
>Mani
>
>> > 
>> > Thanks,
>> > Mani
>> > 
>> > > +
>> > >     - if:
>> > >         properties:
>> > >           compatible:
>> > > @@ -365,6 +389,39 @@ allOf:
>> > >               - const: ahb # AHB Reset
>> > >               - const: axi_m_sticky # AXI Master Sticky reset
>> > > +  - if:
>> > > +      properties:
>> > > +        compatible:
>> > > +          contains:
>> > > +            enum:
>> > > +              - qcom,pcie-ipq9574
>> > > +    then:
>> > > +      properties:
>> > > +        clocks:
>> > > +          minItems: 6
>> > > +          maxItems: 6
>> > > +        clock-names:
>> > > +          items:
>> > > +            - const: ahb  # AHB clock
>> > > +            - const: aux  # Auxiliary clock
>> > > +            - const: axi_m # AXI Master clock
>> > > +            - const: axi_s # AXI Slave clock
>> > > +            - const: axi_bridge # AXI bridge clock
>> > > +            - const: rchng
>> > > +        resets:
>> > > +          minItems: 8
>> > > +          maxItems: 8
>> > > +        reset-names:
>> > > +          items:
>> > > +            - const: pipe # PIPE reset
>> > > +            - const: sticky # Core Sticky reset
>> > > +            - const: axi_s_sticky # AXI Slave Sticky reset
>> > > +            - const: axi_s # AXI Slave reset
>> > > +            - const: axi_m_sticky # AXI Master Sticky reset
>> > > +            - const: axi_m # AXI Master reset
>> > > +            - const: aux # AUX Reset
>> > > +            - const: ahb # AHB Reset
>> > > +
>> > >     - if:
>> > >         properties:
>> > >           compatible:
>> > > @@ -681,6 +738,16 @@ allOf:
>> > >           - interconnects
>> > >           - interconnect-names
>> > > +  - if:
>> > > +      properties:
>> > > +        compatible:
>> > > +          contains:
>> > > +            enum:
>> > > +              - qcom,pcie-ipq9574
>> > > +    then:
>> > > +      required:
>> > > +        - msi-parent
>> > > +
>> > >     - if:
>> > >         not:
>> > >           properties:
>> > > @@ -693,6 +760,7 @@ allOf:
>> > >                   - qcom,pcie-ipq8064v2
>> > >                   - qcom,pcie-ipq8074
>> > >                   - qcom,pcie-ipq8074-gen3
>> > > +                - qcom,pcie-ipq9574
>> > >                   - qcom,pcie-qcs404
>> > >       then:
>> > >         required:
>> > > -- 
>> > > 2.17.1
>> > > 
>> > 
>> Thanks,
>> Devi Priya
>
Manivannan Sadhasivam March 3, 2023, 5:40 p.m. UTC | #7 
On Fri, Mar 03, 2023 at 05:16:58PM +0200, Dmitry Baryshkov wrote:
> 28 февраля 2023 г. 08:33:58 GMT+02:00, Manivannan Sadhasivam <mani@kernel.org> пишет:
> >On Tue, Feb 28, 2023 at 10:56:53AM +0530, Devi Priya wrote:
> >> 
> >> 
> >> On 2/24/2023 1:53 PM, Manivannan Sadhasivam wrote:
> >> > On Tue, Feb 14, 2023 at 10:11:29PM +0530, Devi Priya wrote:
> >> > > Document the compatible for IPQ9574
> >> > > 
> >> Hi Mani, Thanks for taking time to review the patch.
> >> > 
> >> > You didn't mention about the "msi-parent" property that is being added
> >> > by this patch
> >> Sure, will update the commit message in the next spin
> >> > 
> >> > > Signed-off-by: Devi Priya <quic_devipriy@quicinc.com>
> >> > > ---
> >> > >   .../devicetree/bindings/pci/qcom,pcie.yaml    | 72 ++++++++++++++++++-
> >> > >   1 file changed, 70 insertions(+), 2 deletions(-)
> >> > > 
> >> > > diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
> >> > > index 872817d6d2bd..dabdf2684e2d 100644
> >> > > --- a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
> >> > > +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
> >> > > @@ -26,6 +26,7 @@ properties:
> >> > >             - qcom,pcie-ipq8064-v2
> >> > >             - qcom,pcie-ipq8074
> >> > >             - qcom,pcie-ipq8074-gen3
> >> > > +          - qcom,pcie-ipq9574
> >> > >             - qcom,pcie-msm8996
> >> > >             - qcom,pcie-qcs404
> >> > >             - qcom,pcie-sa8540p
> >> > > @@ -44,11 +45,11 @@ properties:
> >> > >     reg:
> >> > >       minItems: 4
> >> > > -    maxItems: 5
> >> > > +    maxItems: 6
> >> > >     reg-names:
> >> > >       minItems: 4
> >> > > -    maxItems: 5
> >> > > +    maxItems: 6
> >> > >     interrupts:
> >> > >       minItems: 1
> >> > > @@ -105,6 +106,8 @@ properties:
> >> > >       items:
> >> > >         - const: pciephy
> >> > > +  msi-parent: true
> >> > > +
> >> > >     power-domains:
> >> > >       maxItems: 1
> >> > > @@ -173,6 +176,27 @@ allOf:
> >> > >               - const: parf # Qualcomm specific registers
> >> > >               - const: config # PCIe configuration space
> >> > > +  - if:
> >> > > +      properties:
> >> > > +        compatible:
> >> > > +          contains:
> >> > > +            enum:
> >> > > +              - qcom,pcie-ipq9574
> >> > > +    then:
> >> > > +      properties:
> >> > > +        reg:
> >> > > +          minItems: 5
> >> > > +          maxItems: 6
> >> > > +        reg-names:
> >> > > +          minItems: 5
> >> > > +          items:
> >> > > +            - const: dbi # DesignWare PCIe registers
> >> > > +            - const: elbi # External local bus interface registers
> >> > > +            - const: atu # ATU address space
> >> > > +            - const: parf # Qualcomm specific registers
> >> > > +            - const: config # PCIe configuration space
> >> > > +            - const: aggr_noc #PCIe aggr_noc
> >> > 
> >> > Why do you need this region unlike other SoCs? Is the driver making use of it?
> >> We have the aggr_noc region in ipq9574 to achieve higher throughput & to
> >> handle multiple PCIe instances. The driver uses it to rate adapt 1-lane PCIe
> >> clocks. My bad, missed it. Will add the driver changes in V2.
> >
> >Hmm, this is something new. How can you achieve higher throughput with this
> >region? Can you explain more on how it is used?
> 
> Based on the name of the region, it looks like it is an interconnect region. 
> 

Well, we only have BCM based interconnects so far. That's why I was curious
about this region and its purpose.

> Devi, if this is the case, then you have to handle it through the interconnect driver, rather than poking directly into these registers.

If that so, it doesn't need to be added in this series itself. I believe that
without aggr_noc region, the PCIe controller can still function properly with
reduced performance. But you can add the interconnect support later as a
separate series.

Thanks,
Mani

> 
> 
> >
> >Thanks,
> >Mani
> >
> >> > 
> >> > Thanks,
> >> > Mani
> >> > 
> >> > > +
> >> > >     - if:
> >> > >         properties:
> >> > >           compatible:
> >> > > @@ -365,6 +389,39 @@ allOf:
> >> > >               - const: ahb # AHB Reset
> >> > >               - const: axi_m_sticky # AXI Master Sticky reset
> >> > > +  - if:
> >> > > +      properties:
> >> > > +        compatible:
> >> > > +          contains:
> >> > > +            enum:
> >> > > +              - qcom,pcie-ipq9574
> >> > > +    then:
> >> > > +      properties:
> >> > > +        clocks:
> >> > > +          minItems: 6
> >> > > +          maxItems: 6
> >> > > +        clock-names:
> >> > > +          items:
> >> > > +            - const: ahb  # AHB clock
> >> > > +            - const: aux  # Auxiliary clock
> >> > > +            - const: axi_m # AXI Master clock
> >> > > +            - const: axi_s # AXI Slave clock
> >> > > +            - const: axi_bridge # AXI bridge clock
> >> > > +            - const: rchng
> >> > > +        resets:
> >> > > +          minItems: 8
> >> > > +          maxItems: 8
> >> > > +        reset-names:
> >> > > +          items:
> >> > > +            - const: pipe # PIPE reset
> >> > > +            - const: sticky # Core Sticky reset
> >> > > +            - const: axi_s_sticky # AXI Slave Sticky reset
> >> > > +            - const: axi_s # AXI Slave reset
> >> > > +            - const: axi_m_sticky # AXI Master Sticky reset
> >> > > +            - const: axi_m # AXI Master reset
> >> > > +            - const: aux # AUX Reset
> >> > > +            - const: ahb # AHB Reset
> >> > > +
> >> > >     - if:
> >> > >         properties:
> >> > >           compatible:
> >> > > @@ -681,6 +738,16 @@ allOf:
> >> > >           - interconnects
> >> > >           - interconnect-names
> >> > > +  - if:
> >> > > +      properties:
> >> > > +        compatible:
> >> > > +          contains:
> >> > > +            enum:
> >> > > +              - qcom,pcie-ipq9574
> >> > > +    then:
> >> > > +      required:
> >> > > +        - msi-parent
> >> > > +
> >> > >     - if:
> >> > >         not:
> >> > >           properties:
> >> > > @@ -693,6 +760,7 @@ allOf:
> >> > >                   - qcom,pcie-ipq8064v2
> >> > >                   - qcom,pcie-ipq8074
> >> > >                   - qcom,pcie-ipq8074-gen3
> >> > > +                - qcom,pcie-ipq9574
> >> > >                   - qcom,pcie-qcs404
> >> > >       then:
> >> > >         required:
> >> > > -- 
> >> > > 2.17.1
> >> > > 
> >> > 
> >> Thanks,
> >> Devi Priya
> >
>
Devi Priya March 7, 2023, 9:45 a.m. UTC | #8 
On 3/3/2023 11:10 PM, Manivannan Sadhasivam wrote:
> On Fri, Mar 03, 2023 at 05:16:58PM +0200, Dmitry Baryshkov wrote:
>> 28 февраля 2023 г. 08:33:58 GMT+02:00, Manivannan Sadhasivam <mani@kernel.org> пишет:
>>> On Tue, Feb 28, 2023 at 10:56:53AM +0530, Devi Priya wrote:
>>>>
>>>>
>>>> On 2/24/2023 1:53 PM, Manivannan Sadhasivam wrote:
>>>>> On Tue, Feb 14, 2023 at 10:11:29PM +0530, Devi Priya wrote:
>>>>>> Document the compatible for IPQ9574
>>>>>>
>>>> Hi Mani, Thanks for taking time to review the patch.
>>>>>
>>>>> You didn't mention about the "msi-parent" property that is being added
>>>>> by this patch
>>>> Sure, will update the commit message in the next spin
>>>>>
>>>>>> Signed-off-by: Devi Priya <quic_devipriy@quicinc.com>
>>>>>> ---
>>>>>>    .../devicetree/bindings/pci/qcom,pcie.yaml    | 72 ++++++++++++++++++-
>>>>>>    1 file changed, 70 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
>>>>>> index 872817d6d2bd..dabdf2684e2d 100644
>>>>>> --- a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
>>>>>> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
>>>>>> @@ -26,6 +26,7 @@ properties:
>>>>>>              - qcom,pcie-ipq8064-v2
>>>>>>              - qcom,pcie-ipq8074
>>>>>>              - qcom,pcie-ipq8074-gen3
>>>>>> +          - qcom,pcie-ipq9574
>>>>>>              - qcom,pcie-msm8996
>>>>>>              - qcom,pcie-qcs404
>>>>>>              - qcom,pcie-sa8540p
>>>>>> @@ -44,11 +45,11 @@ properties:
>>>>>>      reg:
>>>>>>        minItems: 4
>>>>>> -    maxItems: 5
>>>>>> +    maxItems: 6
>>>>>>      reg-names:
>>>>>>        minItems: 4
>>>>>> -    maxItems: 5
>>>>>> +    maxItems: 6
>>>>>>      interrupts:
>>>>>>        minItems: 1
>>>>>> @@ -105,6 +106,8 @@ properties:
>>>>>>        items:
>>>>>>          - const: pciephy
>>>>>> +  msi-parent: true
>>>>>> +
>>>>>>      power-domains:
>>>>>>        maxItems: 1
>>>>>> @@ -173,6 +176,27 @@ allOf:
>>>>>>                - const: parf # Qualcomm specific registers
>>>>>>                - const: config # PCIe configuration space
>>>>>> +  - if:
>>>>>> +      properties:
>>>>>> +        compatible:
>>>>>> +          contains:
>>>>>> +            enum:
>>>>>> +              - qcom,pcie-ipq9574
>>>>>> +    then:
>>>>>> +      properties:
>>>>>> +        reg:
>>>>>> +          minItems: 5
>>>>>> +          maxItems: 6
>>>>>> +        reg-names:
>>>>>> +          minItems: 5
>>>>>> +          items:
>>>>>> +            - const: dbi # DesignWare PCIe registers
>>>>>> +            - const: elbi # External local bus interface registers
>>>>>> +            - const: atu # ATU address space
>>>>>> +            - const: parf # Qualcomm specific registers
>>>>>> +            - const: config # PCIe configuration space
>>>>>> +            - const: aggr_noc #PCIe aggr_noc
>>>>>
>>>>> Why do you need this region unlike other SoCs? Is the driver making use of it?
>>>> We have the aggr_noc region in ipq9574 to achieve higher throughput & to
>>>> handle multiple PCIe instances. The driver uses it to rate adapt 1-lane PCIe
>>>> clocks. My bad, missed it. Will add the driver changes in V2.
>>>
>>> Hmm, this is something new. How can you achieve higher throughput with this
>>> region? Can you explain more on how it is used?
>>
>> Based on the name of the region, it looks like it is an interconnect region.
>>
> 
> Well, we only have BCM based interconnects so far. That's why I was curious
> about this region and its purpose.
For connected PCIe slave devices that are running at frequency lesser
than the ANOC frequency (342MHz), the rate adapter of ANOC needs to be
configured
> 
>> Devi, if this is the case, then you have to handle it through the interconnect driver, rather than poking directly into these registers.
> 
> If that so, it doesn't need to be added in this series itself. I believe that
> without aggr_noc region, the PCIe controller can still function properly with
> reduced performance. But you can add the interconnect support later as a
> separate series.
Sure, okay. The ANOC runs at a fixed frequency of 342MHz and the 
interconnect clocks are not scaled. The aggr_noc register is just a 
magic register for configuring it's rate adapter to ensure no wait 
cycles are inserted.

> 
> Thanks,
> Mani
> 
>>
>>
>>>
>>> Thanks,
>>> Mani
>>>
>>>>>
>>>>> Thanks,
>>>>> Mani
>>>>>
>>>>>> +
>>>>>>      - if:
>>>>>>          properties:
>>>>>>            compatible:
>>>>>> @@ -365,6 +389,39 @@ allOf:
>>>>>>                - const: ahb # AHB Reset
>>>>>>                - const: axi_m_sticky # AXI Master Sticky reset
>>>>>> +  - if:
>>>>>> +      properties:
>>>>>> +        compatible:
>>>>>> +          contains:
>>>>>> +            enum:
>>>>>> +              - qcom,pcie-ipq9574
>>>>>> +    then:
>>>>>> +      properties:
>>>>>> +        clocks:
>>>>>> +          minItems: 6
>>>>>> +          maxItems: 6
>>>>>> +        clock-names:
>>>>>> +          items:
>>>>>> +            - const: ahb  # AHB clock
>>>>>> +            - const: aux  # Auxiliary clock
>>>>>> +            - const: axi_m # AXI Master clock
>>>>>> +            - const: axi_s # AXI Slave clock
>>>>>> +            - const: axi_bridge # AXI bridge clock
>>>>>> +            - const: rchng
>>>>>> +        resets:
>>>>>> +          minItems: 8
>>>>>> +          maxItems: 8
>>>>>> +        reset-names:
>>>>>> +          items:
>>>>>> +            - const: pipe # PIPE reset
>>>>>> +            - const: sticky # Core Sticky reset
>>>>>> +            - const: axi_s_sticky # AXI Slave Sticky reset
>>>>>> +            - const: axi_s # AXI Slave reset
>>>>>> +            - const: axi_m_sticky # AXI Master Sticky reset
>>>>>> +            - const: axi_m # AXI Master reset
>>>>>> +            - const: aux # AUX Reset
>>>>>> +            - const: ahb # AHB Reset
>>>>>> +
>>>>>>      - if:
>>>>>>          properties:
>>>>>>            compatible:
>>>>>> @@ -681,6 +738,16 @@ allOf:
>>>>>>            - interconnects
>>>>>>            - interconnect-names
>>>>>> +  - if:
>>>>>> +      properties:
>>>>>> +        compatible:
>>>>>> +          contains:
>>>>>> +            enum:
>>>>>> +              - qcom,pcie-ipq9574
>>>>>> +    then:
>>>>>> +      required:
>>>>>> +        - msi-parent
>>>>>> +
>>>>>>      - if:
>>>>>>          not:
>>>>>>            properties:
>>>>>> @@ -693,6 +760,7 @@ allOf:
>>>>>>                    - qcom,pcie-ipq8064v2
>>>>>>                    - qcom,pcie-ipq8074
>>>>>>                    - qcom,pcie-ipq8074-gen3
>>>>>> +                - qcom,pcie-ipq9574
>>>>>>                    - qcom,pcie-qcs404
>>>>>>        then:
>>>>>>          required:
>>>>>> -- 
>>>>>> 2.17.1
>>>>>>
>>>>>
>>>> Thanks,
>>>> Devi Priya
>>>
>>
> 
Thanks,
Devi Priya
Dmitry Baryshkov March 7, 2023, 11:38 a.m. UTC | #9 
On Tue, 7 Mar 2023 at 11:45, Devi Priya <quic_devipriy@quicinc.com> wrote:
>
>
>
> On 3/3/2023 11:10 PM, Manivannan Sadhasivam wrote:
> > On Fri, Mar 03, 2023 at 05:16:58PM +0200, Dmitry Baryshkov wrote:
> >> 28 февраля 2023 г. 08:33:58 GMT+02:00, Manivannan Sadhasivam <mani@kernel.org> пишет:
> >>> On Tue, Feb 28, 2023 at 10:56:53AM +0530, Devi Priya wrote:
> >>>>
> >>>>
> >>>> On 2/24/2023 1:53 PM, Manivannan Sadhasivam wrote:
> >>>>> On Tue, Feb 14, 2023 at 10:11:29PM +0530, Devi Priya wrote:
> >>>>>> Document the compatible for IPQ9574
> >>>>>>
> >>>> Hi Mani, Thanks for taking time to review the patch.
> >>>>>
> >>>>> You didn't mention about the "msi-parent" property that is being added
> >>>>> by this patch
> >>>> Sure, will update the commit message in the next spin
> >>>>>
> >>>>>> Signed-off-by: Devi Priya <quic_devipriy@quicinc.com>
> >>>>>> ---
> >>>>>>    .../devicetree/bindings/pci/qcom,pcie.yaml    | 72 ++++++++++++++++++-
> >>>>>>    1 file changed, 70 insertions(+), 2 deletions(-)
> >>>>>>
> >>>>>> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
> >>>>>> index 872817d6d2bd..dabdf2684e2d 100644
> >>>>>> --- a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
> >>>>>> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
> >>>>>> @@ -26,6 +26,7 @@ properties:
> >>>>>>              - qcom,pcie-ipq8064-v2
> >>>>>>              - qcom,pcie-ipq8074
> >>>>>>              - qcom,pcie-ipq8074-gen3
> >>>>>> +          - qcom,pcie-ipq9574
> >>>>>>              - qcom,pcie-msm8996
> >>>>>>              - qcom,pcie-qcs404
> >>>>>>              - qcom,pcie-sa8540p
> >>>>>> @@ -44,11 +45,11 @@ properties:
> >>>>>>      reg:
> >>>>>>        minItems: 4
> >>>>>> -    maxItems: 5
> >>>>>> +    maxItems: 6
> >>>>>>      reg-names:
> >>>>>>        minItems: 4
> >>>>>> -    maxItems: 5
> >>>>>> +    maxItems: 6
> >>>>>>      interrupts:
> >>>>>>        minItems: 1
> >>>>>> @@ -105,6 +106,8 @@ properties:
> >>>>>>        items:
> >>>>>>          - const: pciephy
> >>>>>> +  msi-parent: true
> >>>>>> +
> >>>>>>      power-domains:
> >>>>>>        maxItems: 1
> >>>>>> @@ -173,6 +176,27 @@ allOf:
> >>>>>>                - const: parf # Qualcomm specific registers
> >>>>>>                - const: config # PCIe configuration space
> >>>>>> +  - if:
> >>>>>> +      properties:
> >>>>>> +        compatible:
> >>>>>> +          contains:
> >>>>>> +            enum:
> >>>>>> +              - qcom,pcie-ipq9574
> >>>>>> +    then:
> >>>>>> +      properties:
> >>>>>> +        reg:
> >>>>>> +          minItems: 5
> >>>>>> +          maxItems: 6
> >>>>>> +        reg-names:
> >>>>>> +          minItems: 5
> >>>>>> +          items:
> >>>>>> +            - const: dbi # DesignWare PCIe registers
> >>>>>> +            - const: elbi # External local bus interface registers
> >>>>>> +            - const: atu # ATU address space
> >>>>>> +            - const: parf # Qualcomm specific registers
> >>>>>> +            - const: config # PCIe configuration space
> >>>>>> +            - const: aggr_noc #PCIe aggr_noc
> >>>>>
> >>>>> Why do you need this region unlike other SoCs? Is the driver making use of it?
> >>>> We have the aggr_noc region in ipq9574 to achieve higher throughput & to
> >>>> handle multiple PCIe instances. The driver uses it to rate adapt 1-lane PCIe
> >>>> clocks. My bad, missed it. Will add the driver changes in V2.
> >>>
> >>> Hmm, this is something new. How can you achieve higher throughput with this
> >>> region? Can you explain more on how it is used?
> >>
> >> Based on the name of the region, it looks like it is an interconnect region.
> >>
> >
> > Well, we only have BCM based interconnects so far. That's why I was curious
> > about this region and its purpose.
> For connected PCIe slave devices that are running at frequency lesser
> than the ANOC frequency (342MHz), the rate adapter of ANOC needs to be
> configured
> >
> >> Devi, if this is the case, then you have to handle it through the interconnect driver, rather than poking directly into these registers.
> >
> > If that so, it doesn't need to be added in this series itself. I believe that
> > without aggr_noc region, the PCIe controller can still function properly with
> > reduced performance. But you can add the interconnect support later as a
> > separate series.
> Sure, okay. The ANOC runs at a fixed frequency of 342MHz and the
> interconnect clocks are not scaled. The aggr_noc register is just a
> magic register for configuring it's rate adapter to ensure no wait
> cycles are inserted.

I have been hesitant at some point, but this looks more and more like
a special kind of interconnect. Please consider moving all the NoC
stuff into a separate driver implementing the ICC API.

>
> >
> > Thanks,
> > Mani
> >
> >>
> >>
> >>>
> >>> Thanks,
> >>> Mani
> >>>
> >>>>>
> >>>>> Thanks,
> >>>>> Mani
> >>>>>
> >>>>>> +
> >>>>>>      - if:
> >>>>>>          properties:
> >>>>>>            compatible:
> >>>>>> @@ -365,6 +389,39 @@ allOf:
> >>>>>>                - const: ahb # AHB Reset
> >>>>>>                - const: axi_m_sticky # AXI Master Sticky reset
> >>>>>> +  - if:
> >>>>>> +      properties:
> >>>>>> +        compatible:
> >>>>>> +          contains:
> >>>>>> +            enum:
> >>>>>> +              - qcom,pcie-ipq9574
> >>>>>> +    then:
> >>>>>> +      properties:
> >>>>>> +        clocks:
> >>>>>> +          minItems: 6
> >>>>>> +          maxItems: 6
> >>>>>> +        clock-names:
> >>>>>> +          items:
> >>>>>> +            - const: ahb  # AHB clock
> >>>>>> +            - const: aux  # Auxiliary clock
> >>>>>> +            - const: axi_m # AXI Master clock
> >>>>>> +            - const: axi_s # AXI Slave clock
> >>>>>> +            - const: axi_bridge # AXI bridge clock
> >>>>>> +            - const: rchng
> >>>>>> +        resets:
> >>>>>> +          minItems: 8
> >>>>>> +          maxItems: 8
> >>>>>> +        reset-names:
> >>>>>> +          items:
> >>>>>> +            - const: pipe # PIPE reset
> >>>>>> +            - const: sticky # Core Sticky reset
> >>>>>> +            - const: axi_s_sticky # AXI Slave Sticky reset
> >>>>>> +            - const: axi_s # AXI Slave reset
> >>>>>> +            - const: axi_m_sticky # AXI Master Sticky reset
> >>>>>> +            - const: axi_m # AXI Master reset
> >>>>>> +            - const: aux # AUX Reset
> >>>>>> +            - const: ahb # AHB Reset
> >>>>>> +
> >>>>>>      - if:
> >>>>>>          properties:
> >>>>>>            compatible:
> >>>>>> @@ -681,6 +738,16 @@ allOf:
> >>>>>>            - interconnects
> >>>>>>            - interconnect-names
> >>>>>> +  - if:
> >>>>>> +      properties:
> >>>>>> +        compatible:
> >>>>>> +          contains:
> >>>>>> +            enum:
> >>>>>> +              - qcom,pcie-ipq9574
> >>>>>> +    then:
> >>>>>> +      required:
> >>>>>> +        - msi-parent
> >>>>>> +
> >>>>>>      - if:
> >>>>>>          not:
> >>>>>>            properties:
> >>>>>> @@ -693,6 +760,7 @@ allOf:
> >>>>>>                    - qcom,pcie-ipq8064v2
> >>>>>>                    - qcom,pcie-ipq8074
> >>>>>>                    - qcom,pcie-ipq8074-gen3
> >>>>>> +                - qcom,pcie-ipq9574
> >>>>>>                    - qcom,pcie-qcs404
> >>>>>>        then:
> >>>>>>          required:
> >>>>>> --
> >>>>>> 2.17.1
> >>>>>>
> >>>>>
> >>>> Thanks,
> >>>> Devi Priya
> >>>
> >>
> >
> Thanks,
> Devi Priya
Manivannan Sadhasivam March 7, 2023, 12:56 p.m. UTC | #10 
On Tue, Mar 07, 2023 at 03:15:08PM +0530, Devi Priya wrote:
> 
> 
> On 3/3/2023 11:10 PM, Manivannan Sadhasivam wrote:
> > On Fri, Mar 03, 2023 at 05:16:58PM +0200, Dmitry Baryshkov wrote:
> > > 28 февраля 2023 г. 08:33:58 GMT+02:00, Manivannan Sadhasivam <mani@kernel.org> пишет:
> > > > On Tue, Feb 28, 2023 at 10:56:53AM +0530, Devi Priya wrote:
> > > > > 
> > > > > 
> > > > > On 2/24/2023 1:53 PM, Manivannan Sadhasivam wrote:
> > > > > > On Tue, Feb 14, 2023 at 10:11:29PM +0530, Devi Priya wrote:
> > > > > > > Document the compatible for IPQ9574
> > > > > > > 
> > > > > Hi Mani, Thanks for taking time to review the patch.
> > > > > > 
> > > > > > You didn't mention about the "msi-parent" property that is being added
> > > > > > by this patch
> > > > > Sure, will update the commit message in the next spin
> > > > > > 
> > > > > > > Signed-off-by: Devi Priya <quic_devipriy@quicinc.com>
> > > > > > > ---
> > > > > > >    .../devicetree/bindings/pci/qcom,pcie.yaml    | 72 ++++++++++++++++++-
> > > > > > >    1 file changed, 70 insertions(+), 2 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
> > > > > > > index 872817d6d2bd..dabdf2684e2d 100644
> > > > > > > --- a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
> > > > > > > +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
> > > > > > > @@ -26,6 +26,7 @@ properties:
> > > > > > >              - qcom,pcie-ipq8064-v2
> > > > > > >              - qcom,pcie-ipq8074
> > > > > > >              - qcom,pcie-ipq8074-gen3
> > > > > > > +          - qcom,pcie-ipq9574
> > > > > > >              - qcom,pcie-msm8996
> > > > > > >              - qcom,pcie-qcs404
> > > > > > >              - qcom,pcie-sa8540p
> > > > > > > @@ -44,11 +45,11 @@ properties:
> > > > > > >      reg:
> > > > > > >        minItems: 4
> > > > > > > -    maxItems: 5
> > > > > > > +    maxItems: 6
> > > > > > >      reg-names:
> > > > > > >        minItems: 4
> > > > > > > -    maxItems: 5
> > > > > > > +    maxItems: 6
> > > > > > >      interrupts:
> > > > > > >        minItems: 1
> > > > > > > @@ -105,6 +106,8 @@ properties:
> > > > > > >        items:
> > > > > > >          - const: pciephy
> > > > > > > +  msi-parent: true
> > > > > > > +
> > > > > > >      power-domains:
> > > > > > >        maxItems: 1
> > > > > > > @@ -173,6 +176,27 @@ allOf:
> > > > > > >                - const: parf # Qualcomm specific registers
> > > > > > >                - const: config # PCIe configuration space
> > > > > > > +  - if:
> > > > > > > +      properties:
> > > > > > > +        compatible:
> > > > > > > +          contains:
> > > > > > > +            enum:
> > > > > > > +              - qcom,pcie-ipq9574
> > > > > > > +    then:
> > > > > > > +      properties:
> > > > > > > +        reg:
> > > > > > > +          minItems: 5
> > > > > > > +          maxItems: 6
> > > > > > > +        reg-names:
> > > > > > > +          minItems: 5
> > > > > > > +          items:
> > > > > > > +            - const: dbi # DesignWare PCIe registers
> > > > > > > +            - const: elbi # External local bus interface registers
> > > > > > > +            - const: atu # ATU address space
> > > > > > > +            - const: parf # Qualcomm specific registers
> > > > > > > +            - const: config # PCIe configuration space
> > > > > > > +            - const: aggr_noc #PCIe aggr_noc
> > > > > > 
> > > > > > Why do you need this region unlike other SoCs? Is the driver making use of it?
> > > > > We have the aggr_noc region in ipq9574 to achieve higher throughput & to
> > > > > handle multiple PCIe instances. The driver uses it to rate adapt 1-lane PCIe
> > > > > clocks. My bad, missed it. Will add the driver changes in V2.
> > > > 
> > > > Hmm, this is something new. How can you achieve higher throughput with this
> > > > region? Can you explain more on how it is used?
> > > 
> > > Based on the name of the region, it looks like it is an interconnect region.
> > > 
> > 
> > Well, we only have BCM based interconnects so far. That's why I was curious
> > about this region and its purpose.
> For connected PCIe slave devices that are running at frequency lesser
> than the ANOC frequency (342MHz), the rate adapter of ANOC needs to be
> configured
> > 
> > > Devi, if this is the case, then you have to handle it through the interconnect driver, rather than poking directly into these registers.
> > 
> > If that so, it doesn't need to be added in this series itself. I believe that
> > without aggr_noc region, the PCIe controller can still function properly with
> > reduced performance. But you can add the interconnect support later as a
> > separate series.
> Sure, okay. The ANOC runs at a fixed frequency of 342MHz and the
> interconnect clocks are not scaled. The aggr_noc register is just a magic
> register for configuring it's rate adapter to ensure no wait cycles are
> inserted.
> 

If the purpose of the aggr_noc region is to configure the interconnect clock,
then it should be modeled as an interconnect driver.

Thanks,
Mani

> > 
> > Thanks,
> > Mani
> > 
> > > 
> > > 
> > > > 
> > > > Thanks,
> > > > Mani
> > > > 
> > > > > > 
> > > > > > Thanks,
> > > > > > Mani
> > > > > > 
> > > > > > > +
> > > > > > >      - if:
> > > > > > >          properties:
> > > > > > >            compatible:
> > > > > > > @@ -365,6 +389,39 @@ allOf:
> > > > > > >                - const: ahb # AHB Reset
> > > > > > >                - const: axi_m_sticky # AXI Master Sticky reset
> > > > > > > +  - if:
> > > > > > > +      properties:
> > > > > > > +        compatible:
> > > > > > > +          contains:
> > > > > > > +            enum:
> > > > > > > +              - qcom,pcie-ipq9574
> > > > > > > +    then:
> > > > > > > +      properties:
> > > > > > > +        clocks:
> > > > > > > +          minItems: 6
> > > > > > > +          maxItems: 6
> > > > > > > +        clock-names:
> > > > > > > +          items:
> > > > > > > +            - const: ahb  # AHB clock
> > > > > > > +            - const: aux  # Auxiliary clock
> > > > > > > +            - const: axi_m # AXI Master clock
> > > > > > > +            - const: axi_s # AXI Slave clock
> > > > > > > +            - const: axi_bridge # AXI bridge clock
> > > > > > > +            - const: rchng
> > > > > > > +        resets:
> > > > > > > +          minItems: 8
> > > > > > > +          maxItems: 8
> > > > > > > +        reset-names:
> > > > > > > +          items:
> > > > > > > +            - const: pipe # PIPE reset
> > > > > > > +            - const: sticky # Core Sticky reset
> > > > > > > +            - const: axi_s_sticky # AXI Slave Sticky reset
> > > > > > > +            - const: axi_s # AXI Slave reset
> > > > > > > +            - const: axi_m_sticky # AXI Master Sticky reset
> > > > > > > +            - const: axi_m # AXI Master reset
> > > > > > > +            - const: aux # AUX Reset
> > > > > > > +            - const: ahb # AHB Reset
> > > > > > > +
> > > > > > >      - if:
> > > > > > >          properties:
> > > > > > >            compatible:
> > > > > > > @@ -681,6 +738,16 @@ allOf:
> > > > > > >            - interconnects
> > > > > > >            - interconnect-names
> > > > > > > +  - if:
> > > > > > > +      properties:
> > > > > > > +        compatible:
> > > > > > > +          contains:
> > > > > > > +            enum:
> > > > > > > +              - qcom,pcie-ipq9574
> > > > > > > +    then:
> > > > > > > +      required:
> > > > > > > +        - msi-parent
> > > > > > > +
> > > > > > >      - if:
> > > > > > >          not:
> > > > > > >            properties:
> > > > > > > @@ -693,6 +760,7 @@ allOf:
> > > > > > >                    - qcom,pcie-ipq8064v2
> > > > > > >                    - qcom,pcie-ipq8074
> > > > > > >                    - qcom,pcie-ipq8074-gen3
> > > > > > > +                - qcom,pcie-ipq9574
> > > > > > >                    - qcom,pcie-qcs404
> > > > > > >        then:
> > > > > > >          required:
> > > > > > > -- 
> > > > > > > 2.17.1
> > > > > > > 
> > > > > > 
> > > > > Thanks,
> > > > > Devi Priya
> > > > 
> > > 
> > 
> Thanks,
> Devi Priya
Devi Priya March 7, 2023, 2:40 p.m. UTC | #11 
On 3/7/2023 6:26 PM, Manivannan Sadhasivam wrote:
> On Tue, Mar 07, 2023 at 03:15:08PM +0530, Devi Priya wrote:
>>
>>
>> On 3/3/2023 11:10 PM, Manivannan Sadhasivam wrote:
>>> On Fri, Mar 03, 2023 at 05:16:58PM +0200, Dmitry Baryshkov wrote:
>>>> 28 февраля 2023 г. 08:33:58 GMT+02:00, Manivannan Sadhasivam <mani@kernel.org> пишет:
>>>>> On Tue, Feb 28, 2023 at 10:56:53AM +0530, Devi Priya wrote:
>>>>>>
>>>>>>
>>>>>> On 2/24/2023 1:53 PM, Manivannan Sadhasivam wrote:
>>>>>>> On Tue, Feb 14, 2023 at 10:11:29PM +0530, Devi Priya wrote:
>>>>>>>> Document the compatible for IPQ9574
>>>>>>>>
>>>>>> Hi Mani, Thanks for taking time to review the patch.
>>>>>>>
>>>>>>> You didn't mention about the "msi-parent" property that is being added
>>>>>>> by this patch
>>>>>> Sure, will update the commit message in the next spin
>>>>>>>
>>>>>>>> Signed-off-by: Devi Priya <quic_devipriy@quicinc.com>
>>>>>>>> ---
>>>>>>>>     .../devicetree/bindings/pci/qcom,pcie.yaml    | 72 ++++++++++++++++++-
>>>>>>>>     1 file changed, 70 insertions(+), 2 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
>>>>>>>> index 872817d6d2bd..dabdf2684e2d 100644
>>>>>>>> --- a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
>>>>>>>> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
>>>>>>>> @@ -26,6 +26,7 @@ properties:
>>>>>>>>               - qcom,pcie-ipq8064-v2
>>>>>>>>               - qcom,pcie-ipq8074
>>>>>>>>               - qcom,pcie-ipq8074-gen3
>>>>>>>> +          - qcom,pcie-ipq9574
>>>>>>>>               - qcom,pcie-msm8996
>>>>>>>>               - qcom,pcie-qcs404
>>>>>>>>               - qcom,pcie-sa8540p
>>>>>>>> @@ -44,11 +45,11 @@ properties:
>>>>>>>>       reg:
>>>>>>>>         minItems: 4
>>>>>>>> -    maxItems: 5
>>>>>>>> +    maxItems: 6
>>>>>>>>       reg-names:
>>>>>>>>         minItems: 4
>>>>>>>> -    maxItems: 5
>>>>>>>> +    maxItems: 6
>>>>>>>>       interrupts:
>>>>>>>>         minItems: 1
>>>>>>>> @@ -105,6 +106,8 @@ properties:
>>>>>>>>         items:
>>>>>>>>           - const: pciephy
>>>>>>>> +  msi-parent: true
>>>>>>>> +
>>>>>>>>       power-domains:
>>>>>>>>         maxItems: 1
>>>>>>>> @@ -173,6 +176,27 @@ allOf:
>>>>>>>>                 - const: parf # Qualcomm specific registers
>>>>>>>>                 - const: config # PCIe configuration space
>>>>>>>> +  - if:
>>>>>>>> +      properties:
>>>>>>>> +        compatible:
>>>>>>>> +          contains:
>>>>>>>> +            enum:
>>>>>>>> +              - qcom,pcie-ipq9574
>>>>>>>> +    then:
>>>>>>>> +      properties:
>>>>>>>> +        reg:
>>>>>>>> +          minItems: 5
>>>>>>>> +          maxItems: 6
>>>>>>>> +        reg-names:
>>>>>>>> +          minItems: 5
>>>>>>>> +          items:
>>>>>>>> +            - const: dbi # DesignWare PCIe registers
>>>>>>>> +            - const: elbi # External local bus interface registers
>>>>>>>> +            - const: atu # ATU address space
>>>>>>>> +            - const: parf # Qualcomm specific registers
>>>>>>>> +            - const: config # PCIe configuration space
>>>>>>>> +            - const: aggr_noc #PCIe aggr_noc
>>>>>>>
>>>>>>> Why do you need this region unlike other SoCs? Is the driver making use of it?
>>>>>> We have the aggr_noc region in ipq9574 to achieve higher throughput & to
>>>>>> handle multiple PCIe instances. The driver uses it to rate adapt 1-lane PCIe
>>>>>> clocks. My bad, missed it. Will add the driver changes in V2.
>>>>>
>>>>> Hmm, this is something new. How can you achieve higher throughput with this
>>>>> region? Can you explain more on how it is used?
>>>>
>>>> Based on the name of the region, it looks like it is an interconnect region.
>>>>
>>>
>>> Well, we only have BCM based interconnects so far. That's why I was curious
>>> about this region and its purpose.
>> For connected PCIe slave devices that are running at frequency lesser
>> than the ANOC frequency (342MHz), the rate adapter of ANOC needs to be
>> configured
>>>
>>>> Devi, if this is the case, then you have to handle it through the interconnect driver, rather than poking directly into these registers.
>>>
>>> If that so, it doesn't need to be added in this series itself. I believe that
>>> without aggr_noc region, the PCIe controller can still function properly with
>>> reduced performance. But you can add the interconnect support later as a
>>> separate series.
>> Sure, okay. The ANOC runs at a fixed frequency of 342MHz and the
>> interconnect clocks are not scaled. The aggr_noc register is just a magic
>> register for configuring it's rate adapter to ensure no wait cycles are
>> inserted.
>>
> 
> If the purpose of the aggr_noc region is to configure the interconnect clock,
> then it should be modeled as an interconnect driver.
Can we use 'syscon' here, as we are not scaling the interconnect
frequency and this is just a single register write for setting
the rate adapter?
> 
> Thanks,
> Mani
> 
>>>
>>> Thanks,
>>> Mani
>>>
>>>>
>>>>
>>>>>
>>>>> Thanks,
>>>>> Mani
>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Mani
>>>>>>>
>>>>>>>> +
>>>>>>>>       - if:
>>>>>>>>           properties:
>>>>>>>>             compatible:
>>>>>>>> @@ -365,6 +389,39 @@ allOf:
>>>>>>>>                 - const: ahb # AHB Reset
>>>>>>>>                 - const: axi_m_sticky # AXI Master Sticky reset
>>>>>>>> +  - if:
>>>>>>>> +      properties:
>>>>>>>> +        compatible:
>>>>>>>> +          contains:
>>>>>>>> +            enum:
>>>>>>>> +              - qcom,pcie-ipq9574
>>>>>>>> +    then:
>>>>>>>> +      properties:
>>>>>>>> +        clocks:
>>>>>>>> +          minItems: 6
>>>>>>>> +          maxItems: 6
>>>>>>>> +        clock-names:
>>>>>>>> +          items:
>>>>>>>> +            - const: ahb  # AHB clock
>>>>>>>> +            - const: aux  # Auxiliary clock
>>>>>>>> +            - const: axi_m # AXI Master clock
>>>>>>>> +            - const: axi_s # AXI Slave clock
>>>>>>>> +            - const: axi_bridge # AXI bridge clock
>>>>>>>> +            - const: rchng
>>>>>>>> +        resets:
>>>>>>>> +          minItems: 8
>>>>>>>> +          maxItems: 8
>>>>>>>> +        reset-names:
>>>>>>>> +          items:
>>>>>>>> +            - const: pipe # PIPE reset
>>>>>>>> +            - const: sticky # Core Sticky reset
>>>>>>>> +            - const: axi_s_sticky # AXI Slave Sticky reset
>>>>>>>> +            - const: axi_s # AXI Slave reset
>>>>>>>> +            - const: axi_m_sticky # AXI Master Sticky reset
>>>>>>>> +            - const: axi_m # AXI Master reset
>>>>>>>> +            - const: aux # AUX Reset
>>>>>>>> +            - const: ahb # AHB Reset
>>>>>>>> +
>>>>>>>>       - if:
>>>>>>>>           properties:
>>>>>>>>             compatible:
>>>>>>>> @@ -681,6 +738,16 @@ allOf:
>>>>>>>>             - interconnects
>>>>>>>>             - interconnect-names
>>>>>>>> +  - if:
>>>>>>>> +      properties:
>>>>>>>> +        compatible:
>>>>>>>> +          contains:
>>>>>>>> +            enum:
>>>>>>>> +              - qcom,pcie-ipq9574
>>>>>>>> +    then:
>>>>>>>> +      required:
>>>>>>>> +        - msi-parent
>>>>>>>> +
>>>>>>>>       - if:
>>>>>>>>           not:
>>>>>>>>             properties:
>>>>>>>> @@ -693,6 +760,7 @@ allOf:
>>>>>>>>                     - qcom,pcie-ipq8064v2
>>>>>>>>                     - qcom,pcie-ipq8074
>>>>>>>>                     - qcom,pcie-ipq8074-gen3
>>>>>>>> +                - qcom,pcie-ipq9574
>>>>>>>>                     - qcom,pcie-qcs404
>>>>>>>>         then:
>>>>>>>>           required:
>>>>>>>> -- 
>>>>>>>> 2.17.1
>>>>>>>>
>>>>>>>
>>>>>> Thanks,
>>>>>> Devi Priya
>>>>>
>>>>
>>>
>> Thanks,
>> Devi Priya
>
Dmitry Baryshkov March 7, 2023, 2:56 p.m. UTC | #12 
On Tue, 7 Mar 2023 at 16:40, Devi Priya <quic_devipriy@quicinc.com> wrote:
>
>
>
> On 3/7/2023 6:26 PM, Manivannan Sadhasivam wrote:
> > On Tue, Mar 07, 2023 at 03:15:08PM +0530, Devi Priya wrote:
> >>
> >>
> >> On 3/3/2023 11:10 PM, Manivannan Sadhasivam wrote:
> >>> On Fri, Mar 03, 2023 at 05:16:58PM +0200, Dmitry Baryshkov wrote:
> >>>> 28 февраля 2023 г. 08:33:58 GMT+02:00, Manivannan Sadhasivam <mani@kernel.org> пишет:
> >>>>> On Tue, Feb 28, 2023 at 10:56:53AM +0530, Devi Priya wrote:
> >>>>>>
> >>>>>>
> >>>>>> On 2/24/2023 1:53 PM, Manivannan Sadhasivam wrote:
> >>>>>>> On Tue, Feb 14, 2023 at 10:11:29PM +0530, Devi Priya wrote:
> >>>>>>>> Document the compatible for IPQ9574
> >>>>>>>>
> >>>>>> Hi Mani, Thanks for taking time to review the patch.
> >>>>>>>
> >>>>>>> You didn't mention about the "msi-parent" property that is being added
> >>>>>>> by this patch
> >>>>>> Sure, will update the commit message in the next spin
> >>>>>>>
> >>>>>>>> Signed-off-by: Devi Priya <quic_devipriy@quicinc.com>
> >>>>>>>> ---
> >>>>>>>>     .../devicetree/bindings/pci/qcom,pcie.yaml    | 72 ++++++++++++++++++-
> >>>>>>>>     1 file changed, 70 insertions(+), 2 deletions(-)
> >>>>>>>>
> >>>>>>>> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
> >>>>>>>> index 872817d6d2bd..dabdf2684e2d 100644
> >>>>>>>> --- a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
> >>>>>>>> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
> >>>>>>>> @@ -26,6 +26,7 @@ properties:
> >>>>>>>>               - qcom,pcie-ipq8064-v2
> >>>>>>>>               - qcom,pcie-ipq8074
> >>>>>>>>               - qcom,pcie-ipq8074-gen3
> >>>>>>>> +          - qcom,pcie-ipq9574
> >>>>>>>>               - qcom,pcie-msm8996
> >>>>>>>>               - qcom,pcie-qcs404
> >>>>>>>>               - qcom,pcie-sa8540p
> >>>>>>>> @@ -44,11 +45,11 @@ properties:
> >>>>>>>>       reg:
> >>>>>>>>         minItems: 4
> >>>>>>>> -    maxItems: 5
> >>>>>>>> +    maxItems: 6
> >>>>>>>>       reg-names:
> >>>>>>>>         minItems: 4
> >>>>>>>> -    maxItems: 5
> >>>>>>>> +    maxItems: 6
> >>>>>>>>       interrupts:
> >>>>>>>>         minItems: 1
> >>>>>>>> @@ -105,6 +106,8 @@ properties:
> >>>>>>>>         items:
> >>>>>>>>           - const: pciephy
> >>>>>>>> +  msi-parent: true
> >>>>>>>> +
> >>>>>>>>       power-domains:
> >>>>>>>>         maxItems: 1
> >>>>>>>> @@ -173,6 +176,27 @@ allOf:
> >>>>>>>>                 - const: parf # Qualcomm specific registers
> >>>>>>>>                 - const: config # PCIe configuration space
> >>>>>>>> +  - if:
> >>>>>>>> +      properties:
> >>>>>>>> +        compatible:
> >>>>>>>> +          contains:
> >>>>>>>> +            enum:
> >>>>>>>> +              - qcom,pcie-ipq9574
> >>>>>>>> +    then:
> >>>>>>>> +      properties:
> >>>>>>>> +        reg:
> >>>>>>>> +          minItems: 5
> >>>>>>>> +          maxItems: 6
> >>>>>>>> +        reg-names:
> >>>>>>>> +          minItems: 5
> >>>>>>>> +          items:
> >>>>>>>> +            - const: dbi # DesignWare PCIe registers
> >>>>>>>> +            - const: elbi # External local bus interface registers
> >>>>>>>> +            - const: atu # ATU address space
> >>>>>>>> +            - const: parf # Qualcomm specific registers
> >>>>>>>> +            - const: config # PCIe configuration space
> >>>>>>>> +            - const: aggr_noc #PCIe aggr_noc
> >>>>>>>
> >>>>>>> Why do you need this region unlike other SoCs? Is the driver making use of it?
> >>>>>> We have the aggr_noc region in ipq9574 to achieve higher throughput & to
> >>>>>> handle multiple PCIe instances. The driver uses it to rate adapt 1-lane PCIe
> >>>>>> clocks. My bad, missed it. Will add the driver changes in V2.
> >>>>>
> >>>>> Hmm, this is something new. How can you achieve higher throughput with this
> >>>>> region? Can you explain more on how it is used?
> >>>>
> >>>> Based on the name of the region, it looks like it is an interconnect region.
> >>>>
> >>>
> >>> Well, we only have BCM based interconnects so far. That's why I was curious
> >>> about this region and its purpose.
> >> For connected PCIe slave devices that are running at frequency lesser
> >> than the ANOC frequency (342MHz), the rate adapter of ANOC needs to be
> >> configured
> >>>
> >>>> Devi, if this is the case, then you have to handle it through the interconnect driver, rather than poking directly into these registers.
> >>>
> >>> If that so, it doesn't need to be added in this series itself. I believe that
> >>> without aggr_noc region, the PCIe controller can still function properly with
> >>> reduced performance. But you can add the interconnect support later as a
> >>> separate series.
> >> Sure, okay. The ANOC runs at a fixed frequency of 342MHz and the
> >> interconnect clocks are not scaled. The aggr_noc register is just a magic
> >> register for configuring it's rate adapter to ensure no wait cycles are
> >> inserted.
> >>
> >
> > If the purpose of the aggr_noc region is to configure the interconnect clock,
> > then it should be modeled as an interconnect driver.
> Can we use 'syscon' here, as we are not scaling the interconnect
> frequency and this is just a single register write for setting
> the rate adapter?

It should be done outside of the PCIe driver.
It is not "just a single register". It is also setting the anoc/snoc
clocks for USB. And maybe something else, which we haven't seen at
this moment. You are still setting up the NoC, even if the icc
frequency is not scaled.
Devi Priya March 8, 2023, 8:49 a.m. UTC | #13 
On 3/7/2023 8:26 PM, Dmitry Baryshkov wrote:
> On Tue, 7 Mar 2023 at 16:40, Devi Priya <quic_devipriy@quicinc.com> wrote:
>>
>>
>>
>> On 3/7/2023 6:26 PM, Manivannan Sadhasivam wrote:
>>> On Tue, Mar 07, 2023 at 03:15:08PM +0530, Devi Priya wrote:
>>>>
>>>>
>>>> On 3/3/2023 11:10 PM, Manivannan Sadhasivam wrote:
>>>>> On Fri, Mar 03, 2023 at 05:16:58PM +0200, Dmitry Baryshkov wrote:
>>>>>> 28 февраля 2023 г. 08:33:58 GMT+02:00, Manivannan Sadhasivam <mani@kernel.org> пишет:
>>>>>>> On Tue, Feb 28, 2023 at 10:56:53AM +0530, Devi Priya wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 2/24/2023 1:53 PM, Manivannan Sadhasivam wrote:
>>>>>>>>> On Tue, Feb 14, 2023 at 10:11:29PM +0530, Devi Priya wrote:
>>>>>>>>>> Document the compatible for IPQ9574
>>>>>>>>>>
>>>>>>>> Hi Mani, Thanks for taking time to review the patch.
>>>>>>>>>
>>>>>>>>> You didn't mention about the "msi-parent" property that is being added
>>>>>>>>> by this patch
>>>>>>>> Sure, will update the commit message in the next spin
>>>>>>>>>
>>>>>>>>>> Signed-off-by: Devi Priya <quic_devipriy@quicinc.com>
>>>>>>>>>> ---
>>>>>>>>>>      .../devicetree/bindings/pci/qcom,pcie.yaml    | 72 ++++++++++++++++++-
>>>>>>>>>>      1 file changed, 70 insertions(+), 2 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
>>>>>>>>>> index 872817d6d2bd..dabdf2684e2d 100644
>>>>>>>>>> --- a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
>>>>>>>>>> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
>>>>>>>>>> @@ -26,6 +26,7 @@ properties:
>>>>>>>>>>                - qcom,pcie-ipq8064-v2
>>>>>>>>>>                - qcom,pcie-ipq8074
>>>>>>>>>>                - qcom,pcie-ipq8074-gen3
>>>>>>>>>> +          - qcom,pcie-ipq9574
>>>>>>>>>>                - qcom,pcie-msm8996
>>>>>>>>>>                - qcom,pcie-qcs404
>>>>>>>>>>                - qcom,pcie-sa8540p
>>>>>>>>>> @@ -44,11 +45,11 @@ properties:
>>>>>>>>>>        reg:
>>>>>>>>>>          minItems: 4
>>>>>>>>>> -    maxItems: 5
>>>>>>>>>> +    maxItems: 6
>>>>>>>>>>        reg-names:
>>>>>>>>>>          minItems: 4
>>>>>>>>>> -    maxItems: 5
>>>>>>>>>> +    maxItems: 6
>>>>>>>>>>        interrupts:
>>>>>>>>>>          minItems: 1
>>>>>>>>>> @@ -105,6 +106,8 @@ properties:
>>>>>>>>>>          items:
>>>>>>>>>>            - const: pciephy
>>>>>>>>>> +  msi-parent: true
>>>>>>>>>> +
>>>>>>>>>>        power-domains:
>>>>>>>>>>          maxItems: 1
>>>>>>>>>> @@ -173,6 +176,27 @@ allOf:
>>>>>>>>>>                  - const: parf # Qualcomm specific registers
>>>>>>>>>>                  - const: config # PCIe configuration space
>>>>>>>>>> +  - if:
>>>>>>>>>> +      properties:
>>>>>>>>>> +        compatible:
>>>>>>>>>> +          contains:
>>>>>>>>>> +            enum:
>>>>>>>>>> +              - qcom,pcie-ipq9574
>>>>>>>>>> +    then:
>>>>>>>>>> +      properties:
>>>>>>>>>> +        reg:
>>>>>>>>>> +          minItems: 5
>>>>>>>>>> +          maxItems: 6
>>>>>>>>>> +        reg-names:
>>>>>>>>>> +          minItems: 5
>>>>>>>>>> +          items:
>>>>>>>>>> +            - const: dbi # DesignWare PCIe registers
>>>>>>>>>> +            - const: elbi # External local bus interface registers
>>>>>>>>>> +            - const: atu # ATU address space
>>>>>>>>>> +            - const: parf # Qualcomm specific registers
>>>>>>>>>> +            - const: config # PCIe configuration space
>>>>>>>>>> +            - const: aggr_noc #PCIe aggr_noc
>>>>>>>>>
>>>>>>>>> Why do you need this region unlike other SoCs? Is the driver making use of it?
>>>>>>>> We have the aggr_noc region in ipq9574 to achieve higher throughput & to
>>>>>>>> handle multiple PCIe instances. The driver uses it to rate adapt 1-lane PCIe
>>>>>>>> clocks. My bad, missed it. Will add the driver changes in V2.
>>>>>>>
>>>>>>> Hmm, this is something new. How can you achieve higher throughput with this
>>>>>>> region? Can you explain more on how it is used?
>>>>>>
>>>>>> Based on the name of the region, it looks like it is an interconnect region.
>>>>>>
>>>>>
>>>>> Well, we only have BCM based interconnects so far. That's why I was curious
>>>>> about this region and its purpose.
>>>> For connected PCIe slave devices that are running at frequency lesser
>>>> than the ANOC frequency (342MHz), the rate adapter of ANOC needs to be
>>>> configured
>>>>>
>>>>>> Devi, if this is the case, then you have to handle it through the interconnect driver, rather than poking directly into these registers.
>>>>>
>>>>> If that so, it doesn't need to be added in this series itself. I believe that
>>>>> without aggr_noc region, the PCIe controller can still function properly with
>>>>> reduced performance. But you can add the interconnect support later as a
>>>>> separate series.
>>>> Sure, okay. The ANOC runs at a fixed frequency of 342MHz and the
>>>> interconnect clocks are not scaled. The aggr_noc register is just a magic
>>>> register for configuring it's rate adapter to ensure no wait cycles are
>>>> inserted.
>>>>
>>>
>>> If the purpose of the aggr_noc region is to configure the interconnect clock,
>>> then it should be modeled as an interconnect driver.
>> Can we use 'syscon' here, as we are not scaling the interconnect
>> frequency and this is just a single register write for setting
>> the rate adapter?
> 
> It should be done outside of the PCIe driver.
> It is not "just a single register". It is also setting the anoc/snoc
> clocks for USB. And maybe something else, which we haven't seen at
> this moment. You are still setting up the NoC, even if the icc
> frequency is not scaled.
> 
Sure Dmitry, Got it

Regards,
Devi Priya
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
index 872817d6d2bd..dabdf2684e2d 100644
--- a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
+++ b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
@@ -26,6 +26,7 @@  properties:
           - qcom,pcie-ipq8064-v2
           - qcom,pcie-ipq8074
           - qcom,pcie-ipq8074-gen3
+          - qcom,pcie-ipq9574
           - qcom,pcie-msm8996
           - qcom,pcie-qcs404
           - qcom,pcie-sa8540p
@@ -44,11 +45,11 @@  properties:
 
   reg:
     minItems: 4
-    maxItems: 5
+    maxItems: 6
 
   reg-names:
     minItems: 4
-    maxItems: 5
+    maxItems: 6
 
   interrupts:
     minItems: 1
@@ -105,6 +106,8 @@  properties:
     items:
       - const: pciephy
 
+  msi-parent: true
+
   power-domains:
     maxItems: 1
 
@@ -173,6 +176,27 @@  allOf:
             - const: parf # Qualcomm specific registers
             - const: config # PCIe configuration space
 
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - qcom,pcie-ipq9574
+    then:
+      properties:
+        reg:
+          minItems: 5
+          maxItems: 6
+        reg-names:
+          minItems: 5
+          items:
+            - const: dbi # DesignWare PCIe registers
+            - const: elbi # External local bus interface registers
+            - const: atu # ATU address space
+            - const: parf # Qualcomm specific registers
+            - const: config # PCIe configuration space
+            - const: aggr_noc #PCIe aggr_noc
+
   - if:
       properties:
         compatible:
@@ -365,6 +389,39 @@  allOf:
             - const: ahb # AHB Reset
             - const: axi_m_sticky # AXI Master Sticky reset
 
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - qcom,pcie-ipq9574
+    then:
+      properties:
+        clocks:
+          minItems: 6
+          maxItems: 6
+        clock-names:
+          items:
+            - const: ahb  # AHB clock
+            - const: aux  # Auxiliary clock
+            - const: axi_m # AXI Master clock
+            - const: axi_s # AXI Slave clock
+            - const: axi_bridge # AXI bridge clock
+            - const: rchng
+        resets:
+          minItems: 8
+          maxItems: 8
+        reset-names:
+          items:
+            - const: pipe # PIPE reset
+            - const: sticky # Core Sticky reset
+            - const: axi_s_sticky # AXI Slave Sticky reset
+            - const: axi_s # AXI Slave reset
+            - const: axi_m_sticky # AXI Master Sticky reset
+            - const: axi_m # AXI Master reset
+            - const: aux # AUX Reset
+            - const: ahb # AHB Reset
+
   - if:
       properties:
         compatible:
@@ -681,6 +738,16 @@  allOf:
         - interconnects
         - interconnect-names
 
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - qcom,pcie-ipq9574
+    then:
+      required:
+        - msi-parent
+
   - if:
       not:
         properties:
@@ -693,6 +760,7 @@  allOf:
                 - qcom,pcie-ipq8064v2
                 - qcom,pcie-ipq8074
                 - qcom,pcie-ipq8074-gen3
+                - qcom,pcie-ipq9574
                 - qcom,pcie-qcs404
     then:
       required: