public inbox for u-boot@lists.denx.de
 help / color / mirror / Atom feed
* [U-Boot] [patch] Add display cpuinfo for Atmel at91sam9261 Cores
@ 2009-05-02 20:06 Remy Bohmer
  2009-05-18 20:25 ` Jean-Christophe PLAGNIOL-VILLARD
  0 siblings, 1 reply; 9+ messages in thread
From: Remy Bohmer @ 2009-05-02 20:06 UTC (permalink / raw)
  To: u-boot

An embedded and charset-unspecified text was scrubbed...
Name: 0001-Add-display-cpuinfo-for-Atmel-at91sam9261-Cores.patch
Url: http://lists.denx.de/pipermail/u-boot/attachments/20090502/b1fab66b/attachment.txt 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [U-Boot] [patch] Add display cpuinfo for Atmel at91sam9261 Cores
  2009-05-02 20:06 [U-Boot] [patch] Add display cpuinfo for Atmel at91sam9261 Cores Remy Bohmer
@ 2009-05-18 20:25 ` Jean-Christophe PLAGNIOL-VILLARD
  2009-06-01 18:21   ` [U-Boot] [PATCH V2] Add display cpuinfo for Atmel at91 arm926ejs based cores Remy Bohmer
  0 siblings, 1 reply; 9+ messages in thread
From: Jean-Christophe PLAGNIOL-VILLARD @ 2009-05-18 20:25 UTC (permalink / raw)
  To: u-boot

On 22:06 Sat 02 May     , Remy Bohmer wrote:
> This patch adds support for displaying CPU type information
> about Atmel AT91 cores during boot. It currently only configures it for the
> at91sam9261ek board, but it can easily be configured for all at91 based cores.
please do this in two seperate patch
please ask Stelian
CC
btw please use git
> 
> This patch also provides at91_dbgu.h which is also used by the upcoming
> USB CDC-ethernet and USB-gadget support for this board.
> 
> Signed-off-by: Remy B?hmer <linux@bohmer.net>
> ---
>  cpu/arm926ejs/at91/Makefile           |    1 
>  cpu/arm926ejs/at91/cpuinfo.c          |   83 ++++++++++++++++++++++++++++
>  include/asm-arm/arch-at91/at91_dbgu.h |   69 +++++++++++++++++++++++
>  include/asm-arm/arch-at91/cpu.h       |   99 ++++++++++++++++++++++++++++++++++
>  include/configs/at91sam9261ek.h       |    2 
>  5 files changed, 254 insertions(+)
>  create mode 100644 cpu/arm926ejs/at91/cpuinfo.c
>  create mode 100644 include/asm-arm/arch-at91/at91_dbgu.h
>  create mode 100644 include/asm-arm/arch-at91/cpu.h
> 
> Index: u-boot-usb.tmp/cpu/arm926ejs/at91/Makefile
> ===================================================================
> --- u-boot-usb.tmp.orig/cpu/arm926ejs/at91/Makefile	2009-05-02 21:43:41.000000000 +0200
> +++ u-boot-usb.tmp/cpu/arm926ejs/at91/Makefile	2009-05-02 22:01:46.000000000 +0200
> @@ -55,6 +55,7 @@ COBJS-y				+= at91sam9rl_serial.o
>  COBJS-$(CONFIG_HAS_DATAFLASH)	+= at91sam9rl_spi.o
>  endif
>  COBJS-$(CONFIG_AT91_LED)	+= led.o
> +COBJS-$(CONFIG_DISPLAY_CPUINFO) +=cpuinfo.o
>  COBJS-y += clock.o
>  COBJS-y += cpu.o
>  COBJS-y	+= timer.o
> Index: u-boot-usb.tmp/cpu/arm926ejs/at91/cpuinfo.c
> ===================================================================
> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ u-boot-usb.tmp/cpu/arm926ejs/at91/cpuinfo.c	2009-05-02 22:01:46.000000000 +0200
> @@ -0,0 +1,83 @@
> +#include <command.h>
> +#include <common.h>
> +#include <arm926ejs.h>
> +#include <asm/hardware.h>
> +#include <asm/arch/at91_dbgu.h>
> +#include <asm/arch/io.h>
> +
> +int print_cpuinfo(void)
> +{
> +	unsigned long cidr;
> +	const char *txt;
> +
> +	cidr = at91_sys_read(AT91_DBGU_CIDR);
> +
could you use the CONFIG_ARM926EJS and other configs to reduce the size impact
> +	switch ((cidr & AT91_CIDR_EPROC) >> 5) {
> +	case 0x1: txt = "ARM946ES";	break;
> +	case 0x2: txt = "ARM7TDMI";	break;
> +	case 0x4: txt = "ARM920T";	break;
> +	case 0x5: txt = "ARM926EJS";	break;
> +	default:  txt = "undefined";	break;
> +	}
> +	printf("Embedded Processor: %s\n", txt);
otherwise ok

Best Regards,
J.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [U-Boot] [PATCH V2] Add display cpuinfo for Atmel at91 arm926ejs based cores
  2009-05-18 20:25 ` Jean-Christophe PLAGNIOL-VILLARD
@ 2009-06-01 18:21   ` Remy Bohmer
  2009-06-01 18:21     ` [U-Boot] [PATCH] Enable display CPU-Info for at91sam9261 Remy Bohmer
  2009-06-01 19:59     ` [U-Boot] [PATCH V2] Add display cpuinfo for Atmel at91 arm926ejs based cores Stelian Pop
  0 siblings, 2 replies; 9+ messages in thread
From: Remy Bohmer @ 2009-06-01 18:21 UTC (permalink / raw)
  To: u-boot

Signed-off-by: Remy Bohmer <linux@bohmer.net>
---
 V2 reworked comments from Jean-Christophe PLAGNIOL-VILLARD

 cpu/arm926ejs/at91/Makefile           |    1 +
 cpu/arm926ejs/at91/cpuinfo.c          |   65 +++++++++++++++++++++
 include/asm-arm/arch-at91/at91_dbgu.h |   69 +++++++++++++++++++++++
 include/asm-arm/arch-at91/cpu.h       |   99 +++++++++++++++++++++++++++++++++
 4 files changed, 234 insertions(+), 0 deletions(-)
 create mode 100644 cpu/arm926ejs/at91/cpuinfo.c
 create mode 100644 include/asm-arm/arch-at91/at91_dbgu.h
 create mode 100644 include/asm-arm/arch-at91/cpu.h

diff --git a/cpu/arm926ejs/at91/Makefile b/cpu/arm926ejs/at91/Makefile
index 71acb0b..5c970a1 100644
--- a/cpu/arm926ejs/at91/Makefile
+++ b/cpu/arm926ejs/at91/Makefile
@@ -32,6 +32,7 @@ COBJS-$(CONFIG_AT91SAM9261)	+= at91sam9261_devices.o
 COBJS-$(CONFIG_AT91SAM9263)	+= at91sam9263_devices.o
 COBJS-$(CONFIG_AT91SAM9RL)	+= at91sam9rl_devices.o
 COBJS-$(CONFIG_AT91_LED)	+= led.o
+COBJS-$(CONFIG_DISPLAY_CPUINFO) +=cpuinfo.o
 COBJS-y += clock.o
 COBJS-y += cpu.o
 COBJS-y	+= timer.o
diff --git a/cpu/arm926ejs/at91/cpuinfo.c b/cpu/arm926ejs/at91/cpuinfo.c
new file mode 100644
index 0000000..1c6abad
--- /dev/null
+++ b/cpu/arm926ejs/at91/cpuinfo.c
@@ -0,0 +1,65 @@
+#include <command.h>
+#include <common.h>
+#include <arm926ejs.h>
+#include <asm/hardware.h>
+#include <asm/arch/at91_dbgu.h>
+#include <asm/arch/io.h>
+
+int print_cpuinfo(void)
+{
+	unsigned long cidr;
+	const char *txt;
+
+	cidr = at91_sys_read(AT91_DBGU_CIDR);
+
+	switch ((cidr & AT91_CIDR_EPROC) >> 5) {
+	 /* 0x1 = ARM946ES, 0x2 = ARM7TDMI, 0x4 = ARM920T */
+	case 0x5: txt = "ARM926EJS";	break;
+	default:  txt = "undefined";	break;
+	}
+	printf("Embedded Processor: %s\n", txt);
+
+	switch ((cidr & AT91_CIDR_ARCH) >> 20) {
+	case 0x19: txt = "AT91SAM9xx";		break;
+	case 0x29: txt = "AT91SAM9XExx";	break;
+	case 0x39: txt = "CAP9";		break;
+	default:   txt = "undefined";		break;
+	}
+	printf("Architecture: %s Series\n", txt);
+
+	printf("CPU-revision: %c\n", (char)('A' + (cidr & AT91_CIDR_VERSION)));
+
+	switch ((cidr & AT91_CIDR_NVPTYP) >> 28) {
+	case 0x0: txt = "ROM";				 break;
+	case 0x1: txt = "ROMless or onchip flash";	 break;
+	case 0x2: txt = "Embedded flash memory";	 break;
+	case 0x3: txt = "ROM and Embedded flash memory"; break;
+	case 0x4: txt = "SRAM emulating ROM";		 break;
+	default:  txt = "undefined";			 break;
+	}
+	printf("NonVolatile Program Memory type: %s\n", txt);
+
+	switch (cidr & AT91_CIDR_SRAMSIZ) {
+	case AT91_CIDR_SRAMSIZ_1K: txt = "1K";		break;
+	case AT91_CIDR_SRAMSIZ_2K: txt = "2K";		break;
+	case AT91_CIDR_SRAMSIZ_112K: txt = "112K";	break;
+	case AT91_CIDR_SRAMSIZ_4K: txt = "4K";		break;
+	case AT91_CIDR_SRAMSIZ_80K: txt = "80K";	break;
+	case AT91_CIDR_SRAMSIZ_160K: txt = "160K";	break;
+	case AT91_CIDR_SRAMSIZ_8K: txt = "8K";		break;
+	case AT91_CIDR_SRAMSIZ_16K: txt = "16K";	break;
+	case AT91_CIDR_SRAMSIZ_32K: txt = "32K";	break;
+	case AT91_CIDR_SRAMSIZ_64K: txt = "64K";	break;
+	case AT91_CIDR_SRAMSIZ_128K: txt = "128K";	break;
+	case AT91_CIDR_SRAMSIZ_256K: txt = "256K";	break;
+	case AT91_CIDR_SRAMSIZ_96K: txt = "96K";	break;
+	case AT91_CIDR_SRAMSIZ_512K: txt = "512K";	break;
+	default:  txt = "undefined";			break;
+	}
+	printf("Internal SRAM size: %s\n", txt);
+
+	if (cidr & AT91_CIDR_EXT)
+		printf("Extension ID: 0x%x\n", at91_sys_read(AT91_DBGU_EXID));
+
+	return 0;
+}
diff --git a/include/asm-arm/arch-at91/at91_dbgu.h b/include/asm-arm/arch-at91/at91_dbgu.h
new file mode 100644
index 0000000..55210da
--- /dev/null
+++ b/include/asm-arm/arch-at91/at91_dbgu.h
@@ -0,0 +1,69 @@
+/*
+ * include/asm-arm/arch-at91/at91_dbgu.h
+ *
+ * Copyright (C) 2005 Ivan Kokshaysky
+ * Copyright (C) SAN People
+ *
+ * Debug Unit (DBGU) - System peripherals registers.
+ * Based on AT91RM9200 datasheet revision E.
+ *
+ * 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.
+ */
+
+#ifndef AT91_DBGU_H
+#define AT91_DBGU_H
+
+#ifdef AT91_DBGU
+#define AT91_DBGU_CR		(AT91_DBGU + 0x00) /* Control Register */
+#define AT91_DBGU_MR		(AT91_DBGU + 0x04) /* Mode Register */
+#define AT91_DBGU_IER		(AT91_DBGU + 0x08) /* Interrupt Enable */
+#define		AT91_DBGU_TXRDY		(1 << 1)   /* Transmitter Ready */
+#define		AT91_DBGU_TXEMPTY	(1 << 9)   /* Transmitter Empty */
+#define AT91_DBGU_IDR		(AT91_DBGU + 0x0c) /* Interrupt Disable */
+#define AT91_DBGU_IMR		(AT91_DBGU + 0x10) /* Interrupt Mask */
+#define AT91_DBGU_SR		(AT91_DBGU + 0x14) /* Status Register */
+#define AT91_DBGU_RHR		(AT91_DBGU + 0x18) /* Receiver Holding */
+#define AT91_DBGU_THR		(AT91_DBGU + 0x1c) /* Transmitter Holding */
+#define AT91_DBGU_BRGR		(AT91_DBGU + 0x20) /* Baud Rate Generator */
+
+#define AT91_DBGU_CIDR		(AT91_DBGU + 0x40) /* Chip ID Register */
+#define AT91_DBGU_EXID		(AT91_DBGU + 0x44) /* Chip ID Extension */
+#define AT91_DBGU_FNR		(AT91_DBGU + 0x48) /* Force NTRST [SAM9 only] */
+#define		AT91_DBGU_FNTRST	(1 << 0)   /* Force NTRST */
+
+#endif /* AT91_DBGU */
+
+/*
+ * Some AT91 parts that don't have full DEBUG units still support the ID
+ * and extensions register.
+ */
+#define		AT91_CIDR_VERSION	(0x1f << 0) /* Version of the Device */
+#define		AT91_CIDR_EPROC		(7    << 5) /* Embedded Processor */
+#define		AT91_CIDR_NVPSIZ	(0xf  << 8) /* Nonvolatile Program
+						       Memory Size */
+#define		AT91_CIDR_NVPSIZ2	(0xf  << 12) /* Second Nonvolatile
+							Program Memory Size */
+#define		AT91_CIDR_SRAMSIZ	(0xf  << 16)	/* Internal SRAM Size */
+#define			AT91_CIDR_SRAMSIZ_1K	(1 << 16)
+#define			AT91_CIDR_SRAMSIZ_2K	(2 << 16)
+#define			AT91_CIDR_SRAMSIZ_112K	(4 << 16)
+#define			AT91_CIDR_SRAMSIZ_4K	(5 << 16)
+#define			AT91_CIDR_SRAMSIZ_80K	(6 << 16)
+#define			AT91_CIDR_SRAMSIZ_160K	(7 << 16)
+#define			AT91_CIDR_SRAMSIZ_8K	(8 << 16)
+#define			AT91_CIDR_SRAMSIZ_16K	(9 << 16)
+#define			AT91_CIDR_SRAMSIZ_32K	(10 << 16)
+#define			AT91_CIDR_SRAMSIZ_64K	(11 << 16)
+#define			AT91_CIDR_SRAMSIZ_128K	(12 << 16)
+#define			AT91_CIDR_SRAMSIZ_256K	(13 << 16)
+#define			AT91_CIDR_SRAMSIZ_96K	(14 << 16)
+#define			AT91_CIDR_SRAMSIZ_512K	(15 << 16)
+#define		AT91_CIDR_ARCH		(0xff << 20) /* Architecture Id */
+#define		AT91_CIDR_NVPTYP	(7    << 28) /* Nonvolatile Program
+							Memory Type */
+#define		AT91_CIDR_EXT		(1    << 31)	/* Extension Flag */
+
+#endif
diff --git a/include/asm-arm/arch-at91/cpu.h b/include/asm-arm/arch-at91/cpu.h
new file mode 100644
index 0000000..b5e5715
--- /dev/null
+++ b/include/asm-arm/arch-at91/cpu.h
@@ -0,0 +1,99 @@
+/*
+ * include/asm-arm/arch-at91/cpu.h
+ *
+ *  Copyright (C) 2006 SAN People
+ *
+ * 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.
+ *
+ */
+
+#ifndef __ASM_ARCH_CPU_H
+#define __ASM_ARCH_CPU_H
+
+#include <asm/hardware.h>
+#include <asm/arch/at91_dbgu.h>
+#include <asm/arch/io.h>
+
+
+#define ARCH_ID_AT91RM9200	0x09290780
+#define ARCH_ID_AT91SAM9260	0x019803a0
+#define ARCH_ID_AT91SAM9261	0x019703a0
+#define ARCH_ID_AT91SAM9263	0x019607a0
+#define ARCH_ID_AT91SAM9RL64	0x019b03a0
+#define ARCH_ID_AT91CAP9	0x039A03A0
+
+#define ARCH_ID_AT91SAM9XE128	0x329973a0
+#define ARCH_ID_AT91SAM9XE256	0x329a93a0
+#define ARCH_ID_AT91SAM9XE512	0x329aa3a0
+
+#define ARCH_ID_AT91M40800	0x14080044
+#define ARCH_ID_AT91R40807	0x44080746
+#define ARCH_ID_AT91M40807	0x14080745
+#define ARCH_ID_AT91R40008	0x44000840
+
+static inline unsigned long at91_cpu_identify(void)
+{
+	return (at91_sys_read(AT91_DBGU_CIDR) & ~AT91_CIDR_VERSION);
+}
+
+
+#define ARCH_FAMILY_AT91X92	0x09200000
+#define ARCH_FAMILY_AT91SAM9	0x01900000
+#define ARCH_FAMILY_AT91SAM9XE	0x02900000
+
+static inline unsigned long at91_arch_identify(void)
+{
+	return (at91_sys_read(AT91_DBGU_CIDR) & AT91_CIDR_ARCH);
+}
+
+
+#ifdef CONFIG_AT91RM9200
+/* no actual detection for this architecture */
+#define cpu_is_at91rm9200()	(1)
+#else
+#define cpu_is_at91rm9200()	(0)
+#endif
+
+#ifdef CONFIG_AT91SAM9261
+#define cpu_is_at91sam9xe()	(at91_arch_identify() == ARCH_FAMILY_AT91SAM9XE)
+#define cpu_is_at91sam9260()	((at91_cpu_identify() == ARCH_ID_AT91SAM9260) \
+					|| cpu_is_at91sam9xe())
+#else
+#define cpu_is_at91sam9xe()	(0)
+#define cpu_is_at91sam9260()	(0)
+#endif
+
+#ifdef CONFIG_AT91SAM9261
+#define cpu_is_at91sam9261()	(at91_cpu_identify() == ARCH_ID_AT91SAM9261)
+#else
+#define cpu_is_at91sam9261()	(0)
+#endif
+
+#ifdef CONFIG_AT91SAM9261
+#define cpu_is_at91sam9263()	(at91_cpu_identify() == ARCH_ID_AT91SAM9263)
+#else
+#define cpu_is_at91sam9263()	(0)
+#endif
+
+#ifdef CONFIG_AT91SAM9RL
+#define cpu_is_at91sam9rl()	(at91_cpu_identify() == ARCH_ID_AT91SAM9RL64)
+#else
+#define cpu_is_at91sam9rl()	(0)
+#endif
+
+#ifdef CONFIG_AT91CAP9
+#define cpu_is_at91cap9()	(at91_cpu_identify() == ARCH_ID_AT91CAP9)
+#else
+#define cpu_is_at91cap9()	(0)
+#endif
+
+/*
+ * Since this is ARM, we will never run on any AVR32 CPU. But these
+ * definitions may reduce clutter in common drivers.
+ */
+#define cpu_is_at32ap7000()	(0)
+
+#endif
-- 
1.6.0.4

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [U-Boot] [PATCH] Enable display CPU-Info for at91sam9261
  2009-06-01 18:21   ` [U-Boot] [PATCH V2] Add display cpuinfo for Atmel at91 arm926ejs based cores Remy Bohmer
@ 2009-06-01 18:21     ` Remy Bohmer
  2009-06-01 20:03       ` Stelian Pop
  2009-06-01 19:59     ` [U-Boot] [PATCH V2] Add display cpuinfo for Atmel at91 arm926ejs based cores Stelian Pop
  1 sibling, 1 reply; 9+ messages in thread
From: Remy Bohmer @ 2009-06-01 18:21 UTC (permalink / raw)
  To: u-boot

Signed-off-by: Remy Bohmer <linux@bohmer.net>
---
 include/configs/at91sam9261ek.h |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/include/configs/at91sam9261ek.h b/include/configs/at91sam9261ek.h
index fdaa71c..9a50347 100644
--- a/include/configs/at91sam9261ek.h
+++ b/include/configs/at91sam9261ek.h
@@ -45,6 +45,8 @@
 #define CONFIG_SKIP_LOWLEVEL_INIT
 #define CONFIG_SKIP_RELOCATE_UBOOT
 
+#define CONFIG_DISPLAY_CPUINFO          1
+
 /*
  * Hardware drivers
  */
-- 
1.6.0.4

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [U-Boot] [PATCH V2] Add display cpuinfo for Atmel at91 arm926ejs based cores
  2009-06-01 18:21   ` [U-Boot] [PATCH V2] Add display cpuinfo for Atmel at91 arm926ejs based cores Remy Bohmer
  2009-06-01 18:21     ` [U-Boot] [PATCH] Enable display CPU-Info for at91sam9261 Remy Bohmer
@ 2009-06-01 19:59     ` Stelian Pop
  2009-06-01 20:20       ` Jean-Christophe PLAGNIOL-VILLARD
  1 sibling, 1 reply; 9+ messages in thread
From: Stelian Pop @ 2009-06-01 19:59 UTC (permalink / raw)
  To: u-boot

Hi R?my,

I do have a few comments, see below:

On Mon, Jun 01, 2009 at 08:21:42PM +0200, Remy Bohmer wrote:

>  V2 reworked comments from Jean-Christophe PLAGNIOL-VILLARD
[...]

> +COBJS-$(CONFIG_DISPLAY_CPUINFO) +=cpuinfo.o

missing space ?

> +	switch ((cidr & AT91_CIDR_EPROC) >> 5) {
> +	 /* 0x1 = ARM946ES, 0x2 = ARM7TDMI, 0x4 = ARM920T */
> +	case 0x5: txt = "ARM926EJS";	break;
> +	default:  txt = "undefined";	break;

since there are only four choices (until now at least...), why not
put the 'txt' for all of them ?

and the 'undefined' string is not a good choice: it represents a
configuration not known to U-Boot, but its numeric value is well
defined. So I suggest you change this to 'unknown', or even better,
'unknown: 0xfoo'.

> +	}
> +	printf("Embedded Processor: %s\n", txt);
> +
> +	switch ((cidr & AT91_CIDR_ARCH) >> 20) {
> +	case 0x19: txt = "AT91SAM9xx";		break;
> +	case 0x29: txt = "AT91SAM9XExx";	break;
> +	case 0x39: txt = "CAP9";		break;
> +	default:   txt = "undefined";		break;

same comments as above for 'undefined'.

> +	switch ((cidr & AT91_CIDR_NVPTYP) >> 28) {
> +	case 0x0: txt = "ROM";				 break;
> +	case 0x1: txt = "ROMless or onchip flash";	 break;
> +	case 0x2: txt = "Embedded flash memory";	 break;
> +	case 0x3: txt = "ROM and Embedded flash memory"; break;
> +	case 0x4: txt = "SRAM emulating ROM";		 break;
> +	default:  txt = "undefined";			 break;

ditto.

> +	switch (cidr & AT91_CIDR_SRAMSIZ) {
> +	case AT91_CIDR_SRAMSIZ_1K: txt = "1K";		break;
> +	case AT91_CIDR_SRAMSIZ_2K: txt = "2K";		break;
> +	case AT91_CIDR_SRAMSIZ_112K: txt = "112K";	break;
> +	case AT91_CIDR_SRAMSIZ_4K: txt = "4K";		break;
> +	case AT91_CIDR_SRAMSIZ_80K: txt = "80K";	break;
> +	case AT91_CIDR_SRAMSIZ_160K: txt = "160K";	break;
> +	case AT91_CIDR_SRAMSIZ_8K: txt = "8K";		break;
> +	case AT91_CIDR_SRAMSIZ_16K: txt = "16K";	break;
> +	case AT91_CIDR_SRAMSIZ_32K: txt = "32K";	break;
> +	case AT91_CIDR_SRAMSIZ_64K: txt = "64K";	break;
> +	case AT91_CIDR_SRAMSIZ_128K: txt = "128K";	break;
> +	case AT91_CIDR_SRAMSIZ_256K: txt = "256K";	break;
> +	case AT91_CIDR_SRAMSIZ_96K: txt = "96K";	break;
> +	case AT91_CIDR_SRAMSIZ_512K: txt = "512K";	break;
> +	default:  txt = "undefined";			break;

bad alignment here (you did align the 'txt =' properly in all other
switch/case statements, but not here).

ditto for 'undefined'.

Stelian.

-- 
Stelian Pop <stelian@popies.net>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [U-Boot] [PATCH] Enable display CPU-Info for at91sam9261
  2009-06-01 18:21     ` [U-Boot] [PATCH] Enable display CPU-Info for at91sam9261 Remy Bohmer
@ 2009-06-01 20:03       ` Stelian Pop
  2009-06-01 20:22         ` Jean-Christophe PLAGNIOL-VILLARD
  0 siblings, 1 reply; 9+ messages in thread
From: Stelian Pop @ 2009-06-01 20:03 UTC (permalink / raw)
  To: u-boot

On Mon, Jun 01, 2009 at 08:21:43PM +0200, Remy Bohmer wrote:

> Signed-off-by: Remy Bohmer <linux@bohmer.net>
> ---
>  include/configs/at91sam9261ek.h |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/include/configs/at91sam9261ek.h b/include/configs/at91sam9261ek.h
> index fdaa71c..9a50347 100644
> --- a/include/configs/at91sam9261ek.h
> +++ b/include/configs/at91sam9261ek.h
> @@ -45,6 +45,8 @@
>  #define CONFIG_SKIP_LOWLEVEL_INIT
>  #define CONFIG_SKIP_RELOCATE_UBOOT
>  
> +#define CONFIG_DISPLAY_CPUINFO          1

For homogeneity reasons, since this is a generic AT91 config option, I think
it makes sense to define this by default for all the AT91 boards - or not
define it at all. Your pick.

Stelian.
-- 
Stelian Pop <stelian@popies.net>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [U-Boot] [PATCH V2] Add display cpuinfo for Atmel at91 arm926ejs based cores
  2009-06-01 19:59     ` [U-Boot] [PATCH V2] Add display cpuinfo for Atmel at91 arm926ejs based cores Stelian Pop
@ 2009-06-01 20:20       ` Jean-Christophe PLAGNIOL-VILLARD
  0 siblings, 0 replies; 9+ messages in thread
From: Jean-Christophe PLAGNIOL-VILLARD @ 2009-06-01 20:20 UTC (permalink / raw)
  To: u-boot

On 21:59 Mon 01 Jun     , Stelian Pop wrote:
> Hi R?my,
> 
> I do have a few comments, see below:
> 
> On Mon, Jun 01, 2009 at 08:21:42PM +0200, Remy Bohmer wrote:
> 
> >  V2 reworked comments from Jean-Christophe PLAGNIOL-VILLARD
> [...]
> 
> > +COBJS-$(CONFIG_DISPLAY_CPUINFO) +=cpuinfo.o
> 
> missing space ?
> 
> > +	switch ((cidr & AT91_CIDR_EPROC) >> 5) {
> > +	 /* 0x1 = ARM946ES, 0x2 = ARM7TDMI, 0x4 = ARM920T */
> > +	case 0x5: txt = "ARM926EJS";	break;
> > +	default:  txt = "undefined";	break;
> 
> since there are only four choices (until now at least...), why not
> put the 'txt' for all of them ?
We need to use the ifdef to reduce the size impact and not implement
non-supported code is important
> 
> and the 'undefined' string is not a good choice: it represents a
> configuration not known to U-Boot, but its numeric value is well
> defined. So I suggest you change this to 'unknown', or even better,
> 'unknown: 0xfoo'.
I agree

Best Regards,
J.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [U-Boot] [PATCH] Enable display CPU-Info for at91sam9261
  2009-06-01 20:03       ` Stelian Pop
@ 2009-06-01 20:22         ` Jean-Christophe PLAGNIOL-VILLARD
  2009-06-02 19:17           ` Remy Bohmer
  0 siblings, 1 reply; 9+ messages in thread
From: Jean-Christophe PLAGNIOL-VILLARD @ 2009-06-01 20:22 UTC (permalink / raw)
  To: u-boot

On 22:03 Mon 01 Jun     , Stelian Pop wrote:
> On Mon, Jun 01, 2009 at 08:21:43PM +0200, Remy Bohmer wrote:
> 
> > Signed-off-by: Remy Bohmer <linux@bohmer.net>
> > ---
> >  include/configs/at91sam9261ek.h |    2 ++
> >  1 files changed, 2 insertions(+), 0 deletions(-)
> > 
> > diff --git a/include/configs/at91sam9261ek.h b/include/configs/at91sam9261ek.h
> > index fdaa71c..9a50347 100644
> > --- a/include/configs/at91sam9261ek.h
> > +++ b/include/configs/at91sam9261ek.h
> > @@ -45,6 +45,8 @@
> >  #define CONFIG_SKIP_LOWLEVEL_INIT
> >  #define CONFIG_SKIP_RELOCATE_UBOOT
> >  
> > +#define CONFIG_DISPLAY_CPUINFO          1
> 
> For homogeneity reasons, since this is a generic AT91 config option, I think
> it makes sense to define this by default for all the AT91 boards - or not
> define it at all. Your pick.
for the Atmel's ref boards maybe but for other boards we need to let this
choice to each board Maintainer

Best Regards,
J.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [U-Boot] [PATCH] Enable display CPU-Info for at91sam9261
  2009-06-01 20:22         ` Jean-Christophe PLAGNIOL-VILLARD
@ 2009-06-02 19:17           ` Remy Bohmer
  0 siblings, 0 replies; 9+ messages in thread
From: Remy Bohmer @ 2009-06-02 19:17 UTC (permalink / raw)
  To: u-boot

Hello,

>> > @@ -45,6 +45,8 @@
>> > ?#define CONFIG_SKIP_LOWLEVEL_INIT
>> > ?#define CONFIG_SKIP_RELOCATE_UBOOT
>> >
>> > +#define CONFIG_DISPLAY_CPUINFO ? ? ? ? ?1
>>
>> For homogeneity reasons, since this is a generic AT91 config option, I think
>> it makes sense to define this by default for all the AT91 boards - or not
>> define it at all. Your pick.

Well, I only have the at91sam9261ek board, and as such it is the only
one I can test.
I do not prefer enabling things for boards I cannot test. Once the
framework is there it can be enabled, if desired.

> for the Atmel's ref boards maybe but for other boards we need to let this
> choice to each board Maintainer

I agree on that.

Kind Regards,

Remy

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2009-06-02 19:17 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-05-02 20:06 [U-Boot] [patch] Add display cpuinfo for Atmel at91sam9261 Cores Remy Bohmer
2009-05-18 20:25 ` Jean-Christophe PLAGNIOL-VILLARD
2009-06-01 18:21   ` [U-Boot] [PATCH V2] Add display cpuinfo for Atmel at91 arm926ejs based cores Remy Bohmer
2009-06-01 18:21     ` [U-Boot] [PATCH] Enable display CPU-Info for at91sam9261 Remy Bohmer
2009-06-01 20:03       ` Stelian Pop
2009-06-01 20:22         ` Jean-Christophe PLAGNIOL-VILLARD
2009-06-02 19:17           ` Remy Bohmer
2009-06-01 19:59     ` [U-Boot] [PATCH V2] Add display cpuinfo for Atmel at91 arm926ejs based cores Stelian Pop
2009-06-01 20:20       ` Jean-Christophe PLAGNIOL-VILLARD

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox