[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 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 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
>>>