diff mbox

[U-Boot] imx_common: spl: Fix SPL boot from SD card

Message ID 1480525936-23418-1-git-send-email-breno.lima@nxp.com
State Rejected
Delegated to: Stefano Babic
Headers show

Commit Message

Breno Matheus Lima Nov. 30, 2016, 5:12 p.m. UTC
Since commit 54e4fcfa3c749a78 ("ARM: mx6: add MMC2 boot device detection
support in SPL") we can no longer boot from SD card on a mx6sx Udoo neo board:

U-Boot SPL 2016.11-32108-g9cd37b0 (Nov 30 2016 - 11:05:54)                      
Trying to boot from MMC2                                                        
MMC Device 1 not found                                                          
spl: could not find mmc device. error: -19                                      
SPL: failed to boot from all boot devices                                       
### ERROR ### Please RESET the board ###

Prior to this commit we had the following logic:

	/* SD/eSD: 8.5.3, Table 8-15  */
	case 0x4:
	case 0x5:
		return BOOT_DEVICE_MMC1;
	/* MMC/eMMC: 8.5.3 */
	case 0x6:
	case 0x7:
		return BOOT_DEVICE_MMC1;

and in the case of MX6SX Udoo board we entered the case 0x5 (SD card boot)
and returned BOOT_DEVICE_MMC1 as expected.

Now this does not happen anymore and BOOT_DEVICE_MMC2 is returned, which
breaks the boot.

In order to fix this problem keep the original behavior for the SD card case by
returning BOOT_DEVICE_MMC1.

Tested on mx6cuboxi and mx6sx UDOO neo boards.

Signed-off-by: Breno Lima <breno.lima@nxp.com>
---
 arch/arm/imx-common/spl.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Fabio Estevam Nov. 30, 2016, 5:43 p.m. UTC | #1
On Wed, Nov 30, 2016 at 3:12 PM, Breno Lima <breno.lima@nxp.com> wrote:
> Since commit 54e4fcfa3c749a78 ("ARM: mx6: add MMC2 boot device detection
> support in SPL") we can no longer boot from SD card on a mx6sx Udoo neo board:
>
> U-Boot SPL 2016.11-32108-g9cd37b0 (Nov 30 2016 - 11:05:54)
> Trying to boot from MMC2
> MMC Device 1 not found
> spl: could not find mmc device. error: -19
> SPL: failed to boot from all boot devices
> ### ERROR ### Please RESET the board ###
>
> Prior to this commit we had the following logic:
>
>         /* SD/eSD: 8.5.3, Table 8-15  */
>         case 0x4:
>         case 0x5:
>                 return BOOT_DEVICE_MMC1;
>         /* MMC/eMMC: 8.5.3 */
>         case 0x6:
>         case 0x7:
>                 return BOOT_DEVICE_MMC1;
>
> and in the case of MX6SX Udoo board we entered the case 0x5 (SD card boot)
> and returned BOOT_DEVICE_MMC1 as expected.
>
> Now this does not happen anymore and BOOT_DEVICE_MMC2 is returned, which
> breaks the boot.
>
> In order to fix this problem keep the original behavior for the SD card case by
> returning BOOT_DEVICE_MMC1.
>
> Tested on mx6cuboxi and mx6sx UDOO neo boards.
>
> Signed-off-by: Breno Lima <breno.lima@nxp.com>

Makes sense, thanks:

Reviewed-by: Fabio Estevam <fabio.estevam@nxp.com>
Marcin Niestroj Dec. 2, 2016, 2:11 p.m. UTC | #2
Hi,

On 30.11.2016 18:12, Breno Lima wrote:
> Since commit 54e4fcfa3c749a78 ("ARM: mx6: add MMC2 boot device detection
> support in SPL") we can no longer boot from SD card on a mx6sx Udoo neo board:
>
> U-Boot SPL 2016.11-32108-g9cd37b0 (Nov 30 2016 - 11:05:54)
> Trying to boot from MMC2
> MMC Device 1 not found
> spl: could not find mmc device. error: -19
> SPL: failed to boot from all boot devices
> ### ERROR ### Please RESET the board ###
>
> Prior to this commit we had the following logic:
>
> 	/* SD/eSD: 8.5.3, Table 8-15  */
> 	case 0x4:
> 	case 0x5:
> 		return BOOT_DEVICE_MMC1;
> 	/* MMC/eMMC: 8.5.3 */
> 	case 0x6:
> 	case 0x7:
> 		return BOOT_DEVICE_MMC1;
>
> and in the case of MX6SX Udoo board we entered the case 0x5 (SD card boot)
> and returned BOOT_DEVICE_MMC1 as expected.

Why is BOOT_DEVICE_MMC1 expected here? As I see in Udoo neo schematics
the SD card is connected to uSDHC2 port, because uSDHC1 is used for
something else.

>
> Now this does not happen anymore and BOOT_DEVICE_MMC2 is returned, which
> breaks the boot.
>
> In order to fix this problem keep the original behavior for the SD card case by
> returning BOOT_DEVICE_MMC1.
>
> Tested on mx6cuboxi and mx6sx UDOO neo boards.
>
> Signed-off-by: Breno Lima <breno.lima@nxp.com>
> ---
>  arch/arm/imx-common/spl.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/arch/arm/imx-common/spl.c b/arch/arm/imx-common/spl.c
> index 325ba26..cbcd936 100644
> --- a/arch/arm/imx-common/spl.c
> +++ b/arch/arm/imx-common/spl.c
> @@ -58,6 +58,7 @@ u32 spl_boot_device(void)
>  	/* SD/eSD: 8.5.3, Table 8-15  */
>  	case 0x4:
>  	case 0x5:
> +		 return BOOT_DEVICE_MMC1;
>  	/* MMC/eMMC: 8.5.3 */
>  	case 0x6:
>  	case 0x7:
>
Fabio Estevam Dec. 2, 2016, 3:03 p.m. UTC | #3
Hi Marcin,

On Fri, Dec 2, 2016 at 12:11 PM, Marcin Niestroj
<m.niestroj@grinn-global.com> wrote:

> Why is BOOT_DEVICE_MMC1 expected here? As I see in Udoo neo schematics
> the SD card is connected to uSDHC2 port, because uSDHC1 is used for
> something else.

Yes, Udoo Neo boots from the SDHC2 port.

However BOOT_DEVICE_MMC1 does not mean "boot from the SDHC1 port".
BOOT_DEVICE_MMC1 just tells SPL to boot from the first registered mmc device.

Please take a look at common/spl/spl_mmc.c

int spl_mmc_get_device_index(u32 boot_device)
{
  switch (boot_device) {
  case BOOT_DEVICE_MMC1:
      return 0;
  case BOOT_DEVICE_MMC2:
  case BOOT_DEVICE_MMC2_2:
    return 1;
}

,so we should return BOOT_DEVICE_MMC1 as we always did.

mx6cuboxi is also broken for the same reason.

If you took the assumption that in "BOOT_DEVICE_MMC1 = Boot from SDHC1
port" in 54e4fcfa3c749a78 ("ARM: mx6: add MMC2 boot device detection
support in SPL") I suggest we need to revert it.
Marcin Niestroj Dec. 2, 2016, 5:16 p.m. UTC | #4
On 02.12.2016 16:03, Fabio Estevam wrote:
> Hi Marcin,
>
> On Fri, Dec 2, 2016 at 12:11 PM, Marcin Niestroj
> <m.niestroj@grinn-global.com> wrote:
>
>> Why is BOOT_DEVICE_MMC1 expected here? As I see in Udoo neo schematics
>> the SD card is connected to uSDHC2 port, because uSDHC1 is used for
>> something else.
>
> Yes, Udoo Neo boots from the SDHC2 port.
>
> However BOOT_DEVICE_MMC1 does not mean "boot from the SDHC1 port".
> BOOT_DEVICE_MMC1 just tells SPL to boot from the first registered mmc device.
>
> Please take a look at common/spl/spl_mmc.c
>
> int spl_mmc_get_device_index(u32 boot_device)
> {
>   switch (boot_device) {
>   case BOOT_DEVICE_MMC1:
>       return 0;
>   case BOOT_DEVICE_MMC2:
>   case BOOT_DEVICE_MMC2_2:
>     return 1;
> }
>
> ,so we should return BOOT_DEVICE_MMC1 as we always did.
>
> mx6cuboxi is also broken for the same reason.
>
> If you took the assumption that in "BOOT_DEVICE_MMC1 = Boot from SDHC1
> port" in 54e4fcfa3c749a78 ("ARM: mx6: add MMC2 boot device detection
> support in SPL") I suggest we need to revert it.
>

Indeed it was my assumption. I agree it should be reverted then.

So to support booting from two SD/MMC devices one should create
board_boot_order() function? That way BOOT_CFG2[3:4] can be read there
to figure out which MMC1 or MMC2 should be configured as the first boot
device.
Fabio Estevam Dec. 2, 2016, 7:05 p.m. UTC | #5
On Fri, Dec 2, 2016 at 3:16 PM, Marcin Niestroj
<m.niestroj@grinn-global.com> wrote:

>> If you took the assumption that in "BOOT_DEVICE_MMC1 = Boot from SDHC1
>> port" in 54e4fcfa3c749a78 ("ARM: mx6: add MMC2 boot device detection
>> support in SPL") I suggest we need to revert it.
>>
>
> Indeed it was my assumption. I agree it should be reverted then.

Thanks for confirming.

Stefano, would you like to take Breno's original patch with the
revert? Or would you like me to resend it with an improved commit log
explaining the reason for the breakage?

> So to support booting from two SD/MMC devices one should create
> board_boot_order() function? That way BOOT_CFG2[3:4] can be read there
> to figure out which MMC1 or MMC2 should be configured as the first boot
> device.

If you want to determine the SD/MMC port dynamically it seems like a solution.

Or if you always know the SD/MMC port you will boot from then you can
simply register the associated SDHC port first.
Stefano Babic Dec. 5, 2016, 11:47 a.m. UTC | #6
Hi Fabio,

On 02/12/2016 20:05, Fabio Estevam wrote:
> On Fri, Dec 2, 2016 at 3:16 PM, Marcin Niestroj
> <m.niestroj@grinn-global.com> wrote:
> 
>>> If you took the assumption that in "BOOT_DEVICE_MMC1 = Boot from SDHC1
>>> port" in 54e4fcfa3c749a78 ("ARM: mx6: add MMC2 boot device detection
>>> support in SPL") I suggest we need to revert it.
>>>
>>
>> Indeed it was my assumption. I agree it should be reverted then.
> 
> Thanks for confirming.
> 
> Stefano, would you like to take Breno's original patch with the
> revert?

I think it is ok to take Breno's original patch. It was just a
misunderstanding and how boot device order is set was cleared in this
thread.

Best regards,
Stefano Babic


> Or would you like me to resend it with an improved commit log
> explaining the reason for the breakage?
> 
>> So to support booting from two SD/MMC devices one should create
>> board_boot_order() function? That way BOOT_CFG2[3:4] can be read there
>> to figure out which MMC1 or MMC2 should be configured as the first boot
>> device.
> 
> If you want to determine the SD/MMC port dynamically it seems like a solution.
> 
> Or if you always know the SD/MMC port you will boot from then you can
> simply register the associated SDHC port first.
>
diff mbox

Patch

diff --git a/arch/arm/imx-common/spl.c b/arch/arm/imx-common/spl.c
index 325ba26..cbcd936 100644
--- a/arch/arm/imx-common/spl.c
+++ b/arch/arm/imx-common/spl.c
@@ -58,6 +58,7 @@  u32 spl_boot_device(void)
 	/* SD/eSD: 8.5.3, Table 8-15  */
 	case 0x4:
 	case 0x5:
+		 return BOOT_DEVICE_MMC1;
 	/* MMC/eMMC: 8.5.3 */
 	case 0x6:
 	case 0x7: