| Message ID | 20240125145007.748295-17-tudor.ambarus@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | spi: s3c64xx: winter cleanup and gs101 support | expand |
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 >
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 >>
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 --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;
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(-)