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

[Jianqiang Chen <jianqian@xilinx.com>]

Add Mads.

Thanks,
Jason

> -----Original Message-----
> From: Michal Simek <michal.simek@xilinx.com>
> Sent: Monday, July 19, 2021 12:53 AM
> To: Qiang Yu <yuq825@gmail.com>; Marek Vasut <marex@denx.de>;
> Jianqiang Chen <jianqian@xilinx.com>
> Cc: dri-devel <dri-devel@lists.freedesktop.org>; lima@lists.freedesktop.org;
> Michal Simek <michals@xilinx.com>; Michal Simek <monstr@monstr.eu>
> Subject: Re: [PATCH] drm/lima: Convert to clk_bulk API
> 
> 
> 
> On 7/18/21 4:56 AM, Qiang Yu wrote:
> > On Sat, Jul 17, 2021 at 10:52 PM Marek Vasut <marex@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@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@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/2021071
> >>>> 6182544.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.
> >>>
> >>> 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.

Hi Mads, can you comment on this?

> 
> Thanks,
> Michal