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(+)