> Date: Sun, 11 Nov 2018 13:10:19 +0000 > From: Ben Pye <b...@curlybracket.co.uk> > > Hey all, > > In my quest for better OpenBSD support on my Chromebook 13 I have found > that sdmmc(4)'s current strategy for suspend/resume only really works > for removable storage. Upon a suspend it marks the device as dying, and > upon resume will detach the card and re-scan the bus. This makes sense > for removable SD cards as they may be removed whilst the device is > asleep, however for non-removable eMMC we can make the assumption that > the device will not be removed during sleep. > > By assuming the device stays present over a sleep we can avoid the > detach and re-scan behaviour that sdmmc(4) currently uses. This is > important if you root storage device is an eMMC device /and/ you are > using softraid(4) as softraid(4) will not handle the detach/attach. > Despite this on my hardware at least suspend/resume works more reliably > if we can avoid this detach behaviour too, on this device we would > really have to detach before we enter sleep as the bus can break during > suspend and then the detach/reattach fails after resume meaning that my > sd0 device just disappears. > > Instead for non-removable devices upon sleep we can deselect the card > but otherwise leave it configured. On resume we still perform a host > reset in sdhc(4) and then we can re-do the device initialisation in > sdmmc(4). During initialisation we use some of the values determined in > the initial sdmmc(4) init as the goal is to put the device into the same > state. We also hook up to the same sdmmc_function's etc so that the > child devices (scsibus etc) do not have to do anything special on a > resume. With this behaviour suspend/resume works reliably on my HP > Chromebook 13 and softraid for encryption continues to work. > > There are a few unresolved questions however and that's why I'm posting > this here now: > > 1. How do we know if a device is non-removable, I don't think an MMC > device indicates this, maybe we have to determine this by the > controller?
The hardware really doesn't know. It all depends how the controller is actually wired up. You have to rely on the system firmware (e.g. BIOS) to tell you. For ACPI systems the SD device namespace is supposed to include a _RMV method that indicates whether the device is removeable or not: https://docs.microsoft.com/en-us/windows-hardware/drivers/bringup/other-acpi-namespace-objects For OpenFirmware/FDT systems there is a "non-removable" property. Of course firmware can be buggy and lie to you... > 2. There are some error cases in sdmmc_mem_scan that are unhandled. They > shouldn't ever be hit, if they were it would indicate that something to > do with our non-removable card has changed over a suspend/resume cycle. > > 3. I appear to hit an issue with some commands failing after a suspend > if I don't add a small delay to sdhc_wait_intr_cold, I haven't yet been > able to work out why this is. Specifically, the second time we read > EXT_CSD in sdmmc_mem_mmc_init fails. I believe that eMMC actually require a magic power-on sequence to fully reset themselves. Another posibility is that the bus width isn't properly reset. After a power-cycle the eMMC device should be operating in 1-bit mode again, but the sdmmc stack might think it is still operating in 4-bit or 8-bit mode. > In it's current state this patch almost certainly breaks the > suspend/resume behaviour for non-removable devices, this should be > easily resolved once we can determine which sdmmc(4) devices are > removable. > > Ben. > > > Index: sys/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 > --- sys/dev/sdmmc/sdhc.c 6 Sep 2018 10:15:17 -0000 1.61 > +++ sys/dev/sdmmc/sdhc.c 11 Nov 2018 12:54:29 -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]; > @@ -373,8 +374,13 @@ 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); > - for (i = 0; i < sizeof hp->regs; i++) > - HWRITE1(hp, i, hp->regs[i]); > + > + /* > + * XXX: We might still want this for non-removable > + * devices? > + */ > + //for (i = 0; i < sizeof hp->regs; i++) > + // HWRITE1(hp, i, hp->regs[i]); > } > rv = config_activate_children(self, act); > break; > @@ -420,6 +426,8 @@ sdhc_host_reset(sdmmc_chipset_handle_t s > > s = splsdmmc(); > > + hp->vdd = 0; > + > /* Disable all interrupts. */ > HWRITE2(hp, SDHC_NINTR_SIGNAL_EN, 0); > > @@ -490,6 +498,14 @@ sdhc_bus_power(sdmmc_chipset_handle_t sc > 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(); > > /* > @@ -1104,6 +1120,9 @@ sdhc_wait_intr_cold(struct sdhc_host *hp > { > int status; > > + /* XXX: Need this delay for some commands after suspend, why? */ > + delay(10); > + > mask |= SDHC_ERROR_INTERRUPT; > timo = timo * tick; > status = hp->intr_status; > @@ -1283,6 +1302,8 @@ sdhc_dump_regs(struct sdhc_host *hp) > HREAD4(hp, SDHC_PRESENT_STATE), SDHC_PRESENT_STATE_BITS); > printf("0x%02x POWER_CTL: %x\n", SDHC_POWER_CTL, > HREAD1(hp, SDHC_POWER_CTL)); > + printf("0x%02x CLOCK_CTL: %x\n", SDHC_CLOCK_CTL, > + HREAD2(hp, SDHC_CLOCK_CTL)); > printf("0x%02x NINTR_STATUS: %x\n", SDHC_NINTR_STATUS, > HREAD2(hp, SDHC_NINTR_STATUS)); > printf("0x%02x EINTR_STATUS: %x\n", SDHC_EINTR_STATUS, > Index: sys/dev/sdmmc/sdmmc.c > =================================================================== > RCS file: /cvs/src/sys/dev/sdmmc/sdmmc.c,v > retrieving revision 1.51 > diff -u -p -r1.51 sdmmc.c > --- sys/dev/sdmmc/sdmmc.c 9 Aug 2018 13:52:36 -0000 1.51 > +++ sys/dev/sdmmc/sdmmc.c 11 Nov 2018 12:54:29 -0000 > @@ -58,6 +58,8 @@ void sdmmc_card_attach(struct sdmmc_soft > void sdmmc_card_detach(struct sdmmc_softc *, int); > int sdmmc_enable(struct sdmmc_softc *); > void sdmmc_disable(struct sdmmc_softc *); > +void sdmmc_suspend(struct sdmmc_softc *); > +void sdmmc_resume(struct sdmmc_softc *); > int sdmmc_scan(struct sdmmc_softc *); > int sdmmc_init(struct sdmmc_softc *); > #ifdef SDMMC_IOCTL > @@ -178,11 +180,19 @@ sdmmc_activate(struct device *self, int > switch (act) { > case DVACT_SUSPEND: > rv = config_activate_children(self, act); > + > + /* XXX: This should be used for removable devices only */ > /* If card in slot, cause a detach/re-attach */ > - if (ISSET(sc->sc_flags, SMF_CARD_PRESENT)) > - sc->sc_dying = -1; > + /*if (ISSET(sc->sc_flags, SMF_CARD_PRESENT)) > + sc->sc_dying = -1;*/ > + > + /* XXX; This should be used for non removable devices only */ > + sdmmc_suspend(sc); > break; > case DVACT_RESUME: > + /* XXX: This should only be for non-removable */ > + sdmmc_resume(sc); > + > rv = config_activate_children(self, act); > wakeup(&sc->sc_tskq); > break; > @@ -438,7 +448,7 @@ sdmmc_enable(struct sdmmc_softc *sc) > > /* Initialize SD/MMC memory card(s). */ > if (ISSET(sc->sc_flags, SMF_MEM_MODE) && > - (error = sdmmc_mem_enable(sc)) != 0) > + (error = sdmmc_mem_enable(sc, 0)) != 0) > goto err; > > err: > @@ -449,6 +459,63 @@ sdmmc_enable(struct sdmmc_softc *sc) > } > > void > +sdmmc_resume(struct sdmmc_softc *sc) > +{ > + u_int32_t host_ocr; > + int error; > + > + rw_enter_write(&sc->sc_lock); > + > + /* > + * Calculate the equivalent of the card OCR from the host > + * capabilities and select the maximum supported bus voltage. > + */ > + host_ocr = sdmmc_chip_host_ocr(sc->sct, sc->sch); > + error = sdmmc_chip_bus_power(sc->sct, sc->sch, host_ocr); > + if (error != 0) { > + printf("%s: can't supply bus power\n", DEVNAME(sc)); > + goto err; > + } > + > + /* > + * Select the minimum clock frequency. > + */ > + error = sdmmc_chip_bus_clock(sc->sct, sc->sch, > + SDMMC_SDCLK_400KHZ, SDMMC_TIMING_LEGACY); > + if (error != 0) { > + printf("%s: can't supply clock\n", DEVNAME(sc)); > + goto err; > + } > + > + /* XXX wait for card to power up */ > + sdmmc_delay(250000); > + > + /* We only support resuming memory cards */ > + if (ISSET(sc->sc_flags, SMF_MEM_MODE)) { > + if ((error = sdmmc_mem_enable(sc, 1)) != 0) > + goto err; > + > + sdmmc_mem_scan(sc, 1); > + } > + > + sdmmc_init(sc); > + > +err: > + if (error != 0) > + sdmmc_disable(sc); > + > + rw_exit(&sc->sc_lock); > +} > + > +void > +sdmmc_suspend(struct sdmmc_softc *sc) > +{ > + rw_enter_write(&sc->sc_lock); > + (void)sdmmc_select_card(sc, NULL); > + rw_exit(&sc->sc_lock); > +} > + > +void > sdmmc_disable(struct sdmmc_softc *sc) > { > /* XXX complete commands if card is still present. */ > @@ -530,7 +597,7 @@ sdmmc_scan(struct sdmmc_softc *sc) > > /* Scan for memory cards on the bus. */ > if (ISSET(sc->sc_flags, SMF_MEM_MODE)) > - sdmmc_mem_scan(sc); > + sdmmc_mem_scan(sc, 0); > > /* There should be at least one function now. */ > if (SIMPLEQ_EMPTY(&sc->sf_head)) { > Index: sys/dev/sdmmc/sdmmc_mem.c > =================================================================== > RCS file: /cvs/src/sys/dev/sdmmc/sdmmc_mem.c,v > retrieving revision 1.33 > diff -u -p -r1.33 sdmmc_mem.c > --- sys/dev/sdmmc/sdmmc_mem.c 4 Jun 2018 13:33:10 -0000 1.33 > +++ sys/dev/sdmmc/sdmmc_mem.c 11 Nov 2018 12:54:29 -0000 > @@ -74,62 +74,62 @@ int sdmmc_mem_write_block_subr(struct sd > * Initialize SD/MMC memory cards and memory in SDIO "combo" cards. > */ > int > -sdmmc_mem_enable(struct sdmmc_softc *sc) > +sdmmc_mem_enable(struct sdmmc_softc *sc, int oldcard) > { > - u_int32_t host_ocr; > u_int32_t card_ocr; > > rw_assert_wrlock(&sc->sc_lock); > > - /* Set host mode to SD "combo" card or SD memory-only. */ > - SET(sc->sc_flags, SMF_SD_MODE|SMF_MEM_MODE); > + if (!oldcard) { > + /* Set host mode to SD "combo" card or SD memory-only. */ > + SET(sc->sc_flags, SMF_SD_MODE|SMF_MEM_MODE); > > - /* Reset memory (*must* do that before CMD55 or CMD1). */ > - sdmmc_go_idle_state(sc); > + /* Reset memory (*must* do that before CMD55 or CMD1). */ > + sdmmc_go_idle_state(sc); > > - /* > - * Read the SD/MMC memory OCR value by issuing CMD55 followed > - * by ACMD41 to read the OCR value from memory-only SD cards. > - * MMC cards will not respond to CMD55 or ACMD41 and this is > - * how we distinguish them from SD cards. > - */ > + /* > + * Read the SD/MMC memory OCR value by issuing CMD55 followed > + * by ACMD41 to read the OCR value from memory-only SD cards. > + * MMC cards will not respond to CMD55 or ACMD41 and this is > + * how we distinguish them from SD cards. > + */ > mmc_mode: > - if (sdmmc_mem_send_op_cond(sc, 0, &card_ocr) != 0) { > - if (ISSET(sc->sc_flags, SMF_SD_MODE) && > - !ISSET(sc->sc_flags, SMF_IO_MODE)) { > - /* Not a SD card, switch to MMC mode. */ > - CLR(sc->sc_flags, SMF_SD_MODE); > - goto mmc_mode; > + if (sdmmc_mem_send_op_cond(sc, 0, &card_ocr) != 0) { > + if (ISSET(sc->sc_flags, SMF_SD_MODE) && > + !ISSET(sc->sc_flags, SMF_IO_MODE)) { > + /* Not a SD card, switch to MMC mode. */ > + CLR(sc->sc_flags, SMF_SD_MODE); > + goto mmc_mode; > + } > + if (!ISSET(sc->sc_flags, SMF_SD_MODE)) { > + DPRINTF(("%s: can't read memory OCR\n", > + DEVNAME(sc))); > + return 1; > + } else { > + /* Not a "combo" card. */ > + CLR(sc->sc_flags, SMF_MEM_MODE); > + return 0; > + } > } > - if (!ISSET(sc->sc_flags, SMF_SD_MODE)) { > - DPRINTF(("%s: can't read memory OCR\n", > + > + /* Set the lowest voltage supported by the card and host. */ > + sc->host_ocr = sdmmc_chip_host_ocr(sc->sct, sc->sch); > + if (sdmmc_set_bus_power(sc, sc->host_ocr, card_ocr) != 0) { > + DPRINTF(("%s: can't supply voltage requested by card\n", > DEVNAME(sc))); > return 1; > - } else { > - /* Not a "combo" card. */ > - CLR(sc->sc_flags, SMF_MEM_MODE); > - return 0; > } > - } > > - /* Set the lowest voltage supported by the card and host. */ > - host_ocr = sdmmc_chip_host_ocr(sc->sct, sc->sch); > - if (sdmmc_set_bus_power(sc, host_ocr, card_ocr) != 0) { > - DPRINTF(("%s: can't supply voltage requested by card\n", > - DEVNAME(sc))); > - return 1; > + sc->host_ocr &= card_ocr; /* only allow the common voltages */ > + > + if (sdmmc_send_if_cond(sc, card_ocr) == 0) > + sc->host_ocr |= SD_OCR_SDHC_CAP; > } > > - /* Tell the card(s) to enter the idle state (again). */ > sdmmc_go_idle_state(sc); > > - host_ocr &= card_ocr; /* only allow the common voltages */ > - > - if (sdmmc_send_if_cond(sc, card_ocr) == 0) > - host_ocr |= SD_OCR_SDHC_CAP; > - > /* Send the new OCR value until all cards are ready. */ > - if (sdmmc_mem_send_op_cond(sc, host_ocr, NULL) != 0) { > + if (sdmmc_mem_send_op_cond(sc, sc->host_ocr, NULL) != 0) { > DPRINTF(("%s: can't send memory OCR\n", DEVNAME(sc))); > return 1; > } > @@ -141,7 +141,7 @@ sdmmc_mem_enable(struct sdmmc_softc *sc) > * relative card address (RCA). CMD2 is ignored by SDIO-only cards. > */ > void > -sdmmc_mem_scan(struct sdmmc_softc *sc) > +sdmmc_mem_scan(struct sdmmc_softc *sc, int oldcard) > { > struct sdmmc_command cmd; > struct sdmmc_function *sf; > @@ -151,74 +151,96 @@ sdmmc_mem_scan(struct sdmmc_softc *sc) > > rw_assert_wrlock(&sc->sc_lock); > > - /* > - * CMD2 is a broadcast command understood by SD cards and MMC > - * cards. All cards begin to respond to the command, but back > - * off if another card drives the CMD line to a different level. > - * Only one card will get its entire response through. That > - * card remains silent once it has been assigned a RCA. > - */ > - for (i = 0; i < 100; i++) { > - bzero(&cmd, sizeof cmd); > - cmd.c_opcode = MMC_ALL_SEND_CID; > - cmd.c_flags = SCF_CMD_BCR | SCF_RSP_R2; > - > - error = sdmmc_mmc_command(sc, &cmd); > - if (error == ETIMEDOUT) { > - /* No more cards there. */ > - break; > - } else if (error != 0) { > - DPRINTF(("%s: can't read CID\n", DEVNAME(sc))); > - break; > - } > - > - /* In MMC mode, find the next available RCA. */ > - next_rca = 1; > - if (!ISSET(sc->sc_flags, SMF_SD_MODE)) > - SIMPLEQ_FOREACH(sf, &sc->sf_head, sf_list) > - next_rca++; > - > - /* Allocate a sdmmc_function structure. */ > - sf = sdmmc_function_alloc(sc); > - sf->rca = next_rca; > - > + if (!oldcard) { > /* > - * Remember the CID returned in the CMD2 response for > - * later decoding. > + * CMD2 is a broadcast command understood by SD cards and MMC > + * cards. All cards begin to respond to the command, but back > + * off if another card drives the CMD line to a different level. > + * Only one card will get its entire response through. That > + * card remains silent once it has been assigned a RCA. > */ > - bcopy(cmd.c_resp, sf->raw_cid, sizeof sf->raw_cid); > - > - /* > - * Silence the card by assigning it a unique RCA, or > - * querying it for its RCA in the case of SD. > - */ > - if (sdmmc_set_relative_addr(sc, sf) != 0) { > - printf("%s: can't set mem RCA\n", DEVNAME(sc)); > - sdmmc_function_free(sf); > - break; > - } > + for (i = 0; i < 100; i++) { > + bzero(&cmd, sizeof cmd); > + cmd.c_opcode = MMC_ALL_SEND_CID; > + cmd.c_flags = SCF_CMD_BCR | SCF_RSP_R2; > + > + error = sdmmc_mmc_command(sc, &cmd); > + if (error == ETIMEDOUT) { > + /* No more cards there. */ > + break; > + } else if (error != 0) { > + DPRINTF(("%s: can't read CID\n", DEVNAME(sc))); > + break; > + } > + > + /* In MMC mode, find the next available RCA. */ > + next_rca = 1; > + if (!ISSET(sc->sc_flags, SMF_SD_MODE)) > + SIMPLEQ_FOREACH(sf, &sc->sf_head, sf_list) > + next_rca++; > + > + /* Allocate a sdmmc_function structure. */ > + sf = sdmmc_function_alloc(sc); > + sf->rca = next_rca; > + > + /* > + * Remember the CID returned in the CMD2 response for > + * later decoding. > + */ > + bcopy(cmd.c_resp, sf->raw_cid, sizeof sf->raw_cid); > + > + /* > + * Silence the card by assigning it a unique RCA, or > + * querying it for its RCA in the case of SD. > + */ > + if (sdmmc_set_relative_addr(sc, sf) != 0) { > + printf("%s: can't set mem RCA\n", DEVNAME(sc)); > + sdmmc_function_free(sf); > + break; > + } > > #if 0 > - /* Verify that the RCA has been set by selecting the card. */ > - if (sdmmc_select_card(sc, sf) != 0) { > - printf("%s: can't select mem RCA %d\n", > - DEVNAME(sc), sf->rca); > - sdmmc_function_free(sf); > - break; > - } > + /* Verify that the RCA has been set by selecting the > card. */ > + if (sdmmc_select_card(sc, sf) != 0) { > + printf("%s: can't select mem RCA %d\n", > + DEVNAME(sc), sf->rca); > + sdmmc_function_free(sf); > + break; > + } > > - /* Deselect. */ > - (void)sdmmc_select_card(sc, NULL); > + /* Deselect. */ > + (void)sdmmc_select_card(sc, NULL); > #endif > > - /* > - * If this is a memory-only card, the card responding > - * first becomes an alias for SDIO function 0. > - */ > - if (sc->sc_fn0 == NULL) > - sc->sc_fn0 = sf; > + /* > + * If this is a memory-only card, the card responding > + * first becomes an alias for SDIO function 0. > + */ > + if (sc->sc_fn0 == NULL) > + sc->sc_fn0 = sf; > > - SIMPLEQ_INSERT_TAIL(&sc->sf_head, sf, sf_list); > + SIMPLEQ_INSERT_TAIL(&sc->sf_head, sf, sf_list); > + } > + } else { > + SIMPLEQ_FOREACH(sf, &sc->sf_head, sf_list) { > + bzero(&cmd, sizeof cmd); > + cmd.c_opcode = MMC_ALL_SEND_CID; > + cmd.c_flags = SCF_CMD_BCR | SCF_RSP_R2; > + > + error = sdmmc_mmc_command(sc, &cmd); > + if (error == ETIMEDOUT) { > + /* XXX: This shouldn't happen, card has gone. */ > + break; > + } else if (error != 0) { > + DPRINTF(("%s: can't read CID\n", DEVNAME(sc))); > + break; > + } > + if (sdmmc_set_relative_addr(sc, sf) != 0) { > + printf("%s: can't set mem RCA\n", DEVNAME(sc)); > + /* XXX: What do we do now? */ > + break; > + } > + } > } > > /* > Index: sys/dev/sdmmc/sdmmcvar.h > =================================================================== > RCS file: /cvs/src/sys/dev/sdmmc/sdmmcvar.h,v > retrieving revision 1.30 > diff -u -p -r1.30 sdmmcvar.h > --- sys/dev/sdmmc/sdmmcvar.h 9 Aug 2018 13:52:36 -0000 1.30 > +++ sys/dev/sdmmc/sdmmcvar.h 11 Nov 2018 12:54:29 -0000 > @@ -215,6 +215,7 @@ struct sdmmc_softc { > TAILQ_HEAD(, sdmmc_intr_handler) sc_intrq; /* interrupt handlers */ > long sc_max_xfer; /* maximum transfer size */ > void *sc_cookies[SDMMC_MAX_FUNCTIONS]; /* pass extra info from bus to > dev */ > + uint32_t host_ocr; > }; > > /* > @@ -276,8 +277,8 @@ int sdmmc_read_cis(struct sdmmc_function > void sdmmc_print_cis(struct sdmmc_function *); > void sdmmc_check_cis_quirks(struct sdmmc_function *); > > -int sdmmc_mem_enable(struct sdmmc_softc *); > -void sdmmc_mem_scan(struct sdmmc_softc *); > +int sdmmc_mem_enable(struct sdmmc_softc *, int); > +void sdmmc_mem_scan(struct sdmmc_softc *, int); > int sdmmc_mem_init(struct sdmmc_softc *, struct sdmmc_function *); > int sdmmc_mem_read_block(struct sdmmc_function *, int, u_char *, size_t); > int sdmmc_mem_write_block(struct sdmmc_function *, int, u_char *, size_t); > >
