Skip to contentSkip to conversation

dw_mmc: add quirk about using only one slot

RFC PATCH In review Jaehoon Chung 2 participants 3 messages Mar 25, 2011

Subsystems (2)
MMC, SD and SDIO Synopsys DesignWare MMC/SD/SDIO driver
lore
Share & tools
lore

Jaehoon Chung

author maintainer
#0 lore raw

RFC PATCH 1/1 dw_mmc: add quirk about using only one slot

If assume only using one slot, i think that dw_mci_queue_request() need not.

Signed-off-by Jaehoon Chung <[email protected]>
Signed-off-by Kyungmin Park <[email protected]
---
 drivers/mmc/host/dw_mmc.c  |    9 +++++++--
 include/linux/mmc/dw_mmc.h |    3 ++-
 2 files changed, 9 insertions(+), 3 deletions(-)
drivers/mmc/host/dw_mmc.c +7 -2
index 882d004..e3f26ea 100644
@@ -673,8 +673,13 @@ static void dw_mci_request(struct mmc_host *mmc, struct mmc_request *mrq)
 		return;
 	}
 
-	/* We don't support multiple blocks of weird lengths. */
-	dw_mci_queue_request(host, slot, mrq);
+	if (host->quirks & DW_MCI_QUIRK_FORCE_ONE_SLOT) {
+		slot->mrq = mrq;
+		host->state = STATE_SENDING_CMD;
+		dw_mci_start_request(host, slot);
+	} else
+		/* We don't support multiple blocks of weird lengths. */
+		dw_mci_queue_request(host, slot, mrq);
 }
 
 static void dw_mci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
include/linux/mmc/dw_mmc.h +2 -1
index c0207a7..82f59c2 100644
@@ -176,7 +176,8 @@ struct dw_mci_dma_ops {
 #define DW_MCI_QUIRK_HIGHSPEED			BIT(2)
 /* Unreliable card detection */
 #define DW_MCI_QUIRK_BROKEN_CARD_DETECTION	BIT(3)
-
+/* Force using only one slot */
+#define DW_MCI_QUIRK_FORCE_ONE_SLOT		BIT(4)
 
 struct dma_pdata;
#1 lore raw
Quoted 14 lines · Jaehoon Chung

If assume only using one slot, i think that dw_mci_queue_request() need not.

Signed-off-by Jaehoon Chung <[email protected]>
Signed-off-by Kyungmin Park <[email protected]

---
 drivers/mmc/host/dw_mmc.c  |    9 +++++++--
 include/linux/mmc/dw_mmc.h |    3 ++-

 2 files changed, 9 insertions(+), 3 deletions(-)
drivers/mmc/host/dw_mmc.c +0 -0
index 882d004..e3f26ea 100644

drivers/mmc/host/dw_mmc.c static void dw_mci_request(struct mmc_host *mmc, struct mmc_request *mrq) · line 676

Quoted 6 lines
drivers/mmc/host/dw_mmc.c +1 -2
@@ -673,8 +673,13 @@ static void dw_mci_request(struct mmc_host *mmc, struct mmc_request *mrq)
                return;
        }
 
-       /* We don't support multiple blocks of weird lengths. */
-       dw_mci_queue_request(host, slot, mrq);
+       if (host->quirks & DW_MCI_QUIRK_FORCE_ONE_SLOT) {

Do we really need a quirk for this? Why not use host->num_slots?

drivers/mmc/host/dw_mmc.c static void dw_mci_request(struct mmc_host *mmc, struct mmc_request *mrq) · line 678

Quoted 2 lines
drivers/mmc/host/dw_mmc.c +2 -0
+               slot->mrq = mrq;
+               host->state = STATE_SENDING_CMD;

I don't think it is safe to manipulate these structures without taking the host->lock. If we are to do this then I think I would like to know why (e.g. do we have performance numbers to support this change) and some analysis of what is protected by host->lock and which functions need the lock to be held. For example I do not think it is safe to call dw_mci_start_request without taking the lock.

drivers/mmc/host/dw_mmc.c static void dw_mci_request(struct mmc_host *mmc, struct mmc_request *mrq) · line 681

Quoted 3 lines
drivers/mmc/host/dw_mmc.c +3 -0
+               dw_mci_start_request(host, slot);
+       } else
+               /* We don't support multiple blocks of weird lengths. */

This comment is obsolete I think and can be removed.

author maintainer
#2 lore raw

Hi Will

Quoted 18 lines · Will Newton
Quoted 17 lines · Jaehoon Chung

If assume only using one slot, i think that dw_mci_queue_request() need not.

Signed-off-by Jaehoon Chung <[email protected]>
Signed-off-by Kyungmin Park <[email protected]
---
 drivers/mmc/host/dw_mmc.c  |    9 +++++++--
 include/linux/mmc/dw_mmc.h |    3 ++-
 2 files changed, 9 insertions(+), 3 deletions(-)
drivers/mmc/host/dw_mmc.c +0 -0
index 882d004..e3f26ea 100644
@@ -673,8 +673,13 @@ static void dw_mci_request(struct mmc_host *mmc, struct mmc_request *mrq)
               return;
       }
Quoted 5 lines

drivers/mmc/host/dw_mmc.c static void dw_mci_request(struct mmc_host *mmc, struct mmc_request *mrq) · line 676

Quoted 3 lines
drivers/mmc/host/dw_mmc.c +1 -2
-       /* We don't support multiple blocks of weird lengths. */
-       dw_mci_queue_request(host, slot, mrq);
+       if (host->quirks & DW_MCI_QUIRK_FORCE_ONE_SLOT) {

Do we really need a quirk for this? Why not use host->num_slots

Yes, you'r right. if using this patch, can use host->num_slots.

Quoted 11 lines

drivers/mmc/host/dw_mmc.c static void dw_mci_request(struct mmc_host *mmc, struct mmc_request *mrq) · line 678

Quoted 3 lines
drivers/mmc/host/dw_mmc.c +2 -0
+               slot->mrq = mrq;
+               host->state = STATE_SENDING_CMD;

I don't think it is safe to manipulate these structures without taking the host->lock. If we are to do this then I think I would like to know why (e.g. do we have performance numbers to support this change) and some analysis of what is protected by host->lock and which functions need the lock to be held. For example I do not think it is safe to call dw_mci_start_request without taking the lock.

I know this patch didn't increase the performance.
BUt If we want to know some analysis..I should analysis what protected by host->lock. Did you analysis what protect by host->lock?

Quoted 5 lines

drivers/mmc/host/dw_mmc.c static void dw_mci_request(struct mmc_host *mmc, struct mmc_request *mrq) · line 681

Quoted 3 lines
drivers/mmc/host/dw_mmc.c +3 -0
+               dw_mci_start_request(host, slot);
+       } else
+               /* We don't support multiple blocks of weird lengths. */

This comment is obsolete I think and can be removed.

Anyway, Thanks for your comment.

Regards,
Jaehoon Chung