[U-Boot] [RFC PATCH] common: miiphyutil: Work and report phy address in hex in mdio cmd

[U-Boot] [RFC PATCH] common: miiphyutil: Work and report phy address in hex in mdio cmd

[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 <michal.simek@xilinx.com>
---

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

diff --git a/cmd/mdio.c b/cmd/mdio.c
index fb13d050752a..21dc103736e7 100644
--- a/cmd/mdio.c
+++ b/cmd/mdio.c
@@ -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++) {
diff --git a/common/miiphyutil.c b/common/miiphyutil.c
index d8ebb384dbfa..aca18db52a00 100644
--- a/common/miiphyutil.c
+++ b/common/miiphyutil.c
@@ -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);
diff --git a/drivers/net/zynq_gem.c b/drivers/net/zynq_gem.c
index 3319e10467d0..526eac658ac5 100644
--- a/drivers/net/zynq_gem.c
+++ b/drivers/net/zynq_gem.c
@@ -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;
-- 
1.9.1

[U-Boot] [RFC PATCH] common: miiphyutil: Work and report phy address in hex in mdio cmd

[Dongpo Li]

On 2016/11/16 16:24, Michal Simek wrote:
> 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 <michal.simek@xilinx.com>
> ---
> 
>  cmd/mdio.c             | 6 +++---
>  common/miiphyutil.c    | 2 +-
>  drivers/net/zynq_gem.c | 2 +-
>  3 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/cmd/mdio.c b/cmd/mdio.c
> index fb13d050752a..21dc103736e7 100644
> --- a/cmd/mdio.c
> +++ b/cmd/mdio.c
> @@ -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.

>  	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 at address %x:\n", addr);
Maybe '0x%x' is more clear than '%x'.

>  
>  		for (devad = devadlo; devad <= devadhi; devad++) {
>  			for (reg = reglo; reg <= reghi; reg++) {
> diff --git a/common/miiphyutil.c b/common/miiphyutil.c
> index d8ebb384dbfa..aca18db52a00 100644
> --- a/common/miiphyutil.c
> +++ b/common/miiphyutil.c
> @@ -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.

>  
>  				if (phydev->dev)
>  					printf(" <--> %s\n", phydev->dev->name);
> diff --git a/drivers/net/zynq_gem.c b/drivers/net/zynq_gem.c
> index 3319e10467d0..526eac658ac5 100644
> --- a/drivers/net/zynq_gem.c
> +++ b/drivers/net/zynq_gem.c
> @@ -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.

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


    Regards,
    Dongpo

.

[U-Boot] [RFC PATCH] common: miiphyutil: Work and report phy address in hex in mdio cmd

[Michal Simek]

On 16.11.2016 10:22, Dongpo Li wrote:
> 
> 
> On 2016/11/16 16:24, Michal Simek wrote:
>> 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 <michal.simek@xilinx.com>
>> ---
>>
>>  cmd/mdio.c             | 6 +++---
>>  common/miiphyutil.c    | 2 +-
>>  drivers/net/zynq_gem.c | 2 +-
>>  3 files changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/cmd/mdio.c b/cmd/mdio.c
>> index fb13d050752a..21dc103736e7 100644
>> --- a/cmd/mdio.c
>> +++ b/cmd/mdio.c
>> @@ -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.

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

[U-Boot] [RFC PATCH] common: miiphyutil: Work and report phy address in hex in mdio cmd

[Dongpo Li]

On 2016/11/16 17:31, Michal Simek wrote:
> On 16.11.2016 10:22, Dongpo Li wrote:
>>
>>
>> On 2016/11/16 16:24, Michal Simek wrote:
>>> 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 <michal.simek@xilinx.com>
>>> ---
>>>
>>>  cmd/mdio.c             | 6 +++---
>>>  common/miiphyutil.c    | 2 +-
>>>  drivers/net/zynq_gem.c | 2 +-
>>>  3 files changed, 5 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/cmd/mdio.c b/cmd/mdio.c
>>> index fb13d050752a..21dc103736e7 100644
>>> --- a/cmd/mdio.c
>>> +++ b/cmd/mdio.c
>>> @@ -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.
> 
> 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    ................
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.

[U-Boot] [RFC PATCH] common: miiphyutil: Work and report phy address in hex in mdio cmd

[Michal Simek]

On 16.11.2016 11:08, Dongpo Li wrote:
> 
> 
> On 2016/11/16 17:31, Michal Simek wrote:
>> On 16.11.2016 10:22, Dongpo Li wrote:
>>>
>>>
>>> On 2016/11/16 16:24, Michal Simek wrote:
>>>> 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 <michal.simek@xilinx.com>
>>>> ---
>>>>
>>>>  cmd/mdio.c             | 6 +++---
>>>>  common/miiphyutil.c    | 2 +-
>>>>  drivers/net/zynq_gem.c | 2 +-
>>>>  3 files changed, 5 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/cmd/mdio.c b/cmd/mdio.c
>>>> index fb13d050752a..21dc103736e7 100644
>>>> --- a/cmd/mdio.c
>>>> +++ b/cmd/mdio.c
>>>> @@ -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.
>>
>> 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    ................
> 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

[U-Boot] [RFC PATCH] common: miiphyutil: Work and report phy address in hex in mdio cmd

[Simon Glass]

On 16 November 2016 at 01:24, Michal Simek <michal.simek@xilinx.com> wrote:
> 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 <michal.simek@xilinx.com>
> ---
>
>  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 <sjg@chromium.org>

[U-Boot] [RFC PATCH] common: miiphyutil: Work and report phy address in hex in mdio cmd

[Joe Hershberger]

On Wed, Nov 16, 2016 at 2:24 AM, Michal Simek <michal.simek@xilinx.com> wrote:
> 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 <michal.simek@xilinx.com>

Acked-by: Joe Hershberger <joe.hershberger@ni.com>