On Tue, Jul 20, 2021 at 12:03 AM Madhurkiran Harikrishnan <[email protected]> wrote: > > 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. > It's not necessary to change the root PLL clock rate to support DVFS. You can also change a divisor to a sub clock if the sub clock is not shared by another module. For example if root PLL is 700MHz, you can set the divisor to GPU clock to 2 to make GPU run at 350MHz. My question is if the GPU_REF_CTRL[13:8] is just such a divisor and can be used to support DVFS (an OPP table in DTS)?
> 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" <[email protected]> wrote: > > Add Mads. > > Thanks, > Jason > > > -----Original Message----- > > From: Michal Simek <[email protected]> > > Sent: Monday, July 19, 2021 12:53 AM > > To: Qiang Yu <[email protected]>; Marek Vasut <[email protected]>; > > Jianqiang Chen <[email protected]> > > Cc: dri-devel <[email protected]>; > [email protected]; > > Michal Simek <[email protected]>; Michal Simek <[email protected]> > > 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 <[email protected]> wrote: > > >> > > >> On 7/17/21 4:21 PM, Qiang Yu wrote: > > >>> On Sat, Jul 17, 2021 at 9:08 PM Marek Vasut <[email protected]> wrote: > > >>>> > > >>>> On 7/17/21 2:34 PM, Qiang Yu wrote: > > >>>>> On Sat, Jul 17, 2021 at 2:20 AM Marek Vasut <[email protected]> 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 > > >>>> [email protected]/ > > >>>> > > >>>>> 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 <[email protected]> > > >>>>>> Cc: Qiang Yu <[email protected]> > > >>>>>> Cc: [email protected] > > >>>>>> --- > > >>>>>> 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 >
