by Ulf Hansson

[permalink] [raw]

Subject: Re: [PATCH V9 03/23] mmc: core: Announce successful insertion of an SD UHS-II card

On Fri, 21 Jul 2023 at 12:14, Victor Shih <[email protected]> wrote:
>
> From: Victor Shih <[email protected]>

Authorship?

>
> Update in previous version:

Please, drop the above.

> To inform the users about SD UHS-II cards, let's extend the print at card
> insertion with a "UHS-II" substring. Within this change, it seems
> reasonable to convert from using "ultra high speed" into "UHS-I speed", for
> the UHS-I type, as it should makes it more clear.
>
> Note that, the new print for UHS-II cards doesn't include the actual
> selected speed mode. Instead, this is going to be added from subsequent
> change.
>
> Signed-off-by: Ulf Hansson <[email protected]>
> Signed-off-by: Victor Shih <[email protected]>
> ---
>
> Updates in V7:
> - Drop MMC_TIMING_SD_UHS2 in favor of MMC_TIMING_UHS2_SPEED_A in
> mmc_card_uhs2 function.
>
> Updates in V4:
> - Make mmc_card_uhs2() take struct mmc_host* as in-param.
>
> ---
>
> drivers/mmc/core/bus.c | 4 +++-
> drivers/mmc/core/host.h | 7 +++++++
> 2 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/core/bus.c b/drivers/mmc/core/bus.c
> index cf32cf135781..d9a3b3d38d8b 100644
> --- a/drivers/mmc/core/bus.c
> +++ b/drivers/mmc/core/bus.c
> @@ -341,7 +341,9 @@ int mmc_add_card(struct mmc_card *card)
> if (mmc_card_hs(card))
> speed_mode = "high speed ";
> else if (mmc_card_uhs(card))
> - speed_mode = "ultra high speed ";
> + speed_mode = "UHS-I speed ";
> + else if (mmc_card_uhs2(card->host))
> + speed_mode = "UHS-II speed ";
> else if (mmc_card_ddr52(card))
> speed_mode = "high speed DDR ";
> else if (mmc_card_hs200(card))
> diff --git a/drivers/mmc/core/host.h b/drivers/mmc/core/host.h
> index 48c4952512a5..9f6e5e31dfea 100644
> --- a/drivers/mmc/core/host.h
> +++ b/drivers/mmc/core/host.h
> @@ -89,5 +89,12 @@ static inline bool mmc_card_sd_express(struct mmc_host *host)
> host->ios.timing == MMC_TIMING_SD_EXP_1_2V;
> }
>
> +static inline bool mmc_card_uhs2(struct mmc_host *host)
> +{
> + return host->ios.timing == MMC_TIMING_UHS2_SPEED_A ||
> + host->ios.timing == MMC_TIMING_UHS2_SPEED_A_HD ||
> + host->ios.timing == MMC_TIMING_UHS2_SPEED_B ||
> + host->ios.timing == MMC_TIMING_UHS2_SPEED_B_HD;
> +}
> #endif
>

Kind regards
Uffe

2023-08-08 18:55:04

by Ulf Hansson

[permalink] [raw]

Subject: Re: [PATCH V9 02/23] mmc: core: Prepare to support SD UHS-II cards

On Fri, 21 Jul 2023 at 12:14, Victor Shih <[email protected]> wrote:
>
> From: Victor Shih <[email protected]>

As I have said earlier, please don't claim authorship of patches that
you haven't autherd.

Of course, if you have made major modifications to a patch, then you
may do so, but that is certainly not the case here.

>
> Update in previous version:

Please drop the above from the commit message.

> The SD UHS-II interface was introduced to the SD spec v4.00 several years
> ago. The interface is fundamentally different from an electrical and a
> protocol point of view, comparing to the legacy SD interface.
>
> However, the legacy SD protocol is supported through a specific transport
> layer (SD-TRAN) defined in the UHS-II addendum of the spec. This allows the
> SD card to be managed in a very similar way as a legacy SD card, hence a
> lot of code can be re-used to support these new types of cards through the
> mmc subsystem.
>
> Moreover, an SD card that supports the UHS-II interface shall also be
> backwards compatible with the legacy SD interface, which allows a UHS-II
> card to be inserted into a legacy slot. As a matter of fact, this is
> already supported by mmc subsystem as of today.
>
> To prepare to add support for UHS-II, this change puts the basic foundation
> in the mmc core in place, allowing it to be more easily reviewed before
> subsequent changes implements the actual support.
>
> Basically, the approach here adds a new UHS-II bus_ops type and adds a
> separate initialization path for the UHS-II card. The intent is to avoid us
> from sprinkling the legacy initialization path, but also to simplify
> implementation of the UHS-II specific bits.
>
> At this point, there is only one new host ops added to manage the various
> ios settings needed for UHS-II. Additional host ops that are needed, are
> being added from subsequent changes.
>
> Signed-off-by: Ulf Hansson <[email protected]>
> Signed-off-by: Victor Shih <[email protected]>
> ---
>
> Updates in V9:
> - move sd_uhs2_operation definition of PatchV8[05/23] to
> PatchV9[02/23] for avoid compilation errors.
> - move uhs2_control definition of PatchV8[05/23] to
> PatchV9[02/23] for avoid compilation errors.
> - move mmc_host flags definition of PatchV8[05/23] to
> PatchV9[02/23] for avoid compilation errors.
> - move mmc_host flags MMC_UHS2_SUPPORT definition of PatchV8[05/23] to
> PatchV9[02/23] for avoid compilation errors.
> - move mmc_host flags MMC_UHS2_SD_TRAN definition of PatchV8[05/23] to
> PatchV9[02/23] for avoid compilation errors.
>
> Updates in V7:
> - Drop sd_uhs2_set_ios function.
> - Used ->uhs2_control() callback for uhs2_set_ios in sd_uhs2_power_up().
> - Used ->uhs2_control() callback for uhs2_set_ios in sd_uhs2_power_off().
> - Drop MMC_TIMING_SD_UHS2 in favor of MMC_TIMING_UHS2_SPEED_A.
> - Modify sd_uhs2_legacy_init to avoid sd_uhs2_reinit cycle issue.
>
> Updates in V4:
> - Re-based, updated a comment and removed white-space.
> - Moved MMC_VQMMC2_VOLTAGE_180 into a later patch in the series.
>
> ---
>
> drivers/mmc/core/Makefile | 2 +-
> drivers/mmc/core/core.c | 17 ++-
> drivers/mmc/core/core.h | 1 +
> drivers/mmc/core/sd_uhs2.c | 294 +++++++++++++++++++++++++++++++++++++
> include/linux/mmc/card.h | 7 +
> include/linux/mmc/host.h | 45 ++++++
> 6 files changed, 364 insertions(+), 2 deletions(-)
> create mode 100644 drivers/mmc/core/sd_uhs2.c
>
> diff --git a/drivers/mmc/core/Makefile b/drivers/mmc/core/Makefile
> index 6a907736cd7a..15b067e8b0d1 100644
> --- a/drivers/mmc/core/Makefile
> +++ b/drivers/mmc/core/Makefile
> @@ -7,7 +7,7 @@ obj-$(CONFIG_MMC) += mmc_core.o
> mmc_core-y := core.o bus.o host.o \
> mmc.o mmc_ops.o sd.o sd_ops.o \
> sdio.o sdio_ops.o sdio_bus.o \
> - sdio_cis.o sdio_io.o sdio_irq.o \
> + sdio_cis.o sdio_io.o sdio_irq.o sd_uhs2.o\
> slot-gpio.o regulator.o
> mmc_core-$(CONFIG_OF) += pwrseq.o
> obj-$(CONFIG_PWRSEQ_SIMPLE) += pwrseq_simple.o
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 3d3e0ca52614..ba8808cd9318 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -2244,6 +2244,18 @@ void mmc_rescan(struct work_struct *work)
> goto out;
> }
>
> + /*
> + * Ideally we should favor initialization of legacy SD cards and defer
> + * UHS-II enumeration. However, it seems like cards doesn't reliably
> + * announce their support for UHS-II in the response to the ACMD41,
> + * while initializing the legacy SD interface. Therefore, let's start
> + * with UHS-II for now.
> + */
> + if (!mmc_attach_sd_uhs2(host)) {
> + mmc_release_host(host);
> + goto out;
> + }
> +
> for (i = 0; i < ARRAY_SIZE(freqs); i++) {
> unsigned int freq = freqs[i];
> if (freq > host->f_max) {
> @@ -2276,10 +2288,13 @@ void mmc_rescan(struct work_struct *work)
>
> void mmc_start_host(struct mmc_host *host)
> {
> + bool power_up = !(host->caps2 &
> + (MMC_CAP2_NO_PRESCAN_POWERUP | MMC_CAP2_SD_UHS2));
> +
> host->f_init = max(min(freqs[0], host->f_max), host->f_min);
> host->rescan_disable = 0;
>
> - if (!(host->caps2 & MMC_CAP2_NO_PRESCAN_POWERUP)) {
> + if (power_up) {
> mmc_claim_host(host);
> mmc_power_up(host, host->ocr_avail);
> mmc_release_host(host);
> diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h
> index 37091a6589ed..920323faa834 100644
> --- a/drivers/mmc/core/core.h
> +++ b/drivers/mmc/core/core.h
> @@ -81,6 +81,7 @@ int mmc_detect_card_removed(struct mmc_host *host);
> int mmc_attach_mmc(struct mmc_host *host);
> int mmc_attach_sd(struct mmc_host *host);
> int mmc_attach_sdio(struct mmc_host *host);
> +int mmc_attach_sd_uhs2(struct mmc_host *host);
>
> /* Module parameters */
> extern bool use_spi_crc;
> diff --git a/drivers/mmc/core/sd_uhs2.c b/drivers/mmc/core/sd_uhs2.c
> new file mode 100644
> index 000000000000..06b2aab52b93
> --- /dev/null
> +++ b/drivers/mmc/core/sd_uhs2.c
> @@ -0,0 +1,294 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2021 Linaro Ltd
> + *
> + * Author: Ulf Hansson <[email protected]>
> + *
> + * Support for SD UHS-II cards
> + */
> +#include <linux/err.h>
> +
> +#include <linux/mmc/host.h>
> +#include <linux/mmc/card.h>
> +
> +#include "core.h"
> +#include "bus.h"
> +#include "sd.h"
> +#include "mmc_ops.h"
> +
> +static const unsigned int sd_uhs2_freqs[] = { 52000000, 26000000 };
> +
> +static int sd_uhs2_power_up(struct mmc_host *host)
> +{
> + int err;
> +
> + if (host->ios.power_mode == MMC_POWER_ON)
> + return 0;
> +
> + host->ios.vdd = fls(host->ocr_avail) - 1;
> + host->ios.clock = host->f_init;
> + host->ios.timing = MMC_TIMING_UHS2_SPEED_A;
> + host->ios.power_mode = MMC_POWER_ON;
> +
> + err = host->ops->uhs2_control(host, UHS2_SET_IOS);
> +
> + return err;
> +}
> +
> +static int sd_uhs2_power_off(struct mmc_host *host)
> +{
> + if (host->ios.power_mode == MMC_POWER_OFF)
> + return 0;
> +
> + host->ios.vdd = 0;
> + host->ios.clock = 0;
> + host->ios.timing = MMC_TIMING_LEGACY;
> + host->ios.power_mode = MMC_POWER_OFF;
> + if (host->flags & MMC_UHS2_SD_TRAN)
> + host->flags &= ~MMC_UHS2_SD_TRAN;

Let's not add this as part of $subject patch, but rather from the
patch that implements the support for SD tran.

Moreover, I am not so fond of the name "host->flags" as it is simply
too generic, but let's discuss this in the next version.

> +
> + return host->ops->uhs2_control(host, UHS2_SET_IOS);
> +}
> +
> +/*
> + * Run the phy initialization sequence, which mainly relies on the UHS-II host
> + * to check that we reach the expected electrical state, between the host and
> + * the card.
> + */
> +static int sd_uhs2_phy_init(struct mmc_host *host)
> +{
> + return 0;
> +}
> +
> +/*
> + * Do the early initialization of the card, by sending the device init broadcast
> + * command and wait for the process to be completed.
> + */
> +static int sd_uhs2_dev_init(struct mmc_host *host)
> +{
> + return 0;
> +}
> +
> +/*
> + * Run the enumeration process by sending the enumerate command to the card.
> + * Note that, we currently support only the point to point connection, which
> + * means only one card can be attached per host/slot.
> + */
> +static int sd_uhs2_enum(struct mmc_host *host, u32 *node_id)
> +{
> + return 0;
> +}
> +
> +/*
> + * Read the UHS-II configuration registers (CFG_REG) of the card, by sending it
> + * commands and by parsing the responses. Store a copy of the relevant data in
> + * card->uhs2_config.
> + */
> +static int sd_uhs2_config_read(struct mmc_host *host, struct mmc_card *card)
> +{
> + return 0;
> +}
> +
> +/*
> + * Based on the card's and host's UHS-II capabilities, let's update the
> + * configuration of the card and the host. This may also include to move to a
> + * greater speed range/mode. Depending on the updated configuration, we may need
> + * to do a soft reset of the card via sending it a GO_DORMANT_STATE command.
> + *
> + * In the final step, let's check if the card signals "config completion", which
> + * indicates that the card has moved from config state into active state.
> + */
> +static int sd_uhs2_config_write(struct mmc_host *host, struct mmc_card *card)
> +{
> + return 0;
> +}
> +
> +/*
> + * 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)
> +{
> + return 0;
> +}
> +
> +/*
> + * 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)
> +{
> + struct mmc_card *card;
> + u32 node_id;
> + int err;
> +
> + err = sd_uhs2_dev_init(host);
> + if (err)
> + return err;
> +
> + err = sd_uhs2_enum(host, &node_id);
> + if (err)
> + return err;
> +
> + card = mmc_alloc_card(host, &sd_type);
> + if (IS_ERR(card))
> + return PTR_ERR(card);
> +
> + card->uhs2_config.node_id = node_id;
> + card->type = MMC_TYPE_SD;
> +
> + err = sd_uhs2_config_read(host, card);
> + if (err)
> + goto err;
> +
> + err = sd_uhs2_config_write(host, card);
> + if (err)
> + goto err;
> +
> + host->card = card;
> + return 0;
> +
> +err:
> + mmc_remove_card(card);
> + return err;
> +}
> +
> +static void sd_uhs2_remove(struct mmc_host *host)
> +{
> + mmc_remove_card(host->card);
> + host->card = NULL;
> +}
> +
> +static int sd_uhs2_alive(struct mmc_host *host)
> +{
> + return mmc_send_status(host->card, NULL);
> +}
> +
> +static void sd_uhs2_detect(struct mmc_host *host)
> +{
> + int err;
> +
> + mmc_get_card(host->card, NULL);
> + err = _mmc_detect_card_removed(host);
> + mmc_put_card(host->card, NULL);
> +
> + if (err) {
> + sd_uhs2_remove(host);
> +
> + mmc_claim_host(host);
> + mmc_detach_bus(host);
> + sd_uhs2_power_off(host);
> + mmc_release_host(host);
> + }
> +}
> +
> +static int sd_uhs2_suspend(struct mmc_host *host)
> +{
> + return 0;
> +}
> +
> +static int sd_uhs2_resume(struct mmc_host *host)
> +{
> + return 0;
> +}
> +
> +static int sd_uhs2_runtime_suspend(struct mmc_host *host)
> +{
> + return 0;
> +}
> +
> +static int sd_uhs2_runtime_resume(struct mmc_host *host)
> +{
> + return 0;
> +}
> +
> +static int sd_uhs2_shutdown(struct mmc_host *host)
> +{
> + return 0;
> +}
> +
> +static int sd_uhs2_hw_reset(struct mmc_host *host)
> +{
> + return 0;
> +}
> +
> +static const struct mmc_bus_ops sd_uhs2_ops = {
> + .remove = sd_uhs2_remove,
> + .alive = sd_uhs2_alive,
> + .detect = sd_uhs2_detect,
> + .suspend = sd_uhs2_suspend,
> + .resume = sd_uhs2_resume,
> + .runtime_suspend = sd_uhs2_runtime_suspend,
> + .runtime_resume = sd_uhs2_runtime_resume,
> + .shutdown = sd_uhs2_shutdown,
> + .hw_reset = sd_uhs2_hw_reset,
> +};
> +
> +static int sd_uhs2_attach(struct mmc_host *host)
> +{
> + int err;
> +
> + err = sd_uhs2_power_up(host);
> + if (err)
> + goto err;
> +
> + err = sd_uhs2_phy_init(host);
> + if (err)
> + goto err;
> +
> + err = sd_uhs2_init_card(host);
> + if (err)
> + goto err;
> +
> + err = sd_uhs2_legacy_init(host, host->card);
> + if (err)
> + goto err;
> +
> + mmc_attach_bus(host, &sd_uhs2_ops);
> +
> + mmc_release_host(host);
> +
> + err = mmc_add_card(host->card);
> + if (err)
> + goto remove_card;
> +
> + mmc_claim_host(host);
> + return 0;
> +
> +remove_card:
> + mmc_remove_card(host->card);
> + host->card = NULL;
> + mmc_claim_host(host);
> + mmc_detach_bus(host);
> +err:
> + sd_uhs2_power_off(host);
> + return err;
> +}
> +
> +int mmc_attach_sd_uhs2(struct mmc_host *host)
> +{
> + int i, err = 0;
> +
> + if (!(host->caps2 & MMC_CAP2_SD_UHS2))
> + return -EOPNOTSUPP;
> +
> + /* Turn off the legacy SD interface before trying with UHS-II. */
> + mmc_power_off(host);
> +
> + /*
> + * Start UHS-II initialization at 52MHz and possibly make a retry at
> + * 26MHz according to the spec. It's required that the host driver
> + * validates ios->clock, to set a rate within the correct range.
> + */
> + for (i = 0; i < ARRAY_SIZE(sd_uhs2_freqs); i++) {
> + host->f_init = sd_uhs2_freqs[i];
> + err = sd_uhs2_attach(host);
> + if (!err)
> + break;
> + }
> +
> + return err;
> +}
> diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
> index daa2f40d9ce6..469fd68f854f 100644
> --- a/include/linux/mmc/card.h
> +++ b/include/linux/mmc/card.h
> @@ -211,6 +211,11 @@ struct sd_ext_reg {
> #define SD_EXT_PERF_CMD_QUEUE (1<<4)
> };
>
> +struct sd_uhs2_config {
> + u32 node_id;
> + /* TODO: Extend with more register configs. */
> +};
> +
> struct sdio_cccr {
> unsigned int sdio_vsn;
> unsigned int sd_vsn;
> @@ -318,6 +323,8 @@ struct mmc_card {
> struct sd_ext_reg ext_power; /* SD extension reg for PM */
> struct sd_ext_reg ext_perf; /* SD extension reg for PERF */
>
> + struct sd_uhs2_config uhs2_config; /* SD UHS-II config */
> +
> unsigned int sdio_funcs; /* number of SDIO functions */
> atomic_t sdio_funcs_probed; /* number of probed SDIO funcs */
> struct sdio_cccr cccr; /* common card info */
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index 461d1543893b..2e3748e4f14d 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -63,6 +63,10 @@ struct mmc_ios {
> #define MMC_TIMING_MMC_HS400 10
> #define MMC_TIMING_SD_EXP 11
> #define MMC_TIMING_SD_EXP_1_2V 12
> +#define MMC_TIMING_UHS2_SPEED_A 13
> +#define MMC_TIMING_UHS2_SPEED_A_HD 14
> +#define MMC_TIMING_UHS2_SPEED_B 15
> +#define MMC_TIMING_UHS2_SPEED_B_HD 16
>
> unsigned char signal_voltage; /* signalling voltage (1.8V or 3.3V) */
>
> @@ -91,6 +95,21 @@ struct mmc_clk_phase_map {
> struct mmc_clk_phase phase[MMC_NUM_CLK_PHASES];
> };
>
> +struct sd_uhs2_caps {
> + /* TODO: Add UHS-II capabilities for the host. */
> +};
> +
> +enum sd_uhs2_operation {
> + UHS2_PHY_INIT = 0,
> + UHS2_SET_CONFIG,
> + UHS2_ENABLE_INT,
> + UHS2_DISABLE_INT,
> + UHS2_ENABLE_CLK,
> + UHS2_DISABLE_CLK,
> + UHS2_CHECK_DORMANT,

Looks like the above is better added in the next couple of patches,
where these actually become used.

> + UHS2_SET_IOS,
> +};
> +
> struct mmc_host;
>
> enum mmc_err_stat {
> @@ -145,6 +164,17 @@ struct mmc_host_ops {
> */
> void (*set_ios)(struct mmc_host *host, struct mmc_ios *ios);
>
> + /*
> + * The uhs2_set_ios callback is mandatory to implement for hosts that
> + * supports the SD UHS-II interface (MMC_CAP2_SD_UHS2), while the
> + * callback is unused for the other cases. Note that, the struct
> + * mmc_ios is being re-used for this as well.
> + *
> + * Expected return values for the uhs2_set_ios callback are a negative
> + * errno in case of a failure or zero for success.
> + */
> + int (*uhs2_set_ios)(struct mmc_host *host, struct mmc_ios *ios);
> +

This isn't used, please drop it.

> /*
> * Return values for the get_ro callback should be:
> * 0 for a read/write card
> @@ -212,6 +242,14 @@ struct mmc_host_ops {
>
> /* Initialize an SD express card, mandatory for MMC_CAP2_SD_EXP. */
> int (*init_sd_express)(struct mmc_host *host, struct mmc_ios *ios);
> +
> + /*
> + * The uhs2_control callback is used to execute SD UHS-II specific
> + * operations. It's mandatory to implement for hosts that supports the
> + * SD UHS-II interface (MMC_CAP2_SD_UHS2). Expected return values are a
> + * negative errno in case of a failure or zero for success.
> + */
> + int (*uhs2_control)(struct mmc_host *host, enum sd_uhs2_operation op);
> };
>
> struct mmc_cqe_ops {
> @@ -396,6 +434,7 @@ struct mmc_host {
> MMC_CAP2_HS200_1_2V_SDR)
> #define MMC_CAP2_SD_EXP (1 << 7) /* SD express via PCIe */
> #define MMC_CAP2_SD_EXP_1_2V (1 << 8) /* SD express 1.2V */
> +#define MMC_CAP2_SD_UHS2 (1 << 9) /* SD UHS-II support */
> #define MMC_CAP2_CD_ACTIVE_HIGH (1 << 10) /* Card-detect signal active high */
> #define MMC_CAP2_RO_ACTIVE_HIGH (1 << 11) /* Write-protect signal active high */
> #define MMC_CAP2_NO_PRESCAN_POWERUP (1 << 14) /* Don't power up before scan */
> @@ -422,6 +461,12 @@ struct mmc_host {
> #endif
> #define MMC_CAP2_ALT_GPT_TEGRA (1 << 28) /* Host with eMMC that has GPT entry at a non-standard location */
>
> + struct sd_uhs2_caps uhs2_caps; /* Host UHS-II capabilities */
> +
> + int flags;
> +#define MMC_UHS2_SUPPORT (1 << 0)
> +#define MMC_UHS2_SD_TRAN (1 << 1)

As I said above, please drop this from the $subject patch and move it
to the patch that implements SD tran.

> +
> int fixed_drv_type; /* fixed driver type for non-removable media */
>
> mmc_pm_flag_t pm_caps; /* supported pm features */

Kind regards
Uffe