[07/10] drm/tidss: Fix dss reset

  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.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/gpu/drm/tidss/tidss_dispc.c | 45 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 44 insertions(+), 1 deletion(-)
  

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@ideasonboard.com>
> ---
>  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;
>
  

On 01/11/2023 16:30, Laurent Pinchart wrote:
> 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()

Can you explain what that would accomplish, or why the code in this 
patch is wrong?

If I understand it right, you're suggesting a more "normal" power up at 
the probe time, and then leaving the DSS enabled, but with autosuspend. 
That would require powering up, doing a reset, and calling 
dispc_runtime_resume. Which can be done, but I'm not sure why it's 
better, as we're not interested in "normal" power up at probe time.

But I can see that my approach looks perhaps a bit odd just by looking 
at these patches. This work was related to keeping the bootloader's 
splash screen on the screen for a longer time, i.e. delaying reset.

For that, I wanted an early function (dispc_init_hw) which would, 
instead of always resetting the DSS as it does in this version, peek at 
the DSS hardware, and see if the DSS is already streaming. If no, do a 
reset and proceed normally. If yes, skip the reset, leave the clocks 
enabled, and keep DSS PM active.

Later, when we'd be doing the first modeset, the driver would do the 
initial reset.

So, that's why I wanted an independent function for the HW probing/init, 
which is called before runtime PM is enabled, and I did not want normal 
runtime resume to be called as dispc_runtime_resume() would break the 
display.

I think a better solution would be to set up the fb of tidss's fbdev to 
use the reserved memory, used for the boot splash screen. But I didn't 
figure out a way to do this. But even there we'd like to delay the reset 
until the first modeset (when the fbdev display is getting enabled).

  Tomi
  

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.

Not sure on the exact implication here, however from the description
this seems a candidate for stable and would need a Fixes tag.

Francesco