Re: [PATCH] net: phy: micrel: reconfigure the phy on resume

[Heiner Kallweit <hkallweit1@gmail.com>]

On 13.01.2021 13:36, Claudiu.Beznea@microchip.com wrote:
> 
> 
> On 13.01.2021 13:09, Heiner Kallweit wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> On 13.01.2021 10:29, Claudiu.Beznea@microchip.com wrote:
>>> Hi Heiner,
>>>
>>> On 08.01.2021 18:31, Heiner Kallweit wrote:
>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>
>>>> On 08.01.2021 16:45, Claudiu Beznea wrote:
>>>>> KSZ9131 is used in setups with SAMA7G5. SAMA7G5 supports a special
>>>>> power saving mode (backup mode) that cuts the power for almost all
>>>>> parts of the SoC. The rail powering the ethernet PHY is also cut off.
>>>>> When resuming, in case the PHY has been configured on probe with
>>>>> slew rate or DLL settings these needs to be restored thus call
>>>>> driver's config_init() on resume.
>>>>>
>>>> When would the SoC enter this backup mode?
>>>
>>> It could enter in this mode based on request for standby or suspend-to-mem:
>>> echo mem > /sys/power/state
>>> echo standby > /sys/power/state
>>>
>>> What I didn't mentioned previously is that the RAM remains in self-refresh
>>> while the rest of the SoC is powered down.
>>>
>>
>> This leaves the question which driver sets backup mode in the SoC.
> 
> From Linux point of view the backup mode is a standard suspend-to-mem PM
> mode. The only difference is in SoC specific PM code
> (arch/arm/mach-at91/pm_suspend.S) where the SoC shutdown command is
> executed at the end and the fact that we save the address in RAM of
> cpu_resume() function in a powered memory. Then, the resume is done with
> the help of bootloader (it configures necessary clocks) and jump the
> execution to the previously saved address, resuming Linux.
> 
>> Whatever/whoever wakes the SoC later would have to take care that basically
>> everything that was switched off is reconfigured (incl. calling phy_init_hw()).
> 
> For this the bootloader should know the PHY settings passed via DT (skew
> settings or DLL settings). The bootloader runs from a little SRAM which, at
> the moment doesn't know to parse DT bindings and the DT parsing lib might
> be big enough that the final bootloader size will cross the SRAM size.
> 
>> So it' more or less the same as waking up from hibernation. Therefore I think
>> the .restore of all subsystems would have to be executed, incl. .restore of
>> the MDIO bus.
> 
> I see your point. I think it has been implemented like a standard
> suspend-to-mem PM mode because the RAM remains in self-refresh whereas in
> hibernation it is shut of (as far as I know).
> 
>> Having said that I don't think that change belongs into the
>> PHY driver.
>> Just imagine tomorrow another PHY type is used in a SAMA7G5 setup.
>> Then you would have to do same change in another PHY driver.
> 
> I understand this. At the moment the PM code for drivers in SAMA7G5 are
> saving/restoring in/from RAM the registers content in suspend/resume()
> functions of each drivers and I think it has been chosen like this as the
> RAM remains in self-refresh. Mapping this mode to hibernation will involve
> saving the content of RAM to a non-volatile support which is not wanted as
> this increases the suspend/resume time and it wasn't intended.
> 
>>
>>
>>>> And would it suspend the
>>>> MDIO bus before cutting power to the PHY?
>>>
>>> SAMA7G5 embeds Cadence macb driver which has a integrated MDIO bus. Inside
>>> macb driver the bus is registered with of_mdiobus_register() or
>>> mdiobus_register() based on the PHY devices present in DT or not. On macb
>>> suspend()/resume() functions there are calls to
>>> phylink_stop()/phylink_start() before cutting/after enabling the power to
>>> the PHY.
>>>
>>>> I'm asking because in mdio_bus_phy_restore() we call phy_init_hw()
>>>> already (that calls the driver's config_init).
>>>
>>> As far as I can see from documentation the .restore API of dev_pm_ops is
>>> hibernation specific (please correct me if I'm wrong). On transitions to
>>> backup mode the suspend()/resume() PM APIs are called on the drivers.
>>>

I'm not a Linux PM expert, to me it seems your use case is somewhere in the
middle between s2r and hibernation. I *think* the assumption with s2r is
that one component shouldn't simply cut the power to another component,
and the kernel has no idea about it.

My personal point of view:
If a driver cuts power to another component in s2r, it should take care that
this component is properly re-initialized once power is back.
Otherwise I would miss to see why we need different callbacks resume and restore.

It may be worth to involve the following people/list:

HIBERNATION (aka Software Suspend, aka swsusp)
M:	"Rafael J. Wysocki" <rjw@rjwysocki.net>
M:	Pavel Machek <pavel@ucw.cz>
L:	linux-pm@vger.kernel.org


>>> Thank you,
>>> Claudiu Beznea
>>>
>>>>
>>>>> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
>>>>> ---
>>>>>  drivers/net/phy/micrel.c | 2 +-
>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
>>>>> index 3fe552675dd2..52d3a0480158 100644
>>>>> --- a/drivers/net/phy/micrel.c
>>>>> +++ b/drivers/net/phy/micrel.c
>>>>> @@ -1077,7 +1077,7 @@ static int kszphy_resume(struct phy_device *phydev)
>>>>>        */
>>>>>       usleep_range(1000, 2000);
>>>>>
>>>>> -     ret = kszphy_config_reset(phydev);
>>>>> +     ret = phydev->drv->config_init(phydev);
>>>>>       if (ret)
>>>>>               return ret;
>>>>>
>>>>>