common: miiphyutil: Work and report phy address in hex in mdio cmd

RFC PATCH New Michal Simek 4 participants 7 messages Nov 16, 2016 – Nov 29, 2016

Subsystems (2)

Network drivers Arm/Zynq architecture

lore

Share & tools

lore

Michal Simek

author

2016-11-16 08:24 UTC #0 lore raw

RFC PATCH 1/1 common: miiphyutil: Work and report phy address in hex in mdio cmd

It is confusing that mdio commands work and report phy id as decimal value when mii is working with hex values.

For example:
ZynqMP> mdio list
gem:
21 - TI DP83867 <--> ethernet at ff0e0000
ZynqMP> mdio read ethernet at ff0e0000 0
Reading from bus gem
PHY at address 21:
0 - 0x1140
ZynqMP> mii dump 21 0
Incorrect PHY address. Range should be 0-31
...
ZynqMP> mii dump 15
0. (1140) -- PHY control register --
(8000:0000) 0.15 = 0 reset

U-Boot normally takes hex values that's why this patch is changing mdio command to handle hex instead of changing mii command to handle decimal values.

Signed-off-by Michal Simek <[email protected]>

---

 cmd/mdio.c             | 6 +++---
 common/miiphyutil.c    | 2 +-
 drivers/net/zynq_gem.c | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

cmd/mdio.c +3 -3

index fb13d050752a..21dc103736e7 100644
@@ -27,12 +27,12 @@ static uint last_reg_hi;
 static int extract_range(char *input, int *plo, int *phi)
 {
 	char *end;
-	*plo = simple_strtol(input, &end, 0);
+	*plo = simple_strtol(input, &end, 16);
 	if (end == input)
 		return -1;
 
 	if ((*end == '-') && *(++end))
-		*phi = simple_strtol(end, NULL, 0);
+		*phi = simple_strtol(end, NULL, 16);
 	else if (*end == '\0')
 		*phi = *plo;
 	else

@@ -79,7 +79,7 @@ static int mdio_read_ranges(struct phy_device *phydev, struct mii_dev *bus,
 
 	printf("Reading from bus %s\n", bus->name);
 	for (addr = addrlo; addr <= addrhi; addr++) {
-		printf("PHY at address %d:\n", addr);
+		printf("PHY@address %x:\n", addr);
 
 		for (devad = devadlo; devad <= devadhi; devad++) {
 			for (reg = reglo; reg <= reghi; reg++) {

common/miiphyutil.c +1 -1

index d8ebb384dbfa..aca18db52a00 100644
@@ -135,7 +135,7 @@ void mdio_list_devices(void)
 			struct phy_device *phydev = bus->phymap[i];
 
 			if (phydev) {
-				printf("%d - %s", i, phydev->drv->name);
+				printf("%x - %s", i, phydev->drv->name);
 
 				if (phydev->dev)
 					printf(" <--> %s\n", phydev->dev->name);

drivers/net/zynq_gem.c +1 -1

index 3319e10467d0..526eac658ac5 100644
@@ -706,7 +706,7 @@ static int zynq_gem_ofdata_to_platdata(struct udevice *dev)
 
 	priv->emio = fdtdec_get_bool(gd->fdt_blob, dev->of_offset, "xlnx,emio");
 
-	printf("ZYNQ GEM: %lx, phyaddr %d, interface %s\n", (ulong)priv->iobase,
+	printf("ZYNQ GEM: %lx, phyaddr %x, interface %s\n", (ulong)priv->iobase,
 	       priv->phyaddr, phy_string_for_interface(priv->interface));
 
 	return 0;

Signature · 1 line

1.9.1

Dongpo Li

2016-11-16 09:22 UTC #1 lore raw

RFC PATCH 1/1 common: miiphyutil: Work and report phy address in hex in mdio cmd

Quoted 35 lines · Michal Simek

It is confusing that mdio commands work and report phy id as decimal value when mii is working with hex values.
For example:
ZynqMP> mdio list
gem:
21 - TI DP83867 <--> ethernet at ff0e0000
ZynqMP> mdio read ethernet at ff0e0000 0
Reading from bus gem
PHY at address 21:
0 - 0x1140
ZynqMP> mii dump 21 0
Incorrect PHY address. Range should be 0-31
...
ZynqMP> mii dump 15
0. (1140) -- PHY control register --
(8000:0000) 0.15 = 0 reset
U-Boot normally takes hex values that's why this patch is changing mdio command to handle hex instead of changing mii command to handle decimal values.
Signed-off-by Michal Simek <[email protected]>
---
 cmd/mdio.c             | 6 +++---
 common/miiphyutil.c    | 2 +-
 drivers/net/zynq_gem.c | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)
cmd/mdio.c +0 -0
index fb13d050752a..21dc103736e7 100644

cmd/mdio.c static uint last_reg_hi; · line 30

Quoted 5 lines

cmd/mdio.c +1 -1

@@ -27,12 +27,12 @@ static uint last_reg_hi;
 static int extract_range(char *input, int *plo, int *phi)
 {
 	char *end;
-	*plo = simple_strtol(input, &end, 0);
+	*plo = simple_strtol(input, &end, 16);

Hi, the last parameter 0 of function simple_strtol should mean
it can recognize the integer automatic as hex value if the data is beginning with 0x, as octet value if beginning with 0, else decimal value.
So this code has no problem.
I checked the implementation of function simple_strtol and found the following code:

        if (!base)
                base = 10;

So I think we should fix the implementation of simple_strtol.

Quoted 10 lines

cmd/mdio.c +1 -1

 	if (end == input)
 		return -1;
 
 	if ((*end == '-') && *(++end))
-		*phi = simple_strtol(end, NULL, 0);
+		*phi = simple_strtol(end, NULL, 16);
 	else if (*end == '\0')
 		*phi = *plo;
 	else

cmd/mdio.c static int mdio_read_ranges(struct phy_device *phydev, struct mii_dev *bus, · line 82

Quoted 5 lines

cmd/mdio.c +1 -1

@@ -79,7 +79,7 @@ static int mdio_read_ranges(struct phy_device *phydev, struct mii_dev *bus,
 
 	printf("Reading from bus %s\n", bus->name);
 	for (addr = addrlo; addr <= addrhi; addr++) {
-		printf("PHY at address %d:\n", addr);
+		printf("PHY at address %x:\n", addr);

Maybe '0x%x' is more clear than '%x'.

common/miiphyutil.c void mdio_list_devices(void) · line 138

Quoted 13 lines

cmd/mdio.c +0 -0

 		for (devad = devadlo; devad <= devadhi; devad++) {
 			for (reg = reglo; reg <= reghi; reg++) {

common/miiphyutil.c +1 -1

index d8ebb384dbfa..aca18db52a00 100644
@@ -135,7 +135,7 @@ void mdio_list_devices(void)
 			struct phy_device *phydev = bus->phymap[i];
 
 			if (phydev) {
-				printf("%d - %s", i, phydev->drv->name);
+				printf("%x - %s", i, phydev->drv->name);

Ditto.

drivers/net/zynq_gem.c static int zynq_gem_ofdata_to_platdata(struct udevice *dev) · line 709

Quoted 13 lines

common/miiphyutil.c +0 -0

 				if (phydev->dev)
 					printf(" <--> %s\n", phydev->dev->name);

drivers/net/zynq_gem.c +1 -1

index 3319e10467d0..526eac658ac5 100644
@@ -706,7 +706,7 @@ static int zynq_gem_ofdata_to_platdata(struct udevice *dev)
 
 	priv->emio = fdtdec_get_bool(gd->fdt_blob, dev->of_offset, "xlnx,emio");
 
-	printf("ZYNQ GEM: %lx, phyaddr %d, interface %s\n", (ulong)priv->iobase,
+	printf("ZYNQ GEM: %lx, phyaddr %x, interface %s\n", (ulong)priv->iobase,

Ditto.

drivers/net/zynq_gem.c static int zynq_gem_ofdata_to_platdata(struct udevice *dev) · line 712

Quoted 4 lines

drivers/net/zynq_gem.c +0 -0

 	       priv->phyaddr, phy_string_for_interface(priv->interface));
 
 	return 0;

Regards,
Dongpo

Michal Simek

author

2016-11-16 09:31 UTC #2 lore raw

RFC PATCH 1/1 common: miiphyutil: Work and report phy address in hex in mdio cmd

Quoted 48 lines · Dongpo Li

Quoted 35 lines · Michal Simek
It is confusing that mdio commands work and report phy id as decimal value when mii is working with hex values.
For example:
ZynqMP> mdio list
gem:
21 - TI DP83867 <--> ethernet at ff0e0000
ZynqMP> mdio read ethernet at ff0e0000 0
Reading from bus gem
PHY at address 21:
0 - 0x1140
ZynqMP> mii dump 21 0
Incorrect PHY address. Range should be 0-31
...
ZynqMP> mii dump 15
0. (1140) -- PHY control register --
(8000:0000) 0.15 = 0 reset
U-Boot normally takes hex values that's why this patch is changing mdio command to handle hex instead of changing mii command to handle decimal values.
Signed-off-by Michal Simek <[email protected]>
---
 cmd/mdio.c             | 6 +++---
 common/miiphyutil.c    | 2 +-
 drivers/net/zynq_gem.c | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)
cmd/mdio.c +0 -0
index fb13d050752a..21dc103736e7 100644
cmd/mdio.c static uint last_reg_hi; · line 30
Quoted 5 lines
cmd/mdio.c +1 -1
@@ -27,12 +27,12 @@ static uint last_reg_hi;
 static int extract_range(char *input, int *plo, int *phi)
 {
 	char *end;
-	*plo = simple_strtol(input, &end, 0);
+	*plo = simple_strtol(input, &end, 16);
Hi, the last parameter 0 of function simple_strtol should mean
it can recognize the integer automatic as hex value if the data is beginning with 0x, as octet value if beginning with 0, else decimal value.
So this code has no problem.
I checked the implementation of function simple_strtol and found the following code:

Quoted 3 lines

        if (!base)
                base = 10;
So I think we should fix the implementation of simple_strtol.

yes. I have checked that implementation too. But the question is if this is aligned with others U-Boot parameters. It is confusing that with one command uses 21 as dec, another 15 or 0x15.

That's why will be worth to synchronize this.

md is one example where all is taken as hex.

ZynqMP> md 15 10

00000015: 00000000 00000000 01000000 00000000    ................
00000025: 00000000 00000000 00000000 00000000    ................
00000035: 00000000 00000000 00000000 00000000    ................
00000045: 00000000 00000000 00000000 00000000    ................
ZynqMP> md f 10
0000000f: fe5000ff 000000ff 00000000 00000000    ..P.............
0000001f: 00000100 00000000 00000000 00000000    ................
0000002f: 00000000 00000000 00000000 00000000    ................
0000003f: 00000000 00000000 00000000 00000000    ................
ZynqMP> md 0xf 10
0000000f: fe5000ff 000000ff 00000000 00000000    ..P.............
0000001f: 00000100 00000000 00000000 00000000    ................
0000002f: 00000000 00000000 00000000 00000000    ................
0000003f: 00000000 00000000 00000000 00000000    ................

No problem to report hex values with 0x prefix.

Thanks,
Michal

Dongpo Li

2016-11-16 10:08 UTC #3 lore raw

RFC PATCH 1/1 common: miiphyutil: Work and report phy address in hex in mdio cmd

Quoted 62 lines · Michal Simek

Quoted 48 lines · Dongpo Li
Quoted 35 lines · Michal Simek
It is confusing that mdio commands work and report phy id as decimal value when mii is working with hex values.
For example:
ZynqMP> mdio list
gem:
21 - TI DP83867 <--> ethernet at ff0e0000
ZynqMP> mdio read ethernet at ff0e0000 0
Reading from bus gem
PHY at address 21:
0 - 0x1140
ZynqMP> mii dump 21 0
Incorrect PHY address. Range should be 0-31
...
ZynqMP> mii dump 15
0. (1140) -- PHY control register --
(8000:0000) 0.15 = 0 reset
U-Boot normally takes hex values that's why this patch is changing mdio command to handle hex instead of changing mii command to handle decimal values.
Signed-off-by Michal Simek <[email protected]>
---
 cmd/mdio.c             | 6 +++---
 common/miiphyutil.c    | 2 +-
 drivers/net/zynq_gem.c | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)
cmd/mdio.c +0 -0
index fb13d050752a..21dc103736e7 100644
cmd/mdio.c static uint last_reg_hi; · line 30
Quoted 5 lines
cmd/mdio.c +1 -1
@@ -27,12 +27,12 @@ static uint last_reg_hi;
 static int extract_range(char *input, int *plo, int *phi)
 {
 	char *end;
-	*plo = simple_strtol(input, &end, 0);
+	*plo = simple_strtol(input, &end, 16);
Hi, the last parameter 0 of function simple_strtol should mean
it can recognize the integer automatic as hex value if the data is beginning with 0x, as octet value if beginning with 0, else decimal value.
So this code has no problem.
I checked the implementation of function simple_strtol and found the following code:
Quoted 3 lines
        if (!base)
                base = 10;
So I think we should fix the implementation of simple_strtol.
yes. I have checked that implementation too. But the question is if this is aligned with others U-Boot parameters. It is confusing that with one command uses 21 as dec, another 15 or 0x15.
That's why will be worth to synchronize this.
md is one example where all is taken as hex.
ZynqMP> md 15 10

Quoted 14 lines

00000015: 00000000 00000000 01000000 00000000    ................
00000025: 00000000 00000000 00000000 00000000    ................
00000035: 00000000 00000000 00000000 00000000    ................
00000045: 00000000 00000000 00000000 00000000    ................
ZynqMP> md f 10
0000000f: fe5000ff 000000ff 00000000 00000000    ..P.............
0000001f: 00000100 00000000 00000000 00000000    ................
0000002f: 00000000 00000000 00000000 00000000    ................
0000003f: 00000000 00000000 00000000 00000000    ................
ZynqMP> md 0xf 10
0000000f: fe5000ff 000000ff 00000000 00000000    ..P.............
0000001f: 00000100 00000000 00000000 00000000    ................
0000002f: 00000000 00000000 00000000 00000000    ................
0000003f: 00000000 00000000 00000000 00000000    ................

I checked the md command, the code is:
addr = simple_strtoul(argv[1], NULL, 16);
length = simple_strtoul(argv[2], NULL, 16);
That's to say, you must consider the input addr and length as hex value if you want to use the md command.

Michal Simek

author

2016-11-16 10:18 UTC #4 lore raw

RFC PATCH 1/1 common: miiphyutil: Work and report phy address in hex in mdio cmd

Quoted 79 lines · Dongpo Li

Quoted 62 lines · Michal Simek
Quoted 48 lines · Dongpo Li
Quoted 35 lines · Michal Simek
It is confusing that mdio commands work and report phy id as decimal value when mii is working with hex values.
For example:
ZynqMP> mdio list
gem:
21 - TI DP83867 <--> ethernet at ff0e0000
ZynqMP> mdio read ethernet at ff0e0000 0
Reading from bus gem
PHY at address 21:
0 - 0x1140
ZynqMP> mii dump 21 0
Incorrect PHY address. Range should be 0-31
...
ZynqMP> mii dump 15
0. (1140) -- PHY control register --
(8000:0000) 0.15 = 0 reset
U-Boot normally takes hex values that's why this patch is changing mdio command to handle hex instead of changing mii command to handle decimal values.
Signed-off-by Michal Simek <[email protected]>
---
 cmd/mdio.c             | 6 +++---
 common/miiphyutil.c    | 2 +-
 drivers/net/zynq_gem.c | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)
cmd/mdio.c +0 -0
index fb13d050752a..21dc103736e7 100644
cmd/mdio.c static uint last_reg_hi; · line 30
Quoted 5 lines
cmd/mdio.c +1 -1
@@ -27,12 +27,12 @@ static uint last_reg_hi;
 static int extract_range(char *input, int *plo, int *phi)
 {
 	char *end;
-	*plo = simple_strtol(input, &end, 0);
+	*plo = simple_strtol(input, &end, 16);
Hi, the last parameter 0 of function simple_strtol should mean
it can recognize the integer automatic as hex value if the data is beginning with 0x, as octet value if beginning with 0, else decimal value.
So this code has no problem.
I checked the implementation of function simple_strtol and found the following code:
Quoted 3 lines
        if (!base)
                base = 10;
So I think we should fix the implementation of simple_strtol.
yes. I have checked that implementation too. But the question is if this is aligned with others U-Boot parameters. It is confusing that with one command uses 21 as dec, another 15 or 0x15.
That's why will be worth to synchronize this.
md is one example where all is taken as hex.
ZynqMP> md 15 10
Quoted 14 lines
00000015: 00000000 00000000 01000000 00000000    ................
00000025: 00000000 00000000 00000000 00000000    ................
00000035: 00000000 00000000 00000000 00000000    ................
00000045: 00000000 00000000 00000000 00000000    ................
ZynqMP> md f 10
0000000f: fe5000ff 000000ff 00000000 00000000    ..P.............
0000001f: 00000100 00000000 00000000 00000000    ................
0000002f: 00000000 00000000 00000000 00000000    ................
0000003f: 00000000 00000000 00000000 00000000    ................
ZynqMP> md 0xf 10
0000000f: fe5000ff 000000ff 00000000 00000000    ..P.............
0000001f: 00000100 00000000 00000000 00000000    ................
0000002f: 00000000 00000000 00000000 00000000    ................
0000003f: 00000000 00000000 00000000 00000000    ................

Quoted 5 lines

I checked the md command, the code is:
addr = simple_strtoul(argv[1], NULL, 16);
length = simple_strtoul(argv[2], NULL, 16);
That's to say, you must consider the input addr and length as hex value if you want to use the md command.

Right and this is just one example where it is expected hex value. IIRC this is quite common for other commands too that's why I sent this path to start discussion about it.

Thanks,
Michal

Simon Glass

2016-11-18 01:14 UTC #5 lore raw

RFC PATCH 1/1 common: miiphyutil: Work and report phy address in hex in mdio cmd

In reply to Michal Simek

Quoted 22 lines · Michal Simek

It is confusing that mdio commands work and report phy id as decimal value when mii is working with hex values.
For example:
ZynqMP> mdio list
gem:
21 - TI DP83867 <--> ethernet at ff0e0000
ZynqMP> mdio read ethernet at ff0e0000 0
Reading from bus gem
PHY at address 21:
0 - 0x1140
ZynqMP> mii dump 21 0
Incorrect PHY address. Range should be 0-31
...
ZynqMP> mii dump 15
0. (1140) -- PHY control register --
(8000:0000) 0.15 = 0 reset
U-Boot normally takes hex values that's why this patch is changing mdio command to handle hex instead of changing mii command to handle decimal values.

Quoted 7 lines

Signed-off-by Michal Simek <[email protected]>

---

 cmd/mdio.c             | 6 +++---
 common/miiphyutil.c    | 2 +-
 drivers/net/zynq_gem.c | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

Reviewed-by Simon Glass <[email protected]>

Joe Hershberger

2016-11-29 23:17 UTC #6 lore raw

RFC PATCH 1/1 common: miiphyutil: Work and report phy address in hex in mdio cmd

In reply to Michal Simek

Quoted 18 lines · Michal Simek

It is confusing that mdio commands work and report phy id as decimal value when mii is working with hex values.
For example:
ZynqMP> mdio list
gem:
21 - TI DP83867 <--> ethernet at ff0e0000
ZynqMP> mdio read ethernet at ff0e0000 0
Reading from bus gem
PHY at address 21:
0 - 0x1140
ZynqMP> mii dump 21 0
Incorrect PHY address. Range should be 0-31
...
ZynqMP> mii dump 15
0. (1140) -- PHY control register --
(8000:0000) 0.15 = 0 reset

Quoted 5 lines

U-Boot normally takes hex values that's why this patch is changing mdio command to handle hex instead of changing mii command to handle decimal values.
Signed-off-by Michal Simek <[email protected]>

Acked-by Joe Hershberger <[email protected]>