Re: [PATCH 19/27] OMAP: DSS2: Use PM runtime HWMOD support
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 -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 19/27] OMAP: DSS2: Use PM runtime HWMOD support
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
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 19/27] OMAP: DSS2: Use PM runtime HWMOD support
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 19/27] OMAP: DSS2: Use PM runtime HWMOD support
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 =
Re: [PATCH 19/27] OMAP: DSS2: Use PM runtime HWMOD support
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 19/27] OMAP: DSS2: Use PM runtime HWMOD support
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 19/27] OMAP: DSS2: Use PM runtime HWMOD support
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 19/27] OMAP: DSS2: Use PM runtime HWMOD support
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 -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 19/27] OMAP: DSS2: Use PM runtime HWMOD support
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),
-
Re: [PATCH 19/27] OMAP: DSS2: Use PM runtime HWMOD support
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 -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 19/27] OMAP: DSS2: Use PM runtime HWMOD support
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 -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 19/27] OMAP: DSS2: Use PM runtime HWMOD support
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 Valkeinentomi.valkei...@ti.com writes:
Hi Kevin,
On Fri, 2011-06-03 at 09:45 -0700, Kevin Hilman wrote:
Tomi Valkeinentomi.valkei...@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 Valkeinentomi.valkei...@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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 19/27] OMAP: DSS2: Use PM runtime HWMOD support
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 Valkeinentomi.valkei...@ti.com writes:
Hi Kevin,
On Fri, 2011-06-03 at 09:45 -0700, Kevin Hilman wrote:
Tomi Valkeinentomi.valkei...@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 Valkeinentomi.valkei...@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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 19/27] OMAP: DSS2: Use PM runtime HWMOD support
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 Valkeinentomi.valkei...@ti.com writes:
Hi Kevin,
On Fri, 2011-06-03 at 09:45 -0700, Kevin Hilman wrote:
Tomi Valkeinentomi.valkei...@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 Valkeinentomi.valkei...@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 majord...@vger.kernel.org
More majordomo info at
Re: [PATCH 19/27] OMAP: DSS2: Use PM runtime HWMOD support
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 19/27] OMAP: DSS2: Use PM runtime HWMOD support
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 19/27] OMAP: DSS2: Use PM runtime HWMOD support
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 19/27] OMAP: DSS2: Use PM runtime HWMOD support
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,
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 19/27] OMAP: DSS2: Use PM runtime HWMOD support
On Fri, 2011-06-03 at 15:53 -0700, Kevin Hilman wrote:
Tomi Valkeinen tomi.valkei...@ti.com writes:
Hi Kevin,
On Fri, 2011-06-03 at 09:45 -0700, Kevin Hilman wrote:
Tomi Valkeinen tomi.valkei...@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.valkei...@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
Re: [PATCH 19/27] OMAP: DSS2: Use PM runtime HWMOD support
Tomi Valkeinen tomi.valkei...@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.valkei...@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...
+ 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.
IOW, this function should simply be a pm_runtime_get_sync(), and the
rest of the stuff in this function should be done in the
-runtime_resume() callback, which is only executed when the usage_count
transitions from zero.
+ DSSDBG(dispc_runtime_get\n);
+
+ r = dss_runtime_get();
+ if (r)
+ goto err_dss_get;
+
+ /* XXX dispc fclk can also come from DSI PLL */
+ clk_enable(dispc.dss_clk);
+
+ r = pm_runtime_get_sync(dispc.pdev-dev);
+ WARN_ON(r);
+ if (r 0)
+ goto err_runtime_get;
+
+ if (dispc_need_ctx_restore())
+ dispc_restore_context();
+ }
+
+ mutex_unlock(dispc.runtime_lock);
+
+ return 0;
+
+err_runtime_get:
+ clk_disable(dispc.dss_clk);
+ dss_runtime_put();
+err_dss_get:
+ mutex_unlock(dispc.runtime_lock);
+
+ return r;
}
+void dispc_runtime_put(void)
+{
+ mutex_lock(dispc.runtime_lock);
+
+ if (--dispc.runtime_count == 0) {
+ int r;
Same problem here.
No usage counting is required (and probably no locking either.) This
function should simply be a pm_runtime_put(), and the rest of the stuff
should be in the driver's -runtime_suspend() callback.
+ DSSDBG(dispc_runtime_put\n);
+
+ dispc_save_context();
+
+ r = pm_runtime_put_sync(dispc.pdev-dev);
Does this need to be the _sync() version? It looks like it could be use
the normal (async) version.: pm_runtime_put().
+ WARN_ON(r);
+
+ clk_disable(dispc.dss_clk);
+
+ dss_runtime_put();
+ }
+
+ mutex_unlock(dispc.runtime_lock);
+}
+
+
bool dispc_go_busy(enum omap_channel channel)
{
int bit;
@@ -530,7 +645,7 @@ void dispc_go(enum omap_channel channel)
int bit;
bool enable_bit, go_bit;
- enable_clocks(1);
+ dispc_runtime_get();
Based on the above suggestions, you probably don't need dedicated
functions for this. Just call pm_runtime_get_sync() here...
if (channel == OMAP_DSS_CHANNEL_LCD ||
channel == OMAP_DSS_CHANNEL_LCD2)
@@ -571,7 +686,7 @@ void dispc_go(enum omap_channel channel)
else
REG_FLD_MOD(DISPC_CONTROL, 1, bit, bit);
end:
- enable_clocks(0);
+ dispc_runtime_put();
...and the pm_runtime_put() here.
Kevin
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 19/27] OMAP: DSS2: Use PM runtime HWMOD support
Hi Kevin,
On Fri, 2011-06-03 at 09:45 -0700, Kevin Hilman wrote:
Tomi Valkeinen tomi.valkei...@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.valkei...@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.
+ 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.
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.
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().
---
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.
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.
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. Of course, if you or somebody else has a solution for this now,
I'm more than happy to use it =).
And this opt-clock problem pretty much covers all your comments below,
except one which I've answered also.
IOW, this function should simply be a pm_runtime_get_sync(), and the
rest of the stuff in this function should be done in the
-runtime_resume() callback, which is only executed when the usage_count
transitions from zero.
+ DSSDBG(dispc_runtime_get\n);
+
+ r = dss_runtime_get();
+ if (r)
+ goto err_dss_get;
+
+ /* XXX dispc fclk can also come from DSI PLL */
+ clk_enable(dispc.dss_clk);
+
+ r = pm_runtime_get_sync(dispc.pdev-dev);
+ WARN_ON(r);
+ if (r 0)
+ goto err_runtime_get;
+
+ if (dispc_need_ctx_restore())
+ dispc_restore_context();
+ }
+
+ mutex_unlock(dispc.runtime_lock);
+
+ return 0;
+
+err_runtime_get:
+ clk_disable(dispc.dss_clk);
+ dss_runtime_put();
+err_dss_get:
+ mutex_unlock(dispc.runtime_lock);
+
+ return r;
}
+void dispc_runtime_put(void)
+{
+ mutex_lock(dispc.runtime_lock);
+
+ if (--dispc.runtime_count == 0) {
+ int r;
Same problem here.
No usage counting is required (and probably no locking either.) This
function should simply be a pm_runtime_put(), and the rest of the stuff
should be in the driver's -runtime_suspend() callback.
+ DSSDBG(dispc_runtime_put\n);
+
+ dispc_save_context();
+
+ r = pm_runtime_put_sync(dispc.pdev-dev);
Does this need to be the _sync() version? It looks like it could be use
the normal (async) version.: pm_runtime_put().
It's sync because we turn off the opt clock below, and the HW shouldn't
be touched after that, which I guess pm_runtime_put (or the subsequent
async suspend) could do.
Tomi
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 19/27] OMAP: DSS2: Use PM runtime HWMOD support
Tomi Valkeinen tomi.valkei...@ti.com writes:
Hi Kevin,
On Fri, 2011-06-03 at 09:45 -0700, Kevin Hilman wrote:
Tomi Valkeinen tomi.valkei...@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.valkei...@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.
+ 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?
From what I can gather, this particular opt clock has to be enabled for
the hwmod itself to be enabled (or disabled), correct?
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.
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.
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.
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()),
