| Message ID | 20191204085447.27491-1-linux@rasmusvillemoes.dk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mmc: sdhci-of-esdhc: Revert "mmc: sdhci-of-esdhc: add erratum A-009204 support" | expand |
ping, any comments on this revert? On 04/12/2019 09.54, Rasmus Villemoes wrote: > This reverts commit 5dd195522562542bc6ebe6e7bd47890d8b7ca93c. > > First, the fix seems to be plain wrong, since the erratum suggests > waiting 5ms before setting setting SYSCTL[RSTD], but this msleep() > happens after the call of sdhci_reset() which is where that bit gets > set (if SDHCI_RESET_DATA is in mask).
On Wed, 4 Dec 2019 at 09:54, Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote: > > This reverts commit 5dd195522562542bc6ebe6e7bd47890d8b7ca93c. > > First, the fix seems to be plain wrong, since the erratum suggests > waiting 5ms before setting setting SYSCTL[RSTD], but this msleep() > happens after the call of sdhci_reset() which is where that bit gets > set (if SDHCI_RESET_DATA is in mask). > > Second, walking the whole device tree to figure out if some node has a > "fsl,p2020-esdhc" compatible string is hugely expensive - about 70 to > 100 us on our mpc8309 board. Walking the device tree is done under a > raw_spin_lock, so this is obviously really bad on an -rt system, and a > waste of time on all. > > In fact, since esdhc_reset() seems to get called around 100 times per > second, that mpc8309 now spends 0.8% of its time determining that > it is not a p2020. Whether those 100 calls/s are normal or due to some > other bug or misconfiguration, regularly hitting a 100 us > non-preemptible window is unacceptable. > > Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> Applied for fixes and by adding a stable tag, thanks! Kind regards Uffe > --- > > The errata sheet for mpc8309 also mentions A-009204, so I'm not at all > opposed to having a fix for that. But it needs to be done properly > without causing a huge performance or latency impact. We should > probably just add a bit to struct sdhci_esdhc which gets initialized > in esdhc_init. > > drivers/mmc/host/sdhci-of-esdhc.c | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/drivers/mmc/host/sdhci-of-esdhc.c b/drivers/mmc/host/sdhci-of-esdhc.c > index 5cca3fa4610b..7f87a90bf56a 100644 > --- a/drivers/mmc/host/sdhci-of-esdhc.c > +++ b/drivers/mmc/host/sdhci-of-esdhc.c > @@ -764,9 +764,6 @@ static void esdhc_reset(struct sdhci_host *host, u8 mask) > sdhci_writel(host, host->ier, SDHCI_INT_ENABLE); > sdhci_writel(host, host->ier, SDHCI_SIGNAL_ENABLE); > > - if (of_find_compatible_node(NULL, NULL, "fsl,p2020-esdhc")) > - mdelay(5); > - > if (mask & SDHCI_RESET_ALL) { > val = sdhci_readl(host, ESDHC_TBCTL); > val &= ~ESDHC_TB_EN; > -- > 2.23.0 >
Yinbo, Yangbo On Tue, 10 Dec 2019 at 10:51, Ulf Hansson <ulf.hansson@linaro.org> wrote: > > On Wed, 4 Dec 2019 at 09:54, Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote: > > > > This reverts commit 5dd195522562542bc6ebe6e7bd47890d8b7ca93c. > > > > First, the fix seems to be plain wrong, since the erratum suggests > > waiting 5ms before setting setting SYSCTL[RSTD], but this msleep() > > happens after the call of sdhci_reset() which is where that bit gets > > set (if SDHCI_RESET_DATA is in mask). > > > > Second, walking the whole device tree to figure out if some node has a > > "fsl,p2020-esdhc" compatible string is hugely expensive - about 70 to > > 100 us on our mpc8309 board. Walking the device tree is done under a > > raw_spin_lock, so this is obviously really bad on an -rt system, and a > > waste of time on all. > > > > In fact, since esdhc_reset() seems to get called around 100 times per > > second, that mpc8309 now spends 0.8% of its time determining that > > it is not a p2020. Whether those 100 calls/s are normal or due to some > > other bug or misconfiguration, regularly hitting a 100 us > > non-preemptible window is unacceptable. > > > > Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> > > Applied for fixes and by adding a stable tag, thanks! Just wanted to highlight, that $subject patch has been applied for fixes, which means we need a new fix the errata A-009204. Rasmus kind of already hinted on how this could be fixed in a better way, hope this helps. Kind regards Uffe > > --- > > > > The errata sheet for mpc8309 also mentions A-009204, so I'm not at all > > opposed to having a fix for that. But it needs to be done properly > > without causing a huge performance or latency impact. We should > > probably just add a bit to struct sdhci_esdhc which gets initialized > > in esdhc_init. > > > > drivers/mmc/host/sdhci-of-esdhc.c | 3 --- > > 1 file changed, 3 deletions(-) > > > > diff --git a/drivers/mmc/host/sdhci-of-esdhc.c b/drivers/mmc/host/sdhci-of-esdhc.c > > index 5cca3fa4610b..7f87a90bf56a 100644 > > --- a/drivers/mmc/host/sdhci-of-esdhc.c > > +++ b/drivers/mmc/host/sdhci-of-esdhc.c > > @@ -764,9 +764,6 @@ static void esdhc_reset(struct sdhci_host *host, u8 mask) > > sdhci_writel(host, host->ier, SDHCI_INT_ENABLE); > > sdhci_writel(host, host->ier, SDHCI_SIGNAL_ENABLE); > > > > - if (of_find_compatible_node(NULL, NULL, "fsl,p2020-esdhc")) > > - mdelay(5); > > - > > if (mask & SDHCI_RESET_ALL) { > > val = sdhci_readl(host, ESDHC_TBCTL); > > val &= ~ESDHC_TB_EN; > > -- > > 2.23.0 > >
> -----Original Message----- > From: linux-mmc-owner@vger.kernel.org <linux-mmc-owner@vger.kernel.org> > On Behalf Of Ulf Hansson > Sent: Wednesday, December 18, 2019 9:40 PM > To: Yinbo Zhu <yinbo.zhu@nxp.com>; Y.b. Lu <yangbo.lu@nxp.com> > Cc: Adrian Hunter <adrian.hunter@intel.com>; Rasmus Villemoes > <linux@rasmusvillemoes.dk>; linux-mmc@vger.kernel.org; Linux Kernel > Mailing List <linux-kernel@vger.kernel.org> > Subject: Re: [PATCH] mmc: sdhci-of-esdhc: Revert "mmc: sdhci-of-esdhc: add > erratum A-009204 support" > > Yinbo, Yangbo > > On Tue, 10 Dec 2019 at 10:51, Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > On Wed, 4 Dec 2019 at 09:54, Rasmus Villemoes > <linux@rasmusvillemoes.dk> wrote: > > > > > > This reverts commit 5dd195522562542bc6ebe6e7bd47890d8b7ca93c. > > > > > > First, the fix seems to be plain wrong, since the erratum suggests > > > waiting 5ms before setting setting SYSCTL[RSTD], but this msleep() > > > happens after the call of sdhci_reset() which is where that bit gets > > > set (if SDHCI_RESET_DATA is in mask). > > > > > > Second, walking the whole device tree to figure out if some node has a > > > "fsl,p2020-esdhc" compatible string is hugely expensive - about 70 to > > > 100 us on our mpc8309 board. Walking the device tree is done under a > > > raw_spin_lock, so this is obviously really bad on an -rt system, and a > > > waste of time on all. > > > > > > In fact, since esdhc_reset() seems to get called around 100 times per > > > second, that mpc8309 now spends 0.8% of its time determining that > > > it is not a p2020. Whether those 100 calls/s are normal or due to some > > > other bug or misconfiguration, regularly hitting a 100 us > > > non-preemptible window is unacceptable. > > > > > > Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> > > > > Applied for fixes and by adding a stable tag, thanks! > > Just wanted to highlight, that $subject patch has been applied for > fixes, which means we need a new fix the errata A-009204. > > Rasmus kind of already hinted on how this could be fixed in a better > way, hope this helps. > Sorry Uffe, I missed the patch since my name wasn't in TO/CC list. I have sent out a patch for re-implementation. Thanks! > Kind regards > Uffe > > > > --- > > > > > > The errata sheet for mpc8309 also mentions A-009204, so I'm not at all > > > opposed to having a fix for that. But it needs to be done properly > > > without causing a huge performance or latency impact. We should > > > probably just add a bit to struct sdhci_esdhc which gets initialized > > > in esdhc_init. > > > > > > drivers/mmc/host/sdhci-of-esdhc.c | 3 --- > > > 1 file changed, 3 deletions(-) > > > > > > diff --git a/drivers/mmc/host/sdhci-of-esdhc.c > b/drivers/mmc/host/sdhci-of-esdhc.c > > > index 5cca3fa4610b..7f87a90bf56a 100644 > > > --- a/drivers/mmc/host/sdhci-of-esdhc.c > > > +++ b/drivers/mmc/host/sdhci-of-esdhc.c > > > @@ -764,9 +764,6 @@ static void esdhc_reset(struct sdhci_host *host, u8 > mask) > > > sdhci_writel(host, host->ier, SDHCI_INT_ENABLE); > > > sdhci_writel(host, host->ier, SDHCI_SIGNAL_ENABLE); > > > > > > - if (of_find_compatible_node(NULL, NULL, "fsl,p2020-esdhc")) > > > - mdelay(5); > > > - > > > if (mask & SDHCI_RESET_ALL) { > > > val = sdhci_readl(host, ESDHC_TBCTL); > > > val &= ~ESDHC_TB_EN; > > > -- > > > 2.23.0 > > >
diff --git a/drivers/mmc/host/sdhci-of-esdhc.c b/drivers/mmc/host/sdhci-of-esdhc.c index 5cca3fa4610b..7f87a90bf56a 100644 --- a/drivers/mmc/host/sdhci-of-esdhc.c +++ b/drivers/mmc/host/sdhci-of-esdhc.c @@ -764,9 +764,6 @@ static void esdhc_reset(struct sdhci_host *host, u8 mask) sdhci_writel(host, host->ier, SDHCI_INT_ENABLE); sdhci_writel(host, host->ier, SDHCI_SIGNAL_ENABLE); - if (of_find_compatible_node(NULL, NULL, "fsl,p2020-esdhc")) - mdelay(5); - if (mask & SDHCI_RESET_ALL) { val = sdhci_readl(host, ESDHC_TBCTL); val &= ~ESDHC_TB_EN;
This reverts commit 5dd195522562542bc6ebe6e7bd47890d8b7ca93c. First, the fix seems to be plain wrong, since the erratum suggests waiting 5ms before setting setting SYSCTL[RSTD], but this msleep() happens after the call of sdhci_reset() which is where that bit gets set (if SDHCI_RESET_DATA is in mask). Second, walking the whole device tree to figure out if some node has a "fsl,p2020-esdhc" compatible string is hugely expensive - about 70 to 100 us on our mpc8309 board. Walking the device tree is done under a raw_spin_lock, so this is obviously really bad on an -rt system, and a waste of time on all. In fact, since esdhc_reset() seems to get called around 100 times per second, that mpc8309 now spends 0.8% of its time determining that it is not a p2020. Whether those 100 calls/s are normal or due to some other bug or misconfiguration, regularly hitting a 100 us non-preemptible window is unacceptable. Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> --- The errata sheet for mpc8309 also mentions A-009204, so I'm not at all opposed to having a fix for that. But it needs to be done properly without causing a huge performance or latency impact. We should probably just add a bit to struct sdhci_esdhc which gets initialized in esdhc_init. drivers/mmc/host/sdhci-of-esdhc.c | 3 --- 1 file changed, 3 deletions(-)