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

[Krzysztof Kozlowski <krzk@kernel.org>]

On 30/01/2025 13:15, Louis-Alexis Eyraud wrote:
> Hello,
> 
> sorry for the delay,
> 
>  ---- On Sat, 18 Jan 2025 17:08:10 +0100  Krzysztof Kozlowski  wrote --- 
>  > On Thu, Jan 16, 2025 at 03:25:58PM +0100, Louis-Alexis Eyraud wrote:
>  > > This commit adds a compatible for the MediaTek MT8370 SoC, with an
>  > > integrated ARM Mali G57 MC2 GPU (Valhall-JM, dual core), and adds
>  > > platform data using the same supplies and the same power domain lists
>  > > as MT8186 (one regulator, two power domains).
>  > > 
>  > > Signed-off-by: Louis-Alexis Eyraud louisalexis.eyraud@collabora.com>
>  > > ---
>  > >  drivers/gpu/drm/panfrost/panfrost_drv.c | 10 ++++++++++
>  > >  1 file changed, 10 insertions(+)
>  > > 
>  > > diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
>  > > index 0f3935556ac761adcd80197d87e8e478df436fd5..1d51b64ed0f0660cc95263a289d5dad204540cfd 100644
>  > > --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
>  > > +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
>  > > @@ -837,6 +837,15 @@ static const struct panfrost_compatible mediatek_mt8192_data = {
>  > >      .pm_features = BIT(GPU_PM_CLK_DIS) | BIT(GPU_PM_VREG_OFF),
>  > >  };
>  > >  
>  > > +/* MT8370 uses the same power domains and power supplies as MT8186 */
>  > > +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),
>  > > +};
>  > 
>  > No, people, stop this nonsense. This is exactly the same as previous.
>  > Don't duplicate entries just because you want a commit.
>  > 
> I added this new compatible in bindings and panfrost driver because there were no other matching compatible 
> Using another mali-vallhal-jm compatible would make the driver probe fail because of power domains number difference. 
> Using mt8186-mali compatible would work without modifications but as it is not the same architecture (mali-bifrost), it would be incorrect.

Fix your email app, so it won't add spaces before quote and will wrap
the text properly.

> 
> I've also misguessed on the dt_match array modifications, sorry.
> I'll amend this patch in order to reuse the mt8186 platform data instead.
> 
>  > > +
>  > >  static const struct of_device_id dt_match[] = {
>  > >      /* Set first to probe before the generic compatibles */
>  > >      { .compatible = "amlogic,meson-gxm-mali",
>  > > @@ -859,6 +868,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 },
>  > 
>  > No, express properly compatibility or say in bindings commit msg why
>  > devices are not compatible.
>  > 
> I'll reword in V2 the commit messages to make the compatible need more explicit.

Your commit msg should then explain that this is not compatible with
mt8186 because programming model or architecture is different. Number of
power domains rarely matters for actual compatibility and as easily
visible in panfrost driver: does not matter here, either.

Best regards,
Krzysztof