On Tue, 7 Mar 2023 at 16:40, Devi Priya <quic_devipriy@xxxxxxxxxxx> 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@xxxxxxxxxx> пишет: > >>>>> 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@xxxxxxxxxxx> > >>>>>>>> --- > >>>>>>>> .../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. -- With best wishes Dmitry
