| Message ID | 20240814072934.2559911-2-avri.altman@wdc.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add SDUC Support | expand |
On Wed, 14 Aug 2024 at 09:31, Avri Altman <avri.altman@wdc.com> wrote: > > ACMD41 was extended to support the host-card handshake during > initialization. The card expects that the HCS & HO2T bits to be set in > the command argument, and sets the applicable bits in the R3 returned > response. On the contrary, if a SDUC card is inserted to a > non-supporting host, it will never respond to this ACMD41 until > eventually, the host will timed out and give up. > > Tested-by: Ricky WU <ricky_wu@realtek.com> > Signed-off-by: Avri Altman <avri.altman@wdc.com> > --- > drivers/mmc/core/sd_ops.c | 19 +++++++++++++++---- > include/linux/mmc/host.h | 6 ++++++ > include/linux/mmc/sd.h | 1 + > 3 files changed, 22 insertions(+), 4 deletions(-) > > diff --git a/drivers/mmc/core/sd_ops.c b/drivers/mmc/core/sd_ops.c > index 8b9b34286ef3..7f6963dac873 100644 > --- a/drivers/mmc/core/sd_ops.c > +++ b/drivers/mmc/core/sd_ops.c > @@ -168,12 +168,16 @@ int mmc_send_app_op_cond(struct mmc_host *host, u32 ocr, u32 *rocr) > .cmd = &cmd > }; > int err; > + u32 sduc_arg = SD_OCR_CCS | SD_OCR_2T; > > cmd.opcode = SD_APP_OP_COND; > + cmd.arg = ocr; > + > if (mmc_host_is_spi(host)) > - cmd.arg = ocr & (1 << 30); /* SPI only defines one bit */ > + cmd.arg &= (1 << 30); /* SPI only defines one bit */ > else > - cmd.arg = ocr; > + cmd.arg |= sduc_arg; > + This code doesn't belong in mmc_send_app_op_cond(), but rather in mmc_sd_get_cid(), which is where we add one various OCR bits before we call mmc_send_app_op_cond() with it. For example, if the response of the SD_SEND_IF_COND commands indicates an SD 2.0 compliant card, we tag on the SD_OCR_CCS bit. It looks like that needs to be extended to the SD_OCR_2T bit too. > cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R3 | MMC_CMD_BCR; > > err = __mmc_poll_for_busy(host, SD_APP_OP_COND_PERIOD_US, > @@ -182,8 +186,15 @@ int mmc_send_app_op_cond(struct mmc_host *host, u32 ocr, u32 *rocr) > if (err) > return err; > > - if (rocr && !mmc_host_is_spi(host)) > - *rocr = cmd.resp[0]; > + if (!mmc_host_is_spi(host)) { > + if (rocr) > + *rocr = cmd.resp[0]; > + > + if ((cmd.resp[0] & sduc_arg) == sduc_arg) > + host->caps2 |= MMC_CAP2_SD_SDUC; > + else > + host->caps2 &= ~MMC_CAP2_SD_SDUC; Please don't abuse the host->caps2 for this purpose. Instead let's keep using the card->state to keep track of what type of card this is. You may have a look at how the MMC_CARD_SDXC bit is being used and just follow that behaviour for the SDUC cards too. Moreover, rather than assigning card->state at this point, let's do that from mmc_decode_csd() instead, when we realize that the card supports the new CSD structure version 3. > + } > > return 0; > } > diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h > index 88c6a76042ee..a9c36a3e1a10 100644 > --- a/include/linux/mmc/host.h > +++ b/include/linux/mmc/host.h > @@ -427,6 +427,7 @@ struct mmc_host { > #define MMC_CAP2_CRYPTO 0 > #endif > #define MMC_CAP2_ALT_GPT_TEGRA (1 << 28) /* Host with eMMC that has GPT entry at a non-standard location */ > +#define MMC_CAP2_SD_SDUC (1 << 29) /* SD over 2TB */ > > int fixed_drv_type; /* fixed driver type for non-removable media */ > > @@ -638,6 +639,11 @@ static inline int mmc_card_uhs(struct mmc_card *card) > card->host->ios.timing <= MMC_TIMING_UHS_DDR50; > } > > +static inline int mmc_card_is_sduc(struct mmc_host *host) > +{ > + return host->caps2 & MMC_CAP2_SD_SDUC; > +} > + > void mmc_retune_timer_stop(struct mmc_host *host); > > static inline void mmc_retune_needed(struct mmc_host *host) > diff --git a/include/linux/mmc/sd.h b/include/linux/mmc/sd.h > index 6727576a8755..865cc0ca8543 100644 > --- a/include/linux/mmc/sd.h > +++ b/include/linux/mmc/sd.h > @@ -36,6 +36,7 @@ > /* OCR bit definitions */ > #define SD_OCR_S18R (1 << 24) /* 1.8V switching request */ > #define SD_ROCR_S18A SD_OCR_S18R /* 1.8V switching accepted by card */ > +#define SD_OCR_2T (1 << 27) /* HO2T/CO2T - SDUC support */ > #define SD_OCR_XPC (1 << 28) /* SDXC power control */ > #define SD_OCR_CCS (1 << 30) /* Card Capacity Status */ > > -- > 2.25.1 > Kind regards Uffe
> On Wed, 14 Aug 2024 at 09:31, Avri Altman <avri.altman@wdc.com> wrote: > > > > ACMD41 was extended to support the host-card handshake during > > initialization. The card expects that the HCS & HO2T bits to be set > > in the command argument, and sets the applicable bits in the R3 > > returned response. On the contrary, if a SDUC card is inserted to a > > non-supporting host, it will never respond to this ACMD41 until > > eventually, the host will timed out and give up. > > > > Tested-by: Ricky WU <ricky_wu@realtek.com> > > Signed-off-by: Avri Altman <avri.altman@wdc.com> > > --- > > drivers/mmc/core/sd_ops.c | 19 +++++++++++++++---- > > include/linux/mmc/host.h | 6 ++++++ > > include/linux/mmc/sd.h | 1 + > > 3 files changed, 22 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/mmc/core/sd_ops.c b/drivers/mmc/core/sd_ops.c > > index 8b9b34286ef3..7f6963dac873 100644 > > --- a/drivers/mmc/core/sd_ops.c > > +++ b/drivers/mmc/core/sd_ops.c > > @@ -168,12 +168,16 @@ int mmc_send_app_op_cond(struct mmc_host > *host, u32 ocr, u32 *rocr) > > .cmd = &cmd > > }; > > int err; > > + u32 sduc_arg = SD_OCR_CCS | SD_OCR_2T; > > > > cmd.opcode = SD_APP_OP_COND; > > + cmd.arg = ocr; > > + > > if (mmc_host_is_spi(host)) > > - cmd.arg = ocr & (1 << 30); /* SPI only defines one bit */ > > + cmd.arg &= (1 << 30); /* SPI only defines one bit */ > > else > > - cmd.arg = ocr; > > + cmd.arg |= sduc_arg; > > + > > This code doesn't belong in mmc_send_app_op_cond(), but rather in > mmc_sd_get_cid(), which is where we add one various OCR bits before we call > mmc_send_app_op_cond() with it. > > For example, if the response of the SD_SEND_IF_COND commands indicates > an SD 2.0 compliant card, we tag on the SD_OCR_CCS bit. It looks like that > needs to be extended to the SD_OCR_2T bit too. OK. > > > cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R3 | MMC_CMD_BCR; > > > > err = __mmc_poll_for_busy(host, SD_APP_OP_COND_PERIOD_US, @@ > > -182,8 +186,15 @@ int mmc_send_app_op_cond(struct mmc_host *host, > u32 ocr, u32 *rocr) > > if (err) > > return err; > > > > - if (rocr && !mmc_host_is_spi(host)) > > - *rocr = cmd.resp[0]; > > + if (!mmc_host_is_spi(host)) { > > + if (rocr) > > + *rocr = cmd.resp[0]; > > + > > + if ((cmd.resp[0] & sduc_arg) == sduc_arg) > > + host->caps2 |= MMC_CAP2_SD_SDUC; > > + else > > + host->caps2 &= ~MMC_CAP2_SD_SDUC; > > Please don't abuse the host->caps2 for this purpose. > > Instead let's keep using the card->state to keep track of what type of card this > is. You may have a look at how the MMC_CARD_SDXC bit is being used and > just follow that behaviour for the SDUC cards too. > > Moreover, rather than assigning card->state at this point, let's do that from > mmc_decode_csd() instead, when we realize that the card supports the new > CSD structure version 3. Just to recap - so we are all on the same page: Ricky suggested this in v1 as well. And we had a discussion if we should use the state field to indicate the card type. However, Ricky had some good point why it should be here: "... I think host->caps2 is for host to claim caps, here can just call mmc_card_set_ult_capacity? Don't need to wait csd, because SDXC and SDHC need to identify by capacity, but SDUC can be identified here And all your mmc_card_is_sduc() I think change to mmc_card_ult_capacity() to know the card type ..." This is because according to the spec, SDUC identification is not mandated by its capacity, but rather by the rocr. I will follow your suggestion to squash 1 & 2 and set ult capacity based on the rocr. Thanks, Avri > > > + } > > > > return 0; > > } > > diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h index > > 88c6a76042ee..a9c36a3e1a10 100644 > > --- a/include/linux/mmc/host.h > > +++ b/include/linux/mmc/host.h > > @@ -427,6 +427,7 @@ struct mmc_host { > > #define MMC_CAP2_CRYPTO 0 > > #endif > > #define MMC_CAP2_ALT_GPT_TEGRA (1 << 28) /* Host with eMMC that > has GPT entry at a non-standard location */ > > +#define MMC_CAP2_SD_SDUC (1 << 29) /* SD over 2TB */ > > > > int fixed_drv_type; /* fixed driver type for non-removable > media */ > > > > @@ -638,6 +639,11 @@ static inline int mmc_card_uhs(struct mmc_card > *card) > > card->host->ios.timing <= MMC_TIMING_UHS_DDR50; } > > > > +static inline int mmc_card_is_sduc(struct mmc_host *host) { > > + return host->caps2 & MMC_CAP2_SD_SDUC; } > > + > > void mmc_retune_timer_stop(struct mmc_host *host); > > > > static inline void mmc_retune_needed(struct mmc_host *host) diff > > --git a/include/linux/mmc/sd.h b/include/linux/mmc/sd.h index > > 6727576a8755..865cc0ca8543 100644 > > --- a/include/linux/mmc/sd.h > > +++ b/include/linux/mmc/sd.h > > @@ -36,6 +36,7 @@ > > /* OCR bit definitions */ > > #define SD_OCR_S18R (1 << 24) /* 1.8V switching request */ > > #define SD_ROCR_S18A SD_OCR_S18R /* 1.8V switching accepted by > card */ > > +#define SD_OCR_2T (1 << 27) /* HO2T/CO2T - SDUC support */ > > #define SD_OCR_XPC (1 << 28) /* SDXC power control */ > > #define SD_OCR_CCS (1 << 30) /* Card Capacity Status */ > > > > -- > > 2.25.1 > > > > Kind regards > Uffe
On Thu, 22 Aug 2024 at 15:17, Avri Altman <Avri.Altman@wdc.com> wrote: > > > On Wed, 14 Aug 2024 at 09:31, Avri Altman <avri.altman@wdc.com> wrote: > > > > > > ACMD41 was extended to support the host-card handshake during > > > initialization. The card expects that the HCS & HO2T bits to be set > > > in the command argument, and sets the applicable bits in the R3 > > > returned response. On the contrary, if a SDUC card is inserted to a > > > non-supporting host, it will never respond to this ACMD41 until > > > eventually, the host will timed out and give up. > > > > > > Tested-by: Ricky WU <ricky_wu@realtek.com> > > > Signed-off-by: Avri Altman <avri.altman@wdc.com> > > > --- > > > drivers/mmc/core/sd_ops.c | 19 +++++++++++++++---- > > > include/linux/mmc/host.h | 6 ++++++ > > > include/linux/mmc/sd.h | 1 + > > > 3 files changed, 22 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/mmc/core/sd_ops.c b/drivers/mmc/core/sd_ops.c > > > index 8b9b34286ef3..7f6963dac873 100644 > > > --- a/drivers/mmc/core/sd_ops.c > > > +++ b/drivers/mmc/core/sd_ops.c > > > @@ -168,12 +168,16 @@ int mmc_send_app_op_cond(struct mmc_host > > *host, u32 ocr, u32 *rocr) > > > .cmd = &cmd > > > }; > > > int err; > > > + u32 sduc_arg = SD_OCR_CCS | SD_OCR_2T; > > > > > > cmd.opcode = SD_APP_OP_COND; > > > + cmd.arg = ocr; > > > + > > > if (mmc_host_is_spi(host)) > > > - cmd.arg = ocr & (1 << 30); /* SPI only defines one bit */ > > > + cmd.arg &= (1 << 30); /* SPI only defines one bit */ > > > else > > > - cmd.arg = ocr; > > > + cmd.arg |= sduc_arg; > > > + > > > > This code doesn't belong in mmc_send_app_op_cond(), but rather in > > mmc_sd_get_cid(), which is where we add one various OCR bits before we call > > mmc_send_app_op_cond() with it. > > > > For example, if the response of the SD_SEND_IF_COND commands indicates > > an SD 2.0 compliant card, we tag on the SD_OCR_CCS bit. It looks like that > > needs to be extended to the SD_OCR_2T bit too. > OK. > > > > > > cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R3 | MMC_CMD_BCR; > > > > > > err = __mmc_poll_for_busy(host, SD_APP_OP_COND_PERIOD_US, @@ > > > -182,8 +186,15 @@ int mmc_send_app_op_cond(struct mmc_host *host, > > u32 ocr, u32 *rocr) > > > if (err) > > > return err; > > > > > > - if (rocr && !mmc_host_is_spi(host)) > > > - *rocr = cmd.resp[0]; > > > + if (!mmc_host_is_spi(host)) { > > > + if (rocr) > > > + *rocr = cmd.resp[0]; > > > + > > > + if ((cmd.resp[0] & sduc_arg) == sduc_arg) > > > + host->caps2 |= MMC_CAP2_SD_SDUC; > > > + else > > > + host->caps2 &= ~MMC_CAP2_SD_SDUC; > > > > Please don't abuse the host->caps2 for this purpose. > > > > Instead let's keep using the card->state to keep track of what type of card this > > is. You may have a look at how the MMC_CARD_SDXC bit is being used and > > just follow that behaviour for the SDUC cards too. > > > > Moreover, rather than assigning card->state at this point, let's do that from > > mmc_decode_csd() instead, when we realize that the card supports the new > > CSD structure version 3. > Just to recap - so we are all on the same page: > Ricky suggested this in v1 as well. > And we had a discussion if we should use the state field to indicate the card type. > However, Ricky had some good point why it should be here: > "... > I think host->caps2 is for host to claim caps, here can just call mmc_card_set_ult_capacity? > Don't need to wait csd, because SDXC and SDHC need to identify by capacity, but SDUC can be identified here > And all your mmc_card_is_sduc() I think change to mmc_card_ult_capacity() to know the card type > ..." > This is because according to the spec, SDUC identification is not mandated by its capacity, but rather by the rocr. In principle you are right. The rocr indicates whether it's an SDSC, SDHC, SDXC or an SDUC card. On the other hand we are checking the CSD structure version for SDSC/SDHC/SDXC - so I would rather be consistent with that way, as it seems to work fine. According to the spec the CSD structure version 3 is dedicated for SDUC cards, right? [...] Kind regards Uffe
> On Thu, 22 Aug 2024 at 15:17, Avri Altman <Avri.Altman@wdc.com> wrote: > > > > > On Wed, 14 Aug 2024 at 09:31, Avri Altman <avri.altman@wdc.com> > wrote: > > > > > > > > ACMD41 was extended to support the host-card handshake during > > > > initialization. The card expects that the HCS & HO2T bits to be > > > > set in the command argument, and sets the applicable bits in the > > > > R3 returned response. On the contrary, if a SDUC card is inserted > > > > to a non-supporting host, it will never respond to this ACMD41 > > > > until eventually, the host will timed out and give up. > > > > > > > > Tested-by: Ricky WU <ricky_wu@realtek.com> > > > > Signed-off-by: Avri Altman <avri.altman@wdc.com> > > > > --- > > > > drivers/mmc/core/sd_ops.c | 19 +++++++++++++++---- > > > > include/linux/mmc/host.h | 6 ++++++ > > > > include/linux/mmc/sd.h | 1 + > > > > 3 files changed, 22 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/drivers/mmc/core/sd_ops.c b/drivers/mmc/core/sd_ops.c > > > > index 8b9b34286ef3..7f6963dac873 100644 > > > > --- a/drivers/mmc/core/sd_ops.c > > > > +++ b/drivers/mmc/core/sd_ops.c > > > > @@ -168,12 +168,16 @@ int mmc_send_app_op_cond(struct > mmc_host > > > *host, u32 ocr, u32 *rocr) > > > > .cmd = &cmd > > > > }; > > > > int err; > > > > + u32 sduc_arg = SD_OCR_CCS | SD_OCR_2T; > > > > > > > > cmd.opcode = SD_APP_OP_COND; > > > > + cmd.arg = ocr; > > > > + > > > > if (mmc_host_is_spi(host)) > > > > - cmd.arg = ocr & (1 << 30); /* SPI only defines one bit */ > > > > + cmd.arg &= (1 << 30); /* SPI only defines one bit > > > > + */ > > > > else > > > > - cmd.arg = ocr; > > > > + cmd.arg |= sduc_arg; > > > > + > > > > > > This code doesn't belong in mmc_send_app_op_cond(), but rather in > > > mmc_sd_get_cid(), which is where we add one various OCR bits before > > > we call > > > mmc_send_app_op_cond() with it. > > > > > > For example, if the response of the SD_SEND_IF_COND commands > > > indicates an SD 2.0 compliant card, we tag on the SD_OCR_CCS bit. It > > > looks like that needs to be extended to the SD_OCR_2T bit too. > > OK. > > > > > > > > > cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R3 | MMC_CMD_BCR; > > > > > > > > err = __mmc_poll_for_busy(host, SD_APP_OP_COND_PERIOD_US, > > > > @@ > > > > -182,8 +186,15 @@ int mmc_send_app_op_cond(struct mmc_host > *host, > > > u32 ocr, u32 *rocr) > > > > if (err) > > > > return err; > > > > > > > > - if (rocr && !mmc_host_is_spi(host)) > > > > - *rocr = cmd.resp[0]; > > > > + if (!mmc_host_is_spi(host)) { > > > > + if (rocr) > > > > + *rocr = cmd.resp[0]; > > > > + > > > > + if ((cmd.resp[0] & sduc_arg) == sduc_arg) > > > > + host->caps2 |= MMC_CAP2_SD_SDUC; > > > > + else > > > > + host->caps2 &= ~MMC_CAP2_SD_SDUC; > > > > > > Please don't abuse the host->caps2 for this purpose. > > > > > > Instead let's keep using the card->state to keep track of what type > > > of card this is. You may have a look at how the MMC_CARD_SDXC bit is > > > being used and just follow that behaviour for the SDUC cards too. > > > > > > Moreover, rather than assigning card->state at this point, let's do > > > that from > > > mmc_decode_csd() instead, when we realize that the card supports the > > > new CSD structure version 3. > > Just to recap - so we are all on the same page: > > Ricky suggested this in v1 as well. > > And we had a discussion if we should use the state field to indicate the card > type. > > However, Ricky had some good point why it should be here: > > "... > > I think host->caps2 is for host to claim caps, here can just call > mmc_card_set_ult_capacity? > > Don't need to wait csd, because SDXC and SDHC need to identify by > > capacity, but SDUC can be identified here And all your > > mmc_card_is_sduc() I think change to mmc_card_ult_capacity() to know the > card type ..." > > This is because according to the spec, SDUC identification is not mandated by > its capacity, but rather by the rocr. > > In principle you are right. The rocr indicates whether it's an SDSC, SDHC, SDXC > or an SDUC card. > > On the other hand we are checking the CSD structure version for > SDSC/SDHC/SDXC - so I would rather be consistent with that way, as it seems > to work fine. Yes. > > According to the spec the CSD structure version 3 is dedicated for SDUC cards, > right? Yes. Thanks, Avri > > [...] > > Kind regards > Uffe
diff --git a/drivers/mmc/core/sd_ops.c b/drivers/mmc/core/sd_ops.c index 8b9b34286ef3..7f6963dac873 100644 --- a/drivers/mmc/core/sd_ops.c +++ b/drivers/mmc/core/sd_ops.c @@ -168,12 +168,16 @@ int mmc_send_app_op_cond(struct mmc_host *host, u32 ocr, u32 *rocr) .cmd = &cmd }; int err; + u32 sduc_arg = SD_OCR_CCS | SD_OCR_2T; cmd.opcode = SD_APP_OP_COND; + cmd.arg = ocr; + if (mmc_host_is_spi(host)) - cmd.arg = ocr & (1 << 30); /* SPI only defines one bit */ + cmd.arg &= (1 << 30); /* SPI only defines one bit */ else - cmd.arg = ocr; + cmd.arg |= sduc_arg; + cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R3 | MMC_CMD_BCR; err = __mmc_poll_for_busy(host, SD_APP_OP_COND_PERIOD_US, @@ -182,8 +186,15 @@ int mmc_send_app_op_cond(struct mmc_host *host, u32 ocr, u32 *rocr) if (err) return err; - if (rocr && !mmc_host_is_spi(host)) - *rocr = cmd.resp[0]; + if (!mmc_host_is_spi(host)) { + if (rocr) + *rocr = cmd.resp[0]; + + if ((cmd.resp[0] & sduc_arg) == sduc_arg) + host->caps2 |= MMC_CAP2_SD_SDUC; + else + host->caps2 &= ~MMC_CAP2_SD_SDUC; + } return 0; } diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h index 88c6a76042ee..a9c36a3e1a10 100644 --- a/include/linux/mmc/host.h +++ b/include/linux/mmc/host.h @@ -427,6 +427,7 @@ struct mmc_host { #define MMC_CAP2_CRYPTO 0 #endif #define MMC_CAP2_ALT_GPT_TEGRA (1 << 28) /* Host with eMMC that has GPT entry at a non-standard location */ +#define MMC_CAP2_SD_SDUC (1 << 29) /* SD over 2TB */ int fixed_drv_type; /* fixed driver type for non-removable media */ @@ -638,6 +639,11 @@ static inline int mmc_card_uhs(struct mmc_card *card) card->host->ios.timing <= MMC_TIMING_UHS_DDR50; } +static inline int mmc_card_is_sduc(struct mmc_host *host) +{ + return host->caps2 & MMC_CAP2_SD_SDUC; +} + void mmc_retune_timer_stop(struct mmc_host *host); static inline void mmc_retune_needed(struct mmc_host *host) diff --git a/include/linux/mmc/sd.h b/include/linux/mmc/sd.h index 6727576a8755..865cc0ca8543 100644 --- a/include/linux/mmc/sd.h +++ b/include/linux/mmc/sd.h @@ -36,6 +36,7 @@ /* OCR bit definitions */ #define SD_OCR_S18R (1 << 24) /* 1.8V switching request */ #define SD_ROCR_S18A SD_OCR_S18R /* 1.8V switching accepted by card */ +#define SD_OCR_2T (1 << 27) /* HO2T/CO2T - SDUC support */ #define SD_OCR_XPC (1 << 28) /* SDXC power control */ #define SD_OCR_CCS (1 << 30) /* Card Capacity Status */