[U-Boot] [PATCH] arm, davinci_emac: fixup

[U-Boot] [PATCH] arm, davinci_emac: fixup

[Heiko Schocher]

since commits:
davinci: emac: add support for more than 1 PHYs
062fe7d332c28ede25626f448681e43d76bb312e

davinci: remove obsolete macro CONFIG_EMAC_MDIO_PHY_NUM
fb1d6332b5430b90a8fa8ebab709f33a60e9f816

I get following warning on the enbw_cmc board:

Err:   serial
Net:    5 ETH PHY detected
miiphy_register: non unique device name 'KSZ8873 @ 0x01'
DaVinci-EMAC
Hit any key to stop autoboot:  0

Also I see some debug printfs:

=> run load
+ emac_close
+ emac_ch_teardown
- emac_ch_teardown
+ emac_ch_teardown
- emac_ch_teardown
- emac_close
+ emac_open
- emac_open
Using DaVinci-EMAC device

reason is 062fe7d332c28ede25626f448681e43d76bb312e new define MAX_PHY.
This is set to 3! I get on this board 5 active phys, so
this leads in wrong memory writes ...

so I changed:

- MAX_PHY from 3 to 7
- print an error message if more then MAX_PHYs are
  detected.
- fill the active_phy_addr array in a for loop with
  0xff
- changed printf() in debug_emac()

Signed-off-by: Heiko Schocher <hs@denx.de>
Cc: Sandeep Paulraj <s-paulraj@ti.com>
Cc: Albert ARIBAUD <albert.u.boot@aribaud.net>
Cc: Wolfgang Denk <wd@denx.de>
Cc: Manjunath Hadli <manjunath.hadli@ti.com>
---
 drivers/net/davinci_emac.c |   17 +++++++++++------
 1 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/net/davinci_emac.c b/drivers/net/davinci_emac.c
index fa31159..a31e9f1 100644
--- a/drivers/net/davinci_emac.c
+++ b/drivers/net/davinci_emac.c
@@ -85,7 +85,7 @@ static int			emac_rx_queue_active = 0;
 /* Receive packet buffers */
 static unsigned char		emac_rx_buffers[EMAC_MAX_RX_BUFFERS * (EMAC_MAX_ETHERNET_PKT_SIZE + EMAC_PKT_ALIGN)];
 
-#define MAX_PHY		3
+#define MAX_PHY		7
 
 /* PHY address for a discovered PHY (0xff - not found) */
 static u_int8_t	active_phy_addr[MAX_PHY] = { 0xff, 0xff, 0xff };
@@ -160,9 +160,8 @@ static int davinci_eth_phy_detect(void)
 	int		j;
 	unsigned int	count = 0;
 
-	active_phy_addr[0] = 0xff;
-	active_phy_addr[1] = 0xff;
-	active_phy_addr[2] = 0xff;
+	for (i = 0; i < MAX_PHY; i++)
+		active_phy_addr[i] = 0xff;
 
 	udelay(1000);
 	phy_act_state = readl(&adap_mdio->ALIVE);
@@ -175,7 +174,13 @@ static int davinci_eth_phy_detect(void)
 	for (i = 0, j = 0; i < 32; i++)
 		if (phy_act_state & (1 << i)) {
 			count++;
-			active_phy_addr[j++] = i;
+			if (count < MAX_PHY)
+				active_phy_addr[j++] = i;
+			else {
+				printf("%s: to much PHYs detected.\n",
+					__func__);
+				break;
+			}
 		}
 
 	num_phy = count;
@@ -752,7 +757,7 @@ int davinci_emac_initialize(void)
 	if (!ret)
 		return(0);
 	else
-		printf(" %d ETH PHY detected\n", ret);
+		debug_emac(" %d ETH PHY detected\n", ret);
 
 	/* Get PHY ID and initialize phy_ops for a detected PHY */
 	for (i = 0; i < num_phy; i++) {
-- 
1.7.6.4

[U-Boot] [PATCH] arm, davinci_emac: fixup

[Wolfgang Denk]

Dear Heiko Schocher,

In message <1320819979-3861-1-git-send-email-hs@denx.de> you wrote:
...
> +			if (count < MAX_PHY)
> +				active_phy_addr[j++] = i;
> +			else {
> +				printf("%s: to much PHYs detected.\n",
> +					__func__);
> +				break;
> +			}

Coding Style: if the "else" branch uses braces, the "if" branch shall
use braces, too.

Also, please s/too much/too many/  in the printf() above.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Be kind to unkind people - they need it the most.

[U-Boot] [PATCH] arm, davinci_emac: fixup

[Heiko Schocher]

Hello Wolfgang,

Wolfgang Denk wrote:
> Dear Heiko Schocher,
> 
> In message <1320819979-3861-1-git-send-email-hs@denx.de> you wrote:
> ...
>> +			if (count < MAX_PHY)
>> +				active_phy_addr[j++] = i;
>> +			else {
>> +				printf("%s: to much PHYs detected.\n",
>> +					__func__);
>> +				break;
>> +			}
> 
> Coding Style: if the "else" branch uses braces, the "if" branch shall
> use braces, too.

Hups, you are right, just checked only with checkpatch ... sorry.
Changed!

> Also, please s/too much/too many/  in the printf() above.

Done.

Thanks!

bye,
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[U-Boot] [PATCH] arm, davinci_emac: fixup

[Prabhakar Lad]

Hi Heiko,

On Wed, Nov 9, 2011 at 11:56 AM, Heiko Schocher <hs@denx.de> wrote:

> since commits:
> davinci: emac: add support for more than 1 PHYs
> 062fe7d332c28ede25626f448681e43d76bb312e
>
> davinci: remove obsolete macro CONFIG_EMAC_MDIO_PHY_NUM
> fb1d6332b5430b90a8fa8ebab709f33a60e9f816
>
> I get following warning on the enbw_cmc board:
>
> Err:   serial
> Net:    5 ETH PHY detected
> miiphy_register: non unique device name 'KSZ8873 @ 0x01'
> DaVinci-EMAC
> Hit any key to stop autoboot:  0
>
> Also I see some debug printfs:
>
> => run load
> + emac_close
> + emac_ch_teardown
> - emac_ch_teardown
> + emac_ch_teardown
> - emac_ch_teardown
> - emac_close
> + emac_open
> - emac_open
> Using DaVinci-EMAC device
>
> reason is 062fe7d332c28ede25626f448681e43d76bb312e new define MAX_PHY.
> This is set to 3! I get on this board 5 active phys, so
> this leads in wrong memory writes ...
>
> so I changed:
>
> - MAX_PHY from 3 to 7
> - print an error message if more then MAX_PHYs are
>  detected.
> - fill the active_phy_addr array in a for loop with
>  0xff
> - changed printf() in debug_emac()
>
> Signed-off-by: Heiko Schocher <hs@denx.de>
> Cc: Sandeep Paulraj <s-paulraj@ti.com>
> Cc: Albert ARIBAUD <albert.u.boot@aribaud.net>
> Cc: Wolfgang Denk <wd@denx.de>
> Cc: Manjunath Hadli <manjunath.hadli@ti.com>
> ---
>  drivers/net/davinci_emac.c |   17 +++++++++++------
>  1 files changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/davinci_emac.c b/drivers/net/davinci_emac.c
> index fa31159..a31e9f1 100644
> --- a/drivers/net/davinci_emac.c
> +++ b/drivers/net/davinci_emac.c
> @@ -85,7 +85,7 @@ static int                    emac_rx_queue_active = 0;
>  /* Receive packet buffers */
>  static unsigned char           emac_rx_buffers[EMAC_MAX_RX_BUFFERS *
> (EMAC_MAX_ETHERNET_PKT_SIZE + EMAC_PKT_ALIGN)];
>
> -#define MAX_PHY                3
> +#define MAX_PHY                7
>
>  /* PHY address for a discovered PHY (0xff - not found) */
>  static u_int8_t        active_phy_addr[MAX_PHY] = { 0xff, 0xff, 0xff };
> @@ -160,9 +160,8 @@ static int davinci_eth_phy_detect(void)
>        int             j;
>        unsigned int    count = 0;
>
> -       active_phy_addr[0] = 0xff;
> -       active_phy_addr[1] = 0xff;
> -       active_phy_addr[2] = 0xff;
> +       for (i = 0; i < MAX_PHY; i++)
> +               active_phy_addr[i] = 0xff;
>
>        udelay(1000);
>        phy_act_state = readl(&adap_mdio->ALIVE);
> @@ -175,7 +174,13 @@ static int davinci_eth_phy_detect(void)
>        for (i = 0, j = 0; i < 32; i++)
>                if (phy_act_state & (1 << i)) {
>                        count++;
> -                       active_phy_addr[j++] = i;
> +                       if (count < MAX_PHY)
> +                               active_phy_addr[j++] = i;
> +                       else {
> +                               printf("%s: to much PHYs detected.\n",
> +                                       __func__);
>
       why not make here count = 0 and then break, so that later
       in davinci_emac_initialize() it wont initializes the phy's



Regards,
--Prabhakar Lad

> +                               break;
> +                       }
>                }
>
>        num_phy = count;
> @@ -752,7 +757,7 @@ int davinci_emac_initialize(void)
>        if (!ret)
>                return(0);
>        else
> -               printf(" %d ETH PHY detected\n", ret);
> +               debug_emac(" %d ETH PHY detected\n", ret);
>
>        /* Get PHY ID and initialize phy_ops for a detected PHY */
>        for (i = 0; i < num_phy; i++) {
> --
> 1.7.6.4
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>

[U-Boot] [PATCH] arm, davinci_emac: fixup

[Heiko Schocher]

Hello Prabhakar Lad,

Prabhakar Lad wrote:
> Hi Heiko,
> 
> On Wed, Nov 9, 2011 at 11:56 AM, Heiko Schocher <hs@denx.de> wrote:
[...]
>> Signed-off-by: Heiko Schocher <hs@denx.de>
>> Cc: Sandeep Paulraj <s-paulraj@ti.com>
>> Cc: Albert ARIBAUD <albert.u.boot@aribaud.net>
>> Cc: Wolfgang Denk <wd@denx.de>
>> Cc: Manjunath Hadli <manjunath.hadli@ti.com>
>> ---
>>  drivers/net/davinci_emac.c |   17 +++++++++++------
>>  1 files changed, 11 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/net/davinci_emac.c b/drivers/net/davinci_emac.c
>> index fa31159..a31e9f1 100644
>> --- a/drivers/net/davinci_emac.c
>> +++ b/drivers/net/davinci_emac.c
[...]
>> @@ -175,7 +174,13 @@ static int davinci_eth_phy_detect(void)
>>        for (i = 0, j = 0; i < 32; i++)
>>                if (phy_act_state & (1 << i)) {
>>                        count++;
>> -                       active_phy_addr[j++] = i;
>> +                       if (count < MAX_PHY)
>> +                               active_phy_addr[j++] = i;
>> +                       else {
>> +                               printf("%s: to much PHYs detected.\n",
>> +                                       __func__);
>>
>        why not make here count = 0 and then break, so that later
>        in davinci_emac_initialize() it wont initializes the phy's

I prefer here the error printf, because you see immediately what
is wrong...

bye,
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[U-Boot] [PATCH] arm, davinci_emac: fixup

[Prabhakar Lad]

Hi Heiko,

On Wed, Nov 9, 2011 at 5:56 PM, Heiko Schocher <hs@denx.de> wrote:

> Hello Prabhakar Lad,
>
> Prabhakar Lad wrote:
> > Hi Heiko,
> >
> > On Wed, Nov 9, 2011 at 11:56 AM, Heiko Schocher <hs@denx.de> wrote:
> [...]
> >> Signed-off-by: Heiko Schocher <hs@denx.de>
> >> Cc: Sandeep Paulraj <s-paulraj@ti.com>
> >> Cc: Albert ARIBAUD <albert.u.boot@aribaud.net>
> >> Cc: Wolfgang Denk <wd@denx.de>
> >> Cc: Manjunath Hadli <manjunath.hadli@ti.com>
> >> ---
> >>  drivers/net/davinci_emac.c |   17 +++++++++++------
> >>  1 files changed, 11 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/net/davinci_emac.c b/drivers/net/davinci_emac.c
> >> index fa31159..a31e9f1 100644
> >> --- a/drivers/net/davinci_emac.c
> >> +++ b/drivers/net/davinci_emac.c
> [...]
> >> @@ -175,7 +174,13 @@ static int davinci_eth_phy_detect(void)
> >>        for (i = 0, j = 0; i < 32; i++)
> >>                if (phy_act_state & (1 << i)) {
> >>                        count++;
> >> -                       active_phy_addr[j++] = i;
> >> +                       if (count < MAX_PHY)
> >> +                               active_phy_addr[j++] = i;
> >> +                       else {
> >> +                               printf("%s: to much PHYs detected.\n",
> >> +                                       __func__);
> >>
> >        why not make here count = 0 and then break, so that later
> >        in davinci_emac_initialize() it wont initializes the phy's
>
> I prefer here the error printf, because you see immediately what
> is wrong...
>
>    Agreed to have a printf, I was suggesting to even have a statement
count = 0;
   in that block, if you don't make count zero later
davinci_emac_initialize() function
   it will proceed further in initializing the phys , which i believe is
not correct.

  Regards,
  --Prabhakar Lad

> bye,
> Heiko
> --
> DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
>

[U-Boot] [PATCH] arm, davinci_emac: fixup

[Heiko Schocher]

Hello Prabhakar Lad,

Prabhakar Lad wrote:
> Hi Heiko,
> 
> On Wed, Nov 9, 2011 at 5:56 PM, Heiko Schocher <hs@denx.de> wrote:
> 
>> Hello Prabhakar Lad,
>>
>> Prabhakar Lad wrote:
>>> Hi Heiko,
>>>
>>> On Wed, Nov 9, 2011 at 11:56 AM, Heiko Schocher <hs@denx.de> wrote:
>> [...]
>>>> Signed-off-by: Heiko Schocher <hs@denx.de>
>>>> Cc: Sandeep Paulraj <s-paulraj@ti.com>
>>>> Cc: Albert ARIBAUD <albert.u.boot@aribaud.net>
>>>> Cc: Wolfgang Denk <wd@denx.de>
>>>> Cc: Manjunath Hadli <manjunath.hadli@ti.com>
>>>> ---
>>>>  drivers/net/davinci_emac.c |   17 +++++++++++------
>>>>  1 files changed, 11 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/net/davinci_emac.c b/drivers/net/davinci_emac.c
>>>> index fa31159..a31e9f1 100644
>>>> --- a/drivers/net/davinci_emac.c
>>>> +++ b/drivers/net/davinci_emac.c
>> [...]
>>>> @@ -175,7 +174,13 @@ static int davinci_eth_phy_detect(void)
>>>>        for (i = 0, j = 0; i < 32; i++)
>>>>                if (phy_act_state & (1 << i)) {
>>>>                        count++;
>>>> -                       active_phy_addr[j++] = i;
>>>> +                       if (count < MAX_PHY)
>>>> +                               active_phy_addr[j++] = i;
>>>> +                       else {
>>>> +                               printf("%s: to much PHYs detected.\n",
>>>> +                                       __func__);
>>>>
>>>        why not make here count = 0 and then break, so that later
>>>        in davinci_emac_initialize() it wont initializes the phy's
>> I prefer here the error printf, because you see immediately what
>> is wrong...
>>
>>    Agreed to have a printf, I was suggesting to even have a statement
> count = 0;
>    in that block, if you don't make count zero later
> davinci_emac_initialize() function
>    it will proceed further in initializing the phys , which i believe is
> not correct.

Ah, Ok, yes, I change this, thanks!

bye,
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[U-Boot] [PATCH v2] arm, davinci_emac: fixup

[Heiko Schocher]

since commits:
davinci: emac: add support for more than 1 PHYs
062fe7d332c28ede25626f448681e43d76bb312e

davinci: remove obsolete macro CONFIG_EMAC_MDIO_PHY_NUM
fb1d6332b5430b90a8fa8ebab709f33a60e9f816

I get following warning on the enbw_cmc board:

Err:   serial
Net:    5 ETH PHY detected
miiphy_register: non unique device name 'KSZ8873 @ 0x01'
DaVinci-EMAC
Hit any key to stop autoboot:  0

Also I see some debug printfs:

=> run load
+ emac_close
+ emac_ch_teardown
- emac_ch_teardown
+ emac_ch_teardown
- emac_ch_teardown
- emac_close
+ emac_open
- emac_open
Using DaVinci-EMAC device

reason is 062fe7d332c28ede25626f448681e43d76bb312e new define MAX_PHY.
This is set to 3! I get on this board 5 active phys, so
this leads in wrong memory writes ...

so I changed:

- MAX_PHY from 3 to 7
- print an error message if more then MAX_PHYs are
  detected.
- fill the active_phy_addr array in a for loop with
  0xff
- changed printf() in debug_emac()

Signed-off-by: Heiko Schocher <hs@denx.de>
Cc: Sandeep Paulraj <s-paulraj@ti.com>
Cc: Albert ARIBAUD <albert.u.boot@aribaud.net>
Cc: Wolfgang Denk <wd@denx.de>
Cc: Manjunath Hadli <manjunath.hadli@ti.com>
Cc: Prabhakar Lad <prabhakar.csengg@gmail.com>

---
- changes for v2:
  - add comments from Wolfgang Denk
    - Codingstyle cleanup
      if the "else" branch uses braces, the "if" branch shall
      use braces, too.
    - s/too much/too many/  in the added printf()
  - add comment from Prabhakar Lad:
    - add count = 0 in error case, so that no phys are
      initialized later.
---
 drivers/net/davinci_emac.c |   18 ++++++++++++------
 1 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/net/davinci_emac.c b/drivers/net/davinci_emac.c
index fa31159..1648e24 100644
--- a/drivers/net/davinci_emac.c
+++ b/drivers/net/davinci_emac.c
@@ -85,7 +85,7 @@ static int			emac_rx_queue_active = 0;
 /* Receive packet buffers */
 static unsigned char		emac_rx_buffers[EMAC_MAX_RX_BUFFERS * (EMAC_MAX_ETHERNET_PKT_SIZE + EMAC_PKT_ALIGN)];
 
-#define MAX_PHY		3
+#define MAX_PHY		7
 
 /* PHY address for a discovered PHY (0xff - not found) */
 static u_int8_t	active_phy_addr[MAX_PHY] = { 0xff, 0xff, 0xff };
@@ -160,9 +160,8 @@ static int davinci_eth_phy_detect(void)
 	int		j;
 	unsigned int	count = 0;
 
-	active_phy_addr[0] = 0xff;
-	active_phy_addr[1] = 0xff;
-	active_phy_addr[2] = 0xff;
+	for (i = 0; i < MAX_PHY; i++)
+		active_phy_addr[i] = 0xff;
 
 	udelay(1000);
 	phy_act_state = readl(&adap_mdio->ALIVE);
@@ -175,7 +174,14 @@ static int davinci_eth_phy_detect(void)
 	for (i = 0, j = 0; i < 32; i++)
 		if (phy_act_state & (1 << i)) {
 			count++;
-			active_phy_addr[j++] = i;
+			if (count < MAX_PHY) {
+				active_phy_addr[j++] = i;
+			} else {
+				printf("%s: to many PHYs detected.\n",
+					__func__);
+				count = 0;
+				break;
+			}
 		}
 
 	num_phy = count;
@@ -752,7 +758,7 @@ int davinci_emac_initialize(void)
 	if (!ret)
 		return(0);
 	else
-		printf(" %d ETH PHY detected\n", ret);
+		debug_emac(" %d ETH PHY detected\n", ret);
 
 	/* Get PHY ID and initialize phy_ops for a detected PHY */
 	for (i = 0; i < num_phy; i++) {
-- 
1.7.6.4

[U-Boot] [PATCH v2] arm, davinci_emac: fixup

[Mike Frysinger]

the summary "fixup" tells us nothing as to what you're fixing.  could you use a 
little more descriptive summary ?
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20111109/e78bff57/attachment.pgp 

[U-Boot] [PATCH v3] arm, davinci_emac: fix driver bug if more then 3 PHYs are detected

[Heiko Schocher]

since commits:
davinci: emac: add support for more than 1 PHYs
062fe7d332c28ede25626f448681e43d76bb312e

davinci: remove obsolete macro CONFIG_EMAC_MDIO_PHY_NUM
fb1d6332b5430b90a8fa8ebab709f33a60e9f816

I get following warning on the enbw_cmc board:

Err:   serial
Net:    5 ETH PHY detected
miiphy_register: non unique device name 'KSZ8873 @ 0x01'
DaVinci-EMAC
Hit any key to stop autoboot:  0

Also I see some debug printfs:

=> run load
+ emac_close
+ emac_ch_teardown
- emac_ch_teardown
+ emac_ch_teardown
- emac_ch_teardown
- emac_close
+ emac_open
- emac_open
Using DaVinci-EMAC device

reason is 062fe7d332c28ede25626f448681e43d76bb312e new define MAX_PHY.
This is set to 3! I get on this board 5 active phys, so
this leads in wrong memory writes ...

so I changed:

- MAX_PHY from 3 to 7
- print an error message if more then MAX_PHYs are
  detected.
- fill the active_phy_addr array in a for loop with
  0xff
- changed printf() in debug_emac()

Signed-off-by: Heiko Schocher <hs@denx.de>
Cc: Sandeep Paulraj <s-paulraj@ti.com>
Cc: Albert ARIBAUD <albert.u.boot@aribaud.net>
Cc: Wolfgang Denk <wd@denx.de>
Cc: Manjunath Hadli <manjunath.hadli@ti.com>
Cc: Prabhakar Lad <prabhakar.csengg@gmail.com>
Cc: Mike Frysinger <vapier@gentoo.org>

---
- changes for v2:
  - add comments from Wolfgang Denk
    - Codingstyle cleanup
      if the "else" branch uses braces, the "if" branch shall
      use braces, too.
    - s/too much/too many/  in the added printf()
  - add comment from Prabhakar Lad:
    - add count = 0 in error case, so that no phys are
      initialized later.
- changes for v3:
  - add comment from Mike Frysinger
    change subject line to a hopefully more descriptive summary

 drivers/net/davinci_emac.c |   18 ++++++++++++------
 1 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/net/davinci_emac.c b/drivers/net/davinci_emac.c
index fa31159..1648e24 100644
--- a/drivers/net/davinci_emac.c
+++ b/drivers/net/davinci_emac.c
@@ -85,7 +85,7 @@ static int			emac_rx_queue_active = 0;
 /* Receive packet buffers */
 static unsigned char		emac_rx_buffers[EMAC_MAX_RX_BUFFERS * (EMAC_MAX_ETHERNET_PKT_SIZE + EMAC_PKT_ALIGN)];
 
-#define MAX_PHY		3
+#define MAX_PHY		7
 
 /* PHY address for a discovered PHY (0xff - not found) */
 static u_int8_t	active_phy_addr[MAX_PHY] = { 0xff, 0xff, 0xff };
@@ -160,9 +160,8 @@ static int davinci_eth_phy_detect(void)
 	int		j;
 	unsigned int	count = 0;
 
-	active_phy_addr[0] = 0xff;
-	active_phy_addr[1] = 0xff;
-	active_phy_addr[2] = 0xff;
+	for (i = 0; i < MAX_PHY; i++)
+		active_phy_addr[i] = 0xff;
 
 	udelay(1000);
 	phy_act_state = readl(&adap_mdio->ALIVE);
@@ -175,7 +174,14 @@ static int davinci_eth_phy_detect(void)
 	for (i = 0, j = 0; i < 32; i++)
 		if (phy_act_state & (1 << i)) {
 			count++;
-			active_phy_addr[j++] = i;
+			if (count < MAX_PHY) {
+				active_phy_addr[j++] = i;
+			} else {
+				printf("%s: to many PHYs detected.\n",
+					__func__);
+				count = 0;
+				break;
+			}
 		}
 
 	num_phy = count;
@@ -752,7 +758,7 @@ int davinci_emac_initialize(void)
 	if (!ret)
 		return(0);
 	else
-		printf(" %d ETH PHY detected\n", ret);
+		debug_emac(" %d ETH PHY detected\n", ret);
 
 	/* Get PHY ID and initialize phy_ops for a detected PHY */
 	for (i = 0; i < num_phy; i++) {
-- 
1.7.6.4

[U-Boot] [PATCH v3] arm, davinci_emac: fix driver bug if more then 3 PHYs are detected

[Tom Rini]

On Wed, Nov 9, 2011 at 9:50 AM, Heiko Schocher <hs@denx.de> wrote:
> since commits:
> davinci: emac: add support for more than 1 PHYs
> 062fe7d332c28ede25626f448681e43d76bb312e
>
> davinci: remove obsolete macro CONFIG_EMAC_MDIO_PHY_NUM
> fb1d6332b5430b90a8fa8ebab709f33a60e9f816
[snip]
> - MAX_PHY from 3 to 7

Why don't we add a CONFIG here and default to 3, ie
#ifndef CONFIG_SYS_something
#define CONFIG_SYS_something 3
#endif

Thanks!

-- 
Tom

[U-Boot] [PATCH v3] arm, davinci_emac: fix driver bug if more then 3 PHYs are detected

[Heiko Schocher]

Hello Tom,

Tom Rini wrote:
> On Wed, Nov 9, 2011 at 9:50 AM, Heiko Schocher <hs@denx.de> wrote:
>> since commits:
>> davinci: emac: add support for more than 1 PHYs
>> 062fe7d332c28ede25626f448681e43d76bb312e
>>
>> davinci: remove obsolete macro CONFIG_EMAC_MDIO_PHY_NUM
>> fb1d6332b5430b90a8fa8ebab709f33a60e9f816
> [snip]
>> - MAX_PHY from 3 to 7
> 
> Why don't we add a CONFIG here and default to 3, ie
> #ifndef CONFIG_SYS_something
> #define CONFIG_SYS_something 3
> #endif

Do we really need a config option for this? Why 3 or 7 as I did?
Shouldn't we set this define to 32, as this is the max possible
PHYs? Ok, we loose some RAM with this option ...

Would CONFIG_SYS_DAVINCI_EMAC_PHY_COUNT a good name?

bye,
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[U-Boot] [PATCH v3] arm, davinci_emac: fix driver bug if more then 3 PHYs are detected

[Tom Rini]

On Wed, Nov 9, 2011 at 9:43 PM, Heiko Schocher <hs@denx.de> wrote:
> Hello Tom,
>
> Tom Rini wrote:
>> On Wed, Nov 9, 2011 at 9:50 AM, Heiko Schocher <hs@denx.de> wrote:
>>> since commits:
>>> davinci: emac: add support for more than 1 PHYs
>>> 062fe7d332c28ede25626f448681e43d76bb312e
>>>
>>> davinci: remove obsolete macro CONFIG_EMAC_MDIO_PHY_NUM
>>> fb1d6332b5430b90a8fa8ebab709f33a60e9f816
>> [snip]
>>> - MAX_PHY from 3 to 7
>>
>> Why don't we add a CONFIG here and default to 3, ie
>> #ifndef CONFIG_SYS_something
>> #define CONFIG_SYS_something 3
>> #endif
>
> Do we really need a config option for this? Why 3 or 7 as I did?
> Shouldn't we set this define to 32, as this is the max possible
> PHYs? Ok, we loose some RAM with this option ...
>
> Would CONFIG_SYS_DAVINCI_EMAC_PHY_COUNT a good name?

Yes, sounds like a good name to me, thanks.

-- 
Tom

[U-Boot] [PATCH v4] arm, davinci_emac: fix driver bug if more then 3 PHYs are detected

[Heiko Schocher]

since commits:
davinci: emac: add support for more than 1 PHYs
062fe7d332c28ede25626f448681e43d76bb312e

davinci: remove obsolete macro CONFIG_EMAC_MDIO_PHY_NUM
fb1d6332b5430b90a8fa8ebab709f33a60e9f816

I get following warning on the enbw_cmc board:

Err:   serial
Net:    5 ETH PHY detected
miiphy_register: non unique device name 'KSZ8873 @ 0x01'
DaVinci-EMAC
Hit any key to stop autoboot:  0

Also I see some debug printfs:

=> run load
+ emac_close
+ emac_ch_teardown
- emac_ch_teardown
+ emac_ch_teardown
- emac_ch_teardown
- emac_close
+ emac_open
- emac_open
Using DaVinci-EMAC device

reason is 062fe7d332c28ede25626f448681e43d76bb312e new define MAX_PHY.
This is set to 3! I get on this board 5 active phys, so
this leads in wrong memory writes ...

so I changed:

- define CONFIG_SYS_DAVINCI_EMAC_PHY_COUNT to set
  the MAX_PHY value, add a description in README
  for the new CONFIG_SYS option.
- print an error message if more then MAX_PHYs are
  detected.
- fill the active_phy_addr array in a for loop with
  0xff
- changed printf() in debug_emac()

Signed-off-by: Heiko Schocher <hs@denx.de>
Cc: Sandeep Paulraj <s-paulraj@ti.com>
Cc: Albert ARIBAUD <albert.u.boot@aribaud.net>
Cc: Wolfgang Denk <wd@denx.de>
Cc: Manjunath Hadli <manjunath.hadli@ti.com>
Cc: Prabhakar Lad <prabhakar.csengg@gmail.com>
Cc: Mike Frysinger <vapier@gentoo.org>
Cc: Tom Rini <tom.rini@gmail.com>

---
- changes for v2:
  - add comments from Wolfgang Denk
    - Codingstyle cleanup
      if the "else" branch uses braces, the "if" branch shall
      use braces, too.
    - s/too much/too many/  in the added printf()
  - add comment from Prabhakar Lad:
    - add count = 0 in error case, so that no phys are
      initialized later.
- changes for v3:
  - add comment from Mike Frysinger
    change subject line to a hopefully more descriptive summary
- changes for v4:
  - add comment from Tom Rini:
    introduce CONFIG_SYS_DAVINCI_EMAC_PHY_COUNT, so boards
    can define this, if they need a value != 3. Add this
    config option to the readme file

 README                     |    6 ++++++
 drivers/net/davinci_emac.c |   24 ++++++++++++++++--------
 2 files changed, 22 insertions(+), 8 deletions(-)

diff --git a/README b/README
index a2c7c9a..049e6c3 100644
--- a/README
+++ b/README
@@ -1027,6 +1027,12 @@ The following options need to be configured:
 			Define this to use i/o functions instead of macros
 			(some hardware wont work with macros)
 
+		CONFIG_DRIVER_TI_EMAC
+		Support for davinci emac
+
+			CONFIG_SYS_DAVINCI_EMAC_PHY_COUNT
+			Define this if you have more then 3 PHYs.
+
 		CONFIG_FTGMAC100
 		Support for Faraday's FTGMAC100 Gigabit SoC Ethernet
 
diff --git a/drivers/net/davinci_emac.c b/drivers/net/davinci_emac.c
index fa31159..36c33af 100644
--- a/drivers/net/davinci_emac.c
+++ b/drivers/net/davinci_emac.c
@@ -85,15 +85,17 @@ static int			emac_rx_queue_active = 0;
 /* Receive packet buffers */
 static unsigned char		emac_rx_buffers[EMAC_MAX_RX_BUFFERS * (EMAC_MAX_ETHERNET_PKT_SIZE + EMAC_PKT_ALIGN)];
 
-#define MAX_PHY		3
+#ifndef CONFIG_SYS_DAVINCI_EMAC_PHY_COUNT
+#define CONFIG_SYS_DAVINCI_EMAC_PHY_COUNT	3
+#endif
 
 /* PHY address for a discovered PHY (0xff - not found) */
-static u_int8_t	active_phy_addr[MAX_PHY] = { 0xff, 0xff, 0xff };
+static u_int8_t	active_phy_addr[CONFIG_SYS_DAVINCI_EMAC_PHY_COUNT];
 
 /* number of PHY found active */
 static u_int8_t	num_phy;
 
-phy_t				phy[MAX_PHY];
+phy_t				phy[CONFIG_SYS_DAVINCI_EMAC_PHY_COUNT];
 
 static int davinci_eth_set_mac_addr(struct eth_device *dev)
 {
@@ -160,9 +162,8 @@ static int davinci_eth_phy_detect(void)
 	int		j;
 	unsigned int	count = 0;
 
-	active_phy_addr[0] = 0xff;
-	active_phy_addr[1] = 0xff;
-	active_phy_addr[2] = 0xff;
+	for (i = 0; i < CONFIG_SYS_DAVINCI_EMAC_PHY_COUNT; i++)
+		active_phy_addr[i] = 0xff;
 
 	udelay(1000);
 	phy_act_state = readl(&adap_mdio->ALIVE);
@@ -175,7 +176,14 @@ static int davinci_eth_phy_detect(void)
 	for (i = 0, j = 0; i < 32; i++)
 		if (phy_act_state & (1 << i)) {
 			count++;
-			active_phy_addr[j++] = i;
+			if (count < CONFIG_SYS_DAVINCI_EMAC_PHY_COUNT) {
+				active_phy_addr[j++] = i;
+			} else {
+				printf("%s: to many PHYs detected.\n",
+					__func__);
+				count = 0;
+				break;
+			}
 		}
 
 	num_phy = count;
@@ -752,7 +760,7 @@ int davinci_emac_initialize(void)
 	if (!ret)
 		return(0);
 	else
-		printf(" %d ETH PHY detected\n", ret);
+		debug_emac(" %d ETH PHY detected\n", ret);
 
 	/* Get PHY ID and initialize phy_ops for a detected PHY */
 	for (i = 0; i < num_phy; i++) {
-- 
1.7.6.4

[U-Boot] [PATCH v4] arm, davinci_emac: fix driver bug if more then 3 PHYs are detected

[Tom Rini]

On Wed, Nov 9, 2011 at 11:00 PM, Heiko Schocher <hs@denx.de> wrote:
> since commits:
> davinci: emac: add support for more than 1 PHYs
> 062fe7d332c28ede25626f448681e43d76bb312e
>
> davinci: remove obsolete macro CONFIG_EMAC_MDIO_PHY_NUM
> fb1d6332b5430b90a8fa8ebab709f33a60e9f816
>
> I get following warning on the enbw_cmc board:
>
> Err: ? serial
> Net: ? ?5 ETH PHY detected
> miiphy_register: non unique device name 'KSZ8873 @ 0x01'
> DaVinci-EMAC
> Hit any key to stop autoboot: ?0
>
> Also I see some debug printfs:
>
> => run load
> + emac_close
> + emac_ch_teardown
> - emac_ch_teardown
> + emac_ch_teardown
> - emac_ch_teardown
> - emac_close
> + emac_open
> - emac_open
> Using DaVinci-EMAC device
>
> reason is 062fe7d332c28ede25626f448681e43d76bb312e new define MAX_PHY.
> This is set to 3! I get on this board 5 active phys, so
> this leads in wrong memory writes ...
>
> so I changed:
>
> - define CONFIG_SYS_DAVINCI_EMAC_PHY_COUNT to set
> ?the MAX_PHY value, add a description in README
> ?for the new CONFIG_SYS option.
> - print an error message if more then MAX_PHYs are
> ?detected.
> - fill the active_phy_addr array in a for loop with
> ?0xff
> - changed printf() in debug_emac()
>
> Signed-off-by: Heiko Schocher <hs@denx.de>
> Cc: Sandeep Paulraj <s-paulraj@ti.com>
> Cc: Albert ARIBAUD <albert.u.boot@aribaud.net>
> Cc: Wolfgang Denk <wd@denx.de>
> Cc: Manjunath Hadli <manjunath.hadli@ti.com>
> Cc: Prabhakar Lad <prabhakar.csengg@gmail.com>
> Cc: Mike Frysinger <vapier@gentoo.org>
> Cc: Tom Rini <tom.rini@gmail.com>

Acked-by: Tom Rini <trini@ti.com>

Thanks.

-- 
Tom