From mboxrd@z Thu Jan 1 00:00:00 1970 From: Guillaume Gardet Date: Thu, 18 Feb 2016 17:42:29 +0100 Subject: [U-Boot] [PATCH] spl: if MMCSD_MODE_RAW fails, try MMCSD_MODE_FS, if available In-Reply-To: <20160218163834.GA25819@skynet> References: <1455696567-11977-1-git-send-email-guillaume.gardet@free.fr> <20160217202722.GW23166@bill-the-cat> <20160218091925.GA15017@skynet> <56C597A8.9000408@free.fr> <20160218130754.GA23954@skynet> <56C5C79C.2080107@free.fr> <20160218142529.GA24624@skynet> <20160218143601.GY23166@bill-the-cat> <20160218160737.GA25076@skynet> <56C5ED42.8070102@free.fr> <20160218163834.GA25819@skynet> Message-ID: <56C5F475.7070707@free.fr> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 >>>>>>>>>>> Cc: Tom Rini >>>>>>>>>>> Cc: Nikita Kiryanov >>>>>>>>>>> Cc: Igor Grinberg >>>>>>>>>>> Cc: Paul Kocialkowski >>>>>>>>>>> Cc: Pantelis Antoniou >>>>>>>>>>> Cc: Simon Glass >>>>>>>>>>> Cc: Matwey V. Kornilov >>>>>>>>>>> >>>>>>>>>>> --- >>>>>>>>>>> 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