Re: [PATCH 2/2] arm64: dts: qcom: Add Motorola Moto G10

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] arm64: dts: qcom: capri simple-framebuffer missing display clocks and GDSC
- [Medium] arm64: dts: qcom: sm4250-motorola-capri: wcn3950-bt never powers off in qca_power_off()
--

commit af8b8ef25e8fb618354806c36e99ddefd993ffb6
Author: Michael McAllister <me@michaelmcallister.org>

arm64: dts: qcom: Add Motorola Moto G10

This patch adds an initial device tree for the Motorola Moto G10 (capri)
smartphone, enabling storage, usb type-c, buttons, graphics, wireless,
bluetooth, and a simple-framebuffer display node.

> diff --git a/arch/arm64/boot/dts/qcom/sm4250-motorola-capri.dts b/arch/arm64/boot/dts/qcom/sm4250-motorola-capri.dts
> new file mode 100644
> index 0000000000000..b079f0cdc800b
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/sm4250-motorola-capri.dts
> @@ -0,0 +1,452 @@
[ ... ]
> +	chosen {
> +		#address-cells = <2>;
> +		#size-cells = <2>;
> +		ranges;
> +
> +		stdout-path = "display0";
> +
> +		framebuffer0: framebuffer@5c000000 {
> +			compatible = "simple-framebuffer";
> +			reg = <0 0x5c000000 0 (720 * 1600 * 4)>;
> +			display = <&mdss>;
> +			format = "a8r8g8b8";
> +			height = <1600>;
> +			stride = <(720 * 4)>;
> +			vddio-supply = <&vreg_l9a>;
> +			width = <720>;
> +		};
> +	};

[Severity: High]
Will the display stay powered and clocked after late_initcall cleanup when
booting with this simple-framebuffer node?

In arch/arm64/boot/dts/qcom/sm6115.dtsi, mdss is disabled by default while
both gcc and dispcc are enabled:

arch/arm64/boot/dts/qcom/sm6115.dtsi:
		dispcc: clock-controller@5f00000 {
			compatible = "qcom,sm6115-dispcc";
			...
		};

Because mdss is disabled and simple-framebuffer does not claim clocks or
power domains through the display = <&mdss> phandle, nothing claims
gcc_disp_hf_axi_clk in drivers/clk/qcom/gcc-sm6115.c or the display clocks
and mdss_gdsc power domain registered by disp_cc_sm6115_probe() in
drivers/clk/qcom/dispcc-sm6115.c:

drivers/clk/qcom/gcc-sm6115.c:
static struct clk_branch gcc_disp_hf_axi_clk = {
	.halt_reg = 0x17020,
	.halt_check = BRANCH_HALT,
	...
};

drivers/clk/qcom/dispcc-sm6115.c:
static struct gdsc mdss_gdsc = {
	.gdscr = 0x3000,
	.pd = {
		.name = "mdss_gdsc",
	},
	.pwrsts = PWRSTS_OFF_ON,
	.flags = HW_CTRL,
};

When clk_disable_unused() and pm_genpd_poweroff_unused() run during boot,
won't they disable gcc_disp_hf_axi_clk, the dispcc branch clocks, and
mdss_gdsc, shutting down the active display scanout and stdout-path console?

For comparison, arch/arm64/boot/dts/qcom/sm6115-xiaomi-lemon.dts keeps
gcc_disp_hf_axi_clk enabled in framebuffer0 and disables dispcc until the
panel driver is wired up:

arch/arm64/boot/dts/qcom/sm6115-xiaomi-lemon.dts:
		framebuffer0: framebuffer@5c000000 {
			...
			clocks = <&gcc GCC_DISP_HF_AXI_CLK>;
		};
...
&dispcc {
	/* Prevent simplefb from dying until we wire panel driver */
	status = "disabled";
};

Should framebuffer0 reference gcc_disp_hf_axi_clk (and either disable dispcc
or claim the dispcc clocks and mdss_gdsc) here as well?

[ ... ]
> +&uart3 {
> +	interrupts-extended = <&intc GIC_SPI 330 IRQ_TYPE_LEVEL_HIGH>,
> +			      <&tlmm 11 IRQ_TYPE_EDGE_FALLING>;
> +	pinctrl-0 = <&bt_uart_default>;
> +	pinctrl-1 = <&bt_uart_sleep>;
> +	pinctrl-names = "default", "sleep";
> +
> +	status = "okay";
> +
> +	bluetooth {
> +		compatible = "qcom,wcn3950-bt";
> +		max-speed = <3000000>;
> +		vddch0-supply = <&vreg_l23a>;
> +		vddio-supply = <&vreg_l9a>;
> +		vddrf-supply = <&vreg_l17a>;
> +		vddxo-supply = <&vreg_l16a>;
> +	};
> +};

[Severity: Medium]
Can the bluetooth controller power off cleanly on this board when using
qcom,wcn3950-bt with direct regulator supplies instead of a power sequencer?

In drivers/bluetooth/hci_qca.c, qca_serdev_probe() falls back to direct
regulator control via qca_init_regulators() when devm_pwrseq_get() does not
find a bluetooth power sequencer. While qca_power_on() and
qca_regulator_init() handle the wcn3950 soc_type, both switch statements in
qca_power_off() and the cleanup check in qca_serdev_remove() omit it:

drivers/bluetooth/hci_qca.c:qca_power_off() {
	switch (soc_type) {
	case QCA_WCN3988:
	case QCA_WCN3990:
	case QCA_WCN3991:
	case QCA_WCN3998:
		host_set_baudrate(hu, 2400);
		qca_send_power_pulse(hu, false);
		break;
	default:
		break;
	}

	if (power && power->pwrseq) {
		pwrseq_disable(power->pwrseq);
		set_bit(QCA_BT_OFF, &qca->flags);
		return;
	}

	switch (soc_type) {
	case QCA_WCN3988:
	case QCA_WCN3990:
	case QCA_WCN3991:
	case QCA_WCN3998:
		qca_regulator_disable(qcadev);
		break;
	...
}

drivers/bluetooth/hci_qca.c:qca_serdev_remove() {
	switch (qcadev->btsoc_type) {
	case QCA_WCN3988:
	case QCA_WCN3990:
	case QCA_WCN3991:
	case QCA_WCN3998:
	case QCA_WCN6750:
	case QCA_WCN6855:
	case QCA_WCN7850:
		if (power->vregs_on)
			qca_power_off(&qcadev->serdev_hu);
		break;
	default:
		break;
	}
}

Without a power-sequencer node (or wcn3950 handling in qca_power_off() and
qca_serdev_remove()), won't powering off bluetooth or unbinding the serdev
driver skip both qca_send_power_pulse(hu, false) and
qca_regulator_disable(qcadev), leaving vddch0, vddio, vddrf, and vddxo
permanently enabled?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1792703903.git.me@michaelmcallister.org?part=2