| Message ID | 1600999061-13669-1-git-send-email-rui_feng@realsil.com.cn |
|---|---|
| State | New |
| Headers | show |
| Series | [1/3] mmc: core: Initial support for SD express card/host | expand |
On Tue, 20 Oct 2020 at 08:52, 冯锐 <rui_feng@realsil.com.cn> wrote: > > Hi All, > > A month has passed, do I need to modify these patches? Unfortunately I didn't get the time to review them before the merge window opened, but I am looking at them now. Allow me a day or two to complete the review. Kind regards Ufffe > > Thanks > > > > > From: Rui Feng <rui_feng@realsil.com.cn> > > > > RTS5261 support legacy SD mode and SD Express mode. > > In SD7.x, SD association introduce SD Express as a new mode. > > This patch makes RTS5261 support SD Express mode. > > > > Signed-off-by: Rui Feng <rui_feng@realsil.com.cn> > > --- > > drivers/mmc/host/rtsx_pci_sdmmc.c | 59 > > +++++++++++++++++++++++++++++++ > > 1 file changed, 59 insertions(+) > > > > diff --git a/drivers/mmc/host/rtsx_pci_sdmmc.c > > b/drivers/mmc/host/rtsx_pci_sdmmc.c > > index 2763a376b054..efde374a4a5e 100644 > > --- a/drivers/mmc/host/rtsx_pci_sdmmc.c > > +++ b/drivers/mmc/host/rtsx_pci_sdmmc.c > > @@ -895,7 +895,9 @@ static int sd_set_bus_width(struct realtek_pci_sdmmc > > *host, static int sd_power_on(struct realtek_pci_sdmmc *host) { > > struct rtsx_pcr *pcr = host->pcr; > > + struct mmc_host *mmc = host->mmc; > > int err; > > + u32 val; > > > > if (host->power_state == SDMMC_POWER_ON) > > return 0; > > @@ -922,6 +924,14 @@ static int sd_power_on(struct realtek_pci_sdmmc > > *host) > > if (err < 0) > > return err; > > > > + if (PCI_PID(pcr) == PID_5261) { > > + val = rtsx_pci_readl(pcr, RTSX_BIPR); > > + if (val & SD_WRITE_PROTECT) { > > + pcr->extra_caps &= ~EXTRA_CAPS_SD_EXPRESS; > > + mmc->caps2 &= ~(MMC_CAP2_SD_EXP | > > MMC_CAP2_SD_EXP_1_2V); > > + } > > + } > > + > > host->power_state = SDMMC_POWER_ON; > > return 0; > > } > > @@ -1127,6 +1137,8 @@ static int sdmmc_get_cd(struct mmc_host *mmc) > > if (val & SD_EXIST) > > cd = 1; > > > > + if (pcr->extra_caps & EXTRA_CAPS_SD_EXPRESS) > > + mmc->caps2 |= MMC_CAP2_SD_EXP | MMC_CAP2_SD_EXP_1_2V; > > mutex_unlock(&pcr->pcr_mutex); > > > > return cd; > > @@ -1308,6 +1320,50 @@ static int sdmmc_execute_tuning(struct > > mmc_host *mmc, u32 opcode) > > return err; > > } > > > > +static int sdmmc_init_sd_express(struct mmc_host *mmc, struct mmc_ios > > +*ios) { > > + u32 relink_time, val; > > + struct realtek_pci_sdmmc *host = mmc_priv(mmc); > > + struct rtsx_pcr *pcr = host->pcr; > > + > > + /* > > + * If card has PCIe availability and WP if off, > > + * reader switch to PCIe mode. > > + */ > > + val = rtsx_pci_readl(pcr, RTSX_BIPR); > > + if (!(val & SD_WRITE_PROTECT)) { > > + /* Set relink_time for changing to PCIe card */ > > + relink_time = 0x8FFF; > > + > > + rtsx_pci_write_register(pcr, 0xFF01, 0xFF, relink_time); > > + rtsx_pci_write_register(pcr, 0xFF02, 0xFF, relink_time >> 8); > > + rtsx_pci_write_register(pcr, 0xFF03, 0x01, relink_time >> 16); > > + > > + rtsx_pci_write_register(pcr, PETXCFG, 0x80, 0x80); > > + rtsx_pci_write_register(pcr, LDO_VCC_CFG0, > > + RTS5261_LDO1_OCP_THD_MASK, > > + pcr->option.sd_800mA_ocp_thd); > > + > > + if (pcr->ops->disable_auto_blink) > > + pcr->ops->disable_auto_blink(pcr); > > + > > + /* For PCIe/NVMe mode can't enter delink issue */ > > + pcr->hw_param.interrupt_en &= ~(SD_INT_EN); > > + rtsx_pci_writel(pcr, RTSX_BIER, pcr->hw_param.interrupt_en); > > + > > + rtsx_pci_write_register(pcr, RTS5260_AUTOLOAD_CFG4, > > + RTS5261_AUX_CLK_16M_EN, RTS5261_AUX_CLK_16M_EN); > > + rtsx_pci_write_register(pcr, RTS5261_FW_CFG0, > > + RTS5261_FW_ENTER_EXPRESS, RTS5261_FW_ENTER_EXPRESS); > > + rtsx_pci_write_register(pcr, RTS5261_FW_CFG1, > > + RTS5261_MCU_BUS_SEL_MASK | > > RTS5261_MCU_CLOCK_SEL_MASK > > + | RTS5261_MCU_CLOCK_GATING | > > RTS5261_DRIVER_ENABLE_FW, > > + RTS5261_MCU_CLOCK_SEL_16M | > > RTS5261_MCU_CLOCK_GATING > > + | RTS5261_DRIVER_ENABLE_FW); > > + } > > + return 0; > > +} > > + > > static const struct mmc_host_ops realtek_pci_sdmmc_ops = { > > .pre_req = sdmmc_pre_req, > > .post_req = sdmmc_post_req, > > @@ -1317,6 +1373,7 @@ static const struct mmc_host_ops > > realtek_pci_sdmmc_ops = { > > .get_cd = sdmmc_get_cd, > > .start_signal_voltage_switch = sdmmc_switch_voltage, > > .execute_tuning = sdmmc_execute_tuning, > > + .init_sd_express = sdmmc_init_sd_express, > > }; > > > > static void init_extra_caps(struct realtek_pci_sdmmc *host) @@ -1338,6 > > +1395,8 @@ static void init_extra_caps(struct realtek_pci_sdmmc *host) > > mmc->caps |= MMC_CAP_8_BIT_DATA; > > if (pcr->extra_caps & EXTRA_CAPS_NO_MMC) > > mmc->caps2 |= MMC_CAP2_NO_MMC; > > + if (pcr->extra_caps & EXTRA_CAPS_SD_EXPRESS) > > + mmc->caps2 |= MMC_CAP2_SD_EXP | MMC_CAP2_SD_EXP_1_2V; > > } > > > > static void realtek_init_host(struct realtek_pci_sdmmc *host) > > -- > > 2.17.1 >
On Fri, 25 Sep 2020 at 03:57, <rui_feng@realsil.com.cn> wrote: > > From: Rui Feng <rui_feng@realsil.com.cn> > > RTS5261 support legacy SD mode and SD Express mode. > In SD7.x, SD association introduce SD Express as a new mode. > This patch makes RTS5261 support SD Express mode. As per patch 2, can you please add some more information about what changes are needed to support SD Express? This just states that the support is implemented, but please elaborate how. > > Signed-off-by: Rui Feng <rui_feng@realsil.com.cn> > --- > drivers/mmc/host/rtsx_pci_sdmmc.c | 59 +++++++++++++++++++++++++++++++ > 1 file changed, 59 insertions(+) > > diff --git a/drivers/mmc/host/rtsx_pci_sdmmc.c b/drivers/mmc/host/rtsx_pci_sdmmc.c > index 2763a376b054..efde374a4a5e 100644 > --- a/drivers/mmc/host/rtsx_pci_sdmmc.c > +++ b/drivers/mmc/host/rtsx_pci_sdmmc.c > @@ -895,7 +895,9 @@ static int sd_set_bus_width(struct realtek_pci_sdmmc *host, > static int sd_power_on(struct realtek_pci_sdmmc *host) > { > struct rtsx_pcr *pcr = host->pcr; > + struct mmc_host *mmc = host->mmc; > int err; > + u32 val; > > if (host->power_state == SDMMC_POWER_ON) > return 0; > @@ -922,6 +924,14 @@ static int sd_power_on(struct realtek_pci_sdmmc *host) > if (err < 0) > return err; > > + if (PCI_PID(pcr) == PID_5261) { > + val = rtsx_pci_readl(pcr, RTSX_BIPR); > + if (val & SD_WRITE_PROTECT) { > + pcr->extra_caps &= ~EXTRA_CAPS_SD_EXPRESS; > + mmc->caps2 &= ~(MMC_CAP2_SD_EXP | MMC_CAP2_SD_EXP_1_2V); This looks a bit weird to me. For a write protected card you want to disable the SD_EXPRESS support, right? Is there no mechanism to support read-only PCIe/NVMe based storage devices? If that is the case, maybe it's simply better to not support the readonly option at all for SD express cards? > + } > + } > + > host->power_state = SDMMC_POWER_ON; > return 0; > } > @@ -1127,6 +1137,8 @@ static int sdmmc_get_cd(struct mmc_host *mmc) > if (val & SD_EXIST) > cd = 1; > > + if (pcr->extra_caps & EXTRA_CAPS_SD_EXPRESS) > + mmc->caps2 |= MMC_CAP2_SD_EXP | MMC_CAP2_SD_EXP_1_2V; This looks wrong. You shouldn't be using the ->get_cd() callback to re-enable mmc caps. Normally set the mmc caps while host probes (from the ->probe() callback), but I guess this is kind of a special case, as the read-only switch state isn't known until we have powered on the card, right? If that is the case, I suggest to re-enable the mmc caps from the ->set_ios() callback instead, when ios->power_mode == MMC_POWER_OFF. > mutex_unlock(&pcr->pcr_mutex); > > return cd; > @@ -1308,6 +1320,50 @@ static int sdmmc_execute_tuning(struct mmc_host *mmc, u32 opcode) > return err; > } > > +static int sdmmc_init_sd_express(struct mmc_host *mmc, struct mmc_ios *ios) > +{ > + u32 relink_time, val; > + struct realtek_pci_sdmmc *host = mmc_priv(mmc); > + struct rtsx_pcr *pcr = host->pcr; > + > + /* > + * If card has PCIe availability and WP if off, > + * reader switch to PCIe mode. > + */ > + val = rtsx_pci_readl(pcr, RTSX_BIPR); > + if (!(val & SD_WRITE_PROTECT)) { This should not be needed, as you have already checked the write protect bit before enabling the mmc caps for SD EXPRESS, right? > + /* Set relink_time for changing to PCIe card */ > + relink_time = 0x8FFF; > + > + rtsx_pci_write_register(pcr, 0xFF01, 0xFF, relink_time); > + rtsx_pci_write_register(pcr, 0xFF02, 0xFF, relink_time >> 8); > + rtsx_pci_write_register(pcr, 0xFF03, 0x01, relink_time >> 16); > + > + rtsx_pci_write_register(pcr, PETXCFG, 0x80, 0x80); > + rtsx_pci_write_register(pcr, LDO_VCC_CFG0, > + RTS5261_LDO1_OCP_THD_MASK, > + pcr->option.sd_800mA_ocp_thd); > + > + if (pcr->ops->disable_auto_blink) > + pcr->ops->disable_auto_blink(pcr); > + > + /* For PCIe/NVMe mode can't enter delink issue */ > + pcr->hw_param.interrupt_en &= ~(SD_INT_EN); > + rtsx_pci_writel(pcr, RTSX_BIER, pcr->hw_param.interrupt_en); > + > + rtsx_pci_write_register(pcr, RTS5260_AUTOLOAD_CFG4, > + RTS5261_AUX_CLK_16M_EN, RTS5261_AUX_CLK_16M_EN); > + rtsx_pci_write_register(pcr, RTS5261_FW_CFG0, > + RTS5261_FW_ENTER_EXPRESS, RTS5261_FW_ENTER_EXPRESS); > + rtsx_pci_write_register(pcr, RTS5261_FW_CFG1, > + RTS5261_MCU_BUS_SEL_MASK | RTS5261_MCU_CLOCK_SEL_MASK > + | RTS5261_MCU_CLOCK_GATING | RTS5261_DRIVER_ENABLE_FW, > + RTS5261_MCU_CLOCK_SEL_16M | RTS5261_MCU_CLOCK_GATING > + | RTS5261_DRIVER_ENABLE_FW); > + } > + return 0; > +} > + > static const struct mmc_host_ops realtek_pci_sdmmc_ops = { > .pre_req = sdmmc_pre_req, > .post_req = sdmmc_post_req, > @@ -1317,6 +1373,7 @@ static const struct mmc_host_ops realtek_pci_sdmmc_ops = { > .get_cd = sdmmc_get_cd, > .start_signal_voltage_switch = sdmmc_switch_voltage, > .execute_tuning = sdmmc_execute_tuning, > + .init_sd_express = sdmmc_init_sd_express, > }; > > static void init_extra_caps(struct realtek_pci_sdmmc *host) > @@ -1338,6 +1395,8 @@ static void init_extra_caps(struct realtek_pci_sdmmc *host) > mmc->caps |= MMC_CAP_8_BIT_DATA; > if (pcr->extra_caps & EXTRA_CAPS_NO_MMC) > mmc->caps2 |= MMC_CAP2_NO_MMC; > + if (pcr->extra_caps & EXTRA_CAPS_SD_EXPRESS) > + mmc->caps2 |= MMC_CAP2_SD_EXP | MMC_CAP2_SD_EXP_1_2V; > } > > static void realtek_init_host(struct realtek_pci_sdmmc *host) > -- > 2.17.1 > A follow up question: Based upon your feedback from our earlier discussions, I believe you stated that the card reader driver (rtsx_pci_driver) will unregister the corresponding mfd/platform device that corresponds to the rtsx_pci_sdmmc_driver - when it gets configured to manage a PCIe/NVMe based storage device. Correct? Perhaps I didn't get that part correctly, but if this is the case, it means that the ->remove() callback (rtsx_pci_sdmmc_drv_remove()) will be invoked. Furthermore, this will cause the ->set_ios() callback to be invoked when the core calls mmc_power_off() in that path. Isn't that a problem that you need to address? Kind regards Uffe
+ Christoph (to help us understand if PCIe/NVMe devices can be marked read-only) On Thu, 22 Oct 2020 at 08:04, 冯锐 <rui_feng@realsil.com.cn> wrote: > > > > > On Fri, 25 Sep 2020 at 03:57, <rui_feng@realsil.com.cn> wrote: > > > > > > From: Rui Feng <rui_feng@realsil.com.cn> > > > > > > RTS5261 support legacy SD mode and SD Express mode. > > > In SD7.x, SD association introduce SD Express as a new mode. > > > This patch makes RTS5261 support SD Express mode. > > > > As per patch 2, can you please add some more information about what changes > > are needed to support SD Express? This just states that the support is > > implemented, but please elaborate how. > > > > > > > > Signed-off-by: Rui Feng <rui_feng@realsil.com.cn> > > > --- > > > drivers/mmc/host/rtsx_pci_sdmmc.c | 59 > > > +++++++++++++++++++++++++++++++ > > > 1 file changed, 59 insertions(+) > > > > > > diff --git a/drivers/mmc/host/rtsx_pci_sdmmc.c > > > b/drivers/mmc/host/rtsx_pci_sdmmc.c > > > index 2763a376b054..efde374a4a5e 100644 > > > --- a/drivers/mmc/host/rtsx_pci_sdmmc.c > > > +++ b/drivers/mmc/host/rtsx_pci_sdmmc.c > > > @@ -895,7 +895,9 @@ static int sd_set_bus_width(struct > > > realtek_pci_sdmmc *host, static int sd_power_on(struct > > > realtek_pci_sdmmc *host) { > > > struct rtsx_pcr *pcr = host->pcr; > > > + struct mmc_host *mmc = host->mmc; > > > int err; > > > + u32 val; > > > > > > if (host->power_state == SDMMC_POWER_ON) > > > return 0; > > > @@ -922,6 +924,14 @@ static int sd_power_on(struct realtek_pci_sdmmc > > *host) > > > if (err < 0) > > > return err; > > > > > > + if (PCI_PID(pcr) == PID_5261) { > > > + val = rtsx_pci_readl(pcr, RTSX_BIPR); > > > + if (val & SD_WRITE_PROTECT) { > > > + pcr->extra_caps &= > > ~EXTRA_CAPS_SD_EXPRESS; > > > + mmc->caps2 &= ~(MMC_CAP2_SD_EXP | > > > + MMC_CAP2_SD_EXP_1_2V); > > > > This looks a bit weird to me. For a write protected card you want to disable the > > SD_EXPRESS support, right? > > > Right. If end user insert a write protected SD express card, I will disable SD_EXPRESS support. > If host switch to SD EXPRESS mode, the card will be recognized as a writable PCIe/NVMe device, > I think this is not end user's purpose. Hmm. Falling back to use the legacy SD interface is probably not what the user expects either. Note that the physical write protect switch/pin isn't mandatory to support and it doesn't even exist for all formats of SD cards. In the mmc core, we are defaulting to make the card write enabled, if the switch isn't supported by the host driver. Additionally, nothing prevents the end user from mounting the filesystem in read-only mode, if that is preferred. > > > Is there no mechanism to support read-only PCIe/NVMe based storage devices? > > If that is the case, maybe it's simply better to not support the readonly option > > at all for SD express cards? > > > I think there's no mechanism to support read-only PCIe/NVMe based storage devices. I have looped in Christoph, maybe he can give us his opinion on this. > But different venders may have different opinions. This is only Realtek's opinion. I understand. However, the most important point for me, is that we don't end up in a situation where each mmc host handles this differently. We should strive towards a consistent behavior. At this point I tend to prefer to default to ignore the write protect switch for SD express, unless we can find a way to properly support it. > > > > + } > > > + } > > > + > > > host->power_state = SDMMC_POWER_ON; > > > return 0; > > > } > > > @@ -1127,6 +1137,8 @@ static int sdmmc_get_cd(struct mmc_host *mmc) > > > if (val & SD_EXIST) > > > cd = 1; > > > > > > + if (pcr->extra_caps & EXTRA_CAPS_SD_EXPRESS) > > > + mmc->caps2 |= MMC_CAP2_SD_EXP | > > MMC_CAP2_SD_EXP_1_2V; > > > > This looks wrong. You shouldn't be using the ->get_cd() callback to re-enable > > mmc caps. > > > > Normally set the mmc caps while host probes (from the ->probe() callback), but > > I guess this is kind of a special case, as the read-only switch state isn't known > > until we have powered on the card, right? > > > Right. > > > If that is the case, I suggest to re-enable the mmc caps from the > > ->set_ios() callback instead, when ios->power_mode == MMC_POWER_OFF. > > > I will move it to sd_power_on(). > > > > mutex_unlock(&pcr->pcr_mutex); > > > > > > return cd; > > > @@ -1308,6 +1320,50 @@ static int sdmmc_execute_tuning(struct > > mmc_host *mmc, u32 opcode) > > > return err; > > > } > > > > > > +static int sdmmc_init_sd_express(struct mmc_host *mmc, struct mmc_ios > > > +*ios) { > > > + u32 relink_time, val; > > > + struct realtek_pci_sdmmc *host = mmc_priv(mmc); > > > + struct rtsx_pcr *pcr = host->pcr; > > > + > > > + /* > > > + * If card has PCIe availability and WP if off, > > > + * reader switch to PCIe mode. > > > + */ > > > + val = rtsx_pci_readl(pcr, RTSX_BIPR); > > > + if (!(val & SD_WRITE_PROTECT)) { > > > > This should not be needed, as you have already checked the write protect bit > > before enabling the mmc caps for SD EXPRESS, right? > > > Right. > > > > + /* Set relink_time for changing to PCIe card */ > > > + relink_time = 0x8FFF; > > > + > > > + rtsx_pci_write_register(pcr, 0xFF01, 0xFF, relink_time); > > > + rtsx_pci_write_register(pcr, 0xFF02, 0xFF, relink_time >> > > 8); > > > + rtsx_pci_write_register(pcr, 0xFF03, 0x01, relink_time > > > + >> 16); > > > + > > > + rtsx_pci_write_register(pcr, PETXCFG, 0x80, 0x80); > > > + rtsx_pci_write_register(pcr, LDO_VCC_CFG0, > > > + RTS5261_LDO1_OCP_THD_MASK, > > > + pcr->option.sd_800mA_ocp_thd); > > > + > > > + if (pcr->ops->disable_auto_blink) > > > + pcr->ops->disable_auto_blink(pcr); > > > + > > > + /* For PCIe/NVMe mode can't enter delink issue */ > > > + pcr->hw_param.interrupt_en &= ~(SD_INT_EN); > > > + rtsx_pci_writel(pcr, RTSX_BIER, > > > + pcr->hw_param.interrupt_en); > > > + > > > + rtsx_pci_write_register(pcr, RTS5260_AUTOLOAD_CFG4, > > > + RTS5261_AUX_CLK_16M_EN, > > RTS5261_AUX_CLK_16M_EN); > > > + rtsx_pci_write_register(pcr, RTS5261_FW_CFG0, > > > + RTS5261_FW_ENTER_EXPRESS, > > RTS5261_FW_ENTER_EXPRESS); > > > + rtsx_pci_write_register(pcr, RTS5261_FW_CFG1, > > > + RTS5261_MCU_BUS_SEL_MASK | > > RTS5261_MCU_CLOCK_SEL_MASK > > > + | RTS5261_MCU_CLOCK_GATING | > > RTS5261_DRIVER_ENABLE_FW, > > > + RTS5261_MCU_CLOCK_SEL_16M | > > RTS5261_MCU_CLOCK_GATING > > > + | RTS5261_DRIVER_ENABLE_FW); > > > + } > > > + return 0; > > > +} > > > + > > > static const struct mmc_host_ops realtek_pci_sdmmc_ops = { > > > .pre_req = sdmmc_pre_req, > > > .post_req = sdmmc_post_req, > > > @@ -1317,6 +1373,7 @@ static const struct mmc_host_ops > > realtek_pci_sdmmc_ops = { > > > .get_cd = sdmmc_get_cd, > > > .start_signal_voltage_switch = sdmmc_switch_voltage, > > > .execute_tuning = sdmmc_execute_tuning, > > > + .init_sd_express = sdmmc_init_sd_express, > > > }; > > > > > > static void init_extra_caps(struct realtek_pci_sdmmc *host) @@ > > > -1338,6 +1395,8 @@ static void init_extra_caps(struct realtek_pci_sdmmc > > *host) > > > mmc->caps |= MMC_CAP_8_BIT_DATA; > > > if (pcr->extra_caps & EXTRA_CAPS_NO_MMC) > > > mmc->caps2 |= MMC_CAP2_NO_MMC; > > > + if (pcr->extra_caps & EXTRA_CAPS_SD_EXPRESS) > > > + mmc->caps2 |= MMC_CAP2_SD_EXP | > > MMC_CAP2_SD_EXP_1_2V; > > > } > > > > > > static void realtek_init_host(struct realtek_pci_sdmmc *host) > > > -- > > > 2.17.1 > > > > > > > A follow up question: > > > > Based upon your feedback from our earlier discussions, I believe you stated > > that the card reader driver (rtsx_pci_driver) will unregister the corresponding > > mfd/platform device that corresponds to the rtsx_pci_sdmmc_driver - when it > > gets configured to manage a PCIe/NVMe based storage device. Correct? > > > > Perhaps I didn't get that part correctly, but if this is the case, it means that the > > ->remove() callback (rtsx_pci_sdmmc_drv_remove()) will be invoked. > > Furthermore, this will cause the ->set_ios() callback to be invoked when the > > core calls mmc_power_off() in that path. Isn't that a problem that you need to > > address? > > > After init_sd_express() is called, mmc_power_off() will not work, so it's not a problem I need to > address.
On Fri, Oct 23, 2020 at 10:02:15AM +0200, Ulf Hansson wrote: > > > Is there no mechanism to support read-only PCIe/NVMe based storage devices? > > > If that is the case, maybe it's simply better to not support the readonly option > > > at all for SD express cards? > > > > > I think there's no mechanism to support read-only PCIe/NVMe based storage devices. > > I have looped in Christoph, maybe he can give us his opinion on this. NVMe namespaces can have a bunch of 'write protection' modes advertised by the controller, which Linux respects. The controller in this case would be part of the SD-Card. IMHO it is a quality of implementation issue of the SD-Card/Controller to have the the write protection mode of the namespace(s) match that of the SD interface, and the SD card spec should talk about that if it doesn't already.
On Fri, 23 Oct 2020 at 11:14, Christoph Hellwig <hch@lst.de> wrote: > > On Fri, Oct 23, 2020 at 10:02:15AM +0200, Ulf Hansson wrote: > > > > Is there no mechanism to support read-only PCIe/NVMe based storage devices? > > > > If that is the case, maybe it's simply better to not support the readonly option > > > > at all for SD express cards? > > > > > > > I think there's no mechanism to support read-only PCIe/NVMe based storage devices. > > > > I have looped in Christoph, maybe he can give us his opinion on this. > > NVMe namespaces can have a bunch of 'write protection' modes advertised > by the controller, which Linux respects. The controller in this case would > be part of the SD-Card. IMHO it is a quality of implementation issue > of the SD-Card/Controller to have the the write protection mode of the > namespace(s) match that of the SD interface, and the SD card spec should > talk about that if it doesn't already. Christoph, thanks for your reply. SD spec mentions the write-protect switch on SD cards, while uSD cards doesn't have one. In general, host drivers implement support for it via a dedicated GPIO line routed to one of the pins in the SD slot. In this SD controller case, which is based upon PCI, it works a bit differently, as the state of the write protect pin is managed through the PCI interface. If I understand you correctly, you are saying that the controller should be able to communicate (upwards to the block layer) its known write protect state for the corresponding NVMe device, during initialization? My apologies if the questions sounds silly, but I have limited knowledge about the NVMe protocol, sorry. Kind regards Uffe
On Fri, Oct 23, 2020 at 02:12:19PM +0200, Ulf Hansson wrote: > If I understand you correctly, you are saying that the controller > should be able to communicate (upwards to the block layer) its known > write protect state for the corresponding NVMe device, during > initialization? Yes.
> > + Christoph (to help us understand if PCIe/NVMe devices can be marked > + read-only) > > On Thu, 22 Oct 2020 at 08:04, 冯锐 <rui_feng@realsil.com.cn> wrote: > > > > > > > > On Fri, 25 Sep 2020 at 03:57, <rui_feng@realsil.com.cn> wrote: > > > > > > > > From: Rui Feng <rui_feng@realsil.com.cn> > > > > > > > > RTS5261 support legacy SD mode and SD Express mode. > > > > In SD7.x, SD association introduce SD Express as a new mode. > > > > This patch makes RTS5261 support SD Express mode. > > > > > > As per patch 2, can you please add some more information about what > > > changes are needed to support SD Express? This just states that the > > > support is implemented, but please elaborate how. > > > > > > > > > > > Signed-off-by: Rui Feng <rui_feng@realsil.com.cn> > > > > --- > > > > drivers/mmc/host/rtsx_pci_sdmmc.c | 59 > > > > +++++++++++++++++++++++++++++++ > > > > 1 file changed, 59 insertions(+) > > > > > > > > diff --git a/drivers/mmc/host/rtsx_pci_sdmmc.c > > > > b/drivers/mmc/host/rtsx_pci_sdmmc.c > > > > index 2763a376b054..efde374a4a5e 100644 > > > > --- a/drivers/mmc/host/rtsx_pci_sdmmc.c > > > > +++ b/drivers/mmc/host/rtsx_pci_sdmmc.c > > > > @@ -895,7 +895,9 @@ static int sd_set_bus_width(struct > > > > realtek_pci_sdmmc *host, static int sd_power_on(struct > > > > realtek_pci_sdmmc *host) { > > > > struct rtsx_pcr *pcr = host->pcr; > > > > + struct mmc_host *mmc = host->mmc; > > > > int err; > > > > + u32 val; > > > > > > > > if (host->power_state == SDMMC_POWER_ON) > > > > return 0; > > > > @@ -922,6 +924,14 @@ static int sd_power_on(struct > > > > realtek_pci_sdmmc > > > *host) > > > > if (err < 0) > > > > return err; > > > > > > > > + if (PCI_PID(pcr) == PID_5261) { > > > > + val = rtsx_pci_readl(pcr, RTSX_BIPR); > > > > + if (val & SD_WRITE_PROTECT) { > > > > + pcr->extra_caps &= > > > ~EXTRA_CAPS_SD_EXPRESS; > > > > + mmc->caps2 &= ~(MMC_CAP2_SD_EXP | > > > > + MMC_CAP2_SD_EXP_1_2V); > > > > > > This looks a bit weird to me. For a write protected card you want to > > > disable the SD_EXPRESS support, right? > > > > > Right. If end user insert a write protected SD express card, I will disable > SD_EXPRESS support. > > If host switch to SD EXPRESS mode, the card will be recognized as a > > writable PCIe/NVMe device, I think this is not end user's purpose. > > Hmm. > > Falling back to use the legacy SD interface is probably not what the user > expects either. > > Note that the physical write protect switch/pin isn't mandatory to support and > it doesn't even exist for all formats of SD cards. In the mmc core, we are > defaulting to make the card write enabled, if the switch isn't supported by the > host driver. Additionally, nothing prevents the end user from mounting the > filesystem in read-only mode, if that is preferred. > > > > > > Is there no mechanism to support read-only PCIe/NVMe based storage > devices? > > > If that is the case, maybe it's simply better to not support the > > > readonly option at all for SD express cards? > > > > > I think there's no mechanism to support read-only PCIe/NVMe based storage > devices. > > I have looped in Christoph, maybe he can give us his opinion on this. > > > But different venders may have different opinions. This is only Realtek's > opinion. > > I understand. However, the most important point for me, is that we don't end > up in a situation where each mmc host handles this differently. We should strive > towards a consistent behavior. > > At this point I tend to prefer to default to ignore the write protect switch for SD > express, unless we can find a way to properly support it. > For information security purpose, some companies or business users set their notebook SD as "read only". Because a lot of "read only" requirements from those companies or business users, notebook vendor controls reader write protect pin to achieve it. Notebook BIOS might have option to choose "read only" or not. This is why we think write protect is more important than speed. If you prefer to consistent behavior, I can ignore the write protect switch for SD express. > > From this, I assume that my interpretations of the behavior was correct. > > Although, can you please elaborate on what you mean by that it will "not > work"? > > Do you mean that rtsx_pci_card_exclusive_check() that is called early in > sdmmc_set_ios() will fail and then make it bail out? Then, could you please add > a comment about that in the code? > In init_sd_express() driver sets 0xFF54 bit0=1 and 0xFF55 bit4=0, then RTS5261 will switch MCU and enter SD EXPRESS mode. After that RTS5261 can't receive any CMD from PCIe, so mmc_power_off() will not work. > Kind regards > Uffe > > ------Please consider the environment before printing this e-mail.
On Mon, 26 Oct 2020 at 09:22, 冯锐 <rui_feng@realsil.com.cn> wrote: > > > > > + Christoph (to help us understand if PCIe/NVMe devices can be marked > > + read-only) > > > > On Thu, 22 Oct 2020 at 08:04, 冯锐 <rui_feng@realsil.com.cn> wrote: > > > > > > > > > > > On Fri, 25 Sep 2020 at 03:57, <rui_feng@realsil.com.cn> wrote: > > > > > > > > > > From: Rui Feng <rui_feng@realsil.com.cn> > > > > > > > > > > RTS5261 support legacy SD mode and SD Express mode. > > > > > In SD7.x, SD association introduce SD Express as a new mode. > > > > > This patch makes RTS5261 support SD Express mode. > > > > > > > > As per patch 2, can you please add some more information about what > > > > changes are needed to support SD Express? This just states that the > > > > support is implemented, but please elaborate how. > > > > > > > > > > > > > > Signed-off-by: Rui Feng <rui_feng@realsil.com.cn> > > > > > --- > > > > > drivers/mmc/host/rtsx_pci_sdmmc.c | 59 > > > > > +++++++++++++++++++++++++++++++ > > > > > 1 file changed, 59 insertions(+) > > > > > > > > > > diff --git a/drivers/mmc/host/rtsx_pci_sdmmc.c > > > > > b/drivers/mmc/host/rtsx_pci_sdmmc.c > > > > > index 2763a376b054..efde374a4a5e 100644 > > > > > --- a/drivers/mmc/host/rtsx_pci_sdmmc.c > > > > > +++ b/drivers/mmc/host/rtsx_pci_sdmmc.c > > > > > @@ -895,7 +895,9 @@ static int sd_set_bus_width(struct > > > > > realtek_pci_sdmmc *host, static int sd_power_on(struct > > > > > realtek_pci_sdmmc *host) { > > > > > struct rtsx_pcr *pcr = host->pcr; > > > > > + struct mmc_host *mmc = host->mmc; > > > > > int err; > > > > > + u32 val; > > > > > > > > > > if (host->power_state == SDMMC_POWER_ON) > > > > > return 0; > > > > > @@ -922,6 +924,14 @@ static int sd_power_on(struct > > > > > realtek_pci_sdmmc > > > > *host) > > > > > if (err < 0) > > > > > return err; > > > > > > > > > > + if (PCI_PID(pcr) == PID_5261) { > > > > > + val = rtsx_pci_readl(pcr, RTSX_BIPR); > > > > > + if (val & SD_WRITE_PROTECT) { > > > > > + pcr->extra_caps &= > > > > ~EXTRA_CAPS_SD_EXPRESS; > > > > > + mmc->caps2 &= ~(MMC_CAP2_SD_EXP | > > > > > + MMC_CAP2_SD_EXP_1_2V); > > > > > > > > This looks a bit weird to me. For a write protected card you want to > > > > disable the SD_EXPRESS support, right? > > > > > > > Right. If end user insert a write protected SD express card, I will disable > > SD_EXPRESS support. > > > If host switch to SD EXPRESS mode, the card will be recognized as a > > > writable PCIe/NVMe device, I think this is not end user's purpose. > > > > Hmm. > > > > Falling back to use the legacy SD interface is probably not what the user > > expects either. > > > > Note that the physical write protect switch/pin isn't mandatory to support and > > it doesn't even exist for all formats of SD cards. In the mmc core, we are > > defaulting to make the card write enabled, if the switch isn't supported by the > > host driver. Additionally, nothing prevents the end user from mounting the > > filesystem in read-only mode, if that is preferred. > > > > > > > > > Is there no mechanism to support read-only PCIe/NVMe based storage > > devices? > > > > If that is the case, maybe it's simply better to not support the > > > > readonly option at all for SD express cards? > > > > > > > I think there's no mechanism to support read-only PCIe/NVMe based storage > > devices. > > > > I have looped in Christoph, maybe he can give us his opinion on this. > > > > > But different venders may have different opinions. This is only Realtek's > > opinion. > > > > I understand. However, the most important point for me, is that we don't end > > up in a situation where each mmc host handles this differently. We should strive > > towards a consistent behavior. > > > > At this point I tend to prefer to default to ignore the write protect switch for SD > > express, unless we can find a way to properly support it. > > > For information security purpose, some companies or business users set their notebook SD as "read only". > Because a lot of "read only" requirements from those companies or business users, notebook vendor controls reader write protect pin to achieve it. > Notebook BIOS might have option to choose "read only" or not. > This is why we think write protect is more important than speed. I understand that it may be used, in some way or the other to provide a hint to the operating system to mount it in read-only mode. Although, if there were a real security feature involved, the internal FW of the SD card would also monitor the switch, to support read-only mode. As I understand it, that's not the common case. > If you prefer to consistent behavior, I can ignore the write protect switch for SD express. At this point, I prefer if you would ignore the write protect switch in the SD controller driver. According to Christoph, it should be possible to support read-only mode via PCIe/NVMe. You may need to add some tweaks to support this in the PCIe controller driver, but I can't advise you how to exactly do this. Perhaps you need to read/store the state of SD write-protect pin before switching to SD express mode, because you may not be able to read it beyond some point? > > > > > From this, I assume that my interpretations of the behavior was correct. > > > > Although, can you please elaborate on what you mean by that it will "not > > work"? > > > > Do you mean that rtsx_pci_card_exclusive_check() that is called early in > > sdmmc_set_ios() will fail and then make it bail out? Then, could you please add > > a comment about that in the code? > > > In init_sd_express() driver sets 0xFF54 bit0=1 and 0xFF55 bit4=0, then RTS5261 will switch MCU and enter SD EXPRESS mode. > After that RTS5261 can't receive any CMD from PCIe, so mmc_power_off() will not work. Thanks for trying to clarify. However, this still doesn't explain to me, what *exactly* will happen when rtsx_pci_card_exclusive_check() is called (or any other functions in ->set_ios()). In principle, "will not work" could mean that the calls to the rtsx_pci_* cardreader interface hangs - and that would not be okay (as it could lead to that the ->remove() callback hangs). So, either you need to put a well written comment in the code about what will happen - or add some kind of protection against potential problems for this. Kind regards Uffe
On Tue, Oct 27, 2020 at 01:54:46PM +0100, Ulf Hansson wrote: > > For information security purpose, some companies or business users set their notebook SD as "read only". > > Because a lot of "read only" requirements from those companies or business users, notebook vendor controls reader write protect pin to achieve it. > > Notebook BIOS might have option to choose "read only" or not. > > This is why we think write protect is more important than speed. > > I understand that it may be used, in some way or the other to provide > a hint to the operating system to mount it in read-only mode. > > Although, if there were a real security feature involved, the internal > FW of the SD card would also monitor the switch, to support read-only > mode. As I understand it, that's not the common case. Yes. "Security" that relies on the driver to fall back to a different mode doesn't work. > > > If you prefer to consistent behavior, I can ignore the write protect switch for SD express. > > At this point, I prefer if you would ignore the write protect switch > in the SD controller driver. Same here. > According to Christoph, it should be possible to support read-only > mode via PCIe/NVMe. You may need to add some tweaks to support this in > the PCIe controller driver, but I can't advise you how to exactly do > this. The NVMe driver already supports write protected namespaces. I'll ask my contact in the JEDEC SD card working group if there was any consideration of the read-only handling for classic SD vs NVMe.
> > On Mon, 26 Oct 2020 at 09:22, 冯锐 <rui_feng@realsil.com.cn> wrote: > > > > > > > > + Christoph (to help us understand if PCIe/NVMe devices can be > > > + marked > > > + read-only) > > > > > > On Thu, 22 Oct 2020 at 08:04, 冯锐 <rui_feng@realsil.com.cn> wrote: > > > > > > > > > > > > > > On Fri, 25 Sep 2020 at 03:57, <rui_feng@realsil.com.cn> wrote: > > > > > > > > > > > > From: Rui Feng <rui_feng@realsil.com.cn> > > > > > > > > > > > > RTS5261 support legacy SD mode and SD Express mode. > > > > > > In SD7.x, SD association introduce SD Express as a new mode. > > > > > > This patch makes RTS5261 support SD Express mode. > > > > > > > > > > As per patch 2, can you please add some more information about > > > > > what changes are needed to support SD Express? This just states > > > > > that the support is implemented, but please elaborate how. > > > > > > > > > > > > > > > > > Signed-off-by: Rui Feng <rui_feng@realsil.com.cn> > > > > > > --- > > > > > > drivers/mmc/host/rtsx_pci_sdmmc.c | 59 > > > > > > +++++++++++++++++++++++++++++++ > > > > > > 1 file changed, 59 insertions(+) > > > > > > > > > > > > diff --git a/drivers/mmc/host/rtsx_pci_sdmmc.c > > > > > > b/drivers/mmc/host/rtsx_pci_sdmmc.c > > > > > > index 2763a376b054..efde374a4a5e 100644 > > > > > > --- a/drivers/mmc/host/rtsx_pci_sdmmc.c > > > > > > +++ b/drivers/mmc/host/rtsx_pci_sdmmc.c > > > > > > @@ -895,7 +895,9 @@ static int sd_set_bus_width(struct > > > > > > realtek_pci_sdmmc *host, static int sd_power_on(struct > > > > > > realtek_pci_sdmmc *host) { > > > > > > struct rtsx_pcr *pcr = host->pcr; > > > > > > + struct mmc_host *mmc = host->mmc; > > > > > > int err; > > > > > > + u32 val; > > > > > > > > > > > > if (host->power_state == SDMMC_POWER_ON) > > > > > > return 0; > > > > > > @@ -922,6 +924,14 @@ static int sd_power_on(struct > > > > > > realtek_pci_sdmmc > > > > > *host) > > > > > > if (err < 0) > > > > > > return err; > > > > > > > > > > > > + if (PCI_PID(pcr) == PID_5261) { > > > > > > + val = rtsx_pci_readl(pcr, RTSX_BIPR); > > > > > > + if (val & SD_WRITE_PROTECT) { > > > > > > + pcr->extra_caps &= > > > > > ~EXTRA_CAPS_SD_EXPRESS; > > > > > > + mmc->caps2 &= ~(MMC_CAP2_SD_EXP > | > > > > > > + MMC_CAP2_SD_EXP_1_2V); > > > > > > > > > > This looks a bit weird to me. For a write protected card you > > > > > want to disable the SD_EXPRESS support, right? > > > > > > > > > Right. If end user insert a write protected SD express card, I > > > > will disable > > > SD_EXPRESS support. > > > > If host switch to SD EXPRESS mode, the card will be recognized as > > > > a writable PCIe/NVMe device, I think this is not end user's purpose. > > > > > > Hmm. > > > > > > Falling back to use the legacy SD interface is probably not what the > > > user expects either. > > > > > > Note that the physical write protect switch/pin isn't mandatory to > > > support and it doesn't even exist for all formats of SD cards. In > > > the mmc core, we are defaulting to make the card write enabled, if > > > the switch isn't supported by the host driver. Additionally, nothing > > > prevents the end user from mounting the filesystem in read-only mode, if > that is preferred. > > > > > > > > > > > > Is there no mechanism to support read-only PCIe/NVMe based > > > > > storage > > > devices? > > > > > If that is the case, maybe it's simply better to not support the > > > > > readonly option at all for SD express cards? > > > > > > > > > I think there's no mechanism to support read-only PCIe/NVMe based > > > > storage > > > devices. > > > > > > I have looped in Christoph, maybe he can give us his opinion on this. > > > > > > > But different venders may have different opinions. This is only > > > > Realtek's > > > opinion. > > > > > > I understand. However, the most important point for me, is that we > > > don't end up in a situation where each mmc host handles this > > > differently. We should strive towards a consistent behavior. > > > > > > At this point I tend to prefer to default to ignore the write > > > protect switch for SD express, unless we can find a way to properly support > it. > > > > > For information security purpose, some companies or business users set their > notebook SD as "read only". > > Because a lot of "read only" requirements from those companies or business > users, notebook vendor controls reader write protect pin to achieve it. > > Notebook BIOS might have option to choose "read only" or not. > > This is why we think write protect is more important than speed. > > I understand that it may be used, in some way or the other to provide a hint to > the operating system to mount it in read-only mode. > > Although, if there were a real security feature involved, the internal FW of the > SD card would also monitor the switch, to support read-only mode. As I > understand it, that's not the common case. > > > If you prefer to consistent behavior, I can ignore the write protect switch for > SD express. > > At this point, I prefer if you would ignore the write protect switch in the SD > controller driver. > I will ignore write protect switch in V3. > According to Christoph, it should be possible to support read-only mode via > PCIe/NVMe. You may need to add some tweaks to support this in the PCIe > controller driver, but I can't advise you how to exactly do this. > > Perhaps you need to read/store the state of SD write-protect pin before > switching to SD express mode, because you may not be able to read it beyond > some point? > > > > > > > > > From this, I assume that my interpretations of the behavior was correct. > > > > > > Although, can you please elaborate on what you mean by that it will > > > "not work"? > > > > > > Do you mean that rtsx_pci_card_exclusive_check() that is called > > > early in > > > sdmmc_set_ios() will fail and then make it bail out? Then, could you > > > please add a comment about that in the code? > > > > > In init_sd_express() driver sets 0xFF54 bit0=1 and 0xFF55 bit4=0, then > RTS5261 will switch MCU and enter SD EXPRESS mode. > > After that RTS5261 can't receive any CMD from PCIe, so mmc_power_off() > will not work. > > Thanks for trying to clarify. > > However, this still doesn't explain to me, what *exactly* will happen when > rtsx_pci_card_exclusive_check() is called (or any other functions in ->set_ios()). > > In principle, "will not work" could mean that the calls to the > rtsx_pci_* cardreader interface hangs - and that would not be okay (as it could > lead to that the ->remove() callback hangs). So, either you need to put a well > written comment in the code about what will happen > - or add some kind of protection against potential problems for this. > "will not work" mean fail and will not hang interface. I will add "host->eject = true" in the end of init_sd_express(), so that set_ios() will do nothing just return. > Kind regards > Uffe > > ------Please consider the environment before printing this e-mail.
On Wed, 28 Oct 2020 at 11:05, 冯锐 <rui_feng@realsil.com.cn> wrote: > > > > > > > > > On Mon, 26 Oct 2020 at 09:22, 冯锐 <rui_feng@realsil.com.cn> wrote: > > > > > > > > > > > > > > + Christoph (to help us understand if PCIe/NVMe devices can be > > > > > + marked > > > > > + read-only) > > > > > > > > > > On Thu, 22 Oct 2020 at 08:04, 冯锐 <rui_feng@realsil.com.cn> wrote: > > > > > > > > > > > > > > > > > > > > On Fri, 25 Sep 2020 at 03:57, <rui_feng@realsil.com.cn> wrote: > > > > > > > > > > > > > > > > From: Rui Feng <rui_feng@realsil.com.cn> > > > > > > > > > > > > > > > > RTS5261 support legacy SD mode and SD Express mode. > > > > > > > > In SD7.x, SD association introduce SD Express as a new mode. > > > > > > > > This patch makes RTS5261 support SD Express mode. > > > > > > > > > > > > > > As per patch 2, can you please add some more information about > > > > > > > what changes are needed to support SD Express? This just > > > > > > > states that the support is implemented, but please elaborate how. > > > > > > > > > > > > > > > > > > > > > > > Signed-off-by: Rui Feng <rui_feng@realsil.com.cn> > > > > > > > > --- > > > > > > > > drivers/mmc/host/rtsx_pci_sdmmc.c | 59 > > > > > > > > +++++++++++++++++++++++++++++++ > > > > > > > > 1 file changed, 59 insertions(+) > > > > > > > > > > > > > > > > diff --git a/drivers/mmc/host/rtsx_pci_sdmmc.c > > > > > > > > b/drivers/mmc/host/rtsx_pci_sdmmc.c > > > > > > > > index 2763a376b054..efde374a4a5e 100644 > > > > > > > > --- a/drivers/mmc/host/rtsx_pci_sdmmc.c > > > > > > > > +++ b/drivers/mmc/host/rtsx_pci_sdmmc.c > > > > > > > > @@ -895,7 +895,9 @@ static int sd_set_bus_width(struct > > > > > > > > realtek_pci_sdmmc *host, static int sd_power_on(struct > > > > > > > > realtek_pci_sdmmc *host) { > > > > > > > > struct rtsx_pcr *pcr = host->pcr; > > > > > > > > + struct mmc_host *mmc = host->mmc; > > > > > > > > int err; > > > > > > > > + u32 val; > > > > > > > > > > > > > > > > if (host->power_state == SDMMC_POWER_ON) > > > > > > > > return 0; > > > > > > > > @@ -922,6 +924,14 @@ static int sd_power_on(struct > > > > > > > > realtek_pci_sdmmc > > > > > > > *host) > > > > > > > > if (err < 0) > > > > > > > > return err; > > > > > > > > > > > > > > > > + if (PCI_PID(pcr) == PID_5261) { > > > > > > > > + val = rtsx_pci_readl(pcr, RTSX_BIPR); > > > > > > > > + if (val & SD_WRITE_PROTECT) { > > > > > > > > + pcr->extra_caps &= > > > > > > > ~EXTRA_CAPS_SD_EXPRESS; > > > > > > > > + mmc->caps2 &= > > ~(MMC_CAP2_SD_EXP > > > | > > > > > > > > + MMC_CAP2_SD_EXP_1_2V); > > > > > > > > > > > > > > This looks a bit weird to me. For a write protected card you > > > > > > > want to disable the SD_EXPRESS support, right? > > > > > > > > > > > > > Right. If end user insert a write protected SD express card, I > > > > > > will disable > > > > > SD_EXPRESS support. > > > > > > If host switch to SD EXPRESS mode, the card will be recognized > > > > > > as a writable PCIe/NVMe device, I think this is not end user's purpose. > > > > > > > > > > Hmm. > > > > > > > > > > Falling back to use the legacy SD interface is probably not what > > > > > the user expects either. > > > > > > > > > > Note that the physical write protect switch/pin isn't mandatory to > > > > > support and it doesn't even exist for all formats of SD cards. In > > > > > the mmc core, we are defaulting to make the card write enabled, if > > > > > the switch isn't supported by the host driver. Additionally, > > > > > nothing prevents the end user from mounting the filesystem in > > > > > read-only mode, if > > > that is preferred. > > > > > > > > > > > > > > > > > > Is there no mechanism to support read-only PCIe/NVMe based > > > > > > > storage > > > > > devices? > > > > > > > If that is the case, maybe it's simply better to not support > > > > > > > the readonly option at all for SD express cards? > > > > > > > > > > > > > I think there's no mechanism to support read-only PCIe/NVMe > > > > > > based storage > > > > > devices. > > > > > > > > > > I have looped in Christoph, maybe he can give us his opinion on this. > > > > > > > > > > > But different venders may have different opinions. This is only > > > > > > Realtek's > > > > > opinion. > > > > > > > > > > I understand. However, the most important point for me, is that we > > > > > don't end up in a situation where each mmc host handles this > > > > > differently. We should strive towards a consistent behavior. > > > > > > > > > > At this point I tend to prefer to default to ignore the write > > > > > protect switch for SD express, unless we can find a way to > > > > > properly support > > > it. > > > > > > > > > For information security purpose, some companies or business users > > > > set their > > > notebook SD as "read only". > > > > Because a lot of "read only" requirements from those companies or > > > > business > > > users, notebook vendor controls reader write protect pin to achieve it. > > > > Notebook BIOS might have option to choose "read only" or not. > > > > This is why we think write protect is more important than speed. > > > > > > I understand that it may be used, in some way or the other to provide > > > a hint to the operating system to mount it in read-only mode. > > > > > > Although, if there were a real security feature involved, the internal > > > FW of the SD card would also monitor the switch, to support read-only > > > mode. As I understand it, that's not the common case. > > > > > > > If you prefer to consistent behavior, I can ignore the write protect > > > > switch for > > > SD express. > > > > > > At this point, I prefer if you would ignore the write protect switch > > > in the SD controller driver. > > > > > I will ignore write protect switch in V3. > > > Sorry I ignore the HW design. > The reader has two mechanism for mode selection (SD Legacy or SD Express). One is SW (MMC driver) and another is HW. > We use HW mechanism when system exit S3 or S4. > HW mechanism selects mode when chip is power on. > Here is an example for HW mechanism. > 1. Reader in SD Legacy mode -> > 2. SD Express card insert -> > 3. MMC driver selects the SD Express mode -> > 4. SD Express initial and use NVMe driver and NVMe disk mount -> > 5. system goes to S4 -> > 6. system exits S4 -> > 7. HW selects SD Express mode -> > 8. SD Express still uses NVMe driver and disk keeps the same > Therefore, after S4, disk is still keep the same. > > Because of HW mechanism selects SD legacy mode when write protect. > If driver can't select SD legacy mode when write protect, disk might unmount and than mount after S3/S4. > Here is an example for write protect. > 1. Reader in SD Legacy mode -> > 2. SD Express card insert with write protect -> > 3. MMC driver selects the SD Express mode -> > 4. SD Express initial and use NVMe driver and NVMe disk mount -> > 5. system goes to S4 -> > 6. system exits S4 -> > 7. Because write protect, HW selects SD legacy mode -> > 8. linux detect HW change, use MMC driver and NVMe disk unmount -> > 9. MMC driver selects the SD Express mode -> > 10. SD Express initial and use NVMe driver and NVMe disk mount > > If driver can select SD legacy mode when write protect, disk can keep the same after S3/S4. > Here is an example for write protect. > 1. Reader in SD Legacy mode -> > 2. SD Express card insert with write protect -> > 3. MMC driver selects the SD legacy mode and disk mount -> > 5. system goes to S4 -> > 6. system exits S4 -> > 7. Because write protect, HW selects SD legacy mode -> > 8. MMC driver selects the SD legacy mode and disk keeps the same. > If I ignore the write protect switch in mmc host driver, behavior of SW will not be consistent with HW. Alright, let's keep the code monitoring the write protect switch then. However, please add a comment in the code that it's needed because the HW reads it when resuming from S3/S4 (and then picks SD legacy interface if it's set in read-only mode). [...] Kind regards Uffe
On Fri, Oct 23, 2020 at 02:12:19PM +0200, Ulf Hansson wrote: > SD spec mentions the write-protect switch on SD cards, while uSD cards > doesn't have one. In general, host drivers implement support for it > via a dedicated GPIO line routed to one of the pins in the SD slot. > > In this SD controller case, which is based upon PCI, it works a bit > differently, as the state of the write protect pin is managed through > the PCI interface. > > If I understand you correctly, you are saying that the controller > should be able to communicate (upwards to the block layer) its known > write protect state for the corresponding NVMe device, during > initialization? I got an answer form a member of the SD commitee, and the answer is: "The SD specification define that case very clearly as following. If card’s access is restricted through one of the SD unique features – PSWD Lock/Unlock, Temporary or Permanent WP (TWP or PWP) or CPRM then if access attempt is done through its NVMe interface the card will restrict the access and respond with Access denied. The access restriction shall be removed through the SD protocol/interface before being able to access the card through the NVMe. That is defined as following in the section 8.1.6 of the SD7.0 onward." Note that if you look at the spec this means only rejecting NVMe commands that write dta for the write protect pin.
> -----Original Message----- > From: Christoph Hellwig [mailto:hch@lst.de] > Sent: Friday, November 13, 2020 12:42 AM > To: Ulf Hansson <ulf.hansson@linaro.org> > Cc: Christoph Hellwig <hch@lst.de>; 冯锐 <rui_feng@realsil.com.cn>; Arnd > Bergmann <arnd@arndb.de>; Greg Kroah-Hartman > <gregkh@linuxfoundation.org>; Linux Kernel Mailing List > <linux-kernel@vger.kernel.org>; linux-mmc@vger.kernel.org > Subject: Re: [PATCH 3/3] mmc: rtsx: Add SD Express mode support for RTS5261 > > On Fri, Oct 23, 2020 at 02:12:19PM +0200, Ulf Hansson wrote: > > SD spec mentions the write-protect switch on SD cards, while uSD cards > > doesn't have one. In general, host drivers implement support for it > > via a dedicated GPIO line routed to one of the pins in the SD slot. > > > > In this SD controller case, which is based upon PCI, it works a bit > > differently, as the state of the write protect pin is managed through > > the PCI interface. > > > > If I understand you correctly, you are saying that the controller > > should be able to communicate (upwards to the block layer) its known > > write protect state for the corresponding NVMe device, during > > initialization? > > I got an answer form a member of the SD commitee, and the answer is: > > "The SD specification define that case very clearly as following. > If card’s access is restricted through one of the SD unique features – PSWD > Lock/Unlock, Temporary or Permanent WP (TWP or PWP) or CPRM then if > access attempt is done through its NVMe interface the card will restrict the > access > and respond with Access denied. The access restriction shall be removed > through the SD protocol/interface before being able to access the card through > the NVMe. > That is defined as following in the section 8.1.6 of the SD7.0 onward." > > Note that if you look at the spec this means only rejecting NVMe commands > that write dta for the write protect pin. > You may misunderstand my meaning. The "WP" I mean is the "Mechanical Write Protect Switch" and it's defined in section 4.3.6 of The SD specification. The "WP" you mentioned is "Card Internal Write Protect". They are two different "WP". For "Mechanical Write Protect Switch" , The SD specification define as following. "It is the responsibility of the host to protect the card. The position of the write protect switch is unknown to the Internal circuitry of the card." So we use legacy SD interface to handle the card which is "WP" by "Mechanical Write Protect Switch".
diff --git a/drivers/mmc/host/rtsx_pci_sdmmc.c b/drivers/mmc/host/rtsx_pci_sdmmc.c index 2763a376b054..efde374a4a5e 100644 --- a/drivers/mmc/host/rtsx_pci_sdmmc.c +++ b/drivers/mmc/host/rtsx_pci_sdmmc.c @@ -895,7 +895,9 @@ static int sd_set_bus_width(struct realtek_pci_sdmmc *host, static int sd_power_on(struct realtek_pci_sdmmc *host) { struct rtsx_pcr *pcr = host->pcr; + struct mmc_host *mmc = host->mmc; int err; + u32 val; if (host->power_state == SDMMC_POWER_ON) return 0; @@ -922,6 +924,14 @@ static int sd_power_on(struct realtek_pci_sdmmc *host) if (err < 0) return err; + if (PCI_PID(pcr) == PID_5261) { + val = rtsx_pci_readl(pcr, RTSX_BIPR); + if (val & SD_WRITE_PROTECT) { + pcr->extra_caps &= ~EXTRA_CAPS_SD_EXPRESS; + mmc->caps2 &= ~(MMC_CAP2_SD_EXP | MMC_CAP2_SD_EXP_1_2V); + } + } + host->power_state = SDMMC_POWER_ON; return 0; } @@ -1127,6 +1137,8 @@ static int sdmmc_get_cd(struct mmc_host *mmc) if (val & SD_EXIST) cd = 1; + if (pcr->extra_caps & EXTRA_CAPS_SD_EXPRESS) + mmc->caps2 |= MMC_CAP2_SD_EXP | MMC_CAP2_SD_EXP_1_2V; mutex_unlock(&pcr->pcr_mutex); return cd; @@ -1308,6 +1320,50 @@ static int sdmmc_execute_tuning(struct mmc_host *mmc, u32 opcode) return err; } +static int sdmmc_init_sd_express(struct mmc_host *mmc, struct mmc_ios *ios) +{ + u32 relink_time, val; + struct realtek_pci_sdmmc *host = mmc_priv(mmc); + struct rtsx_pcr *pcr = host->pcr; + + /* + * If card has PCIe availability and WP if off, + * reader switch to PCIe mode. + */ + val = rtsx_pci_readl(pcr, RTSX_BIPR); + if (!(val & SD_WRITE_PROTECT)) { + /* Set relink_time for changing to PCIe card */ + relink_time = 0x8FFF; + + rtsx_pci_write_register(pcr, 0xFF01, 0xFF, relink_time); + rtsx_pci_write_register(pcr, 0xFF02, 0xFF, relink_time >> 8); + rtsx_pci_write_register(pcr, 0xFF03, 0x01, relink_time >> 16); + + rtsx_pci_write_register(pcr, PETXCFG, 0x80, 0x80); + rtsx_pci_write_register(pcr, LDO_VCC_CFG0, + RTS5261_LDO1_OCP_THD_MASK, + pcr->option.sd_800mA_ocp_thd); + + if (pcr->ops->disable_auto_blink) + pcr->ops->disable_auto_blink(pcr); + + /* For PCIe/NVMe mode can't enter delink issue */ + pcr->hw_param.interrupt_en &= ~(SD_INT_EN); + rtsx_pci_writel(pcr, RTSX_BIER, pcr->hw_param.interrupt_en); + + rtsx_pci_write_register(pcr, RTS5260_AUTOLOAD_CFG4, + RTS5261_AUX_CLK_16M_EN, RTS5261_AUX_CLK_16M_EN); + rtsx_pci_write_register(pcr, RTS5261_FW_CFG0, + RTS5261_FW_ENTER_EXPRESS, RTS5261_FW_ENTER_EXPRESS); + rtsx_pci_write_register(pcr, RTS5261_FW_CFG1, + RTS5261_MCU_BUS_SEL_MASK | RTS5261_MCU_CLOCK_SEL_MASK + | RTS5261_MCU_CLOCK_GATING | RTS5261_DRIVER_ENABLE_FW, + RTS5261_MCU_CLOCK_SEL_16M | RTS5261_MCU_CLOCK_GATING + | RTS5261_DRIVER_ENABLE_FW); + } + return 0; +} + static const struct mmc_host_ops realtek_pci_sdmmc_ops = { .pre_req = sdmmc_pre_req, .post_req = sdmmc_post_req, @@ -1317,6 +1373,7 @@ static const struct mmc_host_ops realtek_pci_sdmmc_ops = { .get_cd = sdmmc_get_cd, .start_signal_voltage_switch = sdmmc_switch_voltage, .execute_tuning = sdmmc_execute_tuning, + .init_sd_express = sdmmc_init_sd_express, }; static void init_extra_caps(struct realtek_pci_sdmmc *host) @@ -1338,6 +1395,8 @@ static void init_extra_caps(struct realtek_pci_sdmmc *host) mmc->caps |= MMC_CAP_8_BIT_DATA; if (pcr->extra_caps & EXTRA_CAPS_NO_MMC) mmc->caps2 |= MMC_CAP2_NO_MMC; + if (pcr->extra_caps & EXTRA_CAPS_SD_EXPRESS) + mmc->caps2 |= MMC_CAP2_SD_EXP | MMC_CAP2_SD_EXP_1_2V; } static void realtek_init_host(struct realtek_pci_sdmmc *host)