[PATCH 0/5] drm/panfrost: add Amlogic integration quirks

[PATCH 0/5] drm/panfrost: add Amlogic integration quirks

[Neil Armstrong]

The T820, G31 & G52 GPUs integrated by Amlogic in the respective GXM, G12A/SM1 & G12B
SoCs needs a quirk in the PWR registers at the GPU reset time.

The coherency integration of the IOMMU in the Mali-G52 found in the Amlogic G12B SoCs
is broken and leads to constant and random faults from the IOMMU.

This serie adds the necessary quirks for the Amlogic integrated GPUs only.

Neil Armstrong (5):
  iommu/io-pgtable-arm: Add BROKEN_NS quirk to disable shareability on
    ARM LPAE
  drm/panfrost: add support specifying pgtbl quirks
  drm/panfrost: add support for reset quirk
  drm/panfrost: add amlogic reset quirk callback
  drm/panfrost: add Amlogic GPU integration quirks

 drivers/gpu/drm/panfrost/panfrost_device.h |  6 ++++++
 drivers/gpu/drm/panfrost/panfrost_drv.c    | 18 ++++++++++++++++++
 drivers/gpu/drm/panfrost/panfrost_gpu.c    | 17 +++++++++++++++++
 drivers/gpu/drm/panfrost/panfrost_gpu.h    |  2 ++
 drivers/gpu/drm/panfrost/panfrost_mmu.c    |  1 +
 drivers/gpu/drm/panfrost/panfrost_regs.h   |  3 +++
 drivers/iommu/io-pgtable-arm.c             |  7 ++++---
 include/linux/io-pgtable.h                 |  4 ++++
 8 files changed, 55 insertions(+), 3 deletions(-)

-- 
2.22.0

[PATCH 1/5] iommu/io-pgtable-arm: Add BROKEN_NS quirk to disable shareability on ARM LPAE

[Neil Armstrong]

The coherency integration of the IOMMU in the Mali-G52 found in the Amlogic G12B SoCs
is broken and leads to constant and random faults from the IOMMU.

Disabling shareability completely fixes the issue.

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 drivers/iommu/io-pgtable-arm.c | 7 ++++---
 include/linux/io-pgtable.h     | 4 ++++
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
index dc7bcf858b6d..d2d48dc86556 100644
--- a/drivers/iommu/io-pgtable-arm.c
+++ b/drivers/iommu/io-pgtable-arm.c
@@ -440,7 +440,9 @@ static arm_lpae_iopte arm_lpae_prot_to_pte(struct arm_lpae_io_pgtable *data,
 				<< ARM_LPAE_PTE_ATTRINDX_SHIFT);
 	}
 
-	if (prot & IOMMU_CACHE)
+	if (data->iop.cfg.quirks & IO_PGTABLE_QUIRK_ARM_BROKEN_SH)
+		pte |= ARM_LPAE_PTE_SH_NS;
+	else if (prot & IOMMU_CACHE)
 		pte |= ARM_LPAE_PTE_SH_IS;
 	else
 		pte |= ARM_LPAE_PTE_SH_OS;
@@ -1005,8 +1007,7 @@ arm_mali_lpae_alloc_pgtable(struct io_pgtable_cfg *cfg, void *cookie)
 {
 	struct arm_lpae_io_pgtable *data;
 
-	/* No quirks for Mali (hopefully) */
-	if (cfg->quirks)
+	if (cfg->quirks & ~(IO_PGTABLE_QUIRK_ARM_BROKEN_SH))
 		return NULL;
 
 	if (cfg->ias > 48 || cfg->oas > 40)
diff --git a/include/linux/io-pgtable.h b/include/linux/io-pgtable.h
index 23285ba645db..efb9c8f20909 100644
--- a/include/linux/io-pgtable.h
+++ b/include/linux/io-pgtable.h
@@ -86,6 +86,9 @@ struct io_pgtable_cfg {
 	 *
 	 * IO_PGTABLE_QUIRK_ARM_TTBR1: (ARM LPAE format) Configure the table
 	 *	for use in the upper half of a split address space.
+	 *
+	 * IO_PGTABLE_QUIRK_ARM_BROKEN_SH: (ARM LPAE format) Disables shareability
+	 *	when coherency integration is broken.
 	 */
 	#define IO_PGTABLE_QUIRK_ARM_NS		BIT(0)
 	#define IO_PGTABLE_QUIRK_NO_PERMS	BIT(1)
@@ -93,6 +96,7 @@ struct io_pgtable_cfg {
 	#define IO_PGTABLE_QUIRK_ARM_MTK_EXT	BIT(3)
 	#define IO_PGTABLE_QUIRK_NON_STRICT	BIT(4)
 	#define IO_PGTABLE_QUIRK_ARM_TTBR1	BIT(5)
+	#define IO_PGTABLE_QUIRK_ARM_BROKEN_SH	BIT(6)
 	unsigned long			quirks;
 	unsigned long			pgsize_bitmap;
 	unsigned int			ias;
-- 
2.22.0

Re: [PATCH 1/5] iommu/io-pgtable-arm: Add BROKEN_NS quirk to disable shareability on ARM LPAE

[Steven Price]

Subject: s/BROKEN_NS/BROKEN_SH/

Steve

On 08/09/2020 16:18, Neil Armstrong wrote:
> The coherency integration of the IOMMU in the Mali-G52 found in the Amlogic G12B SoCs
> is broken and leads to constant and random faults from the IOMMU.
> 
> Disabling shareability completely fixes the issue.
> 
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
>   drivers/iommu/io-pgtable-arm.c | 7 ++++---
>   include/linux/io-pgtable.h     | 4 ++++
>   2 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
> index dc7bcf858b6d..d2d48dc86556 100644
> --- a/drivers/iommu/io-pgtable-arm.c
> +++ b/drivers/iommu/io-pgtable-arm.c
> @@ -440,7 +440,9 @@ static arm_lpae_iopte arm_lpae_prot_to_pte(struct arm_lpae_io_pgtable *data,
>   				<< ARM_LPAE_PTE_ATTRINDX_SHIFT);
>   	}
>   
> -	if (prot & IOMMU_CACHE)
> +	if (data->iop.cfg.quirks & IO_PGTABLE_QUIRK_ARM_BROKEN_SH)
> +		pte |= ARM_LPAE_PTE_SH_NS;
> +	else if (prot & IOMMU_CACHE)
>   		pte |= ARM_LPAE_PTE_SH_IS;
>   	else
>   		pte |= ARM_LPAE_PTE_SH_OS;
> @@ -1005,8 +1007,7 @@ arm_mali_lpae_alloc_pgtable(struct io_pgtable_cfg *cfg, void *cookie)
>   {
>   	struct arm_lpae_io_pgtable *data;
>   
> -	/* No quirks for Mali (hopefully) */
> -	if (cfg->quirks)
> +	if (cfg->quirks & ~(IO_PGTABLE_QUIRK_ARM_BROKEN_SH))
>   		return NULL;
>   
>   	if (cfg->ias > 48 || cfg->oas > 40)
> diff --git a/include/linux/io-pgtable.h b/include/linux/io-pgtable.h
> index 23285ba645db..efb9c8f20909 100644
> --- a/include/linux/io-pgtable.h
> +++ b/include/linux/io-pgtable.h
> @@ -86,6 +86,9 @@ struct io_pgtable_cfg {
>   	 *
>   	 * IO_PGTABLE_QUIRK_ARM_TTBR1: (ARM LPAE format) Configure the table
>   	 *	for use in the upper half of a split address space.
> +	 *
> +	 * IO_PGTABLE_QUIRK_ARM_BROKEN_SH: (ARM LPAE format) Disables shareability
> +	 *	when coherency integration is broken.
>   	 */
>   	#define IO_PGTABLE_QUIRK_ARM_NS		BIT(0)
>   	#define IO_PGTABLE_QUIRK_NO_PERMS	BIT(1)
> @@ -93,6 +96,7 @@ struct io_pgtable_cfg {
>   	#define IO_PGTABLE_QUIRK_ARM_MTK_EXT	BIT(3)
>   	#define IO_PGTABLE_QUIRK_NON_STRICT	BIT(4)
>   	#define IO_PGTABLE_QUIRK_ARM_TTBR1	BIT(5)
> +	#define IO_PGTABLE_QUIRK_ARM_BROKEN_SH	BIT(6)
>   	unsigned long			quirks;
>   	unsigned long			pgsize_bitmap;
>   	unsigned int			ias;
> 

Re: [PATCH 1/5] iommu/io-pgtable-arm: Add BROKEN_NS quirk to disable shareability on ARM LPAE

[Neil Armstrong]

On 09/09/2020 14:23, Steven Price wrote:
> Subject: s/BROKEN_NS/BROKEN_SH/

Thanks,
Neil

> 
> Steve
> 
> On 08/09/2020 16:18, Neil Armstrong wrote:
>> The coherency integration of the IOMMU in the Mali-G52 found in the Amlogic G12B SoCs
>> is broken and leads to constant and random faults from the IOMMU.
>>
>> Disabling shareability completely fixes the issue.
>>
>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>> ---
>>   drivers/iommu/io-pgtable-arm.c | 7 ++++---
>>   include/linux/io-pgtable.h     | 4 ++++
>>   2 files changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
>> index dc7bcf858b6d..d2d48dc86556 100644
>> --- a/drivers/iommu/io-pgtable-arm.c
>> +++ b/drivers/iommu/io-pgtable-arm.c
>> @@ -440,7 +440,9 @@ static arm_lpae_iopte arm_lpae_prot_to_pte(struct arm_lpae_io_pgtable *data,
>>                   << ARM_LPAE_PTE_ATTRINDX_SHIFT);
>>       }
>>   -    if (prot & IOMMU_CACHE)
>> +    if (data->iop.cfg.quirks & IO_PGTABLE_QUIRK_ARM_BROKEN_SH)
>> +        pte |= ARM_LPAE_PTE_SH_NS;
>> +    else if (prot & IOMMU_CACHE)
>>           pte |= ARM_LPAE_PTE_SH_IS;
>>       else
>>           pte |= ARM_LPAE_PTE_SH_OS;
>> @@ -1005,8 +1007,7 @@ arm_mali_lpae_alloc_pgtable(struct io_pgtable_cfg *cfg, void *cookie)
>>   {
>>       struct arm_lpae_io_pgtable *data;
>>   -    /* No quirks for Mali (hopefully) */
>> -    if (cfg->quirks)
>> +    if (cfg->quirks & ~(IO_PGTABLE_QUIRK_ARM_BROKEN_SH))
>>           return NULL;
>>         if (cfg->ias > 48 || cfg->oas > 40)
>> diff --git a/include/linux/io-pgtable.h b/include/linux/io-pgtable.h
>> index 23285ba645db..efb9c8f20909 100644
>> --- a/include/linux/io-pgtable.h
>> +++ b/include/linux/io-pgtable.h
>> @@ -86,6 +86,9 @@ struct io_pgtable_cfg {
>>        *
>>        * IO_PGTABLE_QUIRK_ARM_TTBR1: (ARM LPAE format) Configure the table
>>        *    for use in the upper half of a split address space.
>> +     *
>> +     * IO_PGTABLE_QUIRK_ARM_BROKEN_SH: (ARM LPAE format) Disables shareability
>> +     *    when coherency integration is broken.
>>        */
>>       #define IO_PGTABLE_QUIRK_ARM_NS        BIT(0)
>>       #define IO_PGTABLE_QUIRK_NO_PERMS    BIT(1)
>> @@ -93,6 +96,7 @@ struct io_pgtable_cfg {
>>       #define IO_PGTABLE_QUIRK_ARM_MTK_EXT    BIT(3)
>>       #define IO_PGTABLE_QUIRK_NON_STRICT    BIT(4)
>>       #define IO_PGTABLE_QUIRK_ARM_TTBR1    BIT(5)
>> +    #define IO_PGTABLE_QUIRK_ARM_BROKEN_SH    BIT(6)
>>       unsigned long            quirks;
>>       unsigned long            pgsize_bitmap;
>>       unsigned int            ias;
>>
> 

[PATCH 2/5] drm/panfrost: add support specifying pgtbl quirks

[Neil Armstrong]

Add a pgtbl_quirks entry in the compatible specific table to permit specyfying IOMMU
quirks for platforms.

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 drivers/gpu/drm/panfrost/panfrost_device.h | 3 +++
 drivers/gpu/drm/panfrost/panfrost_mmu.c    | 1 +
 2 files changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h
index 953f7536a773..2cf1a6a13af8 100644
--- a/drivers/gpu/drm/panfrost/panfrost_device.h
+++ b/drivers/gpu/drm/panfrost/panfrost_device.h
@@ -70,6 +70,9 @@ struct panfrost_compatible {
 	int num_pm_domains;
 	/* Only required if num_pm_domains > 1. */
 	const char * const *pm_domain_names;
+
+	/* IOMMU quirks flags */
+	unsigned long pgtbl_quirks;
 };
 
 struct panfrost_device {
diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
index e8f7b11352d2..55a846c70e46 100644
--- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
+++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
@@ -368,6 +368,7 @@ int panfrost_mmu_pgtable_alloc(struct panfrost_file_priv *priv)
 	mmu->as = -1;
 
 	mmu->pgtbl_cfg = (struct io_pgtable_cfg) {
+		.quirks = pfdev->comp ? pfdev->comp->pgtbl_quirks : 0,
 		.pgsize_bitmap	= SZ_4K | SZ_2M,
 		.ias		= FIELD_GET(0xff, pfdev->features.mmu_features),
 		.oas		= FIELD_GET(0xff00, pfdev->features.mmu_features),
-- 
2.22.0

Re: [PATCH 2/5] drm/panfrost: add support specifying pgtbl quirks

[Steven Price]

On 08/09/2020 16:18, Neil Armstrong wrote:
> Add a pgtbl_quirks entry in the compatible specific table to permit specyfying IOMMU
> quirks for platforms.
> 
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>

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

> ---
>   drivers/gpu/drm/panfrost/panfrost_device.h | 3 +++
>   drivers/gpu/drm/panfrost/panfrost_mmu.c    | 1 +
>   2 files changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h
> index 953f7536a773..2cf1a6a13af8 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_device.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_device.h
> @@ -70,6 +70,9 @@ struct panfrost_compatible {
>   	int num_pm_domains;
>   	/* Only required if num_pm_domains > 1. */
>   	const char * const *pm_domain_names;
> +
> +	/* IOMMU quirks flags */
> +	unsigned long pgtbl_quirks;
>   };
>   
>   struct panfrost_device {
> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> index e8f7b11352d2..55a846c70e46 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> @@ -368,6 +368,7 @@ int panfrost_mmu_pgtable_alloc(struct panfrost_file_priv *priv)
>   	mmu->as = -1;
>   
>   	mmu->pgtbl_cfg = (struct io_pgtable_cfg) {
> +		.quirks = pfdev->comp ? pfdev->comp->pgtbl_quirks : 0,
>   		.pgsize_bitmap	= SZ_4K | SZ_2M,
>   		.ias		= FIELD_GET(0xff, pfdev->features.mmu_features),
>   		.oas		= FIELD_GET(0xff00, pfdev->features.mmu_features),
> 

[PATCH 3/5] drm/panfrost: add support for reset quirk

[Neil Armstrong]

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.

This adds a callback in the device compatible struct of permit this.

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 drivers/gpu/drm/panfrost/panfrost_device.h | 3 +++
 drivers/gpu/drm/panfrost/panfrost_gpu.c    | 4 ++++
 2 files changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h
index 2cf1a6a13af8..4c9cd5452ba5 100644
--- a/drivers/gpu/drm/panfrost/panfrost_device.h
+++ b/drivers/gpu/drm/panfrost/panfrost_device.h
@@ -73,6 +73,9 @@ struct panfrost_compatible {
 
 	/* IOMMU quirks flags */
 	unsigned long pgtbl_quirks;
+
+	/* Vendor implementation quirks at reset time callback */
+	void (*vendor_reset_quirk)(struct panfrost_device *pfdev);
 };
 
 struct panfrost_device {
diff --git a/drivers/gpu/drm/panfrost/panfrost_gpu.c b/drivers/gpu/drm/panfrost/panfrost_gpu.c
index e0f190e43813..c129aaf77790 100644
--- a/drivers/gpu/drm/panfrost/panfrost_gpu.c
+++ b/drivers/gpu/drm/panfrost/panfrost_gpu.c
@@ -62,6 +62,10 @@ int panfrost_gpu_soft_reset(struct panfrost_device *pfdev)
 	gpu_write(pfdev, GPU_INT_CLEAR, GPU_IRQ_RESET_COMPLETED);
 	gpu_write(pfdev, GPU_CMD, GPU_CMD_SOFT_RESET);
 
+	/* The Amlogic GPU integration needs quirks at this stage */
+	if (pfdev->comp->vendor_reset_quirk)
+		pfdev->comp->vendor_reset_quirk(pfdev);
+
 	ret = readl_relaxed_poll_timeout(pfdev->iomem + GPU_INT_RAWSTAT,
 		val, val & GPU_IRQ_RESET_COMPLETED, 100, 10000);
 
-- 
2.22.0

Re: [PATCH 3/5] drm/panfrost: add support for reset quirk

[Steven Price]

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.
> 
> This adds a callback in the device compatible struct of permit this.
> 
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
>   drivers/gpu/drm/panfrost/panfrost_device.h | 3 +++
>   drivers/gpu/drm/panfrost/panfrost_gpu.c    | 4 ++++
>   2 files changed, 7 insertions(+)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h
> index 2cf1a6a13af8..4c9cd5452ba5 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_device.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_device.h
> @@ -73,6 +73,9 @@ struct panfrost_compatible {
>   
>   	/* IOMMU quirks flags */
>   	unsigned long pgtbl_quirks;
> +
> +	/* Vendor implementation quirks at reset time callback */
> +	void (*vendor_reset_quirk)(struct panfrost_device *pfdev);
>   };
>   
>   struct panfrost_device {
> diff --git a/drivers/gpu/drm/panfrost/panfrost_gpu.c b/drivers/gpu/drm/panfrost/panfrost_gpu.c
> index e0f190e43813..c129aaf77790 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_gpu.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_gpu.c
> @@ -62,6 +62,10 @@ int panfrost_gpu_soft_reset(struct panfrost_device *pfdev)
>   	gpu_write(pfdev, GPU_INT_CLEAR, GPU_IRQ_RESET_COMPLETED);
>   	gpu_write(pfdev, GPU_CMD, GPU_CMD_SOFT_RESET);
>   
> +	/* The Amlogic GPU integration needs quirks at this stage */
> +	if (pfdev->comp->vendor_reset_quirk)
> +		pfdev->comp->vendor_reset_quirk(pfdev);
> +
>   	ret = readl_relaxed_poll_timeout(pfdev->iomem + GPU_INT_RAWSTAT,
>   		val, val & GPU_IRQ_RESET_COMPLETED, 100, 10000);

Placing the quirk before the reset has completed is dodgy. Can this be 
ordered after the GPU_IRQ_RESET_COMPLETED signal has been seen? The 
problem is the reset could (in theory) cause a power transition (e.g. if 
the GPU is reset while a core is powered) and changing the PWR_OVERRIDEx 
registers during a transition is undefined. But I don't know the details 
of how the hardware is broken so it is possible the override is needed 
for the reset to complete so this would need testing.

I also wonder if this could live in panfrost_gpu_init_quirks() instead? 
Although that is mostly about quirks common to all Mali GPU 
implementations rather than a specific implementation. Although now I've 
looked I've noticed we have a bug as we don't appear to reapply those 
quirks after a reset - I'll send a patch!

Steve

Re: [PATCH 3/5] drm/panfrost: add support for reset quirk

[Neil Armstrong]

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.
>>
>> This adds a callback in the device compatible struct of permit this.
>>
>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>> ---
>>   drivers/gpu/drm/panfrost/panfrost_device.h | 3 +++
>>   drivers/gpu/drm/panfrost/panfrost_gpu.c    | 4 ++++
>>   2 files changed, 7 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h
>> index 2cf1a6a13af8..4c9cd5452ba5 100644
>> --- a/drivers/gpu/drm/panfrost/panfrost_device.h
>> +++ b/drivers/gpu/drm/panfrost/panfrost_device.h
>> @@ -73,6 +73,9 @@ struct panfrost_compatible {
>>         /* IOMMU quirks flags */
>>       unsigned long pgtbl_quirks;
>> +
>> +    /* Vendor implementation quirks at reset time callback */
>> +    void (*vendor_reset_quirk)(struct panfrost_device *pfdev);
>>   };
>>     struct panfrost_device {
>> diff --git a/drivers/gpu/drm/panfrost/panfrost_gpu.c b/drivers/gpu/drm/panfrost/panfrost_gpu.c
>> index e0f190e43813..c129aaf77790 100644
>> --- a/drivers/gpu/drm/panfrost/panfrost_gpu.c
>> +++ b/drivers/gpu/drm/panfrost/panfrost_gpu.c
>> @@ -62,6 +62,10 @@ int panfrost_gpu_soft_reset(struct panfrost_device *pfdev)
>>       gpu_write(pfdev, GPU_INT_CLEAR, GPU_IRQ_RESET_COMPLETED);
>>       gpu_write(pfdev, GPU_CMD, GPU_CMD_SOFT_RESET);
>>   +    /* The Amlogic GPU integration needs quirks at this stage */
>> +    if (pfdev->comp->vendor_reset_quirk)
>> +        pfdev->comp->vendor_reset_quirk(pfdev);
>> +
>>       ret = readl_relaxed_poll_timeout(pfdev->iomem + GPU_INT_RAWSTAT,
>>           val, val & GPU_IRQ_RESET_COMPLETED, 100, 10000);
> 
> Placing the quirk before the reset has completed is dodgy. Can this be ordered after the GPU_IRQ_RESET_COMPLETED signal has been seen? The problem is the reset could (in theory) cause a power transition (e.g. if the GPU is reset while a core is powered) and changing the PWR_OVERRIDEx registers during a transition is undefined. But I don't know the details of how the hardware is broken so it is possible the override is needed for the reset to complete so this would need testing.
> 
> I also wonder if this could live in panfrost_gpu_init_quirks() instead? Although that is mostly about quirks common to all Mali GPU implementations rather than a specific implementation. Although now I've looked I've noticed we have a bug as we don't appear to reapply those quirks after a reset - I'll send a patch!

Indeed, it needs to be applied after each reset, so if you send a patch for this pretty sure it could live in panfrost_gpu_init_quirks().

Neil

> 
> Steve

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

[Neil Armstrong]

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);
+	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
-- 
2.22.0

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

[Alyssa Rosenzweig]

[-- Attachment #1: Type: text/plain, Size: 763 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Steven Price]

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
> 

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

[Neil Armstrong]

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
>>
> 

[PATCH 5/5] drm/panfrost: add Amlogic GPU integration quirks

[Neil Armstrong]

This adds the required GPU quirks, including the quirk in the PWR registers at the GPU
reset time and the IOMMU quirk for shareability issues observed on G52 in Amlogic G12B SoCs.

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 drivers/gpu/drm/panfrost/panfrost_drv.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
index 36463c89e966..efde5e2acc35 100644
--- a/drivers/gpu/drm/panfrost/panfrost_drv.c
+++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
@@ -656,7 +656,25 @@ static const struct panfrost_compatible default_data = {
 	.pm_domain_names = NULL,
 };
 
+static const struct panfrost_compatible amlogic_gxm_data = {
+	.num_supplies = ARRAY_SIZE(default_supplies),
+	.supply_names = default_supplies,
+	.vendor_reset_quirk = panfrost_gpu_amlogic_reset_quirk,
+};
+
+static const struct panfrost_compatible amlogic_g12a_data = {
+	.num_supplies = ARRAY_SIZE(default_supplies),
+	.supply_names = default_supplies,
+	.vendor_reset_quirk = panfrost_gpu_amlogic_reset_quirk,
+	.pgtbl_quirks = IO_PGTABLE_QUIRK_ARM_BROKEN_SH,
+};
+
 static const struct of_device_id dt_match[] = {
+	/* Set first to probe before the generic compatibles */
+	{ .compatible = "amlogic,meson-gxm-mali",
+	  .data = &amlogic_gxm_data, },
+	{ .compatible = "amlogic,meson-g12a-mali",
+	  .data = &amlogic_g12a_data, },
 	{ .compatible = "arm,mali-t604", .data = &default_data, },
 	{ .compatible = "arm,mali-t624", .data = &default_data, },
 	{ .compatible = "arm,mali-t628", .data = &default_data, },
-- 
2.22.0

Re: [PATCH 5/5] drm/panfrost: add Amlogic GPU integration quirks

[Alyssa Rosenzweig]

[-- Attachment #1: Type: text/plain, Size: 1799 bytes --]

Reviewed-by: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
> This adds the required GPU quirks, including the quirk in the PWR registers at the GPU
> reset time and the IOMMU quirk for shareability issues observed on G52 in Amlogic G12B SoCs.
> 
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
>  drivers/gpu/drm/panfrost/panfrost_drv.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> index 36463c89e966..efde5e2acc35 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> @@ -656,7 +656,25 @@ static const struct panfrost_compatible default_data = {
>  	.pm_domain_names = NULL,
>  };
>  
> +static const struct panfrost_compatible amlogic_gxm_data = {
> +	.num_supplies = ARRAY_SIZE(default_supplies),
> +	.supply_names = default_supplies,
> +	.vendor_reset_quirk = panfrost_gpu_amlogic_reset_quirk,
> +};
> +
> +static const struct panfrost_compatible amlogic_g12a_data = {
> +	.num_supplies = ARRAY_SIZE(default_supplies),
> +	.supply_names = default_supplies,
> +	.vendor_reset_quirk = panfrost_gpu_amlogic_reset_quirk,
> +	.pgtbl_quirks = IO_PGTABLE_QUIRK_ARM_BROKEN_SH,
> +};
> +
>  static const struct of_device_id dt_match[] = {
> +	/* Set first to probe before the generic compatibles */
> +	{ .compatible = "amlogic,meson-gxm-mali",
> +	  .data = &amlogic_gxm_data, },
> +	{ .compatible = "amlogic,meson-g12a-mali",
> +	  .data = &amlogic_g12a_data, },
>  	{ .compatible = "arm,mali-t604", .data = &default_data, },
>  	{ .compatible = "arm,mali-t624", .data = &default_data, },
>  	{ .compatible = "arm,mali-t628", .data = &default_data, },
> -- 
> 2.22.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]