Re: [PATCH V6 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 3 potential issue(s) to consider:
- [High] arm64: dts: qcom: monaco-evk-sd-card: invalid root syntax drops regulators
- [Medium] arm64: dts: qcom: contradictory no-1-8-v property in monaco-evk-sd-card.dtso
- [Low] arm64: dts: qcom: monaco-evk: invalid sdhc1_opp_table overrides in SD overlay
--

commit 6648a626b1d62a283e700d9a648c1b8158385e14
Author: Monish Chunara <monish.chunara@oss.qualcomm.com>

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

This commit removes the eMMC-specific sdhc_1 configuration from the base
monaco-evk.dts and introduces the monaco-evk-sd-card.dtso overlay to
configure sdhc_1 for SD card storage.

> 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
> index 0000000000000..bc4ea12587a2e
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/monaco-evk-sd-card.dtso
> @@ -0,0 +1,72 @@
> +// SPDX-License-Identifier: BSD-3-Clause
> +/*
> + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
> + */
> +
> +/dts-v1/;
> +/plugin/;
> +
> +#include <dt-bindings/gpio/gpio.h>
> +
> +/ {
> +        vmmc_sdc: regulator-mmc-sdc {
> +                compatible = "regulator-fixed";
> +
> +                regulator-name = "vmmc_sdc";
> +                regulator-min-microvolt = <2950000>;
> +                regulator-max-microvolt = <2950000>;
> +        };
> +
> +        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>;
> +        };
> +};

[Severity: High]
Should this root node block use &{/} instead of / (like
monaco-evk-ifp-mezzanine.dtso does)?

In a /plugin/; overlay, dtc places child nodes defined under / { ... };
directly at the root of the .dtbo blob rather than wrapping them inside a
fragment@N node with an __overlay__ subnode.

When fdt_overlay_apply() merges monaco-evk-sd-card.dtbo into
monaco-evk.dtb, overlay_merge() in scripts/dtc/libfdt/fdt_overlay.c skips
any top-level subnode that lacks an __overlay__ subnode:

	fdt_for_each_subnode(fragment, fdto, 0) {
		int overlay;
		int target;

		overlay = fdt_subnode_offset(fdto, fragment, "__overlay__");
		if (overlay == -FDT_ERR_NOTFOUND)
			continue;

Similarly, init_overlay_changeset() in drivers/of/overlay.c ignores
top-level nodes without an __overlay__ child when applying overlays at
runtime.

As a result, both regulator-mmc-sdc (vmmc_sdc) and regulator-sdc
(vreg_sdc) are silently dropped from the merged device tree, while
vmmc-supply and vqmmc-supply in &sdhc_1 are rewritten to dangling phandles
that cause regulator lookup to fail in
sdhci_msm_probe() -> sdhci_msm_register_vreg() ->
mmc_regulator_get_supply() -> of_get_regulator().

> +
> +&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]
As reported by Sashiko on v3 [1], is no-1-8-v intended here?

The overlay defines a 1.8V state on the vreg_sdc GPIO regulator for
vqmmc-supply:

	states = <1800000 1>, <2950000 0>;

and also adds 100 MHz and 202 MHz OPPs to &sdhc1_opp_table for UHS-I
modes.

However, setting no-1-8-v on &sdhc_1 causes
sdhci_msm_probe() -> sdhci_get_of_property() -> sdhci_get_property() to
set SDHCI_QUIRK2_NO_1_8_V, which in sdhci_setup_host() clears
SDHCI_SUPPORT_SDR104, SDHCI_SUPPORT_SDR50, SDHCI_SUPPORT_DDR50, and
MMC_CAP_UHS. That prevents 1.8V signaling switches in mmc_sd_init_card(),
caps the SD card at 50 MHz High-Speed mode, and leaves the 1.8V regulator
state and higher-frequency OPPs unused.

> +
> +	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>;
> +	};
> +
> +	opp-202000000 {
> +		opp-hz = /bits/ 64 <202000000>;
> +		required-opps = <&rpmhpd_opp_svs_l1>;
> +	};
> +};

[Severity: Low]
As reported by Sashiko on v4 [2], why does this board overlay modify
&sdhc1_opp_table?

In arch/arm64/boot/dts/qcom/monaco.dtsi, sdhc1_opp_table already defines:

	sdhc1_opp_table: opp-table {
		compatible = "operating-points-v2";

		opp-50000000 {
			opp-hz = /bits/ 64 <50000000>;
			required-opps = <&rpmhpd_opp_low_svs>;
		};

		opp-100000000 {
			opp-hz = /bits/ 64 <100000000>;
			required-opps = <&rpmhpd_opp_svs>;
		};

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

		opp-384000000 {
			opp-hz = /bits/ 64 <384000000>;
			required-opps = <&rpmhpd_opp_nom>;
		};
	};

And in drivers/clk/qcom/gcc-qcs8300.c, ftbl_gcc_sdcc1_apps_clk_src (which
uses clk_rcg2_floor_ops) only provides 100 MHz, 192 MHz, and 384 MHz at
the top end, with no 202 MHz entry:

	static const struct freq_tbl ftbl_gcc_sdcc1_apps_clk_src[] = {
		...
		F(50000000, P_GCC_GPLL0_OUT_EVEN, 6, 0, 0),
		F(100000000, P_GCC_GPLL0_OUT_EVEN, 3, 0, 0),
		F(192000000, P_GCC_GPLL9_OUT_MAIN, 4, 0, 0),
		F(384000000, P_GCC_GPLL9_OUT_MAIN, 2, 0, 0),
		{ }
	};

Because device tree overlays merge child nodes without removing existing
ones, &sdhc1_opp_table in monaco-evk-sd-card.dtso lowers the SoC-defined
voltage corner for opp-100000000 from rpmhpd_opp_svs to
rpmhpd_opp_low_svs, leaves opp-200000000 and opp-384000000 in place, and
adds opp-202000000.

When dev_pm_opp_set_rate() calls clk_round_rate() and looks up the ceiling
OPP via _find_freq_ceil(), any 200-208 MHz request rounds down to
192000000 Hz and always matches the existing opp-200000000 entry before
opp-202000000, while sdhci_msm_probe() calling
dev_pm_opp_set_rate(&pdev->dev, INT_MAX) still selects opp-384000000.

[1] https://lore.kernel.org/r/20260701083148.0F8341F00A3A@smtp.kernel.org
[2] https://lore.kernel.org/r/20260716144010.1DC4F1F000E9@smtp.kernel.org

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