| Message ID | 1595534997-29187-1-git-send-email-Bryan.Whitehead@microchip.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net-next] mscc: Add LCPLL Reset to VSC8574 Family of phy drivers | expand |
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.Whitehead@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.
Hi Bryan,
I love your patch! Perhaps something to improve:
[auto build test WARNING on net-next/master]
url: https://github.com/0day-ci/linux/commits/Bryan-Whitehead/mscc-Add-LCPLL-Reset-to-VSC8574-Family-of-phy-drivers/20200724-043103
base: https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 15be4ea3f07034a50eee2db6f3fefd2bec582170
config: ia64-allmodconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=ia64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
drivers/net/phy/mscc/mscc_main.c: In function 'vsc8574_reset_lcpll':
>> drivers/net/phy/mscc/mscc_main.c:953:6: warning: variable 'reg_val' set but not used [-Wunused-but-set-variable]
953 | u16 reg_val = 0;
| ^~~~~~~
vim +/reg_val +953 drivers/net/phy/mscc/mscc_main.c
949
950 /* bus->mdio_lock should be locked when using this function */
951 static int vsc8574_reset_lcpll(struct phy_device *phydev)
952 {
> 953 u16 reg_val = 0;
954 int ret = 0;
955
956 phy_base_write(phydev, MSCC_EXT_PAGE_ACCESS,
957 MSCC_PHY_PAGE_EXTENDED_GPIO);
958
959 /* Read LCPLL config vector into PRAM */
960 phy_base_write(phydev, 18, 0x8023);
961 ret = vsc8574_wait_for_micro_complete(phydev);
962 if (ret)
963 goto done;
964
965 /* Set Address to Poke */
966 phy_base_write(phydev, 18, 0xd7d5);
967 ret = vsc8574_wait_for_micro_complete(phydev);
968 if (ret)
969 goto done;
970
971 /* Poke to reset PLL Start up State Machine,
972 * set disable_fsm:bit 119
973 */
974 phy_base_write(phydev, 18, 0x8d06);
975 ret = vsc8574_wait_for_micro_complete(phydev);
976 if (ret)
977 goto done;
978
979 /* Rewrite PLL config vector */
980 phy_base_write(phydev, 18, 0x80c0);
981 ret = vsc8574_wait_for_micro_complete(phydev);
982 if (ret)
983 goto done;
984
985 usleep_range(10000, 20000);
986
987 /* Poke to deassert Reset of PLL State Machine,
988 * clear disable_fsm:bit 119
989 */
990 phy_base_write(phydev, 18, 0x8506);
991 ret = vsc8574_wait_for_micro_complete(phydev);
992 if (ret)
993 goto done;
994
995 /* Rewrite PLL config vector */
996 phy_base_write(phydev, 18, 0x80c0);
997 ret = vsc8574_wait_for_micro_complete(phydev);
998 if (ret)
999 goto done;
1000
1001 usleep_range(10000, 20000);
1002
1003 phy_base_write(phydev, MSCC_EXT_PAGE_ACCESS,
1004 MSCC_PHY_PAGE_EXTENDED_3);
1005 reg_val = phy_base_read(phydev, 20);
1006 reg_val = phy_base_read(phydev, 20);
1007
1008 usleep_range(110000, 200000);
1009
1010 done:
1011 phy_base_write(phydev, MSCC_EXT_PAGE_ACCESS, MSCC_PHY_PAGE_STANDARD);
1012 return ret;
1013 }
1014
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
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@microchip.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
Thanks Florian, I will apply your suggestions. > -----Original Message----- > From: Florian Fainelli <f.fainelli@gmail.com> > Sent: Thursday, July 23, 2020 5:19 PM > To: Bryan Whitehead - C21958 <Bryan.Whitehead@microchip.com>; > davem@davemloft.net > Cc: netdev@vger.kernel.org; UNGLinuxDriver > <UNGLinuxDriver@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.Whitehead@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
Thanks Andrew, I will apply your suggestions. > -----Original Message----- > From: Andrew Lunn <andrew@lunn.ch> > Sent: Friday, July 24, 2020 9:19 AM > To: Bryan Whitehead - C21958 <Bryan.Whitehead@microchip.com> > Cc: davem@davemloft.net; netdev@vger.kernel.org; UNGLinuxDriver > <UNGLinuxDriver@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 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@microchip.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
Hi Florian, see below. > > /* 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. There are a couple issues with the use of phy_read_poll_timeout 1) It requires the use of phy_read, which acquires bus->mdio_lock. But this function is run with the assumption that, that lock is already acquired. There for I presume it will deadlock. 2) The implementation of phy_base_read uses __phy_package_read which uses a shared phy addr, rather than the addr associated with the phydev. These issues could be eliminated if I used read_poll_timeout directly. Does that seem reasonable to you? Regards, Bryan
On 7/24/20 9:29 AM, Bryan.Whitehead@microchip.com wrote: > Hi Florian, see below. > >>> /* 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. > > There are a couple issues with the use of phy_read_poll_timeout > 1) It requires the use of phy_read, which acquires bus->mdio_lock. > But this function is run with the assumption that, that lock is already acquired. > There for I presume it will deadlock> 2) The implementation of phy_base_read uses __phy_package_read which uses a shared phy addr, rather than the addr associated with the phydev. > > These issues could be eliminated if I used read_poll_timeout directly. > Does that seem reasonable to you? Certainly, whatever makes the code more maintainable and makes use of existing functions is better on all counts.
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); + } + + 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; + + /* Set Address to Poke */ + phy_base_write(phydev, 18, 0xd7d5); + ret = vsc8574_wait_for_micro_complete(phydev); + if (ret) + goto done; + + /* Poke to reset PLL Start up State Machine, + * set disable_fsm:bit 119 + */ + phy_base_write(phydev, 18, 0x8d06); + ret = vsc8574_wait_for_micro_complete(phydev); + if (ret) + goto done; + + /* Rewrite PLL config vector */ + phy_base_write(phydev, 18, 0x80c0); + ret = vsc8574_wait_for_micro_complete(phydev); + if (ret) + goto done; + + usleep_range(10000, 20000); + + /* Poke to deassert Reset of PLL State Machine, + * clear disable_fsm:bit 119 + */ + phy_base_write(phydev, 18, 0x8506); + ret = vsc8574_wait_for_micro_complete(phydev); + if (ret) + goto done; + + /* Rewrite PLL config vector */ + phy_base_write(phydev, 18, 0x80c0); + ret = vsc8574_wait_for_micro_complete(phydev); + if (ret) + goto done; + + usleep_range(10000, 20000); + + phy_base_write(phydev, MSCC_EXT_PAGE_ACCESS, + MSCC_PHY_PAGE_EXTENDED_3); + reg_val = phy_base_read(phydev, 20); + reg_val = 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 +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; + } + 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. Signed-off-by: Bryan Whitehead <Bryan.Whitehead@microchip.com> --- drivers/net/phy/mscc/mscc_main.c | 90 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 90 insertions(+)