[U-Boot] [PATCH 0/2] fix issue with mmc partition management
This series aims at addressing an issue discovered with SPL mode when the MMC device being used lacks an environment partition. http://www.mail-archive.com/meta-ti@yoctoproject.org/msg04320.html includes details on the original failure with this diagnosis:
This is a bug in handling mmc_switch_part: what's happening is that the code reconfigures the mmc device to look at the partition on which the environment is to be found, but fails to restore it to reflect the state of the whole device. I.e., the mmc capacity and lba are zero in my case (I have no partition 2 on the uSD card), but mmc_switch_part() returns -ENODEV on the attempt to switch back in fini_mmc_for_env() without also resetting the capacity to what the rest of the system expects.
The first fixes a mistaken assumption about how mmc_switch_part() behaves. (Personally, I think mmc_switch_part() should have recorded the new partition in the device structure, but that's an behavioral change that I'm not going to introduce.)
The second fixes the underlying problem, which was the failure to restore the capacity configuration to the whole device after interacting with the environment.
FWIW: The second patch is relevant to 2014.07; the first is not.
Peter A. Bigot (2): env_mmc: remove condition on call to mmc_switch_part mmc: restore capacity when switching to partition 0
common/env_mmc.c | 11 ++++------- drivers/mmc/mmc.c | 11 ++++++++--- 2 files changed, 12 insertions(+), 10 deletions(-)
Though it might be expected to do so, mmc_switch_part() does not change the part_num field of the device on which the partition has been changed. As such, checking to see whether the partition is already the target partition will fail to correctly restore the original configuration in cases like env_mmc which rely on this behavior to avoid having to preserve the pre-switch partition number outside the device structure.
Signed-off-by: Peter A. Bigot pab@pabigot.com --- common/env_mmc.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/common/env_mmc.c b/common/env_mmc.c index a7621a8..9556296 100644 --- a/common/env_mmc.c +++ b/common/env_mmc.c @@ -78,11 +78,9 @@ static int mmc_set_env_part(struct mmc *mmc) dev = 0; #endif
- if (part != mmc->part_num) { - ret = mmc_switch_part(dev, part); - if (ret) - puts("MMC partition switch failed\n"); - } + ret = mmc_switch_part(dev, part); + if (ret) + puts("MMC partition switch failed\n");
return ret; } @@ -113,8 +111,7 @@ static void fini_mmc_for_env(struct mmc *mmc) #ifdef CONFIG_SPL_BUILD dev = 0; #endif - if (CONFIG_SYS_MMC_ENV_PART != mmc->part_num) - mmc_switch_part(dev, mmc->part_num); + mmc_switch_part(dev, mmc->part_num); #endif }
On 09/02/2014 05:31 PM, Peter A. Bigot wrote:
Though it might be expected to do so, mmc_switch_part() does not change the part_num field of the device on which the partition has been changed. As such, checking to see whether the partition is already the target partition will fail to correctly restore the original configuration in cases like env_mmc which rely on this behavior to avoid having to preserve the pre-switch partition number outside the device structure.
diff --git a/common/env_mmc.c b/common/env_mmc.c
- if (part != mmc->part_num) {
ret = mmc_switch_part(dev, part);if (ret)puts("MMC partition switch failed\n");- }
- ret = mmc_switch_part(dev, part);
- if (ret)
puts("MMC partition switch failed\n");
I'm not sure this is correct, but it might be.
I believe the if() is present to avoid any attempt to call mmc_switch_part() on a device that doesn't have HW partitions (in which case, both part and mmc->part_num should already be 0), since such an attempt might print an error message. If you don't observe any error message printed after this change, then perhaps this patch is fine.
On 09/03/2014 10:46 AM, Stephen Warren wrote:
On 09/02/2014 05:31 PM, Peter A. Bigot wrote:
Though it might be expected to do so, mmc_switch_part() does not change the part_num field of the device on which the partition has been changed. As such, checking to see whether the partition is already the target partition will fail to correctly restore the original configuration in cases like env_mmc which rely on this behavior to avoid having to preserve the pre-switch partition number outside the device structure.
diff --git a/common/env_mmc.c b/common/env_mmc.c
- if (part != mmc->part_num) {
ret = mmc_switch_part(dev, part);if (ret)puts("MMC partition switch failed\n");- }
- ret = mmc_switch_part(dev, part);
- if (ret)
puts("MMC partition switch failed\n");I'm not sure this is correct, but it might be.
I believe the if() is present to avoid any attempt to call mmc_switch_part() on a device that doesn't have HW partitions (in which case, both part and mmc->part_num should already be 0), since such an attempt might print an error message.
That could be true. The patch that added the feature didn't provide that information. In my case, the device does have HW partitions.
If you don't observe any error message printed after this change, then perhaps this patch is fine.
It does work in my environment, but would not retain the behavior you describe. The existing code is still wrong, but the error is elsewhere: I'll provide a new patch to supersede this one.
Peter
The code to set the MMC partition uses an weak function to obtain the correct partition number. Use that instead of the compile-time default when deciding whether it needs to switch back.
Signed-off-by: Peter A. Bigot pab@pabigot.com ---
V2: * Preserve desired behavior of avoiding diagnostic when no HW partition supported * Supersedes https://patchwork.ozlabs.org/patch/385355/
common/env_mmc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/common/env_mmc.c b/common/env_mmc.c index a7621a8..14648e3 100644 --- a/common/env_mmc.c +++ b/common/env_mmc.c @@ -113,7 +113,7 @@ static void fini_mmc_for_env(struct mmc *mmc) #ifdef CONFIG_SPL_BUILD dev = 0; #endif - if (CONFIG_SYS_MMC_ENV_PART != mmc->part_num) + if (mmc_get_env_part(mmc) != mmc->part_num) mmc_switch_part(dev, mmc->part_num); #endif }
On 09/03/2014 10:32 AM, Peter A. Bigot wrote:
The code to set the MMC partition uses an weak function to obtain the correct partition number. Use that instead of the compile-time default when deciding whether it needs to switch back.
Yes, this clearly fixes a bug.
Can you also please add a Fixes: tag that refers to the commit which introduced the problem (i.e. which updated mmc_set_env_part() to call mmc_get_env_part(), but forgot to update fini_mmc_for_env() to match.
On 09/03/2014 11:52 AM, Stephen Warren wrote:
On 09/03/2014 10:32 AM, Peter A. Bigot wrote:
The code to set the MMC partition uses an weak function to obtain the correct partition number. Use that instead of the compile-time default when deciding whether it needs to switch back.
Yes, this clearly fixes a bug.
Can you also please add a Fixes: tag that refers to the commit which introduced the problem (i.e. which updated mmc_set_env_part() to call mmc_get_env_part(), but forgot to update fini_mmc_for_env() to match.
Done.
If this tag is important enough to ask people to add it and resubmit their patches with no other changes, it should probably be described at http://www.denx.de/wiki/view/U-Boot/Patches#Review_Process_Git_Tags and suggested in the section on general patch submission rules, so the poor contributor might have a chance of being able to avoid the rework.
Peter
On 09/03/2014 11:30 AM, Peter A. Bigot wrote:
On 09/03/2014 11:52 AM, Stephen Warren wrote:
On 09/03/2014 10:32 AM, Peter A. Bigot wrote:
The code to set the MMC partition uses an weak function to obtain the correct partition number. Use that instead of the compile-time default when deciding whether it needs to switch back.
Yes, this clearly fixes a bug.
Can you also please add a Fixes: tag that refers to the commit which introduced the problem (i.e. which updated mmc_set_env_part() to call mmc_get_env_part(), but forgot to update fini_mmc_for_env() to match.
Done.
If this tag is important enough to ask people to add it and resubmit their patches with no other changes, it should probably be described at http://www.denx.de/wiki/view/U-Boot/Patches#Review_Process_Git_Tags and suggested in the section on general patch submission rules, so the poor contributor might have a chance of being able to avoid the rework.
I'd expect that if the only issue was a patch was a missing fixes line, the person applying the patch could manually edit it in when applying the patch, so all the contributor would have to do is reply to the email with the desired content. Still, different committers have different levels of tolerance for this, so YMMV!
On 09/03/2014 12:46 PM, Stephen Warren wrote:
On 09/03/2014 11:30 AM, Peter A. Bigot wrote:
On 09/03/2014 11:52 AM, Stephen Warren wrote:
On 09/03/2014 10:32 AM, Peter A. Bigot wrote:
The code to set the MMC partition uses an weak function to obtain the correct partition number. Use that instead of the compile-time default when deciding whether it needs to switch back.
Yes, this clearly fixes a bug.
Can you also please add a Fixes: tag that refers to the commit which introduced the problem (i.e. which updated mmc_set_env_part() to call mmc_get_env_part(), but forgot to update fini_mmc_for_env() to match.
Done.
If this tag is important enough to ask people to add it and resubmit their patches with no other changes, it should probably be described at http://www.denx.de/wiki/view/U-Boot/Patches#Review_Process_Git_Tags and suggested in the section on general patch submission rules, so the poor contributor might have a chance of being able to avoid the rework.
I'd expect that if the only issue was a patch was a missing fixes line, the person applying the patch could manually edit it in when applying the patch, so all the contributor would have to do is reply to the email with the desired content. Still, different committers have different levels of tolerance for this, so YMMV!
Ah; I'd forgotten that patchwork collects that sort of thing. The need to provide the tag should still be described in the patch process wiki pages, but replying would have been an easier solution if I'd known that doing so would have satisfied your request to add it.
Peter
The code to set the MMC partition uses an weak function to obtain the correct partition number. Use that instead of the compile-time default when deciding whether it needs to switch back.
Fixes: 6e7b7df4df43574 ("env_mmc: support env partition setup in runtime") Signed-off-by: Peter A. Bigot pab@pabigot.com --- V3: * Add Fixes line as requested
V2: * Preserve desired behavior of avoiding diagnostic when no HW partition supported * Supersedes https://patchwork.ozlabs.org/patch/385355/
common/env_mmc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/common/env_mmc.c b/common/env_mmc.c index a7621a8..14648e3 100644 --- a/common/env_mmc.c +++ b/common/env_mmc.c @@ -113,7 +113,7 @@ static void fini_mmc_for_env(struct mmc *mmc) #ifdef CONFIG_SPL_BUILD dev = 0; #endif - if (CONFIG_SYS_MMC_ENV_PART != mmc->part_num) + if (mmc_get_env_part(mmc) != mmc->part_num) mmc_switch_part(dev, mmc->part_num); #endif }
Hi Peter,
On 09/03/14 20:22, Peter A. Bigot wrote:
The code to set the MMC partition uses an weak function to obtain the correct partition number. Use that instead of the compile-time default when deciding whether it needs to switch back.
Fixes: 6e7b7df4df43574 ("env_mmc: support env partition setup in runtime")
It is sometimes also useful to Cc the original author of the patch. Cc: Dmitry Lifshitz lifshitz@compulab.co.il
Signed-off-by: Peter A. Bigot pab@pabigot.com
V3:
- Add Fixes line as requested
V2:
- Preserve desired behavior of avoiding diagnostic when no HW partition supported
- Supersedes https://patchwork.ozlabs.org/patch/385355/
common/env_mmc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/common/env_mmc.c b/common/env_mmc.c index a7621a8..14648e3 100644 --- a/common/env_mmc.c +++ b/common/env_mmc.c @@ -113,7 +113,7 @@ static void fini_mmc_for_env(struct mmc *mmc) #ifdef CONFIG_SPL_BUILD dev = 0; #endif
- if (CONFIG_SYS_MMC_ENV_PART != mmc->part_num)
- if (mmc_get_env_part(mmc) != mmc->part_num) mmc_switch_part(dev, mmc->part_num);
#endif }
Hi,
On 09/03/2014 08:22 PM, Peter A. Bigot wrote:
The code to set the MMC partition uses an weak function to obtain the correct partition number. Use that instead of the compile-time default when deciding whether it needs to switch back.
Fixes: 6e7b7df4df43574 ("env_mmc: support env partition setup in runtime") Signed-off-by: Peter A. Bigot pab@pabigot.com
Thank you for fixing this.
Acked-by: Dmitry Lifshitz lifshitz@compulab.co.il
Dmitry
Hi Peter,
On Sep 3, 2014, at 8:22 PM, Peter A. Bigot pab@pabigot.com wrote:
The code to set the MMC partition uses an weak function to obtain the correct partition number. Use that instead of the compile-time default when deciding whether it needs to switch back.
Fixes: 6e7b7df4df43574 ("env_mmc: support env partition setup in runtime") Signed-off-by: Peter A. Bigot pab@pabigot.com
V3:
- Add Fixes line as requested
V2:
- Preserve desired behavior of avoiding diagnostic when no HW partition supported
- Supersedes https://patchwork.ozlabs.org/patch/385355/
common/env_mmc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/common/env_mmc.c b/common/env_mmc.c index a7621a8..14648e3 100644 --- a/common/env_mmc.c +++ b/common/env_mmc.c @@ -113,7 +113,7 @@ static void fini_mmc_for_env(struct mmc *mmc) #ifdef CONFIG_SPL_BUILD dev = 0; #endif
- if (CONFIG_SYS_MMC_ENV_PART != mmc->part_num)
- if (mmc_get_env_part(mmc) != mmc->part_num) mmc_switch_part(dev, mmc->part_num);
#endif } -- 1.8.5.5
Applied, thanks.
Acked-by: Pantelis Antoniou panto@antoniou-consulting.com
The capacity and lba for an MMC device with part_num 0 reflects the whole device. When mmc_switch_part() successfully switches to a partition, the capacity is changed to that partition. As partition 0 does not physically exist, attempts to switch back to the whole device will indicate an error, but the capacity setting for the whole device must still be restored to match the partition.
Signed-off-by: Peter A. Bigot pab@pabigot.com --- drivers/mmc/mmc.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c index a26f3ce..fa04a3f 100644 --- a/drivers/mmc/mmc.c +++ b/drivers/mmc/mmc.c @@ -594,10 +594,15 @@ int mmc_switch_part(int dev_num, unsigned int part_num) ret = mmc_switch(mmc, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_PART_CONF, (mmc->part_config & ~PART_ACCESS_MASK) | (part_num & PART_ACCESS_MASK)); - if (ret) - return ret;
- return mmc_set_capacity(mmc, part_num); + /* + * Set the capacity if the switch succeeded or was intended + * to return to representing the raw device. + */ + if ((ret == 0) || ((ret == -ENODEV) && (part_num == 0))) + ret = mmc_set_capacity(mmc, part_num); + + return ret; }
int mmc_getcd(struct mmc *mmc)
On 09/02/2014 05:31 PM, Peter A. Bigot wrote:
The capacity and lba for an MMC device with part_num 0 reflects the whole device. When mmc_switch_part() successfully switches to a partition, the capacity is changed to that partition. As partition 0 does not physically exist, attempts to switch back to the whole device will indicate an error, but the capacity setting for the whole device must still be restored to match the partition.
diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
@@ -594,10 +594,15 @@ int mmc_switch_part(int dev_num, unsigned int part_num) ret = mmc_switch(mmc, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_PART_CONF, (mmc->part_config & ~PART_ACCESS_MASK) | (part_num & PART_ACCESS_MASK));
if (ret)
return ret;return mmc_set_capacity(mmc, part_num);
- /*
* Set the capacity if the switch succeeded or was intended* to return to representing the raw device.*/- if ((ret == 0) || ((ret == -ENODEV) && (part_num == 0)))
ret = mmc_set_capacity(mmc, part_num);- return ret; }
I think this wouldn't be needed without patch 1/2, since without that patch, no partition switching should ever happen if HW partitions don't exist, and hence this patch shouldn't be required.
On 09/03/2014 10:48 AM, Stephen Warren wrote:
On 09/02/2014 05:31 PM, Peter A. Bigot wrote:
The capacity and lba for an MMC device with part_num 0 reflects the whole device. When mmc_switch_part() successfully switches to a partition, the capacity is changed to that partition. As partition 0 does not physically exist, attempts to switch back to the whole device will indicate an error, but the capacity setting for the whole device must still be restored to match the partition.
diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
@@ -594,10 +594,15 @@ int mmc_switch_part(int dev_num, unsigned int part_num) ret = mmc_switch(mmc, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_PART_CONF, (mmc->part_config & ~PART_ACCESS_MASK) | (part_num & PART_ACCESS_MASK));
if (ret)
return ret;return mmc_set_capacity(mmc, part_num);
- /*
* Set the capacity if the switch succeeded or was intended* to return to representing the raw device.*/- if ((ret == 0) || ((ret == -ENODEV) && (part_num == 0)))
ret = mmc_set_capacity(mmc, part_num);- return ret; }
I think this wouldn't be needed without patch 1/2, since without that patch, no partition switching should ever happen if HW partitions don't exist, and hence this patch shouldn't be required.
Not so.
In SPL mode, the mmc device passed in to the environment code is set up for partition 0. In the failure case, u-boot is configured to expect an environment in partition 2, and so invokes mmc_switch_part to go to partition 2 to see if it's a valid partition. In my case that fails because the partition size is zero, but regardless the mmc_switch_part back to mmc->part_num fails because the mmc_switch() call rejects the attempt with an error.
Without this second patch, you end up with mmc->part_num left at zero but the capacity/lba fields configured for partition 2 which does not exist and has size zero, and SPL is unable to locate u-boot.img to continue.
Please review the details in the meta-ti discussion.
I'll respond to the comments on patch 1 separately.
Peter
On 09/03/2014 09:59 AM, Peter A. Bigot wrote:
On 09/03/2014 10:48 AM, Stephen Warren wrote:
On 09/02/2014 05:31 PM, Peter A. Bigot wrote:
The capacity and lba for an MMC device with part_num 0 reflects the whole device. When mmc_switch_part() successfully switches to a partition, the capacity is changed to that partition. As partition 0 does not physically exist, attempts to switch back to the whole device will indicate an error, but the capacity setting for the whole device must still be restored to match the partition.
diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
@@ -594,10 +594,15 @@ int mmc_switch_part(int dev_num, unsigned int part_num) ret = mmc_switch(mmc, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_PART_CONF, (mmc->part_config & ~PART_ACCESS_MASK) | (part_num & PART_ACCESS_MASK));
if (ret)
return ret;return mmc_set_capacity(mmc, part_num);
- /*
* Set the capacity if the switch succeeded or was intended* to return to representing the raw device.*/- if ((ret == 0) || ((ret == -ENODEV) && (part_num == 0)))
ret = mmc_set_capacity(mmc, part_num);- return ret; }
I think this wouldn't be needed without patch 1/2, since without that patch, no partition switching should ever happen if HW partitions don't exist, and hence this patch shouldn't be required.
Not so.
In SPL mode, the mmc device passed in to the environment code is set up for partition 0. In the failure case, u-boot is configured to expect an
What failure case?
environment in partition 2, and so invokes mmc_switch_part to go to partition 2 to see if it's a valid partition. In my case that fails because the partition size is zero, but regardless the mmc_switch_part back to mmc->part_num fails because the mmc_switch() call rejects the attempt with an error.
Isn't that where the bug should be fixed then; why doesn't mmc_switch() work as desired? If mmc_switch() isn't intended to work on devices without HW partitions, then why is it being called at all in any case (normal or failure case)?
I also wonder why, if your board configuration is set up to assume an eMMC device with HW partitions, you're using a device without eMMC HW partitions; it seems like either the board configuration or your HW configuration is incorrect (or at least don't match), so if you have problems, it's not surprising, and not something that should be fixed.
Without this second patch, you end up with mmc->part_num left at zero but the capacity/lba fields configured for partition 2 which does not exist and has size zero, and SPL is unable to locate u-boot.img to continue.
Please review the details in the meta-ti discussion.
I have no idea what that is.
On 09/03/2014 11:05 AM, Stephen Warren wrote:
On 09/03/2014 09:59 AM, Peter A. Bigot wrote:
On 09/03/2014 10:48 AM, Stephen Warren wrote:
On 09/02/2014 05:31 PM, Peter A. Bigot wrote:
The capacity and lba for an MMC device with part_num 0 reflects the whole device. When mmc_switch_part() successfully switches to a partition, the capacity is changed to that partition. As partition 0 does not physically exist, attempts to switch back to the whole device will indicate an error, but the capacity setting for the whole device must still be restored to match the partition.
diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
@@ -594,10 +594,15 @@ int mmc_switch_part(int dev_num, unsigned int part_num) ret = mmc_switch(mmc, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_PART_CONF, (mmc->part_config & ~PART_ACCESS_MASK) | (part_num & PART_ACCESS_MASK));
if (ret)
return ret;return mmc_set_capacity(mmc, part_num);
- /*
* Set the capacity if the switch succeeded or was intended* to return to representing the raw device.*/- if ((ret == 0) || ((ret == -ENODEV) && (part_num == 0)))
ret = mmc_set_capacity(mmc, part_num);- return ret; }
I think this wouldn't be needed without patch 1/2, since without that patch, no partition switching should ever happen if HW partitions don't exist, and hence this patch shouldn't be required.
Not so.
In SPL mode, the mmc device passed in to the environment code is set up for partition 0. In the failure case, u-boot is configured to expect an
What failure case?
environment in partition 2, and so invokes mmc_switch_part to go to partition 2 to see if it's a valid partition. In my case that fails because the partition size is zero, but regardless the mmc_switch_part back to mmc->part_num fails because the mmc_switch() call rejects the attempt with an error.
Isn't that where the bug should be fixed then; why doesn't mmc_switch() work as desired? If mmc_switch() isn't intended to work on devices without HW partitions, then why is it being called at all in any case (normal or failure case)?
I have no idea.
I also wonder why, if your board configuration is set up to assume an eMMC device with HW partitions, you're using a device without eMMC HW partitions; it seems like either the board configuration or your HW configuration is incorrect (or at least don't match), so if you have problems, it's not surprising, and not something that should be fixed.
This is a beaglebone (black). It has an eMMC, and an SD card. It can boot from either, and will fall back to the SD card if the eMMC is uninitialized or under other magic conditions.
If you believe the beaglebone u-boot configuration is wrong for how the beaglebone is intended to be used, I'll refer you to the TI folks to sort it out.
Without this second patch, you end up with mmc->part_num left at zero but the capacity/lba fields configured for partition 2 which does not exist and has size zero, and SPL is unable to locate u-boot.img to continue.
Please review the details in the meta-ti discussion.
I have no idea what that is.
The link in the cover letter I provided with the patches, in hopes it would answer questions about why I was doing this. It all starts here:
http://www.mail-archive.com/meta-ti@yoctoproject.org/msg04276.html
Tom Rini: OK, so I've provided the patch upstream as you requested. I'm not going to continue to fight to get it incorporated. I believe you understand that there's a problem, and it's on hardware you're probably paid to support, unlike me. Y'all can figure out whether and how you want to resolve it.
Peter
On Tue, Sep 02, 2014 at 06:31:23PM -0500, Peter A. Bigot wrote:
The capacity and lba for an MMC device with part_num 0 reflects the whole device. When mmc_switch_part() successfully switches to a partition, the capacity is changed to that partition. As partition 0 does not physically exist, attempts to switch back to the whole device will indicate an error, but the capacity setting for the whole device must still be restored to match the partition.
Signed-off-by: Peter A. Bigot pab@pabigot.com
Tested-by: Tom Rini trini@ti.com
Hi Tom,
On Sep 11, 2014, at 8:45 PM, Tom Rini trini@ti.com wrote:
On Tue, Sep 02, 2014 at 06:31:23PM -0500, Peter A. Bigot wrote:
The capacity and lba for an MMC device with part_num 0 reflects the whole device. When mmc_switch_part() successfully switches to a partition, the capacity is changed to that partition. As partition 0 does not physically exist, attempts to switch back to the whole device will indicate an error, but the capacity setting for the whole device must still be restored to match the partition.
Signed-off-by: Peter A. Bigot pab@pabigot.com
Tested-by: Tom Rini trini@ti.com
I’m going to take this in. It’s required to get the bone to boot from a standard MMC and I’d rather not introduce another boneblack variant to get it to boot from MMC.
-- Tom
Tested-by: Pantelis Antoniou panto@antoniou-consulting.com Acked-by: Pantelis Antoniou panto@antoniou-consulting.com
On Tue, Sep 02, 2014 at 06:31:21PM -0500, Peter A. Bigot wrote:
This series aims at addressing an issue discovered with SPL mode when the MMC device being used lacks an environment partition. http://www.mail-archive.com/meta-ti@yoctoproject.org/msg04320.html includes details on the original failure with this diagnosis:
This is a bug in handling mmc_switch_part: what's happening is that the code reconfigures the mmc device to look at the partition on which the environment is to be found, but fails to restore it to reflect the state of the whole device. I.e., the mmc capacity and lba are zero in my case (I have no partition 2 on the uSD card), but mmc_switch_part() returns -ENODEV on the attempt to switch back in fini_mmc_for_env() without also resetting the capacity to what the rest of the system expects.
1/2 has been superseded and there's some questions about 2/2. I'm going to take 2/2 as it fixes a real problem. But Stephen brings up some good questions that do need to be answered. The first thing is that SPL today assumes that it only has one MMC device registered with the subsystem, not 2. This may be reworkable (and maybe not have a big size change) but it's a bit late in this release cycle for that. So what this means is that on some hardware like say Beaglebone Black we either have an SD card (which lacks physical partitions) or we have an eMMC which means the 2 boot partitions and the user partition.
The next part of the problem is that we have some cases where we say "environment is on eMMC, in one of the boot partitions" and we say "SPL needs to look at the environment". This situation works fine when we boot from eMMC. Things fail when we use this same binary to boot from SD card. We don't have env and somewhere along the line what fails is that we tried to switch physical partition, noticed a failure, tried to correct said failure but instead end up with our internal representation of the SD/MMC device being invalid. Peter's patch 2/2 corrects this so that when we hit a failure it goes and re-sets that representation.
But the next question is "wait, why did it get set _wrong_ to start with?". I'm not sure actually. The fini_mmc_for_env call isn't making us be restored to what the physical partition was before, if that's what it was intended to do, it's making sure that we're still on the env partition. Which I'm not sure why it needs to do since we've already read a copy into memory at this point.
participants (6)
-
Dmitry Lifshitz -
Igor Grinberg -
Pantelis Antoniou -
Peter A. Bigot -
Stephen Warren -
Tom Rini