Re: [PATCH V10 18/23] mmc: sdhci-uhs2: add request() and others

[Adrian Hunter <adrian.hunter@intel.com>]

On 6/09/23 19:14, Victor Shih wrote:
> On Thu, Aug 31, 2023 at 7:20 PM Adrian Hunter <adrian.hunter@intel.com> wrote:
>>
>> On 31/08/23 13:33, Victor Shih wrote:
>>> On Thu, Aug 31, 2023 at 4:33 PM Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>>
>>>> On 18/08/23 13:02, Victor Shih wrote:
>>>>> From: Victor Shih <victor.shih@genesyslogic.com.tw>
>>>>>
>>>>> This is a sdhci version of mmc's request operation.
>>>>> It covers both UHS-I and UHS-II.
>>>>>
>>>>> Signed-off-by: Ben Chuang <ben.chuang@genesyslogic.com.tw>
>>>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>>>>> Signed-off-by: Victor Shih <victor.shih@genesyslogic.com.tw>
>>>>> ---
>>>>>
>>>>> Updates in V10:
>>>>>  - Use tmode_half_duplex to instead of uhs2_tmode0_flag
>>>>>    in sdhci_uhs2_set_transfer_mode().
>>>>>
>>>>> Updates in V9:
>>>>>  - Modify the annotations in __sdhci_uhs2_send_command().
>>>>>
>>>>> Updates in V8:
>>>>>  - Adjust the position of matching brackets in
>>>>>    sdhci_uhs2_send_command_retry().
>>>>>  - Modify CameCase definition in __sdhci_uhs2_finish_command().
>>>>>  - Modify error message in __sdhci_uhs2_finish_command().
>>>>>  - sdhci_uhs2_send_command_retry() to instead of sdhci_uhs2_send_command()
>>>>>    in sdhci_uhs2_request().
>>>>>  - Use sdhci_uhs2_mode() to simplify code in sdhci_uhs2_request_atomic().
>>>>>  - Add forward declaration for sdhci_send_command().
>>>>>
>>>>> Updates in V7:
>>>>>  - Cancel export state of some functions.
>>>>>  - Remove unnecessary whitespace changes.
>>>>>
>>>>> Updates in V6:
>>>>>  - Add uhs2_dev_cmd() to simplify code.
>>>>>  - Remove unnecessary functions.
>>>>>  - Cancel export state of some functions.
>>>>>  - Drop use CONFIG_MMC_DEBUG().
>>>>>  - Wrap at 100 columns in some functions.
>>>>>
>>>>> ---
>>>>>
>>>>>  drivers/mmc/host/sdhci-uhs2.c | 412 ++++++++++++++++++++++++++++++++++
>>>>>  drivers/mmc/host/sdhci.c      |  49 ++--
>>>>>  drivers/mmc/host/sdhci.h      |   8 +
>>>>>  3 files changed, 454 insertions(+), 15 deletions(-)
>>>>>
>>>>> diff --git a/drivers/mmc/host/sdhci-uhs2.c b/drivers/mmc/host/sdhci-uhs2.c
>>>>> index 09b86fec9f7b..08fef7174239 100644
>>>>> --- a/drivers/mmc/host/sdhci-uhs2.c
>>>>> +++ b/drivers/mmc/host/sdhci-uhs2.c
>>>>> @@ -14,6 +14,8 @@
>>>>>  #include <linux/module.h>
>>>>>  #include <linux/iopoll.h>
>>>>>  #include <linux/bitfield.h>
>>>>> +#include <linux/mmc/mmc.h>
>>>>> +#include <linux/mmc/host.h>
>>>>>
>>>>>  #include "sdhci.h"
>>>>>  #include "sdhci-uhs2.h"
>>>>> @@ -24,6 +26,8 @@
>>>>>  #define SDHCI_UHS2_DUMP(f, x...) \
>>>>>       pr_err("%s: " DRIVER_NAME ": " f, mmc_hostname(host->mmc), ## x)
>>>>>
>>>>> +#define UHS2_ARG_IOADR_MASK 0xfff
>>>>> +
>>>>>  void sdhci_uhs2_dump_regs(struct sdhci_host *host)
>>>>>  {
>>>>>       if (!(sdhci_uhs2_mode(host)))
>>>>> @@ -58,6 +62,11 @@ EXPORT_SYMBOL_GPL(sdhci_uhs2_dump_regs);
>>>>>   *                                                                           *
>>>>>  \*****************************************************************************/
>>>>>
>>>>> +static inline u16 uhs2_dev_cmd(struct mmc_command *cmd)
>>>>> +{
>>>>> +     return be16_to_cpu((__be16)cmd->uhs2_cmd->arg) & UHS2_ARG_IOADR_MASK;
>>>>> +}
>>>>> +
>>>>>  static inline int mmc_opt_regulator_set_ocr(struct mmc_host *mmc,
>>>>>                                           struct regulator *supply,
>>>>>                                           unsigned short vdd_bit)
>>>>> @@ -446,6 +455,408 @@ static int sdhci_uhs2_control(struct mmc_host *mmc, enum sd_uhs2_operation op)
>>>>>       return err;
>>>>>  }
>>>>>
>>>>> +/*****************************************************************************\
>>>>> + *                                                                           *
>>>>> + * Core functions                                                            *
>>>>> + *                                                                           *
>>>>> +\*****************************************************************************/
>>>>> +
>>>>> +static void sdhci_uhs2_prepare_data(struct sdhci_host *host, struct mmc_command *cmd)
>>>>> +{
>>>>> +     struct mmc_data *data = cmd->data;
>>>>> +
>>>>> +     sdhci_initialize_data(host, data);
>>>>> +
>>>>> +     sdhci_prepare_dma(host, data);
>>>>> +
>>>>> +     sdhci_writew(host, data->blksz, SDHCI_UHS2_BLOCK_SIZE);
>>>>> +     sdhci_writew(host, data->blocks, SDHCI_UHS2_BLOCK_COUNT);
>>>>> +}
>>>>> +
>>>>> +static void sdhci_uhs2_finish_data(struct sdhci_host *host)
>>>>> +{
>>>>> +     struct mmc_data *data = host->data;
>>>>> +
>>>>> +     __sdhci_finish_data_common(host);
>>>>> +
>>>>> +     __sdhci_finish_mrq(host, data->mrq);
>>>>> +}
>>>>> +
>>>>> +static void sdhci_uhs2_set_transfer_mode(struct sdhci_host *host, struct mmc_command *cmd)
>>>>> +{
>>>>> +     u16 mode;
>>>>> +     struct mmc_data *data = cmd->data;
>>>>> +
>>>>> +     if (!data) {
>>>>> +             /* clear Auto CMD settings for no data CMDs */
>>>>> +             if (uhs2_dev_cmd(cmd) == UHS2_DEV_CMD_TRANS_ABORT) {
>>>>> +                     mode =  0;
>>>>> +             } else {
>>>>> +                     mode = sdhci_readw(host, SDHCI_UHS2_TRANS_MODE);
>>>>> +                     if (cmd->opcode == MMC_STOP_TRANSMISSION || cmd->opcode == MMC_ERASE)
>>>>> +                             mode |= SDHCI_UHS2_TRNS_WAIT_EBSY;
>>>>> +                     else
>>>>> +                             /* send status mode */
>>>>> +                             if (cmd->opcode == MMC_SEND_STATUS)
>>>>> +                                     mode = 0;
>>>>> +             }
>>>>> +
>>>>> +             DBG("UHS2 no data trans mode is 0x%x.\n", mode);
>>>>> +
>>>>> +             sdhci_writew(host, mode, SDHCI_UHS2_TRANS_MODE);
>>>>> +             return;
>>>>> +     }
>>>>> +
>>>>> +     WARN_ON(!host->data);
>>>>> +
>>>>> +     mode = SDHCI_UHS2_TRNS_BLK_CNT_EN | SDHCI_UHS2_TRNS_WAIT_EBSY;
>>>>> +     if (data->flags & MMC_DATA_WRITE)
>>>>> +             mode |= SDHCI_UHS2_TRNS_DATA_TRNS_WRT;
>>>>> +
>>>>> +     if (data->blocks == 1 &&
>>>>> +         data->blksz != 512 &&
>>>>> +         cmd->opcode != MMC_READ_SINGLE_BLOCK &&
>>>>> +         cmd->opcode != MMC_WRITE_BLOCK) {
>>>>> +             mode &= ~SDHCI_UHS2_TRNS_BLK_CNT_EN;
>>>>> +             mode |= SDHCI_UHS2_TRNS_BLK_BYTE_MODE;
>>>>> +     }
>>>>> +
>>>>> +     if (host->flags & SDHCI_REQ_USE_DMA)
>>>>> +             mode |= SDHCI_UHS2_TRNS_DMA;
>>>>> +
>>>>> +     if ((mmc_card_uhs2_hd_mode(host->mmc)) && cmd->uhs2_cmd->tmode_half_duplex)
>>>>
>>>> Should not check mmc_card_uhs2_hd_mode(host->mmc).  The mmc core
>>>> must get it right.
>>>>
>>>> Also why is the setting different for different commands?
>>>>
>>>
>>> Hi, Adrian
>>>
>>> I will drop the check  mmc_card_uhs2_hd_mode(host->mmc) in the next version.
>>> But I'm not quite sure what the "why is the setting different for
>>> different commands" means.
>>> Could you help explain it a little bit more clearly?
>>
>> In mmc_uhs2_prepare_cmd() there is this code:
>>
>>         if (cmd->opcode == SD_APP_SEND_SCR || cmd->opcode == SD_APP_SD_STATUS ||
>>             cmd->opcode == MMC_SEND_EXT_CSD || cmd->opcode == SD_SWITCH ||
>>             cmd->opcode == SD_READ_EXTR_SINGLE || cmd->opcode == MMC_SEND_CSD ||
>>             cmd->opcode == MMC_SEND_CID)
>>                 cmd->uhs2_cmd->tmode_half_duplex = 0;
>>         else
>>                 cmd->uhs2_cmd->tmode_half_duplex = mmc_card_uhs2_hd_mode(host);
>>
>> So different commands can have different duplex?  Why is that?
>>
> 
> Hi, Adrian
> 
> Please correct me if I understand wrong.
> We use tmode_half_duplex instead of uhs2_tmode0_flag.
> As I know, the above commands need to be sent in tmode0.
> That's why I set different duplex for different commands.

UHS-II Addendum 7.2.1.2 DCMD says:

 "Host may set DM to 1 for DCMD which supports multi-block read / write regardless of
 data transfer length (e.g., CMD18, CMD25). Otherwise, it shall not set DM to 1.
 (e.g. CMD6, CMD17, CMD24). These rules are also applied to other multi-block read / write
 commands defined in other Part of SD specifications (for example, Host may set DM to 1
 for ACMD18 or ACMD25)."

Which sounds like we should check for CMD18 and CMD25 rather than the other way around?
Perhaps use mmc_op_multi() and add a comment.