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
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 <[email protected]>
---
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
Add a pgtbl_quirks entry in the compatible specific table to permit specyfying IOMMU
quirks for platforms.
Signed-off-by: Neil Armstrong <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
>
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> ---
> 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;
>
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 <[email protected]>
>> ---
>> 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;
>>
>
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 <[email protected]>
>> ---
>> 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
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 <[email protected]>
>> ---
>> 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
>>
>
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 <[email protected]>
Reviewed-by: Steven Price <[email protected]>
> ---
> 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),
>