lkml.org 
[lkml]   [2022]   [Mar]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [PATCH v5 1/5] arm64/dts/qcom/sc7280: remove assigned-clock-rate property for mdp clk
Date


> -----Original Message-----
> From: Doug Anderson <dianders@chromium.org>
> Sent: Monday, March 14, 2022 7:28 PM
> To: dmitry.baryshkov@linaro.org
> Cc: Vinod Polimera <vpolimer@qti.qualcomm.com>; Stephen Boyd
> <swboyd@chromium.org>; 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; linux-
> kernel@vger.kernel.org; robdclark@gmail.com; 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.
>
> 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.
>
>
- 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
\
 
 \ /
  Last update: 2022-03-14 15:56    [W:0.092 / U:0.144 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site