[PATCH] net: phy: broadcom: Wire suspend/resume for BCM54612E

[PATCH] net: phy: broadcom: Wire suspend/resume for BCM54612E

[Marco von Rosenberg]

On some devices, the bootloader suspends the PHY before booting the OS.
Not having a resume callback wired up is a problem in such situations
since it is then never resumed.
This behavior was observed with a Huawei enterprise WLAN access point.

The BCM54612E ethernet PHY supports IDDQ-SR.
Therefore wire-up the suspend and resume callbacks
to point to bcm54xx_suspend() and bcm54xx_resume().

The same wire-up has been done in commit 38b6a9073007 ("net: phy:
broadcom: Wire suspend/resume for BCM50610 and BCM50610M") for two PHYs
also supporting IDDQ-SR.

Signed-off-by: Marco von Rosenberg <marcovr@selfnet.de>
---
 drivers/net/phy/broadcom.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
index 04b2e6eeb195..ac14f223649b 100644
--- a/drivers/net/phy/broadcom.c
+++ b/drivers/net/phy/broadcom.c
@@ -1060,6 +1060,8 @@ static struct phy_driver broadcom_drivers[] = {
 	.handle_interrupt = bcm_phy_handle_interrupt,
 	.link_change_notify	= bcm54xx_link_change_notify,
 	.led_brightness_set	= bcm_phy_led_brightness_set,
+	.suspend	= bcm54xx_suspend,
+	.resume		= bcm54xx_resume,
 }, {
 	.phy_id		= PHY_ID_BCM54616S,
 	.phy_id_mask	= 0xfffffff0,
-- 
2.42.0

Re: [PATCH] net: phy: broadcom: Wire suspend/resume for BCM54612E

[Andrew Lunn]

On Mon, Oct 30, 2023 at 11:54:45PM +0100, Marco von Rosenberg wrote:
> On some devices, the bootloader suspends the PHY before booting the OS.
> Not having a resume callback wired up is a problem in such situations
> since it is then never resumed.

Hi Marco

This description seems odd to me. I'm guessing here:

Are we talking about a device which as been suspended? The PHY has
been left running because there is no suspend callback? Something then
triggers a resume. The bootloader then suspends the active PHY? Linux
then boots, detects its a resume, so does not touch the hardware
because there is no resume callback? The suspended PHY is then
useless.

Adding suspend/resume calls makes sense, i just don't follow the
commit message reasoning.

	Andrew

Re: [PATCH] net: phy: broadcom: Wire suspend/resume for BCM54612E

[Marco von Rosenberg]

On Tuesday, October 31, 2023 1:31:11 AM CET Andrew Lunn wrote:
> Are we talking about a device which as been suspended? The PHY has
> been left running because there is no suspend callback? Something then
> triggers a resume. The bootloader then suspends the active PHY? Linux
> then boots, detects its a resume, so does not touch the hardware
> because there is no resume callback? The suspended PHY is then
> useless.

Hi Andrew,

thanks for your feedback. I guess a bit of context is missing here. The issue 
has nothing to do with an ordinary suspension of the OS. The main point is 
that on initial power-up, the bootloader suspends the PHY before booting 
Linux. With a resume callback defined, Linux would call it on boot and make the 
PHY usable. However, since there is no resume callback defined for this PHY, 
Linux doesn't touch the hardware and thus the PHY is not usable.
So this specific issue is primarily solved by adding the resume callback. The 
suspend callback is just added for completeness.

Does this clarify the issue? If so, I'll adjust the commit message and submit
an updated patch.

	Marco

Re: [PATCH] net: phy: broadcom: Wire suspend/resume for BCM54612E

[Andrew Lunn]

On Wed, Nov 01, 2023 at 10:42:52PM +0100, Marco von Rosenberg wrote:
> On Tuesday, October 31, 2023 1:31:11 AM CET Andrew Lunn wrote:
> > Are we talking about a device which as been suspended? The PHY has
> > been left running because there is no suspend callback? Something then
> > triggers a resume. The bootloader then suspends the active PHY? Linux
> > then boots, detects its a resume, so does not touch the hardware
> > because there is no resume callback? The suspended PHY is then
> > useless.
> 
> Hi Andrew,
> 
> thanks for your feedback. I guess a bit of context is missing here. The issue 
> has nothing to do with an ordinary suspension of the OS. The main point is 
> that on initial power-up, the bootloader suspends the PHY before booting 
> Linux. With a resume callback defined, Linux would call it on boot and make the 
> PHY usable.

Ah, so you rely on phy_attach_direct() calling phy_resume(phydev).

This seems an odd way to solve the problem. It was not Linux which
suspend the PHY, so using resume is asymmetric.

I think soft_reset() or config_init() should be taking the PHY out of
suspend.

	Andrew

Re: [PATCH] net: phy: broadcom: Wire suspend/resume for BCM54612E

[Marco von Rosenberg]

On Wednesday, November 1, 2023 11:06:56 PM CET Andrew Lunn wrote:
> On Wed, Nov 01, 2023 at 10:42:52PM +0100, Marco von Rosenberg wrote:
> > On Tuesday, October 31, 2023 1:31:11 AM CET Andrew Lunn wrote:
> > > Are we talking about a device which as been suspended? The PHY has
> > > been left running because there is no suspend callback? Something then
> > > triggers a resume. The bootloader then suspends the active PHY? Linux
> > > then boots, detects its a resume, so does not touch the hardware
> > > because there is no resume callback? The suspended PHY is then
> > > useless.
> > 
> > Hi Andrew,
> > 
> > thanks for your feedback. I guess a bit of context is missing here. The
> > issue has nothing to do with an ordinary suspension of the OS. The main
> > point is that on initial power-up, the bootloader suspends the PHY before
> > booting Linux. With a resume callback defined, Linux would call it on
> > boot and make the PHY usable.
> 
> Ah, so you rely on phy_attach_direct() calling phy_resume(phydev).
> 
> This seems an odd way to solve the problem. It was not Linux which
> suspend the PHY, so using resume is asymmetric.
> 
> I think soft_reset() or config_init() should be taking the PHY out of
> suspend.

I agree with all of your points. This is just one way which happens to solve
this specific problem. Of course it might be asymmetric to see the patch as
a solution to my problem. However is there anything fundamentally wrong with
adding suspend/resume callbacks? I see some other drivers having these
callbacks defined and some not (it seems a bit inconsistent throughout the
drivers in broadcom.c to be honest).

I'm wondering if I should just omit this whole "motivation" paragraph in the 
commit message and just use the commit message of commit 38b6a9073007 ("net: 
phy: broadcom: Wire suspend/resume for BCM50610 and BCM50610M") as a template.
I mean, regardless of my motivation, I would say it makes sense for this PHY 
to support suspend and resume.

	Marco

Re: [PATCH] net: phy: broadcom: Wire suspend/resume for BCM54612E

[Florian Fainelli]

On 11/2/2023 6:47 PM, Marco von Rosenberg wrote:
> On Wednesday, November 1, 2023 11:06:56 PM CET Andrew Lunn wrote:
>> On Wed, Nov 01, 2023 at 10:42:52PM +0100, Marco von Rosenberg wrote:
>>> On Tuesday, October 31, 2023 1:31:11 AM CET Andrew Lunn wrote:
>>>> Are we talking about a device which as been suspended? The PHY has
>>>> been left running because there is no suspend callback? Something then
>>>> triggers a resume. The bootloader then suspends the active PHY? Linux
>>>> then boots, detects its a resume, so does not touch the hardware
>>>> because there is no resume callback? The suspended PHY is then
>>>> useless.
>>>
>>> Hi Andrew,
>>>
>>> thanks for your feedback. I guess a bit of context is missing here. The
>>> issue has nothing to do with an ordinary suspension of the OS. The main
>>> point is that on initial power-up, the bootloader suspends the PHY before
>>> booting Linux. With a resume callback defined, Linux would call it on
>>> boot and make the PHY usable.
>>
>> Ah, so you rely on phy_attach_direct() calling phy_resume(phydev).
>>
>> This seems an odd way to solve the problem. It was not Linux which
>> suspend the PHY, so using resume is asymmetric.
>>
>> I think soft_reset() or config_init() should be taking the PHY out of
>> suspend.

We have an unconditional call to __phy_resume() in phy_start() and we 
should always have a call to phy_start() regardless of the path though 
you have a point Andrew that we should ensure that by the time 
phy_init_hw() is called we have taken the device out of IDDQ-SR.

> 
> I agree with all of your points. This is just one way which happens to solve
> this specific problem. Of course it might be asymmetric to see the patch as
> a solution to my problem. However is there anything fundamentally wrong with
> adding suspend/resume callbacks? I see some other drivers having these
> callbacks defined and some not (it seems a bit inconsistent throughout the
> drivers in broadcom.c to be honest).
> 
> I'm wondering if I should just omit this whole "motivation" paragraph in the
> commit message and just use the commit message of commit 38b6a9073007 ("net:
> phy: broadcom: Wire suspend/resume for BCM50610 and BCM50610M") as a template.
> I mean, regardless of my motivation, I would say it makes sense for this PHY
> to support suspend and resume.

I would remove the motivation aspect from the paragraph and we could 
also improve the driver a bit to ensure that IDDQ-SR is disabled upon 
config_init(). Other than that your patch is just fine with me. Can you 
re-submit in a few days when net-next opens again?

Thanks!
-- 
Florian

Re: [PATCH] net: phy: broadcom: Wire suspend/resume for BCM54612E

[Marco von Rosenberg]

On Friday, November 3, 2023 4:39:55 AM CET Florian Fainelli wrote:
> We have an unconditional call to __phy_resume() in phy_start() and we
> should always have a call to phy_start() regardless of the path though
> you have a point Andrew that we should ensure that by the time
> phy_init_hw() is called we have taken the device out of IDDQ-SR.
> 
> > I agree with all of your points. This is just one way which happens to
> > solve this specific problem. Of course it might be asymmetric to see the
> > patch as a solution to my problem. However is there anything
> > fundamentally wrong with adding suspend/resume callbacks? I see some
> > other drivers having these callbacks defined and some not (it seems a bit
> > inconsistent throughout the drivers in broadcom.c to be honest).
> > 
> > I'm wondering if I should just omit this whole "motivation" paragraph in
> > the commit message and just use the commit message of commit 38b6a9073007
> > ("net: phy: broadcom: Wire suspend/resume for BCM50610 and BCM50610M") as
> > a template. I mean, regardless of my motivation, I would say it makes
> > sense for this PHY to support suspend and resume.
> 
> I would remove the motivation aspect from the paragraph and we could
> also improve the driver a bit to ensure that IDDQ-SR is disabled upon
> config_init(). Other than that your patch is just fine with me. Can you
> re-submit in a few days when net-next opens again?

Ok, I'll re-submit the patch when net-next is open again with an updated 
commit message. And I agree, disabling IDDQ-SR in config_init() would make 
sense for a future patch since this would fix this potential issue also for 
other PHYs.

	Marco

Re: [PATCH] net: phy: broadcom: Wire suspend/resume for BCM54612E

[Andrew Lunn]

On Fri, Nov 03, 2023 at 02:47:38AM +0100, Marco von Rosenberg wrote:
> On Wednesday, November 1, 2023 11:06:56 PM CET Andrew Lunn wrote:
> > On Wed, Nov 01, 2023 at 10:42:52PM +0100, Marco von Rosenberg wrote:
> > > On Tuesday, October 31, 2023 1:31:11 AM CET Andrew Lunn wrote:
> > > > Are we talking about a device which as been suspended? The PHY has
> > > > been left running because there is no suspend callback? Something then
> > > > triggers a resume. The bootloader then suspends the active PHY? Linux
> > > > then boots, detects its a resume, so does not touch the hardware
> > > > because there is no resume callback? The suspended PHY is then
> > > > useless.
> > > 
> > > Hi Andrew,
> > > 
> > > thanks for your feedback. I guess a bit of context is missing here. The
> > > issue has nothing to do with an ordinary suspension of the OS. The main
> > > point is that on initial power-up, the bootloader suspends the PHY before
> > > booting Linux. With a resume callback defined, Linux would call it on
> > > boot and make the PHY usable.
> > 
> > Ah, so you rely on phy_attach_direct() calling phy_resume(phydev).
> > 
> > This seems an odd way to solve the problem. It was not Linux which
> > suspend the PHY, so using resume is asymmetric.
> > 
> > I think soft_reset() or config_init() should be taking the PHY out of
> > suspend.
> 
> I agree with all of your points. This is just one way which happens to solve
> this specific problem. Of course it might be asymmetric to see the patch as
> a solution to my problem. However is there anything fundamentally wrong with
> adding suspend/resume callbacks?

No, there is nothing wrong with that at all, if you want to support
suspend/resume. I do however see that as a different use case to what
you describe as your problem. It fixing your problem is more of a side
effect.

We can go with this fix, but please change your justification in the
commit message. Also, its unlikely, but resume could be made
conditional in phy_attach_direct(), and you would then be back to a
broken PHY on boot. Fixing this in config_init() is the correct way
for your use case.

       Andrew