* [U-Boot] [PATCH v2] mmc: Update "mmc->part_num" when performing a partition switch @ 2013-06-04 19:17 Fabio Estevam 2013-06-04 19:21 ` Stephen Warren 0 siblings, 1 reply; 11+ messages in thread From: Fabio Estevam @ 2013-06-04 19:17 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [U-Boot] [PATCH v2] mmc: Update "mmc->part_num" when performing a partition switch 2013-06-04 19:17 [U-Boot] [PATCH v2] mmc: Update "mmc->part_num" when performing a partition switch Fabio Estevam @ 2013-06-04 19:21 ` Stephen Warren 2013-06-04 19:30 ` Fabio Estevam 0 siblings, 1 reply; 11+ messages in thread From: Stephen Warren @ 2013-06-04 19:21 UTC (permalink / raw) To: u-boot 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? ^ permalink raw reply [flat|nested] 11+ messages in thread
* [U-Boot] [PATCH v2] mmc: Update "mmc->part_num" when performing a partition switch 2013-06-04 19:21 ` Stephen Warren @ 2013-06-04 19:30 ` Fabio Estevam 2013-06-04 19:36 ` Stephen Warren 0 siblings, 1 reply; 11+ messages in thread From: Fabio Estevam @ 2013-06-04 19:30 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [U-Boot] [PATCH v2] mmc: Update "mmc->part_num" when performing a partition switch 2013-06-04 19:30 ` Fabio Estevam @ 2013-06-04 19:36 ` Stephen Warren 2013-06-04 20:45 ` Fabio Estevam 0 siblings, 1 reply; 11+ messages in thread From: Stephen Warren @ 2013-06-04 19:36 UTC (permalink / raw) To: u-boot 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* [U-Boot] [PATCH v2] mmc: Update "mmc->part_num" when performing a partition switch 2013-06-04 19:36 ` Stephen Warren @ 2013-06-04 20:45 ` Fabio Estevam 2013-06-04 22:12 ` Stephen Warren 0 siblings, 1 reply; 11+ messages in thread From: Fabio Estevam @ 2013-06-04 20:45 UTC (permalink / raw) To: u-boot 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() ? ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [U-Boot] [PATCH v2] mmc: Update "mmc->part_num" when performing a partition switch 2013-06-04 20:45 ` Fabio Estevam @ 2013-06-04 22:12 ` Stephen Warren 2013-06-04 23:09 ` Fabio Estevam 2013-06-04 23:16 ` Fabio Estevam 0 siblings, 2 replies; 11+ messages in thread From: Stephen Warren @ 2013-06-04 22:12 UTC (permalink / raw) To: u-boot 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 } ^ permalink raw reply [flat|nested] 11+ messages in thread
* [U-Boot] [PATCH v2] mmc: Update "mmc->part_num" when performing a partition switch 2013-06-04 22:12 ` Stephen Warren @ 2013-06-04 23:09 ` Fabio Estevam 2013-06-04 23:30 ` Stephen Warren 2013-06-04 23:16 ` Fabio Estevam 1 sibling, 1 reply; 11+ messages in thread From: Fabio Estevam @ 2013-06-04 23:09 UTC (permalink / raw) To: u-boot 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 > ^ permalink raw reply [flat|nested] 11+ messages in thread
* [U-Boot] [PATCH v2] mmc: Update "mmc->part_num" when performing a partition switch 2013-06-04 23:09 ` Fabio Estevam @ 2013-06-04 23:30 ` Stephen Warren 2013-06-04 23:48 ` Fabio Estevam 0 siblings, 1 reply; 11+ messages in thread From: Stephen Warren @ 2013-06-04 23:30 UTC (permalink / raw) To: u-boot 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* [U-Boot] [PATCH v2] mmc: Update "mmc->part_num" when performing a partition switch 2013-06-04 23:30 ` Stephen Warren @ 2013-06-04 23:48 ` Fabio Estevam 0 siblings, 0 replies; 11+ messages in thread From: Fabio Estevam @ 2013-06-04 23:48 UTC (permalink / raw) To: u-boot 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* [U-Boot] [PATCH v2] mmc: Update "mmc->part_num" when performing a partition switch 2013-06-04 22:12 ` Stephen Warren 2013-06-04 23:09 ` Fabio Estevam @ 2013-06-04 23:16 ` Fabio Estevam 2013-06-04 23:31 ` Stephen Warren 1 sibling, 1 reply; 11+ messages in thread From: Fabio Estevam @ 2013-06-04 23:16 UTC (permalink / raw) To: u-boot 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* [U-Boot] [PATCH v2] mmc: Update "mmc->part_num" when performing a partition switch 2013-06-04 23:16 ` Fabio Estevam @ 2013-06-04 23:31 ` Stephen Warren 0 siblings, 0 replies; 11+ messages in thread From: Stephen Warren @ 2013-06-04 23:31 UTC (permalink / raw) To: u-boot 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2013-06-04 23:48 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-06-04 19:17 [U-Boot] [PATCH v2] mmc: Update "mmc->part_num" when performing a partition switch Fabio Estevam 2013-06-04 19:21 ` Stephen Warren 2013-06-04 19:30 ` Fabio Estevam 2013-06-04 19:36 ` Stephen Warren 2013-06-04 20:45 ` Fabio Estevam 2013-06-04 22:12 ` Stephen Warren 2013-06-04 23:09 ` Fabio Estevam 2013-06-04 23:30 ` Stephen Warren 2013-06-04 23:48 ` Fabio Estevam 2013-06-04 23:16 ` Fabio Estevam 2013-06-04 23:31 ` Stephen Warren
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox