Re: [RFC PATCH 2/2] arm64: dts: qcom: qcm6490: Add qcm6490 idp and rb3 board

[Mukesh Ojha]

On 11/6/2023 5:24 PM, Dmitry Baryshkov wrote:
> On Mon, 6 Nov 2023 at 13:41, Mukesh Ojha <quic_mojha@xxxxxxxxxxx> 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@xxxxxxxxxxx> 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/@Krzysztof,
> >
> > 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@xxxxxxxxxxx/

> Are ADSP and CDSP physically present on sc7280?

Yes, they are present but not used.

> 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 ?

[3]
https://lore.kernel.org/linux-arm-msm/20231003175456.14774-3-quic_kbajaj@xxxxxxxxxxx/

-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@xxxxxxxxxxx/
> >
> > [2]
> > commit 90c856602e0346ce9ff234062e86a198d71fa723
> > Author: Douglas Anderson <dianders@xxxxxxxxxxxx>
> > 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@xxxxxxxxxxxx>
> >       Reviewed-by: Stephen Boyd <swboyd@xxxxxxxxxxxx>
> >       Reviewed-by: Matthias Kaehlcke <mka@xxxxxxxxxxxx>
> >       Signed-off-by: Bjorn Andersson <bjorn.andersson@xxxxxxxxxx>
> >       Link:
> > https://lore.kernel.org/r/20220125144316.v2.3.Iac012fa8d727be46448d47027a1813ea716423ce@changeid
> >
> >
> > > Best regards,
> > > Krzysztof