Hi Joe,
On 28 January 2015 at 03:22, Joe Hershberger joe.hershberger@gmail.com wrote:
On Tue, Jan 27, 2015 at 8:34 PM, Simon Glass sjg@chromium.org wrote:
Hi Joe,
On 27 January 2015 at 16:27, Joe Hershberger joe.hershberger@ni.com wrote:
First just add support for MAC drivers.
I don't fully understand this partly because my knowledge of the network stack is limited. So I'll make a few comments and we can go from there.
Sounds good.
Signed-off-by: Joe Hershberger joe.hershberger@ni.com
common/board_r.c | 4 +- common/cmd_bdinfo.c | 2 + include/dm/uclass-id.h | 1 + include/net.h | 23 ++++ net/eth.c | 320 ++++++++++++++++++++++++++++++++++++++++++++++++- 5 files changed, 346 insertions(+), 4 deletions(-)
diff --git a/common/board_r.c b/common/board_r.c index a301cc2..9a41cae 100644 --- a/common/board_r.c +++ b/common/board_r.c @@ -572,7 +572,7 @@ static int initr_bbmii(void) } #endif
-#ifdef CONFIG_CMD_NET +#if defined(CONFIG_CMD_NET) && !defined(CONFIG_DM_ETH) static int initr_net(void) { puts("Net: "); @@ -841,7 +841,7 @@ init_fnc_t init_sequence_r[] = { #ifdef CONFIG_BITBANGMII initr_bbmii, #endif -#ifdef CONFIG_CMD_NET +#if defined(CONFIG_CMD_NET) && !defined(CONFIG_DM_ETH) INIT_FUNC_WATCHDOG_RESET initr_net, #endif diff --git a/common/cmd_bdinfo.c b/common/cmd_bdinfo.c index e6d8a7a..8688cf9 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/include/dm/uclass-id.h b/include/dm/uclass-id.h index f17c3c2..b04cbc9 100644 --- a/include/dm/uclass-id.h +++ b/include/dm/uclass-id.h @@ -33,6 +33,7 @@ enum uclass_id { UCLASS_I2C, /* I2C bus */ UCLASS_I2C_GENERIC, /* Generic I2C device */ UCLASS_I2C_EEPROM, /* I2C EEPROM device */
UCLASS_ETH, /* Network device */Ethernet device?
Sure.
UCLASS_COUNT, UCLASS_INVALID = -1,diff --git a/include/net.h b/include/net.h index 7eef9cc..25636e2 100644 --- a/include/net.h +++ b/include/net.h @@ -78,6 +78,29 @@ enum eth_state_t { ETH_STATE_ACTIVE };
+#ifdef CONFIG_DM_ETH +struct eth_pdata {
phys_addr_t iobase;+};
+struct eth_ops {
int (*init)(struct udevice *dev, bd_t *bis);int (*send)(struct udevice *dev, void *packet, int length);int (*recv)(struct udevice *dev);void (*halt)(struct udevice *dev);+#ifdef CONFIG_MCAST_TFTP
int (*mcast)(struct udevice *dev, const u8 *enetaddr, u8 set);+#endif
int (*write_hwaddr)(struct udevice *dev);+};
+struct udevice *eth_get_dev(void); /* get the current device */ +unsigned char *eth_get_ethaddr(void); /* get the current device MAC */ +int eth_init_state_only(bd_t *bis); /* 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]; diff --git a/net/eth.c b/net/eth.c index c02548c..d245b65 100644 --- a/net/eth.c +++ b/net/eth.c @@ -72,6 +72,321 @@ static int eth_mac_skip(int index) return ((skip_state = getenv(enetvar)) != NULL); }
+static void eth_current_changed(void);
+#ifdef CONFIG_DM_ETH +#include <dm.h>
+struct eth_device_priv {
unsigned char enetaddr[6];int state;int index;void *priv;Suggestion: you could have per-child platform data as well as per-child private data. See u-boot-dm/master for this. It is used for I2C and SPI to hold the address of the child on the bus.
You could use it to hold the MAC address. I think this *might* be better because the MAC address can then be set and retained even when the device is probed/removed.
So in net.h you'll see that I have a platdata structure as well... the reason I didn't put the MAC address in there is that the data is not sourced from the device tree or the platform data in general. It either comes from the env (default) or it is read from the network MAC's eeprom. I'll need a little more convincing that the MAC should move.
OK. If it only exists when the device is active (probed) then that is fine.
+};
+struct eth_uclass_priv {
struct udevice *current;int max_index;+};
+static void eth_set_current_to_next(void) +{
struct uclass *uc;struct eth_uclass_priv *priv;uclass_get(UCLASS_ETH, &uc);priv = uc->priv;uclass_next_device(&(priv->current));Note that this will probe the device. I think you are storing the current ethernet device in the uclass, but you could just as well have a static variable in this file if that is easier.
I started out using a static variable for the current device, but other code in u-boot uses the current device (via the eth_get_dev() function) and I want it to not require and extern to a non-static variable.
OK.
(Also remove internal brackets)
Yes... forgot to clean this up.
If priv->current is NULL, this will die.
This function should not get called if priv->current is NULL.
Also to avoid confusion I think you should use uc_priv for the uclass priv local variable, to distinguish it from priv.
OK.
if (!priv->current)uclass_first_device(UCLASS_ETH, &(priv->current));If I understand this correctly, I think you want:
if (priv->current) uclass_next_device(&priv->current) else uclass_first_device(uc, &priv->current)
No... I'm not switching on priv->current being NULL... I get the next, and then if THAT is NULL, I start at the beginning. priv->current must never be NULL (outside of this transition).
This is emulating the behavior that used to exist where the linked list instead of terminating linked back to the head. This way we always cycle through all adapters and never stop.
OK I see.
+}
+struct udevice *eth_get_dev(void) +{
struct uclass *uc;uclass_get(UCLASS_ETH, &uc);struct eth_uclass_priv *priv = uc->priv;This is OK so long as the device has been probed. I think it always is because you use uclass_next_device() to get the device.
Why would the device being probed have any relationship with the priv in the uclass itself? I would expect the uclass priv to be there regardless. This is not the per-device uc_priv.
Yes you are right, I was confused by the 'priv' name.
return priv->current;+}
+static void eth_set_dev(struct udevice *dev) +{
struct uclass *uc;uclass_get(UCLASS_ETH, &uc);struct eth_uclass_priv *priv = uc->priv;priv->current = dev;+}
+unsigned char *eth_get_ethaddr(void) +{
struct eth_device_priv *priv;if (eth_get_dev()) {priv = eth_get_dev()->uclass_priv;if (priv)This check should be unnecessary. Same in other cases below.
So this is guaranteed? In all cases? I ran into some pre-allocated buffers that don't get created if post_probe returns an error, for instance.
If the device is probed then it has uclass_priv. If post_probe() returns an error then the device will be removed (i.e. the probe will fail).
return priv->enetaddr;}return NULL;+}
+/* Set active state */ +int eth_init_state_only(bd_t *bis) +{
struct eth_device_priv *priv;if (eth_get_dev()) {priv = eth_get_dev()->uclass_priv;if (priv)priv->state = ETH_STATE_ACTIVE;}return 0;+} +/* Set passive state */ +void eth_halt_state_only(void) +{
struct eth_device_priv *priv;if (eth_get_dev()) {priv = eth_get_dev()->uclass_priv;if (priv)priv->state = ETH_STATE_PASSIVE;}+}
+int eth_get_dev_index(void) +{
struct eth_device_priv *priv;if (eth_get_dev()) {priv = eth_get_dev()->uclass_priv;if (priv)return priv->index;}return -1;+}
+int eth_init(bd_t *bis) +{
struct udevice *current, *old_current, *dev;struct uclass *uc;current = eth_get_dev();if (!current) {puts("No ethernet found.\n");return -1;}/* Sync environment with network devices */uclass_get(UCLASS_ETH, &uc);uclass_foreach_dev(dev, uc) {uchar env_enetaddr[6];if (eth_getenv_enetaddr_by_index("eth", dev->seq,env_enetaddr)) {struct eth_device_priv *priv = dev->uclass_priv;if (priv)memcpy(priv->enetaddr, env_enetaddr, 6);}};old_current = current;do {debug("Trying %s\n", current->name);if (current->driver) {const struct eth_ops *ops =current->driver->ops;
if (ops->init(current, bis) >= 0) {struct eth_device_priv *priv =current->uclass_priv;if (priv)priv->state = ETH_STATE_ACTIVE;return 0;}}debug("FAIL\n");eth_try_another(0);current = eth_get_dev();} while (old_current != current);return -1;-ENODEV?
OK.
+}
+void eth_halt(void) +{
struct udevice *current;current = eth_get_dev();if (!current)return;if (!current->driver)return;const struct eth_ops *ops = current->driver->ops;ops->halt(current);struct eth_device_priv *priv = current->uclass_priv;if (priv)priv->state = ETH_STATE_PASSIVE;+}
+int eth_send(void *packet, int length) +{
struct udevice *current;current = eth_get_dev();if (!current)return -1;if (!current->driver)return -1;const struct eth_ops *ops = current->driver->ops;return ops->send(current, packet, length);+}
As a general comment, there is an implicit assumption that only one device can be active at a time. I suppose that is a U-Boot limitation, but we don't need to keep it for driver model, or at least we could permit multiple devices to be probed at a time.
I agree that longer term we should look for ways to remove that limitation, but for now everything that uses the network assumes that the "ethact" env var selects the device. There would be a lot of commands to break compatibility with to allow them to specify the device. I say we hold off on that for now.
OK
One thing to be careful of is probing devices before you use them. By using uclass_first_device() you will probe the device. If you want to pick a particular device and probe it, you will need to find the device through another means - e.g. a helper function in the uclass to find the device without probing it.
But still I wonder if you should have functions that are passed a udevice, like eth_rx(struct udevice *dev).
Even if those functions did, the commands would need to change too. Future enhancement, I think.
OK
+int eth_rx(void) +{
struct udevice *current;current = eth_get_dev();if (!current)return -1;if (!current->driver)return -1;const struct eth_ops *ops = current->driver->ops;return ops->recv(current);+}
+static int eth_write_hwaddr(struct udevice *dev, const char *base_name,
int eth_number)+{
unsigned char env_enetaddr[6];int ret = 0;eth_getenv_enetaddr_by_index(base_name, eth_number,env_enetaddr);
struct eth_device_priv *priv = dev->uclass_priv;if (!is_zero_ether_addr(env_enetaddr)) {if (!is_zero_ether_addr(priv->enetaddr) &&memcmp(priv->enetaddr, env_enetaddr, 6)) {printf("\nWarning: %s MAC addresses don'tmatch:\n",
dev->name);printf("Address in SROM is %pM\n",priv->enetaddr);printf("Address in environment is %pM\n",env_enetaddr);}memcpy(priv->enetaddr, env_enetaddr, 6);} else if (is_valid_ether_addr(priv->enetaddr)) {eth_setenv_enetaddr_by_index(base_name, eth_number,priv->enetaddr);printf("\nWarning: %s using MAC address from netdevice\n",
dev->name);} else if (is_zero_ether_addr(priv->enetaddr)) {printf("\nError: %s address not set.\n",dev->name);return -EINVAL;}const struct eth_ops *ops = dev->driver->ops;if (ops->write_hwaddr && !eth_mac_skip(eth_number)) {if (!is_valid_ether_addr(priv->enetaddr)) {printf("\nError: %s address %pM illegalvalue\n",
dev->name, priv->enetaddr);return -EINVAL;}ret = ops->write_hwaddr(dev);if (ret)printf("\nWarning: %s failed to set MACaddress\n",
dev->name);}return ret;+}
+static int eth_uclass_init(struct uclass *class) +{
bootstage_mark(BOOTSTAGE_ID_NET_ETH_START);struct eth_uclass_priv *priv = class->priv;priv->max_index = 0;What is max_index used for?
For knowing the number of devices that have been bound and allocating the next unused index for the next device.
Is this different from the number of devices in the uclass?
eth_env_init();return 0;+}
+static int eth_post_bind(struct udevice *dev) +{
struct udevice *first;uclass_first_device(UCLASS_ETH, &first);if (first == dev) {eth_current_changed();eth_set_dev(dev);}struct eth_device_priv *priv = dev->uclass_priv;struct eth_uclass_priv *uclass_priv = dev->uclass->priv;if (priv) {priv->state = ETH_STATE_INIT;priv->index = uclass_priv->max_index++;OK I see it is the number of devices. Does this ever decrease?
Never decreases.
Anyway, struct udevice has a seq member which can give every device a unique sequence number in the uclass automatically (and if you define DM_UC_FLAG_SEQ_ALIAS then the device tree aliases can provide this numbering)
I actually used the seq number originally and that worked fine with platdata. When I moved to the device tree, that's where it fell apart. In the device tree, it is expected that the eth name will include an eth@baseaddr format. The DM code then takes that value that is the baseaddr and sets it as the requested seq. That means I get eth268443648 instead of eth0. No good. So now I keep track of my own seq (index) to make sure I get consistent device names.
That should be fixed now - see u-boot-dm/master. Also the alias approach is useful since it lets you number the devices within the uclass as you wish.
Otherwise I'm not sure what this function is trying to do.
I needs to initialize the priv->current variable so that everything that tries to use the network finds a current eth device. It also initializes the state variable, which is also important.
OK, also you can't really probe a device in the post_bind() handler. Probing should only happen when the device is used.
}return 0;+}
+static int eth_pre_unbind(struct udevice *dev) +{
struct udevice *first;struct udevice *current;current = eth_get_dev();uclass_first_device(UCLASS_ETH, &first);if (current == dev) {if (dev == first)uclass_next_device(¤t);elsecurrent = first;eth_current_changed();eth_set_dev(current);}I'm not sure what this function is trying to do.
Again, this is just ensuring the "current" variable is always valid.
OK. Shouldn't this happen when the device is removed rather than when it is unbound (well I suppose if it happens when removed it doesn't need to happen when unbound).
return 0;+}
+static int eth_post_probe(struct udevice *dev) +{
char *ethprime = getenv("ethprime");if (ethprime && strcmp(dev->name, ethprime) == 0)eth_set_dev(dev);What does this do?
This allows the initial "current" device to be specified by they "ethprime" env var. This is preserving existing behaviors as controlled by these env vars.
OK
if (strchr(dev->name, ' '))printf("\nWarning: eth device name \"%s\" has aspace!\n",
dev->name);BTW if this is an error you could refuse to bind it - e.g. in the eth_post_bind, return -EINVAL
Good idea.
struct eth_device_priv *priv = dev->uclass_priv;if (priv)return eth_write_hwaddr(dev, "eth", priv->index);return 1;+}
+UCLASS_DRIVER(eth) = {
.name = "eth",.id = UCLASS_ETH,.post_bind = eth_post_bind,.pre_unbind = eth_pre_unbind,.post_probe = eth_post_probe,.init = eth_uclass_init,.priv_auto_alloc_size = sizeof(struct eth_uclass_priv),.per_device_auto_alloc_size = sizeof(struct eth_device_priv),+}; +#endif
+#ifndef CONFIG_DM_ETH /*
- CPU and board-specific Ethernet initializations. Aliased function
- signals caller to move on
@@ -423,6 +738,7 @@ int eth_rx(void)
return eth_current->recv(eth_current);} +#endif /* ifndef CONFIG_DM_ETH */
#ifdef CONFIG_API static void eth_save_packet(void *packet, int length) @@ -486,7 +802,7 @@ static void eth_current_changed(void)
void eth_try_another(int first_restart) {
static struct eth_device *first_failed;
static void *first_failed; char *ethrotate; /*@@ -515,7 +831,7 @@ void eth_set_current(void) { static char *act; static int env_changed_id;
struct eth_device *old_current;
void *old_current; int env_id; if (!eth_get_dev()) /* XXX no current */
Regards, Simon