[U-Boot] [PATCH v3 1/3] net: Add a command to manipulate ethernet devices

[U-Boot] [PATCH v3 1/3] net: Add a command to manipulate ethernet devices

[Alban Bedel]

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"
+);
-- 
2.1.1

[U-Boot] [PATCH v3 2/3] net: Add a command to access the EEPROM from ethernet devices

[Alban Bedel]

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
-- 
2.1.1

[U-Boot] [PATCH v3 2/3] net: Add a command to access the EEPROM from ethernet devices

[Simon Glass]

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

[U-Boot] [PATCH v3 2/3] net: Add a command to access the EEPROM from ethernet devices

[Joe Hershberger]

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

[U-Boot] [PATCH v3 2/3] net: Add a command to access the EEPROM from ethernet devices

[Simon Glass]

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

[U-Boot] [PATCH v3 2/3] net: Add a command to access the EEPROM from ethernet devices

[Alban Bedel]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20141015/ffe9c05e/attachment.pgp>

[U-Boot] [PATCH v3 2/3] net: Add a command to access the EEPROM from ethernet devices

[Simon Glass]

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

[U-Boot] [PATCH v3 2/3] net: Add a command to access the EEPROM from ethernet devices

[Alban Bedel]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20141021/dd433873/attachment.pgp>

[U-Boot] [PATCH v3 2/3] net: Add a command to access the EEPROM from ethernet devices

[Simon Glass]

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

[U-Boot] [PATCH v3 2/3] net: Add a command to access the EEPROM from ethernet devices

[Marek Vasut]

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' 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.

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

[U-Boot] [PATCH v3 3/3] usb: eth: smsc95xx: Add EEPROM access support for LAN9514

[Alban Bedel]

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__ */
-- 
2.1.1