[U-Boot] [PATCH v2 0/5] ARM: omap-common: Add board detection support for TI EVMs

[U-Boot] [PATCH v2 0/5] ARM: omap-common: Add board detection support for TI EVMs

[Steve Kipisz]

Several TI EVMs have onboard EEPROM that contain board description
information. The onboard EEPROM on Beaglebone, Beaglebone Black, AM335x
EVM, AM43x EVM, AM57xx EVM, Beagleboard-x15 all share the same format.

This series of patches introduces code which is generic among these
platforms. The boards can use the data for any operations they might
choose.

Lokesh Vutla (1):
  ARM: omap-common: Add standard access for board description EEPROM

Steve Kipisz (4):
  ARM: OMAP4/5: Centralize early clock initialization
  ARM: OMAP4/5: Centralize gpi2c_init
  ARM: OMAP4/5: Add generic board detection hook
  board: ti: AM57xx: Add detection logic for AM57xx-evm

 arch/arm/cpu/armv7/omap-common/Makefile        |   1 +
 arch/arm/cpu/armv7/omap-common/clocks-common.c |  26 ++++-
 arch/arm/cpu/armv7/omap-common/hwinit-common.c |  14 ++-
 arch/arm/cpu/armv7/omap-common/ti-i2c-eeprom.c | 148 +++++++++++++++++++++++++
 arch/arm/include/asm/arch-omap4/sys_proto.h    |   4 +-
 arch/arm/include/asm/arch-omap5/sys_proto.h    |   4 +-
 arch/arm/include/asm/omap_common.h             | 133 +++++++++++++++++++++-
 board/ti/am57xx/board.c                        |  53 +++++++++
 include/configs/am57xx_evm.h                   |   4 +
 include/configs/ti_omap5_common.h              |   2 +
 10 files changed, 380 insertions(+), 9 deletions(-)
 create mode 100644 arch/arm/cpu/armv7/omap-common/ti-i2c-eeprom.c

-- 
1.9.1

[U-Boot] [PATCH v2 1/5] ARM: OMAP4/5: Centralize early clock initialization

[Steve Kipisz]

Early clock initialization is currently done in two stages for OMAP4/5
SoCs. The first stage is the initialization of console clocks and
then we initialize basic clocks for functionality necessary for SoC
initialization and basic board functionality.

By splitting up prcm_init and centralizing this clock initialization,
we setup the code for follow on patches that can do board specific
initialization such as board detection which will depend on these
basic clocks.

As part of this change, since the early clock initialization
is centralized, we no longer need to expose the console clock
initialization and build it just for SPL.

NOTE: we change the sequence slightly by initializing console clocks
timer after the io settings are complete, but this is not expected
to have any functioanlity impact since we setup the basic IO drive
strength initialization as part of do_io_settings

Signed-off-by: Steve Kipisz <s-kipisz2@ti.com>
---
v2 Based on:
 master      a6104737 ARM: at91: sama5: change the environment address to 0x6000

Changes in v2:
 - New patch

 arch/arm/cpu/armv7/omap-common/clocks-common.c | 26 ++++++++++++++++++++++++--
 arch/arm/cpu/armv7/omap-common/hwinit-common.c |  3 +--
 arch/arm/include/asm/arch-omap4/sys_proto.h    |  2 +-
 arch/arm/include/asm/arch-omap5/sys_proto.h    |  2 +-
 4 files changed, 27 insertions(+), 6 deletions(-)

diff --git a/arch/arm/cpu/armv7/omap-common/clocks-common.c b/arch/arm/cpu/armv7/omap-common/clocks-common.c
index e28b79568d1d..2ede0818e444 100644
--- a/arch/arm/cpu/armv7/omap-common/clocks-common.c
+++ b/arch/arm/cpu/armv7/omap-common/clocks-common.c
@@ -769,7 +769,8 @@ void lock_dpll(u32 const base)
 	wait_for_lock(base);
 }
 
-void setup_clocks_for_console(void)
+#ifdef CONFIG_SPL_BUILD
+static void setup_clocks_for_console(void)
 {
 	/* Do not add any spl_debug prints in this function */
 	clrsetbits_le32((*prcm)->cm_l4per_clkstctrl, CD_CLKCTRL_CLKTRCTRL_MASK,
@@ -801,6 +802,9 @@ void setup_clocks_for_console(void)
 			CD_CLKCTRL_CLKTRCTRL_HW_AUTO <<
 			CD_CLKCTRL_CLKTRCTRL_SHIFT);
 }
+#else
+static inline void setup_clocks_for_console(void) { }
+#endif
 
 void do_enable_clocks(u32 const *clk_domains,
 			    u32 const *clk_modules_hw_auto,
@@ -853,14 +857,32 @@ void do_disable_clocks(u32 const *clk_domains,
 		disable_clock_domain(clk_domains[i]);
 }
 
-void prcm_init(void)
+/**
+ * setup_early_clocks() - Setup early clocks needed for SoC
+ *
+ * Setup clocks for console, SPL basic initialization clocks and initialize
+ * the timer. This is invoked prior prcm_init.
+ */
+void setup_early_clocks(void)
 {
+	setup_clocks_for_console();
+
 	switch (omap_hw_init_context()) {
 	case OMAP_INIT_CONTEXT_SPL:
 	case OMAP_INIT_CONTEXT_UBOOT_FROM_NOR:
 	case OMAP_INIT_CONTEXT_UBOOT_AFTER_CH:
 		enable_basic_clocks();
 		timer_init();
+		/* Fall through */
+	}
+}
+
+void prcm_init(void)
+{
+	switch (omap_hw_init_context()) {
+	case OMAP_INIT_CONTEXT_SPL:
+	case OMAP_INIT_CONTEXT_UBOOT_FROM_NOR:
+	case OMAP_INIT_CONTEXT_UBOOT_AFTER_CH:
 		scale_vcores(*omap_vcores);
 		setup_dplls();
 		setup_warmreset_time();
diff --git a/arch/arm/cpu/armv7/omap-common/hwinit-common.c b/arch/arm/cpu/armv7/omap-common/hwinit-common.c
index 80794f9c611a..91f2dead364b 100644
--- a/arch/arm/cpu/armv7/omap-common/hwinit-common.c
+++ b/arch/arm/cpu/armv7/omap-common/hwinit-common.c
@@ -125,10 +125,9 @@ void s_init(void)
 	set_mux_conf_regs();
 #ifdef CONFIG_SPL_BUILD
 	srcomp_enable();
-	setup_clocks_for_console();
-
 	do_io_settings();
 #endif
+	setup_early_clocks();
 	prcm_init();
 }
 
diff --git a/arch/arm/include/asm/arch-omap4/sys_proto.h b/arch/arm/include/asm/arch-omap4/sys_proto.h
index f30f86539130..71e3d776aa0d 100644
--- a/arch/arm/include/asm/arch-omap4/sys_proto.h
+++ b/arch/arm/include/asm/arch-omap4/sys_proto.h
@@ -37,7 +37,7 @@ void do_set_mux(u32 base, struct pad_conf_entry const *array, int size);
 void set_muxconf_regs_essential(void);
 u32 wait_on_value(u32, u32, void *, u32);
 void sdelay(unsigned long);
-void setup_clocks_for_console(void);
+void setup_early_clocks(void);
 void prcm_init(void);
 void bypass_dpll(u32 const base);
 void freq_update_core(void);
diff --git a/arch/arm/include/asm/arch-omap5/sys_proto.h b/arch/arm/include/asm/arch-omap5/sys_proto.h
index 7fcb78389403..b9e09e7c52a8 100644
--- a/arch/arm/include/asm/arch-omap5/sys_proto.h
+++ b/arch/arm/include/asm/arch-omap5/sys_proto.h
@@ -48,7 +48,7 @@ void do_set_mux32(u32 base, struct pad_conf_entry const *array, int size);
 void set_muxconf_regs_essential(void);
 u32 wait_on_value(u32, u32, void *, u32);
 void sdelay(unsigned long);
-void setup_clocks_for_console(void);
+void setup_early_clocks(void);
 void prcm_init(void);
 void bypass_dpll(u32 const base);
 void freq_update_core(void);
-- 
1.9.1

[U-Boot] [PATCH v2 2/5] ARM: OMAP4/5: Centralize gpi2c_init

[Steve Kipisz]

Centralize gpi2c_init into omap_common from the sys_proto header so
that the information can be reused across SoCs.

Signed-off-by: Steve Kipisz <s-kipisz2@ti.com>
---
v2 Based on:
 master      a6104737 ARM: at91: sama5: change the environment address to 0x6000

Changes in v2:
 - New Patch

 arch/arm/include/asm/arch-omap4/sys_proto.h | 1 -
 arch/arm/include/asm/arch-omap5/sys_proto.h | 1 -
 arch/arm/include/asm/omap_common.h          | 3 +++
 3 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/arm/include/asm/arch-omap4/sys_proto.h b/arch/arm/include/asm/arch-omap4/sys_proto.h
index 71e3d776aa0d..26e9a194f036 100644
--- a/arch/arm/include/asm/arch-omap4/sys_proto.h
+++ b/arch/arm/include/asm/arch-omap4/sys_proto.h
@@ -51,7 +51,6 @@ void save_omap_boot_params(void);
 void init_omap_revision(void);
 void do_io_settings(void);
 void sri2c_init(void);
-void gpi2c_init(void);
 int omap_vc_bypass_send_value(u8 sa, u8 reg_addr, u8 reg_data);
 u32 warm_reset(void);
 void force_emif_self_refresh(void);
diff --git a/arch/arm/include/asm/arch-omap5/sys_proto.h b/arch/arm/include/asm/arch-omap5/sys_proto.h
index b9e09e7c52a8..18902628739b 100644
--- a/arch/arm/include/asm/arch-omap5/sys_proto.h
+++ b/arch/arm/include/asm/arch-omap5/sys_proto.h
@@ -62,7 +62,6 @@ void save_omap_boot_params(void);
 void init_omap_revision(void);
 void do_io_settings(void);
 void sri2c_init(void);
-void gpi2c_init(void);
 int omap_vc_bypass_send_value(u8 sa, u8 reg_addr, u8 reg_data);
 u32 warm_reset(void);
 void force_emif_self_refresh(void);
diff --git a/arch/arm/include/asm/omap_common.h b/arch/arm/include/asm/omap_common.h
index d7b81c101b79..d773b0430ad4 100644
--- a/arch/arm/include/asm/omap_common.h
+++ b/arch/arm/include/asm/omap_common.h
@@ -617,6 +617,9 @@ void disable_edma3_clocks(void);
 
 void omap_die_id(unsigned int *die_id);
 
+/* Initialize general purpose I2C(0) on the SoC */
+void gpi2c_init(void);
+
 /* ABB */
 #define OMAP_ABB_NOMINAL_OPP		0
 #define OMAP_ABB_FAST_OPP		1
-- 
1.9.1

[U-Boot] [PATCH v2 3/5] ARM: omap-common: Add standard access for board description EEPROM

[Steve Kipisz]

From: Lokesh Vutla <lokeshvutla@ti.com>

Several TI EVMs have EEPROM that can contain board description information
such as revision, DDR definition, serial number, etc. In just about all
cases, these EEPROM are on the I2C bus and provides us the opportunity
to centralize the generic operations involved.

The on-board EEPROM on the BeagleBone Black, BeagleBone, AM335x EVM,
AM43x GP EVM, AM57xx-evm, BeagleBoard-X15 share the same format.
However, DRA-7* EVMs, OMAP4SDP use a modified format.

We hence introduce logic which is generic between these platforms
without enforcing any specific format. This allows the boards to use the
relevant format for operations that they might choose.

It is important to note that this logic is fundamental to the board
configuration process such as DDR configuration which is needed in
SPL, hence cannot be part of the standard u-boot driver model (which
is available later in the process). Hence, to aid efficiency, the
eeprom contents are copied over to SRAM scratchpad memory area at the
first invocation to retrieve data.

The follow on patches introduce the use of this library for AM57x
platform support. AM335x/AM43xx cleanups need to first ensure usage of
omap_common prior to switch over to this generic solution.

Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
Signed-off-by: Steve Kipisz <s-kipisz2@ti.com>
---
v2 Based on
 master      a6104737  ARM: at91: sama5: change the environment address to 0x6000

Changes in v2 (since v1)
	- make the EEPROM code mor generic for TI EVMs
	- rename structures/subroutines to ti_am_xxxxx
	- add routines to access the EEPROM data
	- redo commit message to be more clear

v1:   http://marc.info/?t=144608007900001&r=1&w=2
	(mailing list squashed original submission)

 arch/arm/cpu/armv7/omap-common/Makefile        |   1 +
 arch/arm/cpu/armv7/omap-common/ti-i2c-eeprom.c | 148 +++++++++++++++++++++++++
 arch/arm/include/asm/omap_common.h             | 130 +++++++++++++++++++++-
 3 files changed, 278 insertions(+), 1 deletion(-)
 create mode 100644 arch/arm/cpu/armv7/omap-common/ti-i2c-eeprom.c

diff --git a/arch/arm/cpu/armv7/omap-common/Makefile b/arch/arm/cpu/armv7/omap-common/Makefile
index 464a5d1d732a..53a9fdb81100 100644
--- a/arch/arm/cpu/armv7/omap-common/Makefile
+++ b/arch/arm/cpu/armv7/omap-common/Makefile
@@ -15,6 +15,7 @@ obj-y	+= clocks-common.o
 obj-y	+= emif-common.o
 obj-y	+= vc.o
 obj-y	+= abb.o
+obj-$(CONFIG_I2C) += ti-i2c-eeprom.o
 endif
 
 ifneq ($(CONFIG_OMAP54XX),)
diff --git a/arch/arm/cpu/armv7/omap-common/ti-i2c-eeprom.c b/arch/arm/cpu/armv7/omap-common/ti-i2c-eeprom.c
new file mode 100644
index 000000000000..f59ebbdb4ee8
--- /dev/null
+++ b/arch/arm/cpu/armv7/omap-common/ti-i2c-eeprom.c
@@ -0,0 +1,148 @@
+/*
+ * Library to support early TI EVM EEPROM handling
+ *
+ * Copyright (C) 2015 Texas Instruments Incorporated - http://www.ti.com/
+ *	Lokesh Vutla
+ *	Steve Kipisz
+ *
+ * SPDX-License-Identifier:    GPL-2.0+
+ */
+
+#include <common.h>
+#include <asm/omap_common.h>
+#include <i2c.h>
+
+/**
+ * ti_i2c_eeprom_init - Initialize an i2c bus and probe for a device
+ * @i2c_bus: i2c bus number to initialize
+ * @dev_addr: Device address to probe for
+ *
+ * Return: 0 on success or corresponding error on failure.
+ */
+static inline int ti_i2c_eeprom_init(int i2c_bus, int dev_addr)
+{
+	int rc;
+
+	rc = i2c_set_bus_num(i2c_bus);
+	if (rc)
+		return rc;
+
+	return i2c_probe(dev_addr);
+}
+
+/**
+ * ti_i2c_eeprom_read - Read data from an EEPROM
+ * @dev_addr: The device address of the EEPROM
+ * @offset: Offset to start reading in the EEPROM
+ * @ep: Pointer to a buffer to read into
+ * @epsize: Size of buffer
+ *
+ * Return: 0 on success or corresponding result of i2c_read
+ */
+static inline int ti_i2c_eeprom_read(int dev_addr, int offset, uchar *ep,
+				     int epsize)
+{
+	return i2c_read(dev_addr, offset, 2, ep, epsize);
+}
+
+/**
+ * ti_eeprom_string_cleanup() - Handle eeprom programming errors
+ * @s:	eeprom string (should be NULL terminated)
+ *
+ * Some Board manufacturers do not add a NULL termination at the
+ * end of string, instead some binary information is kludged in, hence
+ * convert the string to just printable characters of ASCII chart.
+ */
+static inline void ti_eeprom_string_cleanup(char *s)
+{
+	int i, l;
+
+	l = strlen(s);
+	for (i = 0; i < l; i++, s++)
+		if (*s < ' ' || *s > '~') {
+			*s = 0;
+			break;
+		}
+}
+
+int __maybe_unused ti_i2c_eeprom_am_get(int bus_addr, int dev_addr,
+					struct ti_am_eeprom **epp)
+{
+	int rc;
+	struct ti_am_eeprom *ep;
+
+	if (!epp)
+		return -1;
+
+	ep = TI_AM_EEPROM_DATA;
+	if (ep->header == TI_EEPROM_HEADER_MAGIC)
+		goto already_read;
+
+	/* Initialize with a known bad marker for i2c fails.. */
+	ep->header = 0xADEAD12C;
+
+	gpi2c_init();
+	rc = ti_i2c_eeprom_init(bus_addr, dev_addr);
+	if (rc)
+		return rc;
+	rc = i2c_read(dev_addr, 0x0, 2, (uint8_t *)ep, sizeof(*ep));
+	if (rc)
+		return rc;
+
+	/* Corrupted data??? */
+	if (ep->header != TI_EEPROM_HEADER_MAGIC)
+		return -1;
+
+already_read:
+	*epp = ep;
+
+	return 0;
+}
+
+int __maybe_unused ti_i2c_eeprom_am_get_print(int bus_addr, int dev_addr,
+					      struct ti_am_eeprom_printable *p)
+{
+	struct ti_am_eeprom *ep;
+	int rc;
+
+	/* Incase of invalid eeprom contents */
+	p->name[0] = 0x00;
+	p->version[0] = 0x00;
+	p->serial[0] = 0x00;
+
+	rc = ti_i2c_eeprom_am_get(bus_addr, dev_addr, &ep);
+	if (rc)
+		return rc;
+
+	/*
+	 * Alas! we have to null terminate and cleanup the strings!
+	 */
+	strlcpy(p->name, ep->name, TI_EEPROM_HDR_NAME_LEN);
+	ti_eeprom_string_cleanup(p->name);
+	strlcpy(p->version, ep->version, TI_EEPROM_HDR_NAME_LEN);
+	ti_eeprom_string_cleanup(p->version);
+	strlcpy(p->serial, ep->serial, TI_EEPROM_HDR_NAME_LEN);
+	ti_eeprom_string_cleanup(p->serial);
+	return 0;
+}
+
+void __maybe_unused set_board_info_env(char *name, char *default_name,
+				       char *revision, char *serial)
+{
+	char *unknown = "unknown";
+
+	if (name)
+		setenv("board_name", name);
+	else
+		setenv("board_name", default_name);
+
+	if (revision)
+		setenv("board_revision", revision);
+	else
+		setenv("board_revision", unknown);
+
+	if (serial)
+		setenv("board_serial", serial);
+	else
+		setenv("board_serial", unknown);
+}
diff --git a/arch/arm/include/asm/omap_common.h b/arch/arm/include/asm/omap_common.h
index d773b0430ad4..a76c67a85d37 100644
--- a/arch/arm/include/asm/omap_common.h
+++ b/arch/arm/include/asm/omap_common.h
@@ -713,7 +713,9 @@ static inline u8 is_dra72x(void)
 #define OMAP_SRAM_SCRATCH_VCORES_PTR    (SRAM_SCRATCH_SPACE_ADDR + 0x1C)
 #define OMAP_SRAM_SCRATCH_SYS_CTRL	(SRAM_SCRATCH_SPACE_ADDR + 0x20)
 #define OMAP_SRAM_SCRATCH_BOOT_PARAMS	(SRAM_SCRATCH_SPACE_ADDR + 0x24)
-#define OMAP5_SRAM_SCRATCH_SPACE_END	(SRAM_SCRATCH_SPACE_ADDR + 0x28)
+#define OMAP_SRAM_SCRATCH_BOARD_EEPROM_START (SRAM_SCRATCH_SPACE_ADDR + 0x28)
+#define OMAP_SRAM_SCRATCH_BOARD_EEPROM_END (SRAM_SCRATCH_SPACE_ADDR + 0x200)
+#define OMAP_SRAM_SCRATCH_SPACE_END	(OMAP_SRAM_SCRATCH_BOARD_EEPROM_END)
 
 /* Boot parameters */
 #define DEVICE_DATA_OFFSET	0x18
@@ -728,4 +730,130 @@ static inline u8 is_dra72x(void)
 u32 omap_sys_boot_device(void);
 #endif
 
+/* Common I2C eeprom definitions */
+#ifndef __ASSEMBLY__
+
+/* TI EEPROM MAGIC Header identifier */
+#define TI_EEPROM_HEADER_MAGIC	0xEE3355AA
+
+#define TI_EEPROM_HDR_NAME_LEN		8
+#define TI_EEPROM_HDR_REV_LEN		12
+#define TI_EEPROM_HDR_SERIAL_LEN	4
+#define TI_EEPROM_HDR_CONFIG_LEN	32
+#define TI_EEPROM_HDR_NO_OF_MAC_ADDR	3
+#define TI_EEPROM_HDR_ETH_ALEN		6
+
+/**
+ * struct ti_am_eeprom - This structure holds data read in from the
+ *                     AM335x, AM437x, AM57xx TI EVM EEPROMs.
+ * @header: This holds the magic number
+ * @name: The name of the board
+ * @version: Board revision
+ * @serial: Board serial number
+ * @config: Reserved
+ * @mac_addr: Any MAC addresses written in the EEPROM
+ *
+ * The data is this structure is read from the EEPROM on the board.
+ * It is used for board detection which is based on name. It is used
+ * to configure specific TI boards. This allows booting of multiple
+ * TI boards with a single MLO and u-boot.
+ */
+struct ti_am_eeprom {
+	unsigned int header;
+	char name[TI_EEPROM_HDR_NAME_LEN];
+	char version[TI_EEPROM_HDR_REV_LEN];
+	char serial[TI_EEPROM_HDR_SERIAL_LEN];
+	char config[TI_EEPROM_HDR_CONFIG_LEN];
+	char mac_addr[TI_EEPROM_HDR_NO_OF_MAC_ADDR][TI_EEPROM_HDR_ETH_ALEN];
+} __attribute__ ((__packed__));
+
+/**
+ * struct ti_am_eeprom_printable - Null terminated, printable EEPROM contents
+ * @name:	NULL terminated name
+ * @version:	NULL terminated version
+ * @serial:	NULL terminated serial number
+ */
+struct ti_am_eeprom_printable {
+	char name[TI_EEPROM_HDR_NAME_LEN + 1];
+	char version[TI_EEPROM_HDR_REV_LEN + 1];
+	char serial[TI_EEPROM_HDR_SERIAL_LEN + 1];
+};
+#define TI_AM_EEPROM_DATA ((struct ti_am_eeprom *)\
+				OMAP_SRAM_SCRATCH_BOARD_EEPROM_START)
+
+/**
+ * ti_i2c_eeprom_am_get() - Consolidated eeprom data collection for AM* TI EVMs
+ * @bus_addr:	I2C bus address
+ * @dev_addr:	I2C slave address
+ * @ep:		Pointer to eeprom structure
+ *
+ * *ep is populated by the this AM generic function that consolidates
+ * the basic initialization logic common accross all AM* platforms.
+ */
+int ti_i2c_eeprom_am_get(int bus_addr, int dev_addr, struct ti_am_eeprom **epp);
+
+/**
+ * ti_i2c_eeprom_am_get_print() - Get a printable representation of eeprom data
+ * @bus_addr:	I2C bus address
+ * @dev_addr:	I2C slave address
+ * @p:		Pointer to eeprom structure
+ *
+ * This reads the eeprom and converts the data into a printable string for
+ * further processing.
+ */
+int ti_i2c_eeprom_am_get_print(int bus_addr, int dev_addr,
+			       struct ti_am_eeprom_printable *p);
+
+/**
+ * set_board_info_env() - Setup commonly used board information environment vars
+ * @name:	Name of the board
+ * @default_name: In case of empty string, what name to use?
+ * @revision:	Revision of the board
+ * @serial:	Serial Number of the board
+ *
+ * In case of NULL revision or serial information "unknown" is setup.
+ * If name is NULL, default_name is used.
+ */
+void set_board_info_env(char *name, char *default_name,
+			char *revision, char *serial);
+
+/**
+ * board_am_is() - Generic Board detection logic
+ * @name_tag:	Tag used in eeprom for the board
+ *
+ * Return: false if board information does not match OR eeprom was'nt read.
+ *	   true otherwise
+ */
+static inline bool board_am_is(char *name_tag)
+{
+	struct ti_am_eeprom *ep = TI_AM_EEPROM_DATA;
+
+	if (ep->header != TI_EEPROM_HEADER_MAGIC)
+		return false;
+	return !strncmp(ep->name, name_tag, TI_EEPROM_HDR_NAME_LEN);
+}
+
+/**
+ * board_am_rev_is() - Compare board revision
+ * @rev_tag:	Revision tag to check in eeprom
+ * @cmp_len:	How many chars to compare?
+ *
+ * NOTE: revision information is often messed up (hence the str len match) :(
+ *
+ * Return: false if board information does not match OR eeprom was'nt read.
+ *	   true otherwise
+ */
+static inline bool board_am_rev_is(char *rev_tag, int cmp_len)
+{
+	struct ti_am_eeprom *ep = TI_AM_EEPROM_DATA;
+	int l;
+
+	if (ep->header != TI_EEPROM_HEADER_MAGIC)
+		return false;
+
+	l = cmp_len > TI_EEPROM_HDR_REV_LEN ? TI_EEPROM_HDR_NAME_LEN : cmp_len;
+	return !strncmp(ep->version, rev_tag, l);
+}
+#endif	/* __ASSEMBLY__ */
+
 #endif /* _OMAP_COMMON_H_ */
-- 
1.9.1

[U-Boot] [PATCH v2 4/5] ARM: OMAP4/5: Add generic board detection hook

[Steve Kipisz]

Many TI EVMs have capability to store relevant board information
such as DDR description in EEPROM. Further many pad configuration
variations can occur as part of revision changes in the platform.
In-order to support these at runtime, we for a board detection hook
which is available for override from board files that may desire to do
so.

NOTE: All TI EVMs are capable of detecting board information based on
early clocks that are configured. However, in case of additional needs
this can be achieved within the override logic from within the board
file.

Signed-off-by: Steve Kipisz <s-kipisz2@ti.com>
---
v2 Based on:
 master     a61047370d0b73ab886c5863e952695b5ee0d75b

Changes in v2:
 - New patch

 arch/arm/cpu/armv7/omap-common/hwinit-common.c | 11 +++++++++++
 arch/arm/include/asm/arch-omap4/sys_proto.h    |  1 +
 arch/arm/include/asm/arch-omap5/sys_proto.h    |  1 +
 3 files changed, 13 insertions(+)

diff --git a/arch/arm/cpu/armv7/omap-common/hwinit-common.c b/arch/arm/cpu/armv7/omap-common/hwinit-common.c
index 91f2dead364b..9e9376d0e6e6 100644
--- a/arch/arm/cpu/armv7/omap-common/hwinit-common.c
+++ b/arch/arm/cpu/armv7/omap-common/hwinit-common.c
@@ -97,6 +97,16 @@ int arch_cpu_init(void)
 }
 #endif /* CONFIG_ARCH_CPU_INIT */
 
+/**
+ * do_board_detect() - Detect board description
+ *
+ * Function to detect board description. This is expected to be
+ * overridden in the SoC family board file where desired.
+ */
+void __weak do_board_detect(void)
+{
+}
+
 /*
  * Routine: s_init
  * Description: Does early system init of watchdog, muxing,  andclocks
@@ -128,6 +138,7 @@ void s_init(void)
 	do_io_settings();
 #endif
 	setup_early_clocks();
+	do_board_detect();
 	prcm_init();
 }
 
diff --git a/arch/arm/include/asm/arch-omap4/sys_proto.h b/arch/arm/include/asm/arch-omap4/sys_proto.h
index 26e9a194f036..fbb52093c65a 100644
--- a/arch/arm/include/asm/arch-omap4/sys_proto.h
+++ b/arch/arm/include/asm/arch-omap4/sys_proto.h
@@ -39,6 +39,7 @@ u32 wait_on_value(u32, u32, void *, u32);
 void sdelay(unsigned long);
 void setup_early_clocks(void);
 void prcm_init(void);
+void do_board_detect(void);
 void bypass_dpll(u32 const base);
 void freq_update_core(void);
 u32 get_sys_clk_freq(void);
diff --git a/arch/arm/include/asm/arch-omap5/sys_proto.h b/arch/arm/include/asm/arch-omap5/sys_proto.h
index 18902628739b..23a33cb233bb 100644
--- a/arch/arm/include/asm/arch-omap5/sys_proto.h
+++ b/arch/arm/include/asm/arch-omap5/sys_proto.h
@@ -50,6 +50,7 @@ u32 wait_on_value(u32, u32, void *, u32);
 void sdelay(unsigned long);
 void setup_early_clocks(void);
 void prcm_init(void);
+void do_board_detect(void);
 void bypass_dpll(u32 const base);
 void freq_update_core(void);
 u32 get_sys_clk_freq(void);
-- 
1.9.1

[U-Boot] [PATCH v2 5/5] board: ti: AM57xx: Add detection logic for AM57xx-evm

[Steve Kipisz]

Current AM57xx evm supports both BeagleBoard-X15
(http://beagleboard.org/x15) and AM57xx EVM
(http://www.ti.com/tool/tmdxevm5728).

The AM572x EValuation Module(EVM) provides an affordable platform to
quickly start evaluation of Sitara. ARM Cortex-A15 AM57x Processors
(AM5728, AM5726, AM5718, AM5716) and accelerate development for HMI,
machine vision, networking, medical imaging and many other industrial
applications. This EVM is based on the same BeagleBoard-X15 Chassis
and adds mPCIe, mSATA, LCD, touchscreen, Camera, push button and TI's
wlink8 offering.

Since the EEPROM contents are compatible between the BeagleBoard-X15 and
the AM57xx-evm, we add support for the detection logic to enable
support for various user programmable scripting capability.

NOTE: U-boot configuration is currently a superset of AM57xx evm and
BeagleBoard-X15 and no additional configuration tweaking is needed.

This change also sets up the stage for future support of TI AM57xx EVMs
to the same base bootloader build.

Signed-off-by: Steve Kipisz <s-kipisz2@ti.com>
---
v2 Based on:
 master     a6104737 ARM: at91: sama5: change the environment address to 0x6000

Build testing: MAKEALL -s omap4 -s omap5 (no warnings/build errors)
	Boot Testing:
	am57xx_evm_nodt_config: http://pastebin.ubuntu.com/13039296/
	beagle_x15_config: http://pastebin.ubuntu.com/13039331/

Changes in v2 (since v1):
	- move the board detection code into the new routine
	  do_board_detect
	- eliminate board.h and move the ix_xxx into board.c
	- redo commit message to be more clear

v1:  http://marc.info/?t=144608007900002&r=1&w=2
     http://marc.info/?t=144608007900004&r=1&w=2
	(mailing list squashed original submission)

 board/ti/am57xx/board.c           | 53 +++++++++++++++++++++++++++++++++++++++
 include/configs/am57xx_evm.h      |  4 +++
 include/configs/ti_omap5_common.h |  2 ++
 3 files changed, 59 insertions(+)

diff --git a/board/ti/am57xx/board.c b/board/ti/am57xx/board.c
index 042f9ab1965a..41dd8333b1eb 100644
--- a/board/ti/am57xx/board.c
+++ b/board/ti/am57xx/board.c
@@ -32,6 +32,9 @@
 
 #include "mux_data.h"
 
+#define is_x15()	board_am_is("BBRDX15_")
+#define is_am572x_evm()	board_am_is("AM572PM_")
+
 #ifdef CONFIG_DRIVER_TI_CPSW
 #include <cpsw.h>
 #endif
@@ -246,6 +249,54 @@ struct vcores_data beagle_x15_volts = {
 	.iva.pmic		= &tps659038,
 };
 
+#ifdef CONFIG_SPL_BUILD
+/* No env to setup for SPL */
+static inline void setup_board_eeprom_env(void) { }
+
+/* Override function to read eeprom information */
+void do_board_detect(void)
+{
+	struct ti_am_eeprom *ep;
+	int rc;
+
+	rc = ti_i2c_eeprom_am_get(CONFIG_EEPROM_BUS_ADDRESS,
+				  CONFIG_EEPROM_CHIP_ADDRESS, &ep);
+	if (rc)
+		printf("ti_i2c_eeprom_init failed %d\n", rc);
+}
+
+#else	/* CONFIG_SPL_BUILD */
+
+static void setup_board_eeprom_env(void)
+{
+	char *name = NULL;
+	int rc;
+	struct ti_am_eeprom_printable p;
+
+	rc = ti_i2c_eeprom_am_get_print(CONFIG_EEPROM_BUS_ADDRESS,
+					CONFIG_EEPROM_CHIP_ADDRESS, &p);
+	if (rc) {
+		printf("Invalid EEPROM data(@0x%p). Default to X15\n",
+		       TI_AM_EEPROM_DATA);
+		goto invalid_eeprom;
+	}
+
+	if (is_x15())
+		name = "beagle_x15";
+	else if (is_am572x_evm())
+		name = "am57xx_evm";
+	else
+		printf("Unidentified board claims %s in eeprom header\n",
+		       p.name);
+
+invalid_eeprom:
+	set_board_info_env(name, "beagle_x15", p.version, p.serial);
+}
+
+/* Eeprom is alread read by SPL.. nothing more to do here.. */
+
+#endif	/* CONFIG_SPL_BUILD */
+
 void hw_data_init(void)
 {
 	*prcm = &dra7xx_prcm;
@@ -265,6 +316,8 @@ int board_init(void)
 int board_late_init(void)
 {
 	init_sata(0);
+	setup_board_eeprom_env();
+
 	/*
 	 * DEV_CTRL.DEV_ON = 1 please - else palmas switches off in 8 seconds
 	 * This is the POWERHOLD-in-Low behavior.
diff --git a/include/configs/am57xx_evm.h b/include/configs/am57xx_evm.h
index 6308cab8e680..1fffdb18fbcd 100644
--- a/include/configs/am57xx_evm.h
+++ b/include/configs/am57xx_evm.h
@@ -88,4 +88,8 @@
 #define CONFIG_SYS_SCSI_MAX_DEVICE	(CONFIG_SYS_SCSI_MAX_SCSI_ID * \
 						CONFIG_SYS_SCSI_MAX_LUN)
 
+/* EEPROM */
+#define CONFIG_EEPROM_CHIP_ADDRESS 0x50
+#define CONFIG_EEPROM_BUS_ADDRESS 0
+
 #endif /* __CONFIG_AM57XX_EVM_H */
diff --git a/include/configs/ti_omap5_common.h b/include/configs/ti_omap5_common.h
index 5acbc92c3f60..ae6e2a556a93 100644
--- a/include/configs/ti_omap5_common.h
+++ b/include/configs/ti_omap5_common.h
@@ -120,6 +120,8 @@
 			"setenv fdtfile dra72-evm.dtb; fi;" \
 		"if test $board_name = beagle_x15; then " \
 			"setenv fdtfile am57xx-beagle-x15.dtb; fi;" \
+		"if test $board_name = am57xx_evm; then " \
+			"setenv fdtfile am57xx-evm.dtb; fi;" \
 		"if test $fdtfile = undefined; then " \
 			"echo WARNING: Could not determine device tree to use; fi; \0" \
 	"loadfdt=load mmc ${bootpart} ${fdtaddr} ${bootdir}/${fdtfile};\0" \
-- 
1.9.1

[U-Boot] [PATCH v2 3/5] ARM: omap-common: Add standard access for board description EEPROM

[Igor Grinberg]

Hi Steve,

On 11/03/15 14:22, Steve Kipisz wrote:
> From: Lokesh Vutla <lokeshvutla@ti.com>

[...]

> 
> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
> Signed-off-by: Steve Kipisz <s-kipisz2@ti.com>
> ---
> v2 Based on
>  master      a6104737  ARM: at91: sama5: change the environment address to 0x6000
> 
> Changes in v2 (since v1)
> 	- make the EEPROM code mor generic for TI EVMs
> 	- rename structures/subroutines to ti_am_xxxxx
> 	- add routines to access the EEPROM data
> 	- redo commit message to be more clear
> 
> v1:   http://marc.info/?t=144608007900001&r=1&w=2
> 	(mailing list squashed original submission)
> 
>  arch/arm/cpu/armv7/omap-common/Makefile        |   1 +
>  arch/arm/cpu/armv7/omap-common/ti-i2c-eeprom.c | 148 +++++++++++++++++++++++++
>  arch/arm/include/asm/omap_common.h             | 130 +++++++++++++++++++++-
>  3 files changed, 278 insertions(+), 1 deletion(-)
>  create mode 100644 arch/arm/cpu/armv7/omap-common/ti-i2c-eeprom.c
> 
> diff --git a/arch/arm/cpu/armv7/omap-common/Makefile b/arch/arm/cpu/armv7/omap-common/Makefile
> index 464a5d1d732a..53a9fdb81100 100644
> --- a/arch/arm/cpu/armv7/omap-common/Makefile
> +++ b/arch/arm/cpu/armv7/omap-common/Makefile
> @@ -15,6 +15,7 @@ obj-y	+= clocks-common.o
>  obj-y	+= emif-common.o
>  obj-y	+= vc.o
>  obj-y	+= abb.o
> +obj-$(CONFIG_I2C) += ti-i2c-eeprom.o

This makes this module compile on all TI SoC based boards enabling I2C.
AFAIU, this is a separate chip (not inside the SoC), so this module will
also compile on non-TI boards that do not have this EEPROM.
I think, it should be more fine grained (e.g. have its own symbol).

>  endif
>  
>  ifneq ($(CONFIG_OMAP54XX),)
> diff --git a/arch/arm/cpu/armv7/omap-common/ti-i2c-eeprom.c b/arch/arm/cpu/armv7/omap-common/ti-i2c-eeprom.c
> new file mode 100644
> index 000000000000..f59ebbdb4ee8
> --- /dev/null
> +++ b/arch/arm/cpu/armv7/omap-common/ti-i2c-eeprom.c

[...]

> +void __maybe_unused set_board_info_env(char *name, char *default_name,
> +				       char *revision, char *serial)

That looks really weird API to me...
You have name and default_name? Why would you need both...

> +{
> +	char *unknown = "unknown";
> +
> +	if (name)
> +		setenv("board_name", name);
> +	else
> +		setenv("board_name", default_name);
> +
> +	if (revision)
> +		setenv("board_revision", revision);
> +	else
> +		setenv("board_revision", unknown);
> +
> +	if (serial)
> +		setenv("board_serial", serial);
> +	else
> +		setenv("board_serial", unknown);
> +}

[...]

> diff --git a/arch/arm/include/asm/omap_common.h b/arch/arm/include/asm/omap_common.h
> index d773b0430ad4..a76c67a85d37 100644
> --- a/arch/arm/include/asm/omap_common.h
> +++ b/arch/arm/include/asm/omap_common.h

[...]

> +/**
> + * set_board_info_env() - Setup commonly used board information environment vars
> + * @name:	Name of the board
> + * @default_name: In case of empty string, what name to use?

That seems redundant.
The caller knows the board name, and if it does not, then it can place
an arbitrary name (like unknown) into name parameter.

> + * @revision:	Revision of the board
> + * @serial:	Serial Number of the board
> + *
> + * In case of NULL revision or serial information "unknown" is setup.
> + * If name is NULL, default_name is used.
> + */
> +void set_board_info_env(char *name, char *default_name,
> +			char *revision, char *serial);
> +
> +/**
> + * board_am_is() - Generic Board detection logic
> + * @name_tag:	Tag used in eeprom for the board
> + *
> + * Return: false if board information does not match OR eeprom was'nt read.
> + *	   true otherwise
> + */
> +static inline bool board_am_is(char *name_tag)
> +{
> +	struct ti_am_eeprom *ep = TI_AM_EEPROM_DATA;
> +
> +	if (ep->header != TI_EEPROM_HEADER_MAGIC)
> +		return false;
> +	return !strncmp(ep->name, name_tag, TI_EEPROM_HDR_NAME_LEN);
> +}

Why do you need to place non trivial function implementation inside the
header file?

> +
> +/**
> + * board_am_rev_is() - Compare board revision
> + * @rev_tag:	Revision tag to check in eeprom
> + * @cmp_len:	How many chars to compare?
> + *
> + * NOTE: revision information is often messed up (hence the str len match) :(
> + *
> + * Return: false if board information does not match OR eeprom was'nt read.
> + *	   true otherwise
> + */
> +static inline bool board_am_rev_is(char *rev_tag, int cmp_len)
> +{
> +	struct ti_am_eeprom *ep = TI_AM_EEPROM_DATA;
> +	int l;
> +
> +	if (ep->header != TI_EEPROM_HEADER_MAGIC)
> +		return false;
> +
> +	l = cmp_len > TI_EEPROM_HDR_REV_LEN ? TI_EEPROM_HDR_NAME_LEN : cmp_len;
> +	return !strncmp(ep->version, rev_tag, l);
> +}

Same here.

> +#endif	/* __ASSEMBLY__ */
> +
>  #endif /* _OMAP_COMMON_H_ */
> 

-- 
Regards,
Igor.

[U-Boot] [PATCH v2 5/5] board: ti: AM57xx: Add detection logic for AM57xx-evm

[Igor Grinberg]

Hi Steve,

On 11/03/15 14:22, Steve Kipisz wrote:

[...]

> Signed-off-by: Steve Kipisz <s-kipisz2@ti.com>
> ---
> v2 Based on:
>  master     a6104737 ARM: at91: sama5: change the environment address to 0x6000
> 
> Build testing: MAKEALL -s omap4 -s omap5 (no warnings/build errors)
> 	Boot Testing:
> 	am57xx_evm_nodt_config: http://pastebin.ubuntu.com/13039296/
> 	beagle_x15_config: http://pastebin.ubuntu.com/13039331/
> 
> Changes in v2 (since v1):
> 	- move the board detection code into the new routine
> 	  do_board_detect
> 	- eliminate board.h and move the ix_xxx into board.c
> 	- redo commit message to be more clear
> 
> v1:  http://marc.info/?t=144608007900002&r=1&w=2
>      http://marc.info/?t=144608007900004&r=1&w=2
> 	(mailing list squashed original submission)

[...]

> +#define is_x15()	board_am_is("BBRDX15_")
> +#define is_am572x_evm()	board_am_is("AM572PM_")

I think board_is_* much more appropriate here...

> +
>  #ifdef CONFIG_DRIVER_TI_CPSW
>  #include <cpsw.h>
>  #endif
> @@ -246,6 +249,54 @@ struct vcores_data beagle_x15_volts = {
>  	.iva.pmic		= &tps659038,
>  };
>  
> +#ifdef CONFIG_SPL_BUILD
> +/* No env to setup for SPL */
> +static inline void setup_board_eeprom_env(void) { }
> +
> +/* Override function to read eeprom information */
> +void do_board_detect(void)
> +{
> +	struct ti_am_eeprom *ep;
> +	int rc;
> +
> +	rc = ti_i2c_eeprom_am_get(CONFIG_EEPROM_BUS_ADDRESS,
> +				  CONFIG_EEPROM_CHIP_ADDRESS, &ep);
> +	if (rc)
> +		printf("ti_i2c_eeprom_init failed %d\n", rc);
> +}

Do you really need this in SPL?

> +
> +#else	/* CONFIG_SPL_BUILD */
> +
> +static void setup_board_eeprom_env(void)
> +{
> +	char *name = NULL;

How about:

	char *name = "beagle_x15";

> +	int rc;
> +	struct ti_am_eeprom_printable p;
> +
> +	rc = ti_i2c_eeprom_am_get_print(CONFIG_EEPROM_BUS_ADDRESS,
> +					CONFIG_EEPROM_CHIP_ADDRESS, &p);
> +	if (rc) {
> +		printf("Invalid EEPROM data(@0x%p). Default to X15\n",
> +		       TI_AM_EEPROM_DATA);
> +		goto invalid_eeprom;
> +	}
> +
> +	if (is_x15())
> +		name = "beagle_x15";

This will not be needed if the above comment is implemented.

> +	else if (is_am572x_evm())
> +		name = "am57xx_evm";
> +	else
> +		printf("Unidentified board claims %s in eeprom header\n",
> +		       p.name);
> +
> +invalid_eeprom:
> +	set_board_info_env(name, "beagle_x15", p.version, p.serial);

If the above comment is implemented, no more need for the
default_name parameter...

> +}
> +
> +/* Eeprom is alread read by SPL.. nothing more to do here.. */
> +
> +#endif	/* CONFIG_SPL_BUILD */

[...]


-- 
Regards,
Igor.

[U-Boot] [PATCH v2 5/5] board: ti: AM57xx: Add detection logic for AM57xx-evm

[Steven Kipisz]

On 11/03/2015 07:29 AM, Igor Grinberg wrote:
> Hi Steve,
>
> On 11/03/15 14:22, Steve Kipisz wrote:
>
> [...]
>
>> Signed-off-by: Steve Kipisz <s-kipisz2@ti.com>
>> ---
>> v2 Based on:
>>   master     a6104737 ARM: at91: sama5: change the environment address to 0x6000
>>
>> Build testing: MAKEALL -s omap4 -s omap5 (no warnings/build errors)
>> 	Boot Testing:
>> 	am57xx_evm_nodt_config: http://pastebin.ubuntu.com/13039296/
>> 	beagle_x15_config: http://pastebin.ubuntu.com/13039331/
>>
>> Changes in v2 (since v1):
>> 	- move the board detection code into the new routine
>> 	  do_board_detect
>> 	- eliminate board.h and move the ix_xxx into board.c
>> 	- redo commit message to be more clear
>>
>> v1:  http://marc.info/?t=144608007900002&r=1&w=2
>>       http://marc.info/?t=144608007900004&r=1&w=2
>> 	(mailing list squashed original submission)
>
> [...]
>
>> +#define is_x15()	board_am_is("BBRDX15_")
>> +#define is_am572x_evm()	board_am_is("AM572PM_")
>
> I think board_is_* much more appropriate here...
>
Ok. so board_is_x15 and board_is_am572x_evm
>> +
>>   #ifdef CONFIG_DRIVER_TI_CPSW
>>   #include <cpsw.h>
>>   #endif
>> @@ -246,6 +249,54 @@ struct vcores_data beagle_x15_volts = {
>>   	.iva.pmic		= &tps659038,
>>   };
>>
>> +#ifdef CONFIG_SPL_BUILD
>> +/* No env to setup for SPL */
>> +static inline void setup_board_eeprom_env(void) { }
>> +
>> +/* Override function to read eeprom information */
>> +void do_board_detect(void)
>> +{
>> +	struct ti_am_eeprom *ep;
>> +	int rc;
>> +
>> +	rc = ti_i2c_eeprom_am_get(CONFIG_EEPROM_BUS_ADDRESS,
>> +				  CONFIG_EEPROM_CHIP_ADDRESS, &ep);
>> +	if (rc)
>> +		printf("ti_i2c_eeprom_init failed %d\n", rc);
>> +}
>
> Do you really need this in SPL?

Yes. We need to detect the board to determine DDR setup, pin mux, 
iodelay. All of that needs to be done in SPL. X15 and EVM are the same, 
but more boards will be added that have some differences.
>
>> +
>> +#else	/* CONFIG_SPL_BUILD */
>> +
>> +static void setup_board_eeprom_env(void)
>> +{
>> +	char *name = NULL;
>
> How about:
>
> 	char *name = "beagle_x15";
>
>> +	int rc;
>> +	struct ti_am_eeprom_printable p;
>> +
>> +	rc = ti_i2c_eeprom_am_get_print(CONFIG_EEPROM_BUS_ADDRESS,
>> +					CONFIG_EEPROM_CHIP_ADDRESS, &p);
>> +	if (rc) {
>> +		printf("Invalid EEPROM data(@0x%p). Default to X15\n",
>> +		       TI_AM_EEPROM_DATA);
>> +		goto invalid_eeprom;
>> +	}
>> +
>> +	if (is_x15())
>> +		name = "beagle_x15";
>
> This will not be needed if the above comment is implemented.
>
>> +	else if (is_am572x_evm())
>> +		name = "am57xx_evm";
>> +	else
>> +		printf("Unidentified board claims %s in eeprom header\n",
>> +		       p.name);
>> +
>> +invalid_eeprom:
>> +	set_board_info_env(name, "beagle_x15", p.version, p.serial);
>
> If the above comment is implemented, no more need for the
> default_name parameter...
>
Ok, I'll look at that.
>> +}
>> +
>> +/* Eeprom is alread read by SPL.. nothing more to do here.. */
>> +
>> +#endif	/* CONFIG_SPL_BUILD */
>
> [...]
>
>
Steve K.

[U-Boot] [PATCH v2 3/5] ARM: omap-common: Add standard access for board description EEPROM

[Steven Kipisz]

On 11/03/2015 07:16 AM, Igor Grinberg wrote:
> Hi Steve,
>
> On 11/03/15 14:22, Steve Kipisz wrote:
>> From: Lokesh Vutla <lokeshvutla@ti.com>
>
> [...]
>
>>
>> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
>> Signed-off-by: Steve Kipisz <s-kipisz2@ti.com>
>> ---
>> v2 Based on
>>   master      a6104737  ARM: at91: sama5: change the environment address to 0x6000
>>
>> Changes in v2 (since v1)
>> 	- make the EEPROM code mor generic for TI EVMs
>> 	- rename structures/subroutines to ti_am_xxxxx
>> 	- add routines to access the EEPROM data
>> 	- redo commit message to be more clear
>>
>> v1:   http://marc.info/?t=144608007900001&r=1&w=2
>> 	(mailing list squashed original submission)
>>
>>   arch/arm/cpu/armv7/omap-common/Makefile        |   1 +
>>   arch/arm/cpu/armv7/omap-common/ti-i2c-eeprom.c | 148 +++++++++++++++++++++++++
>>   arch/arm/include/asm/omap_common.h             | 130 +++++++++++++++++++++-
>>   3 files changed, 278 insertions(+), 1 deletion(-)
>>   create mode 100644 arch/arm/cpu/armv7/omap-common/ti-i2c-eeprom.c
>>
>> diff --git a/arch/arm/cpu/armv7/omap-common/Makefile b/arch/arm/cpu/armv7/omap-common/Makefile
>> index 464a5d1d732a..53a9fdb81100 100644
>> --- a/arch/arm/cpu/armv7/omap-common/Makefile
>> +++ b/arch/arm/cpu/armv7/omap-common/Makefile
>> @@ -15,6 +15,7 @@ obj-y	+= clocks-common.o
>>   obj-y	+= emif-common.o
>>   obj-y	+= vc.o
>>   obj-y	+= abb.o
>> +obj-$(CONFIG_I2C) += ti-i2c-eeprom.o
>
> This makes this module compile on all TI SoC based boards enabling I2C.
> AFAIU, this is a separate chip (not inside the SoC), so this module will
> also compile on non-TI boards that do not have this EEPROM.
> I think, it should be more fine grained (e.g. have its own symbol).
>
Can you give a suggestion?
>>   endif
>>
>>   ifneq ($(CONFIG_OMAP54XX),)
>> diff --git a/arch/arm/cpu/armv7/omap-common/ti-i2c-eeprom.c b/arch/arm/cpu/armv7/omap-common/ti-i2c-eeprom.c
>> new file mode 100644
>> index 000000000000..f59ebbdb4ee8
>> --- /dev/null
>> +++ b/arch/arm/cpu/armv7/omap-common/ti-i2c-eeprom.c
>
> [...]
>
>> +void __maybe_unused set_board_info_env(char *name, char *default_name,
>> +				       char *revision, char *serial)
>
> That looks really weird API to me...
> You have name and default_name? Why would you need both...
>
Default to beagle_x15 if the EEPROM isn't programmed. Looking at your 
other post, I think I see how to remove default_name.
>> +{
>> +	char *unknown = "unknown";
>> +
>> +	if (name)
>> +		setenv("board_name", name);
>> +	else
>> +		setenv("board_name", default_name);
>> +
>> +	if (revision)
>> +		setenv("board_revision", revision);
>> +	else
>> +		setenv("board_revision", unknown);
>> +
>> +	if (serial)
>> +		setenv("board_serial", serial);
>> +	else
>> +		setenv("board_serial", unknown);
>> +}
>
> [...]
>
>> diff --git a/arch/arm/include/asm/omap_common.h b/arch/arm/include/asm/omap_common.h
>> index d773b0430ad4..a76c67a85d37 100644
>> --- a/arch/arm/include/asm/omap_common.h
>> +++ b/arch/arm/include/asm/omap_common.h
>
> [...]
>
>> +/**
>> + * set_board_info_env() - Setup commonly used board information environment vars
>> + * @name:	Name of the board
>> + * @default_name: In case of empty string, what name to use?
>
> That seems redundant.
> The caller knows the board name, and if it does not, then it can place
> an arbitrary name (like unknown) into name parameter.
>
>> + * @revision:	Revision of the board
>> + * @serial:	Serial Number of the board
>> + *
>> + * In case of NULL revision or serial information "unknown" is setup.
>> + * If name is NULL, default_name is used.
>> + */
>> +void set_board_info_env(char *name, char *default_name,
>> +			char *revision, char *serial);
>> +
>> +/**
>> + * board_am_is() - Generic Board detection logic
>> + * @name_tag:	Tag used in eeprom for the board
>> + *
>> + * Return: false if board information does not match OR eeprom was'nt read.
>> + *	   true otherwise
>> + */
>> +static inline bool board_am_is(char *name_tag)
>> +{
>> +	struct ti_am_eeprom *ep = TI_AM_EEPROM_DATA;
>> +
>> +	if (ep->header != TI_EEPROM_HEADER_MAGIC)
>> +		return false;
>> +	return !strncmp(ep->name, name_tag, TI_EEPROM_HDR_NAME_LEN);
>> +}
>
> Why do you need to place non trivial function implementation inside the
> header file?
>
>> +
>> +/**
>> + * board_am_rev_is() - Compare board revision
>> + * @rev_tag:	Revision tag to check in eeprom
>> + * @cmp_len:	How many chars to compare?
>> + *
>> + * NOTE: revision information is often messed up (hence the str len match) :(
>> + *
>> + * Return: false if board information does not match OR eeprom was'nt read.
>> + *	   true otherwise
>> + */
>> +static inline bool board_am_rev_is(char *rev_tag, int cmp_len)
>> +{
>> +	struct ti_am_eeprom *ep = TI_AM_EEPROM_DATA;
>> +	int l;
>> +
>> +	if (ep->header != TI_EEPROM_HEADER_MAGIC)
>> +		return false;
>> +
>> +	l = cmp_len > TI_EEPROM_HDR_REV_LEN ? TI_EEPROM_HDR_NAME_LEN : cmp_len;
>> +	return !strncmp(ep->version, rev_tag, l);
>> +}
>
> Same here.
>
I thought by making them static inline would save space.
>> +#endif	/* __ASSEMBLY__ */
>> +
>>   #endif /* _OMAP_COMMON_H_ */
>>
>
Steve K.

[U-Boot] [PATCH v2 3/5] ARM: omap-common: Add standard access for board description EEPROM

[Nishanth Menon]

On 11/03/2015 09:13 AM, Steven Kipisz wrote:
> On 11/03/2015 07:16 AM, Igor Grinberg wrote:
>> Hi Steve,
>>
>> On 11/03/15 14:22, Steve Kipisz wrote:
>>> From: Lokesh Vutla <lokeshvutla@ti.com>
>>
>> [...]
>>
>>>
>>> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
>>> Signed-off-by: Steve Kipisz <s-kipisz2@ti.com>
>>> ---
>>> v2 Based on
>>>   master      a6104737  ARM: at91: sama5: change the environment
>>> address to 0x6000
>>>
>>> Changes in v2 (since v1)
>>>     - make the EEPROM code mor generic for TI EVMs
>>>     - rename structures/subroutines to ti_am_xxxxx
>>>     - add routines to access the EEPROM data
>>>     - redo commit message to be more clear
>>>
>>> v1:   http://marc.info/?t=144608007900001&r=1&w=2
>>>     (mailing list squashed original submission)
>>>
>>>   arch/arm/cpu/armv7/omap-common/Makefile        |   1 +
>>>   arch/arm/cpu/armv7/omap-common/ti-i2c-eeprom.c | 148
>>> +++++++++++++++++++++++++
>>>   arch/arm/include/asm/omap_common.h             | 130
>>> +++++++++++++++++++++-
>>>   3 files changed, 278 insertions(+), 1 deletion(-)
>>>   create mode 100644 arch/arm/cpu/armv7/omap-common/ti-i2c-eeprom.c
>>>
>>> diff --git a/arch/arm/cpu/armv7/omap-common/Makefile
>>> b/arch/arm/cpu/armv7/omap-common/Makefile
>>> index 464a5d1d732a..53a9fdb81100 100644
>>> --- a/arch/arm/cpu/armv7/omap-common/Makefile
>>> +++ b/arch/arm/cpu/armv7/omap-common/Makefile
>>> @@ -15,6 +15,7 @@ obj-y    += clocks-common.o
>>>   obj-y    += emif-common.o
>>>   obj-y    += vc.o
>>>   obj-y    += abb.o
>>> +obj-$(CONFIG_I2C) += ti-i2c-eeprom.o
>>
>> This makes this module compile on all TI SoC based boards enabling I2C.
>> AFAIU, this is a separate chip (not inside the SoC), so this module will
>> also compile on non-TI boards that do not have this EEPROM.
>> I think, it should be more fine grained (e.g. have its own symbol).
>>
> Can you give a suggestion?

Are you sure this will be built into non-ti SoCs with I2C enabled if you
are not using the function? I assume  __maybe_unused should take care of
that, no - let the compiler do the gc anyways?


It should have been documented on commit message as well.


>>> + * @revision:    Revision of the board
>>> + * @serial:    Serial Number of the board
>>> + *
>>> + * In case of NULL revision or serial information "unknown" is setup.
>>> + * If name is NULL, default_name is used.
>>> + */
>>> +void set_board_info_env(char *name, char *default_name,
>>> +            char *revision, char *serial);
>>> +
>>> +/**
>>> + * board_am_is() - Generic Board detection logic
>>> + * @name_tag:    Tag used in eeprom for the board
>>> + *
>>> + * Return: false if board information does not match OR eeprom
>>> was'nt read.
>>> + *       true otherwise
>>> + */
>>> +static inline bool board_am_is(char *name_tag)
>>> +{
>>> +    struct ti_am_eeprom *ep = TI_AM_EEPROM_DATA;
>>> +
>>> +    if (ep->header != TI_EEPROM_HEADER_MAGIC)
>>> +        return false;
>>> +    return !strncmp(ep->name, name_tag, TI_EEPROM_HDR_NAME_LEN);
>>> +}
>>
>> Why do you need to place non trivial function implementation inside the
>> header file?
>>
>>> +
>>> +/**
>>> + * board_am_rev_is() - Compare board revision
>>> + * @rev_tag:    Revision tag to check in eeprom
>>> + * @cmp_len:    How many chars to compare?
>>> + *
>>> + * NOTE: revision information is often messed up (hence the str len
>>> match) :(
>>> + *
>>> + * Return: false if board information does not match OR eeprom
>>> was'nt read.
>>> + *       true otherwise
>>> + */
>>> +static inline bool board_am_rev_is(char *rev_tag, int cmp_len)
>>> +{
>>> +    struct ti_am_eeprom *ep = TI_AM_EEPROM_DATA;
>>> +    int l;
>>> +
>>> +    if (ep->header != TI_EEPROM_HEADER_MAGIC)
>>> +        return false;
>>> +
>>> +    l = cmp_len > TI_EEPROM_HDR_REV_LEN ? TI_EEPROM_HDR_NAME_LEN :
>>> cmp_len;
>>> +    return !strncmp(ep->version, rev_tag, l);
>>> +}
>>
>> Same here.
>>
> I thought by making them static inline would save space.

I prefer that myself as well.


-- 
Regards,
Nishanth Menon

[U-Boot] [PATCH v2 5/5] board: ti: AM57xx: Add detection logic for AM57xx-evm

[Nishanth Menon]

On 11/03/2015 06:22 AM, Steve Kipisz wrote:
> diff --git a/include/configs/ti_omap5_common.h b/include/configs/ti_omap5_common.h
> index 5acbc92c3f60..ae6e2a556a93 100644
> --- a/include/configs/ti_omap5_common.h
> +++ b/include/configs/ti_omap5_common.h
> @@ -120,6 +120,8 @@
>  			"setenv fdtfile dra72-evm.dtb; fi;" \
>  		"if test $board_name = beagle_x15; then " \
>  			"setenv fdtfile am57xx-beagle-x15.dtb; fi;" \
> +		"if test $board_name = am57xx_evm; then " \
> +			"setenv fdtfile am57xx-evm.dtb; fi;" \
>  		"if test $fdtfile = undefined; then " \
>  			"echo WARNING: Could not determine device tree to use; fi; \0" \
>  	"loadfdt=load mmc ${bootpart} ${fdtaddr} ${bootdir}/${fdtfile};\0" \

Hmmm.... I might keep this nugget out of upstream code for now.
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/
does not contain am57xx-evm.

it is possible that when we post patches upstream, it might become dt
overlay instead of evm dtb. I dont know about what will eventually
become in upstream. So, I suggest dropping the above change.



Also, in the future, could you cc beagleboard-x15
<beagleboard-x15@googlegroups.com> for all patches that impact
beagleboard-X15 I am sure folks there will be interested to review as well?

-- 
Regards,
Nishanth Menon

[U-Boot] [PATCH v2 5/5] board: ti: AM57xx: Add detection logic for AM57xx-evm

[Steven Kipisz]

On 11/03/2015 09:31 AM, Nishanth Menon wrote:
> On 11/03/2015 06:22 AM, Steve Kipisz wrote:
>> diff --git a/include/configs/ti_omap5_common.h b/include/configs/ti_omap5_common.h
>> index 5acbc92c3f60..ae6e2a556a93 100644
>> --- a/include/configs/ti_omap5_common.h
>> +++ b/include/configs/ti_omap5_common.h
>> @@ -120,6 +120,8 @@
>>   			"setenv fdtfile dra72-evm.dtb; fi;" \
>>   		"if test $board_name = beagle_x15; then " \
>>   			"setenv fdtfile am57xx-beagle-x15.dtb; fi;" \
>> +		"if test $board_name = am57xx_evm; then " \
>> +			"setenv fdtfile am57xx-evm.dtb; fi;" \
>>   		"if test $fdtfile = undefined; then " \
>>   			"echo WARNING: Could not determine device tree to use; fi; \0" \
>>   	"loadfdt=load mmc ${bootpart} ${fdtaddr} ${bootdir}/${fdtfile};\0" \
>
> Hmmm.... I might keep this nugget out of upstream code for now.
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/
> does not contain am57xx-evm.
>
> it is possible that when we post patches upstream, it might become dt
> overlay instead of evm dtb. I dont know about what will eventually
> become in upstream. So, I suggest dropping the above change.
>
>
Got it.
>
> Also, in the future, could you cc beagleboard-x15
> <beagleboard-x15@googlegroups.com> for all patches that impact
> beagleboard-X15 I am sure folks there will be interested to review as well?
>
Ok. I'll add it to my send-email script.

Steve K.

[U-Boot] [PATCH v2 1/5] ARM: OMAP4/5: Centralize early clock initialization

[Tom Rini]

On Tue, Nov 03, 2015 at 06:22:38AM -0600, Steve Kipisz wrote:

> Early clock initialization is currently done in two stages for OMAP4/5
> SoCs. The first stage is the initialization of console clocks and
> then we initialize basic clocks for functionality necessary for SoC
> initialization and basic board functionality.
> 
> By splitting up prcm_init and centralizing this clock initialization,
> we setup the code for follow on patches that can do board specific
> initialization such as board detection which will depend on these
> basic clocks.
> 
> As part of this change, since the early clock initialization
> is centralized, we no longer need to expose the console clock
> initialization and build it just for SPL.
> 
> NOTE: we change the sequence slightly by initializing console clocks
> timer after the io settings are complete, but this is not expected
> to have any functioanlity impact since we setup the basic IO drive
> strength initialization as part of do_io_settings
> 
> Signed-off-by: Steve Kipisz <s-kipisz2@ti.com>

Reviewed-by: Tom Rini <trini@konsulko.com>

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20151103/08f1907c/attachment.sig>

[U-Boot] [PATCH v2 2/5] ARM: OMAP4/5: Centralize gpi2c_init

[Tom Rini]

On Tue, Nov 03, 2015 at 06:22:39AM -0600, Steve Kipisz wrote:

> Centralize gpi2c_init into omap_common from the sys_proto header so
> that the information can be reused across SoCs.
> 
> Signed-off-by: Steve Kipisz <s-kipisz2@ti.com>

Reviewed-by: Tom Rini <trini@konsulko.com>

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

[U-Boot] [PATCH v2 3/5] ARM: omap-common: Add standard access for board description EEPROM

[Tom Rini]

On Tue, Nov 03, 2015 at 09:26:44AM -0600, Nishanth Menon wrote:
> On 11/03/2015 09:13 AM, Steven Kipisz wrote:
> > On 11/03/2015 07:16 AM, Igor Grinberg wrote:
> >> Hi Steve,
> >>
> >> On 11/03/15 14:22, Steve Kipisz wrote:
> >>> From: Lokesh Vutla <lokeshvutla@ti.com>
> >>
> >> [...]
> >>
> >>>
> >>> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
> >>> Signed-off-by: Steve Kipisz <s-kipisz2@ti.com>
> >>> ---
> >>> v2 Based on
> >>>   master      a6104737  ARM: at91: sama5: change the environment
> >>> address to 0x6000
> >>>
> >>> Changes in v2 (since v1)
> >>>     - make the EEPROM code mor generic for TI EVMs
> >>>     - rename structures/subroutines to ti_am_xxxxx
> >>>     - add routines to access the EEPROM data
> >>>     - redo commit message to be more clear
> >>>
> >>> v1:   http://marc.info/?t=144608007900001&r=1&w=2
> >>>     (mailing list squashed original submission)
> >>>
> >>>   arch/arm/cpu/armv7/omap-common/Makefile        |   1 +
> >>>   arch/arm/cpu/armv7/omap-common/ti-i2c-eeprom.c | 148
> >>> +++++++++++++++++++++++++
> >>>   arch/arm/include/asm/omap_common.h             | 130
> >>> +++++++++++++++++++++-
> >>>   3 files changed, 278 insertions(+), 1 deletion(-)
> >>>   create mode 100644 arch/arm/cpu/armv7/omap-common/ti-i2c-eeprom.c
> >>>
> >>> diff --git a/arch/arm/cpu/armv7/omap-common/Makefile
> >>> b/arch/arm/cpu/armv7/omap-common/Makefile
> >>> index 464a5d1d732a..53a9fdb81100 100644
> >>> --- a/arch/arm/cpu/armv7/omap-common/Makefile
> >>> +++ b/arch/arm/cpu/armv7/omap-common/Makefile
> >>> @@ -15,6 +15,7 @@ obj-y    += clocks-common.o
> >>>   obj-y    += emif-common.o
> >>>   obj-y    += vc.o
> >>>   obj-y    += abb.o
> >>> +obj-$(CONFIG_I2C) += ti-i2c-eeprom.o
> >>
> >> This makes this module compile on all TI SoC based boards enabling I2C.
> >> AFAIU, this is a separate chip (not inside the SoC), so this module will
> >> also compile on non-TI boards that do not have this EEPROM.
> >> I think, it should be more fine grained (e.g. have its own symbol).
> >>
> > Can you give a suggestion?
> 
> Are you sure this will be built into non-ti SoCs with I2C enabled if you
> are not using the function? I assume  __maybe_unused should take care of
> that, no - let the compiler do the gc anyways?

The gc should work, yes.  But this is also TI-centric code and should
end up in board/ti/ similar to how the Siemens AM335x EEPROM code is
under board/siemens/ because they didn't re-use the TI format :)

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20151103/99ae020c/attachment.sig>

[U-Boot] [PATCH v2 4/5] ARM: OMAP4/5: Add generic board detection hook

[Tom Rini]

On Tue, Nov 03, 2015 at 06:22:41AM -0600, Steve Kipisz wrote:

> Many TI EVMs have capability to store relevant board information
> such as DDR description in EEPROM. Further many pad configuration
> variations can occur as part of revision changes in the platform.
> In-order to support these at runtime, we for a board detection hook
> which is available for override from board files that may desire to do
> so.
> 
> NOTE: All TI EVMs are capable of detecting board information based on
> early clocks that are configured. However, in case of additional needs
> this can be achieved within the override logic from within the board
> file.
> 
> Signed-off-by: Steve Kipisz <s-kipisz2@ti.com>

Reviewed-by: Tom Rini <trini@konsulko.com>

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

[U-Boot] [PATCH v2 3/5] ARM: omap-common: Add standard access for board description EEPROM

[Nishanth Menon]

On 11/03/2015 10:07 AM, Tom Rini wrote:
[...]
>>>>> > >>> +obj-$(CONFIG_I2C) += ti-i2c-eeprom.o
>>>> > >>
>>>> > >> This makes this module compile on all TI SoC based boards enabling I2C.
>>>> > >> AFAIU, this is a separate chip (not inside the SoC), so this module will
>>>> > >> also compile on non-TI boards that do not have this EEPROM.
>>>> > >> I think, it should be more fine grained (e.g. have its own symbol).
>>>> > >>
>>> > > Can you give a suggestion?
>> > 
>> > Are you sure this will be built into non-ti SoCs with I2C enabled if you
>> > are not using the function? I assume  __maybe_unused should take care of
>> > that, no - let the compiler do the gc anyways?
> The gc should work, yes.  But this is also TI-centric code and should
> end up in board/ti/ similar to how the Siemens AM335x EEPROM code is
> under board/siemens/ because they didn't re-use the TI format :)

aaah - Nice! a board/ti/common folder for generic stuff like these does
indeed sound like the right solution to me. at least that should prevent
cruft that crept into am437x basically duplicating am335x eeprom data
all over again.


-- 
Regards,
Nishanth Menon

[U-Boot] [PATCH v2 3/5] ARM: omap-common: Add standard access for board description EEPROM

[Igor Grinberg]

On 11/03/15 17:26, Nishanth Menon wrote:
> On 11/03/2015 09:13 AM, Steven Kipisz wrote:
>> On 11/03/2015 07:16 AM, Igor Grinberg wrote:
>>> Hi Steve,
>>>
>>> On 11/03/15 14:22, Steve Kipisz wrote:
>>>> From: Lokesh Vutla <lokeshvutla@ti.com>
>>>
>>> [...]
>>>
>>>>
>>>> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
>>>> Signed-off-by: Steve Kipisz <s-kipisz2@ti.com>
>>>> ---
>>>> v2 Based on
>>>>   master      a6104737  ARM: at91: sama5: change the environment
>>>> address to 0x6000
>>>>
>>>> Changes in v2 (since v1)
>>>>     - make the EEPROM code mor generic for TI EVMs
>>>>     - rename structures/subroutines to ti_am_xxxxx
>>>>     - add routines to access the EEPROM data
>>>>     - redo commit message to be more clear
>>>>
>>>> v1:   http://marc.info/?t=144608007900001&r=1&w=2
>>>>     (mailing list squashed original submission)
>>>>
>>>>   arch/arm/cpu/armv7/omap-common/Makefile        |   1 +
>>>>   arch/arm/cpu/armv7/omap-common/ti-i2c-eeprom.c | 148
>>>> +++++++++++++++++++++++++
>>>>   arch/arm/include/asm/omap_common.h             | 130
>>>> +++++++++++++++++++++-
>>>>   3 files changed, 278 insertions(+), 1 deletion(-)
>>>>   create mode 100644 arch/arm/cpu/armv7/omap-common/ti-i2c-eeprom.c
>>>>
>>>> diff --git a/arch/arm/cpu/armv7/omap-common/Makefile
>>>> b/arch/arm/cpu/armv7/omap-common/Makefile
>>>> index 464a5d1d732a..53a9fdb81100 100644
>>>> --- a/arch/arm/cpu/armv7/omap-common/Makefile
>>>> +++ b/arch/arm/cpu/armv7/omap-common/Makefile
>>>> @@ -15,6 +15,7 @@ obj-y    += clocks-common.o
>>>>   obj-y    += emif-common.o
>>>>   obj-y    += vc.o
>>>>   obj-y    += abb.o
>>>> +obj-$(CONFIG_I2C) += ti-i2c-eeprom.o
>>>
>>> This makes this module compile on all TI SoC based boards enabling I2C.
>>> AFAIU, this is a separate chip (not inside the SoC), so this module will
>>> also compile on non-TI boards that do not have this EEPROM.
>>> I think, it should be more fine grained (e.g. have its own symbol).
>>>
>> Can you give a suggestion?
> 
> Are you sure this will be built into non-ti SoCs with I2C enabled if you
> are not using the function? I assume  __maybe_unused should take care of
> that, no - let the compiler do the gc anyways?
> 
> 
> It should have been documented on commit message as well.
> 
> 
>>>> + * @revision:    Revision of the board
>>>> + * @serial:    Serial Number of the board
>>>> + *
>>>> + * In case of NULL revision or serial information "unknown" is setup.
>>>> + * If name is NULL, default_name is used.
>>>> + */
>>>> +void set_board_info_env(char *name, char *default_name,
>>>> +            char *revision, char *serial);
>>>> +
>>>> +/**
>>>> + * board_am_is() - Generic Board detection logic
>>>> + * @name_tag:    Tag used in eeprom for the board
>>>> + *
>>>> + * Return: false if board information does not match OR eeprom
>>>> was'nt read.
>>>> + *       true otherwise
>>>> + */
>>>> +static inline bool board_am_is(char *name_tag)
>>>> +{
>>>> +    struct ti_am_eeprom *ep = TI_AM_EEPROM_DATA;
>>>> +
>>>> +    if (ep->header != TI_EEPROM_HEADER_MAGIC)
>>>> +        return false;
>>>> +    return !strncmp(ep->name, name_tag, TI_EEPROM_HDR_NAME_LEN);
>>>> +}
>>>
>>> Why do you need to place non trivial function implementation inside the
>>> header file?
>>>
>>>> +
>>>> +/**
>>>> + * board_am_rev_is() - Compare board revision
>>>> + * @rev_tag:    Revision tag to check in eeprom
>>>> + * @cmp_len:    How many chars to compare?
>>>> + *
>>>> + * NOTE: revision information is often messed up (hence the str len
>>>> match) :(
>>>> + *
>>>> + * Return: false if board information does not match OR eeprom
>>>> was'nt read.
>>>> + *       true otherwise
>>>> + */
>>>> +static inline bool board_am_rev_is(char *rev_tag, int cmp_len)
>>>> +{
>>>> +    struct ti_am_eeprom *ep = TI_AM_EEPROM_DATA;
>>>> +    int l;
>>>> +
>>>> +    if (ep->header != TI_EEPROM_HEADER_MAGIC)
>>>> +        return false;
>>>> +
>>>> +    l = cmp_len > TI_EEPROM_HDR_REV_LEN ? TI_EEPROM_HDR_NAME_LEN :
>>>> cmp_len;
>>>> +    return !strncmp(ep->version, rev_tag, l);
>>>> +}
>>>
>>> Same here.
>>>
>> I thought by making them static inline would save space.
> 
> I prefer that myself as well.

I'm not sure I understand what space will it save?
AFAIK, inline places the function code inside the the caller function
and thus spreads into each caller, no? It probably saves some branches,
but how does that save space?

Also, AFAIR, we try to not place code inside headers, unless the code
is a stub.

-- 
Regards,
Igor.

[U-Boot] [PATCH v2 5/5] board: ti: AM57xx: Add detection logic for AM57xx-evm

[Igor Grinberg]

On 11/03/15 17:09, Steven Kipisz wrote:
> On 11/03/2015 07:29 AM, Igor Grinberg wrote:
>> Hi Steve,
>>
>> On 11/03/15 14:22, Steve Kipisz wrote:
>>
>> [...]
>>
>>> Signed-off-by: Steve Kipisz <s-kipisz2@ti.com>
>>> ---
>>> v2 Based on:
>>>   master     a6104737 ARM: at91: sama5: change the environment address to 0x6000
>>>
>>> Build testing: MAKEALL -s omap4 -s omap5 (no warnings/build errors)
>>>     Boot Testing:
>>>     am57xx_evm_nodt_config: http://pastebin.ubuntu.com/13039296/
>>>     beagle_x15_config: http://pastebin.ubuntu.com/13039331/
>>>
>>> Changes in v2 (since v1):
>>>     - move the board detection code into the new routine
>>>       do_board_detect
>>>     - eliminate board.h and move the ix_xxx into board.c
>>>     - redo commit message to be more clear
>>>
>>> v1:  http://marc.info/?t=144608007900002&r=1&w=2
>>>       http://marc.info/?t=144608007900004&r=1&w=2
>>>     (mailing list squashed original submission)
>>
>> [...]
>>
>>> +#define is_x15()    board_am_is("BBRDX15_")
>>> +#define is_am572x_evm()    board_am_is("AM572PM_")
>>
>> I think board_is_* much more appropriate here...
>>
> Ok. so board_is_x15 and board_is_am572x_evm

Yep. Sounds better.

>>> +
>>>   #ifdef CONFIG_DRIVER_TI_CPSW
>>>   #include <cpsw.h>
>>>   #endif
>>> @@ -246,6 +249,54 @@ struct vcores_data beagle_x15_volts = {
>>>       .iva.pmic        = &tps659038,
>>>   };
>>>
>>> +#ifdef CONFIG_SPL_BUILD
>>> +/* No env to setup for SPL */
>>> +static inline void setup_board_eeprom_env(void) { }
>>> +
>>> +/* Override function to read eeprom information */
>>> +void do_board_detect(void)
>>> +{
>>> +    struct ti_am_eeprom *ep;
>>> +    int rc;
>>> +
>>> +    rc = ti_i2c_eeprom_am_get(CONFIG_EEPROM_BUS_ADDRESS,
>>> +                  CONFIG_EEPROM_CHIP_ADDRESS, &ep);
>>> +    if (rc)
>>> +        printf("ti_i2c_eeprom_init failed %d\n", rc);
>>> +}
>>
>> Do you really need this in SPL?
> 
> Yes. We need to detect the board to determine DDR setup, pin mux, iodelay. All of that needs to be done in SPL. X15 and EVM are the same, but more boards will be added that have some differences.

Ok.

>>
>>> +
>>> +#else    /* CONFIG_SPL_BUILD */
>>> +
>>> +static void setup_board_eeprom_env(void)
>>> +{
>>> +    char *name = NULL;
>>
>> How about:
>>
>>     char *name = "beagle_x15";
>>
>>> +    int rc;
>>> +    struct ti_am_eeprom_printable p;
>>> +
>>> +    rc = ti_i2c_eeprom_am_get_print(CONFIG_EEPROM_BUS_ADDRESS,
>>> +                    CONFIG_EEPROM_CHIP_ADDRESS, &p);
>>> +    if (rc) {
>>> +        printf("Invalid EEPROM data(@0x%p). Default to X15\n",
>>> +               TI_AM_EEPROM_DATA);
>>> +        goto invalid_eeprom;
>>> +    }
>>> +
>>> +    if (is_x15())
>>> +        name = "beagle_x15";
>>
>> This will not be needed if the above comment is implemented.
>>
>>> +    else if (is_am572x_evm())
>>> +        name = "am57xx_evm";
>>> +    else
>>> +        printf("Unidentified board claims %s in eeprom header\n",
>>> +               p.name);
>>> +
>>> +invalid_eeprom:
>>> +    set_board_info_env(name, "beagle_x15", p.version, p.serial);
>>
>> If the above comment is implemented, no more need for the
>> default_name parameter...
>>
> Ok, I'll look at that.
>>> +}
>>> +
>>> +/* Eeprom is alread read by SPL.. nothing more to do here.. */
>>> +
>>> +#endif    /* CONFIG_SPL_BUILD */
>>
>> [...]
>>
>>
> Steve K.
> 

-- 
Regards,
Igor.

[U-Boot] [PATCH v2 3/5] ARM: omap-common: Add standard access for board description EEPROM

[Nishanth Menon]

On 11/03/2015 11:02 AM, Igor Grinberg wrote:

>>>>> +
>>>>> +/**
>>>>> + * board_am_rev_is() - Compare board revision
>>>>> + * @rev_tag:    Revision tag to check in eeprom
>>>>> + * @cmp_len:    How many chars to compare?
>>>>> + *
>>>>> + * NOTE: revision information is often messed up (hence the str len
>>>>> match) :(
>>>>> + *
>>>>> + * Return: false if board information does not match OR eeprom
>>>>> was'nt read.
>>>>> + *       true otherwise
>>>>> + */
>>>>> +static inline bool board_am_rev_is(char *rev_tag, int cmp_len)
>>>>> +{
>>>>> +    struct ti_am_eeprom *ep = TI_AM_EEPROM_DATA;
>>>>> +    int l;
>>>>> +
>>>>> +    if (ep->header != TI_EEPROM_HEADER_MAGIC)
>>>>> +        return false;
>>>>> +
>>>>> +    l = cmp_len > TI_EEPROM_HDR_REV_LEN ? TI_EEPROM_HDR_NAME_LEN :
>>>>> cmp_len;
>>>>> +    return !strncmp(ep->version, rev_tag, l);
>>>>> +}
>>>>
>>>> Same here.
>>>>
>>> I thought by making them static inline would save space.
>>
>> I prefer that myself as well.
> 
> I'm not sure I understand what space will it save?
> AFAIK, inline places the function code inside the the caller function
> and thus spreads into each caller, no? It probably saves some branches,
> but how does that save space?

I dont think it saves space, but rather a function call overhead for
trivial code as above.

> Also, AFAIR, we try to not place code inside headers, unless the code
> is a stub.


That does not always make sense. here it is a straight forward
comparison.. why hide it a function call deep when you can inline it?

-- 
Regards,
Nishanth Menon

[U-Boot] [PATCH v2 3/5] ARM: omap-common: Add standard access for board description EEPROM

[Igor Grinberg]

On 11/03/15 19:45, Nishanth Menon wrote:
> On 11/03/2015 11:02 AM, Igor Grinberg wrote:
> 
>>>>>> +
>>>>>> +/**
>>>>>> + * board_am_rev_is() - Compare board revision
>>>>>> + * @rev_tag:    Revision tag to check in eeprom
>>>>>> + * @cmp_len:    How many chars to compare?
>>>>>> + *
>>>>>> + * NOTE: revision information is often messed up (hence the str len
>>>>>> match) :(
>>>>>> + *
>>>>>> + * Return: false if board information does not match OR eeprom
>>>>>> was'nt read.
>>>>>> + *       true otherwise
>>>>>> + */
>>>>>> +static inline bool board_am_rev_is(char *rev_tag, int cmp_len)
>>>>>> +{
>>>>>> +    struct ti_am_eeprom *ep = TI_AM_EEPROM_DATA;
>>>>>> +    int l;
>>>>>> +
>>>>>> +    if (ep->header != TI_EEPROM_HEADER_MAGIC)
>>>>>> +        return false;
>>>>>> +
>>>>>> +    l = cmp_len > TI_EEPROM_HDR_REV_LEN ? TI_EEPROM_HDR_NAME_LEN :
>>>>>> cmp_len;
>>>>>> +    return !strncmp(ep->version, rev_tag, l);
>>>>>> +}
>>>>>
>>>>> Same here.
>>>>>
>>>> I thought by making them static inline would save space.
>>>
>>> I prefer that myself as well.
>>
>> I'm not sure I understand what space will it save?
>> AFAIK, inline places the function code inside the the caller function
>> and thus spreads into each caller, no? It probably saves some branches,
>> but how does that save space?
> 
> I dont think it saves space, but rather a function call overhead for
> trivial code as above.

I'm sorry, IMO the above code is not trivial enough... 
or just it is not trivial: several variables on the stack,
some logic which is not that straight forward for someone who is
not familiar with the code and defines that are mapped into an SRAM...
and we also have strings comparison in the end.
Come on... that is not trivial at all. Moreover it gives a very bad
example to any newcomer...

> 
>> Also, AFAIR, we try to not place code inside headers, unless the code
>> is a stub.
> 
> 
> That does not always make sense. here it is a straight forward
> comparison.. why hide it a function call deep when you can inline it?

Well, at least because header files are not meant to hold code.
Accepting this gives a bad example and reduces code quality...

-- 
Regards,
Igor.

[U-Boot] [PATCH v2 3/5] ARM: omap-common: Add standard access for board description EEPROM

[Nishanth Menon]

On 11/03/2015 03:07 PM, Igor Grinberg wrote:
> On 11/03/15 19:45, Nishanth Menon wrote:
>> On 11/03/2015 11:02 AM, Igor Grinberg wrote:
>>
>>>>>>> +
>>>>>>> +/**
>>>>>>> + * board_am_rev_is() - Compare board revision
>>>>>>> + * @rev_tag:    Revision tag to check in eeprom
>>>>>>> + * @cmp_len:    How many chars to compare?
>>>>>>> + *
>>>>>>> + * NOTE: revision information is often messed up (hence the str len
>>>>>>> match) :(
>>>>>>> + *
>>>>>>> + * Return: false if board information does not match OR eeprom
>>>>>>> was'nt read.
>>>>>>> + *       true otherwise
>>>>>>> + */
>>>>>>> +static inline bool board_am_rev_is(char *rev_tag, int cmp_len)
>>>>>>> +{
>>>>>>> +    struct ti_am_eeprom *ep = TI_AM_EEPROM_DATA;
>>>>>>> +    int l;
>>>>>>> +
>>>>>>> +    if (ep->header != TI_EEPROM_HEADER_MAGIC)
>>>>>>> +        return false;
>>>>>>> +
>>>>>>> +    l = cmp_len > TI_EEPROM_HDR_REV_LEN ? TI_EEPROM_HDR_NAME_LEN :
>>>>>>> cmp_len;
>>>>>>> +    return !strncmp(ep->version, rev_tag, l);
>>>>>>> +}
>>>>>>
>>>>>> Same here.
>>>>>>
>>>>> I thought by making them static inline would save space.
>>>>
>>>> I prefer that myself as well.
>>>
>>> I'm not sure I understand what space will it save?
>>> AFAIK, inline places the function code inside the the caller function
>>> and thus spreads into each caller, no? It probably saves some branches,
>>> but how does that save space?
>>
>> I dont think it saves space, but rather a function call overhead for
>> trivial code as above.
> 
> I'm sorry, IMO the above code is not trivial enough... 
> or just it is not trivial: several variables on the stack,
> some logic which is not that straight forward for someone who is
> not familiar with the code and defines that are mapped into an SRAM...
> and we also have strings comparison in the end.
> Come on... that is not trivial at all. Moreover it gives a very bad
> example to any newcomer...
> 

:) OK, agreed. makes sense. I think Steve can definitely push it off
into the board/ti/common/ file :).

>>
>>> Also, AFAIR, we try to not place code inside headers, unless the code
>>> is a stub.
>>
>>
>> That does not always make sense. here it is a straight forward
>> comparison.. why hide it a function call deep when you can inline it?
> 
> Well, at least because header files are not meant to hold code.
> Accepting this gives a bad example and reduces code quality...
> 

Fair enough. Thanks for reviewing it.

-- 
Regards,
Nishanth Menon

[U-Boot] [PATCH v2 1/5] ARM: OMAP4/5: Centralize early clock initialization

[Lokesh Vutla]

On Tuesday 03 November 2015 05:52 PM, Steve Kipisz wrote:
> Early clock initialization is currently done in two stages for OMAP4/5
> SoCs. The first stage is the initialization of console clocks and
> then we initialize basic clocks for functionality necessary for SoC
> initialization and basic board functionality.
> 
> By splitting up prcm_init and centralizing this clock initialization,
> we setup the code for follow on patches that can do board specific
> initialization such as board detection which will depend on these
> basic clocks.
> 
> As part of this change, since the early clock initialization
> is centralized, we no longer need to expose the console clock
> initialization and build it just for SPL.
> 
> NOTE: we change the sequence slightly by initializing console clocks
> timer after the io settings are complete, but this is not expected
> to have any functioanlity impact since we setup the basic IO drive
> strength initialization as part of do_io_settings
> 
> Signed-off-by: Steve Kipisz <s-kipisz2@ti.com>
> ---
> v2 Based on:
>  master      a6104737 ARM: at91: sama5: change the environment address to 0x6000
> 
> Changes in v2:
>  - New patch
> 
>  arch/arm/cpu/armv7/omap-common/clocks-common.c | 26 ++++++++++++++++++++++++--
>  arch/arm/cpu/armv7/omap-common/hwinit-common.c |  3 +--
>  arch/arm/include/asm/arch-omap4/sys_proto.h    |  2 +-
>  arch/arm/include/asm/arch-omap5/sys_proto.h    |  2 +-
>  4 files changed, 27 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm/cpu/armv7/omap-common/clocks-common.c b/arch/arm/cpu/armv7/omap-common/clocks-common.c
> index e28b79568d1d..2ede0818e444 100644
> --- a/arch/arm/cpu/armv7/omap-common/clocks-common.c
> +++ b/arch/arm/cpu/armv7/omap-common/clocks-common.c
> @@ -769,7 +769,8 @@ void lock_dpll(u32 const base)
>  	wait_for_lock(base);
>  }
>  
> -void setup_clocks_for_console(void)
> +#ifdef CONFIG_SPL_BUILD
> +static void setup_clocks_for_console(void)
>  {
>  	/* Do not add any spl_debug prints in this function */
>  	clrsetbits_le32((*prcm)->cm_l4per_clkstctrl, CD_CLKCTRL_CLKTRCTRL_MASK,
> @@ -801,6 +802,9 @@ void setup_clocks_for_console(void)
>  			CD_CLKCTRL_CLKTRCTRL_HW_AUTO <<
>  			CD_CLKCTRL_CLKTRCTRL_SHIFT);
>  }
> +#else
> +static inline void setup_clocks_for_console(void) { }
> +#endif

Please drop this if condition as clocks will be needed in u-boot for xip
boot.

>  
>  void do_enable_clocks(u32 const *clk_domains,
>  			    u32 const *clk_modules_hw_auto,
> @@ -853,14 +857,32 @@ void do_disable_clocks(u32 const *clk_domains,
>  		disable_clock_domain(clk_domains[i]);
>  }
>  
> -void prcm_init(void)
> +/**
> + * setup_early_clocks() - Setup early clocks needed for SoC
> + *
> + * Setup clocks for console, SPL basic initialization clocks and initialize
> + * the timer. This is invoked prior prcm_init.
> + */
> +void setup_early_clocks(void)
>  {
> +	setup_clocks_for_console();
> +
>  	switch (omap_hw_init_context()) {
>  	case OMAP_INIT_CONTEXT_SPL:
>  	case OMAP_INIT_CONTEXT_UBOOT_FROM_NOR:
>  	case OMAP_INIT_CONTEXT_UBOOT_AFTER_CH:

Add setup_clocks_for_console() here instead of adding in the beginning
of function.

Thanks and regards,
Lokesh

[U-Boot] [PATCH v2 1/5] ARM: OMAP4/5: Centralize early clock initialization

[Steven Kipisz]

On 11/04/2015 02:32 AM, Lokesh Vutla wrote:
>
>
> On Tuesday 03 November 2015 05:52 PM, Steve Kipisz wrote:
>> Early clock initialization is currently done in two stages for OMAP4/5
>> SoCs. The first stage is the initialization of console clocks and
>> then we initialize basic clocks for functionality necessary for SoC
>> initialization and basic board functionality.
>>
>> By splitting up prcm_init and centralizing this clock initialization,
>> we setup the code for follow on patches that can do board specific
>> initialization such as board detection which will depend on these
>> basic clocks.
>>
>> As part of this change, since the early clock initialization
>> is centralized, we no longer need to expose the console clock
>> initialization and build it just for SPL.
>>
>> NOTE: we change the sequence slightly by initializing console clocks
>> timer after the io settings are complete, but this is not expected
>> to have any functioanlity impact since we setup the basic IO drive
>> strength initialization as part of do_io_settings
>>
>> Signed-off-by: Steve Kipisz <s-kipisz2@ti.com>
>> ---
>> v2 Based on:
>>   master      a6104737 ARM: at91: sama5: change the environment address to 0x6000
>>
>> Changes in v2:
>>   - New patch
>>
>>   arch/arm/cpu/armv7/omap-common/clocks-common.c | 26 ++++++++++++++++++++++++--
>>   arch/arm/cpu/armv7/omap-common/hwinit-common.c |  3 +--
>>   arch/arm/include/asm/arch-omap4/sys_proto.h    |  2 +-
>>   arch/arm/include/asm/arch-omap5/sys_proto.h    |  2 +-
>>   4 files changed, 27 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/arm/cpu/armv7/omap-common/clocks-common.c b/arch/arm/cpu/armv7/omap-common/clocks-common.c
>> index e28b79568d1d..2ede0818e444 100644
>> --- a/arch/arm/cpu/armv7/omap-common/clocks-common.c
>> +++ b/arch/arm/cpu/armv7/omap-common/clocks-common.c
>> @@ -769,7 +769,8 @@ void lock_dpll(u32 const base)
>>   	wait_for_lock(base);
>>   }
>>
>> -void setup_clocks_for_console(void)
>> +#ifdef CONFIG_SPL_BUILD
>> +static void setup_clocks_for_console(void)
>>   {
>>   	/* Do not add any spl_debug prints in this function */
>>   	clrsetbits_le32((*prcm)->cm_l4per_clkstctrl, CD_CLKCTRL_CLKTRCTRL_MASK,
>> @@ -801,6 +802,9 @@ void setup_clocks_for_console(void)
>>   			CD_CLKCTRL_CLKTRCTRL_HW_AUTO <<
>>   			CD_CLKCTRL_CLKTRCTRL_SHIFT);
>>   }
>> +#else
>> +static inline void setup_clocks_for_console(void) { }
>> +#endif
>
> Please drop this if condition as clocks will be needed in u-boot for xip
> boot.
>
How about changing it to
#if defined(CONFIG_SPL_BUILD) || defined(CONFIG_NOR_BOOT)

>>
>>   void do_enable_clocks(u32 const *clk_domains,
>>   			    u32 const *clk_modules_hw_auto,
>> @@ -853,14 +857,32 @@ void do_disable_clocks(u32 const *clk_domains,
>>   		disable_clock_domain(clk_domains[i]);
>>   }
>>
>> -void prcm_init(void)
>> +/**
>> + * setup_early_clocks() - Setup early clocks needed for SoC
>> + *
>> + * Setup clocks for console, SPL basic initialization clocks and initialize
>> + * the timer. This is invoked prior prcm_init.
>> + */
>> +void setup_early_clocks(void)
>>   {
>> +	setup_clocks_for_console();
>> +
>>   	switch (omap_hw_init_context()) {
>>   	case OMAP_INIT_CONTEXT_SPL:
>>   	case OMAP_INIT_CONTEXT_UBOOT_FROM_NOR:
>>   	case OMAP_INIT_CONTEXT_UBOOT_AFTER_CH:
>
> Add setup_clocks_for_console() here instead of adding in the beginning
> of function.
>
> Thanks and regards,
> Lokesh
>