Message-ID: <CAPDyKFrC5Yv_WmENL4mYWum-bE2XDyvGRCuid0bxqKZ6HX78Fg@mail.gmail.com>
Date:   Mon, 6 Jul 2020 16:42:17 +0200
From:   Ulf Hansson <ulf.hansson@...aro.org>
To:     冯锐 <rui_feng@...lsil.com.cn>
Cc:     Arnd Bergmann <arnd@...db.de>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
        linux-mmc <linux-mmc@...r.kernel.org>
Subject: Re: [PATCH] mmc: rtsx: Add SD Express mode support for RTS5261

On Wed, 1 Jul 2020 at 11:52, 冯锐 <rui_feng@...lsil.com.cn> wrote:
>
> Hi Hansson:
>
> I'm sorry to bother you. One month ago you said you will post some patches and keep my posted,
> but I can't found the patches or I miss the patches? Users are looking forward to the patch, If you
> are busy, I'll post a patch to let the retry in mmc core to do nothing just return in rtsx host driver.

Apologize for the delay. It turned out to be a little more complex
than I first thought. Also, I got my hands on the "Part A2 SD Host
Controller Specification Ver7.00 Draft", which I would like to have a
closer look at before posting a patch.

I will try my best to get something submitted this week (and I will
keep you in the loop, of course).

Kind regards
Uffe

>
> Kind regards
>
> > On Tue, 2 Jun 2020 at 04:41, 冯锐 <rui_feng@...lsil.com.cn> wrote:
> > >
> > > >
> > > > +linux-mmc
> > > >
> > > > On Mon, 1 Jun 2020 at 09:34, 冯锐 <rui_feng@...lsil.com.cn> wrote:
> > > > >
> > > > > >
> > > > > > On Tue, 19 May 2020 at 11:18, 冯锐 <rui_feng@...lsil.com.cn> wrote:
> > > > > > >
> > > > > > > > On Tue, 28 Apr 2020 at 05:44, 冯锐 <rui_feng@...lsil.com.cn>
> > wrote:
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > On Mon, Apr 27, 2020 at 11:41 AM 冯锐
> > > > > > > > > > <rui_feng@...lsil.com.cn>
> > > > > > > > wrote:
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > > On Sun, Apr 26, 2020 at 09:25:46AM +0800,
> > > > > > > > > > > > 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.
> > > > > > > > > > > > > SD Express mode is distinguished by CMD8.
> > > > > > > > > > > > > Therefore, CMD8 has new bit for SD Express.
> > > > > > > > > > > > > SD Express is based on PCIe/NVMe.
> > > > > > > > > > > > > RTS5261 uses CMD8 to switch to SD Express mode.
> > > > > > > > > > > >
> > > > > > > > > > > > So how does this bit work?  They way I imagined SD
> > > > > > > > > > > > Express to work is that the actual SD Card just
> > > > > > > > > > > > shows up as a real PCIe device, similar to say Thunderbolt.
> > > > > > > > > > >
> > > > > > > > > > > New SD Express card has dual mode. One is SD mode and
> > > > > > > > > > > another is PCIe
> > > > > > > > > > mode.
> > > > > > > > > > > In PCIe mode, it act as a PCIe device and use PCIe
> > > > > > > > > > > protocol not Thunderbolt
> > > > > > > > > > protocol.
> > > > > > > > > >
> > > > > > > > > > I think what Christoph was asking about is why you need
> > > > > > > > > > to issue any commands at all in SD mode when you want to
> > > > > > > > > > use PCIe mode
> > > > > > instead.
> > > > > > > > > > What happens if you load the NVMe dthriver before
> > > > > > > > > > loading the
> > > > > > > > > > rts5261
> > > > > > > > driver?
> > > > > > > > > >
> > > > > > > > > >        Arnd
> > > > > > > > > >
> > > > > > > > > > ------Please consider the environment before printing this e-mail.
> > > > > > > > >
> > > > > > > > > RTS5261 support SD mode and PCIe/NVMe mode. The workflow
> > > > > > > > > is as
> > > > > > follows.
> > > > > > > > > 1.RTS5261 work in SD mode.
> > > > > > > > > 2.If card is plugged in, Host send CMD8 to ask card's PCIe
> > availability.
> > > > > > > >
> > > > > > > > This sounds like the card insert/removal needs to be managed
> > > > > > > > by the rtsx_pci_sdmmc driver (mmc).
> > > > > > > >
> > > > > > > > > 3.If the card has PCIe availability, RTS5261 switch to
> > > > > > > > > PCIe/NVMe
> > > > mode.
> > > > > > > >
> > > > > > > > This switch is done by the mmc driver, but how does the
> > > > > > > > PCIe/NVMe driver know when to take over? Isn't there a
> > > > synchronization point needed?
> > > > > > > >
> > > > > > > > > 4.Mmc driver exit and NVMe driver start working.
> > > > > > > >
> > > > > > > > Having the mmc driver to exit seems wrong to me. Else how
> > > > > > > > would you handle a card being removed and inserted again?
> > > > > > > >
> > > > > > > > In principle you want the mmc core to fail to detect the
> > > > > > > > card and then do a handover, somehow. No?
> > > > > > > >
> > > > > > > > Although, to make this work there are a couple of problems
> > > > > > > > you need to deal with.
> > > > > > > >
> > > > > > > > 1. If the mmc core doesn't successfully detect a card, it
> > > > > > > > will request the mmc host to power off the card. In this
> > > > > > > > situation, you want to keep the power to the card, but leave
> > > > > > > > it to be managed by the
> > > > > > PCIe/NVMe driver in some way.
> > > > > > > >
> > > > > > > > 2. During system resume, the mmc core may try to restore
> > > > > > > > power for a card, especially if it's a removable slot, as to
> > > > > > > > make sure it gets detected if someone inserted a card while
> > > > > > > > the system was
> > > > suspended.
> > > > > > > > Not sure if this plays well with the PCIe/NVMe driver's behaviour.
> > > > > > > > Again, I think some kind of synchronization is needed.
> > > > > > > >
> > > > > > > > > 5.If card is unplugged, RTS5261 will switch to SD mode.
> > > > > > > >
> > > > > > > > Alright, clearly the mmc driver is needed to manage card
> > > > insert/removal.
> > > > > > > >
> > > > > > > > > We should send CMD8 in SD mode to ask card's PCIe
> > > > > > > > > availability, and the
> > > > > > > > order of NVMe driver and rts5261 driver doesn't matter.
> > > > > > > >
> > > > > > > > That assumes there's another synchronization mechanism.
> > > > > > > > Maybe there is, but I don't understand how.
> > > > > > > >
> > > > > > > If no card in RTS5261, RTS5261 works in SD mode. If you run
> > > > > > > command lspci,
> > > > > > you can see the RTS5261 device.
> > > > > >
> > > > > > Right.
> > > > > >
> > > > > > The rtsx_pci_driver (drivers/misc/cardreader/rtsx_pcr.c) has
> > > > > > registered itself as a pci driver and been probed successfully,
> > > > > > I assume. Then during
> > > > > > rtsx_pci_probe() an mfd device is added via mfd_add_devices(),
> > > > > > which corresponds to the rtsx_pci_sdmmc
> > > > > > (drivers/mmc/host/rtsx_pci_sdmmc.c) platform driver.
> > > > > >
> > > > > > > When insert a SD Express card, Mmc driver will send CMD8 to
> > > > > > > ask the card's PCIe availability, because it's a SD EXPRESS
> > > > > > > card,
> > > > > >
> > > > > > Okay, so this will then be a part of the rtsx_pci_sdmmc driver's
> > > > > > probe
> > > > sequence.
> > > > > > Or more exactly, when rtsx_pci_sdmmc_drv_probe() completes
> > > > > > successfully, a mmc rescan work becomes scheduled to try to
> > > > > > detect an SD/MMC card. Then the CMD8 command is sent...
> > > > > >
> > > > > > > RTS5261 will switch to NVMe mode, after switch if you run
> > > > > > > lspci, you can see
> > > > > > RTS5261 disappeared and a NVMe device replaces RTS5261.
> > > > > >
> > > > > > Can you elaborate more exactly how this managed?
> > > > > >
> > > > > > It kind of sounds like the original PCI device is being deleted?
> > > > > > How is this managed?
> > > > > >
> > > > > > In any case, the rtsx_pci_driver's ->remove() callback,
> > > > > > rtsx_pci_remove(), should be invoked, I assume?
> > > > > >
> > > > > > That would then lead to that mfd_remove_devices() gets called,
> > > > > > which makes the ->remove() callback of the rtsx_pci_sdmmc
> > > > > > driver, rtsx_pci_sdmmc_drv_remove(), to be invoked. Correct?
> > > > > >
> > > > > Yes, after RTS5261 switch to NVMe mode, rtsx_pci_remove() and
> > > > rtsx_pci_sdmmc_drv_remove() will be invoked.
> > > >
> > > > So, the ->remove() callbacks are invoked because the PCI device that
> > > > corresponds to the rtsx_pci_driver is being deleted. Can you explain
> > > > who deletes the PCI device and why?
> > > >
> > > > I am not a PCI expert, so apologize for my ignorance - but I really
> > > > want to understand how this is supposed to work.
> > > >
> > > Rtsx host driver sets RTS5261 0xFF54 bit0=1 and 0xFF55 bit4=0, then RTS5261
> > will switch MCU and enter SD EXPRESS mode.
> > > Because hardware design is involved, sorry I can't explain much more details
> > about that.
> >
> > Okay, so somehow that will trigger the PCI bus to remove the corresponding
> > PCI device, I guess.
> >
> > >
> > > > >
> > > > > > > In NVMe mode, RTS5261 only provide a bridge between SD Express
> > > > > > > card and
> > > > > > PCIe. For NVMe driver, just like a new NVMe device is inserted.
> > > > > >
> > > > > > I don't understand what that means, but I am also not an expert
> > > > > > on
> > > > PCI/NVMe.
> > > > > > Care to explain more?
> > > > > >
> > > > > In NVMe mode, SD Express card connect the computer via PCIe.
> > > > > IN SD mode, card connect computer via reader.
> > > >
> > > > That didn't make better sense to me, sorry. I do know about the SD
> > > > spec and the SD-express card protocol parts. Anyway, let's leave this for
> > now.
> > > >
> > > > >
> > > > > > > Mmc core doesn't successfully detect the card and handover to
> > > > > > > NVMe driver. Because of detect the card failed,
> > > > > >
> > > > > > How do you make sure that the rtsx_pci_sdmmc driver is leaving
> > > > > > the card in the correct state for NVMe?
> > > > > >
> > > > > > For example, the mmc core has a loop re-trying with a lower
> > > > > > initialization frequency for the card (400KHz, 300KHz, 200KHz,
> > > > > > 100KHz). This will cause additional requests to the rtsx_pci_sdmmc
> > driver.
> > > > > >
> > > > > > > Mmc driver will request the RTS5261 to power off the card, but
> > > > > > > at that time
> > > > > > power off the card will not succeed.
> > > > > >
> > > > > > Yes, assuming no card was found, the mmc core calls mmc_power_off().
> > > > > > Ths leads to the rtsx_pci_sdmmc driver's ->set_ios() callback
> > > > > > being invoked, requesting the card to be powered off. I don't
> > > > > > see how you are managing this, what am I missing?
> > > > > >
> > > > > Before power off card and re-trying initialization, rtsx driver
> > > > > sets RTS5261
> > > > 0xFF55 bit4=0.
> > > > > After set 0xFF55 bit4=0, RTS5261 can't receive any CMD from PCIe
> > > > > and
> > > > prepare for device disappear.
> > > > > Therefore, MMC driver can't change card status.
> > > >
> > > > Okay, so beyond that point - any calls to the interface that is
> > > > provided from drivers/misc/cardreader/rtsx_pcr will fail, when
> > > > invoked by the rtsx_pci_sdmmc driver?
> > > >
> > > Yes.
> > >
> > > > To me, that sounds a bit fragile and it's also relying on a specific
> > > > behaviour of the RTS5261 card reader interface. I wonder if this
> > > > could be considered as a common behaviour...??
> > > >
> > > It's a feature proposal by realtek not common.
> >
> > Yes, of course.
> >
> > >
> > > > Perhaps it's better to teach the mmc core *more* about SD express cards.
> > > > Maybe add a new host ops for dealing with the specific CMD8 command
> > > > and make the mmc core to "bail out", rather than keep retrying the
> > > > initialization. In principle I think the core should accept that it
> > > > may have found an SD express card, then abort further communication
> > > > with it. At least until the mmc host indicates that a
> > > > re-initialization of the card can be done, which could be through a
> > remove/re-probe, for example.
> > > >
> > > In SD7.x spec host should send CMD8 with bit20=1 and bit21=1 to ask card's
> > PCIe availability.
> > > So the CMD8 is not specific for RTS5261, it's just newly defined in SD7.x spec.
> >
> > Yes, of course.
> >
> > So, there are two PCIe modes. 1.8V I/O (mandatory and corresponds to
> > bit20) and 1.2V I/O (optional and corresponds to bit21). It's important that the
> > mmc host informs the mmc core about it's capabilities, so we can set the
> > correct bits when sending CMD8.
> >
> > What do your host support?
> >
> > > The mmc core will request host to power off card and has a loop
> > > re-trying with different initialization frequency for the card (400KHz, 300KHz,
> > 200KHz, 100KHz), if I don't modify mmc core, I can't stop the power off and
> > re-trying, if I modify mmc core, RTS5261 will become a special case for mmc
> > core.
> > > So make the operation fail is the minimum modification in mmc core for me.
> > Do you have any other suggestion?
> >
> > Along the lines of what I suggested above. I think the mmc core should stop
> > sending commands beyond the CMD8, if the card responds to support PCIe.
> >
> > >
> > > > >
> > > > > > As stated above, I assume you the corresponding platform device
> > > > > > for rtsx_pci_sdmmc being deleted and thus triggering the
> > > > > > rtsx_pci_sdmmc_drv_remove() being called. Correct? If not, how
> > > > > > does the driver manage this?
> > > > > >
> > > > > Yes.
> > > > >
> > > > > > > When the card is unplugged, RTS5261 will switch to SD mode by
> > > > > > > itself and don't need mmc driver to do anything,
> > > > > >
> > > > > > Okay.
> > > > > >
> > > > > > So that means the rtsx_pci_sdmmc driver is being probed again?
> > > > > >
> > > > > Yes.
> > > > >
> > > > > > > If you run lspci, you can see NVMe device disappeared and
> > > > > > > RTS5261 appeared
> > > > > > again.
> > > > > >
> > > > > > I see.
> > > > > >
> > > >
> > > > If you need some help on the mmc core parts, I am willing to help out.
> > > > However, first, I would like to get some better understanding of who
> > > > and why the PCI device is deleted.
> > > >
> > > Can I stop the re-trying in host driver other than modify mmc core?
> >
> > We need to modify the core, but let me try to help in regards to that.
> > I will post some patches within a couple of days and keep you posted.
> >
> > Let's see how this goes.
> >
> Hi
> > > As above, I'm sorry I can't explain much more details about hardware design.
> >
> > Sure, it's okay.
> >
> > Kind regards
> > Uffe

Powered by blists - more mailing lists