| Message ID | 20240904145256.3670679-1-avri.altman@wdc.com |
|---|---|
| Headers | show |
| Series | Add SDUC Support | expand |
On 9/4/24 15:52, Avri Altman wrote: > For open-ended read/write - just send CMD22 before issuing the command. > While at it, make sure that the rw command arg is properly casting the > lower 32 bits, as it can be larger now. > > Signed-off-by: Avri Altman <avri.altman@wdc.com> > --- > drivers/mmc/core/block.c | 6 +++++- > drivers/mmc/core/core.c | 3 +++ > include/linux/mmc/core.h | 5 +++++ > 3 files changed, 13 insertions(+), 1 deletion(-) > > diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c > index cc7318089cf2..54469261bc25 100644 > --- a/drivers/mmc/core/block.c > +++ b/drivers/mmc/core/block.c > @@ -226,6 +226,7 @@ static void mmc_blk_rw_rq_prep(struct mmc_queue_req *mqrq, > static void mmc_blk_hsq_req_done(struct mmc_request *mrq); > static int mmc_spi_err_check(struct mmc_card *card); > static int mmc_blk_busy_cb(void *cb_data, bool *busy); > +static int mmc_blk_wait_for_idle(struct mmc_queue *mq, struct mmc_host *host); > > static struct mmc_blk_data *mmc_blk_get(struct gendisk *disk) > { > @@ -1710,7 +1711,7 @@ static void mmc_blk_rw_rq_prep(struct mmc_queue_req *mqrq, > > brq->mrq.cmd = &brq->cmd; > > - brq->cmd.arg = blk_rq_pos(req); > + brq->cmd.arg = blk_rq_pos(req) & 0xFFFFFFFF; > if (!mmc_card_blockaddr(card)) > brq->cmd.arg <<= 9; > brq->cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_ADTC; > @@ -1758,6 +1759,9 @@ static void mmc_blk_rw_rq_prep(struct mmc_queue_req *mqrq, > (do_data_tag ? (1 << 29) : 0); > brq->sbc.flags = MMC_RSP_R1 | MMC_CMD_AC; > brq->mrq.sbc = &brq->sbc; > + } else if (mmc_card_ult_capacity(card)) { > + brq->cmd.ext_addr = (blk_rq_pos(req) >> 32) & 0x3F; > + brq->cmd.has_ext_addr = 1; > } > } > > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c > index d6c819dd68ed..a0b2999684b3 100644 > --- a/drivers/mmc/core/core.c > +++ b/drivers/mmc/core/core.c > @@ -336,6 +336,9 @@ int mmc_start_request(struct mmc_host *host, struct mmc_request *mrq) > { > int err; > > + if (mrq->cmd && mrq->cmd->has_ext_addr) > + mmc_send_ext_addr(host, mrq->cmd->ext_addr); We should check that this was actually successful, right? > + > init_completion(&mrq->cmd_completion); > > mmc_retune_hold(host); > diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h > index f0ac2e469b32..41c21c216584 100644 > --- a/include/linux/mmc/core.h > +++ b/include/linux/mmc/core.h > @@ -76,6 +76,11 @@ struct mmc_command { > */ > #define mmc_cmd_type(cmd) ((cmd)->flags & MMC_CMD_MASK) > > + /* for SDUC */ > + u8 has_ext_addr; > + u8 ext_addr; > + u16 reserved; Is there a reason for has_ext_addr being u8? What's the reserved for? > + > unsigned int retries; /* max number of retries */ > int error; /* command error */ >
Thanks for having a look. > > > > + if (mrq->cmd && mrq->cmd->has_ext_addr) > > + mmc_send_ext_addr(host, mrq->cmd->ext_addr); > > We should check that this was actually successful, right? Actually no, as errors in CMD22 are being carried in the subsequent command. > > > + > > init_completion(&mrq->cmd_completion); > > > > mmc_retune_hold(host); > > diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h index > > f0ac2e469b32..41c21c216584 100644 > > --- a/include/linux/mmc/core.h > > +++ b/include/linux/mmc/core.h > > @@ -76,6 +76,11 @@ struct mmc_command { > > */ > > #define mmc_cmd_type(cmd) ((cmd)->flags & MMC_CMD_MASK) > > > > + /* for SDUC */ > > + u8 has_ext_addr; > > + u8 ext_addr; > > + u16 reserved; > > Is there a reason for has_ext_addr being u8? Theoretically a single bit suffices, and since ext_addr uses only 6 bits, I had that bit to spare in ext_addr, but I see no reason to be cheap here - see the reserved u16. > What's the reserved for? Not to break the packed 4bytes alignment of mmc_command. Thanks, Avri > > > + > > unsigned int retries; /* max number of retries */ > > int error; /* command error */ > >
On 5/09/24 09:12, Avri Altman wrote: > Thanks for having a look. > >>> >>> + if (mrq->cmd && mrq->cmd->has_ext_addr) >>> + mmc_send_ext_addr(host, mrq->cmd->ext_addr); >> >> We should check that this was actually successful, right? > Actually no, as errors in CMD22 are being carried in the subsequent command. > >> >>> + >>> init_completion(&mrq->cmd_completion); >>> >>> mmc_retune_hold(host); >>> diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h index >>> f0ac2e469b32..41c21c216584 100644 >>> --- a/include/linux/mmc/core.h >>> +++ b/include/linux/mmc/core.h >>> @@ -76,6 +76,11 @@ struct mmc_command { >>> */ >>> #define mmc_cmd_type(cmd) ((cmd)->flags & MMC_CMD_MASK) >>> >>> + /* for SDUC */ >>> + u8 has_ext_addr; >>> + u8 ext_addr; >>> + u16 reserved; >> >> Is there a reason for has_ext_addr being u8? > Theoretically a single bit suffices, and since ext_addr uses only 6 bits, I had that bit to spare in ext_addr, It could be a bool instead of u8 though. > but I see no reason to be cheap here - see the reserved u16. > >> What's the reserved for? > Not to break the packed 4bytes alignment of mmc_command. Although it isn't "__packed" so compiler should take care of alignment. > > Thanks, > Avri >> >>> + >>> unsigned int retries; /* max number of retries */ >>> int error; /* command error */ >>> >
On 9/5/24 07:12, Avri Altman wrote: > Thanks for having a look. > >>> >>> + if (mrq->cmd && mrq->cmd->has_ext_addr) >>> + mmc_send_ext_addr(host, mrq->cmd->ext_addr); >> >> We should check that this was actually successful, right? > Actually no, as errors in CMD22 are being carried in the subsequent command. I see, sorry for the noise. > >> >>> + >>> init_completion(&mrq->cmd_completion); >>> >>> mmc_retune_hold(host); >>> diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h index >>> f0ac2e469b32..41c21c216584 100644 >>> --- a/include/linux/mmc/core.h >>> +++ b/include/linux/mmc/core.h >>> @@ -76,6 +76,11 @@ struct mmc_command { >>> */ >>> #define mmc_cmd_type(cmd) ((cmd)->flags & MMC_CMD_MASK) >>> >>> + /* for SDUC */ >>> + u8 has_ext_addr; >>> + u8 ext_addr; >>> + u16 reserved; >> >> Is there a reason for has_ext_addr being u8? > Theoretically a single bit suffices, and since ext_addr uses only 6 bits, I had that bit to spare in ext_addr, > but I see no reason to be cheap here - see the reserved u16. > >> What's the reserved for? > Not to break the packed 4bytes alignment of mmc_command. FWIW, that's what it looks like so I was a bit surprised: pahole -C mmc_command vmlinux struct mmc_command { u32 opcode; /* 0 4 */ u32 arg; /* 4 4 */ u32 resp[4]; /* 8 16 */ unsigned int flags; /* 24 4 */ bool has_ext_addr; /* 28 1 */ u8 ext_addr; /* 29 1 */ u16 reserved; /* 30 2 */ unsigned int retries; /* 32 4 */ int error; /* 36 4 */ unsigned int busy_timeout; /* 40 4 */ /* XXX 4 bytes hole, try to pack */ struct mmc_data * data; /* 48 8 */ struct mmc_request * mrq; /* 56 8 */ /* size: 64, cachelines: 1, members: 12 */ /* sum members: 60, holes: 1, sum holes: 4 */ }; has_ext_addr also should be equivalent to: mmc_card_ult_capacity(card) && opcode in [17,18,24,25,32,33] but the flag is also fine. I'm a bit hesitant to put my R-b but it all looks plausible to me FWIW.
> >>> + /* for SDUC */ > >>> + u8 has_ext_addr; > >>> + u8 ext_addr; > >>> + u16 reserved; > >> > >> Is there a reason for has_ext_addr being u8? > > Theoretically a single bit suffices, and since ext_addr uses only 6 > > bits, I had that bit to spare in ext_addr, but I see no reason to be cheap here - > see the reserved u16. > > > >> What's the reserved for? > > Not to break the packed 4bytes alignment of mmc_command. > > FWIW, that's what it looks like so I was a bit surprised: > > pahole -C mmc_command vmlinux > struct mmc_command { > u32 opcode; /* 0 4 */ > u32 arg; /* 4 4 */ > u32 resp[4]; /* 8 16 */ > unsigned int flags; /* 24 4 */ > bool has_ext_addr; /* 28 1 */ > u8 ext_addr; /* 29 1 */ > u16 reserved; /* 30 2 */ > unsigned int retries; /* 32 4 */ > int error; /* 36 4 */ > unsigned int busy_timeout; /* 40 4 */ > > /* XXX 4 bytes hole, try to pack */ > > struct mmc_data * data; /* 48 8 */ > struct mmc_request * mrq; /* 56 8 */ > > /* size: 64, cachelines: 1, members: 12 */ > /* sum members: 60, holes: 1, sum holes: 4 */ }; Moving the sduc members to the end minimizes the padding further: $ pahole -C mmc_command vmlinux struct mmc_command { u32 opcode; /* 0 4 */ u32 arg; /* 4 4 */ u32 resp[4]; /* 8 16 */ unsigned int flags; /* 24 4 */ unsigned int retries; /* 28 4 */ int error; /* 32 4 */ unsigned int busy_timeout; /* 36 4 */ struct mmc_data * data; /* 40 8 */ struct mmc_request * mrq; /* 48 8 */ u8 has_ext_addr; /* 56 1 */ u8 ext_addr; /* 57 1 */ u16 reserved; /* 58 2 */ /* size: 64, cachelines: 1, members: 12 */ /* padding: 4 */ }; I guess I can do that. Thanks, Avri > > has_ext_addr also should be equivalent to: > mmc_card_ult_capacity(card) && opcode in [17,18,24,25,32,33] but the flag is > also fine. > > I'm a bit hesitant to put my R-b but it all looks plausible to me FWIW.
On 4/09/24 17:52, Avri Altman wrote: > Ultra Capacity SD cards (SDUC) was already introduced in SD7.0. Those > cards support capacity larger than 2TB and up to including 128TB. Thus, > the address range of the card expands beyond the 32-bit command > argument. To that end, a new command - CMD22 is defined, to carry the > extra 6-bit upper part of the 38-bit block address that enable access to > 128TB memory space. > > SDUC capacity is agnostic to the interface mode: UHS-I and UHS-II – Same > as SDXC. > > The spec defines several extensions/modifications to the current SDXC > cards, which we address in patches 1 - 10. Otherwise requirements are > out-of-scope of this change. Specifically, CMDQ (CMD44+CMD45), and > Extension for Video Speed Class (CMD20). > > First publication of SDUC was in [1]. This series was developed and > tested separately from [1] and does not borrow from it. > > [1] https://lwn.net/Articles/982566/ > > --- > Changes in v6: > - Remove Ricky's tested-by tag - the series has changed greatly > - Call mmc_send_ext_addr from mmc_start_request (Adrian) > > Changes in v5: > - leave out the mask in mmc_send_ext_addr (Adrian) > - leave out close-ended SDUC support > - remove 500msec write delay as there is no busy indication (Adrian) > - disable mmc-test for SDUC > - move enabling SDUC to the last patch (Adrian) > > Changes in v4: > - Squash patches 1 & 2 (Ulf) > - Amend SD_OCR_2T to SD_OCR_CCS in mmc_sd_get_cid (Ulf) > - Use card state instead of caps2 (Ricky & Ulf) > - Switch patches 5 & 6 (Ulf) > > Changes in v3: > - Some more kernel test robot fixes > - Fix a typo in a commit log (Ricky WU) > - Fix ACMD22 returned value > - Add 'Tested-by' tag for the whole series (Ricky WU) > > Changes in v2: > - Attend kernel test robot warnings > > --- > > Avri Altman (9): > mmc: sd: SDUC Support Recognition > mmc: sd: Add Extension memory addressing > mmc: core: Add open-ended Ext memory addressing > mmc: core: Don't use close-ended rw for SDUC > mmc: core: Allow mmc erase to carry large addresses > mmc: core: Add Ext memory addressing for erase > mmc: core: Adjust ACMD22 to SDUC > mmc: core: Disable SDUC for mmc_test > mmc: core: Enable SDUC > > drivers/mmc/core/block.c | 43 +++++++++++++++++++++++------- > drivers/mmc/core/bus.c | 4 ++- > drivers/mmc/core/card.h | 3 +++ > drivers/mmc/core/core.c | 52 +++++++++++++++++++++++++------------ > drivers/mmc/core/core.h | 16 +++++++++--- > drivers/mmc/core/mmc_test.c | 7 +++++ > drivers/mmc/core/sd.c | 36 ++++++++++++++++--------- > drivers/mmc/core/sd.h | 2 +- > drivers/mmc/core/sd_ops.c | 16 ++++++++++++ > drivers/mmc/core/sd_ops.h | 1 + > drivers/mmc/core/sdio.c | 2 +- > include/linux/mmc/card.h | 2 +- > include/linux/mmc/core.h | 5 ++++ > include/linux/mmc/sd.h | 4 +++ > 14 files changed, 146 insertions(+), 47 deletions(-) > I made a few comments, but this looks good otherwise.