[RFC PATCH 1/1] doc: net: Rewrite network driver documentation

[Simon Glass]

Hi Andre,

On Sun, 24 Nov 2019 at 18:32, Andre Przywara <andre.przywara at 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 at arm.com>

Great to see this!

Minor comments below.

Reviewed-by: Simon Glass <sjg at 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