[U-Boot] [PATCH 0/5] preparation to intriduce keystone support

[U-Boot] [PATCH 0/5] preparation to intriduce keystone support

[Murali Karicheri]

The Texas Instruments Keystone SOC u-boot support requires some
common code modifications to be done before introducing the
support itself.

This series of patches makes these modifications.

Signed-off-by: Vitaly Andrianov <vitalya@ti.com>
Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>

Murali Karicheri (5):
  tools: mkimage: add support for gpimage format
  ubifs: fix checkpatch warning
  ubifs: return filesize from ubifs load operations
  arm: add support for arch timer
  NAND: DaVinci: allow forced disable of subpage writes

 arch/arm/lib/Makefile           |    1 +
 arch/arm/lib/arch_timer.c       |   69 ++++++++++++++++++++++++++
 common/cmd_ubifs.c              |   16 ++----
 common/image.c                  |    1 +
 drivers/mtd/nand/davinci_nand.c |    3 ++
 fs/ubifs/ubifs.c                |   14 +++---
 fs/ubifs/ubifs.h                |    7 +++
 include/image.h                 |    1 +
 tools/Makefile                  |    6 +++
 tools/gpheader.h                |   41 ++++++++++++++++
 tools/gpimage-common.c          |  103 +++++++++++++++++++++++++++++++++++++++
 tools/gpimage.c                 |   78 +++++++++++++++++++++++++++++
 tools/imagetool.c               |    2 +
 tools/imagetool.h               |    1 +
 tools/omapimage.c               |   97 ++++++------------------------------
 tools/omapimage.h               |    5 --
 16 files changed, 340 insertions(+), 105 deletions(-)
 create mode 100644 arch/arm/lib/arch_timer.c
 create mode 100644 tools/gpheader.h
 create mode 100644 tools/gpimage-common.c
 create mode 100644 tools/gpimage.c

-- 
1.7.9.5

[U-Boot] [PATCH 1/5] tools: mkimage: add support for gpimage format

[Murali Karicheri]

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

Signed-off-by: Vitaly Andrianov <vitalya@ti.com>
Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
---
 - Tested only on keystone. Need help to test on OMAP since this is 
   impacted.

 common/image.c         |    1 +
 include/image.h        |    1 +
 tools/Makefile         |    6 +++
 tools/gpheader.h       |   41 +++++++++++++++++++
 tools/gpimage-common.c |  103 ++++++++++++++++++++++++++++++++++++++++++++++++
 tools/gpimage.c        |   78 ++++++++++++++++++++++++++++++++++++
 tools/imagetool.c      |    2 +
 tools/imagetool.h      |    1 +
 tools/omapimage.c      |   97 +++++++--------------------------------------
 tools/omapimage.h      |    5 ---
 10 files changed, 248 insertions(+), 87 deletions(-)
 create mode 100644 tools/gpheader.h
 create mode 100644 tools/gpimage-common.c
 create mode 100644 tools/gpimage.c

diff --git a/common/image.c b/common/image.c
index ae95c3f..52ee840 100644
--- a/common/image.c
+++ b/common/image.c
@@ -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,		    "",		  "",			},
 };
 
diff --git a/include/image.h b/include/image.h
index 7de2bb2..0a3d346 100644
--- a/include/image.h
+++ b/include/image.h
@@ -214,6 +214,7 @@ struct lmb;
 #define IH_TYPE_KERNEL_NOLOAD	14	/* OS Kernel Image, can run from any load address */
 #define IH_TYPE_PBLIMAGE	15	/* Freescale PBL Boot Image	*/
 #define IH_TYPE_MXSIMAGE	16	/* Freescale MXSBoot Image	*/
+#define IH_TYPE_GPIMAGE		17	/* TI Keystone GPHeader Image	*/
 
 /*
  * Compression Types
diff --git a/tools/Makefile b/tools/Makefile
index 328cea3..cbbe479 100644
--- a/tools/Makefile
+++ b/tools/Makefile
@@ -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
 NOPED_OBJ_FILES-y += pblimage.o
 NOPED_OBJ_FILES-y += proftool.o
@@ -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 \
 			$(obj)os_support.o \
 			$(obj)pblimage.o \
 			$(obj)sha1.o \
@@ -248,7 +252,9 @@ $(obj)mkimage$(SFX):	$(obj)aisimage.o \
 			$(obj)md5.o \
 			$(obj)mkimage.o \
 			$(obj)mxsimage.o \
+			$(obj)gpimage-common.o \
 			$(obj)omapimage.o \
+			$(obj)gpimage.o \
 			$(obj)os_support.o \
 			$(obj)pblimage.o \
 			$(obj)sha1.o \
diff --git a/tools/gpheader.h b/tools/gpheader.h
new file mode 100644
index 0000000..0bfc0c2
--- /dev/null
+++ b/tools/gpheader.h
@@ -0,0 +1,41 @@
+/*
+ * (C) Copyright 2014
+ * Texas Instruments Incorporated
+ * Refactored common functions in to gpimage-common.c. Include this common
+ * header file
+ *
+ * (C) Copyright 2010
+ * Linaro LTD, www.linaro.org
+ * Author: John Rigby <john.rigby@linaro.org>
+ * Based on TI's signGP.c
+ *
+ * (C) Copyright 2009
+ * Stefano Babic, DENX Software Engineering, sbabic at denx.de.
+ *
+ * (C) Copyright 2008
+ * Marvell Semiconductor <www.marvell.com>
+ * Written-by: Prafulla Wadaskar <prafulla@marvell.com>
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+#ifndef _GPIMAGE_H_
+#define _GPIMAGE_H_
+
+/* common headers for gpimage and omapimage formats */
+struct gp_header {
+	uint32_t size;
+	uint32_t load_addr;
+};
+#define GPIMAGE_HDR_SIZE (sizeof(struct gp_header))
+
+/* common functions across gpimage and omapimage handlers */
+uint32_t gpimage_swap32(uint32_t data);
+int valid_gph_size(uint32_t size);
+int valid_gph_load_addr(uint32_t load_addr);
+int gph_verify_header(struct gp_header *gph, int do_swap32);
+void gph_print_header(const struct gp_header *gph, int do_swap32);
+void gph_set_header(struct gp_header *gph, uint32_t size, uint32_t load_addr,
+			int do_swap32);
+int gpimage_check_params(struct image_tool_params *params);
+#endif
diff --git a/tools/gpimage-common.c b/tools/gpimage-common.c
new file mode 100644
index 0000000..85b9819
--- /dev/null
+++ b/tools/gpimage-common.c
@@ -0,0 +1,103 @@
+/*
+ * (C) Copyright 2014
+ * Texas Instruments Incorporated
+ * Refactored common functions in to gpimage-common.c.
+ *
+ * (C) Copyright 2010
+ * Linaro LTD, www.linaro.org
+ * Author: John Rigby <john.rigby@linaro.org>
+ * Based on TI's signGP.c
+ *
+ * (C) Copyright 2009
+ * Stefano Babic, DENX Software Engineering, sbabic at denx.de.
+ *
+ * (C) Copyright 2008
+ * Marvell Semiconductor <www.marvell.com>
+ * Written-by: Prafulla Wadaskar <prafulla@marvell.com>
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+#include "imagetool.h"
+#include <compiler.h>
+#include <image.h>
+#include "gpheader.h"
+
+uint32_t gpimage_swap32(uint32_t data)
+{
+	return cpu_to_be32(data);
+}
+
+/* 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;
+}
+
+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;
+	}
+
+	if (!valid_gph_size(gph_size))
+		return -1;
+	if (!valid_gph_load_addr(gph_load_addr))
+		return -1;
+	return 0;
+}
+
+void gph_print_header(const 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;
+	}
+
+	if (!valid_gph_size(gph_size)) {
+		fprintf(stderr, "Error: invalid image size %x\n", gph_size);
+		exit(EXIT_FAILURE);
+	}
+
+	if (!valid_gph_load_addr(gph_load_addr)) {
+		fprintf(stderr, "Error: invalid image load address %x\n",
+			gph_load_addr);
+		exit(EXIT_FAILURE);
+	}
+	printf("GP Header: Size %x LoadAddr %x\n", gph_size, gph_load_addr);
+}
+
+void gph_set_header(struct gp_header *gph, uint32_t size, uint32_t load_addr,
+	int do_swap32)
+{
+	if (do_swap32) {
+		gph->size = gpimage_swap32(size);
+		gph->load_addr = gpimage_swap32(load_addr);
+	} else {
+		gph->size = size;
+		gph->load_addr = load_addr;
+	}
+}
+
+int gpimage_check_params(struct image_tool_params *params)
+{
+	return	(params->dflag && (params->fflag || params->lflag)) ||
+		(params->fflag && (params->dflag || params->lflag)) ||
+		(params->lflag && (params->dflag || params->fflag));
+}
diff --git a/tools/gpimage.c b/tools/gpimage.c
new file mode 100644
index 0000000..c316bfb
--- /dev/null
+++ b/tools/gpimage.c
@@ -0,0 +1,78 @@
+/*
+ * (C) Copyright 2014
+ * Texas Instruments Incorporated
+ * Add gpimage format for keystone devices to format spl image. This is
+ * Based on omapimage.c
+ *
+ * (C) Copyright 2010
+ * Linaro LTD, www.linaro.org
+ * Author: John Rigby <john.rigby@linaro.org>
+ * Based on TI's signGP.c
+ *
+ * (C) Copyright 2009
+ * Stefano Babic, DENX Software Engineering, sbabic at denx.de.
+ *
+ * (C) Copyright 2008
+ * Marvell Semiconductor <www.marvell.com>
+ * Written-by: Prafulla Wadaskar <prafulla@marvell.com>
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+#include "imagetool.h"
+#include <compiler.h>
+#include <image.h>
+#include "gpheader.h"
+
+static uint8_t gpimage_header[GPIMAGE_HDR_SIZE];
+
+/* to be in keystone gpimage */
+static int gpimage_check_image_types(uint8_t type)
+{
+	if (type == IH_TYPE_GPIMAGE)
+		return EXIT_SUCCESS;
+	else
+		return EXIT_FAILURE;
+}
+
+static int gpimage_verify_header(unsigned char *ptr, int image_size,
+			struct image_tool_params *params)
+{
+	struct gp_header *gph = (struct gp_header *)ptr;
+
+	return gph_verify_header(gph, 1);
+}
+
+static void gpimage_print_header(const void *ptr)
+{
+	const struct gp_header *gph = (struct gp_header *)ptr;
+
+	gph_print_header(gph, 1);
+}
+
+static void gpimage_set_header(void *ptr, struct stat *sbuf, int ifd,
+				struct image_tool_params *params)
+{
+	struct gp_header *gph = (struct gp_header *)ptr;
+
+	gph_set_header(gph, sbuf->st_size - GPIMAGE_HDR_SIZE, params->addr, 1);
+}
+
+/*
+ * gpimage parameters
+ */
+static struct image_type_params gpimage_params = {
+	.name		= "TI KeyStone GP Image support",
+	.header_size	= GPIMAGE_HDR_SIZE,
+	.hdr		= (void *)&gpimage_header,
+	.check_image_type = gpimage_check_image_types,
+	.verify_header	= gpimage_verify_header,
+	.print_header	= gpimage_print_header,
+	.set_header	= gpimage_set_header,
+	.check_params	= gpimage_check_params,
+};
+
+void init_gpimage_type(void)
+{
+	register_image_type(&gpimage_params);
+}
diff --git a/tools/imagetool.c b/tools/imagetool.c
index 29d2189..da72115 100644
--- a/tools/imagetool.c
+++ b/tools/imagetool.c
@@ -45,6 +45,8 @@ void register_image_tool(imagetool_register_t image_register)
 	init_ubl_image_type();
 	/* Init Davinci AIS support */
 	init_ais_image_type();
+	/* Init TI Keystone boot image generation/list support */
+	init_gpimage_type();
 }
 
 /*
diff --git a/tools/imagetool.h b/tools/imagetool.h
index c2c9aea..a3e9d30 100644
--- a/tools/imagetool.h
+++ b/tools/imagetool.h
@@ -167,6 +167,7 @@ void init_mxs_image_type(void);
 void init_fit_image_type(void);
 void init_ubl_image_type(void);
 void init_omap_image_type(void);
+void init_gpimage_type(void);
 
 void pbl_load_uboot(int fd, struct image_tool_params *mparams);
 
diff --git a/tools/omapimage.c b/tools/omapimage.c
index d59bc4d..de5a50a 100644
--- a/tools/omapimage.c
+++ b/tools/omapimage.c
@@ -15,26 +15,17 @@
  */
 
 #include "imagetool.h"
+#include <compiler.h>
 #include <image.h>
+#include "gpheader.h"
 #include "omapimage.h"
 
 /* Header size is CH header rounded up to 512 bytes plus GP header */
 #define OMAP_CH_HDR_SIZE 512
-#define OMAP_GP_HDR_SIZE (sizeof(struct gp_header))
-#define OMAP_FILE_HDR_SIZE (OMAP_CH_HDR_SIZE+OMAP_GP_HDR_SIZE)
+#define OMAP_FILE_HDR_SIZE (OMAP_CH_HDR_SIZE + GPIMAGE_HDR_SIZE)
 
 static int do_swap32 = 0;
 
-static uint32_t omapimage_swap32(uint32_t data)
-{
-	uint32_t result = 0;
-	result  = (data & 0xFF000000) >> 24;
-	result |= (data & 0x00FF0000) >> 8;
-	result |= (data & 0x0000FF00) << 8;
-	result |= (data & 0x000000FF) << 24;
-	return result;
-}
-
 static uint8_t omapimage_header[OMAP_FILE_HDR_SIZE];
 
 static int omapimage_check_image_types(uint8_t type)
@@ -46,40 +37,18 @@ static int omapimage_check_image_types(uint8_t type)
 	}
 }
 
-/*
- * Only the simplest image type is currently supported:
- * TOC pointing to CHSETTINGS
- * TOC terminator
- * CHSETTINGS
- *
- * padding to OMAP_CH_HDR_SIZE bytes
- *
- * gp header
- *   size
- *   load_addr
- */
-static int valid_gph_size(uint32_t size)
-{
-	return size;
-}
-
-static int valid_gph_load_addr(uint32_t load_addr)
-{
-	return load_addr;
-}
-
 static int omapimage_verify_header(unsigned char *ptr, int image_size,
 			struct image_tool_params *params)
 {
 	struct ch_toc *toc = (struct ch_toc *)ptr;
 	struct gp_header *gph = (struct gp_header *)(ptr+OMAP_CH_HDR_SIZE);
-	uint32_t offset, size, gph_size, gph_load_addr;
+	uint32_t offset, size;
 
 	while (toc->section_offset != 0xffffffff
 			&& toc->section_size != 0xffffffff) {
 		if (do_swap32) {
-			offset = omapimage_swap32(toc->section_offset);
-			size = omapimage_swap32(toc->section_size);
+			offset = gpimage_swap32(toc->section_offset);
+			size = gpimage_swap32(toc->section_size);
 		} else {
 			offset = toc->section_offset;
 			size = toc->section_size;
@@ -92,17 +61,7 @@ static int omapimage_verify_header(unsigned char *ptr, int image_size,
 		toc++;
 	}
 
-	if (do_swap32) {
-		gph_size = omapimage_swap32(gph->size);
-		gph_load_addr = omapimage_swap32(gph->load_addr);
-	} else {
-		gph_size = gph->size;
-		gph_load_addr = gph->load_addr;
-	}
-
-	if (!valid_gph_size(gph_size))
-		return -1;
-	if (!valid_gph_load_addr(gph_load_addr))
+	if (gph_verify_header(gph, do_swap32) < 0)
 		return -1;
 
 	return 0;
@@ -135,13 +94,13 @@ static void omapimage_print_header(const void *ptr)
 	const struct ch_toc *toc = (struct ch_toc *)ptr;
 	const struct gp_header *gph =
 			(struct gp_header *)(ptr+OMAP_CH_HDR_SIZE);
-	uint32_t offset, size, gph_size, gph_load_addr;
+	uint32_t offset, size;
 
 	while (toc->section_offset != 0xffffffff
 			&& toc->section_size != 0xffffffff) {
 		if (do_swap32) {
-			offset = omapimage_swap32(toc->section_offset);
-			size = omapimage_swap32(toc->section_size);
+			offset = gpimage_swap32(toc->section_offset);
+			size = gpimage_swap32(toc->section_size);
 		} else {
 			offset = toc->section_offset;
 			size = toc->section_size;
@@ -160,26 +119,7 @@ static void omapimage_print_header(const void *ptr)
 		toc++;
 	}
 
-	if (do_swap32) {
-		gph_size = omapimage_swap32(gph->size);
-		gph_load_addr = omapimage_swap32(gph->load_addr);
-	} else {
-		gph_size = gph->size;
-		gph_load_addr = gph->load_addr;
-	}
-
-	if (!valid_gph_size(gph_size)) {
-		fprintf(stderr, "Error: invalid image size %x\n", gph_size);
-		exit(EXIT_FAILURE);
-	}
-
-	if (!valid_gph_load_addr(gph_load_addr)) {
-		fprintf(stderr, "Error: invalid image load address %x\n",
-				gph_load_addr);
-		exit(EXIT_FAILURE);
-	}
-
-	printf("GP Header: Size %x LoadAddr %x\n", gph_size, gph_load_addr);
+	gph_print_header(gph, do_swap32);
 }
 
 static int toc_offset(void *hdr, void *member)
@@ -208,8 +148,8 @@ static void omapimage_set_header(void *ptr, struct stat *sbuf, int ifd,
 	toc++;
 	memset(toc, 0xff, sizeof(*toc));
 
-	gph->size = sbuf->st_size - OMAP_FILE_HDR_SIZE;
-	gph->load_addr = params->addr;
+	gph_set_header(gph, sbuf->st_size - OMAP_FILE_HDR_SIZE,
+		       params->addr, 0);
 
 	if (strncmp(params->imagename, "byteswap", 8) == 0) {
 		do_swap32 = 1;
@@ -217,20 +157,13 @@ static void omapimage_set_header(void *ptr, struct stat *sbuf, int ifd,
 		uint32_t *data = (uint32_t *)ptr;
 
 		while (swapped <= (sbuf->st_size / sizeof(uint32_t))) {
-			*data = omapimage_swap32(*data);
+			*data = gpimage_swap32(*data);
 			swapped++;
 			data++;
 		}
 	}
 }
 
-int omapimage_check_params(struct image_tool_params *params)
-{
-	return	(params->dflag && (params->fflag || params->lflag)) ||
-		(params->fflag && (params->dflag || params->lflag)) ||
-		(params->lflag && (params->dflag || params->fflag));
-}
-
 /*
  * omapimage parameters
  */
@@ -242,7 +175,7 @@ static struct image_type_params omapimage_params = {
 	.verify_header	= omapimage_verify_header,
 	.print_header	= omapimage_print_header,
 	.set_header	= omapimage_set_header,
-	.check_params	= omapimage_check_params,
+	.check_params	= gpimage_check_params,
 };
 
 void init_omap_image_type(void)
diff --git a/tools/omapimage.h b/tools/omapimage.h
index 45d14ea..8744ae7 100644
--- a/tools/omapimage.h
+++ b/tools/omapimage.h
@@ -25,10 +25,5 @@ struct ch_settings {
 	uint32_t flags;
 };
 
-struct gp_header {
-	uint32_t size;
-	uint32_t load_addr;
-};
-
 #define KEY_CHSETTINGS 0xC0C0C0C1
 #endif /* _OMAPIMAGE_H_ */
-- 
1.7.9.5

[U-Boot] [PATCH 1/5] tools: mkimage: add support for gpimage format

[Wolfgang Denk]

Dear Murali Karicheri,

In message <1390255810-19486-2-git-send-email-m-karicheri2@ti.com> 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
...
> --- a/common/image.c
> +++ b/common/image.c
> @@ -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.

> --- a/tools/Makefile
> +++ b/tools/Makefile
> @@ -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.

> @@ -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.


> +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.

> +/* 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.

> +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..

> +void gph_print_header(const 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;
> +	}

This is repeasted code.  Get rid of it.

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

And again.



Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"The X11 source code style is ATROCIOUS and should not be used  as  a
model."                                                   - Doug Gwyn

[U-Boot] [PATCH 1/5] tools: mkimage: add support for gpimage format

[Tom Rini]

On Mon, Jan 20, 2014 at 11:46:49PM +0100, Wolfgang Denk wrote:
> Dear Murali Karicheri,
> 
> In message <1390255810-19486-2-git-send-email-m-karicheri2@ti.com> 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
> ...
> > --- a/common/image.c
> > +++ b/common/image.c
> > @@ -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.
> 
> > --- a/tools/Makefile
> > +++ b/tools/Makefile
> > @@ -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.
> 
> > @@ -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.
> 
> 
> > +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.
> 
> > +/* 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.
> 
> > +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..

This appears to be a blind re-use of the omap code where we do have a
case where we have to do this kind of swap.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20140120/474b3cc0/attachment.pgp>

[U-Boot] [PATCH 1/5] tools: mkimage: add support for gpimage format

[Wolfgang Denk]

Dear Murali,

In message <3E54258959B69E4282D79E01AB1F32B70466CE4A@DFLE11.ent.ti.com> you wrote:
>
> >> +	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..
> 
> 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?


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Q:  Do you know what the death rate around here is?
A:  One per person.

[U-Boot] [PATCH 1/5] tools: mkimage: add support for gpimage format

[Tom Rini]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 01/21/2014 02:34 PM, Wolfgang Denk wrote:
> Dear Murali,
> 
> In message
> <3E54258959B69E4282D79E01AB1F32B70466CE4A@DFLE11.ent.ti.com> you
> wrote:
>> 
>>>> +	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..
>> 
>> 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?

In omapimage? Yes.  On gpimage?  I doubt it.

- -- 
Tom
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJS3uVpAAoJENk4IS6UOR1WzoUP/1SFGSUOUV+T9F/2qFTpftNf
Wt/3d+Pel7YFxzswZcj2Tmgdf8dpN6/3VUXByRxvru7MPOXvKLkVgr6m94s0a5lH
T6i/492u5H+BdHVBXfQlSqULMOExWk6CEtRzIydEqrLmS6PehTCcuvDLrPYBeegk
xncOap21YoPzfA8cvFhZ8y+ucVCbS7U/zHhLoNw69pc+AW0aZjTzOgEBdTHagkCX
KV4WxfTAmBnOl+u7Pn38GPU9zfvVre4UDINiVN+/Qr6vru9B5MYAUIdOAb9a9O6x
a00Yoh9xRfW9PbAyguW+EoURX5aIhoLFTTEDJ6HKyvmt4qhw+riG2/GrkXaRYJPD
kifL1wfkQOYHD5E59soJ5XV1GqIfocvEO4aCmdNmizJlXsjlLNgeIklRdv1dA1Dt
f2RaSlip/B1+joLyxthCqG9VfU2PKz3m6+Wy2SxIO1lf2cGbgKFJY6oGi5gGpTCy
+TbpZx+1ltF36zKIFIdKqHc9w9xMc05VYZfSqfOBmGe3NTuR1tRB2FvrBdEYOqIW
LNOLAz0od/mTxG0tiP+BkGNx+ZwYMhck3zZIXdWBOmCigu0RFFG5SSIF0uGoAv/z
Th3y3qhCDJ2YGef+euWJvQwz1oxXb7WdV23p+zmDPSyQUpl0eKDyiMrq0ooo/TZI
pplaUIOfbd5sumSZC4E2
=X3XI
-----END PGP SIGNATURE-----

[U-Boot] [PATCH 2/5] ubifs: fix checkpatch warning

[Murali Karicheri]

Fix the following checkpatch warning:-

WARNING: externs should be avoided in .c files

Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
---
 common/cmd_ubifs.c |    9 ---------
 fs/ubifs/ubifs.h   |    7 +++++++
 2 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/common/cmd_ubifs.c b/common/cmd_ubifs.c
index d9af023..fdc8bfe 100644
--- a/common/cmd_ubifs.c
+++ b/common/cmd_ubifs.c
@@ -21,15 +21,6 @@
 static int ubifs_initialized;
 static int ubifs_mounted;
 
-extern struct super_block *ubifs_sb;
-
-/* Prototypes */
-int ubifs_init(void);
-int ubifs_mount(char *vol_name);
-void ubifs_umount(struct ubifs_info *c);
-int ubifs_ls(char *dir_name);
-int ubifs_load(char *filename, u32 addr, u32 size);
-
 int do_ubifs_mount(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 {
 	char *vol_name;
diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h
index 633631e..2213201 100644
--- a/fs/ubifs/ubifs.h
+++ b/fs/ubifs/ubifs.h
@@ -2137,6 +2137,13 @@ void ubifs_compress(const void *in_buf, int in_len, void *out_buf, int *out_len,
 int ubifs_decompress(const void *buf, int len, void *out, int *out_len,
 		     int compr_type);
 
+/* these are used in cmd_ubifs.c */
+int ubifs_init(void);
+int ubifs_mount(char *vol_name);
+void ubifs_umount(struct ubifs_info *c);
+int ubifs_ls(char *dir_name);
+int ubifs_load(char *filename, u32 addr, u32 size);
+
 #include "debug.h"
 #include "misc.h"
 #include "key.h"
-- 
1.7.9.5

[U-Boot] [U-Boot,2/5] ubifs: fix checkpatch warning

[Tom Rini]

On Mon, Jan 20, 2014 at 05:10:07PM -0500, Karicheri, Muralidharan wrote:

> Fix the following checkpatch warning:-
> 
> WARNING: externs should be avoided in .c files
> 
> Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20140221/b26ff6b6/attachment.pgp>

[U-Boot] [PATCH 3/5] ubifs: return filesize from ubifs load operations

[Murali Karicheri]

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 <vitalya@ti.com>
Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
---
 common/cmd_ubifs.c |    7 +++++--
 fs/ubifs/ubifs.c   |   14 +++++++-------
 fs/ubifs/ubifs.h   |    2 +-
 3 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/common/cmd_ubifs.c b/common/cmd_ubifs.c
index fdc8bfe..a2926e0 100644
--- a/common/cmd_ubifs.c
+++ b/common/cmd_ubifs.c
@@ -110,6 +110,7 @@ int do_ubifs_load(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	int ret;
 	u32 addr;
 	u32 size = 0;
+	char buf[32];
 
 	if (!ubifs_mounted) {
 		printf("UBIFS not mounted, use ubifs mount to mount volume first!\n");
@@ -132,12 +133,14 @@ int do_ubifs_load(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	}
 	debug("Loading file '%s' to address 0x%08x (size %d)\n", filename, addr, size);
 
-	ret = ubifs_load(filename, addr, size);
+	ret = ubifs_load(filename, addr, &size);
 	if (ret) {
 		printf("** File not found %s **\n", filename);
 		ret = CMD_RET_FAILURE;
+	} else {
+		sprintf(buf, "%X", size);
+		setenv("filesize", buf);
 	}
-
 	return ret;
 }
 
diff --git a/fs/ubifs/ubifs.c b/fs/ubifs/ubifs.c
index 273c0a9..4f1d5c7 100644
--- a/fs/ubifs/ubifs.c
+++ b/fs/ubifs/ubifs.c
@@ -677,7 +677,7 @@ error:
 	return err;
 }
 
-int ubifs_load(char *filename, u32 addr, u32 size)
+int ubifs_load(char *filename, u32 addr, u32 *size)
 {
 	struct ubifs_info *c = ubifs_sb->s_fs_info;
 	unsigned long inum;
@@ -711,12 +711,12 @@ int ubifs_load(char *filename, u32 addr, u32 size)
 	 * If no size was specified or if size bigger than filesize
 	 * set size to filesize
 	 */
-	if ((size == 0) || (size > inode->i_size))
-		size = inode->i_size;
+	if ((*size == 0) || (*size > inode->i_size))
+		*size = inode->i_size;
 
-	count = (size + UBIFS_BLOCK_SIZE - 1) >> UBIFS_BLOCK_SHIFT;
+	count = (*size + UBIFS_BLOCK_SIZE - 1) >> UBIFS_BLOCK_SHIFT;
 	printf("Loading file '%s' to addr 0x%08x with size %d (0x%08x)...\n",
-	       filename, addr, size, size);
+	       filename, addr, *size, *size);
 
 	page.addr = (void *)addr;
 	page.index = 0;
@@ -725,8 +725,8 @@ int ubifs_load(char *filename, u32 addr, u32 size)
 		/*
 		 * Make sure to not read beyond the requested size
 		 */
-		if (((i + 1) == count) && (size < inode->i_size))
-			last_block_size = size - (i * PAGE_SIZE);
+		if (((i + 1) == count) && (*size < inode->i_size))
+			last_block_size = *size - (i * PAGE_SIZE);
 
 		err = do_readpage(c, inode, &page, last_block_size);
 		if (err)
diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h
index 2213201..00485b3 100644
--- a/fs/ubifs/ubifs.h
+++ b/fs/ubifs/ubifs.h
@@ -2142,7 +2142,7 @@ int ubifs_init(void);
 int ubifs_mount(char *vol_name);
 void ubifs_umount(struct ubifs_info *c);
 int ubifs_ls(char *dir_name);
-int ubifs_load(char *filename, u32 addr, u32 size);
+int ubifs_load(char *filename, u32 addr, u32 *size);
 
 #include "debug.h"
 #include "misc.h"
-- 
1.7.9.5

[U-Boot] [PATCH 3/5] ubifs: return filesize from ubifs load operations

[Tom Rini]

On Mon, Jan 20, 2014 at 05:10:08PM -0500, Murali Karicheri wrote:

> 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 <vitalya@ti.com>
> Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>

But we already have code to do this, which now produces warnings when
this patch is applied.  Can you confirm that we are in fact not setting
filesize already when loading from ubifs?  Thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20140221/3656df77/attachment.pgp>

[U-Boot] [PATCH 4/5] arm: add support for arch timer

[Murali Karicheri]

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

Signed-off-by: Vitaly Andrianov <vitalya@ti.com>
Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
---
 arch/arm/lib/Makefile     |    1 +
 arch/arm/lib/arch_timer.c |   69 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 70 insertions(+)
 create mode 100644 arch/arm/lib/arch_timer.c

diff --git a/arch/arm/lib/Makefile b/arch/arm/lib/Makefile
index 321997c..58d58f8 100644
--- a/arch/arm/lib/Makefile
+++ b/arch/arm/lib/Makefile
@@ -42,6 +42,7 @@ obj-$(CONFIG_USE_ARCH_MEMCPY) += memcpy.o
 else
 obj-$(CONFIG_SPL_FRAMEWORK) += spl.o
 endif
+COBJS-$(CONFIG_SYS_ARCH_TIMER) += arch_timer.o
 
 ifdef CONFIG_ARM64
 obj-y	+= interrupts_64.o
diff --git a/arch/arm/lib/arch_timer.c b/arch/arm/lib/arch_timer.c
new file mode 100644
index 0000000..6ffa650
--- /dev/null
+++ b/arch/arm/lib/arch_timer.c
@@ -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.
+ *
+ * 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., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+#include <common.h>
+#include <asm/io.h>
+#include <div64.h>
+
+DECLARE_GLOBAL_DATA_PTR;
+
+int timer_init(void)
+{
+	gd->arch.tbl = 0;
+	gd->arch.tbu = 0;
+
+	gd->arch.timer_rate_hz = CONFIG_SYS_HZ_CLOCK / CONFIG_SYS_HZ;
+
+	return 0;
+}
+
+unsigned long long get_ticks(void)
+{
+	ulong nowl, nowu;
+
+	asm volatile("mrrc p15, 0, %0, %1, c14" : "=r" (nowl), "=r" (nowu));
+
+	gd->arch.tbl = nowl;
+	gd->arch.tbu = nowu;
+
+	return (((unsigned long long)gd->arch.tbu) << 32) | gd->arch.tbl;
+}
+
+
+ulong get_timer(ulong base)
+{
+	return lldiv(get_ticks(), gd->arch.timer_rate_hz) - base;
+}
+
+void __udelay(unsigned long usec)
+{
+	unsigned long long endtime;
+
+	endtime = lldiv((unsigned long long)usec * gd->arch.timer_rate_hz,
+			1000UL);
+
+	endtime += get_ticks();
+
+	while (get_ticks() < endtime)
+		;
+}
+
+ulong get_tbclk(void)
+{
+	return gd->arch.timer_rate_hz;
+}
-- 
1.7.9.5

[U-Boot] [PATCH 4/5] arm: add support for arch timer

[Wolfgang Denk]

Dear Murali Karicheri,

In message <1390255810-19486-5-git-send-email-m-karicheri2@ti.com> you wrote:
> This patch add basic support for the architecture timer found on recent
> ARMv7 based SoCs.

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


> --- /dev/null
> +++ b/arch/arm/lib/arch_timer.c
> @@ -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.
> + *
> + * 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., 675 Mass Ave, Cambridge, MA 02139, USA.
> + */

Please use SPDX License tag instead.


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
The world is coming to an end -- save your buffers!

[U-Boot] [PATCH 4/5] arm: add support for arch timer

[Wolfgang Denk]

Dear Murali,

In message <3E54258959B69E4282D79E01AB1F32B70466CE69@DFLE11.ent.ti.com> you wrote:
>
> >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.

I cannot see any logical reason to include this patch in this series,
so please resubmit when adding code that uses this feature.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
It's not an optical illusion, it just looks like one.   -- Phil White

[U-Boot] [PATCH 5/5] NAND: DaVinci: allow forced disable of subpage writes

[Murali Karicheri]

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

Signed-off-by: Vitaly Andrianov <vitalya@ti.com>
Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
---
 drivers/mtd/nand/davinci_nand.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c
index 5b17d7b..75b03a7 100644
--- a/drivers/mtd/nand/davinci_nand.c
+++ b/drivers/mtd/nand/davinci_nand.c
@@ -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;
-- 
1.7.9.5

[U-Boot] [PATCH 5/5] NAND: DaVinci: allow forced disable of subpage writes

[Wolfgang Denk]

Dear Murali Karicheri,

In message <1390255810-19486-6-git-send-email-m-karicheri2@ti.com> you wrote:
> This patch introduces a configurable mechanism to disable subpage writes
> in the DaVinci NAND driver.
> 
> Signed-off-by: Vitaly Andrianov <vitalya@ti.com>
> Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
> ---
>  drivers/mtd/nand/davinci_nand.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c
> index 5b17d7b..75b03a7 100644
> --- a/drivers/mtd/nand/davinci_nand.c
> +++ b/drivers/mtd/nand/davinci_nand.c
> @@ -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;

New CONFIG_ options MUSt be documented in the README.

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

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
While most peoples' opinions change, the conviction of their correct-
ness never does.

[U-Boot] [PATCH 5/5] NAND: DaVinci: allow forced disable of subpage writes

[Wolfgang Denk]

Dear Murali,

In message <3E54258959B69E4282D79E01AB1F32B70466CE81@DFLE11.ent.ti.com> you wrote:
>
> >New CONFIG_ options MUSt be documented in the README.
> 
> Will do.
> 
> >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?

The adding of the documentation to the README and of the code should
be in a single patch, not one after the other.  And the patch should
be art of a series that actually uses this feature.  It has no logical
place in the current patch series.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
That Microsoft, the Trabant of the operating  system  world,  may  be
glancing  over the Berlin Wall at the Audis and BMWs and Mercedes. In
their own universe Trabants and Ladas were mainstream too...
                                                   -- Evan Leibovitch

[U-Boot] [PATCH 5/5] NAND: DaVinci: allow forced disable of subpage writes

[Scott Wood]

On Mon, 2014-01-20 at 17:10 -0500, Murali Karicheri wrote:
> This patch introduces a configurable mechanism to disable subpage writes
> in the DaVinci NAND driver.
> 
> Signed-off-by: Vitaly Andrianov <vitalya@ti.com>
> Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
> ---
>  drivers/mtd/nand/davinci_nand.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c
> index 5b17d7b..75b03a7 100644
> --- a/drivers/mtd/nand/davinci_nand.c
> +++ b/drivers/mtd/nand/davinci_nand.c
> @@ -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?

-Scott

[U-Boot] [PATCH 5/5] NAND: DaVinci: allow forced disable of subpage writes

[Scott Wood]

On Tue, 2014-01-28 at 23:19 +0000, Karicheri, Muralidharan wrote:
> >-----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
> >
> >On Mon, 2014-01-20 at 17:10 -0500, Murali Karicheri wrote:
> >> This patch introduces a configurable mechanism to disable subpage
> >> writes in the DaVinci NAND driver.
> >>
> >> Signed-off-by: Vitaly Andrianov <vitalya@ti.com>
> >> Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
> >> ---
> >>  drivers/mtd/nand/davinci_nand.c |    3 +++
> >>  1 file changed, 3 insertions(+)
> >>
> >> diff --git a/drivers/mtd/nand/davinci_nand.c
> >> b/drivers/mtd/nand/davinci_nand.c index 5b17d7b..75b03a7 100644
> >> --- a/drivers/mtd/nand/davinci_nand.c
> >> +++ b/drivers/mtd/nand/davinci_nand.c
> >> @@ -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.

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.

-Scott

[U-Boot] [PATCH 5/5] NAND: DaVinci: allow forced disable of subpage writes

[Tom Rini]

On Wed, Feb 12, 2014 at 06:00:22PM -0600, Scott Wood wrote:
> 
> On Tue, 2014-01-28 at 23:19 +0000, Karicheri, Muralidharan wrote:
> > >-----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
> > >
> > >On Mon, 2014-01-20 at 17:10 -0500, Murali Karicheri wrote:
> > >> This patch introduces a configurable mechanism to disable subpage
> > >> writes in the DaVinci NAND driver.
> > >>
> > >> Signed-off-by: Vitaly Andrianov <vitalya@ti.com>
> > >> Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
> > >> ---
> > >>  drivers/mtd/nand/davinci_nand.c |    3 +++
> > >>  1 file changed, 3 insertions(+)
> > >>
> > >> diff --git a/drivers/mtd/nand/davinci_nand.c
> > >> b/drivers/mtd/nand/davinci_nand.c index 5b17d7b..75b03a7 100644
> > >> --- a/drivers/mtd/nand/davinci_nand.c
> > >> +++ b/drivers/mtd/nand/davinci_nand.c
> > >> @@ -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.
> 
> 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.

> 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.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20140218/c75a2a56/attachment.pgp>

[U-Boot] [PATCH 5/5] NAND: DaVinci: allow forced disable of subpage writes

[Murali Karicheri]

On 2/18/2014 9:57 AM, Rini, Tom wrote:
> On Wed, Feb 12, 2014 at 06:00:22PM -0600, Scott Wood wrote:
>> On Tue, 2014-01-28 at 23:19 +0000, Karicheri, Muralidharan wrote:
>>>> -----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
>>>>
>>>> On Mon, 2014-01-20 at 17:10 -0500, Murali Karicheri wrote:
>>>>> This patch introduces a configurable mechanism to disable subpage
>>>>> writes in the DaVinci NAND driver.
>>>>>
>>>>> Signed-off-by: Vitaly Andrianov <vitalya@ti.com>
>>>>> Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
>>>>> ---
>>>>>   drivers/mtd/nand/davinci_nand.c |    3 +++
>>>>>   1 file changed, 3 insertions(+)
>>>>>
>>>>> diff --git a/drivers/mtd/nand/davinci_nand.c
>>>>> b/drivers/mtd/nand/davinci_nand.c index 5b17d7b..75b03a7 100644
>>>>> --- a/drivers/mtd/nand/davinci_nand.c
>>>>> +++ b/drivers/mtd/nand/davinci_nand.c
>>>>> @@ -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.
>> 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.
>
>> 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.
Tom/Scott,

My next version of the series will have readme updated with this option 
(no change in name
as this can be used by other drivers as well)  and mention that 
davinci_nand driver is the
driver that currently uses this option.

Murali