| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAK00qKDjRkqRuTqe0nFaEhx3Jx+1gkOnhpm4B7rEh5YsK=8OYg@mail.gmail.com>
Date: Fri, 15 Sep 2023 17:40:36 +0800
From: Victor Shih <victorshihgli@...il.com>
To: Adrian Hunter <adrian.hunter@...el.com>
Cc: ulf.hansson@...aro.org, linux-mmc@...r.kernel.org,
linux-kernel@...r.kernel.org, benchuanggli@...il.com,
HL.Liu@...esyslogic.com.tw, Greg.tu@...esyslogic.com.tw,
takahiro.akashi@...aro.org, dlunev@...omium.org,
Ben Chuang <ben.chuang@...esyslogic.com.tw>,
Victor Shih <victor.shih@...esyslogic.com.tw>
Subject: Re: [PATCH V10 18/23] mmc: sdhci-uhs2: add request() and others
On Tue, Sep 12, 2023 at 8:40 PM Adrian Hunter <adrian.hunter@...el.com> wrote:
>
> On 6/09/23 19:14, Victor Shih wrote:
> > On Thu, Aug 31, 2023 at 7:20 PM Adrian Hunter <adrian.hunter@...el.com> wrote:
> >>
> >> On 31/08/23 13:33, Victor Shih wrote:
> >>> On Thu, Aug 31, 2023 at 4:33 PM Adrian Hunter <adrian.hunter@...el.com> wrote:
> >>>>
> >>>> On 18/08/23 13:02, Victor Shih wrote:
> >>>>> From: Victor Shih <victor.shih@...esyslogic.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@...esyslogic.com.tw>
> >>>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@...aro.org>
> >>>>> Signed-off-by: Victor Shih <victor.shih@...esyslogic.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.
>
Hi, Adrian
I will update this in the V12 version.
Thanks, Victor Shih
Powered by blists - more mailing lists