Thanks Florian, I will apply your suggestions.
> -----Original Message-----
> From: Florian Fainelli <f.faine...@gmail.com>
> Sent: Thursday, July 23, 2020 5:19 PM
> To: Bryan Whitehead - C21958 <bryan.whiteh...@microchip.com>;
> da...@davemloft.net
> Cc: netdev@vger.kernel.org; UNGLinuxDriver
> <unglinuxdri...@microchip.com>
> Subject: Re: [PATCH net-next] mscc: Add LCPLL Reset to VSC8574 Family of phy
> drivers
>
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
>
> On 7/23/20 1:09 PM, Bryan Whitehead wrote:
> > 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.
> >
> > Signed-off-by: Bryan Whitehead <bryan.whiteh...@microchip.com>
>
> Can you copy the PHY library maintainers for future changes such that this
> does
> not escape their review?
>
> > ---
> > drivers/net/phy/mscc/mscc_main.c | 90
> > ++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 90 insertions(+)
> >
> > diff --git a/drivers/net/phy/mscc/mscc_main.c
> > b/drivers/net/phy/mscc/mscc_main.c
> > index a4fbf3a..f2fa221 100644
> > --- a/drivers/net/phy/mscc/mscc_main.c
> > +++ b/drivers/net/phy/mscc/mscc_main.c
> > @@ -929,6 +929,90 @@ 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_wait_for_micro_complete(struct phy_device *phydev)
> > +{
> > + u16 timeout = 500;
> > + u16 reg18g = 0;
> > +
> > + reg18g = phy_base_read(phydev, 18);
> > + while (reg18g & 0x8000) {
> > + timeout--;
> > + if (timeout == 0)
> > + return -1;
> > + usleep_range(1000, 2000);
> > + reg18g = phy_base_read(phydev, 18);
>
> Please consider using phy_read_poll_timeout() instead of open coding this busy
> waiting loop.
>
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +/* bus->mdio_lock should be locked when using this function */ static
> > +int vsc8574_reset_lcpll(struct phy_device *phydev) {
> > + u16 reg_val = 0;
> > + int ret = 0;
> > +
> > + phy_base_write(phydev, MSCC_EXT_PAGE_ACCESS,
> > + MSCC_PHY_PAGE_EXTENDED_GPIO);
> > +
> > + /* Read LCPLL config vector into PRAM */
> > + phy_base_write(phydev, 18, 0x8023);
> > + ret = vsc8574_wait_for_micro_complete(phydev);
> > + if (ret)
> > + goto done;
>
> It might make sense to write a helper function that encapsulates the:
>
> - phy_base_write()
> - wait_for_complete
>
> pattern and use it throughout, with an option delay range argument so you can
> put that in there, too.
> --
> Florian
