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 <[email protected]>
---
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/[email protected]/
[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));
+
+ 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;
+
+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);
+ }
+}
+
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,
--
Frank Oltmanns <[email protected]>
Hello Frank,
On Sun, Mar 10, 2024 at 02:32:29PM +0100, Frank Oltmanns wrote:
> +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));
If you get multiple reset notifier calls within 100ms of the first one,
the delay from the last one will not be 100ms, so this may violate expectations
you're describing in the commit message.
schedule_delayed_work doesn't re-schedule the work if it's already pending.
Kind regards,
o.
Hello Ondřej,
On 2024-03-10 at 23:23:57 +0100, Ondřej Jirman <[email protected]> wrote:
> Hello Frank,
>
> On Sun, Mar 10, 2024 at 02:32:29PM +0100, Frank Oltmanns wrote:
>> +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));
>
> If you get multiple reset notifier calls within 100ms of the first one,
> the delay from the last one will not be 100ms, so this may violate expectations
> you're describing in the commit message.
Let me start by saying, the implicit expectation is that the new user of
pll-video0 will get an exclusive lock on pll-video0 (otherwise,
data-clock would reset pll-video0 after those 100ms rendering the whole
endeavor of the new user pointless). This constraint makes the chances
that the above event (two consecutive rate changes within 100ms) occurs
very slim.
That being said, I don't see a problem with cancelling the delayed work
on PRE_RATE_CHANGE and restarting it on ABORT_RATE_CHANGE like this:
+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 == PRE_RATE_CHANGE) {
+ rate_reset->is_cancelled = cancel_delayed_work(&rate_reset->reset_rate_work);
+ } else if ((event == POST_RATE_CHANGE) ||
+ (event == ABORT_RATE_CHANGE) && rate_reset->is_cancelled) {
+ schedule_delayed_work(&rate_reset->reset_rate_work, msecs_to_jiffies(100));
+ rate_reset->is_cancelled = false;
+ }
+
+ return NOTIFY_DONE;
+}
The need for this new code is slim (IMHO) but on the other hand it
doesn't add much complexity. I'll add it in V2 including the
is_cancelled member in sun4i_rate_reset_nb if I don't receive any
objections during this week.
Thanks,
Frank
>
> schedule_delayed_work doesn't re-schedule the work if it's already pending.
>
> Kind regards,
> o.
Hi Frank!
Thanks on tackling this issue.
Dne nedelja, 10. marec 2024 ob 14:32:29 CET je Frank Oltmanns napisal(a):
> 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 <[email protected]>
> ---
> 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/[email protected]/
> [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
> +
> + 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.
> +
> +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?
Best regards,
Jernej
> + }
> +}
> +
> 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,
>
On 2024-03-13 at 19:11:37 +0100, Jernej Škrabec <[email protected]> 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):
>> 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 <[email protected]>
>> ---
>> 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/[email protected]/
>> [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!
>> +
>> + 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.
>
>> +
>> +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
>
>> + }
>> +}
>> +
>> 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,
>>
Hi,
On Sun, Mar 10, 2024 at 02:32:29PM +0100, Frank Oltmanns wrote:
> 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 <[email protected]>
> ---
> 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.
Workqueues don't have an upper boundary on when they execute. As we
discussed multiple times, this should be solved in the clock framework
itself, not bypassing it.
Maxime
Dne četrtek, 14. marec 2024 ob 15:42:24 CET je Maxime Ripard napisal(a):
> Hi,
>
> On Sun, Mar 10, 2024 at 02:32:29PM +0100, Frank Oltmanns wrote:
> > 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 <[email protected]>
> > ---
> > 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.
>
> Workqueues don't have an upper boundary on when they execute. As we
> discussed multiple times, this should be solved in the clock framework
> itself, not bypassing it.
I think TCON code still needs to be touched due to clk_rate_exclusive_get()
calls which effectively lock whole chain. You can't have both TCONs locking
rate on A64 for this to work correctly.
What was original reason for clk_rate_exclusive_get()? I forgot already.
Best regards,
Jernej
>
> Maxime
>
On Thu, Mar 14, 2024 at 06:20:58PM +0100, Jernej Škrabec wrote:
> Dne četrtek, 14. marec 2024 ob 15:42:24 CET je Maxime Ripard napisal(a):
> > On Sun, Mar 10, 2024 at 02:32:29PM +0100, Frank Oltmanns wrote:
> > > 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 <[email protected]>
> > > ---
> > > 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.
> >
> > Workqueues don't have an upper boundary on when they execute. As we
> > discussed multiple times, this should be solved in the clock framework
> > itself, not bypassing it.
>
> I think TCON code still needs to be touched due to clk_rate_exclusive_get()
> calls which effectively lock whole chain. You can't have both TCONs locking
> rate on A64 for this to work correctly.
>
> What was original reason for clk_rate_exclusive_get()? I forgot already.
IIRC, it was because the D-PHY and DSI controller derive from the same
clock, and we needed to make sure setting one wouldn't affect the other
one.
Maxime