On Thu, Jan 4, 2018 at 2:02 PM, Uri Mashiach uri.mashiach@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@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@variscite.com wrote:
On Thu, Jan 4, 2018 at 11:14 AM, Stefano Babic sbabic@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@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().
Regards, Eran
-- Regards, Uri