* [PATCH 0/4] spi: bcm2835aux: auxiliary spi improvements
@ 2016-02-09 18:10 stephanolbrich at gmx.de
2016-02-09 18:10 ` [PATCH 1/4] spi: bcm2835aux: fix bitmask defines stephanolbrich at gmx.de
` (3 more replies)
0 siblings, 4 replies; 25+ 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] 25+ messages in thread* [PATCH 1/4] spi: bcm2835aux: fix bitmask defines 2016-02-09 18:10 [PATCH 0/4] spi: bcm2835aux: auxiliary spi improvements stephanolbrich at gmx.de @ 2016-02-09 18:10 ` stephanolbrich at gmx.de 2016-02-09 19:54 ` Stefan Wahren 2016-02-09 23:44 ` Eric Anholt 2016-02-09 18:10 ` [PATCH 2/4] spi: bcm2835aux: disable tx fifo empty irq stephanolbrich at gmx.de ` (2 subsequent siblings) 3 siblings, 2 replies; 25+ 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] 25+ messages in thread
* [PATCH 1/4] spi: bcm2835aux: fix bitmask defines 2016-02-09 18:10 ` [PATCH 1/4] spi: bcm2835aux: fix bitmask defines stephanolbrich at gmx.de @ 2016-02-09 19:54 ` Stefan Wahren 2016-02-10 20:08 ` Stephan Olbrich 2016-02-09 23:44 ` Eric Anholt 1 sibling, 1 reply; 25+ 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] 25+ messages in thread
* [PATCH 1/4] spi: bcm2835aux: fix bitmask defines 2016-02-09 19:54 ` Stefan Wahren @ 2016-02-10 20:08 ` Stephan Olbrich 0 siblings, 0 replies; 25+ 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] 25+ messages in thread
* [PATCH 1/4] spi: bcm2835aux: fix bitmask defines 2016-02-09 18:10 ` [PATCH 1/4] spi: bcm2835aux: fix bitmask defines stephanolbrich at gmx.de 2016-02-09 19:54 ` Stefan Wahren @ 2016-02-09 23:44 ` Eric Anholt 1 sibling, 0 replies; 25+ 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] 25+ messages in thread
* [PATCH 2/4] spi: bcm2835aux: disable tx fifo empty irq 2016-02-09 18:10 [PATCH 0/4] spi: bcm2835aux: auxiliary spi improvements stephanolbrich at gmx.de 2016-02-09 18:10 ` [PATCH 1/4] spi: bcm2835aux: fix bitmask defines stephanolbrich at gmx.de @ 2016-02-09 18:10 ` stephanolbrich at gmx.de 2016-02-09 23:45 ` Eric Anholt 2016-02-09 18:10 ` [PATCH 3/4] spi: bcm2835aux: set up spi-mode before asserting cs-gpio stephanolbrich at gmx.de 2016-02-09 18:10 ` [PATCH 4/4] spi: bcm2835aux: fix CPOL/CPHA setting stephanolbrich at gmx.de 3 siblings, 1 reply; 25+ 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] 25+ messages in thread
* [PATCH 2/4] spi: bcm2835aux: disable tx fifo empty irq 2016-02-09 18:10 ` [PATCH 2/4] spi: bcm2835aux: disable tx fifo empty irq stephanolbrich at gmx.de @ 2016-02-09 23:45 ` Eric Anholt 0 siblings, 0 replies; 25+ 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] 25+ messages in thread
* [PATCH 3/4] spi: bcm2835aux: set up spi-mode before asserting cs-gpio 2016-02-09 18:10 [PATCH 0/4] spi: bcm2835aux: auxiliary spi improvements stephanolbrich at gmx.de 2016-02-09 18:10 ` [PATCH 1/4] spi: bcm2835aux: fix bitmask defines stephanolbrich at gmx.de 2016-02-09 18:10 ` [PATCH 2/4] spi: bcm2835aux: disable tx fifo empty irq stephanolbrich at gmx.de @ 2016-02-09 18:10 ` stephanolbrich at gmx.de 2016-02-09 23:49 ` Eric Anholt 2016-02-11 18:06 ` Martin Sperl 2016-02-09 18:10 ` [PATCH 4/4] spi: bcm2835aux: fix CPOL/CPHA setting stephanolbrich at gmx.de 3 siblings, 2 replies; 25+ 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] 25+ messages in thread
* [PATCH 3/4] spi: bcm2835aux: set up spi-mode before asserting cs-gpio 2016-02-09 18:10 ` [PATCH 3/4] spi: bcm2835aux: set up spi-mode before asserting cs-gpio stephanolbrich at gmx.de @ 2016-02-09 23:49 ` Eric Anholt 2016-02-10 8:01 ` Mark Brown 2016-02-11 18:06 ` Martin Sperl 1 sibling, 1 reply; 25+ 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] 25+ messages in thread
* [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 2016-02-10 18:59 ` Eric Anholt 0 siblings, 1 reply; 25+ 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] 25+ messages in thread
* [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 2016-02-10 19:02 ` Mark Brown 0 siblings, 1 reply; 25+ 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] 25+ messages in thread
* [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 2016-02-10 20:26 ` Stephan Olbrich 0 siblings, 1 reply; 25+ 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] 25+ messages in thread
* [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 2016-02-10 23:19 ` Martin Sperl 2016-02-11 12:27 ` Mark Brown 0 siblings, 2 replies; 25+ 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] 25+ messages in thread
* [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 2016-02-11 12:27 ` Mark Brown 1 sibling, 0 replies; 25+ 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] 25+ messages in thread
* [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 @ 2016-02-11 12:27 ` Mark Brown 1 sibling, 0 replies; 25+ 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] 25+ messages in thread
* [PATCH 3/4] spi: bcm2835aux: set up spi-mode before asserting cs-gpio 2016-02-09 18:10 ` [PATCH 3/4] spi: bcm2835aux: set up spi-mode before asserting cs-gpio stephanolbrich at gmx.de 2016-02-09 23:49 ` Eric Anholt @ 2016-02-11 18:06 ` Martin Sperl 1 sibling, 0 replies; 25+ 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] 25+ messages in thread
* [PATCH 4/4] spi: bcm2835aux: fix CPOL/CPHA setting 2016-02-09 18:10 [PATCH 0/4] spi: bcm2835aux: auxiliary spi improvements stephanolbrich at gmx.de ` (2 preceding siblings ...) 2016-02-09 18:10 ` [PATCH 3/4] spi: bcm2835aux: set up spi-mode before asserting cs-gpio stephanolbrich at gmx.de @ 2016-02-09 18:10 ` stephanolbrich at gmx.de 2016-02-09 20:21 ` Stefan Wahren 2016-02-10 0:13 ` Eric Anholt 3 siblings, 2 replies; 25+ 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] 25+ messages in thread
* [PATCH 4/4] spi: bcm2835aux: fix CPOL/CPHA setting 2016-02-09 18:10 ` [PATCH 4/4] spi: bcm2835aux: fix CPOL/CPHA setting stephanolbrich at gmx.de @ 2016-02-09 20:21 ` Stefan Wahren 2016-02-10 0:13 ` Eric Anholt 1 sibling, 0 replies; 25+ 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] 25+ messages in thread
* [PATCH 4/4] spi: bcm2835aux: fix CPOL/CPHA setting 2016-02-09 18:10 ` [PATCH 4/4] spi: bcm2835aux: fix CPOL/CPHA setting stephanolbrich at gmx.de 2016-02-09 20:21 ` Stefan Wahren @ 2016-02-10 0:13 ` Eric Anholt 2016-02-10 20:45 ` Stephan Olbrich 2016-02-11 15:25 ` Martin Sperl 1 sibling, 2 replies; 25+ 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] 25+ messages in thread
* [PATCH 4/4] spi: bcm2835aux: fix CPOL/CPHA setting 2016-02-10 0:13 ` Eric Anholt @ 2016-02-10 20:45 ` Stephan Olbrich 2016-02-10 21:24 ` Eric Anholt 2016-02-11 15:25 ` Martin Sperl 1 sibling, 1 reply; 25+ 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] 25+ messages in thread
* [PATCH 4/4] spi: bcm2835aux: fix CPOL/CPHA setting 2016-02-10 20:45 ` Stephan Olbrich @ 2016-02-10 21:24 ` Eric Anholt 0 siblings, 0 replies; 25+ 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] 25+ messages in thread
* [PATCH 4/4] spi: bcm2835aux: fix CPOL/CPHA setting 2016-02-10 0:13 ` Eric Anholt 2016-02-10 20:45 ` Stephan Olbrich @ 2016-02-11 15:25 ` Martin Sperl 2016-02-11 16:05 ` Stephan Olbrich 1 sibling, 1 reply; 25+ 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] 25+ messages in thread
* [PATCH 4/4] spi: bcm2835aux: fix CPOL/CPHA setting 2016-02-11 15:25 ` Martin Sperl @ 2016-02-11 16:05 ` Stephan Olbrich 2016-02-11 16:19 ` Martin Sperl 0 siblings, 1 reply; 25+ 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] 25+ messages in thread
* [PATCH 4/4] spi: bcm2835aux: fix CPOL/CPHA setting 2016-02-11 16:05 ` Stephan Olbrich @ 2016-02-11 16:19 ` Martin Sperl 2016-02-11 18:44 ` Martin Sperl 0 siblings, 1 reply; 25+ 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] 25+ messages in thread
* [PATCH 4/4] spi: bcm2835aux: fix CPOL/CPHA setting 2016-02-11 16:19 ` Martin Sperl @ 2016-02-11 18:44 ` Martin Sperl 0 siblings, 0 replies; 25+ 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] 25+ messages in thread
end of thread, other threads:[~2016-02-11 18:44 UTC | newest] Thread overview: 25+ 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 at gmx.de 2016-02-09 18:10 ` [PATCH 1/4] spi: bcm2835aux: fix bitmask defines stephanolbrich at gmx.de 2016-02-09 19:54 ` Stefan Wahren 2016-02-10 20:08 ` Stephan Olbrich 2016-02-09 23:44 ` Eric Anholt 2016-02-09 18:10 ` [PATCH 2/4] spi: bcm2835aux: disable tx fifo empty irq stephanolbrich at gmx.de 2016-02-09 23:45 ` Eric Anholt 2016-02-09 18:10 ` [PATCH 3/4] spi: bcm2835aux: set up spi-mode before asserting cs-gpio stephanolbrich at gmx.de 2016-02-09 23:49 ` Eric Anholt 2016-02-10 8:01 ` Mark Brown 2016-02-10 18:59 ` Eric Anholt 2016-02-10 19:02 ` Mark Brown 2016-02-10 20:26 ` Stephan Olbrich 2016-02-10 23:19 ` Martin Sperl 2016-02-11 12:27 ` Mark Brown 2016-02-11 18:06 ` Martin Sperl 2016-02-09 18:10 ` [PATCH 4/4] spi: bcm2835aux: fix CPOL/CPHA setting stephanolbrich at gmx.de 2016-02-09 20:21 ` Stefan Wahren 2016-02-10 0:13 ` Eric Anholt 2016-02-10 20:45 ` Stephan Olbrich 2016-02-10 21:24 ` Eric Anholt 2016-02-11 15:25 ` Martin Sperl 2016-02-11 16:05 ` Stephan Olbrich 2016-02-11 16:19 ` Martin Sperl 2016-02-11 18:44 ` Martin Sperl
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).