Re: [PATCH v4 4/5] drm/panfrost: devfreq: set opp to the recommended one to configure regulator

[Steven Price]

On 06/09/2022 16:30, Clément Péron wrote:
> Enabling panfrost GPU OPP with dynamic regulator will make OPP
> responsible to enable and configure it.
> 
> Unfortunatly OPP configure and enable the regulator when an OPP

NIT: Unfortunately

> is asked to be set, which is not the case during
> panfrost_devfreq_init().
> 
> This leave the regulator unconfigured and if no GPU load is
> triggered, no OPP is asked to be set which make the regulator framework
> switching it off during regulator_late_cleanup() without
> noticing and therefore make the board hang as any access to GPU
> memory space make bus locks up.
> 
> Call dev_pm_opp_set_opp() with the recommend OPP in
> panfrost_devfreq_init() to enable the regulator, this will properly
> configure and enable the regulator and will avoid any switch off
> by regulator_late_cleanup().
> 
> Suggested-by: Viresh Kumar <viresh.kumar@xxxxxxxxxx>
> Signed-off-by: Clément Péron <peron.clem@xxxxxxxxx>

Reviewed-by: Steven Price <steven.price@xxxxxxx>

Note this same sequence is used in the Lima driver, so it would be good
to submit the fix there too as it presumably is affected by the same
issue. I've CC'd Qiang for visibility.

I'll push this patch to drm-misc-fixes (with the typo above fixed), the
device tree patches can go through a different tree.

Steve

> ---
>  drivers/gpu/drm/panfrost/panfrost_devfreq.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> index 5110cd9b2425..fe5f12f16a63 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> @@ -131,6 +131,17 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
>  		return PTR_ERR(opp);
>  
>  	panfrost_devfreq_profile.initial_freq = cur_freq;
> +
> +	/*
> +	 * Set the recommend OPP this will enable and configure the regulator
> +	 * if any and will avoid a switch off by regulator_late_cleanup()
> +	 */
> +	ret = dev_pm_opp_set_opp(dev, opp);
> +	if (ret) {
> +		DRM_DEV_ERROR(dev, "Couldn't set recommended OPP\n");
> +		return ret;
> +	}
> +
>  	dev_pm_opp_put(opp);
>  
>  	/*