[PATCH] mmc: msm: fix dma usage not to use internal APIs

[PATCH] mmc: msm: fix dma usage not to use internal APIs

[Daniel Walker]

Remove parts of this driver which use internal API calls. This
replaces the calls as suggested by Russell King.

Cc: Russell King - ARM Linux <linux@arm.linux.org.uk>
Signed-off-by: Daniel Walker <dwalker@codeaurora.org>
---
 drivers/mmc/host/msm_sdcc.c |   49 +++++++++++++++++++-----------------------
 1 files changed, 22 insertions(+), 27 deletions(-)

diff --git a/drivers/mmc/host/msm_sdcc.c b/drivers/mmc/host/msm_sdcc.c
index 5decfd0..733d233 100644
--- a/drivers/mmc/host/msm_sdcc.c
+++ b/drivers/mmc/host/msm_sdcc.c
@@ -383,14 +383,30 @@ static int msmsdcc_config_dma(struct msmsdcc_host *host, struct mmc_data *data)
 	host->curr.user_pages = 0;
 
 	box = &nc->cmd[0];
-	for (i = 0; i < host->dma.num_ents; i++) {
-		box->cmd = CMD_MODE_BOX;
 
-	/* Initialize sg dma address */
-	sg->dma_address = page_to_dma(mmc_dev(host->mmc), sg_page(sg))
-				+ sg->offset;
+	/* location of command block must be 64 bit aligned */
+	BUG_ON(host->dma.cmd_busaddr & 0x07);
+
+	nc->cmdptr = (host->dma.cmd_busaddr >> 3) | CMD_PTR_LP;
+	host->dma.hdr.cmdptr = DMOV_CMD_PTR_LIST |
+			       DMOV_CMD_ADDR(host->dma.cmdptr_busaddr);
+	host->dma.hdr.complete_func = msmsdcc_dma_complete_func;
+
+	n = dma_map_sg(mmc_dev(host->mmc), host->dma.sg,
+			host->dma.num_ents, host->dma.dir);
+	if (n == 0) {
+		printk(KERN_ERR "%s: Unable to map in all sg elements\n",
+			mmc_hostname(host->mmc));
+		host->dma.sg = NULL;
+		host->dma.num_ents = 0;
+		return -ENOMEM;
+	}
+
+	for_each_sg(host->dma.sg, sg, n, i) {
+
+		box->cmd = CMD_MODE_BOX;
 
-	if (i == (host->dma.num_ents - 1))
+		if (i == n - 1)
 			box->cmd |= CMD_LC;
 		rows = (sg_dma_len(sg) % MCI_FIFOSIZE) ?
 			(sg_dma_len(sg) / MCI_FIFOSIZE) + 1 :
@@ -418,27 +434,6 @@ static int msmsdcc_config_dma(struct msmsdcc_host *host, struct mmc_data *data)
 			box->cmd |= CMD_DST_CRCI(crci);
 		}
 		box++;
-		sg++;
-	}
-
-	/* location of command block must be 64 bit aligned */
-	BUG_ON(host->dma.cmd_busaddr & 0x07);
-
-	nc->cmdptr = (host->dma.cmd_busaddr >> 3) | CMD_PTR_LP;
-	host->dma.hdr.cmdptr = DMOV_CMD_PTR_LIST |
-			       DMOV_CMD_ADDR(host->dma.cmdptr_busaddr);
-	host->dma.hdr.complete_func = msmsdcc_dma_complete_func;
-
-	n = dma_map_sg(mmc_dev(host->mmc), host->dma.sg,
-			host->dma.num_ents, host->dma.dir);
-/* dsb inside dma_map_sg will write nc out to mem as well */
-
-	if (n != host->dma.num_ents) {
-		printk(KERN_ERR "%s: Unable to map in all sg elements\n",
-			mmc_hostname(host->mmc));
-		host->dma.sg = NULL;
-		host->dma.num_ents = 0;
-		return -ENOMEM;
 	}
 
 	return 0;
-- 
1.7.0.4

-- 
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH] mmc: msm: fix dma usage not to use internal APIs

[Daniel Walker]

Remove parts of this driver which use internal API calls. This
replaces the calls as suggested by Russell King.

Cc: Russell King - ARM Linux <linux@arm.linux.org.uk>
Signed-off-by: Daniel Walker <dwalker@codeaurora.org>
---
 drivers/mmc/host/msm_sdcc.c |   49 +++++++++++++++++++-----------------------
 1 files changed, 22 insertions(+), 27 deletions(-)

diff --git a/drivers/mmc/host/msm_sdcc.c b/drivers/mmc/host/msm_sdcc.c
index 5decfd0..733d233 100644
--- a/drivers/mmc/host/msm_sdcc.c
+++ b/drivers/mmc/host/msm_sdcc.c
@@ -383,14 +383,30 @@ static int msmsdcc_config_dma(struct msmsdcc_host *host, struct mmc_data *data)
 	host->curr.user_pages = 0;
 
 	box = &nc->cmd[0];
-	for (i = 0; i < host->dma.num_ents; i++) {
-		box->cmd = CMD_MODE_BOX;
 
-	/* Initialize sg dma address */
-	sg->dma_address = page_to_dma(mmc_dev(host->mmc), sg_page(sg))
-				+ sg->offset;
+	/* location of command block must be 64 bit aligned */
+	BUG_ON(host->dma.cmd_busaddr & 0x07);
+
+	nc->cmdptr = (host->dma.cmd_busaddr >> 3) | CMD_PTR_LP;
+	host->dma.hdr.cmdptr = DMOV_CMD_PTR_LIST |
+			       DMOV_CMD_ADDR(host->dma.cmdptr_busaddr);
+	host->dma.hdr.complete_func = msmsdcc_dma_complete_func;
+
+	n = dma_map_sg(mmc_dev(host->mmc), host->dma.sg,
+			host->dma.num_ents, host->dma.dir);
+	if (n == 0) {
+		printk(KERN_ERR "%s: Unable to map in all sg elements\n",
+			mmc_hostname(host->mmc));
+		host->dma.sg = NULL;
+		host->dma.num_ents = 0;
+		return -ENOMEM;
+	}
+
+	for_each_sg(host->dma.sg, sg, n, i) {
+
+		box->cmd = CMD_MODE_BOX;
 
-	if (i == (host->dma.num_ents - 1))
+		if (i == n - 1)
 			box->cmd |= CMD_LC;
 		rows = (sg_dma_len(sg) % MCI_FIFOSIZE) ?
 			(sg_dma_len(sg) / MCI_FIFOSIZE) + 1 :
@@ -418,27 +434,6 @@ static int msmsdcc_config_dma(struct msmsdcc_host *host, struct mmc_data *data)
 			box->cmd |= CMD_DST_CRCI(crci);
 		}
 		box++;
-		sg++;
-	}
-
-	/* location of command block must be 64 bit aligned */
-	BUG_ON(host->dma.cmd_busaddr & 0x07);
-
-	nc->cmdptr = (host->dma.cmd_busaddr >> 3) | CMD_PTR_LP;
-	host->dma.hdr.cmdptr = DMOV_CMD_PTR_LIST |
-			       DMOV_CMD_ADDR(host->dma.cmdptr_busaddr);
-	host->dma.hdr.complete_func = msmsdcc_dma_complete_func;
-
-	n = dma_map_sg(mmc_dev(host->mmc), host->dma.sg,
-			host->dma.num_ents, host->dma.dir);
-/* dsb inside dma_map_sg will write nc out to mem as well */
-
-	if (n != host->dma.num_ents) {
-		printk(KERN_ERR "%s: Unable to map in all sg elements\n",
-			mmc_hostname(host->mmc));
-		host->dma.sg = NULL;
-		host->dma.num_ents = 0;
-		return -ENOMEM;
 	}
 
 	return 0;
-- 
1.7.0.4

-- 
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH] mmc: msm: fix dma usage not to use internal APIs

[Russell King - ARM Linux]

On Tue, Jan 18, 2011 at 03:03:25PM -0800, Daniel Walker wrote:
> Remove parts of this driver which use internal API calls. This
> replaces the calls as suggested by Russell King.
> 
> Cc: Russell King - ARM Linux <linux@arm.linux.org.uk>
> Signed-off-by: Daniel Walker <dwalker@codeaurora.org>

Acked-by: Russell King <rmk+kernel@arm.linux.org.uk>

> ---
>  drivers/mmc/host/msm_sdcc.c |   49 +++++++++++++++++++-----------------------
>  1 files changed, 22 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/mmc/host/msm_sdcc.c b/drivers/mmc/host/msm_sdcc.c
> index 5decfd0..733d233 100644
> --- a/drivers/mmc/host/msm_sdcc.c
> +++ b/drivers/mmc/host/msm_sdcc.c
> @@ -383,14 +383,30 @@ static int msmsdcc_config_dma(struct msmsdcc_host *host, struct mmc_data *data)
>  	host->curr.user_pages = 0;
>  
>  	box = &nc->cmd[0];
> -	for (i = 0; i < host->dma.num_ents; i++) {
> -		box->cmd = CMD_MODE_BOX;
>  
> -	/* Initialize sg dma address */
> -	sg->dma_address = page_to_dma(mmc_dev(host->mmc), sg_page(sg))
> -				+ sg->offset;
> +	/* location of command block must be 64 bit aligned */
> +	BUG_ON(host->dma.cmd_busaddr & 0x07);
> +
> +	nc->cmdptr = (host->dma.cmd_busaddr >> 3) | CMD_PTR_LP;
> +	host->dma.hdr.cmdptr = DMOV_CMD_PTR_LIST |
> +			       DMOV_CMD_ADDR(host->dma.cmdptr_busaddr);
> +	host->dma.hdr.complete_func = msmsdcc_dma_complete_func;
> +
> +	n = dma_map_sg(mmc_dev(host->mmc), host->dma.sg,
> +			host->dma.num_ents, host->dma.dir);
> +	if (n == 0) {
> +		printk(KERN_ERR "%s: Unable to map in all sg elements\n",
> +			mmc_hostname(host->mmc));
> +		host->dma.sg = NULL;
> +		host->dma.num_ents = 0;
> +		return -ENOMEM;
> +	}
> +
> +	for_each_sg(host->dma.sg, sg, n, i) {
> +
> +		box->cmd = CMD_MODE_BOX;
>  
> -	if (i == (host->dma.num_ents - 1))
> +		if (i == n - 1)
>  			box->cmd |= CMD_LC;
>  		rows = (sg_dma_len(sg) % MCI_FIFOSIZE) ?
>  			(sg_dma_len(sg) / MCI_FIFOSIZE) + 1 :
> @@ -418,27 +434,6 @@ static int msmsdcc_config_dma(struct msmsdcc_host *host, struct mmc_data *data)
>  			box->cmd |= CMD_DST_CRCI(crci);
>  		}
>  		box++;
> -		sg++;
> -	}
> -
> -	/* location of command block must be 64 bit aligned */
> -	BUG_ON(host->dma.cmd_busaddr & 0x07);
> -
> -	nc->cmdptr = (host->dma.cmd_busaddr >> 3) | CMD_PTR_LP;
> -	host->dma.hdr.cmdptr = DMOV_CMD_PTR_LIST |
> -			       DMOV_CMD_ADDR(host->dma.cmdptr_busaddr);
> -	host->dma.hdr.complete_func = msmsdcc_dma_complete_func;
> -
> -	n = dma_map_sg(mmc_dev(host->mmc), host->dma.sg,
> -			host->dma.num_ents, host->dma.dir);
> -/* dsb inside dma_map_sg will write nc out to mem as well */
> -
> -	if (n != host->dma.num_ents) {
> -		printk(KERN_ERR "%s: Unable to map in all sg elements\n",
> -			mmc_hostname(host->mmc));
> -		host->dma.sg = NULL;
> -		host->dma.num_ents = 0;
> -		return -ENOMEM;
>  	}
>  
>  	return 0;
> -- 
> 1.7.0.4
> 
> -- 
> Sent by a consultant of the Qualcomm Innovation Center, Inc.
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH] mmc: msm: fix dma usage not to use internal APIs

[Russell King - ARM Linux]

On Tue, Jan 18, 2011 at 03:03:25PM -0800, Daniel Walker wrote:
> Remove parts of this driver which use internal API calls. This
> replaces the calls as suggested by Russell King.
> 
> Cc: Russell King - ARM Linux <linux@arm.linux.org.uk>
> Signed-off-by: Daniel Walker <dwalker@codeaurora.org>

Acked-by: Russell King <rmk+kernel@arm.linux.org.uk>

> ---
>  drivers/mmc/host/msm_sdcc.c |   49 +++++++++++++++++++-----------------------
>  1 files changed, 22 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/mmc/host/msm_sdcc.c b/drivers/mmc/host/msm_sdcc.c
> index 5decfd0..733d233 100644
> --- a/drivers/mmc/host/msm_sdcc.c
> +++ b/drivers/mmc/host/msm_sdcc.c
> @@ -383,14 +383,30 @@ static int msmsdcc_config_dma(struct msmsdcc_host *host, struct mmc_data *data)
>  	host->curr.user_pages = 0;
>  
>  	box = &nc->cmd[0];
> -	for (i = 0; i < host->dma.num_ents; i++) {
> -		box->cmd = CMD_MODE_BOX;
>  
> -	/* Initialize sg dma address */
> -	sg->dma_address = page_to_dma(mmc_dev(host->mmc), sg_page(sg))
> -				+ sg->offset;
> +	/* location of command block must be 64 bit aligned */
> +	BUG_ON(host->dma.cmd_busaddr & 0x07);
> +
> +	nc->cmdptr = (host->dma.cmd_busaddr >> 3) | CMD_PTR_LP;
> +	host->dma.hdr.cmdptr = DMOV_CMD_PTR_LIST |
> +			       DMOV_CMD_ADDR(host->dma.cmdptr_busaddr);
> +	host->dma.hdr.complete_func = msmsdcc_dma_complete_func;
> +
> +	n = dma_map_sg(mmc_dev(host->mmc), host->dma.sg,
> +			host->dma.num_ents, host->dma.dir);
> +	if (n == 0) {
> +		printk(KERN_ERR "%s: Unable to map in all sg elements\n",
> +			mmc_hostname(host->mmc));
> +		host->dma.sg = NULL;
> +		host->dma.num_ents = 0;
> +		return -ENOMEM;
> +	}
> +
> +	for_each_sg(host->dma.sg, sg, n, i) {
> +
> +		box->cmd = CMD_MODE_BOX;
>  
> -	if (i == (host->dma.num_ents - 1))
> +		if (i == n - 1)
>  			box->cmd |= CMD_LC;
>  		rows = (sg_dma_len(sg) % MCI_FIFOSIZE) ?
>  			(sg_dma_len(sg) / MCI_FIFOSIZE) + 1 :
> @@ -418,27 +434,6 @@ static int msmsdcc_config_dma(struct msmsdcc_host *host, struct mmc_data *data)
>  			box->cmd |= CMD_DST_CRCI(crci);
>  		}
>  		box++;
> -		sg++;
> -	}
> -
> -	/* location of command block must be 64 bit aligned */
> -	BUG_ON(host->dma.cmd_busaddr & 0x07);
> -
> -	nc->cmdptr = (host->dma.cmd_busaddr >> 3) | CMD_PTR_LP;
> -	host->dma.hdr.cmdptr = DMOV_CMD_PTR_LIST |
> -			       DMOV_CMD_ADDR(host->dma.cmdptr_busaddr);
> -	host->dma.hdr.complete_func = msmsdcc_dma_complete_func;
> -
> -	n = dma_map_sg(mmc_dev(host->mmc), host->dma.sg,
> -			host->dma.num_ents, host->dma.dir);
> -/* dsb inside dma_map_sg will write nc out to mem as well */
> -
> -	if (n != host->dma.num_ents) {
> -		printk(KERN_ERR "%s: Unable to map in all sg elements\n",
> -			mmc_hostname(host->mmc));
> -		host->dma.sg = NULL;
> -		host->dma.num_ents = 0;
> -		return -ENOMEM;
>  	}
>  
>  	return 0;
> -- 
> 1.7.0.4
> 
> -- 
> Sent by a consultant of the Qualcomm Innovation Center, Inc.
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH] mmc: msm: fix dma usage not to use internal APIs

[Daniel Walker]

On Tue, 2011-01-18 at 15:03 -0800, Daniel Walker wrote:
> Remove parts of this driver which use internal API calls. This
> replaces the calls as suggested by Russell King.
> 
> Cc: Russell King - ARM Linux <linux@arm.linux.org.uk>
> Signed-off-by: Daniel Walker <dwalker@codeaurora.org>
> ---
>  drivers/mmc/host/msm_sdcc.c |   49 +++++++++++++++++++-----------------------
>  1 files changed, 22 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/mmc/host/msm_sdcc.c b/drivers/mmc/host/msm_sdcc.c
> index 5decfd0..733d233 100644
> --- a/drivers/mmc/host/msm_sdcc.c
> +++ b/drivers/mmc/host/msm_sdcc.c
> @@ -383,14 +383,30 @@ static int msmsdcc_config_dma(struct msmsdcc_host *host, struct mmc_data *data)
>  	host->curr.user_pages = 0;
>  
>  	box = &nc->cmd[0];
> -	for (i = 0; i < host->dma.num_ents; i++) {
> -		box->cmd = CMD_MODE_BOX;
>  
> -	/* Initialize sg dma address */
> -	sg->dma_address = page_to_dma(mmc_dev(host->mmc), sg_page(sg))
> -				+ sg->offset;

It would seem there was a reason for this change.. The person that added
this was Brent Degraaf (who is CC'd) ..

Quoting him below speaking about why dma_map_sg() isn't just used,

"Previous version of msm_sdcc.c had a cache coherency problem for
precisely this reason. The dma_map_sg is what cleans the caches for
these commands and so must be done AFTER the commands are populated. If
this entry is left until the map function is called, the
box->dst_row_addr will not be filled properly for reads."

Daniel


-- 
Sent by an consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum.

[PATCH] mmc: msm: fix dma usage not to use internal APIs

[Daniel Walker]

On Tue, 2011-01-18 at 15:03 -0800, Daniel Walker wrote:
> Remove parts of this driver which use internal API calls. This
> replaces the calls as suggested by Russell King.
> 
> Cc: Russell King - ARM Linux <linux@arm.linux.org.uk>
> Signed-off-by: Daniel Walker <dwalker@codeaurora.org>
> ---
>  drivers/mmc/host/msm_sdcc.c |   49 +++++++++++++++++++-----------------------
>  1 files changed, 22 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/mmc/host/msm_sdcc.c b/drivers/mmc/host/msm_sdcc.c
> index 5decfd0..733d233 100644
> --- a/drivers/mmc/host/msm_sdcc.c
> +++ b/drivers/mmc/host/msm_sdcc.c
> @@ -383,14 +383,30 @@ static int msmsdcc_config_dma(struct msmsdcc_host *host, struct mmc_data *data)
>  	host->curr.user_pages = 0;
>  
>  	box = &nc->cmd[0];
> -	for (i = 0; i < host->dma.num_ents; i++) {
> -		box->cmd = CMD_MODE_BOX;
>  
> -	/* Initialize sg dma address */
> -	sg->dma_address = page_to_dma(mmc_dev(host->mmc), sg_page(sg))
> -				+ sg->offset;

It would seem there was a reason for this change.. The person that added
this was Brent Degraaf (who is CC'd) ..

Quoting him below speaking about why dma_map_sg() isn't just used,

"Previous version of msm_sdcc.c had a cache coherency problem for
precisely this reason. The dma_map_sg is what cleans the caches for
these commands and so must be done AFTER the commands are populated. If
this entry is left until the map function is called, the
box->dst_row_addr will not be filled properly for reads."

Daniel


-- 
Sent by an consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum.

Re: [PATCH] mmc: msm: fix dma usage not to use internal APIs

[Russell King - ARM Linux]

On Thu, Jan 20, 2011 at 03:20:21AM -0800, Daniel Walker wrote:
> On Tue, 2011-01-18 at 15:03 -0800, Daniel Walker wrote:
> > Remove parts of this driver which use internal API calls. This
> > replaces the calls as suggested by Russell King.
> > 
> > Cc: Russell King - ARM Linux <linux@arm.linux.org.uk>
> > Signed-off-by: Daniel Walker <dwalker@codeaurora.org>
> > ---
> >  drivers/mmc/host/msm_sdcc.c |   49 +++++++++++++++++++-----------------------
> >  1 files changed, 22 insertions(+), 27 deletions(-)
> > 
> > diff --git a/drivers/mmc/host/msm_sdcc.c b/drivers/mmc/host/msm_sdcc.c
> > index 5decfd0..733d233 100644
> > --- a/drivers/mmc/host/msm_sdcc.c
> > +++ b/drivers/mmc/host/msm_sdcc.c
> > @@ -383,14 +383,30 @@ static int msmsdcc_config_dma(struct msmsdcc_host *host, struct mmc_data *data)
> >  	host->curr.user_pages = 0;
> >  
> >  	box = &nc->cmd[0];
> > -	for (i = 0; i < host->dma.num_ents; i++) {
> > -		box->cmd = CMD_MODE_BOX;
> >  
> > -	/* Initialize sg dma address */
> > -	sg->dma_address = page_to_dma(mmc_dev(host->mmc), sg_page(sg))
> > -				+ sg->offset;
> 
> It would seem there was a reason for this change.. The person that added
> this was Brent Degraaf (who is CC'd) ..
> 
> Quoting him below speaking about why dma_map_sg() isn't just used,
> 
> "Previous version of msm_sdcc.c had a cache coherency problem for
> precisely this reason. The dma_map_sg is what cleans the caches for
> these commands and so must be done AFTER the commands are populated. If
> this entry is left until the map function is called, the
> box->dst_row_addr will not be filled properly for reads."

It would be nice if problems such as this were discussed on the
linux-arm-kernel mailing list when they appear.

My response to this is:

dma_map_sg() does two things.  For each scatterlist entry:

* it converts the entry to a DMA address for the device and puts
  this in sg_dma_address().  The length of the entry is sg_dma_len().

* for each of the buffers described by the scatterlist, it ensures
  that the DMA device can see the data for DMA_TO_DEVICE transfers.
  for DMA_FROM_DEVICE transfers, it ensures that there are no dirty
  cache lines.  For some implementations the cache lines are
  invalidated to achieve this.

It is not guaranteed (and implementations do not predictably) cause any
other cache lines to be evicted or written back which are not described
by the scatterlist.

On dma_unmap_sg(), for each scatterlist entry:

* for DMA_FROM_DEVICE, some implementations cause the cache lines
  described by the scatterlist to be invalidated.

So, for anything _outside_ of the scatterlist, it is *unpredictable*
whether anything happens to them by way of the DMA API, and writing a
driver to rely on any behaviour observed is invalid.

The driver uses the passed scatterlist from the MMC layer to map, so the
only thing that the DMA API is going to touch is the pages which it's
performing IO on.

Therefore, it is entirely wrong for this driver to expect this box->blah
data to be written back by the DMA scatterlist mapping code.

Now, looking at the code a little more closely, host->dma.nc is allocated
via dma_alloc_coherent().  This returns either strongly ordered memory
for architectures prior to ARMv6, or 'normal non-cacheable' for ARMv6
onwards.

Strongly ordered requires no additional maintainence to ensure that writes
to it are immediately visible to hardware.  However, ARMv6 and later
requires a data synchronization barrier to ensure that writes to 'normal
non-cachable' memory are visible before writes to 'device' memory complete.

>From what I can see, the driver does use writel() as does the DMA driver
in arch/arm/mach-msm/dma.c, so there should be no problem with ARMv6 CPUs.

So, can we get a more detailed explaination as to the exact problem being
seen?

[PATCH] mmc: msm: fix dma usage not to use internal APIs

[Russell King - ARM Linux]

On Thu, Jan 20, 2011 at 03:20:21AM -0800, Daniel Walker wrote:
> On Tue, 2011-01-18 at 15:03 -0800, Daniel Walker wrote:
> > Remove parts of this driver which use internal API calls. This
> > replaces the calls as suggested by Russell King.
> > 
> > Cc: Russell King - ARM Linux <linux@arm.linux.org.uk>
> > Signed-off-by: Daniel Walker <dwalker@codeaurora.org>
> > ---
> >  drivers/mmc/host/msm_sdcc.c |   49 +++++++++++++++++++-----------------------
> >  1 files changed, 22 insertions(+), 27 deletions(-)
> > 
> > diff --git a/drivers/mmc/host/msm_sdcc.c b/drivers/mmc/host/msm_sdcc.c
> > index 5decfd0..733d233 100644
> > --- a/drivers/mmc/host/msm_sdcc.c
> > +++ b/drivers/mmc/host/msm_sdcc.c
> > @@ -383,14 +383,30 @@ static int msmsdcc_config_dma(struct msmsdcc_host *host, struct mmc_data *data)
> >  	host->curr.user_pages = 0;
> >  
> >  	box = &nc->cmd[0];
> > -	for (i = 0; i < host->dma.num_ents; i++) {
> > -		box->cmd = CMD_MODE_BOX;
> >  
> > -	/* Initialize sg dma address */
> > -	sg->dma_address = page_to_dma(mmc_dev(host->mmc), sg_page(sg))
> > -				+ sg->offset;
> 
> It would seem there was a reason for this change.. The person that added
> this was Brent Degraaf (who is CC'd) ..
> 
> Quoting him below speaking about why dma_map_sg() isn't just used,
> 
> "Previous version of msm_sdcc.c had a cache coherency problem for
> precisely this reason. The dma_map_sg is what cleans the caches for
> these commands and so must be done AFTER the commands are populated. If
> this entry is left until the map function is called, the
> box->dst_row_addr will not be filled properly for reads."

It would be nice if problems such as this were discussed on the
linux-arm-kernel mailing list when they appear.

My response to this is:

dma_map_sg() does two things.  For each scatterlist entry:

* it converts the entry to a DMA address for the device and puts
  this in sg_dma_address().  The length of the entry is sg_dma_len().

* for each of the buffers described by the scatterlist, it ensures
  that the DMA device can see the data for DMA_TO_DEVICE transfers.
  for DMA_FROM_DEVICE transfers, it ensures that there are no dirty
  cache lines.  For some implementations the cache lines are
  invalidated to achieve this.

It is not guaranteed (and implementations do not predictably) cause any
other cache lines to be evicted or written back which are not described
by the scatterlist.

On dma_unmap_sg(), for each scatterlist entry:

* for DMA_FROM_DEVICE, some implementations cause the cache lines
  described by the scatterlist to be invalidated.

So, for anything _outside_ of the scatterlist, it is *unpredictable*
whether anything happens to them by way of the DMA API, and writing a
driver to rely on any behaviour observed is invalid.

The driver uses the passed scatterlist from the MMC layer to map, so the
only thing that the DMA API is going to touch is the pages which it's
performing IO on.

Therefore, it is entirely wrong for this driver to expect this box->blah
data to be written back by the DMA scatterlist mapping code.

Now, looking at the code a little more closely, host->dma.nc is allocated
via dma_alloc_coherent().  This returns either strongly ordered memory
for architectures prior to ARMv6, or 'normal non-cacheable' for ARMv6
onwards.

Strongly ordered requires no additional maintainence to ensure that writes
to it are immediately visible to hardware.  However, ARMv6 and later
requires a data synchronization barrier to ensure that writes to 'normal
non-cachable' memory are visible before writes to 'device' memory complete.

Re: [PATCH] mmc: msm: fix dma usage not to use internal APIs

[Russell King - ARM Linux]

On Thu, Jan 20, 2011 at 01:02:46PM +0000, Russell King - ARM Linux wrote:
> Strongly ordered requires no additional maintainence to ensure that writes
> to it are immediately visible to hardware.  However, ARMv6 and later
> requires a data synchronization barrier to ensure that writes to 'normal
> non-cachable' memory are visible before writes to 'device' memory complete.
> 
> >From what I can see, the driver does use writel() as does the DMA driver
> in arch/arm/mach-msm/dma.c, so there should be no problem with ARMv6 CPUs.

BTW, it looks like the work-around was added at the time when writel() did
not have the necessary barriers:

commit 56a8b5b8ae81bd766e527a0e5274a087c3c1109d
Author: San Mehat <san@google.com>
Date:   Sat Nov 21 12:29:46 2009 -0800

    mmc: msm_sdcc: Reduce command timeouts and improve reliability.

+       n = dma_map_sg(mmc_dev(host->mmc), host->dma.sg,
+                       host->dma.num_ents, host->dma.dir);
+/* dsb inside dma_map_sg will write nc out to mem as well */
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    so we are talking about ARMv6 or later as previous versions did not
    have dsb.

vs

commit e936771a76a7b61ca55a5142a3de835c2e196871
Author: Catalin Marinas <catalin.marinas@arm.com>
Date:   Wed Jul 28 22:00:54 2010 +0100

    ARM: 6271/1: Introduce *_relaxed() I/O accessors

commit 79f64dbf68c8a9779a7e9a25e0a9f0217a25b57a
Author: Catalin Marinas <catalin.marinas@arm.com>
Date:   Wed Jul 28 22:01:55 2010 +0100

    ARM: 6273/1: Add barriers to the I/O accessors if ARM_DMA_MEM_BUFFERABLE

    When the coherent DMA buffers are mapped as Normal Non-cacheable
    (ARM_DMA_MEM_BUFFERABLE enabled), buffer accesses are no longer ordered
    with Device memory accesses causing failures in device drivers that do
    not use the mandatory memory barriers before starting a DMA transfer.
    LKML discussions led to the conclusion that such barriers have to be
    added to the I/O accessors:

    http://thread.gmane.org/gmane.linux.kernel/683509/focus=686153
    http://thread.gmane.org/gmane.linux.ide/46414
    http://thread.gmane.org/gmane.linux.kernel.cross-arch/5250

    This patch introduces a wmb() barrier to the write*() I/O accessors to
    handle the situations where Normal Non-cacheable writes are still in the
    processor (or L2 cache controller) write buffer before a DMA transfer
    command is issued. For the read*() accessors, a rmb() is introduced
    after the I/O to avoid speculative loads where the driver polls for a
    DMA transfer ready bit.

So the necessary barriers were found to be necessary way after MSM
discovered the problem.  It _is_ related to the ARMv6 weakly ordered
memory model, and it _was_ a bug in the ARM IO accessor implementation.

It would've been nice to have had the problem discussed at architecture
level so maybe the problem could've been found sooner and fixed earlier.

[PATCH] mmc: msm: fix dma usage not to use internal APIs

[Russell King - ARM Linux]

On Thu, Jan 20, 2011 at 01:02:46PM +0000, Russell King - ARM Linux wrote:
> Strongly ordered requires no additional maintainence to ensure that writes
> to it are immediately visible to hardware.  However, ARMv6 and later
> requires a data synchronization barrier to ensure that writes to 'normal
> non-cachable' memory are visible before writes to 'device' memory complete.
> 
> >From what I can see, the driver does use writel() as does the DMA driver
> in arch/arm/mach-msm/dma.c, so there should be no problem with ARMv6 CPUs.

BTW, it looks like the work-around was added at the time when writel() did
not have the necessary barriers:

commit 56a8b5b8ae81bd766e527a0e5274a087c3c1109d
Author: San Mehat <san@google.com>
Date:   Sat Nov 21 12:29:46 2009 -0800

    mmc: msm_sdcc: Reduce command timeouts and improve reliability.

+       n = dma_map_sg(mmc_dev(host->mmc), host->dma.sg,
+                       host->dma.num_ents, host->dma.dir);
+/* dsb inside dma_map_sg will write nc out to mem as well */
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    so we are talking about ARMv6 or later as previous versions did not
    have dsb.

vs

commit e936771a76a7b61ca55a5142a3de835c2e196871
Author: Catalin Marinas <catalin.marinas@arm.com>
Date:   Wed Jul 28 22:00:54 2010 +0100

    ARM: 6271/1: Introduce *_relaxed() I/O accessors

commit 79f64dbf68c8a9779a7e9a25e0a9f0217a25b57a
Author: Catalin Marinas <catalin.marinas@arm.com>
Date:   Wed Jul 28 22:01:55 2010 +0100

    ARM: 6273/1: Add barriers to the I/O accessors if ARM_DMA_MEM_BUFFERABLE

    When the coherent DMA buffers are mapped as Normal Non-cacheable
    (ARM_DMA_MEM_BUFFERABLE enabled), buffer accesses are no longer ordered
    with Device memory accesses causing failures in device drivers that do
    not use the mandatory memory barriers before starting a DMA transfer.
    LKML discussions led to the conclusion that such barriers have to be
    added to the I/O accessors:

    http://thread.gmane.org/gmane.linux.kernel/683509/focus=686153
    http://thread.gmane.org/gmane.linux.ide/46414
    http://thread.gmane.org/gmane.linux.kernel.cross-arch/5250

    This patch introduces a wmb() barrier to the write*() I/O accessors to
    handle the situations where Normal Non-cacheable writes are still in the
    processor (or L2 cache controller) write buffer before a DMA transfer
    command is issued. For the read*() accessors, a rmb() is introduced
    after the I/O to avoid speculative loads where the driver polls for a
    DMA transfer ready bit.

So the necessary barriers were found to be necessary way after MSM
discovered the problem.  It _is_ related to the ARMv6 weakly ordered
memory model, and it _was_ a bug in the ARM IO accessor implementation.

It would've been nice to have had the problem discussed at architecture
level so maybe the problem could've been found sooner and fixed earlier.

Re: [PATCH] mmc: msm: fix dma usage not to use internal APIs

[Daniel Walker]

On Thu, 2011-01-20 at 13:12 +0000, Russell King - ARM Linux wrote:
> On Thu, Jan 20, 2011 at 01:02:46PM +0000, Russell King - ARM Linux wrote:
> > Strongly ordered requires no additional maintainence to ensure that writes
> > to it are immediately visible to hardware.  However, ARMv6 and later
> > requires a data synchronization barrier to ensure that writes to 'normal
> > non-cachable' memory are visible before writes to 'device' memory complete.
> > 
> > >From what I can see, the driver does use writel() as does the DMA driver
> > in arch/arm/mach-msm/dma.c, so there should be no problem with ARMv6 CPUs.
> 
> BTW, it looks like the work-around was added at the time when writel() did
> not have the necessary barriers:
> 
> commit 56a8b5b8ae81bd766e527a0e5274a087c3c1109d
> Author: San Mehat <san@google.com>
> Date:   Sat Nov 21 12:29:46 2009 -0800
> 
>     mmc: msm_sdcc: Reduce command timeouts and improve reliability.
> 
> +       n = dma_map_sg(mmc_dev(host->mmc), host->dma.sg,
> +                       host->dma.num_ents, host->dma.dir);
> +/* dsb inside dma_map_sg will write nc out to mem as well */
>     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>     so we are talking about ARMv6 or later as previous versions did not
>     have dsb.

The changes were created in early Nov. 2009 on a 2.6.29 kernel,

> vs
> 
> commit e936771a76a7b61ca55a5142a3de835c2e196871
> Author: Catalin Marinas <catalin.marinas@arm.com>
> Date:   Wed Jul 28 22:00:54 2010 +0100
> 
>     ARM: 6271/1: Introduce *_relaxed() I/O accessors
> 
> commit 79f64dbf68c8a9779a7e9a25e0a9f0217a25b57a
> Author: Catalin Marinas <catalin.marinas@arm.com>
> Date:   Wed Jul 28 22:01:55 2010 +0100

well before these two commits.

> So the necessary barriers were found to be necessary way after MSM
> discovered the problem.  It _is_ related to the ARMv6 weakly ordered
> memory model, and it _was_ a bug in the ARM IO accessor implementation.

Ok, so unless Brent wants to step in an give more comments on this it
sounds like the problem has been fix already ..

> It would've been nice to have had the problem discussed at architecture
> level so maybe the problem could've been found sooner and fixed earlier.

Yes .. At least we're communicating now ..

Daniel


-- 
Sent by an consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum.

[PATCH] mmc: msm: fix dma usage not to use internal APIs

[Daniel Walker]

On Thu, 2011-01-20 at 13:12 +0000, Russell King - ARM Linux wrote:
> On Thu, Jan 20, 2011 at 01:02:46PM +0000, Russell King - ARM Linux wrote:
> > Strongly ordered requires no additional maintainence to ensure that writes
> > to it are immediately visible to hardware.  However, ARMv6 and later
> > requires a data synchronization barrier to ensure that writes to 'normal
> > non-cachable' memory are visible before writes to 'device' memory complete.
> > 
> > >From what I can see, the driver does use writel() as does the DMA driver
> > in arch/arm/mach-msm/dma.c, so there should be no problem with ARMv6 CPUs.
> 
> BTW, it looks like the work-around was added at the time when writel() did
> not have the necessary barriers:
> 
> commit 56a8b5b8ae81bd766e527a0e5274a087c3c1109d
> Author: San Mehat <san@google.com>
> Date:   Sat Nov 21 12:29:46 2009 -0800
> 
>     mmc: msm_sdcc: Reduce command timeouts and improve reliability.
> 
> +       n = dma_map_sg(mmc_dev(host->mmc), host->dma.sg,
> +                       host->dma.num_ents, host->dma.dir);
> +/* dsb inside dma_map_sg will write nc out to mem as well */
>     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>     so we are talking about ARMv6 or later as previous versions did not
>     have dsb.

The changes were created in early Nov. 2009 on a 2.6.29 kernel,

> vs
> 
> commit e936771a76a7b61ca55a5142a3de835c2e196871
> Author: Catalin Marinas <catalin.marinas@arm.com>
> Date:   Wed Jul 28 22:00:54 2010 +0100
> 
>     ARM: 6271/1: Introduce *_relaxed() I/O accessors
> 
> commit 79f64dbf68c8a9779a7e9a25e0a9f0217a25b57a
> Author: Catalin Marinas <catalin.marinas@arm.com>
> Date:   Wed Jul 28 22:01:55 2010 +0100

well before these two commits.

> So the necessary barriers were found to be necessary way after MSM
> discovered the problem.  It _is_ related to the ARMv6 weakly ordered
> memory model, and it _was_ a bug in the ARM IO accessor implementation.

Ok, so unless Brent wants to step in an give more comments on this it
sounds like the problem has been fix already ..

> It would've been nice to have had the problem discussed at architecture
> level so maybe the problem could've been found sooner and fixed earlier.

Yes .. At least we're communicating now ..

Daniel


-- 
Sent by an consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum.

Re: [PATCH] mmc: msm: fix dma usage not to use internal APIs

[Brent DeGraaf]

Russell,

This code was not added simply for the dsb inside the dma_map_sg call.

This dma mapping call was introduced to deal with speculative dfetches:
the scatter-gather area can be in normal memory, so we need to do a cache
invalidate (which is taken care of by the mapping function) before reading
data into the area using dma, or it's possible that a speculative dfetch
could pull old data from the cache during the transfer.  (Maybe I should
have beefed up the comment with more detail explaining  the role of the
whole mapping call instead of using just the word "also" to signify that
the non-cacheable box data was also put in-order from this command.)

BTW, I have just looked at the new kernel mapping routines and they still
do the proper thing for speculative cpus, but older cpus without
speculative data fetches will do an unnecessary pre-invalidate.

I'd like to talk about the additional barriers added to writel, however. 
Our approach for such writes is to only add a barrier when ordering was
important because barriering between each individual writel will interfere
with our cpu's write-gathering capabilities, slowing things up a bit. 
Perhaps something could be done that is mach-based for this macro.  Do you
have any suggestions?

Best regards,
Brent DeGraaf

On Thu, January 20, 2011 7:08 am, Daniel Walker wrote:
> On Thu, 2011-01-20 at 13:12 +0000, Russell King - ARM Linux wrote:
>> On Thu, Jan 20, 2011 at 01:02:46PM +0000, Russell King - ARM Linux
>> wrote:
>> > Strongly ordered requires no additional maintainence to ensure that
>> writes
>> > to it are immediately visible to hardware.  However, ARMv6 and later
>> > requires a data synchronization barrier to ensure that writes to
>> 'normal
>> > non-cachable' memory are visible before writes to 'device' memory
>> complete.
>> >
>> > >From what I can see, the driver does use writel() as does the DMA
>> driver
>> > in arch/arm/mach-msm/dma.c, so there should be no problem with ARMv6
>> CPUs.
>>
>> BTW, it looks like the work-around was added at the time when writel()
>> did
>> not have the necessary barriers:
>>
>> commit 56a8b5b8ae81bd766e527a0e5274a087c3c1109d
>> Author: San Mehat <san@google.com>
>> Date:   Sat Nov 21 12:29:46 2009 -0800
>>
>>     mmc: msm_sdcc: Reduce command timeouts and improve reliability.
>>
>> +       n = dma_map_sg(mmc_dev(host->mmc), host->dma.sg,
>> +                       host->dma.num_ents, host->dma.dir);
>> +/* dsb inside dma_map_sg will write nc out to mem as well */
>>     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>     so we are talking about ARMv6 or later as previous versions did not
>>     have dsb.
>
> The changes were created in early Nov. 2009 on a 2.6.29 kernel,
>
>> vs
>>
>> commit e936771a76a7b61ca55a5142a3de835c2e196871
>> Author: Catalin Marinas <catalin.marinas@arm.com>
>> Date:   Wed Jul 28 22:00:54 2010 +0100
>>
>>     ARM: 6271/1: Introduce *_relaxed() I/O accessors
>>
>> commit 79f64dbf68c8a9779a7e9a25e0a9f0217a25b57a
>> Author: Catalin Marinas <catalin.marinas@arm.com>
>> Date:   Wed Jul 28 22:01:55 2010 +0100
>
> well before these two commits.
>
>> So the necessary barriers were found to be necessary way after MSM
>> discovered the problem.  It _is_ related to the ARMv6 weakly ordered
>> memory model, and it _was_ a bug in the ARM IO accessor implementation.
>
> Ok, so unless Brent wants to step in an give more comments on this it
> sounds like the problem has been fix already ..
>
>> It would've been nice to have had the problem discussed at architecture
>> level so maybe the problem could've been found sooner and fixed earlier.
>
> Yes .. At least we're communicating now ..
>
> Daniel
>
>
> --
> Sent by an consultant of the Qualcomm Innovation Center, Inc.
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
> Forum.
>
>
>


-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH] mmc: msm: fix dma usage not to use internal APIs

[Brent DeGraaf]

Russell,

This code was not added simply for the dsb inside the dma_map_sg call.

This dma mapping call was introduced to deal with speculative dfetches:
the scatter-gather area can be in normal memory, so we need to do a cache
invalidate (which is taken care of by the mapping function) before reading
data into the area using dma, or it's possible that a speculative dfetch
could pull old data from the cache during the transfer.  (Maybe I should
have beefed up the comment with more detail explaining  the role of the
whole mapping call instead of using just the word "also" to signify that
the non-cacheable box data was also put in-order from this command.)

BTW, I have just looked at the new kernel mapping routines and they still
do the proper thing for speculative cpus, but older cpus without
speculative data fetches will do an unnecessary pre-invalidate.

I'd like to talk about the additional barriers added to writel, however. 
Our approach for such writes is to only add a barrier when ordering was
important because barriering between each individual writel will interfere
with our cpu's write-gathering capabilities, slowing things up a bit. 
Perhaps something could be done that is mach-based for this macro.  Do you
have any suggestions?

Best regards,
Brent DeGraaf

On Thu, January 20, 2011 7:08 am, Daniel Walker wrote:
> On Thu, 2011-01-20 at 13:12 +0000, Russell King - ARM Linux wrote:
>> On Thu, Jan 20, 2011 at 01:02:46PM +0000, Russell King - ARM Linux
>> wrote:
>> > Strongly ordered requires no additional maintainence to ensure that
>> writes
>> > to it are immediately visible to hardware.  However, ARMv6 and later
>> > requires a data synchronization barrier to ensure that writes to
>> 'normal
>> > non-cachable' memory are visible before writes to 'device' memory
>> complete.
>> >
>> > >From what I can see, the driver does use writel() as does the DMA
>> driver
>> > in arch/arm/mach-msm/dma.c, so there should be no problem with ARMv6
>> CPUs.
>>
>> BTW, it looks like the work-around was added at the time when writel()
>> did
>> not have the necessary barriers:
>>
>> commit 56a8b5b8ae81bd766e527a0e5274a087c3c1109d
>> Author: San Mehat <san@google.com>
>> Date:   Sat Nov 21 12:29:46 2009 -0800
>>
>>     mmc: msm_sdcc: Reduce command timeouts and improve reliability.
>>
>> +       n = dma_map_sg(mmc_dev(host->mmc), host->dma.sg,
>> +                       host->dma.num_ents, host->dma.dir);
>> +/* dsb inside dma_map_sg will write nc out to mem as well */
>>     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>     so we are talking about ARMv6 or later as previous versions did not
>>     have dsb.
>
> The changes were created in early Nov. 2009 on a 2.6.29 kernel,
>
>> vs
>>
>> commit e936771a76a7b61ca55a5142a3de835c2e196871
>> Author: Catalin Marinas <catalin.marinas@arm.com>
>> Date:   Wed Jul 28 22:00:54 2010 +0100
>>
>>     ARM: 6271/1: Introduce *_relaxed() I/O accessors
>>
>> commit 79f64dbf68c8a9779a7e9a25e0a9f0217a25b57a
>> Author: Catalin Marinas <catalin.marinas@arm.com>
>> Date:   Wed Jul 28 22:01:55 2010 +0100
>
> well before these two commits.
>
>> So the necessary barriers were found to be necessary way after MSM
>> discovered the problem.  It _is_ related to the ARMv6 weakly ordered
>> memory model, and it _was_ a bug in the ARM IO accessor implementation.
>
> Ok, so unless Brent wants to step in an give more comments on this it
> sounds like the problem has been fix already ..
>
>> It would've been nice to have had the problem discussed at architecture
>> level so maybe the problem could've been found sooner and fixed earlier.
>
> Yes .. At least we're communicating now ..
>
> Daniel
>
>
> --
> Sent by an consultant of the Qualcomm Innovation Center, Inc.
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
> Forum.
>
>
>


-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH] mmc: msm: fix dma usage not to use internal APIs

[Brent DeGraaf]

Russell,

I just had a chat with Daniel and I think I understand what you're doing
now.  The reason for the original change was to ensure there was a barrier
(dmb minimum) between population of the nc box structure and the command
port write to the datamover.  With the original code structure, the dsb
for the cache management is happening too early to benefit the nc writes.
Since dsbs are costly operations, I elected to call the other api, then do
the cache management with its barrier after everything was populated.

Since the nc box and the command port writes are not using writel to do
their assignment (unless I'm missing some change here), at minimum we'd
need to add a dmb at the point where the dma_map_sg call was done in my
prior fix if move it back to its original location.  Performance will
suffer, but it'll be reliable.

Best regards,
Brent


On Fri, January 21, 2011 8:13 am, Brent DeGraaf wrote:
> Russell,
>
> This code was not added simply for the dsb inside the dma_map_sg call.
>
> This dma mapping call was introduced to deal with speculative dfetches:
> the scatter-gather area can be in normal memory, so we need to do a cache
> invalidate (which is taken care of by the mapping function) before reading
> data into the area using dma, or it's possible that a speculative dfetch
> could pull old data from the cache during the transfer.  (Maybe I should
> have beefed up the comment with more detail explaining  the role of the
> whole mapping call instead of using just the word "also" to signify that
> the non-cacheable box data was also put in-order from this command.)
>
> BTW, I have just looked at the new kernel mapping routines and they still
> do the proper thing for speculative cpus, but older cpus without
> speculative data fetches will do an unnecessary pre-invalidate.
>
> I'd like to talk about the additional barriers added to writel, however.
> Our approach for such writes is to only add a barrier when ordering was
> important because barriering between each individual writel will interfere
> with our cpu's write-gathering capabilities, slowing things up a bit.
> Perhaps something could be done that is mach-based for this macro.  Do you
> have any suggestions?
>
> Best regards,
> Brent DeGraaf
>
> On Thu, January 20, 2011 7:08 am, Daniel Walker wrote:
>> On Thu, 2011-01-20 at 13:12 +0000, Russell King - ARM Linux wrote:
>>> On Thu, Jan 20, 2011 at 01:02:46PM +0000, Russell King - ARM Linux
>>> wrote:
>>> > Strongly ordered requires no additional maintainence to ensure that
>>> writes
>>> > to it are immediately visible to hardware.  However, ARMv6 and later
>>> > requires a data synchronization barrier to ensure that writes to
>>> 'normal
>>> > non-cachable' memory are visible before writes to 'device' memory
>>> complete.
>>> >
>>> > >From what I can see, the driver does use writel() as does the DMA
>>> driver
>>> > in arch/arm/mach-msm/dma.c, so there should be no problem with ARMv6
>>> CPUs.
>>>
>>> BTW, it looks like the work-around was added at the time when writel()
>>> did
>>> not have the necessary barriers:
>>>
>>> commit 56a8b5b8ae81bd766e527a0e5274a087c3c1109d
>>> Author: San Mehat <san@google.com>
>>> Date:   Sat Nov 21 12:29:46 2009 -0800
>>>
>>>     mmc: msm_sdcc: Reduce command timeouts and improve reliability.
>>>
>>> +       n = dma_map_sg(mmc_dev(host->mmc), host->dma.sg,
>>> +                       host->dma.num_ents, host->dma.dir);
>>> +/* dsb inside dma_map_sg will write nc out to mem as well */
>>>     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>>     so we are talking about ARMv6 or later as previous versions did not
>>>     have dsb.
>>
>> The changes were created in early Nov. 2009 on a 2.6.29 kernel,
>>
>>> vs
>>>
>>> commit e936771a76a7b61ca55a5142a3de835c2e196871
>>> Author: Catalin Marinas <catalin.marinas@arm.com>
>>> Date:   Wed Jul 28 22:00:54 2010 +0100
>>>
>>>     ARM: 6271/1: Introduce *_relaxed() I/O accessors
>>>
>>> commit 79f64dbf68c8a9779a7e9a25e0a9f0217a25b57a
>>> Author: Catalin Marinas <catalin.marinas@arm.com>
>>> Date:   Wed Jul 28 22:01:55 2010 +0100
>>
>> well before these two commits.
>>
>>> So the necessary barriers were found to be necessary way after MSM
>>> discovered the problem.  It _is_ related to the ARMv6 weakly ordered
>>> memory model, and it _was_ a bug in the ARM IO accessor implementation.
>>
>> Ok, so unless Brent wants to step in an give more comments on this it
>> sounds like the problem has been fix already ..
>>
>>> It would've been nice to have had the problem discussed at architecture
>>> level so maybe the problem could've been found sooner and fixed
>>> earlier.
>>
>> Yes .. At least we're communicating now ..
>>
>> Daniel
>>
>>
>> --
>> Sent by an consultant of the Qualcomm Innovation Center, Inc.
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
>> Forum.
>>
>>
>>


-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH] mmc: msm: fix dma usage not to use internal APIs

[Brent DeGraaf]

Russell,

I just had a chat with Daniel and I think I understand what you're doing
now.  The reason for the original change was to ensure there was a barrier
(dmb minimum) between population of the nc box structure and the command
port write to the datamover.  With the original code structure, the dsb
for the cache management is happening too early to benefit the nc writes.
Since dsbs are costly operations, I elected to call the other api, then do
the cache management with its barrier after everything was populated.

Since the nc box and the command port writes are not using writel to do
their assignment (unless I'm missing some change here), at minimum we'd
need to add a dmb at the point where the dma_map_sg call was done in my
prior fix if move it back to its original location.  Performance will
suffer, but it'll be reliable.

Best regards,
Brent


On Fri, January 21, 2011 8:13 am, Brent DeGraaf wrote:
> Russell,
>
> This code was not added simply for the dsb inside the dma_map_sg call.
>
> This dma mapping call was introduced to deal with speculative dfetches:
> the scatter-gather area can be in normal memory, so we need to do a cache
> invalidate (which is taken care of by the mapping function) before reading
> data into the area using dma, or it's possible that a speculative dfetch
> could pull old data from the cache during the transfer.  (Maybe I should
> have beefed up the comment with more detail explaining  the role of the
> whole mapping call instead of using just the word "also" to signify that
> the non-cacheable box data was also put in-order from this command.)
>
> BTW, I have just looked at the new kernel mapping routines and they still
> do the proper thing for speculative cpus, but older cpus without
> speculative data fetches will do an unnecessary pre-invalidate.
>
> I'd like to talk about the additional barriers added to writel, however.
> Our approach for such writes is to only add a barrier when ordering was
> important because barriering between each individual writel will interfere
> with our cpu's write-gathering capabilities, slowing things up a bit.
> Perhaps something could be done that is mach-based for this macro.  Do you
> have any suggestions?
>
> Best regards,
> Brent DeGraaf
>
> On Thu, January 20, 2011 7:08 am, Daniel Walker wrote:
>> On Thu, 2011-01-20 at 13:12 +0000, Russell King - ARM Linux wrote:
>>> On Thu, Jan 20, 2011 at 01:02:46PM +0000, Russell King - ARM Linux
>>> wrote:
>>> > Strongly ordered requires no additional maintainence to ensure that
>>> writes
>>> > to it are immediately visible to hardware.  However, ARMv6 and later
>>> > requires a data synchronization barrier to ensure that writes to
>>> 'normal
>>> > non-cachable' memory are visible before writes to 'device' memory
>>> complete.
>>> >
>>> > >From what I can see, the driver does use writel() as does the DMA
>>> driver
>>> > in arch/arm/mach-msm/dma.c, so there should be no problem with ARMv6
>>> CPUs.
>>>
>>> BTW, it looks like the work-around was added at the time when writel()
>>> did
>>> not have the necessary barriers:
>>>
>>> commit 56a8b5b8ae81bd766e527a0e5274a087c3c1109d
>>> Author: San Mehat <san@google.com>
>>> Date:   Sat Nov 21 12:29:46 2009 -0800
>>>
>>>     mmc: msm_sdcc: Reduce command timeouts and improve reliability.
>>>
>>> +       n = dma_map_sg(mmc_dev(host->mmc), host->dma.sg,
>>> +                       host->dma.num_ents, host->dma.dir);
>>> +/* dsb inside dma_map_sg will write nc out to mem as well */
>>>     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>>     so we are talking about ARMv6 or later as previous versions did not
>>>     have dsb.
>>
>> The changes were created in early Nov. 2009 on a 2.6.29 kernel,
>>
>>> vs
>>>
>>> commit e936771a76a7b61ca55a5142a3de835c2e196871
>>> Author: Catalin Marinas <catalin.marinas@arm.com>
>>> Date:   Wed Jul 28 22:00:54 2010 +0100
>>>
>>>     ARM: 6271/1: Introduce *_relaxed() I/O accessors
>>>
>>> commit 79f64dbf68c8a9779a7e9a25e0a9f0217a25b57a
>>> Author: Catalin Marinas <catalin.marinas@arm.com>
>>> Date:   Wed Jul 28 22:01:55 2010 +0100
>>
>> well before these two commits.
>>
>>> So the necessary barriers were found to be necessary way after MSM
>>> discovered the problem.  It _is_ related to the ARMv6 weakly ordered
>>> memory model, and it _was_ a bug in the ARM IO accessor implementation.
>>
>> Ok, so unless Brent wants to step in an give more comments on this it
>> sounds like the problem has been fix already ..
>>
>>> It would've been nice to have had the problem discussed at architecture
>>> level so maybe the problem could've been found sooner and fixed
>>> earlier.
>>
>> Yes .. At least we're communicating now ..
>>
>> Daniel
>>
>>
>> --
>> Sent by an consultant of the Qualcomm Innovation Center, Inc.
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
>> Forum.
>>
>>
>>


-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH] mmc: msm: fix dma usage not to use internal APIs

[Russell King - ARM Linux]

On Fri, Jan 21, 2011 at 08:57:42AM -0800, Brent DeGraaf wrote:
> Russell,
> 
> I just had a chat with Daniel and I think I understand what you're doing
> now.  The reason for the original change was to ensure there was a barrier
> (dmb minimum) between population of the nc box structure and the command
> port write to the datamover.

> With the original code structure, the dsb for the cache management is
> happening too early to benefit the nc writes.

That DSB has precisely *nothing* to do with making sure that anything but
the cache operations which the DMA API performed are complete by the time
the API returns.

> Since dsbs are costly operations, I elected to call the other api, then do
> the cache management with its barrier after everything was populated.
> 
> Since the nc box and the command port writes are not using writel to do
> their assignment (unless I'm missing some change here),

You must not use writel on anything but ioremapped memory.  So that's
correct.

Writing to the device registers though should be using a memory accessor
like writel.  That contains the necessary barrier to ensure that any
previous writes to memory will be ordered with respect to device writes.

As you seem to already be using writel, you have all the necessary barriers
in place provided by architecture code.

[PATCH] mmc: msm: fix dma usage not to use internal APIs

[Russell King - ARM Linux]

On Fri, Jan 21, 2011 at 08:57:42AM -0800, Brent DeGraaf wrote:
> Russell,
> 
> I just had a chat with Daniel and I think I understand what you're doing
> now.  The reason for the original change was to ensure there was a barrier
> (dmb minimum) between population of the nc box structure and the command
> port write to the datamover.

> With the original code structure, the dsb for the cache management is
> happening too early to benefit the nc writes.

That DSB has precisely *nothing* to do with making sure that anything but
the cache operations which the DMA API performed are complete by the time
the API returns.

> Since dsbs are costly operations, I elected to call the other api, then do
> the cache management with its barrier after everything was populated.
> 
> Since the nc box and the command port writes are not using writel to do
> their assignment (unless I'm missing some change here),

You must not use writel on anything but ioremapped memory.  So that's
correct.

Writing to the device registers though should be using a memory accessor
like writel.  That contains the necessary barrier to ensure that any
previous writes to memory will be ordered with respect to device writes.

As you seem to already be using writel, you have all the necessary barriers
in place provided by architecture code.

Re: [PATCH] mmc: msm: fix dma usage not to use internal APIs

[Russell King - ARM Linux]

On Fri, Jan 21, 2011 at 08:13:59AM -0800, Brent DeGraaf wrote:
> Russell,
> 
> This code was not added simply for the dsb inside the dma_map_sg call.
> 
> This dma mapping call was introduced to deal with speculative dfetches:
> the scatter-gather area can be in normal memory, so we need to do a cache
> invalidate (which is taken care of by the mapping function) before reading
> data into the area using dma, or it's possible that a speculative dfetch
> could pull old data from the cache during the transfer.  (Maybe I should
> have beefed up the comment with more detail explaining  the role of the
> whole mapping call instead of using just the word "also" to signify that
> the non-cacheable box data was also put in-order from this command.)

Again, why don't you talk about these problems on the architecture list
rather than keeping it to your self.

This is one of the *biggest* problems I have with people setting up per-
platform mailing lists.  It hides information from the rest of the community
and causes idiotic practices and abuses such as has happened here.

In October 2009, the problem of speculative accesses causing DMA buffer
corruption was fixed.

> BTW, I have just looked at the new kernel mapping routines and they still
> do the proper thing for speculative cpus, but older cpus without
> speculative data fetches will do an unnecessary pre-invalidate.

Sigh.  Your comment is just soo wrong there it's untrue - do you have
any idea what would happen if we didn't invalidate before passing the
buffer to DMA for DMA-from-device?

Think about what would happen if during the DMA operation, the buffer
contained some dirty cache lines, and these happened to be evicted over
the top of the DMA'd data, corrupting it.

The invalidate on map is something that's been there since day one of
ARMs DMA API support.  It's not unnecessary, it's really very very
important.

> I'd like to talk about the additional barriers added to writel, however. 
> Our approach for such writes is to only add a barrier when ordering was
> important because barriering between each individual writel will interfere
> with our cpu's write-gathering capabilities, slowing things up a bit. 

That's one of the prices we pay for sharing the kernel with other
architectures.  Linus Torvalds refuses to tackle the problem of weakly
ordered IO - his position is that architectures with weakly ordered
memory models *must* behave in the same way that x86 does.  He has a
point, as many drivers only get written and tested on x86, so driver
writers have no idea whether their driver would work on a relaxed IO
CPU.

Many people from various different architectures have tried to influence
this position but have always been unsuccessful.  Architectures have
tried to get agreement on a way to allow relaxed IO, but it hasn't
happened.

The barrier in writel() has to stay to ensure correctness with existing
drivers.  Same for readl()'s memory barrier.  However, we do have a set
of relaxed operations - but the problem is that they can not be used in
any driver which is shared with other architectures as they aren't an
official part of the IO API.

So, if you're happy that your driver is limited to only ever running on
ARM, then you can use the relaxed IO operators.  Just append a _relaxed
suffix.  Bear in mind though that you will have to add an appropriate
mb() between writes to coherent DMA memory and the write which tells
the DMA hardware to start accessing that memory - or use writel()
instead (which is probably the preferred method).

[PATCH] mmc: msm: fix dma usage not to use internal APIs

[Russell King - ARM Linux]

On Fri, Jan 21, 2011 at 08:13:59AM -0800, Brent DeGraaf wrote:
> Russell,
> 
> This code was not added simply for the dsb inside the dma_map_sg call.
> 
> This dma mapping call was introduced to deal with speculative dfetches:
> the scatter-gather area can be in normal memory, so we need to do a cache
> invalidate (which is taken care of by the mapping function) before reading
> data into the area using dma, or it's possible that a speculative dfetch
> could pull old data from the cache during the transfer.  (Maybe I should
> have beefed up the comment with more detail explaining  the role of the
> whole mapping call instead of using just the word "also" to signify that
> the non-cacheable box data was also put in-order from this command.)

Again, why don't you talk about these problems on the architecture list
rather than keeping it to your self.

This is one of the *biggest* problems I have with people setting up per-
platform mailing lists.  It hides information from the rest of the community
and causes idiotic practices and abuses such as has happened here.

In October 2009, the problem of speculative accesses causing DMA buffer
corruption was fixed.

> BTW, I have just looked at the new kernel mapping routines and they still
> do the proper thing for speculative cpus, but older cpus without
> speculative data fetches will do an unnecessary pre-invalidate.

Sigh.  Your comment is just soo wrong there it's untrue - do you have
any idea what would happen if we didn't invalidate before passing the
buffer to DMA for DMA-from-device?

Think about what would happen if during the DMA operation, the buffer
contained some dirty cache lines, and these happened to be evicted over
the top of the DMA'd data, corrupting it.

The invalidate on map is something that's been there since day one of
ARMs DMA API support.  It's not unnecessary, it's really very very
important.

> I'd like to talk about the additional barriers added to writel, however. 
> Our approach for such writes is to only add a barrier when ordering was
> important because barriering between each individual writel will interfere
> with our cpu's write-gathering capabilities, slowing things up a bit. 

That's one of the prices we pay for sharing the kernel with other
architectures.  Linus Torvalds refuses to tackle the problem of weakly
ordered IO - his position is that architectures with weakly ordered
memory models *must* behave in the same way that x86 does.  He has a
point, as many drivers only get written and tested on x86, so driver
writers have no idea whether their driver would work on a relaxed IO
CPU.

Many people from various different architectures have tried to influence
this position but have always been unsuccessful.  Architectures have
tried to get agreement on a way to allow relaxed IO, but it hasn't
happened.

The barrier in writel() has to stay to ensure correctness with existing
drivers.  Same for readl()'s memory barrier.  However, we do have a set
of relaxed operations - but the problem is that they can not be used in
any driver which is shared with other architectures as they aren't an
official part of the IO API.

So, if you're happy that your driver is limited to only ever running on
ARM, then you can use the relaxed IO operators.  Just append a _relaxed
suffix.  Bear in mind though that you will have to add an appropriate
mb() between writes to coherent DMA memory and the write which tells
the DMA hardware to start accessing that memory - or use writel()
instead (which is probably the preferred method).

Re: [PATCH] mmc: msm: fix dma usage not to use internal APIs

[Daniel Walker]

On Fri, 2011-01-21 at 19:28 +0000, Russell King - ARM Linux wrote:
> Again, why don't you talk about these problems on the architecture
> list
> rather than keeping it to your self.
> 
> This is one of the *biggest* problems I have with people setting up
> per-
> platform mailing lists.  It hides information from the rest of the
> community
> and causes idiotic practices and abuses such as has happened here.

I don't think this was discussed on any list .. It was done prior to
QuiC involvement in the community ..

Daniel

-- 
Sent by an consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum.

[PATCH] mmc: msm: fix dma usage not to use internal APIs

[Daniel Walker]

On Fri, 2011-01-21 at 19:28 +0000, Russell King - ARM Linux wrote:
> Again, why don't you talk about these problems on the architecture
> list
> rather than keeping it to your self.
> 
> This is one of the *biggest* problems I have with people setting up
> per-
> platform mailing lists.  It hides information from the rest of the
> community
> and causes idiotic practices and abuses such as has happened here.

I don't think this was discussed on any list .. It was done prior to
QuiC involvement in the community ..

Daniel

-- 
Sent by an consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum.

Re: [PATCH] mmc: msm: fix dma usage not to use internal APIs

[David Brown]

On Fri, Jan 21 2011, Russell King - ARM Linux wrote:

> On Fri, Jan 21, 2011 at 08:13:59AM -0800, Brent DeGraaf wrote:

> This is one of the *biggest* problems I have with people setting up per-
> platform mailing lists.  It hides information from the rest of the community
> and causes idiotic practices and abuses such as has happened here.

I have been requesting, internally, that nothing should be sent
to the linux-arm-msm mailing list that isn't sent to at least
linux-arm-kernel, and preferrably linux-kernel as well.  It hasn't
happened yet, but I will be asking people to resend patches if they only
send them to the MSM list.

As Daniel pointed out, though, I don't think this particular case was
even discussed.  We're trying to get better.

David

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH] mmc: msm: fix dma usage not to use internal APIs

[David Brown]

On Fri, Jan 21 2011, Russell King - ARM Linux wrote:

> On Fri, Jan 21, 2011 at 08:13:59AM -0800, Brent DeGraaf wrote:

> This is one of the *biggest* problems I have with people setting up per-
> platform mailing lists.  It hides information from the rest of the community
> and causes idiotic practices and abuses such as has happened here.

I have been requesting, internally, that nothing should be sent
to the linux-arm-msm mailing list that isn't sent to at least
linux-arm-kernel, and preferrably linux-kernel as well.  It hasn't
happened yet, but I will be asking people to resend patches if they only
send them to the MSM list.

As Daniel pointed out, though, I don't think this particular case was
even discussed.  We're trying to get better.

David

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH] mmc: msm: fix dma usage not to use internal APIs

[Brent DeGraaf]

Russell,

Thanks for your comments. Yes, reverting is OK. The writel's are doing the
controller write after the barrier so even though it'll be a little
slower, it will be correct.

Regarding the "unnecessary" pre-invalidate, yeah, I misspoke.  I meant
unnecessary POST-invalidates for non-speculative cpus.  I just checked and
it looks like someone's already put a "FIXME" in the current 2.6.37 source
for those.

We'll look into how much gains we can expect from the relaxed I/O calls
and decide where to go from there.  Thanks for pointing out the api.

Best regards,
Brent

On Fri, January 21, 2011 1:17 pm, David Brown wrote:
> On Fri, Jan 21 2011, Russell King - ARM Linux wrote:
>
>> On Fri, Jan 21, 2011 at 08:13:59AM -0800, Brent DeGraaf wrote:
>
>> This is one of the *biggest* problems I have with people setting up per-
>> platform mailing lists.  It hides information from the rest of the
>> community
>> and causes idiotic practices and abuses such as has happened here.
>
> I have been requesting, internally, that nothing should be sent
> to the linux-arm-msm mailing list that isn't sent to at least
> linux-arm-kernel, and preferrably linux-kernel as well.  It hasn't
> happened yet, but I will be asking people to resend patches if they only
> send them to the MSM list.
>
> As Daniel pointed out, though, I don't think this particular case was
> even discussed.  We're trying to get better.
>
> David
>
> --
> Sent by an employee of the Qualcomm Innovation Center, Inc.
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
>


-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH] mmc: msm: fix dma usage not to use internal APIs

[Brent DeGraaf]

Russell,

Thanks for your comments. Yes, reverting is OK. The writel's are doing the
controller write after the barrier so even though it'll be a little
slower, it will be correct.

Regarding the "unnecessary" pre-invalidate, yeah, I misspoke.  I meant
unnecessary POST-invalidates for non-speculative cpus.  I just checked and
it looks like someone's already put a "FIXME" in the current 2.6.37 source
for those.

We'll look into how much gains we can expect from the relaxed I/O calls
and decide where to go from there.  Thanks for pointing out the api.

Best regards,
Brent

On Fri, January 21, 2011 1:17 pm, David Brown wrote:
> On Fri, Jan 21 2011, Russell King - ARM Linux wrote:
>
>> On Fri, Jan 21, 2011 at 08:13:59AM -0800, Brent DeGraaf wrote:
>
>> This is one of the *biggest* problems I have with people setting up per-
>> platform mailing lists.  It hides information from the rest of the
>> community
>> and causes idiotic practices and abuses such as has happened here.
>
> I have been requesting, internally, that nothing should be sent
> to the linux-arm-msm mailing list that isn't sent to at least
> linux-arm-kernel, and preferrably linux-kernel as well.  It hasn't
> happened yet, but I will be asking people to resend patches if they only
> send them to the MSM list.
>
> As Daniel pointed out, though, I don't think this particular case was
> even discussed.  We're trying to get better.
>
> David
>
> --
> Sent by an employee of the Qualcomm Innovation Center, Inc.
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
>


-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH] mmc: msm: fix dma usage not to use internal APIs

[Russell King - ARM Linux]

On Fri, Jan 21, 2011 at 01:37:15PM -0800, Brent DeGraaf wrote:
> Russell,
> 
> Thanks for your comments. Yes, reverting is OK. The writel's are doing the
> controller write after the barrier so even though it'll be a little
> slower, it will be correct.
> 
> Regarding the "unnecessary" pre-invalidate, yeah, I misspoke.  I meant
> unnecessary POST-invalidates for non-speculative cpus.  I just checked and
> it looks like someone's already put a "FIXME" in the current 2.6.37 source
> for those.

We don't do post-invalidates either.  We may call into the processor
specific code, but the sum of what they do for pre-ARMv6 CPUs is as
follows:

arch/arm/mm/proc-arm1020e.S:ENTRY(arm1020e_dma_unmap_area)
arch/arm/mm/proc-arm1020e.S-    mov     pc, lr
arch/arm/mm/proc-arm1020e.S-ENDPROC(arm1020e_dma_unmap_area)
--
arch/arm/mm/proc-arm1020.S:ENTRY(arm1020_dma_unmap_area)
arch/arm/mm/proc-arm1020.S-     mov     pc, lr
arch/arm/mm/proc-arm1020.S-ENDPROC(arm1020_dma_unmap_area)
--
arch/arm/mm/proc-arm1022.S:ENTRY(arm1022_dma_unmap_area)
arch/arm/mm/proc-arm1022.S-     mov     pc, lr
arch/arm/mm/proc-arm1022.S-ENDPROC(arm1022_dma_unmap_area)
--
arch/arm/mm/proc-arm1026.S:ENTRY(arm1026_dma_unmap_area)
arch/arm/mm/proc-arm1026.S-     mov     pc, lr
arch/arm/mm/proc-arm1026.S-ENDPROC(arm1026_dma_unmap_area)
--
arch/arm/mm/proc-arm920.S:ENTRY(arm920_dma_unmap_area)
arch/arm/mm/proc-arm920.S-      mov     pc, lr
arch/arm/mm/proc-arm920.S-ENDPROC(arm920_dma_unmap_area)
--
arch/arm/mm/proc-arm922.S:ENTRY(arm922_dma_unmap_area)
arch/arm/mm/proc-arm922.S-      mov     pc, lr
arch/arm/mm/proc-arm922.S-ENDPROC(arm922_dma_unmap_area)
--
arch/arm/mm/proc-arm925.S:ENTRY(arm925_dma_unmap_area)
arch/arm/mm/proc-arm925.S-      mov     pc, lr
arch/arm/mm/proc-arm925.S-ENDPROC(arm925_dma_unmap_area)
--
arch/arm/mm/proc-arm926.S:ENTRY(arm926_dma_unmap_area)
arch/arm/mm/proc-arm926.S-      mov     pc, lr
arch/arm/mm/proc-arm926.S-ENDPROC(arm926_dma_unmap_area)
--
arch/arm/mm/proc-arm940.S:ENTRY(arm940_dma_unmap_area)
arch/arm/mm/proc-arm940.S-      mov     pc, lr
arch/arm/mm/proc-arm940.S-ENDPROC(arm940_dma_unmap_area)
--
arch/arm/mm/proc-arm946.S:ENTRY(arm946_dma_unmap_area)
arch/arm/mm/proc-arm946.S-      mov     pc, lr
arch/arm/mm/proc-arm946.S-ENDPROC(arm946_dma_unmap_area)
--
arch/arm/mm/proc-feroceon.S:ENTRY(feroceon_dma_unmap_area)
arch/arm/mm/proc-feroceon.S-    mov     pc, lr
arch/arm/mm/proc-feroceon.S-ENDPROC(feroceon_dma_unmap_area)
--
arch/arm/mm/proc-mohawk.S:ENTRY(mohawk_dma_unmap_area)
arch/arm/mm/proc-mohawk.S-      mov     pc, lr
arch/arm/mm/proc-mohawk.S-ENDPROC(mohawk_dma_unmap_area)
--
arch/arm/mm/proc-xsc3.S:ENTRY(xsc3_dma_unmap_area)
arch/arm/mm/proc-xsc3.S-        mov     pc, lr
arch/arm/mm/proc-xsc3.S-ENDPROC(xsc3_dma_unmap_area)
--
arch/arm/mm/proc-xscale.S:ENTRY(xscale_dma_unmap_area)
arch/arm/mm/proc-xscale.S-      mov     pc, lr
arch/arm/mm/proc-xscale.S-ENDPROC(xscale_dma_unmap_area)

So we don't do anything for non-speculative prefetching CPUs on unmap.

[PATCH] mmc: msm: fix dma usage not to use internal APIs

[Russell King - ARM Linux]

On Fri, Jan 21, 2011 at 01:37:15PM -0800, Brent DeGraaf wrote:
> Russell,
> 
> Thanks for your comments. Yes, reverting is OK. The writel's are doing the
> controller write after the barrier so even though it'll be a little
> slower, it will be correct.
> 
> Regarding the "unnecessary" pre-invalidate, yeah, I misspoke.  I meant
> unnecessary POST-invalidates for non-speculative cpus.  I just checked and
> it looks like someone's already put a "FIXME" in the current 2.6.37 source
> for those.

We don't do post-invalidates either.  We may call into the processor
specific code, but the sum of what they do for pre-ARMv6 CPUs is as
follows:

arch/arm/mm/proc-arm1020e.S:ENTRY(arm1020e_dma_unmap_area)
arch/arm/mm/proc-arm1020e.S-    mov     pc, lr
arch/arm/mm/proc-arm1020e.S-ENDPROC(arm1020e_dma_unmap_area)
--
arch/arm/mm/proc-arm1020.S:ENTRY(arm1020_dma_unmap_area)
arch/arm/mm/proc-arm1020.S-     mov     pc, lr
arch/arm/mm/proc-arm1020.S-ENDPROC(arm1020_dma_unmap_area)
--
arch/arm/mm/proc-arm1022.S:ENTRY(arm1022_dma_unmap_area)
arch/arm/mm/proc-arm1022.S-     mov     pc, lr
arch/arm/mm/proc-arm1022.S-ENDPROC(arm1022_dma_unmap_area)
--
arch/arm/mm/proc-arm1026.S:ENTRY(arm1026_dma_unmap_area)
arch/arm/mm/proc-arm1026.S-     mov     pc, lr
arch/arm/mm/proc-arm1026.S-ENDPROC(arm1026_dma_unmap_area)
--
arch/arm/mm/proc-arm920.S:ENTRY(arm920_dma_unmap_area)
arch/arm/mm/proc-arm920.S-      mov     pc, lr
arch/arm/mm/proc-arm920.S-ENDPROC(arm920_dma_unmap_area)
--
arch/arm/mm/proc-arm922.S:ENTRY(arm922_dma_unmap_area)
arch/arm/mm/proc-arm922.S-      mov     pc, lr
arch/arm/mm/proc-arm922.S-ENDPROC(arm922_dma_unmap_area)
--
arch/arm/mm/proc-arm925.S:ENTRY(arm925_dma_unmap_area)
arch/arm/mm/proc-arm925.S-      mov     pc, lr
arch/arm/mm/proc-arm925.S-ENDPROC(arm925_dma_unmap_area)
--
arch/arm/mm/proc-arm926.S:ENTRY(arm926_dma_unmap_area)
arch/arm/mm/proc-arm926.S-      mov     pc, lr
arch/arm/mm/proc-arm926.S-ENDPROC(arm926_dma_unmap_area)
--
arch/arm/mm/proc-arm940.S:ENTRY(arm940_dma_unmap_area)
arch/arm/mm/proc-arm940.S-      mov     pc, lr
arch/arm/mm/proc-arm940.S-ENDPROC(arm940_dma_unmap_area)
--
arch/arm/mm/proc-arm946.S:ENTRY(arm946_dma_unmap_area)
arch/arm/mm/proc-arm946.S-      mov     pc, lr
arch/arm/mm/proc-arm946.S-ENDPROC(arm946_dma_unmap_area)
--
arch/arm/mm/proc-feroceon.S:ENTRY(feroceon_dma_unmap_area)
arch/arm/mm/proc-feroceon.S-    mov     pc, lr
arch/arm/mm/proc-feroceon.S-ENDPROC(feroceon_dma_unmap_area)
--
arch/arm/mm/proc-mohawk.S:ENTRY(mohawk_dma_unmap_area)
arch/arm/mm/proc-mohawk.S-      mov     pc, lr
arch/arm/mm/proc-mohawk.S-ENDPROC(mohawk_dma_unmap_area)
--
arch/arm/mm/proc-xsc3.S:ENTRY(xsc3_dma_unmap_area)
arch/arm/mm/proc-xsc3.S-        mov     pc, lr
arch/arm/mm/proc-xsc3.S-ENDPROC(xsc3_dma_unmap_area)
--
arch/arm/mm/proc-xscale.S:ENTRY(xscale_dma_unmap_area)
arch/arm/mm/proc-xscale.S-      mov     pc, lr
arch/arm/mm/proc-xscale.S-ENDPROC(xscale_dma_unmap_area)

So we don't do anything for non-speculative prefetching CPUs on unmap.

Re: [PATCH] mmc: msm: fix dma usage not to use internal APIs

[Daniel Walker]

On Tue, 2011-01-18 at 22:41 +0000, Russell King - ARM Linux wrote:

> +		box->cmd = CMD_MODE_BOX;
> 
> needs to be here.

Yeah, this fixed it.. I'll send out another one.

Daniel

-- 

Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH] mmc: msm: fix dma usage not to use internal APIs

[Daniel Walker]

On Tue, 2011-01-18 at 22:41 +0000, Russell King - ARM Linux wrote:

> +		box->cmd = CMD_MODE_BOX;
> 
> needs to be here.

Yeah, this fixed it.. I'll send out another one.

Daniel

-- 

Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH] mmc: msm: fix dma usage not to use internal APIs

[Russell King - ARM Linux]

On Tue, Jan 18, 2011 at 02:25:18PM -0800, Daniel Walker wrote:
> This doesn't work yet. I tried booting with this patch, and the boot
> stops after detecting the MMC partitions. It won't actually boot off
> the MMC. Without this patch and with the page_to_dma/pfn_to_dma changes
> it does work.
> 
> Not-Signed-off-by: Daniel Walker <dwalker@codeaurora.org>
> ---
>  drivers/mmc/host/msm_sdcc.c |   46 +++++++++++++++++-------------------------
>  1 files changed, 19 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/mmc/host/msm_sdcc.c b/drivers/mmc/host/msm_sdcc.c
> index 5decfd0..1f1c7cb 100644
> --- a/drivers/mmc/host/msm_sdcc.c
> +++ b/drivers/mmc/host/msm_sdcc.c
> @@ -383,14 +383,27 @@ static int msmsdcc_config_dma(struct msmsdcc_host *host, struct mmc_data *data)
>  	host->curr.user_pages = 0;
>  
>  	box = &nc->cmd[0];
> -	for (i = 0; i < host->dma.num_ents; i++) {
> -		box->cmd = CMD_MODE_BOX;

It probably happens because this line got dropped (due to the random
formatting in this file, it makes it harder to read.)

>  
> -	/* Initialize sg dma address */
> -	sg->dma_address = page_to_dma(mmc_dev(host->mmc), sg_page(sg))
> -				+ sg->offset;
> +	/* location of command block must be 64 bit aligned */
> +	BUG_ON(host->dma.cmd_busaddr & 0x07);
>  
> -	if (i == (host->dma.num_ents - 1))
> +	nc->cmdptr = (host->dma.cmd_busaddr >> 3) | CMD_PTR_LP;
> +	host->dma.hdr.cmdptr = DMOV_CMD_PTR_LIST |
> +			       DMOV_CMD_ADDR(host->dma.cmdptr_busaddr);
> +	host->dma.hdr.complete_func = msmsdcc_dma_complete_func;
> +
> +	n = dma_map_sg(mmc_dev(host->mmc), host->dma.sg,
> +			host->dma.num_ents, host->dma.dir);
> +	if (n == 0) {
> +		printk(KERN_ERR "%s: Unable to map in all sg elements\n",
> +			mmc_hostname(host->mmc));
> +		host->dma.sg = NULL;
> +		host->dma.num_ents = 0;
> +		return -ENOMEM;
> +	}
> +
> +	for_each_sg(host->dma.sg, sg, n, i) {

+		box->cmd = CMD_MODE_BOX;

needs to be here.

[PATCH] mmc: msm: fix dma usage not to use internal APIs

[Russell King - ARM Linux]

On Tue, Jan 18, 2011 at 02:25:18PM -0800, Daniel Walker wrote:
> This doesn't work yet. I tried booting with this patch, and the boot
> stops after detecting the MMC partitions. It won't actually boot off
> the MMC. Without this patch and with the page_to_dma/pfn_to_dma changes
> it does work.
> 
> Not-Signed-off-by: Daniel Walker <dwalker@codeaurora.org>
> ---
>  drivers/mmc/host/msm_sdcc.c |   46 +++++++++++++++++-------------------------
>  1 files changed, 19 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/mmc/host/msm_sdcc.c b/drivers/mmc/host/msm_sdcc.c
> index 5decfd0..1f1c7cb 100644
> --- a/drivers/mmc/host/msm_sdcc.c
> +++ b/drivers/mmc/host/msm_sdcc.c
> @@ -383,14 +383,27 @@ static int msmsdcc_config_dma(struct msmsdcc_host *host, struct mmc_data *data)
>  	host->curr.user_pages = 0;
>  
>  	box = &nc->cmd[0];
> -	for (i = 0; i < host->dma.num_ents; i++) {
> -		box->cmd = CMD_MODE_BOX;

It probably happens because this line got dropped (due to the random
formatting in this file, it makes it harder to read.)

>  
> -	/* Initialize sg dma address */
> -	sg->dma_address = page_to_dma(mmc_dev(host->mmc), sg_page(sg))
> -				+ sg->offset;
> +	/* location of command block must be 64 bit aligned */
> +	BUG_ON(host->dma.cmd_busaddr & 0x07);
>  
> -	if (i == (host->dma.num_ents - 1))
> +	nc->cmdptr = (host->dma.cmd_busaddr >> 3) | CMD_PTR_LP;
> +	host->dma.hdr.cmdptr = DMOV_CMD_PTR_LIST |
> +			       DMOV_CMD_ADDR(host->dma.cmdptr_busaddr);
> +	host->dma.hdr.complete_func = msmsdcc_dma_complete_func;
> +
> +	n = dma_map_sg(mmc_dev(host->mmc), host->dma.sg,
> +			host->dma.num_ents, host->dma.dir);
> +	if (n == 0) {
> +		printk(KERN_ERR "%s: Unable to map in all sg elements\n",
> +			mmc_hostname(host->mmc));
> +		host->dma.sg = NULL;
> +		host->dma.num_ents = 0;
> +		return -ENOMEM;
> +	}
> +
> +	for_each_sg(host->dma.sg, sg, n, i) {

+		box->cmd = CMD_MODE_BOX;

needs to be here.

[PATCH] mmc: msm: fix dma usage not to use internal APIs

[Daniel Walker]

This doesn't work yet. I tried booting with this patch, and the boot
stops after detecting the MMC partitions. It won't actually boot off
the MMC. Without this patch and with the page_to_dma/pfn_to_dma changes
it does work.

Not-Signed-off-by: Daniel Walker <dwalker@codeaurora.org>
---
 drivers/mmc/host/msm_sdcc.c |   46 +++++++++++++++++-------------------------
 1 files changed, 19 insertions(+), 27 deletions(-)

diff --git a/drivers/mmc/host/msm_sdcc.c b/drivers/mmc/host/msm_sdcc.c
index 5decfd0..1f1c7cb 100644
--- a/drivers/mmc/host/msm_sdcc.c
+++ b/drivers/mmc/host/msm_sdcc.c
@@ -383,14 +383,27 @@ static int msmsdcc_config_dma(struct msmsdcc_host *host, struct mmc_data *data)
 	host->curr.user_pages = 0;
 
 	box = &nc->cmd[0];
-	for (i = 0; i < host->dma.num_ents; i++) {
-		box->cmd = CMD_MODE_BOX;
 
-	/* Initialize sg dma address */
-	sg->dma_address = page_to_dma(mmc_dev(host->mmc), sg_page(sg))
-				+ sg->offset;
+	/* location of command block must be 64 bit aligned */
+	BUG_ON(host->dma.cmd_busaddr & 0x07);
 
-	if (i == (host->dma.num_ents - 1))
+	nc->cmdptr = (host->dma.cmd_busaddr >> 3) | CMD_PTR_LP;
+	host->dma.hdr.cmdptr = DMOV_CMD_PTR_LIST |
+			       DMOV_CMD_ADDR(host->dma.cmdptr_busaddr);
+	host->dma.hdr.complete_func = msmsdcc_dma_complete_func;
+
+	n = dma_map_sg(mmc_dev(host->mmc), host->dma.sg,
+			host->dma.num_ents, host->dma.dir);
+	if (n == 0) {
+		printk(KERN_ERR "%s: Unable to map in all sg elements\n",
+			mmc_hostname(host->mmc));
+		host->dma.sg = NULL;
+		host->dma.num_ents = 0;
+		return -ENOMEM;
+	}
+
+	for_each_sg(host->dma.sg, sg, n, i) {
+		if (i == n - 1)
 			box->cmd |= CMD_LC;
 		rows = (sg_dma_len(sg) % MCI_FIFOSIZE) ?
 			(sg_dma_len(sg) / MCI_FIFOSIZE) + 1 :
@@ -418,27 +431,6 @@ static int msmsdcc_config_dma(struct msmsdcc_host *host, struct mmc_data *data)
 			box->cmd |= CMD_DST_CRCI(crci);
 		}
 		box++;
-		sg++;
-	}
-
-	/* location of command block must be 64 bit aligned */
-	BUG_ON(host->dma.cmd_busaddr & 0x07);
-
-	nc->cmdptr = (host->dma.cmd_busaddr >> 3) | CMD_PTR_LP;
-	host->dma.hdr.cmdptr = DMOV_CMD_PTR_LIST |
-			       DMOV_CMD_ADDR(host->dma.cmdptr_busaddr);
-	host->dma.hdr.complete_func = msmsdcc_dma_complete_func;
-
-	n = dma_map_sg(mmc_dev(host->mmc), host->dma.sg,
-			host->dma.num_ents, host->dma.dir);
-/* dsb inside dma_map_sg will write nc out to mem as well */
-
-	if (n != host->dma.num_ents) {
-		printk(KERN_ERR "%s: Unable to map in all sg elements\n",
-			mmc_hostname(host->mmc));
-		host->dma.sg = NULL;
-		host->dma.num_ents = 0;
-		return -ENOMEM;
 	}
 
 	return 0;
-- 
1.7.0.4

-- 
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH] mmc: msm: fix dma usage not to use internal APIs

[Daniel Walker]

This doesn't work yet. I tried booting with this patch, and the boot
stops after detecting the MMC partitions. It won't actually boot off
the MMC. Without this patch and with the page_to_dma/pfn_to_dma changes
it does work.

Not-Signed-off-by: Daniel Walker <dwalker@codeaurora.org>
---
 drivers/mmc/host/msm_sdcc.c |   46 +++++++++++++++++-------------------------
 1 files changed, 19 insertions(+), 27 deletions(-)

diff --git a/drivers/mmc/host/msm_sdcc.c b/drivers/mmc/host/msm_sdcc.c
index 5decfd0..1f1c7cb 100644
--- a/drivers/mmc/host/msm_sdcc.c
+++ b/drivers/mmc/host/msm_sdcc.c
@@ -383,14 +383,27 @@ static int msmsdcc_config_dma(struct msmsdcc_host *host, struct mmc_data *data)
 	host->curr.user_pages = 0;
 
 	box = &nc->cmd[0];
-	for (i = 0; i < host->dma.num_ents; i++) {
-		box->cmd = CMD_MODE_BOX;
 
-	/* Initialize sg dma address */
-	sg->dma_address = page_to_dma(mmc_dev(host->mmc), sg_page(sg))
-				+ sg->offset;
+	/* location of command block must be 64 bit aligned */
+	BUG_ON(host->dma.cmd_busaddr & 0x07);
 
-	if (i == (host->dma.num_ents - 1))
+	nc->cmdptr = (host->dma.cmd_busaddr >> 3) | CMD_PTR_LP;
+	host->dma.hdr.cmdptr = DMOV_CMD_PTR_LIST |
+			       DMOV_CMD_ADDR(host->dma.cmdptr_busaddr);
+	host->dma.hdr.complete_func = msmsdcc_dma_complete_func;
+
+	n = dma_map_sg(mmc_dev(host->mmc), host->dma.sg,
+			host->dma.num_ents, host->dma.dir);
+	if (n == 0) {
+		printk(KERN_ERR "%s: Unable to map in all sg elements\n",
+			mmc_hostname(host->mmc));
+		host->dma.sg = NULL;
+		host->dma.num_ents = 0;
+		return -ENOMEM;
+	}
+
+	for_each_sg(host->dma.sg, sg, n, i) {
+		if (i == n - 1)
 			box->cmd |= CMD_LC;
 		rows = (sg_dma_len(sg) % MCI_FIFOSIZE) ?
 			(sg_dma_len(sg) / MCI_FIFOSIZE) + 1 :
@@ -418,27 +431,6 @@ static int msmsdcc_config_dma(struct msmsdcc_host *host, struct mmc_data *data)
 			box->cmd |= CMD_DST_CRCI(crci);
 		}
 		box++;
-		sg++;
-	}
-
-	/* location of command block must be 64 bit aligned */
-	BUG_ON(host->dma.cmd_busaddr & 0x07);
-
-	nc->cmdptr = (host->dma.cmd_busaddr >> 3) | CMD_PTR_LP;
-	host->dma.hdr.cmdptr = DMOV_CMD_PTR_LIST |
-			       DMOV_CMD_ADDR(host->dma.cmdptr_busaddr);
-	host->dma.hdr.complete_func = msmsdcc_dma_complete_func;
-
-	n = dma_map_sg(mmc_dev(host->mmc), host->dma.sg,
-			host->dma.num_ents, host->dma.dir);
-/* dsb inside dma_map_sg will write nc out to mem as well */
-
-	if (n != host->dma.num_ents) {
-		printk(KERN_ERR "%s: Unable to map in all sg elements\n",
-			mmc_hostname(host->mmc));
-		host->dma.sg = NULL;
-		host->dma.num_ents = 0;
-		return -ENOMEM;
 	}
 
 	return 0;
-- 
1.7.0.4

-- 
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.