Re: [PATCH] remoteproc: q6v5: Add support to vote for rpmh power domains

[Rajendra Nayak]

Hi Bjorn,

On 8/6/2018 10:18 PM, Bjorn Andersson wrote:
> On Fri 29 Jun 03:20 PDT 2018, Rajendra Nayak wrote:
>
> > With rpmh ARC resources being modelled as power domains with
> > performance state, add support to proxy vote on these for SDM845.
> > Add support to vote on multiple of them, now that genpd supports
> > associating multiple power domains to a device.
>
> Thanks for writing up this patch Rajendra.
>
> > Signed-off-by: Rajendra Nayak <rnayak@xxxxxxxxxxxxxx>
> > ---
> > This patch is dependent on the rpmh powerdomain driver
> > still under review,
> > https://lkml.org/lkml/2018/6/27/7
> >
> >   drivers/remoteproc/qcom_q6v5_pil.c | 77 +++++++++++++++++++++++++++++-
> >   1 file changed, 75 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
> > index 2bf8e7c49f2a..2b5be6d15779 100644
> > --- a/drivers/remoteproc/qcom_q6v5_pil.c
> > +++ b/drivers/remoteproc/qcom_q6v5_pil.c
> > @@ -25,6 +25,8 @@
> >   #include <linux/of_address.h>
> >   #include <linux/of_device.h>
> >   #include <linux/platform_device.h>
> > +#include <linux/pm_domain.h>
> > +#include <linux/pm_runtime.h>
> >   #include <linux/regmap.h>
> >   #include <linux/regulator/consumer.h>
> >   #include <linux/remoteproc.h>
> > @@ -132,6 +134,7 @@ struct rproc_hexagon_res {
> >   	char **proxy_clk_names;
> >   	char **reset_clk_names;
> >   	char **active_clk_names;
> > +	char **pd_names;
> >   	int version;
> >   	bool need_mem_protection;
> >   	bool has_alt_reset;
> > @@ -161,9 +164,11 @@ struct q6v5 {
> >   	struct clk *active_clks[8];
> >   	struct clk *reset_clks[4];
> >   	struct clk *proxy_clks[4];
> > +	struct device *pd_devs[3];
> >   	int active_clk_count;
> >   	int reset_clk_count;
> >   	int proxy_clk_count;
> > +	int pd_count;
> >   
> >   	struct reg_info active_regs[1];
> >   	struct reg_info proxy_regs[3];
> > @@ -324,6 +329,23 @@ static void q6v5_clk_disable(struct device *dev,
> >   		clk_disable_unprepare(clks[i]);
> >   }
> >   
> > +static int q6v5_powerdomain_enable(struct device *dev, struct device **devs,
> > +				   int count)
> > +{
> > +	int i;
> > +
> > +	if (!count)
> > +		return 0;
> > +
> > +	if (count > 1)
> > +		for (i = 0; i < count; i++)
> > +			dev_pm_genpd_set_performance_state(devs[i], INT_MAX);
> > +	else
> > +		dev_pm_genpd_set_performance_state(dev, INT_MAX);
>
> I would prefer if we could just set the performance state during
> initialization, but I see that we only aggregate the state during
> dev_pm_genpd_set_performance_state().
>
> As such you need to also reduce the votes in the disable path; or we
> will just max out any shared corners from the first time we boot this
> remoteproc.

Right, I need to drop the votes along with doing a runtime suspend of the
device.

> For this to work I believe _genpd_power_o{n,ff}() would need to
> aggregate the performance state of all enabled consumers, something that
> would make the interface more convenient to use.

This isn't done today. There was some discussion in another thread on *if*
we should do this and what could be the implications [1]

> > +
> > +	return pm_runtime_get_sync(dev);
> > +}
> > +
> [..]
> > @@ -1142,6 +1173,35 @@ static int q6v5_init_clocks(struct device *dev, struct clk **clks,
> >   	return i;
> >   }
> >   
> > +static int q6v5_powerdomain_init(struct device *dev, struct device **devs,
> > +				 char **pd_names)
> > +{
> > +	int i = 0, num_pds;
> > +
> > +	if (!pd_names)
> > +		return 0;
> > +
> > +	while (pd_names[i])
> > +		i++;
> > +
> > +	num_pds = i;
> > +
> > +	if (num_pds > 1) {
> > +		for (i = 0; i < num_pds; i++) {
> > +			devs[i] = genpd_dev_pm_attach_by_id(dev, i);
>
> This should be done by_name

Right, I posted this out before the by_name api was available :)
I will move to it when I respin.

> > +			if (IS_ERR(devs[i]))
> > +				return PTR_ERR(devs[i]);
> > +			if (!device_link_add(dev, devs[i], DL_FLAG_STATELESS |
> > +					     DL_FLAG_PM_RUNTIME))
> > +				return -EINVAL;
> > +		}
> > +	}
> > +
> > +	pm_runtime_enable(dev);
>
> Don't you need a call to something like pm_suspend_ignore_children()
> here as well, to prevent a pm_runtime_get_sync() in a child device to
> power on our rails at runtime?

Are there any child nodes of remoteproc which do runtime control of
resources via runtime pm?

Thanks for the review.
regards,
Rajendra

[1] https://lkml.org/lkml/2018/6/15/139