[U-Boot] eth/net driver documentation

[U-Boot] eth/net driver documentation

[Mike Frysinger]

is there any documentation that covers the NET_MULTI driver stack ?  i.e. if  
i was a network chip vendor and wanted to write a u-boot driver for my part, 
where would i look ?  or is it a matter of "the code is the documentation" ?  
i can start a small doc that covers the existing stack if that is the case ... 
more to keep my sanity so i dont have to keep going through the ethernet stack 
to remind myself of how things fit together ;).
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20090715/8eeb9f58/attachment.pgp 

[U-Boot] eth/net driver documentation

[Ben Warren]

Hi Mike,
Mike Frysinger wrote:
> is there any documentation that covers the NET_MULTI driver stack ?  i.e. if  
> i was a network chip vendor and wanted to write a u-boot driver for my part, 
> where would i look ?  or is it a matter of "the code is the documentation" ?  
> i can start a small doc that covers the existing stack if that is the case ... 
> more to keep my sanity so i dont have to keep going through the ethernet stack 
> to remind myself of how things fit together ;).
> -mike
>   
Isn't the code nicely self-documenting? :)  I think yours is a great 
idea.  It's a pretty simple interface, but not necessarily intuitive.  
I'll be glad to help if needed.

regards,
Ben

[U-Boot] [PATCH 1/2] net: rename NetRxPkt to NetRxPacket

[Mike Frysinger]

The net code is mostly consistent in using 'Packet' rather than 'Pkt', so
rename the minor detractor to follow suite.

Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
 include/net.h |    4 ++--
 net/bootp.c   |    2 +-
 net/net.c     |    8 ++++----
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/include/net.h b/include/net.h
index 5a1d36e..88a9513 100644
--- a/include/net.h
+++ b/include/net.h
@@ -331,8 +331,8 @@ extern IPaddr_t		NetOurIP;		/* Our    IP addr (0 = unknown)	*/
 extern IPaddr_t		NetServerIP;		/* Server IP addr (0 = unknown)	*/
 extern volatile uchar * NetTxPacket;		/* THE transmit packet		*/
 extern volatile uchar * NetRxPackets[PKTBUFSRX];/* Receive packets		*/
-extern volatile uchar * NetRxPkt;		/* Current receive packet	*/
-extern int		NetRxPktLen;		/* Current rx packet length	*/
+extern volatile uchar * NetRxPacket;		/* Current receive packet	*/
+extern int		NetRxPacketLen;		/* Current rx packet length	*/
 extern unsigned		NetIPID;		/* IP ID (counting)		*/
 extern uchar		NetBcastAddr[6];	/* Ethernet boardcast address	*/
 extern uchar		NetEtherNullAddr[6];
diff --git a/net/bootp.c b/net/bootp.c
index 77057c6..d5f9c4b 100644
--- a/net/bootp.c
+++ b/net/bootp.c
@@ -124,7 +124,7 @@ static void BootpCopyNetParams(Bootp_t *bp)
 	NetCopyIP(&tmp_ip, &bp->bp_siaddr);
 	if (tmp_ip != 0)
 		NetCopyIP(&NetServerIP, &bp->bp_siaddr);
-	memcpy (NetServerEther, ((Ethernet_t *)NetRxPkt)->et_src, 6);
+	memcpy (NetServerEther, ((Ethernet_t *)NetRxPacket)->et_src, 6);
 #endif
 	if (strlen(bp->bp_file) > 0)
 		copy_filename (BootFile, bp->bp_file, sizeof(BootFile));
diff --git a/net/net.c b/net/net.c
index 5637cf5..e215fd8 100644
--- a/net/net.c
+++ b/net/net.c
@@ -139,8 +139,8 @@ uchar		NetServerEther[6] =	/* Boot server enet address		*/
 			{ 0, 0, 0, 0, 0, 0 };
 IPaddr_t	NetOurIP;		/* Our IP addr (0 = unknown)		*/
 IPaddr_t	NetServerIP;		/* Server IP addr (0 = unknown)		*/
-volatile uchar *NetRxPkt;		/* Current receive packet		*/
-int		NetRxPktLen;		/* Current rx packet length		*/
+volatile uchar *NetRxPacket;		/* Current receive packet		*/
+int		NetRxPacketLen;		/* Current rx packet length		*/
 unsigned	NetIPID;		/* IP packet ID				*/
 uchar		NetBcastAddr[6] =	/* Ethernet bcast address		*/
 			{ 0xff, 0xff, 0xff, 0xff, 0xff, 0xff };
@@ -1122,8 +1122,8 @@ NetReceive(volatile uchar * inpkt, int len)
 	printf("packet received\n");
 #endif
 
-	NetRxPkt = inpkt;
-	NetRxPktLen = len;
+	NetRxPacket = inpkt;
+	NetRxPacketLen = len;
 	et = (Ethernet_t *)inpkt;
 
 	/* too small packet? */
-- 
1.6.3.3

[U-Boot] [PATCH 2/2] document network driver framework

[Mike Frysinger]

Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
Ben: some things to note:
	- i adopted Jean's proposed naming scheme in the CONFIG section
	- i deprecated calling the driver-specific entry point
	  "xxx_initialization()" in favor of "xxx_register()" because the
	  former is way too confusing with everyone also having "xxx_init()"

 doc/README.drivers.eth |  199 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 199 insertions(+), 0 deletions(-)
 create mode 100644 doc/README.drivers.eth

diff --git a/doc/README.drivers.eth b/doc/README.drivers.eth
new file mode 100644
index 0000000..00b4eb1
--- /dev/null
+++ b/doc/README.drivers.eth
@@ -0,0 +1,199 @@
+-----------------------
+ Ethernet Driver Guide
+-----------------------
+
+The networking stack in Das U-Boot is designed for multiple network devices
+to be easily added and controlled at runtime.  This guide is meant for people
+who wish to review the net driver stack with an eye towards implementing your
+own ethernet device driver.  Here we will describe a new pseudo 'APE' driver.
+
+----------------
+ CONFIG Options
+----------------
+
+The common config defines your device should respect (if applicable):
+	CONFIG_MII  - configure in MII mode
+	CONFIG_RMII - configure in RMII mode
+
+	CONFIG_CMD_MII - support the MII command
+
+If you want to add config options specific to your driver, you should use the
+naming convention:
+	CONFIG_NET_<DRIVERNAME>_<OPTION>
+For example, if the APE driver could operate in either 16bit or 32bit mode,
+there would probably be the option:
+	CONFIG_NET_APE_32BIT - operate in 32bit mode rather than 16bit default
+
+And to control whether your driver is even enabled, you should use:
+	CONFIG_SYS_NET_<DRIVERNAME>
+So the APE driver would look like:
+	CONFIG_SYS_NET_APE
+
+------------------
+ Driver Functions
+------------------
+
+All functions you will be implementing in this document have the return value
+meaning of 0 for success and non-zero for failure.
+
+ ----------
+  Register
+ ----------
+
+When U-Boot initializes, it will call the common function eth_initialize().
+This will in turn call the board-specific board_eth_init() (or if that fails,
+the cpu-specific cpu_eth_init()).  These board-specific functions can do random
+system handling, but ultimately they will call the driver-specific register
+function which in turn takes care of initializing that particular instance.
+
+Keep in mind that you should code the driver to avoid storing state in global
+data as someone might want to hook up two of the same devices to one board.  If
+the state is maintained as global data, it makes using both of those devices
+impossible.
+
+So the call graph at this stage would look something like:
+board_init()
+	eth_initialize()
+		board_eth_init() / cpu_eth_init()
+			driver_register()
+				initialize eth_device
+				eth_register()
+
+At this point in time, the only thing you need to worry about is the driver's
+register function.  The pseudo code would look something like:
+int ape_register(bd_t *bis, int iobase)
+{
+	struct ape_priv *priv;
+	struct eth_device *dev;
+
+	priv = malloc(sizeof(*priv));
+	if (priv == NULL)
+		return 1;
+
+	dev = malloc(sizeof(*dev));
+	if (dev == NULL) {
+		free(priv);
+		return 1;
+	}
+
+	/* setup whatever private state you need */
+
+	memset(dev, 0, sizeof(*dev));
+	sprintf(dev->name, "APE");
+
+	/* if your device has dedicated hardware storage for the
+	 * MAC, read it and initialize dev->enetaddr with it
+	 */
+	ape_mac_read(dev->enetaddr);
+
+	dev->iobase = iobase;
+	dev->priv = priv;
+	dev->init = ape_init;
+	dev->halt = ape_halt;
+	dev->send = ape_send;
+	dev->recv = ape_recv;
+
+	eth_register(dev);
+
+#ifdef CONFIG_CMD_MII)
+	miiphy_register(dev->name, ape_mii_read, ape_mii_write);
+#endif
+
+	return 0;
+}
+
+The exact arguments needed to initialize your device are up to you.  If you
+need to pass more/less arguments, that's fine.  You should also add the
+prototype for your new register function to include/netdev.h.  You might notice
+that many drivers seem to use xxx_initialize() rather than xxx_register().
+This is the old naming convention and should be avoided as it causes confusion
+with the driver-specific init function.
+
+Other than locating the MAC address in dedicated hardware storage, you should
+not touch the hardware in anyway.  That step is handled in the driver-specific
+init function.  Remember that we are only registering the device here, we are
+not checking its state or doing random probing.
+
+ -----------
+  Callbacks
+ -----------
+
+Now that we've registered with the ethernet layer, we can start getting some
+real work done.  You will need four functions:
+	int ape_init(struct eth_device *dev, bd_t *bis);
+	int ape_send(struct eth_device *dev, volatile void *packet, int length);
+	int ape_recv(struct eth_device *dev);
+	int ape_halt(struct eth_device *dev);
+
+The init function checks the hardware (probing/identifying) and gets it ready
+for send/recv operations.  You often do things here such as resetting the MAC
+and/or PHY, and waiting for the link to autonegotiate.  You should also take
+the opportunity to program the device's MAC address with the dev->enetaddr
+member.  This allows the rest of U-Boot to dynamically change the MAC address
+and have the new settings be respected.
+
+The send function does what you think -- transmit the specified packet whose
+size is specified by length (in bytes).  You should not return until the
+transmission is complete, and you should leave the state such that the send
+function can be called multiple times in a row.
+
+The recv function should process packets as long as the hardware has them
+readily available before returning.  i.e. you should drain the hardware fifo.
+The common code sets up packet buffers for you already (NetRxPackets), so there
+is no need to allocate your own.  For each packet you receive, you should call
+the NetReceive() function on it with the packet length.  So the pseudo code
+here would look something like:
+int ape_recv(struct eth_device *dev)
+{
+	int length, i = 0;
+	...
+	while (packets_are_available()) {
+		...
+		length = ape_get_packet(&NetRxPackets[i]);
+		...
+		NetReceive(&NetRxPackets[i], length);
+		...
+		if (++i >= PKTBUFSRX)
+			i = 0;
+		...
+	}
+	...
+	return 0;
+}
+
+The halt function should turn off / disable the hardware and place it back in
+its reset state.
+
+So the call graph at this stage would look something like:
+some net operation (ping / tftp / whatever...)
+	eth_init()
+		dev->init()
+	eth_send()
+		dev->send()
+	eth_rx()
+		dev->recv()
+	eth_halt()
+		dev->halt()
+
+----------------
+ CONFIG_CMD_MII
+----------------
+
+If your device supports banging arbitrary values on the MII bus (pretty much
+every device does), you should add support for the mii command.  Doing so is
+fairly trivial and makes debugging mii issues a lot easier at runtime.
+
+After you have called eth_register() in your driver's register function, add
+a call to miiphy_register() like so:
+#ifdef CONFIG_CMD_MII
+	miiphy_register(dev->name, mii_read, mii_write);
+#endif
+
+And then define the mii_read and mii_write functions if you haven't already.
+Their syntax is straightforward:
+	int mii_read(char *devname, uchar addr, uchar reg, ushort *val);
+	int mii_write(char *devname, uchar addr, uchar reg, ushort val);
+
+The read function should read the register 'reg' from the phy at address 'addr'
+and store the result in the pointer 'val'.  The implementation for the write
+function should logically follow.
-- 
1.6.3.3

[U-Boot] [PATCH 1/2] net: rename NetRxPkt to NetRxPacket

[Ben Warren]

Mike,

Mike Frysinger wrote:
> The net code is mostly consistent in using 'Packet' rather than 'Pkt', so
> rename the minor detractor to follow suite.
>
> Signed-off-by: Mike Frysinger <vapier@gentoo.org>
> ---
>  include/net.h |    4 ++--
>  net/bootp.c   |    2 +-
>  net/net.c     |    8 ++++----
>  3 files changed, 7 insertions(+), 7 deletions(-)
>   
Applied to net repo.

thanks,
Ben

[U-Boot] [PATCH 2/2] document network driver framework

[Ben Warren]

Mike Frysinger wrote:
> Signed-off-by: Mike Frysinger <vapier@gentoo.org>
> ---
> Ben: some things to note:
> 	- i adopted Jean's proposed naming scheme in the CONFIG section
>   
Is this a generally-accepted naming convention?  I personally think it's 
crap, and since there isn't a single driver that uses it yet, you might 
say this is a bit ahead of the curve.
> 	- i deprecated calling the driver-specific entry point
> 	  "xxx_initialization()" in favor of "xxx_register()" because the
> 	  former is way too confusing with everyone also having "xxx_init()"
>   
That may be so, but since there isn't a single driver that uses this 
naming convention, you're wishing something that ain't so.

Other than that, nice writeup.  thanks!

Ben

[U-Boot] [PATCH 2/2] document network driver framework

[Mike Frysinger]

On Tuesday 21 July 2009 01:09:11 Ben Warren wrote:
> Mike Frysinger wrote:
> > Signed-off-by: Mike Frysinger <vapier@gentoo.org>
> > ---
> > Ben: some things to note:
> > 	- i adopted Jean's proposed naming scheme in the CONFIG section
>
> Is this a generally-accepted naming convention?  I personally think it's
> crap, and since there isn't a single driver that uses it yet, you might
> say this is a bit ahead of the curve.

some style needed to be suggested, and what Jean proposed is better than what 
we have today (which is nothing)

> > 	- i deprecated calling the driver-specific entry point
> > 	  "xxx_initialization()" in favor of "xxx_register()" because the
> > 	  former is way too confusing with everyone also having "xxx_init()"
>
> That may be so, but since there isn't a single driver that uses this
> naming convention, you're wishing something that ain't so.

that's why i said "should", deprecated current naming, and noted existing 
practice.  if you agree with the proposal, it's easy enough to run sed on a 
few files to fix one function name.  you agree with my comment that today's 
behavior is confusing even if you stare and bang on the code day in and day 
out ?  it's even worse for the occasional observer ...
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20090721/ac05a5f9/attachment.pgp 

[U-Boot] [PATCH 2/2] document network driver framework

[Wolfgang Denk]

Dear Ben Warren,

In message <4A654D77.4070901@gmail.com> you wrote:
> Mike Frysinger wrote:
> > Signed-off-by: Mike Frysinger <vapier@gentoo.org>
> > ---
> > Ben: some things to note:
> > 	- i adopted Jean's proposed naming scheme in the CONFIG section
> >   
> Is this a generally-accepted naming convention?  I personally think it's 
> crap, and since there isn't a single driver that uses it yet, you might 
> say this is a bit ahead of the curve.

No, this is NOT a generally-accepted naming convention. It has been
proposed, and rejected.

> That may be so, but since there isn't a single driver that uses this 
> naming convention, you're wishing something that ain't so.

Thanks for pointing out.

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 only thing necessary for the triumph of evil is for good  men  to
do nothing.                                            - Edmund Burke

[U-Boot] [PATCH 2/2] document network driver framework

[Wolfgang Denk]

Dear Mike Frysinger,

In message <200907210228.09882.vapier@gentoo.org> you wrote:
> >
> > Is this a generally-accepted naming convention?  I personally think it's
> > crap, and since there isn't a single driver that uses it yet, you might
> > say this is a bit ahead of the curve.
>
> some style needed to be suggested, and what Jean proposed is better than what 
> we have today (which is nothing)

Arent't we pretty much doing what Linux is doing here, too? I see lots
of XXX_init functions in the Linux network code, for example.

> that's why i said "should", deprecated current naming, and noted existing 
> practice.  if you agree with the proposal, it's easy enough to run sed on a  
> few files to fix one function name.  you agree with my comment that today's  
> behavior is confusing even if you stare and bang on the code day in and day  
> out ?  it's even worse for the occasional observer ...

Hm... renaming  something  from  "xxx_init()"  into  "xxx_register()"
because  other  code  is also also using "xxx_init()" does not really
make anything clearer to me. Actually IMO  it  just  adds  confusion,
because  if  other's  use  "xxx_init()" I'd expect from a consistence
point of view that we use "xxx_init()", too.

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 one who says it cannot be done should never interrupt the one who
is doing it.

[U-Boot] [PATCH 2/2] document network driver framework

[Mike Frysinger]

On Tuesday 21 July 2009 03:32:55 Wolfgang Denk wrote:
> Mike Frysinger wrote:
> > > Is this a generally-accepted naming convention?  I personally think
> > > it's crap, and since there isn't a single driver that uses it yet, you
> > > might say this is a bit ahead of the curve.
> >
> > some style needed to be suggested, and what Jean proposed is better than
> > what we have today (which is nothing)
>
> Arent't we pretty much doing what Linux is doing here, too? I see lots
> of XXX_init functions in the Linux network code, for example.
>
> > that's why i said "should", deprecated current naming, and noted existing
> > practice.  if you agree with the proposal, it's easy enough to run sed on
> > a few files to fix one function name.  you agree with my comment that
> > today's behavior is confusing even if you stare and bang on the code day
> > in and day out ?  it's even worse for the occasional observer ...
>
> Hm... renaming  something  from  "xxx_init()"  into  "xxx_register()"
> because  other  code  is also also using "xxx_init()" does not really
> make anything clearer to me. Actually IMO  it  just  adds  confusion,
> because  if  other's  use  "xxx_init()" I'd expect from a consistence
> point of view that we use "xxx_init()", too.

your reply reinforces my point.  i'm not talking about xxx_init(), i'm talking 
about xxx_initialize().  network drivers atm define both -- xxx_initialize() 
is to initialize the eth_driver structure and *register* with the eth layer, 
and xxx_init() to *initialize* the hardware.  i'm proposing renaming 
xxx_initialize() to xxx_register().
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20090721/e729fcc0/attachment-0001.pgp 

[U-Boot] [PATCH 2/2] document network driver framework

[Ben Warren]

Mike Frysinger wrote:
> On Tuesday 21 July 2009 03:32:55 Wolfgang Denk wrote:
>   
>> Mike Frysinger wrote:
>>     
>>>> Is this a generally-accepted naming convention?  I personally think
>>>> it's crap, and since there isn't a single driver that uses it yet, you
>>>> might say this is a bit ahead of the curve.
>>>>         
>>> some style needed to be suggested, and what Jean proposed is better than
>>> what we have today (which is nothing)
>>>       
>> Arent't we pretty much doing what Linux is doing here, too? I see lots
>> of XXX_init functions in the Linux network code, for example.
>>
>>     
>>> that's why i said "should", deprecated current naming, and noted existing
>>> practice.  if you agree with the proposal, it's easy enough to run sed on
>>> a few files to fix one function name.  you agree with my comment that
>>> today's behavior is confusing even if you stare and bang on the code day
>>> in and day out ?  it's even worse for the occasional observer ...
>>>       
>> Hm... renaming  something  from  "xxx_init()"  into  "xxx_register()"
>> because  other  code  is also also using "xxx_init()" does not really
>> make anything clearer to me. Actually IMO  it  just  adds  confusion,
>> because  if  other's  use  "xxx_init()" I'd expect from a consistence
>> point of view that we use "xxx_init()", too.
>>     
>
> your reply reinforces my point.  i'm not talking about xxx_init(), i'm talking 
> about xxx_initialize().  network drivers atm define both -- xxx_initialize() 
> is to initialize the eth_driver structure and *register* with the eth layer, 
> and xxx_init() to *initialize* the hardware.  i'm proposing renaming 
> xxx_initialize() to xxx_register().
> -mike
>   
I understand what you're saying, and  think in principle it's probably a 
good idea to rename to something other than xxx_initialize().  I just 
think a document that outlines best practices that are not in use *at 
all* seems a bit silly.

If we're going to go this way, IMHO we should change all function names 
at once.  It would be easy to do, but would be a huge, potentially 
intrusive patch that I'm not sure buys us much.

regards,
Ben

[U-Boot] [PATCH 2/2] document network driver framework

[Mike Frysinger]

On Tuesday 21 July 2009 16:55:34 Ben Warren wrote:
> Mike Frysinger wrote:
> > On Tuesday 21 July 2009 03:32:55 Wolfgang Denk wrote:
> >> Mike Frysinger wrote:
> >>>> Is this a generally-accepted naming convention?  I personally think
> >>>> it's crap, and since there isn't a single driver that uses it yet, you
> >>>> might say this is a bit ahead of the curve.
> >>>
> >>> some style needed to be suggested, and what Jean proposed is better
> >>> than what we have today (which is nothing)
> >>
> >> Arent't we pretty much doing what Linux is doing here, too? I see lots
> >> of XXX_init functions in the Linux network code, for example.
> >>
> >>> that's why i said "should", deprecated current naming, and noted
> >>> existing practice.  if you agree with the proposal, it's easy enough to
> >>> run sed on a few files to fix one function name.  you agree with my
> >>> comment that today's behavior is confusing even if you stare and bang
> >>> on the code day in and day out ?  it's even worse for the occasional
> >>> observer ...
> >>
> >> Hm... renaming  something  from  "xxx_init()"  into  "xxx_register()"
> >> because  other  code  is also also using "xxx_init()" does not really
> >> make anything clearer to me. Actually IMO  it  just  adds  confusion,
> >> because  if  other's  use  "xxx_init()" I'd expect from a consistence
> >> point of view that we use "xxx_init()", too.
> >
> > your reply reinforces my point.  i'm not talking about xxx_init(), i'm
> > talking about xxx_initialize().  network drivers atm define both --
> > xxx_initialize() is to initialize the eth_driver structure and *register*
> > with the eth layer, and xxx_init() to *initialize* the hardware.  i'm
> > proposing renaming xxx_initialize() to xxx_register().
>
> I understand what you're saying, and  think in principle it's probably a
> good idea to rename to something other than xxx_initialize().  I just
> think a document that outlines best practices that are not in use *at
> all* seems a bit silly.

considering the document makes note of existing practice and suggests the new 
naming schema, i think it's fine and shouldnt hold up merging.

> If we're going to go this way, IMHO we should change all function names
> at once.  It would be easy to do, but would be a huge, potentially
> intrusive patch that I'm not sure buys us much.

how quickly we convert older drivers doesnt matter to me, but if you prefer 
sooner rather than later, that's easy enough to do and hand off to Wolfgang 
for the first patch in "next".
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20090721/8d30c793/attachment.pgp 

[U-Boot] [PATCH 2/2] document network driver framework

[Andy Fleming]

On Sat, Jul 18, 2009 at 8:04 PM, Mike Frysinger <vapier@gentoo.org> wrote:

> Signed-off-by: Mike Frysinger <vapier@gentoo.org>
> ---
> Ben: some things to note:
>        - i adopted Jean's proposed naming scheme in the CONFIG section
>        - i deprecated calling the driver-specific entry point
>          "xxx_initialization()" in favor of "xxx_register()" because the
>          former is way too confusing with everyone also having "xxx_init()"
>
>  doc/README.drivers.eth |  199
> ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 199 insertions(+), 0 deletions(-)
>  create mode 100644 doc/README.drivers.eth
>
> diff --git a/doc/README.drivers.eth b/doc/README.drivers.eth
> new file mode 100644
> index 0000000..00b4eb1
> --- /dev/null
> +++ b/doc/README.drivers.eth
> @@ -0,0 +1,199 @@
> +-----------------------
> + Ethernet Driver Guide
> +-----------------------
> +
> +The networking stack in Das U-Boot is designed for multiple network
> devices
> +to be easily added and controlled at runtime.  This guide is meant for
> people
> +who wish to review the net driver stack with an eye towards implementing
> your
> +own ethernet device driver.  Here we will describe a new pseudo 'APE'
> driver.
> +
> +----------------
> + CONFIG Options
> +----------------
> +
> +The common config defines your device should respect (if applicable):
> +       CONFIG_MII  - configure in MII mode
> +       CONFIG_RMII - configure in RMII mode



That's awfully global, and inflexible.  I don't think there's any convention
here that should be encouraged.  The data bus configuration is a per-device
attribute.  If some system architects choose to allow this into their config
files, far be it from me to protest, but I'm against it.

Also, CONFIG_MII doesn't mean what you think it means.  It stems from the
obnoxious overlap in terms set forth by IEEE 802.3.  The MII in this case
refers to the MII Management bus.  Defining CONFIG_MII will cause
miiphyutil.c to be built, which is a bit-bang MII Management bus driver.  It
will also enable some MII Management features.  CONFIG_MII is therefore a
relevant CONFIG option for most drivers.

Andy

[U-Boot] [PATCH 2/2] document network driver framework

[Mike Frysinger]

On Wednesday 22 July 2009 19:00:40 Andy Fleming wrote:
> On Sat, Jul 18, 2009 at 8:04 PM, Mike Frysinger <vapier@gentoo.org> wrote:
> > +----------------
> > + CONFIG Options
> > +----------------
> > +
> > +The common config defines your device should respect (if applicable):
> > +       CONFIG_MII  - configure in MII mode
> > +       CONFIG_RMII - configure in RMII mode
>
> That's awfully global, and inflexible.  I don't think there's any
> convention here that should be encouraged.  The data bus configuration is a
> per-device attribute.  If some system architects choose to allow this into
> their config files, far be it from me to protest, but I'm against it.

feel free to fix it then.  this is a case of me documenting existing practice 
and not feeling like doing the footwork to fix drivers.  you can not like it 
all you want, but that doesnt change reality.

> Also, CONFIG_MII doesn't mean what you think it means.  It stems from the
> obnoxious overlap in terms set forth by IEEE 802.3.  The MII in this case
> refers to the MII Management bus.  Defining CONFIG_MII will cause
> miiphyutil.c to be built, which is a bit-bang MII Management bus driver. 
> It will also enable some MII Management features.  CONFIG_MII is therefore
> a relevant CONFIG option for most drivers.

yes, that's obvious, so i'm not sure why i missed it (especially considering i 
also wrote support for it in the Blackfin net driver).  i'll update the doc.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20090722/8996d90e/attachment.pgp 

[U-Boot] [PATCH 2/2] document network driver framework

[Jean-Christophe PLAGNIOL-VILLARD]

On 21:04 Sat 18 Jul     , Mike Frysinger wrote:
> Signed-off-by: Mike Frysinger <vapier@gentoo.org>
> ---
> Ben: some things to note:
> 	- i adopted Jean's proposed naming scheme in the CONFIG section
> 	- i deprecated calling the driver-specific entry point
> 	  "xxx_initialization()" in favor of "xxx_register()" because the
> 	  former is way too confusing with everyone also having "xxx_init()"
> 
>  doc/README.drivers.eth |  199 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 199 insertions(+), 0 deletions(-)
>  create mode 100644 doc/README.drivers.eth
> 
> diff --git a/doc/README.drivers.eth b/doc/README.drivers.eth
> new file mode 100644
> index 0000000..00b4eb1
> --- /dev/null
> +++ b/doc/README.drivers.eth
> @@ -0,0 +1,199 @@
> +-----------------------
> + Ethernet Driver Guide
> +-----------------------
> +
> +The networking stack in Das U-Boot is designed for multiple network devices
> +to be easily added and controlled at runtime.  This guide is meant for people
> +who wish to review the net driver stack with an eye towards implementing your
> +own ethernet device driver.  Here we will describe a new pseudo 'APE' driver.
> +
> +----------------
> + CONFIG Options
> +----------------
> +
> +The common config defines your device should respect (if applicable):
> +	CONFIG_MII  - configure in MII mode
> +	CONFIG_RMII - configure in RMII mode
how will you deal you on your board you have a MII and a RMII chip used
I think need to be configure via drivers specifc CONFIG if your driver can not
deal with 2 different instance config or via driver init param
> +
> +	CONFIG_CMD_MII - support the MII command
> +
> +If you want to add config options specific to your driver, you should use the
> +naming convention:
> +	CONFIG_NET_<DRIVERNAME>_<OPTION>
> +For example, if the APE driver could operate in either 16bit or 32bit mode,
> +there would probably be the option:
> +	CONFIG_NET_APE_32BIT - operate in 32bit mode rather than 16bit default
> +
> +And to control whether your driver is even enabled, you should use:
> +	CONFIG_SYS_NET_<DRIVERNAME>
In my proposition CONFIG_SYS_ will be not use for driver enabling but for
hardware specific information
> +So the APE driver would look like:
> +	CONFIG_SYS_NET_APE
so it will be
	CONFIG_NET_APE
> +
> +------------------
> + Driver Functions
> +------------------
> +
> +All functions you will be implementing in this document have the return value
> +meaning of 0 for success and non-zero for failure.
> +
<snip>
> +----------------
> + CONFIG_CMD_MII
> +----------------
> +
> +If your device supports banging arbitrary values on the MII bus (pretty much
> +every device does), you should add support for the mii command.  Doing so is
> +fairly trivial and makes debugging mii issues a lot easier at runtime.
> +
> +After you have called eth_register() in your driver's register function, add
> +a call to miiphy_register() like so:
> +#ifdef CONFIG_CMD_MII
> +	miiphy_register(dev->name, mii_read, mii_write);
> +#endif
> +
> +And then define the mii_read and mii_write functions if you haven't already.
> +Their syntax is straightforward:
> +	int mii_read(char *devname, uchar addr, uchar reg, ushort *val);
> +	int mii_write(char *devname, uchar addr, uchar reg, ushort val);
> +
> +The read function should read the register 'reg' from the phy at address 'addr'
> +and store the result in the pointer 'val'.  The implementation for the write
> +function should logically follow.
we will rewrite this api when we will push the phylib

Best Regards,
J.

[U-Boot] [PATCH 2/2] document network driver framework

[Wolfgang Denk]

Dear Jean-Christophe PLAGNIOL-VILLARD,

In message <20090723214946.GE9480@game.jcrosoft.org> you wrote:
>
> > +And to control whether your driver is even enabled, you should use:
> > +	CONFIG_SYS_NET_<DRIVERNAME>
> In my proposition CONFIG_SYS_ will be not use for driver enabling but for
> hardware specific information

Enabling or disabling a driver i ssomething that a dumb user can do,
so it should be done using

	CONFIG_<DRIVERNAME>

as in CONFIG_E1000, CONFIG_EEPRO100, etc.

> > +So the APE driver would look like:
> > +	CONFIG_SYS_NET_APE
> so it will be
> 	CONFIG_NET_APE

No. It should be CONFIG_APE


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
Niklaus Wirth has lamented that, whereas Europeans pronounce his name
correctly (Ni-klows Virt), Americans invariably mangle it into (Nick-
les Worth). Which is to say that Europeans  call  him  by  name,  but
Americans call him by value.

[U-Boot] [PATCH 2/2] document network driver framework

[Ben Warren]

Wolfgang Denk wrote:
> Dear Jean-Christophe PLAGNIOL-VILLARD,
>
> In message <20090723214946.GE9480@game.jcrosoft.org> you wrote:
>   
>>> +And to control whether your driver is even enabled, you should use:
>>> +	CONFIG_SYS_NET_<DRIVERNAME>
>>>       
>> In my proposition CONFIG_SYS_ will be not use for driver enabling but for
>> hardware specific information
>>     
>
> Enabling or disabling a driver i ssomething that a dumb user can do,
> so it should be done using
>
> 	CONFIG_<DRIVERNAME>
>
> as in CONFIG_E1000, CONFIG_EEPRO100, etc.
>
>   
Fine with me, although I understand the argument that it doesn't convey 
enough information, especially when dealing with devices that are more 
obscure than Intel E1000.  CONFIG_SYS_APE is definitely wrong, I think 
CONFIG_NET_APE should be for networking features such as selectable 
protocols.  Maybe a compromise if one is needed would be 
CONFIG_NETDEV_APE ?  Or is it too verbose?

regards,
Ben

[U-Boot] [PATCH 2/2] document network driver framework

[Wolfgang Denk]

Dear Ben Warren,

In message <4A68E0C1.2000605@gmail.com> you wrote:
>
> > Enabling or disabling a driver i ssomething that a dumb user can do,
> > so it should be done using
> >
> > 	CONFIG_<DRIVERNAME>
> >
> > as in CONFIG_E1000, CONFIG_EEPRO100, etc.
> >
> >   
> Fine with me, although I understand the argument that it doesn't convey 
> enough information, especially when dealing with devices that are more 
> obscure than Intel E1000.  CONFIG_SYS_APE is definitely wrong, I think 

I understand the argument, too.

But then I find it extremely useful to use the very same config names
as Linux, and Linux uses CONFIG_E100, CONFIG_NATSEMI, CONFIG_NE2K_PCI,
CONFIG_8139CP, CONFIG_TLAN, CONFIG_ATP, CONFIG_E1000, ...

> CONFIG_NET_APE should be for networking features such as selectable 
> protocols.  Maybe a compromise if one is needed would be 
> CONFIG_NETDEV_APE ?  Or is it too verbose?

I think we should remain compatible with Linux here, too.

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
Little known fact about Middle Earth:   The Hobbits had a very sophi-
sticated computer network!   It was a Tolkien Ring...

[U-Boot] [PATCH v2] document network driver framework

[Mike Frysinger]

Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
v2
	- drop CONFIG naming section
	- fix MII documentation

 doc/README.drivers.eth |  177 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 177 insertions(+), 0 deletions(-)
 create mode 100644 doc/README.drivers.eth

diff --git a/doc/README.drivers.eth b/doc/README.drivers.eth
new file mode 100644
index 0000000..7f21909
--- /dev/null
+++ b/doc/README.drivers.eth
@@ -0,0 +1,177 @@
+-----------------------
+ Ethernet Driver Guide
+-----------------------
+
+The networking stack in Das U-Boot is designed for multiple network devices
+to be easily added and controlled at runtime.  This guide is meant for people
+who wish to review the net driver stack with an eye towards implementing your
+own ethernet device driver.  Here we will describe a new pseudo 'APE' driver.
+
+------------------
+ Driver Functions
+------------------
+
+All functions you will be implementing in this document have the return value
+meaning of 0 for success and non-zero for failure.
+
+ ----------
+  Register
+ ----------
+
+When U-Boot initializes, it will call the common function eth_initialize().
+This will in turn call the board-specific board_eth_init() (or if that fails,
+the cpu-specific cpu_eth_init()).  These board-specific functions can do random
+system handling, but ultimately they will call the driver-specific register
+function which in turn takes care of initializing that particular instance.
+
+Keep in mind that you should code the driver to avoid storing state in global
+data as someone might want to hook up two of the same devices to one board.  If
+the state is maintained as global data, it makes using both of those devices
+impossible.
+
+So the call graph at this stage would look something like:
+board_init()
+	eth_initialize()
+		board_eth_init() / cpu_eth_init()
+			driver_register()
+				initialize eth_device
+				eth_register()
+
+At this point in time, the only thing you need to worry about is the driver's
+register function.  The pseudo code would look something like:
+int ape_register(bd_t *bis, int iobase)
+{
+	struct ape_priv *priv;
+	struct eth_device *dev;
+
+	priv = malloc(sizeof(*priv));
+	if (priv == NULL)
+		return 1;
+
+	dev = malloc(sizeof(*dev));
+	if (dev == NULL) {
+		free(priv);
+		return 1;
+	}
+
+	/* setup whatever private state you need */
+
+	memset(dev, 0, sizeof(*dev));
+	sprintf(dev->name, "APE");
+
+	/* if your device has dedicated hardware storage for the
+	 * MAC, read it and initialize dev->enetaddr with it
+	 */
+	ape_mac_read(dev->enetaddr);
+
+	dev->iobase = iobase;
+	dev->priv = priv;
+	dev->init = ape_init;
+	dev->halt = ape_halt;
+	dev->send = ape_send;
+	dev->recv = ape_recv;
+
+	eth_register(dev);
+
+#ifdef CONFIG_CMD_MII)
+	miiphy_register(dev->name, ape_mii_read, ape_mii_write);
+#endif
+
+	return 0;
+}
+
+The exact arguments needed to initialize your device are up to you.  If you
+need to pass more/less arguments, that's fine.  You should also add the
+prototype for your new register function to include/netdev.h.  You might notice
+that many drivers seem to use xxx_initialize() rather than xxx_register().
+This is the old naming convention and should be avoided as it causes confusion
+with the driver-specific init function.
+
+Other than locating the MAC address in dedicated hardware storage, you should
+not touch the hardware in anyway.  That step is handled in the driver-specific
+init function.  Remember that we are only registering the device here, we are
+not checking its state or doing random probing.
+
+ -----------
+  Callbacks
+ -----------
+
+Now that we've registered with the ethernet layer, we can start getting some
+real work done.  You will need four functions:
+	int ape_init(struct eth_device *dev, bd_t *bis);
+	int ape_send(struct eth_device *dev, volatile void *packet, int length);
+	int ape_recv(struct eth_device *dev);
+	int ape_halt(struct eth_device *dev);
+
+The init function checks the hardware (probing/identifying) and gets it ready
+for send/recv operations.  You often do things here such as resetting the MAC
+and/or PHY, and waiting for the link to autonegotiate.  You should also take
+the opportunity to program the device's MAC address with the dev->enetaddr
+member.  This allows the rest of U-Boot to dynamically change the MAC address
+and have the new settings be respected.
+
+The send function does what you think -- transmit the specified packet whose
+size is specified by length (in bytes).  You should not return until the
+transmission is complete, and you should leave the state such that the send
+function can be called multiple times in a row.
+
+The recv function should process packets as long as the hardware has them
+readily available before returning.  i.e. you should drain the hardware fifo.
+The common code sets up packet buffers for you already (NetRxPackets), so there
+is no need to allocate your own.  For each packet you receive, you should call
+the NetReceive() function on it with the packet length.  So the pseudo code
+here would look something like:
+int ape_recv(struct eth_device *dev)
+{
+	int length, i = 0;
+	...
+	while (packets_are_available()) {
+		...
+		length = ape_get_packet(&NetRxPackets[i]);
+		...
+		NetReceive(&NetRxPackets[i], length);
+		...
+		if (++i >= PKTBUFSRX)
+			i = 0;
+		...
+	}
+	...
+	return 0;
+}
+
+The halt function should turn off / disable the hardware and place it back in
+its reset state.
+
+So the call graph at this stage would look something like:
+some net operation (ping / tftp / whatever...)
+	eth_init()
+		dev->init()
+	eth_send()
+		dev->send()
+	eth_rx()
+		dev->recv()
+	eth_halt()
+		dev->halt()
+
+-----------------------------
+ CONFIG_MII / CONFIG_CMD_MII
+-----------------------------
+
+If your device supports banging arbitrary values on the MII bus (pretty much
+every device does), you should add support for the mii command.  Doing so is
+fairly trivial and makes debugging mii issues a lot easier at runtime.
+
+After you have called eth_register() in your driver's register function, add
+a call to miiphy_register() like so:
+#if defined(CONFIG_MII) || defined(CONFIG_CMD_MII)
+	miiphy_register(dev->name, mii_read, mii_write);
+#endif
+
+And then define the mii_read and mii_write functions if you haven't already.
+Their syntax is straightforward:
+	int mii_read(char *devname, uchar addr, uchar reg, ushort *val);
+	int mii_write(char *devname, uchar addr, uchar reg, ushort val);
+
+The read function should read the register 'reg' from the phy at address 'addr'
+and store the result in the pointer 'val'.  The implementation for the write
+function should logically follow.
-- 
1.6.4.2

[U-Boot] [PATCH v2] document network driver framework

[Wolfgang Denk]

Dear Mike Frysinger,

In message <1252521682-5067-1-git-send-email-vapier@gentoo.org> you wrote:
> Signed-off-by: Mike Frysinger <vapier@gentoo.org>
> ---
> v2
> 	- drop CONFIG naming section
> 	- fix MII documentation
> 
>  doc/README.drivers.eth |  177 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 177 insertions(+), 0 deletions(-)
>  create mode 100644 doc/README.drivers.eth

Acked-by: Wolfgang Denk <wd@denx.de>

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
Virtue is a relative term.
	-- Spock, "Friday's Child", stardate 3499.1

[U-Boot] [PATCH v2] document network driver framework

[Wolfgang Denk]

Dear Ben,

In message <1252521682-5067-1-git-send-email-vapier@gentoo.org> Mike Frysinger wrote:
> Signed-off-by: Mike Frysinger <vapier@gentoo.org>
> ---
> v2
> 	- drop CONFIG naming section
> 	- fix MII documentation
> 
>  doc/README.drivers.eth |  177 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 177 insertions(+), 0 deletions(-)
>  create mode 100644 doc/README.drivers.eth

Will you run this through the net repo, or shall I pick it up
directly?

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
How many seconds are there in a year? If I tell you there are 3.155 x
10^7, you won't even try to remember it. On the other hand, who could
forget that, to within half a percent, pi seconds is a nanocentury.
                                                - Tom Duff, Bell Labs

[U-Boot] [PATCH v2] document network driver framework

[Ben Warren]

On Sun, Oct 4, 2009 at 12:12 PM, Wolfgang Denk <wd@denx.de> wrote:

> Dear Ben,
>
> In message <1252521682-5067-1-git-send-email-vapier@gentoo.org> Mike
> Frysinger wrote:
> > Signed-off-by: Mike Frysinger <vapier@gentoo.org>
> > ---
> > v2
> >       - drop CONFIG naming section
> >       - fix MII documentation
> >
> >  doc/README.drivers.eth |  177
> ++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 files changed, 177 insertions(+), 0 deletions(-)
> >  create mode 100644 doc/README.drivers.eth
>
> Will you run this through the net repo, or shall I pick it up
> directly?
>
> I'll take care of it.


> Best regards,
>
> Wolfgang Denk
>
>
regards,
Ben

[U-Boot] [PATCH v2] document network driver framework

[Ben Warren]

Hi Mike,

Mike Frysinger wrote:
> Signed-off-by: Mike Frysinger <vapier@gentoo.org>
> ---
> v2
> 	- drop CONFIG naming section
> 	- fix MII documentation
>
>  doc/README.drivers.eth |  177 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 177 insertions(+), 0 deletions(-)
>  create mode 100644 doc/README.drivers.eth
>   
Applied to net repo.

I'll add info about the return code of the initialize() functions in a 
separate patch.  It wouldn't be fair to ask you to do that.

thanks,
Ben