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

[Uri Mashiach]

On 01/04/2018 01:37 PM, Stefano Babic wrote:
> 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 ?

The cl-som-imx7 board doesn't boot from MMC3, so the patch doesn't 
influence the board.

I prefer the approach of using the spl_boot_list instead of "loosing" 
the boot instance, that might be used in other future boards.

-- 
Regards,
Uri