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