Re: [PATCH] Fix broken bus voltage setting in sdhc

[Ben Pye]

On Sat, Nov 10, 2018 at 06:30:37AM +0000, Ben Pye wrote:
> On Wed, Nov 07, 2018 at 05:33:13PM +0100, Mark Kettenis wrote:
> > > From: Ben Pye <b...@curlybracket.co.uk>
> > > Content-Type: text/plain; charset="utf-8"
> > > 
> > > I have been attempting to run OpenBSD on my HP Chromebook 13, it's a
> > > Skylake device with eMMC storage. Previously sdhc attempted to set the
> > > same bus voltage multiple times, and after the first, successful,
> > > attempt it would break resulting in all later commands timing out. This
> > > patch changes sdhc such that it only sets the voltage if the request is
> > > for a different level, this is the behaviour FreeBSD has.
> > 
> > That makes sense.  We'll need to test this on more hardware.  And
> > maybe we need to reset hp->vdd in some places (suspend/resume, resets).
> 
> As you suspected suspend/resume isn't working with this current patch. At
> least for my hardware I need to restore the bus_power setting on resume
> in addition to the other registers. This seems to get the eMMC device
> back up.

I have included the new patch to get the device attached after suspend
below. I believe if you are not using softraid over this device then
this should be sufficient to get working suspend/resume. Unfortunately
softraid does not handle the detach and reattach of sd0, though I am now
convinced that it's really a seperate issue.

Ben.

Index: dev/sdmmc/sdhc.c
===================================================================
RCS file: /cvs/src/sys/dev/sdmmc/sdhc.c,v
retrieving revision 1.61
diff -u -p -r1.61 sdhc.c
--- dev/sdmmc/sdhc.c    6 Sep 2018 10:15:17 -0000       1.61
+++ dev/sdmmc/sdhc.c    10 Nov 2018 15:40:40 -0000
@@ -53,6 +53,8 @@ struct sdhc_host {
        u_int8_t regs[14];              /* host controller state */
        u_int16_t intr_status;          /* soft interrupt status */
        u_int16_t intr_error_status;    /* soft error status */
+       u_int8_t vdd;                   /* current vdd */
+       u_int8_t save_vdd;              /* vdd before suspend */
 
        bus_dmamap_t adma_map;
        bus_dma_segment_t adma_segs[1];
@@ -364,6 +366,7 @@ sdhc_activate(struct device *self, int a
                /* Save the host controller state. */
                for (n = 0; n < sc->sc_nhosts; n++) {
                        hp = sc->sc_host[n];
+                       hp->save_vdd = hp->vdd;
                        for (i = 0; i < sizeof hp->regs; i++)
                                hp->regs[i] = HREAD1(hp, i);
                }
@@ -373,6 +376,7 @@ sdhc_activate(struct device *self, int a
                for (n = 0; n < sc->sc_nhosts; n++) {
                        hp = sc->sc_host[n];
                        (void)sdhc_host_reset(hp);
+                       sdhc_bus_power(hp, hp->save_vdd);
                        for (i = 0; i < sizeof hp->regs; i++)
                                HWRITE1(hp, i, hp->regs[i]);
                }
@@ -420,6 +424,8 @@ sdhc_host_reset(sdmmc_chipset_handle_t s
 
        s = splsdmmc();
 
+       hp->vdd = 0;
+
        /* Disable all interrupts. */
        HWRITE2(hp, SDHC_NINTR_SIGNAL_EN, 0);
 
@@ -489,6 +495,14 @@ sdhc_bus_power(sdmmc_chipset_handle_t sc
        struct sdhc_host *hp = sch;
        u_int8_t vdd;
        int s;
+
+       /*
+        * If the requested vdd is the same as current vdd return.
+        */
+       if (hp->vdd == ocr)
+               return 0;
+
+       hp->vdd = ocr;
 
        s = splsdmmc();