linux-kernel - Re: [PATCH 7/9] arm64: dts: qcom: sc7280: Add CDSP node

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <d5d53346-ca3b-986a-e104-d87c37115b62@quicinc.com>
Date:   Tue, 31 Oct 2023 12:14:07 +0530
From:   Mukesh Ojha <quic_mojha@...cinc.com>
To:     Doug Anderson <dianders@...omium.org>,
        Luca Weiss <luca.weiss@...rphone.com>
CC:     Andy Gross <agross@...nel.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Konrad Dybcio <konrad.dybcio@...aro.org>,
        Mathieu Poirier <mathieu.poirier@...aro.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Manivannan Sadhasivam <mani@...nel.org>,
        <cros-qcom-dts-watchers@...omium.org>,
        <~postmarketos/upstreaming@...ts.sr.ht>,
        <phone-devel@...r.kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
        Rob Herring <robh@...nel.org>,
        Matti Lehtimäki <matti.lehtimaki@...il.com>,
        <linux-arm-msm@...r.kernel.org>,
        <linux-remoteproc@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 7/9] arm64: dts: qcom: sc7280: Add CDSP node



On 10/30/2023 8:33 PM, Doug Anderson wrote:
> Hi,
> 
> On Mon, Oct 30, 2023 at 7:43 AM Luca Weiss <luca.weiss@...rphone.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@...rphone.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@...rphone.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@...00000 {
>>>>>>                      no-map;
>>>>>>              };
>>>>>>
>>>>>> +           cdsp_mem: memory@...00000 {
>>>>>> +                   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