| Message ID | 1455041435-8015-4-git-send-email-stephanolbrich@gmx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
stephanolbrich@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.
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.
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).
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.
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.
> 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
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.
> On 09.02.2016, at 19:10, stephanolbrich@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
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);