Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:3 as permitted sender) client-ip=2620:137:e000::3:3;
Message-ID: <d5d53346-ca3b-986a-e104-d87c37115b62@quicinc.com>
Date:   Tue, 31 Oct 2023 12:14:07 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.13.0
Subject: Re: [PATCH 7/9] arm64: dts: qcom: sc7280: Add CDSP node
Content-Language: en-US
To:     Doug Anderson <dianders@chromium.org>,
        Luca Weiss <luca.weiss@fairphone.com>
CC:     Andy Gross <agross@kernel.org>,
        Bjorn Andersson <andersson@kernel.org>,
        Konrad Dybcio <konrad.dybcio@linaro.org>,
        Mathieu Poirier <mathieu.poirier@linaro.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Conor Dooley <conor+dt@kernel.org>,
        Manivannan Sadhasivam <mani@kernel.org>,
        <cros-qcom-dts-watchers@chromium.org>,
        <~postmarketos/upstreaming@lists.sr.ht>,
        <phone-devel@vger.kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>,
        Rob Herring <robh@kernel.org>,
        =?UTF-8?Q?Matti_Lehtim=c3=a4ki?= <matti.lehtimaki@gmail.com>,
        <linux-arm-msm@vger.kernel.org>,
        <linux-remoteproc@vger.kernel.org>, <devicetree@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>
References: <20231027-sc7280-remoteprocs-v1-0-05ce95d9315a@fairphone.com>
 <20231027-sc7280-remoteprocs-v1-7-05ce95d9315a@fairphone.com>
 <7934a36a-9438-719a-2ed0-4a78757b044b@quicinc.com>
 <CWLNP6QNUXN1.SNVACF2IEGI8@fairphone.com>
 <CAD=FV=U6mi0h0MBFMC+ba4oq-te6_+WR6fj1XjAq7tmUu64bUA@mail.gmail.com>
 <CWLUQWPZNAS5.3F4Y5W13OD08M@fairphone.com>
 <CAD=FV=XbwbjFgMjq-y_L-9EO+xfxwGo6RYV8Wh6P5oBR=oDf_g@mail.gmail.com>
From:   Mukesh Ojha <quic_mojha@quicinc.com>
In-Reply-To: <CAD=FV=XbwbjFgMjq-y_L-9EO+xfxwGo6RYV8Wh6P5oBR=oDf_g@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 10/30/2023 8:33 PM, Doug Anderson wrote:
> Hi,
> 
> On Mon, Oct 30, 2023 at 7:43 AM Luca Weiss <luca.weiss@fairphone.com> wrote:
>>
>> On Mon Oct 30, 2023 at 3:11 PM CET, Doug Anderson wrote:
>>> Hi,
>>>
>>> On Mon, Oct 30, 2023 at 2:12 AM Luca Weiss <luca.weiss@fairphone.com> wrote:
>>>>
>>>> On Mon Oct 30, 2023 at 10:04 AM CET, Mukesh Ojha wrote:
>>>>>
>>>>>
>>>>> On 10/27/2023 7:50 PM, Luca Weiss wrote:
>>>>>> Add the node for the ADSP found on the SC7280 SoC, using standard
>>>>>> Qualcomm firmware.
>>>>>>
>>>>>> The memory region for sc7280-chrome-common.dtsi is taken from msm-5.4
>>>>>> yupik.dtsi since the other areas also seem to match that file there,
>>>>>> though I cannot be sure there.
>>>>>>
>>>>>> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
>>>>>> ---
>>>>>>    arch/arm64/boot/dts/qcom/sc7280-chrome-common.dtsi |   5 +
>>>>>>    arch/arm64/boot/dts/qcom/sc7280.dtsi               | 138 +++++++++++++++++++++
>>>>>>    2 files changed, 143 insertions(+)
>>>>>>
>>>>>> diff --git a/arch/arm64/boot/dts/qcom/sc7280-chrome-common.dtsi b/arch/arm64/boot/dts/qcom/sc7280-chrome-common.dtsi
>>>>>> index eb55616e0892..6e5a9d4c1fda 100644
>>>>>> --- a/arch/arm64/boot/dts/qcom/sc7280-chrome-common.dtsi
>>>>>> +++ b/arch/arm64/boot/dts/qcom/sc7280-chrome-common.dtsi
>>>>>> @@ -29,6 +29,11 @@ adsp_mem: memory@86700000 {
>>>>>>                      no-map;
>>>>>>              };
>>>>>>
>>>>>> +           cdsp_mem: memory@88f00000 {
>>>>>> +                   reg = <0x0 0x88f00000 0x0 0x1e00000>;
>>>>>> +                   no-map;
>>>>>> +           };
>>>>>> +
>>>>>
>>>>> Just a question, why to do it here, if chrome does not use this ?
>>>>
>>>> Other memory regions in sc7280.dtsi also get referenced but not actually
>>>> defined in that file, like mpss_mem and wpss_mem. Alternatively we can
>>>> also try and solve this differently, but then we should probably also
>>>> adjust mpss and wpss to be consistent.
>>>>
>>>> Apart from either declaring cdsp_mem in sc7280.dtsi or
>>>> "/delete-property/ memory-region;" for CDSP I don't really have better
>>>> ideas though.
>>>>
>>>> I also imagine these ChromeOS devices will want to enable cdsp at some
>>>> point but I don't know any plans there.
>>>
>>> Given that "remoteproc_cdsp" has status "disabled" in the dtsi, it
>>> feels like the dtsi shouldn't be reserving memory. I guess maybe
>>> memory regions can't be status "disabled"?
>>
>> Hi Doug,
>>
>> That's how it works in really any qcom dtsi though. I think in most/all
>> cases normally the reserved-memory is already declared in the SoC dtsi
>> file and also used with the memory-region property.
>>
>> I wouldn't be against adjusting sc7280.dtsi to match the way it's done
>> in the other dtsi files though, so to have all the required labels
>> already defined in the dtsi so it doesn't rely on these labels being
>> defined in the device dts.
>>
>> In other words, currently if you include sc7280.dtsi and try to build,
>> you first have to define the labels mpss_mem and wpss_mem (after this
>> patch series also cdsp_mem and adsp_mem) for it to build.
>>
>> I'm quite neutral either way, let me know :)
> 
> I haven't done a ton of thinking about this, so if I'm spouting
> gibberish then feel free to ignore me. :-P It just feels weird that
> when all the "dtsi" files are combined and you look at what you end up
> on a sc7280 Chrome board that you'll be reserving 32MB of memory for a
> device that's set (in the same device tree) to be "disabled", right?
> ...the 32MB is completely wasted, I think. If we wanted to enable the
> CDSP we'd have to modify the device tree anyway, so it seems like that
> same modification would set the CDSP to "okay" and also reserve the
> memory...
> 
> In that vein, it seems like maybe you could move the "cdsp_mem" to the
> SoC .dsti file with a status of "disabled". . I guess we don't do that
> elsewhere, but maybe we should be? As far as I can tell without
> testing it (just looking at fdt_scan_reserved_mem()) this should
> work...

What do you think about moving present reserve memory block from
sc7280-chrome-common to sc7280.dtsi and delete the stuff which
chrome does not need it sc7280-chrome-common ?

-Mukesh
> 
> -Doug