diff mbox series

[v2,1/3] interconnect: imx: Switch from imx_icc_node_adj_desc to fsl,icc-id node assignment

Message ID 20220101163956.3344467-1-abel.vesa@nxp.com
State Superseded
Headers show
Series [v2,1/3] interconnect: imx: Switch from imx_icc_node_adj_desc to fsl,icc-id node assignment | expand

Commit Message

Abel Vesa Jan. 1, 2022, 4:39 p.m. UTC 
In order to be able to have more than one NoCs in the interconnect net
we need to decouple the NoC from the dram. So instead of using the
imx_icc_node_adj_desc, we use the fsl,icc-id property that is in
each NoC (or pl301) to the icc node (based on the id) to it.
Along with all the NoC and pl301 nodes in the dts we will have a
interconnect dedicated node. This node will be the actual device of the
icc provider.

Signed-off-by: Abel Vesa <abel.vesa@nxp.com>
---

No changes since v1.

 drivers/interconnect/imx/imx.c | 70 +++++++++++++++-------------------
 1 file changed, 31 insertions(+), 39 deletions(-)

Comments

Georgi Djakov Jan. 1, 2022, 5:25 p.m. UTC | #1 
Hi Abel,

Thanks for working on this!

On 1.01.22 18:39, Abel Vesa wrote:
> In order to be able to have more than one NoCs in the interconnect net
> we need to decouple the NoC from the dram. So instead of using the
> imx_icc_node_adj_desc, we use the fsl,icc-id property that is in
> each NoC (or pl301) to the icc node (based on the id) to it.

I believe that this DT property should be documented somewhere.
Maybe in Documentation/devicetree/bindings/interconnect/fsl,imx8m-noc.yaml?

BR,
Georgi

> Along with all the NoC and pl301 nodes in the dts we will have a
> interconnect dedicated node. This node will be the actual device of the
> icc provider.
> 
> Signed-off-by: Abel Vesa <abel.vesa@nxp.com>
> ---
> 
> No changes since v1.
> 
>   drivers/interconnect/imx/imx.c | 70 +++++++++++++++-------------------
>   1 file changed, 31 insertions(+), 39 deletions(-)
Abel Vesa Jan. 3, 2022, 3:01 p.m. UTC | #2 
On 22-01-01 20:26:05, Georgi Djakov wrote:
> Hi Abel,
> 
> On 1.01.22 18:39, Abel Vesa wrote:
> > The aggregate function will return whatever is the highest
> > rate for that specific node. The imx_icc_get_bw sets the
> 
> Adding some more details about why we switch from
> icc_std_aggregate to imx_icc_aggregate would be nice.
> 

On a second look, I think I can drop the imx_icc_aggregate and use the
icc_std_aggregate instead, as long as I use the peak_bw and forget about
the agg_bw.

> > initial avg and peak to 0 in order to avoid setting them to
> > INT_MAX by the interconnect core.
> 
> Do we need a Fixes tag for this?
> 

Neah, the imx interconnect is not used by any platform yet.

> I would recommend to split imx_icc_get_bw and imx_icc_aggregate
> changes into separate patches. These also seem to be unrelated to
> the imx_icc_node_adj_desc patchset.
> 

Since I can use icc_std_aggregate, the imx_icc_aggregate change will be
dropped.

> Thanks,
> Georgi
> 
> > Signed-off-by: Abel Vesa <abel.vesa@nxp.com>
> > ---
> > 
> > No changes since v1.
> > 
> >   drivers/interconnect/imx/imx.c | 20 +++++++++++++++++++-
> >   1 file changed, 19 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/interconnect/imx/imx.c b/drivers/interconnect/imx/imx.c
> > index 34bfc7936387..4d8a2a2d2608 100644
> > --- a/drivers/interconnect/imx/imx.c
> > +++ b/drivers/interconnect/imx/imx.c
> > @@ -25,6 +25,23 @@ struct imx_icc_node {
> >   	struct dev_pm_qos_request qos_req;
> >   };
> > +static int imx_icc_get_bw(struct icc_node *node, u32 *avg, u32 *peak)
> > +{
> > +	*avg = 0;
> > +	*peak = 0;
> > +
> > +	return 0;
> > +}
> > +
> > +static int imx_icc_aggregate(struct icc_node *node, u32 tag, u32 avg_bw,
> > +		      u32 peak_bw, u32 *agg_avg, u32 *agg_peak)
> > +{
> > +	*agg_avg = max(*agg_avg, avg_bw);
> > +	*agg_peak = max(*agg_peak, peak_bw);
> > +
> > +	return 0;
> > +}
> > +
> >   static int imx_icc_node_set(struct icc_node *node)
> >   {
> >   	struct device *dev = node->provider->dev;
> > @@ -233,7 +250,8 @@ int imx_icc_register(struct platform_device *pdev,
> >   	if (!provider)
> >   		return -ENOMEM;
> >   	provider->set = imx_icc_set;
> > -	provider->aggregate = icc_std_aggregate;
> > +	provider->get_bw = imx_icc_get_bw;
> > +	provider->aggregate = imx_icc_aggregate;
> >   	provider->xlate = of_icc_xlate_onecell;
> >   	provider->data = data;
> >   	provider->dev = dev;
>
diff mbox series

Patch

diff --git a/drivers/interconnect/imx/imx.c b/drivers/interconnect/imx/imx.c
index c770951a909c..34bfc7936387 100644
--- a/drivers/interconnect/imx/imx.c
+++ b/drivers/interconnect/imx/imx.c
@@ -34,8 +34,8 @@  static int imx_icc_node_set(struct icc_node *node)
 	if (!node_data->qos_dev)
 		return 0;
 
-	freq = (node->avg_bw + node->peak_bw) * node_data->desc->adj->bw_mul;
-	do_div(freq, node_data->desc->adj->bw_div);
+	freq = max(node->avg_bw, node->peak_bw);
+
 	dev_dbg(dev, "node %s device %s avg_bw %ukBps peak_bw %ukBps min_freq %llukHz\n",
 		node->name, dev_name(node_data->qos_dev),
 		node->avg_bw, node->peak_bw, freq);
@@ -79,41 +79,35 @@  static int imx_icc_node_init_qos(struct icc_provider *provider,
 				 struct icc_node *node)
 {
 	struct imx_icc_node *node_data = node->data;
-	const struct imx_icc_node_adj_desc *adj = node_data->desc->adj;
 	struct device *dev = provider->dev;
-	struct device_node *dn = NULL;
 	struct platform_device *pdev;
+	struct device_node *np = NULL, *dn = NULL;
+	int idx;
 
-	if (adj->main_noc) {
-		node_data->qos_dev = dev;
-		dev_dbg(dev, "icc node %s[%d] is main noc itself\n",
-			node->name, node->id);
-	} else {
-		dn = of_parse_phandle(dev->of_node, adj->phandle_name, 0);
-		if (!dn) {
-			dev_warn(dev, "Failed to parse %s\n",
-				 adj->phandle_name);
-			return -ENODEV;
-		}
-		/* Allow scaling to be disabled on a per-node basis */
-		if (!of_device_is_available(dn)) {
-			dev_warn(dev, "Missing property %s, skip scaling %s\n",
-				 adj->phandle_name, node->name);
-			of_node_put(dn);
-			return 0;
-		}
+	for_each_node_with_property(np, "fsl,icc-id") {
+		of_property_read_u32(np, "fsl,icc-id", &idx);
+		if (idx == node_data->desc->id)
+			dn = np;
+	}
 
-		pdev = of_find_device_by_node(dn);
-		of_node_put(dn);
-		if (!pdev) {
-			dev_warn(dev, "node %s[%d] missing device for %pOF\n",
-				 node->name, node->id, dn);
-			return -EPROBE_DEFER;
-		}
-		node_data->qos_dev = &pdev->dev;
-		dev_dbg(dev, "node %s[%d] has device node %pOF\n",
-			node->name, node->id, dn);
+	if (!dn)
+		return 0;
+
+	if (!of_device_is_available(dn)) {
+		dev_warn(dev, "%pOF is disabled\n", dn);
+		return 0;
+	}
+
+	pdev = of_find_device_by_node(dn);
+	of_node_put(dn);
+	if (!pdev) {
+		dev_warn(dev, "node %s[%d] missing device for %pOF\n",
+			 node->name, node->id, dn);
+		return -EPROBE_DEFER;
 	}
+	node_data->qos_dev = &pdev->dev;
+	dev_dbg(dev, "node %s[%d] has device node %pOF\n", node->name,
+							node->id, dn);
 
 	return dev_pm_qos_add_request(node_data->qos_dev,
 				      &node_data->qos_req,
@@ -151,12 +145,10 @@  static struct icc_node *imx_icc_node_add(struct icc_provider *provider,
 	node_data->desc = node_desc;
 	icc_node_add(node, provider);
 
-	if (node_desc->adj) {
-		ret = imx_icc_node_init_qos(provider, node);
-		if (ret < 0) {
-			imx_icc_node_destroy(node);
-			return ERR_PTR(ret);
-		}
+	ret = imx_icc_node_init_qos(provider, node);
+	if (ret < 0) {
+		imx_icc_node_destroy(node);
+		return ERR_PTR(ret);
 	}
 
 	return node;
@@ -244,7 +236,7 @@  int imx_icc_register(struct platform_device *pdev,
 	provider->aggregate = icc_std_aggregate;
 	provider->xlate = of_icc_xlate_onecell;
 	provider->data = data;
-	provider->dev = dev->parent;
+	provider->dev = dev;
 	platform_set_drvdata(pdev, provider);
 
 	ret = icc_provider_add(provider);