[PATCH v2 1/7] drm: mxsfb: Simplify LCDIF clock handling

[Marek Vasut]

On 4/6/22 21:32, Lucas Stach wrote:
> Hi Marek,

Hi,

> Am Freitag, dem 11.03.2022 um 18:05 +0100 schrieb Marek Vasut:
>> The current clock handling in the LCDIF driver is a convoluted mess.
> 
> Here we agree...
> 
>> Implement runtime PM ops which turn the clock ON and OFF and let the
>> pm_runtime_get_sync()/pm_runtime_put_sync() calls in .atomic_enable
>> and .atomic_disable callbacks turn the clock ON and OFF at the right
>> time.
>>
>> This requires slight reordering in mxsfb_crtc_atomic_enable() and
>> mxsfb_crtc_atomic_disable(), since all the register accesses must
>> happen only with clock enabled and clock frequency configuration
>> must happen with clock disabled.
>>
> ... on that one I don't. Please don't move the pixel clock into the RPM
> calls. We have a very well defined point between atomic enable/disable
> where the pixel clock is actually needed. All the other configuration
> accesses don't need the pixel clock to be active.

On the other hand, "all the other" configuration happens during probe, 
at which point all the clock are enabled anyway. And then during 
runtime, the pixel clock of this IP are either enabled or this IP is 
completely shut down.

So, where exactly does this patch make the clock handling any worse than 
it currently is ?

[...]

>> @@ -274,44 +256,37 @@ static int mxsfb_load(struct drm_device *drm,
>>   
>>   	ret = platform_get_irq(pdev, 0);
>>   	if (ret < 0)
>> -		goto err_vblank;
>> +		return ret;
>>   	mxsfb->irq = ret;
>>   
>> -	pm_runtime_get_sync(drm->dev);
>>   	ret = mxsfb_irq_install(drm, mxsfb->irq);
>> -	pm_runtime_put_sync(drm->dev);
>> -
> Here you do a bunch of stuff resulting in register access without
> enabling the clocks. I don't really see how this works, maybe because
> the clocks are still on when you run the probe?

Right

> Better enable the register access clocks here and then tell RPM about
> the fact that the device is running by calling pm_runtime_set_active
> before pm_runtime_enable. This way theoretically allows to build a
> kernel without CONFIG_PM and things still work, even if the RPM calls
> are stubs.

I would much rather move this driver to RPM and have RPM handle the 
clock altogether in one place.

[...]

>> @@ -411,9 +405,6 @@ static void mxsfb_crtc_atomic_disable(struct drm_crtc *crtc,
>>   	}
>>   	spin_unlock_irq(&drm->event_lock);
>>   
>> -	drm_crtc_vblank_off(crtc);
>> -
>> -	mxsfb_disable_axi_clk(mxsfb);
>>   	pm_runtime_put_sync(drm->dev);
>>
> Not the fault of your patch, but why is this a _sync call?

See 4201f4e848450 ("drm/mxsfb: Add pm_runtime calls to 
pipe_enable/disable"), likely the intention was for this call to 
complete before existing the atomic_disable.