Re: [PATCH v5 5/7] arm64: dts: qcom: Add initial support for MSM8952

[Muzaffer Kadir]

Hi Konrad,

Thanks a lot for review,

On Mon, 26 Oct 2026 16:01:49 +0100, Konrad Dybcio wrote:
> On 10/11/26 6:51 PM, Muzaffer Kadir via B4 Relay wrote:
>> From: Muzaffer Kadir <muzafferkadir@xxxxxxxxxxxxxx>
>>
>> Add initial support for MSM8952 SoC.
>>
>> AI help is used for calculating qfprom nodes.
>>
>> Downstream reference for port is [1] which is very lightly modified
>> version of [2].
> 
> Generally this looks good, couple more comments
> 
> [...]
> 
>> +	soc: soc@0 {
>> +		compatible = "simple-bus";
>> +		ranges = <0 0 0 0xffffffff>;
> 
> this is equal to `ranges;`

Changing this to empty ranges would make ranges property
to expect parent and child size-cells and address-cells to
be equal but root size-cells and address-cells are 2 and soc
size-cells and address-cells are 1.

The Devicetree Specification describes an empty ranges as:
"If the property is defined with an <empty> value, it
specifies that the parent and child address space is identical,
and no address translation is required."
(Devicetree Specification v0.4, section 2.3.8)

So this change creates these warnings:
/soc@0:ranges: empty "ranges" property but its #address-cells (1) differs from / (2)
/soc@0:ranges: empty "ranges" property but its #size-cells (1) differs from / (2)

I will keep this unchanged.

> 
> [...]
> 
> 
>> +		sdhc_1: mmc@7824900 {
>> +			compatible = "qcom,msm8952-sdhci", "qcom,sdhci-msm-v4";
>> +			reg = <0x07824900 0x500>, <0x07824000 0x800>, <0x7824e00 0x200>;
>> +			reg-names = "hc", "core", "cqhci";
> 
> Please break up these into one-per-line (also for properties below)
> the last reg entry needs a leading zero
> 
> (in general one-per-line is cool)
> 
> [...]
> 
> 
>> +		usb: usb@78db000 {
>> +			compatible = "qcom,ci-hdrc";
>> +			reg = <0x078db000 0x200>,
>> +			      <0x078db200 0x200>;
>> +			interrupts-extended = <&mpm 58 IRQ_TYPE_LEVEL_HIGH>,
>> +					      <&mpm 49 IRQ_TYPE_LEVEL_HIGH>;
>> +			clocks = <&gcc GCC_USB_HS_AHB_CLK>,
>> +				 <&gcc GCC_USB_HS_SYSTEM_CLK>;
>> +			clock-names = "iface",
>> +				      "core";
>> +			assigned-clocks = <&gcc GCC_USB_HS_SYSTEM_CLK>;
>> +			assigned-clock-rates = <80000000>;
>> +			resets = <&gcc GCC_USB_HS_BCR>;
>> +			reset-names = "core";
>> +			hnp-disable;
>> +			srp-disable;
>> +			adp-disable;
>> +			ahb-burst-config = <0>;
>> +			dr_mode = "otg";
> 
> dr_mode = otg is default - you can drop it
> 
> [...]
> 
>> +			pll_opp_table_c1: opp-table {
>> +				compatible = "operating-points-v2";
>> +
>> +				opp-499200000 {
>> +					opp-hz = /bits/ 64 <499200000>;
>> +				};
>> +
>> +				opp-806400000 {
>> +					opp-hz = /bits/ 64 <806400000>;
>> +				};
>> +
>> +				opp-960000000 {
>> +					opp-hz = /bits/ 64 <960000000>;
>> +				};
>> +
>> +				opp-1094400000 {
>> +					opp-hz = /bits/ 64 <1094400000>;
>> +				};
>> +
>> +				opp-1344000000 {
>> +					opp-hz = /bits/ 64 <1344000000>;
>> +				};
>> +
>> +				opp-1440000000 {
>> +					opp-hz = /bits/ 64 <1440000000>;
>> +				};
>> +
>> +				opp-1516800000 {
>> +					opp-hz = /bits/ 64 <1516800000>;
>> +				};
>> +
>> +				opp-1651200000 {
>> +					opp-hz = /bits/ 64 <1651200000>;
>> +				};
> 
> Drop the last entry for the same reasons you explained
> 
> [...]
> 
> 
>> +		a53pll_cci: clock@b1d0000 {
>> +			compatible = "qcom,msm8939-a53pll";
> 
> This needs a 8952-specific compatible (bindings-only fallback
> is fine)
> 
>> +			reg = <0x0b1d0000 0x40>;
>> +			clocks = <&xo_board>;
>> +			clock-names = "xo";
>> +			#clock-cells = <0>;
>> +		};
>> +
>> +		apcs2: mailbox@b1d1000 {
>> +			compatible = "qcom,msm8939-apcs-kpss-global", "syscon";
> 
> Likewise
>> +			reg = <0x0b1d1000 0x1000>;
>> +			clocks = <&a53pll_cci>,
>> +				 <&gcc GPLL0_VOTE>,
>> +				 <&rpmcc RPM_SMD_XO_CLK_SRC>;
>> +			clock-names = "pll",
>> +				      "aux",
>> +				      "ref";
>> +			#clock-cells = <0>;
>> +			#mbox-cells = <1>;
>> +		};
>> +
>> +		adsp: remoteproc@c200000 {
>> +			compatible = "qcom,msm8996-adsp-pil";
> 
> And likewise
> 
> also IMEM, DSI, RPMPD, RPMCC
> 
> [...]
> 
>> +	thermal-zones {
>> +		aoss0-thermal {
>> +			polling-delay-passive = <250>;
> 
> TSENS sensors are interrupt-driven so you don't need polling
> 
> AOSS is surely the wrong name - that hw didn't come until years
> later than 8952 was released. I can't find a mapping of the
> sensors for this SoC right now, so either drop these or give them
> some generic names

Understood, I will change these to generic names
(excluding known ones from downstream)

> 
> [...]
> 
>> +	timer {
>> +		compatible = "arm,armv8-timer";
>> +		interrupts = <GIC_PPI 2 (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_LOW)>,
>> +			     <GIC_PPI 3 (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_LOW)>,
>> +			     <GIC_PPI 4 (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_LOW)>,
>> +			     <GIC_PPI 1 (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_LOW)>;
>> +		clock-frequency = <19200000>;
> 
> Drop this property, it's only needed for very broken FWs

For other parts I will change them as requested.

Best Regards,
Muzaffer Kadir