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

[Icenowy Zheng <uwu@icenowy.me>]

在 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]]
>