Changes in v2:
- Fixed bindings to restrict number of power domains for MT8188's
GPU to three like MT8183(b).
This series adds support for MT8188's Mali-G57 MC3.
AngeloGioacchino Del Regno (2):
dt-bindings: gpu: mali-bifrost: Add compatible for MT8188 SoC
drm/panfrost: Add support for Mali on the MT8188 SoC
.../devicetree/bindings/gpu/arm,mali-bifrost.yaml | 5 ++++-
drivers/gpu/drm/panfrost/panfrost_drv.c | 9 +++++++++
2 files changed, 13 insertions(+), 1 deletion(-)
--
2.45.1
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 <[email protected]>
---
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
Add a compatible for the MediaTek MT8188 SoC, with an integrated
ARM Mali G57 MC3 (Valhall-JM) GPU.
Signed-off-by: AngeloGioacchino Del Regno <[email protected]>
---
Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml b/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml
index e796a1ff8c82..8acb46adabe2 100644
--- a/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml
+++ b/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml
@@ -34,6 +34,7 @@ properties:
- const: arm,mali-valhall-jm # Mali Valhall GPU model/revision is fully discoverable
- items:
- enum:
+ - mediatek,mt8188-mali
- mediatek,mt8192-mali
- const: arm,mali-valhall-jm # Mali Valhall GPU model/revision is fully discoverable
@@ -195,7 +196,9 @@ allOf:
properties:
compatible:
contains:
- const: mediatek,mt8183b-mali
+ enum:
+ - mediatek,mt8183b-mali
+ - mediatek,mt8188-mali
then:
properties:
power-domains:
--
2.45.1
On Tue, Jun 04, 2024 at 02:39:21PM +0200, AngeloGioacchino Del Regno wrote:
> Add a compatible for the MediaTek MT8188 SoC, with an integrated
> ARM Mali G57 MC3 (Valhall-JM) GPU.
>
> Signed-off-by: AngeloGioacchino Del Regno <[email protected]>
Acked-by: Conor Dooley <[email protected]>
Thanks,
Conor.
On Tue, Jun 4, 2024 at 8:39 PM AngeloGioacchino Del Regno
<[email protected]> wrote:
>
> Add a compatible for the MediaTek MT8188 SoC, with an integrated
> ARM Mali G57 MC3 (Valhall-JM) GPU.
>
> Signed-off-by: AngeloGioacchino Del Regno <[email protected]>
Reviewed-by: Chen-Yu Tsai <[email protected]>
> ---
> Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml b/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml
> index e796a1ff8c82..8acb46adabe2 100644
> --- a/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml
> +++ b/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml
> @@ -34,6 +34,7 @@ properties:
> - const: arm,mali-valhall-jm # Mali Valhall GPU model/revision is fully discoverable
> - items:
> - enum:
> + - mediatek,mt8188-mali
> - mediatek,mt8192-mali
> - const: arm,mali-valhall-jm # Mali Valhall GPU model/revision is fully discoverable
>
> @@ -195,7 +196,9 @@ allOf:
> properties:
> compatible:
> contains:
> - const: mediatek,mt8183b-mali
> + enum:
> + - mediatek,mt8183b-mali
> + - mediatek,mt8188-mali
> then:
> properties:
> power-domains:
> --
> 2.45.1
>
>
On Tue, Jun 4, 2024 at 8:39 PM AngeloGioacchino Del Regno
<[email protected]> 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 <[email protected]>
Reviewed-by: Chen-Yu Tsai <[email protected]>
> ---
> 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
>
>
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 <[email protected]>
> ---
> 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 },
> {}
> };
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 <[email protected]>
>> ---
>> 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 },
>> {}
>> };
>
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 <[email protected]>
>>> ---
>>> 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 },
>>> {}
>>> };
>>
>
>
>