diff mbox series

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

Message ID 20200908151853.4837-5-narmstrong@baylibre.com (mailing list archive)
State New, archived
Headers show
Series drm/panfrost: add Amlogic integration quirks | expand

Commit Message

Neil Armstrong Sept. 8, 2020, 3:18 p.m. UTC 
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(+)

Comments

Alyssa Rosenzweig Sept. 8, 2020, 7:10 p.m. UTC | #1 
> 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.

Since this is Amlogic magic, not specifically GPU, I'd rephrase this as
"Since the Amlogic's integration of the GPU cores with the SoC is not
publicly documented..."

> +	/*
> +	 * 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"
> +	 */

PWR_OVERRIDE1 is the Amlogic specific value.

Per the name, for PWR_KEY, I'd do add "#define GPU_PWR_KEY_UNLOCK
0x2968A819" in panfrost-gpu.h so it's clear which value is which.
kernel test robot Sept. 8, 2020, 9:10 p.m. UTC | #2 
Hi Neil,

I love your patch! Perhaps something to improve:

[auto build test WARNING on iommu/next]
[also build test WARNING on soc/for-next linus/master v5.9-rc4 next-20200908]
[cannot apply to xlnx/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Neil-Armstrong/drm-panfrost-add-Amlogic-integration-quirks/20200908-232205
base:   https://git.kernel.org/pub/scm/linux/kernel/git/joro/iommu.git next
config: arm-randconfig-r014-20200908 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project df63eedef64d715ce1f31843f7de9c11fe1e597f)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install arm cross compiling tool for clang build
        # apt-get install binutils-arm-linux-gnueabi
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=arm 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/gpu/drm/panfrost/panfrost_gpu.c:82:6: warning: no previous prototype for function 'panfrost_gpu_amlogic_quirks' [-Wmissing-prototypes]
   void panfrost_gpu_amlogic_quirks(struct panfrost_device *pfdev)
        ^
   drivers/gpu/drm/panfrost/panfrost_gpu.c:82:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   void panfrost_gpu_amlogic_quirks(struct panfrost_device *pfdev)
   ^
   static 
   1 warning generated.

# https://github.com/0day-ci/linux/commit/10c9c1e2d5b129f4936ea0b84cb489da43364d8c
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Neil-Armstrong/drm-panfrost-add-Amlogic-integration-quirks/20200908-232205
git checkout 10c9c1e2d5b129f4936ea0b84cb489da43364d8c
vim +/panfrost_gpu_amlogic_quirks +82 drivers/gpu/drm/panfrost/panfrost_gpu.c

    81	
  > 82	void panfrost_gpu_amlogic_quirks(struct panfrost_device *pfdev)
    83	{
    84		/*
    85		 * The Amlogic integrated Mali-T820, Mali-G31 & Mali-G52 needs
    86		 * these undocumented bits to be set in order to operate
    87		 * correctly.
    88		 * These GPU_PWR registers contains:
    89		 * "device-specific power control value"
    90		 */
    91		gpu_write(pfdev, GPU_PWR_KEY, 0x2968A819);
    92		gpu_write(pfdev, GPU_PWR_OVERRIDE1, 0xfff | (0x20 << 16));
    93	}
    94	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Steven Price Sept. 9, 2020, 12:23 p.m. UTC | #3 
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

> +	gpu_write(pfdev, GPU_PWR_OVERRIDE1, 0xfff | (0x20 << 16));

But PWR_OVERRIDE1 is indeed device specific so I can't offer an insight 
here.

> +}
> +
>   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.

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
>
Neil Armstrong Sept. 9, 2020, 12:27 p.m. UTC | #4 
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
>>
>
diff mbox series

Patch

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);
+	gpu_write(pfdev, GPU_PWR_OVERRIDE1, 0xfff | (0x20 << 16));
+}
+
 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);
+
 #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