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

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

[Bryan Whitehead]

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;
+		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 */
-- 
2.7.4

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

[Florian Fainelli]

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

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

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3316 bytes --]

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 61171 bytes --]

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

[Andrew Lunn]

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

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

[Bryan.Whitehead]

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

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

[Bryan.Whitehead]

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

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

[Bryan.Whitehead]

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

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

[Florian Fainelli]

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.
-- 
Florian