* [PATCH/RFC] fbdev: sh_mobile_hdmi: Re-init regs before irq re-enable on resume
@ 2014-09-23 12:21 ` Geert Uytterhoeven
0 siblings, 0 replies; 18+ messages in thread
From: Geert Uytterhoeven @ 2014-09-23 12:21 UTC (permalink / raw)
To: Jean-Christophe Plagniol-Villard, Tomi Valkeinen, Rafael J. Wysocki
Cc: Ulf Hansson, Guennadi Liakhovetski, linux-fbdev, linux-pm,
linux-sh, Geert Uytterhoeven
When the PM domain containing the HDMI hardware block is powered down,
the HDMI register values (incl. interrupt polarity settings) are lost.
During resume, after powering up the PM domain, interrupts are
re-enabled, and an interrupt storm happens due to incorrect interrupt
polarity settings:
irq 163: nobody cared (try booting with the "irqpoll" option)
...
Disabling IRQ #163
To fix this, re-initialize the interrupt polarity settings, and the
htop1 register block (if present), during resume.
As the .suspend_noirq() and .resume_noirq() callbacks are not called
when using the generic PM domain, the normal .resume() callback is used,
and the device interrupt needs to be disabled/enabled manually.
This fixes resume from s2ram with power down of the A4MP PM domain on
r8a7740/Armadillo.
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
Is there a specific reason why the .suspend_noirq() and .resume_noirq()
callbacks are not called when using genpd, unlike .suspend(),
.suspend_late(), .resume_early(), and .resume()?
---
drivers/video/fbdev/sh_mobile_hdmi.c | 44 ++++++++++++++++++++++++++++++++++--
1 file changed, 42 insertions(+), 2 deletions(-)
diff --git a/drivers/video/fbdev/sh_mobile_hdmi.c b/drivers/video/fbdev/sh_mobile_hdmi.c
index 9a33ee0413fb584d..7c72a3f02056445d 100644
--- a/drivers/video/fbdev/sh_mobile_hdmi.c
+++ b/drivers/video/fbdev/sh_mobile_hdmi.c
@@ -281,6 +281,7 @@ struct sh_hdmi {
u8 edid_block_addr;
u8 edid_segment_nr;
u8 edid_blocks;
+ int irq;
struct clk *hdmi_clk;
struct device *dev;
struct delayed_work edid_work;
@@ -1299,6 +1300,7 @@ static int __init sh_hdmi_probe(struct platform_device *pdev)
hdmi->dev = &pdev->dev;
hdmi->entity.owner = THIS_MODULE;
hdmi->entity.ops = &sh_hdmi_ops;
+ hdmi->irq = irq;
hdmi->hdmi_clk = clk_get(&pdev->dev, "ick");
if (IS_ERR(hdmi->hdmi_clk)) {
@@ -1415,12 +1417,11 @@ static int __exit sh_hdmi_remove(struct platform_device *pdev)
{
struct sh_hdmi *hdmi = entity_to_sh_hdmi(platform_get_drvdata(pdev));
struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- int irq = platform_get_irq(pdev, 0);
snd_soc_unregister_codec(&pdev->dev);
/* No new work will be scheduled, wait for running ISR */
- free_irq(irq, hdmi);
+ free_irq(hdmi->irq, hdmi);
/* Wait for already scheduled work */
cancel_delayed_work_sync(&hdmi->edid_work);
pm_runtime_put(&pdev->dev);
@@ -1435,10 +1436,49 @@ static int __exit sh_hdmi_remove(struct platform_device *pdev)
return 0;
}
+static int sh_hdmi_suspend(struct device *dev)
+{
+ struct platform_device *pdev = to_platform_device(dev);
+ struct sh_hdmi *hdmi = entity_to_sh_hdmi(platform_get_drvdata(pdev));
+
+ disable_irq(hdmi->irq);
+ /* Wait for already scheduled work */
+ cancel_delayed_work_sync(&hdmi->edid_work);
+ return 0;
+}
+
+static int sh_hdmi_resume(struct device *dev)
+{
+ struct platform_device *pdev = to_platform_device(dev);
+ struct sh_mobile_hdmi_info *pdata = dev_get_platdata(dev);
+ struct sh_hdmi *hdmi = entity_to_sh_hdmi(platform_get_drvdata(pdev));
+
+ /* Re-init interrupt polarity */
+ if (pdata->flags & HDMI_OUTPUT_PUSH_PULL)
+ hdmi_bit_set(hdmi, 0x02, 0x02, HDMI_SYSTEM_CTRL);
+
+ if (pdata->flags & HDMI_OUTPUT_POLARITY_HI)
+ hdmi_bit_set(hdmi, 0x01, 0x01, HDMI_SYSTEM_CTRL);
+
+ /* Re-init htop1 */
+ if (hdmi->htop1)
+ sh_hdmi_htop1_init(hdmi);
+
+ /* Now it's safe to enable interrupts again */
+ enable_irq(hdmi->irq);
+ return 0;
+}
+
+static const struct dev_pm_ops sh_hdmi_pm_ops = {
+ .suspend = sh_hdmi_suspend,
+ .resume = sh_hdmi_resume,
+};
+
static struct platform_driver sh_hdmi_driver = {
.remove = __exit_p(sh_hdmi_remove),
.driver = {
.name = "sh-mobile-hdmi",
+ .pm = &sh_hdmi_pm_ops,
},
};
--
1.9.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH/RFC] fbdev: sh_mobile_hdmi: Re-init regs before irq re-enable on resume
2014-09-23 12:21 ` Geert Uytterhoeven
@ 2014-09-23 17:26 ` Ulf Hansson
-1 siblings, 0 replies; 18+ messages in thread
From: Ulf Hansson @ 2014-09-23 17:26 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Jean-Christophe Plagniol-Villard, Tomi Valkeinen,
Rafael J. Wysocki, Guennadi Liakhovetski, linux-fbdev, linux-pm,
Linux-sh list
On 23 September 2014 14:21, Geert Uytterhoeven <geert+renesas@glider.be> wrote:
> When the PM domain containing the HDMI hardware block is powered down,
> the HDMI register values (incl. interrupt polarity settings) are lost.
> During resume, after powering up the PM domain, interrupts are
> re-enabled, and an interrupt storm happens due to incorrect interrupt
> polarity settings:
>
> irq 163: nobody cared (try booting with the "irqpoll" option)
> ...
> Disabling IRQ #163
>
> To fix this, re-initialize the interrupt polarity settings, and the
> htop1 register block (if present), during resume.
>
> As the .suspend_noirq() and .resume_noirq() callbacks are not called
> when using the generic PM domain, the normal .resume() callback is used,
> and the device interrupt needs to be disabled/enabled manually.
>
> This fixes resume from s2ram with power down of the A4MP PM domain on
> r8a7740/Armadillo.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> Is there a specific reason why the .suspend_noirq() and .resume_noirq()
> callbacks are not called when using genpd, unlike .suspend(),
> .suspend_late(), .resume_early(), and .resume()?
Hi Geert,
Interesting issue you are trying to fix here.
In principle I consider the *noirq() callbacks in genpd as workarounds
to handle the corner cases we had when using runtime PM and system PM
together. This is at least how the OMAP PM domain uses them.
Today, there are a better option which is to use
pm_runtime_force_suspend|resume() and to declare the runtime PM
callbacks within CONFIG_PM instead of CONFIG_PM_RUNTIME. I am actually
working on a patchset that tries this approach, do you think that
could solve your issue?
That said, I also think it's a bug in genpd to not invoke
pm_generic_suspend|resume_noirq() from its corresponding callbacks. I
think we should add them, but let's see if its trivial to do that. :-)
Kind regards
Uffe
> ---
> drivers/video/fbdev/sh_mobile_hdmi.c | 44 ++++++++++++++++++++++++++++++++++--
> 1 file changed, 42 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/video/fbdev/sh_mobile_hdmi.c b/drivers/video/fbdev/sh_mobile_hdmi.c
> index 9a33ee0413fb584d..7c72a3f02056445d 100644
> --- a/drivers/video/fbdev/sh_mobile_hdmi.c
> +++ b/drivers/video/fbdev/sh_mobile_hdmi.c
> @@ -281,6 +281,7 @@ struct sh_hdmi {
> u8 edid_block_addr;
> u8 edid_segment_nr;
> u8 edid_blocks;
> + int irq;
> struct clk *hdmi_clk;
> struct device *dev;
> struct delayed_work edid_work;
> @@ -1299,6 +1300,7 @@ static int __init sh_hdmi_probe(struct platform_device *pdev)
> hdmi->dev = &pdev->dev;
> hdmi->entity.owner = THIS_MODULE;
> hdmi->entity.ops = &sh_hdmi_ops;
> + hdmi->irq = irq;
>
> hdmi->hdmi_clk = clk_get(&pdev->dev, "ick");
> if (IS_ERR(hdmi->hdmi_clk)) {
> @@ -1415,12 +1417,11 @@ static int __exit sh_hdmi_remove(struct platform_device *pdev)
> {
> struct sh_hdmi *hdmi = entity_to_sh_hdmi(platform_get_drvdata(pdev));
> struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> - int irq = platform_get_irq(pdev, 0);
>
> snd_soc_unregister_codec(&pdev->dev);
>
> /* No new work will be scheduled, wait for running ISR */
> - free_irq(irq, hdmi);
> + free_irq(hdmi->irq, hdmi);
> /* Wait for already scheduled work */
> cancel_delayed_work_sync(&hdmi->edid_work);
> pm_runtime_put(&pdev->dev);
> @@ -1435,10 +1436,49 @@ static int __exit sh_hdmi_remove(struct platform_device *pdev)
> return 0;
> }
>
> +static int sh_hdmi_suspend(struct device *dev)
> +{
> + struct platform_device *pdev = to_platform_device(dev);
> + struct sh_hdmi *hdmi = entity_to_sh_hdmi(platform_get_drvdata(pdev));
> +
> + disable_irq(hdmi->irq);
> + /* Wait for already scheduled work */
> + cancel_delayed_work_sync(&hdmi->edid_work);
> + return 0;
> +}
> +
> +static int sh_hdmi_resume(struct device *dev)
> +{
> + struct platform_device *pdev = to_platform_device(dev);
> + struct sh_mobile_hdmi_info *pdata = dev_get_platdata(dev);
> + struct sh_hdmi *hdmi = entity_to_sh_hdmi(platform_get_drvdata(pdev));
> +
> + /* Re-init interrupt polarity */
> + if (pdata->flags & HDMI_OUTPUT_PUSH_PULL)
> + hdmi_bit_set(hdmi, 0x02, 0x02, HDMI_SYSTEM_CTRL);
> +
> + if (pdata->flags & HDMI_OUTPUT_POLARITY_HI)
> + hdmi_bit_set(hdmi, 0x01, 0x01, HDMI_SYSTEM_CTRL);
> +
> + /* Re-init htop1 */
> + if (hdmi->htop1)
> + sh_hdmi_htop1_init(hdmi);
> +
> + /* Now it's safe to enable interrupts again */
> + enable_irq(hdmi->irq);
> + return 0;
> +}
> +
> +static const struct dev_pm_ops sh_hdmi_pm_ops = {
> + .suspend = sh_hdmi_suspend,
> + .resume = sh_hdmi_resume,
> +};
> +
> static struct platform_driver sh_hdmi_driver = {
> .remove = __exit_p(sh_hdmi_remove),
> .driver = {
> .name = "sh-mobile-hdmi",
> + .pm = &sh_hdmi_pm_ops,
> },
> };
>
> --
> 1.9.1
>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH/RFC] fbdev: sh_mobile_hdmi: Re-init regs before irq re-enable on resume
@ 2014-09-23 17:26 ` Ulf Hansson
0 siblings, 0 replies; 18+ messages in thread
From: Ulf Hansson @ 2014-09-23 17:26 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Jean-Christophe Plagniol-Villard, Tomi Valkeinen,
Rafael J. Wysocki, Guennadi Liakhovetski, linux-fbdev, linux-pm,
Linux-sh list
On 23 September 2014 14:21, Geert Uytterhoeven <geert+renesas@glider.be> wrote:
> When the PM domain containing the HDMI hardware block is powered down,
> the HDMI register values (incl. interrupt polarity settings) are lost.
> During resume, after powering up the PM domain, interrupts are
> re-enabled, and an interrupt storm happens due to incorrect interrupt
> polarity settings:
>
> irq 163: nobody cared (try booting with the "irqpoll" option)
> ...
> Disabling IRQ #163
>
> To fix this, re-initialize the interrupt polarity settings, and the
> htop1 register block (if present), during resume.
>
> As the .suspend_noirq() and .resume_noirq() callbacks are not called
> when using the generic PM domain, the normal .resume() callback is used,
> and the device interrupt needs to be disabled/enabled manually.
>
> This fixes resume from s2ram with power down of the A4MP PM domain on
> r8a7740/Armadillo.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> Is there a specific reason why the .suspend_noirq() and .resume_noirq()
> callbacks are not called when using genpd, unlike .suspend(),
> .suspend_late(), .resume_early(), and .resume()?
Hi Geert,
Interesting issue you are trying to fix here.
In principle I consider the *noirq() callbacks in genpd as workarounds
to handle the corner cases we had when using runtime PM and system PM
together. This is at least how the OMAP PM domain uses them.
Today, there are a better option which is to use
pm_runtime_force_suspend|resume() and to declare the runtime PM
callbacks within CONFIG_PM instead of CONFIG_PM_RUNTIME. I am actually
working on a patchset that tries this approach, do you think that
could solve your issue?
That said, I also think it's a bug in genpd to not invoke
pm_generic_suspend|resume_noirq() from its corresponding callbacks. I
think we should add them, but let's see if its trivial to do that. :-)
Kind regards
Uffe
> ---
> drivers/video/fbdev/sh_mobile_hdmi.c | 44 ++++++++++++++++++++++++++++++++++--
> 1 file changed, 42 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/video/fbdev/sh_mobile_hdmi.c b/drivers/video/fbdev/sh_mobile_hdmi.c
> index 9a33ee0413fb584d..7c72a3f02056445d 100644
> --- a/drivers/video/fbdev/sh_mobile_hdmi.c
> +++ b/drivers/video/fbdev/sh_mobile_hdmi.c
> @@ -281,6 +281,7 @@ struct sh_hdmi {
> u8 edid_block_addr;
> u8 edid_segment_nr;
> u8 edid_blocks;
> + int irq;
> struct clk *hdmi_clk;
> struct device *dev;
> struct delayed_work edid_work;
> @@ -1299,6 +1300,7 @@ static int __init sh_hdmi_probe(struct platform_device *pdev)
> hdmi->dev = &pdev->dev;
> hdmi->entity.owner = THIS_MODULE;
> hdmi->entity.ops = &sh_hdmi_ops;
> + hdmi->irq = irq;
>
> hdmi->hdmi_clk = clk_get(&pdev->dev, "ick");
> if (IS_ERR(hdmi->hdmi_clk)) {
> @@ -1415,12 +1417,11 @@ static int __exit sh_hdmi_remove(struct platform_device *pdev)
> {
> struct sh_hdmi *hdmi = entity_to_sh_hdmi(platform_get_drvdata(pdev));
> struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> - int irq = platform_get_irq(pdev, 0);
>
> snd_soc_unregister_codec(&pdev->dev);
>
> /* No new work will be scheduled, wait for running ISR */
> - free_irq(irq, hdmi);
> + free_irq(hdmi->irq, hdmi);
> /* Wait for already scheduled work */
> cancel_delayed_work_sync(&hdmi->edid_work);
> pm_runtime_put(&pdev->dev);
> @@ -1435,10 +1436,49 @@ static int __exit sh_hdmi_remove(struct platform_device *pdev)
> return 0;
> }
>
> +static int sh_hdmi_suspend(struct device *dev)
> +{
> + struct platform_device *pdev = to_platform_device(dev);
> + struct sh_hdmi *hdmi = entity_to_sh_hdmi(platform_get_drvdata(pdev));
> +
> + disable_irq(hdmi->irq);
> + /* Wait for already scheduled work */
> + cancel_delayed_work_sync(&hdmi->edid_work);
> + return 0;
> +}
> +
> +static int sh_hdmi_resume(struct device *dev)
> +{
> + struct platform_device *pdev = to_platform_device(dev);
> + struct sh_mobile_hdmi_info *pdata = dev_get_platdata(dev);
> + struct sh_hdmi *hdmi = entity_to_sh_hdmi(platform_get_drvdata(pdev));
> +
> + /* Re-init interrupt polarity */
> + if (pdata->flags & HDMI_OUTPUT_PUSH_PULL)
> + hdmi_bit_set(hdmi, 0x02, 0x02, HDMI_SYSTEM_CTRL);
> +
> + if (pdata->flags & HDMI_OUTPUT_POLARITY_HI)
> + hdmi_bit_set(hdmi, 0x01, 0x01, HDMI_SYSTEM_CTRL);
> +
> + /* Re-init htop1 */
> + if (hdmi->htop1)
> + sh_hdmi_htop1_init(hdmi);
> +
> + /* Now it's safe to enable interrupts again */
> + enable_irq(hdmi->irq);
> + return 0;
> +}
> +
> +static const struct dev_pm_ops sh_hdmi_pm_ops = {
> + .suspend = sh_hdmi_suspend,
> + .resume = sh_hdmi_resume,
> +};
> +
> static struct platform_driver sh_hdmi_driver = {
> .remove = __exit_p(sh_hdmi_remove),
> .driver = {
> .name = "sh-mobile-hdmi",
> + .pm = &sh_hdmi_pm_ops,
> },
> };
>
> --
> 1.9.1
>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH/RFC] fbdev: sh_mobile_hdmi: Re-init regs before irq re-enable on resume
2014-09-23 17:26 ` Ulf Hansson
@ 2014-09-24 8:32 ` Geert Uytterhoeven
-1 siblings, 0 replies; 18+ messages in thread
From: Geert Uytterhoeven @ 2014-09-24 8:32 UTC (permalink / raw)
To: Ulf Hansson
Cc: Geert Uytterhoeven, Jean-Christophe Plagniol-Villard,
Tomi Valkeinen, Rafael J. Wysocki, Guennadi Liakhovetski,
Linux Fbdev development list, linux-pm, Linux-sh list
Hi Ulf,
On Tue, Sep 23, 2014 at 7:26 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 23 September 2014 14:21, Geert Uytterhoeven <geert+renesas@glider.be> wrote:
>> When the PM domain containing the HDMI hardware block is powered down,
>> the HDMI register values (incl. interrupt polarity settings) are lost.
>> During resume, after powering up the PM domain, interrupts are
>> re-enabled, and an interrupt storm happens due to incorrect interrupt
>> polarity settings:
>>
>> irq 163: nobody cared (try booting with the "irqpoll" option)
>> ...
>> Disabling IRQ #163
>>
>> To fix this, re-initialize the interrupt polarity settings, and the
>> htop1 register block (if present), during resume.
>>
>> As the .suspend_noirq() and .resume_noirq() callbacks are not called
>> when using the generic PM domain, the normal .resume() callback is used,
>> and the device interrupt needs to be disabled/enabled manually.
>>
>> This fixes resume from s2ram with power down of the A4MP PM domain on
>> r8a7740/Armadillo.
>>
>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>> ---
>> Is there a specific reason why the .suspend_noirq() and .resume_noirq()
>> callbacks are not called when using genpd, unlike .suspend(),
>> .suspend_late(), .resume_early(), and .resume()?
>
> Hi Geert,
>
> Interesting issue you are trying to fix here.
>
> In principle I consider the *noirq() callbacks in genpd as workarounds
> to handle the corner cases we had when using runtime PM and system PM
> together. This is at least how the OMAP PM domain uses them.
>
> Today, there are a better option which is to use
> pm_runtime_force_suspend|resume() and to declare the runtime PM
> callbacks within CONFIG_PM instead of CONFIG_PM_RUNTIME. I am actually
> working on a patchset that tries this approach, do you think that
> could solve your issue?
I don't follow 100% what you're telling me, but I don't think this would help:
the HDMI interrupt is used for HDMI hotplug detection, so it should stay
enabled even when the HDMI device is runtime suspended.
Only when the system is suspended, and the PM domain will be powered
down, the interrupt can be disabled.
After powering up the PM domain, but before the interrupt is enabled,
the registers must be restored.
> That said, I also think it's a bug in genpd to not invoke
> pm_generic_suspend|resume_noirq() from its corresponding callbacks. I
> think we should add them, but let's see if its trivial to do that. :-)
pm_genpd_suspend() and pm_genpd_suspend_late() call the
pm_generic_*() version if no genpd->suspend_power_off callback is
set up.
pm_genpd_suspend_noirq() does some really different things (i.e.
powering down the PM domain), so IMHO this isn't trivial to change.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@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
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH/RFC] fbdev: sh_mobile_hdmi: Re-init regs before irq re-enable on resume
@ 2014-09-24 8:32 ` Geert Uytterhoeven
0 siblings, 0 replies; 18+ messages in thread
From: Geert Uytterhoeven @ 2014-09-24 8:32 UTC (permalink / raw)
To: Ulf Hansson
Cc: Geert Uytterhoeven, Jean-Christophe Plagniol-Villard,
Tomi Valkeinen, Rafael J. Wysocki, Guennadi Liakhovetski,
Linux Fbdev development list, linux-pm, Linux-sh list
Hi Ulf,
On Tue, Sep 23, 2014 at 7:26 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 23 September 2014 14:21, Geert Uytterhoeven <geert+renesas@glider.be> wrote:
>> When the PM domain containing the HDMI hardware block is powered down,
>> the HDMI register values (incl. interrupt polarity settings) are lost.
>> During resume, after powering up the PM domain, interrupts are
>> re-enabled, and an interrupt storm happens due to incorrect interrupt
>> polarity settings:
>>
>> irq 163: nobody cared (try booting with the "irqpoll" option)
>> ...
>> Disabling IRQ #163
>>
>> To fix this, re-initialize the interrupt polarity settings, and the
>> htop1 register block (if present), during resume.
>>
>> As the .suspend_noirq() and .resume_noirq() callbacks are not called
>> when using the generic PM domain, the normal .resume() callback is used,
>> and the device interrupt needs to be disabled/enabled manually.
>>
>> This fixes resume from s2ram with power down of the A4MP PM domain on
>> r8a7740/Armadillo.
>>
>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>> ---
>> Is there a specific reason why the .suspend_noirq() and .resume_noirq()
>> callbacks are not called when using genpd, unlike .suspend(),
>> .suspend_late(), .resume_early(), and .resume()?
>
> Hi Geert,
>
> Interesting issue you are trying to fix here.
>
> In principle I consider the *noirq() callbacks in genpd as workarounds
> to handle the corner cases we had when using runtime PM and system PM
> together. This is at least how the OMAP PM domain uses them.
>
> Today, there are a better option which is to use
> pm_runtime_force_suspend|resume() and to declare the runtime PM
> callbacks within CONFIG_PM instead of CONFIG_PM_RUNTIME. I am actually
> working on a patchset that tries this approach, do you think that
> could solve your issue?
I don't follow 100% what you're telling me, but I don't think this would help:
the HDMI interrupt is used for HDMI hotplug detection, so it should stay
enabled even when the HDMI device is runtime suspended.
Only when the system is suspended, and the PM domain will be powered
down, the interrupt can be disabled.
After powering up the PM domain, but before the interrupt is enabled,
the registers must be restored.
> That said, I also think it's a bug in genpd to not invoke
> pm_generic_suspend|resume_noirq() from its corresponding callbacks. I
> think we should add them, but let's see if its trivial to do that. :-)
pm_genpd_suspend() and pm_genpd_suspend_late() call the
pm_generic_*() version if no genpd->suspend_power_off callback is
set up.
pm_genpd_suspend_noirq() does some really different things (i.e.
powering down the PM domain), so IMHO this isn't trivial to change.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@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
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH/RFC] fbdev: sh_mobile_hdmi: Re-init regs before irq re-enable on resume
2014-09-24 8:32 ` Geert Uytterhoeven
@ 2014-09-24 13:07 ` Ulf Hansson
-1 siblings, 0 replies; 18+ messages in thread
From: Ulf Hansson @ 2014-09-24 13:07 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Geert Uytterhoeven, Jean-Christophe Plagniol-Villard,
Tomi Valkeinen, Rafael J. Wysocki, Guennadi Liakhovetski,
Linux Fbdev development list, linux-pm, Linux-sh list
On 24 September 2014 10:32, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> Hi Ulf,
>
> On Tue, Sep 23, 2014 at 7:26 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> On 23 September 2014 14:21, Geert Uytterhoeven <geert+renesas@glider.be> wrote:
>>> When the PM domain containing the HDMI hardware block is powered down,
>>> the HDMI register values (incl. interrupt polarity settings) are lost.
>>> During resume, after powering up the PM domain, interrupts are
>>> re-enabled, and an interrupt storm happens due to incorrect interrupt
>>> polarity settings:
>>>
>>> irq 163: nobody cared (try booting with the "irqpoll" option)
>>> ...
>>> Disabling IRQ #163
>>>
>>> To fix this, re-initialize the interrupt polarity settings, and the
>>> htop1 register block (if present), during resume.
>>>
>>> As the .suspend_noirq() and .resume_noirq() callbacks are not called
>>> when using the generic PM domain, the normal .resume() callback is used,
>>> and the device interrupt needs to be disabled/enabled manually.
>>>
>>> This fixes resume from s2ram with power down of the A4MP PM domain on
>>> r8a7740/Armadillo.
>>>
>>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>>> ---
>>> Is there a specific reason why the .suspend_noirq() and .resume_noirq()
>>> callbacks are not called when using genpd, unlike .suspend(),
>>> .suspend_late(), .resume_early(), and .resume()?
>>
>> Hi Geert,
>>
>> Interesting issue you are trying to fix here.
>>
>> In principle I consider the *noirq() callbacks in genpd as workarounds
>> to handle the corner cases we had when using runtime PM and system PM
>> together. This is at least how the OMAP PM domain uses them.
>>
>> Today, there are a better option which is to use
>> pm_runtime_force_suspend|resume() and to declare the runtime PM
>> callbacks within CONFIG_PM instead of CONFIG_PM_RUNTIME. I am actually
>> working on a patchset that tries this approach, do you think that
>> could solve your issue?
>
> I don't follow 100% what you're telling me, but I don't think this would help:
> the HDMI interrupt is used for HDMI hotplug detection, so it should stay
> enabled even when the HDMI device is runtime suspended.
> Only when the system is suspended, and the PM domain will be powered
> down, the interrupt can be disabled.
>
> After powering up the PM domain, but before the interrupt is enabled,
> the registers must be restored.
I checked the details about the runtime PM support in the driver. To
me, it seems there are some additional pieces missing.
For a short while, let's just ignore the behaviour of the generic PM
domain. Then I would suggest to add the following below to the driver.
Please tell me if you think I am wrong, I don't no much about ALSA and HDMI. :-)
1) Add a runtime PM suspend callback to:
- Save register context.
- Enable wakeup IRQ if "wakeup IRQ capabilities" is set.
2) Add a runtime PM resume callback to:
- Restore register context.
- Disable wakeup IRQ.
3) Add a system PM suspend callback to:
- Disable "wakeup IRQ capabilities".
- Put the device into low power state.
Could likely be done by:
pm_runtime_get_sync();
"disable wakeup irq cap";
pm_runtime_put();
pm_runtime_force_suspend();
4) Add a system PM resume callback to:
- Enable "wakeup IRQ capabilities".
- Restore power to the device.
Could likely be done by:
"enable wakeup irq_cap";
pm_runtime_force_resume();
5) Currently the ->probe() method invokes pm_runtime_get_sync(),
causing the runtime PM resume callbacks to be invoked in this path.
Unless I am missing a point, this will cause the device stay runtime
PM resumed all the time. Is that really what you want?
In this context, and what I am trying to understand, is what changes
are needed to the generic PM domain, such it can be used under these
circumstances.
Kind regards
Uffe
>
>> That said, I also think it's a bug in genpd to not invoke
>> pm_generic_suspend|resume_noirq() from its corresponding callbacks. I
>> think we should add them, but let's see if its trivial to do that. :-)
>
> pm_genpd_suspend() and pm_genpd_suspend_late() call the
> pm_generic_*() version if no genpd->suspend_power_off callback is
> set up.
>
> pm_genpd_suspend_noirq() does some really different things (i.e.
> powering down the PM domain), so IMHO this isn't trivial to change.
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@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
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH/RFC] fbdev: sh_mobile_hdmi: Re-init regs before irq re-enable on resume
@ 2014-09-24 13:07 ` Ulf Hansson
0 siblings, 0 replies; 18+ messages in thread
From: Ulf Hansson @ 2014-09-24 13:07 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Geert Uytterhoeven, Jean-Christophe Plagniol-Villard,
Tomi Valkeinen, Rafael J. Wysocki, Guennadi Liakhovetski,
Linux Fbdev development list, linux-pm, Linux-sh list
On 24 September 2014 10:32, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> Hi Ulf,
>
> On Tue, Sep 23, 2014 at 7:26 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> On 23 September 2014 14:21, Geert Uytterhoeven <geert+renesas@glider.be> wrote:
>>> When the PM domain containing the HDMI hardware block is powered down,
>>> the HDMI register values (incl. interrupt polarity settings) are lost.
>>> During resume, after powering up the PM domain, interrupts are
>>> re-enabled, and an interrupt storm happens due to incorrect interrupt
>>> polarity settings:
>>>
>>> irq 163: nobody cared (try booting with the "irqpoll" option)
>>> ...
>>> Disabling IRQ #163
>>>
>>> To fix this, re-initialize the interrupt polarity settings, and the
>>> htop1 register block (if present), during resume.
>>>
>>> As the .suspend_noirq() and .resume_noirq() callbacks are not called
>>> when using the generic PM domain, the normal .resume() callback is used,
>>> and the device interrupt needs to be disabled/enabled manually.
>>>
>>> This fixes resume from s2ram with power down of the A4MP PM domain on
>>> r8a7740/Armadillo.
>>>
>>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>>> ---
>>> Is there a specific reason why the .suspend_noirq() and .resume_noirq()
>>> callbacks are not called when using genpd, unlike .suspend(),
>>> .suspend_late(), .resume_early(), and .resume()?
>>
>> Hi Geert,
>>
>> Interesting issue you are trying to fix here.
>>
>> In principle I consider the *noirq() callbacks in genpd as workarounds
>> to handle the corner cases we had when using runtime PM and system PM
>> together. This is at least how the OMAP PM domain uses them.
>>
>> Today, there are a better option which is to use
>> pm_runtime_force_suspend|resume() and to declare the runtime PM
>> callbacks within CONFIG_PM instead of CONFIG_PM_RUNTIME. I am actually
>> working on a patchset that tries this approach, do you think that
>> could solve your issue?
>
> I don't follow 100% what you're telling me, but I don't think this would help:
> the HDMI interrupt is used for HDMI hotplug detection, so it should stay
> enabled even when the HDMI device is runtime suspended.
> Only when the system is suspended, and the PM domain will be powered
> down, the interrupt can be disabled.
>
> After powering up the PM domain, but before the interrupt is enabled,
> the registers must be restored.
I checked the details about the runtime PM support in the driver. To
me, it seems there are some additional pieces missing.
For a short while, let's just ignore the behaviour of the generic PM
domain. Then I would suggest to add the following below to the driver.
Please tell me if you think I am wrong, I don't no much about ALSA and HDMI. :-)
1) Add a runtime PM suspend callback to:
- Save register context.
- Enable wakeup IRQ if "wakeup IRQ capabilities" is set.
2) Add a runtime PM resume callback to:
- Restore register context.
- Disable wakeup IRQ.
3) Add a system PM suspend callback to:
- Disable "wakeup IRQ capabilities".
- Put the device into low power state.
Could likely be done by:
pm_runtime_get_sync();
"disable wakeup irq cap";
pm_runtime_put();
pm_runtime_force_suspend();
4) Add a system PM resume callback to:
- Enable "wakeup IRQ capabilities".
- Restore power to the device.
Could likely be done by:
"enable wakeup irq_cap";
pm_runtime_force_resume();
5) Currently the ->probe() method invokes pm_runtime_get_sync(),
causing the runtime PM resume callbacks to be invoked in this path.
Unless I am missing a point, this will cause the device stay runtime
PM resumed all the time. Is that really what you want?
In this context, and what I am trying to understand, is what changes
are needed to the generic PM domain, such it can be used under these
circumstances.
Kind regards
Uffe
>
>> That said, I also think it's a bug in genpd to not invoke
>> pm_generic_suspend|resume_noirq() from its corresponding callbacks. I
>> think we should add them, but let's see if its trivial to do that. :-)
>
> pm_genpd_suspend() and pm_genpd_suspend_late() call the
> pm_generic_*() version if no genpd->suspend_power_off callback is
> set up.
>
> pm_genpd_suspend_noirq() does some really different things (i.e.
> powering down the PM domain), so IMHO this isn't trivial to change.
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@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
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH/RFC] fbdev: sh_mobile_hdmi: Re-init regs before irq re-enable on resume
2014-09-24 13:07 ` Ulf Hansson
@ 2014-09-24 13:42 ` Geert Uytterhoeven
-1 siblings, 0 replies; 18+ messages in thread
From: Geert Uytterhoeven @ 2014-09-24 13:42 UTC (permalink / raw)
To: Ulf Hansson
Cc: Geert Uytterhoeven, Jean-Christophe Plagniol-Villard,
Tomi Valkeinen, Rafael J. Wysocki, Guennadi Liakhovetski,
Linux Fbdev development list, linux-pm, Linux-sh list
Hi Ulf,
On Wed, Sep 24, 2014 at 3:07 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 24 September 2014 10:32, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>> On Tue, Sep 23, 2014 at 7:26 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>> On 23 September 2014 14:21, Geert Uytterhoeven <geert+renesas@glider.be> wrote:
>>>> When the PM domain containing the HDMI hardware block is powered down,
>>>> the HDMI register values (incl. interrupt polarity settings) are lost.
>>>> During resume, after powering up the PM domain, interrupts are
>>>> re-enabled, and an interrupt storm happens due to incorrect interrupt
>>>> polarity settings:
>>>>
>>>> irq 163: nobody cared (try booting with the "irqpoll" option)
>>>> ...
>>>> Disabling IRQ #163
>>>>
>>>> To fix this, re-initialize the interrupt polarity settings, and the
>>>> htop1 register block (if present), during resume.
>>>>
>>>> As the .suspend_noirq() and .resume_noirq() callbacks are not called
>>>> when using the generic PM domain, the normal .resume() callback is used,
>>>> and the device interrupt needs to be disabled/enabled manually.
>>>>
>>>> This fixes resume from s2ram with power down of the A4MP PM domain on
>>>> r8a7740/Armadillo.
>>>>
>>>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>>>> ---
>>>> Is there a specific reason why the .suspend_noirq() and .resume_noirq()
>>>> callbacks are not called when using genpd, unlike .suspend(),
>>>> .suspend_late(), .resume_early(), and .resume()?
>>>
>>> Hi Geert,
>>>
>>> Interesting issue you are trying to fix here.
>>>
>>> In principle I consider the *noirq() callbacks in genpd as workarounds
>>> to handle the corner cases we had when using runtime PM and system PM
>>> together. This is at least how the OMAP PM domain uses them.
>>>
>>> Today, there are a better option which is to use
>>> pm_runtime_force_suspend|resume() and to declare the runtime PM
>>> callbacks within CONFIG_PM instead of CONFIG_PM_RUNTIME. I am actually
>>> working on a patchset that tries this approach, do you think that
>>> could solve your issue?
>>
>> I don't follow 100% what you're telling me, but I don't think this would help:
>> the HDMI interrupt is used for HDMI hotplug detection, so it should stay
>> enabled even when the HDMI device is runtime suspended.
>> Only when the system is suspended, and the PM domain will be powered
>> down, the interrupt can be disabled.
>>
>> After powering up the PM domain, but before the interrupt is enabled,
>> the registers must be restored.
>
> I checked the details about the runtime PM support in the driver. To
> me, it seems there are some additional pieces missing.
>
> For a short while, let's just ignore the behaviour of the generic PM
> domain. Then I would suggest to add the following below to the driver.
> Please tell me if you think I am wrong, I don't no much about ALSA and HDMI. :-)
>
> 1) Add a runtime PM suspend callback to:
> - Save register context.
> - Enable wakeup IRQ if "wakeup IRQ capabilities" is set.
>
> 2) Add a runtime PM resume callback to:
> - Restore register context.
> - Disable wakeup IRQ.
>
> 3) Add a system PM suspend callback to:
> - Disable "wakeup IRQ capabilities".
> - Put the device into low power state.
> Could likely be done by:
> pm_runtime_get_sync();
> "disable wakeup irq cap";
> pm_runtime_put();
> pm_runtime_force_suspend();
>
> 4) Add a system PM resume callback to:
> - Enable "wakeup IRQ capabilities".
> - Restore power to the device.
> Could likely be done by:
> "enable wakeup irq_cap";
> pm_runtime_force_resume();
Thanks, those changes all look OK to me.
Unfortunately I do not have documentation for the HDMI hardware block,
and I am not in the position to do proper regression testing (the driver
doesn't really work on the hardware I do have), so I don't plan to implement
these changes.
> 5) Currently the ->probe() method invokes pm_runtime_get_sync(),
> causing the runtime PM resume callbacks to be invoked in this path.
> Unless I am missing a point, this will cause the device stay runtime
> PM resumed all the time. Is that really what you want?
That's correct. Right now the device is never runtime-suspended.
Given my above reasoning, I think that's fine for now. I just wanted to fix
the most severe issue (a system lock-up).
> In this context, and what I am trying to understand, is what changes
> are needed to the generic PM domain, such it can be used under these
> circumstances.
It seems no changes are needed to the generic PM domain.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@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
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH/RFC] fbdev: sh_mobile_hdmi: Re-init regs before irq re-enable on resume
@ 2014-09-24 13:42 ` Geert Uytterhoeven
0 siblings, 0 replies; 18+ messages in thread
From: Geert Uytterhoeven @ 2014-09-24 13:42 UTC (permalink / raw)
To: Ulf Hansson
Cc: Geert Uytterhoeven, Jean-Christophe Plagniol-Villard,
Tomi Valkeinen, Rafael J. Wysocki, Guennadi Liakhovetski,
Linux Fbdev development list, linux-pm, Linux-sh list
Hi Ulf,
On Wed, Sep 24, 2014 at 3:07 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 24 September 2014 10:32, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>> On Tue, Sep 23, 2014 at 7:26 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>> On 23 September 2014 14:21, Geert Uytterhoeven <geert+renesas@glider.be> wrote:
>>>> When the PM domain containing the HDMI hardware block is powered down,
>>>> the HDMI register values (incl. interrupt polarity settings) are lost.
>>>> During resume, after powering up the PM domain, interrupts are
>>>> re-enabled, and an interrupt storm happens due to incorrect interrupt
>>>> polarity settings:
>>>>
>>>> irq 163: nobody cared (try booting with the "irqpoll" option)
>>>> ...
>>>> Disabling IRQ #163
>>>>
>>>> To fix this, re-initialize the interrupt polarity settings, and the
>>>> htop1 register block (if present), during resume.
>>>>
>>>> As the .suspend_noirq() and .resume_noirq() callbacks are not called
>>>> when using the generic PM domain, the normal .resume() callback is used,
>>>> and the device interrupt needs to be disabled/enabled manually.
>>>>
>>>> This fixes resume from s2ram with power down of the A4MP PM domain on
>>>> r8a7740/Armadillo.
>>>>
>>>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>>>> ---
>>>> Is there a specific reason why the .suspend_noirq() and .resume_noirq()
>>>> callbacks are not called when using genpd, unlike .suspend(),
>>>> .suspend_late(), .resume_early(), and .resume()?
>>>
>>> Hi Geert,
>>>
>>> Interesting issue you are trying to fix here.
>>>
>>> In principle I consider the *noirq() callbacks in genpd as workarounds
>>> to handle the corner cases we had when using runtime PM and system PM
>>> together. This is at least how the OMAP PM domain uses them.
>>>
>>> Today, there are a better option which is to use
>>> pm_runtime_force_suspend|resume() and to declare the runtime PM
>>> callbacks within CONFIG_PM instead of CONFIG_PM_RUNTIME. I am actually
>>> working on a patchset that tries this approach, do you think that
>>> could solve your issue?
>>
>> I don't follow 100% what you're telling me, but I don't think this would help:
>> the HDMI interrupt is used for HDMI hotplug detection, so it should stay
>> enabled even when the HDMI device is runtime suspended.
>> Only when the system is suspended, and the PM domain will be powered
>> down, the interrupt can be disabled.
>>
>> After powering up the PM domain, but before the interrupt is enabled,
>> the registers must be restored.
>
> I checked the details about the runtime PM support in the driver. To
> me, it seems there are some additional pieces missing.
>
> For a short while, let's just ignore the behaviour of the generic PM
> domain. Then I would suggest to add the following below to the driver.
> Please tell me if you think I am wrong, I don't no much about ALSA and HDMI. :-)
>
> 1) Add a runtime PM suspend callback to:
> - Save register context.
> - Enable wakeup IRQ if "wakeup IRQ capabilities" is set.
>
> 2) Add a runtime PM resume callback to:
> - Restore register context.
> - Disable wakeup IRQ.
>
> 3) Add a system PM suspend callback to:
> - Disable "wakeup IRQ capabilities".
> - Put the device into low power state.
> Could likely be done by:
> pm_runtime_get_sync();
> "disable wakeup irq cap";
> pm_runtime_put();
> pm_runtime_force_suspend();
>
> 4) Add a system PM resume callback to:
> - Enable "wakeup IRQ capabilities".
> - Restore power to the device.
> Could likely be done by:
> "enable wakeup irq_cap";
> pm_runtime_force_resume();
Thanks, those changes all look OK to me.
Unfortunately I do not have documentation for the HDMI hardware block,
and I am not in the position to do proper regression testing (the driver
doesn't really work on the hardware I do have), so I don't plan to implement
these changes.
> 5) Currently the ->probe() method invokes pm_runtime_get_sync(),
> causing the runtime PM resume callbacks to be invoked in this path.
> Unless I am missing a point, this will cause the device stay runtime
> PM resumed all the time. Is that really what you want?
That's correct. Right now the device is never runtime-suspended.
Given my above reasoning, I think that's fine for now. I just wanted to fix
the most severe issue (a system lock-up).
> In this context, and what I am trying to understand, is what changes
> are needed to the generic PM domain, such it can be used under these
> circumstances.
It seems no changes are needed to the generic PM domain.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH/RFC] fbdev: sh_mobile_hdmi: Re-init regs before irq re-enable on resume
2014-09-23 12:21 ` Geert Uytterhoeven
@ 2014-09-30 10:24 ` Tomi Valkeinen
-1 siblings, 0 replies; 18+ messages in thread
From: Tomi Valkeinen @ 2014-09-30 10:24 UTC (permalink / raw)
To: Geert Uytterhoeven, Jean-Christophe Plagniol-Villard,
Rafael J. Wysocki, Ulf Hansson
Cc: Guennadi Liakhovetski, linux-fbdev, linux-pm, linux-sh
[-- Attachment #1: Type: text/plain, Size: 1458 bytes --]
Hi Geert, Ulf,
On 23/09/14 15:21, Geert Uytterhoeven wrote:
> When the PM domain containing the HDMI hardware block is powered down,
> the HDMI register values (incl. interrupt polarity settings) are lost.
> During resume, after powering up the PM domain, interrupts are
> re-enabled, and an interrupt storm happens due to incorrect interrupt
> polarity settings:
>
> irq 163: nobody cared (try booting with the "irqpoll" option)
> ...
> Disabling IRQ #163
>
> To fix this, re-initialize the interrupt polarity settings, and the
> htop1 register block (if present), during resume.
>
> As the .suspend_noirq() and .resume_noirq() callbacks are not called
> when using the generic PM domain, the normal .resume() callback is used,
> and the device interrupt needs to be disabled/enabled manually.
>
> This fixes resume from s2ram with power down of the A4MP PM domain on
> r8a7740/Armadillo.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> Is there a specific reason why the .suspend_noirq() and .resume_noirq()
> callbacks are not called when using genpd, unlike .suspend(),
> .suspend_late(), .resume_early(), and .resume()?
> ---
> drivers/video/fbdev/sh_mobile_hdmi.c | 44 ++++++++++++++++++++++++++++++++++--
> 1 file changed, 42 insertions(+), 2 deletions(-)
Should I queue this version for 3.18, or is there some modifications you
think needs to be done first?
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH/RFC] fbdev: sh_mobile_hdmi: Re-init regs before irq re-enable on resume
@ 2014-09-30 10:24 ` Tomi Valkeinen
0 siblings, 0 replies; 18+ messages in thread
From: Tomi Valkeinen @ 2014-09-30 10:24 UTC (permalink / raw)
To: Geert Uytterhoeven, Jean-Christophe Plagniol-Villard,
Rafael J. Wysocki, Ulf Hansson
Cc: Guennadi Liakhovetski, linux-fbdev, linux-pm, linux-sh
[-- Attachment #1: Type: text/plain, Size: 1458 bytes --]
Hi Geert, Ulf,
On 23/09/14 15:21, Geert Uytterhoeven wrote:
> When the PM domain containing the HDMI hardware block is powered down,
> the HDMI register values (incl. interrupt polarity settings) are lost.
> During resume, after powering up the PM domain, interrupts are
> re-enabled, and an interrupt storm happens due to incorrect interrupt
> polarity settings:
>
> irq 163: nobody cared (try booting with the "irqpoll" option)
> ...
> Disabling IRQ #163
>
> To fix this, re-initialize the interrupt polarity settings, and the
> htop1 register block (if present), during resume.
>
> As the .suspend_noirq() and .resume_noirq() callbacks are not called
> when using the generic PM domain, the normal .resume() callback is used,
> and the device interrupt needs to be disabled/enabled manually.
>
> This fixes resume from s2ram with power down of the A4MP PM domain on
> r8a7740/Armadillo.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> Is there a specific reason why the .suspend_noirq() and .resume_noirq()
> callbacks are not called when using genpd, unlike .suspend(),
> .suspend_late(), .resume_early(), and .resume()?
> ---
> drivers/video/fbdev/sh_mobile_hdmi.c | 44 ++++++++++++++++++++++++++++++++++--
> 1 file changed, 42 insertions(+), 2 deletions(-)
Should I queue this version for 3.18, or is there some modifications you
think needs to be done first?
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH/RFC] fbdev: sh_mobile_hdmi: Re-init regs before irq re-enable on resume
2014-09-30 10:24 ` Tomi Valkeinen
@ 2014-09-30 10:41 ` Geert Uytterhoeven
-1 siblings, 0 replies; 18+ messages in thread
From: Geert Uytterhoeven @ 2014-09-30 10:41 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: Geert Uytterhoeven, Jean-Christophe Plagniol-Villard,
Rafael J. Wysocki, Ulf Hansson, Guennadi Liakhovetski,
Linux Fbdev development list, Linux PM list, Linux-sh list
Hi Tomi,
On Tue, Sep 30, 2014 at 12:24 PM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On 23/09/14 15:21, Geert Uytterhoeven wrote:
>> When the PM domain containing the HDMI hardware block is powered down,
>> the HDMI register values (incl. interrupt polarity settings) are lost.
>> During resume, after powering up the PM domain, interrupts are
>> re-enabled, and an interrupt storm happens due to incorrect interrupt
>> polarity settings:
>>
>> irq 163: nobody cared (try booting with the "irqpoll" option)
>> ...
>> Disabling IRQ #163
>>
>> To fix this, re-initialize the interrupt polarity settings, and the
>> htop1 register block (if present), during resume.
>>
>> As the .suspend_noirq() and .resume_noirq() callbacks are not called
>> when using the generic PM domain, the normal .resume() callback is used,
>> and the device interrupt needs to be disabled/enabled manually.
>>
>> This fixes resume from s2ram with power down of the A4MP PM domain on
>> r8a7740/Armadillo.
>>
>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>> ---
>> Is there a specific reason why the .suspend_noirq() and .resume_noirq()
>> callbacks are not called when using genpd, unlike .suspend(),
>> .suspend_late(), .resume_early(), and .resume()?
>> ---
>> drivers/video/fbdev/sh_mobile_hdmi.c | 44 ++++++++++++++++++++++++++++++++++--
>> 1 file changed, 42 insertions(+), 2 deletions(-)
>
> Should I queue this version for 3.18, or is there some modifications you
> think needs to be done first?
Given the lack of documentation, I cannot do more improvements.
Please apply this version, so the interrupt storm on resume is fixed.
Thanks for applying!
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@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
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH/RFC] fbdev: sh_mobile_hdmi: Re-init regs before irq re-enable on resume
@ 2014-09-30 10:41 ` Geert Uytterhoeven
0 siblings, 0 replies; 18+ messages in thread
From: Geert Uytterhoeven @ 2014-09-30 10:41 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: Geert Uytterhoeven, Jean-Christophe Plagniol-Villard,
Rafael J. Wysocki, Ulf Hansson, Guennadi Liakhovetski,
Linux Fbdev development list, Linux PM list, Linux-sh list
Hi Tomi,
On Tue, Sep 30, 2014 at 12:24 PM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On 23/09/14 15:21, Geert Uytterhoeven wrote:
>> When the PM domain containing the HDMI hardware block is powered down,
>> the HDMI register values (incl. interrupt polarity settings) are lost.
>> During resume, after powering up the PM domain, interrupts are
>> re-enabled, and an interrupt storm happens due to incorrect interrupt
>> polarity settings:
>>
>> irq 163: nobody cared (try booting with the "irqpoll" option)
>> ...
>> Disabling IRQ #163
>>
>> To fix this, re-initialize the interrupt polarity settings, and the
>> htop1 register block (if present), during resume.
>>
>> As the .suspend_noirq() and .resume_noirq() callbacks are not called
>> when using the generic PM domain, the normal .resume() callback is used,
>> and the device interrupt needs to be disabled/enabled manually.
>>
>> This fixes resume from s2ram with power down of the A4MP PM domain on
>> r8a7740/Armadillo.
>>
>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>> ---
>> Is there a specific reason why the .suspend_noirq() and .resume_noirq()
>> callbacks are not called when using genpd, unlike .suspend(),
>> .suspend_late(), .resume_early(), and .resume()?
>> ---
>> drivers/video/fbdev/sh_mobile_hdmi.c | 44 ++++++++++++++++++++++++++++++++++--
>> 1 file changed, 42 insertions(+), 2 deletions(-)
>
> Should I queue this version for 3.18, or is there some modifications you
> think needs to be done first?
Given the lack of documentation, I cannot do more improvements.
Please apply this version, so the interrupt storm on resume is fixed.
Thanks for applying!
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@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
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH/RFC] fbdev: sh_mobile_hdmi: Re-init regs before irq re-enable on resume
2014-09-30 10:41 ` Geert Uytterhoeven
@ 2014-09-30 10:42 ` Tomi Valkeinen
-1 siblings, 0 replies; 18+ messages in thread
From: Tomi Valkeinen @ 2014-09-30 10:42 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Geert Uytterhoeven, Jean-Christophe Plagniol-Villard,
Rafael J. Wysocki, Ulf Hansson, Guennadi Liakhovetski,
Linux Fbdev development list, Linux PM list, Linux-sh list
[-- Attachment #1: Type: text/plain, Size: 1818 bytes --]
On 30/09/14 13:41, Geert Uytterhoeven wrote:
> Hi Tomi,
>
> On Tue, Sep 30, 2014 at 12:24 PM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>> On 23/09/14 15:21, Geert Uytterhoeven wrote:
>>> When the PM domain containing the HDMI hardware block is powered down,
>>> the HDMI register values (incl. interrupt polarity settings) are lost.
>>> During resume, after powering up the PM domain, interrupts are
>>> re-enabled, and an interrupt storm happens due to incorrect interrupt
>>> polarity settings:
>>>
>>> irq 163: nobody cared (try booting with the "irqpoll" option)
>>> ...
>>> Disabling IRQ #163
>>>
>>> To fix this, re-initialize the interrupt polarity settings, and the
>>> htop1 register block (if present), during resume.
>>>
>>> As the .suspend_noirq() and .resume_noirq() callbacks are not called
>>> when using the generic PM domain, the normal .resume() callback is used,
>>> and the device interrupt needs to be disabled/enabled manually.
>>>
>>> This fixes resume from s2ram with power down of the A4MP PM domain on
>>> r8a7740/Armadillo.
>>>
>>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>>> ---
>>> Is there a specific reason why the .suspend_noirq() and .resume_noirq()
>>> callbacks are not called when using genpd, unlike .suspend(),
>>> .suspend_late(), .resume_early(), and .resume()?
>>> ---
>>> drivers/video/fbdev/sh_mobile_hdmi.c | 44 ++++++++++++++++++++++++++++++++++--
>>> 1 file changed, 42 insertions(+), 2 deletions(-)
>>
>> Should I queue this version for 3.18, or is there some modifications you
>> think needs to be done first?
>
> Given the lack of documentation, I cannot do more improvements.
>
> Please apply this version, so the interrupt storm on resume is fixed.
Ok, queued. Thanks!
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH/RFC] fbdev: sh_mobile_hdmi: Re-init regs before irq re-enable on resume
@ 2014-09-30 10:42 ` Tomi Valkeinen
0 siblings, 0 replies; 18+ messages in thread
From: Tomi Valkeinen @ 2014-09-30 10:42 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Geert Uytterhoeven, Jean-Christophe Plagniol-Villard,
Rafael J. Wysocki, Ulf Hansson, Guennadi Liakhovetski,
Linux Fbdev development list, Linux PM list, Linux-sh list
[-- Attachment #1: Type: text/plain, Size: 1818 bytes --]
On 30/09/14 13:41, Geert Uytterhoeven wrote:
> Hi Tomi,
>
> On Tue, Sep 30, 2014 at 12:24 PM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>> On 23/09/14 15:21, Geert Uytterhoeven wrote:
>>> When the PM domain containing the HDMI hardware block is powered down,
>>> the HDMI register values (incl. interrupt polarity settings) are lost.
>>> During resume, after powering up the PM domain, interrupts are
>>> re-enabled, and an interrupt storm happens due to incorrect interrupt
>>> polarity settings:
>>>
>>> irq 163: nobody cared (try booting with the "irqpoll" option)
>>> ...
>>> Disabling IRQ #163
>>>
>>> To fix this, re-initialize the interrupt polarity settings, and the
>>> htop1 register block (if present), during resume.
>>>
>>> As the .suspend_noirq() and .resume_noirq() callbacks are not called
>>> when using the generic PM domain, the normal .resume() callback is used,
>>> and the device interrupt needs to be disabled/enabled manually.
>>>
>>> This fixes resume from s2ram with power down of the A4MP PM domain on
>>> r8a7740/Armadillo.
>>>
>>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>>> ---
>>> Is there a specific reason why the .suspend_noirq() and .resume_noirq()
>>> callbacks are not called when using genpd, unlike .suspend(),
>>> .suspend_late(), .resume_early(), and .resume()?
>>> ---
>>> drivers/video/fbdev/sh_mobile_hdmi.c | 44 ++++++++++++++++++++++++++++++++++--
>>> 1 file changed, 42 insertions(+), 2 deletions(-)
>>
>> Should I queue this version for 3.18, or is there some modifications you
>> think needs to be done first?
>
> Given the lack of documentation, I cannot do more improvements.
>
> Please apply this version, so the interrupt storm on resume is fixed.
Ok, queued. Thanks!
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH/RFC] fbdev: sh_mobile_hdmi: Re-init regs before irq re-enable on resume
2014-09-30 10:24 ` Tomi Valkeinen
@ 2014-09-30 10:43 ` Ulf Hansson
-1 siblings, 0 replies; 18+ messages in thread
From: Ulf Hansson @ 2014-09-30 10:43 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: Geert Uytterhoeven, Jean-Christophe Plagniol-Villard,
Rafael J. Wysocki, Guennadi Liakhovetski,
Linux Fbdev development list, linux-pm, Linux-sh list
On 30 September 2014 12:24, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> Hi Geert, Ulf,
>
> On 23/09/14 15:21, Geert Uytterhoeven wrote:
>> When the PM domain containing the HDMI hardware block is powered down,
>> the HDMI register values (incl. interrupt polarity settings) are lost.
>> During resume, after powering up the PM domain, interrupts are
>> re-enabled, and an interrupt storm happens due to incorrect interrupt
>> polarity settings:
>>
>> irq 163: nobody cared (try booting with the "irqpoll" option)
>> ...
>> Disabling IRQ #163
>>
>> To fix this, re-initialize the interrupt polarity settings, and the
>> htop1 register block (if present), during resume.
>>
>> As the .suspend_noirq() and .resume_noirq() callbacks are not called
>> when using the generic PM domain, the normal .resume() callback is used,
>> and the device interrupt needs to be disabled/enabled manually.
>>
>> This fixes resume from s2ram with power down of the A4MP PM domain on
>> r8a7740/Armadillo.
>>
>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>> ---
>> Is there a specific reason why the .suspend_noirq() and .resume_noirq()
>> callbacks are not called when using genpd, unlike .suspend(),
>> .suspend_late(), .resume_early(), and .resume()?
>> ---
>> drivers/video/fbdev/sh_mobile_hdmi.c | 44 ++++++++++++++++++++++++++++++++++--
>> 1 file changed, 42 insertions(+), 2 deletions(-)
>
> Should I queue this version for 3.18, or is there some modifications you
> think needs to be done first?
I have no objections for this patch. Please go ahead.
The reason for my comments was to try to figure out if there were any
impact to the generic PM domain.
Kind regards
Uffe
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH/RFC] fbdev: sh_mobile_hdmi: Re-init regs before irq re-enable on resume
@ 2014-09-30 10:43 ` Ulf Hansson
0 siblings, 0 replies; 18+ messages in thread
From: Ulf Hansson @ 2014-09-30 10:43 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: Geert Uytterhoeven, Jean-Christophe Plagniol-Villard,
Rafael J. Wysocki, Guennadi Liakhovetski,
Linux Fbdev development list, linux-pm, Linux-sh list
On 30 September 2014 12:24, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> Hi Geert, Ulf,
>
> On 23/09/14 15:21, Geert Uytterhoeven wrote:
>> When the PM domain containing the HDMI hardware block is powered down,
>> the HDMI register values (incl. interrupt polarity settings) are lost.
>> During resume, after powering up the PM domain, interrupts are
>> re-enabled, and an interrupt storm happens due to incorrect interrupt
>> polarity settings:
>>
>> irq 163: nobody cared (try booting with the "irqpoll" option)
>> ...
>> Disabling IRQ #163
>>
>> To fix this, re-initialize the interrupt polarity settings, and the
>> htop1 register block (if present), during resume.
>>
>> As the .suspend_noirq() and .resume_noirq() callbacks are not called
>> when using the generic PM domain, the normal .resume() callback is used,
>> and the device interrupt needs to be disabled/enabled manually.
>>
>> This fixes resume from s2ram with power down of the A4MP PM domain on
>> r8a7740/Armadillo.
>>
>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>> ---
>> Is there a specific reason why the .suspend_noirq() and .resume_noirq()
>> callbacks are not called when using genpd, unlike .suspend(),
>> .suspend_late(), .resume_early(), and .resume()?
>> ---
>> drivers/video/fbdev/sh_mobile_hdmi.c | 44 ++++++++++++++++++++++++++++++++++--
>> 1 file changed, 42 insertions(+), 2 deletions(-)
>
> Should I queue this version for 3.18, or is there some modifications you
> think needs to be done first?
I have no objections for this patch. Please go ahead.
The reason for my comments was to try to figure out if there were any
impact to the generic PM domain.
Kind regards
Uffe
^ permalink raw reply [flat|nested] 18+ messages in thread