Hi, I had created a patch sometimes ago to test on our platform. That is correct, we have three clocks going to gp,pp0 and pp1 (Although all are at same rate). DVFS is not supported primarily because at zynqmp clocks are shared, for example it could come from VPLL or IOPLL. All zynqmp specific patches can be found here: https://github.com/Xilinx/meta-xilinx/tree/rel-v2021.1/meta-xilinx-bsp/recipes-graphics/mali/kernel-module-mali -Mads On 7/19/21, 10:38 AM, "Jianqiang Chen" <jianqian@xxxxxxxxxx> wrote: Add Mads. Thanks, Jason > -----Original Message----- > From: Michal Simek <michal.simek@xxxxxxxxxx> > Sent: Monday, July 19, 2021 12:53 AM > To: Qiang Yu <yuq825@xxxxxxxxx>; Marek Vasut <marex@xxxxxxx>; > Jianqiang Chen <jianqian@xxxxxxxxxx> > Cc: dri-devel <dri-devel@xxxxxxxxxxxxxxxxxxxxx>; lima@xxxxxxxxxxxxxxxxxxxxx; > Michal Simek <michals@xxxxxxxxxx>; Michal Simek <monstr@xxxxxxxxx> > 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@xxxxxxx> wrote: > >> > >> On 7/17/21 4:21 PM, Qiang Yu wrote: > >>> On Sat, Jul 17, 2021 at 9:08 PM Marek Vasut <marex@xxxxxxx> wrote: > >>>> > >>>> On 7/17/21 2:34 PM, Qiang Yu wrote: > >>>>> On Sat, Jul 17, 2021 at 2:20 AM Marek Vasut <marex@xxxxxxx> 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@xxxxxxx/ > >>>> > >>>>> 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@xxxxxxx> > >>>>>> Cc: Qiang Yu <yuq825@xxxxxxxxx> > >>>>>> Cc: lima@xxxxxxxxxxxxxxxxxxxxx > >>>>>> --- > >>>>>> 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
Attachment:
0001-lima-Add-ZynqMP-specific-changes-for-Lima.patch
Description: 0001-lima-Add-ZynqMP-specific-changes-for-Lima.patch
