* [PATCH net 1/3] net: phy: broadcom: Add power down exit reset state delay
2021-03-10 20:41 [PATCH net 0/3] net: phy: broadcom: Suspend fixes Florian Fainelli
@ 2021-03-10 20:41 ` Florian Fainelli
2021-03-10 22:36 ` kernel test robot
2021-03-10 23:20 ` kernel test robot
2021-03-10 20:41 ` [PATCH net 2/3] net: phy: broadcom: Only set BMCR.PDOWN to suspend Florian Fainelli
2021-03-10 20:41 ` [PATCH net 3/3] net: phy: broadcom: Use corrected suspend for BCM54811 Florian Fainelli
2 siblings, 2 replies; 13+ messages in thread
From: Florian Fainelli @ 2021-03-10 20:41 UTC (permalink / raw)
To: netdev; +Cc: Florian Fainelli, andrew, hkallweit1, kuba, davem
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
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH net 1/3] net: phy: broadcom: Add power down exit reset state delay
2021-03-10 20:41 ` [PATCH net 1/3] net: phy: broadcom: Add power down exit reset state delay Florian Fainelli
@ 2021-03-10 22:36 ` kernel test robot
2021-03-10 23:20 ` kernel test robot
1 sibling, 0 replies; 13+ messages in thread
From: kernel test robot @ 2021-03-10 22:36 UTC (permalink / raw)
To: Florian Fainelli, netdev
Cc: kbuild-all, clang-built-linux, Florian Fainelli, andrew,
hkallweit1, kuba, davem
[-- 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 --]
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH net 1/3] net: phy: broadcom: Add power down exit reset state delay
2021-03-10 20:41 ` [PATCH net 1/3] net: phy: broadcom: Add power down exit reset state delay Florian Fainelli
@ 2021-03-10 23:20 ` kernel test robot
2021-03-10 23:20 ` kernel test robot
1 sibling, 0 replies; 13+ messages in thread
From: kernel test robot @ 2021-03-10 23:20 UTC (permalink / raw)
To: Florian Fainelli, netdev
Cc: kbuild-all, Florian Fainelli, andrew, hkallweit1, kuba, davem
[-- 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 --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH net 2/3] net: phy: broadcom: Only set BMCR.PDOWN to suspend
2021-03-10 20:41 [PATCH net 0/3] net: phy: broadcom: Suspend fixes Florian Fainelli
2021-03-10 20:41 ` [PATCH net 1/3] net: phy: broadcom: Add power down exit reset state delay Florian Fainelli
@ 2021-03-10 20:41 ` Florian Fainelli
2021-03-10 21:07 ` Heiner Kallweit
2021-03-10 20:41 ` [PATCH net 3/3] net: phy: broadcom: Use corrected suspend for BCM54811 Florian Fainelli
2 siblings, 1 reply; 13+ messages in thread
From: Florian Fainelli @ 2021-03-10 20:41 UTC (permalink / raw)
To: netdev; +Cc: Florian Fainelli, andrew, hkallweit1, kuba, davem
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
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH net 2/3] net: phy: broadcom: Only set BMCR.PDOWN to suspend
2021-03-10 20:41 ` [PATCH net 2/3] net: phy: broadcom: Only set BMCR.PDOWN to suspend Florian Fainelli
@ 2021-03-10 21:07 ` Heiner Kallweit
2021-03-10 21:15 ` Florian Fainelli
0 siblings, 1 reply; 13+ messages in thread
From: Heiner Kallweit @ 2021-03-10 21:07 UTC (permalink / raw)
To: Florian Fainelli, netdev; +Cc: andrew, kuba, davem
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,
>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH net 2/3] net: phy: broadcom: Only set BMCR.PDOWN to suspend
2021-03-10 21:07 ` Heiner Kallweit
@ 2021-03-10 21:15 ` Florian Fainelli
2021-03-10 21:43 ` Heiner Kallweit
0 siblings, 1 reply; 13+ messages in thread
From: Florian Fainelli @ 2021-03-10 21:15 UTC (permalink / raw)
To: Heiner Kallweit, netdev; +Cc: andrew, kuba, davem
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
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH net 2/3] net: phy: broadcom: Only set BMCR.PDOWN to suspend
2021-03-10 21:15 ` Florian Fainelli
@ 2021-03-10 21:43 ` Heiner Kallweit
2021-03-10 22:56 ` Florian Fainelli
0 siblings, 1 reply; 13+ messages in thread
From: Heiner Kallweit @ 2021-03-10 21:43 UTC (permalink / raw)
To: Florian Fainelli, netdev; +Cc: andrew, kuba, davem
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.
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH net 2/3] net: phy: broadcom: Only set BMCR.PDOWN to suspend
2021-03-10 21:43 ` Heiner Kallweit
@ 2021-03-10 22:56 ` Florian Fainelli
2021-03-11 3:31 ` Florian Fainelli
0 siblings, 1 reply; 13+ messages in thread
From: Florian Fainelli @ 2021-03-10 22:56 UTC (permalink / raw)
To: Heiner Kallweit, netdev; +Cc: andrew, kuba, davem
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
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH net 2/3] net: phy: broadcom: Only set BMCR.PDOWN to suspend
2021-03-10 22:56 ` Florian Fainelli
@ 2021-03-11 3:31 ` Florian Fainelli
0 siblings, 0 replies; 13+ messages in thread
From: Florian Fainelli @ 2021-03-11 3:31 UTC (permalink / raw)
To: Heiner Kallweit, netdev; +Cc: andrew, kuba, davem
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH net 3/3] net: phy: broadcom: Use corrected suspend for BCM54811
2021-03-10 20:41 [PATCH net 0/3] net: phy: broadcom: Suspend fixes Florian Fainelli
2021-03-10 20:41 ` [PATCH net 1/3] net: phy: broadcom: Add power down exit reset state delay Florian Fainelli
2021-03-10 20:41 ` [PATCH net 2/3] net: phy: broadcom: Only set BMCR.PDOWN to suspend Florian Fainelli
@ 2021-03-10 20:41 ` Florian Fainelli
2 siblings, 0 replies; 13+ messages in thread
From: Florian Fainelli @ 2021-03-10 20:41 UTC (permalink / raw)
To: netdev; +Cc: Florian Fainelli, andrew, hkallweit1, kuba, davem
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
^ permalink raw reply related [flat|nested] 13+ messages in thread