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
