by Konrad Dybcio

[permalink] [raw]

Subject: Re: [RESEND v6 5/8] arm64: dts: qcom: sc7280: Add LPASS PIL node

On 26.06.2023 13:07, Mohammad Rafi Shaik wrote:
>
> On 6/16/2023 4:55 PM, Konrad Dybcio wrote:
>> On 16.06.2023 12:35, Mohammad Rafi Shaik wrote:
>>> From: Srinivasa Rao Mandadapu <[email protected]>
>>>
>>> Add LPASS PIL node for sc7280 based audioreach platforms.
>>>
>>> Signed-off-by: Srinivasa Rao Mandadapu <[email protected]>
>>> Signed-off-by: Mohammad Rafi Shaik <[email protected]>
>>> ---
>> The node should reside in the SoC DTSI, ideally the bindings should
>> be compatible with the PAS setup to the point where only a compatible
>> swap is needed..
>>
>> Konrad
> Thanks for comment,
>
> The base SOC DTSI file already has an entry with the same physical base address which is using for ADSP BYPASS solution.
> For Audioreach solution required the same base address for the remoteproc device tree node.
>
> Will create a new common dtsi file for Audioreach as suggested by you in previous patch and add this in that common dtsi file.
>
> Please confirm is it okay ?
Let me copypaste the same answer I gave you to your off-list reply because
you seem to not have read it:

Taking a closer look, the qdsp6ss region is only used in combination
with PIL mode.. Perhaps it could be remodeled such that:

lpasscc only maps the top_cc region and the single clock within
(which lets us remove the pil-mode property)

qcom_q6v5_adsp takes care of everything inside the qdsp6ss region
(so, toggling the branches)

This would prevent us from trying to map the block @ 0x03000000 twice.

Konrad
>
> Rafi.
>
>>> .../sc7280-herobrine-audioreach-wcd9385.dtsi | 90 +++++++++++++++++++
>>> 1 file changed, 90 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine-audioreach-wcd9385.dtsi b/arch/arm64/boot/dts/qcom/sc7280-herobrine-audioreach-wcd9385.dtsi
>>> index 95d3aa08ebde..9daea1b25656 100644
>>> --- a/arch/arm64/boot/dts/qcom/sc7280-herobrine-audioreach-wcd9385.dtsi
>>> +++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine-audioreach-wcd9385.dtsi
>>> @@ -7,6 +7,8 @@
>>>    */
>>> #include <dt-bindings/sound/qcom,q6afe.h>
>>> +#include <dt-bindings/clock/qcom,lpass-sc7280.h>
>>> +#include <dt-bindings/soc/qcom,gpr.h>
>>> /{
>>>       /* BOARD-SPECIFIC TOP LEVEL NODES */
>>> @@ -105,4 +107,92 @@ platform {
>>>               };
>>>           };
>>>       };
>>> +
>>> +    remoteproc_adsp: remoteproc@3000000 {
>>> +        compatible = "qcom,sc7280-adsp-pil";
>>> +        reg = <0 0x03000000 0 0x5000>, <0 0x0355b000 0 0x10>;
>>> +        reg-names = "qdsp6ss_base", "lpass_efuse";
>>> +
>>> +        interrupts-extended = <&pdc 6 IRQ_TYPE_LEVEL_HIGH>,
>>> +                      <&adsp_smp2p_in 0 IRQ_TYPE_EDGE_RISING>,
>>> +                      <&adsp_smp2p_in 1 IRQ_TYPE_EDGE_RISING>,
>>> +                      <&adsp_smp2p_in 2 IRQ_TYPE_EDGE_RISING>,
>>> +                      <&adsp_smp2p_in 3 IRQ_TYPE_EDGE_RISING>,
>>> +                      <&adsp_smp2p_in 7 IRQ_TYPE_EDGE_RISING>;
>>> +
>>> +        interrupt-names = "wdog", "fatal", "ready",
>>> +                  "handover", "stop-ack",
>>> +                  "shutdown-ack";
>>> +
>>> +        qcom,qmp = <&aoss_qmp>;
>>> +
>>> +        clocks = <&rpmhcc RPMH_CXO_CLK>,
>>> +             <&gcc GCC_CFG_NOC_LPASS_CLK>;
>>> +        clock-names = "xo", "gcc_cfg_noc_lpass";
>>> +
>>> +        iommus = <&apps_smmu 0x1800 0x0>;
>>> +
>>> +        power-domains =    <&rpmhpd SC7280_CX>;
>>> +        power-domain-names = "cx";
>>> +
>>> +        required-opps = <&rpmhpd_opp_nom>;
>>> +
>>> +        resets = <&pdc_reset PDC_AUDIO_SYNC_RESET>,
>>> +             <&aoss_reset AOSS_CC_LPASS_RESTART>;
>>> +        reset-names = "pdc_sync", "cc_lpass";
>>> +
>>> +        qcom,halt-regs = <&tcsr_1 0x3000 0x5000 0x8000 0x13000>;
>>> +
>>> +        memory-region = <&adsp_mem>;
>>> +
>>> +        qcom,smem-states = <&adsp_smp2p_out 0>;
>>> +        qcom,smem-state-names = "stop";
>>> +
>>> +        glink-edge {
>>> +            interrupts-extended = <&ipcc IPCC_CLIENT_LPASS
>>> +                          IPCC_MPROC_SIGNAL_GLINK_QMP
>>> +                          IRQ_TYPE_EDGE_RISING>;
>>> +
>>> +            mboxes = <&ipcc IPCC_CLIENT_LPASS
>>> +                 IPCC_MPROC_SIGNAL_GLINK_QMP>;
>>> +
>>> +            label = "lpass";
>>> +            qcom,remote-pid = <2>;
>>> +
>>> +            gpr {
>>> +                compatible = "qcom,gpr";
>>> +                qcom,glink-channels = "adsp_apps";
>>> +                qcom,domain = <GPR_DOMAIN_ID_ADSP>;
>>> +                qcom,intents = <512 20>;
>>> +                #address-cells = <1>;
>>> +                #size-cells = <0>;
>>> +
>>> +                q6apm: service@1 {
>>> +                    compatible = "qcom,q6apm";
>>> +                    reg = <GPR_APM_MODULE_IID>;
>>> +                    #sound-dai-cells = <0>;
>>> +
>>> +                    q6apmdai: dais {
>>> +                        compatible = "qcom,q6apm-dais";
>>> +                        iommus = <&apps_smmu 0x1801 0x0>;
>>> +                    };
>>> +
>>> +                    q6apmbedai: bedais {
>>> +                        compatible = "qcom,q6apm-lpass-dais";
>>> +                        #sound-dai-cells = <1>;
>>> +                    };
>>> +                };
>>> +
>>> +                q6prm: service@2 {
>>> +                    compatible = "qcom,q6prm";
>>> +                    reg = <GPR_PRM_MODULE_IID>;
>>> +
>>> +                    q6prmcc: clock-controller {
>>> +                        compatible = "qcom,q6prm-lpass-clocks";
>>> +                        #clock-cells = <2>;
>>> +                    };
>>> +                };
>>> +            };
>>> +        };
>>> +    };
>>> };

2023-06-26 15:56:07

by Krzysztof Kozlowski

[permalink] [raw]

Subject: Re: [RESEND v6 6/8] arm64: dts: qcom: sc7280: Modify VA/RX/TX macro clock nodes for audioreach solution

On 26/06/2023 14:24, Konrad Dybcio wrote:
> On 26.06.2023 13:13, Mohammad Rafi Shaik wrote:
>>
>> On 6/16/2023 4:59 PM, Konrad Dybcio wrote:
>>> On 16.06.2023 12:35, Mohammad Rafi Shaik wrote:
>>>> From: Srinivasa Rao Mandadapu <[email protected]>
>>>>
>>>> Modify VA, RX and TX macro and lpass_tlmm clock properties and
>>>> enable them. For audioreach solution mclk, npl and fsgen clocks
>>>> are enabled through the q6prm clock driver.
>>>>
>>>> Delete the power domain properties from VA, RX and TX macro,
>>>> for audioreach solution the macro, dcodec power domains enabled
>>>> through the q6prm clock driver.
>>>>
>>>> Signed-off-by: Srinivasa Rao Mandadapu <[email protected]>
>>>> Signed-off-by: Mohammad Rafi Shaik <[email protected]>
>>>> ---
>>> Maybe sc7280-audioreach.dtsi containing all these changes that could be
>>> reused by others would be in order?
>> Thanks for comment,
>>
>> yes, will create a common sc7280-audioreach.dtsi file, which will contain common audioreach changes
>> and could be reused by others.
>>>> .../sc7280-herobrine-audioreach-wcd9385.dtsi | 43 +++++++++++++++++++
>>>> 1 file changed, 43 insertions(+)
>>>>
>>>> diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine-audioreach-wcd9385.dtsi b/arch/arm64/boot/dts/qcom/sc7280-herobrine-audioreach-wcd9385.dtsi
>>>> index 9daea1b25656..c02ca393378f 100644
>>>> --- a/arch/arm64/boot/dts/qcom/sc7280-herobrine-audioreach-wcd9385.dtsi
>>>> +++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine-audioreach-wcd9385.dtsi
>>>> @@ -196,3 +196,46 @@ q6prmcc: clock-controller {
>>>>           };
>>>>       };
>>>> };
>>>> +
>>>> +&lpass_rx_macro {
>>>> +    /delete-property/ power-domains;
>>>> +    /delete-property/ power-domain-names;
>>> Surely they shouldn't cause issues, even if the vote would be
>>> superfluous? They are still powered by these power domains, I'd assume?
>> No, In Audioreach case this macro and decodec clocks are not power by power domains,
>> this macro and decodec hw clocks are enrolled by q6prmcc clock voting.
> So the same piece of hardware is modeled differently twice?
>
> i.e. the same GDSCs are reached once with register accesses and once
> registered as "Q6 vote clocks"?
>
> that sounds like a bit of an overstep to register them with genpd and CCF
> depending on what entity controls them.. perhaps the "q6 vote clocks" could
> be remodeled as power domains as that's what they're ultimately seem to
> be referencing.. Krzysztof should have an opinion.

I think on SM8450 and newer these were already modeled as clocks, not
power domains. Anyway, for me, the previous/existing/coming code looks
like done by coincidence or copying some downstream choices, not with
any design in mind. Unfortunately, I don't know what to do with it now,
because the bindings were merged like that.

Best regards,
Krzysztof