| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAPDyKFrvzZSYWkqD_JgZSCmF8p+Bj7JXfdbZ5D=OsszTPsSDdw@mail.gmail.com>
Date: Fri, 17 Jul 2020 13:26:05 +0200
From: Ulf Hansson <ulf.hansson@...aro.org>
To: Ben Chuang <benchuanggli@...il.com>
Cc: Adrian Hunter <adrian.hunter@...el.com>,
"linux-mmc@...r.kernel.org" <linux-mmc@...r.kernel.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
Ben Chuang <ben.chuang@...esyslogic.com.tw>,
Takahiro Akashi <takahiro.akashi@...aro.org>,
greg.tu@...esyslogic.com.tw
Subject: Re: [RFC PATCH V3 02/21] mmc: core: UHS-II support, modify power-up sequence
On Fri, 10 Jul 2020 at 13:07, Ben Chuang <benchuanggli@...il.com> wrote:
>
> From: AKASHI Takahiro <takahiro.akashi@...aro.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@...esyslogic.com.tw>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@...aro.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
Powered by blists - more mailing lists