* Re: [PATCH 19/27] OMAP: DSS2: Use PM runtime & HWMOD support
From: Cousson, Benoit @ 2011-06-06 13:46 UTC (permalink / raw)
To: Valkeinen, Tomi
Cc: Hilman, Kevin, linux-omap@vger.kernel.org,
linux-fbdev@vger.kernel.org, paul@pwsan.com
In-Reply-To: <1307366474.1910.44.camel@deskari>
On 6/6/2011 3:21 PM, Valkeinen, Tomi wrote:
> On Mon, 2011-06-06 at 15:15 +0200, Cousson, Benoit wrote:
>> On 6/6/2011 3:01 PM, Valkeinen, Tomi wrote:
>>> On Mon, 2011-06-06 at 14:56 +0200, Cousson, Benoit wrote:
>
>>> In this long term solution, if the dss_fclk is the main_clk, how does
>>> the framework handle the situation when we want to switch from the
>>> standard DSS fclk to the one from DSI PLL?
>>
>> That part cannot be done by the hwmod fmwk anyway. The goal of the fmwk
>> is to ensure that the module is accessible by the driver whatever the
>> PRCM clock used.
>> Enabling the DSI PLL will require the PRCM clock to be enabled first.
>>
>> Using the DSI PLL as the fclk is doable, but is it really useful or needed?
>
> Yes, it's useful and needed. It gives us much finer control to the clock
> frequencies, and so allows us to go to higher frequencies and also more
> exactly to the required pixel clock.
>
>> Assuming you need that mode, you will always have to explicitly switch
>> from DSI to PRCM clock before trying to disable the DSS.
>> This is something you will have to do inside the DSS driver. It should
>> be transparent to the hwmod fmwk.
>
> This sounds ok.
>
> I think the main question is how do we disable the standard DSS fclk
> from PRCM when using DSI PLL? As far as I know, disabling that clock
> will allow some areas of OMAP to be shut down even while DSS is working.
> So from power management point of view it sounds a needed feature.
Yes, at least in theory, but considering that any use case that will
require the DSI PLL will use a LCD panel + backlight, or an OLED panel
that will consume 50 times more than the 186 MHz clock, I do not think
it is really needed.
Moreover, that clock is generated by the PER DPLL that will be always
enabled in most usecase because it does generate the UART, I2C and most
basic peripherals clocks. If we cannot gate the PER DPLL, there is no
saving to expect from gating the DSS fclk only.
Bottom-line is that there is no practical power saving to expect from
that mode.
> If the clock is main_clk for the HWMOD, it sounds to me it's always
> enabled if the HWMOD is enabled?
Yes, but that sounds to me a good trade off to avoid unnecessary
complexity in your driver or in the hwmod fmwk.
Regards,
Benoit
^ permalink raw reply
* Re: [PATCH 19/27] OMAP: DSS2: Use PM runtime & HWMOD support
From: Tomi Valkeinen @ 2011-06-06 13:55 UTC (permalink / raw)
To: Cousson, Benoit
Cc: Hilman, Kevin, linux-omap@vger.kernel.org,
linux-fbdev@vger.kernel.org, paul@pwsan.com
In-Reply-To: <4DECDA3A.7080808@ti.com>
On Mon, 2011-06-06 at 15:46 +0200, Cousson, Benoit wrote:
> On 6/6/2011 3:21 PM, Valkeinen, Tomi wrote:
> > On Mon, 2011-06-06 at 15:15 +0200, Cousson, Benoit wrote:
> >> On 6/6/2011 3:01 PM, Valkeinen, Tomi wrote:
> >>> On Mon, 2011-06-06 at 14:56 +0200, Cousson, Benoit wrote:
> >
> >>> In this long term solution, if the dss_fclk is the main_clk, how does
> >>> the framework handle the situation when we want to switch from the
> >>> standard DSS fclk to the one from DSI PLL?
> >>
> >> That part cannot be done by the hwmod fmwk anyway. The goal of the fmwk
> >> is to ensure that the module is accessible by the driver whatever the
> >> PRCM clock used.
> >> Enabling the DSI PLL will require the PRCM clock to be enabled first.
> >>
> >> Using the DSI PLL as the fclk is doable, but is it really useful or needed?
> >
> > Yes, it's useful and needed. It gives us much finer control to the clock
> > frequencies, and so allows us to go to higher frequencies and also more
> > exactly to the required pixel clock.
> >
> >> Assuming you need that mode, you will always have to explicitly switch
> >> from DSI to PRCM clock before trying to disable the DSS.
> >> This is something you will have to do inside the DSS driver. It should
> >> be transparent to the hwmod fmwk.
> >
> > This sounds ok.
> >
> > I think the main question is how do we disable the standard DSS fclk
> > from PRCM when using DSI PLL? As far as I know, disabling that clock
> > will allow some areas of OMAP to be shut down even while DSS is working.
> > So from power management point of view it sounds a needed feature.
>
> Yes, at least in theory, but considering that any use case that will
> require the DSI PLL will use a LCD panel + backlight, or an OLED panel
> that will consume 50 times more than the 186 MHz clock, I do not think
> it is really needed.
> Moreover, that clock is generated by the PER DPLL that will be always
> enabled in most usecase because it does generate the UART, I2C and most
> basic peripherals clocks. If we cannot gate the PER DPLL, there is no
> saving to expect from gating the DSS fclk only.
> Bottom-line is that there is no practical power saving to expect from
> that mode.
>
> > If the clock is main_clk for the HWMOD, it sounds to me it's always
> > enabled if the HWMOD is enabled?
>
> Yes, but that sounds to me a good trade off to avoid unnecessary
> complexity in your driver or in the hwmod fmwk.
Ok, if there are no real power savings there, then I agree, it's
pointless to add that complexity.
So how do we go forward in short term? I'd very much like to remove all
the "silly" code from the DSS pm_runtime patch series caused by this
opt_clock handling. Is it possible to get some kind of a temporary
solution in the hwmod framework which would somehow solve this from DSS
driver's point of view? A flag that causes hwmod fmwk to enable
opt-clocks automatically? Or is it possible to have more than one
mandatory clock?
This way when your long-term solution is done, the driver would not need
any changes.
Tomi
^ permalink raw reply
* Re: [PATCH 19/27] OMAP: DSS2: Use PM runtime & HWMOD support
From: Cousson, Benoit @ 2011-06-06 15:28 UTC (permalink / raw)
To: Valkeinen, Tomi
Cc: Hilman, Kevin, linux-omap@vger.kernel.org,
linux-fbdev@vger.kernel.org, paul@pwsan.com
In-Reply-To: <1307368525.1910.50.camel@deskari>
On 6/6/2011 3:55 PM, Valkeinen, Tomi wrote:
> On Mon, 2011-06-06 at 15:46 +0200, Cousson, Benoit wrote:
>> On 6/6/2011 3:21 PM, Valkeinen, Tomi wrote:
>>> On Mon, 2011-06-06 at 15:15 +0200, Cousson, Benoit wrote:
>>>> On 6/6/2011 3:01 PM, Valkeinen, Tomi wrote:
>>>>> On Mon, 2011-06-06 at 14:56 +0200, Cousson, Benoit wrote:
>>>
>>>>> In this long term solution, if the dss_fclk is the main_clk, how does
>>>>> the framework handle the situation when we want to switch from the
>>>>> standard DSS fclk to the one from DSI PLL?
>>>>
>>>> That part cannot be done by the hwmod fmwk anyway. The goal of the fmwk
>>>> is to ensure that the module is accessible by the driver whatever the
>>>> PRCM clock used.
>>>> Enabling the DSI PLL will require the PRCM clock to be enabled first.
>>>>
>>>> Using the DSI PLL as the fclk is doable, but is it really useful or needed?
>>>
>>> Yes, it's useful and needed. It gives us much finer control to the clock
>>> frequencies, and so allows us to go to higher frequencies and also more
>>> exactly to the required pixel clock.
>>>
>>>> Assuming you need that mode, you will always have to explicitly switch
>>>> from DSI to PRCM clock before trying to disable the DSS.
>>>> This is something you will have to do inside the DSS driver. It should
>>>> be transparent to the hwmod fmwk.
>>>
>>> This sounds ok.
>>>
>>> I think the main question is how do we disable the standard DSS fclk
>>> from PRCM when using DSI PLL? As far as I know, disabling that clock
>>> will allow some areas of OMAP to be shut down even while DSS is working.
>>> So from power management point of view it sounds a needed feature.
>>
>> Yes, at least in theory, but considering that any use case that will
>> require the DSI PLL will use a LCD panel + backlight, or an OLED panel
>> that will consume 50 times more than the 186 MHz clock, I do not think
>> it is really needed.
>> Moreover, that clock is generated by the PER DPLL that will be always
>> enabled in most usecase because it does generate the UART, I2C and most
>> basic peripherals clocks. If we cannot gate the PER DPLL, there is no
>> saving to expect from gating the DSS fclk only.
>> Bottom-line is that there is no practical power saving to expect from
>> that mode.
>>
>>> If the clock is main_clk for the HWMOD, it sounds to me it's always
>>> enabled if the HWMOD is enabled?
>>
>> Yes, but that sounds to me a good trade off to avoid unnecessary
>> complexity in your driver or in the hwmod fmwk.
>
> Ok, if there are no real power savings there, then I agree, it's
> pointless to add that complexity.
>
> So how do we go forward in short term? I'd very much like to remove all
> the "silly" code from the DSS pm_runtime patch series caused by this
> opt_clock handling. Is it possible to get some kind of a temporary
> solution in the hwmod framework which would somehow solve this from DSS
> driver's point of view? A flag that causes hwmod fmwk to enable
> opt-clocks automatically? Or is it possible to have more than one
> mandatory clock?
Before doing that, could you maybe just try something to make OMAP4
looks a little bit more like OMAP3?
dss_fck -> ick
dss_dss_fck -> main_clk
That should ensure that both modulemode and the PRCM fclk will be
managed by pm_runtime.
I just did a basic patch for the first module, you should maybe change
some other entries.
Regards,
Benoit
---
diff --git a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
index 614d680..4dfd18a 100644
--- a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
+++ b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
@@ -1134,7 +1134,7 @@ static struct omap_hwmod_addr_space
omap44xx_dss_dma_addrs[] = {
static struct omap_hwmod_ocp_if omap44xx_l3_main_2__dss = {
.master = &omap44xx_l3_main_2_hwmod,
.slave = &omap44xx_dss_hwmod,
- .clk = "l3_div_ck",
+ .clk = "dss_fck",
.addr = omap44xx_dss_dma_addrs,
.addr_cnt = ARRAY_SIZE(omap44xx_dss_dma_addrs),
.user = OCP_USER_SDMA,
@@ -1167,14 +1167,13 @@ static struct omap_hwmod_ocp_if
*omap44xx_dss_slaves[] = {
static struct omap_hwmod_opt_clk dss_opt_clks[] = {
{ .role = "sys_clk", .clk = "dss_sys_clk" },
{ .role = "tv_clk", .clk = "dss_tv_clk" },
- { .role = "dss_clk", .clk = "dss_dss_clk" },
{ .role = "video_clk", .clk = "dss_48mhz_clk" },
};
static struct omap_hwmod omap44xx_dss_hwmod = {
.name = "dss_core",
.class = &omap44xx_dss_hwmod_class,
- .main_clk = "dss_fck",
+ .main_clk = "dss_dss_fck",
.prcm = {
.omap4 = {
.clkctrl_reg = OMAP4430_CM_DSS_DSS_CLKCTRL,
^ permalink raw reply related
* Re: [PATCH 21/29] s3fb: use display information in info not in var for panning
From: Laurent Pinchart @ 2011-06-06 16:16 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1306364301-8195-22-git-send-email-laurent.pinchart@ideasonboard.com>
Hi Tormod,
On Friday 03 June 2011 11:26:51 Tormod Volden wrote:
> On Thu, May 26, 2011 at 6:31 PM, Laurent Pinchart wrote:
> > On Thursday 26 May 2011 16:12:21 Tormod Volden wrote:
> >> On Thu, May 26, 2011 at 12:58 AM, Laurent Pinchart wrote:
> >> > We must not use any information in the passed var besides xoffset,
> >> > yoffset and vmode as otherwise applications might abuse it. Also use
> >> > the aligned fix.line_length and not the (possible) unaligned
> >> > xres_virtual.
> >> >
> >> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >> > Cc: Antonino Daplas <adaplas@gmail.com>
> >> > ---
> >> > drivers/video/savage/savagefb_driver.c | 16 +++++++---------
> >> > 1 files changed, 7 insertions(+), 9 deletions(-)
> >>
> >> The patch title is misleading, this is not the s3fb driver but the
> >> savagefb driver.
> >
> > Yes, sorry about that. I've fixed the patch title, as well as the next
> > patch.
>
> Hi Laurent,
> I haven't seen any updated patch posted,
That's because I haven't posted the updated patches yet :-) I was waiting for
more review.
> but anyway, the patch looks otherwise correct to me, and I have tested it on
> my Savage TwisterK:
>
> Reviewed-by: Tormod Volden <debian.tormod@gmail.com>
Thank you.
It's been a week and a half since I posted the first version, I'll now send an
updated one with correct titles and Acked-by/Reviewed-by lines.
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [Patch 2/2 resend] Prevent vga16fb from accessing hw after it
From: Bruno Prémont @ 2011-06-06 20:08 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <20110524223221.741ffbc0@neptune.home>
On Mon, 06 June 2011 Paul Mundt <lethal@linux-sh.org> wrote:
> On Tue, May 24, 2011 at 10:32:21PM +0200, Bruno Pr??mont wrote:
> > diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
> > index 5aac00e..bd9f93b 100644
> > --- a/drivers/video/fbmem.c
> > +++ b/drivers/video/fbmem.c
> > @@ -1661,6 +1661,11 @@ static int do_unregister_framebuffer(struct fb_info *fb_info)
> > device_destroy(fb_class, MKDEV(FB_MAJOR, i));
> > event.info = fb_info;
> > fb_notifier_call_chain(FB_EVENT_FB_UNREGISTERED, &event);
> > + if (fb_info->fbops->fb_unregistered) {
> > + mutex_lock(&fb_info->lock);
> > + fb_info->fbops->fb_unregistered(fb_info);
> > + mutex_unlock(&fb_info->lock);
> > + }
> >
> > /* this may free fb info */
> > put_fb_info(fb_info);
>
> I'm not sure I really see the point, given that you can already do all of
> the same work by tying in to the notifier chain. See for example the
> sh_mobile_hdmi driver and its unreg notifier.
You can but is it a good idea to hook the driver itself to notifier chain
and do the work to find out if the info it's being notified for is one it
cares about?
In addition, if driver gets informed via the notifier it's unknown if kernel
users or driver get notified first, thus fb driver cannot give all kernel
users opportunity to cleanup before cleaning-up itself.
At best notification order depends on loading order of modules for fb driver
and kernel fb user (like fbcon).
Bruno
^ permalink raw reply
* Re: [PATCH 19/27] OMAP: DSS2: Use PM runtime & HWMOD support
From: Tomi Valkeinen @ 2011-06-07 6:47 UTC (permalink / raw)
To: Cousson, Benoit
Cc: Hilman, Kevin, linux-omap@vger.kernel.org,
linux-fbdev@vger.kernel.org, paul@pwsan.com
In-Reply-To: <4DECCE90.6070201@ti.com>
On Mon, 2011-06-06 at 14:56 +0200, Cousson, Benoit wrote:
> That terminology in the PRCM just means that an opt clock will not be
> handled automatically by the PRCM and will require SW control.
> This is not the case for mandatory clock. Upon module enable the PRCM
> will ensure that all mandatory clocks (functional and interface) are
> enabled automagically. If the clock is marked as optional it means that
> the SW will have to enable it explicitly before enabling the module.
Is that correct? This would mean that whenever a hwmod has opt clock, it
needs to implement similar hack functions that are present in this
patch, to be able to enable the opt clock before enabling the hwmod, and
to disable the opt clock after disabling the hwmod.
I'd rather hope the optional clock could be enabled whenever the driver
needs it, between enabling and disabling the hwmod.
If it's required that the opt clocks are enabled before enabling the
hwmod, what is the point of having them as optional and driver
controlled? The hwmod fmwk could as well handle the opt clocks in that
case.
Tomi
^ permalink raw reply
* Re: [PATCH 19/27] OMAP: DSS2: Use PM runtime & HWMOD support
From: Tomi Valkeinen @ 2011-06-07 6:52 UTC (permalink / raw)
To: Cousson, Benoit
Cc: Hilman, Kevin, linux-omap@vger.kernel.org,
linux-fbdev@vger.kernel.org, paul@pwsan.com
In-Reply-To: <4DECF215.5020505@ti.com>
On Mon, 2011-06-06 at 17:28 +0200, Cousson, Benoit wrote:
> Before doing that, could you maybe just try something to make OMAP4
> looks a little bit more like OMAP3?
>
> dss_fck -> ick
> dss_dss_fck -> main_clk
>
> That should ensure that both modulemode and the PRCM fclk will be
> managed by pm_runtime.
I made the changes as you suggested, and while I haven't made the
changes to omapdss yet to see if I can remove the dispc_runtime_get/put
style function, I can boot up and start the dss.
However, after booting up but before enabling the dss driver, I can see
that the clock counts are:
dss_tv_clk 0
dss_sys_clk 0
dss_fck 7
dss_dss_clk 0
dss_48mhz_clk 0
So the modulemode is set for all dss hwmods? Isn't this exactly how it's
_not_ meant to be, as modulemode should be set only after enabling the
fck?
Tomi
diff --git a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
index b374cd0..d7d86b6 100644
--- a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
+++ b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
@@ -1133,7 +1133,7 @@ static struct omap_hwmod_addr_space omap44xx_dss_dma_addrs[] = {
static struct omap_hwmod_ocp_if omap44xx_l3_main_2__dss = {
.master = &omap44xx_l3_main_2_hwmod,
.slave = &omap44xx_dss_hwmod,
- .clk = "l3_div_ck",
+ .clk = "dss_fck",
.addr = omap44xx_dss_dma_addrs,
.addr_cnt = ARRAY_SIZE(omap44xx_dss_dma_addrs),
.user = OCP_USER_SDMA,
@@ -1170,7 +1170,7 @@ static struct omap_hwmod_opt_clk dss_opt_clks[] = {
static struct omap_hwmod omap44xx_dss_hwmod = {
.name = "dss_core",
.class = &omap44xx_dss_hwmod_class,
- .main_clk = "dss_fck",
+ .main_clk = "dss_dss_clk",
.prcm = {
.omap4 = {
.clkctrl_reg = OMAP4430_CM_DSS_DSS_CLKCTRL,
@@ -1230,7 +1230,7 @@ static struct omap_hwmod_addr_space omap44xx_dss_dispc_dma_addrs[] = {
static struct omap_hwmod_ocp_if omap44xx_l3_main_2__dss_dispc = {
.master = &omap44xx_l3_main_2_hwmod,
.slave = &omap44xx_dss_dispc_hwmod,
- .clk = "l3_div_ck",
+ .clk = "dss_fck",
.addr = omap44xx_dss_dispc_dma_addrs,
.addr_cnt = ARRAY_SIZE(omap44xx_dss_dispc_dma_addrs),
.user = OCP_USER_SDMA,
@@ -1279,7 +1279,7 @@ static struct omap_hwmod omap44xx_dss_dispc_hwmod = {
.mpu_irqs_cnt = ARRAY_SIZE(omap44xx_dss_dispc_irqs),
.sdma_reqs = omap44xx_dss_dispc_sdma_reqs,
.sdma_reqs_cnt = ARRAY_SIZE(omap44xx_dss_dispc_sdma_reqs),
- .main_clk = "dss_fck",
+ .main_clk = "dss_dss_clk",
.prcm = {
.omap4 = {
.clkctrl_reg = OMAP4430_CM_DSS_DSS_CLKCTRL,
@@ -1335,7 +1335,7 @@ static struct omap_hwmod_addr_space omap44xx_dss_dsi1_dma_addrs[] = {
static struct omap_hwmod_ocp_if omap44xx_l3_main_2__dss_dsi1 = {
.master = &omap44xx_l3_main_2_hwmod,
.slave = &omap44xx_dss_dsi1_hwmod,
- .clk = "l3_div_ck",
+ .clk = "dss_fck",
.addr = omap44xx_dss_dsi1_dma_addrs,
.addr_cnt = ARRAY_SIZE(omap44xx_dss_dsi1_dma_addrs),
.user = OCP_USER_SDMA,
@@ -1377,7 +1377,7 @@ static struct omap_hwmod omap44xx_dss_dsi1_hwmod = {
.mpu_irqs_cnt = ARRAY_SIZE(omap44xx_dss_dsi1_irqs),
.sdma_reqs = omap44xx_dss_dsi1_sdma_reqs,
.sdma_reqs_cnt = ARRAY_SIZE(omap44xx_dss_dsi1_sdma_reqs),
- .main_clk = "dss_fck",
+ .main_clk = "dss_dss_clk",
.prcm = {
.omap4 = {
.clkctrl_reg = OMAP4430_CM_DSS_DSS_CLKCTRL,
@@ -1412,7 +1412,7 @@ static struct omap_hwmod_addr_space omap44xx_dss_dsi2_dma_addrs[] = {
static struct omap_hwmod_ocp_if omap44xx_l3_main_2__dss_dsi2 = {
.master = &omap44xx_l3_main_2_hwmod,
.slave = &omap44xx_dss_dsi2_hwmod,
- .clk = "l3_div_ck",
+ .clk = "dss_fck",
.addr = omap44xx_dss_dsi2_dma_addrs,
.addr_cnt = ARRAY_SIZE(omap44xx_dss_dsi2_dma_addrs),
.user = OCP_USER_SDMA,
@@ -1449,7 +1449,7 @@ static struct omap_hwmod omap44xx_dss_dsi2_hwmod = {
.mpu_irqs_cnt = ARRAY_SIZE(omap44xx_dss_dsi2_irqs),
.sdma_reqs = omap44xx_dss_dsi2_sdma_reqs,
.sdma_reqs_cnt = ARRAY_SIZE(omap44xx_dss_dsi2_sdma_reqs),
- .main_clk = "dss_fck",
+ .main_clk = "dss_dss_clk",
.prcm = {
.omap4 = {
.clkctrl_reg = OMAP4430_CM_DSS_DSS_CLKCTRL,
@@ -1502,7 +1502,7 @@ static struct omap_hwmod_addr_space omap44xx_dss_hdmi_dma_addrs[] = {
static struct omap_hwmod_ocp_if omap44xx_l3_main_2__dss_hdmi = {
.master = &omap44xx_l3_main_2_hwmod,
.slave = &omap44xx_dss_hdmi_hwmod,
- .clk = "l3_div_ck",
+ .clk = "dss_fck",
.addr = omap44xx_dss_hdmi_dma_addrs,
.addr_cnt = ARRAY_SIZE(omap44xx_dss_hdmi_dma_addrs),
.user = OCP_USER_SDMA,
@@ -1544,7 +1544,7 @@ static struct omap_hwmod omap44xx_dss_hdmi_hwmod = {
.mpu_irqs_cnt = ARRAY_SIZE(omap44xx_dss_hdmi_irqs),
.sdma_reqs = omap44xx_dss_hdmi_sdma_reqs,
.sdma_reqs_cnt = ARRAY_SIZE(omap44xx_dss_hdmi_sdma_reqs),
- .main_clk = "dss_fck",
+ .main_clk = "dss_dss_clk",
.prcm = {
.omap4 = {
.clkctrl_reg = OMAP4430_CM_DSS_DSS_CLKCTRL,
@@ -1595,7 +1595,7 @@ static struct omap_hwmod_addr_space omap44xx_dss_rfbi_dma_addrs[] = {
static struct omap_hwmod_ocp_if omap44xx_l3_main_2__dss_rfbi = {
.master = &omap44xx_l3_main_2_hwmod,
.slave = &omap44xx_dss_rfbi_hwmod,
- .clk = "l3_div_ck",
+ .clk = "dss_fck",
.addr = omap44xx_dss_rfbi_dma_addrs,
.addr_cnt = ARRAY_SIZE(omap44xx_dss_rfbi_dma_addrs),
.user = OCP_USER_SDMA,
@@ -1634,7 +1634,7 @@ static struct omap_hwmod omap44xx_dss_rfbi_hwmod = {
.class = &omap44xx_rfbi_hwmod_class,
.sdma_reqs = omap44xx_dss_rfbi_sdma_reqs,
.sdma_reqs_cnt = ARRAY_SIZE(omap44xx_dss_rfbi_sdma_reqs),
- .main_clk = "dss_fck",
+ .main_clk = "dss_dss_clk",
.prcm = {
.omap4 = {
.clkctrl_reg = OMAP4430_CM_DSS_DSS_CLKCTRL,
@@ -1670,7 +1670,7 @@ static struct omap_hwmod_addr_space omap44xx_dss_venc_dma_addrs[] = {
static struct omap_hwmod_ocp_if omap44xx_l3_main_2__dss_venc = {
.master = &omap44xx_l3_main_2_hwmod,
.slave = &omap44xx_dss_venc_hwmod,
- .clk = "l3_div_ck",
+ .clk = "dss_fck",
.addr = omap44xx_dss_venc_dma_addrs,
.addr_cnt = ARRAY_SIZE(omap44xx_dss_venc_dma_addrs),
.user = OCP_USER_SDMA,
@@ -1707,7 +1707,7 @@ static struct omap_hwmod_opt_clk venc_opt_clks[] = {
static struct omap_hwmod omap44xx_dss_venc_hwmod = {
.name = "dss_venc",
.class = &omap44xx_venc_hwmod_class,
- .main_clk = "dss_fck",
+ .main_clk = "dss_dss_clk",
.prcm = {
.omap4 = {
.clkctrl_reg = OMAP4430_CM_DSS_DSS_CLKCTRL,
^ permalink raw reply related
* Re: [PATCH 19/27] OMAP: DSS2: Use PM runtime & HWMOD support
From: Cousson, Benoit @ 2011-06-07 7:12 UTC (permalink / raw)
To: Valkeinen, Tomi
Cc: Hilman, Kevin, linux-omap@vger.kernel.org,
linux-fbdev@vger.kernel.org, paul@pwsan.com
In-Reply-To: <1307429265.1858.6.camel@deskari>
On 6/7/2011 8:47 AM, Valkeinen, Tomi wrote:
> On Mon, 2011-06-06 at 14:56 +0200, Cousson, Benoit wrote:
>
>> That terminology in the PRCM just means that an opt clock will not be
>> handled automatically by the PRCM and will require SW control.
>> This is not the case for mandatory clock. Upon module enable the PRCM
>> will ensure that all mandatory clocks (functional and interface) are
>> enabled automagically. If the clock is marked as optional it means that
>> the SW will have to enable it explicitly before enabling the module.
>
> Is that correct? This would mean that whenever a hwmod has opt clock, it
> needs to implement similar hack functions that are present in this
> patch, to be able to enable the opt clock before enabling the hwmod, and
> to disable the opt clock after disabling the hwmod.
No, because most hwmods with opt_clock does have a real main_clk as
well. In the case of the GPIO, the driver need to enable the opt clock
only if the debounce feature is needed.
In general we always have one main functional clock to enable the module
first.
> I'd rather hope the optional clock could be enabled whenever the driver
> needs it, between enabling and disabling the hwmod.
Yeah, this is the case most of the time, except for you.
> If it's required that the opt clocks are enabled before enabling the
> hwmod, what is the point of having them as optional and driver
> controlled? The hwmod fmwk could as well handle the opt clocks in that
> case.
There is no point... It is just due to the particular clock setting
required by the DSS. That specific case was simply not taken into
account originally. You are just the first one to hit that issue :-(
Just because the DSS can choose its main functional clock, the HW team
decided to mark them all as opt clock, in order to let the SW decide
which one to use.
Regards,
Benoit
^ permalink raw reply
* Re: [PATCH 19/27] OMAP: DSS2: Use PM runtime & HWMOD support
From: Tomi Valkeinen @ 2011-06-07 7:21 UTC (permalink / raw)
To: Cousson, Benoit
Cc: Hilman, Kevin, linux-omap@vger.kernel.org,
linux-fbdev@vger.kernel.org, paul@pwsan.com
In-Reply-To: <4DEDCF73.9030607@ti.com>
On Tue, 2011-06-07 at 09:12 +0200, Cousson, Benoit wrote:
> On 6/7/2011 8:47 AM, Valkeinen, Tomi wrote:
> > I'd rather hope the optional clock could be enabled whenever the driver
> > needs it, between enabling and disabling the hwmod.
>
> Yeah, this is the case most of the time, except for you.
Are you talking only about the DSS_FCLK opt-clock from PRCM, or all DSS
opt clocks (sys clk, hdmi clk, tv clk, dac clock)?
I hope the rest of the opt clocks can be enabled later. Although I guess
all/many of them will be needed during reset, but that should be already
handled by the hwmod fmwk.
Tomi
^ permalink raw reply
* Re: [PATCH 19/27] OMAP: DSS2: Use PM runtime & HWMOD support
From: Cousson, Benoit @ 2011-06-07 7:27 UTC (permalink / raw)
To: Valkeinen, Tomi
Cc: Hilman, Kevin, linux-omap@vger.kernel.org,
linux-fbdev@vger.kernel.org, paul@pwsan.com
In-Reply-To: <1307431291.1858.15.camel@deskari>
On 6/7/2011 9:21 AM, Valkeinen, Tomi wrote:
> On Tue, 2011-06-07 at 09:12 +0200, Cousson, Benoit wrote:
>> On 6/7/2011 8:47 AM, Valkeinen, Tomi wrote:
>
>>> I'd rather hope the optional clock could be enabled whenever the driver
>>> needs it, between enabling and disabling the hwmod.
>>
>> Yeah, this is the case most of the time, except for you.
>
> Are you talking only about the DSS_FCLK opt-clock from PRCM, or all DSS
> opt clocks (sys clk, hdmi clk, tv clk, dac clock)?
>
> I hope the rest of the opt clocks can be enabled later. Although I guess
> all/many of them will be needed during reset, but that should be already
> handled by the hwmod fmwk.
Yes, sorry, for the confusion, but the point is that they all look the
same to me :-)
The PRCM does not make any difference for any of these opt_clock, they
are all under SW control.
Except that one of them must be enabled because internally it is used as
a functional clock.
Benoit
^ permalink raw reply
* Re: [PATCH 19/27] OMAP: DSS2: Use PM runtime & HWMOD support
From: Tomi Valkeinen @ 2011-06-07 9:08 UTC (permalink / raw)
To: Cousson, Benoit
Cc: Hilman, Kevin, linux-omap@vger.kernel.org,
linux-fbdev@vger.kernel.org, paul@pwsan.com
In-Reply-To: <1307429547.1858.10.camel@deskari>
On Tue, 2011-06-07 at 09:52 +0300, Tomi Valkeinen wrote:
> On Mon, 2011-06-06 at 17:28 +0200, Cousson, Benoit wrote:
>
> > Before doing that, could you maybe just try something to make OMAP4
> > looks a little bit more like OMAP3?
> >
> > dss_fck -> ick
> > dss_dss_fck -> main_clk
> >
> > That should ensure that both modulemode and the PRCM fclk will be
> > managed by pm_runtime.
>
> I made the changes as you suggested, and while I haven't made the
> changes to omapdss yet to see if I can remove the dispc_runtime_get/put
> style function, I can boot up and start the dss.
>
> However, after booting up but before enabling the dss driver, I can see
> that the clock counts are:
>
> dss_tv_clk 0
> dss_sys_clk 0
> dss_fck 7
> dss_dss_clk 0
> dss_48mhz_clk 0
>
> So the modulemode is set for all dss hwmods? Isn't this exactly how it's
> _not_ meant to be, as modulemode should be set only after enabling the
> fck?
This also seems to keep the DSS from going to RET or OFF, at least in
the TI internal PM testing tree.
So is the PM side buggy there, and it shouldn't care about the
modulemode being enabled if other clocks are off, or is it the hwmod
side that's buggy, and it should disable the modulemode also?
Tomi
^ permalink raw reply
* Re: [PATCH 19/27] OMAP: DSS2: Use PM runtime & HWMOD support
From: Cousson, Benoit @ 2011-06-07 11:37 UTC (permalink / raw)
To: Valkeinen, Tomi
Cc: Hilman, Kevin, linux-omap@vger.kernel.org,
linux-fbdev@vger.kernel.org, paul@pwsan.com
In-Reply-To: <1307429547.1858.10.camel@deskari>
On 6/7/2011 8:52 AM, Valkeinen, Tomi wrote:
> On Mon, 2011-06-06 at 17:28 +0200, Cousson, Benoit wrote:
>
>> Before doing that, could you maybe just try something to make OMAP4
>> looks a little bit more like OMAP3?
>>
>> dss_fck -> ick
>> dss_dss_fck -> main_clk
>>
>> That should ensure that both modulemode and the PRCM fclk will be
>> managed by pm_runtime.
>
> I made the changes as you suggested, and while I haven't made the
> changes to omapdss yet to see if I can remove the dispc_runtime_get/put
> style function, I can boot up and start the dss.
>
> However, after booting up but before enabling the dss driver, I can see
> that the clock counts are:
>
> dss_tv_clk 0
> dss_sys_clk 0
> dss_fck 7
> dss_dss_clk 0
> dss_48mhz_clk 0
>
> So the modulemode is set for all dss hwmods? Isn't this exactly how it's
> _not_ meant to be, as modulemode should be set only after enabling the
> fck?
The issue is that there is only one modulemode for the whole DSS.
Potentially only the dss_hwmod should have it. But then you have to
ensure that this device is enabled before any other DSS devices.
If you cannot do that at your level, we will have to set a hwmod
dependency between DSS modules and the main DSS subsystem.
For the moment we do not have such HW dependencies.
Benoit
>
> Tomi
>
>
> diff --git a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
> index b374cd0..d7d86b6 100644
> --- a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
> +++ b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
> @@ -1133,7 +1133,7 @@ static struct omap_hwmod_addr_space omap44xx_dss_dma_addrs[] = {
> static struct omap_hwmod_ocp_if omap44xx_l3_main_2__dss = {
> .master =&omap44xx_l3_main_2_hwmod,
> .slave =&omap44xx_dss_hwmod,
> - .clk = "l3_div_ck",
> + .clk = "dss_fck",
> .addr = omap44xx_dss_dma_addrs,
> .addr_cnt = ARRAY_SIZE(omap44xx_dss_dma_addrs),
> .user = OCP_USER_SDMA,
> @@ -1170,7 +1170,7 @@ static struct omap_hwmod_opt_clk dss_opt_clks[] = {
> static struct omap_hwmod omap44xx_dss_hwmod = {
> .name = "dss_core",
> .class =&omap44xx_dss_hwmod_class,
> - .main_clk = "dss_fck",
> + .main_clk = "dss_dss_clk",
> .prcm = {
> .omap4 = {
> .clkctrl_reg = OMAP4430_CM_DSS_DSS_CLKCTRL,
> @@ -1230,7 +1230,7 @@ static struct omap_hwmod_addr_space omap44xx_dss_dispc_dma_addrs[] = {
> static struct omap_hwmod_ocp_if omap44xx_l3_main_2__dss_dispc = {
> .master =&omap44xx_l3_main_2_hwmod,
> .slave =&omap44xx_dss_dispc_hwmod,
> - .clk = "l3_div_ck",
> + .clk = "dss_fck",
> .addr = omap44xx_dss_dispc_dma_addrs,
> .addr_cnt = ARRAY_SIZE(omap44xx_dss_dispc_dma_addrs),
> .user = OCP_USER_SDMA,
> @@ -1279,7 +1279,7 @@ static struct omap_hwmod omap44xx_dss_dispc_hwmod = {
> .mpu_irqs_cnt = ARRAY_SIZE(omap44xx_dss_dispc_irqs),
> .sdma_reqs = omap44xx_dss_dispc_sdma_reqs,
> .sdma_reqs_cnt = ARRAY_SIZE(omap44xx_dss_dispc_sdma_reqs),
> - .main_clk = "dss_fck",
> + .main_clk = "dss_dss_clk",
> .prcm = {
> .omap4 = {
> .clkctrl_reg = OMAP4430_CM_DSS_DSS_CLKCTRL,
> @@ -1335,7 +1335,7 @@ static struct omap_hwmod_addr_space omap44xx_dss_dsi1_dma_addrs[] = {
> static struct omap_hwmod_ocp_if omap44xx_l3_main_2__dss_dsi1 = {
> .master =&omap44xx_l3_main_2_hwmod,
> .slave =&omap44xx_dss_dsi1_hwmod,
> - .clk = "l3_div_ck",
> + .clk = "dss_fck",
> .addr = omap44xx_dss_dsi1_dma_addrs,
> .addr_cnt = ARRAY_SIZE(omap44xx_dss_dsi1_dma_addrs),
> .user = OCP_USER_SDMA,
> @@ -1377,7 +1377,7 @@ static struct omap_hwmod omap44xx_dss_dsi1_hwmod = {
> .mpu_irqs_cnt = ARRAY_SIZE(omap44xx_dss_dsi1_irqs),
> .sdma_reqs = omap44xx_dss_dsi1_sdma_reqs,
> .sdma_reqs_cnt = ARRAY_SIZE(omap44xx_dss_dsi1_sdma_reqs),
> - .main_clk = "dss_fck",
> + .main_clk = "dss_dss_clk",
> .prcm = {
> .omap4 = {
> .clkctrl_reg = OMAP4430_CM_DSS_DSS_CLKCTRL,
> @@ -1412,7 +1412,7 @@ static struct omap_hwmod_addr_space omap44xx_dss_dsi2_dma_addrs[] = {
> static struct omap_hwmod_ocp_if omap44xx_l3_main_2__dss_dsi2 = {
> .master =&omap44xx_l3_main_2_hwmod,
> .slave =&omap44xx_dss_dsi2_hwmod,
> - .clk = "l3_div_ck",
> + .clk = "dss_fck",
> .addr = omap44xx_dss_dsi2_dma_addrs,
> .addr_cnt = ARRAY_SIZE(omap44xx_dss_dsi2_dma_addrs),
> .user = OCP_USER_SDMA,
> @@ -1449,7 +1449,7 @@ static struct omap_hwmod omap44xx_dss_dsi2_hwmod = {
> .mpu_irqs_cnt = ARRAY_SIZE(omap44xx_dss_dsi2_irqs),
> .sdma_reqs = omap44xx_dss_dsi2_sdma_reqs,
> .sdma_reqs_cnt = ARRAY_SIZE(omap44xx_dss_dsi2_sdma_reqs),
> - .main_clk = "dss_fck",
> + .main_clk = "dss_dss_clk",
> .prcm = {
> .omap4 = {
> .clkctrl_reg = OMAP4430_CM_DSS_DSS_CLKCTRL,
> @@ -1502,7 +1502,7 @@ static struct omap_hwmod_addr_space omap44xx_dss_hdmi_dma_addrs[] = {
> static struct omap_hwmod_ocp_if omap44xx_l3_main_2__dss_hdmi = {
> .master =&omap44xx_l3_main_2_hwmod,
> .slave =&omap44xx_dss_hdmi_hwmod,
> - .clk = "l3_div_ck",
> + .clk = "dss_fck",
> .addr = omap44xx_dss_hdmi_dma_addrs,
> .addr_cnt = ARRAY_SIZE(omap44xx_dss_hdmi_dma_addrs),
> .user = OCP_USER_SDMA,
> @@ -1544,7 +1544,7 @@ static struct omap_hwmod omap44xx_dss_hdmi_hwmod = {
> .mpu_irqs_cnt = ARRAY_SIZE(omap44xx_dss_hdmi_irqs),
> .sdma_reqs = omap44xx_dss_hdmi_sdma_reqs,
> .sdma_reqs_cnt = ARRAY_SIZE(omap44xx_dss_hdmi_sdma_reqs),
> - .main_clk = "dss_fck",
> + .main_clk = "dss_dss_clk",
> .prcm = {
> .omap4 = {
> .clkctrl_reg = OMAP4430_CM_DSS_DSS_CLKCTRL,
> @@ -1595,7 +1595,7 @@ static struct omap_hwmod_addr_space omap44xx_dss_rfbi_dma_addrs[] = {
> static struct omap_hwmod_ocp_if omap44xx_l3_main_2__dss_rfbi = {
> .master =&omap44xx_l3_main_2_hwmod,
> .slave =&omap44xx_dss_rfbi_hwmod,
> - .clk = "l3_div_ck",
> + .clk = "dss_fck",
> .addr = omap44xx_dss_rfbi_dma_addrs,
> .addr_cnt = ARRAY_SIZE(omap44xx_dss_rfbi_dma_addrs),
> .user = OCP_USER_SDMA,
> @@ -1634,7 +1634,7 @@ static struct omap_hwmod omap44xx_dss_rfbi_hwmod = {
> .class =&omap44xx_rfbi_hwmod_class,
> .sdma_reqs = omap44xx_dss_rfbi_sdma_reqs,
> .sdma_reqs_cnt = ARRAY_SIZE(omap44xx_dss_rfbi_sdma_reqs),
> - .main_clk = "dss_fck",
> + .main_clk = "dss_dss_clk",
> .prcm = {
> .omap4 = {
> .clkctrl_reg = OMAP4430_CM_DSS_DSS_CLKCTRL,
> @@ -1670,7 +1670,7 @@ static struct omap_hwmod_addr_space omap44xx_dss_venc_dma_addrs[] = {
> static struct omap_hwmod_ocp_if omap44xx_l3_main_2__dss_venc = {
> .master =&omap44xx_l3_main_2_hwmod,
> .slave =&omap44xx_dss_venc_hwmod,
> - .clk = "l3_div_ck",
> + .clk = "dss_fck",
> .addr = omap44xx_dss_venc_dma_addrs,
> .addr_cnt = ARRAY_SIZE(omap44xx_dss_venc_dma_addrs),
> .user = OCP_USER_SDMA,
> @@ -1707,7 +1707,7 @@ static struct omap_hwmod_opt_clk venc_opt_clks[] = {
> static struct omap_hwmod omap44xx_dss_venc_hwmod = {
> .name = "dss_venc",
> .class =&omap44xx_venc_hwmod_class,
> - .main_clk = "dss_fck",
> + .main_clk = "dss_dss_clk",
> .prcm = {
> .omap4 = {
> .clkctrl_reg = OMAP4430_CM_DSS_DSS_CLKCTRL,
>
>
^ permalink raw reply
* Re: [PATCH 19/27] OMAP: DSS2: Use PM runtime & HWMOD support
From: Tomi Valkeinen @ 2011-06-07 11:51 UTC (permalink / raw)
To: Cousson, Benoit
Cc: Hilman, Kevin, linux-omap@vger.kernel.org,
linux-fbdev@vger.kernel.org, paul@pwsan.com
In-Reply-To: <4DEE0D7E.6090309@ti.com>
On Tue, 2011-06-07 at 13:37 +0200, Cousson, Benoit wrote:
> On 6/7/2011 8:52 AM, Valkeinen, Tomi wrote:
> > On Mon, 2011-06-06 at 17:28 +0200, Cousson, Benoit wrote:
> >
> >> Before doing that, could you maybe just try something to make OMAP4
> >> looks a little bit more like OMAP3?
> >>
> >> dss_fck -> ick
> >> dss_dss_fck -> main_clk
> >>
> >> That should ensure that both modulemode and the PRCM fclk will be
> >> managed by pm_runtime.
> >
> > I made the changes as you suggested, and while I haven't made the
> > changes to omapdss yet to see if I can remove the dispc_runtime_get/put
> > style function, I can boot up and start the dss.
> >
> > However, after booting up but before enabling the dss driver, I can see
> > that the clock counts are:
> >
> > dss_tv_clk 0
> > dss_sys_clk 0
> > dss_fck 7
> > dss_dss_clk 0
> > dss_48mhz_clk 0
> >
> > So the modulemode is set for all dss hwmods? Isn't this exactly how it's
> > _not_ meant to be, as modulemode should be set only after enabling the
> > fck?
>
> The issue is that there is only one modulemode for the whole DSS.
> Potentially only the dss_hwmod should have it. But then you have to
> ensure that this device is enabled before any other DSS devices.
Does that change anything? Isn't the above (modulemode enabled before
opt clock) still true, even if it was enabled only once for the dss_core
hwmod?
And for PM, it doesn't matter if the dss_fck is enabled once or seven
times, I presume a use count of one will still prevent RET or OFF?
> If you cannot do that at your level, we will have to set a hwmod
> dependency between DSS modules and the main DSS subsystem.
> For the moment we do not have such HW dependencies.
I can do this in the driver, and in fact I already do. The dss_core
hwmod is enabled by all the other hwmods before they do anything.
My reasoning for this dependency is that the dss_core contains for
example the clock mux registers, and other misc registers used by most
other dss modules. But I'm not sure if this dependency should be in the
hwmod level or not.
Tomi
^ permalink raw reply
* Re: [PATCH 19/27] OMAP: DSS2: Use PM runtime & HWMOD support
From: Cousson, Benoit @ 2011-06-07 16:43 UTC (permalink / raw)
To: Valkeinen, Tomi
Cc: Hilman, Kevin, linux-omap@vger.kernel.org,
linux-fbdev@vger.kernel.org, paul@pwsan.com
In-Reply-To: <1307447504.1858.37.camel@deskari>
On 6/7/2011 1:51 PM, Valkeinen, Tomi wrote:
> On Tue, 2011-06-07 at 13:37 +0200, Cousson, Benoit wrote:
>> On 6/7/2011 8:52 AM, Valkeinen, Tomi wrote:
>>> On Mon, 2011-06-06 at 17:28 +0200, Cousson, Benoit wrote:
>>>
>>>> Before doing that, could you maybe just try something to make OMAP4
>>>> looks a little bit more like OMAP3?
>>>>
>>>> dss_fck -> ick
>>>> dss_dss_fck -> main_clk
>>>>
>>>> That should ensure that both modulemode and the PRCM fclk will be
>>>> managed by pm_runtime.
>>>
>>> I made the changes as you suggested, and while I haven't made the
>>> changes to omapdss yet to see if I can remove the dispc_runtime_get/put
>>> style function, I can boot up and start the dss.
>>>
>>> However, after booting up but before enabling the dss driver, I can see
>>> that the clock counts are:
>>>
>>> dss_tv_clk 0
>>> dss_sys_clk 0
>>> dss_fck 7
>>> dss_dss_clk 0
>>> dss_48mhz_clk 0
>>>
>>> So the modulemode is set for all dss hwmods? Isn't this exactly how it's
>>> _not_ meant to be, as modulemode should be set only after enabling the
>>> fck?
>>
>> The issue is that there is only one modulemode for the whole DSS.
>> Potentially only the dss_hwmod should have it. But then you have to
>> ensure that this device is enabled before any other DSS devices.
>
> Does that change anything? Isn't the above (modulemode enabled before
> opt clock) still true, even if it was enabled only once for the dss_core
> hwmod?
It does not really change anything, but it is more accurate.
Modulemode need to be enable after the opt clocks that act as a
functional clock and before enabling HW_AUTO for the clockdomain.
The important parameter is the clock domain mode change. It is another
issue that we have to fix. It might not affect you for the moment.
> And for PM, it doesn't matter if the dss_fck is enabled once or seven
> times, I presume a use count of one will still prevent RET or OFF?
>
>> If you cannot do that at your level, we will have to set a hwmod
>> dependency between DSS modules and the main DSS subsystem.
>> For the moment we do not have such HW dependencies.
>
> I can do this in the driver, and in fact I already do. The dss_core
> hwmod is enabled by all the other hwmods before they do anything.
>
> My reasoning for this dependency is that the dss_core contains for
> example the clock mux registers, and other misc registers used by most
> other dss modules. But I'm not sure if this dependency should be in the
> hwmod level or not.
It makes sense for me as well to have that dependency between drivers.
Having it for hwmod for my point of view will make the hwmod state out
of sync with the driver that manage it potentially. That kind of hard
coded dependencies at hwmod level should maybe be considered only if the
dependent hwmod does not belong to any driver.
Benoit
^ permalink raw reply
* Re: [PATCH 19/27] OMAP: DSS2: Use PM runtime & HWMOD support
From: Tomi Valkeinen @ 2011-06-08 7:55 UTC (permalink / raw)
To: Cousson, Benoit
Cc: Hilman, Kevin, linux-omap@vger.kernel.org,
linux-fbdev@vger.kernel.org, paul@pwsan.com
In-Reply-To: <4DEE5537.7060008@ti.com>
On Tue, 2011-06-07 at 18:43 +0200, Cousson, Benoit wrote:
> On 6/7/2011 1:51 PM, Valkeinen, Tomi wrote:
> > On Tue, 2011-06-07 at 13:37 +0200, Cousson, Benoit wrote:
> >> The issue is that there is only one modulemode for the whole DSS.
> >> Potentially only the dss_hwmod should have it. But then you have to
> >> ensure that this device is enabled before any other DSS devices.
> >
> > Does that change anything? Isn't the above (modulemode enabled before
> > opt clock) still true, even if it was enabled only once for the dss_core
> > hwmod?
>
> It does not really change anything, but it is more accurate.
> Modulemode need to be enable after the opt clocks that act as a
> functional clock and before enabling HW_AUTO for the clockdomain.
>
> The important parameter is the clock domain mode change. It is another
> issue that we have to fix. It might not affect you for the moment.
Ok. But the main issue now is the PM. If I change the clocks in hwmod
data as you suggested, dss_fck will always stay enabled and prevent RET
and OFF. So the fix is not acceptable even for temporary use.
So is there some way to fix this, or shall we just go forward with the
current patch series having the somewhat hacky way to use pm_runtime?
I would personally like to get the driver right from the start, even if
that means more hacks in the hwmod fmwk (because that's where the
problems are). But if that is very difficult, I'm fine with the current
patch series.
Tomi
^ permalink raw reply
* [PATCH 1/3] video: s3c-fb: fix misleading kfree in remove function
From: Jingoo Han @ 2011-06-08 10:43 UTC (permalink / raw)
To: linux-fbdev
This patch fixes misleading kfree in remove function.
Signed-off-by: Jingoo Han <jg1.han@samsung.com>
---
drivers/video/s3c-fb.c | 3 +--
1 files changed, 1 insertions(+), 2 deletions(-)
diff --git a/drivers/video/s3c-fb.c b/drivers/video/s3c-fb.c
index 0352afa..148e19d 100644
--- a/drivers/video/s3c-fb.c
+++ b/drivers/video/s3c-fb.c
@@ -1487,11 +1487,10 @@ static int __devexit s3c_fb_remove(struct platform_device *pdev)
release_mem_region(sfb->regs_res->start, resource_size(sfb->regs_res));
- kfree(sfb);
-
pm_runtime_put_sync(sfb->dev);
pm_runtime_disable(sfb->dev);
+ kfree(sfb);
return 0;
}
--
1.7.1
^ permalink raw reply related
* [PATCH] video: s3c-fb: fix virtual resolution checking
From: Jingoo Han @ 2011-06-08 10:43 UTC (permalink / raw)
To: linux-fbdev
This patch fixes mishandling in virtual resolution checking.
Previously, virtual resolution is changed to virtual_x and virtual_y
which mean the size for buffer allocation, when s3c_fb_check_var is
called by fb_check_var. However, it is meaningless, since virtual_x
and virtual_y are fixed and user cannot change virtual resolution.
Therefore, virtual resolution should be more than resolution
such as xres and yres.
Signed-off-by: Jingoo Han <jg1.han@samsung.com>
---
drivers/video/s3c-fb.c | 5 ++---
1 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/video/s3c-fb.c b/drivers/video/s3c-fb.c
index 0352afa..e48129a 100644
--- a/drivers/video/s3c-fb.c
+++ b/drivers/video/s3c-fb.c
@@ -235,13 +235,12 @@ static int s3c_fb_check_var(struct fb_var_screeninfo *var,
struct fb_info *info)
{
struct s3c_fb_win *win = info->par;
- struct s3c_fb_pd_win *windata = win->windata;
struct s3c_fb *sfb = win->parent;
dev_dbg(sfb->dev, "checking parameters\n");
- var->xres_virtual = max((unsigned int)windata->virtual_x, var->xres);
- var->yres_virtual = max((unsigned int)windata->virtual_y, var->yres);
+ var->xres_virtual = max(var->xres_virtual, var->xres);
+ var->yres_virtual = max(var->yres_virtual, var->yres);
if (!s3c_fb_validate_win_bpp(win, var->bits_per_pixel)) {
dev_dbg(sfb->dev, "win %d: unsupported bpp %d\n",
--
1.7.1
^ permalink raw reply related
* [PATCH] video: s3c-fb: move enabling channel for window
From: Jingoo Han @ 2011-06-08 10:43 UTC (permalink / raw)
To: linux-fbdev
This patch moves enabling channel for window, because there should
be enabling channel before enabling window. If the sequence is
reversed, it makes the problem in displaying images to lcd panel.
Signed-off-by: Jingoo Han <jg1.han@samsung.com>
---
drivers/video/s3c-fb.c | 14 +++++++-------
1 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/video/s3c-fb.c b/drivers/video/s3c-fb.c
index 0352afa..4f1bc39 100644
--- a/drivers/video/s3c-fb.c
+++ b/drivers/video/s3c-fb.c
@@ -558,6 +558,13 @@ static int s3c_fb_set_par(struct fb_info *info)
vidosd_set_alpha(win, alpha);
vidosd_set_size(win, data);
+ /* Enable DMA channel for this window */
+ if (sfb->variant.has_shadowcon) {
+ data = readl(sfb->regs + SHADOWCON);
+ data |= SHADOWCON_CHx_ENABLE(win_no);
+ writel(data, sfb->regs + SHADOWCON);
+ }
+
data = WINCONx_ENWIN;
/* note, since we have to round up the bits-per-pixel, we end up
@@ -637,13 +644,6 @@ static int s3c_fb_set_par(struct fb_info *info)
writel(data, regs + sfb->variant.wincon + (win_no * 4));
writel(0x0, regs + sfb->variant.winmap + (win_no * 4));
- /* Enable DMA channel for this window */
- if (sfb->variant.has_shadowcon) {
- data = readl(sfb->regs + SHADOWCON);
- data |= SHADOWCON_CHx_ENABLE(win_no);
- writel(data, sfb->regs + SHADOWCON);
- }
-
shadow_protect_win(win, 0);
return 0;
--
1.7.1
^ permalink raw reply related
* [PATCH] video: s3c-fb: fix misleading kfree in remove function
From: Jingoo Han @ 2011-06-08 10:50 UTC (permalink / raw)
To: linux-fbdev
This patch fixes misleading kfree in remove function.
Signed-off-by: Jingoo Han <jg1.han@samsung.com>
---
drivers/video/s3c-fb.c | 3 +--
1 files changed, 1 insertions(+), 2 deletions(-)
diff --git a/drivers/video/s3c-fb.c b/drivers/video/s3c-fb.c
index 0352afa..148e19d 100644
--- a/drivers/video/s3c-fb.c
+++ b/drivers/video/s3c-fb.c
@@ -1487,11 +1487,10 @@ static int __devexit s3c_fb_remove(struct platform_device *pdev)
release_mem_region(sfb->regs_res->start, resource_size(sfb->regs_res));
- kfree(sfb);
-
pm_runtime_put_sync(sfb->dev);
pm_runtime_disable(sfb->dev);
+ kfree(sfb);
return 0;
}
--
1.7.1
^ permalink raw reply related
* Re: [PATCH 1/3] video: s3c-fb: fix misleading kfree in remove function
From: JinGoo Han @ 2011-06-08 10:58 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1307529797-27153-1-git-send-email-jg1.han@samsung.com>
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="windows-1254", Size: 447 bytes --]
> ------- Original Message -------
> Sender : JinGoo Han<jg1.han@samsung.com>
> Date : Jun 08, 2011 19:43 (GMT+09:00)
> Title : [PATCH 1/3] video: s3c-fb: fix misleading kfree in remove function
Sorry. [PATCH 1/3] is typo.
I have just sent the patch '[PATCH] video: s3c-fb: fix misleading kfree in remove function'.
ÿôèº{.nÇ+‰·Ÿ®‰†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±ýöÝzÿâžØ^n‡r¡ö¦zË\x1aëh™¨èÚ&£ûàz¿äz¹Þ—ú+€Ê+zf£¢·hšˆ§~††Ûiÿÿïêÿ‘êçz_è®\x0fæj:+v‰¨þ)ߣøm
^ permalink raw reply
* Re: [PATCH 19/27] OMAP: DSS2: Use PM runtime & HWMOD support
From: Cousson, Benoit @ 2011-06-08 20:39 UTC (permalink / raw)
To: Valkeinen, Tomi
Cc: Hilman, Kevin, linux-omap@vger.kernel.org,
linux-fbdev@vger.kernel.org, paul@pwsan.com
In-Reply-To: <1307519752.1944.21.camel@deskari>
On 6/8/2011 9:55 AM, Valkeinen, Tomi wrote:
> On Tue, 2011-06-07 at 18:43 +0200, Cousson, Benoit wrote:
>> On 6/7/2011 1:51 PM, Valkeinen, Tomi wrote:
>>> On Tue, 2011-06-07 at 13:37 +0200, Cousson, Benoit wrote:
>
>>>> The issue is that there is only one modulemode for the whole DSS.
>>>> Potentially only the dss_hwmod should have it. But then you have to
>>>> ensure that this device is enabled before any other DSS devices.
>>>
>>> Does that change anything? Isn't the above (modulemode enabled before
>>> opt clock) still true, even if it was enabled only once for the dss_core
>>> hwmod?
>>
>> It does not really change anything, but it is more accurate.
>> Modulemode need to be enable after the opt clocks that act as a
>> functional clock and before enabling HW_AUTO for the clockdomain.
>>
>> The important parameter is the clock domain mode change. It is another
>> issue that we have to fix. It might not affect you for the moment.
>
> Ok. But the main issue now is the PM. If I change the clocks in hwmod
> data as you suggested, dss_fck will always stay enabled and prevent RET
> and OFF. So the fix is not acceptable even for temporary use.
Mmm, the issue is probably due to the way we are managing interface
clock that are usually able to autoidle. Because of that the
_disable_clocks function will not try to disable an interface clock
except if you use the following flag: OCPIF_SWSUP_IDLE.
static struct omap_hwmod_ocp_if omap44xx_l3_main_2__dss = {
.master = &omap44xx_l3_main_2_hwmod,
.slave = &omap44xx_dss_hwmod,
.clk = "dss_fck",
.addr = omap44xx_dss_dma_addrs,
.addr_cnt = ARRAY_SIZE(omap44xx_dss_dma_addrs),
.user = OCP_USER_SDMA,
+ .flags = OCPIF_SWSUP_IDLE,
>
> So is there some way to fix this, or shall we just go forward with the
> current patch series having the somewhat hacky way to use pm_runtime?
>
> I would personally like to get the driver right from the start, even if
> that means more hacks in the hwmod fmwk (because that's where the
> problems are). But if that is very difficult, I'm fine with the current
> patch series.
I hope we'll be able to fix the fmwk for 3.0.1.
Benoit
^ permalink raw reply
* [RFC] fbmem: reset file->private_data on failed fb_open()
From: Wu Fengguang @ 2011-06-09 3:06 UTC (permalink / raw)
To: Antonino A. Daplas; +Cc: Andrew Morton, linux-fbdev, LKML
I wrote this when looking at NULL dereference bug
https://bugzilla.kernel.org/show_bug.cgi?id\x18912
Will it help by clearing private_data? I have no idea at all, because
for regular files, ->release won't be called on failed ->open. Just in
case there are some exceptions in fbmem...
---
drivers/video/fbmem.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
--- linux-next.orig/drivers/video/fbmem.c 2011-06-09 10:36:06.000000000 +0800
+++ linux-next/drivers/video/fbmem.c 2011-06-09 10:39:30.000000000 +0800
@@ -1424,26 +1424,28 @@ __releases(&info->lock)
file->private_data = info;
if (info->fbops->fb_open) {
res = info->fbops->fb_open(info,1);
if (res)
module_put(info->fbops->owner);
}
#ifdef CONFIG_FB_DEFERRED_IO
if (info->fbdefio)
fb_deferred_io_open(info, inode, file);
#endif
out:
mutex_unlock(&info->lock);
- if (res)
+ if (res) {
+ file->private_data = NULL;
put_fb_info(info);
+ }
return res;
}
static int
fb_release(struct inode *inode, struct file *file)
__acquires(&info->lock)
__releases(&info->lock)
{
struct fb_info * const info = file->private_data;
mutex_lock(&info->lock);
if (info->fbops->fb_release)
^ permalink raw reply
* [PATCH] [resend] video: s3c-fb: fix misleading kfree in remove function
From: Jingoo Han @ 2011-06-09 4:26 UTC (permalink / raw)
To: linux-fbdev
This patch fixes misleading kfree in remove function.
Signed-off-by: Jingoo Han <jg1.han@samsung.com>
---
drivers/video/s3c-fb.c | 3 +--
1 files changed, 1 insertions(+), 2 deletions(-)
diff --git a/drivers/video/s3c-fb.c b/drivers/video/s3c-fb.c
index 0352afa..148e19d 100644
--- a/drivers/video/s3c-fb.c
+++ b/drivers/video/s3c-fb.c
@@ -1487,11 +1487,10 @@ static int __devexit s3c_fb_remove(struct platform_device *pdev)
release_mem_region(sfb->regs_res->start, resource_size(sfb->regs_res));
- kfree(sfb);
-
pm_runtime_put_sync(sfb->dev);
pm_runtime_disable(sfb->dev);
+ kfree(sfb);
return 0;
}
--
1.7.1
^ permalink raw reply related
* [PATCH] [resend] video: s3c-fb: fix virtual resolution checking
From: Jingoo Han @ 2011-06-09 4:26 UTC (permalink / raw)
To: linux-fbdev
This patch fixes mishandling in virtual resolution checking.
Previously, virtual resolution is changed to virtual_x and virtual_y
which mean the size for buffer allocation, when s3c_fb_check_var is
called by fb_check_var. However, it is meaningless, since virtual_x
and virtual_y are fixed and user cannot change virtual resolution.
Therefore, virtual resolution should be more than resolution
such as xres and yres.
Signed-off-by: Jingoo Han <jg1.han@samsung.com>
---
drivers/video/s3c-fb.c | 5 ++---
1 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/video/s3c-fb.c b/drivers/video/s3c-fb.c
index 0352afa..e48129a 100644
--- a/drivers/video/s3c-fb.c
+++ b/drivers/video/s3c-fb.c
@@ -235,13 +235,12 @@ static int s3c_fb_check_var(struct fb_var_screeninfo *var,
struct fb_info *info)
{
struct s3c_fb_win *win = info->par;
- struct s3c_fb_pd_win *windata = win->windata;
struct s3c_fb *sfb = win->parent;
dev_dbg(sfb->dev, "checking parameters\n");
- var->xres_virtual = max((unsigned int)windata->virtual_x, var->xres);
- var->yres_virtual = max((unsigned int)windata->virtual_y, var->yres);
+ var->xres_virtual = max(var->xres_virtual, var->xres);
+ var->yres_virtual = max(var->yres_virtual, var->yres);
if (!s3c_fb_validate_win_bpp(win, var->bits_per_pixel)) {
dev_dbg(sfb->dev, "win %d: unsupported bpp %d\n",
--
1.7.1
^ permalink raw reply related
* [PATCH] [resend] video: s3c-fb: move enabling channel for window
From: Jingoo Han @ 2011-06-09 4:26 UTC (permalink / raw)
To: linux-fbdev
This patch moves enabling channel for window, because there should
be enabling channel before enabling window. If the sequence is
reversed, it makes the problem in displaying images to lcd panel.
Signed-off-by: Jingoo Han <jg1.han@samsung.com>
---
drivers/video/s3c-fb.c | 14 +++++++-------
1 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/video/s3c-fb.c b/drivers/video/s3c-fb.c
index 0352afa..4f1bc39 100644
--- a/drivers/video/s3c-fb.c
+++ b/drivers/video/s3c-fb.c
@@ -558,6 +558,13 @@ static int s3c_fb_set_par(struct fb_info *info)
vidosd_set_alpha(win, alpha);
vidosd_set_size(win, data);
+ /* Enable DMA channel for this window */
+ if (sfb->variant.has_shadowcon) {
+ data = readl(sfb->regs + SHADOWCON);
+ data |= SHADOWCON_CHx_ENABLE(win_no);
+ writel(data, sfb->regs + SHADOWCON);
+ }
+
data = WINCONx_ENWIN;
/* note, since we have to round up the bits-per-pixel, we end up
@@ -637,13 +644,6 @@ static int s3c_fb_set_par(struct fb_info *info)
writel(data, regs + sfb->variant.wincon + (win_no * 4));
writel(0x0, regs + sfb->variant.winmap + (win_no * 4));
- /* Enable DMA channel for this window */
- if (sfb->variant.has_shadowcon) {
- data = readl(sfb->regs + SHADOWCON);
- data |= SHADOWCON_CHx_ENABLE(win_no);
- writel(data, sfb->regs + SHADOWCON);
- }
-
shadow_protect_win(win, 0);
return 0;
--
1.7.1
^ 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