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

[Tom Rini]

On Tue, Sep 02, 2014 at 12:00:49PM -0500, Peter A. Bigot wrote:
> 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.

OK.

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

This is probably "good enough" for meta-ti, especially if you get it
into mainline as it could be in v2014.10.

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

Well, Pantelis?  How does this all look when compared with master?

-- 
Tom