Re: [PATCH V22 07/22] mmc: core: Support UHS-II card control and access

From: Ulf Hansson
Date: Mon Oct 07 2024 - 11:37:18 EST

On Fri, 13 Sept 2024 at 12:29, Victor Shih <victorshihgli@xxxxxxxxx> wrote:
>
> From: Victor Shih <victor.shih@xxxxxxxxxxxxxxxxxxx>
>
> Embed UHS-II access/control functionality into the MMC request
> processing flow.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@xxxxxxxxxx>
> Signed-off-by: Jason Lai <jason.lai@xxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Victor Shih <victor.shih@xxxxxxxxxxxxxxxxxxx>
> ---
>
> Updates in V16:
> - Separate the Error Recovery mechanism into a separate patch#8.
>
> Updates in V15:
> - Move struct uhs2_command uhs2_cmd to struct mmc_request and
> modify whatever other changers to make it work.
> - Refer the SD Host Controller Standard Specification Section 3.10
> to add Error Recovery mechanism to recover the command error.
>
> Updates in V13:
> - Separate __mmc_go_idle() into one patch for re-factorring the code.
> - Move mmc_decode_scr declaration to sd.h.
> - Ues uhs2_sd_tran to stead MMC_UHS2_SD_TRAN.
> - Drop unnecessary comment.
>
> Updates in V12:
> - Use mmc_op_multi() to check DCMD which supports multi read/write
> in mmc_uhs2_prepare_cmd().
>
> Updates in V10:
> - Move some definitions of PatchV9[02/23] to PatchV10[06/23].
> - Move some definitions of PatchV9[05/23] to PatchV10[06/23].
> - Drop do_multi in the mmc_blk_rw_rq_prep().
> - Use tmode_half_duplex to instead of uhs2_tmode0_flag.
> - Move entire control of the tmode into mmc_uhs2_prepare_cmd().
>
> Updates in V8:
> - Add MMC_UHS2_SUPPORT to be cleared in sd_uhs2_detect().
> - Modify return value in sd_uhs2_attach().
>
> Updates in V7:
> - Add mmc_uhs2_card_prepare_cmd helper function in sd_ops.h.
> - Drop uhs2_state in favor of ios->timing.
> - Remove unnecessary functions.
>
> ---
>
> drivers/mmc/core/core.c | 6 +-
> drivers/mmc/core/sd.c | 10 +-
> drivers/mmc/core/sd.h | 5 +
> drivers/mmc/core/sd_ops.c | 9 +
> drivers/mmc/core/sd_ops.h | 13 +
> drivers/mmc/core/sd_uhs2.c | 1100 ++++++++++++++++++++++++++++++++++--
> include/linux/mmc/core.h | 17 +
> include/linux/mmc/host.h | 15 +
> 8 files changed, 1136 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 8b8aff131f8b..1841a2190571 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -351,6 +351,8 @@ int mmc_start_request(struct mmc_host *host, struct mmc_request *mrq)
> if (err)
> return err;
>
> + mmc_uhs2_card_prepare_cmd(host, mrq);
> +
> led_trigger_event(host->led, LED_FULL);
> __mmc_start_request(host, mrq);
>
> @@ -450,6 +452,8 @@ int mmc_cqe_start_req(struct mmc_host *host, struct mmc_request *mrq)
> if (err)
> goto out_err;
>
> + mmc_uhs2_card_prepare_cmd(host, mrq);
> +
> err = host->cqe_ops->cqe_request(host, mrq);
> if (err)
> goto out_err;
> @@ -1132,7 +1136,7 @@ u32 mmc_select_voltage(struct mmc_host *host, u32 ocr)
> return 0;
> }
>
> - if (host->caps2 & MMC_CAP2_FULL_PWR_CYCLE) {
> + if (!mmc_card_uhs2(host) && host->caps2 & MMC_CAP2_FULL_PWR_CYCLE) {
> bit = ffs(ocr) - 1;
> ocr &= 3 << bit;
> mmc_power_cycle(host, ocr);
> diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
> index 12fe282bea77..d5576a2f4435 100644
> --- a/drivers/mmc/core/sd.c
> +++ b/drivers/mmc/core/sd.c
> @@ -194,7 +194,7 @@ static int mmc_decode_csd(struct mmc_card *card)
> /*
> * Given a 64-bit response, decode to our card SCR structure.
> */
> -static int mmc_decode_scr(struct mmc_card *card)
> +int mmc_decode_scr(struct mmc_card *card)
> {
> struct sd_scr *scr = &card->scr;
> unsigned int scr_struct;
> @@ -894,7 +894,7 @@ int mmc_sd_get_csd(struct mmc_card *card)
> return 0;
> }
>
> -static int mmc_sd_get_ro(struct mmc_host *host)
> +int mmc_sd_get_ro(struct mmc_host *host)
> {
> int ro;
>
> @@ -1623,7 +1623,7 @@ static void mmc_sd_detect(struct mmc_host *host)
> }
> }
>
> -static int sd_can_poweroff_notify(struct mmc_card *card)
> +int sd_can_poweroff_notify(struct mmc_card *card)
> {
> return card->ext_power.feature_support & SD_EXT_POWER_OFF_NOTIFY;
> }
> @@ -1651,7 +1651,7 @@ static int sd_busy_poweroff_notify_cb(void *cb_data, bool *busy)
> return 0;
> }
>
> -static int sd_poweroff_notify(struct mmc_card *card)
> +int sd_poweroff_notify(struct mmc_card *card)
> {
> struct sd_busy_data cb_data;
> u8 *reg_buf;
> @@ -1699,7 +1699,7 @@ static int _mmc_sd_suspend(struct mmc_host *host)
> if (mmc_card_suspended(card))
> goto out;
>
> - if (sd_can_poweroff_notify(card))
> + if (mmc_sd_can_poweroff_notify(card))

There is no need to change this. Please stick with sd_can_poweroff_notify().

> err = sd_poweroff_notify(card);
> else if (!mmc_host_is_spi(host))
> err = mmc_deselect_cards(host);
> diff --git a/drivers/mmc/core/sd.h b/drivers/mmc/core/sd.h
> index fe6dd46927a4..49afaef269cd 100644
> --- a/drivers/mmc/core/sd.h
> +++ b/drivers/mmc/core/sd.h
> @@ -11,10 +11,15 @@ struct mmc_card;
>
> int mmc_sd_get_cid(struct mmc_host *host, u32 ocr, u32 *cid, u32 *rocr);
> int mmc_sd_get_csd(struct mmc_card *card);
> +int mmc_decode_scr(struct mmc_card *card);
> +int mmc_sd_get_ro(struct mmc_host *host);
> void mmc_decode_cid(struct mmc_card *card);
> int mmc_sd_setup_card(struct mmc_host *host, struct mmc_card *card,
> bool reinit);
> unsigned mmc_sd_get_max_clock(struct mmc_card *card);
> int mmc_sd_switch_hs(struct mmc_card *card);
>
> +int sd_can_poweroff_notify(struct mmc_card *card);
> +int sd_poweroff_notify(struct mmc_card *card);
> +
> #endif
> diff --git a/drivers/mmc/core/sd_ops.c b/drivers/mmc/core/sd_ops.c
> index f93c392040ae..5cd1216edf0d 100644
> --- a/drivers/mmc/core/sd_ops.c
> +++ b/drivers/mmc/core/sd_ops.c
> @@ -41,6 +41,15 @@ int mmc_app_cmd(struct mmc_host *host, struct mmc_card *card)
> if (WARN_ON(card && card->host != host))
> return -EINVAL;
>
> + /*
> + * UHS2 packet has APP bit so only set APP_CMD flag here.
> + * Will set the APP bit when assembling UHS2 packet.
> + */
> + if (host->uhs2_sd_tran) {
> + host->uhs2_app_cmd = true;
> + return 0;
> + }
> +
> cmd.opcode = MMC_APP_CMD;
>
> if (card) {
> diff --git a/drivers/mmc/core/sd_ops.h b/drivers/mmc/core/sd_ops.h
> index 7667fc223b74..e3af68a52de8 100644
> --- a/drivers/mmc/core/sd_ops.h
> +++ b/drivers/mmc/core/sd_ops.h
> @@ -11,6 +11,7 @@
> #include <linux/types.h>
>
> struct mmc_card;
> +struct mmc_command;
> struct mmc_host;
>
> int mmc_app_set_bus_width(struct mmc_card *card, int width);
> @@ -21,6 +22,18 @@ int mmc_send_relative_addr(struct mmc_host *host, unsigned int *rca);
> int mmc_app_send_scr(struct mmc_card *card);
> int mmc_app_sd_status(struct mmc_card *card, void *ssr);
> int mmc_app_cmd(struct mmc_host *host, struct mmc_card *card);
> +void mmc_uhs2_prepare_cmd(struct mmc_host *host, struct mmc_request *mrq);
> +
> +static inline void mmc_uhs2_card_prepare_cmd(struct mmc_host *host, struct mmc_request *mrq)
> +{
> + if (host->uhs2_sd_tran)
> + mmc_uhs2_prepare_cmd(host, mrq);
> +}

Please remove this inline function and just check "host->uhs2_sd_tran"
in mmc_start_request() and mmc_cqe_start_req(). If "uhs2_sd_tran" is
set, then just call mmc_uhs2_prepare_cmd() directly.

> +
> +static inline int mmc_sd_can_poweroff_notify(struct mmc_card *card)
> +{
> + return card->ext_power.feature_support & SD_EXT_POWER_OFF_NOTIFY;
> +}

This function is a copy of sd_can_poweroff_notify(). Please use
sd_can_poweroff_notify() and remove mmc_sd_can_poweroff_notify().

>
> #endif
>
> diff --git a/drivers/mmc/core/sd_uhs2.c b/drivers/mmc/core/sd_uhs2.c
> index beb2541338ff..85939a2582dc 100644
> --- a/drivers/mmc/core/sd_uhs2.c
> +++ b/drivers/mmc/core/sd_uhs2.c
> @@ -1,23 +1,51 @@
> // SPDX-License-Identifier: GPL-2.0-only
> /*
> * Copyright (C) 2021 Linaro Ltd
> - *
> * Author: Ulf Hansson <ulf.hansson@xxxxxxxxxx>
> *
> + * Copyright (C) 2014 Intel Corp, All Rights Reserved.
> + * Author: Yi Sun <yi.y.sun@xxxxxxxxx>
> + *
> + * Copyright (C) 2020 Genesys Logic, Inc.
> + * Authors: Ben Chuang <ben.chuang@xxxxxxxxxxxxxxxxxxx>
> + *
> + * Copyright (C) 2020 Linaro Limited
> + * Author: AKASHI Takahiro <takahiro.akashi@xxxxxxxxxx>
> + *
> + * Copyright (C) 2022 Genesys Logic, Inc.
> + * Authors: Jason Lai <jason.lai@xxxxxxxxxxxxxxxxxxx>
> + *
> + * Copyright (C) 2023 Genesys Logic, Inc.
> + * Authors: Victor Shih <victor.shih@xxxxxxxxxxxxxxxxxxx>
> + *
> * Support for SD UHS-II cards
> */
> #include <linux/err.h>
> +#include <linux/pm_runtime.h>
>
> #include <linux/mmc/host.h>
> #include <linux/mmc/card.h>
> +#include <linux/mmc/mmc.h>
> +#include <linux/mmc/sd.h>
> +#include <linux/mmc/sd_uhs2.h>
>
> +#include "card.h"
> #include "core.h"
> #include "bus.h"
> #include "sd.h"
> +#include "sd_ops.h"
> #include "mmc_ops.h"
>
> +#define UHS2_WAIT_CFG_COMPLETE_PERIOD_US (1 * 1000)
> +#define UHS2_WAIT_CFG_COMPLETE_TIMEOUT_MS 100
> +
> static const unsigned int sd_uhs2_freqs[] = { 52000000, 26000000 };
>
> +struct sd_uhs2_wait_active_state_data {
> + struct mmc_host *host;
> + struct mmc_command *cmd;
> +};
> +
> static int sd_uhs2_power_up(struct mmc_host *host)
> {
> int err;
> @@ -42,12 +70,46 @@ static int sd_uhs2_power_off(struct mmc_host *host)
>
> host->ios.vdd = 0;
> host->ios.clock = 0;
> - host->ios.timing = MMC_TIMING_LEGACY;
> + /* Must set UHS2 timing to identify UHS2 mode */
> + host->ios.timing = MMC_TIMING_UHS2_SPEED_A;

The only code that seems to check for MMC_TIMING_UHS2_SPEED_A is
__sdhci_uhs2_set_ios() (that is added later in the series). Moreover
that code only cares about MMC_TIMING_UHS2_SPEED_A if "ios->power_mode
!= MMC_POWER_OFF".

In other words, I think it should be okay to keep MMC_TIMING_LEGACY here, right?

> host->ios.power_mode = MMC_POWER_OFF;
> + host->uhs2_sd_tran = false;
>
> return host->ops->uhs2_control(host, UHS2_SET_IOS);
> }
>

[...]

> /*
> * Allocate the data structure for the mmc_card and run the UHS-II specific
> * initialization sequence.
> */
> -static int sd_uhs2_init_card(struct mmc_host *host)
> +static int sd_uhs2_init_card(struct mmc_host *host, struct mmc_card *oldcard)
> {
> struct mmc_card *card;
> u32 node_id;
> @@ -131,9 +818,14 @@ static int sd_uhs2_init_card(struct mmc_host *host)
> if (err)
> return err;
>
> - card = mmc_alloc_card(host, &sd_type);
> - if (IS_ERR(card))
> - return PTR_ERR(card);
> + if (oldcard) {
> + card = oldcard;
> + } else {
> + card = mmc_alloc_card(host, &sd_type);
> + if (IS_ERR(card))
> + return PTR_ERR(card);
> + }
> + host->card = card;

Why do you need to assign the host->card at this point, isn't it okay
to do that later where it was originally?

Anyway, there is a more important comment a few lines below that also
relates to this.

>
> card->uhs2_config.node_id = node_id;
> card->type = MMC_TYPE_SD;
> @@ -146,18 +838,189 @@ static int sd_uhs2_init_card(struct mmc_host *host)
> if (err)
> goto err;
>
> - host->card = card;
> + /* If change speed to Range B, need to GO_DORMANT_STATE */
> + if (host->ios.timing == MMC_TIMING_UHS2_SPEED_B ||
> + host->ios.timing == MMC_TIMING_UHS2_SPEED_B_HD) {
> + err = sd_uhs2_go_dormant_state(host, node_id);
> + if (err)
> + goto err;
> + }
> +
> + host->uhs2_sd_tran = true;
> +
> return 0;
>
> err:
> - mmc_remove_card(card);
> + if (!oldcard)
> + mmc_remove_card(card);
> + else
> + sd_uhs2_remove(host);

This doesn't work. If there is an "oldcard", that's because we are
trying to re-initialize the card during a resume. If we fail to
re-initialize it we must not remove the oldcard in this path, but
rather just return/propagate an error code.

Instead, the oldcard should be removed from the mmc_rescan() work when
it realizes that the call to "host->bus_ops->detect(host)" fails (and
it must fail then).

> return err;
> }
>
> -static void sd_uhs2_remove(struct mmc_host *host)
> +/*
> + * Initialize the UHS-II card through the SD-TRAN transport layer. This enables
> + * commands/requests to be backwards compatible through the legacy SD protocol.
> + * UHS-II cards has a specific power limit specified for VDD1/VDD2, that should
> + * be set through a legacy CMD6. Note that, the power limit that becomes set,
> + * survives a soft reset through the GO_DORMANT_STATE command.
> + */
> +static int sd_uhs2_legacy_init(struct mmc_host *host, struct mmc_card *card)
> {
> - mmc_remove_card(host->card);
> - host->card = NULL;
> + int err;
> + u32 cid[4];
> + u32 ocr;
> + u32 rocr;
> + u8 *status;
> + int ro;
> +
> + /* Send CMD0 to reset SD card */
> + err = __mmc_go_idle(host);
> + if (err)
> + return err;
> +
> + mmc_delay(1);
> +
> + /* Send CMD8 to communicate SD interface operation condition */
> + err = mmc_send_if_cond(host, host->ocr_avail);
> + if (err) {
> + dev_warn(mmc_dev(host), "CMD8 error\n");
> + goto err;
> + }
> +
> + /*
> + * Probe SD card working voltage.
> + */
> + err = mmc_send_app_op_cond(host, 0, &ocr);
> + if (err)
> + goto err;
> +
> + card->ocr = ocr;
> +
> + /*
> + * Some SD cards claims an out of spec VDD voltage range. Let's treat
> + * these bits as being in-valid and especially also bit7.
> + */
> + ocr &= ~0x7FFF;
> + rocr = mmc_select_voltage(host, ocr);
> + /*
> + * Some cards have zero value of rocr in UHS-II mode. Assign host's
> + * ocr value to rocr.
> + */
> + if (!rocr)
> + rocr = host->ocr_avail;
> +
> + rocr |= (SD_OCR_CCS | SD_OCR_XPC);
> +
> + /* Wait SD power on ready */
> + ocr = rocr;
> +
> + err = mmc_send_app_op_cond(host, ocr, &rocr);
> + if (err)
> + goto err;
> +
> + err = mmc_send_cid(host, cid);
> + if (err)
> + goto err;
> +
> + memcpy(card->raw_cid, cid, sizeof(card->raw_cid));
> + mmc_decode_cid(card);
> +
> + /*
> + * For native busses: get card RCA and quit open drain mode.
> + */
> + err = mmc_send_relative_addr(host, &card->rca);
> + if (err)
> + goto err;
> +
> + err = mmc_sd_get_csd(card);
> + if (err)
> + goto err;
> +
> + /*
> + * Select card, as all following commands rely on that.
> + */
> + err = mmc_select_card(card);
> + if (err)
> + goto err;
> +
> + /*
> + * Fetch SCR from card.
> + */
> + err = mmc_app_send_scr(card);
> + if (err)
> + goto err;
> +
> + err = mmc_decode_scr(card);
> + if (err)
> + goto err;
> +
> + /*
> + * Switch to high power consumption mode.
> + * Even switch failed, sd card can still work at lower power consumption mode, but
> + * performance will be lower than high power consumption mode.
> + */
> + status = kmalloc(64, GFP_KERNEL);
> + if (!status)
> + return -ENOMEM;
> +
> + if (!(card->csd.cmdclass & CCC_SWITCH)) {
> + pr_warn("%s: card lacks mandatory switch function, performance might suffer\n",
> + mmc_hostname(card->host));
> + } else {
> + /* send CMD6 to set Maximum Power Consumption to get better performance */
> + err = mmc_sd_switch(card, 0, 3, SD4_SET_POWER_LIMIT_1_80W, status);
> + if (!err)
> + err = mmc_sd_switch(card, 1, 3, SD4_SET_POWER_LIMIT_1_80W, status);
> +
> + err = 0;
> + }
> +
> + /*
> + * Check if read-only switch is active.
> + */
> + ro = mmc_sd_get_ro(host);
> + if (ro < 0) {
> + pr_warn("%s: host does not support read-only switch, assuming write-enable\n",
> + mmc_hostname(host));
> + } else if (ro > 0) {
> + mmc_card_set_readonly(card);
> + }

Some of the above commands in sd_uhs2_legacy_init() are only needed
during the first initialization of the card. When re-initializing a
card during a resume, we can skip several of these steps as they are
simply unnecessary.

However, to validate that it's the same card that remains inserted
during re-initialization, we need to read the CID register and compare
it against the CID register that we have stored earlier in
card->raw_cid. Please have a look at mmc_sd_init_card() to see how
this works for legacy SD cards.

> +
> + /*
> + * NOTE:
> + * Should we read Externsion Register to check power notification feature here?
> + */

Yes, this is needed too.

> +
> + kfree(status);
> +
> + return 0;
> +
> +err:
> + sd_uhs2_remove(host);

Again, this doesn't work if we are re-initializing the card during a
resume. The card must only be removed if this is the first
initialization of the card - otherwise the removal needs to be managed
by the mmc_rescan() work.

> + return err;
> +}
> +
> +static int sd_uhs2_reinit(struct mmc_host *host)
> +{
> + struct mmc_card *card = host->card;
> + int err;
> +
> + sd_uhs2_power_up(host);
> + err = sd_uhs2_phy_init(host);
> + if (err)
> + return err;
> +
> + err = sd_uhs2_init_card(host, card);
> + if (err)
> + return err;
> +
> + err = sd_uhs2_legacy_init(host, card);
> + if (err)
> + return err;
> +
> + mmc_card_set_present(card);

Drop this, it's already managed when calling mmc_add_card() and that
should be sufficient.

> + return err;
> }
>
> static int sd_uhs2_alive(struct mmc_host *host)
> @@ -179,38 +1042,194 @@ static void sd_uhs2_detect(struct mmc_host *host)
> mmc_claim_host(host);
> mmc_detach_bus(host);
> sd_uhs2_power_off(host);
> + /* Remove UHS2 timing to indicate the end of UHS2 mode */
> + host->ios.timing = MMC_TIMING_LEGACY;

If my earlier comment for sd_uhs2_power_off() was correct, this part
should be dropped too.

> mmc_release_host(host);
> }
> }
>
> +static int _sd_uhs2_suspend(struct mmc_host *host)
> +{
> + struct mmc_card *card = host->card;
> + int err = 0;
> +
> + mmc_claim_host(host);
> +
> + if (mmc_card_suspended(card))
> + goto out;
> +
> + if (mmc_sd_can_poweroff_notify(card))

Use sd_can_poweroff_notify() instead.

> + err = sd_poweroff_notify(card);
> +
> + if (!err) {
> + sd_uhs2_power_off(host);
> + mmc_card_set_suspended(card);
> + }
> +
> +out:
> + mmc_release_host(host);
> + return err;
> +}
> +

[...]

>
> @@ -237,7 +1256,7 @@ static int sd_uhs2_attach(struct mmc_host *host)
> if (err)
> goto err;
>
> - err = sd_uhs2_init_card(host);
> + err = sd_uhs2_init_card(host, NULL);
> if (err)
> goto err;
>
> @@ -254,21 +1273,33 @@ static int sd_uhs2_attach(struct mmc_host *host)
> goto remove_card;
>
> mmc_claim_host(host);
> +
> return 0;
>
> remove_card:
> - mmc_remove_card(host->card);
> - host->card = NULL;
> + sd_uhs2_remove(host);
> mmc_claim_host(host);
> - mmc_detach_bus(host);
> +
> err:
> + mmc_detach_bus(host);
> sd_uhs2_power_off(host);
> + /* Remove UHS2 timing to indicate the end of UHS2 mode */
> + host->ios.timing = MMC_TIMING_LEGACY;

Again, see my earlier comment for sd_uhs2_power_off(). Can this part be dropped?

> return err;
> }
>
> +/**
> + * mmc_attach_sd_uhs2 - select UHS2 interface
> + * @host: MMC host
> + *
> + * Try to select UHS2 interface and initialize the bus for a given
> + * frequency, @freq.
> + *
> + * Return: 0 on success, non-zero error on failure
> + */
> int mmc_attach_sd_uhs2(struct mmc_host *host)
> {
> - int i, err = 0;
> + int i, err;
>
> if (!(host->caps2 & MMC_CAP2_SD_UHS2))
> return -EOPNOTSUPP;
> @@ -283,6 +1314,9 @@ int mmc_attach_sd_uhs2(struct mmc_host *host)
> */
> for (i = 0; i < ARRAY_SIZE(sd_uhs2_freqs); i++) {
> host->f_init = sd_uhs2_freqs[i];
> + pr_debug("%s: %s: trying to init UHS-II card at %u Hz\n",
> + mmc_hostname(host), __func__, host->f_init);
> +

This looks like it belongs in patch 2 instead. I have amended patch 2
with this before I applied it.

> err = sd_uhs2_attach(host);
> if (!err)
> break;

[...]

Other than the above comments, I just wanted to highlight that I have
just queued up a series from Avri that adds support for SD Ultra
Capacity (SDUC) cards.

There are some adjustments needed for your series for UHS-II to cope
with SDUC, but I think those should be quite minor.

Kind regards
Uffe