[RFC PATCH 1/1] doc: net: Rewrite network driver documentation
André Przywara
andre.przywara at arm.com
Sat Dec 28 16:06:59 CET 2019
On 27/12/2019 16:41, Simon Glass wrote:
Hi Simon,
> 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.
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 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 = ð_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
>
More information about the U-Boot
mailing list