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

["Luca Weiss" <luca.weiss@fairphone.com>]

On Tue Oct 31, 2023 at 7:44 AM CET, Mukesh Ojha wrote:
>
>
> 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 ?

Hi Mukesh,

I'll do that in v2, thanks!

Regards
Luca

>
> -Mukesh
> > 
> > -Doug