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

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

Hello Sascha,

On 17.02.25 11:53, Sascha Hauer wrote:
> 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?

Yes, this was indeed wrong. csd.capacity in Linux is in units of sectors.

(x / SECTOR_SHIFT) >> 11 -> x / SZ_1M.

I fixed that in v2.

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

I'd prefer to keep the rest as-is as it's a verbatim copy from Linux
mmc_init_erase() in drivers/mmc/core/core.c.

Thanks,
Ahmad

> 
> 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 |