mmc: wmt-sdmmc: Fix setting BM_EIGHTBIT_MODE bit in wmt_mci_set_ios()

PATCH Superseded Axel Lin 2 participants 4 messages Oct 22, 2013 – Oct 23, 2013

Subsystems (2)

MMC, SD and SDIO Arm/VT8500 Arm architecture

lore

Share & tools

lore

Axel Lin

author

2013-10-22 08:54 UTC #0 lore raw

PATCH 1/1 mmc: wmt-sdmmc: Fix setting BM_EIGHTBIT_MODE bit in wmt_mci_set_ios()

For MMC_BUS_WIDTH_8 case, current code missed setting BM_EIGHTBIT_MODE bit. Also has a small refactor to make the code looks better in readability.

So the bit settings witch below logic:

SDMMC_BUSMODE register:
Set EIGHTBIT_MODE bit for 8 bit mode, Set FOURBIT_MODE bit for 4 bit mode. Clear both EIGHTBIT_MODE and FOURBIT_MODE bits for 1 bit mode.

SDMMC_EXTCTRL register:
Set EXT_EIGHTBIT bit for 8 bit mode, Clear EXT_EIGHTBIT bit for 1/4 bit mode.

Add define for EXT_EIGHTBIT to avoid using magic number. BM_ONEBIT_MASK is no longer used, thus remove it.

Signed-off-by Axel Lin <[email protected]>

---
Hi Tony,
After checking wm8505+sdmmc+controller+suppliment.pdf, I'm wondering if this dirver works for MMC_BUS_WIDTH_8 case without setting BM_EIGHTBIT_MODE bit. I don't have this h/w to test.
I'd appreciate if you can review and test if this patch works. Thanks,
Axel

 drivers/mmc/host/wmt-sdmmc.c | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

drivers/mmc/host/wmt-sdmmc.c +15 ~~-16~~

index 8a48a50..6ed18f8 100644
@@ -72,7 +72,6 @@
 #define BM_SPI_CS			0x20
 #define BM_SD_POWER			0x40
 #define BM_SOFT_RESET			0x80
-#define BM_ONEBIT_MASK			0xFD
 
 /* SDMMC_BLKLEN bit fields */
 #define BLKL_CRCERR_ABORT		0x0800

@@ -120,6 +119,8 @@
 #define STS2_DATARSP_BUSY		0x20
 #define STS2_DIS_FORCECLK		0x80
 
+/* SDMMC_EXTCTRL bit fields */
+#define EXT_EIGHTBIT			0x04
 
 /* MMC/SD DMA Controller Registers */
 #define SDDMA_GCR			0x100

@@ -672,7 +673,7 @@ static void wmt_mci_request(struct mmc_host *mmc, struct mmc_request *req)
 static void wmt_mci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 {
 	struct wmt_mci_priv *priv;
-	u32 reg_tmp;
+	u32 busmode, extctrl;
 
 	priv = mmc_priv(mmc);

@@ -687,28 +688,26 @@ static void wmt_mci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 	if (ios->clock != 0)
 		clk_set_rate(priv->clk_sdmmc, ios->clock);
 
+	busmode = readb(priv->sdmmc_base + SDMMC_BUSMODE);
+	extctrl = readb(priv->sdmmc_base + SDMMC_EXTCTRL);
+
+	busmode &= ~(BM_EIGHTBIT_MODE | BM_FOURBIT_MODE);
+	extctrl &= ~EXT_EIGHTBIT;
+
 	switch (ios->bus_width) {
 	case MMC_BUS_WIDTH_8:
-		reg_tmp = readb(priv->sdmmc_base + SDMMC_EXTCTRL);
-		writeb(reg_tmp | 0x04, priv->sdmmc_base + SDMMC_EXTCTRL);
+		busmode |= BM_EIGHTBIT_MODE;
+		extctrl |= EXT_EIGHTBIT;
 		break;
 	case MMC_BUS_WIDTH_4:
-		reg_tmp = readb(priv->sdmmc_base + SDMMC_BUSMODE);
-		writeb(reg_tmp | BM_FOURBIT_MODE, priv->sdmmc_base +
-		       SDMMC_BUSMODE);
-
-		reg_tmp = readb(priv->sdmmc_base + SDMMC_EXTCTRL);
-		writeb(reg_tmp & 0xFB, priv->sdmmc_base + SDMMC_EXTCTRL);
+		busmode |= BM_FOURBIT_MODE;
 		break;
 	case MMC_BUS_WIDTH_1:
-		reg_tmp = readb(priv->sdmmc_base + SDMMC_BUSMODE);
-		writeb(reg_tmp & BM_ONEBIT_MASK, priv->sdmmc_base +
-		       SDMMC_BUSMODE);
-
-		reg_tmp = readb(priv->sdmmc_base + SDMMC_EXTCTRL);
-		writeb(reg_tmp & 0xFB, priv->sdmmc_base + SDMMC_EXTCTRL);
 		break;
 	}
+
+	writeb(busmode, priv->sdmmc_base + SDMMC_BUSMODE);
+	writeb(extctrl, priv->sdmmc_base + SDMMC_EXTCTRL);
 }
 
 static int wmt_mci_get_ro(struct mmc_host *mmc)

Signature · 1 line

1.8.1.2

Tony Prisk

2013-10-23 04:51 UTC #1 lore raw

Quoted 88 lines · Axel Lin

For MMC_BUS_WIDTH_8 case, current code missed setting BM_EIGHTBIT_MODE bit. Also has a small refactor to make the code looks better in readability.

So the bit settings witch below logic:

SDMMC_BUSMODE register:
Set EIGHTBIT_MODE bit for 8 bit mode, Set FOURBIT_MODE bit for 4 bit mode. Clear both EIGHTBIT_MODE and FOURBIT_MODE bits for 1 bit mode.

SDMMC_EXTCTRL register:
Set EXT_EIGHTBIT bit for 8 bit mode, Clear EXT_EIGHTBIT bit for 1/4 bit mode.

Add define for EXT_EIGHTBIT to avoid using magic number. BM_ONEBIT_MASK is no longer used, thus remove it.

Signed-off-by Axel Lin <[email protected]>

  drivers/mmc/host/wmt-sdmmc.c | 31 +++++++++++++++----------------
  1 file changed, 15 insertions(+), 16 deletions(-)

drivers/mmc/host/wmt-sdmmc.c +12 ~~-14~~

index 8a48a50..6ed18f8 100644
@@ -72,7 +72,6 @@
  #define BM_SPI_CS			0x20
  #define BM_SD_POWER			0x40
  #define BM_SOFT_RESET			0x80
-#define BM_ONEBIT_MASK			0xFD
 
  /* SDMMC_BLKLEN bit fields */
  #define BLKL_CRCERR_ABORT		0x0800

@@ -120,6 +119,8 @@
  #define STS2_DATARSP_BUSY		0x20
  #define STS2_DIS_FORCECLK		0x80
 
+/* SDMMC_EXTCTRL bit fields */
+#define EXT_EIGHTBIT			0x04
 
  /* MMC/SD DMA Controller Registers */
  #define SDDMA_GCR			0x100

@@ -672,7 +673,7 @@ static void wmt_mci_request(struct mmc_host *mmc, struct mmc_request *req)
  static void wmt_mci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
  {
  	struct wmt_mci_priv *priv;
-	u32 reg_tmp;
+	u32 busmode, extctrl;
 
  	priv = mmc_priv(mmc);

@@ -687,28 +688,26 @@ static void wmt_mci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
  	if (ios->clock != 0)
  		clk_set_rate(priv->clk_sdmmc, ios->clock);
 
+	busmode = readb(priv->sdmmc_base + SDMMC_BUSMODE);
+	extctrl = readb(priv->sdmmc_base + SDMMC_EXTCTRL);
+
+	busmode &= ~(BM_EIGHTBIT_MODE | BM_FOURBIT_MODE);
+	extctrl &= ~EXT_EIGHTBIT;
+
  	switch (ios->bus_width) {
  	case MMC_BUS_WIDTH_8:
-		reg_tmp = readb(priv->sdmmc_base + SDMMC_EXTCTRL);
-		writeb(reg_tmp | 0x04, priv->sdmmc_base + SDMMC_EXTCTRL);
+		busmode |= BM_EIGHTBIT_MODE;
+		extctrl |= EXT_EIGHTBIT;
  		break;
  	case MMC_BUS_WIDTH_4:
-		reg_tmp = readb(priv->sdmmc_base + SDMMC_BUSMODE);
-		writeb(reg_tmp | BM_FOURBIT_MODE, priv->sdmmc_base +
-		       SDMMC_BUSMODE);
-
-		reg_tmp = readb(priv->sdmmc_base + SDMMC_EXTCTRL);
-		writeb(reg_tmp & 0xFB, priv->sdmmc_base + SDMMC_EXTCTRL);
+		busmode |= BM_FOURBIT_MODE;
  		break;
  	case MMC_BUS_WIDTH_1:
-		reg_tmp = readb(priv->sdmmc_base + SDMMC_BUSMODE);
-		writeb(reg_tmp & BM_ONEBIT_MASK, priv->sdmmc_base +
-		       SDMMC_BUSMODE);
-

drivers/mmc/host/wmt-sdmmc.c static void wmt_mci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) · line 713

Quoted 10 lines

drivers/mmc/host/wmt-sdmmc.c +3 -2

-		reg_tmp = readb(priv->sdmmc_base + SDMMC_EXTCTRL);
-		writeb(reg_tmp & 0xFB, priv->sdmmc_base + SDMMC_EXTCTRL);
  		break;
  	}
+
+	writeb(busmode, priv->sdmmc_base + SDMMC_BUSMODE);
+	writeb(extctrl, priv->sdmmc_base + SDMMC_EXTCTRL);
  }
 
  static int wmt_mci_get_ro(struct mmc_host *mmc)

I don't have any means of testing this - All the vendor hardware I have uses 4-bit mode.

Regards
Tony Prisk

Axel Lin

author

2013-10-23 06:48 UTC #2 lore raw

2013/10/23 Tony Prisk <[email protected]>:

Quoted 94 lines · Axel Lin

Quoted 93 lines

For MMC_BUS_WIDTH_8 case, current code missed setting BM_EIGHTBIT_MODE bit.
Also has a small refactor to make the code looks better in readability.

So the bit settings witch below logic:

SDMMC_BUSMODE register:
Set EIGHTBIT_MODE bit for 8 bit mode, Set FOURBIT_MODE bit for 4 bit mode. Clear both EIGHTBIT_MODE and FOURBIT_MODE bits for 1 bit mode.

SDMMC_EXTCTRL register:
Set EXT_EIGHTBIT bit for 8 bit mode, Clear EXT_EIGHTBIT bit for 1/4 bit mode.

Add define for EXT_EIGHTBIT to avoid using magic number. BM_ONEBIT_MASK is no longer used, thus remove it.

Signed-off-by Axel Lin <[email protected]>

---
Hi Tony,
After checking wm8505+sdmmc+controller+suppliment.pdf, I'm wondering if this
dirver works for MMC_BUS_WIDTH_8 case without setting BM_EIGHTBIT_MODE bit.
I don't have this h/w to test.
I'd appreciate if you can review and test if this patch works. Thanks,
Axel

  drivers/mmc/host/wmt-sdmmc.c | 31 +++++++++++++++----------------
  1 file changed, 15 insertions(+), 16 deletions(-)

drivers/mmc/host/wmt-sdmmc.c +2 -2

index 8a48a50..6ed18f8 100644
@@ -72,7 +72,6 @@
  #define BM_SPI_CS                     0x20
  #define BM_SD_POWER                   0x40
  #define BM_SOFT_RESET                 0x80
-#define BM_ONEBIT_MASK                 0xFD
    /* SDMMC_BLKLEN bit fields */
  #define BLKL_CRCERR_ABORT             0x0800

@@ -120,6 +119,8 @@
  #define STS2_DATARSP_BUSY             0x20
  #define STS2_DIS_FORCECLK             0x80
  +/* SDMMC_EXTCTRL bit fields */
+#define EXT_EIGHTBIT                   0x04
    /* MMC/SD DMA Controller Registers */
  #define SDDMA_GCR                     0x100

@@ -672,7 +673,7 @@ static void wmt_mci_request(struct mmc_host *mmc,
 struct mmc_request *req)
  static void wmt_mci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
  {
        struct wmt_mci_priv *priv;
-       u32 reg_tmp;
+       u32 busmode, extctrl;
        priv = mmc_priv(mmc);
  @@ -687,28 +688,26 @@ static void wmt_mci_set_ios(struct mmc_host *mmc,

struct mmc_ios *ios)

        if (ios->clock != 0)
                clk_set_rate(priv->clk_sdmmc, ios->clock);

+ busmode = readb(priv->sdmmc_base + SDMMC_BUSMODE);

drivers/mmc/host/wmt-sdmmc.c +8 ~~-14~~

+       extctrl = readb(priv->sdmmc_base + SDMMC_EXTCTRL);
+
+       busmode &= ~(BM_EIGHTBIT_MODE | BM_FOURBIT_MODE);
+       extctrl &= ~EXT_EIGHTBIT;
+
        switch (ios->bus_width) {
        case MMC_BUS_WIDTH_8:
-               reg_tmp = readb(priv->sdmmc_base + SDMMC_EXTCTRL);
-               writeb(reg_tmp | 0x04, priv->sdmmc_base + SDMMC_EXTCTRL);
+               busmode |= BM_EIGHTBIT_MODE;
+               extctrl |= EXT_EIGHTBIT;
                break;
        case MMC_BUS_WIDTH_4:
-               reg_tmp = readb(priv->sdmmc_base + SDMMC_BUSMODE);
-               writeb(reg_tmp | BM_FOURBIT_MODE, priv->sdmmc_base +
-                      SDMMC_BUSMODE);
-
-               reg_tmp = readb(priv->sdmmc_base + SDMMC_EXTCTRL);
-               writeb(reg_tmp & 0xFB, priv->sdmmc_base + SDMMC_EXTCTRL);
+               busmode |= BM_FOURBIT_MODE;
                break;
        case MMC_BUS_WIDTH_1:
-               reg_tmp = readb(priv->sdmmc_base + SDMMC_BUSMODE);
-               writeb(reg_tmp & BM_ONEBIT_MASK, priv->sdmmc_base +
-                      SDMMC_BUSMODE);
-
-               reg_tmp = readb(priv->sdmmc_base + SDMMC_EXTCTRL);
-               writeb(reg_tmp & 0xFB, priv->sdmmc_base + SDMMC_EXTCTRL);
                break;
        }

Quoted 8 lines

drivers/mmc/host/wmt-sdmmc.c static void wmt_mci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) · line 713
Quoted 5 lines
drivers/mmc/host/wmt-sdmmc.c +3 -0
+
+       writeb(busmode, priv->sdmmc_base + SDMMC_BUSMODE);
+       writeb(extctrl, priv->sdmmc_base + SDMMC_EXTCTRL);
  }
    static int wmt_mci_get_ro(struct mmc_host *mmc)
I don't have any means of testing this - All the vendor hardware I have uses 4-bit mode.

Oh, well.
But how do you think about this patch? Since the code is there, and it looks wrong because wm8505+sdmmc+controller+suppliment.pdf says

SDMMC Ext Control (Offset 0x0034)
BIT2 RW
0: One Bit / Four bit mode
1: Eight bit mode

Probably still good to make the code matches the document.

Regards,
Axel

Tony Prisk

2013-10-23 18:10 UTC #3 lore raw

Quoted 111 lines · Axel Lin

2013/10/23 Tony Prisk <[email protected]>:

Quoted 92 lines · Axel Lin

Quoted 91 lines

For MMC_BUS_WIDTH_8 case, current code missed setting BM_EIGHTBIT_MODE bit.
Also has a small refactor to make the code looks better in readability.

So the bit settings witch below logic:

SDMMC_BUSMODE register:
Set EIGHTBIT_MODE bit for 8 bit mode, Set FOURBIT_MODE bit for 4 bit mode. Clear both EIGHTBIT_MODE and FOURBIT_MODE bits for 1 bit mode.

SDMMC_EXTCTRL register:
Set EXT_EIGHTBIT bit for 8 bit mode, Clear EXT_EIGHTBIT bit for 1/4 bit mode.

Add define for EXT_EIGHTBIT to avoid using magic number. BM_ONEBIT_MASK is no longer used, thus remove it.

Signed-off-by Axel Lin <[email protected]>

---
Hi Tony,
After checking wm8505+sdmmc+controller+suppliment.pdf, I'm wondering if this
dirver works for MMC_BUS_WIDTH_8 case without setting BM_EIGHTBIT_MODE bit.
I don't have this h/w to test.
I'd appreciate if you can review and test if this patch works. Thanks,
Axel

   drivers/mmc/host/wmt-sdmmc.c | 31 +++++++++++++++----------------
   1 file changed, 15 insertions(+), 16 deletions(-)

drivers/mmc/host/wmt-sdmmc.c +2 -2

index 8a48a50..6ed18f8 100644
@@ -72,7 +72,6 @@
   #define BM_SPI_CS                     0x20
   #define BM_SD_POWER                   0x40
   #define BM_SOFT_RESET                 0x80
-#define BM_ONEBIT_MASK                 0xFD
     /* SDMMC_BLKLEN bit fields */
   #define BLKL_CRCERR_ABORT             0x0800

@@ -120,6 +119,8 @@
   #define STS2_DATARSP_BUSY             0x20
   #define STS2_DIS_FORCECLK             0x80
   +/* SDMMC_EXTCTRL bit fields */
+#define EXT_EIGHTBIT                   0x04
     /* MMC/SD DMA Controller Registers */
   #define SDDMA_GCR                     0x100

@@ -672,7 +673,7 @@ static void wmt_mci_request(struct mmc_host *mmc,
 struct mmc_request *req)
   static void wmt_mci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
   {
         struct wmt_mci_priv *priv;
-       u32 reg_tmp;
+       u32 busmode, extctrl;
         priv = mmc_priv(mmc);
   @@ -687,28 +688,26 @@ static void wmt_mci_set_ios(struct mmc_host *mmc,

struct mmc_ios *ios)

         if (ios->clock != 0)
                 clk_set_rate(priv->clk_sdmmc, ios->clock);

+ busmode = readb(priv->sdmmc_base + SDMMC_BUSMODE);

drivers/mmc/host/wmt-sdmmc.c +8 ~~-14~~

+       extctrl = readb(priv->sdmmc_base + SDMMC_EXTCTRL);
+
+       busmode &= ~(BM_EIGHTBIT_MODE | BM_FOURBIT_MODE);
+       extctrl &= ~EXT_EIGHTBIT;
+
         switch (ios->bus_width) {
         case MMC_BUS_WIDTH_8:
-               reg_tmp = readb(priv->sdmmc_base + SDMMC_EXTCTRL);
-               writeb(reg_tmp | 0x04, priv->sdmmc_base + SDMMC_EXTCTRL);
+               busmode |= BM_EIGHTBIT_MODE;
+               extctrl |= EXT_EIGHTBIT;
                 break;
         case MMC_BUS_WIDTH_4:
-               reg_tmp = readb(priv->sdmmc_base + SDMMC_BUSMODE);
-               writeb(reg_tmp | BM_FOURBIT_MODE, priv->sdmmc_base +
-                      SDMMC_BUSMODE);
-
-               reg_tmp = readb(priv->sdmmc_base + SDMMC_EXTCTRL);
-               writeb(reg_tmp & 0xFB, priv->sdmmc_base + SDMMC_EXTCTRL);
+               busmode |= BM_FOURBIT_MODE;
                 break;
         case MMC_BUS_WIDTH_1:
-               reg_tmp = readb(priv->sdmmc_base + SDMMC_BUSMODE);
-               writeb(reg_tmp & BM_ONEBIT_MASK, priv->sdmmc_base +
-                      SDMMC_BUSMODE);
-
-               reg_tmp = readb(priv->sdmmc_base + SDMMC_EXTCTRL);
-               writeb(reg_tmp & 0xFB, priv->sdmmc_base + SDMMC_EXTCTRL);
                 break;

Quoted 8 lines

drivers/mmc/host/wmt-sdmmc.c static void wmt_mci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) · line 713
Quoted 6 lines
         }
drivers/mmc/host/wmt-sdmmc.c +3 -0
+
+       writeb(busmode, priv->sdmmc_base + SDMMC_BUSMODE);
+       writeb(extctrl, priv->sdmmc_base + SDMMC_EXTCTRL);
   }
     static int wmt_mci_get_ro(struct mmc_host *mmc)
I don't have any means of testing this - All the vendor hardware I have uses 4-bit mode.

Oh, well.
But how do you think about this patch? Since the code is there, and it looks wrong because wm8505+sdmmc+controller+suppliment.pdf says

SDMMC Ext Control (Offset 0x0034)
BIT2 RW
0: One Bit / Four bit mode
1: Eight bit mode

Quoted 4 lines

Probably still good to make the code matches the document.
Regards,
Axel

I have no issue with the patch - it seems to make sense in relation to the documentation.
I would however suggest that given it is untested, that you perhaps add a note to the header to indicate it as such so no one gets confused down the track if/when there is 8-bit hardware.

Regards
Tony Prisk