* [PATCH] mmc: mmc: Fix the timing for clock changing in mmc @ 2020-02-12 2:42 Kyungmin Seo 2020-03-04 11:08 ` Ulf Hansson 0 siblings, 1 reply; 13+ messages in thread From: Kyungmin Seo @ 2020-02-12 2:42 UTC (permalink / raw) To: ulf.hansson, kyungmin.seo; +Cc: linux-mmc, linux-kernel The clock has to be changed after sending CMD6 for HS mode selection in mmc_hs400_to_hs200() function. The JEDEC 5.0 and 5.1 said that "High-speed" mode selection has to enable the the high speed mode timing in the Device, before chaning the clock frequency to a frequency between 26MHz and 52MHz. Signed-off-by: Kyungmin Seo <kyungmin.seo@intel.com> --- drivers/mmc/core/mmc.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c index 3486bc7fbb64..98640b51c73e 100644 --- a/drivers/mmc/core/mmc.c +++ b/drivers/mmc/core/mmc.c @@ -1196,10 +1196,6 @@ int mmc_hs400_to_hs200(struct mmc_card *card) int err; u8 val; - /* Reduce frequency to HS */ - max_dtr = card->ext_csd.hs_max_dtr; - mmc_set_clock(host, max_dtr); - /* Switch HS400 to HS DDR */ val = EXT_CSD_TIMING_HS; err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_HS_TIMING, @@ -1210,6 +1206,10 @@ int mmc_hs400_to_hs200(struct mmc_card *card) mmc_set_timing(host, MMC_TIMING_MMC_DDR52); + /* Reduce frequency to HS */ + max_dtr = card->ext_csd.hs_max_dtr; + mmc_set_clock(host, max_dtr); + err = mmc_switch_status(card); if (err) goto out_err; -- 2.17.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] mmc: mmc: Fix the timing for clock changing in mmc 2020-02-12 2:42 [PATCH] mmc: mmc: Fix the timing for clock changing in mmc Kyungmin Seo @ 2020-03-04 11:08 ` Ulf Hansson 2020-03-05 8:57 ` Seo, Kyungmin 0 siblings, 1 reply; 13+ messages in thread From: Ulf Hansson @ 2020-03-04 11:08 UTC (permalink / raw) To: Kyungmin Seo; +Cc: linux-mmc, Linux Kernel Mailing List On Wed, 12 Feb 2020 at 03:40, Kyungmin Seo <kyungmin.seo@intel.com> wrote: > > The clock has to be changed after sending CMD6 for HS mode selection in > mmc_hs400_to_hs200() function. > > The JEDEC 5.0 and 5.1 said that "High-speed" mode selection has to > enable the the high speed mode timing in the Device, before chaning the > clock frequency to a frequency between 26MHz and 52MHz. I think that is based upon the assumption that you are using a lower frequency to start with. For example, assume that you are running with 400KHz during card initialization, then you want to send the CMD6 to switch to HS mode and that should be done, before updating the clock rate. mmc_hs400_to_hs200() goes the opposite direction, so I think the current code looks correct to me. Kind regards Uffe > > Signed-off-by: Kyungmin Seo <kyungmin.seo@intel.com> > --- > drivers/mmc/core/mmc.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c > index 3486bc7fbb64..98640b51c73e 100644 > --- a/drivers/mmc/core/mmc.c > +++ b/drivers/mmc/core/mmc.c > @@ -1196,10 +1196,6 @@ int mmc_hs400_to_hs200(struct mmc_card *card) > int err; > u8 val; > > - /* Reduce frequency to HS */ > - max_dtr = card->ext_csd.hs_max_dtr; > - mmc_set_clock(host, max_dtr); > - > /* Switch HS400 to HS DDR */ > val = EXT_CSD_TIMING_HS; > err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_HS_TIMING, > @@ -1210,6 +1206,10 @@ int mmc_hs400_to_hs200(struct mmc_card *card) > > mmc_set_timing(host, MMC_TIMING_MMC_DDR52); > > + /* Reduce frequency to HS */ > + max_dtr = card->ext_csd.hs_max_dtr; > + mmc_set_clock(host, max_dtr); > + > err = mmc_switch_status(card); > if (err) > goto out_err; > -- > 2.17.1 > ^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH] mmc: mmc: Fix the timing for clock changing in mmc 2020-03-04 11:08 ` Ulf Hansson @ 2020-03-05 8:57 ` Seo, Kyungmin 2020-03-05 15:17 ` Ulf Hansson 0 siblings, 1 reply; 13+ messages in thread From: Seo, Kyungmin @ 2020-03-05 8:57 UTC (permalink / raw) To: Ulf Hansson; +Cc: linux-mmc, Linux Kernel Mailing List The mmc_hs400_to_hs200 function is called only in HS400 mode. I saw the clock change from 200MHz to 52MHz via oscilloscope on real platform. I think CMD6 is sent in HS400 mode with 200MHz clock, but it's not. First CMD6 in mmc_hs400_to_hs200 function is sent with 52MHz clock. Thanks KM -----Original Message----- From: Ulf Hansson <ulf.hansson@linaro.org> Sent: Wednesday, March 4, 2020 8:09 PM To: Seo, Kyungmin <kyungmin.seo@intel.com> Cc: linux-mmc@vger.kernel.org; Linux Kernel Mailing List <linux-kernel@vger.kernel.org> Subject: Re: [PATCH] mmc: mmc: Fix the timing for clock changing in mmc On Wed, 12 Feb 2020 at 03:40, Kyungmin Seo <kyungmin.seo@intel.com> wrote: > > The clock has to be changed after sending CMD6 for HS mode selection > in > mmc_hs400_to_hs200() function. > > The JEDEC 5.0 and 5.1 said that "High-speed" mode selection has to > enable the the high speed mode timing in the Device, before chaning > the clock frequency to a frequency between 26MHz and 52MHz. I think that is based upon the assumption that you are using a lower frequency to start with. For example, assume that you are running with 400KHz during card initialization, then you want to send the CMD6 to switch to HS mode and that should be done, before updating the clock rate. mmc_hs400_to_hs200() goes the opposite direction, so I think the current code looks correct to me. Kind regards Uffe > > Signed-off-by: Kyungmin Seo <kyungmin.seo@intel.com> > --- > drivers/mmc/core/mmc.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c index > 3486bc7fbb64..98640b51c73e 100644 > --- a/drivers/mmc/core/mmc.c > +++ b/drivers/mmc/core/mmc.c > @@ -1196,10 +1196,6 @@ int mmc_hs400_to_hs200(struct mmc_card *card) > int err; > u8 val; > > - /* Reduce frequency to HS */ > - max_dtr = card->ext_csd.hs_max_dtr; > - mmc_set_clock(host, max_dtr); > - > /* Switch HS400 to HS DDR */ > val = EXT_CSD_TIMING_HS; > err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, > EXT_CSD_HS_TIMING, @@ -1210,6 +1206,10 @@ int > mmc_hs400_to_hs200(struct mmc_card *card) > > mmc_set_timing(host, MMC_TIMING_MMC_DDR52); > > + /* Reduce frequency to HS */ > + max_dtr = card->ext_csd.hs_max_dtr; > + mmc_set_clock(host, max_dtr); > + > err = mmc_switch_status(card); > if (err) > goto out_err; > -- > 2.17.1 > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] mmc: mmc: Fix the timing for clock changing in mmc 2020-03-05 8:57 ` Seo, Kyungmin @ 2020-03-05 15:17 ` Ulf Hansson 2020-03-10 4:27 ` Seo, Kyungmin 0 siblings, 1 reply; 13+ messages in thread From: Ulf Hansson @ 2020-03-05 15:17 UTC (permalink / raw) To: Seo, Kyungmin, Adrian Hunter, Chaotian Jing Cc: linux-mmc, Linux Kernel Mailing List + Adrian, Chaotian On Thu, 5 Mar 2020 at 09:57, Seo, Kyungmin <kyungmin.seo@intel.com> wrote: > > The mmc_hs400_to_hs200 function is called only in HS400 mode. > I saw the clock change from 200MHz to 52MHz via oscilloscope on real platform. > > I think CMD6 is sent in HS400 mode with 200MHz clock, but it's not. > First CMD6 in mmc_hs400_to_hs200 function is sent with 52MHz clock. I had a vague memory that we have discussed a similar problem as your are pointing out on the mailing list already. And I was right. Please read up on the below references, [1], [2] for the earlier discussions. I suggested a solution for Chaotian to try, but it seems like he never managed to give it a go, as I don't recall new patch being posted. Perhaps you can pick up were Chaotian left and see if you can implement the suggested solution(s). My main concern is breaking other host drivers, as that seems quite likely to happen, if we aren't careful about this. Kind regards Uffe [1] https://lore.kernel.org/linux-mmc/1548921212-5219-1-git-send-email-chaotian.jing@mediatek.com/ [2] https://lore.kernel.org/linux-mmc/CAPDyKFquyyXx1MqNLVXuFxcEDB9nKzN8LGGNUP2yxoVMQrWiUg@mail.gmail.com/ > > Thanks > KM > > -----Original Message----- > From: Ulf Hansson <ulf.hansson@linaro.org> > Sent: Wednesday, March 4, 2020 8:09 PM > To: Seo, Kyungmin <kyungmin.seo@intel.com> > Cc: linux-mmc@vger.kernel.org; Linux Kernel Mailing List <linux-kernel@vger.kernel.org> > Subject: Re: [PATCH] mmc: mmc: Fix the timing for clock changing in mmc > > On Wed, 12 Feb 2020 at 03:40, Kyungmin Seo <kyungmin.seo@intel.com> wrote: > > > > The clock has to be changed after sending CMD6 for HS mode selection > > in > > mmc_hs400_to_hs200() function. > > > > The JEDEC 5.0 and 5.1 said that "High-speed" mode selection has to > > enable the the high speed mode timing in the Device, before chaning > > the clock frequency to a frequency between 26MHz and 52MHz. > > I think that is based upon the assumption that you are using a lower frequency to start with. > > For example, assume that you are running with 400KHz during card initialization, then you want to send the CMD6 to switch to HS mode and that should be done, before updating the clock rate. > > mmc_hs400_to_hs200() goes the opposite direction, so I think the current code looks correct to me. > > Kind regards > Uffe > > > > > Signed-off-by: Kyungmin Seo <kyungmin.seo@intel.com> > > --- > > drivers/mmc/core/mmc.c | 8 ++++---- > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c index > > 3486bc7fbb64..98640b51c73e 100644 > > --- a/drivers/mmc/core/mmc.c > > +++ b/drivers/mmc/core/mmc.c > > @@ -1196,10 +1196,6 @@ int mmc_hs400_to_hs200(struct mmc_card *card) > > int err; > > u8 val; > > > > - /* Reduce frequency to HS */ > > - max_dtr = card->ext_csd.hs_max_dtr; > > - mmc_set_clock(host, max_dtr); > > - > > /* Switch HS400 to HS DDR */ > > val = EXT_CSD_TIMING_HS; > > err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, > > EXT_CSD_HS_TIMING, @@ -1210,6 +1206,10 @@ int > > mmc_hs400_to_hs200(struct mmc_card *card) > > > > mmc_set_timing(host, MMC_TIMING_MMC_DDR52); > > > > + /* Reduce frequency to HS */ > > + max_dtr = card->ext_csd.hs_max_dtr; > > + mmc_set_clock(host, max_dtr); > > + > > err = mmc_switch_status(card); > > if (err) > > goto out_err; > > -- > > 2.17.1 > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH] mmc: mmc: Fix the timing for clock changing in mmc 2020-03-05 15:17 ` Ulf Hansson @ 2020-03-10 4:27 ` Seo, Kyungmin 2020-03-10 9:05 ` Ulf Hansson 0 siblings, 1 reply; 13+ messages in thread From: Seo, Kyungmin @ 2020-03-10 4:27 UTC (permalink / raw) To: Ulf Hansson, Hunter, Adrian, Chaotian Jing Cc: linux-mmc, Linux Kernel Mailing List I read the link and patch of Chaotian Jing. I also point out what Chaotian said. Most host controllers have DLL tuning values for each mode. When host controller is set as HS400 mode with 50MHz clock, host controller uses DLL value which is tuned with 200MHz clock. If DLL value in HS400 mode doesn't have the pass range in HS mode, command transfer failing may fail. In order to make robust sdhci driver, I think the patch needs to be considered. Of course, CMD6 with HS400 mode and 200MHz clock should not cause any problem because it's correct configuration. Thanks -----Original Message----- From: Ulf Hansson <ulf.hansson@linaro.org> Sent: Friday, March 6, 2020 12:18 AM To: Seo, Kyungmin <kyungmin.seo@intel.com>; Hunter, Adrian <adrian.hunter@intel.com>; Chaotian Jing <chaotian.jing@mediatek.com> Cc: linux-mmc@vger.kernel.org; Linux Kernel Mailing List <linux-kernel@vger.kernel.org> Subject: Re: [PATCH] mmc: mmc: Fix the timing for clock changing in mmc + Adrian, Chaotian On Thu, 5 Mar 2020 at 09:57, Seo, Kyungmin <kyungmin.seo@intel.com> wrote: > > The mmc_hs400_to_hs200 function is called only in HS400 mode. > I saw the clock change from 200MHz to 52MHz via oscilloscope on real platform. > > I think CMD6 is sent in HS400 mode with 200MHz clock, but it's not. > First CMD6 in mmc_hs400_to_hs200 function is sent with 52MHz clock. I had a vague memory that we have discussed a similar problem as your are pointing out on the mailing list already. And I was right. Please read up on the below references, [1], [2] for the earlier discussions. I suggested a solution for Chaotian to try, but it seems like he never managed to give it a go, as I don't recall new patch being posted. Perhaps you can pick up were Chaotian left and see if you can implement the suggested solution(s). My main concern is breaking other host drivers, as that seems quite likely to happen, if we aren't careful about this. Kind regards Uffe [1] https://lore.kernel.org/linux-mmc/1548921212-5219-1-git-send-email-chaotian.jing@mediatek.com/ [2] https://lore.kernel.org/linux-mmc/CAPDyKFquyyXx1MqNLVXuFxcEDB9nKzN8LGGNUP2yxoVMQrWiUg@mail.gmail.com/ > > Thanks > KM > > -----Original Message----- > From: Ulf Hansson <ulf.hansson@linaro.org> > Sent: Wednesday, March 4, 2020 8:09 PM > To: Seo, Kyungmin <kyungmin.seo@intel.com> > Cc: linux-mmc@vger.kernel.org; Linux Kernel Mailing List > <linux-kernel@vger.kernel.org> > Subject: Re: [PATCH] mmc: mmc: Fix the timing for clock changing in > mmc > > On Wed, 12 Feb 2020 at 03:40, Kyungmin Seo <kyungmin.seo@intel.com> wrote: > > > > The clock has to be changed after sending CMD6 for HS mode selection > > in > > mmc_hs400_to_hs200() function. > > > > The JEDEC 5.0 and 5.1 said that "High-speed" mode selection has to > > enable the the high speed mode timing in the Device, before chaning > > the clock frequency to a frequency between 26MHz and 52MHz. > > I think that is based upon the assumption that you are using a lower frequency to start with. > > For example, assume that you are running with 400KHz during card initialization, then you want to send the CMD6 to switch to HS mode and that should be done, before updating the clock rate. > > mmc_hs400_to_hs200() goes the opposite direction, so I think the current code looks correct to me. > > Kind regards > Uffe > > > > > Signed-off-by: Kyungmin Seo <kyungmin.seo@intel.com> > > --- > > drivers/mmc/core/mmc.c | 8 ++++---- > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c index > > 3486bc7fbb64..98640b51c73e 100644 > > --- a/drivers/mmc/core/mmc.c > > +++ b/drivers/mmc/core/mmc.c > > @@ -1196,10 +1196,6 @@ int mmc_hs400_to_hs200(struct mmc_card *card) > > int err; > > u8 val; > > > > - /* Reduce frequency to HS */ > > - max_dtr = card->ext_csd.hs_max_dtr; > > - mmc_set_clock(host, max_dtr); > > - > > /* Switch HS400 to HS DDR */ > > val = EXT_CSD_TIMING_HS; > > err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, > > EXT_CSD_HS_TIMING, @@ -1210,6 +1206,10 @@ int > > mmc_hs400_to_hs200(struct mmc_card *card) > > > > mmc_set_timing(host, MMC_TIMING_MMC_DDR52); > > > > + /* Reduce frequency to HS */ > > + max_dtr = card->ext_csd.hs_max_dtr; > > + mmc_set_clock(host, max_dtr); > > + > > err = mmc_switch_status(card); > > if (err) > > goto out_err; > > -- > > 2.17.1 > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] mmc: mmc: Fix the timing for clock changing in mmc 2020-03-10 4:27 ` Seo, Kyungmin @ 2020-03-10 9:05 ` Ulf Hansson 2020-03-10 10:44 ` Adrian Hunter 0 siblings, 1 reply; 13+ messages in thread From: Ulf Hansson @ 2020-03-10 9:05 UTC (permalink / raw) To: Seo, Kyungmin Cc: Hunter, Adrian, Chaotian Jing, linux-mmc, Linux Kernel Mailing List On Tue, 10 Mar 2020 at 05:28, Seo, Kyungmin <kyungmin.seo@intel.com> wrote: > > I read the link and patch of Chaotian Jing. > I also point out what Chaotian said. > Most host controllers have DLL tuning values for each mode. When host controller is set as HS400 mode with 50MHz clock, host controller uses DLL value which is tuned with 200MHz clock. > > If DLL value in HS400 mode doesn't have the pass range in HS mode, command transfer failing may fail. > In order to make robust sdhci driver, I think the patch needs to be considered. I have, but I am not picking it up in its current form. > Of course, CMD6 with HS400 mode and 200MHz clock should not cause any problem because it's correct configuration. Yes, but not for all cases, as I said in my reply in those email-threads. What I had in mind, is that I I think we should inform mmc_hs400_to_hs200() about under what situation it's getting called. Depending on that, we should either decrease the clock rate before or after we send the CMD6. Would that work for your case? Kind regards Uffe > > Thanks > > -----Original Message----- > From: Ulf Hansson <ulf.hansson@linaro.org> > Sent: Friday, March 6, 2020 12:18 AM > To: Seo, Kyungmin <kyungmin.seo@intel.com>; Hunter, Adrian <adrian.hunter@intel.com>; Chaotian Jing <chaotian.jing@mediatek.com> > Cc: linux-mmc@vger.kernel.org; Linux Kernel Mailing List <linux-kernel@vger.kernel.org> > Subject: Re: [PATCH] mmc: mmc: Fix the timing for clock changing in mmc > > + Adrian, Chaotian > > On Thu, 5 Mar 2020 at 09:57, Seo, Kyungmin <kyungmin.seo@intel.com> wrote: > > > > The mmc_hs400_to_hs200 function is called only in HS400 mode. > > I saw the clock change from 200MHz to 52MHz via oscilloscope on real platform. > > > > I think CMD6 is sent in HS400 mode with 200MHz clock, but it's not. > > First CMD6 in mmc_hs400_to_hs200 function is sent with 52MHz clock. > > I had a vague memory that we have discussed a similar problem as your are pointing out on the mailing list already. And I was right. > > Please read up on the below references, [1], [2] for the earlier discussions. I suggested a solution for Chaotian to try, but it seems like he never managed to give it a go, as I don't recall new patch being posted. > > Perhaps you can pick up were Chaotian left and see if you can implement the suggested solution(s). My main concern is breaking other host drivers, as that seems quite likely to happen, if we aren't careful about this. > > Kind regards > Uffe > > [1] > https://lore.kernel.org/linux-mmc/1548921212-5219-1-git-send-email-chaotian.jing@mediatek.com/ > [2] > https://lore.kernel.org/linux-mmc/CAPDyKFquyyXx1MqNLVXuFxcEDB9nKzN8LGGNUP2yxoVMQrWiUg@mail.gmail.com/ > > > > > > > Thanks > > KM > > > > -----Original Message----- > > From: Ulf Hansson <ulf.hansson@linaro.org> > > Sent: Wednesday, March 4, 2020 8:09 PM > > To: Seo, Kyungmin <kyungmin.seo@intel.com> > > Cc: linux-mmc@vger.kernel.org; Linux Kernel Mailing List > > <linux-kernel@vger.kernel.org> > > Subject: Re: [PATCH] mmc: mmc: Fix the timing for clock changing in > > mmc > > > > On Wed, 12 Feb 2020 at 03:40, Kyungmin Seo <kyungmin.seo@intel.com> wrote: > > > > > > The clock has to be changed after sending CMD6 for HS mode selection > > > in > > > mmc_hs400_to_hs200() function. > > > > > > The JEDEC 5.0 and 5.1 said that "High-speed" mode selection has to > > > enable the the high speed mode timing in the Device, before chaning > > > the clock frequency to a frequency between 26MHz and 52MHz. > > > > I think that is based upon the assumption that you are using a lower frequency to start with. > > > > For example, assume that you are running with 400KHz during card initialization, then you want to send the CMD6 to switch to HS mode and that should be done, before updating the clock rate. > > > > mmc_hs400_to_hs200() goes the opposite direction, so I think the current code looks correct to me. > > > > Kind regards > > Uffe > > > > > > > > Signed-off-by: Kyungmin Seo <kyungmin.seo@intel.com> > > > --- > > > drivers/mmc/core/mmc.c | 8 ++++---- > > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c index > > > 3486bc7fbb64..98640b51c73e 100644 > > > --- a/drivers/mmc/core/mmc.c > > > +++ b/drivers/mmc/core/mmc.c > > > @@ -1196,10 +1196,6 @@ int mmc_hs400_to_hs200(struct mmc_card *card) > > > int err; > > > u8 val; > > > > > > - /* Reduce frequency to HS */ > > > - max_dtr = card->ext_csd.hs_max_dtr; > > > - mmc_set_clock(host, max_dtr); > > > - > > > /* Switch HS400 to HS DDR */ > > > val = EXT_CSD_TIMING_HS; > > > err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, > > > EXT_CSD_HS_TIMING, @@ -1210,6 +1206,10 @@ int > > > mmc_hs400_to_hs200(struct mmc_card *card) > > > > > > mmc_set_timing(host, MMC_TIMING_MMC_DDR52); > > > > > > + /* Reduce frequency to HS */ > > > + max_dtr = card->ext_csd.hs_max_dtr; > > > + mmc_set_clock(host, max_dtr); > > > + > > > err = mmc_switch_status(card); > > > if (err) > > > goto out_err; > > > -- > > > 2.17.1 > > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] mmc: mmc: Fix the timing for clock changing in mmc 2020-03-10 9:05 ` Ulf Hansson @ 2020-03-10 10:44 ` Adrian Hunter 2020-03-10 15:41 ` Ulf Hansson 0 siblings, 1 reply; 13+ messages in thread From: Adrian Hunter @ 2020-03-10 10:44 UTC (permalink / raw) To: Ulf Hansson, Seo, Kyungmin Cc: Chaotian Jing, linux-mmc, Linux Kernel Mailing List On 10/03/20 11:05 am, Ulf Hansson wrote: > On Tue, 10 Mar 2020 at 05:28, Seo, Kyungmin <kyungmin.seo@intel.com> wrote: >> >> I read the link and patch of Chaotian Jing. >> I also point out what Chaotian said. >> Most host controllers have DLL tuning values for each mode. When host controller is set as HS400 mode with 50MHz clock, host controller uses DLL value which is tuned with 200MHz clock. >> >> If DLL value in HS400 mode doesn't have the pass range in HS mode, command transfer failing may fail. >> In order to make robust sdhci driver, I think the patch needs to be considered. > > I have, but I am not picking it up in its current form. > >> Of course, CMD6 with HS400 mode and 200MHz clock should not cause any problem because it's correct configuration. > > Yes, but not for all cases, as I said in my reply in those email-threads. > > What I had in mind, is that I I think we should inform > mmc_hs400_to_hs200() about under what situation it's getting called. > Depending on that, we should either decrease the clock rate before or > after we send the CMD6. > > Would that work for your case? Ulf, would you consider a new call back e.g. diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c index c2abd417a84a..1bc18fe2632f 100644 --- a/drivers/mmc/core/mmc.c +++ b/drivers/mmc/core/mmc.c @@ -1237,7 +1237,10 @@ int mmc_hs400_to_hs200(struct mmc_card *card) /* Reduce frequency to HS */ max_dtr = card->ext_csd.hs_max_dtr; - mmc_set_clock(host, max_dtr); + if (host->ops->hs400_to_hs200_prep) + host->ops->hs400_to_hs200_prep(host, max_dtr); + else + mmc_set_clock(host, max_dtr); /* Switch HS400 to HS DDR */ val = EXT_CSD_TIMING_HS; > > Kind regards > Uffe > >> >> Thanks >> >> -----Original Message----- >> From: Ulf Hansson <ulf.hansson@linaro.org> >> Sent: Friday, March 6, 2020 12:18 AM >> To: Seo, Kyungmin <kyungmin.seo@intel.com>; Hunter, Adrian <adrian.hunter@intel.com>; Chaotian Jing <chaotian.jing@mediatek.com> >> Cc: linux-mmc@vger.kernel.org; Linux Kernel Mailing List <linux-kernel@vger.kernel.org> >> Subject: Re: [PATCH] mmc: mmc: Fix the timing for clock changing in mmc >> >> + Adrian, Chaotian >> >> On Thu, 5 Mar 2020 at 09:57, Seo, Kyungmin <kyungmin.seo@intel.com> wrote: >>> >>> The mmc_hs400_to_hs200 function is called only in HS400 mode. >>> I saw the clock change from 200MHz to 52MHz via oscilloscope on real platform. >>> >>> I think CMD6 is sent in HS400 mode with 200MHz clock, but it's not. >>> First CMD6 in mmc_hs400_to_hs200 function is sent with 52MHz clock. >> >> I had a vague memory that we have discussed a similar problem as your are pointing out on the mailing list already. And I was right. >> >> Please read up on the below references, [1], [2] for the earlier discussions. I suggested a solution for Chaotian to try, but it seems like he never managed to give it a go, as I don't recall new patch being posted. >> >> Perhaps you can pick up were Chaotian left and see if you can implement the suggested solution(s). My main concern is breaking other host drivers, as that seems quite likely to happen, if we aren't careful about this. >> >> Kind regards >> Uffe >> >> [1] >> https://lore.kernel.org/linux-mmc/1548921212-5219-1-git-send-email-chaotian.jing@mediatek.com/ >> [2] >> https://lore.kernel.org/linux-mmc/CAPDyKFquyyXx1MqNLVXuFxcEDB9nKzN8LGGNUP2yxoVMQrWiUg@mail.gmail.com/ >> >> >> >>> >>> Thanks >>> KM >>> >>> -----Original Message----- >>> From: Ulf Hansson <ulf.hansson@linaro.org> >>> Sent: Wednesday, March 4, 2020 8:09 PM >>> To: Seo, Kyungmin <kyungmin.seo@intel.com> >>> Cc: linux-mmc@vger.kernel.org; Linux Kernel Mailing List >>> <linux-kernel@vger.kernel.org> >>> Subject: Re: [PATCH] mmc: mmc: Fix the timing for clock changing in >>> mmc >>> >>> On Wed, 12 Feb 2020 at 03:40, Kyungmin Seo <kyungmin.seo@intel.com> wrote: >>>> >>>> The clock has to be changed after sending CMD6 for HS mode selection >>>> in >>>> mmc_hs400_to_hs200() function. >>>> >>>> The JEDEC 5.0 and 5.1 said that "High-speed" mode selection has to >>>> enable the the high speed mode timing in the Device, before chaning >>>> the clock frequency to a frequency between 26MHz and 52MHz. >>> >>> I think that is based upon the assumption that you are using a lower frequency to start with. >>> >>> For example, assume that you are running with 400KHz during card initialization, then you want to send the CMD6 to switch to HS mode and that should be done, before updating the clock rate. >>> >>> mmc_hs400_to_hs200() goes the opposite direction, so I think the current code looks correct to me. >>> >>> Kind regards >>> Uffe >>> >>>> >>>> Signed-off-by: Kyungmin Seo <kyungmin.seo@intel.com> >>>> --- >>>> drivers/mmc/core/mmc.c | 8 ++++---- >>>> 1 file changed, 4 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c index >>>> 3486bc7fbb64..98640b51c73e 100644 >>>> --- a/drivers/mmc/core/mmc.c >>>> +++ b/drivers/mmc/core/mmc.c >>>> @@ -1196,10 +1196,6 @@ int mmc_hs400_to_hs200(struct mmc_card *card) >>>> int err; >>>> u8 val; >>>> >>>> - /* Reduce frequency to HS */ >>>> - max_dtr = card->ext_csd.hs_max_dtr; >>>> - mmc_set_clock(host, max_dtr); >>>> - >>>> /* Switch HS400 to HS DDR */ >>>> val = EXT_CSD_TIMING_HS; >>>> err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, >>>> EXT_CSD_HS_TIMING, @@ -1210,6 +1206,10 @@ int >>>> mmc_hs400_to_hs200(struct mmc_card *card) >>>> >>>> mmc_set_timing(host, MMC_TIMING_MMC_DDR52); >>>> >>>> + /* Reduce frequency to HS */ >>>> + max_dtr = card->ext_csd.hs_max_dtr; >>>> + mmc_set_clock(host, max_dtr); >>>> + >>>> err = mmc_switch_status(card); >>>> if (err) >>>> goto out_err; >>>> -- >>>> 2.17.1 >>>> ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] mmc: mmc: Fix the timing for clock changing in mmc 2020-03-10 10:44 ` Adrian Hunter @ 2020-03-10 15:41 ` Ulf Hansson 2020-03-11 2:13 ` Chaotian Jing 0 siblings, 1 reply; 13+ messages in thread From: Ulf Hansson @ 2020-03-10 15:41 UTC (permalink / raw) To: Adrian Hunter Cc: Seo, Kyungmin, Chaotian Jing, linux-mmc, Linux Kernel Mailing List On Tue, 10 Mar 2020 at 11:44, Adrian Hunter <adrian.hunter@intel.com> wrote: > > On 10/03/20 11:05 am, Ulf Hansson wrote: > > On Tue, 10 Mar 2020 at 05:28, Seo, Kyungmin <kyungmin.seo@intel.com> wrote: > >> > >> I read the link and patch of Chaotian Jing. > >> I also point out what Chaotian said. > >> Most host controllers have DLL tuning values for each mode. When host controller is set as HS400 mode with 50MHz clock, host controller uses DLL value which is tuned with 200MHz clock. > >> > >> If DLL value in HS400 mode doesn't have the pass range in HS mode, command transfer failing may fail. > >> In order to make robust sdhci driver, I think the patch needs to be considered. > > > > I have, but I am not picking it up in its current form. > > > >> Of course, CMD6 with HS400 mode and 200MHz clock should not cause any problem because it's correct configuration. > > > > Yes, but not for all cases, as I said in my reply in those email-threads. > > > > What I had in mind, is that I I think we should inform > > mmc_hs400_to_hs200() about under what situation it's getting called. > > Depending on that, we should either decrease the clock rate before or > > after we send the CMD6. > > > > Would that work for your case? > > Ulf, would you consider a new call back e.g. That could work, but I am not sure what's best, honestly. The problem may be generic or it could be specific to some host controller? I think we need to answer that question first. What do you think? Br Uffe > > diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c > index c2abd417a84a..1bc18fe2632f 100644 > --- a/drivers/mmc/core/mmc.c > +++ b/drivers/mmc/core/mmc.c > @@ -1237,7 +1237,10 @@ int mmc_hs400_to_hs200(struct mmc_card *card) > > /* Reduce frequency to HS */ > max_dtr = card->ext_csd.hs_max_dtr; > - mmc_set_clock(host, max_dtr); > + if (host->ops->hs400_to_hs200_prep) > + host->ops->hs400_to_hs200_prep(host, max_dtr); > + else > + mmc_set_clock(host, max_dtr); > > /* Switch HS400 to HS DDR */ > val = EXT_CSD_TIMING_HS; > > [...] Kind regards Uffe ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] mmc: mmc: Fix the timing for clock changing in mmc 2020-03-10 15:41 ` Ulf Hansson @ 2020-03-11 2:13 ` Chaotian Jing 2020-03-11 8:57 ` Ulf Hansson 0 siblings, 1 reply; 13+ messages in thread From: Chaotian Jing @ 2020-03-11 2:13 UTC (permalink / raw) To: Ulf Hansson Cc: Adrian Hunter, Seo, Kyungmin, linux-mmc, Linux Kernel Mailing List On Tue, 2020-03-10 at 16:41 +0100, Ulf Hansson wrote: > On Tue, 10 Mar 2020 at 11:44, Adrian Hunter <adrian.hunter@intel.com> wrote: > > > > On 10/03/20 11:05 am, Ulf Hansson wrote: > > > On Tue, 10 Mar 2020 at 05:28, Seo, Kyungmin <kyungmin.seo@intel.com> wrote: > > >> > > >> I read the link and patch of Chaotian Jing. > > >> I also point out what Chaotian said. > > >> Most host controllers have DLL tuning values for each mode. When host controller is set as HS400 mode with 50MHz clock, host controller uses DLL value which is tuned with 200MHz clock. > > >> > > >> If DLL value in HS400 mode doesn't have the pass range in HS mode, command transfer failing may fail. > > >> In order to make robust sdhci driver, I think the patch needs to be considered. > > > > > > I have, but I am not picking it up in its current form. > > > > > >> Of course, CMD6 with HS400 mode and 200MHz clock should not cause any problem because it's correct configuration. > > > > > > Yes, but not for all cases, as I said in my reply in those email-threads. > > > > > > What I had in mind, is that I I think we should inform > > > mmc_hs400_to_hs200() about under what situation it's getting called. > > > Depending on that, we should either decrease the clock rate before or > > > after we send the CMD6. > > > > > > Would that work for your case? > > > > Ulf, would you consider a new call back e.g. > > That could work, but I am not sure what's best, honestly. > > The problem may be generic or it could be specific to some host > controller? I think we need to answer that question first. > > What do you think? > > Br > Uffe > When start to send CMD6 to switch to HS mode, both Host & eMMC device are working on HS400 mode, so the timing used is MUST at HS400 mode and the clock MUST keep at current clock(usually 200Mhz). after received the response of CMD6, Never use CMD13 to polling card status for timing switch. if host has ops->card_busy() or caps WAIT_WHILE_BUSY, then use it, if not,just do mmc_delay() for specific time. the next step is that call mmc_set_ios() to set current timing to HS mode and clock to 50Mhz to let Host driver that eMMC device has been switched to HS mode and Host can switch to HS mode at 50Mhz(may apply parameters for this low speed). > > > > diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c > > index c2abd417a84a..1bc18fe2632f 100644 > > --- a/drivers/mmc/core/mmc.c > > +++ b/drivers/mmc/core/mmc.c > > @@ -1237,7 +1237,10 @@ int mmc_hs400_to_hs200(struct mmc_card *card) > > > > /* Reduce frequency to HS */ > > max_dtr = card->ext_csd.hs_max_dtr; > > - mmc_set_clock(host, max_dtr); > > + if (host->ops->hs400_to_hs200_prep) > > + host->ops->hs400_to_hs200_prep(host, max_dtr); > > + else > > + mmc_set_clock(host, max_dtr); > > > > /* Switch HS400 to HS DDR */ > > val = EXT_CSD_TIMING_HS; > > > > > > [...] > > Kind regards > Uffe ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] mmc: mmc: Fix the timing for clock changing in mmc 2020-03-11 2:13 ` Chaotian Jing @ 2020-03-11 8:57 ` Ulf Hansson 2020-03-11 9:59 ` Adrian Hunter 0 siblings, 1 reply; 13+ messages in thread From: Ulf Hansson @ 2020-03-11 8:57 UTC (permalink / raw) To: Chaotian Jing Cc: Adrian Hunter, Seo, Kyungmin, linux-mmc, Linux Kernel Mailing List On Wed, 11 Mar 2020 at 03:13, Chaotian Jing <chaotian.jing@mediatek.com> wrote: > > On Tue, 2020-03-10 at 16:41 +0100, Ulf Hansson wrote: > > On Tue, 10 Mar 2020 at 11:44, Adrian Hunter <adrian.hunter@intel.com> wrote: > > > > > > On 10/03/20 11:05 am, Ulf Hansson wrote: > > > > On Tue, 10 Mar 2020 at 05:28, Seo, Kyungmin <kyungmin.seo@intel.com> wrote: > > > >> > > > >> I read the link and patch of Chaotian Jing. > > > >> I also point out what Chaotian said. > > > >> Most host controllers have DLL tuning values for each mode. When host controller is set as HS400 mode with 50MHz clock, host controller uses DLL value which is tuned with 200MHz clock. > > > >> > > > >> If DLL value in HS400 mode doesn't have the pass range in HS mode, command transfer failing may fail. > > > >> In order to make robust sdhci driver, I think the patch needs to be considered. > > > > > > > > I have, but I am not picking it up in its current form. > > > > > > > >> Of course, CMD6 with HS400 mode and 200MHz clock should not cause any problem because it's correct configuration. > > > > > > > > Yes, but not for all cases, as I said in my reply in those email-threads. > > > > > > > > What I had in mind, is that I I think we should inform > > > > mmc_hs400_to_hs200() about under what situation it's getting called. > > > > Depending on that, we should either decrease the clock rate before or > > > > after we send the CMD6. > > > > > > > > Would that work for your case? > > > > > > Ulf, would you consider a new call back e.g. > > > > That could work, but I am not sure what's best, honestly. > > > > The problem may be generic or it could be specific to some host > > controller? I think we need to answer that question first. > > > > What do you think? > > > > Br > > Uffe > > > When start to send CMD6 to switch to HS mode, both Host & eMMC device > are working on HS400 mode, so the timing used is MUST at HS400 mode and > the clock MUST keep at current clock(usually 200Mhz). after received the > response of CMD6, Never use CMD13 to polling card status for timing > switch. if host has ops->card_busy() or caps WAIT_WHILE_BUSY, then use > it, if not,just do mmc_delay() for specific time. The CMD13 is currently not used when polling, because we set the send_status parameter to false in the calls to __mmc_switch(). So this should already be covered, according to your suggestions. Right? When it comes to keeping the clock rate as is, before sending the CMD6 - I fully agree that it's a good idea when doing a periodic retuning. As you would expect things to work as they are. The problem is, when you have received a CRC error and the re-tuning is being triggered because of that. In that case it may be a better option to decrease the clock rate, at least that is what I recall Adrian needs for his cases. Adrian? What will happen when you receive a CRC error and there is re-tuning triggered, is that something you have seen happening on you boards? > > the next step is that call mmc_set_ios() to set current timing to HS > mode and clock to 50Mhz to let Host driver that eMMC device has been > switched to HS mode and Host can switch to HS mode at 50Mhz(may apply > parameters for this low speed). Yep, makes sense. > > > > > > diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c > > > index c2abd417a84a..1bc18fe2632f 100644 > > > --- a/drivers/mmc/core/mmc.c > > > +++ b/drivers/mmc/core/mmc.c > > > @@ -1237,7 +1237,10 @@ int mmc_hs400_to_hs200(struct mmc_card *card) > > > > > > /* Reduce frequency to HS */ > > > max_dtr = card->ext_csd.hs_max_dtr; > > > - mmc_set_clock(host, max_dtr); > > > + if (host->ops->hs400_to_hs200_prep) > > > + host->ops->hs400_to_hs200_prep(host, max_dtr); > > > + else > > > + mmc_set_clock(host, max_dtr); > > > > > > /* Switch HS400 to HS DDR */ > > > val = EXT_CSD_TIMING_HS; > > > > > > > > > > [...] > > > > Kind regards > > Uffe > Kind regards Uffe ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] mmc: mmc: Fix the timing for clock changing in mmc 2020-03-11 8:57 ` Ulf Hansson @ 2020-03-11 9:59 ` Adrian Hunter 2020-03-11 14:06 ` Ulf Hansson 0 siblings, 1 reply; 13+ messages in thread From: Adrian Hunter @ 2020-03-11 9:59 UTC (permalink / raw) To: Ulf Hansson, Chaotian Jing Cc: Seo, Kyungmin, linux-mmc, Linux Kernel Mailing List On 11/03/20 10:57 am, Ulf Hansson wrote: > On Wed, 11 Mar 2020 at 03:13, Chaotian Jing <chaotian.jing@mediatek.com> wrote: >> >> On Tue, 2020-03-10 at 16:41 +0100, Ulf Hansson wrote: >>> On Tue, 10 Mar 2020 at 11:44, Adrian Hunter <adrian.hunter@intel.com> wrote: >>>> >>>> On 10/03/20 11:05 am, Ulf Hansson wrote: >>>>> On Tue, 10 Mar 2020 at 05:28, Seo, Kyungmin <kyungmin.seo@intel.com> wrote: >>>>>> >>>>>> I read the link and patch of Chaotian Jing. >>>>>> I also point out what Chaotian said. >>>>>> Most host controllers have DLL tuning values for each mode. When host controller is set as HS400 mode with 50MHz clock, host controller uses DLL value which is tuned with 200MHz clock. >>>>>> >>>>>> If DLL value in HS400 mode doesn't have the pass range in HS mode, command transfer failing may fail. >>>>>> In order to make robust sdhci driver, I think the patch needs to be considered. >>>>> >>>>> I have, but I am not picking it up in its current form. >>>>> >>>>>> Of course, CMD6 with HS400 mode and 200MHz clock should not cause any problem because it's correct configuration. >>>>> >>>>> Yes, but not for all cases, as I said in my reply in those email-threads. >>>>> >>>>> What I had in mind, is that I I think we should inform >>>>> mmc_hs400_to_hs200() about under what situation it's getting called. >>>>> Depending on that, we should either decrease the clock rate before or >>>>> after we send the CMD6. >>>>> >>>>> Would that work for your case? >>>> >>>> Ulf, would you consider a new call back e.g. >>> >>> That could work, but I am not sure what's best, honestly. >>> >>> The problem may be generic or it could be specific to some host >>> controller? I think we need to answer that question first. >>> >>> What do you think? >>> >>> Br >>> Uffe >>> >> When start to send CMD6 to switch to HS mode, both Host & eMMC device >> are working on HS400 mode, so the timing used is MUST at HS400 mode and >> the clock MUST keep at current clock(usually 200Mhz). after received the >> response of CMD6, Never use CMD13 to polling card status for timing >> switch. if host has ops->card_busy() or caps WAIT_WHILE_BUSY, then use >> it, if not,just do mmc_delay() for specific time. > > The CMD13 is currently not used when polling, because we set the > send_status parameter to false in the calls to __mmc_switch(). So this > should already be covered, according to your suggestions. Right? > > When it comes to keeping the clock rate as is, before sending the CMD6 > - I fully agree that it's a good idea when doing a periodic retuning. > As you would expect things to work as they are. > > The problem is, when you have received a CRC error and the re-tuning > is being triggered because of that. In that case it may be a better > option to decrease the clock rate, at least that is what I recall > Adrian needs for his cases. Adrian? It seems hardware supports HS400 only at the expected 200MHz frequency. The assumption then is that the command will be seen by the card but the response may have a CRC error. So we would need to ignore CRC errors, but it would also be worth waiting the timeout if the card is still busy whether or not there is an error. The only way to mitigate errors then is to increase the number of retries. > > What will happen when you receive a CRC error and there is re-tuning > triggered, is that something you have seen happening on you boards? > >> >> the next step is that call mmc_set_ios() to set current timing to HS >> mode and clock to 50Mhz to let Host driver that eMMC device has been >> switched to HS mode and Host can switch to HS mode at 50Mhz(may apply >> parameters for this low speed). > > Yep, makes sense. > >>>> >>>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c >>>> index c2abd417a84a..1bc18fe2632f 100644 >>>> --- a/drivers/mmc/core/mmc.c >>>> +++ b/drivers/mmc/core/mmc.c >>>> @@ -1237,7 +1237,10 @@ int mmc_hs400_to_hs200(struct mmc_card *card) >>>> >>>> /* Reduce frequency to HS */ >>>> max_dtr = card->ext_csd.hs_max_dtr; >>>> - mmc_set_clock(host, max_dtr); >>>> + if (host->ops->hs400_to_hs200_prep) >>>> + host->ops->hs400_to_hs200_prep(host, max_dtr); >>>> + else >>>> + mmc_set_clock(host, max_dtr); >>>> >>>> /* Switch HS400 to HS DDR */ >>>> val = EXT_CSD_TIMING_HS; >>>> >>>> >>> >>> [...] >>> >>> Kind regards >>> Uffe >> > > Kind regards > Uffe > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] mmc: mmc: Fix the timing for clock changing in mmc 2020-03-11 9:59 ` Adrian Hunter @ 2020-03-11 14:06 ` Ulf Hansson 2020-03-11 14:43 ` Adrian Hunter 0 siblings, 1 reply; 13+ messages in thread From: Ulf Hansson @ 2020-03-11 14:06 UTC (permalink / raw) To: Adrian Hunter Cc: Chaotian Jing, Seo, Kyungmin, linux-mmc, Linux Kernel Mailing List On Wed, 11 Mar 2020 at 11:00, Adrian Hunter <adrian.hunter@intel.com> wrote: > > On 11/03/20 10:57 am, Ulf Hansson wrote: > > On Wed, 11 Mar 2020 at 03:13, Chaotian Jing <chaotian.jing@mediatek.com> wrote: > >> > >> On Tue, 2020-03-10 at 16:41 +0100, Ulf Hansson wrote: > >>> On Tue, 10 Mar 2020 at 11:44, Adrian Hunter <adrian.hunter@intel.com> wrote: > >>>> > >>>> On 10/03/20 11:05 am, Ulf Hansson wrote: > >>>>> On Tue, 10 Mar 2020 at 05:28, Seo, Kyungmin <kyungmin.seo@intel.com> wrote: > >>>>>> > >>>>>> I read the link and patch of Chaotian Jing. > >>>>>> I also point out what Chaotian said. > >>>>>> Most host controllers have DLL tuning values for each mode. When host controller is set as HS400 mode with 50MHz clock, host controller uses DLL value which is tuned with 200MHz clock. > >>>>>> > >>>>>> If DLL value in HS400 mode doesn't have the pass range in HS mode, command transfer failing may fail. > >>>>>> In order to make robust sdhci driver, I think the patch needs to be considered. > >>>>> > >>>>> I have, but I am not picking it up in its current form. > >>>>> > >>>>>> Of course, CMD6 with HS400 mode and 200MHz clock should not cause any problem because it's correct configuration. > >>>>> > >>>>> Yes, but not for all cases, as I said in my reply in those email-threads. > >>>>> > >>>>> What I had in mind, is that I I think we should inform > >>>>> mmc_hs400_to_hs200() about under what situation it's getting called. > >>>>> Depending on that, we should either decrease the clock rate before or > >>>>> after we send the CMD6. > >>>>> > >>>>> Would that work for your case? > >>>> > >>>> Ulf, would you consider a new call back e.g. > >>> > >>> That could work, but I am not sure what's best, honestly. > >>> > >>> The problem may be generic or it could be specific to some host > >>> controller? I think we need to answer that question first. > >>> > >>> What do you think? > >>> > >>> Br > >>> Uffe > >>> > >> When start to send CMD6 to switch to HS mode, both Host & eMMC device > >> are working on HS400 mode, so the timing used is MUST at HS400 mode and > >> the clock MUST keep at current clock(usually 200Mhz). after received the > >> response of CMD6, Never use CMD13 to polling card status for timing > >> switch. if host has ops->card_busy() or caps WAIT_WHILE_BUSY, then use > >> it, if not,just do mmc_delay() for specific time. > > > > The CMD13 is currently not used when polling, because we set the > > send_status parameter to false in the calls to __mmc_switch(). So this > > should already be covered, according to your suggestions. Right? > > > > When it comes to keeping the clock rate as is, before sending the CMD6 > > - I fully agree that it's a good idea when doing a periodic retuning. > > As you would expect things to work as they are. > > > > The problem is, when you have received a CRC error and the re-tuning > > is being triggered because of that. In that case it may be a better > > option to decrease the clock rate, at least that is what I recall > > Adrian needs for his cases. Adrian? > > It seems hardware supports HS400 only at the expected 200MHz frequency. Yes, that's my understanding as well. > The assumption then is that the command will be seen by the card but the > response may have a CRC error. So we would need to ignore CRC errors, but > it would also be worth waiting the timeout if the card is still busy whether > or not there is an error. Alright, so you're saying that keeping the clock rate to HS400 speed (decrease it after CMD6) could be fine, if we implement the above instead? > > The only way to mitigate errors then is to increase the number of retries. We already use MMC_CMD_RETRIES for CMD6. Is that sufficient you think (again assuming we implement to allow CRC errors for these CMD6 commands)? Or are you suggesting we may need a re-try of the hole re-tune thing? Maybe a better option is then to simply bail out and do full re-init of the card? > > > > > What will happen when you receive a CRC error and there is re-tuning > > triggered, is that something you have seen happening on you boards? > > > >> > >> the next step is that call mmc_set_ios() to set current timing to HS > >> mode and clock to 50Mhz to let Host driver that eMMC device has been > >> switched to HS mode and Host can switch to HS mode at 50Mhz(may apply > >> parameters for this low speed). > > > > Yep, makes sense. > > > >>>> > >>>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c > >>>> index c2abd417a84a..1bc18fe2632f 100644 > >>>> --- a/drivers/mmc/core/mmc.c > >>>> +++ b/drivers/mmc/core/mmc.c > >>>> @@ -1237,7 +1237,10 @@ int mmc_hs400_to_hs200(struct mmc_card *card) > >>>> > >>>> /* Reduce frequency to HS */ > >>>> max_dtr = card->ext_csd.hs_max_dtr; > >>>> - mmc_set_clock(host, max_dtr); > >>>> + if (host->ops->hs400_to_hs200_prep) > >>>> + host->ops->hs400_to_hs200_prep(host, max_dtr); > >>>> + else > >>>> + mmc_set_clock(host, max_dtr); > >>>> > >>>> /* Switch HS400 to HS DDR */ > >>>> val = EXT_CSD_TIMING_HS; > >>>> > >>>> Kind regards Uffe ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] mmc: mmc: Fix the timing for clock changing in mmc 2020-03-11 14:06 ` Ulf Hansson @ 2020-03-11 14:43 ` Adrian Hunter 0 siblings, 0 replies; 13+ messages in thread From: Adrian Hunter @ 2020-03-11 14:43 UTC (permalink / raw) To: Ulf Hansson Cc: Chaotian Jing, Seo, Kyungmin, linux-mmc, Linux Kernel Mailing List On 11/03/20 4:06 pm, Ulf Hansson wrote: > On Wed, 11 Mar 2020 at 11:00, Adrian Hunter <adrian.hunter@intel.com> wrote: >> >> On 11/03/20 10:57 am, Ulf Hansson wrote: >>> On Wed, 11 Mar 2020 at 03:13, Chaotian Jing <chaotian.jing@mediatek.com> wrote: >>>> >>>> On Tue, 2020-03-10 at 16:41 +0100, Ulf Hansson wrote: >>>>> On Tue, 10 Mar 2020 at 11:44, Adrian Hunter <adrian.hunter@intel.com> wrote: >>>>>> >>>>>> On 10/03/20 11:05 am, Ulf Hansson wrote: >>>>>>> On Tue, 10 Mar 2020 at 05:28, Seo, Kyungmin <kyungmin.seo@intel.com> wrote: >>>>>>>> >>>>>>>> I read the link and patch of Chaotian Jing. >>>>>>>> I also point out what Chaotian said. >>>>>>>> Most host controllers have DLL tuning values for each mode. When host controller is set as HS400 mode with 50MHz clock, host controller uses DLL value which is tuned with 200MHz clock. >>>>>>>> >>>>>>>> If DLL value in HS400 mode doesn't have the pass range in HS mode, command transfer failing may fail. >>>>>>>> In order to make robust sdhci driver, I think the patch needs to be considered. >>>>>>> >>>>>>> I have, but I am not picking it up in its current form. >>>>>>> >>>>>>>> Of course, CMD6 with HS400 mode and 200MHz clock should not cause any problem because it's correct configuration. >>>>>>> >>>>>>> Yes, but not for all cases, as I said in my reply in those email-threads. >>>>>>> >>>>>>> What I had in mind, is that I I think we should inform >>>>>>> mmc_hs400_to_hs200() about under what situation it's getting called. >>>>>>> Depending on that, we should either decrease the clock rate before or >>>>>>> after we send the CMD6. >>>>>>> >>>>>>> Would that work for your case? >>>>>> >>>>>> Ulf, would you consider a new call back e.g. >>>>> >>>>> That could work, but I am not sure what's best, honestly. >>>>> >>>>> The problem may be generic or it could be specific to some host >>>>> controller? I think we need to answer that question first. >>>>> >>>>> What do you think? >>>>> >>>>> Br >>>>> Uffe >>>>> >>>> When start to send CMD6 to switch to HS mode, both Host & eMMC device >>>> are working on HS400 mode, so the timing used is MUST at HS400 mode and >>>> the clock MUST keep at current clock(usually 200Mhz). after received the >>>> response of CMD6, Never use CMD13 to polling card status for timing >>>> switch. if host has ops->card_busy() or caps WAIT_WHILE_BUSY, then use >>>> it, if not,just do mmc_delay() for specific time. >>> >>> The CMD13 is currently not used when polling, because we set the >>> send_status parameter to false in the calls to __mmc_switch(). So this >>> should already be covered, according to your suggestions. Right? >>> >>> When it comes to keeping the clock rate as is, before sending the CMD6 >>> - I fully agree that it's a good idea when doing a periodic retuning. >>> As you would expect things to work as they are. >>> >>> The problem is, when you have received a CRC error and the re-tuning >>> is being triggered because of that. In that case it may be a better >>> option to decrease the clock rate, at least that is what I recall >>> Adrian needs for his cases. Adrian? >> >> It seems hardware supports HS400 only at the expected 200MHz frequency. > > Yes, that's my understanding as well. > >> The assumption then is that the command will be seen by the card but the >> response may have a CRC error. So we would need to ignore CRC errors, but >> it would also be worth waiting the timeout if the card is still busy whether >> or not there is an error. > > Alright, so you're saying that keeping the clock rate to HS400 speed > (decrease it after CMD6) could be fine, if we implement the above > instead? Yes, but I would like to try adding a simple retry at the lower frequency as well and see how well that works. I will test that tomorrow. > >> >> The only way to mitigate errors then is to increase the number of retries. > > We already use MMC_CMD_RETRIES for CMD6. Is that sufficient you think > (again assuming we implement to allow CRC errors for these CMD6 > commands)? That is only 3. If the CRC errors are random, then trying more times might help. > > Or are you suggesting we may need a re-try of the hole re-tune thing? > Maybe a better option is then to simply bail out and do full re-init > of the card? The block driver will try that at some point. I will see if I can test how well that works in this case. > >> >>> >>> What will happen when you receive a CRC error and there is re-tuning >>> triggered, is that something you have seen happening on you boards? >>> >>>> >>>> the next step is that call mmc_set_ios() to set current timing to HS >>>> mode and clock to 50Mhz to let Host driver that eMMC device has been >>>> switched to HS mode and Host can switch to HS mode at 50Mhz(may apply >>>> parameters for this low speed). >>> >>> Yep, makes sense. >>> >>>>>> >>>>>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c >>>>>> index c2abd417a84a..1bc18fe2632f 100644 >>>>>> --- a/drivers/mmc/core/mmc.c >>>>>> +++ b/drivers/mmc/core/mmc.c >>>>>> @@ -1237,7 +1237,10 @@ int mmc_hs400_to_hs200(struct mmc_card *card) >>>>>> >>>>>> /* Reduce frequency to HS */ >>>>>> max_dtr = card->ext_csd.hs_max_dtr; >>>>>> - mmc_set_clock(host, max_dtr); >>>>>> + if (host->ops->hs400_to_hs200_prep) >>>>>> + host->ops->hs400_to_hs200_prep(host, max_dtr); >>>>>> + else >>>>>> + mmc_set_clock(host, max_dtr); >>>>>> >>>>>> /* Switch HS400 to HS DDR */ >>>>>> val = EXT_CSD_TIMING_HS; >>>>>> >>>>>> > > Kind regards > Uffe > ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2020-03-11 14:44 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-02-12 2:42 [PATCH] mmc: mmc: Fix the timing for clock changing in mmc Kyungmin Seo 2020-03-04 11:08 ` Ulf Hansson 2020-03-05 8:57 ` Seo, Kyungmin 2020-03-05 15:17 ` Ulf Hansson 2020-03-10 4:27 ` Seo, Kyungmin 2020-03-10 9:05 ` Ulf Hansson 2020-03-10 10:44 ` Adrian Hunter 2020-03-10 15:41 ` Ulf Hansson 2020-03-11 2:13 ` Chaotian Jing 2020-03-11 8:57 ` Ulf Hansson 2020-03-11 9:59 ` Adrian Hunter 2020-03-11 14:06 ` Ulf Hansson 2020-03-11 14:43 ` Adrian Hunter
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).