On 21/03/2019 10:46, Marc Zyngier wrote:
> 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).
I guess I hope that that would never be an issue.
>>
>>> 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.
There are devm_clk_bulk APIs available and so we can use them there as well.
Cheers
Jon
--
nvpublic
