[U-Boot] [PATCH v2 3/6] net: e1000: Convert to driver model

Joe Hershberger joe.hershberger at gmail.com
Tue Aug 11 19:33:27 CEST 2015


Hi Simon,

On Tue, Aug 11, 2015 at 12:17 PM, Simon Glass <sjg at chromium.org> wrote:
> Hi Joe,
>
> On 11 August 2015 at 09:53, Joe Hershberger <joe.hershberger at gmail.com> wrote:
>> Hi Simon,
>>
>> On Tue, Aug 11, 2015 at 8:39 AM, Simon Glass <sjg at chromium.org> wrote:
>>> Update this driver to support driver model.
>>>
>>> Signed-off-by: Simon Glass <sjg at chromium.org>
>>> ---
>>
>> Acked-by: Joe Hershberger <joe.hershberger at ni.com>
>>
>> ...but a few nits below.
>>
>>>
>>> Changes in v2: None
>>>
>>>  drivers/net/e1000.c | 137 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>  drivers/net/e1000.h |   4 ++
>>>  2 files changed, 141 insertions(+)
>>>
>>> diff --git a/drivers/net/e1000.c b/drivers/net/e1000.c
>>> index 11bf9ca..25d0b39 100644
>>> --- a/drivers/net/e1000.c
>>> +++ b/drivers/net/e1000.c
>>> @@ -30,6 +30,7 @@ tested on both gig copper and gig fiber boards
>>>   */
>>>
>>>  #include <common.h>
>>> +#include <dm.h>
>>>  #include <errno.h>
>>>  #include <pci.h>
>>>  #include "e1000.h"
>>> @@ -47,12 +48,21 @@ tested on both gig copper and gig fiber boards
>>>  /* Intel i210 needs the DMA descriptor rings aligned to 128b */
>>>  #define E1000_BUFFER_ALIGN     128
>>>
>>> +/*
>>> + * TODO(sjg at chromium.org): Even with driver model we share these buffers.
>>> + * Concurrent receiving on multiple active Ethernet devices will not work.
>>> + * Normally U-Boot does not support this anyway. To fix it in this driver,
>>
>> It is true that we don't support this right now. Eventually we will,
>> but I'm sure we will have to revisit every driver anyway, so this is
>> fine for now.
>>
>>> + * nove these buffers and the tx/rx pointers to struct e1000_hw.
>>
>> move
>>
>>> + */
>>>  DEFINE_ALIGN_BUFFER(struct e1000_tx_desc, tx_base, 16, E1000_BUFFER_ALIGN);
>>>  DEFINE_ALIGN_BUFFER(struct e1000_rx_desc, rx_base, 16, E1000_BUFFER_ALIGN);
>>>  DEFINE_ALIGN_BUFFER(unsigned char, packet, 4096, E1000_BUFFER_ALIGN);
>>>
>>>  static int tx_tail;
>>>  static int rx_tail, rx_last;
>>> +#ifdef CONFIG_DM_ETH
>>> +static int num_cards;  /* Number of E1000 devices seen so far */
>>> +#endif
>>>
>>>  static struct pci_device_id e1000_supported[] = {
>>>         { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82542) },
>>> @@ -5279,8 +5289,10 @@ void e1000_get_bus_type(struct e1000_hw *hw)
>>>         }
>>>  }
>>>
>>> +#ifndef CONFIG_DM_ETH
>>>  /* A list of all registered e1000 devices */
>>>  static LIST_HEAD(e1000_hw_list);
>>> +#endif
>>>
>>>  static int e1000_init_one(struct e1000_hw *hw, int cardnum, pci_dev_t devno,
>>>                           unsigned char enetaddr[6])
>>> @@ -5367,6 +5379,7 @@ static void e1000_name(char *str, int cardnum)
>>>         sprintf(str, "e1000#%u", cardnum);
>>>  }
>>>
>>> +#ifndef CONFIG_DM_ETH
>>>  /**************************************************************************
>>>  TRANSMIT - Transmit a frame
>>>  ***************************************************************************/
>>> @@ -5479,13 +5492,22 @@ struct e1000_hw *e1000_find_card(unsigned int cardnum)
>>>
>>>         return NULL;
>>>  }
>>> +#endif /* nCONFIG_DM_ETH */
>>
>> Usually such a comment looks like this instead:
>>
>> +#endif /* !CONFIG_DM_ETH */
>>
>> but I don't care that much.
>>
>>>
>>>  #ifdef CONFIG_CMD_E1000
>>>  static int do_e1000(cmd_tbl_t *cmdtp, int flag,
>>>                 int argc, char * const argv[])
>>>  {
>>>         unsigned char *mac = NULL;
>>> +#ifdef CONFIG_DM_ETH
>>> +       struct eth_pdata *plat;
>>> +       struct udevice *dev;
>>> +       char name[30];
>>> +       int ret;
>>> +#else
>>>         struct e1000_hw *hw;
>>> +#endif
>>> +       int cardnum;
>>>
>>>         if (argc < 3) {
>>>                 cmd_usage(cmdtp);
>>> @@ -5494,9 +5516,18 @@ static int do_e1000(cmd_tbl_t *cmdtp, int flag,
>>>
>>>         /* Make sure we can find the requested e1000 card */
>>>         cardnum = simple_strtoul(argv[1], NULL, 10);
>>> +#ifdef CONFIG_DM_ETH
>>> +       e1000_name(name, cardnum);
>>> +       ret = uclass_get_device_by_name(UCLASS_ETH, name, &dev);
>>> +       if (!ret) {
>>> +               plat = dev_get_platdata(dev);
>>> +               mac = plat->enetaddr;
>>> +       }
>>> +#else
>>>         hw = e1000_find_card(cardnum);
>>>         if (hw)
>>>                 mac = hw->nic->enetaddr;
>>> +#endif
>>>         if (!mac) {
>>>                 printf("e1000: ERROR: No such device: e1000#%s\n", argv[1]);
>>>                 return 1;
>>> @@ -5531,3 +5562,109 @@ U_BOOT_CMD(
>>>         "       - Manage the Intel E1000 PCI device"
>>>  );
>>>  #endif /* not CONFIG_CMD_E1000 */
>>> +
>>> +#ifdef CONFIG_DM_ETH
>>> +static int e1000_eth_start(struct udevice *dev)
>>> +{
>>> +       struct eth_pdata *plat = dev_get_platdata(dev);
>>> +       struct e1000_hw *hw = dev_get_priv(dev);
>>
>> Did you ever decide what to do with these type of accessors to add a
>> little type safety?
>
> Not really. I like your idea of asking people to add an accessor
> function for each driver. But I have not gone back to it.
>
>>
>>> +
>>> +       return _e1000_init(hw, plat->enetaddr);
>>> +}
>>> +
>>> +static void e1000_eth_stop(struct udevice *dev)
>>> +{
>>> +       struct e1000_hw *hw = dev_get_priv(dev);
>>> +
>>> +       _e1000_disable(hw);
>>> +}
>>> +
>>> +static int e1000_eth_send(struct udevice *dev, void *packet, int length)
>>> +{
>>> +       struct e1000_hw *hw = dev_get_priv(dev);
>>> +       int ret;
>>> +
>>> +       ret = _e1000_transmit(hw, packet, length);
>>> +
>>> +       return ret ? 0 : -ETIMEDOUT;
>>> +}
>>> +
>>> +static int e1000_eth_recv(struct udevice *dev, int flags, uchar **packetp)
>>> +{
>>> +       struct e1000_hw *hw = dev_get_priv(dev);
>>> +       int len;
>>> +
>>> +       len = _e1000_poll(hw);
>>> +       if (len)
>>> +               *packetp = packet;
>>> +
>>> +       return len ? len : -EAGAIN;
>>> +}
>>> +
>>> +static int e1000_free_pkt(struct udevice *dev, uchar *packet, int length)
>>> +{
>>> +       struct e1000_hw *hw = dev_get_priv(dev);
>>> +
>>> +       fill_rx(hw);
>>> +
>>> +       return 0;
>>> +}
>>> +
>>> +static int e1000_eth_probe(struct udevice *dev)
>>> +{
>>> +       struct eth_pdata *plat = dev_get_platdata(dev);
>>> +       struct e1000_hw *hw = dev_get_priv(dev);
>>> +       int ret;
>>> +
>>> +       hw->name = dev->name;
>>> +       ret = e1000_init_one(hw, trailing_strtol(dev->name), pci_get_bdf(dev),
>>> +                            plat->enetaddr);
>>> +       if (ret < 0) {
>>> +               printf(pr_fmt("failed to initialize card: %d\n"), ret);
>>> +               return ret;
>>> +       }
>>> +
>>> +       return 0;
>>> +}
>>> +
>>> +static int e1000_eth_bind(struct udevice *dev)
>>> +{
>>> +       char name[20];
>>> +
>>> +       /*
>>> +        * A simple way to number the devices. When device tree is used this
>>> +        * is unnecessary, but when the device is just discovered on the PCI
>>> +        * bus we need a name. We could instead have the uclass figure out
>>> +        * which devices are different and number them.
>>> +        */
>>
>> I guess it depends if we expect to see this pattern in many drivers.
>
> I suspect we may want to address this, but let's see.
>
>>
>>> +       e1000_name(name, num_cards++);
>>> +
>>> +       return device_set_name(dev, name);
>>> +}
>>> +
>>> +static const struct eth_ops e1000_eth_ops = {
>>> +       .start  = e1000_eth_start,
>>> +       .send   = e1000_eth_send,
>>> +       .recv   = e1000_eth_recv,
>>> +       .stop   = e1000_eth_stop,
>>> +       .free_pkt = e1000_free_pkt,
>>> +};
>>> +
>>> +static const struct udevice_id e1000_eth_ids[] = {
>>> +       { .compatible = "realtek,e1000" },
>>> +       { }
>>> +};
>>> +
>>> +U_BOOT_DRIVER(eth_e1000) = {
>>> +       .name   = "eth_e1000",
>>> +       .id     = UCLASS_ETH,
>>> +       .of_match = e1000_eth_ids,
>>> +       .bind   = e1000_eth_bind,
>>> +       .probe  = e1000_eth_probe,
>>> +       .ops    = &e1000_eth_ops,
>>> +       .priv_auto_alloc_size = sizeof(struct e1000_hw),
>>> +       .platdata_auto_alloc_size = sizeof(struct eth_pdata),
>>> +};
>>> +
>>> +U_BOOT_PCI_DEVICE(eth_e1000, e1000_supported);
>>> +#endif
>>> diff --git a/drivers/net/e1000.h b/drivers/net/e1000.h
>>> index bfacd4e..543459d 100644
>>> --- a/drivers/net/e1000.h
>>> +++ b/drivers/net/e1000.h
>>> @@ -22,7 +22,9 @@
>>>  #include <linux/list.h>
>>>  #include <malloc.h>
>>>  #include <net.h>
>>> +#ifndef CONFIG_DM_ETH
>>>  #include <netdev.h>
>>> +#endif
>>
>> Is there a need to not include this in the DM case? Or are you just
>> trying to document what should be removed when !DM is purged?
>> Typically I would not #ifdef an include.
>
> I gives a build error at present.

OK. I guess it would be good to know what the error is and if it's
easily addressable.

>>>  #include <asm/io.h>>>>  #include <pci.h>
>>>
>>> @@ -1073,7 +1075,9 @@ typedef enum {
>>>  struct e1000_hw {
>>>         const char *name;
>>>         struct list_head list_node;
>>> +#ifndef CONFIG_DM_ETH
>>>         struct eth_device *nic;
>>> +#endif
>>>  #ifdef CONFIG_E1000_SPI
>>>         struct spi_slave spi;
>>>  #endif
>>> --
>>> 2.5.0.rc2.392.g76e840b
>
> Regards,
> Simon


More information about the U-Boot mailing list