| Message ID | 20211222055958.1383233-2-oskari@lemmela.net (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | ebe33e5a98dcf14a9630845f3f10c193584ac054 |
| Headers | show |
| Series | spi: ar934x: fix transfer size and delays | expand |
On Wed, Dec 22, 2021 at 07:59:57AM +0200, Oskari Lemmela wrote: > If bits_per_word is configured, transfer only word amount > of data per iteration. Does this actually materially affect what the hardware does? How much data is transferred in an internal loop in the driver is completely immaterial, bits per word only matters for formatting of the transferred data.
On 22.12.2021 14.32, Mark Brown wrote: > On Wed, Dec 22, 2021 at 07:59:57AM +0200, Oskari Lemmela wrote: >> If bits_per_word is configured, transfer only word amount >> of data per iteration. > Does this actually materially affect what the hardware does? How much > data is transferred in an internal loop in the driver is completely > immaterial, bits per word only matters for formatting of the transferred > data. I don't have logic analyzator to verify what hardware actual does. I tested this with transferring 32bits to ATSAMD20J15 slave. Running loop in 8bits or 16bits, transfer is done correctly without any errors. When running loop in 24bits or 32bits directly I got error from spi_sync_transfer. Oskari
On Wed, Dec 22, 2021 at 04:27:36PM +0200, Oskari Lemmelä wrote: > On 22.12.2021 14.32, Mark Brown wrote: > > Does this actually materially affect what the hardware does? How much > > data is transferred in an internal loop in the driver is completely > > immaterial, bits per word only matters for formatting of the transferred > > data. > I don't have logic analyzator to verify what hardware actual does. > I tested this with transferring 32bits to ATSAMD20J15 slave. > Running loop in 8bits or 16bits, transfer is done correctly without > any errors. When running loop in 24bits or 32bits directly I got > error from spi_sync_transfer. This doesn't inspire confidence TBH. Given the lack of any change in the interaction with the hardware it doesn't seem likely that the word length is being changed at any point. Possibly there's a bug somewhere that needs fixing but it's been misdiagnosed. Note also that the commit log is not good here, now I look at the code the driver only supports 8 bits per word at the minute and the change adds support for higher word lengths. If you are seeing an issue that might point towards what it is.
On 22.12.2021 16.59, Mark Brown wrote: > On Wed, Dec 22, 2021 at 04:27:36PM +0200, Oskari Lemmelä wrote: >> On 22.12.2021 14.32, Mark Brown wrote: >>> Does this actually materially affect what the hardware does? How much >>> data is transferred in an internal loop in the driver is completely >>> immaterial, bits per word only matters for formatting of the transferred >>> data. >> I don't have logic analyzator to verify what hardware actual does. >> I tested this with transferring 32bits to ATSAMD20J15 slave. >> Running loop in 8bits or 16bits, transfer is done correctly without >> any errors. When running loop in 24bits or 32bits directly I got >> error from spi_sync_transfer. > This doesn't inspire confidence TBH. Given the lack of any change in > the interaction with the hardware it doesn't seem likely that the word > length is being changed at any point. Possibly there's a bug somewhere > that needs fixing but it's been misdiagnosed. I did find datasheet for AR9344 and hardware supports shifting bits. 8.25.6 SPI Content to Shift Out or In (SPI_SHIFT_CNT_ADDR) Address: 0x1FFF0014 Access: Read/Write Reset: 0x0 ------------------------------------------- Bit | Bit Name | Desc 31 | SHIFT_EN | Enables shifting data out 30 | SHIFT_CHNL | If set to 1, enables chip select 2 29 | | If set to 1, enables chip select 1 28 | | If set to 1, enables chip select 0 27 | SHIFT_CLKOUT | Initial value of the clock signal 26 | TERMINATE | When set to 1, deassert the chip select 25:7 | RES | Reserved 6:0 | SHIFT_COUNT | The number of bits to be shifted out or shifted in on the data line This is currently implemented in defines #define AR934X_SPI_REG_SHIFT_CTRL 0x14 #define AR934X_SPI_SHIFT_EN BIT(31) #define AR934X_SPI_SHIFT_CS(n) BIT(28 + (n)) #define AR934X_SPI_SHIFT_TERM 26 #define AR934X_SPI_SHIFT_VAL(cs, term, count) \ (AR934X_SPI_SHIFT_EN | AR934X_SPI_SHIFT_CS(cs) | \ (term) << AR934X_SPI_SHIFT_TERM | (count)) In the transfer loop count value is set to number of bits per word. reg = AR934X_SPI_SHIFT_VAL(spi->chip_select, term, trx_cur * 8); iowrite32(reg, sp->base + AR934X_SPI_REG_SHIFT_CTRL); So actually hardware support any word size between 1-32bits. > Note also that the commit log is not good here, now I look at the code > the driver only supports 8 bits per word at the minute and the change > adds support for higher word lengths. If you are seeing an issue that > might point towards what it is. Should I split this in two commits? First one fixing SPI_BPW_MASK(32) typo. Then second commit which implements 8bits, 16bits and 24bits word sizes? Or should driver implement support for any word size between 1-32bits? Oskari
diff --git a/drivers/spi/spi-ar934x.c b/drivers/spi/spi-ar934x.c index def32e0aaefe..a2cf37aca234 100644 --- a/drivers/spi/spi-ar934x.c +++ b/drivers/spi/spi-ar934x.c @@ -82,7 +82,7 @@ static int ar934x_spi_transfer_one_message(struct spi_controller *master, struct spi_device *spi = m->spi; unsigned long trx_done, trx_cur; int stat = 0; - u8 term = 0; + u8 bpw, term = 0; int div, i; u32 reg; const u8 *tx_buf; @@ -90,6 +90,11 @@ static int ar934x_spi_transfer_one_message(struct spi_controller *master, m->actual_length = 0; list_for_each_entry(t, &m->transfers, transfer_list) { + if (t->bits_per_word >= 8 && t->bits_per_word < 32) + bpw = t->bits_per_word >> 3; + else + bpw = 4; + if (t->speed_hz) div = ar934x_spi_clk_div(sp, t->speed_hz); else @@ -105,10 +110,10 @@ static int ar934x_spi_transfer_one_message(struct spi_controller *master, iowrite32(reg, sp->base + AR934X_SPI_REG_CTRL); iowrite32(0, sp->base + AR934X_SPI_DATAOUT); - for (trx_done = 0; trx_done < t->len; trx_done += 4) { + for (trx_done = 0; trx_done < t->len; trx_done += bpw) { trx_cur = t->len - trx_done; - if (trx_cur > 4) - trx_cur = 4; + if (trx_cur > bpw) + trx_cur = bpw; else if (list_is_last(&t->transfer_list, &m->transfers)) term = 1; @@ -191,7 +196,8 @@ static int ar934x_spi_probe(struct platform_device *pdev) ctlr->mode_bits = SPI_LSB_FIRST; ctlr->setup = ar934x_spi_setup; ctlr->transfer_one_message = ar934x_spi_transfer_one_message; - ctlr->bits_per_word_mask = SPI_BPW_MASK(8); + ctlr->bits_per_word_mask = SPI_BPW_MASK(32) | SPI_BPW_MASK(24) | + SPI_BPW_MASK(16) | SPI_BPW_MASK(8); ctlr->dev.of_node = pdev->dev.of_node; ctlr->num_chipselect = 3;
If bits_per_word is configured, transfer only word amount of data per iteration. Signed-off-by: Oskari Lemmela <oskari@lemmela.net> --- drivers/spi/spi-ar934x.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-)