On 07/16/2017 06:41 AM, Alexandre Belloni wrote:

On 06/07/2017 at 05:18:19 +0200, Pavel Machek wrote:

quoted

On Sat 2017-07-15 20:33:58, Alexandre Belloni wrote:

quoted

On 15/07/2017 at 10:20:27 -0700, Florian Fainelli wrote:

quoted

quoted

We already have

struct regulator_state {
       int uV; /* suspend voltage */
       unsigned int mode; /* suspend regulator operating mode */
       int enabled; /* is regulator enabled in this suspend state */
       int disabled; /* is the regulator disabled in this suspend state */
};

 * struct regulation_constraints - regulator operating constraints.
  * @state_disk: State for regulator when system is suspended in disk
  * mode.
  * @state_mem: State for regulator when system is suspended in mem
  * mode.
  * @state_standby: State for regulator when system is suspended in
  * standby
  *                 mode.
   
. So it seems that maybe we should tell the drivers if we are entering
"state_mem" or "state_standby" (something I may have opposed, sorry),
then the driver can get neccessary information from regulator
framework.

OK, so what would be the mechanism to tell these drivers about the
system wide suspend state they are entering if it is not via
platform_suspend_target_state()?

Keep in mind that regulators might be one aspect of what could be
causing the platform to behave specifically in one suspend state vs.
another, but there could be pieces of HW within the SoC that can't be
described with power domains, voltage islands etc. that would still have
inherent suspend states properties (like memory retention, pin/pad
controls etc. etc). We still need some mechanism, possibly centralized

I concur, the regulator stuff is one aspect of one of our suspend state
(cutting VDDcore). But we have another state where the main clock (going
to the IPs) is going from a few hundred MHz to 32kHz. This is currently
handled by calling at91_suspend_entering_slow_clock(). I think it is
important to take that into account so we can remove this hack from the
kernel.

Cure should not be worse then the disease... and it is in this case.

For clocks, take a look at clock framework, perhaps it already has "clock_will_be_suspended"
as regulator framework had. If not, implement it.

See Rafael's comment, currently, the clock framework can't say whether
the clock will change because it doesn't know anything about the suspend
target.

quoted

Same with memory retention, pin/pad controls.

Same here.

Exactly, here is another side effect of not knowing the platform
suspend/state that I came across on our platforms:

https://patchwork.kernel.org/patch/9561575/

(we later discussed this in details with Linus and this is why this very
patch set is being introduced now)
-- 
Florian