diff mbox

[U-Boot] mx6qsabrelite: Remove mx6qsabrelite code in favor of nitrogen6x

Message ID 1373856014-20390-1-git-send-email-festevam@gmail.com
State Changes Requested
Delegated to: Stefano Babic
Headers show

Commit Message

Fabio Estevam July 15, 2013, 2:40 a.m. UTC
From: Fabio Estevam <fabio.estevam@freescale.com>

mx6qsabrelite and nitrogen6q boards are hardware compatible, so let's avoid the
code duplication and only use the nitrogen6x source code to make board code
maintainance easier.

Tested booting a mainline device tree kernel on a mx6qsabrelite board.

Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
 .../nitrogen6x/README.mx6qsabrelite}               |   0
 .../nitrogen6x/{README => README.nitrogen6x}       |   0
 board/freescale/mx6qsabrelite/Makefile             |  41 -
 board/freescale/mx6qsabrelite/mx6qsabrelite.c      | 848 ---------------------
 boards.cfg                                         |   2 +-
 include/configs/mx6qsabrelite.h                    | 297 --------
 include/configs/nitrogen6x.h                       |  80 +-
 7 files changed, 80 insertions(+), 1188 deletions(-)
 rename board/{freescale/mx6qsabrelite/README => boundary/nitrogen6x/README.mx6qsabrelite} (100%)
 rename board/boundary/nitrogen6x/{README => README.nitrogen6x} (100%)
 delete mode 100644 board/freescale/mx6qsabrelite/Makefile
 delete mode 100644 board/freescale/mx6qsabrelite/mx6qsabrelite.c
 delete mode 100644 include/configs/mx6qsabrelite.h

Comments

Fabio Estevam July 15, 2013, 2:52 a.m. UTC | #1
Hi Otavio,

On Sun, Jul 14, 2013 at 11:44 PM, Otavio Salvador
<otavio@ossystems.com.br> wrote:
> On Sun, Jul 14, 2013 at 11:40 PM, Fabio Estevam <festevam@gmail.com> wrote:
>> From: Fabio Estevam <fabio.estevam@freescale.com>
>>
>> mx6qsabrelite and nitrogen6q boards are hardware compatible, so let's avoid the
>> code duplication and only use the nitrogen6x source code to make board code
>> maintainance easier.
>>
>> Tested booting a mainline device tree kernel on a mx6qsabrelite board.
>>
>> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
>
> I think this is a huge improvement I am just not sure about the
> environment preserving for sabrelite. I think most people using
> sabrelite end using the U-Boot from SPI NOR.

I can't say whether SPI NOR or SD card is the most common medium for
storing the environment variables for sabrelite, but my main goal here
was to try to keep compability with existing code.

Currently mx6qsabrelite defines CONFIG_ENV_IS_IN_MMC, so I kept the
same here. This patch introduces no change in functional behaviour.

If we think CONFIG_ENV_IS_IN_xxx should be changed, then this is
something to be handled by a separate patch.

Regards,

Fabio Estevam
Otavio Salvador July 15, 2013, 2:58 a.m. UTC | #2
On Sun, Jul 14, 2013 at 11:52 PM, Fabio Estevam <festevam@gmail.com> wrote:
> Hi Otavio,
>
> On Sun, Jul 14, 2013 at 11:44 PM, Otavio Salvador
> <otavio@ossystems.com.br> wrote:
>> On Sun, Jul 14, 2013 at 11:40 PM, Fabio Estevam <festevam@gmail.com> wrote:
>>> From: Fabio Estevam <fabio.estevam@freescale.com>
>>>
>>> mx6qsabrelite and nitrogen6q boards are hardware compatible, so let's avoid the
>>> code duplication and only use the nitrogen6x source code to make board code
>>> maintainance easier.
>>>
>>> Tested booting a mainline device tree kernel on a mx6qsabrelite board.
>>>
>>> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
>>
>> I think this is a huge improvement I am just not sure about the
>> environment preserving for sabrelite. I think most people using
>> sabrelite end using the U-Boot from SPI NOR.
>
> I can't say whether SPI NOR or SD card is the most common medium for
> storing the environment variables for sabrelite, but my main goal here
> was to try to keep compability with existing code.
>
> Currently mx6qsabrelite defines CONFIG_ENV_IS_IN_MMC, so I kept the
> same here. This patch introduces no change in functional behaviour.
>
> If we think CONFIG_ENV_IS_IN_xxx should be changed, then this is
> something to be handled by a separate patch.

Agreed :-)

Reviewed-by: Otavio Salvador <otavio@ossystems.com.br>

--
Otavio Salvador                             O.S. Systems
http://www.ossystems.com.br        http://projetos.ossystems.com.br
Mobile: +55 (53) 9981-7854            Mobile: +1 (347) 903-9750
Eric Nelson July 15, 2013, 4:09 a.m. UTC | #3
On 07/14/2013 07:58 PM, Otavio Salvador wrote:
> On Sun, Jul 14, 2013 at 11:52 PM, Fabio Estevam <festevam@gmail.com> wrote:
>> Hi Otavio,
>>
>> On Sun, Jul 14, 2013 at 11:44 PM, Otavio Salvador
>> <otavio@ossystems.com.br> wrote:
>>> On Sun, Jul 14, 2013 at 11:40 PM, Fabio Estevam <festevam@gmail.com> wrote:
>>>> From: Fabio Estevam <fabio.estevam@freescale.com>
>>>>
>>>> mx6qsabrelite and nitrogen6q boards are hardware compatible, so let's avoid the
>>>> code duplication and only use the nitrogen6x source code to make board code
>>>> maintainance easier.
>>>>
>>>> Tested booting a mainline device tree kernel on a mx6qsabrelite board.
>>>>
>>>> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
>>>
>>> I think this is a huge improvement I am just not sure about the
>>> environment preserving for sabrelite. I think most people using
>>> sabrelite end using the U-Boot from SPI NOR.
>>
>> I can't say whether SPI NOR or SD card is the most common medium for
>> storing the environment variables for sabrelite, but my main goal here
>> was to try to keep compability with existing code.
>>
>> Currently mx6qsabrelite defines CONFIG_ENV_IS_IN_MMC, so I kept the
>> same here. This patch introduces no change in functional behaviour.
>>
>> If we think CONFIG_ENV_IS_IN_xxx should be changed, then this is
>> something to be handled by a separate patch.
>
> Agreed :-)
>
> Reviewed-by: Otavio Salvador <otavio@ossystems.com.br>
>

+1

We should also add something to the README file though.
Fabio Estevam July 15, 2013, 4:23 a.m. UTC | #4
Hi Eric,

On Mon, Jul 15, 2013 at 1:09 AM, Eric Nelson
<eric.nelson@boundarydevices.com> wrote:

>> Agreed :-)
>>
>> Reviewed-by: Otavio Salvador <otavio@ossystems.com.br>
>>
>
> +1
>
> We should also add something to the README file though.

In this patch I am still using the original README's:

rename board/{freescale/mx6qsabrelite/README =>
boundary/nitrogen6x/README.mx6qsabrelite} (100%)
rename board/boundary/nitrogen6x/{README => README.nitrogen6x} (100%)

What would you like me to the README?

Regards,

Fabio Estevam
Stefano Babic July 15, 2013, 7:14 a.m. UTC | #5
Hi Fabio,

On 15/07/2013 04:40, Fabio Estevam wrote:
> From: Fabio Estevam <fabio.estevam@freescale.com>
> 
> mx6qsabrelite and nitrogen6q boards are hardware compatible, so let's avoid the
> code duplication and only use the nitrogen6x source code to make board code
> maintainance easier.
> 
> Tested booting a mainline device tree kernel on a mx6qsabrelite board.
> 
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> ---
>  .../nitrogen6x/README.mx6qsabrelite}               |   0
>  .../nitrogen6x/{README => README.nitrogen6x}       |   0
>  board/freescale/mx6qsabrelite/Makefile             |  41 -
>  board/freescale/mx6qsabrelite/mx6qsabrelite.c      | 848 ---------------------
>  boards.cfg                                         |   2 +-
>  include/configs/mx6qsabrelite.h                    | 297 --------
>  include/configs/nitrogen6x.h                       |  80 +-

Should we update the MAINTAINERS, too ? It is weird that we have two
maintainers for the same code.

Best regards,
Stefano
Eric Nelson July 15, 2013, 2:20 p.m. UTC | #6
Hi Fabio,

On 07/14/2013 09:23 PM, Fabio Estevam wrote:
> Hi Eric,
>
> On Mon, Jul 15, 2013 at 1:09 AM, Eric Nelson
> <eric.nelson@boundarydevices.com> wrote:
>
>>> Agreed :-)
>>>
>>> Reviewed-by: Otavio Salvador <otavio@ossystems.com.br>
>>>
>>
>> +1
>>
>> We should also add something to the README file though.
>
> In this patch I am still using the original README's:
>

Thanks for pointing it out. I had missed this.

> rename board/{freescale/mx6qsabrelite/README =>
> boundary/nitrogen6x/README.mx6qsabrelite} (100%)

I also have to admit not having read this README.

It appears to give pretty bad advice, suggesting that the
user use the iMX6DQ_SPI_to_uSDHC3.bin shim to force
boot from SDHC3.

I think this explains how people keep ending up at that
stale Linaro post.

> rename board/boundary/nitrogen6x/{README => README.nitrogen6x} (100%)
>
> What would you like me to the README?
>

It seems that there are two policy differences between
the mx6qsabrelite.h and nitrogen6x.h files:

1. Use of MMC for environment storage
2. Use of boot script in nitrogen6x

I think we can dispense with #1. Can you think of any reason
a user would care where this is stored?

The second is a bit more subtle. The boot script approach
allows booting any O/S from any FAT or ext2/3/4 from any
SD card or SATA).

OTOH, if there are a significant number of people who don't
have boot scripts in their image(s), we'll give them a
minor speed bump during the transition.

Since these are all environment settings, it seems easy
enough to allow things to be configured in "the Freescale way"
by adding a layer of in-direction.

i.e. we could point 'bootcmd' at either 'bootcmd_boundary'
or 'bootcmd_freescale' and allow a user to select their
flavour of boot.

This would prevent the need for a compile-time switch.

The other difference I note in the default environment
is the inclusion of network boot.

I don't think including this bit does any harm, though
I would suggest that it be a conscious choice and not
an automatic fall-back. In order to enable network
boot, a user already needs to configure at least the
server IP and boot path. Why not also ask them to
set 'bootcmd' to 'bootcmd_net'?

Let me know your thoughts.

Regards,


Eric
Stephan Bauroth July 15, 2013, 2:55 p.m. UTC | #7
Hi Eric, Hi everyone else,

> It seems that there are two policy differences between
> the mx6qsabrelite.h and nitrogen6x.h files:

> 1. Use of MMC for environment storage
> 2. Use of boot script in nitrogen6x

> I think we can dispense with #1. Can you think of any reason
> a user would care where this is stored?

I don't agree. I usually don't have a MMC put into the slot on my board, 
as I want to boot my board via NFS. Last week I had exactly the situation 
that my environment was resetted everytime i rebooted the board. Since i 
was testing a very unstable kernel, this happened a lot and i had to 
reinitialise the environment for networking booting every time. At the end 
it took me half an hour to find the swtich and the thing was done. But 
mentioning the fact in the README would have saved some time. :)


regards,
Stephan
Eric Nelson July 15, 2013, 3:31 p.m. UTC | #8
Hi Stephan,

On 07/15/2013 07:55 AM, Stephan Bauroth wrote:
> Hi Eric, Hi everyone else,
>
>  > It seems that there are two policy differences between
>  > the mx6qsabrelite.h and nitrogen6x.h files:
>
>  > 1. Use of MMC for environment storage
>  > 2. Use of boot script in nitrogen6x
>
>  > I think we can dispense with #1. Can you think of any reason
>  > a user would care where this is stored?
>
> I don't agree. I usually don't have a MMC put into the slot on my board,
> as I want to boot my board via NFS.

This sounds like a vote for having the environment in SPI-NOR.

 > Last week I had exactly the situation that my environment was
 > resetted everytime i rebooted the board. Since i was testing a
 > very unstable kernel, this happened a lot and i had to
 > reinitialise the environment for networking booting every
> time.

I'm not sure what you're not agreeing with. I'm suggesting that
in order to configure for boot to network, you'll need to
run a few commands:

	U-Boot > setenv serverip 192.168.0.44
	U-Boot > setenv nfsroot /path/to/rootfs
	U-Boot > setenv bootcmd "run bootcmd_net"
	U-Boot > saveenv

The 'bootcmd' is the question I have. The mx6qsabrelite.h file
currently has this:

	   "mmc dev ${mmcdev}; if mmc rescan; then " \
		   "if run loadbootscript; then " \
			   "run bootscript; " \
		   "else " \
			   "if run loaduimage; then " \
				   "run mmcboot; " \
			   "else run netboot; " \
			   "fi; " \
		   "fi; " \
	   "else run netboot; fi"

That is, "try to run from SD card, and network if that fails".

What I'm suggesting is that we make the SD card part of
things above 'bootcmd_freescale', so you have three choices
for configurations:

	U-Boot > setenv bootcmd "run bootcmd_boundary"
	U-Boot > setenv bootcmd "run bootcmd_freescale"
	U-Boot > setenv bootcmd "run bootcmd_net"

'bootcmd_freescale' and 'bootcmd_net' can be essentially
the Freescale scripts, so they match documentation done
by the Freescale team. 'bootcmd_boundary' would be the
default for boards we ship.

Any of these can be saved to SPI NOR for use at POR.

 > At the end it took me half an hour to find the swtich and the
> thing was done. But mentioning the fact in the README would have saved
> some time. :)
>

I think we're all in agreement about proper notes in the README.
We're just trying to figure out the policy to document.

Also note that since a user can over-ride 'bootcmd', all of this
are just defaults.

When I'm doing network boots, I find it easier to simply set the
'bootargs' and 'bootcmd' variables directly

U-Boot > setenv bootargs ... root=/dev/nfs nfsroot=...
U-Boot > setenv bootcmd 'dhcp 10800000 192.168.0.44:uImage && bootm'

This skips all of the conditionals and is much easier to verify.

Regards,


Eric
Fabio Estevam July 15, 2013, 3:33 p.m. UTC | #9
Hi Eric,

On Mon, Jul 15, 2013 at 11:20 AM, Eric Nelson
<eric.nelson@boundarydevices.com> wrote:

> Thanks for pointing it out. I had missed this.
>
>
>> rename board/{freescale/mx6qsabrelite/README =>
>> boundary/nitrogen6x/README.mx6qsabrelite} (100%)
>
>
> I also have to admit not having read this README.
>
> It appears to give pretty bad advice, suggesting that the
> user use the iMX6DQ_SPI_to_uSDHC3.bin shim to force
> boot from SDHC3.
>
> I think this explains how people keep ending up at that
> stale Linaro post.

As I explained to Otavio, this patch does not attempt to introduce any
change in current behaviour.

The only intention here is to get rid of code duplication.

If we want to change the current behaviour and align it with
nitrogen6x, then we should do this on a separate patch.

I will address Stefano's suggestion of changing the MAINTAINER file
and plan to send a v2 later today.

Thanks,

Fabio Estevam
Eric Nelson July 15, 2013, 3:43 p.m. UTC | #10
On 07/15/2013 08:33 AM, Fabio Estevam wrote:
> Hi Eric,
>
> On Mon, Jul 15, 2013 at 11:20 AM, Eric Nelson
> <eric.nelson@boundarydevices.com> wrote:
>
>> Thanks for pointing it out. I had missed this.
>>
>>
>>> rename board/{freescale/mx6qsabrelite/README =>
>>> boundary/nitrogen6x/README.mx6qsabrelite} (100%)
>>
>>
>> I also have to admit not having read this README.
>>
>> It appears to give pretty bad advice, suggesting that the
>> user use the iMX6DQ_SPI_to_uSDHC3.bin shim to force
>> boot from SDHC3.
>>
>> I think this explains how people keep ending up at that
>> stale Linaro post.
>
> As I explained to Otavio, this patch does not attempt to introduce any
> change in current behaviour.
>
> The only intention here is to get rid of code duplication.
>
> If we want to change the current behaviour and align it with
> nitrogen6x, then we should do this on a separate patch.
>
> I will address Stefano's suggestion of changing the MAINTAINER file
> and plan to send a v2 later today.
>

Ok.
diff mbox

Patch

diff --git a/board/freescale/mx6qsabrelite/README b/board/boundary/nitrogen6x/README.mx6qsabrelite
similarity index 100%
rename from board/freescale/mx6qsabrelite/README
rename to board/boundary/nitrogen6x/README.mx6qsabrelite
diff --git a/board/boundary/nitrogen6x/README b/board/boundary/nitrogen6x/README.nitrogen6x
similarity index 100%
rename from board/boundary/nitrogen6x/README
rename to board/boundary/nitrogen6x/README.nitrogen6x
diff --git a/board/freescale/mx6qsabrelite/Makefile b/board/freescale/mx6qsabrelite/Makefile
deleted file mode 100644
index cf344e4..0000000
diff --git a/board/freescale/mx6qsabrelite/mx6qsabrelite.c b/board/freescale/mx6qsabrelite/mx6qsabrelite.c
deleted file mode 100644
index 862bc30..0000000
diff --git a/boards.cfg b/boards.cfg
index db56488..86e2fd6 100644
--- a/boards.cfg
+++ b/boards.cfg
@@ -259,7 +259,7 @@  vision2                      arm         armv7       vision2             ttcontr
 cgtqmx6qeval				 arm		 armv7		 cgtqmx6eval		 congatec		mx6		cgtqmx6eval:IMX_CONFIG=board/freescale/imx/ddr/mx6q_4x_mt41j128.cfg,MX6Q
 mx6qarm2                     arm         armv7       mx6qarm2            freescale      mx6		mx6qarm2:IMX_CONFIG=board/freescale/mx6qarm2/imximage.cfg
 mx6qsabreauto                arm         armv7       mx6qsabreauto       freescale      mx6		mx6qsabreauto:IMX_CONFIG=board/freescale/mx6qsabreauto/imximage.cfg,MX6Q
-mx6qsabrelite                arm         armv7       mx6qsabrelite       freescale      mx6		mx6qsabrelite:IMX_CONFIG=board/freescale/imx/ddr/mx6q_4x_mt41j128.cfg
+mx6qsabrelite                arm         armv7       nitrogen6x          boundary       mx6		nitrogen6x:IMX_CONFIG=board/boundary/nitrogen6x/nitrogen6q.cfg,MX6Q,DDR_MB=1024,SABRELITE
 mx6dlsabresd                 arm         armv7       mx6sabresd          freescale      mx6		mx6sabresd:IMX_CONFIG=board/boundary/nitrogen6x/nitrogen6dl.cfg,MX6DL
 mx6qsabresd                  arm         armv7       mx6sabresd          freescale      mx6		mx6sabresd:IMX_CONFIG=board/freescale/imx/ddr/mx6q_4x_mt41j128.cfg,MX6Q
 mx6slevk                     arm         armv7       mx6slevk            freescale      mx6		mx6slevk:IMX_CONFIG=board/freescale/mx6slevk/imximage.cfg,MX6SL
diff --git a/include/configs/mx6qsabrelite.h b/include/configs/mx6qsabrelite.h
deleted file mode 100644
index c7db81d..0000000
diff --git a/include/configs/nitrogen6x.h b/include/configs/nitrogen6x.h
index 74df66c..85eecfc 100644
--- a/include/configs/nitrogen6x.h
+++ b/include/configs/nitrogen6x.h
@@ -186,6 +186,80 @@ 
 
 #define CONFIG_DRIVE_TYPES CONFIG_DRIVE_SATA CONFIG_DRIVE_MMC
 
+#if defined(CONFIG_SABRELITE)
+#define CONFIG_EXTRA_ENV_SETTINGS \
+	"script=boot.scr\0" \
+	"uimage=uImage\0" \
+	"console=ttymxc1\0" \
+	"fdt_high=0xffffffff\0" \
+	"initrd_high=0xffffffff\0" \
+	"fdt_file=imx6q-sabrelite.dtb\0" \
+	"fdt_addr=0x11000000\0" \
+	"boot_fdt=try\0" \
+	"ip_dyn=yes\0" \
+	"mmcdev=0\0" \
+	"mmcpart=1\0" \
+	"mmcroot=/dev/mmcblk0p2 rootwait rw\0" \
+	"mmcargs=setenv bootargs console=${console},${baudrate} " \
+		"root=${mmcroot}\0" \
+	"loadbootscript=" \
+		"fatload mmc ${mmcdev}:${mmcpart} ${loadaddr} ${script};\0" \
+	"bootscript=echo Running bootscript from mmc ...; " \
+		"source\0" \
+	"loaduimage=fatload mmc ${mmcdev}:${mmcpart} ${loadaddr} ${uimage}\0" \
+	"loadfdt=fatload mmc ${mmcdev}:${mmcpart} ${fdt_addr} ${fdt_file}\0" \
+	"mmcboot=echo Booting from mmc ...; " \
+		"run mmcargs; " \
+		"if test ${boot_fdt} = yes || test ${boot_fdt} = try; then " \
+			"if run loadfdt; then " \
+				"bootm ${loadaddr} - ${fdt_addr}; " \
+			"else " \
+				"if test ${boot_fdt} = try; then " \
+					"bootm; " \
+				"else " \
+					"echo WARN: Cannot load the DT; " \
+				"fi; " \
+			"fi; " \
+		"else " \
+			"bootm; " \
+		"fi;\0" \
+	"netargs=setenv bootargs console=${console},${baudrate} " \
+		"root=/dev/nfs " \
+	"ip=dhcp nfsroot=${serverip}:${nfsroot},v3,tcp\0" \
+		"netboot=echo Booting from net ...; " \
+		"run netargs; " \
+		"if test ${ip_dyn} = yes; then " \
+			"setenv get_cmd dhcp; " \
+		"else " \
+			"setenv get_cmd tftp; " \
+		"fi; " \
+		"${get_cmd} ${uimage}; " \
+		"if test ${boot_fdt} = yes || test ${boot_fdt} = try; then " \
+			"if ${get_cmd} ${fdt_addr} ${fdt_file}; then " \
+				"bootm ${loadaddr} - ${fdt_addr}; " \
+			"else " \
+				"if test ${boot_fdt} = try; then " \
+					"bootm; " \
+				"else " \
+					"echo WARN: Cannot load the DT; " \
+				"fi; " \
+			"fi; " \
+		"else " \
+			"bootm; " \
+		"fi;\0"
+
+#define CONFIG_BOOTCOMMAND \
+	   "mmc dev ${mmcdev}; if mmc rescan; then " \
+		   "if run loadbootscript; then " \
+			   "run bootscript; " \
+		   "else " \
+			   "if run loaduimage; then " \
+				   "run mmcboot; " \
+			   "else run netboot; " \
+			   "fi; " \
+		   "fi; " \
+	   "else run netboot; fi"
+#else
 #define CONFIG_EXTRA_ENV_SETTINGS \
 	"console=ttymxc1\0" \
 	"clearenv=if sf probe || sf probe || sf probe 1 ; then " \
@@ -219,6 +293,7 @@ 
 		"done ; " \
 	"done\0" \
 
+#endif
 /* Miscellaneous configurable options */
 #define CONFIG_SYS_LONGHELP
 #define CONFIG_SYS_HUSH_PARSER
@@ -258,8 +333,11 @@ 
 
 #define CONFIG_ENV_SIZE			(8 * 1024)
 
-/* #define CONFIG_ENV_IS_IN_MMC */
+#if defined(CONFIG_SABRELITE)
+#define CONFIG_ENV_IS_IN_MMC
+#else
 #define CONFIG_ENV_IS_IN_SPI_FLASH
+#endif
 
 #if defined(CONFIG_ENV_IS_IN_MMC)
 #define CONFIG_ENV_OFFSET		(6 * 64 * 1024)