From 804a40830e0456e439de53555ba896b982aab323 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Mo=C5=84?= Date: Sat, 9 Jun 2018 08:44:05 +0200 Subject: [PATCH] Fix race conditions and misconfiguration in sdmmc Properly configure pull up and pull down offsets for autocal. Run autocal prior to every transfer. Prevent race conditions in sdmmc_wait_for_event() - make sure the fault handler has highest priority, then the target irq, state conditions and finally the error mask. Do not clear all bits (|=) when acknowledging fault conditions, only acknowledge the fault conditions itself. Enable interrupts before preparing command registers - if sdmmc is fast enough it can actually finish transfer before we enabled the interrupts. Enabling interrupts clears the COMMAND COMPLETE status bit. Temporarily print all the sdmmc messages in stage2 - for yet unknown reason respecting the log level results in some failures. This results in working microsd card in stage2 on my switch with Samsung EVO+ 256GB microsd card. --- fusee/fusee-primary/src/sdmmc.c | 62 +++++++++++++++++----------- fusee/fusee-secondary/src/sdmmc.c | 68 +++++++++++++++++++------------ 2 files changed, 82 insertions(+), 48 deletions(-) diff --git a/fusee/fusee-primary/src/sdmmc.c b/fusee/fusee-primary/src/sdmmc.c index 10495384a..e8dc93346 100644 --- a/fusee/fusee-primary/src/sdmmc.c +++ b/fusee/fusee-primary/src/sdmmc.c @@ -910,32 +910,38 @@ static int sdmmc_set_up_clocking_parameters(struct mmc *mmc, enum sdmmc_bus_volt { // Clear the I/O conditioning constants. mmc->regs->vendor_clock_cntrl &= ~(MMC_CLOCK_TRIM_MASK | MMC_CLOCK_TAP_MASK); - mmc->regs->auto_cal_config &= ~MMC_AUTOCAL_PDPU_CONFIG_MASK; // Per the TRM, set the PADPIPE clock enable. mmc->regs->vendor_clock_cntrl |= MMC_CLOCK_PADPIPE_CLKEN_OVERRIDE; - switch (operating_voltage) { - case MMC_VOLTAGE_1V8: - mmc->regs->auto_cal_config |= MMC_AUTOCAL_PDPU_SDMMC4_1V8; - break; - case MMC_VOLTAGE_3V3: - mmc->regs->auto_cal_config |= MMC_AUTOCAL_PDPU_SDMMC1_3V3; - break; - default: - printk("ERROR: currently no controllers support voltage %d", mmc->operating_voltage); - return EINVAL; - - } - // Set up the I/O conditioning constants used to ensure we have a reliable clock. // Constants above and procedure below from the TRM. switch (mmc->controller) { case SWITCH_EMMC: + if (operating_voltage != MMC_VOLTAGE_1V8) { + mmc_print(mmc, "ERROR: eMMC can only run at 1V8, but mmc struct claims voltage %d", operating_voltage); + return EINVAL; + } + + mmc->regs->auto_cal_config &= ~MMC_AUTOCAL_PDPU_CONFIG_MASK; + mmc->regs->auto_cal_config |= MMC_AUTOCAL_PDPU_SDMMC4_1V8; mmc->regs->vendor_clock_cntrl |= (MMC_CLOCK_TRIM_SDMMC4 | MMC_CLOCK_TAP_SDMMC4); break; case SWITCH_MICROSD: + switch (operating_voltage) { + case MMC_VOLTAGE_1V8: + mmc->regs->auto_cal_config &= ~MMC_AUTOCAL_PDPU_CONFIG_MASK; + mmc->regs->auto_cal_config |= MMC_AUTOCAL_PDPU_SDMMC1_1V8; + break; + case MMC_VOLTAGE_3V3: + mmc->regs->auto_cal_config &= ~MMC_AUTOCAL_PDPU_CONFIG_MASK; + mmc->regs->auto_cal_config |= MMC_AUTOCAL_PDPU_SDMMC1_3V3; + break; + default: + mmc_print(mmc, "ERROR: microsd does not support voltage %d", operating_voltage); + return EINVAL; + } mmc->regs->vendor_clock_cntrl |= (MMC_CLOCK_TRIM_SDMMC1 | MMC_CLOCK_TAP_SDMMC1); break; @@ -1756,6 +1762,7 @@ static int sdmmc_wait_for_event(struct mmc *mmc, uint32_t fault_conditions, fault_handler_t fault_handler) { uint32_t timebase = get_time(); + uint32_t intstatus; int rc; // Wait until we either wind up ready, or until we've timed out. @@ -1763,7 +1770,13 @@ static int sdmmc_wait_for_event(struct mmc *mmc, if (get_time_since(timebase) > mmc->timeout) return ETIMEDOUT; - if (mmc->regs->int_status & fault_conditions) { + // Read intstatus into temporary variable to make sure that the + // priorities are: fault conditions, target irq, errors + // This makes sure that if fault conditions and target irq + // comes nearly at the same time that the fault handler will + // always be called + intstatus = mmc->regs->int_status; + if (intstatus & fault_conditions) { // If we don't have a handler, fault. if (!fault_handler) { @@ -1779,22 +1792,23 @@ static int sdmmc_wait_for_event(struct mmc *mmc, } // Finally, EOI the relevant interrupt. - mmc->regs->int_status |= fault_conditions; + mmc->regs->int_status = fault_conditions; + intstatus &= ~(fault_conditions); // Reset the timebase, so it applies to the next // DMA interval. timebase = get_time(); } - if (mmc->regs->int_status & target_irq) + if (intstatus & target_irq) return 0; if (state_conditions && !(mmc->regs->present_state & state_conditions)) return 0; // If an error occurs, return it. - if (mmc->regs->int_status & MMC_STATUS_ERROR_MASK) - return (mmc->regs->int_status & MMC_STATUS_ERROR_MASK); + if (intstatus & MMC_STATUS_ERROR_MASK) + return (intstatus & MMC_STATUS_ERROR_MASK); } } @@ -2042,7 +2056,7 @@ static void sdmmc_enable_interrupts(struct mmc *mmc, bool enabled) MMC_STATUS_DMA_INTERRUPT | MMC_STATUS_ERROR_MASK; // Clear any pending interrupts. - mmc->regs->int_status |= all_interrupts; + mmc->regs->int_status = all_interrupts; // And enable or disable the pseudo-interrupts. if (enabled) { @@ -2160,6 +2174,8 @@ static int sdmmc_send_command(struct mmc *mmc, enum sdmmc_command command, } } + sdmmc_run_autocal(mmc, true); + // If we have data to send, prepare it. sdmmc_prepare_command_data(mmc, blocks_to_transfer, is_write, auto_terminate, mmc->use_dma, argument); @@ -2168,12 +2184,12 @@ static int sdmmc_send_command(struct mmc *mmc, enum sdmmc_command command, if (blocks_to_transfer && is_write && mmc->use_dma && data_buffer) memcpy(sdmmc_bounce_buffer, (void *)mmc->active_data_buffer, total_data_to_xfer); - // Configure the controller to send the command. - sdmmc_prepare_command_registers(mmc, blocks_to_transfer, command, response_type, checks); - // Ensure we get the status response we want. sdmmc_enable_interrupts(mmc, true); + // Configure the controller to send the command. + sdmmc_prepare_command_registers(mmc, blocks_to_transfer, command, response_type, checks); + // Wait for the command to be completed. rc = sdmmc_wait_for_command_completion(mmc); if (rc) { diff --git a/fusee/fusee-secondary/src/sdmmc.c b/fusee/fusee-secondary/src/sdmmc.c index 10495384a..9dea8e657 100644 --- a/fusee/fusee-secondary/src/sdmmc.c +++ b/fusee/fusee-secondary/src/sdmmc.c @@ -650,8 +650,10 @@ int sdmmc_set_loglevel(int loglevel) static void mmc_vprint(struct mmc *mmc, char *fmt, int required_loglevel, va_list list) { // Allow debug prints to be silenced by a negative loglevel. - if (sdmmc_loglevel < required_loglevel) - return; + //TODO: respect the log level, most likely there are still some timing problems + //which make it not working when the logging is supressed + //if (sdmmc_loglevel < required_loglevel) + // return; printk("%s: ", mmc->name); vprintk(fmt, list); @@ -910,32 +912,38 @@ static int sdmmc_set_up_clocking_parameters(struct mmc *mmc, enum sdmmc_bus_volt { // Clear the I/O conditioning constants. mmc->regs->vendor_clock_cntrl &= ~(MMC_CLOCK_TRIM_MASK | MMC_CLOCK_TAP_MASK); - mmc->regs->auto_cal_config &= ~MMC_AUTOCAL_PDPU_CONFIG_MASK; // Per the TRM, set the PADPIPE clock enable. mmc->regs->vendor_clock_cntrl |= MMC_CLOCK_PADPIPE_CLKEN_OVERRIDE; - switch (operating_voltage) { - case MMC_VOLTAGE_1V8: - mmc->regs->auto_cal_config |= MMC_AUTOCAL_PDPU_SDMMC4_1V8; - break; - case MMC_VOLTAGE_3V3: - mmc->regs->auto_cal_config |= MMC_AUTOCAL_PDPU_SDMMC1_3V3; - break; - default: - printk("ERROR: currently no controllers support voltage %d", mmc->operating_voltage); - return EINVAL; - - } - // Set up the I/O conditioning constants used to ensure we have a reliable clock. // Constants above and procedure below from the TRM. switch (mmc->controller) { case SWITCH_EMMC: + if (operating_voltage != MMC_VOLTAGE_1V8) { + mmc_print(mmc, "ERROR: eMMC can only run at 1V8, but mmc struct claims voltage %d", operating_voltage); + return EINVAL; + } + + mmc->regs->auto_cal_config &= ~MMC_AUTOCAL_PDPU_CONFIG_MASK; + mmc->regs->auto_cal_config |= MMC_AUTOCAL_PDPU_SDMMC4_1V8; mmc->regs->vendor_clock_cntrl |= (MMC_CLOCK_TRIM_SDMMC4 | MMC_CLOCK_TAP_SDMMC4); break; case SWITCH_MICROSD: + switch (operating_voltage) { + case MMC_VOLTAGE_1V8: + mmc->regs->auto_cal_config &= ~MMC_AUTOCAL_PDPU_CONFIG_MASK; + mmc->regs->auto_cal_config |= MMC_AUTOCAL_PDPU_SDMMC1_1V8; + break; + case MMC_VOLTAGE_3V3: + mmc->regs->auto_cal_config &= ~MMC_AUTOCAL_PDPU_CONFIG_MASK; + mmc->regs->auto_cal_config |= MMC_AUTOCAL_PDPU_SDMMC1_3V3; + break; + default: + mmc_print(mmc, "ERROR: microsd does not support voltage %d", operating_voltage); + return EINVAL; + } mmc->regs->vendor_clock_cntrl |= (MMC_CLOCK_TRIM_SDMMC1 | MMC_CLOCK_TAP_SDMMC1); break; @@ -1756,6 +1764,7 @@ static int sdmmc_wait_for_event(struct mmc *mmc, uint32_t fault_conditions, fault_handler_t fault_handler) { uint32_t timebase = get_time(); + uint32_t intstatus; int rc; // Wait until we either wind up ready, or until we've timed out. @@ -1763,7 +1772,13 @@ static int sdmmc_wait_for_event(struct mmc *mmc, if (get_time_since(timebase) > mmc->timeout) return ETIMEDOUT; - if (mmc->regs->int_status & fault_conditions) { + // Read intstatus into temporary variable to make sure that the + // priorities are: fault conditions, target irq, errors + // This makes sure that if fault conditions and target irq + // comes nearly at the same time that the fault handler will + // always be called + intstatus = mmc->regs->int_status; + if (intstatus & fault_conditions) { // If we don't have a handler, fault. if (!fault_handler) { @@ -1779,22 +1794,23 @@ static int sdmmc_wait_for_event(struct mmc *mmc, } // Finally, EOI the relevant interrupt. - mmc->regs->int_status |= fault_conditions; + mmc->regs->int_status = fault_conditions; + intstatus &= ~(fault_conditions); // Reset the timebase, so it applies to the next // DMA interval. timebase = get_time(); } - if (mmc->regs->int_status & target_irq) + if (intstatus & target_irq) return 0; if (state_conditions && !(mmc->regs->present_state & state_conditions)) return 0; // If an error occurs, return it. - if (mmc->regs->int_status & MMC_STATUS_ERROR_MASK) - return (mmc->regs->int_status & MMC_STATUS_ERROR_MASK); + if (intstatus & MMC_STATUS_ERROR_MASK) + return (intstatus & MMC_STATUS_ERROR_MASK); } } @@ -2042,7 +2058,7 @@ static void sdmmc_enable_interrupts(struct mmc *mmc, bool enabled) MMC_STATUS_DMA_INTERRUPT | MMC_STATUS_ERROR_MASK; // Clear any pending interrupts. - mmc->regs->int_status |= all_interrupts; + mmc->regs->int_status = all_interrupts; // And enable or disable the pseudo-interrupts. if (enabled) { @@ -2160,6 +2176,8 @@ static int sdmmc_send_command(struct mmc *mmc, enum sdmmc_command command, } } + sdmmc_run_autocal(mmc, true); + // If we have data to send, prepare it. sdmmc_prepare_command_data(mmc, blocks_to_transfer, is_write, auto_terminate, mmc->use_dma, argument); @@ -2168,12 +2186,12 @@ static int sdmmc_send_command(struct mmc *mmc, enum sdmmc_command command, if (blocks_to_transfer && is_write && mmc->use_dma && data_buffer) memcpy(sdmmc_bounce_buffer, (void *)mmc->active_data_buffer, total_data_to_xfer); - // Configure the controller to send the command. - sdmmc_prepare_command_registers(mmc, blocks_to_transfer, command, response_type, checks); - // Ensure we get the status response we want. sdmmc_enable_interrupts(mmc, true); + // Configure the controller to send the command. + sdmmc_prepare_command_registers(mmc, blocks_to_transfer, command, response_type, checks); + // Wait for the command to be completed. rc = sdmmc_wait_for_command_completion(mmc); if (rc) {