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.
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?
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.
+};
+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.
(Also remove internal brackets)
If priv->current is NULL, this will die.
Also to avoid confusion I think you should use uc_priv for the uclass priv local variable, to distinguish it from priv.
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)
+}
+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.
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.
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?
+}
+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.
But still I wonder if you should have functions that are passed a udevice, like eth_rx(struct udevice *dev).
+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't match:\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 net device\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 illegal value\n",dev->name, priv->enetaddr);return -EINVAL;}ret = ops->write_hwaddr(dev);if (ret)printf("\nWarning: %s failed to set MAC address\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?
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?
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)
Otherwise I'm not sure what this function is trying to do.
}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.
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?
if (strchr(dev->name, ' '))printf("\nWarning: eth device name \"%s\" has a space!\n",dev->name);
BTW if this is an error you could refuse to bind it - e.g. in the eth_post_bind, return -EINVAL
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 */-- 1.7.11.5
Regards, Simon