Skip to contentSkip to conversation

Beaglebone Black broken since commit fd61d39970b9901217efc7536d9f3a61b4e1752a

U-Boot New Guillaume Gardet 3 participants 17 messages Feb 16, 2016 – Feb 20, 2016

lore
Share & tools
lore

Guillaume Gardet

author
#0 lore raw

Hi,

Beaglebone black (am335x_evm_defconfig) is broken (with MMC boot and u-boot.img on ext4 partition). I bisected it to commit fd61d39970b9901217efc7536d9f3a61b4e1752a
spl: mmc: add break statements in spl_mmc_load_image()
from Nikita Kiryanov (in Cc).

Working image gives:
********************************************************************************
U-Boot SPL 2016.01-rc1-00036-g483ab3d (Feb 16 2016 - 19:04:10) bad magic
bad magic
spl_register_fat_device: fat register err - -1
spl_register_fat_device: fat register err - -1
spl_load_image_fat: error reading image u-boot.img, err - -1
spl: ext4fs_open failed
spl: ext4fs_open failed
spl_load_image_ext: error reading image uImage, err - -1

U-Boot 2016.01-rc1-00033-g9884f44 (Feb 16 2016 - 18:55:52 +0100)
********************************************************************************

Since this commit, we get:
********************************************************************************
U-Boot SPL 2016.01-rc1-00037-gfd61d39 (Feb 16 2016 - 19:01:54) bad magic
bad magic
### ERROR ### Please RESET the board ###
********************************************************************************

Guillaume

author
#1 lore raw

PATCH 1/1 spl: if MMCSD_MODE_RAW fails, try MMCSD_MODE_FS, if available

Since commit fd61d39970b9901217efc7536d9f3a61b4e1752a:
spl: mmc: add break statements in spl_mmc_load_image()
RAW and FS boot modes are now exclusive again. So, if MMCSD_MODE_RAW fails, the board hangs. This patch allows to try MMCSD_MODE_FS then, if available.

It has been tested on a beaglebone black to boot on an EXT partition.

Signed-off-by Guillaume GARDET <[email protected]>
Cc Tom Rini <[email protected]>
Cc Nikita Kiryanov <[email protected]>
Cc Igor Grinberg <[email protected]>
Cc Paul Kocialkowski <[email protected]>
Cc Pantelis Antoniou <[email protected]>
Cc Simon Glass <[email protected]>
Cc Matwey V. Kornilov <[email protected]>
---
 common/spl/spl_mmc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
common/spl/spl_mmc.c +1 -1
index c3931c6..2eef0f2 100644
@@ -284,7 +284,7 @@ int spl_mmc_load_image(u32 boot_device)
 		if (!err)
 			return err;
 #endif
-		break;
+		/* Fall through */
 	case MMCSD_MODE_FS:
 		debug("spl: mmc boot mode: fs\n");
Signature · 1 line
1.8.4.5
#2 lore raw

PATCH 1/1 spl: if MMCSD_MODE_RAW fails, try MMCSD_MODE_FS, if available

Quoted 25 lines · Guillaume GARDET

Since commit fd61d39970b9901217efc7536d9f3a61b4e1752a:
spl: mmc: add break statements in spl_mmc_load_image()
RAW and FS boot modes are now exclusive again. So, if MMCSD_MODE_RAW fails, the board hangs. This patch allows to try MMCSD_MODE_FS then, if available.

It has been tested on a beaglebone black to boot on an EXT partition.

Signed-off-by Guillaume GARDET <[email protected]>
Cc Tom Rini <[email protected]>
Cc Nikita Kiryanov <[email protected]>
Cc Igor Grinberg <[email protected]>
Cc Paul Kocialkowski <[email protected]>
Cc Pantelis Antoniou <[email protected]>
Cc Simon Glass <[email protected]>
Cc Matwey V. Kornilov <[email protected]>
---
 common/spl/spl_mmc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
common/spl/spl_mmc.c +0 -0
index c3931c6..2eef0f2 100644

common/spl/spl_mmc.c int spl_mmc_load_image(u32 boot_device) · line 289

Quoted 7 lines
common/spl/spl_mmc.c +1 -1
@@ -284,7 +284,7 @@ int spl_mmc_load_image(u32 boot_device)
 		if (!err)
 			return err;
 #endif
-		break;
+		/* Fall through */
 	case MMCSD_MODE_FS:
 		debug("spl: mmc boot mode: fs\n");

This also essentially reverts fd61d399. So Nikita, was there a specific use case that was broken before, or was the code just unclear in intentions here? Thanks!

Signature · 8 lines
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160217/3438b967/attachment.sig>
#3 lore raw

PATCH 1/1 spl: if MMCSD_MODE_RAW fails, try MMCSD_MODE_FS, if available

Hi Tom, Guillaume,

Quoted 22 lines · Tom Rini
Quoted 20 lines · Guillaume GARDET

Since commit fd61d39970b9901217efc7536d9f3a61b4e1752a:
spl: mmc: add break statements in spl_mmc_load_image()
RAW and FS boot modes are now exclusive again. So, if MMCSD_MODE_RAW fails, the board hangs. This patch allows to try MMCSD_MODE_FS then, if available.

It has been tested on a beaglebone black to boot on an EXT partition.

Signed-off-by Guillaume GARDET <[email protected]>
Cc Tom Rini <[email protected]>
Cc Nikita Kiryanov <[email protected]>
Cc Igor Grinberg <[email protected]>
Cc Paul Kocialkowski <[email protected]>
Cc Pantelis Antoniou <[email protected]>
Cc Simon Glass <[email protected]>
Cc Matwey V. Kornilov <[email protected]>
---
 common/spl/spl_mmc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Quoted 16 lines

common/spl/spl_mmc.c int spl_mmc_load_image(u32 boot_device) · line 289

Quoted 12 lines
common/spl/spl_mmc.c +1 -1
index c3931c6..2eef0f2 100644
@@ -284,7 +284,7 @@ int spl_mmc_load_image(u32 boot_device)
 		if (!err)
 			return err;
 #endif
-		break;
+		/* Fall through */
 	case MMCSD_MODE_FS:
 		debug("spl: mmc boot mode: fs\n");

This also essentially reverts fd61d399. So Nikita, was there a specific use case that was broken before, or was the code just unclear in intentions here? Thanks!

There was no broken use case that I'm aware of. The change was made as part of a code improvement series and was meant to address what I consider to be bad and problematic design. Instead of reverting it though, how about implementing something similar to what I did in the main common/spl/spl.c:board_init_r()? You would have a weak function that will default to the original spl_boot_mode() if not overridden, and allow the user to define a sequence of boot modes otherwise.

Quoted 3 lines
Signature · 2 lines
-- 
Tom
author
#4 lore raw

PATCH 1/1 spl: if MMCSD_MODE_RAW fails, try MMCSD_MODE_FS, if available

Hi Tom, Nikita ,

Le 18/02/2016 10:19, Nikita Kiryanov a ?crit :

Quoted 40 lines

Hi Tom, Guillaume,

Quoted 22 lines · Tom Rini
Quoted 20 lines · Guillaume GARDET

Since commit fd61d39970b9901217efc7536d9f3a61b4e1752a:
spl: mmc: add break statements in spl_mmc_load_image()
RAW and FS boot modes are now exclusive again. So, if MMCSD_MODE_RAW fails, the board hangs. This patch allows to try MMCSD_MODE_FS then, if available.

It has been tested on a beaglebone black to boot on an EXT partition.

Signed-off-by Guillaume GARDET <[email protected]>
Cc Tom Rini <[email protected]>
Cc Nikita Kiryanov <[email protected]>
Cc Igor Grinberg <[email protected]>
Cc Paul Kocialkowski <[email protected]>
Cc Pantelis Antoniou <[email protected]>
Cc Simon Glass <[email protected]>
Cc Matwey V. Kornilov <[email protected]>
---
  common/spl/spl_mmc.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
Quoted 15 lines

common/spl/spl_mmc.c int spl_mmc_load_image(u32 boot_device) · line 289

Quoted 12 lines
common/spl/spl_mmc.c +1 -1
index c3931c6..2eef0f2 100644
@@ -284,7 +284,7 @@ int spl_mmc_load_image(u32 boot_device)
  		if (!err)
  			return err;
  #endif
-		break;
+		/* Fall through */
  	case MMCSD_MODE_FS:
  		debug("spl: mmc boot mode: fs\n");

This also essentially reverts fd61d399. So Nikita, was there a specific use case that was broken before, or was the code just unclear in intentions here? Thanks!

Quoted 7 lines

There was no broken use case that I'm aware of. The change was made as part of a code improvement series and was meant to address what I consider to be bad and problematic design. Instead of reverting it though, how about implementing something similar to what I did in the main common/spl/spl.c:board_init_r()? You would have a weak function that will default to the original spl_boot_mode() if not overridden, and allow the user to define a sequence of boot modes otherwise.

The thing is you broke a wanted behavior currently in use. So, the priority is to come back to the previous behavior. Then, if someone (you, me or someone else) wants to improve this code, the way you suggested, it would be very nice. But it will need a lot more work, tests and reviews.

Guillaume

Quoted 5 lines
Quoted 3 lines
Signature · 2 lines
-- 
Tom
#5 lore raw

PATCH 1/1 spl: if MMCSD_MODE_RAW fails, try MMCSD_MODE_FS, if available

Quoted 51 lines · Guillaume Gardet

Hi Tom, Nikita ,

Le 18/02/2016 10:19, Nikita Kiryanov a ?crit :

Quoted 40 lines

Hi Tom, Guillaume,

Quoted 22 lines · Tom Rini
Quoted 20 lines · Guillaume GARDET

Since commit fd61d39970b9901217efc7536d9f3a61b4e1752a:
spl: mmc: add break statements in spl_mmc_load_image()
RAW and FS boot modes are now exclusive again. So, if MMCSD_MODE_RAW fails, the board hangs. This patch allows to try MMCSD_MODE_FS then, if available.

It has been tested on a beaglebone black to boot on an EXT partition.

Signed-off-by Guillaume GARDET <[email protected]>
Cc Tom Rini <[email protected]>
Cc Nikita Kiryanov <[email protected]>
Cc Igor Grinberg <[email protected]>
Cc Paul Kocialkowski <[email protected]>
Cc Pantelis Antoniou <[email protected]>
Cc Simon Glass <[email protected]>
Cc Matwey V. Kornilov <[email protected]>
---
 common/spl/spl_mmc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Quoted 15 lines

common/spl/spl_mmc.c int spl_mmc_load_image(u32 boot_device) · line 289

Quoted 12 lines
common/spl/spl_mmc.c +1 -1
index c3931c6..2eef0f2 100644
@@ -284,7 +284,7 @@ int spl_mmc_load_image(u32 boot_device)
 		if (!err)
 			return err;
 #endif
-		break;
+		/* Fall through */
 	case MMCSD_MODE_FS:
 		debug("spl: mmc boot mode: fs\n");

This also essentially reverts fd61d399. So Nikita, was there a specific use case that was broken before, or was the code just unclear in intentions here? Thanks!

Quoted 7 lines

There was no broken use case that I'm aware of. The change was made as part of a code improvement series and was meant to address what I consider to be bad and problematic design. Instead of reverting it though, how about implementing something similar to what I did in the main common/spl/spl.c:board_init_r()? You would have a weak function that will default to the original spl_boot_mode() if not overridden, and allow the user to define a sequence of boot modes otherwise.

Quoted 1 line

The thing is you broke a wanted behavior currently in use. So, the priority is to come back to the previous behavior.

Could you add a comment indicating that this is wanted behavior that has a user, and who the user is?

Quoted 12 lines

Then, if someone (you, me or someone else) wants to improve this code, the way you suggested, it would be very nice. But it will need a lot more work, tests and reviews.

Guillaume

Quoted 5 lines
Quoted 3 lines
Signature · 2 lines
-- 
Tom
author
#6 lore raw

PATCH 1/1 spl: if MMCSD_MODE_RAW fails, try MMCSD_MODE_FS, if available

Le 18/02/2016 14:07, Nikita Kiryanov a ?crit :

Quoted 51 lines · Guillaume Gardet
Quoted 50 lines

Hi Tom, Nikita ,

Le 18/02/2016 10:19, Nikita Kiryanov a ?crit :

Quoted 47 lines

Hi Tom, Guillaume,

Quoted 22 lines · Tom Rini
Quoted 20 lines · Guillaume GARDET

Since commit fd61d39970b9901217efc7536d9f3a61b4e1752a:
spl: mmc: add break statements in spl_mmc_load_image()
RAW and FS boot modes are now exclusive again. So, if MMCSD_MODE_RAW fails, the board hangs. This patch allows to try MMCSD_MODE_FS then, if available.

It has been tested on a beaglebone black to boot on an EXT partition.

Signed-off-by Guillaume GARDET <[email protected]>
Cc Tom Rini <[email protected]>
Cc Nikita Kiryanov <[email protected]>
Cc Igor Grinberg <[email protected]>
Cc Paul Kocialkowski <[email protected]>
Cc Pantelis Antoniou <[email protected]>
Cc Simon Glass <[email protected]>
Cc Matwey V. Kornilov <[email protected]>
---
  common/spl/spl_mmc.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
Quoted 15 lines

common/spl/spl_mmc.c int spl_mmc_load_image(u32 boot_device) · line 289

Quoted 12 lines
common/spl/spl_mmc.c +1 -1
index c3931c6..2eef0f2 100644
@@ -284,7 +284,7 @@ int spl_mmc_load_image(u32 boot_device)
  		if (!err)
  			return err;
  #endif
-		break;
+		/* Fall through */
  	case MMCSD_MODE_FS:
  		debug("spl: mmc boot mode: fs\n");

This also essentially reverts fd61d399. So Nikita, was there a specific use case that was broken before, or was the code just unclear in intentions here? Thanks!

There was no broken use case that I'm aware of. The change was made as part of a code improvement series and was meant to address what I consider to be bad and problematic design. Instead of reverting it though, how about implementing something similar to what I did in the main common/spl/spl.c:board_init_r()? You would have a weak function that will default to the original spl_boot_mode() if not overridden, and allow the user to define a sequence of boot modes otherwise.

Quoted 3 lines
Quoted 1 line

The thing is you broke a wanted behavior currently in use. So, the priority is to come back to the previous behavior.

Could you add a comment indicating that this is wanted behavior that has a user, and who the user is?

Not sure what you mean.
I think, "Fall through" code comment explains it (as you done with MMCSD_MODE_EMMCBOOT). I (and openSUSE ARM) do not pretend to be the only user(s) of this feature, so I won't add my name there. If you mean commit message, I think the current one is enough.

Guillaume

Quoted 10 lines
Quoted 10 lines

Then, if someone (you, me or someone else) wants to improve this code, the way you suggested, it would be very nice. But it will need a lot more work, tests and reviews.

Guillaume

Quoted 3 lines
Quoted 2 lines
Signature · 1 line
Tom
#7 lore raw

PATCH 1/1 spl: if MMCSD_MODE_RAW fails, try MMCSD_MODE_FS, if available

Quoted 54 lines · Guillaume Gardet

Le 18/02/2016 14:07, Nikita Kiryanov a ?crit :

Quoted 51 lines · Guillaume Gardet
Quoted 50 lines

Hi Tom, Nikita ,

Le 18/02/2016 10:19, Nikita Kiryanov a ?crit :

Quoted 47 lines

Hi Tom, Guillaume,

Quoted 22 lines · Tom Rini
Quoted 20 lines · Guillaume GARDET

Since commit fd61d39970b9901217efc7536d9f3a61b4e1752a:
spl: mmc: add break statements in spl_mmc_load_image()
RAW and FS boot modes are now exclusive again. So, if MMCSD_MODE_RAW fails, the board hangs. This patch allows to try MMCSD_MODE_FS then, if available.

It has been tested on a beaglebone black to boot on an EXT partition.

Signed-off-by Guillaume GARDET <[email protected]>
Cc Tom Rini <[email protected]>
Cc Nikita Kiryanov <[email protected]>
Cc Igor Grinberg <[email protected]>
Cc Paul Kocialkowski <[email protected]>
Cc Pantelis Antoniou <[email protected]>
Cc Simon Glass <[email protected]>
Cc Matwey V. Kornilov <[email protected]>
---
 common/spl/spl_mmc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Quoted 15 lines

common/spl/spl_mmc.c int spl_mmc_load_image(u32 boot_device) · line 289

Quoted 12 lines
common/spl/spl_mmc.c +1 -1
index c3931c6..2eef0f2 100644
@@ -284,7 +284,7 @@ int spl_mmc_load_image(u32 boot_device)
 		if (!err)
 			return err;
 #endif
-		break;
+		/* Fall through */
 	case MMCSD_MODE_FS:
 		debug("spl: mmc boot mode: fs\n");

This also essentially reverts fd61d399. So Nikita, was there a specific use case that was broken before, or was the code just unclear in intentions here? Thanks!

There was no broken use case that I'm aware of. The change was made as part of a code improvement series and was meant to address what I consider to be bad and problematic design. Instead of reverting it though, how about implementing something similar to what I did in the main common/spl/spl.c:board_init_r()? You would have a weak function that will default to the original spl_boot_mode() if not overridden, and allow the user to define a sequence of boot modes otherwise.

Quoted 5 lines
Quoted 3 lines
Quoted 1 line

The thing is you broke a wanted behavior currently in use. So, the priority is to come back to the previous behavior.

Could you add a comment indicating that this is wanted behavior that has a user, and who the user is?

Not sure what you mean.

I mean something like:
/* If raw mode fails, try fs mode. Some boards, such as beaglebone black,
* depend on this funcitonality.
*/

Quoted 1 line

I think, "Fall through" code comment explains it (as you done with MMCSD_MODE_EMMCBOOT).

That was meant to communicate a very different type of coupling between the two cases though..

#8 lore raw

PATCH 1/1 spl: if MMCSD_MODE_RAW fails, try MMCSD_MODE_FS, if available

Quoted 59 lines · Nikita Kiryanov
Quoted 58 lines · Guillaume Gardet

Le 18/02/2016 14:07, Nikita Kiryanov a ?crit :

Quoted 54 lines · Guillaume Gardet
Quoted 43 lines

Hi Tom, Nikita ,

Le 18/02/2016 10:19, Nikita Kiryanov a ?crit :

Quoted 40 lines

Hi Tom, Guillaume,

Quoted 37 lines · Tom Rini
Quoted 25 lines · Guillaume GARDET

Since commit fd61d39970b9901217efc7536d9f3a61b4e1752a:
spl: mmc: add break statements in spl_mmc_load_image()
RAW and FS boot modes are now exclusive again. So, if MMCSD_MODE_RAW fails, the board hangs. This patch allows to try MMCSD_MODE_FS then, if available.

It has been tested on a beaglebone black to boot on an EXT partition.

Signed-off-by Guillaume GARDET <[email protected]>
Cc Tom Rini <[email protected]>
Cc Nikita Kiryanov <[email protected]>
Cc Igor Grinberg <[email protected]>
Cc Paul Kocialkowski <[email protected]>
Cc Pantelis Antoniou <[email protected]>
Cc Simon Glass <[email protected]>
Cc Matwey V. Kornilov <[email protected]>
---
 common/spl/spl_mmc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
common/spl/spl_mmc.c +0 -0
index c3931c6..2eef0f2 100644

common/spl/spl_mmc.c int spl_mmc_load_image(u32 boot_device) · line 289

Quoted 7 lines
common/spl/spl_mmc.c +1 -1
@@ -284,7 +284,7 @@ int spl_mmc_load_image(u32 boot_device)
 		if (!err)
 			return err;
 #endif
-		break;
+		/* Fall through */
 	case MMCSD_MODE_FS:
 		debug("spl: mmc boot mode: fs\n");

This also essentially reverts fd61d399. So Nikita, was there a specific use case that was broken before, or was the code just unclear in intentions here? Thanks!

Quoted 8 lines
Quoted 7 lines

There was no broken use case that I'm aware of. The change was made as part of a code improvement series and was meant to address what I consider to be bad and problematic design. Instead of reverting it though, how about implementing something similar to what I did in the main common/spl/spl.c:board_init_r()? You would have a weak function that will default to the original spl_boot_mode() if not overridden, and allow the user to define a sequence of boot modes otherwise.

The thing is you broke a wanted behavior currently in use. So, the priority is to come back to the previous behavior.

Could you add a comment indicating that this is wanted behavior that has a user, and who the user is?

Quoted 6 lines
Quoted 1 line

Not sure what you mean.

I mean something like:
/* If raw mode fails, try fs mode. Some boards, such as beaglebone black,
* depend on this funcitonality.
*/

But it's not board specific, it's use-case specific. It's saying that instead of shoving both SPL and U-Boot into the correct magic raw location, just shove SPL there and let U-Boot itself be in the /boot partition. This is just as applicable on say imx6 as it is on TI parts.

Signature · 8 lines
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160218/29db3564/attachment.sig>
#9 lore raw

PATCH 1/1 spl: if MMCSD_MODE_RAW fails, try MMCSD_MODE_FS, if available

Quoted 60 lines · Tom Rini
Quoted 59 lines · Nikita Kiryanov
Quoted 58 lines · Guillaume Gardet

Le 18/02/2016 14:07, Nikita Kiryanov a ?crit :

Quoted 54 lines · Guillaume Gardet
Quoted 43 lines

Hi Tom, Nikita ,

Le 18/02/2016 10:19, Nikita Kiryanov a ?crit :

Quoted 40 lines

Hi Tom, Guillaume,

Quoted 37 lines · Tom Rini
Quoted 25 lines · Guillaume GARDET

Since commit fd61d39970b9901217efc7536d9f3a61b4e1752a:
spl: mmc: add break statements in spl_mmc_load_image()
RAW and FS boot modes are now exclusive again. So, if MMCSD_MODE_RAW fails, the board hangs. This patch allows to try MMCSD_MODE_FS then, if available.

It has been tested on a beaglebone black to boot on an EXT partition.

Signed-off-by Guillaume GARDET <[email protected]>
Cc Tom Rini <[email protected]>
Cc Nikita Kiryanov <[email protected]>
Cc Igor Grinberg <[email protected]>
Cc Paul Kocialkowski <[email protected]>
Cc Pantelis Antoniou <[email protected]>
Cc Simon Glass <[email protected]>
Cc Matwey V. Kornilov <[email protected]>
---
 common/spl/spl_mmc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
common/spl/spl_mmc.c +0 -0
index c3931c6..2eef0f2 100644

common/spl/spl_mmc.c int spl_mmc_load_image(u32 boot_device) · line 289

Quoted 7 lines
common/spl/spl_mmc.c +1 -1
@@ -284,7 +284,7 @@ int spl_mmc_load_image(u32 boot_device)
 		if (!err)
 			return err;
 #endif
-		break;
+		/* Fall through */
 	case MMCSD_MODE_FS:
 		debug("spl: mmc boot mode: fs\n");

This also essentially reverts fd61d399. So Nikita, was there a specific use case that was broken before, or was the code just unclear in intentions here? Thanks!

Quoted 8 lines
Quoted 7 lines

There was no broken use case that I'm aware of. The change was made as part of a code improvement series and was meant to address what I consider to be bad and problematic design. Instead of reverting it though, how about implementing something similar to what I did in the main common/spl/spl.c:board_init_r()? You would have a weak function that will default to the original spl_boot_mode() if not overridden, and allow the user to define a sequence of boot modes otherwise.

The thing is you broke a wanted behavior currently in use. So, the priority is to come back to the previous behavior.

Could you add a comment indicating that this is wanted behavior that has a user, and who the user is?

Quoted 8 lines
Quoted 6 lines
Quoted 1 line

Not sure what you mean.

I mean something like:
/* If raw mode fails, try fs mode. Some boards, such as beaglebone black,
* depend on this funcitonality.
*/

But it's not board specific, it's use-case specific.

The comment I proposed does not suggest it's board specific, just that this specific use case is used by someone.

Quoted 3 lines

instead of shoving both SPL and U-Boot into the correct magic raw location, just shove SPL there and let U-Boot itself be in the /boot partition. This is just as applicable on say imx6 as it is on TI parts.

I don't think that's clear enough that this is the purpose of the missing break statement. It's a little too implicit. What I'm suggesting is that we make it a bit more explicit, barring a rewrite.

Quoted 3 lines
Signature · 2 lines
-- 
Tom
author
#10 lore raw

PATCH 1/1 spl: if MMCSD_MODE_RAW fails, try MMCSD_MODE_FS, if available

Le 18/02/2016 17:07, Nikita Kiryanov a ?crit :

Quoted 68 lines · Tom Rini
Quoted 56 lines · Nikita Kiryanov
Quoted 55 lines · Guillaume Gardet
Quoted 54 lines

Le 18/02/2016 14:07, Nikita Kiryanov a ?crit :

Quoted 52 lines · Guillaume Gardet
Quoted 51 lines

Hi Tom, Nikita ,

Le 18/02/2016 10:19, Nikita Kiryanov a ?crit :

Quoted 40 lines

Hi Tom, Guillaume,

Quoted 22 lines · Tom Rini
Quoted 20 lines · Guillaume GARDET

Since commit fd61d39970b9901217efc7536d9f3a61b4e1752a:
spl: mmc: add break statements in spl_mmc_load_image()
RAW and FS boot modes are now exclusive again. So, if MMCSD_MODE_RAW fails, the board hangs. This patch allows to try MMCSD_MODE_FS then, if available.

It has been tested on a beaglebone black to boot on an EXT partition.

Signed-off-by Guillaume GARDET <[email protected]>
Cc Tom Rini <[email protected]>
Cc Nikita Kiryanov <[email protected]>
Cc Igor Grinberg <[email protected]>
Cc Paul Kocialkowski <[email protected]>
Cc Pantelis Antoniou <[email protected]>
Cc Simon Glass <[email protected]>
Cc Matwey V. Kornilov <[email protected]>
---
  common/spl/spl_mmc.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
Quoted 15 lines

common/spl/spl_mmc.c int spl_mmc_load_image(u32 boot_device) · line 289

Quoted 12 lines
common/spl/spl_mmc.c +1 -1
index c3931c6..2eef0f2 100644
@@ -284,7 +284,7 @@ int spl_mmc_load_image(u32 boot_device)
  		if (!err)
  			return err;
  #endif
-		break;
+		/* Fall through */
  	case MMCSD_MODE_FS:
  		debug("spl: mmc boot mode: fs\n");

This also essentially reverts fd61d399. So Nikita, was there a specific use case that was broken before, or was the code just unclear in intentions here? Thanks!

Quoted 7 lines

There was no broken use case that I'm aware of. The change was made as part of a code improvement series and was meant to address what I consider to be bad and problematic design. Instead of reverting it though, how about implementing something similar to what I did in the main common/spl/spl.c:board_init_r()? You would have a weak function that will default to the original spl_boot_mode() if not overridden, and allow the user to define a sequence of boot modes otherwise.

The thing is you broke a wanted behavior currently in use. So, the priority is to come back to the previous behavior.

Quoted 8 lines
Quoted 7 lines
Quoted 3 lines
Quoted 2 lines

Could you add a comment indicating that this is wanted behavior that has a user, and who the user is?

Not sure what you mean.

I mean something like:
/* If raw mode fails, try fs mode. Some boards, such as beaglebone black,
* depend on this funcitonality.
*/

But it's not board specific, it's use-case specific.

The comment I proposed does not suggest it's board specific, just that this specific use case is used by someone.

Quoted 6 lines
Quoted 3 lines

instead of shoving both SPL and U-Boot into the correct magic raw location, just shove SPL there and let U-Boot itself be in the /boot partition. This is just as applicable on say imx6 as it is on TI parts.

I don't think that's clear enough that this is the purpose of the missing break statement. It's a little too implicit. What I'm suggesting is that we make it a bit more explicit, barring a rewrite.

So, maybe just:

     /* If raw mode fails, try fs mode. */

?

Guillaume

Quoted 5 lines
Quoted 3 lines
Signature · 2 lines
-- 
Tom
#11 lore raw

PATCH 1/1 spl: if MMCSD_MODE_RAW fails, try MMCSD_MODE_FS, if available

Quoted 74 lines · Guillaume Gardet

Le 18/02/2016 17:07, Nikita Kiryanov a ?crit :

Quoted 71 lines · Tom Rini
Quoted 56 lines · Nikita Kiryanov
Quoted 55 lines · Guillaume Gardet
Quoted 54 lines

Le 18/02/2016 14:07, Nikita Kiryanov a ?crit :

Quoted 52 lines · Guillaume Gardet
Quoted 51 lines

Hi Tom, Nikita ,

Le 18/02/2016 10:19, Nikita Kiryanov a ?crit :

Quoted 40 lines

Hi Tom, Guillaume,

Quoted 22 lines · Tom Rini
Quoted 20 lines · Guillaume GARDET

Since commit fd61d39970b9901217efc7536d9f3a61b4e1752a:
spl: mmc: add break statements in spl_mmc_load_image()
RAW and FS boot modes are now exclusive again. So, if MMCSD_MODE_RAW fails, the board hangs. This patch allows to try MMCSD_MODE_FS then, if available.

It has been tested on a beaglebone black to boot on an EXT partition.

Signed-off-by Guillaume GARDET <[email protected]>
Cc Tom Rini <[email protected]>
Cc Nikita Kiryanov <[email protected]>
Cc Igor Grinberg <[email protected]>
Cc Paul Kocialkowski <[email protected]>
Cc Pantelis Antoniou <[email protected]>
Cc Simon Glass <[email protected]>
Cc Matwey V. Kornilov <[email protected]>
---
 common/spl/spl_mmc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Quoted 15 lines

common/spl/spl_mmc.c int spl_mmc_load_image(u32 boot_device) · line 289

Quoted 12 lines
common/spl/spl_mmc.c +1 -1
index c3931c6..2eef0f2 100644
@@ -284,7 +284,7 @@ int spl_mmc_load_image(u32 boot_device)
 		if (!err)
 			return err;
 #endif
-		break;
+		/* Fall through */
 	case MMCSD_MODE_FS:
 		debug("spl: mmc boot mode: fs\n");

This also essentially reverts fd61d399. So Nikita, was there a specific use case that was broken before, or was the code just unclear in intentions here? Thanks!

Quoted 7 lines

There was no broken use case that I'm aware of. The change was made as part of a code improvement series and was meant to address what I consider to be bad and problematic design. Instead of reverting it though, how about implementing something similar to what I did in the main common/spl/spl.c:board_init_r()? You would have a weak function that will default to the original spl_boot_mode() if not overridden, and allow the user to define a sequence of boot modes otherwise.

The thing is you broke a wanted behavior currently in use. So, the priority is to come back to the previous behavior.

Quoted 8 lines
Quoted 7 lines
Quoted 3 lines
Quoted 2 lines

Could you add a comment indicating that this is wanted behavior that has a user, and who the user is?

Not sure what you mean.

I mean something like:
/* If raw mode fails, try fs mode. Some boards, such as beaglebone black,
* depend on this funcitonality.
*/

But it's not board specific, it's use-case specific.

The comment I proposed does not suggest it's board specific, just that this specific use case is used by someone.

Quoted 3 lines

instead of shoving both SPL and U-Boot into the correct magic raw location, just shove SPL there and let U-Boot itself be in the /boot partition. This is just as applicable on say imx6 as it is on TI parts.

Quoted 7 lines
Quoted 3 lines

I don't think that's clear enough that this is the purpose of the missing break statement. It's a little too implicit. What I'm suggesting is that we make it a bit more explicit, barring a rewrite.

So, maybe just:

    /* If raw mode fails, try fs mode. */

?

That'll work too.

Quoted 10 lines

Guillaume

Quoted 5 lines
Quoted 3 lines
Signature · 2 lines
-- 
Tom
author
#12 lore raw

PATCH 1/1 spl: if MMCSD_MODE_RAW fails, try MMCSD_MODE_FS, if available

Le 18/02/2016 17:38, Nikita Kiryanov a ?crit :

Quoted 73 lines · Guillaume Gardet
Quoted 72 lines

Le 18/02/2016 17:07, Nikita Kiryanov a ?crit :

Quoted 70 lines · Tom Rini
Quoted 55 lines · Nikita Kiryanov
Quoted 54 lines · Guillaume Gardet
Quoted 53 lines

Le 18/02/2016 14:07, Nikita Kiryanov a ?crit :

Quoted 52 lines · Guillaume Gardet
Quoted 51 lines

Hi Tom, Nikita ,

Le 18/02/2016 10:19, Nikita Kiryanov a ?crit :

Quoted 40 lines

Hi Tom, Guillaume,

Quoted 22 lines · Tom Rini
Quoted 20 lines · Guillaume GARDET

Since commit fd61d39970b9901217efc7536d9f3a61b4e1752a:
spl: mmc: add break statements in spl_mmc_load_image()
RAW and FS boot modes are now exclusive again. So, if MMCSD_MODE_RAW fails, the board hangs. This patch allows to try MMCSD_MODE_FS then, if available.

It has been tested on a beaglebone black to boot on an EXT partition.

Signed-off-by Guillaume GARDET <[email protected]>
Cc Tom Rini <[email protected]>
Cc Nikita Kiryanov <[email protected]>
Cc Igor Grinberg <[email protected]>
Cc Paul Kocialkowski <[email protected]>
Cc Pantelis Antoniou <[email protected]>
Cc Simon Glass <[email protected]>
Cc Matwey V. Kornilov <[email protected]>
---
  common/spl/spl_mmc.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
Quoted 15 lines

common/spl/spl_mmc.c int spl_mmc_load_image(u32 boot_device) · line 289

Quoted 12 lines
common/spl/spl_mmc.c +1 -1
index c3931c6..2eef0f2 100644
@@ -284,7 +284,7 @@ int spl_mmc_load_image(u32 boot_device)
  		if (!err)
  			return err;
  #endif
-		break;
+		/* Fall through */
  	case MMCSD_MODE_FS:
  		debug("spl: mmc boot mode: fs\n");

This also essentially reverts fd61d399. So Nikita, was there a specific use case that was broken before, or was the code just unclear in intentions here? Thanks!

Quoted 7 lines

There was no broken use case that I'm aware of. The change was made as part of a code improvement series and was meant to address what I consider to be bad and problematic design. Instead of reverting it though, how about implementing something similar to what I did in the main common/spl/spl.c:board_init_r()? You would have a weak function that will default to the original spl_boot_mode() if not overridden, and allow the user to define a sequence of boot modes otherwise.

The thing is you broke a wanted behavior currently in use. So, the priority is to come back to the previous behavior.

Quoted 8 lines
Quoted 7 lines
Quoted 3 lines
Quoted 2 lines

Could you add a comment indicating that this is wanted behavior that has a user, and who the user is?

Not sure what you mean.

I mean something like:
/* If raw mode fails, try fs mode. Some boards, such as beaglebone black,
* depend on this funcitonality.
*/

But it's not board specific, it's use-case specific.

The comment I proposed does not suggest it's board specific, just that this specific use case is used by someone.

Quoted 3 lines

instead of shoving both SPL and U-Boot into the correct magic raw location, just shove SPL there and let U-Boot itself be in the /boot partition. This is just as applicable on say imx6 as it is on TI parts.

Quoted 7 lines
Quoted 6 lines
Quoted 3 lines

I don't think that's clear enough that this is the purpose of the missing break statement. It's a little too implicit. What I'm suggesting is that we make it a bit more explicit, barring a rewrite.

So, maybe just:

     /* If raw mode fails, try fs mode. */

?

That'll work too.

If Tom is ok, I will send a V2.

Guillaume

#13 lore raw

PATCH 1/1 spl: if MMCSD_MODE_RAW fails, try MMCSD_MODE_FS, if available

Quoted 79 lines · Guillaume Gardet

Le 18/02/2016 17:38, Nikita Kiryanov a ?crit :

Quoted 76 lines · Guillaume Gardet
Quoted 75 lines

Le 18/02/2016 17:07, Nikita Kiryanov a ?crit :

Quoted 73 lines · Tom Rini
Quoted 55 lines · Nikita Kiryanov
Quoted 54 lines · Guillaume Gardet
Quoted 53 lines

Le 18/02/2016 14:07, Nikita Kiryanov a ?crit :

Quoted 52 lines · Guillaume Gardet
Quoted 51 lines

Hi Tom, Nikita ,

Le 18/02/2016 10:19, Nikita Kiryanov a ?crit :

Quoted 40 lines

Hi Tom, Guillaume,

Quoted 22 lines · Tom Rini
Quoted 20 lines · Guillaume GARDET

Since commit fd61d39970b9901217efc7536d9f3a61b4e1752a:
spl: mmc: add break statements in spl_mmc_load_image()
RAW and FS boot modes are now exclusive again. So, if MMCSD_MODE_RAW fails, the board hangs. This patch allows to try MMCSD_MODE_FS then, if available.

It has been tested on a beaglebone black to boot on an EXT partition.

Signed-off-by Guillaume GARDET <[email protected]>
Cc Tom Rini <[email protected]>
Cc Nikita Kiryanov <[email protected]>
Cc Igor Grinberg <[email protected]>
Cc Paul Kocialkowski <[email protected]>
Cc Pantelis Antoniou <[email protected]>
Cc Simon Glass <[email protected]>
Cc Matwey V. Kornilov <[email protected]>
---
 common/spl/spl_mmc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Quoted 15 lines

common/spl/spl_mmc.c int spl_mmc_load_image(u32 boot_device) · line 289

Quoted 12 lines
common/spl/spl_mmc.c +1 -1
index c3931c6..2eef0f2 100644
@@ -284,7 +284,7 @@ int spl_mmc_load_image(u32 boot_device)
 		if (!err)
 			return err;
 #endif
-		break;
+		/* Fall through */
 	case MMCSD_MODE_FS:
 		debug("spl: mmc boot mode: fs\n");

This also essentially reverts fd61d399. So Nikita, was there a specific use case that was broken before, or was the code just unclear in intentions here? Thanks!

Quoted 7 lines

There was no broken use case that I'm aware of. The change was made as part of a code improvement series and was meant to address what I consider to be bad and problematic design. Instead of reverting it though, how about implementing something similar to what I did in the main common/spl/spl.c:board_init_r()? You would have a weak function that will default to the original spl_boot_mode() if not overridden, and allow the user to define a sequence of boot modes otherwise.

The thing is you broke a wanted behavior currently in use. So, the priority is to come back to the previous behavior.

Quoted 8 lines
Quoted 7 lines
Quoted 3 lines
Quoted 2 lines

Could you add a comment indicating that this is wanted behavior that has a user, and who the user is?

Not sure what you mean.

I mean something like:
/* If raw mode fails, try fs mode. Some boards, such as beaglebone black,
* depend on this funcitonality.
*/

But it's not board specific, it's use-case specific.

The comment I proposed does not suggest it's board specific, just that this specific use case is used by someone.

Quoted 3 lines

instead of shoving both SPL and U-Boot into the correct magic raw location, just shove SPL there and let U-Boot itself be in the /boot partition. This is just as applicable on say imx6 as it is on TI parts.

I don't think that's clear enough that this is the purpose of the missing break statement. It's a little too implicit. What I'm suggesting is that we make it a bit more explicit, barring a rewrite.

Quoted 6 lines
Quoted 4 lines
Quoted 3 lines

So, maybe just:

    /* If raw mode fails, try fs mode. */

?

That'll work too.

If Tom is ok, I will send a V2.

OK with me, thanks!

Signature · 8 lines
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160218/3d229563/attachment.sig>
author
#14 lore raw

PATCH v2 1/1 spl: if MMCSD_MODE_RAW fails, try MMCSD_MODE_FS

Since commit fd61d39970b9901217efc7536d9f3a61b4e1752a:
spl: mmc: add break statements in spl_mmc_load_image()
RAW and FS boot modes are now exclusive again. So, if MMCSD_MODE_RAW fails, the board hangs. This patch allows to try MMCSD_MODE_FS then.

It has been tested on a beaglebone black to boot on an EXT partition.

Signed-off-by Guillaume GARDET <[email protected]>
Cc Tom Rini <[email protected]>
Cc Nikita Kiryanov <[email protected]>
Cc Igor Grinberg <[email protected]>
Cc Paul Kocialkowski <[email protected]>
Cc Pantelis Antoniou <[email protected]>
Cc Simon Glass <[email protected]>
Cc Matwey V. Kornilov <[email protected]>

---
Changes in V2:
- Replace "/* Fall through */" comment by:
"/* If RAW mode fails, try FS mode. */"

 common/spl/spl_mmc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
common/spl/spl_mmc.c +1 -1
index c3931c6..c27a250 100644
@@ -284,7 +284,7 @@ int spl_mmc_load_image(u32 boot_device)
 		if (!err)
 			return err;
 #endif
-		break;
+		/* If RAW mode fails, try FS mode. */
 	case MMCSD_MODE_FS:
 		debug("spl: mmc boot mode: fs\n");
Signature · 1 line
1.8.4.5
#15 lore raw

PATCH v2 1/1 spl: if MMCSD_MODE_RAW fails, try MMCSD_MODE_FS

Quoted 6 lines · Guillaume GARDET

Since commit fd61d39970b9901217efc7536d9f3a61b4e1752a:
spl: mmc: add break statements in spl_mmc_load_image()
RAW and FS boot modes are now exclusive again. So, if MMCSD_MODE_RAW fails, the board hangs. This patch allows to try MMCSD_MODE_FS then.

It has been tested on a beaglebone black to boot on an EXT partition.

Acked-by Nikita Kiryanov <[email protected]>
Quoted 34 lines
Signed-off-by Guillaume GARDET <[email protected]>
Cc Tom Rini <[email protected]>
Cc Nikita Kiryanov <[email protected]>
Cc Igor Grinberg <[email protected]>
Cc Paul Kocialkowski <[email protected]>
Cc Pantelis Antoniou <[email protected]>
Cc Simon Glass <[email protected]>
Cc Matwey V. Kornilov <[email protected]>

---
Changes in V2:
- Replace "/* Fall through */" comment by:
"/* If RAW mode fails, try FS mode. */"

 common/spl/spl_mmc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
common/spl/spl_mmc.c +1 -1
index c3931c6..c27a250 100644
@@ -284,7 +284,7 @@ int spl_mmc_load_image(u32 boot_device)
 		if (!err)
 			return err;
 #endif
-		break;
+		/* If RAW mode fails, try FS mode. */
 	case MMCSD_MODE_FS:
 		debug("spl: mmc boot mode: fs\n");
Signature · 1 line
1.8.4.5
#16 lore raw

PATCH v2 1/1 spl: if MMCSD_MODE_RAW fails, try MMCSD_MODE_FS

In reply to Guillaume GARDET
Quoted 7 lines · Guillaume GARDET

Since commit fd61d39970b9901217efc7536d9f3a61b4e1752a:
spl: mmc: add break statements in spl_mmc_load_image()
RAW and FS boot modes are now exclusive again. So, if MMCSD_MODE_RAW fails, the board hangs. This patch allows to try MMCSD_MODE_FS then.

It has been tested on a beaglebone black to boot on an EXT partition.

Quoted 9 lines
Signed-off-by Guillaume GARDET <[email protected]>
Cc Tom Rini <[email protected]>
Cc Nikita Kiryanov <[email protected]>
Cc Igor Grinberg <[email protected]>
Cc Paul Kocialkowski <[email protected]>
Cc Pantelis Antoniou <[email protected]>
Cc Simon Glass <[email protected]>
Cc Matwey V. Kornilov <[email protected]>
Acked-by Nikita Kiryanov <[email protected]>

Applied to u-boot/master, thanks!

Signature · 8 lines
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160219/3db2cb05/attachment.sig>