Re: [PATCH 8/9] arm64: dts: qcom: Add DTS for MSM8976 and MSM8956 SoCs

[AngeloGioacchino Del Regno]

Il 08/11/22 05:55, Bjorn Andersson ha scritto:
> On Fri, Nov 04, 2022 at 06:21:21PM +0100, AngeloGioacchino Del Regno wrote:
> > From: AngeloGioacchino Del Regno <angelogioacchino.delregno@xxxxxxxxxxxxxx>
> >
> > This commit adds device trees for MSM8956 and MSM8976 SoCs.
> > They are *almost* identical, with minor differences, such as
> > MSM8956 having two A72 cores less.
> >
> > However, there is a bug in Sony Loire bootloader that requires presence
> > of all 8 cores in the cpu{} node, so these will not be deleted.
> >
> > Co-developed-by: Konrad Dybcio <konrad.dybcio@xxxxxxxxxxxxxx>
> > Signed-off-by: Konrad Dybcio <konrad.dybcio@xxxxxxxxxxxxxx>
> > Co-developed-by: Marijn Suijten <marijn.suijten@xxxxxxxxxxxxxx>
> > Signed-off-by: Marijn Suijten <marijn.suijten@xxxxxxxxxxxxxx>
> > Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@xxxxxxxxxxxxxx>
> > Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@xxxxxxxxxxxxx>

Hello,

Thanks to everyone for the feedback!
I'll send a new version this Friday, according to the received reviews.

In the meanwhile, a few answers will follow, check below.

> > ---
> >   arch/arm64/boot/dts/qcom/msm8956.dtsi |   18 +
> >   arch/arm64/boot/dts/qcom/msm8976.dtsi | 1208 +++++++++++++++++++++++++
> >   2 files changed, 1226 insertions(+)
> >   create mode 100644 arch/arm64/boot/dts/qcom/msm8956.dtsi
> >   create mode 100644 arch/arm64/boot/dts/qcom/msm8976.dtsi

..snip..

> > +		cpu-map {
> > +			cluster0 {
> > +				core0 {
> > +					cpu = <&CPU0>;
> > +				};
> > +
> > +				core1 {
> > +					cpu = <&CPU1>;
> > +				};
> > +
> > +				core2 {
> > +					cpu = <&CPU2>;
> > +				};
> > +
> > +				core3 {
> > +					cpu = <&CPU3>;
> > +				};
> > +			};
> > +
> > +			cluster1 {
>
> Are you sure that the two clusters should be expressed separately in the
> cpu-map?

This SoC has two clusters with split L2 cache, can shutdown one cluster CPU, or
the L2 cache for one cluster, or one entire cluster, hence can also manage idle
states on a per-cluster basis.

Also, as per bindings/cpu/cpu-topology.txt - I am here describing the hierarchy
of CPUs in MSM8976, containing one "little" cluster, composed of four slower CPU
cores and its own L2 cache slice, and one "big" cluster, composed of four (8976)
or two (8956) faster CPU cores and its own L2 cache slice.

That said, I am sure that the two clusters shall be expressed separately.

Am I underestimating and/or ignoring anything?

> > +				core0 {
> > +					cpu = <&CPU4>;
> > +				};
> > +

..snip..

> > +
> > +	reserved-memory {
> > +		#address-cells = <2>;
> > +		#size-cells = <2>;
> > +		ranges;
> > +
> > +		cont_splash_mem: memory@83000000 {
>
> memory is "reserved", please use specific node names for these regions.

Agreed.

> > +			reg = <0x0 0x83000000 0x0 0x2800000>;
> > +		};
> [..]
> > +		apcs: syscon@b011000 {
> > +			compatible = "syscon";
>
> Why not use qcom,msm8976-apcs-kpss-global here?

There's no reason not to use the suggested compatible. I'm sorry for the miss.

> > +			reg = <0x0b011000 0x1000>;
> > +		};
> [..]
> > +
> > +		imem: imem@8600000 {
> > +			compatible = "simple-mfd";
>
> sram/qcom,imem.yaml please.

Will do on v2.

Regards,
Angelo