[v3,06/15] dt-bindings: display/msm: sc7180-dpu: Describe SM6125

  SM6125 is identical to SM6375 except that while downstream also defines
a throttle clock, its presence results in timeouts whereas SM6375
requires it to not observe any timeouts.  This is represented by
reducing the clock array length to 6 so that it cannot be passed.  Note
that any SoC other than SM6375 (currently SC7180 and SM6350) are
unconstrained and could either pass or leave out this "throttle" clock.

Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
---
 .../devicetree/bindings/display/msm/qcom,sc7180-dpu.yaml   | 14 ++++++++++++++
 1 file changed, 14 insertions(+)
  

On 19/07/2023 00:24, Marijn Suijten wrote:
> SM6125 is identical to SM6375 except that while downstream also defines
> a throttle clock, its presence results in timeouts whereas SM6375
> requires it to not observe any timeouts.  This is represented by
> reducing the clock array length to 6 so that it cannot be passed.  Note
> that any SoC other than SM6375 (currently SC7180 and SM6350) are
> unconstrained and could either pass or leave out this "throttle" clock.

Could you please describe, what kind of timeouts do you observe? Is this 
the DSI underruns issue? If so, it might be fixed by the MDSS 
interconnect fix ([1]).

[1] https://patchwork.freedesktop.org/series/116576/

> 
> Reviewed-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
> ---
>   .../devicetree/bindings/display/msm/qcom,sc7180-dpu.yaml   | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/msm/qcom,sc7180-dpu.yaml b/Documentation/devicetree/bindings/display/msm/qcom,sc7180-dpu.yaml
> index 630b11480496..37f66940c5e3 100644
> --- a/Documentation/devicetree/bindings/display/msm/qcom,sc7180-dpu.yaml
> +++ b/Documentation/devicetree/bindings/display/msm/qcom,sc7180-dpu.yaml
> @@ -15,6 +15,7 @@ properties:
>     compatible:
>       enum:
>         - qcom,sc7180-dpu
> +      - qcom,sm6125-dpu
>         - qcom,sm6350-dpu
>         - qcom,sm6375-dpu
>   
> @@ -73,6 +74,19 @@ allOf:
>           clock-names:
>             minItems: 7
>   
> +  - if:
> +      properties:
> +        compatible:
> +          const: qcom,sm6125-dpu
> +
> +    then:
> +      properties:
> +        clocks:
> +          maxItems: 6
> +
> +        clock-names:
> +          maxItems: 6
> +
>   examples:
>     - |
>       #include <dt-bindings/clock/qcom,dispcc-sc7180.h>
>
  

On 2023-07-19 01:06:03, Dmitry Baryshkov wrote:
> On 19/07/2023 00:24, Marijn Suijten wrote:
> > SM6125 is identical to SM6375 except that while downstream also defines
> > a throttle clock, its presence results in timeouts whereas SM6375
> > requires it to not observe any timeouts.  This is represented by
> > reducing the clock array length to 6 so that it cannot be passed.  Note
> > that any SoC other than SM6375 (currently SC7180 and SM6350) are
> > unconstrained and could either pass or leave out this "throttle" clock.
> 
> Could you please describe, what kind of timeouts do you observe? Is this 
> the DSI underruns issue?

Ping-pong timeouts and low(er) framerate.  However, they were previosuly
not happening on a random boot out of tens... and now I can no longer
reproduce the timeout on 4 consecutive boots after adding the throttle
clock.  Could it perhaps be the power domains and opps that we added in
v2 and v3?

We previously discussed in DMs that the rate was bouncing between 25MHz
and 403MHz without the clock specified, and with it it it got set at 385
or 403MHz.  Now, a month or so later, repeatedly running this command
shows 25MHz when the panel is not being refreshed, and between 337 and
403MHz on modetest -r -v:

    sony-pdx201 ~ $ sudo ./debugcc -p sm6125 gcc_disp_throttle_core_clk
                gcc_disp_throttle_core_clk: 337.848277MHz (337848277Hz)

Either all these boots are flukes, or it is really fixed and this patch
should be revised...

> If so, it might be fixed by the MDSS 
> interconnect fix ([1]).
> 
> [1] https://patchwork.freedesktop.org/series/116576/

Might have an effect but I don't have any interconnects defined in this
SoC DT yet.

- Marijn

> > Reviewed-by: Rob Herring <robh@kernel.org>
> > Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
> > ---
> >   .../devicetree/bindings/display/msm/qcom,sc7180-dpu.yaml   | 14 ++++++++++++++
> >   1 file changed, 14 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/display/msm/qcom,sc7180-dpu.yaml b/Documentation/devicetree/bindings/display/msm/qcom,sc7180-dpu.yaml
> > index 630b11480496..37f66940c5e3 100644
> > --- a/Documentation/devicetree/bindings/display/msm/qcom,sc7180-dpu.yaml
> > +++ b/Documentation/devicetree/bindings/display/msm/qcom,sc7180-dpu.yaml
> > @@ -15,6 +15,7 @@ properties:
> >     compatible:
> >       enum:
> >         - qcom,sc7180-dpu
> > +      - qcom,sm6125-dpu
> >         - qcom,sm6350-dpu
> >         - qcom,sm6375-dpu
> >   
> > @@ -73,6 +74,19 @@ allOf:
> >           clock-names:
> >             minItems: 7
> >   
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          const: qcom,sm6125-dpu
> > +
> > +    then:
> > +      properties:
> > +        clocks:
> > +          maxItems: 6
> > +
> > +        clock-names:
> > +          maxItems: 6
> > +
> >   examples:
> >     - |
> >       #include <dt-bindings/clock/qcom,dispcc-sc7180.h>
> > 
> 
> -- 
> With best wishes
> Dmitry
>
  

On Thu, 20 Jul 2023 at 01:09, Marijn Suijten
<marijn.suijten@somainline.org> wrote:
>
> On 2023-07-19 01:06:03, Dmitry Baryshkov wrote:
> > On 19/07/2023 00:24, Marijn Suijten wrote:
> > > SM6125 is identical to SM6375 except that while downstream also defines
> > > a throttle clock, its presence results in timeouts whereas SM6375
> > > requires it to not observe any timeouts.  This is represented by
> > > reducing the clock array length to 6 so that it cannot be passed.  Note
> > > that any SoC other than SM6375 (currently SC7180 and SM6350) are
> > > unconstrained and could either pass or leave out this "throttle" clock.
> >
> > Could you please describe, what kind of timeouts do you observe? Is this
> > the DSI underruns issue?
>
> Ping-pong timeouts and low(er) framerate.  However, they were previosuly
> not happening on a random boot out of tens... and now I can no longer
> reproduce the timeout on 4 consecutive boots after adding the throttle
> clock.  Could it perhaps be the power domains and opps that we added in
> v2 and v3?

Quite unlikely, but who knows. My main question is whether we should
continue skipping the throttle clocks or if it should be enabled now.

>
> We previously discussed in DMs that the rate was bouncing between 25MHz
> and 403MHz without the clock specified, and with it it it got set at 385
> or 403MHz.  Now, a month or so later, repeatedly running this command
> shows 25MHz when the panel is not being refreshed, and between 337 and
> 403MHz on modetest -r -v:
>
>     sony-pdx201 ~ $ sudo ./debugcc -p sm6125 gcc_disp_throttle_core_clk
>                 gcc_disp_throttle_core_clk: 337.848277MHz (337848277Hz)
>
> Either all these boots are flukes, or it is really fixed and this patch
> should be revised...
>
> > If so, it might be fixed by the MDSS
> > interconnect fix ([1]).
> >
> > [1] https://patchwork.freedesktop.org/series/116576/
>
> Might have an effect but I don't have any interconnects defined in this
> SoC DT yet.
>
> - Marijn
>
> > > Reviewed-by: Rob Herring <robh@kernel.org>
> > > Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
> > > ---
> > >   .../devicetree/bindings/display/msm/qcom,sc7180-dpu.yaml   | 14 ++++++++++++++
> > >   1 file changed, 14 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/display/msm/qcom,sc7180-dpu.yaml b/Documentation/devicetree/bindings/display/msm/qcom,sc7180-dpu.yaml
> > > index 630b11480496..37f66940c5e3 100644
> > > --- a/Documentation/devicetree/bindings/display/msm/qcom,sc7180-dpu.yaml
> > > +++ b/Documentation/devicetree/bindings/display/msm/qcom,sc7180-dpu.yaml
> > > @@ -15,6 +15,7 @@ properties:
> > >     compatible:
> > >       enum:
> > >         - qcom,sc7180-dpu
> > > +      - qcom,sm6125-dpu
> > >         - qcom,sm6350-dpu
> > >         - qcom,sm6375-dpu
> > >
> > > @@ -73,6 +74,19 @@ allOf:
> > >           clock-names:
> > >             minItems: 7
> > >
> > > +  - if:
> > > +      properties:
> > > +        compatible:
> > > +          const: qcom,sm6125-dpu
> > > +
> > > +    then:
> > > +      properties:
> > > +        clocks:
> > > +          maxItems: 6
> > > +
> > > +        clock-names:
> > > +          maxItems: 6
> > > +
> > >   examples:
> > >     - |
> > >       #include <dt-bindings/clock/qcom,dispcc-sc7180.h>
> > >
> >
> > --
> > With best wishes
> > Dmitry
> >
  

On 2023-07-20 01:24:27, Dmitry Baryshkov wrote:
> On Thu, 20 Jul 2023 at 01:09, Marijn Suijten
> <marijn.suijten@somainline.org> wrote:
> >
> > On 2023-07-19 01:06:03, Dmitry Baryshkov wrote:
> > > On 19/07/2023 00:24, Marijn Suijten wrote:
> > > > SM6125 is identical to SM6375 except that while downstream also defines
> > > > a throttle clock, its presence results in timeouts whereas SM6375
> > > > requires it to not observe any timeouts.  This is represented by
> > > > reducing the clock array length to 6 so that it cannot be passed.  Note
> > > > that any SoC other than SM6375 (currently SC7180 and SM6350) are
> > > > unconstrained and could either pass or leave out this "throttle" clock.
> > >
> > > Could you please describe, what kind of timeouts do you observe? Is this
> > > the DSI underruns issue?
> >
> > Ping-pong timeouts and low(er) framerate.  However, they were previosuly
> > not happening on a random boot out of tens... and now I can no longer
> > reproduce the timeout on 4 consecutive boots after adding the throttle
> > clock.  Could it perhaps be the power domains and opps that we added in
> > v2 and v3?
> 
> Quite unlikely, but who knows. My main question is whether we should
> continue skipping the throttle clocks or if it should be enabled now.

And that "main question" could ... drum roll please ... only be answered
by knowing if this got "accidentally" fixed by providing the right PMs
or some other change entirely while I changed base branch and defconfig.
Or if this is just a fluke that persisted multiple boots but will fall
apart in some time and/or when someone else runs this on their device?

- Marijn

<snip>
  

On 2023-07-20 21:10:52, Marijn Suijten wrote:
> On 2023-07-20 01:24:27, Dmitry Baryshkov wrote:
> > On Thu, 20 Jul 2023 at 01:09, Marijn Suijten
> > <marijn.suijten@somainline.org> wrote:
> > >
> > > On 2023-07-19 01:06:03, Dmitry Baryshkov wrote:
> > > > On 19/07/2023 00:24, Marijn Suijten wrote:
> > > > > SM6125 is identical to SM6375 except that while downstream also defines
> > > > > a throttle clock, its presence results in timeouts whereas SM6375
> > > > > requires it to not observe any timeouts.  This is represented by
> > > > > reducing the clock array length to 6 so that it cannot be passed.  Note
> > > > > that any SoC other than SM6375 (currently SC7180 and SM6350) are
> > > > > unconstrained and could either pass or leave out this "throttle" clock.
> > > >
> > > > Could you please describe, what kind of timeouts do you observe? Is this
> > > > the DSI underruns issue?
> > >
> > > Ping-pong timeouts and low(er) framerate.  However, they were previosuly
> > > not happening on a random boot out of tens... and now I can no longer
> > > reproduce the timeout on 4 consecutive boots after adding the throttle
> > > clock.  Could it perhaps be the power domains and opps that we added in
> > > v2 and v3?
> > 
> > Quite unlikely, but who knows. My main question is whether we should
> > continue skipping the throttle clocks or if it should be enabled now.
> 
> And that "main question" could ... drum roll please ... only be answered
> by knowing if this got "accidentally" fixed by providing the right PMs
> or some other change entirely while I changed base branch and defconfig.
> Or if this is just a fluke that persisted multiple boots but will fall
> apart in some time and/or when someone else runs this on their device?

I'm going to write this off as PEBKAC from my past self.  I went back to
an older branch where I recall adding this clock, added it to DT again,
and observed no timeouts or inexplicable behaviour on multiple boots.

Since downstream passes it as well, I'll revise this series for v4 to
require it in dt-bindings, and include it in DT.

- Marijn