Re: [PATCH v2 4/6] pmdomain: renesas: Add R-Car X5H MDLC driver

[Ulf Hansson]

On Wed, Jul 15, 2026 at 11:11 AM Geert Uytterhoeven
<geert@xxxxxxxxxxxxxx> wrote:
>
> Hi Ulf,
>
> On Tue, 14 Jul 2026 at 14:04, Ulf Hansson <ulf.hansson@xxxxxxxxxxxxxxxx> wrote:
> > On Wed, Jul 8, 2026 at 12:15 PM Geert Uytterhoeven
> > <geert+renesas@xxxxxxxxx> wrote:
> > >
> > > Add a minimal Module Controller driver for the R-Car X5H (R8A78000) SoC.
> > > For now this just supports the always-on power domains, and dummy clocks
> > > and resets for the serial console (which is enabled by the boot loader).
> > >
> > > Signed-off-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
>
> > > --- /dev/null
> > > +++ b/drivers/pmdomain/renesas/r8a78000-mdlc.c
>
> > > +static struct generic_pm_domain *r8a78000_genpd_xlate(
> > > +                       const struct of_phandle_args *spec, void *data)
> > > +{
> > > +       struct r8a78000_mdlc_priv *priv = container_of(data,
> > > +                                       struct r8a78000_mdlc_priv, genpd_data);
> > > +       struct device *dev = priv->dev;
> > > +       u32 id;
> > > +
> > > +       if (spec->args_count != 2)
> > > +               return ERR_PTR(-EINVAL);
> > > +
> > > +       id = spec->args[0];
> > > +
> > > +       if (id >= R8A78000_MDLC_PD_AON) {
> > > +               dev_dbg(dev,
> > > +                       "Mapping HW power domain 0x%x to always-on domain\n",
> > > +                       id);
> > > +               return r8a78000_genpd_always_on;
> >
> > This looks odd, but perhaps it's just a temporary mapping that you
> > intend to change later, no?
>
> The hardware has multiple always-on domains, but on the software side,
> using only one is sufficient.  When needed, it can be changed later.
> Other domains will be added later, in the else branch.

I see, perhaps add a comment about this in the code, to make this more clear?

>
> > > +       }
> > > +
> > > +       dev_err(dev, "Unknown power domain 0x%x\n", id);
> > > +       return ERR_PTR(-ENOENT);
> > > +}
>
> > > +static int r8a78000_genpd_always_on_singleton(struct device *dev)
> > > +{
> > > +       struct generic_pm_domain *genpd;
> > > +       int ret;
> > > +
> > > +       guard(mutex)(&r8a78000_mdlc_lock);
> > > +
> > > +       if (r8a78000_genpd_always_on)
> > > +               return 0;
> > > +
> >
> > I guess the mutex is used to protect the global
> > r8a78000_genpd_always_on, but it looks like that isn't really needed
> > based upon how things are being called during the probe.
>
> Why would it not be needed? There are multiple instances of the MDLC
> device, but only one instance of the always-on domain.

Aha, I see. Thanks for clarifying!

>
> > > +       genpd = kzalloc_obj(*genpd);
> > > +       if (!genpd)
> > > +               return -ENOMEM;
> > > +
> > > +       genpd->name = "always-on";
> > > +       genpd->attach_dev = r8a78000_mdlc_attach_dev;
> > > +
> > > +       ret = pm_genpd_init(genpd, &pm_domain_always_on_gov, false);
> > > +       if (ret) {
> > > +               kfree(genpd);
> > > +               return dev_err_probe(dev, ret,
> > > +                                    "Failed to create always-on domain\n");
> > > +       }
> > > +
> > > +       r8a78000_genpd_always_on = genpd;
> > > +       return 0;
> > > +}
> > > +
> > > +static int r8a78000_mdlc_probe(struct platform_device *pdev)
> > > +{
> > > +       struct device *dev = &pdev->dev;
> > > +       struct device_node *np = dev->of_node;
> > > +       struct r8a78000_mdlc_priv *priv;
> > > +       const struct mdlc_info *info;
> > > +       struct resource *res;
> > > +       int ret;
> > > +
> > > +       ret = r8a78000_genpd_always_on_singleton(dev);
> > > +       if (ret)
> > > +               return ret;
> > > +
> > > +       info = of_device_get_match_data(dev);
> > > +       if (!info)
> > > +               return -ENODEV;
> > > +
> > > +       priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > > +       if (!priv)
> > > +               return -ENOMEM;
> > > +
> > > +       priv->dev = dev;
> > > +       priv->np = np;
> > > +
> > > +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > +       if (!res)
> > > +               return -ENODEV;
> > > +
> > > +       for (; info->base; info++) {
> > > +               if (info->base == res->start)
> > > +                       break;
> > > +       }
> > > +
> > > +       if (!info->base) {
> > > +               dev_dbg(dev, "Unsupported MDLC instance 0x%pa\n", &res->start);
> > > +               return -ENODEV;
> > > +       }
> > > +
> > > +       priv->mod_map = info->mod_map;
> > > +
> > > +       scoped_guard(mutex, &r8a78000_mdlc_lock) {
> > > +               hlist_add_head(&priv->link, &r8a78000_mdlc_list);
> > > +       }
> > > +
> > > +       ret = devm_add_action_or_reset(dev, r8a78000_mdlc_unlink, priv);
> > > +       if (ret)
> > > +               return dev_err_probe(dev, ret, "failed to add action\n");
> > > +
> > > +       /* Note that no actual domains are registered, just need translation */
> > > +       priv->genpd_data.xlate = r8a78000_genpd_xlate;
> > > +       ret = of_genpd_add_provider_onecell(np, &priv->genpd_data);
> > > +       if (ret)
> > > +               return dev_err_probe(dev, ret,
> > > +                                    "Failed to register genpd provider\n");
> > > +
> > > +       ret = devm_add_action_or_reset(dev, r8a78000_genpd_del_provider, np);
> > > +       if (ret)
> > > +               return dev_err_probe(dev, ret,
> > > +                                    "failed to add unregister action\n");
> > > +
> > > +       priv->rcdev.ops = &r8a78000_mdlc_reset_ops;
> > > +       priv->rcdev.of_node = np;
> > > +       priv->rcdev.of_reset_n_cells = 1;
> > > +       priv->rcdev.of_xlate = r8a78000_mdlc_reset_xlate;
> > > +
> > > +       ret = devm_reset_controller_register(dev, &priv->rcdev);
> > > +       if (ret)
> > > +               return dev_err_probe(dev, ret,
> > > +                                    "Failed to register reset controller\n");
> > > +
> >
> > In some of the error paths above it looks like we end up leaking the
> > data allocated for r8a78000_genpd_always_on. Perhaps clean up that
> > somewhere here.
>
> That is intentional: the always-on domain is shared by all MDLC
> instances.  If one MDLC fails to probe, the MDLC instances that did
> succeed still need the always-on domain.

Okay, so perhaps we should add a reference counting of it, to make
sure we don't leak the data?

>
> >
> > > +       return 0;
> > > +}
> >
> > [...]
> >
> > In regards to the merge strategy, I intend to pick patch 2 (shared via
> > the immutable dt branch) and $subject patch. Please let me know if
> > there are any issues with that.
>
> That is fine, thanks!

Great, thanks!

Kind regards
Uffe