On Wed, 1 Apr 2020 at 21:57, Marek Vasut <marex@xxxxxxx> wrote:
>
> Patch all drivers and core code which uses mmc_set_signal_voltage()
> and prepare it for the fact that mmc_set_signal_voltage() can return
> a value > 0, which would happen if the signal voltage switch did NOT
> happen, because the voltage was already set correctly.
I am not sure why you want to change mmc_set_signal_voltage(), can you
elaborate on that?
I thought we discussed changing mmc_regulator_set_vqmmc(), what am I missing?
>
> Signed-off-by: Marek Vasut <marex@xxxxxxx>
> Cc: Alexandre Torgue <alexandre.torgue@xxxxxx>
> Cc: Linus Walleij <linus.walleij@xxxxxxxxxx>
> Cc: Ludovic Barre <ludovic.barre@xxxxxx>
> Cc: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
> Cc: Maxime Coquelin <mcoquelin.stm32@xxxxxxxxx>
> Cc: Patrice Chotard <patrice.chotard@xxxxxx>
> Cc: Patrick Delaunay <patrick.delaunay@xxxxxx>
> Cc: Russell King <linux@xxxxxxxxxxxxxxx>
> Cc: Ulf Hansson <ulf.hansson@xxxxxxxxxx>
> Cc: linux-stm32@xxxxxxxxxxxxxxxxxxxxxxxxxxxx
> To: linux-mmc@xxxxxxxxxxxxxxx
> ---
> drivers/mmc/core/core.c | 10 +++++-----
> drivers/mmc/core/mmc.c | 16 ++++++++--------
> drivers/mmc/host/dw_mmc-k3.c | 2 +-
> drivers/mmc/host/dw_mmc.c | 3 +--
> drivers/mmc/host/mtk-sd.c | 2 +-
> drivers/mmc/host/renesas_sdhi_core.c | 2 +-
> drivers/mmc/host/sdhci-sprd.c | 2 +-
> drivers/mmc/host/sdhci.c | 6 +++---
> 8 files changed, 21 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 4c5de6d37ac7..98a3552205cb 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -1142,7 +1142,7 @@ int mmc_set_signal_voltage(struct mmc_host *host, int signal_voltage)
> if (host->ops->start_signal_voltage_switch)
> err = host->ops->start_signal_voltage_switch(host, &host->ios);
>
> - if (err)
> + if (err < 0)
> host->ios.signal_voltage = old_signal_voltage;
>
> return err;
> @@ -1152,11 +1152,11 @@ int mmc_set_signal_voltage(struct mmc_host *host, int signal_voltage)
> void mmc_set_initial_signal_voltage(struct mmc_host *host)
> {
> /* Try to set signal voltage to 3.3V but fall back to 1.8v or 1.2v */
> - if (!mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_330))
> + if (mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_330) >= 0)
> dev_dbg(mmc_dev(host), "Initial signal voltage of 3.3v\n");
> - else if (!mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_180))
> + else if (mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_180) >= 0)
> dev_dbg(mmc_dev(host), "Initial signal voltage of 1.8v\n");
> - else if (!mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_120))
> + else if (mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_120) >= 0)
> dev_dbg(mmc_dev(host), "Initial signal voltage of 1.2v\n");
> }
>
> @@ -1172,7 +1172,7 @@ int mmc_host_set_uhs_voltage(struct mmc_host *host)
> host->ios.clock = 0;
> mmc_set_ios(host);
>
> - if (mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_180))
> + if (mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_180) < 0)
> return -EAGAIN;
>
> /* Keep clock gated for at least 10 ms, though spec only says 5 ms */
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index de94fbe629bd..9f5aae051f6d 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -1121,7 +1121,7 @@ static int mmc_select_hs_ddr(struct mmc_card *card)
> */
> if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_DDR_1_2V) {
> err = mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_120);
> - if (!err)
> + if (err >= 0)
> return 0;
> }
>
> @@ -1130,7 +1130,7 @@ static int mmc_select_hs_ddr(struct mmc_card *card)
> err = mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_180);
>
> /* make sure vccq is 3.3v after switching disaster */
> - if (err)
> + if (err < 0)
> err = mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_330);
>
> return err;
> @@ -1339,11 +1339,11 @@ static int mmc_select_hs400es(struct mmc_card *card)
> if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400_1_2V)
> err = mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_120);
>
> - if (err && card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400_1_8V)
> + if (err < 0 && card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400_1_8V)
> err = mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_180);
>
> /* If fails try again during next card power cycle */
> - if (err)
> + if (err < 0)
> goto out_err;
>
> err = mmc_select_bus_width(card);
> @@ -1437,11 +1437,11 @@ static int mmc_select_hs200(struct mmc_card *card)
> if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS200_1_2V)
> err = mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_120);
>
> - if (err && card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS200_1_8V)
> + if (err < 0 && card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS200_1_8V)
> err = mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_180);
>
> /* If fails try again during next card power cycle */
> - if (err)
> + if (err < 0)
> return err;
>
> mmc_select_driver_type(card);
> @@ -1480,7 +1480,7 @@ static int mmc_select_hs200(struct mmc_card *card)
> err:
> if (err) {
> /* fall back to the old signal voltage, if fails report error */
> - if (mmc_set_signal_voltage(host, old_signal_voltage))
> + if (mmc_set_signal_voltage(host, old_signal_voltage) < 0)
> err = -EIO;
>
> pr_err("%s: %s failed, error %d\n", mmc_hostname(card->host),
> @@ -1769,7 +1769,7 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,
> err = mmc_select_bus_width(card);
> if (err > 0 && mmc_card_hs(card)) {
> err = mmc_select_hs_ddr(card);
> - if (err)
> + if (err < 0)
> goto free_card;
> }
> }
> diff --git a/drivers/mmc/host/dw_mmc-k3.c b/drivers/mmc/host/dw_mmc-k3.c
> index 23b6f65b3785..50977ff18074 100644
> --- a/drivers/mmc/host/dw_mmc-k3.c
> +++ b/drivers/mmc/host/dw_mmc-k3.c
> @@ -424,7 +424,7 @@ static int dw_mci_hi3660_switch_voltage(struct mmc_host *mmc,
>
> if (!IS_ERR(mmc->supply.vqmmc)) {
> ret = mmc_regulator_set_vqmmc(mmc, ios);
> - if (ret) {
> + if (ret < 0) {
This change makes sense to me, however it's also a bit confusing, as
the changelog refers to changes for mmc_set_signal_voltage().
As I understand it, we want mmc_regulator_set_vqmmc() to return 1, in
case the current voltage level is the same as the requested "new"
target".
> dev_err(host->dev, "Regulator set error %d\n", ret);
> return ret;
> }
[...]
So, to conclude, it seems like $subject patch needs to be reworked a
bit - just keep the changes you have made to the host drivers, then
throw away the other parts in core.
Kind regards
Uffe
