[PATCH v2] pinctrl: pinctrl-imx8mq: Add suspend/resume ops
To support pinctl hog restore after LPSR resume back,
add the generic suspend/resume in pinctrl-imx along with the
generic pm opsto be used by platform specific drivers. Then
make use of the newly added ops in i.MX8MQ platform specific
driver.
Signed-off-by: Robin Gong
Signed-off-by: Abel Vesa
---
Changes since v1:
- Replaced the imx8mq specific ops with generic ones.
drivers/pinctrl/freescale/pinctrl-imx.c| 25 +
drivers/pinctrl/freescale/pinctrl-imx.h| 4
drivers/pinctrl/freescale/pinctrl-imx8mq.c | 1 +
3 files changed, 30 insertions(+)
diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c
b/drivers/pinctrl/freescale/pinctrl-imx.c
index 188001b..ee120ee 100644
--- a/drivers/pinctrl/freescale/pinctrl-imx.c
+++ b/drivers/pinctrl/freescale/pinctrl-imx.c
@@ -887,3 +887,28 @@ int imx_pinctrl_probe(struct platform_device *pdev,
return ret;
}
+
+int imx_pinctrl_suspend(struct device *dev)
+{
+ struct imx_pinctrl *ipctl = dev_get_drvdata(dev);
+
+ if (!ipctl)
+ return -EINVAL;
+
+ return pinctrl_force_sleep(ipctl->pctl);
+}
+
+int imx_pinctrl_resume(struct device *dev)
+{
+ struct imx_pinctrl *ipctl = dev_get_drvdata(dev);
+
+ if (!ipctl)
+ return -EINVAL;
+
+ return pinctrl_force_default(ipctl->pctl);
+}
+
+const struct dev_pm_ops imx_pinctrl_pm_ops = {
+ SET_LATE_SYSTEM_SLEEP_PM_OPS(imx_pinctrl_suspend,
+ imx_pinctrl_resume)
+};
diff --git a/drivers/pinctrl/freescale/pinctrl-imx.h
b/drivers/pinctrl/freescale/pinctrl-imx.h
index 98a4889..ae78314 100644
--- a/drivers/pinctrl/freescale/pinctrl-imx.h
+++ b/drivers/pinctrl/freescale/pinctrl-imx.h
@@ -17,6 +17,7 @@
struct platform_device;
extern struct pinmux_ops imx_pmx_ops;
+extern const struct dev_pm_ops imx_pinctrl_pm_ops;
/**
* struct imx_pin_mmio - MMIO pin configurations
@@ -136,6 +137,9 @@ struct imx_pinctrl {
int imx_pinctrl_probe(struct platform_device *pdev,
const struct imx_pinctrl_soc_info *info);
+int imx_pinctrl_suspend(struct device *dev);
+int imx_pinctrl_resume(struct device *dev);
+
#ifdef CONFIG_PINCTRL_IMX_SCU
#define BM_PAD_CTL_GP_ENABLE BIT(30)
#define BM_PAD_CTL_IFMUX_ENABLEBIT(31)
diff --git a/drivers/pinctrl/freescale/pinctrl-imx8mq.c
b/drivers/pinctrl/freescale/pinctrl-imx8mq.c
index 8d39af5..50aa1c0 100644
--- a/drivers/pinctrl/freescale/pinctrl-imx8mq.c
+++ b/drivers/pinctrl/freescale/pinctrl-imx8mq.c
@@ -339,6 +339,7 @@ static struct platform_driver imx8mq_pinctrl_driver = {
.driver = {
.name = "imx8mq-pinctrl",
.of_match_table = of_match_ptr(imx8mq_pinctrl_of_match),
+ .pm = &imx_pinctrl_pm_ops,
.suppress_bind_attrs = true,
},
.probe = imx8mq_pinctrl_probe,
--
2.7.4
Re: [PATCH] pinctrl: intel: save HOSTSW_OWN register over suspend/resume
On Thu, Apr 04, 2019 at 09:06:04PM +0800, Chris Chiu wrote:
> On Wed, Apr 3, 2019 at 9:06 PM Andy Shevchenko
> wrote:
> > On Wed, Apr 03, 2019 at 03:06:43PM +0800, Chris Chiu wrote:
> > > On Tue, Apr 2, 2019 at 7:58 PM Andy Shevchenko
> > > wrote:
> > This better to make as a separate helper function
> >
> > static u32 intel_gpio_is_requested(chip, base, size)
> > {
> > u32 requested = 0;
> > unsigned int i;
> >
> > for () {
> > if ()
> > requested |= BIT();
> > }
> > return requested;
> > }
> >
> > (Note u32 as a type)
> >
>
> Thanks. I made a minor modification for the check function. I think to
> pass a padgroup
> as the argument would be better instead of base, size which I may need
> to check if
> the size > 32 (of course it shouldn't happen) or not.
Group size is never bigger than 32 pins.
The helper should be pure GPIO, that's why I still would like to see base
there. Otherwise it would be layering violation.
> +intel_padgroup_has_gpio_requested(struct gpio_chip *chip, const
> struct intel_padgroup *gpp)
Namespace is intel_gpio_ here.
> +{
> + u32 requested = 0;
> + int i;
> +
> + if (gpp == NULL)
> + return 0;
> +
> + if (gpp->gpio_base < 0)
> + return 0;
> +
> + for (i = 0; i < gpp->size; i++)
> + if (gpiochip_is_requested(chip, gpp->gpio_base + i))
> + requested |= BIT(i);
> +
> + return requested;
> +}
> > > + if (requested) {
> > > + if (communities[i].hostown[gpp] !=
> > > readl(base + gpp * 4)) {
> > > +
> > > writel(communities[i].hostown[gpp], base + gpp * 4);
> >
> > The idea here not to check this at all, but rather apply a mask.
> >
> > u32 value;
> >
> > ...
> > value = readl();
> > value = (value & ~requested) | (hostown[gpp] & requested);
> > writel(value);
> >
>
> I made the following per your suggestion. So basically I don't need to show a
> warning for the abnormal HOSTSW_OWN value change? I will submit a formal
> patch for review if there's no big problem for these code logic. Please advise
> if any. Thanks.
You still have all data to produce a warning if it's needed.
((value ^ hostown[gpp]) & requested) will return the changed bits.
> + base = community->regs + community->hostown_offset;
> + for (gpp = 0; gpp < community->ngpps; gpp++) {
> + const struct intel_padgroup *padgrp =
> &community->gpps[i];
> + u32 requested =
> intel_padgroup_has_gpio_requested(&pctrl->chip, padgrp);
> +
> + if (requested) {
You may not need this check at all.
> + u32 value = readl(base + gpp * 4);
> + u32 saved = communities[i].hostown[gpp];
> +
> + value = (value & ~requested) | (saved
> & requested);
> + writel(value, base + gpp * 4);
It's possible to split this as well to another helper function.
static void intel_gpio_update_pad_mode(void __iomem *hostown, u32 mask, u32
value)
{
...
}
> + dev_dbg(dev, "restored hostown %d/%u
> %#08x\n", i, gpp,
> + readl(base + gpp * 4));
> + }
> + }
--
With Best Regards,
Andy Shevchenko
Re: [PATCH] pinctrl: intel: save HOSTSW_OWN register over suspend/resume
On Wed, Apr 3, 2019 at 9:06 PM Andy Shevchenko
wrote:
>
> On Wed, Apr 03, 2019 at 03:06:43PM +0800, Chris Chiu wrote:
> > On Tue, Apr 2, 2019 at 7:58 PM Andy Shevchenko
> > wrote:
>
> > > Instead you may need to loop over each pin in the part of the group
> > > related to
> > > one 32-bit HOSTSW_OWN register (i.e. 8, see PADOWN_*() macros in the
> > > driver),
> > > check if it's requested and break a loop. If loop index is off-by-one a
> > > limit,
> > > nothing to do, otherwise restore hostown register.
> > >
> > > More pedantic approach is to collect the mask inside the loop and apply
> > > it.
> > >
> > > The check function name is gpiochip_is_requested().
> > >
> > > (One of Intel's drivers which is using that at ->resume() is
> > > drivers/gpio/gpio-lynxpoint.c)
> > >
> > > P.S. I prefer pedantic approach. The simplification one is showed in
> > > order to
> > > give you an idea.
>
> > Thanks for your great comment. I remove the useless hostown save function
> > and make the following change in ->resume() to detect and restore the
> > abnormal
> > HOSTSW_OWN. Please help comment if there're still problems. Thanks.
>
>
> This better to make as a separate helper function
>
> static u32 intel_gpio_is_requested(chip, base, size)
> {
> u32 requested = 0;
> unsigned int i;
>
> for () {
> if ()
> requested |= BIT();
> }
> return requested;
> }
>
> (Note u32 as a type)
>
Thanks. I made a minor modification for the check function. I think to
pass a padgroup
as the argument would be better instead of base, size which I may need
to check if
the size > 32 (of course it shouldn't happen) or not.
+intel_padgroup_has_gpio_requested(struct gpio_chip *chip, const
struct intel_padgroup *gpp)
+{
+ u32 requested = 0;
+ int i;
+
+ if (gpp == NULL)
+ return 0;
+
+ if (gpp->gpio_base < 0)
+ return 0;
+
+ for (i = 0; i < gpp->size; i++)
+ if (gpiochip_is_requested(chip, gpp->gpio_base + i))
+ requested |= BIT(i);
+
+ return requested;
+}
+
int intel_pinctrl_resume(struct device *dev)
{
struct platform_device *pdev = to_platform_device(dev);
> > + if (requested) {
> > + if (communities[i].hostown[gpp] !=
> > readl(base + gpp * 4)) {
> > +
> > writel(communities[i].hostown[gpp], base + gpp * 4);
>
> The idea here not to check this at all, but rather apply a mask.
>
> u32 value;
>
> ...
> value = readl();
> value = (value & ~requested) | (hostown[gpp] & requested);
> writel(value);
>
I made the following per your suggestion. So basically I don't need to show a
warning for the abnormal HOSTSW_OWN value change? I will submit a formal
patch for review if there's no big problem for these code logic. Please advise
if any. Thanks.
@@ -1588,6 +1619,22 @@ int intel_pinctrl_resume(struct device *dev)
dev_dbg(dev, "restored mask %d/%u %#08x\n", i, gpp,
readl(base + gpp * 4));
}
+
+ base = community->regs + community->hostown_offset;
+ for (gpp = 0; gpp < community->ngpps; gpp++) {
+ const struct intel_padgroup *padgrp =
&community->gpps[i];
+ u32 requested =
intel_padgroup_has_gpio_requested(&pctrl->chip, padgrp);
+
+ if (requested) {
+ u32 value = readl(base + gpp * 4);
+ u32 saved = communities[i].hostown[gpp];
+
+ value = (value & ~requested) | (saved
& requested);
+ writel(value, base + gpp * 4);
+ dev_dbg(dev, "restored hostown %d/%u
%#08x\n", i, gpp,
+ readl(base + gpp * 4));
+ }
+ }
Re: [PATCH] pinctrl: intel: save HOSTSW_OWN register over suspend/resume
On Wed, Apr 03, 2019 at 03:06:43PM +0800, Chris Chiu wrote:
> On Tue, Apr 2, 2019 at 7:58 PM Andy Shevchenko
> wrote:
> > Instead you may need to loop over each pin in the part of the group related
> > to
> > one 32-bit HOSTSW_OWN register (i.e. 8, see PADOWN_*() macros in the
> > driver),
> > check if it's requested and break a loop. If loop index is off-by-one a
> > limit,
> > nothing to do, otherwise restore hostown register.
> >
> > More pedantic approach is to collect the mask inside the loop and apply it.
> >
> > The check function name is gpiochip_is_requested().
> >
> > (One of Intel's drivers which is using that at ->resume() is
> > drivers/gpio/gpio-lynxpoint.c)
> >
> > P.S. I prefer pedantic approach. The simplification one is showed in order
> > to
> > give you an idea.
> Thanks for your great comment. I remove the useless hostown save function
> and make the following change in ->resume() to detect and restore the abnormal
> HOSTSW_OWN. Please help comment if there're still problems. Thanks.
> + base = community->regs + community->hostown_offset;
> + for (gpp = 0; gpp < community->ngpps; gpp++) {
> + const struct intel_padgroup *padgrp =
> &community->gpps[i];
> + unsigned int requested = 0;
> + int j;
> +
> + if (padgrp->gpio_base < 0)
> + continue;
> +
> + for (j = 0; j < padgrp->size; j++)
> + if
> (gpiochip_is_requested(&pctrl->chip, padgrp->gpio_base + j))
> + requested |= BIT(j);
> +
This better to make as a separate helper function
static u32 intel_gpio_is_requested(chip, base, size)
{
u32 requested = 0;
unsigned int i;
for () {
if ()
requested |= BIT();
}
return requested;
}
(Note u32 as a type)
> + if (requested) {
> + if (communities[i].hostown[gpp] !=
> readl(base + gpp * 4)) {
> +
> writel(communities[i].hostown[gpp], base + gpp * 4);
The idea here not to check this at all, but rather apply a mask.
u32 value;
...
value = readl();
value = (value & ~requested) | (hostown[gpp] & requested);
writel(value);
> + dev_warn(dev, "hostown been
> changed during resume\n");
> + dev_dbg(dev, "restored hostown
> %d/%u %#08x\n", i, gpp,
> + readl(base + gpp * 4));
> + }
> + }
> + }
--
With Best Regards,
Andy Shevchenko
Re: [RFC PATCH 4/4] bus: ti-sysc: Ensure PRU-ICSS doesn't break suspend/resume
On 02/04/2019 19:57, Tony Lindgren wrote:
> * Roger Quadros [190402 13:38]:
>> The PRU-ICSS subsystem's SYSCONFIG register is similar to
>> omap4-simple but has 2 special bits STANDBY_INIT and SUB_MWAIT.
>>
>> The STANDBY_INIT bit initiates a Standby sequence (when set) and
>> triggers a MStandby request to the SoC's PRCM module. This same
>> bit is also used to enable the OCP master ports (when cleared).
>>
>> Some PRU applications require the OCP master port access to be
>> enabled thus keeping it out of standby.
>
> So do we need to configure this depending on the application?
Yes.
>
>> During sustem suspend/resume we must ensure that the PRUSS is in
>> standby else it will break resume.
>>
>> NOTE:
>> 1. This patch only adds the PM callbacks with code to fix the System
>> Suspend/Resume hang issue on AM33xx/AM437x SoCs, but does not
>> implement the full context save and restore required for the PRUSS
>> drivers to work across system suspend/resume when the power domain
>> is switched off (L4PER domain is switched OFF on AM335x/AM437x
>> during system suspend/resume, so PRUSS modules do lose context).
>
> I think we already restore the interconnect target module access
> properly on resume. If not we should fix that.
>
> Saving and restoring the child device state is up to the device
> drivers managing the child device(s), and there's not much ti-sysc.c
> can do about it, right?
Agreed. In that case this handling should be done by pruss.c
and uio_pruss.c in their suspend/resume handlers.
Suman, do you agree?
>
>> @@ -92,6 +93,7 @@ struct sysc {
>> bool enabled;
>> bool needs_resume;
>> bool child_needs_resume;
>> +bool in_standby;
>> struct delayed_work idle_work;
>> };
>
> We should start using bitfields for the bool here, might as well
> already do it now:
>
> unsigned long in_standby:1;
>
> See "17) Using bool" in Documentation/process/coding-style.rst.
>
>> @@ -1023,6 +1025,21 @@ static int __maybe_unused sysc_noirq_suspend(struct
>> device *dev)
>> if (ddata->cfg.quirks & SYSC_QUIRK_LEGACY_IDLE)
>> return 0;
>>
>> +if (ddata->cap->type == TI_SYSC_PRUSS) {
>
> Should this test be made more generic based on the mstandby
> bit being configured?
No other module uses this bit. It is specific to PRUSS.
>
> And can you please make these into separate functions to
> avoid cluttering the suspend and resume functions. Something
> like sysc_handle_mstandby() maybe?
>
>> +u32 reg, mask;
>> +const struct sysc_regbits *regbits = ddata->cap->regbits;
>> +
>> +mask = BIT(regbits->standby_init_shift);
>> +reg = sysc_read(ddata, ddata->offsets[SYSC_SYSCONFIG]);
>> +ddata->in_standby = reg & mask;
>
> Hmm so could we just assume that the device drivers for child device(s)
> configure the MSTANDBY bit? Or do we need to manage it in ti-sysc?
>
I'm in favor of managing it in the child device driver.
Let's see if Suman has any concerns.
> See for example drivers/usb/musb/omap2430.c omap2430_low_level_init()
> and omap2430_low_level_exit(). That's a separate register though.
>
--
cheers,
-roger
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [PATCH] pinctrl: intel: save HOSTSW_OWN register over suspend/resume
On Tue, Apr 2, 2019 at 7:58 PM Andy Shevchenko
wrote:
> > + base = community->regs + community->hostown_offset;
> > + for (gpp = 0; gpp < community->ngpps; gpp++) {
> > + if (communities[i].hostown[gpp] &&
> > + communities[i].hostown[gpp] != readl(base
> > + gpp * 4)) {
> > + writel(communities[i].hostown[gpp],
> > base + gpp * 4);
> > + dev_warn(dev, "hostown changed after
> > resume\n");
> > + dev_dbg(dev, "restored hostown %d/%u
> > %#08x\n", i, gpp,
> > + readl(base + gpp * 4));
> > + }
> > + }
>
> Instead you may need to loop over each pin in the part of the group related to
> one 32-bit HOSTSW_OWN register (i.e. 8, see PADOWN_*() macros in the driver),
> check if it's requested and break a loop. If loop index is off-by-one a limit,
> nothing to do, otherwise restore hostown register.
>
> More pedantic approach is to collect the mask inside the loop and apply it.
>
> The check function name is gpiochip_is_requested().
>
> (One of Intel's drivers which is using that at ->resume() is
> drivers/gpio/gpio-lynxpoint.c)
>
> P.S. I prefer pedantic approach. The simplification one is showed in order to
> give you an idea.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
Thanks for your great comment. I remove the useless hostown save function
and make the following change in ->resume() to detect and restore the abnormal
HOSTSW_OWN. Please help comment if there're still problems. Thanks.
@@ -1588,6 +1600,29 @@ int intel_pinctrl_resume(struct device *dev)
dev_dbg(dev, "restored mask %d/%u %#08x\n", i, gpp,
readl(base + gpp * 4));
}
+
+ base = community->regs + community->hostown_offset;
+ for (gpp = 0; gpp < community->ngpps; gpp++) {
+ const struct intel_padgroup *padgrp =
&community->gpps[i];
+ unsigned int requested = 0;
+ int j;
+
+ if (padgrp->gpio_base < 0)
+ continue;
+
+ for (j = 0; j < padgrp->size; j++)
+ if
(gpiochip_is_requested(&pctrl->chip, padgrp->gpio_base + j))
+ requested |= BIT(j);
+
+ if (requested) {
+ if (communities[i].hostown[gpp] !=
readl(base + gpp * 4)) {
+
writel(communities[i].hostown[gpp], base + gpp * 4);
+ dev_warn(dev, "hostown been
changed during resume\n");
+ dev_dbg(dev, "restored hostown
%d/%u %#08x\n", i, gpp,
+ readl(base + gpp * 4));
+ }
+ }
+ }
}
return 0;
Re: [RFC PATCH 4/4] bus: ti-sysc: Ensure PRU-ICSS doesn't break suspend/resume
* Roger Quadros [190402 13:38]:
> The PRU-ICSS subsystem's SYSCONFIG register is similar to
> omap4-simple but has 2 special bits STANDBY_INIT and SUB_MWAIT.
>
> The STANDBY_INIT bit initiates a Standby sequence (when set) and
> triggers a MStandby request to the SoC's PRCM module. This same
> bit is also used to enable the OCP master ports (when cleared).
>
> Some PRU applications require the OCP master port access to be
> enabled thus keeping it out of standby.
So do we need to configure this depending on the application?
> During sustem suspend/resume we must ensure that the PRUSS is in
> standby else it will break resume.
>
> NOTE:
> 1. This patch only adds the PM callbacks with code to fix the System
> Suspend/Resume hang issue on AM33xx/AM437x SoCs, but does not
> implement the full context save and restore required for the PRUSS
> drivers to work across system suspend/resume when the power domain
> is switched off (L4PER domain is switched OFF on AM335x/AM437x
> during system suspend/resume, so PRUSS modules do lose context).
I think we already restore the interconnect target module access
properly on resume. If not we should fix that.
Saving and restoring the child device state is up to the device
drivers managing the child device(s), and there's not much ti-sysc.c
can do about it, right?
> @@ -92,6 +93,7 @@ struct sysc {
> bool enabled;
> bool needs_resume;
> bool child_needs_resume;
> + bool in_standby;
> struct delayed_work idle_work;
> };
We should start using bitfields for the bool here, might as well
already do it now:
unsigned long in_standby:1;
See "17) Using bool" in Documentation/process/coding-style.rst.
> @@ -1023,6 +1025,21 @@ static int __maybe_unused sysc_noirq_suspend(struct
> device *dev)
> if (ddata->cfg.quirks & SYSC_QUIRK_LEGACY_IDLE)
> return 0;
>
> + if (ddata->cap->type == TI_SYSC_PRUSS) {
Should this test be made more generic based on the mstandby
bit being configured?
And can you please make these into separate functions to
avoid cluttering the suspend and resume functions. Something
like sysc_handle_mstandby() maybe?
> + u32 reg, mask;
> + const struct sysc_regbits *regbits = ddata->cap->regbits;
> +
> + mask = BIT(regbits->standby_init_shift);
> + reg = sysc_read(ddata, ddata->offsets[SYSC_SYSCONFIG]);
> + ddata->in_standby = reg & mask;
Hmm so could we just assume that the device drivers for child device(s)
configure the MSTANDBY bit? Or do we need to manage it in ti-sysc?
See for example drivers/usb/musb/omap2430.c omap2430_low_level_init()
and omap2430_low_level_exit(). That's a separate register though.
Regards,
Tony
[RFC PATCH 4/4] bus: ti-sysc: Ensure PRU-ICSS doesn't break suspend/resume
The PRU-ICSS subsystem's SYSCONFIG register is similar to
omap4-simple but has 2 special bits STANDBY_INIT and SUB_MWAIT.
The STANDBY_INIT bit initiates a Standby sequence (when set) and
triggers a MStandby request to the SoC's PRCM module. This same
bit is also used to enable the OCP master ports (when cleared).
Some PRU applications require the OCP master port access to be
enabled thus keeping it out of standby.
During sustem suspend/resume we must ensure that the PRUSS is in
standby else it will break resume.
NOTE:
1. This patch only adds the PM callbacks with code to fix the System
Suspend/Resume hang issue on AM33xx/AM437x SoCs, but does not
implement the full context save and restore required for the PRUSS
drivers to work across system suspend/resume when the power domain
is switched off (L4PER domain is switched OFF on AM335x/AM437x
during system suspend/resume, so PRUSS modules do lose context).
2. The PRUSS driver functionality on AM57xx SoCs is not affected that
much because the PER power domain to which the PRUSS IPs belong is
not switched OFF during suspend/resume.
Based on work by Suman Anna.
Cc: Suman Anna
Signed-off-by: Roger Quadros
---
drivers/bus/ti-sysc.c | 36
1 file changed, 36 insertions(+)
diff --git a/drivers/bus/ti-sysc.c b/drivers/bus/ti-sysc.c
index e4ab4d422ea5..9c94ce08dd36 100644
--- a/drivers/bus/ti-sysc.c
+++ b/drivers/bus/ti-sysc.c
@@ -71,6 +71,7 @@ static const char * const clock_names[SYSC_MAX_CLOCKS] = {
* @name: name if available
* @revision: interconnect target module revision
* @needs_resume: runtime resume needed on resume from suspend
+ * @in_standby: flag used by PRUSS type during suspend/resume
*/
struct sysc {
struct device *dev;
@@ -92,6 +93,7 @@ struct sysc {
bool enabled;
bool needs_resume;
bool child_needs_resume;
+ bool in_standby;
struct delayed_work idle_work;
};
@@ -1023,6 +1025,21 @@ static int __maybe_unused sysc_noirq_suspend(struct
device *dev)
if (ddata->cfg.quirks & SYSC_QUIRK_LEGACY_IDLE)
return 0;
+ if (ddata->cap->type == TI_SYSC_PRUSS) {
+ u32 reg, mask;
+ const struct sysc_regbits *regbits = ddata->cap->regbits;
+
+ mask = BIT(regbits->standby_init_shift);
+ reg = sysc_read(ddata, ddata->offsets[SYSC_SYSCONFIG]);
+ ddata->in_standby = reg & mask;
+
+ /* initiate MStandby */
+ if (!ddata->in_standby) {
+ reg |= mask;
+ sysc_write(ddata, ddata->offsets[SYSC_SYSCONFIG], reg);
+ }
+ }
+
return pm_runtime_force_suspend(dev);
}
@@ -1035,6 +1052,25 @@ static int __maybe_unused sysc_noirq_resume(struct
device *dev)
if (ddata->cfg.quirks & SYSC_QUIRK_LEGACY_IDLE)
return 0;
+ if (ddata->cap->type == TI_SYSC_PRUSS && !ddata->in_standby) {
+ u32 reg;
+ const struct sysc_regbits *regbits = ddata->cap->regbits;
+
+ /* re-enable OCP master ports/disable MStandby */
+ reg = sysc_read(ddata, ddata->offsets[SYSC_SYSCONFIG]);
+ reg &= ~BIT(regbits->standby_init_shift);
+ sysc_write(ddata, ddata->offsets[SYSC_SYSCONFIG], reg);
+ ddata->in_standby = 0;
+
+ /* wait till ready for transactions - delay is arbitrary */
+ usleep_range(50, 100);
+ reg = sysc_read(ddata, ddata->offsets[SYSC_SYSCONFIG]);
+ if (reg & BIT(regbits->sub_mwait_shift)) {
+ dev_err(dev, "timeout waiting for SUB_MWAIT_READY\n");
+ return -ETIMEDOUT;
+ }
+ }
+
return pm_runtime_force_resume(dev);
}
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [PATCH] pinctrl: intel: save HOSTSW_OWN register over suspend/resume
On Tue, Apr 02, 2019 at 02:16:19PM +0800, Chris Chiu wrote:
> On Mon, Apr 1, 2019 at 8:23 PM Andy Shevchenko
> wrote:
> > On Mon, Apr 01, 2019 at 06:41:57PM +0800, Chris Chiu wrote:
> Thanks for the comment. My first version did mimic the logic of the interrupt
> mask restore but it was based on the DMI quirk. It saves HOSTSW_OWN
> for each padgroup and restores them all after resume if DMI info matched.
>
> What really confused me is how to do this specifically for a requested GPIO
> pin. So here's my new proposed patch. Please suggests if there's any better
> idea. Thanks.
> struct intel_community_context {
> u32 *intmask;
> + u32 *hostown;
This is okay.
> };
> +#ifdef CONFIG_PM_SLEEP
> +static void intel_save_hostown(struct intel_pinctrl *pctrl, unsigned int
> pin);
> +#endif
> +
No need for this...
> /* Disable TX buffer and enable RX (this will be input) */
> __intel_gpio_set_direction(padcfg0, true);
> +#ifdef CONFIG_PM_SLEEP
> + intel_save_hostown(pctrl, pin);
> +#endif
...and for this.
Just save all of them at ->suspend()
> for (i = 0; i < pctrl->ncommunities; i++) {
> struct intel_community *community = &pctrl->communities[i];
> - u32 *intmask;
> + u32 *intmask, *hostown;
>
> intmask = devm_kcalloc(pctrl->dev, community->ngpps,
>sizeof(*intmask), GFP_KERNEL);
> @@ -1292,6 +1299,13 @@ static int intel_pinctrl_pm_init(struct
> intel_pinctrl *pctrl)
> return -ENOMEM;
>
> communities[i].intmask = intmask;
> +
> + hostown = devm_kcalloc(pctrl->dev, community->ngpps,
> + sizeof(*hostown), GFP_KERNEL);
> + if (!hostown)
> + return -ENOMEM;
> +
> + communities[i].hostown= hostown;
This is good.
> }
> +static void intel_save_hostown(struct intel_pinctrl *pctrl, unsigned int pin)
> +{
> + const struct intel_community *community;
> + const struct intel_padgroup *padgrp;
> + int i;
> +
> + community = intel_get_community(pctrl, pin);
> + if (!community)
> + return;
> + if (!community->hostown_offset)
> + return;
> +
> + padgrp = intel_community_get_padgroup(community, pin);
> + if (!padgrp)
> + return;
> +
> + for (i = 0; i < pctrl->ncommunities; i++) {
> + const struct intel_community *comm = &pctrl->communities[i];
> + int j;
> +
> + for (j = 0; j < comm->ngpps; j++) {
> + const struct intel_padgroup *pgrp = &comm->gpps[j];
> +
> + if (padgrp == pgrp) {
> + struct intel_community_context *communities;
> + void __iomem *base;
> +
> + communities = pctrl->context.communities;
> + base = community->regs +
> community->hostown_offset;
> + communities[i].hostown[j] = readl(base + j *
> 4);
> + break;
> + }
> + }
> + }
> + return;
Useless.
> +}
This is too complicated. Just add
base = community->regs + community->hostown_offset;
for (gpp = 0; gpp < community->ngpps; gpp++)
communities[i].hostown[gpp] = readl(base + gpp * 4);
into ->suspend() loop.
> + base = community->regs + community->hostown_offset;
> + for (gpp = 0; gpp < community->ngpps; gpp++) {
> + if (communities[i].hostown[gpp] &&
> + communities[i].hostown[gpp] != readl(base
> + gpp * 4)) {
> + writel(communities[i].hostown[gpp],
> base + gpp * 4);
> + dev_warn(dev, "hostown changed after
> resume\n");
> + dev_dbg(dev, "restored hostown %d/%u
> %#08x\n", i, gpp,
> + readl(base + gpp * 4));
> + }
> + }
Instead you may need to loop over each pin in the part of the group related to
one 32-bit HOSTSW_OWN register (i.e. 8, see PADOWN_*() macros in the driver),
check if it's requested and break a loop. If loop index is off-by-one a limit,
nothing to do, otherwise restore hostown register.
More pedantic approach is to collect the mask inside the loop and apply it.
The check function name is gpiochip_is_requested().
(One of Intel's drivers which is using that at ->resume() is
drivers/gpio/gpio-lynxpoint.c)
P.S. I prefer pedantic approach. The simplification one is showed in order to
give you an idea.
--
With Best Regards,
Andy Shevchenko
Re: [PATCH] pinctrl: intel: save HOSTSW_OWN register over suspend/resume
On Mon, Apr 1, 2019 at 8:23 PM Andy Shevchenko
wrote:
>
> On Mon, Apr 01, 2019 at 06:41:57PM +0800, Chris Chiu wrote:
>
> Thanks for the patch.
> My comments below.
>
> > diff --git a/drivers/pinctrl/intel/pinctrl-intel.c
> > b/drivers/pinctrl/intel/pinctrl-intel.c
> > index 8cda7b535b02..d1cfa5adef9b 100644
> > --- a/drivers/pinctrl/intel/pinctrl-intel.c
> > +++ b/drivers/pinctrl/intel/pinctrl-intel.c
> > @@ -77,6 +77,7 @@ struct intel_pad_context {
> > u32 padcfg0;
> > u32 padcfg1;
> > u32 padcfg2;
>
> > + u32 hostown;
>
> This is wrong. We have one register per entire (*) group of pins to keep host
> ownership. Basically it's a mask.
>
> *) if it's <= 32, otherwise there are more registers. But in any case it's 1
> bit per pin, and not 32 bits.
>
> > for (i = 0; i < pctrl->soc->npins; i++)
>
> Thus, the actual actions should mimic what we do for interrupt mask.
>
> --
> With Best Regards,
> Andy Shevchenko
>
Thanks for the comment. My first version did mimic the logic of the interrupt
mask restore but it was based on the DMI quirk. It saves HOSTSW_OWN
for each padgroup and restores them all after resume if DMI info matched.
What really confused me is how to do this specifically for a requested GPIO
pin. So here's my new proposed patch. Please suggests if there's any better
idea. Thanks.
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -81,6 +81,7 @@ struct intel_pad_context {
struct intel_community_context {
u32 *intmask;
+ u32 *hostown;
};
struct intel_pinctrl_context {
@@ -117,6 +118,10 @@ struct intel_pinctrl {
#define pin_to_padno(c, p) ((p) - (c)->pin_base)
#define padgroup_offset(g, p) ((p) - (g)->base)
+#ifdef CONFIG_PM_SLEEP
+static void intel_save_hostown(struct intel_pinctrl *pctrl, unsigned int pin);
+#endif
+
static struct intel_community *intel_get_community(struct intel_pinctrl *pctrl,
unsigned int pin)
{
@@ -456,7 +461,9 @@ static int intel_gpio_request_enable(struct
pinctrl_dev *pctldev,
intel_gpio_set_gpio_mode(padcfg0);
/* Disable TX buffer and enable RX (this will be input) */
__intel_gpio_set_direction(padcfg0, true);
+#ifdef CONFIG_PM_SLEEP
+ intel_save_hostown(pctrl, pin);
+#endif
raw_spin_unlock_irqrestore(&pctrl->lock, flags);
return 0;
@@ -1284,7 +1291,7 @@ static int intel_pinctrl_pm_init(struct
intel_pinctrl *pctrl)
for (i = 0; i < pctrl->ncommunities; i++) {
struct intel_community *community = &pctrl->communities[i];
- u32 *intmask;
+ u32 *intmask, *hostown;
intmask = devm_kcalloc(pctrl->dev, community->ngpps,
sizeof(*intmask), GFP_KERNEL);
@@ -1292,6 +1299,13 @@ static int intel_pinctrl_pm_init(struct
intel_pinctrl *pctrl)
return -ENOMEM;
communities[i].intmask = intmask;
+
+ hostown = devm_kcalloc(pctrl->dev, community->ngpps,
+ sizeof(*hostown), GFP_KERNEL);
+ if (!hostown)
+ return -ENOMEM;
+
+ communities[i].hostown= hostown;
}
pctrl->context.pads = pads;
@@ -1447,6 +1461,43 @@ int intel_pinctrl_probe_by_uid(struct
platform_device *pdev)
EXPORT_SYMBOL_GPL(intel_pinctrl_probe_by_uid);
#ifdef CONFIG_PM_SLEEP
+static void intel_save_hostown(struct intel_pinctrl *pctrl, unsigned int pin)
+{
+ const struct intel_community *community;
+ const struct intel_padgroup *padgrp;
+ int i;
+
+ community = intel_get_community(pctrl, pin);
+ if (!community)
+ return;
+ if (!community->hostown_offset)
+ return;
+
+ padgrp = intel_community_get_padgroup(community, pin);
+ if (!padgrp)
+ return;
+
+ for (i = 0; i < pctrl->ncommunities; i++) {
+ const struct intel_community *comm = &pctrl->communities[i];
+ int j;
+
+ for (j = 0; j < comm->ngpps; j++) {
+ const struct intel_padgroup *pgrp = &comm->gpps[j];
+
+ if (padgrp == pgrp) {
+ struct intel_community_context *communities;
+ void __iomem *base;
+
+ communities = pctrl->context.communities;
+ base = community->regs +
community->hostown_offset;
+ communities[i].hostown[j] = readl(base + j * 4);
+ break;
+ }
+ }
+ }
+ return;
+}
+
static bool intel_pinctrl_should_save(struct intel_pinctrl *pctrl,
unsigned int pin)
{
const struct pin_desc *pd = pin_desc_get(pctrl->pctldev, pin);
@@ -1588,6 +1639,17 @@ int intel_pinctrl_resume(struct device *dev)
[PATCH 4.4 024/131] extcon: usb-gpio: Dont miss event during suspend/resume
4.4-stable review patch. If anyone has any objections, please let me know. -- From: Roger Quadros commit 04c080080855ce84dcd490a2e04805608a21085d upstream. Pin state might have changed during suspend/resume while our interrupts were disabled and if device doesn't support wakeup. Scan for change during resume for such case. Signed-off-by: Roger Quadros Signed-off-by: Chanwoo Choi Signed-off-by: Arnd Bergmann Signed-off-by: Greg Kroah-Hartman --- drivers/extcon/extcon-usb-gpio.c |3 +++ 1 file changed, 3 insertions(+) --- a/drivers/extcon/extcon-usb-gpio.c +++ b/drivers/extcon/extcon-usb-gpio.c @@ -192,6 +192,9 @@ static int usb_extcon_resume(struct devi } enable_irq(info->id_irq); + if (!device_may_wakeup(dev)) + queue_delayed_work(system_power_efficient_wq, + &info->wq_detcable, 0); return ret; }
[PATCH 4.4 020/131] usb: dwc3: gadget: Fix suspend/resume during device mode
4.4-stable review patch. If anyone has any objections, please let me know.
--
From: Roger Quadros
commit 9772b47a4c2916d645c551228b6085ea24acbe5d upstream.
Gadget controller might not be always active during system
suspend/resume as gadget driver might not have yet been loaded or
might have been unloaded prior to system suspend.
Check if we're active and only then perform
necessary actions during suspend/resume.
Signed-off-by: Roger Quadros
Signed-off-by: Felipe Balbi
Signed-off-by: Arnd Bergmann
Signed-off-by: Greg Kroah-Hartman
---
drivers/usb/dwc3/gadget.c |6 ++
1 file changed, 6 insertions(+)
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2894,6 +2894,9 @@ void dwc3_gadget_exit(struct dwc3 *dwc)
int dwc3_gadget_suspend(struct dwc3 *dwc)
{
+ if (!dwc->gadget_driver)
+ return 0;
+
if (dwc->pullups_connected) {
dwc3_gadget_disable_irq(dwc);
dwc3_gadget_run_stop(dwc, true, true);
@@ -2912,6 +2915,9 @@ int dwc3_gadget_resume(struct dwc3 *dwc)
struct dwc3_ep *dep;
int ret;
+ if (!dwc->gadget_driver)
+ return 0;
+
/* Start with SuperSpeed Default */
dwc3_gadget_ep0_desc.wMaxPacketSize = cpu_to_le16(512);
[PATCH 4.4 013/131] ALSA: hda - Record the current power state before suspend/resume calls
4.4-stable review patch. If anyone has any objections, please let me know.
--
From: Takashi Iwai
commit 98081ca62cbac31fb0f7efaf90b2e7384ce22257 upstream.
Currently we deal with single codec and suspend codec callbacks for
all S3, S4 and runtime PM handling. But it turned out that we want
distinguish the call patterns sometimes, e.g. for applying some init
sequence only at probing and restoring from hibernate.
This patch slightly modifies the common PM callbacks for HD-audio
codec and stores the currently processed PM event in power_state of
the codec's device.power field, which is currently unused. The codec
callback can take a look at this event value and judges which purpose
it's being called.
Signed-off-by: Takashi Iwai
Signed-off-by: Greg Kroah-Hartman
---
sound/pci/hda/hda_codec.c | 43 +--
1 file changed, 41 insertions(+), 2 deletions(-)
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -3004,6 +3004,7 @@ static void hda_call_codec_resume(struct
hda_jackpoll_work(&codec->jackpoll_work.work);
else
snd_hda_jack_report_sync(codec);
+ codec->core.dev.power.power_state = PMSG_ON;
atomic_dec(&codec->core.in_pm);
}
@@ -3036,10 +3037,48 @@ static int hda_codec_runtime_resume(stru
}
#endif /* CONFIG_PM */
+#ifdef CONFIG_PM_SLEEP
+static int hda_codec_pm_suspend(struct device *dev)
+{
+ dev->power.power_state = PMSG_SUSPEND;
+ return pm_runtime_force_suspend(dev);
+}
+
+static int hda_codec_pm_resume(struct device *dev)
+{
+ dev->power.power_state = PMSG_RESUME;
+ return pm_runtime_force_resume(dev);
+}
+
+static int hda_codec_pm_freeze(struct device *dev)
+{
+ dev->power.power_state = PMSG_FREEZE;
+ return pm_runtime_force_suspend(dev);
+}
+
+static int hda_codec_pm_thaw(struct device *dev)
+{
+ dev->power.power_state = PMSG_THAW;
+ return pm_runtime_force_resume(dev);
+}
+
+static int hda_codec_pm_restore(struct device *dev)
+{
+ dev->power.power_state = PMSG_RESTORE;
+ return pm_runtime_force_resume(dev);
+}
+#endif /* CONFIG_PM_SLEEP */
+
/* referred in hda_bind.c */
const struct dev_pm_ops hda_codec_driver_pm = {
- SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
- pm_runtime_force_resume)
+#ifdef CONFIG_PM_SLEEP
+ .suspend = hda_codec_pm_suspend,
+ .resume = hda_codec_pm_resume,
+ .freeze = hda_codec_pm_freeze,
+ .thaw = hda_codec_pm_thaw,
+ .poweroff = hda_codec_pm_suspend,
+ .restore = hda_codec_pm_restore,
+#endif /* CONFIG_PM_SLEEP */
SET_RUNTIME_PM_OPS(hda_codec_runtime_suspend, hda_codec_runtime_resume,
NULL)
};
Re: [PATCH] pinctrl: intel: save HOSTSW_OWN register over suspend/resume
On Mon, Apr 01, 2019 at 06:41:57PM +0800, Chris Chiu wrote:
> On Mon, Apr 1, 2019 at 3:49 PM Mika Westerberg
> wrote:
> > On Fri, Mar 29, 2019 at 04:38:20PM +0800, Chris Chiu wrote:
> > Sure I can but it probably does not happen until end of the week because
> > I'm currently busy with something else.
>
> Thanks for your attention. I don't want to distract you so I'll try to
> refine the
> patch. It would be a great help if you can help review and give comments.
>
> Don't know whether if the following patch still get the wrong idea about
> your thought. It saves the hostsw_own when GPIO requested, check
> if the value differs in resume() and restore if necessary. Please kindly
> correct me if any. Thanks
Thanks for the patch.
My comments below.
> diff --git a/drivers/pinctrl/intel/pinctrl-intel.c
> b/drivers/pinctrl/intel/pinctrl-intel.c
> index 8cda7b535b02..d1cfa5adef9b 100644
> --- a/drivers/pinctrl/intel/pinctrl-intel.c
> +++ b/drivers/pinctrl/intel/pinctrl-intel.c
> @@ -77,6 +77,7 @@ struct intel_pad_context {
> u32 padcfg0;
> u32 padcfg1;
> u32 padcfg2;
> + u32 hostown;
This is wrong. We have one register per entire (*) group of pins to keep host
ownership. Basically it's a mask.
*) if it's <= 32, otherwise there are more registers. But in any case it's 1
bit per pin, and not 32 bits.
> for (i = 0; i < pctrl->soc->npins; i++)
Thus, the actual actions should mimic what we do for interrupt mask.
--
With Best Regards,
Andy Shevchenko
Re: [PATCH] pinctrl: intel: save HOSTSW_OWN register over suspend/resume
On Mon, Apr 1, 2019 at 3:49 PM Mika Westerberg
wrote:
>
> On Fri, Mar 29, 2019 at 04:38:20PM +0800, Chris Chiu wrote:
> > On Thu, Mar 28, 2019 at 8:34 PM Mika Westerberg
> > wrote:
> > >
> > > On Thu, Mar 28, 2019 at 08:19:59PM +0800, Chris Chiu wrote:
> > > > On Thu, Mar 28, 2019 at 5:38 PM Daniel Drake wrote:
> > > > >
> > > > > On Thu, Mar 28, 2019 at 5:17 PM Andy Shevchenko
> > > > > wrote:
> > > > > > Hmm... Can you confirm that laptop you declared as a fixed case and
> > > > > > the
> > > > > > mentioned here is the same one?
> > > > >
> > > > > They are definitely not the same exact unit - originally we had a
> > > > > pre-production sample, and now we briefly diagnosed a real production
> > > > > unit that was sold to a customer. There could be subtle motherboard
> > > > > variations as you mention.
> > > > >
> > > > > > If it's the case, I recommend to ping Asus again and make them
> > > > > > check and fix.
> > > > >
> > > > > We'll keep an eye open for any opportunities to go deeper here.
> > > > > However further investigation on both our side and theirs is blocked
> > > > > by not having any of the affected hardware (since the models are now
> > > > > so old), so I'm not very optimistic that we'll be able to make
> > > > > progress there.
> > > > >
> > > > > > Meanwhile, Mika's proposal sounds feasible and not so intrusive. We
> > > > > > may
> > > > > > implement this later on.
> > > > >
> > > > > Chris will work on implementing this for your consideration.
> > > > >
> > > > > Thanks for the quick feedback!
> > > > > Daniel
> > > >
> > > > What if I modify the patch as follows? It doesn't save HOSTSW_OWN
> > > > register.
> > > > It just toggles the bit specifically for the IRQ GPIO pin after resume
> > > > when DMI
> > > > matches.
> > >
> > > I don't really like having quirks like this if we can avoid it and in
> > > this case I think we can. Just always save HOSTSW_OWN and then restore
> > > it if there is a GPIO requested and the value differs (and log a warning
> > > or something like that).
> >
> > You mean save the content of hostsw_own register on padgroup based ex.
> > communities[i].hostown[gpp] = readl(base + gpp * 4);
> >
> > And then check the hostown bit for the GPIO requested pin in
> > intel_pinctrl_resume(),
> > differs the hostsw_own bit on pin base (like padcfg), then restore the
> > hostsw_own
> > value of the padgroug which the GPIO pin is belonging to?
>
> Yes.
>
> > I think what you mean should be a much more straightforward solution
> > for this. Could
> > you implement this in your way and we can try to help verification. Thanks.
>
> Sure I can but it probably does not happen until end of the week because
> I'm currently busy with something else.
Thanks for your attention. I don't want to distract you so I'll try to
refine the
patch. It would be a great help if you can help review and give comments.
Don't know whether if the following patch still get the wrong idea about
your thought. It saves the hostsw_own when GPIO requested, check
if the value differs in resume() and restore if necessary. Please kindly
correct me if any. Thanks
diff --git a/drivers/pinctrl/intel/pinctrl-intel.c
b/drivers/pinctrl/intel/pinctrl-intel.c
index 8cda7b535b02..d1cfa5adef9b 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -77,6 +77,7 @@ struct intel_pad_context {
u32 padcfg0;
u32 padcfg1;
u32 padcfg2;
+ u32 hostown;
};
struct intel_community_context {
@@ -219,6 +220,24 @@ static bool intel_pad_acpi_mode(struct
intel_pinctrl *pctrl, unsigned int pin)
return !(readl(hostown) & BIT(gpp_offset));
}
+static void __iomem *intel_get_hostown(struct intel_pinctrl *pctrl,
unsigned int pin)
+{
+ const struct intel_community *community;
+ const struct intel_padgroup *padgrp;
+
+ community = intel_get_community(pctrl, pin);
+ if (!community)
+ return NULL;
+ if (!community->hostown_offset)
+ return NULL;
+
+ padgrp = intel_community_get_padgroup(community, pin);
+ if (!padgrp)
+ return NULL;
+
+ return community->regs + community->hostown_offset +
padgrp->reg_num * 4;
+}
+
static bool intel_pad_locked(struct intel_pinctrl *pctrl, unsigned int pin)
{
struct intel_community *community;
@@ -442,7 +461,7 @@ static int intel_gpio_request_enable(struct
pinctrl_dev *pctldev,
unsigned int pin)
{
struct intel_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
- void __iomem *padcfg0;
+ void __iomem *padcfg0, *hostown;
unsigned long flags;
raw_spin_lock_irqsave(&pctrl->lock, flags);
@@ -457,6 +476,10 @@ static int intel_gpio_request_enable(struct
pinctrl_dev *pctldev,
/* Disable TX buffer and enable RX (this will be input) */
__intel_gpio_set_direction(padcfg0, true);
+ /* Save HOSTSW_OWN */
+ hostown =
Re: [PATCH] pinctrl: intel: save HOSTSW_OWN register over suspend/resume
On Fri, Mar 29, 2019 at 04:38:20PM +0800, Chris Chiu wrote: > On Thu, Mar 28, 2019 at 8:34 PM Mika Westerberg > wrote: > > > > On Thu, Mar 28, 2019 at 08:19:59PM +0800, Chris Chiu wrote: > > > On Thu, Mar 28, 2019 at 5:38 PM Daniel Drake wrote: > > > > > > > > On Thu, Mar 28, 2019 at 5:17 PM Andy Shevchenko > > > > wrote: > > > > > Hmm... Can you confirm that laptop you declared as a fixed case and > > > > > the > > > > > mentioned here is the same one? > > > > > > > > They are definitely not the same exact unit - originally we had a > > > > pre-production sample, and now we briefly diagnosed a real production > > > > unit that was sold to a customer. There could be subtle motherboard > > > > variations as you mention. > > > > > > > > > If it's the case, I recommend to ping Asus again and make them check > > > > > and fix. > > > > > > > > We'll keep an eye open for any opportunities to go deeper here. > > > > However further investigation on both our side and theirs is blocked > > > > by not having any of the affected hardware (since the models are now > > > > so old), so I'm not very optimistic that we'll be able to make > > > > progress there. > > > > > > > > > Meanwhile, Mika's proposal sounds feasible and not so intrusive. We > > > > > may > > > > > implement this later on. > > > > > > > > Chris will work on implementing this for your consideration. > > > > > > > > Thanks for the quick feedback! > > > > Daniel > > > > > > What if I modify the patch as follows? It doesn't save HOSTSW_OWN > > > register. > > > It just toggles the bit specifically for the IRQ GPIO pin after resume > > > when DMI > > > matches. > > > > I don't really like having quirks like this if we can avoid it and in > > this case I think we can. Just always save HOSTSW_OWN and then restore > > it if there is a GPIO requested and the value differs (and log a warning > > or something like that). > > You mean save the content of hostsw_own register on padgroup based ex. > communities[i].hostown[gpp] = readl(base + gpp * 4); > > And then check the hostown bit for the GPIO requested pin in > intel_pinctrl_resume(), > differs the hostsw_own bit on pin base (like padcfg), then restore the > hostsw_own > value of the padgroug which the GPIO pin is belonging to? Yes. > I think what you mean should be a much more straightforward solution > for this. Could > you implement this in your way and we can try to help verification. Thanks. Sure I can but it probably does not happen until end of the week because I'm currently busy with something else.
Re: [PATCH net-next v3 0/2] net: phy: marvell10g: implement suspend/resume callbacks
From: Antoine Tenart Date: Tue, 26 Mar 2019 15:53:00 +0100 > This series implements the suspend/resume callbacks in the > marvell10g PHY driver. Heiner gave you feedback, you have not responded. Therefore I am removing your patches from my queue.
Re: [PATCH] pinctrl: intel: save HOSTSW_OWN register over suspend/resume
On Thu, Mar 28, 2019 at 8:34 PM Mika Westerberg wrote: > > On Thu, Mar 28, 2019 at 08:19:59PM +0800, Chris Chiu wrote: > > On Thu, Mar 28, 2019 at 5:38 PM Daniel Drake wrote: > > > > > > On Thu, Mar 28, 2019 at 5:17 PM Andy Shevchenko > > > wrote: > > > > Hmm... Can you confirm that laptop you declared as a fixed case and the > > > > mentioned here is the same one? > > > > > > They are definitely not the same exact unit - originally we had a > > > pre-production sample, and now we briefly diagnosed a real production > > > unit that was sold to a customer. There could be subtle motherboard > > > variations as you mention. > > > > > > > If it's the case, I recommend to ping Asus again and make them check > > > > and fix. > > > > > > We'll keep an eye open for any opportunities to go deeper here. > > > However further investigation on both our side and theirs is blocked > > > by not having any of the affected hardware (since the models are now > > > so old), so I'm not very optimistic that we'll be able to make > > > progress there. > > > > > > > Meanwhile, Mika's proposal sounds feasible and not so intrusive. We may > > > > implement this later on. > > > > > > Chris will work on implementing this for your consideration. > > > > > > Thanks for the quick feedback! > > > Daniel > > > > What if I modify the patch as follows? It doesn't save HOSTSW_OWN register. > > It just toggles the bit specifically for the IRQ GPIO pin after resume when > > DMI > > matches. > > I don't really like having quirks like this if we can avoid it and in > this case I think we can. Just always save HOSTSW_OWN and then restore > it if there is a GPIO requested and the value differs (and log a warning > or something like that). You mean save the content of hostsw_own register on padgroup based ex. communities[i].hostown[gpp] = readl(base + gpp * 4); And then check the hostown bit for the GPIO requested pin in intel_pinctrl_resume(), differs the hostsw_own bit on pin base (like padcfg), then restore the hostsw_own value of the padgroug which the GPIO pin is belonging to? I think what you mean should be a much more straightforward solution for this. Could you implement this in your way and we can try to help verification. Thanks. Chris
Re: [PATCH] pinctrl: intel: save HOSTSW_OWN register over suspend/resume
On Thu, Mar 28, 2019 at 08:19:59PM +0800, Chris Chiu wrote: > On Thu, Mar 28, 2019 at 5:38 PM Daniel Drake wrote: > > > > On Thu, Mar 28, 2019 at 5:17 PM Andy Shevchenko > > wrote: > > > Hmm... Can you confirm that laptop you declared as a fixed case and the > > > mentioned here is the same one? > > > > They are definitely not the same exact unit - originally we had a > > pre-production sample, and now we briefly diagnosed a real production > > unit that was sold to a customer. There could be subtle motherboard > > variations as you mention. > > > > > If it's the case, I recommend to ping Asus again and make them check and > > > fix. > > > > We'll keep an eye open for any opportunities to go deeper here. > > However further investigation on both our side and theirs is blocked > > by not having any of the affected hardware (since the models are now > > so old), so I'm not very optimistic that we'll be able to make > > progress there. > > > > > Meanwhile, Mika's proposal sounds feasible and not so intrusive. We may > > > implement this later on. > > > > Chris will work on implementing this for your consideration. > > > > Thanks for the quick feedback! > > Daniel > > What if I modify the patch as follows? It doesn't save HOSTSW_OWN register. > It just toggles the bit specifically for the IRQ GPIO pin after resume when > DMI > matches. I don't really like having quirks like this if we can avoid it and in this case I think we can. Just always save HOSTSW_OWN and then restore it if there is a GPIO requested and the value differs (and log a warning or something like that).
Re: [PATCH] pinctrl: intel: save HOSTSW_OWN register over suspend/resume
On Thu, Mar 28, 2019 at 5:38 PM Daniel Drake wrote:
>
> On Thu, Mar 28, 2019 at 5:17 PM Andy Shevchenko
> wrote:
> > Hmm... Can you confirm that laptop you declared as a fixed case and the
> > mentioned here is the same one?
>
> They are definitely not the same exact unit - originally we had a
> pre-production sample, and now we briefly diagnosed a real production
> unit that was sold to a customer. There could be subtle motherboard
> variations as you mention.
>
> > If it's the case, I recommend to ping Asus again and make them check and
> > fix.
>
> We'll keep an eye open for any opportunities to go deeper here.
> However further investigation on both our side and theirs is blocked
> by not having any of the affected hardware (since the models are now
> so old), so I'm not very optimistic that we'll be able to make
> progress there.
>
> > Meanwhile, Mika's proposal sounds feasible and not so intrusive. We may
> > implement this later on.
>
> Chris will work on implementing this for your consideration.
>
> Thanks for the quick feedback!
> Daniel
What if I modify the patch as follows? It doesn't save HOSTSW_OWN register.
It just toggles the bit specifically for the IRQ GPIO pin after resume when DMI
matches.
---
drivers/pinctrl/intel/pinctrl-intel.c | 67 +++
1 file changed, 67 insertions(+)
diff --git a/drivers/pinctrl/intel/pinctrl-intel.c
b/drivers/pinctrl/intel/pinctrl-intel.c
index 8cda7b535b02..994abc5ecd32 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -19,6 +19,7 @@
#include
#include
#include
+#include
#include "../core.h"
#include "pinctrl-intel.h"
@@ -73,6 +74,8 @@
#define DEBOUNCE_PERIOD31250 /* ns */
+#define PINCTRL_QUIRK_KEEP_HOSTOWN BIT(0)
+
struct intel_pad_context {
u32 padcfg0;
u32 padcfg1;
@@ -112,11 +115,37 @@ struct intel_pinctrl {
size_t ncommunities;
struct intel_pinctrl_context context;
int irq;
+ u32 quirks;
};
#define pin_to_padno(c, p) ((p) - (c)->pin_base)
#define padgroup_offset(g, p) ((p) - (g)->base)
+static const struct dmi_system_id dmi_retain_hostown_table[] = {
+ {
+ .ident = "ASUSTeK COMPUTER INC. ASUS E403NA",
+ .matches = {
+ DMI_MATCH(DMI_BOARD_VENDOR, "ASUSTeK COMPUTER INC."),
+ DMI_MATCH(DMI_BOARD_NAME, "E403NA"),
+ },
+ },
+ {
+ .ident = "ASUSTeK COMPUTER INC. ASUS X540NA",
+ .matches = {
+ DMI_MATCH(DMI_BOARD_VENDOR, "ASUSTeK COMPUTER INC."),
+ DMI_MATCH(DMI_BOARD_NAME, "X540NA"),
+ },
+ },
+ {
+ .ident = "ASUSTeK COMPUTER INC. ASUS X541NA",
+ .matches = {
+ DMI_MATCH(DMI_BOARD_VENDOR, "ASUSTeK COMPUTER INC."),
+ DMI_MATCH(DMI_BOARD_NAME, "X541NA"),
+ },
+ },
+ { }
+};
+
static struct intel_community *intel_get_community(struct intel_pinctrl *pctrl,
unsigned int pin)
{
@@ -219,6 +248,32 @@ static bool intel_pad_acpi_mode(struct
intel_pinctrl *pctrl, unsigned int pin)
return !(readl(hostown) & BIT(gpp_offset));
}
+static void intel_pad_force_hostown(struct intel_pinctrl *pctrl,
unsigned int pin)
+{
+ const struct intel_community *community;
+ const struct intel_padgroup *padgrp;
+ unsigned int offset, gpp_offset;
+ void __iomem *hostown;
+
+ community = intel_get_community(pctrl, pin);
+ if (!community)
+ return;
+ if (!community->hostown_offset)
+ return;
+
+ padgrp = intel_community_get_padgroup(community, pin);
+ if (!padgrp)
+ return;
+
+ gpp_offset = padgroup_offset(padgrp, pin);
+ offset = community->hostown_offset + padgrp->reg_num * 4;
+ hostown = community->regs + offset;
+
+ writel(readl(hostown) | BIT(gpp_offset), hostown);
+
+ return;
+}
+
static bool intel_pad_locked(struct intel_pinctrl *pctrl, unsigned int pin)
{
struct intel_community *community;
@@ -1318,6 +1373,11 @@ int intel_pinctrl_probe(struct platform_device *pdev,
pctrl->soc = soc_data;
raw_spin_lock_init(&pctrl->lock);
+ if (dmi_first_match(dmi_retain_hostown_table)) {
+ pctrl->quirks |= PINCTRL_QUIRK_KEEP_HOSTOWN;
+ dev_info(&pdev->dev, "enabling KEEP_HOSTOWN quirk on
this hw\n");
+ }
+
/*
* Make a copy of the communities which we can use to hold pointers
* to the registers.
@@ -1549,6 +1609,13 @@ int intel_pinctrl_resume(struct device *dev)
if (!intel_pinctrl_should_save(pctrl, desc->number))
continue;
+ if ((pctrl->quirks & PINCTRL_QUIRK_KEEP_HOSTOWN) &&
+ intel_pad_ac
Re: [PATCH] pinctrl: intel: save HOSTSW_OWN register over suspend/resume
On Thu, Mar 28, 2019 at 5:17 PM Andy Shevchenko wrote: > Hmm... Can you confirm that laptop you declared as a fixed case and the > mentioned here is the same one? They are definitely not the same exact unit - originally we had a pre-production sample, and now we briefly diagnosed a real production unit that was sold to a customer. There could be subtle motherboard variations as you mention. > If it's the case, I recommend to ping Asus again and make them check and fix. We'll keep an eye open for any opportunities to go deeper here. However further investigation on both our side and theirs is blocked by not having any of the affected hardware (since the models are now so old), so I'm not very optimistic that we'll be able to make progress there. > Meanwhile, Mika's proposal sounds feasible and not so intrusive. We may > implement this later on. Chris will work on implementing this for your consideration. Thanks for the quick feedback! Daniel
Re: [PATCH] pinctrl: intel: save HOSTSW_OWN register over suspend/resume
On Wed, Mar 27, 2019 at 07:29:40PM +0200, Mika Westerberg wrote: > On Wed, Mar 27, 2019 at 04:22:04PM +0800, Daniel Drake wrote: > > On Tue, Nov 21, 2017 at 8:13 PM Mika Westerberg > > wrote: > > > On Tue, Nov 21, 2017 at 07:54:26PM +0800, Chris Chiu wrote: > > > > Yup, I checked the value of the corresponded pin. It shows following > > > > before > > > > suspend > > > > pin 18 (GPIO_18) GPIO 0x40800102 0x00024075 > > > > > > > > Then after resume > > > > pin 18 (GPIO_18) GPIO 0x40800102 0x00024075 [ACPI] > > > > > > OK, so ownership is changed to ACPI. > > > > > > > What else register do you suggest me to compare? The PADCFG2 is invalid > > > > > > It's fine APL does not have PADCFG2. > > > > > > Hmm, I don't understand how this can work in Windows either. The Windows > > > people told me that they don't save and restore anything else than > > > padcfg registers + ie. If the ownership is changed to ACPI it means you > > > don't get interrupts anymore (only GPEs) and that applies to Windows as > > > well. > > > > In the mails after the one quoted above, we reported back to you that > > the new BIOS from Asus solved the issue. > > > > However, during the time that has followed, we have had numerous user > > reports from Asus E403NA, X540NA, and X541NA laptops (basically the > > same models that we originally discussed) where the touchpad stops > > working after suspend/resume, even with the latest BIOS. We managed to > > get an affected E403NA unit in-hands again, and confirmed that > > HOSTSW_OWN was being lost like we had observed before. Hmm... Can you confirm that laptop you declared as a fixed case and the mentioned here is the same one? If they are different, I have a theory that PCBs of those two are not the same and used GPIO pin can be also not the same, therefore the BIOS fixes only one revision of the model, but didn't consider the rest. If it's the case, I recommend to ping Asus again and make them check and fix. Meanwhile, Mika's proposal sounds feasible and not so intrusive. We may implement this later on. > > > > Unfortunately as this was a customer laptop we had to return it > > immediately, before we could investigate further. We don't have access > > to any more units since they are old models now. > > > > However I'm wondering if you have any other ideas or if you think > > something like our workaround patch might be acceptable under these > > circumstances: > > https://github.com/endlessm/linux/commit/f391452299f62a3d0cbe5333be90f69e9895d8ff > > I wonder if it would be simpler to save it always and then upon resume > compare them and if changed, log this in dmesg and restore the saved > one. -- With Best Regards, Andy Shevchenko
Re: [PATCH] pinctrl: intel: save HOSTSW_OWN register over suspend/resume
On Wed, Mar 27, 2019 at 07:29:40PM +0200, Mika Westerberg wrote: > I wonder if it would be simpler to save it always and then upon resume > compare them and if changed, log this in dmesg and restore the saved > one. Actually I think better is to restore hostsw_own only for GPIOs that are already requested by us. The BIOS should have no business messing those anyway once they are owned by the GPIO driver.
Re: [PATCH] pinctrl: intel: save HOSTSW_OWN register over suspend/resume
+Andy On Wed, Mar 27, 2019 at 04:22:04PM +0800, Daniel Drake wrote: > Hi Mika, > > Digging up this old thread again... > > On Tue, Nov 21, 2017 at 8:13 PM Mika Westerberg > wrote: > > > > On Tue, Nov 21, 2017 at 07:54:26PM +0800, Chris Chiu wrote: > > > Yup, I checked the value of the corresponded pin. It shows following > > > before > > > suspend > > > pin 18 (GPIO_18) GPIO 0x40800102 0x00024075 > > > > > > Then after resume > > > pin 18 (GPIO_18) GPIO 0x40800102 0x00024075 [ACPI] > > > > OK, so ownership is changed to ACPI. > > > > > What else register do you suggest me to compare? The PADCFG2 is invalid > > > > It's fine APL does not have PADCFG2. > > > > Hmm, I don't understand how this can work in Windows either. The Windows > > people told me that they don't save and restore anything else than > > padcfg registers + ie. If the ownership is changed to ACPI it means you > > don't get interrupts anymore (only GPEs) and that applies to Windows as > > well. > > In the mails after the one quoted above, we reported back to you that > the new BIOS from Asus solved the issue. > > However, during the time that has followed, we have had numerous user > reports from Asus E403NA, X540NA, and X541NA laptops (basically the > same models that we originally discussed) where the touchpad stops > working after suspend/resume, even with the latest BIOS. We managed to > get an affected E403NA unit in-hands again, and confirmed that > HOSTSW_OWN was being lost like we had observed before. > > Unfortunately as this was a customer laptop we had to return it > immediately, before we could investigate further. We don't have access > to any more units since they are old models now. > > However I'm wondering if you have any other ideas or if you think > something like our workaround patch might be acceptable under these > circumstances: > https://github.com/endlessm/linux/commit/f391452299f62a3d0cbe5333be90f69e9895d8ff I wonder if it would be simpler to save it always and then upon resume compare them and if changed, log this in dmesg and restore the saved one.
Re: [PATCH] pinctrl: intel: save HOSTSW_OWN register over suspend/resume
Hi Mika, Digging up this old thread again... On Tue, Nov 21, 2017 at 8:13 PM Mika Westerberg wrote: > > On Tue, Nov 21, 2017 at 07:54:26PM +0800, Chris Chiu wrote: > > Yup, I checked the value of the corresponded pin. It shows following before > > suspend > > pin 18 (GPIO_18) GPIO 0x40800102 0x00024075 > > > > Then after resume > > pin 18 (GPIO_18) GPIO 0x40800102 0x00024075 [ACPI] > > OK, so ownership is changed to ACPI. > > > What else register do you suggest me to compare? The PADCFG2 is invalid > > It's fine APL does not have PADCFG2. > > Hmm, I don't understand how this can work in Windows either. The Windows > people told me that they don't save and restore anything else than > padcfg registers + ie. If the ownership is changed to ACPI it means you > don't get interrupts anymore (only GPEs) and that applies to Windows as > well. In the mails after the one quoted above, we reported back to you that the new BIOS from Asus solved the issue. However, during the time that has followed, we have had numerous user reports from Asus E403NA, X540NA, and X541NA laptops (basically the same models that we originally discussed) where the touchpad stops working after suspend/resume, even with the latest BIOS. We managed to get an affected E403NA unit in-hands again, and confirmed that HOSTSW_OWN was being lost like we had observed before. Unfortunately as this was a customer laptop we had to return it immediately, before we could investigate further. We don't have access to any more units since they are old models now. However I'm wondering if you have any other ideas or if you think something like our workaround patch might be acceptable under these circumstances: https://github.com/endlessm/linux/commit/f391452299f62a3d0cbe5333be90f69e9895d8ff Thanks Daniel
[PATCH net-next v3 1/2] net: phy: marvell10g: implement suspend/resume callbacks
This patch adds the suspend/resume callbacks for Marvell 10G PHYs. The
three PCS (base-t, base-r and 1000base-x) are set in low power (the PCS
are powered down) when the PHY isn't used.
Signed-off-by: Antoine Tenart
Reviewed-by: Andrew Lunn
---
drivers/net/phy/marvell10g.c | 14 ++
1 file changed, 14 insertions(+)
diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
index 100b401b1f4a..b56cd35182d5 100644
--- a/drivers/net/phy/marvell10g.c
+++ b/drivers/net/phy/marvell10g.c
@@ -226,11 +226,25 @@ static int mv3310_probe(struct phy_device *phydev)
static int mv3310_suspend(struct phy_device *phydev)
{
+ phy_modify_mmd(phydev, MDIO_MMD_PCS, MV_PCS_BASE_T + MDIO_CTRL1,
+ MDIO_CTRL1_LPOWER, MDIO_CTRL1_LPOWER);
+ phy_modify_mmd(phydev, MDIO_MMD_PCS, MV_PCS_BASE_R + MDIO_CTRL1,
+ MDIO_CTRL1_LPOWER, MDIO_CTRL1_LPOWER);
+ phy_modify_mmd(phydev, MDIO_MMD_PCS, MV_PCS_1000BASEX + MDIO_CTRL1,
+ MDIO_CTRL1_LPOWER, MDIO_CTRL1_LPOWER);
+
return 0;
}
static int mv3310_resume(struct phy_device *phydev)
{
+ phy_modify_mmd(phydev, MDIO_MMD_PCS, MV_PCS_BASE_T + MDIO_CTRL1,
+ MDIO_CTRL1_LPOWER, 0);
+ phy_modify_mmd(phydev, MDIO_MMD_PCS, MV_PCS_BASE_R + MDIO_CTRL1,
+ MDIO_CTRL1_LPOWER, 0);
+ phy_modify_mmd(phydev, MDIO_MMD_PCS, MV_PCS_1000BASEX + MDIO_CTRL1,
+ MDIO_CTRL1_LPOWER, 0);
+
return mv3310_hwmon_config(phydev, true);
}
--
2.20.1
[PATCH net-next v3 2/2] net: phy: marvell10g: add the suspend/resume callbacks for the 88x2210
When the 88x2110 PHY support was added, the suspend and resume callbacks
were forgotten. This patch adds them to the 88x2110 PHY callback
definition.
Signed-off-by: Antoine Tenart
Reviewed-by: Andrew Lunn
---
drivers/net/phy/marvell10g.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
index b56cd35182d5..08dd2ea08236 100644
--- a/drivers/net/phy/marvell10g.c
+++ b/drivers/net/phy/marvell10g.c
@@ -488,6 +488,8 @@ static struct phy_driver mv3310_drivers[] = {
.name = "mv88x2110",
.get_features = genphy_c45_pma_read_abilities,
.probe = mv3310_probe,
+ .suspend= mv3310_suspend,
+ .resume = mv3310_resume,
.soft_reset = genphy_no_soft_reset,
.config_init= mv3310_config_init,
.config_aneg= mv3310_config_aneg,
--
2.20.1
[PATCH 4.14 29/41] ALSA: hda - Record the current power state before suspend/resume calls
4.14-stable review patch. If anyone has any objections, please let me know.
--
From: Takashi Iwai
commit 98081ca62cbac31fb0f7efaf90b2e7384ce22257 upstream.
Currently we deal with single codec and suspend codec callbacks for
all S3, S4 and runtime PM handling. But it turned out that we want
distinguish the call patterns sometimes, e.g. for applying some init
sequence only at probing and restoring from hibernate.
This patch slightly modifies the common PM callbacks for HD-audio
codec and stores the currently processed PM event in power_state of
the codec's device.power field, which is currently unused. The codec
callback can take a look at this event value and judges which purpose
it's being called.
Signed-off-by: Takashi Iwai
Signed-off-by: Greg Kroah-Hartman
---
sound/pci/hda/hda_codec.c | 43 +--
1 file changed, 41 insertions(+), 2 deletions(-)
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -2900,6 +2900,7 @@ static void hda_call_codec_resume(struct
hda_jackpoll_work(&codec->jackpoll_work.work);
else
snd_hda_jack_report_sync(codec);
+ codec->core.dev.power.power_state = PMSG_ON;
atomic_dec(&codec->core.in_pm);
}
@@ -2932,10 +2933,48 @@ static int hda_codec_runtime_resume(stru
}
#endif /* CONFIG_PM */
+#ifdef CONFIG_PM_SLEEP
+static int hda_codec_pm_suspend(struct device *dev)
+{
+ dev->power.power_state = PMSG_SUSPEND;
+ return pm_runtime_force_suspend(dev);
+}
+
+static int hda_codec_pm_resume(struct device *dev)
+{
+ dev->power.power_state = PMSG_RESUME;
+ return pm_runtime_force_resume(dev);
+}
+
+static int hda_codec_pm_freeze(struct device *dev)
+{
+ dev->power.power_state = PMSG_FREEZE;
+ return pm_runtime_force_suspend(dev);
+}
+
+static int hda_codec_pm_thaw(struct device *dev)
+{
+ dev->power.power_state = PMSG_THAW;
+ return pm_runtime_force_resume(dev);
+}
+
+static int hda_codec_pm_restore(struct device *dev)
+{
+ dev->power.power_state = PMSG_RESTORE;
+ return pm_runtime_force_resume(dev);
+}
+#endif /* CONFIG_PM_SLEEP */
+
/* referred in hda_bind.c */
const struct dev_pm_ops hda_codec_driver_pm = {
- SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
- pm_runtime_force_resume)
+#ifdef CONFIG_PM_SLEEP
+ .suspend = hda_codec_pm_suspend,
+ .resume = hda_codec_pm_resume,
+ .freeze = hda_codec_pm_freeze,
+ .thaw = hda_codec_pm_thaw,
+ .poweroff = hda_codec_pm_suspend,
+ .restore = hda_codec_pm_restore,
+#endif /* CONFIG_PM_SLEEP */
SET_RUNTIME_PM_OPS(hda_codec_runtime_suspend, hda_codec_runtime_resume,
NULL)
};
[PATCH 5.0 51/52] ALSA: hda - Record the current power state before suspend/resume calls
5.0-stable review patch. If anyone has any objections, please let me know.
--
From: Takashi Iwai
commit 98081ca62cbac31fb0f7efaf90b2e7384ce22257 upstream.
Currently we deal with single codec and suspend codec callbacks for
all S3, S4 and runtime PM handling. But it turned out that we want
distinguish the call patterns sometimes, e.g. for applying some init
sequence only at probing and restoring from hibernate.
This patch slightly modifies the common PM callbacks for HD-audio
codec and stores the currently processed PM event in power_state of
the codec's device.power field, which is currently unused. The codec
callback can take a look at this event value and judges which purpose
it's being called.
Signed-off-by: Takashi Iwai
Signed-off-by: Greg Kroah-Hartman
---
sound/pci/hda/hda_codec.c | 43 +--
1 file changed, 41 insertions(+), 2 deletions(-)
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -2917,6 +2917,7 @@ static void hda_call_codec_resume(struct
hda_jackpoll_work(&codec->jackpoll_work.work);
else
snd_hda_jack_report_sync(codec);
+ codec->core.dev.power.power_state = PMSG_ON;
snd_hdac_leave_pm(&codec->core);
}
@@ -2950,10 +2951,48 @@ static int hda_codec_runtime_resume(stru
}
#endif /* CONFIG_PM */
+#ifdef CONFIG_PM_SLEEP
+static int hda_codec_pm_suspend(struct device *dev)
+{
+ dev->power.power_state = PMSG_SUSPEND;
+ return pm_runtime_force_suspend(dev);
+}
+
+static int hda_codec_pm_resume(struct device *dev)
+{
+ dev->power.power_state = PMSG_RESUME;
+ return pm_runtime_force_resume(dev);
+}
+
+static int hda_codec_pm_freeze(struct device *dev)
+{
+ dev->power.power_state = PMSG_FREEZE;
+ return pm_runtime_force_suspend(dev);
+}
+
+static int hda_codec_pm_thaw(struct device *dev)
+{
+ dev->power.power_state = PMSG_THAW;
+ return pm_runtime_force_resume(dev);
+}
+
+static int hda_codec_pm_restore(struct device *dev)
+{
+ dev->power.power_state = PMSG_RESTORE;
+ return pm_runtime_force_resume(dev);
+}
+#endif /* CONFIG_PM_SLEEP */
+
/* referred in hda_bind.c */
const struct dev_pm_ops hda_codec_driver_pm = {
- SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
- pm_runtime_force_resume)
+#ifdef CONFIG_PM_SLEEP
+ .suspend = hda_codec_pm_suspend,
+ .resume = hda_codec_pm_resume,
+ .freeze = hda_codec_pm_freeze,
+ .thaw = hda_codec_pm_thaw,
+ .poweroff = hda_codec_pm_suspend,
+ .restore = hda_codec_pm_restore,
+#endif /* CONFIG_PM_SLEEP */
SET_RUNTIME_PM_OPS(hda_codec_runtime_suspend, hda_codec_runtime_resume,
NULL)
};
[PATCH 4.19 43/45] ALSA: hda - Record the current power state before suspend/resume calls
4.19-stable review patch. If anyone has any objections, please let me know.
--
From: Takashi Iwai
commit 98081ca62cbac31fb0f7efaf90b2e7384ce22257 upstream.
Currently we deal with single codec and suspend codec callbacks for
all S3, S4 and runtime PM handling. But it turned out that we want
distinguish the call patterns sometimes, e.g. for applying some init
sequence only at probing and restoring from hibernate.
This patch slightly modifies the common PM callbacks for HD-audio
codec and stores the currently processed PM event in power_state of
the codec's device.power field, which is currently unused. The codec
callback can take a look at this event value and judges which purpose
it's being called.
Signed-off-by: Takashi Iwai
Signed-off-by: Greg Kroah-Hartman
---
sound/pci/hda/hda_codec.c | 43 +--
1 file changed, 41 insertions(+), 2 deletions(-)
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -2909,6 +2909,7 @@ static void hda_call_codec_resume(struct
hda_jackpoll_work(&codec->jackpoll_work.work);
else
snd_hda_jack_report_sync(codec);
+ codec->core.dev.power.power_state = PMSG_ON;
snd_hdac_leave_pm(&codec->core);
}
@@ -2942,10 +2943,48 @@ static int hda_codec_runtime_resume(stru
}
#endif /* CONFIG_PM */
+#ifdef CONFIG_PM_SLEEP
+static int hda_codec_pm_suspend(struct device *dev)
+{
+ dev->power.power_state = PMSG_SUSPEND;
+ return pm_runtime_force_suspend(dev);
+}
+
+static int hda_codec_pm_resume(struct device *dev)
+{
+ dev->power.power_state = PMSG_RESUME;
+ return pm_runtime_force_resume(dev);
+}
+
+static int hda_codec_pm_freeze(struct device *dev)
+{
+ dev->power.power_state = PMSG_FREEZE;
+ return pm_runtime_force_suspend(dev);
+}
+
+static int hda_codec_pm_thaw(struct device *dev)
+{
+ dev->power.power_state = PMSG_THAW;
+ return pm_runtime_force_resume(dev);
+}
+
+static int hda_codec_pm_restore(struct device *dev)
+{
+ dev->power.power_state = PMSG_RESTORE;
+ return pm_runtime_force_resume(dev);
+}
+#endif /* CONFIG_PM_SLEEP */
+
/* referred in hda_bind.c */
const struct dev_pm_ops hda_codec_driver_pm = {
- SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
- pm_runtime_force_resume)
+#ifdef CONFIG_PM_SLEEP
+ .suspend = hda_codec_pm_suspend,
+ .resume = hda_codec_pm_resume,
+ .freeze = hda_codec_pm_freeze,
+ .thaw = hda_codec_pm_thaw,
+ .poweroff = hda_codec_pm_suspend,
+ .restore = hda_codec_pm_restore,
+#endif /* CONFIG_PM_SLEEP */
SET_RUNTIME_PM_OPS(hda_codec_runtime_suspend, hda_codec_runtime_resume,
NULL)
};
[PATCH 4.9 17/30] ALSA: hda - Record the current power state before suspend/resume calls
4.9-stable review patch. If anyone has any objections, please let me know.
--
From: Takashi Iwai
commit 98081ca62cbac31fb0f7efaf90b2e7384ce22257 upstream.
Currently we deal with single codec and suspend codec callbacks for
all S3, S4 and runtime PM handling. But it turned out that we want
distinguish the call patterns sometimes, e.g. for applying some init
sequence only at probing and restoring from hibernate.
This patch slightly modifies the common PM callbacks for HD-audio
codec and stores the currently processed PM event in power_state of
the codec's device.power field, which is currently unused. The codec
callback can take a look at this event value and judges which purpose
it's being called.
Signed-off-by: Takashi Iwai
Signed-off-by: Greg Kroah-Hartman
---
sound/pci/hda/hda_codec.c | 43 +--
1 file changed, 41 insertions(+), 2 deletions(-)
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -3004,6 +3004,7 @@ static void hda_call_codec_resume(struct
hda_jackpoll_work(&codec->jackpoll_work.work);
else
snd_hda_jack_report_sync(codec);
+ codec->core.dev.power.power_state = PMSG_ON;
atomic_dec(&codec->core.in_pm);
}
@@ -3036,10 +3037,48 @@ static int hda_codec_runtime_resume(stru
}
#endif /* CONFIG_PM */
+#ifdef CONFIG_PM_SLEEP
+static int hda_codec_pm_suspend(struct device *dev)
+{
+ dev->power.power_state = PMSG_SUSPEND;
+ return pm_runtime_force_suspend(dev);
+}
+
+static int hda_codec_pm_resume(struct device *dev)
+{
+ dev->power.power_state = PMSG_RESUME;
+ return pm_runtime_force_resume(dev);
+}
+
+static int hda_codec_pm_freeze(struct device *dev)
+{
+ dev->power.power_state = PMSG_FREEZE;
+ return pm_runtime_force_suspend(dev);
+}
+
+static int hda_codec_pm_thaw(struct device *dev)
+{
+ dev->power.power_state = PMSG_THAW;
+ return pm_runtime_force_resume(dev);
+}
+
+static int hda_codec_pm_restore(struct device *dev)
+{
+ dev->power.power_state = PMSG_RESTORE;
+ return pm_runtime_force_resume(dev);
+}
+#endif /* CONFIG_PM_SLEEP */
+
/* referred in hda_bind.c */
const struct dev_pm_ops hda_codec_driver_pm = {
- SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
- pm_runtime_force_resume)
+#ifdef CONFIG_PM_SLEEP
+ .suspend = hda_codec_pm_suspend,
+ .resume = hda_codec_pm_resume,
+ .freeze = hda_codec_pm_freeze,
+ .thaw = hda_codec_pm_thaw,
+ .poweroff = hda_codec_pm_suspend,
+ .restore = hda_codec_pm_restore,
+#endif /* CONFIG_PM_SLEEP */
SET_RUNTIME_PM_OPS(hda_codec_runtime_suspend, hda_codec_runtime_resume,
NULL)
};
Re: [alsa-devel] [PATCH] ASoC: intel: Fix crash at suspend/resume after failed codec registration
On Mon, Mar 25, 2019 at 03:05:33PM +, Mark Brown wrote: > On Mon, Mar 25, 2019 at 07:21:00AM -0700, Guenter Roeck wrote: > > > It is actually a bit more complicated than that. The stored pointer > > (drv->soc_card) > > isn't released. The problem is that dev_get_drvdata(drv->soc_card->dev) is > > NULL, > > which causes the crash. I don't think there is a UAF involved - I built the > > test image with KASAN enabled and it did not barf at me. > > What is a "UAF"? > use-after-free. Sorry, I saw that term used so often recently that I somehow thought it was common and started using it myself. Guenter > > Overall the implementation does seem a bit suspicious to me. I don't really > > understand why the platform driver handles suspend/resume for the cards. > > But that may just be my lack of understanding. However, either case, I > > think the > > Haswell driver (sst-haswell-pcm.c) has a similar problem. I am also not > > sure if > > It's certainly a bit unusual, usually the platform driver would just > deal with suspending itself and the card driver would handle overall > card suspension together with the core.
Re: [alsa-devel] [PATCH] ASoC: intel: Fix crash at suspend/resume after failed codec registration
On Mon, Mar 25, 2019 at 07:21:00AM -0700, Guenter Roeck wrote: > It is actually a bit more complicated than that. The stored pointer > (drv->soc_card) > isn't released. The problem is that dev_get_drvdata(drv->soc_card->dev) is > NULL, > which causes the crash. I don't think there is a UAF involved - I built the > test image with KASAN enabled and it did not barf at me. What is a "UAF"? > Overall the implementation does seem a bit suspicious to me. I don't really > understand why the platform driver handles suspend/resume for the cards. > But that may just be my lack of understanding. However, either case, I think > the > Haswell driver (sst-haswell-pcm.c) has a similar problem. I am also not sure > if It's certainly a bit unusual, usually the platform driver would just deal with suspending itself and the card driver would handle overall card suspension together with the core. signature.asc Description: PGP signature
Re: [alsa-devel] [PATCH] ASoC: intel: Fix crash at suspend/resume after failed codec registration
On Mon, Mar 25, 2019 at 09:18:04AM -0400, Pierre-Louis Bossart wrote: > On 3/25/19 8:12 AM, Mark Brown wrote: > > These are driver specific issues not device model issues as far as I can > > see? The issue fixed by this as is that you're storing a pointer in the > > ASoC level (not device model level) probe that you don't free when the > > component is unbound, causing you to dereference it later during > > suspend. There is absolutely no problem with the machine driver not > > being guaranteed to bind at the time it's initially registered, that's > > perfectly normal and should cause no problems. > Agree, what I was referring is that if the machine probe and card > registration fails (not just deferred), the parent acpi/pci driver isn't > notified - there is just no means to provide that information and that leads > to all kinds of configuration issues. If there are issues here they could happen via means other than a probe failing so there's a problem whatever is going on - someone manually unbinding a device for example. signature.asc Description: PGP signature
Re: [alsa-devel] [PATCH] ASoC: intel: Fix crash at suspend/resume after failed codec registration
On 3/25/19 5:12 AM, Mark Brown wrote: On Sat, Mar 23, 2019 at 09:55:46AM -0400, Pierre-Louis Bossart wrote: I'd like to highlight that there is a fundamental flaw in the way the machine drivers are handled. Since we don't have a hook for the machine driver in the BIOS, the DSP driver creates a platform_device which will instantiate the machine driver. When errors happen in the machine driver probe, they are suppressed due to a 'feature' of the device model, so you can end-up with a broken configuration that is still reported as a successful strobe. These are driver specific issues not device model issues as far as I can see? The issue fixed by this as is that you're storing a pointer in the ASoC level (not device model level) probe that you don't free when the component is unbound, causing you to dereference it later during suspend. There is absolutely no problem with the machine driver not being guaranteed to bind at the time it's initially registered, that's perfectly normal and should cause no problems. It is actually a bit more complicated than that. The stored pointer (drv->soc_card) isn't released. The problem is that dev_get_drvdata(drv->soc_card->dev) is NULL, which causes the crash. I don't think there is a UAF involved - I built the test image with KASAN enabled and it did not barf at me. It may of course well be that there _should_ be a UAF but it doesn't happen because some pointer that should be released isn't released due to some memory or reference count leak. But that would be a different problem. Overall the implementation does seem a bit suspicious to me. I don't really understand why the platform driver handles suspend/resume for the cards. But that may just be my lack of understanding. However, either case, I think the Haswell driver (sst-haswell-pcm.c) has a similar problem. I am also not sure if there are more problems lurking - I see a similar but different crash in v4.4.y but have not been able to track it down. Actually, I found the problem fixed here while trying to reproduce that crash with the latest kernel. Guenter
[PATCH v2 1/2] pinctrl: pinctrl-imx: Add suspend/resume for LPSR
From: Robin Gong
To support pinctl hog restore after LPSR resume back,
add suspend/resume in pinctrl driver.
Signed-off-by: Robin Gong
Signed-off-by: Abel Vesa
---
Changes since v1:
* fixed Robin's email
drivers/pinctrl/freescale/pinctrl-imx.c | 20
drivers/pinctrl/freescale/pinctrl-imx.h | 3 +++
2 files changed, 23 insertions(+)
diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c
b/drivers/pinctrl/freescale/pinctrl-imx.c
index 188001b..93c0253 100644
--- a/drivers/pinctrl/freescale/pinctrl-imx.c
+++ b/drivers/pinctrl/freescale/pinctrl-imx.c
@@ -887,3 +887,23 @@ int imx_pinctrl_probe(struct platform_device *pdev,
return ret;
}
+
+int imx_pinctrl_suspend(struct device *dev)
+{
+ struct imx_pinctrl *ipctl = dev_get_drvdata(dev);
+
+ if (!ipctl)
+ return -EINVAL;
+
+ return pinctrl_force_sleep(ipctl->pctl);
+}
+
+int imx_pinctrl_resume(struct device *dev)
+{
+ struct imx_pinctrl *ipctl = dev_get_drvdata(dev);
+
+ if (!ipctl)
+ return -EINVAL;
+
+ return pinctrl_force_default(ipctl->pctl);
+}
diff --git a/drivers/pinctrl/freescale/pinctrl-imx.h
b/drivers/pinctrl/freescale/pinctrl-imx.h
index 98a4889..795669c 100644
--- a/drivers/pinctrl/freescale/pinctrl-imx.h
+++ b/drivers/pinctrl/freescale/pinctrl-imx.h
@@ -136,6 +136,9 @@ struct imx_pinctrl {
int imx_pinctrl_probe(struct platform_device *pdev,
const struct imx_pinctrl_soc_info *info);
+int imx_pinctrl_suspend(struct device *dev);
+int imx_pinctrl_resume(struct device *dev);
+
#ifdef CONFIG_PINCTRL_IMX_SCU
#define BM_PAD_CTL_GP_ENABLE BIT(30)
#define BM_PAD_CTL_IFMUX_ENABLEBIT(31)
--
2.7.4
Re: [PATCH 1/2] pinctrl: pinctrl-imx: Add suspend/resume for LPSR
On Mon, Mar 25, 2019 at 3:49 PM Abel Vesa wrote: > > From: Robin Gong Please fix Robin's email.
[PATCH 1/2] pinctrl: pinctrl-imx: Add suspend/resume for LPSR
From: Robin Gong
To support pinctl hog restore after LPSR resume back,
add suspend/resume in pinctrl driver.
Signed-off-by: Robin Gong
Signed-off-by: Abel Vesa
---
drivers/pinctrl/freescale/pinctrl-imx.c | 20
drivers/pinctrl/freescale/pinctrl-imx.h | 3 +++
2 files changed, 23 insertions(+)
diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c
b/drivers/pinctrl/freescale/pinctrl-imx.c
index 188001b..93c0253 100644
--- a/drivers/pinctrl/freescale/pinctrl-imx.c
+++ b/drivers/pinctrl/freescale/pinctrl-imx.c
@@ -887,3 +887,23 @@ int imx_pinctrl_probe(struct platform_device *pdev,
return ret;
}
+
+int imx_pinctrl_suspend(struct device *dev)
+{
+ struct imx_pinctrl *ipctl = dev_get_drvdata(dev);
+
+ if (!ipctl)
+ return -EINVAL;
+
+ return pinctrl_force_sleep(ipctl->pctl);
+}
+
+int imx_pinctrl_resume(struct device *dev)
+{
+ struct imx_pinctrl *ipctl = dev_get_drvdata(dev);
+
+ if (!ipctl)
+ return -EINVAL;
+
+ return pinctrl_force_default(ipctl->pctl);
+}
diff --git a/drivers/pinctrl/freescale/pinctrl-imx.h
b/drivers/pinctrl/freescale/pinctrl-imx.h
index 98a4889..795669c 100644
--- a/drivers/pinctrl/freescale/pinctrl-imx.h
+++ b/drivers/pinctrl/freescale/pinctrl-imx.h
@@ -136,6 +136,9 @@ struct imx_pinctrl {
int imx_pinctrl_probe(struct platform_device *pdev,
const struct imx_pinctrl_soc_info *info);
+int imx_pinctrl_suspend(struct device *dev);
+int imx_pinctrl_resume(struct device *dev);
+
#ifdef CONFIG_PINCTRL_IMX_SCU
#define BM_PAD_CTL_GP_ENABLE BIT(30)
#define BM_PAD_CTL_IFMUX_ENABLEBIT(31)
--
2.7.4
Re: [alsa-devel] [PATCH] ASoC: intel: Fix crash at suspend/resume after failed codec registration
On 3/25/19 8:12 AM, Mark Brown wrote: On Sat, Mar 23, 2019 at 09:55:46AM -0400, Pierre-Louis Bossart wrote: I'd like to highlight that there is a fundamental flaw in the way the machine drivers are handled. Since we don't have a hook for the machine driver in the BIOS, the DSP driver creates a platform_device which will instantiate the machine driver. When errors happen in the machine driver probe, they are suppressed due to a 'feature' of the device model, so you can end-up with a broken configuration that is still reported as a successful strobe. These are driver specific issues not device model issues as far as I can see? The issue fixed by this as is that you're storing a pointer in the ASoC level (not device model level) probe that you don't free when the component is unbound, causing you to dereference it later during suspend. There is absolutely no problem with the machine driver not being guaranteed to bind at the time it's initially registered, that's perfectly normal and should cause no problems. Agree, what I was referring is that if the machine probe and card registration fails (not just deferred), the parent acpi/pci driver isn't notified - there is just no means to provide that information and that leads to all kinds of configuration issues.
Applied "ASoC: intel: Fix crash at suspend/resume after failed codec registration" to the asoc tree
The patch
ASoC: intel: Fix crash at suspend/resume after failed codec registration
has been applied to the asoc tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git
All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying
to this mail.
Thanks,
Mark
>From 8f71370f4b02730e8c27faf460af7a3586e24e1f Mon Sep 17 00:00:00 2001
From: Guenter Roeck
Date: Fri, 22 Mar 2019 15:39:48 -0700
Subject: [PATCH] ASoC: intel: Fix crash at suspend/resume after failed codec
registration
If codec registration fails after the ASoC Intel SST driver has been probed,
the kernel will Oops and crash at suspend/resume.
general protection fault: [#1] PREEMPT SMP KASAN PTI
CPU: 1 PID: 2811 Comm: cat Tainted: GW 4.19.30 #15
Hardware name: GOOGLE Clapper, BIOS Google_Clapper.5216.199.7 08/22/2014
RIP: 0010:snd_soc_suspend+0x5a/0xd21
Code: 03 80 3c 10 00 49 89 d7 74 0b 48 89 df e8 71 72 c4 fe 4c 89
fa 48 8b 03 48 89 45 d0 48 8d 98 a0 01 00 00 48 89 d8 48 c1 e8 03
<8a> 04 10 84 c0 0f 85 85 0c 00 00 80 3b 00 0f 84 6b 0c 00 00 48 8b
RSP: 0018:888035407750 EFLAGS: 00010202
RAX: 0034 RBX: 01a0 RCX:
RDX: dc00 RSI: 0008 RDI: 88805c417098
RBP: 8880354077b0 R08: dc00 R09: ed100b975718
R10: 0001 R11: 949ea4a3 R12: 11100b975746
R13: dc00 R14: 88805cba4588 R15: dc00
FS: 794a78e91b80() GS:888068d0() knlGS:
CS: 0010 DS: ES: CR0: 80050033
CR2: 7bd5283ccf58 CR3: 4b7aa000 CR4: 001006e0
Call Trace:
? dpm_complete+0x67b/0x67b
? i915_gem_suspend+0x14d/0x1ad
sst_soc_prepare+0x91/0x1dd
? sst_be_hw_params+0x7e/0x7e
dpm_prepare+0x39a/0x88b
dpm_suspend_start+0x13/0x9d
suspend_devices_and_enter+0x18f/0xbd7
? arch_suspend_enable_irqs+0x11/0x11
? printk+0xd9/0x12d
? lock_release+0x95f/0x95f
? log_buf_vmcoreinfo_setup+0x131/0x131
? rcu_read_lock_sched_held+0x140/0x22a
? __bpf_trace_rcu_utilization+0xa/0xa
? __pm_pr_dbg+0x186/0x190
? pm_notifier_call_chain+0x39/0x39
? suspend_test+0x9d/0x9d
pm_suspend+0x2f4/0x728
? trace_suspend_resume+0x3da/0x3da
? lock_release+0x95f/0x95f
? kernfs_fop_write+0x19f/0x32d
state_store+0xd8/0x147
? sysfs_kf_read+0x155/0x155
kernfs_fop_write+0x23e/0x32d
__vfs_write+0x108/0x608
? vfs_read+0x2e9/0x2e9
? rcu_read_lock_sched_held+0x140/0x22a
? __bpf_trace_rcu_utilization+0xa/0xa
? debug_smp_processor_id+0x10/0x10
? selinux_file_permission+0x1c5/0x3c8
? rcu_sync_lockdep_assert+0x6a/0xad
? __sb_start_write+0x129/0x2ac
vfs_write+0x1aa/0x434
ksys_write+0xfe/0x1be
? __ia32_sys_read+0x82/0x82
do_syscall_64+0xcd/0x120
entry_SYSCALL_64_after_hwframe+0x49/0xbe
In the observed situation, the problem is seen because the codec driver
failed to probe due to a hardware problem.
max98090 i2c-193C9890:00: Failed to read device revision: -1
max98090 i2c-193C9890:00: ASoC: failed to probe component -1
cht-bsw-max98090 cht-bsw-max98090: ASoC: failed to instantiate card -1
cht-bsw-max98090 cht-bsw-max98090: snd_soc_register_card failed -1
cht-bsw-max98090: probe of cht-bsw-max98090 failed with error -1
The problem is similar to the problem solved with commit 2fc995a87f2e
("ASoC: intel: Fix crash at suspend/resume without card registration"),
but codec registration fails at a later point. At that time, the pointer
checked with the above mentioned commit is already set, but it is not
cleared if the device is subsequently removed. Adding a remove function
to clear the pointer fixes the problem.
Cc: sta...@vger.kernel.org
Cc: Jarkko Nikula
Cc: Curtis Malainey
Signed-off-by: Guenter Roeck
Acked-by: Pierre-Louis Bossart
Signed-off-by: Mark Brown
---
sound/soc/intel/atom/sst-mfld-platform-pcm.c | 8
1 file changed, 8 insertions(+)
diff --git a/sound/soc/intel/atom/sst-mfld-platform-pcm.c
b/sound/soc/intel/atom/sst-mfld-platform-pcm.c
index 08cea5b5cda9..0e8b1c5eec88 100644
--- a/sound/soc/intel/atom/sst-mfld-platform-pcm.c
+++ b/sound/soc/intel/atom/sst-mfld-platform-pcm.c
@@ -706,9 +706,17 @@ static int sst_soc_probe(struct snd_soc_component
*component)
return sst_dsp_init_v2_dpcm(component);
}
+static void sst_soc_remove(struct snd_soc_component *component)
+{
+ s
Re: [alsa-devel] [PATCH] ASoC: intel: Fix crash at suspend/resume after failed codec registration
On Sat, Mar 23, 2019 at 09:55:46AM -0400, Pierre-Louis Bossart wrote: > I'd like to highlight that there is a fundamental flaw in the way the > machine drivers are handled. Since we don't have a hook for the machine > driver in the BIOS, the DSP driver creates a platform_device which will > instantiate the machine driver. When errors happen in the machine driver > probe, they are suppressed due to a 'feature' of the device model, so you > can end-up with a broken configuration that is still reported as a > successful strobe. These are driver specific issues not device model issues as far as I can see? The issue fixed by this as is that you're storing a pointer in the ASoC level (not device model level) probe that you don't free when the component is unbound, causing you to dereference it later during suspend. There is absolutely no problem with the machine driver not being guaranteed to bind at the time it's initially registered, that's perfectly normal and should cause no problems. signature.asc Description: PGP signature
Re: [PATCH] ASoC: intel: Fix crash at suspend/resume after failed codec registration
On Fri, Mar 22, 2019 at 03:39:48PM -0700, Guenter Roeck wrote: > general protection fault: [#1] PREEMPT SMP KASAN PTI > CPU: 1 PID: 2811 Comm: cat Tainted: GW 4.19.30 #15 > Hardware name: GOOGLE Clapper, BIOS Google_Clapper.5216.199.7 08/22/2014 > RIP: 0010:snd_soc_suspend+0x5a/0xd21 > Code: 03 80 3c 10 00 49 89 d7 74 0b 48 89 df e8 71 72 c4 fe 4c 89 > fa 48 8b 03 48 89 45 d0 48 8d 98 a0 01 00 00 48 89 d8 48 c1 e8 03 > <8a> 04 10 84 c0 0f 85 85 0c 00 00 80 3b 00 0f 84 6b 0c 00 00 48 8b Please think hard before including complete backtraces in upstream reports, they are very large and contain almost no useful information relative to their size so often obscure the relevant content in your message. If part of the backtrace is usefully illustrative then it's usually better to pull out the relevant sections. signature.asc Description: PGP signature
Re: [alsa-devel] [PATCH] ASoC: intel: Fix crash at suspend/resume after failed codec registration
On 3/22/19 6:39 PM, Guenter Roeck wrote:
If codec registration fails after the ASoC Intel SST driver has been probed,
the kernel will Oops and crash at suspend/resume.
general protection fault: [#1] PREEMPT SMP KASAN PTI
CPU: 1 PID: 2811 Comm: cat Tainted: GW 4.19.30 #15
Hardware name: GOOGLE Clapper, BIOS Google_Clapper.5216.199.7 08/22/2014
RIP: 0010:snd_soc_suspend+0x5a/0xd21
Code: 03 80 3c 10 00 49 89 d7 74 0b 48 89 df e8 71 72 c4 fe 4c 89
fa 48 8b 03 48 89 45 d0 48 8d 98 a0 01 00 00 48 89 d8 48 c1 e8 03
<8a> 04 10 84 c0 0f 85 85 0c 00 00 80 3b 00 0f 84 6b 0c 00 00 48 8b
RSP: 0018:888035407750 EFLAGS: 00010202
RAX: 0034 RBX: 01a0 RCX:
RDX: dc00 RSI: 0008 RDI: 88805c417098
RBP: 8880354077b0 R08: dc00 R09: ed100b975718
R10: 0001 R11: 949ea4a3 R12: 11100b975746
R13: dc00 R14: 88805cba4588 R15: dc00
FS: 794a78e91b80() GS:888068d0() knlGS:
CS: 0010 DS: ES: CR0: 80050033
CR2: 7bd5283ccf58 CR3: 4b7aa000 CR4: 001006e0
Call Trace:
? dpm_complete+0x67b/0x67b
? i915_gem_suspend+0x14d/0x1ad
sst_soc_prepare+0x91/0x1dd
? sst_be_hw_params+0x7e/0x7e
dpm_prepare+0x39a/0x88b
dpm_suspend_start+0x13/0x9d
suspend_devices_and_enter+0x18f/0xbd7
? arch_suspend_enable_irqs+0x11/0x11
? printk+0xd9/0x12d
? lock_release+0x95f/0x95f
? log_buf_vmcoreinfo_setup+0x131/0x131
? rcu_read_lock_sched_held+0x140/0x22a
? __bpf_trace_rcu_utilization+0xa/0xa
? __pm_pr_dbg+0x186/0x190
? pm_notifier_call_chain+0x39/0x39
? suspend_test+0x9d/0x9d
pm_suspend+0x2f4/0x728
? trace_suspend_resume+0x3da/0x3da
? lock_release+0x95f/0x95f
? kernfs_fop_write+0x19f/0x32d
state_store+0xd8/0x147
? sysfs_kf_read+0x155/0x155
kernfs_fop_write+0x23e/0x32d
__vfs_write+0x108/0x608
? vfs_read+0x2e9/0x2e9
? rcu_read_lock_sched_held+0x140/0x22a
? __bpf_trace_rcu_utilization+0xa/0xa
? debug_smp_processor_id+0x10/0x10
? selinux_file_permission+0x1c5/0x3c8
? rcu_sync_lockdep_assert+0x6a/0xad
? __sb_start_write+0x129/0x2ac
vfs_write+0x1aa/0x434
ksys_write+0xfe/0x1be
? __ia32_sys_read+0x82/0x82
do_syscall_64+0xcd/0x120
entry_SYSCALL_64_after_hwframe+0x49/0xbe
In the observed situation, the problem is seen because the codec driver
failed to probe due to a hardware problem.
max98090 i2c-193C9890:00: Failed to read device revision: -1
max98090 i2c-193C9890:00: ASoC: failed to probe component -1
cht-bsw-max98090 cht-bsw-max98090: ASoC: failed to instantiate card -1
cht-bsw-max98090 cht-bsw-max98090: snd_soc_register_card failed -1
cht-bsw-max98090: probe of cht-bsw-max98090 failed with error -1
The problem is similar to the problem solved with commit 2fc995a87f2e
("ASoC: intel: Fix crash at suspend/resume without card registration"),
but codec registration fails at a later point. At that time, the pointer
checked with the above mentioned commit is already set, but it is not
cleared if the device is subsequently removed. Adding a remove function
to clear the pointer fixes the problem.
Makes sense
Acked-by: Pierre-Louis Bossart
I'd like to highlight that there is a fundamental flaw in the way the
machine drivers are handled. Since we don't have a hook for the machine
driver in the BIOS, the DSP driver creates a platform_device which will
instantiate the machine driver. When errors happen in the machine driver
probe, they are suppressed due to a 'feature' of the device model, so
you can end-up with a broken configuration that is still reported as a
successful strobe.
Cc: sta...@vger.kernel.org
Cc: Jarkko Nikula
Cc: Curtis Malainey
Signed-off-by: Guenter Roeck
---
sound/soc/intel/atom/sst-mfld-platform-pcm.c | 8
1 file changed, 8 insertions(+)
diff --git a/sound/soc/intel/atom/sst-mfld-platform-pcm.c
b/sound/soc/intel/atom/sst-mfld-platform-pcm.c
index 08cea5b5cda9..0e8b1c5eec88 100644
--- a/sound/soc/intel/atom/sst-mfld-platform-pcm.c
+++ b/sound/soc/intel/atom/sst-mfld-platform-pcm.c
@@ -706,9 +706,17 @@ static int sst_soc_probe(struct snd_soc_component
*component)
return sst_dsp_init_v2_dpcm(component);
}
+static void sst_soc_remove(struct snd_soc_component *component)
+{
+ struct sst_data *drv = dev_get_drvdata(component->dev);
+
+ drv->soc_card = NULL;
+}
+
static const struct snd_soc_component_driver sst_soc_platform_drv = {
.name = DRV_NAME,
.probe = sst_soc_probe,
+ .remove = sst_soc_remove,
.ops= &sst_platform_ops,
.compr_ops = &sst_platform_compr_ops,
.pcm_new= sst_pcm_new,
[PATCH] ASoC: intel: Fix crash at suspend/resume after failed codec registration
If codec registration fails after the ASoC Intel SST driver has been probed,
the kernel will Oops and crash at suspend/resume.
general protection fault: [#1] PREEMPT SMP KASAN PTI
CPU: 1 PID: 2811 Comm: cat Tainted: GW 4.19.30 #15
Hardware name: GOOGLE Clapper, BIOS Google_Clapper.5216.199.7 08/22/2014
RIP: 0010:snd_soc_suspend+0x5a/0xd21
Code: 03 80 3c 10 00 49 89 d7 74 0b 48 89 df e8 71 72 c4 fe 4c 89
fa 48 8b 03 48 89 45 d0 48 8d 98 a0 01 00 00 48 89 d8 48 c1 e8 03
<8a> 04 10 84 c0 0f 85 85 0c 00 00 80 3b 00 0f 84 6b 0c 00 00 48 8b
RSP: 0018:888035407750 EFLAGS: 00010202
RAX: 0034 RBX: 01a0 RCX:
RDX: dc00 RSI: 0008 RDI: 88805c417098
RBP: 8880354077b0 R08: dc00 R09: ed100b975718
R10: 0001 R11: 949ea4a3 R12: 11100b975746
R13: dc00 R14: 88805cba4588 R15: dc00
FS: 794a78e91b80() GS:888068d0() knlGS:
CS: 0010 DS: ES: CR0: 80050033
CR2: 7bd5283ccf58 CR3: 4b7aa000 CR4: 001006e0
Call Trace:
? dpm_complete+0x67b/0x67b
? i915_gem_suspend+0x14d/0x1ad
sst_soc_prepare+0x91/0x1dd
? sst_be_hw_params+0x7e/0x7e
dpm_prepare+0x39a/0x88b
dpm_suspend_start+0x13/0x9d
suspend_devices_and_enter+0x18f/0xbd7
? arch_suspend_enable_irqs+0x11/0x11
? printk+0xd9/0x12d
? lock_release+0x95f/0x95f
? log_buf_vmcoreinfo_setup+0x131/0x131
? rcu_read_lock_sched_held+0x140/0x22a
? __bpf_trace_rcu_utilization+0xa/0xa
? __pm_pr_dbg+0x186/0x190
? pm_notifier_call_chain+0x39/0x39
? suspend_test+0x9d/0x9d
pm_suspend+0x2f4/0x728
? trace_suspend_resume+0x3da/0x3da
? lock_release+0x95f/0x95f
? kernfs_fop_write+0x19f/0x32d
state_store+0xd8/0x147
? sysfs_kf_read+0x155/0x155
kernfs_fop_write+0x23e/0x32d
__vfs_write+0x108/0x608
? vfs_read+0x2e9/0x2e9
? rcu_read_lock_sched_held+0x140/0x22a
? __bpf_trace_rcu_utilization+0xa/0xa
? debug_smp_processor_id+0x10/0x10
? selinux_file_permission+0x1c5/0x3c8
? rcu_sync_lockdep_assert+0x6a/0xad
? __sb_start_write+0x129/0x2ac
vfs_write+0x1aa/0x434
ksys_write+0xfe/0x1be
? __ia32_sys_read+0x82/0x82
do_syscall_64+0xcd/0x120
entry_SYSCALL_64_after_hwframe+0x49/0xbe
In the observed situation, the problem is seen because the codec driver
failed to probe due to a hardware problem.
max98090 i2c-193C9890:00: Failed to read device revision: -1
max98090 i2c-193C9890:00: ASoC: failed to probe component -1
cht-bsw-max98090 cht-bsw-max98090: ASoC: failed to instantiate card -1
cht-bsw-max98090 cht-bsw-max98090: snd_soc_register_card failed -1
cht-bsw-max98090: probe of cht-bsw-max98090 failed with error -1
The problem is similar to the problem solved with commit 2fc995a87f2e
("ASoC: intel: Fix crash at suspend/resume without card registration"),
but codec registration fails at a later point. At that time, the pointer
checked with the above mentioned commit is already set, but it is not
cleared if the device is subsequently removed. Adding a remove function
to clear the pointer fixes the problem.
Cc: sta...@vger.kernel.org
Cc: Jarkko Nikula
Cc: Curtis Malainey
Signed-off-by: Guenter Roeck
---
sound/soc/intel/atom/sst-mfld-platform-pcm.c | 8
1 file changed, 8 insertions(+)
diff --git a/sound/soc/intel/atom/sst-mfld-platform-pcm.c
b/sound/soc/intel/atom/sst-mfld-platform-pcm.c
index 08cea5b5cda9..0e8b1c5eec88 100644
--- a/sound/soc/intel/atom/sst-mfld-platform-pcm.c
+++ b/sound/soc/intel/atom/sst-mfld-platform-pcm.c
@@ -706,9 +706,17 @@ static int sst_soc_probe(struct snd_soc_component
*component)
return sst_dsp_init_v2_dpcm(component);
}
+static void sst_soc_remove(struct snd_soc_component *component)
+{
+ struct sst_data *drv = dev_get_drvdata(component->dev);
+
+ drv->soc_card = NULL;
+}
+
static const struct snd_soc_component_driver sst_soc_platform_drv = {
.name = DRV_NAME,
.probe = sst_soc_probe,
+ .remove = sst_soc_remove,
.ops= &sst_platform_ops,
.compr_ops = &sst_platform_compr_ops,
.pcm_new= sst_pcm_new,
--
2.7.4
[BACKPORT 4.4.y 13/25] extcon: usb-gpio: Don't miss event during suspend/resume
From: Roger Quadros Pin state might have changed during suspend/resume while our interrupts were disabled and if device doesn't support wakeup. Scan for change during resume for such case. Signed-off-by: Roger Quadros Signed-off-by: Chanwoo Choi (cherry picked from commit 04c080080855ce84dcd490a2e04805608a21085d) Signed-off-by: Arnd Bergmann --- drivers/extcon/extcon-usb-gpio.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/extcon/extcon-usb-gpio.c b/drivers/extcon/extcon-usb-gpio.c index 2b2fecffb1ad..c6a7c9ddf0ac 100644 --- a/drivers/extcon/extcon-usb-gpio.c +++ b/drivers/extcon/extcon-usb-gpio.c @@ -192,6 +192,9 @@ static int usb_extcon_resume(struct device *dev) } enable_irq(info->id_irq); + if (!device_may_wakeup(dev)) + queue_delayed_work(system_power_efficient_wq, + &info->wq_detcable, 0); return ret; } -- 2.20.0
Re: [PATCH] rtc: cros-ec: Fail suspend/resume if wake IRQ can't be configured
On 15/03/2019 11:51:12-0700, Stephen Boyd wrote: > If we encounter a failure during suspend where this RTC was programmed > to wakeup the system from suspend, but that wakeup couldn't be > configured because the system didn't support wakeup interrupts, we'll > run into the following warning: > > Unbalanced IRQ 166 wake disable > WARNING: CPU: 7 PID: 3071 at kernel/irq/manage.c:669 > irq_set_irq_wake+0x108/0x278 > > This happens because the suspend process isn't aborted when the RTC > fails to configure the wakeup IRQ. Instead, we continue suspending the > system and then another suspend callback fails the suspend process and > "unwinds" the previously suspended drivers by calling their resume > callbacks. When we get back to resuming this RTC driver, we'll call > disable_irq_wake() on an IRQ that hasn't been configured for wake. > > Let's just fail suspend/resume here if we can't configure the system to > wake and the user has chosen to wakeup with this device. This fixes this > warning and makes the code more robust in case there are systems out > there that can't wakeup from suspend on this line but the user has > chosen to do so. > > Cc: Enric Balletbo i Serra > Cc: Evan Green > Cc: Benson Leung > Cc: Guenter Roeck > Signed-off-by: Stephen Boyd > --- > drivers/rtc/rtc-cros-ec.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > Applied, thanks. -- Alexandre Belloni, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
[PATCH v10 7/7] ARM: tegra: Add firmware calls required for suspend-resume on Tegra30
In order to suspend-resume CPU with Trusted Foundations firmware being
present on Tegra30, the LP1/LP2 boot vectors and CPU caches need to be
set up using the firmware calls and then suspend code shall avoid
re-disabling parts that were disabled by the firmware.
Tested-by: Robert Yang
Tested-by: Michał Mirosław
Signed-off-by: Dmitry Osipenko
---
arch/arm/mach-tegra/pm.c| 49 +
arch/arm/mach-tegra/reset-handler.S | 26 +++
arch/arm/mach-tegra/sleep.S | 14 ++---
3 files changed, 84 insertions(+), 5 deletions(-)
diff --git a/arch/arm/mach-tegra/pm.c b/arch/arm/mach-tegra/pm.c
index 1ad5719779b0..abf5f88778f4 100644
--- a/arch/arm/mach-tegra/pm.c
+++ b/arch/arm/mach-tegra/pm.c
@@ -33,11 +33,13 @@
#include
#include
+#include
#include
#include
#include
#include
#include
+#include
#include "iomap.h"
#include "pm.h"
@@ -159,6 +161,28 @@ int tegra_cpu_do_idle(void)
static int tegra_sleep_cpu(unsigned long v2p)
{
+ /*
+* L2 cache disabling using kernel API only allowed when all
+* secondary CPU's are offline. Cache have to be disabled with
+* MMU-on if cache maintenance is done via Trusted Foundations
+* firmware. Note that CPUIDLE won't ever enter powergate on Tegra30
+* if any of secondary CPU's is online and this is the LP2-idle
+* code-path only for Tegra20/30.
+*/
+ if (trusted_foundations_registered())
+ outer_disable();
+
+ /*
+* Note that besides of setting up CPU reset vector this firmware
+* call may also do the following, depending on the FW version:
+* 1) Disable L2. But this doesn't matter since we already
+* disabled the L2.
+* 2) Disable D-cache. This need to be taken into account in
+* particular by the tegra_disable_clean_inv_dcache() which
+* shall avoid the re-disable.
+*/
+ call_firmware_op(prepare_idle, TF_PM_MODE_LP2);
+
setup_mm_for_reboot();
tegra_sleep_cpu_finish(v2p);
@@ -197,6 +221,14 @@ void tegra_idle_lp2_last(void)
cpu_suspend(PHYS_OFFSET - PAGE_OFFSET, &tegra_sleep_cpu);
+ /*
+* Resume L2 cache if it wasn't re-enabled early during resume,
+* which is the case for Tegra30 that has to re-enable the cache
+* via firmware call. In other cases cache is already enabled and
+* hence re-enabling is a no-op. This is always a no-op on Tegra114+.
+*/
+ outer_resume();
+
restore_cpu_complex();
cpu_cluster_pm_exit();
}
@@ -215,6 +247,15 @@ enum tegra_suspend_mode tegra_pm_validate_suspend_mode(
static int tegra_sleep_core(unsigned long v2p)
{
+ /*
+* Cache have to be disabled with MMU-on if cache maintenance is done
+* via Trusted Foundations firmware. This is a no-op on Tegra114+.
+*/
+ if (trusted_foundations_registered())
+ outer_disable();
+
+ call_firmware_op(prepare_idle, TF_PM_MODE_LP1);
+
setup_mm_for_reboot();
tegra_sleep_core_finish(v2p);
@@ -342,6 +383,14 @@ static int tegra_suspend_enter(suspend_state_t state)
cpu_suspend(PHYS_OFFSET - PAGE_OFFSET, tegra_sleep_func);
+ /*
+* Resume L2 cache if it wasn't re-enabled early during resume,
+* which is the case for Tegra30 that has to re-enable the cache
+* via firmware call. In other cases cache is already enabled and
+* hence re-enabling is a no-op.
+*/
+ outer_resume();
+
switch (mode) {
case TEGRA_SUSPEND_LP1:
tegra_suspend_exit_lp1();
diff --git a/arch/arm/mach-tegra/reset-handler.S
b/arch/arm/mach-tegra/reset-handler.S
index 31fb53f9ce13..cd94d7c41fc0 100644
--- a/arch/arm/mach-tegra/reset-handler.S
+++ b/arch/arm/mach-tegra/reset-handler.S
@@ -20,6 +20,7 @@
#include
#include
+#include
#include
#include
@@ -76,6 +77,7 @@ ENTRY(tegra_resume)
orr r1, r1, #1
str r1, [r0]
#endif
+ bl tegra_resume_trusted_foundations
#ifdef CONFIG_CACHE_L2X0
/* L2 cache resume & re-enable */
@@ -88,6 +90,30 @@ end_ca9_scu_l2_resume:
b cpu_resume
ENDPROC(tegra_resume)
+
+/*
+ * tegra_resume_trusted_foundations
+ *
+ * Trusted Foundations firmware initialization.
+ *
+ * Doesn't return if firmware presents.
+ * Corrupted registers: r1, r2
+ */
+ENTRY(tegra_resume_trusted_foundations)
+ /* Check whether Trusted Foundations firmware presents. */
+ mov32 r2, TEGRA_IRAM_BASE + TEGRA_IRAM_RESET_HANDLER_OFFSET
+ ldr r1, =__tegra_cpu_reset_handler_data_offset + \
+ RESET_DATA(TF_PRESENT)
+ ldr r1, [r2, r1]
+ cmp r1, #0
+ reteq lr
+
+ .arch_extension sec
+ /*
[PATCH 2/3] habanalabs: prevent host crash during suspend/resume
This patch fixes the implementation of suspend/resume of the device so that
upon resume of the device, the host won't crash due to PCI completion
timeout.
Upon suspend, the device is being reset due to PERST. Therefore, upon
resume, the driver must initialize the PCI controller as if the driver was
loaded. If the controller is not initialized and the device tries to
access the device through the PCI bars, the host will crash with PCI
completion timeout error.
Signed-off-by: Oded Gabbay
---
drivers/misc/habanalabs/device.c| 46 +++--
drivers/misc/habanalabs/goya/goya.c | 63 +
2 files changed, 43 insertions(+), 66 deletions(-)
diff --git a/drivers/misc/habanalabs/device.c b/drivers/misc/habanalabs/device.c
index 470d8005b50e..77d51be66c7e 100644
--- a/drivers/misc/habanalabs/device.c
+++ b/drivers/misc/habanalabs/device.c
@@ -416,6 +416,27 @@ int hl_device_suspend(struct hl_device *hdev)
pci_save_state(hdev->pdev);
+ /* Block future CS/VM/JOB completion operations */
+ rc = atomic_cmpxchg(&hdev->in_reset, 0, 1);
+ if (rc) {
+ dev_err(hdev->dev, "Can't suspend while in reset\n");
+ return -EIO;
+ }
+
+ /* This blocks all other stuff that is not blocked by in_reset */
+ hdev->disabled = true;
+
+ /*
+* Flush anyone that is inside the critical section of enqueue
+* jobs to the H/W
+*/
+ hdev->asic_funcs->hw_queues_lock(hdev);
+ hdev->asic_funcs->hw_queues_unlock(hdev);
+
+ /* Flush processes that are sending message to CPU */
+ mutex_lock(&hdev->send_cpu_message_lock);
+ mutex_unlock(&hdev->send_cpu_message_lock);
+
rc = hdev->asic_funcs->suspend(hdev);
if (rc)
dev_err(hdev->dev,
@@ -443,21 +464,38 @@ int hl_device_resume(struct hl_device *hdev)
pci_set_power_state(hdev->pdev, PCI_D0);
pci_restore_state(hdev->pdev);
- rc = pci_enable_device(hdev->pdev);
+ rc = pci_enable_device_mem(hdev->pdev);
if (rc) {
dev_err(hdev->dev,
"Failed to enable PCI device in resume\n");
return rc;
}
+ pci_set_master(hdev->pdev);
+
rc = hdev->asic_funcs->resume(hdev);
if (rc) {
- dev_err(hdev->dev,
- "Failed to enable PCI access from device CPU\n");
- return rc;
+ dev_err(hdev->dev, "Failed to resume device after suspend\n");
+ goto disable_device;
+ }
+
+
+ hdev->disabled = false;
+ atomic_set(&hdev->in_reset, 0);
+
+ rc = hl_device_reset(hdev, true, false);
+ if (rc) {
+ dev_err(hdev->dev, "Failed to reset device during resume\n");
+ goto disable_device;
}
return 0;
+
+disable_device:
+ pci_clear_master(hdev->pdev);
+ pci_disable_device(hdev->pdev);
+
+ return rc;
}
static void hl_device_hard_reset_pending(struct work_struct *work)
diff --git a/drivers/misc/habanalabs/goya/goya.c
b/drivers/misc/habanalabs/goya/goya.c
index 238dd57c541b..538d8d59d9dc 100644
--- a/drivers/misc/habanalabs/goya/goya.c
+++ b/drivers/misc/habanalabs/goya/goya.c
@@ -1201,15 +1201,6 @@ static int goya_stop_external_queues(struct hl_device
*hdev)
return retval;
}
-static void goya_resume_external_queues(struct hl_device *hdev)
-{
- WREG32(mmDMA_QM_0_GLBL_CFG1, 0);
- WREG32(mmDMA_QM_1_GLBL_CFG1, 0);
- WREG32(mmDMA_QM_2_GLBL_CFG1, 0);
- WREG32(mmDMA_QM_3_GLBL_CFG1, 0);
- WREG32(mmDMA_QM_4_GLBL_CFG1, 0);
-}
-
/*
* goya_init_cpu_queues - Initialize PQ/CQ/EQ of CPU
*
@@ -2178,36 +2169,6 @@ static int goya_stop_internal_queues(struct hl_device
*hdev)
return retval;
}
-static void goya_resume_internal_queues(struct hl_device *hdev)
-{
- WREG32(mmMME_QM_GLBL_CFG1, 0);
- WREG32(mmMME_CMDQ_GLBL_CFG1, 0);
-
- WREG32(mmTPC0_QM_GLBL_CFG1, 0);
- WREG32(mmTPC0_CMDQ_GLBL_CFG1, 0);
-
- WREG32(mmTPC1_QM_GLBL_CFG1, 0);
- WREG32(mmTPC1_CMDQ_GLBL_CFG1, 0);
-
- WREG32(mmTPC2_QM_GLBL_CFG1, 0);
- WREG32(mmTPC2_CMDQ_GLBL_CFG1, 0);
-
- WREG32(mmTPC3_QM_GLBL_CFG1, 0);
- WREG32(mmTPC3_CMDQ_GLBL_CFG1, 0);
-
- WREG32(mmTPC4_QM_GLBL_CFG1, 0);
- WREG32(mmTPC4_CMDQ_GLBL_CFG1, 0);
-
- WREG32(mmTPC5_QM_GLBL_CFG1, 0);
- WREG32(mmTPC5_CMDQ_GLBL_CFG1, 0);
-
- WREG32(mmTPC6_QM_GLBL_CFG1, 0);
- WREG32(mmTPC6_CMDQ_GLBL_CFG1, 0);
-
- WREG32(mmTPC7_QM_GLBL_CFG1, 0);
- WREG32(mmTPC7_CMDQ_GLBL_CFG1, 0);
-}
-
static void goya_dma_stall(struct hl_device *hdev)
{
WREG32(mmDMA_QM_0_GLBL_CFG1, 1 << DMA_QM_0_GLBL_CFG1_DMA_STOP_SHIFT);
@@ -2905,20 +2866,
Re: [PATCH] rtc: cros-ec: Fail suspend/resume if wake IRQ can't be configured
Hi Stephen, On Fri, Mar 15, 2019 at 11:51:12AM -0700, Stephen Boyd wrote: > If we encounter a failure during suspend where this RTC was programmed > to wakeup the system from suspend, but that wakeup couldn't be > configured because the system didn't support wakeup interrupts, we'll > run into the following warning: > > Unbalanced IRQ 166 wake disable > WARNING: CPU: 7 PID: 3071 at kernel/irq/manage.c:669 > irq_set_irq_wake+0x108/0x278 > > This happens because the suspend process isn't aborted when the RTC > fails to configure the wakeup IRQ. Instead, we continue suspending the > system and then another suspend callback fails the suspend process and > "unwinds" the previously suspended drivers by calling their resume > callbacks. When we get back to resuming this RTC driver, we'll call > disable_irq_wake() on an IRQ that hasn't been configured for wake. > > Let's just fail suspend/resume here if we can't configure the system to > wake and the user has chosen to wakeup with this device. This fixes this > warning and makes the code more robust in case there are systems out > there that can't wakeup from suspend on this line but the user has > chosen to do so. > > Cc: Enric Balletbo i Serra > Cc: Evan Green > Cc: Benson Leung > Cc: Guenter Roeck > Signed-off-by: Stephen Boyd Acked-By: Benson Leung -- Benson Leung Staff Software Engineer Chrome OS Kernel Google Inc. ble...@google.com Chromium OS Project ble...@chromium.org signature.asc Description: PGP signature
[PATCH] rtc: cros-ec: Fail suspend/resume if wake IRQ can't be configured
If we encounter a failure during suspend where this RTC was programmed to wakeup the system from suspend, but that wakeup couldn't be configured because the system didn't support wakeup interrupts, we'll run into the following warning: Unbalanced IRQ 166 wake disable WARNING: CPU: 7 PID: 3071 at kernel/irq/manage.c:669 irq_set_irq_wake+0x108/0x278 This happens because the suspend process isn't aborted when the RTC fails to configure the wakeup IRQ. Instead, we continue suspending the system and then another suspend callback fails the suspend process and "unwinds" the previously suspended drivers by calling their resume callbacks. When we get back to resuming this RTC driver, we'll call disable_irq_wake() on an IRQ that hasn't been configured for wake. Let's just fail suspend/resume here if we can't configure the system to wake and the user has chosen to wakeup with this device. This fixes this warning and makes the code more robust in case there are systems out there that can't wakeup from suspend on this line but the user has chosen to do so. Cc: Enric Balletbo i Serra Cc: Evan Green Cc: Benson Leung Cc: Guenter Roeck Signed-off-by: Stephen Boyd --- drivers/rtc/rtc-cros-ec.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/rtc/rtc-cros-ec.c b/drivers/rtc/rtc-cros-ec.c index e5444296075e..4d6bf9304ceb 100644 --- a/drivers/rtc/rtc-cros-ec.c +++ b/drivers/rtc/rtc-cros-ec.c @@ -298,7 +298,7 @@ static int cros_ec_rtc_suspend(struct device *dev) struct cros_ec_rtc *cros_ec_rtc = dev_get_drvdata(&pdev->dev); if (device_may_wakeup(dev)) - enable_irq_wake(cros_ec_rtc->cros_ec->irq); + return enable_irq_wake(cros_ec_rtc->cros_ec->irq); return 0; } @@ -309,7 +309,7 @@ static int cros_ec_rtc_resume(struct device *dev) struct cros_ec_rtc *cros_ec_rtc = dev_get_drvdata(&pdev->dev); if (device_may_wakeup(dev)) - disable_irq_wake(cros_ec_rtc->cros_ec->irq); + return disable_irq_wake(cros_ec_rtc->cros_ec->irq); return 0; } -- Sent by a computer through tubes
Applied "spi: spi-mem: stm32-qspi: add suspend/resume support" to the spi tree
The patch
spi: spi-mem: stm32-qspi: add suspend/resume support
has been applied to the spi tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git
All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying
to this mail.
Thanks,
Mark
>From 2e541b64ee5269278fde5c87953a9963a8219ed4 Mon Sep 17 00:00:00 2001
From: Ludovic Barre
Date: Fri, 8 Mar 2019 14:12:21 +0100
Subject: [PATCH] spi: spi-mem: stm32-qspi: add suspend/resume support
This patch adds suspend and resume support for spi-stm32-qspi
drivers.
Signed-off-by: Ludovic Barre
Signed-off-by: Mark Brown
---
drivers/spi/spi-stm32-qspi.c | 39
1 file changed, 35 insertions(+), 4 deletions(-)
diff --git a/drivers/spi/spi-stm32-qspi.c b/drivers/spi/spi-stm32-qspi.c
index 7354f9d68dba..3e8ca10011cc 100644
--- a/drivers/spi/spi-stm32-qspi.c
+++ b/drivers/spi/spi-stm32-qspi.c
@@ -13,6 +13,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -101,6 +102,9 @@ struct stm32_qspi {
struct completion data_completion;
u32 fmode;
+ u32 cr_reg;
+ u32 dcr_reg;
+
/*
* to protect device configuration, could be different between
* 2 flash access (bk1, bk2)
@@ -355,7 +359,7 @@ static int stm32_qspi_setup(struct spi_device *spi)
struct spi_controller *ctrl = spi->master;
struct stm32_qspi *qspi = spi_controller_get_devdata(ctrl);
struct stm32_qspi_flash *flash;
- u32 cr, presc;
+ u32 presc;
if (ctrl->busy)
return -EBUSY;
@@ -371,11 +375,12 @@ static int stm32_qspi_setup(struct spi_device *spi)
flash->presc = presc;
mutex_lock(&qspi->lock);
- cr = FIELD_PREP(CR_FTHRES_MASK, 3) | CR_SSHIFT | CR_EN;
- writel_relaxed(cr, qspi->io_base + QSPI_CR);
+ qspi->cr_reg = FIELD_PREP(CR_FTHRES_MASK, 3) | CR_SSHIFT | CR_EN;
+ writel_relaxed(qspi->cr_reg, qspi->io_base + QSPI_CR);
/* set dcr fsize to max address */
- writel_relaxed(DCR_FSIZE_MASK, qspi->io_base + QSPI_DCR);
+ qspi->dcr_reg = DCR_FSIZE_MASK;
+ writel_relaxed(qspi->dcr_reg, qspi->io_base + QSPI_DCR);
mutex_unlock(&qspi->lock);
return 0;
@@ -489,6 +494,31 @@ static int stm32_qspi_remove(struct platform_device *pdev)
return 0;
}
+static int __maybe_unused stm32_qspi_suspend(struct device *dev)
+{
+ struct stm32_qspi *qspi = dev_get_drvdata(dev);
+
+ clk_disable_unprepare(qspi->clk);
+ pinctrl_pm_select_sleep_state(dev);
+
+ return 0;
+}
+
+static int __maybe_unused stm32_qspi_resume(struct device *dev)
+{
+ struct stm32_qspi *qspi = dev_get_drvdata(dev);
+
+ pinctrl_pm_select_default_state(dev);
+ clk_prepare_enable(qspi->clk);
+
+ writel_relaxed(qspi->cr_reg, qspi->io_base + QSPI_CR);
+ writel_relaxed(qspi->dcr_reg, qspi->io_base + QSPI_DCR);
+
+ return 0;
+}
+
+SIMPLE_DEV_PM_OPS(stm32_qspi_pm_ops, stm32_qspi_suspend, stm32_qspi_resume);
+
static const struct of_device_id stm32_qspi_match[] = {
{.compatible = "st,stm32f469-qspi"},
{}
@@ -501,6 +531,7 @@ static struct platform_driver stm32_qspi_driver = {
.driver = {
.name = "stm32-qspi",
.of_match_table = stm32_qspi_match,
+ .pm = &stm32_qspi_pm_ops,
},
};
module_platform_driver(stm32_qspi_driver);
--
2.20.1
Re: [Patch 1/1] drm/atomic: integrate private objects with suspend/resume helpers
Ville Syrjälä wrote on Thu [2019-Mar-14
17:31:29 +0200]:
> On Thu, Mar 14, 2019 at 08:44:45AM -0500, Benoit Parrot wrote:
> > During a suspend cycle the atomic state is saved to be used during the
> > restore cycle.
> >
> > However the current state duplication logic does not duplicate private
> > objects. This leads to state inconsistencies at resume time.
> >
> > With private objects modeset lock now integrated, we can make sure that
> > private object state are properly saved and restored.
> >
> > Signed-off-by: Benoit Parrot
> > ---
> > drivers/gpu/drm/drm_atomic_helper.c | 16
> > 1 file changed, 16 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> > b/drivers/gpu/drm/drm_atomic_helper.c
> > index 540a77a2ade9..b108021cc092 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -3189,6 +3189,7 @@ drm_atomic_helper_duplicate_state(struct drm_device
> > *dev,
> > struct drm_connector_list_iter conn_iter;
> > struct drm_plane *plane;
> > struct drm_crtc *crtc;
> > + struct drm_private_obj *privobj;
> > int err = 0;
> >
> > state = drm_atomic_state_alloc(dev);
> > @@ -3218,6 +3219,16 @@ drm_atomic_helper_duplicate_state(struct drm_device
> > *dev,
> > }
> > }
> >
> > + drm_for_each_privobj(privobj, dev) {
> > + struct drm_private_state *priv_state;
> > +
> > + priv_state = drm_atomic_get_private_obj_state(state, privobj);
> > + if (IS_ERR(priv_state)) {
> > + err = PTR_ERR(priv_state);
> > + goto free;
> > + }
> > + }
> > +
> > drm_connector_list_iter_begin(dev, &conn_iter);
> > drm_for_each_connector_iter(conn, &conn_iter) {
> > struct drm_connector_state *conn_state;
> > @@ -3325,12 +3336,17 @@ int
> > drm_atomic_helper_commit_duplicated_state(struct drm_atomic_state *state,
> > struct drm_connector_state *new_conn_state;
> > struct drm_crtc *crtc;
> > struct drm_crtc_state *new_crtc_state;
> > + struct drm_private_obj *privobj;
> > + struct drm_private_state *new_priv_state;
> >
> > state->acquire_ctx = ctx;
> >
> > for_each_new_plane_in_state(state, plane, new_plane_state, i)
> > state->planes[i].old_state = plane->state;
> >
> > + for_each_new_private_obj_in_state(state, privobj, new_priv_state, i)
> > + state->private_objs[i].old_state = privobj->state;
> > +
> > for_each_new_crtc_in_state(state, crtc, new_crtc_state, i)
> > state->crtcs[i].old_state = crtc->state;
>
> Random order between crtc vs. plane vs. connector vs. priv is tickling
> my ocd nerve a bit.
These loops are independent from each other.
And even without this patch the order is different between
drm_atomic_helper_duplicate_state() and
drm_atomic_helper_commit_duplicated_state(). :)
Benoit
>
> Otherwise looks sensible to me.
> Reviewed-by: Ville Syrjälä
>
> >
> > --
> > 2.17.1
> >
> > ___
> > dri-devel mailing list
> > dri-de...@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Ville Syrjälä
> Intel
RE: [PATCH] mailbox: imx: keep MU irq working during suspend/resume
Ping...
Best Regards!
Anson Huang
> -Original Message-
> From: Anson Huang [mailto:anson.hu...@nxp.com]
> Sent: 2019年2月12日 20:40
> To: jassisinghb...@gmail.com; shawn...@kernel.org;
> s.ha...@pengutronix.de; ker...@pengutronix.de; feste...@gmail.com;
> linux-kernel@vger.kernel.org; linux-arm-ker...@lists.infradead.org
> Cc: dl-linux-imx
> Subject: [PATCH] mailbox: imx: keep MU irq working during suspend/resume
>
> During noirq suspend phase, mailbox MU irq will be masked but many
> drivers still need to communicate with system controller firmware via
> mailbox, if MU irq is masked, it will cause RPC timeout as below:
>
> [ 23.372103] imx-scu scu: RPC send msg timeout
>
> Setting MU irq to be wakeup source is NOT working as GIC driver does NOT
> have .irq_set_wake implemented, so to support suspend/resume, just make
> imx mailbox driver NOT suspend, since MU is always a wakeup source on
> i.MX platforms with system controller inside, and its power/clock is
> maintained by system controller, mailbox driver no need to manage them.
>
> Signed-off-by: Anson Huang
> ---
> drivers/mailbox/imx-mailbox.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mailbox/imx-mailbox.c b/drivers/mailbox/imx-mailbox.c
> index 774362a..85fc5b5 100644
> --- a/drivers/mailbox/imx-mailbox.c
> +++ b/drivers/mailbox/imx-mailbox.c
> @@ -187,8 +187,8 @@ static int imx_mu_startup(struct mbox_chan *chan)
> return 0;
> }
>
> - ret = request_irq(priv->irq, imx_mu_isr, IRQF_SHARED, cp->irq_desc,
> - chan);
> + ret = request_irq(priv->irq, imx_mu_isr, IRQF_SHARED |
> + IRQF_NO_SUSPEND, cp->irq_desc, chan);
> if (ret) {
> dev_err(priv->dev,
> "Unable to acquire IRQ %d\n", priv->irq);
> --
> 2.7.4
[PATCH 2/2] spi: spi-mem: stm32-qspi: add suspend/resume support
From: Ludovic Barre
This patch adds suspend and resume support for spi-stm32-qspi
drivers.
Signed-off-by: Ludovic Barre
---
drivers/spi/spi-stm32-qspi.c | 39 +++
1 file changed, 35 insertions(+), 4 deletions(-)
diff --git a/drivers/spi/spi-stm32-qspi.c b/drivers/spi/spi-stm32-qspi.c
index 7354f9d..3e8ca10 100644
--- a/drivers/spi/spi-stm32-qspi.c
+++ b/drivers/spi/spi-stm32-qspi.c
@@ -13,6 +13,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -101,6 +102,9 @@ struct stm32_qspi {
struct completion data_completion;
u32 fmode;
+ u32 cr_reg;
+ u32 dcr_reg;
+
/*
* to protect device configuration, could be different between
* 2 flash access (bk1, bk2)
@@ -355,7 +359,7 @@ static int stm32_qspi_setup(struct spi_device *spi)
struct spi_controller *ctrl = spi->master;
struct stm32_qspi *qspi = spi_controller_get_devdata(ctrl);
struct stm32_qspi_flash *flash;
- u32 cr, presc;
+ u32 presc;
if (ctrl->busy)
return -EBUSY;
@@ -371,11 +375,12 @@ static int stm32_qspi_setup(struct spi_device *spi)
flash->presc = presc;
mutex_lock(&qspi->lock);
- cr = FIELD_PREP(CR_FTHRES_MASK, 3) | CR_SSHIFT | CR_EN;
- writel_relaxed(cr, qspi->io_base + QSPI_CR);
+ qspi->cr_reg = FIELD_PREP(CR_FTHRES_MASK, 3) | CR_SSHIFT | CR_EN;
+ writel_relaxed(qspi->cr_reg, qspi->io_base + QSPI_CR);
/* set dcr fsize to max address */
- writel_relaxed(DCR_FSIZE_MASK, qspi->io_base + QSPI_DCR);
+ qspi->dcr_reg = DCR_FSIZE_MASK;
+ writel_relaxed(qspi->dcr_reg, qspi->io_base + QSPI_DCR);
mutex_unlock(&qspi->lock);
return 0;
@@ -489,6 +494,31 @@ static int stm32_qspi_remove(struct platform_device *pdev)
return 0;
}
+static int __maybe_unused stm32_qspi_suspend(struct device *dev)
+{
+ struct stm32_qspi *qspi = dev_get_drvdata(dev);
+
+ clk_disable_unprepare(qspi->clk);
+ pinctrl_pm_select_sleep_state(dev);
+
+ return 0;
+}
+
+static int __maybe_unused stm32_qspi_resume(struct device *dev)
+{
+ struct stm32_qspi *qspi = dev_get_drvdata(dev);
+
+ pinctrl_pm_select_default_state(dev);
+ clk_prepare_enable(qspi->clk);
+
+ writel_relaxed(qspi->cr_reg, qspi->io_base + QSPI_CR);
+ writel_relaxed(qspi->dcr_reg, qspi->io_base + QSPI_DCR);
+
+ return 0;
+}
+
+SIMPLE_DEV_PM_OPS(stm32_qspi_pm_ops, stm32_qspi_suspend, stm32_qspi_resume);
+
static const struct of_device_id stm32_qspi_match[] = {
{.compatible = "st,stm32f469-qspi"},
{}
@@ -501,6 +531,7 @@ static struct platform_driver stm32_qspi_driver = {
.driver = {
.name = "stm32-qspi",
.of_match_table = stm32_qspi_match,
+ .pm = &stm32_qspi_pm_ops,
},
};
module_platform_driver(stm32_qspi_driver);
--
2.7.4
Re: [PATCH net-next v2 0/3] net: phy: marvell10g: implement suspend/resume callbacks
From: Antoine Tenart Date: Fri, 1 Mar 2019 12:00:44 +0100 > This series implements the suspend/resume callbacks in the marvell10g > PHY driver: > > - When the PHY isn't used, it is set in low power mode. > - At boot time we might now know the PHY status (as it's depending on > the hardware configuration, or on what the previous stages > configured), it is forced in low power. > > Doing this prevents a PHY which was initialized in a previous stage from > negotiating with the link partner when a local port is down. If the PHY > isn't shutdown when a port is not used, the link partner's PHY may > report the link being up while it's not. Looks like this needs some more discussion and Russell has some concerns still. So deferring to the next merge window, sorry.
[PATCH v9 7/7] ARM: tegra: Add firmware calls required for suspend-resume on Tegra30
In order to suspend-resume CPU with Trusted Foundations firmware being
present on Tegra30, the LP1/LP2 boot vectors and CPU caches need to be
set up using the firmware calls and then suspend code shall avoid
re-disabling parts that were disabled by the firmware.
Tested-by: Robert Yang
Tested-by: Michał Mirosław
Signed-off-by: Dmitry Osipenko
---
arch/arm/mach-tegra/pm.c| 49 +
arch/arm/mach-tegra/reset-handler.S | 26 +++
arch/arm/mach-tegra/sleep.S | 14 ++---
3 files changed, 84 insertions(+), 5 deletions(-)
diff --git a/arch/arm/mach-tegra/pm.c b/arch/arm/mach-tegra/pm.c
index 1ad5719779b0..abf5f88778f4 100644
--- a/arch/arm/mach-tegra/pm.c
+++ b/arch/arm/mach-tegra/pm.c
@@ -33,11 +33,13 @@
#include
#include
+#include
#include
#include
#include
#include
#include
+#include
#include "iomap.h"
#include "pm.h"
@@ -159,6 +161,28 @@ int tegra_cpu_do_idle(void)
static int tegra_sleep_cpu(unsigned long v2p)
{
+ /*
+* L2 cache disabling using kernel API only allowed when all
+* secondary CPU's are offline. Cache have to be disabled with
+* MMU-on if cache maintenance is done via Trusted Foundations
+* firmware. Note that CPUIDLE won't ever enter powergate on Tegra30
+* if any of secondary CPU's is online and this is the LP2-idle
+* code-path only for Tegra20/30.
+*/
+ if (trusted_foundations_registered())
+ outer_disable();
+
+ /*
+* Note that besides of setting up CPU reset vector this firmware
+* call may also do the following, depending on the FW version:
+* 1) Disable L2. But this doesn't matter since we already
+* disabled the L2.
+* 2) Disable D-cache. This need to be taken into account in
+* particular by the tegra_disable_clean_inv_dcache() which
+* shall avoid the re-disable.
+*/
+ call_firmware_op(prepare_idle, TF_PM_MODE_LP2);
+
setup_mm_for_reboot();
tegra_sleep_cpu_finish(v2p);
@@ -197,6 +221,14 @@ void tegra_idle_lp2_last(void)
cpu_suspend(PHYS_OFFSET - PAGE_OFFSET, &tegra_sleep_cpu);
+ /*
+* Resume L2 cache if it wasn't re-enabled early during resume,
+* which is the case for Tegra30 that has to re-enable the cache
+* via firmware call. In other cases cache is already enabled and
+* hence re-enabling is a no-op. This is always a no-op on Tegra114+.
+*/
+ outer_resume();
+
restore_cpu_complex();
cpu_cluster_pm_exit();
}
@@ -215,6 +247,15 @@ enum tegra_suspend_mode tegra_pm_validate_suspend_mode(
static int tegra_sleep_core(unsigned long v2p)
{
+ /*
+* Cache have to be disabled with MMU-on if cache maintenance is done
+* via Trusted Foundations firmware. This is a no-op on Tegra114+.
+*/
+ if (trusted_foundations_registered())
+ outer_disable();
+
+ call_firmware_op(prepare_idle, TF_PM_MODE_LP1);
+
setup_mm_for_reboot();
tegra_sleep_core_finish(v2p);
@@ -342,6 +383,14 @@ static int tegra_suspend_enter(suspend_state_t state)
cpu_suspend(PHYS_OFFSET - PAGE_OFFSET, tegra_sleep_func);
+ /*
+* Resume L2 cache if it wasn't re-enabled early during resume,
+* which is the case for Tegra30 that has to re-enable the cache
+* via firmware call. In other cases cache is already enabled and
+* hence re-enabling is a no-op.
+*/
+ outer_resume();
+
switch (mode) {
case TEGRA_SUSPEND_LP1:
tegra_suspend_exit_lp1();
diff --git a/arch/arm/mach-tegra/reset-handler.S
b/arch/arm/mach-tegra/reset-handler.S
index 31fb53f9ce13..cd94d7c41fc0 100644
--- a/arch/arm/mach-tegra/reset-handler.S
+++ b/arch/arm/mach-tegra/reset-handler.S
@@ -20,6 +20,7 @@
#include
#include
+#include
#include
#include
@@ -76,6 +77,7 @@ ENTRY(tegra_resume)
orr r1, r1, #1
str r1, [r0]
#endif
+ bl tegra_resume_trusted_foundations
#ifdef CONFIG_CACHE_L2X0
/* L2 cache resume & re-enable */
@@ -88,6 +90,30 @@ end_ca9_scu_l2_resume:
b cpu_resume
ENDPROC(tegra_resume)
+
+/*
+ * tegra_resume_trusted_foundations
+ *
+ * Trusted Foundations firmware initialization.
+ *
+ * Doesn't return if firmware presents.
+ * Corrupted registers: r1, r2
+ */
+ENTRY(tegra_resume_trusted_foundations)
+ /* Check whether Trusted Foundations firmware presents. */
+ mov32 r2, TEGRA_IRAM_BASE + TEGRA_IRAM_RESET_HANDLER_OFFSET
+ ldr r1, =__tegra_cpu_reset_handler_data_offset + \
+ RESET_DATA(TF_PRESENT)
+ ldr r1, [r2, r1]
+ cmp r1, #0
+ reteq lr
+
+ .arch_extension sec
+ /*
Re: [PATCH net-next v2 1/3] net: phy: marvell10g: implement suspend/resume callbacks
On Fri, Mar 01, 2019 at 12:00:45PM +0100, Antoine Tenart wrote: > This patch adds the suspend/resume callbacks for Marvell 10G PHYs. The > three PCS (base-t, base-r and 1000base-x) are set in low power (the PCS > are powered down) when the PHY isn't used. > > Signed-off-by: Antoine Tenart Reviewed-by: Andrew Lunn Andrew
Re: [PATCH net-next v2 2/3] net: phy: marvell10g: add the suspend/resume callbacks for the 88x2210
On Fri, Mar 01, 2019 at 12:00:46PM +0100, Antoine Tenart wrote: > When the 88x2110 PHY support was added, the suspend and resume callbacks > were forgotten. This patch adds them to the 88x2110 PHY callback > definition. > > Signed-off-by: Antoine Tenart Reviewed-by: Andrew Lunn Andrew
[PATCH net-next v2 1/3] net: phy: marvell10g: implement suspend/resume callbacks
This patch adds the suspend/resume callbacks for Marvell 10G PHYs. The
three PCS (base-t, base-r and 1000base-x) are set in low power (the PCS
are powered down) when the PHY isn't used.
Signed-off-by: Antoine Tenart
---
drivers/net/phy/marvell10g.c | 14 ++
1 file changed, 14 insertions(+)
diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
index 79106e70010f..92462344c5ad 100644
--- a/drivers/net/phy/marvell10g.c
+++ b/drivers/net/phy/marvell10g.c
@@ -226,11 +226,25 @@ static int mv3310_probe(struct phy_device *phydev)
static int mv3310_suspend(struct phy_device *phydev)
{
+ phy_modify_mmd(phydev, MDIO_MMD_PCS, MV_PCS_BASE_T + MDIO_CTRL1,
+ MDIO_CTRL1_LPOWER, MDIO_CTRL1_LPOWER);
+ phy_modify_mmd(phydev, MDIO_MMD_PCS, MV_PCS_BASE_R + MDIO_CTRL1,
+ MDIO_CTRL1_LPOWER, MDIO_CTRL1_LPOWER);
+ phy_modify_mmd(phydev, MDIO_MMD_PCS, MV_PCS_1000BASEX + MDIO_CTRL1,
+ MDIO_CTRL1_LPOWER, MDIO_CTRL1_LPOWER);
+
return 0;
}
static int mv3310_resume(struct phy_device *phydev)
{
+ phy_modify_mmd(phydev, MDIO_MMD_PCS, MV_PCS_BASE_T + MDIO_CTRL1,
+ MDIO_CTRL1_LPOWER, 0);
+ phy_modify_mmd(phydev, MDIO_MMD_PCS, MV_PCS_BASE_R + MDIO_CTRL1,
+ MDIO_CTRL1_LPOWER, 0);
+ phy_modify_mmd(phydev, MDIO_MMD_PCS, MV_PCS_1000BASEX + MDIO_CTRL1,
+ MDIO_CTRL1_LPOWER, 0);
+
return mv3310_hwmon_config(phydev, true);
}
--
2.20.1
[PATCH net-next v2 2/3] net: phy: marvell10g: add the suspend/resume callbacks for the 88x2210
When the 88x2110 PHY support was added, the suspend and resume callbacks
were forgotten. This patch adds them to the 88x2110 PHY callback
definition.
Signed-off-by: Antoine Tenart
---
drivers/net/phy/marvell10g.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
index 92462344c5ad..4f6f96080182 100644
--- a/drivers/net/phy/marvell10g.c
+++ b/drivers/net/phy/marvell10g.c
@@ -488,6 +488,8 @@ static struct phy_driver mv3310_drivers[] = {
.name = "mv88x2110",
.get_features = genphy_c45_pma_read_abilities,
.probe = mv3310_probe,
+ .suspend= mv3310_suspend,
+ .resume = mv3310_resume,
.soft_reset = gen10g_no_soft_reset,
.config_init= mv3310_config_init,
.config_aneg= mv3310_config_aneg,
--
2.20.1
[PATCH net-next v2 0/3] net: phy: marvell10g: implement suspend/resume callbacks
Hello, This series implements the suspend/resume callbacks in the marvell10g PHY driver: - When the PHY isn't used, it is set in low power mode. - At boot time we might now know the PHY status (as it's depending on the hardware configuration, or on what the previous stages configured), it is forced in low power. Doing this prevents a PHY which was initialized in a previous stage from negotiating with the link partner when a local port is down. If the PHY isn't shutdown when a port is not used, the link partner's PHY may report the link being up while it's not. Thanks, Antoine Since v1: - Fixed a mix up in the patches where two implementations of the suspend/resume callbacks were kept in the driver. - Rebased on the latest net-next. Antoine Tenart (3): net: phy: marvell10g: implement suspend/resume callbacks net: phy: marvell10g: add the suspend/resume callbacks for the 88x2210 net: phy: marvell10g: set the PHY in low power by default drivers/net/phy/marvell10g.c | 39 ++-- 1 file changed, 28 insertions(+), 11 deletions(-) -- 2.20.1
[PATCH v3 4/4] net: macb: Add support for suspend/resume with full power down
When macb device is suspended and system is powered down, the clocks
are removed and hence macb should be closed gracefully and restored
upon resume. This patch does the same by switching off the net device,
suspending phy and performing necessary cleanup of interrupts and BDs.
Upon resume, all these are reinitialized again.
Reset of macb device is done only when GEM is not a wake device.
Even when gem is a wake device, tx queues can be stopped and ptp device
can be closed (tsu clock will be disabled in pm_runtime_suspend) as
wake event detection has no dependency on this.
Signed-off-by: Kedareswara rao Appana
Signed-off-by: Harini Katakam
---
v3:
Fix >80 char lines
v2 changes:
Fixed parameter passed to phy calls.
drivers/net/ethernet/cadence/macb_main.c | 40 ++--
1 file changed, 38 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/cadence/macb_main.c
b/drivers/net/ethernet/cadence/macb_main.c
index 5173c02..ad099fd 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -4283,16 +4283,34 @@ static int __maybe_unused macb_suspend(struct device
*dev)
{
struct net_device *netdev = dev_get_drvdata(dev);
struct macb *bp = netdev_priv(netdev);
+ struct macb_queue *queue = bp->queues;
+ unsigned long flags;
+ unsigned int q;
+
+ if (!netif_running(netdev))
+ return 0;
- netif_carrier_off(netdev);
- netif_device_detach(netdev);
if (bp->wol & MACB_WOL_ENABLED) {
macb_writel(bp, IER, MACB_BIT(WOL));
macb_writel(bp, WOL, MACB_BIT(MAG));
enable_irq_wake(bp->queues[0].irq);
+ netif_device_detach(netdev);
+ } else {
+ netif_device_detach(netdev);
+ for (q = 0, queue = bp->queues; q < bp->num_queues;
+++q, ++queue)
+ napi_disable(&queue->napi);
+ phy_stop(netdev->phydev);
+ phy_suspend(netdev->phydev);
+ spin_lock_irqsave(&bp->lock, flags);
+ macb_reset_hw(bp);
+ spin_unlock_irqrestore(&bp->lock, flags);
}
+ netif_carrier_off(netdev);
+ if (bp->ptp_info)
+ bp->ptp_info->ptp_remove(netdev);
pm_runtime_force_suspend(dev);
return 0;
@@ -4302,6 +4320,11 @@ static int __maybe_unused macb_resume(struct device *dev)
{
struct net_device *netdev = dev_get_drvdata(dev);
struct macb *bp = netdev_priv(netdev);
+ struct macb_queue *queue = bp->queues;
+ unsigned int q;
+
+ if (!netif_running(netdev))
+ return 0;
pm_runtime_force_resume(dev);
@@ -4309,9 +4332,22 @@ static int __maybe_unused macb_resume(struct device *dev)
macb_writel(bp, IDR, MACB_BIT(WOL));
macb_writel(bp, WOL, 0);
disable_irq_wake(bp->queues[0].irq);
+ } else {
+ macb_writel(bp, NCR, MACB_BIT(MPE));
+ for (q = 0, queue = bp->queues; q < bp->num_queues;
+++q, ++queue)
+ napi_enable(&queue->napi);
+ phy_resume(netdev->phydev);
+ phy_init_hw(netdev->phydev);
+ phy_start(netdev->phydev);
}
+ bp->macbgem_ops.mog_init_rings(bp);
+ macb_init_hw(bp);
+ macb_set_rx_mode(netdev);
netif_device_attach(netdev);
+ if (bp->ptp_info)
+ bp->ptp_info->ptp_init(netdev);
return 0;
}
--
2.7.4
[PATCH v8 7/7] ARM: tegra: Add firmware calls required for suspend-resume on Tegra30
In order to suspend-resume CPU with Trusted Foundations firmware being
present on Tegra30, the LP1/LP2 boot vectors and CPU caches need to be
set up using the firmware calls and then suspend code shall avoid
re-disabling parts that were disabled by the firmware.
Tested-by: Robert Yang
Tested-by: Michał Mirosław
Signed-off-by: Dmitry Osipenko
---
arch/arm/mach-tegra/pm.c| 49 +
arch/arm/mach-tegra/reset-handler.S | 26 +++
arch/arm/mach-tegra/sleep.S | 14 ++---
3 files changed, 84 insertions(+), 5 deletions(-)
diff --git a/arch/arm/mach-tegra/pm.c b/arch/arm/mach-tegra/pm.c
index 1ad5719779b0..abf5f88778f4 100644
--- a/arch/arm/mach-tegra/pm.c
+++ b/arch/arm/mach-tegra/pm.c
@@ -33,11 +33,13 @@
#include
#include
+#include
#include
#include
#include
#include
#include
+#include
#include "iomap.h"
#include "pm.h"
@@ -159,6 +161,28 @@ int tegra_cpu_do_idle(void)
static int tegra_sleep_cpu(unsigned long v2p)
{
+ /*
+* L2 cache disabling using kernel API only allowed when all
+* secondary CPU's are offline. Cache have to be disabled with
+* MMU-on if cache maintenance is done via Trusted Foundations
+* firmware. Note that CPUIDLE won't ever enter powergate on Tegra30
+* if any of secondary CPU's is online and this is the LP2-idle
+* code-path only for Tegra20/30.
+*/
+ if (trusted_foundations_registered())
+ outer_disable();
+
+ /*
+* Note that besides of setting up CPU reset vector this firmware
+* call may also do the following, depending on the FW version:
+* 1) Disable L2. But this doesn't matter since we already
+* disabled the L2.
+* 2) Disable D-cache. This need to be taken into account in
+* particular by the tegra_disable_clean_inv_dcache() which
+* shall avoid the re-disable.
+*/
+ call_firmware_op(prepare_idle, TF_PM_MODE_LP2);
+
setup_mm_for_reboot();
tegra_sleep_cpu_finish(v2p);
@@ -197,6 +221,14 @@ void tegra_idle_lp2_last(void)
cpu_suspend(PHYS_OFFSET - PAGE_OFFSET, &tegra_sleep_cpu);
+ /*
+* Resume L2 cache if it wasn't re-enabled early during resume,
+* which is the case for Tegra30 that has to re-enable the cache
+* via firmware call. In other cases cache is already enabled and
+* hence re-enabling is a no-op. This is always a no-op on Tegra114+.
+*/
+ outer_resume();
+
restore_cpu_complex();
cpu_cluster_pm_exit();
}
@@ -215,6 +247,15 @@ enum tegra_suspend_mode tegra_pm_validate_suspend_mode(
static int tegra_sleep_core(unsigned long v2p)
{
+ /*
+* Cache have to be disabled with MMU-on if cache maintenance is done
+* via Trusted Foundations firmware. This is a no-op on Tegra114+.
+*/
+ if (trusted_foundations_registered())
+ outer_disable();
+
+ call_firmware_op(prepare_idle, TF_PM_MODE_LP1);
+
setup_mm_for_reboot();
tegra_sleep_core_finish(v2p);
@@ -342,6 +383,14 @@ static int tegra_suspend_enter(suspend_state_t state)
cpu_suspend(PHYS_OFFSET - PAGE_OFFSET, tegra_sleep_func);
+ /*
+* Resume L2 cache if it wasn't re-enabled early during resume,
+* which is the case for Tegra30 that has to re-enable the cache
+* via firmware call. In other cases cache is already enabled and
+* hence re-enabling is a no-op.
+*/
+ outer_resume();
+
switch (mode) {
case TEGRA_SUSPEND_LP1:
tegra_suspend_exit_lp1();
diff --git a/arch/arm/mach-tegra/reset-handler.S
b/arch/arm/mach-tegra/reset-handler.S
index 3bf202819534..19a609046547 100644
--- a/arch/arm/mach-tegra/reset-handler.S
+++ b/arch/arm/mach-tegra/reset-handler.S
@@ -20,6 +20,7 @@
#include
#include
+#include
#include
#include
@@ -76,6 +77,7 @@ ENTRY(tegra_resume)
orr r1, r1, #1
str r1, [r0]
#endif
+ bl tegra_resume_trusted_foundations
#ifdef CONFIG_CACHE_L2X0
/* L2 cache resume & re-enable */
@@ -88,6 +90,30 @@ end_ca9_scu_l2_resume:
b cpu_resume
ENDPROC(tegra_resume)
+
+/*
+ * tegra_resume_trusted_foundations
+ *
+ * Trusted Foundations firmware initialization.
+ *
+ * Doesn't return if firmware presents.
+ * Corrupted registers: r1, r2
+ */
+ENTRY(tegra_resume_trusted_foundations)
+ /* Check whether Trusted Foundations firmware presents. */
+ mov32 r2, TEGRA_IRAM_BASE + TEGRA_IRAM_RESET_HANDLER_OFFSET
+ ldr r1, =__tegra_cpu_reset_handler_data_offset + \
+ RESET_DATA(TF_PRESENT)
+ ldr r1, [r2, r1]
+ cmp r1, #0
+ reteq lr
+
+ .arch_extension sec
+ /*
[PATCH net-next 0/3] net: phy: marvell10g: implement suspend/resume callbacks
Hello, This series implements the suspend/resume callbacks in the marvell10g PHY driver: - When the PHY isn't used, it is set in low power mode. - At boot time we might now know the PHY status (as it's depending on the hardware configuration, or on what the previous stages configured), it is forced in low power. Doing this prevents a PHY which was initialized in a previous stage from negotiating with the link partner when a local port is down. If the PHY isn't shutdown when a port is not used, the link partner's PHY may report the link being up while it's not. Thanks, Antoine Antoine Tenart (3): net: phy: marvell10g: implement suspend/resume callbacks net: phy: marvell10g: add the suspend/resume callbacks for the 88x2210 net: phy: marvell10g: set the PHY in low power by default drivers/net/phy/marvell10g.c | 39 ++-- 1 file changed, 28 insertions(+), 11 deletions(-) -- 2.20.1
[PATCH net-next 1/3] net: phy: marvell10g: implement suspend/resume callbacks
This patch adds the suspend/resume callbacks for Marvell 10G PHYs. The
three PCS (base-t, base-r and 1000base-x) are set in low power (the PCS
are powered down) when the PHY isn't used.
Signed-off-by: Antoine Tenart
---
drivers/net/phy/marvell10g.c | 24
1 file changed, 24 insertions(+)
diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
index 79106e70010f..95aeeda0eb16 100644
--- a/drivers/net/phy/marvell10g.c
+++ b/drivers/net/phy/marvell10g.c
@@ -201,6 +201,30 @@ static int mv3310_hwmon_probe(struct phy_device *phydev)
}
#endif
+static int mv3310_suspend(struct phy_device *phydev)
+{
+ phy_modify_mmd(phydev, MDIO_MMD_PCS, MV_PCS_BASE_T + MDIO_CTRL1,
+ MDIO_CTRL1_LPOWER, MDIO_CTRL1_LPOWER);
+ phy_modify_mmd(phydev, MDIO_MMD_PCS, MV_PCS_BASE_R + MDIO_CTRL1,
+ MDIO_CTRL1_LPOWER, MDIO_CTRL1_LPOWER);
+ phy_modify_mmd(phydev, MDIO_MMD_PCS, MV_PCS_1000BASEX + MDIO_CTRL1,
+ MDIO_CTRL1_LPOWER, MDIO_CTRL1_LPOWER);
+
+ return 0;
+}
+
+static int mv3310_resume(struct phy_device *phydev)
+{
+ phy_modify_mmd(phydev, MDIO_MMD_PCS, MV_PCS_BASE_T + MDIO_CTRL1,
+ MDIO_CTRL1_LPOWER, 0);
+ phy_modify_mmd(phydev, MDIO_MMD_PCS, MV_PCS_BASE_R + MDIO_CTRL1,
+ MDIO_CTRL1_LPOWER, 0);
+ phy_modify_mmd(phydev, MDIO_MMD_PCS, MV_PCS_1000BASEX + MDIO_CTRL1,
+ MDIO_CTRL1_LPOWER, 0);
+
+ return mv3310_hwmon_config(phydev, true);
+}
+
static int mv3310_probe(struct phy_device *phydev)
{
struct mv3310_priv *priv;
--
2.20.1
[PATCH net-next 2/3] net: phy: marvell10g: add the suspend/resume callbacks for the 88x2210
When the 88x2110 PHY support was added, the suspend and resume callbacks
were forgotten. This patch adds them to the 88x2110 PHY callback
definition.
Signed-off-by: Antoine Tenart
---
drivers/net/phy/marvell10g.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
index 95aeeda0eb16..e5d098bd33a6 100644
--- a/drivers/net/phy/marvell10g.c
+++ b/drivers/net/phy/marvell10g.c
@@ -498,6 +498,8 @@ static struct phy_driver mv3310_drivers[] = {
.name = "mv88x2110",
.get_features = genphy_c45_pma_read_abilities,
.probe = mv3310_probe,
+ .suspend= mv3310_suspend,
+ .resume = mv3310_resume,
.soft_reset = gen10g_no_soft_reset,
.config_init= mv3310_config_init,
.config_aneg= mv3310_config_aneg,
--
2.20.1
[PATCH v7 7/7] ARM: tegra: Add firmware calls required for suspend-resume on Tegra30
In order to suspend-resume CPU with Trusted Foundations firmware being
present on Tegra30, the LP1/LP2 boot vectors and CPU caches need to be
set up using the firmware calls and then suspend code shall avoid
re-disabling parts that were disabled by the firmware.
Tested-by: Robert Yang
Tested-by: Michał Mirosław
Signed-off-by: Dmitry Osipenko
---
arch/arm/mach-tegra/pm.c| 49 +
arch/arm/mach-tegra/reset-handler.S | 26 +++
arch/arm/mach-tegra/sleep.S | 14 ++---
3 files changed, 84 insertions(+), 5 deletions(-)
diff --git a/arch/arm/mach-tegra/pm.c b/arch/arm/mach-tegra/pm.c
index 1ad5719779b0..abf5f88778f4 100644
--- a/arch/arm/mach-tegra/pm.c
+++ b/arch/arm/mach-tegra/pm.c
@@ -33,11 +33,13 @@
#include
#include
+#include
#include
#include
#include
#include
#include
+#include
#include "iomap.h"
#include "pm.h"
@@ -159,6 +161,28 @@ int tegra_cpu_do_idle(void)
static int tegra_sleep_cpu(unsigned long v2p)
{
+ /*
+* L2 cache disabling using kernel API only allowed when all
+* secondary CPU's are offline. Cache have to be disabled with
+* MMU-on if cache maintenance is done via Trusted Foundations
+* firmware. Note that CPUIDLE won't ever enter powergate on Tegra30
+* if any of secondary CPU's is online and this is the LP2-idle
+* code-path only for Tegra20/30.
+*/
+ if (trusted_foundations_registered())
+ outer_disable();
+
+ /*
+* Note that besides of setting up CPU reset vector this firmware
+* call may also do the following, depending on the FW version:
+* 1) Disable L2. But this doesn't matter since we already
+* disabled the L2.
+* 2) Disable D-cache. This need to be taken into account in
+* particular by the tegra_disable_clean_inv_dcache() which
+* shall avoid the re-disable.
+*/
+ call_firmware_op(prepare_idle, TF_PM_MODE_LP2);
+
setup_mm_for_reboot();
tegra_sleep_cpu_finish(v2p);
@@ -197,6 +221,14 @@ void tegra_idle_lp2_last(void)
cpu_suspend(PHYS_OFFSET - PAGE_OFFSET, &tegra_sleep_cpu);
+ /*
+* Resume L2 cache if it wasn't re-enabled early during resume,
+* which is the case for Tegra30 that has to re-enable the cache
+* via firmware call. In other cases cache is already enabled and
+* hence re-enabling is a no-op. This is always a no-op on Tegra114+.
+*/
+ outer_resume();
+
restore_cpu_complex();
cpu_cluster_pm_exit();
}
@@ -215,6 +247,15 @@ enum tegra_suspend_mode tegra_pm_validate_suspend_mode(
static int tegra_sleep_core(unsigned long v2p)
{
+ /*
+* Cache have to be disabled with MMU-on if cache maintenance is done
+* via Trusted Foundations firmware. This is a no-op on Tegra114+.
+*/
+ if (trusted_foundations_registered())
+ outer_disable();
+
+ call_firmware_op(prepare_idle, TF_PM_MODE_LP1);
+
setup_mm_for_reboot();
tegra_sleep_core_finish(v2p);
@@ -342,6 +383,14 @@ static int tegra_suspend_enter(suspend_state_t state)
cpu_suspend(PHYS_OFFSET - PAGE_OFFSET, tegra_sleep_func);
+ /*
+* Resume L2 cache if it wasn't re-enabled early during resume,
+* which is the case for Tegra30 that has to re-enable the cache
+* via firmware call. In other cases cache is already enabled and
+* hence re-enabling is a no-op.
+*/
+ outer_resume();
+
switch (mode) {
case TEGRA_SUSPEND_LP1:
tegra_suspend_exit_lp1();
diff --git a/arch/arm/mach-tegra/reset-handler.S
b/arch/arm/mach-tegra/reset-handler.S
index 3bf202819534..19a609046547 100644
--- a/arch/arm/mach-tegra/reset-handler.S
+++ b/arch/arm/mach-tegra/reset-handler.S
@@ -20,6 +20,7 @@
#include
#include
+#include
#include
#include
@@ -76,6 +77,7 @@ ENTRY(tegra_resume)
orr r1, r1, #1
str r1, [r0]
#endif
+ bl tegra_resume_trusted_foundations
#ifdef CONFIG_CACHE_L2X0
/* L2 cache resume & re-enable */
@@ -88,6 +90,30 @@ end_ca9_scu_l2_resume:
b cpu_resume
ENDPROC(tegra_resume)
+
+/*
+ * tegra_resume_trusted_foundations
+ *
+ * Trusted Foundations firmware initialization.
+ *
+ * Doesn't return if firmware presents.
+ * Corrupted registers: r1, r2
+ */
+ENTRY(tegra_resume_trusted_foundations)
+ /* Check whether Trusted Foundations firmware presents. */
+ mov32 r2, TEGRA_IRAM_BASE + TEGRA_IRAM_RESET_HANDLER_OFFSET
+ ldr r1, =__tegra_cpu_reset_handler_data_offset + \
+ RESET_DATA(TF_PRESENT)
+ ldr r1, [r2, r1]
+ cmp r1, #0
+ reteq lr
+
+ .arch_extension sec
+ /*
[PATCH 4.20 005/183] mt76x0u: fix suspend/resume
4.20-stable review patch. If anyone has any objections, please let me know.
--
From: Stanislaw Gruszka
commit d04ca383860bef90a0dab4eb397907f7f05e839e upstream.
We need to reset MCU and do other initializations on resume otherwise
MT7610U device will fail to initialize, what cause system hung due to
USB requests timeouts.
Patch fixes 4.19 -> 4.20 regression.
Cc: sta...@vger.kernel.org # 4.20+
Signed-off-by: Stanislaw Gruszka
Acked-by: Lorenzo Bianconi
Signed-off-by: Kalle Valo
Signed-off-by: Greg Kroah-Hartman
---
drivers/net/wireless/mediatek/mt76/mt76x0/usb.c | 46 +++-
1 file changed, 29 insertions(+), 17 deletions(-)
--- a/drivers/net/wireless/mediatek/mt76/mt76x0/usb.c
+++ b/drivers/net/wireless/mediatek/mt76/mt76x0/usb.c
@@ -159,39 +159,49 @@ static const struct ieee80211_ops mt76x0
.wake_tx_queue = mt76_wake_tx_queue,
};
-static int mt76x0u_register_device(struct mt76x02_dev *dev)
+static int mt76x0u_init_hardware(struct mt76x02_dev *dev)
{
- struct ieee80211_hw *hw = dev->mt76.hw;
int err;
- err = mt76u_alloc_queues(&dev->mt76);
- if (err < 0)
- goto out_err;
-
- err = mt76u_mcu_init_rx(&dev->mt76);
- if (err < 0)
- goto out_err;
-
mt76x0_chip_onoff(dev, true, true);
- if (!mt76x02_wait_for_mac(&dev->mt76)) {
- err = -ETIMEDOUT;
- goto out_err;
- }
+
+ if (!mt76x02_wait_for_mac(&dev->mt76))
+ return -ETIMEDOUT;
err = mt76x0u_mcu_init(dev);
if (err < 0)
- goto out_err;
+ return err;
mt76x0_init_usb_dma(dev);
err = mt76x0_init_hardware(dev);
if (err < 0)
- goto out_err;
+ return err;
mt76_rmw(dev, MT_US_CYC_CFG, MT_US_CYC_CNT, 0x1e);
mt76_wr(dev, MT_TXOP_CTRL_CFG,
FIELD_PREP(MT_TXOP_TRUN_EN, 0x3f) |
FIELD_PREP(MT_TXOP_EXT_CCA_DLY, 0x58));
+ return 0;
+}
+
+static int mt76x0u_register_device(struct mt76x02_dev *dev)
+{
+ struct ieee80211_hw *hw = dev->mt76.hw;
+ int err;
+
+ err = mt76u_alloc_queues(&dev->mt76);
+ if (err < 0)
+ goto out_err;
+
+ err = mt76u_mcu_init_rx(&dev->mt76);
+ if (err < 0)
+ goto out_err;
+
+ err = mt76x0u_init_hardware(dev);
+ if (err < 0)
+ goto out_err;
+
err = mt76x0_register_device(dev);
if (err < 0)
goto out_err;
@@ -300,6 +310,8 @@ static int __maybe_unused mt76x0_suspend
mt76u_stop_queues(&dev->mt76);
mt76x0u_mac_stop(dev);
+ clear_bit(MT76_STATE_MCU_RUNNING, &dev->mt76.state);
+ mt76x0_chip_onoff(dev, false, false);
usb_kill_urb(usb->mcu.res.urb);
return 0;
@@ -327,7 +339,7 @@ static int __maybe_unused mt76x0_resume(
tasklet_enable(&usb->rx_tasklet);
tasklet_enable(&usb->tx_tasklet);
- ret = mt76x0_init_hardware(dev);
+ ret = mt76x0u_init_hardware(dev);
if (ret)
goto err;
[PATCH v6 7/7] ARM: tegra: Add firmware calls required for suspend-resume on Tegra30
In order to suspend-resume CPU with Trusted Foundations firmware being
present on Tegra30, the LP1/LP2 boot vectors and CPU caches need to be
set up using the firmware calls and then suspend code shall avoid
re-disabling parts that were disabled by the firmware.
Signed-off-by: Dmitry Osipenko
---
arch/arm/mach-tegra/pm.c| 49 +
arch/arm/mach-tegra/reset-handler.S | 26 +++
arch/arm/mach-tegra/sleep.S | 14 ++---
3 files changed, 84 insertions(+), 5 deletions(-)
diff --git a/arch/arm/mach-tegra/pm.c b/arch/arm/mach-tegra/pm.c
index 1ad5719779b0..f209f59e0daf 100644
--- a/arch/arm/mach-tegra/pm.c
+++ b/arch/arm/mach-tegra/pm.c
@@ -33,11 +33,13 @@
#include
#include
+#include
#include
#include
#include
#include
#include
+#include
#include "iomap.h"
#include "pm.h"
@@ -159,6 +161,28 @@ int tegra_cpu_do_idle(void)
static int tegra_sleep_cpu(unsigned long v2p)
{
+ /*
+* L2 cache disabling using kernel API only allowed when all
+* secondary CPU's are offline. Cache have to be disabled with
+* MMU-on if cache maintenance is done via Trusted Foundations
+* firmware. Note that CPUIDLE won't ever enter powergate on Tegra30
+* if any of secondary CPU's is online and this is the LP2-idle
+* code-path only for Tegra20/30.
+*/
+ if (trusted_foundations_registered())
+ outer_disable();
+
+ /*
+* Note that besides of setting up CPU reset vector this firmware
+* call may also do the following, depending on the FW version:
+* 1) Disable L2. But this doesn't matter since we already
+* disabled the L2.
+* 2) Disable D-cache. This need to be taken into account in
+* particular by the tegra_disable_clean_inv_dcache() which
+* shall avoid the re-disable.
+*/
+ call_firmware_op(prepare_idle, TF_PM_MODE_LP2);
+
setup_mm_for_reboot();
tegra_sleep_cpu_finish(v2p);
@@ -197,6 +221,14 @@ void tegra_idle_lp2_last(void)
cpu_suspend(PHYS_OFFSET - PAGE_OFFSET, &tegra_sleep_cpu);
+ /*
+* Resume L2 cache if it wasn't re-enabled early during resume,
+* which is the case for Tegra30 that has to re-enable the cache
+* via firmware call. In other cases cache is already enabled and
+* hence re-enabling is a no-op. This is always a no-op on Tegra114+.
+*/
+ outer_resume();
+
restore_cpu_complex();
cpu_cluster_pm_exit();
}
@@ -215,6 +247,15 @@ enum tegra_suspend_mode tegra_pm_validate_suspend_mode(
static int tegra_sleep_core(unsigned long v2p)
{
+ /*
+* Cache have to be disabled with MMU-on if cache maintenance is done
+* via Trusted Foundations firmware. This is a no-op on Tegra114+.
+*/
+ if (trusted_foundations_registered())
+ outer_disable();
+
+ call_firmware_op(prepare_idle, TF_PM_MODE_LP1);
+
setup_mm_for_reboot();
tegra_sleep_core_finish(v2p);
@@ -342,6 +383,14 @@ static int tegra_suspend_enter(suspend_state_t state)
cpu_suspend(PHYS_OFFSET - PAGE_OFFSET, tegra_sleep_func);
+ /*
+* Resume L2 cache if it wasn't re-enabled early during resume,
+* which is the case for Tegra30 that has to re-enable the cache
+* via firmware call. In other cases cache is already enabled and
+* hence re-enabling is a no-op.
+*/
+ outer_resume();
+
switch (mode) {
case TEGRA_SUSPEND_LP1:
tegra_suspend_exit_lp1();
diff --git a/arch/arm/mach-tegra/reset-handler.S
b/arch/arm/mach-tegra/reset-handler.S
index 3bf202819534..19a609046547 100644
--- a/arch/arm/mach-tegra/reset-handler.S
+++ b/arch/arm/mach-tegra/reset-handler.S
@@ -20,6 +20,7 @@
#include
#include
+#include
#include
#include
@@ -76,6 +77,7 @@ ENTRY(tegra_resume)
orr r1, r1, #1
str r1, [r0]
#endif
+ bl tegra_resume_trusted_foundations
#ifdef CONFIG_CACHE_L2X0
/* L2 cache resume & re-enable */
@@ -88,6 +90,30 @@ end_ca9_scu_l2_resume:
b cpu_resume
ENDPROC(tegra_resume)
+
+/*
+ * tegra_resume_trusted_foundations
+ *
+ * Trusted Foundations firmware initialization.
+ *
+ * Doesn't return if firmware presents.
+ * Corrupted registers: r1, r2
+ */
+ENTRY(tegra_resume_trusted_foundations)
+ /* Check whether Trusted Foundations firmware presents. */
+ mov32 r2, TEGRA_IRAM_BASE + TEGRA_IRAM_RESET_HANDLER_OFFSET
+ ldr r1, =__tegra_cpu_reset_handler_data_offset + \
+ RESET_DATA(TF_PRESENT)
+ ldr r1, [r2, r1]
+ cmp r1, #0
+ reteq lr
+
+ .arch_extension sec
+ /* First call after suspend wakes fir
Re: [PATCH v5 8/8] ARM: tegra: Add firmware calls required for suspend-resume
22.02.2019 20:59, Dmitry Osipenko пишет:
> In order to resume CPU from suspend via trusted Foundations firmware,
> the LP1/LP2 boot vectors and CPU caches need to be set up using the
> firmware calls.
>
> Signed-off-by: Dmitry Osipenko
> ---
> arch/arm/mach-tegra/pm.c| 53 ++---
> arch/arm/mach-tegra/reset-handler.S | 26 ++
> arch/arm/mach-tegra/sleep.S | 3 +-
> 3 files changed, 61 insertions(+), 21 deletions(-)
>
> diff --git a/arch/arm/mach-tegra/pm.c b/arch/arm/mach-tegra/pm.c
> index 66c8cd63dd86..f209f59e0daf 100644
> --- a/arch/arm/mach-tegra/pm.c
> +++ b/arch/arm/mach-tegra/pm.c
> @@ -33,6 +33,7 @@
> #include
>
> #include
> +#include
> #include
> #include
> #include
> @@ -160,6 +161,28 @@ int tegra_cpu_do_idle(void)
>
> static int tegra_sleep_cpu(unsigned long v2p)
> {
> + /*
> + * L2 cache disabling using kernel API only allowed when all
> + * secondary CPU's are offline. Cache have to be disabled with
> + * MMU-on if cache maintenance is done via Trusted Foundations
> + * firmware. Note that CPUIDLE won't ever enter powergate on Tegra30
> + * if any of secondary CPU's is online and this is the LP2-idle
> + * code-path only for Tegra20/30.
> + */
> + if (trusted_foundations_registered())
> + outer_disable();
> +
> + /*
> + * Note that besides of setting up CPU reset vector this firmware
> + * call may also do the following, depending on the FW version:
> + * 1) Disable L2. But this doesn't matter since we already
> + * disabled the L2.
> + * 2) Disable D-cache. This need to be taken into account in
> + * particular by the tegra_disable_clean_inv_dcache() which
> + * shall avoid the re-disable.
> + */
> + call_firmware_op(prepare_idle, TF_PM_MODE_LP2);
> +
> setup_mm_for_reboot();
> tegra_sleep_cpu_finish(v2p);
>
> @@ -196,24 +219,13 @@ void tegra_idle_lp2_last(void)
> cpu_cluster_pm_enter();
> suspend_cpu_complex();
>
> - /*
> - * L2 cache disabling using kernel API only allowed when all
> - * secondary CPU's are offline. Cache have to be disabled early
> - * if cache maintenance is done via Trusted Foundations firmware.
> - * Note that CPUIDLE won't ever enter powergate on Tegra30 if any
> - * of secondary CPU's is online and this is the LP2 codepath only
> - * for Tegra20/30.
> - */
> - if (trusted_foundations_registered())
> - outer_disable();
> -
> cpu_suspend(PHYS_OFFSET - PAGE_OFFSET, &tegra_sleep_cpu);
>
> /*
>* Resume L2 cache if it wasn't re-enabled early during resume,
>* which is the case for Tegra30 that has to re-enable the cache
>* via firmware call. In other cases cache is already enabled and
> - * hence re-enabling is a no-op.
> + * hence re-enabling is a no-op. This is always a no-op on Tegra114+.
>*/
> outer_resume();
>
> @@ -235,6 +247,15 @@ enum tegra_suspend_mode tegra_pm_validate_suspend_mode(
>
> static int tegra_sleep_core(unsigned long v2p)
> {
> + /*
> + * Cache have to be disabled with MMU-on if cache maintenance is done
> + * via Trusted Foundations firmware. This is a no-op on Tegra114+.
> + */
> + if (trusted_foundations_registered())
> + outer_disable();
> +
> + call_firmware_op(prepare_idle, TF_PM_MODE_LP1);
> +
> setup_mm_for_reboot();
> tegra_sleep_core_finish(v2p);
>
> @@ -360,14 +381,6 @@ static int tegra_suspend_enter(suspend_state_t state)
> break;
> }
>
> - /*
> - * Cache have to be disabled early if cache maintenance is done
> - * via Trusted Foundations firmware. Otherwise this is a no-op,
> - * like on Tegra114+.
> - */
> - if (trusted_foundations_registered())
> - outer_disable();
> -
> cpu_suspend(PHYS_OFFSET - PAGE_OFFSET, tegra_sleep_func);
Seems I messed up the rebasing a tad. Will send another version.
[PATCH v5 8/8] ARM: tegra: Add firmware calls required for suspend-resume
In order to resume CPU from suspend via trusted Foundations firmware,
the LP1/LP2 boot vectors and CPU caches need to be set up using the
firmware calls.
Signed-off-by: Dmitry Osipenko
---
arch/arm/mach-tegra/pm.c| 53 ++---
arch/arm/mach-tegra/reset-handler.S | 26 ++
arch/arm/mach-tegra/sleep.S | 3 +-
3 files changed, 61 insertions(+), 21 deletions(-)
diff --git a/arch/arm/mach-tegra/pm.c b/arch/arm/mach-tegra/pm.c
index 66c8cd63dd86..f209f59e0daf 100644
--- a/arch/arm/mach-tegra/pm.c
+++ b/arch/arm/mach-tegra/pm.c
@@ -33,6 +33,7 @@
#include
#include
+#include
#include
#include
#include
@@ -160,6 +161,28 @@ int tegra_cpu_do_idle(void)
static int tegra_sleep_cpu(unsigned long v2p)
{
+ /*
+* L2 cache disabling using kernel API only allowed when all
+* secondary CPU's are offline. Cache have to be disabled with
+* MMU-on if cache maintenance is done via Trusted Foundations
+* firmware. Note that CPUIDLE won't ever enter powergate on Tegra30
+* if any of secondary CPU's is online and this is the LP2-idle
+* code-path only for Tegra20/30.
+*/
+ if (trusted_foundations_registered())
+ outer_disable();
+
+ /*
+* Note that besides of setting up CPU reset vector this firmware
+* call may also do the following, depending on the FW version:
+* 1) Disable L2. But this doesn't matter since we already
+* disabled the L2.
+* 2) Disable D-cache. This need to be taken into account in
+* particular by the tegra_disable_clean_inv_dcache() which
+* shall avoid the re-disable.
+*/
+ call_firmware_op(prepare_idle, TF_PM_MODE_LP2);
+
setup_mm_for_reboot();
tegra_sleep_cpu_finish(v2p);
@@ -196,24 +219,13 @@ void tegra_idle_lp2_last(void)
cpu_cluster_pm_enter();
suspend_cpu_complex();
- /*
-* L2 cache disabling using kernel API only allowed when all
-* secondary CPU's are offline. Cache have to be disabled early
-* if cache maintenance is done via Trusted Foundations firmware.
-* Note that CPUIDLE won't ever enter powergate on Tegra30 if any
-* of secondary CPU's is online and this is the LP2 codepath only
-* for Tegra20/30.
-*/
- if (trusted_foundations_registered())
- outer_disable();
-
cpu_suspend(PHYS_OFFSET - PAGE_OFFSET, &tegra_sleep_cpu);
/*
* Resume L2 cache if it wasn't re-enabled early during resume,
* which is the case for Tegra30 that has to re-enable the cache
* via firmware call. In other cases cache is already enabled and
-* hence re-enabling is a no-op.
+* hence re-enabling is a no-op. This is always a no-op on Tegra114+.
*/
outer_resume();
@@ -235,6 +247,15 @@ enum tegra_suspend_mode tegra_pm_validate_suspend_mode(
static int tegra_sleep_core(unsigned long v2p)
{
+ /*
+* Cache have to be disabled with MMU-on if cache maintenance is done
+* via Trusted Foundations firmware. This is a no-op on Tegra114+.
+*/
+ if (trusted_foundations_registered())
+ outer_disable();
+
+ call_firmware_op(prepare_idle, TF_PM_MODE_LP1);
+
setup_mm_for_reboot();
tegra_sleep_core_finish(v2p);
@@ -360,14 +381,6 @@ static int tegra_suspend_enter(suspend_state_t state)
break;
}
- /*
-* Cache have to be disabled early if cache maintenance is done
-* via Trusted Foundations firmware. Otherwise this is a no-op,
-* like on Tegra114+.
-*/
- if (trusted_foundations_registered())
- outer_disable();
-
cpu_suspend(PHYS_OFFSET - PAGE_OFFSET, tegra_sleep_func);
/*
diff --git a/arch/arm/mach-tegra/reset-handler.S
b/arch/arm/mach-tegra/reset-handler.S
index 3bf202819534..19a609046547 100644
--- a/arch/arm/mach-tegra/reset-handler.S
+++ b/arch/arm/mach-tegra/reset-handler.S
@@ -20,6 +20,7 @@
#include
#include
+#include
#include
#include
@@ -76,6 +77,7 @@ ENTRY(tegra_resume)
orr r1, r1, #1
str r1, [r0]
#endif
+ bl tegra_resume_trusted_foundations
#ifdef CONFIG_CACHE_L2X0
/* L2 cache resume & re-enable */
@@ -88,6 +90,30 @@ end_ca9_scu_l2_resume:
b cpu_resume
ENDPROC(tegra_resume)
+
+/*
+ * tegra_resume_trusted_foundations
+ *
+ * Trusted Foundations firmware initialization.
+ *
+ * Doesn't return if firmware presents.
+ * Corrupted registers: r1, r2
+ */
+ENTRY(tegra_resume_trusted_foundations)
+ /* Check whether Trusted Foundations firmware presents. */
+ mov32 r2, TEGRA_IRAM_BASE + TEGRA_IRAM_RESET_HANDLER_OFFSET
+ ldr r1, =__tegra_cpu_reset_handler_data_offset + \
+
Re: recalibrating x86 TSC during suspend/resume
On Fri, Feb 22, Paolo Bonzini wrote: > On 22/02/19 12:44, Thomas Gleixner wrote: > >> The specific usecase I have is a workload within VMs that makes heavy > >> use of TSC. The kernel is booted with 'clocksource=tsc highres=off > >> nohz=off' > >> because only this clocksource gives enough granularity. The default > >> paravirtualized clock will return the same values via > >> clock_gettime(CLOCK_MONOTONIC) if the timespan between two calls is too > >> short. This does not happen with 'clocksource=tsc'. > > This shouldn't happen. clock_gettime(CLOCK_MONOTONIC) should be > monotonic increasing. Do you have a testcase? Two years ago I tweaked sysbench to track the execution time of the 'memory' test: https://github.com/olafhering/sysbench https://github.com/olafhering/sysbench/blame/pv/src/tests/memory/sb_memory.c The checks in diff_timespec() triggered with clocksource=xen, but I can not reproduce it right now with 5.0 and 4.4 based kernels. I have no data how KVM behaves. In the end the hypervisor was tweaked to tolerate a certain jitter in expected TSC speed before emulation kicks in. Up to ~1MHz would be ok to stay within the 500PPM limit that ntpd can handle. But now there is that "island" issue that needs to be resolved in one way or another. Olaf signature.asc Description: PGP signature
Re: [PATCH 7/7] iommu/ipmmu-vmsa: Add suspend/resume support
Hi Geert,
On 20/02/2019 15:05, Geert Uytterhoeven wrote:
During PSCI system suspend, R-Car Gen3 SoCs are powered down, and all
IPMMU state is lost. Hence after s2ram, devices wired behind an IPMMU,
and configured to use it, will see their DMA operations hang.
To fix this, restore all IPMMU contexts, and re-enable all active
micro-TLBs during system resume.
To avoid overhead on platforms not needing it, the resume code has a
build time dependency on sleep and PSCI support, and a runtime
dependency on PSCI.
Frankly, that aspect looks horribly hacky. It's not overly reasonable
for random device drivers to be poking at PSCI internals, and while it
might happen to correlate on some known set of current SoCs with current
firmware, in general it's really too fragile to be accurate:
- Firmware is free to implement suspend callbacks in a way which doesn't
actually lose power.
- Support for CPU_SUSPEND does not imply that SYSTEM_SUSPEND is even
implemented, let alone how it might behave.
- The dev_pm_ops callbacks can also be used for hibernate, wherein it
doesn't really matter what the firmware may or may not do if the user
has pulled the plug and resumed us a week later.
Furthermore, I think any attempt to detect whether you need to resume or
not is inherently fraught with danger - from testing the arm-smmu
runtime PM ops, I've seen register state take a surprisingly long time
to decay in a switched-off power domain, to the point where unless you
check every bit of every register you can't necessarily be certain that
they're really all still valid, and by that point you're doing far more
work than just unconditionally reinitialising the whole state anyway.
Upon resuming from hibernate, the state left by the cold-boot stage
almost certainly *will* be valid, but it probably won't be the state you
actually want.
Really, the whole idea smells of the premature optimisation demons
anyway - is the resume overhead measurably significant?
Signed-off-by: Geert Uytterhoeven
---
This patch takes a different approach than the BSP, which implements a
bulk save/restore of all registers during system suspend/resume.
drivers/iommu/ipmmu-vmsa.c | 52 +-
1 file changed, 51 insertions(+), 1 deletion(-)
diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
index 92a766dd8b459f0c..5d22139914e8f033 100644
--- a/drivers/iommu/ipmmu-vmsa.c
+++ b/drivers/iommu/ipmmu-vmsa.c
@@ -22,6 +22,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -36,7 +37,10 @@
#define arm_iommu_detach_device(...) do {} while (0)
#endif
-#define IPMMU_CTX_MAX 8U
+#define IPMMU_CTX_MAX 8U
+#define IPMMU_CTX_INVALID -1
+
+#define IPMMU_UTLB_MAX 48U
struct ipmmu_features {
bool use_ns_alias_offset;
@@ -58,6 +62,7 @@ struct ipmmu_vmsa_device {
spinlock_t lock;/* Protects ctx and domains[] */
DECLARE_BITMAP(ctx, IPMMU_CTX_MAX);
struct ipmmu_vmsa_domain *domains[IPMMU_CTX_MAX];
+ s8 utlb_ctx[IPMMU_UTLB_MAX];
struct iommu_group *group;
struct dma_iommu_mapping *mapping;
@@ -335,6 +340,7 @@ static void ipmmu_utlb_enable(struct ipmmu_vmsa_domain
*domain,
ipmmu_write(mmu, IMUCTR(utlb),
IMUCTR_TTSEL_MMU(domain->context_id) | IMUCTR_FLUSH |
IMUCTR_MMUEN);
+ mmu->utlb_ctx[utlb] = domain->context_id;
}
/*
@@ -346,6 +352,7 @@ static void ipmmu_utlb_disable(struct ipmmu_vmsa_domain
*domain,
struct ipmmu_vmsa_device *mmu = domain->mmu;
ipmmu_write(mmu, IMUCTR(utlb), 0);
+ mmu->utlb_ctx[utlb] = IPMMU_CTX_INVALID;
}
static void ipmmu_tlb_flush_all(void *cookie)
@@ -1043,6 +1050,7 @@ static int ipmmu_probe(struct platform_device *pdev)
spin_lock_init(&mmu->lock);
bitmap_zero(mmu->ctx, IPMMU_CTX_MAX);
mmu->features = of_device_get_match_data(&pdev->dev);
+ memset(mmu->utlb_ctx, IPMMU_CTX_INVALID, mmu->features->num_utlbs);
dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(40));
/* Map I/O memory and request IRQ. */
@@ -1158,10 +1166,52 @@ static int ipmmu_remove(struct platform_device *pdev)
return 0;
}
+#if defined(CONFIG_PM_SLEEP) && defined(CONFIG_ARM_PSCI_FW)
+static int ipmmu_resume_noirq(struct device *dev)
+{
+ struct ipmmu_vmsa_device *mmu = dev_get_drvdata(dev);
+ unsigned int i;
+
+ /* This is the best we can do to check for the presence of PSCI */
+ if (!psci_ops.cpu_suspend)
+ return 0;
+
+ /* Reset root MMU and restore contexts */
+ if (ipmmu_is_root(mmu)) {
+ ipmmu_device_reset(mmu);
+
+ for (i = 0; i < mmu->num_ctx; i++) {
+ if (!mmu->domains[i])
+
Re: recalibrating x86 TSC during suspend/resume
On 22/02/19 12:44, Thomas Gleixner wrote: >> The specific usecase I have is a workload within VMs that makes heavy >> use of TSC. The kernel is booted with 'clocksource=tsc highres=off nohz=off' >> because only this clocksource gives enough granularity. The default >> paravirtualized clock will return the same values via >> clock_gettime(CLOCK_MONOTONIC) if the timespan between two calls is too >> short. This does not happen with 'clocksource=tsc'. This shouldn't happen. clock_gettime(CLOCK_MONOTONIC) should be monotonic increasing. Do you have a testcase? The KVM clocksource is high-resolution and also TSC-based, the difference is that it performs two multiplications instead of one. The first uses TSC parameters from the host. The second, which is the one in arch/x86/entry/vdso/vclock_gettime.c's do_hres function, will have a 1:1 multiplier (excluding adjtime shearing) because kvmclock already returns nanoseconds. > Newer Intels support TSC scaling for VMX, which could solve the problem. It > affects TSC readout by: > > TSC = (read(HWTSC) * multiplier) >> 48 > > So you can standarize on a TSC frequency accross a fleet. Not sure when > that was introduced and no idea whether it's available on AMD. It's Skylake (server parts only) or newer. AMD instead has had it (almost) forever. QEMU 2.6 or newer will use it automatically across live migration, if available. > For a software solution we could try the following: > > 1) Provide the raw TSC frequency of the host to the guest in some magic > software defined MSR or CPUID. If there is an existing mechanism, use > that. This shouldn't be needed for two reasons: 1) you could also use kvmclock's provided mult/shift 2) I am not convinced that kvmclock has the behavior that Olaf mentions, and if it does it would be a bug. Paolo
Re: recalibrating x86 TSC during suspend/resume
Am Fri, 22 Feb 2019 12:44:39 +0100 (CET) schrieb Thomas Gleixner : > Whether that is accurate enough or not to make NTP happy, I can't tell, but > it's definitely worth a try. Thanks Thomas, I will look into the suggestions. Olaf pgpKvKEGb9vSF.pgp Description: Digitale Signatur von OpenPGP
Re: recalibrating x86 TSC during suspend/resume
On Fri, 22 Feb 2019, Olaf Hering wrote: > Is there a way to recalibrate the x86 TSC during a suspend/resume cycle? No. > While the frequency will remain the same on a Laptop, it may (or rather: > it definitly will) differ if a VM is migrated from one host to another. > The hypervisor may choose to emulate the expected TSC frequency on the > destination host, but this emulation comes with a significant > performance cost. Therefore it would be good if the kernel evaluates the > environment during resume. > > The specific usecase I have is a workload within VMs that makes heavy > use of TSC. The kernel is booted with 'clocksource=tsc highres=off nohz=off' > because only this clocksource gives enough granularity. The default > paravirtualized clock will return the same values via > clock_gettime(CLOCK_MONOTONIC) if the timespan between two calls is too > short. This does not happen with 'clocksource=tsc'. > > Right now it is not possible to migrate VMs to hosts with different CPU > speeds. This leads to "islands" of identical hardware, and makes > maintenance of hosts harder than it needs to be. If the VM kernel would > be able to cope with CPU/TSC frequency changes, the pool of potential > destination hosts will become significant larger. The problem with recalibrating TSC on resume is that it would have to be 1) quick 2) accurate, so NTP does not get utterly unhappy. Newer Intels support TSC scaling for VMX, which could solve the problem. It affects TSC readout by: TSC = (read(HWTSC) * multiplier) >> 48 So you can standarize on a TSC frequency accross a fleet. Not sure when that was introduced and no idea whether it's available on AMD. For a software solution we could try the following: 1) Provide the raw TSC frequency of the host to the guest in some magic software defined MSR or CPUID. If there is an existing mechanism, use that. 2) On resume check whether the MSR/CPUID is available and if so readout that information and check whether the frequency is the same as before. If not it is trivial enough to adjust the guest mult/shift values for both raw and NTP adjusted clocks before they are used again, i.e. before timekeeping_resume(). Need to look what's the best place, but probably the clocksource resume callback. Plus if TSC deadline timer is used, we'd need the same adjustment there. That's backward compatible, because if the MSR/CPUID is not there, then the recalibration is not tried. Whether that is accurate enough or not to make NTP happy, I can't tell, but it's definitely worth a try. Thanks, tglx
recalibrating x86 TSC during suspend/resume
Is there a way to recalibrate the x86 TSC during a suspend/resume cycle? While the frequency will remain the same on a Laptop, it may (or rather: it definitly will) differ if a VM is migrated from one host to another. The hypervisor may choose to emulate the expected TSC frequency on the destination host, but this emulation comes with a significant performance cost. Therefore it would be good if the kernel evaluates the environment during resume. The specific usecase I have is a workload within VMs that makes heavy use of TSC. The kernel is booted with 'clocksource=tsc highres=off nohz=off' because only this clocksource gives enough granularity. The default paravirtualized clock will return the same values via clock_gettime(CLOCK_MONOTONIC) if the timespan between two calls is too short. This does not happen with 'clocksource=tsc'. Right now it is not possible to migrate VMs to hosts with different CPU speeds. This leads to "islands" of identical hardware, and makes maintenance of hosts harder than it needs to be. If the VM kernel would be able to cope with CPU/TSC frequency changes, the pool of potential destination hosts will become significant larger. The current result of a migration with non-emulated TSC between hosts of different speed is: [ 42.452258] clocksource: timekeeping watchdog on CPU1: Marking clocksource 'tsc' as unstable because the skew is too large: [ 42.452270] clocksource: 'xen' wd_now: 6d34a86adb wd_last: 6d1dc51793 mask: [ 42.452272] clocksource: 'tsc' cs_now: 1fd2ce46bb cs_last: 1f95c4ca75 mask: [ 42.452273] tsc: Marking TSC unstable due to clocksource watchdog Thanks, Olaf signature.asc Description: PGP signature
Re: [PATCH 7/7] iommu/ipmmu-vmsa: Add suspend/resume support
Hi Laurent,
On Wed, Feb 20, 2019 at 5:11 PM Laurent Pinchart
wrote:
> On Wed, Feb 20, 2019 at 05:05:49PM +0100, Geert Uytterhoeven wrote:
> > On Wed, Feb 20, 2019 at 4:42 PM Laurent Pinchart wrote:
> > > On Wed, Feb 20, 2019 at 04:05:31PM +0100, Geert Uytterhoeven wrote:
> > >> During PSCI system suspend, R-Car Gen3 SoCs are powered down, and all
> > >> IPMMU state is lost. Hence after s2ram, devices wired behind an IPMMU,
> > >> and configured to use it, will see their DMA operations hang.
> > >>
> > >> To fix this, restore all IPMMU contexts, and re-enable all active
> > >> micro-TLBs during system resume.
> > >>
> > >> To avoid overhead on platforms not needing it, the resume code has a
> > >> build time dependency on sleep and PSCI support, and a runtime
> > >> dependency on PSCI.
> > >> --- a/drivers/iommu/ipmmu-vmsa.c
> > >> +++ b/drivers/iommu/ipmmu-vmsa.c
> > >> @@ -1158,10 +1166,52 @@ static int ipmmu_remove(struct platform_device
> > >> *pdev)
> > >> return 0;
> > >> }
> > >>
> > >> +#if defined(CONFIG_PM_SLEEP) && defined(CONFIG_ARM_PSCI_FW)
> > >> +static int ipmmu_resume_noirq(struct device *dev)
> > >> +{
> > >> + struct ipmmu_vmsa_device *mmu = dev_get_drvdata(dev);
> > >> + unsigned int i;
> > >> +
> > >> + /* This is the best we can do to check for the presence of PSCI */
> > >> + if (!psci_ops.cpu_suspend)
> > >> + return 0;
> > >
> > > PSCI suspend disabling power to the SoC completely may be a common
> > > behaviour on our development boards, but isn't mandated by the PSCI
> > > specification if I'm not mistaken. Is there a way to instead detect that
> > > power has been lost, perhaps by checking whether a register has been
> > > reset to its default value ?
> >
> > The approach here is the same as in the clk and pinctrl drivers.
> >
> > I think we could check if the IMCTR registers for allocated domains in root
> > IPMMUs are non-zero. But that's about as expensive as doing the full
> > restore, I think.
>
> Would reading just one register be more expensive that full
> reconfiguration ? Or is there no single register that could serve this
> purpose ?
>
> > And it may have to be done for each and every IPMMU instance, or do
> > you trust caching for this?
>
> If we can find a single register I think that reading it for every IPMMU
> instance wouldn't be an issue.
Upon more thought, probably it can be done by reading the IMCTR
for the first non-zero domain. Will look into it...
> > >> +static const struct dev_pm_ops ipmmu_pm = {
> > >> + SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(NULL, ipmmu_resume_noirq)
> > >> +};
> > >> +#define DEV_PM_OPS &ipmmu_pm
> > >> +#else
> > >> +#define DEV_PM_OPS NULL
> > >> +#endif /* CONFIG_PM_SLEEP && CONFIG_ARM_PSCI_FW */
> > >> +
> > >> static struct platform_driver ipmmu_driver = {
> > >> .driver = {
> > >> .name = "ipmmu-vmsa",
> > >> .of_match_table = of_match_ptr(ipmmu_of_ids),
> > >> + .pm = DEV_PM_OPS,
> > >
> > > I would have used conditional compilation here instead of using a
> > > DEV_PM_OPS macro, as I think the macro decreases readability (and also
> > > given how its generic name could later conflict with something else).
> >
> > You mean
> >
> > #ifdef ...
> > .pm = &ipmmu_pm,
> > #endif
> >
> > and marking ipmmu_pm __maybe_unused__?
>
> Yes. Up to you.
I'm not such a big fan of __maybe_unused. It's easy to add, and too
easy to forget to remove it when it's no longer needed (casts have the
same problem).
Usually people just annotate the actual suspend/resume methods with
__maybe_unused, which still leaves the (large) struct dev_pm_ops in
memory.
So I started preferring the DEV_PM_OPS approach...
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
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
Re: [PATCH 7/7] iommu/ipmmu-vmsa: Add suspend/resume support
Hi Geert,
On Wed, Feb 20, 2019 at 05:05:49PM +0100, Geert Uytterhoeven wrote:
> On Wed, Feb 20, 2019 at 4:42 PM Laurent Pinchart wrote:
> > On Wed, Feb 20, 2019 at 04:05:31PM +0100, Geert Uytterhoeven wrote:
> >> During PSCI system suspend, R-Car Gen3 SoCs are powered down, and all
> >> IPMMU state is lost. Hence after s2ram, devices wired behind an IPMMU,
> >> and configured to use it, will see their DMA operations hang.
> >>
> >> To fix this, restore all IPMMU contexts, and re-enable all active
> >> micro-TLBs during system resume.
> >>
> >> To avoid overhead on platforms not needing it, the resume code has a
> >> build time dependency on sleep and PSCI support, and a runtime
> >> dependency on PSCI.
> >>
> >> Signed-off-by: Geert Uytterhoeven
> >> ---
> >> This patch takes a different approach than the BSP, which implements a
> >> bulk save/restore of all registers during system suspend/resume.
> >
> > I like this approach better too.
>
> Thanks ;-)
>
> >> --- a/drivers/iommu/ipmmu-vmsa.c
> >> +++ b/drivers/iommu/ipmmu-vmsa.c
>
> >> @@ -58,6 +62,7 @@ struct ipmmu_vmsa_device {
> >> spinlock_t lock;/* Protects ctx and
> >> domains[] */
> >> DECLARE_BITMAP(ctx, IPMMU_CTX_MAX);
> >> struct ipmmu_vmsa_domain *domains[IPMMU_CTX_MAX];
> >> + s8 utlb_ctx[IPMMU_UTLB_MAX];
> >
> > How about making this a bitmask instead to save memory ? I would also
> > rename it as utlb_ctx doesn't really carry the meaning of the field,
> > whose purpose is to store whether the µTLB is enabled or disabled.
>
> This field isn't just a binary flag, but stores the context used for the
> uTLB, so we can map from micro-TLB to context.
> Given there can be 8 contexts, plus the need to indicate unused contexts,
> that means 4 bits/micro-TLB. So the overhead is just 24 bytes per IPMMU
> instance.
My bad, I've overlooked that.
> I considered allocating the array dynamically (by having s8 utlb_ctx[]
> at the end of the structure), but didn't go that route, as the domains[]
> array already uses more memory.
>
> >> @@ -1158,10 +1166,52 @@ static int ipmmu_remove(struct platform_device
> >> *pdev)
> >> return 0;
> >> }
> >>
> >> +#if defined(CONFIG_PM_SLEEP) && defined(CONFIG_ARM_PSCI_FW)
> >> +static int ipmmu_resume_noirq(struct device *dev)
> >> +{
> >> + struct ipmmu_vmsa_device *mmu = dev_get_drvdata(dev);
> >> + unsigned int i;
> >> +
> >> + /* This is the best we can do to check for the presence of PSCI */
> >> + if (!psci_ops.cpu_suspend)
> >> + return 0;
> >
> > PSCI suspend disabling power to the SoC completely may be a common
> > behaviour on our development boards, but isn't mandated by the PSCI
> > specification if I'm not mistaken. Is there a way to instead detect that
> > power has been lost, perhaps by checking whether a register has been
> > reset to its default value ?
>
> The approach here is the same as in the clk and pinctrl drivers.
>
> I think we could check if the IMCTR registers for allocated domains in root
> IPMMUs are non-zero. But that's about as expensive as doing the full
> restore, I think.
Would reading just one register be more expensive that full
reconfiguration ? Or is there no single register that could serve this
purpose ?
> And it may have to be done for each and every IPMMU instance, or do
> you trust caching for this?
If we can find a single register I think that reading it for every IPMMU
instance wouldn't be an issue.
> >> +
> >> + /* Reset root MMU and restore contexts */
> >
> > I think the rest of the code adds a period at the end of sentences in
> > comments.
>
> The balance seems to be just under 50% ;-)
>
> >> + if (ipmmu_is_root(mmu)) {
> >> + ipmmu_device_reset(mmu);
> >> +
> >> + for (i = 0; i < mmu->num_ctx; i++) {
> >> + if (!mmu->domains[i])
> >> + continue;
> >> +
> >> + ipmmu_context_init(mmu->domains[i]);
> >> + }
> >> + }
> >> +
> >> + /* Re-enable active micro-TLBs */
> >> + for (i = 0; i < mmu->features->num_utlbs; i++) {
> >> + if (mmu->utlb_ctx[i] == IPMMU_CTX_INVALID)
> >> + continue;
> >> +
&
Re: [PATCH 7/7] iommu/ipmmu-vmsa: Add suspend/resume support
Hi Laurent,
On Wed, Feb 20, 2019 at 4:42 PM Laurent Pinchart
wrote:
> On Wed, Feb 20, 2019 at 04:05:31PM +0100, Geert Uytterhoeven wrote:
> > During PSCI system suspend, R-Car Gen3 SoCs are powered down, and all
> > IPMMU state is lost. Hence after s2ram, devices wired behind an IPMMU,
> > and configured to use it, will see their DMA operations hang.
> >
> > To fix this, restore all IPMMU contexts, and re-enable all active
> > micro-TLBs during system resume.
> >
> > To avoid overhead on platforms not needing it, the resume code has a
> > build time dependency on sleep and PSCI support, and a runtime
> > dependency on PSCI.
> >
> > Signed-off-by: Geert Uytterhoeven
> > ---
> > This patch takes a different approach than the BSP, which implements a
> > bulk save/restore of all registers during system suspend/resume.
>
> I like this approach better too.
Thanks ;-)
> > --- a/drivers/iommu/ipmmu-vmsa.c
> > +++ b/drivers/iommu/ipmmu-vmsa.c
> > @@ -58,6 +62,7 @@ struct ipmmu_vmsa_device {
> > spinlock_t lock;/* Protects ctx and domains[]
> > */
> > DECLARE_BITMAP(ctx, IPMMU_CTX_MAX);
> > struct ipmmu_vmsa_domain *domains[IPMMU_CTX_MAX];
> > + s8 utlb_ctx[IPMMU_UTLB_MAX];
>
> How about making this a bitmask instead to save memory ? I would also
> rename it as utlb_ctx doesn't really carry the meaning of the field,
> whose purpose is to store whether the µTLB is enabled or disabled.
This field isn't just a binary flag, but stores the context used for the
uTLB, so we can map from micro-TLB to context.
Given there can be 8 contexts, plus the need to indicate unused contexts,
that means 4 bits/micro-TLB. So the overhead is just 24 bytes per IPMMU
instance.
I considered allocating the array dynamically (by having s8 utlb_ctx[]
at the end of the structure), but didn't go that route, as the domains[]
array already uses more memory.
> > @@ -1158,10 +1166,52 @@ static int ipmmu_remove(struct platform_device
> > *pdev)
> > return 0;
> > }
> >
> > +#if defined(CONFIG_PM_SLEEP) && defined(CONFIG_ARM_PSCI_FW)
> > +static int ipmmu_resume_noirq(struct device *dev)
> > +{
> > + struct ipmmu_vmsa_device *mmu = dev_get_drvdata(dev);
> > + unsigned int i;
> > +
> > + /* This is the best we can do to check for the presence of PSCI */
> > + if (!psci_ops.cpu_suspend)
> > + return 0;
>
> PSCI suspend disabling power to the SoC completely may be a common
> behaviour on our development boards, but isn't mandated by the PSCI
> specification if I'm not mistaken. Is there a way to instead detect that
> power has been lost, perhaps by checking whether a register has been
> reset to its default value ?
The approach here is the same as in the clk and pinctrl drivers.
I think we could check if the IMCTR registers for allocated domains in root
IPMMUs are non-zero. But that's about as expensive as doing the full
restore, I think. And it may have to be done for each and every IPMMU
instance, or do you trust caching for this?
> > +
> > + /* Reset root MMU and restore contexts */
>
> I think the rest of the code adds a period at the end of sentences in
> comments.
The balance seems to be just under 50% ;-)
> > + if (ipmmu_is_root(mmu)) {
> > + ipmmu_device_reset(mmu);
> > +
> > + for (i = 0; i < mmu->num_ctx; i++) {
> > + if (!mmu->domains[i])
> > + continue;
> > +
> > + ipmmu_context_init(mmu->domains[i]);
> > + }
> > + }
> > +
> > + /* Re-enable active micro-TLBs */
> > + for (i = 0; i < mmu->features->num_utlbs; i++) {
> > + if (mmu->utlb_ctx[i] == IPMMU_CTX_INVALID)
> > + continue;
> > +
> > + ipmmu_utlb_enable(mmu->root->domains[mmu->utlb_ctx[i]], i);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static const struct dev_pm_ops ipmmu_pm = {
> > + SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(NULL, ipmmu_resume_noirq)
> > +};
> > +#define DEV_PM_OPS &ipmmu_pm
> > +#else
> > +#define DEV_PM_OPS NULL
> > +#endif /* CONFIG_PM_SLEEP && CONFIG_ARM_PSCI_FW */
> > +
> > static struct platform_driver ipmmu_driver = {
> > .driver = {
> > .name = "ipmmu-vmsa",
> > .of_match_table = of_match_ptr(ipmmu_of_ids),
> > + .pm = DEV_PM_OPS,
>
> I would ha
Re: [PATCH 7/7] iommu/ipmmu-vmsa: Add suspend/resume support
Hi Geert,
Thank you for the patch.
On Wed, Feb 20, 2019 at 04:05:31PM +0100, Geert Uytterhoeven wrote:
> During PSCI system suspend, R-Car Gen3 SoCs are powered down, and all
> IPMMU state is lost. Hence after s2ram, devices wired behind an IPMMU,
> and configured to use it, will see their DMA operations hang.
>
> To fix this, restore all IPMMU contexts, and re-enable all active
> micro-TLBs during system resume.
>
> To avoid overhead on platforms not needing it, the resume code has a
> build time dependency on sleep and PSCI support, and a runtime
> dependency on PSCI.
>
> Signed-off-by: Geert Uytterhoeven
> ---
> This patch takes a different approach than the BSP, which implements a
> bulk save/restore of all registers during system suspend/resume.
I like this approach better too.
> drivers/iommu/ipmmu-vmsa.c | 52 +-
> 1 file changed, 51 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
> index 92a766dd8b459f0c..5d22139914e8f033 100644
> --- a/drivers/iommu/ipmmu-vmsa.c
> +++ b/drivers/iommu/ipmmu-vmsa.c
> @@ -22,6 +22,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -36,7 +37,10 @@
> #define arm_iommu_detach_device(...) do {} while (0)
> #endif
>
> -#define IPMMU_CTX_MAX 8U
> +#define IPMMU_CTX_MAX8U
> +#define IPMMU_CTX_INVALID-1
> +
> +#define IPMMU_UTLB_MAX 48U
>
> struct ipmmu_features {
> bool use_ns_alias_offset;
> @@ -58,6 +62,7 @@ struct ipmmu_vmsa_device {
> spinlock_t lock;/* Protects ctx and domains[] */
> DECLARE_BITMAP(ctx, IPMMU_CTX_MAX);
> struct ipmmu_vmsa_domain *domains[IPMMU_CTX_MAX];
> + s8 utlb_ctx[IPMMU_UTLB_MAX];
How about making this a bitmask instead to save memory ? I would also
rename it as utlb_ctx doesn't really carry the meaning of the field,
whose purpose is to store whether the µTLB is enabled or disabled.
>
> struct iommu_group *group;
> struct dma_iommu_mapping *mapping;
> @@ -335,6 +340,7 @@ static void ipmmu_utlb_enable(struct ipmmu_vmsa_domain
> *domain,
> ipmmu_write(mmu, IMUCTR(utlb),
> IMUCTR_TTSEL_MMU(domain->context_id) | IMUCTR_FLUSH |
> IMUCTR_MMUEN);
> + mmu->utlb_ctx[utlb] = domain->context_id;
> }
>
> /*
> @@ -346,6 +352,7 @@ static void ipmmu_utlb_disable(struct ipmmu_vmsa_domain
> *domain,
> struct ipmmu_vmsa_device *mmu = domain->mmu;
>
> ipmmu_write(mmu, IMUCTR(utlb), 0);
> + mmu->utlb_ctx[utlb] = IPMMU_CTX_INVALID;
> }
>
> static void ipmmu_tlb_flush_all(void *cookie)
> @@ -1043,6 +1050,7 @@ static int ipmmu_probe(struct platform_device *pdev)
> spin_lock_init(&mmu->lock);
> bitmap_zero(mmu->ctx, IPMMU_CTX_MAX);
> mmu->features = of_device_get_match_data(&pdev->dev);
> + memset(mmu->utlb_ctx, IPMMU_CTX_INVALID, mmu->features->num_utlbs);
> dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(40));
>
> /* Map I/O memory and request IRQ. */
> @@ -1158,10 +1166,52 @@ static int ipmmu_remove(struct platform_device *pdev)
> return 0;
> }
>
> +#if defined(CONFIG_PM_SLEEP) && defined(CONFIG_ARM_PSCI_FW)
> +static int ipmmu_resume_noirq(struct device *dev)
> +{
> + struct ipmmu_vmsa_device *mmu = dev_get_drvdata(dev);
> + unsigned int i;
> +
> + /* This is the best we can do to check for the presence of PSCI */
> + if (!psci_ops.cpu_suspend)
> + return 0;
PSCI suspend disabling power to the SoC completely may be a common
behaviour on our development boards, but isn't mandated by the PSCI
specification if I'm not mistaken. Is there a way to instead detect that
power has been lost, perhaps by checking whether a register has been
reset to its default value ?
> +
> + /* Reset root MMU and restore contexts */
I think the rest of the code adds a period at the end of sentences in
comments.
> + if (ipmmu_is_root(mmu)) {
> + ipmmu_device_reset(mmu);
> +
> + for (i = 0; i < mmu->num_ctx; i++) {
> + if (!mmu->domains[i])
> + continue;
> +
> + ipmmu_context_init(mmu->domains[i]);
> + }
> + }
> +
> + /* Re-enable active micro-TLBs */
> + for (i = 0; i < mmu->features->num_utlbs; i++) {
> + if (mmu->utlb_ctx[i] == IPMMU_CTX_INVALID)
> + continue;
> +
> + ipmmu_utlb_enable(mmu->root-&
[PATCH 7/7] iommu/ipmmu-vmsa: Add suspend/resume support
During PSCI system suspend, R-Car Gen3 SoCs are powered down, and all
IPMMU state is lost. Hence after s2ram, devices wired behind an IPMMU,
and configured to use it, will see their DMA operations hang.
To fix this, restore all IPMMU contexts, and re-enable all active
micro-TLBs during system resume.
To avoid overhead on platforms not needing it, the resume code has a
build time dependency on sleep and PSCI support, and a runtime
dependency on PSCI.
Signed-off-by: Geert Uytterhoeven
---
This patch takes a different approach than the BSP, which implements a
bulk save/restore of all registers during system suspend/resume.
drivers/iommu/ipmmu-vmsa.c | 52 +-
1 file changed, 51 insertions(+), 1 deletion(-)
diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
index 92a766dd8b459f0c..5d22139914e8f033 100644
--- a/drivers/iommu/ipmmu-vmsa.c
+++ b/drivers/iommu/ipmmu-vmsa.c
@@ -22,6 +22,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -36,7 +37,10 @@
#define arm_iommu_detach_device(...) do {} while (0)
#endif
-#define IPMMU_CTX_MAX 8U
+#define IPMMU_CTX_MAX 8U
+#define IPMMU_CTX_INVALID -1
+
+#define IPMMU_UTLB_MAX 48U
struct ipmmu_features {
bool use_ns_alias_offset;
@@ -58,6 +62,7 @@ struct ipmmu_vmsa_device {
spinlock_t lock;/* Protects ctx and domains[] */
DECLARE_BITMAP(ctx, IPMMU_CTX_MAX);
struct ipmmu_vmsa_domain *domains[IPMMU_CTX_MAX];
+ s8 utlb_ctx[IPMMU_UTLB_MAX];
struct iommu_group *group;
struct dma_iommu_mapping *mapping;
@@ -335,6 +340,7 @@ static void ipmmu_utlb_enable(struct ipmmu_vmsa_domain
*domain,
ipmmu_write(mmu, IMUCTR(utlb),
IMUCTR_TTSEL_MMU(domain->context_id) | IMUCTR_FLUSH |
IMUCTR_MMUEN);
+ mmu->utlb_ctx[utlb] = domain->context_id;
}
/*
@@ -346,6 +352,7 @@ static void ipmmu_utlb_disable(struct ipmmu_vmsa_domain
*domain,
struct ipmmu_vmsa_device *mmu = domain->mmu;
ipmmu_write(mmu, IMUCTR(utlb), 0);
+ mmu->utlb_ctx[utlb] = IPMMU_CTX_INVALID;
}
static void ipmmu_tlb_flush_all(void *cookie)
@@ -1043,6 +1050,7 @@ static int ipmmu_probe(struct platform_device *pdev)
spin_lock_init(&mmu->lock);
bitmap_zero(mmu->ctx, IPMMU_CTX_MAX);
mmu->features = of_device_get_match_data(&pdev->dev);
+ memset(mmu->utlb_ctx, IPMMU_CTX_INVALID, mmu->features->num_utlbs);
dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(40));
/* Map I/O memory and request IRQ. */
@@ -1158,10 +1166,52 @@ static int ipmmu_remove(struct platform_device *pdev)
return 0;
}
+#if defined(CONFIG_PM_SLEEP) && defined(CONFIG_ARM_PSCI_FW)
+static int ipmmu_resume_noirq(struct device *dev)
+{
+ struct ipmmu_vmsa_device *mmu = dev_get_drvdata(dev);
+ unsigned int i;
+
+ /* This is the best we can do to check for the presence of PSCI */
+ if (!psci_ops.cpu_suspend)
+ return 0;
+
+ /* Reset root MMU and restore contexts */
+ if (ipmmu_is_root(mmu)) {
+ ipmmu_device_reset(mmu);
+
+ for (i = 0; i < mmu->num_ctx; i++) {
+ if (!mmu->domains[i])
+ continue;
+
+ ipmmu_context_init(mmu->domains[i]);
+ }
+ }
+
+ /* Re-enable active micro-TLBs */
+ for (i = 0; i < mmu->features->num_utlbs; i++) {
+ if (mmu->utlb_ctx[i] == IPMMU_CTX_INVALID)
+ continue;
+
+ ipmmu_utlb_enable(mmu->root->domains[mmu->utlb_ctx[i]], i);
+ }
+
+ return 0;
+}
+
+static const struct dev_pm_ops ipmmu_pm = {
+ SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(NULL, ipmmu_resume_noirq)
+};
+#define DEV_PM_OPS &ipmmu_pm
+#else
+#define DEV_PM_OPS NULL
+#endif /* CONFIG_PM_SLEEP && CONFIG_ARM_PSCI_FW */
+
static struct platform_driver ipmmu_driver = {
.driver = {
.name = "ipmmu-vmsa",
.of_match_table = of_match_ptr(ipmmu_of_ids),
+ .pm = DEV_PM_OPS,
},
.probe = ipmmu_probe,
.remove = ipmmu_remove,
--
2.17.1
[PATCH 0/7] iommu/ipmmu-vmsa: Suspend/resume support and assorted cleanups
Hi Jörg, Magnus,
On R-Car Gen3 systems with PSCI, PSCI may power down the SoC during
system suspend, thus losing all IOMMU state. Hence after s2ram, devices
behind an IPMMU (e.g. SATA), and configured to use it, will fail to
complete their I/O operations.
This patch series adds suspend/resume support to the Renesas IPMMU-VMSA
IOMMU driver, and performs some smaller cleanups and fixes during the
process. Most patches are fairly independent, except for patch 7/7,
which depends on patches 5/7 and 6/7.
This has been tested on Salvator-XS with R-Car H3 ES2.0, with IPMMU
suport for SATA enabled. To play safe, the resume operation has also
been tested on R-Car M2-W, where it is currently not enabled due to the
absence of PSCI in the firmware.
Thanks for your comments!
Geert Uytterhoeven (7):
iommu/ipmmu-vmsa: Link IOMMUs and devices in sysfs
iommu/ipmmu-vmsa: Call ipmmu_ctx_write_root() instead of open coding
iommu/ipmmu-vmsa: Prepare to handle 40-bit error addresses
iommu/ipmmu-vmsa: Make IPMMU_CTX_MAX unsigned
iommu/ipmmu-vmsa: Move num_utlbs to SoC-specific features
iommu/ipmmu-vmsa: Extract hardware context initialization
iommu/ipmmu-vmsa: Add suspend/resume support
drivers/iommu/ipmmu-vmsa.c | 194 +
1 file changed, 131 insertions(+), 63 deletions(-)
--
2.17.1
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
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
[PATCH 0/3] usb: gadget: atmel: support USB suspend/resume
This patch series hooks up proper support for USB suspend and resume to the Atmel UDC. Jonas Bonn (3): usb: gadget: atmel_usba_udc: simplify setting of interrupt-enabled mask usb: gadget: atmel: support USB suspend usb: gadget: atmel: tie wake lock to running clock drivers/usb/gadget/udc/atmel_usba_udc.c | 84 - drivers/usb/gadget/udc/atmel_usba_udc.h | 1 + 2 files changed, 71 insertions(+), 14 deletions(-) -- 2.19.1
[PATCH 4.19 50/85] gpio: mxc: move gpio noirq suspend/resume to syscore phase
4.19-stable review patch. If anyone has any objections, please let me know.
--
commit 1a5287a3dbc34cd0c02c8f64c9131bd23cdfe2bb upstream.
During noirq suspend/resume phase, GPIO irq could arrive
and its registers like IMR will be changed by irq handle
process, to make the GPIO registers exactly when it is
powered ON after resume, move the GPIO noirq suspend/resume
callback to syscore suspend/resume phase, local irq is
disabled at this phase so GPIO registers are atomic.
Fixes: c19fdaeea0aa ("gpio: mxc: add power management support")
Signed-off-by: Anson Huang
Signed-off-by: Linus Walleij
Signed-off-by: Martin Hundebøll
Cc: # 4.19.x+
Signed-off-by: Sasha Levin
---
drivers/gpio/gpio-mxc.c | 41 -
1 file changed, 24 insertions(+), 17 deletions(-)
diff --git a/drivers/gpio/gpio-mxc.c b/drivers/gpio/gpio-mxc.c
index 995cf0b9e0b1..2d1dfa1e0745 100644
--- a/drivers/gpio/gpio-mxc.c
+++ b/drivers/gpio/gpio-mxc.c
@@ -17,6 +17,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -550,33 +551,38 @@ static void mxc_gpio_restore_regs(struct mxc_gpio_port
*port)
writel(port->gpio_saved_reg.dr, port->base + GPIO_DR);
}
-static int __maybe_unused mxc_gpio_noirq_suspend(struct device *dev)
+static int mxc_gpio_syscore_suspend(void)
{
- struct platform_device *pdev = to_platform_device(dev);
- struct mxc_gpio_port *port = platform_get_drvdata(pdev);
+ struct mxc_gpio_port *port;
- mxc_gpio_save_regs(port);
- clk_disable_unprepare(port->clk);
+ /* walk through all ports */
+ list_for_each_entry(port, &mxc_gpio_ports, node) {
+ mxc_gpio_save_regs(port);
+ clk_disable_unprepare(port->clk);
+ }
return 0;
}
-static int __maybe_unused mxc_gpio_noirq_resume(struct device *dev)
+static void mxc_gpio_syscore_resume(void)
{
- struct platform_device *pdev = to_platform_device(dev);
- struct mxc_gpio_port *port = platform_get_drvdata(pdev);
+ struct mxc_gpio_port *port;
int ret;
- ret = clk_prepare_enable(port->clk);
- if (ret)
- return ret;
- mxc_gpio_restore_regs(port);
-
- return 0;
+ /* walk through all ports */
+ list_for_each_entry(port, &mxc_gpio_ports, node) {
+ ret = clk_prepare_enable(port->clk);
+ if (ret) {
+ pr_err("mxc: failed to enable gpio clock %d\n", ret);
+ return;
+ }
+ mxc_gpio_restore_regs(port);
+ }
}
-static const struct dev_pm_ops mxc_gpio_dev_pm_ops = {
- SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(mxc_gpio_noirq_suspend,
mxc_gpio_noirq_resume)
+static struct syscore_ops mxc_gpio_syscore_ops = {
+ .suspend = mxc_gpio_syscore_suspend,
+ .resume = mxc_gpio_syscore_resume,
};
static struct platform_driver mxc_gpio_driver = {
@@ -584,7 +590,6 @@ static struct platform_driver mxc_gpio_driver = {
.name = "gpio-mxc",
.of_match_table = mxc_gpio_dt_ids,
.suppress_bind_attrs = true,
- .pm = &mxc_gpio_dev_pm_ops,
},
.probe = mxc_gpio_probe,
.id_table = mxc_gpio_devtype,
@@ -592,6 +597,8 @@ static struct platform_driver mxc_gpio_driver = {
static int __init gpio_mxc_init(void)
{
+ register_syscore_ops(&mxc_gpio_syscore_ops);
+
return platform_driver_register(&mxc_gpio_driver);
}
subsys_initcall(gpio_mxc_init);
--
2.19.1
[PATCH 4.20 49/92] gpio: mxc: move gpio noirq suspend/resume to syscore phase
4.20-stable review patch. If anyone has any objections, please let me know.
--
commit 1a5287a3dbc34cd0c02c8f64c9131bd23cdfe2bb upstream.
During noirq suspend/resume phase, GPIO irq could arrive
and its registers like IMR will be changed by irq handle
process, to make the GPIO registers exactly when it is
powered ON after resume, move the GPIO noirq suspend/resume
callback to syscore suspend/resume phase, local irq is
disabled at this phase so GPIO registers are atomic.
Fixes: c19fdaeea0aa ("gpio: mxc: add power management support")
Signed-off-by: Anson Huang
Signed-off-by: Linus Walleij
Signed-off-by: Martin Hundebøll
Cc: # 4.19.x+
Signed-off-by: Sasha Levin
---
drivers/gpio/gpio-mxc.c | 41 -
1 file changed, 24 insertions(+), 17 deletions(-)
diff --git a/drivers/gpio/gpio-mxc.c b/drivers/gpio/gpio-mxc.c
index 995cf0b9e0b1..2d1dfa1e0745 100644
--- a/drivers/gpio/gpio-mxc.c
+++ b/drivers/gpio/gpio-mxc.c
@@ -17,6 +17,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -550,33 +551,38 @@ static void mxc_gpio_restore_regs(struct mxc_gpio_port
*port)
writel(port->gpio_saved_reg.dr, port->base + GPIO_DR);
}
-static int __maybe_unused mxc_gpio_noirq_suspend(struct device *dev)
+static int mxc_gpio_syscore_suspend(void)
{
- struct platform_device *pdev = to_platform_device(dev);
- struct mxc_gpio_port *port = platform_get_drvdata(pdev);
+ struct mxc_gpio_port *port;
- mxc_gpio_save_regs(port);
- clk_disable_unprepare(port->clk);
+ /* walk through all ports */
+ list_for_each_entry(port, &mxc_gpio_ports, node) {
+ mxc_gpio_save_regs(port);
+ clk_disable_unprepare(port->clk);
+ }
return 0;
}
-static int __maybe_unused mxc_gpio_noirq_resume(struct device *dev)
+static void mxc_gpio_syscore_resume(void)
{
- struct platform_device *pdev = to_platform_device(dev);
- struct mxc_gpio_port *port = platform_get_drvdata(pdev);
+ struct mxc_gpio_port *port;
int ret;
- ret = clk_prepare_enable(port->clk);
- if (ret)
- return ret;
- mxc_gpio_restore_regs(port);
-
- return 0;
+ /* walk through all ports */
+ list_for_each_entry(port, &mxc_gpio_ports, node) {
+ ret = clk_prepare_enable(port->clk);
+ if (ret) {
+ pr_err("mxc: failed to enable gpio clock %d\n", ret);
+ return;
+ }
+ mxc_gpio_restore_regs(port);
+ }
}
-static const struct dev_pm_ops mxc_gpio_dev_pm_ops = {
- SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(mxc_gpio_noirq_suspend,
mxc_gpio_noirq_resume)
+static struct syscore_ops mxc_gpio_syscore_ops = {
+ .suspend = mxc_gpio_syscore_suspend,
+ .resume = mxc_gpio_syscore_resume,
};
static struct platform_driver mxc_gpio_driver = {
@@ -584,7 +590,6 @@ static struct platform_driver mxc_gpio_driver = {
.name = "gpio-mxc",
.of_match_table = mxc_gpio_dt_ids,
.suppress_bind_attrs = true,
- .pm = &mxc_gpio_dev_pm_ops,
},
.probe = mxc_gpio_probe,
.id_table = mxc_gpio_devtype,
@@ -592,6 +597,8 @@ static struct platform_driver mxc_gpio_driver = {
static int __init gpio_mxc_init(void)
{
+ register_syscore_ops(&mxc_gpio_syscore_ops);
+
return platform_driver_register(&mxc_gpio_driver);
}
subsys_initcall(gpio_mxc_init);
--
2.19.1
[PATCH v4 8/8] ARM: tegra: Add firmware calls required for suspend-resume
In order to resume CPU from suspend via trusted Foundations firmware,
the LP1/LP2 boot vectors shall be specified using the firmware calls.
Signed-off-by: Dmitry Osipenko
---
arch/arm/mach-tegra/pm.c| 14 ++
arch/arm/mach-tegra/reset-handler.S | 26 ++
2 files changed, 40 insertions(+)
diff --git a/arch/arm/mach-tegra/pm.c b/arch/arm/mach-tegra/pm.c
index 66c8cd63dd86..97bdfffc244e 100644
--- a/arch/arm/mach-tegra/pm.c
+++ b/arch/arm/mach-tegra/pm.c
@@ -33,6 +33,7 @@
#include
#include
+#include
#include
#include
#include
@@ -207,6 +208,8 @@ void tegra_idle_lp2_last(void)
if (trusted_foundations_registered())
outer_disable();
+ call_firmware_op(prepare_idle, TF_PM_MODE_LP2);
+
cpu_suspend(PHYS_OFFSET - PAGE_OFFSET, &tegra_sleep_cpu);
/*
@@ -368,6 +371,17 @@ static int tegra_suspend_enter(suspend_state_t state)
if (trusted_foundations_registered())
outer_disable();
+ switch (mode) {
+ case TEGRA_SUSPEND_LP1:
+ call_firmware_op(prepare_idle, TF_PM_MODE_LP1);
+ break;
+ case TEGRA_SUSPEND_LP2:
+ call_firmware_op(prepare_idle, TF_PM_MODE_LP2);
+ break;
+ default:
+ break;
+ }
+
cpu_suspend(PHYS_OFFSET - PAGE_OFFSET, tegra_sleep_func);
/*
diff --git a/arch/arm/mach-tegra/reset-handler.S
b/arch/arm/mach-tegra/reset-handler.S
index c827b185d281..24fb375e11a9 100644
--- a/arch/arm/mach-tegra/reset-handler.S
+++ b/arch/arm/mach-tegra/reset-handler.S
@@ -20,6 +20,7 @@
#include
#include
+#include
#include
#include
@@ -76,6 +77,7 @@ ENTRY(tegra_resume)
orr r1, r1, #1
str r1, [r0]
#endif
+ bl tegra_resume_trusted_foundations
#ifdef CONFIG_CACHE_L2X0
/* L2 cache resume & re-enable */
@@ -88,6 +90,30 @@ end_ca9_scu_l2_resume:
b cpu_resume
ENDPROC(tegra_resume)
+
+/*
+ * tegra_resume_trusted_foundations
+ *
+ * Trusted Foundations firmware initialization.
+ *
+ * Doesn't return if firmware presents.
+ * Corrupted registers: r1, r2
+ */
+ENTRY(tegra_resume_trusted_foundations)
+ /* Check whether Trusted Foundations firmware presents. */
+ mov32 r2, TEGRA_IRAM_BASE + TEGRA_IRAM_RESET_HANDLER_OFFSET
+ ldr r1, =__tegra_cpu_reset_handler_data_offset + \
+ RESET_DATA(TF_PRESENT)
+ ldr r1, [r2, r1]
+ cmp r1, #0
+ reteq lr
+
+ .arch_extension sec
+ /* First call after suspend wakes firmware. No arguments required. */
+ smc #0
+
+ b cpu_resume
+ENDPROC(tegra_resume_trusted_foundations)
#endif
.align L1_CACHE_SHIFT
--
2.20.1
Re: [PATCH 2/4] PM / devfreq: Handle monitor suspend/resume in the devfreq core
Hi Chanwoo,
On Thu, Feb 14, 2019 at 11:10:00PM +0900, Chanwoo Choi wrote:
> Hi Matthias,
>
> 2019년 2월 14일 (목) 오후 7:16, Matthias Kaehlcke 님이 작성:
> >
> > devfreq expects governors to call devfreq_monitor_suspend/resume()
> > in response to DEVFREQ_GOV_SUSPEND/RESUME events. Since the devfreq
> > core itself generates these events and invokes the governor's event
> > handler the suspend/resume of the load monitoring can be done in the
> > common code.
> >
> > Call devfreq_monitor_suspend/resume() when the governor reports a
> > successful handling of DEVFREQ_GOV_SUSPEND/RESUME and remove these
> > calls from the simpleondemand and tegra governors. Make
> > devfreq_monitor_suspend/resume() static since these functions are now
> > only used by the devfreq core.
>
> The devfreq core generates the all events including
> DEVFREQ_GOV_START/STOP/INTERVAL.
> It is possible to make 'devfreq_monitor_*()' function as the static
> instead of exported function.
> And call them in devfreq.c as this patch as following:
>
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -728,6 +728,8 @@ struct devfreq *devfreq_add_device(struct device *dev,
> goto err_init;
> }
>
> + devfreq_monitor_start(devfreq);
> +
> list_add(&devfreq->node, &devfreq_list);
>
> mutex_unlock(&devfreq_list_lock);
> @@ -760,6 +762,7 @@ int devfreq_remove_device(struct devfreq *devfreq)
> if (devfreq->governor)
> devfreq->governor->event_handler(devfreq,
> DEVFREQ_GOV_STOP, NULL);
> + devfreq_monitor_stop(devfreq);
> device_unregister(&devfreq->dev);
>
> return 0;
> @@ -1259,6 +1262,9 @@ static ssize_t polling_interval_store(struct device
> *dev,
> df->governor->event_handler(df, DEVFREQ_GOV_INTERVAL, &value);
> ret = count;
>
> + if (!ret)
> + devfreq_interval_update(devfreq, (unsigned int *)data);
> +
> return ret;
> }
> static DEVICE_ATTR_RW(polling_interval);
> diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
> index 79efa1e..515fb85 100644
> --- a/drivers/devfreq/tegra-devfreq.c
> +++ b/drivers/devfreq/tegra-devfreq.c
> @@ -580,13 +580,11 @@ static int tegra_governor_event_handler(struct
> devfreq *devfreq,
>
> switch (event) {
> case DEVFREQ_GOV_START:
> - devfreq_monitor_start(devfreq);
> tegra_actmon_enable_interrupts(tegra);
> break;
>
> case DEVFREQ_GOV_STOP:
> tegra_actmon_disable_interrupts(tegra);
> - devfreq_monitor_stop(devfreq);
> break;
indeed, that's similar to "[4/4] PM / devfreq: Handle monitor
start/stop in the devfreq core" of this series.
> Instead,
>
> If the governor should execute some codes before and after of
> DEVFREQ_GOV_SUSPEND, DEVFREQ_GOV_RESUME,
> it is impossible to change the order between devfreq_monitor_*() function
> and the specific governor in the event_handler callback function of
> each governor.
>
> For example, if some govenor requires the following sequencue,
> after this patch, it is not possible.
>
> case DEVFREQ_GOV_SUSPEND:
> /* execute some code before devfreq_monitor_suspend() */
> devfreq_monitor_suspend()
> /* execute some code after devfreq_monitor_suspend() */
I agree that the patch introduces this limitation, however I'm not
convinced it is a problem in practice.
For suspend we'd essentially end up with:
governor_suspend
governor->suspend
monitor_suspend
update_status
stop polling
What would a governor want to do after polling is stopped? Is there
any real world or halfway reasonable hypothetical example?
And for resume:
governor_resume
governor->resume
monitor_resume
start polling
Same question here, what would the governor realistically want to do
after polling has started that it couldn't have done before?
Cheers
Matthias
Re: [PATCH 2/4] PM / devfreq: Handle monitor suspend/resume in the devfreq core
Hi Matthias,
2019년 2월 14일 (목) 오후 7:16, Matthias Kaehlcke 님이 작성:
>
> devfreq expects governors to call devfreq_monitor_suspend/resume()
> in response to DEVFREQ_GOV_SUSPEND/RESUME events. Since the devfreq
> core itself generates these events and invokes the governor's event
> handler the suspend/resume of the load monitoring can be done in the
> common code.
>
> Call devfreq_monitor_suspend/resume() when the governor reports a
> successful handling of DEVFREQ_GOV_SUSPEND/RESUME and remove these
> calls from the simpleondemand and tegra governors. Make
> devfreq_monitor_suspend/resume() static since these functions are now
> only used by the devfreq core.
The devfreq core generates the all events including
DEVFREQ_GOV_START/STOP/INTERVAL.
It is possible to make 'devfreq_monitor_*()' function as the static
instead of exported function.
And call them in devfreq.c as this patch as following:
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -728,6 +728,8 @@ struct devfreq *devfreq_add_device(struct device *dev,
goto err_init;
}
+ devfreq_monitor_start(devfreq);
+
list_add(&devfreq->node, &devfreq_list);
mutex_unlock(&devfreq_list_lock);
@@ -760,6 +762,7 @@ int devfreq_remove_device(struct devfreq *devfreq)
if (devfreq->governor)
devfreq->governor->event_handler(devfreq,
DEVFREQ_GOV_STOP, NULL);
+ devfreq_monitor_stop(devfreq);
device_unregister(&devfreq->dev);
return 0;
@@ -1259,6 +1262,9 @@ static ssize_t polling_interval_store(struct device *dev,
df->governor->event_handler(df, DEVFREQ_GOV_INTERVAL, &value);
ret = count;
+ if (!ret)
+ devfreq_interval_update(devfreq, (unsigned int *)data);
+
return ret;
}
static DEVICE_ATTR_RW(polling_interval);
diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
index 79efa1e..515fb85 100644
--- a/drivers/devfreq/tegra-devfreq.c
+++ b/drivers/devfreq/tegra-devfreq.c
@@ -580,13 +580,11 @@ static int tegra_governor_event_handler(struct
devfreq *devfreq,
switch (event) {
case DEVFREQ_GOV_START:
- devfreq_monitor_start(devfreq);
tegra_actmon_enable_interrupts(tegra);
break;
case DEVFREQ_GOV_STOP:
tegra_actmon_disable_interrupts(tegra);
- devfreq_monitor_stop(devfreq);
break;
Instead,
If the governor should execute some codes before and after of
DEVFREQ_GOV_SUSPEND, DEVFREQ_GOV_RESUME,
it is impossible to change the order between devfreq_monitor_*() function
and the specific governor in the event_handler callback function of
each governor.
For example, if some govenor requires the following sequencue,
after this patch, it is not possible.
case DEVFREQ_GOV_SUSPEND:
/* execute some code before devfreq_monitor_suspend() */
devfreq_monitor_suspend()
/* execute some code after devfreq_monitor_suspend() */
>
> Signed-off-by: Matthias Kaehlcke
> ---
> drivers/devfreq/devfreq.c | 18 --
> drivers/devfreq/governor.h| 2 --
> drivers/devfreq/governor_simpleondemand.c | 8
> drivers/devfreq/tegra-devfreq.c | 2 --
> 4 files changed, 8 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 1d3a43f8b3a10..7fab6c4cf719b 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -447,15 +447,13 @@ EXPORT_SYMBOL(devfreq_monitor_stop);
> * devfreq_monitor_suspend() - Suspend load monitoring of a devfreq instance
> * @devfreq: the devfreq instance.
> *
> - * Helper function to suspend devfreq device load monitoing. Function
> - * to be called from governor in response to DEVFREQ_GOV_SUSPEND
> - * event or when polling interval is set to zero.
> + * Helper function to suspend devfreq device load monitoring.
> *
> * Note: Though this function is same as devfreq_monitor_stop(),
> * intentionally kept separate to provide hooks for collecting
> * transition statistics.
> */
> -void devfreq_monitor_suspend(struct devfreq *devfreq)
> +static void devfreq_monitor_suspend(struct devfreq *devfreq)
> {
> mutex_lock(&devfreq->lock);
> if (devfreq->monitor_state != DEVFREQ_MONITOR_RUNNING) {
> @@ -468,17 +466,14 @@ void devfreq_monitor_suspend(struct devfreq *devfreq)
> mutex_unlock(&devfreq->lock);
> cancel_delayed_work_sync(&devfreq->work);
> }
> -EXPORT_SYMBOL(devfreq_monitor_suspend);
>
> /**
> * devfreq_monitor_resume() - Resume load monitoring of a devfreq instance
> * @devfreq:the devfr
[PATCH 2/4] PM / devfreq: Handle monitor suspend/resume in the devfreq core
devfreq expects governors to call devfreq_monitor_suspend/resume()
in response to DEVFREQ_GOV_SUSPEND/RESUME events. Since the devfreq
core itself generates these events and invokes the governor's event
handler the suspend/resume of the load monitoring can be done in the
common code.
Call devfreq_monitor_suspend/resume() when the governor reports a
successful handling of DEVFREQ_GOV_SUSPEND/RESUME and remove these
calls from the simpleondemand and tegra governors. Make
devfreq_monitor_suspend/resume() static since these functions are now
only used by the devfreq core.
Signed-off-by: Matthias Kaehlcke
---
drivers/devfreq/devfreq.c | 18 --
drivers/devfreq/governor.h| 2 --
drivers/devfreq/governor_simpleondemand.c | 8
drivers/devfreq/tegra-devfreq.c | 2 --
4 files changed, 8 insertions(+), 22 deletions(-)
diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 1d3a43f8b3a10..7fab6c4cf719b 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -447,15 +447,13 @@ EXPORT_SYMBOL(devfreq_monitor_stop);
* devfreq_monitor_suspend() - Suspend load monitoring of a devfreq instance
* @devfreq: the devfreq instance.
*
- * Helper function to suspend devfreq device load monitoing. Function
- * to be called from governor in response to DEVFREQ_GOV_SUSPEND
- * event or when polling interval is set to zero.
+ * Helper function to suspend devfreq device load monitoring.
*
* Note: Though this function is same as devfreq_monitor_stop(),
* intentionally kept separate to provide hooks for collecting
* transition statistics.
*/
-void devfreq_monitor_suspend(struct devfreq *devfreq)
+static void devfreq_monitor_suspend(struct devfreq *devfreq)
{
mutex_lock(&devfreq->lock);
if (devfreq->monitor_state != DEVFREQ_MONITOR_RUNNING) {
@@ -468,17 +466,14 @@ void devfreq_monitor_suspend(struct devfreq *devfreq)
mutex_unlock(&devfreq->lock);
cancel_delayed_work_sync(&devfreq->work);
}
-EXPORT_SYMBOL(devfreq_monitor_suspend);
/**
* devfreq_monitor_resume() - Resume load monitoring of a devfreq instance
* @devfreq:the devfreq instance.
*
- * Helper function to resume devfreq device load monitoing. Function
- * to be called from governor in response to DEVFREQ_GOV_RESUME
- * event or when polling interval is set to non-zero.
+ * Helper function to resume devfreq device load monitoring.
*/
-void devfreq_monitor_resume(struct devfreq *devfreq)
+static void devfreq_monitor_resume(struct devfreq *devfreq)
{
unsigned long freq;
@@ -501,7 +496,6 @@ void devfreq_monitor_resume(struct devfreq *devfreq)
out:
mutex_unlock(&devfreq->lock);
}
-EXPORT_SYMBOL(devfreq_monitor_resume);
/**
* devfreq_interval_update() - Update device devfreq monitoring interval
@@ -903,6 +897,8 @@ int devfreq_suspend_device(struct devfreq *devfreq)
DEVFREQ_GOV_SUSPEND, NULL);
if (ret)
return ret;
+
+ devfreq_monitor_suspend(devfreq);
}
if (devfreq->suspend_freq) {
@@ -944,6 +940,8 @@ int devfreq_resume_device(struct devfreq *devfreq)
DEVFREQ_GOV_RESUME, NULL);
if (ret)
return ret;
+
+ devfreq_monitor_resume(devfreq);
}
return 0;
diff --git a/drivers/devfreq/governor.h b/drivers/devfreq/governor.h
index f53339ca610fc..d136792c0cc91 100644
--- a/drivers/devfreq/governor.h
+++ b/drivers/devfreq/governor.h
@@ -59,8 +59,6 @@ struct devfreq_governor {
extern void devfreq_monitor_start(struct devfreq *devfreq);
extern void devfreq_monitor_stop(struct devfreq *devfreq);
-extern void devfreq_monitor_suspend(struct devfreq *devfreq);
-extern void devfreq_monitor_resume(struct devfreq *devfreq);
extern void devfreq_interval_update(struct devfreq *devfreq,
unsigned int *delay);
diff --git a/drivers/devfreq/governor_simpleondemand.c
b/drivers/devfreq/governor_simpleondemand.c
index c0417f0e081e0..52eb0c734b312 100644
--- a/drivers/devfreq/governor_simpleondemand.c
+++ b/drivers/devfreq/governor_simpleondemand.c
@@ -103,14 +103,6 @@ static int devfreq_simple_ondemand_handler(struct devfreq
*devfreq,
devfreq_interval_update(devfreq, (unsigned int *)data);
break;
- case DEVFREQ_GOV_SUSPEND:
- devfreq_monitor_suspend(devfreq);
- break;
-
- case DEVFREQ_GOV_RESUME:
- devfreq_monitor_resume(devfreq);
- break;
-
default:
break;
}
diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
index c59d2eee5d309..79efa1e51bd06 100644
--- a/drivers/devfreq/tegra-devfreq.c
+++ b/drivers/devfreq/tegra-devfreq.c
@@ -591,11 +591,9 @@ static
Re: [PATCH v1 2/2] PCI/ASPM: Save LTR Capability for suspend/resume
On Wed, Feb 13, 2019 at 10:53:01AM +0530, Vidya Sagar wrote:
> On 2/12/2019 4:25 AM, Bjorn Helgaas wrote:
> > From: Bjorn Helgaas
> >
> > Latency Tolerance Reporting (LTR) allows Endpoints and Switch Upstream
> > Ports to report their latency requirements to upstream components. If ASPM
> > L1 PM substates are enabled, the LTR information helps determine when a
> > Link enters L1.2 [1].
> >
> > Software must set the maximum latency values in the LTR Capability based on
> > characteristics of the platform, then set LTR Mechanism Enable in the
> > Device Control 2 register in the PCIe Capability. The device can then use
> > LTR to report its latency tolerance.
> >
> > If the device reports a maximum latency value of zero, that means the
> > device requires the highest possible performance and the ASPM L1.2 substate
> > is effectively disabled.
> >
> > We put devices in D3 for suspend, and we assume their internal state is
> > lost. On resume, previously we did not restore the LTR Capability, but we
> > did restore the LTR Mechanism Enable bit, so devices would request the
> > highest possible performance and ASPM L1.2 wouldn't be used.
> >
> > [1] PCIe r4.0, sec 5.5.1
> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=201469
> > Signed-off-by: Bjorn Helgaas
> > ---
> > drivers/pci/pci.c | 53
> > +++--
> > 1 file changed, 51 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index c9d8e3c837de..13d65991c77b 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -1233,7 +1233,6 @@ static void pci_restore_pcie_state(struct pci_dev
> > *dev)
> > pcie_capability_write_word(dev, PCI_EXP_SLTCTL2, cap[i++]);
> > }
> > -
> > static int pci_save_pcix_state(struct pci_dev *dev)
> > {
> > int pos;
> > @@ -1270,6 +1269,45 @@ static void pci_restore_pcix_state(struct pci_dev
> > *dev)
> > pci_write_config_word(dev, pos + PCI_X_CMD, cap[i++]);
> > }
> > +static void pci_save_ltr_state(struct pci_dev *dev)
> > +{
> > + int ltr;
> > + struct pci_cap_saved_state *save_state;
> > + u16 *cap;
> > +
> > + if (!pci_is_pcie(dev))
> > + return;
> > +
> > + ltr = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_LTR);
> > + if (!ltr)
> > + return;
> > +
> > + save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_LTR);
> > + if (!save_state) {
> > + pci_err(dev, "no suspend buffer for LTR; ASPM issues possible
> > after resume\n");
> > + return;
> > + }
> > +
> > + cap = (u16 *)&save_state->cap.data[0];
> > + pci_read_config_word(dev, ltr + PCI_LTR_MAX_SNOOP_LAT, cap++);
> > + pci_read_config_word(dev, ltr + PCI_LTR_MAX_NOSNOOP_LAT, cap++);
> > +}
> > +
> > +static void pci_restore_ltr_state(struct pci_dev *dev)
> > +{
> > + struct pci_cap_saved_state *save_state;
> > + int ltr;
> > + u16 *cap;
> > +
> > + save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_LTR);
> > + ltr = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_LTR);
> > + if (!save_state || !ltr)
> > + return;
> > +
> > + cap = (u16 *)&save_state->cap.data[0];
> > + pci_write_config_word(dev, ltr + PCI_LTR_MAX_SNOOP_LAT, *cap++);
> > + pci_write_config_word(dev, ltr + PCI_LTR_MAX_NOSNOOP_LAT, *cap++);
> > +}
> > /**
> >* pci_save_state - save the PCI configuration space of a device before
> > suspending
> > @@ -1291,6 +1329,7 @@ int pci_save_state(struct pci_dev *dev)
> > if (i != 0)
> > return i;
> > + pci_save_ltr_state(dev);
> > pci_save_dpc_state(dev);
> > return pci_save_vc_state(dev);
> > }
> > @@ -1390,7 +1429,12 @@ void pci_restore_state(struct pci_dev *dev)
> > if (!dev->state_saved)
> > return;
> > - /* PCI Express register must be restored first */
> > + /*
> > +* Restore max latencies (in the LTR capability) before enabling
> > +* LTR itself (in the PCIe capability).
> > +*/
> > + pci_restore_ltr_state(dev);
> > +
> > pci_restore_pcie_state(dev);
> > pci_restore_pasid_state(dev);
> > pci_restore_pri_state(dev);
> > @@ -2998,6 +3042,11 @@ void pci_allocate_cap_save_buffers(struct pci_dev
> > *dev)
> > if (error)
> > pci_err(dev, "unable to preallocate PCI-X save buffer\n");
> > + error = pci_add_ext_cap_save_buffer(dev, PCI_EXT_CAP_ID_LTR,
> > + 2 * sizeof(u16));
> > + if (error)
> > + pci_err(dev, "unable to allocate suspend buffer for LTR\n");
> > +
> > pci_allocate_vc_save_buffers(dev);
> > }
> Don't we have to save and restore L1SS control registers (PCI_L1SS_CTL1 &
> PCI_L1SS_CTL2) as well?
I think you're right! It's getting embarrassing how much stuff we
missed. I'll work on this, and look for other capabilities where we
might be missing save/restore.
Bjorn
Re: [PATCH v1 2/2] PCI/ASPM: Save LTR Capability for suspend/resume
On 2/12/2019 4:25 AM, Bjorn Helgaas wrote:
From: Bjorn Helgaas
Latency Tolerance Reporting (LTR) allows Endpoints and Switch Upstream
Ports to report their latency requirements to upstream components. If ASPM
L1 PM substates are enabled, the LTR information helps determine when a
Link enters L1.2 [1].
Software must set the maximum latency values in the LTR Capability based on
characteristics of the platform, then set LTR Mechanism Enable in the
Device Control 2 register in the PCIe Capability. The device can then use
LTR to report its latency tolerance.
If the device reports a maximum latency value of zero, that means the
device requires the highest possible performance and the ASPM L1.2 substate
is effectively disabled.
We put devices in D3 for suspend, and we assume their internal state is
lost. On resume, previously we did not restore the LTR Capability, but we
did restore the LTR Mechanism Enable bit, so devices would request the
highest possible performance and ASPM L1.2 wouldn't be used.
[1] PCIe r4.0, sec 5.5.1
Link: https://bugzilla.kernel.org/show_bug.cgi?id=201469
Signed-off-by: Bjorn Helgaas
---
drivers/pci/pci.c | 53 +++--
1 file changed, 51 insertions(+), 2 deletions(-)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index c9d8e3c837de..13d65991c77b 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1233,7 +1233,6 @@ static void pci_restore_pcie_state(struct pci_dev *dev)
pcie_capability_write_word(dev, PCI_EXP_SLTCTL2, cap[i++]);
}
-
static int pci_save_pcix_state(struct pci_dev *dev)
{
int pos;
@@ -1270,6 +1269,45 @@ static void pci_restore_pcix_state(struct pci_dev *dev)
pci_write_config_word(dev, pos + PCI_X_CMD, cap[i++]);
}
+static void pci_save_ltr_state(struct pci_dev *dev)
+{
+ int ltr;
+ struct pci_cap_saved_state *save_state;
+ u16 *cap;
+
+ if (!pci_is_pcie(dev))
+ return;
+
+ ltr = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_LTR);
+ if (!ltr)
+ return;
+
+ save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_LTR);
+ if (!save_state) {
+ pci_err(dev, "no suspend buffer for LTR; ASPM issues possible after
resume\n");
+ return;
+ }
+
+ cap = (u16 *)&save_state->cap.data[0];
+ pci_read_config_word(dev, ltr + PCI_LTR_MAX_SNOOP_LAT, cap++);
+ pci_read_config_word(dev, ltr + PCI_LTR_MAX_NOSNOOP_LAT, cap++);
+}
+
+static void pci_restore_ltr_state(struct pci_dev *dev)
+{
+ struct pci_cap_saved_state *save_state;
+ int ltr;
+ u16 *cap;
+
+ save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_LTR);
+ ltr = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_LTR);
+ if (!save_state || !ltr)
+ return;
+
+ cap = (u16 *)&save_state->cap.data[0];
+ pci_write_config_word(dev, ltr + PCI_LTR_MAX_SNOOP_LAT, *cap++);
+ pci_write_config_word(dev, ltr + PCI_LTR_MAX_NOSNOOP_LAT, *cap++);
+}
/**
* pci_save_state - save the PCI configuration space of a device before
suspending
@@ -1291,6 +1329,7 @@ int pci_save_state(struct pci_dev *dev)
if (i != 0)
return i;
+ pci_save_ltr_state(dev);
pci_save_dpc_state(dev);
return pci_save_vc_state(dev);
}
@@ -1390,7 +1429,12 @@ void pci_restore_state(struct pci_dev *dev)
if (!dev->state_saved)
return;
- /* PCI Express register must be restored first */
+ /*
+* Restore max latencies (in the LTR capability) before enabling
+* LTR itself (in the PCIe capability).
+*/
+ pci_restore_ltr_state(dev);
+
pci_restore_pcie_state(dev);
pci_restore_pasid_state(dev);
pci_restore_pri_state(dev);
@@ -2998,6 +3042,11 @@ void pci_allocate_cap_save_buffers(struct pci_dev *dev)
if (error)
pci_err(dev, "unable to preallocate PCI-X save buffer\n");
+ error = pci_add_ext_cap_save_buffer(dev, PCI_EXT_CAP_ID_LTR,
+ 2 * sizeof(u16));
+ if (error)
+ pci_err(dev, "unable to allocate suspend buffer for LTR\n");
+
pci_allocate_vc_save_buffers(dev);
}
Don't we have to save and restore L1SS control registers (PCI_L1SS_CTL1
& PCI_L1SS_CTL2) as well?
[PATCH] mailbox: imx: keep MU irq working during suspend/resume
During noirq suspend phase, mailbox MU irq will be masked
but many drivers still need to communicate with system
controller firmware via mailbox, if MU irq is masked, it
will cause RPC timeout as below:
[ 23.372103] imx-scu scu: RPC send msg timeout
Setting MU irq to be wakeup source is NOT working as GIC
driver does NOT have .irq_set_wake implemented, so to
support suspend/resume, just make imx mailbox driver NOT
suspend, since MU is always a wakeup source on i.MX platforms
with system controller inside, and its power/clock is
maintained by system controller, mailbox driver no need
to manage them.
Signed-off-by: Anson Huang
---
drivers/mailbox/imx-mailbox.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/mailbox/imx-mailbox.c b/drivers/mailbox/imx-mailbox.c
index 774362a..85fc5b5 100644
--- a/drivers/mailbox/imx-mailbox.c
+++ b/drivers/mailbox/imx-mailbox.c
@@ -187,8 +187,8 @@ static int imx_mu_startup(struct mbox_chan *chan)
return 0;
}
- ret = request_irq(priv->irq, imx_mu_isr, IRQF_SHARED, cp->irq_desc,
- chan);
+ ret = request_irq(priv->irq, imx_mu_isr, IRQF_SHARED |
+ IRQF_NO_SUSPEND, cp->irq_desc, chan);
if (ret) {
dev_err(priv->dev,
"Unable to acquire IRQ %d\n", priv->irq);
--
2.7.4
[PATCH v1 2/2] PCI/ASPM: Save LTR Capability for suspend/resume
From: Bjorn Helgaas
Latency Tolerance Reporting (LTR) allows Endpoints and Switch Upstream
Ports to report their latency requirements to upstream components. If ASPM
L1 PM substates are enabled, the LTR information helps determine when a
Link enters L1.2 [1].
Software must set the maximum latency values in the LTR Capability based on
characteristics of the platform, then set LTR Mechanism Enable in the
Device Control 2 register in the PCIe Capability. The device can then use
LTR to report its latency tolerance.
If the device reports a maximum latency value of zero, that means the
device requires the highest possible performance and the ASPM L1.2 substate
is effectively disabled.
We put devices in D3 for suspend, and we assume their internal state is
lost. On resume, previously we did not restore the LTR Capability, but we
did restore the LTR Mechanism Enable bit, so devices would request the
highest possible performance and ASPM L1.2 wouldn't be used.
[1] PCIe r4.0, sec 5.5.1
Link: https://bugzilla.kernel.org/show_bug.cgi?id=201469
Signed-off-by: Bjorn Helgaas
---
drivers/pci/pci.c | 53 +++--
1 file changed, 51 insertions(+), 2 deletions(-)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index c9d8e3c837de..13d65991c77b 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1233,7 +1233,6 @@ static void pci_restore_pcie_state(struct pci_dev *dev)
pcie_capability_write_word(dev, PCI_EXP_SLTCTL2, cap[i++]);
}
-
static int pci_save_pcix_state(struct pci_dev *dev)
{
int pos;
@@ -1270,6 +1269,45 @@ static void pci_restore_pcix_state(struct pci_dev *dev)
pci_write_config_word(dev, pos + PCI_X_CMD, cap[i++]);
}
+static void pci_save_ltr_state(struct pci_dev *dev)
+{
+ int ltr;
+ struct pci_cap_saved_state *save_state;
+ u16 *cap;
+
+ if (!pci_is_pcie(dev))
+ return;
+
+ ltr = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_LTR);
+ if (!ltr)
+ return;
+
+ save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_LTR);
+ if (!save_state) {
+ pci_err(dev, "no suspend buffer for LTR; ASPM issues possible
after resume\n");
+ return;
+ }
+
+ cap = (u16 *)&save_state->cap.data[0];
+ pci_read_config_word(dev, ltr + PCI_LTR_MAX_SNOOP_LAT, cap++);
+ pci_read_config_word(dev, ltr + PCI_LTR_MAX_NOSNOOP_LAT, cap++);
+}
+
+static void pci_restore_ltr_state(struct pci_dev *dev)
+{
+ struct pci_cap_saved_state *save_state;
+ int ltr;
+ u16 *cap;
+
+ save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_LTR);
+ ltr = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_LTR);
+ if (!save_state || !ltr)
+ return;
+
+ cap = (u16 *)&save_state->cap.data[0];
+ pci_write_config_word(dev, ltr + PCI_LTR_MAX_SNOOP_LAT, *cap++);
+ pci_write_config_word(dev, ltr + PCI_LTR_MAX_NOSNOOP_LAT, *cap++);
+}
/**
* pci_save_state - save the PCI configuration space of a device before
suspending
@@ -1291,6 +1329,7 @@ int pci_save_state(struct pci_dev *dev)
if (i != 0)
return i;
+ pci_save_ltr_state(dev);
pci_save_dpc_state(dev);
return pci_save_vc_state(dev);
}
@@ -1390,7 +1429,12 @@ void pci_restore_state(struct pci_dev *dev)
if (!dev->state_saved)
return;
- /* PCI Express register must be restored first */
+ /*
+* Restore max latencies (in the LTR capability) before enabling
+* LTR itself (in the PCIe capability).
+*/
+ pci_restore_ltr_state(dev);
+
pci_restore_pcie_state(dev);
pci_restore_pasid_state(dev);
pci_restore_pri_state(dev);
@@ -2998,6 +3042,11 @@ void pci_allocate_cap_save_buffers(struct pci_dev *dev)
if (error)
pci_err(dev, "unable to preallocate PCI-X save buffer\n");
+ error = pci_add_ext_cap_save_buffer(dev, PCI_EXT_CAP_ID_LTR,
+ 2 * sizeof(u16));
+ if (error)
+ pci_err(dev, "unable to allocate suspend buffer for LTR\n");
+
pci_allocate_vc_save_buffers(dev);
}
PCI LTR - ASPM handling upon suspend / resume cycle. Regression since 4.18
Hi, Lately we (Intel) have got a few bugs on suspend / resume. The complaint is that our device becomes unavailable after suspend / resume cycle. The bug on which we have most data is [1]. The original submitter reported a regression since commit 9ab105deb60fa76d66cae5548819b4e8703d2056: PCI/ASPM: Disable ASPM L1.2 Substate if we don't have LTR When in the ASPM L1.0 state (but not the PCI-PM L1.0 state), the most recent LTR value and the LTR_L1.2_THRESHOLD determines whether the link enters the L1.2 substate. If we don't have LTR enabled, prevent the use of ASPM L1.2. PCI-PM L1.2 may still be used because it doesn't depend on LTR_L1.2_THRESHOLD (see PCIe r4.0, sec 5.5.1). After this commit, L1.2 is disabled upon resume: L1SubCtl1: PCI-PM_L1.2+ PCI-PM_L1.1+ ASPM_L1.2- ASPM_L1.1+ T_CommonMode=0us LTR1.2_Threshold=163840ns Whereas it wasn't before this commit: L1SubCtl1: PCI-PM_L1.2+ PCI-PM_L1.1+ ASPM_L1.2+ ASPM_L1.1+ T_CommonMode=0us LTR1.2_Threshold=163840ns I am copying here an initial analysis by Bjorn (from [2]): 1) Linux has no support for saving/restoring the Max Latency values in the LTR Capability. This results in the latencies being zero after you resume, as you see in the lspci output. The device still *works* after resume, but power consumption should increase because the device is effectively requesting the best possible service, so we probably don't use the L1.2 state at all. 2) Linux has no support for programming the Max Latency values for hot- added devices. When using ACPI hotplug, firmware may do this, but for native PCIe hotplug (pciehp), the new device should again be requesting the best possible service, resulting in more power consumption than necessary. The platform is supposed to supply a _DSM method with information required to program these values Another user found another commit impacting his device after suspend / resume: commit 6f9db69ad93cd6ab77d5571cf748ff7cdcfb0285 ACPI / PM: Default to s2idle in all machines supporting LP S0 The Dell Venue Pro 7140 supports the Low Power S0 Idle state, but does not support any of the _DSM functions that the current heuristic checks for. Since suspend-to-mem can not be safely performed on this machine, and since the bitfield check can't cover this case, it is safer to enable s2idle by default by checking for the presence of the _DSM alone and removing the bitfield check. This user confirmed that using suspend-to-mem instead of suspend-to- idle works for him. A user contacted my privately to let me know that he has issues with devices from other vendors although I can't tell if the problem is the same or not. Note that this problem started from kernel 4.18. Thank you. [1] - https://bugzilla.kernel.org/show_bug.cgi?id=201469 [2] - https://bugzilla.kernel.org/show_bug.cgi?id=201469#c26
Re: [PATCH v2 0/3] scsi: arcmsr: Fix suspend/resume of ACB_ADAPTER_TYPE_B part 2
On Tue, 2019-01-22 at 21:41 -0500, Martin K. Petersen wrote: > Ching, > > > This patch series are against to mkp's 5.1/scsi-queue. > > Applied to 5.1/scsi-queue. Thank you. > > PS. Your file permissions are odd. I always have to change your diffs > from 755 to 644 before applying. > Thanks Martin and Dan's help. The file permission problem also confused to me. I used Evolution mail of CentOS 6.x to submit the patches. The mail context format is Plain text, preformatted. I inserted the diff text file to the mail, and diff file listing as below. -rw-r--r--. 1 root root 1663 Jan 16 04:11 p1.txt Don't know why and when it's permission changed from 644 to 755.
Re: [PATCH v2 0/3] scsi: arcmsr: Fix suspend/resume of ACB_ADAPTER_TYPE_B part 2
Ching, > This patch series are against to mkp's 5.1/scsi-queue. Applied to 5.1/scsi-queue. Thank you. PS. Your file permissions are odd. I always have to change your diffs from 755 to 644 before applying. -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH net-next] net: dsa: mv88e6xxx: Add suspend/resume callbacks
Hi Andrew, Andrew Lunn wrote on Tue, 22 Jan 2019 14:20:05 +0100: > > I am not sure to understand what is lost. On my setup ethtool shows > > that everything is fine after resume but maybe I fall into a "default" > > working case. > > Hi Miquèl > > Is the power removed from the switch? If so, you need to restore the > full switch configuration. The current code might be sufficient for > runtime suspend, where the switch is put into a low power mode, but it > is kept powered. It is not sufficient for a full power cycle. > > > When I compare with the two drivers pointed out by Andrew: > > * qca8k resume callback: > > * enable ports, > > * call dsa_switch_resume(). > > * bcm_sf2 resume callback: > > * call dsa_switch_resume(), > > * reset the switch, > > * refresh rules (Not applicable?), > > * enable the PHYs, > > * setup the ports, > > * configure vlan (Not applicable?), > > * mv88e6xxx resume callback: > > * reset the switch, > > * enable the PHYs, > > * setup the ports, > > * enable IRQs, > > * call dsa_switch_resume(). > > Here are some commands to try before you suspend: > > ip link add name br0 type bridge > ip link set br0 up > ip link set lan1 up > ip link set lan1 master br0 > ip link set lan2 up > ip link set lan2 master br0 > > At this point, you should be able to pass traffic between lan1 and > lan2. > > bridge fdb add 00:42:42:42:42:42 lan1 > bridge fdb add 00:24:24:24:24:24 lan2 > > That adds two static forwarding database entries. You can show these using > > bridge fdb show > > There are likely to be additional dynamic FDB entries. It is O.K. to > loose the dynamic entries over a suspend/resume cycle, but the static > ones must remain. Thank you very much for this, it is really helpful. > > > This looks pretty similar. Maybe the ports setup are set to default > > values while I should save some parameters at suspend? I changed a > > few parameters (like the MTU or the queue length) but they seem to be > > correct across suspend cycles. > > MTU and queue length have nothing to do with the actual switch. Your > tests need to actually program the hardware. I didn't checked the hw was programmed indeed. Well, I'll try with the test described above and will propose something. Thanks, Miquèl
Re: [PATCH net-next] net: dsa: mv88e6xxx: Add suspend/resume callbacks
> I am not sure to understand what is lost. On my setup ethtool shows > that everything is fine after resume but maybe I fall into a "default" > working case. Hi Miquèl Is the power removed from the switch? If so, you need to restore the full switch configuration. The current code might be sufficient for runtime suspend, where the switch is put into a low power mode, but it is kept powered. It is not sufficient for a full power cycle. > When I compare with the two drivers pointed out by Andrew: > * qca8k resume callback: > * enable ports, > * call dsa_switch_resume(). > * bcm_sf2 resume callback: > * call dsa_switch_resume(), > * reset the switch, > * refresh rules (Not applicable?), > * enable the PHYs, > * setup the ports, > * configure vlan (Not applicable?), > * mv88e6xxx resume callback: > * reset the switch, > * enable the PHYs, > * setup the ports, > * enable IRQs, > * call dsa_switch_resume(). Here are some commands to try before you suspend: ip link add name br0 type bridge ip link set br0 up ip link set lan1 up ip link set lan1 master br0 ip link set lan2 up ip link set lan2 master br0 At this point, you should be able to pass traffic between lan1 and lan2. bridge fdb add 00:42:42:42:42:42 lan1 bridge fdb add 00:24:24:24:24:24 lan2 That adds two static forwarding database entries. You can show these using bridge fdb show There are likely to be additional dynamic FDB entries. It is O.K. to loose the dynamic entries over a suspend/resume cycle, but the static ones must remain. > This looks pretty similar. Maybe the ports setup are set to default > values while I should save some parameters at suspend? I changed a > few parameters (like the MTU or the queue length) but they seem to be > correct across suspend cycles. MTU and queue length have nothing to do with the actual switch. Your tests need to actually program the hardware. Andrew
Re: [PATCH net-next] net: dsa: mv88e6xxx: Add suspend/resume callbacks
Hi Florian, Florian Fainelli wrote on Thu, 17 Jan 2019 10:00:46 -0800: > On 1/17/19 7:50 AM, Miquel Raynal wrote: > > Hi Andrew, Vivien, > > > > Vivien Didelot wrote on Thu, 17 Jan 2019 > > 10:46:41 -0500: > > > >> Hi, > >> > >> On Wed, 16 Jan 2019 23:23:29 +0100, Andrew Lunn wrote: > >>> Hi Florian > >>> > >>>> A possible approach could be to call the port_disable, port_enable > >>>> callbacks from dsa_slave_suspend() and dsa_slave_resume(), I might have > >>>> some patches doing that already somewhere. > >>> > >>> I expect it is also on Viviens TODO list, since this really could be > >>> in the core. > >> > >> Indeed that is! > > > > So, shall I wait for Vivien's patches (adding port_disable/enable() > > in dsa_slave_suspend/resume()) and keep the driver as-is or do you want > > me to manually call port_disable/enable() from the mv88e6xxx driver? > > Up to you guys, the only thing that I an tell you is that my platform > loses its register contents during suspend/resume, therefore you must > make sure the driver re-applies the entire switch configuration, > identical to how it was prior to suspend. If you need me to test > something, please holler. I am not sure to understand what is lost. On my setup ethtool shows that everything is fine after resume but maybe I fall into a "default" working case. When I compare with the two drivers pointed out by Andrew: * qca8k resume callback: * enable ports, * call dsa_switch_resume(). * bcm_sf2 resume callback: * call dsa_switch_resume(), * reset the switch, * refresh rules (Not applicable?), * enable the PHYs, * setup the ports, * configure vlan (Not applicable?), * mv88e6xxx resume callback: * reset the switch, * enable the PHYs, * setup the ports, * enable IRQs, * call dsa_switch_resume(). This looks pretty similar. Maybe the ports setup are set to default values while I should save some parameters at suspend? I changed a few parameters (like the MTU or the queue length) but they seem to be correct across suspend cycles. Can you show a diff of what part of the configuration is lost? Anyway, I have little background working with switches, so I might miss something big. Thanks, Miquèl
