[U-Boot] Beaglebone Black broken since commit fd61d39970b9901217efc7536d9f3a61b4e1752a

[U-Boot] Beaglebone Black broken since commit fd61d39970b9901217efc7536d9f3a61b4e1752a

[Guillaume Gardet]

Hi,

Beaglebone black (am335x_evm_defconfig) is broken (with MMC boot and u-boot.img on ext4 partition).
I bisected it to commit fd61d39970b9901217efc7536d9f3a61b4e1752a
     spl: mmc: add break statements in spl_mmc_load_image()
from Nikita Kiryanov (in Cc).

Working image gives:
********************************************************************************
     U-Boot SPL 2016.01-rc1-00036-g483ab3d (Feb 16 2016 - 19:04:10)
     bad magic
     bad magic
     spl_register_fat_device: fat register err - -1
     spl_register_fat_device: fat register err - -1
     spl_load_image_fat: error reading image u-boot.img, err - -1
     spl: ext4fs_open failed
     spl: ext4fs_open failed
     spl_load_image_ext: error reading image uImage, err - -1


     U-Boot 2016.01-rc1-00033-g9884f44 (Feb 16 2016 - 18:55:52 +0100)
********************************************************************************

Since this commit, we get:
********************************************************************************
     U-Boot SPL 2016.01-rc1-00037-gfd61d39 (Feb 16 2016 - 19:01:54)
     bad magic
     bad magic
     ### ERROR ### Please RESET the board ###
********************************************************************************


Guillaume

[U-Boot] [PATCH] spl: if MMCSD_MODE_RAW fails, try MMCSD_MODE_FS, if available

[Guillaume GARDET]

Since commit fd61d39970b9901217efc7536d9f3a61b4e1752a:
        spl: mmc: add break statements in spl_mmc_load_image() 
RAW and FS boot modes are now exclusive again. So, if MMCSD_MODE_RAW fails, the
board hangs. This patch allows to try MMCSD_MODE_FS then, if available.

It has been tested on a beaglebone black to boot on an EXT partition.

Signed-off-by: Guillaume GARDET <guillaume.gardet@free.fr>
Cc: Tom Rini <trini@konsulko.com>
Cc: Nikita Kiryanov <nikita@compulab.co.il>
Cc: Igor Grinberg <grinberg@compulab.co.il>
Cc: Paul Kocialkowski <contact@paulk.fr>
Cc: Pantelis Antoniou <panto@antoniou-consulting.com>
Cc: Simon Glass <sjg@chromium.org>
Cc: Matwey V. Kornilov <matwey.kornilov@gmail.com>

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

diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c
index c3931c6..2eef0f2 100644
--- a/common/spl/spl_mmc.c
+++ b/common/spl/spl_mmc.c
@@ -284,7 +284,7 @@ int spl_mmc_load_image(u32 boot_device)
 		if (!err)
 			return err;
 #endif
-		break;
+		/* Fall through */
 	case MMCSD_MODE_FS:
 		debug("spl: mmc boot mode: fs\n");
 
-- 
1.8.4.5

[U-Boot] [PATCH] spl: if MMCSD_MODE_RAW fails, try MMCSD_MODE_FS, if available

[Tom Rini]

On Wed, Feb 17, 2016 at 09:09:27AM +0100, Guillaume GARDET wrote:

> Since commit fd61d39970b9901217efc7536d9f3a61b4e1752a:
>         spl: mmc: add break statements in spl_mmc_load_image() 
> RAW and FS boot modes are now exclusive again. So, if MMCSD_MODE_RAW fails, the
> board hangs. This patch allows to try MMCSD_MODE_FS then, if available.
> 
> It has been tested on a beaglebone black to boot on an EXT partition.
> 
> Signed-off-by: Guillaume GARDET <guillaume.gardet@free.fr>
> Cc: Tom Rini <trini@konsulko.com>
> Cc: Nikita Kiryanov <nikita@compulab.co.il>
> Cc: Igor Grinberg <grinberg@compulab.co.il>
> Cc: Paul Kocialkowski <contact@paulk.fr>
> Cc: Pantelis Antoniou <panto@antoniou-consulting.com>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Matwey V. Kornilov <matwey.kornilov@gmail.com>
> 
> ---
>  common/spl/spl_mmc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c
> index c3931c6..2eef0f2 100644
> --- a/common/spl/spl_mmc.c
> +++ b/common/spl/spl_mmc.c
> @@ -284,7 +284,7 @@ int spl_mmc_load_image(u32 boot_device)
>  		if (!err)
>  			return err;
>  #endif
> -		break;
> +		/* Fall through */
>  	case MMCSD_MODE_FS:
>  		debug("spl: mmc boot mode: fs\n");

This also essentially reverts fd61d399.  So Nikita, was there a specific
use case that was broken before, or was the code just unclear in
intentions here?  Thanks!

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

[U-Boot] [PATCH] spl: if MMCSD_MODE_RAW fails, try MMCSD_MODE_FS, if available

[Nikita Kiryanov]

Hi Tom, Guillaume,

On Wed, Feb 17, 2016 at 03:27:22PM -0500, Tom Rini wrote:
> On Wed, Feb 17, 2016 at 09:09:27AM +0100, Guillaume GARDET wrote:
> 
> > Since commit fd61d39970b9901217efc7536d9f3a61b4e1752a:
> >         spl: mmc: add break statements in spl_mmc_load_image() 
> > RAW and FS boot modes are now exclusive again. So, if MMCSD_MODE_RAW fails, the
> > board hangs. This patch allows to try MMCSD_MODE_FS then, if available.
> > 
> > It has been tested on a beaglebone black to boot on an EXT partition.
> > 
> > Signed-off-by: Guillaume GARDET <guillaume.gardet@free.fr>
> > Cc: Tom Rini <trini@konsulko.com>
> > Cc: Nikita Kiryanov <nikita@compulab.co.il>
> > Cc: Igor Grinberg <grinberg@compulab.co.il>
> > Cc: Paul Kocialkowski <contact@paulk.fr>
> > Cc: Pantelis Antoniou <panto@antoniou-consulting.com>
> > Cc: Simon Glass <sjg@chromium.org>
> > Cc: Matwey V. Kornilov <matwey.kornilov@gmail.com>
> > 
> > ---
> >  common/spl/spl_mmc.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c
> > index c3931c6..2eef0f2 100644
> > --- a/common/spl/spl_mmc.c
> > +++ b/common/spl/spl_mmc.c
> > @@ -284,7 +284,7 @@ int spl_mmc_load_image(u32 boot_device)
> >  		if (!err)
> >  			return err;
> >  #endif
> > -		break;
> > +		/* Fall through */
> >  	case MMCSD_MODE_FS:
> >  		debug("spl: mmc boot mode: fs\n");
> 
> This also essentially reverts fd61d399.  So Nikita, was there a specific
> use case that was broken before, or was the code just unclear in
> intentions here?  Thanks!

There was no broken use case that I'm aware of. The change was made as
part of a code improvement series and was meant to address what I
consider to be bad and problematic design. Instead of reverting it
though, how about implementing something similar to what I did in the
main common/spl/spl.c:board_init_r()? You would have a weak function
that will default to the original spl_boot_mode() if not overridden,
and allow the user to define a sequence of boot modes otherwise.

> 
> -- 
> Tom

[U-Boot] [PATCH] spl: if MMCSD_MODE_RAW fails, try MMCSD_MODE_FS, if available

[Guillaume Gardet]

Hi Tom, Nikita ,

Le 18/02/2016 10:19, Nikita Kiryanov a ?crit :
> Hi Tom, Guillaume,
>
> On Wed, Feb 17, 2016 at 03:27:22PM -0500, Tom Rini wrote:
>> On Wed, Feb 17, 2016 at 09:09:27AM +0100, Guillaume GARDET wrote:
>>
>>> Since commit fd61d39970b9901217efc7536d9f3a61b4e1752a:
>>>          spl: mmc: add break statements in spl_mmc_load_image()
>>> RAW and FS boot modes are now exclusive again. So, if MMCSD_MODE_RAW fails, the
>>> board hangs. This patch allows to try MMCSD_MODE_FS then, if available.
>>>
>>> It has been tested on a beaglebone black to boot on an EXT partition.
>>>
>>> Signed-off-by: Guillaume GARDET <guillaume.gardet@free.fr>
>>> Cc: Tom Rini <trini@konsulko.com>
>>> Cc: Nikita Kiryanov <nikita@compulab.co.il>
>>> Cc: Igor Grinberg <grinberg@compulab.co.il>
>>> Cc: Paul Kocialkowski <contact@paulk.fr>
>>> Cc: Pantelis Antoniou <panto@antoniou-consulting.com>
>>> Cc: Simon Glass <sjg@chromium.org>
>>> Cc: Matwey V. Kornilov <matwey.kornilov@gmail.com>
>>>
>>> ---
>>>   common/spl/spl_mmc.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c
>>> index c3931c6..2eef0f2 100644
>>> --- a/common/spl/spl_mmc.c
>>> +++ b/common/spl/spl_mmc.c
>>> @@ -284,7 +284,7 @@ int spl_mmc_load_image(u32 boot_device)
>>>   		if (!err)
>>>   			return err;
>>>   #endif
>>> -		break;
>>> +		/* Fall through */
>>>   	case MMCSD_MODE_FS:
>>>   		debug("spl: mmc boot mode: fs\n");
>> This also essentially reverts fd61d399.  So Nikita, was there a specific
>> use case that was broken before, or was the code just unclear in
>> intentions here?  Thanks!
> There was no broken use case that I'm aware of. The change was made as
> part of a code improvement series and was meant to address what I
> consider to be bad and problematic design. Instead of reverting it
> though, how about implementing something similar to what I did in the
> main common/spl/spl.c:board_init_r()? You would have a weak function
> that will default to the original spl_boot_mode() if not overridden,
> and allow the user to define a sequence of boot modes otherwise.

The thing is you broke a wanted behavior currently in use. So, the priority is to come back to the previous behavior.
Then, if someone (you, me or someone else) wants to improve this code, the way you suggested, it would be very nice.
But it will need a lot more work, tests and reviews.


Guillaume

>
>> -- 
>> Tom
>
>

[U-Boot] [PATCH] spl: if MMCSD_MODE_RAW fails, try MMCSD_MODE_FS, if available

[Nikita Kiryanov]

On Thu, Feb 18, 2016 at 11:06:32AM +0100, Guillaume Gardet wrote:
> Hi Tom, Nikita ,
> 
> Le 18/02/2016 10:19, Nikita Kiryanov a ?crit :
> >Hi Tom, Guillaume,
> >
> >On Wed, Feb 17, 2016 at 03:27:22PM -0500, Tom Rini wrote:
> >>On Wed, Feb 17, 2016 at 09:09:27AM +0100, Guillaume GARDET wrote:
> >>
> >>>Since commit fd61d39970b9901217efc7536d9f3a61b4e1752a:
> >>>         spl: mmc: add break statements in spl_mmc_load_image()
> >>>RAW and FS boot modes are now exclusive again. So, if MMCSD_MODE_RAW fails, the
> >>>board hangs. This patch allows to try MMCSD_MODE_FS then, if available.
> >>>
> >>>It has been tested on a beaglebone black to boot on an EXT partition.
> >>>
> >>>Signed-off-by: Guillaume GARDET <guillaume.gardet@free.fr>
> >>>Cc: Tom Rini <trini@konsulko.com>
> >>>Cc: Nikita Kiryanov <nikita@compulab.co.il>
> >>>Cc: Igor Grinberg <grinberg@compulab.co.il>
> >>>Cc: Paul Kocialkowski <contact@paulk.fr>
> >>>Cc: Pantelis Antoniou <panto@antoniou-consulting.com>
> >>>Cc: Simon Glass <sjg@chromium.org>
> >>>Cc: Matwey V. Kornilov <matwey.kornilov@gmail.com>
> >>>
> >>>---
> >>>  common/spl/spl_mmc.c | 2 +-
> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>>diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c
> >>>index c3931c6..2eef0f2 100644
> >>>--- a/common/spl/spl_mmc.c
> >>>+++ b/common/spl/spl_mmc.c
> >>>@@ -284,7 +284,7 @@ int spl_mmc_load_image(u32 boot_device)
> >>>  		if (!err)
> >>>  			return err;
> >>>  #endif
> >>>-		break;
> >>>+		/* Fall through */
> >>>  	case MMCSD_MODE_FS:
> >>>  		debug("spl: mmc boot mode: fs\n");
> >>This also essentially reverts fd61d399.  So Nikita, was there a specific
> >>use case that was broken before, or was the code just unclear in
> >>intentions here?  Thanks!
> >There was no broken use case that I'm aware of. The change was made as
> >part of a code improvement series and was meant to address what I
> >consider to be bad and problematic design. Instead of reverting it
> >though, how about implementing something similar to what I did in the
> >main common/spl/spl.c:board_init_r()? You would have a weak function
> >that will default to the original spl_boot_mode() if not overridden,
> >and allow the user to define a sequence of boot modes otherwise.
> 
> The thing is you broke a wanted behavior currently in use. So, the priority is to come back to the previous behavior.

Could you add a comment indicating that this is wanted behavior that
has a user, and who the user is?

> Then, if someone (you, me or someone else) wants to improve this code, the way you suggested, it would be very nice.
> But it will need a lot more work, tests and reviews.
> 
> 
> Guillaume
> 
> >
> >>-- 
> >>Tom
> >
> >
> 

[U-Boot] [PATCH] spl: if MMCSD_MODE_RAW fails, try MMCSD_MODE_FS, if available

[Guillaume Gardet]

Le 18/02/2016 14:07, Nikita Kiryanov a ?crit :
> On Thu, Feb 18, 2016 at 11:06:32AM +0100, Guillaume Gardet wrote:
>> Hi Tom, Nikita ,
>>
>> Le 18/02/2016 10:19, Nikita Kiryanov a ?crit :
>>> Hi Tom, Guillaume,
>>>
>>> On Wed, Feb 17, 2016 at 03:27:22PM -0500, Tom Rini wrote:
>>>> On Wed, Feb 17, 2016 at 09:09:27AM +0100, Guillaume GARDET wrote:
>>>>
>>>>> Since commit fd61d39970b9901217efc7536d9f3a61b4e1752a:
>>>>>          spl: mmc: add break statements in spl_mmc_load_image()
>>>>> RAW and FS boot modes are now exclusive again. So, if MMCSD_MODE_RAW fails, the
>>>>> board hangs. This patch allows to try MMCSD_MODE_FS then, if available.
>>>>>
>>>>> It has been tested on a beaglebone black to boot on an EXT partition.
>>>>>
>>>>> Signed-off-by: Guillaume GARDET <guillaume.gardet@free.fr>
>>>>> Cc: Tom Rini <trini@konsulko.com>
>>>>> Cc: Nikita Kiryanov <nikita@compulab.co.il>
>>>>> Cc: Igor Grinberg <grinberg@compulab.co.il>
>>>>> Cc: Paul Kocialkowski <contact@paulk.fr>
>>>>> Cc: Pantelis Antoniou <panto@antoniou-consulting.com>
>>>>> Cc: Simon Glass <sjg@chromium.org>
>>>>> Cc: Matwey V. Kornilov <matwey.kornilov@gmail.com>
>>>>>
>>>>> ---
>>>>>   common/spl/spl_mmc.c | 2 +-
>>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c
>>>>> index c3931c6..2eef0f2 100644
>>>>> --- a/common/spl/spl_mmc.c
>>>>> +++ b/common/spl/spl_mmc.c
>>>>> @@ -284,7 +284,7 @@ int spl_mmc_load_image(u32 boot_device)
>>>>>   		if (!err)
>>>>>   			return err;
>>>>>   #endif
>>>>> -		break;
>>>>> +		/* Fall through */
>>>>>   	case MMCSD_MODE_FS:
>>>>>   		debug("spl: mmc boot mode: fs\n");
>>>> This also essentially reverts fd61d399.  So Nikita, was there a specific
>>>> use case that was broken before, or was the code just unclear in
>>>> intentions here?  Thanks!
>>> There was no broken use case that I'm aware of. The change was made as
>>> part of a code improvement series and was meant to address what I
>>> consider to be bad and problematic design. Instead of reverting it
>>> though, how about implementing something similar to what I did in the
>>> main common/spl/spl.c:board_init_r()? You would have a weak function
>>> that will default to the original spl_boot_mode() if not overridden,
>>> and allow the user to define a sequence of boot modes otherwise.
>> The thing is you broke a wanted behavior currently in use. So, the priority is to come back to the previous behavior.
> Could you add a comment indicating that this is wanted behavior that
> has a user, and who the user is?

Not sure what you mean.
I think, "Fall through" code comment explains it (as you done with MMCSD_MODE_EMMCBOOT).
I (and openSUSE ARM) do not pretend to be the only user(s) of this feature, so I won't add my name there.
If you mean commit message, I think the current one is enough.


Guillaume


>
>> Then, if someone (you, me or someone else) wants to improve this code, the way you suggested, it would be very nice.
>> But it will need a lot more work, tests and reviews.
>>
>>
>> Guillaume
>>
>>>> -- 
>>>> Tom
>>>

[U-Boot] [PATCH] spl: if MMCSD_MODE_RAW fails, try MMCSD_MODE_FS, if available

[Nikita Kiryanov]

On Thu, Feb 18, 2016 at 02:31:08PM +0100, Guillaume Gardet wrote:
> 
> 
> Le 18/02/2016 14:07, Nikita Kiryanov a ?crit :
> >On Thu, Feb 18, 2016 at 11:06:32AM +0100, Guillaume Gardet wrote:
> >>Hi Tom, Nikita ,
> >>
> >>Le 18/02/2016 10:19, Nikita Kiryanov a ?crit :
> >>>Hi Tom, Guillaume,
> >>>
> >>>On Wed, Feb 17, 2016 at 03:27:22PM -0500, Tom Rini wrote:
> >>>>On Wed, Feb 17, 2016 at 09:09:27AM +0100, Guillaume GARDET wrote:
> >>>>
> >>>>>Since commit fd61d39970b9901217efc7536d9f3a61b4e1752a:
> >>>>>         spl: mmc: add break statements in spl_mmc_load_image()
> >>>>>RAW and FS boot modes are now exclusive again. So, if MMCSD_MODE_RAW fails, the
> >>>>>board hangs. This patch allows to try MMCSD_MODE_FS then, if available.
> >>>>>
> >>>>>It has been tested on a beaglebone black to boot on an EXT partition.
> >>>>>
> >>>>>Signed-off-by: Guillaume GARDET <guillaume.gardet@free.fr>
> >>>>>Cc: Tom Rini <trini@konsulko.com>
> >>>>>Cc: Nikita Kiryanov <nikita@compulab.co.il>
> >>>>>Cc: Igor Grinberg <grinberg@compulab.co.il>
> >>>>>Cc: Paul Kocialkowski <contact@paulk.fr>
> >>>>>Cc: Pantelis Antoniou <panto@antoniou-consulting.com>
> >>>>>Cc: Simon Glass <sjg@chromium.org>
> >>>>>Cc: Matwey V. Kornilov <matwey.kornilov@gmail.com>
> >>>>>
> >>>>>---
> >>>>>  common/spl/spl_mmc.c | 2 +-
> >>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>
> >>>>>diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c
> >>>>>index c3931c6..2eef0f2 100644
> >>>>>--- a/common/spl/spl_mmc.c
> >>>>>+++ b/common/spl/spl_mmc.c
> >>>>>@@ -284,7 +284,7 @@ int spl_mmc_load_image(u32 boot_device)
> >>>>>  		if (!err)
> >>>>>  			return err;
> >>>>>  #endif
> >>>>>-		break;
> >>>>>+		/* Fall through */
> >>>>>  	case MMCSD_MODE_FS:
> >>>>>  		debug("spl: mmc boot mode: fs\n");
> >>>>This also essentially reverts fd61d399.  So Nikita, was there a specific
> >>>>use case that was broken before, or was the code just unclear in
> >>>>intentions here?  Thanks!
> >>>There was no broken use case that I'm aware of. The change was made as
> >>>part of a code improvement series and was meant to address what I
> >>>consider to be bad and problematic design. Instead of reverting it
> >>>though, how about implementing something similar to what I did in the
> >>>main common/spl/spl.c:board_init_r()? You would have a weak function
> >>>that will default to the original spl_boot_mode() if not overridden,
> >>>and allow the user to define a sequence of boot modes otherwise.
> >>The thing is you broke a wanted behavior currently in use. So, the priority is to come back to the previous behavior.
> >Could you add a comment indicating that this is wanted behavior that
> >has a user, and who the user is?
> 
> Not sure what you mean.

I mean something like:
/* If raw mode fails, try fs mode. Some boards, such as beaglebone black,
 * depend on this funcitonality.
 */

> I think, "Fall through" code comment explains it (as you done with MMCSD_MODE_EMMCBOOT).

That was meant to communicate a very different type of coupling between
the two cases though..

[U-Boot] [PATCH] spl: if MMCSD_MODE_RAW fails, try MMCSD_MODE_FS, if available

[Tom Rini]

On Thu, Feb 18, 2016 at 04:25:29PM +0200, Nikita Kiryanov wrote:
> On Thu, Feb 18, 2016 at 02:31:08PM +0100, Guillaume Gardet wrote:
> > 
> > 
> > Le 18/02/2016 14:07, Nikita Kiryanov a ?crit :
> > >On Thu, Feb 18, 2016 at 11:06:32AM +0100, Guillaume Gardet wrote:
> > >>Hi Tom, Nikita ,
> > >>
> > >>Le 18/02/2016 10:19, Nikita Kiryanov a ?crit :
> > >>>Hi Tom, Guillaume,
> > >>>
> > >>>On Wed, Feb 17, 2016 at 03:27:22PM -0500, Tom Rini wrote:
> > >>>>On Wed, Feb 17, 2016 at 09:09:27AM +0100, Guillaume GARDET wrote:
> > >>>>
> > >>>>>Since commit fd61d39970b9901217efc7536d9f3a61b4e1752a:
> > >>>>>         spl: mmc: add break statements in spl_mmc_load_image()
> > >>>>>RAW and FS boot modes are now exclusive again. So, if MMCSD_MODE_RAW fails, the
> > >>>>>board hangs. This patch allows to try MMCSD_MODE_FS then, if available.
> > >>>>>
> > >>>>>It has been tested on a beaglebone black to boot on an EXT partition.
> > >>>>>
> > >>>>>Signed-off-by: Guillaume GARDET <guillaume.gardet@free.fr>
> > >>>>>Cc: Tom Rini <trini@konsulko.com>
> > >>>>>Cc: Nikita Kiryanov <nikita@compulab.co.il>
> > >>>>>Cc: Igor Grinberg <grinberg@compulab.co.il>
> > >>>>>Cc: Paul Kocialkowski <contact@paulk.fr>
> > >>>>>Cc: Pantelis Antoniou <panto@antoniou-consulting.com>
> > >>>>>Cc: Simon Glass <sjg@chromium.org>
> > >>>>>Cc: Matwey V. Kornilov <matwey.kornilov@gmail.com>
> > >>>>>
> > >>>>>---
> > >>>>>  common/spl/spl_mmc.c | 2 +-
> > >>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> > >>>>>
> > >>>>>diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c
> > >>>>>index c3931c6..2eef0f2 100644
> > >>>>>--- a/common/spl/spl_mmc.c
> > >>>>>+++ b/common/spl/spl_mmc.c
> > >>>>>@@ -284,7 +284,7 @@ int spl_mmc_load_image(u32 boot_device)
> > >>>>>  		if (!err)
> > >>>>>  			return err;
> > >>>>>  #endif
> > >>>>>-		break;
> > >>>>>+		/* Fall through */
> > >>>>>  	case MMCSD_MODE_FS:
> > >>>>>  		debug("spl: mmc boot mode: fs\n");
> > >>>>This also essentially reverts fd61d399.  So Nikita, was there a specific
> > >>>>use case that was broken before, or was the code just unclear in
> > >>>>intentions here?  Thanks!
> > >>>There was no broken use case that I'm aware of. The change was made as
> > >>>part of a code improvement series and was meant to address what I
> > >>>consider to be bad and problematic design. Instead of reverting it
> > >>>though, how about implementing something similar to what I did in the
> > >>>main common/spl/spl.c:board_init_r()? You would have a weak function
> > >>>that will default to the original spl_boot_mode() if not overridden,
> > >>>and allow the user to define a sequence of boot modes otherwise.
> > >>The thing is you broke a wanted behavior currently in use. So, the priority is to come back to the previous behavior.
> > >Could you add a comment indicating that this is wanted behavior that
> > >has a user, and who the user is?
> > 
> > Not sure what you mean.
> 
> I mean something like:
> /* If raw mode fails, try fs mode. Some boards, such as beaglebone black,
>  * depend on this funcitonality.
>  */

But it's not board specific, it's use-case specific.  It's saying that
instead of shoving both SPL and U-Boot into the correct magic raw
location, just shove SPL there and let U-Boot itself be in the /boot
partition.  This is just as applicable on say imx6 as it is on TI parts.

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

[U-Boot] [PATCH] spl: if MMCSD_MODE_RAW fails, try MMCSD_MODE_FS, if available

[Nikita Kiryanov]

On Thu, Feb 18, 2016 at 09:36:01AM -0500, Tom Rini wrote:
> On Thu, Feb 18, 2016 at 04:25:29PM +0200, Nikita Kiryanov wrote:
> > On Thu, Feb 18, 2016 at 02:31:08PM +0100, Guillaume Gardet wrote:
> > > 
> > > 
> > > Le 18/02/2016 14:07, Nikita Kiryanov a ?crit :
> > > >On Thu, Feb 18, 2016 at 11:06:32AM +0100, Guillaume Gardet wrote:
> > > >>Hi Tom, Nikita ,
> > > >>
> > > >>Le 18/02/2016 10:19, Nikita Kiryanov a ?crit :
> > > >>>Hi Tom, Guillaume,
> > > >>>
> > > >>>On Wed, Feb 17, 2016 at 03:27:22PM -0500, Tom Rini wrote:
> > > >>>>On Wed, Feb 17, 2016 at 09:09:27AM +0100, Guillaume GARDET wrote:
> > > >>>>
> > > >>>>>Since commit fd61d39970b9901217efc7536d9f3a61b4e1752a:
> > > >>>>>         spl: mmc: add break statements in spl_mmc_load_image()
> > > >>>>>RAW and FS boot modes are now exclusive again. So, if MMCSD_MODE_RAW fails, the
> > > >>>>>board hangs. This patch allows to try MMCSD_MODE_FS then, if available.
> > > >>>>>
> > > >>>>>It has been tested on a beaglebone black to boot on an EXT partition.
> > > >>>>>
> > > >>>>>Signed-off-by: Guillaume GARDET <guillaume.gardet@free.fr>
> > > >>>>>Cc: Tom Rini <trini@konsulko.com>
> > > >>>>>Cc: Nikita Kiryanov <nikita@compulab.co.il>
> > > >>>>>Cc: Igor Grinberg <grinberg@compulab.co.il>
> > > >>>>>Cc: Paul Kocialkowski <contact@paulk.fr>
> > > >>>>>Cc: Pantelis Antoniou <panto@antoniou-consulting.com>
> > > >>>>>Cc: Simon Glass <sjg@chromium.org>
> > > >>>>>Cc: Matwey V. Kornilov <matwey.kornilov@gmail.com>
> > > >>>>>
> > > >>>>>---
> > > >>>>>  common/spl/spl_mmc.c | 2 +-
> > > >>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >>>>>
> > > >>>>>diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c
> > > >>>>>index c3931c6..2eef0f2 100644
> > > >>>>>--- a/common/spl/spl_mmc.c
> > > >>>>>+++ b/common/spl/spl_mmc.c
> > > >>>>>@@ -284,7 +284,7 @@ int spl_mmc_load_image(u32 boot_device)
> > > >>>>>  		if (!err)
> > > >>>>>  			return err;
> > > >>>>>  #endif
> > > >>>>>-		break;
> > > >>>>>+		/* Fall through */
> > > >>>>>  	case MMCSD_MODE_FS:
> > > >>>>>  		debug("spl: mmc boot mode: fs\n");
> > > >>>>This also essentially reverts fd61d399.  So Nikita, was there a specific
> > > >>>>use case that was broken before, or was the code just unclear in
> > > >>>>intentions here?  Thanks!
> > > >>>There was no broken use case that I'm aware of. The change was made as
> > > >>>part of a code improvement series and was meant to address what I
> > > >>>consider to be bad and problematic design. Instead of reverting it
> > > >>>though, how about implementing something similar to what I did in the
> > > >>>main common/spl/spl.c:board_init_r()? You would have a weak function
> > > >>>that will default to the original spl_boot_mode() if not overridden,
> > > >>>and allow the user to define a sequence of boot modes otherwise.
> > > >>The thing is you broke a wanted behavior currently in use. So, the priority is to come back to the previous behavior.
> > > >Could you add a comment indicating that this is wanted behavior that
> > > >has a user, and who the user is?
> > > 
> > > Not sure what you mean.
> > 
> > I mean something like:
> > /* If raw mode fails, try fs mode. Some boards, such as beaglebone black,
> >  * depend on this funcitonality.
> >  */
> 
> But it's not board specific, it's use-case specific.

The comment I proposed does not suggest it's board specific, just that
this specific use case is used by someone.

> instead of shoving both SPL and U-Boot into the correct magic raw
> location, just shove SPL there and let U-Boot itself be in the /boot
> partition.  This is just as applicable on say imx6 as it is on TI parts.

I don't think that's clear enough that this is the purpose of the
missing break statement. It's a little too implicit. What I'm suggesting
is that we make it a bit more explicit, barring a rewrite.

> 
> -- 
> Tom

[U-Boot] [PATCH] spl: if MMCSD_MODE_RAW fails, try MMCSD_MODE_FS, if available

[Guillaume Gardet]

Le 18/02/2016 17:07, Nikita Kiryanov a ?crit :
> On Thu, Feb 18, 2016 at 09:36:01AM -0500, Tom Rini wrote:
>> On Thu, Feb 18, 2016 at 04:25:29PM +0200, Nikita Kiryanov wrote:
>>> On Thu, Feb 18, 2016 at 02:31:08PM +0100, Guillaume Gardet wrote:
>>>>
>>>> Le 18/02/2016 14:07, Nikita Kiryanov a ?crit :
>>>>> On Thu, Feb 18, 2016 at 11:06:32AM +0100, Guillaume Gardet wrote:
>>>>>> Hi Tom, Nikita ,
>>>>>>
>>>>>> Le 18/02/2016 10:19, Nikita Kiryanov a ?crit :
>>>>>>> Hi Tom, Guillaume,
>>>>>>>
>>>>>>> On Wed, Feb 17, 2016 at 03:27:22PM -0500, Tom Rini wrote:
>>>>>>>> On Wed, Feb 17, 2016 at 09:09:27AM +0100, Guillaume GARDET wrote:
>>>>>>>>
>>>>>>>>> Since commit fd61d39970b9901217efc7536d9f3a61b4e1752a:
>>>>>>>>>          spl: mmc: add break statements in spl_mmc_load_image()
>>>>>>>>> RAW and FS boot modes are now exclusive again. So, if MMCSD_MODE_RAW fails, the
>>>>>>>>> board hangs. This patch allows to try MMCSD_MODE_FS then, if available.
>>>>>>>>>
>>>>>>>>> It has been tested on a beaglebone black to boot on an EXT partition.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Guillaume GARDET <guillaume.gardet@free.fr>
>>>>>>>>> Cc: Tom Rini <trini@konsulko.com>
>>>>>>>>> Cc: Nikita Kiryanov <nikita@compulab.co.il>
>>>>>>>>> Cc: Igor Grinberg <grinberg@compulab.co.il>
>>>>>>>>> Cc: Paul Kocialkowski <contact@paulk.fr>
>>>>>>>>> Cc: Pantelis Antoniou <panto@antoniou-consulting.com>
>>>>>>>>> Cc: Simon Glass <sjg@chromium.org>
>>>>>>>>> Cc: Matwey V. Kornilov <matwey.kornilov@gmail.com>
>>>>>>>>>
>>>>>>>>> ---
>>>>>>>>>   common/spl/spl_mmc.c | 2 +-
>>>>>>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>>>
>>>>>>>>> diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c
>>>>>>>>> index c3931c6..2eef0f2 100644
>>>>>>>>> --- a/common/spl/spl_mmc.c
>>>>>>>>> +++ b/common/spl/spl_mmc.c
>>>>>>>>> @@ -284,7 +284,7 @@ int spl_mmc_load_image(u32 boot_device)
>>>>>>>>>   		if (!err)
>>>>>>>>>   			return err;
>>>>>>>>>   #endif
>>>>>>>>> -		break;
>>>>>>>>> +		/* Fall through */
>>>>>>>>>   	case MMCSD_MODE_FS:
>>>>>>>>>   		debug("spl: mmc boot mode: fs\n");
>>>>>>>> This also essentially reverts fd61d399.  So Nikita, was there a specific
>>>>>>>> use case that was broken before, or was the code just unclear in
>>>>>>>> intentions here?  Thanks!
>>>>>>> There was no broken use case that I'm aware of. The change was made as
>>>>>>> part of a code improvement series and was meant to address what I
>>>>>>> consider to be bad and problematic design. Instead of reverting it
>>>>>>> though, how about implementing something similar to what I did in the
>>>>>>> main common/spl/spl.c:board_init_r()? You would have a weak function
>>>>>>> that will default to the original spl_boot_mode() if not overridden,
>>>>>>> and allow the user to define a sequence of boot modes otherwise.
>>>>>> The thing is you broke a wanted behavior currently in use. So, the priority is to come back to the previous behavior.
>>>>> Could you add a comment indicating that this is wanted behavior that
>>>>> has a user, and who the user is?
>>>> Not sure what you mean.
>>> I mean something like:
>>> /* If raw mode fails, try fs mode. Some boards, such as beaglebone black,
>>>   * depend on this funcitonality.
>>>   */
>> But it's not board specific, it's use-case specific.
> The comment I proposed does not suggest it's board specific, just that
> this specific use case is used by someone.
>
>> instead of shoving both SPL and U-Boot into the correct magic raw
>> location, just shove SPL there and let U-Boot itself be in the /boot
>> partition.  This is just as applicable on say imx6 as it is on TI parts.
> I don't think that's clear enough that this is the purpose of the
> missing break statement. It's a little too implicit. What I'm suggesting
> is that we make it a bit more explicit, barring a rewrite.

So, maybe just:
     /* If raw mode fails, try fs mode. */
?


Guillaume

>
>> -- 
>> Tom
>
>

[U-Boot] [PATCH] spl: if MMCSD_MODE_RAW fails, try MMCSD_MODE_FS, if available

[Nikita Kiryanov]

On Thu, Feb 18, 2016 at 05:11:46PM +0100, Guillaume Gardet wrote:
> 
> 
> Le 18/02/2016 17:07, Nikita Kiryanov a ?crit :
> >On Thu, Feb 18, 2016 at 09:36:01AM -0500, Tom Rini wrote:
> >>On Thu, Feb 18, 2016 at 04:25:29PM +0200, Nikita Kiryanov wrote:
> >>>On Thu, Feb 18, 2016 at 02:31:08PM +0100, Guillaume Gardet wrote:
> >>>>
> >>>>Le 18/02/2016 14:07, Nikita Kiryanov a ?crit :
> >>>>>On Thu, Feb 18, 2016 at 11:06:32AM +0100, Guillaume Gardet wrote:
> >>>>>>Hi Tom, Nikita ,
> >>>>>>
> >>>>>>Le 18/02/2016 10:19, Nikita Kiryanov a ?crit :
> >>>>>>>Hi Tom, Guillaume,
> >>>>>>>
> >>>>>>>On Wed, Feb 17, 2016 at 03:27:22PM -0500, Tom Rini wrote:
> >>>>>>>>On Wed, Feb 17, 2016 at 09:09:27AM +0100, Guillaume GARDET wrote:
> >>>>>>>>
> >>>>>>>>>Since commit fd61d39970b9901217efc7536d9f3a61b4e1752a:
> >>>>>>>>>         spl: mmc: add break statements in spl_mmc_load_image()
> >>>>>>>>>RAW and FS boot modes are now exclusive again. So, if MMCSD_MODE_RAW fails, the
> >>>>>>>>>board hangs. This patch allows to try MMCSD_MODE_FS then, if available.
> >>>>>>>>>
> >>>>>>>>>It has been tested on a beaglebone black to boot on an EXT partition.
> >>>>>>>>>
> >>>>>>>>>Signed-off-by: Guillaume GARDET <guillaume.gardet@free.fr>
> >>>>>>>>>Cc: Tom Rini <trini@konsulko.com>
> >>>>>>>>>Cc: Nikita Kiryanov <nikita@compulab.co.il>
> >>>>>>>>>Cc: Igor Grinberg <grinberg@compulab.co.il>
> >>>>>>>>>Cc: Paul Kocialkowski <contact@paulk.fr>
> >>>>>>>>>Cc: Pantelis Antoniou <panto@antoniou-consulting.com>
> >>>>>>>>>Cc: Simon Glass <sjg@chromium.org>
> >>>>>>>>>Cc: Matwey V. Kornilov <matwey.kornilov@gmail.com>
> >>>>>>>>>
> >>>>>>>>>---
> >>>>>>>>>  common/spl/spl_mmc.c | 2 +-
> >>>>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>>>>>
> >>>>>>>>>diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c
> >>>>>>>>>index c3931c6..2eef0f2 100644
> >>>>>>>>>--- a/common/spl/spl_mmc.c
> >>>>>>>>>+++ b/common/spl/spl_mmc.c
> >>>>>>>>>@@ -284,7 +284,7 @@ int spl_mmc_load_image(u32 boot_device)
> >>>>>>>>>  		if (!err)
> >>>>>>>>>  			return err;
> >>>>>>>>>  #endif
> >>>>>>>>>-		break;
> >>>>>>>>>+		/* Fall through */
> >>>>>>>>>  	case MMCSD_MODE_FS:
> >>>>>>>>>  		debug("spl: mmc boot mode: fs\n");
> >>>>>>>>This also essentially reverts fd61d399.  So Nikita, was there a specific
> >>>>>>>>use case that was broken before, or was the code just unclear in
> >>>>>>>>intentions here?  Thanks!
> >>>>>>>There was no broken use case that I'm aware of. The change was made as
> >>>>>>>part of a code improvement series and was meant to address what I
> >>>>>>>consider to be bad and problematic design. Instead of reverting it
> >>>>>>>though, how about implementing something similar to what I did in the
> >>>>>>>main common/spl/spl.c:board_init_r()? You would have a weak function
> >>>>>>>that will default to the original spl_boot_mode() if not overridden,
> >>>>>>>and allow the user to define a sequence of boot modes otherwise.
> >>>>>>The thing is you broke a wanted behavior currently in use. So, the priority is to come back to the previous behavior.
> >>>>>Could you add a comment indicating that this is wanted behavior that
> >>>>>has a user, and who the user is?
> >>>>Not sure what you mean.
> >>>I mean something like:
> >>>/* If raw mode fails, try fs mode. Some boards, such as beaglebone black,
> >>>  * depend on this funcitonality.
> >>>  */
> >>But it's not board specific, it's use-case specific.
> >The comment I proposed does not suggest it's board specific, just that
> >this specific use case is used by someone.
> >
> >>instead of shoving both SPL and U-Boot into the correct magic raw
> >>location, just shove SPL there and let U-Boot itself be in the /boot
> >>partition.  This is just as applicable on say imx6 as it is on TI parts.
> >I don't think that's clear enough that this is the purpose of the
> >missing break statement. It's a little too implicit. What I'm suggesting
> >is that we make it a bit more explicit, barring a rewrite.
> 
> So, maybe just:
>     /* If raw mode fails, try fs mode. */
> ?

That'll work too.

> 
> 
> Guillaume
> 
> >
> >>-- 
> >>Tom
> >
> >
> 

[U-Boot] [PATCH] spl: if MMCSD_MODE_RAW fails, try MMCSD_MODE_FS, if available

[Guillaume Gardet]

Le 18/02/2016 17:38, Nikita Kiryanov a ?crit :
> On Thu, Feb 18, 2016 at 05:11:46PM +0100, Guillaume Gardet wrote:
>>
>> Le 18/02/2016 17:07, Nikita Kiryanov a ?crit :
>>> On Thu, Feb 18, 2016 at 09:36:01AM -0500, Tom Rini wrote:
>>>> On Thu, Feb 18, 2016 at 04:25:29PM +0200, Nikita Kiryanov wrote:
>>>>> On Thu, Feb 18, 2016 at 02:31:08PM +0100, Guillaume Gardet wrote:
>>>>>> Le 18/02/2016 14:07, Nikita Kiryanov a ?crit :
>>>>>>> On Thu, Feb 18, 2016 at 11:06:32AM +0100, Guillaume Gardet wrote:
>>>>>>>> Hi Tom, Nikita ,
>>>>>>>>
>>>>>>>> Le 18/02/2016 10:19, Nikita Kiryanov a ?crit :
>>>>>>>>> Hi Tom, Guillaume,
>>>>>>>>>
>>>>>>>>> On Wed, Feb 17, 2016 at 03:27:22PM -0500, Tom Rini wrote:
>>>>>>>>>> On Wed, Feb 17, 2016 at 09:09:27AM +0100, Guillaume GARDET wrote:
>>>>>>>>>>
>>>>>>>>>>> Since commit fd61d39970b9901217efc7536d9f3a61b4e1752a:
>>>>>>>>>>>          spl: mmc: add break statements in spl_mmc_load_image()
>>>>>>>>>>> RAW and FS boot modes are now exclusive again. So, if MMCSD_MODE_RAW fails, the
>>>>>>>>>>> board hangs. This patch allows to try MMCSD_MODE_FS then, if available.
>>>>>>>>>>>
>>>>>>>>>>> It has been tested on a beaglebone black to boot on an EXT partition.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Guillaume GARDET <guillaume.gardet@free.fr>
>>>>>>>>>>> Cc: Tom Rini <trini@konsulko.com>
>>>>>>>>>>> Cc: Nikita Kiryanov <nikita@compulab.co.il>
>>>>>>>>>>> Cc: Igor Grinberg <grinberg@compulab.co.il>
>>>>>>>>>>> Cc: Paul Kocialkowski <contact@paulk.fr>
>>>>>>>>>>> Cc: Pantelis Antoniou <panto@antoniou-consulting.com>
>>>>>>>>>>> Cc: Simon Glass <sjg@chromium.org>
>>>>>>>>>>> Cc: Matwey V. Kornilov <matwey.kornilov@gmail.com>
>>>>>>>>>>>
>>>>>>>>>>> ---
>>>>>>>>>>>   common/spl/spl_mmc.c | 2 +-
>>>>>>>>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c
>>>>>>>>>>> index c3931c6..2eef0f2 100644
>>>>>>>>>>> --- a/common/spl/spl_mmc.c
>>>>>>>>>>> +++ b/common/spl/spl_mmc.c
>>>>>>>>>>> @@ -284,7 +284,7 @@ int spl_mmc_load_image(u32 boot_device)
>>>>>>>>>>>   		if (!err)
>>>>>>>>>>>   			return err;
>>>>>>>>>>>   #endif
>>>>>>>>>>> -		break;
>>>>>>>>>>> +		/* Fall through */
>>>>>>>>>>>   	case MMCSD_MODE_FS:
>>>>>>>>>>>   		debug("spl: mmc boot mode: fs\n");
>>>>>>>>>> This also essentially reverts fd61d399.  So Nikita, was there a specific
>>>>>>>>>> use case that was broken before, or was the code just unclear in
>>>>>>>>>> intentions here?  Thanks!
>>>>>>>>> There was no broken use case that I'm aware of. The change was made as
>>>>>>>>> part of a code improvement series and was meant to address what I
>>>>>>>>> consider to be bad and problematic design. Instead of reverting it
>>>>>>>>> though, how about implementing something similar to what I did in the
>>>>>>>>> main common/spl/spl.c:board_init_r()? You would have a weak function
>>>>>>>>> that will default to the original spl_boot_mode() if not overridden,
>>>>>>>>> and allow the user to define a sequence of boot modes otherwise.
>>>>>>>> The thing is you broke a wanted behavior currently in use. So, the priority is to come back to the previous behavior.
>>>>>>> Could you add a comment indicating that this is wanted behavior that
>>>>>>> has a user, and who the user is?
>>>>>> Not sure what you mean.
>>>>> I mean something like:
>>>>> /* If raw mode fails, try fs mode. Some boards, such as beaglebone black,
>>>>>   * depend on this funcitonality.
>>>>>   */
>>>> But it's not board specific, it's use-case specific.
>>> The comment I proposed does not suggest it's board specific, just that
>>> this specific use case is used by someone.
>>>
>>>> instead of shoving both SPL and U-Boot into the correct magic raw
>>>> location, just shove SPL there and let U-Boot itself be in the /boot
>>>> partition.  This is just as applicable on say imx6 as it is on TI parts.
>>> I don't think that's clear enough that this is the purpose of the
>>> missing break statement. It's a little too implicit. What I'm suggesting
>>> is that we make it a bit more explicit, barring a rewrite.
>> So, maybe just:
>>      /* If raw mode fails, try fs mode. */
>> ?
> That'll work too.

If Tom is ok, I will send a V2.


Guillaume

[U-Boot] [PATCH] spl: if MMCSD_MODE_RAW fails, try MMCSD_MODE_FS, if available

[Tom Rini]

On Thu, Feb 18, 2016 at 05:42:29PM +0100, Guillaume Gardet wrote:
> 
> 
> Le 18/02/2016 17:38, Nikita Kiryanov a ?crit :
> >On Thu, Feb 18, 2016 at 05:11:46PM +0100, Guillaume Gardet wrote:
> >>
> >>Le 18/02/2016 17:07, Nikita Kiryanov a ?crit :
> >>>On Thu, Feb 18, 2016 at 09:36:01AM -0500, Tom Rini wrote:
> >>>>On Thu, Feb 18, 2016 at 04:25:29PM +0200, Nikita Kiryanov wrote:
> >>>>>On Thu, Feb 18, 2016 at 02:31:08PM +0100, Guillaume Gardet wrote:
> >>>>>>Le 18/02/2016 14:07, Nikita Kiryanov a ?crit :
> >>>>>>>On Thu, Feb 18, 2016 at 11:06:32AM +0100, Guillaume Gardet wrote:
> >>>>>>>>Hi Tom, Nikita ,
> >>>>>>>>
> >>>>>>>>Le 18/02/2016 10:19, Nikita Kiryanov a ?crit :
> >>>>>>>>>Hi Tom, Guillaume,
> >>>>>>>>>
> >>>>>>>>>On Wed, Feb 17, 2016 at 03:27:22PM -0500, Tom Rini wrote:
> >>>>>>>>>>On Wed, Feb 17, 2016 at 09:09:27AM +0100, Guillaume GARDET wrote:
> >>>>>>>>>>
> >>>>>>>>>>>Since commit fd61d39970b9901217efc7536d9f3a61b4e1752a:
> >>>>>>>>>>>         spl: mmc: add break statements in spl_mmc_load_image()
> >>>>>>>>>>>RAW and FS boot modes are now exclusive again. So, if MMCSD_MODE_RAW fails, the
> >>>>>>>>>>>board hangs. This patch allows to try MMCSD_MODE_FS then, if available.
> >>>>>>>>>>>
> >>>>>>>>>>>It has been tested on a beaglebone black to boot on an EXT partition.
> >>>>>>>>>>>
> >>>>>>>>>>>Signed-off-by: Guillaume GARDET <guillaume.gardet@free.fr>
> >>>>>>>>>>>Cc: Tom Rini <trini@konsulko.com>
> >>>>>>>>>>>Cc: Nikita Kiryanov <nikita@compulab.co.il>
> >>>>>>>>>>>Cc: Igor Grinberg <grinberg@compulab.co.il>
> >>>>>>>>>>>Cc: Paul Kocialkowski <contact@paulk.fr>
> >>>>>>>>>>>Cc: Pantelis Antoniou <panto@antoniou-consulting.com>
> >>>>>>>>>>>Cc: Simon Glass <sjg@chromium.org>
> >>>>>>>>>>>Cc: Matwey V. Kornilov <matwey.kornilov@gmail.com>
> >>>>>>>>>>>
> >>>>>>>>>>>---
> >>>>>>>>>>>  common/spl/spl_mmc.c | 2 +-
> >>>>>>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>>>>>>>
> >>>>>>>>>>>diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c
> >>>>>>>>>>>index c3931c6..2eef0f2 100644
> >>>>>>>>>>>--- a/common/spl/spl_mmc.c
> >>>>>>>>>>>+++ b/common/spl/spl_mmc.c
> >>>>>>>>>>>@@ -284,7 +284,7 @@ int spl_mmc_load_image(u32 boot_device)
> >>>>>>>>>>>  		if (!err)
> >>>>>>>>>>>  			return err;
> >>>>>>>>>>>  #endif
> >>>>>>>>>>>-		break;
> >>>>>>>>>>>+		/* Fall through */
> >>>>>>>>>>>  	case MMCSD_MODE_FS:
> >>>>>>>>>>>  		debug("spl: mmc boot mode: fs\n");
> >>>>>>>>>>This also essentially reverts fd61d399.  So Nikita, was there a specific
> >>>>>>>>>>use case that was broken before, or was the code just unclear in
> >>>>>>>>>>intentions here?  Thanks!
> >>>>>>>>>There was no broken use case that I'm aware of. The change was made as
> >>>>>>>>>part of a code improvement series and was meant to address what I
> >>>>>>>>>consider to be bad and problematic design. Instead of reverting it
> >>>>>>>>>though, how about implementing something similar to what I did in the
> >>>>>>>>>main common/spl/spl.c:board_init_r()? You would have a weak function
> >>>>>>>>>that will default to the original spl_boot_mode() if not overridden,
> >>>>>>>>>and allow the user to define a sequence of boot modes otherwise.
> >>>>>>>>The thing is you broke a wanted behavior currently in use. So, the priority is to come back to the previous behavior.
> >>>>>>>Could you add a comment indicating that this is wanted behavior that
> >>>>>>>has a user, and who the user is?
> >>>>>>Not sure what you mean.
> >>>>>I mean something like:
> >>>>>/* If raw mode fails, try fs mode. Some boards, such as beaglebone black,
> >>>>>  * depend on this funcitonality.
> >>>>>  */
> >>>>But it's not board specific, it's use-case specific.
> >>>The comment I proposed does not suggest it's board specific, just that
> >>>this specific use case is used by someone.
> >>>
> >>>>instead of shoving both SPL and U-Boot into the correct magic raw
> >>>>location, just shove SPL there and let U-Boot itself be in the /boot
> >>>>partition.  This is just as applicable on say imx6 as it is on TI parts.
> >>>I don't think that's clear enough that this is the purpose of the
> >>>missing break statement. It's a little too implicit. What I'm suggesting
> >>>is that we make it a bit more explicit, barring a rewrite.
> >>So, maybe just:
> >>     /* If raw mode fails, try fs mode. */
> >>?
> >That'll work too.
> 
> If Tom is ok, I will send a V2.

OK with me, thanks!

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

[U-Boot] [PATCH V2] spl: if MMCSD_MODE_RAW fails, try MMCSD_MODE_FS

[Guillaume GARDET]

Since commit fd61d39970b9901217efc7536d9f3a61b4e1752a:
        spl: mmc: add break statements in spl_mmc_load_image() 
RAW and FS boot modes are now exclusive again. So, if MMCSD_MODE_RAW fails, the
board hangs. This patch allows to try MMCSD_MODE_FS then.

It has been tested on a beaglebone black to boot on an EXT partition.

Signed-off-by: Guillaume GARDET <guillaume.gardet@free.fr>
Cc: Tom Rini <trini@konsulko.com>
Cc: Nikita Kiryanov <nikita@compulab.co.il>
Cc: Igor Grinberg <grinberg@compulab.co.il>
Cc: Paul Kocialkowski <contact@paulk.fr>
Cc: Pantelis Antoniou <panto@antoniou-consulting.com>
Cc: Simon Glass <sjg@chromium.org>
Cc: Matwey V. Kornilov <matwey.kornilov@gmail.com>

---
Changes in V2:
	- Replace "/* Fall through */" comment by: 
	"/* If RAW mode fails, try FS mode. */"

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

diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c
index c3931c6..c27a250 100644
--- a/common/spl/spl_mmc.c
+++ b/common/spl/spl_mmc.c
@@ -284,7 +284,7 @@ int spl_mmc_load_image(u32 boot_device)
 		if (!err)
 			return err;
 #endif
-		break;
+		/* If RAW mode fails, try FS mode. */
 	case MMCSD_MODE_FS:
 		debug("spl: mmc boot mode: fs\n");
 
-- 
1.8.4.5

[U-Boot] [PATCH V2] spl: if MMCSD_MODE_RAW fails, try MMCSD_MODE_FS

[Nikita Kiryanov]

On Thu, Feb 18, 2016 at 06:17:36PM +0100, Guillaume GARDET wrote:
> Since commit fd61d39970b9901217efc7536d9f3a61b4e1752a:
>         spl: mmc: add break statements in spl_mmc_load_image() 
> RAW and FS boot modes are now exclusive again. So, if MMCSD_MODE_RAW fails, the
> board hangs. This patch allows to try MMCSD_MODE_FS then.
> 
> It has been tested on a beaglebone black to boot on an EXT partition.

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

> 
> Signed-off-by: Guillaume GARDET <guillaume.gardet@free.fr>
> Cc: Tom Rini <trini@konsulko.com>
> Cc: Nikita Kiryanov <nikita@compulab.co.il>
> Cc: Igor Grinberg <grinberg@compulab.co.il>
> Cc: Paul Kocialkowski <contact@paulk.fr>
> Cc: Pantelis Antoniou <panto@antoniou-consulting.com>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Matwey V. Kornilov <matwey.kornilov@gmail.com>
> 
> ---
> Changes in V2:
> 	- Replace "/* Fall through */" comment by: 
> 	"/* If RAW mode fails, try FS mode. */"
> 
>  common/spl/spl_mmc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c
> index c3931c6..c27a250 100644
> --- a/common/spl/spl_mmc.c
> +++ b/common/spl/spl_mmc.c
> @@ -284,7 +284,7 @@ int spl_mmc_load_image(u32 boot_device)
>  		if (!err)
>  			return err;
>  #endif
> -		break;
> +		/* If RAW mode fails, try FS mode. */
>  	case MMCSD_MODE_FS:
>  		debug("spl: mmc boot mode: fs\n");
>  
> -- 
> 1.8.4.5
> 

[U-Boot] [U-Boot, V2] spl: if MMCSD_MODE_RAW fails, try MMCSD_MODE_FS

[Tom Rini]

On Thu, Feb 18, 2016 at 06:17:36PM +0100, Guillaume GARDET wrote:

> Since commit fd61d39970b9901217efc7536d9f3a61b4e1752a:
>         spl: mmc: add break statements in spl_mmc_load_image() 
> RAW and FS boot modes are now exclusive again. So, if MMCSD_MODE_RAW fails, the
> board hangs. This patch allows to try MMCSD_MODE_FS then.
> 
> It has been tested on a beaglebone black to boot on an EXT partition.
> 
> Signed-off-by: Guillaume GARDET <guillaume.gardet@free.fr>
> Cc: Tom Rini <trini@konsulko.com>
> Cc: Nikita Kiryanov <nikita@compulab.co.il>
> Cc: Igor Grinberg <grinberg@compulab.co.il>
> Cc: Paul Kocialkowski <contact@paulk.fr>
> Cc: Pantelis Antoniou <panto@antoniou-consulting.com>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Matwey V. Kornilov <matwey.kornilov@gmail.com>
> Acked-by: Nikita Kiryanov <nikita@compulab.co.il>

Applied to u-boot/master, thanks!

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

[U-Boot] [PATCH] spl: if MMCSD_MODE_RAW fails, try MMCSD_MODE_FS, if available

[Guillaume GARDET]

In SPL MMC, boot modes are exclusive. So, if MMCSD_MODE_RAW fails, the board hangs. This patch allows to
try MMCSD_MODE_FS then, if available.

It has been tested on a pandaboard (rev. A3).

Signed-off-by: Guillaume GARDET <guillaume.gardet@free.fr>
Cc: Tom Rini <trini@ti.com>
---
 common/spl/spl_mmc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c
index ee71f79..2c34b75 100644
--- a/common/spl/spl_mmc.c
+++ b/common/spl/spl_mmc.c
@@ -101,7 +101,8 @@ void spl_mmc_load_image(void)
 		err = mmc_load_image_raw(mmc,
 			CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR);
 #if defined(CONFIG_SPL_FAT_SUPPORT) || defined(CONFIG_SPL_EXT_SUPPORT)
-	} else if (boot_mode == MMCSD_MODE_FS) {
+	}
+	if (err || boot_mode == MMCSD_MODE_FS) {
 		debug("boot mode - FS\n");
 #ifdef CONFIG_SPL_FAT_SUPPORT
 #ifdef CONFIG_SPL_OS_BOOT
-- 
1.8.4.5

[U-Boot] [PATCH] spl: if MMCSD_MODE_RAW fails, try MMCSD_MODE_FS, if available

[Guillaume Gardet]

Ping.
Just a friendly reminder.

Guillaume

Le 18/11/2014 10:44, Guillaume GARDET a ?crit :
> In SPL MMC, boot modes are exclusive. So, if MMCSD_MODE_RAW fails, the board hangs. This patch allows to
> try MMCSD_MODE_FS then, if available.
>
> It has been tested on a pandaboard (rev. A3).
>
> Signed-off-by: Guillaume GARDET <guillaume.gardet@free.fr>
> Cc: Tom Rini <trini@ti.com>
> ---
>   common/spl/spl_mmc.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c
> index ee71f79..2c34b75 100644
> --- a/common/spl/spl_mmc.c
> +++ b/common/spl/spl_mmc.c
> @@ -101,7 +101,8 @@ void spl_mmc_load_image(void)
>   		err = mmc_load_image_raw(mmc,
>   			CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR);
>   #if defined(CONFIG_SPL_FAT_SUPPORT) || defined(CONFIG_SPL_EXT_SUPPORT)
> -	} else if (boot_mode == MMCSD_MODE_FS) {
> +	}
> +	if (err || boot_mode == MMCSD_MODE_FS) {
>   		debug("boot mode - FS\n");
>   #ifdef CONFIG_SPL_FAT_SUPPORT
>   #ifdef CONFIG_SPL_OS_BOOT

[U-Boot] [PATCH] spl: if MMCSD_MODE_RAW fails, try MMCSD_MODE_FS, if available

[Robert Nelson]

On Tue, Nov 18, 2014 at 3:44 AM, Guillaume GARDET
<guillaume.gardet@free.fr> wrote:
> In SPL MMC, boot modes are exclusive. So, if MMCSD_MODE_RAW fails, the board hangs. This patch allows to
> try MMCSD_MODE_FS then, if available.
>
> It has been tested on a pandaboard (rev. A3).

HI Guillaume,

What mode did you test this is? (RAW or FS)

In Raw Mode with the omap5_uevm & beaglebone black, i've had to revert
this. (I'm using RAW mode by default)

U-Boot SPL 2015.01-rc3-dirty (Dec 08 2014 - 20:04:01)
OMAP5432 ES2.0
SPL: Please implement spl_start_uboot() for your board
SPL: Direct Linux boot not active!
spl: wrong MMC boot mode
### ERROR ### Please RESET the board ###

Disk setup, I'm using

sudo dd if=/dev/zero of=${DISK} bs=1M count=10

sudo dd if=./u-boot/MLO of=${DISK} count=1 seek=1 conv=notrunc bs=128k
sudo dd if=./u-boot/u-boot.img of=${DISK} count=2 seek=1 conv=notrunc bs=384k

sudo sfdisk --in-order --Linux --unit M ${DISK} <<-__EOF__
1,,0x83,*
__EOF__

Regards,

-- 
Robert Nelson
http://www.rcn-ee.com/

[U-Boot] [PATCH] spl: if MMCSD_MODE_RAW fails, try MMCSD_MODE_FS, if available

[Guillaume Gardet]

Hi Robert,


Le 12/12/2014 22:49, Robert Nelson a ?crit :
> On Tue, Nov 18, 2014 at 3:44 AM, Guillaume GARDET
> <guillaume.gardet@free.fr> wrote:
>> In SPL MMC, boot modes are exclusive. So, if MMCSD_MODE_RAW fails, the board hangs. This patch allows to
>> try MMCSD_MODE_FS then, if available.
>>
>> It has been tested on a pandaboard (rev. A3).
> HI Guillaume,
>
> What mode did you test this is? (RAW or FS)

I did test for FS for sure, but not sure about raw mode.

>
> In Raw Mode with the omap5_uevm & beaglebone black, i've had to revert
> this. (I'm using RAW mode by default)
>
> U-Boot SPL 2015.01-rc3-dirty (Dec 08 2014 - 20:04:01)
> OMAP5432 ES2.0
> SPL: Please implement spl_start_uboot() for your board
> SPL: Direct Linux boot not active!
> spl: wrong MMC boot mode
> ### ERROR ### Please RESET the board ###

Ok, I think I found the problem. Could you test the following patch, please?
http://guillaume.gardet.free.fr/u-boot/0001-spl-mmc-Fix-raw-boot-mode-related-to-commit.patch

If this patch fix your boot problem, I will send it ASAP.


Guillaume

[U-Boot] [PATCH] spl: if MMCSD_MODE_RAW fails, try MMCSD_MODE_FS, if available

[Guillaume Gardet]

Le 15/12/2014 09:43, Guillaume Gardet a ?crit :
> Hi Robert,
>
>
> Le 12/12/2014 22:49, Robert Nelson a ?crit :
>> On Tue, Nov 18, 2014 at 3:44 AM, Guillaume GARDET
>> <guillaume.gardet@free.fr> wrote:
>>> In SPL MMC, boot modes are exclusive. So, if MMCSD_MODE_RAW fails, the board hangs. This patch allows to
>>> try MMCSD_MODE_FS then, if available.
>>>
>>> It has been tested on a pandaboard (rev. A3).
>> HI Guillaume,
>>
>> What mode did you test this is? (RAW or FS)
>
> I did test for FS for sure, but not sure about raw mode.
>
>>
>> In Raw Mode with the omap5_uevm & beaglebone black, i've had to revert
>> this. (I'm using RAW mode by default)
>>
>> U-Boot SPL 2015.01-rc3-dirty (Dec 08 2014 - 20:04:01)
>> OMAP5432 ES2.0
>> SPL: Please implement spl_start_uboot() for your board
>> SPL: Direct Linux boot not active!
>> spl: wrong MMC boot mode
>> ### ERROR ### Please RESET the board ###
>
> Ok, I think I found the problem. Could you test the following patch, please?
> http://guillaume.gardet.free.fr/u-boot/0001-spl-mmc-Fix-raw-boot-mode-related-to-commit.patch
>
> If this patch fix your boot problem, I will send it ASAP.

Just updated the patch with a  better fix:
http://guillaume.gardet.free.fr/u-boot/0001-spl-mmc-Fix-raw-boot-mode-related-to-commit.patch


Guillaume

[U-Boot] [PATCH] spl: if MMCSD_MODE_RAW fails, try MMCSD_MODE_FS, if available

[Guillaume Gardet]

Le 15/12/2014 10:01, Guillaume Gardet a ?crit :
> Le 15/12/2014 09:43, Guillaume Gardet a ?crit :
>> Hi Robert,
>>
>>
>> Le 12/12/2014 22:49, Robert Nelson a ?crit :
>>> On Tue, Nov 18, 2014 at 3:44 AM, Guillaume GARDET
>>> <guillaume.gardet@free.fr> wrote:
>>>> In SPL MMC, boot modes are exclusive. So, if MMCSD_MODE_RAW fails, the board hangs. This patch allows to
>>>> try MMCSD_MODE_FS then, if available.
>>>>
>>>> It has been tested on a pandaboard (rev. A3).
>>> HI Guillaume,
>>>
>>> What mode did you test this is? (RAW or FS)
>>
>> I did test for FS for sure, but not sure about raw mode.
>>
>>>
>>> In Raw Mode with the omap5_uevm & beaglebone black, i've had to revert
>>> this. (I'm using RAW mode by default)
>>>
>>> U-Boot SPL 2015.01-rc3-dirty (Dec 08 2014 - 20:04:01)
>>> OMAP5432 ES2.0
>>> SPL: Please implement spl_start_uboot() for your board
>>> SPL: Direct Linux boot not active!
>>> spl: wrong MMC boot mode
>>> ### ERROR ### Please RESET the board ###
>>
>> Ok, I think I found the problem. Could you test the following patch, please?
>> http://guillaume.gardet.free.fr/u-boot/0001-spl-mmc-Fix-raw-boot-mode-related-to-commit.patch
>>
>> If this patch fix your boot problem, I will send it ASAP.
>
> Just updated the patch with a  better fix:
> http://guillaume.gardet.free.fr/u-boot/0001-spl-mmc-Fix-raw-boot-mode-related-to-commit.patch

As I was able to test this patch, I just sent it:
https://patchwork.ozlabs.org/patch/421858/


Guillaume