Re: [PATCH 4/5] drm/panfrost: add amlogic reset quirk callback

[Neil Armstrong <narmstrong@baylibre.com>]

Hi,
On 09/09/2020 14:23, Steven Price wrote:
> On 08/09/2020 16:18, Neil Armstrong wrote:
>> The T820, G31 & G52 GPUs integratewd by Amlogic in the respective GXM, G12A/SM1 & G12B
>> SoCs needs a quirk in the PWR registers at the GPU reset time.
>>
>> Since the documentation of the GPU cores are not public, we do not know what does these
>> values, but they permit having a fully functional GPU running with Panfrost.
>>
>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>> ---
>>   drivers/gpu/drm/panfrost/panfrost_gpu.c  | 13 +++++++++++++
>>   drivers/gpu/drm/panfrost/panfrost_gpu.h  |  2 ++
>>   drivers/gpu/drm/panfrost/panfrost_regs.h |  3 +++
>>   3 files changed, 18 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/panfrost/panfrost_gpu.c b/drivers/gpu/drm/panfrost/panfrost_gpu.c
>> index c129aaf77790..018737bd4ac6 100644
>> --- a/drivers/gpu/drm/panfrost/panfrost_gpu.c
>> +++ b/drivers/gpu/drm/panfrost/panfrost_gpu.c
>> @@ -80,6 +80,19 @@ int panfrost_gpu_soft_reset(struct panfrost_device *pfdev)
>>       return 0;
>>   }
>>   +void panfrost_gpu_amlogic_quirks(struct panfrost_device *pfdev)
>> +{
>> +    /*
>> +     * The Amlogic integrated Mali-T820, Mali-G31 & Mali-G52 needs
>> +     * these undocumented bits to be set in order to operate
>> +     * correctly.
>> +     * These GPU_PWR registers contains:
>> +     * "device-specific power control value"
>> +     */
>> +    gpu_write(pfdev, GPU_PWR_KEY, 0x2968A819);
> 
> As Alyssa has mentioned this magic value is not Amlogic specific, but is just the unlock key value, so please add the define in panfrost-gpu.h

Acked

> 
>> +    gpu_write(pfdev, GPU_PWR_OVERRIDE1, 0xfff | (0x20 << 16));
> 
> But PWR_OVERRIDE1 is indeed device specific so I can't offer an insight here.

Yep.

> 
>> +}
>> +
>>   static void panfrost_gpu_init_quirks(struct panfrost_device *pfdev)
>>   {
>>       u32 quirks = 0;
>> diff --git a/drivers/gpu/drm/panfrost/panfrost_gpu.h b/drivers/gpu/drm/panfrost/panfrost_gpu.h
>> index 4112412087b2..a881d7dc812f 100644
>> --- a/drivers/gpu/drm/panfrost/panfrost_gpu.h
>> +++ b/drivers/gpu/drm/panfrost/panfrost_gpu.h
>> @@ -16,4 +16,6 @@ int panfrost_gpu_soft_reset(struct panfrost_device *pfdev);
>>   void panfrost_gpu_power_on(struct panfrost_device *pfdev);
>>   void panfrost_gpu_power_off(struct panfrost_device *pfdev);
>>   +void panfrost_gpu_amlogic_reset_quirk(struct panfrost_device *pfdev);
> 
> You need to be consistent about the name - this has _reset_, the above function doesn't.

Yep, will be fixed in next version.

Thanks for the review,
Neil

> 
> Steve
> 
>> +
>>   #endif
>> diff --git a/drivers/gpu/drm/panfrost/panfrost_regs.h b/drivers/gpu/drm/panfrost/panfrost_regs.h
>> index ea38ac60581c..fa0d02f3c830 100644
>> --- a/drivers/gpu/drm/panfrost/panfrost_regs.h
>> +++ b/drivers/gpu/drm/panfrost/panfrost_regs.h
>> @@ -51,6 +51,9 @@
>>   #define GPU_STATUS            0x34
>>   #define   GPU_STATUS_PRFCNT_ACTIVE    BIT(2)
>>   #define GPU_LATEST_FLUSH_ID        0x38
>> +#define GPU_PWR_KEY            0x050    /* (WO) Power manager key register */
>> +#define GPU_PWR_OVERRIDE0        0x054    /* (RW) Power manager override settings */
>> +#define GPU_PWR_OVERRIDE1        0x058    /* (RW) Power manager override settings */
>>   #define GPU_FAULT_STATUS        0x3C
>>   #define GPU_FAULT_ADDRESS_LO        0x40
>>   #define GPU_FAULT_ADDRESS_HI        0x44
>>
> 


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic