On Fri, 10 Jul 2020 at 13:07, Ben Chuang <benchuanggli@xxxxxxxxx> wrote: > > From: AKASHI Takahiro <takahiro.akashi@xxxxxxxxxx> > > 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@xxxxxxxxxxxxxxxxxxx> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@xxxxxxxxxx> > --- > 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