Re: [PATCH v2 10/28] spi: s3c64xx: use full mask for {RX, TX}_FIFO_LVL

[Tudor Ambarus <tudor.ambarus@xxxxxxxxxx>]

On 1/25/24 20:03, Sam Protsenko wrote:
> On Thu, Jan 25, 2024 at 8:50 AM Tudor Ambarus <tudor.ambarus@xxxxxxxxxx> wrote:
>>
>> SPI_STATUSn.{RX, TX}_FIFO_LVL fields show the data level in the RX and
>> TX FIFOs. The IP supports FIFOs from 8 to 256 bytes, but apart from the
>> MODE_CFG.{RX, TX}_RDY_LVL fields that configure the {RX, TX} FIFO
>> trigger level in the interrupt mode, there's nothing in the registers
>> that configure the FIFOs depth. Is the responsibility of the SoC that
>> integrates the IP to dictate the FIFO depth and of the SPI driver to
>> make sure it doesn't bypass the FIFO length.
>>
>> {RX, TX}_FIFO_LVL was used to pass the FIFO length information based on
>> the IP configuration in the SoC. Its value was defined so that it
>> includes the entire FIFO length. For example, if one wanted to specify a
>> 64 FIFO length (0x40), it wold configure the FIFO level to 127 (0x7f).
> 
> s/wodl/would/

oh, yes, thanks
> 
>> This is not only wrong, because it doesn't respect the IP's register
>> fields, it's also misleading. Use the full mask for the
>> SPI_STATUSn.{RX, TX}_FIFO_LVL fields. No change in functionality is
>> expected.
>>
>> Signed-off-by: Tudor Ambarus <tudor.ambarus@xxxxxxxxxx>
>> ---
>>  drivers/spi/spi-s3c64xx.c | 21 +++++++++++----------
>>  1 file changed, 11 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
>> index d046810da51f..b048e81e6207 100644
>> --- a/drivers/spi/spi-s3c64xx.c
>> +++ b/drivers/spi/spi-s3c64xx.c
>> @@ -78,6 +78,8 @@
>>  #define S3C64XX_SPI_INT_RX_FIFORDY_EN          BIT(1)
>>  #define S3C64XX_SPI_INT_TX_FIFORDY_EN          BIT(0)
>>
>> +#define S3C64XX_SPI_ST_RX_FIFO_LVL             GENMASK(23, 15)
> 
> What about s3c* architectures, where RX_LVL starts with bit #13, as
> can be seen from .rx_lvl_offset values in corresponding port_configs?
> Wouldn't this change break those?

ah, wonderful catch, Sam. I break those indeed.
> 
> More generally, I don't understand why this patch is needed. Looks

I said in the commit message and subject that I'd like to use the full
FIFO level mask rather than just a partial mask. On gs101 at least, that
register field is on 9 bits, but as the code is now, we consider that
register on 7 bits. For gs101 the FIFO size is always 64 bytes, thus
indirectly the fifo_lvl_mask is always 0x7f.

Unfortunately I'll drop this patch because I don't have access to all
the SoC datasheets, thus I can't tell for sure if that register is
always 9 bits wide. s3c2443 and s3c6410, which have the rx-lvl-offset
set to 13, use just 0x7f masks. That's a pitty.