Re: [PATCH] drm/sun4i: tcon: Support keeping dclk rate upon ancestor clock changes

On 2024-03-13 at 19:11:37 +0100, Jernej Škrabec [off-list ref] wrote:
Hi Jernej,

Thank you for your having a thorough look at this!

Hi Frank!

Thanks on tackling this issue.

Dne nedelja, 10. marec 2024 ob 14:32:29 CET je Frank Oltmanns napisal(a):

quoted

Allow the dclk to reset its rate when a rate change is initiated from an
ancestor clock. This makes it possible to no longer to get an exclusive
lock. As a consequence, it is now possible to set new rates if
necessary, e.g. when an external display is connected.

The first user of this functionality is the A64 because PLL-VIDEO0 is an
ancestor for both HDMI and TCON0. This allows to select an optimal rate
for TCON0 as long as there is no external HDMI connection. Once a change
in PLL-VIDEO0 is performed when an HDMI connection is established, TCON0
can react gracefully and select an optimal rate based on this the new
constraint.

Signed-off-by: Frank Oltmanns <redacted>
---
I would like to make the Allwinner A64's data-clock keep its rate
when its ancestor's (pll-video0) rate changes. Keeping data-clock's rate
is required, to let the A64 drive both an LCD and HDMI display at the
same time, because both have pll-video0 as an ancestor.

TCONs that use this flag store the ideal rate for their data-clock and
subscribe to be notified when data-clock changes. When rate setting has
finished (indicated by a POST_RATE_CHANGE event) the call back function
schedules delayed work to set the data-clock's rate to the initial value
after 100 ms. Using delayed work maks sure that the clock setting is
finished.

I've implemented this functionality as a quirk, so that it is possible
to use it only for the A64.

This patch supersedes [1].

This work is inspired by an out-of-tree patchset [2] [3] [4].
Unfortunately, the patchset uses clk_set_rate() directly in a notifier
callback, which the following comment on clk_notifier_register()
forbids: "The callbacks associated with the notifier must not re-enter
into the clk framework by calling any top-level clk APIs." [5]
Furthermore, that out-of-tree patchset no longer works since 6.6,
because setting pll-mipi is now also resetting pll-video0 and therefore
causes a race condition.

Thank you for considering this contribution,
  Frank

[1] https://lore.kernel.org/lkml/20230825-pll-mipi_keep_rate-v1-0-35bc43570730@oltmanns.dev/ (local)
[2] https://codeberg.org/megi/linux/commit/a37cda2fff41a67a2bacf82b1594e10335d0bd8a
[3] https://codeberg.org/megi/linux/commit/24dc09128d2c8efc6ddf19249420e9798e967a46
[4] https://codeberg.org/megi/linux/commit/728a93d46f99f0eb231ed6fa8971a45f97c7182c
[5] https://elixir.bootlin.com/linux/v6.7.9/source/drivers/clk/clk.c#L4669
---
 drivers/gpu/drm/sun4i/sun4i_tcon.c | 70 ++++++++++++++++++++++++++++++++++----
 drivers/gpu/drm/sun4i/sun4i_tcon.h | 12 +++++++
 2 files changed, 76 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
index a1a2c845ade0..b880bd44049a 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c

@@ -108,9 +108,11 @@ static void sun4i_tcon_channel_set_status(struct sun4i_tcon *tcon, int channel,

 	if (enabled) {
 		clk_prepare_enable(clk);
-		clk_rate_exclusive_get(clk);
+		if (!tcon->quirks->restores_rate)
+			clk_rate_exclusive_get(clk);
 	} else {
-		clk_rate_exclusive_put(clk);
+		if (!tcon->quirks->restores_rate)
+			clk_rate_exclusive_put(clk);
 		clk_disable_unprepare(clk);
 	}
 }

@@ -343,6 +345,53 @@ static void sun4i_tcon0_mode_set_dithering(struct sun4i_tcon *tcon,
 	regmap_write(tcon->regs, SUN4I_TCON_FRM_CTL_REG, val);
 }

+static void sun4i_rate_reset_notifier_delayed_update(struct work_struct *work)
+{
+	struct sun4i_rate_reset_nb *rate_reset = container_of(work, struct sun4i_rate_reset_nb,
+							    reset_rate_work.work);
+
+	clk_set_rate(rate_reset->target_clk, rate_reset->saved_rate);
+}
+
+static int sun4i_rate_reset_notifier_cb(struct notifier_block *nb,
+				      unsigned long event, void *data)
+{
+	struct sun4i_rate_reset_nb *rate_reset = to_sun4i_rate_reset_nb(nb);
+
+	if (event == POST_RATE_CHANGE)
+		schedule_delayed_work(&rate_reset->reset_rate_work, msecs_to_jiffies(100));

Do we need that delay though? Since clock is set exclusive on TV TCONs, then
it shouldn't be changed. Alternative, simpler variation would be something
like this:
https://elixir.bootlin.com/linux/v6.8/source/drivers/tty/serial/8250/8250_dw.c#L333

Interesting! My reason for scheduling the work was to make sure we don't
call any clk API functions from the callback as described in my cover
letter above. But using the queue is a much nicer approach for achieving
the same thing! Thanks!

quoted

+
+	return NOTIFY_DONE;
+}
+
+static void sun4i_rate_reset_notifier_register(struct sun4i_rate_reset_nb *rate_reset_nb)
+{
+	if (rate_reset_nb->is_registered)
+		return;
+
+	rate_reset_nb->clk_nb.notifier_call = sun4i_rate_reset_notifier_cb;
+
+	INIT_DELAYED_WORK(&rate_reset_nb->reset_rate_work,
+			  sun4i_rate_reset_notifier_delayed_update);
+
+	if (!clk_notifier_register(rate_reset_nb->target_clk,
+				   &rate_reset_nb->clk_nb))
+		rate_reset_nb->is_registered = true;
+}
+
+static struct sun4i_rate_reset_nb tcon_rate_reset_tcon0_nb;

Is there any specific reason for global variable? Note that R40 and T507 have
2 LCD and 2 TV TCONs. If it's ever used there, it won't fly. Please move to
TCON struct. You can drop a few fields for doing so.

The only reason is that global variables were used for the SoC specific
callbacks that served as my inspiration. So, I thought it was a common
pattern. Actually, I wanted to make it part of the tcon struct but
refrained from doing so to not break the pattern. Now that I have your
blessing, I'll refactor.

quoted

+
+static void sun4i_tcon0_set_dclk_rate(struct sun4i_tcon *tcon, unsigned long rate)
+{
+	clk_set_rate(tcon->dclk, rate);
+
+	if (tcon->quirks->restores_rate) {
+		tcon_rate_reset_tcon0_nb.target_clk = tcon->dclk;
+		tcon_rate_reset_tcon0_nb.saved_rate = rate;
+		sun4i_rate_reset_notifier_register(&tcon_rate_reset_tcon0_nb);

Can't be registration done at TCON init time?

Probably yes! I'll make sure to do so in V2.

Thanks again for the great feedback,
  Frank

Best regards,
Jernej

quoted

+	}
+}
+
 static void sun4i_tcon0_mode_set_cpu(struct sun4i_tcon *tcon,
 				     const struct drm_encoder *encoder,
 				     const struct drm_display_mode *mode)

@@ -360,8 +409,8 @@ static void sun4i_tcon0_mode_set_cpu(struct sun4i_tcon *tcon,
 	 */
 	tcon->dclk_min_div = SUN6I_DSI_TCON_DIV;
 	tcon->dclk_max_div = SUN6I_DSI_TCON_DIV;
-	clk_set_rate(tcon->dclk, mode->crtc_clock * 1000 * (bpp / lanes)
-						  / SUN6I_DSI_TCON_DIV);
+	sun4i_tcon0_set_dclk_rate(tcon, mode->crtc_clock * 1000 * (bpp / lanes)
+				  / SUN6I_DSI_TCON_DIV);

 	/* Set the resolution */
 	regmap_write(tcon->regs, SUN4I_TCON0_BASIC0_REG,

@@ -434,7 +483,7 @@ static void sun4i_tcon0_mode_set_lvds(struct sun4i_tcon *tcon,

 	tcon->dclk_min_div = 7;
 	tcon->dclk_max_div = 7;
-	clk_set_rate(tcon->dclk, mode->crtc_clock * 1000);
+	sun4i_tcon0_set_dclk_rate(tcon, mode->crtc_clock * 1000);

 	/* Set the resolution */
 	regmap_write(tcon->regs, SUN4I_TCON0_BASIC0_REG,

@@ -516,7 +565,7 @@ static void sun4i_tcon0_mode_set_rgb(struct sun4i_tcon *tcon,

 	tcon->dclk_min_div = tcon->quirks->dclk_min_div;
 	tcon->dclk_max_div = 127;
-	clk_set_rate(tcon->dclk, mode->crtc_clock * 1000);
+	sun4i_tcon0_set_dclk_rate(tcon, mode->crtc_clock * 1000);

 	/* Set the resolution */
 	regmap_write(tcon->regs, SUN4I_TCON0_BASIC0_REG,

@@ -1503,6 +1552,14 @@ static const struct sun4i_tcon_quirks sun8i_a33_quirks = {
 	.supports_lvds		= true,
 };

+static const struct sun4i_tcon_quirks sun50i_a64_lcd_quirks = {
+	.supports_lvds		= true,
+	.has_channel_0		= true,
+	.restores_rate		= true,
+	.dclk_min_div		= 1,
+	.setup_lvds_phy		= sun6i_tcon_setup_lvds_phy,
+};
+
 static const struct sun4i_tcon_quirks sun8i_a83t_lcd_quirks = {
 	.supports_lvds		= true,
 	.has_channel_0		= true,

@@ -1561,6 +1618,7 @@ const struct of_device_id sun4i_tcon_of_table[] = {
 	{ .compatible = "allwinner,sun9i-a80-tcon-tv", .data = &sun9i_a80_tcon_tv_quirks },
 	{ .compatible = "allwinner,sun20i-d1-tcon-lcd", .data = &sun20i_d1_lcd_quirks },
 	{ .compatible = "allwinner,sun20i-d1-tcon-tv", .data = &sun8i_r40_tv_quirks },
+	{ .compatible = "allwinner,sun50i-a64-tcon-lcd", .data = &sun50i_a64_lcd_quirks },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, sun4i_tcon_of_table);

diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.h b/drivers/gpu/drm/sun4i/sun4i_tcon.h
index fa23aa23fe4a..bd4abc90062b 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.h
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.h

@@ -243,6 +243,7 @@ struct sun4i_tcon_quirks {
 	bool    needs_edp_reset; /* a80 edp reset needed for tcon0 access */
 	bool	supports_lvds;   /* Does the TCON support an LVDS output? */
 	bool	polarity_in_ch0; /* some tcon1 channels have polarity bits in tcon0 pol register */
+	bool	restores_rate;   /* restores the initial rate when rate changes */
 	u8	dclk_min_div;	/* minimum divider for TCON0 DCLK */

 	/* callback to handle tcon muxing options */

@@ -300,4 +301,15 @@ void sun4i_tcon_set_status(struct sun4i_tcon *crtc,

 extern const struct of_device_id sun4i_tcon_of_table[];

+struct sun4i_rate_reset_nb {
+	struct notifier_block	clk_nb;
+	struct delayed_work	reset_rate_work;
+
+	struct clk		*target_clk;
+	unsigned long		saved_rate;
+	bool			is_registered;
+};
+
+#define to_sun4i_rate_reset_nb(_nb) container_of(_nb, struct sun4i_rate_reset_nb, clk_nb)
+
 #endif /* __SUN4I_TCON_H__ */

---
base-commit: dcb6c8ee6acc6c347caec1e73fb900c0f4ff9806
change-id: 20240304-tcon_keep_stable_rate-5729c7706343

Best regards,

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel