Message-ID: <87sf0twep0.fsf@oltmanns.dev>
Date: Thu, 14 Mar 2024 06:43:55 +0100
From: Frank Oltmanns <frank@...manns.dev>
To: Jernej Škrabec <jernej.skrabec@...il.com>
Cc: Maxime Ripard <mripard@...nel.org>,  Chen-Yu Tsai <wens@...e.org>,
  Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,  Thomas Zimmermann
 <tzimmermann@...e.de>,  David Airlie <airlied@...il.com>,  Daniel Vetter
 <daniel@...ll.ch>,  Samuel Holland <samuel@...lland.org>,  Icenowy Zheng
 <uwu@...nowy.me>,  Ondrej Jirman <x@...x.eu>,
  dri-devel@...ts.freedesktop.org,  linux-arm-kernel@...ts.infradead.org,
  linux-sunxi@...ts.linux.dev,  linux-kernel@...r.kernel.org
Subject: Re: [PATCH] drm/sun4i: tcon: Support keeping dclk rate upon
 ancestor clock changes

On 2024-03-13 at 19:11:37 +0100, Jernej Škrabec <jernej.skrabec@...il.com> 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 <frank@...manns.dev>
>> ---
>> 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/
>> [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,
>>

Powered by blists - more mailing lists