[U-Boot] [PATCH 1/2] imx7: spl: Use SPL boot device MMC1 for all of the SOCs MMC/SD boot devices

[Stefano Babic]

On 04/01/2018 11:56, Eran Matityahu wrote:
> On Thu, Jan 4, 2018 at 12:42 PM, Stefano Babic <sbabic at denx.de> wrote:
>> On 04/01/2018 11:11, Eran Matityahu wrote:
>>> On Thu, Jan 4, 2018 at 12:02 PM, Eran Matityahu <eran.m at variscite.com> wrote:
>>>> On Thu, Jan 4, 2018 at 11:14 AM, Stefano Babic <sbabic at denx.de> wrote:
>>>>> Hi Eran,
>>>>>
>>>>> On 03/01/2018 14:58, Eran Matityahu wrote:
>>>>>> Hi Uri.
>>>>>>
>>>>>>> Hello Eran,
>>>>>>>
>>>>>>> On 01/03/2018 12:53 PM, Eran Matityahu wrote:
>>>>>>>>
>>>>>>>> Use only one SPL MMC device, similarly to the iMX6 code
>>>>>>>
>>>>>>>
>>>>>>> What is the reason for not using MMC2?
>>>>>>
>>>>>> The reason is so that you won't have to initialize more than one MMC
>>>>>> device in SPL.
>>>>>> Also, to be consistent with the iMX6 SPL code.
>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> Signed-off-by: Eran Matityahu <eran.m at variscite.com>
>>>>>>>> ---
>>>>>>>>   arch/arm/mach-imx/spl.c | 3 +--
>>>>>>>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/arch/arm/mach-imx/spl.c b/arch/arm/mach-imx/spl.c
>>>>>>>> index d0d1b73aa6..6b5bd8990c 100644
>>>>>>>> --- a/arch/arm/mach-imx/spl.c
>>>>>>>> +++ b/arch/arm/mach-imx/spl.c
>>>>>>>> @@ -106,10 +106,9 @@ u32 spl_boot_device(void)
>>>>>>>>         switch (boot_device_spl) {
>>>>>>>>         case SD1_BOOT:
>>>>>>>>         case MMC1_BOOT:
>>>>>>>> -               return BOOT_DEVICE_MMC1;
>>>>>>>>         case SD2_BOOT:
>>>>>>>>         case MMC2_BOOT:
>>>>>>>> -               return BOOT_DEVICE_MMC2;
>>>>>>>> +               return BOOT_DEVICE_MMC1;
>>>>>>>>         case SPI_NOR_BOOT:
>>>>>>>>                 return BOOT_DEVICE_SPI;
>>>>>>>>         default:
>>>>>
>>>>> The reason to have spl_boot_device() is not to initialize more as one
>>>>> MMC device, but to find which storage contains the next image to be
>>>>> started (u-boot.img). This is generally (but not in all projects) the
>>>>> same storage from where the BootROM has loaded SPL.
>>>>>
>>>>> According to this, this patch seems wrong. If SPL / u-boot.img are
>>>>> stored on MMC2 (and maybe MMC2 is the only MMC device for the board),
>>>>> your patch breaks booting.
>>>>>
>>>>> If you have special case, you can write a board_boot_order() in your
>>>>> board code to overwrite the behavior.
>>>>>
>>>>> Best regards,
>>>>> Stefano Babic
>>>>
>>>> The iMX6 spl_boot_device() doesn't even check which MMC device the
>>>> BootROM has loaded SPL from. It just returns BOOT_DEVICE_MMC1
>>>> in case the boot device was any MMC/SD device, and leaves it to the
>>>> board code to detect the exact device and init the appropriate device
>>>> with the next image (u-boot,img), accordingly.
>>>> My suggestion is to do the same here.
>>>>
>>>> In my iMX7 board, I can boot from MMC1 (SD card) and MMC3 (eMMC),
>>>> but let's say it's MMC2 in sake of this explanation.
>>>> Without this patch, in order to boot from MMC2 (with both SPL and u-boot.img
>>>> on MMC2), I have to initialize both MMC1 and MMC2 devices because SPL
>>>> loops on all devices until it finds a match, and it halts if the first
>>>> device is not
>>>> initialized.
>>>>
>>>> With this patch I can use get_boot_device() inside board_mmc_init() and
>>>> only initialize the MMC device I want to load the next image from (usually
>>>> the same device).
>>>>
>>>> I know I can approach it differently and change the spl_boot_list[0] device to
>>>> BOOT_DEVICE_MMC1 in board_boot_order(), but I figured the behaviour
>>>> should be the same for iMX6 and iMX7.
>>>> If you think the correct way is to return BOOT_DEVICE_MMC2, then we
>>>> should probably also add a BOOT_DEVICE_MMC3 definition, and also change
>>>> the iMX6 code to do the same.
>>>>
>>> By the way, in my opinion, the iMX6 way
>>
>> The imx6 way is the right way to do - rather, i.MX7 does not follow the
>> same approach.
>>
>> In i.MX6 code, spl_boot_device() returns the type of boot device instead
>> of the instance of the peripheral. In fact. it returns a imx6_bmode
>> (let's away the serial rom, it is messy to detect).
>>
>> A following board_boot_order() then choose which is the instance for
>> that detected type, and this is then used to load u-boot.img. This is,
>> at the end, board specific. Even if in most cases, u-boot.img resides on
>> the same storage as SPL, there are cases where this is not true.
>>
>> And just a single MMC is instantiated in SPL - this is decided inside
>> board code. See for example pcm058.c (but there are plenty of other
>> examples), just a single MMC is initialized by SPL.
>>
>> On i.MX7, the same approach was not followed. A single spl_boot_device()
>> tries to do all.
>>
>> I agree that i.MX6 approach is better, and I will glad if you would move
>> i.MX7 to have the same behavior as i.MX6.
>>
>>
>>> (and this patch also),
>>
>> No, even if it does not depend from the patch - see above.
>>
>>> is the
>>> preferred way,
>>> since usually you'll only need one MMC device in SPL.
>>>
> We are saying the same thing.

:-)

> Except, you are wrong in one little thing: the i.MX6 version of
> spl_boot_device() doesn't return an imx6_bmode. It detects the
> imx6_bmode and returns a BOOT_DEVICE_*.

True, but this is used as "type" for i.MX6, it is a real device for
i.MX7 (get_boot_device() in arch/arm/mach-imx/mx7/soc.c). This is also
due to differences in SOC, I admit.

> In case of an MMC/.SD boot mode it returns BOOT_DEVICE_MMC1.
> This patch indeed makes the i.MX7 behaviour the same as i.MX6.

The thing is if this patch breaks some boards. As far as I can see,
there is just 2 i.MX7 with SPL support: colibri_imx7 (it has just
USDHC1, no problem) and cl-som-imx7 that initialize MMC3 (but I do not
know if it boots from it, in any case it is not MMC2). Uri, you
commented this patch and you are the maintainer for cl-som-imx7. Do you
see any problem with that ?

Best regards,
Stefano

-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================