lkml.org 
[lkml]   [2023]   [Aug]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH V9 06/23] mmc: core: Support UHS-II card control and access
    On Fri, 21 Jul 2023 at 12:14, Victor Shih <victorshihgli@gmail.com> wrote:
    >
    > From: Victor Shih <victor.shih@genesyslogic.com.tw>
    >
    > Embed UHS-II access/control functionality into the MMC request
    > processing flow.

    This deserves to be extended a bit. There is quite some code being
    added in the $subject patch.

    >
    > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
    > Signed-off-by: Jason Lai <jason.lai@genesyslogic.com.tw>
    > Signed-off-by: Victor Shih <victor.shih@genesyslogic.com.tw>
    > ---
    >
    > Updates in V8:
    > - Add MMC_UHS2_SUPPORT to be cleared in sd_uhs2_detect().
    > - Modify return value in sd_uhs2_attach().
    >
    > Updates in V7:
    > - Add mmc_uhs2_card_prepare_cmd helper function in sd_ops.h.
    > - Drop uhs2_state in favor of ios->timing.
    > - Remove unnecessary functions.
    >
    > ---
    >
    > drivers/mmc/core/block.c | 18 +-
    > drivers/mmc/core/core.c | 8 +
    > drivers/mmc/core/mmc_ops.c | 25 +-
    > drivers/mmc/core/mmc_ops.h | 1 +
    > drivers/mmc/core/sd.c | 13 +-
    > drivers/mmc/core/sd.h | 4 +
    > drivers/mmc/core/sd_ops.c | 11 +
    > drivers/mmc/core/sd_ops.h | 18 +
    > drivers/mmc/core/sd_uhs2.c | 1137 +++++++++++++++++++++++++++++++++++-
    > 9 files changed, 1176 insertions(+), 59 deletions(-)
    >
    > diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
    > index f701efb1fa78..6617ae9fc840 100644
    > --- a/drivers/mmc/core/block.c
    > +++ b/drivers/mmc/core/block.c
    > @@ -918,15 +918,9 @@ static int mmc_sd_num_wr_blocks(struct mmc_card *card, u32 *written_blocks)
    >
    > struct scatterlist sg;
    >
    > - cmd.opcode = MMC_APP_CMD;
    > - cmd.arg = card->rca << 16;
    > - cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_AC;
    > -
    > - err = mmc_wait_for_cmd(card->host, &cmd, 0);
    > - if (err)
    > - return err;
    > - if (!mmc_host_is_spi(card->host) && !(cmd.resp[0] & R1_APP_CMD))
    > - return -EIO;
    > + err = mmc_app_cmd(card->host, card);
    > + if (err)
    > + return err;
    >
    > memset(&cmd, 0, sizeof(struct mmc_command));

    The entire chunk of change above deserves its own separate
    cleanup-patch. If you want to send it separately I can apply
    immediately - or if you decide to make it part of the series then it
    should precede the $subject patch.

    Note that, after the cleanup above, the call to memset() can be dropped too.

    >
    > @@ -1612,6 +1606,9 @@ static void mmc_blk_rw_rq_prep(struct mmc_queue_req *mqrq,

    I commented on the changes in mmc_blk_rw_rq_prep() already in version
    6 [1] - but it seems like you haven't addressed my comments yet.

    I have therefore copied the similar comment again, see below.

    > struct request *req = mmc_queue_req_to_req(mqrq);
    > struct mmc_blk_data *md = mq->blkdata;
    > bool do_rel_wr, do_data_tag;
    > + bool do_multi;
    > +
    > + do_multi = (card->host->flags & MMC_UHS2_SD_TRAN) ? true : false;
    >
    > mmc_blk_data_prep(mq, mqrq, recovery_mode, &do_rel_wr, &do_data_tag);
    >
    > @@ -1622,7 +1619,7 @@ static void mmc_blk_rw_rq_prep(struct mmc_queue_req *mqrq,
    > brq->cmd.arg <<= 9;
    > brq->cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_ADTC;
    >
    > - if (brq->data.blocks > 1 || do_rel_wr) {
    > + if (brq->data.blocks > 1 || do_rel_wr || do_multi) {

    This looks wrong to me. UHS2 can use single block read/writes too. Right?

    > /* SPI multiblock writes terminate using a special
    > * token, not a STOP_TRANSMISSION request.
    > */
    > @@ -1635,6 +1632,7 @@ static void mmc_blk_rw_rq_prep(struct mmc_queue_req *mqrq,
    > brq->mrq.stop = NULL;
    > readcmd = MMC_READ_SINGLE_BLOCK;
    > writecmd = MMC_WRITE_BLOCK;
    > + brq->cmd.uhs2_tmode0_flag = 1;
    > }

    As "do_multi" is always set for UHS2, setting this flag here seems to
    be wrong/redundant.

    Anyway, if I understand correctly, the flag is intended to be used to
    inform the host driver whether the so-called 2L_HD_mode (half-duplex
    or full-duplex) should be used for the I/O request or not. Did I
    understand this correctly?

    To fix the above behaviour, I suggest we try to move the entire
    control of the flag into mmc_uhs2_prepare_cmd(). It seems like we need
    the flag to be set for multi block read/writes (CMD18 and CMD25), but
    only if the host and card supports the 2L_HD_mode too, right?

    According to my earlier suggestions, we should also be able to check
    the 2L_HD_mode via the bits we have set in the ios->timing, no?

    Moreover, by making mmc_uhs2_prepare_cmd() responsible for setting the
    flag, we can move the definition of the flag into the struct
    uhs2_command instead. While at it, I suggest we also rename the flag
    into "tmode_half_duplex", to better describe its purpose. Note that,
    this also means the interpretation of the flag becomes inverted.

    > brq->cmd.opcode = rq_data_dir(req) == READ ? readcmd : writecmd;
    >

    Until we have agreed on how to move forward with the above, I am
    temporarily pausing further review.

    [...]

    Kind regards
    Uffe

    [1]
    https://lore.kernel.org/linux-mmc/CAPDyKFoV3Ch-xzXxiT2RnDeLvsO454Pwq1vQL_bdNLptM+amAg@mail.gmail.com/

    \
     
     \ /
      Last update: 2023-08-08 18:21    [W:9.243 / U:0.052 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site