Re: [PATCH v5 8/8] mmc: sdhci-cadence: add Altera Agilex5 SD6HC support

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Agilex5-specific hardware reset sequence is missing during system resume.
--

--- Patch [8]: [PATCH v5 8/8] mmc: sdhci-cadence: add Altera Agilex5 SD6HC support ---
commit f23f3c9ace34e8e3ffc420a82aed2510f467099c
Author: Tanmay Kathpalia <[email protected]>

mmc: sdhci-cadence: add Altera Agilex5 SD6HC support

This patch adds support for the Altera Agilex5 SoC with a Cadence SD6HC
controller. It implements a platform-specific initialization routine to
handle overlapping resets for the SDHCI, SoftPHY, and OCP/AXI clock domains.

[ ... ]

> diff --git a/drivers/mmc/host/sdhci-cadence-core.c b/drivers/mmc/host/sdhci-cadence-core.c
> index 18a5329f27db9..e215f6a4c344f 100644
> --- a/drivers/mmc/host/sdhci-cadence-core.c
> +++ b/drivers/mmc/host/sdhci-cadence-core.c

[ ... ]

> @@ -462,6 +481,44 @@ static int elba_drv_init(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +static int sdhci_cdns6_agilex5_init(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct reset_control_bulk_data resets[] = {
> +		{ .id = "sdhc-reset" },
> +		{ .id = "combophy" },
> +		{ .id = "sdmmc-ocp" },
> +	};
> +	int ret;
> +
> +	/*
> +	 * Assert SDHCI, SoftPHY (combophy), and SDMMC OCP/AXI resets together
> +	 * so their active periods overlap before all domains are released.
> +	 * SoftPHY is shared with NAND, but only one of SDMMC
> +	 * or NAND is enabled on a given board.
> +	 */
> +	ret = reset_control_bulk_get_exclusive(dev, ARRAY_SIZE(resets), resets);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to get resets\n");
> +
> +	ret = reset_control_bulk_assert(ARRAY_SIZE(resets), resets);
> +	if (ret) {
> +		dev_err_probe(dev, ret, "failed to assert resets\n");
> +		goto out_put;
> +	}
> +
> +	/* Hold resets asserted long enough for all clock domains to capture. */
> +	usleep_range(10, 20);
> +
> +	ret = reset_control_bulk_deassert(ARRAY_SIZE(resets), resets);
> +	if (ret)
> +		dev_err_probe(dev, ret, "failed to deassert resets\n");
> +
> +out_put:
> +	reset_control_bulk_put(ARRAY_SIZE(resets), resets);
> +	return ret;
> +}

[Severity: Medium]
Is this reset sequence also required during system resume?

Looking at sdhci_cdns_resume() in sdhci-cadence-core.c, it appears the
platform-specific init routine is not executed upon waking:

static int sdhci_cdns_resume(struct device *dev)
{
	...
	ret = clk_prepare_enable(pltfm_host->clk);
	...
	ret = sdhci_resume_host(host);
}

If the SoC power-collapses these domains during suspend, will they require
this specific overlapping reset sequence upon wake to correctly synchronize
the clock domains again? Or is this hardware state maintained across suspend
or perhaps handled transparently by the boot firmware?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=8