Re: [PATCH] cmd: mtd: Enable speed benchmarking

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hello Michael,

>> MA35D1> mtd read nor0 0x81000000 0 0x10000
>> Reading 65536 byte(s) at offset 0x00000000
>> MA35D1> mtd read.benchmark nor0 0x81000000 0 0x10000
>> Reading 65536 byte(s) at offset 0x00000000
>> Read speed: 3752kiB/s
>>
>
> I was using time and reading but this looked much nicer. I will give it a try
> and review.

Thanks! It turned out very useful for development purposes :-)

>> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
>> ---

[...]

>> -       bool dump, read, raw, woob, write_empty_pages, has_pages = false;
>> +       bool dump, read, raw, woob, benchmark, write_empty_pages, has_pages = false;
>>         u64 start_off, off, len, remaining, default_len;
>>         struct mtd_oob_ops io_op = {};
>>         uint user_addr = 0, npages;
>> +       u32 bench_start, bench_end;
>
> unsigned long

Okay.

>>         const char *cmd = argv[0];
>>         struct mtd_info *mtd;
>>         u32 oob_len;
>> @@ -490,6 +492,7 @@ static int do_mtd_io(struct cmd_tbl *cmdtp, int flag, int argc,
>>         read = dump || !strncmp(cmd, "read", 4);
>>         raw = strstr(cmd, ".raw");
>>         woob = strstr(cmd, ".oob");
>> +       benchmark = strstr(cmd, ".benchmark");
>>         write_empty_pages = !has_pages || strstr(cmd, ".dontskipff");
>>
>>         argc -= 2;
>> @@ -559,6 +562,9 @@ static int do_mtd_io(struct cmd_tbl *cmdtp, int flag, int argc,
>>
>>         led_activity_blink();
>>
>> +       if (benchmark)
>> +               bench_start = timer_get_us();
>> +
>>         /* Loop over the pages to do the actual read/write */
>>         while (remaining) {
>>                 /* Skip the block if it is bad */
>> @@ -586,6 +592,13 @@ static int do_mtd_io(struct cmd_tbl *cmdtp, int flag, int argc,
>>                 io_op.oobbuf += io_op.oobretlen;
>>         }
>>
>> +       if (benchmark && bench_start) {
>> +               bench_end = timer_get_us();
>> +               printf("%s speed: %lukiB/s\n",
>> +                      read ? "Read" : "Write",
>> +                      ((io_op.len * 1000000) / (bench_end - bench_start)) / 1024);
>
> Did you check if it can not wrap?

That is a good question, if my calculations are correct, time capture
may wrap if one spends more than 1h12 in the Bootloader. So yes it is
'doable' to make the time overlap.

But is this relevant? It really is a development feature at the moment
(developers comparing speeds would notice the overlap) and Bootloaders
in general are not supposed to run for that long anyway. So I would
argue that it is acceptable like that.

Thanks,
Miquèl