Re: [PATCH v1] arm64: dts: qcom: qcs615: Add QUPv3 configuration

[Viken Dadhaniya]

On 10/28/2024 6:38 PM, Konrad Dybcio wrote:
> On 28.10.2024 12:19 PM, Viken Dadhaniya wrote:
> > On 10/26/2024 1:10 AM, Konrad Dybcio wrote:
> > > On 11.10.2024 12:33 PM, Viken Dadhaniya wrote:
> > > > Add DT support for QUPv3 Serial Engines.
> > > >
> > > > Co-developed-by: Mukesh Kumar Savaliya <quic_msavaliy@xxxxxxxxxxx>
> > > > Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@xxxxxxxxxxx>
> > > > Signed-off-by: Viken Dadhaniya <quic_vdadhani@xxxxxxxxxxx>
> > > > ---
> > >
> > > [...]
> > >
> > > > +    qup_opp_table: opp-table-qup {
> > > > +        compatible = "operating-points-v2";
> > >
> > > opp-shared;
> >
> > Added in v2.
> >
> > > > +
> > > > +        opp-75000000 {
> > > > +            opp-hz = /bits/ 64 <75000000>;
> > > > +            required-opps = <&rpmhpd_opp_low_svs>;
> > > > +        };
> > > > +
> > > > +        opp-100000000 {
> > > > +            opp-hz = /bits/ 64 <100000000>;
> > > > +            required-opps = <&rpmhpd_opp_svs>;
> > > > +        };
> > > > +
> > > > +        opp-128000000 {
> > > > +            opp-hz = /bits/ 64 <128000000>;
> > > > +            required-opps = <&rpmhpd_opp_nom>;
> > > > +        };
> > > > +    };
> > > > +
> > > >        psci {
> > > >            compatible = "arm,psci-1.0";
> > > >            method = "smc";
> > > > @@ -392,6 +427,24 @@
> > > >                #size-cells = <1>;
> > > >            };
> > > >    +        gpi_dma0: qcom,gpi-dma@800000  {
> > > > +            compatible = "qcom,sdm845-gpi-dma";
> > >
> > > You must define a new compatible for qcs615, sdm845 is used as a fallback
> > > (so that we don't have to add new driver entries). You will however need
> > > to submit a separate dt-bindings change.
> >
> > We have added sdm845 in compatible due to below comment in driver file
> >
> > File: https://github.com/torvalds/linux/blob/81983758430957d9a5cb3333fe324fd70cf63e7e/drivers/dma/qcom/gpi.c#L2284
> >
> >    /*
> >     * Do not grow the list for compatible devices. Instead use
> >     * qcom,sdm845-gpi-dma (for ee_offset = 0x0) or qcom,sm6350-gpi-dma
> >     * (for ee_offset = 0x10000).
> >     */
> >
> > Do we still require new compatible for qcs615 ?
>
> The comment concerns the driver side. All devicetree compatible strings
> present in the tree must be documented in Documentation/devicetree/bindings/
> which is where you should add your one as well.

Thanks, Pushed separate patch for GPI documentation patch.
https://lore.kernel.org/all/w66ki7lwrqol24iptikn7ccna25ujqoywjena5ulekf6vynxny@dylbj2r34h7l/T/


> > > > +            reg = <0x0 0x800000 0x0 0x60000>;
> > > > +            #dma-cells = <3>;
> > > > +            interrupts = <GIC_SPI 244 IRQ_TYPE_LEVEL_HIGH>,
> > > > +                     <GIC_SPI 245 IRQ_TYPE_LEVEL_HIGH>,
> > > > +                     <GIC_SPI 246 IRQ_TYPE_LEVEL_HIGH>,
> > > > +                     <GIC_SPI 247 IRQ_TYPE_LEVEL_HIGH>,
> > > > +                     <GIC_SPI 248 IRQ_TYPE_LEVEL_HIGH>,
> > > > +                     <GIC_SPI 249 IRQ_TYPE_LEVEL_HIGH>,
> > > > +                     <GIC_SPI 250 IRQ_TYPE_LEVEL_HIGH>,
> > > > +                     <GIC_SPI 251 IRQ_TYPE_LEVEL_HIGH>;
> > > > +            dma-channels = <8>;
> > > > +            dma-channel-mask = <0xf>;
> > > > +            iommus = <&apps_smmu 0xd6 0x0>;
> > > > +            status = "disabled";
> > >
> > > Any reason?
> >
> > By default, we are disabling all nodes. In the target-specific file, we will update them based on the required use case.
> >
> > > > +        };
> > > > +
> > > >            qupv3_id_0: geniqup@8c0000 {
> > > >                compatible = "qcom,geni-se-qup";
> > > >                reg = <0x0 0x8c0000 0x0 0x6000>;
> > > > @@ -400,6 +453,7 @@
> > > >                     <&gcc GCC_QUPV3_WRAP_0_S_AHB_CLK>;
> > > >                clock-names = "m-ahb",
> > > >                          "s-ahb";
> > > > +            iommus = <&apps_smmu 0xc3 0x0>;
> > >
> > > This looks like a separate fix
> >
> > One QUP and UART (console) related change has been added as part of the base DTSI. Now, we are adding all remaining QUP and I2C/SPI/UART configurations with IOMMUs and other properties.
> >
> > Is there any specific reason to make this a separate fix?
>
> Did this work properly before?

Earlier, we verified the console UART use case, and it worked.

> Konrad