blk/mmc: miscellaneous bug fixes

blk/components/partitioning.c

@@ -98,7 +98,7 @@ static bool gpt_partitions_init()
         blk_storage_info_t *driver_storage_info = config.driver.conn.storage_info.vaddr;
         client_storage_info->sector_size = driver_storage_info->sector_size;
         client_storage_info->capacity = clients[i].sectors / (BLK_TRANSFER_SIZE / MSDOS_MBR_SECTOR_SIZE);
-        client_storage_info->read_only = false;
+        client_storage_info->read_only = driver_storage_info->read_only;
         __atomic_store_n(&client_storage_info->ready, true, __ATOMIC_RELEASE);
     }
 
@@ -331,7 +331,7 @@ static bool mbr_handle_response()
     cache_clean_and_invalidate(mbr_state.req_addr, mbr_state.req_addr + (BLK_TRANSFER_SIZE * mbr_req_count));
     sddf_memcpy(&msdos_mbr, (void *)mbr_state.req_addr, sizeof(struct msdos_mbr));
 
-    /* There is only one partition entry in Protective MBR of the GPT parition schema */
+    /* There is only one partition entry in Protective MBR of the GPT partition schema */
     if (msdos_mbr.partitions[0].type == MSDOS_MBR_PARTITION_TYPE_GPT) {
         LOG_BLK_VIRT("Protective MBR of GPT is detected\n");
 

blk/components/virt.c

@@ -29,13 +29,8 @@ __attribute__((__section__(".blk_virt_config"))) blk_virt_config_t config;
 
 /* Driver queue handle */
 blk_queue_handle_t drv_h;
-
-/* Client specific info */
-typedef struct client {
-    blk_queue_handle_t queue_h;
-    uintptr_t data_paddr;
-} client_t;
-client_t clients[SDDF_BLK_MAX_CLIENTS];
+/* Client queue handles */
+blk_queue_handle_t client_queues[SDDF_BLK_MAX_CLIENTS];
 
 /* Request info to be bookkept from client */
 typedef struct reqbk {
@@ -66,7 +61,7 @@ void init(void)
         blk_req_queue_t *curr_req = client->conn.req_queue.vaddr;
         blk_resp_queue_t *curr_resp = client->conn.resp_queue.vaddr;
         uint32_t queue_capacity = client->conn.num_buffers;
-        blk_queue_init(&clients[i].queue_h, curr_req, curr_resp, queue_capacity);
+        blk_queue_init(&client_queues[i], curr_req, curr_resp, queue_capacity);
     }
 
     /* Initialise driver queue */
@@ -118,12 +113,12 @@ static void handle_driver()
             assert(false);
         }
 
-        blk_queue_handle_t h = clients[reqbk.cli_id].queue_h;
+        blk_queue_handle_t *h = &client_queues[reqbk.cli_id];
 
         /* Response queue should never be full since number of inflight requests (ialloc size)
          * should always be less than or equal to resp queue capacity.
          */
-        err = blk_enqueue_resp(&h, drv_status, drv_success_count, reqbk.cli_req_id);
+        err = blk_enqueue_resp(h, drv_status, drv_success_count, reqbk.cli_req_id);
         assert(!err);
         client_notify[reqbk.cli_id] = true;
     }
@@ -139,7 +134,7 @@ static void handle_driver()
 static bool handle_client(int cli_id)
 {
     int err = 0;
-    blk_queue_handle_t h = clients[cli_id].queue_h;
+    blk_queue_handle_t h = client_queues[cli_id];
     uintptr_t cli_data_base_paddr = config.clients[cli_id].data.io_addr;
     uintptr_t cli_data_base_vaddr = (uintptr_t)config.clients[cli_id].data.region.vaddr;
     uint64_t cli_data_region_size = config.clients[cli_id].data.region.size;

drivers/blk/mmc/imx/blk_driver.mk

@@ -14,7 +14,7 @@ blk_driver.elf: blk/mmc/imx/blk_driver.o
 blk/mmc/imx/blk_driver.o: ${USDHC_DRIVER_DIR}/usdhc.c |blk/mmc/imx
 	$(CC) -c $(CFLAGS) -o $@ $<
 
--include blk_driver.d
+-include blk/mmc/imx/mmc_driver.d
 
 blk/mmc/imx:
 	mkdir -p $@

drivers/blk/mmc/imx/usdhc.c

@@ -108,6 +108,7 @@ static struct driver_state {
         CardIdentStateOpCond,
         CardIdentStateSendCid,
         CardIdentStateSendRca,
+        CardIdentStateFrequencyChange,
         CardIdentStateSendCsd,
         CardIdentStateCardSelect,
         CardIdentStateDone,
@@ -270,18 +271,26 @@ static drv_status_t handle_interrupt_status(sd_cmd_t cmd)
     /* Important Note: INT_STATUS register is of the W1C (write 1 to clear) type */
     uint32_t int_status = usdhc_regs->int_status;
 
-    /* !cmd.data_present => !(int_status & USDHC_INT_STATUS_TC) */
-    assert(cmd.data_present || !(int_status & USDHC_INT_STATUS_TC));
-
     /* If any bits aside from Command Complete / Transfer Complete are set... */
     if (int_status & ~(USDHC_INT_STATUS_CC | USDHC_INT_STATUS_TC)) {
         /* [IMX8MDQLQRM] Tables 10-44, 10-45, 10-46.
 
             TODO: Map the specific errors to something sensible.
             TODO: Run the RST_C / RST_D to reset the comamnd/ data inhibit?
                 & Follow the proper [SD-HOST] error handling flow.
+
+            NOTE: As we don't do this, any errors cause the driver to
+                  continuously error with 'Could not send a command as CMD/DATA-inhibit
+                  fields were set' for any further commands.
          */
         LOG_DRIVER("-> received error response\n");
+
+        if (int_status & USDHC_INT_STATUS_DMAE) {
+            /* DMA error ==> probably a virtualiser error because of an
+               incorrect memory address. */
+            LOG_DRIVER_ERR("DMA error encountered: DS_ADDR: 0x%x\n", usdhc_regs->ds_addr);
+        }
+
         usdhc_regs->int_status = 0xffffffff;
 
         if (!card_detected()) {
@@ -294,6 +303,9 @@ static drv_status_t handle_interrupt_status(sd_cmd_t cmd)
         return DrvErrorInternal;
     }
 
+    /* !cmd.data_present => !(int_status & USDHC_INT_STATUS_TC) */
+    assert(cmd.data_present || !(int_status & USDHC_INT_STATUS_TC));
+
     if (int_status & USDHC_INT_STATUS_CC) {
         LOG_DRIVER("-> received response\n");
         usdhc_regs->int_status = USDHC_INT_STATUS_CC;
@@ -338,16 +350,33 @@ drv_status_t send_command_inner(command_state_t *state, sd_cmd_t cmd, uint32_t c
     switch (*state) {
     case SendStateInit:
         /* [IMX8MDQLQRM] 10.3.7.1.5 Command Transfer Type
-           The host driver checks the Command Inhibit DAT field (PRES_STATE[CDIHB]) and
-           the \Command Inhibit CMD field (PRES_STATE[CIHB]) in the Present State register
-           before writing to this register.
-        */
+         * The host driver checks the Command Inhibit DAT field (PRES_STATE[CDIHB]) and
+         * the \Command Inhibit CMD field (PRES_STATE[CIHB]) in the Present State register
+         * before writing to this register.
+         *
+         * There seems to be an issue with this IMX8 uSDHC device, where even
+         * though the CC and TC interrupts have come in and been acknowledged,
+         * the Command/Data Inhibit flags are still set. They seem to disappear
+         * after a few reads, and can sometimes appear even after doing:
+         *
+         *    while inhibit;
+         *    if inhibit { print error }
+         *    send command;
+         *
+         * So there's something really strange going on. @peterc thinks that it
+         * can be the card pulling the CMD/DATA lines high and marking it as busy.
+         *
+         * References:
+         * - https://github.com/ARM-software/arm-trusted-firmware/blob/2377542785a13e776f1c52ffdeb22ef0e83873a5/drivers/imx/usdhc/imx_usdhc.c#L122-L125
+         * - https://github.com/zephyrproject-rtos/zephyr/pull/40522
+         * - https://patchwork.ozlabs.org/project/uboot/patch/1357665792-8141-1-git-send-email-l.majewski@samsung.com/
+         */
+#ifdef DEBUG_DRIVER
         if (usdhc_regs->pres_state & (USDHC_PRES_STATE_CIHB | USDHC_PRES_STATE_CDIHB)) {
-            LOG_DRIVER_ERR("Could not send a command as CMD/DATA-inhibit fields were set\n");
-            usdhc_debug();
-
-            return DrvErrorInternal;
+            LOG_DRIVER("CMD/DATA-inhibit fields were set when sending a command..."
+                       "sending anyway in the hope it goes away\n");
         }
+#endif
 
         *state = SendStateSend;
         fallthrough;
@@ -756,12 +785,14 @@ drv_status_t perform_card_identification_and_select()
 
         /* The card is now in the STANDBY state of the 'Data transfer mode' */
         card_info.card_state = CardStateStdby;
-        /* [SD-PHY] 4.3 Data Transfer Mode
-            > In Data Transfer Mode the host may operate the card in f_PP frequency range.
+        driver_state.card_ident = CardIdentStateSendCsd;
+        fallthrough;
 
-            TODO(#187): Actually do `usdhc_change_clock_frequency(ClockSpeedDefault_25MHz)`
+    case CardIdentStateFrequencyChange:
+        /* [SD-PHY] 4.3 Data Transfer Mode
+         * > In Data Transfer Mode the host may operate the card in f_PP frequency range.
          */
-        driver_state.card_ident = CardIdentStateSendCsd;
+        usdhc_change_clock_frequency(ClockSpeedDefaultSpeed_25MHz);
         fallthrough;
 
     case CardIdentStateSendCsd:

drivers/blk/mmc/imx/usdhc.h

@@ -142,6 +142,7 @@ typedef struct imx_usdhc_regs {
 #define USDHC_INT_STATUS_CIE   BIT(19) /* Command index error. */
 #define USDHC_INT_STATUS_DTOE  BIT(20) /* Data timeout error. */
 #define USDHC_INT_STATUS_AC12E BIT(24) /* Auto CMD12 error. */
+#define USDHC_INT_STATUS_DMAE  BIT(28) /* DMA error. */
 
 /* [IMX8MDQLQRM] Section 10.3.7.1.15 Interrupt Status Enable (used ones only) */
 #define USDHC_INT_STATUS_EN_CCSEN    BIT(0)   /* Command complete status enable */
@@ -309,7 +310,8 @@ typedef enum sd_card_state {
 typedef enum sd_clock_freq {
     /* [SD-PHY] 4.2.1 Card Reset "The cards are initialized... 400KHz clock frequency" */
     ClockSpeedIdentify_400KHz = 400 * KHZ,
-
-    // TODO: Higher speeds are currently never used.
-    // ClockSpeedDefaultSpeed_25MHz = 25 * MHZ,
+    /* [SD-PHY] 4.3 "In Data Transfer mode the host may operate the card in fPP frequency range "
+     * All card classes support Default Speed (25 MHz).
+     */
+    ClockSpeedDefaultSpeed_25MHz = 25 * MHZ,
 } sd_clock_freq_t;