[U-Boot] [PATCH 0/3] Three mpc5200 patches from Pengutronix.de
Three generic mpc5200 patches from Pengutronix.de that missed being submitted to mainline.
---
Sascha Hauer (3): mpc5200: make i2c faster mpc52xx phy: initialize only when needed mpc5200: do not use printf in i2c_init()
cpu/mpc5xxx/i2c.c | 16 +++------------- drivers/net/mpc5xxx_fec.c | 11 +++++++++-- 2 files changed, 12 insertions(+), 15 deletions(-)
From: Sascha Hauer s.hauer@pengutronix.de
On boards which have the environment in eeprom, i2c_init() is called before the console and ram is initialized. Do not printf here.
Signed-off-by: Sascha Hauer s.hauer@pengutronix.de --- cpu/mpc5xxx/i2c.c | 10 ---------- 1 files changed, 0 insertions(+), 10 deletions(-)
diff --git a/cpu/mpc5xxx/i2c.c b/cpu/mpc5xxx/i2c.c index 7d76274..843af9c 100644 --- a/cpu/mpc5xxx/i2c.c +++ b/cpu/mpc5xxx/i2c.c @@ -269,7 +269,6 @@ static int mpc_get_fdr(int speed) if (gd->flags & GD_FLG_RELOC) { fdr = divider; } else { - printf("%ld kHz, ", best_speed / 1000); return divider; } } @@ -310,29 +309,24 @@ int i2c_read(uchar chip, uint addr, int alen, uchar *buf, int len) xaddr[3] = addr & 0xFF;
if (wait_for_bb()) { - printf("i2c_read: bus is busy\n"); goto Done; }
mpc_reg_out(®s->mcr, I2C_STA, I2C_STA); if (do_address(chip, 0)) { - printf("i2c_read: failed to address chip\n"); goto Done; }
if (send_bytes(chip, &xaddr[4-alen], alen)) { - printf("i2c_read: send_bytes failed\n"); goto Done; }
mpc_reg_out(®s->mcr, I2C_RSTA, I2C_RSTA); if (do_address(chip, 1)) { - printf("i2c_read: failed to address chip\n"); goto Done; }
if (receive_bytes(chip, (char *)buf, len)) { - printf("i2c_read: receive_bytes failed\n"); goto Done; }
@@ -354,23 +348,19 @@ int i2c_write(uchar chip, uint addr, int alen, uchar *buf, int len) xaddr[3] = addr & 0xFF;
if (wait_for_bb()) { - printf("i2c_write: bus is busy\n"); goto Done; }
mpc_reg_out(®s->mcr, I2C_STA, I2C_STA); if (do_address(chip, 0)) { - printf("i2c_write: failed to address chip\n"); goto Done; }
if (send_bytes(chip, &xaddr[4-alen], alen)) { - printf("i2c_write: send_bytes failed\n"); goto Done; }
if (send_bytes(chip, (char *)buf, len)) { - printf("i2c_write: send_bytes failed\n"); goto Done; }
Dear Jon,
In message 20090321133844.11905.61201.stgit@localhost you wrote:
From: Sascha Hauer s.hauer@pengutronix.de
On boards which have the environment in eeprom, i2c_init() is called before the console and ram is initialized. Do not printf here.
Signed-off-by: Sascha Hauer s.hauer@pengutronix.de
cpu/mpc5xxx/i2c.c | 10 ---------- 1 files changed, 0 insertions(+), 10 deletions(-)
Rather than doing this unconsitionally, this change should only be implemented on boards which actually hold the envrionment in I2C EEPROM (alternatively, one should consider fixing these boards to use more appropriate storage; in addition to being not reliable, I2C EEPROM is slow - see http://www.denx.de/wiki/DULG/AN2004_11_BootTimeOptimization )
Best regards,
Wolfgang Denk
On Sat, Mar 21, 2009 at 3:47 PM, Wolfgang Denk wd@denx.de wrote:
Dear Jon,
In message 20090321133844.11905.61201.stgit@localhost you wrote:
From: Sascha Hauer s.hauer@pengutronix.de
On boards which have the environment in eeprom, i2c_init() is called before the console and ram is initialized. Do not printf here.
Signed-off-by: Sascha Hauer s.hauer@pengutronix.de
cpu/mpc5xxx/i2c.c | 10 ---------- 1 files changed, 0 insertions(+), 10 deletions(-)
Rather than doing this unconditionally, this change should only be implemented on boards which actually hold the envrionment in I2C EEPROM (alternatively, one should consider fixing these boards to use more appropriate storage; in addition to being not reliable, I2C EEPROM is slow - see http://www.denx.de/wiki/DULG/AN2004_11_BootTimeOptimization )
How about this instead? Is there a variable that indicates when the console is safe to use, if so I can adjust the macro.
From: Jon Smirl jonsmirl@gmail.com
On boards which have the environment in eeprom, i2c_init() is called before the console and ram is initialized. Do not printf here.
Signed-off-by: Jon Smirl jonsmirl@gmail.com --- cpu/mpc5xxx/i2c.c | 7 +++++++ 1 files changed, 7 insertions(+), 0 deletions(-)
diff --git a/cpu/mpc5xxx/i2c.c b/cpu/mpc5xxx/i2c.c index 7d76274..40f1682 100644 --- a/cpu/mpc5xxx/i2c.c +++ b/cpu/mpc5xxx/i2c.c @@ -41,6 +41,13 @@ DECLARE_GLOBAL_DATA_PTR; #define I2C_TIMEOUT 100 #define I2C_RETRIES 3
+#ifdef CONFIG_ENV_IS_IN_EEPROM +/* On boards which have the environment in eeprom, i2c_init() + * is called before the console and ram is initialized. Do + * not printf here. */ +#define printf(format, arg...) do {} while (0) +#endif + struct mpc5xxx_i2c_tap { int scl2tap; int tap2tap;
Dear Jon Smirl,
In message 9e4733910903211339u60f5531cp7a9574f2d1ba264f@mail.gmail.com you wrote:
How about this instead? Is there a variable that indicates when the console is safe to use, if so I can adjust the macro.
gd->have_console
Best regards,
Wolfgang Denk
Dear Jon Smirl,
In message 9e4733910903211339u60f5531cp7a9574f2d1ba264f@mail.gmail.com you wrote:
How about this instead? Is there a variable that indicates when the console is safe to use, if so I can adjust the macro.
BTW: you might want to compare with cpu/mpc8xx/i2c.c or cpu/ppc4xx/i2c.c
Best regards,
Wolfgang Denk
On Sat, Mar 21, 2009 at 4:56 PM, Wolfgang Denk wd@denx.de wrote:
Dear Jon Smirl,
In message 9e4733910903211339u60f5531cp7a9574f2d1ba264f@mail.gmail.com you wrote:
How about this instead? Is there a variable that indicates when the console is safe to use, if so I can adjust the macro.
BTW: you might want to compare with cpu/mpc8xx/i2c.c or cpu/ppc4xx/i2c.c
I've test this version with the eeprom and it works correctly.
----------------------------------------------- mpc5200 with eeprom, suppress printf until console initialized
From: Jon Smirl jonsmirl@gmail.com
On boards which have the environment in eeprom, i2c_init() is called before the console and ram is initialized. Suppress printfs until the console is initialized.
Signed-off-by: Jon Smirl jonsmirl@gmail.com --- cpu/mpc5xxx/i2c.c | 7 +++++++ 1 files changed, 7 insertions(+), 0 deletions(-)
diff --git a/cpu/mpc5xxx/i2c.c b/cpu/mpc5xxx/i2c.c index 7d76274..a5478b2 100644 --- a/cpu/mpc5xxx/i2c.c +++ b/cpu/mpc5xxx/i2c.c @@ -41,6 +41,13 @@ DECLARE_GLOBAL_DATA_PTR; #define I2C_TIMEOUT 100 #define I2C_RETRIES 3
+#ifdef CONFIG_ENV_IS_IN_EEPROM +/* On boards which have the environment in eeprom, i2c_init() + * is called before the console and ram is initialized. Suppress + * printf until the console is initialized. */ +#define printf(format, arg...) if (gd->have_console) printf(format, ## arg) +#endif + struct mpc5xxx_i2c_tap { int scl2tap; int tap2tap;
Dear Jon,
In message 9e4733910903220701h104932cdu6b226e5c05e9b3d4@mail.gmail.com you wrote:
I've test this version with the eeprom and it works correctly.
It may work, but such a macro definition violates some rules.
+#define printf(format, arg...) if (gd->have_console) printf(format, ## arg)
There are only a few places in cpu/mpc5xxx/i2c.c where printf() is used that we should explicitly code this, there.
Thanks.
Best regards,
Wolfgang Denk
On Sun, Mar 22, 2009 at 10:21 AM, Wolfgang Denk wd@denx.de wrote:
Dear Jon,
In message 9e4733910903220701h104932cdu6b226e5c05e9b3d4@mail.gmail.com you wrote:
I've test this version with the eeprom and it works correctly.
It may work, but such a macro definition violates some rules.
+#define printf(format, arg...) if (gd->have_console) printf(format, ## arg)
There are only a few places in cpu/mpc5xxx/i2c.c where printf() is used that we should explicitly code this, there.
Would it be better to put the check for (gd->have_console) into printf itself?
Dear Jon Smirl,
In message 9e4733910903220723u31546286q233c9b24b7a5d53@mail.gmail.com you wrote:
Would it be better to put the check for (gd->have_console) into printf itself?
No, as you could also use puts() or putc(). There are only a few isolated places where we need this, so let's make this clearly visible there.
Best regards,
Wolfgang Denk
On Sun, Mar 22, 2009 at 11:23 AM, Wolfgang Denk wd@denx.de wrote:
Dear Jon Smirl,
In message 9e4733910903220723u31546286q233c9b24b7a5d53@mail.gmail.com you wrote:
Would it be better to put the check for (gd->have_console) into printf itself?
No, as you could also use puts() or putc(). There are only a few isolated places where we need this, so let's make this clearly visible there.
mpc5200 with eeprom, suppress printf until console initialized
From: Jon Smirl jonsmirl@gmail.com
On boards which have the environment in eeprom, i2c_init() is called before the console and ram is initialized. Suppress printfs until the console is initialized.
Signed-off-by: Jon Smirl jonsmirl@gmail.com --- cpu/mpc5xxx/i2c.c | 30 ++++++++++++++++++++---------- 1 files changed, 20 insertions(+), 10 deletions(-)
diff --git a/cpu/mpc5xxx/i2c.c b/cpu/mpc5xxx/i2c.c index 7d76274..0860ecf 100644 --- a/cpu/mpc5xxx/i2c.c +++ b/cpu/mpc5xxx/i2c.c @@ -269,7 +269,8 @@ static int mpc_get_fdr(int speed) if (gd->flags & GD_FLG_RELOC) { fdr = divider; } else { - printf("%ld kHz, ", best_speed / 1000); + if (gd->have_console) + printf("%ld kHz, ", best_speed / 1000); return divider; } } @@ -310,29 +311,34 @@ int i2c_read(uchar chip, uint addr, int alen, uchar *buf, int len) xaddr[3] = addr & 0xFF;
if (wait_for_bb()) { - printf("i2c_read: bus is busy\n"); + if (gd->have_console) + printf("i2c_read: bus is busy\n"); goto Done; }
mpc_reg_out(®s->mcr, I2C_STA, I2C_STA); if (do_address(chip, 0)) { - printf("i2c_read: failed to address chip\n"); + if (gd->have_console) + printf("i2c_read: failed to address chip\n"); goto Done; }
if (send_bytes(chip, &xaddr[4-alen], alen)) { - printf("i2c_read: send_bytes failed\n"); + if (gd->have_console) + printf("i2c_read: send_bytes failed\n"); goto Done; }
mpc_reg_out(®s->mcr, I2C_RSTA, I2C_RSTA); if (do_address(chip, 1)) { - printf("i2c_read: failed to address chip\n"); + if (gd->have_console) + printf("i2c_read: failed to address chip\n"); goto Done; }
if (receive_bytes(chip, (char *)buf, len)) { - printf("i2c_read: receive_bytes failed\n"); + if (gd->have_console) + printf("i2c_read: receive_bytes failed\n"); goto Done; }
@@ -354,23 +360,27 @@ int i2c_write(uchar chip, uint addr, int alen, uchar *buf, int len) xaddr[3] = addr & 0xFF;
if (wait_for_bb()) { - printf("i2c_write: bus is busy\n"); + if (gd->have_console) + printf("i2c_write: bus is busy\n"); goto Done; }
mpc_reg_out(®s->mcr, I2C_STA, I2C_STA); if (do_address(chip, 0)) { - printf("i2c_write: failed to address chip\n"); + if (gd->have_console) + printf("i2c_write: failed to address chip\n"); goto Done; }
if (send_bytes(chip, &xaddr[4-alen], alen)) { - printf("i2c_write: send_bytes failed\n"); + if (gd->have_console) + printf("i2c_write: send_bytes failed\n"); goto Done; }
if (send_bytes(chip, (char *)buf, len)) { - printf("i2c_write: send_bytes failed\n"); + if (gd->have_console) + printf("i2c_write: send_bytes failed\n"); goto Done; }
Hello Jon,
Jon Smirl wrote:
On Sun, Mar 22, 2009 at 11:23 AM, Wolfgang Denk wd@denx.de wrote:
Dear Jon Smirl,
In message 9e4733910903220723u31546286q233c9b24b7a5d53@mail.gmail.com you wrote:
Would it be better to put the check for (gd->have_console) into printf itself?
No, as you could also use puts() or putc(). There are only a few isolated places where we need this, so let's make this clearly visible there.
mpc5200 with eeprom, suppress printf until console initialized
From: Jon Smirl jonsmirl@gmail.com
On boards which have the environment in eeprom, i2c_init() is called before the console and ram is initialized. Suppress printfs until the console is initialized.
Signed-off-by: Jon Smirl jonsmirl@gmail.com
cpu/mpc5xxx/i2c.c | 30 ++++++++++++++++++++---------- 1 files changed, 20 insertions(+), 10 deletions(-)
Acked-by: Heiko Schocher hs@denx.de
bye Heiko
Dear Jon Smirl,
In message 9e4733910903221955h4a6b3c24t34cce6989911be21@mail.gmail.com you wrote:
On Sun, Mar 22, 2009 at 11:23 AM, Wolfgang Denk wd@denx.de wrote:
Dear Jon Smirl,
In message 9e4733910903220723u31546286q233c9b24b7a5d53@mail.gmail.com y=
ou wrote:
Would it be better to put the check for (gd->have_console) =A0into print=
f itself?
No, as you could also use puts() or putc(). =A0There =A0are =A0only =A0a =
=A0few
isolated =A0places =A0where =A0we =A0need =A0this, =A0so =A0let's make th=
is clearly
visible there.
mpc5200 with eeprom, suppress printf until console initialized
From: Jon Smirl jonsmirl@gmail.com
Argh.... Such comments have to go *below* the '---' line, otherwise they become part of the commit message.
And your patch is line-wrapped and thus corrupted by your mailer.
Please FIX your mailer (or even better use git-send-email to submit patches).
On boards which have the environment in eeprom, i2c_init() is called before the console and ram is initialized. Suppress printfs until the console is initialized.
Signed-off-by: Jon Smirl jonsmirl@gmail.com
cpu/mpc5xxx/i2c.c | 30 ++++++++++++++++++++---------- 1 files changed, 20 insertions(+), 10 deletions(-)
Fixed manually and applied. Thanks.
Best regards,
Wolfgang Denk
On Wed, Mar 25, 2009 at 5:51 PM, Wolfgang Denk wd@denx.de wrote:
Fixed manually and applied. Thanks.
I used stgit to smtp.gmail.com::587 for the initial patch.
I wasn't thinking and used cut/paste to paste the last patch into a reply. These patches are at git.digispeaker.com if that is easier.
From: Sascha Hauer s.hauer@pengutronix.de
Do not initialize phy on startup, instead initialize it when we actually need it.
Signed-off-by: Sascha Hauer s.hauer@pengutronix.de --- drivers/net/mpc5xxx_fec.c | 11 +++++++++-- 1 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/net/mpc5xxx_fec.c b/drivers/net/mpc5xxx_fec.c index 0f1d1af..1876b76 100644 --- a/drivers/net/mpc5xxx_fec.c +++ b/drivers/net/mpc5xxx_fec.c @@ -42,6 +42,8 @@ typedef struct { int fec5xxx_miiphy_read(char *devname, uint8 phyAddr, uint8 regAddr, uint16 * retVal); int fec5xxx_miiphy_write(char *devname, uint8 phyAddr, uint8 regAddr, uint16 data);
+static int mpc5xxx_fec_init_phy(struct eth_device *dev, bd_t * bis); + /********************************************************************/ #if (DEBUG & 0x2) static void mpc5xxx_fec_phydump (char *devname) @@ -249,6 +251,8 @@ static int mpc5xxx_fec_init(struct eth_device *dev, bd_t * bis) printf ("mpc5xxx_fec_init... Begin\n"); #endif
+ mpc5xxx_fec_init_phy(dev, bis); + /* * Initialize RxBD/TxBD rings */ @@ -387,6 +391,11 @@ static int mpc5xxx_fec_init_phy(struct eth_device *dev, bd_t * bis) { mpc5xxx_fec_priv *fec = (mpc5xxx_fec_priv *)dev->priv; const uint8 phyAddr = CONFIG_PHY_ADDR; /* Only one PHY */ + static int initialized = 0; + + if(initialized) + return 0; + initialized = 1;
#if (DEBUG & 0x1) printf ("mpc5xxx_fec_init_phy... Begin\n"); @@ -937,8 +946,6 @@ int mpc5xxx_fec_initialize(bd_t * bis) mpc5xxx_fec_set_hwaddr(fec, env_enetaddr); }
- mpc5xxx_fec_init_phy(dev, bis); - return 1; }
Is this patch ok for inclusion?
On Sat, Mar 21, 2009 at 9:38 AM, Jon jonsmirl@gmail.com wrote:
From: Sascha Hauer s.hauer@pengutronix.de
Do not initialize phy on startup, instead initialize it when we actually need it.
Signed-off-by: Sascha Hauer s.hauer@pengutronix.de
drivers/net/mpc5xxx_fec.c | 11 +++++++++-- 1 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/net/mpc5xxx_fec.c b/drivers/net/mpc5xxx_fec.c index 0f1d1af..1876b76 100644 --- a/drivers/net/mpc5xxx_fec.c +++ b/drivers/net/mpc5xxx_fec.c @@ -42,6 +42,8 @@ typedef struct { int fec5xxx_miiphy_read(char *devname, uint8 phyAddr, uint8 regAddr, uint16 * retVal); int fec5xxx_miiphy_write(char *devname, uint8 phyAddr, uint8 regAddr, uint16 data);
+static int mpc5xxx_fec_init_phy(struct eth_device *dev, bd_t * bis);
/********************************************************************/ #if (DEBUG & 0x2) static void mpc5xxx_fec_phydump (char *devname) @@ -249,6 +251,8 @@ static int mpc5xxx_fec_init(struct eth_device *dev, bd_t * bis) printf ("mpc5xxx_fec_init... Begin\n"); #endif
- mpc5xxx_fec_init_phy(dev, bis);
/* * Initialize RxBD/TxBD rings */ @@ -387,6 +391,11 @@ static int mpc5xxx_fec_init_phy(struct eth_device *dev, bd_t * bis) { mpc5xxx_fec_priv *fec = (mpc5xxx_fec_priv *)dev->priv; const uint8 phyAddr = CONFIG_PHY_ADDR; /* Only one PHY */
- static int initialized = 0;
- if(initialized)
- return 0;
- initialized = 1;
#if (DEBUG & 0x1) printf ("mpc5xxx_fec_init_phy... Begin\n"); @@ -937,8 +946,6 @@ int mpc5xxx_fec_initialize(bd_t * bis) mpc5xxx_fec_set_hwaddr(fec, env_enetaddr); }
- mpc5xxx_fec_init_phy(dev, bis);
return 1; }
Dear Jon Smirl,
In message 9e4733910903251212j4436f1afu817456c7f6e9c970@mail.gmail.com you wrote:
Is this patch ok for inclusion?
On Sat, Mar 21, 2009 at 9:38 AM, Jon jonsmirl@gmail.com wrote:
From: Sascha Hauer s.hauer@pengutronix.de
Do not initialize phy on startup, instead initialize it when we actually need it.
Signed-off-by: Sascha Hauer s.hauer@pengutronix.de
drivers/net/mpc5xxx_fec.c | 11 +++++++++-- 1 files changed, 9 insertions(+), 2 deletions(-)
This is more for Ben to review, I think.
Best regards,
Wolfgang Denk
On Wed, Mar 25, 2009 at 5:53 PM, Wolfgang Denk wd@denx.de wrote:
Dear Jon Smirl,
In message 9e4733910903251212j4436f1afu817456c7f6e9c970@mail.gmail.com you wrote:
Is this patch ok for inclusion?
Ping? Any answer on this one?
On Sat, Mar 21, 2009 at 9:38 AM, Jon jonsmirl@gmail.com wrote:
From: Sascha Hauer s.hauer@pengutronix.de
Do not initialize phy on startup, instead initialize it when we actually need it.
Signed-off-by: Sascha Hauer s.hauer@pengutronix.de
drivers/net/mpc5xxx_fec.c | 11 +++++++++-- 1 files changed, 9 insertions(+), 2 deletions(-)
This is more for Ben to review, I think.
Best regards,
Wolfgang Denk
-- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de Intuition, however illogical, is recognized as a command prerogative. -- Kirk, "Obsession", stardate 3620.7
On Mon, Mar 30, 2009 at 6:04 PM, Jon Smirl jonsmirl@gmail.com wrote:
On Wed, Mar 25, 2009 at 5:53 PM, Wolfgang Denk wd@denx.de wrote:
Dear Jon Smirl,
In message 9e4733910903251212j4436f1afu817456c7f6e9c970@mail.gmail.com
you wrote:
Is this patch ok for inclusion?
Ping? Any answer on this one?
Sorry, I have some catching up to do. Will respond within the next couple of days.
regards, Ben
Dear Jon,
In message 20090321133846.11905.81764.stgit@localhost you wrote:
From: Sascha Hauer s.hauer@pengutronix.de
Do not initialize phy on startup, instead initialize it when we actually need it.
Signed-off-by: Sascha Hauer s.hauer@pengutronix.de
drivers/net/mpc5xxx_fec.c | 11 +++++++++-- 1 files changed, 9 insertions(+), 2 deletions(-)
Applied, thanks.
Best regards,
Wolfgang Denk
From: Sascha Hauer s.hauer@pengutronix.de
The mpc5xxx i2c driver has great delays while waiting for the chip status. make the delays smaller and the driver faster.
Signed-off-by: Sascha Hauer s.hauer@pengutronix.de --- cpu/mpc5xxx/i2c.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/cpu/mpc5xxx/i2c.c b/cpu/mpc5xxx/i2c.c index 843af9c..41feb1d 100644 --- a/cpu/mpc5xxx/i2c.c +++ b/cpu/mpc5xxx/i2c.c @@ -38,7 +38,7 @@ DECLARE_GLOBAL_DATA_PTR; #error CONFIG_SYS_I2C_MODULE is not properly configured #endif
-#define I2C_TIMEOUT 100 +#define I2C_TIMEOUT 10000 #define I2C_RETRIES 3
struct mpc5xxx_i2c_tap { @@ -94,7 +94,7 @@ static int wait_for_bb(void) mpc_reg_out(®s->mcr, 0, 0); mpc_reg_out(®s->mcr, I2C_EN, 0); #endif - udelay(1000); + udelay(1); status = mpc_reg_in(®s->msr); }
@@ -109,7 +109,7 @@ static int wait_for_pin(int *status) *status = mpc_reg_in(®s->msr);
while (timeout-- && !(*status & I2C_IF)) { - udelay(1000); + udelay(1); *status = mpc_reg_in(®s->msr); }
Dear Jon,
In message 20090321133848.11905.59350.stgit@localhost you wrote:
From: Sascha Hauer s.hauer@pengutronix.de
The mpc5xxx i2c driver has great delays while waiting for the chip status. make the delays smaller and the driver faster.
Signed-off-by: Sascha Hauer s.hauer@pengutronix.de
cpu/mpc5xxx/i2c.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)
Are you absolutely sure that reducing these delays from 1 millisecond to 1 microsecond will work on all boards with all currently supported devices?
And how much do we make I2C faster this way? Where do we save time, and how much?
Best regards,
Wolfgang Denk
On Sat, Mar 21, 2009 at 3:48 PM, Wolfgang Denk wd@denx.de wrote:
Dear Jon,
In message 20090321133848.11905.59350.stgit@localhost you wrote:
From: Sascha Hauer s.hauer@pengutronix.de
The mpc5xxx i2c driver has great delays while waiting for the chip status. make the delays smaller and the driver faster.
Signed-off-by: Sascha Hauer s.hauer@pengutronix.de
cpu/mpc5xxx/i2c.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)
Are you absolutely sure that reducing these delays from 1 millisecond to 1 microsecond will work on all boards with all currently supported devices?
And how much do we make I2C faster this way? Where do we save time, and how much?
I measured the delay. It is between 40us and 52us on my hardware so I adjusted the loop to 60us. The previous 1ms delay was 20x bigger than necessary. If the hardware is slower, it will just loop and add more delay.
----------------------------------------------------------------------
mpc5200: reduce delays in i2c
The mpc5xxx i2c driver has great delays while waiting for the chip status. Make the delays smaller and the driver faster. The measured delay is 55us at 100Khz. Set the delay to 15us which should work for 400Khz. 100Khz will loop four times and get a 60us delay.
Signed-off-by: Jon Smirl jonsmirl@gmail.com --- cpu/mpc5xxx/i2c.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/cpu/mpc5xxx/i2c.c b/cpu/mpc5xxx/i2c.c index a5478b2..dcb184c 100644 --- a/cpu/mpc5xxx/i2c.c +++ b/cpu/mpc5xxx/i2c.c @@ -38,7 +38,7 @@ DECLARE_GLOBAL_DATA_PTR; #error CONFIG_SYS_I2C_MODULE is not properly configured #endif
-#define I2C_TIMEOUT 100 +#define I2C_TIMEOUT 1000 #define I2C_RETRIES 3
#ifdef CONFIG_ENV_IS_IN_EEPROM @@ -101,7 +101,7 @@ static int wait_for_bb(void) mpc_reg_out(®s->mcr, 0, 0); mpc_reg_out(®s->mcr, I2C_EN, 0); #endif - udelay(1000); + udelay(15); status = mpc_reg_in(®s->msr); }
@@ -116,7 +116,7 @@ static int wait_for_pin(int *status) *status = mpc_reg_in(®s->msr);
while (timeout-- && !(*status & I2C_IF)) { - udelay(1000); + udelay(15); *status = mpc_reg_in(®s->msr); }
Best regards,
Wolfgang Denk
-- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de It's certainly convenient the way the crime (or condition) of stupidity carries with it its own punishment, automatically admisistered without remorse, pity, or prejudice. :-) -- Tom Christiansen in 559seq$ag1$1@csnews.cs.colorado.edu
Dear Jon,
In message 9e4733910903220720l723de65fm59a7e6a7fed1eda7@mail.gmail.com you wrote:
And how much do we make I2C faster this way? Where do we save time, and how much?
I measured the delay. It is between 40us and 52us on my hardware so I adjusted the loop to 60us. The previous 1ms delay was 20x bigger than necessary. If the hardware is slower, it will just loop and add more delay.
You did not answer my question in which way this actually makes I2C faster? Where do we save time, and how much?
Which operation of I2C on the board gets acelerated, and how much?
The mpc5xxx i2c driver has great delays while waiting for the chip status. Make the delays smaller and the driver faster. The measured delay is 55us at 100Khz. Set the delay to 15us which should work for 400Khz. 100Khz will loop four times and get a 60us delay.
Signed-off-by: Jon Smirl jonsmirl@gmail.com
cpu/mpc5xxx/i2c.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/cpu/mpc5xxx/i2c.c b/cpu/mpc5xxx/i2c.c index a5478b2..dcb184c 100644 --- a/cpu/mpc5xxx/i2c.c +++ b/cpu/mpc5xxx/i2c.c @@ -38,7 +38,7 @@ DECLARE_GLOBAL_DATA_PTR; #error CONFIG_SYS_I2C_MODULE is not properly configured #endif
-#define I2C_TIMEOUT 100 +#define I2C_TIMEOUT 1000 #define I2C_RETRIES 3
#ifdef CONFIG_ENV_IS_IN_EEPROM @@ -101,7 +101,7 @@ static int wait_for_bb(void) mpc_reg_out(®s->mcr, 0, 0); mpc_reg_out(®s->mcr, I2C_EN, 0); #endif
udelay(1000);
udelay(15);
Hm.. You increase the I2C_TIMEOUT by a factor of 10, but reduce the per-loop delay by a factor of > 66; instead of the old total timeout of 100 * 1000 us = 100 milliseconds we now have a timeout of 1000 * 15 us = 15 milliseconds. That is much, much less.
Is this change intentional? If yes, are you sure it is correct with all existing boards and I2C devices out there?
@@ -116,7 +116,7 @@ static int wait_for_pin(int *status) *status = mpc_reg_in(®s->msr);
while (timeout-- && !(*status & I2C_IF)) {
udelay(1000);
udelay(15);
Ditto here.
Best regards,
Wolfgang Denk
On Sun, Mar 22, 2009 at 11:21 AM, Wolfgang Denk wd@denx.de wrote:
Dear Jon,
In message 9e4733910903220720l723de65fm59a7e6a7fed1eda7@mail.gmail.com you wrote:
And how much do we make I2C faster this way? Where do we save time, and how much?
I measured the delay. It is between 40us and 52us on my hardware so I adjusted the loop to 60us. The previous 1ms delay was 20x bigger than necessary. If the hardware is slower, it will just loop and add more delay.
You did not answer my question in which way this actually makes I2C faster? Where do we save time, and how much?
I increased the retry loop to 10,000. This definitely makes my system faster. On my bus the actual i2c delay is 40-55us. The original code always delayed 1,000us so for me it a gain of 940us on each i2c operation. This is visible during things like probe.
mpc5200: reduce delays in i2c
From: Sascha Hauer s.hauer@pengutronix.de
The mpc5xxx i2c driver has great delays while waiting for the chip status. make the delays smaller and the driver faster. The measured delay is 55us at 100Khz. Set the delay to 15us which should work for 400Khz. 100Khz will loop four times and get a 60us delay.
Signed-off-by: Sascha Hauer s.hauer@pengutronix.de --- cpu/mpc5xxx/i2c.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/cpu/mpc5xxx/i2c.c b/cpu/mpc5xxx/i2c.c index 0860ecf..58470d1 100644 --- a/cpu/mpc5xxx/i2c.c +++ b/cpu/mpc5xxx/i2c.c @@ -38,7 +38,7 @@ DECLARE_GLOBAL_DATA_PTR; #error CONFIG_SYS_I2C_MODULE is not properly configured #endif
-#define I2C_TIMEOUT 100 +#define I2C_TIMEOUT 10000 #define I2C_RETRIES 3
struct mpc5xxx_i2c_tap { @@ -94,7 +94,7 @@ static int wait_for_bb(void) mpc_reg_out(®s->mcr, 0, 0); mpc_reg_out(®s->mcr, I2C_EN, 0); #endif - udelay(1000); + udelay(15); status = mpc_reg_in(®s->msr); }
@@ -109,7 +109,7 @@ static int wait_for_pin(int *status) *status = mpc_reg_in(®s->msr);
while (timeout-- && !(*status & I2C_IF)) { - udelay(1000); + udelay(15); *status = mpc_reg_in(®s->msr); }
Dear Jon,
In message 9e4733910903221959n452a067kd46b313b9b453ec3@mail.gmail.com you wrote:
You did not answer my question in which way this actually makes I2C faster? Where do we save time, and how much?
I increased the retry loop to 10,000. This definitely makes my system faster. On my bus the actual i2c delay is 40-55us. The original code always delayed 1,000us so for me it a gain of 940us on each i2c operation. This is visible during things like probe.
Hm... you don't really convince me. Being "visible" is nice, but is it a reason to change the code? I mean, if you said something like: "without that patch operation FOO takes 20 seconds, and with the patch it takes less than 3 seconds", then everybody can understand why this is a good thing to do.
And: the I2C probe operation itself is just a debug thing, nothing you do in normal operation, so it doesn't look that time critical to me.
I agree that the change itself looks harmless, and I'm willing to accept it, but at least make sure not to reduce the total timeout values - that would require re-testing on some boards.
Best regards,
Wolfgang Denk
Hi,
On Sat, Mar 21, 2009 at 09:38:48AM -0400, Jon wrote:
From: Sascha Hauer s.hauer@pengutronix.de
The mpc5xxx i2c driver has great delays while waiting for the chip status. make the delays smaller and the driver faster.
Signed-off-by: Sascha Hauer s.hauer@pengutronix.de
cpu/mpc5xxx/i2c.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/cpu/mpc5xxx/i2c.c b/cpu/mpc5xxx/i2c.c index 843af9c..41feb1d 100644 --- a/cpu/mpc5xxx/i2c.c +++ b/cpu/mpc5xxx/i2c.c @@ -38,7 +38,7 @@ DECLARE_GLOBAL_DATA_PTR; #error CONFIG_SYS_I2C_MODULE is not properly configured #endif
-#define I2C_TIMEOUT 100 +#define I2C_TIMEOUT 10000 #define I2C_RETRIES 3
struct mpc5xxx_i2c_tap { @@ -94,7 +94,7 @@ static int wait_for_bb(void) mpc_reg_out(®s->mcr, 0, 0); mpc_reg_out(®s->mcr, I2C_EN, 0); #endif
udelay(1000);
udelay(1);
The actual condition we wait for takes only a few useconds to become true, but we wait for at least 1ms. As the I2C EEPROM environment routine reads nearly the complete EEPROM several times during startup the speed gain is enormous. I can't recall if it was intentional to reduce the overall timeout with this patch, but I can imagine that the i2c detect function will be faster aswell.
@@ -109,7 +109,7 @@ static int wait_for_pin(int *status) *status = mpc_reg_in(®s->msr);
while (timeout-- && !(*status & I2C_IF)) {
udelay(1000);
udelay(1);
Same here.
Sascha
On Sun, Mar 22, 2009 at 5:16 PM, Sascha Hauer s.hauer@pengutronix.de wrote:
On Sat, Mar 21, 2009 at 09:38:48AM -0400, Jon wrote:
struct mpc5xxx_i2c_tap { @@ -94,7 +94,7 @@ static int wait_for_bb(void) mpc_reg_out(®s->mcr, 0, 0); mpc_reg_out(®s->mcr, I2C_EN, 0); #endif
- udelay(1000);
- udelay(1);
status = mpc_reg_in(®s->msr); }
The actual condition we wait for takes only a few useconds to become true, but we wait for at least 1ms. As the I2C EEPROM environment routine reads nearly the complete EEPROM several times during startup the speed gain is enormous. I can't recall if it was intentional to reduce the overall timeout with this patch, but I can imagine that the i2c detect function will be faster aswell.
Even better, since this is entirely a busywait, the udelay(1) could be dropped entirely and the exit condition be based on the value of timebase. That would give the absolute maximum resolution to the busyloop. However, the incremental improvement probably isn't a great deal better than this patch.
g.
Dear Grant Likely,
In message fa686aa40903222000l13369f61gf93b26b4c804e383@mail.gmail.com you wrote:
Even better, since this is entirely a busywait, the udelay(1) could be dropped entirely and the exit condition be based on the value of timebase. That would give the absolute maximum resolution to the busyloop. However, the incremental improvement probably isn't a great deal better than this patch.
I disagree. Keep in mind that on systems with hardware watchdog enabled udelay() implicitly takes care of triggering the watchdog, so using it in busy-wait loops is always a good idea.
Best regards,
Wolfgang Denk
participants (7)
-
Ben Warren -
Grant Likely -
Heiko Schocher -
Jon -
Jon Smirl
-
Sascha Hauer -
Wolfgang Denk