Patchwork [U-Boot,v4] powerpc/eeprom: cleanup mac command

login
register
mail settings
Submitter York Sun
Date Aug. 11, 2011, 3:31 p.m.
Message ID <1313076710-12662-1-git-send-email-yorksun@freescale.com>
Download mbox | patch
Permalink /patch/109631/
State Changes Requested
Headers show

Comments

York Sun - Aug. 11, 2011, 3:31 p.m.
Change the help message to be more helpful. Print argument format.
Fix MAX_NUM_PORTS to comply with v1 NXID format. Accept hexadecimal and
decimal for port count and index.

Signed-off-by: York Sun <yorksun@freescale.com>
---
 board/freescale/common/sys_eeprom.c |    8 ++++----
 common/cmd_mac.c                    |   29 +++++++++++++++++------------
 2 files changed, 21 insertions(+), 16 deletions(-)
Wolfgang Denk - Oct. 14, 2011, 9:32 p.m.
Dear York Sun,

In message <1313076710-12662-1-git-send-email-yorksun@freescale.com> you wrote:
> Change the help message to be more helpful. Print argument format.
> Fix MAX_NUM_PORTS to comply with v1 NXID format. Accept hexadecimal and
> decimal for port count and index.
...

>  	case 'p':	/* MAC table size */
> -		e.mac_count = simple_strtoul(argv[2], NULL, 16);
> +		e.mac_count = simple_strtoul(argv[2], NULL, 0);
>  		update_crc();
>  		break;
> -	case '0' ... '9':	/* "mac 0" through "mac 22" */
> -		set_mac_address(simple_strtoul(argv[1], NULL, 10), argv[2]);
> +	case '0' ... '9':	/* "mac 0" through "mac 31" */
> +		set_mac_address(simple_strtoul(argv[1], NULL, 0), argv[2]);

These changes silently change the behaviour of the command.  So far,
an argument line "24" would be parsed as hex number (i. e. result in
decimal value 36); with this change it suddenly becomes devcimal 24,
which is quite unexpected.

There are very few exceptions wher eU-Boot uses and expects decimal
numbers, and I strongly recommend not to add any new such
inconsistencies.

> +#ifdef CONFIG_SYS_I2C_EEPROM_NXID
> +	"    - program the MAC address for port n [n=0...30]"

0...30 ?  The code above says "mac 0" through "mac 31"?
Or is it 0 ... 22 ?

Best regards,

Wolfgang Denk
Tabi Timur-B04825 - Oct. 14, 2011, 9:38 p.m.
On Fri, Oct 14, 2011 at 4:32 PM, Wolfgang Denk <wd@denx.de> wrote:

> These changes silently change the behaviour of the command.  So far,
> an argument line "24" would be parsed as hex number (i. e. result in
> decimal value 36); with this change it suddenly becomes devcimal 24,
> which is quite unexpected.

Support for numbers larger than 9 is new, so very few people would be affected.

> There are very few exceptions wher eU-Boot uses and expects decimal
> numbers, and I strongly recommend not to add any new such
> inconsistencies.

I understand, except that in this case, it's counter-intuitive.  No
one is going to think to do this:

     mac ports b

to set the number of ports to 11, but that's what you have to do.
Wolfgang Denk - Oct. 14, 2011, 9:46 p.m.
Dear Tabi Timur-B04825,

In message <CAOZdJXUhGKnFCkpS1bPkCHswsvqn-Ki8y=aXZH51_fZKWT8Qmw@mail.gmail.com> you wrote:
> 
> > There are very few exceptions wher eU-Boot uses and expects decimal
> > numbers, and I strongly recommend not to add any new such
> > inconsistencies.
>
> I understand, except that in this case, it's counter-intuitive.  No
> one is going to think to do this:
>
>      mac ports b
>
> to set the number of ports to 11, but that's what you have to do.

I'm not sure how you counted to get the "no one" result, but this is
_exactly_ what I would do, knowing very well that "mac ports 11" would
mean a decimal value of 17.

So far, the only command (that I remember) that accepts decimal
argument is "sleep", and I'm still angry with myself that I let this
slip through.

Please do not add more such inconsistencies.

Best regards,

Wolfgang Denk
York Sun - Oct. 14, 2011, 10:31 p.m.
Wolfgang,

On Fri, 2011-10-14 at 23:32 +0200, Wolfgang Denk wrote:
> Dear York Sun,
> 
> In message <1313076710-12662-1-git-send-email-yorksun@freescale.com> you wrote:
> > Change the help message to be more helpful. Print argument format.
> > Fix MAX_NUM_PORTS to comply with v1 NXID format. Accept hexadecimal and
> > decimal for port count and index.
> ...
> 
> >  	case 'p':	/* MAC table size */
> > -		e.mac_count = simple_strtoul(argv[2], NULL, 16);
> > +		e.mac_count = simple_strtoul(argv[2], NULL, 0);
> >  		update_crc();
> >  		break;
> > -	case '0' ... '9':	/* "mac 0" through "mac 22" */
> > -		set_mac_address(simple_strtoul(argv[1], NULL, 10), argv[2]);
> > +	case '0' ... '9':	/* "mac 0" through "mac 31" */
> > +		set_mac_address(simple_strtoul(argv[1], NULL, 0), argv[2]);
> 
> These changes silently change the behaviour of the command.  So far,
> an argument line "24" would be parsed as hex number (i. e. result in
> decimal value 36); with this change it suddenly becomes devcimal 24,
> which is quite unexpected.
> 
> There are very few exceptions wher eU-Boot uses and expects decimal
> numbers, and I strongly recommend not to add any new such
> inconsistencies.

Changing back to hex number is not a problem. But it brings a problem
when trying to set mac address to ports 10-15. For example, for port 14,
you may want to use

mac e xxxxxxxx

where 'e' is another defined command. In order to make this work, we
need to use two bytes for cmd.

Back to my current patch, it was from another nice feedback. To accept
both hex and decimal. In order to use hex, you will have to prefix it
with 0x.

Regards,

York
Wolfgang Denk - Oct. 15, 2011, 9:39 a.m.
Dear York Sun,

In message <1318631500.7780.56.camel@oslab-l1> you wrote:
> 
> > There are very few exceptions wher eU-Boot uses and expects decimal
> > numbers, and I strongly recommend not to add any new such
> > inconsistencies.
> 
> Changing back to hex number is not a problem. But it brings a problem
> when trying to set mac address to ports 10-15. For example, for port 14,
> you may want to use
> 
> mac e xxxxxxxx
> 
> where 'e' is another defined command. In order to make this work, we
> need to use two bytes for cmd.

No, not really.

Instead you should fix the command decode.  If there are potential
confusions with the "mac errata" command then it's obviously not
sufficient to compare just the first character of the sub-command
name.

Frankly, the whole implementation of this command is a mess.

It is IMO a major design bug that the command accepts sub-commands, but
then does not use a sub-command to program the MAC address for port n.

Instead of implementing your own (broken, as we see now) way to
implement sub-commands, you should use the standard way with a
sub-command table as used elsewhere (thenyou get correct decoding for
free) - of course, you need to get rid of the inconsistency in your
command interface first.


My recommendation is therefor: 1) introduce a "mac" subcommand for
programming the MAC (this solves the decode issue), and 2) convert the
code to proper sub-command decoding.

> Back to my current patch, it was from another nice feedback. To accept
> both hex and decimal. In order to use hex, you will have to prefix it
> with 0x.

Yes, that is what your patch would do.  But the standard input bae in
U-Boot is 16, not 10.  So your patch introduces inconsistent
behaviour, which is why I NAKed it.

Best regards,

Wolfgang Denk

Patch

diff --git a/board/freescale/common/sys_eeprom.c b/board/freescale/common/sys_eeprom.c
index d2ed036..bd3572b 100644
--- a/board/freescale/common/sys_eeprom.c
+++ b/board/freescale/common/sys_eeprom.c
@@ -34,7 +34,7 @@ 
 #endif
 
 #ifdef CONFIG_SYS_I2C_EEPROM_NXID
-#define MAX_NUM_PORTS	23
+#define MAX_NUM_PORTS	31
 #define NXID_VERSION	1
 #endif
 
@@ -398,11 +398,11 @@  int do_mac(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 		set_date(argv[2]);
 		break;
 	case 'p':	/* MAC table size */
-		e.mac_count = simple_strtoul(argv[2], NULL, 16);
+		e.mac_count = simple_strtoul(argv[2], NULL, 0);
 		update_crc();
 		break;
-	case '0' ... '9':	/* "mac 0" through "mac 22" */
-		set_mac_address(simple_strtoul(argv[1], NULL, 10), argv[2]);
+	case '0' ... '9':	/* "mac 0" through "mac 31" */
+		set_mac_address(simple_strtoul(argv[1], NULL, 0), argv[2]);
 		break;
 	case 'h':	/* help */
 	default:
diff --git a/common/cmd_mac.c b/common/cmd_mac.c
index 1884c2a..bd9cc19 100644
--- a/common/cmd_mac.c
+++ b/common/cmd_mac.c
@@ -29,21 +29,26 @@  extern int do_mac(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]);
 U_BOOT_CMD(
 	mac, 3, 1,  do_mac,
 	"display and program the system ID and MAC addresses in EEPROM",
-	"[read|save|id|num|errata|date|ports|0|1|2|3|4|5|6|7]\n"
+	"without argument\n"
+	"    - show content of system ID and MAC addresses\n"
 	"read\n"
-	"    - show content of EEPROM\n"
+	"    - read EEPROM without showing\n"
 	"mac save\n"
 	"    - save to the EEPROM\n"
 	"mac id\n"
-	"    - program system id\n"
-	"mac num\n"
-	"    - program system serial number\n"
-	"mac errata\n"
-	"    - program errata data\n"
-	"mac date\n"
-	"    - program date\n"
-	"mac ports\n"
+	"    - program system id (fixed)\n"
+	"mac num <string>\n"
+	"    - program <string> as system serial number\n"
+	"mac errata <string>\n"
+	"    - program <string> as errata data\n"
+	"mac date <YYMMDDhhmmss>\n"
+	"    - program timestamp\n"
+	"mac ports <n>\n"
 	"    - program the number of ports\n"
-	"mac X\n"
-	"    - program the MAC address for port X [X=0...7]"
+	"mac <n> <XX:XX:XX:XX:XX:XX>\n"
+#ifdef CONFIG_SYS_I2C_EEPROM_NXID
+	"    - program the MAC address for port n [n=0...30]"
+#else
+	"    - program the MAC address for port n [n=0...7]"
+#endif
 );