All of lore.kernel.org
 help / color / mirror / Atom feed
* [U-Boot] [PATCH] imx: Introduce CONFIG_SPL_FORCE_MMC_BOOT to force MMC boot on falcon mode
@ 2019-09-03 21:43 Lukasz Majewski
  2019-09-04  4:45 ` Heiko Schocher
  0 siblings, 1 reply; 12+ messages in thread
From: Lukasz Majewski @ 2019-09-03 21:43 UTC (permalink / raw)
  To: u-boot

This change tries to fix the following problem:

- The board boots (to be more precise - ROM loads SPL) from a slow SPI-NOR
  memory.
  As a result the spl_boot_device() will return SPI-NOR as a boot device
  (which is correct).

- The problem is that in 'falcon boot' the eMMC is used as a boot medium to
  load kernel from its partition.
  Calling spl_boot_device() will break things as it returns SPI-NOR device.

To fix this issue the new CONFIG_SPL_FORCE_MMC_BOOT Kconfig flag is
introduced to handle this special use case. By default it is not defined,
so there is no change in the legacy code flow.

Signed-off-by: Lukasz Majewski <lukma@denx.de>


---

This patch is a follow up of previous discussion/fix:
https://patchwork.ozlabs.org/patch/796237/

Travis-CI:
https://travis-ci.org/lmajewski/u-boot-dfu/builds/580272414
---
 arch/arm/mach-imx/spl.c | 11 +++++++++++
 common/spl/Kconfig      |  7 +++++++
 2 files changed, 18 insertions(+)

diff --git a/arch/arm/mach-imx/spl.c b/arch/arm/mach-imx/spl.c
index 1f230aca3397..daa3d8f7ed94 100644
--- a/arch/arm/mach-imx/spl.c
+++ b/arch/arm/mach-imx/spl.c
@@ -178,7 +178,18 @@ int g_dnl_bind_fixup(struct usb_device_descriptor *dev, const char *name)
 /* called from spl_mmc to see type of boot mode for storage (RAW or FAT) */
 u32 spl_boot_mode(const u32 boot_device)
 {
+/*
+ * When CONFIG_SPL_FORCE_MMC_BOOT is defined the 'boot_device' is used
+ * unconditionally to decide about device to use for booting.
+ * This is crucial for falcon boot mode, when board boots up (i.e. ROM
+ * loads SPL) from slow SPI-NOR memory and afterwards the SPL's 'falcon' boot
+ * mode is used to load Linux OS from eMMC partition.
+ */
+#ifdef CONFIG_SPL_FORCE_MMC_BOOT
+	switch (boot_device) {
+#else
 	switch (spl_boot_device()) {
+#endif
 	/* for MMC return either RAW or FAT mode */
 	case BOOT_DEVICE_MMC1:
 	case BOOT_DEVICE_MMC2:
diff --git a/common/spl/Kconfig b/common/spl/Kconfig
index f467eca2be72..3460beb62d59 100644
--- a/common/spl/Kconfig
+++ b/common/spl/Kconfig
@@ -607,6 +607,13 @@ config SPL_MMC_SUPPORT
 	  this option to build the drivers in drivers/mmc as part of an SPL
 	  build.
 
+config SPL_FORCE_MMC_BOOT
+	bool "Force SPL's falcon boot from MMC"
+	depends on SPL_MMC_SUPPORT
+	default n
+	help
+	  Force SPL's falcon boot to use MMC device for Linux kernel booting.
+
 config SPL_MMC_TINY
 	bool "Tiny MMC framework in SPL"
 	depends on SPL_MMC_SUPPORT
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [U-Boot] [PATCH] imx: Introduce CONFIG_SPL_FORCE_MMC_BOOT to force MMC boot on falcon mode
  2019-09-03 21:43 [U-Boot] [PATCH] imx: Introduce CONFIG_SPL_FORCE_MMC_BOOT to force MMC boot on falcon mode Lukasz Majewski
@ 2019-09-04  4:45 ` Heiko Schocher
  2019-09-04  8:46   ` Lukasz Majewski
  0 siblings, 1 reply; 12+ messages in thread
From: Heiko Schocher @ 2019-09-04  4:45 UTC (permalink / raw)
  To: u-boot

Hello Lukasz,

added Stefano to cc as he is the imx custodian.

Am 03.09.2019 um 23:43 schrieb Lukasz Majewski:
> This change tries to fix the following problem:
> 
> - The board boots (to be more precise - ROM loads SPL) from a slow SPI-NOR
>    memory.
>    As a result the spl_boot_device() will return SPI-NOR as a boot device
>    (which is correct).
> 
> - The problem is that in 'falcon boot' the eMMC is used as a boot medium to
>    load kernel from its partition.
>    Calling spl_boot_device() will break things as it returns SPI-NOR device.
> 
> To fix this issue the new CONFIG_SPL_FORCE_MMC_BOOT Kconfig flag is
> introduced to handle this special use case. By default it is not defined,
> so there is no change in the legacy code flow.
> 
> Signed-off-by: Lukasz Majewski <lukma@denx.de>
> 
> 
> ---
> 
> This patch is a follow up of previous discussion/fix:
> https://patchwork.ozlabs.org/patch/796237/
> 
> Travis-CI:
> https://travis-ci.org/lmajewski/u-boot-dfu/builds/580272414
> ---
>   arch/arm/mach-imx/spl.c | 11 +++++++++++
>   common/spl/Kconfig      |  7 +++++++
>   2 files changed, 18 insertions(+)

I have just similiar change for an imx6ull based board ...
just antoher usecase: In factory empty board boots into USB SDP mode,
and SPL gets loaded with usb_loader ... SPL detects a sd card (there
is no sd card slot mounted, mmc boot is disabled! They attach only one
for installing software) and boots U-Boot and system from sd card.
Than all software gets installed fully automated with the help of
swupdate ...

> diff --git a/arch/arm/mach-imx/spl.c b/arch/arm/mach-imx/spl.c
> index 1f230aca3397..daa3d8f7ed94 100644
> --- a/arch/arm/mach-imx/spl.c
> +++ b/arch/arm/mach-imx/spl.c
> @@ -178,7 +178,18 @@ int g_dnl_bind_fixup(struct usb_device_descriptor *dev, const char *name)
>   /* called from spl_mmc to see type of boot mode for storage (RAW or FAT) */
>   u32 spl_boot_mode(const u32 boot_device)
>   {
> +/*
> + * When CONFIG_SPL_FORCE_MMC_BOOT is defined the 'boot_device' is used
> + * unconditionally to decide about device to use for booting.
> + * This is crucial for falcon boot mode, when board boots up (i.e. ROM
> + * loads SPL) from slow SPI-NOR memory and afterwards the SPL's 'falcon' boot
> + * mode is used to load Linux OS from eMMC partition.
> + */
> +#ifdef CONFIG_SPL_FORCE_MMC_BOOT
> +	switch (boot_device) {
> +#else
>   	switch (spl_boot_device()) {
> +#endif

Just dummy question .. couldn;t we switch to use always boot_device?

In my case I had a board specific:

void board_boot_order(u32 *spl_boot_list)
{
         spl_boot_list[0] = BOOT_DEVICE_MMC1;
         spl_boot_list[1] = BOOT_DEVICE_SPI;
         spl_boot_list[2] = BOOT_DEVICE_BOARD;
         spl_boot_list[3] = BOOT_DEVICE_NONE;
}

which leads that BOOT_DEVICE_MMC1 is passed to spl_boot_mode()

If MMC1 is not found, it tries to boot U-Boot from SPI, and if this is
empty, as fallback board go into USB SDP mode.

The weak function of board_boot_order is:

__weak void board_boot_order(u32 *spl_boot_list)
{
         spl_boot_list[0] = spl_boot_device();
}

which is exactly what we pass to spl_boot_mode() ... so instead calling
spl_boot_device() twice we can use the passed boot_device ... and
save the ifdef and new Kconfig option.

But may I oversee something ?

>   	/* for MMC return either RAW or FAT mode */
>   	case BOOT_DEVICE_MMC1:
>   	case BOOT_DEVICE_MMC2:
> diff --git a/common/spl/Kconfig b/common/spl/Kconfig
> index f467eca2be72..3460beb62d59 100644
> --- a/common/spl/Kconfig
> +++ b/common/spl/Kconfig
> @@ -607,6 +607,13 @@ config SPL_MMC_SUPPORT
>   	  this option to build the drivers in drivers/mmc as part of an SPL
>   	  build.
>   
> +config SPL_FORCE_MMC_BOOT
> +	bool "Force SPL's falcon boot from MMC"
> +	depends on SPL_MMC_SUPPORT
> +	default n
> +	help
> +	  Force SPL's falcon boot to use MMC device for Linux kernel booting.

Hmm... falcon boot is just one use case. I would drop "falcon boot"
It just to force boot from MMC.

> +
>   config SPL_MMC_TINY
>   	bool "Tiny MMC framework in SPL"
>   	depends on SPL_MMC_SUPPORT
> 

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [U-Boot] [PATCH] imx: Introduce CONFIG_SPL_FORCE_MMC_BOOT to force MMC boot on falcon mode
  2019-09-04  4:45 ` Heiko Schocher
@ 2019-09-04  8:46   ` Lukasz Majewski
  2019-09-04  8:59     ` Stefano Babic
  0 siblings, 1 reply; 12+ messages in thread
From: Lukasz Majewski @ 2019-09-04  8:46 UTC (permalink / raw)
  To: u-boot

Hi Heiko,

> Hello Lukasz,
> 
> added Stefano to cc as he is the imx custodian.

I've relied on patman ... Thanks for adding Stefano to CC.

> 
> Am 03.09.2019 um 23:43 schrieb Lukasz Majewski:
> > This change tries to fix the following problem:
> > 
> > - The board boots (to be more precise - ROM loads SPL) from a slow
> > SPI-NOR memory.
> >    As a result the spl_boot_device() will return SPI-NOR as a boot
> > device (which is correct).
> > 
> > - The problem is that in 'falcon boot' the eMMC is used as a boot
> > medium to load kernel from its partition.
> >    Calling spl_boot_device() will break things as it returns
> > SPI-NOR device.
> > 
> > To fix this issue the new CONFIG_SPL_FORCE_MMC_BOOT Kconfig flag is
> > introduced to handle this special use case. By default it is not
> > defined, so there is no change in the legacy code flow.
> > 
> > Signed-off-by: Lukasz Majewski <lukma@denx.de>
> > 
> > 
> > ---
> > 
> > This patch is a follow up of previous discussion/fix:
> > https://patchwork.ozlabs.org/patch/796237/
> > 
> > Travis-CI:
> > https://travis-ci.org/lmajewski/u-boot-dfu/builds/580272414
> > ---
> >   arch/arm/mach-imx/spl.c | 11 +++++++++++
> >   common/spl/Kconfig      |  7 +++++++
> >   2 files changed, 18 insertions(+)  
> 
> I have just similiar change for an imx6ull based board ...

Also one more of my boards uses this trick (i.MX28 based one).

> just antoher usecase: In factory empty board boots into USB SDP mode,
> and SPL gets loaded with usb_loader ... SPL detects a sd card (there
> is no sd card slot mounted, mmc boot is disabled! They attach only one
> for installing software)

So there is no dedicated SD card slot (also the mmc is disabled on
that point).
Instead, in the factory the sd card is attached to pads - just for this
time.

> and boots U-Boot and system from sd card.
> Than all software gets installed fully automated with the help of
> swupdate ...

Ok.

> 
> > diff --git a/arch/arm/mach-imx/spl.c b/arch/arm/mach-imx/spl.c
> > index 1f230aca3397..daa3d8f7ed94 100644
> > --- a/arch/arm/mach-imx/spl.c
> > +++ b/arch/arm/mach-imx/spl.c
> > @@ -178,7 +178,18 @@ int g_dnl_bind_fixup(struct
> > usb_device_descriptor *dev, const char *name) /* called from
> > spl_mmc to see type of boot mode for storage (RAW or FAT) */ u32
> > spl_boot_mode(const u32 boot_device) {
> > +/*
> > + * When CONFIG_SPL_FORCE_MMC_BOOT is defined the 'boot_device' is
> > used
> > + * unconditionally to decide about device to use for booting.
> > + * This is crucial for falcon boot mode, when board boots up (i.e.
> > ROM
> > + * loads SPL) from slow SPI-NOR memory and afterwards the SPL's
> > 'falcon' boot
> > + * mode is used to load Linux OS from eMMC partition.
> > + */
> > +#ifdef CONFIG_SPL_FORCE_MMC_BOOT
> > +	switch (boot_device) {
> > +#else
> >   	switch (spl_boot_device()) {
> > +#endif  
> 
> Just dummy question .. couldn;t we switch to use always boot_device?

Stefano has provided some rationale for the need of spl_boot_device()
in the previous version of this fix [1]:

https://patchwork.ozlabs.org/patch/796237/


> 
> In my case I had a board specific:
> 
> void board_boot_order(u32 *spl_boot_list)
> {
>          spl_boot_list[0] = BOOT_DEVICE_MMC1;
>          spl_boot_list[1] = BOOT_DEVICE_SPI;
>          spl_boot_list[2] = BOOT_DEVICE_BOARD;
>          spl_boot_list[3] = BOOT_DEVICE_NONE;
> }
> 
> which leads that BOOT_DEVICE_MMC1 is passed to spl_boot_mode()

Is your board normally booting from MMC or SPI-NOR (from where SoC ROM
loads SPL) ?

> 
> If MMC1 is not found, it tries to boot U-Boot from SPI, and if this is
> empty, as fallback board go into USB SDP mode.
> 
> The weak function of board_boot_order is:
> 
> __weak void board_boot_order(u32 *spl_boot_list)
> {
>          spl_boot_list[0] = spl_boot_device();
> }
> 
> which is exactly what we pass to spl_boot_mode() ... so instead
> calling spl_boot_device() twice we can use the passed boot_device ...
> and save the ifdef and new Kconfig option.
> 
> But may I oversee something ?

Please read the Stefano's reply from [1] - the spl_boot_device() has a
valid use cases as well.

> 
> >   	/* for MMC return either RAW or FAT mode */
> >   	case BOOT_DEVICE_MMC1:
> >   	case BOOT_DEVICE_MMC2:
> > diff --git a/common/spl/Kconfig b/common/spl/Kconfig
> > index f467eca2be72..3460beb62d59 100644
> > --- a/common/spl/Kconfig
> > +++ b/common/spl/Kconfig
> > @@ -607,6 +607,13 @@ config SPL_MMC_SUPPORT
> >   	  this option to build the drivers in drivers/mmc as part
> > of an SPL build.
> >   
> > +config SPL_FORCE_MMC_BOOT
> > +	bool "Force SPL's falcon boot from MMC"
> > +	depends on SPL_MMC_SUPPORT
> > +	default n
> > +	help
> > +	  Force SPL's falcon boot to use MMC device for Linux
> > kernel booting.  
> 
> Hmm... falcon boot is just one use case. I would drop "falcon boot"
> It just to force boot from MMC.

Ok. I will rewrite the help message.

> 
> > +
> >   config SPL_MMC_TINY
> >   	bool "Tiny MMC framework in SPL"
> >   	depends on SPL_MMC_SUPPORT
> >   
> 
> bye,
> Heiko



Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190904/51782664/attachment.sig>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [U-Boot] [PATCH] imx: Introduce CONFIG_SPL_FORCE_MMC_BOOT to force MMC boot on falcon mode
  2019-09-04  8:46   ` Lukasz Majewski
@ 2019-09-04  8:59     ` Stefano Babic
  2019-09-04  9:54       ` Lukasz Majewski
  0 siblings, 1 reply; 12+ messages in thread
From: Stefano Babic @ 2019-09-04  8:59 UTC (permalink / raw)
  To: u-boot

On 04/09/19 10:46, Lukasz Majewski wrote:
> Hi Heiko,
> 
>> Hello Lukasz,
>>
>> added Stefano to cc as he is the imx custodian.
> 
> I've relied on patman ... Thanks for adding Stefano to CC.
> 
>>
>> Am 03.09.2019 um 23:43 schrieb Lukasz Majewski:
>>> This change tries to fix the following problem:
>>>
>>> - The board boots (to be more precise - ROM loads SPL) from a slow
>>> SPI-NOR memory.
>>>    As a result the spl_boot_device() will return SPI-NOR as a boot
>>> device (which is correct).
>>>
>>> - The problem is that in 'falcon boot' the eMMC is used as a boot
>>> medium to load kernel from its partition.
>>>    Calling spl_boot_device() will break things as it returns
>>> SPI-NOR device.
>>>
>>> To fix this issue the new CONFIG_SPL_FORCE_MMC_BOOT Kconfig flag is
>>> introduced to handle this special use case. By default it is not
>>> defined, so there is no change in the legacy code flow.
>>>
>>> Signed-off-by: Lukasz Majewski <lukma@denx.de>
>>>
>>>
>>> ---
>>>
>>> This patch is a follow up of previous discussion/fix:
>>> https://patchwork.ozlabs.org/patch/796237/
>>>
>>> Travis-CI:
>>> https://travis-ci.org/lmajewski/u-boot-dfu/builds/580272414
>>> ---
>>>   arch/arm/mach-imx/spl.c | 11 +++++++++++
>>>   common/spl/Kconfig      |  7 +++++++
>>>   2 files changed, 18 insertions(+)  
>>
>> I have just similiar change for an imx6ull based board ...
> 
> Also one more of my boards uses this trick (i.MX28 based one).
> 
>> just antoher usecase: In factory empty board boots into USB SDP mode,
>> and SPL gets loaded with usb_loader ... SPL detects a sd card (there
>> is no sd card slot mounted, mmc boot is disabled! They attach only one
>> for installing software)
> 
> So there is no dedicated SD card slot (also the mmc is disabled on
> that point).
> Instead, in the factory the sd card is attached to pads - just for this
> time.
> 
>> and boots U-Boot and system from sd card.
>> Than all software gets installed fully automated with the help of
>> swupdate ...
> 
> Ok.
> 
>>
>>> diff --git a/arch/arm/mach-imx/spl.c b/arch/arm/mach-imx/spl.c
>>> index 1f230aca3397..daa3d8f7ed94 100644
>>> --- a/arch/arm/mach-imx/spl.c
>>> +++ b/arch/arm/mach-imx/spl.c
>>> @@ -178,7 +178,18 @@ int g_dnl_bind_fixup(struct
>>> usb_device_descriptor *dev, const char *name) /* called from
>>> spl_mmc to see type of boot mode for storage (RAW or FAT) */ u32
>>> spl_boot_mode(const u32 boot_device) {
>>> +/*
>>> + * When CONFIG_SPL_FORCE_MMC_BOOT is defined the 'boot_device' is
>>> used
>>> + * unconditionally to decide about device to use for booting.
>>> + * This is crucial for falcon boot mode, when board boots up (i.e.
>>> ROM
>>> + * loads SPL) from slow SPI-NOR memory and afterwards the SPL's
>>> 'falcon' boot
>>> + * mode is used to load Linux OS from eMMC partition.
>>> + */
>>> +#ifdef CONFIG_SPL_FORCE_MMC_BOOT
>>> +	switch (boot_device) {
>>> +#else
>>>   	switch (spl_boot_device()) {
>>> +#endif  
>>
>> Just dummy question .. couldn;t we switch to use always boot_device?
> 
> Stefano has provided some rationale for the need of spl_boot_device()
> in the previous version of this fix [1]:
> 
> https://patchwork.ozlabs.org/patch/796237/
> 
> 
>>
>> In my case I had a board specific:
>>
>> void board_boot_order(u32 *spl_boot_list)
>> {
>>          spl_boot_list[0] = BOOT_DEVICE_MMC1;
>>          spl_boot_list[1] = BOOT_DEVICE_SPI;
>>          spl_boot_list[2] = BOOT_DEVICE_BOARD;
>>          spl_boot_list[3] = BOOT_DEVICE_NONE;
>> }
>>
>> which leads that BOOT_DEVICE_MMC1 is passed to spl_boot_mode()
> 
> Is your board normally booting from MMC or SPI-NOR (from where SoC ROM
> loads SPL) ?
> 
>>
>> If MMC1 is not found, it tries to boot U-Boot from SPI, and if this is
>> empty, as fallback board go into USB SDP mode.
>>
>> The weak function of board_boot_order is:
>>
>> __weak void board_boot_order(u32 *spl_boot_list)
>> {
>>          spl_boot_list[0] = spl_boot_device();
>> }
>>
>> which is exactly what we pass to spl_boot_mode() ... so instead
>> calling spl_boot_device() twice we can use the passed boot_device ...
>> and save the ifdef and new Kconfig option.
>>
>> But may I oversee something ?
> 
> Please read the Stefano's reply from [1] - the spl_boot_device() has a
> valid use cases as well.

Nevertheless, why do we need to add a new compiler switch if this can be
check into board code ? You just need that spl_boot_device() returns the
device you want (MMC for falcon boot). Why do you need to force it in
this way ?

Regards,
Stefano

> 
>>
>>>   	/* for MMC return either RAW or FAT mode */
>>>   	case BOOT_DEVICE_MMC1:
>>>   	case BOOT_DEVICE_MMC2:
>>> diff --git a/common/spl/Kconfig b/common/spl/Kconfig
>>> index f467eca2be72..3460beb62d59 100644
>>> --- a/common/spl/Kconfig
>>> +++ b/common/spl/Kconfig
>>> @@ -607,6 +607,13 @@ config SPL_MMC_SUPPORT
>>>   	  this option to build the drivers in drivers/mmc as part
>>> of an SPL build.
>>>   
>>> +config SPL_FORCE_MMC_BOOT
>>> +	bool "Force SPL's falcon boot from MMC"
>>> +	depends on SPL_MMC_SUPPORT
>>> +	default n
>>> +	help
>>> +	  Force SPL's falcon boot to use MMC device for Linux
>>> kernel booting.  
>>
>> Hmm... falcon boot is just one use case. I would drop "falcon boot"
>> It just to force boot from MMC.
> 
> Ok. I will rewrite the help message.
> 
>>
>>> +
>>>   config SPL_MMC_TINY
>>>   	bool "Tiny MMC framework in SPL"
>>>   	depends on SPL_MMC_SUPPORT
>>>   
>>
>> bye,
>> Heiko
> 
> 
> 
> Best regards,
> 
> Lukasz Majewski
> 
> --
> 
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma at denx.de
> 


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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [U-Boot] [PATCH] imx: Introduce CONFIG_SPL_FORCE_MMC_BOOT to force MMC boot on falcon mode
  2019-09-04  8:59     ` Stefano Babic
@ 2019-09-04  9:54       ` Lukasz Majewski
  2019-09-04 10:35         ` Lukasz Majewski
  0 siblings, 1 reply; 12+ messages in thread
From: Lukasz Majewski @ 2019-09-04  9:54 UTC (permalink / raw)
  To: u-boot

Hi Stefano, Heiko,

> On 04/09/19 10:46, Lukasz Majewski wrote:
> > Hi Heiko,
> >   
> >> Hello Lukasz,
> >>
> >> added Stefano to cc as he is the imx custodian.  
> > 
> > I've relied on patman ... Thanks for adding Stefano to CC.
> >   
> >>
> >> Am 03.09.2019 um 23:43 schrieb Lukasz Majewski:  
> >>> This change tries to fix the following problem:
> >>>
> >>> - The board boots (to be more precise - ROM loads SPL) from a slow
> >>> SPI-NOR memory.
> >>>    As a result the spl_boot_device() will return SPI-NOR as a boot
> >>> device (which is correct).
> >>>
> >>> - The problem is that in 'falcon boot' the eMMC is used as a boot
> >>> medium to load kernel from its partition.
> >>>    Calling spl_boot_device() will break things as it returns
> >>> SPI-NOR device.
> >>>
> >>> To fix this issue the new CONFIG_SPL_FORCE_MMC_BOOT Kconfig flag
> >>> is introduced to handle this special use case. By default it is
> >>> not defined, so there is no change in the legacy code flow.
> >>>
> >>> Signed-off-by: Lukasz Majewski <lukma@denx.de>
> >>>
> >>>
> >>> ---
> >>>
> >>> This patch is a follow up of previous discussion/fix:
> >>> https://patchwork.ozlabs.org/patch/796237/
> >>>
> >>> Travis-CI:
> >>> https://travis-ci.org/lmajewski/u-boot-dfu/builds/580272414
> >>> ---
> >>>   arch/arm/mach-imx/spl.c | 11 +++++++++++
> >>>   common/spl/Kconfig      |  7 +++++++
> >>>   2 files changed, 18 insertions(+)    
> >>
> >> I have just similiar change for an imx6ull based board ...  
> > 
> > Also one more of my boards uses this trick (i.MX28 based one).
> >   
> >> just antoher usecase: In factory empty board boots into USB SDP
> >> mode, and SPL gets loaded with usb_loader ... SPL detects a sd
> >> card (there is no sd card slot mounted, mmc boot is disabled! They
> >> attach only one for installing software)  
> > 
> > So there is no dedicated SD card slot (also the mmc is disabled on
> > that point).
> > Instead, in the factory the sd card is attached to pads - just for
> > this time.
> >   
> >> and boots U-Boot and system from sd card.
> >> Than all software gets installed fully automated with the help of
> >> swupdate ...  
> > 
> > Ok.
> >   
> >>  
> >>> diff --git a/arch/arm/mach-imx/spl.c b/arch/arm/mach-imx/spl.c
> >>> index 1f230aca3397..daa3d8f7ed94 100644
> >>> --- a/arch/arm/mach-imx/spl.c
> >>> +++ b/arch/arm/mach-imx/spl.c
> >>> @@ -178,7 +178,18 @@ int g_dnl_bind_fixup(struct
> >>> usb_device_descriptor *dev, const char *name) /* called from
> >>> spl_mmc to see type of boot mode for storage (RAW or FAT) */ u32
> >>> spl_boot_mode(const u32 boot_device) {
> >>> +/*
> >>> + * When CONFIG_SPL_FORCE_MMC_BOOT is defined the 'boot_device' is
> >>> used
> >>> + * unconditionally to decide about device to use for booting.
> >>> + * This is crucial for falcon boot mode, when board boots up
> >>> (i.e. ROM
> >>> + * loads SPL) from slow SPI-NOR memory and afterwards the SPL's
> >>> 'falcon' boot
> >>> + * mode is used to load Linux OS from eMMC partition.
> >>> + */
> >>> +#ifdef CONFIG_SPL_FORCE_MMC_BOOT
> >>> +	switch (boot_device) {
> >>> +#else
> >>>   	switch (spl_boot_device()) {
> >>> +#endif    
> >>
> >> Just dummy question .. couldn;t we switch to use always
> >> boot_device?  
> > 
> > Stefano has provided some rationale for the need of
> > spl_boot_device() in the previous version of this fix [1]:
> > 
> > https://patchwork.ozlabs.org/patch/796237/
> > 
> >   
> >>
> >> In my case I had a board specific:
> >>
> >> void board_boot_order(u32 *spl_boot_list)
> >> {
> >>          spl_boot_list[0] = BOOT_DEVICE_MMC1;
> >>          spl_boot_list[1] = BOOT_DEVICE_SPI;
> >>          spl_boot_list[2] = BOOT_DEVICE_BOARD;
> >>          spl_boot_list[3] = BOOT_DEVICE_NONE;
> >> }
> >>
> >> which leads that BOOT_DEVICE_MMC1 is passed to spl_boot_mode()  
> > 
> > Is your board normally booting from MMC or SPI-NOR (from where SoC
> > ROM loads SPL) ?
> >   
> >>
> >> If MMC1 is not found, it tries to boot U-Boot from SPI, and if
> >> this is empty, as fallback board go into USB SDP mode.
> >>
> >> The weak function of board_boot_order is:
> >>
> >> __weak void board_boot_order(u32 *spl_boot_list)
> >> {
> >>          spl_boot_list[0] = spl_boot_device();
> >> }
> >>
> >> which is exactly what we pass to spl_boot_mode() ... so instead
> >> calling spl_boot_device() twice we can use the passed boot_device
> >> ... and save the ifdef and new Kconfig option.
> >>
> >> But may I oversee something ?  
> > 
> > Please read the Stefano's reply from [1] - the spl_boot_device()
> > has a valid use cases as well.  
> 
> Nevertheless, why do we need to add a new compiler switch if this can
> be check into board code ? You just need that spl_boot_device()
> returns the device you want (MMC for falcon boot). 

Current status:

git grep -n "spl_boot_mode" | grep weak
common/spl/spl_mmc.c:287:u32 __weak spl_boot_mode(const u32 boot_device)

git grep -n "board_boot_order" | grep weak
common/spl/spl.c:491:__weak void board_boot_order(u32 *spl_boot_list)




The spl_boot_device() is defined in arch/arm/mach-imx/spl.c [*] as a NON
__weak function:
git grep -n "spl_boot_device" | grep weak



Shall I make this function [*] as __weak and provide override for it
in the board file?

The problem is in the call of spl_boot_mode() (which is overridden
already in arch/arm/mach-imx/spl.c) at common/spl/spl_mmc.c

Which then calls spl_boot_device() [**], which may return (correctly)
SPI-NOR for eMMC.


As is now spl_boot_device() is not a __weak function.

> Why do you need to
> force it in this way ?

The option is to use the proposed patch to make spl_boot_device as a
weak function.

> 
> Regards,
> Stefano
> 
> >   
> >>  
> >>>   	/* for MMC return either RAW or FAT mode */
> >>>   	case BOOT_DEVICE_MMC1:
> >>>   	case BOOT_DEVICE_MMC2:
> >>> diff --git a/common/spl/Kconfig b/common/spl/Kconfig
> >>> index f467eca2be72..3460beb62d59 100644
> >>> --- a/common/spl/Kconfig
> >>> +++ b/common/spl/Kconfig
> >>> @@ -607,6 +607,13 @@ config SPL_MMC_SUPPORT
> >>>   	  this option to build the drivers in drivers/mmc as
> >>> part of an SPL build.
> >>>   
> >>> +config SPL_FORCE_MMC_BOOT
> >>> +	bool "Force SPL's falcon boot from MMC"
> >>> +	depends on SPL_MMC_SUPPORT
> >>> +	default n
> >>> +	help
> >>> +	  Force SPL's falcon boot to use MMC device for Linux
> >>> kernel booting.    
> >>
> >> Hmm... falcon boot is just one use case. I would drop "falcon boot"
> >> It just to force boot from MMC.  
> > 
> > Ok. I will rewrite the help message.
> >   
> >>  
> >>> +
> >>>   config SPL_MMC_TINY
> >>>   	bool "Tiny MMC framework in SPL"
> >>>   	depends on SPL_MMC_SUPPORT
> >>>     
> >>
> >> bye,
> >> Heiko  
> > 
> > 
> > 
> > Best regards,
> > 
> > Lukasz Majewski
> > 
> > --
> > 
> > DENX Software Engineering GmbH,      Managing Director: Wolfgang
> > Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell,
> > Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email:
> > lukma at denx.de 
> 
> 



Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190904/02d5ca2b/attachment.sig>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [U-Boot] [PATCH] imx: Introduce CONFIG_SPL_FORCE_MMC_BOOT to force MMC boot on falcon mode
  2019-09-04  9:54       ` Lukasz Majewski
@ 2019-09-04 10:35         ` Lukasz Majewski
  2019-09-05  8:08           ` Stefano Babic
  0 siblings, 1 reply; 12+ messages in thread
From: Lukasz Majewski @ 2019-09-04 10:35 UTC (permalink / raw)
  To: u-boot

On Wed, 4 Sep 2019 11:54:40 +0200
Lukasz Majewski <lukma@denx.de> wrote:

> Hi Stefano, Heiko,
> 
> > On 04/09/19 10:46, Lukasz Majewski wrote:  
> > > Hi Heiko,
> > >     
> > >> Hello Lukasz,
> > >>
> > >> added Stefano to cc as he is the imx custodian.    
> > > 
> > > I've relied on patman ... Thanks for adding Stefano to CC.
> > >     
> > >>
> > >> Am 03.09.2019 um 23:43 schrieb Lukasz Majewski:    
> > >>> This change tries to fix the following problem:
> > >>>
> > >>> - The board boots (to be more precise - ROM loads SPL) from a
> > >>> slow SPI-NOR memory.
> > >>>    As a result the spl_boot_device() will return SPI-NOR as a
> > >>> boot device (which is correct).
> > >>>
> > >>> - The problem is that in 'falcon boot' the eMMC is used as a
> > >>> boot medium to load kernel from its partition.
> > >>>    Calling spl_boot_device() will break things as it returns
> > >>> SPI-NOR device.
> > >>>
> > >>> To fix this issue the new CONFIG_SPL_FORCE_MMC_BOOT Kconfig flag
> > >>> is introduced to handle this special use case. By default it is
> > >>> not defined, so there is no change in the legacy code flow.
> > >>>
> > >>> Signed-off-by: Lukasz Majewski <lukma@denx.de>
> > >>>
> > >>>
> > >>> ---
> > >>>
> > >>> This patch is a follow up of previous discussion/fix:
> > >>> https://patchwork.ozlabs.org/patch/796237/
> > >>>
> > >>> Travis-CI:
> > >>> https://travis-ci.org/lmajewski/u-boot-dfu/builds/580272414
> > >>> ---
> > >>>   arch/arm/mach-imx/spl.c | 11 +++++++++++
> > >>>   common/spl/Kconfig      |  7 +++++++
> > >>>   2 files changed, 18 insertions(+)      
> > >>
> > >> I have just similiar change for an imx6ull based board ...    
> > > 
> > > Also one more of my boards uses this trick (i.MX28 based one).
> > >     
> > >> just antoher usecase: In factory empty board boots into USB SDP
> > >> mode, and SPL gets loaded with usb_loader ... SPL detects a sd
> > >> card (there is no sd card slot mounted, mmc boot is disabled!
> > >> They attach only one for installing software)    
> > > 
> > > So there is no dedicated SD card slot (also the mmc is disabled on
> > > that point).
> > > Instead, in the factory the sd card is attached to pads - just for
> > > this time.
> > >     
> > >> and boots U-Boot and system from sd card.
> > >> Than all software gets installed fully automated with the help of
> > >> swupdate ...    
> > > 
> > > Ok.
> > >     
> > >>    
> > >>> diff --git a/arch/arm/mach-imx/spl.c b/arch/arm/mach-imx/spl.c
> > >>> index 1f230aca3397..daa3d8f7ed94 100644
> > >>> --- a/arch/arm/mach-imx/spl.c
> > >>> +++ b/arch/arm/mach-imx/spl.c
> > >>> @@ -178,7 +178,18 @@ int g_dnl_bind_fixup(struct
> > >>> usb_device_descriptor *dev, const char *name) /* called from
> > >>> spl_mmc to see type of boot mode for storage (RAW or FAT) */ u32
> > >>> spl_boot_mode(const u32 boot_device) {
> > >>> +/*
> > >>> + * When CONFIG_SPL_FORCE_MMC_BOOT is defined the 'boot_device'
> > >>> is used
> > >>> + * unconditionally to decide about device to use for booting.
> > >>> + * This is crucial for falcon boot mode, when board boots up
> > >>> (i.e. ROM
> > >>> + * loads SPL) from slow SPI-NOR memory and afterwards the SPL's
> > >>> 'falcon' boot
> > >>> + * mode is used to load Linux OS from eMMC partition.
> > >>> + */
> > >>> +#ifdef CONFIG_SPL_FORCE_MMC_BOOT
> > >>> +	switch (boot_device) {
> > >>> +#else
> > >>>   	switch (spl_boot_device()) {
> > >>> +#endif      
> > >>
> > >> Just dummy question .. couldn;t we switch to use always
> > >> boot_device?    
> > > 
> > > Stefano has provided some rationale for the need of
> > > spl_boot_device() in the previous version of this fix [1]:
> > > 
> > > https://patchwork.ozlabs.org/patch/796237/
> > > 
> > >     
> > >>
> > >> In my case I had a board specific:
> > >>
> > >> void board_boot_order(u32 *spl_boot_list)
> > >> {
> > >>          spl_boot_list[0] = BOOT_DEVICE_MMC1;
> > >>          spl_boot_list[1] = BOOT_DEVICE_SPI;
> > >>          spl_boot_list[2] = BOOT_DEVICE_BOARD;
> > >>          spl_boot_list[3] = BOOT_DEVICE_NONE;
> > >> }
> > >>
> > >> which leads that BOOT_DEVICE_MMC1 is passed to spl_boot_mode()
> > >>  
> > > 
> > > Is your board normally booting from MMC or SPI-NOR (from where SoC
> > > ROM loads SPL) ?
> > >     
> > >>
> > >> If MMC1 is not found, it tries to boot U-Boot from SPI, and if
> > >> this is empty, as fallback board go into USB SDP mode.
> > >>
> > >> The weak function of board_boot_order is:
> > >>
> > >> __weak void board_boot_order(u32 *spl_boot_list)
> > >> {
> > >>          spl_boot_list[0] = spl_boot_device();
> > >> }
> > >>
> > >> which is exactly what we pass to spl_boot_mode() ... so instead
> > >> calling spl_boot_device() twice we can use the passed boot_device
> > >> ... and save the ifdef and new Kconfig option.
> > >>
> > >> But may I oversee something ?    
> > > 
> > > Please read the Stefano's reply from [1] - the spl_boot_device()
> > > has a valid use cases as well.    
> > 
> > Nevertheless, why do we need to add a new compiler switch if this
> > can be check into board code ? You just need that spl_boot_device()
> > returns the device you want (MMC for falcon boot).   
> 
> Current status:
> 
> git grep -n "spl_boot_mode" | grep weak
> common/spl/spl_mmc.c:287:u32 __weak spl_boot_mode(const u32
> boot_device)
> 
> git grep -n "board_boot_order" | grep weak
> common/spl/spl.c:491:__weak void board_boot_order(u32 *spl_boot_list)
> 
> 
> 
> 
> The spl_boot_device() is defined in arch/arm/mach-imx/spl.c [*] as a
> NON __weak function:
> git grep -n "spl_boot_device" | grep weak
> 
> 
> 
> Shall I make this function [*] as __weak and provide override for it
> in the board file?
> 
> The problem is in the call of spl_boot_mode() (which is overridden
> already in arch/arm/mach-imx/spl.c) at common/spl/spl_mmc.c
> 
> Which then calls spl_boot_device() [**], which may return (correctly)
> SPI-NOR for eMMC.
> 
> 
> As is now spl_boot_device() is not a __weak function.
> 
> > Why do you need to
> > force it in this way ?  
> 
> The option is to use the proposed patch to make spl_boot_device as a
> weak function.

The above text is a bit misleading - better version below.

The solution would be:

1. Use the proposed patch (with Kconfig option)

2. Define spl_boot_device as weak and override it in board file.
   However, it is not the preferred solution as spl_boot_device() on my
   setup correctly returns SPI-NOR (as SOC ROM loads SPL from SPI-NOR,
   not eMMC).

> 
> > 
> > Regards,
> > Stefano
> >   
> > >     
> > >>    
> > >>>   	/* for MMC return either RAW or FAT mode */
> > >>>   	case BOOT_DEVICE_MMC1:
> > >>>   	case BOOT_DEVICE_MMC2:
> > >>> diff --git a/common/spl/Kconfig b/common/spl/Kconfig
> > >>> index f467eca2be72..3460beb62d59 100644
> > >>> --- a/common/spl/Kconfig
> > >>> +++ b/common/spl/Kconfig
> > >>> @@ -607,6 +607,13 @@ config SPL_MMC_SUPPORT
> > >>>   	  this option to build the drivers in drivers/mmc as
> > >>> part of an SPL build.
> > >>>   
> > >>> +config SPL_FORCE_MMC_BOOT
> > >>> +	bool "Force SPL's falcon boot from MMC"
> > >>> +	depends on SPL_MMC_SUPPORT
> > >>> +	default n
> > >>> +	help
> > >>> +	  Force SPL's falcon boot to use MMC device for Linux
> > >>> kernel booting.      
> > >>
> > >> Hmm... falcon boot is just one use case. I would drop "falcon
> > >> boot" It just to force boot from MMC.    
> > > 
> > > Ok. I will rewrite the help message.
> > >     
> > >>    
> > >>> +
> > >>>   config SPL_MMC_TINY
> > >>>   	bool "Tiny MMC framework in SPL"
> > >>>   	depends on SPL_MMC_SUPPORT
> > >>>       
> > >>
> > >> bye,
> > >> Heiko    
> > > 
> > > 
> > > 
> > > Best regards,
> > > 
> > > Lukasz Majewski
> > > 
> > > --
> > > 
> > > DENX Software Engineering GmbH,      Managing Director: Wolfgang
> > > Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell,
> > > Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email:
> > > lukma at denx.de   
> > 
> >   
> 
> 
> 
> Best regards,
> 
> Lukasz Majewski
> 
> --
> 
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email:
> lukma at denx.de



Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190904/70f120a5/attachment.sig>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [U-Boot] [PATCH] imx: Introduce CONFIG_SPL_FORCE_MMC_BOOT to force MMC boot on falcon mode
  2019-09-04 10:35         ` Lukasz Majewski
@ 2019-09-05  8:08           ` Stefano Babic
  2019-09-05  9:42             ` Lukasz Majewski
  2019-09-05 10:02             ` Heiko Schocher
  0 siblings, 2 replies; 12+ messages in thread
From: Stefano Babic @ 2019-09-05  8:08 UTC (permalink / raw)
  To: u-boot

On 04/09/19 12:35, Lukasz Majewski wrote:
> On Wed, 4 Sep 2019 11:54:40 +0200
> Lukasz Majewski <lukma@denx.de> wrote:
> 
>> Hi Stefano, Heiko,
>>
>>> On 04/09/19 10:46, Lukasz Majewski wrote:  
>>>> Hi Heiko,
>>>>     
>>>>> Hello Lukasz,
>>>>>
>>>>> added Stefano to cc as he is the imx custodian.    
>>>>
>>>> I've relied on patman ... Thanks for adding Stefano to CC.
>>>>     
>>>>>
>>>>> Am 03.09.2019 um 23:43 schrieb Lukasz Majewski:    
>>>>>> This change tries to fix the following problem:
>>>>>>
>>>>>> - The board boots (to be more precise - ROM loads SPL) from a
>>>>>> slow SPI-NOR memory.
>>>>>>    As a result the spl_boot_device() will return SPI-NOR as a
>>>>>> boot device (which is correct).
>>>>>>
>>>>>> - The problem is that in 'falcon boot' the eMMC is used as a
>>>>>> boot medium to load kernel from its partition.
>>>>>>    Calling spl_boot_device() will break things as it returns
>>>>>> SPI-NOR device.
>>>>>>
>>>>>> To fix this issue the new CONFIG_SPL_FORCE_MMC_BOOT Kconfig flag
>>>>>> is introduced to handle this special use case. By default it is
>>>>>> not defined, so there is no change in the legacy code flow.
>>>>>>
>>>>>> Signed-off-by: Lukasz Majewski <lukma@denx.de>
>>>>>>
>>>>>>
>>>>>> ---
>>>>>>
>>>>>> This patch is a follow up of previous discussion/fix:
>>>>>> https://patchwork.ozlabs.org/patch/796237/
>>>>>>
>>>>>> Travis-CI:
>>>>>> https://travis-ci.org/lmajewski/u-boot-dfu/builds/580272414
>>>>>> ---
>>>>>>   arch/arm/mach-imx/spl.c | 11 +++++++++++
>>>>>>   common/spl/Kconfig      |  7 +++++++
>>>>>>   2 files changed, 18 insertions(+)      
>>>>>
>>>>> I have just similiar change for an imx6ull based board ...    
>>>>
>>>> Also one more of my boards uses this trick (i.MX28 based one).
>>>>     
>>>>> just antoher usecase: In factory empty board boots into USB SDP
>>>>> mode, and SPL gets loaded with usb_loader ... SPL detects a sd
>>>>> card (there is no sd card slot mounted, mmc boot is disabled!
>>>>> They attach only one for installing software)    
>>>>
>>>> So there is no dedicated SD card slot (also the mmc is disabled on
>>>> that point).
>>>> Instead, in the factory the sd card is attached to pads - just for
>>>> this time.
>>>>     
>>>>> and boots U-Boot and system from sd card.
>>>>> Than all software gets installed fully automated with the help of
>>>>> swupdate ...    
>>>>
>>>> Ok.
>>>>     
>>>>>    
>>>>>> diff --git a/arch/arm/mach-imx/spl.c b/arch/arm/mach-imx/spl.c
>>>>>> index 1f230aca3397..daa3d8f7ed94 100644
>>>>>> --- a/arch/arm/mach-imx/spl.c
>>>>>> +++ b/arch/arm/mach-imx/spl.c
>>>>>> @@ -178,7 +178,18 @@ int g_dnl_bind_fixup(struct
>>>>>> usb_device_descriptor *dev, const char *name) /* called from
>>>>>> spl_mmc to see type of boot mode for storage (RAW or FAT) */ u32
>>>>>> spl_boot_mode(const u32 boot_device) {
>>>>>> +/*
>>>>>> + * When CONFIG_SPL_FORCE_MMC_BOOT is defined the 'boot_device'
>>>>>> is used
>>>>>> + * unconditionally to decide about device to use for booting.
>>>>>> + * This is crucial for falcon boot mode, when board boots up
>>>>>> (i.e. ROM
>>>>>> + * loads SPL) from slow SPI-NOR memory and afterwards the SPL's
>>>>>> 'falcon' boot
>>>>>> + * mode is used to load Linux OS from eMMC partition.
>>>>>> + */
>>>>>> +#ifdef CONFIG_SPL_FORCE_MMC_BOOT
>>>>>> +	switch (boot_device) {
>>>>>> +#else
>>>>>>   	switch (spl_boot_device()) {
>>>>>> +#endif      
>>>>>
>>>>> Just dummy question .. couldn;t we switch to use always
>>>>> boot_device?    
>>>>
>>>> Stefano has provided some rationale for the need of
>>>> spl_boot_device() in the previous version of this fix [1]:
>>>>
>>>> https://patchwork.ozlabs.org/patch/796237/
>>>>
>>>>     
>>>>>
>>>>> In my case I had a board specific:
>>>>>
>>>>> void board_boot_order(u32 *spl_boot_list)
>>>>> {
>>>>>          spl_boot_list[0] = BOOT_DEVICE_MMC1;
>>>>>          spl_boot_list[1] = BOOT_DEVICE_SPI;
>>>>>          spl_boot_list[2] = BOOT_DEVICE_BOARD;
>>>>>          spl_boot_list[3] = BOOT_DEVICE_NONE;
>>>>> }
>>>>>
>>>>> which leads that BOOT_DEVICE_MMC1 is passed to spl_boot_mode()
>>>>>  
>>>>
>>>> Is your board normally booting from MMC or SPI-NOR (from where SoC
>>>> ROM loads SPL) ?
>>>>     
>>>>>
>>>>> If MMC1 is not found, it tries to boot U-Boot from SPI, and if
>>>>> this is empty, as fallback board go into USB SDP mode.
>>>>>
>>>>> The weak function of board_boot_order is:
>>>>>
>>>>> __weak void board_boot_order(u32 *spl_boot_list)
>>>>> {
>>>>>          spl_boot_list[0] = spl_boot_device();
>>>>> }
>>>>>
>>>>> which is exactly what we pass to spl_boot_mode() ... so instead
>>>>> calling spl_boot_device() twice we can use the passed boot_device
>>>>> ... and save the ifdef and new Kconfig option.
>>>>>
>>>>> But may I oversee something ?    
>>>>
>>>> Please read the Stefano's reply from [1] - the spl_boot_device()
>>>> has a valid use cases as well.    
>>>
>>> Nevertheless, why do we need to add a new compiler switch if this
>>> can be check into board code ? You just need that spl_boot_device()
>>> returns the device you want (MMC for falcon boot).   
>>
>> Current status:
>>
>> git grep -n "spl_boot_mode" | grep weak
>> common/spl/spl_mmc.c:287:u32 __weak spl_boot_mode(const u32
>> boot_device)
>>
>> git grep -n "board_boot_order" | grep weak
>> common/spl/spl.c:491:__weak void board_boot_order(u32 *spl_boot_list)
>>
>>
>>
>>
>> The spl_boot_device() is defined in arch/arm/mach-imx/spl.c [*] as a
>> NON __weak function:
>> git grep -n "spl_boot_device" | grep weak
>>
>>
>>
>> Shall I make this function [*] as __weak and provide override for it
>> in the board file?
>>
>> The problem is in the call of spl_boot_mode() (which is overridden
>> already in arch/arm/mach-imx/spl.c) at common/spl/spl_mmc.c
>>
>> Which then calls spl_boot_device() [**], which may return (correctly)
>> SPI-NOR for eMMC.
>>
>>
>> As is now spl_boot_device() is not a __weak function.
>>
>>> Why do you need to
>>> force it in this way ?  
>>
>> The option is to use the proposed patch to make spl_boot_device as a
>> weak function.
> 
> The above text is a bit misleading - better version below.
> 
> The solution would be:
> 
> 1. Use the proposed patch (with Kconfig option)
> 
> 2. Define spl_boot_device as weak and override it in board file.
>    However, it is not the preferred solution as spl_boot_device() on my
>    setup correctly returns SPI-NOR (as SOC ROM loads SPL from SPI-NOR,
>    not eMMC).

Ok, but is a wek not thought for this purpose ? If it is weak, you can
change it and decide to return the device you want. You could also check
if Falco boot is enabled, at compile time with CONFIG_SPL_OS_BOOT or at
runtime with spl_start_uboot(). Should it not be enough ?

Regards,
Stefano

> 
>>
>>>
>>> Regards,
>>> Stefano
>>>   
>>>>     
>>>>>    
>>>>>>   	/* for MMC return either RAW or FAT mode */
>>>>>>   	case BOOT_DEVICE_MMC1:
>>>>>>   	case BOOT_DEVICE_MMC2:
>>>>>> diff --git a/common/spl/Kconfig b/common/spl/Kconfig
>>>>>> index f467eca2be72..3460beb62d59 100644
>>>>>> --- a/common/spl/Kconfig
>>>>>> +++ b/common/spl/Kconfig
>>>>>> @@ -607,6 +607,13 @@ config SPL_MMC_SUPPORT
>>>>>>   	  this option to build the drivers in drivers/mmc as
>>>>>> part of an SPL build.
>>>>>>   
>>>>>> +config SPL_FORCE_MMC_BOOT
>>>>>> +	bool "Force SPL's falcon boot from MMC"
>>>>>> +	depends on SPL_MMC_SUPPORT
>>>>>> +	default n
>>>>>> +	help
>>>>>> +	  Force SPL's falcon boot to use MMC device for Linux
>>>>>> kernel booting.      
>>>>>
>>>>> Hmm... falcon boot is just one use case. I would drop "falcon
>>>>> boot" It just to force boot from MMC.    
>>>>
>>>> Ok. I will rewrite the help message.
>>>>     
>>>>>    
>>>>>> +
>>>>>>   config SPL_MMC_TINY
>>>>>>   	bool "Tiny MMC framework in SPL"
>>>>>>   	depends on SPL_MMC_SUPPORT
>>>>>>       
>>>>>
>>>>> bye,
>>>>> Heiko    
>>>>
>>>>
>>>>
>>>> Best regards,
>>>>
>>>> Lukasz Majewski
>>>>
>>>> --
>>>>
>>>> DENX Software Engineering GmbH,      Managing Director: Wolfgang
>>>> Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell,
>>>> Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email:
>>>> lukma at denx.de   
>>>
>>>   
>>
>>
>>
>> Best regards,
>>
>> Lukasz Majewski
>>
>> --
>>
>> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
>> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
>> Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email:
>> lukma at denx.de
> 
> 
> 
> Best regards,
> 
> Lukasz Majewski
> 
> --
> 
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma at denx.de
> 


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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [U-Boot] [PATCH] imx: Introduce CONFIG_SPL_FORCE_MMC_BOOT to force MMC boot on falcon mode
  2019-09-05  8:08           ` Stefano Babic
@ 2019-09-05  9:42             ` Lukasz Majewski
  2019-09-05 10:18               ` Heiko Schocher
  2019-09-05 10:02             ` Heiko Schocher
  1 sibling, 1 reply; 12+ messages in thread
From: Lukasz Majewski @ 2019-09-05  9:42 UTC (permalink / raw)
  To: u-boot

Hi Stefano,

> On 04/09/19 12:35, Lukasz Majewski wrote:
> > On Wed, 4 Sep 2019 11:54:40 +0200
> > Lukasz Majewski <lukma@denx.de> wrote:
> >   
> >> Hi Stefano, Heiko,
> >>  
> >>> On 04/09/19 10:46, Lukasz Majewski wrote:    
> >>>> Hi Heiko,
> >>>>       
> >>>>> Hello Lukasz,
> >>>>>
> >>>>> added Stefano to cc as he is the imx custodian.      
> >>>>
> >>>> I've relied on patman ... Thanks for adding Stefano to CC.
> >>>>       
> >>>>>
> >>>>> Am 03.09.2019 um 23:43 schrieb Lukasz Majewski:      
> >>>>>> This change tries to fix the following problem:
> >>>>>>
> >>>>>> - The board boots (to be more precise - ROM loads SPL) from a
> >>>>>> slow SPI-NOR memory.
> >>>>>>    As a result the spl_boot_device() will return SPI-NOR as a
> >>>>>> boot device (which is correct).
> >>>>>>
> >>>>>> - The problem is that in 'falcon boot' the eMMC is used as a
> >>>>>> boot medium to load kernel from its partition.
> >>>>>>    Calling spl_boot_device() will break things as it returns
> >>>>>> SPI-NOR device.
> >>>>>>
> >>>>>> To fix this issue the new CONFIG_SPL_FORCE_MMC_BOOT Kconfig
> >>>>>> flag is introduced to handle this special use case. By default
> >>>>>> it is not defined, so there is no change in the legacy code
> >>>>>> flow.
> >>>>>>
> >>>>>> Signed-off-by: Lukasz Majewski <lukma@denx.de>
> >>>>>>
> >>>>>>
> >>>>>> ---
> >>>>>>
> >>>>>> This patch is a follow up of previous discussion/fix:
> >>>>>> https://patchwork.ozlabs.org/patch/796237/
> >>>>>>
> >>>>>> Travis-CI:
> >>>>>> https://travis-ci.org/lmajewski/u-boot-dfu/builds/580272414
> >>>>>> ---
> >>>>>>   arch/arm/mach-imx/spl.c | 11 +++++++++++
> >>>>>>   common/spl/Kconfig      |  7 +++++++
> >>>>>>   2 files changed, 18 insertions(+)        
> >>>>>
> >>>>> I have just similiar change for an imx6ull based board ...      
> >>>>
> >>>> Also one more of my boards uses this trick (i.MX28 based one).
> >>>>       
> >>>>> just antoher usecase: In factory empty board boots into USB SDP
> >>>>> mode, and SPL gets loaded with usb_loader ... SPL detects a sd
> >>>>> card (there is no sd card slot mounted, mmc boot is disabled!
> >>>>> They attach only one for installing software)      
> >>>>
> >>>> So there is no dedicated SD card slot (also the mmc is disabled
> >>>> on that point).
> >>>> Instead, in the factory the sd card is attached to pads - just
> >>>> for this time.
> >>>>       
> >>>>> and boots U-Boot and system from sd card.
> >>>>> Than all software gets installed fully automated with the help
> >>>>> of swupdate ...      
> >>>>
> >>>> Ok.
> >>>>       
> >>>>>      
> >>>>>> diff --git a/arch/arm/mach-imx/spl.c b/arch/arm/mach-imx/spl.c
> >>>>>> index 1f230aca3397..daa3d8f7ed94 100644
> >>>>>> --- a/arch/arm/mach-imx/spl.c
> >>>>>> +++ b/arch/arm/mach-imx/spl.c
> >>>>>> @@ -178,7 +178,18 @@ int g_dnl_bind_fixup(struct
> >>>>>> usb_device_descriptor *dev, const char *name) /* called from
> >>>>>> spl_mmc to see type of boot mode for storage (RAW or FAT) */
> >>>>>> u32 spl_boot_mode(const u32 boot_device) {
> >>>>>> +/*
> >>>>>> + * When CONFIG_SPL_FORCE_MMC_BOOT is defined the 'boot_device'
> >>>>>> is used
> >>>>>> + * unconditionally to decide about device to use for booting.
> >>>>>> + * This is crucial for falcon boot mode, when board boots up
> >>>>>> (i.e. ROM
> >>>>>> + * loads SPL) from slow SPI-NOR memory and afterwards the
> >>>>>> SPL's 'falcon' boot
> >>>>>> + * mode is used to load Linux OS from eMMC partition.
> >>>>>> + */
> >>>>>> +#ifdef CONFIG_SPL_FORCE_MMC_BOOT
> >>>>>> +	switch (boot_device) {
> >>>>>> +#else
> >>>>>>   	switch (spl_boot_device()) {
> >>>>>> +#endif        
> >>>>>
> >>>>> Just dummy question .. couldn;t we switch to use always
> >>>>> boot_device?      
> >>>>
> >>>> Stefano has provided some rationale for the need of
> >>>> spl_boot_device() in the previous version of this fix [1]:
> >>>>
> >>>> https://patchwork.ozlabs.org/patch/796237/
> >>>>
> >>>>       
> >>>>>
> >>>>> In my case I had a board specific:
> >>>>>
> >>>>> void board_boot_order(u32 *spl_boot_list)
> >>>>> {
> >>>>>          spl_boot_list[0] = BOOT_DEVICE_MMC1;
> >>>>>          spl_boot_list[1] = BOOT_DEVICE_SPI;
> >>>>>          spl_boot_list[2] = BOOT_DEVICE_BOARD;
> >>>>>          spl_boot_list[3] = BOOT_DEVICE_NONE;
> >>>>> }
> >>>>>
> >>>>> which leads that BOOT_DEVICE_MMC1 is passed to spl_boot_mode()
> >>>>>    
> >>>>
> >>>> Is your board normally booting from MMC or SPI-NOR (from where
> >>>> SoC ROM loads SPL) ?
> >>>>       
> >>>>>
> >>>>> If MMC1 is not found, it tries to boot U-Boot from SPI, and if
> >>>>> this is empty, as fallback board go into USB SDP mode.
> >>>>>
> >>>>> The weak function of board_boot_order is:
> >>>>>
> >>>>> __weak void board_boot_order(u32 *spl_boot_list)
> >>>>> {
> >>>>>          spl_boot_list[0] = spl_boot_device();
> >>>>> }
> >>>>>
> >>>>> which is exactly what we pass to spl_boot_mode() ... so instead
> >>>>> calling spl_boot_device() twice we can use the passed
> >>>>> boot_device ... and save the ifdef and new Kconfig option.
> >>>>>
> >>>>> But may I oversee something ?      
> >>>>
> >>>> Please read the Stefano's reply from [1] - the spl_boot_device()
> >>>> has a valid use cases as well.      
> >>>
> >>> Nevertheless, why do we need to add a new compiler switch if this
> >>> can be check into board code ? You just need that
> >>> spl_boot_device() returns the device you want (MMC for falcon
> >>> boot).     
> >>
> >> Current status:
> >>
> >> git grep -n "spl_boot_mode" | grep weak
> >> common/spl/spl_mmc.c:287:u32 __weak spl_boot_mode(const u32
> >> boot_device)
> >>
> >> git grep -n "board_boot_order" | grep weak
> >> common/spl/spl.c:491:__weak void board_boot_order(u32
> >> *spl_boot_list)
> >>
> >>
> >>
> >>
> >> The spl_boot_device() is defined in arch/arm/mach-imx/spl.c [*] as
> >> a NON __weak function:
> >> git grep -n "spl_boot_device" | grep weak
> >>
> >>
> >>
> >> Shall I make this function [*] as __weak and provide override for
> >> it in the board file?
> >>
> >> The problem is in the call of spl_boot_mode() (which is overridden
> >> already in arch/arm/mach-imx/spl.c) at common/spl/spl_mmc.c
> >>
> >> Which then calls spl_boot_device() [**], which may return
> >> (correctly) SPI-NOR for eMMC.
> >>
> >>
> >> As is now spl_boot_device() is not a __weak function.
> >>  
> >>> Why do you need to
> >>> force it in this way ?    
> >>
> >> The option is to use the proposed patch to make spl_boot_device as
> >> a weak function.  
> > 
> > The above text is a bit misleading - better version below.
> > 
> > The solution would be:
> > 
> > 1. Use the proposed patch (with Kconfig option)
> > 
> > 2. Define spl_boot_device as weak and override it in board file.
> >    However, it is not the preferred solution as spl_boot_device()
> > on my setup correctly returns SPI-NOR (as SOC ROM loads SPL from
> > SPI-NOR, not eMMC).  
> 
> Ok, but is a wek not thought for this purpose ? 

Making the spl_boot_device() __weak would be a fix only for a specific
appearance of spl_boot_device() in arch/arm/mach-imx/spl.c (L178):

------------------>8--------------------------
#if defined(CONFIG_SPL_MMC_SUPPORT)

/* called from spl_mmc to see type of boot mode for storage (RAW or FAT) */
u32 spl_boot_mode(const u32 boot_device)
{
	switch (spl_boot_device()) {
	/* for MMC return either RAW or FAT mode */
------------------8<--------------------------

The above code is only executed when SPL_MMC_SUPPORT is enabled and
IMHO the spl_boot_device() call assumes that one boots up from eMMC
(but not from SPI-NOR).

I'm (in a first place) curious if this code is correct - the boot_device
is passed as a parameter to this function, but then it is ignored.


> If it is weak, you can
> change it and decide to return the device you want. You could also
> check if Falco boot is enabled, at compile time with
> CONFIG_SPL_OS_BOOT or at runtime with spl_start_uboot(). Should it
> not be enough ?
> 
> Regards,
> Stefano
> 
> >   
> >>  
> >>>
> >>> Regards,
> >>> Stefano
> >>>     
> >>>>       
> >>>>>      
> >>>>>>   	/* for MMC return either RAW or FAT mode */
> >>>>>>   	case BOOT_DEVICE_MMC1:
> >>>>>>   	case BOOT_DEVICE_MMC2:
> >>>>>> diff --git a/common/spl/Kconfig b/common/spl/Kconfig
> >>>>>> index f467eca2be72..3460beb62d59 100644
> >>>>>> --- a/common/spl/Kconfig
> >>>>>> +++ b/common/spl/Kconfig
> >>>>>> @@ -607,6 +607,13 @@ config SPL_MMC_SUPPORT
> >>>>>>   	  this option to build the drivers in drivers/mmc as
> >>>>>> part of an SPL build.
> >>>>>>   
> >>>>>> +config SPL_FORCE_MMC_BOOT
> >>>>>> +	bool "Force SPL's falcon boot from MMC"
> >>>>>> +	depends on SPL_MMC_SUPPORT
> >>>>>> +	default n
> >>>>>> +	help
> >>>>>> +	  Force SPL's falcon boot to use MMC device for Linux
> >>>>>> kernel booting.        
> >>>>>
> >>>>> Hmm... falcon boot is just one use case. I would drop "falcon
> >>>>> boot" It just to force boot from MMC.      
> >>>>
> >>>> Ok. I will rewrite the help message.
> >>>>       
> >>>>>      
> >>>>>> +
> >>>>>>   config SPL_MMC_TINY
> >>>>>>   	bool "Tiny MMC framework in SPL"
> >>>>>>   	depends on SPL_MMC_SUPPORT
> >>>>>>         
> >>>>>
> >>>>> bye,
> >>>>> Heiko      
> >>>>
> >>>>
> >>>>
> >>>> Best regards,
> >>>>
> >>>> Lukasz Majewski
> >>>>
> >>>> --
> >>>>
> >>>> DENX Software Engineering GmbH,      Managing Director: Wolfgang
> >>>> Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194
> >>>> Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax:
> >>>> (+49)-8142-66989-80 Email: lukma at denx.de     
> >>>
> >>>     
> >>
> >>
> >>
> >> Best regards,
> >>
> >> Lukasz Majewski
> >>
> >> --
> >>
> >> DENX Software Engineering GmbH,      Managing Director: Wolfgang
> >> Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell,
> >> Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email:
> >> lukma at denx.de  
> > 
> > 
> > 
> > Best regards,
> > 
> > Lukasz Majewski
> > 
> > --
> > 
> > DENX Software Engineering GmbH,      Managing Director: Wolfgang
> > Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell,
> > Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email:
> > lukma at denx.de 
> 
> 



Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190905/c8916b4d/attachment.sig>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [U-Boot] [PATCH] imx: Introduce CONFIG_SPL_FORCE_MMC_BOOT to force MMC boot on falcon mode
  2019-09-05  8:08           ` Stefano Babic
  2019-09-05  9:42             ` Lukasz Majewski
@ 2019-09-05 10:02             ` Heiko Schocher
  1 sibling, 0 replies; 12+ messages in thread
From: Heiko Schocher @ 2019-09-05 10:02 UTC (permalink / raw)
  To: u-boot

Hello Stefano, Lukasz,

Am 05.09.2019 um 10:08 schrieb Stefano Babic:
> On 04/09/19 12:35, Lukasz Majewski wrote:
>> On Wed, 4 Sep 2019 11:54:40 +0200
>> Lukasz Majewski <lukma@denx.de> wrote:
>>
>>> Hi Stefano, Heiko,
>>>
>>>> On 04/09/19 10:46, Lukasz Majewski wrote:
>>>>> Hi Heiko,
>>>>>      
>>>>>> Hello Lukasz,
>>>>>>
>>>>>> added Stefano to cc as he is the imx custodian.
>>>>>
>>>>> I've relied on patman ... Thanks for adding Stefano to CC.
>>>>>      
>>>>>>
>>>>>> Am 03.09.2019 um 23:43 schrieb Lukasz Majewski:
>>>>>>> This change tries to fix the following problem:
>>>>>>>
>>>>>>> - The board boots (to be more precise - ROM loads SPL) from a
>>>>>>> slow SPI-NOR memory.
>>>>>>>     As a result the spl_boot_device() will return SPI-NOR as a
>>>>>>> boot device (which is correct).
>>>>>>>
>>>>>>> - The problem is that in 'falcon boot' the eMMC is used as a
>>>>>>> boot medium to load kernel from its partition.
>>>>>>>     Calling spl_boot_device() will break things as it returns
>>>>>>> SPI-NOR device.
>>>>>>>
>>>>>>> To fix this issue the new CONFIG_SPL_FORCE_MMC_BOOT Kconfig flag
>>>>>>> is introduced to handle this special use case. By default it is
>>>>>>> not defined, so there is no change in the legacy code flow.
>>>>>>>
>>>>>>> Signed-off-by: Lukasz Majewski <lukma@denx.de>
>>>>>>>
>>>>>>>
>>>>>>> ---
>>>>>>>
>>>>>>> This patch is a follow up of previous discussion/fix:
>>>>>>> https://patchwork.ozlabs.org/patch/796237/
>>>>>>>
>>>>>>> Travis-CI:
>>>>>>> https://travis-ci.org/lmajewski/u-boot-dfu/builds/580272414
>>>>>>> ---
>>>>>>>    arch/arm/mach-imx/spl.c | 11 +++++++++++
>>>>>>>    common/spl/Kconfig      |  7 +++++++
>>>>>>>    2 files changed, 18 insertions(+)
>>>>>>
>>>>>> I have just similiar change for an imx6ull based board ...
>>>>>
>>>>> Also one more of my boards uses this trick (i.MX28 based one).
>>>>>      
>>>>>> just antoher usecase: In factory empty board boots into USB SDP
>>>>>> mode, and SPL gets loaded with usb_loader ... SPL detects a sd
>>>>>> card (there is no sd card slot mounted, mmc boot is disabled!
>>>>>> They attach only one for installing software)
>>>>>
>>>>> So there is no dedicated SD card slot (also the mmc is disabled on
>>>>> that point).
>>>>> Instead, in the factory the sd card is attached to pads - just for
>>>>> this time.
>>>>>      
>>>>>> and boots U-Boot and system from sd card.
>>>>>> Than all software gets installed fully automated with the help of
>>>>>> swupdate ...
>>>>>
>>>>> Ok.
>>>>>      
>>>>>>     
>>>>>>> diff --git a/arch/arm/mach-imx/spl.c b/arch/arm/mach-imx/spl.c
>>>>>>> index 1f230aca3397..daa3d8f7ed94 100644
>>>>>>> --- a/arch/arm/mach-imx/spl.c
>>>>>>> +++ b/arch/arm/mach-imx/spl.c
>>>>>>> @@ -178,7 +178,18 @@ int g_dnl_bind_fixup(struct
>>>>>>> usb_device_descriptor *dev, const char *name) /* called from
>>>>>>> spl_mmc to see type of boot mode for storage (RAW or FAT) */ u32
>>>>>>> spl_boot_mode(const u32 boot_device) {
>>>>>>> +/*
>>>>>>> + * When CONFIG_SPL_FORCE_MMC_BOOT is defined the 'boot_device'
>>>>>>> is used
>>>>>>> + * unconditionally to decide about device to use for booting.
>>>>>>> + * This is crucial for falcon boot mode, when board boots up
>>>>>>> (i.e. ROM
>>>>>>> + * loads SPL) from slow SPI-NOR memory and afterwards the SPL's
>>>>>>> 'falcon' boot
>>>>>>> + * mode is used to load Linux OS from eMMC partition.
>>>>>>> + */
>>>>>>> +#ifdef CONFIG_SPL_FORCE_MMC_BOOT
>>>>>>> +	switch (boot_device) {
>>>>>>> +#else
>>>>>>>    	switch (spl_boot_device()) {
>>>>>>> +#endif
>>>>>>
>>>>>> Just dummy question .. couldn;t we switch to use always
>>>>>> boot_device?
>>>>>
>>>>> Stefano has provided some rationale for the need of
>>>>> spl_boot_device() in the previous version of this fix [1]:
>>>>>
>>>>> https://patchwork.ozlabs.org/patch/796237/
>>>>>
>>>>>      
>>>>>>
>>>>>> In my case I had a board specific:
>>>>>>
>>>>>> void board_boot_order(u32 *spl_boot_list)
>>>>>> {
>>>>>>           spl_boot_list[0] = BOOT_DEVICE_MMC1;
>>>>>>           spl_boot_list[1] = BOOT_DEVICE_SPI;
>>>>>>           spl_boot_list[2] = BOOT_DEVICE_BOARD;
>>>>>>           spl_boot_list[3] = BOOT_DEVICE_NONE;
>>>>>> }
>>>>>>
>>>>>> which leads that BOOT_DEVICE_MMC1 is passed to spl_boot_mode()
>>>>>>   
>>>>>
>>>>> Is your board normally booting from MMC or SPI-NOR (from where SoC
>>>>> ROM loads SPL) ?
>>>>>      
>>>>>>
>>>>>> If MMC1 is not found, it tries to boot U-Boot from SPI, and if
>>>>>> this is empty, as fallback board go into USB SDP mode.
>>>>>>
>>>>>> The weak function of board_boot_order is:
>>>>>>
>>>>>> __weak void board_boot_order(u32 *spl_boot_list)
>>>>>> {
>>>>>>           spl_boot_list[0] = spl_boot_device();
>>>>>> }
>>>>>>
>>>>>> which is exactly what we pass to spl_boot_mode() ... so instead
>>>>>> calling spl_boot_device() twice we can use the passed boot_device
>>>>>> ... and save the ifdef and new Kconfig option.
>>>>>>
>>>>>> But may I oversee something ?
>>>>>
>>>>> Please read the Stefano's reply from [1] - the spl_boot_device()
>>>>> has a valid use cases as well.
>>>>
>>>> Nevertheless, why do we need to add a new compiler switch if this
>>>> can be check into board code ? You just need that spl_boot_device()
>>>> returns the device you want (MMC for falcon boot).
>>>
>>> Current status:
>>>
>>> git grep -n "spl_boot_mode" | grep weak
>>> common/spl/spl_mmc.c:287:u32 __weak spl_boot_mode(const u32
>>> boot_device)
>>>
>>> git grep -n "board_boot_order" | grep weak
>>> common/spl/spl.c:491:__weak void board_boot_order(u32 *spl_boot_list)
>>>
>>>
>>>
>>>
>>> The spl_boot_device() is defined in arch/arm/mach-imx/spl.c [*] as a
>>> NON __weak function:
>>> git grep -n "spl_boot_device" | grep weak
>>>
>>>
>>>
>>> Shall I make this function [*] as __weak and provide override for it
>>> in the board file?
>>>
>>> The problem is in the call of spl_boot_mode() (which is overridden
>>> already in arch/arm/mach-imx/spl.c) at common/spl/spl_mmc.c
>>>
>>> Which then calls spl_boot_device() [**], which may return (correctly)
>>> SPI-NOR for eMMC.
>>>
>>>
>>> As is now spl_boot_device() is not a __weak function.
>>>
>>>> Why do you need to
>>>> force it in this way ?
>>>
>>> The option is to use the proposed patch to make spl_boot_device as a
>>> weak function.
>>
>> The above text is a bit misleading - better version below.
>>
>> The solution would be:
>>
>> 1. Use the proposed patch (with Kconfig option)
>>
>> 2. Define spl_boot_device as weak and override it in board file.
>>     However, it is not the preferred solution as spl_boot_device() on my
>>     setup correctly returns SPI-NOR (as SOC ROM loads SPL from SPI-NOR,
>>     not eMMC).
> 
> Ok, but is a wek not thought for this purpose ? If it is weak, you can
> change it and decide to return the device you want. You could also check
> if Falco boot is enabled, at compile time with CONFIG_SPL_OS_BOOT or at
> runtime with spl_start_uboot(). Should it not be enough ?

I am fine with a weak function only.

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [U-Boot] [PATCH] imx: Introduce CONFIG_SPL_FORCE_MMC_BOOT to force MMC boot on falcon mode
  2019-09-05  9:42             ` Lukasz Majewski
@ 2019-09-05 10:18               ` Heiko Schocher
  2019-09-09  8:02                 ` Lukasz Majewski
  0 siblings, 1 reply; 12+ messages in thread
From: Heiko Schocher @ 2019-09-05 10:18 UTC (permalink / raw)
  To: u-boot

Hello Lukasz,

Am 05.09.2019 um 11:42 schrieb Lukasz Majewski:
> Hi Stefano,
> 
>> On 04/09/19 12:35, Lukasz Majewski wrote:
>>> On Wed, 4 Sep 2019 11:54:40 +0200
>>> Lukasz Majewski <lukma@denx.de> wrote:
>>>    
>>>> Hi Stefano, Heiko,
>>>>   
>>>>> On 04/09/19 10:46, Lukasz Majewski wrote:
>>>>>> Hi Heiko,
>>>>>>        
>>>>>>> Hello Lukasz,
>>>>>>>
>>>>>>> added Stefano to cc as he is the imx custodian.
>>>>>>
>>>>>> I've relied on patman ... Thanks for adding Stefano to CC.
>>>>>>        
>>>>>>>
>>>>>>> Am 03.09.2019 um 23:43 schrieb Lukasz Majewski:
>>>>>>>> This change tries to fix the following problem:
>>>>>>>>
>>>>>>>> - The board boots (to be more precise - ROM loads SPL) from a
>>>>>>>> slow SPI-NOR memory.
>>>>>>>>     As a result the spl_boot_device() will return SPI-NOR as a
>>>>>>>> boot device (which is correct).
>>>>>>>>
>>>>>>>> - The problem is that in 'falcon boot' the eMMC is used as a
>>>>>>>> boot medium to load kernel from its partition.
>>>>>>>>     Calling spl_boot_device() will break things as it returns
>>>>>>>> SPI-NOR device.
>>>>>>>>
>>>>>>>> To fix this issue the new CONFIG_SPL_FORCE_MMC_BOOT Kconfig
>>>>>>>> flag is introduced to handle this special use case. By default
>>>>>>>> it is not defined, so there is no change in the legacy code
>>>>>>>> flow.
>>>>>>>>
>>>>>>>> Signed-off-by: Lukasz Majewski <lukma@denx.de>
>>>>>>>>
>>>>>>>>
>>>>>>>> ---
>>>>>>>>
>>>>>>>> This patch is a follow up of previous discussion/fix:
>>>>>>>> https://patchwork.ozlabs.org/patch/796237/
>>>>>>>>
>>>>>>>> Travis-CI:
>>>>>>>> https://travis-ci.org/lmajewski/u-boot-dfu/builds/580272414
>>>>>>>> ---
>>>>>>>>    arch/arm/mach-imx/spl.c | 11 +++++++++++
>>>>>>>>    common/spl/Kconfig      |  7 +++++++
>>>>>>>>    2 files changed, 18 insertions(+)
>>>>>>>
>>>>>>> I have just similiar change for an imx6ull based board ...
>>>>>>
>>>>>> Also one more of my boards uses this trick (i.MX28 based one).
>>>>>>        
>>>>>>> just antoher usecase: In factory empty board boots into USB SDP
>>>>>>> mode, and SPL gets loaded with usb_loader ... SPL detects a sd
>>>>>>> card (there is no sd card slot mounted, mmc boot is disabled!
>>>>>>> They attach only one for installing software)
>>>>>>
>>>>>> So there is no dedicated SD card slot (also the mmc is disabled
>>>>>> on that point).
>>>>>> Instead, in the factory the sd card is attached to pads - just
>>>>>> for this time.
>>>>>>        
>>>>>>> and boots U-Boot and system from sd card.
>>>>>>> Than all software gets installed fully automated with the help
>>>>>>> of swupdate ...
>>>>>>
>>>>>> Ok.
>>>>>>        
>>>>>>>       
>>>>>>>> diff --git a/arch/arm/mach-imx/spl.c b/arch/arm/mach-imx/spl.c
>>>>>>>> index 1f230aca3397..daa3d8f7ed94 100644
>>>>>>>> --- a/arch/arm/mach-imx/spl.c
>>>>>>>> +++ b/arch/arm/mach-imx/spl.c
>>>>>>>> @@ -178,7 +178,18 @@ int g_dnl_bind_fixup(struct
>>>>>>>> usb_device_descriptor *dev, const char *name) /* called from
>>>>>>>> spl_mmc to see type of boot mode for storage (RAW or FAT) */
>>>>>>>> u32 spl_boot_mode(const u32 boot_device) {
>>>>>>>> +/*
>>>>>>>> + * When CONFIG_SPL_FORCE_MMC_BOOT is defined the 'boot_device'
>>>>>>>> is used
>>>>>>>> + * unconditionally to decide about device to use for booting.
>>>>>>>> + * This is crucial for falcon boot mode, when board boots up
>>>>>>>> (i.e. ROM
>>>>>>>> + * loads SPL) from slow SPI-NOR memory and afterwards the
>>>>>>>> SPL's 'falcon' boot
>>>>>>>> + * mode is used to load Linux OS from eMMC partition.
>>>>>>>> + */
>>>>>>>> +#ifdef CONFIG_SPL_FORCE_MMC_BOOT
>>>>>>>> +	switch (boot_device) {
>>>>>>>> +#else
>>>>>>>>    	switch (spl_boot_device()) {
>>>>>>>> +#endif
>>>>>>>
>>>>>>> Just dummy question .. couldn;t we switch to use always
>>>>>>> boot_device?
>>>>>>
>>>>>> Stefano has provided some rationale for the need of
>>>>>> spl_boot_device() in the previous version of this fix [1]:
>>>>>>
>>>>>> https://patchwork.ozlabs.org/patch/796237/
>>>>>>
>>>>>>        
>>>>>>>
>>>>>>> In my case I had a board specific:
>>>>>>>
>>>>>>> void board_boot_order(u32 *spl_boot_list)
>>>>>>> {
>>>>>>>           spl_boot_list[0] = BOOT_DEVICE_MMC1;
>>>>>>>           spl_boot_list[1] = BOOT_DEVICE_SPI;
>>>>>>>           spl_boot_list[2] = BOOT_DEVICE_BOARD;
>>>>>>>           spl_boot_list[3] = BOOT_DEVICE_NONE;
>>>>>>> }
>>>>>>>
>>>>>>> which leads that BOOT_DEVICE_MMC1 is passed to spl_boot_mode()
>>>>>>>     
>>>>>>
>>>>>> Is your board normally booting from MMC or SPI-NOR (from where
>>>>>> SoC ROM loads SPL) ?
>>>>>>        
>>>>>>>
>>>>>>> If MMC1 is not found, it tries to boot U-Boot from SPI, and if
>>>>>>> this is empty, as fallback board go into USB SDP mode.
>>>>>>>
>>>>>>> The weak function of board_boot_order is:
>>>>>>>
>>>>>>> __weak void board_boot_order(u32 *spl_boot_list)
>>>>>>> {
>>>>>>>           spl_boot_list[0] = spl_boot_device();
>>>>>>> }
>>>>>>>
>>>>>>> which is exactly what we pass to spl_boot_mode() ... so instead
>>>>>>> calling spl_boot_device() twice we can use the passed
>>>>>>> boot_device ... and save the ifdef and new Kconfig option.
>>>>>>>
>>>>>>> But may I oversee something ?
>>>>>>
>>>>>> Please read the Stefano's reply from [1] - the spl_boot_device()
>>>>>> has a valid use cases as well.
>>>>>
>>>>> Nevertheless, why do we need to add a new compiler switch if this
>>>>> can be check into board code ? You just need that
>>>>> spl_boot_device() returns the device you want (MMC for falcon
>>>>> boot).
>>>>
>>>> Current status:
>>>>
>>>> git grep -n "spl_boot_mode" | grep weak
>>>> common/spl/spl_mmc.c:287:u32 __weak spl_boot_mode(const u32
>>>> boot_device)
>>>>
>>>> git grep -n "board_boot_order" | grep weak
>>>> common/spl/spl.c:491:__weak void board_boot_order(u32
>>>> *spl_boot_list)
>>>>
>>>>
>>>>
>>>>
>>>> The spl_boot_device() is defined in arch/arm/mach-imx/spl.c [*] as
>>>> a NON __weak function:
>>>> git grep -n "spl_boot_device" | grep weak
>>>>
>>>>
>>>>
>>>> Shall I make this function [*] as __weak and provide override for
>>>> it in the board file?
>>>>
>>>> The problem is in the call of spl_boot_mode() (which is overridden
>>>> already in arch/arm/mach-imx/spl.c) at common/spl/spl_mmc.c
>>>>
>>>> Which then calls spl_boot_device() [**], which may return
>>>> (correctly) SPI-NOR for eMMC.
>>>>
>>>>
>>>> As is now spl_boot_device() is not a __weak function.
>>>>   
>>>>> Why do you need to
>>>>> force it in this way ?
>>>>
>>>> The option is to use the proposed patch to make spl_boot_device as
>>>> a weak function.
>>>
>>> The above text is a bit misleading - better version below.
>>>
>>> The solution would be:
>>>
>>> 1. Use the proposed patch (with Kconfig option)
>>>
>>> 2. Define spl_boot_device as weak and override it in board file.
>>>     However, it is not the preferred solution as spl_boot_device()
>>> on my setup correctly returns SPI-NOR (as SOC ROM loads SPL from
>>> SPI-NOR, not eMMC).
>>
>> Ok, but is a wek not thought for this purpose ?
> 
> Making the spl_boot_device() __weak would be a fix only for a specific
> appearance of spl_boot_device() in arch/arm/mach-imx/spl.c (L178):
> 
> ------------------>8--------------------------
> #if defined(CONFIG_SPL_MMC_SUPPORT)
> 
> /* called from spl_mmc to see type of boot mode for storage (RAW or FAT) */
> u32 spl_boot_mode(const u32 boot_device)
> {
> 	switch (spl_boot_device()) {
> 	/* for MMC return either RAW or FAT mode */
> ------------------8<--------------------------
> 
> The above code is only executed when SPL_MMC_SUPPORT is enabled and
> IMHO the spl_boot_device() call assumes that one boots up from eMMC
> (but not from SPI-NOR).

Indeed, it is not that easy ... so forget my previous commment.

> I'm (in a first place) curious if this code is correct - the boot_device
> is passed as a parameter to this function, but then it is ignored.

Yes, that was also my first thought, so I simply replaced

diff --git a/arch/arm/mach-imx/spl.c b/arch/arm/mach-imx/spl.c
index 4023f916ee..7fabec206b 100644
--- a/arch/arm/mach-imx/spl.c
+++ b/arch/arm/mach-imx/spl.c
@@ -178,7 +178,7 @@ int g_dnl_bind_fixup(struct usb_device_descriptor *dev, const char *name)
  /* called from spl_mmc to see type of boot mode for storage (RAW or FAT) */
  u32 spl_boot_mode(const u32 boot_device)
  {
-       switch (spl_boot_device()) {
+       switch (boot_device) {
         /* for MMC return either RAW or FAT mode */
         case BOOT_DEVICE_MMC1:
         case BOOT_DEVICE_MMC2:

but I see Stefanos arguments:
https://patchwork.ozlabs.org/patch/796237/#1735852

So your current patch seems the best solution for me ...

bye,
Heiko


> 
> 
>> If it is weak, you can
>> change it and decide to return the device you want. You could also
>> check if Falco boot is enabled, at compile time with
>> CONFIG_SPL_OS_BOOT or at runtime with spl_start_uboot(). Should it
>> not be enough ?
>>
>> Regards,
>> Stefano
>>
>>>    
>>>>   
>>>>>
>>>>> Regards,
>>>>> Stefano
>>>>>      
>>>>>>        
>>>>>>>       
>>>>>>>>    	/* for MMC return either RAW or FAT mode */
>>>>>>>>    	case BOOT_DEVICE_MMC1:
>>>>>>>>    	case BOOT_DEVICE_MMC2:
>>>>>>>> diff --git a/common/spl/Kconfig b/common/spl/Kconfig
>>>>>>>> index f467eca2be72..3460beb62d59 100644
>>>>>>>> --- a/common/spl/Kconfig
>>>>>>>> +++ b/common/spl/Kconfig
>>>>>>>> @@ -607,6 +607,13 @@ config SPL_MMC_SUPPORT
>>>>>>>>    	  this option to build the drivers in drivers/mmc as
>>>>>>>> part of an SPL build.
>>>>>>>>    
>>>>>>>> +config SPL_FORCE_MMC_BOOT
>>>>>>>> +	bool "Force SPL's falcon boot from MMC"
>>>>>>>> +	depends on SPL_MMC_SUPPORT
>>>>>>>> +	default n
>>>>>>>> +	help
>>>>>>>> +	  Force SPL's falcon boot to use MMC device for Linux
>>>>>>>> kernel booting.
>>>>>>>
>>>>>>> Hmm... falcon boot is just one use case. I would drop "falcon
>>>>>>> boot" It just to force boot from MMC.
>>>>>>
>>>>>> Ok. I will rewrite the help message.
>>>>>>        
>>>>>>>       
>>>>>>>> +
>>>>>>>>    config SPL_MMC_TINY
>>>>>>>>    	bool "Tiny MMC framework in SPL"
>>>>>>>>    	depends on SPL_MMC_SUPPORT
>>>>>>>>          
>>>>>>>
>>>>>>> bye,
>>>>>>> Heiko
>>>>>>
>>>>>>
>>>>>>
>>>>>> Best regards,
>>>>>>
>>>>>> Lukasz Majewski
>>>>>>
>>>>>> --
>>>>>>
>>>>>> DENX Software Engineering GmbH,      Managing Director: Wolfgang
>>>>>> Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194
>>>>>> Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax:
>>>>>> (+49)-8142-66989-80 Email: lukma at denx.de
>>>>>
>>>>>      
>>>>
>>>>
>>>>
>>>> Best regards,
>>>>
>>>> Lukasz Majewski
>>>>
>>>> --
>>>>
>>>> DENX Software Engineering GmbH,      Managing Director: Wolfgang
>>>> Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell,
>>>> Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email:
>>>> lukma at denx.de
>>>
>>>
>>>
>>> Best regards,
>>>
>>> Lukasz Majewski
>>>
>>> --
>>>
>>> DENX Software Engineering GmbH,      Managing Director: Wolfgang
>>> Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell,
>>> Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email:
>>> lukma at denx.de
>>
>>
> 
> 
> 
> Best regards,
> 
> Lukasz Majewski
> 
> --
> 
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma at denx.de
> 

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [U-Boot] [PATCH] imx: Introduce CONFIG_SPL_FORCE_MMC_BOOT to force MMC boot on falcon mode
  2019-09-05 10:18               ` Heiko Schocher
@ 2019-09-09  8:02                 ` Lukasz Majewski
  2019-09-09 11:27                   ` Stefano Babic
  0 siblings, 1 reply; 12+ messages in thread
From: Lukasz Majewski @ 2019-09-09  8:02 UTC (permalink / raw)
  To: u-boot

Hi Heiko, Stefano

> Hello Lukasz,
> 
> Am 05.09.2019 um 11:42 schrieb Lukasz Majewski:
> > Hi Stefano,
> >   
> >> On 04/09/19 12:35, Lukasz Majewski wrote:  
> >>> On Wed, 4 Sep 2019 11:54:40 +0200
> >>> Lukasz Majewski <lukma@denx.de> wrote:
> >>>      
> >>>> Hi Stefano, Heiko,
> >>>>     
> >>>>> On 04/09/19 10:46, Lukasz Majewski wrote:  
> >>>>>> Hi Heiko,
> >>>>>>          
> >>>>>>> Hello Lukasz,
> >>>>>>>
> >>>>>>> added Stefano to cc as he is the imx custodian.  
> >>>>>>
> >>>>>> I've relied on patman ... Thanks for adding Stefano to CC.
> >>>>>>          
> >>>>>>>
> >>>>>>> Am 03.09.2019 um 23:43 schrieb Lukasz Majewski:  
> >>>>>>>> This change tries to fix the following problem:
> >>>>>>>>
> >>>>>>>> - The board boots (to be more precise - ROM loads SPL) from a
> >>>>>>>> slow SPI-NOR memory.
> >>>>>>>>     As a result the spl_boot_device() will return SPI-NOR as
> >>>>>>>> a boot device (which is correct).
> >>>>>>>>
> >>>>>>>> - The problem is that in 'falcon boot' the eMMC is used as a
> >>>>>>>> boot medium to load kernel from its partition.
> >>>>>>>>     Calling spl_boot_device() will break things as it returns
> >>>>>>>> SPI-NOR device.
> >>>>>>>>
> >>>>>>>> To fix this issue the new CONFIG_SPL_FORCE_MMC_BOOT Kconfig
> >>>>>>>> flag is introduced to handle this special use case. By
> >>>>>>>> default it is not defined, so there is no change in the
> >>>>>>>> legacy code flow.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Lukasz Majewski <lukma@denx.de>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> ---
> >>>>>>>>
> >>>>>>>> This patch is a follow up of previous discussion/fix:
> >>>>>>>> https://patchwork.ozlabs.org/patch/796237/
> >>>>>>>>
> >>>>>>>> Travis-CI:
> >>>>>>>> https://travis-ci.org/lmajewski/u-boot-dfu/builds/580272414
> >>>>>>>> ---
> >>>>>>>>    arch/arm/mach-imx/spl.c | 11 +++++++++++
> >>>>>>>>    common/spl/Kconfig      |  7 +++++++
> >>>>>>>>    2 files changed, 18 insertions(+)  
> >>>>>>>
> >>>>>>> I have just similiar change for an imx6ull based board ...  
> >>>>>>
> >>>>>> Also one more of my boards uses this trick (i.MX28 based one).
> >>>>>>          
> >>>>>>> just antoher usecase: In factory empty board boots into USB
> >>>>>>> SDP mode, and SPL gets loaded with usb_loader ... SPL detects
> >>>>>>> a sd card (there is no sd card slot mounted, mmc boot is
> >>>>>>> disabled! They attach only one for installing software)  
> >>>>>>
> >>>>>> So there is no dedicated SD card slot (also the mmc is disabled
> >>>>>> on that point).
> >>>>>> Instead, in the factory the sd card is attached to pads - just
> >>>>>> for this time.
> >>>>>>          
> >>>>>>> and boots U-Boot and system from sd card.
> >>>>>>> Than all software gets installed fully automated with the help
> >>>>>>> of swupdate ...  
> >>>>>>
> >>>>>> Ok.
> >>>>>>          
> >>>>>>>         
> >>>>>>>> diff --git a/arch/arm/mach-imx/spl.c
> >>>>>>>> b/arch/arm/mach-imx/spl.c index 1f230aca3397..daa3d8f7ed94
> >>>>>>>> 100644 --- a/arch/arm/mach-imx/spl.c
> >>>>>>>> +++ b/arch/arm/mach-imx/spl.c
> >>>>>>>> @@ -178,7 +178,18 @@ int g_dnl_bind_fixup(struct
> >>>>>>>> usb_device_descriptor *dev, const char *name) /* called from
> >>>>>>>> spl_mmc to see type of boot mode for storage (RAW or FAT) */
> >>>>>>>> u32 spl_boot_mode(const u32 boot_device) {
> >>>>>>>> +/*
> >>>>>>>> + * When CONFIG_SPL_FORCE_MMC_BOOT is defined the
> >>>>>>>> 'boot_device' is used
> >>>>>>>> + * unconditionally to decide about device to use for
> >>>>>>>> booting.
> >>>>>>>> + * This is crucial for falcon boot mode, when board boots up
> >>>>>>>> (i.e. ROM
> >>>>>>>> + * loads SPL) from slow SPI-NOR memory and afterwards the
> >>>>>>>> SPL's 'falcon' boot
> >>>>>>>> + * mode is used to load Linux OS from eMMC partition.
> >>>>>>>> + */
> >>>>>>>> +#ifdef CONFIG_SPL_FORCE_MMC_BOOT
> >>>>>>>> +	switch (boot_device) {
> >>>>>>>> +#else
> >>>>>>>>    	switch (spl_boot_device()) {
> >>>>>>>> +#endif  
> >>>>>>>
> >>>>>>> Just dummy question .. couldn;t we switch to use always
> >>>>>>> boot_device?  
> >>>>>>
> >>>>>> Stefano has provided some rationale for the need of
> >>>>>> spl_boot_device() in the previous version of this fix [1]:
> >>>>>>
> >>>>>> https://patchwork.ozlabs.org/patch/796237/
> >>>>>>
> >>>>>>          
> >>>>>>>
> >>>>>>> In my case I had a board specific:
> >>>>>>>
> >>>>>>> void board_boot_order(u32 *spl_boot_list)
> >>>>>>> {
> >>>>>>>           spl_boot_list[0] = BOOT_DEVICE_MMC1;
> >>>>>>>           spl_boot_list[1] = BOOT_DEVICE_SPI;
> >>>>>>>           spl_boot_list[2] = BOOT_DEVICE_BOARD;
> >>>>>>>           spl_boot_list[3] = BOOT_DEVICE_NONE;
> >>>>>>> }
> >>>>>>>
> >>>>>>> which leads that BOOT_DEVICE_MMC1 is passed to spl_boot_mode()
> >>>>>>>       
> >>>>>>
> >>>>>> Is your board normally booting from MMC or SPI-NOR (from where
> >>>>>> SoC ROM loads SPL) ?
> >>>>>>          
> >>>>>>>
> >>>>>>> If MMC1 is not found, it tries to boot U-Boot from SPI, and if
> >>>>>>> this is empty, as fallback board go into USB SDP mode.
> >>>>>>>
> >>>>>>> The weak function of board_boot_order is:
> >>>>>>>
> >>>>>>> __weak void board_boot_order(u32 *spl_boot_list)
> >>>>>>> {
> >>>>>>>           spl_boot_list[0] = spl_boot_device();
> >>>>>>> }
> >>>>>>>
> >>>>>>> which is exactly what we pass to spl_boot_mode() ... so
> >>>>>>> instead calling spl_boot_device() twice we can use the passed
> >>>>>>> boot_device ... and save the ifdef and new Kconfig option.
> >>>>>>>
> >>>>>>> But may I oversee something ?  
> >>>>>>
> >>>>>> Please read the Stefano's reply from [1] - the
> >>>>>> spl_boot_device() has a valid use cases as well.  
> >>>>>
> >>>>> Nevertheless, why do we need to add a new compiler switch if
> >>>>> this can be check into board code ? You just need that
> >>>>> spl_boot_device() returns the device you want (MMC for falcon
> >>>>> boot).  
> >>>>
> >>>> Current status:
> >>>>
> >>>> git grep -n "spl_boot_mode" | grep weak
> >>>> common/spl/spl_mmc.c:287:u32 __weak spl_boot_mode(const u32
> >>>> boot_device)
> >>>>
> >>>> git grep -n "board_boot_order" | grep weak
> >>>> common/spl/spl.c:491:__weak void board_boot_order(u32
> >>>> *spl_boot_list)
> >>>>
> >>>>
> >>>>
> >>>>
> >>>> The spl_boot_device() is defined in arch/arm/mach-imx/spl.c [*]
> >>>> as a NON __weak function:
> >>>> git grep -n "spl_boot_device" | grep weak
> >>>>
> >>>>
> >>>>
> >>>> Shall I make this function [*] as __weak and provide override for
> >>>> it in the board file?
> >>>>
> >>>> The problem is in the call of spl_boot_mode() (which is
> >>>> overridden already in arch/arm/mach-imx/spl.c) at
> >>>> common/spl/spl_mmc.c
> >>>>
> >>>> Which then calls spl_boot_device() [**], which may return
> >>>> (correctly) SPI-NOR for eMMC.
> >>>>
> >>>>
> >>>> As is now spl_boot_device() is not a __weak function.
> >>>>     
> >>>>> Why do you need to
> >>>>> force it in this way ?  
> >>>>
> >>>> The option is to use the proposed patch to make spl_boot_device
> >>>> as a weak function.  
> >>>
> >>> The above text is a bit misleading - better version below.
> >>>
> >>> The solution would be:
> >>>
> >>> 1. Use the proposed patch (with Kconfig option)
> >>>
> >>> 2. Define spl_boot_device as weak and override it in board file.
> >>>     However, it is not the preferred solution as spl_boot_device()
> >>> on my setup correctly returns SPI-NOR (as SOC ROM loads SPL from
> >>> SPI-NOR, not eMMC).  
> >>
> >> Ok, but is a wek not thought for this purpose ?  
> > 
> > Making the spl_boot_device() __weak would be a fix only for a
> > specific appearance of spl_boot_device() in arch/arm/mach-imx/spl.c
> > (L178): 
> > ------------------>8--------------------------  
> > #if defined(CONFIG_SPL_MMC_SUPPORT)
> > 
> > /* called from spl_mmc to see type of boot mode for storage (RAW or
> > FAT) */ u32 spl_boot_mode(const u32 boot_device)
> > {
> > 	switch (spl_boot_device()) {
> > 	/* for MMC return either RAW or FAT mode */
> > ------------------8<--------------------------
> > 
> > The above code is only executed when SPL_MMC_SUPPORT is enabled and
> > IMHO the spl_boot_device() call assumes that one boots up from eMMC
> > (but not from SPI-NOR).  
> 
> Indeed, it is not that easy ... so forget my previous commment.
> 
> > I'm (in a first place) curious if this code is correct - the
> > boot_device is passed as a parameter to this function, but then it
> > is ignored.  
> 
> Yes, that was also my first thought, so I simply replaced
> 
> diff --git a/arch/arm/mach-imx/spl.c b/arch/arm/mach-imx/spl.c
> index 4023f916ee..7fabec206b 100644
> --- a/arch/arm/mach-imx/spl.c
> +++ b/arch/arm/mach-imx/spl.c
> @@ -178,7 +178,7 @@ int g_dnl_bind_fixup(struct usb_device_descriptor
> *dev, const char *name) /* called from spl_mmc to see type of boot
> mode for storage (RAW or FAT) */ u32 spl_boot_mode(const u32
> boot_device) {
> -       switch (spl_boot_device()) {
> +       switch (boot_device) {
>          /* for MMC return either RAW or FAT mode */
>          case BOOT_DEVICE_MMC1:
>          case BOOT_DEVICE_MMC2:
> 
> but I see Stefanos arguments:
> https://patchwork.ozlabs.org/patch/796237/#1735852
> 
> So your current patch seems the best solution for me ...
> 

Can I assume that we do have a consensus here?

If yes - I will prepare v2 of this patch taking in the comments
regarding Heiko's production flashing comments.

> bye,
> Heiko
> 
> 
> > 
> >   
> >> If it is weak, you can
> >> change it and decide to return the device you want. You could also
> >> check if Falco boot is enabled, at compile time with
> >> CONFIG_SPL_OS_BOOT or at runtime with spl_start_uboot(). Should it
> >> not be enough ?
> >>
> >> Regards,
> >> Stefano
> >>  
> >>>      
> >>>>     
> >>>>>
> >>>>> Regards,
> >>>>> Stefano
> >>>>>        
> >>>>>>          
> >>>>>>>         
> >>>>>>>>    	/* for MMC return either RAW or FAT mode */
> >>>>>>>>    	case BOOT_DEVICE_MMC1:
> >>>>>>>>    	case BOOT_DEVICE_MMC2:
> >>>>>>>> diff --git a/common/spl/Kconfig b/common/spl/Kconfig
> >>>>>>>> index f467eca2be72..3460beb62d59 100644
> >>>>>>>> --- a/common/spl/Kconfig
> >>>>>>>> +++ b/common/spl/Kconfig
> >>>>>>>> @@ -607,6 +607,13 @@ config SPL_MMC_SUPPORT
> >>>>>>>>    	  this option to build the drivers in drivers/mmc
> >>>>>>>> as part of an SPL build.
> >>>>>>>>    
> >>>>>>>> +config SPL_FORCE_MMC_BOOT
> >>>>>>>> +	bool "Force SPL's falcon boot from MMC"
> >>>>>>>> +	depends on SPL_MMC_SUPPORT
> >>>>>>>> +	default n
> >>>>>>>> +	help
> >>>>>>>> +	  Force SPL's falcon boot to use MMC device for
> >>>>>>>> Linux kernel booting.  
> >>>>>>>
> >>>>>>> Hmm... falcon boot is just one use case. I would drop "falcon
> >>>>>>> boot" It just to force boot from MMC.  
> >>>>>>
> >>>>>> Ok. I will rewrite the help message.
> >>>>>>          
> >>>>>>>         
> >>>>>>>> +
> >>>>>>>>    config SPL_MMC_TINY
> >>>>>>>>    	bool "Tiny MMC framework in SPL"
> >>>>>>>>    	depends on SPL_MMC_SUPPORT
> >>>>>>>>            
> >>>>>>>
> >>>>>>> bye,
> >>>>>>> Heiko  
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>> Best regards,
> >>>>>>
> >>>>>> Lukasz Majewski
> >>>>>>
> >>>>>> --
> >>>>>>
> >>>>>> DENX Software Engineering GmbH,      Managing Director:
> >>>>>> Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194
> >>>>>> Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax:
> >>>>>> (+49)-8142-66989-80 Email: lukma at denx.de  
> >>>>>
> >>>>>        
> >>>>
> >>>>
> >>>>
> >>>> Best regards,
> >>>>
> >>>> Lukasz Majewski
> >>>>
> >>>> --
> >>>>
> >>>> DENX Software Engineering GmbH,      Managing Director: Wolfgang
> >>>> Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194
> >>>> Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax:
> >>>> (+49)-8142-66989-80 Email: lukma at denx.de  
> >>>
> >>>
> >>>
> >>> Best regards,
> >>>
> >>> Lukasz Majewski
> >>>
> >>> --
> >>>
> >>> DENX Software Engineering GmbH,      Managing Director: Wolfgang
> >>> Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell,
> >>> Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email:
> >>> lukma at denx.de  
> >>
> >>  
> > 
> > 
> > 
> > Best regards,
> > 
> > Lukasz Majewski
> > 
> > --
> > 
> > DENX Software Engineering GmbH,      Managing Director: Wolfgang
> > Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell,
> > Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email:
> > lukma at denx.de 
> 



Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190909/b6a952d9/attachment.sig>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [U-Boot] [PATCH] imx: Introduce CONFIG_SPL_FORCE_MMC_BOOT to force MMC boot on falcon mode
  2019-09-09  8:02                 ` Lukasz Majewski
@ 2019-09-09 11:27                   ` Stefano Babic
  0 siblings, 0 replies; 12+ messages in thread
From: Stefano Babic @ 2019-09-09 11:27 UTC (permalink / raw)
  To: u-boot

On 09/09/19 10:02, Lukasz Majewski wrote:
> Hi Heiko, Stefano
> 
>> Hello Lukasz,
>>
>> Am 05.09.2019 um 11:42 schrieb Lukasz Majewski:
>>> Hi Stefano,
>>>   
>>>> On 04/09/19 12:35, Lukasz Majewski wrote:  
>>>>> On Wed, 4 Sep 2019 11:54:40 +0200
>>>>> Lukasz Majewski <lukma@denx.de> wrote:
>>>>>      
>>>>>> Hi Stefano, Heiko,
>>>>>>     
>>>>>>> On 04/09/19 10:46, Lukasz Majewski wrote:  
>>>>>>>> Hi Heiko,
>>>>>>>>          
>>>>>>>>> Hello Lukasz,
>>>>>>>>>
>>>>>>>>> added Stefano to cc as he is the imx custodian.  
>>>>>>>>
>>>>>>>> I've relied on patman ... Thanks for adding Stefano to CC.
>>>>>>>>          
>>>>>>>>>
>>>>>>>>> Am 03.09.2019 um 23:43 schrieb Lukasz Majewski:  
>>>>>>>>>> This change tries to fix the following problem:
>>>>>>>>>>
>>>>>>>>>> - The board boots (to be more precise - ROM loads SPL) from a
>>>>>>>>>> slow SPI-NOR memory.
>>>>>>>>>>     As a result the spl_boot_device() will return SPI-NOR as
>>>>>>>>>> a boot device (which is correct).
>>>>>>>>>>
>>>>>>>>>> - The problem is that in 'falcon boot' the eMMC is used as a
>>>>>>>>>> boot medium to load kernel from its partition.
>>>>>>>>>>     Calling spl_boot_device() will break things as it returns
>>>>>>>>>> SPI-NOR device.
>>>>>>>>>>
>>>>>>>>>> To fix this issue the new CONFIG_SPL_FORCE_MMC_BOOT Kconfig
>>>>>>>>>> flag is introduced to handle this special use case. By
>>>>>>>>>> default it is not defined, so there is no change in the
>>>>>>>>>> legacy code flow.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Lukasz Majewski <lukma@denx.de>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> ---
>>>>>>>>>>
>>>>>>>>>> This patch is a follow up of previous discussion/fix:
>>>>>>>>>> https://patchwork.ozlabs.org/patch/796237/
>>>>>>>>>>
>>>>>>>>>> Travis-CI:
>>>>>>>>>> https://travis-ci.org/lmajewski/u-boot-dfu/builds/580272414
>>>>>>>>>> ---
>>>>>>>>>>    arch/arm/mach-imx/spl.c | 11 +++++++++++
>>>>>>>>>>    common/spl/Kconfig      |  7 +++++++
>>>>>>>>>>    2 files changed, 18 insertions(+)  
>>>>>>>>>
>>>>>>>>> I have just similiar change for an imx6ull based board ...  
>>>>>>>>
>>>>>>>> Also one more of my boards uses this trick (i.MX28 based one).
>>>>>>>>          
>>>>>>>>> just antoher usecase: In factory empty board boots into USB
>>>>>>>>> SDP mode, and SPL gets loaded with usb_loader ... SPL detects
>>>>>>>>> a sd card (there is no sd card slot mounted, mmc boot is
>>>>>>>>> disabled! They attach only one for installing software)  
>>>>>>>>
>>>>>>>> So there is no dedicated SD card slot (also the mmc is disabled
>>>>>>>> on that point).
>>>>>>>> Instead, in the factory the sd card is attached to pads - just
>>>>>>>> for this time.
>>>>>>>>          
>>>>>>>>> and boots U-Boot and system from sd card.
>>>>>>>>> Than all software gets installed fully automated with the help
>>>>>>>>> of swupdate ...  
>>>>>>>>
>>>>>>>> Ok.
>>>>>>>>          
>>>>>>>>>         
>>>>>>>>>> diff --git a/arch/arm/mach-imx/spl.c
>>>>>>>>>> b/arch/arm/mach-imx/spl.c index 1f230aca3397..daa3d8f7ed94
>>>>>>>>>> 100644 --- a/arch/arm/mach-imx/spl.c
>>>>>>>>>> +++ b/arch/arm/mach-imx/spl.c
>>>>>>>>>> @@ -178,7 +178,18 @@ int g_dnl_bind_fixup(struct
>>>>>>>>>> usb_device_descriptor *dev, const char *name) /* called from
>>>>>>>>>> spl_mmc to see type of boot mode for storage (RAW or FAT) */
>>>>>>>>>> u32 spl_boot_mode(const u32 boot_device) {
>>>>>>>>>> +/*
>>>>>>>>>> + * When CONFIG_SPL_FORCE_MMC_BOOT is defined the
>>>>>>>>>> 'boot_device' is used
>>>>>>>>>> + * unconditionally to decide about device to use for
>>>>>>>>>> booting.
>>>>>>>>>> + * This is crucial for falcon boot mode, when board boots up
>>>>>>>>>> (i.e. ROM
>>>>>>>>>> + * loads SPL) from slow SPI-NOR memory and afterwards the
>>>>>>>>>> SPL's 'falcon' boot
>>>>>>>>>> + * mode is used to load Linux OS from eMMC partition.
>>>>>>>>>> + */
>>>>>>>>>> +#ifdef CONFIG_SPL_FORCE_MMC_BOOT
>>>>>>>>>> +	switch (boot_device) {
>>>>>>>>>> +#else
>>>>>>>>>>    	switch (spl_boot_device()) {
>>>>>>>>>> +#endif  
>>>>>>>>>
>>>>>>>>> Just dummy question .. couldn;t we switch to use always
>>>>>>>>> boot_device?  
>>>>>>>>
>>>>>>>> Stefano has provided some rationale for the need of
>>>>>>>> spl_boot_device() in the previous version of this fix [1]:
>>>>>>>>
>>>>>>>> https://patchwork.ozlabs.org/patch/796237/
>>>>>>>>
>>>>>>>>          
>>>>>>>>>
>>>>>>>>> In my case I had a board specific:
>>>>>>>>>
>>>>>>>>> void board_boot_order(u32 *spl_boot_list)
>>>>>>>>> {
>>>>>>>>>           spl_boot_list[0] = BOOT_DEVICE_MMC1;
>>>>>>>>>           spl_boot_list[1] = BOOT_DEVICE_SPI;
>>>>>>>>>           spl_boot_list[2] = BOOT_DEVICE_BOARD;
>>>>>>>>>           spl_boot_list[3] = BOOT_DEVICE_NONE;
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> which leads that BOOT_DEVICE_MMC1 is passed to spl_boot_mode()
>>>>>>>>>       
>>>>>>>>
>>>>>>>> Is your board normally booting from MMC or SPI-NOR (from where
>>>>>>>> SoC ROM loads SPL) ?
>>>>>>>>          
>>>>>>>>>
>>>>>>>>> If MMC1 is not found, it tries to boot U-Boot from SPI, and if
>>>>>>>>> this is empty, as fallback board go into USB SDP mode.
>>>>>>>>>
>>>>>>>>> The weak function of board_boot_order is:
>>>>>>>>>
>>>>>>>>> __weak void board_boot_order(u32 *spl_boot_list)
>>>>>>>>> {
>>>>>>>>>           spl_boot_list[0] = spl_boot_device();
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> which is exactly what we pass to spl_boot_mode() ... so
>>>>>>>>> instead calling spl_boot_device() twice we can use the passed
>>>>>>>>> boot_device ... and save the ifdef and new Kconfig option.
>>>>>>>>>
>>>>>>>>> But may I oversee something ?  
>>>>>>>>
>>>>>>>> Please read the Stefano's reply from [1] - the
>>>>>>>> spl_boot_device() has a valid use cases as well.  
>>>>>>>
>>>>>>> Nevertheless, why do we need to add a new compiler switch if
>>>>>>> this can be check into board code ? You just need that
>>>>>>> spl_boot_device() returns the device you want (MMC for falcon
>>>>>>> boot).  
>>>>>>
>>>>>> Current status:
>>>>>>
>>>>>> git grep -n "spl_boot_mode" | grep weak
>>>>>> common/spl/spl_mmc.c:287:u32 __weak spl_boot_mode(const u32
>>>>>> boot_device)
>>>>>>
>>>>>> git grep -n "board_boot_order" | grep weak
>>>>>> common/spl/spl.c:491:__weak void board_boot_order(u32
>>>>>> *spl_boot_list)
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> The spl_boot_device() is defined in arch/arm/mach-imx/spl.c [*]
>>>>>> as a NON __weak function:
>>>>>> git grep -n "spl_boot_device" | grep weak
>>>>>>
>>>>>>
>>>>>>
>>>>>> Shall I make this function [*] as __weak and provide override for
>>>>>> it in the board file?
>>>>>>
>>>>>> The problem is in the call of spl_boot_mode() (which is
>>>>>> overridden already in arch/arm/mach-imx/spl.c) at
>>>>>> common/spl/spl_mmc.c
>>>>>>
>>>>>> Which then calls spl_boot_device() [**], which may return
>>>>>> (correctly) SPI-NOR for eMMC.
>>>>>>
>>>>>>
>>>>>> As is now spl_boot_device() is not a __weak function.
>>>>>>     
>>>>>>> Why do you need to
>>>>>>> force it in this way ?  
>>>>>>
>>>>>> The option is to use the proposed patch to make spl_boot_device
>>>>>> as a weak function.  
>>>>>
>>>>> The above text is a bit misleading - better version below.
>>>>>
>>>>> The solution would be:
>>>>>
>>>>> 1. Use the proposed patch (with Kconfig option)
>>>>>
>>>>> 2. Define spl_boot_device as weak and override it in board file.
>>>>>     However, it is not the preferred solution as spl_boot_device()
>>>>> on my setup correctly returns SPI-NOR (as SOC ROM loads SPL from
>>>>> SPI-NOR, not eMMC).  
>>>>
>>>> Ok, but is a wek not thought for this purpose ?  
>>>
>>> Making the spl_boot_device() __weak would be a fix only for a
>>> specific appearance of spl_boot_device() in arch/arm/mach-imx/spl.c
>>> (L178): 
>>> ------------------>8--------------------------  
>>> #if defined(CONFIG_SPL_MMC_SUPPORT)
>>>
>>> /* called from spl_mmc to see type of boot mode for storage (RAW or
>>> FAT) */ u32 spl_boot_mode(const u32 boot_device)
>>> {
>>> 	switch (spl_boot_device()) {
>>> 	/* for MMC return either RAW or FAT mode */
>>> ------------------8<--------------------------
>>>
>>> The above code is only executed when SPL_MMC_SUPPORT is enabled and
>>> IMHO the spl_boot_device() call assumes that one boots up from eMMC
>>> (but not from SPI-NOR).  
>>
>> Indeed, it is not that easy ... so forget my previous commment.
>>
>>> I'm (in a first place) curious if this code is correct - the
>>> boot_device is passed as a parameter to this function, but then it
>>> is ignored.  
>>
>> Yes, that was also my first thought, so I simply replaced
>>
>> diff --git a/arch/arm/mach-imx/spl.c b/arch/arm/mach-imx/spl.c
>> index 4023f916ee..7fabec206b 100644
>> --- a/arch/arm/mach-imx/spl.c
>> +++ b/arch/arm/mach-imx/spl.c
>> @@ -178,7 +178,7 @@ int g_dnl_bind_fixup(struct usb_device_descriptor
>> *dev, const char *name) /* called from spl_mmc to see type of boot
>> mode for storage (RAW or FAT) */ u32 spl_boot_mode(const u32
>> boot_device) {
>> -       switch (spl_boot_device()) {
>> +       switch (boot_device) {
>>          /* for MMC return either RAW or FAT mode */
>>          case BOOT_DEVICE_MMC1:
>>          case BOOT_DEVICE_MMC2:
>>
>> but I see Stefanos arguments:
>> https://patchwork.ozlabs.org/patch/796237/#1735852
>>
>> So your current patch seems the best solution for me ...
>>
> 
> Can I assume that we do have a consensus here?
> 
> If yes - I will prepare v2 of this patch taking in the comments
> regarding Heiko's production flashing comments.

ok, fine.

Regards,
Stefano

> 
>> bye,
>> Heiko
>>
>>
>>>
>>>   
>>>> If it is weak, you can
>>>> change it and decide to return the device you want. You could also
>>>> check if Falco boot is enabled, at compile time with
>>>> CONFIG_SPL_OS_BOOT or at runtime with spl_start_uboot(). Should it
>>>> not be enough ?
>>>>
>>>> Regards,
>>>> Stefano
>>>>  
>>>>>      
>>>>>>     
>>>>>>>
>>>>>>> Regards,
>>>>>>> Stefano
>>>>>>>        
>>>>>>>>          
>>>>>>>>>         
>>>>>>>>>>    	/* for MMC return either RAW or FAT mode */
>>>>>>>>>>    	case BOOT_DEVICE_MMC1:
>>>>>>>>>>    	case BOOT_DEVICE_MMC2:
>>>>>>>>>> diff --git a/common/spl/Kconfig b/common/spl/Kconfig
>>>>>>>>>> index f467eca2be72..3460beb62d59 100644
>>>>>>>>>> --- a/common/spl/Kconfig
>>>>>>>>>> +++ b/common/spl/Kconfig
>>>>>>>>>> @@ -607,6 +607,13 @@ config SPL_MMC_SUPPORT
>>>>>>>>>>    	  this option to build the drivers in drivers/mmc
>>>>>>>>>> as part of an SPL build.
>>>>>>>>>>    
>>>>>>>>>> +config SPL_FORCE_MMC_BOOT
>>>>>>>>>> +	bool "Force SPL's falcon boot from MMC"
>>>>>>>>>> +	depends on SPL_MMC_SUPPORT
>>>>>>>>>> +	default n
>>>>>>>>>> +	help
>>>>>>>>>> +	  Force SPL's falcon boot to use MMC device for
>>>>>>>>>> Linux kernel booting.  
>>>>>>>>>
>>>>>>>>> Hmm... falcon boot is just one use case. I would drop "falcon
>>>>>>>>> boot" It just to force boot from MMC.  
>>>>>>>>
>>>>>>>> Ok. I will rewrite the help message.
>>>>>>>>          
>>>>>>>>>         
>>>>>>>>>> +
>>>>>>>>>>    config SPL_MMC_TINY
>>>>>>>>>>    	bool "Tiny MMC framework in SPL"
>>>>>>>>>>    	depends on SPL_MMC_SUPPORT
>>>>>>>>>>            
>>>>>>>>>
>>>>>>>>> bye,
>>>>>>>>> Heiko  
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Best regards,
>>>>>>>>
>>>>>>>> Lukasz Majewski
>>>>>>>>
>>>>>>>> --
>>>>>>>>
>>>>>>>> DENX Software Engineering GmbH,      Managing Director:
>>>>>>>> Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194
>>>>>>>> Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax:
>>>>>>>> (+49)-8142-66989-80 Email: lukma at denx.de  
>>>>>>>
>>>>>>>        
>>>>>>
>>>>>>
>>>>>>
>>>>>> Best regards,
>>>>>>
>>>>>> Lukasz Majewski
>>>>>>
>>>>>> --
>>>>>>
>>>>>> DENX Software Engineering GmbH,      Managing Director: Wolfgang
>>>>>> Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194
>>>>>> Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax:
>>>>>> (+49)-8142-66989-80 Email: lukma at denx.de  
>>>>>
>>>>>
>>>>>
>>>>> Best regards,
>>>>>
>>>>> Lukasz Majewski
>>>>>
>>>>> --
>>>>>
>>>>> DENX Software Engineering GmbH,      Managing Director: Wolfgang
>>>>> Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell,
>>>>> Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email:
>>>>> lukma at denx.de  
>>>>
>>>>  
>>>
>>>
>>>
>>> Best regards,
>>>
>>> Lukasz Majewski
>>>
>>> --
>>>
>>> DENX Software Engineering GmbH,      Managing Director: Wolfgang
>>> Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell,
>>> Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email:
>>> lukma at denx.de 
>>
> 
> 
> 
> Best regards,
> 
> Lukasz Majewski
> 
> --
> 
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma at denx.de
> 


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

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2019-09-09 11:27 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-03 21:43 [U-Boot] [PATCH] imx: Introduce CONFIG_SPL_FORCE_MMC_BOOT to force MMC boot on falcon mode Lukasz Majewski
2019-09-04  4:45 ` Heiko Schocher
2019-09-04  8:46   ` Lukasz Majewski
2019-09-04  8:59     ` Stefano Babic
2019-09-04  9:54       ` Lukasz Majewski
2019-09-04 10:35         ` Lukasz Majewski
2019-09-05  8:08           ` Stefano Babic
2019-09-05  9:42             ` Lukasz Majewski
2019-09-05 10:18               ` Heiko Schocher
2019-09-09  8:02                 ` Lukasz Majewski
2019-09-09 11:27                   ` Stefano Babic
2019-09-05 10:02             ` Heiko Schocher

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.