Re: [PATCH 1/3 v2] ARM: dts: rk3288-tinker.dtsi: Fix SD card detection

["Heiko Stübner" <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org>]

Am Montag, 25. Februar 2019, 22:18:28 CET schrieb Doug Anderson:
> Hi,
> On Mon, Feb 25, 2019 at 1:11 PM David Summers
> 
> <beagleboard-8lkWs+DogrrqRUWtDFdff/XRex20P6io@public.gmane.org> wrote:
> > On 25/02/2019 17:13, Doug Anderson wrote:
> > > Hi,
> > > 
> > > On Fri, Feb 22, 2019 at 10:48 AM David Summers
> > > 
> > > <beagleboard-8lkWs+DogrrqRUWtDFdff/XRex20P6io@public.gmane.org> wrote:
> > >> The Problem:
> > >> 
> > >> On ASUS Tinker Board S, when booting from the eMMC, and there is no
> > >> card sd slot, then there are constant errors.
> > >> 
> > >> Cause:
> > >> 
> > >> Thanks must go to Robin Murphy @ ARM for idenifying the problem. The
> > >> rk808 on the Tinker Board and Tinker Board S has many regulators, one
> > >> vccio_sd powers the IO for the sd card. Unfortunatly this is also used
> > >> in the card detect. Now when no card is install, the regulator is
> > >> powered down. This means that the card detect floats, and this means
> > >> random card detection.
> > > 
> > > Yeah, this is broken on a lot of SoCs that use dw_mmc.  :(  Really the
> > > card detect line needs to be on a different rail and this is why all
> > > boards I've worked on recently have a the card detect going to a GPIO
> > > instead of the dw_mmc CD.
> > > 
> > > IIRC Rockchip moved the Card Detect to a different rail on newer SoCs
> > > (like rk3399) but we still used a GPIO even there since we didn't like
> > > the default/automatic muxing of JTAG and SD signals.
> > > 
> > > The one board I was involved in that did it wrong (where we discovered
> > > this issue) was exynos5250-snow.  You can see some discussion about
> > > the issue at:
> > > 
> > > http://lists.infradead.org/pipermail/linux-arm-kernel/2014-August/282474
> > > .html
> > > 
> > > In that discussion I'm pretty sure that Ulf suggested that a better
> > > way to go was to use something like "broken-cd" which I think was
> > > supposed to switch us to use polling.  AKA periodically the SD card
> > > would be powered on and we'd check for a card, then we'd power
> > > everything off.  ...but that was never implemented for snow at least
> > > so there may be something more than just adding the property.  You can
> > > read through the whole thread for more details.
> > > 
> > > 
> > > IIRC leaving the IO rail always on like you're proposing can also work
> > > OK but there may be some corner cases, especially if you are trying to
> > > reach UHS speeds and/or if the bootloader ever tries to use UHS
> > > speeds.  It's almost certainly busted if the bootloader did UHS since
> > > it will leave the line at ~1.8 V and the kernel will expect it to be
> > > at ~3.3 V.  ...but maybe you rely on the bootloader not doing UHS and
> > > maybe things are generally OK if not?  There may also be cases where
> > > you can't properly power down / reset a card because the card may be
> > > drawing power through the IO lines when you power off its main lines.
> > > That's not good for the card and can also put it in a bad state.  I
> > > haven't done all the research here so this may be a bit of FUD--it's
> > > just a vague recollection from many years ago.
> > > 
> > > 
> > > ...so to make a long story short, a better solution is to allow the IO
> > > lines to be powered off but then poll for the card periodically.
> > > 
> > >> The Solution:
> > >> 
> > >> Make sure that the sd IO is always powered, this means card detection
> > >> is always active, which is what should be done on a board with an sd
> > >> slot, which both the Tinker Board and Tinker Board S are. Hence change
> > >> is made to the .dtsi which takes effect on all Tinker Boards as
> > >> required.
> > >> 
> > >> The change also adds "regulator-boot-on" the Tinker Board boot from
> > >> uboot, and the sd card is always one option. Hence the IO must be
> > >> powered in uboot, and so setting this flag.
> > >> 
> > >> Also removed is "disable-wp" the micro sd card which are used have no
> > >> write  protection, so the concept doesn't mean anything, and the
> > >> Tinker Boards work without this. Hence it is removed to simply.
> > > 
> > > As others have said, please leave disable-wp.  There's no way for the
> > > kernel to know if you have a SD or uSD slot and the only difference
> > > between the two (electrically) is that there's no write protect for
> > > micro SD.
> > > 
> > > 
> > > Also: please CC dw_mmc people on future patches in this area.
> > > 
> > > $ ./scripts/get_maintainer.pl -f drivers/mmc/host/dw_mmc.c
> > > Jaehoon Chung <jh80.chung-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> (maintainer:SYNOPSYS DESIGNWARE
> > > MMC/SD/SDIO DRIVER)
> > > Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> (maintainer:MULTIMEDIA CARD
> > > (MMC), SECURE DIGITAL (SD) AND...)
> > > linux-mmc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org (open list:SYNOPSYS DESIGNWARE MMC/SD/SDIO
> > > DRIVER)
> > > 
> > > -Doug
> > 
> > I think the possible problem is that without this we were getting a lot
> > of errors. Now as the errors happen when the sd io is power down, and so
> > CD floats; then the IO will be powered back up gain - to access the
> > card, only to find no card.
> 
> I definitely haven't thought through all the consequences of adding
> polling.  ...but given that the problem is really common with SoCs
> using dw_mmc it's probably worth it to figure out out something sane.
> In theory you could have some code that knows that the card detect
> becomes reliable once the IOs are powered on...
> 
> > So this means the power line goes up and down a lot. Now if we have
> > broken-cd, and polling has to be used, doesn't this also have to power
> > up the IO so it can poll, and the poll puts a bit more load on the
> > processor.
> > 
> > So question is which is better? To keep the IO powered up, or to have it
> > going up and down?
> > 
> > Anyway I'm happy with either solution. So if we can agree which is best,
> > I'll do the patch for that.
> 
> I don't know which is better.  ...but I wouldn't expect that turning
> on regulators and checking a GPIO ever second or so would burn much
> power.

and should also save actual power if the regulator isn't running all
the time :-)