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

From: Adrian Hunter
Date: Tue Sep 12 2023 - 08:40:03 EST

Next message: Marc Zyngier: "Re: [PATCH v3 2/3] dt-bindings: interrupt-controller: renesas,rzg2l-irqc: Update description for '#interrupt-cells' property"
Previous message: Conor Dooley: "Re: [PATCH 6.1 000/600] 6.1.53-rc1 review"
Next in thread: Victor Shih: "Re: [PATCH V10 18/23] mmc: sdhci-uhs2: add request() and others"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 6/09/23 19:14, Victor Shih wrote:
> On Thu, Aug 31, 2023 at 7:20 PM Adrian Hunter <adrian.hunter@xxxxxxxxx> wrote:
>>
>> On 31/08/23 13:33, Victor Shih wrote:
>>> On Thu, Aug 31, 2023 at 4:33 PM Adrian Hunter <adrian.hunter@xxxxxxxxx> wrote:
>>>>
>>>> On 18/08/23 13:02, Victor Shih wrote:
>>>>> From: Victor Shih <victor.shih@xxxxxxxxxxxxxxxxxxx>
>>>>>
>>>>> 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@xxxxxxxxxxxxxxxxxxx>
>>>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@xxxxxxxxxx>
>>>>> Signed-off-by: Victor Shih <victor.shih@xxxxxxxxxxxxxxxxxxx>
>>>>> ---
>>>>>
>>>>> 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.

Next message: Marc Zyngier: "Re: [PATCH v3 2/3] dt-bindings: interrupt-controller: renesas,rzg2l-irqc: Update description for '#interrupt-cells' property"
Previous message: Conor Dooley: "Re: [PATCH 6.1 000/600] 6.1.53-rc1 review"
Next in thread: Victor Shih: "Re: [PATCH V10 18/23] mmc: sdhci-uhs2: add request() and others"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]