[U-Boot] [PATCH 0/3] ARM: imx: enhance support for the cm-fx6 module

[U-Boot] [PATCH 0/3] ARM: imx: enhance support for the cm-fx6 module

[Christopher Spinrath]

Hi,

this series aims at enhancing support for the cm-fx6 module. In
particular, with respect to the upstream linux kernel.

The first patch improves the default environment. It is non-functional
but makes it more convenient to adapt certain settings.

The later two patches add mtd partition support for the on-board spi
flash chip. They pick up the discussion about specifying a default
partitioning in the device tree from here [1]. In short: adding the
default partitioning to the device tree was rejected by the linux/
device tree community during mainlining large parts of the device tree.
It was proposed to implement the partition/mtd handling in u-boot.
On the other hand, it was argued that the flash chip becomes some
kind of "black-box" since there will be no partition labeling (in
particular, with old u-boot versions).

IMHO defining the mtd partitioning has the following (dis-)advantages:

Advantages:
- It is easier for the user to change the partitioning (e.g. to use
  the unsued 1MB free space).

- The flash ship is used entirely for u-boot. So it is quite natural
  that u-boot manages it. Also, moving the partition table to it
  allows us to change the layout in future versions of u-boot (almost
  independently of the kernel - there are still non-device tree kernels).

- U-Boot becomes the single point of definition for all device tree
  kernels. Otherwise, each kernel (vendor vs. upstream + version)
  would ship its own partitioning. Moreover, u-boot has to know
  something about the partitioning, too, because it has to know where
  the environment is saved.

Disadvantages:
- Users of the upstream linux kernel have to use a recent u-boot
  version to avoid the "black box" effect. A concrete impact is
  that the update routine (described/proposed by CompuLab) does
  not work out of the box with older u-boot versions.

- Updating u-boot is something users might not want or miss to do.

However, I think nowadays it is ok to demand a recent u-boot in
combination with the upstream kernel. The cm-fx6 wouldn't be
the first board doing so. Hence, I propose these patches here.

Cheers,
Christopher

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2016-June/434562.html

[U-Boot] [PATCH 1/3] ARM: configs: cm_fx6: improve default environment

[Christopher Spinrath]

Currently, entire script segments have to be changed in the default
environment to change the kernel image location or to append kernel
cmdline parameters. In the later case this has to be changed for
every possible boot device.

Introduce new variables for kernel image locations and boot device
independent kernel parameters to make it easier to change these
settings.

Signed-off-by: Christopher Spinrath <christopher.spinrath@rwth-aachen.de>
---
 include/configs/cm_fx6.h | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/include/configs/cm_fx6.h b/include/configs/cm_fx6.h
index 1f20ec3..f054ca8 100644
--- a/include/configs/cm_fx6.h
+++ b/include/configs/cm_fx6.h
@@ -69,6 +69,8 @@
 	"stderr=serial,vga\0" \
 	"panel=HDMI\0" \
 	"autoload=no\0" \
+	"uImage=uImage-cm-fx6\0" \
+	"zImage=zImage-cm-fx6\0" \
 	"kernel=uImage-cm-fx6\0" \
 	"script=boot.scr\0" \
 	"dtb=cm-fx6.dtb\0" \
@@ -81,10 +83,10 @@
 	"video_dvi=mxcfb0:dev=dvi,1280x800M-32 at 50,if=RGB32\0" \
 	"doboot=bootm ${loadaddr}\0" \
 	"doloadfdt=false\0" \
-	"setboottypez=setenv kernel zImage-cm-fx6;" \
+	"setboottypez=setenv kernel ${zImage};" \
 		"setenv doboot bootz ${loadaddr} - ${fdtaddr};" \
 		"setenv doloadfdt true;\0" \
-	"setboottypem=setenv kernel uImage-cm-fx6;" \
+	"setboottypem=setenv kernel ${uImage};" \
 		"setenv doboot bootm ${loadaddr};" \
 		"setenv doloadfdt false;\0"\
 	"mmcroot=/dev/mmcblk0p2 rw rootwait\0" \
@@ -92,13 +94,13 @@
 	"nandroot=/dev/mtdblock4 rw\0" \
 	"nandrootfstype=ubifs\0" \
 	"mmcargs=setenv bootargs console=${console} root=${mmcroot} " \
-		"${video}\0" \
+		"${video} ${extrabootargs}\0" \
 	"sataargs=setenv bootargs console=${console} root=${sataroot} " \
-		"${video}\0" \
+		"${video} ${extrabootargs}\0" \
 	"nandargs=setenv bootargs console=${console} " \
 		"root=${nandroot} " \
 		"rootfstype=${nandrootfstype} " \
-		"${video}\0" \
+		"${video} ${extrabootargs}\0" \
 	"nandboot=if run nandloadkernel; then " \
 			"run nandloadfdt;" \
 			"run setboottypem;" \
-- 
2.8.3

[U-Boot] [PATCH 1/3] ARM: configs: cm_fx6: improve default environment

[Igor Grinberg]

On 06/19/2016 06:44 PM, Christopher Spinrath wrote:
> Currently, entire script segments have to be changed in the default
> environment to change the kernel image location or to append kernel
> cmdline parameters. In the later case this has to be changed for
> every possible boot device.
> 
> Introduce new variables for kernel image locations and boot device
> independent kernel parameters to make it easier to change these
> settings.
> 
> Signed-off-by: Christopher Spinrath <christopher.spinrath@rwth-aachen.de>

Reviewed-by: Igor Grinberg <grinberg@compulab.co.il>

-- 
Regards,
Igor.

[U-Boot] [PATCH 1/3] ARM: configs: cm_fx6: improve default environment

[Nikita Kiryanov]

On Sun, Jun 19, 2016 at 05:44:54PM +0200, Christopher Spinrath wrote:
> Currently, entire script segments have to be changed in the default
> environment to change the kernel image location or to append kernel
> cmdline parameters. In the later case this has to be changed for
> every possible boot device.
> 
> Introduce new variables for kernel image locations and boot device
> independent kernel parameters to make it easier to change these
> settings.

Reviewed-by: Nikita Kiryanov <nikita@compulab.co.il>

> 
> Signed-off-by: Christopher Spinrath <christopher.spinrath@rwth-aachen.de>
> ---
>  include/configs/cm_fx6.h | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 

[U-Boot] [PATCH 2/3] ARM: board: cm_fx6: fixup mtd partitions in the fdt

[Christopher Spinrath]

The cm-fx6 module has an on-board st,m25p compatible spi flash chip
used for u-boot (binary & environment). Overwrite the partitions in
the device tree by the partition table provided in the mtdparts
environment variable, if it is set.

This allows to specify a kernel independent partitioning in the
environment and provides a convient way for the user to adapt the
partition table.

Signed-off-by: Christopher Spinrath <christopher.spinrath@rwth-aachen.de>
---
 board/compulab/cm_fx6/cm_fx6.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/board/compulab/cm_fx6/cm_fx6.c b/board/compulab/cm_fx6/cm_fx6.c
index 712057a..81a7ae2 100644
--- a/board/compulab/cm_fx6/cm_fx6.c
+++ b/board/compulab/cm_fx6/cm_fx6.c
@@ -12,6 +12,7 @@
 #include <dm.h>
 #include <fsl_esdhc.h>
 #include <miiphy.h>
+#include <mtd_node.h>
 #include <netdev.h>
 #include <errno.h>
 #include <usb.h>
@@ -28,6 +29,7 @@
 #include <asm/io.h>
 #include <asm/gpio.h>
 #include <dm/platform_data/serial_mxc.h>
+#include <jffs2/load_kernel.h>
 #include "common.h"
 #include "../common/eeprom.h"
 #include "../common/common.h"
@@ -581,6 +583,13 @@ int cm_fx6_setup_ecspi(void) { return 0; }
 
 #ifdef CONFIG_OF_BOARD_SETUP
 #define USDHC3_PATH	"/soc/aips-bus at 02100000/usdhc at 02198000/"
+
+#ifdef CONFIG_FDT_FIXUP_PARTITIONS
+struct node_info nodes[] = {
+	{ "st,m25p",	MTD_DEV_TYPE_NOR,	},
+};
+#endif
+
 int ft_board_setup(void *blob, bd_t *bd)
 {
 	u32 baseboard_rev;
@@ -589,6 +598,8 @@ int ft_board_setup(void *blob, bd_t *bd)
 	char baseboard_name[16];
 	int err;
 
+	fdt_shrink_to_minimum(blob); /* Make room for new properties */
+
 	/* MAC addr */
 	if (eth_getenv_enetaddr("ethaddr", enetaddr)) {
 		fdt_find_and_setprop(blob,
@@ -607,7 +618,6 @@ int ft_board_setup(void *blob, bd_t *bd)
 		return 0; /* Assume not an early revision SB-FX6m baseboard */
 
 	if (!strncmp("SB-FX6m", baseboard_name, 7) && baseboard_rev <= 120) {
-		fdt_shrink_to_minimum(blob); /* Make room for new properties */
 		nodeoffset = fdt_path_offset(blob, USDHC3_PATH);
 		fdt_delprop(blob, nodeoffset, "cd-gpios");
 		fdt_find_and_setprop(blob, USDHC3_PATH, "broken-cd",
@@ -616,6 +626,10 @@ int ft_board_setup(void *blob, bd_t *bd)
 				     NULL, 0, 1);
 	}
 
+#ifdef CONFIG_FDT_FIXUP_PARTITIONS
+	fdt_fixup_mtdparts(blob, nodes, ARRAY_SIZE(nodes));
+#endif
+
 	return 0;
 }
 #endif
-- 
2.8.3

[U-Boot] [PATCH 2/3] ARM: board: cm_fx6: fixup mtd partitions in the fdt

[Igor Grinberg]

Hi Christopher,

On 06/19/2016 06:44 PM, Christopher Spinrath wrote:
> The cm-fx6 module has an on-board st,m25p compatible spi flash chip
> used for u-boot (binary & environment). Overwrite the partitions in
> the device tree by the partition table provided in the mtdparts
> environment variable, if it is set.
> 
> This allows to specify a kernel independent partitioning in the
> environment and provides a convient way for the user to adapt the
> partition table.
> 
> Signed-off-by: Christopher Spinrath <christopher.spinrath@rwth-aachen.de>
> ---
>  board/compulab/cm_fx6/cm_fx6.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/board/compulab/cm_fx6/cm_fx6.c b/board/compulab/cm_fx6/cm_fx6.c
> index 712057a..81a7ae2 100644
> --- a/board/compulab/cm_fx6/cm_fx6.c
> +++ b/board/compulab/cm_fx6/cm_fx6.c
> @@ -12,6 +12,7 @@
>  #include <dm.h>
>  #include <fsl_esdhc.h>
>  #include <miiphy.h>
> +#include <mtd_node.h>
>  #include <netdev.h>
>  #include <errno.h>
>  #include <usb.h>
> @@ -28,6 +29,7 @@
>  #include <asm/io.h>
>  #include <asm/gpio.h>
>  #include <dm/platform_data/serial_mxc.h>
> +#include <jffs2/load_kernel.h>

Why is this needed?

>  #include "common.h"
>  #include "../common/eeprom.h"
>  #include "../common/common.h"
> @@ -581,6 +583,13 @@ int cm_fx6_setup_ecspi(void) { return 0; }
>  
>  #ifdef CONFIG_OF_BOARD_SETUP
>  #define USDHC3_PATH	"/soc/aips-bus at 02100000/usdhc at 02198000/"
> +
> +#ifdef CONFIG_FDT_FIXUP_PARTITIONS
> +struct node_info nodes[] = {
> +	{ "st,m25p",	MTD_DEV_TYPE_NOR,	},
> +};
> +#endif
> +
>  int ft_board_setup(void *blob, bd_t *bd)
>  {
>  	u32 baseboard_rev;
> @@ -589,6 +598,8 @@ int ft_board_setup(void *blob, bd_t *bd)
>  	char baseboard_name[16];
>  	int err;
>  
> +	fdt_shrink_to_minimum(blob); /* Make room for new properties */
> +
>  	/* MAC addr */
>  	if (eth_getenv_enetaddr("ethaddr", enetaddr)) {
>  		fdt_find_and_setprop(blob,
> @@ -607,7 +618,6 @@ int ft_board_setup(void *blob, bd_t *bd)
>  		return 0; /* Assume not an early revision SB-FX6m baseboard */
>  
>  	if (!strncmp("SB-FX6m", baseboard_name, 7) && baseboard_rev <= 120) {
> -		fdt_shrink_to_minimum(blob); /* Make room for new properties */
>  		nodeoffset = fdt_path_offset(blob, USDHC3_PATH);
>  		fdt_delprop(blob, nodeoffset, "cd-gpios");
>  		fdt_find_and_setprop(blob, USDHC3_PATH, "broken-cd",
> @@ -616,6 +626,10 @@ int ft_board_setup(void *blob, bd_t *bd)
>  				     NULL, 0, 1);
>  	}
>  
> +#ifdef CONFIG_FDT_FIXUP_PARTITIONS
> +	fdt_fixup_mtdparts(blob, nodes, ARRAY_SIZE(nodes));
> +#endif

I really dislike the ifdeffery inside functions.
Care to introduce a stub for the !CONFIG_FDT_FIXUP_PARTITIONS case in
include/fdt_support.h for this one?

> +
>  	return 0;
>  }
>  #endif
> 

-- 
Regards,
Igor.

[U-Boot] [PATCH 2/3] ARM: board: cm_fx6: fixup mtd partitions in the fdt

[Igor Grinberg]

On 06/19/2016 06:44 PM, Christopher Spinrath wrote:
> The cm-fx6 module has an on-board st,m25p compatible spi flash chip
> used for u-boot (binary & environment). Overwrite the partitions in
> the device tree by the partition table provided in the mtdparts
> environment variable, if it is set.
> 
> This allows to specify a kernel independent partitioning in the
> environment and provides a convient way for the user to adapt the
> partition table.
> 
> Signed-off-by: Christopher Spinrath <christopher.spinrath@rwth-aachen.de>
> ---
>  board/compulab/cm_fx6/cm_fx6.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/board/compulab/cm_fx6/cm_fx6.c b/board/compulab/cm_fx6/cm_fx6.c
> index 712057a..81a7ae2 100644
> --- a/board/compulab/cm_fx6/cm_fx6.c
> +++ b/board/compulab/cm_fx6/cm_fx6.c

[...]

> +#ifdef CONFIG_FDT_FIXUP_PARTITIONS
> +struct node_info nodes[] = {
> +	{ "st,m25p",	MTD_DEV_TYPE_NOR,	},

Nikita, is this enough for all flashes we assemble on cm-fx6?

> +};
> +#endif
> +

[...]

-- 
Regards,
Igor.

[U-Boot] [PATCH 2/3] ARM: board: cm_fx6: fixup mtd partitions in the fdt

[Nikita Kiryanov]

On Wed, Jun 22, 2016 at 07:17:53PM +0300, Igor Grinberg wrote:
> On 06/19/2016 06:44 PM, Christopher Spinrath wrote:
> > The cm-fx6 module has an on-board st,m25p compatible spi flash chip
> > used for u-boot (binary & environment). Overwrite the partitions in
> > the device tree by the partition table provided in the mtdparts
> > environment variable, if it is set.
> > 
> > This allows to specify a kernel independent partitioning in the
> > environment and provides a convient way for the user to adapt the
> > partition table.
> > 
> > Signed-off-by: Christopher Spinrath <christopher.spinrath@rwth-aachen.de>
> > ---
> >  board/compulab/cm_fx6/cm_fx6.c | 16 +++++++++++++++-
> >  1 file changed, 15 insertions(+), 1 deletion(-)
> > 
> > diff --git a/board/compulab/cm_fx6/cm_fx6.c b/board/compulab/cm_fx6/cm_fx6.c
> > index 712057a..81a7ae2 100644
> > --- a/board/compulab/cm_fx6/cm_fx6.c
> > +++ b/board/compulab/cm_fx6/cm_fx6.c
> 
> [...]
> 
> > +#ifdef CONFIG_FDT_FIXUP_PARTITIONS
> > +struct node_info nodes[] = {
> > +	{ "st,m25p",	MTD_DEV_TYPE_NOR,	},
> 
> Nikita, is this enough for all flashes we assemble on cm-fx6?

Yes, CM-FX6 is using M25PX16 and SST25VF016B, both of which are
supported by the m25p80.c driver. However, on the mainline branch
I don't see "m25p" in the list of device ids, and IIRC the request
is to favor "jedec,spi-nor" as compatible string over device specific
ones.


> 
> > +};
> > +#endif
> > +
> 
> [...]
> 
> -- 
> Regards,
> Igor.
> 

[U-Boot] [PATCH 2/3] ARM: board: cm_fx6: fixup mtd partitions in the fdt

[Christopher Spinrath]

Hi Nikita,

On 07/07/2016 10:53 AM, Nikita Kiryanov wrote:
> On Wed, Jun 22, 2016 at 07:17:53PM +0300, Igor Grinberg wrote:
>> On 06/19/2016 06:44 PM, Christopher Spinrath wrote:
>>> The cm-fx6 module has an on-board st,m25p compatible spi flash chip
>>> used for u-boot (binary & environment). Overwrite the partitions in
>>> the device tree by the partition table provided in the mtdparts
>>> environment variable, if it is set.
>>>
>>> This allows to specify a kernel independent partitioning in the
>>> environment and provides a convient way for the user to adapt the
>>> partition table.
>>>
>>> Signed-off-by: Christopher Spinrath <christopher.spinrath@rwth-aachen.de>
>>> ---
>>>  board/compulab/cm_fx6/cm_fx6.c | 16 +++++++++++++++-
>>>  1 file changed, 15 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/board/compulab/cm_fx6/cm_fx6.c b/board/compulab/cm_fx6/cm_fx6.c
>>> index 712057a..81a7ae2 100644
>>> --- a/board/compulab/cm_fx6/cm_fx6.c
>>> +++ b/board/compulab/cm_fx6/cm_fx6.c
>>
>> [...]
>>
>>> +#ifdef CONFIG_FDT_FIXUP_PARTITIONS
>>> +struct node_info nodes[] = {
>>> +	{ "st,m25p",	MTD_DEV_TYPE_NOR,	},
>>
>> Nikita, is this enough for all flashes we assemble on cm-fx6?
> 
> Yes, CM-FX6 is using M25PX16 and SST25VF016B, both of which are
> supported by the m25p80.c driver. However, on the mainline branch
> I don't see "m25p" in the list of device ids, and IIRC the request
> is to favor "jedec,spi-nor" as compatible string over device specific
> ones.

Linux is going to use "st,m25p", "jedec,spi-nor" as compatible list
(currently queued for inclusion in v4.8:
https://git.kernel.org/cgit/linux/kernel/git/arm/arm-soc.git/tree/arch/arm/boot/dts/imx6q-cm-fx6.dts?h=next/dt#n123
).

I have chosen "st,m25p" here to cover both the mainline and CompuLab's
device trees (I have seen some where "jedec,spi-nor" is not in the
list). However, if you prefer I will switch to "jedec,spi-nor"
(excluding some device trees) in v2.

Thanks,
Christopher

> 
>>
>>> +};
>>> +#endif
>>> +
>>
>> [...]
>>
>> -- 
>> Regards,
>> Igor.
>>

[U-Boot] [PATCH 2/3] ARM: board: cm_fx6: fixup mtd partitions in the fdt

[Nikita Kiryanov]

Hi Christopher,

On Thu, Jul 07, 2016 at 03:30:25PM +0200, Christopher Spinrath wrote:
> Hi Nikita,
> 
> On 07/07/2016 10:53 AM, Nikita Kiryanov wrote:
> > On Wed, Jun 22, 2016 at 07:17:53PM +0300, Igor Grinberg wrote:
> >> On 06/19/2016 06:44 PM, Christopher Spinrath wrote:
> >>> The cm-fx6 module has an on-board st,m25p compatible spi flash chip
> >>> used for u-boot (binary & environment). Overwrite the partitions in
> >>> the device tree by the partition table provided in the mtdparts
> >>> environment variable, if it is set.
> >>>
> >>> This allows to specify a kernel independent partitioning in the
> >>> environment and provides a convient way for the user to adapt the
> >>> partition table.
> >>>
> >>> Signed-off-by: Christopher Spinrath <christopher.spinrath@rwth-aachen.de>
> >>> ---
> >>>  board/compulab/cm_fx6/cm_fx6.c | 16 +++++++++++++++-
> >>>  1 file changed, 15 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/board/compulab/cm_fx6/cm_fx6.c b/board/compulab/cm_fx6/cm_fx6.c
> >>> index 712057a..81a7ae2 100644
> >>> --- a/board/compulab/cm_fx6/cm_fx6.c
> >>> +++ b/board/compulab/cm_fx6/cm_fx6.c
> >>
> >> [...]
> >>
> >>> +#ifdef CONFIG_FDT_FIXUP_PARTITIONS
> >>> +struct node_info nodes[] = {
> >>> +	{ "st,m25p",	MTD_DEV_TYPE_NOR,	},
> >>
> >> Nikita, is this enough for all flashes we assemble on cm-fx6?
> > 
> > Yes, CM-FX6 is using M25PX16 and SST25VF016B, both of which are
> > supported by the m25p80.c driver. However, on the mainline branch
> > I don't see "m25p" in the list of device ids, and IIRC the request
> > is to favor "jedec,spi-nor" as compatible string over device specific
> > ones.
> 
> Linux is going to use "st,m25p", "jedec,spi-nor" as compatible list
> (currently queued for inclusion in v4.8:
> https://git.kernel.org/cgit/linux/kernel/git/arm/arm-soc.git/tree/arch/arm/boot/dts/imx6q-cm-fx6.dts?h=next/dt#n123
> ).
> 
> I have chosen "st,m25p" here to cover both the mainline and CompuLab's
> device trees (I have seen some where "jedec,spi-nor" is not in the
> list). However, if you prefer I will switch to "jedec,spi-nor"
> (excluding some device trees) in v2.

Does it have to be an "or" situation? m25p is necessary to serve older CM-FX6
kernels, but it is not supported in the mainline kernel, so the correct
course of actions seems to be to use both "st,m25p" and "jedec,spi-nor".

[U-Boot] [PATCH 2/3] ARM: board: cm_fx6: fixup mtd partitions in the fdt

[Christopher Spinrath]

Hi Nikita,

On 10.07.2016 09:52, Nikita Kiryanov wrote:
> Hi Christopher,
>
> On Thu, Jul 07, 2016 at 03:30:25PM +0200, Christopher Spinrath wrote:
>> Hi Nikita,
>>
>> On 07/07/2016 10:53 AM, Nikita Kiryanov wrote:
>>> On Wed, Jun 22, 2016 at 07:17:53PM +0300, Igor Grinberg wrote:
>>>> On 06/19/2016 06:44 PM, Christopher Spinrath wrote:
>>>>> The cm-fx6 module has an on-board st,m25p compatible spi flash chip
>>>>> used for u-boot (binary & environment). Overwrite the partitions in
>>>>> the device tree by the partition table provided in the mtdparts
>>>>> environment variable, if it is set.
>>>>>
>>>>> This allows to specify a kernel independent partitioning in the
>>>>> environment and provides a convient way for the user to adapt the
>>>>> partition table.
>>>>>
>>>>> Signed-off-by: Christopher Spinrath <christopher.spinrath@rwth-aachen.de>
>>>>> ---
>>>>>   board/compulab/cm_fx6/cm_fx6.c | 16 +++++++++++++++-
>>>>>   1 file changed, 15 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/board/compulab/cm_fx6/cm_fx6.c b/board/compulab/cm_fx6/cm_fx6.c
>>>>> index 712057a..81a7ae2 100644
>>>>> --- a/board/compulab/cm_fx6/cm_fx6.c
>>>>> +++ b/board/compulab/cm_fx6/cm_fx6.c
>>>>
>>>> [...]
>>>>
>>>>> +#ifdef CONFIG_FDT_FIXUP_PARTITIONS
>>>>> +struct node_info nodes[] = {
>>>>> +	{ "st,m25p",	MTD_DEV_TYPE_NOR,	},
>>>>
>>>> Nikita, is this enough for all flashes we assemble on cm-fx6?
>>>
>>> Yes, CM-FX6 is using M25PX16 and SST25VF016B, both of which are
>>> supported by the m25p80.c driver. However, on the mainline branch
>>> I don't see "m25p" in the list of device ids, and IIRC the request
>>> is to favor "jedec,spi-nor" as compatible string over device specific
>>> ones.
>>
>> Linux is going to use "st,m25p", "jedec,spi-nor" as compatible list
>> (currently queued for inclusion in v4.8:
>> https://git.kernel.org/cgit/linux/kernel/git/arm/arm-soc.git/tree/arch/arm/boot/dts/imx6q-cm-fx6.dts?h=next/dt#n123
>> ).
>>
>> I have chosen "st,m25p" here to cover both the mainline and CompuLab's
>> device trees (I have seen some where "jedec,spi-nor" is not in the
>> list). However, if you prefer I will switch to "jedec,spi-nor"
>> (excluding some device trees) in v2.
>
> Does it have to be an "or" situation? m25p is necessary to serve older CM-FX6
> kernels, but it is not supported in the mainline kernel, so the correct
> course of actions seems to be to use both "st,m25p" and "jedec,spi-nor".

This has nothing to do with the kernel itself; the compatible is only 
used to find the correct dt node describing the flash chip and as I said 
in my previous email, the upstream device tree node is going to have the 
st,m25p compatible string.

On the other hand, m25p is not documented but it used to be in upstream 
device trees. Since it has been removed from most of them recently, I 
agree that having both compatibles is a good idea. From a quick look at 
the source code it should be possible. I will verify this for v2.

Thanks,
Christopher

[U-Boot] [PATCH 2/3] ARM: board: cm_fx6: fixup mtd partitions in the fdt

[Christopher Spinrath]

Hi Igor,

On 06/22/2016 06:02 PM, Igor Grinberg wrote:
> Hi Christopher,
> 
> On 06/19/2016 06:44 PM, Christopher Spinrath wrote:
>> The cm-fx6 module has an on-board st,m25p compatible spi flash chip
>> used for u-boot (binary & environment). Overwrite the partitions in
>> the device tree by the partition table provided in the mtdparts
>> environment variable, if it is set.
>>
>> This allows to specify a kernel independent partitioning in the
>> environment and provides a convient way for the user to adapt the
>> partition table.
>>
>> Signed-off-by: Christopher Spinrath <christopher.spinrath@rwth-aachen.de>
>> ---
>>  board/compulab/cm_fx6/cm_fx6.c | 16 +++++++++++++++-
>>  1 file changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/board/compulab/cm_fx6/cm_fx6.c b/board/compulab/cm_fx6/cm_fx6.c
>> index 712057a..81a7ae2 100644
>> --- a/board/compulab/cm_fx6/cm_fx6.c
>> +++ b/board/compulab/cm_fx6/cm_fx6.c
>> @@ -12,6 +12,7 @@
>>  #include <dm.h>
>>  #include <fsl_esdhc.h>
>>  #include <miiphy.h>
>> +#include <mtd_node.h>
>>  #include <netdev.h>
>>  #include <errno.h>
>>  #include <usb.h>
>> @@ -28,6 +29,7 @@
>>  #include <asm/io.h>
>>  #include <asm/gpio.h>
>>  #include <dm/platform_data/serial_mxc.h>
>> +#include <jffs2/load_kernel.h>
> 
> Why is this needed?
> 
The MTD_DEV_TYPE_NOR constant is defined in this header. I agree that it
is a bit ugly but this header is used for the same purpose in other
board files, too (e.g.board/pdm360ng/pdm360ng.c).

>>  #include "common.h"
>>  #include "../common/eeprom.h"
>>  #include "../common/common.h"
>> @@ -581,6 +583,13 @@ int cm_fx6_setup_ecspi(void) { return 0; }
>>  
>>  #ifdef CONFIG_OF_BOARD_SETUP
>>  #define USDHC3_PATH	"/soc/aips-bus at 02100000/usdhc at 02198000/"
>> +
>> +#ifdef CONFIG_FDT_FIXUP_PARTITIONS
>> +struct node_info nodes[] = {
>> +	{ "st,m25p",	MTD_DEV_TYPE_NOR,	},
>> +};
>> +#endif
>> +
>>  int ft_board_setup(void *blob, bd_t *bd)
>>  {
>>  	u32 baseboard_rev;
>> @@ -589,6 +598,8 @@ int ft_board_setup(void *blob, bd_t *bd)
>>  	char baseboard_name[16];
>>  	int err;
>>  
>> +	fdt_shrink_to_minimum(blob); /* Make room for new properties */
>> +
>>  	/* MAC addr */
>>  	if (eth_getenv_enetaddr("ethaddr", enetaddr)) {
>>  		fdt_find_and_setprop(blob,
>> @@ -607,7 +618,6 @@ int ft_board_setup(void *blob, bd_t *bd)
>>  		return 0; /* Assume not an early revision SB-FX6m baseboard */
>>  
>>  	if (!strncmp("SB-FX6m", baseboard_name, 7) && baseboard_rev <= 120) {
>> -		fdt_shrink_to_minimum(blob); /* Make room for new properties */
>>  		nodeoffset = fdt_path_offset(blob, USDHC3_PATH);
>>  		fdt_delprop(blob, nodeoffset, "cd-gpios");
>>  		fdt_find_and_setprop(blob, USDHC3_PATH, "broken-cd",
>> @@ -616,6 +626,10 @@ int ft_board_setup(void *blob, bd_t *bd)
>>  				     NULL, 0, 1);
>>  	}
>>  
>> +#ifdef CONFIG_FDT_FIXUP_PARTITIONS
>> +	fdt_fixup_mtdparts(blob, nodes, ARRAY_SIZE(nodes));
>> +#endif
> 
> I really dislike the ifdeffery inside functions.
> Care to introduce a stub for the !CONFIG_FDT_FIXUP_PARTITIONS case in
> include/fdt_support.h for this one?
> 
Sure. Is the header the correct place for this or should I add a #else
case in the .c file?

Cheers,
Christopher

>> +
>>  	return 0;
>>  }
>>  #endif
>>
> 

[U-Boot] [PATCH 2/3] ARM: board: cm_fx6: fixup mtd partitions in the fdt

[Igor Grinberg]

Hi Christopher,

On 06/22/2016 10:21 PM, Christopher Spinrath wrote:
> Hi Igor,
> 
> On 06/22/2016 06:02 PM, Igor Grinberg wrote:
>> Hi Christopher,
>>
>> On 06/19/2016 06:44 PM, Christopher Spinrath wrote:
>>> The cm-fx6 module has an on-board st,m25p compatible spi flash chip
>>> used for u-boot (binary & environment). Overwrite the partitions in
>>> the device tree by the partition table provided in the mtdparts
>>> environment variable, if it is set.
>>>
>>> This allows to specify a kernel independent partitioning in the
>>> environment and provides a convient way for the user to adapt the
>>> partition table.
>>>
>>> Signed-off-by: Christopher Spinrath <christopher.spinrath@rwth-aachen.de>
>>> ---
>>>  board/compulab/cm_fx6/cm_fx6.c | 16 +++++++++++++++-
>>>  1 file changed, 15 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/board/compulab/cm_fx6/cm_fx6.c b/board/compulab/cm_fx6/cm_fx6.c
>>> index 712057a..81a7ae2 100644
>>> --- a/board/compulab/cm_fx6/cm_fx6.c
>>> +++ b/board/compulab/cm_fx6/cm_fx6.c

[...]

>>> @@ -28,6 +29,7 @@
>>>  #include <asm/io.h>
>>>  #include <asm/gpio.h>
>>>  #include <dm/platform_data/serial_mxc.h>
>>> +#include <jffs2/load_kernel.h>
>>
>> Why is this needed?
>>
> The MTD_DEV_TYPE_NOR constant is defined in this header. I agree that it
> is a bit ugly but this header is used for the same purpose in other
> board files, too (e.g.board/pdm360ng/pdm360ng.c).

I see.
I don't feel right to request this in scope of these patches, but
if you can take care of that one (even in a follow up patch) - it would
be awesome.

[...]

>>> @@ -616,6 +626,10 @@ int ft_board_setup(void *blob, bd_t *bd)
>>>  				     NULL, 0, 1);
>>>  	}
>>>  
>>> +#ifdef CONFIG_FDT_FIXUP_PARTITIONS
>>> +	fdt_fixup_mtdparts(blob, nodes, ARRAY_SIZE(nodes));
>>> +#endif
>>
>> I really dislike the ifdeffery inside functions.
>> Care to introduce a stub for the !CONFIG_FDT_FIXUP_PARTITIONS case in
>> include/fdt_support.h for this one?
>>
> Sure. Is the header the correct place for this or should I add a #else
> case in the .c file?

IMO, the header file is better for stubbing things out.
It does not require you to add .c into compilation.
There are also already several examples inside this header.

-- 
Regards,
Igor.

[U-Boot] [PATCH 2/3] ARM: board: cm_fx6: fixup mtd partitions in the fdt

[Christopher Spinrath]

Hi Igor,

On 06/23/2016 10:56 AM, Igor Grinberg wrote:
> Hi Christopher,
>
> On 06/22/2016 10:21 PM, Christopher Spinrath wrote:
>> Hi Igor,
>>
>> On 06/22/2016 06:02 PM, Igor Grinberg wrote:
>>> Hi Christopher,
>>>
>>> On 06/19/2016 06:44 PM, Christopher Spinrath wrote:
>>>> The cm-fx6 module has an on-board st,m25p compatible spi flash chip
>>>> used for u-boot (binary & environment). Overwrite the partitions in
>>>> the device tree by the partition table provided in the mtdparts
>>>> environment variable, if it is set.
>>>>
>>>> This allows to specify a kernel independent partitioning in the
>>>> environment and provides a convient way for the user to adapt the
>>>> partition table.
>>>>
>>>> Signed-off-by: Christopher Spinrath <christopher.spinrath@rwth-aachen.de>
>>>> ---
>>>>  board/compulab/cm_fx6/cm_fx6.c | 16 +++++++++++++++-
>>>>  1 file changed, 15 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/board/compulab/cm_fx6/cm_fx6.c b/board/compulab/cm_fx6/cm_fx6.c
>>>> index 712057a..81a7ae2 100644
>>>> --- a/board/compulab/cm_fx6/cm_fx6.c
>>>> +++ b/board/compulab/cm_fx6/cm_fx6.c
>
> [...]
>
>>>> @@ -28,6 +29,7 @@
>>>>  #include <asm/io.h>
>>>>  #include <asm/gpio.h>
>>>>  #include <dm/platform_data/serial_mxc.h>
>>>> +#include <jffs2/load_kernel.h>
>>>
>>> Why is this needed?
>>>
>> The MTD_DEV_TYPE_NOR constant is defined in this header. I agree that it
>> is a bit ugly but this header is used for the same purpose in other
>> board files, too (e.g.board/pdm360ng/pdm360ng.c).
>
> I see.
> I don't feel right to request this in scope of these patches, but
> if you can take care of that one (even in a follow up patch) - it would
> be awesome.
>

I prefer to defer it, since it touches another subsystem (jffs).

> [...]
>
>>>> @@ -616,6 +626,10 @@ int ft_board_setup(void *blob, bd_t *bd)
>>>>  				     NULL, 0, 1);
>>>>  	}
>>>>
>>>> +#ifdef CONFIG_FDT_FIXUP_PARTITIONS
>>>> +	fdt_fixup_mtdparts(blob, nodes, ARRAY_SIZE(nodes));
>>>> +#endif
>>>
>>> I really dislike the ifdeffery inside functions.
>>> Care to introduce a stub for the !CONFIG_FDT_FIXUP_PARTITIONS case in
>>> include/fdt_support.h for this one?
>>>
>> Sure. Is the header the correct place for this or should I add a #else
>> case in the .c file?
>
> IMO, the header file is better for stubbing things out.
> It does not require you to add .c into compilation.
> There are also already several examples inside this header.
>
Ok.

Thanks,
Christopher

[U-Boot] [PATCH 3/3] ARM: configs: cm_fx6: add mtd support

[Christopher Spinrath]

The cm-fx6 module has an on-board spi flash chip. Enable mtd support
and the mtdparts command. Also define a default partitioning, add
it to the default environment, and enable support to overwrite the
partitioning defined in a device tree by it.

These changes move the effective default partitioning from the device
tree shipped with the vendor kernels to u-boot which becomes the single
point of definition for the partitioning for all device tree based
kernels (in particular, for the upstream linux kernel which does not
have a default partitioning defined in its device tree).

Signed-off-by: Christopher Spinrath <christopher.spinrath@rwth-aachen.de>
---
 include/configs/cm_fx6.h | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/include/configs/cm_fx6.h b/include/configs/cm_fx6.h
index f054ca8..c839b03 100644
--- a/include/configs/cm_fx6.h
+++ b/include/configs/cm_fx6.h
@@ -18,6 +18,7 @@
 #define CONFIG_MACH_TYPE		4273
 
 /* CMD */
+#define CONFIG_CMD_MTDPARTS
 
 /* MMC */
 #define CONFIG_SYS_FSL_USDHC_NUM	3
@@ -53,6 +54,20 @@
 #define CONFIG_SF_DEFAULT_SPEED		25000000
 #define CONFIG_SF_DEFAULT_MODE		(SPI_MODE_0)
 
+/* MTD support */
+#ifndef CONFIG_SPL_BUILD
+#define CONFIG_FDT_FIXUP_PARTITIONS
+#define CONFIG_MTD_DEVICE
+#define CONFIG_MTD_PARTITIONS
+#define CONFIG_SPI_FLASH_MTD
+#endif
+
+#define MTDIDS_DEFAULT		"nor0=spi0.0"
+#define MTDPARTS_DEFAULT	"mtdparts=spi0.0:" \
+				"768k(uboot)," \
+				"256k(uboot-environment)," \
+				"-(reserved)"
+
 /* Environment */
 #define CONFIG_ENV_IS_IN_SPI_FLASH
 #define CONFIG_ENV_SPI_MAX_HZ		CONFIG_SF_DEFAULT_SPEED
@@ -83,6 +98,8 @@
 	"video_dvi=mxcfb0:dev=dvi,1280x800M-32 at 50,if=RGB32\0" \
 	"doboot=bootm ${loadaddr}\0" \
 	"doloadfdt=false\0" \
+	"mtdids=" MTDIDS_DEFAULT "\0" \
+	"mtdparts=" MTDPARTS_DEFAULT "\0" \
 	"setboottypez=setenv kernel ${zImage};" \
 		"setenv doboot bootz ${loadaddr} - ${fdtaddr};" \
 		"setenv doloadfdt true;\0" \
@@ -157,7 +174,7 @@
 	"run setupnandboot;" \
 	"run nandboot;"
 
-#define CONFIG_PREBOOT		"usb start"
+#define CONFIG_PREBOOT		"usb start;sf probe"
 
 /* SPI */
 #define CONFIG_SPI
-- 
2.8.3

[U-Boot] [PATCH 3/3] ARM: configs: cm_fx6: add mtd support

[Igor Grinberg]

On 06/19/2016 06:44 PM, Christopher Spinrath wrote:
> The cm-fx6 module has an on-board spi flash chip. Enable mtd support
> and the mtdparts command. Also define a default partitioning, add
> it to the default environment, and enable support to overwrite the
> partitioning defined in a device tree by it.
> 
> These changes move the effective default partitioning from the device
> tree shipped with the vendor kernels to u-boot which becomes the single
> point of definition for the partitioning for all device tree based
> kernels (in particular, for the upstream linux kernel which does not
> have a default partitioning defined in its device tree).
> 
> Signed-off-by: Christopher Spinrath <christopher.spinrath@rwth-aachen.de>
> ---
>  include/configs/cm_fx6.h | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/include/configs/cm_fx6.h b/include/configs/cm_fx6.h
> index f054ca8..c839b03 100644
> --- a/include/configs/cm_fx6.h
> +++ b/include/configs/cm_fx6.h

[...]

> @@ -157,7 +174,7 @@
>  	"run setupnandboot;" \
>  	"run nandboot;"
>  
> -#define CONFIG_PREBOOT		"usb start"
> +#define CONFIG_PREBOOT		"usb start;sf probe"

Probably, this is really needed.
Care to explain?

>  
>  /* SPI */
>  #define CONFIG_SPI
> 

-- 
Regards,
Igor.

[U-Boot] [PATCH 3/3] ARM: configs: cm_fx6: add mtd support

[Christopher Spinrath]

Hi Igor,

On 06/22/2016 06:15 PM, Igor Grinberg wrote:
> On 06/19/2016 06:44 PM, Christopher Spinrath wrote:
>> The cm-fx6 module has an on-board spi flash chip. Enable mtd support
>> and the mtdparts command. Also define a default partitioning, add
>> it to the default environment, and enable support to overwrite the
>> partitioning defined in a device tree by it.
>>
>> These changes move the effective default partitioning from the device
>> tree shipped with the vendor kernels to u-boot which becomes the single
>> point of definition for the partitioning for all device tree based
>> kernels (in particular, for the upstream linux kernel which does not
>> have a default partitioning defined in its device tree).
>>
>> Signed-off-by: Christopher Spinrath <christopher.spinrath@rwth-aachen.de>
>> ---
>>  include/configs/cm_fx6.h | 19 ++++++++++++++++++-
>>  1 file changed, 18 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/configs/cm_fx6.h b/include/configs/cm_fx6.h
>> index f054ca8..c839b03 100644
>> --- a/include/configs/cm_fx6.h
>> +++ b/include/configs/cm_fx6.h
> 
> [...]
> 
>> @@ -157,7 +174,7 @@
>>  	"run setupnandboot;" \
>>  	"run nandboot;"
>>  
>> -#define CONFIG_PREBOOT		"usb start"
>> +#define CONFIG_PREBOOT		"usb start;sf probe"
> 
> Probably, this is really needed.
> Care to explain?
>
The sf probe command probes for the spi flash and registers (on success)
the device as nor0. This device is used by mtdparts (cf. the mtdids
variable; it maps the U-Boot name nor0 to the kernel name spi0.0) and
the mtd fixup code in patch 2 (cf. the nodes array; it specifies the
compatible of the flash chip of type NOR #0, i.e. nor0).

Without this all mtdparts commands will fail and the fixup code won't
work because there is nor0 device.

Cheers,
Christopher

>>  
>>  /* SPI */
>>  #define CONFIG_SPI
>>
> 

[U-Boot] [PATCH 3/3] ARM: configs: cm_fx6: add mtd support

[Igor Grinberg]

On 06/22/2016 10:27 PM, Christopher Spinrath wrote:
> Hi Igor,
> 
> On 06/22/2016 06:15 PM, Igor Grinberg wrote:
>> On 06/19/2016 06:44 PM, Christopher Spinrath wrote:
>>> The cm-fx6 module has an on-board spi flash chip. Enable mtd support
>>> and the mtdparts command. Also define a default partitioning, add
>>> it to the default environment, and enable support to overwrite the
>>> partitioning defined in a device tree by it.
>>>
>>> These changes move the effective default partitioning from the device
>>> tree shipped with the vendor kernels to u-boot which becomes the single
>>> point of definition for the partitioning for all device tree based
>>> kernels (in particular, for the upstream linux kernel which does not
>>> have a default partitioning defined in its device tree).
>>>
>>> Signed-off-by: Christopher Spinrath <christopher.spinrath@rwth-aachen.de>
>>> ---
>>>  include/configs/cm_fx6.h | 19 ++++++++++++++++++-
>>>  1 file changed, 18 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/include/configs/cm_fx6.h b/include/configs/cm_fx6.h
>>> index f054ca8..c839b03 100644
>>> --- a/include/configs/cm_fx6.h
>>> +++ b/include/configs/cm_fx6.h
>>
>> [...]
>>
>>> @@ -157,7 +174,7 @@
>>>  	"run setupnandboot;" \
>>>  	"run nandboot;"
>>>  
>>> -#define CONFIG_PREBOOT		"usb start"
>>> +#define CONFIG_PREBOOT		"usb start;sf probe"
>>
>> Probably, this is really needed.
>> Care to explain?
>>
> The sf probe command probes for the spi flash and registers (on success)
> the device as nor0. This device is used by mtdparts (cf. the mtdids
> variable; it maps the U-Boot name nor0 to the kernel name spi0.0) and
> the mtd fixup code in patch 2 (cf. the nodes array; it specifies the
> compatible of the flash chip of type NOR #0, i.e. nor0).
> 
> Without this all mtdparts commands will fail and the fixup code won't
> work because there is nor0 device.

Thanks for the explanation!
That sounds to me like this should go away once we have the DM in place
for spi flash and MTD (added Simon to Cc).
Meanwhile, may be a short notice in the commit message?

-- 
Regards,
Igor.

[U-Boot] [PATCH 3/3] ARM: configs: cm_fx6: add mtd support

[Christopher Spinrath]

Hi Igor,

On 06/23/2016 11:03 AM, Igor Grinberg wrote:
> On 06/22/2016 10:27 PM, Christopher Spinrath wrote:
>> Hi Igor,
>>
>> On 06/22/2016 06:15 PM, Igor Grinberg wrote:
>>> On 06/19/2016 06:44 PM, Christopher Spinrath wrote:
>>>> The cm-fx6 module has an on-board spi flash chip. Enable mtd support
>>>> and the mtdparts command. Also define a default partitioning, add
>>>> it to the default environment, and enable support to overwrite the
>>>> partitioning defined in a device tree by it.
>>>>
>>>> These changes move the effective default partitioning from the device
>>>> tree shipped with the vendor kernels to u-boot which becomes the single
>>>> point of definition for the partitioning for all device tree based
>>>> kernels (in particular, for the upstream linux kernel which does not
>>>> have a default partitioning defined in its device tree).
>>>>
>>>> Signed-off-by: Christopher Spinrath <christopher.spinrath@rwth-aachen.de>
>>>> ---
>>>>  include/configs/cm_fx6.h | 19 ++++++++++++++++++-
>>>>  1 file changed, 18 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/include/configs/cm_fx6.h b/include/configs/cm_fx6.h
>>>> index f054ca8..c839b03 100644
>>>> --- a/include/configs/cm_fx6.h
>>>> +++ b/include/configs/cm_fx6.h
>>>
>>> [...]
>>>
>>>> @@ -157,7 +174,7 @@
>>>>  	"run setupnandboot;" \
>>>>  	"run nandboot;"
>>>>
>>>> -#define CONFIG_PREBOOT		"usb start"
>>>> +#define CONFIG_PREBOOT		"usb start;sf probe"
>>>
>>> Probably, this is really needed.
>>> Care to explain?
>>>
>> The sf probe command probes for the spi flash and registers (on success)
>> the device as nor0. This device is used by mtdparts (cf. the mtdids
>> variable; it maps the U-Boot name nor0 to the kernel name spi0.0) and
>> the mtd fixup code in patch 2 (cf. the nodes array; it specifies the
>> compatible of the flash chip of type NOR #0, i.e. nor0).
>>
>> Without this all mtdparts commands will fail and the fixup code won't
>> work because there is nor0 device.
>
> Thanks for the explanation!
> That sounds to me like this should go away once we have the DM in place
> for spi flash and MTD (added Simon to Cc).
> Meanwhile, may be a short notice in the commit message?
>

Sure, I will add it.

Thanks,
Christopher

[U-Boot] [PATCH 0/3] ARM: imx: enhance support for the cm-fx6 module

[Igor Grinberg]

Hi Christopher,

Thanks for doing this work.

On 06/19/2016 06:44 PM, Christopher Spinrath wrote:
> Hi,
> 
> this series aims at enhancing support for the cm-fx6 module. In
> particular, with respect to the upstream linux kernel.
> 
> The first patch improves the default environment. It is non-functional
> but makes it more convenient to adapt certain settings.
> 
> The later two patches add mtd partition support for the on-board spi
> flash chip. They pick up the discussion about specifying a default
> partitioning in the device tree from here [1]. In short: adding the
> default partitioning to the device tree was rejected by the linux/
> device tree community during mainlining large parts of the device tree.
> It was proposed to implement the partition/mtd handling in u-boot.
> On the other hand, it was argued that the flash chip becomes some
> kind of "black-box" since there will be no partition labeling (in
> particular, with old u-boot versions).
> 
> IMHO defining the mtd partitioning has the following (dis-)advantages:

You mean defining it in U-Boot instead of upstream DT.

> 
> Advantages:
> - It is easier for the user to change the partitioning (e.g. to use
>   the unsued 1MB free space).

I know it says reserved, but that is not exactly unused...
It is intended to hold a splash screen of up to 1MB size and may be other
things (like dtb, etc.).
By the time the partitioning layout was defined, it was still unclear
what requirements of that additional space will be.
So it was called reserved to provide a kind of warning to the users as
it might be used at some point.

> 
> - The flash ship is used entirely for u-boot.

Close, but not precise...
The spi flash chip is used for all boot purposes, so it might be beyond
U-Boot.

>   So it is quite natural
>   that u-boot manages it. Also, moving the partition table to it
>   allows us to change the layout in future versions of u-boot (almost
>   independently of the kernel - there are still non-device tree kernels).

Please don't change the layout as it will break backwards compatibility.
Also, there is not much you can change.

> 
> - U-Boot becomes the single point of definition for all device tree
>   kernels. Otherwise, each kernel (vendor vs. upstream + version)
>   would ship its own partitioning.

True.

>   Moreover, u-boot has to know
>   something about the partitioning, too, because it has to know where
>   the environment is saved.

It does, although the env address is hardcoded, but it should not be
changed.
The reason for providing a partition for it is purely for Linux to able
to change the environment variables.

> 
> Disadvantages:
> - Users of the upstream linux kernel have to use a recent u-boot
>   version to avoid the "black box" effect. A concrete impact is
>   that the update routine (described/proposed by CompuLab) does
>   not work out of the box with older u-boot versions.

True.

> 
> - Updating u-boot is something users might not want or miss to do.
> 
> However, I think nowadays it is ok to demand a recent u-boot in
> combination with the upstream kernel. The cm-fx6 wouldn't be
> the first board doing so. Hence, I propose these patches here.
> 
> Cheers,
> Christopher
> 
> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2016-June/434562.html
> 
> 

-- 
Regards,
Igor.

[U-Boot] [PATCH 0/3] ARM: imx: enhance support for the cm-fx6 module

[Christopher Spinrath]

Hi Igor,

thanks for your review!

On 06/22/2016 05:46 PM, Igor Grinberg wrote:
> Hi Christopher,
> 
> Thanks for doing this work.
> 
> On 06/19/2016 06:44 PM, Christopher Spinrath wrote:
>> Hi,
>>
>> this series aims at enhancing support for the cm-fx6 module. In
>> particular, with respect to the upstream linux kernel.
>>
>> The first patch improves the default environment. It is non-functional
>> but makes it more convenient to adapt certain settings.
>>
>> The later two patches add mtd partition support for the on-board spi
>> flash chip. They pick up the discussion about specifying a default
>> partitioning in the device tree from here [1]. In short: adding the
>> default partitioning to the device tree was rejected by the linux/
>> device tree community during mainlining large parts of the device tree.
>> It was proposed to implement the partition/mtd handling in u-boot.
>> On the other hand, it was argued that the flash chip becomes some
>> kind of "black-box" since there will be no partition labeling (in
>> particular, with old u-boot versions).
>>
>> IMHO defining the mtd partitioning has the following (dis-)advantages:
> 
> You mean defining it in U-Boot instead of upstream DT.
>

Yes.

>>
>> Advantages:
>> - It is easier for the user to change the partitioning (e.g. to use
>>   the unsued 1MB free space).
> 
> I know it says reserved, but that is not exactly unused...
> It is intended to hold a splash screen of up to 1MB size and may be other
> things (like dtb, etc.).
> By the time the partitioning layout was defined, it was still unclear
> what requirements of that additional space will be.
> So it was called reserved to provide a kind of warning to the users as
> it might be used at some point.
> 
Good to know. So this would be a use case for this series. People can
rename the last partition to e.g. uboot-splash.

>>
>> - The flash ship is used entirely for u-boot.
> 
> Close, but not precise...
> The spi flash chip is used for all boot purposes, so it might be beyond
> U-Boot.
> 
Ok. But still it is U-Boot (or another bootloader) that relies on the
memory layout.

>>   So it is quite natural
>>   that u-boot manages it. Also, moving the partition table to it
>>   allows us to change the layout in future versions of u-boot (almost
>>   independently of the kernel - there are still non-device tree kernels).
> 
> Please don't change the layout as it will break backwards compatibility.
> Also, there is not much you can change.
> 
I have no intention do so but users may want to change it (and now, can
do so easily). For example, choose a better name for the "reserved"
partition.

>>
>> - U-Boot becomes the single point of definition for all device tree
>>   kernels. Otherwise, each kernel (vendor vs. upstream + version)
>>   would ship its own partitioning.
> 
> True.
> 
>>   Moreover, u-boot has to know
>>   something about the partitioning, too, because it has to know where
>>   the environment is saved.
> 
> It does, although the env address is hardcoded, but it should not be
> changed.
> The reason for providing a partition for it is purely for Linux to able
> to change the environment variables.
> 
Indeed, but moving both definitions into one project reduces the risk of
having conflicts and allows (advanced) users to change it by adapting
only one project (e.g. another bootloader may use another partitioning).

Cheers,
Christopher

>>
>> Disadvantages:
>> - Users of the upstream linux kernel have to use a recent u-boot
>>   version to avoid the "black box" effect. A concrete impact is
>>   that the update routine (described/proposed by CompuLab) does
>>   not work out of the box with older u-boot versions.
> 
> True.
> 
>>
>> - Updating u-boot is something users might not want or miss to do.
>>
>> However, I think nowadays it is ok to demand a recent u-boot in
>> combination with the upstream kernel. The cm-fx6 wouldn't be
>> the first board doing so. Hence, I propose these patches here.
>>
>> Cheers,
>> Christopher
>>
>> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2016-June/434562.html
>>
>>
>