* [RFC PATCH V3 02/21] mmc: core: UHS-II support, modify power-up sequence
@ 2020-07-10 11:08 Ben Chuang
2020-07-17 11:26 ` Ulf Hansson
0 siblings, 1 reply; 5+ messages in thread
From: Ben Chuang @ 2020-07-10 11:08 UTC (permalink / raw)
To: adrian.hunter, ulf.hansson
Cc: linux-mmc, linux-kernel, ben.chuang, takahiro.akashi, greg.tu,
Ben Chuang
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
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;
+ } 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);
--
2.27.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [RFC PATCH V3 02/21] mmc: core: UHS-II support, modify power-up sequence
2020-07-10 11:08 [RFC PATCH V3 02/21] mmc: core: UHS-II support, modify power-up sequence Ben Chuang
@ 2020-07-17 11:26 ` Ulf Hansson
2020-07-24 11:11 ` Ben Chuang
0 siblings, 1 reply; 5+ messages in thread
From: Ulf Hansson @ 2020-07-17 11:26 UTC (permalink / raw)
To: Ben Chuang
Cc: Adrian Hunter, linux-mmc, Linux Kernel Mailing List, Ben Chuang,
Takahiro Akashi, greg.tu
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFC PATCH V3 02/21] mmc: core: UHS-II support, modify power-up sequence
2020-07-17 11:26 ` Ulf Hansson
@ 2020-07-24 11:11 ` Ben Chuang
2020-07-24 12:35 ` Ulf Hansson
0 siblings, 1 reply; 5+ messages in thread
From: Ben Chuang @ 2020-07-24 11:11 UTC (permalink / raw)
To: Ulf Hansson
Cc: Adrian Hunter, linux-mmc, Linux Kernel Mailing List, Ben Chuang,
Takahiro Akashi, greg.tu
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFC PATCH V3 02/21] mmc: core: UHS-II support, modify power-up sequence
2020-07-24 11:11 ` Ben Chuang
@ 2020-07-24 12:35 ` Ulf Hansson
2020-08-21 9:18 ` Ben Chuang
0 siblings, 1 reply; 5+ messages in thread
From: Ulf Hansson @ 2020-07-24 12:35 UTC (permalink / raw)
To: Ben Chuang
Cc: Adrian Hunter, linux-mmc, Linux Kernel Mailing List, Ben Chuang,
Takahiro Akashi, greg.tu
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFC PATCH V3 02/21] mmc: core: UHS-II support, modify power-up sequence
2020-07-24 12:35 ` Ulf Hansson
@ 2020-08-21 9:18 ` Ben Chuang
0 siblings, 0 replies; 5+ messages in thread
From: Ben Chuang @ 2020-08-21 9:18 UTC (permalink / raw)
To: Ulf Hansson
Cc: Adrian Hunter, linux-mmc, Linux Kernel Mailing List, Ben Chuang,
Takahiro Akashi, greg.tu
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-08-21 9:19 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-10 11:08 [RFC PATCH V3 02/21] mmc: core: UHS-II support, modify power-up sequence Ben Chuang
2020-07-17 11:26 ` Ulf Hansson
2020-07-24 11:11 ` Ben Chuang
2020-07-24 12:35 ` Ulf Hansson
2020-08-21 9:18 ` Ben Chuang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).