On Fri, 27 Nov 2015 15:35:18 +0100
Bernhard Nortmann <[email protected]> wrote:

> This patch introduces a new function aw_write_buffer() to
> allow a better distinction between FEL write operations.
> 
> aw_fel_write() is meant for "privileged" internal use, while
> aw_write_buffer() now represents the preferred entry point
> for user code like the "write", "fill" or "clear" commands.
> 
> There is some deliberate code duplication here that makes sense
> when combined with the next patch, where the aw_usb_write()
> call will differ. We want aw_fel_write() to enforce no progress
> update/callback, while aw_write_buffer() will (optionally)
> support it.

If we need comments like this, then maybe the series is split into
too many small patches? Tracking such dependencies and nuances
does not exactly make the review easier. But it's a minor thing
and there is no need for another review round.

> Signed-off-by: Bernhard Nortmann <[email protected]>
>
> ---
>  fel.c | 51 +++++++++++++++++++++++++++++++++------------------
>  1 file changed, 33 insertions(+), 18 deletions(-)
> 
> diff --git a/fel.c b/fel.c
> index aca8162..3428117 100644
> --- a/fel.c
> +++ b/fel.c
> @@ -254,14 +254,6 @@ void aw_fel_read(libusb_device_handle *usb, uint32_t 
> offset, void *buf, size_t l
>  
>  void aw_fel_write(libusb_device_handle *usb, void *buf, uint32_t offset, 
> size_t len)
>  {
> -     /* safeguard against overwriting an already loaded U-Boot binary */
> -     if (uboot_size > 0 && offset <= uboot_entry + uboot_size && offset + 
> len >= uboot_entry) {
> -             fprintf(stderr, "ERROR: Attempt to overwrite U-Boot! "
> -                     "Request 0x%08X-0x%08X overlaps 0x%08X-0x%08X.\n",
> -                     offset, offset + (int)len,
> -                     uboot_entry, uboot_entry + uboot_size);
> -             exit(1);
> -     }
>       aw_send_fel_request(usb, AW_FEL_1_WRITE, offset, len);
>       aw_usb_write(usb, buf, len);
>       aw_read_fel_status(usb);
> @@ -273,6 +265,33 @@ void aw_fel_execute(libusb_device_handle *usb, uint32_t 
> offset)
>       aw_read_fel_status(usb);
>  }
>  
> +/*
> + * This function is a higher-level wrapper for the FEL write functionality.
> + * Unlike aw_fel_write() above - which is reserved for internal use - this
> + * routine is meant to be called from "user" code, and supports (= allows)
> + * progress callbacks.
> + * The return value represents elapsed time in seconds (needed for 
> execution).
> + */
> +double aw_write_buffer(libusb_device_handle *usb, void *buf, uint32_t offset,
> +                    size_t len)
> +{
> +     /* safeguard against overwriting an already loaded U-Boot binary */
> +     if (uboot_size > 0 && offset <= uboot_entry + uboot_size
> +                        && offset + len >= uboot_entry)
> +     {
> +             fprintf(stderr, "ERROR: Attempt to overwrite U-Boot! "
> +                     "Request 0x%08X-0x%08X overlaps 0x%08X-0x%08X.\n",
> +                     offset, offset + len,
> +                     uboot_entry, uboot_entry + uboot_size);
> +             exit(1);
> +     }
> +     double start = gettime();
> +     aw_send_fel_request(usb, AW_FEL_1_WRITE, offset, len);
> +     aw_usb_write(usb, buf, len);
> +     aw_read_fel_status(usb);
> +     return gettime() - start;
> +}
> +
>  void hexdump(void *data, uint32_t offset, size_t size)
>  {
>       size_t j;
> @@ -362,7 +381,7 @@ void aw_fel_fill(libusb_device_handle *usb, uint32_t 
> offset, size_t size, unsign
>  {
>       unsigned char buf[size];
>       memset(buf, value, size);
> -     aw_fel_write(usb, buf, offset, size);
> +     aw_write_buffer(usb, buf, offset, size);
>  }
>  
>  /*
> @@ -1068,7 +1087,7 @@ void aw_fel_write_uboot_image(libusb_device_handle *usb,
>       pr_info("Writing image \"%.*s\", %u bytes @ 0x%08X.\n",
>               IH_NMLEN, buf + HEADER_NAME_OFFSET, data_size, load_addr);
>  
> -     aw_fel_write(usb, buf + HEADER_SIZE, load_addr, data_size);
> +     aw_write_buffer(usb, buf + HEADER_SIZE, load_addr, data_size);
>  
>       /* keep track of U-Boot memory region in global vars */
>       uboot_entry = load_addr;
> @@ -1270,17 +1289,13 @@ int main(int argc, char **argv)
>                       aw_fel_print_version(handle);
>                       skip=1;
>               } else if (strcmp(argv[1], "write") == 0 && argc > 3) {
> -                     double t1, t2;
>                       size_t size;
>                       void *buf = load_file(argv[3], &size);
>                       uint32_t offset = strtoul(argv[2], NULL, 0);
> -                     t1 = gettime();
> -                     aw_fel_write(handle, buf, offset, size);
> -                     t2 = gettime();
> -                     if (t2 > t1)
> -                             pr_info("Written %.1f KB in %.1f sec (speed: 
> %.1f KB/s)\n",
> -                                     (double)size / 1000., t2 - t1,
> -                                     (double)size / (t2 - t1) / 1000.);
> +                     double elapsed = aw_write_buffer(handle, buf, offset, 
> size);
> +                     if (elapsed > 0)
> +                             pr_info("%.1f kB written in %.1f sec (speed: 
> %.1f kB/s)\n",
> +                                     kilo(size), elapsed, kilo(size) / 
> elapsed);
>                       /*
>                        * If we have transferred a script, try to inform U-Boot
>                        * about its address.

Reviewed-by: Siarhei Siamashka <[email protected]>

-- 
Best regards,
Siarhei Siamashka

-- 
You received this message because you are subscribed to the Google Groups 
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to [email protected].
For more options, visit https://groups.google.com/d/optout.

Reply via email to