答复: [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@xxxxxxxxxxxxxx> wrote:
> >
> > >
> > > On Fri, 25 Sep 2020 at 03:57, <rui_feng@xxxxxxxxxxxxxx> wrote:
> > > >
> > > > From: Rui Feng <rui_feng@xxxxxxxxxxxxxx>
> > > >
> > > > 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@xxxxxxxxxxxxxx>
> > > > ---
> > > >  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.