net: phy: micrel: reconfigure the phy on resume

Commit Message

Claudiu Beznea Jan. 8, 2021, 3:45 p.m. UTC

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.

Signed-off-by: Claudiu Beznea <[e-mail]>
---
 drivers/net/phy/micrel.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Andrew Lunn Jan. 8, 2021, 4:10 p.m. UTC | #1

On Fri, Jan 08, 2021 at 05:45:54PM +0200, 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.
> 
> Signed-off-by: Claudiu Beznea <[e-mail]>

Reviewed-by: Andrew Lunn <[e-mail]>

    Andrew

Heiner Kallweit Jan. 8, 2021, 4:31 p.m. UTC | #2

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? And would it suspend the
MDIO bus before cutting 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).

> Signed-off-by: Claudiu Beznea <[e-mail]>
> ---
>  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;
>  
>

Claudiu Beznea Jan. 13, 2021, 9:29 a.m. UTC | #3

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.

> 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.

Thank you,
Claudiu Beznea

> 
>> Signed-off-by: Claudiu Beznea <[e-mail]>
>> ---
>>  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;
>>
>>
>

Heiner Kallweit Jan. 13, 2021, 11:09 a.m. UTC | #4

On 13.01.2021 10:29, [e-mail] 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.
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()). 
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. 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.


>> 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.
> 
> Thank you,
> Claudiu Beznea
> 
>>
>>> Signed-off-by: Claudiu Beznea <[e-mail]>
>>> ---
>>>  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;
>>>
>>>

Claudiu Beznea Jan. 13, 2021, 12:36 p.m. UTC | #5

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, [e-mail] 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.
>>
>> Thank you,
>> Claudiu Beznea
>>
>>>
>>>> Signed-off-by: Claudiu Beznea <[e-mail]>
>>>> ---
>>>>  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;
>>>>
>>>>
>

Heiner Kallweit Jan. 13, 2021, 9:34 p.m. UTC | #6

On 13.01.2021 13:36, [e-mail] 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, [e-mail] 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" <[e-mail]>
M:	Pavel Machek <[e-mail]>
L:	[e-mail]


>>> Thank you,
>>> Claudiu Beznea
>>>
>>>>
>>>>> Signed-off-by: Claudiu Beznea <[e-mail]>
>>>>> ---
>>>>>  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;
>>>>>
>>>>>

Russell King (Oracle) Jan. 13, 2021, 10:01 p.m. UTC | #7

On Wed, Jan 13, 2021 at 10:34:53PM +0100, Heiner Kallweit wrote:
> On 13.01.2021 13:36, [e-mail] wrote:
> > On 13.01.2021 13:09, Heiner Kallweit wrote:
> >> On 13.01.2021 10:29, [e-mail] wrote:
> >>> 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

This is a standard way to enter S2R - I've used it many times in the
past on platforms that support it.

> 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.

When entering S2R, power can (and probably will) be cut to all system
components, certainly all components that do not support wakeup. If
the system doesn't support WoL, then that will include the ethernet
PHY.

When resuming, the responsibility is of the kernel and each driver's
.resume function to ensure that the hardware state is restored. Only
each device driver that knows the device itself can restore the state
of that device. In the case of an ethernet PHY, that is phylib and
its associated PHY driver.

One has to be a tad careful with phylib and PHYs compared to their
MAC devices in terms of the resume order - it has not been unheard
of over the years for a MAC device to be resumed before its connected
PHY has been.

Jakub Kicinski Jan. 13, 2021, 11:28 p.m. UTC | #8

On Fri, 8 Jan 2021 17:45:54 +0200 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.
> 
> Signed-off-by: Claudiu Beznea <[e-mail]>

I think Heiner rises an interesting point, and we can't have this patch
hanging in patchwork for much longer. We'll be expecting a fresh posting
once the discussion settles, regardless of the decision. 

Thanks!

Heiner Kallweit Jan. 14, 2021, 7:30 a.m. UTC | #9

On 13.01.2021 23:01, Russell King - ARM Linux admin wrote:
> On Wed, Jan 13, 2021 at 10:34:53PM +0100, Heiner Kallweit wrote:
>> On 13.01.2021 13:36, [e-mail] wrote:
>>> On 13.01.2021 13:09, Heiner Kallweit wrote:
>>>> On 13.01.2021 10:29, [e-mail] wrote:
>>>>> 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
> 
> This is a standard way to enter S2R - I've used it many times in the
> past on platforms that support it.
> 
>> 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.
> 
> When entering S2R, power can (and probably will) be cut to all system
> components, certainly all components that do not support wakeup. If
> the system doesn't support WoL, then that will include the ethernet
> PHY.
> 
I'm with you if we talk about a driver's suspend callback cutting power
to the component it controls, or at least putting it to a power-saving
state. However a driver shouldn't have to expect that during S2R somebody
else cuts the power. If this would be the case, then I think we wouldn't
need separate resume and restore pm callbacks in general.

> When resuming, the responsibility is of the kernel and each driver's
> .resume function to ensure that the hardware state is restored. Only
> each device driver that knows the device itself can restore the state
> of that device. In the case of an ethernet PHY, that is phylib and
> its associated PHY driver.
> 
Also in phylib we have separate functions mdio_bus_phy_resume() and
mdio_bus_phy_restore(), with the first one not fully reconfiguring
the PHY.

> One has to be a tad careful with phylib and PHYs compared to their
> MAC devices in terms of the resume order - it has not been unheard
> of over the years for a MAC device to be resumed before its connected
> PHY has been.
> 
Right.

Claudiu Beznea Jan. 14, 2021, 10:12 a.m. UTC | #10

Hi, Rafael, Pavel,

I recently posted a patch for re-configuring an ethernet PHY on its
.resume() callback as this PHY is in a setup with SAMA7G5 SoC that supports
a power saving mode (called backup). In this power saving mode most of the
SoC devices' power is cut of (except the RAM + its controller since the RAM
is kept in self-refresh).

This mode is mapped on standard suspend-to-mem modes (standby or mem). When
one issues:
echo mem > /sys/power/state
echo standby > /sys/power/state
the suspend()/resume() functions of each driver are saving/restoring the
content of its registers and in the platform specific PM code
(arch/arm/mach-at91/{pm.c, pm_suspend.S} the RAM is switched to
self-refresh and the CPU executes its shutdown command [1].

The power to CPU (and to other devices on the board) is delivered by a
PMIC. The PMIC regulators have (among other states) a state corresponding
to the system running state and a state corresponding to the backup state.
The PMIC regulators for each state are configured in the system
initialization phase based on device tree bindings.

On suspend, on PMIC driver is configured the next running state but the
transition to this next running state is done when the CPU executes
shutdown command [1]. There is a physical connection b/w PMIC and SoC that
asserts when CPU executes its shutdown command.

+-----+  shdn  +-----+
|PMIC |<-------| SoC |
+-----+        +-----+

So, at the end of the suspend procedure for backup mode the PMIC will
switch to the state corresponding to backup mode where all its regulators
(except the one that feeds the RAM and RAM controllers) were configured to
be turn off. Since there is a limited amount of rails on the PMIC used in
the configuration with this SoC some of the closed rails may feed other
devices on the board, e.g. an ethernet PHY, some may be on the same rail
with CPU.


+-----+  shdn  +-----+
|PMIC |<-------| SoC |
+-----+        +-----+
   | 3v3         / \
   +-------+------+
           |
           |       +---------+
           +------>| eth PHY |
                   +---------+

Up to this moment we treat this backup mode as S2R mode since the memory
was kept in self-refresh mode. Each driver was saving/restoring in/from RAM
the content of associated registers in the suspend/resume phase.

Now, SAMA7G5 is used in setups with KSZ9131 PHY in RGMII mode. For RGMII
mode user may configure skew values or DLL settings on the PHY that is
imperative for 1Gbps speeds to be reached on Ethernet link in RGMII. These
skew settings may depend on the length of TX/RX lines b/w PHY and MAC
inside SoC. These settings are passed to the PHY driver via device tree
bindings.

For this PHY I prepared a patch that re-configures these settings on PHY
driver (see at the end of the tread) on resume() function of the driver.

Heiner (in To list) proposed me to involve you (since you are the experts
on PM) in the discussion.

The questions that arries this topic (Heiner, Russel, anyone involved in
the discussion, correct me if I wrongly understood):
1/ is it OK to still treat this backup mode as a S2R mode or as a hibernate
mode? Since hibernate would treat the devices (including Ethernet PHY in
this case) as they were just powered and restore the registers content but
taking into account that in backup mode we keep the RAM in self-refresh?
2/ is it OK to have these kind of reconfiguration of one device that end up
in suspend mode with no power (in this case the Ethernet PHY) due to a
system power cut off (in this case CPU + PMIC)?

Thank you,
Claudiu Beznea

[1]
https://elixir.bootlin.com/linux/latest/source/arch/arm/mach-at91/pm_suspend.S#L159


On 13.01.2021 23:34, Heiner Kallweit wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 13.01.2021 13:36, [e-mail] 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, [e-mail] 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" <[e-mail]>
> M:      Pavel Machek <[e-mail]>
> L:      [e-mail]
> 
> 
>>>> Thank you,
>>>> Claudiu Beznea
>>>>
>>>>>
>>>>>> Signed-off-by: Claudiu Beznea <[e-mail]>
>>>>>> ---
>>>>>>  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;
>>>>>>
>>>>>>
>

Russell King (Oracle) Jan. 14, 2021, 10:25 a.m. UTC | #11

On Thu, Jan 14, 2021 at 10:12:13AM +0000, [e-mail] wrote:
> Up to this moment we treat this backup mode as S2R mode since the memory
> was kept in self-refresh mode. Each driver was saving/restoring in/from RAM
> the content of associated registers in the suspend/resume phase.

This is exactly what suspend-to-RAM is. The system is largely powered
down with the state saved in RAM, and the RAM placed in self-refresh
mode. Some devices or parts of devices may remain powered up if needed
to be a wake-up source.

> The questions that arries this topic (Heiner, Russel, anyone involved in
> the discussion, correct me if I wrongly understood):
> 1/ is it OK to still treat this backup mode as a S2R mode or as a hibernate
> mode? Since hibernate would treat the devices (including Ethernet PHY in
> this case) as they were just powered and restore the registers content but
> taking into account that in backup mode we keep the RAM in self-refresh?

Hibernate mode is a deeper power-saving mode, where all that applies
with suspend-to-ram applies, plus the critical contents of the RAM is
stored to non-volatile media, and the RAM powered down in addition to
what would also be powered down in the suspend-to-ram case.

If you are placing the RAM in self-refresh and powering the system down,
you are in suspend-to-ram mode, not hibernate mode.

> 2/ is it OK to have these kind of reconfiguration of one device that end up
> in suspend mode with no power (in this case the Ethernet PHY) due to a
> system power cut off (in this case CPU + PMIC)?

You have nothing out of the ordinary here.  Going back years, the
Assabet/Neponest (SA1110 based platform) does this. When the CPU
enters suspend mode, a pin on the processor indicates to the external
world that has happened, and that cuts power to most of the system
including the smc91x and integrated PHY. (it doesn't use phylib.)

So there's really nothing special about your situation; from what you
have described, it's a pretty standard suspend-to-ram implementation.

As I've said, if phylib/PHY driver is not restoring the state of the
PHY on resume from suspend-to-ram, then that's an issue with phylib
and/or the phy driver.

Claudiu Beznea Jan. 14, 2021, 10:41 a.m. UTC | #12

On 14.01.2021 12:25, Russell King - ARM Linux admin wrote:
> 
> As I've said, if phylib/PHY driver is not restoring the state of the
> PHY on resume from suspend-to-ram, then that's an issue with phylib
> and/or the phy driver.

In the patch I proposed in this thread the restoring is done in PHY driver.
Do you think I should continue the investigation and check if something
should be done from the phylib itself?

Thank you,
Claudiu Beznea

> 
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
>

Heiner Kallweit Jan. 14, 2021, 11:05 a.m. UTC | #13

On 14.01.2021 11:41, [e-mail] wrote:
> 
> 
> On 14.01.2021 12:25, Russell King - ARM Linux admin wrote:
>>
>> As I've said, if phylib/PHY driver is not restoring the state of the
>> PHY on resume from suspend-to-ram, then that's an issue with phylib
>> and/or the phy driver.
> 
> In the patch I proposed in this thread the restoring is done in PHY driver.
> Do you think I should continue the investigation and check if something
> should be done from the phylib itself?
> 
It was the right move to approach the PM maintainers to clarify whether
the resume PM callback has to assume that power had been cut off and
it has to completely reconfigure the device. If they confirm this
understanding, then:
- the general question remains why there's separate resume and restore
  callbacks, and what restore is supposed to do that resume doesn't
  have to do
- it should be sufficient to use mdio_bus_phy_restore also as resume
  callback (instead of changing each and every PHY driver's resume),
  because we can expect that somebody cutting off power to the PHY
  properly suspends the MDIO bus before

> Thank you,
> Claudiu Beznea
> 
>>
>> --
>> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
>> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Claudiu Beznea Feb. 11, 2021, 11:18 a.m. UTC | #14

Hi, Rafael, Pavel,

I know you may be busy. Just a gentle ping on this topic. It would be nice
to have your input in this.

Thank you,
Claudiu Beznea

On 14.01.2021 13:05, Heiner Kallweit wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 14.01.2021 11:41, [e-mail] wrote:
>>
>>
>> On 14.01.2021 12:25, Russell King - ARM Linux admin wrote:
>>>
>>> As I've said, if phylib/PHY driver is not restoring the state of the
>>> PHY on resume from suspend-to-ram, then that's an issue with phylib
>>> and/or the phy driver.
>>
>> In the patch I proposed in this thread the restoring is done in PHY driver.
>> Do you think I should continue the investigation and check if something
>> should be done from the phylib itself?
>>
> It was the right move to approach the PM maintainers to clarify whether
> the resume PM callback has to assume that power had been cut off and
> it has to completely reconfigure the device. If they confirm this
> understanding, then:
> - the general question remains why there's separate resume and restore
>   callbacks, and what restore is supposed to do that resume doesn't
>   have to do
> - it should be sufficient to use mdio_bus_phy_restore also as resume
>   callback (instead of changing each and every PHY driver's resume),
>   because we can expect that somebody cutting off power to the PHY
>   properly suspends the MDIO bus before
> 
>> Thank you,
>> Claudiu Beznea
>>
>>>
>>> --
>>> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
>>> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
>

Pavel Machek Feb. 11, 2021, 12:17 p.m. UTC | #15

On Thu 2021-01-14 12:05:21, Heiner Kallweit wrote:
> On 14.01.2021 11:41, [e-mail] wrote:
> > 
> > 
> > On 14.01.2021 12:25, Russell King - ARM Linux admin wrote:
> >>
> >> As I've said, if phylib/PHY driver is not restoring the state of the
> >> PHY on resume from suspend-to-ram, then that's an issue with phylib
> >> and/or the phy driver.
> > 
> > In the patch I proposed in this thread the restoring is done in PHY driver.
> > Do you think I should continue the investigation and check if something
> > should be done from the phylib itself?
> > 
> It was the right move to approach the PM maintainers to clarify whether
> the resume PM callback has to assume that power had been cut off and
> it has to completely reconfigure the device. If they confirm this
> understanding, then:

Power to some devices can be cut during s2ram, yes.

> - the general question remains why there's separate resume and restore
>   callbacks, and what restore is supposed to do that resume doesn't
>   have to do

You'll often have same implementation, yes.

> - it should be sufficient to use mdio_bus_phy_restore also as resume
>   callback (instead of changing each and every PHY driver's resume),
>   because we can expect that somebody cutting off power to the PHY
>   properly suspends the MDIO bus before

If restore works with power cut and power not cut then yes, you should
get away with that.

Best regards,
								Pavel

Heiner Kallweit Feb. 11, 2021, 12:36 p.m. UTC | #16

On 11.02.2021 13:17, Pavel Machek wrote:
> On Thu 2021-01-14 12:05:21, Heiner Kallweit wrote:
>> On 14.01.2021 11:41, [e-mail] wrote:
>>>
>>>
>>> On 14.01.2021 12:25, Russell King - ARM Linux admin wrote:
>>>>
>>>> As I've said, if phylib/PHY driver is not restoring the state of the
>>>> PHY on resume from suspend-to-ram, then that's an issue with phylib
>>>> and/or the phy driver.
>>>
>>> In the patch I proposed in this thread the restoring is done in PHY driver.
>>> Do you think I should continue the investigation and check if something
>>> should be done from the phylib itself?
>>>
>> It was the right move to approach the PM maintainers to clarify whether
>> the resume PM callback has to assume that power had been cut off and
>> it has to completely reconfigure the device. If they confirm this
>> understanding, then:
> 
> Power to some devices can be cut during s2ram, yes.
> 
Thanks for the confirmation.

>> - the general question remains why there's separate resume and restore
>>   callbacks, and what restore is supposed to do that resume doesn't
>>   have to do
> 
> You'll often have same implementation, yes.
> 

If resume and restore both have to assume that power was cut off,
then they have to fully re-initialize the device. Therefore it's still
not clear to me when you would have differing implementations for both
callbacks.

>> - it should be sufficient to use mdio_bus_phy_restore also as resume
>>   callback (instead of changing each and every PHY driver's resume),
>>   because we can expect that somebody cutting off power to the PHY
>>   properly suspends the MDIO bus before
> 
> If restore works with power cut and power not cut then yes, you should
> get away with that.
> 
> Best regards,
> 								Pavel
> 

Heiner

Pavel Machek Feb. 11, 2021, 8:34 p.m. UTC | #17

On Thu 2021-02-11 13:36:16, Heiner Kallweit wrote:
> On 11.02.2021 13:17, Pavel Machek wrote:
> > On Thu 2021-01-14 12:05:21, Heiner Kallweit wrote:
> >> On 14.01.2021 11:41, [e-mail] wrote:
> >>>
> >>>
> >>> On 14.01.2021 12:25, Russell King - ARM Linux admin wrote:
> >>>>
> >>>> As I've said, if phylib/PHY driver is not restoring the state of the
> >>>> PHY on resume from suspend-to-ram, then that's an issue with phylib
> >>>> and/or the phy driver.
> >>>
> >>> In the patch I proposed in this thread the restoring is done in PHY driver.
> >>> Do you think I should continue the investigation and check if something
> >>> should be done from the phylib itself?
> >>>
> >> It was the right move to approach the PM maintainers to clarify whether
> >> the resume PM callback has to assume that power had been cut off and
> >> it has to completely reconfigure the device. If they confirm this
> >> understanding, then:
> > 
> > Power to some devices can be cut during s2ram, yes.
> > 
> Thanks for the confirmation.
> 
> >> - the general question remains why there's separate resume and restore
> >>   callbacks, and what restore is supposed to do that resume doesn't
> >>   have to do
> > 
> > You'll often have same implementation, yes.
> > 
> 
> If resume and restore both have to assume that power was cut off,
> then they have to fully re-initialize the device. Therefore it's still
> not clear to me when you would have differing implementations for both
> callbacks.

Full re-init is easiest way, yes.

But restore had different Linux kernel already booting on device, and
maybe touching the hardware, and resume may or may not cut power to
all devices.

So yes they can be different.

Regards,
								Pavel

Patch

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;