Hi Simon, On Tue, Aug 11, 2015 at 12:17 PM, Simon Glass <s...@chromium.org> wrote: > 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.
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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot