[PATCH] ads7846: allocate separate cache lines for tx and rx data

[PATCH] ads7846: allocate separate cache lines for tx and rx data

[Alessandro Rubini]

From: Alessandro Rubini <rubini@gnudd.com>

Since the SPI master might use DMA, tx and rx buffers must live on
different cache lines.  Here one cache line is used for all tx and
another for all rx.  This will work whether the SPI master maps/unmaps
one buffer at a time or all together.  The patch only fixes the analog
inputs, as touch screen operation is not affected, although I didn't
check why.  I tested on at91sam9263, where buffers are all mapped
initially and unmapped individually at irq time.

The issue was discussed with Russell King on linux-arm-kernel.

Signed-off-by: Alessandro Rubini <rubini@gnudd.com>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: David Brownell <dbrownell@users.sourceforge.net>
---
 drivers/input/touchscreen/ads7846.c |   53 +++++++++++++++++++++++++----------
 1 files changed, 38 insertions(+), 15 deletions(-)

diff --git a/drivers/input/touchscreen/ads7846.c b/drivers/input/touchscreen/ads7846.c
index ba9d38c..6c940c9 100644
--- a/drivers/input/touchscreen/ads7846.c
+++ b/drivers/input/touchscreen/ads7846.c
@@ -27,6 +27,7 @@
 #include <linux/gpio.h>
 #include <linux/spi/spi.h>
 #include <linux/spi/ads7846.h>
+#include <linux/dma-mapping.h>
 #include <asm/irq.h>
 
 
@@ -196,13 +197,20 @@ struct ads7846 {
  */
 
 struct ser_req {
+	struct spi_message	msg;
+	struct spi_transfer	xfer[6];
+	/* data buffers must live on different cache lines, for DMA access */
+	struct tx_data		*tx;
+	struct rx_data		*rx;
+};
+struct tx_data {
 	u8			ref_on;
 	u8			command;
 	u8			ref_off;
-	u16			scratch;
+};
+struct rx_data {
 	__be16			sample;
-	struct spi_message	msg;
-	struct spi_transfer	xfer[6];
+	__be16			scratch;
 };
 
 static void ads7846_enable(struct ads7846 *ts);
@@ -218,12 +226,27 @@ static int ads7846_read12_ser(struct device *dev, unsigned command)
 {
 	struct spi_device	*spi = to_spi_device(dev);
 	struct ads7846		*ts = dev_get_drvdata(dev);
-	struct ser_req		*req = kzalloc(sizeof *req, GFP_KERNEL);
+	struct ser_req		*req;
+	void			*ptr;
 	int			status;
 	int			use_internal;
+	int			cacheline = dma_get_cache_alignment();
+	int			size;
+
+	/*
+	 * allocate the request and two different cache lines for tx and rx.
+	 * Use a single kmalloc and some simple quasi-constant aritmetics
+	 */
+	size = ALIGN(sizeof *req, cacheline)
+		+ ALIGN(sizeof *req->tx, cacheline)
+		+ ALIGN(sizeof *req->rx, cacheline);
 
-	if (!req)
+	ptr = kzalloc(size, GFP_KERNEL);
+	if (!ptr)
 		return -ENOMEM;
+	req = ptr;
+	req->tx = ptr + ALIGN(sizeof *req, cacheline);
+	req->rx = (void *)req->tx + ALIGN(sizeof *req->tx, cacheline);
 
 	spi_message_init(&req->msg);
 
@@ -232,12 +255,12 @@ static int ads7846_read12_ser(struct device *dev, unsigned command)
 
 	/* maybe turn on internal vREF, and let it settle */
 	if (use_internal) {
-		req->ref_on = REF_ON;
-		req->xfer[0].tx_buf = &req->ref_on;
+		req->tx->ref_on = REF_ON;
+		req->xfer[0].tx_buf = &req->tx->ref_on;
 		req->xfer[0].len = 1;
 		spi_message_add_tail(&req->xfer[0], &req->msg);
 
-		req->xfer[1].rx_buf = &req->scratch;
+		req->xfer[1].rx_buf = &req->rx->scratch;
 		req->xfer[1].len = 2;
 
 		/* for 1uF, settle for 800 usec; no cap, 100 usec.  */
@@ -246,24 +269,24 @@ static int ads7846_read12_ser(struct device *dev, unsigned command)
 	}
 
 	/* take sample */
-	req->command = (u8) command;
-	req->xfer[2].tx_buf = &req->command;
+	req->tx->command = (u8) command;
+	req->xfer[2].tx_buf = &req->tx->command;
 	req->xfer[2].len = 1;
 	spi_message_add_tail(&req->xfer[2], &req->msg);
 
-	req->xfer[3].rx_buf = &req->sample;
+	req->xfer[3].rx_buf = &req->rx->sample;
 	req->xfer[3].len = 2;
 	spi_message_add_tail(&req->xfer[3], &req->msg);
 
 	/* REVISIT:  take a few more samples, and compare ... */
 
 	/* converter in low power mode & enable PENIRQ */
-	req->ref_off = PWRDOWN;
-	req->xfer[4].tx_buf = &req->ref_off;
+	req->tx->ref_off = PWRDOWN;
+	req->xfer[4].tx_buf = &req->tx->ref_off;
 	req->xfer[4].len = 1;
 	spi_message_add_tail(&req->xfer[4], &req->msg);
 
-	req->xfer[5].rx_buf = &req->scratch;
+	req->xfer[5].rx_buf = &req->rx->scratch;
 	req->xfer[5].len = 2;
 	CS_CHANGE(req->xfer[5]);
 	spi_message_add_tail(&req->xfer[5], &req->msg);
@@ -276,7 +299,7 @@ static int ads7846_read12_ser(struct device *dev, unsigned command)
 
 	if (status == 0) {
 		/* on-wire is a must-ignore bit, a BE12 value, then padding */
-		status = be16_to_cpu(req->sample);
+		status = be16_to_cpu(req->rx->sample);
 		status = status >> 3;
 		status &= 0x0fff;
 	}
-- 
1.6.0.2

-------------------------------------------------------------------
List admin: http://lists.arm.linux.org.uk/mailman/listinfo/linux-arm-kernel
FAQ:        http://www.arm.linux.org.uk/mailinglists/faq.php
Etiquette:  http://www.arm.linux.org.uk/mailinglists/etiquette.php

Re: [PATCH] ads7846: allocate separate cache lines for tx and rx data

[David Brownell]

On Wednesday 15 July 2009, Alessandro Rubini wrote:
> From: Alessandro Rubini <rubini@gnudd.com>
> 
> Since the SPI master might use DMA, tx and rx buffers must live on
> different cache lines.

Not true.  Full duplex tranfsers using a single buffer are
explicitly allowed.


> Here one cache line is used for all tx and 
> another for all rx.  This will work whether the SPI master maps/unmaps
> one buffer at a time or all together.  The patch only fixes the analog
> inputs, as touch screen operation is not affected, although I didn't
> check why.  I tested on at91sam9263, where buffers are all mapped
> initially and unmapped individually at irq time.

If that spi_master driver mis-handles this, it's a bug in that
driver.  (And I hope you were using a version of the '9263 chip
which fixed its SPI bugs.  Initial silicon had a SPI controller
which was essentially unusuable in a multitasking environment.)

 
> The issue was discussed with Russell King on linux-arm-kernel.

Gee, but not with the author of that driver or the maintainer of
the SPI framework.   Who could have pointed out instantly where
the true bug resides.

 
> 
> Signed-off-by: Alessandro Rubini <rubini@gnudd.com>
> Cc: Russell King <linux@arm.linux.org.uk>
> Cc: David Brownell <dbrownell@users.sourceforge.net>
> ---
>  drivers/input/touchscreen/ads7846.c |   53 +++++++++++++++++++++++++----------
>  1 files changed, 38 insertions(+), 15 deletions(-)
> 

Re: [PATCH] ads7846: allocate separate cache lines for tx and rx data

[Alessandro Rubini]

>> Since the SPI master might use DMA, tx and rx buffers must live on
>> different cache lines.
> 
> Not true.  Full duplex tranfsers using a single buffer are
> explicitly allowed.

Ok, thanks.

> If that spi_master driver mis-handles this, it's a bug in that
> driver.

Well, the driver receives one spi message made of 4 or 6 transfers.
It does one at a time, shouldn't it?  If the transfer description is
intermixed with the data buffer, we fetch a line from ram with
not-yet-filled input buffers. So I get invalid data from the analog
inputs -- usually zero, as the structure is kzalloced.

>> The issue was discussed with Russell King on linux-arm-kernel.
> 
> Gee, but not with the author of that driver or the maintainer of
> the SPI framework.   Who could have pointed out instantly where
> the true bug resides.

So, where is it? I don't get it I'm sorry.

ps: yes, it's a revB silicon.

/alessandro

Re: [PATCH] ads7846: allocate separate cache lines for tx and rx data

[Russell King - ARM Linux]

On Wed, Jul 15, 2009 at 09:33:26PM +0200, Alessandro Rubini wrote:
> >> The issue was discussed with Russell King on linux-arm-kernel.
> > 
> > Gee, but not with the author of that driver or the maintainer of
> > the SPI framework.   Who could have pointed out instantly where
> > the true bug resides.
> 
> So, where is it? I don't get it I'm sorry.

Clearly David has no knowledge of DMA cache line handing and has not
read the DMA API document.  I really suggest that you ignore him until
he gets a fucking clue.

Re: [PATCH] ads7846: allocate separate cache lines for tx and rx data

[Russell King - ARM Linux]

On Thu, Jul 16, 2009 at 08:15:29AM +0100, Russell King - ARM Linux wrote:
> On Wed, Jul 15, 2009 at 09:33:26PM +0200, Alessandro Rubini wrote:
> > >> The issue was discussed with Russell King on linux-arm-kernel.
> > > 
> > > Gee, but not with the author of that driver or the maintainer of
> > > the SPI framework.   Who could have pointed out instantly where
> > > the true bug resides.
> > 
> > So, where is it? I don't get it I'm sorry.
> 
> Clearly David has no knowledge of DMA cache line handing and has not
> read the DMA API document.  I really suggest that you ignore him until
> he gets a fucking clue.

... and until that happens, I suggest you remove the ADS7846 chip from
your hardware; as that DMA stuff in the driver currently stands, it
can NEVER work on ARM or any other DMA cache incoherent CPU which uses
DMA for SPI transfers.

Re: [PATCH] ads7846: allocate separate cache lines for tx and rx data

[David Brownell]

On Thursday 16 July 2009, Russell King - ARM Linux wrote:
> Clearly David has no knowledge of DMA cache line handing and has not
> read the DMA API document.  I really suggest that you ignore him until
> he gets a fucking clue.

Clearly common courtesy is too much for Russell.  I suggest you
ignore him until he learns to restrain his baser impulses ...
his personal attacks have no place in technical discussions.
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ads7846: allocate separate cache lines for tx and rx data

[David Brownell]

On Wednesday 15 July 2009, Alessandro Rubini wrote:
> >> Since the SPI master might use DMA, tx and rx buffers must live on
> >> different cache lines.
> > 
> > Not true.  Full duplex tranfsers using a single buffer are
> > explicitly allowed.
> 
> Ok, thanks.
> 
> > If that spi_master driver mis-handles this, it's a bug in that
> > driver.
> 
> Well, the driver receives one spi message made of 4 or 6 transfers.
> It does one at a time, shouldn't it?

Yes, where each transfer may be full or half duplex.


> If the transfer description is 
> intermixed with the data buffer, we fetch a line from ram with
> not-yet-filled input buffers. So I get invalid data from the analog
> inputs -- usually zero, as the structure is kzalloced.

If you're referring to the way the spi_message and spi_transfer
structs sit in the same cache lines as the data buffers, that's
something that should get fixed.  It started out that way since
none of the *initial* configurations used DMA, and the driver was
evolved from existing code.  Sometime later this was noted to be
a bug when used with DMA...

It seems that e8f462d202026d8e99f553ed5a09422321226ac9 wasn't a
complete fix ... this explains why the touchscreen behaves but
not the ADC inputs (as you noted).

Note that this issue is unrelated to full duplex DMA support.
Full duplex DMA is no problem.  It's not at all common, but that's
exactly what DMA_BIDIRECTIONAL means.  If you look at atmel_spi
you will notice that it maps the TX first, flushing caches; then
the RX next.  The other way around would break; some drivers
got that wrong, and had to be fixed last year sometime.

 
> >> The issue was discussed with Russell King on linux-arm-kernel.
> > 
> > Gee, but not with the author of that driver or the maintainer of
> > the SPI framework.   Who could have pointed out instantly where
> > the true bug resides.
> 
> So, where is it? I don't get it I'm sorry.

See above; this something an earlier patch didn't quite cover.


> ps: yes, it's a revB silicon.

Good.  That SPI bug in revA made it pretty useless for SPI stuff.
Bitbanging works annoyingly slowly!

- Dave

Re: [PATCH] ads7846: allocate separate cache lines for tx and rx data

[David Brownell]

On Thursday 16 July 2009, Russell King - ARM Linux wrote:
> ... and until that happens, I suggest you remove the ADS7846 chip from
> your hardware; as that DMA stuff in the driver currently stands, it
> can NEVER work on ARM or any other DMA cache incoherent CPU which uses
> DMA for SPI transfers.

Wrong again.  There's a localized issue, easily enough fixed.
As noted by Alessandro, earlier:  the touchscreen logic does
not have this issue.  Only the ADC logic.

- Dave

p.s. Note by the way that L-A-K is only getting Russell's
     flamage, not the responses disproving his claims.

Re: [PATCH] ads7846: allocate separate cache lines for tx and rx data

[Alessandro Rubini]

> If you're referring to the way the spi_message and spi_transfer
> structs sit in the same cache lines as the data buffers, that's
> something that should get fixed.

Yes, that's what my patch did. The "strange" part is using a single
malloc instead of three (re-reading your message, the full-duplex
isn't involved here, so I'm not sure tx and rx data can safely be
merged in a cache line).

> It seems that e8f462d202026d8e99f553ed5a09422321226ac9 wasn't a
> complete fix ... this explains why the touchscreen behaves but
> not the ADC inputs (as you noted).

Yes, basically I did the same split of data from control.

> Note that this issue is unrelated to full duplex DMA support.

Yes, that's right. But full duplex is not involved here, it's
just 2 or 3 rounds of "one byte tx then two bytes rx".

/alessandro

Re: [PATCH] ads7846: allocate separate cache lines for tx and rx data

[David Brownell]

On Thursday 16 July 2009, Alessandro Rubini wrote:
> > Note that this issue is unrelated to full duplex DMA support.
> 
> Yes, that's right. But full duplex is not involved here, it's
> just 2 or 3 rounds of "one byte tx then two bytes rx".

Then your patch description was *seriously* misleading:

> > > Since the SPI master might use DMA, tx and rx buffers must live on
> > > different cache lines. ...

I'll have another look at that patch to see if it could make
sense after I discard the description.

- dave

Re: [PATCH] ads7846: allocate separate cache lines for tx and rx data

[Marek Vasut]

Dne Čt 16. července 2009 09:28:04 David Brownell napsal(a):
> On Thursday 16 July 2009, Russell King - ARM Linux wrote:
> > Clearly David has no knowledge of DMA cache line handing and has not
> > read the DMA API document.  I really suggest that you ignore him until
> > he gets a fucking clue.
>
> Clearly common courtesy is too much for Russell.  I suggest you
> ignore him until he learns to restrain his baser impulses ...
> his personal attacks have no place in technical discussions.

It's very interesting. I experienced no problems when it came to communicating 
with him (which I cant say about you though).
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ads7846: allocate separate cache lines for tx and rx data

[David Brownell]

On Thursday 16 July 2009, Marek Vasut wrote:
> > Clearly common courtesy is too much for Russell.  I suggest you
> > ignore him until he learns to restrain his baser impulses ...
> > his personal attacks have no place in technical discussions.
> 
> It's very interesting. I experienced no problems when it came to communicating 
> with him (which I cant say about you though).

Yeah, I don't know why he chooses to attack like that either.
It wasn't provoked.
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ads7846: allocate separate cache lines for tx and rx data

[Russell King - ARM Linux]

On Thu, Jul 16, 2009 at 12:00:00PM +0200, Marek Vasut wrote:
> Dne Čt 16. července 2009 09:28:04 David Brownell napsal(a):
> > On Thursday 16 July 2009, Russell King - ARM Linux wrote:
> > > Clearly David has no knowledge of DMA cache line handing and has not
> > > read the DMA API document.  I really suggest that you ignore him until
> > > he gets a fucking clue.
> >
> > Clearly common courtesy is too much for Russell.  I suggest you
> > ignore him until he learns to restrain his baser impulses ...
> > his personal attacks have no place in technical discussions.
> 
> It's very interesting. I experienced no problems when it came to
> communicating with him (which I cant say about you though).

David and myself have a history of communication problems, to the
extent that I try to avoid reading his witterings.

However, the observant will notice that David has cut out the part
I quoted which elicited my response which I found offensive.

It would also be nice that if David knows the answer to something,
he doesn't spout unhelpful claptrap like "Who could have pointed out
instantly where the true bug resides" and instead be useful and
state the answer.

But, I guess such things are too much for some people.
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html