[U-Boot-Users] [PATCH 1/1] Fix 8313ERDB board configuration

[U-Boot-Users] [PATCH 1/1] Fix 8313ERDB board configuration

[York Sun]

Change LCRR clock ratio from 2 to 4 to commodate VSC7385.
Correct TSEC1 vs TSEC2 assignment.
Define ETHADDR and ETH1ADDR always.

Signed-off-by: York Sun <yorksun@freescale.com>
Signed-off-by: Timur Tabi <timur@freescale.com>
---
 include/configs/MPC8313ERDB.h |   14 ++++++--------
 1 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/include/configs/MPC8313ERDB.h b/include/configs/MPC8313ERDB.h
index 6eec240..f9fa535 100644
--- a/include/configs/MPC8313ERDB.h
+++ b/include/configs/MPC8313ERDB.h
@@ -42,9 +42,12 @@
 
 /*
  * On-board devices
+ *
+ * TSEC1 is VSC switch
+ * TSEC2 is SoC TSEC
  */
 #define CONFIG_VSC7385_ENET
-
+#define CONFIG_TSEC2
 
 #ifdef CFG_66MHZ
 #define CONFIG_83XX_CLKIN	66666667	/* in Hz */
@@ -80,7 +83,7 @@
 
 #ifdef CONFIG_VSC7385_ENET
 
-#define CONFIG_TSEC2
+#define CONFIG_TSEC1
 
 /* The flash address and size of the VSC7385 firmware image */
 #define CONFIG_VSC7385_IMAGE		0xFE7FE000
@@ -209,7 +212,7 @@
 /*
  * Local Bus LCRR and LBCR regs
  */
-#define CFG_LCRR	LCRR_EADC_1 | LCRR_CLKDIV_2	/* 0x00010002 */
+#define CFG_LCRR	LCRR_EADC_1 | LCRR_CLKDIV_4
 #define CFG_LBC_LBCR	( 0x00040000 /* TODO */ \
 			| (0xFF << LBCR_BMT_SHIFT) \
 			| 0xF )	/* 0x0004ff0f */
@@ -523,13 +526,8 @@
  */
 #define CONFIG_ENV_OVERWRITE
 
-#ifdef CONFIG_HAS_ETH0
 #define CONFIG_ETHADDR		00:E0:0C:00:95:01
-#endif
-
-#ifdef CONFIG_HAS_ETH1
 #define CONFIG_ETH1ADDR		00:E0:0C:00:95:02
-#endif
 
 #define CONFIG_IPADDR		10.0.0.2
 #define CONFIG_SERVERIP		10.0.0.1
-- 
1.5.2.2

[U-Boot-Users] [PATCH 1/1] Fix 8313ERDB board configuration

[Ben Warren]

Hi York,

York Sun wrote:
> Change LCRR clock ratio from 2 to 4 to commodate VSC7385.
> Correct TSEC1 vs TSEC2 assignment.
> Define ETHADDR and ETH1ADDR always.
>
> Signed-off-by: York Sun <yorksun@freescale.com>
> Signed-off-by: Timur Tabi <timur@freescale.com>
> ---
>  include/configs/MPC8313ERDB.h |   14 ++++++--------
>  1 files changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/include/configs/MPC8313ERDB.h b/include/configs/MPC8313ERDB.h
> index 6eec240..f9fa535 100644
> --- a/include/configs/MPC8313ERDB.h
> +++ b/include/configs/MPC8313ERDB.h
> @@ -42,9 +42,12 @@
>  
>  /*
>   * On-board devices
> + *
> + * TSEC1 is VSC switch
> + * TSEC2 is SoC TSEC
>   */
>  #define CONFIG_VSC7385_ENET
> -
> +#define CONFIG_TSEC2
>  
>  #ifdef CFG_66MHZ
>  #define CONFIG_83XX_CLKIN	66666667	/* in Hz */
> @@ -80,7 +83,7 @@
>  
>  #ifdef CONFIG_VSC7385_ENET
>  
> -#define CONFIG_TSEC2
> +#define CONFIG_TSEC1
>  
>  /* The flash address and size of the VSC7385 firmware image */
>  #define CONFIG_VSC7385_IMAGE		0xFE7FE000
> @@ -209,7 +212,7 @@
>  /*
>   * Local Bus LCRR and LBCR regs
>   */
> -#define CFG_LCRR	LCRR_EADC_1 | LCRR_CLKDIV_2	/* 0x00010002 */
> +#define CFG_LCRR	LCRR_EADC_1 | LCRR_CLKDIV_4
>  #define CFG_LBC_LBCR	( 0x00040000 /* TODO */ \
>  			| (0xFF << LBCR_BMT_SHIFT) \
>  			| 0xF )	/* 0x0004ff0f */
> @@ -523,13 +526,8 @@
>   */
>  #define CONFIG_ENV_OVERWRITE
>  
> -#ifdef CONFIG_HAS_ETH0
>  #define CONFIG_ETHADDR		00:E0:0C:00:95:01
> -#endif
> -
> -#ifdef CONFIG_HAS_ETH1
>  #define CONFIG_ETH1ADDR		00:E0:0C:00:95:02
> -#endif
>   
Please also remove the default MAC address assignments.
>  
>  #define CONFIG_IPADDR		10.0.0.2
>  #define CONFIG_SERVERIP		10.0.0.1
>   
While you're at it, putting default IP addresses is also pointless.

regards,
Ben

[U-Boot-Users] [PATCH 1/1] Fix 8313ERDB board configuration

[York Sun]

Ben,

On Thu, 2008-05-15 at 13:50 -0700, Ben Warren wrote:
> > -#ifdef CONFIG_HAS_ETH0
> >  #define CONFIG_ETHADDR		00:E0:0C:00:95:01
> > -#endif
> > -
> > -#ifdef CONFIG_HAS_ETH1
> >  #define CONFIG_ETH1ADDR		00:E0:0C:00:95:02
> > -#endif
> >   
> Please also remove the default MAC address assignments.

I think we need them, don't we?

> >  
> >  #define CONFIG_IPADDR		10.0.0.2
> >  #define CONFIG_SERVERIP		10.0.0.1
> >   
> While you're at it, putting default IP addresses is also pointless.

At least it tells you that you can put your address here. It doesn't
hurt.
And BTW, I want to make my patch as small as possible. I'll leave the
rest to the board maintainer.

York

[U-Boot-Users] [PATCH 1/1] Fix 8313ERDB board configuration

[Ben Warren]

York Sun wrote:
> Ben,
>
> On Thu, 2008-05-15 at 13:50 -0700, Ben Warren wrote:
>   
>>> -#ifdef CONFIG_HAS_ETH0
>>>  #define CONFIG_ETHADDR		00:E0:0C:00:95:01
>>> -#endif
>>> -
>>> -#ifdef CONFIG_HAS_ETH1
>>>  #define CONFIG_ETH1ADDR		00:E0:0C:00:95:02
>>> -#endif
>>>   
>>>       
>> Please also remove the default MAC address assignments.
>>     
>
> I think we need them, don't we?
>
>   
No, there have been many discussions about this, and the consensus is 
that there shouldn't be default addresses assigned.  One of many reasons 
is that the numbers you've chosen actually belong to some kind of 
corporate entity (I would guess Freescale) and not the end user.  It's 
better for the end user to either use private addresses or ones that 
they have purchased from IEEE.
>>>  
>>>  #define CONFIG_IPADDR		10.0.0.2
>>>  #define CONFIG_SERVERIP		10.0.0.1
>>>   
>>>       
>> While you're at it, putting default IP addresses is also pointless.
>>     
>
> At least it tells you that you can put your address here. It doesn't
> hurt.
> And BTW, I want to make my patch as small as possible. I'll leave the
> rest to the board maintainer.
>
> York
>
>
>   
I'm less picky about this one because as you say it doesn't really 
hurt.  All it will do is initialize the network to an unreachable state 
on the vast majority of purchasers of your eval boards.  Let's leave it 
up to the board maintainer.

regards,
Ben

[U-Boot-Users] [PATCH 1/1] Fix 8313ERDB board configuration

[Wolfgang Denk]

In message <1210886469.880.45.camel@laptop> you wrote:
> 
> > Please also remove the default MAC address assignments.
> 
> I think we need them, don't we?

No, or you can install this U-Boot image exactly on one specific,
single board.

> > >  #define CONFIG_IPADDR		10.0.0.2
> > >  #define CONFIG_SERVERIP		10.0.0.1
> > >   
> > While you're at it, putting default IP addresses is also pointless.
> 
> At least it tells you that you can put your address here. It doesn't
> hurt.

Yes, it does hurt. It's a PITA. Please get rid of such "standard"
network settings that are almost always and everywhere wrong.

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 at denx.de
Those who do not  understand  Unix  are  condemned  to  reinvent  it,
poorly.              - Henry Spencer, University of Toronto Unix hack

[U-Boot-Users] [PATCH 1/1] Fix 8313ERDB board configuration

[Timur Tabi]

Wolfgang Denk wrote:
> In message <1210886469.880.45.camel@laptop> you wrote:
>>> Please also remove the default MAC address assignments.
>> I think we need them, don't we?
> 
> No, or you can install this U-Boot image exactly on one specific,
> single board.

I understand your argument, but it's rather disingenuous when you allow this
code to exist:

#ifdef	CONFIG_ETHADDR
	"ethaddr="	MK_STR(CONFIG_ETHADDR)		"\0"
#endif
#ifdef	CONFIG_ETH1ADDR
	"eth1addr="	MK_STR(CONFIG_ETH1ADDR)		"\0"
#endif
#ifdef	CONFIG_ETH2ADDR
	"eth2addr="	MK_STR(CONFIG_ETH2ADDR)		"\0"
#endif
#ifdef	CONFIG_ETH3ADDR
	"eth3addr="	MK_STR(CONFIG_ETH3ADDR)		"\0"
#endif
#ifdef	CONFIG_IPADDR
	"ipaddr="	MK_STR(CONFIG_IPADDR)		"\0"
#endif
#ifdef	CONFIG_SERVERIP
	"serverip="	MK_STR(CONFIG_SERVERIP)		"\0"
#endif

If you get rid of this block, you'll solve the problem for all boards, instead
of nagging people who post patches for unrelated bugs.

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot-Users] [PATCH 1/1] Fix 8313ERDB board configuration

[Wolfgang Denk]

In message <482CB880.6050300@freescale.com> you wrote:
>
> I understand your argument, but it's rather disingenuous when you allow this
> code to exist:

"UNIX was not designed to stop you from doing stupid things,  because
that would also stop you from doing clever things."       - Doug Gwyn

> If you get rid of this block, you'll solve the problem for all boards, instead
> of nagging people who post patches for unrelated bugs.

There are a few cases where exactly this is  needed.  The  fact  that
some  feature  exists  does  not  mean that you should use it without
careful consideration of the effects and side-effects.

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 at denx.de
It's a small world, but I wouldn't want to paint it.

[U-Boot-Users] [PATCH 1/1] Fix 8313ERDB board configuration

[Timur Tabi]

Wolfgang Denk wrote:

> There are a few cases where exactly this is  needed.  The  fact  that
> some  feature  exists  does  not  mean that you should use it without
> careful consideration of the effects and side-effects.

Fair enough, but the changes that are in York's patch have been tested.
Considering that the fix window for 1.3.3 is closing fast, we don't have the
bandwidth for additional testing of the additional and unrelated changes that
are being requested.

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot-Users] [PATCH 1/1] Fix 8313ERDB board configuration

[Wolfgang Denk]

In message <482CBCA4.2050009@freescale.com> you wrote:
>
> Fair enough, but the changes that are in York's patch have been tested.
> Considering that the fix window for 1.3.3 is closing fast, we don't have the
> bandwidth for additional testing of the additional and unrelated changes that
> are being requested.

What exactly is the risk?

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 at denx.de
Use C++ to confuse your enemies; use C to produce stable code.

[U-Boot-Users] [PATCH 1/1] Fix 8313ERDB board configuration

[Timur Tabi]

Wolfgang Denk wrote:
> In message <482CBCA4.2050009@freescale.com> you wrote:
>> Fair enough, but the changes that are in York's patch have been tested.
>> Considering that the fix window for 1.3.3 is closing fast, we don't have the
>> bandwidth for additional testing of the additional and unrelated changes that
>> are being requested.
> 
> What exactly is the risk?

Probably not much, but that isn't my point.  I find it really annoying when
someone posts a patch to fix a specific bug, and the reply is "why don't you fix
this other bug too, while you're at it?"

Right now, the 8313ERDB will generate a machine check if you boot it with the
top-of-tree kernel.  This patch fixes that.  For 1.3.4, if you like, we can
submit a patch that removes all MAC and IP address macros from all 83xx boards.

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot-Users] [PATCH 1/1] Fix 8313ERDB board configuration

[Ben Warren]

Timur Tabi wrote:
> Wolfgang Denk wrote:
>   
>> In message <482CBCA4.2050009@freescale.com> you wrote:
>>     
>>> Fair enough, but the changes that are in York's patch have been tested.
>>> Considering that the fix window for 1.3.3 is closing fast, we don't have the
>>> bandwidth for additional testing of the additional and unrelated changes that
>>> are being requested.
>>>       
>> What exactly is the risk?
>>     
>
> Probably not much, but that isn't my point.  I find it really annoying when
> someone posts a patch to fix a specific bug, and the reply is "why don't you fix
> this other bug too, while you're at it?"
>
> Right now, the 8313ERDB will generate a machine check if you boot it with the
> top-of-tree kernel.  This patch fixes that.  For 1.3.4, if you like, we can
> submit a patch that removes all MAC and IP address macros from all 83xx boards.
>
>   
That seems reasonable.  Wolfgang, if you agree, please pull York's patch 
directly.

regards,
Ben

[U-Boot-Users] [PATCH 1/1] Fix 8313ERDB board configuration

[Wolfgang Denk]

In message <482CD47E.9010003@gmail.com> you wrote:
>
> >>> Fair enough, but the changes that are in York's patch have been tested.
> >>> Considering that the fix window for 1.3.3 is closing fast, we don't have the
> >>> bandwidth for additional testing of the additional and unrelated changes that
> >>> are being requested.
...
> That seems reasonable.  Wolfgang, if you agree, please pull York's patch 
> directly.

Sorry, I missed this one.

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 at denx.de
f u cn rd ths, itn tyg h myxbl cd.

[U-Boot-Users] [PATCH 1/1] Fix 8313ERDB board configuration

[Wolfgang Denk]

In message <12108831871831-git-send-email-yorksun@freescale.com> you wrote:
> Change LCRR clock ratio from 2 to 4 to commodate VSC7385.
> Correct TSEC1 vs TSEC2 assignment.
> Define ETHADDR and ETH1ADDR always.
> 
> Signed-off-by: York Sun <yorksun@freescale.com>
> Signed-off-by: Timur Tabi <timur@freescale.com>
> ---
>  include/configs/MPC8313ERDB.h |   14 ++++++--------
>  1 files changed, 6 insertions(+), 8 deletions(-)

Applied.

Please keep the review comments in mind, and send a follow-up patch
ASAP.

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 at denx.de
The universe contains any amount of horrible ways  to  be  woken  up,
such as the noise of the mob breaking down the front door, the scream
of fire engines, or the realization that today is the Monday which on
Friday night was a comfortably long way off.
                                 - Terry Pratchett, _Moving Pictures_