| Message ID | 20240604123922.331469-3-angelogioacchino.delregno@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/panfrost: Add MT8188 support | expand |
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 > >
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 }, > {} > };
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 }, >> {} >> }; >
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 --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 }, {} };
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(+)