[U-Boot] several problems with ethernet on MCF5445x

[U-Boot] several problems with ethernet on MCF5445x

[w.wegner at astro-kom.de]

Dear list,

I am trying to get ethernet to work on my custom MCF54455 board
and having some trouble.

I have a DP83848J PHY connected in MII mode to each of the FEC0
and FEC1 ports (seperate MDIO connection). Both PHYs are set to
address 0x0. *)

In my config (basically copied from M54455EVB), I set:

#define CONFIG_NET_MULTI		1
#define CONFIG_MII		1
#define CONFIG_MII_INIT		1
#define CONFIG_SYS_DISCOVER_PHY
#define CONFIG_SYS_FEC0_PINMUX	0
#define CONFIG_SYS_FEC1_PINMUX	0
#define CONFIG_SYS_FEC0_MIIBASE	CONFIG_SYS_FEC0_IOBASE
#define CONFIG_SYS_FEC1_MIIBASE	CONFIG_SYS_FEC1_IOBASE
[...]
#define CONFIG_HAS_ETH1
/*
 * this is defined on a per-target basis in immap.h - not a very clean
 * solution, so I define it here:
 */
#define CONFIG_SYS_FEC1_IOBASE		(MMAP_FEC1)

This gives me 2 FEC devices on startup.

My problems:
- with CONFIG_SYS_FEC1_MIIBASE set as above, U-Boot crashes upon
  "Retry count exceeded; starting again" during unsuccessful dhcp; this does
  not happen when I set it to CONFIG_SYS_FEC0_IOBASE. (But this setting
  seems wrong to me, as I have seperate MDIO connections)
- ethernet does not work at all unless I comment out
		if (info->iobase == CONFIG_SYS_FEC0_IOBASE)
			gpio->par_fec |= GPIO_PAR_FEC_FEC0_RMII_GPIO;
		else
			gpio->par_fec |= GPIO_PAR_FEC_FEC1_RMII_ATA;
  in mcf5445x/cpu_init.c: fecpin_setclear() (and initialize PAR_FEC in my own
  board init code).
  I do not understand what this code is for at all?
- what sense do CONFIG_SYS_FEC[01]_PINMUX have? They are filled into
  the "pinmux" field of struct fec_info_s in mcffec.c, but I did not find a place
  using them? From their naming I would have expected them to be responsible
  for correct pin mux setting (PAR_FEC), but then they would have to be used
  by fecpin_setclear(), which again would introduce weird dependencies on the
  mcffec.c code.

*) Is it needed/useful to set both PHYs to different addresses although they
  are connected to seperate MDIO connections (and seemingly correctly
  found when booting the linux kernel)?

Can anybody comment on this? Did I overlook something about where the
pin assignment is normally configured, or is it really just some strange
coincidence that it works on the EVB but not on my board?
 
Regards,
Wolfgang

[U-Boot] several problems with ethernet on MCF5445x

[John Schmoller]

On Tue, 2010-03-23 at 15:50 +0100, w.wegner at astro-kom.de wrote:
> Dear list,
> 
> I am trying to get ethernet to work on my custom MCF54455 board
> and having some trouble.
> 
> I have a DP83848J PHY connected in MII mode to each of the FEC0
> and FEC1 ports (seperate MDIO connection). Both PHYs are set to
> address 0x0. *)

Are you sure this is what you want?  From the data sheet:

Strapping PHY Address 0 puts the part into Isolate Mode.

When in the MII isolate mode, the DP83848J does not
respond to packet data present at TXD[3:0], TX_EN inputs
and presents a high impedance on the TX_CLK, RX_CLK,
RX_DV, RX_ER, RXD[3:0], COL, and CRS outputs. When
in Isolate mode, the DP83848J will continue to respond to
all management transactions.

John

[U-Boot] several problems with ethernet on MCF5445x

[w.wegner at astro-kom.de]

Dear John,

On 23 Mar 2010 at 10:11, John Schmoller wrote:

> On Tue, 2010-03-23 at 15:50 +0100, w.wegner at astro-kom.de wrote:
> > I have a DP83848J PHY connected in MII mode to each of the FEC0
> > and FEC1 ports (seperate MDIO connection). Both PHYs are set to
> > address 0x0. *)
> 
> Are you sure this is what you want?  From the data sheet:
> 
> Strapping PHY Address 0 puts the part into Isolate Mode.
[...]

sorry, my fault...
I forgot about the internal pullup and did not read carefully
that U-Boot correctly identified the PHY at address 1.

So, both PHYs are strapped to address 0x01.

My concern was, if there is any problem arising from two PHYs
sharing the same address while being accessed through
different physical interfaces. E.g., it seems I can only access
one of the PHYs via the "mii" commands - I am not sure if
this is a limitation originating from the identical addresses, or
some other problem with my setup.

Thanks for pointing out that mistake!

Regards,
Wolfgang

[U-Boot] several problems with ethernet on MCF5445x

[Liew Tsi Chung-R5AAHP]

Wolfgang,

As long as you are not using only one FEC's mdio/mdc to communicate the
both PHY at address 1. 

Best Regards,
TsiChung


-----Original Message-----
From: u-boot-bounces@lists.denx.de [mailto:u-boot-bounces at lists.denx.de]
On Behalf Of w.wegner at astro-kom.de
Sent: Tuesday, March 23, 2010 10:40 AM
To: John Schmoller; u-boot at lists.denx.de
Subject: Re: [U-Boot] several problems with ethernet on MCF5445x

Dear John,

On 23 Mar 2010 at 10:11, John Schmoller wrote:

> On Tue, 2010-03-23 at 15:50 +0100, w.wegner at astro-kom.de wrote:
> > I have a DP83848J PHY connected in MII mode to each of the FEC0 and 
> > FEC1 ports (seperate MDIO connection). Both PHYs are set to address 
> > 0x0. *)
> 
> Are you sure this is what you want?  From the data sheet:
> 
> Strapping PHY Address 0 puts the part into Isolate Mode.
[...]

sorry, my fault...
I forgot about the internal pullup and did not read carefully that
U-Boot correctly identified the PHY at address 1.

So, both PHYs are strapped to address 0x01.

My concern was, if there is any problem arising from two PHYs sharing
the same address while being accessed through different physical
interfaces. E.g., it seems I can only access one of the PHYs via the
"mii" commands - I am not sure if this is a limitation originating from
the identical addresses, or some other problem with my setup.

Thanks for pointing out that mistake!

Regards,
Wolfgang

_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] several problems with ethernet on MCF5445x

[w.wegner at astro-kom.de]

Hi all,

I still have problems with the ethernet on my custom MCF54452 board.

Here is my current configuration:

/* this is defined for M54455EVB in m68k/immap.h *sigh* */
#define CONFIG_SYS_FEC1_IOBASE		(MMAP_FEC1)

#define CONFIG_NET_MULTI		1
#define CONFIG_MII		1
#define CONFIG_MII_INIT		1
#define CONFIG_SYS_DISCOVER_PHY
#define CONFIG_SYS_RX_ETH_BUFFER	8
#define CONFIG_SYS_FAULT_ECHO_LINK_DOWN
#define CONFIG_SYS_FEC0_PINMUX	0
#define CONFIG_SYS_FEC1_PINMUX	0
#define CONFIG_SYS_FEC0_MIIBASE	CONFIG_SYS_FEC0_IOBASE
#define CONFIG_SYS_FEC1_MIIBASE	CONFIG_SYS_FEC1_IOBASE
#define MCFFEC_TOUT_LOOP 50000
#define CONFIG_HAS_ETH1

I modified fecpin_setclear() in cpu_init.c to set par_fec to
GPIO_PAR_FEC_FEC[01]_MII, still have to make a proper configuration
for this.

Now U-Boot recognizes both FEC0 and FEC1, but I can only access FEC0 with
the mii commands. Switching to FEC1 is accepted, but all commands work on
FEC0 (I can easily check this by manually switching the status LEDs via the
PHY registers). FEC0 is working - though with frequent "TX timeout" messages,
I did not yet figure out how to explicitly switch to FEC1.

When a command like dhcp times out and eth_init tries to switch to the other
interface (FEC1) with eth_current->init(), U-Boot locks up. I still have to look
further into this what exactly fails.

Furthermore, it seems strange to me that I have to set
CONFIG_FEC_SHARED_PHY=y in the linux configuration to get both PHYs
recognized, but I definitely have both PHYs wired to separate pins in the
hardware.

What I am unsure about:
- how am I supposed to set CONFIG_SYS_FEC1_MIIBASE? I think I have to
  set it to CONFIG_SYS_FEC1_IOBASE because I have seperate PHYs
  connected to seperate physical interfaces, is this correct?
- is there maybe any relation to the (from my view inverted) setting of
  CONFIG_SHARED_PHY in the linux kernel?

Best regards,
Wolfgang

[U-Boot] several problems with ethernet on MCF5445x

[w.wegner at astro-kom.de]

Hi,

On 26 Mar 2010 at 14:13, w.wegner at astro-kom.de wrote:
[...]
> When a command like dhcp times out and eth_init tries to switch to the other
> interface (FEC1) with eth_current->init(), U-Boot locks up. I still have to look
> further into this what exactly fails.

the lockup itself is caused by mii_discover_phy in mcfmii.c.

Apparently, phytype is detected as 0x0, and then in this loop

				for (i = 0; i < (sizeof(phyinfo) / sizeof(phy_info_t)); i++) {
					if (phyinfo[i].phyid == phytype) {
#ifdef ET_DEBUG
						printf("phyid %x - %s\n",
						       phyinfo[i].phyid,
						       phyinfo[i].strid);
#endif
						strcpy(info->phy_name, phyinfo[i].strid);
						info->phyname_init = 1;
						found = 1;
						break;
					}
				}

phytype is matched against the last entry of phyinfo erroneously:

phy_info_t phyinfo[] = {
	{0x0022561B, "AMD79C784VC"},	/* AMD 79C784VC */
[...]
	{0, 0}
};

However, I did not yet find out why phytype is detected as 0x0...

In any case, what should be the correct termination condition for
the above loop? Of course
	for (i = 0; i < (sizeof(phyinfo) / sizeof(phy_info_t) - 1); i++)
would do the trick, but it seems overly complicated to me. Any
better ideas?

Wolfgang

[U-Boot] several problems with ethernet on MCF5445x

[w.wegner at astro-kom.de]

On 26 Mar 2010 at 15:55, w.wegner at astro-kom.de wrote:
> phytype is matched against the last entry of phyinfo erroneously:
> 
> phy_info_t phyinfo[] = {
> 	{0x0022561B, "AMD79C784VC"},	/* AMD 79C784VC */
> [...]
> 	{0, 0}
> };

ouch, things are even worse... I did not quote enough of the source
code of mcfmii.c:

#ifndef CONFIG_SYS_UNSPEC_PHYID
#	define CONFIG_SYS_UNSPEC_PHYID		0
#endif
#ifndef CONFIG_SYS_UNSPEC_STRID
#	define CONFIG_SYS_UNSPEC_STRID		0
#endif
[...]

phy_info_t phyinfo[] = {
	{0x0022561B, "AMD79C784VC"},	/* AMD 79C784VC */
[...]
#if defined(CONFIG_SYS_UNSPEC_PHYID) && defined(CONFIG_SYS_UNSPEC_STRID)
	{CONFIG_SYS_UNSPEC_PHYID, CONFIG_SYS_UNSPEC_STRID},
#endif
	{0, 0}
};

So the list phyinfo is terminated by a double {0, 0}, {0, 0}.

Can phyid == 0x0 be used as a simple termination condition,
or could this be a valid phy id?

Wolfgang

[U-Boot] several problems with ethernet on MCF5445x

[Wolfgang Wegner]

Hi all,

the problems boiled down to two things:
- full MII and non-shared MII-management interface is not
  currently supported for MCF5445x
- when a PHY returns id 0x0, matching for this ID results in
  a lock-up in mcfmii.c

considering PHY ID 0x0 as broken anyways, I could get my board
to run with both ethernet interfaces with the patch below. As this
is not yet thoroughly tested and my tree is too much out of sync
at the moment, I do not provide a proper patch for now, but maybe
this already helps somebody else in case he runs into the same
problems.

Regards,
Wolfgang


diff --git a/cpu/mcf5445x/cpu_init.c b/cpu/mcf5445x/cpu_init.c
index 48b37df..8b86d6d 100644
--- a/cpu/mcf5445x/cpu_init.c
+++ b/cpu/mcf5445x/cpu_init.c
@@ -152,13 +152,32 @@ int fecpin_setclear(struct eth_device *dev, int setclear)
 	struct fec_info_s *info = (struct fec_info_s *)dev->priv;
 
 	if (setclear) {
+#ifdef CONFIG_SYS_FEC_NO_SHARED_MII
+		if (info->iobase == CONFIG_SYS_FEC0_IOBASE)
+			gpio->par_feci2c |=
+			    (GPIO_PAR_FECI2C_MDC0_MDC0 |
+			     GPIO_PAR_FECI2C_MDIO0_MDIO0);
+		else
+			gpio->par_feci2c |=
+			    (GPIO_PAR_FECI2C_MDC1_MDC1 |
+			     GPIO_PAR_FECI2C_MDIO1_MDIO1);
+#else
 		gpio->par_feci2c |=
 		    (GPIO_PAR_FECI2C_MDC0_MDC0 | GPIO_PAR_FECI2C_MDIO0_MDIO0);
+#endif
 
 		if (info->iobase == CONFIG_SYS_FEC0_IOBASE)
+#ifdef CONFIG_SYS_FEC_FULL_MII
+			gpio->par_fec |= GPIO_PAR_FEC_FEC0_MII;
+#else
 			gpio->par_fec |= GPIO_PAR_FEC_FEC0_RMII_GPIO;
+#endif
 		else
+#ifdef CONFIG_SYS_FEC_FULL_MII
+			gpio->par_fec |= GPIO_PAR_FEC_FEC1_MII;
+#else
 			gpio->par_fec |= GPIO_PAR_FEC_FEC1_RMII_ATA;
+#endif
 	} else {
 		gpio->par_feci2c &=
 		    ~(GPIO_PAR_FECI2C_MDC0_MDC0 | GPIO_PAR_FECI2C_MDIO0_MDIO0);
diff --git a/drivers/net/mcfmii.c b/drivers/net/mcfmii.c
index 4acc29e..e83bb07 100644
--- a/drivers/net/mcfmii.c
+++ b/drivers/net/mcfmii.c
@@ -185,7 +185,10 @@ int mii_discover_phy(struct eth_device *dev)
 				printf("PHY @ 0x%x pass %d\n", phyno, pass);
 #endif
 
-				for (i = 0; i < (sizeof(phyinfo) / sizeof(phy_info_t)); i++) {
+				for (i = 0;
+					(i < (sizeof(phyinfo) / sizeof(phy_info_t)))
+					&& (phyinfo[i].phyid != 0);
+					i++) {
 					if (phyinfo[i].phyid == phytype) {
 #ifdef ET_DEBUG
 						printf("phyid %x - %s\n",

[U-Boot] [PATCH] fix lockup in mcfmii/mii_discover_phy() in case communication fails

[Wolfgang Wegner]

Signed-off-by: Wolfgang Wegner <w.wegner@astro-kom.de>
---
 drivers/net/mcfmii.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/drivers/net/mcfmii.c b/drivers/net/mcfmii.c
index 4acc29e..83c0873 100644
--- a/drivers/net/mcfmii.c
+++ b/drivers/net/mcfmii.c
@@ -185,7 +185,11 @@ int mii_discover_phy(struct eth_device *dev)
 				printf("PHY @ 0x%x pass %d\n", phyno, pass);
 #endif
 
-				for (i = 0; i < (sizeof(phyinfo) / sizeof(phy_info_t)); i++) {
+				for (i = 0;
+					(i < (sizeof(phyinfo)
+						/ sizeof(phy_info_t)))
+					&& (phyinfo[i].phyid != 0);
+					i++) {
 					if (phyinfo[i].phyid == phytype) {
 #ifdef ET_DEBUG
 						printf("phyid %x - %s\n",
-- 
1.5.6.5

[U-Boot] [PATCH] add CONFIG_SYS_FEC_NO_SHARED_PHY for MCF5445x

[Wolfgang Wegner]

This patch adds the possibility to handle seperate PHYs to MCF5445x.
Naming is chosen to resemble the contrary CONFIG_FEC_SHARED_PHY in the
linux kernel.

Signed-off-by: Wolfgang Wegner <w.wegner@astro-kom.de>
---
 cpu/mcf5445x/cpu_init.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/cpu/mcf5445x/cpu_init.c b/cpu/mcf5445x/cpu_init.c
index 8d51d35..db03296 100644
--- a/cpu/mcf5445x/cpu_init.c
+++ b/cpu/mcf5445x/cpu_init.c
@@ -185,8 +185,19 @@ int fecpin_setclear(struct eth_device *dev, int setclear)
 	struct fec_info_s *info = (struct fec_info_s *)dev->priv;
 
 	if (setclear) {
+#ifdef CONFIG_SYS_FEC_NO_SHARED_PHY
+		if (info->iobase == CONFIG_SYS_FEC0_IOBASE)
+			gpio->par_feci2c |=
+			    (GPIO_PAR_FECI2C_MDC0_MDC0 |
+			     GPIO_PAR_FECI2C_MDIO0_MDIO0);
+		else
+			gpio->par_feci2c |=
+			    (GPIO_PAR_FECI2C_MDC1_MDC1 |
+			     GPIO_PAR_FECI2C_MDIO1_MDIO1);
+#else
 		gpio->par_feci2c |=
 		    (GPIO_PAR_FECI2C_MDC0_MDC0 | GPIO_PAR_FECI2C_MDIO0_MDIO0);
+#endif
 
 		if (info->iobase == CONFIG_SYS_FEC0_IOBASE)
 			gpio->par_fec |= GPIO_PAR_FEC_FEC0_RMII_GPIO;
-- 
1.5.6.5

[U-Boot] [PATCH] add CONFIG_SYS_FEC_FULL_MII for MCF5445x

[Wolfgang Wegner]

This patch adds support for full MII interface on MCF5445x (in contrast
to RMII as used on the evaluation boards).

Signed-off-by: Wolfgang Wegner <w.wegner@astro-kom.de>
---
 cpu/mcf5445x/cpu_init.c |   13 +++++++++++--
 1 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/cpu/mcf5445x/cpu_init.c b/cpu/mcf5445x/cpu_init.c
index db03296..2389019 100644
--- a/cpu/mcf5445x/cpu_init.c
+++ b/cpu/mcf5445x/cpu_init.c
@@ -207,10 +207,19 @@ int fecpin_setclear(struct eth_device *dev, int setclear)
 		gpio->par_feci2c &=
 		    ~(GPIO_PAR_FECI2C_MDC0_MDC0 | GPIO_PAR_FECI2C_MDIO0_MDIO0);
 
-		if (info->iobase == CONFIG_SYS_FEC0_IOBASE)
+		if (info->iobase == CONFIG_SYS_FEC0_IOBASE) {
+#ifdef CONFIG_SYS_FEC_FULL_MII
+			gpio->par_fec |= GPIO_PAR_FEC_FEC0_MII;
+#else
 			gpio->par_fec &= GPIO_PAR_FEC_FEC0_UNMASK;
-		else
+#endif
+		} else {
+#ifdef CONFIG_SYS_FEC_FULL_MII
+			gpio->par_fec |= GPIO_PAR_FEC_FEC1_MII;
+#else
 			gpio->par_fec &= GPIO_PAR_FEC_FEC1_UNMASK;
+#endif
+		}
 	}
 	return 0;
 }
-- 
1.5.6.5

[U-Boot] [PATCH] add CONFIG_SYS_FEC_NO_SHARED_PHY for MCF5445x

[Liew Tsi Chung-R5AAHP]

Acked. 

Best Regards,
TsiChung


-----Original Message-----
From: u-boot-bounces@lists.denx.de [mailto:u-boot-bounces at lists.denx.de]
On Behalf Of Wolfgang Wegner
Sent: Tuesday, March 30, 2010 12:20 PM
To: u-boot at lists.denx.de
Cc: Wolfgang Wegner
Subject: [U-Boot] [PATCH] add CONFIG_SYS_FEC_NO_SHARED_PHY for MCF5445x

This patch adds the possibility to handle seperate PHYs to MCF5445x.
Naming is chosen to resemble the contrary CONFIG_FEC_SHARED_PHY in the
linux kernel.

Signed-off-by: Wolfgang Wegner <w.wegner@astro-kom.de>
---
 cpu/mcf5445x/cpu_init.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/cpu/mcf5445x/cpu_init.c b/cpu/mcf5445x/cpu_init.c index
8d51d35..db03296 100644
--- a/cpu/mcf5445x/cpu_init.c
+++ b/cpu/mcf5445x/cpu_init.c
@@ -185,8 +185,19 @@ int fecpin_setclear(struct eth_device *dev, int
setclear)
 	struct fec_info_s *info = (struct fec_info_s *)dev->priv;
 
 	if (setclear) {
+#ifdef CONFIG_SYS_FEC_NO_SHARED_PHY
+		if (info->iobase == CONFIG_SYS_FEC0_IOBASE)
+			gpio->par_feci2c |=
+			    (GPIO_PAR_FECI2C_MDC0_MDC0 |
+			     GPIO_PAR_FECI2C_MDIO0_MDIO0);
+		else
+			gpio->par_feci2c |=
+			    (GPIO_PAR_FECI2C_MDC1_MDC1 |
+			     GPIO_PAR_FECI2C_MDIO1_MDIO1);
+#else
 		gpio->par_feci2c |=
 		    (GPIO_PAR_FECI2C_MDC0_MDC0 |
GPIO_PAR_FECI2C_MDIO0_MDIO0);
+#endif
 
 		if (info->iobase == CONFIG_SYS_FEC0_IOBASE)
 			gpio->par_fec |= GPIO_PAR_FEC_FEC0_RMII_GPIO;
--
1.5.6.5

_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH] fix lockup in mcfmii/mii_discover_phy() in case communication fails

[Ben Warren]

Hi Wolfgang,

On 3/30/2010 10:19 AM, Wolfgang Wegner wrote:
> Signed-off-by: Wolfgang Wegner<w.wegner@astro-kom.de>
> ---
>   drivers/net/mcfmii.c |    6 +++++-
>   1 files changed, 5 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/net/mcfmii.c b/drivers/net/mcfmii.c
> index 4acc29e..83c0873 100644
> --- a/drivers/net/mcfmii.c
> +++ b/drivers/net/mcfmii.c
> @@ -185,7 +185,11 @@ int mii_discover_phy(struct eth_device *dev)
>   				printf("PHY @ 0x%x pass %d\n", phyno, pass);
>   #endif
>
> -				for (i = 0; i<  (sizeof(phyinfo) / sizeof(phy_info_t)); i++) {
> +				for (i = 0;
> +					(i<  (sizeof(phyinfo)
> +						/ sizeof(phy_info_t)))
> +					&&  (phyinfo[i].phyid != 0);
> +					i++) {
>   					if (phyinfo[i].phyid == phytype) {
>   #ifdef ET_DEBUG
>   						printf("phyid %x - %s\n",
>    
This is brutal.  Using 8-space tabs really does a good job of 
highlighting deep nesting of conditionals.  If you're unable to make the 
driver easier to read (and I understand if that's the case), my 
preference would be to keep the sizeof()s on one line and combine the 
second && term and the i++ on another.  This makes lines > 80 chars, but 
it was already that way.

regards,
Ben

[U-Boot] [PATCH] fix lockup in mcfmii/mii_discover_phy() in case communication fails

[w.wegner at astro-kom.de]

Hi Ben,

On 4 Apr 2010 at 22:44, Ben Warren wrote:

> Hi Wolfgang,
> 
> On 3/30/2010 10:19 AM, Wolfgang Wegner wrote:
> > Signed-off-by: Wolfgang Wegner<w.wegner@astro-kom.de>
> > ---
> >   drivers/net/mcfmii.c |    6 +++++-
> >   1 files changed, 5 insertions(+), 1 deletions(-)
> >
> > diff --git a/drivers/net/mcfmii.c b/drivers/net/mcfmii.c
> > index 4acc29e..83c0873 100644
> > --- a/drivers/net/mcfmii.c
> > +++ b/drivers/net/mcfmii.c
> > @@ -185,7 +185,11 @@ int mii_discover_phy(struct eth_device *dev)
> >   				printf("PHY @ 0x%x pass %d\n", phyno, pass);
> >   #endif
> >
> > -				for (i = 0; i<  (sizeof(phyinfo) / sizeof(phy_info_t)); i++) {
> > +				for (i = 0;
> > +					(i<  (sizeof(phyinfo)
> > +						/ sizeof(phy_info_t)))
> > +					&&  (phyinfo[i].phyid != 0);
> > +					i++) {
> >   					if (phyinfo[i].phyid == phytype) {
> >   #ifdef ET_DEBUG
> >   						printf("phyid %x - %s\n",
> >    
> This is brutal.  Using 8-space tabs really does a good job of 
> highlighting deep nesting of conditionals.  If you're unable to make the 
> driver easier to read (and I understand if that's the case), my 
> preference would be to keep the sizeof()s on one line and combine the 
> second && term and the i++ on another.  This makes lines > 80 chars, but 
> it was already that way.

I agree, but was worried if the > 80 char line would be a reason to reject the
patch and wanted to have it as non-intrusive as possible.

If there are no objections, I am happy to supply a second version with
formatting as you suggested.

Regards,
Wolfgang

[U-Boot] [PATCH v2 RFC] fix lockup in mcfmii/mii_discover_phy() in case communication fails

[Wolfgang Wegner]

Signed-off-by: Wolfgang Wegner <w.wegner@astro-kom.de>
---
modified formatting of inner for() loop and changed if(cond){...} to
if(!cond) continue; to avoid deep indentation.

 drivers/net/mcfmii.c |   45 +++++++++++++++++++++++----------------------
 1 files changed, 23 insertions(+), 22 deletions(-)

diff --git a/drivers/net/mcfmii.c b/drivers/net/mcfmii.c
index 4acc29e..060bdd7 100644
--- a/drivers/net/mcfmii.c
+++ b/drivers/net/mcfmii.c
@@ -175,38 +175,39 @@ int mii_discover_phy(struct eth_device *dev)
 #ifdef ET_DEBUG
 			printf("PHY type 0x%x pass %d type\n", phytype, pass);
 #endif
-			if (phytype != 0xffff) {
-				phyaddr = phyno;
-				phytype <<= 16;
-				phytype |=
-				    mii_send(mk_mii_read(phyno, PHY_PHYIDR2));
+			if (phytype == 0xffff)
+				continue;
+			phyaddr = phyno;
+			phytype <<= 16;
+			phytype |=
+			    mii_send(mk_mii_read(phyno, PHY_PHYIDR2));
 
 #ifdef ET_DEBUG
-				printf("PHY @ 0x%x pass %d\n", phyno, pass);
+			printf("PHY @ 0x%x pass %d\n", phyno, pass);
 #endif
 
-				for (i = 0; i < (sizeof(phyinfo) / sizeof(phy_info_t)); i++) {
-					if (phyinfo[i].phyid == phytype) {
+			for (i = 0; (i < (sizeof(phyinfo) / sizeof(phy_info_t)))
+				&& (phyinfo[i].phyid != 0); i++) {
+				if (phyinfo[i].phyid == phytype) {
 #ifdef ET_DEBUG
-						printf("phyid %x - %s\n",
-						       phyinfo[i].phyid,
-						       phyinfo[i].strid);
+					printf("phyid %x - %s\n",
+					       phyinfo[i].phyid,
+					       phyinfo[i].strid);
 #endif
-						strcpy(info->phy_name, phyinfo[i].strid);
-						info->phyname_init = 1;
-						found = 1;
-						break;
-					}
+					strcpy(info->phy_name, phyinfo[i].strid);
+					info->phyname_init = 1;
+					found = 1;
+					break;
 				}
+			}
 
-				if (!found) {
+			if (!found) {
 #ifdef ET_DEBUG
-					printf("0x%08x\n", phytype);
+				printf("0x%08x\n", phytype);
 #endif
-					strcpy(info->phy_name, "unknown");
-					info->phyname_init = 1;
-					break;
-				}
+				strcpy(info->phy_name, "unknown");
+				info->phyname_init = 1;
+				break;
 			}
 		}
 	}
-- 
1.5.6.5

[U-Boot] [PATCH] fix lockup in mcfmii/mii_discover_phy() in case communication fails

[w.wegner at astro-kom.de]

Hi,

I just sent another proposal having the for(...) in only two lines
by replacing the outer indentation level 
	if (phytype != 0xffff) {...
	}
to
	if (phytype == 0xffff)
		continue;

Any comments on this?

Regards,
Wolfgang

[U-Boot] [PATCH v2 RFC] fix lockup in mcfmii/mii_discover_phy() in case communication fails

[Ben Warren]

Hi Wolfgang,

On 4/6/2010 2:13 AM, Wolfgang Wegner wrote:
> Signed-off-by: Wolfgang Wegner<w.wegner@astro-kom.de>
> ---
> modified formatting of inner for() loop and changed if(cond){...} to
> if(!cond) continue; to avoid deep indentation.
>
>   drivers/net/mcfmii.c |   45 +++++++++++++++++++++++----------------------
>   1 files changed, 23 insertions(+), 22 deletions(-)
>    
Much better.  Applied to net repo.

thanks,
Ben