| Message ID | 1320819979-3861-1-git-send-email-hs@denx.de |
|---|---|
| State | Superseded |
| Headers | show |
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
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
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@lists.denx.de > http://lists.denx.de/mailman/listinfo/u-boot >
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
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 >
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
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++) {
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(-)