[U-Boot] MPC512x FEC/MII

[U-Boot] MPC512x FEC/MII

[Schneider, Kolja]

Hi all,

I have recently updated the U-Boot on one of our MPC512x-based boards from 2009.11 (+some patches) to a current u-boot build and noticed that the git commit  525856d59910c72687ab6201f39cdf1c04cfc15 apparenty broke the mii commands (see below) on this board. The patch moved PHY initialization from probe into init routine. The mii read commands return zero values regardless of previous FEC usage. Has anyone else come across similar behavior?

Thanks a lot,
Kolja

/* with git commit  525856d59910c72687ab6201f39cdf1c04cfc15 */
EM10A=> mii i
PHY 0x00: OUI = 0x0000, Model = 0x00, Rev = 0x00,  10baseT, HDX
PHY 0x01: OUI = 0x0000, Model = 0x00, Rev = 0x00,  10baseT, HDX
PHY 0x02: OUI = 0x0000, Model = 0x00, Rev = 0x00,  10baseT, HDX
PHY 0x03: OUI = 0x0000, Model = 0x00, Rev = 0x00,  10baseT, HDX
/* snip */

/* without git commit  525856d59910c72687ab6201f39cdf1c04cfc15 */
EM10A=> mii i
PHY 0x00: OUI = 0x0885, Model = 0x11, Rev = 0x03, 100baseT, FDX
PHY 0x01: OUI = 0x0885, Model = 0x11, Rev = 0x03, 100baseT, FDX

=== modified file 'drivers/net/mpc512x_fec.c'
--- drivers/net/mpc512x_fec.c	2010-05-03 21:52:48 +0000
+++ drivers/net/mpc512x_fec.c	2010-05-03 21:52:48 +0000
@@ -160,7 +160,7 @@
 }
 
 /********************************************************************/
-static void mpc512x_fec_set_hwaddr (mpc512x_fec_priv *fec, char *mac)
+static void mpc512x_fec_set_hwaddr (mpc512x_fec_priv *fec, unsigned char *mac)
 {
 	u8 currByte;			/* byte for which to compute the CRC */
 	int byte;			/* loop - counter */
@@ -226,6 +226,12 @@
 	printf ("mpc512x_fec_init... Begin\n");
 #endif
 
+	mpc512x_fec_set_hwaddr (fec, dev->enetaddr);
+	out_be32(&fec->eth->gaddr1, 0x00000000);
+	out_be32(&fec->eth->gaddr2, 0x00000000);
+
+	mpc512x_fec_init_phy (dev, bis);
+
 	/* Set interrupt mask register */
 	out_be32(&fec->eth->imask, 0x00000000);
 
@@ -611,8 +617,6 @@
 	volatile immap_t *im = (immap_t *) CONFIG_SYS_IMMR;
 	mpc512x_fec_priv *fec;
 	struct eth_device *dev;
-	int i;
-	char *tmp, *end, env_enetaddr[6];
 	void * bd;
 
 	fec = (mpc512x_fec_priv *) malloc (sizeof(*fec));
@@ -663,25 +667,6 @@
 	 */
 	out_be32(&fec->eth->ievent, 0xffffffff);
 
-	/*
-	 * Try to set the mac address now. The fec mac address is
-	 * a garbage after reset. When not using fec for booting
-	 * the Linux fec driver will try to work with this garbage.
-	 */
-	tmp = getenv ("ethaddr");
-	if (tmp) {
-		for (i=0; i<6; i++) {
-			env_enetaddr[i] = tmp ? simple_strtoul (tmp, &end, 16) : 0;
-			if (tmp)
-				tmp = (*end) ? end+1 : end;
-		}
-		mpc512x_fec_set_hwaddr (fec, env_enetaddr);
-		out_be32(&fec->eth->gaddr1, 0x00000000);
-		out_be32(&fec->eth->gaddr2, 0x00000000);
-	}
-
-	mpc512x_fec_init_phy (dev, bis);
-
 	return 1;
 }

Kolja Schneider, Software Design
MEN Mikro Elektronik GmbH
Neuwieder Stra?e 5-7
90411 N?rnberg, Germany
Phone +49-911-99 33 5-251
Fax +49-911-99 33 5-910
Kolja.Schneider at men.de
www.men.de



MEN Mikro Elektronik GmbH - Manfred Schmitz (CTO), Udo Fuchs (CFO) - Handelsregister/Trade Register AG N?rnberg HRB 5540
Please consider the environment before printing this e-mail

[U-Boot] MPC512x FEC/MII

[Bernhard Kaindl]

  Hello Kolja,

we also get broken output from mii_cmd when the commit which you show 
below is applied, reverting it helps.

Hardware: Our own MPC5121ADS-based board and the MPC5121ADS development 
board.

The commit is titled:

mpc512x_fec: Move PHY initialization from probe into init routin
This saves the autonegotation delay when not using ethernet in U-Boot",

This exactly what it does, the effect is no suprise at all:

Of course, the move of the PHY init from the fec_probe (executed before 
the u-boot prompt appears) to the fec_init (called only and every time a 
new network command is executed) has the effect which we see:

The PHY is only initialized while a network command (such as dhcp, tftp, 
...) is being executed.

Also, when network commands terminate, the network device's halt routine 
is executed and in the case of the MPC512x FEC driver, it resets the FEC 
unit of the chip, bringing the MII communication down again. The result 
is that we never see a working mii command with this patch.

But not only the mii_cmd is broken by this change, also the Ethernet 
autonegotation process which is executed at the end of 
mpc512x_fec_init_phy() is moved from network device probe call to the 
netcommand init calls:

This causes the the Ethernet link is down after the network device probe 
and the autonegotiation process is executed EVERY time a network command 
runs. This means every time you run a dhcp, tftp, nfs, dns or other 
network command, you get a new autonegotiation process which may be fast 
but may also be slow. You may notice these delay at every network 
command when you run a series of commands to net-boot your device, e.g.:

dhcp; tftp of a boot-script (as part of the dhcp call);tftp kernel; tftp 
initramfs; tftp fdt

In our case, the routers seem to be set up so that after a system 
appears on the network (auto-negotiation complete), it takes several 
seconds before packets are being forwarded to the DHCP and TFTP servers, 
so it became quite painful.

We had to revert this change and I replaced it with a patch to not wait 
for the auto-negoation to complete in probe.
This means that the PHY can auto-negotiatiate in the background and the 
board does not waste 2.5 seconds waiting
for it if no cable is connected, which achieves the goal of the patch 
(to not wait for auto-neg when not needed).

We've had two iterations of this patch so far and I need to review the 
second iteration more closely before posting.

Best Regards,
Bernhard

Am 09.09.2011 11:51, schrieb Schneider, Kolja:
> Hi all,
>
> I have recently updated the U-Boot on one of our MPC512x-based boards from 2009.11 (+some patches) to a current u-boot build and noticed that the git commit  525856d59910c72687ab6201f39cdf1c04cfc15 apparenty broke the mii commands (see below) on this board. The patch moved PHY initialization from probe into init routine. The mii read commands return zero values regardless of previous FEC usage. Has anyone else come across similar behavior?
>
> Thanks a lot,
> Kolja
>
> /* with git commit  525856d59910c72687ab6201f39cdf1c04cfc15 */
> EM10A=>  mii i
> PHY 0x00: OUI = 0x0000, Model = 0x00, Rev = 0x00,  10baseT, HDX
> PHY 0x01: OUI = 0x0000, Model = 0x00, Rev = 0x00,  10baseT, HDX
> PHY 0x02: OUI = 0x0000, Model = 0x00, Rev = 0x00,  10baseT, HDX
> PHY 0x03: OUI = 0x0000, Model = 0x00, Rev = 0x00,  10baseT, HDX
> /* snip */
>
> /* without git commit  525856d59910c72687ab6201f39cdf1c04cfc15 */
> EM10A=>  mii i
> PHY 0x00: OUI = 0x0885, Model = 0x11, Rev = 0x03, 100baseT, FDX
> PHY 0x01: OUI = 0x0885, Model = 0x11, Rev = 0x03, 100baseT, FDX
>
> === modified file 'drivers/net/mpc512x_fec.c'
> --- drivers/net/mpc512x_fec.c	2010-05-03 21:52:48 +0000
> +++ drivers/net/mpc512x_fec.c	2010-05-03 21:52:48 +0000
> @@ -160,7 +160,7 @@
>   }
>
>   /********************************************************************/
> -static void mpc512x_fec_set_hwaddr (mpc512x_fec_priv *fec, char *mac)
> +static void mpc512x_fec_set_hwaddr (mpc512x_fec_priv *fec, unsigned char *mac)
>   {
>   	u8 currByte;			/* byte for which to compute the CRC */
>   	int byte;			/* loop - counter */
> @@ -226,6 +226,12 @@
>   	printf ("mpc512x_fec_init... Begin\n");
>   #endif
>
> +	mpc512x_fec_set_hwaddr (fec, dev->enetaddr);
> +	out_be32(&fec->eth->gaddr1, 0x00000000);
> +	out_be32(&fec->eth->gaddr2, 0x00000000);
> +
> +	mpc512x_fec_init_phy (dev, bis);
> +
>   	/* Set interrupt mask register */
>   	out_be32(&fec->eth->imask, 0x00000000);
>
> @@ -611,8 +617,6 @@
>   	volatile immap_t *im = (immap_t *) CONFIG_SYS_IMMR;
>   	mpc512x_fec_priv *fec;
>   	struct eth_device *dev;
> -	int i;
> -	char *tmp, *end, env_enetaddr[6];
>   	void * bd;
>
>   	fec = (mpc512x_fec_priv *) malloc (sizeof(*fec));
> @@ -663,25 +667,6 @@
>   	 */
>   	out_be32(&fec->eth->ievent, 0xffffffff);
>
> -	/*
> -	 * Try to set the mac address now. The fec mac address is
> -	 * a garbage after reset. When not using fec for booting
> -	 * the Linux fec driver will try to work with this garbage.
> -	 */
> -	tmp = getenv ("ethaddr");
> -	if (tmp) {
> -		for (i=0; i<6; i++) {
> -			env_enetaddr[i] = tmp ? simple_strtoul (tmp,&end, 16) : 0;
> -			if (tmp)
> -				tmp = (*end) ? end+1 : end;
> -		}
> -		mpc512x_fec_set_hwaddr (fec, env_enetaddr);
> -		out_be32(&fec->eth->gaddr1, 0x00000000);
> -		out_be32(&fec->eth->gaddr2, 0x00000000);
> -	}
> -
> -	mpc512x_fec_init_phy (dev, bis);
> -
>   	return 1;
>   }
>
> Kolja Schneider, Software Design
> MEN Mikro Elektronik GmbH
> Neuwieder Stra?e 5-7
> 90411 N?rnberg, Germany
> Phone +49-911-99 33 5-251
> Fax +49-911-99 33 5-910
> Kolja.Schneider at men.de
> www.men.de
>
>
>
> MEN Mikro Elektronik GmbH - Manfred Schmitz (CTO), Udo Fuchs (CFO) - Handelsregister/Trade Register AG N?rnberg HRB 5540
> Please consider the environment before printing this e-mail
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] MPC512x FEC/MII

[Detlev Zundel]

Hi Bernhard,

> we also get broken output from mii_cmd when the commit which you show 
> below is applied, reverting it helps.
>
> Hardware: Our own MPC5121ADS-based board and the MPC5121ADS development 
> board.
>
> The commit is titled:
>
> mpc512x_fec: Move PHY initialization from probe into init routin
> This saves the autonegotation delay when not using ethernet in U-Boot",
>
> This exactly what it does, the effect is no suprise at all:
>
> Of course, the move of the PHY init from the fec_probe (executed before 
> the u-boot prompt appears) to the fec_init (called only and every time a 
> new network command is executed) has the effect which we see:
>
> The PHY is only initialized while a network command (such as dhcp, tftp, 
> ...) is being executed.

Well, this is actually in line with the U-Boot design principles of
initializing a device only if its needed[1].  This principle was
disregarded regularly in the past and so it seems kind of naturla that
"fixing" it results in changes of behaviour.  We should however fix the
fallout rather than going back to violating the principles.

If the mii routines need initializations, then they should ensure that
they have been done.

Cheers
  Detlev

[1] http://www.denx.de/wiki/view/U-Boot/DesignPrinciples#2_Keep_it_Fast

-- 
Practice random senselessness and act kind of beautiful.
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de

[U-Boot] MPC512x FEC/MII

[Schneider, Kolja]

Hi Detlev,

> -----Urspr?ngliche Nachricht-----
> Von: Detlev Zundel [mailto:dzu at denx.de]
> Gesendet: Dienstag, 27. September 2011 16:28
> An: Bernhard Kaindl
> Cc: Schneider, Kolja; u-boot at lists.denx.de
> Betreff: Re: [U-Boot] MPC512x FEC/MII
> 
> Hi Bernhard,
> 
> > we also get broken output from mii_cmd when the commit which you
> show
> > below is applied, reverting it helps.
> >
> > Hardware: Our own MPC5121ADS-based board and the MPC5121ADS
> development
> > board.
> >
> > The commit is titled:
> >
> > mpc512x_fec: Move PHY initialization from probe into init routin
> > This saves the autonegotation delay when not using ethernet in U-Boot",
> >
> > This exactly what it does, the effect is no suprise at all:
> >
> > Of course, the move of the PHY init from the fec_probe (executed before
> > the u-boot prompt appears) to the fec_init (called only and every time a
> > new network command is executed) has the effect which we see:
> >
> > The PHY is only initialized while a network command (such as dhcp, tftp,
> > ...) is being executed.
> 
> Well, this is actually in line with the U-Boot design principles of
> initializing a device only if its needed[1].  This principle was
> disregarded regularly in the past and so it seems kind of naturla that
> "fixing" it results in changes of behaviour.  We should however fix the
> fallout rather than going back to violating the principles.
> 
> If the mii routines need initializations, then they should ensure that
> they have been done.

This really is a shame as the mii tools are quite helpful for debugging/hardware verification. If the mii-command performs a re-initialisation each time they are invoked, they are probably close to useless (as one would need several commands to perform certain tasks...).

However I understand this is something that is not quite in the scope of a bootloader. As long as an easy workaround is available...

:) Kolja

[U-Boot] MPC512x FEC/MII

[Detlev Zundel]

Hi Kolja,

[...]

>> > The PHY is only initialized while a network command (such as dhcp, tftp,
>> > ...) is being executed.
>> 
>> Well, this is actually in line with the U-Boot design principles of
>> initializing a device only if its needed[1].  This principle was
>> disregarded regularly in the past and so it seems kind of naturla that
>> "fixing" it results in changes of behaviour.  We should however fix the
>> fallout rather than going back to violating the principles.
>> 
>> If the mii routines need initializations, then they should ensure that
>> they have been done.
>
> This really is a shame as the mii tools are quite helpful for
> debugging/hardware verification. If the mii-command performs a
> re-initialisation each time they are invoked, they are probably close
> to useless (as one would need several commands to perform certain
> tasks...).

I never said that the mii commands should initialize the phy _every_
time they are called.  All I said was that if they need an init, they
should ensure it has been run - this can also be done only once.

> However I understand this is something that is not quite in the scope
> of a bootloader. As long as an easy workaround is available...

I think this is in the scope of a bootloader - all I want to say is that
we should fix the problems that we encounter getting closer to our own
goals rather than giving up our goals to fix an individual problem.

Actually I believe that the move towards a driver model discussed on the
ML only recently, can solve many of these problems currently handled in
all the sub-modules and can centralize it in a single place.  By then we
would have a "initialized" field for every device and the commands (mii
or net commands) that deal with a net device can ensure that its "init"
method runs if this is not set.

Also with this approach, before booting an operating system, we can
deinitialize all devices again.  Although this sounds somewhat strange,
this was one of the more hard to find bugs that I have seen - an usb
controller happily updating frame counters in RAM while Linux booted...

So maybe the solution to this problem can be reached by helping other
code changes ;)

Cheers
  Detlev

-- 
Progress in  mathematics comes from  repeated acts of generalization.
If mathematics is anything, it is the art of chosing the most elegant
generalization for some abstract pattern.  Thus esthetics is central.
                                     -- Douglas Hofstadter
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de