On 27/12/2019 16:41, Simon Glass wrote: Hi Simon,
> On Sun, 24 Nov 2019 at 18:32, Andre Przywara <[email protected]> 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 <[email protected]> > > 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 <[email protected]> > >> --- >> 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 >
