Re: [PATCH 2/2] arm64: dts: qcom: sm7325: Add Samsung Galaxy M52 5G

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] arm64: dts: qcom: sm7325-samsung-m52xq: missing status okay on qupv3_id_0
- [High] arm64: dts: qcom: sm7325-samsung-m52xq: overlapping adsp_mem reserved memory
- [High] arm64: dts: qcom: sm7325-samsung-m52xq: wrong WCN6750 bluetooth RFA supplies
- [High] arm64: dts: qcom: sm7325-samsung-m52xq: wrong IRQ trigger type on nfc@27
- [Low] arm64: dts: qcom: sm7325-samsung-m52xq: conflicting reg and memory-region on fb
--

commit 25a93cb2dad726fce008e0298f37e0192577485b
Author: Asdlerto Korry <korryasdlerto@gmail.com>

arm64: dts: qcom: sm7325: Add Samsung Galaxy M52 5G

This commit adds the initial device tree for the SM7325-based Samsung Galaxy
M52 5G (samsung,m52xq) smartphone.

> diff --git a/arch/arm64/boot/dts/qcom/sm7325-samsung-m52xq.dts b/arch/arm64/boot/dts/qcom/sm7325-samsung-m52xq.dts
> new file mode 100644
> index 0000000000000..5ba9623df6c40
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/sm7325-samsung-m52xq.dts
> @@ -0,0 +1,828 @@

[ ... ]

> +/* The following reserved memory regions have different addresses or sizes */
> +/delete-node/ &adsp_mem;
> +/delete-node/ &rmtfs_mem;
> +
> +/ {
> +	model = "Samsung Galaxy M52 5G";
> +	compatible = "samsung,m52xq", "qcom,sm7325";
> +	chassis-type = "handset";
> +
> +	aliases {
> +		serial1 = &uart7;
> +	};
> +
> +	chosen {
> +		#address-cells = <2>;
> +		#size-cells = <2>;
> +		ranges;
> +
> +		framebuffer0: framebuffer@e1000000 {
> +			compatible = "simple-framebuffer";
> +			reg = <0x0 0xe1000000 0x0 (1080 * 2400 * 4)>;
> +			width = <1080>;
> +			height = <2400>;
> +			stride = <(1080 * 4)>;
> +			format = "a8r8g8b8";
> +			memory-region = <&splash_mem>;

[Severity: Low]
Should one of reg or memory-region be dropped from framebuffer0?

Both simplefb_get_memory_of() in drivers/gpu/drm/sysfb/simpledrm.c and
simplefb_parse_dt() in drivers/video/fbdev/simplefb.c check whether reg is
present when memory-region is specified, emit a runtime warning preferring
memory-region over reg, and use the 0x2300000-byte splash_mem resource
instead of the (1080 * 2400 * 4)-byte reg property.

[ ... ]

> +	reserved-memory {
> +		cdsp_secure_heap_mem: cdsp-secure-heap@81800000 {
> +			reg = <0x0 0x81800000 0x0 0x1e00000>;
> +			no-map;
> +		};
> +
> +		adsp_mem: adsp@86700000 {
> +			reg = <0x0 0x86700000 0x0 0x4800000>;
> +			no-map;
> +		};
> +
> +		/* Mainline video_mem is downstream cvp_mem */
> +		real_video_mem: video@8ad00000 {
> +			reg = <0x0 0x8ad00000 0x0 0x500000>;
> +			no-map;
> +		};

[Severity: High]
Does adsp_mem overlap with cdsp_mem and real_video_mem here?

In arch/arm64/boot/dts/qcom/kodiak.dtsi (included via sm7325.dtsi), cdsp_mem
is defined at 0x88f00000..0x8ad00000:

arch/arm64/boot/dts/qcom/kodiak.dtsi:
		cdsp_mem: cdsp@88f00000 {
			reg = <0x0 0x88f00000 0x0 0x1e00000>;
			no-map;
		};

Because cdsp_mem is not removed with /delete-node/ &cdsp_mem;, defining
adsp_mem at 0x86700000 with size 0x4800000 (0x86700000..0x8af00000) covers
the entire cdsp_mem range and also overlaps the first 0x200000 bytes of
real_video_mem (0x8ad00000..0x8b200000), which triggers overlap errors in
fdt_scan_reserved_mem_late() -> __rmem_check_for_overlap() during boot.

Also, &venus below does not set memory-region = <&real_video_mem>, so
venus continues using video_mem (0x8b200000) from kodiak.dtsi while
real_video_mem remains unreferenced.

[ ... ]

> +&i2c9 {
> +	clock-frequency = <100000>;
> +
> +	status = "okay";
> +
> +	nfc@27 {
> +		compatible = "samsung,s3fwrn5-i2c";
> +		reg = <0x27>;
> +
> +		interrupts-extended = <&tlmm 41 IRQ_TYPE_LEVEL_HIGH>;

[Severity: High]
Should nfc@27 use IRQ_TYPE_EDGE_RISING instead of IRQ_TYPE_LEVEL_HIGH?

In drivers/nfc/s3fwrn5/i2c.c, s3fwrn5_i2c_probe() and s3fwrn5_i2c_set_mode()
set phy->irq_skip = true on startup and power/mode transitions, and request
a threaded IRQ with IRQF_ONESHOT. When the interrupt line asserts while
phy->irq_skip is true (or mode is S3FWRN5_MODE_COLD),
s3fwrn5_i2c_irq_thread_fn() returns IRQ_HANDLED without reading I2C to
clear the interrupt source:

drivers/nfc/s3fwrn5/i2c.c:s3fwrn5_i2c_irq_thread_fn() {
	...
	if (phy->irq_skip)
		goto out;
	...
out:
	mutex_unlock(&phy->common.mutex);

	return IRQ_HANDLED;
}

With IRQ_TYPE_LEVEL_HIGH, the line remains asserted and immediately
re-triggers in an endless IRQ loop as soon as the oneshot handler finishes.

[ ... ]

> +&qupv3_id_1 {
> +	status = "okay";
> +};

[Severity: High]
Is &qupv3_id_0 missing status = "okay" here?

In arch/arm64/boot/dts/qcom/kodiak.dtsi, uart7 (serial@99c000) is a child of
qupv3_id_0 (geniqup@9c0000), which has status = "disabled" by default.
Because geni_se_probe() in drivers/soc/qcom/qcom-geni-se.c only populates
child devices for enabled QUP wrapper nodes, leaving qupv3_id_0 disabled
prevents uart7 and its child bluetooth node from ever being populated or
probed.

[ ... ]

> +	bluetooth: bluetooth {
> +		compatible = "qcom,wcn6750-bt";
> +
> +		pinctrl-0 = <&bluetooth_enable_default>, <&sw_ctrl_default>;
> +		pinctrl-names = "default";
> +
> +		enable-gpios = <&tlmm 85 GPIO_ACTIVE_HIGH>;
> +		swctrl-gpios = <&tlmm 86 GPIO_ACTIVE_HIGH>;
> +
> +		vddio-supply = <&vreg_l19b_1p8>;
> +		vddaon-supply = <&vreg_s7b_0p972>;
> +		vddbtcxmx-supply = <&vreg_s7b_0p972>;
> +		vddrfacmn-supply = <&vreg_s7b_0p972>;
> +		vddrfa0p8-supply = <&vreg_s7b_0p972>;
> +		vddrfa1p7-supply = <&vreg_s1b_1p872>;
> +		vddrfa1p2-supply = <&vreg_s7b_0p972>;
> +		vddrfa2p2-supply = <&vreg_s8b_1p272>;
> +		vddasd-supply = <&vreg_l11c_2p8>;

[Severity: High]
Are vddrfa1p2-supply and vddrfa2p2-supply connected to the wrong regulators
here?

vddrfa1p2-supply is connected to vreg_s7b_0p972 (0.535V..1.12V) and
vddrfa2p2-supply is connected to vreg_s8b_1p272 (1.2V..1.5V), while
vreg_s1c_2p19 (2.19V..2.21V) defined in regulators-1 is left completely
unused. When qca_soc_data_wcn6750 probes in drivers/bluetooth/hci_qca.c, the
2.19V SMPS1C rail for the 2.2V RFA supply will never be enabled and will be
turned off as unused. Should vddrfa1p2-supply be <&vreg_s8b_1p272> and
vddrfa2p2-supply be <&vreg_s1c_2p19>?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104-m52xq-dts-v1-0-b198f02912ff@gmail.com?part=2