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
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 <[email protected]>
---
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
SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12 is missing and needed for this controller.
Signed-off-by: Scott Branden <[email protected]>
---
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
Group the read and write functions to improve readability. Now all
similar functions are grouped together to evaluate behaviours.
Signed-off-by: Scott Branden <[email protected]>
---
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
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 <[email protected]>
---
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
Make the shift calculations consistent rather than having different
implementations to calculate the same thing.
Signed-off-by: Scott Branden <[email protected]>
---
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
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).
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...)
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?
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.
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.
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.
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
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
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.
>
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.
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.
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?
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.
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.
>
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.
>
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.
>
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,
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,
>
>
>
>
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