The regulator-fixed driver is possible to be off by firmware
like PSCI while the system is suspended. If a consumer could get
such a condition from regulator_is_enabled(), it's useful by
consumers.
So, add some regulator_ops members for it. And then, if
regulator-fixed nodes have suitable sub-nodes and properties [1],
regulator_is_enabled() will return false while suspend() of
a consumer.
[1]
- The node has regulator-state-(standby|mem|disk) sub-nodes.
- The node doesn't have regulator-always-on.
- The sub-node has regulator-off-in-suspend property.
Signed-off-by: Yoshihiro Shimoda <[email protected]>
---
drivers/regulator/fixed.c | 29 +++++++++++++++++++++++++++++
1 file changed, 29 insertions(+)
diff --git a/drivers/regulator/fixed.c b/drivers/regulator/fixed.c
index d54830e..0bd4a1a 100644
--- a/drivers/regulator/fixed.c
+++ b/drivers/regulator/fixed.c
@@ -35,6 +35,7 @@ struct fixed_voltage_data {
struct clk *enable_clock;
unsigned int clk_enable_counter;
+ bool disabled_in_suspend;
};
struct fixed_dev_type {
@@ -49,6 +50,31 @@ static const struct fixed_dev_type fixed_clkenable_data = {
.has_enable_clock = true,
};
+static int reg_is_enabled(struct regulator_dev *rdev)
+{
+ struct fixed_voltage_data *priv = rdev_get_drvdata(rdev);
+
+ return !priv->disabled_in_suspend;
+}
+
+static int reg_prepare_disable(struct regulator_dev *rdev)
+{
+ struct fixed_voltage_data *priv = rdev_get_drvdata(rdev);
+
+ priv->disabled_in_suspend = true;
+
+ return 0;
+}
+
+static int reg_resume_early_disable(struct regulator_dev *rdev)
+{
+ struct fixed_voltage_data *priv = rdev_get_drvdata(rdev);
+
+ priv->disabled_in_suspend = false;
+
+ return 0;
+}
+
static int reg_clock_enable(struct regulator_dev *rdev)
{
struct fixed_voltage_data *priv = rdev_get_drvdata(rdev);
@@ -132,6 +158,9 @@ of_get_fixed_voltage_config(struct device *dev,
}
static struct regulator_ops fixed_voltage_ops = {
+ .is_enabled = reg_is_enabled,
+ .set_prepare_disable = reg_prepare_disable,
+ .clear_resume_early_disable = reg_resume_early_disable,
};
static struct regulator_ops fixed_voltage_clkenabled_ops = {
--
2.7.4
On Fri, Jun 26, 2020 at 06:32:20PM +0900, Yoshihiro Shimoda wrote:
> +static int reg_is_enabled(struct regulator_dev *rdev)
> +{
> + struct fixed_voltage_data *priv = rdev_get_drvdata(rdev);
> +
> + return !priv->disabled_in_suspend;
> +}
This is broken, the state of the regualtor during system runtime need
have no connection with the state of the regulator during system
suspend.
> +static int reg_prepare_disable(struct regulator_dev *rdev)
> +{
> + struct fixed_voltage_data *priv = rdev_get_drvdata(rdev);
> +
> + priv->disabled_in_suspend = true;
> +
> + return 0;
> +}
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.
Hi Mark,
> From: Mark Brown, Sent: Friday, June 26, 2020 11:39 PM
>
> On Fri, Jun 26, 2020 at 06:32:20PM +0900, Yoshihiro Shimoda wrote:
>
> > +static int reg_is_enabled(struct regulator_dev *rdev)
> > +{
> > + struct fixed_voltage_data *priv = rdev_get_drvdata(rdev);
> > +
> > + return !priv->disabled_in_suspend;
> > +}
>
> This is broken, the state of the regualtor during system runtime need
> have no connection with the state of the regulator during system
> suspend.
>
> > +static int reg_prepare_disable(struct regulator_dev *rdev)
> > +{
> > + struct fixed_voltage_data *priv = rdev_get_drvdata(rdev);
> > +
> > + priv->disabled_in_suspend = true;
> > +
> > + return 0;
> > +}
>
> 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 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.
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.
[1]
https://lore.kernel.org/linux-renesas-soc/CAMuHMdXjU7N4oG89YsozGijMpjgKGN6ezw2qm6FeGX=JyRhsvg@mail.gmail.com/
[2]
https://lore.kernel.org/linux-renesas-soc/CAPDyKFpiBU1D+a7zb+Ggm0_HZ+YR4=LXJZ5MPytXtT=uBEdjPA@mail.gmail.com/
Best regards,
Yoshihiro Shimoda
On Mon, Jun 29, 2020 at 04:15:39PM +0200, Geert Uytterhoeven wrote:
> Hi Sudeep,
>
> On Mon, Jun 29, 2020 at 3:40 PM Sudeep Holla <[email protected]> wrote:
> > 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
> > > > > 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.
>
> Saying the implementation is "fragile" might be bad wording.
> The issue is more with the specification being vague (see more below).
>
Please elaborate on the vague part of the specification, happy to help you
get that fixed if it really needs to be.
> > > > 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.
>
> Like "arm,psci-system-suspend-is-power-down"[1]?
>
Really sounds hack to me.
> > > 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
>
> OK, so that's the "safe" way to handle this: assume power is lost.
>
> > > except the RAM and whatever is needed for wake sources during suspend so
>
> Oh, even wake-up sources may become unpowered[2] ;-)
That is some serious issue. If there is no power, how can you expect it
to be wake up source ? Sounds something wrong fundamentally IMO.
> And thus stop working ;-(
>
> > > 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.
>
> I think you're misunderstanding the issue: this is not related at all
> to Linux keeping state for non-Linux users.
>
OK, thanks for confirming.
> This is all about how to know what exactly PSCI is powering down during
> SYSTEM_SUSPEND. In this specific case, it is about knowing if the eMMC
> is powered down or not, as Linux should follow a specific procedure to
> prepare the eMMC for that, and Linux should not if that isn't the case.
>
OK, unless you are optimising, you shouldn't care then what PSCI does.
If you don't need eMMC, just suspend/power it off before you enter system/
psci suspend.
> I had a quick look at the latest revision of the PSCI specification, and
> it doesn't look like anything has changed in that area since my old patch
> series from 2017. So it still boils down to: we don't know what a
> specific PSCI implementation will do, as basically anything is
> compliant, so the only safe thing is to assume the worst.
>
The specification states clearly:
"... all devices in the system must be in a state that is compatible
with entry into the system state. These preconditions are beyond the scope
of this specification and are therefore not described here."
"Prior to the call, the OS must disable all sources of wakeup other than
those it needs to support for its implementation of suspend to RAM."
And of course, the firmware must rely on OSPM to do proper device PM if
it is not shared resource. Trying to be aggressive and turning off all
the wakeup sources which out knowledge of it is broken firmware.
I see nothing has been fixed in the firmware too and we are still
discussing the same after 3 years ????. Clearly we should start trusting
firmware and built capability to fix and replace it if there are bugs
just like kernel and stop hacking around in the kernel to deal with
just broken platform/psci firmware.
--
Regards,
Sudeep
On Mon, Jun 29, 2020 at 05:42:07PM +0100, Sudeep Holla wrote:
> On Mon, Jun 29, 2020 at 05:14:50PM +0100, Mark Brown wrote:
> > On Mon, Jun 29, 2020 at 04:07:28PM +0100, Sudeep Holla wrote:
> >
> > > The specification states clearly:
> > > "... all devices in the system must be in a state that is compatible
> > > with entry into the system state. These preconditions are beyond the scope
> > > of this specification and are therefore not described here."
> > > "Prior to the call, the OS must disable all sources of wakeup other than
> > > those it needs to support for its implementation of suspend to RAM."
> > This gets a bit circular for a generic OS since the OS needs some way to
> > figure out what it's supposed to do on a given platform - for example
> > the OS may be happy to use wakeup sources that the firmware is just
> > going to cut power on.
> While I understand the sentiments here, PSCI is targeted to address CPU
> power state management mainly and system states like suspend/reset and
> poweroff which involves last CPU. This is one of the reason it is out of
> the scope of the specification.
Sure, but as soon as we start talking about the last CPU stuff we're
inevitably talking about the system as a whole.
> Here is my understanding. DT specifies all the wakeup sources. Linux
> can configure those and user may choose to enable a subset of them is
> wakeup. As stated in the spec and also based on what we already do in
> the kernel, we disable all other wakeup sources.
> The PSCI firmware can then either read those from the interrupt controller
> or based on static platform understanding, must not disable those wakeup
> sources.
That bit about static platform understanding isn't super helpful for the
OS, so long as the firmware might do that the OS is pretty much out of
luck.
> > > I see nothing has been fixed in the firmware too and we are still
> > > discussing the same after 3 years ????. Clearly we should start trusting
> > > firmware and built capability to fix and replace it if there are bugs
> > > just like kernel and stop hacking around in the kernel to deal with
> > > just broken platform/psci firmware.
> > This isn't just an issue of buggy firmware as far as I can see, it's
> > also a lack of ability for the OS and firmware to communicate
> > information about their intentions to each other. As things stand you'd
> > need to put static information in the DT.
> It is easy for DT to get out of sync with firmware unless it is generated
> by the same firmware. That's the reason why I am against such multiple
The ability for things to get out of sync also concerns me as I said
further back in the thread but I'm not sure we have much alternative,
realistically we're going to need some facility to work around firmware
that isn't ideal.
> sources of information. I understand ACPI has more flexibility and I did
ACPI has a much stronger idea of what the system looks like which helps
it a lot here.
> Each device or platform having its specific property in DT is not scalable.
> Not sure if it is a generic problem. If it is, I would like to understand
> more details on such lack of ability for communtication between OS and
> firmware.
It seems like a generic issue from where I'm sitting.
On Mon, Jun 29, 2020 at 06:26:51PM +0100, Mark Brown wrote:
> On Mon, Jun 29, 2020 at 05:42:07PM +0100, Sudeep Holla wrote:
> > On Mon, Jun 29, 2020 at 05:14:50PM +0100, Mark Brown wrote:
> > > On Mon, Jun 29, 2020 at 04:07:28PM +0100, Sudeep Holla wrote:
> > >
> > > > The specification states clearly:
> > > > "... all devices in the system must be in a state that is compatible
> > > > with entry into the system state. These preconditions are beyond the scope
> > > > of this specification and are therefore not described here."
> > > > "Prior to the call, the OS must disable all sources of wakeup other than
> > > > those it needs to support for its implementation of suspend to RAM."
>
> > > This gets a bit circular for a generic OS since the OS needs some way to
> > > figure out what it's supposed to do on a given platform - for example
> > > the OS may be happy to use wakeup sources that the firmware is just
> > > going to cut power on.
>
> > While I understand the sentiments here, PSCI is targeted to address CPU
> > power state management mainly and system states like suspend/reset and
> > poweroff which involves last CPU. This is one of the reason it is out of
> > the scope of the specification.
>
> Sure, but as soon as we start talking about the last CPU stuff we're
> inevitably talking about the system as a whole.
>
> > Here is my understanding. DT specifies all the wakeup sources. Linux
> > can configure those and user may choose to enable a subset of them is
> > wakeup. As stated in the spec and also based on what we already do in
> > the kernel, we disable all other wakeup sources.
>
> > The PSCI firmware can then either read those from the interrupt controller
> > or based on static platform understanding, must not disable those wakeup
> > sources.
>
> That bit about static platform understanding isn't super helpful for the
> OS, so long as the firmware might do that the OS is pretty much out of
> luck.
>
I don't disagree. That's one of the reason I wanted to gather requirement
few years back when PSCI system suspend was introduced. I couldn't
convince PSCI spec authors myself.
> > > > I see nothing has been fixed in the firmware too and we are still
> > > > discussing the same after 3 years ????. Clearly we should start trusting
> > > > firmware and built capability to fix and replace it if there are bugs
> > > > just like kernel and stop hacking around in the kernel to deal with
> > > > just broken platform/psci firmware.
>
> > > This isn't just an issue of buggy firmware as far as I can see, it's
> > > also a lack of ability for the OS and firmware to communicate
> > > information about their intentions to each other. As things stand you'd
> > > need to put static information in the DT.
>
> > It is easy for DT to get out of sync with firmware unless it is generated
> > by the same firmware. That's the reason why I am against such multiple
>
> The ability for things to get out of sync also concerns me as I said
> further back in the thread but I'm not sure we have much alternative,
> realistically we're going to need some facility to work around firmware
> that isn't ideal.
>
I understand and I agree. That's the main reason I want to understand this
better and provide a generic solution. The current pm_suspend_via_firmware
seem to have different intentions(at-least that's what I grasped quickly
reading through the document)
> > sources of information. I understand ACPI has more flexibility and I did
>
> ACPI has a much stronger idea of what the system looks like which helps
> it a lot here.
>
Yes, I wanted something similar initially but didn't get good response
both from community and PSCI spec authors.
> > Each device or platform having its specific property in DT is not scalable.
> > Not sure if it is a generic problem. If it is, I would like to understand
> > more details on such lack of ability for communtication between OS and
> > firmware.
>
> It seems like a generic issue from where I'm sitting.
I can't disagree with that. The description of the issue and the solution
in the thread is. We need make it more generic and provide generic solution,.
I already see 2-3 threads addressing this in isolation as specific issue.
Also we need to check with DT maintainers if there are fine with the idea
of binding to address this issue.
--
Regards,
Sudeep
Hi Sudeep,
On Mon, Jun 29, 2020 at 3:40 PM Sudeep Holla <[email protected]> wrote:
> 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
> > > > 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.
Saying the implementation is "fragile" might be bad wording.
The issue is more with the specification being vague (see more below).
> > > 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.
Like "arm,psci-system-suspend-is-power-down"[1]?
> > 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
OK, so that's the "safe" way to handle this: assume power is lost.
> > except the RAM and whatever is needed for wake sources during suspend so
Oh, even wake-up sources may become unpowered[2] ;-)
And thus stop working ;-(
> > 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.
I think you're misunderstanding the issue: this is not related at all
to Linux keeping state for non-Linux users.
This is all about how to know what exactly PSCI is powering down during
SYSTEM_SUSPEND. In this specific case, it is about knowing if the eMMC
is powered down or not, as Linux should follow a specific procedure to
prepare the eMMC for that, and Linux should not if that isn't the case.
I had a quick look at the latest revision of the PSCI specification, and
it doesn't look like anything has changed in that area since my old patch
series from 2017. So it still boils down to: we don't know what a
specific PSCI implementation will do, as basically anything is
compliant, so the only safe thing is to assume the worst.
Or am I missing something?
Thanks!
[1] "[PATCH/RFC 4/6] drivers: firmware: psci: Fix non-PMIC wake-up if
SYSTEM_SUSPEND cuts power"
https://lore.kernel.org/linux-arm-kernel/[email protected]/
[2] [PATCH/RFC 0/6] PSCI: Fix non-PMIC wake-up if SYSTEM_SUSPEND cuts
power
https://lore.kernel.org/linux-arm-kernel/[email protected]/
[3] https://developer.arm.com/architectures/system-architectures/software-standards/psci
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Mon, Jun 29, 2020 at 05:14:50PM +0100, Mark Brown wrote:
> On Mon, Jun 29, 2020 at 04:07:28PM +0100, Sudeep Holla wrote:
> > On Mon, Jun 29, 2020 at 04:15:39PM +0200, Geert Uytterhoeven wrote:
>
> > > This is all about how to know what exactly PSCI is powering down during
> > > SYSTEM_SUSPEND. In this specific case, it is about knowing if the eMMC
> > > is powered down or not, as Linux should follow a specific procedure to
> > > prepare the eMMC for that, and Linux should not if that isn't the case.
>
> > OK, unless you are optimising, you shouldn't care then what PSCI does.
> > If you don't need eMMC, just suspend/power it off before you enter system/
> > psci suspend.
>
> That only works if the power off procedure doesn't require that power be
> removed as part of the procedure. There's a reasonable argument that
> specs that have such requirements are unhelpful but that doesn't mean
> that nobody will make hardware with such requrements which creates
> problems for generic code that needs to control that hardware if it
> can't discover the power state over suspend.
>
Fair enough.
> > > I had a quick look at the latest revision of the PSCI specification, and
> > > it doesn't look like anything has changed in that area since my old patch
> > > series from 2017. So it still boils down to: we don't know what a
> > > specific PSCI implementation will do, as basically anything is
> > > compliant, so the only safe thing is to assume the worst.
>
> > The specification states clearly:
> > "... all devices in the system must be in a state that is compatible
> > with entry into the system state. These preconditions are beyond the scope
> > of this specification and are therefore not described here."
> > "Prior to the call, the OS must disable all sources of wakeup other than
> > those it needs to support for its implementation of suspend to RAM."
>
> This gets a bit circular for a generic OS since the OS needs some way to
> figure out what it's supposed to do on a given platform - for example
> the OS may be happy to use wakeup sources that the firmware is just
> going to cut power on.
>
While I understand the sentiments here, PSCI is targeted to address CPU
power state management mainly and system states like suspend/reset and
poweroff which involves last CPU. This is one of the reason it is out of
the scope of the specification.
Here is my understanding. DT specifies all the wakeup sources. Linux
can configure those and user may choose to enable a subset of them is
wakeup. As stated in the spec and also based on what we already do in
the kernel, we disable all other wakeup sources.
The PSCI firmware can then either read those from the interrupt controller
or based on static platform understanding, must not disable those wakeup
sources.
> > I see nothing has been fixed in the firmware too and we are still
> > discussing the same after 3 years ????. Clearly we should start trusting
> > firmware and built capability to fix and replace it if there are bugs
> > just like kernel and stop hacking around in the kernel to deal with
> > just broken platform/psci firmware.
>
> This isn't just an issue of buggy firmware as far as I can see, it's
> also a lack of ability for the OS and firmware to communicate
> information about their intentions to each other. As things stand you'd
> need to put static information in the DT.
It is easy for DT to get out of sync with firmware unless it is generated
by the same firmware. That's the reason why I am against such multiple
sources of information. I understand ACPI has more flexibility and I did
discuss this in LPC few years back and people were happy to have simple
model as I have mentioned above. I was pushing to add more clarity to
the specification but as I mentioned it was rejected as it is more a cpu
and system centric spec and avoids talking about devices which I kind
of agree.
Each device or platform having its specific property in DT is not scalable.
Not sure if it is a generic problem. If it is, I would like to understand
more details on such lack of ability for communtication between OS and
firmware.
--
Regards,
Sudeep
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
On Mon, Jun 29, 2020 at 04:07:28PM +0100, Sudeep Holla wrote:
> On Mon, Jun 29, 2020 at 04:15:39PM +0200, Geert Uytterhoeven wrote:
> > This is all about how to know what exactly PSCI is powering down during
> > SYSTEM_SUSPEND. In this specific case, it is about knowing if the eMMC
> > is powered down or not, as Linux should follow a specific procedure to
> > prepare the eMMC for that, and Linux should not if that isn't the case.
> OK, unless you are optimising, you shouldn't care then what PSCI does.
> If you don't need eMMC, just suspend/power it off before you enter system/
> psci suspend.
That only works if the power off procedure doesn't require that power be
removed as part of the procedure. There's a reasonable argument that
specs that have such requirements are unhelpful but that doesn't mean
that nobody will make hardware with such requrements which creates
problems for generic code that needs to control that hardware if it
can't discover the power state over suspend.
> > I had a quick look at the latest revision of the PSCI specification, and
> > it doesn't look like anything has changed in that area since my old patch
> > series from 2017. So it still boils down to: we don't know what a
> > specific PSCI implementation will do, as basically anything is
> > compliant, so the only safe thing is to assume the worst.
> The specification states clearly:
> "... all devices in the system must be in a state that is compatible
> with entry into the system state. These preconditions are beyond the scope
> of this specification and are therefore not described here."
> "Prior to the call, the OS must disable all sources of wakeup other than
> those it needs to support for its implementation of suspend to RAM."
This gets a bit circular for a generic OS since the OS needs some way to
figure out what it's supposed to do on a given platform - for example
the OS may be happy to use wakeup sources that the firmware is just
going to cut power on.
> I see nothing has been fixed in the firmware too and we are still
> discussing the same after 3 years ????. Clearly we should start trusting
> firmware and built capability to fix and replace it if there are bugs
> just like kernel and stop hacking around in the kernel to deal with
> just broken platform/psci firmware.
This isn't just an issue of buggy firmware as far as I can see, it's
also a lack of ability for the OS and firmware to communicate
information about their intentions to each other. As things stand you'd
need to put static information in the DT.
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.
> > 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 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.
> 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.
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]
> https://lore.kernel.org/linux-renesas-soc/CAMuHMdXjU7N4oG89YsozGijMpjgKGN6ezw2qm6FeGX=JyRhsvg@mail.gmail.com/
>
> [2]
> https://lore.kernel.org/linux-renesas-soc/CAPDyKFpiBU1D+a7zb+Ggm0_HZ+YR4=LXJZ5MPytXtT=uBEdjPA@mail.gmail.com/
>
> Best regards,
> Yoshihiro Shimoda
>
Hi Mark,
> From: Mark Brown, Sent: Monday, June 29, 2020 9:58 PM
>
> 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.
Thank you very much for the discussion about the firmware.
> > > 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 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.
>
> > 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.
>
> TBH I worry about a property drifting out of sync with the firmware on
> systems where the firmware can be updated.
I understood it.
> 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.
Thank you for your comment! So, I'll make such a patch series later.
> 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.
I don't intend to use any regulator API for this issue for now.
Best regards,
Yoshihiro Shimoda
> > [1]
> > https://lore.kernel.org/linux-renesas-soc/CAMuHMdXjU7N4oG89YsozGijMpjgKGN6ezw2qm6FeGX=JyRhsvg@mail.gmail.com/
> >
> > [2]
> > https://lore.kernel.org/linux-renesas-soc/CAPDyKFpiBU1D+a7zb+Ggm0_HZ+YR4=LXJZ5MPytXtT=uBEdjPA@mail.gmail.com/
> >
> > Best regards,
> > Yoshihiro Shimoda
> >