| Message ID | 1595870308-19041-1-git-send-email-Bryan.Whitehead@microchip.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | [v2,net-next] mscc: Add LCPLL Reset to VSC8574 Family of phy drivers | expand |
From: Bryan Whitehead <Bryan.Whitehead@microchip.com> Date: Mon, 27 Jul 2020 13:18:28 -0400 > @@ -929,6 +929,77 @@ static bool vsc8574_is_serdes_init(struct phy_device *phydev) > } > > /* bus->mdio_lock should be locked when using this function */ > +/* Page should already be set to MSCC_PHY_PAGE_EXTENDED_GPIO */ > +static int vsc8574_micro_command(struct phy_device *phydev, u16 command) ... > +/* bus->mdio_lock should be locked when using this function */ Please don't dup this comment, it's not appropriate. Thank you.
On 7/30/20 4:36 PM, David Miller wrote: > From: Bryan Whitehead <Bryan.Whitehead@microchip.com> > Date: Mon, 27 Jul 2020 13:18:28 -0400 > >> @@ -929,6 +929,77 @@ static bool vsc8574_is_serdes_init(struct phy_device *phydev) >> } >> >> /* bus->mdio_lock should be locked when using this function */ >> +/* Page should already be set to MSCC_PHY_PAGE_EXTENDED_GPIO */ >> +static int vsc8574_micro_command(struct phy_device *phydev, u16 command) > ... >> +/* bus->mdio_lock should be locked when using this function */ > > Please don't dup this comment, it's not appropriate. Agree put a mutex assertion instead if you want to catch offenders at run time?
Thanks David and Florian, see below. > On 7/30/20 4:36 PM, David Miller wrote: > > From: Bryan Whitehead <Bryan.Whitehead@microchip.com> > > Date: Mon, 27 Jul 2020 13:18:28 -0400 > > > >> @@ -929,6 +929,77 @@ static bool vsc8574_is_serdes_init(struct > >> phy_device *phydev) } > >> > >> /* bus->mdio_lock should be locked when using this function */ > >> +/* Page should already be set to MSCC_PHY_PAGE_EXTENDED_GPIO */ > >> +static int vsc8574_micro_command(struct phy_device *phydev, u16 > >> +command) > > ... > >> +/* bus->mdio_lock should be locked when using this function */ > > > > Please don't dup this comment, it's not appropriate. > > Agree put a mutex assertion instead if you want to catch offenders at run time? > -- > Florian I was simply following the pattern that already exists in the driver. Would you like me to remove the same comment from the rest of the functions in the driver? The lock is already checked in the existing low level functions, phy_base_read, and phy_base_write. The check is of the following form if (unlikely(!mutex_is_locked(&phydev->mdio.bus->mdio_lock))) { dev_err(&phydev->mdio.dev, "MDIO bus lock not held!\n"); dump_stack(); } Is this a reasonable mutex assertion, or is there an existing preferred helper macro that can be used instead? Bryan
diff --git a/drivers/net/phy/mscc/mscc_main.c b/drivers/net/phy/mscc/mscc_main.c index a4fbf3a..db34faac 100644 --- a/drivers/net/phy/mscc/mscc_main.c +++ b/drivers/net/phy/mscc/mscc_main.c @@ -929,6 +929,77 @@ static bool vsc8574_is_serdes_init(struct phy_device *phydev) } /* bus->mdio_lock should be locked when using this function */ +/* Page should already be set to MSCC_PHY_PAGE_EXTENDED_GPIO */ +static int vsc8574_micro_command(struct phy_device *phydev, u16 command) +{ + u16 reg18g = 0; + + phy_base_write(phydev, 18, command); + + return read_poll_timeout(phy_base_read, reg18g, + !(reg18g & 0x8000), + 4000, 500000, 0, phydev, 18); +} + +/* bus->mdio_lock should be locked when using this function */ +static int vsc8574_reset_lcpll(struct phy_device *phydev) +{ + int ret = 0; + + phy_base_write(phydev, MSCC_EXT_PAGE_ACCESS, + MSCC_PHY_PAGE_EXTENDED_GPIO); + + /* Read LCPLL config vector into PRAM */ + ret = vsc8574_micro_command(phydev, 0x8023); + if (ret) + goto done; + + /* Set Address to Poke */ + ret = vsc8574_micro_command(phydev, 0xd7d5); + if (ret) + goto done; + + /* Poke to reset PLL Start up State Machine, + * set disable_fsm:bit 119 + */ + ret = vsc8574_micro_command(phydev, 0x8d06); + if (ret) + goto done; + + /* Rewrite PLL config vector */ + ret = vsc8574_micro_command(phydev, 0x80c0); + if (ret) + goto done; + + usleep_range(10000, 20000); + + /* Poke to deassert Reset of PLL State Machine, + * clear disable_fsm:bit 119 + */ + ret = vsc8574_micro_command(phydev, 0x8506); + if (ret) + goto done; + + /* Rewrite PLL config vector */ + ret = vsc8574_micro_command(phydev, 0x80c0); + if (ret) + goto done; + + usleep_range(10000, 20000); + + phy_base_write(phydev, MSCC_EXT_PAGE_ACCESS, + MSCC_PHY_PAGE_EXTENDED_3); + phy_base_read(phydev, 20); + phy_base_read(phydev, 20); + + usleep_range(110000, 200000); + +done: + phy_base_write(phydev, MSCC_EXT_PAGE_ACCESS, MSCC_PHY_PAGE_STANDARD); + return ret; +} + +/* bus->mdio_lock should be locked when using this function */ static int vsc8574_config_pre_init(struct phy_device *phydev) { static const struct reg_val pre_init1[] = { @@ -1002,6 +1073,12 @@ static int vsc8574_config_pre_init(struct phy_device *phydev) bool serdes_init; int ret; + ret = vsc8574_reset_lcpll(phydev); + if (ret) { + dev_err(dev, "failed lcpll reset\n"); + return ret; + } + phy_base_write(phydev, MSCC_EXT_PAGE_ACCESS, MSCC_PHY_PAGE_STANDARD); /* all writes below are broadcasted to all PHYs in the same package */
The LCPLL Reset sequence is added to the initialization path of the VSC8574 Family of phy drivers. The LCPLL Reset sequence is known to reduce hardware inter-op issues when using the QSGMII MAC interface. This patch is submitted to net-next to avoid merging conflicts that may arise if submitted to net. V2 Updates: Make use of read_poll_timeout for micro command completion. Combine command starter and timeout into helper function vsc8574_micro_command Removed unused variable reg_val from vsc8574_reset_lcpll Signed-off-by: Bryan Whitehead <Bryan.Whitehead@microchip.com> --- drivers/net/phy/mscc/mscc_main.c | 77 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 77 insertions(+)