Re: [PATCH 2/4] arm64: dts: microchip: lan969x: add SDMMC nodes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Potential clock calculation truncation bug with `45000000` rate in `sdmmc1` leading to an out-of-spec initialization clock.
- [Medium] Identical clocks assigned to `hclock` and `multclk` in `sdmmc` nodes incorrectly model the hardware.
- [Low] Child nodes are not sorted alphanumerically by their unit-address.
- [Low] The compatible string 'microchip,lan9691-sdhci' is undocumented.
--

--- Patch [2]: [PATCH 2/4] arm64: dts: microchip: lan969x: add SDMMC nodes ---
commit 3b81135d2fb8590ea896af2a6f1a054ed2baf2c4
Author: Robert Marko <robert.marko@sartura.hr>

arm64: dts: microchip: lan969x: add SDMMC nodes

Add nodes for both SDMMC controllers.

> diff --git a/arch/arm64/boot/dts/microchip/lan9691.dtsi b/arch/arm64/boot/dts/microchip/lan9691.dtsi
> index a8541c6a098d1..2d943ea757689 100644
> --- a/arch/arm64/boot/dts/microchip/lan9691.dtsi
> +++ b/arch/arm64/boot/dts/microchip/lan9691.dtsi
> @@ -414,6 +414,30 @@ qspi0: spi@e0804000 {
>  			status = "disabled";
>  		};
>  
> +		sdmmc0: mmc@e0830000 {
> +			compatible = "microchip,lan9691-sdhci",
> +				     "microchip,sam9x60-sdhci";

[Severity: Low]
Is this new compatible string "microchip,lan9691-sdhci" documented in the
device tree bindings?

Running make dtbs_check might result in warnings if the string is missing from
the dt-bindings schemas.

> +			reg = <0xe0830000 0x00000300>;
> +			interrupts = <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&clks GCK_ID_SDMMC0>, <&clks GCK_ID_SDMMC0>;
> +			clock-names = "hclock", "multclk";

[Severity: Medium]
Are these clock assignments correct for hclock and multclk?

In AT91/Microchip SoC bindings, hclock typically points to the system peripheral
bus clock (AHB), while multclk points to the generated core clock.

By specifying the generated clock (GCK_ID_SDMMC0) for both, the driver will
never manage the actual AHB clock state, which could lead to power management
issues.

> +			assigned-clocks = <&clks GCK_ID_SDMMC0>;
> +			assigned-clock-rates = <100000000>;
> +			status = "disabled";
> +		};
> +
> +		sdmmc1: mmc@e0838000 {

[Severity: Low]
Should this node be placed after qspi2: spi@e0834000 to maintain alphanumeric
sorting by unit-address?

The address 0xe0838000 is greater than 0xe0834000.

> +			compatible = "microchip,lan9691-sdhci",
> +				     "microchip,sam9x60-sdhci";
> +			reg = <0xe0838000 0x00000300>;
> +			interrupts = <GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&clks GCK_ID_SDMMC1>, <&clks GCK_ID_SDMMC1>;
> +			clock-names = "hclock", "multclk";
> +			assigned-clocks = <&clks GCK_ID_SDMMC1>;
> +			assigned-clock-rates = <45000000>;

[Severity: High]
Does this 45MHz rate cause SD card initialization to fail due to exceeding the
400kHz limit?

During SDHCI probe, sdhci-of-at91 computes the base clock by dividing the
gck_rate by 2 and truncating to integer MHz (22.5 MHz -> 22 MHz). It writes 22
to the capabilities register with a multiplier of 2, so the core thinks the max
clock is 44 MHz.

When requesting a 400 kHz initialization clock, the core calculates a divider
of 110 (44,000,000 / 110 = 400,000).

However, the actual programmable clock source is exactly 45 MHz. Applying the
110 divider to the real 45 MHz source yields an actual initialization clock of
409,090 Hz, which exceeds the strict 400 kHz SD specification maximum.

Could we use a clock rate that divides cleanly into an integer MHz (like
48 MHz) to avoid this truncation issue?

> +			status = "disabled";
> +		};
> +
>  		qspi2: spi@e0834000 {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260824110432.1074136-1-robert.marko@sartura.hr?part=2