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

[Florian Fainelli <f.fainelli@gmail.com>]

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