Hi Joe, On 11 August 2015 at 09:53, Joe Hershberger <joe.hershber...@gmail.com> wrote: > Hi Simon, > > On Tue, Aug 11, 2015 at 8:39 AM, Simon Glass <s...@chromium.org> wrote: >> Update this driver to support driver model. >> >> Signed-off-by: Simon Glass <s...@chromium.org> >> --- > > Acked-by: Joe Hershberger <joe.hershber...@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(s...@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. > >> #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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot