[U-Boot] [PATCH] powerpc/CoreNet: add tool to support pbl image build.
Xie Shaohui-B21989
B21989 at freescale.com
Tue Jun 5 07:35:57 CEST 2012
>-----Original Message-----
>From: Wood Scott-B07421
>Sent: Tuesday, June 05, 2012 8:33 AM
>To: Xie Shaohui-B21989
>Cc: u-boot at lists.denx.de; Tabi Timur-B04825
>Subject: Re: [U-Boot] [PATCH] powerpc/CoreNet: add tool to support pbl
>image build.
>
>On 06/04/2012 03:57 AM, Shaohui Xie wrote:
>> From: Shaohui Xie <b21989 at freescale.com>
>>
>> Provides a tool to build boot Image for PBL(Pre boot loader) which is
>> used on Freescale CoreNet SoCs, PBL can be used to load some
>> instructions and/or data for pre-initialization. The default output
>> image is u-boot.pbl, for more details please refer to
>doc/README.pblimage.
>>
>> Signed-off-by: Shaohui Xie <b21989 at freescale.com>
>> ---
>> rebased to lasted tree.
>>
>> Makefile | 5 +
>> board/freescale/corenet_ds/config.mk | 26 +++
>> board/freescale/corenet_ds/pblimage.cfg | 60 ++++++
>> common/image.c | 1 +
>> doc/README.pblimage | 140 +++++++++++++
>> include/image.h | 1 +
>> tools/Makefile | 2 +
>> tools/mkimage.c | 5 +
>> tools/mkimage.h | 2 +
>> tools/pblimage.c | 329
>+++++++++++++++++++++++++++++++
>> tools/pblimage.h | 36 ++++
>> 11 files changed, 607 insertions(+), 0 deletions(-) create mode
>> 100644 board/freescale/corenet_ds/config.mk
>> create mode 100644 board/freescale/corenet_ds/pblimage.cfg
>> create mode 100644 doc/README.pblimage create mode 100644
>> tools/pblimage.c create mode 100644 tools/pblimage.h
>>
>> diff --git a/Makefile b/Makefile
>> index 57ad45b..99f993a 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -416,6 +416,10 @@ $(obj)u-boot.kwb: $(obj)u-boot.bin
>> $(obj)tools/mkimage -n $(CONFIG_SYS_KWD_CONFIG) -T kwbimage \
>> -a $(CONFIG_SYS_TEXT_BASE) -e $(CONFIG_SYS_TEXT_BASE) -d $< $@
>*>
>> +$(obj)u-boot.pbl: $(obj)u-boot.bin
>> + $(obj)tools/mkimage -n $(CONFIG_PBL_CONFIG) -T pblimage \
>> + -d $< $@
>> +
>> $(obj)u-boot.sha1: $(obj)u-boot.bin
>> $(obj)tools/ubsha1 $(obj)u-boot.bin
>>
>> @@ -773,6 +777,7 @@ clobber: tidy
>> $(obj)cscope.* $(obj)*.*~
>> @rm -f $(obj)u-boot $(obj)u-boot.map $(obj)u-boot.hex $(ALL-y)
>> @rm -f $(obj)u-boot.kwb
>> + @rm -f $(obj)u-boot.pbl
>> @rm -f $(obj)u-boot.imx
>> @rm -f $(obj)u-boot.ubl
>> @rm -f $(obj)u-boot.ais
>> diff --git a/board/freescale/corenet_ds/config.mk
>> b/board/freescale/corenet_ds/config.mk
>> new file mode 100644
>> index 0000000..72464dc
>> --- /dev/null
>> +++ b/board/freescale/corenet_ds/config.mk
>> @@ -0,0 +1,26 @@
>> +#
>> +
>> +#PBL preamble and RCW header
>> +aa55aa55 010e0100
>> +#64 bytes RCW data for P4080, replace it when building image #for
>> +P3041DS or P5020DS.
>> +4c580000 00000000 18185218 0000cccc
>> +40464000 3c3c2000 58000000 61000000
>> +00000000 00000000 00000000 008b6000
>> +00000000 00000000 00000000 00000000
>
>Could you have the tool source this from a separate file, rather than
>require the user to replace it manually?
[Xie Shaohui] Then I have to prepare a separate file and a tool... It is quite simple to replace, just copy and paste, and users may need to modify the RCW when the default one does not fit their use case, they will always have to do it manually. It's simple to do it here.
>
>Talk to Timur (when he gets back from vacation in a couple weeks) about
>his RCW tool and how best to accept the output it produces.
>
>Why is eSPI in here? Isn't this supposed to just generically write an
>image into CPC SRAM?
[Xie Shaohui] No. some interfaces need to be pre-initialized before PBL start to load stuff from it, and default configurations for SPI is suitable, this tool provides a more compatible configurations.
>
>> diff --git a/doc/README.pblimage b/doc/README.pblimage new file mode
>> 100644 index 0000000..73d90f1
>> --- /dev/null
>> +++ b/doc/README.pblimage
>> @@ -0,0 +1,140 @@
>> + 3). Boot from Nand
>> + Write u-boot.pbl to Nand from offset 0x0, Note that in case of eLBC
>NAND
>> + flash, the address starts from the first good block.
>> + for ex in u-boot:
>> + =>tftp 100000 u-boot.pbl
>> + =>nand info
>> + =>nand erase 0 100000
>> + =>nand write 100000 0 $filesize
>> + Change SW1[1:5] = off on off off on
>> + Change SW7[1:4] = on off off on
>
>How do you load the environment? We should find a way, possibly using SPL,
>to have the environment ready early. We do not want to wait post
>relocation for the full NAND/SD/SPI driver to load the environment.
[Xie Shaohui] This tool did not intend to provide a way to load the environment. The ENV thing should belong to the boot Image, this tool is a wrapper.
>
>> diff --git a/tools/mkimage.h b/tools/mkimage.h index 5fe1a48..a886305
>> 100644
>> --- a/tools/mkimage.h
>> +++ b/tools/mkimage.h
>> @@ -147,6 +147,8 @@ void mkimage_register (struct image_type_params
>*tparams);
>> *
>> * Supported image types init functions
>> */
>> +void pbl_load_uboot(int, struct mkimage_params *);
>
>Please provide names for parameters.
[Xie Shaohui] OK.
>
>> +/*
>> + * PBL can load 64 bytes each time in MAX, so the u-boot need to be
>> +splited into
>> + * pieces of 64 bytes, PBL needs a command for each piece, the
>> +command looks
>> + * like 81xxxxxx, the "xxxxxx" is offset, it starts from F80000 in our
>case.
>> + */
>
>Better wording:
>
> The PBL can load up to 64 bytes at a time, so we split the U-Boot
> image into 64 byte chunks. PBL needs a command for each piece, of
> the form "81xxxxxx", where "xxxxxx" is the offset.
[Xie Shaohui] OK. Thanks.
>
>Why are we hardcoding 0xf80000? Where does it come from?
[Xie Shaohui] it's from CONFIG_SYS_TEXT_BASE. I hardcoded it for simple coding.
>
>> +static unsigned int uboot_label = 0x81F80000;
>
>I'm not sure that label is the right word here. Maybe "next_pbl_cmd"?
[Xie Shaohui] Right. "next_pbl_cmd" is better.
>
>> +/*
>> + * need to store all bytes in memory for calculating crc32, then
>> +write the
>> + * bytes to image file for PBL boot.
>> + */
>> +static unsigned char mem_buf[600000];
>
>600000 is arbitrary. Do you have any checking if the image size exceeds
>this? Can you dynamically allocate the buffer instead?
[Xie Shaohui] Actually, I thought of dynamically allocate the buffer at beginning, but I found at least I need to allocate a buffer to store the parsed result of the configure file, but we won't know the size until we parsed the file, so I decided to use an arbitrary size, I thought 600k is big enough.
>
>> +static unsigned char *pmem_buf = mem_buf; static int mem_byte_cnt;
>
>s/mem_byte_cnt/pbl_size/
[Xie Shaohui] OK.
>
>> +static char *fname = "Unknown";
>> +static int lineno = -1;
>> +static struct pbl_header pblimage_header;
>> +
>> +static union
>> +{
>> + char c[4];
>> + unsigned char l;
>> +} endian_test = { {'l', '?', '?', 'b'} };
>
>Please fix whitespace.
[Xie Shaohui] OK. Thanks. I did not find it when using checkpatch.pl
>
>> +#define ENDIANNESS ((char)endian_test.l)
>> +
>> +static void generate_pbl_cmd(void)
>> +{
>> + unsigned int val = uboot_label;
>
>s/unsigned int/uint32_t/
[Xie Shaohui] OK.
>
>> + uboot_label += 0x40;
>> + int i;
>> +
>> + for (i = 3; i >= 0; i--) {
>> + *pmem_buf++ = (val >> (i * 8)) & 0xff;
>> + mem_byte_cnt++;
>> + }
>> +}
>> +
>> +static void pbl_fget(size_t size, FILE *stream) {
>> + unsigned char c;
>> + while (size) {
>> + c = (unsigned char)fgetc(stream);
>
>Check for fgetc() returning EOF before converting to unsigned char.
[Xie Shaohui] OK.
>
>> +/* load splited u-boot with PBI command 81xxxxxx. */
>
>s/splited/split/
[Xie Shaohui] OK.
>
>> +static void pbl_parser(char *name)
>> +{
>> + FILE *fd = NULL;
>> + char *line = NULL;
>> + char *token, *saveptr1, *saveptr2;
>> + size_t len = 0;
>> +
>> + fname = name;
>> + fd = fopen(name, "r");
>> + if (fd == NULL) {
>> + printf("Error:%s - Can't open\n", fname);
>> + exit(EXIT_FAILURE);
>> + }
>> +
>> + while ((getline(&line, &len, fd)) > 0) {
>> + lineno++;
>> + token = strtok_r(line, "\r\n", &saveptr1);
>
>Why do you need to tokenize if you're using getline()?
[Xie Shaohui] I need to drop empty lines.
>
>> +static uint32_t crc_table[256];
>> +
>> +static void make_crc_table(void)
>> +{
>> + uint32_t mask;
>> + int i, j;
>> + uint32_t poly; /* polynomial exclusive-or pattern */
>> +
>> + /*
>> + * the polynomial used by PBL is 1 + x1 + x2 + x4 + x5 + x7 + x8 +
>x10
>> + * + x11 + x12 + x16 + x22 + x23 + x26 + x32.
>> + */
>> + poly = 0x04c11db7;
>> +
>> + for (i = 0; i < 256; i++) {
>> + mask = i << 24;
>> + for (j = 0; j < 8; j++) {
>> + if (mask & 0x80000000)
>> + mask = (mask << 1) ^ poly;
>> + else
>> + mask <<= 1;
>> + }
>> + crc_table[i] = mask;
>> + }
>> +}
>> +
>> +unsigned long pbl_crc32(unsigned long crc, const char *buf, unsigned
>> +int len) {
>> + unsigned int crc32_val = 0xffffffff;
>> + unsigned int xor = 0x0;
>> + int i;
>> +
>> + make_crc_table();
>> +
>> + for (i = 0; i < len; i++)
>> + crc32_val = (crc32_val << 8) ^
>> + crc_table[(crc32_val >> 24) ^ (*buf++ & 0xff)];
>> +
>> + crc32_val = crc32_val ^ xor;
>> + if (crc32_val < 0) {
>> + crc32_val += 0xffffffff;
>> + crc32_val += 1;
>> + }
>> + return crc32_val;
>> +}
>
>Is this the standard CRC32 function (which we should be able to pull in
>from elsewhere instead of reimplementing here) or is it something
>different (which deserves a comment)?
[Xie Shaohui] This not standard CRC32 function, I tried use the one which u-boot provides, but it did not work with PBL, PBL uses a different algorithm. I've indicated it in make_crc_table() above.
>
>> +static unsigned int reverse_byte(unsigned int val) {
>> + unsigned int temp;
>> + unsigned char *p1;
>> + int j;
>> +
>> + temp = val;
>> + p1 = (unsigned char *)&temp;
>> + for (j = 3; j >= 0; j--)
>> + *p1++ = (val >> (j * 8)) & 0xff;
>> + return temp;
>> +}
>> +
>> +/* write end command and crc command to memory. */ static void
>> +add_end_cmd(void) {
>> + unsigned int pbl_end_cmd[4] = {0x09138000, 0x00000000,
>> + 0x091380c0, 0x00000000};
>
>s/unsigned int/uint32_t/
[Xie Shaohui] OK.
>
>> + unsigned int crc32_pbl;
>> + int i;
>> + unsigned char *p = (unsigned char *)&pbl_end_cmd;
>> +
>> + if (ENDIANNESS == 'l') {
>> + for (i = 0; i < 4; i++)
>> + pbl_end_cmd[i] = reverse_byte(pbl_end_cmd[i]);
>> + }
>
>Can you use htonl() or similar, to avoid the endian test? Or have a
>function to write out a 32-bit word by bytes, similar to what you open-
>code in generate_pbl_cmd().
[Xie Shaohui] OK. I'll have a try.
>
>
>> + for (i = 0; i < 16; i++) {
>> + *pmem_buf++ = *p++;
>> + mem_byte_cnt++;
>> + }
>
>memcpy()?
[Xie Shaohui] memcpy() works here, but also need to move pointer of *pmem_buf and calculate the mem_byte_cnt. So I used this one straight.
>
>> + if (((mem_byte_cnt) % 16) != 0) {
>
>Unnecessary parentheses.
[Xie Shaohui] OK.
>
>> + for (i = 0; i < 8; i++) {
>> + *pmem_buf++ = 0x0;
>> + mem_byte_cnt++;
>> + }
>> + }
>> + if ((mem_byte_cnt % 16 != 0)) {
>> + printf("Error: Bad size of image file\n");
>> + exit(EXIT_FAILURE);
>> + }
>> +
>> +}
>
>No blank line at end of block.
[Xie Shaohui] OK.
>
>> diff --git a/tools/pblimage.h b/tools/pblimage.h new file mode 100644
>> index 0000000..887d4c9
>> --- /dev/null
>> +++ b/tools/pblimage.h
>> @@ -0,0 +1,36 @@
>> +/*
>> + * Copyright 2012 Freescale Semiconductor, Inc.
>> + *
>> + * See file CREDITS for list of people who contributed to this
>> + * project.
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License as
>> + * published by the Free Software Foundation; either version 2 of
>> + * the License, or (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program; if not, write to the Free Software
>> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
>> + * MA 02111-1307 USA
>> + */
>> +
>> +#ifndef _PBLIMAGE_H_
>> +#define _PBLIMAGE_H_
>
>s/_PBLIMAGE_H_/PBLIMAGE_H/
>
>Leading underscores are reserved to the C implementation when followed by
>an uppercase letter or another underscore, or when in file scope.
>
>I know most of the other tools/ headers do this, but it's wrong.
[Xie Shaohui] OK. Thanks for reviewing this patch.
Best Regards,
Shaohui Xie
More information about the U-Boot
mailing list