drm/etnaviv: disable MLCG and pulse eater on GPU reset

[Sui Jingfeng]

Hi

On 2023/6/14 15:45, Lucas Stach wrote:
> Hi,
>
> Am Mittwoch, dem 14.06.2023 um 00:42 +0800 schrieb Sui Jingfeng:
>> Hi, Lucas
>>
>>
>> I love your patch, perhaps something to improve:
>>
>> The MLCG stand for "Module Level Clock Gating",
>>
>> without reading the commit message, I guess there may have people don't
>> know its meaning.
>>
> Yea, I expect people to read the commit message and not just the
> subject if they want to know what a patch does. The MLCG abbreviation
> is already well established within the driver code.

Yeah, I agree with you that the reviewer should read the commit message 
and the patch itself(code)

But after look the code quite a while, I'm wondering that

1) is the "Module Level" absolutely needed ?

2) is it accurate enough ?


For question in 1),  I meant that is it better by saying: "drm/etnaviv: 
disable clock gating and pulse eater on GPU reset"

For question in 2),  I mean that the code inside the 
etnaviv_hw_reset(struct etnaviv_gpu *gpu) function are per GPU instance 
level.


Every GPU instance managed by the drm/etnaviv will run those clock 
gating related code.

So it is per GPU instance level.


As far as I can understand, the "Module Level" stand for the 
drm/etnaviv(etnaviv.ko) as a whole

Nearly all patches for the gpu/drm/drivers are module level by default,

What's you really want to emphasize?


I think you probably want to drop the "ML",  and replace the "MLCG" with 
"clock gating".

explain the "ML" in the commit message should be enough.

>> There are still more thing in this patch can only be understand relay on
>> guessing... :-)
>>
> That's unfortunately true. I don't know exactly what the pulse eater
> control bits mean either. I've taken them verbatim from the reset
> sequence in the Vivante kernel driver, which is also why I didn't try
> to assign some meaning by giving them a name, but keep them as BITs in
> the driver code.
>
> Regards,
> Lucas
>
>> But drm/etnaviv drvier still works with this patch applied, so
>>
>>
>> On 2023/6/7 20:58, Lucas Stach wrote:
>>> Module level clock gating and the pulse eater might interfere with
>>> the GPU reset, as they both have the potential to stop the clock
>>> and thus reset propagation to parts of the GPU.
>>>
>>> Signed-off-by: Lucas Stach <l.stach at pengutronix.de>
>>> Reviewed-by: Christian Gmeiner <cgmeiner at igalia.com>
>>
>> Tested-by: Sui Jingfeng <suijingfeng at loongson.cn>
>>
>>
>>> ---
>>> I'm not aware of any cases where this would have been an issue, but
>>> it is what the downstream driver does and fundametally seems like
>>> the right thing to do.
>>> ---
>>>    drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 13 ++++++++++++-
>>>    1 file changed, 12 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
>>> index de8c9894967c..41aab1aa330b 100644
>>> --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
>>> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
>>> @@ -505,8 +505,19 @@ static int etnaviv_hw_reset(struct etnaviv_gpu *gpu)
>>>    	timeout = jiffies + msecs_to_jiffies(1000);
>>>    
>>>    	while (time_is_after_jiffies(timeout)) {
>>> -		/* enable clock */
>>>    		unsigned int fscale = 1 << (6 - gpu->freq_scale);
>>> +		u32 pulse_eater = 0x01590880;
>>> +
>>> +		/* disable clock gating */
>>> +		gpu_write_power(gpu, VIVS_PM_POWER_CONTROLS, 0x0);
>>> +
>>> +		/* disable pulse eater */
>>> +		pulse_eater |= BIT(17);
>>> +		gpu_write_power(gpu, VIVS_PM_PULSE_EATER, pulse_eater);
>>> +		pulse_eater |= BIT(0);
>>> +		gpu_write_power(gpu, VIVS_PM_PULSE_EATER, pulse_eater);
>>> +
>>> +		/* enable clock */
>>>    		control = VIVS_HI_CLOCK_CONTROL_FSCALE_VAL(fscale);
>>>    		etnaviv_gpu_load_clock(gpu, control);
>>>    

-- 
Jingfeng