[PATCH] Fix broken bus voltage setting in sdhc

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] Fix broken bus voltage setting in sdhc

Ben Pye
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.

Ben.

Index: sys/dev/sdmmc/sdhc.c
===================================================================
RCS file: /cvs/src/sys/dev/sdmmc/sdhc.c,v
retrieving revision 1.61
diff -u -p -u -p -r1.61 sdhc.c
--- sys/dev/sdmmc/sdhc.c 6 Sep 2018 10:15:17 -0000 1.61
+++ sys/dev/sdmmc/sdhc.c 7 Nov 2018 15:36:10 -0000
@@ -53,6 +53,7 @@ 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 */
 
  bus_dmamap_t adma_map;
  bus_dma_segment_t adma_segs[1];
@@ -420,6 +421,8 @@ sdhc_host_reset(sdmmc_chipset_handle_t s
 
  s = splsdmmc();
 
+ hp->vdd = 0;
+
  /* Disable all interrupts. */
  HWRITE2(hp, SDHC_NINTR_SIGNAL_EN, 0);
 
@@ -491,6 +494,16 @@ sdhc_bus_power(sdmmc_chipset_handle_t sc
  int s;
 
  s = splsdmmc();
+
+ /*
+ * If the requested vdd is the same as current vdd return.
+ */
+ if (hp->vdd == ocr) {
+ splx(s);
+ return 0;
+ }
+
+ hp->vdd = ocr;
 
  /*
  * Disable bus power before voltage change.

Reply | Threaded
Open this post in threaded view
|

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

Mark Kettenis
> From: Ben Pye <[hidden email]>
> 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).

Cheers,

Mark

> Index: sys/dev/sdmmc/sdhc.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/sdmmc/sdhc.c,v
> retrieving revision 1.61
> diff -u -p -u -p -r1.61 sdhc.c
> --- sys/dev/sdmmc/sdhc.c 6 Sep 2018 10:15:17 -0000 1.61
> +++ sys/dev/sdmmc/sdhc.c 7 Nov 2018 15:36:10 -0000
> @@ -53,6 +53,7 @@ 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 */
>  
>   bus_dmamap_t adma_map;
>   bus_dma_segment_t adma_segs[1];
> @@ -420,6 +421,8 @@ sdhc_host_reset(sdmmc_chipset_handle_t s
>  
>   s = splsdmmc();
>  
> + hp->vdd = 0;
> +
>   /* Disable all interrupts. */
>   HWRITE2(hp, SDHC_NINTR_SIGNAL_EN, 0);
>  
> @@ -491,6 +494,16 @@ sdhc_bus_power(sdmmc_chipset_handle_t sc
>   int s;
>  
>   s = splsdmmc();
> +
> + /*
> + * If the requested vdd is the same as current vdd return.
> + */
> + if (hp->vdd == ocr) {
> + splx(s);
> + return 0;
> + }
> +
> + hp->vdd = ocr;
>  
>   /*
>   * Disable bus power before voltage change.
>
>

Reply | Threaded
Open this post in threaded view
|

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

Ben Pye
On Wed, Nov 07, 2018 at 05:33:13PM +0100, Mark Kettenis wrote:

> > From: Ben Pye <[hidden email]>
> > 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 am however hitting another (potentially unrelated?) issue having done
this. I am using softraid for FDE and so my physical device is sd0 and
my softraid device is sd1. After resume sd0 is detached and reattached
and appears to still be working, the device name is printed to the dmesg
output. sd1 however stays in a broken state and so eventually the system
crashes as it's root filesystem just disappeared. I have included a
(partial - I have ended up adding a lot of logging to get it coming up
after suspend) dmesg below. Any ideas?

Thanks
Ben

sdmmc0: CID: mid=0x90 oid=0x014a pnm="HBG4e\^E" rev=0x00 psn=0x507b6955 mdt=000
sdmmc0: read_bl_len=512 sector_size=512
scsibus1 at sdmmc0: 2 targets, initiator 0
sd0 at scsibus1 targ 1 lun 0: <SD/MMC, HBG4e\\005, 0000> SCSI2 0/direct removable
sd0: 29824MB, 512 bytes/sector, 61079552 sectors
ugen0 at uhub0 port 3 "Intel Bluetooth" rev 2.00/0.03 addr 2
uvideo0 at uhub0 port 7 configuration 1 interface 0 "Generic HP Truevision HD" rev 2.00/0.06 a
video0 at uvideo0
vscsi0 at root
scsibus2 at vscsi0: 256 targets
softraid0 at root
scsibus3 at softraid0: 256 targets
softraid0: sd1 was not shutdown properly
sd1 at scsibus3 targ 1 lun 0: <OPENBSD, SR CRYPTO, 006> SCSI2 0/direct fixed
sd1: 29823MB, 512 bytes/sector, 61077937 sectors
sdhc0: timeout waiting for 0 (state=1fef0106<CL,D3L,D2L,D1L,WPS,CD,CSS,CI,WTA,DLA,CID>)
softraid0: I/O error 5 on dev 0x400 at block 16
softraid0: could not write metadata to sd0a
root on sd1a (2d11d44f123d9141.a) swap on sd1b dump on sd1b
... (iwm, usb, video, etc messages omitted)
sd0 detached
scsibus1 detached
softraid0: sd1: i/o error 0 @ CRYPTO block 11831120
softraid0: sd1: i/o error 0 @ CRYPTO block 11820400
softraid0: sd1: i/o error 0 @ CRYPTO block 11831120
softraid0: sd1: i/o error 0 @ CRYPTO block 9755216
softraid0: sd1: i/o error 0 @ CRYPTO block 9748640
... (more i/o errors omitted)
sdmmc0: CID: mid=0x90 oid=0x014a pnm="HBG4e\^E" rev=0x00 psn=0x507b6955 mdt=000
sdmmc0: read_bl_len=512 sector_size=512
scsibus1 at sdmmc0: 2 targets, initiator 0
sd0 at scsibus1 targ 1 lun 0: <SD/MMC, HBG4e\\005, 0000> SCSI2 0/direct removable
sd0: 29824MB, 512 bytes/sector, 61079552 sectors
softraid0: sd1: i/o error 0 @ CRYPTO block 15148176
softraid0: sd1: i/o error 0 @ CRYPTO block 15141600
softraid0: sd1: i/o error 0 @ CRYPTO block 9755216
softraid0: sd1: i/o error 0 @ CRYPTO block 9748640
softraid0: sd1: i/o error 0 @ CRYPTO block 8144
softraid0: sd1: i/o error 0 @ CRYPTO block 1568
softraid0: sd1: i/o error 0 @ CRYPTO block 6291568
... (further i/o errors until panic)

>
> Cheers,
>
> Mark
>
> > Index: sys/dev/sdmmc/sdhc.c
> > ===================================================================
> > RCS file: /cvs/src/sys/dev/sdmmc/sdhc.c,v
> > retrieving revision 1.61
> > diff -u -p -u -p -r1.61 sdhc.c
> > --- sys/dev/sdmmc/sdhc.c 6 Sep 2018 10:15:17 -0000 1.61
> > +++ sys/dev/sdmmc/sdhc.c 7 Nov 2018 15:36:10 -0000
> > @@ -53,6 +53,7 @@ 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 */
> >  
> >   bus_dmamap_t adma_map;
> >   bus_dma_segment_t adma_segs[1];
> > @@ -420,6 +421,8 @@ sdhc_host_reset(sdmmc_chipset_handle_t s
> >  
> >   s = splsdmmc();
> >  
> > + hp->vdd = 0;
> > +
> >   /* Disable all interrupts. */
> >   HWRITE2(hp, SDHC_NINTR_SIGNAL_EN, 0);
> >  
> > @@ -491,6 +494,16 @@ sdhc_bus_power(sdmmc_chipset_handle_t sc
> >   int s;
> >  
> >   s = splsdmmc();
> > +
> > + /*
> > + * If the requested vdd is the same as current vdd return.
> > + */
> > + if (hp->vdd == ocr) {
> > + splx(s);
> > + return 0;
> > + }
> > +
> > + hp->vdd = ocr;
> >  
> >   /*
> >   * Disable bus power before voltage change.
> >
> >
>

Reply | Threaded
Open this post in threaded view
|

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 <[hidden email]>
> > > 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();

Reply | Threaded
Open this post in threaded view
|

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

Brandon Mercer-3
On Sat, Nov 10, 2018 at 03:45:41PM +0000, Ben Pye wrote:

> 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 <[hidden email]>
> > > > 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.
>

Thanks for this, Ben. I am running this on my chromebook 13. I've added
the device ids for the mmc to pcidevs as well.

OK?

Index: sys/dev/pci/pcidevs
===================================================================
RCS file: /cvs/src/sys/dev/pci/pcidevs,v
retrieving revision 1.1866
diff -u -p -u -r1.1866 pcidevs
--- sys/dev/pci/pcidevs 8 Nov 2018 06:54:13 -0000 1.1866
+++ sys/dev/pci/pcidevs 10 Nov 2018 17:10:16 -0000
@@ -5108,6 +5108,7 @@ product INTEL 100SERIES_LP_PCIE_12 0x9d1
 product INTEL 100SERIES_LP_PMC 0x9d21 100 Series PMC
 product INTEL 100SERIES_LP_SMB 0x9d23 100 Series SMBus
 product INTEL 100SERIES_LP_SPI 0x9d24 100 Series SPI
+product INTEL 100SERIES_LP_EMMC 0x9d2b 100 Series eMMC
 product INTEL 100SERIES_LP_UART_1 0x9d27 100 Series UART
 product INTEL 100SERIES_LP_UART_2 0x9d28 100 Series UART
 product INTEL 100SERIES_LP_XHCI 0x9d2f 100 Series xHCI
Index: sys/dev/pci/pcidevs.h
===================================================================
RCS file: /cvs/src/sys/dev/pci/pcidevs.h,v
retrieving revision 1.1859
diff -u -p -u -r1.1859 pcidevs.h
--- sys/dev/pci/pcidevs.h 8 Nov 2018 06:55:06 -0000 1.1859
+++ sys/dev/pci/pcidevs.h 10 Nov 2018 17:10:18 -0000
@@ -5113,6 +5113,7 @@
 #define PCI_PRODUCT_INTEL_100SERIES_LP_PMC 0x9d21 /* 100 Series PMC */
 #define PCI_PRODUCT_INTEL_100SERIES_LP_SMB 0x9d23 /* 100 Series SMBus */
 #define PCI_PRODUCT_INTEL_100SERIES_LP_SPI 0x9d24 /* 100 Series SPI */
+#define PCI_PRODUCT_INTEL_100SERIES_LP_EMMC 0x9d2b /* 100 Series eMMC */
 #define PCI_PRODUCT_INTEL_100SERIES_LP_UART_1 0x9d27 /* 100 Series UART */
 #define PCI_PRODUCT_INTEL_100SERIES_LP_UART_2 0x9d28 /* 100 Series UART */
 #define PCI_PRODUCT_INTEL_100SERIES_LP_XHCI 0x9d2f /* 100 Series xHCI */
Index: sys/dev/pci/pcidevs_data.h
===================================================================
RCS file: /cvs/src/sys/dev/pci/pcidevs_data.h,v
retrieving revision 1.1854
diff -u -p -u -r1.1854 pcidevs_data.h
--- sys/dev/pci/pcidevs_data.h 8 Nov 2018 06:55:06 -0000 1.1854
+++ sys/dev/pci/pcidevs_data.h 10 Nov 2018 17:10:19 -0000
@@ -17936,6 +17936,10 @@ static const struct pci_known_product pc
     "100 Series SPI",
  },
  {
+    PCI_VENDOR_INTEL, PCI_PRODUCT_INTEL_100SERIES_LP_EMMC,
+    "100 Series eMMC",
+ },
+ {
     PCI_VENDOR_INTEL, PCI_PRODUCT_INTEL_100SERIES_LP_UART_1,
     "100 Series UART",
  },
Index: sys/dev/sdmmc/sdhc.c
===================================================================
RCS file: /cvs/src/sys/dev/sdmmc/sdhc.c,v
retrieving revision 1.61
diff -u -p -u -r1.61 sdhc.c
--- sys/dev/sdmmc/sdhc.c 6 Sep 2018 10:15:17 -0000 1.61
+++ sys/dev/sdmmc/sdhc.c 10 Nov 2018 17:10:19 -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();
 

Reply | Threaded
Open this post in threaded view
|

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

Jonathan Gray-11
On Sat, Nov 10, 2018 at 01:22:47PM -0500, Brandon Mercer wrote:

> On Sat, Nov 10, 2018 at 03:45:41PM +0000, Ben Pye wrote:
> > 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 <[hidden email]>
> > > > > 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.
> >
>
> Thanks for this, Ben. I am running this on my chromebook 13. I've added
> the device ids for the mmc to pcidevs as well.
>
> OK?
>
> Index: sys/dev/pci/pcidevs
> ===================================================================
> RCS file: /cvs/src/sys/dev/pci/pcidevs,v
> retrieving revision 1.1866
> diff -u -p -u -r1.1866 pcidevs
> --- sys/dev/pci/pcidevs 8 Nov 2018 06:54:13 -0000 1.1866
> +++ sys/dev/pci/pcidevs 10 Nov 2018 17:10:16 -0000
> @@ -5108,6 +5108,7 @@ product INTEL 100SERIES_LP_PCIE_12 0x9d1
>  product INTEL 100SERIES_LP_PMC 0x9d21 100 Series PMC
>  product INTEL 100SERIES_LP_SMB 0x9d23 100 Series SMBus
>  product INTEL 100SERIES_LP_SPI 0x9d24 100 Series SPI
> +product INTEL 100SERIES_LP_EMMC 0x9d2b 100 Series eMMC

This is missing a tab before 0x and should be put after the UART_2
entry to be sorted by device id.

>  product INTEL 100SERIES_LP_UART_1 0x9d27 100 Series UART
>  product INTEL 100SERIES_LP_UART_2 0x9d28 100 Series UART
>  product INTEL 100SERIES_LP_XHCI 0x9d2f 100 Series xHCI
> Index: sys/dev/pci/pcidevs.h
> ===================================================================
> RCS file: /cvs/src/sys/dev/pci/pcidevs.h,v
> retrieving revision 1.1859
> diff -u -p -u -r1.1859 pcidevs.h
> --- sys/dev/pci/pcidevs.h 8 Nov 2018 06:55:06 -0000 1.1859
> +++ sys/dev/pci/pcidevs.h 10 Nov 2018 17:10:18 -0000
> @@ -5113,6 +5113,7 @@
>  #define PCI_PRODUCT_INTEL_100SERIES_LP_PMC 0x9d21 /* 100 Series PMC */
>  #define PCI_PRODUCT_INTEL_100SERIES_LP_SMB 0x9d23 /* 100 Series SMBus */
>  #define PCI_PRODUCT_INTEL_100SERIES_LP_SPI 0x9d24 /* 100 Series SPI */
> +#define PCI_PRODUCT_INTEL_100SERIES_LP_EMMC 0x9d2b /* 100 Series eMMC */
>  #define PCI_PRODUCT_INTEL_100SERIES_LP_UART_1 0x9d27 /* 100 Series UART */
>  #define PCI_PRODUCT_INTEL_100SERIES_LP_UART_2 0x9d28 /* 100 Series UART */
>  #define PCI_PRODUCT_INTEL_100SERIES_LP_XHCI 0x9d2f /* 100 Series xHCI */
> Index: sys/dev/pci/pcidevs_data.h
> ===================================================================
> RCS file: /cvs/src/sys/dev/pci/pcidevs_data.h,v
> retrieving revision 1.1854
> diff -u -p -u -r1.1854 pcidevs_data.h
> --- sys/dev/pci/pcidevs_data.h 8 Nov 2018 06:55:06 -0000 1.1854
> +++ sys/dev/pci/pcidevs_data.h 10 Nov 2018 17:10:19 -0000
> @@ -17936,6 +17936,10 @@ static const struct pci_known_product pc
>      "100 Series SPI",
>   },
>   {
> +    PCI_VENDOR_INTEL, PCI_PRODUCT_INTEL_100SERIES_LP_EMMC,
> +    "100 Series eMMC",
> + },
> + {
>      PCI_VENDOR_INTEL, PCI_PRODUCT_INTEL_100SERIES_LP_UART_1,
>      "100 Series UART",
>   },
> Index: sys/dev/sdmmc/sdhc.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/sdmmc/sdhc.c,v
> retrieving revision 1.61
> diff -u -p -u -r1.61 sdhc.c
> --- sys/dev/sdmmc/sdhc.c 6 Sep 2018 10:15:17 -0000 1.61
> +++ sys/dev/sdmmc/sdhc.c 10 Nov 2018 17:10:19 -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();
>  
>

Reply | Threaded
Open this post in threaded view
|

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

Ben Pye
In reply to this post by Ben Pye
On Sat, Nov 10, 2018 at 03:45:41PM +0000, Ben Pye wrote:

> 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 <[hidden email]>
> > > > 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.
>

As per my recent email to this list, I think that setting the bus power
back after resume is probably the wrong approach to take. For this
reason I'd suggest we don't keep the save_vdd value and call to
sdhc_bus_power after resume. This will have to be handled differently, I
have edited the patch below to reflect this.

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,7 @@ 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 */
>  
>   bus_dmamap_t adma_map;
>   bus_dma_segment_t adma_segs[1];
> @@ -420,6 +421,8 @@ sdhc_host_reset(sdmmc_chipset_handle_t s
>  
>   s = splsdmmc();
>  
> + hp->vdd = 0;
> +
>   /* Disable all interrupts. */
>   HWRITE2(hp, SDHC_NINTR_SIGNAL_EN, 0);
>  
> @@ -491,6 +494,16 @@ 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();
>