Re: [PATCH v6 2/4] spi: bcm2835: add bcm2835 auxiliary spi device driver

[Stephan Olbrich <stephanolbrich@xxxxxx>]

Am Montag, 4. Januar 2016, 15:23:52 schrieb Martin Sperl:
> > On 04.01.2016, at 14:51, Stephan Olbrich <stephanolbrich@xxxxxx> wrote:
> > 
> > According to the spi documentation [1]: "CPHA indicates the clock phase
> > used to sample data; CPHA=0 says sample on the leading edge, CPHA=1 means
> > the trailing edge."
> > Which is from my understanding different from what the BMC2835 ARM
> > Peripherals [2] says for BCM2835_AUX_SPI_CNTL0_CPHA_IN:
> > "If 1 data is clocked in on the rising edge of the SPI clock
> > If 0 data is clocked in on the falling edge of the SPI clock"
> > 
> > I would expect the bits to be set dependant on the clock polarity (CPOL).
> 
> I am only having “typical” devices with do Mode 0,0 or 1,1 for which it
> works.

Are you only writing to your devices or reading as well? From what I think how 
this works, reading should not have worked with Mode 0,0

> > The other issue I have, is that the chip select is set before the clock
> > polarity and the polarity is reset and set again between each transfers of
> > a message. If CPOL is set to 1 this leads to additional rising and
> > falling edges of the clock while chip select is active, where no data is
> > sampled.
> This is something I have seen before with the main spi HW.
> We may need to port the same thing there.
> 
> The corresponding patch for spi-bcm2835.c is this:
> acace73df2c1913a526c1b41e4741a4a6704c863

Thanks for the hint. I tried to implement a similar solution. Could you test 
if this works for you?
It also contains a fix for the IRQ defines, a hopefully correct solution to 
the CPHA issue and reduces the number of unnecessary interrupts.

---
 drivers/spi/spi-bcm2835aux.c | 70 
++++++++++++++++++++++++++++++++------------
 1 file changed, 52 insertions(+), 18 deletions(-)

diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c
index 7de6f84..d8be445 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
 
@@ -212,9 +212,15 @@ static irqreturn_t bcm2835aux_spi_interrupt(int irq, void 
*dev_id)
 		ret = IRQ_HANDLED;
 	}
 
-	/* and if rx_len is 0 then wake up completion and disable spi */
+	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 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);
 	}
 
@@ -307,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;
 }
@@ -330,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;
@@ -352,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;
@@ -382,6 +374,46 @@ 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_IN;
+		else
+			bs->cntl[0] |= BCM2835_AUX_SPI_CNTL0_CPHA_OUT;
+	} else {
+		if (spi->mode & SPI_CPHA)
+			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]);
+
+	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)
 {
@@ -410,6 +442,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.1.4


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