Re: [U-Boot] [RFC PATCH 2/3] net: add phylib implementation

2009-08-27 Thread Scott Wood
On Thu, Aug 27, 2009 at 01:54:47AM -0400, Mike Frysinger wrote:
 On Thursday 27 August 2009 01:11:32 Stefan Roese wrote:
  On Thursday 27 August 2009 03:55:57 Mike Frysinger wrote:
+int phy_init(void)
+{
+   /* Initialize the list */
+   INIT_LIST_HEAD(phy_drvs.list);
  
   does it really need to be dynamic ?  arent there static initializers so
   this can be done in .data ?
 
  Using static initializers fails on PPC because of the somewhat broken
  relocation. So this dynamic implementation is the preferred version, at
  least for PPC people.
 
 so the ppc port cant use the .data section at all ?  that doesnt sound 
 correct 
 to me.

We can't have initialized pointers in .data without manual runtime fixup. 
Only .text has relocation information.

Someone tried to get proper relocation working a while ago, but ran into
toolchain bugs.  Maybe current toolchains are better...

-Scott
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


Re: [U-Boot] [RFC PATCH 2/3] net: add phylib implementation

2009-08-27 Thread Peter Tyser
On Thu, 2009-08-27 at 10:38 -0500, Scott Wood wrote:
 On Thu, Aug 27, 2009 at 01:54:47AM -0400, Mike Frysinger wrote:
  On Thursday 27 August 2009 01:11:32 Stefan Roese wrote:
   On Thursday 27 August 2009 03:55:57 Mike Frysinger wrote:
 +int phy_init(void)
 +{
 + /* Initialize the list */
 + INIT_LIST_HEAD(phy_drvs.list);
   
does it really need to be dynamic ?  arent there static initializers so
this can be done in .data ?
  
   Using static initializers fails on PPC because of the somewhat broken
   relocation. So this dynamic implementation is the preferred version, at
   least for PPC people.
  
  so the ppc port cant use the .data section at all ?  that doesnt sound 
  correct 
  to me.
 
 We can't have initialized pointers in .data without manual runtime fixup. 
 Only .text has relocation information.
 
 Someone tried to get proper relocation working a while ago, but ran into
 toolchain bugs.  Maybe current toolchains are better...

X-ES's board's in U-Boot fully relocate to SDRAM with the
CONFIG_RELOC_FIXUP_WORKS define and -mrelocatable compiler flag.  This
feature has worked with gcc-4.3.1/binutils-2.18.90 and
gcc-4.2.2/binutils-2.17.50.

Does anyone have a specific example of a toolchain that doesn't work?
It'd be great to get the issue figured out and get rid of all those
gd-reloc_off references that currently litter the code...

Peter

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


Re: [U-Boot] [RFC PATCH 2/3] net: add phylib implementation

2009-08-27 Thread Scott Wood
Mike Frysinger wrote:
 On Thursday 27 August 2009 11:38:07 Scott Wood wrote:
 On Thu, Aug 27, 2009 at 01:54:47AM -0400, Mike Frysinger wrote:
 On Thursday 27 August 2009 01:11:32 Stefan Roese wrote:
 On Thursday 27 August 2009 03:55:57 Mike Frysinger wrote:
 +int phy_init(void)
 +{
 +/* Initialize the list */
 +INIT_LIST_HEAD(phy_drvs.list);
 does it really need to be dynamic ?  arent there static initializers
 so this can be done in .data ?
 Using static initializers fails on PPC because of the somewhat broken
 relocation. So this dynamic implementation is the preferred version, at
 least for PPC people.
 so the ppc port cant use the .data section at all ?  that doesnt sound
 correct to me.
 We can't have initialized pointers in .data without manual runtime fixup.
 Only .text has relocation information.
 
 we're talking about a list structure and its initialized state, it isnt 
 pointing to anything.

It's pointing to itself.

-Scott
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


Re: [U-Boot] [RFC PATCH 2/3] net: add phylib implementation

2009-08-26 Thread Mike Frysinger
On Wednesday 26 August 2009 16:13:02 Jean-Christophe PLAGNIOL-VILLARD wrote:
 the current implementation will allow you to probe, instance and mananage
 one phy_device per mii_device

instance - instantiate
mananage - manage

 --- /dev/null
 +++ b/drivers/net/phy/phylib.c
 @@ -0,0 +1,535 @@
 +/*
 + * drivers/net/phy/phy_device.c

needs updating

 +static int genphy_config_init(struct phy_device *phydev);

why does _init() have a static def in this C file but the other genhy_config_* 
funcs have a non-static def in the H file

 +int phy_driver_register(struct phy_driver *phydrv)
 +{
 + if(!phydrv)

missing space

 +int phy_init(void)
 +{
 + /* Initialize the list */
 + INIT_LIST_HEAD(phy_drvs.list);

does it really need to be dynamic ?  arent there static initializers so this 
can be done in .data ?

 + phydev = calloc(1, sizeof(struct phy_device));

this is odd style.  why dont we stop jerkin around in all our code and 
introduce zalloc() already.  the first patch in this series has malloc/memset 
usage too.

 +static int phy_connect(struct mii_device *miidev, int phy_addr)
 +{
 + unsigned int phyaddr = (phy_addr + 1u) % 32u;

what's this business all about ?  why wrap the phy addr ?

 + printf(%s found at 0x%x\n, phydev-phydrv-name, phyaddr);

0x%x - %#x

 + for (i = 0; i  32; i++) {

i'd stick a comment here about mii bus being limited to 7 bits as not everyone 
knows it

 + if(!phy_connect(miidev, phy_addr))
 + if(phydrv-config_init)

missing space after the if

 + printf(%dMbps %s duplex link detected\n, phydev-speed,
 + phydev-duplex? full : half);

need space before the ?

 + } else {
 + printf(*Warning* no link detected\n);
 + }
 +
 + return 0;

should no link really return success ?

 + if (phydev-duplex == DUPLEX_FULL){

need space before that {

 + result = genphy_config_advert(phydev);
 +
 + if (result  0) /* error */
 + return result;

the error comment seems kind of useless

 + /* Only restart aneg if we are advertising something different
 +  * than we were before.  */

embedded tab at the end of the comment doesnt belong

 + now = get_timer(0);
 + while (get_timer(now)  PHY_AN_TIMEOUT) {

kind of useless to check the timer right away.  please convert to a do {...} 
while rather than a while {...}.

 + puts(PHY remote fault detected\n);
 + /* Restart auto-negotiation */
 + puts(PHY restarting auto-negotiation\n);

combine the puts()

 +#define ETHTOOL_GSET 0x0001 /* Get settings. */

i dont think we need all this ETHTOOL_xxx junk

 +/* compatibility with older code */
 +#define SPARC_ETH_GSET   ETHTOOL_GSET
 +#define SPARC_ETH_SSET   ETHTOOL_SSET

or this

 +/* Wake-On-Lan options. */
 +#define WAKE_PHY (1  0)
 +#define WAKE_UCAST   (1  1)
 +#define WAKE_MCAST   (1  2)
 +#define WAKE_BCAST   (1  3)
 +#define WAKE_ARP (1  4)
 +#define WAKE_MAGIC   (1  5)
 +#define WAKE_MAGICSECURE (1  6) /* only meaningful if WAKE_MAGIC */

does WOL really make sense in u-boot ?
-mike


signature.asc
Description: This is a digitally signed message part.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


Re: [U-Boot] [RFC PATCH 2/3] net: add phylib implementation

2009-08-26 Thread Stefan Roese
On Thursday 27 August 2009 03:55:57 Mike Frysinger wrote:
  +int phy_init(void)
  +{
  +   /* Initialize the list */
  +   INIT_LIST_HEAD(phy_drvs.list);

 does it really need to be dynamic ?  arent there static initializers so
 this can be done in .data ?

Using static initializers fails on PPC because of the somewhat broken 
relocation. So this dynamic implementation is the preferred version, at least 
for PPC people.

Cheers,
Stefan

--
DENX Software Engineering GmbH,  MD: Wolfgang Denk  Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-0 Fax: (+49)-8142-66989-80 Email: off...@denx.de
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


Re: [U-Boot] [RFC PATCH 2/3] net: add phylib implementation

2009-08-26 Thread Mike Frysinger
On Thursday 27 August 2009 01:11:32 Stefan Roese wrote:
 On Thursday 27 August 2009 03:55:57 Mike Frysinger wrote:
   +int phy_init(void)
   +{
   + /* Initialize the list */
   + INIT_LIST_HEAD(phy_drvs.list);
 
  does it really need to be dynamic ?  arent there static initializers so
  this can be done in .data ?

 Using static initializers fails on PPC because of the somewhat broken
 relocation. So this dynamic implementation is the preferred version, at
 least for PPC people.

so the ppc port cant use the .data section at all ?  that doesnt sound correct 
to me.
-mike


signature.asc
Description: This is a digitally signed message part.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot