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/20210716182544.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. Thanks, Michal
