[PATCH v5 1/5] arm64/dts/qcom/sc7280: remove assigned-clock-rate property for mdp clk

[Vinod Polimera]

> -----Original Message-----
> From: Doug Anderson <dianders at chromium.org>
> Sent: Monday, March 14, 2022 7:28 PM
> To: dmitry.baryshkov at linaro.org
> Cc: Vinod Polimera <vpolimer at qti.qualcomm.com>; Stephen Boyd
> <swboyd at chromium.org>; quic_vpolimer <quic_vpolimer at quicinc.com>;
> devicetree at vger.kernel.org; dri-devel at lists.freedesktop.org;
> freedreno at lists.freedesktop.org; linux-arm-msm at vger.kernel.org; linux-
> kernel at vger.kernel.org; robdclark at gmail.com; quic_kalyant
> <quic_kalyant at quicinc.com>
> Subject: Re: [PATCH v5 1/5] arm64/dts/qcom/sc7280: remove assigned-clock-
> rate property for mdp clk
> 
> WARNING: This email originated from outside of Qualcomm. Please be wary
> of any links or attachments, and do not enable macros.
> 
> Hi,
> 
> On Fri, Mar 11, 2022 at 1:22 AM Dmitry Baryshkov
> <dmitry.baryshkov at linaro.org> wrote:
> >
> > On Fri, 11 Mar 2022 at 11:06, Vinod Polimera
> <vpolimer at qti.qualcomm.com> wrote:
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Stephen Boyd <swboyd at chromium.org>
> > > > Sent: Wednesday, March 9, 2022 1:36 AM
> > > > To: quic_vpolimer <quic_vpolimer at quicinc.com>;
> > > > devicetree at vger.kernel.org; dri-devel at lists.freedesktop.org;
> > > > freedreno at lists.freedesktop.org; linux-arm-msm at vger.kernel.org
> > > > Cc: linux-kernel at vger.kernel.org; robdclark at gmail.com;
> > > > dianders at chromium.org; quic_kalyant <quic_kalyant at quicinc.com>
> > > > Subject: Re: [PATCH v5 1/5] arm64/dts/qcom/sc7280: remove assigned-
> clock-
> > > > rate property for mdp clk
> > > >
> > > > WARNING: This email originated from outside of Qualcomm. Please be
> wary
> > > > of any links or attachments, and do not enable macros.
> > > >
> > > > Quoting Vinod Polimera (2022-03-08 08:54:56)
> > > > > Kernel clock driver assumes that initial rate is the
> > > > > max rate for that clock and was not allowing it to scale
> > > > > beyond the assigned clock value.
> > > >
> > > > How? I see ftbl_disp_cc_mdss_mdp_clk_src[] has multiple frequencies
> and
> > > > clk_rcg2_shared_ops so it doesn't look like anything in the clk driver
> > > > is preventing the frequency from changing beyond the assigned value.
> > >
> > > Folowing the comment of Stephen, i have checked a bit more. it appears
> that clock driver is not setting the max clock from assgined clocks, dpu driver
> is doing that.
> > > i am planning to fix it as below.
> > > 1) assign ULONG_MAX to max_rate while initializing clock in dpu driver.
> > > 2) remove unnecessary checks in the core_perf library. If rate doesn't
> match with the entries in the opp table, it will throw error, hence furthur
> checks are not needed.
> > > 3) no changes in dt are required. (we can drop all the posted ones)
> >
> > Why? They made perfect sense. The dts assignments should be replaced
> > by the opp setting in the bind function, as this would also set the
> > performance point of the respective power domain.
> 
> Right. You should still _post_ the dts patches. It's nice to avoid
> unneeded "assigned-clocks" in the dts. The patch description should
> just be clear that it relies on the driver patch and shouldn't land /
> be backported without the driver patch.
> 
> 
- I have checked the latest  msm/next based on your comment and found that disp_io_util file
Is removed. From the latest origin, I have made changes to earlier patch series to address the comments.

> > > Changes :
> > > ```--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> > > @@ -284,17 +284,6 @@ void dpu_core_perf_crtc_release_bw(struct
> drm_crtc *crtc)
> > >         }
> > >  }
> > >
> > > -static int _dpu_core_perf_set_core_clk_rate(struct dpu_kms *kms, u64
> rate)
> > > -{
> > > -       struct dss_clk *core_clk = kms->perf.core_clk;
> > > -
> > > -       if (core_clk->max_rate && (rate > core_clk->max_rate))
> > > -               rate = core_clk->max_rate;
> > > -
> > > -       core_clk->rate = rate;
> > > -       return dev_pm_opp_set_rate(&kms->pdev->dev, core_clk->rate);
> > > -}
> > > -
> > >  static u64 _dpu_core_perf_get_core_clk_rate(struct dpu_kms *kms)
> > >  {
> > >         u64 clk_rate = kms->perf.perf_tune.min_core_clk;
> > > @@ -405,7 +394,7 @@ int dpu_core_perf_crtc_update(struct drm_crtc
> *crtc,
> > >
> > >                 trace_dpu_core_perf_update_clk(kms->dev, stop_req,
> clk_rate);
> > >
> > > -               ret = _dpu_core_perf_set_core_clk_rate(kms, clk_rate);
> > > +               ret = dev_pm_opp_set_rate(&kms->pdev->dev, clk_rate);
> > >                 if (ret) {
> > >                         DPU_ERROR("failed to set %s clock rate %llu\n",
> > >                                         kms->perf.core_clk->clk_name, clk_rate);
> > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_io_util.c
> > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_io_util.c
> >
> > This file has been removed in msm/next
> 
> To echo Dmitry, please make sure that your patch applies to msm-next,
> As I understand it, that means the branch msm-next on:
> 
> https://gitlab.freedesktop.org/drm/msm.git
> 
> -Doug