| Message ID | 20200609081431.6376-1-zbestahu@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mmc: sdio: Fix 1-bit mode for SD-combo cards during suspend | expand |
On Tue, 9 Jun 2020 at 10:14, Yue Hu <zbestahu@gmail.com> wrote: > > From: Yue Hu <huyue2@yulong.com> > > Commit 6b5eda369ac3 ("sdio: put active devices into 1-bit mode during > suspend") disabled 4-bit mode during system suspend. After this patch, > commit 7310ece86ad7 ("mmc: implement SD-combo (IO+mem) support") used > new sdio_enable_4bit_bus() instead of sdio_enable_wide() to support > SD-combo cards, also for card resume. However, no corresponding support > added during suspend. That is not correct. Let's fix it. I believe the change makes sense to me. However, the commit 6b5eda369ac3 that you refer to is from v2.6.34, which is more than ten years ago. That makes me wonder, are these cards really being used? Did you test this with a combo card? > > Signed-off-by: Yue Hu <huyue2@yulong.com> > --- > drivers/mmc/core/sdio.c | 23 ++++++++++++++++++++++- > 1 file changed, 22 insertions(+), 1 deletion(-) > > diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c > index ebb387a..2d2ae35 100644 > --- a/drivers/mmc/core/sdio.c > +++ b/drivers/mmc/core/sdio.c > @@ -285,6 +285,27 @@ static int sdio_disable_wide(struct mmc_card *card) > return 0; > } > > +static int sdio_disable_4bit_bus(struct mmc_card *card) > +{ > + int err; > + > + if (card->type == MMC_TYPE_SDIO) > + goto out; > + > + if (!(card->host->caps & MMC_CAP_4_BIT_DATA)) > + return 0; > + > + if (!(card->scr.bus_widths & SD_SCR_BUS_WIDTH_4)) > + return 0; > + > + err = mmc_app_set_bus_width(card, MMC_BUS_WIDTH_1); > + if (err) > + return err; > + > +out: > + return sdio_disable_wide(card); > +} > + > > static int sdio_enable_4bit_bus(struct mmc_card *card) > { > @@ -960,7 +981,7 @@ static int mmc_sdio_suspend(struct mmc_host *host) > mmc_claim_host(host); > > if (mmc_card_keep_power(host) && mmc_card_wake_sdio_irq(host)) > - sdio_disable_wide(host->card); > + sdio_disable_4bit_bus(host->card); > > if (!mmc_card_keep_power(host)) { > mmc_power_off(host); > -- > 1.9.1 > Kind regards Uffe
On Tue, 9 Jun 2020 12:01:42 +0200 Ulf Hansson <ulf.hansson@linaro.org> wrote: > On Tue, 9 Jun 2020 at 10:14, Yue Hu <zbestahu@gmail.com> wrote: > > > > From: Yue Hu <huyue2@yulong.com> > > > > Commit 6b5eda369ac3 ("sdio: put active devices into 1-bit mode during > > suspend") disabled 4-bit mode during system suspend. After this patch, > > commit 7310ece86ad7 ("mmc: implement SD-combo (IO+mem) support") used > > new sdio_enable_4bit_bus() instead of sdio_enable_wide() to support > > SD-combo cards, also for card resume. However, no corresponding support > > added during suspend. That is not correct. Let's fix it. > > I believe the change makes sense to me. > > However, the commit 6b5eda369ac3 that you refer to is from v2.6.34, > which is more than ten years ago. That makes me wonder, are these > cards really being used? Current code logic will switch to 1-bit/4-bit mode in suspend/resume. > > Did you test this with a combo card? No, i have no real environment to test it. I'm just reading the code. Obviously, sdio_disable_wide() used in suspend is for SDIO cards only. However, sdio_enable_4bit_mode() used in resume is for SD-combo cards. We should also add the support in suspend to avoid the potential issue. Thank you. > > > > > Signed-off-by: Yue Hu <huyue2@yulong.com> > > --- > > drivers/mmc/core/sdio.c | 23 ++++++++++++++++++++++- > > 1 file changed, 22 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c > > index ebb387a..2d2ae35 100644 > > --- a/drivers/mmc/core/sdio.c > > +++ b/drivers/mmc/core/sdio.c > > @@ -285,6 +285,27 @@ static int sdio_disable_wide(struct mmc_card *card) > > return 0; > > } > > > > +static int sdio_disable_4bit_bus(struct mmc_card *card) > > +{ > > + int err; > > + > > + if (card->type == MMC_TYPE_SDIO) > > + goto out; > > + > > + if (!(card->host->caps & MMC_CAP_4_BIT_DATA)) > > + return 0; > > + > > + if (!(card->scr.bus_widths & SD_SCR_BUS_WIDTH_4)) > > + return 0; > > + > > + err = mmc_app_set_bus_width(card, MMC_BUS_WIDTH_1); > > + if (err) > > + return err; > > + > > +out: > > + return sdio_disable_wide(card); > > +} > > + > > > > static int sdio_enable_4bit_bus(struct mmc_card *card) > > { > > @@ -960,7 +981,7 @@ static int mmc_sdio_suspend(struct mmc_host *host) > > mmc_claim_host(host); > > > > if (mmc_card_keep_power(host) && mmc_card_wake_sdio_irq(host)) > > - sdio_disable_wide(host->card); > > + sdio_disable_4bit_bus(host->card); > > > > if (!mmc_card_keep_power(host)) { > > mmc_power_off(host); > > -- > > 1.9.1 > > > > Kind regards > Uffe
On Tue, 9 Jun 2020 at 12:40, Yue Hu <zbestahu@gmail.com> wrote: > > On Tue, 9 Jun 2020 12:01:42 +0200 > Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > On Tue, 9 Jun 2020 at 10:14, Yue Hu <zbestahu@gmail.com> wrote: > > > > > > From: Yue Hu <huyue2@yulong.com> > > > > > > Commit 6b5eda369ac3 ("sdio: put active devices into 1-bit mode during > > > suspend") disabled 4-bit mode during system suspend. After this patch, > > > commit 7310ece86ad7 ("mmc: implement SD-combo (IO+mem) support") used > > > new sdio_enable_4bit_bus() instead of sdio_enable_wide() to support > > > SD-combo cards, also for card resume. However, no corresponding support > > > added during suspend. That is not correct. Let's fix it. > > > > I believe the change makes sense to me. > > > > However, the commit 6b5eda369ac3 that you refer to is from v2.6.34, > > which is more than ten years ago. That makes me wonder, are these > > cards really being used? > > Current code logic will switch to 1-bit/4-bit mode in suspend/resume. > > > > > Did you test this with a combo card? > > No, i have no real environment to test it. I'm just reading the code. > Obviously, sdio_disable_wide() used in suspend is for SDIO cards only. > However, sdio_enable_4bit_mode() used in resume is for SD-combo cards. > We should also add the support in suspend to avoid the potential issue. > > Thank you. Alright, let's give this a try and hopefully there is no regression reported. Applied for next, thanks! Kind regards Uffe > > > > > > > > > Signed-off-by: Yue Hu <huyue2@yulong.com> > > > --- > > > drivers/mmc/core/sdio.c | 23 ++++++++++++++++++++++- > > > 1 file changed, 22 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c > > > index ebb387a..2d2ae35 100644 > > > --- a/drivers/mmc/core/sdio.c > > > +++ b/drivers/mmc/core/sdio.c > > > @@ -285,6 +285,27 @@ static int sdio_disable_wide(struct mmc_card *card) > > > return 0; > > > } > > > > > > +static int sdio_disable_4bit_bus(struct mmc_card *card) > > > +{ > > > + int err; > > > + > > > + if (card->type == MMC_TYPE_SDIO) > > > + goto out; > > > + > > > + if (!(card->host->caps & MMC_CAP_4_BIT_DATA)) > > > + return 0; > > > + > > > + if (!(card->scr.bus_widths & SD_SCR_BUS_WIDTH_4)) > > > + return 0; > > > + > > > + err = mmc_app_set_bus_width(card, MMC_BUS_WIDTH_1); > > > + if (err) > > > + return err; > > > + > > > +out: > > > + return sdio_disable_wide(card); > > > +} > > > + > > > > > > static int sdio_enable_4bit_bus(struct mmc_card *card) > > > { > > > @@ -960,7 +981,7 @@ static int mmc_sdio_suspend(struct mmc_host *host) > > > mmc_claim_host(host); > > > > > > if (mmc_card_keep_power(host) && mmc_card_wake_sdio_irq(host)) > > > - sdio_disable_wide(host->card); > > > + sdio_disable_4bit_bus(host->card); > > > > > > if (!mmc_card_keep_power(host)) { > > > mmc_power_off(host); > > > -- > > > 1.9.1 > > > > > > > Kind regards > > Uffe >
diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c index ebb387a..2d2ae35 100644 --- a/drivers/mmc/core/sdio.c +++ b/drivers/mmc/core/sdio.c @@ -285,6 +285,27 @@ static int sdio_disable_wide(struct mmc_card *card) return 0; } +static int sdio_disable_4bit_bus(struct mmc_card *card) +{ + int err; + + if (card->type == MMC_TYPE_SDIO) + goto out; + + if (!(card->host->caps & MMC_CAP_4_BIT_DATA)) + return 0; + + if (!(card->scr.bus_widths & SD_SCR_BUS_WIDTH_4)) + return 0; + + err = mmc_app_set_bus_width(card, MMC_BUS_WIDTH_1); + if (err) + return err; + +out: + return sdio_disable_wide(card); +} + static int sdio_enable_4bit_bus(struct mmc_card *card) { @@ -960,7 +981,7 @@ static int mmc_sdio_suspend(struct mmc_host *host) mmc_claim_host(host); if (mmc_card_keep_power(host) && mmc_card_wake_sdio_irq(host)) - sdio_disable_wide(host->card); + sdio_disable_4bit_bus(host->card); if (!mmc_card_keep_power(host)) { mmc_power_off(host);