Re: [PATCH 3/4] ARM: dts: qcom: apq8074-dragonboard: enable adsp and MSS

[Konrad Dybcio <konrad.dybcio@linaro.org>]

On 9.05.2023 01:38, Dmitry Baryshkov wrote:
> On Mon, 8 May 2023 at 21:01, Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
>>
>> On 08/05/2023 13:49, Dmitry Baryshkov wrote:
>>> On 08/05/2023 13:38, Konrad Dybcio wrote:
>>>>
>>>>
>>>> On 8.05.2023 12:33, Dmitry Baryshkov wrote:
>>>>> On 08/05/2023 11:33, Konrad Dybcio wrote:
>>>>>>
>>>>>>
>>>>>> On 7.05.2023 21:07, Dmitry Baryshkov wrote:
>>>>>>> Enable ADSP and Modem DSPs on APQ8074 dragonboard. The MSS region
>>>>>>> differs from the one defined in the msm8974, so it overriden locally.
>>>>>>>
>>>>>>> The modem is specified use mba.mbn instead of mbn.b00 (for the sake of
>>>>>>> similarity with other platforms). This requires a patch for remoteproc
>>>>>>> to be applied [1].
>>>>>>>
>>>>>>> [1]
>>>>>>> https://lore.kernel.org/all/20230507172041.2320279-1-dmitry.baryshkov@linaro.org/
>>>>>>>
>>>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>>>> ---
>>>>>>>    .../arm/boot/dts/qcom-apq8074-dragonboard.dts | 28
>>>>>>> +++++++++++++++++++
>>>>>>>    1 file changed, 28 insertions(+)
>>>>>>>
>>>>>>> diff --git a/arch/arm/boot/dts/qcom-apq8074-dragonboard.dts
>>>>>>> b/arch/arm/boot/dts/qcom-apq8074-dragonboard.dts
>>>>>>> index 6b047c679370..c893afc00eb4 100644
>>>>>>> --- a/arch/arm/boot/dts/qcom-apq8074-dragonboard.dts
>>>>>>> +++ b/arch/arm/boot/dts/qcom-apq8074-dragonboard.dts
>>>>>>> @@ -4,6 +4,8 @@
>>>>>>>    #include "qcom-pm8841.dtsi"
>>>>>>>    #include "qcom-pm8941.dtsi"
>>>>>>>    +/delete-node/ &mpss_region;
>>>>>>> +
>>>>>>>    / {
>>>>>>>        model = "Qualcomm APQ8074 Dragonboard";
>>>>>>>        compatible = "qcom,apq8074-dragonboard", "qcom,apq8074";
>>>>>>> @@ -17,6 +19,13 @@ aliases {
>>>>>>>        chosen {
>>>>>>>            stdout-path = "serial0:115200n8";
>>>>>>>        };
>>>>>>> +
>>>>>>> +    reserved-memory {
>>>>>>> +        mpss_region: mpss@ac00000 {
>>>>>>> +            reg = <0x0ac00000 0x2500000>;
>>>>>>> +            no-map;
>>>>>>> +        };
>>>>>>> +    };
>>>>>>>    };
>>>>>>>      &blsp1_uart2 {
>>>>>>> @@ -39,6 +48,25 @@ eeprom: eeprom@52 {
>>>>>>>        };
>>>>>>>    };
>>>>>>>    +&remoteproc_adsp {
>>>>>>> +    cx-supply = <&pm8841_s2>;
>>>>>>> +
>>>>>>> +    firmware-name = "qcom/apq8074/adsp.mbn";
>>>>>>> +
>>>>>>> +    status = "okay";
>>>>>>> +};
>>>>>>> +
>>>>>>> +&remoteproc_mss {
>>>>>>> +    cx-supply = <&pm8841_s2>;
>>>>>>> +    mss-supply = <&pm8841_s3>;
>>>>>>> +    mx-supply = <&pm8841_s1>;
>>>>>>> +    pll-supply = <&pm8941_l12>;
>>>>>> High time to move this to rpmpd!
>>>>>> I won't object to adding this though, as it obviously works
>>>>>> and is already used on other boards..
>>>>>
>>>>> I think the problem is that they are not level-voted on this
>>>>> platform, so they are regulators, not PDs.
>>>> They're corner-voted.
>>>
>>> Hmm. Indeed. In msm8974-regulators I see both voltage and corner entries
>>> for these regulators.
>>
>> Checked. Only CX and GFX (yes, MX not included) are enabled as corners
>> in vendor dtsi. So this probably doesn't gain us a lot.
> 
> I did a check. Implementing CX as a powerdomain here makes things
> worse for now. We should first teach mss/pas/etc drivers to properly
> handle the case when there is a single power-domain, which should be
> unbound after staring the DSP Consider e.g. ADSP and CDSP on sm8150 or
> CDSP on sm8250. But that's definitely a topic for a different patch
> series. After that we can consider landing msm8974_genpd.
Ack, thanks for looking into it.

Konrad
> 
>>
>>>
>>>>
>>>> Konrad
>>>>>
>>>>>>
>>>>>>> +
>>>>>>> +    firmware-name = "qcom/apq8074/mba.mbn", "qcom/apq8074/modem.mbn";
>>>>>> Could you please keep it one entry per line?
>>>>>
>>>>> Sure.
>>>>>
>>>>>>
>>>>>> Otherwise,
>>>>>>
>>>>>> Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>>>>>
>>>>>> Konrad
>>>>>>> +
>>>>>>> +    status = "okay";
>>>>>>> +};
>>>>>>> +
>>>>>>>    &rpm_requests {
>>>>>>>        regulators-0 {
>>>>>>>            compatible = "qcom,rpm-pm8841-regulators";
>>>>>
>>>
>>
>> --
>> With best wishes
>> Dmitry
>>
> 
>