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

Thu Jan 4 12:16:13 UTC 2018

On 04/01/2018 13:05, Eran Matityahu wrote:
> On Thu, Jan 4, 2018 at 2:02 PM, Uri Mashiach
> <uri.mashiach at compulab.co.il> wrote:
>>
>>
>> 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.
> 
> You do not actually lose the boot instance.
> You can always use get_boot_device().

Yes, I see the same (and this is the major difference with i.MX6).
Boards requiring to do internal logic based on the booting device can
still run get_boot_device() and we are not missing anything. As result
of this discussion and taking into account that we are neither losing
features nor breaking boards, I agree the patch can be merged.

Best regards,
Stefano Babic

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