Re: [PATCH] drm/lima: Convert to clk_bulk API

[Marek Vasut <marex@denx.de>]

On 7/17/21 2:34 PM, Qiang Yu wrote:
> On Sat, Jul 17, 2021 at 2:20 AM Marek Vasut <marex@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@denx.de>
>> Cc: Qiang Yu <yuq825@gmail.com>
>> Cc: lima@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.