diff mbox series

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

Message ID 20230419060639.38853-4-jaewon02.kim@samsung.com (mailing list archive)
State New
Headers show
Series [v2,1/4] spi: s3c64xx: changed to PIO mode if there is no DMA | expand

Commit Message

Jaewon Kim April 19, 2023, 6:06 a.m. UTC
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(+)

Comments

Krzysztof Kozlowski April 19, 2023, 8:19 a.m. UTC | #1
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
Jaewon Kim April 19, 2023, 9:41 a.m. UTC | #2
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
Andi Shyti April 19, 2023, 3:56 p.m. UTC | #3
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
Jaewon Kim April 21, 2023, 2:53 a.m. UTC | #4
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 mbox series

Patch

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;