mail archive of the barebox mailing list
 help / color / mirror / Atom feed
* [PATCH] I2C: i.MX: early: Use internal udelay
@ 2023-01-26 18:56 John Watts
  2023-01-30 10:27 ` Sascha Hauer
  2023-02-01 17:50 ` Alexander Shiyan
  0 siblings, 2 replies; 16+ messages in thread
From: John Watts @ 2023-01-26 18:56 UTC (permalink / raw)
  To: barebox; +Cc: John Watts

udelay isn't provided in the PBL, so use our own definition.

This avoids boards having to define udelay in their code.

Signed-off-by: John Watts <contact@jookia.org>
---
 drivers/i2c/busses/i2c-imx-early.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-imx-early.c b/drivers/i2c/busses/i2c-imx-early.c
index 6c8bdc7904..fcf279eff8 100644
--- a/drivers/i2c/busses/i2c-imx-early.c
+++ b/drivers/i2c/busses/i2c-imx-early.c
@@ -90,6 +90,13 @@ static int i2c_fsl_acked(struct fsl_i2c *fsl_i2c)
 	return i2c_fsl_poll_status(fsl_i2c, 0, I2SR_RXAK);
 }
 
+static void __udelay(int us)
+{
+	volatile int i;
+
+	for (i = 0; i < us * 1000; i++);
+}
+
 static int i2c_fsl_start(struct fsl_i2c *fsl_i2c)
 {
 	unsigned int temp = 0;
@@ -104,7 +111,7 @@ static int i2c_fsl_start(struct fsl_i2c *fsl_i2c)
 			  fsl_i2c, FSL_I2C_I2CR);
 
 	/* Wait controller to be stable */
-	udelay(100);
+	__udelay(100);
 
 	/* Start I2C transaction */
 	temp = fsl_i2c_read_reg(fsl_i2c, FSL_I2C_I2CR);
-- 
2.39.1




^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] I2C: i.MX: early: Use internal udelay
  2023-01-26 18:56 [PATCH] I2C: i.MX: early: Use internal udelay John Watts
@ 2023-01-30 10:27 ` Sascha Hauer
  2023-01-30 10:42   ` John Watts
  2023-02-02 14:21   ` Jules Maselbas
  2023-02-01 17:50 ` Alexander Shiyan
  1 sibling, 2 replies; 16+ messages in thread
From: Sascha Hauer @ 2023-01-30 10:27 UTC (permalink / raw)
  To: John Watts; +Cc: barebox

On Fri, Jan 27, 2023 at 05:56:43AM +1100, John Watts wrote:
> udelay isn't provided in the PBL, so use our own definition.
> 
> This avoids boards having to define udelay in their code.
> 
> Signed-off-by: John Watts <contact@jookia.org>
> ---
>  drivers/i2c/busses/i2c-imx-early.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-imx-early.c b/drivers/i2c/busses/i2c-imx-early.c
> index 6c8bdc7904..fcf279eff8 100644
> --- a/drivers/i2c/busses/i2c-imx-early.c
> +++ b/drivers/i2c/busses/i2c-imx-early.c
> @@ -90,6 +90,13 @@ static int i2c_fsl_acked(struct fsl_i2c *fsl_i2c)
>  	return i2c_fsl_poll_status(fsl_i2c, 0, I2SR_RXAK);
>  }
>  
> +static void __udelay(int us)
> +{
> +	volatile int i;
> +
> +	for (i = 0; i < us * 1000; i++);
> +}

This takes around 5 times too long on a i.MX8MM and around 50 times too
long on a i.MX6Q. This was measured under a regular barebox on the
shell. In an early environment with MMU disabled it takes 730 times too
long.

Maybe we could do this:

static void __udelay(void *base, int us)
{
	int i;

	for (i = 0; i < us * 4; i++)
		readb(base);
}

The time spent for a register read depends on the bus clock which
doesn't change that much between the different SoCs.

Sascha


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] I2C: i.MX: early: Use internal udelay
  2023-01-30 10:27 ` Sascha Hauer
@ 2023-01-30 10:42   ` John Watts
  2023-01-30 12:17     ` Sascha Hauer
  2023-02-02 14:21   ` Jules Maselbas
  1 sibling, 1 reply; 16+ messages in thread
From: John Watts @ 2023-01-30 10:42 UTC (permalink / raw)
  To: Sascha Hauer; +Cc: barebox

On Mon, Jan 30, 2023 at 11:27:27AM +0100, Sascha Hauer wrote:
> This takes around 5 times too long on a i.MX8MM and around 50 times too
> long on a i.MX6Q. This was measured under a regular barebox on the
> shell. In an early environment with MMU disabled it takes 730 times too
> long.

Yikes! I'm glad I broke this in to its own patch then. :)

I did copy this code from these boards:

arch/arm/boards/cm-fx6/lowlevel.c
arch/arm/boards/skov-imx6/lowlevel.c
arch/arm/boards/technexion-wandboard/lowlevel.c
arch/arm/mach-imx/xload-gpmi-nand.c
arch/arm/mach-imx/imx6-mmdc.c (uses 1000 loops)

Maybe it's time for an i.MX6-wide early udelay?
Though this code might break if its timing is already wrong.

> Maybe we could do this:
> 
> static void __udelay(void *base, int us)
> {
> 	int i;
> 
> 	for (i = 0; i < us * 4; i++)
> 		readb(base);
> }
> 
> The time spent for a register read depends on the bus clock which
> doesn't change that much between the different SoCs.

This seems like a better solution if you've tested it, I'm not too sure
how to check this. Would I just then specify the controller as the base?

Though the actual goal of the code is to wait for the controller to bestable.
Maybe there's a better way than a delay?

> 
> Sascha

John.



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] I2C: i.MX: early: Use internal udelay
  2023-01-30 10:42   ` John Watts
@ 2023-01-30 12:17     ` Sascha Hauer
  2023-01-30 12:24       ` John Watts
  0 siblings, 1 reply; 16+ messages in thread
From: Sascha Hauer @ 2023-01-30 12:17 UTC (permalink / raw)
  To: John Watts; +Cc: barebox

On Mon, Jan 30, 2023 at 09:42:53PM +1100, John Watts wrote:
> On Mon, Jan 30, 2023 at 11:27:27AM +0100, Sascha Hauer wrote:
> > This takes around 5 times too long on a i.MX8MM and around 50 times too
> > long on a i.MX6Q. This was measured under a regular barebox on the
> > shell. In an early environment with MMU disabled it takes 730 times too
> > long.
> 
> Yikes! I'm glad I broke this in to its own patch then. :)
> 
> I did copy this code from these boards:
> 
> arch/arm/boards/cm-fx6/lowlevel.c
> arch/arm/boards/skov-imx6/lowlevel.c
> arch/arm/boards/technexion-wandboard/lowlevel.c
> arch/arm/mach-imx/xload-gpmi-nand.c
> arch/arm/mach-imx/imx6-mmdc.c (uses 1000 loops)
> 
> Maybe it's time for an i.MX6-wide early udelay?

i.MX6 is not enough, the code could run on other i.MX SoCs as well.

> Though this code might break if its timing is already wrong.
> 
> > Maybe we could do this:
> > 
> > static void __udelay(void *base, int us)
> > {
> > 	int i;
> > 
> > 	for (i = 0; i < us * 4; i++)
> > 		readb(base);
> > }
> > 
> > The time spent for a register read depends on the bus clock which
> > doesn't change that much between the different SoCs.
> 
> This seems like a better solution if you've tested it, I'm not too sure
> how to check this. Would I just then specify the controller as the base?

Yes, the I2C controller base should be passed here.

I tested this with a call to this udelay function with a sufficiently
great number of microseconds surrounded by a printf.

> 
> Though the actual goal of the code is to wait for the controller to bestable.
> Maybe there's a better way than a delay?

I briefly looked into this, but the commit history of both barebox or
Linux do not give a real conclusion what we are waiting here for.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] I2C: i.MX: early: Use internal udelay
  2023-01-30 12:17     ` Sascha Hauer
@ 2023-01-30 12:24       ` John Watts
  2023-01-30 12:31         ` Sascha Hauer
  0 siblings, 1 reply; 16+ messages in thread
From: John Watts @ 2023-01-30 12:24 UTC (permalink / raw)
  To: Sascha Hauer; +Cc: barebox

On Mon, Jan 30, 2023 at 01:17:52PM +0100, Sascha Hauer wrote:
> On Mon, Jan 30, 2023 at 09:42:53PM +1100, John Watts wrote:
> > On Mon, Jan 30, 2023 at 11:27:27AM +0100, Sascha Hauer wrote:
> > > This takes around 5 times too long on a i.MX8MM and around 50 times too
> > > long on a i.MX6Q. This was measured under a regular barebox on the
> > > shell. In an early environment with MMU disabled it takes 730 times too
> > > long.
> > 
> > Yikes! I'm glad I broke this in to its own patch then. :)
> > 
> > I did copy this code from these boards:
> > 
> > arch/arm/boards/cm-fx6/lowlevel.c
> > arch/arm/boards/skov-imx6/lowlevel.c
> > arch/arm/boards/technexion-wandboard/lowlevel.c
> > arch/arm/mach-imx/xload-gpmi-nand.c
> > arch/arm/mach-imx/imx6-mmdc.c (uses 1000 loops)
> > 
> > Maybe it's time for an i.MX6-wide early udelay?
> 
> i.MX6 is not enough, the code could run on other i.MX SoCs as well.

Do you think a patch that implements udelay in PBL using the code below with
a fixed base and drops these implementations would be a good idea?

> I tested this with a call to this udelay function with a sufficiently
> great number of microseconds surrounded by a printf.

Ah okay.

> I briefly looked into this, but the commit history of both barebox or
> Linux do not give a real conclusion what we are waiting here for.
> 
> Sascha

I see.

John.



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] I2C: i.MX: early: Use internal udelay
  2023-01-30 12:24       ` John Watts
@ 2023-01-30 12:31         ` Sascha Hauer
  2023-01-30 12:56           ` John Watts
  0 siblings, 1 reply; 16+ messages in thread
From: Sascha Hauer @ 2023-01-30 12:31 UTC (permalink / raw)
  To: John Watts; +Cc: barebox

On Mon, Jan 30, 2023 at 11:24:55PM +1100, John Watts wrote:
> On Mon, Jan 30, 2023 at 01:17:52PM +0100, Sascha Hauer wrote:
> > On Mon, Jan 30, 2023 at 09:42:53PM +1100, John Watts wrote:
> > > On Mon, Jan 30, 2023 at 11:27:27AM +0100, Sascha Hauer wrote:
> > > > This takes around 5 times too long on a i.MX8MM and around 50 times too
> > > > long on a i.MX6Q. This was measured under a regular barebox on the
> > > > shell. In an early environment with MMU disabled it takes 730 times too
> > > > long.
> > > 
> > > Yikes! I'm glad I broke this in to its own patch then. :)
> > > 
> > > I did copy this code from these boards:
> > > 
> > > arch/arm/boards/cm-fx6/lowlevel.c
> > > arch/arm/boards/skov-imx6/lowlevel.c
> > > arch/arm/boards/technexion-wandboard/lowlevel.c
> > > arch/arm/mach-imx/xload-gpmi-nand.c
> > > arch/arm/mach-imx/imx6-mmdc.c (uses 1000 loops)
> > > 
> > > Maybe it's time for an i.MX6-wide early udelay?
> > 
> > i.MX6 is not enough, the code could run on other i.MX SoCs as well.
> 
> Do you think a patch that implements udelay in PBL using the code below with
> a fixed base and drops these implementations would be a good idea?

As said, the code is also for other i.MX SoCs, so a fixed base address
won't do it. You would first have to detect the SoC type, but we can
only get this from the device tree.

I'm afraid there is no good solution for this problem, at least we
haven't found any over the years.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] I2C: i.MX: early: Use internal udelay
  2023-01-30 12:31         ` Sascha Hauer
@ 2023-01-30 12:56           ` John Watts
  2023-01-30 16:36             ` Sascha Hauer
  0 siblings, 1 reply; 16+ messages in thread
From: John Watts @ 2023-01-30 12:56 UTC (permalink / raw)
  To: Sascha Hauer; +Cc: barebox

On Mon, Jan 30, 2023 at 01:31:42PM +0100, Sascha Hauer wrote:
> As said, the code is also for other i.MX SoCs, so a fixed base address
> won't do it. You would first have to detect the SoC type, but we can
> only get this from the device tree.
> 
> I'm afraid there is no good solution for this problem, at least we
> haven't found any over the years.
> 
> Sascha

Oh right, because Barebox does multi-image builds. That is an unsatisfying
problem but it makes sense.

So for this patch I imagine I should just take the udelay you've written,
use the I2C registers as the base address and just poke around for a while?

I'm still unsure on how to test this- can I just disable the MMU and dcache
in board.c and do some prints with some type of timer?

John.



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] I2C: i.MX: early: Use internal udelay
  2023-01-30 12:56           ` John Watts
@ 2023-01-30 16:36             ` Sascha Hauer
  2023-01-30 18:42               ` John Watts
  0 siblings, 1 reply; 16+ messages in thread
From: Sascha Hauer @ 2023-01-30 16:36 UTC (permalink / raw)
  To: John Watts; +Cc: barebox

On Mon, Jan 30, 2023 at 11:56:20PM +1100, John Watts wrote:
> On Mon, Jan 30, 2023 at 01:31:42PM +0100, Sascha Hauer wrote:
> > As said, the code is also for other i.MX SoCs, so a fixed base address
> > won't do it. You would first have to detect the SoC type, but we can
> > only get this from the device tree.
> > 
> > I'm afraid there is no good solution for this problem, at least we
> > haven't found any over the years.
> > 
> > Sascha
> 
> Oh right, because Barebox does multi-image builds. That is an unsatisfying
> problem but it makes sense.
> 
> So for this patch I imagine I should just take the udelay you've written,
> use the I2C registers as the base address and just poke around for a while?

Yes, sounds good.

> 
> I'm still unsure on how to test this- can I just disable the MMU and dcache
> in board.c and do some prints with some type of timer?

You can't disable MMU during runtime, but you can compile without MMU
support, just disable CONFIG_MMU. However, the early I2C code already
runs with MMU disabled, can't you just put in some test code there?

Sascha


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] I2C: i.MX: early: Use internal udelay
  2023-01-30 16:36             ` Sascha Hauer
@ 2023-01-30 18:42               ` John Watts
  2023-01-31  6:14                 ` Sascha Hauer
  0 siblings, 1 reply; 16+ messages in thread
From: John Watts @ 2023-01-30 18:42 UTC (permalink / raw)
  To: Sascha Hauer; +Cc: barebox

On Mon, Jan 30, 2023 at 05:36:58PM +0100, Sascha Hauer wrote:
> You can't disable MMU during runtime, but you can compile without MMU
> support, just disable CONFIG_MMU. However, the early I2C code already
> runs with MMU disabled, can't you just put in some test code there?

This is going to sound really silly but I'm not sure how to time the code
without a clock. I guess grabserial?

> 
> Sascha

John.



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] I2C: i.MX: early: Use internal udelay
  2023-01-30 18:42               ` John Watts
@ 2023-01-31  6:14                 ` Sascha Hauer
  2023-01-31  6:33                   ` John Watts
  0 siblings, 1 reply; 16+ messages in thread
From: Sascha Hauer @ 2023-01-31  6:14 UTC (permalink / raw)
  To: John Watts; +Cc: barebox

On Tue, Jan 31, 2023 at 05:42:29AM +1100, John Watts wrote:
> On Mon, Jan 30, 2023 at 05:36:58PM +0100, Sascha Hauer wrote:
> > You can't disable MMU during runtime, but you can compile without MMU
> > support, just disable CONFIG_MMU. However, the early I2C code already
> > runs with MMU disabled, can't you just put in some test code there?
> 
> This is going to sound really silly but I'm not sure how to time the code
> without a clock. I guess grabserial?

You could udelay(10000000) and measure the time with a stopwatch.
That should be accurate enough for this purpose already, in the end
the time will differ anyway between SoCs.

If you want to measure more exactly we have this little perl script
which prints a timestamp for each line printed.

Sascha

------------------------8<----------------------

#! /usr/bin/perl
#
# ptx_ts - Pengutronix' Add A Time Stamp Filter V1
# written by Wolfram Sang, Copyright 2009 Pengutronix
# free software - no warranty - WTFPL V2, see http://sam.zoy.org/wtfpl/

use warnings;
use strict;
use Time::HiRes qw(gettimeofday tv_interval);

my $arg = defined($ARGV[0]) ? $ARGV[0] : '(?=foo)bar'; # false-branch is a regexp that never matches
if ($arg eq '--help') {
        print "ptx_ts [regexp] - a filter which prepends a timestamp to every line of STDOUT; time will be reset if [regexp] matches\n";
        print "  Example: microcom <microcom_options> | ptx_ts 'U-Boot 2.0'\n";
        exit 0;
}

my $old;
my $base;
$| = 1; # Flush output immediately

sub reset_time {
        $old = 0;
        $base = [gettimeofday()];
}

reset_time;
while (<STDIN>) {
        reset_time if (/$arg/o);
        my $new = tv_interval($base);
        my $diff = $new - $old;
        printf("[%10.6f] <%10.6f> $_", $new, $diff);
        $old = $new;
}


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] I2C: i.MX: early: Use internal udelay
  2023-01-31  6:14                 ` Sascha Hauer
@ 2023-01-31  6:33                   ` John Watts
  0 siblings, 0 replies; 16+ messages in thread
From: John Watts @ 2023-01-31  6:33 UTC (permalink / raw)
  To: Sascha Hauer; +Cc: barebox

On Tue, Jan 31, 2023 at 07:14:40AM +0100, Sascha Hauer wrote:
> On Tue, Jan 31, 2023 at 05:42:29AM +1100, John Watts wrote:
> > On Mon, Jan 30, 2023 at 05:36:58PM +0100, Sascha Hauer wrote:
> > > You can't disable MMU during runtime, but you can compile without MMU
> > > support, just disable CONFIG_MMU. However, the early I2C code already
> > > runs with MMU disabled, can't you just put in some test code there?
> > 
> > This is going to sound really silly but I'm not sure how to time the code
> > without a clock. I guess grabserial?
> 
> You could udelay(10000000) and measure the time with a stopwatch.
> That should be accurate enough for this purpose already, in the end
> the time will differ anyway between SoCs.
> 
> If you want to measure more exactly we have this little perl script
> which prints a timestamp for each line printed.

Right I shall try this- though I do wonder if this is a lost cause if the bus
clock changes in future i.MX chips.

Unless this is specific to the I2C clock. But I was under the assumption the
registers aren't dependent on the clock.

> Sascha

John.



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] I2C: i.MX: early: Use internal udelay
  2023-01-26 18:56 [PATCH] I2C: i.MX: early: Use internal udelay John Watts
  2023-01-30 10:27 ` Sascha Hauer
@ 2023-02-01 17:50 ` Alexander Shiyan
  2023-02-01 18:12   ` Jookia
  2023-02-01 19:44   ` Ahmad Fatoum
  1 sibling, 2 replies; 16+ messages in thread
From: Alexander Shiyan @ 2023-02-01 17:50 UTC (permalink / raw)
  To: John Watts; +Cc: barebox

Hello!

Just a nitpick, udelay means milliseconds (udelay_(us)),
which isn't quite right in this case,
so I think we should use the "loops" value here (delay_loops() or so)).

Thanks!

чт, 26 янв. 2023 г. в 21:57, John Watts <contact@jookia.org>:
>
> udelay isn't provided in the PBL, so use our own definition.
>
> This avoids boards having to define udelay in their code.
>
> Signed-off-by: John Watts <contact@jookia.org>
> ---
>  drivers/i2c/busses/i2c-imx-early.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/busses/i2c-imx-early.c b/drivers/i2c/busses/i2c-imx-early.c
> index 6c8bdc7904..fcf279eff8 100644
> --- a/drivers/i2c/busses/i2c-imx-early.c
> +++ b/drivers/i2c/busses/i2c-imx-early.c
> @@ -90,6 +90,13 @@ static int i2c_fsl_acked(struct fsl_i2c *fsl_i2c)
>         return i2c_fsl_poll_status(fsl_i2c, 0, I2SR_RXAK);
>  }
>
> +static void __udelay(int us)
> +{
> +       volatile int i;
> +
> +       for (i = 0; i < us * 1000; i++);
> +}
> +
>  static int i2c_fsl_start(struct fsl_i2c *fsl_i2c)
>  {
>         unsigned int temp = 0;
> @@ -104,7 +111,7 @@ static int i2c_fsl_start(struct fsl_i2c *fsl_i2c)
>                           fsl_i2c, FSL_I2C_I2CR);
>
>         /* Wait controller to be stable */
> -       udelay(100);
> +       __udelay(100);
>
>         /* Start I2C transaction */
>         temp = fsl_i2c_read_reg(fsl_i2c, FSL_I2C_I2CR);
> --
> 2.39.1
>
>



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] I2C: i.MX: early: Use internal udelay
  2023-02-01 17:50 ` Alexander Shiyan
@ 2023-02-01 18:12   ` Jookia
  2023-02-01 19:44   ` Ahmad Fatoum
  1 sibling, 0 replies; 16+ messages in thread
From: Jookia @ 2023-02-01 18:12 UTC (permalink / raw)
  To: Alexander Shiyan; +Cc: barebox

On Wed, Feb 01, 2023 at 08:50:50PM +0300, Alexander Shiyan wrote:
> Hello!
> 
> Just a nitpick, udelay means milliseconds (udelay_(us)),
> which isn't quite right in this case,
> so I think we should use the "loops" value here (delay_loops() or so)).
> 
> Thanks!

Hi Alexander,

Thanks for the review!

Are you sure? us means microseconds from what I understand.
I've also posted an updated version of this patch just last night.

John.



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] I2C: i.MX: early: Use internal udelay
  2023-02-01 17:50 ` Alexander Shiyan
  2023-02-01 18:12   ` Jookia
@ 2023-02-01 19:44   ` Ahmad Fatoum
  1 sibling, 0 replies; 16+ messages in thread
From: Ahmad Fatoum @ 2023-02-01 19:44 UTC (permalink / raw)
  To: Alexander Shiyan, John Watts; +Cc: barebox

Hello Alexander,

On 01.02.23 18:50, Alexander Shiyan wrote:
> Hello!
> 
> Just a nitpick, udelay means milliseconds (udelay_(us)),
> which isn't quite right in this case,
> so I think we should use the "loops" value here (delay_loops() or so)).

udelay is already defined used ARMv8 architected timer, so what John is
doingvis providing an implementation for i.MX6. So even if it uses a loop,
the iterations are chosen to be roughly the amount of microseconds
in the argument and thus the name is ok IMO. See his latest revision
of the patch.

Cheers,
Ahmad

> 
> Thanks!
> 
> чт, 26 янв. 2023 г. в 21:57, John Watts <contact@jookia.org>:
>>
>> udelay isn't provided in the PBL, so use our own definition.
>>
>> This avoids boards having to define udelay in their code.
>>
>> Signed-off-by: John Watts <contact@jookia.org>
>> ---
>>  drivers/i2c/busses/i2c-imx-early.c | 9 ++++++++-
>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-imx-early.c b/drivers/i2c/busses/i2c-imx-early.c
>> index 6c8bdc7904..fcf279eff8 100644
>> --- a/drivers/i2c/busses/i2c-imx-early.c
>> +++ b/drivers/i2c/busses/i2c-imx-early.c
>> @@ -90,6 +90,13 @@ static int i2c_fsl_acked(struct fsl_i2c *fsl_i2c)
>>         return i2c_fsl_poll_status(fsl_i2c, 0, I2SR_RXAK);
>>  }
>>
>> +static void __udelay(int us)
>> +{
>> +       volatile int i;
>> +
>> +       for (i = 0; i < us * 1000; i++);
>> +}
>> +
>>  static int i2c_fsl_start(struct fsl_i2c *fsl_i2c)
>>  {
>>         unsigned int temp = 0;
>> @@ -104,7 +111,7 @@ static int i2c_fsl_start(struct fsl_i2c *fsl_i2c)
>>                           fsl_i2c, FSL_I2C_I2CR);
>>
>>         /* Wait controller to be stable */
>> -       udelay(100);
>> +       __udelay(100);
>>
>>         /* Start I2C transaction */
>>         temp = fsl_i2c_read_reg(fsl_i2c, FSL_I2C_I2CR);
>> --
>> 2.39.1
>>
>>
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |




^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] I2C: i.MX: early: Use internal udelay
  2023-01-30 10:27 ` Sascha Hauer
  2023-01-30 10:42   ` John Watts
@ 2023-02-02 14:21   ` Jules Maselbas
  2023-02-02 14:27     ` Ahmad Fatoum
  1 sibling, 1 reply; 16+ messages in thread
From: Jules Maselbas @ 2023-02-02 14:21 UTC (permalink / raw)
  To: Sascha Hauer; +Cc: John Watts, barebox

Hi John and Sascha,

On Mon, Jan 30, 2023 at 11:27:27AM +0100, Sascha Hauer wrote:
> On Fri, Jan 27, 2023 at 05:56:43AM +1100, John Watts wrote:
> > udelay isn't provided in the PBL, so use our own definition.
> > 
> > This avoids boards having to define udelay in their code.
> > 
> > Signed-off-by: John Watts <contact@jookia.org>
> > ---
> >  drivers/i2c/busses/i2c-imx-early.c | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-imx-early.c b/drivers/i2c/busses/i2c-imx-early.c
> > index 6c8bdc7904..fcf279eff8 100644
> > --- a/drivers/i2c/busses/i2c-imx-early.c
> > +++ b/drivers/i2c/busses/i2c-imx-early.c
> > @@ -90,6 +90,13 @@ static int i2c_fsl_acked(struct fsl_i2c *fsl_i2c)
> >  	return i2c_fsl_poll_status(fsl_i2c, 0, I2SR_RXAK);
> >  }
> >  
> > +static void __udelay(int us)
> > +{
> > +	volatile int i;
> > +
> > +	for (i = 0; i < us * 1000; i++);
> > +}
> 
> This takes around 5 times too long on a i.MX8MM and around 50 times too
> long on a i.MX6Q. This was measured under a regular barebox on the
> shell. In an early environment with MMU disabled it takes 730 times too
> long.
> 
> Maybe we could do this:
> 
> static void __udelay(void *base, int us)
> {
> 	int i;
> 
> 	for (i = 0; i < us * 4; i++)
> 		readb(base);
> }
> 
> The time spent for a register read depends on the bus clock which
> doesn't change that much between the different SoCs.
> 

Some arm devices have an architecture timer, isn't it possible to use
the udelay defined in arch/arm/lib64/pbl.c on i.MX ? I am not very
experienced on ARM cpus, is this only possible on armv7/armv8, and not
on every i.MX SoCs ?

Cheers,
-- Jules







^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] I2C: i.MX: early: Use internal udelay
  2023-02-02 14:21   ` Jules Maselbas
@ 2023-02-02 14:27     ` Ahmad Fatoum
  0 siblings, 0 replies; 16+ messages in thread
From: Ahmad Fatoum @ 2023-02-02 14:27 UTC (permalink / raw)
  To: Jules Maselbas, Sascha Hauer; +Cc: John Watts, barebox

Hello Jules,

On 02.02.23 15:21, Jules Maselbas wrote:
> On Mon, Jan 30, 2023 at 11:27:27AM +0100, Sascha Hauer wrote:
>> The time spent for a register read depends on the bus clock which
>> doesn't change that much between the different SoCs.
>>
> 
> Some arm devices have an architecture timer, isn't it possible to use
> the udelay defined in arch/arm/lib64/pbl.c on i.MX ? I am not very
> experienced on ARM cpus, is this only possible on armv7/armv8, and not
> on every i.MX SoCs ?

Cortex-A7 and later i.MX has an ARMv7 architected timer, but "normal"
i.MX6Q/DL with Cortex-A9 doesn't.

Cheers,
Ahmad

> 
> Cheers,
> -- Jules
> 
> 
> 
> 
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |




^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2023-02-02 14:28 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-26 18:56 [PATCH] I2C: i.MX: early: Use internal udelay John Watts
2023-01-30 10:27 ` Sascha Hauer
2023-01-30 10:42   ` John Watts
2023-01-30 12:17     ` Sascha Hauer
2023-01-30 12:24       ` John Watts
2023-01-30 12:31         ` Sascha Hauer
2023-01-30 12:56           ` John Watts
2023-01-30 16:36             ` Sascha Hauer
2023-01-30 18:42               ` John Watts
2023-01-31  6:14                 ` Sascha Hauer
2023-01-31  6:33                   ` John Watts
2023-02-02 14:21   ` Jules Maselbas
2023-02-02 14:27     ` Ahmad Fatoum
2023-02-01 17:50 ` Alexander Shiyan
2023-02-01 18:12   ` Jookia
2023-02-01 19:44   ` Ahmad Fatoum

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox