Improves polling mode of s3c64xx driver

[PATCH v2 3/4] spi: s3c64xx: add sleep during transfer

Posted by Jaewon Kim 2 years, 8 months ago

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);
+	}
 
 	/* millisecs to xfer 'len' bytes @ 'cur_speed' */
 	ms = xfer->len * 8 * 1000 / sdd->cur_speed;
-- 
2.17.1

Re: [PATCH v2 3/4] spi: s3c64xx: add sleep during transfer

Posted by Krzysztof Kozlowski 2 years, 8 months ago

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

Re: [PATCH v2 3/4] spi: s3c64xx: add sleep during transfer

Posted by Jaewon Kim 2 years, 8 months ago

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

Re: [PATCH v2 3/4] spi: s3c64xx: add sleep during transfer

Posted by Andi Shyti 2 years, 8 months ago

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

Re: [PATCH v2 3/4] spi: s3c64xx: add sleep during transfer

Posted by Jaewon Kim 2 years, 8 months ago

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