On 13.01.2021 13:36, claudiu.bez...@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.bez...@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" <r...@rjwysocki.net> M: Pavel Machek <pa...@ucw.cz> L: linux...@vger.kernel.org >>> Thank you, >>> Claudiu Beznea >>> >>>> >>>>> Signed-off-by: Claudiu Beznea <claudiu.bez...@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; >>>>> >>>>>
