Re: [RFC PATCH 3/3] remoteproc: Add Renesas rcar driver

[Geert Uytterhoeven <geert@linux-m68k.org>]

On Mon, Nov 8, 2021 at 7:42 PM Mathieu Poirier
<mathieu.poirier@linaro.org> wrote:
> On Wed, Oct 27, 2021 at 09:30:20AM +0200, Julien Massot wrote:
> > Renesas Gen3 platform includes a Cortex-r7 processor.
> >
> > Both: the application cores (A5x) and the realtime core (CR7)
> > share access to the RAM and devices with the same address map,
> > so device addresses are equal to the Linux physical addresses.
> >
> > In order to initialize this remote processor we need to:
> > - power on the realtime core
> > - put the firmware in a ram area
> > - set the boot address for this firmware (reset vector)
> > - Deassert the reset
> >
> > This initial driver allows to start and stop the Cortex R7
> > processor.
> >
> > Signed-off-by: Julien Massot <julien.massot@iot.bzh>

> > --- a/drivers/remoteproc/Kconfig
> > +++ b/drivers/remoteproc/Kconfig
> > @@ -261,6 +261,17 @@ config QCOM_WCNSS_PIL
> >         verified and booted with the help of the Peripheral Authentication
> >         System (PAS) in TrustZone.
> >
> > +config RCAR_REMOTEPROC
> > +     tristate "Renesas RCAR remoteproc support"
>
> It is probably a good idea to include the type of SoC being supported, something
> like:
>
>         tristate "Renesas Gen3 RCAR remoteproc support"

R-Car Gen3 please

> That will make it easier to support future RCAR processors that may not share
> the same architecture.

> > --- /dev/null
> > +++ b/drivers/remoteproc/rcar_rproc.c

> > +static int rcar_rproc_probe(struct platform_device *pdev)
> > +{
> > +     struct device *dev = &pdev->dev;
> > +     struct device_node *np = dev->of_node;
> > +     struct rcar_rproc *priv;
> > +     struct rproc *rproc;
> > +     int ret;
> > +
> > +     rproc = rproc_alloc(dev, np->name, &rcar_rproc_ops,
> > +                         NULL, sizeof(*priv));
> > +     if (!rproc)
> > +             return -ENOMEM;
> > +
> > +     priv = rproc->priv;
> > +     priv->rproc = rproc;
>
> I don't see rcar_rproc::rproc being used anywhere.
>
> > +     priv->dev = dev;
> > +
> > +     priv->rst = devm_reset_control_get_exclusive(&pdev->dev, NULL);
> > +     if (IS_ERR(priv->rst)) {
> > +             ret = PTR_ERR(priv->rst);
> > +             dev_err(dev, "fail to acquire rproc reset\n");
> > +             goto free_rproc;
> > +     }
> > +
> > +     pm_runtime_enable(priv->dev);
> > +     ret = pm_runtime_get_sync(priv->dev);
>
> There is no dev_pm_ops for the platform driver nor clocks to manage for this
> device - is there something that requires pm_runtime operations to be called?

Given

    cr7_rproc: cr7 {
        compatible = "renesas,rcar-cr7";
        memory-region = <&cr7_ram>;
        power-domains = <&sysc R8A7795_PD_CR7>;
        resets = <&cpg 222>;
        status = "okay";
    };

the pm_runtime_get_sync() is intended to power the CR7 power domain,
right?

However, I have my doubt about the (bindings for) that node, as it
does not represent the hardware.  Shouldn't the Cortex R7 have its
own CPU node instead, with an appropriate enable-method?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds