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.