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