| Message ID | 20230419060639.38853-3-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: > Adds cpu_relax() to prevent long busy-wait. How cpu_relax prevents long waiting? > There is busy-wait loop to check data transfer completion in polling mode. > > Signed-off-by: Jaewon Kim <jaewon02.kim@samsung.com> > --- > drivers/spi/spi-s3c64xx.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c > index 273aa02322d9..886722fb40ea 100644 > --- a/drivers/spi/spi-s3c64xx.c > +++ b/drivers/spi/spi-s3c64xx.c > @@ -568,6 +568,7 @@ static int s3c64xx_wait_for_pio(struct s3c64xx_spi_driver_data *sdd, > > val = msecs_to_loops(ms); > do { > + cpu_relax(); Shouldn't this be just readl_poll_timeout()? Or the syntax would be too complicated? > status = readl(regs + S3C64XX_SPI_STATUS); > } while (RX_FIFO_LVL(status, sdd) < xfer->len && --val); > Best regards, Krzysztof
On 23. 4. 19. 17:14, Krzysztof Kozlowski wrote: > On 19/04/2023 08:06, Jaewon Kim wrote: >> Adds cpu_relax() to prevent long busy-wait. > How cpu_relax prevents long waiting? As I know, cpu_relax() can be converted to yield. This can prevent excessive use of the CPU in busy-loop. I'll replace poor sentence like below in v3. ("Adds cpu_relax() to allow CPU relaxation in busy-loop") >> There is busy-wait loop to check data transfer completion in polling mode. >> >> Signed-off-by: Jaewon Kim<jaewon02.kim@samsung.com> >> --- >> drivers/spi/spi-s3c64xx.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c >> index 273aa02322d9..886722fb40ea 100644 >> --- a/drivers/spi/spi-s3c64xx.c >> +++ b/drivers/spi/spi-s3c64xx.c >> @@ -568,6 +568,7 @@ static int s3c64xx_wait_for_pio(struct s3c64xx_spi_driver_data *sdd, >> >> val = msecs_to_loops(ms); >> do { >> + cpu_relax(); > Shouldn't this be just readl_poll_timeout()? Or the syntax would be too > complicated? I think we can replace this while() loop to readl_poll_timeout(). However, we should use 0 value as 'delay_us' parameter. Because delay can affect throughput. My purpose is add relax to this busy-loop. we cannot give relax if we change to readl_poll_timeout(). >> status = readl(regs + S3C64XX_SPI_STATUS); >> } while (RX_FIFO_LVL(status, sdd) < xfer->len && --val); >> > Best regards, > Krzysztof > > Thanks Jaewon Kim
On 19/04/2023 13:13, Jaewon Kim wrote: > > On 23. 4. 19. 17:14, Krzysztof Kozlowski wrote: >> On 19/04/2023 08:06, Jaewon Kim wrote: >>> Adds cpu_relax() to prevent long busy-wait. >> How cpu_relax prevents long waiting? > > As I know, cpu_relax() can be converted to yield. This can prevent > excessive use of the CPU in busy-loop. That's ok, you just wrote that it will prevent long waiting, so I assume it will shorten the wait time. > > I'll replace poor sentence like below in v3. > > ("Adds cpu_relax() to allow CPU relaxation in busy-loop") > >>> There is busy-wait loop to check data transfer completion in polling mode. >>> >>> Signed-off-by: Jaewon Kim<jaewon02.kim@samsung.com> >>> --- >>> drivers/spi/spi-s3c64xx.c | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c >>> index 273aa02322d9..886722fb40ea 100644 >>> --- a/drivers/spi/spi-s3c64xx.c >>> +++ b/drivers/spi/spi-s3c64xx.c >>> @@ -568,6 +568,7 @@ static int s3c64xx_wait_for_pio(struct s3c64xx_spi_driver_data *sdd, >>> >>> val = msecs_to_loops(ms); >>> do { >>> + cpu_relax(); >> Shouldn't this be just readl_poll_timeout()? Or the syntax would be too >> complicated? > > I think we can replace this while() loop to readl_poll_timeout(). > > However, we should use 0 value as 'delay_us' parameter. Because delay > can affect throughput. > > > My purpose is add relax to this busy-loop. > > we cannot give relax if we change to readl_poll_timeout(). readl_poll_timeout() will know to do the best. You do not need to add cpu_relax there. Best regards, Krzysztof
On 23. 4. 21. 00:39, Krzysztof Kozlowski wrote: > On 19/04/2023 13:13, Jaewon Kim wrote: >> On 23. 4. 19. 17:14, Krzysztof Kozlowski wrote: >>> On 19/04/2023 08:06, Jaewon Kim wrote: >>>> Adds cpu_relax() to prevent long busy-wait. >>> How cpu_relax prevents long waiting? >> As I know, cpu_relax() can be converted to yield. This can prevent >> excessive use of the CPU in busy-loop. > That's ok, you just wrote that it will prevent long waiting, so I assume > it will shorten the wait time. > >> I'll replace poor sentence like below in v3. >> >> ("Adds cpu_relax() to allow CPU relaxation in busy-loop") >> >>>> There is busy-wait loop to check data transfer completion in polling mode. >>>> >>>> Signed-off-by: Jaewon Kim<jaewon02.kim@samsung.com> >>>> --- >>>> drivers/spi/spi-s3c64xx.c | 1 + >>>> 1 file changed, 1 insertion(+) >>>> >>>> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c >>>> index 273aa02322d9..886722fb40ea 100644 >>>> --- a/drivers/spi/spi-s3c64xx.c >>>> +++ b/drivers/spi/spi-s3c64xx.c >>>> @@ -568,6 +568,7 @@ static int s3c64xx_wait_for_pio(struct s3c64xx_spi_driver_data *sdd, >>>> >>>> val = msecs_to_loops(ms); >>>> do { >>>> + cpu_relax(); >>> Shouldn't this be just readl_poll_timeout()? Or the syntax would be too >>> complicated? >> I think we can replace this while() loop to readl_poll_timeout(). >> >> However, we should use 0 value as 'delay_us' parameter. Because delay >> can affect throughput. >> >> >> My purpose is add relax to this busy-loop. >> >> we cannot give relax if we change to readl_poll_timeout(). > readl_poll_timeout() will know to do the best. You do not need to add > cpu_relax there. Okay, I will change it to readl_poll_timeout() > > Best regards, > Krzysztof > > Thanks Jaewon Kim
diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c index 273aa02322d9..886722fb40ea 100644 --- a/drivers/spi/spi-s3c64xx.c +++ b/drivers/spi/spi-s3c64xx.c @@ -568,6 +568,7 @@ static int s3c64xx_wait_for_pio(struct s3c64xx_spi_driver_data *sdd, val = msecs_to_loops(ms); do { + cpu_relax(); status = readl(regs + S3C64XX_SPI_STATUS); } while (RX_FIFO_LVL(status, sdd) < xfer->len && --val);
Adds cpu_relax() to prevent long busy-wait. There is busy-wait loop to check data transfer completion in polling mode. Signed-off-by: Jaewon Kim <jaewon02.kim@samsung.com> --- drivers/spi/spi-s3c64xx.c | 1 + 1 file changed, 1 insertion(+)