[PATCH resend] sdhci: work around broken dma boundary behaviour

[PATCH resend] sdhci: work around broken dma boundary behaviour

[Mikko Vinni]

Some SD host controllers (noticed on an integrated JMicron SD reader
on an HP Pavilion dv5-1250eo laptop) don't update the dma address
register before signaling a dma interrupt due to a dma boundary.
Detect this and update the register to the next 512KB boundary,
at which the transfer presumably stopped.

As long as each transfer is at most 512KB in size (on this hardware
the max seems to be 65536 bytes), this fix is needed at most once
per transfer.

Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=28462

Signed-off-by: Mikko Vinni <mmvinni <at> yahoo.com>
---
Sent on 2011-02-21 21:23:32 GMT
(http://thread.gmane.org/gmane.linux.kernel.mmc/5568/focus=6145)

Hoping to be able to drop this patch eventually from my own
repo and have the hardware just work with mainline code.
Maybe first in -next if nobody sees any serious problems
straight away?

This patch should not break anything for anybody whose
hardware isn't already broken.

 drivers/mmc/host/sdhci.c |   24 +++++++++++++++++++++---
 1 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index a25db42..8651731 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1537,9 +1537,27 @@ static void sdhci_data_irq(struct sdhci_host *host, u32 intmask)
 		 * boundaries, but as we can't disable the feature
 		 * we need to at least restart the transfer.
 		 */
-		if (intmask & SDHCI_INT_DMA_END)
-			sdhci_writel(host, sdhci_readl(host, SDHCI_DMA_ADDRESS),
-				SDHCI_DMA_ADDRESS);
+		if (intmask & SDHCI_INT_DMA_END) {
+			u32 dmastart, dmanow;
+			dmastart = sg_dma_address(host->data->sg);
+			dmanow = sdhci_readl(host, SDHCI_DMA_ADDRESS);
+			if (dmanow == dmastart) {
+				/*
+				 * HW failed to increase the address.
+				 * Update to the next 512KB block boundary.
+				 */
+				dmanow = (dmanow & ~0x7ffff) + 0x80000;
+				if (dmanow > dmastart + host->data->blksz *
+							host->data->blocks) {
+					WARN_ON(1);
+					dmanow = dmastart;
+				}
+				DBG("%s: next DMA address forced "
+				    "from 0x%08x to 0x%08x\n",
+				    mmc_hostname(host->mmc), dmastart, dmanow);
+			}
+			sdhci_writel(host, dmanow, SDHCI_DMA_ADDRESS);
+		}
 
 		if (intmask & SDHCI_INT_DATA_END) {
 			if (host->cmd) {
-- 
1.7.4.1

Re: [PATCH resend] sdhci: work around broken dma boundary behaviour

[Chris Ball]

Hi Mikko,

On Mon, Mar 07 2011, Mikko Vinni wrote:
> Some SD host controllers (noticed on an integrated JMicron SD reader
> on an HP Pavilion dv5-1250eo laptop) don't update the dma address
> register before signaling a dma interrupt due to a dma boundary.
> Detect this and update the register to the next 512KB boundary,
> at which the transfer presumably stopped.
>
> As long as each transfer is at most 512KB in size (on this hardware
> the max seems to be 65536 bytes), this fix is needed at most once
> per transfer.
>
> Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=28462
>
> Signed-off-by: Mikko Vinni <mmvinni <at> yahoo.com>
> ---
> Sent on 2011-02-21 21:23:32 GMT
> (http://thread.gmane.org/gmane.linux.kernel.mmc/5568/focus=6145)
>
> Hoping to be able to drop this patch eventually from my own
> repo and have the hardware just work with mainline code.
> Maybe first in -next if nobody sees any serious problems
> straight away?
>
> This patch should not break anything for anybody whose
> hardware isn't already broken.
>
>  drivers/mmc/host/sdhci.c |   24 +++++++++++++++++++++---
>  1 files changed, 21 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index a25db42..8651731 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1537,9 +1537,27 @@ static void sdhci_data_irq(struct sdhci_host *host, u32 intmask)
>  		 * boundaries, but as we can't disable the feature
>  		 * we need to at least restart the transfer.
>  		 */
> -		if (intmask & SDHCI_INT_DMA_END)
> -			sdhci_writel(host, sdhci_readl(host, SDHCI_DMA_ADDRESS),
> -				SDHCI_DMA_ADDRESS);
> +		if (intmask & SDHCI_INT_DMA_END) {
> +			u32 dmastart, dmanow;
> +			dmastart = sg_dma_address(host->data->sg);
> +			dmanow = sdhci_readl(host, SDHCI_DMA_ADDRESS);
> +			if (dmanow == dmastart) {
> +				/*
> +				 * HW failed to increase the address.
> +				 * Update to the next 512KB block boundary.
> +				 */
> +				dmanow = (dmanow & ~0x7ffff) + 0x80000;
> +				if (dmanow > dmastart + host->data->blksz *
> +							host->data->blocks) {
> +					WARN_ON(1);
> +					dmanow = dmastart;
> +				}
> +				DBG("%s: next DMA address forced "
> +				    "from 0x%08x to 0x%08x\n",
> +				    mmc_hostname(host->mmc), dmastart, dmanow);
> +			}
> +			sdhci_writel(host, dmanow, SDHCI_DMA_ADDRESS);
> +		}
>  
>  		if (intmask & SDHCI_INT_DATA_END) {
>  			if (host->cmd) {

Thanks, I agree, pushed to -next for testing.  I'd still appreciate
a Reviewed-by: from someone on linux-mmc@, and would like to hear
if anyone thinks this needs to be a quirk rather than a generic fix.

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH resend] sdhci: work around broken dma boundary behaviour

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 531 bytes --]

> Thanks, I agree, pushed to -next for testing.  I'd still appreciate
> a Reviewed-by: from someone on linux-mmc@, and would like to hear
> if anyone thinks this needs to be a quirk rather than a generic fix.

Not surprisingly, I prefer the generic fix over a quirk ;) It seems to not cost
much detecting the flaw at runtime. I try to have a closer review tomorrow.

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH resend] sdhci: work around broken dma boundary behaviour

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 3068 bytes --]

Hi,

I finally found some time.

On Mon, Mar 07, 2011 at 10:40:57PM +0200, Mikko Vinni wrote:
> Some SD host controllers (noticed on an integrated JMicron SD reader
> on an HP Pavilion dv5-1250eo laptop) don't update the dma address
> register before signaling a dma interrupt due to a dma boundary.
> Detect this and update the register to the next 512KB boundary,
> at which the transfer presumably stopped.
> 
> As long as each transfer is at most 512KB in size (on this hardware
> the max seems to be 65536 bytes), this fix is needed at most once
> per transfer.

But we can't guarantee that. Transfer could be up to 65535 * 2K.

> Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=28462
> 
> Signed-off-by: Mikko Vinni <mmvinni <at> yahoo.com>

Proper EMail please.

> 
> ---
> Sent on 2011-02-21 21:23:32 GMT
> (http://thread.gmane.org/gmane.linux.kernel.mmc/5568/focus=6145)
> 
> Hoping to be able to drop this patch eventually from my own
> repo and have the hardware just work with mainline code.
> Maybe first in -next if nobody sees any serious problems
> straight away?
> 
> This patch should not break anything for anybody whose
> hardware isn't already broken.
> 
>  drivers/mmc/host/sdhci.c |   24 +++++++++++++++++++++---
>  1 files changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index a25db42..8651731 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1537,9 +1537,27 @@ static void sdhci_data_irq(struct sdhci_host *host, u32 intmask)
>  		 * boundaries, but as we can't disable the feature
>  		 * we need to at least restart the transfer.
>  		 */
> -		if (intmask & SDHCI_INT_DMA_END)
> -			sdhci_writel(host, sdhci_readl(host, SDHCI_DMA_ADDRESS),
> -				SDHCI_DMA_ADDRESS);
> +		if (intmask & SDHCI_INT_DMA_END) {
> +			u32 dmastart, dmanow;
> +			dmastart = sg_dma_address(host->data->sg);

This will only work for the first 512K, right?

> +			dmanow = sdhci_readl(host, SDHCI_DMA_ADDRESS);
> +			if (dmanow == dmastart) {
> +				/*
> +				 * HW failed to increase the address.
> +				 * Update to the next 512KB block boundary.
> +				 */
> +				dmanow = (dmanow & ~0x7ffff) + 0x80000;

Hmm, hardcoding these values is probably not a good idea. They should be
dependent on what is written to MAKE_BLKSIZE. Maybe a common define?

> +				if (dmanow > dmastart + host->data->blksz *
> +							host->data->blocks) {
> +					WARN_ON(1);
> +					dmanow = dmastart;
> +				}

Did this happen?

> +				DBG("%s: next DMA address forced "
> +				    "from 0x%08x to 0x%08x\n",
> +				    mmc_hostname(host->mmc), dmastart, dmanow);
> +			}
> +			sdhci_writel(host, dmanow, SDHCI_DMA_ADDRESS);
> +		}
>  
>  		if (intmask & SDHCI_INT_DATA_END) {
>  			if (host->cmd) {
> -- 
> 1.7.4.1

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH resend] sdhci: work around broken dma boundary behaviour

[Mikko Vinni]

Hi,



Wolfram Sang wrote:
> I finally found some time.

Greatly appreciated.

> > Some SD host controllers (noticed on an  integrated JMicron SD reader
> > on an HP Pavilion dv5-1250eo laptop) don't  update the dma address
> > register before signaling a dma interrupt due to  a dma boundary.
> > Detect this and update the register to the next 512KB  boundary,
> > at which the transfer presumably stopped.
> > 
> > As  long as each transfer is at most 512KB in size (on this hardware
> > the max  seems to be 65536 bytes), this fix is needed at most once
> > per  transfer.
> 
> But we can't guarantee that. Transfer could be up to 65535 *  2K.

In sdhci.c function sdhci_prepare_data there are these checks:

    /* Sanity checks */
    BUG_ON(data->blksz * data->blocks > 524288);
    BUG_ON(data->blksz > host->mmc->max_blk_size);
    BUG_ON(data->blocks > 65535);

I thought the first BUG_ON makes sure that the transfer doesn't go too big.
Then again, I might be missing something. Is 524288 not in bytes?

> 
> > Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=28462
> > 
> >  Signed-off-by: Mikko Vinni <mmvinni <at> yahoo.com>
> 
> Proper EMail  please.

Hm, @gmail.com? (cc added for further emails)


> > diff --git a/drivers/mmc/host/sdhci.c  b/drivers/mmc/host/sdhci.c
> > index a25db42..8651731 100644
> > ---  a/drivers/mmc/host/sdhci.c
> > +++ b/drivers/mmc/host/sdhci.c
> > @@  -1537,9 +1537,27 @@ static void sdhci_data_irq(struct sdhci_host *host, 
>u32  intmask)
> >           * boundaries, but  as we can't disable the feature
> >            * we need to at least restart the transfer.
> >            */
> > -         if (intmask & SDHCI_INT_DMA_END)
> >  -            sdhci_writel(host,  sdhci_readl(host, SDHCI_DMA_ADDRESS),
> > -                  SDHCI_DMA_ADDRESS);
> > +        if (intmask  & SDHCI_INT_DMA_END) {
> > +             u32 dmastart, dmanow;
> > +             dmastart =  sg_dma_address(host->data->sg);
> 
> This will only work for the first  512K, right?

True. If a transfer can cross more than one boundary, I suppose an
additional variable is needed to keep track of the current state.

> 
> > +             dmanow = sdhci_readl(host, SDHCI_DMA_ADDRESS);
> >  +            if (dmanow ==  dmastart) {
> > +                 /*
> > +                  * HW failed to increase the  address.
> > +                  * Update to the next 512KB block boundary.
> >  +                  */
> > +                 dmanow = (dmanow & ~0x7ffff) + 0x80000;
> 
> Hmm,  hardcoding these values is probably not a good idea. They should be
> dependent  on what is written to MAKE_BLKSIZE. Maybe a common define?

Sorry, implementing that goes beyond my comfort zone. I would be happy to
test patches, though.

> 
> >  +                if  (dmanow > dmastart + host->data->blksz *
> > +                              host->data->blocks) {
> >  +                     WARN_ON(1);
> > +                     dmanow =  dmastart;
> > +                 }
> 
> Did this happen?

No, but I though it brings some protection in case somebody *does*
change the boundary value without checking the code first (and happens
to be running on flawed hardware).

> 
> > +                 DBG("%s: next DMA  address forced "
> > +                     "from 0x%08x to  0x%08x\n",
> > +                     mmc_hostname(host->mmc), dmastart,  dmanow);
> > +             }
> > +             sdhci_writel(host, dmanow, SDHCI_DMA_ADDRESS);
> > +         }
> >  
> >           if (intmask & SDHCI_INT_DATA_END) {
> >               if (host->cmd)  {
> > -- 
> > 1.7.4.1
> 
> Regards,
> 
>    Wolfram
> 

Thanks

Mikko


      

Re: [PATCH resend] sdhci: work around broken dma boundary behaviour

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 3921 bytes --]


> > But we can't guarantee that. Transfer could be up to 65535 *  2K.
> 
> In sdhci.c function sdhci_prepare_data there are these checks:
> 
>     /* Sanity checks */
>     BUG_ON(data->blksz * data->blocks > 524288);
>     BUG_ON(data->blksz > host->mmc->max_blk_size);
>     BUG_ON(data->blocks > 65535);
> 
> I thought the first BUG_ON makes sure that the transfer doesn't go too big.
> Then again, I might be missing something. Is 524288 not in bytes?

Sorry, I simply missed that. Hmmm, another hard coded value :(

> > > Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=28462
> > > 
> > >  Signed-off-by: Mikko Vinni <mmvinni <at> yahoo.com>
> > 
> > Proper EMail  please.
> 
> Hm, @gmail.com? (cc added for further emails)

I was just referring to using "<at>" instead of "@". The provider
doesn't really matter :)

> > > diff --git a/drivers/mmc/host/sdhci.c  b/drivers/mmc/host/sdhci.c
> > > index a25db42..8651731 100644
> > > ---  a/drivers/mmc/host/sdhci.c
> > > +++ b/drivers/mmc/host/sdhci.c
> > > @@  -1537,9 +1537,27 @@ static void sdhci_data_irq(struct sdhci_host *host, 
> >u32  intmask)
> > >           * boundaries, but  as we can't disable the feature
> > >            * we need to at least restart the transfer.
> > >            */
> > > -         if (intmask & SDHCI_INT_DMA_END)
> > >  -            sdhci_writel(host,  sdhci_readl(host, SDHCI_DMA_ADDRESS),
> > > -                  SDHCI_DMA_ADDRESS);
> > > +        if (intmask  & SDHCI_INT_DMA_END) {
> > > +             u32 dmastart, dmanow;
> > > +             dmastart =  sg_dma_address(host->data->sg);
> > 
> > This will only work for the first  512K, right?
> 
> True. If a transfer can cross more than one boundary, I suppose an
> additional variable is needed to keep track of the current state.

Yeah, thought that, too. I also wondered if we then just could not
always write our own value to DMA_ADDRESS. This is redundant on working
hardware, but the the check is not much cheaper than just doing it.

Would that change also be beyond your comfort zone?

> > > +             dmanow = sdhci_readl(host, SDHCI_DMA_ADDRESS);
> > >  +            if (dmanow ==  dmastart) {
> > > +                 /*
> > > +                  * HW failed to increase the  address.
> > > +                  * Update to the next 512KB block boundary.
> > >  +                  */
> > > +                 dmanow = (dmanow & ~0x7ffff) + 0x80000;
> > 
> > Hmm,  hardcoding these values is probably not a good idea. They should be
> > dependent  on what is written to MAKE_BLKSIZE. Maybe a common define?
> 
> Sorry, implementing that goes beyond my comfort zone. I would be happy to
> test patches, though.

I was imagining something like:

#define SDHCI_DEFAULT_BOUNDARY_SIZE	(512 * 1024)

which could be used directly in your code and later like

SDHCI_MAKE_BLKSZ(ilog2(SDHCI_DEFAULT_BOUNDARY_SIZE) - 12, ...);

(Maybe the ilog2-thingie could be another macro)

> > >  +                if  (dmanow > dmastart + host->data->blksz *
> > > +                              host->data->blocks) {
> > >  +                     WARN_ON(1);
> > > +                     dmanow =  dmastart;
> > > +                 }
> > 
> > Did this happen?
> 
> No, but I though it brings some protection in case somebody *does*
> change the boundary value without checking the code first (and happens
> to be running on flawed hardware).

So, it could go if we make that dependent if I understand correctly.

Oh, and what I forgot to say last time:

Thanks a lot for your debugging efforts! I read the bugzilla entry and
your persistency for nailing the cause is greatly appreciated. Good
work.

All the best,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH resend] sdhci: work around broken dma boundary behaviour

[Mikko Vinni]

Wolfram Sang wrote:

> > > >  Signed-off-by: Mikko Vinni <mmvinni <at>  yahoo.com>
> > > 
> > > Proper EMail  please.
> > 
> > Hm, @gmail.com? (cc added for further emails)
> 
> I was just  referring to using "<at>" instead of "@". The provider
> doesn't really  matter :)

Ah, ok. Yahoo makes it practically impossible to send well-formed
patches from the web interface, but as long as it's not completely
rejected for casual email, I prefer to keep the number of spam
addresses to the minimum.

> > > > -          if (intmask & SDHCI_INT_DMA_END)
> > >  >  -            sdhci_writel(host,   sdhci_readl(host, 
SDHCI_DMA_ADDRESS),
> > > > -                   SDHCI_DMA_ADDRESS);
> > > >  +        if (intmask  & SDHCI_INT_DMA_END)  {
> > > > +             u32 dmastart,  dmanow;
> > > > +             dmastart  =  sg_dma_address(host->data->sg);
> > > 
> > > This  will only work for the first  512K, right?
> > 
> > True. If a  transfer can cross more than one boundary, I suppose an
> > additional  variable is needed to keep track of the current state.
> 
> Yeah, thought  that, too. I also wondered if we then just could not
> always write our own  value to DMA_ADDRESS. This is redundant on working
> hardware, but the the  check is not much cheaper than just doing it.
> 
> Would that change also be  beyond your comfort zone?

Almost. There is one statement in the spec ("At the end of transfer, the Host
Controller may issue or may not issue DMA Interrupt"), which makes
me wonder whether a host hontroller may issue a DMA Interrupt also at
the end of a transfer which doesn't finish at the boundary.

> 
> > > > +              dmanow = sdhci_readl(host, SDHCI_DMA_ADDRESS);
> > >  >  +            if (dmanow ==   dmastart) {
> > > > +                  /*
> > > > +                   * HW failed to increase the  address.
> > > >  +                  * Update to the  next 512KB block boundary.
> > > >  +                   */
> > > > +                  dmanow = (dmanow & ~0x7ffff) +  0x80000;
> > > 
> > > Hmm,  hardcoding these values is  probably not a good idea. They should be
> > > dependent  on what is  written to MAKE_BLKSIZE. Maybe a common define?
> > 
> > Sorry,  implementing that goes beyond my comfort zone. I would be happy to
> > test  patches, though.
> 
> I was imagining something like:
> 
> #define  SDHCI_DEFAULT_BOUNDARY_SIZE    (512 * 1024)
> 
> which could be  used directly in your code and later  like
> 
> SDHCI_MAKE_BLKSZ(ilog2(SDHCI_DEFAULT_BOUNDARY_SIZE) - 12,  ...);
> 
> (Maybe the ilog2-thingie could be another macro)

In sdhci.c or sdhci.c? I see SDHCI_MAKE_BLKSZ is used also in
drivers/mmc/host/sdhci-esdhc-imx.c and drivers/mmc/host/sdhci-of-esdhc.c.

I only compile tested this so far, so no proper patch yet, but what I would
write based on the comments is something like this:

--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -201,6 +201,9 @@
 #define SDHCI_MAX_DIV_SPEC_200    256
 #define SDHCI_MAX_DIV_SPEC_300    2046
 
+#define SDHCI_DEFAULT_BOUNDARY_SIZE    (512 * 1024)
+#define SDHCI_DEFAULT_BOUNDARY_ARG    (ilog2(SDHCI_DEFAULT_BOUNDARY_SIZE) - 12)
+
 struct sdhci_ops {
 #ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
     u32        (*read_l)(struct sdhci_host *host, int reg);

--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -808,7 +808,8 @@ static void sdhci_prepare_data(struct sdhci_host *host, 
struct mmc_data *data)
     sdhci_set_transfer_irqs(host);
 
     /* We do not handle DMA boundaries, so set it to max (512 KiB) */
-    sdhci_writew(host, SDHCI_MAKE_BLKSZ(7, data->blksz), SDHCI_BLOCK_SIZE);
+    sdhci_writew(host, SDHCI_MAKE_BLKSZ(SDHCI_DEFAULT_BOUNDARY_ARG,
+        data->blksz), SDHCI_BLOCK_SIZE);
     sdhci_writew(host, data->blocks, SDHCI_BLOCK_COUNT);
 }
 
@@ -1545,9 +1546,20 @@ static void sdhci_data_irq(struct sdhci_host *host, u32 
intmask)
          * boundaries, but as we can't disable the feature
          * we need to at least restart the transfer.
          */
-        if (intmask & SDHCI_INT_DMA_END)
-            sdhci_writel(host, sdhci_readl(host, SDHCI_DMA_ADDRESS),
-                SDHCI_DMA_ADDRESS);
+        if (intmask & SDHCI_INT_DMA_END) {
+            u32 dmastart, dmanow;
+            dmastart = sg_dma_address(host->data->sg);
+            dmanow = sdhci_readl(host, SDHCI_DMA_ADDRESS);
+            /*
+             * Force update to the next DMA block boundary.
+             */
+            dmanow = (dmastart &
+                ~(SDHCI_DEFAULT_BOUNDARY_SIZE - 1)) +
+                SDHCI_DEFAULT_BOUNDARY_SIZE;
+            DBG("%s: next DMA address after 0x%08x is 0x%08x\n",
+                mmc_hostname(host->mmc), dmastart, dmanow);
+            sdhci_writel(host, dmanow, SDHCI_DMA_ADDRESS);
+        }
 
         if (intmask & SDHCI_INT_DATA_END) {
             if (host->cmd) {


I will test this a bit later and send it as a proper patch, if it looks better
than the original one.

Mikko


> 
> > >  >  +                if   (dmanow > dmastart + host->data->blksz *
> > > > +                                host->data->blocks) {
> > > >  +                       WARN_ON(1);
> > > > +                      dmanow =  dmastart;
> > > > +                  }
> > > 
> > > Did this  happen?
> > 
> > No, but I though it brings some protection in case  somebody *does*
> > change the boundary value without checking the code  first (and happens
> > to be running on flawed hardware).
> 
> So, it  could go if we make that dependent if I understand correctly.
> 
> Oh, and  what I forgot to say last time:
> 
> Thanks a lot for your debugging efforts!  I read the bugzilla entry and
> your persistency for nailing the cause is  greatly appreciated. Good
> work.
> 
> All the best,
> 
>     Wolfram
> 
> -- 
> Pengutronix e.K.                            | Wolfram Sang                 |
> Industrial Linux Solutions                  | http://www.pengutronix.de/  |
> 


      

Re: [PATCH resend] sdhci: work around broken dma boundary behaviour

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 3556 bytes --]


> > I was just  referring to using "<at>" instead of "@". The provider
> > doesn't really  matter :)
> 
> Ah, ok. Yahoo makes it practically impossible to send well-formed
> patches from the web interface, but as long as it's not completely
> rejected for casual email, I prefer to keep the number of spam
> addresses to the minimum.

Okay, well, that is up to Chris.

> Almost. There is one statement in the spec ("At the end of transfer, the Host
> Controller may issue or may not issue DMA Interrupt"), which makes
> me wonder whether a host hontroller may issue a DMA Interrupt also at
> the end of a transfer which doesn't finish at the boundary.

Then we'll have a "useless" update. Won't hurt AFAICS, but might
surprise people examining the debug output.

> In sdhci.c or sdhci.c? I see SDHCI_MAKE_BLKSZ is used also in
> drivers/mmc/host/sdhci-esdhc-imx.c and drivers/mmc/host/sdhci-of-esdhc.c.

In those, I think it is okay to leave 0x07, because they always want to
clear all bits.

> I only compile tested this so far, so no proper patch yet, but what I would
> write based on the comments is something like this:

(Sidenote: Indentation is broken. Tabwidth is 8)

> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -201,6 +201,9 @@
>  #define SDHCI_MAX_DIV_SPEC_200    256
>  #define SDHCI_MAX_DIV_SPEC_300    2046
>  
> +#define SDHCI_DEFAULT_BOUNDARY_SIZE    (512 * 1024)
> +#define SDHCI_DEFAULT_BOUNDARY_ARG    (ilog2(SDHCI_DEFAULT_BOUNDARY_SIZE) - 12)
> +
>  struct sdhci_ops {
>  #ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
>      u32        (*read_l)(struct sdhci_host *host, int reg);
> 
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -808,7 +808,8 @@ static void sdhci_prepare_data(struct sdhci_host *host, 
> struct mmc_data *data)
>      sdhci_set_transfer_irqs(host);
>  
>      /* We do not handle DMA boundaries, so set it to max (512 KiB) */
> -    sdhci_writew(host, SDHCI_MAKE_BLKSZ(7, data->blksz), SDHCI_BLOCK_SIZE);
> +    sdhci_writew(host, SDHCI_MAKE_BLKSZ(SDHCI_DEFAULT_BOUNDARY_ARG,
> +        data->blksz), SDHCI_BLOCK_SIZE);
>      sdhci_writew(host, data->blocks, SDHCI_BLOCK_COUNT);
>  }
>  
> @@ -1545,9 +1546,20 @@ static void sdhci_data_irq(struct sdhci_host *host, u32 
> intmask)
>           * boundaries, but as we can't disable the feature
>           * we need to at least restart the transfer.
>           */
> -        if (intmask & SDHCI_INT_DMA_END)
> -            sdhci_writel(host, sdhci_readl(host, SDHCI_DMA_ADDRESS),
> -                SDHCI_DMA_ADDRESS);
> +        if (intmask & SDHCI_INT_DMA_END) {
> +            u32 dmastart, dmanow;
> +            dmastart = sg_dma_address(host->data->sg);

Consecutive transfers won't work (I know you know ;)).

> +            dmanow = sdhci_readl(host, SDHCI_DMA_ADDRESS);
> +            /*
> +             * Force update to the next DMA block boundary.
> +             */
> +            dmanow = (dmastart &
> +                ~(SDHCI_DEFAULT_BOUNDARY_SIZE - 1)) +
> +                SDHCI_DEFAULT_BOUNDARY_SIZE;
> +            DBG("%s: next DMA address after 0x%08x is 0x%08x\n",
> +                mmc_hostname(host->mmc), dmastart, dmanow);
> +            sdhci_writel(host, dmanow, SDHCI_DMA_ADDRESS);
> +        }

Other than that, looks like the right direction to me.

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH resend] sdhci: work around broken dma boundary behaviour

[Mikko Vinni]

Hi,


> Then we'll have a  "useless" update. Won't hurt AFAICS, but might
> surprise people examining the  debug output.

Ok.

> > I only compile  tested this so far, so no proper patch yet, but what I would
> > write based  on the comments is something like this:
> 
> (Sidenote: Indentation is broken.  Tabwidth is 8)

Well yes, that is what yahoo likes to break. I will send a proper patch via
git send-email if/when I have actually tested it and it seems to work.

> 
> > --- a/drivers/mmc/host/sdhci.h
> > +++  b/drivers/mmc/host/sdhci.h
> > @@ -1545,9 +1546,20 @@ static void  sdhci_data_irq(struct sdhci_host *host, 
>u32 
>
> > intmask)
> >            * boundaries, but as we can't disable the  feature
> >           * we need to at least restart  the transfer.
> >           */
> > -         if (intmask & SDHCI_INT_DMA_END)
> > -             sdhci_writel(host, sdhci_readl(host,  SDHCI_DMA_ADDRESS),
> > -                 SDHCI_DMA_ADDRESS);
> > +        if (intmask  & SDHCI_INT_DMA_END) {
> > +             u32 dmastart, dmanow;
> > +             dmastart = sg_dma_address(host->data->sg);
> 
> Consecutive transfers  won't work (I know you know ;)).

I assume you mean a single transfer that exceeds whatever is
defined in SDHCI_DEFAULT_BOUNDARY_SIZE. As long as
it is kept at 512K things should be fine, but adding the auxiliary
variable to facilitate smaller values would make the patch
more invasive. Being an mmc non-hacker, I would rather leave
that kind of invasive patches for others :)


> 
> > +             dmanow = sdhci_readl(host, SDHCI_DMA_ADDRESS);
> > +             /*
> > +              * Force update to the next DMA block boundary.
> > +              */
> > +             dmanow = (dmastart &
> > +                 ~(SDHCI_DEFAULT_BOUNDARY_SIZE - 1)) +
> > +                  SDHCI_DEFAULT_BOUNDARY_SIZE;
> > +             DBG("%s: next DMA address after 0x%08x is 0x%08x\n",
> > +                 mmc_hostname(host->mmc), dmastart,  dmanow);
> > +            sdhci_writel(host,  dmanow, SDHCI_DMA_ADDRESS);
> > +        }
> 
> Other  than that, looks like the right direction to me.

Good to hear.

Mikko


      

Re: [PATCH resend] sdhci: work around broken dma boundary behaviour

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1320 bytes --]

> > > intmask)
> > >            * boundaries, but as we can't disable the  feature
> > >           * we need to at least restart  the transfer.
> > >           */
> > > -         if (intmask & SDHCI_INT_DMA_END)
> > > -             sdhci_writel(host, sdhci_readl(host,  SDHCI_DMA_ADDRESS),
> > > -                 SDHCI_DMA_ADDRESS);
> > > +        if (intmask  & SDHCI_INT_DMA_END) {
> > > +             u32 dmastart, dmanow;
> > > +             dmastart = sg_dma_address(host->data->sg);
> > 
> > Consecutive transfers  won't work (I know you know ;)).
> 
> I assume you mean a single transfer that exceeds whatever is
> defined in SDHCI_DEFAULT_BOUNDARY_SIZE. As long as
> it is kept at 512K things should be fine, but adding the auxiliary
> variable to facilitate smaller values would make the patch
> more invasive. Being an mmc non-hacker, I would rather leave
> that kind of invasive patches for others :)

If you don't want to invest the effort, this is your decision, which is
fine. Other than that, I'd say you are not missing the required skills
for that change. (/me is just helping out here as well)

Cheers,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[RFC] mmc: sdhci: work around broken dma boundary behavior

[Mikko Vinni]

Some SD host controllers (noticed on an integrated JMicron
SD reader on an HP Pavilion dv5-1250eo laptop) don't update
the dma address register before signaling a dma interrupt due
to a dma boundary. Update the register manually to the next
boundary (by default 512KiB), at which the transfer stopped.

As long as each transfer is at most 512KiB in size (guaranteed
by a BUG_ON in sdhci_prepare_data()) and the boundary is kept
at the default value, this fix is needed at most once per
transfer. Smaller boundaries are taken care of by counting
the transferred bytes.

Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=28462

Signed-off-by: Mikko Vinni <mmvinni@yahoo.com>
---
 
This is alternative to
http://article.gmane.org/gmane.linux.kernel.mmc/6497

The main concern about the original patch was that it won't
work for if the boundary is made smaller and a large transfer
would cross the boundary twice. With this version this is taken
care of by re-using the bytes_xfered field from struct mmc_data.

Grepping through the code revealed no other uses of bytes_xfered
in host/sdhci.c except after the transfer is complete.

This patch has been tested with at least the boundary sizes
4, 8, 32, and 512KiB. The mmc-test tests all passed when tested
with 32KiB. This is on the "broken" controller. No other card readers
were tested so far.

The added debug output looks like this:

sdhci [sdhci_irq()]: *** mmc0 got interrupt: 0x00000008
sdhci [sdhci_data_irq()]: mmc0: DMA base 0x00100000, transferred 0x008000 bytes, next 0x00108000
sdhci [sdhci_irq()]: *** mmc0 got interrupt: 0x00000008
sdhci [sdhci_data_irq()]: mmc0: DMA base 0x00100000, transferred 0x010000 bytes, next 0x00110000
sdhci [sdhci_irq()]: *** mmc0 got interrupt: 0x00000008
sdhci [sdhci_data_irq()]: mmc0: DMA base 0x00100000, transferred 0x018000 bytes, next 0x00118000
sdhci [sdhci_irq()]: *** mmc0 got interrupt: 0x00000008
sdhci [sdhci_data_irq()]: mmc0: DMA base 0x00100000, transferred 0x020000 bytes, next 0x00120000

In my opinion this is more risky than the original patch
because this will affect the behavior on all controllers that
use sdhci, and not just on those ones that don't update SDHCI_DMA_ADDRESS
themselves. Comments welcome!

 drivers/mmc/host/sdhci.c |   30 +++++++++++++++++++++++++-----
 drivers/mmc/host/sdhci.h |    6 ++++++
 2 files changed, 31 insertions(+), 5 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 9e15f41..0319903 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -669,6 +669,7 @@ static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_data *data)
 
 	host->data = data;
 	host->data_early = 0;
+	host->data->bytes_xfered = 0;
 
 	count = sdhci_calc_timeout(host, data);
 	sdhci_writeb(host, count, SDHCI_TIMEOUT_CONTROL);
@@ -807,8 +808,9 @@ static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_data *data)
 
 	sdhci_set_transfer_irqs(host);
 
-	/* We do not handle DMA boundaries, so set it to max (512 KiB) */
-	sdhci_writew(host, SDHCI_MAKE_BLKSZ(7, data->blksz), SDHCI_BLOCK_SIZE);
+	/* Set the DMA boundary value and block size */
+	sdhci_writew(host, SDHCI_MAKE_BLKSZ(SDHCI_DEFAULT_BOUNDARY_ARG,
+		data->blksz), SDHCI_BLOCK_SIZE);
 	sdhci_writew(host, data->blocks, SDHCI_BLOCK_COUNT);
 }
 
@@ -1544,10 +1546,28 @@ static void sdhci_data_irq(struct sdhci_host *host, u32 intmask)
 		 * We currently don't do anything fancy with DMA
 		 * boundaries, but as we can't disable the feature
 		 * we need to at least restart the transfer.
+		 *
+		 * According to the spec sdhci_readl(host, SDHCI_DMA_ADDRESS)
+		 * should return a valid address to continue from, but as
+		 * some controllers are faulty, don't trust them.
 		 */
-		if (intmask & SDHCI_INT_DMA_END)
-			sdhci_writel(host, sdhci_readl(host, SDHCI_DMA_ADDRESS),
-				SDHCI_DMA_ADDRESS);
+		if (intmask & SDHCI_INT_DMA_END) {
+			u32 dmastart, dmanow;
+			dmastart = sg_dma_address(host->data->sg);
+			dmanow = dmastart + host->data->bytes_xfered;
+			/*
+			 * Force update to the next DMA block boundary.
+			 */
+			dmanow = (dmanow &
+				~(SDHCI_DEFAULT_BOUNDARY_SIZE - 1)) +
+				SDHCI_DEFAULT_BOUNDARY_SIZE;
+			host->data->bytes_xfered = dmanow - dmastart;
+			DBG("%s: DMA base 0x%08x, transferred 0x%06x bytes,"
+				" next 0x%08x\n",
+				mmc_hostname(host->mmc), dmastart,
+				host->data->bytes_xfered, dmanow);
+			sdhci_writel(host, dmanow, SDHCI_DMA_ADDRESS);
+		}
 
 		if (intmask & SDHCI_INT_DATA_END) {
 			if (host->cmd) {
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 25e8bde..85750a9 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -202,6 +202,12 @@
 #define SDHCI_MAX_DIV_SPEC_200	256
 #define SDHCI_MAX_DIV_SPEC_300	2046
 
+/*
+ * Host SDMA buffer boundary. Valid values from 4K to 512K in powers of 2.
+ */
+#define SDHCI_DEFAULT_BOUNDARY_SIZE  (512 * 1024)
+#define SDHCI_DEFAULT_BOUNDARY_ARG   (ilog2(SDHCI_DEFAULT_BOUNDARY_SIZE) - 12)
+
 struct sdhci_ops {
 #ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
 	u32		(*read_l)(struct sdhci_host *host, int reg);
-- 
1.7.4.1

Re: [RFC] mmc: sdhci: work around broken dma boundary behavior

[Chris Ball]

Hi Wolfram,

On Tue, Mar 29 2011, Mikko Vinni wrote:
> In my opinion this is more risky than the original patch because this
> will affect the behavior on all controllers that use sdhci, and not
> just on those ones that don't update SDHCI_DMA_ADDRESS themselves.
> Comments welcome!

Any thoughts on which of the two approaches you prefer?

Thanks,

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [RFC] mmc: sdhci: work around broken dma boundary behavior

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 572 bytes --]

> > In my opinion this is more risky than the original patch because this
> > will affect the behavior on all controllers that use sdhci, and not
> > just on those ones that don't update SDHCI_DMA_ADDRESS themselves.
> > Comments welcome!
> 
> Any thoughts on which of the two approaches you prefer?

Thanks for the ping. I prefer the latter approach but it should be given enough
time testing in -next.

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [RFC] mmc: sdhci: work around broken dma boundary behavior

[Chris Ball]

Hi Mikko,

On Tue, Mar 29 2011, Mikko Vinni wrote:
> Some SD host controllers (noticed on an integrated JMicron
> SD reader on an HP Pavilion dv5-1250eo laptop) don't update
> the dma address register before signaling a dma interrupt due
> to a dma boundary. Update the register manually to the next
> boundary (by default 512KiB), at which the transfer stopped.
>
> As long as each transfer is at most 512KiB in size (guaranteed
> by a BUG_ON in sdhci_prepare_data()) and the boundary is kept
> at the default value, this fix is needed at most once per
> transfer. Smaller boundaries are taken care of by counting
> the transferred bytes.
>
> Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=28462
>
> Signed-off-by: Mikko Vinni <mmvinni@yahoo.com>

I know you posted this as an RFC, but I've pushed it to mmc-next for
testing now, and will plan on merging it for .40 if everything goes well.
Thanks very much!

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [RFC] mmc: sdhci: work around broken dma boundary behavior

[Mikko Vinni]

>From Chris Ball:

> > Some SD host  controllers (noticed on an integrated JMicron
> > SD reader on an HP  Pavilion dv5-1250eo laptop) don't update
> > the dma address register before  signaling a dma interrupt due
> > to a dma boundary. Update the register  manually to the next
> > boundary (by default 512KiB), at which the transfer  stopped.
> >
> > As long as each transfer is at most 512KiB in size  (guaranteed
> > by a BUG_ON in sdhci_prepare_data()) and the boundary is  kept
> > at the default value, this fix is needed at most once per
> >  transfer. Smaller boundaries are taken care of by counting
> > the  transferred bytes.
> >
> > Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=28462
> >
> >  Signed-off-by: Mikko Vinni <mmvinni@yahoo.com>
> 
> I know you  posted this as an RFC, but I've pushed it to mmc-next for
> testing now, and  will plan on merging it for .40 if everything goes well.

Ok, thanks.

Mikko


> Thanks very  much!
> 
> - Chris.
>