Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
References: <20230418074008.69752-1-me@crly.cz> <87cz3uzpx1.fsf@oltmanns.dev>
From:   Frank Oltmanns <frank@oltmanns.dev>
To:     Maxime Ripard <mripard@kernel.org>,
        Jernej Skrabec <jernej.skrabec@gmail.com>
Cc:     Roman Beranek <me@crly.cz>, Chen-Yu Tsai <wens@csie.org>,
        David Airlie <airlied@gmail.com>,
        Daniel Vetter <daniel@ffwll.ch>,
        Samuel Holland <samuel@sholland.org>,
        Ondrej Jirman <megi@xff.cz>, dri-devel@lists.freedesktop.org,
        linux-arm-kernel@lists.infradead.org, linux-sunxi@lists.linux.dev,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 0/7] drm: sun4i: set proper TCON0 DCLK rate in DSI mode
Date:   Mon, 01 May 2023 15:40:49 +0200
In-reply-to: <87cz3uzpx1.fsf@oltmanns.dev>
Message-ID: <87mt2o9njh.fsf@oltmanns.dev>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

Maxime, Jernej, I was trying to understand why pll-video0 is not updated
and I tracked down the culprit to ccu_nkm.c.

On 2023-04-23 at 15:24:33 +0200, Frank Oltmanns <frank@oltmanns.dev> wrote:
> On 2023-04-18 at 09:40:01 +0200, Roman Beranek <me@crly.cz> wrote:
>> According to Allwinner's BSP code, in DSI mode, TCON0 clock needs to be
>> running at what's effectively the per-lane datarate of the DSI link.
>> Given that the TCON DCLK divider is fixed to 4 (SUN6I_DSI_TCON_DIV),
>> DCLK can't be set equal to the dotclock. Therefore labeling TCON DCLK
>> as sun4i_dotclock or tcon-pixel-clock shall be avoided.
>>
>> With bpp bits per pixel transmitted over n DSI lanes, the target DCLK
>> rate for a given pixel clock is obtained as follows:
>>
>> DCLK rate = 1/4 * bpp / n * pixel clock
>>
>> Effect of this change can be observed through the rate of Vblank IRQs
>> which should now match refresh rate implied by set display mode. It
>> was verified to do so on a A64 board with a 2-lane and a 4-lane panel.
[...]
> I've tried your patches on my pinephone. I also set the panel's clock to
> 72 MHz, so at 24 bpp and 4 lanes that should result in a data clock of
> 108 MHz. This should be possible when pll-video0 is at 297 MHz.
>
> Unfortunately, pll-video0 is not set and therefore the relevant part of
> the clk_summary looks like this:
>
>                           enable  prepare  protect              hardware
> clock                      count    count    count        rate    enable
> ------------------------------------------------------------------------
>  pll-video0                    1        1        1   294000000         Y
>     hdmi                       0        0        0   294000000         N
>     tcon1                      0        0        0   294000000         N
>     pll-mipi                   1        1        1   431200000         Y
>        tcon0                   2        2        1   431200000         Y
>           tcon-data-clock      1        1        1   107800000         Y
>     pll-video0-2x              0        0        0   588000000         Y
>
> Note, I've cut the columns accuracy, phase, and duty cycle, because they
> show the same values for all clocks (0, 0, 50000).
>
> My understanding was that with this patchset setting the parent clock
> should be possible. Do you have any idea why it doesn't work on the
> pinephone? Or maybe it does work on yours and I'm making some kind of
> mistake?

To better understand what's going on I've extended the clk_rate_request
class to also output the requested rate. The relevant output is this
(leading line numbers by me for referencing the lines below):
line  1:     kworker/u8:2-49      [002] .....     1.850141: clk_rate_request_start: tcon-data-clock rate 108000000 min 0 max 18446744073709551615, parent tcon0 (588000000)
line  2:     kworker/u8:2-49      [002] .....     1.850149: clk_rate_request_start: tcon0 rate 432000000 min 0 max 18446744073709551615, parent pll-mipi (588000000)
line  3:     kworker/u8:2-49      [002] .....     1.850154: clk_rate_request_start: pll-mipi rate 432000000 min 0 max 18446744073709551615, parent pll-video0 (294000000)
line  4:     kworker/u8:2-49      [002] .....     1.850168: clk_rate_request_done: pll-mipi rate 431200000 min 0 max 18446744073709551615, parent pll-video0 (294000000)
line  5:     kworker/u8:2-49      [002] .....     1.850169: clk_rate_request_done: tcon0 rate 431200000 min 0 max 18446744073709551615, parent pll-mipi (431200000)
line  6:     kworker/u8:2-49      [002] .....     1.850171: clk_rate_request_done: tcon-data-clock rate 107800000 min 0 max 18446744073709551615, parent tcon0 (431200000)
line  7:     kworker/u8:2-49      [002] .....     1.850172: clk_rate_request_start: tcon-data-clock rate 108000000 min 0 max 18446744073709551615, parent tcon0 (588000000)
line  8:     kworker/u8:2-49      [002] .....     1.850174: clk_rate_request_start: tcon0 rate 432000000 min 0 max 18446744073709551615, parent pll-mipi (588000000)
line  9:     kworker/u8:2-49      [002] .....     1.850179: clk_rate_request_start: pll-mipi rate 432000000 min 0 max 18446744073709551615, parent pll-video0 (294000000)
line 10:     kworker/u8:2-49      [002] .....     1.850190: clk_rate_request_done: pll-mipi rate 431200000 min 0 max 18446744073709551615, parent pll-video0 (294000000)
line 11:     kworker/u8:2-49      [002] .....     1.850191: clk_rate_request_done: tcon0 rate 431200000 min 0 max 18446744073709551615, parent pll-mipi (431200000)
line 12:     kworker/u8:2-49      [002] .....     1.850192: clk_rate_request_done: tcon-data-clock rate 107800000 min 0 max 18446744073709551615, parent tcon0 (431200000)
line 13:     kworker/u8:2-49      [002] .....     1.850193: clk_rate_request_start: tcon0 rate 431200000 min 0 max 18446744073709551615, parent pll-mipi (588000000)
line 14:     kworker/u8:2-49      [002] .....     1.850195: clk_rate_request_start: pll-mipi rate 431200000 min 0 max 18446744073709551615, parent pll-video0 (294000000)
line 15:     kworker/u8:2-49      [002] .....     1.850205: clk_rate_request_done: pll-mipi rate 431200000 min 0 max 18446744073709551615, parent pll-video0 (294000000)
line 16:     kworker/u8:2-49      [002] .....     1.850206: clk_rate_request_done: tcon0 rate 431200000 min 0 max 18446744073709551615, parent pll-mipi (431200000)
line 17:     kworker/u8:2-49      [002] .....     1.850208: clk_rate_request_start: pll-mipi rate 431200000 min 0 max 18446744073709551615, parent pll-video0 (294000000)
line 18:     kworker/u8:2-49      [002] .....     1.850219: clk_rate_request_done: pll-mipi rate 431200000 min 0 max 18446744073709551615, parent pll-video0 (294000000)
line 19:     kworker/u8:2-49      [002] .....     1.850229: clk_set_rate: pll-mipi 431200000
line 20:     kworker/u8:2-49      [003] .....     1.850508: clk_set_rate_complete: pll-mipi 431200000
line 21:     kworker/u8:2-49      [003] .....     1.850513: clk_set_rate: tcon0 431200000
line 22:     kworker/u8:2-49      [003] .....     1.850515: clk_set_rate_complete: tcon0 431200000
line 23:     kworker/u8:2-49      [003] .....     1.850516: clk_set_rate: tcon-data-clock 107800000
line 24:     kworker/u8:2-49      [003] .....     1.850524: clk_set_rate_complete: tcon-data-clock 107800000
line 25:     kworker/u8:2-49      [003] .....     1.853320: clk_prepare: tcon-data-clock
line 26:     kworker/u8:2-49      [003] .....     1.853324: clk_prepare_complete: tcon-data-clock
line 27:     kworker/u8:2-49      [003] d..1.     1.853328: clk_enable: tcon-data-clock
line 28:     kworker/u8:2-49      [003] d..1.     1.853333: clk_enable_complete: tcon-data-clock

In line 1 we can see that a rate of 108 MHz is requested for
tcon-data-clock. In lines 2 and 3 this is forwarded to tcon0 and
pll-mipi (432 MHz). What surprised me, is that there is no request to
set the rate of pll-video0. Instead pll-mipi (and subsequently tcon0)
are set to 431.2 MHz (lines 4,5) and consequently tcon-data-clock is at
107.8 MHz (line 6) as I also reported in my previous mail (see quote
above).

When figuring out the call stack, I traced the whole thing down to
ccu_nkm_determine_rate(). The simplified call stack looks like this:

clk_set_rate(tcon-data-clock, 108MHz)
   clk_core_set_rate_nolock(tcon-data-clock, 108MHz)
      clk_core_req_round_rate_nolock(tcon-data-clock, 108MHz)
         clk_core_round_rate_nolock(tcon-data-clock, 108MHz)
            sun4i_dclk_round_rate(tcon-data-clock)
               clk_hw_round_rate(tcon0, 432MHz)
                  clk_core_round_rate_nolock(tcon0, 432MHz)
                     clk_mux_determine_rate_flags(tcon0, 432MHz)
                        clk_core_round_rate_nolock(pll-mipi, 432MHz)
                           ccu_nkm_determine_rate(pll-mipi, 432MHz)

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.

So, I can see two options:
 a. Set pll-video0 to 297 MHz on boot
 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.

Until then, is option a acceptable in mainline?

Thanks,
  Frank

>
> On a brighter note, when I initialize pll-video0 to 297 MHz in
> sunxi-ng/ccu-sun50i-a64.c:sun50i_a64_ccu_probe() I get an even 108 Mhz
> for the data clock. The patch is:
>
> 	writel(0x515, reg + SUN50I_A64_PLL_MIPI_REG);
>
> +	/*
> +	 * Initialize PLL VIDEO0 to default values (297 MHz)
> +	 * to clean up any changes made by bootloader
> +	 */
> +	writel(0x03006207, reg + 0x10);
> +
> 	ret = devm_sunxi_ccu_probe(&pdev->dev, reg, &sun50i_a64_ccu_desc);
> 	if (ret)
> 		return ret;
>
> Best,
>   Frank