Message ID | 1328173887-24983-5-git-send-email-chander.kashyap@linaro.org |
---|---|
State | Superseded |
Headers | show |
On Thursday 02 February 2012 04:11:27 Chander Kashyap wrote: > --- /dev/null > +++ b/board/samsung/smdk5250/tools/mkexynos_image.c tools should be in tools/. there are already plenty of examples in there (see tools/msxboot.c as the first example i found by reading tools/Makefile). > +int main(int argc, char **argv) > +{ > ... > + unsigned char buffer[BUFSIZE] = {0}; this is an implicit memset() and from what i can see in the code, useless. you read() the entire buffer, so there's no need to initialize it. > + if (argc != 3) { > + printf(" %d Wrong number of arguments\n", argc); this should tell the user how to use the tool. fprintf(stderr, "Usage: %s <infile> <outfile>\n", argv[0]); > + if (ifd) > + close(ifd); this if() is wrong (0 is a valid fd) and useless (you already abort if ifd did not succeed). just delete the if statement. > + len = lseek(ifd, 0, SEEK_END); > + lseek(ifd, 0, SEEK_SET); lazy man's stat() :P. just use stat(). and change the type of "len" to "off_t". > + count = (len < CHECKSUM_OFFSET) ? len : CHECKSUM_OFFSET; > + > + if (read(ifd, buffer, count) != count) { count should be a ssize_t. although, this doesn't handle partial interrupted reads, so i wonder if this could shouldn't just be changed to use stdio fopen/fread. probably would be simpler that way too. > + if (ifd) > + close(ifd); > + if (ofd) > + close(ofd); these if checks are wrong for the same reason mentioned above > + unsigned long checksum = 0; > ... > + for (i = 0, checksum = 0; i < CHECKSUM_OFFSET; i++) > + checksum += buffer[i]; > + memcpy(&buffer[CHECKSUM_OFFSET], &checksum, sizeof(checksum)); pretty sure this fails if this tool is run on a big-endian machine, as well as 64bit vs 32bit. change the type of "checksum" to uint32_t, then use something like: put_unaligned_le32(checksum, &buffer[CHECKSUM_OFFSET]); > + if (write(ofd, buffer, BUFSIZE) != BUFSIZE) { same issues as the read() mentioned above > + if (ifd) > + close(ifd); > + if (ofd) > + close(ofd); same bad if() logic > + if (ifd) > + close(ifd); > + if (ofd) > + close(ofd); same bad if() logic -mike
Hi Mike, On 3 February 2012 02:51, Mike Frysinger <vapier@gentoo.org> wrote: > On Thursday 02 February 2012 04:11:27 Chander Kashyap wrote: >> --- /dev/null >> +++ b/board/samsung/smdk5250/tools/mkexynos_image.c > > tools should be in tools/. there are already plenty of examples in there (see > tools/msxboot.c as the first example i found by reading tools/Makefile). > >> +int main(int argc, char **argv) >> +{ >> ... >> + unsigned char buffer[BUFSIZE] = {0}; > > this is an implicit memset() and from what i can see in the code, useless. > you read() the entire buffer, so there's no need to initialize it. > >> + if (argc != 3) { >> + printf(" %d Wrong number of arguments\n", argc); > > this should tell the user how to use the tool. > fprintf(stderr, "Usage: %s <infile> <outfile>\n", argv[0]); Yes That's better. > >> + if (ifd) >> + close(ifd); > > this if() is wrong (0 is a valid fd) and useless (you already abort if ifd did > not succeed). just delete the if statement. Ah yes. I will fix it. > >> + len = lseek(ifd, 0, SEEK_END); >> + lseek(ifd, 0, SEEK_SET); > > lazy man's stat() :P. just use stat(). and change the type of "len" to > "off_t". > >> + count = (len < CHECKSUM_OFFSET) ? len : CHECKSUM_OFFSET; >> + >> + if (read(ifd, buffer, count) != count) { > > count should be a ssize_t. although, this doesn't handle partial interrupted > reads, so i wonder if this could shouldn't just be changed to use stdio > fopen/fread. probably would be simpler that way too. > >> + if (ifd) >> + close(ifd); >> + if (ofd) >> + close(ofd); > > these if checks are wrong for the same reason mentioned above > >> + unsigned long checksum = 0; >> ... >> + for (i = 0, checksum = 0; i < CHECKSUM_OFFSET; i++) >> + checksum += buffer[i]; >> + memcpy(&buffer[CHECKSUM_OFFSET], &checksum, sizeof(checksum)); > > pretty sure this fails if this tool is run on a big-endian machine, as well as > 64bit vs 32bit. change the type of "checksum" to uint32_t, then use something > like: > put_unaligned_le32(checksum, &buffer[CHECKSUM_OFFSET]); > Will take care of it. >> + if (write(ofd, buffer, BUFSIZE) != BUFSIZE) { > > same issues as the read() mentioned above > >> + if (ifd) >> + close(ifd); >> + if (ofd) >> + close(ofd); > > same bad if() logic > >> + if (ifd) >> + close(ifd); >> + if (ofd) >> + close(ofd); > > same bad if() logic > -mike
On Thu, Feb 2, 2012 at 1:21 PM, Mike Frysinger <vapier@gentoo.org> wrote: > On Thursday 02 February 2012 04:11:27 Chander Kashyap wrote: > > +int main(int argc, char **argv) > > +{ > > ... > > + unsigned char buffer[BUFSIZE] = {0}; > > this is an implicit memset() and from what i can see in the code, useless. > you read() the entire buffer, so there's no need to initialize it. Funny, I was just about to submit a patch to add this = {0} myself when I found this message. ;) I would say that it (or a memset, whichever people prefer) is a good idea so that this tool can be used to make a reasonable SPL out of any source binary executable, even ones that are smaller than 14K. Specifically, I assembled a bit of quick-and-dirty code for debugging (60 bytes) and wanted to convince the processor to load it as a BL2. This tool worked for the process, but it produced a file with some random data in it. Ick. I wouldn't hold up committing this patch series for it, though... -Doug
On Wednesday 08 February 2012 18:35:28 Doug Anderson wrote: > On Thu, Feb 2, 2012 at 1:21 PM, Mike Frysinger wrote: > > On Thursday 02 February 2012 04:11:27 Chander Kashyap wrote: > > > +int main(int argc, char **argv) > > > +{ > > > ... > > > + unsigned char buffer[BUFSIZE] = {0}; > > > > this is an implicit memset() and from what i can see in the code, > > useless. you read() the entire buffer, so there's no need to initialize > > it. > > Funny, I was just about to submit a patch to add this = {0} myself when I > found this message. ;) I would say that it (or a memset, whichever people > prefer) is a good idea so that this tool can be used to make a reasonable > SPL out of any source binary executable, even ones that are smaller than > 14K. you're right ... i'll claim that i was deceived by the lack of input checking. sounds like the code should be aborting if the input is too large instead of silently truncating. then the memset/{0} is unnecessary: - write out the data read - lseek to the checksum position - write checksum - ftruncate to total length (16KiB?) -mike
On 9 February 2012 09:21, Mike Frysinger <vapier@gentoo.org> wrote: > On Wednesday 08 February 2012 18:35:28 Doug Anderson wrote: >> On Thu, Feb 2, 2012 at 1:21 PM, Mike Frysinger wrote: >> > On Thursday 02 February 2012 04:11:27 Chander Kashyap wrote: >> > > +int main(int argc, char **argv) >> > > +{ >> > > ... >> > > + unsigned char buffer[BUFSIZE] = {0}; >> > >> > this is an implicit memset() and from what i can see in the code, >> > useless. you read() the entire buffer, so there's no need to initialize >> > it. >> >> Funny, I was just about to submit a patch to add this = {0} myself when I >> found this message. ;) I would say that it (or a memset, whichever people >> prefer) is a good idea so that this tool can be used to make a reasonable >> SPL out of any source binary executable, even ones that are smaller than >> 14K. > > you're right ... i'll claim that i was deceived by the lack of input checking. > sounds like the code should be aborting if the input is too large instead of > silently truncating. then the memset/{0} is unnecessary: > - write out the data read > - lseek to the checksum position > - write checksum > - ftruncate to total length (16KiB?) BUFSIZE is already made 14K, so no need to ftruncate. > -mike > > _______________________________________________ > Samsung mailing list > Samsung@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/samsung >
On Thursday 09 February 2012 00:25:35 Chander Kashyap wrote: > On 9 February 2012 09:21, Mike Frysinger wrote: > > On Wednesday 08 February 2012 18:35:28 Doug Anderson wrote: > >> On Thu, Feb 2, 2012 at 1:21 PM, Mike Frysinger wrote: > >> > On Thursday 02 February 2012 04:11:27 Chander Kashyap wrote: > >> > > +int main(int argc, char **argv) > >> > > +{ > >> > > ... > >> > > + unsigned char buffer[BUFSIZE] = {0}; > >> > > >> > this is an implicit memset() and from what i can see in the code, > >> > useless. you read() the entire buffer, so there's no need to > >> > initialize it. > >> > >> Funny, I was just about to submit a patch to add this = {0} myself when > >> I found this message. ;) I would say that it (or a memset, whichever > >> people prefer) is a good idea so that this tool can be used to make a > >> reasonable SPL out of any source binary executable, even ones that are > >> smaller than 14K. > > > > you're right ... i'll claim that i was deceived by the lack of input > > checking. sounds like the code should be aborting if the input is too > > large instead of silently truncating. then the memset/{0} is > > unnecessary: > > - write out the data read > > - lseek to the checksum position > > - write checksum > > - ftruncate to total length (16KiB?) > > BUFSIZE is already made 14K, so no need to ftruncate. yes, in v9, it's 14KiB. i was looking at v7 which used 16KiB. -mike
diff --git a/board/samsung/smdk5250/Makefile b/board/samsung/smdk5250/Makefile index 80e8be3..9bba6e7 100644 --- a/board/samsung/smdk5250/Makefile +++ b/board/samsung/smdk5250/Makefile @@ -29,18 +29,34 @@ SOBJS := lowlevel_init.o COBJS := clock_init.o COBJS += dmc_init.o COBJS += tzpc_init.o + +ifndef CONFIG_SPL_BUILD COBJS += smdk5250.o +endif + +ifdef CONFIG_SPL_BUILD +COBJS += mmc_boot.o +endif SRCS := $(SOBJS:.o=.S) $(COBJS:.o=.c) OBJS := $(addprefix $(obj),$(COBJS) $(SOBJS)) ALL := $(obj).depend $(LIB) +ifdef CONFIG_SPL_BUILD +ALL += $(OBJTREE)/tools/mk$(BOARD)spl +endif + all: $(ALL) $(LIB): $(OBJS) $(call cmd_link_o_target, $(OBJS)) +ifdef CONFIG_SPL_BUILD +$(OBJTREE)/tools/mk$(BOARD)spl: tools/mkexynos_image.c + $(HOSTCC) tools/mkexynos_image.c -o $(OBJTREE)/tools/mk$(BOARD)spl +endif + ######################################################################### # defines $(obj).depend target diff --git a/board/samsung/smdk5250/mmc_boot.c b/board/samsung/smdk5250/mmc_boot.c new file mode 100644 index 0000000..449a919 --- /dev/null +++ b/board/samsung/smdk5250/mmc_boot.c @@ -0,0 +1,58 @@ +/* + * Copyright (C) 2012 Samsung Electronics + * + * 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 + */ + +#include<common.h> +#include<config.h> + +/* +* Copy U-boot from mmc to RAM: +* COPY_BL2_FNPTR_ADDR: Address in iRAM, which Contains +* Pointer to API (Data transfer from mmc to ram) +*/ +void copy_uboot_to_ram(void) +{ + u32 (*copy_bl2)(u32, u32, u32) = (void *) *(u32 *)COPY_BL2_FNPTR_ADDR; + + copy_bl2(BL2_START_OFFSET, BL2_SIZE_BLOC_COUNT, CONFIG_SYS_TEXT_BASE); +} + +void board_init_f(unsigned long bootflag) +{ + __attribute__((noreturn)) void (*uboot)(void); + copy_uboot_to_ram(); + + /* Jump to U-Boot image */ + uboot = (void *)CONFIG_SYS_TEXT_BASE; + (*uboot)(); + /* Never returns Here */ +} + +/* Place Holders */ +void board_init_r(gd_t *id, ulong dest_addr) +{ + /* Function attribute is no-return */ + /* This Function never executes */ + while (1) + ; +} + +void save_boot_params(u32 r0, u32 r1, u32 r2, u32 r3) {} diff --git a/board/samsung/smdk5250/tools/mkexynos_image.c b/board/samsung/smdk5250/tools/mkexynos_image.c new file mode 100644 index 0000000..3159e7c --- /dev/null +++ b/board/samsung/smdk5250/tools/mkexynos_image.c @@ -0,0 +1,117 @@ +/* + * Copyright (C) 2012 Samsung Electronics + * + * 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 + */ + +#include <stdio.h> +#include <stdlib.h> +#include <unistd.h> +#include <fcntl.h> +#include <errno.h> +#include <string.h> +#include <sys/stat.h> + +#define CHECKSUM_OFFSET (14*1024-4) +#define BUFSIZE (16*1024) +#define FILE_PERM (S_IRUSR | S_IWUSR | S_IRGRP \ + | S_IWGRP | S_IROTH | S_IWOTH) +/* +* Requirement: +* IROM code reads first 14K bytes from boot device. +* It then calculates the checksum of 14K-4 bytes and compare with data at +* 14K-4 offset. +* +* This function takes two filenames: +* IN "u-boot-spl.bin" and +* OUT "$(BOARD)-spl.bin as filenames. +* It reads the "u-boot-spl.bin" in 16K buffer. +* It calculates checksum of 14K-4 Bytes and stores at 14K-4 offset in buffer. +* It writes the buffer to "$(BOARD)-spl.bin" file. +*/ + +int main(int argc, char **argv) +{ + int i, len; + unsigned char buffer[BUFSIZE] = {0}; + int ifd, ofd; + unsigned long checksum = 0, count; + + if (argc != 3) { + printf(" %d Wrong number of arguments\n", argc); + exit(EXIT_FAILURE); + } + + ifd = open(argv[1], O_RDONLY); + if (ifd < 0) { + fprintf(stderr, "%s: Can't open %s: %s\n", + argv[0], argv[1], strerror(errno)); + exit(EXIT_FAILURE); + } + + ofd = open(argv[2], O_WRONLY | O_CREAT | O_TRUNC, FILE_PERM); + if (ifd < 0) { + fprintf(stderr, "%s: Can't open %s: %s\n", + argv[0], argv[2], strerror(errno)); + if (ifd) + close(ifd); + exit(EXIT_FAILURE); + } + + len = lseek(ifd, 0, SEEK_END); + lseek(ifd, 0, SEEK_SET); + + count = (len < CHECKSUM_OFFSET) ? len : CHECKSUM_OFFSET; + + if (read(ifd, buffer, count) != count) { + fprintf(stderr, "%s: Can't read %s: %s\n", + argv[0], argv[1], strerror(errno)); + + if (ifd) + close(ifd); + if (ofd) + close(ofd); + + exit(EXIT_FAILURE); + } + + for (i = 0, checksum = 0; i < CHECKSUM_OFFSET; i++) + checksum += buffer[i]; + + memcpy(&buffer[CHECKSUM_OFFSET], &checksum, sizeof(checksum)); + + if (write(ofd, buffer, BUFSIZE) != BUFSIZE) { + fprintf(stderr, "%s: Can't write %s: %s\n", + argv[0], argv[2], strerror(errno)); + + if (ifd) + close(ifd); + if (ofd) + close(ofd); + + exit(EXIT_FAILURE); + } + + if (ifd) + close(ifd); + if (ofd) + close(ofd); + + return EXIT_SUCCESS; +} diff --git a/include/configs/smdk5250.h b/include/configs/smdk5250.h index 67e4012..f54d7ac 100644 --- a/include/configs/smdk5250.h +++ b/include/configs/smdk5250.h @@ -102,6 +102,10 @@ #define CONFIG_BOOTDELAY 3 #define CONFIG_ZERO_BOOTDELAY_CHECK +/* MMC SPL */ +#define CONFIG_SPL +#define COPY_BL2_FNPTR_ADDR 0x02020030 + #define CONFIG_BOOTCOMMAND "mmc read 40007000 451 2000; bootm 40007000" /* Miscellaneous configurable options */ @@ -178,6 +182,9 @@ #define CONFIG_BL2_OFFSET (CONFIG_BL1_OFFSET + CONFIG_BL1_SIZE) #define CONFIG_ENV_OFFSET (CONFIG_BL2_OFFSET + CONFIG_BL2_SIZE) +/* U-boot copy size from boot Media to DRAM.*/ +#define BL2_START_OFFSET (CONFIG_BL2_OFFSET/512) +#define BL2_SIZE_BLOC_COUNT (CONFIG_BL2_SIZE/512) #define CONFIG_DOS_PARTITION #define CONFIG_IRAM_STACK 0x02050000
This patch adds support for MMC SPL booting. Signed-off-by: Chander Kashyap <chander.kashyap@linaro.org> --- Changes for v2: - None Changes for v3: - None Changes for v4: - None Changes for v5: - None Changes for v6: - None Changes for v7: - None board/samsung/smdk5250/Makefile | 16 ++++ board/samsung/smdk5250/mmc_boot.c | 58 ++++++++++++ board/samsung/smdk5250/tools/mkexynos_image.c | 117 +++++++++++++++++++++++++ include/configs/smdk5250.h | 7 ++ 4 files changed, 198 insertions(+), 0 deletions(-) create mode 100644 board/samsung/smdk5250/mmc_boot.c create mode 100644 board/samsung/smdk5250/tools/mkexynos_image.c