Re: [PATCH v5 2/3] drm/panfrost: Add support for Mali on the MT8370 SoC

[Louis-Alexis Eyraud <louisalexis.eyraud@collabora.com>]

Hi,

On Wed, 2025-05-07 at 15:06 +0100, Steven Price wrote:
> On 02/05/2025 13:17, Louis-Alexis Eyraud wrote:
> > Add a compatible for the MediaTek MT8370 SoC, with an integrated
> > ARM
> > Mali G57 MC2 GPU (Valhall-JM, dual core), with new platform data
> > for
> > its support in the panfrost driver.
> > It uses the same data as MT8186 for the power management features
> > to
> > describe power supplies, pm_domains and enablement (one regulator,
> > two
> > power domains) but also sets the FORCE_AARCH64_PGTABLE flag in the
> > GPU
> > configuration quirks bitfield to enable AARCH64 4K page table
> > format
> > mode.
> > As MT8186 and MT8370 SoC have different GPU architecture (Mali G52
> > 2EE
> > MC2 for MT8186), making them not compatible, and this mode is only
> > enabled for Mediatek SoC that are Mali G57 based (compatible with
> > mediatek,mali-mt8188 or mediatek,mali-8192), having specific
> > platform
> > data allows to set this flag for MT8370 without modifying MT8186
> > configuration and behaviour.
> > 
> > Signed-off-by: Louis-Alexis Eyraud
> > <louisalexis.eyraud@collabora.com>
> 
> With one minor comment fixed below:
> 
> Reviewed-by: Steven Price <steven.price@arm.com>
> 
> > ---
> >  drivers/gpu/drm/panfrost/panfrost_drv.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c
> > b/drivers/gpu/drm/panfrost/panfrost_drv.c
> > index
> > f1ec3b02f15a0029d20c7d81046ded59854e885c..8e0a1ae6940c73b7b60233950
> > ae3abdfa843cc8e 100644
> > --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> > +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> > @@ -846,6 +846,16 @@ static const struct panfrost_compatible
> > mediatek_mt8192_data = {
> >  	.gpu_quirks = BIT(GPU_QUIRK_FORCE_AARCH64_PGTABLE),
> >  };
> >  
> > +/* MT8370 uses the same power domains and power supplies as MT8186
> > */
> 
> This comment is not correct - you've got the power domains of MT8186,
> but the supplies of MT8183. The comment doesn't actually add much so
> you 
> could just drop it.
> 
I'll remove it.

> If you're feeling adventurous then one option here is to actually
> clean 
> up the mediatek entries a little. Instead of referring to particular 
> part numbers we could have:
> 
> static const char * const mediatek_2_pm_domains[] = { "core0",
> "core1" };
> static const char * const mediatek_3_pm_domains[] = { "core0",
> "core1", "core2" };
> static const char * const mediatek_5_pm_domains[] = { "core0",
> "core1", "core2",
>                                                       "core3",
> "core4" };
> 
> Or even just have the mediatek_5_domains[] array (dropping the '5' in
> the name) and not use ARRAY_SIZE().
> 
> Equally the supplies arrays could be renamed. We have the one with 
> "sram" for legacy and everything else uses {"mali", NULL} but we have
> two definitions for it (mt8183_b and mt8192).
> 
> Thanks,
> Steve
I'll do the array cleaning as well in a separate patch in the v6.

Regards,
Louis-Alexis
> 
> > +static const struct panfrost_compatible mediatek_mt8370_data = {
> > +	.num_supplies = ARRAY_SIZE(mediatek_mt8183_b_supplies) -
> > 1,
> > +	.supply_names = mediatek_mt8183_b_supplies,
> > +	.num_pm_domains = ARRAY_SIZE(mediatek_mt8186_pm_domains),
> > +	.pm_domain_names = mediatek_mt8186_pm_domains,
> > +	.pm_features = BIT(GPU_PM_CLK_DIS) | BIT(GPU_PM_VREG_OFF),
> > +	.gpu_quirks = BIT(GPU_QUIRK_FORCE_AARCH64_PGTABLE),
> > +};
> > +
> >  static const struct of_device_id dt_match[] = {
> >  	/* Set first to probe before the generic compatibles */
> >  	{ .compatible = "amlogic,meson-gxm-mali",
> > @@ -868,6 +878,7 @@ static const struct of_device_id dt_match[] = {
> >  	{ .compatible = "mediatek,mt8186-mali", .data =
> > &mediatek_mt8186_data },
> >  	{ .compatible = "mediatek,mt8188-mali", .data =
> > &mediatek_mt8188_data },
> >  	{ .compatible = "mediatek,mt8192-mali", .data =
> > &mediatek_mt8192_data },
> > +	{ .compatible = "mediatek,mt8370-mali", .data =
> > &mediatek_mt8370_data },
> >  	{ .compatible = "allwinner,sun50i-h616-mali", .data =
> > &allwinner_h616_data },
> >  	{}
> >  };
> >