| Message ID | 87hb86a5mm.fsf@ti.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Tomi Valkeinen |
| Headers | show |
On Fri, 2011-06-03 at 15:53 -0700, Kevin Hilman wrote: > Tomi Valkeinen <tomi.valkeinen@ti.com> writes: > > > Hi Kevin, > > > > On Fri, 2011-06-03 at 09:45 -0700, Kevin Hilman wrote: > >> Tomi Valkeinen <tomi.valkeinen@ti.com> writes: > >> > >> > Use PM runtime and HWMOD support to handle enabling and disabling of DSS > >> > modules. > >> > > >> > Each DSS module will have get and put functions which can be used to > >> > enable and disable that module. The functions use pm_runtime and hwmod > >> > opt-clocks to enable the hardware. > >> > > >> > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> > >> > >> [...] > >> > >> > +int dispc_runtime_get(void) > >> > +{ > >> > + int r; > >> > + > >> > + mutex_lock(&dispc.runtime_lock); > >> > >> It's not clear to me what the lock is trying to protect. I guess it's > >> the counter? I don't think it should be needed... > > > > Yes, the counter. I don't think > > > > if (dispc.runtime_count++ == 0) > > > > is thread safe. > > OK, if it's just the counter, you can drop the mutex and use an atomic > variable and use atomic_inc(), atomic_dec() etc. Then it will be clear > from reading what exactly is protected. Hmm, sorry, my mistake. It's actually for the whole function: we can't do "put" before the whole "get" has finished. Otherwise we could end up, for example, disabling a clock before enabling it. > >> > + if (dispc.runtime_count++ == 0) { > >> > >> You shouldn't need your own use-counting here. The runtime PM core is > >> already doing usage counting. > >> > >> Instead, you need to use the ->runtime_suspend() and ->runtime_resume() > >> callbacks (in dev_pm_ops). These callbacks are called by the runtime PM > >> core when the usage count goes to/from zero. > > > > Yes, I wish I could do that =). > > > > I tried to explain this in the 00-patch, I guess I should've explained > > it in this patch also. Perhaps also in a comment. > > Oops, my fault. I didn't read the whole 00 patch. I'm pretty ignorant > about DSS, so I was focused in on the runtime PM implementation only. > Sorry about that. > > > From the introduction: > > > > --- > > > > Due to DSS' peculiar clock setup the code is not as elegant as I'd like. The > > problem is that on OMAP4 we have to enable an optional clock before calling > > pm_runtime_get(), and similarly we need to keep the optional clock enabled > > until after pm_runtime_put() has been called. > > Just to clarify, what exactly does the opt clock have to be enabled for? I'm not sure if this is a valid definition, but in my mind the opt clock has two uses: 1) a functional clock, to make the HW tick and registers accessible, and 2) act as a source clock for the outgoing pixel clock. Of these, the first one is the one that matters in this scope. So, it's a mandatory clock, but it's optional in the sense that there are other clocks that can be used in place of that clock. All OMAPs support the DSS fclk from PRCM (this is the default). If I remember right, OMAP2 also supports using sys clock as the DSS fclk. OMAP3 and 4 support using DSI PLL (whose source clock is sys clock) as the fclk. > From what I can gather, this particular opt clock has to be enabled for > the hwmod itself to be enabled (or disabled), correct? Yes, the registers are unaccessible and the HW doesn't come out of reset without the fclk. > This has been a known issue for reset[1], but I wasn't aware that it was > needed for a normal (re)enable of the hwmod. > > > This causes some extra complications. For example, we can't use runtime_resume > > callback to enable the opt clocks, as they are required before calling > > pm_runtime_get(). > > > > Yuck. > > > --- > > > > So, the opt clock handling required by the driver pretty much messes the > > whole idea of pm_runtime callbacks here. We can't do pretty much > > anything in the suspend and resume callbacks. > > > > We can't do save_context() in the suspend callback, because suspend > > could be called later, after pm_runtime_put_sync() and > > dispc_runtime_put() has returned. And and that point we've turned off > > the opt clock and can't touch the registers. If we'd move the opt-clock > > clk_disable to suspend, but clk_enable in dispc_runtime_get(), we'd get > > mismatched clk_disable/enable, so we can't do that. etc. > > Double yuck. > > It's clear now that you at least wanted to do the "right" thing and > thought about the variousways to do it, but weren't able to for various > reasons. Thanks! > > > I didn't come up with any other solutions to this. In the end, the > > dispc_runtime_get/put are quite clean functions (well, says me =), and > > their usage is more or less equivalent to pm_runtime_get/put. So I don't > > see it as a horrible solution. > > I agree, it's not horrible. Just induces very mild nausea. ;) > > > If and when the hwmod/pm_runtime/something handles this opt-clock issue, > > it shouldn't be too big of a job to use pm_runtime properly in the > > omapdss. > > Agreed. I certainly won't hold up this patch unless we come up with an > alternate solution very quickly (which I will try below, and wait for > Paul/Benoit to correct me.) > > > Of course, if you or somebody else has a solution for this now, I'm > > more than happy to use it =). > > I don't have a solid solution, but so far, this sounds like an IP > requirement that should be managed at the hwmod level. I agree. How I imagine things should work is that the hwmod code defaults to the standard PRCM clock, but allows us to change the functional clock at a later time, allowing the original clock to be disabled. But I don't know how this would work in practice. The DSI PLL clock has to be programmed via DSI HW, and the bits that control the clock muxes are inside DSS. So the HWMOD code can't probably handle this by itself, but needs the DSS to do certain things at certain times. > One approach would be to have an option to selectively manage optional > clocks in the hwmod enable/idle path. We're doing it for reset[1], but > not for anything else. And based on the changelog there[1], it doesn't > even sound like we fully understand the exact requirements around reset. Yes, the OMAP4 DSS reset is a bit unclear for me. > From the contortions you've had to go through though, it sounds like the > same optional clocks that are needed for reset are needed for a "normal" > module enable/disable. > > I tried a simple hack to do that below[2]. Can you see if that works > for you and if you can remove the opt clk mgmt from your driver(s)? I > only boot tested it on 3430/n900 which only has this opt-clk flag set > for the GPIO IP blocks. I didn't test it yet, but I would imagine it works. But it doesn't help us towards properly using the other clocks as fclk. However, it's not really any worse than the current DSS code. We don't currently turn off the DSS fclk from the PRCM, even if we would use the clock from the DSI PLL. So perhaps an approach where hwmod would enable the fclk from the PRCM automatically would be good. It's no worse than the other option, and it'd give us the ability to use pm_runtime in the proper way. In this (hopefully temporary) solution the clock wouldn't really be optional clock, but mandatory. > Another idea off the top of my head is to extend runtime PM to have a > couple additional callbacks. Something like ->runtime_resume_prepare() > which would be called before the HW is enabled (and thus before > ->runtime_resume()), and similarily ->runtime_suspend_complete() which > would be called after ->runtime_suspend() and after the HW has been > disabled. > > I don't really like this approach because so far this is the only driver > that has needed something like this. And in the case of this IP the > enable/disable of the optional clocks seems like a HW requirement for > the correct enable/disable of the IP, so it seems like something that > should be managed by the hwmod. Yes, I agree, that doesn't sound like a very good approach. Tomi -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Tomi, On 6/4/2011 10:01 AM, Valkeinen, Tomi wrote: > On Fri, 2011-06-03 at 15:53 -0700, Kevin Hilman wrote: >> Tomi Valkeinen<tomi.valkeinen@ti.com> writes: >> >>> Hi Kevin, >>> >>> On Fri, 2011-06-03 at 09:45 -0700, Kevin Hilman wrote: >>>> Tomi Valkeinen<tomi.valkeinen@ti.com> writes: >>>> >>>>> Use PM runtime and HWMOD support to handle enabling and disabling of DSS >>>>> modules. >>>>> >>>>> Each DSS module will have get and put functions which can be used to >>>>> enable and disable that module. The functions use pm_runtime and hwmod >>>>> opt-clocks to enable the hardware. >>>>> >>>>> Signed-off-by: Tomi Valkeinen<tomi.valkeinen@ti.com> >>>> >>>> [...] >>>> >>>>> +int dispc_runtime_get(void) >>>>> +{ >>>>> + int r; >>>>> + >>>>> + mutex_lock(&dispc.runtime_lock); >>>> >>>> It's not clear to me what the lock is trying to protect. I guess it's >>>> the counter? I don't think it should be needed... >>> >>> Yes, the counter. I don't think >>> >>> if (dispc.runtime_count++ == 0) >>> >>> is thread safe. >> >> OK, if it's just the counter, you can drop the mutex and use an atomic >> variable and use atomic_inc(), atomic_dec() etc. Then it will be clear >> from reading what exactly is protected. > > Hmm, sorry, my mistake. It's actually for the whole function: we can't > do "put" before the whole "get" has finished. Otherwise we could end up, > for example, disabling a clock before enabling it. > >>>>> + if (dispc.runtime_count++ == 0) { >>>> >>>> You shouldn't need your own use-counting here. The runtime PM core is >>>> already doing usage counting. >>>> >>>> Instead, you need to use the ->runtime_suspend() and ->runtime_resume() >>>> callbacks (in dev_pm_ops). These callbacks are called by the runtime PM >>>> core when the usage count goes to/from zero. >>> >>> Yes, I wish I could do that =). >>> >>> I tried to explain this in the 00-patch, I guess I should've explained >>> it in this patch also. Perhaps also in a comment. >> >> Oops, my fault. I didn't read the whole 00 patch. I'm pretty ignorant >> about DSS, so I was focused in on the runtime PM implementation only. >> Sorry about that. >> >>> From the introduction: >>> >>> --- >>> >>> Due to DSS' peculiar clock setup the code is not as elegant as I'd like. The >>> problem is that on OMAP4 we have to enable an optional clock before calling >>> pm_runtime_get(), and similarly we need to keep the optional clock enabled >>> until after pm_runtime_put() has been called. >> >> Just to clarify, what exactly does the opt clock have to be enabled for? > > I'm not sure if this is a valid definition, but in my mind the opt clock > has two uses: 1) a functional clock, to make the HW tick and registers > accessible, and 2) act as a source clock for the outgoing pixel clock. 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. The modulemode was not there previously on OMAP2 & 3, but it is more or less equivalent to icken=1 + fcken=1. This idea was to hide the explicit clock management especially for the iclk that were already supposed to always be in autoidle. Since the current hwmod + clock fmwks are still based on the previous clock centric approach we used to have on OMAP2 & 3, we cannot match properly the modulemode to any clock and thus cannot handle properly the DSS fclk as the main clock instead of the optional clock. A temporary option will be to consider the modulemode as the interface clock and thus remove it from the main_clk and replace it by the real DSS fclk. It should work be will unfortunately not be compliant with PRCM recommendation to enable the modulemode once every clocks are enabled. The long term solution is to update the hwmod fmwk to handle the modulemode directly and not through the clock fmwk. It will allow the main_clk to be connnected to the dss_fclk. You will not have that nasty opt_clock issue anymore. Regards, Benoit -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2011-06-06 at 14:56 +0200, Cousson, Benoit wrote: > Hi Tomi, > > On 6/4/2011 10:01 AM, Valkeinen, Tomi wrote: > > On Fri, 2011-06-03 at 15:53 -0700, Kevin Hilman wrote: > >> Tomi Valkeinen<tomi.valkeinen@ti.com> writes: > >> > >>> Hi Kevin, > >>> > >>> On Fri, 2011-06-03 at 09:45 -0700, Kevin Hilman wrote: > >>>> Tomi Valkeinen<tomi.valkeinen@ti.com> writes: > >>>> > >>>>> Use PM runtime and HWMOD support to handle enabling and disabling of DSS > >>>>> modules. > >>>>> > >>>>> Each DSS module will have get and put functions which can be used to > >>>>> enable and disable that module. The functions use pm_runtime and hwmod > >>>>> opt-clocks to enable the hardware. > >>>>> > >>>>> Signed-off-by: Tomi Valkeinen<tomi.valkeinen@ti.com> > >>>> > >>>> [...] > >>>> > >>>>> +int dispc_runtime_get(void) > >>>>> +{ > >>>>> + int r; > >>>>> + > >>>>> + mutex_lock(&dispc.runtime_lock); > >>>> > >>>> It's not clear to me what the lock is trying to protect. I guess it's > >>>> the counter? I don't think it should be needed... > >>> > >>> Yes, the counter. I don't think > >>> > >>> if (dispc.runtime_count++ == 0) > >>> > >>> is thread safe. > >> > >> OK, if it's just the counter, you can drop the mutex and use an atomic > >> variable and use atomic_inc(), atomic_dec() etc. Then it will be clear > >> from reading what exactly is protected. > > > > Hmm, sorry, my mistake. It's actually for the whole function: we can't > > do "put" before the whole "get" has finished. Otherwise we could end up, > > for example, disabling a clock before enabling it. > > > >>>>> + if (dispc.runtime_count++ == 0) { > >>>> > >>>> You shouldn't need your own use-counting here. The runtime PM core is > >>>> already doing usage counting. > >>>> > >>>> Instead, you need to use the ->runtime_suspend() and ->runtime_resume() > >>>> callbacks (in dev_pm_ops). These callbacks are called by the runtime PM > >>>> core when the usage count goes to/from zero. > >>> > >>> Yes, I wish I could do that =). > >>> > >>> I tried to explain this in the 00-patch, I guess I should've explained > >>> it in this patch also. Perhaps also in a comment. > >> > >> Oops, my fault. I didn't read the whole 00 patch. I'm pretty ignorant > >> about DSS, so I was focused in on the runtime PM implementation only. > >> Sorry about that. > >> > >>> From the introduction: > >>> > >>> --- > >>> > >>> Due to DSS' peculiar clock setup the code is not as elegant as I'd like. The > >>> problem is that on OMAP4 we have to enable an optional clock before calling > >>> pm_runtime_get(), and similarly we need to keep the optional clock enabled > >>> until after pm_runtime_put() has been called. > >> > >> Just to clarify, what exactly does the opt clock have to be enabled for? > > > > I'm not sure if this is a valid definition, but in my mind the opt clock > > has two uses: 1) a functional clock, to make the HW tick and registers > > accessible, and 2) act as a source clock for the outgoing pixel clock. > > 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. > > The modulemode was not there previously on OMAP2 & 3, but it is more or > less equivalent to icken=1 + fcken=1. > This idea was to hide the explicit clock management especially for the > iclk that were already supposed to always be in autoidle. > > Since the current hwmod + clock fmwks are still based on the previous > clock centric approach we used to have on OMAP2 & 3, we cannot match > properly the modulemode to any clock and thus cannot handle properly the > DSS fclk as the main clock instead of the optional clock. > > A temporary option will be to consider the modulemode as the interface > clock and thus remove it from the main_clk and replace it by the real > DSS fclk. > > It should work be will unfortunately not be compliant with PRCM > recommendation to enable the modulemode once every clocks are enabled. > > The long term solution is to update the hwmod fmwk to handle the > modulemode directly and not through the clock fmwk. It will allow the > main_clk to be connnected to the dss_fclk. > > You will not have that nasty opt_clock issue anymore. 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? Tomi -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 6/6/2011 3:01 PM, Valkeinen, Tomi wrote: > On Mon, 2011-06-06 at 14:56 +0200, Cousson, Benoit wrote: >> Hi Tomi, >> >> On 6/4/2011 10:01 AM, Valkeinen, Tomi wrote: >>> On Fri, 2011-06-03 at 15:53 -0700, Kevin Hilman wrote: >>>> Tomi Valkeinen<tomi.valkeinen@ti.com> writes: >>>> >>>>> Hi Kevin, >>>>> >>>>> On Fri, 2011-06-03 at 09:45 -0700, Kevin Hilman wrote: >>>>>> Tomi Valkeinen<tomi.valkeinen@ti.com> writes: >>>>>> >>>>>>> Use PM runtime and HWMOD support to handle enabling and disabling of DSS >>>>>>> modules. >>>>>>> >>>>>>> Each DSS module will have get and put functions which can be used to >>>>>>> enable and disable that module. The functions use pm_runtime and hwmod >>>>>>> opt-clocks to enable the hardware. >>>>>>> >>>>>>> Signed-off-by: Tomi Valkeinen<tomi.valkeinen@ti.com> >>>>>> >>>>>> [...] >>>>>> >>>>>>> +int dispc_runtime_get(void) >>>>>>> +{ >>>>>>> + int r; >>>>>>> + >>>>>>> + mutex_lock(&dispc.runtime_lock); >>>>>> >>>>>> It's not clear to me what the lock is trying to protect. I guess it's >>>>>> the counter? I don't think it should be needed... >>>>> >>>>> Yes, the counter. I don't think >>>>> >>>>> if (dispc.runtime_count++ == 0) >>>>> >>>>> is thread safe. >>>> >>>> OK, if it's just the counter, you can drop the mutex and use an atomic >>>> variable and use atomic_inc(), atomic_dec() etc. Then it will be clear >>>> from reading what exactly is protected. >>> >>> Hmm, sorry, my mistake. It's actually for the whole function: we can't >>> do "put" before the whole "get" has finished. Otherwise we could end up, >>> for example, disabling a clock before enabling it. >>> >>>>>>> + if (dispc.runtime_count++ == 0) { >>>>>> >>>>>> You shouldn't need your own use-counting here. The runtime PM core is >>>>>> already doing usage counting. >>>>>> >>>>>> Instead, you need to use the ->runtime_suspend() and ->runtime_resume() >>>>>> callbacks (in dev_pm_ops). These callbacks are called by the runtime PM >>>>>> core when the usage count goes to/from zero. >>>>> >>>>> Yes, I wish I could do that =). >>>>> >>>>> I tried to explain this in the 00-patch, I guess I should've explained >>>>> it in this patch also. Perhaps also in a comment. >>>> >>>> Oops, my fault. I didn't read the whole 00 patch. I'm pretty ignorant >>>> about DSS, so I was focused in on the runtime PM implementation only. >>>> Sorry about that. >>>> >>>>> From the introduction: >>>>> >>>>> --- >>>>> >>>>> Due to DSS' peculiar clock setup the code is not as elegant as I'd like. The >>>>> problem is that on OMAP4 we have to enable an optional clock before calling >>>>> pm_runtime_get(), and similarly we need to keep the optional clock enabled >>>>> until after pm_runtime_put() has been called. >>>> >>>> Just to clarify, what exactly does the opt clock have to be enabled for? >>> >>> I'm not sure if this is a valid definition, but in my mind the opt clock >>> has two uses: 1) a functional clock, to make the HW tick and registers >>> accessible, and 2) act as a source clock for the outgoing pixel clock. >> >> 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. >> >> The modulemode was not there previously on OMAP2& 3, but it is more or >> less equivalent to icken=1 + fcken=1. >> This idea was to hide the explicit clock management especially for the >> iclk that were already supposed to always be in autoidle. >> >> Since the current hwmod + clock fmwks are still based on the previous >> clock centric approach we used to have on OMAP2& 3, we cannot match >> properly the modulemode to any clock and thus cannot handle properly the >> DSS fclk as the main clock instead of the optional clock. >> >> A temporary option will be to consider the modulemode as the interface >> clock and thus remove it from the main_clk and replace it by the real >> DSS fclk. >> >> It should work be will unfortunately not be compliant with PRCM >> recommendation to enable the modulemode once every clocks are enabled. >> >> The long term solution is to update the hwmod fmwk to handle the >> modulemode directly and not through the clock fmwk. It will allow the >> main_clk to be connnected to the dss_fclk. >> >> You will not have that nasty opt_clock issue anymore. > > 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? 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. Regards, Benoit -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. If the clock is main_clk for the HWMOD, it sounds to me it's always enabled if the HWMOD is enabled? Tomi -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c index 293fa6c..7bcf802 100644 --- a/arch/arm/mach-omap2/omap_hwmod.c +++ b/arch/arm/mach-omap2/omap_hwmod.c @@ -586,6 +586,9 @@ static int _init_opt_clks(struct omap_hwmod *oh) return ret; } +static void _enable_optional_clocks(struct omap_hwmod *oh); +static void _disable_optional_clocks(struct omap_hwmod *oh); + /** * _enable_clocks - enable hwmod main clock and interface clocks * @oh: struct omap_hwmod * @@ -599,6 +602,9 @@ static int _enable_clocks(struct omap_hwmod *oh) pr_debug("omap_hwmod: %s: enabling clocks\n", oh->name); + if (oh->flags & HWMOD_CONTROL_OPT_CLKS_IN_RESET) + _enable_optional_clocks(oh); + if (oh->_clk) clk_enable(oh->_clk); @@ -642,6 +648,9 @@ static int _disable_clocks(struct omap_hwmod *oh) } } + if (oh->flags & HWMOD_CONTROL_OPT_CLKS_IN_RESET) + _disable_optional_clocks(oh); + /* The opt clocks are controlled by the device driver. */ return 0;