| Message ID | 20230419060639.38853-4-jaewon02.kim@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Improves polling mode of s3c64xx driver | expand |
On 19/04/2023 08:06, Jaewon Kim wrote: > In polling mode, the status register is constantly read to check transfer > completion. It cause excessive CPU usage. > So, it calculates the SPI transfer time and made it sleep. > > Signed-off-by: Jaewon Kim <jaewon02.kim@samsung.com> > --- > drivers/spi/spi-s3c64xx.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c > index 886722fb40ea..cf3060b2639b 100644 > --- a/drivers/spi/spi-s3c64xx.c > +++ b/drivers/spi/spi-s3c64xx.c > @@ -561,6 +561,14 @@ static int s3c64xx_wait_for_pio(struct s3c64xx_spi_driver_data *sdd, > u32 cpy_len; > u8 *buf; > int ms; > + u32 tx_time; > + > + /* sleep during signal transfer time */ > + status = readl(regs + S3C64XX_SPI_STATUS); > + if (RX_FIFO_LVL(status, sdd) < xfer->len) { > + tx_time = (xfer->len * 8 * 1000 * 1000) / sdd->cur_speed; > + usleep_range(tx_time / 2, tx_time); > + } Did you actually check the delays introduced by it? Is it worth? > > /* millisecs to xfer 'len' bytes @ 'cur_speed' */ > ms = xfer->len * 8 * 1000 / sdd->cur_speed; You have now some code duplication so this could be combined. Best regards, Krzysztof
On 23. 4. 19. 17:19, Krzysztof Kozlowski wrote: > On 19/04/2023 08:06, Jaewon Kim wrote: >> In polling mode, the status register is constantly read to check transfer >> completion. It cause excessive CPU usage. >> So, it calculates the SPI transfer time and made it sleep. >> >> Signed-off-by: Jaewon Kim <jaewon02.kim@samsung.com> >> --- >> drivers/spi/spi-s3c64xx.c | 8 ++++++++ >> 1 file changed, 8 insertions(+) >> >> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c >> index 886722fb40ea..cf3060b2639b 100644 >> --- a/drivers/spi/spi-s3c64xx.c >> +++ b/drivers/spi/spi-s3c64xx.c >> @@ -561,6 +561,14 @@ static int s3c64xx_wait_for_pio(struct s3c64xx_spi_driver_data *sdd, >> u32 cpy_len; >> u8 *buf; >> int ms; >> + u32 tx_time; >> + >> + /* sleep during signal transfer time */ >> + status = readl(regs + S3C64XX_SPI_STATUS); >> + if (RX_FIFO_LVL(status, sdd) < xfer->len) { >> + tx_time = (xfer->len * 8 * 1000 * 1000) / sdd->cur_speed; >> + usleep_range(tx_time / 2, tx_time); >> + } > Did you actually check the delays introduced by it? Is it worth? Yes, I already test it. Throughput was the same, CPU utilization decreased to 30~40% from 100%. Tested board is ExynosAutov9 SADK. > >> >> /* millisecs to xfer 'len' bytes @ 'cur_speed' */ >> ms = xfer->len * 8 * 1000 / sdd->cur_speed; > You have now some code duplication so this could be combined. > > Best regards, > Krzysztof > > Thanks Jaewon Kim
Hi Jaewon, > >> In polling mode, the status register is constantly read to check transfer > >> completion. It cause excessive CPU usage. > >> So, it calculates the SPI transfer time and made it sleep. > >> > >> Signed-off-by: Jaewon Kim <jaewon02.kim@samsung.com> > >> --- > >> drivers/spi/spi-s3c64xx.c | 8 ++++++++ > >> 1 file changed, 8 insertions(+) > >> > >> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c > >> index 886722fb40ea..cf3060b2639b 100644 > >> --- a/drivers/spi/spi-s3c64xx.c > >> +++ b/drivers/spi/spi-s3c64xx.c > >> @@ -561,6 +561,14 @@ static int s3c64xx_wait_for_pio(struct s3c64xx_spi_driver_data *sdd, > >> u32 cpy_len; > >> u8 *buf; > >> int ms; > >> + u32 tx_time; > >> + > >> + /* sleep during signal transfer time */ > >> + status = readl(regs + S3C64XX_SPI_STATUS); > >> + if (RX_FIFO_LVL(status, sdd) < xfer->len) { > >> + tx_time = (xfer->len * 8 * 1000 * 1000) / sdd->cur_speed; > >> + usleep_range(tx_time / 2, tx_time); > >> + } > > Did you actually check the delays introduced by it? Is it worth? > > Yes, I already test it. > > Throughput was the same, CPU utilization decreased to 30~40% from 100%. > > Tested board is ExynosAutov9 SADK. > > > > > >> > >> /* millisecs to xfer 'len' bytes @ 'cur_speed' */ > >> ms = xfer->len * 8 * 1000 / sdd->cur_speed; > > You have now some code duplication so this could be combined. you could put the 'if' under the 'ms = ...' and just use ms without declaring any tx_time. Andi
Hi Andi, On 23. 4. 20. 00:56, Andi Shyti wrote: > Hi Jaewon, > >>>> In polling mode, the status register is constantly read to check transfer >>>> completion. It cause excessive CPU usage. >>>> So, it calculates the SPI transfer time and made it sleep. >>>> >>>> Signed-off-by: Jaewon Kim <jaewon02.kim@samsung.com> >>>> --- >>>> drivers/spi/spi-s3c64xx.c | 8 ++++++++ >>>> 1 file changed, 8 insertions(+) >>>> >>>> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c >>>> index 886722fb40ea..cf3060b2639b 100644 >>>> --- a/drivers/spi/spi-s3c64xx.c >>>> +++ b/drivers/spi/spi-s3c64xx.c >>>> @@ -561,6 +561,14 @@ static int s3c64xx_wait_for_pio(struct s3c64xx_spi_driver_data *sdd, >>>> u32 cpy_len; >>>> u8 *buf; >>>> int ms; >>>> + u32 tx_time; >>>> + >>>> + /* sleep during signal transfer time */ >>>> + status = readl(regs + S3C64XX_SPI_STATUS); >>>> + if (RX_FIFO_LVL(status, sdd) < xfer->len) { >>>> + tx_time = (xfer->len * 8 * 1000 * 1000) / sdd->cur_speed; >>>> + usleep_range(tx_time / 2, tx_time); >>>> + } >>> Did you actually check the delays introduced by it? Is it worth? >> Yes, I already test it. >> >> Throughput was the same, CPU utilization decreased to 30~40% from 100%. >> >> Tested board is ExynosAutov9 SADK. >> >> >>>> >>>> /* millisecs to xfer 'len' bytes @ 'cur_speed' */ >>>> ms = xfer->len * 8 * 1000 / sdd->cur_speed; >>> You have now some code duplication so this could be combined. > you could put the 'if' under the 'ms = ...' and just use ms > without declaring any tx_time. > > Andi The unit of 'tx_time' is 'us'. tx_time = (xfer->len * 8 * 1000 * 1000) / sdd->cur_speed; ms = xfer->len * 8 * 1000 / sdd->cur_speed; I add tx_time to minimize existing code modifications. If we are not using tx_time, we need to change ms to us and change the related code. Thanks Jaewon Kim
diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c index 886722fb40ea..cf3060b2639b 100644 --- a/drivers/spi/spi-s3c64xx.c +++ b/drivers/spi/spi-s3c64xx.c @@ -561,6 +561,14 @@ static int s3c64xx_wait_for_pio(struct s3c64xx_spi_driver_data *sdd, u32 cpy_len; u8 *buf; int ms; + u32 tx_time; + + /* sleep during signal transfer time */ + status = readl(regs + S3C64XX_SPI_STATUS); + if (RX_FIFO_LVL(status, sdd) < xfer->len) { + tx_time = (xfer->len * 8 * 1000 * 1000) / sdd->cur_speed; + usleep_range(tx_time / 2, tx_time); + } /* millisecs to xfer 'len' bytes @ 'cur_speed' */ ms = xfer->len * 8 * 1000 / sdd->cur_speed;
In polling mode, the status register is constantly read to check transfer completion. It cause excessive CPU usage. So, it calculates the SPI transfer time and made it sleep. Signed-off-by: Jaewon Kim <jaewon02.kim@samsung.com> --- drivers/spi/spi-s3c64xx.c | 8 ++++++++ 1 file changed, 8 insertions(+)