[PATCH] i2c: i2c-s3c2410: Remove recently introduced performance overheads

[PATCH] i2c: i2c-s3c2410: Remove recently introduced performance overheads

[Mark Brown]

The changes in "i2c-s3c2410: use exponential back off while polling for
bus idle" remove the initial busy wait for I2C transfers to complete and
replace it with usleep_range() calls which will schedule.

Since for older SoCs I2C transfers would usually complete within an
extremely small number of CPU cycles there is a win from not having to
schedule.  This happens because on the older SoCs the cores run at a
smaller multiple of the speeds that the I2C bus is operating at; on more
modern SoCs the busy wait is less likely to be effective.

Fix the issue by restoring the busy wait, reducing the number of spins
from 20 to 3 which covers the overwhelming majority of I2C transfers on
the SoCs where the busy wait is effective.

Signed-off-by: Mark Brown <broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org>
---
 drivers/i2c/busses/i2c-s3c2410.c |   21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c
index 31f802b..9050821 100644
--- a/drivers/i2c/busses/i2c-s3c2410.c
+++ b/drivers/i2c/busses/i2c-s3c2410.c
@@ -530,6 +530,7 @@ static void s3c24xx_i2c_wait_idle(struct s3c24xx_i2c *i2c)
 	unsigned long iicstat;
 	ktime_t start, now;
 	unsigned long delay;
+	int spins;
 
 	/* ensure the stop has been through the bus */
 
@@ -542,12 +543,22 @@ static void s3c24xx_i2c_wait_idle(struct s3c24xx_i2c *i2c)
 	 * end of a transaction.  However, really slow i2c devices can stretch
 	 * the clock, delaying STOP generation.
 	 *
-	 * As a compromise between idle detection latency for the normal, fast
-	 * case, and system load in the slow device case, use an exponential
-	 * back off in the polling loop, up to 1/10th of the total timeout,
-	 * then continue to poll at a constant rate up to the timeout.
+	 * On slower SoCs this typically happens within a very small number of
+	 * instructions so busy wait briefly to avoid scheduling overhead.
+	 */
+	spins = 3;
+	do {
+		cpu_relax();
+		iicstat = readl(i2c->regs + S3C2410_IICSTAT);
+	} while ((iicstat & S3C2410_IICSTAT_START) && --spins);
+
+	/*
+	 * If we do get an appreciable delay as a compromise between idle
+	 * detection latency for the normal, fast case, and system load in the
+	 * slow device case, use an exponential back off in the polling loop,
+	 * up to 1/10th of the total timeout, then continue to poll at a
+	 * constant rate up to the timeout.
 	 */
-	iicstat = readl(i2c->regs + S3C2410_IICSTAT);
 	delay = 1;
 	while ((iicstat & S3C2410_IICSTAT_START) &&
 	       ktime_us_delta(now, start) < S3C2410_IDLE_TIMEOUT) {
-- 
1.7.10.4

Re: [PATCH] i2c: i2c-s3c2410: Remove recently introduced performance overheads

[Daniel Kurtz]

On Tue, Nov 20, 2012 at 1:57 PM, Mark Brown
<broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org> wrote:
> The changes in "i2c-s3c2410: use exponential back off while polling for
> bus idle" remove the initial busy wait for I2C transfers to complete and
> replace it with usleep_range() calls which will schedule.
>
> Since for older SoCs I2C transfers would usually complete within an
> extremely small number of CPU cycles there is a win from not having to
> schedule.  This happens because on the older SoCs the cores run at a
> smaller multiple of the speeds that the I2C bus is operating at; on more
> modern SoCs the busy wait is less likely to be effective.
>
> Fix the issue by restoring the busy wait, reducing the number of spins
> from 20 to 3 which covers the overwhelming majority of I2C transfers on
> the SoCs where the busy wait is effective.
>
> Signed-off-by: Mark Brown <broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-s3c2410.c |   21 ++++++++++++++++-----
>  1 file changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c
> index 31f802b..9050821 100644
> --- a/drivers/i2c/busses/i2c-s3c2410.c
> +++ b/drivers/i2c/busses/i2c-s3c2410.c
> @@ -530,6 +530,7 @@ static void s3c24xx_i2c_wait_idle(struct s3c24xx_i2c *i2c)
>         unsigned long iicstat;
>         ktime_t start, now;
>         unsigned long delay;
> +       int spins;
>
>         /* ensure the stop has been through the bus */
>
> @@ -542,12 +543,22 @@ static void s3c24xx_i2c_wait_idle(struct s3c24xx_i2c *i2c)
>          * end of a transaction.  However, really slow i2c devices can stretch
>          * the clock, delaying STOP generation.
>          *
> -        * As a compromise between idle detection latency for the normal, fast
> -        * case, and system load in the slow device case, use an exponential
> -        * back off in the polling loop, up to 1/10th of the total timeout,
> -        * then continue to poll at a constant rate up to the timeout.
> +        * On slower SoCs this typically happens within a very small number of
> +        * instructions so busy wait briefly to avoid scheduling overhead.
> +        */
> +       spins = 3;
> +       do {
> +               cpu_relax();
> +               iicstat = readl(i2c->regs + S3C2410_IICSTAT);
> +       } while ((iicstat & S3C2410_IICSTAT_START) && --spins);

Can you avoid one cpu_relax() by reading IICSTAT first, and switch to
while { } instead of do { } while ()?

> +
> +       /*
> +        * If we do get an appreciable delay as a compromise between idle
> +        * detection latency for the normal, fast case, and system load in the
> +        * slow device case, use an exponential back off in the polling loop,
> +        * up to 1/10th of the total timeout, then continue to poll at a
> +        * constant rate up to the timeout.
>          */
> -       iicstat = readl(i2c->regs + S3C2410_IICSTAT);
>         delay = 1;
>         while ((iicstat & S3C2410_IICSTAT_START) &&
>                ktime_us_delta(now, start) < S3C2410_IDLE_TIMEOUT) {
> --
> 1.7.10.4

Re: [PATCH] i2c: i2c-s3c2410: Remove recently introduced performance overheads

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 537 bytes --]

On Tue, Nov 20, 2012 at 05:45:55PM +0800, Daniel Kurtz wrote:
> On Tue, Nov 20, 2012 at 1:57 PM, Mark Brown

> > +       spins = 3;
> > +       do {
> > +               cpu_relax();
> > +               iicstat = readl(i2c->regs + S3C2410_IICSTAT);
> > +       } while ((iicstat & S3C2410_IICSTAT_START) && --spins);

> Can you avoid one cpu_relax() by reading IICSTAT first, and switch to
> while { } instead of do { } while ()?

Of course.  The CPUs that care about this are single core so the
cpu_relax() has no effect on them anyway.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH] i2c: i2c-s3c2410: Remove recently introduced performance overheads

[Mark Brown]

The changes in "i2c-s3c2410: use exponential back off while polling for
bus idle" remove the initial busy wait for I2C transfers to complete and
replace it with usleep_range() calls which will schedule.

Since for older SoCs I2C transfers would usually complete within an
extremely small number of CPU cycles there is a win from not having to
schedule.  This happens because on the older SoCs the cores run at a
smaller multiple of the speeds that the I2C bus is operating at; on more
modern SoCs the busy wait is less likely to be effective.

Fix the issue by restoring the busy wait, reducing the number of spins
from 20 to 3 which covers the overwhelming majority of I2C transfers on
the SoCs where the busy wait is effective.

Signed-off-by: Mark Brown <broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org>
---
 drivers/i2c/busses/i2c-s3c2410.c |   20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c
index 31f802b..25dc3e2 100644
--- a/drivers/i2c/busses/i2c-s3c2410.c
+++ b/drivers/i2c/busses/i2c-s3c2410.c
@@ -530,6 +530,7 @@ static void s3c24xx_i2c_wait_idle(struct s3c24xx_i2c *i2c)
 	unsigned long iicstat;
 	ktime_t start, now;
 	unsigned long delay;
+	int spins;
 
 	/* ensure the stop has been through the bus */
 
@@ -542,12 +543,23 @@ static void s3c24xx_i2c_wait_idle(struct s3c24xx_i2c *i2c)
 	 * end of a transaction.  However, really slow i2c devices can stretch
 	 * the clock, delaying STOP generation.
 	 *
-	 * As a compromise between idle detection latency for the normal, fast
-	 * case, and system load in the slow device case, use an exponential
-	 * back off in the polling loop, up to 1/10th of the total timeout,
-	 * then continue to poll at a constant rate up to the timeout.
+	 * On slower SoCs this typically happens within a very small number of
+	 * instructions so busy wait briefly to avoid scheduling overhead.
 	 */
+	spins = 3;
 	iicstat = readl(i2c->regs + S3C2410_IICSTAT);
+	while ((iicstat & S3C2410_IICSTAT_START) && --spins) {
+		cpu_relax();
+		iicstat = readl(i2c->regs + S3C2410_IICSTAT);
+	}
+
+	/*
+	 * If we do get an appreciable delay as a compromise between idle
+	 * detection latency for the normal, fast case, and system load in the
+	 * slow device case, use an exponential back off in the polling loop,
+	 * up to 1/10th of the total timeout, then continue to poll at a
+	 * constant rate up to the timeout.
+	 */
 	delay = 1;
 	while ((iicstat & S3C2410_IICSTAT_START) &&
 	       ktime_us_delta(now, start) < S3C2410_IDLE_TIMEOUT) {
-- 
1.7.10.4

Re: [PATCH] i2c: i2c-s3c2410: Remove recently introduced performance overheads

[Olof Johansson]

On Tue, Nov 20, 2012 at 8:12 PM, Mark Brown
<broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org> wrote:
> The changes in "i2c-s3c2410: use exponential back off while polling for
> bus idle" remove the initial busy wait for I2C transfers to complete and
> replace it with usleep_range() calls which will schedule.
>
> Since for older SoCs I2C transfers would usually complete within an
> extremely small number of CPU cycles there is a win from not having to
> schedule.  This happens because on the older SoCs the cores run at a
> smaller multiple of the speeds that the I2C bus is operating at; on more
> modern SoCs the busy wait is less likely to be effective.
>
> Fix the issue by restoring the busy wait, reducing the number of spins
> from 20 to 3 which covers the overwhelming majority of I2C transfers on
> the SoCs where the busy wait is effective.
>
> Signed-off-by: Mark Brown <broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org>

Acked-by: Olof Johansson <olof-nZhT3qVonbNeoWH0uzbU5w@public.gmane.org>


-Olof

Re: [PATCH] i2c: i2c-s3c2410: Remove recently introduced performance overheads

[Daniel Kurtz]

On Wed, Nov 21, 2012 at 1:08 PM, Olof Johansson <olof-nZhT3qVonbNeoWH0uzbU5w@public.gmane.org> wrote:
> On Tue, Nov 20, 2012 at 8:12 PM, Mark Brown
> <broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org> wrote:
>> The changes in "i2c-s3c2410: use exponential back off while polling for
>> bus idle" remove the initial busy wait for I2C transfers to complete and
>> replace it with usleep_range() calls which will schedule.
>>
>> Since for older SoCs I2C transfers would usually complete within an
>> extremely small number of CPU cycles there is a win from not having to
>> schedule.  This happens because on the older SoCs the cores run at a
>> smaller multiple of the speeds that the I2C bus is operating at; on more
>> modern SoCs the busy wait is less likely to be effective.
>>
>> Fix the issue by restoring the busy wait, reducing the number of spins
>> from 20 to 3 which covers the overwhelming majority of I2C transfers on
>> the SoCs where the busy wait is effective.
>>
>> Signed-off-by: Mark Brown <broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org>
>
> Acked-by: Olof Johansson <olof-nZhT3qVonbNeoWH0uzbU5w@public.gmane.org>

Reviewed-by: Daniel Kurtz <djkurtz-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>

>
> -Olof

Re: [PATCH] i2c: i2c-s3c2410: Remove recently introduced performance overheads

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1134 bytes --]

On Wed, Nov 21, 2012 at 01:12:11PM +0900, Mark Brown wrote:
> The changes in "i2c-s3c2410: use exponential back off while polling for
> bus idle" remove the initial busy wait for I2C transfers to complete and
> replace it with usleep_range() calls which will schedule.
> 
> Since for older SoCs I2C transfers would usually complete within an
> extremely small number of CPU cycles there is a win from not having to
> schedule.  This happens because on the older SoCs the cores run at a
> smaller multiple of the speeds that the I2C bus is operating at; on more
> modern SoCs the busy wait is less likely to be effective.
> 
> Fix the issue by restoring the busy wait, reducing the number of spins
> from 20 to 3 which covers the overwhelming majority of I2C transfers on
> the SoCs where the busy wait is effective.
> 
> Signed-off-by: Mark Brown <broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org>

Applied to for-next, thanks everyone!

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]