| Message ID | 1501643563-16094-2-git-send-email-shawn.lin@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 2 August 2017 at 05:12, Shawn Lin <shawn.lin@rock-chips.com> wrote: > Per the SD physical layer simplified specification V4.10, > section 4.6.2, the taac and nasc for SDHC are always fixed and > the software should use the recommended value for timeout. > When parsing the CSD, we sanely set them to zero for SDHC, the > additional check of SDHC in mmc_set_data_timeout is bogus since > all the calculation for timeout_ns and timeout_clk is zero as well, > so the we could safely remove it and let the following check to > cover it. For SD cards with CSD structure version set to 0 (old cards) we are using the taac/nasc values. Perhaps you can clarify that in the above changelog. > > Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> > --- > > drivers/mmc/core/core.c | 10 +++------- > 1 file changed, 3 insertions(+), 7 deletions(-) > > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c > index 6177eb0..bcd72b4 100644 > --- a/drivers/mmc/core/core.c > +++ b/drivers/mmc/core/core.c > @@ -761,14 +761,10 @@ void mmc_set_data_timeout(struct mmc_data *data, const struct mmc_card *card) > limit_us = 100000; > > /* > - * SDHC cards always use these fixed values. > + * Assign limit value if invalid. Note that for the SDHC case, > + * we set taac and nasc to zero when parsing CSD, so it's safe > + * to fall through here. > */ > - if (timeout_us > limit_us || mmc_card_blockaddr(card)) { I think the mmc_card_blockaddr() here is what make this a bit strange. Because as you are saying, in those cases the taac/nasc are set to zero. So in principle we should at least be able to remove the check for the mmc_card_blockaddr(), but don't you think we need to keep the other check for "timeout_us > limit_us", to still cope with legacy cards? > - data->timeout_ns = limit_us * 1000; > - data->timeout_clks = 0; > - } > - > - /* assign limit value if invalid */ > if (timeout_us == 0) > data->timeout_ns = limit_us * 1000; > } > -- Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2017/8/3 18:51, Ulf Hansson wrote: > On 2 August 2017 at 05:12, Shawn Lin <shawn.lin@rock-chips.com> wrote: >> Per the SD physical layer simplified specification V4.10, >> section 4.6.2, the taac and nasc for SDHC are always fixed and >> the software should use the recommended value for timeout. >> When parsing the CSD, we sanely set them to zero for SDHC, the >> additional check of SDHC in mmc_set_data_timeout is bogus since >> all the calculation for timeout_ns and timeout_clk is zero as well, >> so the we could safely remove it and let the following check to >> cover it. > > For SD cards with CSD structure version set to 0 (old cards) we are > using the taac/nasc values. Perhaps you can clarify that in the above > changelog. > Sure. >> >> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> >> --- >> >> drivers/mmc/core/core.c | 10 +++------- >> 1 file changed, 3 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c >> index 6177eb0..bcd72b4 100644 >> --- a/drivers/mmc/core/core.c >> +++ b/drivers/mmc/core/core.c >> @@ -761,14 +761,10 @@ void mmc_set_data_timeout(struct mmc_data *data, const struct mmc_card *card) >> limit_us = 100000; >> >> /* >> - * SDHC cards always use these fixed values. >> + * Assign limit value if invalid. Note that for the SDHC case, >> + * we set taac and nasc to zero when parsing CSD, so it's safe >> + * to fall through here. >> */ >> - if (timeout_us > limit_us || mmc_card_blockaddr(card)) { > > I think the mmc_card_blockaddr() here is what make this a bit strange. > Because as you are saying, in those cases the taac/nasc are set to > zero. > > So in principle we should at least be able to remove the check for the > mmc_card_blockaddr(), but don't you think we need to keep the other > check for "timeout_us > limit_us", to still cope with legacy cards? > yes, will fix that. >> - data->timeout_ns = limit_us * 1000; >> - data->timeout_clks = 0; >> - } >> - >> - /* assign limit value if invalid */ >> if (timeout_us == 0) >> data->timeout_ns = limit_us * 1000; >> } >> -- > > Kind regards > Uffe > > >
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c index 6177eb0..bcd72b4 100644 --- a/drivers/mmc/core/core.c +++ b/drivers/mmc/core/core.c @@ -761,14 +761,10 @@ void mmc_set_data_timeout(struct mmc_data *data, const struct mmc_card *card) limit_us = 100000; /* - * SDHC cards always use these fixed values. + * Assign limit value if invalid. Note that for the SDHC case, + * we set taac and nasc to zero when parsing CSD, so it's safe + * to fall through here. */ - if (timeout_us > limit_us || mmc_card_blockaddr(card)) { - data->timeout_ns = limit_us * 1000; - data->timeout_clks = 0; - } - - /* assign limit value if invalid */ if (timeout_us == 0) data->timeout_ns = limit_us * 1000; }
Per the SD physical layer simplified specification V4.10, section 4.6.2, the taac and nasc for SDHC are always fixed and the software should use the recommended value for timeout. When parsing the CSD, we sanely set them to zero for SDHC, the additional check of SDHC in mmc_set_data_timeout is bogus since all the calculation for timeout_ns and timeout_clk is zero as well, so the we could safely remove it and let the following check to cover it. Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> --- drivers/mmc/core/core.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-)