preparation to intriduce keystone support

This patch add support for gpimage format as a preparatory patch for porting u-boot for keystone2 devices and is based on omapimage format. It re-uses gph header to store the size and loadaddr as done in omapimage.c

Dear Murali Karicheri,

Quoted 4 lines · In message <[email protected]> you wrote:

This patch add support for gpimage format as a preparatory patch for porting u-boot for keystone2 devices and is based on omapimage format. It re-uses gph header to store the size and loadaddr as done in omapimage.c

...

common/image.c static const table_entry_t uimage_type[] = { · line 141

Quoted 8 lines

common/image.c +1 -0

@@ -137,6 +137,7 @@ static const table_entry_t uimage_type[] = {
 	{	IH_TYPE_STANDALONE, "standalone", "Standalone Program", },
 	{	IH_TYPE_UBLIMAGE,   "ublimage",   "Davinci UBL image",},
 	{	IH_TYPE_MXSIMAGE,   "mxsimage",   "Freescale MXS Boot Image",},
+	{	IH_TYPE_GPIMAGE,    "gpimage",    "TI KeyStone SPL Image",},
 	{	-1,		    "",		  "",			},

Please keep the list sorted. While you are at it, please also fi the incorrect placement of the IH_TYPE_MXSIMAGE entry.

tools/Makefile NOPED_OBJ_FILES-y += imagetool.o · line 90

Quoted 10 lines

tools/Makefile +2 -0

@@ -84,7 +84,9 @@ NOPED_OBJ_FILES-y += imagetool.o
 NOPED_OBJ_FILES-y += mkenvimage.o
 NOPED_OBJ_FILES-y += mkimage.o
 NOPED_OBJ_FILES-y += mxsimage.o
+NOPED_OBJ_FILES-y += gpimage-common.o
 NOPED_OBJ_FILES-y += omapimage.o
+NOPED_OBJ_FILES-y += gpimage.o
 NOPED_OBJ_FILES-y += os_support.o

Please keep the list sorted.

tools/Makefile $(obj)dumpimage$(SFX): $(obj)aisimage.o \ · line 226

Quoted 7 lines

tools/Makefile +2 -0

@@ -219,7 +221,9 @@ $(obj)dumpimage$(SFX):	$(obj)aisimage.o \
 			$(obj)dumpimage.o \
 			$(obj)md5.o \
 			$(obj)mxsimage.o \
+			$(obj)gpimage-common.o \
 			$(obj)omapimage.o \
+			$(obj)gpimage.o \

Ditto. Please fix this issue globally.

tools/Makefile $(obj)dumpimage$(SFX): $(obj)aisimage.o \

Quoted 4 lines

tools/Makefile +4 -0

+uint32_t gpimage_swap32(uint32_t data)
+{
+	return cpu_to_be32(data);
+}

The function name is misleading. Also it is not clear why you need this function at all. You can use cpu_to_be32() directly - that will make the code much easier to read.

tools/Makefile

Quoted 10 lines

tools/Makefile +10 -0

+/* TODO: do we need the below 2 functions?? */
+int valid_gph_size(uint32_t size)
+{
+	return size;
+}
+
+int valid_gph_load_addr(uint32_t load_addr)
+{
+	return load_addr;
+}

These functions are obviously bogus. Please dump them.

tools/Makefile

Quoted 11 lines

tools/Makefile +11 -0

+int gph_verify_header(struct gp_header *gph, int do_swap32)
+{
+	uint32_t gph_size, gph_load_addr;
+
+	if (do_swap32) {
+		gph_size = gpimage_swap32(gph->size);
+		gph_load_addr = gpimage_swap32(gph->load_addr);
+	} else {
+		gph_size = gph->size;
+		gph_load_addr = gph->load_addr;
+	}

I think it should be possible top write this code in such a way that you can avoid both the if-else and passing the do_swap32 parameter. It is my impression that the whole endianess handling needs some refinemant.

Actually I cannot see a place where do_swap32=0 is used..

Quoted 13 lines

tools/gpimage-common.c line 52

Quoted 8 lines

tools/gpimage-common.c new file +7 -0

+	if (do_swap32) {
+		gph_size = gpimage_swap32(gph->size);
+		gph_load_addr = gpimage_swap32(gph->load_addr);
+	} else {
+		gph_size = gph->size;
+		gph_load_addr = gph->load_addr;
+	}

I think it should be possible top write this code in such a way that you can avoid both the if- else and passing the do_swap32 parameter.
It is my impression that the whole endianess handling needs some refinemant.

Quoted 1 line

Actually I cannot see a place where do_swap32=0 is used..

This is done to keep the omapimage usage of this variable.

Dear Murali,

In message <[email protected]> you

Quoted 10 lines · wrote:

Quoted 10 lines

Quoted 4 lines

(quoted diff) +1 -0

+	if (do_swap32) { +		gph_size = gpimage_swap32(gph->size); +

gph_load_addr = gpimage_swap32(gph->load_addr); + } else { +
gph_size = gph->size; + gph_load_addr = gph->load_addr; + }

I think it should be possible top write this code in such a way that you can avoid both the if- else and passing the do_swap32 parameter. It is my impression that the whole endianess handling needs some refinemant.

Quoted 3 lines

Quoted 1 line

Actually I cannot see a place where do_swap32=0 is used..

This is done to keep the omapimage usage of this variable.

Is this actually used? It appear the omapimage handling would set do_swap32=1 only when imagename == "byteswap" (see "tools/omapimage.c"). Is this a practical usecase anywhere?

Fix the following checkpatch warning:-

WARNING: externs should be avoided in .c files

Signed-off-by Murali Karicheri <[email protected]>

This patch extends the ubifs load implementation to return the size of the file loaded from the filesystem. The ubifs command implementation has also been modified to set the filesize environment variable accordingly.

Signed-off-by Vitaly Andrianov <[email protected]>

Signed-off-by Murali Karicheri <[email protected]>

This patch add basic support for the architecture timer found on recent ARMv7 based SoCs.

arch/arm/lib/arch_timer.c new file +7 -0

@@ -0,0 +1,69 @@
+/*
+ * Copyright (c) 2012, Texas Instruments <www.ti.com>
+ *
+ * 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.

Quoted 3 lines

Are we adding dead code here? Or where exactly is this code being used?

This is a preparatory patch for keystone port that we are currently working on. I can repost the reworked code along with patch for the port if it can't be merged without the user code.

This patch introduces a configurable mechanism to disable subpage writes in the DaVinci NAND driver.

Signed-off-by Vitaly Andrianov <[email protected]>

Signed-off-by Murali Karicheri <[email protected]>

---
 drivers/mtd/nand/davinci_nand.c |    3 +++
 1 file changed, 3 insertions(+)

drivers/mtd/nand/davinci_nand.c +0 -0

index 5b17d7b..75b03a7 100644

Quoted 2 lines

New CONFIG_ options MUSt be documented in the README.

Will do.

Quoted 1 line

Also, you are just adding dead code. There are no users for this option.

This is a preparatory patch for keystone u-boot port and will bundle with it if it make more sense. But this looks harmless since this option allow users to disable subpage write. So I want to merge this right away after adding the option name to README. Any issues?

This patch introduces a configurable mechanism to disable subpage writes in the DaVinci NAND driver.

Signed-off-by Vitaly Andrianov <[email protected]>

Signed-off-by Murali Karicheri <[email protected]>

---
 drivers/mtd/nand/davinci_nand.c |    3 +++
 1 file changed, 3 insertions(+)

drivers/mtd/nand/davinci_nand.c +0 -0

index 5b17d7b..75b03a7 100644

Quoted 17 lines

-----Original Message-----
From: Scott Wood [mailto:scottwood at freescale.com]
Sent: Wednesday, January 22, 2014 3:48 PM
To: Karicheri, Muralidharan
Cc: u-boot at lists.denx.de; Rini, Tom; Andrianov, Vitaly
Subject: Re: [U-Boot] [PATCH 5/5] NAND: DaVinci: allow forced disable of subpage writes

Quoted 9 lines · Murali Karicheri

This patch introduces a configurable mechanism to disable subpage writes in the DaVinci NAND driver.

Signed-off-by Vitaly Andrianov <[email protected]>

Signed-off-by Murali Karicheri <[email protected]>

---
 drivers/mtd/nand/davinci_nand.c |    3 +++
 1 file changed, 3 insertions(+)

Quoted 17 lines

Quoted 5 lines

(quoted diff) +0 -0

b/drivers/mtd/nand/davinci_nand.c index 5b17d7b..75b03a7 100644

drivers/mtd/nand/davinci_nand.c void davinci_nand_init(struct nand_chip *nand) · line 617

Quoted 9 lines

drivers/mtd/nand/davinci_nand.c +3 -0

@@ -609,6 +609,9 @@ void davinci_nand_init(struct nand_chip *nand)
 #ifdef CONFIG_SYS_NAND_USE_FLASH_BBT
 	nand->bbt_options	  |= NAND_BBT_USE_FLASH;
 #endif
+#ifdef CONFIG_SYS_NAND_NO_SUBPAGE_WRITE
+	nand->options	  |= NAND_NO_SUBPAGE_WRITE;
+#endif
 #ifdef CONFIG_SYS_NAND_HW_ECC
 	nand->ecc.mode = NAND_ECC_HW;
 	nand->ecc.size = 512;

Why does it need to be configurable?

Same driver used in multiple platforms, and on some platforms like the one I work with doesn't support subpage writes and has to be disabled.

Quoted 35 lines · Muralidharan

Quoted 34 lines

-----Original Message-----
From: Scott Wood [mailto:scottwood at freescale.com]
Sent: Wednesday, January 22, 2014 3:48 PM
To: Karicheri, Muralidharan
Cc: u-boot at lists.denx.de; Rini, Tom; Andrianov, Vitaly
Subject: Re: [U-Boot] [PATCH 5/5] NAND: DaVinci: allow forced disable of subpage writes

Quoted 14 lines · Murali Karicheri

This patch introduces a configurable mechanism to disable subpage writes in the DaVinci NAND driver.

Signed-off-by Vitaly Andrianov <[email protected]>

Signed-off-by Murali Karicheri <[email protected]>

---
 drivers/mtd/nand/davinci_nand.c |    3 +++
 1 file changed, 3 insertions(+)

(quoted diff) +0 -0

b/drivers/mtd/nand/davinci_nand.c index 5b17d7b..75b03a7 100644

drivers/mtd/nand/davinci_nand.c void davinci_nand_init(struct nand_chip *nand) · line 617

Quoted 9 lines

drivers/mtd/nand/davinci_nand.c +3 -0

@@ -609,6 +609,9 @@ void davinci_nand_init(struct nand_chip *nand)
 #ifdef CONFIG_SYS_NAND_USE_FLASH_BBT
 	nand->bbt_options	  |= NAND_BBT_USE_FLASH;
 #endif
+#ifdef CONFIG_SYS_NAND_NO_SUBPAGE_WRITE
+	nand->options	  |= NAND_NO_SUBPAGE_WRITE;
+#endif
 #ifdef CONFIG_SYS_NAND_HW_ECC
 	nand->ecc.mode = NAND_ECC_HW;
 	nand->ecc.size = 512;

Why does it need to be configurable?

Quoted 2 lines

Same driver used in multiple platforms, and on some platforms like the one I work with doesn't support subpage writes and has to be disabled.

The option needs to be documented, and it should be either named in a davinci-specific manner, or the documentation should clearly list which drivers pay attention to it.

It seems odd though, that the ability to support subpage writes is an independent thing rather than a consequence of some other variable such as controller version.

Quoted 34 lines · Scott Wood

Quoted 33 lines · Muralidharan

Quoted 33 lines

-----Original Message-----
From: Scott Wood [mailto:scottwood at freescale.com]
Sent: Wednesday, January 22, 2014 3:48 PM
To: Karicheri, Muralidharan
Cc: u-boot at lists.denx.de; Rini, Tom; Andrianov, Vitaly
Subject: Re: [U-Boot] [PATCH 5/5] NAND: DaVinci: allow forced disable of subpage writes

Quoted 14 lines · Murali Karicheri

This patch introduces a configurable mechanism to disable subpage writes in the DaVinci NAND driver.

Signed-off-by Vitaly Andrianov <[email protected]>

Signed-off-by Murali Karicheri <[email protected]>

---
  drivers/mtd/nand/davinci_nand.c |    3 +++
  1 file changed, 3 insertions(+)

(quoted diff) +0 -0

b/drivers/mtd/nand/davinci_nand.c index 5b17d7b..75b03a7 100644

drivers/mtd/nand/davinci_nand.c void davinci_nand_init(struct nand_chip *nand) · line 617

Quoted 9 lines

drivers/mtd/nand/davinci_nand.c +3 -0

@@ -609,6 +609,9 @@ void davinci_nand_init(struct nand_chip *nand)
 #ifdef CONFIG_SYS_NAND_USE_FLASH_BBT
  	nand->bbt_options	  |= NAND_BBT_USE_FLASH;
  #endif
+#ifdef CONFIG_SYS_NAND_NO_SUBPAGE_WRITE
+	nand->options	  |= NAND_NO_SUBPAGE_WRITE;
+#endif
  #ifdef CONFIG_SYS_NAND_HW_ECC
  	nand->ecc.mode = NAND_ECC_HW;
  	nand->ecc.size = 512;

Why does it need to be configurable?

Quoted 5 lines

Quoted 2 lines

Same driver used in multiple platforms, and on some platforms like the one I work with doesn't support subpage writes and has to be disabled.

The option needs to be documented, and it should be either named in a davinci-specific manner, or the documentation should clearly list which drivers pay attention to it.

Agreed.

Quoted 3 lines

It seems odd though, that the ability to support subpage writes is an independent thing rather than a consequence of some other variable such as controller version.

I suspect this is similar to the OMAP driver where we could support subpages but it gets tricky coordinating with the HW ECC engine for BCH8 and higher. Perhaps there are davinci targets with smaller ECC levels where it works out OK but keystone is doing BCH8 or 16 and it's not worth the savings.