Date:   Fri, 3 Mar 2023 23:10:36 +0530
From:   Manivannan Sadhasivam <mani@kernel.org>
To:     Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Cc:     Manivannan Sadhasivam <mani@kernel.org>,
        Devi Priya <quic_devipriy@quicinc.com>, agross@kernel.org,
        andersson@kernel.org, konrad.dybcio@linaro.org,
        lpieralisi@kernel.org, kw@linux.com, robh@kernel.org,
        bhelgaas@google.com, krzysztof.kozlowski+dt@linaro.org,
        vkoul@kernel.org, kishon@kernel.org, mturquette@baylibre.com,
        sboyd@kernel.org, p.zabel@pengutronix.de, svarbanov@mm-sol.com,
        linux-arm-msm@vger.kernel.org, linux-pci@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-phy@lists.infradead.org, linux-clk@vger.kernel.org,
        quic_srichara@quicinc.com, quic_gokulsri@quicinc.com,
        quic_sjaganat@quicinc.com, quic_kathirav@quicinc.com,
        quic_arajkuma@quicinc.com, quic_anusha@quicinc.com
Subject: Re: [PATCH 1/7] dt-bindings: PCI: qcom: Add IPQ9574 specific
 compatible
Message-ID: <20230303174036.GB6782@thinkpad>
References: <20230214164135.17039-1-quic_devipriy@quicinc.com>
 <20230214164135.17039-2-quic_devipriy@quicinc.com>
 <20230224082332.GA5443@thinkpad>
 <bd153038-4427-1f11-1941-5f13fec01cf7@quicinc.com>
 <20230228063358.GA4839@thinkpad>
 <9BD62D8E-4E14-4269-B72D-C83EF4D43040@linaro.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <9BD62D8E-4E14-4269-B72D-C83EF4D43040@linaro.org>
Precedence: bulk

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
> >
> 

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