Subject: Re: [PATCH 1/2] spi: ar934x: fix transfer size
To:     Mark Brown <broonie@kernel.org>
Cc:     linux-spi@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20211222055958.1383233-1-oskari@lemmela.net>
 <20211222055958.1383233-2-oskari@lemmela.net>
 <YcMa1TIg3x3oBKBl@sirena.org.uk>
 <98a574a2-45c3-5d7c-6405-0cd279a81816@lemmela.net>
 <YcM9Ug3JhaxynLt0@sirena.org.uk>
From:   =?UTF-8?Q?Oskari_Lemmel=c3=a4?= <oskari@lemmela.net>
Message-ID: <6079952b-edef-4a3b-ff7d-16acee5821b8@lemmela.net>
Date:   Wed, 22 Dec 2021 17:55:04 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.14.0
MIME-Version: 1.0
In-Reply-To: <YcM9Ug3JhaxynLt0@sirena.org.uk>
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Precedence: bulk

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