Re: [PATCH V6 3/3] arm64: dts: qcom: monaco-evk: Add SDHCI support for eMMC 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: root node syntax in monaco-evk-sd-card.dtso drops regulators
- [Medium] arm64: dts: qcom: missing supports-cqe on sdhc_1 in monaco-ac-evk.dts
- [Medium] arm64: dts: qcom: invalid and duplicate OPPs in monaco-evk-sd-card.dtso
--

commit 55ce3aa0fb14b29f7953daf1505d7c18f0d799da
Author: Monish Chunara <monish.chunara@oss.qualcomm.com>

arm64: dts: qcom: monaco-evk: Add SDHCI support for eMMC via overlay

This series moves the eMMC supports-cqe property from the Monaco SoC device
tree to the board level and switches Monaco EVK SDHCI storage selection between
SD card and eMMC to device tree overlays.

> 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>;
> +        };
> +};
> +
> +&sdhc_1 {
> +	vmmc-supply = <&vmmc_sdc>;
> +	vqmmc-supply = <&vreg_sdc>;

[Severity: High]
Should the root node in monaco-evk-sd-card.dtso use &{/} { ... }; instead of
/ { ... };?

In a /plugin/; device tree overlay, defining nodes under / { ... }; places
regulator-mmc-sdc and regulator-sdc directly under the root of the compiled
.dtbo rather than wrapping them in a fragment node with an __overlay__ child.

When fdt_overlay_apply() -> overlay_merge() in scripts/dtc/libfdt/fdt_overlay.c
(or init_overlay_changeset() in drivers/of/overlay.c) merges the overlay:

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

both regulator nodes are skipped and omitted from the merged DTB, while
vmmc-supply and vqmmc-supply in sdhc_1 are left pointing at phandles that do
not exist in the merged tree. When sdhci_msm_probe() -> sdhci_setup_host() ->
mmc_regulator_get_supply() runs, resolving both supplies fails with -ENODEV.

[ ... ]

> +&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: Medium]
Is this override of sdhc1_opp_table intended for sdhc_1 on Monaco?

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 supports
50 MHz, 100 MHz, 192 MHz, and 384 MHz, with no 202 MHz entry.

Because overlay_apply_node() merges new subnodes without removing existing
ones, applying monaco-evk-sd-card.dtso leaves opp-200000000 and opp-384000000
in sdhc1_opp_table alongside the new opp-202000000 node, while also lowering
the 100 MHz required-opps vote from rpmhpd_opp_svs to rpmhpd_opp_low_svs.

[ ... ]

> diff --git a/arch/arm64/boot/dts/qcom/monaco.dtsi b/arch/arm64/boot/dts/qcom/monaco.dtsi
> index 35fbee63d4c65..591ad233f90eb 100644
> --- a/arch/arm64/boot/dts/qcom/monaco.dtsi
> +++ b/arch/arm64/boot/dts/qcom/monaco.dtsi
> @@ -4843,7 +4843,6 @@ &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>,
>  			qcom,dll-config = <0x000f64ee>;
>  			qcom,ddr-config = <0x80040868>;
>  			bus-width = <8>;
> -			supports-cqe;
>  			dma-coherent;

[Severity: Medium]
Should arch/arm64/boot/dts/qcom/monaco-ac-evk.dts also have supports-cqe added
to &sdhc_1?

monaco-ac-evk.dts includes monaco.dtsi and enables sdhc_1 for non-removable
eMMC:

&sdhc_1 {
	vmmc-supply = <&vreg_l8a>;
	vqmmc-supply = <&vreg_s2s>;

	no-sd;
	no-sdio;
	non-removable;

	status = "okay";
};

In drivers/mmc/host/sdhci-msm.c, sdhci_msm_probe() checks for supports-cqe:

	if (of_property_read_bool(node, "supports-cqe"))
		ret = sdhci_msm_cqe_add_host(host, pdev);
	else
		ret = sdhci_add_host(host);

Removing supports-cqe from monaco.dtsi without adding it to monaco-ac-evk.dts
causes sdhci_msm_probe() to skip sdhci_msm_cqe_add_host() (and
sdhci_msm_ice_init() called from within it), disabling CQE and ICE on
Monaco-AC EVK.

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