| Message ID | 20200604112040.22144-2-wsa+renesas@sang-engineering.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | renesas_sdhi: fix hang when SCC loses its clock | expand |
Hi Wolfram-san, Thank you for the patch! > From: Wolfram Sang, Sent: Thursday, June 4, 2020 8:21 PM > > The driver specific downgrade function makes more sense if we run it > before we switch anything, not after we already switched. Otherwise some > non-HS400 communication has already happened. > > No need to convert users. There is only one currenty which needs this > change in a later patchset. Perhaps, should we add Fixes tag like below? Fixes: ba6c7ac3a2f4 ("mmc: core: more fine-grained hooks for HS400 tuning") > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > --- > drivers/mmc/core/mmc.c | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c > index 4203303f946a..f97994eace3b 100644 > --- a/drivers/mmc/core/mmc.c > +++ b/drivers/mmc/core/mmc.c > @@ -1156,6 +1156,10 @@ static int mmc_select_hs400(struct mmc_card *card) > host->ios.bus_width == MMC_BUS_WIDTH_8)) > return 0; > > + /* Prepare host to downgrade to HS timing */ > + if (host->ops->hs400_downgrade) > + host->ops->hs400_downgrade(host); > + IICU, we should call hs400_downgrade() between the __mmc_switch("EXT_CSD_TIMING_HS") and mmc_set_timing(card->host, MMC_TIMING_MMC_HS) because the switch command should be issued in HS400 mode. > /* Switch card to HS mode */ > val = EXT_CSD_TIMING_HS; > err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, > @@ -1171,10 +1175,6 @@ static int mmc_select_hs400(struct mmc_card *card) > /* Set host controller to HS timing */ > mmc_set_timing(card->host, MMC_TIMING_MMC_HS); > > - /* Prepare host to downgrade to HS timing */ > - if (host->ops->hs400_downgrade) > - host->ops->hs400_downgrade(host); > - > /* Reduce frequency to HS frequency */ > max_dtr = card->ext_csd.hs_max_dtr; > mmc_set_clock(host, max_dtr); > @@ -1241,6 +1241,9 @@ int mmc_hs400_to_hs200(struct mmc_card *card) > int err; > u8 val; > > + if (host->ops->hs400_downgrade) > + host->ops->hs400_downgrade(host); > + IIUC, this also should be called between __mmc_switch("EXT_CSD_TIMING_HS") to mmc_set_timing(MMC_TIMING_MMC_DDR52). Best regards, Yoshihiro Shimoda
Hi Shimoda-san, thank you very much for the review! > > The driver specific downgrade function makes more sense if we run it > > before we switch anything, not after we already switched. Otherwise some > > non-HS400 communication has already happened. > > > > No need to convert users. There is only one currenty which needs this > > change in a later patchset. > > Perhaps, should we add Fixes tag like below? > > Fixes: ba6c7ac3a2f4 ("mmc: core: more fine-grained hooks for HS400 tuning") I am not sure. While it is more correct to move the call to hs400_downgrade upwards, it does not really fix a bug on its own. Without patch 2/2 of this series, there is not really a huge difference when we disable the SCC the old way. For the new way, this patch is a prerequisite. > > + /* Prepare host to downgrade to HS timing */ > > + if (host->ops->hs400_downgrade) > > + host->ops->hs400_downgrade(host); > > + > > IICU, we should call hs400_downgrade() between the __mmc_switch("EXT_CSD_TIMING_HS") > and mmc_set_timing(card->host, MMC_TIMING_MMC_HS) because the switch command should > be issued in HS400 mode. Yes, of course, you are right. Will fix! Kind regards, Wolfram
Hi Wolfram-san, > From: Wolfram Sang, Sent: Tuesday, June 9, 2020 5:41 AM > > Hi Shimoda-san, > > thank you very much for the review! > > > > The driver specific downgrade function makes more sense if we run it > > > before we switch anything, not after we already switched. Otherwise some > > > non-HS400 communication has already happened. > > > > > > No need to convert users. There is only one currenty which needs this > > > change in a later patchset. > > > > Perhaps, should we add Fixes tag like below? > > > > Fixes: ba6c7ac3a2f4 ("mmc: core: more fine-grained hooks for HS400 tuning") > > I am not sure. While it is more correct to move the call to > hs400_downgrade upwards, it does not really fix a bug on its own. > Without patch 2/2 of this series, there is not really a huge difference > when we disable the SCC the old way. For the new way, this patch is a > prerequisite. I got it. So, we should not add this Fixes tag. Best regards, Yoshihiro Shimoda
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c index 4203303f946a..f97994eace3b 100644 --- a/drivers/mmc/core/mmc.c +++ b/drivers/mmc/core/mmc.c @@ -1156,6 +1156,10 @@ static int mmc_select_hs400(struct mmc_card *card) host->ios.bus_width == MMC_BUS_WIDTH_8)) return 0; + /* Prepare host to downgrade to HS timing */ + if (host->ops->hs400_downgrade) + host->ops->hs400_downgrade(host); + /* Switch card to HS mode */ val = EXT_CSD_TIMING_HS; err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, @@ -1171,10 +1175,6 @@ static int mmc_select_hs400(struct mmc_card *card) /* Set host controller to HS timing */ mmc_set_timing(card->host, MMC_TIMING_MMC_HS); - /* Prepare host to downgrade to HS timing */ - if (host->ops->hs400_downgrade) - host->ops->hs400_downgrade(host); - /* Reduce frequency to HS frequency */ max_dtr = card->ext_csd.hs_max_dtr; mmc_set_clock(host, max_dtr); @@ -1241,6 +1241,9 @@ int mmc_hs400_to_hs200(struct mmc_card *card) int err; u8 val; + if (host->ops->hs400_downgrade) + host->ops->hs400_downgrade(host); + /* Reduce frequency to HS */ max_dtr = card->ext_csd.hs_max_dtr; mmc_set_clock(host, max_dtr); @@ -1268,9 +1271,6 @@ int mmc_hs400_to_hs200(struct mmc_card *card) mmc_set_timing(host, MMC_TIMING_MMC_HS); - if (host->ops->hs400_downgrade) - host->ops->hs400_downgrade(host); - err = mmc_switch_status(card, true); if (err) goto out_err;
The driver specific downgrade function makes more sense if we run it before we switch anything, not after we already switched. Otherwise some non-HS400 communication has already happened. No need to convert users. There is only one currenty which needs this change in a later patchset. Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> --- drivers/mmc/core/mmc.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)