Hi Joe,
On 11 August 2015 at 09:53, Joe Hershberger joe.hershberger@gmail.com wrote:
Hi Simon,
On Tue, Aug 11, 2015 at 8:39 AM, Simon Glass sjg@chromium.org wrote:
Update this driver to support driver model.
Signed-off-by: Simon Glass sjg@chromium.org
Acked-by: Joe Hershberger joe.hershberger@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@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