Re: [PATCH v3 1/3] riscv: dts: spacemit: k3-deepcomputing-fml13v05: add P1 PMIC regulator tree

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] riscv: dts: spacemit: missing regulator-always-on on dldo4 in fml13v05
- [Low] riscv: dts: spacemit: unused reg_aux_vcc3v3 regulator node in fml13v05
--

commit 4ce9192349ccaa9cafe27a0ab82d02be78ea635e
Author: Sandie Cao <sandie.cao@deepcomputing.io>

riscv: dts: spacemit: k3-deepcomputing-fml13v05: add P1 PMIC regulator tree

This commit adds the P1 PMIC node and regulator topology to the
DeepComputing FML13V05 board device tree.

> diff --git a/arch/riscv/boot/dts/spacemit/k3-deepcomputing-fml13v05.dts b/arch/riscv/boot/dts/spacemit/k3-deepcomputing-fml13v05.dts
> index b5d5112de3581..804abc85602ab 100644
> --- a/arch/riscv/boot/dts/spacemit/k3-deepcomputing-fml13v05.dts
> +++ b/arch/riscv/boot/dts/spacemit/k3-deepcomputing-fml13v05.dts
> @@ -22,6 +22,161 @@ memory@100000000 {
>  		device_type = "memory";
>  		reg = <0x1 0x00000000 0x4 0x00000000>;
>  	};
> +
> +	reg_aux_vcc3v3: regulator-aux-vcc3v3 {
> +		compatible = "regulator-fixed";
> +		regulator-name = "AUX_VCC3V3";
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +		regulator-always-on;
> +	};
> +
> +	reg_aux_vcc5v: regulator-aux-vcc5v {
> +		compatible = "regulator-fixed";
> +		regulator-name = "AUX_VCC5V";
> +		regulator-min-microvolt = <5000000>;
> +		regulator-max-microvolt = <5000000>;
> +		regulator-always-on;
> +	};
> +};
> +
> +&i2c8 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&i2c8_cfg>;
> +	status = "okay";
> +
> +	p1@41 {
> +		compatible = "spacemit,p1";
> +		reg = <0x41>;
> +		interrupts = <64 IRQ_TYPE_LEVEL_HIGH>;
> +		vin1-supply = <&reg_aux_vcc5v>;
> +		vin2-supply = <&reg_aux_vcc5v>;
> +		vin3-supply = <&reg_aux_vcc5v>;
> +		vin4-supply = <&reg_aux_vcc5v>;
> +		vin5-supply = <&reg_aux_vcc5v>;
> +		vin6-supply = <&reg_aux_vcc5v>;
> +		aldoin-supply = <&reg_aux_vcc5v>;
> +		dldoin1-supply = <&buck4>;
> +		dldoin2-supply = <&buck4>;

[Severity: Low]
Is reg_aux_vcc3v3 needed in this patch?

In p1@41, vin1-supply through vin6-supply and aldoin-supply all reference
reg_aux_vcc5v, while dldoin1-supply and dldoin2-supply reference buck4.
The reg_aux_vcc3v3 fixed regulator node has no GPIO control and no
consumers in k3-deepcomputing-fml13v05.dts, including after the subsequent
USB and SD card patches in the series (where hub@1 uses compatible
usb5e3,608, which forbids vdd-supply in
Documentation/devicetree/bindings/usb/genesys,gl850g.yaml, and sdhci0 only
references aldo1).

[ ... ]

> +			dldo3: dldo3 {
> +				regulator-min-microvolt = <800000>;
> +				regulator-max-microvolt = <800000>;
> +				regulator-always-on;
> +				regulator-boot-on;
> +			};
> +
> +			dldo4: dldo4 {
> +				regulator-min-microvolt = <1800000>;
> +				regulator-max-microvolt = <1800000>;
> +				regulator-boot-on;
> +			};

[Severity: High]
Should dldo4 also have regulator-always-on?

When p1_regulator_probe() registers dldo4 with regulator-boot-on and
without regulator-always-on, of_get_regulation_constraints() sets
constraints->boot_on = true, constraints->always_on = false, and sets
REGULATOR_CHANGE_STATUS in constraints->valid_ops_mask. During
set_machine_constraints(), _regulator_do_enable(rdev) enables dldo4 in
hardware, while rdev->use_count stays at 0 because no node in k3.dtsi or
k3-deepcomputing-fml13v05.dts references &dldo4 as a supply.

Thirty seconds after late_initcall_sync(regulator_init_complete),
regulator_init_complete_work_function() calls regulator_late_cleanup() in
drivers/regulator/core.c:

	if (c && c->always_on)
		return 0;

	if (!regulator_ops_is_valid(rdev, REGULATOR_CHANGE_STATUS))
		return 0;

	regulator_lock(rdev);

	if (rdev->use_count)
		goto unlock;

	/* If reading the status failed, assume that it's off. */
	if (_regulator_is_enabled(rdev) <= 0)
		goto unlock;

	if (have_full_constraints()) {
		/* We log since this may kill the system if it goes
		 * wrong.
		 */
		rdev_info(rdev, "disabling\n");
		ret = _regulator_do_disable(rdev);

This disables the 1.8V dldo4 rail at boot. All other fixed boot-on LDO
rails in p1@41 (aldo1, aldo2, aldo4, dldo1..dldo3, dldo5..dldo7), as well
as dldo4 in arch/riscv/boot/dts/spacemit/k3-com260.dtsi (fixed in commit
f314c98807c1 ("riscv: dts: spacemit: k3-com260: keep dldo4 enabled") to
prevent a CPU hard-lockup), specify regulator-always-on.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102060012.100623-1-sandie.cao@deepcomputing.io?part=1