[V6,05/24] mmc: core: Add definitions for SD UHS-II cards

Add UHS-II specific data structures for commands and defines for
registers, as described in Part 1 UHS-II Addendum Version 1.01.

UHS-II related definitions are listed below:
  1. UHS-II card capability: sd_uhs2_caps{}
  2. UHS-II configuration: sd_uhs2_config{}
  3. UHS-II Command structure: uhs2_command{}
  4. UHS-II register I/O address and register field definitions: sd_uhs2.h

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
Signed-off-by: Jason Lai <jason.lai@genesyslogic.com.tw>
Signed-off-by: Victor Shih <victor.shih@genesyslogic.com.tw>
---
 include/linux/mmc/card.h    |  42 +++++-
 include/linux/mmc/core.h    |  13 ++
 include/linux/mmc/host.h    |  63 ++++++++-
 include/linux/mmc/sd_uhs2.h | 263 ++++++++++++++++++++++++++++++++++++
 4 files changed, 379 insertions(+), 2 deletions(-)
 create mode 100644 include/linux/mmc/sd_uhs2.h

Hi Victor.

On Tue, 13 Dec 2022 at 10:01, Victor Shih <victorshihgli@gmail.com> wrote:
>
> Add UHS-II specific data structures for commands and defines for
> registers, as described in Part 1 UHS-II Addendum Version 1.01.
>
> UHS-II related definitions are listed below:
>   1. UHS-II card capability: sd_uhs2_caps{}
>   2. UHS-II configuration: sd_uhs2_config{}
>   3. UHS-II Command structure: uhs2_command{}
>   4. UHS-II register I/O address and register field definitions: sd_uhs2.h
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> Signed-off-by: Jason Lai <jason.lai@genesyslogic.com.tw>
> Signed-off-by: Victor Shih <victor.shih@genesyslogic.com.tw>

I have looked through the series in detail and it's certainly moving
in the right direction, thanks for being persistent and working on
this!

> ---
>  include/linux/mmc/card.h    |  42 +++++-
>  include/linux/mmc/core.h    |  13 ++
>  include/linux/mmc/host.h    |  63 ++++++++-
>  include/linux/mmc/sd_uhs2.h | 263 ++++++++++++++++++++++++++++++++++++
>  4 files changed, 379 insertions(+), 2 deletions(-)
>  create mode 100644 include/linux/mmc/sd_uhs2.h
>
> diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
> index 4a42f31b7bb0..d638a2c689cf 100644
> --- a/include/linux/mmc/card.h
> +++ b/include/linux/mmc/card.h
> @@ -190,6 +190,13 @@ struct sd_switch_caps {
>  #define SD_MAX_CURRENT_400     (1 << SD_SET_CURRENT_LIMIT_400)
>  #define SD_MAX_CURRENT_600     (1 << SD_SET_CURRENT_LIMIT_600)
>  #define SD_MAX_CURRENT_800     (1 << SD_SET_CURRENT_LIMIT_800)
> +       unsigned int            sd4_curr_limit;

This is unused, can we drop it?

> +#define SD4_SET_POWER_LIMIT_0_72W      0
> +#define SD4_SET_POWER_LIMIT_1_44W      1
> +#define SD4_SET_POWER_LIMIT_2_16W      2
> +#define SD4_SET_POWER_LIMIT_2_88W      3
> +#define SD4_SET_POWER_LIMIT_1_80W      4
> +#define SD4_SET_POWER_NO_CHANGE                (-1)

SD4_SET_POWER_NO_CHANGE seems to be redundant, please drop it.

Note that, I realize that the series only supports
SD4_SET_POWER_LIMIT_1_80W, but I think it's fine to keep the others
definitions, as they are just mirroring what exists in the SD spec.
Right?

>  };
>
>  struct sd_ext_reg {
> @@ -213,7 +220,35 @@ struct sd_ext_reg {
>
>  struct sd_uhs2_config {
>         u32                     node_id;
> -       /* TODO: Extend with more register configs. */
> +
> +       u32                     dap;
> +       u32                     gap;
> +       u32                     n_fcu;
> +       u32                     maxblk_len;
> +       u8                      n_lanes;
> +       u8                      dadr_len;
> +       u8                      app_type;
> +       u8                      phy_minor_rev;
> +       u8                      phy_major_rev;
> +       u8                      can_hibernate;
> +       u8                      n_lss_sync;
> +       u8                      n_lss_dir;
> +       u8                      link_minor_rev;
> +       u8                      link_major_rev;
> +       u8                      dev_type;
> +       u8                      n_data_gap;
> +
> +       u32                     n_fcu_set;
> +       u32                     maxblk_len_set;
> +       u8                      n_lanes_set;
> +       u8                      speed_range_set;
> +       u8                      n_lss_sync_set;
> +       u8                      n_lss_dir_set;
> +       u8                      n_data_gap_set;
> +       u8                      pwrctrl_mode_set;
> +       u8                      max_retry_set;
> +
> +       u8                      cfg_complete;

There seems to be a couple of variables in the above struct that are
unused (like dap, gap, cfg_complete, etc). Can we drop them?

>  };
>
>  struct sdio_cccr {
> @@ -323,6 +358,9 @@ struct mmc_card {
>         struct sd_ext_reg       ext_perf;       /* SD extension reg for PERF */
>
>         struct sd_uhs2_config   uhs2_config;    /* SD UHS-II config */
> +       u8                      uhs2_state;     /* SD UHS-II states */
> +#define MMC_UHS2_INITIALIZED   BIT(1)

Rather than introducing a new variable in the struct mmc_card for
this, I suggest we add a new bool in the struct mmc_host for this. If
I understand correctly, this flag indicates that the host
communication is prepared to support SD-tran commands? Perhaps also
name the variable to "uhs2_sd_tran", to try to reflect its purpose?

Moreover, it looks to me that sd_uhs2_power_off() would be a good
place for where to reset the flag, rather than having that sprinkled
across several error paths.

> +#define MMC_UHS2_SPEED_B       BIT(2)

I was looking through the series to figure out how this bit is being
used. So, it gets set in sd_uhs2_config_write(), based upon whether
the host and the card, both supports UHS2 speed mode B. Then, in
sd_uhs2_config_write() we calls ->uhs2_control(host, UHS2_SET_CONFIG).

By looking at the changes for the sdhci driver, the MMC_UHS2_SPEED_B
bit gets checked at "UHS2_SET_CONFIG" through the ->uhs2_control()
callbacks, to determine whether the SDHCI_UHS2_PHY_SET_SPEED_B bit
should be written to the sdhci_uhs2_phy_set_reg or not. This seems
reasonable to me. However, I wonder if the call to
->uhs2_control(host, UHS2_SET_SPEED_B) in sd_uhs2_change_speed() is
then really needed, as it looks like that would just be re-writing the
same bit again in sdhci. Is this deliberate and needed? If not, should
we drop the call to ->uhs2_control(host, UHS2_SET_SPEED_B)?

When it comes to the actual MMC_UHS2_SPEED_B bit, I think it better
belongs in the ios->timing variable. In this regard, I suggest we add
the below bits, to indicate the current UHS2 speed mode.

MMC_TIMING_UHS2_SPEED_A
MMC_TIMING_UHS2_SPEED_A_HD (HD for half-duplex)
MMC_TIMING_UHS2_SPEED_B
MMC_TIMING_UHS2_SPEED_B_HD (HD for half-duplex)

These should help us to support all the various modes, for now. Let's
leave UHS2 speed range C and D for later.

I would also suggest extending mmc_card_uhs2(), that is introduced in
patch3, to return true, if any of the bits above are being set.

Finally, in sd_uhs2_power_up() I think it would be better to set the
->ios.timing to MMC_TIMING_UHS2_SPEED_A, as the initial value, rather
than MMC_TIMING_SD_UHS2 (as was introduced in patch2). In other words,
let's drop MMC_TIMING_SD_UHS2 in favor of MMC_TIMING_UHS2_SPEED_A.

>
>         unsigned int            sdio_funcs;     /* number of SDIO functions */
>         atomic_t                sdio_funcs_probed; /* number of probed SDIO funcs */
> @@ -364,4 +402,6 @@ bool mmc_card_is_blockaddr(struct mmc_card *card);
>  #define mmc_card_sdio(c)       ((c)->type == MMC_TYPE_SDIO)
>  #define mmc_card_sd_combo(c)   ((c)->type == MMC_TYPE_SD_COMBO)
>
> +#define mmc_card_can_poweroff_notify(c)        ((c)->ext_power.feature_support & SD_EXT_POWER_OFF_NOTIFY)
> +

Please move this to drivers/mmc/core/sd_ops.h and implement this as a
regular c-function. I also suggest renaming it to
mmc_sd_can_poweroff_notify().

[...]

> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -16,6 +16,7 @@
>  #include <linux/mmc/pm.h>
>  #include <linux/dma-direction.h>
>  #include <linux/blk-crypto-profile.h>
> +#include <linux/mmc/sd_uhs2.h>
>
>  struct mmc_ios {
>         unsigned int    clock;                  /* clock rate */
> @@ -96,7 +97,48 @@ struct mmc_clk_phase_map {
>  };
>
>  struct sd_uhs2_caps {
> -       /* TODO: Add UHS-II capabilities for the host. */
> +       u32     dap;
> +       u32     gap;
> +       u32     group_desc;
> +       u32     maxblk_len;
> +       u32     n_fcu;
> +       u8      n_lanes;
> +       u8      addr64;
> +       u8      card_type;
> +       u8      phy_rev;
> +       u8      speed_range;
> +       u8      can_hibernate;
> +       u8      n_lss_sync;
> +       u8      n_lss_dir;
> +       u8      link_rev;
> +       u8      host_type;
> +       u8      n_data_gap;
> +
> +       u32     maxblk_len_set;
> +       u32     n_fcu_set;
> +       u8      n_lanes_set;
> +       u8      n_lss_sync_set;
> +       u8      n_lss_dir_set;
> +       u8      n_data_gap_set;
> +       u8      max_retry_set;

As I found a few unused variables in the earlier struct definitions,
please walk through this too, to make sure we don't waste data.

> +};
> +
> +struct sd_uhs2_ios {
> +       bool            is_2L_HD_mode;

I think we should use the ios->timing variable for this instead. If
the two data-lines are supported to run in half-duplex mode, I think
it's better to use one of the below bits, as I kind of also suggested
earlier above.

MMC_TIMING_UHS2_SPEED_A_HD
MMC_TIMING_UHS2_SPEED_B_HD

As "is_2L_HD_mode" seems to be checked at couple of different places
in the core and in sdhci, perhaps we should add a new helper function,
"mmc_card_uhs2_hd_mode()" that would return true if any of the above
bits are set?

> +       bool            is_APP_CMD;

I think this better belongs in the struct mmc_host, please move it
there and then also rename it into "uhs2_app_cmd". Note that, there is
also one case in mmc_sd_num_wr_blocks() that sends an MMC_APP_CMD, but
without calling mmc_app_cmd(). This needs to be fixed.

> +       unsigned int    power_delay_ms;         /* waiting for stable power */

This seems to be unused, so let's drop it!

> +};
> +
> +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,
> +       UHS2_SET_SPEED_B,
> +       UHS2_POST_ATTACH_SD,
>  };
>
>  struct mmc_host;
> @@ -231,6 +273,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 {
> @@ -323,6 +373,7 @@ struct mmc_pwrseq;
>
>  struct mmc_supply {
>         struct regulator *vmmc;         /* Card power supply */
> +       struct regulator *vmmc2;        /* UHS2 VDD2 power supply */

In patch4 I added support for vqmmc2, which is targeted to be used for
VDD2. Is there any reason that can't be used?

>         struct regulator *vqmmc;        /* Optional Vccq supply */
>         struct regulator *vqmmc2;       /* Optional supply for phy */
>  };
> @@ -344,10 +395,12 @@ struct mmc_host {
>         u32                     ocr_avail_sdio; /* SDIO-specific OCR */
>         u32                     ocr_avail_sd;   /* SD-specific OCR */
>         u32                     ocr_avail_mmc;  /* MMC-specific OCR */
> +       u32                     ocr_avail_uhs2; /* UHS2-specific OCR */

The way I implemented support for vqmmc2, we shouldn't need a specific
ocr mask for UHS2. So, unless I am mistaken we should be able to use
only 1.8V, for now.

>         struct wakeup_source    *ws;            /* Enable consume of uevents */
>         u32                     max_current_330;
>         u32                     max_current_300;
>         u32                     max_current_180;
> +       u32                     max_current_180_vdd2; /* UHS2 vdd2 max curt. */

This looks sdhci specific. Should we move it to the sdhci struct instead?

>
>  #define MMC_VDD_165_195                0x00000080      /* VDD voltage 1.65 - 1.95 */
>  #define MMC_VDD_20_21          0x00000100      /* VDD voltage 2.0 ~ 2.1 */
> @@ -366,6 +419,7 @@ struct mmc_host {
>  #define MMC_VDD_33_34          0x00200000      /* VDD voltage 3.3 ~ 3.4 */
>  #define MMC_VDD_34_35          0x00400000      /* VDD voltage 3.4 ~ 3.5 */
>  #define MMC_VDD_35_36          0x00800000      /* VDD voltage 3.5 ~ 3.6 */
> +#define MMC_VDD2_165_195       0x00000080      /* UHS2 VDD2 1.65 ~ 1.95 */

Please drop this. We already have MMC_VDD_165_195, for these bits, see above.

>
>         u32                     caps;           /* Host capabilities */
>
> @@ -443,7 +497,11 @@ struct mmc_host {
>  #endif
>  #define MMC_CAP2_ALT_GPT_TEGRA (1 << 28)       /* Host with eMMC that has GPT entry at a non-standard location */
>
> +       int flags;
> +#define MMC_UHS2_SUPPORT       (1 << 0)

According to my comments above, I don't think this is needed at all.
Please drop it.

> +
>         struct sd_uhs2_caps     uhs2_caps;      /* Host UHS-II capabilities */
> +       struct sd_uhs2_ios      uhs2_ios;       /* Host UHS-II capabilities */

As indicated above, the struct sd_uhs2_ios can be dropped.

>
>         int                     fixed_drv_type; /* fixed driver type for non-removable media */
>
> @@ -695,4 +753,7 @@ int mmc_send_tuning(struct mmc_host *host, u32 opcode, int *cmd_error);
>  int mmc_send_abort_tuning(struct mmc_host *host, u32 opcode);
>  int mmc_get_ext_csd(struct mmc_card *card, u8 **new_ext_csd);
>
> +#define mmc_uhs2_2L_HD_mode(h) ((h)->uhs2_ios.is_2L_HD_mode)
> +#define mmc_uhs2_APP_cmd(h)    ((h)->uhs2_ios.is_APP_CMD)
> +

These are unused. Please drop or replace them with something that can be used.

>  #endif /* LINUX_MMC_HOST_H */
> diff --git a/include/linux/mmc/sd_uhs2.h b/include/linux/mmc/sd_uhs2.h
> new file mode 100644
> index 000000000000..8fcf702cf4a5
> --- /dev/null
> +++ b/include/linux/mmc/sd_uhs2.h
> @@ -0,0 +1,263 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Header file for UHS-II packets, Host Controller registers and I/O
> + * accessors.
> + *
> + *  Copyright (C) 2014 Intel Corp, All Rights Reserved.
> + */
> +#ifndef LINUX_MMC_UHS2_H
> +#define LINUX_MMC_UHS2_H
> +
> +/* LINK Layer definition */
> +/*
> + * UHS2 Header:
> + * Refer to UHS-II Addendum Version 1.02 Figure 5-2, the format of CCMD Header is described below:
> + *      bit [3:0]  : DID(Destination ID = Node ID of UHS2 card)
> + *      bit [6:4]  : TYP(Packet Type)
> + *                   000b: CCMD(Control command packet)
> + *                   001b: DCMD(Data command packet)
> + *                   010b: RES(Response packet)
> + *                   011b: DATA(Data payload packet)
> + *                   111b: MSG(Message packet)
> + *                   Others: Reserved
> + *      bit [7]    : NP(Native Packet)
> + *      bit [10:8] : TID(Transaction ID)
> + *      bit [11]   : Reserved
> + *      bit [15:12]: SID(Source ID 0: Node ID of Host)
> + *
> + * Broadcast CCMD issued by Host is represented as DID=SID=0.
> + */
> +/*
> + * UHS2 Argument:
> + * Refer to UHS-II Addendum Version 1.02 Figure 6-5, the format of CCMD Argument is described below:
> + *      bit [3:0]  : MSB of IOADR
> + *      bit [5:4]  : PLEN(Payload Length)
> + *                   00b: 0 byte
> + *                   01b: 4 bytes
> + *                   10b: 8 bytes
> + *                   11b: 16 bytes
> + *      bit [6]    : Reserved
> + *      bit [7]    : R/W(Read/Write)
> + *                   0: Control read command
> + *                   1: Control write command
> + *      bit [15:8] : LSB of IOADR
> + *
> + * I/O Address specifies the address of register in UHS-II I/O space accessed by CCMD.
> + * The unit of I/O Address is 4 Bytes. It is transmitted in MSB first, LSB last.
> + */
> +#define UHS2_NATIVE_PACKET_POS 7
> +#define UHS2_NATIVE_PACKET     (1 << UHS2_NATIVE_PACKET_POS)
> +
> +#define UHS2_PACKET_TYPE_POS   4
> +#define UHS2_PACKET_TYPE_CCMD  (0 << UHS2_PACKET_TYPE_POS)
> +#define UHS2_PACKET_TYPE_DCMD  (1 << UHS2_PACKET_TYPE_POS)
> +#define UHS2_PACKET_TYPE_RES   (2 << UHS2_PACKET_TYPE_POS)
> +#define UHS2_PACKET_TYPE_DATA  (3 << UHS2_PACKET_TYPE_POS)
> +#define UHS2_PACKET_TYPE_MSG   (7 << UHS2_PACKET_TYPE_POS)
> +
> +#define UHS2_DEST_ID_MASK      0x0F
> +#define UHS2_DEST_ID           0x1
> +
> +#define UHS2_SRC_ID_POS                12
> +#define UHS2_SRC_ID_MASK       0xF000
> +
> +#define UHS2_TRANS_ID_POS      8
> +#define UHS2_TRANS_ID_MASK     0x0700
> +
> +/* UHS2 MSG */
> +#define UHS2_MSG_CTG_POS       5
> +#define UHS2_MSG_CTG_LMSG      0x00
> +#define UHS2_MSG_CTG_INT       0x60
> +#define UHS2_MSG_CTG_AMSG      0x80
> +
> +#define UHS2_MSG_CTG_FCREQ     0x00
> +#define UHS2_MSG_CTG_FCRDY     0x01
> +#define UHS2_MSG_CTG_STAT      0x02
> +
> +#define UHS2_MSG_CODE_POS                      8
> +#define UHS2_MSG_CODE_FC_UNRECOVER_ERR         0x8
> +#define UHS2_MSG_CODE_STAT_UNRECOVER_ERR       0x8
> +#define UHS2_MSG_CODE_STAT_RECOVER_ERR         0x1
> +
> +/* TRANS Layer definition */
> +
> +/* Native packets*/
> +#define UHS2_NATIVE_CMD_RW_POS 7
> +#define UHS2_NATIVE_CMD_WRITE  (1 << UHS2_NATIVE_CMD_RW_POS)
> +#define UHS2_NATIVE_CMD_READ   (0 << UHS2_NATIVE_CMD_RW_POS)
> +
> +#define UHS2_NATIVE_CMD_PLEN_POS       4
> +#define UHS2_NATIVE_CMD_PLEN_4B                (1 << UHS2_NATIVE_CMD_PLEN_POS)
> +#define UHS2_NATIVE_CMD_PLEN_8B                (2 << UHS2_NATIVE_CMD_PLEN_POS)
> +#define UHS2_NATIVE_CMD_PLEN_16B       (3 << UHS2_NATIVE_CMD_PLEN_POS)
> +
> +#define UHS2_NATIVE_CCMD_GET_MIOADR_MASK       0xF00
> +#define UHS2_NATIVE_CCMD_MIOADR_MASK           0x0F
> +
> +#define UHS2_NATIVE_CCMD_LIOADR_POS            8
> +#define UHS2_NATIVE_CCMD_GET_LIOADR_MASK       0x0FF
> +
> +#define UHS2_CCMD_DEV_INIT_COMPLETE_FLAG       BIT(11)
> +#define UHS2_DEV_INIT_PAYLOAD_LEN              1
> +#define UHS2_DEV_INIT_RESP_LEN                 6
> +#define UHS2_DEV_ENUM_PAYLOAD_LEN              1
> +#define UHS2_DEV_ENUM_RESP_LEN                 8
> +#define UHS2_CFG_WRITE_PAYLOAD_LEN             2
> +#define UHS2_CFG_WRITE_PHY_SET_RESP_LEN                4
> +#define UHS2_CFG_WRITE_GENERIC_SET_RESP_LEN    5
> +#define UHS2_GO_DORMANT_PAYLOAD_LEN            1
> +
> +/*
> + * UHS2 Argument:
> + * Refer to UHS-II Addendum Version 1.02 Figure 6-8, the format of DCMD Argument is described below:
> + *      bit [3:0]  : Reserved
> + *      bit [6:3]  : TMODE(Transfer Mode)
> + *                   bit 3: DAM(Data Access Mode)
> + *                   bit 4: TLUM(TLEN Unit Mode)
> + *                   bit 5: LM(Length Mode)
> + *                   bit 6: DM(Duplex Mode)
> + *      bit [7]    : R/W(Read/Write)
> + *                   0: Control read command
> + *                   1: Control write command
> + *      bit [15:8] : Reserved
> + *
> + * I/O Address specifies the address of register in UHS-II I/O space accessed by CCMD.
> + * The unit of I/O Address is 4 Bytes. It is transmitted in MSB first, LSB last.
> + */
> +#define UHS2_DCMD_DM_POS               6
> +#define UHS2_DCMD_2L_HD_MODE           (1 << UHS2_DCMD_DM_POS)
> +#define UHS2_DCMD_LM_POS               5
> +#define UHS2_DCMD_LM_TLEN_EXIST                (1 << UHS2_DCMD_LM_POS)
> +#define UHS2_DCMD_TLUM_POS             4
> +#define UHS2_DCMD_TLUM_BYTE_MODE       (1 << UHS2_DCMD_TLUM_POS)
> +#define UHS2_NATIVE_DCMD_DAM_POS       3
> +#define UHS2_NATIVE_DCMD_DAM_IO                (1 << UHS2_NATIVE_DCMD_DAM_POS)
> +
> +/*
> + * Per UHS2 spec, DCMD payload should be MSB first. There may be
> + * two types of data be assembled to MSB:
> + * 1. TLEN: Input block size for single read/write and number of blocks
> + * for multiple read/write to calculate TLEN as MSB first per spec.
> + * 2. SD command argument.
> + */
> +static inline __be32 uhs2_dcmd_convert_msb(u32 input)
> +{
> +       u32 ret = 0;
> +
> +       ret = ((input & 0xFF) << 24) |
> +               (((input >> 8) & 0xFF) << 16) |
> +               (((input >> 16) & 0xFF) << 8) |
> +               ((input >> 24) & 0xFF);
> +       return cpu_to_be32(ret);
> +}

This looks unused, can we drop it?

> +
> +#define UHS2_RES_NACK_POS      7
> +#define UHS2_RES_NACK_MASK     (0x1 << UHS2_RES_NACK_POS)
> +
> +#define UHS2_RES_ECODE_POS     4
> +#define UHS2_RES_ECODE_MASK    0x7
> +#define UHS2_RES_ECODE_COND    1
> +#define UHS2_RES_ECODE_ARG     2
> +#define UHS2_RES_ECODE_GEN     3
> +
> +/* IOADR of device registers */
> +#define UHS2_IOADR_GENERIC_CAPS                0x00
> +#define UHS2_IOADR_PHY_CAPS            0x02
> +#define UHS2_IOADR_LINK_CAPS           0x04
> +#define UHS2_IOADR_RSV_CAPS            0x06
> +#define UHS2_IOADR_GENERIC_SETTINGS    0x08
> +#define UHS2_IOADR_PHY_SETTINGS                0x0A
> +#define UHS2_IOADR_LINK_SETTINGS       0x0C
> +#define UHS2_IOADR_PRESET              0x40
> +
> +/* SD application packets */
> +#define UHS2_SD_CMD_INDEX_POS  8
> +
> +#define UHS2_SD_CMD_APP_POS    14
> +#define UHS2_SD_CMD_APP                (1 << UHS2_SD_CMD_APP_POS)
> +
> +/* UHS-II Device Registers */
> +#define UHS2_DEV_CONFIG_REG    0x000
> +
> +/* General Caps and Settings registers */
> +#define UHS2_DEV_CONFIG_GEN_CAPS       (UHS2_DEV_CONFIG_REG + 0x000)
> +#define UHS2_DEV_CONFIG_N_LANES_POS    8
> +#define UHS2_DEV_CONFIG_N_LANES_MASK   0x3F
> +#define UHS2_DEV_CONFIG_2L_HD_FD       0x1
> +#define UHS2_DEV_CONFIG_2D1U_FD                0x2
> +#define UHS2_DEV_CONFIG_1D2U_FD                0x4
> +#define UHS2_DEV_CONFIG_2D2U_FD                0x8
> +#define UHS2_DEV_CONFIG_DADR_POS       14
> +#define UHS2_DEV_CONFIG_DADR_MASK      0x1
> +#define UHS2_DEV_CONFIG_APP_POS                16
> +#define UHS2_DEV_CONFIG_APP_MASK       0xFF
> +#define UHS2_DEV_CONFIG_APP_SD_MEM     0x1
> +
> +#define UHS2_DEV_CONFIG_GEN_SET                        (UHS2_DEV_CONFIG_REG + 0x008)
> +#define UHS2_DEV_CONFIG_GEN_SET_N_LANES_POS    8
> +#define UHS2_DEV_CONFIG_GEN_SET_2L_FD_HD       0x0
> +#define UHS2_DEV_CONFIG_GEN_SET_2D1U_FD                0x2
> +#define UHS2_DEV_CONFIG_GEN_SET_1D2U_FD                0x3
> +#define UHS2_DEV_CONFIG_GEN_SET_2D2U_FD                0x4
> +#define UHS2_DEV_CONFIG_GEN_SET_CFG_COMPLETE   BIT(31)
> +
> +/* PHY Caps and Settings registers */
> +#define UHS2_DEV_CONFIG_PHY_CAPS       (UHS2_DEV_CONFIG_REG + 0x002)
> +#define UHS2_DEV_CONFIG_PHY_MINOR_MASK 0xF
> +#define UHS2_DEV_CONFIG_PHY_MAJOR_POS  4
> +#define UHS2_DEV_CONFIG_PHY_MAJOR_MASK 0x3
> +#define UHS2_DEV_CONFIG_CAN_HIBER_POS  15
> +#define UHS2_DEV_CONFIG_CAN_HIBER_MASK 0x1
> +#define UHS2_DEV_CONFIG_PHY_CAPS1      (UHS2_DEV_CONFIG_REG + 0x003)
> +#define UHS2_DEV_CONFIG_N_LSS_SYN_MASK 0xF
> +#define UHS2_DEV_CONFIG_N_LSS_DIR_POS  4
> +#define UHS2_DEV_CONFIG_N_LSS_DIR_MASK 0xF
> +
> +#define UHS2_DEV_CONFIG_PHY_SET                        (UHS2_DEV_CONFIG_REG + 0x00A)
> +#define UHS2_DEV_CONFIG_PHY_SET_SPEED_POS      6
> +#define UHS2_DEV_CONFIG_PHY_SET_SPEED_A                0x0
> +#define UHS2_DEV_CONFIG_PHY_SET_SPEED_B                0x1
> +
> +/* LINK-TRAN Caps and Settings registers */
> +#define UHS2_DEV_CONFIG_LINK_TRAN_CAPS         (UHS2_DEV_CONFIG_REG + 0x004)
> +#define UHS2_DEV_CONFIG_LT_MINOR_MASK          0xF
> +#define UHS2_DEV_CONFIG_LT_MAJOR_POS           4
> +#define UHS2_DEV_CONFIG_LT_MAJOR_MASK          0x3
> +#define UHS2_DEV_CONFIG_N_FCU_POS              8
> +#define UHS2_DEV_CONFIG_N_FCU_MASK             0xFF
> +#define UHS2_DEV_CONFIG_DEV_TYPE_POS           16
> +#define UHS2_DEV_CONFIG_DEV_TYPE_MASK          0x7
> +#define UHS2_DEV_CONFIG_MAX_BLK_LEN_POS                20
> +#define UHS2_DEV_CONFIG_MAX_BLK_LEN_MASK       0xFFF
> +#define UHS2_DEV_CONFIG_LINK_TRAN_CAPS1                (UHS2_DEV_CONFIG_REG + 0x005)
> +#define UHS2_DEV_CONFIG_N_DATA_GAP_MASK                0xFF
> +
> +#define UHS2_DEV_CONFIG_LINK_TRAN_SET          (UHS2_DEV_CONFIG_REG + 0x00C)
> +#define UHS2_DEV_CONFIG_LT_SET_MAX_BLK_LEN     0x200
> +#define UHS2_DEV_CONFIG_LT_SET_MAX_RETRY_POS   16
> +
> +/* Preset register */
> +#define UHS2_DEV_CONFIG_PRESET (UHS2_DEV_CONFIG_REG + 0x040)
> +
> +#define UHS2_DEV_INT_REG       0x100
> +
> +#define UHS2_DEV_STATUS_REG    0x180
> +
> +#define UHS2_DEV_CMD_REG               0x200
> +#define UHS2_DEV_CMD_FULL_RESET                (UHS2_DEV_CMD_REG + 0x000)
> +#define UHS2_DEV_CMD_GO_DORMANT_STATE  (UHS2_DEV_CMD_REG + 0x001)
> +#define UHS2_DEV_CMD_DORMANT_HIBER     BIT(7)
> +#define UHS2_DEV_CMD_DEVICE_INIT       (UHS2_DEV_CMD_REG + 0x002)
> +#define UHS2_DEV_INIT_COMPLETE_FLAG    BIT(11)
> +#define UHS2_DEV_CMD_ENUMERATE         (UHS2_DEV_CMD_REG + 0x003)
> +#define UHS2_DEV_CMD_TRANS_ABORT       (UHS2_DEV_CMD_REG + 0x004)
> +
> +#define UHS2_RCLK_MAX  52000000
> +#define UHS2_RCLK_MIN  26000000
> +
> +struct sd_uhs2_wait_active_state_data {
> +       struct mmc_host *host;
> +       struct mmc_command *cmd;
> +};

Please move this to drivers/mmc/core/sd_uhs2.c.

> +
> +#endif /* LINUX_MMC_UHS2_H */
> --
> 2.25.1
>

I plan to have a closer look at the VDD2 things - but I think there
should be plenty of stuff for us to discuss and hopefully for you to
continue to work on, in the meantime.

Kind regards
Uffe

On Tue, 13 Dec 2022 at 10:01, Victor Shih <victorshihgli@gmail.com> wrote:
>
> Add UHS-II specific data structures for commands and defines for
> registers, as described in Part 1 UHS-II Addendum Version 1.01.
>
> UHS-II related definitions are listed below:
>   1. UHS-II card capability: sd_uhs2_caps{}
>   2. UHS-II configuration: sd_uhs2_config{}
>   3. UHS-II Command structure: uhs2_command{}
>   4. UHS-II register I/O address and register field definitions: sd_uhs2.h
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> Signed-off-by: Jason Lai <jason.lai@genesyslogic.com.tw>
> Signed-off-by: Victor Shih <victor.shih@genesyslogic.com.tw>

[...]

> --- a/include/linux/mmc/core.h
> +++ b/include/linux/mmc/core.h
> @@ -23,6 +23,14 @@ enum mmc_blk_status {
>         MMC_BLK_NEW_REQUEST,
>  };
>
> +struct uhs2_command {
> +       u16     header;
> +       u16     arg;
> +       __be32  *payload;
> +       u32     payload_len;
> +       u32     packet_len;
> +};
> +
>  struct mmc_command {
>         u32                     opcode;
>         u32                     arg;
> @@ -109,6 +117,11 @@ struct mmc_command {
>         unsigned int            busy_timeout;   /* busy detect timeout in ms */
>         struct mmc_data         *data;          /* data segment associated with cmd */
>         struct mmc_request      *mrq;           /* associated request */
> +
> +       struct uhs2_command     *uhs2_cmd;      /* UHS2 command */
> +       u8                      *uhs2_resp;     /* UHS2 native cmd resp */
> +       u8                      uhs2_resp_len;  /* UHS2 native cmd resp len */
> +       u8                      uhs2_tmode0_flag; /* UHS2 transfer mode flag */

I forgot to comment on this flag in my earlier reply. It looks like
the flag is being set for "MMC_CMD_ADTC" commands. This makes me
wonder, why can't we keep using cmd->flags to check this, rather than
adding a new flag solely for UHS2? Or is there a problem with this?

I will continue to comment around this at patch7 too.

[...]

Kind regards
Uffe

Hi, Ulf

On Tue, Feb 7, 2023 at 12:13 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> Hi Victor.
>
> On Tue, 13 Dec 2022 at 10:01, Victor Shih <victorshihgli@gmail.com> wrote:
> >
> > Add UHS-II specific data structures for commands and defines for
> > registers, as described in Part 1 UHS-II Addendum Version 1.01.
> >
> > UHS-II related definitions are listed below:
> >   1. UHS-II card capability: sd_uhs2_caps{}
> >   2. UHS-II configuration: sd_uhs2_config{}
> >   3. UHS-II Command structure: uhs2_command{}
> >   4. UHS-II register I/O address and register field definitions: sd_uhs2.h
> >
> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > Signed-off-by: Jason Lai <jason.lai@genesyslogic.com.tw>
> > Signed-off-by: Victor Shih <victor.shih@genesyslogic.com.tw>
>
> I have looked through the series in detail and it's certainly moving
> in the right direction, thanks for being persistent and working on
> this!
>
> > ---
> >  include/linux/mmc/card.h    |  42 +++++-
> >  include/linux/mmc/core.h    |  13 ++
> >  include/linux/mmc/host.h    |  63 ++++++++-
> >  include/linux/mmc/sd_uhs2.h | 263 ++++++++++++++++++++++++++++++++++++
> >  4 files changed, 379 insertions(+), 2 deletions(-)
> >  create mode 100644 include/linux/mmc/sd_uhs2.h
> >
> > diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
> > index 4a42f31b7bb0..d638a2c689cf 100644
> > --- a/include/linux/mmc/card.h
> > +++ b/include/linux/mmc/card.h
> > @@ -190,6 +190,13 @@ struct sd_switch_caps {
> >  #define SD_MAX_CURRENT_400     (1 << SD_SET_CURRENT_LIMIT_400)
> >  #define SD_MAX_CURRENT_600     (1 << SD_SET_CURRENT_LIMIT_600)
> >  #define SD_MAX_CURRENT_800     (1 << SD_SET_CURRENT_LIMIT_800)
> > +       unsigned int            sd4_curr_limit;
>
> This is unused, can we drop it?
>

I will drop it and update in V7 version.

> > +#define SD4_SET_POWER_LIMIT_0_72W      0
> > +#define SD4_SET_POWER_LIMIT_1_44W      1
> > +#define SD4_SET_POWER_LIMIT_2_16W      2
> > +#define SD4_SET_POWER_LIMIT_2_88W      3
> > +#define SD4_SET_POWER_LIMIT_1_80W      4
> > +#define SD4_SET_POWER_NO_CHANGE                (-1)
>
> SD4_SET_POWER_NO_CHANGE seems to be redundant, please drop it.
>

I will drop it and update in V7 version.

> Note that, I realize that the series only supports
> SD4_SET_POWER_LIMIT_1_80W, but I think it's fine to keep the others
> definitions, as they are just mirroring what exists in the SD spec.
> Right?
>
> >  };
> >
> >  struct sd_ext_reg {
> > @@ -213,7 +220,35 @@ struct sd_ext_reg {
> >
> >  struct sd_uhs2_config {
> >         u32                     node_id;
> > -       /* TODO: Extend with more register configs. */
> > +
> > +       u32                     dap;
> > +       u32                     gap;
> > +       u32                     n_fcu;
> > +       u32                     maxblk_len;
> > +       u8                      n_lanes;
> > +       u8                      dadr_len;
> > +       u8                      app_type;
> > +       u8                      phy_minor_rev;
> > +       u8                      phy_major_rev;
> > +       u8                      can_hibernate;
> > +       u8                      n_lss_sync;
> > +       u8                      n_lss_dir;
> > +       u8                      link_minor_rev;
> > +       u8                      link_major_rev;
> > +       u8                      dev_type;
> > +       u8                      n_data_gap;
> > +
> > +       u32                     n_fcu_set;
> > +       u32                     maxblk_len_set;
> > +       u8                      n_lanes_set;
> > +       u8                      speed_range_set;
> > +       u8                      n_lss_sync_set;
> > +       u8                      n_lss_dir_set;
> > +       u8                      n_data_gap_set;
> > +       u8                      pwrctrl_mode_set;
> > +       u8                      max_retry_set;
> > +
> > +       u8                      cfg_complete;
>
> There seems to be a couple of variables in the above struct that are
> unused (like dap, gap, cfg_complete, etc). Can we drop them?
>

I will drop it and update in V7 version.

> >  };
> >
> >  struct sdio_cccr {
> > @@ -323,6 +358,9 @@ struct mmc_card {
> >         struct sd_ext_reg       ext_perf;       /* SD extension reg for PERF */
> >
> >         struct sd_uhs2_config   uhs2_config;    /* SD UHS-II config */
> > +       u8                      uhs2_state;     /* SD UHS-II states */
> > +#define MMC_UHS2_INITIALIZED   BIT(1)
>
> Rather than introducing a new variable in the struct mmc_card for
> this, I suggest we add a new bool in the struct mmc_host for this. If
> I understand correctly, this flag indicates that the host
> communication is prepared to support SD-tran commands? Perhaps also
> name the variable to "uhs2_sd_tran", to try to reflect its purpose?
>
> Moreover, it looks to me that sd_uhs2_power_off() would be a good
> place for where to reset the flag, rather than having that sprinkled
> across several error paths.
>
> > +#define MMC_UHS2_SPEED_B       BIT(2)
>
> I was looking through the series to figure out how this bit is being
> used. So, it gets set in sd_uhs2_config_write(), based upon whether
> the host and the card, both supports UHS2 speed mode B. Then, in
> sd_uhs2_config_write() we calls ->uhs2_control(host, UHS2_SET_CONFIG).
>
> By looking at the changes for the sdhci driver, the MMC_UHS2_SPEED_B
> bit gets checked at "UHS2_SET_CONFIG" through the ->uhs2_control()
> callbacks, to determine whether the SDHCI_UHS2_PHY_SET_SPEED_B bit
> should be written to the sdhci_uhs2_phy_set_reg or not. This seems
> reasonable to me. However, I wonder if the call to
> ->uhs2_control(host, UHS2_SET_SPEED_B) in sd_uhs2_change_speed() is
> then really needed, as it looks like that would just be re-writing the
> same bit again in sdhci. Is this deliberate and needed? If not, should
> we drop the call to ->uhs2_control(host, UHS2_SET_SPEED_B)?
>
> When it comes to the actual MMC_UHS2_SPEED_B bit, I think it better
> belongs in the ios->timing variable. In this regard, I suggest we add
> the below bits, to indicate the current UHS2 speed mode.
>
> MMC_TIMING_UHS2_SPEED_A
> MMC_TIMING_UHS2_SPEED_A_HD (HD for half-duplex)
> MMC_TIMING_UHS2_SPEED_B
> MMC_TIMING_UHS2_SPEED_B_HD (HD for half-duplex)
>
> These should help us to support all the various modes, for now. Let's
> leave UHS2 speed range C and D for later.
>
> I would also suggest extending mmc_card_uhs2(), that is introduced in
> patch3, to return true, if any of the bits above are being set.
>
> Finally, in sd_uhs2_power_up() I think it would be better to set the
> ->ios.timing to MMC_TIMING_UHS2_SPEED_A, as the initial value, rather
> than MMC_TIMING_SD_UHS2 (as was introduced in patch2). In other words,
> let's drop MMC_TIMING_SD_UHS2 in favor of MMC_TIMING_UHS2_SPEED_A.
>
> >
> >         unsigned int            sdio_funcs;     /* number of SDIO functions */
> >         atomic_t                sdio_funcs_probed; /* number of probed SDIO funcs */
> > @@ -364,4 +402,6 @@ bool mmc_card_is_blockaddr(struct mmc_card *card);
> >  #define mmc_card_sdio(c)       ((c)->type == MMC_TYPE_SDIO)
> >  #define mmc_card_sd_combo(c)   ((c)->type == MMC_TYPE_SD_COMBO)
> >
> > +#define mmc_card_can_poweroff_notify(c)        ((c)->ext_power.feature_support & SD_EXT_POWER_OFF_NOTIFY)
> > +
>
> Please move this to drivers/mmc/core/sd_ops.h and implement this as a
> regular c-function. I also suggest renaming it to
> mmc_sd_can_poweroff_notify().
>
> [...]
>
> > --- a/include/linux/mmc/host.h
> > +++ b/include/linux/mmc/host.h
> > @@ -16,6 +16,7 @@
> >  #include <linux/mmc/pm.h>
> >  #include <linux/dma-direction.h>
> >  #include <linux/blk-crypto-profile.h>
> > +#include <linux/mmc/sd_uhs2.h>
> >
> >  struct mmc_ios {
> >         unsigned int    clock;                  /* clock rate */
> > @@ -96,7 +97,48 @@ struct mmc_clk_phase_map {
> >  };
> >
> >  struct sd_uhs2_caps {
> > -       /* TODO: Add UHS-II capabilities for the host. */
> > +       u32     dap;
> > +       u32     gap;
> > +       u32     group_desc;
> > +       u32     maxblk_len;
> > +       u32     n_fcu;
> > +       u8      n_lanes;
> > +       u8      addr64;
> > +       u8      card_type;
> > +       u8      phy_rev;
> > +       u8      speed_range;
> > +       u8      can_hibernate;
> > +       u8      n_lss_sync;
> > +       u8      n_lss_dir;
> > +       u8      link_rev;
> > +       u8      host_type;
> > +       u8      n_data_gap;
> > +
> > +       u32     maxblk_len_set;
> > +       u32     n_fcu_set;
> > +       u8      n_lanes_set;
> > +       u8      n_lss_sync_set;
> > +       u8      n_lss_dir_set;
> > +       u8      n_data_gap_set;
> > +       u8      max_retry_set;
>
> As I found a few unused variables in the earlier struct definitions,
> please walk through this too, to make sure we don't waste data.
>

I will drop it and update in V7 version.

> > +};
> > +
> > +struct sd_uhs2_ios {
> > +       bool            is_2L_HD_mode;
>
> I think we should use the ios->timing variable for this instead. If
> the two data-lines are supported to run in half-duplex mode, I think
> it's better to use one of the below bits, as I kind of also suggested
> earlier above.
>
> MMC_TIMING_UHS2_SPEED_A_HD
> MMC_TIMING_UHS2_SPEED_B_HD
>
> As "is_2L_HD_mode" seems to be checked at couple of different places
> in the core and in sdhci, perhaps we should add a new helper function,
> "mmc_card_uhs2_hd_mode()" that would return true if any of the above
> bits are set?
>
> > +       bool            is_APP_CMD;
>
> I think this better belongs in the struct mmc_host, please move it
> there and then also rename it into "uhs2_app_cmd". Note that, there is
> also one case in mmc_sd_num_wr_blocks() that sends an MMC_APP_CMD, but
> without calling mmc_app_cmd(). This needs to be fixed.
>
> > +       unsigned int    power_delay_ms;         /* waiting for stable power */
>
> This seems to be unused, so let's drop it!
>

This power_delay_ms is used in the sd_uhs2_power_up function.

> > +};
> > +
> > +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,
> > +       UHS2_SET_SPEED_B,
> > +       UHS2_POST_ATTACH_SD,
> >  };
> >
> >  struct mmc_host;
> > @@ -231,6 +273,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 {
> > @@ -323,6 +373,7 @@ struct mmc_pwrseq;
> >
> >  struct mmc_supply {
> >         struct regulator *vmmc;         /* Card power supply */
> > +       struct regulator *vmmc2;        /* UHS2 VDD2 power supply */
>
> In patch4 I added support for vqmmc2, which is targeted to be used for
> VDD2. Is there any reason that can't be used?
>
> >         struct regulator *vqmmc;        /* Optional Vccq supply */
> >         struct regulator *vqmmc2;       /* Optional supply for phy */
> >  };
> > @@ -344,10 +395,12 @@ struct mmc_host {
> >         u32                     ocr_avail_sdio; /* SDIO-specific OCR */
> >         u32                     ocr_avail_sd;   /* SD-specific OCR */
> >         u32                     ocr_avail_mmc;  /* MMC-specific OCR */
> > +       u32                     ocr_avail_uhs2; /* UHS2-specific OCR */
>
> The way I implemented support for vqmmc2, we shouldn't need a specific
> ocr mask for UHS2. So, unless I am mistaken we should be able to use
> only 1.8V, for now.
>
> >         struct wakeup_source    *ws;            /* Enable consume of uevents */
> >         u32                     max_current_330;
> >         u32                     max_current_300;
> >         u32                     max_current_180;
> > +       u32                     max_current_180_vdd2; /* UHS2 vdd2 max curt. */
>
> This looks sdhci specific. Should we move it to the sdhci struct instead?
>
> >
> >  #define MMC_VDD_165_195                0x00000080      /* VDD voltage 1.65 - 1.95 */
> >  #define MMC_VDD_20_21          0x00000100      /* VDD voltage 2.0 ~ 2.1 */
> > @@ -366,6 +419,7 @@ struct mmc_host {
> >  #define MMC_VDD_33_34          0x00200000      /* VDD voltage 3.3 ~ 3.4 */
> >  #define MMC_VDD_34_35          0x00400000      /* VDD voltage 3.4 ~ 3.5 */
> >  #define MMC_VDD_35_36          0x00800000      /* VDD voltage 3.5 ~ 3.6 */
> > +#define MMC_VDD2_165_195       0x00000080      /* UHS2 VDD2 1.65 ~ 1.95 */
>
> Please drop this. We already have MMC_VDD_165_195, for these bits, see above.
>

I will drop it and update in V7 version.

> >
> >         u32                     caps;           /* Host capabilities */
> >
> > @@ -443,7 +497,11 @@ struct mmc_host {
> >  #endif
> >  #define MMC_CAP2_ALT_GPT_TEGRA (1 << 28)       /* Host with eMMC that has GPT entry at a non-standard location */
> >
> > +       int flags;
> > +#define MMC_UHS2_SUPPORT       (1 << 0)
>
> According to my comments above, I don't think this is needed at all.
> Please drop it.
>
> > +
> >         struct sd_uhs2_caps     uhs2_caps;      /* Host UHS-II capabilities */
> > +       struct sd_uhs2_ios      uhs2_ios;       /* Host UHS-II capabilities */
>
> As indicated above, the struct sd_uhs2_ios can be dropped.
>
> >
> >         int                     fixed_drv_type; /* fixed driver type for non-removable media */
> >
> > @@ -695,4 +753,7 @@ int mmc_send_tuning(struct mmc_host *host, u32 opcode, int *cmd_error);
> >  int mmc_send_abort_tuning(struct mmc_host *host, u32 opcode);
> >  int mmc_get_ext_csd(struct mmc_card *card, u8 **new_ext_csd);
> >
> > +#define mmc_uhs2_2L_HD_mode(h) ((h)->uhs2_ios.is_2L_HD_mode)
> > +#define mmc_uhs2_APP_cmd(h)    ((h)->uhs2_ios.is_APP_CMD)
> > +
>
> These are unused. Please drop or replace them with something that can be used.
>

I will drop it and update in V7 version.

> >  #endif /* LINUX_MMC_HOST_H */
> > diff --git a/include/linux/mmc/sd_uhs2.h b/include/linux/mmc/sd_uhs2.h
> > new file mode 100644
> > index 000000000000..8fcf702cf4a5
> > --- /dev/null
> > +++ b/include/linux/mmc/sd_uhs2.h
> > @@ -0,0 +1,263 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * Header file for UHS-II packets, Host Controller registers and I/O
> > + * accessors.
> > + *
> > + *  Copyright (C) 2014 Intel Corp, All Rights Reserved.
> > + */
> > +#ifndef LINUX_MMC_UHS2_H
> > +#define LINUX_MMC_UHS2_H
> > +
> > +/* LINK Layer definition */
> > +/*
> > + * UHS2 Header:
> > + * Refer to UHS-II Addendum Version 1.02 Figure 5-2, the format of CCMD Header is described below:
> > + *      bit [3:0]  : DID(Destination ID = Node ID of UHS2 card)
> > + *      bit [6:4]  : TYP(Packet Type)
> > + *                   000b: CCMD(Control command packet)
> > + *                   001b: DCMD(Data command packet)
> > + *                   010b: RES(Response packet)
> > + *                   011b: DATA(Data payload packet)
> > + *                   111b: MSG(Message packet)
> > + *                   Others: Reserved
> > + *      bit [7]    : NP(Native Packet)
> > + *      bit [10:8] : TID(Transaction ID)
> > + *      bit [11]   : Reserved
> > + *      bit [15:12]: SID(Source ID 0: Node ID of Host)
> > + *
> > + * Broadcast CCMD issued by Host is represented as DID=SID=0.
> > + */
> > +/*
> > + * UHS2 Argument:
> > + * Refer to UHS-II Addendum Version 1.02 Figure 6-5, the format of CCMD Argument is described below:
> > + *      bit [3:0]  : MSB of IOADR
> > + *      bit [5:4]  : PLEN(Payload Length)
> > + *                   00b: 0 byte
> > + *                   01b: 4 bytes
> > + *                   10b: 8 bytes
> > + *                   11b: 16 bytes
> > + *      bit [6]    : Reserved
> > + *      bit [7]    : R/W(Read/Write)
> > + *                   0: Control read command
> > + *                   1: Control write command
> > + *      bit [15:8] : LSB of IOADR
> > + *
> > + * I/O Address specifies the address of register in UHS-II I/O space accessed by CCMD.
> > + * The unit of I/O Address is 4 Bytes. It is transmitted in MSB first, LSB last.
> > + */
> > +#define UHS2_NATIVE_PACKET_POS 7
> > +#define UHS2_NATIVE_PACKET     (1 << UHS2_NATIVE_PACKET_POS)
> > +
> > +#define UHS2_PACKET_TYPE_POS   4
> > +#define UHS2_PACKET_TYPE_CCMD  (0 << UHS2_PACKET_TYPE_POS)
> > +#define UHS2_PACKET_TYPE_DCMD  (1 << UHS2_PACKET_TYPE_POS)
> > +#define UHS2_PACKET_TYPE_RES   (2 << UHS2_PACKET_TYPE_POS)
> > +#define UHS2_PACKET_TYPE_DATA  (3 << UHS2_PACKET_TYPE_POS)
> > +#define UHS2_PACKET_TYPE_MSG   (7 << UHS2_PACKET_TYPE_POS)
> > +
> > +#define UHS2_DEST_ID_MASK      0x0F
> > +#define UHS2_DEST_ID           0x1
> > +
> > +#define UHS2_SRC_ID_POS                12
> > +#define UHS2_SRC_ID_MASK       0xF000
> > +
> > +#define UHS2_TRANS_ID_POS      8
> > +#define UHS2_TRANS_ID_MASK     0x0700
> > +
> > +/* UHS2 MSG */
> > +#define UHS2_MSG_CTG_POS       5
> > +#define UHS2_MSG_CTG_LMSG      0x00
> > +#define UHS2_MSG_CTG_INT       0x60
> > +#define UHS2_MSG_CTG_AMSG      0x80
> > +
> > +#define UHS2_MSG_CTG_FCREQ     0x00
> > +#define UHS2_MSG_CTG_FCRDY     0x01
> > +#define UHS2_MSG_CTG_STAT      0x02
> > +
> > +#define UHS2_MSG_CODE_POS                      8
> > +#define UHS2_MSG_CODE_FC_UNRECOVER_ERR         0x8
> > +#define UHS2_MSG_CODE_STAT_UNRECOVER_ERR       0x8
> > +#define UHS2_MSG_CODE_STAT_RECOVER_ERR         0x1
> > +
> > +/* TRANS Layer definition */
> > +
> > +/* Native packets*/
> > +#define UHS2_NATIVE_CMD_RW_POS 7
> > +#define UHS2_NATIVE_CMD_WRITE  (1 << UHS2_NATIVE_CMD_RW_POS)
> > +#define UHS2_NATIVE_CMD_READ   (0 << UHS2_NATIVE_CMD_RW_POS)
> > +
> > +#define UHS2_NATIVE_CMD_PLEN_POS       4
> > +#define UHS2_NATIVE_CMD_PLEN_4B                (1 << UHS2_NATIVE_CMD_PLEN_POS)
> > +#define UHS2_NATIVE_CMD_PLEN_8B                (2 << UHS2_NATIVE_CMD_PLEN_POS)
> > +#define UHS2_NATIVE_CMD_PLEN_16B       (3 << UHS2_NATIVE_CMD_PLEN_POS)
> > +
> > +#define UHS2_NATIVE_CCMD_GET_MIOADR_MASK       0xF00
> > +#define UHS2_NATIVE_CCMD_MIOADR_MASK           0x0F
> > +
> > +#define UHS2_NATIVE_CCMD_LIOADR_POS            8
> > +#define UHS2_NATIVE_CCMD_GET_LIOADR_MASK       0x0FF
> > +
> > +#define UHS2_CCMD_DEV_INIT_COMPLETE_FLAG       BIT(11)
> > +#define UHS2_DEV_INIT_PAYLOAD_LEN              1
> > +#define UHS2_DEV_INIT_RESP_LEN                 6
> > +#define UHS2_DEV_ENUM_PAYLOAD_LEN              1
> > +#define UHS2_DEV_ENUM_RESP_LEN                 8
> > +#define UHS2_CFG_WRITE_PAYLOAD_LEN             2
> > +#define UHS2_CFG_WRITE_PHY_SET_RESP_LEN                4
> > +#define UHS2_CFG_WRITE_GENERIC_SET_RESP_LEN    5
> > +#define UHS2_GO_DORMANT_PAYLOAD_LEN            1
> > +
> > +/*
> > + * UHS2 Argument:
> > + * Refer to UHS-II Addendum Version 1.02 Figure 6-8, the format of DCMD Argument is described below:
> > + *      bit [3:0]  : Reserved
> > + *      bit [6:3]  : TMODE(Transfer Mode)
> > + *                   bit 3: DAM(Data Access Mode)
> > + *                   bit 4: TLUM(TLEN Unit Mode)
> > + *                   bit 5: LM(Length Mode)
> > + *                   bit 6: DM(Duplex Mode)
> > + *      bit [7]    : R/W(Read/Write)
> > + *                   0: Control read command
> > + *                   1: Control write command
> > + *      bit [15:8] : Reserved
> > + *
> > + * I/O Address specifies the address of register in UHS-II I/O space accessed by CCMD.
> > + * The unit of I/O Address is 4 Bytes. It is transmitted in MSB first, LSB last.
> > + */
> > +#define UHS2_DCMD_DM_POS               6
> > +#define UHS2_DCMD_2L_HD_MODE           (1 << UHS2_DCMD_DM_POS)
> > +#define UHS2_DCMD_LM_POS               5
> > +#define UHS2_DCMD_LM_TLEN_EXIST                (1 << UHS2_DCMD_LM_POS)
> > +#define UHS2_DCMD_TLUM_POS             4
> > +#define UHS2_DCMD_TLUM_BYTE_MODE       (1 << UHS2_DCMD_TLUM_POS)
> > +#define UHS2_NATIVE_DCMD_DAM_POS       3
> > +#define UHS2_NATIVE_DCMD_DAM_IO                (1 << UHS2_NATIVE_DCMD_DAM_POS)
> > +
> > +/*
> > + * Per UHS2 spec, DCMD payload should be MSB first. There may be
> > + * two types of data be assembled to MSB:
> > + * 1. TLEN: Input block size for single read/write and number of blocks
> > + * for multiple read/write to calculate TLEN as MSB first per spec.
> > + * 2. SD command argument.
> > + */
> > +static inline __be32 uhs2_dcmd_convert_msb(u32 input)
> > +{
> > +       u32 ret = 0;
> > +
> > +       ret = ((input & 0xFF) << 24) |
> > +               (((input >> 8) & 0xFF) << 16) |
> > +               (((input >> 16) & 0xFF) << 8) |
> > +               ((input >> 24) & 0xFF);
> > +       return cpu_to_be32(ret);
> > +}
>
> This looks unused, can we drop it?
>

I will drop it and update in V7 version.

> > +
> > +#define UHS2_RES_NACK_POS      7
> > +#define UHS2_RES_NACK_MASK     (0x1 << UHS2_RES_NACK_POS)
> > +
> > +#define UHS2_RES_ECODE_POS     4
> > +#define UHS2_RES_ECODE_MASK    0x7
> > +#define UHS2_RES_ECODE_COND    1
> > +#define UHS2_RES_ECODE_ARG     2
> > +#define UHS2_RES_ECODE_GEN     3
> > +
> > +/* IOADR of device registers */
> > +#define UHS2_IOADR_GENERIC_CAPS                0x00
> > +#define UHS2_IOADR_PHY_CAPS            0x02
> > +#define UHS2_IOADR_LINK_CAPS           0x04
> > +#define UHS2_IOADR_RSV_CAPS            0x06
> > +#define UHS2_IOADR_GENERIC_SETTINGS    0x08
> > +#define UHS2_IOADR_PHY_SETTINGS                0x0A
> > +#define UHS2_IOADR_LINK_SETTINGS       0x0C
> > +#define UHS2_IOADR_PRESET              0x40
> > +
> > +/* SD application packets */
> > +#define UHS2_SD_CMD_INDEX_POS  8
> > +
> > +#define UHS2_SD_CMD_APP_POS    14
> > +#define UHS2_SD_CMD_APP                (1 << UHS2_SD_CMD_APP_POS)
> > +
> > +/* UHS-II Device Registers */
> > +#define UHS2_DEV_CONFIG_REG    0x000
> > +
> > +/* General Caps and Settings registers */
> > +#define UHS2_DEV_CONFIG_GEN_CAPS       (UHS2_DEV_CONFIG_REG + 0x000)
> > +#define UHS2_DEV_CONFIG_N_LANES_POS    8
> > +#define UHS2_DEV_CONFIG_N_LANES_MASK   0x3F
> > +#define UHS2_DEV_CONFIG_2L_HD_FD       0x1
> > +#define UHS2_DEV_CONFIG_2D1U_FD                0x2
> > +#define UHS2_DEV_CONFIG_1D2U_FD                0x4
> > +#define UHS2_DEV_CONFIG_2D2U_FD                0x8
> > +#define UHS2_DEV_CONFIG_DADR_POS       14
> > +#define UHS2_DEV_CONFIG_DADR_MASK      0x1
> > +#define UHS2_DEV_CONFIG_APP_POS                16
> > +#define UHS2_DEV_CONFIG_APP_MASK       0xFF
> > +#define UHS2_DEV_CONFIG_APP_SD_MEM     0x1
> > +
> > +#define UHS2_DEV_CONFIG_GEN_SET                        (UHS2_DEV_CONFIG_REG + 0x008)
> > +#define UHS2_DEV_CONFIG_GEN_SET_N_LANES_POS    8
> > +#define UHS2_DEV_CONFIG_GEN_SET_2L_FD_HD       0x0
> > +#define UHS2_DEV_CONFIG_GEN_SET_2D1U_FD                0x2
> > +#define UHS2_DEV_CONFIG_GEN_SET_1D2U_FD                0x3
> > +#define UHS2_DEV_CONFIG_GEN_SET_2D2U_FD                0x4
> > +#define UHS2_DEV_CONFIG_GEN_SET_CFG_COMPLETE   BIT(31)
> > +
> > +/* PHY Caps and Settings registers */
> > +#define UHS2_DEV_CONFIG_PHY_CAPS       (UHS2_DEV_CONFIG_REG + 0x002)
> > +#define UHS2_DEV_CONFIG_PHY_MINOR_MASK 0xF
> > +#define UHS2_DEV_CONFIG_PHY_MAJOR_POS  4
> > +#define UHS2_DEV_CONFIG_PHY_MAJOR_MASK 0x3
> > +#define UHS2_DEV_CONFIG_CAN_HIBER_POS  15
> > +#define UHS2_DEV_CONFIG_CAN_HIBER_MASK 0x1
> > +#define UHS2_DEV_CONFIG_PHY_CAPS1      (UHS2_DEV_CONFIG_REG + 0x003)
> > +#define UHS2_DEV_CONFIG_N_LSS_SYN_MASK 0xF
> > +#define UHS2_DEV_CONFIG_N_LSS_DIR_POS  4
> > +#define UHS2_DEV_CONFIG_N_LSS_DIR_MASK 0xF
> > +
> > +#define UHS2_DEV_CONFIG_PHY_SET                        (UHS2_DEV_CONFIG_REG + 0x00A)
> > +#define UHS2_DEV_CONFIG_PHY_SET_SPEED_POS      6
> > +#define UHS2_DEV_CONFIG_PHY_SET_SPEED_A                0x0
> > +#define UHS2_DEV_CONFIG_PHY_SET_SPEED_B                0x1
> > +
> > +/* LINK-TRAN Caps and Settings registers */
> > +#define UHS2_DEV_CONFIG_LINK_TRAN_CAPS         (UHS2_DEV_CONFIG_REG + 0x004)
> > +#define UHS2_DEV_CONFIG_LT_MINOR_MASK          0xF
> > +#define UHS2_DEV_CONFIG_LT_MAJOR_POS           4
> > +#define UHS2_DEV_CONFIG_LT_MAJOR_MASK          0x3
> > +#define UHS2_DEV_CONFIG_N_FCU_POS              8
> > +#define UHS2_DEV_CONFIG_N_FCU_MASK             0xFF
> > +#define UHS2_DEV_CONFIG_DEV_TYPE_POS           16
> > +#define UHS2_DEV_CONFIG_DEV_TYPE_MASK          0x7
> > +#define UHS2_DEV_CONFIG_MAX_BLK_LEN_POS                20
> > +#define UHS2_DEV_CONFIG_MAX_BLK_LEN_MASK       0xFFF
> > +#define UHS2_DEV_CONFIG_LINK_TRAN_CAPS1                (UHS2_DEV_CONFIG_REG + 0x005)
> > +#define UHS2_DEV_CONFIG_N_DATA_GAP_MASK                0xFF
> > +
> > +#define UHS2_DEV_CONFIG_LINK_TRAN_SET          (UHS2_DEV_CONFIG_REG + 0x00C)
> > +#define UHS2_DEV_CONFIG_LT_SET_MAX_BLK_LEN     0x200
> > +#define UHS2_DEV_CONFIG_LT_SET_MAX_RETRY_POS   16
> > +
> > +/* Preset register */
> > +#define UHS2_DEV_CONFIG_PRESET (UHS2_DEV_CONFIG_REG + 0x040)
> > +
> > +#define UHS2_DEV_INT_REG       0x100
> > +
> > +#define UHS2_DEV_STATUS_REG    0x180
> > +
> > +#define UHS2_DEV_CMD_REG               0x200
> > +#define UHS2_DEV_CMD_FULL_RESET                (UHS2_DEV_CMD_REG + 0x000)
> > +#define UHS2_DEV_CMD_GO_DORMANT_STATE  (UHS2_DEV_CMD_REG + 0x001)
> > +#define UHS2_DEV_CMD_DORMANT_HIBER     BIT(7)
> > +#define UHS2_DEV_CMD_DEVICE_INIT       (UHS2_DEV_CMD_REG + 0x002)
> > +#define UHS2_DEV_INIT_COMPLETE_FLAG    BIT(11)
> > +#define UHS2_DEV_CMD_ENUMERATE         (UHS2_DEV_CMD_REG + 0x003)
> > +#define UHS2_DEV_CMD_TRANS_ABORT       (UHS2_DEV_CMD_REG + 0x004)
> > +
> > +#define UHS2_RCLK_MAX  52000000
> > +#define UHS2_RCLK_MIN  26000000
> > +
> > +struct sd_uhs2_wait_active_state_data {
> > +       struct mmc_host *host;
> > +       struct mmc_command *cmd;
> > +};
>
> Please move this to drivers/mmc/core/sd_uhs2.c.
>

I will update it in V7 version.

> > +
> > +#endif /* LINUX_MMC_UHS2_H */
> > --
> > 2.25.1
> >
>
> I plan to have a closer look at the VDD2 things - but I think there
> should be plenty of stuff for us to discuss and hopefully for you to
> continue to work on, in the meantime.
>
> Kind regards
> Uffe

Thanks, Victor Shih

[...]

> >
> > I think this better belongs in the struct mmc_host, please move it
> > there and then also rename it into "uhs2_app_cmd". Note that, there is
> > also one case in mmc_sd_num_wr_blocks() that sends an MMC_APP_CMD, but
> > without calling mmc_app_cmd(). This needs to be fixed.
> >
> > > +       unsigned int    power_delay_ms;         /* waiting for stable power */
> >
> > This seems to be unused, so let's drop it!
> >
>
> This power_delay_ms is used in the sd_uhs2_power_up function.
>

Correct. Although, the value isn't being set, which means it's always
0. Is that deliberate?

I realize that we are going to end up calling msleep(0) for this case,
so there is a delay being done anyway.

[...]

Kind regards
Uffe