[U-Boot] [PATCH v3 1/3] net: Add a command to manipulate ethernet devices
Add the 'eth' command for operations on ethernet devices. This first version only contains a command to show a device properties, currently only name, index and MAC address.
Signed-off-by: Alban Bedel alban.bedel@avionic-design.de --- v1: * Patch didn't exists v2: * Patch didn't exists v3: * Replace the dedicated 'eth_eeprom' command with a subcommand to the 'eth' command --- common/cmd_net.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+)
diff --git a/common/cmd_net.c b/common/cmd_net.c index 09489d4..9cc0bdf 100644 --- a/common/cmd_net.c +++ b/common/cmd_net.c @@ -445,3 +445,51 @@ U_BOOT_CMD( );
#endif /* CONFIG_CMD_LINK_LOCAL */ + +static int do_eth_show(struct eth_device *dev, + int argc, char * const argv[]) +{ + printf("Name : %s\n", dev->name); + printf("Index: %d\n", dev->index); + printf("MAC : %pM\n", dev->enetaddr); + return 0; +} + +static int do_eth(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) +{ + struct eth_device *dev; + char *endp = NULL; + int index; + + if (argc < 3) + return CMD_RET_USAGE; + + /* Get the ethernet device, by ID or by name */ + index = (int) simple_strtoul(argv[1], &endp, 16); + if (endp > argv[1]) + dev = eth_get_dev_by_index(index); + else + dev = eth_get_dev_by_name(argv[2]); + + if (!dev) { + printf("Ethernet device not found\n"); + return CMD_RET_FAILURE; + } + + if (!strcmp(argv[2], "show")) + return do_eth_show(dev, argc - 2, argv + 2); + + printf("Unknown eth sub command: %s\n", argv[2]); + + return CMD_RET_USAGE; +} + +U_BOOT_CMD( + eth, 7, 0, do_eth, + "extended ops for ethernet devices", + "<dev> <command> [<args>...]\n" + " - run command on a device, device may be a name or id.\n" + "\n" + "eth <dev> show\n" + " - show basic information about the ethernet device\n" +);
Many ethernet devices use an EEPROM to store various settings, most commonly the device MAC address. But on some devices it can contains a lot more, for example USB device might also have many USB related parameters.
This commit add a set of commands to read/write this EEPROM, write a default configuration and read/write the device MAC address. The defaults command allow priming the EEPROM for devices that need more than just a MAC address in the EEPROM.
Signed-off-by: Alban Bedel alban.bedel@avionic-design.de --- v2: * No changes since v1 v3: * Replace the dedicated 'eth_eeprom' command with a subcommand to the newly introduced 'eth' command --- common/cmd_net.c | 121 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ include/net.h | 28 +++++++++++++ net/eth.c | 46 +++++++++++++++++++++ 3 files changed, 195 insertions(+)
diff --git a/common/cmd_net.c b/common/cmd_net.c index 9cc0bdf..1c2e254 100644 --- a/common/cmd_net.c +++ b/common/cmd_net.c @@ -446,6 +446,109 @@ U_BOOT_CMD(
#endif /* CONFIG_CMD_LINK_LOCAL */
+#if defined(CONFIG_CMD_ETH_EEPROM) +static int do_eth_eeprom_rw(struct eth_device *dev, + int argc, char * const argv[]) +{ + ulong addr, offset, length = 1; + + if (argc < 3) + return CMD_RET_USAGE; + + addr = simple_strtoul(argv[1], NULL, 16); + offset = simple_strtoul(argv[2], NULL, 16); + if (argc > 3) + length = simple_strtoul(argv[3], NULL, 16); + + if (!strcmp(argv[0], "write")) { + if (eth_eeprom_write(dev, offset, length, (void *)addr)) { + printf("EEPROM write failed\n"); + return CMD_RET_FAILURE; + } + return CMD_RET_SUCCESS; + } else if (!strcmp(argv[0], "read")) { + if (eth_eeprom_read(dev, offset, length, (void *)addr)) { + printf("EEPROM read failed\n"); + return CMD_RET_FAILURE; + } + return CMD_RET_SUCCESS; + } + + return CMD_RET_USAGE; +} + +static int do_eth_eeprom_defaults(struct eth_device *dev, + int argc, char * const argv[]) +{ + if (eth_eeprom_defaults(dev)) { + printf("EEPROM write failed\n"); + return CMD_RET_FAILURE; + } + + return CMD_RET_SUCCESS; +} + +static int do_eth_eeprom_set_mac(struct eth_device *dev, + int argc, char * const argv[]) +{ + u8 mac[6]; + + if (argc < 2) + return CMD_RET_USAGE; + + eth_parse_enetaddr(argv[1], mac); + if (!is_valid_ether_addr(mac)) { + printf("Invalid mac address given\n"); + return CMD_RET_FAILURE; + } + + printf("Writing MAC to EEPROM ....\n"); + if (eth_eeprom_write_mac(dev, mac)) { + printf("EEPROM write failed\n"); + return CMD_RET_FAILURE; + } + + return CMD_RET_SUCCESS; +} + +static int do_eth_eeprom_show_mac(struct eth_device *dev, + int argc, char * const argv[]) +{ + u8 data[6]; + + if (eth_eeprom_read_mac(dev, data)) { + printf("EEPROM read failed\n"); + return CMD_RET_FAILURE; + } + + printf("%pM\n", data); + if (!is_valid_ether_addr(data)) + printf("Warning: MAC address is not valid!\n"); + + return CMD_RET_SUCCESS; +} + +static int do_eth_eeprom(struct eth_device *dev, + int argc, char * const argv[]) +{ + if (argc < 2) + return CMD_RET_USAGE; + + if (!strcmp(argv[1], "read") || !strcmp(argv[1], "write")) + return do_eth_eeprom_rw(dev, argc - 1, argv + 1); + if (!strcmp(argv[1], "defaults")) + return do_eth_eeprom_defaults(dev, argc - 1, argv + 1); + if (!strcmp(argv[1], "set_mac")) + return do_eth_eeprom_set_mac(dev, argc - 1, argv + 1); + if (!strcmp(argv[1], "show_mac")) + return do_eth_eeprom_show_mac(dev, argc - 1, argv + 1); + + printf("Unknown eeprom sub command: %s\n", argv[1]); + + return CMD_RET_USAGE; +} +#endif + static int do_eth_show(struct eth_device *dev, int argc, char * const argv[]) { @@ -478,6 +581,10 @@ static int do_eth(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
if (!strcmp(argv[2], "show")) return do_eth_show(dev, argc - 2, argv + 2); +#if defined(CONFIG_CMD_ETH_EEPROM) + if (!strcmp(argv[2], "eeprom")) + return do_eth_eeprom(dev, argc - 2, argv + 2); +#endif
printf("Unknown eth sub command: %s\n", argv[2]);
@@ -492,4 +599,18 @@ U_BOOT_CMD( "\n" "eth <dev> show\n" " - show basic information about the ethernet device\n" +#if defined(CONFIG_CMD_ETH_EEPROM) + "eth <dev> eeprom <command> [<args>...]\n" + " - access the EEPROM of the ethernet device:\n" + " read <addr> <off> [<size>]\n" + " - read <size> bytes starting at offset <off> to memory address <addr>.\n" + " write <addr> <off> [<size>]\n" + " - write <size> bytes starting at offset <off> from memory address <addr>.\n" + " defaults\n" + " - write default settings in the EEPROM.\n" + " set_mac <mac>\n" + " - set the MAC address in the EEPROM to 'mac'\n" + " show_mac\n" + " - read the MAC address from the EEPROM." +#endif ); diff --git a/include/net.h b/include/net.h index 18d279e..fba7572 100644 --- a/include/net.h +++ b/include/net.h @@ -92,6 +92,25 @@ struct eth_device { int (*mcast) (struct eth_device *, const u8 *enetaddr, u8 set); #endif int (*write_hwaddr) (struct eth_device *); +#ifdef CONFIG_CMD_ETH_EEPROM + /* Read data from the ethernet device eeprom */ + int (*eeprom_read)(struct eth_device *, + u32 offset, u32 length, u8 *data); + /* Write data to the ethernet device eeprom */ + int (*eeprom_write)(struct eth_device *, + u32 offset, u32 length, u8 *data); + /* Write the default settings to the eeprom */ + int (*eeprom_defaults)(struct eth_device *); + /* Read the MAC stored in the eeprom, if not implemented + * the MAC is assumed to be at the given offset. */ + int (*eeprom_read_mac)(struct eth_device *, u8 *enetaddr); + /* Write the MAC in the eeprom, if not implemented + * the MAC is assumed to be at the given offset. */ + int (*eeprom_write_mac)(struct eth_device *, u8 *enetaddr); + /* Offset of the MAC address for the default implementation. + * Set to a negative value if the MAC is not in the EEPROM. */ + int eeprom_mac_offset; +#endif struct eth_device *next; int index; void *priv; @@ -172,6 +191,15 @@ int eth_mcast_join(IPaddr_t mcast_addr, u8 join); u32 ether_crc(size_t len, unsigned char const *p); #endif
+#ifdef CONFIG_CMD_ETH_EEPROM +int eth_eeprom_read(struct eth_device *dev, u32 offset, + u32 length, u8 *data); +int eth_eeprom_write(struct eth_device *dev, u32 offset, + u32 length, u8 *data); +int eth_eeprom_defaults(struct eth_device *dev); +int eth_eeprom_read_mac(struct eth_device *, u8 *enetaddr); +int eth_eeprom_write_mac(struct eth_device *, u8 *enetaddr); +#endif
/**********************************************************************/ /* diff --git a/net/eth.c b/net/eth.c index 76ffa05..2cde72c 100644 --- a/net/eth.c +++ b/net/eth.c @@ -542,3 +542,49 @@ char *eth_get_name(void) { return eth_current ? eth_current->name : "unknown"; } + +#ifdef CONFIG_CMD_ETH_EEPROM +int eth_eeprom_read(struct eth_device *dev, u32 offset, + u32 length, u8 *data) +{ + return dev->eeprom_read ? + dev->eeprom_read(dev, offset, length, data) : + -ENOSYS; +} + +int eth_eeprom_write(struct eth_device *dev, u32 offset, + u32 length, u8 *data) +{ + return dev->eeprom_write ? + dev->eeprom_write(dev, offset, length, data) : + -ENOSYS; +} + +int eth_eeprom_defaults(struct eth_device *dev) +{ + return dev->eeprom_defaults ? dev->eeprom_defaults(dev) : + -ENOSYS; +} + +int eth_eeprom_read_mac(struct eth_device *dev, u8 *enetaddr) +{ + if (dev->eeprom_read_mac) + return dev->eeprom_read_mac(dev, enetaddr); + + return dev->eeprom_mac_offset >= 0 ? + eth_eeprom_read(dev, dev->eeprom_mac_offset, + 6, enetaddr) : + -ENOSYS; +} + +int eth_eeprom_write_mac(struct eth_device *dev, u8 *enetaddr) +{ + if (dev->eeprom_write_mac) + return dev->eeprom_write_mac(dev, enetaddr); + + return dev->eeprom_mac_offset >= 0 ? + eth_eeprom_write(dev, dev->eeprom_mac_offset, + 6, enetaddr) : + -ENOSYS; +} +#endif
Hi,
On 14 October 2014 18:26, Alban Bedel alban.bedel@avionic-design.de wrote:
Many ethernet devices use an EEPROM to store various settings, most commonly the device MAC address. But on some devices it can contains a lot more, for example USB device might also have many USB related parameters.
This commit add a set of commands to read/write this EEPROM, write a default configuration and read/write the device MAC address. The defaults command allow priming the EEPROM for devices that need more than just a MAC address in the EEPROM.
Signed-off-by: Alban Bedel alban.bedel@avionic-design.de
v2: * No changes since v1 v3: * Replace the dedicated 'eth_eeprom' command with a subcommand to the newly introduced 'eth' command
I see a few EEPROM implementations in the code base. It feels to me like we need an EEPROM interface. In driver model terms this could be a uclass. I started something here:
http://patchwork.ozlabs.org/patch/399039/
Of course we don't have DM for Ethernet yet - when we do I think a child EEPROM device below the Ethernet would make sense. It could be created by the Ethernet driver when it knows that this information exists. But even without that I feel that the EEPROM should be logically separated from Ethernet.
So I suggest that instead of an #ifdef to adjust the Ethernet API, add a pointer to another struct containing the EEPROM API and put it in its own header. I also feel that there should ultimately be an eeprom command which operates on these. Then the only Ethernet API call would be to find the EEPROM pointer, if there is one.
If someone feels like taking it on, driver model support for Ethernet should be pretty easy. Or even EEPROM could be done now and this might avoid churn. But I would be happy for this series to be applied as is while working on a driver model version. I just don't feel we should be adding new subsystems that don't use driver model.
Regards, Simon
On Tue, Oct 14, 2014 at 12:21 PM, Simon Glass sjg@chromium.org wrote:
Hi,
On 14 October 2014 18:26, Alban Bedel alban.bedel@avionic-design.de
wrote:
Many ethernet devices use an EEPROM to store various settings, most commonly the device MAC address. But on some devices it can contains a lot more, for example USB device might also have many USB related parameters.
This commit add a set of commands to read/write this EEPROM, write a default configuration and read/write the device MAC address. The defaults command allow priming the EEPROM for devices that need more than just a MAC address in the EEPROM.
Signed-off-by: Alban Bedel alban.bedel@avionic-design.de
v2: * No changes since v1 v3: * Replace the dedicated 'eth_eeprom' command with a subcommand to the newly introduced 'eth' command
I see a few EEPROM implementations in the code base. It feels to me like we need an EEPROM interface. In driver model terms this could be a uclass. I started something here:
http://patchwork.ozlabs.org/patch/399039/
Of course we don't have DM for Ethernet yet - when we do I think a child EEPROM device below the Ethernet would make sense. It could be created by the Ethernet driver when it knows that this information exists. But even without that I feel that the EEPROM should be logically separated from Ethernet.
So I suggest that instead of an #ifdef to adjust the Ethernet API, add a pointer to another struct containing the EEPROM API and put it in its own header. I also feel that there should ultimately be an eeprom command which operates on these. Then the only Ethernet API call would be to find the EEPROM pointer, if there is one.
If someone feels like taking it on, driver model support for Ethernet should be pretty easy. Or even EEPROM could be done now and this might avoid churn. But I would be happy for this series to be applied as is while working on a driver model version. I just don't feel we should be adding new subsystems that don't use driver model.
I agree that we shouldn't add new subsystems that don't use DM. I think this (adding eeprom access to Ethernet) is a decent compromise until we have driver model for Ethernet. I do like your idea of having an eeprom class / subsystem / command that can operate on this type of thing the same way no matter where it's connected, but it probably isn't a good idea to add an "eeprom" command for that without using DM first.
So your idea sounds good, but it leaves me with one question... if we want to accept this now as it is, then do we want to introduce a new command (eth eeprom) when we already know we want to change its behavior or delete it once we add a uclass for eeproms?
BTW, speaking of DM for Ethernet, I'll take on moving it over around the end of this month if no one beats me to it.
Cheers, -Joe
Hi Joe,
On 14 October 2014 21:14, Joe Hershberger joe.hershberger@gmail.com wrote:
On Tue, Oct 14, 2014 at 12:21 PM, Simon Glass sjg@chromium.org wrote:
Hi,
On 14 October 2014 18:26, Alban Bedel alban.bedel@avionic-design.de wrote:
Many ethernet devices use an EEPROM to store various settings, most commonly the device MAC address. But on some devices it can contains a lot more, for example USB device might also have many USB related parameters.
This commit add a set of commands to read/write this EEPROM, write a default configuration and read/write the device MAC address. The defaults command allow priming the EEPROM for devices that need more than just a MAC address in the EEPROM.
Signed-off-by: Alban Bedel alban.bedel@avionic-design.de
v2: * No changes since v1 v3: * Replace the dedicated 'eth_eeprom' command with a subcommand to the newly introduced 'eth' command
I see a few EEPROM implementations in the code base. It feels to me like we need an EEPROM interface. In driver model terms this could be a uclass. I started something here:
http://patchwork.ozlabs.org/patch/399039/
Of course we don't have DM for Ethernet yet - when we do I think a child EEPROM device below the Ethernet would make sense. It could be created by the Ethernet driver when it knows that this information exists. But even without that I feel that the EEPROM should be logically separated from Ethernet.
So I suggest that instead of an #ifdef to adjust the Ethernet API, add a pointer to another struct containing the EEPROM API and put it in its own header. I also feel that there should ultimately be an eeprom command which operates on these. Then the only Ethernet API call would be to find the EEPROM pointer, if there is one.
If someone feels like taking it on, driver model support for Ethernet should be pretty easy. Or even EEPROM could be done now and this might avoid churn. But I would be happy for this series to be applied as is while working on a driver model version. I just don't feel we should be adding new subsystems that don't use driver model.
I agree that we shouldn't add new subsystems that don't use DM. I think this (adding eeprom access to Ethernet) is a decent compromise until we have driver model for Ethernet. I do like your idea of having an eeprom class / subsystem / command that can operate on this type of thing the same way no matter where it's connected, but it probably isn't a good idea to add an "eeprom" command for that without using DM first.
So your idea sounds good, but it leaves me with one question... if we want to accept this now as it is, then do we want to introduce a new command (eth eeprom) when we already know we want to change its behavior or delete it once we add a uclass for eeproms?
Good question, in general I suppose we want to avoid changing the commands, although if you get to Ethernet in this merge window then it might not matter much. One option is to indicate in the help that it is a temporary command and the syntax may change.
BTW, speaking of DM for Ethernet, I'll take on moving it over around the end of this month if no one beats me to it.
SGTM, looking forward to it.
Regards, Simon
On Tue, 14 Oct 2014 21:18:37 +0200 Simon Glass sjg@chromium.org wrote:
Hi Joe,
On 14 October 2014 21:14, Joe Hershberger joe.hershberger@gmail.com wrote:
On Tue, Oct 14, 2014 at 12:21 PM, Simon Glass sjg@chromium.org wrote:
Hi,
On 14 October 2014 18:26, Alban Bedel alban.bedel@avionic-design.de wrote:
Many ethernet devices use an EEPROM to store various settings, most commonly the device MAC address. But on some devices it can contains a lot more, for example USB device might also have many USB related parameters.
This commit add a set of commands to read/write this EEPROM, write a default configuration and read/write the device MAC address. The defaults command allow priming the EEPROM for devices that need more than just a MAC address in the EEPROM.
Signed-off-by: Alban Bedel alban.bedel@avionic-design.de
v2: * No changes since v1 v3: * Replace the dedicated 'eth_eeprom' command with a subcommand to the newly introduced 'eth' command
I see a few EEPROM implementations in the code base. It feels to me like we need an EEPROM interface. In driver model terms this could be a uclass. I started something here:
http://patchwork.ozlabs.org/patch/399039/
Of course we don't have DM for Ethernet yet - when we do I think a child EEPROM device below the Ethernet would make sense. It could be created by the Ethernet driver when it knows that this information exists. But even without that I feel that the EEPROM should be logically separated from Ethernet.
So I suggest that instead of an #ifdef to adjust the Ethernet API, add a pointer to another struct containing the EEPROM API and put it in its own header. I also feel that there should ultimately be an eeprom command which operates on these. Then the only Ethernet API call would be to find the EEPROM pointer, if there is one.
If someone feels like taking it on, driver model support for Ethernet should be pretty easy. Or even EEPROM could be done now and this might avoid churn. But I would be happy for this series to be applied as is while working on a driver model version. I just don't feel we should be adding new subsystems that don't use driver model.
I agree that we shouldn't add new subsystems that don't use DM. I think this (adding eeprom access to Ethernet) is a decent compromise until we have driver model for Ethernet. I do like your idea of having an eeprom class / subsystem / command that can operate on this type of thing the same way no matter where it's connected, but it probably isn't a good idea to add an "eeprom" command for that without using DM first.
So your idea sounds good, but it leaves me with one question... if we want to accept this now as it is, then do we want to introduce a new command (eth eeprom) when we already know we want to change its behavior or delete it once we add a uclass for eeproms?
Good question, in general I suppose we want to avoid changing the commands, although if you get to Ethernet in this merge window then it might not matter much. One option is to indicate in the help that it is a temporary command and the syntax may change.
A generic EEPROM driver / command sounds good, however we would still need a few ethernet driver specific commands like the ones to manipulate the MAC address.
If possible I would really like to settle the command syntax, so I would suggest using the following scheme for the future eeprom command:
eeprom <dev> <command> [<args>...]
If this command is also available internally with an interface such as:
int eeprom_cmd(struct eeprom_device *dev, int argc, char * const argv[]);
the 'eth eeprom' implementation could easily provides its own commands on the eeprom while falling back on eeprom_cmd() for the generic ones. That would avoid any code duplication while keeping the 'eth eeprom' functionality.
I'm unsure about the 'defaults' commands, I would tend to see it at the eeprom level as it might be needed in various situations. However it must be provided by the device, not the eeprom driver itself. It might also make sense to drop the callback in favour of a simple data array as it would allow setting it from device tree.
Alban
Hi Alban,
On 15 October 2014 03:42, Alban Bedel alban.bedel@avionic-design.de wrote:
On Tue, 14 Oct 2014 21:18:37 +0200 Simon Glass sjg@chromium.org wrote:
Hi Joe,
On 14 October 2014 21:14, Joe Hershberger joe.hershberger@gmail.com wrote:
On Tue, Oct 14, 2014 at 12:21 PM, Simon Glass sjg@chromium.org wrote:
Hi,
On 14 October 2014 18:26, Alban Bedel alban.bedel@avionic-design.de wrote:
Many ethernet devices use an EEPROM to store various settings, most commonly the device MAC address. But on some devices it can contains a lot more, for example USB device might also have many USB related parameters.
This commit add a set of commands to read/write this EEPROM, write a default configuration and read/write the device MAC address. The defaults command allow priming the EEPROM for devices that need more than just a MAC address in the EEPROM.
Signed-off-by: Alban Bedel alban.bedel@avionic-design.de
v2: * No changes since v1 v3: * Replace the dedicated 'eth_eeprom' command with a subcommand to the newly introduced 'eth' command
I see a few EEPROM implementations in the code base. It feels to me like we need an EEPROM interface. In driver model terms this could be a uclass. I started something here:
http://patchwork.ozlabs.org/patch/399039/
Of course we don't have DM for Ethernet yet - when we do I think a child EEPROM device below the Ethernet would make sense. It could be created by the Ethernet driver when it knows that this information exists. But even without that I feel that the EEPROM should be logically separated from Ethernet.
So I suggest that instead of an #ifdef to adjust the Ethernet API, add a pointer to another struct containing the EEPROM API and put it in its own header. I also feel that there should ultimately be an eeprom command which operates on these. Then the only Ethernet API call would be to find the EEPROM pointer, if there is one.
If someone feels like taking it on, driver model support for Ethernet should be pretty easy. Or even EEPROM could be done now and this might avoid churn. But I would be happy for this series to be applied as is while working on a driver model version. I just don't feel we should be adding new subsystems that don't use driver model.
I agree that we shouldn't add new subsystems that don't use DM. I think this (adding eeprom access to Ethernet) is a decent compromise until we have driver model for Ethernet. I do like your idea of having an eeprom class / subsystem / command that can operate on this type of thing the same way no matter where it's connected, but it probably isn't a good idea to add an "eeprom" command for that without using DM first.
So your idea sounds good, but it leaves me with one question... if we want to accept this now as it is, then do we want to introduce a new command (eth eeprom) when we already know we want to change its behavior or delete it once we add a uclass for eeproms?
Good question, in general I suppose we want to avoid changing the commands, although if you get to Ethernet in this merge window then it might not matter much. One option is to indicate in the help that it is a temporary command and the syntax may change.
A generic EEPROM driver / command sounds good, however we would still need a few ethernet driver specific commands like the ones to manipulate the MAC address.
If possible I would really like to settle the command syntax, so I would suggest using the following scheme for the future eeprom command:
eeprom <dev> <command> [<args>...]
If this command is also available internally with an interface such as:
int eeprom_cmd(struct eeprom_device *dev, int argc, char * const argv[]);
the 'eth eeprom' implementation could easily provides its own commands on the eeprom while falling back on eeprom_cmd() for the generic ones. That would avoid any code duplication while keeping the 'eth eeprom' functionality.
There is already an eeprom command actually - I think Marex was looking at tidying it up. But what you propose sounds good.
With driver model, the Ethernet would just provide another eeprom device when enabled. Yes agreed you can have 'eth eeprom' for anything specific to Ethernet.
I'm unsure about the 'defaults' commands, I would tend to see it at the eeprom level as it might be needed in various situations. However it must be provided by the device, not the eeprom driver itself. It might also make sense to drop the callback in favour of a simple data array as it would allow setting it from device tree.
I'm not sure what this refers to sorry.
Alban
Regards, SImon
On Fri, 17 Oct 2014 14:12:18 -0600 Simon Glass sjg@chromium.org wrote:
I'm unsure about the 'defaults' commands, I would tend to see it at the eeprom level as it might be needed in various situations. However it must be provided by the device, not the eeprom driver itself. It might also make sense to drop the callback in favour of a simple data array as it would allow setting it from device tree.
I'm not sure what this refers to sorry.
It refer to the 'eth eeprom defaults' command from this patch. The USB device I wrote that for need a lot more in the EEPROM than just the MAC address. This command allow writing the "factory defaults" in the EEPROM. Somehow I have a feeling that such a command would make sense for a few other use-case than just USB ethernet devices.
Alban
Hi Alban,
On 21 October 2014 06:51, Alban Bedel alban.bedel@avionic-design.de wrote:
On Fri, 17 Oct 2014 14:12:18 -0600 Simon Glass sjg@chromium.org wrote:
I'm unsure about the 'defaults' commands, I would tend to see it at the eeprom level as it might be needed in various situations. However it must be provided by the device, not the eeprom driver itself. It might also make sense to drop the callback in favour of a simple data array as it would allow setting it from device tree.
I'm not sure what this refers to sorry.
It refer to the 'eth eeprom defaults' command from this patch. The USB device I wrote that for need a lot more in the EEPROM than just the MAC address. This command allow writing the "factory defaults" in the EEPROM. Somehow I have a feeling that such a command would make sense for a few other use-case than just USB ethernet devices.
Quite possibly, although one option is to cross that bridge when you come to it.
Regards, Simon
On Tuesday, October 14, 2014 at 07:21:06 PM, Simon Glass wrote:
Hi,
On 14 October 2014 18:26, Alban Bedel alban.bedel@avionic-design.de wrote:
Many ethernet devices use an EEPROM to store various settings, most commonly the device MAC address. But on some devices it can contains a lot more, for example USB device might also have many USB related parameters.
This commit add a set of commands to read/write this EEPROM, write a default configuration and read/write the device MAC address. The defaults command allow priming the EEPROM for devices that need more than just a MAC address in the EEPROM.
Signed-off-by: Alban Bedel alban.bedel@avionic-design.de
v2: * No changes since v1 v3: * Replace the dedicated 'eth_eeprom' command with a subcommand
to the newly introduced 'eth' commandI see a few EEPROM implementations in the code base. It feels to me like we need an EEPROM interface. In driver model terms this could be a uclass. I started something here:
http://patchwork.ozlabs.org/patch/399039/
Of course we don't have DM for Ethernet yet - when we do I think a child EEPROM device below the Ethernet would make sense. It could be created by the Ethernet driver when it knows that this information exists. But even without that I feel that the EEPROM should be logically separated from Ethernet.
So I suggest that instead of an #ifdef to adjust the Ethernet API, add a pointer to another struct containing the EEPROM API and put it in its own header. I also feel that there should ultimately be an eeprom command which operates on these. Then the only Ethernet API call would be to find the EEPROM pointer, if there is one.
If someone feels like taking it on, driver model support for Ethernet should be pretty easy. Or even EEPROM could be done now and this might avoid churn. But I would be happy for this series to be applied as is while working on a driver model version. I just don't feel we should be adding new subsystems that don't use driver model.
Speaking of eeprom command, I am now in the process of cleaning up common/cmd_eeprom.c . Simon, do you have anything related to DM which explicitly touches this file ? If not, I'd suggest you wait a bit until I manage to resolve the ifdef maze in there and look into it only after the cleanup is done.
Best regards, Marek Vasut
Use the new ethernet eeprom API to allow the user to read/write the EEPROM.
Signed-off-by: Alban Bedel alban.bedel@avionic-design.de --- v2: * Rework the defaults implementation to use the proper config depending on the device type. * Allow the board to override the defaults data. * Use the proper defaults instead of the Tamonten config. * Fix style error in usb_ether.h * Add a comment to explain why the default MAC has all bits set
v3: * No changes since v2
This patch is based on earlier work from Thierry Reding, I assumed that the default config was derived from the datasheet. However it turned out that this config had been customized for the Tamonten boards. I restored the config according to the defaults found in the datasheet. Sadly the datasheet doesn't properly document all the fields from the EEPROM, so it may still have a few bugs as I don't have the default values for all fields. --- drivers/usb/eth/smsc95xx.c | 246 +++++++++++++++++++++++++++++++++++++++++++-- include/usb_ether.h | 8 ++ 2 files changed, 247 insertions(+), 7 deletions(-)
diff --git a/drivers/usb/eth/smsc95xx.c b/drivers/usb/eth/smsc95xx.c index 6bca34d..4e06be7 100644 --- a/drivers/usb/eth/smsc95xx.c +++ b/drivers/usb/eth/smsc95xx.c @@ -59,6 +59,8 @@
#define E2P_CMD 0x30 #define E2P_CMD_BUSY_ 0x80000000 +#define E2P_CMD_EWEN_ 0x20000000 +#define E2P_CMD_WRITE_ 0x30000000 #define E2P_CMD_READ_ 0x00000000 #define E2P_CMD_TIMEOUT_ 0x00000400 #define E2P_CMD_LOADED_ 0x00000200 @@ -146,6 +148,155 @@ struct smsc95xx_private { int have_hwaddr; /* 1 if we have a hardware MAC address */ };
+#ifdef CONFIG_CMD_ETH_EEPROM +struct smsc95xx_eeprom_device { + unsigned short vendor; + unsigned short product; + struct smsc95xx_eeprom_defaults *defaults; +}; + +/* Default values as used by the controller when the EEPROM hasn't + * been programmed yet. Note that when unset the MAC address has + * all bits set instead of all bits cleared as is usual in u-boot. */ +static u8 smsc9514_eeprom_defaults_data[] = { + /* 0x00 */ + 0xA5, /* Signature */ + 0xFF, 0xFF, /* MAC bytes 0-1 */ + 0xFF, 0xFF, /* MAC bytes 2-3 */ + 0xFF, 0xFF, /* MAC bytes 4-5 */ + 0x01, /* FS Polling Interval for Interrupt Endpoint */ + 0x04, /* HS Polling Interval for Interrupt Endpoint */ + 0x05, /* Configuration Flags */ + 0x09, 0x04, /* Language ID */ + 0x0a, /* Manufacturer ID String Descriptor Length (bytes) */ + 0x2f, /* Manufacturer ID String Descriptor EEPROM Word Offset */ + 0x10, /* Product Name String Descriptor Length (bytes) */ + 0x34, /* Product Name String Descriptor EEPROM Word Offset */ + /* 0x10 */ + 0x12, /* Serial Number String Descriptor Length (bytes) */ + 0x3c, /* Serial Number String Descriptor EEPROM Word Offset */ + 0x08, /* Configuration String Descriptor Length (bytes) */ + 0x45, /* Configuration String Descriptor Word Offset */ + 0x08, /* Interface String Descriptor Length (bytes) */ + 0x49, /* Interface String Descriptor Word Offset */ + 0x12, /* Hi-Speed Device Descriptor Length (bytes) */ + 0x1d, /* Hi-Speed Device Descriptor Word Offset */ + 0x12, /* Hi-Speed Configuration and Interface Descriptor Length (bytes) */ + 0x26, /* Hi-Speed Configuration and Interface Descriptor Word Offset */ + 0x12, /* Full-Speed Device Descriptor Length (bytes) */ + 0x1d, /* Full-Speed Device Descriptor Word Offset */ + 0x12, /* Full-Speed Configuration and Interface Descriptor Length (bytes) */ + 0x26, /* Full-Speed Configuration and Interface Descriptor Word Offset */ + 0x00, 0x00, /* RESERVED */ + /* 0x20 */ + 0x24, 0x04, /* Vendor ID */ + 0x14, 0x95, /* Product ID */ + 0x00, 0x01, /* Device ID */ + 0x9b, /* Config Data Byte 1 Register (CFG1) */ + 0x18, /* Config Data Byte 2 Register (CFG2) */ + 0x00, /* Config Data Byte 3 Register (CFG3) */ + 0x02, /* Non-Removable Devices Register (NRD) */ + 0x00, /* Port Disable (Self) Register (PDS) */ + 0x00, /* Port Disable (Bus) Register (PDB) */ + 0x01, /* Max Power (Self) Register (MAXPS) */ + 0x00, /* Max Power (Bus) Register (MAXPB) */ + 0x01, /* Hub Controller Max Current (Self) Register (HCMCS) */ + 0x00, /* Hub Controller Max Current (Bus) Register (HCMCB) */ + /* 0x30 */ + 0x32, /* Power-on Time Register (PWRT) */ + 0x00, /* Boost_Up Register (BOOSTUP) */ + 0x00, /* Boost_5 Register (BOOST5) */ + 0x00, /* Boost_4:2 Register (BOOST42) */ + 0x00, /* RESERVED */ + 0x00, /* Port Swap Register (PRTSP) */ + 0x21, /* Port Remap 12 Register (PRTR12) */ + 0x43, /* Port Remap 34 Register (PRTR34) */ + 0x05, /* Port Remap 5 Register (PRTR5) */ + 0x01, /* Status/Command Register (STCD) */ + /* 0x3A - Device Descriptor */ + 0x12, 0x01, + 0x00, 0x02, + 0xff, 0x00, + /* 0x40 */ + 0xff, 0x40, + 0x24, 0x04, + 0x00, 0xec, + 0x00, 0x01, + 0x01, 0x02, + 0x03, 0x01, + /* 0x4C - Configuration and Interface Descriptor */ + 0x09, 0x02, + 0x27, 0x00, + /* 0x50 */ + 0x01, 0x01, + 0x04, 0xc0, + 0x00, 0x09, + 0x04, 0x00, + 0x00, 0x03, + 0xff, 0x00, + 0xff, 0x05, + /* 0x5E - Manufacturer ID String Descriptor */ + 0x0a, 0x03, + /* 0x60 */ + 0x53, 0x00, /* S */ + 0x4d, 0x00, /* M */ + 0x53, 0x00, /* S */ + 0x43, 0x00, /* C */ + /* 0x68 - Product Name String */ + 0x10, 0x03, + 0x4c, 0x00, /* L */ + 0x41, 0x00, /* A */ + 0x4e, 0x00, /* N */ + /* 0x70 */ + 0x39, 0x00, /* 9 */ + 0x35, 0x00, /* 5 */ + 0x31, 0x00, /* 1 */ + 0x34, 0x00, /* 5 */ + /* 0x78 - Serial Number String Descriptor */ + 0x12, 0x03, + 0x31, 0x00, /* 1 */ + 0x32, 0x00, /* 2 */ + 0x33, 0x00, /* 3 */ + /* 0x80 */ + 0x34, 0x00, /* 4 */ + 0x35, 0x00, /* 5 */ + 0x36, 0x00, /* 6 */ + 0x37, 0x00, /* 7 */ + 0x38, 0x00, /* 8 */ + /* 0x8A - Configuration String Descriptor */ + 0x08, 0x03, + 0x43, 0x00, /* C */ + 0x66, 0x00, /* f */ + /* 0x90 */ + 0x67, 0x00, /* g */ + /* 0x92 - Interface String Descriptor */ + 0x08, 0x03, + 0x69, 0x00, /* i */ + 0x2f, 0x00, /* / */ + 0x66, 0x00, /* f */ + /* 0x9A - END */ + 0x00, 0x00, + 0x00, 0x00, + 0x00, 0x00, + /* 0xA0 */ +}; + +/* This can be overriden by the board to use custom data */ +struct smsc95xx_eeprom_defaults __weak smsc9514_eeprom_defaults = { + .data = smsc9514_eeprom_defaults_data, + .size = ARRAY_SIZE(smsc9514_eeprom_defaults_data), +}; + +static struct smsc95xx_eeprom_device smsc95xx_eeprom_devices[] = { + { + .vendor = 0x0424, + .product = 0x9514, + .defaults = &smsc9514_eeprom_defaults, + }, + {} +}; +#endif + /* * Smsc95xx infrastructure commands */ @@ -285,9 +436,10 @@ static int smsc95xx_wait_eeprom(struct ueth_data *dev) return 0; }
-static int smsc95xx_read_eeprom(struct ueth_data *dev, u32 offset, u32 length, - u8 *data) +static int smsc95xx_read_eeprom(struct eth_device *eth, u32 offset, u32 length, + u8 *data) { + struct ueth_data *dev = (struct ueth_data *)eth->priv; u32 val; int i, ret;
@@ -310,6 +462,81 @@ static int smsc95xx_read_eeprom(struct ueth_data *dev, u32 offset, u32 length, return 0; }
+#ifdef CONFIG_CMD_ETH_EEPROM +static int smsc95xx_write_eeprom(struct eth_device *eth, u32 offset, u32 length, + u8 *data) +{ + struct ueth_data *dev = (struct ueth_data *)eth->priv; + u32 val; + int i, ret; + + ret = smsc95xx_eeprom_confirm_not_busy(dev); + if (ret) + return ret; + + /* Issue write/erase enable command */ + val = E2P_CMD_BUSY_ | E2P_CMD_EWEN_; + ret = smsc95xx_write_reg(dev, E2P_CMD, val); + if (ret < 0) + return ret; + + ret = smsc95xx_wait_eeprom(dev); + if (ret < 0) + return ret; + + for (i = 0; i < length; i++) { + /* Fill data register */ + val = data[i]; + ret = smsc95xx_write_reg(dev, E2P_DATA, val); + if (ret < 0) + return ret; + + /* Send "write" command */ + val = E2P_CMD_BUSY_ | E2P_CMD_WRITE_ | + (offset & E2P_CMD_ADDR_); + ret = smsc95xx_write_reg(dev, E2P_CMD, val); + if (ret < 0) + return ret; + + ret = smsc95xx_wait_eeprom(dev); + if (ret < 0) + return ret; + + offset++; + } + return 0; +} + +static int smsc95xx_defaults_eeprom(struct eth_device *eth) +{ + struct ueth_data *ueth = (struct ueth_data *)eth->priv; + struct smsc95xx_eeprom_defaults *dflt; + struct usb_device *usb_dev; + int i; + + /* Try to find the device type, we must look at the parent to handle + * devices like the LAN9512 and LAN9514 that include a usb hub. */ + for (usb_dev = ueth->pusb_dev, dflt = NULL; + usb_dev && !dflt; + usb_dev = usb_dev->parent) { + for (i = 0; smsc95xx_eeprom_devices[i].defaults; i++) { + if (smsc95xx_eeprom_devices[i].vendor == + usb_dev->descriptor.idVendor && + smsc95xx_eeprom_devices[i].product == + usb_dev->descriptor.idProduct) { + dflt = smsc95xx_eeprom_devices[i].defaults; + break; + } + } + } + + if (dflt && dflt->data && dflt->size > 0) + return smsc95xx_write_eeprom(eth, 0, dflt->size, dflt->data); + + return -1; +} +#endif + /* * mii_nway_restart - restart NWay (autonegotiation) for this interface * @@ -349,12 +576,11 @@ static int smsc95xx_phy_initialize(struct ueth_data *dev) return 0; }
-static int smsc95xx_init_mac_address(struct eth_device *eth, - struct ueth_data *dev) +static int smsc95xx_init_mac_address(struct eth_device *eth) { /* try reading mac address from EEPROM */ - if (smsc95xx_read_eeprom(dev, EEPROM_MAC_OFFSET, ETH_ALEN, - eth->enetaddr) == 0) { + if (smsc95xx_read_eeprom(eth, EEPROM_MAC_OFFSET, ETH_ALEN, + eth->enetaddr) == 0) { if (is_valid_ether_addr(eth->enetaddr)) { /* eeprom values are valid so use them */ debug("MAC address read from EEPROM\n"); @@ -507,7 +733,7 @@ static int smsc95xx_init(struct eth_device *eth, bd_t *bd) debug("timeout waiting for PHY Reset\n"); return -1; } - if (!priv->have_hwaddr && smsc95xx_init_mac_address(eth, dev) == 0) + if (!priv->have_hwaddr && smsc95xx_init_mac_address(eth) == 0) priv->have_hwaddr = 1; if (!priv->have_hwaddr) { puts("Error: SMSC95xx: No MAC address set - set usbethaddr\n"); @@ -894,6 +1120,12 @@ int smsc95xx_eth_get_info(struct usb_device *dev, struct ueth_data *ss, eth->recv = smsc95xx_recv; eth->halt = smsc95xx_halt; eth->write_hwaddr = smsc95xx_write_hwaddr; +#ifdef CONFIG_CMD_ETH_EEPROM + eth->eeprom_read = smsc95xx_read_eeprom; + eth->eeprom_write = smsc95xx_write_eeprom; + eth->eeprom_defaults = smsc95xx_defaults_eeprom; + eth->eeprom_mac_offset = EEPROM_MAC_OFFSET; +#endif eth->priv = ss; return 1; } diff --git a/include/usb_ether.h b/include/usb_ether.h index 35700a2..e7d371f 100644 --- a/include/usb_ether.h +++ b/include/usb_ether.h @@ -61,4 +61,12 @@ int smsc95xx_eth_probe(struct usb_device *dev, unsigned int ifnum, int smsc95xx_eth_get_info(struct usb_device *dev, struct ueth_data *ss, struct eth_device *eth);
+/* Default EEPROM data, to allow the boards to override it */ +struct smsc95xx_eeprom_defaults { + u8 *data; + u32 size; +}; + +extern struct smsc95xx_eeprom_defaults smsc9514_eeprom_defaults; + #endif /* __USB_ETHER_H__ */
participants (4)
-
Alban Bedel -
Joe Hershberger -
Marek Vasut -
Simon Glass