RE: [PATCH v4 4/4] arm64/dts/qcom/sm8250: remove assigned-clock-rate property for mdp clk

[Vinod Polimera <vpolimer@qti.qualcomm.com>]

> WARNING: This email originated from outside of Qualcomm. Please be wary
> of any links or attachments, and do not enable macros.
> 
> On Sat, 5 Mar 2022 at 00:49, Doug Anderson <dianders@chromium.org>
> wrote:
> > On Thu, Mar 3, 2022 at 4:16 PM Dmitry Baryshkov
> > <dmitry.baryshkov@linaro.org> wrote:
> > >
> > > On Fri, 4 Mar 2022 at 02:56, Stephen Boyd <swboyd@chromium.org>
> wrote:
> > > >
> > > > Quoting Dmitry Baryshkov (2022-03-03 15:50:50)
> > > > > On Thu, 3 Mar 2022 at 12:40, Vinod Polimera
> <quic_vpolimer@quicinc.com> wrote:
> > > > > >
> > > > > > 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.
> > > > > >
> > > > > > Drop the assigned clock rate property and vote on the mdp clock as
> per
> > > > > > calculated value during the usecase.
> > > > > >
> > > > > > Fixes: 7c1dffd471("arm64: dts: qcom: sm8250.dtsi: add display
> system nodes")
> > > > >
> > > > > Please remove the Fixes tags from all commits. Otherwise the
> patches
> > > > > might be picked up into earlier kernels, which do not have a patch
> > > > > adding a vote on the MDP clock.
> > > >
> > > > What patch is that? The Fixes tag could point to that commit.
> > >
> > > Please correct me if I'm wrong.
> > > Currently the dtsi enforces bumping the MDP clock when the mdss
> device
> > > is being probed and when the dpu device is being probed.
> > > Later during the DPU lifetime the core_perf would change the clock's
> > > rate as it sees fit according to the CRTC requirements.
> >
> > "Currently" means _before_ ${SUBJECT} patch lands, right? Since
> > ${SUBJECT} patch is removing the bump to max.
> 
> Yes. 'Before this patch'.
> 
> >
> >
> > > However it would happen only when the during the
> > > dpu_crtc_atomic_flush(), before we call this function, the MDP clock
> > > is left in the undetermined state. The power rails controlled by the
> > > opp table are left in the undetermined state.
> > >
> > > I suppose that during the dpu_bind we should bump the clock to the max
> > > possible freq and let dpu_core_perf handle it afterwards.
> >
> > Definitely feels like seeing the clock to something predictable during
> > the initial probe makes sense. If it's just for the initial probe then
> > setting it to max (based on the opp table) seems fine.
> 
> Vinod, could you please implement it?
> 
> > I think an
> > earlier version of this series set it to max every time we did runtime
> > resume. We'd have to have a good reason to do that.
> 
> Yes, this is correct. Based on the comments I had the impression that
> there was a suggestion that the place for the calls was wrong. Most
> probably I was instead projecting my own thoughts.
> 
I had discussed internally with the team. Traditionally, mdp clk vote during
probe/bind is required when display is turned on in bootloader and persists
till first update in kernel. As in chromebook, timing engine will be turned 
off during depthcharge exit and as there is no display transition from 
bootloader to kernel, mdp clk can be voted based on the calculated value 
during framework update and does not required vote during probe/bind.

Thanks,
Vinod.
> --
> With best wishes
> Dmitry