[PATCH net 0/3] net: phy: broadcom: Suspend fixes

[PATCH net 0/3] net: phy: broadcom: Suspend fixes

[Florian Fainelli]

Hi all,

This patch series fixes the suspend entry for BCM54810 and BCM54811 PHYs
which can be put into an incorrect state depending on the timing of the
BMCR.PDOWN write and essentially not achieve the desired power savings.

In addition, there is a correctness change added that waits 40us upon
clearing the BMCR.PDOWN bit per the datasheet information.

The BCM5464 PHY entry is not changed since I do no have hardware to test
this on, however it could be switched to the non read/modify/write
version of the suspend routine if necessary.

Florian Fainelli (3):
  net: phy: broadcom: Add power down exit reset state delay
  net: phy: broadcom: Only set BMCR.PDOWN to suspend
  net: phy: broadcom: Use corrected suspend for BCM54811

 drivers/net/phy/broadcom.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

-- 
2.25.1

[PATCH net 1/3] net: phy: broadcom: Add power down exit reset state delay

[Florian Fainelli]

Per the datasheet, when we clear the power down bit, the PHY remains in
an internal reset state for 40us and then resume normal operation.
Account for that delay to avoid any issues in the future if
genphy_resume() changes.

Fixes: fe26821fa614 ("net: phy: broadcom: Wire suspend/resume for BCM54810")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/phy/broadcom.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
index fa0be591ae79..b8eb736fb456 100644
--- a/drivers/net/phy/broadcom.c
+++ b/drivers/net/phy/broadcom.c
@@ -399,6 +399,11 @@ static int bcm54xx_resume(struct phy_device *phydev)
 	if (ret < 0)
 		return ret;
 
+	/* Upon exiting power down, the PHY remains in an internal reset state
+	 * for 40us
+	 */
+	usleep(40);
+
 	return bcm54xx_config_init(phydev);
 }
 
-- 
2.25.1

[PATCH net 2/3] net: phy: broadcom: Only set BMCR.PDOWN to suspend

[Florian Fainelli]

B50212E PHYs have been observed to get into an incorrect state with the
visible effect of having both activity and link LEDs flashing
alternatively instead of being turned off as intended when
genphy_suspend() was issued. The BCM54810 is a similar design and
equally suffers from that issue.

The datasheet is not particularly clear whether a read/modify/write
sequence is acceptable and only indicates that BMCR.PDOWN=1 should be
utilized to enter the power down mode. When this was done the PHYs were
always measured to have power levels that match the expectations and
LEDs powered off.

Fixes: fe26821fa614 ("net: phy: broadcom: Wire suspend/resume for BCM54810")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/phy/broadcom.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
index b8eb736fb456..b33ffd44f799 100644
--- a/drivers/net/phy/broadcom.c
+++ b/drivers/net/phy/broadcom.c
@@ -388,6 +388,21 @@ static int bcm54xx_config_init(struct phy_device *phydev)
 	return 0;
 }
 
+static int bcm54xx_suspend(struct phy_device *phydev)
+{
+	/* We cannot perform a read/modify/write like what genphy_suspend()
+	 * does because depending on the time we can observe the PHY having
+	 * both of its LEDs flashing indicating that it is in an incorrect
+	 * state and not powered down as expected.
+	 *
+	 * There is not a clear indication in the datasheet whether a
+	 * read/modify/write would be acceptable, but a blind write to the
+	 * register has been proven to be functional unlike the
+	 * Read/Modify/Write.
+	 */
+	return phy_write(phydev, MII_BMCR, BMCR_PDOWN);
+}
+
 static int bcm54xx_resume(struct phy_device *phydev)
 {
 	int ret;
@@ -778,7 +793,7 @@ static struct phy_driver broadcom_drivers[] = {
 	.config_aneg    = bcm5481_config_aneg,
 	.config_intr    = bcm_phy_config_intr,
 	.handle_interrupt = bcm_phy_handle_interrupt,
-	.suspend	= genphy_suspend,
+	.suspend	= bcm54xx_suspend,
 	.resume		= bcm54xx_resume,
 }, {
 	.phy_id         = PHY_ID_BCM54811,
-- 
2.25.1

[PATCH net 3/3] net: phy: broadcom: Use corrected suspend for BCM54811

[Florian Fainelli]

Use the non read/modify/write version of the suspend procedure for the
BCM54811 PHY.

Fixes: b0ed0bbfb304 ("net: phy: broadcom: add support for BCM54811 PHY")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/phy/broadcom.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
index b33ffd44f799..14980ef44b42 100644
--- a/drivers/net/phy/broadcom.c
+++ b/drivers/net/phy/broadcom.c
@@ -804,7 +804,7 @@ static struct phy_driver broadcom_drivers[] = {
 	.config_aneg    = bcm5481_config_aneg,
 	.config_intr    = bcm_phy_config_intr,
 	.handle_interrupt = bcm_phy_handle_interrupt,
-	.suspend	= genphy_suspend,
+	.suspend	= bcm54xx_suspend,
 	.resume		= bcm54xx_resume,
 }, {
 	.phy_id		= PHY_ID_BCM5482,
-- 
2.25.1

Re: [PATCH net 2/3] net: phy: broadcom: Only set BMCR.PDOWN to suspend

[Heiner Kallweit]

On 10.03.2021 21:41, Florian Fainelli wrote:
> B50212E PHYs have been observed to get into an incorrect state with the
> visible effect of having both activity and link LEDs flashing
> alternatively instead of being turned off as intended when
> genphy_suspend() was issued. The BCM54810 is a similar design and
> equally suffers from that issue.
> 
> The datasheet is not particularly clear whether a read/modify/write
> sequence is acceptable and only indicates that BMCR.PDOWN=1 should be
> utilized to enter the power down mode. When this was done the PHYs were
> always measured to have power levels that match the expectations and
> LEDs powered off.
> 
> Fixes: fe26821fa614 ("net: phy: broadcom: Wire suspend/resume for BCM54810")
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  drivers/net/phy/broadcom.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
> index b8eb736fb456..b33ffd44f799 100644
> --- a/drivers/net/phy/broadcom.c
> +++ b/drivers/net/phy/broadcom.c
> @@ -388,6 +388,21 @@ static int bcm54xx_config_init(struct phy_device *phydev)
>  	return 0;
>  }
>  
> +static int bcm54xx_suspend(struct phy_device *phydev)
> +{
> +	/* We cannot perform a read/modify/write like what genphy_suspend()
> +	 * does because depending on the time we can observe the PHY having
> +	 * both of its LEDs flashing indicating that it is in an incorrect
> +	 * state and not powered down as expected.
> +	 *
> +	 * There is not a clear indication in the datasheet whether a
> +	 * read/modify/write would be acceptable, but a blind write to the
> +	 * register has been proven to be functional unlike the
> +	 * Read/Modify/Write.
> +	 */
> +	return phy_write(phydev, MII_BMCR, BMCR_PDOWN);

This clears all other bits in MII_BMCR, incl. ANENABLE and the ones used in
forced mode. So you have to rely on somebody calling genphy_config_aneg()
to sync the register bits with the values cached in struct phy_device
on resume. Typically the phylib state machine takes care, but do we have
to consider use cases where this is not the case?

Heiner

> +}
> +
>  static int bcm54xx_resume(struct phy_device *phydev)
>  {
>  	int ret;
> @@ -778,7 +793,7 @@ static struct phy_driver broadcom_drivers[] = {
>  	.config_aneg    = bcm5481_config_aneg,
>  	.config_intr    = bcm_phy_config_intr,
>  	.handle_interrupt = bcm_phy_handle_interrupt,
> -	.suspend	= genphy_suspend,
> +	.suspend	= bcm54xx_suspend,
>  	.resume		= bcm54xx_resume,
>  }, {
>  	.phy_id         = PHY_ID_BCM54811,
> 

Re: [PATCH net 2/3] net: phy: broadcom: Only set BMCR.PDOWN to suspend

[Florian Fainelli]

On 3/10/21 1:07 PM, Heiner Kallweit wrote:
> On 10.03.2021 21:41, Florian Fainelli wrote:
>> B50212E PHYs have been observed to get into an incorrect state with the
>> visible effect of having both activity and link LEDs flashing
>> alternatively instead of being turned off as intended when
>> genphy_suspend() was issued. The BCM54810 is a similar design and
>> equally suffers from that issue.
>>
>> The datasheet is not particularly clear whether a read/modify/write
>> sequence is acceptable and only indicates that BMCR.PDOWN=1 should be
>> utilized to enter the power down mode. When this was done the PHYs were
>> always measured to have power levels that match the expectations and
>> LEDs powered off.
>>
>> Fixes: fe26821fa614 ("net: phy: broadcom: Wire suspend/resume for BCM54810")
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>> ---
>>  drivers/net/phy/broadcom.c | 17 ++++++++++++++++-
>>  1 file changed, 16 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
>> index b8eb736fb456..b33ffd44f799 100644
>> --- a/drivers/net/phy/broadcom.c
>> +++ b/drivers/net/phy/broadcom.c
>> @@ -388,6 +388,21 @@ static int bcm54xx_config_init(struct phy_device *phydev)
>>  	return 0;
>>  }
>>  
>> +static int bcm54xx_suspend(struct phy_device *phydev)
>> +{
>> +	/* We cannot perform a read/modify/write like what genphy_suspend()
>> +	 * does because depending on the time we can observe the PHY having
>> +	 * both of its LEDs flashing indicating that it is in an incorrect
>> +	 * state and not powered down as expected.
>> +	 *
>> +	 * There is not a clear indication in the datasheet whether a
>> +	 * read/modify/write would be acceptable, but a blind write to the
>> +	 * register has been proven to be functional unlike the
>> +	 * Read/Modify/Write.
>> +	 */
>> +	return phy_write(phydev, MII_BMCR, BMCR_PDOWN);
> 
> This clears all other bits in MII_BMCR, incl. ANENABLE and the ones used in
> forced mode. So you have to rely on somebody calling genphy_config_aneg()
> to sync the register bits with the values cached in struct phy_device
> on resume. Typically the phylib state machine takes care, but do we have
> to consider use cases where this is not the case?

Good point, how about if we had forced the link before suspending, does
PHYLIB take care of re-applying the same parameters? It arguably should
do that in all cases given that power to the PHY can be cut depending on
the suspend mode.
-- 
Florian

Re: [PATCH net 2/3] net: phy: broadcom: Only set BMCR.PDOWN to suspend

[Heiner Kallweit]

On 10.03.2021 22:15, Florian Fainelli wrote:
> On 3/10/21 1:07 PM, Heiner Kallweit wrote:
>> On 10.03.2021 21:41, Florian Fainelli wrote:
>>> B50212E PHYs have been observed to get into an incorrect state with the
>>> visible effect of having both activity and link LEDs flashing
>>> alternatively instead of being turned off as intended when
>>> genphy_suspend() was issued. The BCM54810 is a similar design and
>>> equally suffers from that issue.
>>>
>>> The datasheet is not particularly clear whether a read/modify/write
>>> sequence is acceptable and only indicates that BMCR.PDOWN=1 should be
>>> utilized to enter the power down mode. When this was done the PHYs were
>>> always measured to have power levels that match the expectations and
>>> LEDs powered off.
>>>
>>> Fixes: fe26821fa614 ("net: phy: broadcom: Wire suspend/resume for BCM54810")
>>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>>> ---
>>>  drivers/net/phy/broadcom.c | 17 ++++++++++++++++-
>>>  1 file changed, 16 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
>>> index b8eb736fb456..b33ffd44f799 100644
>>> --- a/drivers/net/phy/broadcom.c
>>> +++ b/drivers/net/phy/broadcom.c
>>> @@ -388,6 +388,21 @@ static int bcm54xx_config_init(struct phy_device *phydev)
>>>  	return 0;
>>>  }
>>>  
>>> +static int bcm54xx_suspend(struct phy_device *phydev)
>>> +{
>>> +	/* We cannot perform a read/modify/write like what genphy_suspend()
>>> +	 * does because depending on the time we can observe the PHY having
>>> +	 * both of its LEDs flashing indicating that it is in an incorrect
>>> +	 * state and not powered down as expected.
>>> +	 *
>>> +	 * There is not a clear indication in the datasheet whether a
>>> +	 * read/modify/write would be acceptable, but a blind write to the
>>> +	 * register has been proven to be functional unlike the
>>> +	 * Read/Modify/Write.
>>> +	 */
>>> +	return phy_write(phydev, MII_BMCR, BMCR_PDOWN);
>>
>> This clears all other bits in MII_BMCR, incl. ANENABLE and the ones used in
>> forced mode. So you have to rely on somebody calling genphy_config_aneg()
>> to sync the register bits with the values cached in struct phy_device
>> on resume. Typically the phylib state machine takes care, but do we have
>> to consider use cases where this is not the case?
> 
> Good point, how about if we had forced the link before suspending, does
> PHYLIB take care of re-applying the same parameters? It arguably should
> do that in all cases given that power to the PHY can be cut depending on
> the suspend mode.
> 

When entering power-down mode the link is lost and we go to HALTED state.
On resume, phy_start() sets UP state and state machine calls
phy_start_aneg(), which takes care of syncing the BMCR forced mode bits.
A potential issue arises if we have a driver that doesn't use the
phylib state machine and prefers to do it on its own.
IIRC I once stumbled across this when I also relied on the phylib state
machine running in a change.
I'm not sure whether we can run into a problem, but it's worth spending
a thought before somebody complains after applying the change.

Re: [PATCH net 1/3] net: phy: broadcom: Add power down exit reset state delay

[kernel test robot]

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

Hi Florian,

I love your patch! Yet something to improve:

[auto build test ERROR on net/master]

url:    https://github.com/0day-ci/linux/commits/Florian-Fainelli/net-phy-broadcom-Suspend-fixes/20210311-044539
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git 8515455720c52a0841bd1c9c5f457c9616900110
config: arm-randconfig-r023-20210310 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project cd9a69289c7825d54450cb6829fef2c8e0f1963a)
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
        # install arm cross compiling tool for clang build
        # apt-get install binutils-arm-linux-gnueabi
        # https://github.com/0day-ci/linux/commit/06734c5da3238dad0a1ec26dac3d4698724157ac
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Florian-Fainelli/net-phy-broadcom-Suspend-fixes/20210311-044539
        git checkout 06734c5da3238dad0a1ec26dac3d4698724157ac
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=arm 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/net/phy/broadcom.c:405:2: error: implicit declaration of function 'usleep' [-Werror,-Wimplicit-function-declaration]
           usleep(40);
           ^
   1 error generated.


vim +/usleep +405 drivers/net/phy/broadcom.c

   390	
   391	static int bcm54xx_resume(struct phy_device *phydev)
   392	{
   393		int ret;
   394	
   395		/* Writes to register other than BMCR would be ignored
   396		 * unless we clear the PDOWN bit first
   397		 */
   398		ret = genphy_resume(phydev);
   399		if (ret < 0)
   400			return ret;
   401	
   402		/* Upon exiting power down, the PHY remains in an internal reset state
   403		 * for 40us
   404		 */
 > 405		usleep(40);
   406	
   407		return bcm54xx_config_init(phydev);
   408	}
   409	

---
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: 35370 bytes --]

Re: [PATCH net 1/3] net: phy: broadcom: Add power down exit reset state delay

[kernel test robot]

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

Hi Florian,

I love your patch! Yet something to improve:

[auto build test ERROR on net/master]

url:    https://github.com/0day-ci/linux/commits/Florian-Fainelli/net-phy-broadcom-Suspend-fixes/20210311-044539
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git 8515455720c52a0841bd1c9c5f457c9616900110
config: arm-randconfig-r023-20210310 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project cd9a69289c7825d54450cb6829fef2c8e0f1963a)
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
        # install arm cross compiling tool for clang build
        # apt-get install binutils-arm-linux-gnueabi
        # https://github.com/0day-ci/linux/commit/06734c5da3238dad0a1ec26dac3d4698724157ac
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Florian-Fainelli/net-phy-broadcom-Suspend-fixes/20210311-044539
        git checkout 06734c5da3238dad0a1ec26dac3d4698724157ac
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=arm 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/net/phy/broadcom.c:405:2: error: implicit declaration of function 'usleep' [-Werror,-Wimplicit-function-declaration]
           usleep(40);
           ^
   1 error generated.


vim +/usleep +405 drivers/net/phy/broadcom.c

   390	
   391	static int bcm54xx_resume(struct phy_device *phydev)
   392	{
   393		int ret;
   394	
   395		/* Writes to register other than BMCR would be ignored
   396		 * unless we clear the PDOWN bit first
   397		 */
   398		ret = genphy_resume(phydev);
   399		if (ret < 0)
   400			return ret;
   401	
   402		/* Upon exiting power down, the PHY remains in an internal reset state
   403		 * for 40us
   404		 */
 > 405		usleep(40);
   406	
   407		return bcm54xx_config_init(phydev);
   408	}
   409	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

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

Re: [PATCH net 2/3] net: phy: broadcom: Only set BMCR.PDOWN to suspend

[Florian Fainelli]

On 3/10/21 1:43 PM, Heiner Kallweit wrote:
> On 10.03.2021 22:15, Florian Fainelli wrote:
>> On 3/10/21 1:07 PM, Heiner Kallweit wrote:
>>> On 10.03.2021 21:41, Florian Fainelli wrote:
>>>> B50212E PHYs have been observed to get into an incorrect state with the
>>>> visible effect of having both activity and link LEDs flashing
>>>> alternatively instead of being turned off as intended when
>>>> genphy_suspend() was issued. The BCM54810 is a similar design and
>>>> equally suffers from that issue.
>>>>
>>>> The datasheet is not particularly clear whether a read/modify/write
>>>> sequence is acceptable and only indicates that BMCR.PDOWN=1 should be
>>>> utilized to enter the power down mode. When this was done the PHYs were
>>>> always measured to have power levels that match the expectations and
>>>> LEDs powered off.
>>>>
>>>> Fixes: fe26821fa614 ("net: phy: broadcom: Wire suspend/resume for BCM54810")
>>>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>>>> ---
>>>>  drivers/net/phy/broadcom.c | 17 ++++++++++++++++-
>>>>  1 file changed, 16 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
>>>> index b8eb736fb456..b33ffd44f799 100644
>>>> --- a/drivers/net/phy/broadcom.c
>>>> +++ b/drivers/net/phy/broadcom.c
>>>> @@ -388,6 +388,21 @@ static int bcm54xx_config_init(struct phy_device *phydev)
>>>>  	return 0;
>>>>  }
>>>>  
>>>> +static int bcm54xx_suspend(struct phy_device *phydev)
>>>> +{
>>>> +	/* We cannot perform a read/modify/write like what genphy_suspend()
>>>> +	 * does because depending on the time we can observe the PHY having
>>>> +	 * both of its LEDs flashing indicating that it is in an incorrect
>>>> +	 * state and not powered down as expected.
>>>> +	 *
>>>> +	 * There is not a clear indication in the datasheet whether a
>>>> +	 * read/modify/write would be acceptable, but a blind write to the
>>>> +	 * register has been proven to be functional unlike the
>>>> +	 * Read/Modify/Write.
>>>> +	 */
>>>> +	return phy_write(phydev, MII_BMCR, BMCR_PDOWN);
>>>
>>> This clears all other bits in MII_BMCR, incl. ANENABLE and the ones used in
>>> forced mode. So you have to rely on somebody calling genphy_config_aneg()
>>> to sync the register bits with the values cached in struct phy_device
>>> on resume. Typically the phylib state machine takes care, but do we have
>>> to consider use cases where this is not the case?
>>
>> Good point, how about if we had forced the link before suspending, does
>> PHYLIB take care of re-applying the same parameters? It arguably should
>> do that in all cases given that power to the PHY can be cut depending on
>> the suspend mode.
>>
> 
> When entering power-down mode the link is lost and we go to HALTED state.
> On resume, phy_start() sets UP state and state machine calls
> phy_start_aneg(), which takes care of syncing the BMCR forced mode bits.
> A potential issue arises if we have a driver that doesn't use the
> phylib state machine and prefers to do it on its own.
> IIRC I once stumbled across this when I also relied on the phylib state
> machine running in a change.
> I'm not sure whether we can run into a problem, but it's worth spending
> a thought before somebody complains after applying the change.

That is a fair point, I could save the BMCR before modifying it and
restore it in bcm54xx_resume() and phy_start_aneg() should not issue an
additional re-negotiation in that case. Let me explore a bit more to
find out which of these BMCR bits makes the PHY go bonkers.

Thanks
-- 
Florian

Re: [PATCH net 1/3] net: phy: broadcom: Add power down exit reset state delay

[kernel test robot]

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

Hi Florian,

I love your patch! Yet something to improve:

[auto build test ERROR on net/master]

url:    https://github.com/0day-ci/linux/commits/Florian-Fainelli/net-phy-broadcom-Suspend-fixes/20210311-044539
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git 8515455720c52a0841bd1c9c5f457c9616900110
config: nios2-randconfig-r006-20210309 (attached as .config)
compiler: nios2-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
        # https://github.com/0day-ci/linux/commit/06734c5da3238dad0a1ec26dac3d4698724157ac
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Florian-Fainelli/net-phy-broadcom-Suspend-fixes/20210311-044539
        git checkout 06734c5da3238dad0a1ec26dac3d4698724157ac
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=nios2 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/net/phy/broadcom.c: In function 'bcm54xx_resume':
>> drivers/net/phy/broadcom.c:405:2: error: implicit declaration of function 'usleep'; did you mean 'fsleep'? [-Werror=implicit-function-declaration]
     405 |  usleep(40);
         |  ^~~~~~
         |  fsleep
   cc1: some warnings being treated as errors


vim +405 drivers/net/phy/broadcom.c

   390	
   391	static int bcm54xx_resume(struct phy_device *phydev)
   392	{
   393		int ret;
   394	
   395		/* Writes to register other than BMCR would be ignored
   396		 * unless we clear the PDOWN bit first
   397		 */
   398		ret = genphy_resume(phydev);
   399		if (ret < 0)
   400			return ret;
   401	
   402		/* Upon exiting power down, the PHY remains in an internal reset state
   403		 * for 40us
   404		 */
 > 405		usleep(40);
   406	
   407		return bcm54xx_config_init(phydev);
   408	}
   409	

---
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: 16399 bytes --]

Re: [PATCH net 1/3] net: phy: broadcom: Add power down exit reset state delay

[kernel test robot]

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

Hi Florian,

I love your patch! Yet something to improve:

[auto build test ERROR on net/master]

url:    https://github.com/0day-ci/linux/commits/Florian-Fainelli/net-phy-broadcom-Suspend-fixes/20210311-044539
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git 8515455720c52a0841bd1c9c5f457c9616900110
config: nios2-randconfig-r006-20210309 (attached as .config)
compiler: nios2-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
        # https://github.com/0day-ci/linux/commit/06734c5da3238dad0a1ec26dac3d4698724157ac
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Florian-Fainelli/net-phy-broadcom-Suspend-fixes/20210311-044539
        git checkout 06734c5da3238dad0a1ec26dac3d4698724157ac
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=nios2 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/net/phy/broadcom.c: In function 'bcm54xx_resume':
>> drivers/net/phy/broadcom.c:405:2: error: implicit declaration of function 'usleep'; did you mean 'fsleep'? [-Werror=implicit-function-declaration]
     405 |  usleep(40);
         |  ^~~~~~
         |  fsleep
   cc1: some warnings being treated as errors


vim +405 drivers/net/phy/broadcom.c

   390	
   391	static int bcm54xx_resume(struct phy_device *phydev)
   392	{
   393		int ret;
   394	
   395		/* Writes to register other than BMCR would be ignored
   396		 * unless we clear the PDOWN bit first
   397		 */
   398		ret = genphy_resume(phydev);
   399		if (ret < 0)
   400			return ret;
   401	
   402		/* Upon exiting power down, the PHY remains in an internal reset state
   403		 * for 40us
   404		 */
 > 405		usleep(40);
   406	
   407		return bcm54xx_config_init(phydev);
   408	}
   409	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

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

Re: [PATCH net 2/3] net: phy: broadcom: Only set BMCR.PDOWN to suspend

[Florian Fainelli]

On 3/10/21 2:56 PM, Florian Fainelli wrote:
> On 3/10/21 1:43 PM, Heiner Kallweit wrote:
>> On 10.03.2021 22:15, Florian Fainelli wrote:
>>> On 3/10/21 1:07 PM, Heiner Kallweit wrote:
>>>> On 10.03.2021 21:41, Florian Fainelli wrote:
>>>>> B50212E PHYs have been observed to get into an incorrect state with the
>>>>> visible effect of having both activity and link LEDs flashing
>>>>> alternatively instead of being turned off as intended when
>>>>> genphy_suspend() was issued. The BCM54810 is a similar design and
>>>>> equally suffers from that issue.
>>>>>
>>>>> The datasheet is not particularly clear whether a read/modify/write
>>>>> sequence is acceptable and only indicates that BMCR.PDOWN=1 should be
>>>>> utilized to enter the power down mode. When this was done the PHYs were
>>>>> always measured to have power levels that match the expectations and
>>>>> LEDs powered off.
>>>>>
>>>>> Fixes: fe26821fa614 ("net: phy: broadcom: Wire suspend/resume for BCM54810")
>>>>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>>>>> ---
>>>>>  drivers/net/phy/broadcom.c | 17 ++++++++++++++++-
>>>>>  1 file changed, 16 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
>>>>> index b8eb736fb456..b33ffd44f799 100644
>>>>> --- a/drivers/net/phy/broadcom.c
>>>>> +++ b/drivers/net/phy/broadcom.c
>>>>> @@ -388,6 +388,21 @@ static int bcm54xx_config_init(struct phy_device *phydev)
>>>>>  	return 0;
>>>>>  }
>>>>>  
>>>>> +static int bcm54xx_suspend(struct phy_device *phydev)
>>>>> +{
>>>>> +	/* We cannot perform a read/modify/write like what genphy_suspend()
>>>>> +	 * does because depending on the time we can observe the PHY having
>>>>> +	 * both of its LEDs flashing indicating that it is in an incorrect
>>>>> +	 * state and not powered down as expected.
>>>>> +	 *
>>>>> +	 * There is not a clear indication in the datasheet whether a
>>>>> +	 * read/modify/write would be acceptable, but a blind write to the
>>>>> +	 * register has been proven to be functional unlike the
>>>>> +	 * Read/Modify/Write.
>>>>> +	 */
>>>>> +	return phy_write(phydev, MII_BMCR, BMCR_PDOWN);
>>>>
>>>> This clears all other bits in MII_BMCR, incl. ANENABLE and the ones used in
>>>> forced mode. So you have to rely on somebody calling genphy_config_aneg()
>>>> to sync the register bits with the values cached in struct phy_device
>>>> on resume. Typically the phylib state machine takes care, but do we have
>>>> to consider use cases where this is not the case?
>>>
>>> Good point, how about if we had forced the link before suspending, does
>>> PHYLIB take care of re-applying the same parameters? It arguably should
>>> do that in all cases given that power to the PHY can be cut depending on
>>> the suspend mode.
>>>
>>
>> When entering power-down mode the link is lost and we go to HALTED state.
>> On resume, phy_start() sets UP state and state machine calls
>> phy_start_aneg(), which takes care of syncing the BMCR forced mode bits.
>> A potential issue arises if we have a driver that doesn't use the
>> phylib state machine and prefers to do it on its own.
>> IIRC I once stumbled across this when I also relied on the phylib state
>> machine running in a change.
>> I'm not sure whether we can run into a problem, but it's worth spending
>> a thought before somebody complains after applying the change.
> 
> That is a fair point, I could save the BMCR before modifying it and
> restore it in bcm54xx_resume() and phy_start_aneg() should not issue an
> additional re-negotiation in that case. Let me explore a bit more to
> find out which of these BMCR bits makes the PHY go bonkers.

I re-tested with different kernels and was convinced that the
problem I saw due to the lack of locking in the 4.9 kernel around
phydrv->suspend() since 4.9 was where I started working on this. It
turns out this was reproducible with different kernel versions as well
so there is really something that is possibly specific to the BCM54810
PHY and the specific design.

I will run more tests to get to the bottom of this hopefully.
-- 
Florian