[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 at free.fr>
>>>>>>>>>>> Cc: Tom Rini <trini at konsulko.com>
>>>>>>>>>>> Cc: Nikita Kiryanov <nikita at compulab.co.il>
>>>>>>>>>>> Cc: Igor Grinberg <grinberg at compulab.co.il>
>>>>>>>>>>> Cc: Paul Kocialkowski <contact at paulk.fr>
>>>>>>>>>>> Cc: Pantelis Antoniou <panto at antoniou-consulting.com>
>>>>>>>>>>> Cc: Simon Glass <sjg at chromium.org>
>>>>>>>>>>> Cc: Matwey V. Kornilov <matwey.kornilov at 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