[V3,6/7] mmc: Implement content of UHS-II card initialization functions

From: Jason Lai <jason.lai@genesyslogic.com.tw>

UHS-II card initialization flow is divided into 2 categories: PHY & Card.
Part 1 - PHY Initialization:
         Every host controller may need their own avtivation operation to
         establish LINK between controller and card. So we add a new member
         function(uhs2_detect_init) in struct mmc_host_ops for host
         controller use.
Part 2 - Card Initialization:
         This part can be divided into 6 substeps.
         1. Send UHS-II CCMD DEVICE_INIT to card.
         2. Send UHS-II CCMD ENUMERATE to card.
         3. Send UHS-II Native Read CCMD to obtain capabilities in CFG_REG
            of card.
         4. Host compares capabilities of host controller and card, then
            write the negotiated values to Setting field in CFG_REG of card
            through UHS-II Native Write CCMD.
         5. Switch host controller's clock to Range B if it is supported by
            both host controller and card.
         6. Execute legacy SD initialization flow.
Part 3 - Provide a function to tranaform legacy SD command packet into
         UHS-II SD-TRAN DCMD packet.

Most of the code added above came from Intel's original patch[3].

[3]
https://patchwork.kernel.org/project/linux-mmc/patch/1419672479-30852-2-
git-send-email-yi.y.sun@intel.com/

Signed-off-by: Jason Lai <jason.lai@genesyslogic.com.tw>
---
 drivers/mmc/core/sd_uhs2.c | 835 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 817 insertions(+), 18 deletions(-)

On Tue, 22 Feb 2022 at 04:40, Jason Lai <jasonlai.genesyslogic@gmail.com> wrote:
>
> From: Jason Lai <jason.lai@genesyslogic.com.tw>
>
> UHS-II card initialization flow is divided into 2 categories: PHY & Card.
> Part 1 - PHY Initialization:
>          Every host controller may need their own avtivation operation to
>          establish LINK between controller and card. So we add a new member
>          function(uhs2_detect_init) in struct mmc_host_ops for host
>          controller use.
> Part 2 - Card Initialization:
>          This part can be divided into 6 substeps.
>          1. Send UHS-II CCMD DEVICE_INIT to card.
>          2. Send UHS-II CCMD ENUMERATE to card.
>          3. Send UHS-II Native Read CCMD to obtain capabilities in CFG_REG
>             of card.
>          4. Host compares capabilities of host controller and card, then
>             write the negotiated values to Setting field in CFG_REG of card
>             through UHS-II Native Write CCMD.
>          5. Switch host controller's clock to Range B if it is supported by
>             both host controller and card.
>          6. Execute legacy SD initialization flow.
> Part 3 - Provide a function to tranaform legacy SD command packet into
>          UHS-II SD-TRAN DCMD packet.
>
> Most of the code added above came from Intel's original patch[3].
>
> [3]
> https://patchwork.kernel.org/project/linux-mmc/patch/1419672479-30852-2-
> git-send-email-yi.y.sun@intel.com/
>
> Signed-off-by: Jason Lai <jason.lai@genesyslogic.com.tw>
> ---
>  drivers/mmc/core/sd_uhs2.c | 835 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 817 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/mmc/core/sd_uhs2.c b/drivers/mmc/core/sd_uhs2.c
> index 800957f74632..f1e8e30301eb 100644
> --- a/drivers/mmc/core/sd_uhs2.c
> +++ b/drivers/mmc/core/sd_uhs2.c
> @@ -3,6 +3,7 @@
>   * Copyright (C) 2021 Linaro Ltd
>   *
>   * Author: Ulf Hansson <ulf.hansson@linaro.org>
> + * Author: Jason Lai <jason.lai@genesyslogic.com.tw>
>   *
>   * Support for SD UHS-II cards
>   */
> @@ -10,19 +11,31 @@
>
>  #include <linux/mmc/host.h>
>  #include <linux/mmc/card.h>
> +#include <linux/mmc/mmc.h>
> +#include <linux/mmc/sd_uhs2.h>
>
>  #include "core.h"
>  #include "bus.h"
> +#include "card.h"
>  #include "sd.h"
> +#include "sd_ops.h"
>  #include "mmc_ops.h"
> +#include "sd_uhs2.h"
>
>  static const unsigned int sd_uhs2_freqs[] = { 52000000, 26000000 };
>
>  static int sd_uhs2_set_ios(struct mmc_host *host)
>  {
>         struct mmc_ios *ios = &host->ios;
> +       int err = 0;
>
> -       return host->ops->uhs2_set_ios(host, ios);
> +       pr_debug("%s: clock %uHz powermode %u Vdd %u timing %u\n",
> +                mmc_hostname(host), ios->clock, ios->power_mode, ios->vdd,
> +                ios->timing);
> +
> +       host->ops->set_ios(host, ios);

We discussed using the ->set_ios() callback in a previous version. To
repeat myself, I don't think it's a good idea. UHS-II needs an
entirely different power sequence than the legacy interface(s), hence
I think it's simply cleaner to separate them.

To move forward, I see two options.
1) Use only the ->uhs2_host_operation() ops.
2) Use a combination of the ->uhs2_set_ios() ops and the
->uhs2_host_operation() ops.

Both options work for me. However, perhaps if you could incorporate
the changes done on the host driver at next submission, it becomes
easier for me to understand what makes best sense.

> +
> +       return err;
>  }
>
>  static int sd_uhs2_power_up(struct mmc_host *host)
> @@ -45,6 +58,43 @@ static void sd_uhs2_power_off(struct mmc_host *host)
>         sd_uhs2_set_ios(host);
>  }

[...]

>
>  /*
> @@ -61,6 +119,77 @@ static int sd_uhs2_phy_init(struct mmc_host *host)
>   */
>  static int sd_uhs2_dev_init(struct mmc_host *host)
>  {
> +       struct mmc_command cmd = {0};
> +       struct uhs2_command uhs2_cmd = {};
> +       u32 cnt;
> +       u32 dap, gap, resp_gap;
> +       u16 header = 0, arg = 0;

No need to initiate these.

> +       u32 payload[1];

u32?

> +       u8 plen = 1;
> +       u8 gd = 0, cf = 1;
> +       u8 resp[6] = {0};
> +       u8 resp_len = 6;

Many of these variables are just constant numbers. If it makes sense
to add definitions for them, then please do that instead. If not, just
give the value directly in the code.

For example: plen = 1; (I assume that is payload length). This can
just be given as an in-parameter to sd_uhs2_cmd_assemble(), without
further explanation.

The point is, sd_uhs2_cmd_assemble() should have a well described
description of its in-parameters, so no need for further descriptions,
I think.

This comment applies to all the new code/functions that are added in
the $subject patch. Please go through all of the code and fix this.


> +       int err;
> +
> +       dap = host->uhs2_caps.dap;
> +       gap = host->uhs2_caps.gap;
> +
> +       header = UHS2_NATIVE_PACKET | UHS2_PACKET_TYPE_CCMD;
> +       arg = ((UHS2_DEV_CMD_DEVICE_INIT & 0xFF) << 8) |
> +              UHS2_NATIVE_CMD_WRITE |
> +              UHS2_NATIVE_CMD_PLEN_4B |
> +              (UHS2_DEV_CMD_DEVICE_INIT >> 8);
> +
> +       /*
> +        * Refer to UHS-II Addendum Version 1.02 section 6.3.1.
> +        * Max. time from DEVICE_INIT CCMD EOP reception on Device
> +        * Rx to its SOP transmission on Device Tx(Tfwd_init_cmd) is
> +        * 1 second.
> +        */
> +       cmd.busy_timeout = 1000;
> +
> +       /*
> +        * Refer to UHS-II Addendum Version 1.02 section 6.2.6.3.
> +        * When the number of the DEVICE_INIT commands is reach to
> +        * 30 tiems, Host shall stop issuing DEVICE_INIT command
> +        * and regard it as an error.
> +        */
> +       for (cnt = 0; cnt < 30; cnt++) {
> +               payload[0] = ((dap & 0xF) << 12) |
> +                             (cf << 11)         |
> +                             ((gd & 0xF) << 4)  |
> +                             (gap & 0xF);

To me, it looks like the payload data deserves to be explained a bit.
Perhaps you can add a comment explaining what pieces it consists of so
this becomes more clear?

> +
> +               sd_uhs2_cmd_assemble(&cmd, &uhs2_cmd, header, arg,  payload, plen, resp, resp_len);
> +
> +               err = mmc_wait_for_cmd(host, &cmd, 0);
> +
> +               if (err) {
> +                       pr_err("%s: %s: UHS2 CMD send fail, err= 0x%x!\n",
> +                              mmc_hostname(host), __func__, err);
> +                       return -EIO;

Why do you override the original error code that was returned from
mmc_wait_for_cmd()?

Normally it's preferred to keep the error code, unless there is good
reason not to.

Again, I won't add more comments like this in the code from the
$subject patch. But please go through it all to avoid this kind of
thing.

> +               }
> +
> +               if (resp[3] != (UHS2_DEV_CMD_DEVICE_INIT & 0xFF)) {
> +                       pr_err("%s: DEVICE_INIT response is wrong!\n",
> +                              mmc_hostname(host));
> +                       return -EIO;
> +               }
> +
> +               if (resp[5] & 0x8) {
> +                       host->uhs2_caps.group_desc = gd;
> +                       break;

I suggest you do a return 0 here. In this way you can skip the check
"if (cnt == 30)" below and just return an error code instead.

> +               }
> +               resp_gap = resp[4] & 0x0F;
> +               if (gap == resp_gap)
> +                       gd++;
> +       }
> +       if (cnt == 30) {
> +               pr_err("%s: DEVICE_INIT fail, already 30 times!\n",
> +                      mmc_hostname(host));
> +               return -EIO;
> +       }
> +
>         return 0;
>  }
>

>  static int sd_uhs2_config_read(struct mmc_host *host, struct mmc_card *card)
>  {
> +       struct mmc_command cmd = {0};
> +       struct uhs2_command uhs2_cmd = {};
> +       u16 header = 0, arg = 0;
> +       u32 cap;
> +       int err;
> +
> +       header = UHS2_NATIVE_PACKET |
> +                UHS2_PACKET_TYPE_CCMD |
> +                card->uhs2_config.node_id;
> +       arg = ((UHS2_DEV_CONFIG_GEN_CAPS & 0xFF) << 8) |
> +              UHS2_NATIVE_CMD_READ |
> +              UHS2_NATIVE_CMD_PLEN_4B |
> +              (UHS2_DEV_CONFIG_GEN_CAPS >> 8);
> +
> +       /* There is no payload because per spec, there should be
> +        * no payload field for read CCMD.
> +        * Plen is set in arg. Per spec, plen for read CCMD
> +        * represents the len of read data which is assigned in payload
> +        * of following RES (p136).
> +        */
> +       sd_uhs2_cmd_assemble(&cmd, &uhs2_cmd, header, arg, NULL, 0, NULL, 0);

We are reading the configuration data here and onwards, piece by
piece. Perhaps if you can add a small comment about each piece we are
reading, before each call to mmc_wait_for_cmd(), that can help to
easier understand what goes on.

[...]

>  static int sd_uhs2_config_write(struct mmc_host *host, struct mmc_card *card)
>  {
> +       struct mmc_command cmd = {0};
> +       struct uhs2_command uhs2_cmd = {};
> +       u16 header = 0, arg = 0;
> +       u32 payload[2];
> +       u8 nMinDataGap;
> +       u8 plen;
> +       int err;
> +       u8 resp[5] = {0};
> +       u8 resp_len = 5;
> +
> +       header = UHS2_NATIVE_PACKET |
> +                UHS2_PACKET_TYPE_CCMD | card->uhs2_config.node_id;
> +       arg = ((UHS2_DEV_CONFIG_GEN_SET & 0xFF) << 8) |
> +              UHS2_NATIVE_CMD_WRITE |
> +              UHS2_NATIVE_CMD_PLEN_8B |
> +              (UHS2_DEV_CONFIG_GEN_SET >> 8);
> +
> +       if (card->uhs2_config.n_lanes == UHS2_DEV_CONFIG_2L_HD_FD &&
> +           host->uhs2_caps.n_lanes == UHS2_DEV_CONFIG_2L_HD_FD) {
> +               /* Support HD */
> +               host->uhs2_caps.flags |= MMC_UHS2_2L_HD;

How is the uhs2_caps.flags field intended to be used? To me it looks
like a way for the mmc core to exchange status/configuration
information about the initialization process of the card, with the mmc
host driver. Perhaps there is more too. Is that correct?

If so, I think it looks quite similar for what we have in the struct
mmc_ios, for the legacy interface(s). I am not saying we should use
that, just trying to understand what would suit best here.

> +               nMinDataGap = 1;
> +       } else {
> +               /* Only support 2L-FD so far */
> +               host->uhs2_caps.flags &= ~MMC_UHS2_2L_HD;
> +               nMinDataGap = 3;
> +       }
> +
> +       /*
> +        * Most UHS-II cards only support FD and 2L-HD mode. Other lane numbers
> +        * defined in UHS-II addendem Ver1.01 are optional.
> +        */
> +       host->uhs2_caps.n_lanes_set = UHS2_DEV_CONFIG_GEN_SET_2L_FD_HD;
> +       card->uhs2_config.n_lanes_set = UHS2_DEV_CONFIG_GEN_SET_2L_FD_HD;

[...]

> +static int sd_uhs2_go_dormant(struct mmc_host *host, bool hibernate, u32 node_id)
> +{

Looks like the in-parameter "hibernate" is superfluous, as it's always
set to "false" by the caller.

> +       struct mmc_command cmd = {0};
> +       struct uhs2_command uhs2_cmd = {};
> +       u16 header = 0, arg = 0;
> +       u32 payload[1];
> +       u8 plen = 1;
> +       int err;
> +
> +       /* Disable Normal INT */
> +       if (!host->ops->uhs2_host_operation(host, UHS2_DISABLE_INT)) {
> +               pr_err("%s: %s: UHS2 DISABLE_INT fail!\n",
> +                      mmc_hostname(host), __func__);
> +               return -EIO;
> +       }
> +
> +       header = UHS2_NATIVE_PACKET | UHS2_PACKET_TYPE_CCMD | node_id;
> +
> +       arg = ((UHS2_DEV_CMD_GO_DORMANT_STATE & 0xFF) << 8) |
> +               UHS2_NATIVE_CMD_WRITE |
> +               UHS2_NATIVE_CMD_PLEN_4B |
> +               (UHS2_DEV_CMD_GO_DORMANT_STATE >> 8);
> +
> +       if (hibernate)
> +               payload[0] = UHS2_DEV_CMD_DORMANT_HIBER;
> +
> +       sd_uhs2_cmd_assemble(&cmd, &uhs2_cmd, header, arg, payload, plen, NULL, 0);
> +
> +       err = mmc_wait_for_cmd(host, &cmd, 0);
> +       if (err) {
> +               pr_err("%s: %s: UHS2 CMD send fail, err= 0x%x!\n",
> +                      mmc_hostname(host), __func__, err);
> +               return -EIO;
> +       }
> +
> +       /* Check Dormant State in Present */
> +       if (!host->ops->uhs2_host_operation(host, UHS2_CHECK_DORMANT)) {
> +               pr_err("%s: %s: UHS2 GO_DORMANT_STATE fail!\n",
> +                      mmc_hostname(host), __func__);
> +               return -EIO;
> +       }
> +
> +       host->ops->uhs2_host_operation(host, UHS2_DISABLE_CLK);
> +
>         return 0;
>  }
>
> +static int sd_uhs2_change_speed(struct mmc_host *host, u32 node_id)
> +{
> +       struct mmc_command cmd = {0};
> +       struct uhs2_command uhs2_cmd = {};
> +       u16 header = 0, arg = 0;
> +       int err;
> +       int timeout = 100;
> +
> +       /* Change Speed Range at controller side. */
> +       if (!host->ops->uhs2_host_operation(host, UHS2_SET_SPEED_B)) {
> +               pr_err("%s: %s: UHS2 SET_SPEED fail!\n",
> +                      mmc_hostname(host), __func__);
> +               return -EIO;
> +       }
> +
> +       err = sd_uhs2_go_dormant(host, false, node_id);
> +       if (err) {
> +               pr_err("%s: %s: UHS2 GO_DORMANT_STATE fail, err= 0x%x!\n",
> +                      mmc_hostname(host), __func__, err);
> +               return -EIO;
> +       }
> +
> +       /* restore sd clock */
> +       mmc_delay(5);
> +       host->ops->uhs2_host_operation(host, UHS2_ENABLE_CLK);

I think the code can be a bit better structured here. More precisely,
since sd_uhs2_go_dormant() is the one that calls
->uhs2_host_operation(host, UHS2_DISABLE_INT) and
->uhs2_host_operation(host, UHS2_DISABLE_CLK), it's then up to
sd_uhs2_change_speed() to restore these changes.

To me, it would be more clear if both enabling and disabling of the
clock /interrupt are managed in sd_uhs2_change_speed().

> +
> +       /* Enable Normal INT */
> +       if (!host->ops->uhs2_host_operation(host, UHS2_ENABLE_INT)) {
> +               pr_err("%s: %s: UHS2 ENABLE_INT fail!\n",
> +                      mmc_hostname(host), __func__);
> +               return -EIO;
> +       }
> +
> +       /*
> +        * According to UHS-II Addendum Version 1.01, chapter 6.2.3, wait card
> +        * switch to Active State
> +        */
> +       header = UHS2_NATIVE_PACKET | UHS2_PACKET_TYPE_CCMD | node_id;
> +       arg = ((UHS2_DEV_CONFIG_GEN_SET & 0xFF) << 8) |
> +               UHS2_NATIVE_CMD_READ |
> +               UHS2_NATIVE_CMD_PLEN_8B |
> +               (UHS2_DEV_CONFIG_GEN_SET >> 8);
> +       do {
> +               sd_uhs2_cmd_assemble(&cmd, &uhs2_cmd, header, arg, NULL, 0, NULL, 0);
> +               err = mmc_wait_for_cmd(host, &cmd, 0);
> +               if (err) {
> +                       pr_err("%s: %s: UHS2 CMD send fail, err= 0x%x!\n",
> +                              mmc_hostname(host), __func__, err);
> +                       return -EIO;
> +               }
> +
> +               if (cmd.resp[1] & UHS2_DEV_CONFIG_GEN_SET_CFG_COMPLETE)
> +                       break;
> +
> +               timeout--;
> +               if (timeout == 0) {
> +                       pr_err("%s: %s: Not switch to Active in 100 ms\n",
> +                              mmc_hostname(host), __func__);
> +                       return -EIO;
> +               }
> +
> +               mmc_delay(1);
> +       } while (1);

We really want to avoid these kinds of polling loops, for several
reasons. Please convert into using __mmc_poll_for_busy() instead.

> +
> +       return 0;
> +}
> +
> +static int sd_uhs2_get_ro(struct mmc_host *host)
> +{
> +       int ro;
> +
> +       /*
> +        * Some systems don't feature a write-protect pin and don't need one.
> +        * E.g. because they only have micro-SD card slot. For those systems
> +        * assume that the SD card is always read-write.
> +        */
> +       if (host->caps2 & MMC_CAP2_NO_WRITE_PROTECT)
> +               return 0;
> +
> +       if (!host->ops->get_ro)
> +               return -1;
> +
> +       ro = host->ops->get_ro(host);
> +
> +       return ro;

This can be replaced with mmc_sd_get_ro(). Let's avoid the open coding
and make that function being shared instead.

> +}
> +
>  /*
>   * Initialize the UHS-II card through the SD-TRAN transport layer. This enables
>   * commands/requests to be backwards compatible through the legacy SD protocol.
> @@ -107,9 +696,127 @@ static int sd_uhs2_config_write(struct mmc_host *host, struct mmc_card *card)
>   */
>  static int sd_uhs2_legacy_init(struct mmc_host *host, struct mmc_card *card)
>  {
> +       int err;
> +       u32 cid[4];
> +       u32 ocr;
> +       u32 rocr = 0;
> +       int ro;
> +
> +       WARN_ON(!host->claimed);

Drop this, it's an internal function, we should know that the host is
claimed before calling sd_uhs2_legacy_init().

> +
> +       /* Send CMD0 to reset SD card */
> +       mmc_go_idle(host);
> +
> +       /* Send CMD8 to communicate SD interface operation condition */
> +       err = mmc_send_if_cond(host, host->ocr_avail);
> +       if (err) {
> +               pr_err("%s: %s: SEND_IF_COND fail!\n",
> +                      mmc_hostname(host), __func__);

Please drop these prints for every command/operation that fails. We
already have trace/debug options for commands/requests.

This applies to all the below code as well (perhaps there are few
cases not covered by the existing trace/debug support, those may be
converted to pr_debug().

> +               return err;
> +       }
> +
> +       /*
> +        * Probe SD card working voltage.
> +        */
> +       err = mmc_send_app_op_cond(host, 0, &ocr);
> +       if (err) {
> +               pr_err("%s: %s: SD_SEND_OP_COND fail!\n",
> +                      mmc_hostname(host), __func__);
> +               return 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);

If the host has MMC_CAP2_FULL_PWR_CYCLE set, mmc_select_voltage() may
end up calling mmc_power_cycle(). This is not going to work for
UHS-II.

Either we need to modify mmc_select_voltage() so it becomes aware that
it can be called for UHS-II initialization, allowing it to avoid the
path to mmc_power_cycle() - or simply open code the part from
mmc_select_voltage() for UHS-II here. I think I prefer the latter.

> +
> +       /*
> +        * Some cards have zero value of rocr in UHS-II mode. Assign host's
> +        * ocr value to rocr.
> +        */
> +       if (!rocr) {
> +               if (host->ocr_avail) {
> +                       rocr = host->ocr_avail;

host->ocr_avail should really be checked in when the host driver calls
mmc_add_host(). It must not be zero, then we should let mmc_add_host()
return an error code. I look into this and send a patch for this
separately.

In other words, you should not need to check it here, but just trust that's set.

> +               } else {
> +                       pr_err("%s: %s: there is no valid OCR.\n",
> +                              mmc_hostname(host), __func__);
> +                       return -EINVAL;
> +               }
> +       }
> +
> +       /* Wait SD power on ready */
> +       ocr = rocr;
> +       err = mmc_send_app_op_cond(host, ocr, &rocr);
> +       if (err) {
> +               pr_err("%s: %s: SD_SEND_OP_COND fail!\n", mmc_hostname(host),
> +                      __func__);
> +               return err;
> +       }
> +
> +       err = mmc_send_cid(host, cid);
> +       if (err) {
> +               pr_err("%s: %s: SD_SEND_CID fail!\n", mmc_hostname(host),
> +                      __func__);
> +               return err;
> +       }
> +       memcpy(card->raw_cid, cid, sizeof(card->raw_cid));
> +
> +       /*
> +        * Call the optional HC's init_card function to handle quirks.
> +        */
> +       if (host->ops->init_card)
> +               host->ops->init_card(host, card);

This can be removed, as it's only for the legacy interface, I think.

> +
> +       /*
> +        * For native busses:  get card RCA and quit open drain mode.
> +        */
> +       err = mmc_send_relative_addr(host, &card->rca);
> +       if (err) {
> +               pr_err("%s: %s: SD_SEND_RCA fail!\n", mmc_hostname(host),
> +                      __func__);
> +               return err;
> +       }
> +
> +       err = mmc_sd_get_csd(card);
> +       if (err) {
> +               pr_err("%s: %s: SD_SEND_CSD fail!\n", mmc_hostname(host),
> +                      __func__);
> +               return err;
> +       }
> +
> +       /*
> +        * Select card, as all following commands rely on that.
> +        */
> +       err = mmc_select_card(card);
> +       if (err) {
> +               pr_err("%s: %s: SD_SEL_DSEL fail!\n", mmc_hostname(host),
> +                      __func__);
> +               return err;
> +       }
> +
> +       /*
> +        * Check if read-only switch is active.
> +        */
> +       ro = sd_uhs2_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);
> +       }
> +
>         return 0;
>  }
>
> +static void sd_uhs2_remove(struct mmc_host *host)
> +{
> +       mmc_remove_card(host->card);
> +       host->card = NULL;
> +}
> +
>  /*
>   * Allocate the data structure for the mmc_card and run the UHS-II specific
>   * initialization sequence.
> @@ -121,16 +828,21 @@ static int sd_uhs2_init_card(struct mmc_host *host)
>         int err;
>
>         err = sd_uhs2_dev_init(host);
> -       if (err)
> +       if (err) {
> +               pr_err("%s: UHS2 DEVICE_INIT fail!\n", mmc_hostname(host));
>                 return err;
> +       }
>
>         err = sd_uhs2_enum(host, &node_id);
> -       if (err)
> +       if (err) {
> +               pr_err("%s: UHS2 ENUMERATE fail!\n", mmc_hostname(host));
>                 return err;
> +       }
>
>         card = mmc_alloc_card(host, &sd_type);
>         if (IS_ERR(card))
>                 return PTR_ERR(card);
> +       host->card = card;
>
>         card->uhs2_config.node_id = node_id;
>         card->type = MMC_TYPE_SD;
> @@ -139,6 +851,16 @@ static int sd_uhs2_init_card(struct mmc_host *host)
>         if (err)
>                 goto err;
>
> +       /* Change to Speed Range B if it is supported */
> +       if (host->uhs2_caps.flags & MMC_UHS2_SPEED_B) {
> +               err = sd_uhs2_change_speed(host, node_id);
> +               if (err) {
> +                       pr_err("%s: %s: UHS2 sd_uhs2_change_speed() fail!\n",
> +                              mmc_hostname(host), __func__);
> +                       return err;
> +               }
> +       }
> +
>         err = sd_uhs2_config_write(host, card);
>         if (err)
>                 goto err;
> @@ -147,20 +869,13 @@ static int sd_uhs2_init_card(struct mmc_host *host)
>         if (err)
>                 goto err;
>
> -       host->card = card;
>         return 0;
>
>  err:
> -       mmc_remove_card(card);
> +       sd_uhs2_remove(host);
>         return err;
>  }

[...]

Kind regards
Uffe

On Wed, Mar 23, 2022 at 05:15:59PM +0100, Ulf Hansson wrote:
> On Tue, 22 Feb 2022 at 04:40, Jason Lai <jasonlai.genesyslogic@gmail.com> wrote:
> >
> > From: Jason Lai <jason.lai@genesyslogic.com.tw>
> >
> > UHS-II card initialization flow is divided into 2 categories: PHY & Card.
> > Part 1 - PHY Initialization:
> >          Every host controller may need their own avtivation operation to
> >          establish LINK between controller and card. So we add a new member
> >          function(uhs2_detect_init) in struct mmc_host_ops for host
> >          controller use.
> > Part 2 - Card Initialization:
> >          This part can be divided into 6 substeps.
> >          1. Send UHS-II CCMD DEVICE_INIT to card.
> >          2. Send UHS-II CCMD ENUMERATE to card.
> >          3. Send UHS-II Native Read CCMD to obtain capabilities in CFG_REG
> >             of card.
> >          4. Host compares capabilities of host controller and card, then
> >             write the negotiated values to Setting field in CFG_REG of card
> >             through UHS-II Native Write CCMD.
> >          5. Switch host controller's clock to Range B if it is supported by
> >             both host controller and card.
> >          6. Execute legacy SD initialization flow.
> > Part 3 - Provide a function to tranaform legacy SD command packet into
> >          UHS-II SD-TRAN DCMD packet.
> >
> > Most of the code added above came from Intel's original patch[3].
> >
> > [3]
> > https://patchwork.kernel.org/project/linux-mmc/patch/1419672479-30852-2-
> > git-send-email-yi.y.sun@intel.com/

To honor the original work, we should add Intel's copyright notice here
as I did before.

-Takahiro Akashi


> > Signed-off-by: Jason Lai <jason.lai@genesyslogic.com.tw>
> > ---
> >  drivers/mmc/core/sd_uhs2.c | 835 ++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 817 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/mmc/core/sd_uhs2.c b/drivers/mmc/core/sd_uhs2.c
> > index 800957f74632..f1e8e30301eb 100644
> > --- a/drivers/mmc/core/sd_uhs2.c
> > +++ b/drivers/mmc/core/sd_uhs2.c
> > @@ -3,6 +3,7 @@
> >   * Copyright (C) 2021 Linaro Ltd
> >   *
> >   * Author: Ulf Hansson <ulf.hansson@linaro.org>
> > + * Author: Jason Lai <jason.lai@genesyslogic.com.tw>
> >   *
> >   * Support for SD UHS-II cards
> >   */
> > @@ -10,19 +11,31 @@
> >
> >  #include <linux/mmc/host.h>
> >  #include <linux/mmc/card.h>
> > +#include <linux/mmc/mmc.h>
> > +#include <linux/mmc/sd_uhs2.h>
> >
> >  #include "core.h"
> >  #include "bus.h"
> > +#include "card.h"
> >  #include "sd.h"
> > +#include "sd_ops.h"
> >  #include "mmc_ops.h"
> > +#include "sd_uhs2.h"
> >
> >  static const unsigned int sd_uhs2_freqs[] = { 52000000, 26000000 };
> >
> >  static int sd_uhs2_set_ios(struct mmc_host *host)
> >  {
> >         struct mmc_ios *ios = &host->ios;
> > +       int err = 0;
> >
> > -       return host->ops->uhs2_set_ios(host, ios);
> > +       pr_debug("%s: clock %uHz powermode %u Vdd %u timing %u\n",
> > +                mmc_hostname(host), ios->clock, ios->power_mode, ios->vdd,
> > +                ios->timing);
> > +
> > +       host->ops->set_ios(host, ios);
> 
> We discussed using the ->set_ios() callback in a previous version. To
> repeat myself, I don't think it's a good idea. UHS-II needs an
> entirely different power sequence than the legacy interface(s), hence
> I think it's simply cleaner to separate them.
> 
> To move forward, I see two options.
> 1) Use only the ->uhs2_host_operation() ops.
> 2) Use a combination of the ->uhs2_set_ios() ops and the
> ->uhs2_host_operation() ops.
> 
> Both options work for me. However, perhaps if you could incorporate
> the changes done on the host driver at next submission, it becomes
> easier for me to understand what makes best sense.
> 
> > +
> > +       return err;
> >  }
> >
> >  static int sd_uhs2_power_up(struct mmc_host *host)
> > @@ -45,6 +58,43 @@ static void sd_uhs2_power_off(struct mmc_host *host)
> >         sd_uhs2_set_ios(host);
> >  }
> 
> [...]
> 
> >
> >  /*
> > @@ -61,6 +119,77 @@ static int sd_uhs2_phy_init(struct mmc_host *host)
> >   */
> >  static int sd_uhs2_dev_init(struct mmc_host *host)
> >  {
> > +       struct mmc_command cmd = {0};
> > +       struct uhs2_command uhs2_cmd = {};
> > +       u32 cnt;
> > +       u32 dap, gap, resp_gap;
> > +       u16 header = 0, arg = 0;
> 
> No need to initiate these.
> 
> > +       u32 payload[1];
> 
> u32?
> 
> > +       u8 plen = 1;
> > +       u8 gd = 0, cf = 1;
> > +       u8 resp[6] = {0};
> > +       u8 resp_len = 6;
> 
> Many of these variables are just constant numbers. If it makes sense
> to add definitions for them, then please do that instead. If not, just
> give the value directly in the code.
> 
> For example: plen = 1; (I assume that is payload length). This can
> just be given as an in-parameter to sd_uhs2_cmd_assemble(), without
> further explanation.
> 
> The point is, sd_uhs2_cmd_assemble() should have a well described
> description of its in-parameters, so no need for further descriptions,
> I think.
> 
> This comment applies to all the new code/functions that are added in
> the $subject patch. Please go through all of the code and fix this.
> 
> 
> > +       int err;
> > +
> > +       dap = host->uhs2_caps.dap;
> > +       gap = host->uhs2_caps.gap;
> > +
> > +       header = UHS2_NATIVE_PACKET | UHS2_PACKET_TYPE_CCMD;
> > +       arg = ((UHS2_DEV_CMD_DEVICE_INIT & 0xFF) << 8) |
> > +              UHS2_NATIVE_CMD_WRITE |
> > +              UHS2_NATIVE_CMD_PLEN_4B |
> > +              (UHS2_DEV_CMD_DEVICE_INIT >> 8);
> > +
> > +       /*
> > +        * Refer to UHS-II Addendum Version 1.02 section 6.3.1.
> > +        * Max. time from DEVICE_INIT CCMD EOP reception on Device
> > +        * Rx to its SOP transmission on Device Tx(Tfwd_init_cmd) is
> > +        * 1 second.
> > +        */
> > +       cmd.busy_timeout = 1000;
> > +
> > +       /*
> > +        * Refer to UHS-II Addendum Version 1.02 section 6.2.6.3.
> > +        * When the number of the DEVICE_INIT commands is reach to
> > +        * 30 tiems, Host shall stop issuing DEVICE_INIT command
> > +        * and regard it as an error.
> > +        */
> > +       for (cnt = 0; cnt < 30; cnt++) {
> > +               payload[0] = ((dap & 0xF) << 12) |
> > +                             (cf << 11)         |
> > +                             ((gd & 0xF) << 4)  |
> > +                             (gap & 0xF);
> 
> To me, it looks like the payload data deserves to be explained a bit.
> Perhaps you can add a comment explaining what pieces it consists of so
> this becomes more clear?
> 
> > +
> > +               sd_uhs2_cmd_assemble(&cmd, &uhs2_cmd, header, arg,  payload, plen, resp, resp_len);
> > +
> > +               err = mmc_wait_for_cmd(host, &cmd, 0);
> > +
> > +               if (err) {
> > +                       pr_err("%s: %s: UHS2 CMD send fail, err= 0x%x!\n",
> > +                              mmc_hostname(host), __func__, err);
> > +                       return -EIO;
> 
> Why do you override the original error code that was returned from
> mmc_wait_for_cmd()?
> 
> Normally it's preferred to keep the error code, unless there is good
> reason not to.
> 
> Again, I won't add more comments like this in the code from the
> $subject patch. But please go through it all to avoid this kind of
> thing.
> 
> > +               }
> > +
> > +               if (resp[3] != (UHS2_DEV_CMD_DEVICE_INIT & 0xFF)) {
> > +                       pr_err("%s: DEVICE_INIT response is wrong!\n",
> > +                              mmc_hostname(host));
> > +                       return -EIO;
> > +               }
> > +
> > +               if (resp[5] & 0x8) {
> > +                       host->uhs2_caps.group_desc = gd;
> > +                       break;
> 
> I suggest you do a return 0 here. In this way you can skip the check
> "if (cnt == 30)" below and just return an error code instead.
> 
> > +               }
> > +               resp_gap = resp[4] & 0x0F;
> > +               if (gap == resp_gap)
> > +                       gd++;
> > +       }
> > +       if (cnt == 30) {
> > +               pr_err("%s: DEVICE_INIT fail, already 30 times!\n",
> > +                      mmc_hostname(host));
> > +               return -EIO;
> > +       }
> > +
> >         return 0;
> >  }
> >
> 
> >  static int sd_uhs2_config_read(struct mmc_host *host, struct mmc_card *card)
> >  {
> > +       struct mmc_command cmd = {0};
> > +       struct uhs2_command uhs2_cmd = {};
> > +       u16 header = 0, arg = 0;
> > +       u32 cap;
> > +       int err;
> > +
> > +       header = UHS2_NATIVE_PACKET |
> > +                UHS2_PACKET_TYPE_CCMD |
> > +                card->uhs2_config.node_id;
> > +       arg = ((UHS2_DEV_CONFIG_GEN_CAPS & 0xFF) << 8) |
> > +              UHS2_NATIVE_CMD_READ |
> > +              UHS2_NATIVE_CMD_PLEN_4B |
> > +              (UHS2_DEV_CONFIG_GEN_CAPS >> 8);
> > +
> > +       /* There is no payload because per spec, there should be
> > +        * no payload field for read CCMD.
> > +        * Plen is set in arg. Per spec, plen for read CCMD
> > +        * represents the len of read data which is assigned in payload
> > +        * of following RES (p136).
> > +        */
> > +       sd_uhs2_cmd_assemble(&cmd, &uhs2_cmd, header, arg, NULL, 0, NULL, 0);
> 
> We are reading the configuration data here and onwards, piece by
> piece. Perhaps if you can add a small comment about each piece we are
> reading, before each call to mmc_wait_for_cmd(), that can help to
> easier understand what goes on.
> 
> [...]
> 
> >  static int sd_uhs2_config_write(struct mmc_host *host, struct mmc_card *card)
> >  {
> > +       struct mmc_command cmd = {0};
> > +       struct uhs2_command uhs2_cmd = {};
> > +       u16 header = 0, arg = 0;
> > +       u32 payload[2];
> > +       u8 nMinDataGap;
> > +       u8 plen;
> > +       int err;
> > +       u8 resp[5] = {0};
> > +       u8 resp_len = 5;
> > +
> > +       header = UHS2_NATIVE_PACKET |
> > +                UHS2_PACKET_TYPE_CCMD | card->uhs2_config.node_id;
> > +       arg = ((UHS2_DEV_CONFIG_GEN_SET & 0xFF) << 8) |
> > +              UHS2_NATIVE_CMD_WRITE |
> > +              UHS2_NATIVE_CMD_PLEN_8B |
> > +              (UHS2_DEV_CONFIG_GEN_SET >> 8);
> > +
> > +       if (card->uhs2_config.n_lanes == UHS2_DEV_CONFIG_2L_HD_FD &&
> > +           host->uhs2_caps.n_lanes == UHS2_DEV_CONFIG_2L_HD_FD) {
> > +               /* Support HD */
> > +               host->uhs2_caps.flags |= MMC_UHS2_2L_HD;
> 
> How is the uhs2_caps.flags field intended to be used? To me it looks
> like a way for the mmc core to exchange status/configuration
> information about the initialization process of the card, with the mmc
> host driver. Perhaps there is more too. Is that correct?
> 
> If so, I think it looks quite similar for what we have in the struct
> mmc_ios, for the legacy interface(s). I am not saying we should use
> that, just trying to understand what would suit best here.
> 
> > +               nMinDataGap = 1;
> > +       } else {
> > +               /* Only support 2L-FD so far */
> > +               host->uhs2_caps.flags &= ~MMC_UHS2_2L_HD;
> > +               nMinDataGap = 3;
> > +       }
> > +
> > +       /*
> > +        * Most UHS-II cards only support FD and 2L-HD mode. Other lane numbers
> > +        * defined in UHS-II addendem Ver1.01 are optional.
> > +        */
> > +       host->uhs2_caps.n_lanes_set = UHS2_DEV_CONFIG_GEN_SET_2L_FD_HD;
> > +       card->uhs2_config.n_lanes_set = UHS2_DEV_CONFIG_GEN_SET_2L_FD_HD;
> 
> [...]
> 
> > +static int sd_uhs2_go_dormant(struct mmc_host *host, bool hibernate, u32 node_id)
> > +{
> 
> Looks like the in-parameter "hibernate" is superfluous, as it's always
> set to "false" by the caller.
> 
> > +       struct mmc_command cmd = {0};
> > +       struct uhs2_command uhs2_cmd = {};
> > +       u16 header = 0, arg = 0;
> > +       u32 payload[1];
> > +       u8 plen = 1;
> > +       int err;
> > +
> > +       /* Disable Normal INT */
> > +       if (!host->ops->uhs2_host_operation(host, UHS2_DISABLE_INT)) {
> > +               pr_err("%s: %s: UHS2 DISABLE_INT fail!\n",
> > +                      mmc_hostname(host), __func__);
> > +               return -EIO;
> > +       }
> > +
> > +       header = UHS2_NATIVE_PACKET | UHS2_PACKET_TYPE_CCMD | node_id;
> > +
> > +       arg = ((UHS2_DEV_CMD_GO_DORMANT_STATE & 0xFF) << 8) |
> > +               UHS2_NATIVE_CMD_WRITE |
> > +               UHS2_NATIVE_CMD_PLEN_4B |
> > +               (UHS2_DEV_CMD_GO_DORMANT_STATE >> 8);
> > +
> > +       if (hibernate)
> > +               payload[0] = UHS2_DEV_CMD_DORMANT_HIBER;
> > +
> > +       sd_uhs2_cmd_assemble(&cmd, &uhs2_cmd, header, arg, payload, plen, NULL, 0);
> > +
> > +       err = mmc_wait_for_cmd(host, &cmd, 0);
> > +       if (err) {
> > +               pr_err("%s: %s: UHS2 CMD send fail, err= 0x%x!\n",
> > +                      mmc_hostname(host), __func__, err);
> > +               return -EIO;
> > +       }
> > +
> > +       /* Check Dormant State in Present */
> > +       if (!host->ops->uhs2_host_operation(host, UHS2_CHECK_DORMANT)) {
> > +               pr_err("%s: %s: UHS2 GO_DORMANT_STATE fail!\n",
> > +                      mmc_hostname(host), __func__);
> > +               return -EIO;
> > +       }
> > +
> > +       host->ops->uhs2_host_operation(host, UHS2_DISABLE_CLK);
> > +
> >         return 0;
> >  }
> >
> > +static int sd_uhs2_change_speed(struct mmc_host *host, u32 node_id)
> > +{
> > +       struct mmc_command cmd = {0};
> > +       struct uhs2_command uhs2_cmd = {};
> > +       u16 header = 0, arg = 0;
> > +       int err;
> > +       int timeout = 100;
> > +
> > +       /* Change Speed Range at controller side. */
> > +       if (!host->ops->uhs2_host_operation(host, UHS2_SET_SPEED_B)) {
> > +               pr_err("%s: %s: UHS2 SET_SPEED fail!\n",
> > +                      mmc_hostname(host), __func__);
> > +               return -EIO;
> > +       }
> > +
> > +       err = sd_uhs2_go_dormant(host, false, node_id);
> > +       if (err) {
> > +               pr_err("%s: %s: UHS2 GO_DORMANT_STATE fail, err= 0x%x!\n",
> > +                      mmc_hostname(host), __func__, err);
> > +               return -EIO;
> > +       }
> > +
> > +       /* restore sd clock */
> > +       mmc_delay(5);
> > +       host->ops->uhs2_host_operation(host, UHS2_ENABLE_CLK);
> 
> I think the code can be a bit better structured here. More precisely,
> since sd_uhs2_go_dormant() is the one that calls
> ->uhs2_host_operation(host, UHS2_DISABLE_INT) and
> ->uhs2_host_operation(host, UHS2_DISABLE_CLK), it's then up to
> sd_uhs2_change_speed() to restore these changes.
> 
> To me, it would be more clear if both enabling and disabling of the
> clock /interrupt are managed in sd_uhs2_change_speed().
> 
> > +
> > +       /* Enable Normal INT */
> > +       if (!host->ops->uhs2_host_operation(host, UHS2_ENABLE_INT)) {
> > +               pr_err("%s: %s: UHS2 ENABLE_INT fail!\n",
> > +                      mmc_hostname(host), __func__);
> > +               return -EIO;
> > +       }
> > +
> > +       /*
> > +        * According to UHS-II Addendum Version 1.01, chapter 6.2.3, wait card
> > +        * switch to Active State
> > +        */
> > +       header = UHS2_NATIVE_PACKET | UHS2_PACKET_TYPE_CCMD | node_id;
> > +       arg = ((UHS2_DEV_CONFIG_GEN_SET & 0xFF) << 8) |
> > +               UHS2_NATIVE_CMD_READ |
> > +               UHS2_NATIVE_CMD_PLEN_8B |
> > +               (UHS2_DEV_CONFIG_GEN_SET >> 8);
> > +       do {
> > +               sd_uhs2_cmd_assemble(&cmd, &uhs2_cmd, header, arg, NULL, 0, NULL, 0);
> > +               err = mmc_wait_for_cmd(host, &cmd, 0);
> > +               if (err) {
> > +                       pr_err("%s: %s: UHS2 CMD send fail, err= 0x%x!\n",
> > +                              mmc_hostname(host), __func__, err);
> > +                       return -EIO;
> > +               }
> > +
> > +               if (cmd.resp[1] & UHS2_DEV_CONFIG_GEN_SET_CFG_COMPLETE)
> > +                       break;
> > +
> > +               timeout--;
> > +               if (timeout == 0) {
> > +                       pr_err("%s: %s: Not switch to Active in 100 ms\n",
> > +                              mmc_hostname(host), __func__);
> > +                       return -EIO;
> > +               }
> > +
> > +               mmc_delay(1);
> > +       } while (1);
> 
> We really want to avoid these kinds of polling loops, for several
> reasons. Please convert into using __mmc_poll_for_busy() instead.
> 
> > +
> > +       return 0;
> > +}
> > +
> > +static int sd_uhs2_get_ro(struct mmc_host *host)
> > +{
> > +       int ro;
> > +
> > +       /*
> > +        * Some systems don't feature a write-protect pin and don't need one.
> > +        * E.g. because they only have micro-SD card slot. For those systems
> > +        * assume that the SD card is always read-write.
> > +        */
> > +       if (host->caps2 & MMC_CAP2_NO_WRITE_PROTECT)
> > +               return 0;
> > +
> > +       if (!host->ops->get_ro)
> > +               return -1;
> > +
> > +       ro = host->ops->get_ro(host);
> > +
> > +       return ro;
> 
> This can be replaced with mmc_sd_get_ro(). Let's avoid the open coding
> and make that function being shared instead.
> 
> > +}
> > +
> >  /*
> >   * Initialize the UHS-II card through the SD-TRAN transport layer. This enables
> >   * commands/requests to be backwards compatible through the legacy SD protocol.
> > @@ -107,9 +696,127 @@ static int sd_uhs2_config_write(struct mmc_host *host, struct mmc_card *card)
> >   */
> >  static int sd_uhs2_legacy_init(struct mmc_host *host, struct mmc_card *card)
> >  {
> > +       int err;
> > +       u32 cid[4];
> > +       u32 ocr;
> > +       u32 rocr = 0;
> > +       int ro;
> > +
> > +       WARN_ON(!host->claimed);
> 
> Drop this, it's an internal function, we should know that the host is
> claimed before calling sd_uhs2_legacy_init().
> 
> > +
> > +       /* Send CMD0 to reset SD card */
> > +       mmc_go_idle(host);
> > +
> > +       /* Send CMD8 to communicate SD interface operation condition */
> > +       err = mmc_send_if_cond(host, host->ocr_avail);
> > +       if (err) {
> > +               pr_err("%s: %s: SEND_IF_COND fail!\n",
> > +                      mmc_hostname(host), __func__);
> 
> Please drop these prints for every command/operation that fails. We
> already have trace/debug options for commands/requests.
> 
> This applies to all the below code as well (perhaps there are few
> cases not covered by the existing trace/debug support, those may be
> converted to pr_debug().
> 
> > +               return err;
> > +       }
> > +
> > +       /*
> > +        * Probe SD card working voltage.
> > +        */
> > +       err = mmc_send_app_op_cond(host, 0, &ocr);
> > +       if (err) {
> > +               pr_err("%s: %s: SD_SEND_OP_COND fail!\n",
> > +                      mmc_hostname(host), __func__);
> > +               return 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);
> 
> If the host has MMC_CAP2_FULL_PWR_CYCLE set, mmc_select_voltage() may
> end up calling mmc_power_cycle(). This is not going to work for
> UHS-II.
> 
> Either we need to modify mmc_select_voltage() so it becomes aware that
> it can be called for UHS-II initialization, allowing it to avoid the
> path to mmc_power_cycle() - or simply open code the part from
> mmc_select_voltage() for UHS-II here. I think I prefer the latter.
> 
> > +
> > +       /*
> > +        * Some cards have zero value of rocr in UHS-II mode. Assign host's
> > +        * ocr value to rocr.
> > +        */
> > +       if (!rocr) {
> > +               if (host->ocr_avail) {
> > +                       rocr = host->ocr_avail;
> 
> host->ocr_avail should really be checked in when the host driver calls
> mmc_add_host(). It must not be zero, then we should let mmc_add_host()
> return an error code. I look into this and send a patch for this
> separately.
> 
> In other words, you should not need to check it here, but just trust that's set.
> 
> > +               } else {
> > +                       pr_err("%s: %s: there is no valid OCR.\n",
> > +                              mmc_hostname(host), __func__);
> > +                       return -EINVAL;
> > +               }
> > +       }
> > +
> > +       /* Wait SD power on ready */
> > +       ocr = rocr;
> > +       err = mmc_send_app_op_cond(host, ocr, &rocr);
> > +       if (err) {
> > +               pr_err("%s: %s: SD_SEND_OP_COND fail!\n", mmc_hostname(host),
> > +                      __func__);
> > +               return err;
> > +       }
> > +
> > +       err = mmc_send_cid(host, cid);
> > +       if (err) {
> > +               pr_err("%s: %s: SD_SEND_CID fail!\n", mmc_hostname(host),
> > +                      __func__);
> > +               return err;
> > +       }
> > +       memcpy(card->raw_cid, cid, sizeof(card->raw_cid));
> > +
> > +       /*
> > +        * Call the optional HC's init_card function to handle quirks.
> > +        */
> > +       if (host->ops->init_card)
> > +               host->ops->init_card(host, card);
> 
> This can be removed, as it's only for the legacy interface, I think.
> 
> > +
> > +       /*
> > +        * For native busses:  get card RCA and quit open drain mode.
> > +        */
> > +       err = mmc_send_relative_addr(host, &card->rca);
> > +       if (err) {
> > +               pr_err("%s: %s: SD_SEND_RCA fail!\n", mmc_hostname(host),
> > +                      __func__);
> > +               return err;
> > +       }
> > +
> > +       err = mmc_sd_get_csd(card);
> > +       if (err) {
> > +               pr_err("%s: %s: SD_SEND_CSD fail!\n", mmc_hostname(host),
> > +                      __func__);
> > +               return err;
> > +       }
> > +
> > +       /*
> > +        * Select card, as all following commands rely on that.
> > +        */
> > +       err = mmc_select_card(card);
> > +       if (err) {
> > +               pr_err("%s: %s: SD_SEL_DSEL fail!\n", mmc_hostname(host),
> > +                      __func__);
> > +               return err;
> > +       }
> > +
> > +       /*
> > +        * Check if read-only switch is active.
> > +        */
> > +       ro = sd_uhs2_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);
> > +       }
> > +
> >         return 0;
> >  }
> >
> > +static void sd_uhs2_remove(struct mmc_host *host)
> > +{
> > +       mmc_remove_card(host->card);
> > +       host->card = NULL;
> > +}
> > +
> >  /*
> >   * Allocate the data structure for the mmc_card and run the UHS-II specific
> >   * initialization sequence.
> > @@ -121,16 +828,21 @@ static int sd_uhs2_init_card(struct mmc_host *host)
> >         int err;
> >
> >         err = sd_uhs2_dev_init(host);
> > -       if (err)
> > +       if (err) {
> > +               pr_err("%s: UHS2 DEVICE_INIT fail!\n", mmc_hostname(host));
> >                 return err;
> > +       }
> >
> >         err = sd_uhs2_enum(host, &node_id);
> > -       if (err)
> > +       if (err) {
> > +               pr_err("%s: UHS2 ENUMERATE fail!\n", mmc_hostname(host));
> >                 return err;
> > +       }
> >
> >         card = mmc_alloc_card(host, &sd_type);
> >         if (IS_ERR(card))
> >                 return PTR_ERR(card);
> > +       host->card = card;
> >
> >         card->uhs2_config.node_id = node_id;
> >         card->type = MMC_TYPE_SD;
> > @@ -139,6 +851,16 @@ static int sd_uhs2_init_card(struct mmc_host *host)
> >         if (err)
> >                 goto err;
> >
> > +       /* Change to Speed Range B if it is supported */
> > +       if (host->uhs2_caps.flags & MMC_UHS2_SPEED_B) {
> > +               err = sd_uhs2_change_speed(host, node_id);
> > +               if (err) {
> > +                       pr_err("%s: %s: UHS2 sd_uhs2_change_speed() fail!\n",
> > +                              mmc_hostname(host), __func__);
> > +                       return err;
> > +               }
> > +       }
> > +
> >         err = sd_uhs2_config_write(host, card);
> >         if (err)
> >                 goto err;
> > @@ -147,20 +869,13 @@ static int sd_uhs2_init_card(struct mmc_host *host)
> >         if (err)
> >                 goto err;
> >
> > -       host->card = card;
> >         return 0;
> >
> >  err:
> > -       mmc_remove_card(card);
> > +       sd_uhs2_remove(host);
> >         return err;
> >  }
> 
> [...]
> 
> Kind regards
> Uffe

On Thu, 24 Mar 2022 at 02:29, AKASHI Takahiro
<takahiro.akashi@linaro.org> wrote:
>
> On Wed, Mar 23, 2022 at 05:15:59PM +0100, Ulf Hansson wrote:
> > On Tue, 22 Feb 2022 at 04:40, Jason Lai <jasonlai.genesyslogic@gmail.com> wrote:
> > >
> > > From: Jason Lai <jason.lai@genesyslogic.com.tw>
> > >
> > > UHS-II card initialization flow is divided into 2 categories: PHY & Card.
> > > Part 1 - PHY Initialization:
> > >          Every host controller may need their own avtivation operation to
> > >          establish LINK between controller and card. So we add a new member
> > >          function(uhs2_detect_init) in struct mmc_host_ops for host
> > >          controller use.
> > > Part 2 - Card Initialization:
> > >          This part can be divided into 6 substeps.
> > >          1. Send UHS-II CCMD DEVICE_INIT to card.
> > >          2. Send UHS-II CCMD ENUMERATE to card.
> > >          3. Send UHS-II Native Read CCMD to obtain capabilities in CFG_REG
> > >             of card.
> > >          4. Host compares capabilities of host controller and card, then
> > >             write the negotiated values to Setting field in CFG_REG of card
> > >             through UHS-II Native Write CCMD.
> > >          5. Switch host controller's clock to Range B if it is supported by
> > >             both host controller and card.
> > >          6. Execute legacy SD initialization flow.
> > > Part 3 - Provide a function to tranaform legacy SD command packet into
> > >          UHS-II SD-TRAN DCMD packet.
> > >
> > > Most of the code added above came from Intel's original patch[3].
> > >
> > > [3]
> > > https://patchwork.kernel.org/project/linux-mmc/patch/1419672479-30852-2-
> > > git-send-email-yi.y.sun@intel.com/
>
> To honor the original work, we should add Intel's copyright notice here
> as I did before.

I think Jason already did, at least for some of the files in this series.

Note also that, the initial code that was posted, is very far from
what code that is going to be merged. Simply because the quality was
very poor and not acceptable for the upstream kernel. That said, I am
not sure we need to keep the copyrights for this, but I leave that
call to Jason to decide.

>
> -Takahiro Akashi
>

Kind regards
Uffe

>
> > > Signed-off-by: Jason Lai <jason.lai@genesyslogic.com.tw>
> > > ---
> > >  drivers/mmc/core/sd_uhs2.c | 835 ++++++++++++++++++++++++++++++++++++-
> > >  1 file changed, 817 insertions(+), 18 deletions(-)
> > >
> > > diff --git a/drivers/mmc/core/sd_uhs2.c b/drivers/mmc/core/sd_uhs2.c
> > > index 800957f74632..f1e8e30301eb 100644
> > > --- a/drivers/mmc/core/sd_uhs2.c
> > > +++ b/drivers/mmc/core/sd_uhs2.c
> > > @@ -3,6 +3,7 @@
> > >   * Copyright (C) 2021 Linaro Ltd
> > >   *
> > >   * Author: Ulf Hansson <ulf.hansson@linaro.org>
> > > + * Author: Jason Lai <jason.lai@genesyslogic.com.tw>
> > >   *
> > >   * Support for SD UHS-II cards
> > >   */
> > > @@ -10,19 +11,31 @@
> > >
> > >  #include <linux/mmc/host.h>
> > >  #include <linux/mmc/card.h>
> > > +#include <linux/mmc/mmc.h>
> > > +#include <linux/mmc/sd_uhs2.h>
> > >
> > >  #include "core.h"
> > >  #include "bus.h"
> > > +#include "card.h"
> > >  #include "sd.h"
> > > +#include "sd_ops.h"
> > >  #include "mmc_ops.h"
> > > +#include "sd_uhs2.h"
> > >
> > >  static const unsigned int sd_uhs2_freqs[] = { 52000000, 26000000 };
> > >
> > >  static int sd_uhs2_set_ios(struct mmc_host *host)
> > >  {
> > >         struct mmc_ios *ios = &host->ios;
> > > +       int err = 0;
> > >
> > > -       return host->ops->uhs2_set_ios(host, ios);
> > > +       pr_debug("%s: clock %uHz powermode %u Vdd %u timing %u\n",
> > > +                mmc_hostname(host), ios->clock, ios->power_mode, ios->vdd,
> > > +                ios->timing);
> > > +
> > > +       host->ops->set_ios(host, ios);
> >
> > We discussed using the ->set_ios() callback in a previous version. To
> > repeat myself, I don't think it's a good idea. UHS-II needs an
> > entirely different power sequence than the legacy interface(s), hence
> > I think it's simply cleaner to separate them.
> >
> > To move forward, I see two options.
> > 1) Use only the ->uhs2_host_operation() ops.
> > 2) Use a combination of the ->uhs2_set_ios() ops and the
> > ->uhs2_host_operation() ops.
> >
> > Both options work for me. However, perhaps if you could incorporate
> > the changes done on the host driver at next submission, it becomes
> > easier for me to understand what makes best sense.
> >
> > > +
> > > +       return err;
> > >  }
> > >
> > >  static int sd_uhs2_power_up(struct mmc_host *host)
> > > @@ -45,6 +58,43 @@ static void sd_uhs2_power_off(struct mmc_host *host)
> > >         sd_uhs2_set_ios(host);
> > >  }
> >
> > [...]
> >
> > >
> > >  /*
> > > @@ -61,6 +119,77 @@ static int sd_uhs2_phy_init(struct mmc_host *host)
> > >   */
> > >  static int sd_uhs2_dev_init(struct mmc_host *host)
> > >  {
> > > +       struct mmc_command cmd = {0};
> > > +       struct uhs2_command uhs2_cmd = {};
> > > +       u32 cnt;
> > > +       u32 dap, gap, resp_gap;
> > > +       u16 header = 0, arg = 0;
> >
> > No need to initiate these.
> >
> > > +       u32 payload[1];
> >
> > u32?
> >
> > > +       u8 plen = 1;
> > > +       u8 gd = 0, cf = 1;
> > > +       u8 resp[6] = {0};
> > > +       u8 resp_len = 6;
> >
> > Many of these variables are just constant numbers. If it makes sense
> > to add definitions for them, then please do that instead. If not, just
> > give the value directly in the code.
> >
> > For example: plen = 1; (I assume that is payload length). This can
> > just be given as an in-parameter to sd_uhs2_cmd_assemble(), without
> > further explanation.
> >
> > The point is, sd_uhs2_cmd_assemble() should have a well described
> > description of its in-parameters, so no need for further descriptions,
> > I think.
> >
> > This comment applies to all the new code/functions that are added in
> > the $subject patch. Please go through all of the code and fix this.
> >
> >
> > > +       int err;
> > > +
> > > +       dap = host->uhs2_caps.dap;
> > > +       gap = host->uhs2_caps.gap;
> > > +
> > > +       header = UHS2_NATIVE_PACKET | UHS2_PACKET_TYPE_CCMD;
> > > +       arg = ((UHS2_DEV_CMD_DEVICE_INIT & 0xFF) << 8) |
> > > +              UHS2_NATIVE_CMD_WRITE |
> > > +              UHS2_NATIVE_CMD_PLEN_4B |
> > > +              (UHS2_DEV_CMD_DEVICE_INIT >> 8);
> > > +
> > > +       /*
> > > +        * Refer to UHS-II Addendum Version 1.02 section 6.3.1.
> > > +        * Max. time from DEVICE_INIT CCMD EOP reception on Device
> > > +        * Rx to its SOP transmission on Device Tx(Tfwd_init_cmd) is
> > > +        * 1 second.
> > > +        */
> > > +       cmd.busy_timeout = 1000;
> > > +
> > > +       /*
> > > +        * Refer to UHS-II Addendum Version 1.02 section 6.2.6.3.
> > > +        * When the number of the DEVICE_INIT commands is reach to
> > > +        * 30 tiems, Host shall stop issuing DEVICE_INIT command
> > > +        * and regard it as an error.
> > > +        */
> > > +       for (cnt = 0; cnt < 30; cnt++) {
> > > +               payload[0] = ((dap & 0xF) << 12) |
> > > +                             (cf << 11)         |
> > > +                             ((gd & 0xF) << 4)  |
> > > +                             (gap & 0xF);
> >
> > To me, it looks like the payload data deserves to be explained a bit.
> > Perhaps you can add a comment explaining what pieces it consists of so
> > this becomes more clear?
> >
> > > +
> > > +               sd_uhs2_cmd_assemble(&cmd, &uhs2_cmd, header, arg,  payload, plen, resp, resp_len);
> > > +
> > > +               err = mmc_wait_for_cmd(host, &cmd, 0);
> > > +
> > > +               if (err) {
> > > +                       pr_err("%s: %s: UHS2 CMD send fail, err= 0x%x!\n",
> > > +                              mmc_hostname(host), __func__, err);
> > > +                       return -EIO;
> >
> > Why do you override the original error code that was returned from
> > mmc_wait_for_cmd()?
> >
> > Normally it's preferred to keep the error code, unless there is good
> > reason not to.
> >
> > Again, I won't add more comments like this in the code from the
> > $subject patch. But please go through it all to avoid this kind of
> > thing.
> >
> > > +               }
> > > +
> > > +               if (resp[3] != (UHS2_DEV_CMD_DEVICE_INIT & 0xFF)) {
> > > +                       pr_err("%s: DEVICE_INIT response is wrong!\n",
> > > +                              mmc_hostname(host));
> > > +                       return -EIO;
> > > +               }
> > > +
> > > +               if (resp[5] & 0x8) {
> > > +                       host->uhs2_caps.group_desc = gd;
> > > +                       break;
> >
> > I suggest you do a return 0 here. In this way you can skip the check
> > "if (cnt == 30)" below and just return an error code instead.
> >
> > > +               }
> > > +               resp_gap = resp[4] & 0x0F;
> > > +               if (gap == resp_gap)
> > > +                       gd++;
> > > +       }
> > > +       if (cnt == 30) {
> > > +               pr_err("%s: DEVICE_INIT fail, already 30 times!\n",
> > > +                      mmc_hostname(host));
> > > +               return -EIO;
> > > +       }
> > > +
> > >         return 0;
> > >  }
> > >
> >
> > >  static int sd_uhs2_config_read(struct mmc_host *host, struct mmc_card *card)
> > >  {
> > > +       struct mmc_command cmd = {0};
> > > +       struct uhs2_command uhs2_cmd = {};
> > > +       u16 header = 0, arg = 0;
> > > +       u32 cap;
> > > +       int err;
> > > +
> > > +       header = UHS2_NATIVE_PACKET |
> > > +                UHS2_PACKET_TYPE_CCMD |
> > > +                card->uhs2_config.node_id;
> > > +       arg = ((UHS2_DEV_CONFIG_GEN_CAPS & 0xFF) << 8) |
> > > +              UHS2_NATIVE_CMD_READ |
> > > +              UHS2_NATIVE_CMD_PLEN_4B |
> > > +              (UHS2_DEV_CONFIG_GEN_CAPS >> 8);
> > > +
> > > +       /* There is no payload because per spec, there should be
> > > +        * no payload field for read CCMD.
> > > +        * Plen is set in arg. Per spec, plen for read CCMD
> > > +        * represents the len of read data which is assigned in payload
> > > +        * of following RES (p136).
> > > +        */
> > > +       sd_uhs2_cmd_assemble(&cmd, &uhs2_cmd, header, arg, NULL, 0, NULL, 0);
> >
> > We are reading the configuration data here and onwards, piece by
> > piece. Perhaps if you can add a small comment about each piece we are
> > reading, before each call to mmc_wait_for_cmd(), that can help to
> > easier understand what goes on.
> >
> > [...]
> >
> > >  static int sd_uhs2_config_write(struct mmc_host *host, struct mmc_card *card)
> > >  {
> > > +       struct mmc_command cmd = {0};
> > > +       struct uhs2_command uhs2_cmd = {};
> > > +       u16 header = 0, arg = 0;
> > > +       u32 payload[2];
> > > +       u8 nMinDataGap;
> > > +       u8 plen;
> > > +       int err;
> > > +       u8 resp[5] = {0};
> > > +       u8 resp_len = 5;
> > > +
> > > +       header = UHS2_NATIVE_PACKET |
> > > +                UHS2_PACKET_TYPE_CCMD | card->uhs2_config.node_id;
> > > +       arg = ((UHS2_DEV_CONFIG_GEN_SET & 0xFF) << 8) |
> > > +              UHS2_NATIVE_CMD_WRITE |
> > > +              UHS2_NATIVE_CMD_PLEN_8B |
> > > +              (UHS2_DEV_CONFIG_GEN_SET >> 8);
> > > +
> > > +       if (card->uhs2_config.n_lanes == UHS2_DEV_CONFIG_2L_HD_FD &&
> > > +           host->uhs2_caps.n_lanes == UHS2_DEV_CONFIG_2L_HD_FD) {
> > > +               /* Support HD */
> > > +               host->uhs2_caps.flags |= MMC_UHS2_2L_HD;
> >
> > How is the uhs2_caps.flags field intended to be used? To me it looks
> > like a way for the mmc core to exchange status/configuration
> > information about the initialization process of the card, with the mmc
> > host driver. Perhaps there is more too. Is that correct?
> >
> > If so, I think it looks quite similar for what we have in the struct
> > mmc_ios, for the legacy interface(s). I am not saying we should use
> > that, just trying to understand what would suit best here.
> >
> > > +               nMinDataGap = 1;
> > > +       } else {
> > > +               /* Only support 2L-FD so far */
> > > +               host->uhs2_caps.flags &= ~MMC_UHS2_2L_HD;
> > > +               nMinDataGap = 3;
> > > +       }
> > > +
> > > +       /*
> > > +        * Most UHS-II cards only support FD and 2L-HD mode. Other lane numbers
> > > +        * defined in UHS-II addendem Ver1.01 are optional.
> > > +        */
> > > +       host->uhs2_caps.n_lanes_set = UHS2_DEV_CONFIG_GEN_SET_2L_FD_HD;
> > > +       card->uhs2_config.n_lanes_set = UHS2_DEV_CONFIG_GEN_SET_2L_FD_HD;
> >
> > [...]
> >
> > > +static int sd_uhs2_go_dormant(struct mmc_host *host, bool hibernate, u32 node_id)
> > > +{
> >
> > Looks like the in-parameter "hibernate" is superfluous, as it's always
> > set to "false" by the caller.
> >
> > > +       struct mmc_command cmd = {0};
> > > +       struct uhs2_command uhs2_cmd = {};
> > > +       u16 header = 0, arg = 0;
> > > +       u32 payload[1];
> > > +       u8 plen = 1;
> > > +       int err;
> > > +
> > > +       /* Disable Normal INT */
> > > +       if (!host->ops->uhs2_host_operation(host, UHS2_DISABLE_INT)) {
> > > +               pr_err("%s: %s: UHS2 DISABLE_INT fail!\n",
> > > +                      mmc_hostname(host), __func__);
> > > +               return -EIO;
> > > +       }
> > > +
> > > +       header = UHS2_NATIVE_PACKET | UHS2_PACKET_TYPE_CCMD | node_id;
> > > +
> > > +       arg = ((UHS2_DEV_CMD_GO_DORMANT_STATE & 0xFF) << 8) |
> > > +               UHS2_NATIVE_CMD_WRITE |
> > > +               UHS2_NATIVE_CMD_PLEN_4B |
> > > +               (UHS2_DEV_CMD_GO_DORMANT_STATE >> 8);
> > > +
> > > +       if (hibernate)
> > > +               payload[0] = UHS2_DEV_CMD_DORMANT_HIBER;
> > > +
> > > +       sd_uhs2_cmd_assemble(&cmd, &uhs2_cmd, header, arg, payload, plen, NULL, 0);
> > > +
> > > +       err = mmc_wait_for_cmd(host, &cmd, 0);
> > > +       if (err) {
> > > +               pr_err("%s: %s: UHS2 CMD send fail, err= 0x%x!\n",
> > > +                      mmc_hostname(host), __func__, err);
> > > +               return -EIO;
> > > +       }
> > > +
> > > +       /* Check Dormant State in Present */
> > > +       if (!host->ops->uhs2_host_operation(host, UHS2_CHECK_DORMANT)) {
> > > +               pr_err("%s: %s: UHS2 GO_DORMANT_STATE fail!\n",
> > > +                      mmc_hostname(host), __func__);
> > > +               return -EIO;
> > > +       }
> > > +
> > > +       host->ops->uhs2_host_operation(host, UHS2_DISABLE_CLK);
> > > +
> > >         return 0;
> > >  }
> > >
> > > +static int sd_uhs2_change_speed(struct mmc_host *host, u32 node_id)
> > > +{
> > > +       struct mmc_command cmd = {0};
> > > +       struct uhs2_command uhs2_cmd = {};
> > > +       u16 header = 0, arg = 0;
> > > +       int err;
> > > +       int timeout = 100;
> > > +
> > > +       /* Change Speed Range at controller side. */
> > > +       if (!host->ops->uhs2_host_operation(host, UHS2_SET_SPEED_B)) {
> > > +               pr_err("%s: %s: UHS2 SET_SPEED fail!\n",
> > > +                      mmc_hostname(host), __func__);
> > > +               return -EIO;
> > > +       }
> > > +
> > > +       err = sd_uhs2_go_dormant(host, false, node_id);
> > > +       if (err) {
> > > +               pr_err("%s: %s: UHS2 GO_DORMANT_STATE fail, err= 0x%x!\n",
> > > +                      mmc_hostname(host), __func__, err);
> > > +               return -EIO;
> > > +       }
> > > +
> > > +       /* restore sd clock */
> > > +       mmc_delay(5);
> > > +       host->ops->uhs2_host_operation(host, UHS2_ENABLE_CLK);
> >
> > I think the code can be a bit better structured here. More precisely,
> > since sd_uhs2_go_dormant() is the one that calls
> > ->uhs2_host_operation(host, UHS2_DISABLE_INT) and
> > ->uhs2_host_operation(host, UHS2_DISABLE_CLK), it's then up to
> > sd_uhs2_change_speed() to restore these changes.
> >
> > To me, it would be more clear if both enabling and disabling of the
> > clock /interrupt are managed in sd_uhs2_change_speed().
> >
> > > +
> > > +       /* Enable Normal INT */
> > > +       if (!host->ops->uhs2_host_operation(host, UHS2_ENABLE_INT)) {
> > > +               pr_err("%s: %s: UHS2 ENABLE_INT fail!\n",
> > > +                      mmc_hostname(host), __func__);
> > > +               return -EIO;
> > > +       }
> > > +
> > > +       /*
> > > +        * According to UHS-II Addendum Version 1.01, chapter 6.2.3, wait card
> > > +        * switch to Active State
> > > +        */
> > > +       header = UHS2_NATIVE_PACKET | UHS2_PACKET_TYPE_CCMD | node_id;
> > > +       arg = ((UHS2_DEV_CONFIG_GEN_SET & 0xFF) << 8) |
> > > +               UHS2_NATIVE_CMD_READ |
> > > +               UHS2_NATIVE_CMD_PLEN_8B |
> > > +               (UHS2_DEV_CONFIG_GEN_SET >> 8);
> > > +       do {
> > > +               sd_uhs2_cmd_assemble(&cmd, &uhs2_cmd, header, arg, NULL, 0, NULL, 0);
> > > +               err = mmc_wait_for_cmd(host, &cmd, 0);
> > > +               if (err) {
> > > +                       pr_err("%s: %s: UHS2 CMD send fail, err= 0x%x!\n",
> > > +                              mmc_hostname(host), __func__, err);
> > > +                       return -EIO;
> > > +               }
> > > +
> > > +               if (cmd.resp[1] & UHS2_DEV_CONFIG_GEN_SET_CFG_COMPLETE)
> > > +                       break;
> > > +
> > > +               timeout--;
> > > +               if (timeout == 0) {
> > > +                       pr_err("%s: %s: Not switch to Active in 100 ms\n",
> > > +                              mmc_hostname(host), __func__);
> > > +                       return -EIO;
> > > +               }
> > > +
> > > +               mmc_delay(1);
> > > +       } while (1);
> >
> > We really want to avoid these kinds of polling loops, for several
> > reasons. Please convert into using __mmc_poll_for_busy() instead.
> >
> > > +
> > > +       return 0;
> > > +}
> > > +
> > > +static int sd_uhs2_get_ro(struct mmc_host *host)
> > > +{
> > > +       int ro;
> > > +
> > > +       /*
> > > +        * Some systems don't feature a write-protect pin and don't need one.
> > > +        * E.g. because they only have micro-SD card slot. For those systems
> > > +        * assume that the SD card is always read-write.
> > > +        */
> > > +       if (host->caps2 & MMC_CAP2_NO_WRITE_PROTECT)
> > > +               return 0;
> > > +
> > > +       if (!host->ops->get_ro)
> > > +               return -1;
> > > +
> > > +       ro = host->ops->get_ro(host);
> > > +
> > > +       return ro;
> >
> > This can be replaced with mmc_sd_get_ro(). Let's avoid the open coding
> > and make that function being shared instead.
> >
> > > +}
> > > +
> > >  /*
> > >   * Initialize the UHS-II card through the SD-TRAN transport layer. This enables
> > >   * commands/requests to be backwards compatible through the legacy SD protocol.
> > > @@ -107,9 +696,127 @@ static int sd_uhs2_config_write(struct mmc_host *host, struct mmc_card *card)
> > >   */
> > >  static int sd_uhs2_legacy_init(struct mmc_host *host, struct mmc_card *card)
> > >  {
> > > +       int err;
> > > +       u32 cid[4];
> > > +       u32 ocr;
> > > +       u32 rocr = 0;
> > > +       int ro;
> > > +
> > > +       WARN_ON(!host->claimed);
> >
> > Drop this, it's an internal function, we should know that the host is
> > claimed before calling sd_uhs2_legacy_init().
> >
> > > +
> > > +       /* Send CMD0 to reset SD card */
> > > +       mmc_go_idle(host);
> > > +
> > > +       /* Send CMD8 to communicate SD interface operation condition */
> > > +       err = mmc_send_if_cond(host, host->ocr_avail);
> > > +       if (err) {
> > > +               pr_err("%s: %s: SEND_IF_COND fail!\n",
> > > +                      mmc_hostname(host), __func__);
> >
> > Please drop these prints for every command/operation that fails. We
> > already have trace/debug options for commands/requests.
> >
> > This applies to all the below code as well (perhaps there are few
> > cases not covered by the existing trace/debug support, those may be
> > converted to pr_debug().
> >
> > > +               return err;
> > > +       }
> > > +
> > > +       /*
> > > +        * Probe SD card working voltage.
> > > +        */
> > > +       err = mmc_send_app_op_cond(host, 0, &ocr);
> > > +       if (err) {
> > > +               pr_err("%s: %s: SD_SEND_OP_COND fail!\n",
> > > +                      mmc_hostname(host), __func__);
> > > +               return 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);
> >
> > If the host has MMC_CAP2_FULL_PWR_CYCLE set, mmc_select_voltage() may
> > end up calling mmc_power_cycle(). This is not going to work for
> > UHS-II.
> >
> > Either we need to modify mmc_select_voltage() so it becomes aware that
> > it can be called for UHS-II initialization, allowing it to avoid the
> > path to mmc_power_cycle() - or simply open code the part from
> > mmc_select_voltage() for UHS-II here. I think I prefer the latter.
> >
> > > +
> > > +       /*
> > > +        * Some cards have zero value of rocr in UHS-II mode. Assign host's
> > > +        * ocr value to rocr.
> > > +        */
> > > +       if (!rocr) {
> > > +               if (host->ocr_avail) {
> > > +                       rocr = host->ocr_avail;
> >
> > host->ocr_avail should really be checked in when the host driver calls
> > mmc_add_host(). It must not be zero, then we should let mmc_add_host()
> > return an error code. I look into this and send a patch for this
> > separately.
> >
> > In other words, you should not need to check it here, but just trust that's set.
> >
> > > +               } else {
> > > +                       pr_err("%s: %s: there is no valid OCR.\n",
> > > +                              mmc_hostname(host), __func__);
> > > +                       return -EINVAL;
> > > +               }
> > > +       }
> > > +
> > > +       /* Wait SD power on ready */
> > > +       ocr = rocr;
> > > +       err = mmc_send_app_op_cond(host, ocr, &rocr);
> > > +       if (err) {
> > > +               pr_err("%s: %s: SD_SEND_OP_COND fail!\n", mmc_hostname(host),
> > > +                      __func__);
> > > +               return err;
> > > +       }
> > > +
> > > +       err = mmc_send_cid(host, cid);
> > > +       if (err) {
> > > +               pr_err("%s: %s: SD_SEND_CID fail!\n", mmc_hostname(host),
> > > +                      __func__);
> > > +               return err;
> > > +       }
> > > +       memcpy(card->raw_cid, cid, sizeof(card->raw_cid));
> > > +
> > > +       /*
> > > +        * Call the optional HC's init_card function to handle quirks.
> > > +        */
> > > +       if (host->ops->init_card)
> > > +               host->ops->init_card(host, card);
> >
> > This can be removed, as it's only for the legacy interface, I think.
> >
> > > +
> > > +       /*
> > > +        * For native busses:  get card RCA and quit open drain mode.
> > > +        */
> > > +       err = mmc_send_relative_addr(host, &card->rca);
> > > +       if (err) {
> > > +               pr_err("%s: %s: SD_SEND_RCA fail!\n", mmc_hostname(host),
> > > +                      __func__);
> > > +               return err;
> > > +       }
> > > +
> > > +       err = mmc_sd_get_csd(card);
> > > +       if (err) {
> > > +               pr_err("%s: %s: SD_SEND_CSD fail!\n", mmc_hostname(host),
> > > +                      __func__);
> > > +               return err;
> > > +       }
> > > +
> > > +       /*
> > > +        * Select card, as all following commands rely on that.
> > > +        */
> > > +       err = mmc_select_card(card);
> > > +       if (err) {
> > > +               pr_err("%s: %s: SD_SEL_DSEL fail!\n", mmc_hostname(host),
> > > +                      __func__);
> > > +               return err;
> > > +       }
> > > +
> > > +       /*
> > > +        * Check if read-only switch is active.
> > > +        */
> > > +       ro = sd_uhs2_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);
> > > +       }
> > > +
> > >         return 0;
> > >  }
> > >
> > > +static void sd_uhs2_remove(struct mmc_host *host)
> > > +{
> > > +       mmc_remove_card(host->card);
> > > +       host->card = NULL;
> > > +}
> > > +
> > >  /*
> > >   * Allocate the data structure for the mmc_card and run the UHS-II specific
> > >   * initialization sequence.
> > > @@ -121,16 +828,21 @@ static int sd_uhs2_init_card(struct mmc_host *host)
> > >         int err;
> > >
> > >         err = sd_uhs2_dev_init(host);
> > > -       if (err)
> > > +       if (err) {
> > > +               pr_err("%s: UHS2 DEVICE_INIT fail!\n", mmc_hostname(host));
> > >                 return err;
> > > +       }
> > >
> > >         err = sd_uhs2_enum(host, &node_id);
> > > -       if (err)
> > > +       if (err) {
> > > +               pr_err("%s: UHS2 ENUMERATE fail!\n", mmc_hostname(host));
> > >                 return err;
> > > +       }
> > >
> > >         card = mmc_alloc_card(host, &sd_type);
> > >         if (IS_ERR(card))
> > >                 return PTR_ERR(card);
> > > +       host->card = card;
> > >
> > >         card->uhs2_config.node_id = node_id;
> > >         card->type = MMC_TYPE_SD;
> > > @@ -139,6 +851,16 @@ static int sd_uhs2_init_card(struct mmc_host *host)
> > >         if (err)
> > >                 goto err;
> > >
> > > +       /* Change to Speed Range B if it is supported */
> > > +       if (host->uhs2_caps.flags & MMC_UHS2_SPEED_B) {
> > > +               err = sd_uhs2_change_speed(host, node_id);
> > > +               if (err) {
> > > +                       pr_err("%s: %s: UHS2 sd_uhs2_change_speed() fail!\n",
> > > +                              mmc_hostname(host), __func__);
> > > +                       return err;
> > > +               }
> > > +       }
> > > +
> > >         err = sd_uhs2_config_write(host, card);
> > >         if (err)
> > >                 goto err;
> > > @@ -147,20 +869,13 @@ static int sd_uhs2_init_card(struct mmc_host *host)
> > >         if (err)
> > >                 goto err;
> > >
> > > -       host->card = card;
> > >         return 0;
> > >
> > >  err:
> > > -       mmc_remove_card(card);
> > > +       sd_uhs2_remove(host);
> > >         return err;
> > >  }
> >
> > [...]
> >
> > Kind regards
> > Uffe

On Thu, Mar 24, 2022 at 6:50 PM AKASHI Takahiro
<takahiro.akashi@linaro.org> wrote:
>
> On Thu, Mar 24, 2022 at 11:22:16AM +0100, Ulf Hansson wrote:
> > On Thu, 24 Mar 2022 at 02:29, AKASHI Takahiro
> > <takahiro.akashi@linaro.org> wrote:
> > >
> > > On Wed, Mar 23, 2022 at 05:15:59PM +0100, Ulf Hansson wrote:
> > > > On Tue, 22 Feb 2022 at 04:40, Jason Lai <jasonlai.genesyslogic@gmail.com> wrote:
> > > > >
> > > > > From: Jason Lai <jason.lai@genesyslogic.com.tw>
> > > > >
> > > > > UHS-II card initialization flow is divided into 2 categories: PHY & Card.
> > > > > Part 1 - PHY Initialization:
> > > > >          Every host controller may need their own private operation to
> > > > >          establish LINK between controller and card. So we add a new member
> > > > >          function(uhs2_detect_init) in struct mmc_host_ops for host
> > > > >          controller use.
> > > > > Part 2 - Card Initialization:
> > > > >          This part can be divided into 6 substeps.
> > > > >          1. Send UHS-II CCMD DEVICE_INIT to card.
> > > > >          2. Send UHS-II CCMD ENUMERATE to card.
> > > > >          3. Send UHS-II Native Read CCMD to obtain capabilities in CFG_REG
> > > > >             of card.
> > > > >          4. Host compares capabilities of host controller and card, then
> > > > >             write the negotiated values to Setting field in CFG_REG of card
> > > > >             through UHS-II Native Write CCMD.
> > > > >          5. Switch host controller's clock to Range B if it is supported by
> > > > >             both host controller and card.
> > > > >          6. Execute legacy SD initialization flow.
> > > > > Part 3 - Provide a function to transform legacy SD command packet into
> > > > >          UHS-II SD-TRAN DCMD packet.
> > > > >
> > > > > Most of the code added above came from Intel's original patch[3].
> > > > >
> > > > > [3]
> > > > > https://patchwork.kernel.org/project/linux-mmc/patch/1419672479-30852-2-
> > > > > git-send-email-yi.y.sun@intel.com/
> > >
> > > To honor the original work, we should add Intel's copyright notice here
> > > as I did before.
> >
> > I think Jason already did, at least for some of the files in this series.
> >
> > Note also that, the initial code that was posted, is very far from
> > what code that is going to be merged. Simply because the quality was
> > very poor and not acceptable for the upstream kernel.
>
> I don't think so.
> I don't see much difference from my modified version[1] which is also
> based on Intel's original work.
>
> [1] https://www.spinics.net/lists/linux-mmc/msg57321.html
>

I agree with most of what Takahiro said. The data structure and command
packet for UHS-II followed Intel's original work. But Intel's original work
cannot actually work. UHS-II SD card was supported successfully after Ben
and Takahiro's hard working on patch set [RFC V3.1] Add support UHS-II for
GL9755: https://patchwork.kernel.org/project/linux-mmc/list/?series=378627&archive=both

In my opinion, keeping Intel's copyright information at head of sd_uhs2.c and
sd_uhs2.h like listed below is a good idea. What do you think?

// SPDX-License-Identifier: GPL-2.0-or-later
/*
 *  linux/drivers/mmc/core/uhs2.c - UHS-II driver
 *
 *  Copyright (C) 2020 Linaro Limited
 *  Author: Ulf Hansson <ulf.hansson@linaro.org>
 *  Copyright (C) 2014 Intel Corp, All Rights Reserved.
 *  Copyright (C) 2020 Genesys Logic, Inc.
 *  Authors: Ben Chuang <ben.chuang@genesyslogic.com.tw>
 *  Authors: Jason Lai <jason.lai@genesyslogic.com.tw>
 *  Copyright (C) 2020 Linaro Limited
 *  Author: AKASHI Takahiro <takahiro.akashi@linaro.org>
 */

kind regards,
Jason Lai

> -Takahiro Akashi
>
> > That said, I am
> > not sure we need to keep the copyrights for this, but I leave that
> > call to Jason to decide.
> >
> > >
> > > -Takahiro Akashi
> > >
> >
> > Kind regards
> > Uffe
> >
> > >
> > > > > Signed-off-by: Jason Lai <jason.lai@genesyslogic.com.tw>
> > > > > ---
> > > > >  drivers/mmc/core/sd_uhs2.c | 835 ++++++++++++++++++++++++++++++++++++-
> > > > >  1 file changed, 817 insertions(+), 18 deletions(-)
> > > > >
> > > > > diff --git a/drivers/mmc/core/sd_uhs2.c b/drivers/mmc/core/sd_uhs2.c
> > > > > index 800957f74632..f1e8e30301eb 100644
> > > > > --- a/drivers/mmc/core/sd_uhs2.c
> > > > > +++ b/drivers/mmc/core/sd_uhs2.c
> > > > > @@ -3,6 +3,7 @@
> > > > >   * Copyright (C) 2021 Linaro Ltd
> > > > >   *
> > > > >   * Author: Ulf Hansson <ulf.hansson@linaro.org>
> > > > > + * Author: Jason Lai <jason.lai@genesyslogic.com.tw>
> > > > >   *
> > > > >   * Support for SD UHS-II cards
> > > > >   */
> > > > > @@ -10,19 +11,31 @@
> > > > >
> > > > >  #include <linux/mmc/host.h>
> > > > >  #include <linux/mmc/card.h>
> > > > > +#include <linux/mmc/mmc.h>
> > > > > +#include <linux/mmc/sd_uhs2.h>
> > > > >
> > > > >  #include "core.h"
> > > > >  #include "bus.h"
> > > > > +#include "card.h"
> > > > >  #include "sd.h"
> > > > > +#include "sd_ops.h"
> > > > >  #include "mmc_ops.h"
> > > > > +#include "sd_uhs2.h"
> > > > >
> > > > >  static const unsigned int sd_uhs2_freqs[] = { 52000000, 26000000 };
> > > > >
> > > > >  static int sd_uhs2_set_ios(struct mmc_host *host)
> > > > >  {
> > > > >         struct mmc_ios *ios = &host->ios;
> > > > > +       int err = 0;
> > > > >
> > > > > -       return host->ops->uhs2_set_ios(host, ios);
> > > > > +       pr_debug("%s: clock %uHz powermode %u Vdd %u timing %u\n",
> > > > > +                mmc_hostname(host), ios->clock, ios->power_mode, ios->vdd,
> > > > > +                ios->timing);
> > > > > +
> > > > > +       host->ops->set_ios(host, ios);
> > > >
> > > > We discussed using the ->set_ios() callback in a previous version. To
> > > > repeat myself, I don't think it's a good idea. UHS-II needs an
> > > > entirely different power sequence than the legacy interface(s), hence
> > > > I think it's simply cleaner to separate them.
> > > >
> > > > To move forward, I see two options.
> > > > 1) Use only the ->uhs2_host_operation() ops.
> > > > 2) Use a combination of the ->uhs2_set_ios() ops and the
> > > > ->uhs2_host_operation() ops.
> > > >
> > > > Both options work for me. However, perhaps if you could incorporate
> > > > the changes done on the host driver at next submission, it becomes
> > > > easier for me to understand what makes best sense.
> > > >
> > > > > +
> > > > > +       return err;
> > > > >  }
> > > > >
> > > > >  static int sd_uhs2_power_up(struct mmc_host *host)
> > > > > @@ -45,6 +58,43 @@ static void sd_uhs2_power_off(struct mmc_host *host)
> > > > >         sd_uhs2_set_ios(host);
> > > > >  }
> > > >
> > > > [...]
> > > >
> > > > >
> > > > >  /*
> > > > > @@ -61,6 +119,77 @@ static int sd_uhs2_phy_init(struct mmc_host *host)
> > > > >   */
> > > > >  static int sd_uhs2_dev_init(struct mmc_host *host)
> > > > >  {
> > > > > +       struct mmc_command cmd = {0};
> > > > > +       struct uhs2_command uhs2_cmd = {};
> > > > > +       u32 cnt;
> > > > > +       u32 dap, gap, resp_gap;
> > > > > +       u16 header = 0, arg = 0;
> > > >
> > > > No need to initiate these.
> > > >
> > > > > +       u32 payload[1];
> > > >
> > > > u32?
> > > >
> > > > > +       u8 plen = 1;
> > > > > +       u8 gd = 0, cf = 1;
> > > > > +       u8 resp[6] = {0};
> > > > > +       u8 resp_len = 6;
> > > >
> > > > Many of these variables are just constant numbers. If it makes sense
> > > > to add definitions for them, then please do that instead. If not, just
> > > > give the value directly in the code.
> > > >
> > > > For example: plen = 1; (I assume that is payload length). This can
> > > > just be given as an in-parameter to sd_uhs2_cmd_assemble(), without
> > > > further explanation.
> > > >
> > > > The point is, sd_uhs2_cmd_assemble() should have a well described
> > > > description of its in-parameters, so no need for further descriptions,
> > > > I think.
> > > >
> > > > This comment applies to all the new code/functions that are added in
> > > > the $subject patch. Please go through all of the code and fix this.
> > > >
> > > >
> > > > > +       int err;
> > > > > +
> > > > > +       dap = host->uhs2_caps.dap;
> > > > > +       gap = host->uhs2_caps.gap;
> > > > > +
> > > > > +       header = UHS2_NATIVE_PACKET | UHS2_PACKET_TYPE_CCMD;
> > > > > +       arg = ((UHS2_DEV_CMD_DEVICE_INIT & 0xFF) << 8) |
> > > > > +              UHS2_NATIVE_CMD_WRITE |
> > > > > +              UHS2_NATIVE_CMD_PLEN_4B |
> > > > > +              (UHS2_DEV_CMD_DEVICE_INIT >> 8);
> > > > > +
> > > > > +       /*
> > > > > +        * Refer to UHS-II Addendum Version 1.02 section 6.3.1.
> > > > > +        * Max. time from DEVICE_INIT CCMD EOP reception on Device
> > > > > +        * Rx to its SOP transmission on Device Tx(Tfwd_init_cmd) is
> > > > > +        * 1 second.
> > > > > +        */
> > > > > +       cmd.busy_timeout = 1000;
> > > > > +
> > > > > +       /*
> > > > > +        * Refer to UHS-II Addendum Version 1.02 section 6.2.6.3.
> > > > > +        * When the number of the DEVICE_INIT commands is reach to
> > > > > +        * 30 tiems, Host shall stop issuing DEVICE_INIT command
> > > > > +        * and regard it as an error.
> > > > > +        */
> > > > > +       for (cnt = 0; cnt < 30; cnt++) {
> > > > > +               payload[0] = ((dap & 0xF) << 12) |
> > > > > +                             (cf << 11)         |
> > > > > +                             ((gd & 0xF) << 4)  |
> > > > > +                             (gap & 0xF);
> > > >
> > > > To me, it looks like the payload data deserves to be explained a bit.
> > > > Perhaps you can add a comment explaining what pieces it consists of so
> > > > this becomes more clear?
> > > >
> > > > > +
> > > > > +               sd_uhs2_cmd_assemble(&cmd, &uhs2_cmd, header, arg,  payload, plen, resp, resp_len);
> > > > > +
> > > > > +               err = mmc_wait_for_cmd(host, &cmd, 0);
> > > > > +
> > > > > +               if (err) {
> > > > > +                       pr_err("%s: %s: UHS2 CMD send fail, err= 0x%x!\n",
> > > > > +                              mmc_hostname(host), __func__, err);
> > > > > +                       return -EIO;
> > > >
> > > > Why do you override the original error code that was returned from
> > > > mmc_wait_for_cmd()?
> > > >
> > > > Normally it's preferred to keep the error code, unless there is good
> > > > reason not to.
> > > >
> > > > Again, I won't add more comments like this in the code from the
> > > > $subject patch. But please go through it all to avoid this kind of
> > > > thing.
> > > >
> > > > > +               }
> > > > > +
> > > > > +               if (resp[3] != (UHS2_DEV_CMD_DEVICE_INIT & 0xFF)) {
> > > > > +                       pr_err("%s: DEVICE_INIT response is wrong!\n",
> > > > > +                              mmc_hostname(host));
> > > > > +                       return -EIO;
> > > > > +               }
> > > > > +
> > > > > +               if (resp[5] & 0x8) {
> > > > > +                       host->uhs2_caps.group_desc = gd;
> > > > > +                       break;
> > > >
> > > > I suggest you do a return 0 here. In this way you can skip the check
> > > > "if (cnt == 30)" below and just return an error code instead.
> > > >
> > > > > +               }
> > > > > +               resp_gap = resp[4] & 0x0F;
> > > > > +               if (gap == resp_gap)
> > > > > +                       gd++;
> > > > > +       }
> > > > > +       if (cnt == 30) {
> > > > > +               pr_err("%s: DEVICE_INIT fail, already 30 times!\n",
> > > > > +                      mmc_hostname(host));
> > > > > +               return -EIO;
> > > > > +       }
> > > > > +
> > > > >         return 0;
> > > > >  }
> > > > >
> > > >
> > > > >  static int sd_uhs2_config_read(struct mmc_host *host, struct mmc_card *card)
> > > > >  {
> > > > > +       struct mmc_command cmd = {0};
> > > > > +       struct uhs2_command uhs2_cmd = {};
> > > > > +       u16 header = 0, arg = 0;
> > > > > +       u32 cap;
> > > > > +       int err;
> > > > > +
> > > > > +       header = UHS2_NATIVE_PACKET |
> > > > > +                UHS2_PACKET_TYPE_CCMD |
> > > > > +                card->uhs2_config.node_id;
> > > > > +       arg = ((UHS2_DEV_CONFIG_GEN_CAPS & 0xFF) << 8) |
> > > > > +              UHS2_NATIVE_CMD_READ |
> > > > > +              UHS2_NATIVE_CMD_PLEN_4B |
> > > > > +              (UHS2_DEV_CONFIG_GEN_CAPS >> 8);
> > > > > +
> > > > > +       /* There is no payload because per spec, there should be
> > > > > +        * no payload field for read CCMD.
> > > > > +        * Plen is set in arg. Per spec, plen for read CCMD
> > > > > +        * represents the len of read data which is assigned in payload
> > > > > +        * of following RES (p136).
> > > > > +        */
> > > > > +       sd_uhs2_cmd_assemble(&cmd, &uhs2_cmd, header, arg, NULL, 0, NULL, 0);
> > > >
> > > > We are reading the configuration data here and onwards, piece by
> > > > piece. Perhaps if you can add a small comment about each piece we are
> > > > reading, before each call to mmc_wait_for_cmd(), that can help to
> > > > easier understand what goes on.
> > > >
> > > > [...]
> > > >
> > > > >  static int sd_uhs2_config_write(struct mmc_host *host, struct mmc_card *card)
> > > > >  {
> > > > > +       struct mmc_command cmd = {0};
> > > > > +       struct uhs2_command uhs2_cmd = {};
> > > > > +       u16 header = 0, arg = 0;
> > > > > +       u32 payload[2];
> > > > > +       u8 nMinDataGap;
> > > > > +       u8 plen;
> > > > > +       int err;
> > > > > +       u8 resp[5] = {0};
> > > > > +       u8 resp_len = 5;
> > > > > +
> > > > > +       header = UHS2_NATIVE_PACKET |
> > > > > +                UHS2_PACKET_TYPE_CCMD | card->uhs2_config.node_id;
> > > > > +       arg = ((UHS2_DEV_CONFIG_GEN_SET & 0xFF) << 8) |
> > > > > +              UHS2_NATIVE_CMD_WRITE |
> > > > > +              UHS2_NATIVE_CMD_PLEN_8B |
> > > > > +              (UHS2_DEV_CONFIG_GEN_SET >> 8);
> > > > > +
> > > > > +       if (card->uhs2_config.n_lanes == UHS2_DEV_CONFIG_2L_HD_FD &&
> > > > > +           host->uhs2_caps.n_lanes == UHS2_DEV_CONFIG_2L_HD_FD) {
> > > > > +               /* Support HD */
> > > > > +               host->uhs2_caps.flags |= MMC_UHS2_2L_HD;
> > > >
> > > > How is the uhs2_caps.flags field intended to be used? To me it looks
> > > > like a way for the mmc core to exchange status/configuration
> > > > information about the initialization process of the card, with the mmc
> > > > host driver. Perhaps there is more too. Is that correct?
> > > >
> > > > If so, I think it looks quite similar for what we have in the struct
> > > > mmc_ios, for the legacy interface(s). I am not saying we should use
> > > > that, just trying to understand what would suit best here.
> > > >
> > > > > +               nMinDataGap = 1;
> > > > > +       } else {
> > > > > +               /* Only support 2L-FD so far */
> > > > > +               host->uhs2_caps.flags &= ~MMC_UHS2_2L_HD;
> > > > > +               nMinDataGap = 3;
> > > > > +       }
> > > > > +
> > > > > +       /*
> > > > > +        * Most UHS-II cards only support FD and 2L-HD mode. Other lane numbers
> > > > > +        * defined in UHS-II addendem Ver1.01 are optional.
> > > > > +        */
> > > > > +       host->uhs2_caps.n_lanes_set = UHS2_DEV_CONFIG_GEN_SET_2L_FD_HD;
> > > > > +       card->uhs2_config.n_lanes_set = UHS2_DEV_CONFIG_GEN_SET_2L_FD_HD;
> > > >
> > > > [...]
> > > >
> > > > > +static int sd_uhs2_go_dormant(struct mmc_host *host, bool hibernate, u32 node_id)
> > > > > +{
> > > >
> > > > Looks like the in-parameter "hibernate" is superfluous, as it's always
> > > > set to "false" by the caller.
> > > >
> > > > > +       struct mmc_command cmd = {0};
> > > > > +       struct uhs2_command uhs2_cmd = {};
> > > > > +       u16 header = 0, arg = 0;
> > > > > +       u32 payload[1];
> > > > > +       u8 plen = 1;
> > > > > +       int err;
> > > > > +
> > > > > +       /* Disable Normal INT */
> > > > > +       if (!host->ops->uhs2_host_operation(host, UHS2_DISABLE_INT)) {
> > > > > +               pr_err("%s: %s: UHS2 DISABLE_INT fail!\n",
> > > > > +                      mmc_hostname(host), __func__);
> > > > > +               return -EIO;
> > > > > +       }
> > > > > +
> > > > > +       header = UHS2_NATIVE_PACKET | UHS2_PACKET_TYPE_CCMD | node_id;
> > > > > +
> > > > > +       arg = ((UHS2_DEV_CMD_GO_DORMANT_STATE & 0xFF) << 8) |
> > > > > +               UHS2_NATIVE_CMD_WRITE |
> > > > > +               UHS2_NATIVE_CMD_PLEN_4B |
> > > > > +               (UHS2_DEV_CMD_GO_DORMANT_STATE >> 8);
> > > > > +
> > > > > +       if (hibernate)
> > > > > +               payload[0] = UHS2_DEV_CMD_DORMANT_HIBER;
> > > > > +
> > > > > +       sd_uhs2_cmd_assemble(&cmd, &uhs2_cmd, header, arg, payload, plen, NULL, 0);
> > > > > +
> > > > > +       err = mmc_wait_for_cmd(host, &cmd, 0);
> > > > > +       if (err) {
> > > > > +               pr_err("%s: %s: UHS2 CMD send fail, err= 0x%x!\n",
> > > > > +                      mmc_hostname(host), __func__, err);
> > > > > +               return -EIO;
> > > > > +       }
> > > > > +
> > > > > +       /* Check Dormant State in Present */
> > > > > +       if (!host->ops->uhs2_host_operation(host, UHS2_CHECK_DORMANT)) {
> > > > > +               pr_err("%s: %s: UHS2 GO_DORMANT_STATE fail!\n",
> > > > > +                      mmc_hostname(host), __func__);
> > > > > +               return -EIO;
> > > > > +       }
> > > > > +
> > > > > +       host->ops->uhs2_host_operation(host, UHS2_DISABLE_CLK);
> > > > > +
> > > > >         return 0;
> > > > >  }
> > > > >
> > > > > +static int sd_uhs2_change_speed(struct mmc_host *host, u32 node_id)
> > > > > +{
> > > > > +       struct mmc_command cmd = {0};
> > > > > +       struct uhs2_command uhs2_cmd = {};
> > > > > +       u16 header = 0, arg = 0;
> > > > > +       int err;
> > > > > +       int timeout = 100;
> > > > > +
> > > > > +       /* Change Speed Range at controller side. */
> > > > > +       if (!host->ops->uhs2_host_operation(host, UHS2_SET_SPEED_B)) {
> > > > > +               pr_err("%s: %s: UHS2 SET_SPEED fail!\n",
> > > > > +                      mmc_hostname(host), __func__);
> > > > > +               return -EIO;
> > > > > +       }
> > > > > +
> > > > > +       err = sd_uhs2_go_dormant(host, false, node_id);
> > > > > +       if (err) {
> > > > > +               pr_err("%s: %s: UHS2 GO_DORMANT_STATE fail, err= 0x%x!\n",
> > > > > +                      mmc_hostname(host), __func__, err);
> > > > > +               return -EIO;
> > > > > +       }
> > > > > +
> > > > > +       /* restore sd clock */
> > > > > +       mmc_delay(5);
> > > > > +       host->ops->uhs2_host_operation(host, UHS2_ENABLE_CLK);
> > > >
> > > > I think the code can be a bit better structured here. More precisely,
> > > > since sd_uhs2_go_dormant() is the one that calls
> > > > ->uhs2_host_operation(host, UHS2_DISABLE_INT) and
> > > > ->uhs2_host_operation(host, UHS2_DISABLE_CLK), it's then up to
> > > > sd_uhs2_change_speed() to restore these changes.
> > > >
> > > > To me, it would be more clear if both enabling and disabling of the
> > > > clock /interrupt are managed in sd_uhs2_change_speed().
> > > >
> > > > > +
> > > > > +       /* Enable Normal INT */
> > > > > +       if (!host->ops->uhs2_host_operation(host, UHS2_ENABLE_INT)) {
> > > > > +               pr_err("%s: %s: UHS2 ENABLE_INT fail!\n",
> > > > > +                      mmc_hostname(host), __func__);
> > > > > +               return -EIO;
> > > > > +       }
> > > > > +
> > > > > +       /*
> > > > > +        * According to UHS-II Addendum Version 1.01, chapter 6.2.3, wait card
> > > > > +        * switch to Active State
> > > > > +        */
> > > > > +       header = UHS2_NATIVE_PACKET | UHS2_PACKET_TYPE_CCMD | node_id;
> > > > > +       arg = ((UHS2_DEV_CONFIG_GEN_SET & 0xFF) << 8) |
> > > > > +               UHS2_NATIVE_CMD_READ |
> > > > > +               UHS2_NATIVE_CMD_PLEN_8B |
> > > > > +               (UHS2_DEV_CONFIG_GEN_SET >> 8);
> > > > > +       do {
> > > > > +               sd_uhs2_cmd_assemble(&cmd, &uhs2_cmd, header, arg, NULL, 0, NULL, 0);
> > > > > +               err = mmc_wait_for_cmd(host, &cmd, 0);
> > > > > +               if (err) {
> > > > > +                       pr_err("%s: %s: UHS2 CMD send fail, err= 0x%x!\n",
> > > > > +                              mmc_hostname(host), __func__, err);
> > > > > +                       return -EIO;
> > > > > +               }
> > > > > +
> > > > > +               if (cmd.resp[1] & UHS2_DEV_CONFIG_GEN_SET_CFG_COMPLETE)
> > > > > +                       break;
> > > > > +
> > > > > +               timeout--;
> > > > > +               if (timeout == 0) {
> > > > > +                       pr_err("%s: %s: Not switch to Active in 100 ms\n",
> > > > > +                              mmc_hostname(host), __func__);
> > > > > +                       return -EIO;
> > > > > +               }
> > > > > +
> > > > > +               mmc_delay(1);
> > > > > +       } while (1);
> > > >
> > > > We really want to avoid these kinds of polling loops, for several
> > > > reasons. Please convert into using __mmc_poll_for_busy() instead.
> > > >
> > > > > +
> > > > > +       return 0;
> > > > > +}
> > > > > +
> > > > > +static int sd_uhs2_get_ro(struct mmc_host *host)
> > > > > +{
> > > > > +       int ro;
> > > > > +
> > > > > +       /*
> > > > > +        * Some systems don't feature a write-protect pin and don't need one.
> > > > > +        * E.g. because they only have micro-SD card slot. For those systems
> > > > > +        * assume that the SD card is always read-write.
> > > > > +        */
> > > > > +       if (host->caps2 & MMC_CAP2_NO_WRITE_PROTECT)
> > > > > +               return 0;
> > > > > +
> > > > > +       if (!host->ops->get_ro)
> > > > > +               return -1;
> > > > > +
> > > > > +       ro = host->ops->get_ro(host);
> > > > > +
> > > > > +       return ro;
> > > >
> > > > This can be replaced with mmc_sd_get_ro(). Let's avoid the open coding
> > > > and make that function being shared instead.
> > > >
> > > > > +}
> > > > > +
> > > > >  /*
> > > > >   * Initialize the UHS-II card through the SD-TRAN transport layer. This enables
> > > > >   * commands/requests to be backwards compatible through the legacy SD protocol.
> > > > > @@ -107,9 +696,127 @@ static int sd_uhs2_config_write(struct mmc_host *host, struct mmc_card *card)
> > > > >   */
> > > > >  static int sd_uhs2_legacy_init(struct mmc_host *host, struct mmc_card *card)
> > > > >  {
> > > > > +       int err;
> > > > > +       u32 cid[4];
> > > > > +       u32 ocr;
> > > > > +       u32 rocr = 0;
> > > > > +       int ro;
> > > > > +
> > > > > +       WARN_ON(!host->claimed);
> > > >
> > > > Drop this, it's an internal function, we should know that the host is
> > > > claimed before calling sd_uhs2_legacy_init().
> > > >
> > > > > +
> > > > > +       /* Send CMD0 to reset SD card */
> > > > > +       mmc_go_idle(host);
> > > > > +
> > > > > +       /* Send CMD8 to communicate SD interface operation condition */
> > > > > +       err = mmc_send_if_cond(host, host->ocr_avail);
> > > > > +       if (err) {
> > > > > +               pr_err("%s: %s: SEND_IF_COND fail!\n",
> > > > > +                      mmc_hostname(host), __func__);
> > > >
> > > > Please drop these prints for every command/operation that fails. We
> > > > already have trace/debug options for commands/requests.
> > > >
> > > > This applies to all the below code as well (perhaps there are few
> > > > cases not covered by the existing trace/debug support, those may be
> > > > converted to pr_debug().
> > > >
> > > > > +               return err;
> > > > > +       }
> > > > > +
> > > > > +       /*
> > > > > +        * Probe SD card working voltage.
> > > > > +        */
> > > > > +       err = mmc_send_app_op_cond(host, 0, &ocr);
> > > > > +       if (err) {
> > > > > +               pr_err("%s: %s: SD_SEND_OP_COND fail!\n",
> > > > > +                      mmc_hostname(host), __func__);
> > > > > +               return 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);
> > > >
> > > > If the host has MMC_CAP2_FULL_PWR_CYCLE set, mmc_select_voltage() may
> > > > end up calling mmc_power_cycle(). This is not going to work for
> > > > UHS-II.
> > > >
> > > > Either we need to modify mmc_select_voltage() so it becomes aware that
> > > > it can be called for UHS-II initialization, allowing it to avoid the
> > > > path to mmc_power_cycle() - or simply open code the part from
> > > > mmc_select_voltage() for UHS-II here. I think I prefer the latter.
> > > >
> > > > > +
> > > > > +       /*
> > > > > +        * Some cards have zero value of rocr in UHS-II mode. Assign host's
> > > > > +        * ocr value to rocr.
> > > > > +        */
> > > > > +       if (!rocr) {
> > > > > +               if (host->ocr_avail) {
> > > > > +                       rocr = host->ocr_avail;
> > > >
> > > > host->ocr_avail should really be checked in when the host driver calls
> > > > mmc_add_host(). It must not be zero, then we should let mmc_add_host()
> > > > return an error code. I look into this and send a patch for this
> > > > separately.
> > > >
> > > > In other words, you should not need to check it here, but just trust that's set.
> > > >
> > > > > +               } else {
> > > > > +                       pr_err("%s: %s: there is no valid OCR.\n",
> > > > > +                              mmc_hostname(host), __func__);
> > > > > +                       return -EINVAL;
> > > > > +               }
> > > > > +       }
> > > > > +
> > > > > +       /* Wait SD power on ready */
> > > > > +       ocr = rocr;
> > > > > +       err = mmc_send_app_op_cond(host, ocr, &rocr);
> > > > > +       if (err) {
> > > > > +               pr_err("%s: %s: SD_SEND_OP_COND fail!\n", mmc_hostname(host),
> > > > > +                      __func__);
> > > > > +               return err;
> > > > > +       }
> > > > > +
> > > > > +       err = mmc_send_cid(host, cid);
> > > > > +       if (err) {
> > > > > +               pr_err("%s: %s: SD_SEND_CID fail!\n", mmc_hostname(host),
> > > > > +                      __func__);
> > > > > +               return err;
> > > > > +       }
> > > > > +       memcpy(card->raw_cid, cid, sizeof(card->raw_cid));
> > > > > +
> > > > > +       /*
> > > > > +        * Call the optional HC's init_card function to handle quirks.
> > > > > +        */
> > > > > +       if (host->ops->init_card)
> > > > > +               host->ops->init_card(host, card);
> > > >
> > > > This can be removed, as it's only for the legacy interface, I think.
> > > >
> > > > > +
> > > > > +       /*
> > > > > +        * For native busses:  get card RCA and quit open drain mode.
> > > > > +        */
> > > > > +       err = mmc_send_relative_addr(host, &card->rca);
> > > > > +       if (err) {
> > > > > +               pr_err("%s: %s: SD_SEND_RCA fail!\n", mmc_hostname(host),
> > > > > +                      __func__);
> > > > > +               return err;
> > > > > +       }
> > > > > +
> > > > > +       err = mmc_sd_get_csd(card);
> > > > > +       if (err) {
> > > > > +               pr_err("%s: %s: SD_SEND_CSD fail!\n", mmc_hostname(host),
> > > > > +                      __func__);
> > > > > +               return err;
> > > > > +       }
> > > > > +
> > > > > +       /*
> > > > > +        * Select card, as all following commands rely on that.
> > > > > +        */
> > > > > +       err = mmc_select_card(card);
> > > > > +       if (err) {
> > > > > +               pr_err("%s: %s: SD_SEL_DSEL fail!\n", mmc_hostname(host),
> > > > > +                      __func__);
> > > > > +               return err;
> > > > > +       }
> > > > > +
> > > > > +       /*
> > > > > +        * Check if read-only switch is active.
> > > > > +        */
> > > > > +       ro = sd_uhs2_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);
> > > > > +       }
> > > > > +
> > > > >         return 0;
> > > > >  }
> > > > >
> > > > > +static void sd_uhs2_remove(struct mmc_host *host)
> > > > > +{
> > > > > +       mmc_remove_card(host->card);
> > > > > +       host->card = NULL;
> > > > > +}
> > > > > +
> > > > >  /*
> > > > >   * Allocate the data structure for the mmc_card and run the UHS-II specific
> > > > >   * initialization sequence.
> > > > > @@ -121,16 +828,21 @@ static int sd_uhs2_init_card(struct mmc_host *host)
> > > > >         int err;
> > > > >
> > > > >         err = sd_uhs2_dev_init(host);
> > > > > -       if (err)
> > > > > +       if (err) {
> > > > > +               pr_err("%s: UHS2 DEVICE_INIT fail!\n", mmc_hostname(host));
> > > > >                 return err;
> > > > > +       }
> > > > >
> > > > >         err = sd_uhs2_enum(host, &node_id);
> > > > > -       if (err)
> > > > > +       if (err) {
> > > > > +               pr_err("%s: UHS2 ENUMERATE fail!\n", mmc_hostname(host));
> > > > >                 return err;
> > > > > +       }
> > > > >
> > > > >         card = mmc_alloc_card(host, &sd_type);
> > > > >         if (IS_ERR(card))
> > > > >                 return PTR_ERR(card);
> > > > > +       host->card = card;
> > > > >
> > > > >         card->uhs2_config.node_id = node_id;
> > > > >         card->type = MMC_TYPE_SD;
> > > > > @@ -139,6 +851,16 @@ static int sd_uhs2_init_card(struct mmc_host *host)
> > > > >         if (err)
> > > > >                 goto err;
> > > > >
> > > > > +       /* Change to Speed Range B if it is supported */
> > > > > +       if (host->uhs2_caps.flags & MMC_UHS2_SPEED_B) {
> > > > > +               err = sd_uhs2_change_speed(host, node_id);
> > > > > +               if (err) {
> > > > > +                       pr_err("%s: %s: UHS2 sd_uhs2_change_speed() fail!\n",
> > > > > +                              mmc_hostname(host), __func__);
> > > > > +                       return err;
> > > > > +               }
> > > > > +       }
> > > > > +
> > > > >         err = sd_uhs2_config_write(host, card);
> > > > >         if (err)
> > > > >                 goto err;
> > > > > @@ -147,20 +869,13 @@ static int sd_uhs2_init_card(struct mmc_host *host)
> > > > >         if (err)
> > > > >                 goto err;
> > > > >
> > > > > -       host->card = card;
> > > > >         return 0;
> > > > >
> > > > >  err:
> > > > > -       mmc_remove_card(card);
> > > > > +       sd_uhs2_remove(host);
> > > > >         return err;
> > > > >  }
> > > >
> > > > [...]
> > > >
> > > > Kind regards
> > > > Uffe

Hi Uffe,

On Thu, Mar 24, 2022 at 12:16 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Tue, 22 Feb 2022 at 04:40, Jason Lai <jasonlai.genesyslogic@gmail.com> wrote:
> >
> > From: Jason Lai <jason.lai@genesyslogic.com.tw>
> >
> > UHS-II card initialization flow is divided into 2 categories: PHY & Card.
> > Part 1 - PHY Initialization:
> >          Every host controller may need their own avtivation operation to
> >          establish LINK between controller and card. So we add a new member
> >          function(uhs2_detect_init) in struct mmc_host_ops for host
> >          controller use.
> > Part 2 - Card Initialization:
> >          This part can be divided into 6 substeps.
> >          1. Send UHS-II CCMD DEVICE_INIT to card.
> >          2. Send UHS-II CCMD ENUMERATE to card.
> >          3. Send UHS-II Native Read CCMD to obtain capabilities in CFG_REG
> >             of card.
> >          4. Host compares capabilities of host controller and card, then
> >             write the negotiated values to Setting field in CFG_REG of card
> >             through UHS-II Native Write CCMD.
> >          5. Switch host controller's clock to Range B if it is supported by
> >             both host controller and card.
> >          6. Execute legacy SD initialization flow.
> > Part 3 - Provide a function to tranaform legacy SD command packet into
> >          UHS-II SD-TRAN DCMD packet.
> >
> > Most of the code added above came from Intel's original patch[3].
> >
> > [3]
> > https://patchwork.kernel.org/project/linux-mmc/patch/1419672479-30852-2-
> > git-send-email-yi.y.sun@intel.com/
> >
> > Signed-off-by: Jason Lai <jason.lai@genesyslogic.com.tw>
> > ---
> >  drivers/mmc/core/sd_uhs2.c | 835 ++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 817 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/mmc/core/sd_uhs2.c b/drivers/mmc/core/sd_uhs2.c
> > index 800957f74632..f1e8e30301eb 100644
> > --- a/drivers/mmc/core/sd_uhs2.c
> > +++ b/drivers/mmc/core/sd_uhs2.c
> > @@ -3,6 +3,7 @@
> >   * Copyright (C) 2021 Linaro Ltd
> >   *
> >   * Author: Ulf Hansson <ulf.hansson@linaro.org>
> > + * Author: Jason Lai <jason.lai@genesyslogic.com.tw>
> >   *
> >   * Support for SD UHS-II cards
> >   */
> > @@ -10,19 +11,31 @@
> >
> >  #include <linux/mmc/host.h>
> >  #include <linux/mmc/card.h>
> > +#include <linux/mmc/mmc.h>
> > +#include <linux/mmc/sd_uhs2.h>
> >
> >  #include "core.h"
> >  #include "bus.h"
> > +#include "card.h"
> >  #include "sd.h"
> > +#include "sd_ops.h"
> >  #include "mmc_ops.h"
> > +#include "sd_uhs2.h"
> >
> >  static const unsigned int sd_uhs2_freqs[] = { 52000000, 26000000 };
> >
> >  static int sd_uhs2_set_ios(struct mmc_host *host)
> >  {
> >         struct mmc_ios *ios = &host->ios;
> > +       int err = 0;
> >
> > -       return host->ops->uhs2_set_ios(host, ios);
> > +       pr_debug("%s: clock %uHz powermode %u Vdd %u timing %u\n",
> > +                mmc_hostname(host), ios->clock, ios->power_mode, ios->vdd,
> > +                ios->timing);
> > +
> > +       host->ops->set_ios(host, ios);
>
> We discussed using the ->set_ios() callback in a previous version. To
> repeat myself, I don't think it's a good idea. UHS-II needs an
> entirely different power sequence than the legacy interface(s), hence
> I think it's simply cleaner to separate them.
>
> To move forward, I see two options.
> 1) Use only the ->uhs2_host_operation() ops.
> 2) Use a combination of the ->uhs2_set_ios() ops and the
> ->uhs2_host_operation() ops.
>

I referred to the usage of "host->ops->set_ios" in core.c, it is called in
mmc_set_ios() and ".set_ios" is directed to sdhci_set_ios(), which is
located in mmc/host/sdhci.c. So I created sd_uhs2_set_ios() and call
host->ops->uhs2_set_ios() inside it. The ".uhs2_set_ios" is left to host
part to implement it.

> Both options work for me. However, perhaps if you could incorporate
> the changes done on the host driver at next submission, it becomes
> easier for me to understand what makes best sense.
>
> > +
> > +       return err;
> >  }
> >
> >  static int sd_uhs2_power_up(struct mmc_host *host)
> > @@ -45,6 +58,43 @@ static void sd_uhs2_power_off(struct mmc_host *host)
> >         sd_uhs2_set_ios(host);
> >  }
>
> [...]
>
> >
> >  /*
> > @@ -61,6 +119,77 @@ static int sd_uhs2_phy_init(struct mmc_host *host)
> >   */
> >  static int sd_uhs2_dev_init(struct mmc_host *host)
> >  {
> > +       struct mmc_command cmd = {0};
> > +       struct uhs2_command uhs2_cmd = {};
> > +       u32 cnt;
> > +       u32 dap, gap, resp_gap;
> > +       u16 header = 0, arg = 0;
>
> No need to initiate these.
>
> > +       u32 payload[1];
>
> u32?
>

I will change it to __be32.

> > +       u8 plen = 1;
> > +       u8 gd = 0, cf = 1;
> > +       u8 resp[6] = {0};
> > +       u8 resp_len = 6;
>
> Many of these variables are just constant numbers. If it makes sense
> to add definitions for them, then please do that instead. If not, just
> give the value directly in the code.
>
> For example: plen = 1; (I assume that is payload length). This can
> just be given as an in-parameter to sd_uhs2_cmd_assemble(), without
> further explanation.
>
> The point is, sd_uhs2_cmd_assemble() should have a well described
> description of its in-parameters, so no need for further descriptions,
> I think.
>
> This comment applies to all the new code/functions that are added in
> the $subject patch. Please go through all of the code and fix this.
>

OK. I will remove these variables and use constant definitions to replace them.

>
> > +       int err;
> > +
> > +       dap = host->uhs2_caps.dap;
> > +       gap = host->uhs2_caps.gap;
> > +
> > +       header = UHS2_NATIVE_PACKET | UHS2_PACKET_TYPE_CCMD;
> > +       arg = ((UHS2_DEV_CMD_DEVICE_INIT & 0xFF) << 8) |
> > +              UHS2_NATIVE_CMD_WRITE |
> > +              UHS2_NATIVE_CMD_PLEN_4B |
> > +              (UHS2_DEV_CMD_DEVICE_INIT >> 8);
> > +
> > +       /*
> > +        * Refer to UHS-II Addendum Version 1.02 section 6.3.1.
> > +        * Max. time from DEVICE_INIT CCMD EOP reception on Device
> > +        * Rx to its SOP transmission on Device Tx(Tfwd_init_cmd) is
> > +        * 1 second.
> > +        */
> > +       cmd.busy_timeout = 1000;
> > +
> > +       /*
> > +        * Refer to UHS-II Addendum Version 1.02 section 6.2.6.3.
> > +        * When the number of the DEVICE_INIT commands is reach to
> > +        * 30 tiems, Host shall stop issuing DEVICE_INIT command
> > +        * and regard it as an error.
> > +        */
> > +       for (cnt = 0; cnt < 30; cnt++) {
> > +               payload[0] = ((dap & 0xF) << 12) |
> > +                             (cf << 11)         |
> > +                             ((gd & 0xF) << 4)  |
> > +                             (gap & 0xF);
>
> To me, it looks like the payload data deserves to be explained a bit.
> Perhaps you can add a comment explaining what pieces it consists of so
> this becomes more clear?
>

The format of Header and Payload will be listed in include/linux/mmc/sd_uhs2.h

> > +
> > +               sd_uhs2_cmd_assemble(&cmd, &uhs2_cmd, header, arg,  payload, plen, resp, resp_len);
> > +
> > +               err = mmc_wait_for_cmd(host, &cmd, 0);
> > +
> > +               if (err) {
> > +                       pr_err("%s: %s: UHS2 CMD send fail, err= 0x%x!\n",
> > +                              mmc_hostname(host), __func__, err);
> > +                       return -EIO;
>
> Why do you override the original error code that was returned from
> mmc_wait_for_cmd()?
>
> Normally it's preferred to keep the error code, unless there is good
> reason not to.
>
> Again, I won't add more comments like this in the code from the
> $subject patch. But please go through it all to avoid this kind of
> thing.
>

OK.

> > +               }
> > +
> > +               if (resp[3] != (UHS2_DEV_CMD_DEVICE_INIT & 0xFF)) {
> > +                       pr_err("%s: DEVICE_INIT response is wrong!\n",
> > +                              mmc_hostname(host));
> > +                       return -EIO;
> > +               }
> > +
> > +               if (resp[5] & 0x8) {
> > +                       host->uhs2_caps.group_desc = gd;
> > +                       break;
>
> I suggest you do a return 0 here. In this way you can skip the check
> "if (cnt == 30)" below and just return an error code instead.
>

OK.

> > +               }
> > +               resp_gap = resp[4] & 0x0F;
> > +               if (gap == resp_gap)
> > +                       gd++;
> > +       }
> > +       if (cnt == 30) {
> > +               pr_err("%s: DEVICE_INIT fail, already 30 times!\n",
> > +                      mmc_hostname(host));
> > +               return -EIO;
> > +       }
> > +
> >         return 0;
> >  }
> >
>
> >  static int sd_uhs2_config_read(struct mmc_host *host, struct mmc_card *card)
> >  {
> > +       struct mmc_command cmd = {0};
> > +       struct uhs2_command uhs2_cmd = {};
> > +       u16 header = 0, arg = 0;
> > +       u32 cap;
> > +       int err;
> > +
> > +       header = UHS2_NATIVE_PACKET |
> > +                UHS2_PACKET_TYPE_CCMD |
> > +                card->uhs2_config.node_id;
> > +       arg = ((UHS2_DEV_CONFIG_GEN_CAPS & 0xFF) << 8) |
> > +              UHS2_NATIVE_CMD_READ |
> > +              UHS2_NATIVE_CMD_PLEN_4B |
> > +              (UHS2_DEV_CONFIG_GEN_CAPS >> 8);
> > +
> > +       /* There is no payload because per spec, there should be
> > +        * no payload field for read CCMD.
> > +        * Plen is set in arg. Per spec, plen for read CCMD
> > +        * represents the len of read data which is assigned in payload
> > +        * of following RES (p136).
> > +        */
> > +       sd_uhs2_cmd_assemble(&cmd, &uhs2_cmd, header, arg, NULL, 0, NULL, 0);
>
> We are reading the configuration data here and onwards, piece by
> piece. Perhaps if you can add a small comment about each piece we are
> reading, before each call to mmc_wait_for_cmd(), that can help to
> easier understand what goes on.
>

The response read back is the content of UHS-II Register.
I will put comments about the bit fields in that register.

> [...]
>
> >  static int sd_uhs2_config_write(struct mmc_host *host, struct mmc_card *card)
> >  {
> > +       struct mmc_command cmd = {0};
> > +       struct uhs2_command uhs2_cmd = {};
> > +       u16 header = 0, arg = 0;
> > +       u32 payload[2];
> > +       u8 nMinDataGap;
> > +       u8 plen;
> > +       int err;
> > +       u8 resp[5] = {0};
> > +       u8 resp_len = 5;
> > +
> > +       header = UHS2_NATIVE_PACKET |
> > +                UHS2_PACKET_TYPE_CCMD | card->uhs2_config.node_id;
> > +       arg = ((UHS2_DEV_CONFIG_GEN_SET & 0xFF) << 8) |
> > +              UHS2_NATIVE_CMD_WRITE |
> > +              UHS2_NATIVE_CMD_PLEN_8B |
> > +              (UHS2_DEV_CONFIG_GEN_SET >> 8);
> > +
> > +       if (card->uhs2_config.n_lanes == UHS2_DEV_CONFIG_2L_HD_FD &&
> > +           host->uhs2_caps.n_lanes == UHS2_DEV_CONFIG_2L_HD_FD) {
> > +               /* Support HD */
> > +               host->uhs2_caps.flags |= MMC_UHS2_2L_HD;
>
> How is the uhs2_caps.flags field intended to be used? To me it looks
> like a way for the mmc core to exchange status/configuration
> information about the initialization process of the card, with the mmc
> host driver. Perhaps there is more too. Is that correct?
>
> If so, I think it looks quite similar for what we have in the struct
> mmc_ios, for the legacy interface(s). I am not saying we should use
> that, just trying to understand what would suit best here.
>

The usage of uhs2_caps.flags is spread out through core and host.
All operations related to it cannot be integrated into uhs2_set_ios()
simply. I recommend maintaining the status quo.

> > +               nMinDataGap = 1;
> > +       } else {
> > +               /* Only support 2L-FD so far */
> > +               host->uhs2_caps.flags &= ~MMC_UHS2_2L_HD;
> > +               nMinDataGap = 3;
> > +       }
> > +
> > +       /*
> > +        * Most UHS-II cards only support FD and 2L-HD mode. Other lane numbers
> > +        * defined in UHS-II addendem Ver1.01 are optional.
> > +        */
> > +       host->uhs2_caps.n_lanes_set = UHS2_DEV_CONFIG_GEN_SET_2L_FD_HD;
> > +       card->uhs2_config.n_lanes_set = UHS2_DEV_CONFIG_GEN_SET_2L_FD_HD;
>
> [...]
>
> > +static int sd_uhs2_go_dormant(struct mmc_host *host, bool hibernate, u32 node_id)
> > +{
>
> Looks like the in-parameter "hibernate" is superfluous, as it's always
> set to "false" by the caller.
>

The in-parameter "hibernate" is designed according to UHS-II
specification. We did not use
it for now. But we are not sure if it will be set to true in future
use. So I suggest keeping it.

> > +       struct mmc_command cmd = {0};
> > +       struct uhs2_command uhs2_cmd = {};
> > +       u16 header = 0, arg = 0;
> > +       u32 payload[1];
> > +       u8 plen = 1;
> > +       int err;
> > +
> > +       /* Disable Normal INT */
> > +       if (!host->ops->uhs2_host_operation(host, UHS2_DISABLE_INT)) {
> > +               pr_err("%s: %s: UHS2 DISABLE_INT fail!\n",
> > +                      mmc_hostname(host), __func__);
> > +               return -EIO;
> > +       }
> > +
> > +       header = UHS2_NATIVE_PACKET | UHS2_PACKET_TYPE_CCMD | node_id;
> > +
> > +       arg = ((UHS2_DEV_CMD_GO_DORMANT_STATE & 0xFF) << 8) |
> > +               UHS2_NATIVE_CMD_WRITE |
> > +               UHS2_NATIVE_CMD_PLEN_4B |
> > +               (UHS2_DEV_CMD_GO_DORMANT_STATE >> 8);
> > +
> > +       if (hibernate)
> > +               payload[0] = UHS2_DEV_CMD_DORMANT_HIBER;
> > +
> > +       sd_uhs2_cmd_assemble(&cmd, &uhs2_cmd, header, arg, payload, plen, NULL, 0);
> > +
> > +       err = mmc_wait_for_cmd(host, &cmd, 0);
> > +       if (err) {
> > +               pr_err("%s: %s: UHS2 CMD send fail, err= 0x%x!\n",
> > +                      mmc_hostname(host), __func__, err);
> > +               return -EIO;
> > +       }
> > +
> > +       /* Check Dormant State in Present */
> > +       if (!host->ops->uhs2_host_operation(host, UHS2_CHECK_DORMANT)) {
> > +               pr_err("%s: %s: UHS2 GO_DORMANT_STATE fail!\n",
> > +                      mmc_hostname(host), __func__);
> > +               return -EIO;
> > +       }
> > +
> > +       host->ops->uhs2_host_operation(host, UHS2_DISABLE_CLK);
> > +
> >         return 0;
> >  }
> >
> > +static int sd_uhs2_change_speed(struct mmc_host *host, u32 node_id)
> > +{
> > +       struct mmc_command cmd = {0};
> > +       struct uhs2_command uhs2_cmd = {};
> > +       u16 header = 0, arg = 0;
> > +       int err;
> > +       int timeout = 100;
> > +
> > +       /* Change Speed Range at controller side. */
> > +       if (!host->ops->uhs2_host_operation(host, UHS2_SET_SPEED_B)) {
> > +               pr_err("%s: %s: UHS2 SET_SPEED fail!\n",
> > +                      mmc_hostname(host), __func__);
> > +               return -EIO;
> > +       }
> > +
> > +       err = sd_uhs2_go_dormant(host, false, node_id);
> > +       if (err) {
> > +               pr_err("%s: %s: UHS2 GO_DORMANT_STATE fail, err= 0x%x!\n",
> > +                      mmc_hostname(host), __func__, err);
> > +               return -EIO;
> > +       }
> > +
> > +       /* restore sd clock */
> > +       mmc_delay(5);
> > +       host->ops->uhs2_host_operation(host, UHS2_ENABLE_CLK);
>
> I think the code can be a bit better structured here. More precisely,
> since sd_uhs2_go_dormant() is the one that calls
> ->uhs2_host_operation(host, UHS2_DISABLE_INT) and
> ->uhs2_host_operation(host, UHS2_DISABLE_CLK), it's then up to
> sd_uhs2_change_speed() to restore these changes.
>
> To me, it would be more clear if both enabling and disabling of the
> clock /interrupt are managed in sd_uhs2_change_speed().
>

OK.

> > +
> > +       /* Enable Normal INT */
> > +       if (!host->ops->uhs2_host_operation(host, UHS2_ENABLE_INT)) {
> > +               pr_err("%s: %s: UHS2 ENABLE_INT fail!\n",
> > +                      mmc_hostname(host), __func__);
> > +               return -EIO;
> > +       }
> > +
> > +       /*
> > +        * According to UHS-II Addendum Version 1.01, chapter 6.2.3, wait card
> > +        * switch to Active State
> > +        */
> > +       header = UHS2_NATIVE_PACKET | UHS2_PACKET_TYPE_CCMD | node_id;
> > +       arg = ((UHS2_DEV_CONFIG_GEN_SET & 0xFF) << 8) |
> > +               UHS2_NATIVE_CMD_READ |
> > +               UHS2_NATIVE_CMD_PLEN_8B |
> > +               (UHS2_DEV_CONFIG_GEN_SET >> 8);
> > +       do {
> > +               sd_uhs2_cmd_assemble(&cmd, &uhs2_cmd, header, arg, NULL, 0, NULL, 0);
> > +               err = mmc_wait_for_cmd(host, &cmd, 0);
> > +               if (err) {
> > +                       pr_err("%s: %s: UHS2 CMD send fail, err= 0x%x!\n",
> > +                              mmc_hostname(host), __func__, err);
> > +                       return -EIO;
> > +               }
> > +
> > +               if (cmd.resp[1] & UHS2_DEV_CONFIG_GEN_SET_CFG_COMPLETE)
> > +                       break;
> > +
> > +               timeout--;
> > +               if (timeout == 0) {
> > +                       pr_err("%s: %s: Not switch to Active in 100 ms\n",
> > +                              mmc_hostname(host), __func__);
> > +                       return -EIO;
> > +               }
> > +
> > +               mmc_delay(1);
> > +       } while (1);
>
> We really want to avoid these kinds of polling loops, for several
> reasons. Please convert into using __mmc_poll_for_busy() instead.
>

OK.

> > +
> > +       return 0;
> > +}
> > +
> > +static int sd_uhs2_get_ro(struct mmc_host *host)
> > +{
> > +       int ro;
> > +
> > +       /*
> > +        * Some systems don't feature a write-protect pin and don't need one.
> > +        * E.g. because they only have micro-SD card slot. For those systems
> > +        * assume that the SD card is always read-write.
> > +        */
> > +       if (host->caps2 & MMC_CAP2_NO_WRITE_PROTECT)
> > +               return 0;
> > +
> > +       if (!host->ops->get_ro)
> > +               return -1;
> > +
> > +       ro = host->ops->get_ro(host);
> > +
> > +       return ro;
>
> This can be replaced with mmc_sd_get_ro(). Let's avoid the open coding
> and make that function being shared instead.
>

OK.

> > +}
> > +
> >  /*
> >   * Initialize the UHS-II card through the SD-TRAN transport layer. This enables
> >   * commands/requests to be backwards compatible through the legacy SD protocol.
> > @@ -107,9 +696,127 @@ static int sd_uhs2_config_write(struct mmc_host *host, struct mmc_card *card)
> >   */
> >  static int sd_uhs2_legacy_init(struct mmc_host *host, struct mmc_card *card)
> >  {
> > +       int err;
> > +       u32 cid[4];
> > +       u32 ocr;
> > +       u32 rocr = 0;
> > +       int ro;
> > +
> > +       WARN_ON(!host->claimed);
>
> Drop this, it's an internal function, we should know that the host is
> claimed before calling sd_uhs2_legacy_init().
>

OK.

> > +
> > +       /* Send CMD0 to reset SD card */
> > +       mmc_go_idle(host);
> > +
> > +       /* Send CMD8 to communicate SD interface operation condition */
> > +       err = mmc_send_if_cond(host, host->ocr_avail);
> > +       if (err) {
> > +               pr_err("%s: %s: SEND_IF_COND fail!\n",
> > +                      mmc_hostname(host), __func__);
>
> Please drop these prints for every command/operation that fails. We
> already have trace/debug options for commands/requests.
>
> This applies to all the below code as well (perhaps there are few
> cases not covered by the existing trace/debug support, those may be
> converted to pr_debug().
>

OK.

> > +               return err;
> > +       }
> > +
> > +       /*
> > +        * Probe SD card working voltage.
> > +        */
> > +       err = mmc_send_app_op_cond(host, 0, &ocr);
> > +       if (err) {
> > +               pr_err("%s: %s: SD_SEND_OP_COND fail!\n",
> > +                      mmc_hostname(host), __func__);
> > +               return 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);
>
> If the host has MMC_CAP2_FULL_PWR_CYCLE set, mmc_select_voltage() may
> end up calling mmc_power_cycle(). This is not going to work for
> UHS-II.
>
> Either we need to modify mmc_select_voltage() so it becomes aware that
> it can be called for UHS-II initialization, allowing it to avoid the
> path to mmc_power_cycle() - or simply open code the part from
> mmc_select_voltage() for UHS-II here. I think I prefer the latter.
>

I will implement sd_uhs2_select_voltage() to replace mmc_select_voltage().

> > +
> > +       /*
> > +        * Some cards have zero value of rocr in UHS-II mode. Assign host's
> > +        * ocr value to rocr.
> > +        */
> > +       if (!rocr) {
> > +               if (host->ocr_avail) {
> > +                       rocr = host->ocr_avail;
>
> host->ocr_avail should really be checked in when the host driver calls
> mmc_add_host(). It must not be zero, then we should let mmc_add_host()
> return an error code. I look into this and send a patch for this
> separately.
>
> In other words, you should not need to check it here, but just trust that's set.
>

OK.

> > +               } else {
> > +                       pr_err("%s: %s: there is no valid OCR.\n",
> > +                              mmc_hostname(host), __func__);
> > +                       return -EINVAL;
> > +               }
> > +       }
> > +
> > +       /* Wait SD power on ready */
> > +       ocr = rocr;
> > +       err = mmc_send_app_op_cond(host, ocr, &rocr);
> > +       if (err) {
> > +               pr_err("%s: %s: SD_SEND_OP_COND fail!\n", mmc_hostname(host),
> > +                      __func__);
> > +               return err;
> > +       }
> > +
> > +       err = mmc_send_cid(host, cid);
> > +       if (err) {
> > +               pr_err("%s: %s: SD_SEND_CID fail!\n", mmc_hostname(host),
> > +                      __func__);
> > +               return err;
> > +       }
> > +       memcpy(card->raw_cid, cid, sizeof(card->raw_cid));
> > +
> > +       /*
> > +        * Call the optional HC's init_card function to handle quirks.
> > +        */
> > +       if (host->ops->init_card)
> > +               host->ops->init_card(host, card);
>
> This can be removed, as it's only for the legacy interface, I think.
>

OK.

Kind regards,
Jason


> > +
> > +       /*
> > +        * For native busses:  get card RCA and quit open drain mode.
> > +        */
> > +       err = mmc_send_relative_addr(host, &card->rca);
> > +       if (err) {
> > +               pr_err("%s: %s: SD_SEND_RCA fail!\n", mmc_hostname(host),
> > +                      __func__);
> > +               return err;
> > +       }
> > +
> > +       err = mmc_sd_get_csd(card);
> > +       if (err) {
> > +               pr_err("%s: %s: SD_SEND_CSD fail!\n", mmc_hostname(host),
> > +                      __func__);
> > +               return err;
> > +       }
> > +
> > +       /*
> > +        * Select card, as all following commands rely on that.
> > +        */
> > +       err = mmc_select_card(card);
> > +       if (err) {
> > +               pr_err("%s: %s: SD_SEL_DSEL fail!\n", mmc_hostname(host),
> > +                      __func__);
> > +               return err;
> > +       }
> > +
> > +       /*
> > +        * Check if read-only switch is active.
> > +        */
> > +       ro = sd_uhs2_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);
> > +       }
> > +
> >         return 0;
> >  }
> >
> > +static void sd_uhs2_remove(struct mmc_host *host)
> > +{
> > +       mmc_remove_card(host->card);
> > +       host->card = NULL;
> > +}
> > +
> >  /*
> >   * Allocate the data structure for the mmc_card and run the UHS-II specific
> >   * initialization sequence.
> > @@ -121,16 +828,21 @@ static int sd_uhs2_init_card(struct mmc_host *host)
> >         int err;
> >
> >         err = sd_uhs2_dev_init(host);
> > -       if (err)
> > +       if (err) {
> > +               pr_err("%s: UHS2 DEVICE_INIT fail!\n", mmc_hostname(host));
> >                 return err;
> > +       }
> >
> >         err = sd_uhs2_enum(host, &node_id);
> > -       if (err)
> > +       if (err) {
> > +               pr_err("%s: UHS2 ENUMERATE fail!\n", mmc_hostname(host));
> >                 return err;
> > +       }
> >
> >         card = mmc_alloc_card(host, &sd_type);
> >         if (IS_ERR(card))
> >                 return PTR_ERR(card);
> > +       host->card = card;
> >
> >         card->uhs2_config.node_id = node_id;
> >         card->type = MMC_TYPE_SD;
> > @@ -139,6 +851,16 @@ static int sd_uhs2_init_card(struct mmc_host *host)
> >         if (err)
> >                 goto err;
> >
> > +       /* Change to Speed Range B if it is supported */
> > +       if (host->uhs2_caps.flags & MMC_UHS2_SPEED_B) {
> > +               err = sd_uhs2_change_speed(host, node_id);
> > +               if (err) {
> > +                       pr_err("%s: %s: UHS2 sd_uhs2_change_speed() fail!\n",
> > +                              mmc_hostname(host), __func__);
> > +                       return err;
> > +               }
> > +       }
> > +
> >         err = sd_uhs2_config_write(host, card);
> >         if (err)
> >                 goto err;
> > @@ -147,20 +869,13 @@ static int sd_uhs2_init_card(struct mmc_host *host)
> >         if (err)
> >                 goto err;
> >
> > -       host->card = card;
> >         return 0;
> >
> >  err:
> > -       mmc_remove_card(card);
> > +       sd_uhs2_remove(host);
> >         return err;
> >  }
>
> [...]
>
> Kind regards
> Uffe

On Thu, 7 Apr 2022 at 12:45, Lai Jason <jasonlai.genesyslogic@gmail.com> wrote:
>
> Hi Uffe,
>
> On Thu, Mar 24, 2022 at 12:16 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >
> > On Tue, 22 Feb 2022 at 04:40, Jason Lai <jasonlai.genesyslogic@gmail.com> wrote:
> > >
> > > From: Jason Lai <jason.lai@genesyslogic.com.tw>
> > >
> > > UHS-II card initialization flow is divided into 2 categories: PHY & Card.
> > > Part 1 - PHY Initialization:
> > >          Every host controller may need their own avtivation operation to
> > >          establish LINK between controller and card. So we add a new member
> > >          function(uhs2_detect_init) in struct mmc_host_ops for host
> > >          controller use.
> > > Part 2 - Card Initialization:
> > >          This part can be divided into 6 substeps.
> > >          1. Send UHS-II CCMD DEVICE_INIT to card.
> > >          2. Send UHS-II CCMD ENUMERATE to card.
> > >          3. Send UHS-II Native Read CCMD to obtain capabilities in CFG_REG
> > >             of card.
> > >          4. Host compares capabilities of host controller and card, then
> > >             write the negotiated values to Setting field in CFG_REG of card
> > >             through UHS-II Native Write CCMD.
> > >          5. Switch host controller's clock to Range B if it is supported by
> > >             both host controller and card.
> > >          6. Execute legacy SD initialization flow.
> > > Part 3 - Provide a function to tranaform legacy SD command packet into
> > >          UHS-II SD-TRAN DCMD packet.
> > >
> > > Most of the code added above came from Intel's original patch[3].
> > >
> > > [3]
> > > https://patchwork.kernel.org/project/linux-mmc/patch/1419672479-30852-2-
> > > git-send-email-yi.y.sun@intel.com/
> > >
> > > Signed-off-by: Jason Lai <jason.lai@genesyslogic.com.tw>
> > > ---
> > >  drivers/mmc/core/sd_uhs2.c | 835 ++++++++++++++++++++++++++++++++++++-
> > >  1 file changed, 817 insertions(+), 18 deletions(-)
> > >
> > > diff --git a/drivers/mmc/core/sd_uhs2.c b/drivers/mmc/core/sd_uhs2.c
> > > index 800957f74632..f1e8e30301eb 100644
> > > --- a/drivers/mmc/core/sd_uhs2.c
> > > +++ b/drivers/mmc/core/sd_uhs2.c
> > > @@ -3,6 +3,7 @@
> > >   * Copyright (C) 2021 Linaro Ltd
> > >   *
> > >   * Author: Ulf Hansson <ulf.hansson@linaro.org>
> > > + * Author: Jason Lai <jason.lai@genesyslogic.com.tw>
> > >   *
> > >   * Support for SD UHS-II cards
> > >   */
> > > @@ -10,19 +11,31 @@
> > >
> > >  #include <linux/mmc/host.h>
> > >  #include <linux/mmc/card.h>
> > > +#include <linux/mmc/mmc.h>
> > > +#include <linux/mmc/sd_uhs2.h>
> > >
> > >  #include "core.h"
> > >  #include "bus.h"
> > > +#include "card.h"
> > >  #include "sd.h"
> > > +#include "sd_ops.h"
> > >  #include "mmc_ops.h"
> > > +#include "sd_uhs2.h"
> > >
> > >  static const unsigned int sd_uhs2_freqs[] = { 52000000, 26000000 };
> > >
> > >  static int sd_uhs2_set_ios(struct mmc_host *host)
> > >  {
> > >         struct mmc_ios *ios = &host->ios;
> > > +       int err = 0;
> > >
> > > -       return host->ops->uhs2_set_ios(host, ios);
> > > +       pr_debug("%s: clock %uHz powermode %u Vdd %u timing %u\n",
> > > +                mmc_hostname(host), ios->clock, ios->power_mode, ios->vdd,
> > > +                ios->timing);
> > > +
> > > +       host->ops->set_ios(host, ios);
> >
> > We discussed using the ->set_ios() callback in a previous version. To
> > repeat myself, I don't think it's a good idea. UHS-II needs an
> > entirely different power sequence than the legacy interface(s), hence
> > I think it's simply cleaner to separate them.
> >
> > To move forward, I see two options.
> > 1) Use only the ->uhs2_host_operation() ops.
> > 2) Use a combination of the ->uhs2_set_ios() ops and the
> > ->uhs2_host_operation() ops.
> >
>
> I referred to the usage of "host->ops->set_ios" in core.c, it is called in
> mmc_set_ios() and ".set_ios" is directed to sdhci_set_ios(), which is
> located in mmc/host/sdhci.c. So I created sd_uhs2_set_ios() and call
> host->ops->uhs2_set_ios() inside it. The ".uhs2_set_ios" is left to host
> part to implement it.

I see. In that case, what you are looking for is an sdhci specific
callback, this wouldn't belong as part of the generic mmc host ops.

That said, I still think we need to choose between the two options I
suggested above. Otherwise, I fear that it will turn into a nightmare
for the mmc host drivers to support both UHS-II and the legacy
interface.

In other words, I strongly suggest that we must not call ->set_ios()
to manage the UHS-II interface.

>
> > Both options work for me. However, perhaps if you could incorporate
> > the changes done on the host driver at next submission, it becomes
> > easier for me to understand what makes best sense.
> >
> > > +
> > > +       return err;
> > >  }
> > >

[...]

> > >  static int sd_uhs2_config_write(struct mmc_host *host, struct mmc_card *card)
> > >  {
> > > +       struct mmc_command cmd = {0};
> > > +       struct uhs2_command uhs2_cmd = {};
> > > +       u16 header = 0, arg = 0;
> > > +       u32 payload[2];
> > > +       u8 nMinDataGap;
> > > +       u8 plen;
> > > +       int err;
> > > +       u8 resp[5] = {0};
> > > +       u8 resp_len = 5;
> > > +
> > > +       header = UHS2_NATIVE_PACKET |
> > > +                UHS2_PACKET_TYPE_CCMD | card->uhs2_config.node_id;
> > > +       arg = ((UHS2_DEV_CONFIG_GEN_SET & 0xFF) << 8) |
> > > +              UHS2_NATIVE_CMD_WRITE |
> > > +              UHS2_NATIVE_CMD_PLEN_8B |
> > > +              (UHS2_DEV_CONFIG_GEN_SET >> 8);
> > > +
> > > +       if (card->uhs2_config.n_lanes == UHS2_DEV_CONFIG_2L_HD_FD &&
> > > +           host->uhs2_caps.n_lanes == UHS2_DEV_CONFIG_2L_HD_FD) {
> > > +               /* Support HD */
> > > +               host->uhs2_caps.flags |= MMC_UHS2_2L_HD;
> >
> > How is the uhs2_caps.flags field intended to be used? To me it looks
> > like a way for the mmc core to exchange status/configuration
> > information about the initialization process of the card, with the mmc
> > host driver. Perhaps there is more too. Is that correct?
> >
> > If so, I think it looks quite similar for what we have in the struct
> > mmc_ios, for the legacy interface(s). I am not saying we should use
> > that, just trying to understand what would suit best here.
> >
>
> The usage of uhs2_caps.flags is spread out through core and host.
> All operations related to it cannot be integrated into uhs2_set_ios()
> simply. I recommend maintaining the status quo.

What is puzzling to me, is that the data is stored below uhs2_caps.*
and that it's called "flags". It's not self-explanatory and it's not
consistent with the way we use the ->set_ios() callback, for the
legacy interface.

It looks to me that we should rather add a new variable to the struct
mmc_host and perhaps name it "uhs2_ios", to keep this data. Whether we
need to create a new struct for "uhs2_ios" or if it's better to extend
struct mmc_ios, I am not sure. I guess exploring this by writing the
code would tell us what is best suited.

>
> > > +               nMinDataGap = 1;
> > > +       } else {
> > > +               /* Only support 2L-FD so far */
> > > +               host->uhs2_caps.flags &= ~MMC_UHS2_2L_HD;
> > > +               nMinDataGap = 3;
> > > +       }
> > > +
> > > +       /*
> > > +        * Most UHS-II cards only support FD and 2L-HD mode. Other lane numbers
> > > +        * defined in UHS-II addendem Ver1.01 are optional.
> > > +        */
> > > +       host->uhs2_caps.n_lanes_set = UHS2_DEV_CONFIG_GEN_SET_2L_FD_HD;
> > > +       card->uhs2_config.n_lanes_set = UHS2_DEV_CONFIG_GEN_SET_2L_FD_HD;
> >
> > [...]
> >
> > > +static int sd_uhs2_go_dormant(struct mmc_host *host, bool hibernate, u32 node_id)
> > > +{
> >
> > Looks like the in-parameter "hibernate" is superfluous, as it's always
> > set to "false" by the caller.
> >
>
> The in-parameter "hibernate" is designed according to UHS-II
> specification. We did not use
> it for now. But we are not sure if it will be set to true in future
> use. So I suggest keeping it.

I understand your point, but I don't agree, sorry. We don't want dead
code around in the kernel, so please remove it.

Perhaps what we can do, is to add a comment in sd_uhs2_go_dormant(),
somewhere we default to not use hibernate, we could simply explain
that hibernation is currently not supported.

>
> > > +       struct mmc_command cmd = {0};
> > > +       struct uhs2_command uhs2_cmd = {};
> > > +       u16 header = 0, arg = 0;
> > > +       u32 payload[1];
> > > +       u8 plen = 1;
> > > +       int err;
> > > +
> > > +       /* Disable Normal INT */
> > > +       if (!host->ops->uhs2_host_operation(host, UHS2_DISABLE_INT)) {
> > > +               pr_err("%s: %s: UHS2 DISABLE_INT fail!\n",
> > > +                      mmc_hostname(host), __func__);
> > > +               return -EIO;
> > > +       }
> > > +
> > > +       header = UHS2_NATIVE_PACKET | UHS2_PACKET_TYPE_CCMD | node_id;
> > > +
> > > +       arg = ((UHS2_DEV_CMD_GO_DORMANT_STATE & 0xFF) << 8) |
> > > +               UHS2_NATIVE_CMD_WRITE |
> > > +               UHS2_NATIVE_CMD_PLEN_4B |
> > > +               (UHS2_DEV_CMD_GO_DORMANT_STATE >> 8);
> > > +
> > > +       if (hibernate)
> > > +               payload[0] = UHS2_DEV_CMD_DORMANT_HIBER;
> > > +
> > > +       sd_uhs2_cmd_assemble(&cmd, &uhs2_cmd, header, arg, payload, plen, NULL, 0);
> > > +
> > > +       err = mmc_wait_for_cmd(host, &cmd, 0);
> > > +       if (err) {
> > > +               pr_err("%s: %s: UHS2 CMD send fail, err= 0x%x!\n",
> > > +                      mmc_hostname(host), __func__, err);
> > > +               return -EIO;
> > > +       }
> > > +
> > > +       /* Check Dormant State in Present */
> > > +       if (!host->ops->uhs2_host_operation(host, UHS2_CHECK_DORMANT)) {
> > > +               pr_err("%s: %s: UHS2 GO_DORMANT_STATE fail!\n",
> > > +                      mmc_hostname(host), __func__);
> > > +               return -EIO;
> > > +       }
> > > +
> > > +       host->ops->uhs2_host_operation(host, UHS2_DISABLE_CLK);
> > > +
> > >         return 0;
> > >  }

[...]

Kind regards
Uffe

On Thu, Apr 7, 2022 at 11:01 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Thu, 7 Apr 2022 at 12:45, Lai Jason <jasonlai.genesyslogic@gmail.com> wrote:
> >
> > Hi Uffe,
> >
> > On Thu, Mar 24, 2022 at 12:16 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > >
> > > On Tue, 22 Feb 2022 at 04:40, Jason Lai <jasonlai.genesyslogic@gmail.com> wrote:
> > > >
> > > > From: Jason Lai <jason.lai@genesyslogic.com.tw>
> > > >
> > > > UHS-II card initialization flow is divided into 2 categories: PHY & Card.
> > > > Part 1 - PHY Initialization:
> > > >          Every host controller may need their own avtivation operation to
> > > >          establish LINK between controller and card. So we add a new member
> > > >          function(uhs2_detect_init) in struct mmc_host_ops for host
> > > >          controller use.
> > > > Part 2 - Card Initialization:
> > > >          This part can be divided into 6 substeps.
> > > >          1. Send UHS-II CCMD DEVICE_INIT to card.
> > > >          2. Send UHS-II CCMD ENUMERATE to card.
> > > >          3. Send UHS-II Native Read CCMD to obtain capabilities in CFG_REG
> > > >             of card.
> > > >          4. Host compares capabilities of host controller and card, then
> > > >             write the negotiated values to Setting field in CFG_REG of card
> > > >             through UHS-II Native Write CCMD.
> > > >          5. Switch host controller's clock to Range B if it is supported by
> > > >             both host controller and card.
> > > >          6. Execute legacy SD initialization flow.
> > > > Part 3 - Provide a function to tranaform legacy SD command packet into
> > > >          UHS-II SD-TRAN DCMD packet.
> > > >
> > > > Most of the code added above came from Intel's original patch[3].
> > > >
> > > > [3]
> > > > https://patchwork.kernel.org/project/linux-mmc/patch/1419672479-30852-2-
> > > > git-send-email-yi.y.sun@intel.com/
> > > >
> > > > Signed-off-by: Jason Lai <jason.lai@genesyslogic.com.tw>
> > > > ---
> > > >  drivers/mmc/core/sd_uhs2.c | 835 ++++++++++++++++++++++++++++++++++++-
> > > >  1 file changed, 817 insertions(+), 18 deletions(-)
> > > >
> > > > diff --git a/drivers/mmc/core/sd_uhs2.c b/drivers/mmc/core/sd_uhs2.c
> > > > index 800957f74632..f1e8e30301eb 100644
> > > > --- a/drivers/mmc/core/sd_uhs2.c
> > > > +++ b/drivers/mmc/core/sd_uhs2.c
> > > > @@ -3,6 +3,7 @@
> > > >   * Copyright (C) 2021 Linaro Ltd
> > > >   *
> > > >   * Author: Ulf Hansson <ulf.hansson@linaro.org>
> > > > + * Author: Jason Lai <jason.lai@genesyslogic.com.tw>
> > > >   *
> > > >   * Support for SD UHS-II cards
> > > >   */
> > > > @@ -10,19 +11,31 @@
> > > >
> > > >  #include <linux/mmc/host.h>
> > > >  #include <linux/mmc/card.h>
> > > > +#include <linux/mmc/mmc.h>
> > > > +#include <linux/mmc/sd_uhs2.h>
> > > >
> > > >  #include "core.h"
> > > >  #include "bus.h"
> > > > +#include "card.h"
> > > >  #include "sd.h"
> > > > +#include "sd_ops.h"
> > > >  #include "mmc_ops.h"
> > > > +#include "sd_uhs2.h"
> > > >
> > > >  static const unsigned int sd_uhs2_freqs[] = { 52000000, 26000000 };
> > > >
> > > >  static int sd_uhs2_set_ios(struct mmc_host *host)
> > > >  {
> > > >         struct mmc_ios *ios = &host->ios;
> > > > +       int err = 0;
> > > >
> > > > -       return host->ops->uhs2_set_ios(host, ios);
> > > > +       pr_debug("%s: clock %uHz powermode %u Vdd %u timing %u\n",
> > > > +                mmc_hostname(host), ios->clock, ios->power_mode, ios->vdd,
> > > > +                ios->timing);
> > > > +
> > > > +       host->ops->set_ios(host, ios);
> > >
> > > We discussed using the ->set_ios() callback in a previous version. To
> > > repeat myself, I don't think it's a good idea. UHS-II needs an
> > > entirely different power sequence than the legacy interface(s), hence
> > > I think it's simply cleaner to separate them.
> > >
> > > To move forward, I see two options.
> > > 1) Use only the ->uhs2_host_operation() ops.
> > > 2) Use a combination of the ->uhs2_set_ios() ops and the
> > > ->uhs2_host_operation() ops.
> > >
> >
> > I referred to the usage of "host->ops->set_ios" in core.c, it is called in
> > mmc_set_ios() and ".set_ios" is directed to sdhci_set_ios(), which is
> > located in mmc/host/sdhci.c. So I created sd_uhs2_set_ios() and call
> > host->ops->uhs2_set_ios() inside it. The ".uhs2_set_ios" is left to host
> > part to implement it.
>
> I see. In that case, what you are looking for is an sdhci specific
> callback, this wouldn't belong as part of the generic mmc host ops.
>
> That said, I still think we need to choose between the two options I
> suggested above. Otherwise, I fear that it will turn into a nightmare
> for the mmc host drivers to support both UHS-II and the legacy
> interface.
>
> In other words, I strongly suggest that we must not call ->set_ios()
> to manage the UHS-II interface.
>

I will defer to you. But it's a bit complicated so I need more time to think
about how to organize these operations.  Therefore, I'll submit PATCH V4
before sd_uhs2_set_ios() being implemented.

> >
> > > Both options work for me. However, perhaps if you could incorporate
> > > the changes done on the host driver at next submission, it becomes
> > > easier for me to understand what makes best sense.
> > >
> > > > +
> > > > +       return err;
> > > >  }
> > > >
>
> [...]
>
> > > >  static int sd_uhs2_config_write(struct mmc_host *host, struct mmc_card *card)
> > > >  {
> > > > +       struct mmc_command cmd = {0};
> > > > +       struct uhs2_command uhs2_cmd = {};
> > > > +       u16 header = 0, arg = 0;
> > > > +       u32 payload[2];
> > > > +       u8 nMinDataGap;
> > > > +       u8 plen;
> > > > +       int err;
> > > > +       u8 resp[5] = {0};
> > > > +       u8 resp_len = 5;
> > > > +
> > > > +       header = UHS2_NATIVE_PACKET |
> > > > +                UHS2_PACKET_TYPE_CCMD | card->uhs2_config.node_id;
> > > > +       arg = ((UHS2_DEV_CONFIG_GEN_SET & 0xFF) << 8) |
> > > > +              UHS2_NATIVE_CMD_WRITE |
> > > > +              UHS2_NATIVE_CMD_PLEN_8B |
> > > > +              (UHS2_DEV_CONFIG_GEN_SET >> 8);
> > > > +
> > > > +       if (card->uhs2_config.n_lanes == UHS2_DEV_CONFIG_2L_HD_FD &&
> > > > +           host->uhs2_caps.n_lanes == UHS2_DEV_CONFIG_2L_HD_FD) {
> > > > +               /* Support HD */
> > > > +               host->uhs2_caps.flags |= MMC_UHS2_2L_HD;
> > >
> > > How is the uhs2_caps.flags field intended to be used? To me it looks
> > > like a way for the mmc core to exchange status/configuration
> > > information about the initialization process of the card, with the mmc
> > > host driver. Perhaps there is more too. Is that correct?
> > >
> > > If so, I think it looks quite similar for what we have in the struct
> > > mmc_ios, for the legacy interface(s). I am not saying we should use
> > > that, just trying to understand what would suit best here.
> > >
> >
> > The usage of uhs2_caps.flags is spread out through core and host.
> > All operations related to it cannot be integrated into uhs2_set_ios()
> > simply. I recommend maintaining the status quo.
>
> What is puzzling to me, is that the data is stored below uhs2_caps.*
> and that it's called "flags". It's not self-explanatory and it's not
> consistent with the way we use the ->set_ios() callback, for the
> legacy interface.
>

Sorry,  I did not understand why uhs2_caps.flag is related to set_ios().
The bit flags are used to indicate current status of UHS2 card:
  - bit 0: MMC_UHS2_SUPPORT
             This flag indicates if current sd card  supports UHS2 mode.
  - bit 1: MMC_UHS2_INITIALIZED
             This flag indicates if current uhs2 sd card  had been initialized.
  - bit 2: MMC_UHS2_2L_HD
             This flag indicates the speed mode of current uhs2 sd card is
             2L-HD mode. The host sets DCMD argument[0] bit 6
             according to this flag. That means this flag was checked in
             every MMC request.
  - bit 3: MMC_UHS2_APP_CMD
             This flag indicates if the current SD_TRAN command is an APP
             command. The host sets DCMD argument[0] bit 14  according to
             this flag.  That means this flag was checked in every MMC
             request.
  - bit 4: MMC_UHS2_SPEED_B
             This flag indicates the speed range of current UHS2 sd card. This
             flag is used only during uhs2 sd card initialization.

> It looks to me that we should rather add a new variable to the struct
> mmc_host and perhaps name it "uhs2_ios", to keep this data. Whether we
> need to create a new struct for "uhs2_ios" or if it's better to extend
> struct mmc_ios, I am not sure. I guess exploring this by writing the
> code would tell us what is best suited.
>

I will add a new variable to the struct mmc_host and name it "uhs2_ios".
In UHS2 CCMD/DCMD command packet, DM in TMODE(bit 6 in Argument)
and bit APP(bit 15 in Argument) are set according to MMC_UHS2_2L_HD
and MMC_UHS2_APP_CMD. Hence, I plan to define struct uhs2_ios as:
struct sd_uhs2_ios {
        bool                  is_2L_HD_mode;    /* DM bit in TMODE in UHS2 DCDM
                                                                  *
argument will be set to 1 when
                                                                  *
this field is true. */
        bool                  is_APP_CMD;         /* APP bit in UHS2 CCMD/DCDM
                                                                  *
argument will be set to 1 when
                                                                  *
this field is true. */
        unsigned int     power_delay_ms;    /* waiting for stable power */
};

As for MMC_UHS2_INITIALIZED and MMC_UHS2_SPEED_B, I plan to add a variable
in struct mmc_card and name it "uhs2_state", to keep current UHS2 card states.
struct mmc_card {
        struct mmc_host *host; /* the host this device belongs to */
        [...]
        struct  sd_uhs2_config  uhs2_config;   /* SD UHS-II config */
        u8                                  uhs2_state;     /* SD
UHS-II states */
#define MMC_UHS2_INITIALIZED    BIT(1)
#define MMC_UHS2_SPEED_B        BIT(2)
        [...]
};

Kind regards,
Jason

> >
> > > > +               nMinDataGap = 1;
> > > > +       } else {
> > > > +               /* Only support 2L-FD so far */
> > > > +               host->uhs2_caps.flags &= ~MMC_UHS2_2L_HD;
> > > > +               nMinDataGap = 3;
> > > > +       }
> > > > +
> > > > +       /*
> > > > +        * Most UHS-II cards only support FD and 2L-HD mode. Other lane numbers
> > > > +        * defined in UHS-II addendem Ver1.01 are optional.
> > > > +        */
> > > > +       host->uhs2_caps.n_lanes_set = UHS2_DEV_CONFIG_GEN_SET_2L_FD_HD;
> > > > +       card->uhs2_config.n_lanes_set = UHS2_DEV_CONFIG_GEN_SET_2L_FD_HD;
> > >
> > > [...]
> > >
> > > > +static int sd_uhs2_go_dormant(struct mmc_host *host, bool hibernate, u32 node_id)
> > > > +{
> > >
> > > Looks like the in-parameter "hibernate" is superfluous, as it's always
> > > set to "false" by the caller.
> > >
> >
> > The in-parameter "hibernate" is designed according to UHS-II
> > specification. We did not use
> > it for now. But we are not sure if it will be set to true in future
> > use. So I suggest keeping it.
>
> I understand your point, but I don't agree, sorry. We don't want dead
> code around in the kernel, so please remove it.
>
> Perhaps what we can do, is to add a comment in sd_uhs2_go_dormant(),
> somewhere we default to not use hibernate, we could simply explain
> that hibernation is currently not supported.
>
> >
> > > > +       struct mmc_command cmd = {0};
> > > > +       struct uhs2_command uhs2_cmd = {};
> > > > +       u16 header = 0, arg = 0;
> > > > +       u32 payload[1];
> > > > +       u8 plen = 1;
> > > > +       int err;
> > > > +
> > > > +       /* Disable Normal INT */
> > > > +       if (!host->ops->uhs2_host_operation(host, UHS2_DISABLE_INT)) {
> > > > +               pr_err("%s: %s: UHS2 DISABLE_INT fail!\n",
> > > > +                      mmc_hostname(host), __func__);
> > > > +               return -EIO;
> > > > +       }
> > > > +
> > > > +       header = UHS2_NATIVE_PACKET | UHS2_PACKET_TYPE_CCMD | node_id;
> > > > +
> > > > +       arg = ((UHS2_DEV_CMD_GO_DORMANT_STATE & 0xFF) << 8) |
> > > > +               UHS2_NATIVE_CMD_WRITE |
> > > > +               UHS2_NATIVE_CMD_PLEN_4B |
> > > > +               (UHS2_DEV_CMD_GO_DORMANT_STATE >> 8);
> > > > +
> > > > +       if (hibernate)
> > > > +               payload[0] = UHS2_DEV_CMD_DORMANT_HIBER;
> > > > +
> > > > +       sd_uhs2_cmd_assemble(&cmd, &uhs2_cmd, header, arg, payload, plen, NULL, 0);
> > > > +
> > > > +       err = mmc_wait_for_cmd(host, &cmd, 0);
> > > > +       if (err) {
> > > > +               pr_err("%s: %s: UHS2 CMD send fail, err= 0x%x!\n",
> > > > +                      mmc_hostname(host), __func__, err);
> > > > +               return -EIO;
> > > > +       }
> > > > +
> > > > +       /* Check Dormant State in Present */
> > > > +       if (!host->ops->uhs2_host_operation(host, UHS2_CHECK_DORMANT)) {
> > > > +               pr_err("%s: %s: UHS2 GO_DORMANT_STATE fail!\n",
> > > > +                      mmc_hostname(host), __func__);
> > > > +               return -EIO;
> > > > +       }
> > > > +
> > > > +       host->ops->uhs2_host_operation(host, UHS2_DISABLE_CLK);
> > > > +
> > > >         return 0;
> > > >  }
>
> [...]
>
> Kind regards
> Uffe

On Tue, 12 Apr 2022 at 05:32, Lai Jason <jasonlai.genesyslogic@gmail.com> wrote:
>
> On Thu, Apr 7, 2022 at 11:01 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >
> > On Thu, 7 Apr 2022 at 12:45, Lai Jason <jasonlai.genesyslogic@gmail.com> wrote:
> > >
> > > Hi Uffe,
> > >
> > > On Thu, Mar 24, 2022 at 12:16 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > >
> > > > On Tue, 22 Feb 2022 at 04:40, Jason Lai <jasonlai.genesyslogic@gmail.com> wrote:
> > > > >
> > > > > From: Jason Lai <jason.lai@genesyslogic.com.tw>
> > > > >
> > > > > UHS-II card initialization flow is divided into 2 categories: PHY & Card.
> > > > > Part 1 - PHY Initialization:
> > > > >          Every host controller may need their own avtivation operation to
> > > > >          establish LINK between controller and card. So we add a new member
> > > > >          function(uhs2_detect_init) in struct mmc_host_ops for host
> > > > >          controller use.
> > > > > Part 2 - Card Initialization:
> > > > >          This part can be divided into 6 substeps.
> > > > >          1. Send UHS-II CCMD DEVICE_INIT to card.
> > > > >          2. Send UHS-II CCMD ENUMERATE to card.
> > > > >          3. Send UHS-II Native Read CCMD to obtain capabilities in CFG_REG
> > > > >             of card.
> > > > >          4. Host compares capabilities of host controller and card, then
> > > > >             write the negotiated values to Setting field in CFG_REG of card
> > > > >             through UHS-II Native Write CCMD.
> > > > >          5. Switch host controller's clock to Range B if it is supported by
> > > > >             both host controller and card.
> > > > >          6. Execute legacy SD initialization flow.
> > > > > Part 3 - Provide a function to tranaform legacy SD command packet into
> > > > >          UHS-II SD-TRAN DCMD packet.
> > > > >
> > > > > Most of the code added above came from Intel's original patch[3].
> > > > >
> > > > > [3]
> > > > > https://patchwork.kernel.org/project/linux-mmc/patch/1419672479-30852-2-
> > > > > git-send-email-yi.y.sun@intel.com/
> > > > >
> > > > > Signed-off-by: Jason Lai <jason.lai@genesyslogic.com.tw>
> > > > > ---
> > > > >  drivers/mmc/core/sd_uhs2.c | 835 ++++++++++++++++++++++++++++++++++++-
> > > > >  1 file changed, 817 insertions(+), 18 deletions(-)
> > > > >
> > > > > diff --git a/drivers/mmc/core/sd_uhs2.c b/drivers/mmc/core/sd_uhs2.c
> > > > > index 800957f74632..f1e8e30301eb 100644
> > > > > --- a/drivers/mmc/core/sd_uhs2.c
> > > > > +++ b/drivers/mmc/core/sd_uhs2.c
> > > > > @@ -3,6 +3,7 @@
> > > > >   * Copyright (C) 2021 Linaro Ltd
> > > > >   *
> > > > >   * Author: Ulf Hansson <ulf.hansson@linaro.org>
> > > > > + * Author: Jason Lai <jason.lai@genesyslogic.com.tw>
> > > > >   *
> > > > >   * Support for SD UHS-II cards
> > > > >   */
> > > > > @@ -10,19 +11,31 @@
> > > > >
> > > > >  #include <linux/mmc/host.h>
> > > > >  #include <linux/mmc/card.h>
> > > > > +#include <linux/mmc/mmc.h>
> > > > > +#include <linux/mmc/sd_uhs2.h>
> > > > >
> > > > >  #include "core.h"
> > > > >  #include "bus.h"
> > > > > +#include "card.h"
> > > > >  #include "sd.h"
> > > > > +#include "sd_ops.h"
> > > > >  #include "mmc_ops.h"
> > > > > +#include "sd_uhs2.h"
> > > > >
> > > > >  static const unsigned int sd_uhs2_freqs[] = { 52000000, 26000000 };
> > > > >
> > > > >  static int sd_uhs2_set_ios(struct mmc_host *host)
> > > > >  {
> > > > >         struct mmc_ios *ios = &host->ios;
> > > > > +       int err = 0;
> > > > >
> > > > > -       return host->ops->uhs2_set_ios(host, ios);
> > > > > +       pr_debug("%s: clock %uHz powermode %u Vdd %u timing %u\n",
> > > > > +                mmc_hostname(host), ios->clock, ios->power_mode, ios->vdd,
> > > > > +                ios->timing);
> > > > > +
> > > > > +       host->ops->set_ios(host, ios);
> > > >
> > > > We discussed using the ->set_ios() callback in a previous version. To
> > > > repeat myself, I don't think it's a good idea. UHS-II needs an
> > > > entirely different power sequence than the legacy interface(s), hence
> > > > I think it's simply cleaner to separate them.
> > > >
> > > > To move forward, I see two options.
> > > > 1) Use only the ->uhs2_host_operation() ops.
> > > > 2) Use a combination of the ->uhs2_set_ios() ops and the
> > > > ->uhs2_host_operation() ops.
> > > >
> > >
> > > I referred to the usage of "host->ops->set_ios" in core.c, it is called in
> > > mmc_set_ios() and ".set_ios" is directed to sdhci_set_ios(), which is
> > > located in mmc/host/sdhci.c. So I created sd_uhs2_set_ios() and call
> > > host->ops->uhs2_set_ios() inside it. The ".uhs2_set_ios" is left to host
> > > part to implement it.
> >
> > I see. In that case, what you are looking for is an sdhci specific
> > callback, this wouldn't belong as part of the generic mmc host ops.
> >
> > That said, I still think we need to choose between the two options I
> > suggested above. Otherwise, I fear that it will turn into a nightmare
> > for the mmc host drivers to support both UHS-II and the legacy
> > interface.
> >
> > In other words, I strongly suggest that we must not call ->set_ios()
> > to manage the UHS-II interface.
> >
>
> I will defer to you. But it's a bit complicated so I need more time to think
> about how to organize these operations.  Therefore, I'll submit PATCH V4
> before sd_uhs2_set_ios() being implemented.

Okay. I will do my best to continue to provide guidance and review.

Perhaps you could also have a closer look at the legacy code for
eMMC/SD, to get some inspiration on how to structure the code in a
consistent way.

>
> > >
> > > > Both options work for me. However, perhaps if you could incorporate
> > > > the changes done on the host driver at next submission, it becomes
> > > > easier for me to understand what makes best sense.
> > > >
> > > > > +
> > > > > +       return err;
> > > > >  }
> > > > >
> >
> > [...]
> >
> > > > >  static int sd_uhs2_config_write(struct mmc_host *host, struct mmc_card *card)
> > > > >  {
> > > > > +       struct mmc_command cmd = {0};
> > > > > +       struct uhs2_command uhs2_cmd = {};
> > > > > +       u16 header = 0, arg = 0;
> > > > > +       u32 payload[2];
> > > > > +       u8 nMinDataGap;
> > > > > +       u8 plen;
> > > > > +       int err;
> > > > > +       u8 resp[5] = {0};
> > > > > +       u8 resp_len = 5;
> > > > > +
> > > > > +       header = UHS2_NATIVE_PACKET |
> > > > > +                UHS2_PACKET_TYPE_CCMD | card->uhs2_config.node_id;
> > > > > +       arg = ((UHS2_DEV_CONFIG_GEN_SET & 0xFF) << 8) |
> > > > > +              UHS2_NATIVE_CMD_WRITE |
> > > > > +              UHS2_NATIVE_CMD_PLEN_8B |
> > > > > +              (UHS2_DEV_CONFIG_GEN_SET >> 8);
> > > > > +
> > > > > +       if (card->uhs2_config.n_lanes == UHS2_DEV_CONFIG_2L_HD_FD &&
> > > > > +           host->uhs2_caps.n_lanes == UHS2_DEV_CONFIG_2L_HD_FD) {
> > > > > +               /* Support HD */
> > > > > +               host->uhs2_caps.flags |= MMC_UHS2_2L_HD;
> > > >
> > > > How is the uhs2_caps.flags field intended to be used? To me it looks
> > > > like a way for the mmc core to exchange status/configuration
> > > > information about the initialization process of the card, with the mmc
> > > > host driver. Perhaps there is more too. Is that correct?
> > > >
> > > > If so, I think it looks quite similar for what we have in the struct
> > > > mmc_ios, for the legacy interface(s). I am not saying we should use
> > > > that, just trying to understand what would suit best here.
> > > >
> > >
> > > The usage of uhs2_caps.flags is spread out through core and host.
> > > All operations related to it cannot be integrated into uhs2_set_ios()
> > > simply. I recommend maintaining the status quo.
> >
> > What is puzzling to me, is that the data is stored below uhs2_caps.*
> > and that it's called "flags". It's not self-explanatory and it's not
> > consistent with the way we use the ->set_ios() callback, for the
> > legacy interface.
> >
>
> Sorry,  I did not understand why uhs2_caps.flag is related to set_ios().
> The bit flags are used to indicate current status of UHS2 card:
>   - bit 0: MMC_UHS2_SUPPORT
>              This flag indicates if current sd card  supports UHS2 mode.

By looking at the code, it seems like MMC_UHS2_SUPPORT is redundant,
at least from the mmc core point of view. Everywhere it's used, we use
MMC_UHS2_INITIALIZED too.

Moreover, in patch 2 (mmc: core: Prepare to support SD UHS-II cards) I
added MMC_CAP2_SD_UHS2 as a generic host cap, which is being used in
mmc_attach_sd_uhs2(). This should be sufficient as a generic cap for
UHS2, I think.

>   - bit 1: MMC_UHS2_INITIALIZED
>              This flag indicates if current uhs2 sd card  had been initialized.

In patch 2 (mmc: core: Prepare to support SD UHS-II cards), I added
MMC_TIMING_SD_UHS2. It looks like it tries to serve a similar purpose.

Moreover, I don't see where MMC_UHS2_INITIALIZED is getting set, but I
assume it must be before we try to initialize the card by sending UHS2
specific commands to it, right?

>   - bit 2: MMC_UHS2_2L_HD
>              This flag indicates the speed mode of current uhs2 sd card is
>              2L-HD mode. The host sets DCMD argument[0] bit 6
>              according to this flag. That means this flag was checked in
>              every MMC request.

If we compare how we managed things like this for the legacy
interface, this information is kept in the "ios->timing" variable.

>   - bit 3: MMC_UHS2_APP_CMD
>              This flag indicates if the current SD_TRAN command is an APP
>              command. The host sets DCMD argument[0] bit 14  according to
>              this flag.  That means this flag was checked in every MMC
>              request.

Alright, so it's a specific flag for UHS2 commands.

>   - bit 4: MMC_UHS2_SPEED_B
>              This flag indicates the speed range of current UHS2 sd card. This
>              flag is used only during uhs2 sd card initialization.
>
> > It looks to me that we should rather add a new variable to the struct
> > mmc_host and perhaps name it "uhs2_ios", to keep this data. Whether we
> > need to create a new struct for "uhs2_ios" or if it's better to extend
> > struct mmc_ios, I am not sure. I guess exploring this by writing the
> > code would tell us what is best suited.
> >
>
> I will add a new variable to the struct mmc_host and name it "uhs2_ios".
> In UHS2 CCMD/DCMD command packet, DM in TMODE(bit 6 in Argument)
> and bit APP(bit 15 in Argument) are set according to MMC_UHS2_2L_HD
> and MMC_UHS2_APP_CMD. Hence, I plan to define struct uhs2_ios as:
> struct sd_uhs2_ios {
>         bool                  is_2L_HD_mode;    /* DM bit in TMODE in UHS2 DCDM
>                                                                   *
> argument will be set to 1 when

Seems reasonable.

>                                                                   *
> this field is true. */
>         bool                  is_APP_CMD;         /* APP bit in UHS2 CCMD/DCDM
>                                                                   *
> argument will be set to 1 when
>                                                                   *
> this field is true. */

Sounds like this better belongs in the struct uhs2_command.

>         unsigned int     power_delay_ms;    /* waiting for stable power */
> };
>
> As for MMC_UHS2_INITIALIZED and MMC_UHS2_SPEED_B, I plan to add a variable
> in struct mmc_card and name it "uhs2_state", to keep current UHS2 card states.
> struct mmc_card {
>         struct mmc_host *host; /* the host this device belongs to */
>         [...]
>         struct  sd_uhs2_config  uhs2_config;   /* SD UHS-II config */
>         u8                                  uhs2_state;     /* SD
> UHS-II states */
> #define MMC_UHS2_INITIALIZED    BIT(1)
> #define MMC_UHS2_SPEED_B        BIT(2)
>         [...]

Please don't.

If it's configurations of the card, the data belongs under struct
sd_uhs2_config. If it's configuration of the host, the data typically
belongs in the struct sd_uhs2_ios.

MMC_UHS2_SPEED_B is typically a speed mode for UHS2. For the legacy
interface in the struct mmc_ios, we have an "unsigned char timing"
variable to keep things like these. We need something similar in the
struct sd_uhs2_ios for this, I think. Maybe we should even turn the
bool for "2L_HD_mode" into being part of this "timing" variable.

When it comes to the MMC_UHS2_INITIALIZED, if I understand correctly,
it means that we have managed to power on the phy and are about to
start the communication with the card. We could use a "bool" in the
struct sd_uhs2_ios, to indicate this. Another option may be to use and
set "MMC_TIMING_SD_UHS2" as a default speed mode in the "timing"
variable above. No matter what, it seems like MMC_TIMING_SD_UHS2
doesn't belong in the legacy ios->variable, as I suggested in patch2.

> };
>
> Kind regards,
> Jason
>

[...]

Kind regards
Uffe

On Tue, Apr 12, 2022 at 7:58 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Tue, 12 Apr 2022 at 05:32, Lai Jason <jasonlai.genesyslogic@gmail.com> wrote:
> >
> > On Thu, Apr 7, 2022 at 11:01 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > >
> > > On Thu, 7 Apr 2022 at 12:45, Lai Jason <jasonlai.genesyslogic@gmail.com> wrote:
> > > >
> > > > Hi Uffe,
> > > >
> > > > On Thu, Mar 24, 2022 at 12:16 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > > >
> > > > > On Tue, 22 Feb 2022 at 04:40, Jason Lai <jasonlai.genesyslogic@gmail.com> wrote:
> > > > > >
> > > > > > From: Jason Lai <jason.lai@genesyslogic.com.tw>
> > > > > >
> > > > > > UHS-II card initialization flow is divided into 2 categories: PHY & Card.
> > > > > > Part 1 - PHY Initialization:
> > > > > >          Every host controller may need their own avtivation operation to
> > > > > >          establish LINK between controller and card. So we add a new member
> > > > > >          function(uhs2_detect_init) in struct mmc_host_ops for host
> > > > > >          controller use.
> > > > > > Part 2 - Card Initialization:
> > > > > >          This part can be divided into 6 substeps.
> > > > > >          1. Send UHS-II CCMD DEVICE_INIT to card.
> > > > > >          2. Send UHS-II CCMD ENUMERATE to card.
> > > > > >          3. Send UHS-II Native Read CCMD to obtain capabilities in CFG_REG
> > > > > >             of card.
> > > > > >          4. Host compares capabilities of host controller and card, then
> > > > > >             write the negotiated values to Setting field in CFG_REG of card
> > > > > >             through UHS-II Native Write CCMD.
> > > > > >          5. Switch host controller's clock to Range B if it is supported by
> > > > > >             both host controller and card.
> > > > > >          6. Execute legacy SD initialization flow.
> > > > > > Part 3 - Provide a function to tranaform legacy SD command packet into
> > > > > >          UHS-II SD-TRAN DCMD packet.
> > > > > >
> > > > > > Most of the code added above came from Intel's original patch[3].
> > > > > >
> > > > > > [3]
> > > > > > https://patchwork.kernel.org/project/linux-mmc/patch/1419672479-30852-2-
> > > > > > git-send-email-yi.y.sun@intel.com/
> > > > > >
> > > > > > Signed-off-by: Jason Lai <jason.lai@genesyslogic.com.tw>
> > > > > > ---
> > > > > >  drivers/mmc/core/sd_uhs2.c | 835 ++++++++++++++++++++++++++++++++++++-
> > > > > >  1 file changed, 817 insertions(+), 18 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/mmc/core/sd_uhs2.c b/drivers/mmc/core/sd_uhs2.c
> > > > > > index 800957f74632..f1e8e30301eb 100644
> > > > > > --- a/drivers/mmc/core/sd_uhs2.c
> > > > > > +++ b/drivers/mmc/core/sd_uhs2.c
> > > > > > @@ -3,6 +3,7 @@
> > > > > >   * Copyright (C) 2021 Linaro Ltd
> > > > > >   *
> > > > > >   * Author: Ulf Hansson <ulf.hansson@linaro.org>
> > > > > > + * Author: Jason Lai <jason.lai@genesyslogic.com.tw>
> > > > > >   *
> > > > > >   * Support for SD UHS-II cards
> > > > > >   */
> > > > > > @@ -10,19 +11,31 @@
> > > > > >
> > > > > >  #include <linux/mmc/host.h>
> > > > > >  #include <linux/mmc/card.h>
> > > > > > +#include <linux/mmc/mmc.h>
> > > > > > +#include <linux/mmc/sd_uhs2.h>
> > > > > >
> > > > > >  #include "core.h"
> > > > > >  #include "bus.h"
> > > > > > +#include "card.h"
> > > > > >  #include "sd.h"
> > > > > > +#include "sd_ops.h"
> > > > > >  #include "mmc_ops.h"
> > > > > > +#include "sd_uhs2.h"
> > > > > >
> > > > > >  static const unsigned int sd_uhs2_freqs[] = { 52000000, 26000000 };
> > > > > >
> > > > > >  static int sd_uhs2_set_ios(struct mmc_host *host)
> > > > > >  {
> > > > > >         struct mmc_ios *ios = &host->ios;
> > > > > > +       int err = 0;
> > > > > >
> > > > > > -       return host->ops->uhs2_set_ios(host, ios);
> > > > > > +       pr_debug("%s: clock %uHz powermode %u Vdd %u timing %u\n",
> > > > > > +                mmc_hostname(host), ios->clock, ios->power_mode, ios->vdd,
> > > > > > +                ios->timing);
> > > > > > +
> > > > > > +       host->ops->set_ios(host, ios);
> > > > >
> > > > > We discussed using the ->set_ios() callback in a previous version. To
> > > > > repeat myself, I don't think it's a good idea. UHS-II needs an
> > > > > entirely different power sequence than the legacy interface(s), hence
> > > > > I think it's simply cleaner to separate them.
> > > > >
> > > > > To move forward, I see two options.
> > > > > 1) Use only the ->uhs2_host_operation() ops.
> > > > > 2) Use a combination of the ->uhs2_set_ios() ops and the
> > > > > ->uhs2_host_operation() ops.
> > > > >
> > > >
> > > > I referred to the usage of "host->ops->set_ios" in core.c, it is called in
> > > > mmc_set_ios() and ".set_ios" is directed to sdhci_set_ios(), which is
> > > > located in mmc/host/sdhci.c. So I created sd_uhs2_set_ios() and call
> > > > host->ops->uhs2_set_ios() inside it. The ".uhs2_set_ios" is left to host
> > > > part to implement it.
> > >
> > > I see. In that case, what you are looking for is an sdhci specific
> > > callback, this wouldn't belong as part of the generic mmc host ops.
> > >
> > > That said, I still think we need to choose between the two options I
> > > suggested above. Otherwise, I fear that it will turn into a nightmare
> > > for the mmc host drivers to support both UHS-II and the legacy
> > > interface.
> > >
> > > In other words, I strongly suggest that we must not call ->set_ios()
> > > to manage the UHS-II interface.
> > >
> >
> > I will defer to you. But it's a bit complicated so I need more time to think
> > about how to organize these operations.  Therefore, I'll submit PATCH V4
> > before sd_uhs2_set_ios() being implemented.
>
> Okay. I will do my best to continue to provide guidance and review.
>
> Perhaps you could also have a closer look at the legacy code for
> eMMC/SD, to get some inspiration on how to structure the code in a
> consistent way.
>
> >
> > > >
> > > > > Both options work for me. However, perhaps if you could incorporate
> > > > > the changes done on the host driver at next submission, it becomes
> > > > > easier for me to understand what makes best sense.
> > > > >
> > > > > > +
> > > > > > +       return err;
> > > > > >  }
> > > > > >
> > >
> > > [...]
> > >
> > > > > >  static int sd_uhs2_config_write(struct mmc_host *host, struct mmc_card *card)
> > > > > >  {
> > > > > > +       struct mmc_command cmd = {0};
> > > > > > +       struct uhs2_command uhs2_cmd = {};
> > > > > > +       u16 header = 0, arg = 0;
> > > > > > +       u32 payload[2];
> > > > > > +       u8 nMinDataGap;
> > > > > > +       u8 plen;
> > > > > > +       int err;
> > > > > > +       u8 resp[5] = {0};
> > > > > > +       u8 resp_len = 5;
> > > > > > +
> > > > > > +       header = UHS2_NATIVE_PACKET |
> > > > > > +                UHS2_PACKET_TYPE_CCMD | card->uhs2_config.node_id;
> > > > > > +       arg = ((UHS2_DEV_CONFIG_GEN_SET & 0xFF) << 8) |
> > > > > > +              UHS2_NATIVE_CMD_WRITE |
> > > > > > +              UHS2_NATIVE_CMD_PLEN_8B |
> > > > > > +              (UHS2_DEV_CONFIG_GEN_SET >> 8);
> > > > > > +
> > > > > > +       if (card->uhs2_config.n_lanes == UHS2_DEV_CONFIG_2L_HD_FD &&
> > > > > > +           host->uhs2_caps.n_lanes == UHS2_DEV_CONFIG_2L_HD_FD) {
> > > > > > +               /* Support HD */
> > > > > > +               host->uhs2_caps.flags |= MMC_UHS2_2L_HD;
> > > > >
> > > > > How is the uhs2_caps.flags field intended to be used? To me it looks
> > > > > like a way for the mmc core to exchange status/configuration
> > > > > information about the initialization process of the card, with the mmc
> > > > > host driver. Perhaps there is more too. Is that correct?
> > > > >
> > > > > If so, I think it looks quite similar for what we have in the struct
> > > > > mmc_ios, for the legacy interface(s). I am not saying we should use
> > > > > that, just trying to understand what would suit best here.
> > > > >
> > > >
> > > > The usage of uhs2_caps.flags is spread out through core and host.
> > > > All operations related to it cannot be integrated into uhs2_set_ios()
> > > > simply. I recommend maintaining the status quo.
> > >
> > > What is puzzling to me, is that the data is stored below uhs2_caps.*
> > > and that it's called "flags". It's not self-explanatory and it's not
> > > consistent with the way we use the ->set_ios() callback, for the
> > > legacy interface.
> > >
> >
> > Sorry,  I did not understand why uhs2_caps.flag is related to set_ios().
> > The bit flags are used to indicate current status of UHS2 card:
> >   - bit 0: MMC_UHS2_SUPPORT
> >              This flag indicates if current sd card  supports UHS2 mode.
>
> By looking at the code, it seems like MMC_UHS2_SUPPORT is redundant,
> at least from the mmc core point of view. Everywhere it's used, we use
> MMC_UHS2_INITIALIZED too.
>

Yes. MMC_UHS2_SUPPORT is only useful in patch set: "Add support UHS-II
for GL9755"
(https://patchwork.kernel.org/project/linux-mmc/list/?series=378627&archive=both).

I will remove it in V4.

> Moreover, in patch 2 (mmc: core: Prepare to support SD UHS-II cards) I
> added MMC_CAP2_SD_UHS2 as a generic host cap, which is being used in
> mmc_attach_sd_uhs2(). This should be sufficient as a generic cap for
> UHS2, I think.
>
> >   - bit 1: MMC_UHS2_INITIALIZED
> >              This flag indicates if current uhs2 sd card  had been initialized.
>
> In patch 2 (mmc: core: Prepare to support SD UHS-II cards), I added
> MMC_TIMING_SD_UHS2. It looks like it tries to serve a similar purpose.
>

MMC_TIMING_SD_UHS2 is used to indicate that host controller is using UHS2
timing now. But MMC_UHS2_INITIALIZED=1 means that UHS2 interface
initialization is complete. The meaning of these two flags are different.

> Moreover, I don't see where MMC_UHS2_INITIALIZED is getting set, but I
> assume it must be before we try to initialize the card by sending UHS2
> specific commands to it, right?
>

It is set in sd_uhs2_init_card():
static int sd_uhs2_init_card(struct mmc_host *host, struct mmc_card *oldcard)
{
        struct mmc_card *card;
        u32 node_id;
        int err;
        [...]
        if (card->uhs2_state & MMC_UHS2_SPEED_B) {
                err = sd_uhs2_change_speed(host, node_id);
                if (err)
                       return err;
        }

        card->uhs2_state |= MMC_UHS2_INITIALIZED;

        err = sd_uhs2_legacy_init(host, card);
        if (err)
                goto err;
        [...]

}

> >   - bit 2: MMC_UHS2_2L_HD
> >              This flag indicates the speed mode of current uhs2 sd card is
> >              2L-HD mode. The host sets DCMD argument[0] bit 6
> >              according to this flag. That means this flag was checked in
> >              every MMC request.
>
> If we compare how we managed things like this for the legacy
> interface, this information is kept in the "ios->timing" variable.
>
> >   - bit 3: MMC_UHS2_APP_CMD
> >              This flag indicates if the current SD_TRAN command is an APP
> >              command. The host sets DCMD argument[0] bit 14  according to
> >              this flag.  That means this flag was checked in every MMC
> >              request.
>
> Alright, so it's a specific flag for UHS2 commands.
>
> >   - bit 4: MMC_UHS2_SPEED_B
> >              This flag indicates the speed range of current UHS2 sd card. This
> >              flag is used only during uhs2 sd card initialization.
> >
> > > It looks to me that we should rather add a new variable to the struct
> > > mmc_host and perhaps name it "uhs2_ios", to keep this data. Whether we
> > > need to create a new struct for "uhs2_ios" or if it's better to extend
> > > struct mmc_ios, I am not sure. I guess exploring this by writing the
> > > code would tell us what is best suited.
> > >
> >
> > I will add a new variable to the struct mmc_host and name it "uhs2_ios".
> > In UHS2 CCMD/DCMD command packet, DM in TMODE(bit 6 in Argument)
> > and bit APP(bit 15 in Argument) are set according to MMC_UHS2_2L_HD
> > and MMC_UHS2_APP_CMD. Hence, I plan to define struct uhs2_ios as:
> > struct sd_uhs2_ios {
> >         bool                  is_2L_HD_mode;    /* DM bit in TMODE in UHS2 DCDM
> >                                                                   *
> > argument will be set to 1 when
>
> Seems reasonable.
>
> >                                                                   *
> > this field is true. */
> >         bool                  is_APP_CMD;         /* APP bit in UHS2 CCMD/DCDM
> >                                                                   *
> > argument will be set to 1 when
> >                                                                   *
> > this field is true. */
>
> Sounds like this better belongs in the struct uhs2_command.
>

Indeed, it is more reasonable to put is_APP_CMD in uhs2_cmd. But uhs2_cmd
is allocated in mmc_start_request() and is_APP_CMD is set in mmc_app_cmd(),
which is called prior to mmc_start_request(). So I use this flag as a mark and
check it when filling uhs2_cmd in uhs2_prepare_sd_cmd().

> >         unsigned int     power_delay_ms;    /* waiting for stable power */
> > };
> >
> > As for MMC_UHS2_INITIALIZED and MMC_UHS2_SPEED_B, I plan to add a variable
> > in struct mmc_card and name it "uhs2_state", to keep current UHS2 card states.
> > struct mmc_card {
> >         struct mmc_host *host; /* the host this device belongs to */
> >         [...]
> >         struct  sd_uhs2_config  uhs2_config;   /* SD UHS-II config */
> >         u8                                  uhs2_state;     /* SD
> > UHS-II states */
> > #define MMC_UHS2_INITIALIZED    BIT(1)
> > #define MMC_UHS2_SPEED_B        BIT(2)
> >         [...]
>
> Please don't.
>
> If it's configurations of the card, the data belongs under struct
> sd_uhs2_config. If it's configuration of the host, the data typically
> belongs in the struct sd_uhs2_ios.
>
> MMC_UHS2_SPEED_B is typically a speed mode for UHS2. For the legacy
> interface in the struct mmc_ios, we have an "unsigned char timing"
> variable to keep things like these. We need something similar in the
> struct sd_uhs2_ios for this, I think. Maybe we should even turn the
> bool for "2L_HD_mode" into being part of this "timing" variable.
>

Actually, MMC_UHS2_INITIALIZED and MMC_UHS2_SPEED_B are more
like "is UHS2 interface initialization complete?" and "is current UHS2 sd card
support Range B clock?". They are not configurations but a kind of flags which
are used to tell driver:
    - MMC_UHS2_SPEED_B:     Current UHS2 sd card supports Range B clock
                                                   frequency. Driver
can execute sd_uhs2_change_speed()
                                                   after sd_uhs2_config_write().
    - MMC_UHS2_INITIALIZED: UHS2 interface handshake done. Driver can access
                                                   current UHS2 sd
card with UHS2 interface.

That's why I named it 'uhs2_state'. What do you think?

Kind regards,
Jason

> When it comes to the MMC_UHS2_INITIALIZED, if I understand correctly,
> it means that we have managed to power on the phy and are about to
> start the communication with the card. We could use a "bool" in the
> struct sd_uhs2_ios, to indicate this. Another option may be to use and
> set "MMC_TIMING_SD_UHS2" as a default speed mode in the "timing"
> variable above. No matter what, it seems like MMC_TIMING_SD_UHS2
> doesn't belong in the legacy ios->variable, as I suggested in patch2.
>
> > };
> >
> > Kind regards,
> > Jason
> >
>
> [...]
>
> Kind regards
> Uffe

[...]

> > > > > > >  static int sd_uhs2_config_write(struct mmc_host *host, struct mmc_card *card)
> > > > > > >  {
> > > > > > > +       struct mmc_command cmd = {0};
> > > > > > > +       struct uhs2_command uhs2_cmd = {};
> > > > > > > +       u16 header = 0, arg = 0;
> > > > > > > +       u32 payload[2];
> > > > > > > +       u8 nMinDataGap;
> > > > > > > +       u8 plen;
> > > > > > > +       int err;
> > > > > > > +       u8 resp[5] = {0};
> > > > > > > +       u8 resp_len = 5;
> > > > > > > +
> > > > > > > +       header = UHS2_NATIVE_PACKET |
> > > > > > > +                UHS2_PACKET_TYPE_CCMD | card->uhs2_config.node_id;
> > > > > > > +       arg = ((UHS2_DEV_CONFIG_GEN_SET & 0xFF) << 8) |
> > > > > > > +              UHS2_NATIVE_CMD_WRITE |
> > > > > > > +              UHS2_NATIVE_CMD_PLEN_8B |
> > > > > > > +              (UHS2_DEV_CONFIG_GEN_SET >> 8);
> > > > > > > +
> > > > > > > +       if (card->uhs2_config.n_lanes == UHS2_DEV_CONFIG_2L_HD_FD &&
> > > > > > > +           host->uhs2_caps.n_lanes == UHS2_DEV_CONFIG_2L_HD_FD) {
> > > > > > > +               /* Support HD */
> > > > > > > +               host->uhs2_caps.flags |= MMC_UHS2_2L_HD;
> > > > > >
> > > > > > How is the uhs2_caps.flags field intended to be used? To me it looks
> > > > > > like a way for the mmc core to exchange status/configuration
> > > > > > information about the initialization process of the card, with the mmc
> > > > > > host driver. Perhaps there is more too. Is that correct?
> > > > > >
> > > > > > If so, I think it looks quite similar for what we have in the struct
> > > > > > mmc_ios, for the legacy interface(s). I am not saying we should use
> > > > > > that, just trying to understand what would suit best here.
> > > > > >
> > > > >
> > > > > The usage of uhs2_caps.flags is spread out through core and host.
> > > > > All operations related to it cannot be integrated into uhs2_set_ios()
> > > > > simply. I recommend maintaining the status quo.
> > > >
> > > > What is puzzling to me, is that the data is stored below uhs2_caps.*
> > > > and that it's called "flags". It's not self-explanatory and it's not
> > > > consistent with the way we use the ->set_ios() callback, for the
> > > > legacy interface.
> > > >
> > >
> > > Sorry,  I did not understand why uhs2_caps.flag is related to set_ios().
> > > The bit flags are used to indicate current status of UHS2 card:
> > >   - bit 0: MMC_UHS2_SUPPORT
> > >              This flag indicates if current sd card  supports UHS2 mode.
> >
> > By looking at the code, it seems like MMC_UHS2_SUPPORT is redundant,
> > at least from the mmc core point of view. Everywhere it's used, we use
> > MMC_UHS2_INITIALIZED too.
> >
>
> Yes. MMC_UHS2_SUPPORT is only useful in patch set: "Add support UHS-II
> for GL9755"
> (https://patchwork.kernel.org/project/linux-mmc/list/?series=378627&archive=both).
>
> I will remove it in V4.
>
> > Moreover, in patch 2 (mmc: core: Prepare to support SD UHS-II cards) I
> > added MMC_CAP2_SD_UHS2 as a generic host cap, which is being used in
> > mmc_attach_sd_uhs2(). This should be sufficient as a generic cap for
> > UHS2, I think.
> >
> > >   - bit 1: MMC_UHS2_INITIALIZED
> > >              This flag indicates if current uhs2 sd card  had been initialized.
> >
> > In patch 2 (mmc: core: Prepare to support SD UHS-II cards), I added
> > MMC_TIMING_SD_UHS2. It looks like it tries to serve a similar purpose.
> >
>
> MMC_TIMING_SD_UHS2 is used to indicate that host controller is using UHS2
> timing now. But MMC_UHS2_INITIALIZED=1 means that UHS2 interface
> initialization is complete. The meaning of these two flags are different.
>
> > Moreover, I don't see where MMC_UHS2_INITIALIZED is getting set, but I
> > assume it must be before we try to initialize the card by sending UHS2
> > specific commands to it, right?
> >
>
> It is set in sd_uhs2_init_card():
> static int sd_uhs2_init_card(struct mmc_host *host, struct mmc_card *oldcard)
> {
>         struct mmc_card *card;
>         u32 node_id;
>         int err;
>         [...]
>         if (card->uhs2_state & MMC_UHS2_SPEED_B) {
>                 err = sd_uhs2_change_speed(host, node_id);
>                 if (err)
>                        return err;
>         }
>
>         card->uhs2_state |= MMC_UHS2_INITIALIZED;

This is probably a local change present only in your own tree, because
I don't see this anywhere in the v3 series.

In any case, it's not obvious to me why you need to set it exactly at
this point. Can you clarify why?

Perhaps, as I stated before, it's time to post the entire series,
including the host driver changes. In this way, I should be able to
get the complete picture.

>
>         err = sd_uhs2_legacy_init(host, card);
>         if (err)
>                 goto err;
>         [...]
>
> }
>
> > >   - bit 2: MMC_UHS2_2L_HD
> > >              This flag indicates the speed mode of current uhs2 sd card is
> > >              2L-HD mode. The host sets DCMD argument[0] bit 6
> > >              according to this flag. That means this flag was checked in
> > >              every MMC request.
> >
> > If we compare how we managed things like this for the legacy
> > interface, this information is kept in the "ios->timing" variable.
> >
> > >   - bit 3: MMC_UHS2_APP_CMD
> > >              This flag indicates if the current SD_TRAN command is an APP
> > >              command. The host sets DCMD argument[0] bit 14  according to
> > >              this flag.  That means this flag was checked in every MMC
> > >              request.
> >
> > Alright, so it's a specific flag for UHS2 commands.
> >
> > >   - bit 4: MMC_UHS2_SPEED_B
> > >              This flag indicates the speed range of current UHS2 sd card. This
> > >              flag is used only during uhs2 sd card initialization.
> > >
> > > > It looks to me that we should rather add a new variable to the struct
> > > > mmc_host and perhaps name it "uhs2_ios", to keep this data. Whether we
> > > > need to create a new struct for "uhs2_ios" or if it's better to extend
> > > > struct mmc_ios, I am not sure. I guess exploring this by writing the
> > > > code would tell us what is best suited.
> > > >
> > >
> > > I will add a new variable to the struct mmc_host and name it "uhs2_ios".
> > > In UHS2 CCMD/DCMD command packet, DM in TMODE(bit 6 in Argument)
> > > and bit APP(bit 15 in Argument) are set according to MMC_UHS2_2L_HD
> > > and MMC_UHS2_APP_CMD. Hence, I plan to define struct uhs2_ios as:
> > > struct sd_uhs2_ios {
> > >         bool                  is_2L_HD_mode;    /* DM bit in TMODE in UHS2 DCDM
> > >                                                                   *
> > > argument will be set to 1 when
> >
> > Seems reasonable.
> >
> > >                                                                   *
> > > this field is true. */
> > >         bool                  is_APP_CMD;         /* APP bit in UHS2 CCMD/DCDM
> > >                                                                   *
> > > argument will be set to 1 when
> > >                                                                   *
> > > this field is true. */
> >
> > Sounds like this better belongs in the struct uhs2_command.
> >
>
> Indeed, it is more reasonable to put is_APP_CMD in uhs2_cmd. But uhs2_cmd
> is allocated in mmc_start_request() and is_APP_CMD is set in mmc_app_cmd(),
> which is called prior to mmc_start_request(). So I use this flag as a mark and
> check it when filling uhs2_cmd in uhs2_prepare_sd_cmd().

I couldn't find where MMC_UHS2_APP_CMD was getting set, but I assume
you have some local changes that make it set in mmc_app_cmd() then.

I decided to have a closer look at this. Instead of using a flag
(MMC_UHS2_APP_CMD) for this, let's re-work the code so it becomes
possible to prepare the "uhs2_cmd" in mmc_app_cmd() too. That means,
from mmc_start_request() we need to check if the mmc_cmd->uhs2_cmd has
already been assigned and then just leave it as is.

>
> > >         unsigned int     power_delay_ms;    /* waiting for stable power */
> > > };
> > >
> > > As for MMC_UHS2_INITIALIZED and MMC_UHS2_SPEED_B, I plan to add a variable
> > > in struct mmc_card and name it "uhs2_state", to keep current UHS2 card states.
> > > struct mmc_card {
> > >         struct mmc_host *host; /* the host this device belongs to */
> > >         [...]
> > >         struct  sd_uhs2_config  uhs2_config;   /* SD UHS-II config */
> > >         u8                                  uhs2_state;     /* SD
> > > UHS-II states */
> > > #define MMC_UHS2_INITIALIZED    BIT(1)
> > > #define MMC_UHS2_SPEED_B        BIT(2)
> > >         [...]
> >
> > Please don't.
> >
> > If it's configurations of the card, the data belongs under struct
> > sd_uhs2_config. If it's configuration of the host, the data typically
> > belongs in the struct sd_uhs2_ios.
> >
> > MMC_UHS2_SPEED_B is typically a speed mode for UHS2. For the legacy
> > interface in the struct mmc_ios, we have an "unsigned char timing"
> > variable to keep things like these. We need something similar in the
> > struct sd_uhs2_ios for this, I think. Maybe we should even turn the
> > bool for "2L_HD_mode" into being part of this "timing" variable.
> >
>
> Actually, MMC_UHS2_INITIALIZED and MMC_UHS2_SPEED_B are more
> like "is UHS2 interface initialization complete?" and "is current UHS2 sd card
> support Range B clock?". They are not configurations but a kind of flags which
> are used to tell driver:
>     - MMC_UHS2_SPEED_B:     Current UHS2 sd card supports Range B clock
>                                                    frequency. Driver
> can execute sd_uhs2_change_speed()
>                                                    after sd_uhs2_config_write().

I assume you mean that if both the card and the host supports Range B
speed mode for UHS-II, we should try to switch to it.

Then I have two follow up questions related to this.
1) Is there some specific need to inform the host driver, in the
progress of switching to the new speed mode, to make this work? I
would believe so, but I don't have the complete picture.
2) After a successful switch to the new speed mode, we should keep
track of that we are running in this mode. In this way we can report
this in logs/debugfs. How do you intend to do this?

>     - MMC_UHS2_INITIALIZED: UHS2 interface handshake done. Driver can access
>                                                    current UHS2 sd
> card with UHS2 interface.

What does "driver can access UHS2 interface" really mean? Is it that
the card is ready to accept SD-TRAN commands, for example? Can you try
to be a bit more precise, please?

>
> That's why I named it 'uhs2_state'. What do you think?

Let's see where the discussion brings us. Although, "state" is rather
confusing - and it's inconsistent with the ways things are implemented
for the legacy interface.

[...]

Kind regards
Uffe

On Wed, Apr 13, 2022 at 7:04 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> [...]
>
> > > > > > > >  static int sd_uhs2_config_write(struct mmc_host *host, struct mmc_card *card)
> > > > > > > >  {
> > > > > > > > +       struct mmc_command cmd = {0};
> > > > > > > > +       struct uhs2_command uhs2_cmd = {};
> > > > > > > > +       u16 header = 0, arg = 0;
> > > > > > > > +       u32 payload[2];
> > > > > > > > +       u8 nMinDataGap;
> > > > > > > > +       u8 plen;
> > > > > > > > +       int err;
> > > > > > > > +       u8 resp[5] = {0};
> > > > > > > > +       u8 resp_len = 5;
> > > > > > > > +
> > > > > > > > +       header = UHS2_NATIVE_PACKET |
> > > > > > > > +                UHS2_PACKET_TYPE_CCMD | card->uhs2_config.node_id;
> > > > > > > > +       arg = ((UHS2_DEV_CONFIG_GEN_SET & 0xFF) << 8) |
> > > > > > > > +              UHS2_NATIVE_CMD_WRITE |
> > > > > > > > +              UHS2_NATIVE_CMD_PLEN_8B |
> > > > > > > > +              (UHS2_DEV_CONFIG_GEN_SET >> 8);
> > > > > > > > +
> > > > > > > > +       if (card->uhs2_config.n_lanes == UHS2_DEV_CONFIG_2L_HD_FD &&
> > > > > > > > +           host->uhs2_caps.n_lanes == UHS2_DEV_CONFIG_2L_HD_FD) {
> > > > > > > > +               /* Support HD */
> > > > > > > > +               host->uhs2_caps.flags |= MMC_UHS2_2L_HD;
> > > > > > >
> > > > > > > How is the uhs2_caps.flags field intended to be used? To me it looks
> > > > > > > like a way for the mmc core to exchange status/configuration
> > > > > > > information about the initialization process of the card, with the mmc
> > > > > > > host driver. Perhaps there is more too. Is that correct?
> > > > > > >
> > > > > > > If so, I think it looks quite similar for what we have in the struct
> > > > > > > mmc_ios, for the legacy interface(s). I am not saying we should use
> > > > > > > that, just trying to understand what would suit best here.
> > > > > > >
> > > > > >
> > > > > > The usage of uhs2_caps.flags is spread out through core and host.
> > > > > > All operations related to it cannot be integrated into uhs2_set_ios()
> > > > > > simply. I recommend maintaining the status quo.
> > > > >
> > > > > What is puzzling to me, is that the data is stored below uhs2_caps.*
> > > > > and that it's called "flags". It's not self-explanatory and it's not
> > > > > consistent with the way we use the ->set_ios() callback, for the
> > > > > legacy interface.
> > > > >
> > > >
> > > > Sorry,  I did not understand why uhs2_caps.flag is related to set_ios().
> > > > The bit flags are used to indicate current status of UHS2 card:
> > > >   - bit 0: MMC_UHS2_SUPPORT
> > > >              This flag indicates if current sd card  supports UHS2 mode.
> > >
> > > By looking at the code, it seems like MMC_UHS2_SUPPORT is redundant,
> > > at least from the mmc core point of view. Everywhere it's used, we use
> > > MMC_UHS2_INITIALIZED too.
> > >
> >
> > Yes. MMC_UHS2_SUPPORT is only useful in patch set: "Add support UHS-II
> > for GL9755"
> > (https://patchwork.kernel.org/project/linux-mmc/list/?series=378627&archive=both).
> >
> > I will remove it in V4.
> >
> > > Moreover, in patch 2 (mmc: core: Prepare to support SD UHS-II cards) I
> > > added MMC_CAP2_SD_UHS2 as a generic host cap, which is being used in
> > > mmc_attach_sd_uhs2(). This should be sufficient as a generic cap for
> > > UHS2, I think.
> > >
> > > >   - bit 1: MMC_UHS2_INITIALIZED
> > > >              This flag indicates if current uhs2 sd card  had been initialized.
> > >
> > > In patch 2 (mmc: core: Prepare to support SD UHS-II cards), I added
> > > MMC_TIMING_SD_UHS2. It looks like it tries to serve a similar purpose.
> > >
> >
> > MMC_TIMING_SD_UHS2 is used to indicate that host controller is using UHS2
> > timing now. But MMC_UHS2_INITIALIZED=1 means that UHS2 interface
> > initialization is complete. The meaning of these two flags are different.
> >
> > > Moreover, I don't see where MMC_UHS2_INITIALIZED is getting set, but I
> > > assume it must be before we try to initialize the card by sending UHS2
> > > specific commands to it, right?
> > >
> >
> > It is set in sd_uhs2_init_card():
> > static int sd_uhs2_init_card(struct mmc_host *host, struct mmc_card *oldcard)
> > {
> >         struct mmc_card *card;
> >         u32 node_id;
> >         int err;
> >         [...]
> >         if (card->uhs2_state & MMC_UHS2_SPEED_B) {
> >                 err = sd_uhs2_change_speed(host, node_id);
> >                 if (err)
> >                        return err;
> >         }
> >
> >         card->uhs2_state |= MMC_UHS2_INITIALIZED;
>
> This is probably a local change present only in your own tree, because
> I don't see this anywhere in the v3 series.
>

Yes. They are in my V4 candidate.

> In any case, it's not obvious to me why you need to set it exactly at
> this point. Can you clarify why?
>

After changing data rate to Range B, the UHS2 handshake between
host and card is complete. Driver can start to use SD CCMD/DCMD
to control UHS2 SD card. Following listed the usage of UHS2 related
variables:
    MMC_TIMING_SD_UHS2 in host->ios.timing:
        - Enable UHS2 interface on host controller (drivers/mmc/host)
        - Read preset value from Preset Value Register of SD Host
Controller. (drivers/mmc/host)
    MMC_UHS2_SPEED_B in card->uhs2_state:
        - Used to indicate if current sd card supports UHS2 Range B data rate.
    MMC_UHS2_INITIALIZED in card->uhs2_state:
        - Used to indicate if UHS2 PHY handshaking between host
controller and sd card is complete.

> Perhaps, as I stated before, it's time to post the entire series,
> including the host driver changes. In this way, I should be able to
> get the complete picture.
>
> >
> >         err = sd_uhs2_legacy_init(host, card);
> >         if (err)
> >                 goto err;
> >         [...]
> >
> > }
> >
> > > >   - bit 2: MMC_UHS2_2L_HD
> > > >              This flag indicates the speed mode of current uhs2 sd card is
> > > >              2L-HD mode. The host sets DCMD argument[0] bit 6
> > > >              according to this flag. That means this flag was checked in
> > > >              every MMC request.
> > >
> > > If we compare how we managed things like this for the legacy
> > > interface, this information is kept in the "ios->timing" variable.
> > >
> > > >   - bit 3: MMC_UHS2_APP_CMD
> > > >              This flag indicates if the current SD_TRAN command is an APP
> > > >              command. The host sets DCMD argument[0] bit 14  according to
> > > >              this flag.  That means this flag was checked in every MMC
> > > >              request.
> > >
> > > Alright, so it's a specific flag for UHS2 commands.
> > >
> > > >   - bit 4: MMC_UHS2_SPEED_B
> > > >              This flag indicates the speed range of current UHS2 sd card. This
> > > >              flag is used only during uhs2 sd card initialization.
> > > >
> > > > > It looks to me that we should rather add a new variable to the struct
> > > > > mmc_host and perhaps name it "uhs2_ios", to keep this data. Whether we
> > > > > need to create a new struct for "uhs2_ios" or if it's better to extend
> > > > > struct mmc_ios, I am not sure. I guess exploring this by writing the
> > > > > code would tell us what is best suited.
> > > > >
> > > >
> > > > I will add a new variable to the struct mmc_host and name it "uhs2_ios".
> > > > In UHS2 CCMD/DCMD command packet, DM in TMODE(bit 6 in Argument)
> > > > and bit APP(bit 15 in Argument) are set according to MMC_UHS2_2L_HD
> > > > and MMC_UHS2_APP_CMD. Hence, I plan to define struct uhs2_ios as:
> > > > struct sd_uhs2_ios {
> > > >         bool                  is_2L_HD_mode;    /* DM bit in TMODE in UHS2 DCDM
> > > >                                                                   *
> > > > argument will be set to 1 when
> > >
> > > Seems reasonable.
> > >
> > > >                                                                   *
> > > > this field is true. */
> > > >         bool                  is_APP_CMD;         /* APP bit in UHS2 CCMD/DCDM
> > > >                                                                   *
> > > > argument will be set to 1 when
> > > >                                                                   *
> > > > this field is true. */
> > >
> > > Sounds like this better belongs in the struct uhs2_command.
> > >
> >
> > Indeed, it is more reasonable to put is_APP_CMD in uhs2_cmd. But uhs2_cmd
> > is allocated in mmc_start_request() and is_APP_CMD is set in mmc_app_cmd(),
> > which is called prior to mmc_start_request(). So I use this flag as a mark and
> > check it when filling uhs2_cmd in uhs2_prepare_sd_cmd().
>
> I couldn't find where MMC_UHS2_APP_CMD was getting set, but I assume
> you have some local changes that make it set in mmc_app_cmd() then.
>

It should be set here:
int mmc_app_cmd(struct mmc_host *host, struct mmc_card *card)
{
    int err;
    struct mmc_command cmd = {};
    [...]
    if (card->uhs2_state & MMC_UHS2_INITIALIZED) {
        host->uhs2_ios.is_APP_CMD = true;
        /* UHS2 does not support APP command(CMD55). It use APP bit in
UHS2 command argument to indicate APP CMD */
        return 0;
    }
    [...]
}
> I decided to have a closer look at this. Instead of using a flag
> (MMC_UHS2_APP_CMD) for this, let's re-work the code so it becomes
> possible to prepare the "uhs2_cmd" in mmc_app_cmd() too. That means,
> from mmc_start_request() we need to check if the mmc_cmd->uhs2_cmd has
> already been assigned and then just leave it as is.
>

uhs2_cmd is a local variable in mmc_start_request() and mmc_cqe_start_req().

> >
> > > >         unsigned int     power_delay_ms;    /* waiting for stable power */
> > > > };
> > > >
> > > > As for MMC_UHS2_INITIALIZED and MMC_UHS2_SPEED_B, I plan to add a variable
> > > > in struct mmc_card and name it "uhs2_state", to keep current UHS2 card states.
> > > > struct mmc_card {
> > > >         struct mmc_host *host; /* the host this device belongs to */
> > > >         [...]
> > > >         struct  sd_uhs2_config  uhs2_config;   /* SD UHS-II config */
> > > >         u8                                  uhs2_state;     /* SD
> > > > UHS-II states */
> > > > #define MMC_UHS2_INITIALIZED    BIT(1)
> > > > #define MMC_UHS2_SPEED_B        BIT(2)
> > > >         [...]
> > >
> > > Please don't.
> > >
> > > If it's configurations of the card, the data belongs under struct
> > > sd_uhs2_config. If it's configuration of the host, the data typically
> > > belongs in the struct sd_uhs2_ios.
> > >
> > > MMC_UHS2_SPEED_B is typically a speed mode for UHS2. For the legacy
> > > interface in the struct mmc_ios, we have an "unsigned char timing"
> > > variable to keep things like these. We need something similar in the
> > > struct sd_uhs2_ios for this, I think. Maybe we should even turn the
> > > bool for "2L_HD_mode" into being part of this "timing" variable.
> > >
> >
> > Actually, MMC_UHS2_INITIALIZED and MMC_UHS2_SPEED_B are more
> > like "is UHS2 interface initialization complete?" and "is current UHS2 sd card
> > support Range B clock?". They are not configurations but a kind of flags which
> > are used to tell driver:
> >     - MMC_UHS2_SPEED_B:     Current UHS2 sd card supports Range B clock
> >                                                    frequency. Driver
> > can execute sd_uhs2_change_speed()
> >                                                    after sd_uhs2_config_write().
>
> I assume you mean that if both the card and the host supports Range B
> speed mode for UHS-II, we should try to switch to it.
>
> Then I have two follow up questions related to this.
> 1) Is there some specific need to inform the host driver, in the
> progress of switching to the new speed mode, to make this work? I
> would believe so, but I don't have the complete picture.

The driver I mentioned above generally includes core/sd/sd_uhs2/mmc_ops.
If we put all initialization flows in one function, MMC_UHS2_SPEED_B will be
declared as a local variable. I try to use a pseudo code to describe
their usage:

int uhs2_init(struct mmc_host *host)
{
    bool    isRangeB_supported;
    int     err;

    err = phy_init();
    if (err)
        return err;

    err= sd_uhs2_dev_init();
    if (err)
        return err;

    err= sd_uhs2_enum();
    if (err)
        return err;

    /* sd_uhs2_config_read() */
    err = Read_Generic_Capability_Register();
    if (err)
        return err;
    err = Read_PHY_Capability_Register();
    if (err)
        return err;
    err= Read_LINK_TRAN_Capability_Register();
    if (err)
        return err;

    /* sd_uhs2_config_Write() */
    Select_Lane_Number_Fit_to_HostController_and_Card;
    err = Write_Generic_Capability_Register();
    if (err)
        return err;

    if (MMC_Host support Range B data rate)
        isRangeB_supported = true;
    else
        isRangeB_supported = false;
    Select_max_lss_sync_Fit_to_HostController_and_Card;
    Select_max_lss_dir_Fit_to_HostController_and_Card;
    err = Write_PHY_Capability_Register();
    if (err)
        return err;

    Select_NFCU_MaxBlkLen_DataGap_MaxRetry_Fit_to_HostController_and_Card;
    err = Write_LINK_TRAN_Capability_Register();
    if (err)
        return err;

    /* card->uhs2_state bit 2: MMC_UHS2_SPEED_B */
    if (isRangeB_supported) {
        err = sd_uhs2_change_speed();
        if (err)
            return err;
    }

    /*
     * All legacy SD functions will use uhs2_cmd to send SD command if
     * this flag is set. Maybe it's easier to understand if I rename it to
     * MMC_SD_UHS2_MODE_ACTIVE?
     */
    card->uhs2_state |= MMC_UHS2_INITIALIZED;
    return 0;
}

> 2) After a successful switch to the new speed mode, we should keep
> track of that we are running in this mode. In this way we can report
> this in logs/debugfs. How do you intend to do this?
>

The mode will be kept until hw_reset/sd_uhs2_power_off/sd_uhs2_remove
being removed. So we can reach the goal by putting some pr_info() there
and check the mode in system log.

> >     - MMC_UHS2_INITIALIZED: UHS2 interface handshake done. Driver can access
> >                                                    current UHS2 sd
> > card with UHS2 interface.
>
> What does "driver can access UHS2 interface" really mean? Is it that
> the card is ready to accept SD-TRAN commands, for example? Can you try
> to be a bit more precise, please?
>

You are right.
In UHS-II Addendum v1.02, Figure 3-10: UHS-II Transaction by SD-TRAN can
help you learn more about SD-TRAN and "Figure 3-11: UHS-II  Initialization Flow"
shows the initialization sequence of uhs2 interface:

Power On or FULL_RESET --> PHY Initialization --> Device Initialization -->
         Enumeration --> Configuration --> Active

After sd card enters Active state, host can start to execute legacy sd
command via sdhci_uhs2_ops.send_command.
mmc_wait_for_cmd --> mmc_wait_for_req
--> __mmc_start_req --> mmc_start_request -> __mmc_start_request
--> host->ops->request==sdhci_request
--> sdhci_send_command_retry --> sdhci_send_command
--> sdhci_uhs2_ops.send_command==sdhci_uhs2_send_command

> >
> > That's why I named it 'uhs2_state'. What do you think?
>
> Let's see where the discussion brings us. Although, "state" is rather
> confusing - and it's inconsistent with the ways things are implemented
> for the legacy interface.
>

I will submit V4, which does not contain uhs2 host driver, in these 2 days for
your review. As for adding uhs2 host part, you prefer adding in V6 or
create another series of patches?

Kind  regards
Jason

> [...]
>
> Kind regards
> Uffe