On Mon, 25 Nov 2019 07:06:35 +0100 Heinrich Schuchardt <xypron.g...@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.przyw...@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. > > > _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
