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

[Doug Anderson <dianders@chromium.org>]

Hi,

On Fri, Mar 11, 2022 at 1:22 AM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On Fri, 11 Mar 2022 at 11:06, Vinod Polimera <vpolimer@qti.qualcomm.com> wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: Stephen Boyd <swboyd@chromium.org>
> > > Sent: Wednesday, March 9, 2022 1:36 AM
> > > To: quic_vpolimer <quic_vpolimer@quicinc.com>;
> > > devicetree@vger.kernel.org; dri-devel@lists.freedesktop.org;
> > > freedreno@lists.freedesktop.org; linux-arm-msm@vger.kernel.org
> > > Cc: linux-kernel@vger.kernel.org; robdclark@gmail.com;
> > > dianders@chromium.org; quic_kalyant <quic_kalyant@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.


> > 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