| Message ID | 1313076710-12662-1-git-send-email-yorksun@freescale.com |
|---|---|
| State | Changes Requested |
| Headers | show |
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
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.
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
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
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
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 );
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(-)