Re: [PATCH V4 2/3] arm64: dts: qcom: monaco-evk: Enable SDHCI for SD Card via overlay

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Removal of eMMC configuration from base DT without a replacement overlay breaks eMMC users.
- [High] Dummy `vmmc_sdc` regulator fails to control physical hardware power.
- [Medium] The `no-1-8-v` property contradicts the 1.8V hardware regulator and renders added high-speed OPPs unreachable.
- [Low] Board-level overlay improperly overrides SoC-level OPP voltage corners.
- [Low] Adding a new 202MHz OPP without deleting the existing 200MHz OPP creates redundant entries.
--

--- Patch [2]: [PATCH V4 2/3] arm64: dts: qcom: monaco-evk: Enable SDHCI for SD Card via overlay ---
commit 999af06615677bf3f3fbc5349952dd6ad0631932
Author: Monish Chunara <monish.chunara@oss.qualcomm.com>

arm64: dts: qcom: monaco-evk: Enable SDHCI for SD Card via overlay

The monaco EVK board supports either eMMC or SD-card, but only one can
be active at a time. Make the base monaco-evk.dtsi storage-agnostic by
removing eMMC-specific configuration from it. Introduce an SD card overlay
that enables the SD Host Controller Interface (SDHCI) for SD-based storage.

> diff --git a/arch/arm64/boot/dts/qcom/Makefile b/arch/arm64/boot/dts/qcom/Makefile
> --- a/arch/arm64/boot/dts/qcom/Makefile
> +++ b/arch/arm64/boot/dts/qcom/Makefile
[ ... ]
> +monaco-evk-sd-card-dtbs := monaco-evk.dtb monaco-evk-sd-card.dtbo
> +dtb-$(CONFIG_ARCH_QCOM) += monaco-evk-sd-card.dtb
> +
>  dtb-$(CONFIG_ARCH_QCOM)	+= msm8216-samsung-fortuna3g.dtb

[ ... ]

> diff --git a/arch/arm64/boot/dts/qcom/monaco-evk.dts b/arch/arm64/boot/dts/qcom/monaco-evk.dts
> --- a/arch/arm64/boot/dts/qcom/monaco-evk.dts
> +++ b/arch/arm64/boot/dts/qcom/monaco-evk.dts
[ ... ]
> -&sdhc_1 {
> -	vmmc-supply = <&vreg_l8a>;
> -	vqmmc-supply = <&vreg_s2s>;
> -
> -	no-sd;
> -	no-sdio;
> -	non-removable;
> -
> -	status = "okay";
> -};

[Severity: High]
The commit message mentions moving media selection to overlays, but only the
SD card overlay is provided. Does this break default eMMC functionality for
users of the base board?

Removing the eMMC configuration from the base device tree without providing a
replacement eMMC overlay leaves eMMC users with no alternative, preventing
the board from booting or functioning correctly if it depends on eMMC.

> diff --git a/arch/arm64/boot/dts/qcom/monaco-evk-sd-card.dtso b/arch/arm64/boot/dts/qcom/monaco-evk-sd-card.dtso
> new file mode 100644
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/monaco-evk-sd-card.dtso
> @@ -0,0 +1,72 @@
[ ... ]
> +/ {
> +        vmmc_sdc: regulator-mmc-sdc {
> +                compatible = "regulator-fixed";
> +
> +                regulator-name = "vmmc_sdc";
> +                regulator-min-microvolt = <2950000>;
> +                regulator-max-microvolt = <2950000>;
> +        };

[Severity: High]
Will this dummy vmmc_sdc regulator fail to control the physical hardware power
since it lacks a gpio or vin-supply property?

Without physical power control, the SD card might fail to power up or
experience intermittent power loss if the physical shared rail drops during
runtime PM.

> +
> +        vreg_sdc: regulator-sdc {
> +		compatible = "regulator-gpio";
> +
> +		regulator-name = "vreg_sdc";
> +		regulator-type = "voltage";
> +		regulator-min-microvolt = <1800000>;
> +		regulator-max-microvolt = <2950000>;
> +
> +		gpios = <&expander1 7 GPIO_ACTIVE_HIGH>;
> +		states = <1800000 1>, <2950000 0>;
> +
> +		startup-delay-us = <100>;
> +        };
> +};
> +
> +&sdhc_1 {
> +	vmmc-supply = <&vmmc_sdc>;
> +	vqmmc-supply = <&vreg_sdc>;
> +
> +	pinctrl-0 = <&sdc1_state_on>, <&sd_cd>;
> +	pinctrl-1 = <&sdc1_state_off>, <&sd_cd>;
> +	pinctrl-names = "default", "sleep";
> +
> +	cap-sd-highspeed;
> +	no-1-8-v;

[Severity: Medium]
Could the no-1-8-v property be incorrect here?

It contradicts the 1.8V hardware regulator (vreg_sdc) configured above and
prevents the MMC core from ever utilizing the high-speed UHS-I OPPs defined
in this overlay.

> +
> +	bus-width = <4>;
> +	cd-gpios = <&tlmm 11 GPIO_ACTIVE_LOW>;
> +	no-mmc;
> +	no-sdio;
> +
> +	status = "okay";
> +};
> +
> +&sdhc1_opp_table {
> +	opp-100000000 {
> +		opp-hz = /bits/ 64 <100000000>;
> +		required-opps = <&rpmhpd_opp_low_svs>;
> +	};

[Severity: Low]
Does this improperly override the SoC-level OPP voltage corners from a
board-level overlay?

Operating Performance Points and their required voltage corners are typically
fundamental properties of the SoC silicon, so modifying them here might be
semantically incorrect and cause instability.

> +
> +	opp-202000000 {
> +		opp-hz = /bits/ 64 <202000000>;
> +		required-opps = <&rpmhpd_opp_svs_l1>;
> +	};
> +};

[Severity: Low]
When adding the 202MHz OPP, does the existing 200MHz OPP need to be explicitly
removed with a /delete-node/ directive?

Leaving it intact could create redundant, overlapping entries for the same
voltage corner when the overlay is parsed.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260716142445.2767955-1-monish.chunara@oss.qualcomm.com?part=2