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                    = &eth_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

Reply via email to