[PATCH 0/2] spi: ar934x: fix transfer size and delays

[PATCH 0/2] spi: ar934x: fix transfer size and delays

[Oskari Lemmela]

Some of slow SPI devices can not handle 32 bits transfers or need
delay between words/transfers.

Series is tested with ATSAMD20J15 slave device which is running @8Mhz.
Limiting bits per word to 16 bits and adding delay between transfers,
gives slave device enough extra time to process reply.

Oskari Lemmela (2):
  spi: ar934x: fix transfer size
  spi: ar934x: fix transfer and word delays

 drivers/spi/spi-ar934x.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

-- 
2.25.1

[PATCH 1/2] spi: ar934x: fix transfer size

[Oskari Lemmela]

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

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

[PATCH 2/2] spi: ar934x: fix transfer and word delays

[Oskari Lemmela]

Add missing delay between transferred messages and words.

Signed-off-by: Oskari Lemmela <oskari@lemmela.net>
---
 drivers/spi/spi-ar934x.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/spi/spi-ar934x.c b/drivers/spi/spi-ar934x.c
index a2cf37aca234..ec7250c4c810 100644
--- a/drivers/spi/spi-ar934x.c
+++ b/drivers/spi/spi-ar934x.c
@@ -142,8 +142,10 @@ static int ar934x_spi_transfer_one_message(struct spi_controller *master,
 					reg >>= 8;
 				}
 			}
+			spi_delay_exec(&t->word_delay, t);
 		}
 		m->actual_length += t->len;
+		spi_transfer_delay_exec(t);
 	}
 
 msg_done:
-- 
2.25.1

Re: [PATCH 1/2] spi: ar934x: fix transfer size

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 368 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 484 bytes --]

Re: (subset) [PATCH 0/2] spi: ar934x: fix transfer size and delays

[Mark Brown]

On Wed, 22 Dec 2021 07:59:56 +0200, Oskari Lemmela wrote:
> Some of slow SPI devices can not handle 32 bits transfers or need
> delay between words/transfers.
> 
> Series is tested with ATSAMD20J15 slave device which is running @8Mhz.
> Limiting bits per word to 16 bits and adding delay between transfers,
> gives slave device enough extra time to process reply.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next

Thanks!

[2/2] spi: ar934x: fix transfer and word delays
      commit: c70282457c380db7deb57c81a6894debc8f88efa

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

Re: [PATCH 1/2] spi: ar934x: fix transfer size

[Oskari Lemmelä]

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

Re: [PATCH 1/2] spi: ar934x: fix transfer size

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1170 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] spi: ar934x: fix transfer size

[Oskari Lemmelä]

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

Re: (subset) [PATCH 0/2] spi: ar934x: fix transfer size and delays

[Mark Brown]

On Wed, 22 Dec 2021 07:59:56 +0200, Oskari Lemmela wrote:
> Some of slow SPI devices can not handle 32 bits transfers or need
> delay between words/transfers.
> 
> Series is tested with ATSAMD20J15 slave device which is running @8Mhz.
> Limiting bits per word to 16 bits and adding delay between transfers,
> gives slave device enough extra time to process reply.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next

Thanks!

[1/2] spi: ar934x: fix transfer size
      commit: ebe33e5a98dcf14a9630845f3f10c193584ac054

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark