Re: [PATCH v9 8/8] riscv: dts: spacemit: k1-musepi-pro: add SD card support with UHS modes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Incorrect PMIC parent supply bindings (`vin-supply` instead of `vinX-supply`).
- [High] Using `regulator-always-on` for SD card supplies breaks UHS soft reset/recovery.
- [Medium] Excessively broad voltage constraints for SD card regulators.
- [Low] Dead Code: Unreferenced `reg_dc_in` fixed regulator.
- [Low] Ambiguous Devicetree Pattern: `cd-inverted` combined with `GPIO_ACTIVE_HIGH`.
--

commit ba761ed57c645870aad2395144a4d7a3530a081d
Author: Trevor Gamblin <tgamblin@baylibre.com>

riscv: dts: spacemit: k1-musepi-pro: add SD card support with UHS modes

Update the Muse Pi Pro devicetree with SD card support to match what
was done for the OrangePi RV2.

Link: https://lore.kernel.org/linux-riscv/20260316-orangepi-sd-card-uhs-v3-0-aefd3b7832df@gmail.com/T/#

> diff --git a/arch/riscv/boot/dts/spacemit/k1-musepi-pro.dts b/arch/riscv/boot/dts/spacemit/k1-musepi-pro.dts
> index 29e333b670cf0..774a4640f0656 100644
> --- a/arch/riscv/boot/dts/spacemit/k1-musepi-pro.dts
> +++ b/arch/riscv/boot/dts/spacemit/k1-musepi-pro.dts
> @@ -18,6 +18,24 @@ aliases {
>  		ethernet0 = &eth0;
>  		serial0 = &uart0;
>  	};
> +	reg_dc_in: dc-in-5v {
> +		compatible = "regulator-fixed";
> +		regulator-name = "dc_in_5v";
> +		regulator-min-microvolt = <5000000>;
> +		regulator-max-microvolt = <5000000>;
> +		regulator-boot-on;
> +		regulator-always-on;
> +	};

Is this fixed regulator used anywhere? It does not appear to be referenced
as a supply by any other node in this device tree.

[ ... ]

> +&i2c8 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&i2c8_cfg>;
> +	status = "okay";
> +
> +	pmic@41 {
> +		compatible = "spacemit,p1";
> +		reg = <0x41>;
> +		interrupts = <64>;
> +		vin-supply = <&reg_vcc_4v>;

Will the regulator framework fail to resolve the parent supply here? The
spacemit-p1 regulator driver and bindings seem to require specific parent
supply names for each regulator group (e.g., vin4-supply for buck4 and
aldoin-supply for aldo1) rather than a generic vin-supply property.

> +
> +		regulators {
> +			buck4: buck4 {
> +				regulator-min-microvolt = <500000>;

Does this constraint allow the voltage to drop below the safe operating
area for the connected hardware? SD cards typically require exactly 3.3V
for VMMC, so allowing 0.5V could potentially cause brownouts.

> +				regulator-max-microvolt = <3300000>;
> +				regulator-ramp-delay = <5000>;
> +				regulator-always-on;

Will this prevent the MMC subsystem from properly resetting the SD card
during a soft reboot or initialization error recovery?

For UHS-I modes, the SD card switches to 1.8V signaling. To reset the card
back to 3.3V, the MMC core calls mmc_power_off() to power cycle the
vmmc-supply. If the regulator is marked as always-on, the disable request
is ignored, which can leave the card stuck in 1.8V mode.

> +			};
> +
> +			aldo1: aldo1 {
> +				regulator-min-microvolt = <500000>;

Similarly, does this minimum voltage constraint need to be tighter for
the VQMMC supply, which usually operates between 1.8V and 3.3V?

> +				regulator-max-microvolt = <3400000>;
> +				regulator-boot-on;
> +				regulator-always-on;

Does this always-on constraint also prevent the I/O power supply from
being properly managed during card power cycles?

> +			};
> +		};
> +	};
> +};
> +
> +&sdhci0 {
> +	pinctrl-names = "default", "uhs";
> +	pinctrl-0 = <&mmc1_cfg>;
> +	pinctrl-1 = <&mmc1_uhs_cfg>;
> +	bus-width = <4>;
> +	cd-gpios = <&gpio K1_GPIO(80) GPIO_ACTIVE_HIGH>;
> +	cd-inverted;

This isn't a bug, but could the hardware polarity be encoded directly into
the GPIO specifier using GPIO_ACTIVE_LOW instead? The modern device tree
practice is to drop the cd-inverted property to avoid the ambiguous double
inversion.

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260511-orangepi-sd-card-uhs-v9-0-ae48c0b2b2cf@gmail.com?part=8