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

[Oskari Lemmelä <oskari@xxxxxxxxxxx>]

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