[meta-ti] [ti-uboot][PATCH] mmc: restore capacity when switching to partition 0

[Peter A. Bigot]

On 09/02/2014 11:44 AM, Tom Rini wrote:
> On Mon, Sep 01, 2014 at 10:11:30PM -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 should still restore the capacity setting.
> In other words:
> # mmc dev 0:1
> ...
> # mmc dev 0:0
>
> Fails?

I don't know.  This error occurs in SPL mode where there is no command 
line and only one device.  From the companion email:

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.

>   And the following patch fixes it.
>
>> Signed-off-by: Peter A. Bigot <pab at pabigot.com>
>> ---
>>   drivers/mmc/mmc.c | 7 ++++---
>>   1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
>> index b5477b1..b05c6ee 100644
>> --- a/drivers/mmc/mmc.c
>> +++ b/drivers/mmc/mmc.c
>> @@ -596,10 +596,11 @@ 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);
>> +	if ((0 == ret) || ((-ENODEV == ret) && (0 == part_num)))

Yes.  There are several ways to fix it; this is the one with the minimal 
change from existing behavior.  (It is necessary to try the mmc_switch() 
call even if part_num is zero since that resets some internal driver state.)

> This is backwards from how we usually code things:
>
> if ((ret == 0) || ((ret == -ENODEV) && (part_num == 0)))

OK; I default to yoda conditions since it saves me at least once a year, 
but can swap things back around.  No objection to explicit 
parenthesization (the GNU folks hate it).

> And should have a comment above it that's a short summary of the commit
> message so this doesn't get lost later on.

OK.

>
>> +		ret = mmc_set_capacity(mmc, part_num);
>> +
>> +	return ret;
>>   }
>>   
>>   int mmc_getcd(struct mmc *mmc)
>> -- 
>> 1.8.5.5
>>
>> -- 
>> _______________________________________________
>> meta-ti mailing list
>> meta-ti at yoctoproject.org
>> https://lists.yoctoproject.org/listinfo/meta-ti

If I update this patch, will you see the patch gets into ti-uboot?

> And speaking of getting lost, this isn't the U-Boot mailing list so the
> change would get lost with the next release.  Please make sure to post
> the next version to the u-boot ML as well and CC Pantelis.   Thanks!

This fix is specific to 2014.07, and may or may not also be influenced 
by TI-specific patches.  AFAICT u-boot doesn't maintain a stable-release 
patch branch.

I mentioned in deleted material that subsequent changes rework how the 
environment code manipulates the mmc device so it doesn't apply to 
u-boot master.  There are several such changes that are cumulative, so 
there's some rework to be done.  I'd have to switch over to using u-boot 
master to see if the problem still remains in the current 
implementation.  At this time that's not in scope for me.

Peter