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

2018-01-04 Thread Stefano Babic 
On 04/01/2018 13:05, Eran Matityahu wrote:
> On Thu, Jan 4, 2018 at 2:02 PM, Uri Mashiach
>  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  wrote:
>
> On 04/01/2018 11:11, Eran Matityahu wrote:
>>
>> On Thu, Jan 4, 2018 at 12:02 PM, Eran Matityahu 
>> wrote:
>>>
>>> On Thu, Jan 4, 2018 at 11:14 AM, Stefano Babic  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 
>>> ---
>>>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.
>

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

2018-01-04 Thread Eran Matityahu 
On Thu, Jan 4, 2018 at 2:02 PM, Uri Mashiach
 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  wrote:

 On 04/01/2018 11:11, Eran Matityahu wrote:
>
> On Thu, Jan 4, 2018 at 12:02 PM, Eran Matityahu 
> wrote:
>>
>> On Thu, Jan 4, 2018 at 11:14 AM, Stefano Babic  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 
>> ---
>>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

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

2018-01-04 Thread 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  wrote:

On 04/01/2018 11:11, Eran Matityahu wrote:

On Thu, Jan 4, 2018 at 12:02 PM, Eran Matityahu  wrote:

On Thu, Jan 4, 2018 at 11:14 AM, Stefano Babic  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 
---
   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

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

2018-01-04 Thread Stefano Babic 
On 04/01/2018 11:56, Eran Matityahu wrote:
> On Thu, Jan 4, 2018 at 12:42 PM, Stefano Babic  wrote:
>> On 04/01/2018 11:11, Eran Matityahu wrote:
>>> On Thu, Jan 4, 2018 at 12:02 PM, Eran Matityahu  
>>> wrote:
 On Thu, Jan 4, 2018 at 11:14 AM, Stefano Babic  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 
 ---
   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

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

2018-01-04 Thread Eran Matityahu 
On Thu, Jan 4, 2018 at 12:42 PM, Stefano Babic  wrote:
> On 04/01/2018 11:11, Eran Matityahu wrote:
>> On Thu, Jan 4, 2018 at 12:02 PM, Eran Matityahu  wrote:
>>> On Thu, Jan 4, 2018 at 11:14 AM, Stefano Babic  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 
>>> ---
>>>   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_*.

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

2018-01-04 Thread Stefano Babic 
On 04/01/2018 11:11, Eran Matityahu wrote:
> On Thu, Jan 4, 2018 at 12:02 PM, Eran Matityahu  wrote:
>> On Thu, Jan 4, 2018 at 11:14 AM, Stefano Babic  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 
>> ---
>>   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.
> 

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
=

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

2018-01-04 Thread Eran Matityahu 
On Thu, Jan 4, 2018 at 12:02 PM, Eran Matityahu  wrote:
> On Thu, Jan 4, 2018 at 11:14 AM, Stefano Babic  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 
> ---
>   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 (and this patch also), is the
preferred way,
since usually you'll only need one MMC device in SPL.

Regards,
Eran

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

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

2018-01-04 Thread Eran Matityahu 
On Thu, Jan 4, 2018 at 11:14 AM, Stefano Babic  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 
 ---
   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.

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

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

2018-01-04 Thread Stefano Babic 
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 
>>> ---
>>>   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

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

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

2018-01-04 Thread Uri Mashiach 



On 01/03/2018 03:58 PM, 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.



A problematic scenario is a detection, by the boot ROM, of the SPL image 
at MMC2.
If the U-Boot image is located at MMC2, the boot sequence will be 
terminated.






Signed-off-by: Eran Matityahu 
---
   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:


[...]
--
Regards,
Uri
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

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

2018-01-03 Thread Eran Matityahu 
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 
>> ---
>>   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:
>>
>
> --
> Regards,
> Uri

Regards,
Eran
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

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

2018-01-03 Thread Uri Mashiach 

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?



Signed-off-by: Eran Matityahu 
---
  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:



--
Regards,
Uri
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot