Re: [PATCHv2 20/28] OMAP: DSS2: Use PM runtime & HWMOD support
On Tue, 2011-06-21 at 07:49 -0700, Kevin Hilman wrote: > Tomi Valkeinen 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 > > Runtime PM usage is much better on this version, thanks! > > Like Paul, I'm not crazy about the helper functions, especially the ones > that don't do much other than call runtime PM with some debug code > around them since runtime PM already has useful debug. > > However, you're the maintainer and the one who has to debug this code > more than I do, so that decision is up to you. I have to say I'm not familiar with runtime PM's debug (yet, I need to check it out), but if I'm debugging DSS, I'm not interested in runtime PM's debug concerning some other drivers or any verbose details about runtime PM. So by having the wrappers I can just observe DSS debug, and see when DSS enables/disables runtime PM etc. If runtime PM's debug allows me to easily only see debugs about DSS, then I agree that the wrappers are extra. 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: [PATCHv2 20/28] OMAP: DSS2: Use PM runtime & HWMOD support
Tomi Valkeinen 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 Runtime PM usage is much better on this version, thanks! Like Paul, I'm not crazy about the helper functions, especially the ones that don't do much other than call runtime PM with some debug code around them since runtime PM already has useful debug. However, you're the maintainer and the one who has to debug this code more than I do, so that decision is up to you. Acked-by: Kevin Hilman 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: [PATCHv2 20/28] OMAP: DSS2: Use PM runtime & HWMOD support
On Fri, Jun 10, 2011 at 09:52:09AM +0300, Tomi Valkeinen wrote: > On Fri, 2011-06-10 at 05:03 +0900, Paul Mundt wrote: > > the use in the error paths and so on you will definitely need to be using > > pm_runtime_put_sync() at least some of the time. > > Hmm, why is that? When the user of, say, dispc, has finished with it and > calls dispc_runtime_put(), the caller shouldn't care if the HW is > actually turned off now or later. > Ah, I forgot that pm_runtime_disable() already does the synchronous bits for you, so you get lucky that way. I was concerned about the race between the work queue and the device pointer going away, but this is already handled by the subsystem via __pm_runtime_barrier() in the disable path. > pm_runtime_put() can return an error value, but my wrappers discard it, > as I don't know in which situations it could happen, and what could the > driver do about it. If the HW cannot be turned off now, why could it be > turned off later, and when would that be? > The return value is primarily aimed at informing you whether it was able to idle the device, whether there were already pending requests, etc. If you're in an exit path you're probably not too concerned with this. If you have some alternate means of cutting power to the block unrelated to runtime pm based clock control you can use the return value as a sanity measure to error out before inadvertently cutting power out underneath another user. -- 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: [PATCHv2 20/28] OMAP: DSS2: Use PM runtime & HWMOD support
On Fri, 2011-06-10 at 05:03 +0900, Paul Mundt wrote:
> On Thu, Jun 09, 2011 at 04:56:42PM +0300, Tomi Valkeinen wrote:
> > +int dispc_runtime_get(void)
> > +{
> > + int r;
> > +
> > + DSSDBG("dispc_runtime_get\n");
> > +
> > + r = pm_runtime_get_sync(&dispc.pdev->dev);
> > + WARN_ON(r < 0);
> > + return r < 0 ? r : 0;
> > +}
> > +
> > +void dispc_runtime_put(void)
> > +{
> > + int r;
> > +
> > + DSSDBG("dispc_runtime_put\n");
> > +
> > + r = pm_runtime_put(&dispc.pdev->dev);
> > + WARN_ON(r < 0);
> > }
> >
> This seems a bit odd. Your runtime_get wrapper is explicitly synchronous
> while your put wrapper is explicitly asynchronous, although these details
> (if intentional) are not at all obvious from the wrapper naming. From
Yes, the naming could be improved to make the (a)synchronousness obvious
to the caller. The functions are internal to DSS, but still.
> the use in the error paths and so on you will definitely need to be using
> pm_runtime_put_sync() at least some of the time.
Hmm, why is that? When the user of, say, dispc, has finished with it and
calls dispc_runtime_put(), the caller shouldn't care if the HW is
actually turned off now or later.
pm_runtime_put() can return an error value, but my wrappers discard it,
as I don't know in which situations it could happen, and what could the
driver do about it. If the HW cannot be turned off now, why could it be
turned off later, and when would that be?
> In the interest of avoiding confusion, I would in general just get rid of
> these wrappers and use the pm_runtime calls openly, as you already do for
> some of the other parts of the API. The runtime PM framework has pretty
> verbose debugging already that goes well beyond what you presently have,
> too.
Yes, my main reason for the wrapper is to hide the device pointer from
the users. So when, say, DSI block needs to use DISPC, it can just call
dispc_runtime_get(), instead of having a mechanism to get the dispc
device (dispc_get_dev() perhaps), and then using pm_runtime directly.
Not a big difference, but I'd like to keep the dispc device pointer
internal to dispc, as there's no real need to access it directly from
elsewhere.
Some of the wrappers are actually private to the file already, like for
dsi. These wrappers could be removed without exposing any device
pointers, but having them keeps the pattern consistent in all parts of
the dss. And it allows me to have the WARN_ONs there easily.
> You seem to have adopted this sync/async pattern for all of the users:
Yes, I think the pattern is similar all around: when you get the device,
you want it to be up and running immediately. When you put the device,
you don't care when it's actually disabled.
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: [PATCHv2 20/28] OMAP: DSS2: Use PM runtime & HWMOD support
On Thu, Jun 09, 2011 at 04:56:42PM +0300, Tomi Valkeinen wrote:
> +int dispc_runtime_get(void)
> +{
> + int r;
> +
> + DSSDBG("dispc_runtime_get\n");
> +
> + r = pm_runtime_get_sync(&dispc.pdev->dev);
> + WARN_ON(r < 0);
> + return r < 0 ? r : 0;
> +}
> +
> +void dispc_runtime_put(void)
> +{
> + int r;
> +
> + DSSDBG("dispc_runtime_put\n");
> +
> + r = pm_runtime_put(&dispc.pdev->dev);
> + WARN_ON(r < 0);
> }
>
This seems a bit odd. Your runtime_get wrapper is explicitly synchronous
while your put wrapper is explicitly asynchronous, although these details
(if intentional) are not at all obvious from the wrapper naming. From
the use in the error paths and so on you will definitely need to be using
pm_runtime_put_sync() at least some of the time.
In the interest of avoiding confusion, I would in general just get rid of
these wrappers and use the pm_runtime calls openly, as you already do for
some of the other parts of the API. The runtime PM framework has pretty
verbose debugging already that goes well beyond what you presently have,
too.
You seem to have adopted this sync/async pattern for all of the users:
> +int dsi_runtime_get(struct platform_device *dsidev)
> {
> - if (enable)
> - dss_clk_enable(DSS_CLK_ICK | DSS_CLK_FCK);
> - else
> - dss_clk_disable(DSS_CLK_ICK | DSS_CLK_FCK);
> + int r;
> + struct dsi_data *dsi = dsi_get_dsidrv_data(dsidev);
> +
> + DSSDBG("dsi_runtime_get\n");
> +
> + r = pm_runtime_get_sync(&dsi->pdev->dev);
> + WARN_ON(r < 0);
> + return r < 0 ? r : 0;
> +}
> +
> +void dsi_runtime_put(struct platform_device *dsidev)
> +{
> + struct dsi_data *dsi = dsi_get_dsidrv_data(dsidev);
> + int r;
> +
> + DSSDBG("dsi_runtime_put\n");
> +
> + r = pm_runtime_put(&dsi->pdev->dev);
> + WARN_ON(r < 0);
> }
>
Here.
> -void dss_clk_disable(enum dss_clock clks)
> -{
..
> - dss_clk_disable_no_ctx(clks);
> + r = pm_runtime_get_sync(&dss.pdev->dev);
> + WARN_ON(r < 0);
> + return r < 0 ? r : 0;
> }
>
> -static void dss_clk_enable_all_no_ctx(void)
> +void dss_runtime_put(void)
> {
..
> + r = pm_runtime_put(&dss.pdev->dev);
> + WARN_ON(r < 0);
> }
>
And here.
> +static int hdmi_runtime_get(void)
> +{
> + int r;
> +
> + DSSDBG("hdmi_runtime_get\n");
> +
> + r = pm_runtime_get_sync(&hdmi.pdev->dev);
> + WARN_ON(r < 0);
> + return r < 0 ? r : 0;
> +}
> +
> +static void hdmi_runtime_put(void)
> +{
> + int r;
> +
> + DSSDBG("hdmi_runtime_put\n");
> +
> + r = pm_runtime_put(&hdmi.pdev->dev);
> + WARN_ON(r < 0);
> +}
> +
And here.
> -static void rfbi_enable_clocks(bool enable)
> +static int rfbi_runtime_get(void)
> {
> - if (enable)
> - dss_clk_enable(DSS_CLK_ICK | DSS_CLK_FCK);
> - else
> - dss_clk_disable(DSS_CLK_ICK | DSS_CLK_FCK);
> + int r;
> +
> + DSSDBG("rfbi_runtime_get\n");
> +
> + r = pm_runtime_get_sync(&rfbi.pdev->dev);
> + WARN_ON(r < 0);
> + return r < 0 ? r : 0;
> +}
> +
> +static void rfbi_runtime_put(void)
> +{
> + int r;
> +
> + DSSDBG("rfbi_runtime_put\n");
> +
> + r = pm_runtime_put(&rfbi.pdev->dev);
> + WARN_ON(r < 0);
> }
>
And here.
> +static int venc_runtime_get(void)
> {
> - if (enable) {
> - dss_clk_enable(DSS_CLK_ICK | DSS_CLK_FCK | DSS_CLK_TVFCK);
> - if (dss_has_feature(FEAT_VENC_REQUIRES_TV_DAC_CLK))
> - dss_clk_enable(DSS_CLK_VIDFCK);
> - } else {
> - dss_clk_disable(DSS_CLK_ICK | DSS_CLK_FCK | DSS_CLK_TVFCK);
> - if (dss_has_feature(FEAT_VENC_REQUIRES_TV_DAC_CLK))
> - dss_clk_disable(DSS_CLK_VIDFCK);
> - }
> + int r;
> +
> + DSSDBG("venc_runtime_get\n");
> +
> + r = pm_runtime_get_sync(&venc.pdev->dev);
> + WARN_ON(r < 0);
> + return r < 0 ? r : 0;
> +}
> +
> +static void venc_runtime_put(void)
> +{
> + int r;
> +
> + DSSDBG("venc_runtime_put\n");
> +
> + r = pm_runtime_put(&venc.pdev->dev);
> + WARN_ON(r < 0);
> }
>
And here.
--
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
