Re: [PATCH v2 5/6] arm64: dts: rockchip: fix USB OTG power on Photonicat in battery mode

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] arm64: dts: rockchip: unequal min/max microvolt on fixed regulator vbat
- [Medium] arm64: dts: rockchip: vcc5v0_usb30_otg0 bypasses unused regulator_5v_out
--

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

arm64: dts: rockchip: fix USB OTG power on Photonicat in battery mode

This commit adds fixed regulator nodes for the VBAT supply, the 5V boost
converter, and the 5V_OUT rail on the Ariaboard Photonicat RK3568, and
updates the USB OTG VBUS regulator input supply so USB host power works on
battery.

Link: https://github.com/immortalwrt/immortalwrt/issues/1534
Link: https://github.com/photonicat/photonicat-hardware/blob/main/Photonicat_rk3568_sch_20221116.pdf (Page 15,18)

> diff --git a/arch/arm64/boot/dts/rockchip/rk3568-photonicat.dts b/arch/arm64/boot/dts/rockchip/rk3568-photonicat.dts
> index e69f5e28e9e64..4d362f665031c 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3568-photonicat.dts
> +++ b/arch/arm64/boot/dts/rockchip/rk3568-photonicat.dts
> @@ -51,6 +51,47 @@ hdmi_con_in: endpoint {
>  		};
>  	};
>  
> +	regulator_5v_boost: regulator-5v-boost {
> +		compatible = "regulator-fixed";
> +		enable-active-high;
> +		gpio = <&gpio4 RK_PD2 GPIO_ACTIVE_HIGH>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&boost_en>;
> +		regulator-name = "5v_boost";
> +		regulator-min-microvolt = <5000000>;
> +		regulator-max-microvolt = <5000000>;
> +		vin-supply = <&vbat>;
> +	};
> +
> +	/*
> +	 * 5V_OUT is the OR-ed combination of the VBAT boost (via D5002)
> +	 * and VCCIN_5V (via D16).
> +	 * It has no enable pin of its own.
> +	 * Model it as a fixed regulator depending on the boost converter,
> +	 * so that VBAT is boosted when running on battery.
> +	 *
> +	 * 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>;
> +	};
> +
> +	vbat: regulator-vbat {
> +		compatible = "regulator-fixed";
> +		regulator-name = "vbat";
> +		regulator-min-microvolt = <3400000>;
> +		regulator-max-microvolt = <4200000>;
> +		regulator-always-on;
> +		regulator-boot-on;
> +	};

[Severity: High]
Will the vbat regulator fail to probe here because regulator-min-microvolt
and regulator-max-microvolt do not match?

In drivers/regulator/fixed.c, reg_fixed_voltage_probe() calls
of_get_fixed_voltage_config(), which requires min_uV and max_uV to be equal
for a fixed regulator:

	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);
	}

When vbat fails with -EINVAL, regulator_resolve_supply() ->
of_regulator_dev_lookup() sees the regulator-vbat node present in the Device
Tree without a registered regulator device and permanently returns
-EPROBE_DEFER when resolving vin-supply = <&vbat> on regulator_5v_boost,
which cascades to vcc5v0_usb30_otg0 and the USB2 OTG PHYs (usb2phy0_otg and
usb2phy1_otg).

> +
>  	vcc_1v8: regulator-vcc-1v8 {
>  		compatible = "regulator-fixed";
>  		regulator-name = "vcc_1v8";
> @@ -140,7 +181,7 @@ vcc5v0_usb30_otg0: regulator-vcc5v0-usb30-otg0 {
>  		regulator-name = "vcc5v0_usb30_otg0";
>  		regulator-min-microvolt = <5000000>;
>  		regulator-max-microvolt = <5000000>;
> -		vin-supply = <&vccin_5v>;
> +		vin-supply = <&regulator_5v_boost>;
>  	};

[Severity: Medium]
Should vcc5v0_usb30_otg0 reference <&regulator_5v_out> for its vin-supply
instead of <&regulator_5v_boost>, or is regulator_5v_out unnecessary?

Both the commit message and the comment above regulator_5v_out state that
5V_OUT supplies vcc5v0_usb30_otg0, but vcc5v0_usb30_otg0 points directly to
regulator_5v_boost, leaving regulator_5v_out without any consumers in
rk3568-photonicat.dts. (The commit message also refers to the nodes as
v5v_boost and v5v_out rather than regulator_5v_boost and regulator_5v_out.)

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