On 04/01/2018 13:05, Eran Matityahu wrote: > On Thu, Jan 4, 2018 at 2:02 PM, Uri Mashiach > <uri.mashi...@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 <sba...@denx.de> wrote: >>>>> >>>>> On 04/01/2018 11:11, Eran Matityahu wrote: >>>>>> >>>>>> On Thu, Jan 4, 2018 at 12:02 PM, Eran Matityahu <era...@variscite.com> >>>>>> wrote: >>>>>>> >>>>>>> On Thu, Jan 4, 2018 at 11:14 AM, Stefano Babic <sba...@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 <era...@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: sba...@denx.de ===================================================================== _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot