On Fri, Feb 6, 2015 at 7:25 PM, Simon Glass sjg@chromium.org wrote:
Hi Joe,
On 2 February 2015 at 17:38, Joe Hershberger joe.hershberger@ni.com
wrote:
First just add support for MAC drivers.
Signed-off-by: Joe Hershberger joe.hershberger@ni.com
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/board_r.c | 4 +- common/cmd_bdinfo.c | 2 + include/dm/uclass-id.h | 1 + include/net.h | 24 ++++ net/eth.c | 319
++++++++++++++++++++++++++++++++++++++++++++++++-
5 files changed, 346 insertions(+), 4 deletions(-)
diff --git a/common/board_r.c b/common/board_r.c index 68a9448..75147b7 100644 --- a/common/board_r.c +++ b/common/board_r.c @@ -556,7 +556,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: "); @@ -825,7 +825,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 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 7eef9cc..4d21d91 100644 --- a/include/net.h +++ b/include/net.h @@ -78,6 +78,30 @@ enum eth_state_t { ETH_STATE_ACTIVE };
+#ifdef CONFIG_DM_ETH
You may not need this?
I need it for the function prototypes that have a different device pointer parameter.
+struct eth_pdata {
phys_addr_t iobase;unsigned char enetaddr[6];+};
+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..1b5a169 100644 --- a/net/eth.c +++ b/net/eth.c @@ -72,6 +72,320 @@ 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> +#include <dm/device-internal.h>
These should go at the top of the file - should be OK to always include them (i.e. no #ifdef)
OK... Moved them.
+struct eth_device_priv {
int state;void *priv;+};
+struct eth_uclass_priv {
struct udevice *current;+};
+static void eth_set_current_to_next(void) +{
struct uclass *uc;struct eth_uclass_priv *uc_priv;uclass_get(UCLASS_ETH, &uc);This can actually return an error, although I agree there is little point in handling it. So I suppose this is OK.
I think this should be a given.
uc_priv = uc->priv;uclass_next_device(&uc_priv->current);if (!uc_priv->current)uclass_first_device(UCLASS_ETH, &uc_priv->current);+}
+struct udevice *eth_get_dev(void) +{
struct uclass *uc;blank line here
OK on all of these.
uclass_get(UCLASS_ETH, &uc);struct eth_uclass_priv *uc_priv = uc->priv;return uc_priv->current;+}
+static void eth_set_dev(struct udevice *dev) +{
struct uclass *uc;blank line here
uclass_get(UCLASS_ETH, &uc);struct eth_uclass_priv *uc_priv = uc->priv;uc_priv->current = dev;+}
+unsigned char *eth_get_ethaddr(void) +{
struct eth_pdata *pdata;blank line here
if (eth_get_dev()) {pdata = eth_get_dev()->platdata;if (pdata)return pdata->enetaddr;}return NULL;+}
+/* Set active state */ +int eth_init_state_only(bd_t *bis) +{
struct eth_device_priv *priv;blank line here
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;blank line here
if (eth_get_dev()) {priv = eth_get_dev()->uclass_priv;if (priv)priv->state = ETH_STATE_PASSIVE;}+}
+int eth_get_dev_index(void) +{
if (eth_get_dev())return eth_get_dev()->seq;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;}device_probe(current);Check error
OK.
/* 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_pdata *pdata = dev->platdata;blank line
Do all devices have the same platdata by design? What if a particular device wants its own?
That's a good question. I imagine some devices may have something unique, but I would expect they would read that data into the priv data that they define. How do other subsystems handle platform data for unique devices?
if (pdata)What do you need this check?
No. This was left over from when enetaddr was in priv.
memcpy(pdata->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;
blank line
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 -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;Seems like eth_get_dev() should return an error code if current or current->driver is NULL.
It's returning a pointer, so I prefer it to be NULL or a valid pointer.
Perhaps you meant to say that eth_send() should return an error code?
const struct eth_ops *ops = current->driver->ops;return ops->send(current, packet, length);+}
+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_pdata *pdata = dev->platdata;if (!is_zero_ether_addr(env_enetaddr)) {if (!is_zero_ether_addr(pdata->enetaddr) &&memcmp(pdata->enetaddr, env_enetaddr, 6)) {printf("\nWarning: %s MAC addresses don't
match:\n",
dev->name);printf("Address in SROM is %pM\n",pdata->enetaddr);printf("Address in environment is %pM\n",env_enetaddr);}memcpy(pdata->enetaddr, env_enetaddr, 6);} else if (is_valid_ether_addr(pdata->enetaddr)) {eth_setenv_enetaddr_by_index(base_name, eth_number,pdata->enetaddr);printf("\nWarning: %s using MAC address from net
device\n",
dev->name);} else if (is_zero_ether_addr(pdata->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(pdata->enetaddr)) {printf("\nError: %s address %pM illegal
value\n",
dev->name, pdata->enetaddr);return -EINVAL;}ret = ops->write_hwaddr(dev);if (ret)printf("\nWarning: %s failed to set MAC
address\n",
dev->name);}The above code seems mostly duplicated but I suppose it is hard to make it common?
It is, but I look forward to the day when we delete the other copy. It would be difficult to combine, for sure.
return ret;+}
+static int eth_uclass_init(struct uclass *class) +{
bootstage_mark(BOOTSTAGE_ID_NET_ETH_START);eth_env_init();return 0;+}
+static int eth_post_bind(struct udevice *dev) +{
if (strchr(dev->name, ' ')) {printf("\nError: eth device name \"%s\" has a space!\n",dev->name);return -EINVAL;}if (!eth_get_dev()) {eth_set_dev(dev);eth_current_changed();}I still don't really get what you are doing here. Perhaps you could add some comments? There should be no probed devices at this stage since you are binding only.
This has changed a fair amount in v3. Please comment there if it's more clear now.
char *ethprime = getenv("ethprime");if (ethprime && strcmp(dev->name, ethprime) == 0) {eth_set_dev(dev);eth_current_changed();}/** Devices need to write the hwaddr even if not probed so that
Linux
* will have access to the hwaddr that u-boot stored for the
device.
*/eth_write_hwaddr(dev, "eth", uclass_resolve_seq(dev));return 0;+}
+static int eth_pre_unbind(struct udevice *dev) +{
struct udevice *first = NULL;struct udevice *current;struct uclass *uc;uclass_get(UCLASS_ETH, &uc);uclass_foreach_dev(current, uc) {if (!first)first = current;if (current == dev) {if (dev == first) {current =
list_entry(current->uclass_node.next,
struct udevice, uclass_node);} else {current = first;}eth_set_dev(current);eth_current_changed();}}If this is turning down a device it really should happen in a remove() method. Maybe in post_remove()?
This has changed a fair amount in v3. Please comment there if it's more clear now.
return 0;+}
+static int eth_post_probe(struct udevice *dev) +{
struct eth_device_priv *priv = dev->uclass_priv;if (priv)priv->state = ETH_STATE_INIT;return 0;+}
+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 +737,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 +801,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 +830,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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot