[PATCH 3/6] spi: bcm2835: fill FIFO before enabling interrupts to

[PATCH 3/6] spi: bcm2835: fill FIFO before enabling interrupts to

[Martin Sperl]

 reduce interrupts/message

To reduce the number of interrupts/message we fill the FIFO before
enabling interrupts - for short messages this reduces the interrupt count
from 2 to 1 interrupt.

There have been rare cases where short (<200ns) chip-select switches with
native CS have been observed during such operation, this is why this
optimization is only enabled for GPIO-CS.

Signed-off-by: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
Tested-by: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
---
 drivers/spi/spi-bcm2835.c |   16 ++++++++++++++++
 1 file changed, 16 insertions(+)

Applies against spi - topic/bcm2835

diff --git a/drivers/spi/spi-bcm2835.c b/drivers/spi/spi-bcm2835.c
index 72d4525..adf157b 100644
--- a/drivers/spi/spi-bcm2835.c
+++ b/drivers/spi/spi-bcm2835.c
@@ -203,6 +203,22 @@ static int bcm2835_spi_transfer_one(struct spi_master *master,
 	bs->tx_len = tfr->len;
 	bs->rx_len = tfr->len;
 
+	/* fill in fifo if we have gpio-cs
+	 * note that there have been rare events where the native-CS
+	 * flapped for <1us which may change the behaviour
+	 * with gpio-cs this does not happen, so it is implemented
+	 * only for this case
+	 */
+	if (gpio_is_valid(spi->cs_gpio)) {
+		/* enable HW block, but without interrupts enabled
+		 * this would triggern an immediate interrupt
+		 */
+		bcm2835_wr(bs, BCM2835_SPI_CS,
+			   cs | BCM2835_SPI_CS_TA);
+		/* fill in tx fifo as much as possible */
+		bcm2835_wr_fifo(bs);
+	}
+
 	/*
 	 * Enable the HW block. This will immediately trigger a DONE (TX
 	 * empty) interrupt, upon which we will fill the TX FIFO with the
-- 
1.7.10.4

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

Re: [PATCH 3/6] spi: bcm2835: fill FIFO before enabling interrupts to

[Mark Brown]

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

On Sun, Mar 29, 2015 at 04:03:25PM +0200, Martin Sperl wrote:
>  reduce interrupts/message

Applied, thanks.  There's something wrong with the way you're sending
patches - long subject lines are being wrapped into the body of the
patch.  Please check this, the easiest thing is most likely to be to use
git format-patch and git send-email to send your patches as that's
pretty much zero effort.

Please while looking at this also look into threading your patches into
a single thread per series, this makes things easier to work with.

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

Re: [PATCH 3/6] spi: bcm2835: fill FIFO before enabling interrupts to

[Stephen Warren]

On 03/29/2015 08:03 AM, Martin Sperl wrote:
>  reduce interrupts/message
> 
> To reduce the number of interrupts/message we fill the FIFO before
> enabling interrupts - for short messages this reduces the interrupt count
> from 2 to 1 interrupt.
> 
> There have been rare cases where short (<200ns) chip-select switches with
> native CS have been observed during such operation, this is why this
> optimization is only enabled for GPIO-CS.

> diff --git a/drivers/spi/spi-bcm2835.c b/drivers/spi/spi-bcm2835.c

> +	/* fill in fifo if we have gpio-cs
> +	 * note that there have been rare events where the native-CS
> +	 * flapped for <1us which may change the behaviour
> +	 * with gpio-cs this does not happen, so it is implemented
> +	 * only for this case
> +	 */
> +	if (gpio_is_valid(spi->cs_gpio)) {
> +		/* enable HW block, but without interrupts enabled
> +		 * this would triggern an immediate interrupt
> +		 */
> +		bcm2835_wr(bs, BCM2835_SPI_CS,
> +			   cs | BCM2835_SPI_CS_TA);
> +		/* fill in tx fifo as much as possible */
> +		bcm2835_wr_fifo(bs);
> +	}

I can understand perfectly why the code fills the FIFO before enabling
interrupts; it avoids having to immediately service an interrupt simply
to fill the FIFO.

However, I'm not sure why this is in any way related to whether the
chip-select GPIO is valid. Surely we always want to do this? How does
the mechanism used to control chip selects influence whether we want to
pre-fill the FIFO?
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/6] spi: bcm2835: fill FIFO before enabling interrupts to

[Mark Brown]

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

On Mon, Mar 30, 2015 at 09:16:21PM -0600, Stephen Warren wrote:
> On 03/29/2015 08:03 AM, Martin Sperl wrote:

> > There have been rare cases where short (<200ns) chip-select switches with
> > native CS have been observed during such operation, this is why this
> > optimization is only enabled for GPIO-CS.

> > +	/* fill in fifo if we have gpio-cs
> > +	 * note that there have been rare events where the native-CS
> > +	 * flapped for <1us which may change the behaviour
> > +	 * with gpio-cs this does not happen, so it is implemented
> > +	 * only for this case
> > +	 */

> However, I'm not sure why this is in any way related to whether the
> chip-select GPIO is valid. Surely we always want to do this? How does
> the mechanism used to control chip selects influence whether we want to
> pre-fill the FIFO?

I think both the comment and the commit message answer that question -
something triggers spurious chip select changes?

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

Re: [PATCH 3/6] spi: bcm2835: fill FIFO before enabling interrupts to

[Martin Sperl]

> On 31.03.2015, at 05:16, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
> 
> I can understand perfectly why the code fills the FIFO before enabling
> interrupts; it avoids having to immediately service an interrupt simply
> to fill the FIFO.
> 
> However, I'm not sure why this is in any way related to whether the
> chip-select GPIO is valid. Surely we always want to do this? How does
> the mechanism used to control chip selects influence whether we want to
> pre-fill the FIFO?

During the time I was building a DMA only driver I saw "rare" glitches
in the CS line when using native CS. The "glitch" is that the CS drops
to inactive for a short period of time - typically 1 sample length at
10MHz sample rate, so <0.1us.

These "glitches" also once have been observed with the current driver
when using native-CS, so I think it is prudent to avoid native-CS when
enabling this optimization.

Hence this limitation or maybe even the full move to GPIO-CS for all.
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/6] spi: bcm2835: fill FIFO before enabling interrupts to

[Stephen Warren]

On 03/30/2015 11:25 PM, Mark Brown wrote:
> On Mon, Mar 30, 2015 at 09:16:21PM -0600, Stephen Warren wrote:
>> On 03/29/2015 08:03 AM, Martin Sperl wrote:
> 
>>> There have been rare cases where short (<200ns) chip-select
>>> switches with native CS have been observed during such
>>> operation, this is why this optimization is only enabled for
>>> GPIO-CS.
> 
>>> +	/* fill in fifo if we have gpio-cs +	 * note that there have
>>> been rare events where the native-CS +	 * flapped for <1us
>>> which may change the behaviour +	 * with gpio-cs this does not
>>> happen, so it is implemented +	 * only for this case +	 */
> 
>> However, I'm not sure why this is in any way related to whether
>> the chip-select GPIO is valid. Surely we always want to do this?
>> How does the mechanism used to control chip selects influence
>> whether we want to pre-fill the FIFO?
> 
> I think both the comment and the commit message answer that
> question - something triggers spurious chip select changes?

I must admit I don't feel either the commit message or the comment
explain the situation. They certainly state that there are glitches in
the "native CS" case, but that doesn't *explain* them. It seems more
likely the under-filling the FIFO would cause periods where the FIFO
was empty which would be aout the only case I could naively think of
for the CS to be de-asserted. More investigation would be useful.

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

Re: [PATCH 3/6] spi: bcm2835: fill FIFO before enabling interrupts to

[Mark Brown]

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

On Sat, Apr 04, 2015 at 07:49:41PM -0600, Stephen Warren wrote:
> On 03/30/2015 11:25 PM, Mark Brown wrote:
> > On Mon, Mar 30, 2015 at 09:16:21PM -0600, Stephen Warren wrote:
> >> On 03/29/2015 08:03 AM, Martin Sperl wrote:

> >>> There have been rare cases where short (<200ns) chip-select
> >>> switches with native CS have been observed during such
> >>> operation, this is why this optimization is only enabled for
> >>> GPIO-CS.

> >> However, I'm not sure why this is in any way related to whether
> >> the chip-select GPIO is valid. Surely we always want to do this?
> >> How does the mechanism used to control chip selects influence
> >> whether we want to pre-fill the FIFO?

> > I think both the comment and the commit message answer that
> > question - something triggers spurious chip select changes?

> I must admit I don't feel either the commit message or the comment
> explain the situation. They certainly state that there are glitches in
> the "native CS" case, but that doesn't *explain* them. It seems more
> likely the under-filling the FIFO would cause periods where the FIFO
> was empty which would be aout the only case I could naively think of
> for the CS to be de-asserted. More investigation would be useful.

Right, and I have to say I do suspect that the underlying thing is that
the FIFO is underrunning, but as far as the optimization is concerned
that's a separate thing.  The reason this isn't enabled for native chip
selects is that it's not working, the reason it's not working is
something that should indeed probably be investigated.

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

Re: [PATCH 3/6] spi: bcm2835: fill FIFO before enabling interrupts to

[Martin Sperl]

> On 06.04.2015, at 19:21, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> Right, and I have to say I do suspect that the underlying thing is that
> the FIFO is underrunning, but as far as the optimization is concerned
> that's a separate thing.  The reason this isn't enabled for native chip
> selects is that it's not working, the reason it's not working is
> something that should indeed probably be investigated.
Actually it happens exactly when setting the CS-register with the
interrupt flags enabled - typically observed in the middle of a transmit
of a byte the CS jumps, but the clock and data continue the transfers
correctly

As the CS register contains the interrupt flags as well as the 
control for the native-chip-selects this is impacting the chip select
lines in native mode.

I have never seen a glitch on the CLK or MISO/MOSI line in my logic
analyzer only CS jumps in the order of 1 sample in the logic analyzer
- @20MHz sample rate, so in the order of 50ns, probably less...

On top that happens only under some rare circumstances and can get rarely
observed - lots of samples (and memory) are needed to catch that one,
which typically results in a message in dmesg due to some unexpected
response crc checksum errors stalled processing.

This is from my experience from over a year ago with a fully DMA
pipelined driver that did all the programming of SPI in DMA as well and
there were some reports of people seeing "glitches" before I moved to
GPIO-CS (equivalents)

See also:
http://www.raspberrypi.org/forums/viewtopic.php?f=44&t=19489&start=125#p451817
for some observations mostly related to CLEAR_TX/CLEAR_RX that also
de-assert CS for short periods.

I also saw it _once_ in one of my traces when using native_cs 2-3 weeks
ago, but unfortunately I did not keep the capture...

Hence the requirement to use of gpio-cs when running this optimization,
as then the CS is not managed by the HW itself.

I have been filling (dd if=/dev/zero) a 2GB SD card repeatedly (more
than 9 times) for the last  12 hours and I have seen no issues -
besides the spi bus being occupied and spi_bus_locked for 17ms for
the data-transfers, which obviously impacts packet-reception (packet
loss) on CAN and ethernet network - but that is expected due to this
heavy load and long transfers.

Note that this test was executed using the latest patches I have sent
running at 16k interrupts/s and 4400 context-switches/second at 4MHz 
SPI-bus-speed for the SD-card.
The transfer of 1.6GB to the SD-card (filling up the partition) took
about 4350 seconds or 366kB/s - 3600s would be the ideal situation
without overheads, latencies or similar.


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

Re: [PATCH 3/6] spi: bcm2835: fill FIFO before enabling interrupts to

[Martin Sperl]

Now remembering a bit more from what I have observed:
> On 07.04.2015, at 08:25, Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org> wrote:
> 
> See also:
> http://www.raspberrypi.org/forums/viewtopic.php?f=44&t=19489&start=125#p451817
> for some observations mostly related to CLEAR_TX/CLEAR_RX that also
> de-assert CS for short periods.

Note that this TX/RX reset with native CS is also inhibiting the use
of DMA for any transfers not a multiple of 4 (DMA transfers data into
the FIFO in words not bytes). Because after those transfers we have
to reset the FIFO or the remaining bytes still in the FIFO will get
shifted out before a subsequent transfer.

So for any DMA enabled driver we need this kind of gpio-cs to avoid
this CS-glitch situation unless we want a "DMA only on multiple of 4"
with native-cs situation...

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

Re: [PATCH 3/6] spi: bcm2835: fill FIFO before enabling interrupts to

[Stephen Warren]

On 04/07/2015 12:25 AM, Martin Sperl wrote:
>
>> On 06.04.2015, at 19:21, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>> Right, and I have to say I do suspect that the underlying thing is that
>> the FIFO is underrunning, but as far as the optimization is concerned
>> that's a separate thing.  The reason this isn't enabled for native chip
>> selects is that it's not working, the reason it's not working is
>> something that should indeed probably be investigated.
 >
> Actually it happens exactly when setting the CS-register with the
> interrupt flags enabled - typically observed in the middle of a transmit
> of a byte the CS jumps, but the clock and data continue the transfers
> correctly
>
> As the CS register contains the interrupt flags as well as the
> control for the native-chip-selects this is impacting the chip select
> lines in native mode.

Is the driver simply programming the HW incorrectly then? I would expect 
the driver to do something roughly like:

* Set up the HW to execute the transaction; everything except enabling 
IRQs and telling the HW to "go"
* Clear stale IRQ status (perhaps do this right at the start)
* Enable IRQs
* Tell the HW to "go"

... then not touch any CS-related register for the entire transfer. 
There shouldn't be a need to enable/disable IRQs during the transfer; 
just leave them enabled the entire time, until all bytes have been 
transferred.

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

Re: [PATCH 3/6] spi: bcm2835: fill FIFO before enabling interrupts to

[Mark Brown]

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

On Tue, Apr 07, 2015 at 09:39:03AM -0600, Stephen Warren wrote:
> On 04/07/2015 12:25 AM, Martin Sperl wrote:

> >As the CS register contains the interrupt flags as well as the
> >control for the native-chip-selects this is impacting the chip select
> >lines in native mode.

> Is the driver simply programming the HW incorrectly then? I would expect the
> driver to do something roughly like:

> * Set up the HW to execute the transaction; everything except enabling IRQs
> and telling the HW to "go"
> * Clear stale IRQ status (perhaps do this right at the start)
> * Enable IRQs
> * Tell the HW to "go"

> ... then not touch any CS-related register for the entire transfer. There
> shouldn't be a need to enable/disable IRQs during the transfer; just leave
> them enabled the entire time, until all bytes have been transferred.

We will need to make sure /CS is kept asserted between transfers in a
message too.

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

Re: [PATCH 3/6] spi: bcm2835: fill FIFO before enabling interrupts to

[Martin Sperl]

> On 07.04.2015, at 17:39, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
> 
> Is the driver simply programming the HW incorrectly then? I would expect the driver to do something roughly like:
> 
> * Set up the HW to execute the transaction; everything except enabling IRQs and telling the HW to "go"
> * Clear stale IRQ status (perhaps do this right at the start)
> * Enable IRQs
> * Tell the HW to "go"

no it actually works differently:
* set TA and some of the other flags (POL,...) in CS-REG, which activates
  the SPI-block and as soon as there is data it will start the transfer.
  if you set the IRQ flags here, then IRQ will trigger as soon as the write
  hits the HW-block, as the fifo is empty

This is what the driver does right in 4.0rcX - it also sets the interrupt
flags.

But the problem is that it takes typcially 20us for the ISR to get REALLY
executed, which means an unecessary delay of 20us before the transfer
really starts.

The prefill approach instead:
* sets TA and some other flags, in CS,but leaves Interrupts disabled
* fills in FIFO which initiates the transfer
* sets the same values in CS-REG as above but now also with IRQ enabled

At this point in time there is a slight chance that CS will toggle for <1us
when using native CS - this does not happen with gpio-CS for obvious reasons.

Similar for the situation where you request a transfer of 13 Bytes via DMA.
You do this by filling in SPI-LEN with 13 to tell the engine to only shift
13 bytes out even if DMA fills in 16 bytes. When this transfer is finished
then there are still 3 bytes in the FIFO, so you have to clean that by 
setting CLEAR_TX/CLEAR_RX in the CS-register. 
If you set only those 2 bit without modifying the others, then there is 
again a chance that native-CS get toggled for a short period of time.

This is obviously not ideal when you have to keep CS low for the next
spi_transfer.

The only way around this was to actually cheat by using only CS2, and
modifying the CSPOL0 and CSPOL1 to do what I want because the CS only
seems to toggle for the "active" CS (as per bits 0:1 in CS).

But that is essentially the same as using cs-gpio, but just uses a
different set of registers.

I guess that there is some sort of logic in the HW that re-evaluates the
state of the native-chip-selects whenever CS-reg is written. Even for the
case where there is no change there seems to be a short period of time
when the CS is not driven (high or low), which, together with some pull-ups,
is pulling those lines high - and this is what we are seeing in some rare
cases.

So any write to the CS-register can influence native chip selects but this
only happens in rare cases typically showing after several hours of
repeated spi-transfers (in my case after 20M CAN messages received and about
100M SPI messages).

Using GPIO-cs solves these situations by not being influenced by the
SPI-hardware in any way in the first place.

Actually - if I think of it - even with the current driver in 4.0rcX
which is configuring CS-reg on every spi_transfer in a spi_message
could trigger the same behavior as well.

But as spi_messages with multiple spi_transfers are rarely used
and as the issue itself is only happening only with a low probability
and some devices not detecting cs changes that are below a certain time
the likleyhood of such a situation being detected are minimal and
would typically get attributed to other factors...

Hope that this answers the question and summarizes the observations
that i have made and why we have to move to gpio-cs for those
optimizations to become active...

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

Re: [PATCH 3/6] spi: bcm2835: fill FIFO before enabling interrupts to

[Mark Brown]

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

On Tue, Apr 07, 2015 at 07:05:01PM +0200, Martin Sperl wrote:

> But as spi_messages with multiple spi_transfers are rarely used
> and as the issue itself is only happening only with a low probability
> and some devices not detecting cs changes that are below a certain time
> the likleyhood of such a situation being detected are minimal and
> would typically get attributed to other factors...

For your workload perhaps.  For some workloads they are very common
(register reads are the most obvious example, they are typically a write
transfer to provide the register address followed by a read transfer for
the data).

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

Re: [PATCH 3/6] spi: bcm2835: fill FIFO before enabling interrupts to

[Martin Sperl]

> On 07.04.2015, at 19:40, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> 
> For your workload perhaps.  For some workloads they are very common
> (register reads are the most obvious example, they are typically a write
> transfer to provide the register address followed by a read transfer for
> the data).
Which spi_write_then_read collapses into a single transfer.

Still I try to run now 4 distinct devices for my testing and
I believe the enc28j60 is one of those devices that does that
and I have not seen an issue there during typical loads...
Obviously there are some devices that are easier to load than
others to trigger such a situation.

It really just shows when running some transfers a lot of times...

So the move to gpio-cs should be on our priority list.

I will come up with a generic approach that allows also to set native
CS if needed... (as sketched in an earlier mail)

Martin

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

Re: [PATCH 3/6] spi: bcm2835: fill FIFO before enabling interrupts to

[Mark Brown]

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

On Tue, Apr 07, 2015 at 08:18:57PM +0200, Martin Sperl wrote:
> > On 07.04.2015, at 19:40, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:

> > For your workload perhaps.  For some workloads they are very common
> > (register reads are the most obvious example, they are typically a write
> > transfer to provide the register address followed by a read transfer for
> > the data).

> Which spi_write_then_read collapses into a single transfer.

No it doesn't.  It creates a single message with two transfers (which is
what I'd expect any user doing this to do, there are others that don't
use the SPI helper APIs).

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

Re: [PATCH 3/6] spi: bcm2835: fill FIFO before enabling interrupts to

[Martin Sperl]

> On 07.04.2015, at 20:23, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> 
> No it doesn't.  It creates a single message with two transfers (which is
> what I'd expect any user doing this to do, there are others that don't
> use the SPI helper APIs).
Well after rereading it now - it does copying, so I was making a naiv
assumption that it would do a single transfer, but then there is the
SPI_3WIRE case that is special.

In principle it could run in a single transfer at the cost of some
memory (64 vs. 32 bytes) for anything that is not SPI_3WIRE. But then
spi_transfer is also in the order of 32 bytes....

This definitely would speed up things for a "typical" interrupt-driven
driver implementation using the transfer_one interface, as it would avoid
one interrupt and 2 context switches.

On the other hand this could also get optimized inside the framework at
the cost of copying twice and we would optimize other cases as well.
But that is some other project...
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/6] spi: bcm2835: fill FIFO before enabling interrupts to

[Stephen Warren]

On 04/07/2015 11:05 AM, Martin Sperl wrote:
> 
>> On 07.04.2015, at 17:39, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
>>
>> Is the driver simply programming the HW incorrectly then? I would expect the driver to do something roughly like:
>>
>> * Set up the HW to execute the transaction; everything except enabling IRQs and telling the HW to "go"
>> * Clear stale IRQ status (perhaps do this right at the start)
>> * Enable IRQs
>> * Tell the HW to "go"
> 
> no it actually works differently:
> * set TA and some of the other flags (POL,...) in CS-REG, which activates
>   the SPI-block and as soon as there is data it will start the transfer.
>   if you set the IRQ flags here, then IRQ will trigger as soon as the write
>   hits the HW-block, as the fifo is empty
>
> This is what the driver does right in 4.0rcX - it also sets the interrupt
> flags.
> 
> But the problem is that it takes typcially 20us for the ISR to get REALLY
> executed, which means an unecessary delay of 20us before the transfer
> really starts.
> 
> The prefill approach instead:
> * sets TA and some other flags, in CS,but leaves Interrupts disabled
> * fills in FIFO which initiates the transfer
> * sets the same values in CS-REG as above but now also with IRQ enabled

Can you fill the FIFO as step 1? That way, you could presumably write to
that CS-REG just once, with all the desired bits set in one go.

> At this point in time there is a slight chance that CS will toggle for <1us
> when using native CS - this does not happen with gpio-CS for obvious reasons.
> 
> Similar for the situation where you request a transfer of 13 Bytes via DMA.
> You do this by filling in SPI-LEN with 13 to tell the engine to only shift
> 13 bytes out even if DMA fills in 16 bytes.

That sounds pretty typical for a 32-bit/dword-wide FIFO containing 8-bit
data. Surely DMA isn't relevant here; every dword write will put 4 bytes
into the FIFO irrespective of whether the write comes from the CPU or a
DMA engine. Generally, the HW will pull dword-sized values out of the
FIFO, and ignore any extra bytes that are packed into the dword beyond
whatever the programmed transfer length is. Doesn't the bcm283x SPI HW
do that too?

> When this transfer is finished
> then there are still 3 bytes in the FIFO, so you have to clean that by 
> setting CLEAR_TX/CLEAR_RX in the CS-register. 

That sounds pretty unusual. Have you tried not cleaning out the FIFO and
seeing if the next transfer does in fact only transfer the data you
write next, not the extra/stale bytes from the previous transfer?
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/6] spi: bcm2835: fill FIFO before enabling interrupts to

[Martin Sperl]

> On 08.04.2015, at 04:01, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
> 
> Can you fill the FIFO as step 1? That way, you could presumably write to
> that CS-REG just once, with all the desired bits set in one go.
Actually the "manual" says that you first need to enable TA - even for a
polling driver and then you can fill the FIFO.

But I have also tried it - it does not work, as the TA flag not only 
initiates the transfer but also activates/deactivates the whole SPI block
(and probably also all the registers). So the data filled into the FIFOs
is lost when TA is inactive.

> That sounds pretty typical for a 32-bit/dword-wide FIFO containing 8-bit
> data. Surely DMA isn't relevant here; every dword write will put 4 bytes
> into the FIFO irrespective of whether the write comes from the CPU or a
> DMA engine. Generally, the HW will pull dword-sized values out of the
> FIFO, and ignore any extra bytes that are packed into the dword beyond
> whatever the programmed transfer length is. Doesn't the bcm283x SPI HW
> do that too?
As the HW can do LoSSI (=9 bit) with the corresponding "command" parsing
where some commands trigger 8 bit reads, others 24 and others 32 bit
reads, it seems as if it reads as many byte from the fifo as necessary
for the specific transfer.

> That sounds pretty unusual. Have you tried not cleaning out the FIFO and
> seeing if the next transfer does in fact only transfer the data you
> write next, not the extra/stale bytes from the previous transfer?
I remember having tried this more than a year ago, but then you get the
"missing" data from the last word transfer (when using DMA).

As I am planning on implementing DMA I guess I will get there.
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/6] spi: bcm2835: fill FIFO before enabling interrupts to

[Martin Sperl]

On 2015-04-07 18:28, Mark Brown wrote:
> On Tue, Apr 07, 2015 at 09:39:03AM -0600, Stephen Warren wrote:
>> On 04/07/2015 12:25 AM, Martin Sperl wrote:
>> ... then not touch any CS-related register for the entire transfer. There
>> shouldn't be a need to enable/disable IRQs during the transfer; just leave
>> them enabled the entire time, until all bytes have been transferred.
>
> We will need to make sure /CS is kept asserted between transfers in a
> message too.
The problem is here that you need to touch the CS-register when an
individual spi-transfer is finished, as only that way you can disable
the (level)interrupts, which would fire immediately again when you exit
chip-select.

So you are at the risk of it happening at that phase as well...
and during the next spi_transfer_one you need to reenable the
interrupts again.

Anyway: my latest incarnation of the driver does now implement full
cs_gpio operation with native-cs getting translated during the
new spi_register_cs phase when registering the bus using the (new)
function-pointer register_cs in spi_master.
So this issue goes away and the code becomes smaller at the same time.

This is also used to set up chip_selects to their defaults levels prior
to any activity on the SPI bus.
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html