Hi Tomi,
Thank you for the patch.
On Wed, Nov 01, 2023 at 11:17:44AM +0200, Tomi Valkeinen wrote:
> The probe function calls dispc_softreset() before runtime PM is enabled
> and without enabling any of the DSS clocks. This happens to work by
> luck, and we need to make sure the DSS HW is active and the fclk is
> enabled.
>
> To fix the above, add a new function, dispc_init_hw(), which does:
>
> - pm_runtime_set_active()
> - clk_prepare_enable(fclk)
> - dispc_softreset().
>
> This ensures that the reset can be successfully accomplished.
>
> Note that we use pm_runtime_set_active(), not the normal
> pm_runtime_get(). The reason for this is that at this point we haven't
> enabled the runtime PM yet and also we don't want the normal resume
> callback to be called: the dispc resume callback does some initial HW
> setup, and it expects that the HW was off (no video ports are
> streaming). If the bootloader has enabled the DSS and has set up a
> boot time splash-screen, the DSS would be enabled and streaming which
> might lead to issues with the normal resume callback.
I think the right way to do this would be, in probe(), to
- power on the device
- enable runtime PM, masking the device as active
- at end of probe, calling pm_runtime_put_autosuspend()
Please also see my review of patch 02/10.
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/tidss/tidss_dispc.c | 45 ++++++++++++++++++++++++++++++++++++-
> 1 file changed, 44 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
> index f204a0701e9f..13db062892e3 100644
> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
> @@ -2724,6 +2724,49 @@ static int dispc_softreset(struct dispc_device *dispc)
> return 0;
> }
>
> +static int dispc_init_hw(struct dispc_device *dispc)
> +{
> + struct device *dev = dispc->dev;
> + int ret;
> +
> + ret = pm_runtime_set_active(dev);
> + if (ret) {
> + dev_err(dev, "Failed to set DSS PM to active\n");
> + return ret;
> + }
> +
> + ret = clk_prepare_enable(dispc->fclk);
> + if (ret) {
> + dev_err(dev, "Failed to enable DSS fclk\n");
> + goto err_runtime_suspend;
> + }
> +
> + ret = dispc_softreset(dispc);
> + if (ret)
> + goto err_clk_disable;
> +
> + clk_disable_unprepare(dispc->fclk);
> + ret = pm_runtime_set_suspended(dev);
> + if (ret) {
> + dev_err(dev, "Failed to set DSS PM to suspended\n");
> + return ret;
> + }
> +
> + return 0;
> +
> +err_clk_disable:
> + clk_disable_unprepare(dispc->fclk);
> +
> +err_runtime_suspend:
> + ret = pm_runtime_set_suspended(dev);
> + if (ret) {
> + dev_err(dev, "Failed to set DSS PM to suspended\n");
> + return ret;
> + }
> +
> + return ret;
> +}
> +
> int dispc_init(struct tidss_device *tidss)
> {
> struct device *dev = tidss->dev;
> @@ -2835,7 +2878,7 @@ int dispc_init(struct tidss_device *tidss)
>
> tidss->dispc = dispc;
>
> - r = dispc_softreset(dispc);
> + r = dispc_init_hw(dispc);
> if (r)
> return r;
>
--
Regards,
Laurent Pinchart
