diff mbox series

[v2,16/28] spi: s3c64xx: simplify s3c64xx_wait_for_pio()

Message ID 20240125145007.748295-17-tudor.ambarus@linaro.org (mailing list archive)
State New
Headers show
Series spi: s3c64xx: winter cleanup and gs101 support | expand

Commit Message

Tudor Ambarus Jan. 25, 2024, 2:49 p.m. UTC
s3c64xx_spi_transfer_one() makes sure that for PIO the xfer->len is
always smaller than the fifo size. Since we can't receive more that the
FIFO size, droop the loop handling, the code becomes less misleading.

Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
---
 drivers/spi/spi-s3c64xx.c | 75 +++++++++------------------------------
 1 file changed, 17 insertions(+), 58 deletions(-)

Comments

Sam Protsenko Jan. 25, 2024, 8:43 p.m. UTC | #1
On Thu, Jan 25, 2024 at 8:50 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
> s3c64xx_spi_transfer_one() makes sure that for PIO the xfer->len is
> always smaller than the fifo size. Since we can't receive more that the
> FIFO size, droop the loop handling, the code becomes less misleading.

Drop (spelling)?

For the patch: how exactly it was tested to make sure there is no regression?

>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> ---
>  drivers/spi/spi-s3c64xx.c | 75 +++++++++------------------------------
>  1 file changed, 17 insertions(+), 58 deletions(-)
>
> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
> index d2dd28ff00c6..00a0878aeb80 100644
> --- a/drivers/spi/spi-s3c64xx.c
> +++ b/drivers/spi/spi-s3c64xx.c
> @@ -485,26 +485,6 @@ static int s3c64xx_enable_datapath(struct s3c64xx_spi_driver_data *sdd,
>         return 0;
>  }
>
> -static u32 s3c64xx_spi_wait_for_timeout(struct s3c64xx_spi_driver_data *sdd,
> -                                       int timeout_ms)
> -{
> -       void __iomem *regs = sdd->regs;
> -       unsigned long val = 1;
> -       u32 status;
> -       u32 max_fifo = FIFO_DEPTH(sdd);
> -
> -       if (timeout_ms)
> -               val = msecs_to_loops(timeout_ms);
> -
> -       do {
> -               status = readl(regs + S3C64XX_SPI_STATUS);
> -       } while (FIELD_GET(S3C64XX_SPI_ST_RX_FIFO_LVL, status) < max_fifo &&
> -                --val);
> -
> -       /* return the actual received data length */
> -       return FIELD_GET(S3C64XX_SPI_ST_RX_FIFO_LVL, status);
> -}
> -
>  static int s3c64xx_wait_for_dma(struct s3c64xx_spi_driver_data *sdd,
>                                 struct spi_transfer *xfer)
>  {
> @@ -553,13 +533,11 @@ static int s3c64xx_wait_for_pio(struct s3c64xx_spi_driver_data *sdd,
>                                 struct spi_transfer *xfer, bool use_irq)
>  {
>         void __iomem *regs = sdd->regs;
> +       u8 *buf = xfer->rx_buf;
> +       unsigned long time_us;
>         unsigned long val;
> -       u32 status;
> -       int loops;
> -       u32 cpy_len;
> -       u8 *buf;
> +       u32 status, len;
>         int ms;
> -       unsigned long time_us;
>
>         /* microsecs to xfer 'len' bytes @ 'cur_speed' */
>         time_us = (xfer->len * 8 * 1000 * 1000) / sdd->cur_speed;
> @@ -582,48 +560,29 @@ static int s3c64xx_wait_for_pio(struct s3c64xx_spi_driver_data *sdd,
>                 status = readl(regs + S3C64XX_SPI_STATUS);
>         } while (FIELD_GET(S3C64XX_SPI_ST_RX_FIFO_LVL, status) < xfer->len &&
>                  --val);
> -
>         if (!val)
>                 return -EIO;
>
>         /* If it was only Tx */
> -       if (!xfer->rx_buf) {
> +       if (!buf) {
>                 sdd->state &= ~TXBUSY;
>                 return 0;
>         }
>
> -       /*
> -        * If the receive length is bigger than the controller fifo
> -        * size, calculate the loops and read the fifo as many times.
> -        * loops = length / max fifo size (calculated by using the
> -        * fifo mask).
> -        * For any size less than the fifo size the below code is
> -        * executed atleast once.
> -        */
> -       loops = xfer->len / FIFO_DEPTH(sdd);
> -       buf = xfer->rx_buf;
> -       do {
> -               /* wait for data to be received in the fifo */
> -               cpy_len = s3c64xx_spi_wait_for_timeout(sdd,
> -                                                      (loops ? ms : 0));
> -
> -               switch (sdd->cur_bpw) {
> -               case 32:
> -                       ioread32_rep(regs + S3C64XX_SPI_RX_DATA,
> -                                    buf, cpy_len / 4);
> -                       break;
> -               case 16:
> -                       ioread16_rep(regs + S3C64XX_SPI_RX_DATA,
> -                                    buf, cpy_len / 2);
> -                       break;
> -               default:
> -                       ioread8_rep(regs + S3C64XX_SPI_RX_DATA,
> -                                   buf, cpy_len);
> -                       break;
> -               }
> +       len = FIELD_GET(S3C64XX_SPI_ST_RX_FIFO_LVL, status);
> +
> +       switch (sdd->cur_bpw) {
> +       case 32:
> +               ioread32_rep(regs + S3C64XX_SPI_RX_DATA, buf, len / 4);
> +               break;
> +       case 16:
> +               ioread16_rep(regs + S3C64XX_SPI_RX_DATA, buf, len / 2);
> +               break;
> +       default:
> +               ioread8_rep(regs + S3C64XX_SPI_RX_DATA, buf, len);
> +               break;
> +       }
>
> -               buf = buf + cpy_len;
> -       } while (loops--);
>         sdd->state &= ~RXBUSY;
>
>         return 0;
> --
> 2.43.0.429.g432eaa2c6b-goog
>
Tudor Ambarus Jan. 26, 2024, 7:56 a.m. UTC | #2
On 1/25/24 20:43, Sam Protsenko wrote:
> On Thu, Jan 25, 2024 at 8:50 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>>
>> s3c64xx_spi_transfer_one() makes sure that for PIO the xfer->len is
>> always smaller than the fifo size. Since we can't receive more that the
>> FIFO size, droop the loop handling, the code becomes less misleading.
> 
> Drop (spelling)?

oh yeah, thanks.

> 
> For the patch: how exactly it was tested to make sure there is no regression?

no regression testing for the entire patch set, I have just a gs101 on
my hands.

However, we shouldn't refrain ourselves on improving things when we
think they're straight forward and they worth it. In this particular
case, for PIO, s3c64xx_spi_transfer_one() does:
	xfer->len = fifo_len - 1;
then in s3c64xx_enable_datapath() we write xfer->len and then in
s3c64xx_wait_for_pio() we code did the following:
	loops = xfer->len / FIFO_DEPTH(sdd);
loops is always zero, this is bogus and we shall remove it.

>>
>> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
>> ---
>>  drivers/spi/spi-s3c64xx.c | 75 +++++++++------------------------------
>>  1 file changed, 17 insertions(+), 58 deletions(-)
>>
>> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
>> index d2dd28ff00c6..00a0878aeb80 100644
>> --- a/drivers/spi/spi-s3c64xx.c
>> +++ b/drivers/spi/spi-s3c64xx.c
>> @@ -485,26 +485,6 @@ static int s3c64xx_enable_datapath(struct s3c64xx_spi_driver_data *sdd,
>>         return 0;
>>  }
>>
>> -static u32 s3c64xx_spi_wait_for_timeout(struct s3c64xx_spi_driver_data *sdd,
>> -                                       int timeout_ms)
>> -{
>> -       void __iomem *regs = sdd->regs;
>> -       unsigned long val = 1;
>> -       u32 status;
>> -       u32 max_fifo = FIFO_DEPTH(sdd);
>> -
>> -       if (timeout_ms)
>> -               val = msecs_to_loops(timeout_ms);
>> -
>> -       do {
>> -               status = readl(regs + S3C64XX_SPI_STATUS);
>> -       } while (FIELD_GET(S3C64XX_SPI_ST_RX_FIFO_LVL, status) < max_fifo &&
>> -                --val);
>> -
>> -       /* return the actual received data length */
>> -       return FIELD_GET(S3C64XX_SPI_ST_RX_FIFO_LVL, status);
>> -}
>> -
>>  static int s3c64xx_wait_for_dma(struct s3c64xx_spi_driver_data *sdd,
>>                                 struct spi_transfer *xfer)
>>  {
>> @@ -553,13 +533,11 @@ static int s3c64xx_wait_for_pio(struct s3c64xx_spi_driver_data *sdd,
>>                                 struct spi_transfer *xfer, bool use_irq)
>>  {
>>         void __iomem *regs = sdd->regs;
>> +       u8 *buf = xfer->rx_buf;
>> +       unsigned long time_us;
>>         unsigned long val;
>> -       u32 status;
>> -       int loops;
>> -       u32 cpy_len;
>> -       u8 *buf;
>> +       u32 status, len;
>>         int ms;
>> -       unsigned long time_us;
>>
>>         /* microsecs to xfer 'len' bytes @ 'cur_speed' */
>>         time_us = (xfer->len * 8 * 1000 * 1000) / sdd->cur_speed;
>> @@ -582,48 +560,29 @@ static int s3c64xx_wait_for_pio(struct s3c64xx_spi_driver_data *sdd,
>>                 status = readl(regs + S3C64XX_SPI_STATUS);
>>         } while (FIELD_GET(S3C64XX_SPI_ST_RX_FIFO_LVL, status) < xfer->len &&
>>                  --val);
>> -
>>         if (!val)
>>                 return -EIO;
>>
>>         /* If it was only Tx */
>> -       if (!xfer->rx_buf) {
>> +       if (!buf) {
>>                 sdd->state &= ~TXBUSY;
>>                 return 0;
>>         }
>>
>> -       /*
>> -        * If the receive length is bigger than the controller fifo
>> -        * size, calculate the loops and read the fifo as many times.
>> -        * loops = length / max fifo size (calculated by using the
>> -        * fifo mask).
>> -        * For any size less than the fifo size the below code is
>> -        * executed atleast once.
>> -        */
>> -       loops = xfer->len / FIFO_DEPTH(sdd);
>> -       buf = xfer->rx_buf;
>> -       do {
>> -               /* wait for data to be received in the fifo */
>> -               cpy_len = s3c64xx_spi_wait_for_timeout(sdd,
>> -                                                      (loops ? ms : 0));
>> -
>> -               switch (sdd->cur_bpw) {
>> -               case 32:
>> -                       ioread32_rep(regs + S3C64XX_SPI_RX_DATA,
>> -                                    buf, cpy_len / 4);
>> -                       break;
>> -               case 16:
>> -                       ioread16_rep(regs + S3C64XX_SPI_RX_DATA,
>> -                                    buf, cpy_len / 2);
>> -                       break;
>> -               default:
>> -                       ioread8_rep(regs + S3C64XX_SPI_RX_DATA,
>> -                                   buf, cpy_len);
>> -                       break;
>> -               }
>> +       len = FIELD_GET(S3C64XX_SPI_ST_RX_FIFO_LVL, status);
>> +
>> +       switch (sdd->cur_bpw) {
>> +       case 32:
>> +               ioread32_rep(regs + S3C64XX_SPI_RX_DATA, buf, len / 4);
>> +               break;
>> +       case 16:
>> +               ioread16_rep(regs + S3C64XX_SPI_RX_DATA, buf, len / 2);
>> +               break;
>> +       default:
>> +               ioread8_rep(regs + S3C64XX_SPI_RX_DATA, buf, len);
>> +               break;
>> +       }
>>
>> -               buf = buf + cpy_len;
>> -       } while (loops--);
>>         sdd->state &= ~RXBUSY;
>>
>>         return 0;
>> --
>> 2.43.0.429.g432eaa2c6b-goog
>>
Sam Protsenko Jan. 26, 2024, 7:31 p.m. UTC | #3
On Fri, Jan 26, 2024 at 1:56 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
> On 1/25/24 20:43, Sam Protsenko wrote:
> > On Thu, Jan 25, 2024 at 8:50 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
> >>
> >> s3c64xx_spi_transfer_one() makes sure that for PIO the xfer->len is
> >> always smaller than the fifo size. Since we can't receive more that the
> >> FIFO size, droop the loop handling, the code becomes less misleading.
> >
> > Drop (spelling)?
>
> oh yeah, thanks.
>
> >
> > For the patch: how exactly it was tested to make sure there is no regression?
>
> no regression testing for the entire patch set, I have just a gs101 on
> my hands.
>
> However, we shouldn't refrain ourselves on improving things when we
> think they're straight forward and they worth it. In this particular

This patch clearly brings a functional change. The way I see things,
the risk of having a regression outweighs the benefits of this
refactoring. I don't think it's even methodologically right to apply
such changes without thoroughly testing it first. It might be ok for
super-easy one-line cleanups, but that's not one of those.

> case, for PIO, s3c64xx_spi_transfer_one() does:
>         xfer->len = fifo_len - 1;
> then in s3c64xx_enable_datapath() we write xfer->len and then in
> s3c64xx_wait_for_pio() we code did the following:
>         loops = xfer->len / FIFO_DEPTH(sdd);
> loops is always zero, this is bogus and we shall remove it.
>

[snip]
diff mbox series

Patch

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index d2dd28ff00c6..00a0878aeb80 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -485,26 +485,6 @@  static int s3c64xx_enable_datapath(struct s3c64xx_spi_driver_data *sdd,
 	return 0;
 }
 
-static u32 s3c64xx_spi_wait_for_timeout(struct s3c64xx_spi_driver_data *sdd,
-					int timeout_ms)
-{
-	void __iomem *regs = sdd->regs;
-	unsigned long val = 1;
-	u32 status;
-	u32 max_fifo = FIFO_DEPTH(sdd);
-
-	if (timeout_ms)
-		val = msecs_to_loops(timeout_ms);
-
-	do {
-		status = readl(regs + S3C64XX_SPI_STATUS);
-	} while (FIELD_GET(S3C64XX_SPI_ST_RX_FIFO_LVL, status) < max_fifo &&
-		 --val);
-
-	/* return the actual received data length */
-	return FIELD_GET(S3C64XX_SPI_ST_RX_FIFO_LVL, status);
-}
-
 static int s3c64xx_wait_for_dma(struct s3c64xx_spi_driver_data *sdd,
 				struct spi_transfer *xfer)
 {
@@ -553,13 +533,11 @@  static int s3c64xx_wait_for_pio(struct s3c64xx_spi_driver_data *sdd,
 				struct spi_transfer *xfer, bool use_irq)
 {
 	void __iomem *regs = sdd->regs;
+	u8 *buf = xfer->rx_buf;
+	unsigned long time_us;
 	unsigned long val;
-	u32 status;
-	int loops;
-	u32 cpy_len;
-	u8 *buf;
+	u32 status, len;
 	int ms;
-	unsigned long time_us;
 
 	/* microsecs to xfer 'len' bytes @ 'cur_speed' */
 	time_us = (xfer->len * 8 * 1000 * 1000) / sdd->cur_speed;
@@ -582,48 +560,29 @@  static int s3c64xx_wait_for_pio(struct s3c64xx_spi_driver_data *sdd,
 		status = readl(regs + S3C64XX_SPI_STATUS);
 	} while (FIELD_GET(S3C64XX_SPI_ST_RX_FIFO_LVL, status) < xfer->len &&
 		 --val);
-
 	if (!val)
 		return -EIO;
 
 	/* If it was only Tx */
-	if (!xfer->rx_buf) {
+	if (!buf) {
 		sdd->state &= ~TXBUSY;
 		return 0;
 	}
 
-	/*
-	 * If the receive length is bigger than the controller fifo
-	 * size, calculate the loops and read the fifo as many times.
-	 * loops = length / max fifo size (calculated by using the
-	 * fifo mask).
-	 * For any size less than the fifo size the below code is
-	 * executed atleast once.
-	 */
-	loops = xfer->len / FIFO_DEPTH(sdd);
-	buf = xfer->rx_buf;
-	do {
-		/* wait for data to be received in the fifo */
-		cpy_len = s3c64xx_spi_wait_for_timeout(sdd,
-						       (loops ? ms : 0));
-
-		switch (sdd->cur_bpw) {
-		case 32:
-			ioread32_rep(regs + S3C64XX_SPI_RX_DATA,
-				     buf, cpy_len / 4);
-			break;
-		case 16:
-			ioread16_rep(regs + S3C64XX_SPI_RX_DATA,
-				     buf, cpy_len / 2);
-			break;
-		default:
-			ioread8_rep(regs + S3C64XX_SPI_RX_DATA,
-				    buf, cpy_len);
-			break;
-		}
+	len = FIELD_GET(S3C64XX_SPI_ST_RX_FIFO_LVL, status);
+
+	switch (sdd->cur_bpw) {
+	case 32:
+		ioread32_rep(regs + S3C64XX_SPI_RX_DATA, buf, len / 4);
+		break;
+	case 16:
+		ioread16_rep(regs + S3C64XX_SPI_RX_DATA, buf, len / 2);
+		break;
+	default:
+		ioread8_rep(regs + S3C64XX_SPI_RX_DATA, buf, len);
+		break;
+	}
 
-		buf = buf + cpy_len;
-	} while (loops--);
 	sdd->state &= ~RXBUSY;
 
 	return 0;