[...] >>> @@ -157,11 +181,17 @@ static bool clk_core_is_prepared(struct clk_core >>> *core) >>> if (!core->ops->is_prepared) >>> return core->prepare_count; >>> >>> - return core->ops->is_prepared(core->hw); >>> + clk_pm_runtime_get(core); >> >> I guess you should assign status to the return code, and check it. > > > Okay. I assume that in case of any failure from runtime pm, the function > should return false? I think so, yes. > > [...] >>> static void clk_disable_unused_subtree(struct clk_core *core) >>> @@ -768,6 +834,9 @@ static void clk_disable_unused_subtree(struct >>> clk_core *core) >>> if (core->flags & CLK_OPS_PARENT_ENABLE) >>> clk_core_prepare_enable(core->parent); >>> >>> + if (clk_pm_runtime_get(core) != 0) >> >> Is there any reason to why you haven't moved this further down in this >> function, like just before calling clk_core_is_enabled()? > > > Yes, clk_enable_lock() takes a spinlock, so I cannot call pm_runtime_get > after it. Of course, you are right! > > >> >> You may also simplify this: >> if (clk_pm_runtime_get(core)) >> >>> + return; >>> + >> >> You need to restore the call made to clk_core_prepare_enable() >> earlier, so please update the error handling to cope with this. >> [...] >>> @@ -2546,6 +2631,8 @@ struct clk *clk_register(struct device *dev, struct >>> clk_hw *hw) >>> goto fail_name; >>> } >>> core->ops = hw->init->ops; >>> + if (dev && (hw->init->flags & CLK_RUNTIME_PM)) >>> + core->dev = dev; >> >> I guess you need this to play safe, although I am really wondering if >> we should try without. >> >> Not that many clocks are currently being registered with a valid >> struct device pointer. For the other cases why not try to use runtime >> PM as per default? > > > I've that tried initially, but it causes failure for all the clock > controllers, which don't enable runtime pm. One of such case is max77686 > PMIC, which provides 3 clocks. Maybe a negative flag (CLK_NO_RUNTIME_PM) > will be a better solution, so by default the runtime pm calls will be > enabled for every driver providing struct device? I assume that's because the runtime PM errors in clk_pm_runtime_get() and friends, are being propagated to the callers? Especially because the runtime PM core returns error codes, in cases when runtime PM hasn't been enabled for the device. > >> Moreover we anyway rely on the clock provider to enable runtime PM for >> the clock device, and when that isn't the case the runtime PM >> deployment in the core should still be safe, right!? > > > I don't get the above comment. Do you want to check if runtime pm has > been enabled during clock registration? Yes, something like that. Apologize, I was clearly being too vague. My point is, we don't need to invent a specific clock provider flag for this. Instead the clock core could just at clock registration check if runtime PM is enabled for the device (pm_runtime_enabled()),. and from that assign an internal clock core flag to keep track of whether runtime PM should be managed or not. > >>> if (dev && dev->driver) >>> core->owner = dev->driver->owner; >>> core->hw = hw; >>> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h >>> index a39c0c530778..8a131eb71fdf 100644 >>> --- a/include/linux/clk-provider.h >>> +++ b/include/linux/clk-provider.h >>> @@ -35,6 +35,7 @@ >>> #define CLK_IS_CRITICAL BIT(11) /* do not gate, ever */ >>> /* parents need enable during gate/ungate, set rate and re-parent */ >>> #define CLK_OPS_PARENT_ENABLE BIT(12) >>> +#define CLK_RUNTIME_PM BIT(13) >>> >>> struct clk; >>> struct clk_hw; >>> -- >>> 1.9.1 >>> >> > > Best regards > -- > Marek Szyprowski, PhD > Samsung R&D Institute Poland > Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
 |