* [PATCH/RFC] fbdev: sh_mobile_hdmi: Re-init regs before irq re-enable on resume
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
* Re: [PATCH/RFC] fbdev: sh_mobile_hdmi: Re-init regs before irq re-enable on resume
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@vger.kernel.org, Linux-sh list
In-Reply-To: <1411474918-2955-1-git-send-email-geert+renesas@glider.be>
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
* RE: [PATCH] fbdev: fsl-sii902x: HDMI support for Freescale SoCs
From: Li.Xiubo @ 2014-09-24 6:45 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org,
pawel.moll-5wv7dgnIgG8@public.gmane.org,
mark.rutland-5wv7dgnIgG8@public.gmane.org,
ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg@public.gmane.org,
galak-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org,
plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org,
grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org,
arnd-r2nGTMty4D4@public.gmane.org,
peter.griffin-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org,
jg1.han-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org,
daniel.vetter-/w4YWyX8dFk@public.gmane.org,
laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org,
robdclark-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <541C3371.8080100-l0cyMroinI0@public.gmane.org>
Hi,
[...]
> >>> 4 files changed, 551 insertions(+)
> >>> create mode 100644 Documentation/devicetree/bindings/video/fsl-
> sii902x.txt
> >>> create mode 100644 drivers/video/fbdev/fsl-sii902x.c
> >>
> >> I don't know how you picked the names of the people you sent this patch
> >> to, but looks to me that most of them are probably not interested in
> >> this patch.
> >>
> >
> > I just using the get_maintainer.pl script.
>
> Yes, and that's good, but you have to use your judgment also.
> get_maintainer.pl gives you names of people who have at some point
> touched the files or directories you are changing. Usually only the
> first names returned by get_maintainer.pl are relevant.
>
Okay :)
> >> Anyway, a few quick comments on the patch:
> >>
> >> - You should probably use regmap instead of direct i2c calls.
> >> Interestingly, you define regmap variable, but you never use it.
> >
> > Yes, this it's my another version in my local machine using regmap which
> > need much more test.
> >
> >> - Use defines for register offsets, instead of magic numbers.
> >
> > This will be fixed together with regmap using.
>
> Well, it's better to send the patch only when you have tested and
> cleaned up your driver.
>
> >> - You should not use static variables. They prevent having multiple
> >> instances of the device.
> >>
> >
> > Okay.
> >
> >> So the SiI902x chip is on the SoC, not on the board? And it's a plain
> >> standard SiI902x in other aspects?
> >>
> >
> > No, it's on board.
> >
> > And it will be used for i.MX and LS1+ platforms.
>
> Ok. In that case, I would suggest you to move to the DRM framework. The
> fbdev framework is not suited to adding external encoders. The end
> result with fbdev will be a SoC/board specific hack driver.
>
> That said, we already have such drivers for fbdev, so I'm not 100%
> against adding a new one. But I'm very very seriously recommending
> moving to DRM.
>
> And if this driver is added to fbdev, I think the device tree bindings
> should use the video ports/endpoints to describe the connections.
>
I will have a try to use the DRM framework.
Thanks,
BRs
Xiubo
> Tomi
>
^ permalink raw reply
* Re: [PATCH/RFC] fbdev: sh_mobile_hdmi: Re-init regs before irq re-enable on resume
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@vger.kernel.org,
Linux-sh list
In-Reply-To: <CAPDyKFrGiEJqHe2+SSNpUhBKfhGraqfomskUbemFmucszv8ukw@mail.gmail.com>
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
* Re: [PATCH/RFC] fbdev: sh_mobile_hdmi: Re-init regs before irq re-enable on resume
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@vger.kernel.org,
Linux-sh list
In-Reply-To: <CAMuHMdXoD6b-SNKKQpS6gzM_sDm4guepn1O1p_KK_7SyTC5CDQ@mail.gmail.com>
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
* Re: [PATCH/RFC] fbdev: sh_mobile_hdmi: Re-init regs before irq re-enable on resume
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@vger.kernel.org,
Linux-sh list
In-Reply-To: <CAPDyKFoGuKsDVnHfALRRtXu8kxXvUG-a-7308VRo=KQP7wAaRQ@mail.gmail.com>
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
* [PATCH 00/27] add pm_runtime_last_busy_and_autosuspend() helper
From: Vinod Koul @ 2014-09-24 16:26 UTC (permalink / raw)
To: linux-kernel
Cc: linux-wireless, Baruch Siach, patches, linux-doc, Seungwon Jeon,
Daniel Vetter, Chris Ball, dri-devel, Marcin Wojtas,
Rafael J. Wysocki, Laurent Pinchart, David Herrmann,
Shinya Kuribayashi, Laurent Pinchart, vinod.koul, Jingoo Han,
Jaehoon Chung, Jani Nikula, Tomi Valkeinen, Alan Stern,
Ben Skeggs, Bjorn Andersson, Wolfram Sang
This patch series adds a simple macro pm_runtime_last_busy_and_autosuspend()
which invokes pm_runtime_mark_last_busy() and pm_runtime_put_autosuspend()
sequentially. Then we do a tree wide update of current patterns which are
present. As evident from log below this pattern is frequent in the
kernel.
This series can be found at
git://git.kernel.org/pub/scm/linux/kernel/git/vkoul/slave-dma.git
topic/pm_runtime_last_busy_and_autosuspend
Fengguang's kbuild has tested it so it shouldn't break things for anyone.
Barring one patch (explictyly mentioned in its changelog) rest are simple
replacements.
If all are okay, this should be merged thru PM tree as it depends on macro
addition.
Subhransu S. Prusty (1):
PM: Add helper pm_runtime_last_busy_and_autosuspend()
Vinod Koul (26):
dmaengine: ste_dma: use pm_runtime_last_busy_and_autosuspend helper
extcon: arizona: use pm_runtime_last_busy_and_autosuspend helper
drm/i915: use pm_runtime_last_busy_and_autosuspend helper
drm/nouveau: use pm_runtime_last_busy_and_autosuspend helper
drm/radeon: use pm_runtime_last_busy_and_autosuspend helper
vga_switcheroo: use pm_runtime_last_busy_and_autosuspend helper
i2c: designware: use pm_runtime_last_busy_and_autosuspend helper
i2c: omap: use pm_runtime_last_busy_and_autosuspend helper
i2c: qup: use pm_runtime_last_busy_and_autosuspend helper
mfd: ab8500-gpadc: use pm_runtime_last_busy_and_autosuspend helper
mfd: arizona: use pm_runtime_last_busy_and_autosuspend helper
mei: use pm_runtime_last_busy_and_autosuspend helper
mmc: use pm_runtime_last_busy_and_autosuspend helper
mmc: mmci: use pm_runtime_last_busy_and_autosuspend helper
mmc: omap_hsmmc: use pm_runtime_last_busy_and_autosuspend helper
mmc: sdhci-pxav3: use pm_runtime_last_busy_and_autosuspend helper
mmc: sdhci: use pm_runtime_last_busy_and_autosuspend helper
NFC: trf7970a: use pm_runtime_last_busy_and_autosuspend helper
pm2301-charger: use pm_runtime_last_busy_and_autosuspend helper
spi: omap2-mcspi: use pm_runtime_last_busy_and_autosuspend helper
spi: orion: use pm_runtime_last_busy_and_autosuspend helper
spi: ti-qspi: use pm_runtime_last_busy_and_autosuspend helper
spi: core: use pm_runtime_last_busy_and_autosuspend helper
tty: serial: omap: use pm_runtime_last_busy_and_autosuspend helper
usb: musb: omap2430: use pm_runtime_last_busy_and_autosuspend helper
video: fbdev: use pm_runtime_last_busy_and_autosuspend helper
Documentation/power/runtime_pm.txt | 4 ++
drivers/dma/ste_dma40.c | 30 ++++---------
drivers/extcon/extcon-arizona.c | 6 +--
drivers/gpu/drm/i915/intel_pm.c | 3 +-
drivers/gpu/drm/nouveau/nouveau_connector.c | 3 +-
drivers/gpu/drm/nouveau/nouveau_drm.c | 9 +---
drivers/gpu/drm/radeon/radeon_connectors.c | 15 ++----
drivers/gpu/drm/radeon/radeon_drv.c | 5 +-
drivers/gpu/drm/radeon/radeon_kms.c | 6 +--
drivers/gpu/vga/vga_switcheroo.c | 7 +--
drivers/i2c/busses/i2c-designware-core.c | 3 +-
drivers/i2c/busses/i2c-omap.c | 6 +--
drivers/i2c/busses/i2c-qup.c | 3 +-
drivers/mfd/ab8500-gpadc.c | 6 +--
drivers/mfd/arizona-irq.c | 3 +-
drivers/misc/mei/client.c | 12 ++----
drivers/mmc/core/core.c | 3 +-
drivers/mmc/host/mmci.c | 12 ++----
drivers/mmc/host/omap_hsmmc.c | 19 ++-------
drivers/mmc/host/sdhci-pxav3.c | 6 +--
drivers/mmc/host/sdhci.c | 3 +-
drivers/nfc/trf7970a.c | 3 +-
drivers/power/pm2301_charger.c | 3 +-
drivers/spi/spi-omap2-mcspi.c | 9 +---
drivers/spi/spi-orion.c | 3 +-
drivers/spi/spi-ti-qspi.c | 5 +-
drivers/spi/spi.c | 6 +--
drivers/tty/serial/omap-serial.c | 60 +++++++++------------------
drivers/usb/musb/omap2430.c | 6 +--
drivers/video/fbdev/auo_k190x.c | 9 +---
include/linux/pm_runtime.h | 6 +++
31 files changed, 97 insertions(+), 177 deletions(-)
Thanks
--
~Vinod
^ permalink raw reply
* [PATCH 27/27] video: fbdev: use pm_runtime_last_busy_and_autosuspend helper
From: Vinod Koul @ 2014-09-24 16:27 UTC (permalink / raw)
To: linux-kernel
Cc: subhransu.s.prusty, vinod.koul, Jean-Christophe Plagniol-Villard,
Tomi Valkeinen, Jingoo Han, Laurent Pinchart, Rob Clark,
Daniel Vetter, linux-fbdev
In-Reply-To: <1411575342-31048-1-git-send-email-vinod.koul@intel.com>
Use the new pm_runtime_last_busy_and_autosuspend helper instead of open
coding the same code
Signed-off-by: Vinod Koul <vinod.koul@intel.com>
---
drivers/video/fbdev/auo_k190x.c | 9 +++------
1 files changed, 3 insertions(+), 6 deletions(-)
diff --git a/drivers/video/fbdev/auo_k190x.c b/drivers/video/fbdev/auo_k190x.c
index 8d2499d..e12f513 100644
--- a/drivers/video/fbdev/auo_k190x.c
+++ b/drivers/video/fbdev/auo_k190x.c
@@ -354,8 +354,7 @@ static void auok190xfb_dpy_deferred_io(struct fb_info *info,
par->update_partial(par, y1, min((u16) (y1 + h), yres));
out:
- pm_runtime_mark_last_busy(info->device);
- pm_runtime_put_autosuspend(info->device);
+ pm_runtime_last_busy_and_autosuspend(info->device);
}
/*
@@ -591,8 +590,7 @@ static int auok190x_read_temperature(struct auok190xfb_par *par)
mutex_unlock(&(par->io_lock));
- pm_runtime_mark_last_busy(dev);
- pm_runtime_put_autosuspend(dev);
+ pm_runtime_last_busy_and_autosuspend(dev);
/* sanitize and split of half-degrees for now */
temp = ((data[0] & AUOK190X_VERSION_TEMP_MASK) >> 1);
@@ -630,8 +628,7 @@ static void auok190x_identify(struct auok190xfb_par *par)
par->panel_size_int, par->panel_size_float, par->panel_model,
par->epd_type, par->tcon_version, par->lut_version);
- pm_runtime_mark_last_busy(dev);
- pm_runtime_put_autosuspend(dev);
+ pm_runtime_last_busy_and_autosuspend(dev);
}
/*
--
1.7.0.4
^ permalink raw reply related
* Re: [PATCH 00/27] add pm_runtime_last_busy_and_autosuspend() helper
From: Rafael J. Wysocki @ 2014-09-24 20:28 UTC (permalink / raw)
To: Vinod Koul
Cc: linux-kernel, subhransu.s.prusty, Alan Stern, Alexandre Courbot,
Andrew Morton, Andy Gross, Baruch Siach, Ben Skeggs,
Bjorn Andersson, Chew, Chiau Ee, Chris Ball, Dan Carpenter,
Daniel Vetter, Dave Airlie, David Herrmann, dmaengine, dri-devel,
Du, Wenkai, Grant Grundler, Ilia Mirkin, intel-gfx,
Ivan T. Ivanov, Jaehoon Chung, Jani Nikula <jani.nikul>
In-Reply-To: <1411575342-31048-1-git-send-email-vinod.koul@intel.com>
On Wednesday, September 24, 2014 09:44:50 PM Vinod Koul wrote:
> This patch series adds a simple macro pm_runtime_last_busy_and_autosuspend()
> which invokes pm_runtime_mark_last_busy() and pm_runtime_put_autosuspend()
> sequentially. Then we do a tree wide update of current patterns which are
> present. As evident from log below this pattern is frequent in the
> kernel.
>
> This series can be found at
> git://git.kernel.org/pub/scm/linux/kernel/git/vkoul/slave-dma.git
> topic/pm_runtime_last_busy_and_autosuspend
>
> Fengguang's kbuild has tested it so it shouldn't break things for anyone.
> Barring one patch (explictyly mentioned in its changelog) rest are simple
> replacements.
>
> If all are okay, this should be merged thru PM tree as it depends on macro
> addition.
>
> Subhransu S. Prusty (1):
> PM: Add helper pm_runtime_last_busy_and_autosuspend()
>
> Vinod Koul (26):
> dmaengine: ste_dma: use pm_runtime_last_busy_and_autosuspend helper
> extcon: arizona: use pm_runtime_last_busy_and_autosuspend helper
> drm/i915: use pm_runtime_last_busy_and_autosuspend helper
> drm/nouveau: use pm_runtime_last_busy_and_autosuspend helper
> drm/radeon: use pm_runtime_last_busy_and_autosuspend helper
> vga_switcheroo: use pm_runtime_last_busy_and_autosuspend helper
> i2c: designware: use pm_runtime_last_busy_and_autosuspend helper
> i2c: omap: use pm_runtime_last_busy_and_autosuspend helper
> i2c: qup: use pm_runtime_last_busy_and_autosuspend helper
> mfd: ab8500-gpadc: use pm_runtime_last_busy_and_autosuspend helper
> mfd: arizona: use pm_runtime_last_busy_and_autosuspend helper
> mei: use pm_runtime_last_busy_and_autosuspend helper
> mmc: use pm_runtime_last_busy_and_autosuspend helper
> mmc: mmci: use pm_runtime_last_busy_and_autosuspend helper
> mmc: omap_hsmmc: use pm_runtime_last_busy_and_autosuspend helper
> mmc: sdhci-pxav3: use pm_runtime_last_busy_and_autosuspend helper
> mmc: sdhci: use pm_runtime_last_busy_and_autosuspend helper
> NFC: trf7970a: use pm_runtime_last_busy_and_autosuspend helper
> pm2301-charger: use pm_runtime_last_busy_and_autosuspend helper
> spi: omap2-mcspi: use pm_runtime_last_busy_and_autosuspend helper
> spi: orion: use pm_runtime_last_busy_and_autosuspend helper
> spi: ti-qspi: use pm_runtime_last_busy_and_autosuspend helper
> spi: core: use pm_runtime_last_busy_and_autosuspend helper
> tty: serial: omap: use pm_runtime_last_busy_and_autosuspend helper
> usb: musb: omap2430: use pm_runtime_last_busy_and_autosuspend helper
> video: fbdev: use pm_runtime_last_busy_and_autosuspend helper
>
> Documentation/power/runtime_pm.txt | 4 ++
> drivers/dma/ste_dma40.c | 30 ++++---------
> drivers/extcon/extcon-arizona.c | 6 +--
> drivers/gpu/drm/i915/intel_pm.c | 3 +-
> drivers/gpu/drm/nouveau/nouveau_connector.c | 3 +-
> drivers/gpu/drm/nouveau/nouveau_drm.c | 9 +---
> drivers/gpu/drm/radeon/radeon_connectors.c | 15 ++----
> drivers/gpu/drm/radeon/radeon_drv.c | 5 +-
> drivers/gpu/drm/radeon/radeon_kms.c | 6 +--
> drivers/gpu/vga/vga_switcheroo.c | 7 +--
> drivers/i2c/busses/i2c-designware-core.c | 3 +-
> drivers/i2c/busses/i2c-omap.c | 6 +--
> drivers/i2c/busses/i2c-qup.c | 3 +-
> drivers/mfd/ab8500-gpadc.c | 6 +--
> drivers/mfd/arizona-irq.c | 3 +-
> drivers/misc/mei/client.c | 12 ++----
> drivers/mmc/core/core.c | 3 +-
> drivers/mmc/host/mmci.c | 12 ++----
> drivers/mmc/host/omap_hsmmc.c | 19 ++-------
> drivers/mmc/host/sdhci-pxav3.c | 6 +--
> drivers/mmc/host/sdhci.c | 3 +-
> drivers/nfc/trf7970a.c | 3 +-
> drivers/power/pm2301_charger.c | 3 +-
> drivers/spi/spi-omap2-mcspi.c | 9 +---
> drivers/spi/spi-orion.c | 3 +-
> drivers/spi/spi-ti-qspi.c | 5 +-
> drivers/spi/spi.c | 6 +--
> drivers/tty/serial/omap-serial.c | 60 +++++++++------------------
> drivers/usb/musb/omap2430.c | 6 +--
> drivers/video/fbdev/auo_k190x.c | 9 +---
> include/linux/pm_runtime.h | 6 +++
> 31 files changed, 97 insertions(+), 177 deletions(-)
OK, I guess this is as good as it gets.
What tree would you like it go through?
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply
* Re: [PATCH 00/27] add pm_runtime_last_busy_and_autosuspend() helper
From: Vinod Koul @ 2014-09-25 7:58 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: linux-wireless, Baruch Siach, patches, linux-doc, Seungwon Jeon,
Daniel Vetter, Chris Ball, dri-devel, Marcin Wojtas,
Rafael J. Wysocki, Laurent Pinchart, David Herrmann,
Shinya Kuribayashi, Laurent Pinchart, Jingoo Han, Jaehoon Chung,
Jani Nikula, Tomi Valkeinen, Alan Stern, Ben Skeggs,
Bjorn Andersson, Wolfram Sang, Dave Airlie
In-Reply-To: <6374313.I12loOK9hA@vostro.rjw.lan>
On Wed, Sep 24, 2014 at 10:28:07PM +0200, Rafael J. Wysocki wrote:
>
> OK, I guess this is as good as it gets.
>
> What tree would you like it go through?
Since rest of the patches are dependent upon 1st patch which should go thru
your tree, we should merge this thru your tree
Thanks
--
~Vinod
^ permalink raw reply
* Re: [PATCH] drm/exynos/fbdev: set smem_len for fbdev
From: Geert Uytterhoeven @ 2014-09-25 9:32 UTC (permalink / raw)
To: Daniel Kurtz
Cc: Linux Fbdev development list, Seung-Woo Kim, Kyungmin Park,
DRI Development, linux-kernel@vger.kernel.org, Günther Noack
In-Reply-To: <1408891857-14258-1-git-send-email-djkurtz@chromium.org>
On Sun, Aug 24, 2014 at 4:50 PM, Daniel Kurtz <djkurtz@chromium.org> wrote:
> Commit [0] stopped setting fix.smem_start and fix.smem_len when creating
> the fbdev.
>
> [0] 2f1eab8d8ab59e799f7d51d62410b398607a7bc3
> drm/exynos/fbdev: don't set fix.smem/mmio_{start,len}
>
> However, smem_len is used by some userland applications to calculate the
> size for mmap. In particular, it is used by xf86-video-fbdev:
>
> http://cgit.freedesktop.org/xorg/xserver/tree/hw/xfree86/fbdevhw/fbdevhw.c?id=xorg-server-1.15.99.903#n571
>
> So, let's restore setting the smem_len to unbreak things for these users.
>
> Note: we are still leaving smem_start set to 0.
Doesn't this cause a system crash when userspace (e.g. fbtest) writes into
the mmap()ed /dev/fb*, as the wrong MMIO region is mapped?
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
* Re: [PATCH] drm/exynos/fbdev: set smem_len for fbdev
From: Daniel Kurtz @ 2014-09-25 10:07 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Linux Fbdev development list, Seung-Woo Kim, Kyungmin Park,
DRI Development, linux-kernel@vger.kernel.org, Günther Noack
In-Reply-To: <CAMuHMdUoh6+gDcZty76HJhcQcJvM5DyBiD4YrZGuAX9e+F32-w@mail.gmail.com>
On Thu, Sep 25, 2014 at 5:32 PM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> On Sun, Aug 24, 2014 at 4:50 PM, Daniel Kurtz <djkurtz@chromium.org> wrote:
>> Commit [0] stopped setting fix.smem_start and fix.smem_len when creating
>> the fbdev.
>>
>> [0] 2f1eab8d8ab59e799f7d51d62410b398607a7bc3
>> drm/exynos/fbdev: don't set fix.smem/mmio_{start,len}
>>
>> However, smem_len is used by some userland applications to calculate the
>> size for mmap. In particular, it is used by xf86-video-fbdev:
>>
>> http://cgit.freedesktop.org/xorg/xserver/tree/hw/xfree86/fbdevhw/fbdevhw.c?id=xorg-server-1.15.99.903#n571
>>
>> So, let's restore setting the smem_len to unbreak things for these users.
>>
>> Note: we are still leaving smem_start set to 0.
>
> Doesn't this cause a system crash when userspace (e.g. fbtest) writes into
> the mmap()ed /dev/fb*, as the wrong MMIO region is mapped?
Do you see a crash during testing, or is your question hypothetical?
I don't think there will be one. exynos's fbev defines its own
fb_ops.fb_mmap, which uses dma_mmap_attrs() to mmap the fb's gem
buffer, which was allocated by dma_alloc_attrs(). This bypasses the
code in drivers/video/fbmem.c:fb_mmap() that references
fix.smem_start.
But, perhaps I am missing something?
-Dan
>
> 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
* Re: [PATCH] drm/exynos/fbdev: set smem_len for fbdev
From: Geert Uytterhoeven @ 2014-09-25 10:14 UTC (permalink / raw)
To: Daniel Kurtz
Cc: Linux Fbdev development list, Seung-Woo Kim, Kyungmin Park,
DRI Development, linux-kernel@vger.kernel.org, Günther Noack
In-Reply-To: <CAGS+omD055Bk+czk5=SDMRu1g-HGZHBobgasKC9PjF80Ci-czA@mail.gmail.com>
On Thu, Sep 25, 2014 at 12:07 PM, Daniel Kurtz <djkurtz@chromium.org> wrote:
> On Thu, Sep 25, 2014 at 5:32 PM, Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
>> On Sun, Aug 24, 2014 at 4:50 PM, Daniel Kurtz <djkurtz@chromium.org> wrote:
>>> Commit [0] stopped setting fix.smem_start and fix.smem_len when creating
>>> the fbdev.
>>>
>>> [0] 2f1eab8d8ab59e799f7d51d62410b398607a7bc3
>>> drm/exynos/fbdev: don't set fix.smem/mmio_{start,len}
>>>
>>> However, smem_len is used by some userland applications to calculate the
>>> size for mmap. In particular, it is used by xf86-video-fbdev:
>>>
>>> http://cgit.freedesktop.org/xorg/xserver/tree/hw/xfree86/fbdevhw/fbdevhw.c?id=xorg-server-1.15.99.903#n571
>>>
>>> So, let's restore setting the smem_len to unbreak things for these users.
>>>
>>> Note: we are still leaving smem_start set to 0.
>>
>> Doesn't this cause a system crash when userspace (e.g. fbtest) writes into
>> the mmap()ed /dev/fb*, as the wrong MMIO region is mapped?
>
> Do you see a crash during testing, or is your question hypothetical?
It was hypothetical.
> I don't think there will be one. exynos's fbev defines its own
> fb_ops.fb_mmap, which uses dma_mmap_attrs() to mmap the fb's gem
> buffer, which was allocated by dma_alloc_attrs(). This bypasses the
> code in drivers/video/fbmem.c:fb_mmap() that references
> fix.smem_start.
>
> But, perhaps I am missing something?
Thanks, having your own mapping function explains the lack of crashes.
Still, some weird software may map /dev/mem instead of /dev/fb0, and
use fb_fix.smem_start. But that's also unsafe in the context of LPAE.
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
* Re: [PATCH 00/27] add pm_runtime_last_busy_and_autosuspend() helper
From: Rafael J. Wysocki @ 2014-09-25 19:54 UTC (permalink / raw)
To: Wolfram Sang, Vinod Koul
Cc: linux-wireless, Baruch Siach, patches, linux-doc, Seungwon Jeon,
Daniel Vetter, Chris Ball, dri-devel, Marcin Wojtas,
Rafael J. Wysocki, Laurent Pinchart, David Herrmann,
Shinya Kuribayashi, Laurent Pinchart, Jingoo Han, Jaehoon Chung,
Jani Nikula, Tomi Valkeinen, Alan Stern, Ben Skeggs,
Bjorn Andersson, Dave Airlie, Ulf Hansson <ulf.hanss>
In-Reply-To: <20140925142758.GB1316@katana>
On Thursday, September 25, 2014 04:27:58 PM Wolfram Sang wrote:
>
> --Bn2rw/3z4jIqBvZU
> Content-Type: text/plain; charset=us-ascii
> Content-Disposition: inline
> Content-Transfer-Encoding: quoted-printable
>
> On Thu, Sep 25, 2014 at 09:22:01AM -0500, Felipe Balbi wrote:
> > On Thu, Sep 25, 2014 at 01:27:18PM +0530, Vinod Koul wrote:
> > > On Wed, Sep 24, 2014 at 03:32:19PM -0500, Felipe Balbi wrote:
> > > > > > > OK, I guess this is as good as it gets.
> > > > > > >
> > > > > > > What tree would you like it go through?
> > > > > >
> > > > > > Do we really need this new helper ? I mean, the very moment when > we
> > > > > > decide to implement ->runtime_idle() we will need to get rid of t> his
> > > > > > change. I wonder if it's really valid...
> > > > >
> > > > > I'm not sure I'm following? This seems to simply implement what dr> ivers
> > > > > have been doing already as one function. Why would it be invalid t> o reduce
> > > > > code duplication?
> > > >
> > > > For two reasons:
> > > >
> > > > 1) the helper has no inteligence whatsoever. It just calls the same
> > > > functions.
> > > >
> > > > 2) the duplication will vanish whenever someone implements
> > > > ->runtime_idle() and have that call pm_runtime_autosuspend() (like PCI
> > > > and USB buses are doing today). This will just be yet another line th> at
> > > > needs to change.
> > > >
> > > > Frankly though, no strong feelings, I just think it's a commit that
> > > > doesn't bring that any benefits other than looking like one line was
> > > > removed.
> > > and yes that is what it tries to do nothing more nothing less. If in fu> ture
> > > there are no users (today we have quite a few), then we can remove the > dead
> > > macro, no harm. But that is not the situation today.
> >
> > as I said, a commit that's bound to be useless. It's not like you're
> > saving 10 lines of code, it's only one. Replacing two simple lines with
> > a function which takes <joke> almost as many characters to type </joke>.
> >
> > IMO, this is pretty useless and I'd rather not see them in the drivers I
> > maintain, sorry.
>
> It is not a NACK from me; yet from a high-level perspective I agree with
> Felipe.
OK
I'd rather not merge something that driver people don't want to use.
Vinod?
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply
* [PATCH 0/2] LLVMLinux: Patches to enable the kernel to be compiled with clang/LLVM
From: Behan Webster @ 2014-09-27 1:10 UTC (permalink / raw)
To: plagnioj, tomi.valkeinen
Cc: behanw, archit, daniel.vetter, geert, laurent.pinchart,
linux-fbdev, linux-kernel, linux-omap, robdclark
Replace the use of nested functions where a normal function will suffice.
Nested functions are not liked by upstream kernel developers in general. Their
use breaks the use of clang as a compiler, and doesn't make the code any
better.
This code now works for both gcc and clang.
The LLVMLinux project aims to fully build the Linux kernel using both gcc and
clang (the C front end for the LLVM compiler infrastructure project).
Behan Webster (2):
arm, fbdev, omap2, LLVMLinux: Remove nested function from omap2 dss
arm, fbdev, omap2, LLVMLinux: Remove nested function from omapfb
drivers/video/fbdev/omap2/dss/dispc-compat.c | 9 +++++----
drivers/video/fbdev/omap2/dss/manager-sysfs.c | 16 +++++++++-------
drivers/video/fbdev/omap2/omapfb/omapfb-main.c | 14 +++++++-------
3 files changed, 21 insertions(+), 18 deletions(-)
--
1.9.1
^ permalink raw reply
* [PATCH 1/2] arm, fbdev, omap2, LLVMLinux: Remove nested function from omap2 dss
From: Behan Webster @ 2014-09-27 1:10 UTC (permalink / raw)
To: plagnioj, tomi.valkeinen
Cc: behanw, archit, daniel.vetter, geert, laurent.pinchart,
linux-fbdev, linux-kernel, linux-omap, robdclark, Arnd Bergmann
In-Reply-To: <1411780253-4492-1-git-send-email-behanw@converseincode.com>
Replace the use of nested functions where a normal function will suffice.
Nested functions are not liked by upstream kernel developers in general. Their
use breaks the use of clang as a compiler, and doesn't make the code any
better.
This code now works for both gcc and clang.
Signed-off-by: Behan Webster <behanw@converseincode.com>
Suggested-by: Arnd Bergmann <arnd@arndb.de>
Cc: Arnd Bergmann <arnd@arndb.de>
---
drivers/video/fbdev/omap2/dss/dispc-compat.c | 9 +++++----
drivers/video/fbdev/omap2/dss/manager-sysfs.c | 16 +++++++++-------
2 files changed, 14 insertions(+), 11 deletions(-)
diff --git a/drivers/video/fbdev/omap2/dss/dispc-compat.c b/drivers/video/fbdev/omap2/dss/dispc-compat.c
index 83779c2..633c461 100644
--- a/drivers/video/fbdev/omap2/dss/dispc-compat.c
+++ b/drivers/video/fbdev/omap2/dss/dispc-compat.c
@@ -634,13 +634,14 @@ void dispc_mgr_disable_sync(enum omap_channel channel)
WARN_ON(1);
}
+static inline void dispc_irq_wait_handler(void *data, u32 mask)
+{
+ complete((struct completion *)data);
+}
+
int omap_dispc_wait_for_irq_interruptible_timeout(u32 irqmask,
unsigned long timeout)
{
- void dispc_irq_wait_handler(void *data, u32 mask)
- {
- complete((struct completion *)data);
- }
int r;
DECLARE_COMPLETION_ONSTACK(completion);
diff --git a/drivers/video/fbdev/omap2/dss/manager-sysfs.c b/drivers/video/fbdev/omap2/dss/manager-sysfs.c
index 37b59fe..a7414fb 100644
--- a/drivers/video/fbdev/omap2/dss/manager-sysfs.c
+++ b/drivers/video/fbdev/omap2/dss/manager-sysfs.c
@@ -44,6 +44,13 @@ static ssize_t manager_display_show(struct omap_overlay_manager *mgr, char *buf)
dssdev->name : "<none>");
}
+static int manager_display_match(struct omap_dss_device *dssdev, void *data)
+{
+ const char *str = data;
+
+ return sysfs_streq(dssdev->name, str);
+}
+
static ssize_t manager_display_store(struct omap_overlay_manager *mgr,
const char *buf, size_t size)
{
@@ -52,17 +59,12 @@ static ssize_t manager_display_store(struct omap_overlay_manager *mgr,
struct omap_dss_device *dssdev = NULL;
struct omap_dss_device *old_dssdev;
- int match(struct omap_dss_device *dssdev, void *data)
- {
- const char *str = data;
- return sysfs_streq(dssdev->name, str);
- }
-
if (buf[size-1] = '\n')
--len;
if (len > 0)
- dssdev = omap_dss_find_device((void *)buf, match);
+ dssdev = omap_dss_find_device((void *)buf,
+ manager_display_match);
if (len > 0 && dssdev = NULL)
return -EINVAL;
--
1.9.1
^ permalink raw reply related
* [PATCH 2/2] arm, fbdev, omap2, LLVMLinux: Remove nested function from omapfb
From: Behan Webster @ 2014-09-27 1:10 UTC (permalink / raw)
To: plagnioj, tomi.valkeinen
Cc: behanw, archit, daniel.vetter, geert, laurent.pinchart,
linux-fbdev, linux-kernel, linux-omap, robdclark, Arnd Bergmann
In-Reply-To: <1411780253-4492-1-git-send-email-behanw@converseincode.com>
Replace the use of nested functions where a normal function will suffice.
Nested functions are not liked by upstream kernel developers in general. Their
use breaks the use of clang as a compiler, and doesn't make the code any
better.
This code now works for both gcc and clang.
Signed-off-by: Behan Webster <behanw@converseincode.com>
Suggested-by: Arnd Bergmann <arnd@arndb.de>
Cc: Arnd Bergmann <arnd@arndb.de>
---
drivers/video/fbdev/omap2/omapfb/omapfb-main.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/video/fbdev/omap2/omapfb/omapfb-main.c b/drivers/video/fbdev/omap2/omapfb/omapfb-main.c
index ec2d132..1587243 100644
--- a/drivers/video/fbdev/omap2/omapfb/omapfb-main.c
+++ b/drivers/video/fbdev/omap2/omapfb/omapfb-main.c
@@ -273,16 +273,16 @@ static struct omapfb_colormode omapfb_colormodes[] = {
},
};
+static bool cmp_component(struct fb_bitfield *f1, struct fb_bitfield *f2)
+{
+ return f1->length = f2->length &&
+ f1->offset = f2->offset &&
+ f1->msb_right = f2->msb_right;
+}
+
static bool cmp_var_to_colormode(struct fb_var_screeninfo *var,
struct omapfb_colormode *color)
{
- bool cmp_component(struct fb_bitfield *f1, struct fb_bitfield *f2)
- {
- return f1->length = f2->length &&
- f1->offset = f2->offset &&
- f1->msb_right = f2->msb_right;
- }
-
if (var->bits_per_pixel = 0 ||
var->red.length = 0 ||
var->blue.length = 0 ||
--
1.9.1
^ permalink raw reply related
* Re: [PATCH 1/2] arm, fbdev, omap2, LLVMLinux: Remove nested function from omap2 dss
From: Felipe Balbi @ 2014-09-27 16:46 UTC (permalink / raw)
To: Behan Webster
Cc: plagnioj, tomi.valkeinen, daniel.vetter, geert, laurent.pinchart,
linux-fbdev, linux-kernel, linux-omap, robdclark, Arnd Bergmann
In-Reply-To: <1411780253-4492-2-git-send-email-behanw@converseincode.com>
[-- Attachment #1: Type: text/plain, Size: 664 bytes --]
On Fri, Sep 26, 2014 at 06:10:52PM -0700, Behan Webster wrote:
> Replace the use of nested functions where a normal function will suffice.
>
> Nested functions are not liked by upstream kernel developers in general. Their
> use breaks the use of clang as a compiler, and doesn't make the code any
> better.
>
> This code now works for both gcc and clang.
>
> Signed-off-by: Behan Webster <behanw@converseincode.com>
> Suggested-by: Arnd Bergmann <arnd@arndb.de>
> Cc: Arnd Bergmann <arnd@arndb.de>
another one that make sense :-) And probably also deserves
checkpatch/coccinelle/sparse.
Reviewed-by: Felipe Balbi <balbi@ti.com>
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH 2/2] arm, fbdev, omap2, LLVMLinux: Remove nested function from omapfb
From: Felipe Balbi @ 2014-09-27 16:46 UTC (permalink / raw)
To: Behan Webster
Cc: plagnioj, tomi.valkeinen, daniel.vetter, geert, laurent.pinchart,
linux-fbdev, linux-kernel, linux-omap, robdclark, Arnd Bergmann
In-Reply-To: <1411780253-4492-3-git-send-email-behanw@converseincode.com>
[-- Attachment #1: Type: text/plain, Size: 571 bytes --]
On Fri, Sep 26, 2014 at 06:10:53PM -0700, Behan Webster wrote:
> Replace the use of nested functions where a normal function will suffice.
>
> Nested functions are not liked by upstream kernel developers in general. Their
> use breaks the use of clang as a compiler, and doesn't make the code any
> better.
>
> This code now works for both gcc and clang.
>
> Signed-off-by: Behan Webster <behanw@converseincode.com>
> Suggested-by: Arnd Bergmann <arnd@arndb.de>
> Cc: Arnd Bergmann <arnd@arndb.de>
Reviewed-by: Felipe Balbi <balbi@ti.com>
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH 1/2] arm, fbdev, omap2, LLVMLinux: Remove nested function from omap2 dss
From: Behan Webster @ 2014-09-27 22:53 UTC (permalink / raw)
To: balbi
Cc: plagnioj, tomi.valkeinen, daniel.vetter, geert, laurent.pinchart,
linux-fbdev, linux-kernel, linux-omap, robdclark, Arnd Bergmann
In-Reply-To: <20140927164554.GA7076@saruman>
On 09/27/14 09:46, Felipe Balbi wrote:
> On Fri, Sep 26, 2014 at 06:10:52PM -0700, Behan Webster wrote:
>> Replace the use of nested functions where a normal function will suffice.
>>
>> Nested functions are not liked by upstream kernel developers in general. Their
>> use breaks the use of clang as a compiler, and doesn't make the code any
>> better.
>>
>> This code now works for both gcc and clang.
>>
>> Signed-off-by: Behan Webster <behanw@converseincode.com>
>> Suggested-by: Arnd Bergmann <arnd@arndb.de>
>> Cc: Arnd Bergmann <arnd@arndb.de>
> another one that make sense :-) And probably also deserves
> checkpatch/coccinelle/sparse.
Indeed! That would be very appreciated.
The clang static analyzer already points this one out. :)
Behan
--
Behan Webster
behanw@converseincode.com
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Mike Turquette @ 2014-09-27 23:56 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140902092508.GR15297@lukather>
Quoting Maxime Ripard (2014-09-02 02:25:08)
> On Fri, Aug 29, 2014 at 04:38:14PM +0200, Thierry Reding wrote:
> > On Fri, Aug 29, 2014 at 04:12:44PM +0200, Maxime Ripard wrote:
> > > On Fri, Aug 29, 2014 at 09:01:17AM +0200, Thierry Reding wrote:
> > > > I would think the memory should still be reserved anyway to make sure
> > > > nothing else is writing over it. And it's in the device tree anyway
> > > > because the driver needs to know where to put framebuffer content. So
> > > > the point I was trying to make is that we can't treat the memory in the
> > > > same way as clocks because it needs to be explicitly managed. Whereas
> > > > clocks don't. The driver is simply too generic to know what to do with
> > > > the clocks.
> > >
> > > You agreed on the fact that the only thing we need to do with the
> > > clocks is claim them. Really, I don't find what's complicated there
> > > (or not generic).
> >
> > That's not what I agreed on. What I said is that the only thing we need
> > to do with the clocks is nothing. They are already in the state that
> > they need to be.
>
> Claim was probably a poor choice of words, but still. We have to keep
> the clock running, and both the solution you've been giving and this
> patch do so in a generic way.
>
> > > > It doesn't know what frequency they should be running at
> > >
> > > We don't care about that. Just like we don't care about which
> > > frequency is the memory bus running at. It will just run at whatever
> > > frequency is appropriate.
> >
> > Exactly. And you shouldn't have to care about them at all. Firmware has
> > already configured the clocks to run at the correct frequencies, and it
> > has made sure that they are enabled.
> >
> > > > or what they're used for
> > >
> > > And we don't care about that either. You're not interested in what
> > > output the framebuffer is setup to use, which is pretty much the same
> > > here, this is the same thing here.
> >
> > That's precisely what I've been saying. The only thing that simplefb
> > cares about is the memory it should be using and the format of the
> > pixels (and how many of them) it writes into that memory. Everything
> > else is assumed to have been set up.
> >
> > Including clocks.
>
> We're really discussing in circles here.
>
> Mike?
>
-EHIGHLATENCYRESPONSE
I forgot about this thread. Sorry.
In an attempt to provide the least helpful answer possible, I will stay
clear of all of the stuff relating to "how simple should simplefb be"
and the related reserved memory discussion.
A few times in this thread it is stated that we can't prevent unused
clocks from being disabled. That is only partially true.
The clock framework DOES provide a flag to prevent UNUSED clocks from
being disabled at late_initcall-time by a clock "garbage collector"
mechanism. Maxime and others familiar with the clock framework are aware
of this.
What the framework doesn't do is to allow for a magic flag in DT, in
platform_data or elsewhere that says, "don't let me get turned off until
the right driver claims me". That would be an external or alternative
method for preventing a clock from being disabled. We have a method for
preventing clocks from being disabled. It is as follows:
struct clk *my_clk = clk_get(...);
clk_prepare_enable(my_clk);
That is how it should be done. Period.
With that said I think that Luc's approach is very sensible. I'm not
sure what purpose in the universe DT is supposed to serve if not for
_this_exact_case_. We have a nice abstracted driver, usable by many
people. The hardware details of how it is hooked up at the board level
can all be hidden behind stable DT bindings that everyone already uses.
What more could you want?
Regards,
Mike
> Your opinion would be very valuable.
>
> > > > so by any definition of what DT should describe they're useless for
> > > > this virtual device.
> > > >
> > > > Furthermore it's fairly likely that as your kernel support progresses
> > > > you'll find that the driver all of a sudden needs to manage some other
> > > > type of resource that you just haven't needed until now because it may
> > > > default to being always on. Then you'll have a hard time keeping
> > > > backwards-compatibility and will have to resort to the kinds of hacks
> > > > that you don't want to see in the kernel.
> > >
> > > Not such a hard time. An older DT wouldn't define the new requirements
> > > anyway, so they wouldn't be used, and we would end up in pretty much
> > > the current case.
> >
> > Except that you have firmware in the wild that sets up an incomplete
> > simplefb node and if you don't want to break compatibility you need to
> > provide fallbacks for the resources that aren't listed in the DT node.
> > And given that those fallbacks are all very board specific you'll need
> > to find ways to keep them enabled if you want to keep existing setups
> > working.
>
> How would an *optional* property break those users?
>
> If you don't need any clock to be kept running (or are hiding them
> under the carpet), of course you don't need such a property.
>
> Maxime
>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux, Kernel and Android engineering
> http://free-electrons.com
^ permalink raw reply
* [PATCH 1/4] dt-bindings: Add a clocks property to the simple-framebuffer binding
From: Hans de Goede @ 2014-09-28 12:43 UTC (permalink / raw)
To: linux-arm-kernel
A simple-framebuffer node represents a framebuffer setup by the firmware /
bootloader. Such a framebuffer may have a number of clocks in use, add a
property to communicate this to the OS.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Documentation/devicetree/bindings/video/simple-framebuffer.txt | 3 +++
1 file changed, 3 insertions(+)
diff --git a/Documentation/devicetree/bindings/video/simple-framebuffer.txt b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
index 70c26f3..e75478e 100644
--- a/Documentation/devicetree/bindings/video/simple-framebuffer.txt
+++ b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
@@ -14,6 +14,9 @@ Required properties:
- r5g6b5 (16-bit pixels, d[15:11]=r, d[10:5]=g, d[4:0]=b).
- a8b8g8r8 (32-bit pixels, d[31:24]=a, d[23:16]=b, d[15:8]=g, d[7:0]=r).
+Optional properties:
+- clocks : List of clocks used by the framebuffer
+
Example:
framebuffer {
--
2.1.0
^ permalink raw reply related
* [PATCH 2/4] simplefb: formalize pseudo palette handling
From: Hans de Goede @ 2014-09-28 12:43 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1411908218-20947-1-git-send-email-hdegoede@redhat.com>
From: Luc Verhaegen <libv@skynet.be>
Signed-off-by: Luc Verhaegen <libv@skynet.be>
[hdegoede@redhat.com: drop unnecessary void * cast]
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/video/fbdev/simplefb.c | 15 ++++++++++++---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
index 210f3a0..ec112c1 100644
--- a/drivers/video/fbdev/simplefb.c
+++ b/drivers/video/fbdev/simplefb.c
@@ -41,6 +41,8 @@ static struct fb_var_screeninfo simplefb_var = {
.vmode = FB_VMODE_NONINTERLACED,
};
+#define PSEUDO_PALETTE_SIZE 16
+
static int simplefb_setcolreg(u_int regno, u_int red, u_int green, u_int blue,
u_int transp, struct fb_info *info)
{
@@ -50,7 +52,7 @@ static int simplefb_setcolreg(u_int regno, u_int red, u_int green, u_int blue,
u32 cb = blue >> (16 - info->var.blue.length);
u32 value;
- if (regno >= 16)
+ if (regno >= PSEUDO_PALETTE_SIZE)
return -EINVAL;
value = (cr << info->var.red.offset) |
@@ -163,11 +165,16 @@ static int simplefb_parse_pd(struct platform_device *pdev,
return 0;
}
+struct simplefb_par {
+ u32 palette[PSEUDO_PALETTE_SIZE];
+};
+
static int simplefb_probe(struct platform_device *pdev)
{
int ret;
struct simplefb_params params;
struct fb_info *info;
+ struct simplefb_par *par;
struct resource *mem;
if (fb_get_options("simplefb", NULL))
@@ -188,11 +195,13 @@ static int simplefb_probe(struct platform_device *pdev)
return -EINVAL;
}
- info = framebuffer_alloc(sizeof(u32) * 16, &pdev->dev);
+ info = framebuffer_alloc(sizeof(struct simplefb_par), &pdev->dev);
if (!info)
return -ENOMEM;
platform_set_drvdata(pdev, info);
+ par = info->par;
+
info->fix = simplefb_fix;
info->fix.smem_start = mem->start;
info->fix.smem_len = resource_size(mem);
@@ -225,7 +234,7 @@ static int simplefb_probe(struct platform_device *pdev)
framebuffer_release(info);
return -ENODEV;
}
- info->pseudo_palette = (void *)(info + 1);
+ info->pseudo_palette = par->palette;
dev_info(&pdev->dev, "framebuffer at 0x%lx, 0x%x bytes, mapped to 0x%p\n",
info->fix.smem_start, info->fix.smem_len,
--
2.1.0
^ permalink raw reply related
* [PATCH 3/4] simplefb: add goto error path to probe
From: Hans de Goede @ 2014-09-28 12:43 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1411908218-20947-1-git-send-email-hdegoede@redhat.com>
From: Luc Verhaegen <libv@skynet.be>
Signed-off-by: Luc Verhaegen <libv@skynet.be>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/video/fbdev/simplefb.c | 20 +++++++++++++-------
1 file changed, 13 insertions(+), 7 deletions(-)
diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
index ec112c1..b7d5c08 100644
--- a/drivers/video/fbdev/simplefb.c
+++ b/drivers/video/fbdev/simplefb.c
@@ -220,8 +220,8 @@ static int simplefb_probe(struct platform_device *pdev)
info->apertures = alloc_apertures(1);
if (!info->apertures) {
- framebuffer_release(info);
- return -ENOMEM;
+ ret = -ENOMEM;
+ goto error_fb_release;
}
info->apertures->ranges[0].base = info->fix.smem_start;
info->apertures->ranges[0].size = info->fix.smem_len;
@@ -231,8 +231,8 @@ static int simplefb_probe(struct platform_device *pdev)
info->screen_base = ioremap_wc(info->fix.smem_start,
info->fix.smem_len);
if (!info->screen_base) {
- framebuffer_release(info);
- return -ENODEV;
+ ret = -ENODEV;
+ goto error_fb_release;
}
info->pseudo_palette = par->palette;
@@ -247,14 +247,20 @@ static int simplefb_probe(struct platform_device *pdev)
ret = register_framebuffer(info);
if (ret < 0) {
dev_err(&pdev->dev, "Unable to register simplefb: %d\n", ret);
- iounmap(info->screen_base);
- framebuffer_release(info);
- return ret;
+ goto error_unmap;
}
dev_info(&pdev->dev, "fb%d: simplefb registered!\n", info->node);
return 0;
+
+ error_unmap:
+ iounmap(info->screen_base);
+
+ error_fb_release:
+ framebuffer_release(info);
+
+ return ret;
}
static int simplefb_remove(struct platform_device *pdev)
--
2.1.0
^ permalink raw reply related
* [PATCH 4/4] simplefb: add clock handling code
From: Hans de Goede @ 2014-09-28 12:43 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1411908218-20947-1-git-send-email-hdegoede@redhat.com>
From: Luc Verhaegen <libv@skynet.be>
This claims and enables clocks listed in the simple framebuffer dt node.
This is needed so that the display engine, in case the required clocks
are known by the kernel code and are described in the dt, will remain
properly enabled.
Signed-off-by: Luc Verhaegen <libv@skynet.be>
[hdegoede@redhat.com: drop dev_err on kzalloc failure]
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/video/fbdev/simplefb.c | 100 ++++++++++++++++++++++++++++++++++++++++-
1 file changed, 98 insertions(+), 2 deletions(-)
diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
index b7d5c08..f329cc1 100644
--- a/drivers/video/fbdev/simplefb.c
+++ b/drivers/video/fbdev/simplefb.c
@@ -26,6 +26,7 @@
#include <linux/module.h>
#include <linux/platform_data/simplefb.h>
#include <linux/platform_device.h>
+#include <linux/clk-provider.h>
static struct fb_fix_screeninfo simplefb_fix = {
.id = "simple",
@@ -165,8 +166,98 @@ static int simplefb_parse_pd(struct platform_device *pdev,
return 0;
}
+/*
+ * Clock handling code.
+ *
+ * Here we handle the clocks property of our "simple-framebuffer" dt node.
+ * This is necessary so that we can make sure that any clocks needed by
+ * the display engine that the bootloader set up for us (and for which it
+ * provided a simplefb dt node), stay up, for the life of the simplefb
+ * driver.
+ *
+ * When the driver unloads, we cleanly disable, and then release the clocks.
+ */
+struct simplefb_clock {
+ struct list_head list;
+ struct clk *clock;
+};
+
+/*
+ * We only complain about errors here, no action is taken as the most likely
+ * error can only happen due to a mismatch between the bootloader which set
+ * up simplefb, and the clock definitions in the device tree. Chances are
+ * that there are no adverse effects, and if there are, a clean teardown of
+ * the fb probe will not help us much either. So just complain and carry on,
+ * and hope that the user actually gets a working fb at the end of things.
+ */
+static void
+simplefb_clocks_init(struct platform_device *pdev, struct list_head *list)
+{
+ struct device_node *np = pdev->dev.of_node;
+ int clock_count, i;
+
+ INIT_LIST_HEAD(list);
+
+ if (dev_get_platdata(&pdev->dev) || !np)
+ return;
+
+ clock_count = of_clk_get_parent_count(np);
+ for (i = 0; i < clock_count; i++) {
+ struct simplefb_clock *entry;
+ struct clk *clock = of_clk_get(np, i);
+ int ret;
+
+ if (IS_ERR(clock)) {
+ dev_err(&pdev->dev, "%s: clock %d not found: %ld\n",
+ __func__, i, PTR_ERR(clock));
+ continue;
+ }
+
+ ret = clk_prepare_enable(clock);
+ if (ret) {
+ dev_err(&pdev->dev,
+ "%s: failed to enable clock %d: %d\n",
+ __func__, i, ret);
+ clk_put(clock);
+ continue;
+ }
+
+ entry = kzalloc(sizeof(struct simplefb_clock), GFP_KERNEL);
+ if (!entry) {
+ clk_disable_unprepare(clock);
+ clk_put(clock);
+ continue;
+ }
+
+ entry->clock = clock;
+ /*
+ * add to the front of the list, so we disable clocks in the
+ * correct order.
+ */
+ list_add(&entry->list, list);
+ }
+}
+
+static void
+simplefb_clocks_destroy(struct list_head *list)
+{
+ struct list_head *pos, *n;
+
+ list_for_each_safe(pos, n, list) {
+ struct simplefb_clock *entry + container_of(pos, struct simplefb_clock, list);
+
+ list_del(&entry->list);
+
+ clk_disable_unprepare(entry->clock);
+ clk_put(entry->clock);
+ kfree(entry);
+ }
+}
+
struct simplefb_par {
u32 palette[PSEUDO_PALETTE_SIZE];
+ struct list_head clock_list[1];
};
static int simplefb_probe(struct platform_device *pdev)
@@ -236,6 +327,8 @@ static int simplefb_probe(struct platform_device *pdev)
}
info->pseudo_palette = par->palette;
+ simplefb_clocks_init(pdev, par->clock_list);
+
dev_info(&pdev->dev, "framebuffer at 0x%lx, 0x%x bytes, mapped to 0x%p\n",
info->fix.smem_start, info->fix.smem_len,
info->screen_base);
@@ -247,14 +340,15 @@ static int simplefb_probe(struct platform_device *pdev)
ret = register_framebuffer(info);
if (ret < 0) {
dev_err(&pdev->dev, "Unable to register simplefb: %d\n", ret);
- goto error_unmap;
+ goto error_clocks;
}
dev_info(&pdev->dev, "fb%d: simplefb registered!\n", info->node);
return 0;
- error_unmap:
+ error_clocks:
+ simplefb_clocks_destroy(par->clock_list);
iounmap(info->screen_base);
error_fb_release:
@@ -266,8 +360,10 @@ static int simplefb_probe(struct platform_device *pdev)
static int simplefb_remove(struct platform_device *pdev)
{
struct fb_info *info = platform_get_drvdata(pdev);
+ struct simplefb_par *par = info->par;
unregister_framebuffer(info);
+ simplefb_clocks_destroy(par->clock_list);
framebuffer_release(info);
return 0;
--
2.1.0
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox