[U-Boot] [U-Boot, v3, 2/9] usb: rockchip: implement skeleton for K_FW_GET_CHIP_VER command

[Philipp Tomsich]

On Thu, 12 Jul 2018, Alberto Panizzo wrote:

> Chip Version is a string saved in BOOTROM address space Little Endian.

There's just one tiny problem: the BOOTROM will not be accessible on ARMv8 
targets after ATF has been loaded.

We should define a common mechanism how to deal with this (e.g. by 
injecting it into the DTS of later stages during SPL) and then provide a 
common abstraction over the platforms that always allow accessing the 
BootROM and those that require additional magic.

>
> Ex for rk3288: 0x33323041 0x32303134 0x30383133 0x56323030
> which brings:  320A20140813V200
>
> Note that memory version do invert MSB/LSB so printing the char
> buffer would show: A02341023180002V
>
> Signed-off-by: Alberto Panizzo <alberto at amarulasolutions.com>

Reviewed-by: Philipp Tomsich <philipp.tomsich at theobroma-systems.com>

Other requested changes (on top of the architectural one from above) 
below.

> ---
> doc/README.rockusb             |  9 ++++++---
> drivers/usb/gadget/f_rockusb.c | 44 +++++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 49 insertions(+), 4 deletions(-)
>
> diff --git a/doc/README.rockusb b/doc/README.rockusb
> index 5405dc4..3a93edc 100644
> --- a/doc/README.rockusb
> +++ b/doc/README.rockusb
> @@ -42,9 +42,12 @@ see doc/README.rockchip for more detail about how to get U-Boot binary.
>
> sudo rkdeveloptool wl  64 <U-Boot binary>
>
> -There are plenty of Rockusb command. but wl(write lba) and
> -rd(reboot) command. These two command can let people flash
> -image to device.
> +Current set of rkdeveloptool commands supported:
> +- rci: Read Chip Info
> +- rfi: Read Flash Id
> +- rd : Reset Device
> +- td : Test Device Ready
> +- wl : Write blocks using LBA
>
> To do
> -----
> diff --git a/drivers/usb/gadget/f_rockusb.c b/drivers/usb/gadget/f_rockusb.c
> index 58e483c..0314ff0 100644
> --- a/drivers/usb/gadget/f_rockusb.c
> +++ b/drivers/usb/gadget/f_rockusb.c
> @@ -523,6 +523,48 @@ static void cb_read_storage_id(struct usb_ep *ep, struct usb_request *req)
> 	rockusb_tx_write_str(emmc_id);
> }
>
> +int __weak rk_get_bootrom_chip_version(unsigned int *chip_info, int size)
> +{
> +	return 0

The default implementation should return an appropriate error code.
Also, I would recommend to return the number of bytes used in the bootrom 
chip-version on the chip this is implemented for.

Documentation for the rk_get_bootrom_chip_version might be useful too 
(e.g. is 'size' a maximum buffer size that needs to be checked against)?

> +}
> +
> +static void cb_get_chip_version(struct usb_ep *ep, struct usb_request *req)
> +{
> +	ALLOC_CACHE_ALIGN_BUFFER(struct fsg_bulk_cb_wrap, cbw,
> +				 sizeof(struct fsg_bulk_cb_wrap));
> +	struct f_rockusb *f_rkusb = get_rkusb();
> +	unsigned int chip_info[4], i;
> +
> +	memset(chip_info, 0, sizeof(chip_info));

While it doesn't hurt, there's no need to perform a memset... you will 
call rk_get_bootrom_chip_version() next anyway.

> +	rk_get_bootrom_chip_version(chip_info, 4);

You should check the error code returned (e.g. in case that a CPU does not 
implement this).  Also: why is the 4 open-coded here and sizeof(chip_info) 
above?

> +
> +	/*
> +	 * Chip Version is a string saved in BOOTROM address space Little Endian
> +	 *
> +	 * Ex for rk3288: 0x33323041 0x32303134 0x30383133 0x56323030
> +	 * which brings:  320A20140813V200
> +	 *
> +	 * Note that memory version do invert MSB/LSB so printing the char
> +	 * buffer will show: A02341023180002V
> +	 */
> +	printf("read chip version: ");
> +	for (i = 0; i < 4; i++) {
> +		printf("%c%c%c%c",
> +		       (chip_info[i] >> 24) & 0xFF,
> +		       (chip_info[i] >> 16) & 0xFF,
> +		       (chip_info[i] >>  8) & 0xFF,
> +		       (chip_info[i] >>  0) & 0xFF);

I'd recommend to do first swap using the functions from 
include/linux/byteorder/generic.h and then just print as a string...

Why the printf? I think you want a debug() ...

> +	}
> +	printf("\n");

Same.

> +	memcpy((char *)cbw, req->buf, USB_BULK_CB_WRAP_LEN);
> +
> +	/* Prepare for sending subsequent CSW_GOOD */
> +	f_rkusb->tag = cbw->tag;
> +	f_rkusb->in_req->complete = tx_handler_send_csw;
> +
> +	rockusb_tx_write((char *)chip_info, sizeof(chip_info));
> +}
> +
> static void cb_write_lba(struct usb_ep *ep, struct usb_request *req)
> {
> 	ALLOC_CACHE_ALIGN_BUFFER(struct fsg_bulk_cb_wrap, cbw,
> @@ -661,7 +703,7 @@ static const struct cmd_dispatch_info cmd_dispatch_info[] = {
> 	},
> 	{
> 		.cmd = K_FW_GET_CHIP_VER,
> -		.cb = cb_not_support,
> +		.cb = cb_get_chip_version,
> 	},
> 	{
> 		.cmd = K_FW_LOW_FORMAT,
>