Message-ID: <CAPDyKFqwsJaYrXMVabR7qui6yqr4FAHfYq1ghfsf0HtRSZpGGw@mail.gmail.com>
Date:   Fri, 23 Oct 2020 10:02:15 +0200
From:   Ulf Hansson <ulf.hansson@...aro.org>
To:     冯锐 <rui_feng@...lsil.com.cn>,
        Christoph Hellwig <hch@....de>
Cc:     Arnd Bergmann <arnd@...db.de>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "linux-mmc@...r.kernel.org" <linux-mmc@...r.kernel.org>
Subject: Re: [PATCH 3/3] mmc: rtsx: Add SD Express mode support for RTS5261

+ Christoph (to help us understand if PCIe/NVMe devices can be marked read-only)

On Thu, 22 Oct 2020 at 08:04, 冯锐 <rui_feng@...lsil.com.cn> wrote:
>
> >
> > On Fri, 25 Sep 2020 at 03:57, <rui_feng@...lsil.com.cn> wrote:
> > >
> > > From: Rui Feng <rui_feng@...lsil.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@...lsil.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.

>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?

Kind regards
Uffe

Powered by blists - more mailing lists