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

[Martin Sperl <kernel@xxxxxxxxxxxxxxxx>]

> On 09.02.2016, at 19:10, stephanolbrich@xxxxxx wrote:
> 
> From: Stephan Olbrich <stephanolbrich@xxxxxx>
> 
> 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@xxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxx>
Tested-by: Martin Sperl <kernel@xxxxxxxxxxxxxxxx>

Martin


--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html