Re: [PATCH/RFC v4 2/4] regulator: fixed: add regulator_ops members for suspend/resume

From: Sudeep Holla <sudeep.holla@arm.com>
To: Mark Brown <broonie@kernel.org>
Cc: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>,
	"ulf.hansson@linaro.org" <ulf.hansson@linaro.org>,
	"lgirdwood@gmail.com" <lgirdwood@gmail.com>,
	"geert+renesas@glider.be" <geert+renesas@glider.be>,
	"magnus.damm@gmail.com" <magnus.damm@gmail.com>,
	"linux-mmc@vger.kernel.org" <linux-mmc@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-renesas-soc@vger.kernel.org" 
	<linux-renesas-soc@vger.kernel.org>,
	Sudeep Holla <sudeep.holla@arm.com>
Subject: Re: [PATCH/RFC v4 2/4] regulator: fixed: add regulator_ops members for suspend/resume
Date: Mon, 29 Jun 2020 14:40:47 +0100	[thread overview]
Message-ID: <20200629134011.GA23284@bogus> (raw)
In-Reply-To: <20200629125756.GC5499@sirena.org.uk>

On Mon, Jun 29, 2020 at 01:57:56PM +0100, Mark Brown wrote:
> On Mon, Jun 29, 2020 at 02:42:26AM +0000, Yoshihiro Shimoda wrote:
> > > From: Mark Brown, Sent: Friday, June 26, 2020 11:39 PM
>
> Copying in Sudeep for the feedback on firmware interfaces.
>

Thanks Mark.

> > > According to the changelog this is all about reflecting changes in the
> > > system state done by firmware but there's no interaction with firmware
> > > here which means this will be at best fragile.  If we need to reflect
> > > changes in firmware configuration I'd expect there to be some
> > > interaction with firmware about how it is configured, or at least that
> > > the configuration would come from the same source.
>

I agree.

> > I should have described background of previous patch series though,
> > according to previous discussion [1] the firmware side (like PSCI) is
> > also fragile unfortunately... So, I thought using regulator-off-in-suspend
> > in a regulator was better.
>

Please fix the firmware. You might have bigger problem than this if the
PSCI firmware is fragile as you state. Better to disable power management
on the platform if the firmware can't be fixed.

> > On other hand, Ulf is talking about either adding a property (perhaps like
> > regulator-off-in-suspend) into a regulator or just adding a new property
> > into MMC [2]. What do you think about Ulf' comment? I'm thinking
> > adding a new property "full-pwr-cycle-in-suspend" is the best solution.
> > This is because using a regulator property and reflecting a state of regulator without
> > firmware is fragile, as you said.

I haven't followed all the threads, but if it related to the policy you
want in the Linux, then may be use DT property or something. I don't know.
But if this is to indicate something based on firmware runtime/configuration,
then NACK for any approaches unconditionally.

>
> TBH I worry about a property drifting out of sync with the firmware on
> systems where the firmware can be updated.  Personally my default
> assumption would always be that we're going to loose power for anything
> except the RAM and whatever is needed for wake sources during suspend so
> I find the discussion a bit surprising but in any case that seems like a
> better option than trying to shoehorn things in the way the series here
> did.  Like I said in my earlier replies if this is done through the
> regulator API I'd expect it to be via the suspend interface.
>

+1. If this platform needs Linux to keep some state on for users in the
firmware or anything outside Linux, it must resume back in the same state
as we entered the suspend state from the kernel.

--
Regards,
Sudeep