[U-Boot] [PATCH] spi: sf: add support for throughput mesurement of sf read/write

[U-Boot] [PATCH] spi: sf: add support for throughput mesurement of sf read/write

[Mugunthan V N]

This patch adds time measurement and throughput calculation for
sf read/write commands.

The output of sf read changes from

---8<---
SF: 4096 bytes @ 0x0 Read: OK
--->8---

to

---8<---
SF: 4096 bytes @ 0x0 Read: OK in 6 ms (666 KiB/s)
--->8---

Signed-off-by: Mugunthan V N <mugunthanvnm@ti.com>
---
 common/cmd_sf.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/common/cmd_sf.c b/common/cmd_sf.c
index ac7f5df..8687f3a 100644
--- a/common/cmd_sf.c
+++ b/common/cmd_sf.c
@@ -15,6 +15,7 @@
 #include <spi_flash.h>
 #include <jffs2/jffs2.h>
 #include <linux/mtd/mtd.h>
+#include <linux/math64.h>
 
 #include <asm/io.h>
 #include <dm/device-internal.h>
@@ -266,6 +267,7 @@ static int do_spi_flash_read_write(int argc, char * const argv[])
 	int ret = 1;
 	int dev = 0;
 	loff_t offset, len, maxsize;
+	unsigned long time;
 
 	if (argc < 3)
 		return -1;
@@ -298,17 +300,27 @@ static int do_spi_flash_read_write(int argc, char * const argv[])
 		int read;
 
 		read = strncmp(argv[0], "read", 4) == 0;
+
+		time = get_timer(0);
 		if (read)
 			ret = spi_flash_read(flash, offset, len, buf);
 		else
 			ret = spi_flash_write(flash, offset, len, buf);
+		time = get_timer(time);
 
 		printf("SF: %zu bytes @ %#x %s: ", (size_t)len, (u32)offset,
 		       read ? "Read" : "Written");
-		if (ret)
+		if (ret) {
 			printf("ERROR %d\n", ret);
-		else
-			printf("OK\n");
+		} else {
+			printf("OK in %lu ms", time);
+			if (time > 0) {
+				puts(" (");
+				print_size(div_u64(len, time) * 1000, "/s");
+				puts(")");
+			}
+			puts("\n");
+		}
 	}
 
 	unmap_physmem(buf, len);
-- 
2.6.2.280.g74301d6

[U-Boot] [PATCH] spi: sf: add support for throughput mesurement of sf read/write

[Mugunthan V N]

Jagan

On Tuesday 27 October 2015 07:24 PM, Mugunthan V N wrote:
> This patch adds time measurement and throughput calculation for
> sf read/write commands.
> 
> The output of sf read changes from
> 
> ---8<---
> SF: 4096 bytes @ 0x0 Read: OK
> --->8---
> 
> to
> 
> ---8<---
> SF: 4096 bytes @ 0x0 Read: OK in 6 ms (666 KiB/s)
> --->8---
> 
> Signed-off-by: Mugunthan V N <mugunthanvnm@ti.com>

A gentle ping

Regards
Mugunthan V N

> ---
>  common/cmd_sf.c | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/common/cmd_sf.c b/common/cmd_sf.c
> index ac7f5df..8687f3a 100644
> --- a/common/cmd_sf.c
> +++ b/common/cmd_sf.c
> @@ -15,6 +15,7 @@
>  #include <spi_flash.h>
>  #include <jffs2/jffs2.h>
>  #include <linux/mtd/mtd.h>
> +#include <linux/math64.h>
>  
>  #include <asm/io.h>
>  #include <dm/device-internal.h>
> @@ -266,6 +267,7 @@ static int do_spi_flash_read_write(int argc, char * const argv[])
>  	int ret = 1;
>  	int dev = 0;
>  	loff_t offset, len, maxsize;
> +	unsigned long time;
>  
>  	if (argc < 3)
>  		return -1;
> @@ -298,17 +300,27 @@ static int do_spi_flash_read_write(int argc, char * const argv[])
>  		int read;
>  
>  		read = strncmp(argv[0], "read", 4) == 0;
> +
> +		time = get_timer(0);
>  		if (read)
>  			ret = spi_flash_read(flash, offset, len, buf);
>  		else
>  			ret = spi_flash_write(flash, offset, len, buf);
> +		time = get_timer(time);
>  
>  		printf("SF: %zu bytes @ %#x %s: ", (size_t)len, (u32)offset,
>  		       read ? "Read" : "Written");
> -		if (ret)
> +		if (ret) {
>  			printf("ERROR %d\n", ret);
> -		else
> -			printf("OK\n");
> +		} else {
> +			printf("OK in %lu ms", time);
> +			if (time > 0) {
> +				puts(" (");
> +				print_size(div_u64(len, time) * 1000, "/s");
> +				puts(")");
> +			}
> +			puts("\n");
> +		}
>  	}
>  
>  	unmap_physmem(buf, len);
> 

[U-Boot] [PATCH] spi: sf: add support for throughput mesurement of sf read/write

[Jagan Teki]

On 17 December 2015 at 12:33, Mugunthan V N <mugunthanvnm@ti.com> wrote:
> Jagan
>
> On Tuesday 27 October 2015 07:24 PM, Mugunthan V N wrote:
>> This patch adds time measurement and throughput calculation for
>> sf read/write commands.
>>
>> The output of sf read changes from
>>
>> ---8<---
>> SF: 4096 bytes @ 0x0 Read: OK
>> --->8---
>>
>> to
>>
>> ---8<---
>> SF: 4096 bytes @ 0x0 Read: OK in 6 ms (666 KiB/s)
>> --->8---
>>
>> Signed-off-by: Mugunthan V N <mugunthanvnm@ti.com>

Was it similar to 'sf update' ? please check it once.

>
> A gentle ping

thanks!
-- 
Jagan.

[U-Boot] [PATCH] spi: sf: add support for throughput mesurement of sf read/write

[Mugunthan V N]

On Thursday 17 December 2015 12:43 PM, Jagan Teki wrote:
> On 17 December 2015 at 12:33, Mugunthan V N <mugunthanvnm@ti.com> wrote:
>> Jagan
>>
>> On Tuesday 27 October 2015 07:24 PM, Mugunthan V N wrote:
>>> This patch adds time measurement and throughput calculation for
>>> sf read/write commands.
>>>
>>> The output of sf read changes from
>>>
>>> ---8<---
>>> SF: 4096 bytes @ 0x0 Read: OK
>>> --->8---
>>>
>>> to
>>>
>>> ---8<---
>>> SF: 4096 bytes @ 0x0 Read: OK in 6 ms (666 KiB/s)
>>> --->8---
>>>
>>> Signed-off-by: Mugunthan V N <mugunthanvnm@ti.com>
> 
> Was it similar to 'sf update' ? please check it once.
> 

sf update out similar but also uses progressive output, in read/write
case it can't be done. The final throughput measurement is similar on
both update and read/write.

Regards
Mugunthan V N

[U-Boot] [PATCH] spi: sf: add support for throughput mesurement of sf read/write

[Jagan Teki]

On 17 December 2015 at 13:26, Mugunthan V N <mugunthanvnm@ti.com> wrote:
> On Thursday 17 December 2015 12:43 PM, Jagan Teki wrote:
>> On 17 December 2015 at 12:33, Mugunthan V N <mugunthanvnm@ti.com> wrote:
>>> Jagan
>>>
>>> On Tuesday 27 October 2015 07:24 PM, Mugunthan V N wrote:
>>>> This patch adds time measurement and throughput calculation for
>>>> sf read/write commands.
>>>>
>>>> The output of sf read changes from
>>>>
>>>> ---8<---
>>>> SF: 4096 bytes @ 0x0 Read: OK
>>>> --->8---
>>>>
>>>> to
>>>>
>>>> ---8<---
>>>> SF: 4096 bytes @ 0x0 Read: OK in 6 ms (666 KiB/s)
>>>> --->8---
>>>>
>>>> Signed-off-by: Mugunthan V N <mugunthanvnm@ti.com>
>>
>> Was it similar to 'sf update' ? please check it once.
>>
>
> sf update out similar but also uses progressive output, in read/write
> case it can't be done. The final throughput measurement is similar on
> both update and read/write.

True, that's what if we need a progressed throughput just use 'sf
update' else normal 'sf read/write' It's look not good to me to add
extra code on top of generic commands. What ever we wanted to extend
features let's added it on 'sf update' than sf read/write, Sorry.

thanks!
-- 
Jagan.

[U-Boot] [PATCH] spi: sf: add support for throughput mesurement of sf read/write

[Stefan Roese]

On 17.12.2015 17:44, Jagan Teki wrote:
> On 17 December 2015 at 13:26, Mugunthan V N <mugunthanvnm@ti.com> wrote:
>> On Thursday 17 December 2015 12:43 PM, Jagan Teki wrote:
>>> On 17 December 2015 at 12:33, Mugunthan V N <mugunthanvnm@ti.com> wrote:
>>>> Jagan
>>>>
>>>> On Tuesday 27 October 2015 07:24 PM, Mugunthan V N wrote:
>>>>> This patch adds time measurement and throughput calculation for
>>>>> sf read/write commands.
>>>>>
>>>>> The output of sf read changes from
>>>>>
>>>>> ---8<---
>>>>> SF: 4096 bytes @ 0x0 Read: OK
>>>>> --->8---
>>>>>
>>>>> to
>>>>>
>>>>> ---8<---
>>>>> SF: 4096 bytes @ 0x0 Read: OK in 6 ms (666 KiB/s)
>>>>> --->8---
>>>>>
>>>>> Signed-off-by: Mugunthan V N <mugunthanvnm@ti.com>
>>>
>>> Was it similar to 'sf update' ? please check it once.
>>>
>>
>> sf update out similar but also uses progressive output, in read/write
>> case it can't be done. The final throughput measurement is similar on
>> both update and read/write.
>
> True, that's what if we need a progressed throughput just use 'sf
> update' else normal 'sf read/write' It's look not good to me to add
> extra code on top of generic commands. What ever we wanted to extend
> features let's added it on 'sf update' than sf read/write, Sorry.

If I need to measure the time of commands, I use the "time"
command ("time sf write ...") by enabling it via CONFIG_CMD_TIME.
This provides all the needed information to detect performance
changes.

Thanks,
Stefan

[U-Boot] [PATCH] spi: sf: add support for throughput mesurement of sf read/write

[Mugunthan V N]

On Friday 18 December 2015 11:37 AM, Stefan Roese wrote:
> On 17.12.2015 17:44, Jagan Teki wrote:
>> On 17 December 2015 at 13:26, Mugunthan V N <mugunthanvnm@ti.com> wrote:
>>> On Thursday 17 December 2015 12:43 PM, Jagan Teki wrote:
>>>> On 17 December 2015 at 12:33, Mugunthan V N <mugunthanvnm@ti.com>
>>>> wrote:
>>>>> Jagan
>>>>>
>>>>> On Tuesday 27 October 2015 07:24 PM, Mugunthan V N wrote:
>>>>>> This patch adds time measurement and throughput calculation for
>>>>>> sf read/write commands.
>>>>>>
>>>>>> The output of sf read changes from
>>>>>>
>>>>>> ---8<---
>>>>>> SF: 4096 bytes @ 0x0 Read: OK
>>>>>> --->8---
>>>>>>
>>>>>> to
>>>>>>
>>>>>> ---8<---
>>>>>> SF: 4096 bytes @ 0x0 Read: OK in 6 ms (666 KiB/s)
>>>>>> --->8---
>>>>>>
>>>>>> Signed-off-by: Mugunthan V N <mugunthanvnm@ti.com>
>>>>
>>>> Was it similar to 'sf update' ? please check it once.
>>>>
>>>
>>> sf update out similar but also uses progressive output, in read/write
>>> case it can't be done. The final throughput measurement is similar on
>>> both update and read/write.
>>
>> True, that's what if we need a progressed throughput just use 'sf
>> update' else normal 'sf read/write' It's look not good to me to add
>> extra code on top of generic commands. What ever we wanted to extend
>> features let's added it on 'sf update' than sf read/write, Sorry.
> 
> If I need to measure the time of commands, I use the "time"
> command ("time sf write ...") by enabling it via CONFIG_CMD_TIME.
> This provides all the needed information to detect performance
> changes.
> 

But similar kind of implementations is present for fatload and tftp. So
I thought having similar performance log for sf read/write will be good
as well.

Regards
Mugunthan V N

[U-Boot] [PATCH] spi: sf: add support for throughput mesurement of sf read/write

[Stefan Roese]

On 21.12.2015 06:37, Mugunthan V N wrote:
> On Friday 18 December 2015 11:37 AM, Stefan Roese wrote:
>> On 17.12.2015 17:44, Jagan Teki wrote:
>>> On 17 December 2015 at 13:26, Mugunthan V N <mugunthanvnm@ti.com> wrote:
>>>> On Thursday 17 December 2015 12:43 PM, Jagan Teki wrote:
>>>>> On 17 December 2015 at 12:33, Mugunthan V N <mugunthanvnm@ti.com>
>>>>> wrote:
>>>>>> Jagan
>>>>>>
>>>>>> On Tuesday 27 October 2015 07:24 PM, Mugunthan V N wrote:
>>>>>>> This patch adds time measurement and throughput calculation for
>>>>>>> sf read/write commands.
>>>>>>>
>>>>>>> The output of sf read changes from
>>>>>>>
>>>>>>> ---8<---
>>>>>>> SF: 4096 bytes @ 0x0 Read: OK
>>>>>>> --->8---
>>>>>>>
>>>>>>> to
>>>>>>>
>>>>>>> ---8<---
>>>>>>> SF: 4096 bytes @ 0x0 Read: OK in 6 ms (666 KiB/s)
>>>>>>> --->8---
>>>>>>>
>>>>>>> Signed-off-by: Mugunthan V N <mugunthanvnm@ti.com>
>>>>>
>>>>> Was it similar to 'sf update' ? please check it once.
>>>>>
>>>>
>>>> sf update out similar but also uses progressive output, in read/write
>>>> case it can't be done. The final throughput measurement is similar on
>>>> both update and read/write.
>>>
>>> True, that's what if we need a progressed throughput just use 'sf
>>> update' else normal 'sf read/write' It's look not good to me to add
>>> extra code on top of generic commands. What ever we wanted to extend
>>> features let's added it on 'sf update' than sf read/write, Sorry.
>>
>> If I need to measure the time of commands, I use the "time"
>> command ("time sf write ...") by enabling it via CONFIG_CMD_TIME.
>> This provides all the needed information to detect performance
>> changes.
>>
>
> But similar kind of implementations is present for fatload and tftp. So
> I thought having similar performance log for sf read/write will be good
> as well.

Yes, I understand. I also find this output helpful. But I'm not sure,
if we should add this timing code to all these functions. Instead
of just using this common time command, if needed. To keep the
code size at a minimum.

Of course its no big code addition, so I don't really have any bigger
reservations against it.

Thanks,
Stefan

[U-Boot] [PATCH] spi: sf: add support for throughput mesurement of sf read/write

[Jagan Teki]

Hi Mugunthan,

On 21 December 2015 at 11:30, Stefan Roese <sr@denx.de> wrote:
> On 21.12.2015 06:37, Mugunthan V N wrote:
>>
>> On Friday 18 December 2015 11:37 AM, Stefan Roese wrote:
>>>
>>> On 17.12.2015 17:44, Jagan Teki wrote:
>>>>
>>>> On 17 December 2015 at 13:26, Mugunthan V N <mugunthanvnm@ti.com> wrote:
>>>>>
>>>>> On Thursday 17 December 2015 12:43 PM, Jagan Teki wrote:
>>>>>>
>>>>>> On 17 December 2015 at 12:33, Mugunthan V N <mugunthanvnm@ti.com>
>>>>>> wrote:
>>>>>>>
>>>>>>> Jagan
>>>>>>>
>>>>>>> On Tuesday 27 October 2015 07:24 PM, Mugunthan V N wrote:
>>>>>>>>
>>>>>>>> This patch adds time measurement and throughput calculation for
>>>>>>>> sf read/write commands.
>>>>>>>>
>>>>>>>> The output of sf read changes from
>>>>>>>>
>>>>>>>> ---8<---
>>>>>>>> SF: 4096 bytes @ 0x0 Read: OK
>>>>>>>> --->8---
>>>>>>>>
>>>>>>>> to
>>>>>>>>
>>>>>>>> ---8<---
>>>>>>>> SF: 4096 bytes @ 0x0 Read: OK in 6 ms (666 KiB/s)
>>>>>>>> --->8---
>>>>>>>>
>>>>>>>> Signed-off-by: Mugunthan V N <mugunthanvnm@ti.com>
>>>>>>
>>>>>>
>>>>>> Was it similar to 'sf update' ? please check it once.
>>>>>>
>>>>>
>>>>> sf update out similar but also uses progressive output, in read/write
>>>>> case it can't be done. The final throughput measurement is similar on
>>>>> both update and read/write.
>>>>
>>>>
>>>> True, that's what if we need a progressed throughput just use 'sf
>>>> update' else normal 'sf read/write' It's look not good to me to add
>>>> extra code on top of generic commands. What ever we wanted to extend
>>>> features let's added it on 'sf update' than sf read/write, Sorry.
>>>
>>>
>>> If I need to measure the time of commands, I use the "time"
>>> command ("time sf write ...") by enabling it via CONFIG_CMD_TIME.
>>> This provides all the needed information to detect performance
>>> changes.
>>>
>>
>> But similar kind of implementations is present for fatload and tftp. So
>> I thought having similar performance log for sf read/write will be good
>> as well.
>
>
> Yes, I understand. I also find this output helpful. But I'm not sure,
> if we should add this timing code to all these functions. Instead
> of just using this common time command, if needed. To keep the
> code size at a minimum.
>
> Of course its no big code addition, so I don't really have any bigger
> reservations against it.

Why can't just use 'sf update' instead of adding again same code in
'sf read/write' since 'sf update' is doing more than just giving
timing performance. Please try!

thanks!
-- 
Jagan.