Re: [PATCH 1/2] mci: core: import Linux logic for higher preferred erase size

[Sascha Hauer <s.hauer@pengutronix.de>]

On Fri, Feb 14, 2025 at 10:48:49AM +0100, Ahmad Fatoum wrote:
> As a comment in the file notes, doing too small a granularity for erases
> has considerable effect on performance:
> 
>   > Example Samsung eMMC 8GTF4:
>   >
>   >   time erase /dev/mmc2.part_of_512m # 1024 trims
>   >   time: 2849ms
>   >
>   >   time erase /dev/mmc2.part_of_512m # single trim
>   >   time: 56ms
> 
> This was deemed acceptable at first, because 3 seconds is still
> tolerable.
> 
> On a SkyHigh S40004, an erase of the whole 3728 MiB ended up
> taking longer than 400s in barebox, but only 4s in Linux, which
> dwarfs the time actually needed for writing.
> 
> Linux has some rather complicated logic to compute a higher erase size
> granularity, which still fits in the max busy timeout that a controller
> may require. Until that's support in barebox, we import a simpler
> heuristic that Linux uses to compute
> 
>   /sys/class/mmc_host/*/*/preferred_erase_size
> 
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  drivers/mci/mci-core.c | 105 ++++++++++++++++++++++++++---------------
>  include/mci.h          |   1 +
>  2 files changed, 69 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/mci/mci-core.c b/drivers/mci/mci-core.c
> index cc3c6fba3653..6d55eb8305b9 100644
> --- a/drivers/mci/mci-core.c
> +++ b/drivers/mci/mci-core.c
> @@ -1774,6 +1774,70 @@ static int mci_startup_mmc(struct mci *mci)
>  	return ret >= MMC_BUS_WIDTH_1 ? 0 : ret;
>  }
>  
> +static void mci_init_erase(struct mci *card)
> +{
> +	unsigned int sz;
> +
> +	if (!IS_ENABLED(CONFIG_MCI_ERASE))
> +		return;
> +
> +	/* TODO: While it's possible to clear many erase groups at once
> +	 * and it greatly improves throughput, drivers need adjustment:
> +	 *
> +	 * Many drivers hardcode a maximal wait time before aborting
> +	 * the wait for R1b and returning -ETIMEDOUT. With long
> +	 * erases/trims, we are bound to run into this timeout, so for now
> +	 * we just split into sufficiently small erases that are unlikely
> +	 * to trigger the timeout.
> +	 *
> +	 * What Linux does and what we should be doing in barebox is:
> +	 *
> +	 *  - add a struct mci_cmd::busy_timeout member that drivers should
> +	 *    use instead of hardcoding their own timeout delay. The busy
> +	 *    timeout length can be calculated by the MCI core after
> +	 *    consulting the appropriate CSD/EXT_CSD/SSR registers.
> +	 *
> +	 *  - add a struct mci_host::max_busy_timeout member, where drivers
> +	 *    can indicate the maximum timeout they are able to support.
> +	 *    The MCI core will never set a busy_timeout that exceeds this
> +	 *    value.
> +	 *
> +	 *  Example Samsung eMMC 8GTF4:
> +	 *
> +	 *    time erase /dev/mmc2.part_of_512m # 1024 trims
> +	 *    time: 2849ms
> +	 *
> +	 *    time erase /dev/mmc2.part_of_512m # single trim
> +	 *    time: 56ms
> +	 */
> +	if (IS_SD(card) && card->ssr.au) {
> +		card->pref_erase = card->ssr.au;
> +	} else if (card->erase_grp_size) {
> +		sz = card->capacity >> 11;
> +		if (sz < 128)
> +			card->pref_erase = 512 * 1024 / 512;
> +		else if (sz < 512)
> +			card->pref_erase = 1024 * 1024 / 512;
> +		else if (sz < 1024)
> +			card->pref_erase = 2 * 1024 * 1024 / 512;
> +		else
> +			card->pref_erase = 4 * 1024 * 1024 / 512;

card->capacity is in bytes, so you are falling into the last case for
cards bigger than 512Kib. Did you mean to right shift by 21 or even 31
instead?

I would prefer using SZ_* and SECTOR_SIZE/SHIFT defines to make this more
readable.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |