diff mbox

[v7,4/4] EXYNOS: SMDK5250: Add MMC SPL support

Message ID 1328173887-24983-5-git-send-email-chander.kashyap@linaro.org
State Superseded
Headers show

Commit Message

Chander Kashyap Feb. 2, 2012, 9:11 a.m. UTC
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

Comments

Mike Frysinger Feb. 2, 2012, 9:21 p.m. UTC | #1
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
Chander Kashyap Feb. 3, 2012, 12:23 p.m. UTC | #2
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
Doug Anderson Feb. 8, 2012, 11:35 p.m. UTC | #3
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
Mike Frysinger Feb. 9, 2012, 3:51 a.m. UTC | #4
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
Chander Kashyap Feb. 9, 2012, 5:25 a.m. UTC | #5
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
>
Mike Frysinger Feb. 9, 2012, 5:34 a.m. UTC | #6
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 mbox

Patch

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