[U-Boot] [PATCH 2/2] [v3] net: abort network initialization if the PHY driver fails
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); + + 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);
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
participants (3)
-
Joe Hershberger -
Marek Vasut -
Timur Tabi