[PATCH 0/1] sdhci-bcm2835: added quirk and removed udelay in write ops

[PATCH 0/1] sdhci-bcm2835: added quirk and removed udelay in write ops

[Scott Branden]

This patch contains driver cleanup of sdhci-bcm2835.
Please note that this has not actually been tested on bcm2835 yet.
Testing comes from other devices with the same sdhci controller.

This patch is being put out for testing and acceptance on the 2835.
Please test and comment.

Scott Branden (1):
  mmc: sdhci-bcm2835: added quirk and removed udelay in write ops

 drivers/mmc/host/sdhci-bcm2835.c |  139 ++++++++++++++++++--------------------
 1 file changed, 66 insertions(+), 73 deletions(-)

-- 
1.7.9.5

[PATCH 1/1] mmc: sdhci-bcm2835: added quirk and removed udelay in write ops

[Scott Branden]

Added quirk SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12 present in controller.
Removed udelay in write ops by using shadow registers for 16 bit
accesses to 32-bit registers (where necessary).
Optimized 32-bit operations when doing 8/16 register accesses.

Signed-off-by: Scott Branden <sbranden@broadcom.com>
---
 drivers/mmc/host/sdhci-bcm2835.c |  139 ++++++++++++++++++--------------------
 1 file changed, 66 insertions(+), 73 deletions(-)

diff --git a/drivers/mmc/host/sdhci-bcm2835.c b/drivers/mmc/host/sdhci-bcm2835.c
index 439d259..d967a4f 100644
--- a/drivers/mmc/host/sdhci-bcm2835.c
+++ b/drivers/mmc/host/sdhci-bcm2835.c
@@ -25,42 +25,28 @@
 #include "sdhci-pltfm.h"
 
 /*
- * 400KHz is max freq for card ID etc. Use that as min card clock. We need to
- * know the min to enable static calculation of max BCM2835_SDHCI_WRITE_DELAY.
- */
-#define MIN_FREQ 400000
-
-/*
  * The Arasan has a bugette whereby it may lose the content of successive
- * writes to registers that are within two SD-card clock cycles of each other
- * (a clock domain crossing problem). It seems, however, that the data
- * register does not have this problem, which is just as well - otherwise we'd
- * have to nobble the DMA engine too.
+ * writes to the same register that are within two SD-card clock cycles of
+ * each other (a clock domain crossing problem).  Problem does not happen with
+ * data.
+ * This wouldn't be a problem with the code except that we can only write the
+ * controller with 32-bit writes.  So two different 16-bit registers in the
+ * written back to back creates the problem.
  *
- * This should probably be dynamically calculated based on the actual card
- * frequency. However, this is the longest we'll have to wait, and doesn't
- * seem to slow access down too much, so the added complexity doesn't seem
- * worth it for now.
- *
- * 1/MIN_FREQ is (max) time per tick of eMMC clock.
- * 2/MIN_FREQ is time for two ticks.
- * Multiply by 1000000 to get uS per two ticks.
- * *1000000 for uSecs.
- * +1 for hack rounding.
+ * In reality, this only happens when a SDHCI_BLOCK_SIZE and SDHCI_BLOCK_COUNT
+ * are written followed by SDHCI_TRANSFER_MODE and SDHCI_COMMAND.
+ * The BLOCK_SIZE and BLOCK_COUNT are meaningless until a command issued so
+ * the work around can be further optimized. We can keep shadow values of
+ * BLOCK_SIZE, BLOCK_COUNT, and TRANSFER_MODE until a COMMAND is issued.
+ * Then, write the BLOCK_SIZE+BLOCK_COUNT in a single 32-bit write followed
+ * by the TRANSFER+COMMAND in another 32-bit write.
  */
-#define BCM2835_SDHCI_WRITE_DELAY	(((2 * 1000000) / MIN_FREQ) + 1)
 
-struct bcm2835_sdhci {
-	u32 shadow;
+struct bcm2835_sdhci_host {
+	u32 shadow_cmd;
+	u32 shadow_blk;
 };
 
-static void bcm2835_sdhci_writel(struct sdhci_host *host, u32 val, int reg)
-{
-	writel(val, host->ioaddr + reg);
-
-	udelay(BCM2835_SDHCI_WRITE_DELAY);
-}
-
 static inline u32 bcm2835_sdhci_readl(struct sdhci_host *host, int reg)
 {
 	u32 val = readl(host->ioaddr + reg);
@@ -71,76 +57,83 @@ static inline u32 bcm2835_sdhci_readl(struct sdhci_host *host, int reg)
 	return val;
 }
 
-static void bcm2835_sdhci_writew(struct sdhci_host *host, u16 val, int reg)
-{
-	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
-	struct bcm2835_sdhci *bcm2835_host = pltfm_host->priv;
-	u32 oldval = (reg == SDHCI_COMMAND) ? bcm2835_host->shadow :
-		bcm2835_sdhci_readl(host, reg & ~3);
-	u32 word_num = (reg >> 1) & 1;
-	u32 word_shift = word_num * 16;
-	u32 mask = 0xffff << word_shift;
-	u32 newval = (oldval & ~mask) | (val << word_shift);
-
-	if (reg == SDHCI_TRANSFER_MODE)
-		bcm2835_host->shadow = newval;
-	else
-		bcm2835_sdhci_writel(host, newval, reg & ~3);
-}
-
 static u16 bcm2835_sdhci_readw(struct sdhci_host *host, int reg)
 {
-	u32 val = bcm2835_sdhci_readl(host, (reg & ~3));
-	u32 word_num = (reg >> 1) & 1;
-	u32 word_shift = word_num * 16;
-	u32 word = (val >> word_shift) & 0xffff;
-
+	u32 val = bcm2835_sdhci_readl(host->ioaddr, (reg & ~3));
+	u16 word = val >> (reg << 3 & 0x18) & 0xffff;
 	return word;
 }
 
-static void bcm2835_sdhci_writeb(struct sdhci_host *host, u8 val, int reg)
+static u8 bcm2835_sdhci_readb(struct sdhci_host *host, int reg)
 {
-	u32 oldval = bcm2835_sdhci_readl(host, reg & ~3);
-	u32 byte_num = reg & 3;
-	u32 byte_shift = byte_num * 8;
-	u32 mask = 0xff << byte_shift;
-	u32 newval = (oldval & ~mask) | (val << byte_shift);
+	u32 val = bcm2835_sdhci_readl(host->ioaddr, (reg & ~3));
+	u8 byte = val >> (reg << 3 & 0x18) & 0xff;
+	return byte;
+}
 
-	bcm2835_sdhci_writel(host, newval, reg & ~3);
+static void bcm2835_sdhci_writel(struct sdhci_host *host, u32 val, int reg)
+{
+	writel(val, host->ioaddr + reg);
 }
 
-static u8 bcm2835_sdhci_readb(struct sdhci_host *host, int reg)
+static void bcm2835_sdhci_writew(struct sdhci_host *host, u16 val, int reg)
 {
-	u32 val = bcm2835_sdhci_readl(host, (reg & ~3));
-	u32 byte_num = reg & 3;
-	u32 byte_shift = byte_num * 8;
-	u32 byte = (val >> byte_shift) & 0xff;
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct bcm2835_sdhci_host *bcm2835_host = pltfm_host->priv;
+	u32 word_shift = reg << 3 & 0x18;
+	u32 mask = 0xffff << word_shift;
+	u32 oldval;
+	u32 newval;
+
+	if (reg == SDHCI_COMMAND) {
+		if (bcm2835_host->shadow_blk != 0) {
+			writel(bcm2835_host->shadow_blk,
+			       host->ioaddr + SDHCI_BLOCK_SIZE);
+			bcm2835_host->shadow_blk = 0;
+		}
+		oldval = bcm2835_host->shadow_cmd;
+	} else if (reg == SDHCI_BLOCK_SIZE || reg == SDHCI_BLOCK_COUNT) {
+		oldval = bcm2835_host->shadow_blk;
+	} else {
+		oldval = readl(host->ioaddr + (reg & ~3));
+	}
+	newval = (oldval & ~mask) | (val << word_shift);
 
-	return byte;
+	if (reg == SDHCI_TRANSFER_MODE)
+		bcm2835_host->shadow_cmd = newval;
+	else if (reg == SDHCI_BLOCK_SIZE || reg == SDHCI_BLOCK_COUNT)
+		bcm2835_host->shadow_blk = newval;
+	else
+		writel(newval, host->ioaddr + (reg & ~3));
 }
 
-static unsigned int bcm2835_sdhci_get_min_clock(struct sdhci_host *host)
+static void bcm2835_sdhci_writeb(struct sdhci_host *host, u8 val, int reg)
 {
-	return MIN_FREQ;
+	u32 oldval = readl(host->ioaddr + (reg & ~3));
+	u32 byte_shift = reg << 3 & 0x18;
+	u32 mask = 0xff << byte_shift;
+	u32 newval = (oldval & ~mask) | (val << byte_shift);
+
+	writel(newval, host->ioaddr + (reg & ~3));
 }
 
 static const struct sdhci_ops bcm2835_sdhci_ops = {
-	.write_l = bcm2835_sdhci_writel,
-	.write_w = bcm2835_sdhci_writew,
-	.write_b = bcm2835_sdhci_writeb,
 	.read_l = bcm2835_sdhci_readl,
 	.read_w = bcm2835_sdhci_readw,
 	.read_b = bcm2835_sdhci_readb,
+	.write_l = bcm2835_sdhci_writel,
+	.write_w = bcm2835_sdhci_writew,
+	.write_b = bcm2835_sdhci_writeb,
 	.set_clock = sdhci_set_clock,
 	.get_max_clock = sdhci_pltfm_clk_get_max_clock,
-	.get_min_clock = bcm2835_sdhci_get_min_clock,
 	.set_bus_width = sdhci_set_bus_width,
 	.reset = sdhci_reset,
 	.set_uhs_signaling = sdhci_set_uhs_signaling,
 };
 
 static const struct sdhci_pltfm_data bcm2835_sdhci_pdata = {
-	.quirks = SDHCI_QUIRK_BROKEN_CARD_DETECTION |
+	.quirks = SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12 |
+		  SDHCI_QUIRK_BROKEN_CARD_DETECTION |
 		  SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK,
 	.ops = &bcm2835_sdhci_ops,
 };
@@ -148,7 +141,7 @@ static const struct sdhci_pltfm_data bcm2835_sdhci_pdata = {
 static int bcm2835_sdhci_probe(struct platform_device *pdev)
 {
 	struct sdhci_host *host;
-	struct bcm2835_sdhci *bcm2835_host;
+	struct bcm2835_sdhci_host *bcm2835_host;
 	struct sdhci_pltfm_host *pltfm_host;
 	int ret;
 
-- 
1.7.9.5

Re: [PATCH 1/1] mmc: sdhci-bcm2835: added quirk and removed udelay in write ops

[Stephen Warren]

On 10/14/2014 08:01 PM, Scott Branden wrote:
> Added quirk SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12 present in controller.
> Removed udelay in write ops by using shadow registers for 16 bit
> accesses to 32-bit registers (where necessary).
> Optimized 32-bit operations when doing 8/16 register accesses.

I'm going to assume this is identical to the patch you sent 8/15? So,
I'll ignore this copy...

[PATCH 1/1] mmc: sdhci-bcm2835: added quirk and removed udelay in write ops

[Scott Branden]

Added quirk SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12 present in controller.
Removed udelay in write ops by using shadow registers for 16 bit
accesses to 32-bit registers (where necessary).
Optimized 32-bit operations when doing 8/16 register accesses.

Signed-off-by: Scott Branden <sbranden@broadcom.com>
---
 drivers/mmc/host/sdhci-bcm2835.c |  139 ++++++++++++++++++--------------------
 1 file changed, 66 insertions(+), 73 deletions(-)

diff --git a/drivers/mmc/host/sdhci-bcm2835.c b/drivers/mmc/host/sdhci-bcm2835.c
index 439d259..d967a4f 100644
--- a/drivers/mmc/host/sdhci-bcm2835.c
+++ b/drivers/mmc/host/sdhci-bcm2835.c
@@ -25,42 +25,28 @@
 #include "sdhci-pltfm.h"
 
 /*
- * 400KHz is max freq for card ID etc. Use that as min card clock. We need to
- * know the min to enable static calculation of max BCM2835_SDHCI_WRITE_DELAY.
- */
-#define MIN_FREQ 400000
-
-/*
  * The Arasan has a bugette whereby it may lose the content of successive
- * writes to registers that are within two SD-card clock cycles of each other
- * (a clock domain crossing problem). It seems, however, that the data
- * register does not have this problem, which is just as well - otherwise we'd
- * have to nobble the DMA engine too.
+ * writes to the same register that are within two SD-card clock cycles of
+ * each other (a clock domain crossing problem).  Problem does not happen with
+ * data.
+ * This wouldn't be a problem with the code except that we can only write the
+ * controller with 32-bit writes.  So two different 16-bit registers in the
+ * written back to back creates the problem.
  *
- * This should probably be dynamically calculated based on the actual card
- * frequency. However, this is the longest we'll have to wait, and doesn't
- * seem to slow access down too much, so the added complexity doesn't seem
- * worth it for now.
- *
- * 1/MIN_FREQ is (max) time per tick of eMMC clock.
- * 2/MIN_FREQ is time for two ticks.
- * Multiply by 1000000 to get uS per two ticks.
- * *1000000 for uSecs.
- * +1 for hack rounding.
+ * In reality, this only happens when a SDHCI_BLOCK_SIZE and SDHCI_BLOCK_COUNT
+ * are written followed by SDHCI_TRANSFER_MODE and SDHCI_COMMAND.
+ * The BLOCK_SIZE and BLOCK_COUNT are meaningless until a command issued so
+ * the work around can be further optimized. We can keep shadow values of
+ * BLOCK_SIZE, BLOCK_COUNT, and TRANSFER_MODE until a COMMAND is issued.
+ * Then, write the BLOCK_SIZE+BLOCK_COUNT in a single 32-bit write followed
+ * by the TRANSFER+COMMAND in another 32-bit write.
  */
-#define BCM2835_SDHCI_WRITE_DELAY	(((2 * 1000000) / MIN_FREQ) + 1)
 
-struct bcm2835_sdhci {
-	u32 shadow;
+struct bcm2835_sdhci_host {
+	u32 shadow_cmd;
+	u32 shadow_blk;
 };
 
-static void bcm2835_sdhci_writel(struct sdhci_host *host, u32 val, int reg)
-{
-	writel(val, host->ioaddr + reg);
-
-	udelay(BCM2835_SDHCI_WRITE_DELAY);
-}
-
 static inline u32 bcm2835_sdhci_readl(struct sdhci_host *host, int reg)
 {
 	u32 val = readl(host->ioaddr + reg);
@@ -71,76 +57,83 @@ static inline u32 bcm2835_sdhci_readl(struct sdhci_host *host, int reg)
 	return val;
 }
 
-static void bcm2835_sdhci_writew(struct sdhci_host *host, u16 val, int reg)
-{
-	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
-	struct bcm2835_sdhci *bcm2835_host = pltfm_host->priv;
-	u32 oldval = (reg == SDHCI_COMMAND) ? bcm2835_host->shadow :
-		bcm2835_sdhci_readl(host, reg & ~3);
-	u32 word_num = (reg >> 1) & 1;
-	u32 word_shift = word_num * 16;
-	u32 mask = 0xffff << word_shift;
-	u32 newval = (oldval & ~mask) | (val << word_shift);
-
-	if (reg == SDHCI_TRANSFER_MODE)
-		bcm2835_host->shadow = newval;
-	else
-		bcm2835_sdhci_writel(host, newval, reg & ~3);
-}
-
 static u16 bcm2835_sdhci_readw(struct sdhci_host *host, int reg)
 {
-	u32 val = bcm2835_sdhci_readl(host, (reg & ~3));
-	u32 word_num = (reg >> 1) & 1;
-	u32 word_shift = word_num * 16;
-	u32 word = (val >> word_shift) & 0xffff;
-
+	u32 val = bcm2835_sdhci_readl(host->ioaddr, (reg & ~3));
+	u16 word = val >> (reg << 3 & 0x18) & 0xffff;
 	return word;
 }
 
-static void bcm2835_sdhci_writeb(struct sdhci_host *host, u8 val, int reg)
+static u8 bcm2835_sdhci_readb(struct sdhci_host *host, int reg)
 {
-	u32 oldval = bcm2835_sdhci_readl(host, reg & ~3);
-	u32 byte_num = reg & 3;
-	u32 byte_shift = byte_num * 8;
-	u32 mask = 0xff << byte_shift;
-	u32 newval = (oldval & ~mask) | (val << byte_shift);
+	u32 val = bcm2835_sdhci_readl(host->ioaddr, (reg & ~3));
+	u8 byte = val >> (reg << 3 & 0x18) & 0xff;
+	return byte;
+}
 
-	bcm2835_sdhci_writel(host, newval, reg & ~3);
+static void bcm2835_sdhci_writel(struct sdhci_host *host, u32 val, int reg)
+{
+	writel(val, host->ioaddr + reg);
 }
 
-static u8 bcm2835_sdhci_readb(struct sdhci_host *host, int reg)
+static void bcm2835_sdhci_writew(struct sdhci_host *host, u16 val, int reg)
 {
-	u32 val = bcm2835_sdhci_readl(host, (reg & ~3));
-	u32 byte_num = reg & 3;
-	u32 byte_shift = byte_num * 8;
-	u32 byte = (val >> byte_shift) & 0xff;
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct bcm2835_sdhci_host *bcm2835_host = pltfm_host->priv;
+	u32 word_shift = reg << 3 & 0x18;
+	u32 mask = 0xffff << word_shift;
+	u32 oldval;
+	u32 newval;
+
+	if (reg == SDHCI_COMMAND) {
+		if (bcm2835_host->shadow_blk != 0) {
+			writel(bcm2835_host->shadow_blk,
+			       host->ioaddr + SDHCI_BLOCK_SIZE);
+			bcm2835_host->shadow_blk = 0;
+		}
+		oldval = bcm2835_host->shadow_cmd;
+	} else if (reg == SDHCI_BLOCK_SIZE || reg == SDHCI_BLOCK_COUNT) {
+		oldval = bcm2835_host->shadow_blk;
+	} else {
+		oldval = readl(host->ioaddr + (reg & ~3));
+	}
+	newval = (oldval & ~mask) | (val << word_shift);
 
-	return byte;
+	if (reg == SDHCI_TRANSFER_MODE)
+		bcm2835_host->shadow_cmd = newval;
+	else if (reg == SDHCI_BLOCK_SIZE || reg == SDHCI_BLOCK_COUNT)
+		bcm2835_host->shadow_blk = newval;
+	else
+		writel(newval, host->ioaddr + (reg & ~3));
 }
 
-static unsigned int bcm2835_sdhci_get_min_clock(struct sdhci_host *host)
+static void bcm2835_sdhci_writeb(struct sdhci_host *host, u8 val, int reg)
 {
-	return MIN_FREQ;
+	u32 oldval = readl(host->ioaddr + (reg & ~3));
+	u32 byte_shift = reg << 3 & 0x18;
+	u32 mask = 0xff << byte_shift;
+	u32 newval = (oldval & ~mask) | (val << byte_shift);
+
+	writel(newval, host->ioaddr + (reg & ~3));
 }
 
 static const struct sdhci_ops bcm2835_sdhci_ops = {
-	.write_l = bcm2835_sdhci_writel,
-	.write_w = bcm2835_sdhci_writew,
-	.write_b = bcm2835_sdhci_writeb,
 	.read_l = bcm2835_sdhci_readl,
 	.read_w = bcm2835_sdhci_readw,
 	.read_b = bcm2835_sdhci_readb,
+	.write_l = bcm2835_sdhci_writel,
+	.write_w = bcm2835_sdhci_writew,
+	.write_b = bcm2835_sdhci_writeb,
 	.set_clock = sdhci_set_clock,
 	.get_max_clock = sdhci_pltfm_clk_get_max_clock,
-	.get_min_clock = bcm2835_sdhci_get_min_clock,
 	.set_bus_width = sdhci_set_bus_width,
 	.reset = sdhci_reset,
 	.set_uhs_signaling = sdhci_set_uhs_signaling,
 };
 
 static const struct sdhci_pltfm_data bcm2835_sdhci_pdata = {
-	.quirks = SDHCI_QUIRK_BROKEN_CARD_DETECTION |
+	.quirks = SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12 |
+		  SDHCI_QUIRK_BROKEN_CARD_DETECTION |
 		  SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK,
 	.ops = &bcm2835_sdhci_ops,
 };
@@ -148,7 +141,7 @@ static const struct sdhci_pltfm_data bcm2835_sdhci_pdata = {
 static int bcm2835_sdhci_probe(struct platform_device *pdev)
 {
 	struct sdhci_host *host;
-	struct bcm2835_sdhci *bcm2835_host;
+	struct bcm2835_sdhci_host *bcm2835_host;
 	struct sdhci_pltfm_host *pltfm_host;
 	int ret;
 
-- 
1.7.9.5

Re: [PATCH 1/1] mmc: sdhci-bcm2835: added quirk and removed udelay in write ops

[Stephen Warren]

On 10/15/2014 10:43 AM, Scott Branden wrote:
> Added quirk SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12 present in controller.
> Removed udelay in write ops by using shadow registers for 16 bit
> accesses to 32-bit registers (where necessary).
> Optimized 32-bit operations when doing 8/16 register accesses.

That's 2 or 3 unrelated changes. They'd be better as separate patches,
so that any issues that arise can be bisected down to the smaller changes.

> diff --git a/drivers/mmc/host/sdhci-bcm2835.c b/drivers/mmc/host/sdhci-bcm2835.c

>  /*
>   * The Arasan has a bugette whereby it may lose the content of successive
> + * writes to the same register that are within two SD-card clock cycles of
> + * each other (a clock domain crossing problem).  Problem does not happen with
                                                    ^ The?
See right >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> ^

> + * data.

Blank line to separate the paragraphs here, to be consistent with the
other paragraph break below?

> + * This wouldn't be a problem with the code except that we can only write the
> + * controller with 32-bit writes.  So two different 16-bit registers in the
> + * written back to back creates the problem.
>   *
> + * In reality, this only happens when a SDHCI_BLOCK_SIZE and SDHCI_BLOCK_COUNT
> + * are written followed by SDHCI_TRANSFER_MODE and SDHCI_COMMAND.

That seems like a rather risky assertion. Even if it's perfectly true
with the MMC core code right now, does the MMC core document a guarantee
that this will always be true? Even if we optimize the WAR for the issue
as you've done, I think we should still have code that validates that
the same register is never written back-to-back to detect this likely
very hard-to-debug problem.

> + * The BLOCK_SIZE and BLOCK_COUNT are meaningless until a command issued so
> + * the work around can be further optimized. We can keep shadow values of
> + * BLOCK_SIZE, BLOCK_COUNT, and TRANSFER_MODE until a COMMAND is issued.
> + * Then, write the BLOCK_SIZE+BLOCK_COUNT in a single 32-bit write followed
> + * by the TRANSFER+COMMAND in another 32-bit write.
>   */

After this patch, the entire WAR for this issue is contained within
bcm2835_sdhci_writew(). It might be a good idea to move the comment next
to that function so it's more at hand to explain the code that's there.
Or at least add a comment to that function the to mention the location
of the explanation for the complex code.

>  static inline u32 bcm2835_sdhci_readl(struct sdhci_host *host, int reg)
>  {
>  	u32 val = readl(host->ioaddr + reg);
> @@ -71,76 +57,83 @@ static inline u32 bcm2835_sdhci_readl(struct sdhci_host *host, int reg)
>  	return val;
>  }
>  
> -static void bcm2835_sdhci_writew(struct sdhci_host *host, u16 val, int reg)
> -{
... (entire function deleted)
> -}

This patch could be a lot smaller if it didn't re-order the functions at
the same time. It makes the patch harder to understand. If you must
re-order the functions, perhaps make that a separate patch that does
nothing else, so that the actual code changes are easier to see?

>  static u16 bcm2835_sdhci_readw(struct sdhci_host *host, int reg)
>  {
> -	u32 val = bcm2835_sdhci_readl(host, (reg & ~3));
> -	u32 word_num = (reg >> 1) & 1;
> -	u32 word_shift = word_num * 16;
> -	u32 word = (val >> word_shift) & 0xffff;
> -
> +	u32 val = bcm2835_sdhci_readl(host->ioaddr, (reg & ~3));

The change from host to host->ioaddr ends up passing the wrong value to
bcm2835_sdhci_readl(). This causes the kernel to crash during boot.

The compiler doesn't warn about this because host->ioaddr is void, so
can be automatically converted to struct sdhci_host *.

> +	u16 word = val >> (reg << 3 & 0x18) & 0xffff;
>  	return word;
>  }

To be honest, I think the existing code is a bit clearer, since it uses
variables with names to explain all the intermediate values. Assuming
the compiler is competent (which admittedly I haven't checked) I would
expect the same code to be generated either way, or at least something
pretty similar. Did you measure the benefit of the optimization?

> +static void bcm2835_sdhci_writew(struct sdhci_host *host, u16 val, int reg)
>  {
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct bcm2835_sdhci_host *bcm2835_host = pltfm_host->priv;
> +	u32 word_shift = reg << 3 & 0x18;
> +	u32 mask = 0xffff << word_shift;
> +	u32 oldval;
> +	u32 newval;
> +
> +	if (reg == SDHCI_COMMAND) {
> +		if (bcm2835_host->shadow_blk != 0) {
> +			writel(bcm2835_host->shadow_blk,
> +			       host->ioaddr + SDHCI_BLOCK_SIZE);
> +			bcm2835_host->shadow_blk = 0;
> +		}

Is it absolutely guaranteed that there's never a need to write 0 to that
register? I can see that no data transfer command is likely to transfer
0 blocks. I assume no other type of command uses that register as a
parameter?

Re: [PATCH 1/1] mmc: sdhci-bcm2835: added quirk and removed udelay in write ops

[Scott Branden]

Great review - thanks.

On 14-10-17 07:37 PM, Stephen Warren wrote:
> On 10/15/2014 10:43 AM, Scott Branden wrote:
>> Added quirk SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12 present in controller.
>> Removed udelay in write ops by using shadow registers for 16 bit
>> accesses to 32-bit registers (where necessary).
>> Optimized 32-bit operations when doing 8/16 register accesses.
>
> That's 2 or 3 unrelated changes. They'd be better as separate patches,
> so that any issues that arise can be bisected down to the smaller changes.
OK - I will split into smaller patches to bisect and understand better.
>
>> diff --git a/drivers/mmc/host/sdhci-bcm2835.c b/drivers/mmc/host/sdhci-bcm2835.c
>
>>   /*
>>    * The Arasan has a bugette whereby it may lose the content of successive
>> + * writes to the same register that are within two SD-card clock cycles of
>> + * each other (a clock domain crossing problem).  Problem does not happen with
>                                                      ^ The?
> See right >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> ^
>
>> + * data.
>
> Blank line to separate the paragraphs here, to be consistent with the
> other paragraph break below?
I'll clean up the comment some more.
>
>> + * This wouldn't be a problem with the code except that we can only write the
>> + * controller with 32-bit writes.  So two different 16-bit registers in the
>> + * written back to back creates the problem.
>>    *
>> + * In reality, this only happens when a SDHCI_BLOCK_SIZE and SDHCI_BLOCK_COUNT
>> + * are written followed by SDHCI_TRANSFER_MODE and SDHCI_COMMAND.
>
> That seems like a rather risky assertion. Even if it's perfectly true
> with the MMC core code right now, does the MMC core document a guarantee
> that this will always be true? Even if we optimize the WAR for the issue
> as you've done, I think we should still have code that validates that
> the same register is never written back-to-back to detect this likely
> very hard-to-debug problem.
You're right - nothing in life is guaranteed.  We had test code for 
this.  I'll add a config option (default on) that verifies back to back 
writes do not occur.
>
>> + * The BLOCK_SIZE and BLOCK_COUNT are meaningless until a command issued so
>> + * the work around can be further optimized. We can keep shadow values of
>> + * BLOCK_SIZE, BLOCK_COUNT, and TRANSFER_MODE until a COMMAND is issued.
>> + * Then, write the BLOCK_SIZE+BLOCK_COUNT in a single 32-bit write followed
>> + * by the TRANSFER+COMMAND in another 32-bit write.
>>    */
>
> After this patch, the entire WAR for this issue is contained within
> bcm2835_sdhci_writew(). It might be a good idea to move the comment next
> to that function so it's more at hand to explain the code that's there.
> Or at least add a comment to that function the to mention the location
> of the explanation for the complex code.
ok, I'll clean up the comment a little more too.
>
>>   static inline u32 bcm2835_sdhci_readl(struct sdhci_host *host, int reg)
>>   {
>>   	u32 val = readl(host->ioaddr + reg);
>> @@ -71,76 +57,83 @@ static inline u32 bcm2835_sdhci_readl(struct sdhci_host *host, int reg)
>>   	return val;
>>   }
>>
>> -static void bcm2835_sdhci_writew(struct sdhci_host *host, u16 val, int reg)
>> -{
> ... (entire function deleted)
>> -}
>
> This patch could be a lot smaller if it didn't re-order the functions at
> the same time. It makes the patch harder to understand. If you must
> re-order the functions, perhaps make that a separate patch that does
> nothing else, so that the actual code changes are easier to see?
ok
>
>>   static u16 bcm2835_sdhci_readw(struct sdhci_host *host, int reg)
>>   {
>> -	u32 val = bcm2835_sdhci_readl(host, (reg & ~3));
>> -	u32 word_num = (reg >> 1) & 1;
>> -	u32 word_shift = word_num * 16;
>> -	u32 word = (val >> word_shift) & 0xffff;
>> -
>> +	u32 val = bcm2835_sdhci_readl(host->ioaddr, (reg & ~3));
>
> The change from host to host->ioaddr ends up passing the wrong value to
> bcm2835_sdhci_readl(). This causes the kernel to crash during boot.
I see that now.  Will fix - unfortunately I ported from an existing 
driver that doesn't need the bcm2835_shdci_readl function.
>
> The compiler doesn't warn about this because host->ioaddr is void, so
> can be automatically converted to struct sdhci_host *.
>
>> +	u16 word = val >> (reg << 3 & 0x18) & 0xffff;
>>   	return word;
>>   }
>
> To be honest, I think the existing code is a bit clearer, since it uses
> variables with names to explain all the intermediate values. Assuming
> the compiler is competent (which admittedly I haven't checked) I would
> expect the same code to be generated either way, or at least something
> pretty similar. Did you measure the benefit of the optimization?
By optimize I meant use the same bit calculation instead of doing 
different calculations for the same operation.  I'll create a macro to 
make it clearer to see.
>
>> +static void bcm2835_sdhci_writew(struct sdhci_host *host, u16 val, int reg)
>>   {
>> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> +	struct bcm2835_sdhci_host *bcm2835_host = pltfm_host->priv;
>> +	u32 word_shift = reg << 3 & 0x18;
>> +	u32 mask = 0xffff << word_shift;
>> +	u32 oldval;
>> +	u32 newval;
>> +
>> +	if (reg == SDHCI_COMMAND) {
>> +		if (bcm2835_host->shadow_blk != 0) {
>> +			writel(bcm2835_host->shadow_blk,
>> +			       host->ioaddr + SDHCI_BLOCK_SIZE);
>> +			bcm2835_host->shadow_blk = 0;
>> +		}
>
> Is it absolutely guaranteed that there's never a need to write 0 to that
> register? I can see that no data transfer command is likely to transfer
> 0 blocks. I assume no other type of command uses that register as a
> parameter?
Correct.
>

[PATCHv2 0/5] sdhci-bcm2835: added quirk and removed udelay in write ops

[Scott Branden]

This patch contains driver cleanup of sdhci-bcm2835.
Please note that this has not actually been tested on bcm2835 yet.
Testing comes from other devices with the same sdhci controller.

This patch is being put out for testing and acceptance on the 2835.
Please test and comment.

Scott Branden (5):
  mmc: sdhci-bcm2835: group read and write functions to improve
    readability
  mmc: sdhci-bcm2835: make shift calculations consistent
  mmc: shdci-bcm2835: add efficient back-to-back write workaround
  mmc: shdci-bcm2835: add verify for 32-bit back-to-back workaround
  mmc: sdhci-bcm2835: add sdhci quirk
    SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12

 drivers/mmc/host/Kconfig         |    9 ++
 drivers/mmc/host/sdhci-bcm2835.c |  172 +++++++++++++++++++++-----------------
 2 files changed, 105 insertions(+), 76 deletions(-)

-- 
1.7.9.5

[PATCHv2 1/5] mmc: sdhci-bcm2835: group read and write functions to improve readability

[Scott Branden]

Group the read and write functions to improve readability.  Now all
similar functions are grouped together to evaluate behaviours.

Signed-off-by: Scott Branden <sbranden@broadcom.com>
---
 drivers/mmc/host/sdhci-bcm2835.c |   61 +++++++++++++++++++-------------------
 1 file changed, 31 insertions(+), 30 deletions(-)

diff --git a/drivers/mmc/host/sdhci-bcm2835.c b/drivers/mmc/host/sdhci-bcm2835.c
index 439d259..c8ee02c 100644
--- a/drivers/mmc/host/sdhci-bcm2835.c
+++ b/drivers/mmc/host/sdhci-bcm2835.c
@@ -54,13 +54,6 @@ struct bcm2835_sdhci {
 	u32 shadow;
 };
 
-static void bcm2835_sdhci_writel(struct sdhci_host *host, u32 val, int reg)
-{
-	writel(val, host->ioaddr + reg);
-
-	udelay(BCM2835_SDHCI_WRITE_DELAY);
-}
-
 static inline u32 bcm2835_sdhci_readl(struct sdhci_host *host, int reg)
 {
 	u32 val = readl(host->ioaddr + reg);
@@ -71,6 +64,34 @@ static inline u32 bcm2835_sdhci_readl(struct sdhci_host *host, int reg)
 	return val;
 }
 
+static u16 bcm2835_sdhci_readw(struct sdhci_host *host, int reg)
+{
+	u32 val = bcm2835_sdhci_readl(host, (reg & ~3));
+	u32 word_num = (reg >> 1) & 1;
+	u32 word_shift = word_num * 16;
+	u32 word = (val >> word_shift) & 0xffff;
+
+	return word;
+}
+
+static u8 bcm2835_sdhci_readb(struct sdhci_host *host, int reg)
+{
+	u32 val = bcm2835_sdhci_readl(host, (reg & ~3));
+	u32 byte_num = reg & 3;
+	u32 byte_shift = byte_num * 8;
+	u32 byte = (val >> byte_shift) & 0xff;
+
+	return byte;
+}
+
+static void bcm2835_sdhci_writel(struct sdhci_host *host, u32 val, int reg)
+{
+	writel(val, host->ioaddr + reg);
+
+	udelay(BCM2835_SDHCI_WRITE_DELAY);
+}
+
+
 static void bcm2835_sdhci_writew(struct sdhci_host *host, u16 val, int reg)
 {
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
@@ -88,16 +109,6 @@ static void bcm2835_sdhci_writew(struct sdhci_host *host, u16 val, int reg)
 		bcm2835_sdhci_writel(host, newval, reg & ~3);
 }
 
-static u16 bcm2835_sdhci_readw(struct sdhci_host *host, int reg)
-{
-	u32 val = bcm2835_sdhci_readl(host, (reg & ~3));
-	u32 word_num = (reg >> 1) & 1;
-	u32 word_shift = word_num * 16;
-	u32 word = (val >> word_shift) & 0xffff;
-
-	return word;
-}
-
 static void bcm2835_sdhci_writeb(struct sdhci_host *host, u8 val, int reg)
 {
 	u32 oldval = bcm2835_sdhci_readl(host, reg & ~3);
@@ -109,28 +120,18 @@ static void bcm2835_sdhci_writeb(struct sdhci_host *host, u8 val, int reg)
 	bcm2835_sdhci_writel(host, newval, reg & ~3);
 }
 
-static u8 bcm2835_sdhci_readb(struct sdhci_host *host, int reg)
-{
-	u32 val = bcm2835_sdhci_readl(host, (reg & ~3));
-	u32 byte_num = reg & 3;
-	u32 byte_shift = byte_num * 8;
-	u32 byte = (val >> byte_shift) & 0xff;
-
-	return byte;
-}
-
 static unsigned int bcm2835_sdhci_get_min_clock(struct sdhci_host *host)
 {
 	return MIN_FREQ;
 }
 
 static const struct sdhci_ops bcm2835_sdhci_ops = {
-	.write_l = bcm2835_sdhci_writel,
-	.write_w = bcm2835_sdhci_writew,
-	.write_b = bcm2835_sdhci_writeb,
 	.read_l = bcm2835_sdhci_readl,
 	.read_w = bcm2835_sdhci_readw,
 	.read_b = bcm2835_sdhci_readb,
+	.write_l = bcm2835_sdhci_writel,
+	.write_w = bcm2835_sdhci_writew,
+	.write_b = bcm2835_sdhci_writeb,
 	.set_clock = sdhci_set_clock,
 	.get_max_clock = sdhci_pltfm_clk_get_max_clock,
 	.get_min_clock = bcm2835_sdhci_get_min_clock,
-- 
1.7.9.5

[PATCHv2 2/5] mmc: sdhci-bcm2835: make shift calculations consistent

[Scott Branden]

Make the shift calculations consistent rather than having different
implementations to calculate the same thing.

Signed-off-by: Scott Branden <sbranden@broadcom.com>
---
 drivers/mmc/host/sdhci-bcm2835.c |   18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/mmc/host/sdhci-bcm2835.c b/drivers/mmc/host/sdhci-bcm2835.c
index c8ee02c..b6cb365 100644
--- a/drivers/mmc/host/sdhci-bcm2835.c
+++ b/drivers/mmc/host/sdhci-bcm2835.c
@@ -54,6 +54,8 @@ struct bcm2835_sdhci {
 	u32 shadow;
 };
 
+#define REG_OFFSET_IN_BITS(reg) ((reg) << 3 & 0x18)
+
 static inline u32 bcm2835_sdhci_readl(struct sdhci_host *host, int reg)
 {
 	u32 val = readl(host->ioaddr + reg);
@@ -67,20 +69,14 @@ static inline u32 bcm2835_sdhci_readl(struct sdhci_host *host, int reg)
 static u16 bcm2835_sdhci_readw(struct sdhci_host *host, int reg)
 {
 	u32 val = bcm2835_sdhci_readl(host, (reg & ~3));
-	u32 word_num = (reg >> 1) & 1;
-	u32 word_shift = word_num * 16;
-	u32 word = (val >> word_shift) & 0xffff;
-
+	u16 word = val >> REG_OFFSET_IN_BITS(reg) & 0xffff;
 	return word;
 }
 
 static u8 bcm2835_sdhci_readb(struct sdhci_host *host, int reg)
 {
 	u32 val = bcm2835_sdhci_readl(host, (reg & ~3));
-	u32 byte_num = reg & 3;
-	u32 byte_shift = byte_num * 8;
-	u32 byte = (val >> byte_shift) & 0xff;
-
+	u8 byte = val >> REG_OFFSET_IN_BITS(reg) & 0xff;
 	return byte;
 }
 
@@ -98,8 +94,7 @@ static void bcm2835_sdhci_writew(struct sdhci_host *host, u16 val, int reg)
 	struct bcm2835_sdhci *bcm2835_host = pltfm_host->priv;
 	u32 oldval = (reg == SDHCI_COMMAND) ? bcm2835_host->shadow :
 		bcm2835_sdhci_readl(host, reg & ~3);
-	u32 word_num = (reg >> 1) & 1;
-	u32 word_shift = word_num * 16;
+	u32 word_shift = REG_OFFSET_IN_BITS(reg);
 	u32 mask = 0xffff << word_shift;
 	u32 newval = (oldval & ~mask) | (val << word_shift);
 
@@ -112,8 +107,7 @@ static void bcm2835_sdhci_writew(struct sdhci_host *host, u16 val, int reg)
 static void bcm2835_sdhci_writeb(struct sdhci_host *host, u8 val, int reg)
 {
 	u32 oldval = bcm2835_sdhci_readl(host, reg & ~3);
-	u32 byte_num = reg & 3;
-	u32 byte_shift = byte_num * 8;
+	u32 byte_shift = REG_OFFSET_IN_BITS(reg);
 	u32 mask = 0xff << byte_shift;
 	u32 newval = (oldval & ~mask) | (val << byte_shift);
 
-- 
1.7.9.5

Re: [PATCHv2 2/5] mmc: sdhci-bcm2835: make shift calculations consistent

[Stephen Warren]

On 10/30/2014 12:36 AM, Scott Branden wrote:
> Make the shift calculations consistent rather than having different
> implementations to calculate the same thing.

> diff --git a/drivers/mmc/host/sdhci-bcm2835.c b/drivers/mmc/host/sdhci-bcm2835.c

> +#define REG_OFFSET_IN_BITS(reg) ((reg) << 3 & 0x18)

This should really be the following so people don't have to memorize
operator precedence:

#define REG_OFFSET_IN_BITS(reg) (((reg) << 3) & 0x18)

(I've been bit by people mis-remembering precedence in very similar code...)

Re: [PATCHv2 2/5] mmc: sdhci-bcm2835: make shift calculations consistent

[Scott Branden]

On 14-11-04 08:48 PM, Stephen Warren wrote:
> On 10/30/2014 12:36 AM, Scott Branden wrote:
>> Make the shift calculations consistent rather than having different
>> implementations to calculate the same thing.
>
>> diff --git a/drivers/mmc/host/sdhci-bcm2835.c b/drivers/mmc/host/sdhci-bcm2835.c
>
>> +#define REG_OFFSET_IN_BITS(reg) ((reg) << 3 & 0x18)
>
> This should really be the following so people don't have to memorize
> operator precedence:
>
> #define REG_OFFSET_IN_BITS(reg) (((reg) << 3) & 0x18)
>
> (I've been bit by people mis-remembering precedence in very similar code...)
>
Good idea.

[PATCHv2 3/5] mmc: shdci-bcm2835: add efficient back-to-back write workaround

[Scott Branden]

The bcm2835 has clock domain issues when back to back writes to certain
registers are written.  The existing driver works around this issue with
udelay.  A more efficient method is to store the 8 and 16 bit writes
to the registers affected and then write them as 32 bits at the appropriate
time.

Signed-off-by: Scott Branden <sbranden@broadcom.com>
---
 drivers/mmc/host/sdhci-bcm2835.c |  103 ++++++++++++++++++++------------------
 1 file changed, 55 insertions(+), 48 deletions(-)

diff --git a/drivers/mmc/host/sdhci-bcm2835.c b/drivers/mmc/host/sdhci-bcm2835.c
index b6cb365..f8c450a 100644
--- a/drivers/mmc/host/sdhci-bcm2835.c
+++ b/drivers/mmc/host/sdhci-bcm2835.c
@@ -24,34 +24,9 @@
 #include <linux/mmc/host.h>
 #include "sdhci-pltfm.h"
 
-/*
- * 400KHz is max freq for card ID etc. Use that as min card clock. We need to
- * know the min to enable static calculation of max BCM2835_SDHCI_WRITE_DELAY.
- */
-#define MIN_FREQ 400000
-
-/*
- * The Arasan has a bugette whereby it may lose the content of successive
- * writes to registers that are within two SD-card clock cycles of each other
- * (a clock domain crossing problem). It seems, however, that the data
- * register does not have this problem, which is just as well - otherwise we'd
- * have to nobble the DMA engine too.
- *
- * This should probably be dynamically calculated based on the actual card
- * frequency. However, this is the longest we'll have to wait, and doesn't
- * seem to slow access down too much, so the added complexity doesn't seem
- * worth it for now.
- *
- * 1/MIN_FREQ is (max) time per tick of eMMC clock.
- * 2/MIN_FREQ is time for two ticks.
- * Multiply by 1000000 to get uS per two ticks.
- * *1000000 for uSecs.
- * +1 for hack rounding.
- */
-#define BCM2835_SDHCI_WRITE_DELAY	(((2 * 1000000) / MIN_FREQ) + 1)
-
-struct bcm2835_sdhci {
-	u32 shadow;
+struct bcm2835_sdhci_host {
+	u32 shadow_cmd;
+	u32 shadow_blk;
 };
 
 #define REG_OFFSET_IN_BITS(reg) ((reg) << 3 & 0x18)
@@ -80,33 +55,71 @@ static u8 bcm2835_sdhci_readb(struct sdhci_host *host, int reg)
 	return byte;
 }
 
-static void bcm2835_sdhci_writel(struct sdhci_host *host, u32 val, int reg)
+static inline void bcm2835_sdhci_writel(struct sdhci_host *host,
+						u32 val, int reg)
 {
 	writel(val, host->ioaddr + reg);
-
-	udelay(BCM2835_SDHCI_WRITE_DELAY);
 }
 
-
+/*
+ * The Arasan has a bugette whereby it may lose the content of successive
+ * writes to the same register that are within two SD-card clock cycles of
+ * each other (a clock domain crossing problem). The data
+ * register does not have this problem, which is just as well - otherwise we'd
+ * have to nobble the DMA engine too.
+ *
+ * This wouldn't be a problem with the code except that we can only write the
+ * controller with 32-bit writes.  So two different 16-bit registers are
+ * written back to back creates the problem.
+ *
+ * In reality, this only happens when SDHCI_BLOCK_SIZE and SDHCI_BLOCK_COUNT
+ * are written followed by SDHCI_TRANSFER_MODE and SDHCI_COMMAND.
+ * The BLOCK_SIZE and BLOCK_COUNT are meaningless until a command issued so
+ * the work around can be further optimized. We can keep shadow values of
+ * BLOCK_SIZE, BLOCK_COUNT, and TRANSFER_MODE until a COMMAND is issued.
+ * Then, write the BLOCK_SIZE+BLOCK_COUNT in a single 32-bit write followed
+ * by the TRANSFER+COMMAND in another 32-bit write.
+ */
 static void bcm2835_sdhci_writew(struct sdhci_host *host, u16 val, int reg)
 {
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
-	struct bcm2835_sdhci *bcm2835_host = pltfm_host->priv;
-	u32 oldval = (reg == SDHCI_COMMAND) ? bcm2835_host->shadow :
-		bcm2835_sdhci_readl(host, reg & ~3);
+	struct bcm2835_sdhci_host *bcm2835_host = pltfm_host->priv;
 	u32 word_shift = REG_OFFSET_IN_BITS(reg);
 	u32 mask = 0xffff << word_shift;
-	u32 newval = (oldval & ~mask) | (val << word_shift);
-
-	if (reg == SDHCI_TRANSFER_MODE)
-		bcm2835_host->shadow = newval;
-	else
+	u32 oldval, newval;
+
+	if (reg == SDHCI_COMMAND) {
+		/* Write the block now as we are issuing a command */
+		if (bcm2835_host->shadow_blk != 0) {
+			bcm2835_sdhci_writel(host, bcm2835_host->shadow_blk,
+				SDHCI_BLOCK_SIZE);
+			bcm2835_host->shadow_blk = 0;
+		}
+		oldval = bcm2835_host->shadow_cmd;
+	} else if (reg == SDHCI_BLOCK_SIZE || reg == SDHCI_BLOCK_COUNT) {
+		/* Block size and count are stored in shadow reg */
+		oldval = bcm2835_host->shadow_blk;
+	} else {
+		/* Read reg, all other registers are not shadowed */
+		oldval = readl(host->ioaddr + (reg & ~3));
+	}
+	newval = (oldval & ~mask) | (val << word_shift);
+
+	if (reg == SDHCI_TRANSFER_MODE) {
+		/* Save the transfer mode until the command is issued */
+		bcm2835_host->shadow_cmd = newval;
+	} else if (reg == SDHCI_BLOCK_SIZE || reg == SDHCI_BLOCK_COUNT) {
+		/* Save the block info until the command is issued */
+		bcm2835_host->shadow_blk = newval;
+	} else {
+		/* Command or other regular 32-bit write */
 		bcm2835_sdhci_writel(host, newval, reg & ~3);
+	}
 }
 
 static void bcm2835_sdhci_writeb(struct sdhci_host *host, u8 val, int reg)
 {
-	u32 oldval = bcm2835_sdhci_readl(host, reg & ~3);
+	u32 oldval = readl(host->ioaddr + (reg & ~3));
 	u32 byte_shift = REG_OFFSET_IN_BITS(reg);
 	u32 mask = 0xff << byte_shift;
 	u32 newval = (oldval & ~mask) | (val << byte_shift);
@@ -114,11 +127,6 @@ static void bcm2835_sdhci_writeb(struct sdhci_host *host, u8 val, int reg)
 	bcm2835_sdhci_writel(host, newval, reg & ~3);
 }
 
-static unsigned int bcm2835_sdhci_get_min_clock(struct sdhci_host *host)
-{
-	return MIN_FREQ;
-}
-
 static const struct sdhci_ops bcm2835_sdhci_ops = {
 	.read_l = bcm2835_sdhci_readl,
 	.read_w = bcm2835_sdhci_readw,
@@ -128,7 +136,6 @@ static const struct sdhci_ops bcm2835_sdhci_ops = {
 	.write_b = bcm2835_sdhci_writeb,
 	.set_clock = sdhci_set_clock,
 	.get_max_clock = sdhci_pltfm_clk_get_max_clock,
-	.get_min_clock = bcm2835_sdhci_get_min_clock,
 	.set_bus_width = sdhci_set_bus_width,
 	.reset = sdhci_reset,
 	.set_uhs_signaling = sdhci_set_uhs_signaling,
@@ -143,7 +150,7 @@ static const struct sdhci_pltfm_data bcm2835_sdhci_pdata = {
 static int bcm2835_sdhci_probe(struct platform_device *pdev)
 {
 	struct sdhci_host *host;
-	struct bcm2835_sdhci *bcm2835_host;
+	struct bcm2835_sdhci_host *bcm2835_host;
 	struct sdhci_pltfm_host *pltfm_host;
 	int ret;
 
-- 
1.7.9.5

Re: [PATCHv2 3/5] mmc: shdci-bcm2835: add efficient back-to-back write workaround

[Stephen Warren]

On 10/30/2014 12:36 AM, Scott Branden wrote:
> The bcm2835 has clock domain issues when back to back writes to certain
> registers are written.  The existing driver works around this issue with
> udelay.  A more efficient method is to store the 8 and 16 bit writes
> to the registers affected and then write them as 32 bits at the appropriate
> time.

> diff --git a/drivers/mmc/host/sdhci-bcm2835.c b/drivers/mmc/host/sdhci-bcm2835.c

>  static void bcm2835_sdhci_writew(struct sdhci_host *host, u16 val, int reg)
>  {
>  	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> -	struct bcm2835_sdhci *bcm2835_host = pltfm_host->priv;
> -	u32 oldval = (reg == SDHCI_COMMAND) ? bcm2835_host->shadow :
> -		bcm2835_sdhci_readl(host, reg & ~3);
> +	struct bcm2835_sdhci_host *bcm2835_host = pltfm_host->priv;

Is that type change for bcm2835_host really correct?

> +	} else {
> +		/* Read reg, all other registers are not shadowed */
> +		oldval = readl(host->ioaddr + (reg & ~3));

Is there any reason to use readl() directly here rather than calling
bcm2835_readl()? ...

>  static void bcm2835_sdhci_writeb(struct sdhci_host *host, u8 val, int reg)
>  {
> -	u32 oldval = bcm2835_sdhci_readl(host, reg & ~3);
> +	u32 oldval = readl(host->ioaddr + (reg & ~3));

... and here in particular, since this seems like an unrelated change?

>  static int bcm2835_sdhci_probe(struct platform_device *pdev)
>  {
>  	struct sdhci_host *host;
> -	struct bcm2835_sdhci *bcm2835_host;
> +	struct bcm2835_sdhci_host *bcm2835_host;

Is that type change for bcm2835_host really correct?

Re: [PATCHv2 3/5] mmc: shdci-bcm2835: add efficient back-to-back write workaround

[Scott Branden]

On 14-11-04 08:57 PM, Stephen Warren wrote:
> On 10/30/2014 12:36 AM, Scott Branden wrote:
>> The bcm2835 has clock domain issues when back to back writes to certain
>> registers are written.  The existing driver works around this issue with
>> udelay.  A more efficient method is to store the 8 and 16 bit writes
>> to the registers affected and then write them as 32 bits at the appropriate
>> time.
>
>> diff --git a/drivers/mmc/host/sdhci-bcm2835.c b/drivers/mmc/host/sdhci-bcm2835.c
>
>>   static void bcm2835_sdhci_writew(struct sdhci_host *host, u16 val, int reg)
>>   {
>>   	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> -	struct bcm2835_sdhci *bcm2835_host = pltfm_host->priv;
>> -	u32 oldval = (reg == SDHCI_COMMAND) ? bcm2835_host->shadow :
>> -		bcm2835_sdhci_readl(host, reg & ~3);
>> +	struct bcm2835_sdhci_host *bcm2835_host = pltfm_host->priv;
>
> Is that type change for bcm2835_host really correct?
Yes - at the top of the patch the structure has been expanded and named 
appropriately.

-struct bcm2835_sdhci {
-	u32 shadow;
+struct bcm2835_sdhci_host {
+	u32 shadow_cmd;
+	u32 shadow_blk;
  };
>
>> +	} else {
>> +		/* Read reg, all other registers are not shadowed */
>> +		oldval = readl(host->ioaddr + (reg & ~3));
>
> Is there any reason to use readl() directly here rather than calling
> bcm2835_readl()? ...
Yes, bcm2835_readl does not need to be called in read-modify-write and 
shadow register situations and just adds overhead.  All that needs to be 
called is readl.  bcm2835_readl has some existing ugly code in it to 
modify the capabilities register on a read function.  This info never 
needs to be for write as you can't overwrite the capabilities register. 
  I hope to get rid of the capabilities hack in a future patch as this 
should never have been acceptable in upstreamed code to begin with.  The 
capabilities override should have been passed in through a device tree 
entry.
>
>>   static void bcm2835_sdhci_writeb(struct sdhci_host *host, u8 val, int reg)
>>   {
>> -	u32 oldval = bcm2835_sdhci_readl(host, reg & ~3);
>> +	u32 oldval = readl(host->ioaddr + (reg & ~3));
>
> ... and here in particular, since this seems like an unrelated change?
Same situation with bcm2835_readl above.  No need to call in 
read-modify-write situations.
>
>>   static int bcm2835_sdhci_probe(struct platform_device *pdev)
>>   {
>>   	struct sdhci_host *host;
>> -	struct bcm2835_sdhci *bcm2835_host;
>> +	struct bcm2835_sdhci_host *bcm2835_host;
>
> Is that type change for bcm2835_host really correct?
>
yes - structure renamed above

Re: [PATCHv2 3/5] mmc: shdci-bcm2835: add efficient back-to-back write workaround

[Stephen Warren]

On 11/04/2014 11:55 PM, Scott Branden wrote:
> On 14-11-04 08:57 PM, Stephen Warren wrote:
>> On 10/30/2014 12:36 AM, Scott Branden wrote:
>>> The bcm2835 has clock domain issues when back to back writes to certain
>>> registers are written.  The existing driver works around this issue with
>>> udelay.  A more efficient method is to store the 8 and 16 bit writes
>>> to the registers affected and then write them as 32 bits at the
>>> appropriate
>>> time.
>>
>>> diff --git a/drivers/mmc/host/sdhci-bcm2835.c
>>> b/drivers/mmc/host/sdhci-bcm2835.c
>>
>>>   static void bcm2835_sdhci_writew(struct sdhci_host *host, u16 val,
>>> int reg)
>>>   {
>>>       struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>> -    struct bcm2835_sdhci *bcm2835_host = pltfm_host->priv;
>>> -    u32 oldval = (reg == SDHCI_COMMAND) ? bcm2835_host->shadow :
>>> -        bcm2835_sdhci_readl(host, reg & ~3);
>>> +    struct bcm2835_sdhci_host *bcm2835_host = pltfm_host->priv;
>>
>> Is that type change for bcm2835_host really correct?
>
> Yes - at the top of the patch the structure has been expanded and named
> appropriately.
> 
> -struct bcm2835_sdhci {
> -    u32 shadow;
> +struct bcm2835_sdhci_host {
> +    u32 shadow_cmd;
> +    u32 shadow_blk;
>  };

Ah yes, sorry for missing that.

>>> +    } else {
>>> +        /* Read reg, all other registers are not shadowed */
>>> +        oldval = readl(host->ioaddr + (reg & ~3));
>>
>> Is there any reason to use readl() directly here rather than calling
>> bcm2835_readl()? ...
>
> Yes, bcm2835_readl does not need to be called in read-modify-write and
> shadow register situations and just adds overhead.  All that needs to be
> called is readl.  bcm2835_readl has some existing ugly code in it to
> modify the capabilities register on a read function.  This info never
> needs to be for write as you can't overwrite the capabilities register.

To be honest, it seems better to do all the read/write through
consistent functions. One advantage of bcm2835_readl() is that it
consistently adds on the base address internally so you don't have to
write it out every time manually. Still, the code ought to work fine
after this change, so I guess it's OK.

> I hope to get rid of the capabilities hack in a future patch as this
> should never have been acceptable in upstreamed code to begin with.  The
> capabilities override should have been passed in through a device tree
> entry.

It's a pretty common technique with precedent. I certainly don't agree
that it should be configured by DT. Arguably, DT makes sense to describe
board-to-board variations, but there's almost zero point putting data
into DT that is SoC description rather than board description; just put
it into the driver to avoid continually parsing the same data over and
over from DT just to get back to the same data that could have been
encoded into the driver. If the data varies between similar controllers,
an of_match table can easily be used to parameterize it based on
compatible value.

Re: [PATCHv2 3/5] mmc: shdci-bcm2835: add efficient back-to-back write workaround

[Scott Branden]

On 14-11-05 08:48 PM, Stephen Warren wrote:
> On 11/04/2014 11:55 PM, Scott Branden wrote:
>> On 14-11-04 08:57 PM, Stephen Warren wrote:
>>> On 10/30/2014 12:36 AM, Scott Branden wrote:
>>>> The bcm2835 has clock domain issues when back to back writes to certain
>>>> registers are written.  The existing driver works around this issue with
>>>> udelay.  A more efficient method is to store the 8 and 16 bit writes
>>>> to the registers affected and then write them as 32 bits at the
>>>> appropriate
>>>> time.
>>>
>>>> diff --git a/drivers/mmc/host/sdhci-bcm2835.c
>>>> b/drivers/mmc/host/sdhci-bcm2835.c
>>>
>>>>    static void bcm2835_sdhci_writew(struct sdhci_host *host, u16 val,
>>>> int reg)
>>>>    {
>>>>        struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>>> -    struct bcm2835_sdhci *bcm2835_host = pltfm_host->priv;
>>>> -    u32 oldval = (reg == SDHCI_COMMAND) ? bcm2835_host->shadow :
>>>> -        bcm2835_sdhci_readl(host, reg & ~3);
>>>> +    struct bcm2835_sdhci_host *bcm2835_host = pltfm_host->priv;
>>>
>>> Is that type change for bcm2835_host really correct?
>>
>> Yes - at the top of the patch the structure has been expanded and named
>> appropriately.
>>
>> -struct bcm2835_sdhci {
>> -    u32 shadow;
>> +struct bcm2835_sdhci_host {
>> +    u32 shadow_cmd;
>> +    u32 shadow_blk;
>>   };
>
> Ah yes, sorry for missing that.
>
>>>> +    } else {
>>>> +        /* Read reg, all other registers are not shadowed */
>>>> +        oldval = readl(host->ioaddr + (reg & ~3));
>>>
>>> Is there any reason to use readl() directly here rather than calling
>>> bcm2835_readl()? ...
>>
>> Yes, bcm2835_readl does not need to be called in read-modify-write and
>> shadow register situations and just adds overhead.  All that needs to be
>> called is readl.  bcm2835_readl has some existing ugly code in it to
>> modify the capabilities register on a read function.  This info never
>> needs to be for write as you can't overwrite the capabilities register.
>
> To be honest, it seems better to do all the read/write through
> consistent functions. One advantage of bcm2835_readl() is that it
> consistently adds on the base address internally so you don't have to
> write it out every time manually. Still, the code ought to work fine
> after this change, so I guess it's OK.
>
>> I hope to get rid of the capabilities hack in a future patch as this
>> should never have been acceptable in upstreamed code to begin with.  The
>> capabilities override should have been passed in through a device tree
>> entry.
>
> It's a pretty common technique with precedent. I certainly don't agree
> that it should be configured by DT. Arguably, DT makes sense to describe
> board-to-board variations, but there's almost zero point putting data
> into DT that is SoC description rather than board description; just put
> it into the driver to avoid continually parsing the same data over and
> over from DT just to get back to the same data that could have been
> encoded into the driver. If the data varies between similar controllers,
> an of_match table can easily be used to parameterize it based on
> compatible value.
There is work to be done here or I will be unable to use this driver in 
our chipsets.  Perhaps it will be easier having another driver 
actually... as the DMA seems quite different on RPI.
>

[PATCHv2 4/5] mmc: shdci-bcm2835: add verify for 32-bit back-to-back workaround

[Scott Branden]

Add a verify option to driver to print out an error message if a
potential back to back write could cause a clock domain issue.

Signed-off-by: Scott Branden <sbranden@broadcom.com>
---
 drivers/mmc/host/Kconfig         |    9 +++++++++
 drivers/mmc/host/sdhci-bcm2835.c |   17 +++++++++++++++++
 2 files changed, 26 insertions(+)

diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index 1386065..020de98 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -292,6 +292,15 @@ config MMC_SDHCI_BCM2835
 
 	  If unsure, say N.
 
+config MMC_SDHCI_BCM2835_VERIFY_WORKAROUND
+	bool "Verify BCM2835 workaround does not do back to back writes"
+	depends on MMC_SDHCI_BCM2835
+	default y
+	help
+	  This enables code that verifies the bcm2835 workaround.
+	  The verification code checks that back to back writes to the same
+	  register do not occur.
+
 config MMC_MOXART
 	tristate "MOXART SD/MMC Host Controller support"
 	depends on ARCH_MOXART && MMC
diff --git a/drivers/mmc/host/sdhci-bcm2835.c b/drivers/mmc/host/sdhci-bcm2835.c
index f8c450a..11af27f 100644
--- a/drivers/mmc/host/sdhci-bcm2835.c
+++ b/drivers/mmc/host/sdhci-bcm2835.c
@@ -27,6 +27,9 @@
 struct bcm2835_sdhci_host {
 	u32 shadow_cmd;
 	u32 shadow_blk;
+#ifdef CONFIG_MMC_SDHCI_BCM2835_VERIFY_WORKAROUND
+	int previous_reg;
+#endif
 };
 
 #define REG_OFFSET_IN_BITS(reg) ((reg) << 3 & 0x18)
@@ -58,6 +61,20 @@ static u8 bcm2835_sdhci_readb(struct sdhci_host *host, int reg)
 static inline void bcm2835_sdhci_writel(struct sdhci_host *host,
 						u32 val, int reg)
 {
+#ifdef CONFIG_MMC_SDHCI_BCM2835_VERIFY_WORKAROUND
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct bcm2835_sdhci_host *bcm2835_host = pltfm_host->priv;
+
+	if (bcm2835_host->previous_reg == reg) {
+		if ((reg != SDHCI_HOST_CONTROL)
+			&& (reg != SDHCI_CLOCK_CONTROL)) {
+			dev_err(mmc_dev(host->mmc),
+			"back-to-back write to 0x%x\n", reg);
+		}
+	}
+	bcm2835_host->previous_reg = reg;
+#endif
+
 	writel(val, host->ioaddr + reg);
 }
 
-- 
1.7.9.5

Re: [PATCHv2 4/5] mmc: shdci-bcm2835: add verify for 32-bit back-to-back workaround

[Stephen Warren]

On 10/30/2014 12:36 AM, Scott Branden wrote:
> Add a verify option to driver to print out an error message if a
> potential back to back write could cause a clock domain issue.

> diff --git a/drivers/mmc/host/sdhci-bcm2835.c b/drivers/mmc/host/sdhci-bcm2835.c

>  static inline void bcm2835_sdhci_writel(struct sdhci_host *host,
>  						u32 val, int reg)
>  {
> +#ifdef CONFIG_MMC_SDHCI_BCM2835_VERIFY_WORKAROUND
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct bcm2835_sdhci_host *bcm2835_host = pltfm_host->priv;
> +
> +	if (bcm2835_host->previous_reg == reg) {
> +		if ((reg != SDHCI_HOST_CONTROL)
> +			&& (reg != SDHCI_CLOCK_CONTROL)) {
> +			dev_err(mmc_dev(host->mmc),
> +			"back-to-back write to 0x%x\n", reg);

This fires a *ton* on reg 0x20 and 0x30 on my rev 2 model B with the
patches applied on top of next-20141031. Without the patches applied,
everything works fine. As far as I can tell, SD card accesses no longer
work (or perhaps there's just so much log spew over serial that it takes
more than 1.5 minutes to get to the login prompt).

Re: [PATCHv2 4/5] mmc: shdci-bcm2835: add verify for 32-bit back-to-back workaround

[Scott Branden]

On 14-11-04 08:44 PM, Stephen Warren wrote:
> On 10/30/2014 12:36 AM, Scott Branden wrote:
>> Add a verify option to driver to print out an error message if a
>> potential back to back write could cause a clock domain issue.
>
>> diff --git a/drivers/mmc/host/sdhci-bcm2835.c b/drivers/mmc/host/sdhci-bcm2835.c
>
>>   static inline void bcm2835_sdhci_writel(struct sdhci_host *host,
>>   						u32 val, int reg)
>>   {
>> +#ifdef CONFIG_MMC_SDHCI_BCM2835_VERIFY_WORKAROUND
>> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> +	struct bcm2835_sdhci_host *bcm2835_host = pltfm_host->priv;
>> +
>> +	if (bcm2835_host->previous_reg == reg) {
>> +		if ((reg != SDHCI_HOST_CONTROL)
>> +			&& (reg != SDHCI_CLOCK_CONTROL)) {
>> +			dev_err(mmc_dev(host->mmc),
>> +			"back-to-back write to 0x%x\n", reg);
>
> This fires a *ton* on reg 0x20 and 0x30 on my rev 2 model B with the
> patches applied on top of next-20141031. Without the patches applied,
> everything works fine. As far as I can tell, SD card accesses no longer
> work (or perhaps there's just so much log spew over serial that it takes
> more than 1.5 minutes to get to the login prompt).
>
Thanks for testing.  Like I said in the cover message - I've never run 
this on a PI actually.  I've run it on other hardware with the same core 
arasan block having the same clock domain issue.  The registers printed 
out do not have the clock domain issue - I'm still getting more details 
from the silicon designers on this.

Without the verify patch the performance is actually quite good.  See 
tests result from Piotr:

On Fri, Oct 31, 2014 at 05:02:59PM +0000, Scott Branden wrote:
 > Please let me know how this works for you.
 >
Scott,
please ignore my previous mail I made mistake when applying patches.

Results of testing your code on top of 3.18-rc2 with Kingston SDC10/8GB:

* when compiling with CONFIG_MMC_SDHCI_BCM2835_VERIFY_WORKAROUND=y there 
is a
lot of:

sdhci-bcm2835 20300000.sdhci: back-to-back write to 0x30
and
sdhci-bcm2835 20300000.sdhci: back-to-back write to 0x20

* performance w/o patches:
yncraspberrypi:~$ sync; time dd if=/dev/zero of=~/test.tmp bs=500K 
count=1024; sy
1024+0 records in
1024+0 records out
524288000 bytes (524 MB) copied, 787.384 s, 666 kB/s

real    13m7.404s
user    0m0.080s
sys     0m56.300s
pi@raspberrypi:~$ time dd if=~/test.tmp of=/dev/null bs=500K count=1024
1024+0 records in
1024+0 records out
524288000 bytes (524 MB) copied, 34.2115 s, 15.3 MB/s

real    0m34.232s
user    0m0.020s
sys     0m31.190s


* performance w/ patches is great IMHO:
yncraspberrypi:~$ sync; time dd if=/dev/zero of=~/test.tmp bs=500K 
count=1024; sy
1024+0 records in
1024+0 records out
524288000 bytes (524 MB) copied, 45.4886 s, 11.5 MB/s

real    0m45.515s
user    0m0.060s
sys     0m30.050s time dd if=~/test.tmp of=/dev/null bs=500K count=1024
1024+0 records in
1024+0 records out
524288000 bytes (524 MB) copied, 33.6292 s, 15.6 MB/s

real    0m33.649s
user    0m0.020s
sys     0m30.730s

Great work!

Have you got plans to enable DMA for this controller ? sys CPU load is quite
big for above code, my tests with bcm2835-mmc and slave_sg from RaspberryPi
Foundation gives about 15s instead of 31s. It would be great to relive CPU a
little bit.

Best Regards,
Piotr Król

Re: [PATCHv2 4/5] mmc: shdci-bcm2835: add verify for 32-bit back-to-back workaround

[Stephen Warren]

On 10/30/2014 12:36 AM, Scott Branden wrote:
> Add a verify option to driver to print out an error message if a
> potential back to back write could cause a clock domain issue.

> index f8c450a..11af27f 100644

> +#ifdef CONFIG_MMC_SDHCI_BCM2835_VERIFY_WORKAROUND
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct bcm2835_sdhci_host *bcm2835_host = pltfm_host->priv;
> +
> +	if (bcm2835_host->previous_reg == reg) {
> +		if ((reg != SDHCI_HOST_CONTROL)
> +			&& (reg != SDHCI_CLOCK_CONTROL)) {

The comment in patch 3 says the problem doesn't apply to the data
register. Why does this check for these two registers rather than data?

> +			dev_err(mmc_dev(host->mmc),
> +			"back-to-back write to 0x%x\n", reg);

The continuation line should be indented at least one more level here.

Re: [PATCHv2 4/5] mmc: shdci-bcm2835: add verify for 32-bit back-to-back workaround

[Scott Branden]

On 14-11-04 08:59 PM, Stephen Warren wrote:
> On 10/30/2014 12:36 AM, Scott Branden wrote:
>> Add a verify option to driver to print out an error message if a
>> potential back to back write could cause a clock domain issue.
>
>> index f8c450a..11af27f 100644
>
>> +#ifdef CONFIG_MMC_SDHCI_BCM2835_VERIFY_WORKAROUND
>> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> +	struct bcm2835_sdhci_host *bcm2835_host = pltfm_host->priv;
>> +
>> +	if (bcm2835_host->previous_reg == reg) {
>> +		if ((reg != SDHCI_HOST_CONTROL)
>> +			&& (reg != SDHCI_CLOCK_CONTROL)) {
>
> The comment in patch 3 says the problem doesn't apply to the data
> register. Why does this check for these two registers rather than data?
This Verify workaround patch still a work in progress.  I'm still 
getting more info from the silicon designers on the back-to-back 
register writes that are affect.  The spew of 0x20 or 0x28 or 0x2c 
register writes are all ok locations that don't need to be worked 
around.  This patch needs to be corrected with the proper register rules 
still.
>
>> +			dev_err(mmc_dev(host->mmc),
>> +			"back-to-back write to 0x%x\n", reg);
>
> The continuation line should be indented at least one more level here.
>

Re: [PATCHv2 4/5] mmc: shdci-bcm2835: add verify for 32-bit back-to-back workaround

[Stephen Warren]

On 11/05/2014 12:00 AM, Scott Branden wrote:
> On 14-11-04 08:59 PM, Stephen Warren wrote:
>> On 10/30/2014 12:36 AM, Scott Branden wrote:
>>> Add a verify option to driver to print out an error message if a
>>> potential back to back write could cause a clock domain issue.
>>
>>> index f8c450a..11af27f 100644
>>
>>> +#ifdef CONFIG_MMC_SDHCI_BCM2835_VERIFY_WORKAROUND
>>> +    struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>> +    struct bcm2835_sdhci_host *bcm2835_host = pltfm_host->priv;
>>> +
>>> +    if (bcm2835_host->previous_reg == reg) {
>>> +        if ((reg != SDHCI_HOST_CONTROL)
>>> +            && (reg != SDHCI_CLOCK_CONTROL)) {
>>
>> The comment in patch 3 says the problem doesn't apply to the data
>> register. Why does this check for these two registers rather than data?
> This Verify workaround patch still a work in progress.  I'm still
> getting more info from the silicon designers on the back-to-back
> register writes that are affect.  The spew of 0x20 or 0x28 or 0x2c
> register writes are all ok locations that don't need to be worked
> around.  This patch needs to be corrected with the proper register rules
> still.

FYI, I applied the series except for this patch, and everything
/appeared/ to work OK for a brief test (boot, log in, reboot). Still,
I'll hold off my Tested-by/acked-by until the comment in patch 3 and the
register list above match, and there's no log spew with everything applied.

Re: [PATCHv2 4/5] mmc: shdci-bcm2835: add verify for 32-bit back-to-back workaround

[Scott Branden]

On 14-11-05 09:01 PM, Stephen Warren wrote:
> On 11/05/2014 12:00 AM, Scott Branden wrote:
>> On 14-11-04 08:59 PM, Stephen Warren wrote:
>>> On 10/30/2014 12:36 AM, Scott Branden wrote:
>>>> Add a verify option to driver to print out an error message if a
>>>> potential back to back write could cause a clock domain issue.
>>>
>>>> index f8c450a..11af27f 100644
>>>
>>>> +#ifdef CONFIG_MMC_SDHCI_BCM2835_VERIFY_WORKAROUND
>>>> +    struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>>> +    struct bcm2835_sdhci_host *bcm2835_host = pltfm_host->priv;
>>>> +
>>>> +    if (bcm2835_host->previous_reg == reg) {
>>>> +        if ((reg != SDHCI_HOST_CONTROL)
>>>> +            && (reg != SDHCI_CLOCK_CONTROL)) {
>>>
>>> The comment in patch 3 says the problem doesn't apply to the data
>>> register. Why does this check for these two registers rather than data?
>> This Verify workaround patch still a work in progress.  I'm still
>> getting more info from the silicon designers on the back-to-back
>> register writes that are affect.  The spew of 0x20 or 0x28 or 0x2c
>> register writes are all ok locations that don't need to be worked
>> around.  This patch needs to be corrected with the proper register rules
>> still.
Thanks for testing.  Yes, I have work to do on the verify patch above still.
>
> FYI, I applied the series except for this patch, and everything
> /appeared/ to work OK for a brief test (boot, log in, reboot). Still,
> I'll hold off my Tested-by/acked-by until the comment in patch 3 and the
> register list above match, and there's no log spew with everything applied.
>

Re: [PATCHv2 4/5] mmc: shdci-bcm2835: add verify for 32-bit back-to-back workaround

[Stefan Wahren]

Hi Scott,

Am 07.11.2014 um 19:31 schrieb Scott Branden:
> On 14-11-05 09:01 PM, Stephen Warren wrote:
>> On 11/05/2014 12:00 AM, Scott Branden wrote:
>>> On 14-11-04 08:59 PM, Stephen Warren wrote:
>>>> On 10/30/2014 12:36 AM, Scott Branden wrote:
>>>>> Add a verify option to driver to print out an error message if a
>>>>> potential back to back write could cause a clock domain issue.
>>>>
>>>>> index f8c450a..11af27f 100644
>>>>
>>>>> +#ifdef CONFIG_MMC_SDHCI_BCM2835_VERIFY_WORKAROUND
>>>>> +    struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>>>> +    struct bcm2835_sdhci_host *bcm2835_host = pltfm_host->priv;
>>>>> +
>>>>> +    if (bcm2835_host->previous_reg == reg) {
>>>>> +        if ((reg != SDHCI_HOST_CONTROL)
>>>>> +            && (reg != SDHCI_CLOCK_CONTROL)) {
>>>>
>>>> The comment in patch 3 says the problem doesn't apply to the data
>>>> register. Why does this check for these two registers rather than data?
>>> This Verify workaround patch still a work in progress.  I'm still
>>> getting more info from the silicon designers on the back-to-back
>>> register writes that are affect.  The spew of 0x20 or 0x28 or 0x2c
>>> register writes are all ok locations that don't need to be worked
>>> around.  This patch needs to be corrected with the proper register rules
>>> still.
> Thanks for testing.  Yes, I have work to do on the verify patch above
> still.

do you still have plans to submit a V3 of this patch series?

I attached an improved version of this patch which avoids a possible
endless loop caused by the dev_err call. So only the first occurence
of a specific register will be logged.

Regards
Stefan


-------------------8<-------------------------------------------

diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index 1526b8a..7b0990f 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -306,6 +306,15 @@ config MMC_SDHCI_BCM2835

 	  If unsure, say N.

+config MMC_SDHCI_BCM2835_VERIFY_WORKAROUND
+	bool "Verify BCM2835 workaround does not do back to back writes"
+	depends on MMC_SDHCI_BCM2835
+	default y
+	help
+	  This enables code that verifies the bcm2835 workaround.
+	  The verification code checks that back to back writes to the same
+	  register do not occur.
+
 config MMC_SDHCI_F_SDH30
 	tristate "SDHCI support for Fujitsu Semiconductor F_SDH30"
 	depends on MMC_SDHCI_PLTFM
diff --git a/drivers/mmc/host/sdhci-bcm2835.c
b/drivers/mmc/host/sdhci-bcm2835.c
index 01ce193d..c1c70df 100644
--- a/drivers/mmc/host/sdhci-bcm2835.c
+++ b/drivers/mmc/host/sdhci-bcm2835.c
@@ -20,15 +20,27 @@
  */

 #include <linux/delay.h>
+#include <linux/hashtable.h>
 #include <linux/module.h>
 #include <linux/mmc/host.h>
+#include <linux/slab.h>
 #include "sdhci-pltfm.h"

 struct bcm2835_sdhci_host {
 	u32 shadow_cmd;
 	u32 shadow_blk;
+	int previous_reg;
 };

+struct reg_hash {
+	struct hlist_node node;
+	int reg;
+};
+
+#define BCM2835_REG_HT_BITS 4
+
+static DEFINE_HASHTABLE(bcm2835_used_regs, BCM2835_REG_HT_BITS);
+
 #define REG_OFFSET_IN_BITS(reg) ((reg) << 3 & 0x18)

 static inline u32 bcm2835_sdhci_readl(struct sdhci_host *host, int reg)
@@ -56,8 +68,37 @@ static u8 bcm2835_sdhci_readb(struct sdhci_host
*host, int reg)
 }

 static inline void bcm2835_sdhci_writel(struct sdhci_host *host,
+					u32 val, int reg)
+{
+	writel(val, host->ioaddr + reg);
+}
+
+static inline void bcm2835_sdhci_writel_verify(struct sdhci_host *host,
 						u32 val, int reg)
 {
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct bcm2835_sdhci_host *bcm2835_host = pltfm_host->priv;
+	struct reg_hash *rh;
+	struct hlist_head *head;
+
+	head = &bcm2835_used_regs[hash_min(reg, BCM2835_REG_HT_BITS)];
+
+	if (bcm2835_host->previous_reg == reg) {
+		if ((reg != SDHCI_HOST_CONTROL) &&
+		    (reg != SDHCI_CLOCK_CONTROL) &&
+		    (hlist_empty(head))) {
+			rh = kzalloc(sizeof(*rh), GFP_KERNEL);
+			if (WARN_ON(!rh))
+				return;
+
+			rh->reg = reg;
+			hash_add(bcm2835_used_regs, &rh->node, rh->reg);
+			dev_err(mmc_dev(host->mmc), "back-to-back write to 0x%x\n",
+				reg);
+		}
+	}
+	bcm2835_host->previous_reg = reg;
+
 	writel(val, host->ioaddr + reg);
 }

@@ -131,7 +172,11 @@ static const struct sdhci_ops bcm2835_sdhci_ops = {
 	.read_l = bcm2835_sdhci_readl,
 	.read_w = bcm2835_sdhci_readw,
 	.read_b = bcm2835_sdhci_readb,
+#ifdef CONFIG_MMC_SDHCI_BCM2835_VERIFY_WORKAROUND
+	.write_l = bcm2835_sdhci_writel_verify,
+#else
 	.write_l = bcm2835_sdhci_writel,
+#endif
 	.write_w = bcm2835_sdhci_writew,
 	.write_b = bcm2835_sdhci_writeb,
 	.set_clock = sdhci_set_clock,

Re: [PATCHv2 4/5] mmc: shdci-bcm2835: add verify for 32-bit back-to-back workaround

[Scott Branden]

Hi Stefan,

On 15-12-22 07:55 AM, Stefan Wahren wrote:
> Hi Scott,
>
> Am 07.11.2014 um 19:31 schrieb Scott Branden:
>> On 14-11-05 09:01 PM, Stephen Warren wrote:
>>> On 11/05/2014 12:00 AM, Scott Branden wrote:
>>>> On 14-11-04 08:59 PM, Stephen Warren wrote:
>>>>> On 10/30/2014 12:36 AM, Scott Branden wrote:
>>>>>> Add a verify option to driver to print out an error message if a
>>>>>> potential back to back write could cause a clock domain issue.
>>>>>
>>>>>> index f8c450a..11af27f 100644
>>>>>
>>>>>> +#ifdef CONFIG_MMC_SDHCI_BCM2835_VERIFY_WORKAROUND
>>>>>> +    struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>>>>> +    struct bcm2835_sdhci_host *bcm2835_host = pltfm_host->priv;
>>>>>> +
>>>>>> +    if (bcm2835_host->previous_reg == reg) {
>>>>>> +        if ((reg != SDHCI_HOST_CONTROL)
>>>>>> +            && (reg != SDHCI_CLOCK_CONTROL)) {
>>>>>
>>>>> The comment in patch 3 says the problem doesn't apply to the data
>>>>> register. Why does this check for these two registers rather than data?
>>>> This Verify workaround patch still a work in progress.  I'm still
>>>> getting more info from the silicon designers on the back-to-back
>>>> register writes that are affect.  The spew of 0x20 or 0x28 or 0x2c
>>>> register writes are all ok locations that don't need to be worked
>>>> around.  This patch needs to be corrected with the proper register rules
>>>> still.
>> Thanks for testing.  Yes, I have work to do on the verify patch above
>> still.
>
> do you still have plans to submit a V3 of this patch series?
No, I do not have plans to submit a V3 of this patch series.

I submitted this patch as RPI has a similar controller to the SoCs I am 
familiar with as well as needing similar work arounds   You can take 
over the patchset.  Or, try and get the sdhci-iproc.c driver going on 
RPI.  The sdhci-iproc is the production driver we use on a variety of 
SoCs and support and test this driver.
>
> I attached an improved version of this patch which avoids a possible
> endless loop caused by the dev_err call. So only the first occurence
> of a specific register will be logged.
OK, but is this really necessary?  If veryify workaround ever prints 
anything then the driver workarounds aren't doing what it is supposed to 
anyway?
>
> Regards
> Stefan
>
>
> -------------------8<-------------------------------------------
>
> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> index 1526b8a..7b0990f 100644
> --- a/drivers/mmc/host/Kconfig
> +++ b/drivers/mmc/host/Kconfig
> @@ -306,6 +306,15 @@ config MMC_SDHCI_BCM2835
>
>   	  If unsure, say N.
>
> +config MMC_SDHCI_BCM2835_VERIFY_WORKAROUND
> +	bool "Verify BCM2835 workaround does not do back to back writes"
> +	depends on MMC_SDHCI_BCM2835
> +	default y
> +	help
> +	  This enables code that verifies the bcm2835 workaround.
> +	  The verification code checks that back to back writes to the same
> +	  register do not occur.
> +
>   config MMC_SDHCI_F_SDH30
>   	tristate "SDHCI support for Fujitsu Semiconductor F_SDH30"
>   	depends on MMC_SDHCI_PLTFM
> diff --git a/drivers/mmc/host/sdhci-bcm2835.c
> b/drivers/mmc/host/sdhci-bcm2835.c
> index 01ce193d..c1c70df 100644
> --- a/drivers/mmc/host/sdhci-bcm2835.c
> +++ b/drivers/mmc/host/sdhci-bcm2835.c
> @@ -20,15 +20,27 @@
>    */
>
>   #include <linux/delay.h>
> +#include <linux/hashtable.h>
>   #include <linux/module.h>
>   #include <linux/mmc/host.h>
> +#include <linux/slab.h>
>   #include "sdhci-pltfm.h"
>
>   struct bcm2835_sdhci_host {
>   	u32 shadow_cmd;
>   	u32 shadow_blk;
> +	int previous_reg;
>   };
>
> +struct reg_hash {
> +	struct hlist_node node;
> +	int reg;
> +};
> +
> +#define BCM2835_REG_HT_BITS 4
> +
> +static DEFINE_HASHTABLE(bcm2835_used_regs, BCM2835_REG_HT_BITS);
> +
>   #define REG_OFFSET_IN_BITS(reg) ((reg) << 3 & 0x18)
>
>   static inline u32 bcm2835_sdhci_readl(struct sdhci_host *host, int reg)
> @@ -56,8 +68,37 @@ static u8 bcm2835_sdhci_readb(struct sdhci_host
> *host, int reg)
>   }
>
>   static inline void bcm2835_sdhci_writel(struct sdhci_host *host,
> +					u32 val, int reg)
> +{
> +	writel(val, host->ioaddr + reg);
> +}
> +
> +static inline void bcm2835_sdhci_writel_verify(struct sdhci_host *host,
>   						u32 val, int reg)
>   {
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct bcm2835_sdhci_host *bcm2835_host = pltfm_host->priv;
> +	struct reg_hash *rh;
> +	struct hlist_head *head;
> +
> +	head = &bcm2835_used_regs[hash_min(reg, BCM2835_REG_HT_BITS)];
> +
> +	if (bcm2835_host->previous_reg == reg) {
> +		if ((reg != SDHCI_HOST_CONTROL) &&
> +		    (reg != SDHCI_CLOCK_CONTROL) &&
> +		    (hlist_empty(head))) {
> +			rh = kzalloc(sizeof(*rh), GFP_KERNEL);
> +			if (WARN_ON(!rh))
> +				return;
> +
> +			rh->reg = reg;
> +			hash_add(bcm2835_used_regs, &rh->node, rh->reg);
> +			dev_err(mmc_dev(host->mmc), "back-to-back write to 0x%x\n",
> +				reg);
> +		}
> +	}
> +	bcm2835_host->previous_reg = reg;
> +
>   	writel(val, host->ioaddr + reg);
>   }
>
> @@ -131,7 +172,11 @@ static const struct sdhci_ops bcm2835_sdhci_ops = {
>   	.read_l = bcm2835_sdhci_readl,
>   	.read_w = bcm2835_sdhci_readw,
>   	.read_b = bcm2835_sdhci_readb,
> +#ifdef CONFIG_MMC_SDHCI_BCM2835_VERIFY_WORKAROUND
> +	.write_l = bcm2835_sdhci_writel_verify,
> +#else
>   	.write_l = bcm2835_sdhci_writel,
> +#endif
>   	.write_w = bcm2835_sdhci_writew,
>   	.write_b = bcm2835_sdhci_writeb,
>   	.set_clock = sdhci_set_clock,
>
>
>
>

Re: [PATCHv2 4/5] mmc: shdci-bcm2835: add verify for 32-bit back-to-back workaround

[Stefan Wahren]

Hi Scott,

Am 22.12.2015 um 20:23 schrieb Scott Branden:
> Hi Stefan,
>
> On 15-12-22 07:55 AM, Stefan Wahren wrote:
>> Hi Scott,
>>
>> Am 07.11.2014 um 19:31 schrieb Scott Branden:
>>> On 14-11-05 09:01 PM, Stephen Warren wrote:
>>>> On 11/05/2014 12:00 AM, Scott Branden wrote:
>>>>> On 14-11-04 08:59 PM, Stephen Warren wrote:
>>>>>> On 10/30/2014 12:36 AM, Scott Branden wrote:
>>>>>>> Add a verify option to driver to print out an error message if a
>>>>>>> potential back to back write could cause a clock domain issue.
>>>>>>
>>>>>>> index f8c450a..11af27f 100644
>>>>>>
>>>>>>> +#ifdef CONFIG_MMC_SDHCI_BCM2835_VERIFY_WORKAROUND
>>>>>>> +    struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>>>>>> +    struct bcm2835_sdhci_host *bcm2835_host = pltfm_host->priv;
>>>>>>> +
>>>>>>> +    if (bcm2835_host->previous_reg == reg) {
>>>>>>> +        if ((reg != SDHCI_HOST_CONTROL)
>>>>>>> +            && (reg != SDHCI_CLOCK_CONTROL)) {
>>>>>>
>>>>>> The comment in patch 3 says the problem doesn't apply to the data
>>>>>> register. Why does this check for these two registers rather than
>>>>>> data?
>>>>> This Verify workaround patch still a work in progress.  I'm still
>>>>> getting more info from the silicon designers on the back-to-back
>>>>> register writes that are affect.  The spew of 0x20 or 0x28 or 0x2c
>>>>> register writes are all ok locations that don't need to be worked
>>>>> around.  This patch needs to be corrected with the proper register
>>>>> rules
>>>>> still.
>>> Thanks for testing.  Yes, I have work to do on the verify patch above
>>> still.
>>
>> do you still have plans to submit a V3 of this patch series?
> No, I do not have plans to submit a V3 of this patch series.
>
> I submitted this patch as RPI has a similar controller to the SoCs I am
> familiar with as well as needing similar work arounds   You can take
> over the patchset.  Or, try and get the sdhci-iproc.c driver going on
> RPI.  The sdhci-iproc is the production driver we use on a variety of
> SoCs and support and test this driver.

after applying the patch series both drivers are very similiar so i 
prefer the latter. I will give it a try. Thanks for the hint about 
sdhci-iproc.

Regards
Stefan

[PATCHv2 5/5] mmc: sdhci-bcm2835: add sdhci quirk SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12

[Scott Branden]

SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12 is missing and needed for this controller.

Signed-off-by: Scott Branden <sbranden@broadcom.com>
---
 drivers/mmc/host/sdhci-bcm2835.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci-bcm2835.c b/drivers/mmc/host/sdhci-bcm2835.c
index 11af27f..2a4c10b 100644
--- a/drivers/mmc/host/sdhci-bcm2835.c
+++ b/drivers/mmc/host/sdhci-bcm2835.c
@@ -159,7 +159,8 @@ static const struct sdhci_ops bcm2835_sdhci_ops = {
 };
 
 static const struct sdhci_pltfm_data bcm2835_sdhci_pdata = {
-	.quirks = SDHCI_QUIRK_BROKEN_CARD_DETECTION |
+	.quirks = SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12 |
+		  SDHCI_QUIRK_BROKEN_CARD_DETECTION |
 		  SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK,
 	.ops = &bcm2835_sdhci_ops,
 };
-- 
1.7.9.5

Re: [PATCHv2 5/5] mmc: sdhci-bcm2835: add sdhci quirk SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12

[Stephen Warren]

On 10/30/2014 12:36 AM, Scott Branden wrote:
> SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12 is missing and needed for this controller.

This seems fine, although any explanation of why this quirk is needed
would be useful.

Re: [PATCHv2 5/5] mmc: sdhci-bcm2835: add sdhci quirk SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12

[Scott Branden]

On 14-11-04 09:00 PM, Stephen Warren wrote:
> On 10/30/2014 12:36 AM, Scott Branden wrote:
>> SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12 is missing and needed for this controller.
>
> This seems fine, although any explanation of why this quirk is needed
> would be useful.
>
I don't know who to talk to at Arasan about this.  Will try hunting 
around a little for more info as to why this is needed to have eMMC and 
SD work properly through our internal testing on other non-2835 chipset 
that shares the same SDHCI controller as 2835.

Re: [PATCHv2 5/5] mmc: sdhci-bcm2835: add sdhci quirk SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12

[Stephen Warren]

On 11/05/2014 12:02 AM, Scott Branden wrote:
> On 14-11-04 09:00 PM, Stephen Warren wrote:
>> On 10/30/2014 12:36 AM, Scott Branden wrote:
>>> SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12 is missing and needed for this
>>> controller.
>>
>> This seems fine, although any explanation of why this quirk is needed
>> would be useful.
>>
> I don't know who to talk to at Arasan about this.  Will try hunting
> around a little for more info as to why this is needed to have eMMC and
> SD work properly through our internal testing on other non-2835 chipset
> that shares the same SDHCI controller as 2835.

I thought I heard that this wasn't a bug in the controller itself, but
rather an integration issue between the IP core and the register bus
it's attached to. Consequently, it may be SoC-specific or at least have
SoC-specific variations?

Re: [PATCHv2 5/5] mmc: sdhci-bcm2835: add sdhci quirk SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12

[Scott Branden]

On 14-11-05 08:50 PM, Stephen Warren wrote:
> On 11/05/2014 12:02 AM, Scott Branden wrote:
>> On 14-11-04 09:00 PM, Stephen Warren wrote:
>>> On 10/30/2014 12:36 AM, Scott Branden wrote:
>>>> SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12 is missing and needed for this
>>>> controller.
>>>
>>> This seems fine, although any explanation of why this quirk is needed
>>> would be useful.
>>>
>> I don't know who to talk to at Arasan about this.  Will try hunting
>> around a little for more info as to why this is needed to have eMMC and
>> SD work properly through our internal testing on other non-2835 chipset
>> that shares the same SDHCI controller as 2835.
>
> I thought I heard that this wasn't a bug in the controller itself, but
> rather an integration issue between the IP core and the register bus
> it's attached to. Consequently, it may be SoC-specific or at least have
> SoC-specific variations?
Yes, this patch is to fix a different bug (in the IP) rather than the 
clock domain integration issue.
>

[PATCH] mmc: sdhci: add quirk for ACMD23 broken

[Scott Branden]

Add quick to handle broken auto-CMD23
Some controllers do not respond after the first auto-CMD23 is issued.

This allows CMD23 to still work (mandatory for the faster UHS-I mode)
rather than disabling CMD23 entirely via SDHCI_QUIRK2_HOST_NO_CMD23.

Signed-off by: Corneliu Doban <cdoban@broadcom.com>
Signed-off-by: Scott Branden <sbranden@broadcom.com>
---
 drivers/mmc/host/sdhci.c  | 3 ++-
 include/linux/mmc/sdhci.h | 2 ++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index ada1a3e..b37331f 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2985,7 +2985,8 @@ int sdhci_add_host(struct sdhci_host *host)
 	/* Auto-CMD23 stuff only works in ADMA or PIO. */
 	if ((host->version >= SDHCI_SPEC_300) &&
 	    ((host->flags & SDHCI_USE_ADMA) ||
-	     !(host->flags & SDHCI_USE_SDMA))) {
+	     !(host->flags & SDHCI_USE_SDMA)) &&
+	     !(host->quirks2 & SDHCI_QUIRK2_ACMD23_BROKEN)) {
 		host->flags |= SDHCI_AUTO_CMD23;
 		DBG("%s: Auto-CMD23 available\n", mmc_hostname(mmc));
 	} else {
diff --git a/include/linux/mmc/sdhci.h b/include/linux/mmc/sdhci.h
index dba793e..d979cf9 100644
--- a/include/linux/mmc/sdhci.h
+++ b/include/linux/mmc/sdhci.h
@@ -100,6 +100,8 @@ struct sdhci_host {
 #define SDHCI_QUIRK2_BROKEN_DDR50			(1<<7)
 /* Stop command (CMD12) can set Transfer Complete when not using MMC_RSP_BUSY */
 #define SDHCI_QUIRK2_STOP_WITH_TC			(1<<8)
+/* Controller broken with using ACMD23 */
+#define SDHCI_QUIRK2_ACMD23_BROKEN			(1<<9)
 
 	int irq;		/* Device IRQ */
 	void __iomem *ioaddr;	/* Mapped address */
-- 
2.2.0

[PATCH v2] mmc: sdhci: add quirk for ACMD23 broken

[Scott Branden]

Add quirk to handle broken auto-CMD23.
Some controllers do not respond after the first auto-CMD23 is issued.

This allows CMD23 to still work (mandatory for the faster UHS-I mode)
rather than disabling CMD23 entirely via SDHCI_QUIRK2_HOST_NO_CMD23.

Signed-off by: Corneliu Doban <cdoban@broadcom.com>
Signed-off-by: Scott Branden <sbranden@broadcom.com>
---
 drivers/mmc/host/sdhci.c  | 3 ++-
 include/linux/mmc/sdhci.h | 2 ++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index ada1a3e..b37331f 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2985,7 +2985,8 @@ int sdhci_add_host(struct sdhci_host *host)
 	/* Auto-CMD23 stuff only works in ADMA or PIO. */
 	if ((host->version >= SDHCI_SPEC_300) &&
 	    ((host->flags & SDHCI_USE_ADMA) ||
-	     !(host->flags & SDHCI_USE_SDMA))) {
+	     !(host->flags & SDHCI_USE_SDMA)) &&
+	     !(host->quirks2 & SDHCI_QUIRK2_ACMD23_BROKEN)) {
 		host->flags |= SDHCI_AUTO_CMD23;
 		DBG("%s: Auto-CMD23 available\n", mmc_hostname(mmc));
 	} else {
diff --git a/include/linux/mmc/sdhci.h b/include/linux/mmc/sdhci.h
index dba793e..d979cf9 100644
--- a/include/linux/mmc/sdhci.h
+++ b/include/linux/mmc/sdhci.h
@@ -100,6 +100,8 @@ struct sdhci_host {
 #define SDHCI_QUIRK2_BROKEN_DDR50			(1<<7)
 /* Stop command (CMD12) can set Transfer Complete when not using MMC_RSP_BUSY */
 #define SDHCI_QUIRK2_STOP_WITH_TC			(1<<8)
+/* Controller broken with using ACMD23 */
+#define SDHCI_QUIRK2_ACMD23_BROKEN			(1<<9)
 
 	int irq;		/* Device IRQ */
 	void __iomem *ioaddr;	/* Mapped address */
-- 
2.2.0

Re: [PATCH v2] mmc: sdhci: add quirk for ACMD23 broken

[Scott Branden]

Hi Chris,

I have not received any feedback on this patch for the SDHCI controller. 
  Anything I can do to help get this accepted into the codebase?

Thanks,
  Scott

On 14-12-04 04:16 PM, Scott Branden wrote:
> Add quirk to handle broken auto-CMD23.
> Some controllers do not respond after the first auto-CMD23 is issued.
>
> This allows CMD23 to still work (mandatory for the faster UHS-I mode)
> rather than disabling CMD23 entirely via SDHCI_QUIRK2_HOST_NO_CMD23.
>
> Signed-off by: Corneliu Doban <cdoban@broadcom.com>
> Signed-off-by: Scott Branden <sbranden@broadcom.com>
> ---
>   drivers/mmc/host/sdhci.c  | 3 ++-
>   include/linux/mmc/sdhci.h | 2 ++
>   2 files changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index ada1a3e..b37331f 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -2985,7 +2985,8 @@ int sdhci_add_host(struct sdhci_host *host)
>   	/* Auto-CMD23 stuff only works in ADMA or PIO. */
>   	if ((host->version >= SDHCI_SPEC_300) &&
>   	    ((host->flags & SDHCI_USE_ADMA) ||
> -	     !(host->flags & SDHCI_USE_SDMA))) {
> +	     !(host->flags & SDHCI_USE_SDMA)) &&
> +	     !(host->quirks2 & SDHCI_QUIRK2_ACMD23_BROKEN)) {
>   		host->flags |= SDHCI_AUTO_CMD23;
>   		DBG("%s: Auto-CMD23 available\n", mmc_hostname(mmc));
>   	} else {
> diff --git a/include/linux/mmc/sdhci.h b/include/linux/mmc/sdhci.h
> index dba793e..d979cf9 100644
> --- a/include/linux/mmc/sdhci.h
> +++ b/include/linux/mmc/sdhci.h
> @@ -100,6 +100,8 @@ struct sdhci_host {
>   #define SDHCI_QUIRK2_BROKEN_DDR50			(1<<7)
>   /* Stop command (CMD12) can set Transfer Complete when not using MMC_RSP_BUSY */
>   #define SDHCI_QUIRK2_STOP_WITH_TC			(1<<8)
> +/* Controller broken with using ACMD23 */
> +#define SDHCI_QUIRK2_ACMD23_BROKEN			(1<<9)
>
>   	int irq;		/* Device IRQ */
>   	void __iomem *ioaddr;	/* Mapped address */
>

Re: [PATCH v2] mmc: sdhci: add quirk for ACMD23 broken

[Chris Ball]

Hi Scott, sorry for the delay,

On Fri, Dec 05 2014, Scott Branden wrote:
> Add quirk to handle broken auto-CMD23.
> Some controllers do not respond after the first auto-CMD23 is issued.
>
> This allows CMD23 to still work (mandatory for the faster UHS-I mode)
> rather than disabling CMD23 entirely via SDHCI_QUIRK2_HOST_NO_CMD23.
>
> Signed-off by: Corneliu Doban <cdoban@broadcom.com>
> Signed-off-by: Scott Branden <sbranden@broadcom.com>
> ---
>  drivers/mmc/host/sdhci.c  | 3 ++-
>  include/linux/mmc/sdhci.h | 2 ++
>  2 files changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index ada1a3e..b37331f 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -2985,7 +2985,8 @@ int sdhci_add_host(struct sdhci_host *host)
>  	/* Auto-CMD23 stuff only works in ADMA or PIO. */
>  	if ((host->version >= SDHCI_SPEC_300) &&
>  	    ((host->flags & SDHCI_USE_ADMA) ||
> -	     !(host->flags & SDHCI_USE_SDMA))) {
> +	     !(host->flags & SDHCI_USE_SDMA)) &&
> +	     !(host->quirks2 & SDHCI_QUIRK2_ACMD23_BROKEN)) {
>  		host->flags |= SDHCI_AUTO_CMD23;
>  		DBG("%s: Auto-CMD23 available\n", mmc_hostname(mmc));
>  	} else {
> diff --git a/include/linux/mmc/sdhci.h b/include/linux/mmc/sdhci.h
> index dba793e..d979cf9 100644
> --- a/include/linux/mmc/sdhci.h
> +++ b/include/linux/mmc/sdhci.h
> @@ -100,6 +100,8 @@ struct sdhci_host {
>  #define SDHCI_QUIRK2_BROKEN_DDR50			(1<<7)
>  /* Stop command (CMD12) can set Transfer Complete when not using MMC_RSP_BUSY */
>  #define SDHCI_QUIRK2_STOP_WITH_TC			(1<<8)
> +/* Controller broken with using ACMD23 */
> +#define SDHCI_QUIRK2_ACMD23_BROKEN			(1<<9)
>  
>  	int irq;		/* Device IRQ */
>  	void __iomem *ioaddr;	/* Mapped address */

This patch doesn't apply the quirk to any chipsets, so the patch
doesn't actually change any behavior when run on a mainline kernel.
Do you have a patch to apply the quirk to your chipset too?  We don't
typically accept patches that "don't do anything" for mainline users,
so that's why we'd like to see the chipset patch too.

Thanks,

- Chris.
-- 
Chris Ball   <http://printf.net/>

Re: [PATCH v2] mmc: sdhci: add quirk for ACMD23 broken

[Scott Branden]

Hi Chris,

We'll finish off cleaning up our driver that uses this patch for review 
then.

Thanks,
  Scott

On 14-12-17 11:48 AM, Chris Ball wrote:
> Hi Scott, sorry for the delay,
>
> On Fri, Dec 05 2014, Scott Branden wrote:
>> Add quirk to handle broken auto-CMD23.
>> Some controllers do not respond after the first auto-CMD23 is issued.
>>
>> This allows CMD23 to still work (mandatory for the faster UHS-I mode)
>> rather than disabling CMD23 entirely via SDHCI_QUIRK2_HOST_NO_CMD23.
>>
>> Signed-off by: Corneliu Doban <cdoban@broadcom.com>
>> Signed-off-by: Scott Branden <sbranden@broadcom.com>
>> ---
>>   drivers/mmc/host/sdhci.c  | 3 ++-
>>   include/linux/mmc/sdhci.h | 2 ++
>>   2 files changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> index ada1a3e..b37331f 100644
>> --- a/drivers/mmc/host/sdhci.c
>> +++ b/drivers/mmc/host/sdhci.c
>> @@ -2985,7 +2985,8 @@ int sdhci_add_host(struct sdhci_host *host)
>>   	/* Auto-CMD23 stuff only works in ADMA or PIO. */
>>   	if ((host->version >= SDHCI_SPEC_300) &&
>>   	    ((host->flags & SDHCI_USE_ADMA) ||
>> -	     !(host->flags & SDHCI_USE_SDMA))) {
>> +	     !(host->flags & SDHCI_USE_SDMA)) &&
>> +	     !(host->quirks2 & SDHCI_QUIRK2_ACMD23_BROKEN)) {
>>   		host->flags |= SDHCI_AUTO_CMD23;
>>   		DBG("%s: Auto-CMD23 available\n", mmc_hostname(mmc));
>>   	} else {
>> diff --git a/include/linux/mmc/sdhci.h b/include/linux/mmc/sdhci.h
>> index dba793e..d979cf9 100644
>> --- a/include/linux/mmc/sdhci.h
>> +++ b/include/linux/mmc/sdhci.h
>> @@ -100,6 +100,8 @@ struct sdhci_host {
>>   #define SDHCI_QUIRK2_BROKEN_DDR50			(1<<7)
>>   /* Stop command (CMD12) can set Transfer Complete when not using MMC_RSP_BUSY */
>>   #define SDHCI_QUIRK2_STOP_WITH_TC			(1<<8)
>> +/* Controller broken with using ACMD23 */
>> +#define SDHCI_QUIRK2_ACMD23_BROKEN			(1<<9)
>>
>>   	int irq;		/* Device IRQ */
>>   	void __iomem *ioaddr;	/* Mapped address */
>
> This patch doesn't apply the quirk to any chipsets, so the patch
> doesn't actually change any behavior when run on a mainline kernel.
> Do you have a patch to apply the quirk to your chipset too?  We don't
> typically accept patches that "don't do anything" for mainline users,
> so that's why we'd like to see the chipset patch too.
>
> Thanks,
>
> - Chris.
>

[PATCH 0/4] Add support for IPROC SDHCI controller

[Scott Branden]

This series of patchsets contains the IPROC SDHCI driver used
in a series of Broadcom SoCs
Quirks are also added to support this controller.

Corneliu Doban (1):
  mmc: sdhci: do not set AUTO_CMD12 for multi-block CMD53

Scott Branden (3):
  mmc: sdhci: add quirk for ACMD23 broken
  mmc: sdhci-iproc: add IPROC SDHCI driver
  mmc: sdhci-iproc: add device tree bindings

 .../devicetree/bindings/mmc/brcm,sdhci-iproc.txt   |  23 ++
 drivers/mmc/host/Kconfig                           |  14 ++
 drivers/mmc/host/Makefile                          |   1 +
 drivers/mmc/host/sdhci-iproc.c                     | 241 +++++++++++++++++++++
 drivers/mmc/host/sdhci.c                           |   7 +-
 include/linux/mmc/sdhci.h                          |   2 +
 6 files changed, 286 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mmc/brcm,sdhci-iproc.txt
 create mode 100644 drivers/mmc/host/sdhci-iproc.c

-- 
2.2.2

[PATCH 1/4] mmc: sdhci: add quirk for ACMD23 broken

[Scott Branden]

Add quirk to handle broken auto-CMD23.
Some controllers do not respond after the first auto-CMD23 is issued.

This allows CMD23 to still work (mandatory for the faster UHS-I mode)
rather than disabling CMD23 entirely via SDHCI_QUIRK2_HOST_NO_CMD23.

Signed-off by: Corneliu Doban <cdoban@broadcom.com>
Signed-off-by: Scott Branden <sbranden@broadcom.com>

Signed-off-by: Scott Branden <sbranden@broadcom.com>
---
 drivers/mmc/host/sdhci.c  | 3 ++-
 include/linux/mmc/sdhci.h | 2 ++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index f1a488e..fcf78ab 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -3085,7 +3085,8 @@ int sdhci_add_host(struct sdhci_host *host)
 	/* Auto-CMD23 stuff only works in ADMA or PIO. */
 	if ((host->version >= SDHCI_SPEC_300) &&
 	    ((host->flags & SDHCI_USE_ADMA) ||
-	     !(host->flags & SDHCI_USE_SDMA))) {
+	     !(host->flags & SDHCI_USE_SDMA)) &&
+	     !(host->quirks2 & SDHCI_QUIRK2_ACMD23_BROKEN)) {
 		host->flags |= SDHCI_AUTO_CMD23;
 		DBG("%s: Auto-CMD23 available\n", mmc_hostname(mmc));
 	} else {
diff --git a/include/linux/mmc/sdhci.h b/include/linux/mmc/sdhci.h
index f767a0d..a29e9bb 100644
--- a/include/linux/mmc/sdhci.h
+++ b/include/linux/mmc/sdhci.h
@@ -106,6 +106,8 @@ struct sdhci_host {
 #define SDHCI_QUIRK2_CLEAR_TRANSFERMODE_REG_BEFORE_CMD	(1<<10)
 /* Capability register bit-63 indicates HS400 support */
 #define SDHCI_QUIRK2_CAPS_BIT63_FOR_HS400		(1<<11)
+/* Controller broken with using ACMD23 */
+#define SDHCI_QUIRK2_ACMD23_BROKEN			(1<<12)
 
 	int irq;		/* Device IRQ */
 	void __iomem *ioaddr;	/* Mapped address */
-- 
2.2.2

[PATCH 2/4] mmc: sdhci: do not set AUTO_CMD12 for multi-block CMD53

[Scott Branden]

From: Corneliu Doban <cdoban@broadcom.com>

For CMD53 in block mode, the host does not need to stop the transfer,
as it stops when the block count (present in CMD53) is reached.

Signed-off-by: Corneliu Doban <cdoban@broadcom.com>
Signed-off-by: Scott Branden <sbranden@broadcom.com>
---
 drivers/mmc/host/sdhci.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index fcf78ab..f35d413 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -28,6 +28,7 @@
 #include <linux/mmc/mmc.h>
 #include <linux/mmc/host.h>
 #include <linux/mmc/card.h>
+#include <linux/mmc/sdio.h>
 #include <linux/mmc/slot-gpio.h>
 
 #include "sdhci.h"
@@ -934,7 +935,8 @@ static void sdhci_set_transfer_mode(struct sdhci_host *host,
 		 * If we are sending CMD23, CMD12 never gets sent
 		 * on successful completion (so no Auto-CMD12).
 		 */
-		if (!host->mrq->sbc && (host->flags & SDHCI_AUTO_CMD12))
+		if (!host->mrq->sbc && (host->flags & SDHCI_AUTO_CMD12) &&
+		    (cmd->opcode != SD_IO_RW_EXTENDED))
 			mode |= SDHCI_TRNS_AUTO_CMD12;
 		else if (host->mrq->sbc && (host->flags & SDHCI_AUTO_CMD23)) {
 			mode |= SDHCI_TRNS_AUTO_CMD23;
-- 
2.2.2

[PATCH 3/4] mmc: sdhci-iproc: add IPROC SDHCI driver

[Scott Branden]

Add IPROC SDHCI driver for IPROC family of Broadcom devices.

Acked-by: Ray Jui <rjui@broadcom.com>
Signed-off-by: Corneliu Doban <cdoban@broadcom.com>
Signed-off-by: Scott Branden <sbranden@broadcom.com>
---
 drivers/mmc/host/Kconfig       |  14 +++
 drivers/mmc/host/Makefile      |   1 +
 drivers/mmc/host/sdhci-iproc.c | 241 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 256 insertions(+)
 create mode 100644 drivers/mmc/host/sdhci-iproc.c

diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index 2d6fbdd..d6a2ff1 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -292,6 +292,20 @@ config MMC_SDHCI_BCM2835
 
 	  If unsure, say N.
 
+config MMC_SDHCI_IPROC
+	tristate "SDHCI platform support for the iProc SD/MMC Controller"
+	depends on ARCH_BCM_IPROC || COMPILE_TEST
+	depends on MMC_SDHCI_PLTFM
+	default ARCH_BCM_IPROC
+	select MMC_SDHCI_IO_ACCESSORS
+	help
+	  This selects the iProc SD/MMC controller.
+
+	  If you have an IPROC platform with SD or MMC devices,
+	  say Y or M here.
+
+	  If unsure, say N.
+
 config MMC_MOXART
 	tristate "MOXART SD/MMC Host Controller support"
 	depends on ARCH_MOXART && MMC
diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
index f7b0a77..32f24bd 100644
--- a/drivers/mmc/host/Makefile
+++ b/drivers/mmc/host/Makefile
@@ -70,6 +70,7 @@ obj-$(CONFIG_MMC_SDHCI_OF_ESDHC)	+= sdhci-of-esdhc.o
 obj-$(CONFIG_MMC_SDHCI_OF_HLWD)		+= sdhci-of-hlwd.o
 obj-$(CONFIG_MMC_SDHCI_BCM_KONA)	+= sdhci-bcm-kona.o
 obj-$(CONFIG_MMC_SDHCI_BCM2835)		+= sdhci-bcm2835.o
+obj-$(CONFIG_MMC_SDHCI_IPROC)		+= sdhci-iproc.o
 obj-$(CONFIG_MMC_SDHCI_MSM)		+= sdhci-msm.o
 obj-$(CONFIG_MMC_SDHCI_ST)		+= sdhci-st.o
 
diff --git a/drivers/mmc/host/sdhci-iproc.c b/drivers/mmc/host/sdhci-iproc.c
new file mode 100644
index 0000000..4139d34
--- /dev/null
+++ b/drivers/mmc/host/sdhci-iproc.c
@@ -0,0 +1,241 @@
+/*
+ * Copyright (C) 2014 Broadcom Corporation
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+/*
+ * iProc SDHCI platform driver
+ */
+
+#include <linux/delay.h>
+#include <linux/module.h>
+#include <linux/mmc/host.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include "sdhci-pltfm.h"
+
+struct sdhci_iproc_data {
+	const struct sdhci_pltfm_data *pdata;
+	u32 caps;
+	u32 caps1;
+};
+
+struct sdhci_iproc_host {
+	const struct sdhci_iproc_data *data;
+	u32 shadow_cmd;
+	u32 shadow_blk;
+};
+
+#define REG_OFFSET_IN_BITS(reg) ((reg) << 3 & 0x18)
+
+static inline u32 sdhci_iproc_readl(struct sdhci_host *host, int reg)
+{
+	u32 val = readl(host->ioaddr + reg);
+
+	pr_debug("%s: readl [0x%02x] 0x%08x\n",
+		 mmc_hostname(host->mmc), reg, val);
+	return val;
+}
+
+static u16 sdhci_iproc_readw(struct sdhci_host *host, int reg)
+{
+	u32 val = sdhci_iproc_readl(host, (reg & ~3));
+	u16 word = val >> REG_OFFSET_IN_BITS(reg) & 0xffff;
+	return word;
+}
+
+static u8 sdhci_iproc_readb(struct sdhci_host *host, int reg)
+{
+	u32 val = sdhci_iproc_readl(host, (reg & ~3));
+	u8 byte = val >> REG_OFFSET_IN_BITS(reg) & 0xff;
+	return byte;
+}
+
+static inline void sdhci_iproc_writel(struct sdhci_host *host, u32 val, int reg)
+{
+	pr_debug("%s: writel [0x%02x] 0x%08x\n",
+		 mmc_hostname(host->mmc), reg, val);
+
+	writel(val, host->ioaddr + reg);
+
+	if (host->clock <= 400000) {
+		/* Round up to micro-second four SD clock delay */
+		if (host->clock)
+			udelay((4 * 1000000 + host->clock - 1) / host->clock);
+		else
+			udelay(10);
+	}
+}
+
+/*
+ * The Arasan has a bugette whereby it may lose the content of successive
+ * writes to the same register that are within two SD-card clock cycles of
+ * each other (a clock domain crossing problem). The data
+ * register does not have this problem, which is just as well - otherwise we'd
+ * have to nobble the DMA engine too.
+ *
+ * This wouldn't be a problem with the code except that we can only write the
+ * controller with 32-bit writes.  So two different 16-bit registers are
+ * written back to back creates the problem.
+ *
+ * In reality, this only happens when SDHCI_BLOCK_SIZE and SDHCI_BLOCK_COUNT
+ * are written followed by SDHCI_TRANSFER_MODE and SDHCI_COMMAND.
+ * The BLOCK_SIZE and BLOCK_COUNT are meaningless until a command issued so
+ * the work around can be further optimized. We can keep shadow values of
+ * BLOCK_SIZE, BLOCK_COUNT, and TRANSFER_MODE until a COMMAND is issued.
+ * Then, write the BLOCK_SIZE+BLOCK_COUNT in a single 32-bit write followed
+ * by the TRANSFER+COMMAND in another 32-bit write.
+ */
+static void sdhci_iproc_writew(struct sdhci_host *host, u16 val, int reg)
+{
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct sdhci_iproc_host *iproc_host = pltfm_host->priv;
+	u32 word_shift = REG_OFFSET_IN_BITS(reg);
+	u32 mask = 0xffff << word_shift;
+	u32 oldval, newval;
+
+	if (reg == SDHCI_COMMAND) {
+		/* Write the block now as we are issuing a command */
+		if (iproc_host->shadow_blk != 0) {
+			sdhci_iproc_writel(host, iproc_host->shadow_blk,
+				SDHCI_BLOCK_SIZE);
+			iproc_host->shadow_blk = 0;
+		}
+		oldval = iproc_host->shadow_cmd;
+	} else if (reg == SDHCI_BLOCK_SIZE || reg == SDHCI_BLOCK_COUNT) {
+		/* Block size and count are stored in shadow reg */
+		oldval = iproc_host->shadow_blk;
+	} else {
+		/* Read reg, all other registers are not shadowed */
+		oldval = sdhci_iproc_readl(host, (reg & ~3));
+	}
+	newval = (oldval & ~mask) | (val << word_shift);
+
+	if (reg == SDHCI_TRANSFER_MODE) {
+		/* Save the transfer mode until the command is issued */
+		iproc_host->shadow_cmd = newval;
+	} else if (reg == SDHCI_BLOCK_SIZE || reg == SDHCI_BLOCK_COUNT) {
+		/* Save the block info until the command is issued */
+		iproc_host->shadow_blk = newval;
+	} else {
+		/* Command or other regular 32-bit write */
+		sdhci_iproc_writel(host, newval, reg & ~3);
+	}
+}
+
+static void sdhci_iproc_writeb(struct sdhci_host *host, u8 val, int reg)
+{
+	u32 oldval = sdhci_iproc_readl(host, (reg & ~3));
+	u32 byte_shift = REG_OFFSET_IN_BITS(reg);
+	u32 mask = 0xff << byte_shift;
+	u32 newval = (oldval & ~mask) | (val << byte_shift);
+
+	sdhci_iproc_writel(host, newval, reg & ~3);
+}
+
+static const struct sdhci_ops sdhci_iproc_ops = {
+	.read_l = sdhci_iproc_readl,
+	.read_w = sdhci_iproc_readw,
+	.read_b = sdhci_iproc_readb,
+	.write_l = sdhci_iproc_writel,
+	.write_w = sdhci_iproc_writew,
+	.write_b = sdhci_iproc_writeb,
+	.set_clock = sdhci_set_clock,
+	.get_max_clock = sdhci_pltfm_clk_get_max_clock,
+	.set_bus_width = sdhci_set_bus_width,
+	.reset = sdhci_reset,
+	.set_uhs_signaling = sdhci_set_uhs_signaling,
+};
+
+static const struct sdhci_pltfm_data sdhci_iproc_pltfm_data = {
+	.quirks = SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK,
+	.quirks2 = SDHCI_QUIRK2_ACMD23_BROKEN,
+	.ops = &sdhci_iproc_ops,
+};
+
+static const struct sdhci_iproc_data iproc_data = {
+	.pdata = &sdhci_iproc_pltfm_data,
+	.caps = 0x05E90000,
+	.caps1 = 0x00000064,
+};
+
+static const struct of_device_id sdhci_iproc_of_match[] = {
+	{ .compatible = "brcm,sdhci-iproc-cygnus", .data = &iproc_data },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, sdhci_iproc_of_match);
+
+static int sdhci_iproc_probe(struct platform_device *pdev)
+{
+	const struct of_device_id *match;
+	const struct sdhci_iproc_data *iproc_data;
+	struct sdhci_host *host;
+	struct sdhci_iproc_host *iproc_host;
+	struct sdhci_pltfm_host *pltfm_host;
+	int ret;
+
+	match = of_match_device(sdhci_iproc_of_match, &pdev->dev);
+	if (!match)
+		return -EINVAL;
+	iproc_data = match->data;
+
+	host = sdhci_pltfm_init(pdev, iproc_data->pdata, sizeof(*iproc_host));
+	if (IS_ERR(host))
+		return PTR_ERR(host);
+
+	pltfm_host = sdhci_priv(host);
+	iproc_host = sdhci_pltfm_priv(pltfm_host);
+
+	iproc_host->data = iproc_data;
+
+	mmc_of_parse(host->mmc);
+	sdhci_get_of_property(pdev);
+
+	/* Enable EMMC 1/8V DDR capable */
+	host->mmc->caps |= MMC_CAP_1_8V_DDR;
+
+	pltfm_host->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(pltfm_host->clk)) {
+		ret = PTR_ERR(pltfm_host->clk);
+		goto err;
+	}
+
+	if (iproc_host->data->pdata->quirks & SDHCI_QUIRK_MISSING_CAPS) {
+		host->caps = iproc_host->data->caps;
+		host->caps1 = iproc_host->data->caps1;
+	}
+
+	return sdhci_add_host(host);
+
+err:
+	sdhci_pltfm_free(pdev);
+	return ret;
+}
+
+static int sdhci_iproc_remove(struct platform_device *pdev)
+{
+	return sdhci_pltfm_unregister(pdev);
+}
+
+static struct platform_driver sdhci_iproc_driver = {
+	.driver = {
+		.name = "sdhci-iproc",
+		.of_match_table = sdhci_iproc_of_match,
+		.pm = SDHCI_PLTFM_PMOPS,
+	},
+	.probe = sdhci_iproc_probe,
+	.remove = sdhci_iproc_remove,
+};
+module_platform_driver(sdhci_iproc_driver);
+
+MODULE_AUTHOR("Broadcom");
+MODULE_DESCRIPTION("IPROC SDHCI driver");
+MODULE_LICENSE("GPL v2");
-- 
2.2.2

[PATCH 4/4] mmc: sdhci-iproc: add device tree bindings

[Scott Branden]

Add device tree binding documentation for IPROC SDHCI driver.

Acked-by: Ray Jui <rjui@broadcom.com>
Signed-off-by: Corneliu Doban <cdoban@broadcom.com>
Signed-off-by: Scott Branden <sbranden@broadcom.com>
---
 .../devicetree/bindings/mmc/brcm,sdhci-iproc.txt   | 23 ++++++++++++++++++++++
 1 file changed, 23 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mmc/brcm,sdhci-iproc.txt

diff --git a/Documentation/devicetree/bindings/mmc/brcm,sdhci-iproc.txt b/Documentation/devicetree/bindings/mmc/brcm,sdhci-iproc.txt
new file mode 100644
index 0000000..72cc9cc
--- /dev/null
+++ b/Documentation/devicetree/bindings/mmc/brcm,sdhci-iproc.txt
@@ -0,0 +1,23 @@
+Broadcom IPROC SDHCI controller
+
+This file documents differences between the core properties described
+by mmc.txt and the properties that represent the IPROC SDHCI controller.
+
+Required properties:
+- compatible : Should be "brcm,sdhci-iproc-cygnus".
+- clocks : The clock feeding the SDHCI controller.
+
+Optional properties:
+  - sdhci,auto-cmd12: specifies that controller should use auto CMD12.
+
+Example:
+
+sdhci0: sdhci@0x18041000 {
+	compatible = "brcm,sdhci-iproc-cygnus";
+	reg = <0x18041000 0x100>;
+	interrupts = <GIC_SPI 108 IRQ_TYPE_LEVEL_HIGH>;
+	clocks = <&lcpll0_clks BCM_CYGNUS_LCPLL0_SDIO_CLK>;
+	bus-width = <4>;
+	sdhci,auto-cmd12;
+	no-1-8-v;
+};
-- 
2.2.2

Re: [PATCH 4/4] mmc: sdhci-iproc: add device tree bindings

[Florian Fainelli]

On 09/02/15 16:06, Scott Branden wrote:
> Add device tree binding documentation for IPROC SDHCI driver.
> 
> Acked-by: Ray Jui <rjui@broadcom.com>
> Signed-off-by: Corneliu Doban <cdoban@broadcom.com>
> Signed-off-by: Scott Branden <sbranden@broadcom.com>
> ---
>  .../devicetree/bindings/mmc/brcm,sdhci-iproc.txt   | 23 ++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mmc/brcm,sdhci-iproc.txt
> 
> diff --git a/Documentation/devicetree/bindings/mmc/brcm,sdhci-iproc.txt b/Documentation/devicetree/bindings/mmc/brcm,sdhci-iproc.txt
> new file mode 100644
> index 0000000..72cc9cc
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mmc/brcm,sdhci-iproc.txt
> @@ -0,0 +1,23 @@
> +Broadcom IPROC SDHCI controller
> +
> +This file documents differences between the core properties described
> +by mmc.txt and the properties that represent the IPROC SDHCI controller.
> +
> +Required properties:
> +- compatible : Should be "brcm,sdhci-iproc-cygnus".
> +- clocks : The clock feeding the SDHCI controller.
> +
> +Optional properties:
> +  - sdhci,auto-cmd12: specifies that controller should use auto CMD12.

Formatting is a little different here, there is one too many space to
begin the line.

> +
> +Example:
> +
> +sdhci0: sdhci@0x18041000 {

0x is typically dropped from this part, to only appear in the "reg"
property.

> +	compatible = "brcm,sdhci-iproc-cygnus";
> +	reg = <0x18041000 0x100>;
> +	interrupts = <GIC_SPI 108 IRQ_TYPE_LEVEL_HIGH>;
> +	clocks = <&lcpll0_clks BCM_CYGNUS_LCPLL0_SDIO_CLK>;
> +	bus-width = <4>;
> +	sdhci,auto-cmd12;
> +	no-1-8-v;
> +};
> 

Unless there is a re-spin, I will fix this myself while applying this
patch to devicetree/next.

Thanks
-- 
Florian

Re: [PATCH 4/4] mmc: sdhci-iproc: add device tree bindings

[Scott Branden]

Hi Florian,

On 15-03-02 03:50 PM, Florian Fainelli wrote:
>
> Unless there is a re-spin, I will fix this myself while applying this
> patch to devicetree/next.
>
> Thanks
>
Yes, you can patch the formatting if the driver is being integrated. 
But I have had no response or feedback on the sdhci driver.  It has been 
sitting for 1 month.

Re: [PATCH 0/4] Add support for IPROC SDHCI controller

[Scott Branden]

Hi Chris,

I have not heard any more feedback on this patchset.  Is it in your 
queue to review or merge into your tree?

Thanks,
  Scott

On 15-02-09 04:06 PM, Scott Branden wrote:
> This series of patchsets contains the IPROC SDHCI driver used
> in a series of Broadcom SoCs
> Quirks are also added to support this controller.
>
> Corneliu Doban (1):
>    mmc: sdhci: do not set AUTO_CMD12 for multi-block CMD53
>
> Scott Branden (3):
>    mmc: sdhci: add quirk for ACMD23 broken
>    mmc: sdhci-iproc: add IPROC SDHCI driver
>    mmc: sdhci-iproc: add device tree bindings
>
>   .../devicetree/bindings/mmc/brcm,sdhci-iproc.txt   |  23 ++
>   drivers/mmc/host/Kconfig                           |  14 ++
>   drivers/mmc/host/Makefile                          |   1 +
>   drivers/mmc/host/sdhci-iproc.c                     | 241 +++++++++++++++++++++
>   drivers/mmc/host/sdhci.c                           |   7 +-
>   include/linux/mmc/sdhci.h                          |   2 +
>   6 files changed, 286 insertions(+), 2 deletions(-)
>   create mode 100644 Documentation/devicetree/bindings/mmc/brcm,sdhci-iproc.txt
>   create mode 100644 drivers/mmc/host/sdhci-iproc.c
>

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH RESEND 0/4] Add support for IPROC SDHCI controller

[Scott Branden]

This series of patchsets contains the IPROC SDHCI driver used
in a series of Broadcom SoCs
Quirks are also added to support this controller.

Corneliu Doban (1):
  mmc: sdhci: do not set AUTO_CMD12 for multi-block CMD53

Scott Branden (3):
  mmc: sdhci: add quirk for ACMD23 broken
  mmc: sdhci-iproc: add IPROC SDHCI driver
  mmc: sdhci-iproc: add device tree bindings

 .../devicetree/bindings/mmc/brcm,sdhci-iproc.txt   |  23 ++
 drivers/mmc/host/Kconfig                           |  14 ++
 drivers/mmc/host/Makefile                          |   1 +
 drivers/mmc/host/sdhci-iproc.c                     | 241 +++++++++++++++++++++
 drivers/mmc/host/sdhci.c                           |   7 +-
 include/linux/mmc/sdhci.h                          |   2 +
 6 files changed, 286 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mmc/brcm,sdhci-iproc.txt
 create mode 100644 drivers/mmc/host/sdhci-iproc.c

-- 
2.2.2

[PATCH 1/4] mmc: sdhci: add quirk for ACMD23 broken

[Scott Branden]

Add quirk to handle broken auto-CMD23.
Some controllers do not respond after the first auto-CMD23 is issued.

This allows CMD23 to still work (mandatory for the faster UHS-I mode)
rather than disabling CMD23 entirely via SDHCI_QUIRK2_HOST_NO_CMD23.

Signed-off by: Corneliu Doban <cdoban@broadcom.com>
Signed-off-by: Scott Branden <sbranden@broadcom.com>

Signed-off-by: Scott Branden <sbranden@broadcom.com>
---
 drivers/mmc/host/sdhci.c  | 3 ++-
 include/linux/mmc/sdhci.h | 2 ++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index f1a488e..fcf78ab 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -3085,7 +3085,8 @@ int sdhci_add_host(struct sdhci_host *host)
 	/* Auto-CMD23 stuff only works in ADMA or PIO. */
 	if ((host->version >= SDHCI_SPEC_300) &&
 	    ((host->flags & SDHCI_USE_ADMA) ||
-	     !(host->flags & SDHCI_USE_SDMA))) {
+	     !(host->flags & SDHCI_USE_SDMA)) &&
+	     !(host->quirks2 & SDHCI_QUIRK2_ACMD23_BROKEN)) {
 		host->flags |= SDHCI_AUTO_CMD23;
 		DBG("%s: Auto-CMD23 available\n", mmc_hostname(mmc));
 	} else {
diff --git a/include/linux/mmc/sdhci.h b/include/linux/mmc/sdhci.h
index f767a0d..a29e9bb 100644
--- a/include/linux/mmc/sdhci.h
+++ b/include/linux/mmc/sdhci.h
@@ -106,6 +106,8 @@ struct sdhci_host {
 #define SDHCI_QUIRK2_CLEAR_TRANSFERMODE_REG_BEFORE_CMD	(1<<10)
 /* Capability register bit-63 indicates HS400 support */
 #define SDHCI_QUIRK2_CAPS_BIT63_FOR_HS400		(1<<11)
+/* Controller broken with using ACMD23 */
+#define SDHCI_QUIRK2_ACMD23_BROKEN			(1<<12)
 
 	int irq;		/* Device IRQ */
 	void __iomem *ioaddr;	/* Mapped address */
-- 
2.2.2

[PATCH 2/4] mmc: sdhci: do not set AUTO_CMD12 for multi-block CMD53

[Scott Branden]

From: Corneliu Doban <cdoban@broadcom.com>

For CMD53 in block mode, the host does not need to stop the transfer,
as it stops when the block count (present in CMD53) is reached.

Signed-off-by: Corneliu Doban <cdoban@broadcom.com>
Signed-off-by: Scott Branden <sbranden@broadcom.com>
---
 drivers/mmc/host/sdhci.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index fcf78ab..f35d413 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -28,6 +28,7 @@
 #include <linux/mmc/mmc.h>
 #include <linux/mmc/host.h>
 #include <linux/mmc/card.h>
+#include <linux/mmc/sdio.h>
 #include <linux/mmc/slot-gpio.h>
 
 #include "sdhci.h"
@@ -934,7 +935,8 @@ static void sdhci_set_transfer_mode(struct sdhci_host *host,
 		 * If we are sending CMD23, CMD12 never gets sent
 		 * on successful completion (so no Auto-CMD12).
 		 */
-		if (!host->mrq->sbc && (host->flags & SDHCI_AUTO_CMD12))
+		if (!host->mrq->sbc && (host->flags & SDHCI_AUTO_CMD12) &&
+		    (cmd->opcode != SD_IO_RW_EXTENDED))
 			mode |= SDHCI_TRNS_AUTO_CMD12;
 		else if (host->mrq->sbc && (host->flags & SDHCI_AUTO_CMD23)) {
 			mode |= SDHCI_TRNS_AUTO_CMD23;
-- 
2.2.2

[PATCH 3/4] mmc: sdhci-iproc: add IPROC SDHCI driver

[Scott Branden]

Add IPROC SDHCI driver for IPROC family of Broadcom devices.

Acked-by: Ray Jui <rjui@broadcom.com>
Signed-off-by: Corneliu Doban <cdoban@broadcom.com>
Signed-off-by: Scott Branden <sbranden@broadcom.com>
---
 drivers/mmc/host/Kconfig       |  14 +++
 drivers/mmc/host/Makefile      |   1 +
 drivers/mmc/host/sdhci-iproc.c | 241 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 256 insertions(+)
 create mode 100644 drivers/mmc/host/sdhci-iproc.c

diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index 2d6fbdd..d6a2ff1 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -292,6 +292,20 @@ config MMC_SDHCI_BCM2835
 
 	  If unsure, say N.
 
+config MMC_SDHCI_IPROC
+	tristate "SDHCI platform support for the iProc SD/MMC Controller"
+	depends on ARCH_BCM_IPROC || COMPILE_TEST
+	depends on MMC_SDHCI_PLTFM
+	default ARCH_BCM_IPROC
+	select MMC_SDHCI_IO_ACCESSORS
+	help
+	  This selects the iProc SD/MMC controller.
+
+	  If you have an IPROC platform with SD or MMC devices,
+	  say Y or M here.
+
+	  If unsure, say N.
+
 config MMC_MOXART
 	tristate "MOXART SD/MMC Host Controller support"
 	depends on ARCH_MOXART && MMC
diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
index f7b0a77..32f24bd 100644
--- a/drivers/mmc/host/Makefile
+++ b/drivers/mmc/host/Makefile
@@ -70,6 +70,7 @@ obj-$(CONFIG_MMC_SDHCI_OF_ESDHC)	+= sdhci-of-esdhc.o
 obj-$(CONFIG_MMC_SDHCI_OF_HLWD)		+= sdhci-of-hlwd.o
 obj-$(CONFIG_MMC_SDHCI_BCM_KONA)	+= sdhci-bcm-kona.o
 obj-$(CONFIG_MMC_SDHCI_BCM2835)		+= sdhci-bcm2835.o
+obj-$(CONFIG_MMC_SDHCI_IPROC)		+= sdhci-iproc.o
 obj-$(CONFIG_MMC_SDHCI_MSM)		+= sdhci-msm.o
 obj-$(CONFIG_MMC_SDHCI_ST)		+= sdhci-st.o
 
diff --git a/drivers/mmc/host/sdhci-iproc.c b/drivers/mmc/host/sdhci-iproc.c
new file mode 100644
index 0000000..4139d34
--- /dev/null
+++ b/drivers/mmc/host/sdhci-iproc.c
@@ -0,0 +1,241 @@
+/*
+ * Copyright (C) 2014 Broadcom Corporation
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+/*
+ * iProc SDHCI platform driver
+ */
+
+#include <linux/delay.h>
+#include <linux/module.h>
+#include <linux/mmc/host.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include "sdhci-pltfm.h"
+
+struct sdhci_iproc_data {
+	const struct sdhci_pltfm_data *pdata;
+	u32 caps;
+	u32 caps1;
+};
+
+struct sdhci_iproc_host {
+	const struct sdhci_iproc_data *data;
+	u32 shadow_cmd;
+	u32 shadow_blk;
+};
+
+#define REG_OFFSET_IN_BITS(reg) ((reg) << 3 & 0x18)
+
+static inline u32 sdhci_iproc_readl(struct sdhci_host *host, int reg)
+{
+	u32 val = readl(host->ioaddr + reg);
+
+	pr_debug("%s: readl [0x%02x] 0x%08x\n",
+		 mmc_hostname(host->mmc), reg, val);
+	return val;
+}
+
+static u16 sdhci_iproc_readw(struct sdhci_host *host, int reg)
+{
+	u32 val = sdhci_iproc_readl(host, (reg & ~3));
+	u16 word = val >> REG_OFFSET_IN_BITS(reg) & 0xffff;
+	return word;
+}
+
+static u8 sdhci_iproc_readb(struct sdhci_host *host, int reg)
+{
+	u32 val = sdhci_iproc_readl(host, (reg & ~3));
+	u8 byte = val >> REG_OFFSET_IN_BITS(reg) & 0xff;
+	return byte;
+}
+
+static inline void sdhci_iproc_writel(struct sdhci_host *host, u32 val, int reg)
+{
+	pr_debug("%s: writel [0x%02x] 0x%08x\n",
+		 mmc_hostname(host->mmc), reg, val);
+
+	writel(val, host->ioaddr + reg);
+
+	if (host->clock <= 400000) {
+		/* Round up to micro-second four SD clock delay */
+		if (host->clock)
+			udelay((4 * 1000000 + host->clock - 1) / host->clock);
+		else
+			udelay(10);
+	}
+}
+
+/*
+ * The Arasan has a bugette whereby it may lose the content of successive
+ * writes to the same register that are within two SD-card clock cycles of
+ * each other (a clock domain crossing problem). The data
+ * register does not have this problem, which is just as well - otherwise we'd
+ * have to nobble the DMA engine too.
+ *
+ * This wouldn't be a problem with the code except that we can only write the
+ * controller with 32-bit writes.  So two different 16-bit registers are
+ * written back to back creates the problem.
+ *
+ * In reality, this only happens when SDHCI_BLOCK_SIZE and SDHCI_BLOCK_COUNT
+ * are written followed by SDHCI_TRANSFER_MODE and SDHCI_COMMAND.
+ * The BLOCK_SIZE and BLOCK_COUNT are meaningless until a command issued so
+ * the work around can be further optimized. We can keep shadow values of
+ * BLOCK_SIZE, BLOCK_COUNT, and TRANSFER_MODE until a COMMAND is issued.
+ * Then, write the BLOCK_SIZE+BLOCK_COUNT in a single 32-bit write followed
+ * by the TRANSFER+COMMAND in another 32-bit write.
+ */
+static void sdhci_iproc_writew(struct sdhci_host *host, u16 val, int reg)
+{
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct sdhci_iproc_host *iproc_host = pltfm_host->priv;
+	u32 word_shift = REG_OFFSET_IN_BITS(reg);
+	u32 mask = 0xffff << word_shift;
+	u32 oldval, newval;
+
+	if (reg == SDHCI_COMMAND) {
+		/* Write the block now as we are issuing a command */
+		if (iproc_host->shadow_blk != 0) {
+			sdhci_iproc_writel(host, iproc_host->shadow_blk,
+				SDHCI_BLOCK_SIZE);
+			iproc_host->shadow_blk = 0;
+		}
+		oldval = iproc_host->shadow_cmd;
+	} else if (reg == SDHCI_BLOCK_SIZE || reg == SDHCI_BLOCK_COUNT) {
+		/* Block size and count are stored in shadow reg */
+		oldval = iproc_host->shadow_blk;
+	} else {
+		/* Read reg, all other registers are not shadowed */
+		oldval = sdhci_iproc_readl(host, (reg & ~3));
+	}
+	newval = (oldval & ~mask) | (val << word_shift);
+
+	if (reg == SDHCI_TRANSFER_MODE) {
+		/* Save the transfer mode until the command is issued */
+		iproc_host->shadow_cmd = newval;
+	} else if (reg == SDHCI_BLOCK_SIZE || reg == SDHCI_BLOCK_COUNT) {
+		/* Save the block info until the command is issued */
+		iproc_host->shadow_blk = newval;
+	} else {
+		/* Command or other regular 32-bit write */
+		sdhci_iproc_writel(host, newval, reg & ~3);
+	}
+}
+
+static void sdhci_iproc_writeb(struct sdhci_host *host, u8 val, int reg)
+{
+	u32 oldval = sdhci_iproc_readl(host, (reg & ~3));
+	u32 byte_shift = REG_OFFSET_IN_BITS(reg);
+	u32 mask = 0xff << byte_shift;
+	u32 newval = (oldval & ~mask) | (val << byte_shift);
+
+	sdhci_iproc_writel(host, newval, reg & ~3);
+}
+
+static const struct sdhci_ops sdhci_iproc_ops = {
+	.read_l = sdhci_iproc_readl,
+	.read_w = sdhci_iproc_readw,
+	.read_b = sdhci_iproc_readb,
+	.write_l = sdhci_iproc_writel,
+	.write_w = sdhci_iproc_writew,
+	.write_b = sdhci_iproc_writeb,
+	.set_clock = sdhci_set_clock,
+	.get_max_clock = sdhci_pltfm_clk_get_max_clock,
+	.set_bus_width = sdhci_set_bus_width,
+	.reset = sdhci_reset,
+	.set_uhs_signaling = sdhci_set_uhs_signaling,
+};
+
+static const struct sdhci_pltfm_data sdhci_iproc_pltfm_data = {
+	.quirks = SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK,
+	.quirks2 = SDHCI_QUIRK2_ACMD23_BROKEN,
+	.ops = &sdhci_iproc_ops,
+};
+
+static const struct sdhci_iproc_data iproc_data = {
+	.pdata = &sdhci_iproc_pltfm_data,
+	.caps = 0x05E90000,
+	.caps1 = 0x00000064,
+};
+
+static const struct of_device_id sdhci_iproc_of_match[] = {
+	{ .compatible = "brcm,sdhci-iproc-cygnus", .data = &iproc_data },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, sdhci_iproc_of_match);
+
+static int sdhci_iproc_probe(struct platform_device *pdev)
+{
+	const struct of_device_id *match;
+	const struct sdhci_iproc_data *iproc_data;
+	struct sdhci_host *host;
+	struct sdhci_iproc_host *iproc_host;
+	struct sdhci_pltfm_host *pltfm_host;
+	int ret;
+
+	match = of_match_device(sdhci_iproc_of_match, &pdev->dev);
+	if (!match)
+		return -EINVAL;
+	iproc_data = match->data;
+
+	host = sdhci_pltfm_init(pdev, iproc_data->pdata, sizeof(*iproc_host));
+	if (IS_ERR(host))
+		return PTR_ERR(host);
+
+	pltfm_host = sdhci_priv(host);
+	iproc_host = sdhci_pltfm_priv(pltfm_host);
+
+	iproc_host->data = iproc_data;
+
+	mmc_of_parse(host->mmc);
+	sdhci_get_of_property(pdev);
+
+	/* Enable EMMC 1/8V DDR capable */
+	host->mmc->caps |= MMC_CAP_1_8V_DDR;
+
+	pltfm_host->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(pltfm_host->clk)) {
+		ret = PTR_ERR(pltfm_host->clk);
+		goto err;
+	}
+
+	if (iproc_host->data->pdata->quirks & SDHCI_QUIRK_MISSING_CAPS) {
+		host->caps = iproc_host->data->caps;
+		host->caps1 = iproc_host->data->caps1;
+	}
+
+	return sdhci_add_host(host);
+
+err:
+	sdhci_pltfm_free(pdev);
+	return ret;
+}
+
+static int sdhci_iproc_remove(struct platform_device *pdev)
+{
+	return sdhci_pltfm_unregister(pdev);
+}
+
+static struct platform_driver sdhci_iproc_driver = {
+	.driver = {
+		.name = "sdhci-iproc",
+		.of_match_table = sdhci_iproc_of_match,
+		.pm = SDHCI_PLTFM_PMOPS,
+	},
+	.probe = sdhci_iproc_probe,
+	.remove = sdhci_iproc_remove,
+};
+module_platform_driver(sdhci_iproc_driver);
+
+MODULE_AUTHOR("Broadcom");
+MODULE_DESCRIPTION("IPROC SDHCI driver");
+MODULE_LICENSE("GPL v2");
-- 
2.2.2

[PATCH 4/4] mmc: sdhci-iproc: add device tree bindings

[Scott Branden]

Add device tree binding documentation for IPROC SDHCI driver.

Acked-by: Ray Jui <rjui@broadcom.com>
Signed-off-by: Corneliu Doban <cdoban@broadcom.com>
Signed-off-by: Scott Branden <sbranden@broadcom.com>
---
 .../devicetree/bindings/mmc/brcm,sdhci-iproc.txt   | 23 ++++++++++++++++++++++
 1 file changed, 23 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mmc/brcm,sdhci-iproc.txt

diff --git a/Documentation/devicetree/bindings/mmc/brcm,sdhci-iproc.txt b/Documentation/devicetree/bindings/mmc/brcm,sdhci-iproc.txt
new file mode 100644
index 0000000..72cc9cc
--- /dev/null
+++ b/Documentation/devicetree/bindings/mmc/brcm,sdhci-iproc.txt
@@ -0,0 +1,23 @@
+Broadcom IPROC SDHCI controller
+
+This file documents differences between the core properties described
+by mmc.txt and the properties that represent the IPROC SDHCI controller.
+
+Required properties:
+- compatible : Should be "brcm,sdhci-iproc-cygnus".
+- clocks : The clock feeding the SDHCI controller.
+
+Optional properties:
+  - sdhci,auto-cmd12: specifies that controller should use auto CMD12.
+
+Example:
+
+sdhci0: sdhci@0x18041000 {
+	compatible = "brcm,sdhci-iproc-cygnus";
+	reg = <0x18041000 0x100>;
+	interrupts = <GIC_SPI 108 IRQ_TYPE_LEVEL_HIGH>;
+	clocks = <&lcpll0_clks BCM_CYGNUS_LCPLL0_SDIO_CLK>;
+	bus-width = <4>;
+	sdhci,auto-cmd12;
+	no-1-8-v;
+};
-- 
2.2.2

Re: [PATCH RESEND 0/4] Add support for IPROC SDHCI controller

[Ulf Hansson]

On 5 March 2015 at 16:59, Scott Branden <sbranden@broadcom.com> wrote:
> This series of patchsets contains the IPROC SDHCI driver used
> in a series of Broadcom SoCs
> Quirks are also added to support this controller.
>
> Corneliu Doban (1):
>   mmc: sdhci: do not set AUTO_CMD12 for multi-block CMD53
>
> Scott Branden (3):
>   mmc: sdhci: add quirk for ACMD23 broken
>   mmc: sdhci-iproc: add IPROC SDHCI driver
>   mmc: sdhci-iproc: add device tree bindings
>
>  .../devicetree/bindings/mmc/brcm,sdhci-iproc.txt   |  23 ++
>  drivers/mmc/host/Kconfig                           |  14 ++
>  drivers/mmc/host/Makefile                          |   1 +
>  drivers/mmc/host/sdhci-iproc.c                     | 241 +++++++++++++++++++++
>  drivers/mmc/host/sdhci.c                           |   7 +-
>  include/linux/mmc/sdhci.h                          |   2 +
>  6 files changed, 286 insertions(+), 2 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/mmc/brcm,sdhci-iproc.txt
>  create mode 100644 drivers/mmc/host/sdhci-iproc.c
>
> --
> 2.2.2
>

Applied, thanks!

Actually, I needed to re-solve some conflicts and I switched to order
of patch3 and patch4, to avoid checkpatch warnings. Just so you know.

Kind regards
Uffe

Re: [PATCH RESEND 0/4] Add support for IPROC SDHCI controller

[Florian Fainelli]

On 05/03/15 08:16, Ulf Hansson wrote:
> On 5 March 2015 at 16:59, Scott Branden <sbranden@broadcom.com> wrote:
>> This series of patchsets contains the IPROC SDHCI driver used
>> in a series of Broadcom SoCs
>> Quirks are also added to support this controller.
>>
>> Corneliu Doban (1):
>>   mmc: sdhci: do not set AUTO_CMD12 for multi-block CMD53
>>
>> Scott Branden (3):
>>   mmc: sdhci: add quirk for ACMD23 broken
>>   mmc: sdhci-iproc: add IPROC SDHCI driver
>>   mmc: sdhci-iproc: add device tree bindings
>>
>>  .../devicetree/bindings/mmc/brcm,sdhci-iproc.txt   |  23 ++
>>  drivers/mmc/host/Kconfig                           |  14 ++
>>  drivers/mmc/host/Makefile                          |   1 +
>>  drivers/mmc/host/sdhci-iproc.c                     | 241 +++++++++++++++++++++
>>  drivers/mmc/host/sdhci.c                           |   7 +-
>>  include/linux/mmc/sdhci.h                          |   2 +
>>  6 files changed, 286 insertions(+), 2 deletions(-)
>>  create mode 100644 Documentation/devicetree/bindings/mmc/brcm,sdhci-iproc.txt
>>  create mode 100644 drivers/mmc/host/sdhci-iproc.c
>>
>> --
>> 2.2.2
>>
> 
> Applied, thanks!
> 
> Actually, I needed to re-solve some conflicts and I switched to order
> of patch3 and patch4, to avoid checkpatch warnings. Just so you know.

Thanks, patch 4 dropped from devicetree/next.
-- 
Florian

[PATCH] mmc: sdhci: fix card presence logic in sdhci_request function

[Scott Branden]

The sdhci_request function should consider a non-removable device
always present.
Call the correct logic already available in sdhci_do_get_cd function.

This fixes some logic paths where MMC requests are being made to
non-removable devices that do not have the card detect pin connected
on the hardware as it is non-removable.

Signed-off-by: Scott Branden <sbranden@broadcom.com>
---
 drivers/mmc/host/sdhci.c | 20 +++-----------------
 1 file changed, 3 insertions(+), 17 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 0ad412a..bafc7ac 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -56,6 +56,7 @@ static void sdhci_enable_preset_value(struct sdhci_host *host, bool enable);
 static int sdhci_pre_dma_transfer(struct sdhci_host *host,
 					struct mmc_data *data,
 					struct sdhci_host_next *next);
+static int sdhci_do_get_cd(struct sdhci_host *host);
 
 #ifdef CONFIG_PM
 static int sdhci_runtime_pm_get(struct sdhci_host *host);
@@ -1356,7 +1357,8 @@ static void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
 
 	sdhci_runtime_pm_get(host);
 
-	present = mmc_gpio_get_cd(host->mmc);
+	/* Firstly check card presence */
+	present = sdhci_do_get_cd(host);
 
 	spin_lock_irqsave(&host->lock, flags);
 
@@ -1379,22 +1381,6 @@ static void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
 
 	host->mrq = mrq;
 
-	/*
-	 * Firstly check card presence from cd-gpio.  The return could
-	 * be one of the following possibilities:
-	 *     negative: cd-gpio is not available
-	 *     zero: cd-gpio is used, and card is removed
-	 *     one: cd-gpio is used, and card is present
-	 */
-	if (present < 0) {
-		/* If polling, assume that the card is always present. */
-		if (host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION)
-			present = 1;
-		else
-			present = sdhci_readl(host, SDHCI_PRESENT_STATE) &
-					SDHCI_CARD_PRESENT;
-	}
-
 	if (!present || host->flags & SDHCI_DEVICE_DEAD) {
 		host->mrq->cmd->error = -ENOMEDIUM;
 		tasklet_schedule(&host->finish_tasklet);
-- 
2.3.0

Re: [PATCH] mmc: sdhci: fix card presence logic in sdhci_request function

[Ulf Hansson]

On 10 March 2015 at 19:35, Scott Branden <sbranden@broadcom.com> wrote:
> The sdhci_request function should consider a non-removable device
> always present.
> Call the correct logic already available in sdhci_do_get_cd function.
>
> This fixes some logic paths where MMC requests are being made to
> non-removable devices that do not have the card detect pin connected
> on the hardware as it is non-removable.
>
> Signed-off-by: Scott Branden <sbranden@broadcom.com>

Thanks, applied!

Kind regards
Uffe

> ---
>  drivers/mmc/host/sdhci.c | 20 +++-----------------
>  1 file changed, 3 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 0ad412a..bafc7ac 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -56,6 +56,7 @@ static void sdhci_enable_preset_value(struct sdhci_host *host, bool enable);
>  static int sdhci_pre_dma_transfer(struct sdhci_host *host,
>                                         struct mmc_data *data,
>                                         struct sdhci_host_next *next);
> +static int sdhci_do_get_cd(struct sdhci_host *host);
>
>  #ifdef CONFIG_PM
>  static int sdhci_runtime_pm_get(struct sdhci_host *host);
> @@ -1356,7 +1357,8 @@ static void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
>
>         sdhci_runtime_pm_get(host);
>
> -       present = mmc_gpio_get_cd(host->mmc);
> +       /* Firstly check card presence */
> +       present = sdhci_do_get_cd(host);
>
>         spin_lock_irqsave(&host->lock, flags);
>
> @@ -1379,22 +1381,6 @@ static void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
>
>         host->mrq = mrq;
>
> -       /*
> -        * Firstly check card presence from cd-gpio.  The return could
> -        * be one of the following possibilities:
> -        *     negative: cd-gpio is not available
> -        *     zero: cd-gpio is used, and card is removed
> -        *     one: cd-gpio is used, and card is present
> -        */
> -       if (present < 0) {
> -               /* If polling, assume that the card is always present. */
> -               if (host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION)
> -                       present = 1;
> -               else
> -                       present = sdhci_readl(host, SDHCI_PRESENT_STATE) &
> -                                       SDHCI_CARD_PRESENT;
> -       }
> -
>         if (!present || host->flags & SDHCI_DEVICE_DEAD) {
>                 host->mrq->cmd->error = -ENOMEDIUM;
>                 tasklet_schedule(&host->finish_tasklet);
> --
> 2.3.0
>