netdev - RE: [PATCH net-next] mscc: Add LCPLL Reset to VSC8574 Family of phy drivers

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <MN2PR11MB3662EE0CFF3200534A728764FA770@MN2PR11MB3662.namprd11.prod.outlook.com>
Date:   Fri, 24 Jul 2020 15:38:40 +0000
From:   <Bryan.Whitehead@...rochip.com>
To:     <andrew@...n.ch>
CC:     <davem@...emloft.net>, <netdev@...r.kernel.org>,
        <UNGLinuxDriver@...rochip.com>
Subject: RE: [PATCH net-next] mscc: Add LCPLL Reset to VSC8574 Family of phy
 drivers

Thanks Andrew, I will apply your suggestions.

> -----Original Message-----
> From: Andrew Lunn <andrew@...n.ch>
> Sent: Friday, July 24, 2020 9:19 AM
> To: Bryan Whitehead - C21958 <Bryan.Whitehead@...rochip.com>
> Cc: davem@...emloft.net; netdev@...r.kernel.org; UNGLinuxDriver
> <UNGLinuxDriver@...rochip.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 Thu, Jul 23, 2020 at 04:09:57PM -0400, 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.Whitehead@...rochip.com>
> > ---
> >  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;
> 
> Hi Bryan
> 
> -ETIMEDOUT;
> 
> But as Florian said, please add a phy_base_read_poll_timeout() following what
> phy_read_poll_timeout() does.
> 
> > +             usleep_range(1000, 2000);
> > +             reg18g = phy_base_read(phydev, 18);
> > +     }
> > +
> > +     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;
> ...
> > +
> > +done:
> > +     phy_base_write(phydev, MSCC_EXT_PAGE_ACCESS,
> MSCC_PHY_PAGE_STANDARD);
> > +     return ret;
> > +}
> 
> So if vsc8574_wait_for_micro_complete() was to return -1, you pass it on.
> 
> > +
> > +/* 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 +1086,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;
> > +     }
> 
> And pass it on further. It could reach user space as an errno. It is just safer to
> always use an errno value.
> 
>      Andrew