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

[Wolfram Sang <w.sang@pengutronix.de>]

[-- 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 --]