Re: [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 
---
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



___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [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 


[...]



Signed-off-by: Lokesh Vutla 
Signed-off-by: Steve Kipisz 
---
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_x
- add routines to access the EEPROM data
- redo commit message to be more clear

v1:   http://marc.info/?t=14460800791=1=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 ..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 /* 

Re: [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 
---
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=14460800792=1=2
  http://marc.info/?t=14460800794=1=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 
  #endif
@@ -246,6 +249,54 @@ struct vcores_data beagle_x15_volts = {
.iva.pmic   = ,
  };

+#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, );
+   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, );
+   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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [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
 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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot