Re: [PATCH RESEND v1 2/2] arm64: dts: qcom: lemans-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] The eMMC device tree overlay creates dummy fixed regulators instead of referencing the existing PMIC RPMH regulators.
- [Low] Unused device tree binding includes in lemans-evk-emmc.dtso.
- [Low] Inconsistent indentation in clock and clock-names assignments.
--

commit 093f95690ab8c3394206ae0a37d52aa9b43fdb76
Author: Monish Chunara <monish.chunara@oss.qualcomm.com>

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

Enable the SDHCI controller for eMMC functionality on the lemans EVK
using a device tree overlay. Configure the corresponding address space
and resources for eMMC.

[ ... ]
> diff --git a/arch/arm64/boot/dts/qcom/lemans-evk-emmc.dtso b/arch/arm64/boot/dts/qcom/lemans-evk-emmc.dtso
> new file mode 100644
> index 0000000000000..52739d39a1ff8
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/lemans-evk-emmc.dtso
> @@ -0,0 +1,64 @@
> +// SPDX-License-Identifier: BSD-3-Clause
> +/*
> + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
> + */
> +
> +/dts-v1/;
> +/plugin/;
> +
> +#include <dt-bindings/gpio/gpio.h>
> +#include <dt-bindings/clock/qcom,rpmh.h>
> +#include <dt-bindings/clock/qcom,sa8775p-gcc.h>

[Severity: Low]
Are these include directives necessary? It looks like the definitions from
these headers are never used in this overlay, which might just add
unnecessary code clutter.

> +
> +/ {
> +	vmmc_sdc1: regulator-l8c {
> +		compatible = "regulator-fixed";
> +		regulator-name = "vreg-sdc1";
> +
> +		regulator-min-microvolt = <2960000>;
> +		regulator-max-microvolt = <2960000>;
> +	};
> +
> +	vqmmc_sdc1: regulator-s4a {
> +		compatible = "regulator-fixed";
> +		regulator-name = "vqmmc-sdc1";
> +
> +		regulator-min-microvolt = <1800000>;
> +		regulator-max-microvolt = <1800000>;
> +	};
> +};
> +
> +&sdhc {
> +	vmmc-supply = <&vmmc_sdc1>;
> +	vqmmc-supply = <&vqmmc_sdc1>;

[Severity: High]
Does wiring the SDHC controller to these dummy fixed regulators prevent
actual hardware control?

By defining these fixed regulators instead of referencing the real PMIC RPMH
regulators (vreg_l8c and vreg_s4a which are already defined in the base
device tree), the kernel might not issue the actual RPMh commands to enable,
disable, or configure voltage for the eMMC.

Could this cause initialization failures if the bootloader has not
pre-configured them, or lead to power management regressions during
suspend/resume where the kernel incorrectly assumes the hardware is
powered down?

[ ... ]
> diff --git a/arch/arm64/boot/dts/qcom/lemans.dtsi b/arch/arm64/boot/dts/qcom/lemans.dtsi
> index 353a6e6fd3acb..9f54d69c1be63 100644
> --- a/arch/arm64/boot/dts/qcom/lemans.dtsi
> +++ b/arch/arm64/boot/dts/qcom/lemans.dtsi
[ ... ]
> @@ -4311,9 +4313,11 @@ sdhc: mmc@87c4000 {
>  					  "pwr_irq";
>  
>  			clocks = <&gcc GCC_SDCC1_AHB_CLK>,
> -				 <&gcc GCC_SDCC1_APPS_CLK>;
> +				<&gcc GCC_SDCC1_APPS_CLK>,
> +				<&rpmhcc RPMH_CXO_CLK>;

[Severity: Low]
This isn't a bug, but is the indentation on the newly added clock items
meant to align with the first item in the list? It looks like it is slightly
misaligned compared to the original styling in the block.

>  			clock-names = "iface",
> -				      "core";
> +				      "core",
> +				      "xo";
>  
>  			interconnects = <&aggre1_noc MASTER_SDC QCOM_ICC_TAG_ALWAYS
>  					 &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>,

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