Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1408833 > unrolled thread

[PATCH] mmc: sdhci: use udelay instead of mdelay

Back to article view | Back to linux.kernel

Contents

  [PATCH] mmc: sdhci: use udelay instead of mdelay "Baranowska, BeataX" <beatax.baranowska@intel.com> - 2016-05-30 10:00 +0200
    Re: [PATCH] mmc: sdhci: use udelay instead of mdelay Arnd Bergmann <arnd@arndb.de> - 2016-05-30 10:10 +0200
      RE: [PATCH] mmc: sdhci: use udelay instead of mdelay "Baranowska, BeataX" <beatax.baranowska@intel.com> - 2016-05-31 11:00 +0200
        Re: [PATCH] mmc: sdhci: use udelay instead of mdelay Arnd Bergmann <arnd@arndb.de> - 2016-05-31 11:20 +0200
          RE: [PATCH] mmc: sdhci: use udelay instead of mdelay "Baranowska, BeataX" <beatax.baranowska@intel.com> - 2016-05-31 11:40 +0200

#1408833 — [PATCH] mmc: sdhci: use udelay instead of mdelay

From: Chuanxiao Dong <chuanxiao.dong@intel.com>

This patch will use udelay instead of mdelay when waiting for
SDHCI hardware to be stable. udelay can help to reduce the waiting
time when is in critical region which is protected by spinlock.

With this patch, __sdhci_set_ios only take a few microseconds to be
done.

Signed-off-by: Chuanxiao Dong <chuanxiao.dong@intel.com>
---
drivers/mmc/host/sdhci.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index e010ea4eb6f5..56d2c7567d97 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -173,8 +173,8 @@ void sdhci_reset(struct sdhci_host *host, u8 mask)
                        sdhci_runtime_pm_bus_off(host);
        }

-       /* Wait max 100 ms */
-       timeout = 100;
+       /* Wait max 10000 ms */
+       timeout = 10000;

        /* hw clears the bit when it's done */
        while (sdhci_readb(host, SDHCI_SOFTWARE_RESET) & mask) {
@@ -185,7 +185,7 @@ void sdhci_reset(struct sdhci_host *host, u8 mask)
                        return;
                }
                timeout--;
-               mdelay(1);
+               udelay(10);
        }
 }
 EXPORT_SYMBOL_GPL(sdhci_reset);
@@ -985,8 +985,8 @@ void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
        /* Initially, a command has no error */
        cmd->error = 0;

-       /* Wait max 10 ms */
-       timeout = 10;
+       /* Wait max 1000 ms */
+       timeout = 1000;

        mask = SDHCI_CMD_INHIBIT;
        if ((cmd->data != NULL) || (cmd->flags & MMC_RSP_BUSY))
@@ -1007,7 +1007,7 @@ void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
                        return;
                }
                timeout--;
-               mdelay(1);
+               udelay(10);
        }

        timeout = jiffies;
@@ -1240,8 +1240,8 @@ void sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
        clk |= SDHCI_CLOCK_INT_EN;
        sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);

-       /* Wait max 20 ms */
-       timeout = 20;
+       /* Wait max 2000 ms */
+       timeout = 2000;
        while (!((clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL))
                & SDHCI_CLOCK_INT_STABLE)) {
                if (timeout == 0) {
@@ -1251,7 +1251,7 @@ void sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
                        return;
                }
                timeout--;
-               mdelay(1);
+               udelay(10);
        }

        clk |= SDHCI_CLOCK_CARD_EN;
--


Regards,
Beata Baranowska

Intel Technology Poland sp. z o.o.
ul. Słowackiego 173 | 80-298 Gdańsk 
Sąd Rejonowy Gdańsk Północ 
VII Wydział Gospodarczy Krajowego Rejestru Sądowego - KRS 101882 
NIP 957-07-52-316 | Kapitał zakładowy 200.000 PLN

[toc] | [next] | [standalone]

#1408839

On Monday, May 30, 2016 7:55:55 AM CEST Baranowska, BeataX wrote:
> From: Chuanxiao Dong <chuanxiao.dong@intel.com>
> 
> This patch will use udelay instead of mdelay when waiting for
> SDHCI hardware to be stable. udelay can help to reduce the waiting
> time when is in critical region which is protected by spinlock.
> 
> With this patch, __sdhci_set_ios only take a few microseconds to be
> done.
> 
> Signed-off-by: Chuanxiao Dong <chuanxiao.dong@intel.com>
> ---
> drivers/mmc/host/sdhci.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index e010ea4eb6f5..56d2c7567d97 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -173,8 +173,8 @@ void sdhci_reset(struct sdhci_host *host, u8 mask)
>                         sdhci_runtime_pm_bus_off(host);
>         }
> 
> -       /* Wait max 100 ms */
> -       timeout = 100;
> +       /* Wait max 10000 ms */
> +       timeout = 10000;
> 
>         /* hw clears the bit when it's done */
>         while (sdhci_readb(host, SDHCI_SOFTWARE_RESET) & mask) {
> @@ -185,7 +185,7 @@ void sdhci_reset(struct sdhci_host *host, u8 mask)
>                         return;
>                 }
>                 timeout--;
> -               mdelay(1);
> +               udelay(10);
>         }
>  }
>  EXPORT_SYMBOL_GPL(sdhci_reset);

This can significantly increase the timeout length. I think you should
instead use time_before() to see how many jiffies have passed since
the start.

However, the real question is why the reset function gets called under
a spinlock in the first place. Can you try to rearrange the code so
it doesn't need the lock at all and you can just use msleep() instead?

	Arnd

[toc] | [prev] | [next] | [standalone]

#1410006

> -----Original Message-----
> From: Arnd Bergmann [mailto:arnd@arndb.de]
> Sent: Monday, May 30, 2016 10:01 AM
> To: Baranowska, BeataX <beatax.baranowska@intel.com>
> Cc: Hunter, Adrian <adrian.hunter@intel.com>; Ulf Hansson
> <ulf.hansson@linaro.org>; linux-mmc@vger.kernel.org; linux-
> kernel@vger.kernel.org; Dong, Chuanxiao <chuanxiao.dong@intel.com>;
> Jarosz, SebastianX <sebastianx.jarosz@intel.com>
> Subject: Re: [PATCH] mmc: sdhci: use udelay instead of mdelay
> 
> On Monday, May 30, 2016 7:55:55 AM CEST Baranowska, BeataX wrote:
> > From: Chuanxiao Dong <chuanxiao.dong@intel.com>
> >
> > This patch will use udelay instead of mdelay when waiting for SDHCI
> > hardware to be stable. udelay can help to reduce the waiting time when
> > is in critical region which is protected by spinlock.
> >
> > With this patch, __sdhci_set_ios only take a few microseconds to be
> > done.
> >
> > Signed-off-by: Chuanxiao Dong <chuanxiao.dong@intel.com>
> > ---
> > drivers/mmc/host/sdhci.c | 18 +++++++++---------
> >  1 file changed, 9 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c index
> > e010ea4eb6f5..56d2c7567d97 100644
> > --- a/drivers/mmc/host/sdhci.c
> > +++ b/drivers/mmc/host/sdhci.c
> > @@ -173,8 +173,8 @@ void sdhci_reset(struct sdhci_host *host, u8 mask)
> >                         sdhci_runtime_pm_bus_off(host);
> >         }
> >
> > -       /* Wait max 100 ms */
> > -       timeout = 100;
> > +       /* Wait max 10000 ms */
> > +       timeout = 10000;
> >
> >         /* hw clears the bit when it's done */
> >         while (sdhci_readb(host, SDHCI_SOFTWARE_RESET) & mask) { @@
> > -185,7 +185,7 @@ void sdhci_reset(struct sdhci_host *host, u8 mask)
> >                         return;
> >                 }
> >                 timeout--;
> > -               mdelay(1);
> > +               udelay(10);
> >         }
> >  }
> >  EXPORT_SYMBOL_GPL(sdhci_reset);
> 
> This can significantly increase the timeout length. I think you should instead
> use time_before() to see how many jiffies have passed since the start.
> 
> However, the real question is why the reset function gets called under a
> spinlock in the first place. Can you try to rearrange the code so it doesn't
> need the lock at all and you can just use msleep() instead?
> 
> 	Arnd

Thank you for your quick reply.
Could you please clarify what do you mean is called under a spinlock? Any is not used here?

Beata Baranowska

[toc] | [prev] | [next] | [standalone]

#1410020

On Tuesday, May 31, 2016 8:53:18 AM CEST Baranowska, BeataX wrote:
> > 
> > On Monday, May 30, 2016 7:55:55 AM CEST Baranowska, BeataX wrote:
> > > From: Chuanxiao Dong <chuanxiao.dong@intel.com>
> > >
> > > This patch will use udelay instead of mdelay when waiting for SDHCI
> > > hardware to be stable. udelay can help to reduce the waiting time when
> > > is in critical region which is protected by spinlock.
> > >
> > > With this patch, __sdhci_set_ios only take a few microseconds to be
> > > done.
> > >
> > > Signed-off-by: Chuanxiao Dong <chuanxiao.dong@intel.com>
> > > ---
> > > drivers/mmc/host/sdhci.c | 18 +++++++++---------
> > >  1 file changed, 9 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c index
> > > e010ea4eb6f5..56d2c7567d97 100644
> > > --- a/drivers/mmc/host/sdhci.c
> > > +++ b/drivers/mmc/host/sdhci.c
> > > @@ -173,8 +173,8 @@ void sdhci_reset(struct sdhci_host *host, u8 mask)
> > >                         sdhci_runtime_pm_bus_off(host);
> > >         }
> > >
> > > -       /* Wait max 100 ms */
> > > -       timeout = 100;
> > > +       /* Wait max 10000 ms */
> > > +       timeout = 10000;
> > >
> > >         /* hw clears the bit when it's done */
> > >         while (sdhci_readb(host, SDHCI_SOFTWARE_RESET) & mask) { @@
> > > -185,7 +185,7 @@ void sdhci_reset(struct sdhci_host *host, u8 mask)
> > >                         return;
> > >                 }
> > >                 timeout--;
> > > -               mdelay(1);
> > > +               udelay(10);
> > >         }
> > >  }
> > >  EXPORT_SYMBOL_GPL(sdhci_reset);
> > 
> > This can significantly increase the timeout length. I think you should instead
> > use time_before() to see how many jiffies have passed since the start.
> > 
> > However, the real question is why the reset function gets called under a
> > spinlock in the first place. Can you try to rearrange the code so it doesn't
> > need the lock at all and you can just use msleep() instead?
> > 
> >       Arnd
> 
> Thank you for your quick reply.
> Could you please clarify what do you mean is called under a spinlock?
> Any is not used here?

You write that the function is called in a critical region protected
by the spinlock, so I was wondering if that is actually necessary.

Usually a device reset should be done in normal process context without
any spinlocks so you can call normal sleeping functions.

	Arnd

[toc] | [prev] | [next] | [standalone]

#1410036

> -----Original Message-----
> From: Arnd Bergmann [mailto:arnd@arndb.de]
> Sent: Tuesday, May 31, 2016 11:17 AM
> To: Baranowska, BeataX <beatax.baranowska@intel.com>
> Cc: Hunter, Adrian <adrian.hunter@intel.com>; Ulf Hansson
> <ulf.hansson@linaro.org>; linux-mmc@vger.kernel.org; linux-
> kernel@vger.kernel.org; Dong, Chuanxiao <chuanxiao.dong@intel.com>;
> Jarosz, SebastianX <sebastianx.jarosz@intel.com>
> Subject: Re: [PATCH] mmc: sdhci: use udelay instead of mdelay
> 
> On Tuesday, May 31, 2016 8:53:18 AM CEST Baranowska, BeataX wrote:
> > >
> > > On Monday, May 30, 2016 7:55:55 AM CEST Baranowska, BeataX wrote:
> > > > From: Chuanxiao Dong <chuanxiao.dong@intel.com>
> > > >
> > > > This patch will use udelay instead of mdelay when waiting for
> > > > SDHCI hardware to be stable. udelay can help to reduce the waiting
> > > > time when is in critical region which is protected by spinlock.
> > > >
> > > > With this patch, __sdhci_set_ios only take a few microseconds to
> > > > be done.
> > > >
> > > > Signed-off-by: Chuanxiao Dong <chuanxiao.dong@intel.com>
> > > > ---
> > > > drivers/mmc/host/sdhci.c | 18 +++++++++---------
> > > >  1 file changed, 9 insertions(+), 9 deletions(-)
> > > >
> > > > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> > > > index
> > > > e010ea4eb6f5..56d2c7567d97 100644
> > > > --- a/drivers/mmc/host/sdhci.c
> > > > +++ b/drivers/mmc/host/sdhci.c
> > > > @@ -173,8 +173,8 @@ void sdhci_reset(struct sdhci_host *host, u8
> mask)
> > > >                         sdhci_runtime_pm_bus_off(host);
> > > >         }
> > > >
> > > > -       /* Wait max 100 ms */
> > > > -       timeout = 100;
> > > > +       /* Wait max 10000 ms */
> > > > +       timeout = 10000;
> > > >
> > > >         /* hw clears the bit when it's done */
> > > >         while (sdhci_readb(host, SDHCI_SOFTWARE_RESET) & mask) {
> > > > @@
> > > > -185,7 +185,7 @@ void sdhci_reset(struct sdhci_host *host, u8 mask)
> > > >                         return;
> > > >                 }
> > > >                 timeout--;
> > > > -               mdelay(1);
> > > > +               udelay(10);
> > > >         }
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(sdhci_reset);
> > >
> > > This can significantly increase the timeout length. I think you
> > > should instead use time_before() to see how many jiffies have passed
> since the start.
> > >
> > > However, the real question is why the reset function gets called
> > > under a spinlock in the first place. Can you try to rearrange the
> > > code so it doesn't need the lock at all and you can just use msleep()
> instead?
> > >
> > >       Arnd
> >
> > Thank you for your quick reply.
> > Could you please clarify what do you mean is called under a spinlock?
> > Any is not used here?
> 
> You write that the function is called in a critical region protected by the
> spinlock, so I was wondering if that is actually necessary.
> 
> Usually a device reset should be done in normal process context without any
> spinlocks so you can call normal sleeping functions.
> 
> 	Arnd

Ok I understand now. Thank you.

Beata Baranowska

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web