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