Re: [PATCH v2 2/2] drm/panfrost: Add support for Mali on the MT8188 SoC

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

Il 05/06/24 11:18, Steven Price ha scritto:
> On 04/06/2024 13:39, AngeloGioacchino Del Regno wrote:
>> MediaTek MT8188 has a Mali-G57 MC3 (Valhall-JM): add a new
>> compatible and platform data using the same supplies and the
>> same power domain lists as MT8183 (one regulator, three power
>> domains).
>>
>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>> ---
>>   drivers/gpu/drm/panfrost/panfrost_drv.c | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
>> index ef9f6c0716d5..4e2d9f671a0d 100644
>> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
>> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
>> @@ -777,6 +777,14 @@ static const struct panfrost_compatible mediatek_mt8186_data = {
>>   	.pm_features = BIT(GPU_PM_CLK_DIS) | BIT(GPU_PM_VREG_OFF),
>>   };
>>   
>> +static const struct panfrost_compatible mediatek_mt8188_data = {
>> +	.num_supplies = ARRAY_SIZE(mediatek_mt8183_b_supplies) - 1,
>> +	.supply_names = mediatek_mt8183_b_supplies,
> 
> I think this is a little confusing. Ideally we'd drop the existing
> mediatek_xxx_supplies which are the same as default_supplies and just
> use that instead.
> 
>> +	.num_pm_domains = ARRAY_SIZE(mediatek_mt8183_pm_domains),
>> +	.pm_domain_names = mediatek_mt8183_pm_domains,
> 
> We'd want at least a comment explaining that this isn't a typo (i.e. /*
> mt8188 uses the same pm domains as mt8183 */). But I'm also wondering if
> it would be sensible to simply have one domain list, something like:
> 
> --->8---
> static const char * const mediatek_pm_domains[] = { "core0", "core1",
> 						    "core2", "core3",
> 						    "core4" };
> 
> static const struct panfrost_compatible mediatek_mt8183_data = {
> 	...
> 	.num_pm_domains = 3,
> 	.pm_domain_names = mediatek_pm_domains,
> };
> ...
> static const struct panfrost_compatible mediatek_mt8186_data = {
> 	...
> 	.num_pm_domains = 2,
> 	.pm_domain_names = mediatek_pm_domains,
> };
> ...
> static const struct panfrost_compatible mediatek_mt8188_data = {
> 	...
> 	.num_pm_domains = 3,
> 	.pm_domain_names = mediatek_pm_domains,
> };
> ...
> static const struct panfrost_compatible mediatek_mt8192_data = {
> 	...
> 	.num_pm_domains = 5,
> 	.pm_domain_names = mediatek_pm_domains,
> };
> --->8---
> 
> OTOH what you've got it no worse than what we already had, so it's up to
> you whether you want to tidy this up or just add a comment so it doesn't
> look like there's a typo.
> 

I didn't disclose my plan, but you've already shown part of it, so seeing that
you preventively agree with at least part of that is fun :-)

I surely won't be able to do what I want to do for *this* cycle as I'm mostly
sure that I won't have time for this in the next 3 weeks - but anyway....

What I was thinking is that we should either look for a number of power domains
limited by a max power domains definition (that should already be present somewhere
in panfrost if I recall correctly) without even caring about the actual power
domain names, or we should look for a number of PDs having any name matching,
in a for loop, snprintf(*something, sizeof(something), "core%d", i).

This means that, with the snprintf idea, we don't even have to set any
pm_domain_names list anymore, at all, and we can either reuse num_pm_domains
or just get the number of PDs limited by the binding - but that's a problem for
the future me/us I guess...

But since we're there...

Please, I'd like to hear your opinion about the core%d idea :-)

Anyway, I think that for now I'm choosing the "comment shortcut" for this patch.

P.S.: Thanks for the feedback!

Cheers,
Angelo

> Steve
> 
>> +	.pm_features = BIT(GPU_PM_CLK_DIS) | BIT(GPU_PM_VREG_OFF),
>> +};
>> +
>>   static const char * const mediatek_mt8192_supplies[] = { "mali", NULL };
>>   static const char * const mediatek_mt8192_pm_domains[] = { "core0", "core1", "core2",
>>   							   "core3", "core4" };
>> @@ -808,6 +816,7 @@ static const struct of_device_id dt_match[] = {
>>   	{ .compatible = "mediatek,mt8183-mali", .data = &mediatek_mt8183_data },
>>   	{ .compatible = "mediatek,mt8183b-mali", .data = &mediatek_mt8183_b_data },
>>   	{ .compatible = "mediatek,mt8186-mali", .data = &mediatek_mt8186_data },
>> +	{ .compatible = "mediatek,mt8188-mali", .data = &mediatek_mt8188_data },
>>   	{ .compatible = "mediatek,mt8192-mali", .data = &mediatek_mt8192_data },
>>   	{}
>>   };
>