[U-Boot] [PATCH v2] mmc: Update "mmc->part_num" when performing a partition switch

[U-Boot] [PATCH v2] mmc: Update "mmc->part_num" when performing a partition switch

[Fabio Estevam]

When running the "save" command several times on a mx6qsabresd we see:

U-Boot > save
Saving Environment to MMC...
Writing to MMC(1)... done
U-Boot > save
Saving Environment to MMC...
MMC partition switch failed
U-Boot > save
Saving Environment to MMC...
Writing to MMC(1)... done
U-Boot > save
Saving Environment to MMC...
MMC partition switch failed
U-Boot > save
Saving Environment to MMC...
Writing to MMC(1)... done
U-Boot > save
Saving Environment to MMC...
MMC partition switch failed

Fix this by updating mmc->part_num inside mmc_switch_part() after a succesful
mmc partition switch.

After this fix, we no longer see the error after the "save" command on a 
mx6qsabresd. Also tested on a mx53loco.

Reported-by: Jason Liu <r64343@freescale.com>

Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
Changes since v1:
- Do the change inside the mmc core

 drivers/mmc/mmc.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
index 2590f1b..7f568ed 100644
--- a/drivers/mmc/mmc.c
+++ b/drivers/mmc/mmc.c
@@ -680,14 +680,20 @@ static int mmc_change_freq(struct mmc *mmc)
 
 int mmc_switch_part(int dev_num, unsigned int part_num)
 {
+	int ret;
 	struct mmc *mmc = find_mmc_device(dev_num);
 
 	if (!mmc)
 		return -1;
 
-	return mmc_switch(mmc, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_PART_CONF,
+	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)
+		mmc->part_num = part_num;
+
+	return ret;
 }
 
 int mmc_getcd(struct mmc *mmc)
-- 
1.8.1.2

[U-Boot] [PATCH v2] mmc: Update "mmc->part_num" when performing a partition switch

[Stephen Warren]

On 06/04/2013 01:17 PM, Fabio Estevam wrote:
> When running the "save" command several times on a mx6qsabresd we see:
> 
> U-Boot > save
> Saving Environment to MMC...
> Writing to MMC(1)... done
> U-Boot > save
> Saving Environment to MMC...
> MMC partition switch failed
> U-Boot > save
> Saving Environment to MMC...
> Writing to MMC(1)... done
> U-Boot > save
> Saving Environment to MMC...
> MMC partition switch failed
> U-Boot > save
> Saving Environment to MMC...
> Writing to MMC(1)... done
> U-Boot > save
> Saving Environment to MMC...
> MMC partition switch failed
> 
> Fix this by updating mmc->part_num inside mmc_switch_part() after a succesful
> mmc partition switch.

This surely has exactly the same issue as the previous version, where
the update of part_num was done inside common/env_mmc.c?

[U-Boot] [PATCH v2] mmc: Update "mmc->part_num" when performing a partition switch

[Fabio Estevam]

On Tue, Jun 4, 2013 at 4:21 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:

> This surely has exactly the same issue as the previous version, where
> the update of part_num was done inside common/env_mmc.c?

My understading is that the mmc core should update mmc->part_num and I
don't see such update, or am I missing something?

Thanks,

Fabio Estevam

[U-Boot] [PATCH v2] mmc: Update "mmc->part_num" when performing a partition switch

[Stephen Warren]

On 06/04/2013 01:30 PM, Fabio Estevam wrote:
> On Tue, Jun 4, 2013 at 4:21 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> 
>> This surely has exactly the same issue as the previous version, where
>> the update of part_num was done inside common/env_mmc.c?
> 
> My understading is that the mmc core should update mmc->part_num and I
> don't see such update, or am I missing something?

That seems like a reasonable way for the code to work. However, you'd
need to modify common/env_mmc.c:init_mmc_for_env() so that it saves off
mmc->part_num before switching MMC partitions, so that
fini_mmc_for_env() knows which partition to switch back to. Right now,
it relies on the fact that mmc_switch_part() does not update
mmc->part_num, and hence uses that value to save the previously selected
partition ID.

Also, if you make this change, I think you can update
common/cmd_mmc.c:do_mmcops(), since it will no longer need to update
mmc->part_num after a successful switch.

[U-Boot] [PATCH v2] mmc: Update "mmc->part_num" when performing a partition switch

[Fabio Estevam]

Hi Stephen,

On Tue, Jun 4, 2013 at 4:36 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:

> That seems like a reasonable way for the code to work. However, you'd
> need to modify common/env_mmc.c:init_mmc_for_env() so that it saves off
> mmc->part_num before switching MMC partitions, so that
> fini_mmc_for_env() knows which partition to switch back to. Right now,
> it relies on the fact that mmc_switch_part() does not update
> mmc->part_num, and hence uses that value to save the previously selected
> partition ID.
>
> Also, if you make this change, I think you can update
> common/cmd_mmc.c:do_mmcops(), since it will no longer need to update
> mmc->part_num after a successful switch.

I understood your second suggestion and did the changes below, but I
am not sure on the first one regarding the change you proposed inside
init_mmc_for_env().

Here is what I did so far:

diff --git a/common/cmd_mmc.c b/common/cmd_mmc.c
index 7d82469..20a2792 100644
--- a/common/cmd_mmc.c
+++ b/common/cmd_mmc.c
@@ -243,8 +243,6 @@ static int do_mmcops(cmd_tbl_t *cmdtp, int flag,
int argc, char * const argv[])

 			if (part != mmc->part_num) {
 				ret = mmc_switch_part(dev, part);
-				if (!ret)
-					mmc->part_num = part;

 				printf("switch to partions #%d, %s\n",
 						part, (!ret) ? "OK" : "ERROR");
diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
index 0a2f535..e8d2ea6 100644
--- a/drivers/mmc/mmc.c
+++ b/drivers/mmc/mmc.c
@@ -702,14 +702,20 @@ static int mmc_change_freq(struct mmc *mmc)

 int mmc_switch_part(int dev_num, unsigned int part_num)
 {
+	int ret;
 	struct mmc *mmc = find_mmc_device(dev_num);

 	if (!mmc)
 		return -1;

-	return mmc_switch(mmc, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_PART_CONF,
+	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)
+		mmc->part_num = part_num;
+
+	return ret;
 }

 int mmc_getcd(struct mmc *mmc)
-- 
1.8.1.2

Could you please explain the change you proposed for saving
mmc->part_num inside init_mmc_for_env() ?

[U-Boot] [PATCH v2] mmc: Update "mmc->part_num" when performing a partition switch

[Stephen Warren]

On 06/04/2013 02:45 PM, Fabio Estevam wrote:
> Hi Stephen,
> 
> On Tue, Jun 4, 2013 at 4:36 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> 
>> That seems like a reasonable way for the code to work. However, you'd
>> need to modify common/env_mmc.c:init_mmc_for_env() so that it saves off
>> mmc->part_num before switching MMC partitions, so that
>> fini_mmc_for_env() knows which partition to switch back to. Right now,
>> it relies on the fact that mmc_switch_part() does not update
>> mmc->part_num, and hence uses that value to save the previously selected
>> partition ID.
>>
>> Also, if you make this change, I think you can update
>> common/cmd_mmc.c:do_mmcops(), since it will no longer need to update
>> mmc->part_num after a successful switch.
> 
> I understood your second suggestion and did the changes below, but I
> am not sure on the first one regarding the change you proposed inside
> init_mmc_for_env().

In env_mmc.c, you'll want something like:

+#ifdef CONFIG_SYS_MMC_ENV_PART
+static char orig_part_num;
+#ifdef CONFIG_SYS_MMC_ENV_PART

 static int init_mmc_for_env(struct mmc *mmc)
 ...
 #ifdef CONFIG_SYS_MMC_ENV_PART
+	orig_part_num = mmc->part_num;
 	if (CONFIG_SYS_MMC_ENV_PART != mmc->part_num) {
 		if (mmc_switch_part(CONFIG_SYS_MMC_ENV_DEV,
 				    CONFIG_SYS_MMC_ENV_PART)) {
 			puts("MMC partition switch failed\n");
 			return -1;
 		}
 	}
 #endif
...
 static void fini_mmc_for_env(struct mmc *mmc)
 {
 #ifdef CONFIG_SYS_MMC_ENV_PART
-	if (CONFIG_SYS_MMC_ENV_PART != mmc->part_num)
+ 	if (mmc->part_num != orig_part_num)
 		mmc_switch_part(CONFIG_SYS_MMC_ENV_DEV,
-				mmc->part_num);
+				orig_part_num);
 #endif
 }

[U-Boot] [PATCH v2] mmc: Update "mmc->part_num" when performing a partition switch

[Fabio Estevam]

On Tue, Jun 4, 2013 at 7:12 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:

> In env_mmc.c, you'll want something like:

Thanks for the patch, but when I add these changes on top of my patch
it results in the original error:

U-Boot > save
Saving Environment to MMC...
Writing to MMC(1)... done
U-Boot > save
Saving Environment to MMC...
MMC partition switch failed
U-Boot >

[U-Boot] [PATCH v2] mmc: Update "mmc->part_num" when performing a partition switch

[Fabio Estevam]

On Tue, Jun 4, 2013 at 7:12 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:

>  static void fini_mmc_for_env(struct mmc *mmc)
>  {
>  #ifdef CONFIG_SYS_MMC_ENV_PART
> -       if (CONFIG_SYS_MMC_ENV_PART != mmc->part_num)
> +       if (mmc->part_num != orig_part_num)
>                 mmc_switch_part(CONFIG_SYS_MMC_ENV_DEV,
> -                               mmc->part_num);
> +                               orig_part_num);

If I keep the original 'mmc->part_num' here, then it works.

[U-Boot] [PATCH v2] mmc: Update "mmc->part_num" when performing a partition switch

[Stephen Warren]

On 06/04/2013 05:09 PM, Fabio Estevam wrote:
> On Tue, Jun 4, 2013 at 7:12 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> 
>> In env_mmc.c, you'll want something like:
> 
> Thanks for the patch, but when I add these changes on top of my patch
> it results in the original error:

Of course; with that patch applied, there is no effective difference in
the code - you've just cleaned it up to have the MMC core manage
mmc->part_num, rather than requiring all callers to update it themselves.

> U-Boot > save
> Saving Environment to MMC...
> Writing to MMC(1)... done
> U-Boot > save
> Saving Environment to MMC...
> MMC partition switch failed
> U-Boot >

I have no idea why that happens. You'll simply have to debug the code.
Do you have CONFIG_SYS_MMC_ENV_PART set? I wasn't aware anyone else used
it, besides Tegra.

[U-Boot] [PATCH v2] mmc: Update "mmc->part_num" when performing a partition switch

[Stephen Warren]

On 06/04/2013 05:16 PM, Fabio Estevam wrote:
> On Tue, Jun 4, 2013 at 7:12 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> 
>>  static void fini_mmc_for_env(struct mmc *mmc)
>>  {
>>  #ifdef CONFIG_SYS_MMC_ENV_PART
>> -       if (CONFIG_SYS_MMC_ENV_PART != mmc->part_num)
>> +       if (mmc->part_num != orig_part_num)
>>                 mmc_switch_part(CONFIG_SYS_MMC_ENV_DEV,
>> -                               mmc->part_num);
>> +                               orig_part_num);
> 
> If I keep the original 'mmc->part_num' here, then it works.

It doesn't "work", it just hides the problem. By passing mmc->part_num
here, you're passing the partition that's already/currently selected
rather than restoring the original value. Selecting it again is a no-op;
IIRC the MMC core explicitly checks for this condition and immediately
returns without touching the HW.

[U-Boot] [PATCH v2] mmc: Update "mmc->part_num" when performing a partition switch

[Fabio Estevam]

On Tue, Jun 4, 2013 at 8:30 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:

> Do you have CONFIG_SYS_MMC_ENV_PART set? I wasn't aware anyone else used
> it, besides Tegra.

Yes, this is the issue. CONFIG_SYS_MMC_ENV_PART is used incorrectly on
this board.

Will update its config file.