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
