On Thu, 21 Mar 2019 09:37:46 +0000,
Jon Hunter <jonathanh@xxxxxxxxxx> wrote:
>
>
> On 20/03/2019 18:08, Marc Zyngier wrote:
> > On Thu, 14 Mar 2019 13:23:13 +0530
> > Sameer Pujar <spujar@xxxxxxxxxx> wrote:
> >
> >> gic-pm driver is using pm_clk_*() interface to manage clock resources. With
> >> this, clocks always remain ON. This happens on Tegra devices which use BPMP
> >> co-processor to manage the clocks, where clocks are enabled during prepare
> >> phase. This is necessary because calls to BPMP are always blocking. When
> >> pm_clk_*() interface is used on such devices, clock prepare count is not
> >> balanced till driver remove() gets executed and hence clocks are seen ON
> >> always. This patch replaces pm_clk_*() with devm_clk_*() framework.
> >
> > Does this mean this code effectively becomes Tegra specific, bypassing
> > the existing PM framework to workaround implementation-specific issues?
> > I don't really mind (Tegra is apparently the only user), but I'd rather
> > make it sure I understand what is going on.
>
> No not at all. It is still generic and not Tegra specific, but for newer
> Tegra devices we cannot use the pm-clk framework and so use the clk
> framework directly. So this still works for any device. Consider it a
> lower common denominator.
But this assumes that the lowest common denominator will be good
enough for whatever comes next. As I said, I don't really mind (I
really hope that GICvX (for X < 3) is dead as a nail).
>
> > Also, I object to the subject line. This is not about switching to some
> > fancy helpers. This is about changing the life-cycle of clocks, which
> > is entirely different. Please make sure the subject reflects what the
> > patch is doing.
> >
> > Otherwise, there isn't much to say about the code. A couple of very
> > minor nits below:
> >
> >>
> >> Suggested-by: Mohan Kumar D <mkumard@xxxxxxxxxx>
> >> Signed-off-by: Sameer Pujar <spujar@xxxxxxxxxx>
> >> Reviewed-by: Jonathan Hunter <jonathanh@xxxxxxxxxx>
> >> ---
> >> drivers/irqchip/irq-gic-pm.c | 70 +++++++++++++++++++++++++++++---------------
> >> 1 file changed, 47 insertions(+), 23 deletions(-)
> >>
> >> diff --git a/drivers/irqchip/irq-gic-pm.c b/drivers/irqchip/irq-gic-pm.c
> >> index ecafd29..ae6c8d6 100644
> >> --- a/drivers/irqchip/irq-gic-pm.c
> >> +++ b/drivers/irqchip/irq-gic-pm.c
> >> @@ -19,7 +19,6 @@
> >> #include <linux/of_irq.h>
> >> #include <linux/irqchip/arm-gic.h>
> >> #include <linux/platform_device.h>
> >> -#include <linux/pm_clock.h>
> >> #include <linux/pm_runtime.h>
> >> #include <linux/slab.h>
> >>
> >> @@ -28,14 +27,29 @@ struct gic_clk_data {
> >> const char *const *clocks;
> >> };
> >>
> >> +struct gic_chip_pm {
> >> + struct gic_chip_data *chip_data;
> >> + const struct gic_clk_data *clk_data;
> >> + struct clk **clk_handle;
> >
> > It is really ugly that clk_handle cannot be bundled with clk_data.
> > Oh well.
>
> I wonder if we should use clk_bulk_data here and then use the clk_bulk
> APIs below? It would simplify the code and given that the clocks are
> well-defined for the GIC (ie. for a given version we know what clocks
> are required) I don't see any harm in doing so.
That'd be fine indeed. It seems to clash with the use devm_clk_get
below, but I don't think the use of the devm* helpers bring much
here.
Thanks,
M.
--
Jazz is not dead, it just smell funny.
