Message-ID: <CAA8EJpqJ4+A0a0VyQjTkkcF7R+1WRtgu0JgzrUPZjGzW8GrQ9g@mail.gmail.com>
Date:   Tue, 14 Nov 2023 14:53:19 +0200
From:   Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To:     Mukesh Ojha <quic_mojha@...cinc.com>
Cc:     Luca Weiss <luca.weiss@...rphone.com>,
        Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
        Komal Bajaj <quic_kbajaj@...cinc.com>,
        Andy Gross <agross@...nel.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Konrad Dybcio <konrad.dybcio@...aro.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        linux-arm-msm@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, quic_nainmeht@...cinc.com
Subject: Re: [RFC PATCH 2/2] arm64: dts: qcom: qcm6490: Add qcm6490 idp and
 rb3 board

On Tue, 14 Nov 2023 at 14:49, Mukesh Ojha <quic_mojha@...cinc.com> wrote:
>
>
>
> On 11/13/2023 9:21 PM, Luca Weiss wrote:
> > On Tue Nov 7, 2023 at 9:10 AM CET, Mukesh Ojha wrote:
> >>
> >>
> >> On 11/7/2023 4:02 AM, Dmitry Baryshkov wrote:
> >>> On Mon, 6 Nov 2023 at 16:46, Mukesh Ojha <quic_mojha@...cinc.com> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 11/6/2023 5:24 PM, Dmitry Baryshkov wrote:
> >>>>> On Mon, 6 Nov 2023 at 13:41, Mukesh Ojha <quic_mojha@...cinc.com> wrote:
> >>>>>>
> >>>>>>
> >>>>>> On 11/5/2023 6:38 PM, Krzysztof Kozlowski wrote:
> >>>>>>> On 03/11/2023 23:22, Dmitry Baryshkov wrote:
> >>>>>>>> On Fri, 3 Nov 2023 at 20:49, Komal Bajaj <quic_kbajaj@...cinc.com> wrote:
> >>>>>>>>>
> >>>>>>>>> Add qcm6490 devicetree file for QCM6490 IDP and QCM6490 RB3
> >>>>>>>>> platform. QCM6490 is derived from SC7280 meant for various
> >>>>>>>>> form factor including IoT.
> >>>>>>>>>
> >>>>>>>>> Supported features are, as of now:
> >>>>>>>>> * Debug UART
> >>>>>>>>> * eMMC (only in IDP)
> >>>>>>>>> * USB
> >>>>>>>>>
> >>>>>>>
> >>>>>>> ...
> >>>>>>>
> >>>>>>>>> +
> >>>>>>>>> diff --git a/arch/arm64/boot/dts/qcom/qcm6490-iot-common.dtsi b/arch/arm64/boot/dts/qcom/qcm6490-iot-common.dtsi
> >>>>>>>>> new file mode 100644
> >>>>>>>>> index 000000000000..01adc97789d0
> >>>>>>>>> --- /dev/null
> >>>>>>>>> +++ b/arch/arm64/boot/dts/qcom/qcm6490-iot-common.dtsi
> >>>>>>>>
> >>>>>>>> I have mixed feelings towards this file. Usually we add such 'common'
> >>>>>>>> files only for the phone platforms where most of the devices are
> >>>>>>>> common.
> >>>>>>>> Do you expect that IDP and RB3 will have a lot of common code other
> >>>>>>>> than these regulator settings?
> >>>>>>>
> >>>>>>> I agree here. What exactly is common in the real hardware between IDP
> >>>>>>> and RB3? Commit msg does not explain it, so I do not see enough
> >>>>>>> justification for common file. Just because some DTS looks similar for
> >>>>>>> different hardware does not mean you should creat common file.
> >>>>>>
> >>>>>> @Dmitry/@...ysztof,
> >>>>>>
> >>>>>> Thank you for reviewing the RFC, we wanted to continue the
> >>>>>> suggestion/discussion given on [1] , where we discussed that this
> >>>>>> qcm6490 is going to be targeted for IOT segment and will have different
> >>>>>> memory map and it is going to use some of co-processors like adsp/cdsp
> >>>>>> which chrome does not use.
> >>>>>>
> >>>>>> So to your question what is common between RB3 and IDP, mostly they will
> >>>>>> share common memory map(similar to [2]) and regulator settings and both
> >>>>>> will use adsp/cdsp etc., we will be posting the memory map changes as
> >>>>>> well in coming weeks once this RFC is acked.
> >>>>>
> >>>>> Is the memory map going to be the same as the one used on Fairphone5?
> >>>>
> >>>> No, Fairphone5 looks to be using chrome memory map and i suggested
> >>>> here to move them into sc7280.dtsi
> >>>>
> >>>> https://lore.kernel.org/lkml/d5d53346-ca3b-986a-e104-d87c37115b62@quicinc.com/
> >>>>
> >>>>>
> >>>>> Are ADSP and CDSP physically present on sc7280?
> >>>>
> >>>> Yes, they are present but not used.
> >>>
> >>> So ADSP and CDSP should go into sc7280.dtsi. They will anyway have
> >>> status = "disabled";
> >>>
> >>>>
> >>>>>
> >>>>> I think that your goal should be to:
> >>>>> - populate missing device in sc7280.dtsi
> >>>>> - maybe add qcm6490.dtsi which defines SoC-level common data (e.g. memory map)
> >>>>> - push the rest to board files.
> >>>>
> >>>> Agree to all of the point.
> >>>> We started with the same thought at[3] but it got lost in discussion
> >>>> due to its differentiation with mobile counter part(fairphone) which
> >>>> follow chrome memory map and hence we came up with qcm6490-iot-common.
> >>>> Do you think, qcm6490-iot.dtsi should be good ?
> >>>
> >>> No. DT describes hardware, and -iot is not a hardware abstraction / unification.
> >>> If you consider your memory map to be generic for the qcm6490 (and FP5
> >>> being the only exception), add it to the qcm6490.dtsi (and let FP5
> >>> override it, like some of the phones do). If it can not be considered
> >>> generic for the SoC, then you have no other choice than to replicate
> >>> it to all board files.
> >>
> >
> > Hi Mukesh,
> >
> >> Thanks for the suggestion.
> >> Let me add @Luca here for information, if he want to share
> >> anything about qcm6490 fp5 memory map.
> >
> > Not sure I have much to share, just probably that on FP5 the memory
> > setup and all the basics just come from a standard QCM6490.LA.3.0
> > release.
> > I don't see any hint that our ODM changed something in the memory map
> > for the device either.
> >
> > I'm also aware that other phones also use QCM6490 SoC, so I'm still
> > wondering where the distinction between "FP5/ChromeOS memory map" vs
> > this new QCM6490 memory map is.
> > There's also e.g. this phone using QCM6490, I've not looked into this at
> > all, but I'm guessing that phone uses the same memory map as FP5.
> > https://www.crosscall.com/en_NL/core-z5-COZ5.MASTER.html
>
> Was looking for your view on the things about qcm6490.dtsi one common
> dtsi file for all qcm6490.dtsi suggested in the mail, but looks like FP5
> is following the memory map based out of sc7280, in that case we have to
> replicate the new memory map for all our IOT boards(idp/rb3) based on
> this SoC.

You can have IoT memory map in the qcm6490.dtsi and have the
board-specific memory map in the qcm6490-fp5.dtsi, if that makes life
easier. I think the phone DT already provides the memory map, so you
just have to add statements to remove conflicting data entries.

>
> -Mukesh
> >
> > Regards
> > Luca
> >
> >>
> >> -Mukesh
> >>>
> >>>>
> >>>> [3]
> >>>> https://lore.kernel.org/linux-arm-msm/20231003175456.14774-3-quic_kbajaj@quicinc.com/
> >>>>
> >>>> -Mukesh
> >>>>>
> >>>>> I don't think that putting regulators to the common file is a good
> >>>>> idea. Platforms will further change and limit voltage limits and
> >>>>> modes, so they usually go to the board file.
> >>>>>
> >>>>>>
> >>>>>>
> >>>>>> Thanks,
> >>>>>> Mukesh
> >>>>>>
> >>>>>> [1]
> >>>>>> https://lore.kernel.org/linux-arm-msm/d97ebf74-ad03-86d6-b826-b57be209b9e2@quicinc.com/
> >>>>>>
> >>>>>> [2]
> >>>>>> commit 90c856602e0346ce9ff234062e86a198d71fa723
> >>>>>> Author: Douglas Anderson <dianders@...omium.org>
> >>>>>> Date:   Tue Jan 25 14:44:20 2022 -0800
> >>>>>>
> >>>>>>         arm64: dts: qcom: sc7280: Factor out Chrome common fragment
> >>>>>>
> >>>>>>         This factors out a device tree fragment from some sc7280 device
> >>>>>>         trees. It represents the device tree bits that should be included for
> >>>>>>         "Chrome" based sc7280 boards. On these boards the bootloader (Coreboot
> >>>>>>         + Depthcharge) configures things slightly different than the
> >>>>>>         bootloader that Qualcomm provides. The modem firmware on these boards
> >>>>>>         also works differently than on other Qulacomm products and thus the
> >>>>>>         reserved memory map needs to be adjusted.
> >>>>>>
> >>>>>>         NOTES:
> >>>>>>         - This is _not_ quite a no-op change. The "herobrine" and "idp"
> >>>>>>           fragments here were different and it looks like someone simply
> >>>>>>           forgot to update the herobrine version. This updates a few numbers
> >>>>>>           to match IDP. This will also cause the `pmk8350_pon` to be disabled
> >>>>>>           on idp/crd, which I belive is a correct change.
> >>>>>>         - At the moment this assumes LTE skus. Once it's clearer how WiFi SKUs
> >>>>>>           will work (how much of the memory map they can reclaim) we may add
> >>>>>>           an extra fragment that will rejigger one way or the other.
> >>>>>>
> >>>>>>         Signed-off-by: Douglas Anderson <dianders@...omium.org>
> >>>>>>         Reviewed-by: Stephen Boyd <swboyd@...omium.org>
> >>>>>>         Reviewed-by: Matthias Kaehlcke <mka@...omium.org>
> >>>>>>         Signed-off-by: Bjorn Andersson <bjorn.andersson@...aro.org>
> >>>>>>         Link:
> >>>>>> https://lore.kernel.org/r/20220125144316.v2.3.Iac012fa8d727be46448d47027a1813ea716423ce@changeid
> >>>>>>
> >>>>>>
> >>>>>>>
> >>>>>>> Best regards,
> >>>>>>> Krzysztof
> >>>>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>
> >>>
> >>>
> >



-- 
With best wishes
Dmitry

Powered by blists - more mailing lists