* [PATCH 0/4] spi: bcm2835aux: auxiliary spi improvements
@ 2016-02-09 18:10 ` stephanolbrich at gmx.de
0 siblings, 0 replies; 52+ messages in thread
From: stephanolbrich-Mmb7MZpHnFY @ 2016-02-09 18:10 UTC (permalink / raw)
To: Mark Brown, Stephen Warren, Lee Jones, Eric Anholt,
linux-spi-u79uwXL29TY76Z2rM5mHXA,
linux-rpi-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
Cc: Stephan Olbrich
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
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH 0/4] spi: bcm2835aux: auxiliary spi improvements
@ 2016-02-09 18:10 ` stephanolbrich at gmx.de
0 siblings, 0 replies; 52+ messages in thread
From: stephanolbrich at gmx.de @ 2016-02-09 18:10 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH 1/4] spi: bcm2835aux: fix bitmask defines
2016-02-09 18:10 ` stephanolbrich at gmx.de
@ 2016-02-09 18:10 ` stephanolbrich at gmx.de
-1 siblings, 0 replies; 52+ messages in thread
From: stephanolbrich-Mmb7MZpHnFY @ 2016-02-09 18:10 UTC (permalink / raw)
To: Mark Brown, Stephen Warren, Lee Jones, Eric Anholt,
linux-spi-u79uwXL29TY76Z2rM5mHXA,
linux-rpi-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
Cc: Stephan Olbrich
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
^ permalink raw reply related [flat|nested] 52+ messages in thread
* [PATCH 1/4] spi: bcm2835aux: fix bitmask defines
@ 2016-02-09 18:10 ` stephanolbrich at gmx.de
0 siblings, 0 replies; 52+ messages in thread
From: stephanolbrich at gmx.de @ 2016-02-09 18:10 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply related [flat|nested] 52+ messages in thread
* [PATCH 2/4] spi: bcm2835aux: disable tx fifo empty irq
2016-02-09 18:10 ` stephanolbrich at gmx.de
@ 2016-02-09 18:10 ` stephanolbrich at gmx.de
-1 siblings, 0 replies; 52+ messages in thread
From: stephanolbrich-Mmb7MZpHnFY @ 2016-02-09 18:10 UTC (permalink / raw)
To: Mark Brown, Stephen Warren, Lee Jones, Eric Anholt,
linux-spi-u79uwXL29TY76Z2rM5mHXA,
linux-rpi-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
Cc: Stephan Olbrich
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
^ permalink raw reply related [flat|nested] 52+ messages in thread
* [PATCH 2/4] spi: bcm2835aux: disable tx fifo empty irq
@ 2016-02-09 18:10 ` stephanolbrich at gmx.de
0 siblings, 0 replies; 52+ messages in thread
From: stephanolbrich at gmx.de @ 2016-02-09 18:10 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply related [flat|nested] 52+ messages in thread
* [PATCH 3/4] spi: bcm2835aux: set up spi-mode before asserting cs-gpio
2016-02-09 18:10 ` stephanolbrich at gmx.de
@ 2016-02-09 18:10 ` stephanolbrich at gmx.de
-1 siblings, 0 replies; 52+ messages in thread
From: stephanolbrich-Mmb7MZpHnFY @ 2016-02-09 18:10 UTC (permalink / raw)
To: Mark Brown, Stephen Warren, Lee Jones, Eric Anholt,
linux-spi-u79uwXL29TY76Z2rM5mHXA,
linux-rpi-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
Cc: Stephan Olbrich
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
^ permalink raw reply related [flat|nested] 52+ messages in thread
* [PATCH 3/4] spi: bcm2835aux: set up spi-mode before asserting cs-gpio
@ 2016-02-09 18:10 ` stephanolbrich at gmx.de
0 siblings, 0 replies; 52+ messages in thread
From: stephanolbrich at gmx.de @ 2016-02-09 18:10 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply related [flat|nested] 52+ messages in thread
* [PATCH 4/4] spi: bcm2835aux: fix CPOL/CPHA setting
2016-02-09 18:10 ` stephanolbrich at gmx.de
@ 2016-02-09 18:10 ` stephanolbrich at gmx.de
-1 siblings, 0 replies; 52+ messages in thread
From: stephanolbrich-Mmb7MZpHnFY @ 2016-02-09 18:10 UTC (permalink / raw)
To: Mark Brown, Stephen Warren, Lee Jones, Eric Anholt,
linux-spi-u79uwXL29TY76Z2rM5mHXA,
linux-rpi-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
Cc: Stephan Olbrich
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
^ permalink raw reply related [flat|nested] 52+ messages in thread
* [PATCH 4/4] spi: bcm2835aux: fix CPOL/CPHA setting
@ 2016-02-09 18:10 ` stephanolbrich at gmx.de
0 siblings, 0 replies; 52+ messages in thread
From: stephanolbrich at gmx.de @ 2016-02-09 18:10 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply related [flat|nested] 52+ messages in thread
* Re: [PATCH 1/4] spi: bcm2835aux: fix bitmask defines
2016-02-09 18:10 ` stephanolbrich at gmx.de
@ 2016-02-09 19:54 ` Stefan Wahren
-1 siblings, 0 replies; 52+ messages in thread
From: Stefan Wahren @ 2016-02-09 19:54 UTC (permalink / raw)
To: stephanolbrich-Mmb7MZpHnFY, Mark Brown, Stephen Warren,
Lee Jones, Eric Anholt, linux-spi-u79uwXL29TY76Z2rM5mHXA,
linux-rpi-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
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
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH 1/4] spi: bcm2835aux: fix bitmask defines
@ 2016-02-09 19:54 ` Stefan Wahren
0 siblings, 0 replies; 52+ messages in thread
From: Stefan Wahren @ 2016-02-09 19:54 UTC (permalink / raw)
To: linux-arm-kernel
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
>
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH 4/4] spi: bcm2835aux: fix CPOL/CPHA setting
2016-02-09 18:10 ` stephanolbrich at gmx.de
@ 2016-02-09 20:21 ` Stefan Wahren
-1 siblings, 0 replies; 52+ messages in thread
From: Stefan Wahren @ 2016-02-09 20:21 UTC (permalink / raw)
To: stephanolbrich-Mmb7MZpHnFY, Mark Brown, Stephen Warren,
Lee Jones, Eric Anholt, linux-spi-u79uwXL29TY76Z2rM5mHXA,
linux-rpi-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
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
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH 4/4] spi: bcm2835aux: fix CPOL/CPHA setting
@ 2016-02-09 20:21 ` Stefan Wahren
0 siblings, 0 replies; 52+ messages in thread
From: Stefan Wahren @ 2016-02-09 20:21 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH 1/4] spi: bcm2835aux: fix bitmask defines
2016-02-09 18:10 ` stephanolbrich at gmx.de
@ 2016-02-09 23:44 ` Eric Anholt
-1 siblings, 0 replies; 52+ messages in thread
From: Eric Anholt @ 2016-02-09 23:44 UTC (permalink / raw)
To: stephanolbrich-Mmb7MZpHnFY, Mark Brown, Stephen Warren,
Lee Jones, linux-spi-u79uwXL29TY76Z2rM5mHXA,
linux-rpi-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
Cc: Stephan Olbrich
[-- 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 --]
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH 1/4] spi: bcm2835aux: fix bitmask defines
@ 2016-02-09 23:44 ` Eric Anholt
0 siblings, 0 replies; 52+ messages in thread
From: Eric Anholt @ 2016-02-09 23:44 UTC (permalink / raw)
To: linux-arm-kernel
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>
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH 2/4] spi: bcm2835aux: disable tx fifo empty irq
2016-02-09 18:10 ` stephanolbrich at gmx.de
@ 2016-02-09 23:45 ` Eric Anholt
-1 siblings, 0 replies; 52+ messages in thread
From: Eric Anholt @ 2016-02-09 23:45 UTC (permalink / raw)
To: stephanolbrich-Mmb7MZpHnFY, Mark Brown, Stephen Warren,
Lee Jones, linux-spi-u79uwXL29TY76Z2rM5mHXA,
linux-rpi-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
Cc: Stephan Olbrich
[-- 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 --]
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH 2/4] spi: bcm2835aux: disable tx fifo empty irq
@ 2016-02-09 23:45 ` Eric Anholt
0 siblings, 0 replies; 52+ messages in thread
From: Eric Anholt @ 2016-02-09 23:45 UTC (permalink / raw)
To: linux-arm-kernel
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>
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH 3/4] spi: bcm2835aux: set up spi-mode before asserting cs-gpio
2016-02-09 18:10 ` stephanolbrich at gmx.de
@ 2016-02-09 23:49 ` Eric Anholt
-1 siblings, 0 replies; 52+ messages in thread
From: Eric Anholt @ 2016-02-09 23:49 UTC (permalink / raw)
To: stephanolbrich-Mmb7MZpHnFY, Mark Brown, Stephen Warren,
Lee Jones, linux-spi-u79uwXL29TY76Z2rM5mHXA,
linux-rpi-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
Cc: Stephan Olbrich
[-- 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 --]
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH 3/4] spi: bcm2835aux: set up spi-mode before asserting cs-gpio
@ 2016-02-09 23:49 ` Eric Anholt
0 siblings, 0 replies; 52+ messages in thread
From: Eric Anholt @ 2016-02-09 23:49 UTC (permalink / raw)
To: linux-arm-kernel
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>
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH 4/4] spi: bcm2835aux: fix CPOL/CPHA setting
2016-02-09 18:10 ` stephanolbrich at gmx.de
@ 2016-02-10 0:13 ` Eric Anholt
-1 siblings, 0 replies; 52+ messages in thread
From: Eric Anholt @ 2016-02-10 0:13 UTC (permalink / raw)
To: stephanolbrich-Mmb7MZpHnFY, Mark Brown, Stephen Warren,
Lee Jones, linux-spi-u79uwXL29TY76Z2rM5mHXA,
linux-rpi-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
Cc: Stephan Olbrich
[-- 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 --]
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH 4/4] spi: bcm2835aux: fix CPOL/CPHA setting
@ 2016-02-10 0:13 ` Eric Anholt
0 siblings, 0 replies; 52+ messages in thread
From: Eric Anholt @ 2016-02-10 0:13 UTC (permalink / raw)
To: linux-arm-kernel
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>
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH 3/4] spi: bcm2835aux: set up spi-mode before asserting cs-gpio
2016-02-09 23:49 ` Eric Anholt
@ 2016-02-10 8:01 ` Mark Brown
-1 siblings, 0 replies; 52+ messages in thread
From: Mark Brown @ 2016-02-10 8:01 UTC (permalink / raw)
To: Eric Anholt
Cc: stephanolbrich-Mmb7MZpHnFY, Stephen Warren, Lee Jones,
linux-spi-u79uwXL29TY76Z2rM5mHXA,
linux-rpi-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
[-- 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 --]
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH 3/4] spi: bcm2835aux: set up spi-mode before asserting cs-gpio
@ 2016-02-10 8:01 ` Mark Brown
0 siblings, 0 replies; 52+ messages in thread
From: Mark Brown @ 2016-02-10 8:01 UTC (permalink / raw)
To: linux-arm-kernel
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>
^ permalink raw reply [flat|nested] 52+ messages in thread
* Applied "spi: bcm2835aux: fix bitmask defines" to the spi tree
[not found] ` <1455041435-8015-2-git-send-email-stephanolbrich-Mmb7MZpHnFY@public.gmane.org>
2016-02-09 19:54 ` Stefan Wahren
2016-02-09 23:44 ` Eric Anholt
@ 2016-02-10 12:55 ` Mark Brown
2 siblings, 0 replies; 52+ messages in thread
From: Mark Brown @ 2016-02-10 12:55 UTC (permalink / raw)
To: Stephan Olbrich, Mark Brown; +Cc: linux-spi-u79uwXL29TY76Z2rM5mHXA
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
^ permalink raw reply related [flat|nested] 52+ messages in thread
* Re: [PATCH 3/4] spi: bcm2835aux: set up spi-mode before asserting cs-gpio
2016-02-10 8:01 ` Mark Brown
@ 2016-02-10 18:59 ` Eric Anholt
-1 siblings, 0 replies; 52+ messages in thread
From: Eric Anholt @ 2016-02-10 18:59 UTC (permalink / raw)
To: Mark Brown
Cc: stephanolbrich-Mmb7MZpHnFY, Stephen Warren, Lee Jones,
linux-spi-u79uwXL29TY76Z2rM5mHXA,
linux-rpi-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
[-- 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 --]
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH 3/4] spi: bcm2835aux: set up spi-mode before asserting cs-gpio
@ 2016-02-10 18:59 ` Eric Anholt
0 siblings, 0 replies; 52+ messages in thread
From: Eric Anholt @ 2016-02-10 18:59 UTC (permalink / raw)
To: linux-arm-kernel
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>
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH 3/4] spi: bcm2835aux: set up spi-mode before asserting cs-gpio
2016-02-10 18:59 ` Eric Anholt
@ 2016-02-10 19:02 ` Mark Brown
-1 siblings, 0 replies; 52+ messages in thread
From: Mark Brown @ 2016-02-10 19:02 UTC (permalink / raw)
To: Eric Anholt
Cc: stephanolbrich-Mmb7MZpHnFY, Stephen Warren, Lee Jones,
linux-spi-u79uwXL29TY76Z2rM5mHXA,
linux-rpi-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
[-- 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 --]
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH 3/4] spi: bcm2835aux: set up spi-mode before asserting cs-gpio
@ 2016-02-10 19:02 ` Mark Brown
0 siblings, 0 replies; 52+ messages in thread
From: Mark Brown @ 2016-02-10 19:02 UTC (permalink / raw)
To: linux-arm-kernel
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>
^ permalink raw reply [flat|nested] 52+ messages in thread
* Applied "spi: bcm2835aux: disable tx fifo empty irq" to the spi tree
[not found] ` <1455041435-8015-3-git-send-email-stephanolbrich-Mmb7MZpHnFY@public.gmane.org>
2016-02-09 23:45 ` Eric Anholt
@ 2016-02-10 19:22 ` Mark Brown
1 sibling, 0 replies; 52+ messages in thread
From: Mark Brown @ 2016-02-10 19:22 UTC (permalink / raw)
To: Stephan Olbrich, Mark Brown; +Cc: linux-spi-u79uwXL29TY76Z2rM5mHXA
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
^ permalink raw reply related [flat|nested] 52+ messages in thread
* Re: [PATCH 1/4] spi: bcm2835aux: fix bitmask defines
2016-02-09 19:54 ` Stefan Wahren
@ 2016-02-10 20:08 ` Stephan Olbrich
-1 siblings, 0 replies; 52+ messages in thread
From: Stephan Olbrich @ 2016-02-10 20:08 UTC (permalink / raw)
To: linux-rpi-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
Cc: Stefan Wahren, Mark Brown, Stephen Warren, Lee Jones,
Eric Anholt, linux-spi-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
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
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH 1/4] spi: bcm2835aux: fix bitmask defines
@ 2016-02-10 20:08 ` Stephan Olbrich
0 siblings, 0 replies; 52+ messages in thread
From: Stephan Olbrich @ 2016-02-10 20:08 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH 3/4] spi: bcm2835aux: set up spi-mode before asserting cs-gpio
2016-02-10 19:02 ` Mark Brown
@ 2016-02-10 20:26 ` Stephan Olbrich
-1 siblings, 0 replies; 52+ messages in thread
From: Stephan Olbrich @ 2016-02-10 20:26 UTC (permalink / raw)
To: linux-rpi-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
Cc: Mark Brown, Eric Anholt, linux-spi-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
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
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH 3/4] spi: bcm2835aux: set up spi-mode before asserting cs-gpio
@ 2016-02-10 20:26 ` Stephan Olbrich
0 siblings, 0 replies; 52+ messages in thread
From: Stephan Olbrich @ 2016-02-10 20:26 UTC (permalink / raw)
To: linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH 4/4] spi: bcm2835aux: fix CPOL/CPHA setting
2016-02-10 0:13 ` Eric Anholt
@ 2016-02-10 20:45 ` Stephan Olbrich
-1 siblings, 0 replies; 52+ messages in thread
From: Stephan Olbrich @ 2016-02-10 20:45 UTC (permalink / raw)
To: linux-rpi-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
Cc: Eric Anholt, Mark Brown, Stephen Warren, Lee Jones,
linux-spi-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
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
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH 4/4] spi: bcm2835aux: fix CPOL/CPHA setting
@ 2016-02-10 20:45 ` Stephan Olbrich
0 siblings, 0 replies; 52+ messages in thread
From: Stephan Olbrich @ 2016-02-10 20:45 UTC (permalink / raw)
To: linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH 4/4] spi: bcm2835aux: fix CPOL/CPHA setting
2016-02-10 20:45 ` Stephan Olbrich
@ 2016-02-10 21:24 ` Eric Anholt
-1 siblings, 0 replies; 52+ messages in thread
From: Eric Anholt @ 2016-02-10 21:24 UTC (permalink / raw)
To: Stephan Olbrich, linux-rpi-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
Cc: Mark Brown, Stephen Warren, Lee Jones,
linux-spi-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
[-- 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 --]
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH 4/4] spi: bcm2835aux: fix CPOL/CPHA setting
@ 2016-02-10 21:24 ` Eric Anholt
0 siblings, 0 replies; 52+ messages in thread
From: Eric Anholt @ 2016-02-10 21:24 UTC (permalink / raw)
To: linux-arm-kernel
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>
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH 3/4] spi: bcm2835aux: set up spi-mode before asserting cs-gpio
2016-02-10 20:26 ` Stephan Olbrich
@ 2016-02-10 23:19 ` Martin Sperl
-1 siblings, 0 replies; 52+ messages in thread
From: Martin Sperl @ 2016-02-10 23:19 UTC (permalink / raw)
To: Stephan Olbrich
Cc: linux-rpi-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Mark Brown,
Eric Anholt, linux-spi-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
> 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
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH 3/4] spi: bcm2835aux: set up spi-mode before asserting cs-gpio
@ 2016-02-10 23:19 ` Martin Sperl
0 siblings, 0 replies; 52+ messages in thread
From: Martin Sperl @ 2016-02-10 23:19 UTC (permalink / raw)
To: linux-arm-kernel
> 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
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH 3/4] spi: bcm2835aux: set up spi-mode before asserting cs-gpio
2016-02-10 20:26 ` Stephan Olbrich
@ 2016-02-11 12:27 ` Mark Brown
-1 siblings, 0 replies; 52+ messages in thread
From: Mark Brown @ 2016-02-11 12:27 UTC (permalink / raw)
To: Stephan Olbrich
Cc: linux-rpi-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Eric Anholt,
linux-spi-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
[-- 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 --]
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH 3/4] spi: bcm2835aux: set up spi-mode before asserting cs-gpio
@ 2016-02-11 12:27 ` Mark Brown
0 siblings, 0 replies; 52+ messages in thread
From: Mark Brown @ 2016-02-11 12:27 UTC (permalink / raw)
To: linux-arm-kernel
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>
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH 4/4] spi: bcm2835aux: fix CPOL/CPHA setting
2016-02-10 0:13 ` Eric Anholt
@ 2016-02-11 15:25 ` Martin Sperl
-1 siblings, 0 replies; 52+ messages in thread
From: Martin Sperl @ 2016-02-11 15:25 UTC (permalink / raw)
To: Eric Anholt
Cc: stephanolbrich-Mmb7MZpHnFY, Mark Brown, Stephen Warren,
Lee Jones, linux-spi-u79uwXL29TY76Z2rM5mHXA,
linux-rpi-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
> 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
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH 4/4] spi: bcm2835aux: fix CPOL/CPHA setting
@ 2016-02-11 15:25 ` Martin Sperl
0 siblings, 0 replies; 52+ messages in thread
From: Martin Sperl @ 2016-02-11 15:25 UTC (permalink / raw)
To: linux-arm-kernel
> 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
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH 4/4] spi: bcm2835aux: fix CPOL/CPHA setting
2016-02-11 15:25 ` Martin Sperl
@ 2016-02-11 16:05 ` Stephan Olbrich
-1 siblings, 0 replies; 52+ messages in thread
From: Stephan Olbrich @ 2016-02-11 16:05 UTC (permalink / raw)
To: linux-rpi-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
Cc: Martin Sperl, Eric Anholt, linux-spi-u79uwXL29TY76Z2rM5mHXA,
Mark Brown, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
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
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH 4/4] spi: bcm2835aux: fix CPOL/CPHA setting
@ 2016-02-11 16:05 ` Stephan Olbrich
0 siblings, 0 replies; 52+ messages in thread
From: Stephan Olbrich @ 2016-02-11 16:05 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH 4/4] spi: bcm2835aux: fix CPOL/CPHA setting
2016-02-11 16:05 ` Stephan Olbrich
@ 2016-02-11 16:19 ` Martin Sperl
-1 siblings, 0 replies; 52+ messages in thread
From: Martin Sperl @ 2016-02-11 16:19 UTC (permalink / raw)
To: Stephan Olbrich
Cc: linux-rpi-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Eric Anholt,
linux-spi-u79uwXL29TY76Z2rM5mHXA, Mark Brown,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
> 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
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH 4/4] spi: bcm2835aux: fix CPOL/CPHA setting
@ 2016-02-11 16:19 ` Martin Sperl
0 siblings, 0 replies; 52+ messages in thread
From: Martin Sperl @ 2016-02-11 16:19 UTC (permalink / raw)
To: linux-arm-kernel
> 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
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH 3/4] spi: bcm2835aux: set up spi-mode before asserting cs-gpio
2016-02-09 18:10 ` stephanolbrich at gmx.de
@ 2016-02-11 18:06 ` Martin Sperl
-1 siblings, 0 replies; 52+ messages in thread
From: Martin Sperl @ 2016-02-11 18:06 UTC (permalink / raw)
To: stephanolbrich-Mmb7MZpHnFY
Cc: Mark Brown, Stephen Warren, Lee Jones, Eric Anholt,
linux-spi-u79uwXL29TY76Z2rM5mHXA,
linux-rpi-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
> 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
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH 3/4] spi: bcm2835aux: set up spi-mode before asserting cs-gpio
@ 2016-02-11 18:06 ` Martin Sperl
0 siblings, 0 replies; 52+ messages in thread
From: Martin Sperl @ 2016-02-11 18:06 UTC (permalink / raw)
To: linux-arm-kernel
> 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
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH 4/4] spi: bcm2835aux: fix CPOL/CPHA setting
2016-02-11 16:19 ` Martin Sperl
@ 2016-02-11 18:44 ` Martin Sperl
-1 siblings, 0 replies; 52+ messages in thread
From: Martin Sperl @ 2016-02-11 18:44 UTC (permalink / raw)
To: Stephan Olbrich
Cc: linux-rpi-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Eric Anholt,
linux-spi-u79uwXL29TY76Z2rM5mHXA, Mark Brown,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
> 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
^ permalink raw reply related [flat|nested] 52+ messages in thread
* [PATCH 4/4] spi: bcm2835aux: fix CPOL/CPHA setting
@ 2016-02-11 18:44 ` Martin Sperl
0 siblings, 0 replies; 52+ messages in thread
From: Martin Sperl @ 2016-02-11 18:44 UTC (permalink / raw)
To: linux-arm-kernel
> 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
^ permalink raw reply related [flat|nested] 52+ messages in thread
end of thread, other threads:[~2016-02-11 18:44 UTC | newest]
Thread overview: 52+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-02-09 18:10 [PATCH 0/4] spi: bcm2835aux: auxiliary spi improvements stephanolbrich-Mmb7MZpHnFY
2016-02-09 18:10 ` stephanolbrich at gmx.de
[not found] ` <1455041435-8015-1-git-send-email-stephanolbrich-Mmb7MZpHnFY@public.gmane.org>
2016-02-09 18:10 ` [PATCH 1/4] spi: bcm2835aux: fix bitmask defines stephanolbrich-Mmb7MZpHnFY
2016-02-09 18:10 ` stephanolbrich at gmx.de
[not found] ` <1455041435-8015-2-git-send-email-stephanolbrich-Mmb7MZpHnFY@public.gmane.org>
2016-02-09 19:54 ` Stefan Wahren
2016-02-09 19:54 ` Stefan Wahren
[not found] ` <56BA43DB.7030209-saaNCTdWVBT7BZbvpMY5sg@public.gmane.org>
2016-02-10 20:08 ` Stephan Olbrich
2016-02-10 20:08 ` Stephan Olbrich
2016-02-09 23:44 ` Eric Anholt
2016-02-09 23:44 ` Eric Anholt
2016-02-10 12:55 ` Applied "spi: bcm2835aux: fix bitmask defines" to the spi tree Mark Brown
2016-02-09 18:10 ` [PATCH 2/4] spi: bcm2835aux: disable tx fifo empty irq stephanolbrich-Mmb7MZpHnFY
2016-02-09 18:10 ` stephanolbrich at gmx.de
[not found] ` <1455041435-8015-3-git-send-email-stephanolbrich-Mmb7MZpHnFY@public.gmane.org>
2016-02-09 23:45 ` Eric Anholt
2016-02-09 23:45 ` Eric Anholt
2016-02-10 19:22 ` Applied "spi: bcm2835aux: disable tx fifo empty irq" to the spi tree Mark Brown
2016-02-09 18:10 ` [PATCH 3/4] spi: bcm2835aux: set up spi-mode before asserting cs-gpio stephanolbrich-Mmb7MZpHnFY
2016-02-09 18:10 ` stephanolbrich at gmx.de
[not found] ` <1455041435-8015-4-git-send-email-stephanolbrich-Mmb7MZpHnFY@public.gmane.org>
2016-02-09 23:49 ` Eric Anholt
2016-02-09 23:49 ` Eric Anholt
[not found] ` <87d1s54f7f.fsf-omZaPlIz5HhaEpDpdNBo/KxOck334EZe@public.gmane.org>
2016-02-10 8:01 ` Mark Brown
2016-02-10 8:01 ` Mark Brown
[not found] ` <20160210080149.GQ13270-GFdadSzt00ze9xe1eoZjHA@public.gmane.org>
2016-02-10 18:59 ` Eric Anholt
2016-02-10 18:59 ` Eric Anholt
[not found] ` <878u2s2xym.fsf-omZaPlIz5HhaEpDpdNBo/KxOck334EZe@public.gmane.org>
2016-02-10 19:02 ` Mark Brown
2016-02-10 19:02 ` Mark Brown
[not found] ` <20160210190204.GD13270-GFdadSzt00ze9xe1eoZjHA@public.gmane.org>
2016-02-10 20:26 ` Stephan Olbrich
2016-02-10 20:26 ` Stephan Olbrich
2016-02-10 23:19 ` Martin Sperl
2016-02-10 23:19 ` Martin Sperl
2016-02-11 12:27 ` Mark Brown
2016-02-11 12:27 ` Mark Brown
2016-02-11 18:06 ` Martin Sperl
2016-02-11 18:06 ` Martin Sperl
2016-02-09 18:10 ` [PATCH 4/4] spi: bcm2835aux: fix CPOL/CPHA setting stephanolbrich-Mmb7MZpHnFY
2016-02-09 18:10 ` stephanolbrich at gmx.de
[not found] ` <1455041435-8015-5-git-send-email-stephanolbrich-Mmb7MZpHnFY@public.gmane.org>
2016-02-09 20:21 ` Stefan Wahren
2016-02-09 20:21 ` Stefan Wahren
2016-02-10 0:13 ` Eric Anholt
2016-02-10 0:13 ` Eric Anholt
[not found] ` <87a8n94e3x.fsf-omZaPlIz5HhaEpDpdNBo/KxOck334EZe@public.gmane.org>
2016-02-10 20:45 ` Stephan Olbrich
2016-02-10 20:45 ` Stephan Olbrich
2016-02-10 21:24 ` Eric Anholt
2016-02-10 21:24 ` Eric Anholt
2016-02-11 15:25 ` Martin Sperl
2016-02-11 15:25 ` Martin Sperl
[not found] ` <16A848EA-6E08-48C4-B3DD-DF27333B7458-d5rIkyn9cnPYtjvyW6yDsg@public.gmane.org>
2016-02-11 16:05 ` Stephan Olbrich
2016-02-11 16:05 ` Stephan Olbrich
2016-02-11 16:19 ` Martin Sperl
2016-02-11 16:19 ` Martin Sperl
[not found] ` <50D0ECA7-8116-4248-AC85-6E1C8002A185-d5rIkyn9cnPYtjvyW6yDsg@public.gmane.org>
2016-02-11 18:44 ` Martin Sperl
2016-02-11 18:44 ` Martin Sperl
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.