Hi Joe,
On 1 March 2015 at 14:45, Joe Hershberger <joe.hershber...@gmail.com> wrote:
> Hi Simon,
>
> On Sun, Mar 1, 2015 at 12:07 PM, Simon Glass <s...@chromium.org> wrote:
>>
>> Hi Joe,
>>
>> On 24 February 2015 at 17:02, Joe Hershberger <joe.hershber...@ni.com>
>> wrote:
>> > First just add support for MAC drivers.
>> >
>> > Signed-off-by: Joe Hershberger <joe.hershber...@ni.com>
>>
>> Reviewed-by: Simon Glass <s...@chromium.org>
>>
>> This looks right to me. I still have some comments on error handling,
>> but I'm OK with you addressing these in a follow-on patch if you like.
>
> Thanks for going back and forth on this to make it right.
I'm pleased to see it coming together.
>
>> > ---
>> >
>> > Changes in v4:
>> > -Redo the seq / probe implementation
>> > --Don't prevent eth_initialize on driver model
>> > --Use eth_initialize to probe all devices and write_hwaddr
>> > --Look up MAC address in post-probe
>> > --Include ethprime handling in eth_initialize
>> > --If current == NULL, always check if there is a device available in
>> > eth_get_dev
>> > --Move env init call from uclass init to eth_initialize
>> > --Print the alias in eth_initialize
>> > -Stop handling selecting a new "current" in pre-unbind as it will now
>> > work itself out by clearing the pointer
>> > -Change -1 returns to error constants
>> > -Remove bd_t *bis from dm eth_ops init function
>> > -Add documentation to the structures
>> > -Add a helper function for eth_uclass_priv
>> > -Change puts to printf
>> > -Add eth_get_ops helper
>> > -Rename init() to start() in ops
>> > -Rename halt() to stop() in ops
>> > -Remove checks for driver==NULL
>> > -Remove priv pointer in per-device priv struct (drivers already get
>> > their own directly from DM)
>> >
>> > Changes in v3:
>> > -Correct the pre_unbind logic
>> > -Correct failure chaining from bind to probe to init
>> > --Fail init if not activated
>> > --Fail probe if ethaddr not set
>> > -Update ethaddr from env unconditionally on init
>> > -Use set current to select the current device regardless of the previous
>> > selection
>> > -Allow current eth dev to be NULL
>> > -Fixed blank line formatting for variable declaration
>> >
>> > Changes in v2:
>> > -Updated comments
>> > -Removed extra parentheses
>> > -Changed eth_uclass_priv local var names to be uc_priv
>> > -Update error codes
>> > -Cause an invalid name to fail binding
>> > -Rebase on top of dm/master
>> > -Stop maintaining our own index and use DM seq now that it works for our
>> > needs
>> > -Move the hwaddr to platdata so that its memory is allocated at bind
>> > when we need it
>> > -Prevent device from being probed before used by a command (i.e. before
>> > eth_init()).
>> >
>> > common/cmd_bdinfo.c | 2 +
>> > drivers/net/Kconfig | 5 +
>> > include/dm/uclass-id.h | 1 +
>> > include/net.h | 52 ++++++++
>> > net/eth.c | 345
>> > ++++++++++++++++++++++++++++++++++++++++++++++++-
>> > 5 files changed, 399 insertions(+), 6 deletions(-)
>> >
>> > diff --git a/common/cmd_bdinfo.c b/common/cmd_bdinfo.c
>> > index aa81da2..b4cce25 100644
>> > --- a/common/cmd_bdinfo.c
>> > +++ b/common/cmd_bdinfo.c
>> > @@ -34,6 +34,7 @@ static void print_eth(int idx)
>> > printf("%-12s= %s\n", name, val);
>> > }
>> >
>> > +#ifndef CONFIG_DM_ETH
>> > __maybe_unused
>> > static void print_eths(void)
>> > {
>> > @@ -52,6 +53,7 @@ static void print_eths(void)
>> > printf("current eth = %s\n", eth_get_name());
>> > printf("ip_addr = %s\n", getenv("ipaddr"));
>> > }
>> > +#endif
>> >
>> > __maybe_unused
>> > static void print_lnum(const char *name, unsigned long long value)
>> > diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
>> > index e69de29..bdd0f05 100644
>> > --- a/drivers/net/Kconfig
>> > +++ b/drivers/net/Kconfig
>> > @@ -0,0 +1,5 @@
>> > +config DM_ETH
>> > + bool "Enable Driver Model for Ethernet drivers"
>> > + depends on DM
>> > + help
>> > + Enable driver model for Ethernet.
>>
>> Here you could mention that the eth_...() interface is then
>> implemented by the Ethernet uclass. Perhaps a few other notes too? See
>> for example drivers/spi/Kconfig or drivers/gpio/Kconfig.
>
> OK
>
>> > diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
>> > index 91bb90d..ad96682 100644
>> > --- a/include/dm/uclass-id.h
>> > +++ b/include/dm/uclass-id.h
>> > @@ -34,6 +34,7 @@ enum uclass_id {
>> > UCLASS_I2C_GENERIC, /* Generic I2C device */
>> > UCLASS_I2C_EEPROM, /* I2C EEPROM device */
>> > UCLASS_MOD_EXP, /* RSA Mod Exp device */
>> > + UCLASS_ETH, /* Ethernet device */
>> >
>> > UCLASS_COUNT,
>> > UCLASS_INVALID = -1,
>> > diff --git a/include/net.h b/include/net.h
>> > index 10d38f8..508c572 100644
>> > --- a/include/net.h
>> > +++ b/include/net.h
>> > @@ -78,6 +78,57 @@ enum eth_state_t {
>> > ETH_STATE_ACTIVE
>> > };
>> >
>> > +#ifdef CONFIG_DM_ETH
>> > +/**
>> > + * struct eth_pdata - Platform data for Ethernet MAC controllers
>> > + *
>> > + * @iobase: The base address of the hardware registers
>> > + * @enetaddr: The Ethernet MAC address that is loaded from EEPROM or
>> > env
>> > + */
>> > +struct eth_pdata {
>> > + phys_addr_t iobase;
>> > + unsigned char enetaddr[6];
>> > +};
>> > +
>> > +/**
>> > + * struct eth_ops - functions of Ethernet MAC controllers
>> > + *
>> > + * start: Prepare the hardware to send and receive packets
>> > + * send: Send the bytes passed in "packet" as a packet on the wire
>> > + * recv: Check if the hardware received a packet. Call the network
>> > stack if so
>> > + * stop: Stop the hardware from looking for packets - may be called
>> > even if
>> > + * state == PASSIVE
>> > + * mcast: Join or leave a multicast group (for TFTP) - optional
>> > + * write_hwaddr: Write a MAC address to the hardware (used to pass it
>> > to Linux
>> > + * on some platforms like ARM). This function expects the
>> > + * eth_pdata::enetaddr field to be populated - optional
>> > + * read_rom_hwaddr: Some devices have a backup of the MAC address
>> > stored in a
>> > + * ROM on the board. This is how the driver should
>> > expose it
>> > + * to the network stack. This function should fill in
>> > the
>> > + * eth_pdata::enetaddr field - optional
>
> I consider this one of the primary purposes that board-specific init exists
> for Ethernet. I think this will help to eliminate them. I'm interested in
> your thoughts about how to generically expose this to a board / SoC /
> driver. For now I was thinking that it would be up to the driver to fan out
> if needed, meaning that if the driver doesn't know the MAC, it has a
> board-hook function call, but if it does, like USB or PCI adapters, it won't
> have board hooks. [sorry for the run-on sentence] I feel it's non-ideal, but
> can work for now.
As a general principle I think we should avoid all board hooks. The
information (if needed) should be in the device tree / platform data
provided by the board, or perhaps set up by the board through some
sort of call. Of course the MAC address is in some ways a special
case. I don't have a good answer for this, but perhaps it will become
apparent once we have more Ethernet driver support?
>
>> > + */
>> > +struct eth_ops {
>> > + int (*start)(struct udevice *dev);
>> > + int (*send)(struct udevice *dev, void *packet, int length);
>> > + int (*recv)(struct udevice *dev);
>> > + void (*stop)(struct udevice *dev);
>> > +#ifdef CONFIG_MCAST_TFTP
>> > + int (*mcast)(struct udevice *dev, const u8 *enetaddr, int join);
>> > +#endif
>> > + int (*write_hwaddr)(struct udevice *dev);
>> > + int (*read_rom_hwaddr)(struct udevice *dev);
>> > +};
>> > +
>> > +#define eth_get_ops(dev) ((struct eth_ops *)(dev)->driver->ops)
>> > +
>> > +struct udevice *eth_get_dev(void); /* get the current device */
>> > +unsigned char *eth_get_ethaddr(void); /* get the current device MAC */
>> > +/* Used only when NetConsole is enabled */
>> > +int eth_init_state_only(void); /* Set active state */
>> > +void eth_halt_state_only(void); /* Set passive state */
>> > +#endif
>> > +
>> > +#ifndef CONFIG_DM_ETH
>> > struct eth_device {
>> > char name[16];
>> > unsigned char enetaddr[6];
>> > @@ -144,6 +195,7 @@ int eth_write_hwaddr(struct eth_device *dev, const
>> > char *base_name,
>> > int eth_number);
>> >
>> > int usb_eth_initialize(bd_t *bi);
>> > +#endif
>> >
>> > int eth_initialize(void); /* Initialize network subsystem
>> > */
>> > void eth_try_another(int first_restart); /* Change the device */
>> > diff --git a/net/eth.c b/net/eth.c
>> > index 7bbaac4..9c2dfb9 100644
>> > --- a/net/eth.c
>> > +++ b/net/eth.c
>> > @@ -1,12 +1,15 @@
>> > /*
>> > - * (C) Copyright 2001-2010
>> > + * (C) Copyright 2001-2015
>> > * Wolfgang Denk, DENX Software Engineering, w...@denx.de.
>> > + * Joe Hershberger, National Instruments
>> > *
>> > * SPDX-License-Identifier: GPL-2.0+
>> > */
>> >
>> > #include <common.h>
>> > #include <command.h>
>> > +#include <dm.h>
>> > +#include <dm/device-internal.h>
>>
>> The dm/ headers should go after the asm/ header below.
>
> What in the asm headers affects the dm headers? Or is it an aesthetics
> request?
>
>
>> > #include <net.h>
>> > #include <miiphy.h>
>> > #include <phy.h>
>> > @@ -74,6 +77,338 @@ static int eth_mac_skip(int index)
>> > return ((skip_state = getenv(enetvar)) != NULL);
>> > }
>> >
>> > +static void eth_current_changed(void);
>> > +
>> > +#ifdef CONFIG_DM_ETH
>> > +/**
>> > + * struct eth_device_priv - private structure for each Ethernet device
>> > + *
>> > + * @state: The state of the Ethernet MAC driver (defined by enum
>> > eth_state_t)
>> > + */
>> > +struct eth_device_priv {
>> > + enum eth_state_t state;
>> > +};
>> > +
>> > +/**
>> > + * struct eth_uclass_priv - The structure attached to the uclass itself
>> > + *
>> > + * @current: The Ethernet device that the network functions are using
>> > + */
>> > +struct eth_uclass_priv {
>> > + struct udevice *current;
>> > +};
>> > +
>> > +static struct eth_uclass_priv *eth_get_uclass_priv(void)
>> > +{
>> > + struct uclass *uc;
>> > +
>> > + uclass_get(UCLASS_ETH, &uc);
>> > + assert(uc);
>> > + return uc->priv;
>> > +}
>> > +
>> > +static void eth_set_current_to_next(void)
>> > +{
>> > + struct eth_uclass_priv *uc_priv;
>> > +
>> > + uc_priv = eth_get_uclass_priv();
>> > + if (uc_priv->current)
>> > + uclass_next_device(&uc_priv->current);
>> > + if (!uc_priv->current)
>> > + uclass_first_device(UCLASS_ETH, &uc_priv->current);
>> > +}
>> > +
>> > +struct udevice *eth_get_dev(void)
>>
>> This function needs a comment. It isn't clear what is it supposed to
>> return. Does it only return probed devices (in which case the check in
>> eth_halt() etc. for device_active() is redundant? Or can it return
>> devices which cannot be probed?
>
> In the current implementation it will return any device (probe-able or not),
> as it is the caller that has the logic to decide how to move on or not.
>
>> I suggest that it only returns probed devices, as it simplifies the code.
>
> I can evaluate that, but it is not currently expected anywhere.
Well I'll leave this to you.
>
>> > +{
>> > + if (!eth_get_uclass_priv()->current)
>> > + uclass_first_device(UCLASS_ETH,
>> > + ð_get_uclass_priv()->current);
>>
>> This can return an error. You will then eat it and return the device
>> anyway, even though uclass_first_device() will not change ->current.
>
> This can return the error from the probe(), but is the probe fails it also
> return a NULL pointer, which is what I'm using to determine success. I can't
> return the device in the error case because uclass_first_device() doesn't
> give it to me.
Overall (and maybe this is best figured out later) I think the
functions that swallow errors should have special names and be
commented that way. The innocuous name eth_get_dev() hides quite a few
features. Perhaps eth_scan_for_suitable_device() ?
>
>> > + return eth_get_uclass_priv()->current;
>>
>> Also I think it would be better to have a local variable here for
>> uc_priv, as in the above functino.
>
> OK
>
>> > +}
>> > +
>> > +static void eth_set_dev(struct udevice *dev)
>> > +{
>> > + device_probe(dev);
>>
>> This needs an error check, since if it fails you cannot use the
>> device. Also in eth_pre_unbind() you call this function with NULL.
>
> The device_probe() function already has that check, so I didn't bother
> adding it again here.
>
>> So I think this function should return an error.
>
> I'm not sure what other error it would return. If it's just the NULL
> pointer, the caller knows what it passed in. NULL pointer is valid here.
> Essentially asking to unset this device as current (as in pre_unbind
> handler).
You are relying on device_probe() returning an error when dev is NULL?
OK, I suppose. Could use a comment.
[snip]
Regards
Simon
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
