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/ > 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? More generally, I don't understand why this patch is needed. Looks like it just changes the naming of the FIFO level accessing macros, making the code more bloated too. > +#define S3C64XX_SPI_ST_TX_FIFO_LVL GENMASK(14, 6) > #define S3C64XX_SPI_ST_RX_OVERRUN_ERR BIT(5) > #define S3C64XX_SPI_ST_RX_UNDERRUN_ERR BIT(4) > #define S3C64XX_SPI_ST_TX_OVERRUN_ERR BIT(3) > @@ -108,9 +110,6 @@ > #define FIFO_LVL_MASK(i) ((i)->port_conf->fifo_lvl_mask[i->port_id]) > #define S3C64XX_SPI_ST_TX_DONE(v, i) (((v) & \ > (1 << (i)->port_conf->tx_st_done)) ? 1 : 0) > -#define TX_FIFO_LVL(v, i) (((v) >> 6) & FIFO_LVL_MASK(i)) > -#define RX_FIFO_LVL(v, i) (((v) >> (i)->port_conf->rx_lvl_offset) & \ > - FIFO_LVL_MASK(i)) > #define FIFO_DEPTH(i) ((FIFO_LVL_MASK(i) >> 1) + 1) > > #define S3C64XX_SPI_POLLING_SIZE 32 > @@ -219,7 +218,7 @@ static void s3c64xx_flush_fifo(struct s3c64xx_spi_driver_data *sdd) > loops = msecs_to_loops(1); > do { > val = readl(regs + S3C64XX_SPI_STATUS); > - } while (TX_FIFO_LVL(val, sdd) && loops--); > + } while (FIELD_GET(S3C64XX_SPI_ST_TX_FIFO_LVL, val) && loops--); > > if (loops == 0) > dev_warn(&sdd->pdev->dev, "Timed out flushing TX FIFO\n"); > @@ -228,7 +227,7 @@ static void s3c64xx_flush_fifo(struct s3c64xx_spi_driver_data *sdd) > loops = msecs_to_loops(1); > do { > val = readl(regs + S3C64XX_SPI_STATUS); > - if (RX_FIFO_LVL(val, sdd)) > + if (FIELD_GET(S3C64XX_SPI_ST_RX_FIFO_LVL, val)) > readl(regs + S3C64XX_SPI_RX_DATA); > else > break; > @@ -499,10 +498,11 @@ static u32 s3c64xx_spi_wait_for_timeout(struct s3c64xx_spi_driver_data *sdd, > > do { > status = readl(regs + S3C64XX_SPI_STATUS); > - } while (RX_FIFO_LVL(status, sdd) < max_fifo && --val); > + } while (FIELD_GET(S3C64XX_SPI_ST_RX_FIFO_LVL, status) < max_fifo && > + --val); > > /* return the actual received data length */ > - return RX_FIFO_LVL(status, sdd); > + return FIELD_GET(S3C64XX_SPI_ST_RX_FIFO_LVL, status); > } > > static int s3c64xx_wait_for_dma(struct s3c64xx_spi_driver_data *sdd, > @@ -533,7 +533,7 @@ static int s3c64xx_wait_for_dma(struct s3c64xx_spi_driver_data *sdd, > if (val && !xfer->rx_buf) { > val = msecs_to_loops(10); > status = readl(regs + S3C64XX_SPI_STATUS); > - while ((TX_FIFO_LVL(status, sdd) > + while ((FIELD_GET(S3C64XX_SPI_ST_TX_FIFO_LVL, status) > || !S3C64XX_SPI_ST_TX_DONE(status, sdd)) > && --val) { > cpu_relax(); > @@ -568,7 +568,7 @@ static int s3c64xx_wait_for_pio(struct s3c64xx_spi_driver_data *sdd, > > /* sleep during signal transfer time */ > status = readl(regs + S3C64XX_SPI_STATUS); > - if (RX_FIFO_LVL(status, sdd) < xfer->len) > + if (FIELD_GET(S3C64XX_SPI_ST_RX_FIFO_LVL, status) < xfer->len) > usleep_range(time_us / 2, time_us); > > if (use_irq) { > @@ -580,7 +580,8 @@ static int s3c64xx_wait_for_pio(struct s3c64xx_spi_driver_data *sdd, > val = msecs_to_loops(ms); > do { > status = readl(regs + S3C64XX_SPI_STATUS); > - } while (RX_FIFO_LVL(status, sdd) < xfer->len && --val); > + } while (FIELD_GET(S3C64XX_SPI_ST_RX_FIFO_LVL, status) < xfer->len && > + --val); > > if (!val) > return -EIO; > -- > 2.43.0.429.g432eaa2c6b-goog >