Re: [PATCH v2 6/6] arm64: dts: rockchip: fix modem power supply on Photonicat

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] arm64: dts: rockchip: vbat min/max microvolt mismatch fails probe
- [Medium] arm64: dts: rockchip: vcc3v4_rf vin-supply change has no runtime effect
- [Medium] arm64: dts: rockchip: vcc5v0_usb30_otg0 leaves regulator_5v_out unused
--

commit e4c4248e8e73884f2860befcec1dafef98cd711c
Author: Coia Prant <coiaprant@gmail.com>

arm64: dts: rockchip: fix modem power supply on Photonicat

This commit changes the vin-supply of the vcc3v4_rf regulator from vccin_5v
to vcc_syson in the Ariaboard Photonicat RK3568 device tree to match the
hardware schematic and keep the M.2 5G modem powered on battery.

Link: https://github.com/photonicat/photonicat-hardware/blob/main/Photonicat_rk3568_sch_20221116.pdf (Page 18)

> diff --git a/arch/arm64/boot/dts/rockchip/rk3568-photonicat.dts b/arch/arm64/boot/dts/rockchip/rk3568-photonicat.dts
> index 4d362f665031c..6a51f97395741 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3568-photonicat.dts
> +++ b/arch/arm64/boot/dts/rockchip/rk3568-photonicat.dts
> @@ -169,7 +169,7 @@ vcc3v4_rf: regulator-vcc3v4-rf {
>  		regulator-name = "vcc3v4_rf";
>  		regulator-min-microvolt = <3400000>;
>  		regulator-max-microvolt = <3400000>;
> -		vin-supply = <&vccin_5v>;
> +		vin-supply = <&vcc_syson>;
>  	};

[Severity: Medium]
Does reparenting vcc3v4_rf from vccin_5v to vcc_syson actually change the
hardware power state when running on battery, as described in the commit
message?

In arch/arm64/boot/dts/rockchip/rk3568-photonicat.dts, vccin_5v, vcc_sysin,
and vcc_syson are all regulator-fixed nodes marked regulator-always-on and
regulator-boot-on with no GPIO, clock, or power-domain control, and vcc_syson
chains directly back to vccin_5v:

	vccin_5v: regulator-vccin-5v {
		compatible = "regulator-fixed";
		regulator-name = "vccin_5v";
		regulator-always-on;
		regulator-boot-on;
		regulator-min-microvolt = <5000000>;
		regulator-max-microvolt = <5000000>;
	};

	vcc_sysin: regulator-vcc-sysin {
		compatible = "regulator-fixed";
		regulator-name = "vcc_sysin";
		regulator-always-on;
		regulator-boot-on;
		regulator-min-microvolt = <5000000>;
		regulator-max-microvolt = <5000000>;
		vin-supply = <&vccin_5v>;
	};

	vcc_syson: regulator-vcc-syson {
		compatible = "regulator-fixed";
		regulator-name = "vcc_syson";
		regulator-always-on;
		regulator-boot-on;
		regulator-min-microvolt = <5000000>;
		regulator-max-microvolt = <5000000>;
		vin-supply = <&vcc_sysin>;
	};

In reg_fixed_voltage_probe() in drivers/regulator/fixed.c, a fixed regulator
without a GPIO, clock, or PM domain uses empty fixed_voltage_ops and performs
no hardware switching.

Should the commit message clarify that this only updates the DT supply
hierarchy to match the schematic, or is there a controllable switch or boost
regulator needed to power the modem on battery?

>  
>  	vcc5v0_usb30_otg0: regulator-vcc5v0-usb30-otg0 {

[Severity: High]
In commit cf017fdefcec ("arm64: dts: rockchip: fix USB OTG power on Photonicat
in battery mode") earlier in this series, vcc5v0_usb30_otg0 was updated to
depend on regulator_5v_boost, which in turn depends on the newly added vbat
fixed regulator:

	regulator_5v_boost: regulator-5v-boost {
		...
		vin-supply = <&vbat>;
	};
	...
	vbat: regulator-vbat {
		compatible = "regulator-fixed";
		regulator-name = "vbat";
		regulator-min-microvolt = <3400000>;
		regulator-max-microvolt = <4200000>;
		regulator-always-on;
		regulator-boot-on;
	};

However, reg_fixed_voltage_probe() -> of_get_fixed_voltage_config() in
drivers/regulator/fixed.c requires regulator-min-microvolt and
regulator-max-microvolt to be equal for a regulator-fixed node:

	if (init_data->constraints.min_uV == init_data->constraints.max_uV) {
		config->microvolts = init_data->constraints.min_uV;
	} else {
		dev_err(dev,
			 "Fixed regulator specified with variable voltages\n");
		return ERR_PTR(-EINVAL);
	}

Won't vbat fail to probe with -EINVAL, causing regulator_5v_boost,
regulator_5v_out, vcc5v0_usb30_otg0, and the consumer USB2 OTG PHY nodes
(usb2phy0_otg and usb2phy1_otg) to permanently defer with -EPROBE_DEFER?

[Severity: Medium]
Also in arch/arm64/boot/dts/rockchip/rk3568-photonicat.dts, the comment above
regulator_5v_out states that 5V_OUT supplies VCC5V0_USB30_OTG0:

	/*
	 * ...
	 * 5V_OUT supplies:
	 * - VCC5V0_USB30_OTG0 (USB VBUS via U12 load switch)
	 * - VCC5V_HDMI (HDMI 5V via D5000)
	 * - J53 expansion header (Pin 1)
	 */
	regulator_5v_out: regulator-5v-out {
		compatible = "regulator-fixed";
		regulator-name = "5v_out";
		regulator-min-microvolt = <5000000>;
		regulator-max-microvolt = <5000000>;
		vin-supply = <&regulator_5v_boost>;
	};

However, vcc5v0_usb30_otg0 sets vin-supply to regulator_5v_boost directly,
leaving regulator_5v_out completely unreferenced in the device tree:

	vcc5v0_usb30_otg0: regulator-vcc5v0-usb30-otg0 {
		...
		vin-supply = <&regulator_5v_boost>;
	};

Should vcc5v0_usb30_otg0 use vin-supply = <&regulator_5v_out> instead?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102171838.2958552-1-coiaprant@gmail.com?part=6