[U-Boot] [PATCH 1/3] spl: mmc: fix switch statement

[U-Boot] [PATCH 1/3] spl: mmc: fix switch statement

[Max Krummenacher]

If CONFIG_SPL_LIBCOMMON_SUPPORT is not defined there is a lone case statement
at the end of the switch leading to a compile error.
Remove the offending case statement.

| common/spl/spl_mmc.c:339:7: error: label at end of compound statement

Signed-off-by: Max Krummenacher <max.krummenacher@toradex.com>
---

 common/spl/spl_mmc.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c
index c674e61..367b4e4 100644
--- a/common/spl/spl_mmc.c
+++ b/common/spl/spl_mmc.c
@@ -342,7 +342,6 @@ static int spl_mmc_load_image(struct spl_image_info *spl_image,
 			return err;
 
 		break;
-	case MMCSD_MODE_UNDEFINED:
 #ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
 	default:
 		puts("spl: mmc: wrong boot mode\n");
-- 
2.6.6

[U-Boot] [PATCH 2/3] imx: make ipu's di configurable

[Max Krummenacher]

The ipu has two display interfaces. Make the used one a parameter
in struct display_info_t instead of using unconditionally DI0.
DI0 is the default setting.

Signed-off-by: Max Krummenacher <max.krummenacher@toradex.com>
---

 arch/arm/imx-common/video.c             | 2 +-
 arch/arm/include/asm/imx-common/video.h | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm/imx-common/video.c b/arch/arm/imx-common/video.c
index fdc987f..549bf9d 100644
--- a/arch/arm/imx-common/video.c
+++ b/arch/arm/imx-common/video.c
@@ -34,7 +34,7 @@ int board_video_skip(void)
 	}
 
 	if (i < display_count) {
-		ret = ipuv3_fb_init(&displays[i].mode, 0,
+		ret = ipuv3_fb_init(&displays[i].mode, displays[i].di ? 1 : 0,
 				    displays[i].pixfmt);
 		if (!ret) {
 			if (displays[i].enable)
diff --git a/arch/arm/include/asm/imx-common/video.h b/arch/arm/include/asm/imx-common/video.h
index cad5f86..941a031 100644
--- a/arch/arm/include/asm/imx-common/video.h
+++ b/arch/arm/include/asm/imx-common/video.h
@@ -12,6 +12,7 @@ struct display_info_t {
 	int	bus;
 	int	addr;
 	int	pixfmt;
+	int	di;
 	int	(*detect)(struct display_info_t const *dev);
 	void	(*enable)(struct display_info_t const *dev);
 	struct	fb_videomode mode;
-- 
2.6.6

[U-Boot] [PATCH 3/3] mx6: synchronize SPL to u-boot offset

[Max Krummenacher]

Synchronize CONFIG_SPL_PAD_TO (used at build time for u-boot-with-spl.imx) with
CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR (used by SPL to read u-boot image).

Signed-off-by: Max Krummenacher <max.krummenacher@toradex.com>
---

 include/configs/imx6_spl.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/configs/imx6_spl.h b/include/configs/imx6_spl.h
index 76d1ca0..0b426bb 100644
--- a/include/configs/imx6_spl.h
+++ b/include/configs/imx6_spl.h
@@ -23,6 +23,8 @@
  *    which consists of a 4K header in front of us that contains the IVT, DCD
  *    and some padding thus 'our' max size is really 0x00908000 - 0x00918000
  *    or 64KB
+ *  - Padding between start of SPL(with IVT...) and U-Boot is 68KB, SPL starts
+ *    at 1KB, U-Boot at 69kB into the storage media.
  */
 #define CONFIG_SYS_THUMB_BUILD
 #define CONFIG_SPL_LDSCRIPT	"arch/arm/cpu/armv7/omap-common/u-boot-spl.lds"
@@ -38,6 +40,7 @@
 /* MMC support */
 #if defined(CONFIG_SPL_MMC_SUPPORT)
 #define CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR	138 /* offset 69KB */
+#define CONFIG_SPL_PAD_TO			0x11000 /* offset 68KB */
 #define CONFIG_SYS_U_BOOT_MAX_SIZE_SECTORS	800 /* 400 KB */
 #define CONFIG_SYS_MMCSD_FS_BOOT_PARTITION	1
 #define CONFIG_SYS_MONITOR_LEN  (CONFIG_SYS_U_BOOT_MAX_SIZE_SECTORS/2*1024)
-- 
2.6.6

[U-Boot] [PATCH 3/3] mx6: synchronize SPL to u-boot offset

[Marek Vasut]

On 10/15/2016 07:10 PM, Max Krummenacher wrote:
> Synchronize CONFIG_SPL_PAD_TO (used at build time for u-boot-with-spl.imx) with
> CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR (used by SPL to read u-boot image).
> 
> Signed-off-by: Max Krummenacher <max.krummenacher@toradex.com>
> ---
> 
>  include/configs/imx6_spl.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/include/configs/imx6_spl.h b/include/configs/imx6_spl.h
> index 76d1ca0..0b426bb 100644
> --- a/include/configs/imx6_spl.h
> +++ b/include/configs/imx6_spl.h
> @@ -23,6 +23,8 @@
>   *    which consists of a 4K header in front of us that contains the IVT, DCD
>   *    and some padding thus 'our' max size is really 0x00908000 - 0x00918000
>   *    or 64KB
> + *  - Padding between start of SPL(with IVT...) and U-Boot is 68KB, SPL starts
> + *    at 1KB, U-Boot at 69kB into the storage media.
>   */
>  #define CONFIG_SYS_THUMB_BUILD
>  #define CONFIG_SPL_LDSCRIPT	"arch/arm/cpu/armv7/omap-common/u-boot-spl.lds"
> @@ -38,6 +40,7 @@
>  /* MMC support */
>  #if defined(CONFIG_SPL_MMC_SUPPORT)
>  #define CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR	138 /* offset 69KB */
> +#define CONFIG_SPL_PAD_TO			0x11000 /* offset 68KB */

Does this mess up boards which can boot from both SD and other boot
media (NAND, SPI NOR, PNOR...) ?

>  #define CONFIG_SYS_U_BOOT_MAX_SIZE_SECTORS	800 /* 400 KB */
>  #define CONFIG_SYS_MMCSD_FS_BOOT_PARTITION	1
>  #define CONFIG_SYS_MONITOR_LEN  (CONFIG_SYS_U_BOOT_MAX_SIZE_SECTORS/2*1024)
> 


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH 3/3] mx6: synchronize SPL to u-boot offset

[Max Krummenacher]

Hi Marak

> > --- a/include/configs/imx6_spl.h
> > +++ b/include/configs/imx6_spl.h
> > @@ -23,6 +23,8 @@
> >   *    which consists of a 4K header in front of us that contains
> > the IVT, DCD
> >   *    and some padding thus 'our' max size is really 0x00908000 -
> > 0x00918000
> >   *    or 64KB
> > + *  - Padding between start of SPL(with IVT...) and U-Boot is
> > 68KB, SPL starts
> > + *    at 1KB, U-Boot at 69kB into the storage media.
> >   */
> >  #define CONFIG_SYS_THUMB_BUILD
> >  #define CONFIG_SPL_LDSCRIPT	"arch/arm/cpu/armv7/omap
> > -common/u-boot-spl.lds"
> > @@ -38,6 +40,7 @@
> >  /* MMC support */
> >  #if defined(CONFIG_SPL_MMC_SUPPORT)
> >  #define CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR	138 /*
> > offset 69KB */
> > +#define CONFIG_SPL_PAD_TO			0x11000 /* offset
> > 68KB */
> 
> Does this mess up boards which can boot from both SD and other boot
> media (NAND, SPI NOR, PNOR...) ?

Good point.
CONFIG_SPL_PAD_TO is used to create the combined SPL/U-Boot binary.
While I have not found any use of this by any config which includes
imx6_spl.h it might hinder future boards.
So it's probably best to define CONFIG_SPL_PAD_TO in the individual
board configs and skip this patch.

Max

[U-Boot] [PATCH 3/3] mx6: synchronize SPL to u-boot offset

[Marek Vasut]

On 10/15/2016 09:17 PM, Max Krummenacher wrote:
> Hi Marak
> 
>>> --- a/include/configs/imx6_spl.h
>>> +++ b/include/configs/imx6_spl.h
>>> @@ -23,6 +23,8 @@
>>>   *    which consists of a 4K header in front of us that contains
>>> the IVT, DCD
>>>   *    and some padding thus 'our' max size is really 0x00908000 -
>>> 0x00918000
>>>   *    or 64KB
>>> + *  - Padding between start of SPL(with IVT...) and U-Boot is
>>> 68KB, SPL starts
>>> + *    at 1KB, U-Boot at 69kB into the storage media.
>>>   */
>>>  #define CONFIG_SYS_THUMB_BUILD
>>>  #define CONFIG_SPL_LDSCRIPT	"arch/arm/cpu/armv7/omap
>>> -common/u-boot-spl.lds"
>>> @@ -38,6 +40,7 @@
>>>  /* MMC support */
>>>  #if defined(CONFIG_SPL_MMC_SUPPORT)
>>>  #define CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR	138 /*
>>> offset 69KB */
>>> +#define CONFIG_SPL_PAD_TO			0x11000 /* offset
>>> 68KB */
>>
>> Does this mess up boards which can boot from both SD and other boot
>> media (NAND, SPI NOR, PNOR...) ?
> 
> Good point.
> CONFIG_SPL_PAD_TO is used to create the combined SPL/U-Boot binary.
> While I have not found any use of this by any config which includes
> imx6_spl.h it might hinder future boards.
> So it's probably best to define CONFIG_SPL_PAD_TO in the individual
> board configs and skip this patch.

So how did you come up with this patch ? What was the thought process?


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH 3/3] mx6: synchronize SPL to u-boot offset

[Max Krummenacher]

Hi Marek

>>>>  #if defined(CONFIG_SPL_MMC_SUPPORT)
>>>>  #define CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR    138 /*
>>>> offset 69KB */
>>>> +#define CONFIG_SPL_PAD_TO                  0x11000 /* offset
>>>> 68KB */
>>>
>>> Does this mess up boards which can boot from both SD and other boot
>>> media (NAND, SPI NOR, PNOR...) ?
>>
>> Good point.
>> CONFIG_SPL_PAD_TO is used to create the combined SPL/U-Boot binary.
>> While I have not found any use of this by any config which includes
>> imx6_spl.h it might hinder future boards.
>> So it's probably best to define CONFIG_SPL_PAD_TO in the individual
>> board configs and skip this patch.
>
> So how did you come up with this patch ? What was the thought process?
>
I'm preparing patches for submission of new boards, Colibri iMX6, Apalis iMX6.
They rely on this patchset
https://www.mail-archive.com/u-boot at lists.denx.de/msg227944.html, so
the board patches are not yet ready for submission.

I forward ported from a downstream 2015.04 U-Boot where 'make
u-boot-with-spl.imx' used an offset of 68KB between SPL and U-Boot
binary matching the SPL code which later loads U-Boot.
With the current master it used an offset of 64KB resulting in an SPL
not loading U-Boot.
Probably the 4KB stem from now counting SPL_PAD_TO from the start of
the IVT/DCD region and earlier counting from the start of SPL.

Max

[U-Boot] [PATCH 3/3] mx6: synchronize SPL to u-boot offset

[Marek Vasut]

On 10/17/2016 02:28 PM, Max Krummenacher wrote:
> Hi Marek
> 
>>>>>  #if defined(CONFIG_SPL_MMC_SUPPORT)
>>>>>  #define CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR    138 /*
>>>>> offset 69KB */
>>>>> +#define CONFIG_SPL_PAD_TO                  0x11000 /* offset
>>>>> 68KB */
>>>>
>>>> Does this mess up boards which can boot from both SD and other boot
>>>> media (NAND, SPI NOR, PNOR...) ?
>>>
>>> Good point.
>>> CONFIG_SPL_PAD_TO is used to create the combined SPL/U-Boot binary.
>>> While I have not found any use of this by any config which includes
>>> imx6_spl.h it might hinder future boards.
>>> So it's probably best to define CONFIG_SPL_PAD_TO in the individual
>>> board configs and skip this patch.
>>
>> So how did you come up with this patch ? What was the thought process?
>>
> I'm preparing patches for submission of new boards, Colibri iMX6, Apalis iMX6.
> They rely on this patchset
> https://www.mail-archive.com/u-boot at lists.denx.de/msg227944.html, so
> the board patches are not yet ready for submission.

OT: Can you make those boards load u-boot.img from extfs partition instead ?

> I forward ported from a downstream 2015.04 U-Boot where 'make
> u-boot-with-spl.imx' used an offset of 68KB between SPL and U-Boot
> binary matching the SPL code which later loads U-Boot.
> With the current master it used an offset of 64KB resulting in an SPL
> not loading U-Boot.
> Probably the 4KB stem from now counting SPL_PAD_TO from the start of
> the IVT/DCD region and earlier counting from the start of SPL.

I see, thanks for clarifying.

> Max
> 


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH 3/3] mx6: synchronize SPL to u-boot offset

[Max Krummenacher]

2016-10-17 14:45 GMT+02:00 Marek Vasut <marex@denx.de>:
> On 10/17/2016 02:28 PM, Max Krummenacher wrote:
>> Hi Marek
>>
>>>>>>  #if defined(CONFIG_SPL_MMC_SUPPORT)
>>>>>>  #define CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR    138 /*
>>>>>> offset 69KB */
>>>>>> +#define CONFIG_SPL_PAD_TO                  0x11000 /* offset
>>>>>> 68KB */
>>>>>
>>>>> Does this mess up boards which can boot from both SD and other boot
>>>>> media (NAND, SPI NOR, PNOR...) ?
>>>>
>>>> Good point.
>>>> CONFIG_SPL_PAD_TO is used to create the combined SPL/U-Boot binary.
>>>> While I have not found any use of this by any config which includes
>>>> imx6_spl.h it might hinder future boards.
>>>> So it's probably best to define CONFIG_SPL_PAD_TO in the individual
>>>> board configs and skip this patch.
>>>
>>> So how did you come up with this patch ? What was the thought process?
>>>
>> I'm preparing patches for submission of new boards, Colibri iMX6, Apalis iMX6.
>> They rely on this patchset
>> https://www.mail-archive.com/u-boot at lists.denx.de/msg227944.html, so
>> the board patches are not yet ready for submission.
>
> OT: Can you make those boards load u-boot.img from extfs partition instead ?

I probably could but did not try.

Also it would change the way we use U-Boot across our module family
and I'm worried that we more often need to recover over USB due to a
corrupted filesystem.
On top of that I made some measurements on loading the kernel from an
ext2 instead of an fat fs and have seen differences bigger than 100ms.
So parsing into a filesystem twice would probably add even more
overhead.

Max

>
>> I forward ported from a downstream 2015.04 U-Boot where 'make
>> u-boot-with-spl.imx' used an offset of 68KB between SPL and U-Boot
>> binary matching the SPL code which later loads U-Boot.
>> With the current master it used an offset of 64KB resulting in an SPL
>> not loading U-Boot.
>> Probably the 4KB stem from now counting SPL_PAD_TO from the start of
>> the IVT/DCD region and earlier counting from the start of SPL.
>
> I see, thanks for clarifying.
>
>> Max
>>
>
>
> --
> Best regards,
> Marek Vasut

[U-Boot] [PATCH 1/3] spl: mmc: fix switch statement

[Marek Vasut]

On 10/15/2016 07:10 PM, Max Krummenacher wrote:
> If CONFIG_SPL_LIBCOMMON_SUPPORT is not defined there is a lone case statement
> at the end of the switch leading to a compile error.
> Remove the offending case statement.
> 
> | common/spl/spl_mmc.c:339:7: error: label at end of compound statement
> 
> Signed-off-by: Max Krummenacher <max.krummenacher@toradex.com>
> ---
> 
>  common/spl/spl_mmc.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c
> index c674e61..367b4e4 100644
> --- a/common/spl/spl_mmc.c
> +++ b/common/spl/spl_mmc.c
> @@ -342,7 +342,6 @@ static int spl_mmc_load_image(struct spl_image_info *spl_image,
>  			return err;
>  
>  		break;
> -	case MMCSD_MODE_UNDEFINED:

This patch is wrong -- in case CONFIG_SPL_LIBCOMMON_SUPPORT is enabled
and mode is MMCSD_MODE_UNDEFINED, the message in the puts() below would
be printed. After applying this change, the message won't be printed .

The fix is probably something like:

case foo:
default:
#ifdef CONFIG_BAR
    puts();
#endif
    break;

>  #ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
>  	default:
>  		puts("spl: mmc: wrong boot mode\n");
> 


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH 1/3] spl: mmc: fix switch statement

[Max Krummenacher]

Hi Marek

Am Samstag, den 15.10.2016, 19:29 +0200 schrieb Marek Vasut:
> On 10/15/2016 07:10 PM, Max Krummenacher wrote:
> > If CONFIG_SPL_LIBCOMMON_SUPPORT is not defined there is a lone case
> > statement
> > at the end of the switch leading to a compile error.
> > Remove the offending case statement.
> > 
> > > common/spl/spl_mmc.c:339:7: error: label at end of compound
> > > statement
> > 
> > Signed-off-by: Max Krummenacher <max.krummenacher@toradex.com>
> > ---
> > 
> >  common/spl/spl_mmc.c | 1 -
> >  1 file changed, 1 deletion(-)
> > 
> > diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c
> > index c674e61..367b4e4 100644
> > --- a/common/spl/spl_mmc.c
> > +++ b/common/spl/spl_mmc.c
> > @@ -342,7 +342,6 @@ static int spl_mmc_load_image(struct
> > spl_image_info *spl_image,
> >  			return err;
> >  
> >  		break;
> > -	case MMCSD_MODE_UNDEFINED:
> 
> This patch is wrong -- in case CONFIG_SPL_LIBCOMMON_SUPPORT is
> enabled
> and mode is MMCSD_MODE_UNDEFINED, the message in the puts() below
> would
> be printed. After applying this change, the message won't be printed


I disagree.

With CONFIG_SPL_LIBCOMMON_SUPPORT we had something like this:
switch(bar) {
...
case foo:
default:
	put("bla\n");
}

as 'case foo:' falls through into 'default:' removing the specific case
does not change anything.

Regards
Max


> The fix is probably something like:
> 
> case foo:
> default:
> #ifdef CONFIG_BAR
>     puts();
> #endif
>     break;
> 
> >  #ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
> >  	default:
> >  		puts("spl: mmc: wrong boot mode\n");
> > 
> 

[U-Boot] [PATCH 1/3] spl: mmc: fix switch statement

[Marek Vasut]

On 10/15/2016 09:18 PM, Max Krummenacher wrote:
> Hi Marek
> 
> Am Samstag, den 15.10.2016, 19:29 +0200 schrieb Marek Vasut:
>> On 10/15/2016 07:10 PM, Max Krummenacher wrote:
>>> If CONFIG_SPL_LIBCOMMON_SUPPORT is not defined there is a lone case
>>> statement
>>> at the end of the switch leading to a compile error.
>>> Remove the offending case statement.
>>>
>>>> common/spl/spl_mmc.c:339:7: error: label at end of compound
>>>> statement
>>>
>>> Signed-off-by: Max Krummenacher <max.krummenacher@toradex.com>
>>> ---
>>>
>>>  common/spl/spl_mmc.c | 1 -
>>>  1 file changed, 1 deletion(-)
>>>
>>> diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c
>>> index c674e61..367b4e4 100644
>>> --- a/common/spl/spl_mmc.c
>>> +++ b/common/spl/spl_mmc.c
>>> @@ -342,7 +342,6 @@ static int spl_mmc_load_image(struct
>>> spl_image_info *spl_image,
>>>  			return err;
>>>  
>>>  		break;
>>> -	case MMCSD_MODE_UNDEFINED:
>>
>> This patch is wrong -- in case CONFIG_SPL_LIBCOMMON_SUPPORT is
>> enabled
>> and mode is MMCSD_MODE_UNDEFINED, the message in the puts() below
>> would
>> be printed. After applying this change, the message won't be printed
> 
> 
> I disagree.
> 
> With CONFIG_SPL_LIBCOMMON_SUPPORT we had something like this:
> switch(bar) {
> ...
> case foo:
> default:
> 	put("bla\n");
> }
> 
> as 'case foo:' falls through into 'default:' removing the specific case
> does not change anything.

Oh, that's a good point, I missed that and that's in fact pretty elegant
fix. Thanks.

Acked-by: Marek Vasut <marex@denx.de>

[...]
-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH 1/3] spl: mmc: fix switch statement

[Tom Rini]

On Sat, Oct 15, 2016 at 07:10:14PM +0200, Max Krummenacher wrote:

> If CONFIG_SPL_LIBCOMMON_SUPPORT is not defined there is a lone case statement
> at the end of the switch leading to a compile error.
> Remove the offending case statement.
> 
> | common/spl/spl_mmc.c:339:7: error: label at end of compound statement
> 
> Signed-off-by: Max Krummenacher <max.krummenacher@toradex.com>

Reviewed-by: Tom Rini <trini@konsulko.com>

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20161016/5bce30e7/attachment.sig>