[U-Boot,RFC,1/1] doc: net: Rewrite network driver documentation

doc/README.drivers.eth seems like a good source for understanding
U-Boot's network subsystem, but is only talking about legacy network
drivers. This is particularly sad as proper documentation would help in
porting drivers over to the driver model.

Rewrite the document to describe network drivers in the new driver model
world. Most driver callbacks are almost identical in their semantic, but
recv() differs in some important details.

Also keep some parts of the original text at the end, to help
understanding old drivers. Add some hints on how to port drivers over.

This also uses the opportunity to reformat the document in reST.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 doc/README.drivers.eth | 438 ++++++++++++++++++++++++++++++-------------------
 1 file changed, 271 insertions(+), 167 deletions(-)

On 11/25/19 2:32 AM, Andre Przywara wrote:
> doc/README.drivers.eth seems like a good source for understanding
> U-Boot's network subsystem, but is only talking about legacy network
> drivers. This is particularly sad as proper documentation would help in
> porting drivers over to the driver model.
>
> Rewrite the document to describe network drivers in the new driver model
> world. Most driver callbacks are almost identical in their semantic, but
> recv() differs in some important details.
>
> Also keep some parts of the original text at the end, to help
> understanding old drivers. Add some hints on how to port drivers over.
>
> This also uses the opportunity to reformat the document in reST.

To me the whole document looks nice. I just have some remarks concerning
hooking the text up in the HTML documentation of U-Boot:

We want to be able to render the restructured text files with

     make htmldocs

Restructured text files should be named *.rst.

Would we move the file to doc/driver-model/eth.rst?

In this case we would hook up the file in doc/driver-model/index.rst.

It would be rendered like this:

https://www.xypron.de/u-boot/driver-model/eth.html

Should we remove all the overlines for the headings so that the text
looks more like the other rst files?

The priorities of underlines differ from other rst files. I suggest to
start with ===== followed by ------. So we have a thicker underline for
the H1 title than for the H2 titles when reading in a plain text editor.

Best regards

Heinrich

>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>   doc/README.drivers.eth | 438 ++++++++++++++++++++++++++++++-------------------
>   1 file changed, 271 insertions(+), 167 deletions(-)
>
> diff --git a/doc/README.drivers.eth b/doc/README.drivers.eth
> index 1a9a23b51b..d1920ee47d 100644
> --- a/doc/README.drivers.eth
> +++ b/doc/README.drivers.eth
> @@ -1,9 +1,3 @@
> -!!! WARNING !!!
> -
> -This guide describes to the old way of doing things. No new Ethernet drivers
> -should be implemented this way. All new drivers should be written against the
> -U-Boot core driver model. See doc/driver-model/README.txt
> -
>   -----------------------
>    Ethernet Driver Guide
>   -----------------------
> @@ -13,203 +7,313 @@ 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.
> +Most exisiting drivers do already - and new network driver MUST - use the
> +U-Boot core driver model. Generic information about this can be found in
> +doc/driver-model/design.rst, this document will thus focus on the network
> +specific code parts.
> +Some drivers are still using the old Ethernet interface, differences between
> +the two and hints about porting will be handled at the end.
> +
> +==================
> + Driver framework
> +==================
> +
> +A network driver following the driver model must declare itself using
> +the UCLASS_ETH .id field in the U-Boot driver struct:
> +
> +.. code-block:: c
> +
> +	U_BOOT_DRIVER(eth_ape) = {
> +		.name			= "eth_ape",
> +		.id			= UCLASS_ETH,
> +		.of_match		= eth_ape_ids,
> +		.ofdata_to_platdata	= eth_ape_ofdata_to_platdata,
> +		.probe			= eth_ape_probe,
> +		.ops			= &eth_ape_ops,
> +		.priv_auto_alloc_size	= sizeof(struct eth_ape_dev),
> +		.platdata_auto_alloc_size = sizeof(struct eth_ape_pdata),
> +		.flags			= DM_FLAG_ALLOC_PRIV_DMA,
> +	};
> +
> +struct eth_ape_dev contains runtime per-instance data, like buffers, pointers
> +to current descriptors, current speed settings, pointers to PHY related data
> +(like struct mii_dev) and so on. Declaring its size in .priv_auto_alloc_size
> +will let the driver framework allocate it at the right time.
> +It can be retrieved using a dev_get_priv(dev) call.
> +
> +struct eth_ape_pdata contains static platform data, like the MMIO base address,
> +a hardware variant, the MAC address. ``struct eth_pdata eth_pdata``
> +as the first member of this struct helps to avoid duplicated code.
> +If you don't need any more platform data beside the standard member,
> +just use sizeof(struct eth_pdata) for the platdata_auto_alloc_size.
> +
> +PCI devices add a line pointing to supported vendor/device ID pairs:
> +
> +.. code-block:: c
> +
> +	static struct pci_device_id supported[] = {
> +		{ PCI_DEVICE(PCI_VENDOR_ID_APE, 0x4223) },
> +		{}
> +	};
> +
> +	U_BOOT_PCI_DEVICE(eth_ape, supported);
> +
> +
> +Device probing and instantiation will be handled by the driver model framework,
> +so follow the guidelines there. The probe() function would initialise the
> +platform specific parts of the hardware, like clocks, resets, GPIOs, the MDIO
> +bus. Also it would take care of any special PHY setup (power rails, enable
> +bits for internal PHYs, etc.).
> +
> +====================
> + Callback functions
> +====================
> +
> +The real work will be done in the callback function the driver provides
> +by defining the members of struct eth_ops:
> +
> +.. code-block:: c
> +
> +	struct eth_ops {
> +		int (*start)(struct udevice *dev);
> +		int (*send)(struct udevice *dev, void *packet, int length);
> +		int (*recv)(struct udevice *dev, int flags, uchar **packetp);
> +		int (*free_pkt)(struct udevice *dev, uchar *packet, int length);
> +		void (*stop)(struct udevice *dev);
> +		int (*mcast)(struct udevice *dev, const u8 *enetaddr, int join);
> +		int (*write_hwaddr)(struct udevice *dev);
> +		int (*read_rom_hwaddr)(struct udevice *dev);
> +	};
> +
> +An up-to-date version of this struct together with more information can be
> +found in include/net.h.
> +
> +Only start, stop, send and recv are required, the rest is optional and will
> +be handled by generic code or ignored if not provided.
> +
> +The **start** function initialises the hardware 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 enetaddr member
> +of the generic struct eth_pdata (which would be the first member of your
> +own platdata struct). This allows the rest of U-Boot to dynamically change
> +the MAC address and have the new settings be respected.
>
> -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.
> -Any such information that is specific to an interface should be stored in a
> -private, driver-defined data structure and pointed to by eth->priv (see below).
> +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 packet buffer can (and
> +will!) be reused for subsequent calls to send().
> +Alternatively you can copy the data to be send, then just queue the copied
> +packet (for instance handing it over to a DMA engine), then return.
> +
> +The **recv** function polls for availability of a new packet. If none is
> +available, return a negative error code, -EAGAIN is probably a good idea.
> +If a packet has been received, make sure it is accessible to the CPU
> +(invalidate caches if needed), then write its address to the packetp pointer,
> +and return the length. If there is an error (receive error, too short or too
> +long packet), return 0 if you require the packet to be cleaned up normally,
> +or a negative error code otherwise (cleanup not neccessary or already done).
> +The U-Boot network stack will then process the packet.
> +
> +If **free_pkt** is defined, U-Boot will call it after a received packet has
> +been processed, so the packet buffer can be freed or recycled. Typically you
> +would hand it back to the hardware to acquire another packet. free_pkt() will
> +be called after recv(), for the same packet, so you don't necessarily need
> +to infer the buffer to free from the ``packet`` pointer, but can rely on that
> +being the last packet that recv() handled.
> +The common code sets up packet buffers for you already in the .bss
> +(net_rx_packets), so there should be no need to allocate your own. This doesn't
> +mean you must use the net_rx_packets array however; you're free to use any
> +buffer you wish.
> +
> +The **stop** function should turn off / disable the hardware and place it back
> +in its reset state.  It can be called at any time (before any call to the
> +related start() function), so make sure it can handle this sort of thing.
> +
> +The (optional) **write_hwaddr** function should program the MAC address stored
> +in pdata->enetaddr into the Ethernet controller.
>
>   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;
> -	struct mii_dev *bus;
> -
> -	priv = malloc(sizeof(*priv));
> -	if (priv == NULL)
> -		return -ENOMEM;
> +.. code-block:: c
>
> -	dev = malloc(sizeof(*dev));
> -	if (dev == NULL) {
> -		free(priv);
> -		return -ENOMEM;
> -	}
> +	(some net operation (ping / tftp / whatever...))
> +	eth_init()
> +		ops->start()
> +	eth_send()
> +		ops->send()
> +	eth_rx()
> +		ops->recv()
> +		(process packet)
> +		if (ops->free_pkt)
> +			ops->free_pkt()
> +	eth_halt()
> +		ops->stop()
>
> -	/* setup whatever private state you need */
>
> -	memset(dev, 0, sizeof(*dev));
> -	sprintf(dev->name, "APE");
> +================================
> + CONFIG_PHYLIB / CONFIG_CMD_MII
> +================================
>
> -	/*
> -	 * if your device has dedicated hardware storage for the
> -	 * MAC, read it and initialize dev->enetaddr with it
> -	 */
> -	ape_mac_read(dev->enetaddr);
> +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.
>
> -	dev->iobase = iobase;
> -	dev->priv = priv;
> -	dev->init = ape_init;
> -	dev->halt = ape_halt;
> -	dev->send = ape_send;
> -	dev->recv = ape_recv;
> -	dev->write_hwaddr = ape_write_hwaddr;
> +In your driver's ``probe()`` function, add a call to mdio_alloc() and
> +mdio_register() like so:
>
> -	eth_register(dev);
> +.. code-block:: c
>
> -#ifdef CONFIG_PHYLIB
>   	bus = mdio_alloc();
>   	if (!bus) {
> -		free(priv);
> -		free(dev);
> +		...
>   		return -ENOMEM;
>   	}
>
>   	bus->read = ape_mii_read;
>   	bus->write = ape_mii_write;
>   	mdio_register(bus);
> -#endif
>
> -	return 1;
> -}
> +And then define the mii_read and mii_write functions if you haven't already.
> +Their syntax is straightforward::
>
> -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.
> +	int mii_read(struct mii_dev *bus, int addr, int devad, int reg);
> +	int mii_write(struct mii_dev *bus, int addr, int devad, int reg,
> +		      u16 val);
>
> -The return value for this function should be as follows:
> -< 0 - failure (hardware failure, not probe failure)
> ->=0 - number of interfaces detected
> +The read function should read the register 'reg' from the phy at address 'addr'
> +and return the result to its caller.  The implementation for the write function
> +should logically follow.
>
> -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.
> +========================
> + Legacy network drivers
> +========================
>
> - -----------
> -  Callbacks
> - -----------
> +!!! WARNING !!!
>
> -Now that we've registered with the ethernet layer, we can start getting some
> -real work done.  You will need five 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);
> -	int ape_write_hwaddr(struct eth_device *dev);
> +This section below describes the old way of doing things. No new Ethernet
> +drivers should be implemented this way. All new drivers should be written
> +against the U-Boot core driver model, as described above.
>
> -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 actual callback functions are fairly similar, the differences are:
>
> -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.
> -For each packet you receive, you should call the net_process_received_packet() function on it
> -along with the packet length.  The common code sets up packet buffers for you
> -already in the .bss (net_rx_packets), so there should be no need to allocate your
> -own.  This doesn't mean you must use the net_rx_packets array however; you're
> -free to call the net_process_received_packet() function with any buffer you wish.  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(&net_rx_packets[i]);
> -		...
> -		net_process_received_packet(&net_rx_packets[i], length);
> -		...
> -		if (++i >= PKTBUFSRX)
> -			i = 0;
> -		...
> -	}
> -	...
> -	return 0;
> -}
> +- ``start()`` is called ``init()``
> +- ``stop()`` is called ``halt()``
> +- The ``recv()`` function must loop until all packets have been received, for
> +  each packet it must call the net_process_received_packet() function,
> +  handing it over the pointer and the length. Afterwards it should free
> +  the packet, before checking for new data.
>
> -The halt function should turn off / disable the hardware and place it back in
> -its reset state.  It can be called at any time (before any call to the related
> -init function), so make sure it can handle this sort of thing.
> +For porting an old driver to the new driver model, split the existing recv()
> +function into the actual new recv() function, just fetching **one** packet,
> +remove the call to net_process_received_packet(), then move the packet
> +cleanup into the ``free_pkt()`` function.
>
> -The write_hwaddr function should program the MAC address stored in dev->enetaddr
> -into the Ethernet controller.
> +Registering the driver and probing a device is handled very differently,
> +follow the recommendations in the driver model design documentation for
> +instructions on how to port this over. For the records, the old way of
> +initialising a network driver is as follows:
> +
> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> + Old network driver registration
> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +
> +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.
> +Any such information that is specific to an interface should be stored in a
> +private, driver-defined data structure and pointed to by eth->priv (see below).
>
>   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_PHYLIB / CONFIG_CMD_MII
> ---------------------------------
> +.. code-block:: c
>
> -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.
> +	board_init()
> +		eth_initialize()
> +			board_eth_init() / cpu_eth_init()
> +				driver_register()
> +					initialize eth_device
> +					eth_register()
>
> -After you have called eth_register() in your driver's register function, add
> -a call to mdio_alloc() and mdio_register() like so:
> -	bus = mdio_alloc();
> -	if (!bus) {
> -		free(priv);
> -		free(dev);
> -		return -ENOMEM;
> +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:
> +
> +.. code-block:: c
> +
> +	int ape_register(bd_t *bis, int iobase)
> +	{
> +		struct ape_priv *priv;
> +		struct eth_device *dev;
> +		struct mii_dev *bus;
> +
> +		priv = malloc(sizeof(*priv));
> +		if (priv == NULL)
> +			return -ENOMEM;
> +
> +		dev = malloc(sizeof(*dev));
> +		if (dev == NULL) {
> +			free(priv);
> +			return -ENOMEM;
> +		}
> +
> +		/* 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;
> +		dev->write_hwaddr = ape_write_hwaddr;
> +
> +		eth_register(dev);
> +
> +	#ifdef CONFIG_PHYLIB
> +		bus = mdio_alloc();
> +		if (!bus) {
> +			free(priv);
> +			free(dev);
> +			return -ENOMEM;
> +		}
> +
> +		bus->read = ape_mii_read;
> +		bus->write = ape_mii_write;
> +		mdio_register(bus);
> +	#endif
> +
> +		return 1;
>   	}
>
> -	bus->read = ape_mii_read;
> -	bus->write = ape_mii_write;
> -	mdio_register(bus);
> +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.
>
> -And then define the mii_read and mii_write functions if you haven't already.
> -Their syntax is straightforward:
> -	int mii_read(struct mii_dev *bus, int addr, int devad, int reg);
> -	int mii_write(struct mii_dev *bus, int addr, int devad, int reg,
> -		      u16 val);
> +The return value for this function should be as follows:
> +< 0 - failure (hardware failure, not probe failure)
> +>=0 - number of interfaces detected
>
> -The read function should read the register 'reg' from the phy at address 'addr'
> -and return the result to its caller.  The implementation for the write function
> -should logically follow.
> +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.
>

On Mon, 25 Nov 2019 07:06:35 +0100
Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:

Hi,

> On 11/25/19 2:32 AM, Andre Przywara wrote:
> > doc/README.drivers.eth seems like a good source for understanding
> > U-Boot's network subsystem, but is only talking about legacy network
> > drivers. This is particularly sad as proper documentation would help in
> > porting drivers over to the driver model.
> >
> > Rewrite the document to describe network drivers in the new driver model
> > world. Most driver callbacks are almost identical in their semantic, but
> > recv() differs in some important details.
> >
> > Also keep some parts of the original text at the end, to help
> > understanding old drivers. Add some hints on how to port drivers over.
> >
> > This also uses the opportunity to reformat the document in reST.  
> 
> To me the whole document looks nice. I just have some remarks concerning
> hooking the text up in the HTML documentation of U-Boot:
> 
> We want to be able to render the restructured text files with
> 
>      make htmldocs
> 
> Restructured text files should be named *.rst.
> 
> Would we move the file to doc/driver-model/eth.rst?

Sure.

> 
> In this case we would hook up the file in doc/driver-model/index.rst.
> 
> It would be rendered like this:
> 
> https://www.xypron.de/u-boot/driver-model/eth.html
> 
> Should we remove all the overlines for the headings so that the text
> looks more like the other rst files?

OK. I have no real experience with rst, but my online research seemed to suggest that overlines are more preferable. Plus the old document had them already.
 
> The priorities of underlines differ from other rst files. I suggest to
> start with ===== followed by ------. So we have a thicker underline for
> the H1 title than for the H2 titles when reading in a plain text editor.

OK, I originally had it like this, but figured that this doesn't give the right size of the headings (subheading being bigger). I tried restview and some online previewer. I might have messed something up, though, so I am happy to change this to match the other documents.

Thanks for having a look!

Cheers,
Andre.

> Best regards
> 
> Heinrich
> 
> >
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > ---
> >   doc/README.drivers.eth | 438 ++++++++++++++++++++++++++++++-------------------
> >   1 file changed, 271 insertions(+), 167 deletions(-)
> >
> > diff --git a/doc/README.drivers.eth b/doc/README.drivers.eth
> > index 1a9a23b51b..d1920ee47d 100644
> > --- a/doc/README.drivers.eth
> > +++ b/doc/README.drivers.eth
> > @@ -1,9 +1,3 @@
> > -!!! WARNING !!!
> > -
> > -This guide describes to the old way of doing things. No new Ethernet drivers
> > -should be implemented this way. All new drivers should be written against the
> > -U-Boot core driver model. See doc/driver-model/README.txt
> > -
> >   -----------------------
> >    Ethernet Driver Guide
> >   -----------------------
> > @@ -13,203 +7,313 @@ 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.
> > +Most exisiting drivers do already - and new network driver MUST - use the
> > +U-Boot core driver model. Generic information about this can be found in
> > +doc/driver-model/design.rst, this document will thus focus on the network
> > +specific code parts.
> > +Some drivers are still using the old Ethernet interface, differences between
> > +the two and hints about porting will be handled at the end.
> > +
> > +==================
> > + Driver framework
> > +==================
> > +
> > +A network driver following the driver model must declare itself using
> > +the UCLASS_ETH .id field in the U-Boot driver struct:
> > +
> > +.. code-block:: c
> > +
> > +	U_BOOT_DRIVER(eth_ape) = {
> > +		.name			= "eth_ape",
> > +		.id			= UCLASS_ETH,
> > +		.of_match		= eth_ape_ids,
> > +		.ofdata_to_platdata	= eth_ape_ofdata_to_platdata,
> > +		.probe			= eth_ape_probe,
> > +		.ops			= &eth_ape_ops,
> > +		.priv_auto_alloc_size	= sizeof(struct eth_ape_dev),
> > +		.platdata_auto_alloc_size = sizeof(struct eth_ape_pdata),
> > +		.flags			= DM_FLAG_ALLOC_PRIV_DMA,
> > +	};
> > +
> > +struct eth_ape_dev contains runtime per-instance data, like buffers, pointers
> > +to current descriptors, current speed settings, pointers to PHY related data
> > +(like struct mii_dev) and so on. Declaring its size in .priv_auto_alloc_size
> > +will let the driver framework allocate it at the right time.
> > +It can be retrieved using a dev_get_priv(dev) call.
> > +
> > +struct eth_ape_pdata contains static platform data, like the MMIO base address,
> > +a hardware variant, the MAC address. ``struct eth_pdata eth_pdata``
> > +as the first member of this struct helps to avoid duplicated code.
> > +If you don't need any more platform data beside the standard member,
> > +just use sizeof(struct eth_pdata) for the platdata_auto_alloc_size.
> > +
> > +PCI devices add a line pointing to supported vendor/device ID pairs:
> > +
> > +.. code-block:: c
> > +
> > +	static struct pci_device_id supported[] = {
> > +		{ PCI_DEVICE(PCI_VENDOR_ID_APE, 0x4223) },
> > +		{}
> > +	};
> > +
> > +	U_BOOT_PCI_DEVICE(eth_ape, supported);
> > +
> > +
> > +Device probing and instantiation will be handled by the driver model framework,
> > +so follow the guidelines there. The probe() function would initialise the
> > +platform specific parts of the hardware, like clocks, resets, GPIOs, the MDIO
> > +bus. Also it would take care of any special PHY setup (power rails, enable
> > +bits for internal PHYs, etc.).
> > +
> > +====================
> > + Callback functions
> > +====================
> > +
> > +The real work will be done in the callback function the driver provides
> > +by defining the members of struct eth_ops:
> > +
> > +.. code-block:: c
> > +
> > +	struct eth_ops {
> > +		int (*start)(struct udevice *dev);
> > +		int (*send)(struct udevice *dev, void *packet, int length);
> > +		int (*recv)(struct udevice *dev, int flags, uchar **packetp);
> > +		int (*free_pkt)(struct udevice *dev, uchar *packet, int length);
> > +		void (*stop)(struct udevice *dev);
> > +		int (*mcast)(struct udevice *dev, const u8 *enetaddr, int join);
> > +		int (*write_hwaddr)(struct udevice *dev);
> > +		int (*read_rom_hwaddr)(struct udevice *dev);
> > +	};
> > +
> > +An up-to-date version of this struct together with more information can be
> > +found in include/net.h.
> > +
> > +Only start, stop, send and recv are required, the rest is optional and will
> > +be handled by generic code or ignored if not provided.
> > +
> > +The **start** function initialises the hardware 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 enetaddr member
> > +of the generic struct eth_pdata (which would be the first member of your
> > +own platdata struct). This allows the rest of U-Boot to dynamically change
> > +the MAC address and have the new settings be respected.
> >
> > -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.
> > -Any such information that is specific to an interface should be stored in a
> > -private, driver-defined data structure and pointed to by eth->priv (see below).
> > +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 packet buffer can (and
> > +will!) be reused for subsequent calls to send().
> > +Alternatively you can copy the data to be send, then just queue the copied
> > +packet (for instance handing it over to a DMA engine), then return.
> > +
> > +The **recv** function polls for availability of a new packet. If none is
> > +available, return a negative error code, -EAGAIN is probably a good idea.
> > +If a packet has been received, make sure it is accessible to the CPU
> > +(invalidate caches if needed), then write its address to the packetp pointer,
> > +and return the length. If there is an error (receive error, too short or too
> > +long packet), return 0 if you require the packet to be cleaned up normally,
> > +or a negative error code otherwise (cleanup not neccessary or already done).
> > +The U-Boot network stack will then process the packet.
> > +
> > +If **free_pkt** is defined, U-Boot will call it after a received packet has
> > +been processed, so the packet buffer can be freed or recycled. Typically you
> > +would hand it back to the hardware to acquire another packet. free_pkt() will
> > +be called after recv(), for the same packet, so you don't necessarily need
> > +to infer the buffer to free from the ``packet`` pointer, but can rely on that
> > +being the last packet that recv() handled.
> > +The common code sets up packet buffers for you already in the .bss
> > +(net_rx_packets), so there should be no need to allocate your own. This doesn't
> > +mean you must use the net_rx_packets array however; you're free to use any
> > +buffer you wish.
> > +
> > +The **stop** function should turn off / disable the hardware and place it back
> > +in its reset state.  It can be called at any time (before any call to the
> > +related start() function), so make sure it can handle this sort of thing.
> > +
> > +The (optional) **write_hwaddr** function should program the MAC address stored
> > +in pdata->enetaddr into the Ethernet controller.
> >
> >   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;
> > -	struct mii_dev *bus;
> > -
> > -	priv = malloc(sizeof(*priv));
> > -	if (priv == NULL)
> > -		return -ENOMEM;
> > +.. code-block:: c
> >
> > -	dev = malloc(sizeof(*dev));
> > -	if (dev == NULL) {
> > -		free(priv);
> > -		return -ENOMEM;
> > -	}
> > +	(some net operation (ping / tftp / whatever...))
> > +	eth_init()
> > +		ops->start()
> > +	eth_send()
> > +		ops->send()
> > +	eth_rx()
> > +		ops->recv()
> > +		(process packet)
> > +		if (ops->free_pkt)
> > +			ops->free_pkt()
> > +	eth_halt()
> > +		ops->stop()
> >
> > -	/* setup whatever private state you need */
> >
> > -	memset(dev, 0, sizeof(*dev));
> > -	sprintf(dev->name, "APE");
> > +================================
> > + CONFIG_PHYLIB / CONFIG_CMD_MII
> > +================================
> >
> > -	/*
> > -	 * if your device has dedicated hardware storage for the
> > -	 * MAC, read it and initialize dev->enetaddr with it
> > -	 */
> > -	ape_mac_read(dev->enetaddr);
> > +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.
> >
> > -	dev->iobase = iobase;
> > -	dev->priv = priv;
> > -	dev->init = ape_init;
> > -	dev->halt = ape_halt;
> > -	dev->send = ape_send;
> > -	dev->recv = ape_recv;
> > -	dev->write_hwaddr = ape_write_hwaddr;
> > +In your driver's ``probe()`` function, add a call to mdio_alloc() and
> > +mdio_register() like so:
> >
> > -	eth_register(dev);
> > +.. code-block:: c
> >
> > -#ifdef CONFIG_PHYLIB
> >   	bus = mdio_alloc();
> >   	if (!bus) {
> > -		free(priv);
> > -		free(dev);
> > +		...
> >   		return -ENOMEM;
> >   	}
> >
> >   	bus->read = ape_mii_read;
> >   	bus->write = ape_mii_write;
> >   	mdio_register(bus);
> > -#endif
> >
> > -	return 1;
> > -}
> > +And then define the mii_read and mii_write functions if you haven't already.
> > +Their syntax is straightforward::
> >
> > -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.
> > +	int mii_read(struct mii_dev *bus, int addr, int devad, int reg);
> > +	int mii_write(struct mii_dev *bus, int addr, int devad, int reg,
> > +		      u16 val);
> >
> > -The return value for this function should be as follows:
> > -< 0 - failure (hardware failure, not probe failure)  
> > ->=0 - number of interfaces detected  
> > +The read function should read the register 'reg' from the phy at address 'addr'
> > +and return the result to its caller.  The implementation for the write function
> > +should logically follow.
> >
> > -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.
> > +========================
> > + Legacy network drivers
> > +========================
> >
> > - -----------
> > -  Callbacks
> > - -----------
> > +!!! WARNING !!!
> >
> > -Now that we've registered with the ethernet layer, we can start getting some
> > -real work done.  You will need five 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);
> > -	int ape_write_hwaddr(struct eth_device *dev);
> > +This section below describes the old way of doing things. No new Ethernet
> > +drivers should be implemented this way. All new drivers should be written
> > +against the U-Boot core driver model, as described above.
> >
> > -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 actual callback functions are fairly similar, the differences are:
> >
> > -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.
> > -For each packet you receive, you should call the net_process_received_packet() function on it
> > -along with the packet length.  The common code sets up packet buffers for you
> > -already in the .bss (net_rx_packets), so there should be no need to allocate your
> > -own.  This doesn't mean you must use the net_rx_packets array however; you're
> > -free to call the net_process_received_packet() function with any buffer you wish.  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(&net_rx_packets[i]);
> > -		...
> > -		net_process_received_packet(&net_rx_packets[i], length);
> > -		...
> > -		if (++i >= PKTBUFSRX)
> > -			i = 0;
> > -		...
> > -	}
> > -	...
> > -	return 0;
> > -}
> > +- ``start()`` is called ``init()``
> > +- ``stop()`` is called ``halt()``
> > +- The ``recv()`` function must loop until all packets have been received, for
> > +  each packet it must call the net_process_received_packet() function,
> > +  handing it over the pointer and the length. Afterwards it should free
> > +  the packet, before checking for new data.
> >
> > -The halt function should turn off / disable the hardware and place it back in
> > -its reset state.  It can be called at any time (before any call to the related
> > -init function), so make sure it can handle this sort of thing.
> > +For porting an old driver to the new driver model, split the existing recv()
> > +function into the actual new recv() function, just fetching **one** packet,
> > +remove the call to net_process_received_packet(), then move the packet
> > +cleanup into the ``free_pkt()`` function.
> >
> > -The write_hwaddr function should program the MAC address stored in dev->enetaddr
> > -into the Ethernet controller.
> > +Registering the driver and probing a device is handled very differently,
> > +follow the recommendations in the driver model design documentation for
> > +instructions on how to port this over. For the records, the old way of
> > +initialising a network driver is as follows:
> > +
> > +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > + Old network driver registration
> > +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > +
> > +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.
> > +Any such information that is specific to an interface should be stored in a
> > +private, driver-defined data structure and pointed to by eth->priv (see below).
> >
> >   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_PHYLIB / CONFIG_CMD_MII
> > ---------------------------------
> > +.. code-block:: c
> >
> > -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.
> > +	board_init()
> > +		eth_initialize()
> > +			board_eth_init() / cpu_eth_init()
> > +				driver_register()
> > +					initialize eth_device
> > +					eth_register()
> >
> > -After you have called eth_register() in your driver's register function, add
> > -a call to mdio_alloc() and mdio_register() like so:
> > -	bus = mdio_alloc();
> > -	if (!bus) {
> > -		free(priv);
> > -		free(dev);
> > -		return -ENOMEM;
> > +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:
> > +
> > +.. code-block:: c
> > +
> > +	int ape_register(bd_t *bis, int iobase)
> > +	{
> > +		struct ape_priv *priv;
> > +		struct eth_device *dev;
> > +		struct mii_dev *bus;
> > +
> > +		priv = malloc(sizeof(*priv));
> > +		if (priv == NULL)
> > +			return -ENOMEM;
> > +
> > +		dev = malloc(sizeof(*dev));
> > +		if (dev == NULL) {
> > +			free(priv);
> > +			return -ENOMEM;
> > +		}
> > +
> > +		/* 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;
> > +		dev->write_hwaddr = ape_write_hwaddr;
> > +
> > +		eth_register(dev);
> > +
> > +	#ifdef CONFIG_PHYLIB
> > +		bus = mdio_alloc();
> > +		if (!bus) {
> > +			free(priv);
> > +			free(dev);
> > +			return -ENOMEM;
> > +		}
> > +
> > +		bus->read = ape_mii_read;
> > +		bus->write = ape_mii_write;
> > +		mdio_register(bus);
> > +	#endif
> > +
> > +		return 1;
> >   	}
> >
> > -	bus->read = ape_mii_read;
> > -	bus->write = ape_mii_write;
> > -	mdio_register(bus);
> > +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.
> >
> > -And then define the mii_read and mii_write functions if you haven't already.
> > -Their syntax is straightforward:
> > -	int mii_read(struct mii_dev *bus, int addr, int devad, int reg);
> > -	int mii_write(struct mii_dev *bus, int addr, int devad, int reg,
> > -		      u16 val);
> > +The return value for this function should be as follows:
> > +< 0 - failure (hardware failure, not probe failure)  
> > +>=0 - number of interfaces detected  
> >
> > -The read function should read the register 'reg' from the phy at address 'addr'
> > -and return the result to its caller.  The implementation for the write function
> > -should logically follow.
> > +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.
> >  
>

Hi Andre,

On Sun, 24 Nov 2019 at 18:32, Andre Przywara <andre.przywara@arm.com> wrote:
>
> doc/README.drivers.eth seems like a good source for understanding
> U-Boot's network subsystem, but is only talking about legacy network
> drivers. This is particularly sad as proper documentation would help in
> porting drivers over to the driver model.
>
> Rewrite the document to describe network drivers in the new driver model
> world. Most driver callbacks are almost identical in their semantic, but
> recv() differs in some important details.
>
> Also keep some parts of the original text at the end, to help
> understanding old drivers. Add some hints on how to port drivers over.
>
> This also uses the opportunity to reformat the document in reST.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>

Great to see this!

Minor comments below.

Reviewed-by: Simon Glass <sjg@chromium.org>

> ---
>  doc/README.drivers.eth | 438 ++++++++++++++++++++++++++++++-------------------
>  1 file changed, 271 insertions(+), 167 deletions(-)
>
> diff --git a/doc/README.drivers.eth b/doc/README.drivers.eth
> index 1a9a23b51b..d1920ee47d 100644
> --- a/doc/README.drivers.eth
> +++ b/doc/README.drivers.eth
> @@ -1,9 +1,3 @@
> -!!! WARNING !!!
> -
> -This guide describes to the old way of doing things. No new Ethernet drivers
> -should be implemented this way. All new drivers should be written against the
> -U-Boot core driver model. See doc/driver-model/README.txt
> -
>  -----------------------
>   Ethernet Driver Guide
>  -----------------------
> @@ -13,203 +7,313 @@ 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.
> +Most exisiting drivers do already - and new network driver MUST - use the

existing

> +U-Boot core driver model. Generic information about this can be found in
> +doc/driver-model/design.rst, this document will thus focus on the network
> +specific code parts.
> +Some drivers are still using the old Ethernet interface, differences between
> +the two and hints about porting will be handled at the end.
> +
> +==================
> + Driver framework
> +==================
> +
> +A network driver following the driver model must declare itself using
> +the UCLASS_ETH .id field in the U-Boot driver struct:
> +
> +.. code-block:: c
> +
> +       U_BOOT_DRIVER(eth_ape) = {
> +               .name                   = "eth_ape",
> +               .id                     = UCLASS_ETH,
> +               .of_match               = eth_ape_ids,
> +               .ofdata_to_platdata     = eth_ape_ofdata_to_platdata,
> +               .probe                  = eth_ape_probe,
> +               .ops                    = &eth_ape_ops,
> +               .priv_auto_alloc_size   = sizeof(struct eth_ape_dev),

I prefer a _priv suffix on this and that is the most common.

> +               .platdata_auto_alloc_size = sizeof(struct eth_ape_pdata),

I normally use platdata, but I agree pdata is better, so let's use
that. One day we can do s/platdata/pdata/

> +               .flags                  = DM_FLAG_ALLOC_PRIV_DMA,
> +       };
> +
> +struct eth_ape_dev contains runtime per-instance data, like buffers, pointers
> +to current descriptors, current speed settings, pointers to PHY related data
> +(like struct mii_dev) and so on. Declaring its size in .priv_auto_alloc_size
> +will let the driver framework allocate it at the right time.
> +It can be retrieved using a dev_get_priv(dev) call.
> +
> +struct eth_ape_pdata contains static platform data, like the MMIO base address,
> +a hardware variant, the MAC address. ``struct eth_pdata eth_pdata``
> +as the first member of this struct helps to avoid duplicated code.
> +If you don't need any more platform data beside the standard member,
> +just use sizeof(struct eth_pdata) for the platdata_auto_alloc_size.
> +
> +PCI devices add a line pointing to supported vendor/device ID pairs:
> +
> +.. code-block:: c
> +
> +       static struct pci_device_id supported[] = {
> +               { PCI_DEVICE(PCI_VENDOR_ID_APE, 0x4223) },
> +               {}
> +       };
> +
> +       U_BOOT_PCI_DEVICE(eth_ape, supported);

It is also possible to declare support for a whole class of PCI devices:

    { PCI_DEVICE_CLASS(PCI_CLASS_SYSTEM_SDHCI << 8, 0xffff00) },

> +
> +
> +Device probing and instantiation will be handled by the driver model framework,
> +so follow the guidelines there. The probe() function would initialise the
> +platform specific parts of the hardware, like clocks, resets, GPIOs, the MDIO
> +bus. Also it would take care of any special PHY setup (power rails, enable
> +bits for internal PHYs, etc.).
> +
> +====================
> + Callback functions

Driver methods

I really don't want to call these callbacks, since the driver is no
the main thread of execution, or in control of execution, as it was in
pre-DM days. So let's call them methods.

> +====================
> +
> +The real work will be done in the callback function the driver provides
> +by defining the members of struct eth_ops:
> +
> +.. code-block:: c
> +
> +       struct eth_ops {
> +               int (*start)(struct udevice *dev);
> +               int (*send)(struct udevice *dev, void *packet, int length);
> +               int (*recv)(struct udevice *dev, int flags, uchar **packetp);
> +               int (*free_pkt)(struct udevice *dev, uchar *packet, int length);
> +               void (*stop)(struct udevice *dev);
> +               int (*mcast)(struct udevice *dev, const u8 *enetaddr, int join);
> +               int (*write_hwaddr)(struct udevice *dev);
> +               int (*read_rom_hwaddr)(struct udevice *dev);
> +       };
> +
> +An up-to-date version of this struct together with more information can be
> +found in include/net.h.
> +
> +Only start, stop, send and recv are required, the rest is optional and will

are optional

are handled (present tense is best for docs I think)

> +be handled by generic code or ignored if not provided.
> +
> +The **start** function initialises the hardware 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 enetaddr member
> +of the generic struct eth_pdata (which would be the first member of your
> +own platdata struct). This allows the rest of U-Boot to dynamically change
> +the MAC address and have the new settings be respected.
>
> -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.
> -Any such information that is specific to an interface should be stored in a
> -private, driver-defined data structure and pointed to by eth->priv (see below).
> +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 packet buffer can (and
> +will!) be reused for subsequent calls to send().

Actually I think it is OK to return before the transmit is complete.
But it must be sent to the hardware and the buffer no-longer used, as
you say.

> +Alternatively you can copy the data to be send, then just queue the copied
> +packet (for instance handing it over to a DMA engine), then return.
> +
> +The **recv** function polls for availability of a new packet. If none is
> +available, return a negative error code, -EAGAIN is probably a good idea.

In fact it must return this to avoid an error - see eth_rx().

Unfortunately struct eth_ops is not commented property. It would be
great to add proper comments with parameters and return values there.
We have this for most uclasses but net slipped through the cracks.

> +If a packet has been received, make sure it is accessible to the CPU
> +(invalidate caches if needed), then write its address to the packetp pointer,
> +and return the length. If there is an error (receive error, too short or too
> +long packet), return 0 if you require the packet to be cleaned up normally,
> +or a negative error code otherwise (cleanup not neccessary or already done).

necessary

> +The U-Boot network stack will then process the packet.
> +
> +If **free_pkt** is defined, U-Boot will call it after a received packet has
> +been processed, so the packet buffer can be freed or recycled. Typically you
> +would hand it back to the hardware to acquire another packet. free_pkt() will
> +be called after recv(), for the same packet, so you don't necessarily need
> +to infer the buffer to free from the ``packet`` pointer, but can rely on that
> +being the last packet that recv() handled.

Very good point.

> +The common code sets up packet buffers for you already in the .bss
> +(net_rx_packets), so there should be no need to allocate your own. This doesn't
> +mean you must use the net_rx_packets array however; you're free to use any
> +buffer you wish.
> +
> +The **stop** function should turn off / disable the hardware and place it back
> +in its reset state.  It can be called at any time (before any call to the
> +related start() function), so make sure it can handle this sort of thing.
> +
> +The (optional) **write_hwaddr** function should program the MAC address stored
> +in pdata->enetaddr into the Ethernet controller.
>
>  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;
> -       struct mii_dev *bus;
> -
> -       priv = malloc(sizeof(*priv));
> -       if (priv == NULL)
> -               return -ENOMEM;
> +.. code-block:: c
>
> -       dev = malloc(sizeof(*dev));
> -       if (dev == NULL) {
> -               free(priv);
> -               return -ENOMEM;
> -       }
> +       (some net operation (ping / tftp / whatever...))
> +       eth_init()
> +               ops->start()
> +       eth_send()
> +               ops->send()
> +       eth_rx()
> +               ops->recv()
> +               (process packet)
> +               if (ops->free_pkt)
> +                       ops->free_pkt()
> +       eth_halt()
> +               ops->stop()
>
> -       /* setup whatever private state you need */
>
> -       memset(dev, 0, sizeof(*dev));
> -       sprintf(dev->name, "APE");
> +================================
> + CONFIG_PHYLIB / CONFIG_CMD_MII
> +================================

Hmm yes, this really needs to move to DM.

Regards,
Simon

On 11/25/19 2:32 AM, Andre Przywara wrote:
> doc/README.drivers.eth seems like a good source for understanding
> U-Boot's network subsystem, but is only talking about legacy network
> drivers. This is particularly sad as proper documentation would help in
> porting drivers over to the driver model.
>
> Rewrite the document to describe network drivers in the new driver model
> world. Most driver callbacks are almost identical in their semantic, but
> recv() differs in some important details.
>
> Also keep some parts of the original text at the end, to help
> understanding old drivers. Add some hints on how to port drivers over.
>
> This also uses the opportunity to reformat the document in reST.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>   doc/README.drivers.eth | 438 ++++++++++++++++++++++++++++++-------------------
>   1 file changed, 271 insertions(+), 167 deletions(-)
>
> diff --git a/doc/README.drivers.eth b/doc/README.drivers.eth

Now that this file is in restructured text I think we should rename the
file with a .rst ending and put it somewhere into the documentation
generated by `make htmldocs`. Cf. https://www.xypron.de/u-boot/. Would
the driver-model chapter be the appropriate place?

Best regards

Heinrich

On 27/12/2019 18:57, Heinrich Schuchardt wrote:

Hi Heinrich,

> On 11/25/19 2:32 AM, Andre Przywara wrote:
>> doc/README.drivers.eth seems like a good source for understanding
>> U-Boot's network subsystem, but is only talking about legacy network
>> drivers. This is particularly sad as proper documentation would help in
>> porting drivers over to the driver model.
>>
>> Rewrite the document to describe network drivers in the new driver model
>> world. Most driver callbacks are almost identical in their semantic, but
>> recv() differs in some important details.
>>
>> Also keep some parts of the original text at the end, to help
>> understanding old drivers. Add some hints on how to port drivers over.
>>
>> This also uses the opportunity to reformat the document in reST.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>> ---
>>   doc/README.drivers.eth | 438
>> ++++++++++++++++++++++++++++++-------------------
>>   1 file changed, 271 insertions(+), 167 deletions(-)
>>
>> diff --git a/doc/README.drivers.eth b/doc/README.drivers.eth
> 
> Now that this file is in restructured text I think we should rename the
> file with a .rst ending and put it somewhere into the documentation
> generated by `make htmldocs`. Cf. https://www.xypron.de/u-boot/. Would
> the driver-model chapter be the appropriate place?

Yeah, I moved it now there, adding it to the index. Also I checked again
on the changes to the headings style you suggested earlier, and this now
seems to work for me here, so I changed that too.

Thanks!
Andre

On 27/12/2019 16:41, Simon Glass wrote:

Hi Simon,

> On Sun, 24 Nov 2019 at 18:32, Andre Przywara <andre.przywara@arm.com> wrote:
>>
>> doc/README.drivers.eth seems like a good source for understanding
>> U-Boot's network subsystem, but is only talking about legacy network
>> drivers. This is particularly sad as proper documentation would help in
>> porting drivers over to the driver model.
>>
>> Rewrite the document to describe network drivers in the new driver model
>> world. Most driver callbacks are almost identical in their semantic, but
>> recv() differs in some important details.
>>
>> Also keep some parts of the original text at the end, to help
>> understanding old drivers. Add some hints on how to port drivers over.
>>
>> This also uses the opportunity to reformat the document in reST.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> 
> Great to see this!
> 
> Minor comments below.

Many thanks for having a look and the comments! I changed all the points
you mentioned, sending a v2 in a minute.

Cheers,
Andre.

> Reviewed-by: Simon Glass <sjg@chromium.org>
> 
>> ---
>>  doc/README.drivers.eth | 438 ++++++++++++++++++++++++++++++-------------------
>>  1 file changed, 271 insertions(+), 167 deletions(-)
>>
>> diff --git a/doc/README.drivers.eth b/doc/README.drivers.eth
>> index 1a9a23b51b..d1920ee47d 100644
>> --- a/doc/README.drivers.eth
>> +++ b/doc/README.drivers.eth
>> @@ -1,9 +1,3 @@
>> -!!! WARNING !!!
>> -
>> -This guide describes to the old way of doing things. No new Ethernet drivers
>> -should be implemented this way. All new drivers should be written against the
>> -U-Boot core driver model. See doc/driver-model/README.txt
>> -
>>  -----------------------
>>   Ethernet Driver Guide
>>  -----------------------
>> @@ -13,203 +7,313 @@ 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.
>> +Most exisiting drivers do already - and new network driver MUST - use the
> 
> existing
> 
>> +U-Boot core driver model. Generic information about this can be found in
>> +doc/driver-model/design.rst, this document will thus focus on the network
>> +specific code parts.
>> +Some drivers are still using the old Ethernet interface, differences between
>> +the two and hints about porting will be handled at the end.
>> +
>> +==================
>> + Driver framework
>> +==================
>> +
>> +A network driver following the driver model must declare itself using
>> +the UCLASS_ETH .id field in the U-Boot driver struct:
>> +
>> +.. code-block:: c
>> +
>> +       U_BOOT_DRIVER(eth_ape) = {
>> +               .name                   = "eth_ape",
>> +               .id                     = UCLASS_ETH,
>> +               .of_match               = eth_ape_ids,
>> +               .ofdata_to_platdata     = eth_ape_ofdata_to_platdata,
>> +               .probe                  = eth_ape_probe,
>> +               .ops                    = &eth_ape_ops,
>> +               .priv_auto_alloc_size   = sizeof(struct eth_ape_dev),
> 
> I prefer a _priv suffix on this and that is the most common.
> 
>> +               .platdata_auto_alloc_size = sizeof(struct eth_ape_pdata),
> 
> I normally use platdata, but I agree pdata is better, so let's use
> that. One day we can do s/platdata/pdata/
> 
>> +               .flags                  = DM_FLAG_ALLOC_PRIV_DMA,
>> +       };
>> +
>> +struct eth_ape_dev contains runtime per-instance data, like buffers, pointers
>> +to current descriptors, current speed settings, pointers to PHY related data
>> +(like struct mii_dev) and so on. Declaring its size in .priv_auto_alloc_size
>> +will let the driver framework allocate it at the right time.
>> +It can be retrieved using a dev_get_priv(dev) call.
>> +
>> +struct eth_ape_pdata contains static platform data, like the MMIO base address,
>> +a hardware variant, the MAC address. ``struct eth_pdata eth_pdata``
>> +as the first member of this struct helps to avoid duplicated code.
>> +If you don't need any more platform data beside the standard member,
>> +just use sizeof(struct eth_pdata) for the platdata_auto_alloc_size.
>> +
>> +PCI devices add a line pointing to supported vendor/device ID pairs:
>> +
>> +.. code-block:: c
>> +
>> +       static struct pci_device_id supported[] = {
>> +               { PCI_DEVICE(PCI_VENDOR_ID_APE, 0x4223) },
>> +               {}
>> +       };
>> +
>> +       U_BOOT_PCI_DEVICE(eth_ape, supported);
> 
> It is also possible to declare support for a whole class of PCI devices:
> 
>     { PCI_DEVICE_CLASS(PCI_CLASS_SYSTEM_SDHCI << 8, 0xffff00) },
> 
>> +
>> +
>> +Device probing and instantiation will be handled by the driver model framework,
>> +so follow the guidelines there. The probe() function would initialise the
>> +platform specific parts of the hardware, like clocks, resets, GPIOs, the MDIO
>> +bus. Also it would take care of any special PHY setup (power rails, enable
>> +bits for internal PHYs, etc.).
>> +
>> +====================
>> + Callback functions
> 
> Driver methods
> 
> I really don't want to call these callbacks, since the driver is no
> the main thread of execution, or in control of execution, as it was in
> pre-DM days. So let's call them methods.
> 
>> +====================
>> +
>> +The real work will be done in the callback function the driver provides
>> +by defining the members of struct eth_ops:
>> +
>> +.. code-block:: c
>> +
>> +       struct eth_ops {
>> +               int (*start)(struct udevice *dev);
>> +               int (*send)(struct udevice *dev, void *packet, int length);
>> +               int (*recv)(struct udevice *dev, int flags, uchar **packetp);
>> +               int (*free_pkt)(struct udevice *dev, uchar *packet, int length);
>> +               void (*stop)(struct udevice *dev);
>> +               int (*mcast)(struct udevice *dev, const u8 *enetaddr, int join);
>> +               int (*write_hwaddr)(struct udevice *dev);
>> +               int (*read_rom_hwaddr)(struct udevice *dev);
>> +       };
>> +
>> +An up-to-date version of this struct together with more information can be
>> +found in include/net.h.
>> +
>> +Only start, stop, send and recv are required, the rest is optional and will
> 
> are optional
> 
> are handled (present tense is best for docs I think)
> 
>> +be handled by generic code or ignored if not provided.
>> +
>> +The **start** function initialises the hardware 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 enetaddr member
>> +of the generic struct eth_pdata (which would be the first member of your
>> +own platdata struct). This allows the rest of U-Boot to dynamically change
>> +the MAC address and have the new settings be respected.
>>
>> -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.
>> -Any such information that is specific to an interface should be stored in a
>> -private, driver-defined data structure and pointed to by eth->priv (see below).
>> +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 packet buffer can (and
>> +will!) be reused for subsequent calls to send().
> 
> Actually I think it is OK to return before the transmit is complete.
> But it must be sent to the hardware and the buffer no-longer used, as
> you say.
> 
>> +Alternatively you can copy the data to be send, then just queue the copied
>> +packet (for instance handing it over to a DMA engine), then return.
>> +
>> +The **recv** function polls for availability of a new packet. If none is
>> +available, return a negative error code, -EAGAIN is probably a good idea.
> 
> In fact it must return this to avoid an error - see eth_rx().
> 
> Unfortunately struct eth_ops is not commented property. It would be
> great to add proper comments with parameters and return values there.
> We have this for most uclasses but net slipped through the cracks.
> 
>> +If a packet has been received, make sure it is accessible to the CPU
>> +(invalidate caches if needed), then write its address to the packetp pointer,
>> +and return the length. If there is an error (receive error, too short or too
>> +long packet), return 0 if you require the packet to be cleaned up normally,
>> +or a negative error code otherwise (cleanup not neccessary or already done).
> 
> necessary
> 
>> +The U-Boot network stack will then process the packet.
>> +
>> +If **free_pkt** is defined, U-Boot will call it after a received packet has
>> +been processed, so the packet buffer can be freed or recycled. Typically you
>> +would hand it back to the hardware to acquire another packet. free_pkt() will
>> +be called after recv(), for the same packet, so you don't necessarily need
>> +to infer the buffer to free from the ``packet`` pointer, but can rely on that
>> +being the last packet that recv() handled.
> 
> Very good point.
> 
>> +The common code sets up packet buffers for you already in the .bss
>> +(net_rx_packets), so there should be no need to allocate your own. This doesn't
>> +mean you must use the net_rx_packets array however; you're free to use any
>> +buffer you wish.
>> +
>> +The **stop** function should turn off / disable the hardware and place it back
>> +in its reset state.  It can be called at any time (before any call to the
>> +related start() function), so make sure it can handle this sort of thing.
>> +
>> +The (optional) **write_hwaddr** function should program the MAC address stored
>> +in pdata->enetaddr into the Ethernet controller.
>>
>>  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;
>> -       struct mii_dev *bus;
>> -
>> -       priv = malloc(sizeof(*priv));
>> -       if (priv == NULL)
>> -               return -ENOMEM;
>> +.. code-block:: c
>>
>> -       dev = malloc(sizeof(*dev));
>> -       if (dev == NULL) {
>> -               free(priv);
>> -               return -ENOMEM;
>> -       }
>> +       (some net operation (ping / tftp / whatever...))
>> +       eth_init()
>> +               ops->start()
>> +       eth_send()
>> +               ops->send()
>> +       eth_rx()
>> +               ops->recv()
>> +               (process packet)
>> +               if (ops->free_pkt)
>> +                       ops->free_pkt()
>> +       eth_halt()
>> +               ops->stop()
>>
>> -       /* setup whatever private state you need */
>>
>> -       memset(dev, 0, sizeof(*dev));
>> -       sprintf(dev->name, "APE");
>> +================================
>> + CONFIG_PHYLIB / CONFIG_CMD_MII
>> +================================
> 
> Hmm yes, this really needs to move to DM.
> 
> Regards,
> Simon
>