| Message ID | 1592813959-5914-1-git-send-email-haibo.chen@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mmc: sdio: fix clock rate setting for SDR12/SDR25 mode | expand |
On Monday 22 June 2020 16:19:19 haibo.chen@nxp.com wrote: > From: Haibo Chen <haibo.chen@nxp.com> > > In current code logic, when work in SDR12/SDR25 mode, the final clock > rate is incorrect, just the legancy 400KHz, because the > card->sw_caps.sd3_bus_mode do not has the flag SD_MODE_UHS_SDR12 or > SD_MODE_UHS_SDR25. Besides, SDIO_SPEED_SDR12 is actually value 0, and > every mode need to config the timing and clock rate, so remove the > ‘if’ operator. > > Signed-off-by: Haibo Chen <haibo.chen@nxp.com> Hello! I do not know what should be the correct behavior according to sdio standards, but I tested this patch with DDR50 card and behavior of that card was not changed, it is working as before. Tested-by: Pali Rohár <pali@kernel.org> > --- > drivers/mmc/core/sdio.c | 15 ++++++++------- > 1 file changed, 8 insertions(+), 7 deletions(-) > > diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c > index 0e32ca7b9488..7b40553d3934 100644 > --- a/drivers/mmc/core/sdio.c > +++ b/drivers/mmc/core/sdio.c > @@ -176,15 +176,18 @@ static int sdio_read_cccr(struct mmc_card *card, u32 ocr) > if (mmc_host_uhs(card->host)) { > if (data & SDIO_UHS_DDR50) > card->sw_caps.sd3_bus_mode > - |= SD_MODE_UHS_DDR50; > + |= SD_MODE_UHS_DDR50 | SD_MODE_UHS_SDR50 > + | SD_MODE_UHS_SDR25 | SD_MODE_UHS_SDR12; > > if (data & SDIO_UHS_SDR50) > card->sw_caps.sd3_bus_mode > - |= SD_MODE_UHS_SDR50; > + |= SD_MODE_UHS_SDR50 | SD_MODE_UHS_SDR25 > + | SD_MODE_UHS_SDR12; > > if (data & SDIO_UHS_SDR104) > card->sw_caps.sd3_bus_mode > - |= SD_MODE_UHS_SDR104; > + |= SD_MODE_UHS_SDR104 | SD_MODE_UHS_SDR50 > + | SD_MODE_UHS_SDR25 | SD_MODE_UHS_SDR12; > } > > ret = mmc_io_rw_direct(card, 0, 0, > @@ -537,10 +540,8 @@ static int sdio_set_bus_speed_mode(struct mmc_card *card) > max_rate = min_not_zero(card->quirk_max_rate, > card->sw_caps.uhs_max_dtr); > > - if (bus_speed) { > - mmc_set_timing(card->host, timing); > - mmc_set_clock(card->host, max_rate); > - } > + mmc_set_timing(card->host, timing); > + mmc_set_clock(card->host, max_rate); > > return 0; > } > -- > 2.17.1 >
On Mon, 22 Jun 2020 at 10:30, <haibo.chen@nxp.com> wrote: > > From: Haibo Chen <haibo.chen@nxp.com> > > In current code logic, when work in SDR12/SDR25 mode, the final clock > rate is incorrect, just the legancy 400KHz, because the > card->sw_caps.sd3_bus_mode do not has the flag SD_MODE_UHS_SDR12 or > SD_MODE_UHS_SDR25. Besides, SDIO_SPEED_SDR12 is actually value 0, and > every mode need to config the timing and clock rate, so remove the > ‘if’ operator. > > Signed-off-by: Haibo Chen <haibo.chen@nxp.com> This looks like a rather serious error, should we tag this for stable? In the meantime, I have applied this for next to get it tested, thanks! Kind regards Uffe > --- > drivers/mmc/core/sdio.c | 15 ++++++++------- > 1 file changed, 8 insertions(+), 7 deletions(-) > > diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c > index 0e32ca7b9488..7b40553d3934 100644 > --- a/drivers/mmc/core/sdio.c > +++ b/drivers/mmc/core/sdio.c > @@ -176,15 +176,18 @@ static int sdio_read_cccr(struct mmc_card *card, u32 ocr) > if (mmc_host_uhs(card->host)) { > if (data & SDIO_UHS_DDR50) > card->sw_caps.sd3_bus_mode > - |= SD_MODE_UHS_DDR50; > + |= SD_MODE_UHS_DDR50 | SD_MODE_UHS_SDR50 > + | SD_MODE_UHS_SDR25 | SD_MODE_UHS_SDR12; > > if (data & SDIO_UHS_SDR50) > card->sw_caps.sd3_bus_mode > - |= SD_MODE_UHS_SDR50; > + |= SD_MODE_UHS_SDR50 | SD_MODE_UHS_SDR25 > + | SD_MODE_UHS_SDR12; > > if (data & SDIO_UHS_SDR104) > card->sw_caps.sd3_bus_mode > - |= SD_MODE_UHS_SDR104; > + |= SD_MODE_UHS_SDR104 | SD_MODE_UHS_SDR50 > + | SD_MODE_UHS_SDR25 | SD_MODE_UHS_SDR12; > } > > ret = mmc_io_rw_direct(card, 0, 0, > @@ -537,10 +540,8 @@ static int sdio_set_bus_speed_mode(struct mmc_card *card) > max_rate = min_not_zero(card->quirk_max_rate, > card->sw_caps.uhs_max_dtr); > > - if (bus_speed) { > - mmc_set_timing(card->host, timing); > - mmc_set_clock(card->host, max_rate); > - } > + mmc_set_timing(card->host, timing); > + mmc_set_clock(card->host, max_rate); > > return 0; > } > -- > 2.17.1 >
> -----Original Message----- > From: Ulf Hansson [mailto:ulf.hansson@linaro.org] > Sent: 2020年7月6日 22:49 > To: BOUGH CHEN <haibo.chen@nxp.com> > Cc: Adrian Hunter <adrian.hunter@intel.com>; linux-mmc@vger.kernel.org; > Pali Rohár <pali@kernel.org>; dl-linux-imx <linux-imx@nxp.com>; Andy Duan > <fugang.duan@nxp.com>; Doug Anderson <dianders@chromium.org>; > huyue2@yulong.com; Matthias Kaehlcke <mka@chromium.org> > Subject: Re: [PATCH] mmc: sdio: fix clock rate setting for SDR12/SDR25 mode > > On Mon, 22 Jun 2020 at 10:30, <haibo.chen@nxp.com> wrote: > > > > From: Haibo Chen <haibo.chen@nxp.com> > > > > In current code logic, when work in SDR12/SDR25 mode, the final clock > > rate is incorrect, just the legancy 400KHz, because the > > card->sw_caps.sd3_bus_mode do not has the flag SD_MODE_UHS_SDR12 or > > SD_MODE_UHS_SDR25. Besides, SDIO_SPEED_SDR12 is actually value 0, and > > every mode need to config the timing and clock rate, so remove the > > ‘if’ operator. > > > > Signed-off-by: Haibo Chen <haibo.chen@nxp.com> > > This looks like a rather serious error, should we tag this for stable? Yes, need to do that. Cc: <stable@vger.kernel.org> Best Regards Haibo Chen > > In the meantime, I have applied this for next to get it tested, thanks! > > Kind regards > Uffe > > > > > --- > > drivers/mmc/core/sdio.c | 15 ++++++++------- > > 1 file changed, 8 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c index > > 0e32ca7b9488..7b40553d3934 100644 > > --- a/drivers/mmc/core/sdio.c > > +++ b/drivers/mmc/core/sdio.c > > @@ -176,15 +176,18 @@ static int sdio_read_cccr(struct mmc_card *card, > u32 ocr) > > if (mmc_host_uhs(card->host)) { > > if (data & SDIO_UHS_DDR50) > > > card->sw_caps.sd3_bus_mode > > - |= > SD_MODE_UHS_DDR50; > > + |= > SD_MODE_UHS_DDR50 | SD_MODE_UHS_SDR50 > > + | > > + SD_MODE_UHS_SDR25 | SD_MODE_UHS_SDR12; > > > > if (data & SDIO_UHS_SDR50) > > > card->sw_caps.sd3_bus_mode > > - |= > SD_MODE_UHS_SDR50; > > + |= > SD_MODE_UHS_SDR50 | SD_MODE_UHS_SDR25 > > + | > > + SD_MODE_UHS_SDR12; > > > > if (data & SDIO_UHS_SDR104) > > > card->sw_caps.sd3_bus_mode > > - |= > SD_MODE_UHS_SDR104; > > + |= > SD_MODE_UHS_SDR104 | SD_MODE_UHS_SDR50 > > + | > > + SD_MODE_UHS_SDR25 | SD_MODE_UHS_SDR12; > > } > > > > ret = mmc_io_rw_direct(card, 0, 0, @@ > -537,10 > > +540,8 @@ static int sdio_set_bus_speed_mode(struct mmc_card *card) > > max_rate = min_not_zero(card->quirk_max_rate, > > card->sw_caps.uhs_max_dtr); > > > > - if (bus_speed) { > > - mmc_set_timing(card->host, timing); > > - mmc_set_clock(card->host, max_rate); > > - } > > + mmc_set_timing(card->host, timing); > > + mmc_set_clock(card->host, max_rate); > > > > return 0; > > } > > -- > > 2.17.1 > >
On Tuesday 07 July 2020 01:48:18 BOUGH CHEN wrote: > > -----Original Message----- > > From: Ulf Hansson [mailto:ulf.hansson@linaro.org] > > Sent: 2020年7月6日 22:49 > > To: BOUGH CHEN <haibo.chen@nxp.com> > > Cc: Adrian Hunter <adrian.hunter@intel.com>; linux-mmc@vger.kernel.org; > > Pali Rohár <pali@kernel.org>; dl-linux-imx <linux-imx@nxp.com>; Andy Duan > > <fugang.duan@nxp.com>; Doug Anderson <dianders@chromium.org>; > > huyue2@yulong.com; Matthias Kaehlcke <mka@chromium.org> > > Subject: Re: [PATCH] mmc: sdio: fix clock rate setting for SDR12/SDR25 mode > > > > On Mon, 22 Jun 2020 at 10:30, <haibo.chen@nxp.com> wrote: > > > > > > From: Haibo Chen <haibo.chen@nxp.com> > > > > > > In current code logic, when work in SDR12/SDR25 mode, the final clock > > > rate is incorrect, just the legancy 400KHz, because the > > > card->sw_caps.sd3_bus_mode do not has the flag SD_MODE_UHS_SDR12 or > > > SD_MODE_UHS_SDR25. Besides, SDIO_SPEED_SDR12 is actually value 0, and > > > every mode need to config the timing and clock rate, so remove the > > > ‘if’ operator. > > > > > > Signed-off-by: Haibo Chen <haibo.chen@nxp.com> > > > > This looks like a rather serious error, should we tag this for stable? > > Yes, need to do that. > > Cc: <stable@vger.kernel.org> Hello! I think you can add Fixes line, e.g.: Fixes: a303c5319c8e ("mmc: sdio: support SDIO UHS cards") > Best Regards > Haibo Chen > > > > In the meantime, I have applied this for next to get it tested, thanks! > > > > Kind regards > > Uffe > > > > > > > > > --- > > > drivers/mmc/core/sdio.c | 15 ++++++++------- > > > 1 file changed, 8 insertions(+), 7 deletions(-) > > > > > > diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c index > > > 0e32ca7b9488..7b40553d3934 100644 > > > --- a/drivers/mmc/core/sdio.c > > > +++ b/drivers/mmc/core/sdio.c > > > @@ -176,15 +176,18 @@ static int sdio_read_cccr(struct mmc_card *card, > > u32 ocr) > > > if (mmc_host_uhs(card->host)) { > > > if (data & SDIO_UHS_DDR50) > > > > > card->sw_caps.sd3_bus_mode > > > - |= > > SD_MODE_UHS_DDR50; > > > + |= > > SD_MODE_UHS_DDR50 | SD_MODE_UHS_SDR50 > > > + | > > > + SD_MODE_UHS_SDR25 | SD_MODE_UHS_SDR12; > > > > > > if (data & SDIO_UHS_SDR50) > > > > > card->sw_caps.sd3_bus_mode > > > - |= > > SD_MODE_UHS_SDR50; > > > + |= > > SD_MODE_UHS_SDR50 | SD_MODE_UHS_SDR25 > > > + | > > > + SD_MODE_UHS_SDR12; > > > > > > if (data & SDIO_UHS_SDR104) > > > > > card->sw_caps.sd3_bus_mode > > > - |= > > SD_MODE_UHS_SDR104; > > > + |= > > SD_MODE_UHS_SDR104 | SD_MODE_UHS_SDR50 > > > + | > > > + SD_MODE_UHS_SDR25 | SD_MODE_UHS_SDR12; > > > } > > > > > > ret = mmc_io_rw_direct(card, 0, 0, @@ > > -537,10 > > > +540,8 @@ static int sdio_set_bus_speed_mode(struct mmc_card *card) > > > max_rate = min_not_zero(card->quirk_max_rate, > > > card->sw_caps.uhs_max_dtr); > > > > > > - if (bus_speed) { > > > - mmc_set_timing(card->host, timing); > > > - mmc_set_clock(card->host, max_rate); > > > - } > > > + mmc_set_timing(card->host, timing); > > > + mmc_set_clock(card->host, max_rate); > > > > > > return 0; > > > } > > > -- > > > 2.17.1 > > >
diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c index 0e32ca7b9488..7b40553d3934 100644 --- a/drivers/mmc/core/sdio.c +++ b/drivers/mmc/core/sdio.c @@ -176,15 +176,18 @@ static int sdio_read_cccr(struct mmc_card *card, u32 ocr) if (mmc_host_uhs(card->host)) { if (data & SDIO_UHS_DDR50) card->sw_caps.sd3_bus_mode - |= SD_MODE_UHS_DDR50; + |= SD_MODE_UHS_DDR50 | SD_MODE_UHS_SDR50 + | SD_MODE_UHS_SDR25 | SD_MODE_UHS_SDR12; if (data & SDIO_UHS_SDR50) card->sw_caps.sd3_bus_mode - |= SD_MODE_UHS_SDR50; + |= SD_MODE_UHS_SDR50 | SD_MODE_UHS_SDR25 + | SD_MODE_UHS_SDR12; if (data & SDIO_UHS_SDR104) card->sw_caps.sd3_bus_mode - |= SD_MODE_UHS_SDR104; + |= SD_MODE_UHS_SDR104 | SD_MODE_UHS_SDR50 + | SD_MODE_UHS_SDR25 | SD_MODE_UHS_SDR12; } ret = mmc_io_rw_direct(card, 0, 0, @@ -537,10 +540,8 @@ static int sdio_set_bus_speed_mode(struct mmc_card *card) max_rate = min_not_zero(card->quirk_max_rate, card->sw_caps.uhs_max_dtr); - if (bus_speed) { - mmc_set_timing(card->host, timing); - mmc_set_clock(card->host, max_rate); - } + mmc_set_timing(card->host, timing); + mmc_set_clock(card->host, max_rate); return 0; }