On Fri, Jul 24, 2020 at 8:36 PM Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote:
>
> On Fri, 24 Jul 2020 at 13:11, Ben Chuang <benchuanggli@xxxxxxxxx> wrote:
> >
> > Hi Ulf,
> >
> > On Fri, Jul 17, 2020 at 7:26 PM Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote:
> > >
> > > 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?
> >
> > 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
