[U-Boot] [PATCH 2/2] [v3] net: abort network initialization if the PHY driver fails

[Joe Hershberger]

Hi Timur Tabi,

On Mon, Jul 9, 2012 at 1:52 PM, Timur Tabi <timur at 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 at freescale.com>
> Acked-by: Nobuhiro Iwamamatsu <nobuhiro.iwamatsu.yj at renesas.com> (sh_eth part)
> Acked-by: Stephan Linz <linz at li-pro.net> (Xilinx part, xilinx_axi_emac and xilinx_ll_temac)
> Reviewed-by: Marek Vasut <marex at 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