[U-Boot] [PATCH] RISCV: image: Add booti support.

[Atish Patra <atish.patra@wdc.com>]

On 4/23/19 5:16 PM, Marek Vasut wrote:
> On 4/24/19 1:36 AM, Atish Patra wrote:
>> This patch adds booti support for RISC-V Linux kernel. The existing
>> bootm method will also continue to work as it is.
>>
>> It depends on the following kernel patch which adds the header to the
>> flat Image.
>>
>> https://patchwork.kernel.org/patch/10913869/
>>
>> Tested on HiFive Unleashed and QEMU.
>> Currently, compressed images such as Image.gz are not supported.
>>
>> Signed-off-by: Atish Patra <atish.patra@wdc.com>
>> ---
>>   arch/riscv/lib/Makefile |  1 +
>>   arch/riscv/lib/image.c  | 60 +++++++++++++++++++++++++++++++++++++++++
>>   cmd/Kconfig             |  2 +-
>>   cmd/booti.c             |  8 ++++--
>>   4 files changed, 68 insertions(+), 3 deletions(-)
>>   create mode 100644 arch/riscv/lib/image.c
>>
>> diff --git a/arch/riscv/lib/Makefile b/arch/riscv/lib/Makefile
>> index 1c332db436a9..6ae6ebbeafda 100644
>> --- a/arch/riscv/lib/Makefile
>> +++ b/arch/riscv/lib/Makefile
>> @@ -7,6 +7,7 @@
>>   # Rick Chen, Andes Technology Corporation <rick@andestech.com>
>>   
>>   obj-$(CONFIG_CMD_BOOTM) += bootm.o
>> +obj-$(CONFIG_CMD_BOOTI) += bootm.o image.o
>>   obj-$(CONFIG_CMD_GO) += boot.o
>>   obj-y	+= cache.o
>>   obj-$(CONFIG_RISCV_RDTIME) += rdtime.o
>> diff --git a/arch/riscv/lib/image.c b/arch/riscv/lib/image.c
>> new file mode 100644
>> index 000000000000..99c2e31066a3
>> --- /dev/null
>> +++ b/arch/riscv/lib/image.c
>> @@ -0,0 +1,60 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * Copyright (C) 2019 Western Digital Corporation or its affiliates.
>> + * Authors:
>> + *	Atish Patra <atish.patra@wdc.com>
>> + * Based on arm/lib/image.c
>> + */
>> +
>> +#include <common.h>
>> +#include <mapmem.h>
>> +#include <linux/sizes.h>
>> +#include <linux/stddef.h>
>> +
>> +DECLARE_GLOBAL_DATA_PTR;
>> +
>> +/* ASCII version of "RISCV" defined in Linux kernel */
>> +#define LINUX_RISCV_IMAGE_MAGIC 0x5643534952
>> +
>> +struct linux_image_h {
>> +	uint32_t	code0;		/* Executable code */
>> +	uint32_t	code1;		/* Executable code */
>> +	uint64_t	text_offset;	/* Image load offset */
>> +	uint64_t	image_size;	/* Effective Image size */
>> +	uint64_t	res1;		/* reserved */
>> +	uint64_t	magic;		/* Magic number */
>> +	uint32_t	res2;		/* reserved */
>> +	uint32_t	res3;		/* reserved */
>> +};
>> +
>> +int booti_setup(ulong image, ulong *relocated_addr, ulong *size,
>> +		bool force_reloc)
>> +{
>> +	struct linux_image_h *lhdr;
>> +	uint64_t dst;
>> +	uint64_t image_size, text_offset;
>> +
>> +	*relocated_addr = image;
> 
> You're setting this here to $image, but you're overriding this at the
> end of the function again with gd->ram_base + text_offset , is that
> intended?
> 

My bad. I will remove it.

>> +	lhdr = (struct linux_image_h *)map_sysmem(image, 0);
>> +
>> +	if (lhdr->magic != LINUX_RISCV_IMAGE_MAGIC) {
>> +		puts("Bad Linux RISCV Image magic!\n");
>> +		return 1;
>> +	}
>> +
>> +	if (lhdr->image_size != 0) {
>> +		image_size = lhdr->image_size;
>> +		text_offset = lhdr->text_offset;
> 
> Maybe you can use lhdr->* directly and get rid of these local variables ?
> 

Done.

>> +	} else {
>> +		puts("Image lacks image_size field, Error!!\n");
> 
> error, lowercase . And use one exclamation mark.
> 
>> +		return 1;
> 
> Use errno.h return code instead.
> 
>> +	}
>> +	*size = image_size;
>> +	dst = gd->ram_base;
>> +	*relocated_addr = dst + text_offset;
> 
> Use gd->ram_base instead of $dst and drop $dst altogether.
> 

I have addressed all the comments. I will send a v2 soon.

Regards,
Atish
>> +
> [...]
>