lkml.org 
[lkml]   [2020]   [Oct]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Subject答复: [PATCH 3/3] mmc: rtsx: Add SD Express mode support for RTS5261
    Date
    > 
    > 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.

    > 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.
    But different venders may have different opinions. This is only Realtek's opinion.

    > > + }
    > > + }
    > > +
    > > 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.

    > Kind regards
    > Uffe
    >
    > ------Please consider the environment before printing this e-mail.
    \
     
     \ /
      Last update: 2020-10-22 08:05    [W:2.244 / U:0.016 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site