Re: [PATCH v6 7/7] pmdomain: mediatek: Add support for MFlexGraphics

[Nicolas Frattaroli <nicolas.frattaroli@collabora.com>]

On Monday, 6 October 2025 12:58:55 Central European Summer Time Nicolas Frattaroli wrote:
> On Friday, 3 October 2025 23:41:16 Central European Summer Time Chia-I Wu wrote:
> > On Fri, Oct 3, 2025 at 1:16 PM Nicolas Frattaroli
> > <nicolas.frattaroli@collabora.com> wrote:
> > >
> > > Various MediaTek SoCs use GPU integration silicon named "MFlexGraphics"
> > > by MediaTek. On the MT8196 and MT6991 SoCs, interacting with this
> > > integration silicon is required to power on the GPU.
> > >
> > > This glue silicon is in the form of an embedded microcontroller running
> > > special-purpose firmware, which autonomously adjusts clocks and
> > > regulators.
> > >
> > > Implement a driver, modelled as a pmdomain driver with a
> > > set_performance_state operation, to support these SoCs.
> [...]
> > > +static int mtk_mfg_probe(struct platform_device *pdev)
> > > +{
> > > +       struct device_node *shmem __free(device_node);
> > > +       struct mtk_mfg *mfg;
> > > +       struct device *dev = &pdev->dev;
> > > +       const struct mtk_mfg_variant *data = of_device_get_match_data(dev);
> > > +       struct resource res;
> > > +       int ret, i;
> > > +
> > > +       mfg = devm_kzalloc(dev, sizeof(*mfg), GFP_KERNEL);
> > > +       if (!mfg)
> > > +               return -ENOMEM;
> > > +
> > > +       mfg->pdev = pdev;
> > > +       mfg->variant = data;
> > > +
> > > +       dev_set_drvdata(dev, mfg);
> > > +
> > > +       mfg->gpr = devm_platform_ioremap_resource(pdev, 0);
> > > +       if (IS_ERR(mfg->gpr))
> > > +               return dev_err_probe(dev, PTR_ERR(mfg->gpr),
> > > +                                    "Couldn't retrieve GPR MMIO registers\n");
> > > +
> > > +       mfg->rpc = devm_platform_ioremap_resource(pdev, 1);
> > > +       if (IS_ERR(mfg->rpc))
> > > +               return dev_err_probe(dev, PTR_ERR(mfg->rpc),
> > > +                                    "Couldn't retrieve RPC MMIO registers\n");
> > > +
> > > +       mfg->clk_eb = devm_clk_get(dev, "eb");
> > > +       if (IS_ERR(mfg->clk_eb))
> > > +               return dev_err_probe(dev, PTR_ERR(mfg->clk_eb),
> > > +                                    "Couldn't get 'eb' clock\n");
> > > +
> > > +       mfg->gpu_clks = devm_kcalloc(dev, data->num_clks, sizeof(*mfg->gpu_clks),
> > > +                                    GFP_KERNEL);
> > > +       if (!mfg->gpu_clks)
> > > +               return -ENOMEM;
> > > +
> > > +       for (i = 0; i < data->num_clks; i++)
> > > +               mfg->gpu_clks[i].id = data->clk_names[i];
> > > +
> > > +       ret = devm_clk_bulk_get(dev, data->num_clks, mfg->gpu_clks);
> > > +       if (ret)
> > > +               return dev_err_probe(dev, ret, "Couldn't get GPU clocks\n");
> > > +
> > > +       mfg->gpu_regs = devm_kcalloc(dev, data->num_regulators,
> > > +                                    sizeof(*mfg->gpu_regs), GFP_KERNEL);
> > > +       if (!mfg->gpu_regs)
> > > +               return -ENOMEM;
> > > +
> > > +       for (i = 0; i < data->num_regulators; i++)
> > > +               mfg->gpu_regs[i].supply = data->regulator_names[i];
> > > +
> > > +       ret = devm_regulator_bulk_get(dev, data->num_regulators, mfg->gpu_regs);
> > > +       if (ret)
> > > +               return dev_err_probe(dev, ret, "Couldn't get GPU regulators\n");
> > > +
> > > +       ret = of_reserved_mem_region_to_resource(dev->of_node, 0, &res);
> > > +       if (ret)
> > > +               return dev_err_probe(dev, ret, "Couldn't get GPUEB shared memory\n");
> > > +
> > > +       mfg->shared_mem = devm_ioremap(dev, res.start, resource_size(&res));
> > > +       if (!mfg->shared_mem)
> > > +               return dev_err_probe(dev, -ENOMEM, "Can't ioremap GPUEB shared memory\n");
> > > +       mfg->shared_mem_size = resource_size(&res);
> > > +       mfg->shared_mem_phys = res.start;
> > > +
> > > +       if (data->init) {
> > > +               ret = data->init(mfg);
> > > +               if (ret)
> > > +                       return dev_err_probe(dev, ret, "Variant init failed\n");
> > > +       }
> > > +
> > > +       mfg->pd.name = dev_name(dev);
> > > +       mfg->pd.attach_dev = mtk_mfg_attach_dev;
> > > +       mfg->pd.detach_dev = mtk_mfg_detach_dev;
> > > +       mfg->pd.power_off = mtk_mfg_power_off;
> > > +       mfg->pd.power_on = mtk_mfg_power_on;
> > > +       mfg->pd.set_performance_state = mtk_mfg_set_performance;
> > > +       mfg->pd.flags = GENPD_FLAG_OPP_TABLE_FW;
> > > +
> > > +       ret = pm_genpd_init(&mfg->pd, NULL, false);
> > > +       if (ret)
> > > +               return dev_err_probe(dev, ret, "Failed to initialise power domain\n");
> > We need to clean up mgf->md on errors from this point on. Maybe we can
> > move this to a later point?
> > 
> > > +
> > > +       ret = mtk_mfg_init_mbox(mfg);
> > > +       if (ret)
> > > +               return dev_err_probe(dev, ret, "Couldn't initialise mailbox\n");
> > We need to free the mboxes from this point on.
> > 
> 
> For this and the one above, does .remove not get called on probe failure? If not,
> I'll definitely do this. Otherwise it seems redundant, though with the added
> concern that .remove does not check before calling those functions.
> 

Alright nevermind, I'm being confused by devres vs. remove callback.

.remove does not get called if the probe function fails, but devres
handlers still do get called.

Sorry for the confusion, will fix things accordingly.