On 10/04/2024 21:34, Martin Blumenstingl wrote:
Hi Neil, On Wed, Apr 3, 2024 at 9:46 AM Neil Armstrong <neil.armstrong@xxxxxxxxxx> wrote:Disable the px_clk when setting the rate to recover a fully configured and correctly reset VCLK clock tree after the rate is set. Fixes: 77d9e1e6b846 ("drm/meson: add support for MIPI-DSI transceiver") Signed-off-by: Neil Armstrong <neil.armstrong@xxxxxxxxxx> --- drivers/gpu/drm/meson/meson_dw_mipi_dsi.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/drivers/gpu/drm/meson/meson_dw_mipi_dsi.c b/drivers/gpu/drm/meson/meson_dw_mipi_dsi.c index a6bc1bdb3d0d..a10cff3ca1fe 100644 --- a/drivers/gpu/drm/meson/meson_dw_mipi_dsi.c +++ b/drivers/gpu/drm/meson/meson_dw_mipi_dsi.c @@ -95,6 +95,7 @@ static int dw_mipi_dsi_phy_init(void *priv_data) return ret; } + clk_disable_unprepare(mipi_dsi->px_clk);nit-pick: clk_disable(mipi_dsi->px_clk); should be enough here as my understanding is that we only need to {un,}prepare a clock once.ret = clk_set_rate(mipi_dsi->px_clk, mipi_dsi->mode->clock * 1000); if (ret) { @@ -103,6 +104,12 @@ static int dw_mipi_dsi_phy_init(void *priv_data) return ret; } + ret = clk_prepare_enable(mipi_dsi->px_clk); + if (ret) { + dev_err(mipi_dsi->dev, "Failed to enable DSI Pixel clock (ret %d)\n", ret); + return ret;If we ever hit this error case then there will be a lot of additional errors in the kernel log: - initially the clock is prepared and enabled in meson_dw_mipi_dsi_probe() by calling devm_clk_get_enabled() - we then disable the clock above (generally disabling a clock is expected to always succeed) - if the clock can NOT be re-enabled here we just log the error - in case a user tries to rmmod the driver (to modprobe it again) to try and recover from an error the automatic disabling of the pix_clk (based on devm_clk_get_enabled() where it was enabled initially) there will be a splat because the clock is already disabled (and enabled count is zero, so it cannot be disabled any further) For the 32-bit SoC video clocks I keep track of them being enabled or disabled, see [0], [1] and [2]. In my case this is important because we can run into cases where the PLL doesn't lock (I am not sure how likely this is for your case). It *seems* like we need to do something similar as dw_mipi_dsi_phy_init() can be called when changing the display resolution (or whenever drm_bridge_funcs.atomic_pre_enable) is called. To illustrate what I have in mind I attached a diff (it's based on this patch) - it's compile tested only as I have no DSI hardware. In case dw_mipi_dsi_phy_init() is called only once per device lifecycle things may get easier.
Indeed your scheme looks good, I'll try it since indeed we only need to prepare it once in the lifetime of the driver.
PS: I'm so happy that we don't need any clock notifiers for this! So: good work with the clock driver bits.
Thx !
Let me know what you think, Martin [0] https://github.com/xdarklight/linux/blob/meson-mx-integration-6.9-20240323/drivers/gpu/drm/meson/meson_vclk.c#L1177-L1179 [1] https://github.com/xdarklight/linux/blob/meson-mx-integration-6.9-20240323/drivers/gpu/drm/meson/meson_vclk.c#L1077 [2] https://github.com/xdarklight/linux/blob/meson-mx-integration-6.9-20240323/drivers/gpu/drm/meson/meson_vclk.c#L1053
