Skip to contentSkip to conversation

spi: bcm2835aux: auxiliary spi improvements

PATCH Superseded stephanolbrich at gmx.de 7 participants 27 messages Feb 9, 2016 – Feb 11, 2016 2 messages not in the archive

Subsystems (3)

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore
#0 lore raw

cover spi: bcm2835aux: auxiliary spi improvements

From: Stephan Olbrich <[email protected]>

This patch series has some improvements and fixes for the auxiliary spi.

1. fix bitmask defines
just fixes a typo (needed in 2.)

2. disable tx fifo empty irq
reduces the numer of interrupts with nothing to do

3. set up spi-mode before asserting cs-gpio
As Martin Sperl suggested this is done in the same way as in spi-bcm2835.c acace73df2c1913a526c1b41e4741a4a6704c863

4. fix CPOL/CPHA setting
From what I've seen in the documentation [1] and seen on the scope this chip doesn't support modes with CPHA=1. With this patch spi mode 0 and 2 should work correctly whereas mode 1 and 3 do not.

[1] https://www.raspberrypi.org/wp-content/uploads/2012/02/BCM2835-ARM-Peripherals.pdf

Stephan Olbrich (4):
spi: bcm2835aux: fix bitmask defines
spi: bcm2835aux: disable tx fifo empty irq
spi: bcm2835aux: set up spi-mode before asserting cs-gpio
spi: bcm2835aux: fix CPOL/CPHA setting

 drivers/spi/spi-bcm2835aux.c | 64 +++++++++++++++++++++++++++++++-------------
 1 file changed, 46 insertions(+), 18 deletions(-)
Signature · 1 line
2.5.0
#1 lore raw

PATCH 1/4 spi: bcm2835aux: fix bitmask defines

From: Stephan Olbrich <[email protected]>

The bitmasks for txempty and idle interrupts were interchanged.

---
 drivers/spi/spi-bcm2835aux.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/spi/spi-bcm2835aux.c +2 -2
index 7de6f84..ecc73c0 100644
@@ -73,8 +73,8 @@
 
 /* Bitfields in CNTL1 */
 #define BCM2835_AUX_SPI_CNTL1_CSHIGH	0x00000700
-#define BCM2835_AUX_SPI_CNTL1_IDLE	0x00000080
-#define BCM2835_AUX_SPI_CNTL1_TXEMPTY	0x00000040
+#define BCM2835_AUX_SPI_CNTL1_TXEMPTY	0x00000080
+#define BCM2835_AUX_SPI_CNTL1_IDLE	0x00000040
 #define BCM2835_AUX_SPI_CNTL1_MSBF_IN	0x00000002
 #define BCM2835_AUX_SPI_CNTL1_KEEP_IN	0x00000001
 
Signature · 1 line
2.5.0
#2 lore raw

PATCH 1/4 spi: bcm2835aux: fix bitmask defines

Hi Stephan,

Am 09.02.2016 um 19:10 schrieb stephanolbrich at gmx.de:

Quoted 14 lines

From: Stephan Olbrich <[email protected]>

The bitmasks for txempty and idle interrupts were interchanged.

---
  drivers/spi/spi-bcm2835aux.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)
drivers/spi/spi-bcm2835aux.c +0 -0
index 7de6f84..ecc73c0 100644

drivers/spi/spi-bcm2835aux.c line 77

Quoted 5 lines
drivers/spi/spi-bcm2835aux.c +0 -2
@@ -73,8 +73,8 @@
 
  /* Bitfields in CNTL1 */
  #define BCM2835_AUX_SPI_CNTL1_CSHIGH	0x00000700
-#define BCM2835_AUX_SPI_CNTL1_IDLE	0x00000080
-#define BCM2835_AUX_SPI_CNTL1_TXEMPTY	0x00000040

according to a comment in this file these values are from brcm_usrlib/dag/vmcsx/vcinclude/bcm2708_chip/aux_io.h.

So you want to say that at least these 2 are wrong and you took the values from BCM2835-ARM-Peripherals.pdf [1]?

I think it's worth to mention it.

Regards

[1] -
https://www.raspberrypi.org/wp-content/uploads/2012/02/BCM2835-ARM-Peripherals.pdf

Quoted 5 lines
drivers/spi/spi-bcm2835aux.c +2 -0
+#define BCM2835_AUX_SPI_CNTL1_TXEMPTY	0x00000080
+#define BCM2835_AUX_SPI_CNTL1_IDLE	0x00000040
  #define BCM2835_AUX_SPI_CNTL1_MSBF_IN	0x00000002
  #define BCM2835_AUX_SPI_CNTL1_KEEP_IN	0x00000001
 
#3 lore raw

PATCH 1/4 spi: bcm2835aux: fix bitmask defines

Hi Stefan

Am Tuesday 09 February 2016, 20:54:03 schrieb Stefan Wahren:

Quoted 28 lines

Hi Stephan,

Am 09.02.2016 um 19:10 schrieb stephanolbrich at gmx.de:

Quoted 15 lines

From: Stephan Olbrich <[email protected]>

The bitmasks for txempty and idle interrupts were interchanged.

---

  drivers/spi/spi-bcm2835aux.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)
drivers/spi/spi-bcm2835aux.c +0 -0
index 7de6f84..ecc73c0 100644

drivers/spi/spi-bcm2835aux.c line 78

Quoted 6 lines
drivers/spi/spi-bcm2835aux.c +0 -2
@@ -73,8 +73,8 @@
 
  /* Bitfields in CNTL1 */
  #define BCM2835_AUX_SPI_CNTL1_CSHIGH	0x00000700
 
-#define BCM2835_AUX_SPI_CNTL1_IDLE	0x00000080
-#define BCM2835_AUX_SPI_CNTL1_TXEMPTY	0x00000040

according to a comment in this file these values are from brcm_usrlib/dag/vmcsx/vcinclude/bcm2708_chip/aux_io.h.

Quoted 4 lines

So you want to say that at least these 2 are wrong and you took the values from BCM2835-ARM-Peripherals.pdf [1]?

I think it's worth to mention it.

Actually I stumbled across this while writing the second patch in this series and the wrong interrupt got disabled but BCM2835-ARM-Peripherals.pdf did confirm my findings.
As Mark has already applied this patch I can't add any comment to it anymore, right?

Quoted 13 lines

[1] -
https://www.raspberrypi.org/wp-content/uploads/2012/02/BCM2835-ARM-Periphera
ls.pdf

Quoted 5 lines
drivers/spi/spi-bcm2835aux.c +2 -0
+#define BCM2835_AUX_SPI_CNTL1_TXEMPTY	0x00000080
+#define BCM2835_AUX_SPI_CNTL1_IDLE	0x00000040
 
  #define BCM2835_AUX_SPI_CNTL1_MSBF_IN	0x00000002
  #define BCM2835_AUX_SPI_CNTL1_KEEP_IN	0x00000001

_______________________________________________
linux-rpi-kernel mailing list
linux-rpi-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rpi-kernel

#4 lore raw

PATCH 1/4 spi: bcm2835aux: fix bitmask defines

In reply to stephanolbrich at gmx.de
Quoted 14 lines · stephanolbrich at gmx.de

From: Stephan Olbrich <[email protected]>

The bitmasks for txempty and idle interrupts were interchanged.

---
 drivers/spi/spi-bcm2835aux.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/spi/spi-bcm2835aux.c +0 -0
index 7de6f84..ecc73c0 100644

drivers/spi/spi-bcm2835aux.c line 79

Quoted 9 lines
drivers/spi/spi-bcm2835aux.c +2 -2
@@ -73,8 +73,8 @@
 
 /* Bitfields in CNTL1 */
 #define BCM2835_AUX_SPI_CNTL1_CSHIGH	0x00000700
-#define BCM2835_AUX_SPI_CNTL1_IDLE	0x00000080
-#define BCM2835_AUX_SPI_CNTL1_TXEMPTY	0x00000040
+#define BCM2835_AUX_SPI_CNTL1_TXEMPTY	0x00000080
+#define BCM2835_AUX_SPI_CNTL1_IDLE	0x00000040
 #define BCM2835_AUX_SPI_CNTL1_MSBF_IN	0x00000002
 #define BCM2835_AUX_SPI_CNTL1_KEEP_IN	0x00000001

Confirmed by looking at the hardware.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160209/d846cff3/attachment.sig>

A message that is not in the archive (sent off-list or not yet received): 1455041435-8015-2-git-send-email-stephanolbrich-Mmb7MZpHnFY@public.gmane.org

#6 lore raw

Applied "spi: bcm2835aux: fix bitmask defines" to the spi tree

The patch

spi: bcm2835aux: fix bitmask defines

has been applied to the spi tree at

git://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git

All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying to this mail.

Thanks,
Mark

Quoted 1 line

From fe0e2304f560f81c1673711ac3f9a8c7c3cbb8be Mon Sep 17 00:00:00 2001

From: Stephan Olbrich <[email protected]>
Date: Tue, 9 Feb 2016 19:10:32 +0100
Subject: [PATCH] spi: bcm2835aux: fix bitmask defines

The bitmasks for txempty and idle interrupts were interchanged.

---
 drivers/spi/spi-bcm2835aux.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/spi/spi-bcm2835aux.c +2 -2
index 7de6f8472a81..ecc73c0a97cf 100644
@@ -73,8 +73,8 @@
 
 /* Bitfields in CNTL1 */
 #define BCM2835_AUX_SPI_CNTL1_CSHIGH	0x00000700
-#define BCM2835_AUX_SPI_CNTL1_IDLE	0x00000080
-#define BCM2835_AUX_SPI_CNTL1_TXEMPTY	0x00000040
+#define BCM2835_AUX_SPI_CNTL1_TXEMPTY	0x00000080
+#define BCM2835_AUX_SPI_CNTL1_IDLE	0x00000040
 #define BCM2835_AUX_SPI_CNTL1_MSBF_IN	0x00000002
 #define BCM2835_AUX_SPI_CNTL1_KEEP_IN	0x00000001
 
Signature · 6 lines
2.7.0

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
#7 lore raw

PATCH 2/4 spi: bcm2835aux: disable tx fifo empty irq

In reply to stephanolbrich at gmx.de

From: Stephan Olbrich <[email protected]>

The tx empty irq can be disabled when all data was copied. This prevents unnecessary interrupts while the last bytes are sent.

---
 drivers/spi/spi-bcm2835aux.c | 6 ++++++
 1 file changed, 6 insertions(+)
drivers/spi/spi-bcm2835aux.c +6 -0
index ecc73c0..d2f0067 100644
@@ -212,6 +212,12 @@ static irqreturn_t bcm2835aux_spi_interrupt(int irq, void *dev_id)
 		ret = IRQ_HANDLED;
 	}
 
+	if (!bs->tx_len) {
+		/* disable tx fifo empty interrupt */
+		bcm2835aux_wr(bs, BCM2835_AUX_SPI_CNTL1, bs->cntl[1] |
+			BCM2835_AUX_SPI_CNTL1_IDLE);
+	}
+
 	/* and if rx_len is 0 then wake up completion and disable spi */
 	if (!bs->rx_len) {
 		bcm2835aux_spi_reset_hw(bs);
Signature · 1 line
2.5.0
#8 lore raw

PATCH 2/4 spi: bcm2835aux: disable tx fifo empty irq

Quoted 29 lines · stephanolbrich at gmx.de

From: Stephan Olbrich <[email protected]>

The tx empty irq can be disabled when all data was copied. This prevents unnecessary interrupts while the last bytes are sent.

---
 drivers/spi/spi-bcm2835aux.c | 6 ++++++
 1 file changed, 6 insertions(+)
drivers/spi/spi-bcm2835aux.c +6 -0
index ecc73c0..d2f0067 100644
@@ -212,6 +212,12 @@ static irqreturn_t bcm2835aux_spi_interrupt(int irq, void *dev_id)
 		ret = IRQ_HANDLED;
 	}
 
+	if (!bs->tx_len) {
+		/* disable tx fifo empty interrupt */
+		bcm2835aux_wr(bs, BCM2835_AUX_SPI_CNTL1, bs->cntl[1] |
+			BCM2835_AUX_SPI_CNTL1_IDLE);
+	}
+
 	/* and if rx_len is 0 then wake up completion and disable spi */
 	if (!bs->rx_len) {
 		bcm2835aux_spi_reset_hw(bs);
Signature · 1 line
2.5.0

Right, we don't want to come back in here with a spurious TX empty interrupt while we wait for the RX bits to trickle in through the FIFO. I'm having a hard time reasoning through how likely this would be, but it seems like a good change.

Aside: I think I see a problem that we reset the hardware before it has
asserted SPI_STAT_BUSY, since we reset as soon as we've collected our RX
data (!bs->rx_len). That means we've potentially missed the trailing
hold time on CS at the end of the transfer, and it's going to resume at
the same point in its state machine when we reassert CNTL0_ENABLE for
the next transfer. That doesn't seem like what we want.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160209/dc3a5fe5/attachment.sig>

A message that is not in the archive (sent off-list or not yet received): 1455041435-8015-3-git-send-email-stephanolbrich-Mmb7MZpHnFY@public.gmane.org

#10 lore raw

Applied "spi: bcm2835aux: disable tx fifo empty irq" to the spi tree

The patch

spi: bcm2835aux: disable tx fifo empty irq

has been applied to the spi tree at

git://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git

All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying to this mail.

Thanks,
Mark

Quoted 1 line

From f29ab1845f3e2684ba1c6de6c3bd5198e4b1459c Mon Sep 17 00:00:00 2001

From: Stephan Olbrich <[email protected]>
Date: Tue, 9 Feb 2016 19:10:33 +0100
Subject: [PATCH] spi: bcm2835aux: disable tx fifo empty irq

The tx empty irq can be disabled when all data was copied. This prevents unnecessary interrupts while the last bytes are sent.

---
 drivers/spi/spi-bcm2835aux.c | 6 ++++++
 1 file changed, 6 insertions(+)
drivers/spi/spi-bcm2835aux.c +6 -0
index 7de6f8472a81..e1b2fec1db63 100644
@@ -212,6 +212,12 @@ static irqreturn_t bcm2835aux_spi_interrupt(int irq, void *dev_id)
 		ret = IRQ_HANDLED;
 	}
 
+	if (!bs->tx_len) {
+		/* disable tx fifo empty interrupt */
+		bcm2835aux_wr(bs, BCM2835_AUX_SPI_CNTL1, bs->cntl[1] |
+			BCM2835_AUX_SPI_CNTL1_IDLE);
+	}
+
 	/* and if rx_len is 0 then wake up completion and disable spi */
 	if (!bs->rx_len) {
 		bcm2835aux_spi_reset_hw(bs);
Signature · 6 lines
2.7.0

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
#11 lore raw

PATCH 3/4 spi: bcm2835aux: set up spi-mode before asserting cs-gpio

In reply to stephanolbrich at gmx.de

From: Stephan Olbrich <[email protected]>

When using reverse polarity for clock (spi-cpol) on a device the clock line gets altered after chip-select has been asserted resulting in an additional clock beat, which confuses hardware.

To avoid this situation this patch moves the setup of polarity (spi-cpol and spi-cpha) outside of the chip-select into prepare_message, which is run prior to asserting chip-select.

---
 drivers/spi/spi-bcm2835aux.c | 54 +++++++++++++++++++++++++++++++-------------
 1 file changed, 38 insertions(+), 16 deletions(-)
drivers/spi/spi-bcm2835aux.c +38 -16
index d2f0067..b90aa34 100644
@@ -218,9 +218,9 @@ static irqreturn_t bcm2835aux_spi_interrupt(int irq, void *dev_id)
 			BCM2835_AUX_SPI_CNTL1_IDLE);
 	}
 
-	/* and if rx_len is 0 then wake up completion and disable spi */
+	/* and if rx_len is 0 then disable interrupts and wake up completion */
 	if (!bs->rx_len) {
-		bcm2835aux_spi_reset_hw(bs);
+		bcm2835aux_wr(bs, BCM2835_AUX_SPI_CNTL1, bs->cntl[1]);
 		complete(&master->xfer_completion);
 	}
 
@@ -313,9 +313,6 @@ static int bcm2835aux_spi_transfer_one_poll(struct spi_master *master,
 		}
 	}
 
-	/* Transfer complete - reset SPI HW */
-	bcm2835aux_spi_reset_hw(bs);
-
 	/* and return without waiting for completion */
 	return 0;
 }
@@ -336,10 +333,6 @@ static int bcm2835aux_spi_transfer_one(struct spi_master *master,
 	 * resulting (potentially) in more interrupts when transferring
 	 * more than 12 bytes
 	 */
-	bs->cntl[0] = BCM2835_AUX_SPI_CNTL0_ENABLE |
-		      BCM2835_AUX_SPI_CNTL0_VAR_WIDTH |
-		      BCM2835_AUX_SPI_CNTL0_MSBF_OUT;
-	bs->cntl[1] = BCM2835_AUX_SPI_CNTL1_MSBF_IN;
 
 	/* set clock */
 	spi_hz = tfr->speed_hz;
@@ -358,13 +351,6 @@ static int bcm2835aux_spi_transfer_one(struct spi_master *master,
 
 	spi_used_hz = clk_hz / (2 * (speed + 1));
 
-	/* handle all the modes */
-	if (spi->mode & SPI_CPOL)
-		bs->cntl[0] |= BCM2835_AUX_SPI_CNTL0_CPOL;
-	if (spi->mode & SPI_CPHA)
-		bs->cntl[0] |= BCM2835_AUX_SPI_CNTL0_CPHA_OUT |
-			       BCM2835_AUX_SPI_CNTL0_CPHA_IN;
-
 	/* set transmit buffers and length */
 	bs->tx_buf = tfr->tx_buf;
 	bs->rx_buf = tfr->rx_buf;
@@ -388,6 +374,40 @@ static int bcm2835aux_spi_transfer_one(struct spi_master *master,
 	return bcm2835aux_spi_transfer_one_irq(master, spi, tfr);
 }
 
+static int bcm2835aux_spi_prepare_message(struct spi_master *master,
+					  struct spi_message *msg)
+{
+	struct spi_device *spi = msg->spi;
+	struct bcm2835aux_spi *bs = spi_master_get_devdata(master);
+
+	bs->cntl[0] = BCM2835_AUX_SPI_CNTL0_ENABLE |
+		      BCM2835_AUX_SPI_CNTL0_VAR_WIDTH |
+		      BCM2835_AUX_SPI_CNTL0_MSBF_OUT;
+	bs->cntl[1] = BCM2835_AUX_SPI_CNTL1_MSBF_IN;
+
+	/* handle all the modes */
+	if (spi->mode & SPI_CPOL)
+		bs->cntl[0] |= BCM2835_AUX_SPI_CNTL0_CPOL;
+	if (spi->mode & SPI_CPHA)
+		bs->cntl[0] |= BCM2835_AUX_SPI_CNTL0_CPHA_OUT |
+			       BCM2835_AUX_SPI_CNTL0_CPHA_IN;
+
+	bcm2835aux_wr(bs, BCM2835_AUX_SPI_CNTL1, bs->cntl[1]);
+	bcm2835aux_wr(bs, BCM2835_AUX_SPI_CNTL0, bs->cntl[0]);
+
+	return 0;
+}
+
+static int bcm2835aux_spi_unprepare_message(struct spi_master *master,
+					    struct spi_message *msg)
+{
+	struct bcm2835aux_spi *bs = spi_master_get_devdata(master);
+
+	bcm2835aux_spi_reset_hw(bs);
+
+	return 0;
+}
+
 static void bcm2835aux_spi_handle_err(struct spi_master *master,
 				      struct spi_message *msg)
 {
@@ -416,6 +436,8 @@ static int bcm2835aux_spi_probe(struct platform_device *pdev)
 	master->num_chipselect = -1;
 	master->transfer_one = bcm2835aux_spi_transfer_one;
 	master->handle_err = bcm2835aux_spi_handle_err;
+	master->prepare_message = bcm2835aux_spi_prepare_message;
+	master->unprepare_message = bcm2835aux_spi_unprepare_message;
 	master->dev.of_node = pdev->dev.of_node;
 
 	bs = spi_master_get_devdata(master);
Signature · 1 line
2.5.0
#12 lore raw

PATCH 3/4 spi: bcm2835aux: set up spi-mode before asserting cs-gpio

Quoted 9 lines · stephanolbrich at gmx.de

From: Stephan Olbrich <[email protected]>

When using reverse polarity for clock (spi-cpol) on a device the clock line gets altered after chip-select has been asserted resulting in an additional clock beat, which confuses hardware.

To avoid this situation this patch moves the setup of polarity (spi-cpol and spi-cpha) outside of the chip-select into prepare_message, which is run prior to asserting chip-select.

This patch surprised me. I would have thought that the solution was to just write the updated CNTL bits for CPOL and wait a moment whenever it changes. The CS only gets asserted later on when we get some data in the TX FIFO, so I think you're just reducing the chance of losing the race to get our inverted clock noticed by the device before the CS gets asserted.

If we're only talking to a device that does an inverted clock, it seems silly that we're resetting the hardware back to non-inverted clock after every transfer/message.

I'd be OK with the patch anyway, since you reduce the number of resets for a multi-transfer message, except for what I think is bug...

Quoted 32 lines
---
 drivers/spi/spi-bcm2835aux.c | 54 +++++++++++++++++++++++++++++++-------------
 1 file changed, 38 insertions(+), 16 deletions(-)
drivers/spi/spi-bcm2835aux.c +2 -5
index d2f0067..b90aa34 100644
@@ -218,9 +218,9 @@ static irqreturn_t bcm2835aux_spi_interrupt(int irq, void *dev_id)
 			BCM2835_AUX_SPI_CNTL1_IDLE);
 	}
 
-	/* and if rx_len is 0 then wake up completion and disable spi */
+	/* and if rx_len is 0 then disable interrupts and wake up completion */
 	if (!bs->rx_len) {
-		bcm2835aux_spi_reset_hw(bs);
+		bcm2835aux_wr(bs, BCM2835_AUX_SPI_CNTL1, bs->cntl[1]);
 		complete(&master->xfer_completion);
 	}
 
@@ -313,9 +313,6 @@ static int bcm2835aux_spi_transfer_one_poll(struct spi_master *master,
 		}
 	}
 
-	/* Transfer complete - reset SPI HW */
-	bcm2835aux_spi_reset_hw(bs);
-
 	/* and return without waiting for completion */
 	return 0;
 }

drivers/spi/spi-bcm2835aux.c static int bcm2835aux_spi_transfer_one(struct spi_master *master, · line 338

Quoted 10 lines
drivers/spi/spi-bcm2835aux.c +0 -4
@@ -336,10 +333,6 @@ static int bcm2835aux_spi_transfer_one(struct spi_master *master,
 	 * resulting (potentially) in more interrupts when transferring
 	 * more than 12 bytes
 	 */
-	bs->cntl[0] = BCM2835_AUX_SPI_CNTL0_ENABLE |
-		      BCM2835_AUX_SPI_CNTL0_VAR_WIDTH |
-		      BCM2835_AUX_SPI_CNTL0_MSBF_OUT;
-	bs->cntl[1] = BCM2835_AUX_SPI_CNTL1_MSBF_IN;
 
 	/* set clock */
 	spi_hz = tfr->speed_hz;

Just below this block, we update cntl[0] with the transfer's speed bits,
so now that you're not resetting cntl[0] on each transfer, their speeds
will all get ORed all together by the end. I think you could just mask
out the max speed before setting the new one.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160209/014504fd/attachment.sig>

maintainer
#13 lore raw

PATCH 3/4 spi: bcm2835aux: set up spi-mode before asserting cs-gpio

Quoted 6 lines · Eric Anholt

This patch surprised me. I would have thought that the solution was to just write the updated CNTL bits for CPOL and wait a moment whenever it changes. The CS only gets asserted later on when we get some data in the TX FIFO, so I think you're just reducing the chance of losing the race to get our inverted clock noticed by the device before the CS gets asserted.

We support (and generally want to use since hardware chip selects are
often very limited in what they do) chip select on GPIO.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160210/5b1f94c4/attachment.sig>

#14 lore raw

PATCH 3/4 spi: bcm2835aux: set up spi-mode before asserting cs-gpio

Quoted 11 lines · Mark Brown
Quoted 6 lines · Eric Anholt

This patch surprised me. I would have thought that the solution was to just write the updated CNTL bits for CPOL and wait a moment whenever it changes. The CS only gets asserted later on when we get some data in the TX FIFO, so I think you're just reducing the chance of losing the race to get our inverted clock noticed by the device before the CS gets asserted.

We support (and generally want to use since hardware chip selects are often very limited in what they do) chip select on GPIO.

Oops, this makes more sense now. Subject even mentioned gpio, and a
GPIO CS must be getting set around the transfer_one call. I'll be ready
to send an r-b for a v2 with the speed update bug fixed (unless transfer
speeds are guaranteed to be constant between a prepare and unprepare).
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160210/01477724/attachment.sig>

maintainer
#15 lore raw

PATCH 3/4 spi: bcm2835aux: set up spi-mode before asserting cs-gpio

Quoted 4 lines · Eric Anholt

Oops, this makes more sense now. Subject even mentioned gpio, and a GPIO CS must be getting set around the transfer_one call. I'll be ready to send an r-b for a v2 with the speed update bug fixed (unless transfer speeds are guaranteed to be constant between a prepare and unprepare).

No guarantee that they'll not change, especially in a situation where we
have two devices on different speeds on a bus working at the same time.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160210/80c1d3df/attachment.sig>

#16 lore raw

PATCH 3/4 spi: bcm2835aux: set up spi-mode before asserting cs-gpio

Am Wednesday 10 February 2016, 19:02:04 schrieb Mark Brown:

Quoted 8 lines · Eric Anholt
Quoted 4 lines

Oops, this makes more sense now. Subject even mentioned gpio, and a GPIO CS must be getting set around the transfer_one call. I'll be ready to send an r-b for a v2 with the speed update bug fixed (unless transfer speeds are guaranteed to be constant between a prepare and unprepare).

No guarantee that they'll not change, especially in a situation where we have two devices on different speeds on a bus working at the same time.

Are you saying, that between a prepare and unprepare there could be transfers for another device? Then all the clock setting couldn't be done in prepare either.
Apart from that, if two transfers in the same message are not guarantied to have the same speed, this still needs to be fixed. I'll roll a v2.

#17 lore raw

PATCH 3/4 spi: bcm2835aux: set up spi-mode before asserting cs-gpio

Quoted 10 lines · Stephan Olbrich

Am Wednesday 10 February 2016, 19:02:04 schrieb Mark Brown:

Quoted 6 lines · Eric Anholt
Quoted 4 lines

Oops, this makes more sense now. Subject even mentioned gpio, and a GPIO CS must be getting set around the transfer_one call. I'll be ready to send an r-b for a v2 with the speed update bug fixed (unless transfer speeds are guaranteed to be constant between a prepare and unprepare).

Quoted 7 lines
Quoted 2 lines

No guarantee that they'll not change, especially in a situation where we have two devices on different speeds on a bus working at the same time.

Are you saying, that between a prepare and unprepare there could be transfers for another device? Then all the clock setting couldn't be done in prepare either.
Apart from that, if two transfers in the same message are not guarantied to have the same speed, this still needs to be fixed. I'll roll a v2.

Prepare/unprepare is always called when processing an spi message. there is never a phase where two spi_messages are prepared concurrently for the same spi bus.

See the implementation of __spi_pump_messages

The sequence for processing is:
* master->prepare_hardware
* master->prepare_message
* spi_map_message
* master->transfer_one_message
* spi_set_cs(true)
at end of message:
* spi_finalize_current_message
* spi_set_cs(false)
* master->unprepare_message

So the use of prepare_message in this context is valid.

Otherwise we would need to add an additional method to setup polarity/cs prior to spi_set_cs(true).

Ciao,
Martin

maintainer
#18 lore raw

PATCH 3/4 spi: bcm2835aux: set up spi-mode before asserting cs-gpio

In reply to Stephan Olbrich
Quoted 10 lines · Stephan Olbrich

Am Wednesday 10 February 2016, 19:02:04 schrieb Mark Brown:

Quoted 2 lines

No guarantee that they'll not change, especially in a situation where we have two devices on different speeds on a bus working at the same time.

Are you saying, that between a prepare and unprepare there could be transfers for another device? Then all the clock setting couldn't be done in prepare either.
Apart from that, if two transfers in the same message are not guarantied to have the same speed, this still needs to be fixed. I'll roll a v2.

There are two prepares, I don't know which you're talking about. The
most common is prepare_transfer_hardware() which is used to power up the
hardware and may have many transfers for many devices before it is
reversed. The other is prepare_message() which is called once per
message.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160211/21540ab2/attachment-0001.sig>

#19 lore raw

PATCH 3/4 spi: bcm2835aux: set up spi-mode before asserting cs-gpio

In reply to stephanolbrich at gmx.de

drivers/spi/spi-bcm2835aux.c static int bcm2835aux_spi_transfer_one(struct spi_master *master,

Quoted 56 lines · stephanolbrich at gmx.de

From: Stephan Olbrich <[email protected]>

When using reverse polarity for clock (spi-cpol) on a device the clock line gets altered after chip-select has been asserted resulting in an additional clock beat, which confuses hardware.

To avoid this situation this patch moves the setup of polarity (spi-cpol and spi-cpha) outside of the chip-select into prepare_message, which is run prior to asserting chip-select.

---
drivers/spi/spi-bcm2835aux.c | 54 +++++++++++++++++++++++++++++++-------------

1 file changed, 38 insertions(+), 16 deletions(-)
drivers/spi/spi-bcm2835aux.c +2 -9
index d2f0067..b90aa34 100644
@@ -218,9 +218,9 @@ static irqreturn_t bcm2835aux_spi_interrupt(int irq, void *dev_id)
 			BCM2835_AUX_SPI_CNTL1_IDLE);
 	}
 
-	/* and if rx_len is 0 then wake up completion and disable spi */
+	/* and if rx_len is 0 then disable interrupts and wake up completion */
 	if (!bs->rx_len) {
-		bcm2835aux_spi_reset_hw(bs);
+		bcm2835aux_wr(bs, BCM2835_AUX_SPI_CNTL1, bs->cntl[1]);
 		complete(&master->xfer_completion);
 	}
 
@@ -313,9 +313,6 @@ static int bcm2835aux_spi_transfer_one_poll(struct spi_master *master,
 		}
 	}
 
-	/* Transfer complete - reset SPI HW */
-	bcm2835aux_spi_reset_hw(bs);
-
 	/* and return without waiting for completion */
 	return 0;
 }
@@ -336,10 +333,6 @@ static int bcm2835aux_spi_transfer_one(struct spi_master *master,
 	 * resulting (potentially) in more interrupts when transferring
 	 * more than 12 bytes
 	 */
-	bs->cntl[0] = BCM2835_AUX_SPI_CNTL0_ENABLE |
-		      BCM2835_AUX_SPI_CNTL0_VAR_WIDTH |
-		      BCM2835_AUX_SPI_CNTL0_MSBF_OUT;
-	bs->cntl[1] = BCM2835_AUX_SPI_CNTL1_MSBF_IN;
 
 	/* set clock */
 	spi_hz = tfr->speed_hz;
@@ -358,13 +351,6 @@ static int bcm2835aux_spi_transfer_one(struct spi_master *master,

Note that here you need to explicitly mask out the clock! otherwise spi messages with different clock speeds will fill up the bitfield of the clock divider with the wrong values.

This has not happened before because the values were re-computed every time now we defer it to prepare

@@ -348,17 +345,12 @@ static int bcm2835aux_spi_transfer_one(struct spi_master *

        } else { /* the slowest we can go */
                speed = BCM2835_AUX_SPI_CNTL0_SPEED_MAX;
        }
+	/* mask out old speed from previous spi_transfer */
+       bs->cntl[0] &= ~BCM2835_AUX_SPI_CNTL0_SPEED;
+	/* set the new speed */
        bs->cntl[0] |= speed << BCM2835_AUX_SPI_CNTL0_SPEED_SHIFT;
Quoted 21 lines
drivers/spi/spi-bcm2835aux.c +1 -7
 	spi_used_hz = clk_hz / (2 * (speed + 1));
 
-	/* handle all the modes */
-	if (spi->mode & SPI_CPOL)
-		bs->cntl[0] |= BCM2835_AUX_SPI_CNTL0_CPOL;
-	if (spi->mode & SPI_CPHA)
-		bs->cntl[0] |= BCM2835_AUX_SPI_CNTL0_CPHA_OUT |
-			       BCM2835_AUX_SPI_CNTL0_CPHA_IN;
-
 	/* set transmit buffers and length */
 	bs->tx_buf = tfr->tx_buf;
 	bs->rx_buf = tfr->rx_buf;
 
@@ -388,6 +374,40 @@ static int bcm2835aux_spi_transfer_one(struct spi_master *master,
 	return bcm2835aux_spi_transfer_one_irq(master, spi, tfr);
 }
 
+

drivers/spi/spi-bcm2835aux.c static int bcm2835aux_spi_transfer_one(struct spi_master *master, · line 387

Quoted 10 lines
drivers/spi/spi-bcm2835aux.c +10 -0
+static int bcm2835aux_spi_unprepare_message(struct spi_master *master,
+					    struct spi_message *msg)
+{
+	struct bcm2835aux_spi *bs = spi_master_get_devdata(master);
+
+	bcm2835aux_spi_reset_hw(bs);
+
+	return 0;
+}
+

I was wondering a long time why we would need this - but after some experimentation it became clear: with auxspi disabled driving the gates so they go into their default states (low) before GPIO-cs is de-asserted.

It may be worth mentioning that fact - others might wonder as well later later.

With the above in place:

Martin

#20 lore raw

PATCH 4/4 spi: bcm2835aux: fix CPOL/CPHA setting

In reply to stephanolbrich at gmx.de

From: Stephan Olbrich <[email protected]>

The auxiliary spi supports only CPHA=0 modes as the first bit is always output to the pin before the first clock cycle. In CPHA=1 modes the first clock edge outputs the second bit hence the slave can never read the first bit.

Also the CPHA registers switch between clocking data in/out on rising/falling edge hence depend on the CPOL setting.

---
 drivers/spi/spi-bcm2835aux.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
drivers/spi/spi-bcm2835aux.c +5 -5
index b90aa34..169f521 100644
@@ -386,12 +386,12 @@ static int bcm2835aux_spi_prepare_message(struct spi_master *master,
 	bs->cntl[1] = BCM2835_AUX_SPI_CNTL1_MSBF_IN;
 
 	/* handle all the modes */
-	if (spi->mode & SPI_CPOL)
+	if (spi->mode & SPI_CPOL) {
 		bs->cntl[0] |= BCM2835_AUX_SPI_CNTL0_CPOL;
-	if (spi->mode & SPI_CPHA)
-		bs->cntl[0] |= BCM2835_AUX_SPI_CNTL0_CPHA_OUT |
-			       BCM2835_AUX_SPI_CNTL0_CPHA_IN;
-
+		bs->cntl[0] |= BCM2835_AUX_SPI_CNTL0_CPHA_OUT;
+	} else {
+		bs->cntl[0] |= BCM2835_AUX_SPI_CNTL0_CPHA_IN;
+	}
 	bcm2835aux_wr(bs, BCM2835_AUX_SPI_CNTL1, bs->cntl[1]);
 	bcm2835aux_wr(bs, BCM2835_AUX_SPI_CNTL0, bs->cntl[0]);
 
Signature · 1 line
2.5.0
#21 lore raw

PATCH 4/4 spi: bcm2835aux: fix CPOL/CPHA setting

Hi Stephan,

Am 09.02.2016 um 19:10 schrieb stephanolbrich at gmx.de:

Quoted 6 lines

From: Stephan Olbrich <[email protected]>

The auxiliary spi supports only CPHA=0 modes as the first bit is always output to the pin before the first clock cycle. In CPHA=1 modes the first clock edge outputs the second bit hence the slave can never read the first bit.

in case auxiliary spi isn't able to handle all modes shouldn't we report this to the upper layers?

Regards

#22 lore raw

PATCH 4/4 spi: bcm2835aux: fix CPOL/CPHA setting

In reply to stephanolbrich at gmx.de
Quoted 26 lines · stephanolbrich at gmx.de

From: Stephan Olbrich <[email protected]>

The auxiliary spi supports only CPHA=0 modes as the first bit is always output to the pin before the first clock cycle. In CPHA=1 modes the first clock edge outputs the second bit hence the slave can never read the first bit.

Also the CPHA registers switch between clocking data in/out on rising/falling edge hence depend on the CPOL setting.

---
 drivers/spi/spi-bcm2835aux.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
drivers/spi/spi-bcm2835aux.c +1 -1
index b90aa34..169f521 100644
@@ -386,12 +386,12 @@ static int bcm2835aux_spi_prepare_message(struct spi_master *master,
 	bs->cntl[1] = BCM2835_AUX_SPI_CNTL1_MSBF_IN;
 
 	/* handle all the modes */
-	if (spi->mode & SPI_CPOL)
+	if (spi->mode & SPI_CPOL) {
 		bs->cntl[0] |= BCM2835_AUX_SPI_CNTL0_CPOL;

drivers/spi/spi-bcm2835aux.c static int bcm2835aux_spi_prepare_message(struct spi_master *master, · line 396

Quoted 10 lines
drivers/spi/spi-bcm2835aux.c +4 -4
-	if (spi->mode & SPI_CPHA)
-		bs->cntl[0] |= BCM2835_AUX_SPI_CNTL0_CPHA_OUT |
-			       BCM2835_AUX_SPI_CNTL0_CPHA_IN;
-
+		bs->cntl[0] |= BCM2835_AUX_SPI_CNTL0_CPHA_OUT;
+	} else {
+		bs->cntl[0] |= BCM2835_AUX_SPI_CNTL0_CPHA_IN;
+	}
 	bcm2835aux_wr(bs, BCM2835_AUX_SPI_CNTL1, bs->cntl[1]);
 	bcm2835aux_wr(bs, BCM2835_AUX_SPI_CNTL0, bs->cntl[0]);

(Note for other readers: A better name for CNTL0_CPHA_* would be CNTL0_*_RISING).

I think you're right about not actually supporting CPHA. I don't see wany way to keep bit 1 out lasting through the first full clock cycle. I think Stefan's right that we should drop CPHA from MODE_BITS (actually, MODE_BITS would be nicer if we just merged it into its one user, I think).

However, this hunk appears to be correct and would fix the timing of our
data going out in the CPOL=0 case and of sampling IN data for CPOL=1.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160209/c71b2207/attachment-0001.sig>

#23 lore raw

PATCH 4/4 spi: bcm2835aux: fix CPOL/CPHA setting

Am Tuesday 09 February 2016, 16:13:06 schrieb Eric Anholt:

Quoted 41 lines
Quoted 40 lines · stephanolbrich at gmx.de

From: Stephan Olbrich <[email protected]>

The auxiliary spi supports only CPHA=0 modes as the first bit is always output to the pin before the first clock cycle. In CPHA=1 modes the first clock edge outputs the second bit hence the slave can never read the first bit.

Also the CPHA registers switch between clocking data in/out on rising/falling edge hence depend on the CPOL setting.

---

 drivers/spi/spi-bcm2835aux.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
drivers/spi/spi-bcm2835aux.c +0 -0
index b90aa34..169f521 100644
@@ -386,12 +386,12 @@ static int bcm2835aux_spi_prepare_message(struct

spi_master *master,>

drivers/spi/spi-bcm2835aux.c +5 -5
 	bs->cntl[1] = BCM2835_AUX_SPI_CNTL1_MSBF_IN;
 
 	/* handle all the modes */
 
-	if (spi->mode & SPI_CPOL)
+	if (spi->mode & SPI_CPOL) {
 
 		bs->cntl[0] |= BCM2835_AUX_SPI_CNTL0_CPOL;
 
-	if (spi->mode & SPI_CPHA)
-		bs->cntl[0] |= BCM2835_AUX_SPI_CNTL0_CPHA_OUT |
-			       BCM2835_AUX_SPI_CNTL0_CPHA_IN;
-
+		bs->cntl[0] |= BCM2835_AUX_SPI_CNTL0_CPHA_OUT;
+	} else {
+		bs->cntl[0] |= BCM2835_AUX_SPI_CNTL0_CPHA_IN;
+	}
Quoted 5 lines

drivers/spi/spi-bcm2835aux.c static int bcm2835aux_spi_prepare_message(struct spi_master *master, · line 396

Quoted 2 lines
drivers/spi/spi-bcm2835aux.c +0 -0
 	bcm2835aux_wr(bs, BCM2835_AUX_SPI_CNTL1, bs->cntl[1]);
 	bcm2835aux_wr(bs, BCM2835_AUX_SPI_CNTL0, bs->cntl[0]);

(Note for other readers: A better name for CNTL0_CPHA_* would be CNTL0_*_RISING).

Should I rename them?

Quoted 5 lines

I think you're right about not actually supporting CPHA. I don't see wany way to keep bit 1 out lasting through the first full clock cycle. I think Stefan's right that we should drop CPHA from MODE_BITS (actually, MODE_BITS would be nicer if we just merged it into its one user, I think).

You are right. I'll fix that.

#24 lore raw

PATCH 4/4 spi: bcm2835aux: fix CPOL/CPHA setting

Quoted 42 lines · Stephan Olbrich

Am Tuesday 09 February 2016, 16:13:06 schrieb Eric Anholt:

Quoted 41 lines
Quoted 40 lines · stephanolbrich at gmx.de

From: Stephan Olbrich <[email protected]>

The auxiliary spi supports only CPHA=0 modes as the first bit is always output to the pin before the first clock cycle. In CPHA=1 modes the first clock edge outputs the second bit hence the slave can never read the first bit.

Also the CPHA registers switch between clocking data in/out on rising/falling edge hence depend on the CPOL setting.

---

 drivers/spi/spi-bcm2835aux.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
drivers/spi/spi-bcm2835aux.c +0 -0
index b90aa34..169f521 100644
@@ -386,12 +386,12 @@ static int bcm2835aux_spi_prepare_message(struct

spi_master *master,>

drivers/spi/spi-bcm2835aux.c +5 -5
 	bs->cntl[1] = BCM2835_AUX_SPI_CNTL1_MSBF_IN;
 
 	/* handle all the modes */
 
-	if (spi->mode & SPI_CPOL)
+	if (spi->mode & SPI_CPOL) {
 
 		bs->cntl[0] |= BCM2835_AUX_SPI_CNTL0_CPOL;
 
-	if (spi->mode & SPI_CPHA)
-		bs->cntl[0] |= BCM2835_AUX_SPI_CNTL0_CPHA_OUT |
-			       BCM2835_AUX_SPI_CNTL0_CPHA_IN;
-
+		bs->cntl[0] |= BCM2835_AUX_SPI_CNTL0_CPHA_OUT;
+	} else {
+		bs->cntl[0] |= BCM2835_AUX_SPI_CNTL0_CPHA_IN;
+	}
Quoted 7 lines
Quoted 5 lines

drivers/spi/spi-bcm2835aux.c static int bcm2835aux_spi_prepare_message(struct spi_master *master, · line 396

Quoted 2 lines
drivers/spi/spi-bcm2835aux.c +0 -0
 	bcm2835aux_wr(bs, BCM2835_AUX_SPI_CNTL1, bs->cntl[1]);
 	bcm2835aux_wr(bs, BCM2835_AUX_SPI_CNTL0, bs->cntl[0]);

(Note for other readers: A better name for CNTL0_CPHA_* would be CNTL0_*_RISING).

Should I rename them?

Up to you. I'm happy to see the work you've done fixing the driver
here, and I don't want to pile things on.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160210/3685dd6e/attachment.sig>

#25 lore raw

PATCH 4/4 spi: bcm2835aux: fix CPOL/CPHA setting

In reply to Eric Anholt
Quoted 45 lines · Eric Anholt
Quoted 26 lines · stephanolbrich at gmx.de

From: Stephan Olbrich <[email protected]>

The auxiliary spi supports only CPHA=0 modes as the first bit is always output to the pin before the first clock cycle. In CPHA=1 modes the first clock edge outputs the second bit hence the slave can never read the first bit.

Also the CPHA registers switch between clocking data in/out on rising/falling edge hence depend on the CPOL setting.

---
drivers/spi/spi-bcm2835aux.c | 10 +++++-----

1 file changed, 5 insertions(+), 5 deletions(-)
drivers/spi/spi-bcm2835aux.c +1 -1
index b90aa34..169f521 100644
@@ -386,12 +386,12 @@ static int bcm2835aux_spi_prepare_message(struct spi_master *master,
 	bs->cntl[1] = BCM2835_AUX_SPI_CNTL1_MSBF_IN;
 
 	/* handle all the modes */
-	if (spi->mode & SPI_CPOL)
+	if (spi->mode & SPI_CPOL) {
 		bs->cntl[0] |= BCM2835_AUX_SPI_CNTL0_CPOL;

drivers/spi/spi-bcm2835aux.c static int bcm2835aux_spi_prepare_message(struct spi_master *master, · line 396

Quoted 10 lines
drivers/spi/spi-bcm2835aux.c +4 -4
-	if (spi->mode & SPI_CPHA)
-		bs->cntl[0] |= BCM2835_AUX_SPI_CNTL0_CPHA_OUT |
-			       BCM2835_AUX_SPI_CNTL0_CPHA_IN;
-
+		bs->cntl[0] |= BCM2835_AUX_SPI_CNTL0_CPHA_OUT;
+	} else {
+		bs->cntl[0] |= BCM2835_AUX_SPI_CNTL0_CPHA_IN;
+	}
 	bcm2835aux_wr(bs, BCM2835_AUX_SPI_CNTL1, bs->cntl[1]);
 	bcm2835aux_wr(bs, BCM2835_AUX_SPI_CNTL0, bs->cntl[0]);

(Note for other readers: A better name for CNTL0_CPHA_* would be CNTL0_*_RISING).

Quoted 8 lines

I think you're right about not actually supporting CPHA. I don't see wany way to keep bit 1 out lasting through the first full clock cycle. I think Stefan's right that we should drop CPHA from MODE_BITS (actually, MODE_BITS would be nicer if we just merged it into its one user, I think).

However, this hunk appears to be correct and would fix the timing of our data going out in the CPOL=0 case and of sampling IN data for CPOL=1.

Are you sure that CPHA is not working?

I have used the following to test all combinations:
for C in "" "-C"; do
for O in "" "-O"; do
for H in "" "-H"; do

    spidev_test -vD /dev/*.1 $H $O -p "\x00\x82\x00" -s 1000000;

done
done
done

I have tested with the 4.5-rc3 kernel without any of your patches.

And looked at the logic-analyzer: I see distinct waveform pattern for clk and data for all events!

The bug with regards to clock polarity needs to get fixed!

I will now apply patch4 only and see how it looks then...

Thanks,
Martin

#26 lore raw

PATCH 4/4 spi: bcm2835aux: fix CPOL/CPHA setting

Martin,

Am Thursday 11 February 2016, 16:25:43 schrieb Martin Sperl:

Quoted 64 lines
Quoted 48 lines · Eric Anholt
Quoted 39 lines · stephanolbrich at gmx.de

From: Stephan Olbrich <[email protected]>

The auxiliary spi supports only CPHA=0 modes as the first bit is always output to the pin before the first clock cycle. In CPHA=1 modes the first clock edge outputs the second bit hence the slave can never read the first bit.

Also the CPHA registers switch between clocking data in/out on rising/falling edge hence depend on the CPOL setting.

---
drivers/spi/spi-bcm2835aux.c | 10 +++++-----

1 file changed, 5 insertions(+), 5 deletions(-)
drivers/spi/spi-bcm2835aux.c +0 -0
index b90aa34..169f521 100644
@@ -386,12 +386,12 @@ static int bcm2835aux_spi_prepare_message(struct

spi_master *master,>>

	bs->cntl[1] = BCM2835_AUX_SPI_CNTL1_MSBF_IN;
	
	/* handle all the modes */
drivers/spi/spi-bcm2835aux.c +1 -1
-	if (spi->mode & SPI_CPOL)
+	if (spi->mode & SPI_CPOL) {
		bs->cntl[0] |= BCM2835_AUX_SPI_CNTL0_CPOL;
drivers/spi/spi-bcm2835aux.c +4 -4
-	if (spi->mode & SPI_CPHA)
-		bs->cntl[0] |= BCM2835_AUX_SPI_CNTL0_CPHA_OUT |
-			       BCM2835_AUX_SPI_CNTL0_CPHA_IN;
-
+		bs->cntl[0] |= BCM2835_AUX_SPI_CNTL0_CPHA_OUT;
+	} else {
+		bs->cntl[0] |= BCM2835_AUX_SPI_CNTL0_CPHA_IN;
+	}
Quoted 2 lines
	bcm2835aux_wr(bs, BCM2835_AUX_SPI_CNTL1, bs->cntl[1]);
	bcm2835aux_wr(bs, BCM2835_AUX_SPI_CNTL0, bs->cntl[0]);

(Note for other readers: A better name for CNTL0_CPHA_* would be CNTL0_*_RISING).

Quoted 8 lines

I think you're right about not actually supporting CPHA. I don't see wany way to keep bit 1 out lasting through the first full clock cycle. I think Stefan's right that we should drop CPHA from MODE_BITS (actually, MODE_BITS would be nicer if we just merged it into its one user, I think).

However, this hunk appears to be correct and would fix the timing of our data going out in the CPOL=0 case and of sampling IN data for CPOL=1.

Are you sure that CPHA is not working?

I have used the following to test all combinations:
for C in "" "-C"; do
for O in "" "-O"; do
for H in "" "-H"; do

    spidev_test -vD /dev/*.1 $H $O -p "\x00\x82\x00" -s 1000000;
Quoted 8 lines

done
done
done

I have tested with the 4.5-rc3 kernel without any of your patches.

And looked at the logic-analyzer: I see distinct waveform pattern for clk and data for all events!

The problem I see, is not that there is no waveform but that it has the wrong timing.
I'll make an example:
Data is 0xAA,0xFF, CPOL=1, CPHA=1
What happens is this:
CS goes active and MOSI goes high (first bit in 1)
After a short time, SCLK goes from low to high and MOSI goes to low (second bit is 0)
Then SCLK goes low (sampling data)
Then SCLK goes high and MOSI goes high (third bit is 1)
and so on.
At the end with the last rising edge of SCLK MOSI goes to low and stays there while SCLK goes to low and then CS to inactive.

So the slave has no chance to get the first bit but gets an additional 0 at the end.
It may not be easy visible with 0x00,0x82,0x00 as data, so could you repeat your test with 0xAA,0xFF if you see the same as I do or not?

Thanks,
Stephan

#27 lore raw

PATCH 4/4 spi: bcm2835aux: fix CPOL/CPHA setting

Quoted 93 lines · Stephan Olbrich

Martin,

Am Thursday 11 February 2016, 16:25:43 schrieb Martin Sperl:

Quoted 68 lines
Quoted 48 lines · Eric Anholt
Quoted 39 lines · stephanolbrich at gmx.de

From: Stephan Olbrich <[email protected]>

The auxiliary spi supports only CPHA=0 modes as the first bit is always output to the pin before the first clock cycle. In CPHA=1 modes the first clock edge outputs the second bit hence the slave can never read the first bit.

Also the CPHA registers switch between clocking data in/out on rising/falling edge hence depend on the CPOL setting.

---
drivers/spi/spi-bcm2835aux.c | 10 +++++-----

1 file changed, 5 insertions(+), 5 deletions(-)
drivers/spi/spi-bcm2835aux.c +0 -0
index b90aa34..169f521 100644
@@ -386,12 +386,12 @@ static int bcm2835aux_spi_prepare_message(struct

spi_master *master,>>

	bs->cntl[1] = BCM2835_AUX_SPI_CNTL1_MSBF_IN;
	
	/* handle all the modes */
drivers/spi/spi-bcm2835aux.c +1 -1
-	if (spi->mode & SPI_CPOL)
+	if (spi->mode & SPI_CPOL) {
		bs->cntl[0] |= BCM2835_AUX_SPI_CNTL0_CPOL;
drivers/spi/spi-bcm2835aux.c +4 -4
-	if (spi->mode & SPI_CPHA)
-		bs->cntl[0] |= BCM2835_AUX_SPI_CNTL0_CPHA_OUT |
-			       BCM2835_AUX_SPI_CNTL0_CPHA_IN;
-
+		bs->cntl[0] |= BCM2835_AUX_SPI_CNTL0_CPHA_OUT;
+	} else {
+		bs->cntl[0] |= BCM2835_AUX_SPI_CNTL0_CPHA_IN;
+	}
Quoted 2 lines
	bcm2835aux_wr(bs, BCM2835_AUX_SPI_CNTL1, bs->cntl[1]);
	bcm2835aux_wr(bs, BCM2835_AUX_SPI_CNTL0, bs->cntl[0]);

(Note for other readers: A better name for CNTL0_CPHA_* would be CNTL0_*_RISING).

Quoted 8 lines

I think you're right about not actually supporting CPHA. I don't see wany way to keep bit 1 out lasting through the first full clock cycle. I think Stefan's right that we should drop CPHA from MODE_BITS (actually, MODE_BITS would be nicer if we just merged it into its one user, I think).

However, this hunk appears to be correct and would fix the timing of our data going out in the CPOL=0 case and of sampling IN data for CPOL=1.

Are you sure that CPHA is not working?

I have used the following to test all combinations:
for C in "" "-C"; do
for O in "" "-O"; do
for H in "" "-H"; do
spidev_test -vD /dev/*.1 $H $O -p "\x00\x82\x00" -s 1000000;
done
done
done

Quoted 4 lines

I have tested with the 4.5-rc3 kernel without any of your patches.

And looked at the logic-analyzer: I see distinct waveform pattern for clk and data for all events!

The problem I see, is not that there is no waveform but that it has the wrong timing.
I'll make an example:
Data is 0xAA,0xFF, CPOL=1, CPHA=1
What happens is this:
CS goes active and MOSI goes high (first bit in 1)
After a short time, SCLK goes from low to high and MOSI goes to low (second bit is 0)
Then SCLK goes low (sampling data)
Then SCLK goes high and MOSI goes high (third bit is 1)
and so on.
At the end with the last rising edge of SCLK MOSI goes to low and stays there while SCLK goes to low and then CS to inactive.

Quoted 4 lines

So the slave has no chance to get the first bit but gets an additional 0 at the end.
It may not be easy visible with 0x00,0x82,0x00 as data, so could you repeat your test with 0xAA,0xFF if you see the same as I do or not?

I did retest also with 0x81,0x00,0x81 and saw my expected behavior - but maybe me understanding of those modes is wrong.

I will send you the images for the pattern I have generated with 0x81, 0x00, 0x81 as a personal email.

Martin

#28 lore raw

PATCH 4/4 spi: bcm2835aux: fix CPOL/CPHA setting

Quoted 8 lines · Martin Sperl
Quoted 6 lines

At the end with the last rising edge of SCLK MOSI goes to low and stays there while SCLK goes to low and then CS to inactive.

So the slave has no chance to get the first bit but gets an additional 0 at the end.

Actually SPI says: sample AT the clock change - not shortly after. So the behavior in principle is correct - even if it is not perfect...

Quoted 8 lines
Quoted 2 lines

It may not be easy visible with 0x00,0x82,0x00 as data, so could you repeat your test with 0xAA,0xFF if you see the same as I do or not?

I did retest also with 0x81,0x00,0x81 and saw my expected behavior - but maybe me understanding of those modes is wrong.

I will send you the images for the pattern I have generated with 0x81, 0x00, 0x81 as a personal email.

There is a means to add an additional HOLD time of 1, 4 or 7 system-clock cycles (undivided) that extend the period while MOSI is still driven with the old bit value after the clock change.

This can get fixed like this (copy paste, so tabs are spaces):

drivers/spi/spi-bcm2835aux.c +29 -0
index bd490c0..6052628 100644
@@ -63,6 +63,10 @@
 #define BCM2835_AUX_SPI_CNTL0_VAR_CS   0x00008000
 #define BCM2835_AUX_SPI_CNTL0_VAR_WIDTH        0x00004000
 #define BCM2835_AUX_SPI_CNTL0_DOUTHOLD 0x00003000
+#define BCM2835_AUX_SPI_CNTL0_DOUTHOLD_0       0x00000000
+#define BCM2835_AUX_SPI_CNTL0_DOUTHOLD_1       0x00001000
+#define BCM2835_AUX_SPI_CNTL0_DOUTHOLD_4       0x00002000
+#define BCM2835_AUX_SPI_CNTL0_DOUTHOLD_7       0x00003000
 #define BCM2835_AUX_SPI_CNTL0_ENABLE   0x00000800
 #define BCM2835_AUX_SPI_CNTL0_CPHA_IN  0x00000400
 #define BCM2835_AUX_SPI_CNTL0_CLEARFIFO        0x00000200
@@ -341,10 +345,35 @@ static int bcm2835aux_spi_transfer_one(struct spi_master *
        } else { /* the slowest we can go */
                speed = BCM2835_AUX_SPI_CNTL0_SPEED_MAX;
        }
+       /* mask out old speed from previous spi_transfer */
+       bs->cntl[0] &= ~BCM2835_AUX_SPI_CNTL0_SPEED;
+       /* and set speed for this round */
        bs->cntl[0] |= speed << BCM2835_AUX_SPI_CNTL0_SPEED_SHIFT;
 
        spi_used_hz = clk_hz / (2 * (speed + 1));
 
+       /* and configure hold time - needs to take speed into account */
+       /* first clear it */
+       bs->cntl[0] &= ~BCM2835_AUX_SPI_CNTL0_DOUTHOLD;
+       switch (speed) {
+       case 0: /* divider 2 */
+       case 1: /* divider 4 */
+               /* data hold time of 1 system clock cycle */
+               bs->cntl[0] |= BCM2835_AUX_SPI_CNTL0_DOUTHOLD_1;
+               break;
+       case 2: /* divider 6 */
+       case 3: /* divider 8 */
+       case 4: /* divider 10 */
+       case 5: /* divider 12 */
+               /* data hold time of 4 system clock cycle */
+               bs->cntl[0] |= BCM2835_AUX_SPI_CNTL0_DOUTHOLD_4;
+               break;
+       default: /* divider 14 and above */
+               /* data hold time of 7 system clock cycle */
+               bs->cntl[0] |= BCM2835_AUX_SPI_CNTL0_DOUTHOLD_7;
+               break;
+       }
+
        /* set transmit buffers and length */
        bs->tx_buf = tfr->tx_buf;
        bs->rx_buf = tfr->rx_buf;

It still means that CPHA=1 may fail to work with devices that run a software based SPI implementation that has a delay between clock change and sampling.
SPI slaves that latch the value on clock change should be fine.

Hopefully the policies for the delays were chosen wisely?

But for all practical purposes the DOUTHOLD is 7 systen clock cycles (undivided or .000000027s) for anything below 18.3MHz (=256MHz/(2*(1+6)).

If this is an unacceptable limitation, then we can disable CPHA support by removing SPI_CPHA from BCM2835_AUX_SPI_MODE_BITS.

Maybe by making it a configuration option in the device-tree?

Martin