[v2,5/6] drm/panel: st7703: Drive XBD599 panel at higher clock rate

  This panel is used in the pinephone that runs on a Allwinner A64 SOC.
The SOC requires pll-mipi to run at more than 500 MHz.

This is the relevant clock tree:
 pll-mipi
    tcon0
       tcon-data-clock

tcon-data-clock has to run at 1/4 the DSI per-lane bit rate. The XBD599
has 24 bpp and 4 lanes. Therefore, the resulting requested
tcon-data-clock rate is:
    crtc_clock * 1000 * (24 / 4) / 4

tcon-data-clock runs at tcon0 / 4 (fixed divisor), so it requests a
parent rate of
    4 * (crtc_clock * 1000 * (24 / 4) / 4)

Since tcon0 is a ccu_mux, the rate of tcon0 equals the rate of pll-mipi.

pll-mipi's constraint to run at 500MHz or higher forces us to have a
crtc_clock >= 83333 kHz if we want a 60 Hz vertical refresh rate.

Change [hv]sync_(start|end) so that we reach a clock rate of 83502 kHz
so that it is high enough to align with pll-pipi limits.

Signed-off-by: Frank Oltmanns <frank@oltmanns.dev>
---
 drivers/gpu/drm/panel/panel-sitronix-st7703.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)
  

Dne ponedeljek, 05. februar 2024 ob 16:22:28 CET je Frank Oltmanns napisal(a):
> This panel is used in the pinephone that runs on a Allwinner A64 SOC.
> The SOC requires pll-mipi to run at more than 500 MHz.
> 
> This is the relevant clock tree:
>  pll-mipi
>     tcon0
>        tcon-data-clock
> 
> tcon-data-clock has to run at 1/4 the DSI per-lane bit rate. The XBD599
> has 24 bpp and 4 lanes. Therefore, the resulting requested
> tcon-data-clock rate is:
>     crtc_clock * 1000 * (24 / 4) / 4
> 
> tcon-data-clock runs at tcon0 / 4 (fixed divisor), so it requests a
> parent rate of
>     4 * (crtc_clock * 1000 * (24 / 4) / 4)
> 
> Since tcon0 is a ccu_mux, the rate of tcon0 equals the rate of pll-mipi.
> 
> pll-mipi's constraint to run at 500MHz or higher forces us to have a
> crtc_clock >= 83333 kHz if we want a 60 Hz vertical refresh rate.
> 
> Change [hv]sync_(start|end) so that we reach a clock rate of 83502 kHz
> so that it is high enough to align with pll-pipi limits.

Typo: pll-pipi -> pll-mipi

Best regards,
Jernej

> 
> Signed-off-by: Frank Oltmanns <frank@oltmanns.dev>
> ---
>  drivers/gpu/drm/panel/panel-sitronix-st7703.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7703.c b/drivers/gpu/drm/panel/panel-sitronix-st7703.c
> index b55bafd1a8be..6886fd7f765e 100644
> --- a/drivers/gpu/drm/panel/panel-sitronix-st7703.c
> +++ b/drivers/gpu/drm/panel/panel-sitronix-st7703.c
> @@ -320,14 +320,14 @@ static int xbd599_init_sequence(struct st7703 *ctx)
>  
>  static const struct drm_display_mode xbd599_mode = {
>  	.hdisplay    = 720,
> -	.hsync_start = 720 + 40,
> -	.hsync_end   = 720 + 40 + 40,
> -	.htotal	     = 720 + 40 + 40 + 40,
> +	.hsync_start = 720 + 65,
> +	.hsync_end   = 720 + 65 + 65,
> +	.htotal      = 720 + 65 + 65 + 65,
>  	.vdisplay    = 1440,
> -	.vsync_start = 1440 + 18,
> -	.vsync_end   = 1440 + 18 + 10,
> -	.vtotal	     = 1440 + 18 + 10 + 17,
> -	.clock	     = 69000,
> +	.vsync_start = 1440 + 30,
> +	.vsync_end   = 1440 + 30 + 22,
> +	.vtotal	     = 1440 + 30 + 22 + 29,
> +	.clock	     = (720 + 65 + 65 + 65) * (1440 + 30 + 22 + 29) * 60 / 1000,
>  	.flags	     = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC,
>  	.width_mm    = 68,
>  	.height_mm   = 136,
> 
>
  

Hi Frank,

On Mon, Feb 05, 2024 at 04:22:28PM +0100, Frank Oltmanns wrote:
> This panel is used in the pinephone that runs on a Allwinner A64 SOC.
> The SOC requires pll-mipi to run at more than 500 MHz.
> 
> This is the relevant clock tree:
>  pll-mipi
>     tcon0
>        tcon-data-clock
> 
> tcon-data-clock has to run at 1/4 the DSI per-lane bit rate. The XBD599
> has 24 bpp and 4 lanes. Therefore, the resulting requested
> tcon-data-clock rate is:
>     crtc_clock * 1000 * (24 / 4) / 4
> 
> tcon-data-clock runs at tcon0 / 4 (fixed divisor), so it requests a
> parent rate of
>     4 * (crtc_clock * 1000 * (24 / 4) / 4)
> 
> Since tcon0 is a ccu_mux, the rate of tcon0 equals the rate of pll-mipi.
> 
> pll-mipi's constraint to run at 500MHz or higher forces us to have a
> crtc_clock >= 83333 kHz if we want a 60 Hz vertical refresh rate.
> 
> Change [hv]sync_(start|end) so that we reach a clock rate of 83502 kHz
> so that it is high enough to align with pll-pipi limits.
> 
> Signed-off-by: Frank Oltmanns <frank@oltmanns.dev>

That commit log is great, but it's kind of off-topic. It's a panel
driver, it can be used on any MIPI-DSI controller, the only relevant
information there should be the panel timings required in the datasheet.

The PLL setup is something for the MIPI-DSI driver to adjust, not for
the panel to care for.

Maxime
  

On 2024-02-08 at 20:05:08 +0100, Maxime Ripard <mripard@kernel.org> wrote:
> [[PGP Signed Part:Undecided]]
> Hi Frank,
>
> On Mon, Feb 05, 2024 at 04:22:28PM +0100, Frank Oltmanns wrote:
>> This panel is used in the pinephone that runs on a Allwinner A64 SOC.
>> The SOC requires pll-mipi to run at more than 500 MHz.
>>
>> This is the relevant clock tree:
>>  pll-mipi
>>     tcon0
>>        tcon-data-clock
>>
>> tcon-data-clock has to run at 1/4 the DSI per-lane bit rate. The XBD599
>> has 24 bpp and 4 lanes. Therefore, the resulting requested
>> tcon-data-clock rate is:
>>     crtc_clock * 1000 * (24 / 4) / 4
>>
>> tcon-data-clock runs at tcon0 / 4 (fixed divisor), so it requests a
>> parent rate of
>>     4 * (crtc_clock * 1000 * (24 / 4) / 4)
>>
>> Since tcon0 is a ccu_mux, the rate of tcon0 equals the rate of pll-mipi.
>>
>> pll-mipi's constraint to run at 500MHz or higher forces us to have a
>> crtc_clock >= 83333 kHz if we want a 60 Hz vertical refresh rate.
>>
>> Change [hv]sync_(start|end) so that we reach a clock rate of 83502 kHz
>> so that it is high enough to align with pll-pipi limits.
>>
>> Signed-off-by: Frank Oltmanns <frank@oltmanns.dev>
>
> That commit log is great, but it's kind of off-topic. It's a panel
> driver, it can be used on any MIPI-DSI controller, the only relevant
> information there should be the panel timings required in the datasheet.
>
> The PLL setup is something for the MIPI-DSI driver to adjust, not for
> the panel to care for.
>

I absolutely agree. It even was the reason for my submission of a
sunxi-ng patch series last year that was accepted, to make pll-mipi more
flexible. :)

The only remaining option I currently see for adjusting the sunxi-ng
driver to further accomodate the panel, is trying to use a higher
divisor than 4 for calculating tcon-data-clock from tcon0. I remember
reading a discussion about this, but as far as I remember that proposal
was rejected (by you, IIRC).

While I appreciate other suggestion as well, I'll look into options for
using a different divisor than 4.

Best regards,
  Frank

>
> Maxime
>
> [[End of PGP Signed Part]]
  

On 2024-02-11 at 16:42:43 +0100, Frank Oltmanns <frank@oltmanns.dev> wrote:
> On 2024-02-08 at 20:05:08 +0100, Maxime Ripard <mripard@kernel.org> wrote:
>> [[PGP Signed Part:Undecided]]
>> Hi Frank,
>>
>> On Mon, Feb 05, 2024 at 04:22:28PM +0100, Frank Oltmanns wrote:
>>> This panel is used in the pinephone that runs on a Allwinner A64 SOC.
>>> The SOC requires pll-mipi to run at more than 500 MHz.
>>>
>>> This is the relevant clock tree:
>>>  pll-mipi
>>>     tcon0
>>>        tcon-data-clock
>>>
>>> tcon-data-clock has to run at 1/4 the DSI per-lane bit rate. The XBD599
>>> has 24 bpp and 4 lanes. Therefore, the resulting requested
>>> tcon-data-clock rate is:
>>>     crtc_clock * 1000 * (24 / 4) / 4
>>>
>>> tcon-data-clock runs at tcon0 / 4 (fixed divisor), so it requests a
>>> parent rate of
>>>     4 * (crtc_clock * 1000 * (24 / 4) / 4)
>>>
>>> Since tcon0 is a ccu_mux, the rate of tcon0 equals the rate of pll-mipi.
>>>
>>> pll-mipi's constraint to run at 500MHz or higher forces us to have a
>>> crtc_clock >= 83333 kHz if we want a 60 Hz vertical refresh rate.
>>>
>>> Change [hv]sync_(start|end) so that we reach a clock rate of 83502 kHz
>>> so that it is high enough to align with pll-pipi limits.
>>>
>>> Signed-off-by: Frank Oltmanns <frank@oltmanns.dev>
>>
>> That commit log is great, but it's kind of off-topic. It's a panel
>> driver, it can be used on any MIPI-DSI controller, the only relevant
>> information there should be the panel timings required in the datasheet.
>>
>> The PLL setup is something for the MIPI-DSI driver to adjust, not for
>> the panel to care for.
>>
>
> I absolutely agree. It even was the reason for my submission of a
> sunxi-ng patch series last year that was accepted, to make pll-mipi more
> flexible. :)
>
> The only remaining option I currently see for adjusting the sunxi-ng
> driver to further accomodate the panel, is trying to use a higher
> divisor than 4 for calculating tcon-data-clock from tcon0. I remember
> reading a discussion about this, but as far as I remember that proposal
> was rejected (by you, IIRC).
>
> While I appreciate other suggestion as well, I'll look into options for
> using a different divisor than 4.

I tried the following:
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
@@ -391,7 +391,7 @@ static void sun4i_tcon0_mode_set_cpu(struct sun4i_tcon *tcon,
         * dclk is required to run at 1/4 the DSI per-lane bit rate.
         */
        tcon->dclk_min_div = SUN6I_DSI_TCON_DIV;
-       tcon->dclk_max_div = SUN6I_DSI_TCON_DIV;
+       tcon->dclk_max_div = 127;
        clk_set_rate(tcon->dclk, mode->crtc_clock * 1000 * (bpp / lanes)
                                                  / SUN6I_DSI_TCON_DIV);

On the pinephone, this selects a divisor of 6 resulting in a 25% frame
drop. I.e., unless I'm missing something using a divisor other than 4 is
not an option. This also matches your report from 2019: "Well, it's also
breaking another panel." [1]

I can currently see the following options:

a. Drive PLL-MIPI outside spec and panel within spec (current situation,
   but missing a small patch [2] that fixes the crtc_clock and nothing
   else) and live with the fact that some pinephones will run
   unreliably.

b. Drive PLL-MIPI and panel within spec and shove data into the panel at
   a too high rate (i.e., apply the rest of this series but not this
   specific patch). This seems to mostly work, but hasn't seen any long
   term testing. Short term testing showed that this approach results in
   a slight but noticable unsmooth scrolling behavior.

c. Drive PLL-MIPI within spec and panel outside spec (i.e., apply a
   future version of the whole series). This has been tested for over a
   month on three devices that I know of. There are no reports of panels
   not working with the suggested parameters.

All options require additional work on the GPU rate which is currently
being discussed in a parallel thread of this series. Unless somebody
comes up with a better idea, I will pause working on fixing PLL-MIPI and
focus on the GPU instead. While I doubt it, maybe fixing the GPU is
sufficient and continuing to drive PLL-MIPI outside spec proves to be
ok.

[1]: https://lore.kernel.org/all/20190828130341.s5z76wejulwdgxlc@flea/
[2]: https://lore.kernel.org/all/20230219114553.288057-2-frank@oltmanns.dev/

Best regards,
  Frank

>
> Best regards,
>   Frank
>
>>
>> Maxime
>>
>> [[End of PGP Signed Part]]
  

On Sun, Feb 11, 2024 at 04:42:43PM +0100, Frank Oltmanns wrote:
> 
> On 2024-02-08 at 20:05:08 +0100, Maxime Ripard <mripard@kernel.org> wrote:
> > [[PGP Signed Part:Undecided]]
> > Hi Frank,
> >
> > On Mon, Feb 05, 2024 at 04:22:28PM +0100, Frank Oltmanns wrote:
> >> This panel is used in the pinephone that runs on a Allwinner A64 SOC.
> >> The SOC requires pll-mipi to run at more than 500 MHz.
> >>
> >> This is the relevant clock tree:
> >>  pll-mipi
> >>     tcon0
> >>        tcon-data-clock
> >>
> >> tcon-data-clock has to run at 1/4 the DSI per-lane bit rate. The XBD599
> >> has 24 bpp and 4 lanes. Therefore, the resulting requested
> >> tcon-data-clock rate is:
> >>     crtc_clock * 1000 * (24 / 4) / 4
> >>
> >> tcon-data-clock runs at tcon0 / 4 (fixed divisor), so it requests a
> >> parent rate of
> >>     4 * (crtc_clock * 1000 * (24 / 4) / 4)
> >>
> >> Since tcon0 is a ccu_mux, the rate of tcon0 equals the rate of pll-mipi.
> >>
> >> pll-mipi's constraint to run at 500MHz or higher forces us to have a
> >> crtc_clock >= 83333 kHz if we want a 60 Hz vertical refresh rate.
> >>
> >> Change [hv]sync_(start|end) so that we reach a clock rate of 83502 kHz
> >> so that it is high enough to align with pll-pipi limits.
> >>
> >> Signed-off-by: Frank Oltmanns <frank@oltmanns.dev>
> >
> > That commit log is great, but it's kind of off-topic. It's a panel
> > driver, it can be used on any MIPI-DSI controller, the only relevant
> > information there should be the panel timings required in the datasheet.
> >
> > The PLL setup is something for the MIPI-DSI driver to adjust, not for
> > the panel to care for.
> >
> 
> I absolutely agree. It even was the reason for my submission of a
> sunxi-ng patch series last year that was accepted, to make pll-mipi more
> flexible. :)
> 
> The only remaining option I currently see for adjusting the sunxi-ng
> driver to further accomodate the panel, is trying to use a higher
> divisor than 4 for calculating tcon-data-clock from tcon0. I remember
> reading a discussion about this, but as far as I remember that proposal
> was rejected (by you, IIRC).
> 
> While I appreciate other suggestion as well, I'll look into options for
> using a different divisor than 4.

Like I said, I'm not against the patch at all, it looks great to me on
principle. I just think you should completely rephrase the commit log
using the datasheet as the only reliable source of the display timings.
Whether sun4i can work around the panel requirements is something
completely orthogonal to the discussion, and thus the commit log.

Maxime
  

Hi Maxime,

On 2024-02-22 at 11:29:51 +0100, Maxime Ripard <mripard@kernel.org> wrote:
> [[PGP Signed Part:Undecided]]
> On Sun, Feb 11, 2024 at 04:42:43PM +0100, Frank Oltmanns wrote:
>>
>> On 2024-02-08 at 20:05:08 +0100, Maxime Ripard <mripard@kernel.org> wrote:
>> > [[PGP Signed Part:Undecided]]
>> > Hi Frank,
>> >
>> > On Mon, Feb 05, 2024 at 04:22:28PM +0100, Frank Oltmanns wrote:
>> >> This panel is used in the pinephone that runs on a Allwinner A64 SOC.
>> >> The SOC requires pll-mipi to run at more than 500 MHz.
>> >>
>> >> This is the relevant clock tree:
>> >>  pll-mipi
>> >>     tcon0
>> >>        tcon-data-clock
>> >>
>> >> tcon-data-clock has to run at 1/4 the DSI per-lane bit rate. The XBD599
>> >> has 24 bpp and 4 lanes. Therefore, the resulting requested
>> >> tcon-data-clock rate is:
>> >>     crtc_clock * 1000 * (24 / 4) / 4
>> >>
>> >> tcon-data-clock runs at tcon0 / 4 (fixed divisor), so it requests a
>> >> parent rate of
>> >>     4 * (crtc_clock * 1000 * (24 / 4) / 4)
>> >>
>> >> Since tcon0 is a ccu_mux, the rate of tcon0 equals the rate of pll-mipi.
>> >>
>> >> pll-mipi's constraint to run at 500MHz or higher forces us to have a
>> >> crtc_clock >= 83333 kHz if we want a 60 Hz vertical refresh rate.
>> >>
>> >> Change [hv]sync_(start|end) so that we reach a clock rate of 83502 kHz
>> >> so that it is high enough to align with pll-pipi limits.
>> >>
>> >> Signed-off-by: Frank Oltmanns <frank@oltmanns.dev>
>> >
>> > That commit log is great, but it's kind of off-topic. It's a panel
>> > driver, it can be used on any MIPI-DSI controller, the only relevant
>> > information there should be the panel timings required in the datasheet.
>> >
>> > The PLL setup is something for the MIPI-DSI driver to adjust, not for
>> > the panel to care for.
>> >
>>
>> I absolutely agree. It even was the reason for my submission of a
>> sunxi-ng patch series last year that was accepted, to make pll-mipi more
>> flexible. :)
>>
>> The only remaining option I currently see for adjusting the sunxi-ng
>> driver to further accomodate the panel, is trying to use a higher
>> divisor than 4 for calculating tcon-data-clock from tcon0. I remember
>> reading a discussion about this, but as far as I remember that proposal
>> was rejected (by you, IIRC).
>>
>> While I appreciate other suggestion as well, I'll look into options for
>> using a different divisor than 4.
>
> Like I said, I'm not against the patch at all, it looks great to me on
> principle. I just think you should completely rephrase the commit log
> using the datasheet as the only reliable source of the display timings.
> Whether sun4i can work around the panel requirements is something
> completely orthogonal to the discussion, and thus the commit log.
>

I was trying to follow the guidelines [1] for describing the reason
behind my changes to the panel. My original commit message was a lot
shorter, which, understandably, resulted in follow up questions [2].
With the current commit log, I'm trying to address those questions.
According to the device tree, the panel is only used in the pinephone.
The only reason for the change is that the SoC used by the only user of
this panel can not provide the rate the panel requests with the current
values. I think this information is relevant.

Unfortunately, as described in [2], I cannot back these values with any
datasheets because I couldn't find any. I could only find hints that
they are not publicly available. Icenowy (added to CC) submitted the
original values.

Best regards,
  Frank

[1]: https://www.kernel.org/doc/html/v6.7/process/submitting-patches.html#describe-your-changes
[2]: https://lore.kernel.org/lkml/87wmsvo0fh.fsf@oltmanns.dev/

>
> Maxime
>
> [[End of PGP Signed Part]]
  

在 2024-02-25星期日的 17:46 +0100，Frank Oltmanns写道：
> Hi Maxime,
> 
> On 2024-02-22 at 11:29:51 +0100, Maxime Ripard <mripard@kernel.org>
> wrote:
> > [[PGP Signed Part:Undecided]]
> > On Sun, Feb 11, 2024 at 04:42:43PM +0100, Frank Oltmanns wrote:
> > > 
> > > On 2024-02-08 at 20:05:08 +0100, Maxime Ripard
> > > <mripard@kernel.org> wrote:
> > > > [[PGP Signed Part:Undecided]]
> > > > Hi Frank,
> > > > 
> > > > On Mon, Feb 05, 2024 at 04:22:28PM +0100, Frank Oltmanns wrote:
> > > > > This panel is used in the pinephone that runs on a Allwinner
> > > > > A64 SOC.
> > > > > The SOC requires pll-mipi to run at more than 500 MHz.
> > > > > 
> > > > > This is the relevant clock tree:
> > > > >  pll-mipi
> > > > >     tcon0
> > > > >        tcon-data-clock
> > > > > 
> > > > > tcon-data-clock has to run at 1/4 the DSI per-lane bit rate.
> > > > > The XBD599
> > > > > has 24 bpp and 4 lanes. Therefore, the resulting requested
> > > > > tcon-data-clock rate is:
> > > > >     crtc_clock * 1000 * (24 / 4) / 4
> > > > > 
> > > > > tcon-data-clock runs at tcon0 / 4 (fixed divisor), so it
> > > > > requests a
> > > > > parent rate of
> > > > >     4 * (crtc_clock * 1000 * (24 / 4) / 4)
> > > > > 
> > > > > Since tcon0 is a ccu_mux, the rate of tcon0 equals the rate
> > > > > of pll-mipi.
> > > > > 
> > > > > pll-mipi's constraint to run at 500MHz or higher forces us to
> > > > > have a
> > > > > crtc_clock >= 83333 kHz if we want a 60 Hz vertical refresh
> > > > > rate.
> > > > > 
> > > > > Change [hv]sync_(start|end) so that we reach a clock rate of
> > > > > 83502 kHz
> > > > > so that it is high enough to align with pll-pipi limits.
> > > > > 
> > > > > Signed-off-by: Frank Oltmanns <frank@oltmanns.dev>
> > > > 
> > > > That commit log is great, but it's kind of off-topic. It's a
> > > > panel
> > > > driver, it can be used on any MIPI-DSI controller, the only
> > > > relevant
> > > > information there should be the panel timings required in the
> > > > datasheet.
> > > > 
> > > > The PLL setup is something for the MIPI-DSI driver to adjust,
> > > > not for
> > > > the panel to care for.
> > > > 
> > > 
> > > I absolutely agree. It even was the reason for my submission of a
> > > sunxi-ng patch series last year that was accepted, to make pll-
> > > mipi more
> > > flexible. :)
> > > 
> > > The only remaining option I currently see for adjusting the
> > > sunxi-ng
> > > driver to further accomodate the panel, is trying to use a higher
> > > divisor than 4 for calculating tcon-data-clock from tcon0. I
> > > remember
> > > reading a discussion about this, but as far as I remember that
> > > proposal
> > > was rejected (by you, IIRC).
> > > 
> > > While I appreciate other suggestion as well, I'll look into
> > > options for
> > > using a different divisor than 4.
> > 
> > Like I said, I'm not against the patch at all, it looks great to me
> > on
> > principle. I just think you should completely rephrase the commit
> > log
> > using the datasheet as the only reliable source of the display
> > timings.
> > Whether sun4i can work around the panel requirements is something
> > completely orthogonal to the discussion, and thus the commit log.
> > 
> 
> I was trying to follow the guidelines [1] for describing the reason
> behind my changes to the panel. My original commit message was a lot
> shorter, which, understandably, resulted in follow up questions [2].
> With the current commit log, I'm trying to address those questions.
> According to the device tree, the panel is only used in the
> pinephone.
> The only reason for the change is that the SoC used by the only user
> of
> this panel can not provide the rate the panel requests with the
> current
> values. I think this information is relevant.
> 
> Unfortunately, as described in [2], I cannot back these values with
> any
> datasheets because I couldn't find any. I could only find hints that
> they are not publicly available. Icenowy (added to CC) submitted the
> original values.

Sorry but this kind of things are just magic from the vendor that I
could hardly explain...

> 
> Best regards,
>   Frank
> 
> [1]:
> https://www.kernel.org/doc/html/v6.7/process/submitting-patches.html#describe-your-changes
> [2]: https://lore.kernel.org/lkml/87wmsvo0fh.fsf@oltmanns.dev/
> 
> > 
> > Maxime
> > 
> > [[End of PGP Signed Part]]
>