diff mbox

[U-Boot] omap3: overo: Select fdtfile for expansion board

Message ID 1402079878-27605-1-git-send-email-ashcharles@gmail.com
State Changes Requested
Delegated to: Tom Rini
Headers show

Commit Message

Ash Charles June 6, 2014, 6:37 p.m. UTC
The u-boot Overo board actually supports both Overo (OMAP35xx)
and Overo Storm (AM/DM37xx) COMs with a range of different expansion
boards.  This provides a mechanism to select the an appropriate device
tree file based on the processor version and, if available, the
expansion board ID written on the expansion board EEPROM. To match the
3.15+ kernels, fdtfile names have this format:
 "omap3-overo[-storm]-<expansion board name>.dtb"

By default, we use "omap3-overo-storm-tobi.dtb".

Signed-off-by: Ash Charles <ashcharles@gmail.com>

Conflicts:
	include/configs/omap3_overo.h
---
 board/overo/overo.c           | 48 +++++++++++++++++++++++++++++++++++++++++++
 include/configs/omap3_overo.h |  2 +-
 2 files changed, 49 insertions(+), 1 deletion(-)

Comments

Stefan Herbrechtsmeier June 7, 2014, 2:29 p.m. UTC | #1
Am 06.06.2014 20:37, schrieb Ash Charles:
> The u-boot Overo board actually supports both Overo (OMAP35xx)
> and Overo Storm (AM/DM37xx) COMs with a range of different expansion
> boards.  This provides a mechanism to select the an appropriate device
> tree file based on the processor version and, if available, the
> expansion board ID written on the expansion board EEPROM. To match the
> 3.15+ kernels, fdtfile names have this format:
>   "omap3-overo[-storm]-<expansion board name>.dtb"
>
> By default, we use "omap3-overo-storm-tobi.dtb".
>
> Signed-off-by: Ash Charles <ashcharles@gmail.com>
>
> Conflicts:
> 	include/configs/omap3_overo.h
> ---
>   board/overo/overo.c           | 48 +++++++++++++++++++++++++++++++++++++++++++
>   include/configs/omap3_overo.h |  2 +-
>   2 files changed, 49 insertions(+), 1 deletion(-)
>
> diff --git a/board/overo/overo.c b/board/overo/overo.c
> index 62b50a8..6a0a655 100644
> --- a/board/overo/overo.c
> +++ b/board/overo/overo.c
> @@ -230,6 +230,52 @@ unsigned int get_expansion_id(void)
>   	return expansion_config.device_vendor;
>   }
>   
> +#ifdef CONFIG_OF_LIBFDT
> +void set_fdt(void)
> +{
> +	int is_storm = 0;
> +	char fdtname[64];
> +	char expansion_name[32];
> +
> +        if (get_cpu_family() != CPU_OMAP34XX)
> +		is_storm = 1;
> +
> +        switch (expansion_config.device_vendor) {
> +        case GUMSTIX_SUMMIT:
> +		snprintf(expansion_name, ARRAY_SIZE(expansion_name), "%s",
> +		        "summit");
> +		break;
> +	case GUMSTIX_CHESTNUT43:
> +		snprintf(expansion_name, ARRAY_SIZE(expansion_name), "%s",
> +		        "chestnut43");
> +		break;
> +	case GUMSTIX_PALO43:
> +		snprintf(expansion_name, ARRAY_SIZE(expansion_name), "%s",
> +		        "palo43");
> +		break;
> +	case GUMSTIX_GALLOP43:
> +		snprintf(expansion_name, ARRAY_SIZE(expansion_name), "%s",
> +		        "gallop43");
> +		break;
> +	case GUMSTIX_ALTO35:
> +		snprintf(expansion_name, ARRAY_SIZE(expansion_name), "%s",
> +		        "alto35");
> +		break;
> +	case GUMSTIX_TOBI:
> +	case GUMSTIX_NO_EEPROM:
> +	default:
> +		snprintf(expansion_name, ARRAY_SIZE(expansion_name), "%s",
> +		        "tobi");
Why you use the tobi as default (unknown expansion board)?

> +	}
> +
> +        snprintf(fdtname, ARRAY_SIZE(fdtname),  "omap3-overo%s-%s.dtb",
> +                 is_storm ? "-storm" : "", expansion_name);
> +        setenv("fdtfile", fdtname);
This makes it impossible to save the fdtfile in the environment.

Why you don't use an command from the environment and add a boardname as 
you do on the duovero?

It would be nice if the overo could support even unknown boards. I'm 
currently working on some patches to separate the setup of the overo 
board and the expansion board. Together with a fdtfile (or board) name 
in the eeprom the overo could boot on foreigner expansion boards without 
any changes on the bootloader.

> +}
> +#else
> +void set_fdt(void) { return; }
> +#endif
> +
>   /*
>    * Routine: misc_init_r
>    * Description: Configure board specific parts
> @@ -360,6 +406,8 @@ int misc_init_r(void)
>   
>   	dieid_num_r();
>   
> +	set_fdt();
> +
>   	return 0;
>   }
>   
> diff --git a/include/configs/omap3_overo.h b/include/configs/omap3_overo.h
> index 7b97be9..f79a528 100644
> --- a/include/configs/omap3_overo.h
> +++ b/include/configs/omap3_overo.h
> @@ -83,7 +83,7 @@
>   /* Environment information */
>   #define CONFIG_EXTRA_ENV_SETTINGS \
>   	DEFAULT_LINUX_BOOT_ENV \
> -	"fdtfile=overo.dtb\0" \
> +	"fdtfile=omap3-overo-storm-tobi.dtb\0" \
>   	"bootdir=/boot\0" \
>   	"bootfile=zImage\0" \
>   	"usbtty=cdc_acm\0" \
Tom Rini June 7, 2014, 5:48 p.m. UTC | #2
On Sat, Jun 07, 2014 at 04:29:27PM +0200, Stefan Herbrechtsmeier wrote:
> Am 06.06.2014 20:37, schrieb Ash Charles:
> >The u-boot Overo board actually supports both Overo (OMAP35xx)
> >and Overo Storm (AM/DM37xx) COMs with a range of different expansion
> >boards.  This provides a mechanism to select the an appropriate device
> >tree file based on the processor version and, if available, the
> >expansion board ID written on the expansion board EEPROM. To match the
> >3.15+ kernels, fdtfile names have this format:
> >  "omap3-overo[-storm]-<expansion board name>.dtb"
[snip]
> >+        snprintf(fdtname, ARRAY_SIZE(fdtname),  "omap3-overo%s-%s.dtb",
> >+                 is_storm ? "-storm" : "", expansion_name);
> >+        setenv("fdtfile", fdtname);
> This makes it impossible to save the fdtfile in the environment.
> 
> Why you don't use an command from the environment and add a
> boardname as you do on the duovero?

In this case it might be harder to do this in a shell cmd.  But we
_must_ check for fdtfile being unset and only setenv if not already set.
And then drop fdtfile from the config file.
Ash Charles June 7, 2014, 8:11 p.m. UTC | #3
On Sat, Jun 7, 2014 at 10:48 AM, Tom Rini <trini@ti.com> wrote:
> In this case it might be harder to do this in a shell cmd.  But we
> _must_ check for fdtfile being unset and only setenv if not already set.
> And then drop fdtfile from the config file.

Stefan and Tom,

Thanks both for your comments and suggestions.  I agree that fdtfile
should only be set if it is currently unset---I'll send an updated
patch.
There is actually already a mechanism for environment variables to be
set by the board eeprom [1][2] so an expansion board could certainly
set the desired fdtfile name.  The challenge (and my biggest
motivation for writing the patch) is making sure we select a dtb for
either 37xx (Overo Storm) or 35xx (non-storm) as otherwise Linux
doesn't boot.

--Ash
[1] http://elinux.org/BeagleBoardPinMux#Expansion_boards
[2] https://github.com/gumstix/meta-gumstix-extras/blob/dylan/recipes-core/i2c-tools/omap3-writeprom/writeprom.sh
Stefan Herbrechtsmeier June 8, 2014, 7:57 a.m. UTC | #4
Am 07.06.2014 22:11, schrieb Ash Charles:
> On Sat, Jun 7, 2014 at 10:48 AM, Tom Rini <trini@ti.com> wrote:
>> In this case it might be harder to do this in a shell cmd.  But we
>> _must_ check for fdtfile being unset and only setenv if not already set.
>> And then drop fdtfile from the config file.
> Thanks both for your comments and suggestions.  I agree that fdtfile
> should only be set if it is currently unset---I'll send an updated
> patch.
It would be nice if you only set an environment variable 'expansionname' 
and setup the fdtfile via a command in the environment. This makes sure, 
that under normal condition the fdtfile is not set and thereby not saved 
in the environment with the command saveenv.

> There is actually already a mechanism for environment variables to be
> set by the board eeprom [1][2] so an expansion board could certainly
> set the desired fdtfile name.
At the moment I use this functionality to set the environment variable 
'fdtfile'.

>    The challenge (and my biggest
> motivation for writing the patch) is making sure we select a dtb for
> either 37xx (Overo Storm) or 35xx (non-storm) as otherwise Linux
> doesn't boot.
Maybe you could set an environment variable 'boardname' to 'overo' or 
'overo-storm' and use something like the following environment command:

if test -n \"${boardname}\" && test -n \"${expansionname}\"; then
     setenv fdtfile omap3-${boardname}-${expansionname}.dtb;
fi;

This allows the expansion board to set the expansionname via eeprom and 
works with both overo boards.

>
> --Ash
> [1] http://elinux.org/BeagleBoardPinMux#Expansion_boards
> [2] https://github.com/gumstix/meta-gumstix-extras/blob/dylan/recipes-core/i2c-tools/omap3-writeprom/writeprom.sh
Ash Charles June 9, 2014, 8:47 p.m. UTC | #5
On Sun, Jun 8, 2014 at 12:57 AM, Stefan Herbrechtsmeier
<stefan@herbrechtsmeier.net> wrote:
> At the moment I use this functionality to set the environment variable
> 'fdtfile'.
Cool :)
....
> Maybe you could set an environment variable 'boardname' to 'overo' or
> 'overo-storm' and use something like the following environment command:
>
> if test -n \"${boardname}\" && test -n \"${expansionname}\"; then
>     setenv fdtfile omap3-${boardname}-${expansionname}.dtb;
> fi;
>
> This allows the expansion board to set the expansionname via eeprom and
> works with both overo boards.
What is the value in testing for ${boardname} and ${expansionname}?
Shouldn't I just test for fdtfile and, if it is not set, proceed to
set it you propose? ${boardname} and ${expansionname} could be set on
boot. I'm sending an updated version of the patch for your comments
(sorry..compile-tested only as I'm not near hardware today).

--Ash
diff mbox

Patch

diff --git a/board/overo/overo.c b/board/overo/overo.c
index 62b50a8..6a0a655 100644
--- a/board/overo/overo.c
+++ b/board/overo/overo.c
@@ -230,6 +230,52 @@  unsigned int get_expansion_id(void)
 	return expansion_config.device_vendor;
 }
 
+#ifdef CONFIG_OF_LIBFDT
+void set_fdt(void)
+{
+	int is_storm = 0;
+	char fdtname[64];
+	char expansion_name[32];
+
+        if (get_cpu_family() != CPU_OMAP34XX)
+		is_storm = 1;
+
+        switch (expansion_config.device_vendor) {
+        case GUMSTIX_SUMMIT:
+		snprintf(expansion_name, ARRAY_SIZE(expansion_name), "%s",
+		        "summit");
+		break;
+	case GUMSTIX_CHESTNUT43:
+		snprintf(expansion_name, ARRAY_SIZE(expansion_name), "%s",
+		        "chestnut43");
+		break;
+	case GUMSTIX_PALO43:
+		snprintf(expansion_name, ARRAY_SIZE(expansion_name), "%s",
+		        "palo43");
+		break;
+	case GUMSTIX_GALLOP43:
+		snprintf(expansion_name, ARRAY_SIZE(expansion_name), "%s",
+		        "gallop43");
+		break;
+	case GUMSTIX_ALTO35:
+		snprintf(expansion_name, ARRAY_SIZE(expansion_name), "%s",
+		        "alto35");
+		break;
+	case GUMSTIX_TOBI:
+	case GUMSTIX_NO_EEPROM:
+	default:
+		snprintf(expansion_name, ARRAY_SIZE(expansion_name), "%s",
+		        "tobi");
+	}
+
+        snprintf(fdtname, ARRAY_SIZE(fdtname),  "omap3-overo%s-%s.dtb",
+                 is_storm ? "-storm" : "", expansion_name);
+        setenv("fdtfile", fdtname);
+}
+#else
+void set_fdt(void) { return; }
+#endif
+
 /*
  * Routine: misc_init_r
  * Description: Configure board specific parts
@@ -360,6 +406,8 @@  int misc_init_r(void)
 
 	dieid_num_r();
 
+	set_fdt();
+
 	return 0;
 }
 
diff --git a/include/configs/omap3_overo.h b/include/configs/omap3_overo.h
index 7b97be9..f79a528 100644
--- a/include/configs/omap3_overo.h
+++ b/include/configs/omap3_overo.h
@@ -83,7 +83,7 @@ 
 /* Environment information */
 #define CONFIG_EXTRA_ENV_SETTINGS \
 	DEFAULT_LINUX_BOOT_ENV \
-	"fdtfile=overo.dtb\0" \
+	"fdtfile=omap3-overo-storm-tobi.dtb\0" \
 	"bootdir=/boot\0" \
 	"bootfile=zImage\0" \
 	"usbtty=cdc_acm\0" \