[U-Boot] [RFC PATCH 1/1] doc: net: Rewrite network driver documentation
Andre Przywara
andre.przywara at arm.com
Mon Nov 25 09:50:05 UTC 2019
On Mon, 25 Nov 2019 07:06:35 +0100
Heinrich Schuchardt <xypron.glpk at 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 at 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 = ð_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.
> >
>
More information about the U-Boot
mailing list