| Message ID | 20200710110819.28965-1-benchuanggli@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
On Fri, 10 Jul 2020 at 13:07, Ben Chuang <benchuanggli@gmail.com> wrote: > > From: AKASHI Takahiro <takahiro.akashi@linaro.org> > > According to Fig. 3-35 in "SD Host Controller Simplified Spec. Ver4.20": > - Prepare vdd1, vdd2 and ios.timing for using after/in step (2) > - chip_select is not used in UHS-II, used to return to the legacy flow Thanks for pointing to the spec, but please explain why/what/how for the change - as this helps me to review. I am going to stop commenting on each patch's commit message, beyond this patch - as it seems the same comment applies to more patches. > > Signed-off-by: Ben Chuang <ben.chuang@genesyslogic.com.tw> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > --- > drivers/mmc/core/core.c | 62 ++++++++++++++++++++++++------------ > drivers/mmc/core/regulator.c | 14 ++++++++ > 2 files changed, 56 insertions(+), 20 deletions(-) > > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c > index 8d2b808e9b58..85c83c82ad0c 100644 > --- a/drivers/mmc/core/core.c > +++ b/drivers/mmc/core/core.c > @@ -1315,33 +1315,51 @@ void mmc_power_up(struct mmc_host *host, u32 ocr) > if (host->ios.power_mode == MMC_POWER_ON) > return; > > - mmc_pwrseq_pre_power_on(host); > + if (host->flags & MMC_UHS2_SUPPORT) { > + /* TODO: handle 'ocr' parameter */ > + host->ios.vdd = fls(host->ocr_avail) - 1; > + host->ios.vdd2 = fls(host->ocr_avail_uhs2) - 1; > + if (mmc_host_is_spi(host)) > + host->ios.chip_select = MMC_CS_HIGH; > + else > + host->ios.chip_select = MMC_CS_DONTCARE; > + host->ios.timing = MMC_TIMING_UHS2; If I understand correctly, the intent is to always try to initialize the UHS-II interface/phy if that is supported. That doesn't seem correct to me. What about if the SD card doesn't support UHS-II, then we should use the legacy SD interface instead right? Or perhaps the MMC_UHS2_SUPPORT bit becomes cleared somewhere in the error path when first trying to initialize an UHS-II card, from subsequent changes? So, assuming that is the intent then, I am still not sure about this approach. What about if we instead always start with legacy SD initialization? When we have read the OCR register, via mmc_send_app_op_cond(), we can check if the card supports UHS-II by looking at the UHS-II Card Status (bit 29). If it turns out that the card supports UHS-II and the host does as well, then we do a mmc_power_off() to completely reset the card/host/phy. Then we can call into a UHS-II specific path, that tries to power on and initialize things according to the UHS-II spec. In this way, we are going to prioritize initialization of legacy SD cards to remain quick, as we won't try to use UHS-II unless the card supports it. Moreover, I get the impression that we can keep the existing code more as is - and instead introduce UHS-II specifics in a separate path. This also also for UHS-II specific optimizations, I think. > + } else { > + mmc_pwrseq_pre_power_on(host); > > - host->ios.vdd = fls(ocr) - 1; > - host->ios.power_mode = MMC_POWER_UP; > - /* Set initial state and call mmc_set_ios */ > - mmc_set_initial_state(host); > + host->ios.vdd = fls(ocr) - 1; > + host->ios.power_mode = MMC_POWER_UP; > + /* Set initial state and call mmc_set_ios */ > + mmc_set_initial_state(host); > > - mmc_set_initial_signal_voltage(host); > + mmc_set_initial_signal_voltage(host); > > - /* > - * This delay should be sufficient to allow the power supply > - * to reach the minimum voltage. > - */ > - mmc_delay(host->ios.power_delay_ms); > - > - mmc_pwrseq_post_power_on(host); > + /* > + * This delay should be sufficient to allow the power supply > + * to reach the minimum voltage. > + */ > + mmc_delay(host->ios.power_delay_ms); > > + mmc_pwrseq_post_power_on(host); > + } > host->ios.clock = host->f_init; > - > host->ios.power_mode = MMC_POWER_ON; > + > mmc_set_ios(host); > > - /* > - * This delay must be at least 74 clock sizes, or 1 ms, or the > - * time required to reach a stable voltage. > - */ > - mmc_delay(host->ios.power_delay_ms); > + if (host->flags & MMC_UHS2_SUPPORT) > + /* > + * This delay should be sufficient to allow the power supply > + * to reach the minimum voltage. > + */ > + /* TODO: avoid an immediate value */ > + mmc_delay(10); > + else > + /* > + * This delay must be at least 74 clock sizes, or 1 ms, or the > + * time required to reach a stable voltage. > + */ > + mmc_delay(host->ios.power_delay_ms); > } > > void mmc_power_off(struct mmc_host *host) > @@ -2307,7 +2325,11 @@ void mmc_start_host(struct mmc_host *host) > > if (!(host->caps2 & MMC_CAP2_NO_PRESCAN_POWERUP)) { > mmc_claim_host(host); > - mmc_power_up(host, host->ocr_avail); > + > + /* Power up here will make UHS2 init ugly. */ > + if (!(host->caps & MMC_CAP_UHS2)) > + mmc_power_up(host, host->ocr_avail); > + According to my suggestions, then this would not be needed. > mmc_release_host(host); > } > > diff --git a/drivers/mmc/core/regulator.c b/drivers/mmc/core/regulator.c > index 96b1d15045d6..05556225d9ac 100644 > --- a/drivers/mmc/core/regulator.c > +++ b/drivers/mmc/core/regulator.c > @@ -247,6 +247,7 @@ int mmc_regulator_get_supply(struct mmc_host *mmc) > > mmc->supply.vmmc = devm_regulator_get_optional(dev, "vmmc"); > mmc->supply.vqmmc = devm_regulator_get_optional(dev, "vqmmc"); > + mmc->supply.vmmc2 = devm_regulator_get_optional(dev, "vmmc2"); Please move the regulator thingy here into a separate patch. Please make sure corresponding header file, adding the vmmc2 to it is part of that change as well. > > if (IS_ERR(mmc->supply.vmmc)) { > if (PTR_ERR(mmc->supply.vmmc) == -EPROBE_DEFER) > @@ -266,6 +267,19 @@ int mmc_regulator_get_supply(struct mmc_host *mmc) > dev_dbg(dev, "No vqmmc regulator found\n"); > } > > + if (IS_ERR(mmc->supply.vmmc2)) { > + if (PTR_ERR(mmc->supply.vmmc2) == -EPROBE_DEFER) > + return -EPROBE_DEFER; > + dev_dbg(dev, "No vmmc2 regulator found\n"); > + } else { > + ret = mmc_regulator_get_ocrmask(mmc->supply.vmmc2); > + if (ret > 0) > + mmc->ocr_avail_uhs2 = ret; > + else > + dev_warn(dev, "Failed getting UHS2 OCR mask: %d\n", > + ret); > + } > + > return 0; > } > EXPORT_SYMBOL_GPL(mmc_regulator_get_supply); > -- > 2.27.0 > Kind regards Uffe
Hi Ulf, On Fri, Jul 17, 2020 at 7:26 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > On Fri, 10 Jul 2020 at 13:07, Ben Chuang <benchuanggli@gmail.com> wrote: > > > > From: AKASHI Takahiro <takahiro.akashi@linaro.org> > > > > According to Fig. 3-35 in "SD Host Controller Simplified Spec. Ver4.20": > > - Prepare vdd1, vdd2 and ios.timing for using after/in step (2) > > - chip_select is not used in UHS-II, used to return to the legacy flow > > Thanks for pointing to the spec, but please explain why/what/how for > the change - as this helps me to review. > > I am going to stop commenting on each patch's commit message, beyond > this patch - as it seems the same comment applies to more patches. > > > > > Signed-off-by: Ben Chuang <ben.chuang@genesyslogic.com.tw> > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > --- > > drivers/mmc/core/core.c | 62 ++++++++++++++++++++++++------------ > > drivers/mmc/core/regulator.c | 14 ++++++++ > > 2 files changed, 56 insertions(+), 20 deletions(-) > > > > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c > > index 8d2b808e9b58..85c83c82ad0c 100644 > > --- a/drivers/mmc/core/core.c > > +++ b/drivers/mmc/core/core.c > > @@ -1315,33 +1315,51 @@ void mmc_power_up(struct mmc_host *host, u32 ocr) > > if (host->ios.power_mode == MMC_POWER_ON) > > return; > > > > - mmc_pwrseq_pre_power_on(host); > > + if (host->flags & MMC_UHS2_SUPPORT) { > > + /* TODO: handle 'ocr' parameter */ > > + host->ios.vdd = fls(host->ocr_avail) - 1; > > + host->ios.vdd2 = fls(host->ocr_avail_uhs2) - 1; > > + if (mmc_host_is_spi(host)) > > + host->ios.chip_select = MMC_CS_HIGH; > > + else > > + host->ios.chip_select = MMC_CS_DONTCARE; > > + host->ios.timing = MMC_TIMING_UHS2; > > If I understand correctly, the intent is to always try to initialize > the UHS-II interface/phy if that is supported. That doesn't seem > correct to me. What about if the SD card doesn't support UHS-II, then > we should use the legacy SD interface instead right? Please always try UHS-II I/F first, then if UHS-II I/F fails, then switch to SD I/F. > > Or perhaps the MMC_UHS2_SUPPORT bit becomes cleared somewhere in the > error path when first trying to initialize an UHS-II card, from > subsequent changes? Yes, MMC_UHS2_SUPPORT will be cleared in some cases. > > So, assuming that is the intent then, I am still not sure about this approach. > > What about if we instead always start with legacy SD initialization? > When we have read the OCR register, via mmc_send_app_op_cond(), we can > check if the card supports UHS-II by looking at the UHS-II Card Status > (bit 29). UHS-II spec recommends to detect UHS-II first. Or in Host controller spec, section 3.13.2 card interface detection sequence, it also starts from UHS-II path, then go SD legacy path if UHS-II initialization fails. The bit29 in response of ACMD41 is defined as “UHS-II Card Status”, not UHS-II supported. We have experience using this value to determine whether a card supports UHS-II, but not every card reports if they support UHS-II by the response of ACMD41 correctly. > > If it turns out that the card supports UHS-II and the host does as > well, then we do a mmc_power_off() to completely reset the > card/host/phy. Then we can call into a UHS-II specific path, that > tries to power on and initialize things according to the UHS-II spec. > > In this way, we are going to prioritize initialization of legacy SD > cards to remain quick, as we won't try to use UHS-II unless the card > supports it. Moreover, I get the impression that we can keep the > existing code more as is - and instead introduce UHS-II specifics in a > separate path. This also also for UHS-II specific optimizations, I > think. Agree that we can try to keep the existing code and also need your advice/help. > > > + } else { > > + mmc_pwrseq_pre_power_on(host); > > > > - host->ios.vdd = fls(ocr) - 1; > > - host->ios.power_mode = MMC_POWER_UP; > > - /* Set initial state and call mmc_set_ios */ > > - mmc_set_initial_state(host); > > + host->ios.vdd = fls(ocr) - 1; > > + host->ios.power_mode = MMC_POWER_UP; > > + /* Set initial state and call mmc_set_ios */ > > + mmc_set_initial_state(host); > > > > - mmc_set_initial_signal_voltage(host); > > + mmc_set_initial_signal_voltage(host); > > > > - /* > > - * This delay should be sufficient to allow the power supply > > - * to reach the minimum voltage. > > - */ > > - mmc_delay(host->ios.power_delay_ms); > > - > > - mmc_pwrseq_post_power_on(host); > > + /* > > + * This delay should be sufficient to allow the power supply > > + * to reach the minimum voltage. > > + */ > > + mmc_delay(host->ios.power_delay_ms); > > > > + mmc_pwrseq_post_power_on(host); > > + } > > host->ios.clock = host->f_init; > > - > > host->ios.power_mode = MMC_POWER_ON; > > + > > mmc_set_ios(host); > > > > - /* > > - * This delay must be at least 74 clock sizes, or 1 ms, or the > > - * time required to reach a stable voltage. > > - */ > > - mmc_delay(host->ios.power_delay_ms); > > + if (host->flags & MMC_UHS2_SUPPORT) > > + /* > > + * This delay should be sufficient to allow the power supply > > + * to reach the minimum voltage. > > + */ > > + /* TODO: avoid an immediate value */ > > + mmc_delay(10); > > + else > > + /* > > + * This delay must be at least 74 clock sizes, or 1 ms, or the > > + * time required to reach a stable voltage. > > + */ > > + mmc_delay(host->ios.power_delay_ms); > > } > > > > void mmc_power_off(struct mmc_host *host) > > @@ -2307,7 +2325,11 @@ void mmc_start_host(struct mmc_host *host) > > > > if (!(host->caps2 & MMC_CAP2_NO_PRESCAN_POWERUP)) { > > mmc_claim_host(host); > > - mmc_power_up(host, host->ocr_avail); > > + > > + /* Power up here will make UHS2 init ugly. */ > > + if (!(host->caps & MMC_CAP_UHS2)) > > + mmc_power_up(host, host->ocr_avail); > > + > > According to my suggestions, then this would not be needed. This should not be needed. Thank you. > > > mmc_release_host(host); > > } > > > > diff --git a/drivers/mmc/core/regulator.c b/drivers/mmc/core/regulator.c > > index 96b1d15045d6..05556225d9ac 100644 > > --- a/drivers/mmc/core/regulator.c > > +++ b/drivers/mmc/core/regulator.c > > @@ -247,6 +247,7 @@ int mmc_regulator_get_supply(struct mmc_host *mmc) > > > > mmc->supply.vmmc = devm_regulator_get_optional(dev, "vmmc"); > > mmc->supply.vqmmc = devm_regulator_get_optional(dev, "vqmmc"); > > + mmc->supply.vmmc2 = devm_regulator_get_optional(dev, "vmmc2"); > > Please move the regulator thingy here into a separate patch. Please > make sure corresponding header file, adding the vmmc2 to it is part of > that change as well. Yes. will do it. > > > > > if (IS_ERR(mmc->supply.vmmc)) { > > if (PTR_ERR(mmc->supply.vmmc) == -EPROBE_DEFER) > > @@ -266,6 +267,19 @@ int mmc_regulator_get_supply(struct mmc_host *mmc) > > dev_dbg(dev, "No vqmmc regulator found\n"); > > } > > > > + if (IS_ERR(mmc->supply.vmmc2)) { > > + if (PTR_ERR(mmc->supply.vmmc2) == -EPROBE_DEFER) > > + return -EPROBE_DEFER; > > + dev_dbg(dev, "No vmmc2 regulator found\n"); > > + } else { > > + ret = mmc_regulator_get_ocrmask(mmc->supply.vmmc2); > > + if (ret > 0) > > + mmc->ocr_avail_uhs2 = ret; > > + else > > + dev_warn(dev, "Failed getting UHS2 OCR mask: %d\n", > > + ret); > > + } > > + > > return 0; > > } > > EXPORT_SYMBOL_GPL(mmc_regulator_get_supply); > > -- > > 2.27.0 > > > > Kind regards > Uffe Best regards, Ben
On Fri, 24 Jul 2020 at 13:11, Ben Chuang <benchuanggli@gmail.com> wrote: > > Hi Ulf, > > On Fri, Jul 17, 2020 at 7:26 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > On Fri, 10 Jul 2020 at 13:07, Ben Chuang <benchuanggli@gmail.com> wrote: > > > > > > From: AKASHI Takahiro <takahiro.akashi@linaro.org> > > > > > > According to Fig. 3-35 in "SD Host Controller Simplified Spec. Ver4.20": > > > - Prepare vdd1, vdd2 and ios.timing for using after/in step (2) > > > - chip_select is not used in UHS-II, used to return to the legacy flow > > > > Thanks for pointing to the spec, but please explain why/what/how for > > the change - as this helps me to review. > > > > I am going to stop commenting on each patch's commit message, beyond > > this patch - as it seems the same comment applies to more patches. > > > > > > > > Signed-off-by: Ben Chuang <ben.chuang@genesyslogic.com.tw> > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > > --- > > > drivers/mmc/core/core.c | 62 ++++++++++++++++++++++++------------ > > > drivers/mmc/core/regulator.c | 14 ++++++++ > > > 2 files changed, 56 insertions(+), 20 deletions(-) > > > > > > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c > > > index 8d2b808e9b58..85c83c82ad0c 100644 > > > --- a/drivers/mmc/core/core.c > > > +++ b/drivers/mmc/core/core.c > > > @@ -1315,33 +1315,51 @@ void mmc_power_up(struct mmc_host *host, u32 ocr) > > > if (host->ios.power_mode == MMC_POWER_ON) > > > return; > > > > > > - mmc_pwrseq_pre_power_on(host); > > > + if (host->flags & MMC_UHS2_SUPPORT) { > > > + /* TODO: handle 'ocr' parameter */ > > > + host->ios.vdd = fls(host->ocr_avail) - 1; > > > + host->ios.vdd2 = fls(host->ocr_avail_uhs2) - 1; > > > + if (mmc_host_is_spi(host)) > > > + host->ios.chip_select = MMC_CS_HIGH; > > > + else > > > + host->ios.chip_select = MMC_CS_DONTCARE; > > > + host->ios.timing = MMC_TIMING_UHS2; > > > > If I understand correctly, the intent is to always try to initialize > > the UHS-II interface/phy if that is supported. That doesn't seem > > correct to me. What about if the SD card doesn't support UHS-II, then > > we should use the legacy SD interface instead right? > > Please always try UHS-II I/F first, then if UHS-II I/F fails, then > switch to SD I/F. > > > > > Or perhaps the MMC_UHS2_SUPPORT bit becomes cleared somewhere in the > > error path when first trying to initialize an UHS-II card, from > > subsequent changes? > > Yes, MMC_UHS2_SUPPORT will be cleared in some cases. > > > > > So, assuming that is the intent then, I am still not sure about this approach. > > > > What about if we instead always start with legacy SD initialization? > > When we have read the OCR register, via mmc_send_app_op_cond(), we can > > check if the card supports UHS-II by looking at the UHS-II Card Status > > (bit 29). > > UHS-II spec recommends to detect UHS-II first. > Or in Host controller spec, section 3.13.2 card interface detection sequence, > it also starts from UHS-II path, then go SD legacy path if UHS-II > initialization fails. I have carefully read the specs. While you are right, that the flow charts seem to prefer to start with UHS-II - I don't think it's a good idea. Have a look at "7.2.3.2 Interface Selection after Power Up", in the UHS-II Addendum Version 2.00. This section states this: "If Host intends to use only Legacy SD interface or detects that Legacy SD Card is inserted, it is allowed to supply only VDD1 and SDCLK, and issue CMD8 in order to accelerate initialization of Legacy SD interface. Note that once UHS-II I/F is disabled, Host requires power cycle to enable UHS-II again." That said, I am also concerned about the case when a bootloader has initialized the SD card. When the kernel tries to re-initialize the card during boot, it may fail with UHS-II - unless the legacy SD init path is tried first. > > The bit29 in response of ACMD41 is defined as “UHS-II Card Status”, > not UHS-II supported. > We have experience using this value to determine whether a card supports UHS-II, > but not every card reports if they support UHS-II by the response of > ACMD41 correctly. I see. If that is the case, I think we should invent an SD quirk for that particular card. Along the lines of what already exists for SDIO and eMMC. So, when a card with this kind of quirk is found, we should simply bail out in the SD legacy init path and try the UHS-II path instead. What card have you found missing to set the bit29? > > > > > If it turns out that the card supports UHS-II and the host does as > > well, then we do a mmc_power_off() to completely reset the > > card/host/phy. Then we can call into a UHS-II specific path, that > > tries to power on and initialize things according to the UHS-II spec. > > > > In this way, we are going to prioritize initialization of legacy SD > > cards to remain quick, as we won't try to use UHS-II unless the card > > supports it. Moreover, I get the impression that we can keep the > > existing code more as is - and instead introduce UHS-II specifics in a > > separate path. This also also for UHS-II specific optimizations, I > > think. > > Agree that we can try to keep the existing code and also need your advice/help. Sure, I will help the best I can. I will have a look at the next patch in the series as well, but it may take some time as I am currently trying to get some time off for holidays. > > > > > > + } else { > > > + mmc_pwrseq_pre_power_on(host); > > > > > > - host->ios.vdd = fls(ocr) - 1; > > > - host->ios.power_mode = MMC_POWER_UP; > > > - /* Set initial state and call mmc_set_ios */ > > > - mmc_set_initial_state(host); > > > + host->ios.vdd = fls(ocr) - 1; > > > + host->ios.power_mode = MMC_POWER_UP; > > > + /* Set initial state and call mmc_set_ios */ > > > + mmc_set_initial_state(host); > > > > > > - mmc_set_initial_signal_voltage(host); > > > + mmc_set_initial_signal_voltage(host); > > > > > > - /* > > > - * This delay should be sufficient to allow the power supply > > > - * to reach the minimum voltage. > > > - */ > > > - mmc_delay(host->ios.power_delay_ms); > > > - > > > - mmc_pwrseq_post_power_on(host); > > > + /* > > > + * This delay should be sufficient to allow the power supply > > > + * to reach the minimum voltage. > > > + */ > > > + mmc_delay(host->ios.power_delay_ms); > > > > > > + mmc_pwrseq_post_power_on(host); > > > + } > > > host->ios.clock = host->f_init; > > > - > > > host->ios.power_mode = MMC_POWER_ON; > > > + > > > mmc_set_ios(host); > > > > > > - /* > > > - * This delay must be at least 74 clock sizes, or 1 ms, or the > > > - * time required to reach a stable voltage. > > > - */ > > > - mmc_delay(host->ios.power_delay_ms); > > > + if (host->flags & MMC_UHS2_SUPPORT) > > > + /* > > > + * This delay should be sufficient to allow the power supply > > > + * to reach the minimum voltage. > > > + */ > > > + /* TODO: avoid an immediate value */ > > > + mmc_delay(10); > > > + else > > > + /* > > > + * This delay must be at least 74 clock sizes, or 1 ms, or the > > > + * time required to reach a stable voltage. > > > + */ > > > + mmc_delay(host->ios.power_delay_ms); > > > } > > > > > > void mmc_power_off(struct mmc_host *host) > > > @@ -2307,7 +2325,11 @@ void mmc_start_host(struct mmc_host *host) > > > > > > if (!(host->caps2 & MMC_CAP2_NO_PRESCAN_POWERUP)) { > > > mmc_claim_host(host); > > > - mmc_power_up(host, host->ocr_avail); > > > + > > > + /* Power up here will make UHS2 init ugly. */ > > > + if (!(host->caps & MMC_CAP_UHS2)) > > > + mmc_power_up(host, host->ocr_avail); > > > + > > > > According to my suggestions, then this would not be needed. > > This should not be needed. Thank you. > > > > > > mmc_release_host(host); > > > } > > > > > > diff --git a/drivers/mmc/core/regulator.c b/drivers/mmc/core/regulator.c > > > index 96b1d15045d6..05556225d9ac 100644 > > > --- a/drivers/mmc/core/regulator.c > > > +++ b/drivers/mmc/core/regulator.c > > > @@ -247,6 +247,7 @@ int mmc_regulator_get_supply(struct mmc_host *mmc) > > > > > > mmc->supply.vmmc = devm_regulator_get_optional(dev, "vmmc"); > > > mmc->supply.vqmmc = devm_regulator_get_optional(dev, "vqmmc"); > > > + mmc->supply.vmmc2 = devm_regulator_get_optional(dev, "vmmc2"); > > > > Please move the regulator thingy here into a separate patch. Please > > make sure corresponding header file, adding the vmmc2 to it is part of > > that change as well. > > Yes. will do it. > > > > > > > > > if (IS_ERR(mmc->supply.vmmc)) { > > > if (PTR_ERR(mmc->supply.vmmc) == -EPROBE_DEFER) > > > @@ -266,6 +267,19 @@ int mmc_regulator_get_supply(struct mmc_host *mmc) > > > dev_dbg(dev, "No vqmmc regulator found\n"); > > > } > > > > > > + if (IS_ERR(mmc->supply.vmmc2)) { > > > + if (PTR_ERR(mmc->supply.vmmc2) == -EPROBE_DEFER) > > > + return -EPROBE_DEFER; > > > + dev_dbg(dev, "No vmmc2 regulator found\n"); > > > + } else { > > > + ret = mmc_regulator_get_ocrmask(mmc->supply.vmmc2); > > > + if (ret > 0) > > > + mmc->ocr_avail_uhs2 = ret; > > > + else > > > + dev_warn(dev, "Failed getting UHS2 OCR mask: %d\n", > > > + ret); > > > + } > > > + > > > return 0; > > > } > > > EXPORT_SYMBOL_GPL(mmc_regulator_get_supply); > > > -- > > > 2.27.0 > > > Kind regards Uffe
On Fri, Jul 24, 2020 at 8:36 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > On Fri, 24 Jul 2020 at 13:11, Ben Chuang <benchuanggli@gmail.com> wrote: > > > > Hi Ulf, > > > > On Fri, Jul 17, 2020 at 7:26 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > > > On Fri, 10 Jul 2020 at 13:07, Ben Chuang <benchuanggli@gmail.com> wrote: > > > > > > > > From: AKASHI Takahiro <takahiro.akashi@linaro.org> > > > > > > > > According to Fig. 3-35 in "SD Host Controller Simplified Spec. Ver4.20": > > > > - Prepare vdd1, vdd2 and ios.timing for using after/in step (2) > > > > - chip_select is not used in UHS-II, used to return to the legacy flow > > > > > > Thanks for pointing to the spec, but please explain why/what/how for > > > the change - as this helps me to review. > > > > > > I am going to stop commenting on each patch's commit message, beyond > > > this patch - as it seems the same comment applies to more patches. > > > > > > > > > > > Signed-off-by: Ben Chuang <ben.chuang@genesyslogic.com.tw> > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > > > --- > > > > drivers/mmc/core/core.c | 62 ++++++++++++++++++++++++------------ > > > > drivers/mmc/core/regulator.c | 14 ++++++++ > > > > 2 files changed, 56 insertions(+), 20 deletions(-) > > > > > > > > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c > > > > index 8d2b808e9b58..85c83c82ad0c 100644 > > > > --- a/drivers/mmc/core/core.c > > > > +++ b/drivers/mmc/core/core.c > > > > @@ -1315,33 +1315,51 @@ void mmc_power_up(struct mmc_host *host, u32 ocr) > > > > if (host->ios.power_mode == MMC_POWER_ON) > > > > return; > > > > > > > > - mmc_pwrseq_pre_power_on(host); > > > > + if (host->flags & MMC_UHS2_SUPPORT) { > > > > + /* TODO: handle 'ocr' parameter */ > > > > + host->ios.vdd = fls(host->ocr_avail) - 1; > > > > + host->ios.vdd2 = fls(host->ocr_avail_uhs2) - 1; > > > > + if (mmc_host_is_spi(host)) > > > > + host->ios.chip_select = MMC_CS_HIGH; > > > > + else > > > > + host->ios.chip_select = MMC_CS_DONTCARE; > > > > + host->ios.timing = MMC_TIMING_UHS2; > > > > > > If I understand correctly, the intent is to always try to initialize > > > the UHS-II interface/phy if that is supported. That doesn't seem > > > correct to me. What about if the SD card doesn't support UHS-II, then > > > we should use the legacy SD interface instead right? > > > > Please always try UHS-II I/F first, then if UHS-II I/F fails, then > > switch to SD I/F. > > > > > > > > Or perhaps the MMC_UHS2_SUPPORT bit becomes cleared somewhere in the > > > error path when first trying to initialize an UHS-II card, from > > > subsequent changes? > > > > Yes, MMC_UHS2_SUPPORT will be cleared in some cases. > > > > > > > > So, assuming that is the intent then, I am still not sure about this approach. > > > > > > What about if we instead always start with legacy SD initialization? > > > When we have read the OCR register, via mmc_send_app_op_cond(), we can > > > check if the card supports UHS-II by looking at the UHS-II Card Status > > > (bit 29). > > > > UHS-II spec recommends to detect UHS-II first. > > Or in Host controller spec, section 3.13.2 card interface detection sequence, > > it also starts from UHS-II path, then go SD legacy path if UHS-II > > initialization fails. > > I have carefully read the specs. While you are right, that the flow > charts seem to prefer to start with UHS-II - I don't think it's a good > idea. > > Have a look at "7.2.3.2 Interface Selection after Power Up", in the > UHS-II Addendum Version 2.00. This section states this: > > "If Host intends to use only Legacy SD interface or detects that > Legacy SD Card is inserted, it is allowed to supply only VDD1 and > SDCLK, and issue CMD8 in order to accelerate initialization of Legacy > SD interface. Note that once UHS-II I/F is disabled, Host requires > power cycle to enable UHS-II again." > > That said, I am also concerned about the case when a bootloader has > initialized the SD card. When the kernel tries to re-initialize the > card during boot, it may fail with UHS-II - unless the legacy SD init > path is tried first. > > > > > The bit29 in response of ACMD41 is defined as “UHS-II Card Status”, > > not UHS-II supported. > > We have experience using this value to determine whether a card supports UHS-II, > > but not every card reports if they support UHS-II by the response of > > ACMD41 correctly. > > I see. > > If that is the case, I think we should invent an SD quirk for that > particular card. Along the lines of what already exists for SDIO and > eMMC. > > So, when a card with this kind of quirk is found, we should simply > bail out in the SD legacy init path and try the UHS-II path instead. > > What card have you found missing to set the bit29? In my hand, two uhs-ii cards, one is a Lexar card and another is an ADATA card. > > > > > > > > > If it turns out that the card supports UHS-II and the host does as > > > well, then we do a mmc_power_off() to completely reset the > > > card/host/phy. Then we can call into a UHS-II specific path, that > > > tries to power on and initialize things according to the UHS-II spec. > > > > > > In this way, we are going to prioritize initialization of legacy SD > > > cards to remain quick, as we won't try to use UHS-II unless the card > > > supports it. Moreover, I get the impression that we can keep the > > > existing code more as is - and instead introduce UHS-II specifics in a > > > separate path. This also also for UHS-II specific optimizations, I > > > think. > > > > Agree that we can try to keep the existing code and also need your advice/help. > > Sure, I will help the best I can. > > I will have a look at the next patch in the series as well, but it may > take some time as I am currently trying to get some time off for > holidays. > > > > > > > > > > + } else { > > > > + mmc_pwrseq_pre_power_on(host); > > > > > > > > - host->ios.vdd = fls(ocr) - 1; > > > > - host->ios.power_mode = MMC_POWER_UP; > > > > - /* Set initial state and call mmc_set_ios */ > > > > - mmc_set_initial_state(host); > > > > + host->ios.vdd = fls(ocr) - 1; > > > > + host->ios.power_mode = MMC_POWER_UP; > > > > + /* Set initial state and call mmc_set_ios */ > > > > + mmc_set_initial_state(host); > > > > > > > > - mmc_set_initial_signal_voltage(host); > > > > + mmc_set_initial_signal_voltage(host); > > > > > > > > - /* > > > > - * This delay should be sufficient to allow the power supply > > > > - * to reach the minimum voltage. > > > > - */ > > > > - mmc_delay(host->ios.power_delay_ms); > > > > - > > > > - mmc_pwrseq_post_power_on(host); > > > > + /* > > > > + * This delay should be sufficient to allow the power supply > > > > + * to reach the minimum voltage. > > > > + */ > > > > + mmc_delay(host->ios.power_delay_ms); > > > > > > > > + mmc_pwrseq_post_power_on(host); > > > > + } > > > > host->ios.clock = host->f_init; > > > > - > > > > host->ios.power_mode = MMC_POWER_ON; > > > > + > > > > mmc_set_ios(host); > > > > > > > > - /* > > > > - * This delay must be at least 74 clock sizes, or 1 ms, or the > > > > - * time required to reach a stable voltage. > > > > - */ > > > > - mmc_delay(host->ios.power_delay_ms); > > > > + if (host->flags & MMC_UHS2_SUPPORT) > > > > + /* > > > > + * This delay should be sufficient to allow the power supply > > > > + * to reach the minimum voltage. > > > > + */ > > > > + /* TODO: avoid an immediate value */ > > > > + mmc_delay(10); > > > > + else > > > > + /* > > > > + * This delay must be at least 74 clock sizes, or 1 ms, or the > > > > + * time required to reach a stable voltage. > > > > + */ > > > > + mmc_delay(host->ios.power_delay_ms); > > > > } > > > > > > > > void mmc_power_off(struct mmc_host *host) > > > > @@ -2307,7 +2325,11 @@ void mmc_start_host(struct mmc_host *host) > > > > > > > > if (!(host->caps2 & MMC_CAP2_NO_PRESCAN_POWERUP)) { > > > > mmc_claim_host(host); > > > > - mmc_power_up(host, host->ocr_avail); > > > > + > > > > + /* Power up here will make UHS2 init ugly. */ > > > > + if (!(host->caps & MMC_CAP_UHS2)) > > > > + mmc_power_up(host, host->ocr_avail); > > > > + > > > > > > According to my suggestions, then this would not be needed. > > > > This should not be needed. Thank you. > > > > > > > > > mmc_release_host(host); > > > > } > > > > > > > > diff --git a/drivers/mmc/core/regulator.c b/drivers/mmc/core/regulator.c > > > > index 96b1d15045d6..05556225d9ac 100644 > > > > --- a/drivers/mmc/core/regulator.c > > > > +++ b/drivers/mmc/core/regulator.c > > > > @@ -247,6 +247,7 @@ int mmc_regulator_get_supply(struct mmc_host *mmc) > > > > > > > > mmc->supply.vmmc = devm_regulator_get_optional(dev, "vmmc"); > > > > mmc->supply.vqmmc = devm_regulator_get_optional(dev, "vqmmc"); > > > > + mmc->supply.vmmc2 = devm_regulator_get_optional(dev, "vmmc2"); > > > > > > Please move the regulator thingy here into a separate patch. Please > > > make sure corresponding header file, adding the vmmc2 to it is part of > > > that change as well. > > > > Yes. will do it. > > > > > > > > > > > > > if (IS_ERR(mmc->supply.vmmc)) { > > > > if (PTR_ERR(mmc->supply.vmmc) == -EPROBE_DEFER) > > > > @@ -266,6 +267,19 @@ int mmc_regulator_get_supply(struct mmc_host *mmc) > > > > dev_dbg(dev, "No vqmmc regulator found\n"); > > > > } > > > > > > > > + if (IS_ERR(mmc->supply.vmmc2)) { > > > > + if (PTR_ERR(mmc->supply.vmmc2) == -EPROBE_DEFER) > > > > + return -EPROBE_DEFER; > > > > + dev_dbg(dev, "No vmmc2 regulator found\n"); > > > > + } else { > > > > + ret = mmc_regulator_get_ocrmask(mmc->supply.vmmc2); > > > > + if (ret > 0) > > > > + mmc->ocr_avail_uhs2 = ret; > > > > + else > > > > + dev_warn(dev, "Failed getting UHS2 OCR mask: %d\n", > > > > + ret); > > > > + } > > > > + > > > > return 0; > > > > } > > > > EXPORT_SYMBOL_GPL(mmc_regulator_get_supply); > > > > -- > > > > 2.27.0 > > > > > > Kind regards > Uffe
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c index 8d2b808e9b58..85c83c82ad0c 100644 --- a/drivers/mmc/core/core.c +++ b/drivers/mmc/core/core.c @@ -1315,33 +1315,51 @@ void mmc_power_up(struct mmc_host *host, u32 ocr) if (host->ios.power_mode == MMC_POWER_ON) return; - mmc_pwrseq_pre_power_on(host); + if (host->flags & MMC_UHS2_SUPPORT) { + /* TODO: handle 'ocr' parameter */ + host->ios.vdd = fls(host->ocr_avail) - 1; + host->ios.vdd2 = fls(host->ocr_avail_uhs2) - 1; + if (mmc_host_is_spi(host)) + host->ios.chip_select = MMC_CS_HIGH; + else + host->ios.chip_select = MMC_CS_DONTCARE; + host->ios.timing = MMC_TIMING_UHS2; + } else { + mmc_pwrseq_pre_power_on(host); - host->ios.vdd = fls(ocr) - 1; - host->ios.power_mode = MMC_POWER_UP; - /* Set initial state and call mmc_set_ios */ - mmc_set_initial_state(host); + host->ios.vdd = fls(ocr) - 1; + host->ios.power_mode = MMC_POWER_UP; + /* Set initial state and call mmc_set_ios */ + mmc_set_initial_state(host); - mmc_set_initial_signal_voltage(host); + mmc_set_initial_signal_voltage(host); - /* - * This delay should be sufficient to allow the power supply - * to reach the minimum voltage. - */ - mmc_delay(host->ios.power_delay_ms); - - mmc_pwrseq_post_power_on(host); + /* + * This delay should be sufficient to allow the power supply + * to reach the minimum voltage. + */ + mmc_delay(host->ios.power_delay_ms); + mmc_pwrseq_post_power_on(host); + } host->ios.clock = host->f_init; - host->ios.power_mode = MMC_POWER_ON; + mmc_set_ios(host); - /* - * This delay must be at least 74 clock sizes, or 1 ms, or the - * time required to reach a stable voltage. - */ - mmc_delay(host->ios.power_delay_ms); + if (host->flags & MMC_UHS2_SUPPORT) + /* + * This delay should be sufficient to allow the power supply + * to reach the minimum voltage. + */ + /* TODO: avoid an immediate value */ + mmc_delay(10); + else + /* + * This delay must be at least 74 clock sizes, or 1 ms, or the + * time required to reach a stable voltage. + */ + mmc_delay(host->ios.power_delay_ms); } void mmc_power_off(struct mmc_host *host) @@ -2307,7 +2325,11 @@ void mmc_start_host(struct mmc_host *host) if (!(host->caps2 & MMC_CAP2_NO_PRESCAN_POWERUP)) { mmc_claim_host(host); - mmc_power_up(host, host->ocr_avail); + + /* Power up here will make UHS2 init ugly. */ + if (!(host->caps & MMC_CAP_UHS2)) + mmc_power_up(host, host->ocr_avail); + mmc_release_host(host); } diff --git a/drivers/mmc/core/regulator.c b/drivers/mmc/core/regulator.c index 96b1d15045d6..05556225d9ac 100644 --- a/drivers/mmc/core/regulator.c +++ b/drivers/mmc/core/regulator.c @@ -247,6 +247,7 @@ int mmc_regulator_get_supply(struct mmc_host *mmc) mmc->supply.vmmc = devm_regulator_get_optional(dev, "vmmc"); mmc->supply.vqmmc = devm_regulator_get_optional(dev, "vqmmc"); + mmc->supply.vmmc2 = devm_regulator_get_optional(dev, "vmmc2"); if (IS_ERR(mmc->supply.vmmc)) { if (PTR_ERR(mmc->supply.vmmc) == -EPROBE_DEFER) @@ -266,6 +267,19 @@ int mmc_regulator_get_supply(struct mmc_host *mmc) dev_dbg(dev, "No vqmmc regulator found\n"); } + if (IS_ERR(mmc->supply.vmmc2)) { + if (PTR_ERR(mmc->supply.vmmc2) == -EPROBE_DEFER) + return -EPROBE_DEFER; + dev_dbg(dev, "No vmmc2 regulator found\n"); + } else { + ret = mmc_regulator_get_ocrmask(mmc->supply.vmmc2); + if (ret > 0) + mmc->ocr_avail_uhs2 = ret; + else + dev_warn(dev, "Failed getting UHS2 OCR mask: %d\n", + ret); + } + return 0; } EXPORT_SYMBOL_GPL(mmc_regulator_get_supply);