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!