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

[Eran Matityahu]

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_*.
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.

Regards,
Eran
>
> 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
> =====================================================================