| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <0ad9b5ac-3fd7-4cf0-befa-2c58589153e6@intel.com>
Date: Mon, 28 Jul 2025 11:17:13 +0300
From: Adrian Hunter <adrian.hunter@...el.com>
To: Benoît Monin <benoit.monin@...tlin.com>, Ulf Hansson
<ulf.hansson@...aro.org>
CC: <linux-mmc@...r.kernel.org>, <linux-kernel@...r.kernel.org>, "Vladimir
Kondratiev" <vladimir.kondratiev@...ileye.com>, Tawfik Bayouk
<tawfik.bayouk@...ileye.com>, Gregory CLEMENT <gregory.clement@...tlin.com>,
Thomas Petazzoni <thomas.petazzoni@...tlin.com>
Subject: Re: [PATCH v3 6/6] mmc: sdhci-cadence: implement multi-block read gap
tuning
On 16/07/2025 18:47, Benoît Monin wrote:
> The controller suspends the clock between blocks when reading from the
> MMC as part of its flow-control, called read block gap. At higher clock
> speed and with IO delay between the controller and the MMC, this clock
> pause can happen too late, during the read of the next block and
> trigger a read error.
>
> To prevent this, the delay can be programmed for each mode via the pair
> of registers HRS37/38. This delay is obtained during tuning, by trying
> a multi-block read and increasing the delay until the read succeeds.
>
> For now, the tuning is only done in HS200, as the read error has only
> been observed at that speed.
>
> Signed-off-by: Benoît Monin <benoit.monin@...tlin.com>
Some minor cosmetic comments below, otherwise:
Acked-by: Adrian Hunter <adrian.hunter@...el.com>
> ---
> drivers/mmc/host/sdhci-cadence.c | 69 +++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 68 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/sdhci-cadence.c b/drivers/mmc/host/sdhci-cadence.c
> index 2d823e158c59844dc7916db6a1d6e3d8b02ea5a0..0a9a90f9791d343b5d64ed602066f6291efa75b5 100644
> --- a/drivers/mmc/host/sdhci-cadence.c
> +++ b/drivers/mmc/host/sdhci-cadence.c
> @@ -36,6 +36,24 @@
> #define SDHCI_CDNS_HRS06_MODE_MMC_HS400 0x5
> #define SDHCI_CDNS_HRS06_MODE_MMC_HS400ES 0x6
>
> +/* Read block gap */
> +#define SDHCI_CDNS_HRS37 0x94 /* interface mode select */
> +#define SDHCI_CDNS_HRS37_MODE_DS 0x0
> +#define SDHCI_CDNS_HRS37_MODE_HS 0x1
> +#define SDHCI_CDNS_HRS37_MODE_UDS_SDR12 0x8
> +#define SDHCI_CDNS_HRS37_MODE_UDS_SDR25 0x9
> +#define SDHCI_CDNS_HRS37_MODE_UDS_SDR50 0xa
> +#define SDHCI_CDNS_HRS37_MODE_UDS_SDR104 0xb
> +#define SDHCI_CDNS_HRS37_MODE_UDS_DDR50 0xc
> +#define SDHCI_CDNS_HRS37_MODE_MMC_LEGACY 0x20
> +#define SDHCI_CDNS_HRS37_MODE_MMC_SDR 0x21
> +#define SDHCI_CDNS_HRS37_MODE_MMC_DDR 0x22
> +#define SDHCI_CDNS_HRS37_MODE_MMC_HS200 0x23
> +#define SDHCI_CDNS_HRS37_MODE_MMC_HS400 0x24
> +#define SDHCI_CDNS_HRS37_MODE_MMC_HS400ES 0x25
> +#define SDHCI_CDNS_HRS38 0x98 /* Read block gap coefficient */
> +#define SDHCI_CDNS_HRS38_BLKGAP_MAX 0xf
> +
> /* SRS - Slot Register Set (SDHCI-compatible) */
> #define SDHCI_CDNS_SRS_BASE 0x200
>
> @@ -251,6 +269,49 @@ static int sdhci_cdns_set_tune_val(struct sdhci_host *host, unsigned int val)
> return 0;
> }
>
> +/**
> + * sdhci_cdns_tune_blkgap() - tune multi-block read gap
> + * @mmc: MMC host
> + *
> + * Tune delay used in multi block read. To do so,
> + * try sending multi-block read command with incremented gap, unless
> + * it succeeds.
> + *
> + * Return: error code
> + */
> +static int sdhci_cdns_tune_blkgap(struct mmc_host *mmc)
> +{
> + struct sdhci_host *host = mmc_priv(mmc);
> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> + struct sdhci_cdns_priv *priv = sdhci_pltfm_priv(pltfm_host);
> + void __iomem *hrs37_reg = priv->hrs_addr + SDHCI_CDNS_HRS37;
> + void __iomem *hrs38_reg = priv->hrs_addr + SDHCI_CDNS_HRS38;
> + int ret;
> + u32 gap;
> + u32 hrs37_mode;
> +
> + switch (host->timing) {
> + case MMC_TIMING_MMC_HS200:
> + hrs37_mode = SDHCI_CDNS_HRS37_MODE_MMC_HS200;
> + break;
> + default:
> + return 0; /* no tuning in this mode */
> + }
Could refrain from the switch statement until it is needed.
So for now, just:
/* Currently only needed in HS200 mode */
if (host->timing != MMC_TIMING_MMC_HS200)
return 0;
writel(SDHCI_CDNS_HRS37_MODE_MMC_HS200, hrs37_reg);
> +
> + writel(hrs37_mode, hrs37_reg);
> +
> + for (gap = 0; gap <= SDHCI_CDNS_HRS38_BLKGAP_MAX; gap++) {
> + writel(gap, hrs38_reg);
> + ret = mmc_read_tuning(NULL, mmc, 512, 32);
> + if (ret == 0)
Kernel style is:
if (!ret)
> + break;
> + }
> +
> + dev_dbg(mmc_dev(mmc), "read block gap tune %s, gap %d\n",
> + ret == 0 ? "OK" : "failed", gap);
Kernel style is:
ret ? "failed" : "OK"
Also up to 100 columns is ok, so could be all one line if you like.
> + return ret;
> +}
> +
> /*
> * In SD mode, software must not use the hardware tuning and instead perform
> * an almost identical procedure to eMMC.
> @@ -261,6 +322,7 @@ static int sdhci_cdns_execute_tuning(struct sdhci_host *host, u32 opcode)
> int max_streak = 0;
> int end_of_streak = 0;
> int i;
> + int ret;
>
> /*
> * Do not execute tuning for UHS_SDR50 or UHS_DDR50.
> @@ -288,7 +350,12 @@ static int sdhci_cdns_execute_tuning(struct sdhci_host *host, u32 opcode)
> return -EIO;
> }
>
> - return sdhci_cdns_set_tune_val(host, end_of_streak - max_streak / 2);
> + ret = sdhci_cdns_set_tune_val(host, end_of_streak - max_streak / 2);
> +
> + if (!ret)
> + ret = sdhci_cdns_tune_blkgap(host->mmc);
> +
> + return ret;
Or tidier:
ret = sdhci_cdns_set_tune_val(host, end_of_streak - max_streak / 2);
if (ret)
return ret;
return sdhci_cdns_tune_blkgap(host->mmc);
> }
>
> static void sdhci_cdns_set_uhs_signaling(struct sdhci_host *host,
>
Powered by blists - more mailing lists