Re: [PATCH v2 06/26] interconnect: icc-clk: add support for scaling using OPP

[Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>]

On 26/06/2023 14:28, Konrad Dybcio wrote:
> On 25.06.2023 22:25, Dmitry Baryshkov wrote:
> > Sometimes it might be required to scale the clock using the OPP
> > framework (e.g. to scale regulators following the required clock rate).
> > Extend the interconnec
> 't'
>
> > -clk framework to handle OPP case in addition to
> > scaling the clock.
> >
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> > ---
> I think we should check for OPP at the icc-clk registration time,
> instead of passing it as a parameter, e.g.:
>
> qn.opp = IS_ERR(dev_pm_opp_get_opp_count)
>
> Not sure if there's a more idiomatic way.

No. icc-clk is not limited to a single clock->icc conversion. So it is 
possible to create several interconnect links, only one of which should 
be the OPP-based one.

> Konrad
> >   drivers/interconnect/icc-clk.c   | 13 +++++++++++--
> >   include/linux/interconnect-clk.h |  1 +
> >   2 files changed, 12 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/interconnect/icc-clk.c b/drivers/interconnect/icc-clk.c
> > index 4d43ebff4257..c7962acdcee7 100644
> > --- a/drivers/interconnect/icc-clk.c
> > +++ b/drivers/interconnect/icc-clk.c
> > @@ -7,10 +7,13 @@
> >   #include <linux/device.h>
> >   #include <linux/interconnect-clk.h>
> >   #include <linux/interconnect-provider.h>
> > +#include <linux/pm_opp.h>
> >   
> >   struct icc_clk_node {
> > +	struct device *dev;
> >   	struct clk *clk;
> >   	bool enabled;
> > +	bool opp;
> >   };
> >   
> >   struct icc_clk_provider {
> > @@ -25,12 +28,16 @@ struct icc_clk_provider {
> >   static int icc_clk_set(struct icc_node *src, struct icc_node *dst)
> >   {
> >   	struct icc_clk_node *qn = src->data;
> > +	unsigned long rate = icc_units_to_bps(src->peak_bw);
> >   	int ret;
> >   
> >   	if (!qn || !qn->clk)
> >   		return 0;
> >   
> > -	if (!src->peak_bw) {
> > +	if (qn->opp)
> > +		return dev_pm_opp_set_rate(qn->dev, rate);
> > +
> > +	if (!rate) {
> >   		if (qn->enabled)
> >   			clk_disable_unprepare(qn->clk);
> >   		qn->enabled = false;
> > @@ -45,7 +52,7 @@ static int icc_clk_set(struct icc_node *src, struct icc_node *dst)
> >   		qn->enabled = true;
> >   	}
> >   
> > -	return clk_set_rate(qn->clk, icc_units_to_bps(src->peak_bw));
> > +	return clk_set_rate(qn->clk, rate);
> >   }
> >   
> >   static int icc_clk_get_bw(struct icc_node *node, u32 *avg, u32 *peak)
> > @@ -106,7 +113,9 @@ struct icc_provider *icc_clk_register(struct device *dev,
> >   	icc_provider_init(provider);
> >   
> >   	for (i = 0, j = 0; i < num_clocks; i++) {
> > +		qp->clocks[i].dev = dev;
> >   		qp->clocks[i].clk = data[i].clk;
> > +		qp->clocks[i].opp = data[i].opp;
> >   
> >   		node = icc_node_create(first_id + j);
> >   		if (IS_ERR(node)) {
> > diff --git a/include/linux/interconnect-clk.h b/include/linux/interconnect-clk.h
> > index 0cd80112bea5..c695e5099901 100644
> > --- a/include/linux/interconnect-clk.h
> > +++ b/include/linux/interconnect-clk.h
> > @@ -11,6 +11,7 @@ struct device;
> >   struct icc_clk_data {
> >   	struct clk *clk;
> >   	const char *name;
> > +	bool opp;
> >   };
> >   
> >   struct icc_provider *icc_clk_register(struct device *dev,

--
With best wishes
Dmitry