[PATCH 0/4] spi: bcm2835aux: auxiliary spi improvements

[PATCH 0/4] spi: bcm2835aux: auxiliary spi improvements

[stephanolbrich-Mmb7MZpHnFY]

From: Stephan Olbrich <stephanolbrich-Mmb7MZpHnFY@public.gmane.org>

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(-)

-- 
2.5.0

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

[PATCH 0/4] spi: bcm2835aux: auxiliary spi improvements

[stephanolbrich at gmx.de]

From: Stephan Olbrich <stephanolbrich@gmx.de>

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(-)

-- 
2.5.0

[PATCH 1/4] spi: bcm2835aux: fix bitmask defines

[stephanolbrich-Mmb7MZpHnFY]

From: Stephan Olbrich <stephanolbrich-Mmb7MZpHnFY@public.gmane.org>

The bitmasks for txempty and idle interrupts were interchanged.

Signed-off-by: Stephan Olbrich <stephanolbrich-Mmb7MZpHnFY@public.gmane.org>
---
 drivers/spi/spi-bcm2835aux.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c
index 7de6f84..ecc73c0 100644
--- a/drivers/spi/spi-bcm2835aux.c
+++ b/drivers/spi/spi-bcm2835aux.c
@@ -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
 
-- 
2.5.0

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

[PATCH 1/4] spi: bcm2835aux: fix bitmask defines

[stephanolbrich at gmx.de]

From: Stephan Olbrich <stephanolbrich@gmx.de>

The bitmasks for txempty and idle interrupts were interchanged.

Signed-off-by: Stephan Olbrich <stephanolbrich@gmx.de>
---
 drivers/spi/spi-bcm2835aux.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c
index 7de6f84..ecc73c0 100644
--- a/drivers/spi/spi-bcm2835aux.c
+++ b/drivers/spi/spi-bcm2835aux.c
@@ -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
 
-- 
2.5.0

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

[stephanolbrich-Mmb7MZpHnFY]

From: Stephan Olbrich <stephanolbrich-Mmb7MZpHnFY@public.gmane.org>

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

Signed-off-by: Stephan Olbrich <stephanolbrich-Mmb7MZpHnFY@public.gmane.org>
---
 drivers/spi/spi-bcm2835aux.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c
index ecc73c0..d2f0067 100644
--- a/drivers/spi/spi-bcm2835aux.c
+++ b/drivers/spi/spi-bcm2835aux.c
@@ -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);
-- 
2.5.0

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

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

[stephanolbrich at gmx.de]

From: Stephan Olbrich <stephanolbrich@gmx.de>

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

Signed-off-by: Stephan Olbrich <stephanolbrich@gmx.de>
---
 drivers/spi/spi-bcm2835aux.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c
index ecc73c0..d2f0067 100644
--- a/drivers/spi/spi-bcm2835aux.c
+++ b/drivers/spi/spi-bcm2835aux.c
@@ -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);
-- 
2.5.0

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

[stephanolbrich-Mmb7MZpHnFY]

From: Stephan Olbrich <stephanolbrich-Mmb7MZpHnFY@public.gmane.org>

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.

Signed-off-by: Stephan Olbrich <stephanolbrich-Mmb7MZpHnFY@public.gmane.org>
---
 drivers/spi/spi-bcm2835aux.c | 54 +++++++++++++++++++++++++++++++-------------
 1 file changed, 38 insertions(+), 16 deletions(-)

diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c
index d2f0067..b90aa34 100644
--- a/drivers/spi/spi-bcm2835aux.c
+++ b/drivers/spi/spi-bcm2835aux.c
@@ -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);
-- 
2.5.0

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

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

[stephanolbrich at gmx.de]

From: Stephan Olbrich <stephanolbrich@gmx.de>

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.

Signed-off-by: Stephan Olbrich <stephanolbrich@gmx.de>
---
 drivers/spi/spi-bcm2835aux.c | 54 +++++++++++++++++++++++++++++++-------------
 1 file changed, 38 insertions(+), 16 deletions(-)

diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c
index d2f0067..b90aa34 100644
--- a/drivers/spi/spi-bcm2835aux.c
+++ b/drivers/spi/spi-bcm2835aux.c
@@ -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);
-- 
2.5.0

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

[stephanolbrich-Mmb7MZpHnFY]

From: Stephan Olbrich <stephanolbrich-Mmb7MZpHnFY@public.gmane.org>

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.

Signed-off-by: Stephan Olbrich <stephanolbrich-Mmb7MZpHnFY@public.gmane.org>
---
 drivers/spi/spi-bcm2835aux.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c
index b90aa34..169f521 100644
--- a/drivers/spi/spi-bcm2835aux.c
+++ b/drivers/spi/spi-bcm2835aux.c
@@ -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]);
 
-- 
2.5.0

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

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

[stephanolbrich at gmx.de]

From: Stephan Olbrich <stephanolbrich@gmx.de>

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.

Signed-off-by: Stephan Olbrich <stephanolbrich@gmx.de>
---
 drivers/spi/spi-bcm2835aux.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c
index b90aa34..169f521 100644
--- a/drivers/spi/spi-bcm2835aux.c
+++ b/drivers/spi/spi-bcm2835aux.c
@@ -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]);
 
-- 
2.5.0

Re: [PATCH 1/4] spi: bcm2835aux: fix bitmask defines

[Stefan Wahren]

Hi Stephan,

Am 09.02.2016 um 19:10 schrieb stephanolbrich-Mmb7MZpHnFY@public.gmane.org:
> From: Stephan Olbrich <stephanolbrich-Mmb7MZpHnFY@public.gmane.org>
>
> The bitmasks for txempty and idle interrupts were interchanged.
>
> Signed-off-by: Stephan Olbrich <stephanolbrich-Mmb7MZpHnFY@public.gmane.org>
> ---
>   drivers/spi/spi-bcm2835aux.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c
> index 7de6f84..ecc73c0 100644
> --- a/drivers/spi/spi-bcm2835aux.c
> +++ b/drivers/spi/spi-bcm2835aux.c
> @@ -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

> +#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
>



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

[PATCH 1/4] spi: bcm2835aux: fix bitmask defines

[Stefan Wahren]

Hi Stephan,

Am 09.02.2016 um 19:10 schrieb stephanolbrich at gmx.de:
> From: Stephan Olbrich <stephanolbrich@gmx.de>
>
> The bitmasks for txempty and idle interrupts were interchanged.
>
> Signed-off-by: Stephan Olbrich <stephanolbrich@gmx.de>
> ---
>   drivers/spi/spi-bcm2835aux.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c
> index 7de6f84..ecc73c0 100644
> --- a/drivers/spi/spi-bcm2835aux.c
> +++ b/drivers/spi/spi-bcm2835aux.c
> @@ -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

> +#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
>

Re: [PATCH 4/4] spi: bcm2835aux: fix CPOL/CPHA setting

[Stefan Wahren]

Hi Stephan,

Am 09.02.2016 um 19:10 schrieb stephanolbrich-Mmb7MZpHnFY@public.gmane.org:
> From: Stephan Olbrich <stephanolbrich-Mmb7MZpHnFY@public.gmane.org>
>
> 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
--
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

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

[Stefan Wahren]

Hi Stephan,

Am 09.02.2016 um 19:10 schrieb stephanolbrich at gmx.de:
> From: Stephan Olbrich <stephanolbrich@gmx.de>
>
> 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

Re: [PATCH 1/4] spi: bcm2835aux: fix bitmask defines

[Eric Anholt]

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

stephanolbrich-Mmb7MZpHnFY@public.gmane.org writes:

> From: Stephan Olbrich <stephanolbrich-Mmb7MZpHnFY@public.gmane.org>
>
> The bitmasks for txempty and idle interrupts were interchanged.
>
> Signed-off-by: Stephan Olbrich <stephanolbrich-Mmb7MZpHnFY@public.gmane.org>
> ---
>  drivers/spi/spi-bcm2835aux.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c
> index 7de6f84..ecc73c0 100644
> --- a/drivers/spi/spi-bcm2835aux.c
> +++ b/drivers/spi/spi-bcm2835aux.c
> @@ -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.

Reviewed-by: Eric Anholt <eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

[PATCH 1/4] spi: bcm2835aux: fix bitmask defines

[Eric Anholt]

stephanolbrich at gmx.de writes:

> From: Stephan Olbrich <stephanolbrich@gmx.de>
>
> The bitmasks for txempty and idle interrupts were interchanged.
>
> Signed-off-by: Stephan Olbrich <stephanolbrich@gmx.de>
> ---
>  drivers/spi/spi-bcm2835aux.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c
> index 7de6f84..ecc73c0 100644
> --- a/drivers/spi/spi-bcm2835aux.c
> +++ b/drivers/spi/spi-bcm2835aux.c
> @@ -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.

Reviewed-by: Eric Anholt <eric@anholt.net>
-------------- 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>

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

[Eric Anholt]

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

stephanolbrich-Mmb7MZpHnFY@public.gmane.org writes:

> From: Stephan Olbrich <stephanolbrich-Mmb7MZpHnFY@public.gmane.org>
>
> The tx empty irq can be disabled when all data was copied.
> This prevents unnecessary interrupts while the last bytes are sent.
>
> Signed-off-by: Stephan Olbrich <stephanolbrich-Mmb7MZpHnFY@public.gmane.org>
> ---
>  drivers/spi/spi-bcm2835aux.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c
> index ecc73c0..d2f0067 100644
> --- a/drivers/spi/spi-bcm2835aux.c
> +++ b/drivers/spi/spi-bcm2835aux.c
> @@ -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);
> -- 
> 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.

Reviewed-by: Eric Anholt <eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org>

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

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

[Eric Anholt]

stephanolbrich at gmx.de writes:

> From: Stephan Olbrich <stephanolbrich@gmx.de>
>
> The tx empty irq can be disabled when all data was copied.
> This prevents unnecessary interrupts while the last bytes are sent.
>
> Signed-off-by: Stephan Olbrich <stephanolbrich@gmx.de>
> ---
>  drivers/spi/spi-bcm2835aux.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c
> index ecc73c0..d2f0067 100644
> --- a/drivers/spi/spi-bcm2835aux.c
> +++ b/drivers/spi/spi-bcm2835aux.c
> @@ -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);
> -- 
> 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.

Reviewed-by: Eric Anholt <eric@anholt.net>

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>

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

[Eric Anholt]

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

stephanolbrich-Mmb7MZpHnFY@public.gmane.org writes:

> From: Stephan Olbrich <stephanolbrich-Mmb7MZpHnFY@public.gmane.org>
>
> 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...

> Signed-off-by: Stephan Olbrich <stephanolbrich-Mmb7MZpHnFY@public.gmane.org>
> ---
>  drivers/spi/spi-bcm2835aux.c | 54 +++++++++++++++++++++++++++++++-------------
>  1 file changed, 38 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c
> index d2f0067..b90aa34 100644
> --- a/drivers/spi/spi-bcm2835aux.c
> +++ b/drivers/spi/spi-bcm2835aux.c    
> @@ -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;

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

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

[Eric Anholt]

stephanolbrich at gmx.de writes:

> From: Stephan Olbrich <stephanolbrich@gmx.de>
>
> 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...

> Signed-off-by: Stephan Olbrich <stephanolbrich@gmx.de>
> ---
>  drivers/spi/spi-bcm2835aux.c | 54 +++++++++++++++++++++++++++++++-------------
>  1 file changed, 38 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c
> index d2f0067..b90aa34 100644
> --- a/drivers/spi/spi-bcm2835aux.c
> +++ b/drivers/spi/spi-bcm2835aux.c    
> @@ -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;

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>

Re: [PATCH 4/4] spi: bcm2835aux: fix CPOL/CPHA setting

[Eric Anholt]

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

stephanolbrich-Mmb7MZpHnFY@public.gmane.org writes:

> From: Stephan Olbrich <stephanolbrich-Mmb7MZpHnFY@public.gmane.org>
>
> 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.
>
> Signed-off-by: Stephan Olbrich <stephanolbrich-Mmb7MZpHnFY@public.gmane.org>
> ---
>  drivers/spi/spi-bcm2835aux.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c
> index b90aa34..169f521 100644
> --- a/drivers/spi/spi-bcm2835aux.c
> +++ b/drivers/spi/spi-bcm2835aux.c
> @@ -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]);

(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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

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

[Eric Anholt]

stephanolbrich at gmx.de writes:

> From: Stephan Olbrich <stephanolbrich@gmx.de>
>
> 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.
>
> Signed-off-by: Stephan Olbrich <stephanolbrich@gmx.de>
> ---
>  drivers/spi/spi-bcm2835aux.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c
> index b90aa34..169f521 100644
> --- a/drivers/spi/spi-bcm2835aux.c
> +++ b/drivers/spi/spi-bcm2835aux.c
> @@ -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]);

(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>

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

[Mark Brown]

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

On Tue, Feb 09, 2016 at 03:49:24PM -0800, Eric Anholt wrote:

> 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.

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

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

[Mark Brown]

On Tue, Feb 09, 2016 at 03:49:24PM -0800, Eric Anholt wrote:

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

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

[Mark Brown]

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

>From fe0e2304f560f81c1673711ac3f9a8c7c3cbb8be Mon Sep 17 00:00:00 2001
From: Stephan Olbrich <stephanolbrich-Mmb7MZpHnFY@public.gmane.org>
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.

Signed-off-by: Stephan Olbrich <stephanolbrich-Mmb7MZpHnFY@public.gmane.org>
Reviewed-by: Eric Anholt <eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org>
Signed-off-by: Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 drivers/spi/spi-bcm2835aux.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c
index 7de6f8472a81..ecc73c0a97cf 100644
--- a/drivers/spi/spi-bcm2835aux.c
+++ b/drivers/spi/spi-bcm2835aux.c
@@ -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
 
-- 
2.7.0

--
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/4] spi: bcm2835aux: set up spi-mode before asserting cs-gpio

[Eric Anholt]

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

Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> writes:

> On Tue, Feb 09, 2016 at 03:49:24PM -0800, Eric Anholt wrote:
>
>> 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).

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

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

[Eric Anholt]

Mark Brown <broonie@kernel.org> writes:

> On Tue, Feb 09, 2016 at 03:49:24PM -0800, Eric Anholt wrote:
>
>> 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>

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

[Mark Brown]

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

On Wed, Feb 10, 2016 at 10:59:29AM -0800, Eric Anholt wrote:

> 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.

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

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

[Mark Brown]

On Wed, Feb 10, 2016 at 10:59:29AM -0800, Eric Anholt wrote:

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

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

[Mark Brown]

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

>From f29ab1845f3e2684ba1c6de6c3bd5198e4b1459c Mon Sep 17 00:00:00 2001
From: Stephan Olbrich <stephanolbrich-Mmb7MZpHnFY@public.gmane.org>
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.

Signed-off-by: Stephan Olbrich <stephanolbrich-Mmb7MZpHnFY@public.gmane.org>
Reviewed-by: Eric Anholt <eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org>
Signed-off-by: Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 drivers/spi/spi-bcm2835aux.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c
index 7de6f8472a81..e1b2fec1db63 100644
--- a/drivers/spi/spi-bcm2835aux.c
+++ b/drivers/spi/spi-bcm2835aux.c
@@ -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);
-- 
2.7.0

--
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 1/4] spi: bcm2835aux: fix bitmask defines

[Stephan Olbrich]

Hi Stefan

Am Tuesday 09 February 2016, 20:54:03 schrieb Stefan Wahren:
> Hi Stephan,
> 
> Am 09.02.2016 um 19:10 schrieb stephanolbrich-Mmb7MZpHnFY@public.gmane.org:
> > From: Stephan Olbrich <stephanolbrich-Mmb7MZpHnFY@public.gmane.org>
> > 
> > The bitmasks for txempty and idle interrupts were interchanged.
> > 
> > Signed-off-by: Stephan Olbrich <stephanolbrich-Mmb7MZpHnFY@public.gmane.org>
> > ---
> > 
> >   drivers/spi/spi-bcm2835aux.c | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c
> > index 7de6f84..ecc73c0 100644
> > --- a/drivers/spi/spi-bcm2835aux.c
> > +++ b/drivers/spi/spi-bcm2835aux.c
> > @@ -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.

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?

> [1] -
> https://www.raspberrypi.org/wp-content/uploads/2012/02/BCM2835-ARM-Periphera
> ls.pdf
> > +#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-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> http://lists.infradead.org/mailman/listinfo/linux-rpi-kernel

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

[PATCH 1/4] spi: bcm2835aux: fix bitmask defines

[Stephan Olbrich]

Hi Stefan

Am Tuesday 09 February 2016, 20:54:03 schrieb Stefan Wahren:
> Hi Stephan,
> 
> Am 09.02.2016 um 19:10 schrieb stephanolbrich at gmx.de:
> > From: Stephan Olbrich <stephanolbrich@gmx.de>
> > 
> > The bitmasks for txempty and idle interrupts were interchanged.
> > 
> > Signed-off-by: Stephan Olbrich <stephanolbrich@gmx.de>
> > ---
> > 
> >   drivers/spi/spi-bcm2835aux.c | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c
> > index 7de6f84..ecc73c0 100644
> > --- a/drivers/spi/spi-bcm2835aux.c
> > +++ b/drivers/spi/spi-bcm2835aux.c
> > @@ -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.

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?

> [1] -
> https://www.raspberrypi.org/wp-content/uploads/2012/02/BCM2835-ARM-Periphera
> ls.pdf
> > +#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

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

[Stephan Olbrich]

Am Wednesday 10 February 2016, 19:02:04 schrieb Mark Brown:
> On Wed, Feb 10, 2016 at 10:59:29AM -0800, Eric Anholt wrote:
> > 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.
--
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

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

[Stephan Olbrich]

Am Wednesday 10 February 2016, 19:02:04 schrieb Mark Brown:
> On Wed, Feb 10, 2016 at 10:59:29AM -0800, Eric Anholt wrote:
> > 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.

Re: [PATCH 4/4] spi: bcm2835aux: fix CPOL/CPHA setting

[Stephan Olbrich]

Am Tuesday 09 February 2016, 16:13:06 schrieb Eric Anholt:
> stephanolbrich-Mmb7MZpHnFY@public.gmane.org writes:
> > From: Stephan Olbrich <stephanolbrich-Mmb7MZpHnFY@public.gmane.org>
> > 
> > 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.
> > 
> > Signed-off-by: Stephan Olbrich <stephanolbrich-Mmb7MZpHnFY@public.gmane.org>
> > ---
> > 
> >  drivers/spi/spi-bcm2835aux.c | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c
> > index b90aa34..169f521 100644
> > --- a/drivers/spi/spi-bcm2835aux.c
> > +++ b/drivers/spi/spi-bcm2835aux.c
> > @@ -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]);
> 
> (Note for other readers: A better name for CNTL0_CPHA_* would be
> CNTL0_*_RISING).

Should I rename them? 

> 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.

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

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

[Stephan Olbrich]

Am Tuesday 09 February 2016, 16:13:06 schrieb Eric Anholt:
> stephanolbrich at gmx.de writes:
> > From: Stephan Olbrich <stephanolbrich@gmx.de>
> > 
> > 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.
> > 
> > Signed-off-by: Stephan Olbrich <stephanolbrich@gmx.de>
> > ---
> > 
> >  drivers/spi/spi-bcm2835aux.c | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c
> > index b90aa34..169f521 100644
> > --- a/drivers/spi/spi-bcm2835aux.c
> > +++ b/drivers/spi/spi-bcm2835aux.c
> > @@ -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]);
> 
> (Note for other readers: A better name for CNTL0_CPHA_* would be
> CNTL0_*_RISING).

Should I rename them? 

> 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.

Re: [PATCH 4/4] spi: bcm2835aux: fix CPOL/CPHA setting

[Eric Anholt]

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

Stephan Olbrich <stephanolbrich-Mmb7MZpHnFY@public.gmane.org> writes:

> Am Tuesday 09 February 2016, 16:13:06 schrieb Eric Anholt:
>> stephanolbrich-Mmb7MZpHnFY@public.gmane.org writes:
>> > From: Stephan Olbrich <stephanolbrich-Mmb7MZpHnFY@public.gmane.org>
>> > 
>> > 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.
>> > 
>> > Signed-off-by: Stephan Olbrich <stephanolbrich-Mmb7MZpHnFY@public.gmane.org>
>> > ---
>> > 
>> >  drivers/spi/spi-bcm2835aux.c | 10 +++++-----
>> >  1 file changed, 5 insertions(+), 5 deletions(-)
>> > 
>> > diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c
>> > index b90aa34..169f521 100644
>> > --- a/drivers/spi/spi-bcm2835aux.c
>> > +++ b/drivers/spi/spi-bcm2835aux.c
>> > @@ -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]);
>> 
>> (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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

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

[Eric Anholt]

Stephan Olbrich <stephanolbrich@gmx.de> writes:

> Am Tuesday 09 February 2016, 16:13:06 schrieb Eric Anholt:
>> stephanolbrich at gmx.de writes:
>> > From: Stephan Olbrich <stephanolbrich@gmx.de>
>> > 
>> > 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.
>> > 
>> > Signed-off-by: Stephan Olbrich <stephanolbrich@gmx.de>
>> > ---
>> > 
>> >  drivers/spi/spi-bcm2835aux.c | 10 +++++-----
>> >  1 file changed, 5 insertions(+), 5 deletions(-)
>> > 
>> > diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c
>> > index b90aa34..169f521 100644
>> > --- a/drivers/spi/spi-bcm2835aux.c
>> > +++ b/drivers/spi/spi-bcm2835aux.c
>> > @@ -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]);
>> 
>> (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>

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

[Martin Sperl]

> On 10.02.2016, at 21:26, Stephan Olbrich <stephanolbrich-Mmb7MZpHnFY@public.gmane.org> wrote:
> 
> Am Wednesday 10 February 2016, 19:02:04 schrieb Mark Brown:
>> On Wed, Feb 10, 2016 at 10:59:29AM -0800, Eric Anholt wrote:
>>> 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.

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

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

[Martin Sperl]

> On 10.02.2016, at 21:26, Stephan Olbrich <stephanolbrich@gmx.de> wrote:
> 
> Am Wednesday 10 February 2016, 19:02:04 schrieb Mark Brown:
>> On Wed, Feb 10, 2016 at 10:59:29AM -0800, Eric Anholt wrote:
>>> 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.

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

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

[Mark Brown]

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

On Wed, Feb 10, 2016 at 09:26:05PM +0100, Stephan Olbrich wrote:
> Am Wednesday 10 February 2016, 19:02:04 schrieb Mark Brown:

> > 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.

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

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

[Mark Brown]

On Wed, Feb 10, 2016 at 09:26:05PM +0100, Stephan Olbrich wrote:
> Am Wednesday 10 February 2016, 19:02:04 schrieb Mark Brown:

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

Re: [PATCH 4/4] spi: bcm2835aux: fix CPOL/CPHA setting

[Martin Sperl]

> On 10.02.2016, at 01:13, Eric Anholt <eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org> wrote:
> 
> stephanolbrich-Mmb7MZpHnFY@public.gmane.org writes:
> 
>> From: Stephan Olbrich <stephanolbrich-Mmb7MZpHnFY@public.gmane.org>
>> 
>> 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.
>> 
>> Signed-off-by: Stephan Olbrich <stephanolbrich-Mmb7MZpHnFY@public.gmane.org>
>> ---
>> drivers/spi/spi-bcm2835aux.c | 10 +++++-----
>> 1 file changed, 5 insertions(+), 5 deletions(-)
>> 
>> diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c
>> index b90aa34..169f521 100644
>> --- a/drivers/spi/spi-bcm2835aux.c
>> +++ b/drivers/spi/spi-bcm2835aux.c
>> @@ -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]);
> 
> (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.

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





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

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

[Martin Sperl]

> On 10.02.2016, at 01:13, Eric Anholt <eric@anholt.net> wrote:
> 
> stephanolbrich at gmx.de writes:
> 
>> From: Stephan Olbrich <stephanolbrich@gmx.de>
>> 
>> 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.
>> 
>> Signed-off-by: Stephan Olbrich <stephanolbrich@gmx.de>
>> ---
>> drivers/spi/spi-bcm2835aux.c | 10 +++++-----
>> 1 file changed, 5 insertions(+), 5 deletions(-)
>> 
>> diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c
>> index b90aa34..169f521 100644
>> --- a/drivers/spi/spi-bcm2835aux.c
>> +++ b/drivers/spi/spi-bcm2835aux.c
>> @@ -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]);
> 
> (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.

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

Re: [PATCH 4/4] spi: bcm2835aux: fix CPOL/CPHA setting

[Stephan Olbrich]

Martin,

Am Thursday 11 February 2016, 16:25:43 schrieb Martin Sperl:
> > On 10.02.2016, at 01:13, Eric Anholt <eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org> wrote:
> > 
> > stephanolbrich-Mmb7MZpHnFY@public.gmane.org writes:
> >> From: Stephan Olbrich <stephanolbrich-Mmb7MZpHnFY@public.gmane.org>
> >> 
> >> 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.
> >> 
> >> Signed-off-by: Stephan Olbrich <stephanolbrich-Mmb7MZpHnFY@public.gmane.org>
> >> ---
> >> drivers/spi/spi-bcm2835aux.c | 10 +++++-----
> >> 1 file changed, 5 insertions(+), 5 deletions(-)
> >> 
> >> diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c
> >> index b90aa34..169f521 100644
> >> --- a/drivers/spi/spi-bcm2835aux.c
> >> +++ b/drivers/spi/spi-bcm2835aux.c
> >> @@ -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]);
> > 
> > (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.
> 
> 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 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

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

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

[Stephan Olbrich]

Martin,

Am Thursday 11 February 2016, 16:25:43 schrieb Martin Sperl:
> > On 10.02.2016, at 01:13, Eric Anholt <eric@anholt.net> wrote:
> > 
> > stephanolbrich at gmx.de writes:
> >> From: Stephan Olbrich <stephanolbrich@gmx.de>
> >> 
> >> 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.
> >> 
> >> Signed-off-by: Stephan Olbrich <stephanolbrich@gmx.de>
> >> ---
> >> drivers/spi/spi-bcm2835aux.c | 10 +++++-----
> >> 1 file changed, 5 insertions(+), 5 deletions(-)
> >> 
> >> diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c
> >> index b90aa34..169f521 100644
> >> --- a/drivers/spi/spi-bcm2835aux.c
> >> +++ b/drivers/spi/spi-bcm2835aux.c
> >> @@ -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]);
> > 
> > (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.
> 
> 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 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

Re: [PATCH 4/4] spi: bcm2835aux: fix CPOL/CPHA setting

[Martin Sperl]

> On 11.02.2016, at 17:05, Stephan Olbrich <stephanolbrich-Mmb7MZpHnFY@public.gmane.org> wrote:
> 
> Martin,
> 
> Am Thursday 11 February 2016, 16:25:43 schrieb Martin Sperl:
>>> On 10.02.2016, at 01:13, Eric Anholt <eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org> wrote:
>>> 
>>> stephanolbrich-Mmb7MZpHnFY@public.gmane.org writes:
>>>> From: Stephan Olbrich <stephanolbrich-Mmb7MZpHnFY@public.gmane.org>
>>>> 
>>>> 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.
>>>> 
>>>> Signed-off-by: Stephan Olbrich <stephanolbrich-Mmb7MZpHnFY@public.gmane.org>
>>>> ---
>>>> drivers/spi/spi-bcm2835aux.c | 10 +++++-----
>>>> 1 file changed, 5 insertions(+), 5 deletions(-)
>>>> 
>>>> diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c
>>>> index b90aa34..169f521 100644
>>>> --- a/drivers/spi/spi-bcm2835aux.c
>>>> +++ b/drivers/spi/spi-bcm2835aux.c
>>>> @@ -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]);
>>> 
>>> (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.
>> 
>> 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 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?

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

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

[Martin Sperl]

> On 11.02.2016, at 17:05, Stephan Olbrich <stephanolbrich@gmx.de> wrote:
> 
> Martin,
> 
> Am Thursday 11 February 2016, 16:25:43 schrieb Martin Sperl:
>>> On 10.02.2016, at 01:13, Eric Anholt <eric@anholt.net> wrote:
>>> 
>>> stephanolbrich at gmx.de writes:
>>>> From: Stephan Olbrich <stephanolbrich@gmx.de>
>>>> 
>>>> 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.
>>>> 
>>>> Signed-off-by: Stephan Olbrich <stephanolbrich@gmx.de>
>>>> ---
>>>> drivers/spi/spi-bcm2835aux.c | 10 +++++-----
>>>> 1 file changed, 5 insertions(+), 5 deletions(-)
>>>> 
>>>> diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c
>>>> index b90aa34..169f521 100644
>>>> --- a/drivers/spi/spi-bcm2835aux.c
>>>> +++ b/drivers/spi/spi-bcm2835aux.c
>>>> @@ -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]);
>>> 
>>> (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.
>> 
>> 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 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?

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

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

[Martin Sperl]

> On 09.02.2016, at 19:10, stephanolbrich-Mmb7MZpHnFY@public.gmane.org wrote:
> 
> From: Stephan Olbrich <stephanolbrich-Mmb7MZpHnFY@public.gmane.org>
> 
> 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.
> 
> Signed-off-by: Stephan Olbrich <stephanolbrich-Mmb7MZpHnFY@public.gmane.org>
> ---
> drivers/spi/spi-bcm2835aux.c | 54 +++++++++++++++++++++++++++++++-------------
> 1 file changed, 38 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c
> index d2f0067..b90aa34 100644
> --- a/drivers/spi/spi-bcm2835aux.c
> +++ b/drivers/spi/spi-bcm2835aux.c
> @@ -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;

> 
> 	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_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:
Reviewed-by: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
Tested-by: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>

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

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

[Martin Sperl]

> On 09.02.2016, at 19:10, stephanolbrich at gmx.de wrote:
> 
> From: Stephan Olbrich <stephanolbrich@gmx.de>
> 
> 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.
> 
> Signed-off-by: Stephan Olbrich <stephanolbrich@gmx.de>
> ---
> drivers/spi/spi-bcm2835aux.c | 54 +++++++++++++++++++++++++++++++-------------
> 1 file changed, 38 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c
> index d2f0067..b90aa34 100644
> --- a/drivers/spi/spi-bcm2835aux.c
> +++ b/drivers/spi/spi-bcm2835aux.c
> @@ -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;

> 
> 	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_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:
Reviewed-by: Martin Sperl <kernel@martin.sperl.org>
Tested-by: Martin Sperl <kernel@martin.sperl.org>

Martin

Re: [PATCH 4/4] spi: bcm2835aux: fix CPOL/CPHA setting

[Martin Sperl]

> On 11.02.2016, at 17:19, Martin Sperl <martin-d5rIkyn9cnPYtjvyW6yDsg@public.gmane.org> wrote:

>> 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...

>> 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):

diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c
index bd490c0..6052628 100644
--- a/drivers/spi/spi-bcm2835aux.c
+++ b/drivers/spi/spi-bcm2835aux.c
@@ -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--
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

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

[Martin Sperl]

> On 11.02.2016, at 17:19, Martin Sperl <martin@sperl.org> wrote:

>> 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...

>> 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):

diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c
index bd490c0..6052628 100644
--- a/drivers/spi/spi-bcm2835aux.c
+++ b/drivers/spi/spi-bcm2835aux.c
@@ -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