[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <2682674.mvXUDI8C0e@jernej-laptop>
Date: Wed, 10 May 2023 20:41:33 +0200
From: Jernej Škrabec <jernej.skrabec@...il.com>
To: Roman Beranek <me@...y.cz>, Frank Oltmanns <frank@...manns.dev>
Cc: Maxime Ripard <mripard@...nel.org>, Chen-Yu Tsai <wens@...e.org>,
David Airlie <airlied@...il.com>,
Daniel Vetter <daniel@...ll.ch>,
Samuel Holland <samuel@...lland.org>,
Ondrej Jirman <megi@....cz>, dri-devel@...ts.freedesktop.org,
linux-arm-kernel@...ts.infradead.org, linux-sunxi@...ts.linux.dev,
linux-kernel@...r.kernel.org, Icenowy Zheng <uwu@...nowy.me>
Subject: Re: [PATCH v2 0/7] drm: sun4i: set proper TCON0 DCLK rate in DSI mode
Dne ponedeljek, 08. maj 2023 ob 16:08:32 CEST je Frank Oltmanns napisal(a):
> Hello again,
>
> On 2023-05-08 at 08:54:28 +0200, Frank Oltmanns <frank@...manns.dev> wrote:
> > Hello Roman,
> >
> > On 2023-05-03 at 16:22:32 +0200, "Roman Beranek" <me@...y.cz> wrote:
> >> Hello everyone,
> >>
> >> I apologize for my absence from the discussion during past week, I got
> >> hit with tonsillitis.
> >
> > I hope you feel better!
> >
> >> On Mon May 1, 2023 at 3:40 PM CEST, Frank Oltmanns wrote:
> >>> Looking at ccu_nkm_determine_rate(), we've found our culprit because it
> >>> does not try parent clock rates other than the current one. The same
> >>> applies to all other ccu_nkm_* functions.
> >>
> >> Yes, that's why I dropped CLK_SET_RATE_PARENT from pll-mipi in v3.
> >>
> >>> b. Add functionality to ccu_nkm_* to also update the parent clock rate.
> >>>
> >>> I'm actually interested in tackling b, but I can't make any promises as
> >>> to if and when I'll be able to solve it. I'm not certain about any side
> >>> effects this might have.
> >>
> >> It sounds like an interesting exercise. But what if HDMI is then added
> >> to the mix?
> >
> > Thanks for interest in this discussion! I really appreciate it!
> >
> > First of all, let me admit that I'm no expert on this. But nobody else
> > has replied so far, and I want to keep this conversation going, so let
> > me share my view.
> >
> > My understanding is that pll-mipi being able to set pll-video0's rate
> > should not have an impact on HDMI, neither positive nor negative. If I'm
> > not mistaken those two things are orthogonal.
> >
> > The relevant part of the clk_summary with your v4 [1] patch on top of
> >
> > drm-next looks like this:
> > enable protect hardware
> >
> > clock count count rate enable
> >
> > ----------------------------------------------------------------------
> >
> > pll-video0 1 1 294000000 Y
> >
> > hdmi 0 0 294000000 N
> > tcon1 0 0 294000000 N
> > pll-mipi 1 1 431200000 Y
> >
> > tcon0 2 1 431200000 Y
> >
> > tcon-data-clock 1 1 107800000 Y
> >
> > pll-video0-2x 0 0 588000000 Y
> >
> > Note, that pll-video0 is protected.
> >
> > I don't own any boards that support HDMI in mainline. For the pinephone
> > this support is added e.g. in megi's kernel, where connecting an HDMI
> > output results in pll-video0's rate being set to 297MHz, even though it
> > is 294MHz after boot.
> >
> > So, for reference, this is the same part of the clk_summary with megi's
> >
> > 6.3.0 kernel, USB-C dock unplugged:
> > enable protect hardware
> >
> > clock count count rate enable
> >
> > ----------------------------------------------------------------------
> >
> > pll-video0 3 0 294000000 Y
> >
> > hdmi-phy-clk 1 0 73500000 Y
> > hdmi 1 0 294000000 Y
> > tcon1 0 0 294000000 N
> > pll-mipi 1 0 431200000 Y
> >
> > tcon0 2 0 431200000 Y
> >
> > tcon-pixel-clock 1 0 107800000 Y
> >
> > pll-video0-2x 0 0 588000000 Y
> >
> > pll-video0 is not protected. When plugging in the USB-C dock with an HDMI
>
> > monitor connected, the situation looks like this:
> Just for reference, the protection count is disabled by this commit [1]
> in megi's kernel.
>
> In the commit message Icenowy Zheng refers to "the ability to keep TCON0
> clock stable when HDMI changes its parent's clock." She implemented this
> in these two previous commits [2] [3]. None of this is in mainline.
Those commits are good follow up series to this, if anyone wants to improve
things further.
Best regards,
Jernej
>
> Best regards,
> Frank
>
> [1]:
> https://github.com/megous/linux/commit/039f7ee3f44adfbe4c6b7c2f1798b9a70c9f
> b9ee [2]:
> https://github.com/megous/linux/commit/a927843932f16e5a7f5ff57fbfd2d5f11c71
> 2b67 [3]:
> https://github.com/megous/linux/commit/0e305371eaa49128856acce9830e6af07944
> 2ad6
> > enable protect hardware
> >
> > clock count count rate enable
> >
> > ----------------------------------------------------------------------
> >
> > pll-video0 4 1 297000000 Y
> >
> > hdmi-phy-clk 1 0 148500000 Y
> > hdmi 1 0 148500000 Y
> > tcon1 1 1 148500000 Y
> > pll-mipi 1 0 424285714 Y
> >
> > tcon0 2 0 424285714 Y
> >
> > tcon-pixel-clock 1 0 106071428 Y
> >
> > pll-video0-2x 0 0 594000000 Y
> >
> > As you can see, pll-video0 is updated to 297 MHz. My understanding is
> > (again: not an expert here) this is only possible due to the missing
> > protection.
> >
> > What I'm trying to say is that I don't see a connection between HDMI and
> > having the functionality in ccu_nkm_* to update the parent clock rate.
> >
> > But I do think it would be preferable to have pll-video0 at 297 MHz
> > after boot on the pinephone. We could achieve this with my two previous
> >
> > proposals:
> > a) Set pll-video0 to 297 MHz on boot
> > b) Add functionality to ccu_nkm_* to also update the parent clock rate.
> >
> > If solution b is viable, the goal for the pinephone (and any other
> > boards supporting HDMI) would then be to select a pixel-data-clock so
> > that the rate for pll-video0 is set to 297 MHz (by selecting an
> > appropriate clock speed for the internal panel).
> >
> > Maybe I'm misunderstanding something. If so, I'd appreciate any
> > corrections.
> >
> > Thanks,
> >
> > Frank
> >
> > [1]: https://lore.kernel.org/all/20230505052110.67514-1-me@crly.cz/
> >
> >> Best regards
> >> Roman
Powered by blists - more mailing lists