Re: [RFC PATCH net-next 3/3] net: phy: mscc: use phy_package_shared

From: Vladimir Oltean
Date: Thu Apr 23 2020 - 08:27:53 EST

Next message: Gerald Schaefer: "Re: [PATCH v2 6/9] s390/module: Use s390_kernel_write() for late relocations"
Previous message: tip-bot2 for Christoph Hellwig: "[tip: x86/mm] x86/mm: Cleanup pgprot_4k_2_large() and pgprot_large_2_4k()"
In reply to: Michael Walle: "[RFC PATCH net-next 3/3] net: phy: mscc: use phy_package_shared"
Next in thread: Andrew Lunn: "Re: [RFC PATCH net-next 1/3] net: phy: add concept of shared storage for PHYs"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Michael,

On Tue, 21 Apr 2020 at 02:28, Michael Walle <michael@xxxxxxxx> wrote:
>
> Use the new phy_package_shared common storage to ease the package
> initialization and to access the global registers.
>
> This was only compile-time tested!
>
> Signed-off-by: Michael Walle <michael@xxxxxxxx>
> ---

I happen to have a board with a VSC8514 PHY. For this driver, only the "Fast
Link Failure Indication" feature seems to be configured via
phy_base_write/__phy_package_write. Functionally I can't test it, because the
GPIO9 pin is not wired on my board. But I could at least check that the value
that should be written can be read back from all PHYs in the package:

Microsemi GE VSC8514 SyncE 0000:00:00.3:10: reading 0x400f from
MSCC_PHY_MAC_CFG_FASTLINK
swp0: PHY [0000:00:00.3:10] driver [Microsemi GE VSC8514 SyncE] (irq=POLL)
Microsemi GE VSC8514 SyncE 0000:00:00.3:11: reading 0x400f from
MSCC_PHY_MAC_CFG_FASTLINK
0000:00:00.5 swp1: PHY [0000:00:00.3:11] driver [Microsemi GE VSC8514
SyncE] (irq=POLL)
Microsemi GE VSC8514 SyncE 0000:00:00.3:12: reading 0x400f from
MSCC_PHY_MAC_CFG_FASTLINK
0000:00:00.5 swp2: PHY [0000:00:00.3:12] driver [Microsemi GE VSC8514
SyncE] (irq=POLL)
Microsemi GE VSC8514 SyncE 0000:00:00.3:13: reading 0x400f from
MSCC_PHY_MAC_CFG_FASTLINK
0000:00:00.5 swp3: PHY [0000:00:00.3:13] driver [Microsemi GE VSC8514
SyncE] (irq=POLL)

So assuming that the feature worked before, it looks like there is no
regression. Of course, the PHY still passes traffic just fine.

So you can add my:

Tested-by: Vladimir Oltean <vladimir.oltean@xxxxxxx>

> drivers/net/phy/mscc/mscc.h | 1 -
> drivers/net/phy/mscc/mscc_main.c | 99 ++++++++++----------------------
> 2 files changed, 29 insertions(+), 71 deletions(-)
>
> diff --git a/drivers/net/phy/mscc/mscc.h b/drivers/net/phy/mscc/mscc.h
> index 030bf8b600df..acdd8ee61a39 100644
> --- a/drivers/net/phy/mscc/mscc.h
> +++ b/drivers/net/phy/mscc/mscc.h
> @@ -353,7 +353,6 @@ struct vsc8531_private {
> const struct vsc85xx_hw_stat *hw_stats;
> u64 *stats;
> int nstats;
> - bool pkg_init;
> /* For multiple port PHYs; the MDIO address of the base PHY in the
> * package.
> */
> diff --git a/drivers/net/phy/mscc/mscc_main.c b/drivers/net/phy/mscc/mscc_main.c
> index 5391acdece05..382b56064de9 100644
> --- a/drivers/net/phy/mscc/mscc_main.c
> +++ b/drivers/net/phy/mscc/mscc_main.c
> @@ -691,27 +691,23 @@ static int vsc85xx_eee_init_seq_set(struct phy_device *phydev)
> /* phydev->bus->mdio_lock should be locked when using this function */
> static int phy_base_write(struct phy_device *phydev, u32 regnum, u16 val)
> {
> - struct vsc8531_private *priv = phydev->priv;
> -
> if (unlikely(!mutex_is_locked(&phydev->mdio.bus->mdio_lock))) {
> dev_err(&phydev->mdio.dev, "MDIO bus lock not held!\n");
> dump_stack();
> }
>
> - return __mdiobus_write(phydev->mdio.bus, priv->base_addr, regnum, val);
> + return __phy_package_write(phydev, regnum, val);
> }
>
> /* phydev->bus->mdio_lock should be locked when using this function */
> static int phy_base_read(struct phy_device *phydev, u32 regnum)
> {
> - struct vsc8531_private *priv = phydev->priv;
> -
> if (unlikely(!mutex_is_locked(&phydev->mdio.bus->mdio_lock))) {
> dev_err(&phydev->mdio.dev, "MDIO bus lock not held!\n");
> dump_stack();
> }
>
> - return __mdiobus_read(phydev->mdio.bus, priv->base_addr, regnum);
> + return __phy_package_read(phydev, regnum);
> }
>
> /* bus->mdio_lock should be locked when using this function */
> @@ -1287,65 +1283,36 @@ static int vsc8584_config_pre_init(struct phy_device *phydev)
> return ret;
> }
>
> -/* Check if one PHY has already done the init of the parts common to all PHYs
> - * in the Quad PHY package.
> - */
> -static bool vsc8584_is_pkg_init(struct phy_device *phydev, bool reversed)
> +static void vsc8584_get_base_addr(struct phy_device *phydev)
> {
> - struct mii_bus *bus = phydev->mdio.bus;
> - struct vsc8531_private *vsc8531;
> - struct phy_device *phy;
> - int i, addr;
> -
> - /* VSC8584 is a Quad PHY */
> - for (i = 0; i < 4; i++) {
> - vsc8531 = phydev->priv;
> -
> - if (reversed)
> - addr = vsc8531->base_addr - i;
> - else
> - addr = vsc8531->base_addr + i;
> -
> - phy = mdiobus_get_phy(bus, addr);
> - if (!phy)
> - continue;
> + struct vsc8531_private *vsc8531 = phydev->priv;
> + u16 val, addr;
>
> - if ((phy->phy_id & phydev->drv->phy_id_mask) !=
> - (phydev->drv->phy_id & phydev->drv->phy_id_mask))
> - continue;
> + mutex_lock(&phydev->mdio.bus->mdio_lock);
> + __phy_write(phydev, MSCC_EXT_PAGE_ACCESS, MSCC_PHY_PAGE_EXTENDED);
>
> - vsc8531 = phy->priv;
> + addr = __phy_read(phydev, MSCC_PHY_EXT_PHY_CNTL_4);
> + addr >>= PHY_CNTL_4_ADDR_POS;
>
> - if (vsc8531 && vsc8531->pkg_init)
> - return true;
> - }
> + val = __phy_read(phydev, MSCC_PHY_ACTIPHY_CNTL);
>
> - return false;
> + if (val & PHY_ADDR_REVERSED)
> + vsc8531->base_addr = phydev->mdio.addr + addr;
> + else
> + vsc8531->base_addr = phydev->mdio.addr - addr;
> + mutex_unlock(&phydev->mdio.bus->mdio_lock);
> }
>
> static int vsc8584_config_init(struct phy_device *phydev)
> {
> struct vsc8531_private *vsc8531 = phydev->priv;
> - u16 addr, val;
> int ret, i;
> + u16 val;
>
> phydev->mdix_ctrl = ETH_TP_MDI_AUTO;
>
> mutex_lock(&phydev->mdio.bus->mdio_lock);
>
> - __mdiobus_write(phydev->mdio.bus, phydev->mdio.addr,
> - MSCC_EXT_PAGE_ACCESS, MSCC_PHY_PAGE_EXTENDED);
> - addr = __mdiobus_read(phydev->mdio.bus, phydev->mdio.addr,
> - MSCC_PHY_EXT_PHY_CNTL_4);
> - addr >>= PHY_CNTL_4_ADDR_POS;
> -
> - val = __mdiobus_read(phydev->mdio.bus, phydev->mdio.addr,
> - MSCC_PHY_ACTIPHY_CNTL);
> - if (val & PHY_ADDR_REVERSED)
> - vsc8531->base_addr = phydev->mdio.addr + addr;
> - else
> - vsc8531->base_addr = phydev->mdio.addr - addr;
> -
> /* Some parts of the init sequence are identical for every PHY in the
> * package. Some parts are modifying the GPIO register bank which is a
> * set of registers that are affecting all PHYs, a few resetting the
> @@ -1359,7 +1326,7 @@ static int vsc8584_config_init(struct phy_device *phydev)
> * do the correct init sequence for all PHYs that are package-critical
> * in this pre-init function.
> */
> - if (!vsc8584_is_pkg_init(phydev, val & PHY_ADDR_REVERSED ? 1 : 0)) {
> + if (phy_package_init_once(phydev)) {
> /* The following switch statement assumes that the lowest
> * nibble of the phy_id_mask is always 0. This works because
> * the lowest nibble of the PHY_ID's below are also 0.
> @@ -1388,8 +1355,6 @@ static int vsc8584_config_init(struct phy_device *phydev)
> goto err;
> }
>
> - vsc8531->pkg_init = true;
> -
> phy_base_write(phydev, MSCC_EXT_PAGE_ACCESS,
> MSCC_PHY_PAGE_EXTENDED_GPIO);
>
> @@ -1427,7 +1392,8 @@ static int vsc8584_config_init(struct phy_device *phydev)
>
> /* Disable SerDes for 100Base-FX */
> ret = vsc8584_cmd(phydev, PROC_CMD_FIBER_MEDIA_CONF |
> - PROC_CMD_FIBER_PORT(addr) | PROC_CMD_FIBER_DISABLE |
> + PROC_CMD_FIBER_PORT(vsc8531->base_addr) |
> + PROC_CMD_FIBER_DISABLE |
> PROC_CMD_READ_MOD_WRITE_PORT |
> PROC_CMD_RST_CONF_PORT | PROC_CMD_FIBER_100BASE_FX);
> if (ret)
> @@ -1435,7 +1401,8 @@ static int vsc8584_config_init(struct phy_device *phydev)
>
> /* Disable SerDes for 1000Base-X */
> ret = vsc8584_cmd(phydev, PROC_CMD_FIBER_MEDIA_CONF |
> - PROC_CMD_FIBER_PORT(addr) | PROC_CMD_FIBER_DISABLE |
> + PROC_CMD_FIBER_PORT(vsc8531->base_addr) |
> + PROC_CMD_FIBER_DISABLE |
> PROC_CMD_READ_MOD_WRITE_PORT |
> PROC_CMD_RST_CONF_PORT | PROC_CMD_FIBER_1000BASE_X);
> if (ret)
> @@ -1750,26 +1717,14 @@ static int vsc8514_config_init(struct phy_device *phydev)
> {
> struct vsc8531_private *vsc8531 = phydev->priv;
> unsigned long deadline;
> - u16 val, addr;
> int ret, i;
> + u16 val;
> u32 reg;
>
> phydev->mdix_ctrl = ETH_TP_MDI_AUTO;
>
> mutex_lock(&phydev->mdio.bus->mdio_lock);
>
> - __phy_write(phydev, MSCC_EXT_PAGE_ACCESS, MSCC_PHY_PAGE_EXTENDED);
> -
> - addr = __phy_read(phydev, MSCC_PHY_EXT_PHY_CNTL_4);
> - addr >>= PHY_CNTL_4_ADDR_POS;
> -
> - val = __phy_read(phydev, MSCC_PHY_ACTIPHY_CNTL);
> -
> - if (val & PHY_ADDR_REVERSED)
> - vsc8531->base_addr = phydev->mdio.addr + addr;
> - else
> - vsc8531->base_addr = phydev->mdio.addr - addr;
> -
> /* Some parts of the init sequence are identical for every PHY in the
> * package. Some parts are modifying the GPIO register bank which is a
> * set of registers that are affecting all PHYs, a few resetting the
> @@ -1781,11 +1736,9 @@ static int vsc8514_config_init(struct phy_device *phydev)
> * do the correct init sequence for all PHYs that are package-critical
> * in this pre-init function.
> */
> - if (!vsc8584_is_pkg_init(phydev, val & PHY_ADDR_REVERSED ? 1 : 0))
> + if (phy_package_init_once(phydev))
> vsc8514_config_pre_init(phydev);
>
> - vsc8531->pkg_init = true;
> -
> phy_base_write(phydev, MSCC_EXT_PAGE_ACCESS,
> MSCC_PHY_PAGE_EXTENDED_GPIO);
>
> @@ -1991,6 +1944,9 @@ static int vsc8514_probe(struct phy_device *phydev)
>
> phydev->priv = vsc8531;
>
> + vsc8584_get_base_addr(phydev);
> + devm_phy_package_join(&phydev->mdio.dev, phydev, vsc8531->base_addr);
> +
> vsc8531->nleds = 4;
> vsc8531->supp_led_modes = VSC85XX_SUPP_LED_MODES;
> vsc8531->hw_stats = vsc85xx_hw_stats;
> @@ -2046,6 +2002,9 @@ static int vsc8584_probe(struct phy_device *phydev)
>
> phydev->priv = vsc8531;
>
> + vsc8584_get_base_addr(phydev);
> + devm_phy_package_join(&phydev->mdio.dev, phydev, vsc8531->base_addr);
> +
> vsc8531->nleds = 4;
> vsc8531->supp_led_modes = VSC8584_SUPP_LED_MODES;
> vsc8531->hw_stats = vsc8584_hw_stats;
> --
> 2.20.1
>

Thanks,
-Vladimir

Next message: Gerald Schaefer: "Re: [PATCH v2 6/9] s390/module: Use s390_kernel_write() for late relocations"
Previous message: tip-bot2 for Christoph Hellwig: "[tip: x86/mm] x86/mm: Cleanup pgprot_4k_2_large() and pgprot_large_2_4k()"
In reply to: Michael Walle: "[RFC PATCH net-next 3/3] net: phy: mscc: use phy_package_shared"
Next in thread: Andrew Lunn: "Re: [RFC PATCH net-next 1/3] net: phy: add concept of shared storage for PHYs"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]