diff mbox series

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

Message ID 20240604123922.331469-3-angelogioacchino.delregno@collabora.com (mailing list archive)
State New, archived
Headers show
Series drm/panfrost: Add MT8188 support | expand

Commit Message

AngeloGioacchino Del Regno June 4, 2024, 12:39 p.m. UTC
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(+)

Comments

Chen-Yu Tsai June 5, 2024, 4:25 a.m. UTC | #1
On Tue, Jun 4, 2024 at 8:39 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> 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>

Reviewed-by: Chen-Yu Tsai <wenst@chromium.org>

> ---
>  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,
> +       .num_pm_domains = ARRAY_SIZE(mediatek_mt8183_pm_domains),
> +       .pm_domain_names = mediatek_mt8183_pm_domains,
> +       .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 },
>         {}
>  };
> --
> 2.45.1
>
>
Steven Price June 5, 2024, 9:18 a.m. UTC | #2
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.

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 },
>  	{}
>  };
AngeloGioacchino Del Regno June 5, 2024, 11:43 a.m. UTC | #3
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 },
>>   	{}
>>   };
>
Steven Price June 5, 2024, 12:39 p.m. UTC | #4
On 05/06/2024 12:43, AngeloGioacchino Del Regno wrote:
> 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 :-)

I was tempted to suggest something like it... ;) But two things made me
wary:

 * It's a bigger change, and I didn't want to suggest such a large
refactor - let's get this patch in first.

 * There is no technical reason why designs will have a power domain per
core. In particular there is the concept of 'stacks' added in later GPUs
(so for these GPUs it should be "stack%d"). We also don't want the
bindings being decided based on the driver, so if there's an integration
which has e.g. "core01", "core23", etc we should support that rather
than encouraging people to invent different names just to make the
bindings easier.

Having said that, we could have special handling for
pm_domain_names==NULL where "core%d" is assumed, leaving the current
approach for any 'weird' GPU integrations that might crop up in the future.

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

Agreed - there's no point holding up adding support for a new GPU for
this clean up.

> P.S.: Thanks for the feedback!

No problem.

Thanks,

Steve

> 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 },
>>>   	{}
>>>   };
>>
> 
> 
>
diff mbox series

Patch

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,
+	.num_pm_domains = ARRAY_SIZE(mediatek_mt8183_pm_domains),
+	.pm_domain_names = mediatek_mt8183_pm_domains,
+	.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 },
 	{}
 };