| Message ID | 1414651017-3545-5-git-send-email-sbranden@broadcom.com |
|---|---|
| State | New |
| Headers | show |
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). -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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, > > > > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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); }
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(+)