Re: [PATCH v2 0/7] drm: sun4i: set proper TCON0 DCLK rate in DSI mode

["Jernej Škrabec" <jernej.skrabec@gmail.com>]

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@oltmanns.dev> wrote:
> > Hello Roman,
> > 
> > On 2023-05-03 at 16:22:32 +0200, "Roman Beranek" <me@crly.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