On 6/09/23 19:14, Victor Shih wrote:
> On Thu, Aug 31, 2023 at 7:20 PM Adrian Hunter <[email protected]> wrote:
>>
>> On 31/08/23 13:33, Victor Shih wrote:
>>> On Thu, Aug 31, 2023 at 4:33 PM Adrian Hunter <[email protected]> wrote:
>>>>
>>>> On 18/08/23 13:02, Victor Shih wrote:
>>>>> From: Victor Shih <[email protected]>
>>>>>
>>>>> This is a sdhci version of mmc's request operation.
>>>>> It covers both UHS-I and UHS-II.
>>>>>
>>>>> Signed-off-by: Ben Chuang <[email protected]>
>>>>> Signed-off-by: AKASHI Takahiro <[email protected]>
>>>>> Signed-off-by: Victor Shih <[email protected]>
>>>>> ---
>>>>>
>>>>> 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.
On Tue, Sep 12, 2023 at 8:40 PM Adrian Hunter <[email protected]> wrote:
>
> On 6/09/23 19:14, Victor Shih wrote:
> > On Thu, Aug 31, 2023 at 7:20 PM Adrian Hunter <[email protected]> wrote:
> >>
> >> On 31/08/23 13:33, Victor Shih wrote:
> >>> On Thu, Aug 31, 2023 at 4:33 PM Adrian Hunter <[email protected]> wrote:
> >>>>
> >>>> On 18/08/23 13:02, Victor Shih wrote:
> >>>>> From: Victor Shih <[email protected]>
> >>>>>
> >>>>> This is a sdhci version of mmc's request operation.
> >>>>> It covers both UHS-I and UHS-II.
> >>>>>
> >>>>> Signed-off-by: Ben Chuang <[email protected]>
> >>>>> Signed-off-by: AKASHI Takahiro <[email protected]>
> >>>>> Signed-off-by: Victor Shih <[email protected]>
> >>>>> ---
> >>>>>
> >>>>> 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.
>
Hi, Adrian
I will update this in the V12 version.
Thanks, Victor Shih