From mboxrd@z Thu Jan  1 00:00:00 1970
From: Joe Hershberger <joe.hershberger@ni.com>
Date: Wed, 23 Jan 2019 14:12:26 +0000
Subject: [U-Boot] [PATCH v2 1/3] net: phy: Add support for accessing MMD
 PHY registers
In-Reply-To: <20190123131314.4514-2-ccaione@baylibre.com>
References: <20190123131314.4514-1-ccaione@baylibre.com>
 <20190123131314.4514-2-ccaione@baylibre.com>
Message-ID: <CANr=Z=Y4BX+Fi51kzydE0QUncNxDjyfQdSxwb3kLDO7H0juBxA@mail.gmail.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

On Wed, Jan 23, 2019 at 7:14 AM Carlo Caione <ccaione@baylibre.com> wrote:
>
> Two new helper functions (phy_read_mmd() and phy_write_mmd()) are added
> to allow access to the MMD PHY registers.
>
> The MMD PHY registers can be accessed by two means:
>
> 1. Using two new MMD access function hooks in the PHY driver. These
> functions can be implemented when the PHY driver does not support the
> standard IEEE Compatible clause 45 access mechanism described in clause
> 22 or if the PHY uses its own non-standard access mechanism.
>
> 2. The standard clause 45 access extensions to the MMD registers through
> the indirection registers (clause 22) in all the other cases.
>
> Signed-off-by: Carlo Caione <ccaione@baylibre.com>
> ---
>  drivers/net/phy/phy.c |  4 +++
>  include/phy.h         | 62 +++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 66 insertions(+)
>
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index cda4caa803..6769047407 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -549,6 +549,10 @@ int phy_register(struct phy_driver *drv)
>                 drv->readext += gd->reloc_off;
>         if (drv->writeext)
>                 drv->writeext += gd->reloc_off;
> +       if (drv->read_mmd)
> +               drv->read_mmd += gd->reloc_off;
> +       if (drv->write_mmd)
> +               drv->write_mmd += gd->reloc_off;
>  #endif
>         return 0;
>  }
> diff --git a/include/phy.h b/include/phy.h
> index b86fdfb2ce..0ce41661fa 100644
> --- a/include/phy.h
> +++ b/include/phy.h
> @@ -101,6 +101,13 @@ struct phy_driver {
>         int (*readext)(struct phy_device *phydev, int addr, int devad, int reg);
>         int (*writeext)(struct phy_device *phydev, int addr, int devad, int reg,
>                         u16 val);
> +
> +       /* Phy specific driver override for reading a MMD register */
> +       int (*read_mmd)(struct phy_device *phydev, int devad, int reg);
> +
> +       /* Phy specific driver override for writing a MMD register */
> +       int (*write_mmd)(struct phy_device *phydev, int devad, int reg, u16 val);
> +
>         struct list_head list;
>  };
>
> @@ -164,6 +171,61 @@ static inline int phy_write(struct phy_device *phydev, int devad, int regnum,
>         return bus->write(bus, phydev->addr, devad, regnum, val);
>  }
>
> +static inline void phy_mmd_indirect(struct phy_device *phydev, int devad,
> +                                   int regnum)
> +{
> +       /* Write the desired MMD Devad */
> +       phy_write(phydev, MDIO_DEVAD_NONE, MII_MMD_CTRL, devad);
> +
> +       /* Write the desired MMD register address */
> +       phy_write(phydev, MDIO_DEVAD_NONE, MII_MMD_DATA, regnum);
> +
> +       /* Select the Function : DATA with no post increment */
> +       phy_write(phydev, MDIO_DEVAD_NONE, MII_MMD_CTRL, (devad | MII_MMD_CTRL_NOINCR));
> +}
> +
> +static inline int phy_read_mmd(struct phy_device *phydev, int devad,
> +                              int regnum)
> +{
> +       int ret;
> +
> +       if (regnum > (u16)~0 || devad > 32)
> +               return -EINVAL;
> +
> +       if (phydev->drv->read_mmd) {
> +               ret = phydev->drv->read_mmd(phydev, devad, regnum);
> +       } else {
> +               phy_mmd_indirect(phydev, devad, regnum);

I think this function should have a name that more clearly conveys
that it doesn't actually complete an indirect access.

Maybe phy_mmd_start_indirect() ?

> +
> +               /* Read the content of the MMD's selected register */
> +               ret = phy_read(phydev, MDIO_DEVAD_NONE, MII_MMD_DATA);
> +       }
> +
> +       return ret;

You shouldn't need this variable... just return from the 2 sites that
return errors.

> +}
> +
> +static inline int phy_write_mmd(struct phy_device *phydev, int devad,
> +                               int regnum, u16 val)
> +{
> +       int ret;
> +
> +       if (regnum > (u16)~0 || devad > 32)
> +               return -EINVAL;
> +
> +       if (phydev->drv->write_mmd) {
> +               ret = phydev->drv->write_mmd(phydev, devad, regnum, val);

You could simply return from here.

> +       } else {
> +               phy_mmd_indirect(phydev, devad, regnum);
> +
> +               /* Write the data into MMD's selected register */
> +               phy_write(phydev, MDIO_DEVAD_NONE, MII_MMD_DATA, val);
> +
> +               ret = 0;

You should return any error from phy_write above and get rid of the
ret variable.

> +       }
> +
> +       return ret;
> +}
> +
>  #ifdef CONFIG_PHYLIB_10G
>  extern struct phy_driver gen10g_driver;
>
> --
> 2.19.1
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot