[v1] arm64/dts/qcom/sc7280: update mdp clk to max supported value to support higher refresh rates

[v1] arm64/dts/qcom/sc7280: update mdp clk to max supported value to support higher refresh rates

[Vinod Polimera]

Panels with higher refresh rate will need mdp clk above 300Mhz.
Select max frequency for mdp clock during bootup, dpu driver will
scale down the clock as per usecase when first update from the framework is received.

Signed-off-by: Vinod Polimera <quic_vpolimer@quicinc.com>
---
 arch/arm64/boot/dts/qcom/sc7280.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
index baf1653..7af96fc 100644
--- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
@@ -2895,7 +2895,7 @@
 				assigned-clocks = <&dispcc DISP_CC_MDSS_MDP_CLK>,
 						<&dispcc DISP_CC_MDSS_VSYNC_CLK>,
 						<&dispcc DISP_CC_MDSS_AHB_CLK>;
-				assigned-clock-rates = <300000000>,
+				assigned-clock-rates = <506666667>,
 							<19200000>,
 							<19200000>;
 				operating-points-v2 = <&mdp_opp_table>;
-- 
2.7.4

[v1] arm64/dts/qcom/sc7280: update mdp clk to max supported value to support higher refresh rates

[Vinod Polimera]

Panels with higher refresh rate will need mdp clk above 300Mhz.
Select max frequency for mdp clock during bootup, dpu driver will
scale down the clock as per usecase when first update from the framework is received.

Signed-off-by: Vinod Polimera <quic_vpolimer@quicinc.com>
---
 arch/arm64/boot/dts/qcom/sc7280.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
index baf1653..7af96fc 100644
--- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
@@ -2895,7 +2895,7 @@
 				assigned-clocks = <&dispcc DISP_CC_MDSS_MDP_CLK>,
 						<&dispcc DISP_CC_MDSS_VSYNC_CLK>,
 						<&dispcc DISP_CC_MDSS_AHB_CLK>;
-				assigned-clock-rates = <300000000>,
+				assigned-clock-rates = <506666667>,
 							<19200000>,
 							<19200000>;
 				operating-points-v2 = <&mdp_opp_table>;
-- 
2.7.4

Re: [v1] arm64/dts/qcom/sc7280: update mdp clk to max supported value to support higher refresh rates

[Stephen Boyd]

Quoting Vinod Polimera (2022-02-21 05:12:06)
> Panels with higher refresh rate will need mdp clk above 300Mhz.
> Select max frequency for mdp clock during bootup, dpu driver will
> scale down the clock as per usecase when first update from the framework is received.
>
> Signed-off-by: Vinod Polimera <quic_vpolimer@quicinc.com>

Please add a Fixes tag.

> ---
>  arch/arm64/boot/dts/qcom/sc7280.dtsi | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> index baf1653..7af96fc 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> @@ -2895,7 +2895,7 @@
>                                 assigned-clocks = <&dispcc DISP_CC_MDSS_MDP_CLK>,
>                                                 <&dispcc DISP_CC_MDSS_VSYNC_CLK>,
>                                                 <&dispcc DISP_CC_MDSS_AHB_CLK>;
> -                               assigned-clock-rates = <300000000>,
> +                               assigned-clock-rates = <506666667>,

Why not simply remove the clock assignment and set the rate based on the
OPP when the driver probes?

>                                                         <19200000>,
>                                                         <19200000>;
>                                 operating-points-v2 = <&mdp_opp_table>;

Re: [v1] arm64/dts/qcom/sc7280: update mdp clk to max supported value to support higher refresh rates

[Stephen Boyd]

Quoting Vinod Polimera (2022-02-21 05:12:06)
> Panels with higher refresh rate will need mdp clk above 300Mhz.
> Select max frequency for mdp clock during bootup, dpu driver will
> scale down the clock as per usecase when first update from the framework is received.
>
> Signed-off-by: Vinod Polimera <quic_vpolimer@quicinc.com>

Please add a Fixes tag.

> ---
>  arch/arm64/boot/dts/qcom/sc7280.dtsi | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> index baf1653..7af96fc 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> @@ -2895,7 +2895,7 @@
>                                 assigned-clocks = <&dispcc DISP_CC_MDSS_MDP_CLK>,
>                                                 <&dispcc DISP_CC_MDSS_VSYNC_CLK>,
>                                                 <&dispcc DISP_CC_MDSS_AHB_CLK>;
> -                               assigned-clock-rates = <300000000>,
> +                               assigned-clock-rates = <506666667>,

Why not simply remove the clock assignment and set the rate based on the
OPP when the driver probes?

>                                                         <19200000>,
>                                                         <19200000>;
>                                 operating-points-v2 = <&mdp_opp_table>;

Re: [v1] arm64/dts/qcom/sc7280: update mdp clk to max supported value to support higher refresh rates

[Doug Anderson]

Hi,

On Tue, Feb 22, 2022 at 12:58 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Vinod Polimera (2022-02-21 05:12:06)
> > Panels with higher refresh rate will need mdp clk above 300Mhz.
> > Select max frequency for mdp clock during bootup, dpu driver will
> > scale down the clock as per usecase when first update from the framework is received.
> >
> > Signed-off-by: Vinod Polimera <quic_vpolimer@quicinc.com>
>
> Please add a Fixes tag.
>
> > ---
> >  arch/arm64/boot/dts/qcom/sc7280.dtsi | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> > index baf1653..7af96fc 100644
> > --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> > @@ -2895,7 +2895,7 @@
> >                                 assigned-clocks = <&dispcc DISP_CC_MDSS_MDP_CLK>,
> >                                                 <&dispcc DISP_CC_MDSS_VSYNC_CLK>,
> >                                                 <&dispcc DISP_CC_MDSS_AHB_CLK>;
> > -                               assigned-clock-rates = <300000000>,
> > +                               assigned-clock-rates = <506666667>,
>
> Why not simply remove the clock assignment and set the rate based on the
> OPP when the driver probes?

I was curious so I dug. It turns out that it _is_ using the OPP. It's
just that the kernel driver currently assumes that the initial rate is
the max rate. :-P You can actually see in msm_dss_parse_clock() that
it walks through each of its clocks at boot and records the boot rate
and stashes it as the "max_rate". That's not a scheme I've seen done
commonly, so if nothing else it deserves a comment in the commit
message.

One other note is that I think there are _two_ places in the dtsi that
are setting this same clock rate, right? The parent node `mdss`, which
you're not touching, and the child `mdss_mdp`, which you are touching.
Seems like you should just do it in one place. If it needs to be done
by the parent then the child could just assume that the clock has
already been set by the parent.

-Doug

Re: [v1] arm64/dts/qcom/sc7280: update mdp clk to max supported value to support higher refresh rates

[Doug Anderson]

Hi,

On Tue, Feb 22, 2022 at 12:58 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Vinod Polimera (2022-02-21 05:12:06)
> > Panels with higher refresh rate will need mdp clk above 300Mhz.
> > Select max frequency for mdp clock during bootup, dpu driver will
> > scale down the clock as per usecase when first update from the framework is received.
> >
> > Signed-off-by: Vinod Polimera <quic_vpolimer@quicinc.com>
>
> Please add a Fixes tag.
>
> > ---
> >  arch/arm64/boot/dts/qcom/sc7280.dtsi | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> > index baf1653..7af96fc 100644
> > --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> > @@ -2895,7 +2895,7 @@
> >                                 assigned-clocks = <&dispcc DISP_CC_MDSS_MDP_CLK>,
> >                                                 <&dispcc DISP_CC_MDSS_VSYNC_CLK>,
> >                                                 <&dispcc DISP_CC_MDSS_AHB_CLK>;
> > -                               assigned-clock-rates = <300000000>,
> > +                               assigned-clock-rates = <506666667>,
>
> Why not simply remove the clock assignment and set the rate based on the
> OPP when the driver probes?

I was curious so I dug. It turns out that it _is_ using the OPP. It's
just that the kernel driver currently assumes that the initial rate is
the max rate. :-P You can actually see in msm_dss_parse_clock() that
it walks through each of its clocks at boot and records the boot rate
and stashes it as the "max_rate". That's not a scheme I've seen done
commonly, so if nothing else it deserves a comment in the commit
message.

One other note is that I think there are _two_ places in the dtsi that
are setting this same clock rate, right? The parent node `mdss`, which
you're not touching, and the child `mdss_mdp`, which you are touching.
Seems like you should just do it in one place. If it needs to be done
by the parent then the child could just assume that the clock has
already been set by the parent.

-Doug

Re: [v1] arm64/dts/qcom/sc7280: update mdp clk to max supported value to support higher refresh rates

[Stephen Boyd]

Quoting Doug Anderson (2022-02-22 13:25:05)
> Hi,
>
> On Tue, Feb 22, 2022 at 12:58 PM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > Quoting Vinod Polimera (2022-02-21 05:12:06)
> > > Panels with higher refresh rate will need mdp clk above 300Mhz.
> > > Select max frequency for mdp clock during bootup, dpu driver will
> > > scale down the clock as per usecase when first update from the framework is received.
> > >
> > > Signed-off-by: Vinod Polimera <quic_vpolimer@quicinc.com>
> >
> > Please add a Fixes tag.
> >
> > > ---
> > >  arch/arm64/boot/dts/qcom/sc7280.dtsi | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> > > index baf1653..7af96fc 100644
> > > --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> > > +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> > > @@ -2895,7 +2895,7 @@
> > >                                 assigned-clocks = <&dispcc DISP_CC_MDSS_MDP_CLK>,
> > >                                                 <&dispcc DISP_CC_MDSS_VSYNC_CLK>,
> > >                                                 <&dispcc DISP_CC_MDSS_AHB_CLK>;
> > > -                               assigned-clock-rates = <300000000>,
> > > +                               assigned-clock-rates = <506666667>,
> >
> > Why not simply remove the clock assignment and set the rate based on the
> > OPP when the driver probes?
>
> I was curious so I dug. It turns out that it _is_ using the OPP. It's
> just that the kernel driver currently assumes that the initial rate is
> the max rate. :-P You can actually see in msm_dss_parse_clock() that
> it walks through each of its clocks at boot and records the boot rate
> and stashes it as the "max_rate". That's not a scheme I've seen done
> commonly, so if nothing else it deserves a comment in the commit
> message.

That sounds like a scheme to detect the max frequency of the clk before
an OPP table is written. It would be better to convert that code to use
OPP tables if available and then drop this assigned clock property from
the DT (in both places).

>
> One other note is that I think there are _two_ places in the dtsi that
> are setting this same clock rate, right? The parent node `mdss`, which
> you're not touching, and the child `mdss_mdp`, which you are touching.
> Seems like you should just do it in one place. If it needs to be done
> by the parent then the child could just assume that the clock has
> already been set by the parent.
>

I see that it's this way on sc7180 too, which is sad but it seems nobody
noticed.

Re: [v1] arm64/dts/qcom/sc7280: update mdp clk to max supported value to support higher refresh rates

[Stephen Boyd]

Quoting Doug Anderson (2022-02-22 13:25:05)
> Hi,
>
> On Tue, Feb 22, 2022 at 12:58 PM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > Quoting Vinod Polimera (2022-02-21 05:12:06)
> > > Panels with higher refresh rate will need mdp clk above 300Mhz.
> > > Select max frequency for mdp clock during bootup, dpu driver will
> > > scale down the clock as per usecase when first update from the framework is received.
> > >
> > > Signed-off-by: Vinod Polimera <quic_vpolimer@quicinc.com>
> >
> > Please add a Fixes tag.
> >
> > > ---
> > >  arch/arm64/boot/dts/qcom/sc7280.dtsi | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> > > index baf1653..7af96fc 100644
> > > --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> > > +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> > > @@ -2895,7 +2895,7 @@
> > >                                 assigned-clocks = <&dispcc DISP_CC_MDSS_MDP_CLK>,
> > >                                                 <&dispcc DISP_CC_MDSS_VSYNC_CLK>,
> > >                                                 <&dispcc DISP_CC_MDSS_AHB_CLK>;
> > > -                               assigned-clock-rates = <300000000>,
> > > +                               assigned-clock-rates = <506666667>,
> >
> > Why not simply remove the clock assignment and set the rate based on the
> > OPP when the driver probes?
>
> I was curious so I dug. It turns out that it _is_ using the OPP. It's
> just that the kernel driver currently assumes that the initial rate is
> the max rate. :-P You can actually see in msm_dss_parse_clock() that
> it walks through each of its clocks at boot and records the boot rate
> and stashes it as the "max_rate". That's not a scheme I've seen done
> commonly, so if nothing else it deserves a comment in the commit
> message.

That sounds like a scheme to detect the max frequency of the clk before
an OPP table is written. It would be better to convert that code to use
OPP tables if available and then drop this assigned clock property from
the DT (in both places).

>
> One other note is that I think there are _two_ places in the dtsi that
> are setting this same clock rate, right? The parent node `mdss`, which
> you're not touching, and the child `mdss_mdp`, which you are touching.
> Seems like you should just do it in one place. If it needs to be done
> by the parent then the child could just assume that the clock has
> already been set by the parent.
>

I see that it's this way on sc7180 too, which is sad but it seems nobody
noticed.

Re: [v1] arm64/dts/qcom/sc7280: update mdp clk to max supported value to support higher refresh rates

[Doug Anderson]

Hi,

On Tue, Feb 22, 2022 at 1:46 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Doug Anderson (2022-02-22 13:25:05)
> > Hi,
> >
> > On Tue, Feb 22, 2022 at 12:58 PM Stephen Boyd <swboyd@chromium.org> wrote:
> > >
> > > Quoting Vinod Polimera (2022-02-21 05:12:06)
> > > > Panels with higher refresh rate will need mdp clk above 300Mhz.
> > > > Select max frequency for mdp clock during bootup, dpu driver will
> > > > scale down the clock as per usecase when first update from the framework is received.
> > > >
> > > > Signed-off-by: Vinod Polimera <quic_vpolimer@quicinc.com>
> > >
> > > Please add a Fixes tag.
> > >
> > > > ---
> > > >  arch/arm64/boot/dts/qcom/sc7280.dtsi | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> > > > index baf1653..7af96fc 100644
> > > > --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> > > > +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> > > > @@ -2895,7 +2895,7 @@
> > > >                                 assigned-clocks = <&dispcc DISP_CC_MDSS_MDP_CLK>,
> > > >                                                 <&dispcc DISP_CC_MDSS_VSYNC_CLK>,
> > > >                                                 <&dispcc DISP_CC_MDSS_AHB_CLK>;
> > > > -                               assigned-clock-rates = <300000000>,
> > > > +                               assigned-clock-rates = <506666667>,
> > >
> > > Why not simply remove the clock assignment and set the rate based on the
> > > OPP when the driver probes?
> >
> > I was curious so I dug. It turns out that it _is_ using the OPP. It's
> > just that the kernel driver currently assumes that the initial rate is
> > the max rate. :-P You can actually see in msm_dss_parse_clock() that
> > it walks through each of its clocks at boot and records the boot rate
> > and stashes it as the "max_rate". That's not a scheme I've seen done
> > commonly, so if nothing else it deserves a comment in the commit
> > message.
>
> That sounds like a scheme to detect the max frequency of the clk before
> an OPP table is written. It would be better to convert that code to use
> OPP tables if available and then drop this assigned clock property from
> the DT (in both places).

Ah, good point! You could just check what the max OPP table rate is.
Then you don't need to worry about specifying the same clock rate
twice.


> > One other note is that I think there are _two_ places in the dtsi that
> > are setting this same clock rate, right? The parent node `mdss`, which
> > you're not touching, and the child `mdss_mdp`, which you are touching.
> > Seems like you should just do it in one place. If it needs to be done
> > by the parent then the child could just assume that the clock has
> > already been set by the parent.
> >
>
> I see that it's this way on sc7180 too, which is sad but it seems nobody
> noticed.

Never too late to fix it! :-)

-Doug

Re: [v1] arm64/dts/qcom/sc7280: update mdp clk to max supported value to support higher refresh rates

[Doug Anderson]

Hi,

On Tue, Feb 22, 2022 at 1:46 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Doug Anderson (2022-02-22 13:25:05)
> > Hi,
> >
> > On Tue, Feb 22, 2022 at 12:58 PM Stephen Boyd <swboyd@chromium.org> wrote:
> > >
> > > Quoting Vinod Polimera (2022-02-21 05:12:06)
> > > > Panels with higher refresh rate will need mdp clk above 300Mhz.
> > > > Select max frequency for mdp clock during bootup, dpu driver will
> > > > scale down the clock as per usecase when first update from the framework is received.
> > > >
> > > > Signed-off-by: Vinod Polimera <quic_vpolimer@quicinc.com>
> > >
> > > Please add a Fixes tag.
> > >
> > > > ---
> > > >  arch/arm64/boot/dts/qcom/sc7280.dtsi | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> > > > index baf1653..7af96fc 100644
> > > > --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> > > > +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> > > > @@ -2895,7 +2895,7 @@
> > > >                                 assigned-clocks = <&dispcc DISP_CC_MDSS_MDP_CLK>,
> > > >                                                 <&dispcc DISP_CC_MDSS_VSYNC_CLK>,
> > > >                                                 <&dispcc DISP_CC_MDSS_AHB_CLK>;
> > > > -                               assigned-clock-rates = <300000000>,
> > > > +                               assigned-clock-rates = <506666667>,
> > >
> > > Why not simply remove the clock assignment and set the rate based on the
> > > OPP when the driver probes?
> >
> > I was curious so I dug. It turns out that it _is_ using the OPP. It's
> > just that the kernel driver currently assumes that the initial rate is
> > the max rate. :-P You can actually see in msm_dss_parse_clock() that
> > it walks through each of its clocks at boot and records the boot rate
> > and stashes it as the "max_rate". That's not a scheme I've seen done
> > commonly, so if nothing else it deserves a comment in the commit
> > message.
>
> That sounds like a scheme to detect the max frequency of the clk before
> an OPP table is written. It would be better to convert that code to use
> OPP tables if available and then drop this assigned clock property from
> the DT (in both places).

Ah, good point! You could just check what the max OPP table rate is.
Then you don't need to worry about specifying the same clock rate
twice.


> > One other note is that I think there are _two_ places in the dtsi that
> > are setting this same clock rate, right? The parent node `mdss`, which
> > you're not touching, and the child `mdss_mdp`, which you are touching.
> > Seems like you should just do it in one place. If it needs to be done
> > by the parent then the child could just assume that the clock has
> > already been set by the parent.
> >
>
> I see that it's this way on sc7180 too, which is sad but it seems nobody
> noticed.

Never too late to fix it! :-)

-Doug