[U-Boot] [PATCH v1 1/1] fastboot: Add support for flashing zImage

[Lukasz Majewski]

Hi Dileep,

> This patch adds support to flash zImage to the boot partition on eMMC.
> Usage: fastboot flash zImage <path_to_zImage>
> 
> Signed-off-by: Angela Stegmaier <angelabaker at ti.com>
> 
> Signed-off-by: Dileep Katta <dileep.katta at linaro.org>
> ---
>  drivers/usb/gadget/f_fastboot.c | 152
> ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 152
> insertions(+)
> 
> diff --git a/drivers/usb/gadget/f_fastboot.c
> b/drivers/usb/gadget/f_fastboot.c index 310175a..d3d16c0 100644
> --- a/drivers/usb/gadget/f_fastboot.c
> +++ b/drivers/usb/gadget/f_fastboot.c
> @@ -23,6 +23,7 @@
>  #ifdef CONFIG_FASTBOOT_FLASH_MMC_DEV
>  #include <fb_mmc.h>
>  #endif
> +#include <android_image.h>
>  
>  #define FASTBOOT_VERSION		"0.4"
>  
> @@ -492,6 +493,152 @@ static void cb_continue(struct usb_ep *ep,
> struct usb_request *req) }
>  
>  #ifdef CONFIG_FASTBOOT_FLASH
> +static int fastboot_update_zimage(void);
> +
> +static u32 fastboot_get_boot_ptn(struct andr_img_hdr *hdr, char
				^^^^^ could you explain this acronym?
	My gut feeling is that this function name could be better.
	As fair as I can tell you return number of header sectors.

> *response,
> +						block_dev_desc_t
> *dev_desc) +{
> +	u32 hdr_sectors = 0;
> +	u32 sector_size;
> +	int status = 0;
> +	strcpy(response, "OKAY");
> +	disk_partition_t info;
> +
> +	status = get_partition_info_efi_by_name(dev_desc, "boot",
> &info);
> +	if (status) {
> +		strcpy(response, "FAILCannot find boot partition");
> +		goto out;
> +	}
> +
> +	/* Read the boot image header */
> +	sector_size = info.blksz;
> +	hdr_sectors = (sizeof(struct andr_img_hdr)/sector_size) + 1;
		      ^^^ checkpatch.pl would complain about this line.

> +	status = dev_desc->block_read(dev_desc->dev, info.start,
> +				      hdr_sectors, (void *)hdr);
> +
> +	if (status < 0) {
> +		strcpy(response, "FAILCannot read hdr from boot
> partition");
> +		goto out;
> +	}
> +	if (android_image_check_header(hdr) != 0) {
> +		printf("bad boot image magic\n");
		^^^^^^ error()?

> +		strcpy(response, "FAILBoot partition not
> initialized");
> +		goto out;
> +	}
> +
> +	return hdr_sectors;
> +
> +out:
> +	strcpy(response, "INFO");
> +	fastboot_tx_write_str(response);
> +
> +	return -1;

	Please use appropriate -Exxxxx error code in the whole file.

> +}
> +
> +#define CEIL(a, b) (((a) / (b)) + ((a % b) > 0 ? 1 : 0))

Maybe you could add this code to ./include/common.h file, so it would
be reusable in the future?

> +
> +static int fastboot_update_zimage(void)
> +{
> +	struct andr_img_hdr *hdr = NULL;
> +	u8 *ramdisk_buffer;
> +	u32 ramdisk_sector_start, ramdisk_sectors;
> +	u32 kernel_sector_start, kernel_sectors;
> +	u32 hdr_sectors = 0;
> +	u32 sectors_per_page = 0;
> +	int ret = 0;
> +	block_dev_desc_t *dev_desc;
> +	disk_partition_t info;
> +	char response[RESPONSE_LEN];
		      ^^^^^^^^^^^^ this is defined as (64 + 1)
	Isn't there any change that such unaligned buffer would cause
	cache flush/invalidation related bugs?

	Please look into ALLOC_CACHE_ALIGN_BUFFER comment
	at ./include/common.h file

> +	u32 addr = CONFIG_USB_FASTBOOT_BUF_ADDR;
> +
> +	strcpy(response, "OKAY");
> +	printf("Flashing zImage...%d bytes\n", download_bytes);
> +
> +	dev_desc = get_dev("mmc", CONFIG_FASTBOOT_FLASH_MMC_DEV);
> +	if (!dev_desc || dev_desc->type == DEV_TYPE_UNKNOWN) {
> +		sprintf(response + strlen(response),
> +			"FAILInvalid mmc device");
> +		ret = -1;
> +		goto out;
> +	}
> +
> +	addr += CEIL(download_bytes, 0x1000) * 0x1000;
				     ^^^^^^ Please use define for this
				     magic number. It would be more
				     informative.
 
> +	hdr = (struct andr_img_hdr *)addr;
> +
> +	hdr_sectors = fastboot_get_boot_ptn(hdr, response, dev_desc);
					^^^ the comment the same as
					above.

> +	if (hdr_sectors <= 0) {
> +		sprintf(response + strlen(response),
> +			"FAILInvalid number of boot sectors %d",
> hdr_sectors);
> +		ret = -1;
> +		goto out;
> +	}
> +	ret = get_partition_info_efi_by_name(dev_desc, "boot",
> &info);
> +	if (ret) {
> +		strcpy(response, "FAILCannot find boot partition");
> +		ret = -1;
> +		goto out;
> +	}
> +
> +	/* Extract ramdisk location and read it into local buffer */
> +	sectors_per_page = hdr->page_size / info.blksz;
> +	ramdisk_sector_start = info.start + sectors_per_page;
> +	ramdisk_sector_start += CEIL(hdr->kernel_size,
> hdr->page_size)*
> +				     sectors_per_page;
> +	ramdisk_sectors = CEIL(hdr->ramdisk_size, hdr->page_size)*
> +			       sectors_per_page;
> +
> +	ramdisk_buffer = (u8 *)hdr;
> +	ramdisk_buffer += (hdr_sectors * info.blksz);
> +	ret = dev_desc->block_read(dev_desc->dev,
> ramdisk_sector_start,
> +				   ramdisk_sectors, ramdisk_buffer);
> +	if (ret < 0) {
> +		sprintf(response, "FAILCannot read ramdisk from
> 'boot'");
> +		ret = -1;
> +		goto out;
> +	}
> +
> +	/* Change the boot img hdr */
> +	hdr->kernel_size = download_bytes;
> +	ret = dev_desc->block_write(dev_desc->dev, info.start,
> +				    hdr_sectors, (void *)hdr);
> +	if (ret < 0) {
> +		sprintf(response, "FAILCannot writeback boot img
> hdr");
> +		ret = -1;
> +		goto out;
> +	}
> +
> +	/* Write the new downloaded kernel*/
> +	kernel_sector_start = info.start + sectors_per_page;
> +	kernel_sectors = CEIL(hdr->kernel_size, hdr->page_size)*
> +			      sectors_per_page;
> +	ret = dev_desc->block_write(dev_desc->dev,
> kernel_sector_start,
> +				    kernel_sectors,
> +				    (const void
> *)CONFIG_USB_FASTBOOT_BUF_ADDR);
> +	if (ret < 0) {
> +		sprintf(response, "FAILCannot write new kernel");
> +		ret = -1;
> +		goto out;
> +	}
> +
> +	/* Write the saved Ramdisk back */
> +	ramdisk_sector_start = info.start + sectors_per_page;
> +	ramdisk_sector_start += CEIL(hdr->kernel_size,
> hdr->page_size)*
> +				     sectors_per_page;
> +	ret = dev_desc->block_write(dev_desc->dev,
> ramdisk_sector_start,
> +				    ramdisk_sectors, ramdisk_buffer);
> +	if (ret < 0) {
> +		sprintf(response, "FAILCannot write back original
> ramdisk");
> +		ret = -1;
> +		goto out;
> +	}
> +	fastboot_tx_write_str(response);
> +	return 0;
> +
> +out:
> +	fastboot_tx_write_str(response);
> +	return ret;
> +}
> +
>  static void cb_flash(struct usb_ep *ep, struct usb_request *req)
>  {
>  	char *cmd = req->buf;
> @@ -505,6 +652,11 @@ static void cb_flash(struct usb_ep *ep, struct
> usb_request *req) }
>  
>  	strcpy(response, "FAILno flash device defined");
> +
> +	if (!strcmp(cmd, "zImage") || !strcmp(cmd, "zimage")) {

	Maybe you could use strcasecmp() function from ./lib/string.c?

> +		fastboot_update_zimage();
> +		return;
> +	}
>  #ifdef CONFIG_FASTBOOT_FLASH_MMC_DEV
>  	fb_mmc_flash_write(cmd, (void *)CONFIG_USB_FASTBOOT_BUF_ADDR,
>  			   download_bytes, response);



-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group