> -----Original Message-----
> From: Doug Anderson <dianders@xxxxxxxxxxxx>
> Sent: Monday, March 14, 2022 7:28 PM
> To: dmitry.baryshkov@xxxxxxxxxx
> Cc: Vinod Polimera <vpolimer@xxxxxxxxxxxxxxxx>; Stephen Boyd
> <swboyd@xxxxxxxxxxxx>; quic_vpolimer <quic_vpolimer@xxxxxxxxxxx>;
> devicetree@xxxxxxxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx;
> freedreno@xxxxxxxxxxxxxxxxxxxxx; linux-arm-msm@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; robdclark@xxxxxxxxx; quic_kalyant
> <quic_kalyant@xxxxxxxxxxx>
> 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@xxxxxxxxxx> wrote:
> >
> > On Fri, 11 Mar 2022 at 11:06, Vinod Polimera
> <vpolimer@xxxxxxxxxxxxxxxx> wrote:
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Stephen Boyd <swboyd@xxxxxxxxxxxx>
> > > > Sent: Wednesday, March 9, 2022 1:36 AM
> > > > To: quic_vpolimer <quic_vpolimer@xxxxxxxxxxx>;
> > > > devicetree@xxxxxxxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx;
> > > > freedreno@xxxxxxxxxxxxxxxxxxxxx; linux-arm-msm@xxxxxxxxxxxxxxx
> > > > Cc: linux-kernel@xxxxxxxxxxxxxxx; robdclark@xxxxxxxxx;
> > > > dianders@xxxxxxxxxxxx; quic_kalyant <quic_kalyant@xxxxxxxxxxx>
> > > > 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
