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
>
