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 wrote: > > Hello Roman, > > > > On 2023-05-03 at 16:22:32 +0200, "Roman Beranek" 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 countcountrateenable > > > > -- > > > > pll-video011 29400 Y > > > >hdmi 00 29400 N > >tcon1 00 29400 N > >pll-mipi 11 43120 Y > > > > tcon0 21 43120 Y > > > > tcon-data-clock 11 10780 Y > > > >pll-video0-2x 00 58800 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 countcountrateenable > > > > -- > > > > pll-video030 29400 Y > > > >hdmi-phy-clk 107350 Y > >hdmi 10 29400 Y > >tcon1 00 29400 N > >pll-mipi 10 43120 Y > > > > tcon0 20 43120 Y > > > > tcon-pixel-clock 10 10780 Y > > > >pll-video0-2x 00 58800 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 countcountrateenable > > > > -- > > > > pll-video04
Re: [PATCH v2 0/7] drm: sun4i: set proper TCON0 DCLK rate in DSI mode
Hello again, On 2023-05-08 at 08:54:28 +0200, Frank Oltmanns wrote: > Hello Roman, > > On 2023-05-03 at 16:22:32 +0200, "Roman Beranek" 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 countcountrateenable > -- > pll-video011 29400 Y >hdmi 00 29400 N >tcon1 00 29400 N >pll-mipi 11 43120 Y > tcon0 21 43120 Y > tcon-data-clock 11 10780 Y >pll-video0-2x 00 58800 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 countcountrateenable > -- > pll-video030 29400 Y >hdmi-phy-clk 107350 Y >hdmi 10 29400 Y >tcon1 00 29400 N >pll-mipi 10 43120 Y > tcon0 20 43120 Y > tcon-pixel-clock 10 10780 Y >pll-video0-2x 00 58800 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. Best regards, Frank [1]: https://github.com/megous/linux/commit/039f7ee3f44adfbe4c6b7c2f1798b9a70c9fb9ee [2]: https://github.com/megous/linux/commit/a927843932f16e5a7f5ff57fbfd2d5f11c712b67 [3]: https://github.com/megous/linux/commit/0e305371eaa49128856acce9830e6af079442ad6 > > enable protect hardware >clock countcountrateenable > -- > pll-video041 29700 Y >hdmi-phy-clk 10 14850 Y >hdmi 10 14850 Y >tcon1 11 14850 Y >pll-mipi 10 424285714 Y > tcon0 20 424285714 Y > tcon-pixel-clock 10 106071428 Y >pll-video0-2x 00 59400 Y > > As you can see,
Re: [PATCH v2 0/7] drm: sun4i: set proper TCON0 DCLK rate in DSI mode
Hello Roman, On 2023-05-03 at 16:22:32 +0200, "Roman Beranek" 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 countcountrateenable -- pll-video011 29400 Y hdmi 00 29400 N tcon1 00 29400 N pll-mipi 11 43120 Y tcon0 21 43120 Y tcon-data-clock 11 10780 Y pll-video0-2x 00 58800 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 countcountrateenable -- pll-video030 29400 Y hdmi-phy-clk 107350 Y hdmi 10 29400 Y tcon1 00 29400 N pll-mipi 10 43120 Y tcon0 20 43120 Y tcon-pixel-clock 10 10780 Y pll-video0-2x 00 58800 Y pll-video0 is not protected. When plugging in the USB-C dock with an HDMI monitor connected, the situation looks like this: enable protect hardware clock countcountrateenable -- pll-video041 29700 Y hdmi-phy-clk 10 14850 Y hdmi 10 14850 Y tcon1 11 14850 Y pll-mipi 10 424285714 Y tcon0 20 424285714 Y tcon-pixel-clock 10 106071428 Y pll-video0-2x 00 59400 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).
Re: [PATCH v2 0/7] drm: sun4i: set proper TCON0 DCLK rate in DSI mode
Hello everyone, I apologize for my absence from the discussion during past week, I got hit with tonsillitis. 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? Best regards Roman
Re: [PATCH v2 0/7] drm: sun4i: set proper TCON0 DCLK rate in DSI mode
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 wrote: > On 2023-04-18 at 09:40:01 +0200, Roman Beranek 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 countcountcountrateenable > > pll-video0111 29400 Y > hdmi 000 29400 N > tcon1 000 29400 N > pll-mipi 111 43120 Y >tcon0 221 43120 Y > tcon-data-clock 111 10780 Y > pll-video0-2x 000 58800 Y > > Note, I've cut the columns accuracy, phase, and duty cycle, because they > show the same values for all clocks (0, 0, 5). > > 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 10800 min 0 max 18446744073709551615, parent tcon0 (58800) line 2: kworker/u8:2-49 [002] . 1.850149: clk_rate_request_start: tcon0 rate 43200 min 0 max 18446744073709551615, parent pll-mipi (58800) line 3: kworker/u8:2-49 [002] . 1.850154: clk_rate_request_start: pll-mipi rate 43200 min 0 max 18446744073709551615, parent pll-video0 (29400) line 4: kworker/u8:2-49 [002] . 1.850168: clk_rate_request_done: pll-mipi rate 43120 min 0 max 18446744073709551615, parent pll-video0 (29400) line 5: kworker/u8:2-49 [002] . 1.850169: clk_rate_request_done: tcon0 rate 43120 min 0 max 18446744073709551615, parent pll-mipi (43120) line 6: kworker/u8:2-49 [002] . 1.850171: clk_rate_request_done: tcon-data-clock rate 10780 min 0 max 18446744073709551615, parent tcon0 (43120) line 7: kworker/u8:2-49 [002] . 1.850172: clk_rate_request_start: tcon-data-clock rate 10800 min 0 max 18446744073709551615, parent tcon0 (58800) line 8: kworker/u8:2-49 [002] . 1.850174: clk_rate_request_start: tcon0 rate 43200 min 0 max 18446744073709551615, parent pll-mipi (58800) line 9: kworker/u8:2-49 [002] . 1.850179: clk_rate_request_start: pll-mipi rate 43200 min 0 max 18446744073709551615, parent pll-video0 (29400) line 10: kworker/u8:2-49 [002] . 1.850190: clk_rate_request_done: pll-mipi rate 43120 min 0 max 18446744073709551615, parent pll-video0 (29400) line 11: kworker/u8:2-49 [002] . 1.850191: clk_rate_request_done: tcon0 rate 43120 min 0 max 18446744073709551615, parent pll-mipi (43120) line 12: kworker/u8:2-49 [002] . 1.850192: clk_rate_request_done: tcon-data-clock rate 10780 min 0 max 18446744073709551615, parent tcon0 (43120) line 13: kworker/u8:2-49 [002] . 1.850193: clk_rate_request_start: tcon0 rate 43120 min 0 max 18446744073709551615, parent pll-mipi (58800) line 14: kworker/u8:2-49 [002] . 1.850195: clk_rate_request_start: pll-mipi rate 43120 min 0 max 18446744073709551615, parent pll-video0
Re: [PATCH v2 0/7] drm: sun4i: set proper TCON0 DCLK rate in DSI mode
Hi Roman,
On 2023-04-18 at 09:40:01 +0200, Roman Beranek 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.
>
> v2:
> 1. prevent reparent of tcon0 to pll-video0-2x
> 2. include pll-video0 in setting TCON0 DCLK rate
> 3. tested the whole thing also on a PinePhone
>
> Roman Beranek (7):
> clk: sunxi-ng: a64: propagate rate change from pll-mipi
> clk: sunxi-ng: a64: export PLL_MIPI
> clk: sunxi-ng: a64: prevent CLK_TCON0 being reparented
> arm64: dts: allwinner: a64: assign PLL_MIPI to CLK_TCON0
> ARM: dts: sunxi: rename tcon's clock output
> drm: sun4i: rename sun4i_dotclock to sun4i_tcon_dclk
> drm: sun4i: calculate proper DCLK rate for DSI
>
> arch/arm/boot/dts/sun5i.dtsi | 2 +-
> arch/arm/boot/dts/sun8i-a23-a33.dtsi | 2 +-
> arch/arm/boot/dts/sun8i-a83t.dtsi | 2 +-
> arch/arm/boot/dts/sun8i-v3s.dtsi | 2 +-
> arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 4 +-
> drivers/clk/sunxi-ng/ccu-sun50i-a64.c | 6 ++-
> drivers/clk/sunxi-ng/ccu-sun50i-a64.h | 4 +-
> drivers/gpu/drm/sun4i/Makefile| 2 +-
> drivers/gpu/drm/sun4i/sun4i_tcon.c| 46 +++
> .../{sun4i_dotclock.c => sun4i_tcon_dclk.c} | 2 +-
> .../{sun4i_dotclock.h => sun4i_tcon_dclk.h} | 0
> include/dt-bindings/clock/sun50i-a64-ccu.h| 1 +
> 12 files changed, 43 insertions(+), 30 deletions(-)
> rename drivers/gpu/drm/sun4i/{sun4i_dotclock.c => sun4i_tcon_dclk.c} (99%)
> rename drivers/gpu/drm/sun4i/{sun4i_dotclock.h => sun4i_tcon_dclk.h} (100%)
>
>
> base-commit: 4aa35a0130d6b8afbefc9ef530a521fb0fb9b8e1
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 countcountcountrateenable
pll-video0111 29400 Y
hdmi 000 29400 N
tcon1 000 29400 N
pll-mipi 111 43120 Y
tcon0 221 43120 Y
tcon-data-clock 111 10780 Y
pll-video0-2x 000 58800 Y
Note, I've cut the columns accuracy, phase, and duty cycle, because they
show the same values for all clocks (0, 0, 5).
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?
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(>dev, reg, _a64_ccu_desc);
if (ret)
return ret;
Best,
Frank
[PATCH v2 0/7] drm: sun4i: set proper TCON0 DCLK rate in DSI mode
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.
v2:
1. prevent reparent of tcon0 to pll-video0-2x
2. include pll-video0 in setting TCON0 DCLK rate
3. tested the whole thing also on a PinePhone
Roman Beranek (7):
clk: sunxi-ng: a64: propagate rate change from pll-mipi
clk: sunxi-ng: a64: export PLL_MIPI
clk: sunxi-ng: a64: prevent CLK_TCON0 being reparented
arm64: dts: allwinner: a64: assign PLL_MIPI to CLK_TCON0
ARM: dts: sunxi: rename tcon's clock output
drm: sun4i: rename sun4i_dotclock to sun4i_tcon_dclk
drm: sun4i: calculate proper DCLK rate for DSI
arch/arm/boot/dts/sun5i.dtsi | 2 +-
arch/arm/boot/dts/sun8i-a23-a33.dtsi | 2 +-
arch/arm/boot/dts/sun8i-a83t.dtsi | 2 +-
arch/arm/boot/dts/sun8i-v3s.dtsi | 2 +-
arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 4 +-
drivers/clk/sunxi-ng/ccu-sun50i-a64.c | 6 ++-
drivers/clk/sunxi-ng/ccu-sun50i-a64.h | 4 +-
drivers/gpu/drm/sun4i/Makefile| 2 +-
drivers/gpu/drm/sun4i/sun4i_tcon.c| 46 +++
.../{sun4i_dotclock.c => sun4i_tcon_dclk.c} | 2 +-
.../{sun4i_dotclock.h => sun4i_tcon_dclk.h} | 0
include/dt-bindings/clock/sun50i-a64-ccu.h| 1 +
12 files changed, 43 insertions(+), 30 deletions(-)
rename drivers/gpu/drm/sun4i/{sun4i_dotclock.c => sun4i_tcon_dclk.c} (99%)
rename drivers/gpu/drm/sun4i/{sun4i_dotclock.h => sun4i_tcon_dclk.h} (100%)
base-commit: 4aa35a0130d6b8afbefc9ef530a521fb0fb9b8e1
--
2.34.1
