[U-Boot] [PATCH 0/2] fix issue with mmc partition management

[U-Boot] [PATCH 0/2] fix issue with mmc partition management

[Peter A. Bigot]

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 at 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(-)

-- 
1.8.5.5

[U-Boot] [PATCH 1/2] env_mmc: remove condition on call to mmc_switch_part

[Peter A. Bigot]

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
 }
 
-- 
1.8.5.5

[U-Boot] [PATCH 1/2] env_mmc: remove condition on call to mmc_switch_part

[Stephen Warren]

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.

[U-Boot] [PATCH 1/2] env_mmc: remove condition on call to mmc_switch_part

[Peter A. Bigot]

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

[U-Boot] [PATCH v2] env_mmc: correct fini partition to match init partition

[Peter A. Bigot]

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
 }
-- 
1.8.5.5

[U-Boot] [PATCH v2] env_mmc: correct fini partition to match init partition

[Stephen Warren]

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.

[U-Boot] [PATCH v2] env_mmc: correct fini partition to match init partition

[Peter A. Bigot]

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

[U-Boot] [PATCH v2] env_mmc: correct fini partition to match init partition

[Stephen Warren]

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!

[U-Boot] [PATCH v2] env_mmc: correct fini partition to match init partition

[Peter A. Bigot]

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

[U-Boot] [PATCH v3] env_mmc: correct fini partition to match init partition

[Peter A. Bigot]

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

[U-Boot] [PATCH v3] env_mmc: correct fini partition to match init partition

[Igor Grinberg]

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

-- 
Regards,
Igor.

[U-Boot] [PATCH v3] env_mmc: correct fini partition to match init partition

[Dmitry Lifshitz]

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

[U-Boot] [PATCH v3] env_mmc: correct fini partition to match init partition

[Pantelis Antoniou]

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>

[U-Boot] [PATCH 2/2] mmc: restore capacity when switching to partition 0

[Peter A. Bigot]

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)
-- 
1.8.5.5

[U-Boot] [PATCH 2/2] mmc: restore capacity when switching to partition 0

[Stephen Warren]

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.

[U-Boot] [PATCH 2/2] mmc: restore capacity when switching to partition 0

[Peter A. Bigot]

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

[U-Boot] [PATCH 2/2] mmc: restore capacity when switching to partition 0

[Stephen Warren]

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.

[U-Boot] [PATCH 2/2] mmc: restore capacity when switching to partition 0

[Peter A. Bigot]

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

[U-Boot] [PATCH 2/2] mmc: restore capacity when switching to partition 0

[Tom Rini]

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>

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20140911/78cb8ffa/attachment.pgp>

[U-Boot] [PATCH 2/2] mmc: restore capacity when switching to partition 0

[Pantelis Antoniou]

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>

[U-Boot] [PATCH 0/2] fix issue with mmc partition management

[Tom Rini]

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 at 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.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20140911/46421755/attachment.pgp>