Re: [PATCH v3 7/8] mmc: sdhci-cadence: add Cadence SD6HC support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] SD6HC PHY timing calculations use the requested clock frequency instead of the actual physical hardware clock.
- [High] SD6HC PHY remains uninitialized during initial MMC card discovery at 400kHz.
- [High] Unvalidated `cdns,delay-element-ps` device tree property leads to multiple division-by-zero kernel panics.
- [High] The Agilex5 platform initialization logic is unreachable dead code due to a missing match table entry.
- [High] `clk_wr_delay` is incorrectly hardcoded to 255 when the required delay exceeds the DLL's maximum limit.
- [Medium] SD6HC PHY code bypasses the driver's established abstraction for accessing Host Register Set (HRS) registers.
- [Medium] `wmb()` is incorrectly used as a mechanism to flush posted MMIO writes to the hardware.
--

--- Patch [7]: [PATCH v3 7/8] mmc: sdhci-cadence: add Cadence SD6HC support ---
commit 61309bb2c01b2db2af64956b37f9af9f380deea8
Author: Tanmay Kathpalia <tanmay.kathpalia@altera.com>

mmc: sdhci-cadence: add Cadence SD6HC support

This commit introduces support for the Cadence sixth-generation SD/SDIO/eMMC
host controller (SD6HC) with an integrated combo-PHY. It splits the existing
driver into core and PHY-specific files, adding new configuration, timing,
and DLL logic.

> diff --git a/drivers/mmc/host/sdhci-cadence-core.c b/drivers/mmc/host/sdhci-cadence-core.c
> --- a/drivers/mmc/host/sdhci-cadence-core.c
> +++ b/drivers/mmc/host/sdhci-cadence-core.c
[ ... ]
> @@ -481,6 +471,16 @@ static const struct sdhci_ops sdhci_cdns4_ops = {
>  	.set_uhs_signaling = sdhci_cdns_set_uhs_signaling,
>  };
>  
> +static const struct sdhci_ops sdhci_cdns6_ops = {
> +	.set_clock = sdhci_set_clock,

[Severity: High]
Might this leave the SD6HC PHY uninitialized during initial MMC card
discovery at 400kHz? 

During power-up, the clock transitions from 0 to 400kHz while remaining in
MMC_TIMING_LEGACY. The core MMC sdhci_set_ios() optimizes clock enabling by
returning early if the timing mode does not change. Because this only hooks
set_uhs_signaling to program the PHY, it looks like it will get bypassed
when only the frequency changes. Does the driver need a custom set_clock
callback to intercept pure frequency changes and initialize the PHY?

> +	.get_timeout_clock = sdhci_cdns_get_timeout_clock,
> +	.set_bus_width = sdhci_set_bus_width,
> +	.reset = sdhci_reset,
> +	.platform_execute_tuning = sdhci_cdns_execute_tuning,
> +	.set_uhs_signaling = sdhci_cdns_set_uhs_signaling,
> +	.hw_reset = sdhci_cdns6_hw_reset,
> +};
[ ... ]
> @@ -671,6 +709,10 @@ static const struct of_device_id sdhci_cdns_match[] = {
>  		.compatible = "cdns,sd4hc",
>  		.data = &sdhci_cdns4_drv_data,
>  	},
> +	{
> +		.compatible = "cdns,sd6hc",
> +		.data = &sdhci_cdns6_drv_data,
> +	},
>  	{ /* sentinel */ }
>  };

[Severity: High]
Is the altr,agilex5-sd6hc compatible string missing from this match table?

Looking at the driver sources, there is an sdhci_cdns6_agilex5_init()
function that asserts and deasserts specific sdhc, combophy, and ocp resets
for the Altera Agilex5. Without a match entry hooking up this initialization
logic, the driver falls back to the generic cdns,sd6hc match data, making
the Agilex5 reset sequence unreachable dead code.

> diff --git a/drivers/mmc/host/sdhci-cadence-phy-v6.c b/drivers/mmc/host/sdhci-cadence-phy-v6.c
> --- /dev/null
> +++ b/drivers/mmc/host/sdhci-cadence-phy-v6.c
[ ... ]
> +static void sdhci_cdns6_write_phy_reg(struct sdhci_cdns_priv *priv, const u32 address,
> +				      const u32 value)
> +{
> +	writel(address, priv->hrs_addr + SDHCI_CDNS_HRS04);
> +	writel(value, priv->hrs_addr + SDHCI_CDNS_HRS05);
> +}

[Severity: Medium]
Will using direct writel() bypass the established priv->priv_writel()
abstraction?

The core sdhci-cadence architecture uses priv->priv_writel() to support
hardware like the Pensando Elba SoC, which requires custom byte-lane enable
writes. Using writel() directly for Host Register Set (HRS) access here
could cause silent hardware programming failures if SoCs with similar quirks
incorporate the SD6HC IP.

[ ... ]
> +	if (!phy->sdhc_extended_wr_mode) {
> +		u32 out_hold, out_setup, out_hold_margin;
> +		u32 n;
> +
> +		if (!data_ddr)
> +			wr0_dly = 1;
> +
> +		out_setup = output_max;
> +		out_hold = output_min;
> +		out_hold_margin = DIV_ROUND_UP(out_setup - out_hold, 4);
> +		out_hold += out_hold_margin;
> +
> +		if (!phy->cp_dll_bypass_mode)
> +			n = DIV_ROUND_UP(256 * out_hold, phy->t_sdmclk_calc);
> +		else
> +			n = DIV_ROUND_UP(out_hold, phy->delay_element) - 1;
> +
> +		if (n <= phy->dll_max_value)
> +			clk_wr_delay = n;
> +		else
> +			clk_wr_delay = 255;

[Severity: High]
Should this assignment clamp to phy->dll_max_value instead of hardcoding 255?

In sdhci_cdns6_phy_lock_dll(), phy->dll_max_value is explicitly restricted to
127 when delay elements are doubled. If n exceeds 127 in that scenario, writing
255 here violates the maximum allowable tap index, potentially causing
unpredictable wraparound or breakage of hardware timing on slow clocks.

[ ... ]
> +	/*
> +	 * Ensure all preceding PHY register writes complete and reach the controller before
> +	 * releasing the PHY from reset. Without this, SDR104 has been observed to fail
> +	 * intermittently on some boards.
> +	 */
> +	wmb();

[Severity: Medium]
Does a wmb() actually flush posted MMIO writes to the hardware here?

A wmb() only orders CPU instructions; it does not flush posted write buffers
on interconnect buses like PCIe or AXI. If the intent is to guarantee MMIO
writes have settled before the PHY is taken out of reset, a dummy readl()
to the same device is usually required to prevent intermittent device failures.

[ ... ]
> +	ret = of_property_read_u32(dev->of_node, "cdns,delay-element-ps", &phy->delay_element);
> +	if (ret)
> +		phy->delay_element = SDHCI_CDNS6_PHY_DEFAULT_DELAY_ELEMENT;
> +
> +	phy->delay_element_org = phy->delay_element;

[Severity: High]
Could this cause a division-by-zero kernel panic if the device tree explicitly
provides a 0 value for the delay element?

The property is read without validating that it is non-zero. Later, in
sdhci_cdns6_phy_lock_dll(), phy->delay_element is used as a divisor in the
DIV_ROUND_UP(phy->t_sdmclk, delay_element) macro. A malformed device tree
could trivially trigger this crash on probe.

[ ... ]
> +void sdhci_cdns6_set_uhs_signaling(struct sdhci_host *host, unsigned int timing)
> +{
> +	struct sdhci_cdns_priv *priv = sdhci_cdns_priv(host);
> +	struct sdhci_cdns6_phy *phy = priv->phy;
> +	int ret;
> +
> +	/* Clock may be 0 during initial ios setup; skip PHY update */
> +	if (!host->mmc->ios.clock)
> +		return;
> +
> +	phy->t_sdclk = DIV_ROUND_DOWN_ULL(1000000000000ULL, host->mmc->ios.clock);

[Severity: High]
Can this lead to PHY timing mismatches by using the requested ios.clock instead
of the actual physical hardware clock?

The host->mmc->ios.clock field contains the requested target frequency. However,
the SDHCI controller can only output discrete frequencies based on its internal
integer dividers, which is tracked in host->mmc->actual_clock. If the requested
clock doesn't perfectly match a divider step, the PHY will be programmed for a
frequency the bus isn't actually running at, potentially causing data corruption.

> +	phy->mode = timing;
> +
> +	ret = sdhci_cdns6_phy_update_timings(host);
[ ... ]
> +void sdhci_cdns6_hw_reset(struct sdhci_host *host)
> +{
> +	struct sdhci_cdns_priv *priv = sdhci_cdns_priv(host);
> +	void __iomem *reg;
> +
> +	reg = priv->hrs_addr + SDHCI_CDNS_HRS11;
> +	writel(SDHCI_CDNS_HRS11_EMMC_RST, reg);

[Severity: Medium]
Does this direct writel() skip the priv->priv_writel() abstraction as well?

Just like in sdhci_cdns6_write_phy_reg(), using writel() directly bypasses
the established abstraction and breaks variants that rely on custom byte-lane
writes for hardware reset.

> +	/* eMMC HW reset assertion: spec requires >= 1us, give margin */
> +	usleep_range(10, 20);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260724145009.7456-1-tanmay.kathpalia@altera.com?part=7