[PATCH] drm/lima: Convert to clk_bulk API

[Michal Simek]

On 7/18/21 4:56 AM, Qiang Yu wrote:
> On Sat, Jul 17, 2021 at 10:52 PM Marek Vasut <marex at denx.de> wrote:
>>
>> On 7/17/21 4:21 PM, Qiang Yu wrote:
>>> On Sat, Jul 17, 2021 at 9:08 PM Marek Vasut <marex at denx.de> wrote:
>>>>
>>>> On 7/17/21 2:34 PM, Qiang Yu wrote:
>>>>> On Sat, Jul 17, 2021 at 2:20 AM Marek Vasut <marex at denx.de> wrote:
>>>>>>
>>>>>> Instead of requesting two separate clock and then handling them
>>>>>> separately in various places of the driver, use clk_bulk_*() API.
>>>>>> This permits handling devices with more than "bus"/"core" clock,
>>>>>> like ZynqMP, which has "gpu"/"gpu_pp0"/"gpu_pp1" all as separate
>>>>>> clock.
>>>>>
>>>>> I can't find the ZynqMP DTS file under arch/arm64/boot/dts/xilinx
>>>>> which has mali GPU node with an upstream kernel, where is it?
>>>>
>>>> Posted here:
>>>> https://patchwork.kernel.org/project/linux-arm-kernel/patch/20210716182544.219490-1-marex@denx.de/
>>>>
>>>>> So what's the relationship between "gpu" clk and "gpu_pp0"/"gpu_pp1"
>>>>> clk? Do they need to be controlled separately or we can just control the
>>>>> "gpu" clk? Because the devfreq code just controls a single module clk.
>>>>
>>>> Per the docs, they are separate enable bits and the zynqmp clock
>>>> controller exports them as separate clock, see bits 24..26 here:
>>>>
>>>> https://www.xilinx.com/html_docs/registers/ug1087/crf_apb___gpu_ref_ctrl.html
>>>>
>>>>>> Signed-off-by: Marek Vasut <marex at denx.de>
>>>>>> Cc: Qiang Yu <yuq825 at gmail.com>
>>>>>> Cc: lima at lists.freedesktop.org
>>>>>> ---
>>>>>>    drivers/gpu/drm/lima/lima_devfreq.c | 17 +++++++++---
>>>>>>    drivers/gpu/drm/lima/lima_devfreq.h |  1 +
>>>>>>    drivers/gpu/drm/lima/lima_device.c  | 42 +++++++++++------------------
>>>>>>    drivers/gpu/drm/lima/lima_device.h  |  4 +--
>>>>>>    4 files changed, 32 insertions(+), 32 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/lima/lima_devfreq.c b/drivers/gpu/drm/lima/lima_devfreq.c
>>>>>> index 8989e215dfc9..533b36932f79 100644
>>>>>> --- a/drivers/gpu/drm/lima/lima_devfreq.c
>>>>>> +++ b/drivers/gpu/drm/lima/lima_devfreq.c
>>>>>> @@ -58,7 +58,7 @@ static int lima_devfreq_get_dev_status(struct device *dev,
>>>>>>           struct lima_devfreq *devfreq = &ldev->devfreq;
>>>>>>           unsigned long irqflags;
>>>>>>
>>>>>> -       status->current_frequency = clk_get_rate(ldev->clk_gpu);
>>>>>> +       status->current_frequency = clk_get_rate(devfreq->clk_gpu);
>>>>>>
>>>>>>           spin_lock_irqsave(&devfreq->lock, irqflags);
>>>>>>
>>>>>> @@ -110,12 +110,23 @@ int lima_devfreq_init(struct lima_device *ldev)
>>>>>>           struct lima_devfreq *ldevfreq = &ldev->devfreq;
>>>>>>           struct dev_pm_opp *opp;
>>>>>>           unsigned long cur_freq;
>>>>>> -       int ret;
>>>>>> +       int i, ret;
>>>>>>
>>>>>>           if (!device_property_present(dev, "operating-points-v2"))
>>>>>>                   /* Optional, continue without devfreq */
>>>>>>                   return 0;
>>>>>>
>>>>>> +       /* Find first clock which are not "bus" clock */
>>>>>> +       for (i = 0; i < ldev->nr_clks; i++) {
>>>>>> +               if (!strcmp(ldev->clks[i].id, "bus"))
>>>>>> +                       continue;
>>>>>> +               ldevfreq->clk_gpu = ldev->clks[i].clk;
>>>>>> +               break;
>>>>>> +       }
>>>>>
>>>>> I'd prefer an explicit name for the required clk name. If some DTS has different
>>>>> name other than "core" for the module clk (ie. "gpu"), it should be changed to
>>>>> "core".
>>>>
>>>> The problem here is, the zynqmp has no core clock, it has "gpu and both
>>>> pixel pipes" super-clock-gate which controls everything, and then
>>>> per-pixel-pipe sub-clock-gates.
>>>
>>> So the "gpu" clk can gate both "gpu_pp0" and "gpu_pp1" clk, how about frequency?
>>
>> I don't think it is a good idea to just gate off the root clock while
>> the sub-clock are still enabled. That might lead to latch ups (+CC
>> Michal, he might know more).
>>
>> And who would enable the sub-clock anyway, it should be the GPU driver, no?
>>
> Right, I understand it's not proper either by HW or SW point of view to just
> use root clk gate.
> 
>>> Can we set clock rate for "gpu" then "gpu_pp0" and "gpu_pp1" pass
>>> through the same
>>> rate? If so, "gpu" works just like "core".
>>
>> I don't think the zynqmp is capable of any DVFS on the GPU at all, it
>> just runs at fixed frequency.
> 
> I see the GPU_REF_CTRL register 13:8 is a divisor, is this for all
> "gpu"/"gpu_pp0"/"gpu_pp1" clk rating? If so, can we use it to dynamically
> change the GPU clk freq because other SoC also use system clock
> to do GPU DVFS, see sun8i-h3.dtsi. If we can't then zynqmp won't finish
> lima_devfreq_init() and get here at all because it does not have
> an OPP table.
> 

Jianqiang: Please take a look at this from zynqmp point of view.

Thanks,
Michal