Re: [PATCH v7 4/5] remoteproc: ingenic: Added remoteproc driver

[Mathieu Poirier]

On Sun, Jun 21, 2020 at 12:30:22PM -0700, Bjorn Andersson wrote:
> On Fri 12 Jun 04:47 PDT 2020, Paul Cercueil wrote:
> > Le jeu. 11 juin 2020 à 19:21, Suman Anna <s-anna@xxxxxx> a écrit :
> > > On 6/11/20 5:21 PM, Paul Cercueil wrote:
> > > > Le jeu. 11 juin 2020 à 16:47, Suman Anna <s-anna@xxxxxx> a écrit :
> > > > > On 5/15/20 5:43 AM, Paul Cercueil wrote:
> [..]
> > > > > > diff --git a/drivers/remoteproc/ingenic_rproc.c
> > > > > > b/drivers/remoteproc/ingenic_rproc.c
> [..]
> > > > > > +    /* The clocks must be enabled for the firmware to be
> > > > > > loaded in TCSM */
> > > > > > +    ret = clk_bulk_prepare_enable(ARRAY_SIZE(vpu->clks),
> > > > > > vpu->clks);
> > > > > > +    if (ret) {
> > > > > > +        dev_err(dev, "Unable to start clocks\n");
> > > > > > +        return ret;
> > > > > > +    }
> > > > > 
> > > > > You are enabling the clocks directly here and also trying to
> > > > > manage them through pm_runtime callbacks again.
> > > > 
> > > > Yes. The clocks need to be enabled in the probe.
> > > 
> > > For the preferred non CONFIG_PM case now and lack of
> > > prepare/unprepare().
> > 
> > I want to make it clear that I'm not against having .prepare/.unprepare, but
> > I want to see what maintainers have to say.
> > 
> 
> I think it's perfectly reasonable to enable all the resources here and
> then if CONFIG_PM isn't set you just leave them enabled throughout.

In my opinion the best way to deal with the status of the CONFIG_PM
configuration option is to move clock relate operations to the prepare/unprepare
callbacks.  Doing so would have several advantages:

1) If rproc->auto_boot is false then clocks aren't enabled needlessly between
the time the driver is probed and the remote processor is started.

2) It would allow for the removal of the runtime PM calls in the core, which in
the current state, prevents the runtime PM mechanic to really serve its purpose.
Indeed, calling runtime PM operations in rproc_fw_boot() and rproc_shutdown()
prevents the remote processor from being suspended during periods of inactivity.
If all that is required for Ingenic's remote processor is to set the clocks
before accessing device memory and switch them off when no longer needed, I
think prepare() and unprepare() are the best choice.

3) As Suman pointed out having runtime PM operations in remoteproc core makes the
task of supporting scenarios where the remote processor is loaded by another
entity more complex.  Given the distinct nature of managing power for remote
processors and the characteristics of each platform I beleive the best place to
call runtime PM operations is in the rproc operations, as it is done
omap_remoteproc.c

Since we are quite early in the release cycle all we need is a couple of small
patches: one to move clock manipulation in the Ingenic driver to the remoteproc
core's prepare/unprepare function and another one to remove runtime PM
operations from the core.

Thanks,
Mathieu

> 
> Regards,
> Bjorn