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;

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

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

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

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