| Message ID | 1341859963-420-1-git-send-email-timur@freescale.com |
|---|---|
| State | Accepted |
| Commit | 11af8d65274df736deeb651d12e0763eec527ea5 |
| Delegated to: | Joe Hershberger |
| Headers | show |
Dear Timur Tabi, > Now that phy_startup() can return an actual error code, check for that > error code and abort network initialization if the PHY fails. > > Signed-off-by: Timur Tabi <timur@freescale.com> > Acked-by: Nobuhiro Iwamamatsu <nobuhiro.iwamatsu.yj@renesas.com> (sh_eth > part) Acked-by: Stephan Linz <linz@li-pro.net> (Xilinx part, > xilinx_axi_emac and xilinx_ll_temac) Reviewed-by: Marek Vasut > <marex@denx.de> (FEC part) > --- > drivers/net/fec_mxc.c | 8 +++++++- > drivers/net/fm/eth.c | 9 ++++++++- > drivers/net/sh_eth.c | 6 +++++- > drivers/net/tsec.c | 8 +++++++- > drivers/net/xilinx_axi_emac.c | 6 +++++- > drivers/net/xilinx_ll_temac.c | 8 +++++++- > 6 files changed, 39 insertions(+), 6 deletions(-) > > diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c > index eee41d7..5700552 100644 A whining remark ... please keep changelog in here ;-) [...] Best regards, Marek Vasut
Hi Timur Tabi, On Mon, Jul 9, 2012 at 1:52 PM, Timur Tabi <timur@freescale.com> wrote: > Now that phy_startup() can return an actual error code, check for that error > code and abort network initialization if the PHY fails. > > Signed-off-by: Timur Tabi <timur@freescale.com> > Acked-by: Nobuhiro Iwamamatsu <nobuhiro.iwamatsu.yj@renesas.com> (sh_eth part) > Acked-by: Stephan Linz <linz@li-pro.net> (Xilinx part, xilinx_axi_emac and xilinx_ll_temac) > Reviewed-by: Marek Vasut <marex@denx.de> (FEC part) > --- > drivers/net/fec_mxc.c | 8 +++++++- > drivers/net/fm/eth.c | 9 ++++++++- > drivers/net/sh_eth.c | 6 +++++- > drivers/net/tsec.c | 8 +++++++- > drivers/net/xilinx_axi_emac.c | 6 +++++- > drivers/net/xilinx_ll_temac.c | 8 +++++++- > 6 files changed, 39 insertions(+), 6 deletions(-) > > diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c > index eee41d7..5700552 100644 > --- a/drivers/net/fec_mxc.c > +++ b/drivers/net/fec_mxc.c > @@ -510,7 +510,13 @@ static int fec_open(struct eth_device *edev) > fec_eth_phy_config(edev); > if (fec->phydev) { > /* Start up the PHY */ > - phy_startup(fec->phydev); > + int ret = phy_startup(fec->phydev); > + Why a blank line here when it isn't in the rest of the implementations? > + if (ret) { > + printf("Could not initialize PHY %s\n", > + fec->phydev->dev->name); > + return ret; > + } > speed = fec->phydev->speed; > } else { > speed = _100BASET; > diff --git a/drivers/net/fm/eth.c b/drivers/net/fm/eth.c > index f34f4db..2b616ad 100644 > --- a/drivers/net/fm/eth.c > +++ b/drivers/net/fm/eth.c > @@ -363,6 +363,9 @@ static int fm_eth_open(struct eth_device *dev, bd_t *bd) > { > struct fm_eth *fm_eth; > struct fsl_enet_mac *mac; > +#ifdef CONFIG_PHYLIB > + int ret; > +#endif > > fm_eth = (struct fm_eth *)dev->priv; > mac = fm_eth->mac; > @@ -384,7 +387,11 @@ static int fm_eth_open(struct eth_device *dev, bd_t *bd) > fmc_tx_port_graceful_stop_disable(fm_eth); > > #ifdef CONFIG_PHYLIB > - phy_startup(fm_eth->phydev); > + ret = phy_startup(fm_eth->phydev); > + if (ret) { > + printf("%s: Could not initialize\n", fm_eth->phydev->dev->name); Why is this string different from the others? Consistency? > + return ret; > + } > #else > fm_eth->phydev->speed = SPEED_1000; > fm_eth->phydev->link = 1; > diff --git a/drivers/net/sh_eth.c b/drivers/net/sh_eth.c > index bb57e4d..268d884 100644 > --- a/drivers/net/sh_eth.c > +++ b/drivers/net/sh_eth.c > @@ -415,7 +415,11 @@ static int sh_eth_config(struct sh_eth_dev *eth, bd_t *bd) > goto err_phy_cfg; > } > phy = port_info->phydev; > - phy_startup(phy); > + ret = phy_startup(phy); > + if (ret) { > + printf(SHETHER_NAME ": phy startup failure\n"); Why is this string different from the others? Consistency? > + return ret; > + } > > val = 0; > > diff --git a/drivers/net/tsec.c b/drivers/net/tsec.c > index 3c1c8f0..f5e314b 100644 > --- a/drivers/net/tsec.c > +++ b/drivers/net/tsec.c > @@ -480,6 +480,7 @@ static int tsec_init(struct eth_device *dev, bd_t * bd) > int i; > struct tsec_private *priv = (struct tsec_private *)dev->priv; > tsec_t *regs = priv->regs; > + int ret; > > /* Make sure the controller is stopped */ > tsec_halt(dev); > @@ -511,7 +512,12 @@ static int tsec_init(struct eth_device *dev, bd_t * bd) > startup_tsec(dev); > > /* Start up the PHY */ > - phy_startup(priv->phydev); > + ret = phy_startup(priv->phydev); > + if (ret) { > + printf("Could not initialize PHY %s\n", > + priv->phydev->dev->name); > + return ret; > + } > > adjust_link(priv, priv->phydev); > > diff --git a/drivers/net/xilinx_axi_emac.c b/drivers/net/xilinx_axi_emac.c > index 7854a04..d777144 100644 > --- a/drivers/net/xilinx_axi_emac.c > +++ b/drivers/net/xilinx_axi_emac.c > @@ -272,7 +272,11 @@ static int setup_phy(struct eth_device *dev) > phydev->advertising = phydev->supported; > priv->phydev = phydev; > phy_config(phydev); > - phy_startup(phydev); > + if (phy_startup(phydev)) { > + printf("axiemac: could not initialize PHY %s\n", Lower-case "could" in string? Consistency? > + phydev->dev->name); > + return 0; Why are you returning 0 here and not the return value from phy_startup()? > + } > > switch (phydev->speed) { > case 1000: > diff --git a/drivers/net/xilinx_ll_temac.c b/drivers/net/xilinx_ll_temac.c > index 27dafc1..b67153b 100644 > --- a/drivers/net/xilinx_ll_temac.c > +++ b/drivers/net/xilinx_ll_temac.c > @@ -232,6 +232,7 @@ static void ll_temac_halt(struct eth_device *dev) > static int ll_temac_init(struct eth_device *dev, bd_t *bis) > { > struct ll_temac *ll_temac = dev->priv; > + int ret; > > printf("%s: Xilinx XPS LocalLink Tri-Mode Ether MAC #%d at 0x%08X.\n", > dev->name, dev->index, dev->iobase); > @@ -240,7 +241,12 @@ static int ll_temac_init(struct eth_device *dev, bd_t *bis) > return -1; > > /* Start up the PHY */ > - phy_startup(ll_temac->phydev); > + ret = phy_startup(ll_temac->phydev); > + if (ret) { > + printf("%s: Could not initialize PHY %s\n", > + dev->name, ll_temac->phydev->dev->name); > + return ret; > + } > > if (!ll_temac_adjust_link(dev)) { > ll_temac_halt(dev); -Joe
Joe Hershberger wrote: >> diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c >> index eee41d7..5700552 100644 >> --- a/drivers/net/fec_mxc.c >> +++ b/drivers/net/fec_mxc.c >> @@ -510,7 +510,13 @@ static int fec_open(struct eth_device *edev) >> fec_eth_phy_config(edev); >> if (fec->phydev) { >> /* Start up the PHY */ >> - phy_startup(fec->phydev); >> + int ret = phy_startup(fec->phydev); >> + > > Why a blank line here when it isn't in the rest of the implementations? The coding standard requires blank lines after variable declarations. >> + if (ret) { >> + printf("Could not initialize PHY %s\n", >> + fec->phydev->dev->name); >> + return ret; >> + } >> speed = fec->phydev->speed; >> } else { >> speed = _100BASET; >> diff --git a/drivers/net/fm/eth.c b/drivers/net/fm/eth.c >> index f34f4db..2b616ad 100644 >> --- a/drivers/net/fm/eth.c >> +++ b/drivers/net/fm/eth.c >> @@ -363,6 +363,9 @@ static int fm_eth_open(struct eth_device *dev, bd_t *bd) >> { >> struct fm_eth *fm_eth; >> struct fsl_enet_mac *mac; >> +#ifdef CONFIG_PHYLIB >> + int ret; >> +#endif >> >> fm_eth = (struct fm_eth *)dev->priv; >> mac = fm_eth->mac; >> @@ -384,7 +387,11 @@ static int fm_eth_open(struct eth_device *dev, bd_t *bd) >> fmc_tx_port_graceful_stop_disable(fm_eth); >> >> #ifdef CONFIG_PHYLIB >> - phy_startup(fm_eth->phydev); >> + ret = phy_startup(fm_eth->phydev); >> + if (ret) { >> + printf("%s: Could not initialize\n", fm_eth->phydev->dev->name); > > Why is this string different from the others? Consistency? Yes. I tried to keep the messages consistent with the other messages in the function. > >> + return ret; >> + } >> #else >> fm_eth->phydev->speed = SPEED_1000; >> fm_eth->phydev->link = 1; >> diff --git a/drivers/net/sh_eth.c b/drivers/net/sh_eth.c >> index bb57e4d..268d884 100644 >> --- a/drivers/net/sh_eth.c >> +++ b/drivers/net/sh_eth.c >> @@ -415,7 +415,11 @@ static int sh_eth_config(struct sh_eth_dev *eth, bd_t *bd) >> goto err_phy_cfg; >> } >> phy = port_info->phydev; >> - phy_startup(phy); >> + ret = phy_startup(phy); >> + if (ret) { >> + printf(SHETHER_NAME ": phy startup failure\n"); > > Why is this string different from the others? Consistency? Yes, it looks like the other messages in sh_eth_config(). >> diff --git a/drivers/net/xilinx_axi_emac.c b/drivers/net/xilinx_axi_emac.c >> index 7854a04..d777144 100644 >> --- a/drivers/net/xilinx_axi_emac.c >> +++ b/drivers/net/xilinx_axi_emac.c >> @@ -272,7 +272,11 @@ static int setup_phy(struct eth_device *dev) >> phydev->advertising = phydev->supported; >> priv->phydev = phydev; >> phy_config(phydev); >> - phy_startup(phydev); >> + if (phy_startup(phydev)) { >> + printf("axiemac: could not initialize PHY %s\n", > > Lower-case "could" in string? Consistency? Yes, consistency. :-) > >> + phydev->dev->name); >> + return 0; > > Why are you returning 0 here and not the return value from phy_startup()? I answered that in another email. This is what setup_phy() is supposed to return on failure.
Hi Timur Tabi, On Tue, Jul 10, 2012 at 11:15 AM, Timur Tabi <timur@freescale.com> wrote: > Joe Hershberger wrote: > >>> diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c >>> index eee41d7..5700552 100644 >>> --- a/drivers/net/fec_mxc.c >>> +++ b/drivers/net/fec_mxc.c >>> @@ -510,7 +510,13 @@ static int fec_open(struct eth_device *edev) >>> fec_eth_phy_config(edev); >>> if (fec->phydev) { >>> /* Start up the PHY */ >>> - phy_startup(fec->phydev); >>> + int ret = phy_startup(fec->phydev); >>> + >> >> Why a blank line here when it isn't in the rest of the implementations? > > The coding standard requires blank lines after variable declarations. > >>> + if (ret) { >>> + printf("Could not initialize PHY %s\n", >>> + fec->phydev->dev->name); >>> + return ret; >>> + } >>> speed = fec->phydev->speed; >>> } else { >>> speed = _100BASET; >>> diff --git a/drivers/net/fm/eth.c b/drivers/net/fm/eth.c >>> index f34f4db..2b616ad 100644 >>> --- a/drivers/net/fm/eth.c >>> +++ b/drivers/net/fm/eth.c >>> @@ -363,6 +363,9 @@ static int fm_eth_open(struct eth_device *dev, bd_t *bd) >>> { >>> struct fm_eth *fm_eth; >>> struct fsl_enet_mac *mac; >>> +#ifdef CONFIG_PHYLIB >>> + int ret; >>> +#endif >>> >>> fm_eth = (struct fm_eth *)dev->priv; >>> mac = fm_eth->mac; >>> @@ -384,7 +387,11 @@ static int fm_eth_open(struct eth_device *dev, bd_t *bd) >>> fmc_tx_port_graceful_stop_disable(fm_eth); >>> >>> #ifdef CONFIG_PHYLIB >>> - phy_startup(fm_eth->phydev); >>> + ret = phy_startup(fm_eth->phydev); >>> + if (ret) { >>> + printf("%s: Could not initialize\n", fm_eth->phydev->dev->name); >> >> Why is this string different from the others? Consistency? > > Yes. I tried to keep the messages consistent with the other messages in > the function. Should you not at least keep the core message the same? "Could not initialize PHY" >> >>> + return ret; >>> + } >>> #else >>> fm_eth->phydev->speed = SPEED_1000; >>> fm_eth->phydev->link = 1; >>> diff --git a/drivers/net/sh_eth.c b/drivers/net/sh_eth.c >>> index bb57e4d..268d884 100644 >>> --- a/drivers/net/sh_eth.c >>> +++ b/drivers/net/sh_eth.c >>> @@ -415,7 +415,11 @@ static int sh_eth_config(struct sh_eth_dev *eth, bd_t *bd) >>> goto err_phy_cfg; >>> } >>> phy = port_info->phydev; >>> - phy_startup(phy); >>> + ret = phy_startup(phy); >>> + if (ret) { >>> + printf(SHETHER_NAME ": phy startup failure\n"); >> >> Why is this string different from the others? Consistency? > > Yes, it looks like the other messages in sh_eth_config(). Same here, at least the core message "Could not initialize PHY" >>> diff --git a/drivers/net/xilinx_axi_emac.c b/drivers/net/xilinx_axi_emac.c >>> index 7854a04..d777144 100644 >>> --- a/drivers/net/xilinx_axi_emac.c >>> +++ b/drivers/net/xilinx_axi_emac.c >>> @@ -272,7 +272,11 @@ static int setup_phy(struct eth_device *dev) >>> phydev->advertising = phydev->supported; >>> priv->phydev = phydev; >>> phy_config(phydev); >>> - phy_startup(phydev); >>> + if (phy_startup(phydev)) { >>> + printf("axiemac: could not initialize PHY %s\n", >> >> Lower-case "could" in string? Consistency? > > Yes, consistency. :-) OK... I'm not sold, but I don't care enough. >> >>> + phydev->dev->name); >>> + return 0; >> >> Why are you returning 0 here and not the return value from phy_startup()? > > I answered that in another email. This is what setup_phy() is supposed to > return on failure. OK. Thanks, -Joe
Joe Hershberger wrote: >>>> @@ -384,7 +387,11 @@ static int fm_eth_open(struct eth_device *dev, bd_t *bd) >>>> fmc_tx_port_graceful_stop_disable(fm_eth); >>>> >>>> #ifdef CONFIG_PHYLIB >>>> - phy_startup(fm_eth->phydev); >>>> + ret = phy_startup(fm_eth->phydev); >>>> + if (ret) { >>>> + printf("%s: Could not initialize\n", fm_eth->phydev->dev->name); >>> >>> Why is this string different from the others? Consistency? >> >> Yes. I tried to keep the messages consistent with the other messages in >> the function. > > Should you not at least keep the core message the same? "Could not > initialize PHY" Well, I suppose I could add the word "PHY" here. >>>> diff --git a/drivers/net/sh_eth.c b/drivers/net/sh_eth.c >>>> index bb57e4d..268d884 100644 >>>> --- a/drivers/net/sh_eth.c >>>> +++ b/drivers/net/sh_eth.c >>>> @@ -415,7 +415,11 @@ static int sh_eth_config(struct sh_eth_dev *eth, bd_t *bd) >>>> goto err_phy_cfg; >>>> } >>>> phy = port_info->phydev; >>>> - phy_startup(phy); >>>> + ret = phy_startup(phy); >>>> + if (ret) { >>>> + printf(SHETHER_NAME ": phy startup failure\n"); >>> >>> Why is this string different from the others? Consistency? >> >> Yes, it looks like the other messages in sh_eth_config(). > > Same here, at least the core message "Could not initialize PHY" I am saying that, just in a different way. "phy startup failure" means the same as "Could not initialize PHY", but the wording matches the rest of the function.
On Tue, Jul 10, 2012 at 1:35 PM, Timur Tabi <timur@freescale.com> wrote: > Joe Hershberger wrote: > >>>>> diff --git a/drivers/net/sh_eth.c b/drivers/net/sh_eth.c >>>>> index bb57e4d..268d884 100644 >>>>> --- a/drivers/net/sh_eth.c >>>>> +++ b/drivers/net/sh_eth.c >>>>> @@ -415,7 +415,11 @@ static int sh_eth_config(struct sh_eth_dev *eth, bd_t *bd) >>>>> goto err_phy_cfg; >>>>> } >>>>> phy = port_info->phydev; >>>>> - phy_startup(phy); >>>>> + ret = phy_startup(phy); >>>>> + if (ret) { >>>>> + printf(SHETHER_NAME ": phy startup failure\n"); >>>> >>>> Why is this string different from the others? Consistency? >>> >>> Yes, it looks like the other messages in sh_eth_config(). >> >> Same here, at least the core message "Could not initialize PHY" > > I am saying that, just in a different way. "phy startup failure" means > the same as "Could not initialize PHY", but the wording matches the rest > of the function. OK. -Joe
diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c index eee41d7..5700552 100644 --- a/drivers/net/fec_mxc.c +++ b/drivers/net/fec_mxc.c @@ -510,7 +510,13 @@ static int fec_open(struct eth_device *edev) fec_eth_phy_config(edev); if (fec->phydev) { /* Start up the PHY */ - phy_startup(fec->phydev); + int ret = phy_startup(fec->phydev); + + if (ret) { + printf("Could not initialize PHY %s\n", + fec->phydev->dev->name); + return ret; + } speed = fec->phydev->speed; } else { speed = _100BASET; diff --git a/drivers/net/fm/eth.c b/drivers/net/fm/eth.c index f34f4db..2b616ad 100644 --- a/drivers/net/fm/eth.c +++ b/drivers/net/fm/eth.c @@ -363,6 +363,9 @@ static int fm_eth_open(struct eth_device *dev, bd_t *bd) { struct fm_eth *fm_eth; struct fsl_enet_mac *mac; +#ifdef CONFIG_PHYLIB + int ret; +#endif fm_eth = (struct fm_eth *)dev->priv; mac = fm_eth->mac; @@ -384,7 +387,11 @@ static int fm_eth_open(struct eth_device *dev, bd_t *bd) fmc_tx_port_graceful_stop_disable(fm_eth); #ifdef CONFIG_PHYLIB - phy_startup(fm_eth->phydev); + ret = phy_startup(fm_eth->phydev); + if (ret) { + printf("%s: Could not initialize\n", fm_eth->phydev->dev->name); + return ret; + } #else fm_eth->phydev->speed = SPEED_1000; fm_eth->phydev->link = 1; diff --git a/drivers/net/sh_eth.c b/drivers/net/sh_eth.c index bb57e4d..268d884 100644 --- a/drivers/net/sh_eth.c +++ b/drivers/net/sh_eth.c @@ -415,7 +415,11 @@ static int sh_eth_config(struct sh_eth_dev *eth, bd_t *bd) goto err_phy_cfg; } phy = port_info->phydev; - phy_startup(phy); + ret = phy_startup(phy); + if (ret) { + printf(SHETHER_NAME ": phy startup failure\n"); + return ret; + } val = 0; diff --git a/drivers/net/tsec.c b/drivers/net/tsec.c index 3c1c8f0..f5e314b 100644 --- a/drivers/net/tsec.c +++ b/drivers/net/tsec.c @@ -480,6 +480,7 @@ static int tsec_init(struct eth_device *dev, bd_t * bd) int i; struct tsec_private *priv = (struct tsec_private *)dev->priv; tsec_t *regs = priv->regs; + int ret; /* Make sure the controller is stopped */ tsec_halt(dev); @@ -511,7 +512,12 @@ static int tsec_init(struct eth_device *dev, bd_t * bd) startup_tsec(dev); /* Start up the PHY */ - phy_startup(priv->phydev); + ret = phy_startup(priv->phydev); + if (ret) { + printf("Could not initialize PHY %s\n", + priv->phydev->dev->name); + return ret; + } adjust_link(priv, priv->phydev); diff --git a/drivers/net/xilinx_axi_emac.c b/drivers/net/xilinx_axi_emac.c index 7854a04..d777144 100644 --- a/drivers/net/xilinx_axi_emac.c +++ b/drivers/net/xilinx_axi_emac.c @@ -272,7 +272,11 @@ static int setup_phy(struct eth_device *dev) phydev->advertising = phydev->supported; priv->phydev = phydev; phy_config(phydev); - phy_startup(phydev); + if (phy_startup(phydev)) { + printf("axiemac: could not initialize PHY %s\n", + phydev->dev->name); + return 0; + } switch (phydev->speed) { case 1000: diff --git a/drivers/net/xilinx_ll_temac.c b/drivers/net/xilinx_ll_temac.c index 27dafc1..b67153b 100644 --- a/drivers/net/xilinx_ll_temac.c +++ b/drivers/net/xilinx_ll_temac.c @@ -232,6 +232,7 @@ static void ll_temac_halt(struct eth_device *dev) static int ll_temac_init(struct eth_device *dev, bd_t *bis) { struct ll_temac *ll_temac = dev->priv; + int ret; printf("%s: Xilinx XPS LocalLink Tri-Mode Ether MAC #%d at 0x%08X.\n", dev->name, dev->index, dev->iobase); @@ -240,7 +241,12 @@ static int ll_temac_init(struct eth_device *dev, bd_t *bis) return -1; /* Start up the PHY */ - phy_startup(ll_temac->phydev); + ret = phy_startup(ll_temac->phydev); + if (ret) { + printf("%s: Could not initialize PHY %s\n", + dev->name, ll_temac->phydev->dev->name); + return ret; + } if (!ll_temac_adjust_link(dev)) { ll_temac_halt(dev);