[RFC,v2,09/11] devfreq: exynos-bus: Add interconnect functionality to exynos-bus

Commit Message

Artur Świgoń Sept. 19, 2019, 2:22 p.m. UTC

From: Artur Świgoń <[e-mail]>

This patch adds interconnect functionality to the exynos-bus devfreq
driver.

The SoC topology is a graph (or, more specifically, a tree) and most of
its edges are taken from the devfreq parent-child hierarchy (cf.
Documentation/devicetree/bindings/devfreq/exynos-bus.txt). Due to
unspecified relative probing order, -EPROBE_DEFER may be propagated to
guarantee that a child is probed before its parent.

Each bus is now an interconnect provider and an interconnect node as well
(cf. Documentation/interconnect/interconnect.rst), i.e. every bus registers
itself as a node. Node IDs are not hardcoded but rather assigned at
runtime, in probing order (subject to the above-mentioned exception
regarding relative order). This approach allows for using this driver with
various Exynos SoCs.

Frequencies requested via the interconnect API for a given node are
propagated to devfreq using dev_pm_qos_update_request(). Please note that
it is not an error when CONFIG_INTERCONNECT is 'n', in which case all
interconnect API functions are no-op.

Signed-off-by: Artur Świgoń <[e-mail]>
---
 drivers/devfreq/exynos-bus.c | 153 +++++++++++++++++++++++++++++++++++
 1 file changed, 153 insertions(+)

Comments

Chanwoo Choi Sept. 25, 2019, 7:03 a.m. UTC | #1

Hi,

I need the time to dig the ICC framework
to understand them detailed. After that, I'll review this.

Basically, I agree this approach. But, I'm wondering
the existing binding method between 'bus_leftbus' and 'bus_dmc'.
From before, I thought that devfreq framework need to
enhance the binding method between parent devfreq device
and passive devfreq device instead of 'devfreq' property.

On this patch, use the same binding way between
'bus_leftbus' and 'bus_dmc' with 'parent' property
as following:

+++ b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
@@ -106,6 +106,7 @@
 &bus_leftbus {
 	devfreq-events = <&ppmu_leftbus_3>, <&ppmu_rightbus_3>;
 	vdd-supply = <&buck3_reg>;
+	parent = <&bus_dmc>;
 	status = "okay";
 };

I'm not sure about continuing to use this method for new feature.
If possible, hope to replace the existing binding style
with new method like of_graph. Actually, I don't know the correct method.


On 19. 9. 19. 오후 11:22, Artur Świgoń wrote:
> From: Artur Świgoń <[e-mail]>
> 
> This patch adds interconnect functionality to the exynos-bus devfreq
> driver.
> 
> The SoC topology is a graph (or, more specifically, a tree) and most of
> its edges are taken from the devfreq parent-child hierarchy (cf.
> Documentation/devicetree/bindings/devfreq/exynos-bus.txt). Due to
> unspecified relative probing order, -EPROBE_DEFER may be propagated to
> guarantee that a child is probed before its parent.
> 
> Each bus is now an interconnect provider and an interconnect node as well
> (cf. Documentation/interconnect/interconnect.rst), i.e. every bus registers
> itself as a node. Node IDs are not hardcoded but rather assigned at
> runtime, in probing order (subject to the above-mentioned exception
> regarding relative order). This approach allows for using this driver with
> various Exynos SoCs.
> 
> Frequencies requested via the interconnect API for a given node are
> propagated to devfreq using dev_pm_qos_update_request(). Please note that
> it is not an error when CONFIG_INTERCONNECT is 'n', in which case all
> interconnect API functions are no-op.
> 
> Signed-off-by: Artur Świgoń <[e-mail]>
> ---
>  drivers/devfreq/exynos-bus.c | 153 +++++++++++++++++++++++++++++++++++
>  1 file changed, 153 insertions(+)
> 
> diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
> index 8d44810cac69..e0232202720d 100644
> --- a/drivers/devfreq/exynos-bus.c
> +++ b/drivers/devfreq/exynos-bus.c
> @@ -14,14 +14,19 @@
>  #include <linux/devfreq-event.h>
>  #include <linux/device.h>
>  #include <linux/export.h>
> +#include <linux/idr.h>
> +#include <linux/interconnect-provider.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/pm_opp.h>
> +#include <linux/pm_qos.h>
>  #include <linux/platform_device.h>
>  #include <linux/regulator/consumer.h>
>  
>  #define DEFAULT_SATURATION_RATIO	40
>  
> +#define icc_units_to_khz(x) ((x) / 8)
> +
>  struct exynos_bus {
>  	struct device *dev;
>  
> @@ -35,6 +40,12 @@ struct exynos_bus {
>  	struct opp_table *opp_table;
>  	struct clk *clk;
>  	unsigned int ratio;
> +
> +	/* One provider per bus, one node per provider */
> +	struct icc_provider provider;
> +	struct icc_node *node;
> +
> +	struct dev_pm_qos_request qos_req;
>  };
>  
>  /*
> @@ -59,6 +70,13 @@ exynos_bus_ops_edev(enable_edev);
>  exynos_bus_ops_edev(disable_edev);
>  exynos_bus_ops_edev(set_event);
>  
> +static int exynos_bus_next_id(void)
> +{
> +	static DEFINE_IDA(exynos_bus_icc_ida);
> +
> +	return ida_alloc(&exynos_bus_icc_ida, GFP_KERNEL);
> +}
> +
>  static int exynos_bus_get_event(struct exynos_bus *bus,
>  				struct devfreq_event_data *edata)
>  {
> @@ -171,6 +189,38 @@ static void exynos_bus_passive_exit(struct device *dev)
>  	clk_disable_unprepare(bus->clk);
>  }
>  
> +static int exynos_bus_icc_set(struct icc_node *src, struct icc_node *dst)
> +{
> +	struct exynos_bus *src_bus = src->data, *dst_bus = dst->data;
> +	s32 src_freq = icc_units_to_khz(src->avg_bw);
> +	s32 dst_freq = icc_units_to_khz(dst->avg_bw);
> +
> +	dev_pm_qos_update_request(&src_bus->qos_req, src_freq);
> +	dev_pm_qos_update_request(&dst_bus->qos_req, dst_freq);
> +
> +	return 0;
> +}
> +
> +static int exynos_bus_icc_aggregate(struct icc_node *node, u32 tag, u32 avg_bw,
> +				    u32 peak_bw, u32 *agg_avg, u32 *agg_peak)
> +{
> +	*agg_avg += avg_bw;
> +	*agg_peak = max(*agg_peak, peak_bw);
> +
> +	return 0;
> +}
> +
> +static struct icc_node *exynos_bus_icc_xlate(struct of_phandle_args *spec,
> +					     void *data)
> +{
> +	struct exynos_bus *bus = data;
> +
> +	if (spec->np != bus->dev->of_node)
> +		return ERR_PTR(-EINVAL);
> +
> +	return bus->node;
> +}
> +
>  static int exynos_bus_parent_parse_of(struct device_node *np,
>  					struct exynos_bus *bus)
>  {
> @@ -366,6 +416,101 @@ static int exynos_bus_profile_init_passive(struct exynos_bus *bus,
>  	return 0;
>  }
>  
> +static int exynos_bus_icc_connect(struct exynos_bus *bus)
> +{
> +	struct device_node *np = bus->dev->of_node;
> +	struct devfreq *parent_devfreq;
> +	struct icc_node *parent_node = NULL;
> +	struct of_phandle_args args;
> +	int ret = 0;
> +
> +	parent_devfreq = devfreq_get_devfreq_by_phandle(bus->dev, 0);
> +	if (!IS_ERR(parent_devfreq)) {
> +		struct exynos_bus *parent_bus;
> +
> +		parent_bus = dev_get_drvdata(parent_devfreq->dev.parent);
> +		parent_node = parent_bus->node;
> +	} else {
> +		/* Look for parent in DT */
> +		int num = of_count_phandle_with_args(np, "parent",
> +						     "#interconnect-cells");
> +		if (num != 1)
> +			goto out; /* 'parent' is optional */
> +
> +		ret = of_parse_phandle_with_args(np, "parent",
> +						 "#interconnect-cells",
> +						 0, &args);
> +		if (ret < 0)
> +			goto out;
> +
> +		of_node_put(args.np);
> +
> +		parent_node = of_icc_get_from_provider(&args);
> +		if (IS_ERR(parent_node)) {
> +			/* May be -EPROBE_DEFER */
> +			ret = PTR_ERR(parent_node);
> +			goto out;
> +		}
> +	}



> +
> +	ret = icc_link_create(bus->node, parent_node->id);
> +
> +out:
> +	return ret;
> +}
> +
> +static int exynos_bus_icc_init(struct exynos_bus *bus)
> +{
> +	struct device *dev = bus->dev;
> +	struct icc_provider *provider = &bus->provider;
> +	struct icc_node *node;
> +	int id, ret;
> +
> +	/* Initialize the interconnect provider */
> +	provider->set = exynos_bus_icc_set;
> +	provider->aggregate = exynos_bus_icc_aggregate;
> +	provider->xlate = exynos_bus_icc_xlate;
> +	provider->dev = dev;
> +	provider->data = bus;
> +
> +	ret = icc_provider_add(provider);
> +	if (ret < 0)
> +		goto out;
> +
> +	ret = id = exynos_bus_next_id();
> +	if (ret < 0)
> +		goto err_node;
> +
> +	node = icc_node_create(id);
> +	if (IS_ERR(node)) {
> +		ret = PTR_ERR(node);
> +		goto err_node;
> +	}
> +
> +	bus->node = node;
> +	node->name = dev->of_node->name;
> +	node->data = bus;
> +	icc_node_add(node, provider);
> +
> +	ret = exynos_bus_icc_connect(bus);
> +	if (ret < 0)
> +		goto err_connect;
> +
> +	ret = dev_pm_qos_add_request(bus->devfreq->dev.parent, &bus->qos_req,
> +				     DEV_PM_QOS_MIN_FREQUENCY, 0);
> +
> +out:
> +	return ret;
> +
> +err_connect:
> +	icc_node_del(node);
> +	icc_node_destroy(id);
> +err_node:
> +	icc_provider_del(provider);
> +
> +	return ret;
> +}
> +
>  static int exynos_bus_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> @@ -415,6 +560,14 @@ static int exynos_bus_probe(struct platform_device *pdev)
>  	if (ret < 0)
>  		goto err;
>  
> +	/*
> +	 * Initialize interconnect provider. A return value of -ENOTSUPP means
> +	 * that CONFIG_INTERCONNECT is disabled.
> +	 */
> +	ret = exynos_bus_icc_init(bus);
> +	if (ret < 0 && ret != -ENOTSUPP)
> +		goto err;
> +
>  	max_state = bus->devfreq->profile->max_state;
>  	min_freq = (bus->devfreq->profile->freq_table[0] / 1000);
>  	max_freq = (bus->devfreq->profile->freq_table[max_state - 1] / 1000);
>

Artur Świgoń Sept. 25, 2019, 7:12 a.m. UTC | #2

Hi,

On Wed, 2019-09-25 at 16:03 +0900, Chanwoo Choi wrote:
> Hi,
> 
> I need the time to dig the ICC framework
> to understand them detailed. After that, I'll review this.
> 
> Basically, I agree this approach. But, I'm wondering
> the existing binding method between 'bus_leftbus' and 'bus_dmc'.
> From before, I thought that devfreq framework need to
> enhance the binding method between parent devfreq device
> and passive devfreq device instead of 'devfreq' property.
> 
> On this patch, use the same binding way between
> 'bus_leftbus' and 'bus_dmc' with 'parent' property
> as following:
> 
> +++ b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> @@ -106,6 +106,7 @@
>  &bus_leftbus {
>  	devfreq-events = <&ppmu_leftbus_3>, <&ppmu_rightbus_3>;
>  	vdd-supply = <&buck3_reg>;
> +	parent = <&bus_dmc>;
>  	status = "okay";
>  };
> 
> I'm not sure about continuing to use this method for new feature.
> If possible, hope to replace the existing binding style
> with new method like of_graph. Actually, I don't know the correct method.

Adding the 'parent' binding was the simplest solution to create this
RFC to show the concept of using interconnect functionality in devfreq.
I agree that a method like of_graph is probably more elegant. I am open
to suggestions.

> On 19. 9. 19. 오후 11:22, Artur Świgoń wrote:
> > From: Artur Świgoń <[e-mail]>
> > 
> > This patch adds interconnect functionality to the exynos-bus devfreq
> > driver.
> > 
> > The SoC topology is a graph (or, more specifically, a tree) and most of
> > its edges are taken from the devfreq parent-child hierarchy (cf.
> > Documentation/devicetree/bindings/devfreq/exynos-bus.txt). Due to
> > unspecified relative probing order, -EPROBE_DEFER may be propagated to
> > guarantee that a child is probed before its parent.
> > 
> > Each bus is now an interconnect provider and an interconnect node as well
> > (cf. Documentation/interconnect/interconnect.rst), i.e. every bus registers
> > itself as a node. Node IDs are not hardcoded but rather assigned at
> > runtime, in probing order (subject to the above-mentioned exception
> > regarding relative order). This approach allows for using this driver with
> > various Exynos SoCs.
> > 
> > Frequencies requested via the interconnect API for a given node are
> > propagated to devfreq using dev_pm_qos_update_request(). Please note that
> > it is not an error when CONFIG_INTERCONNECT is 'n', in which case all
> > interconnect API functions are no-op.
> > 
> > Signed-off-by: Artur Świgoń <[e-mail]>
> > ---
> >  drivers/devfreq/exynos-bus.c | 153 +++++++++++++++++++++++++++++++++++
> >  1 file changed, 153 insertions(+)
> > 
> > diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
> > index 8d44810cac69..e0232202720d 100644
> > --- a/drivers/devfreq/exynos-bus.c
> > +++ b/drivers/devfreq/exynos-bus.c
> > @@ -14,14 +14,19 @@
> >  #include <linux/devfreq-event.h>
> >  #include <linux/device.h>
> >  #include <linux/export.h>
> > +#include <linux/idr.h>
> > +#include <linux/interconnect-provider.h>
> >  #include <linux/module.h>
> >  #include <linux/of.h>
> >  #include <linux/pm_opp.h>
> > +#include <linux/pm_qos.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/regulator/consumer.h>
> >  
> >  #define DEFAULT_SATURATION_RATIO	40
> >  
> > +#define icc_units_to_khz(x) ((x) / 8)
> > +
> >  struct exynos_bus {
> >  	struct device *dev;
> >  
> > @@ -35,6 +40,12 @@ struct exynos_bus {
> >  	struct opp_table *opp_table;
> >  	struct clk *clk;
> >  	unsigned int ratio;
> > +
> > +	/* One provider per bus, one node per provider */
> > +	struct icc_provider provider;
> > +	struct icc_node *node;
> > +
> > +	struct dev_pm_qos_request qos_req;
> >  };
> >  
> >  /*
> > @@ -59,6 +70,13 @@ exynos_bus_ops_edev(enable_edev);
> >  exynos_bus_ops_edev(disable_edev);
> >  exynos_bus_ops_edev(set_event);
> >  
> > +static int exynos_bus_next_id(void)
> > +{
> > +	static DEFINE_IDA(exynos_bus_icc_ida);
> > +
> > +	return ida_alloc(&exynos_bus_icc_ida, GFP_KERNEL);
> > +}
> > +
> >  static int exynos_bus_get_event(struct exynos_bus *bus,
> >  				struct devfreq_event_data *edata)
> >  {
> > @@ -171,6 +189,38 @@ static void exynos_bus_passive_exit(struct device *dev)
> >  	clk_disable_unprepare(bus->clk);
> >  }
> >  
> > +static int exynos_bus_icc_set(struct icc_node *src, struct icc_node *dst)
> > +{
> > +	struct exynos_bus *src_bus = src->data, *dst_bus = dst->data;
> > +	s32 src_freq = icc_units_to_khz(src->avg_bw);
> > +	s32 dst_freq = icc_units_to_khz(dst->avg_bw);
> > +
> > +	dev_pm_qos_update_request(&src_bus->qos_req, src_freq);
> > +	dev_pm_qos_update_request(&dst_bus->qos_req, dst_freq);
> > +
> > +	return 0;
> > +}
> > +
> > +static int exynos_bus_icc_aggregate(struct icc_node *node, u32 tag, u32 avg_bw,
> > +				    u32 peak_bw, u32 *agg_avg, u32 *agg_peak)
> > +{
> > +	*agg_avg += avg_bw;
> > +	*agg_peak = max(*agg_peak, peak_bw);
> > +
> > +	return 0;
> > +}
> > +
> > +static struct icc_node *exynos_bus_icc_xlate(struct of_phandle_args *spec,
> > +					     void *data)
> > +{
> > +	struct exynos_bus *bus = data;
> > +
> > +	if (spec->np != bus->dev->of_node)
> > +		return ERR_PTR(-EINVAL);
> > +
> > +	return bus->node;
> > +}
> > +
> >  static int exynos_bus_parent_parse_of(struct device_node *np,
> >  					struct exynos_bus *bus)
> >  {
> > @@ -366,6 +416,101 @@ static int exynos_bus_profile_init_passive(struct exynos_bus *bus,
> >  	return 0;
> >  }
> >  
> > +static int exynos_bus_icc_connect(struct exynos_bus *bus)
> > +{
> > +	struct device_node *np = bus->dev->of_node;
> > +	struct devfreq *parent_devfreq;
> > +	struct icc_node *parent_node = NULL;
> > +	struct of_phandle_args args;
> > +	int ret = 0;
> > +
> > +	parent_devfreq = devfreq_get_devfreq_by_phandle(bus->dev, 0);
> > +	if (!IS_ERR(parent_devfreq)) {
> > +		struct exynos_bus *parent_bus;
> > +
> > +		parent_bus = dev_get_drvdata(parent_devfreq->dev.parent);
> > +		parent_node = parent_bus->node;
> > +	} else {
> > +		/* Look for parent in DT */
> > +		int num = of_count_phandle_with_args(np, "parent",
> > +						     "#interconnect-cells");
> > +		if (num != 1)
> > +			goto out; /* 'parent' is optional */
> > +
> > +		ret = of_parse_phandle_with_args(np, "parent",
> > +						 "#interconnect-cells",
> > +						 0, &args);
> > +		if (ret < 0)
> > +			goto out;
> > +
> > +		of_node_put(args.np);
> > +
> > +		parent_node = of_icc_get_from_provider(&args);
> > +		if (IS_ERR(parent_node)) {
> > +			/* May be -EPROBE_DEFER */
> > +			ret = PTR_ERR(parent_node);
> > +			goto out;
> > +		}
> > +	}
> 
> 
> 
> > +
> > +	ret = icc_link_create(bus->node, parent_node->id);
> > +
> > +out:
> > +	return ret;
> > +}
> > +
> > +static int exynos_bus_icc_init(struct exynos_bus *bus)
> > +{
> > +	struct device *dev = bus->dev;
> > +	struct icc_provider *provider = &bus->provider;
> > +	struct icc_node *node;
> > +	int id, ret;
> > +
> > +	/* Initialize the interconnect provider */
> > +	provider->set = exynos_bus_icc_set;
> > +	provider->aggregate = exynos_bus_icc_aggregate;
> > +	provider->xlate = exynos_bus_icc_xlate;
> > +	provider->dev = dev;
> > +	provider->data = bus;
> > +
> > +	ret = icc_provider_add(provider);
> > +	if (ret < 0)
> > +		goto out;
> > +
> > +	ret = id = exynos_bus_next_id();
> > +	if (ret < 0)
> > +		goto err_node;
> > +
> > +	node = icc_node_create(id);
> > +	if (IS_ERR(node)) {
> > +		ret = PTR_ERR(node);
> > +		goto err_node;
> > +	}
> > +
> > +	bus->node = node;
> > +	node->name = dev->of_node->name;
> > +	node->data = bus;
> > +	icc_node_add(node, provider);
> > +
> > +	ret = exynos_bus_icc_connect(bus);
> > +	if (ret < 0)
> > +		goto err_connect;
> > +
> > +	ret = dev_pm_qos_add_request(bus->devfreq->dev.parent, &bus->qos_req,
> > +				     DEV_PM_QOS_MIN_FREQUENCY, 0);
> > +
> > +out:
> > +	return ret;
> > +
> > +err_connect:
> > +	icc_node_del(node);
> > +	icc_node_destroy(id);
> > +err_node:
> > +	icc_provider_del(provider);
> > +
> > +	return ret;
> > +}
> > +
> >  static int exynos_bus_probe(struct platform_device *pdev)
> >  {
> >  	struct device *dev = &pdev->dev;
> > @@ -415,6 +560,14 @@ static int exynos_bus_probe(struct platform_device *pdev)
> >  	if (ret < 0)
> >  		goto err;
> >  
> > +	/*
> > +	 * Initialize interconnect provider. A return value of -ENOTSUPP means
> > +	 * that CONFIG_INTERCONNECT is disabled.
> > +	 */
> > +	ret = exynos_bus_icc_init(bus);
> > +	if (ret < 0 && ret != -ENOTSUPP)
> > +		goto err;
> > +
> >  	max_state = bus->devfreq->profile->max_state;
> >  	min_freq = (bus->devfreq->profile->freq_table[0] / 1000);
> >  	max_freq = (bus->devfreq->profile->freq_table[max_state - 1] / 1000);
> > 
> 
>

Artur Świgoń Dec. 2, 2019, 5:05 p.m. UTC | #3

Hi Chanwoo,

On Wed, 2019-09-25 at 16:03 +0900, Chanwoo Choi wrote:
> Hi,
> 
> I need the time to dig the ICC framework
> to understand them detailed. After that, I'll review this.

Any updates on this topic?

Regardless of the purpose of this RFC, I think patches 01-04
are still beneficial to devfreq. I can rebase and post them
as a separate series if you wish.

> Basically, I agree this approach. But, I'm wondering
> the existing binding method between 'bus_leftbus' and 'bus_dmc'.
> From before, I thought that devfreq framework need to
> enhance the binding method between parent devfreq device
> and passive devfreq device instead of 'devfreq' property.
> 
> On this patch, use the same binding way between
> 'bus_leftbus' and 'bus_dmc' with 'parent' property
> as following:
> 
> +++ b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> @@ -106,6 +106,7 @@
>  &bus_leftbus {
>  	devfreq-events = <&ppmu_leftbus_3>, <&ppmu_rightbus_3>;
>  	vdd-supply = <&buck3_reg>;
> +	parent = <&bus_dmc>;
>  	status = "okay";
>  };
> 
> I'm not sure about continuing to use this method for new feature.
> If possible, hope to replace the existing binding style
> with new method like of_graph. Actually, I don't know the correct method.
> 
> 
> On 19. 9. 19. 오후 11:22, Artur Świgoń wrote:
> > From: Artur Świgoń <[e-mail]>
> > 
> > This patch adds interconnect functionality to the exynos-bus devfreq
> > driver.
> > 
> > The SoC topology is a graph (or, more specifically, a tree) and most of
> > its edges are taken from the devfreq parent-child hierarchy (cf.
> > Documentation/devicetree/bindings/devfreq/exynos-bus.txt). Due to
> > unspecified relative probing order, -EPROBE_DEFER may be propagated to
> > guarantee that a child is probed before its parent.
> > 
> > Each bus is now an interconnect provider and an interconnect node as well
> > (cf. Documentation/interconnect/interconnect.rst), i.e. every bus registers
> > itself as a node. Node IDs are not hardcoded but rather assigned at
> > runtime, in probing order (subject to the above-mentioned exception
> > regarding relative order). This approach allows for using this driver with
> > various Exynos SoCs.
> > 
> > Frequencies requested via the interconnect API for a given node are
> > propagated to devfreq using dev_pm_qos_update_request(). Please note that
> > it is not an error when CONFIG_INTERCONNECT is 'n', in which case all
> > interconnect API functions are no-op.
> > 
> > Signed-off-by: Artur Świgoń <[e-mail]>
> > ---
> >  drivers/devfreq/exynos-bus.c | 153 +++++++++++++++++++++++++++++++++++
> >  1 file changed, 153 insertions(+)
> > 
> > diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
> > index 8d44810cac69..e0232202720d 100644
> > --- a/drivers/devfreq/exynos-bus.c
> > +++ b/drivers/devfreq/exynos-bus.c
> > @@ -14,14 +14,19 @@
> >  #include <linux/devfreq-event.h>
> >  #include <linux/device.h>
> >  #include <linux/export.h>
> > +#include <linux/idr.h>
> > +#include <linux/interconnect-provider.h>
> >  #include <linux/module.h>
> >  #include <linux/of.h>
> >  #include <linux/pm_opp.h>
> > +#include <linux/pm_qos.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/regulator/consumer.h>
> >  
> >  #define DEFAULT_SATURATION_RATIO	40
> >  
> > +#define icc_units_to_khz(x) ((x) / 8)
> > +
> >  struct exynos_bus {
> >  	struct device *dev;
> >  
> > @@ -35,6 +40,12 @@ struct exynos_bus {
> >  	struct opp_table *opp_table;
> >  	struct clk *clk;
> >  	unsigned int ratio;
> > +
> > +	/* One provider per bus, one node per provider */
> > +	struct icc_provider provider;
> > +	struct icc_node *node;
> > +
> > +	struct dev_pm_qos_request qos_req;
> >  };
> >  
> >  /*
> > @@ -59,6 +70,13 @@ exynos_bus_ops_edev(enable_edev);
> >  exynos_bus_ops_edev(disable_edev);
> >  exynos_bus_ops_edev(set_event);
> >  
> > +static int exynos_bus_next_id(void)
> > +{
> > +	static DEFINE_IDA(exynos_bus_icc_ida);
> > +
> > +	return ida_alloc(&exynos_bus_icc_ida, GFP_KERNEL);
> > +}
> > +
> >  static int exynos_bus_get_event(struct exynos_bus *bus,
> >  				struct devfreq_event_data *edata)
> >  {
> > @@ -171,6 +189,38 @@ static void exynos_bus_passive_exit(struct device *dev)
> >  	clk_disable_unprepare(bus->clk);
> >  }
> >  
> > +static int exynos_bus_icc_set(struct icc_node *src, struct icc_node *dst)
> > +{
> > +	struct exynos_bus *src_bus = src->data, *dst_bus = dst->data;
> > +	s32 src_freq = icc_units_to_khz(src->avg_bw);
> > +	s32 dst_freq = icc_units_to_khz(dst->avg_bw);
> > +
> > +	dev_pm_qos_update_request(&src_bus->qos_req, src_freq);
> > +	dev_pm_qos_update_request(&dst_bus->qos_req, dst_freq);
> > +
> > +	return 0;
> > +}
> > +
> > +static int exynos_bus_icc_aggregate(struct icc_node *node, u32 tag, u32 avg_bw,
> > +				    u32 peak_bw, u32 *agg_avg, u32 *agg_peak)
> > +{
> > +	*agg_avg += avg_bw;
> > +	*agg_peak = max(*agg_peak, peak_bw);
> > +
> > +	return 0;
> > +}
> > +
> > +static struct icc_node *exynos_bus_icc_xlate(struct of_phandle_args *spec,
> > +					     void *data)
> > +{
> > +	struct exynos_bus *bus = data;
> > +
> > +	if (spec->np != bus->dev->of_node)
> > +		return ERR_PTR(-EINVAL);
> > +
> > +	return bus->node;
> > +}
> > +
> >  static int exynos_bus_parent_parse_of(struct device_node *np,
> >  					struct exynos_bus *bus)
> >  {
> > @@ -366,6 +416,101 @@ static int exynos_bus_profile_init_passive(struct exynos_bus *bus,
> >  	return 0;
> >  }
> >  
> > +static int exynos_bus_icc_connect(struct exynos_bus *bus)
> > +{
> > +	struct device_node *np = bus->dev->of_node;
> > +	struct devfreq *parent_devfreq;
> > +	struct icc_node *parent_node = NULL;
> > +	struct of_phandle_args args;
> > +	int ret = 0;
> > +
> > +	parent_devfreq = devfreq_get_devfreq_by_phandle(bus->dev, 0);
> > +	if (!IS_ERR(parent_devfreq)) {
> > +		struct exynos_bus *parent_bus;
> > +
> > +		parent_bus = dev_get_drvdata(parent_devfreq->dev.parent);
> > +		parent_node = parent_bus->node;
> > +	} else {
> > +		/* Look for parent in DT */
> > +		int num = of_count_phandle_with_args(np, "parent",
> > +						     "#interconnect-cells");
> > +		if (num != 1)
> > +			goto out; /* 'parent' is optional */
> > +
> > +		ret = of_parse_phandle_with_args(np, "parent",
> > +						 "#interconnect-cells",
> > +						 0, &args);
> > +		if (ret < 0)
> > +			goto out;
> > +
> > +		of_node_put(args.np);
> > +
> > +		parent_node = of_icc_get_from_provider(&args);
> > +		if (IS_ERR(parent_node)) {
> > +			/* May be -EPROBE_DEFER */
> > +			ret = PTR_ERR(parent_node);
> > +			goto out;
> > +		}
> > +	}
> 
> 
> 
> > +
> > +	ret = icc_link_create(bus->node, parent_node->id);
> > +
> > +out:
> > +	return ret;
> > +}
> > +
> > +static int exynos_bus_icc_init(struct exynos_bus *bus)
> > +{
> > +	struct device *dev = bus->dev;
> > +	struct icc_provider *provider = &bus->provider;
> > +	struct icc_node *node;
> > +	int id, ret;
> > +
> > +	/* Initialize the interconnect provider */
> > +	provider->set = exynos_bus_icc_set;
> > +	provider->aggregate = exynos_bus_icc_aggregate;
> > +	provider->xlate = exynos_bus_icc_xlate;
> > +	provider->dev = dev;
> > +	provider->data = bus;
> > +
> > +	ret = icc_provider_add(provider);
> > +	if (ret < 0)
> > +		goto out;
> > +
> > +	ret = id = exynos_bus_next_id();
> > +	if (ret < 0)
> > +		goto err_node;
> > +
> > +	node = icc_node_create(id);
> > +	if (IS_ERR(node)) {
> > +		ret = PTR_ERR(node);
> > +		goto err_node;
> > +	}
> > +
> > +	bus->node = node;
> > +	node->name = dev->of_node->name;
> > +	node->data = bus;
> > +	icc_node_add(node, provider);
> > +
> > +	ret = exynos_bus_icc_connect(bus);
> > +	if (ret < 0)
> > +		goto err_connect;
> > +
> > +	ret = dev_pm_qos_add_request(bus->devfreq->dev.parent, &bus->qos_req,
> > +				     DEV_PM_QOS_MIN_FREQUENCY, 0);
> > +
> > +out:
> > +	return ret;
> > +
> > +err_connect:
> > +	icc_node_del(node);
> > +	icc_node_destroy(id);
> > +err_node:
> > +	icc_provider_del(provider);
> > +
> > +	return ret;
> > +}
> > +
> >  static int exynos_bus_probe(struct platform_device *pdev)
> >  {
> >  	struct device *dev = &pdev->dev;
> > @@ -415,6 +560,14 @@ static int exynos_bus_probe(struct platform_device *pdev)
> >  	if (ret < 0)
> >  		goto err;
> >  
> > +	/*
> > +	 * Initialize interconnect provider. A return value of -ENOTSUPP means
> > +	 * that CONFIG_INTERCONNECT is disabled.
> > +	 */
> > +	ret = exynos_bus_icc_init(bus);
> > +	if (ret < 0 && ret != -ENOTSUPP)
> > +		goto err;
> > +
> >  	max_state = bus->devfreq->profile->max_state;
> >  	min_freq = (bus->devfreq->profile->freq_table[0] / 1000);
> >  	max_freq = (bus->devfreq->profile->freq_table[max_state - 1] / 1000);
> > 
> 

Best regards,

Chanwoo Choi Dec. 5, 2019, 2:36 a.m. UTC | #4

Hi Artur,

On 12/3/19 2:05 AM, Artur Świgoń wrote:
> Hi Chanwoo,
> 
> On Wed, 2019-09-25 at 16:03 +0900, Chanwoo Choi wrote:
>> Hi,
>>
>> I need the time to dig the ICC framework
>> to understand them detailed. After that, I'll review this.
> 
> Any updates on this topic?

I'm sorry for delaying the review of  this topic related to icc.
The review and merge of devfreq pm-qos feature will be finished over soon.
Because this series depends on the devfreq pm-qos feature

I'll dig into ICC related patches for exynos and imx[1].
[1] https://lore.kernel.org/linux-arm-kernel/[e-mail]/T/
[PATCH RFC v6 0/9] interconnect: Add imx support via devfreq

> 
> Regardless of the purpose of this RFC, I think patches 01-04
> are still beneficial to devfreq. I can rebase and post them
> as a separate series if you wish.

Yes. please split out patch1-4 from this series
and send them based on linux-next.git separately.

> 
>> Basically, I agree this approach. But, I'm wondering
>> the existing binding method between 'bus_leftbus' and 'bus_dmc'.
>> From before, I thought that devfreq framework need to
>> enhance the binding method between parent devfreq device
>> and passive devfreq device instead of 'devfreq' property.
>>
>> On this patch, use the same binding way between
>> 'bus_leftbus' and 'bus_dmc' with 'parent' property
>> as following:
>>
>> +++ b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
>> @@ -106,6 +106,7 @@
>>  &bus_leftbus {
>>  	devfreq-events = <&ppmu_leftbus_3>, <&ppmu_rightbus_3>;
>>  	vdd-supply = <&buck3_reg>;
>> +	parent = <&bus_dmc>;
>>  	status = "okay";
>>  };
>>
>> I'm not sure about continuing to use this method for new feature.
>> If possible, hope to replace the existing binding style
>> with new method like of_graph. Actually, I don't know the correct method.
>>
>>
>> On 19. 9. 19. 오후 11:22, Artur Świgoń wrote:
>>> From: Artur Świgoń <[e-mail]>
>>>
>>> This patch adds interconnect functionality to the exynos-bus devfreq
>>> driver.
>>>
>>> The SoC topology is a graph (or, more specifically, a tree) and most of
>>> its edges are taken from the devfreq parent-child hierarchy (cf.
>>> Documentation/devicetree/bindings/devfreq/exynos-bus.txt). Due to
>>> unspecified relative probing order, -EPROBE_DEFER may be propagated to
>>> guarantee that a child is probed before its parent.
>>>
>>> Each bus is now an interconnect provider and an interconnect node as well
>>> (cf. Documentation/interconnect/interconnect.rst), i.e. every bus registers
>>> itself as a node. Node IDs are not hardcoded but rather assigned at
>>> runtime, in probing order (subject to the above-mentioned exception
>>> regarding relative order). This approach allows for using this driver with
>>> various Exynos SoCs.
>>>
>>> Frequencies requested via the interconnect API for a given node are
>>> propagated to devfreq using dev_pm_qos_update_request(). Please note that
>>> it is not an error when CONFIG_INTERCONNECT is 'n', in which case all
>>> interconnect API functions are no-op.
>>>
>>> Signed-off-by: Artur Świgoń <[e-mail]>
>>> ---
>>>  drivers/devfreq/exynos-bus.c | 153 +++++++++++++++++++++++++++++++++++
>>>  1 file changed, 153 insertions(+)
>>>
>>> diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
>>> index 8d44810cac69..e0232202720d 100644
>>> --- a/drivers/devfreq/exynos-bus.c
>>> +++ b/drivers/devfreq/exynos-bus.c
>>> @@ -14,14 +14,19 @@
>>>  #include <linux/devfreq-event.h>
>>>  #include <linux/device.h>
>>>  #include <linux/export.h>
>>> +#include <linux/idr.h>
>>> +#include <linux/interconnect-provider.h>
>>>  #include <linux/module.h>
>>>  #include <linux/of.h>
>>>  #include <linux/pm_opp.h>
>>> +#include <linux/pm_qos.h>
>>>  #include <linux/platform_device.h>
>>>  #include <linux/regulator/consumer.h>
>>>  
>>>  #define DEFAULT_SATURATION_RATIO	40
>>>  
>>> +#define icc_units_to_khz(x) ((x) / 8)
>>> +
>>>  struct exynos_bus {
>>>  	struct device *dev;
>>>  
>>> @@ -35,6 +40,12 @@ struct exynos_bus {
>>>  	struct opp_table *opp_table;
>>>  	struct clk *clk;
>>>  	unsigned int ratio;
>>> +
>>> +	/* One provider per bus, one node per provider */
>>> +	struct icc_provider provider;
>>> +	struct icc_node *node;
>>> +
>>> +	struct dev_pm_qos_request qos_req;
>>>  };
>>>  
>>>  /*
>>> @@ -59,6 +70,13 @@ exynos_bus_ops_edev(enable_edev);
>>>  exynos_bus_ops_edev(disable_edev);
>>>  exynos_bus_ops_edev(set_event);
>>>  
>>> +static int exynos_bus_next_id(void)
>>> +{
>>> +	static DEFINE_IDA(exynos_bus_icc_ida);
>>> +
>>> +	return ida_alloc(&exynos_bus_icc_ida, GFP_KERNEL);
>>> +}
>>> +
>>>  static int exynos_bus_get_event(struct exynos_bus *bus,
>>>  				struct devfreq_event_data *edata)
>>>  {
>>> @@ -171,6 +189,38 @@ static void exynos_bus_passive_exit(struct device *dev)
>>>  	clk_disable_unprepare(bus->clk);
>>>  }
>>>  
>>> +static int exynos_bus_icc_set(struct icc_node *src, struct icc_node *dst)
>>> +{
>>> +	struct exynos_bus *src_bus = src->data, *dst_bus = dst->data;
>>> +	s32 src_freq = icc_units_to_khz(src->avg_bw);
>>> +	s32 dst_freq = icc_units_to_khz(dst->avg_bw);
>>> +
>>> +	dev_pm_qos_update_request(&src_bus->qos_req, src_freq);
>>> +	dev_pm_qos_update_request(&dst_bus->qos_req, dst_freq);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int exynos_bus_icc_aggregate(struct icc_node *node, u32 tag, u32 avg_bw,
>>> +				    u32 peak_bw, u32 *agg_avg, u32 *agg_peak)
>>> +{
>>> +	*agg_avg += avg_bw;
>>> +	*agg_peak = max(*agg_peak, peak_bw);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static struct icc_node *exynos_bus_icc_xlate(struct of_phandle_args *spec,
>>> +					     void *data)
>>> +{
>>> +	struct exynos_bus *bus = data;
>>> +
>>> +	if (spec->np != bus->dev->of_node)
>>> +		return ERR_PTR(-EINVAL);
>>> +
>>> +	return bus->node;
>>> +}
>>> +
>>>  static int exynos_bus_parent_parse_of(struct device_node *np,
>>>  					struct exynos_bus *bus)
>>>  {
>>> @@ -366,6 +416,101 @@ static int exynos_bus_profile_init_passive(struct exynos_bus *bus,
>>>  	return 0;
>>>  }
>>>  
>>> +static int exynos_bus_icc_connect(struct exynos_bus *bus)
>>> +{
>>> +	struct device_node *np = bus->dev->of_node;
>>> +	struct devfreq *parent_devfreq;
>>> +	struct icc_node *parent_node = NULL;
>>> +	struct of_phandle_args args;
>>> +	int ret = 0;
>>> +
>>> +	parent_devfreq = devfreq_get_devfreq_by_phandle(bus->dev, 0);
>>> +	if (!IS_ERR(parent_devfreq)) {
>>> +		struct exynos_bus *parent_bus;
>>> +
>>> +		parent_bus = dev_get_drvdata(parent_devfreq->dev.parent);
>>> +		parent_node = parent_bus->node;
>>> +	} else {
>>> +		/* Look for parent in DT */
>>> +		int num = of_count_phandle_with_args(np, "parent",
>>> +						     "#interconnect-cells");
>>> +		if (num != 1)
>>> +			goto out; /* 'parent' is optional */
>>> +
>>> +		ret = of_parse_phandle_with_args(np, "parent",
>>> +						 "#interconnect-cells",
>>> +						 0, &args);
>>> +		if (ret < 0)
>>> +			goto out;
>>> +
>>> +		of_node_put(args.np);
>>> +
>>> +		parent_node = of_icc_get_from_provider(&args);
>>> +		if (IS_ERR(parent_node)) {
>>> +			/* May be -EPROBE_DEFER */
>>> +			ret = PTR_ERR(parent_node);
>>> +			goto out;
>>> +		}
>>> +	}
>>
>>
>>
>>> +
>>> +	ret = icc_link_create(bus->node, parent_node->id);
>>> +
>>> +out:
>>> +	return ret;
>>> +}
>>> +
>>> +static int exynos_bus_icc_init(struct exynos_bus *bus)
>>> +{
>>> +	struct device *dev = bus->dev;
>>> +	struct icc_provider *provider = &bus->provider;
>>> +	struct icc_node *node;
>>> +	int id, ret;
>>> +
>>> +	/* Initialize the interconnect provider */
>>> +	provider->set = exynos_bus_icc_set;
>>> +	provider->aggregate = exynos_bus_icc_aggregate;
>>> +	provider->xlate = exynos_bus_icc_xlate;
>>> +	provider->dev = dev;
>>> +	provider->data = bus;
>>> +
>>> +	ret = icc_provider_add(provider);
>>> +	if (ret < 0)
>>> +		goto out;
>>> +
>>> +	ret = id = exynos_bus_next_id();
>>> +	if (ret < 0)
>>> +		goto err_node;
>>> +
>>> +	node = icc_node_create(id);
>>> +	if (IS_ERR(node)) {
>>> +		ret = PTR_ERR(node);
>>> +		goto err_node;
>>> +	}
>>> +
>>> +	bus->node = node;
>>> +	node->name = dev->of_node->name;
>>> +	node->data = bus;
>>> +	icc_node_add(node, provider);
>>> +
>>> +	ret = exynos_bus_icc_connect(bus);
>>> +	if (ret < 0)
>>> +		goto err_connect;
>>> +
>>> +	ret = dev_pm_qos_add_request(bus->devfreq->dev.parent, &bus->qos_req,
>>> +				     DEV_PM_QOS_MIN_FREQUENCY, 0);
>>> +
>>> +out:
>>> +	return ret;
>>> +
>>> +err_connect:
>>> +	icc_node_del(node);
>>> +	icc_node_destroy(id);
>>> +err_node:
>>> +	icc_provider_del(provider);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>>  static int exynos_bus_probe(struct platform_device *pdev)
>>>  {
>>>  	struct device *dev = &pdev->dev;
>>> @@ -415,6 +560,14 @@ static int exynos_bus_probe(struct platform_device *pdev)
>>>  	if (ret < 0)
>>>  		goto err;
>>>  
>>> +	/*
>>> +	 * Initialize interconnect provider. A return value of -ENOTSUPP means
>>> +	 * that CONFIG_INTERCONNECT is disabled.
>>> +	 */
>>> +	ret = exynos_bus_icc_init(bus);
>>> +	if (ret < 0 && ret != -ENOTSUPP)
>>> +		goto err;
>>> +
>>>  	max_state = bus->devfreq->profile->max_state;
>>>  	min_freq = (bus->devfreq->profile->freq_table[0] / 1000);
>>>  	max_freq = (bus->devfreq->profile->freq_table[max_state - 1] / 1000);
>>>
>>
> 
> Best regards,
>

Chanwoo Choi Dec. 16, 2019, 12:44 a.m. UTC | #5

Hi,

On 9/19/19 11:22 PM, Artur Świgoń wrote:
> From: Artur Świgoń <[e-mail]>
> 
> This patch adds interconnect functionality to the exynos-bus devfreq
> driver.
> 
> The SoC topology is a graph (or, more specifically, a tree) and most of
> its edges are taken from the devfreq parent-child hierarchy (cf.
> Documentation/devicetree/bindings/devfreq/exynos-bus.txt). Due to
> unspecified relative probing order, -EPROBE_DEFER may be propagated to
> guarantee that a child is probed before its parent.
> 
> Each bus is now an interconnect provider and an interconnect node as well
> (cf. Documentation/interconnect/interconnect.rst), i.e. every bus registers
> itself as a node. Node IDs are not hardcoded but rather assigned at
> runtime, in probing order (subject to the above-mentioned exception
> regarding relative order). This approach allows for using this driver with
> various Exynos SoCs.
> 
> Frequencies requested via the interconnect API for a given node are
> propagated to devfreq using dev_pm_qos_update_request(). Please note that
> it is not an error when CONFIG_INTERCONNECT is 'n', in which case all
> interconnect API functions are no-op.
> 
> Signed-off-by: Artur Świgoń <[e-mail]>
> ---
>  drivers/devfreq/exynos-bus.c | 153 +++++++++++++++++++++++++++++++++++
>  1 file changed, 153 insertions(+)
> 
> diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
> index 8d44810cac69..e0232202720d 100644
> --- a/drivers/devfreq/exynos-bus.c
> +++ b/drivers/devfreq/exynos-bus.c
> @@ -14,14 +14,19 @@
>  #include <linux/devfreq-event.h>
>  #include <linux/device.h>
>  #include <linux/export.h>
> +#include <linux/idr.h>
> +#include <linux/interconnect-provider.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/pm_opp.h>
> +#include <linux/pm_qos.h>
>  #include <linux/platform_device.h>
>  #include <linux/regulator/consumer.h>
>  
>  #define DEFAULT_SATURATION_RATIO	40
>  
> +#define icc_units_to_khz(x) ((x) / 8)

icc_units_to_khz() -> kpbs_to_khz()

> +
>  struct exynos_bus {
>  	struct device *dev;
>  
> @@ -35,6 +40,12 @@ struct exynos_bus {
>  	struct opp_table *opp_table;
>  	struct clk *clk;
>  	unsigned int ratio;
> +
> +	/* One provider per bus, one node per provider */
> +	struct icc_provider provider;
> +	struct icc_node *node;
> +
> +	struct dev_pm_qos_request qos_req;
>  };
>  
>  /*
> @@ -59,6 +70,13 @@ exynos_bus_ops_edev(enable_edev);
>  exynos_bus_ops_edev(disable_edev);
>  exynos_bus_ops_edev(set_event);
>  
> +static int exynos_bus_next_id(void)
> +{
> +	static DEFINE_IDA(exynos_bus_icc_ida);
> +
> +	return ida_alloc(&exynos_bus_icc_ida, GFP_KERNEL);
> +}
> +
>  static int exynos_bus_get_event(struct exynos_bus *bus,
>  				struct devfreq_event_data *edata)
>  {
> @@ -171,6 +189,38 @@ static void exynos_bus_passive_exit(struct device *dev)
>  	clk_disable_unprepare(bus->clk);
>  }
>  
> +static int exynos_bus_icc_set(struct icc_node *src, struct icc_node *dst)
> +{
> +	struct exynos_bus *src_bus = src->data, *dst_bus = dst->data;
> +	s32 src_freq = icc_units_to_khz(src->avg_bw);
> +	s32 dst_freq = icc_units_to_khz(dst->avg_bw);
> +
> +	dev_pm_qos_update_request(&src_bus->qos_req, src_freq);

Have to check the return value.
If return error, show the waring with dev_warn.

> +	dev_pm_qos_update_request(&dst_bus->qos_req, dst_freq);

ditto.

> +
> +	return 0;
> +}
> +
> +static int exynos_bus_icc_aggregate(struct icc_node *node, u32 tag, u32 avg_bw,
> +				    u32 peak_bw, u32 *agg_avg, u32 *agg_peak)
> +{
> +	*agg_avg += avg_bw;
> +	*agg_peak = max(*agg_peak, peak_bw);
> +
> +	return 0;
> +}
> +
> +static struct icc_node *exynos_bus_icc_xlate(struct of_phandle_args *spec,
> +					     void *data)
> +{
> +	struct exynos_bus *bus = data;
> +
> +	if (spec->np != bus->dev->of_node)
> +		return ERR_PTR(-EINVAL);
> +
> +	return bus->node;
> +}
> +
>  static int exynos_bus_parent_parse_of(struct device_node *np,
>  					struct exynos_bus *bus)
>  {
> @@ -366,6 +416,101 @@ static int exynos_bus_profile_init_passive(struct exynos_bus *bus,
>  	return 0;
>  }
>  
> +static int exynos_bus_icc_connect(struct exynos_bus *bus)
> +{
> +	struct device_node *np = bus->dev->of_node;
> +	struct devfreq *parent_devfreq;
> +	struct icc_node *parent_node = NULL;
> +	struct of_phandle_args args;
> +	int ret = 0;
> +
> +	parent_devfreq = devfreq_get_devfreq_by_phandle(bus->dev, 0);
> +	if (!IS_ERR(parent_devfreq)) {
> +		struct exynos_bus *parent_bus;
> +
> +		parent_bus = dev_get_drvdata(parent_devfreq->dev.parent);
> +		parent_node = parent_bus->node;
> +	} else {
> +		/* Look for parent in DT */
> +		int num = of_count_phandle_with_args(np, "parent",
> +						     "#interconnect-cells");
> +		if (num != 1)
> +			goto out; /* 'parent' is optional */
> +
> +		ret = of_parse_phandle_with_args(np, "parent",
> +						 "#interconnect-cells",
> +						 0, &args);


Actually, I agree your approach. I think that it is very useful
and necessary to guarantee the PM QoS requirements between devices.

But,
As I already commented, I'm not sure that the "parent" property 
is proper for only this driver. If possible, you better to get
the parent phandle through other way like OF graph.

If you suggest the standard way to make the tree between
the exynos-bus, I'll agree.

Also, for interconnect path, you have to add the connection
between 'bus_display' and 'bus_leftbus' regardless
of the existing 'devfreq' property.
- bus_display - bus_leftbus - bus_dmc

> +		if (ret < 0)
> +			goto out;
> +
> +		of_node_put(args.np);
> +
> +		parent_node = of_icc_get_from_provider(&args);
> +		if (IS_ERR(parent_node)) {
> +			/* May be -EPROBE_DEFER */
> +			ret = PTR_ERR(parent_node);
> +			goto out;
> +		}
> +	}
> +
> +	ret = icc_link_create(bus->node, parent_node->id);
> +
> +out:
> +	return ret;
> +}
> +
> +static int exynos_bus_icc_init(struct exynos_bus *bus)
> +{
> +	struct device *dev = bus->dev;
> +	struct icc_provider *provider = &bus->provider;
> +	struct icc_node *node;
> +	int id, ret;
> +
> +	/* Initialize the interconnect provider */
> +	provider->set = exynos_bus_icc_set;
> +	provider->aggregate = exynos_bus_icc_aggregate;
> +	provider->xlate = exynos_bus_icc_xlate;
> +	provider->dev = dev;
> +	provider->data = bus;
> +
> +	ret = icc_provider_add(provider);
> +	if (ret < 0)
> +		goto out;

Return error without goto because there is no any requirement
to free the resource before.

> +
> +	ret = id = exynos_bus_next_id();
> +	if (ret < 0)
> +		goto err_node;
> +
> +	node = icc_node_create(id);
> +	if (IS_ERR(node)) {
> +		ret = PTR_ERR(node);
> +		goto err_node;
> +	}
> +
> +	bus->node = node;
> +	node->name = dev->of_node->name;
> +	node->data = bus;
> +	icc_node_add(node, provider);
> +
> +	ret = exynos_bus_icc_connect(bus);
> +	if (ret < 0)
> +		goto err_connect;
> +
> +	ret = dev_pm_qos_add_request(bus->devfreq->dev.parent, &bus->qos_req,

Check whether this line is over 80 char.

> +				     DEV_PM_QOS_MIN_FREQUENCY, 0);

	Check the return value.

> +
> +out:

Remove this goto due to not necessary.

> +	return ret;

	return 0;

> +
> +err_connect:
> +	icc_node_del(node);
> +	icc_node_destroy(id);
> +err_node:
> +	icc_provider_del(provider);
> +
> +	return ret;
> +}
> +
>  static int exynos_bus_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> @@ -415,6 +560,14 @@ static int exynos_bus_probe(struct platform_device *pdev)
>  	if (ret < 0)
>  		goto err;
>  
> +	/*
> +	 * Initialize interconnect provider. A return value of -ENOTSUPP means
> +	 * that CONFIG_INTERCONNECT is disabled.
> +	 */
> +	ret = exynos_bus_icc_init(bus);
> +	if (ret < 0 && ret != -ENOTSUPP)
> +		goto err;

Print error message.
	dev_err(dev, "failed to initialize the interconnect provider");

> +
>  	max_state = bus->devfreq->profile->max_state;
>  	min_freq = (bus->devfreq->profile->freq_table[0] / 1000);
>  	max_freq = (bus->devfreq->profile->freq_table[max_state - 1] / 1000);
>

Artur Świgoń Dec. 18, 2019, 10:18 a.m. UTC | #6

Hi,

Thank you for the review.

On Mon, 2019-12-16 at 09:44 +0900, Chanwoo Choi wrote:
> Hi,
> 
> On 9/19/19 11:22 PM, Artur Świgoń wrote:
> > From: Artur Świgoń <[e-mail]>
> > 
> > This patch adds interconnect functionality to the exynos-bus devfreq
> > driver.
> > 
> > The SoC topology is a graph (or, more specifically, a tree) and most of
> > its edges are taken from the devfreq parent-child hierarchy (cf.
> > Documentation/devicetree/bindings/devfreq/exynos-bus.txt). Due to
> > unspecified relative probing order, -EPROBE_DEFER may be propagated to
> > guarantee that a child is probed before its parent.
> > 
> > Each bus is now an interconnect provider and an interconnect node as well
> > (cf. Documentation/interconnect/interconnect.rst), i.e. every bus registers
> > itself as a node. Node IDs are not hardcoded but rather assigned at
> > runtime, in probing order (subject to the above-mentioned exception
> > regarding relative order). This approach allows for using this driver with
> > various Exynos SoCs.
> > 
> > Frequencies requested via the interconnect API for a given node are
> > propagated to devfreq using dev_pm_qos_update_request(). Please note that
> > it is not an error when CONFIG_INTERCONNECT is 'n', in which case all
> > interconnect API functions are no-op.
> > 
> > Signed-off-by: Artur Świgoń <[e-mail]>
> > ---
> >  drivers/devfreq/exynos-bus.c | 153 +++++++++++++++++++++++++++++++++++
> >  1 file changed, 153 insertions(+)
> > 
> > diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
> > index 8d44810cac69..e0232202720d 100644
> > --- a/drivers/devfreq/exynos-bus.c
> > +++ b/drivers/devfreq/exynos-bus.c
> > @@ -14,14 +14,19 @@
> >  #include <linux/devfreq-event.h>
> >  #include <linux/device.h>
> >  #include <linux/export.h>
> > +#include <linux/idr.h>
> > +#include <linux/interconnect-provider.h>
> >  #include <linux/module.h>
> >  #include <linux/of.h>
> >  #include <linux/pm_opp.h>
> > +#include <linux/pm_qos.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/regulator/consumer.h>
> >  
> >  #define DEFAULT_SATURATION_RATIO	40
> >  
> > +#define icc_units_to_khz(x) ((x) / 8)
> 
> icc_units_to_khz() -> kpbs_to_khz()

OK

> > +
> >  struct exynos_bus {
> >  	struct device *dev;
> >  
> > @@ -35,6 +40,12 @@ struct exynos_bus {
> >  	struct opp_table *opp_table;
> >  	struct clk *clk;
> >  	unsigned int ratio;
> > +
> > +	/* One provider per bus, one node per provider */
> > +	struct icc_provider provider;
> > +	struct icc_node *node;
> > +
> > +	struct dev_pm_qos_request qos_req;
> >  };
> >  
> >  /*
> > @@ -59,6 +70,13 @@ exynos_bus_ops_edev(enable_edev);
> >  exynos_bus_ops_edev(disable_edev);
> >  exynos_bus_ops_edev(set_event);
> >  
> > +static int exynos_bus_next_id(void)
> > +{
> > +	static DEFINE_IDA(exynos_bus_icc_ida);
> > +
> > +	return ida_alloc(&exynos_bus_icc_ida, GFP_KERNEL);
> > +}
> > +
> >  static int exynos_bus_get_event(struct exynos_bus *bus,
> >  				struct devfreq_event_data *edata)
> >  {
> > @@ -171,6 +189,38 @@ static void exynos_bus_passive_exit(struct device *dev)
> >  	clk_disable_unprepare(bus->clk);
> >  }
> >  
> > +static int exynos_bus_icc_set(struct icc_node *src, struct icc_node *dst)
> > +{
> > +	struct exynos_bus *src_bus = src->data, *dst_bus = dst->data;
> > +	s32 src_freq = icc_units_to_khz(src->avg_bw);
> > +	s32 dst_freq = icc_units_to_khz(dst->avg_bw);
> > +
> > +	dev_pm_qos_update_request(&src_bus->qos_req, src_freq);
> 
> Have to check the return value.
> If return error, show the waring with dev_warn.

OK, I will change it to:

ret = dev_pm_qos_update_request(&src_bus->qos_req, src_freq);
if (ret < 0) {
	dev_warn(src_bus->dev, "failed to update PM QoS request");
	return ret;
}

> > +	dev_pm_qos_update_request(&dst_bus->qos_req, dst_freq);
> 
> ditto.

OK (same as above).

> > +
> > +	return 0;
> > +}
> > +
> > +static int exynos_bus_icc_aggregate(struct icc_node *node, u32 tag, u32 avg_bw,
> > +				    u32 peak_bw, u32 *agg_avg, u32 *agg_peak)
> > +{
> > +	*agg_avg += avg_bw;
> > +	*agg_peak = max(*agg_peak, peak_bw);
> > +
> > +	return 0;
> > +}
> > +
> > +static struct icc_node *exynos_bus_icc_xlate(struct of_phandle_args *spec,
> > +					     void *data)
> > +{
> > +	struct exynos_bus *bus = data;
> > +
> > +	if (spec->np != bus->dev->of_node)
> > +		return ERR_PTR(-EINVAL);
> > +
> > +	return bus->node;
> > +}
> > +
> >  static int exynos_bus_parent_parse_of(struct device_node *np,
> >  					struct exynos_bus *bus)
> >  {
> > @@ -366,6 +416,101 @@ static int exynos_bus_profile_init_passive(struct exynos_bus *bus,
> >  	return 0;
> >  }
> >  
> > +static int exynos_bus_icc_connect(struct exynos_bus *bus)
> > +{
> > +	struct device_node *np = bus->dev->of_node;
> > +	struct devfreq *parent_devfreq;
> > +	struct icc_node *parent_node = NULL;
> > +	struct of_phandle_args args;
> > +	int ret = 0;
> > +
> > +	parent_devfreq = devfreq_get_devfreq_by_phandle(bus->dev, 0);
> > +	if (!IS_ERR(parent_devfreq)) {
> > +		struct exynos_bus *parent_bus;
> > +
> > +		parent_bus = dev_get_drvdata(parent_devfreq->dev.parent);
> > +		parent_node = parent_bus->node;
> > +	} else {
> > +		/* Look for parent in DT */
> > +		int num = of_count_phandle_with_args(np, "parent",
> > +						     "#interconnect-cells");
> > +		if (num != 1)
> > +			goto out; /* 'parent' is optional */
> > +
> > +		ret = of_parse_phandle_with_args(np, "parent",
> > +						 "#interconnect-cells",
> > +						 0, &args);
> 
> 
> Actually, I agree your approach. I think that it is very useful
> and necessary to guarantee the PM QoS requirements between devices.
> 
> But,
> As I already commented, I'm not sure that the "parent" property 
> is proper for only this driver. If possible, you better to get
> the parent phandle through other way like OF graph.
> 
> If you suggest the standard way to make the tree between
> the exynos-bus, I'll agree.

As I commented in the answer to patch 08, I will use the
'exynos,interconnect-parent-node' property for bus_display,
bus_leftbus and bus_dmc.

> Also, for interconnect path, you have to add the connection
> between 'bus_display' and 'bus_leftbus' regardless
> of the existing 'devfreq' property.
> - bus_display - bus_leftbus - bus_dmc
> 
> > +		if (ret < 0)
> > +			goto out;
> > +
> > +		of_node_put(args.np);
> > +
> > +		parent_node = of_icc_get_from_provider(&args);
> > +		if (IS_ERR(parent_node)) {
> > +			/* May be -EPROBE_DEFER */
> > +			ret = PTR_ERR(parent_node);
> > +			goto out;
> > +		}
> > +	}
> > +
> > +	ret = icc_link_create(bus->node, parent_node->id);
> > +
> > +out:
> > +	return ret;
> > +}
> > +
> > +static int exynos_bus_icc_init(struct exynos_bus *bus)
> > +{
> > +	struct device *dev = bus->dev;
> > +	struct icc_provider *provider = &bus->provider;
> > +	struct icc_node *node;
> > +	int id, ret;
> > +
> > +	/* Initialize the interconnect provider */
> > +	provider->set = exynos_bus_icc_set;
> > +	provider->aggregate = exynos_bus_icc_aggregate;
> > +	provider->xlate = exynos_bus_icc_xlate;
> > +	provider->dev = dev;
> > +	provider->data = bus;
> > +
> > +	ret = icc_provider_add(provider);
> > +	if (ret < 0)
> > +		goto out;
> 
> Return error without goto because there is no any requirement
> to free the resource before.

OK.

> > +
> > +	ret = id = exynos_bus_next_id();
> > +	if (ret < 0)
> > +		goto err_node;
> > +
> > +	node = icc_node_create(id);
> > +	if (IS_ERR(node)) {
> > +		ret = PTR_ERR(node);
> > +		goto err_node;
> > +	}
> > +
> > +	bus->node = node;
> > +	node->name = dev->of_node->name;
> > +	node->data = bus;
> > +	icc_node_add(node, provider);
> > +
> > +	ret = exynos_bus_icc_connect(bus);
> > +	if (ret < 0)
> > +		goto err_connect;
> > +
> > +	ret = dev_pm_qos_add_request(bus->devfreq->dev.parent, &bus->qos_req,
> 
> Check whether this line is over 80 char.

It looks like 77 columns to me.

> 
> > +				     DEV_PM_QOS_MIN_FREQUENCY, 0);
> 
> 	Check the return value.

OK.

> 
> > +
> > +out:
> 
> Remove this goto due to not necessary.
> 
> > +	return ret;
> 
> 	return 0;

OK.

Please also note that this function as well as exynos_bus_icc_connect()
will
slightly change in v3 due to the changes regarding DT properties.

> 
> > +
> > +err_connect:
> > +	icc_node_del(node);
> > +	icc_node_destroy(id);
> > +err_node:
> > +	icc_provider_del(provider);
> > +
> > +	return ret;
> > +}
> > +
> >  static int exynos_bus_probe(struct platform_device *pdev)
> >  {
> >  	struct device *dev = &pdev->dev;
> > @@ -415,6 +560,14 @@ static int exynos_bus_probe(struct platform_device *pdev)
> >  	if (ret < 0)
> >  		goto err;
> >  
> > +	/*
> > +	 * Initialize interconnect provider. A return value of -ENOTSUPP means
> > +	 * that CONFIG_INTERCONNECT is disabled.
> > +	 */
> > +	ret = exynos_bus_icc_init(bus);
> > +	if (ret < 0 && ret != -ENOTSUPP)
> > +		goto err;
> 
> Print error message.
> 	dev_err(dev, "failed to initialize the interconnect provider");

OK.

> 
> > +
> >  	max_state = bus->devfreq->profile->max_state;
> >  	min_freq = (bus->devfreq->profile->freq_table[0] / 1000);
> >  	max_freq = (bus->devfreq->profile->freq_table[max_state - 1] / 1000);
> > 
> 
> 

Best regards,

Chanwoo Choi Dec. 18, 2019, 10:39 a.m. UTC | #7

Hi,

2019년 12월 18일 (수) 오후 7:19, Artur Świgoń <[e-mail]>님이 작성:
>
> Hi,
>
> Thank you for the review.
>
> On Mon, 2019-12-16 at 09:44 +0900, Chanwoo Choi wrote:
> > Hi,
> >
> > On 9/19/19 11:22 PM, Artur Świgoń wrote:
> > > From: Artur Świgoń <[e-mail]>
> > >
> > > This patch adds interconnect functionality to the exynos-bus devfreq
> > > driver.
> > >
> > > The SoC topology is a graph (or, more specifically, a tree) and most of
> > > its edges are taken from the devfreq parent-child hierarchy (cf.
> > > Documentation/devicetree/bindings/devfreq/exynos-bus.txt). Due to
> > > unspecified relative probing order, -EPROBE_DEFER may be propagated to
> > > guarantee that a child is probed before its parent.
> > >
> > > Each bus is now an interconnect provider and an interconnect node as well
> > > (cf. Documentation/interconnect/interconnect.rst), i.e. every bus registers
> > > itself as a node. Node IDs are not hardcoded but rather assigned at
> > > runtime, in probing order (subject to the above-mentioned exception
> > > regarding relative order). This approach allows for using this driver with
> > > various Exynos SoCs.
> > >
> > > Frequencies requested via the interconnect API for a given node are
> > > propagated to devfreq using dev_pm_qos_update_request(). Please note that
> > > it is not an error when CONFIG_INTERCONNECT is 'n', in which case all
> > > interconnect API functions are no-op.
> > >
> > > Signed-off-by: Artur Świgoń <[e-mail]>
> > > ---
> > >  drivers/devfreq/exynos-bus.c | 153 +++++++++++++++++++++++++++++++++++
> > >  1 file changed, 153 insertions(+)
> > >
> > > diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
> > > index 8d44810cac69..e0232202720d 100644
> > > --- a/drivers/devfreq/exynos-bus.c
> > > +++ b/drivers/devfreq/exynos-bus.c
> > > @@ -14,14 +14,19 @@
> > >  #include <linux/devfreq-event.h>
> > >  #include <linux/device.h>
> > >  #include <linux/export.h>
> > > +#include <linux/idr.h>
> > > +#include <linux/interconnect-provider.h>
> > >  #include <linux/module.h>
> > >  #include <linux/of.h>
> > >  #include <linux/pm_opp.h>
> > > +#include <linux/pm_qos.h>
> > >  #include <linux/platform_device.h>
> > >  #include <linux/regulator/consumer.h>
> > >
> > >  #define DEFAULT_SATURATION_RATIO   40
> > >
> > > +#define icc_units_to_khz(x) ((x) / 8)
> >
> > icc_units_to_khz() -> kpbs_to_khz()
>
> OK
>
> > > +
> > >  struct exynos_bus {
> > >     struct device *dev;
> > >
> > > @@ -35,6 +40,12 @@ struct exynos_bus {
> > >     struct opp_table *opp_table;
> > >     struct clk *clk;
> > >     unsigned int ratio;
> > > +
> > > +   /* One provider per bus, one node per provider */
> > > +   struct icc_provider provider;
> > > +   struct icc_node *node;
> > > +
> > > +   struct dev_pm_qos_request qos_req;
> > >  };
> > >
> > >  /*
> > > @@ -59,6 +70,13 @@ exynos_bus_ops_edev(enable_edev);
> > >  exynos_bus_ops_edev(disable_edev);
> > >  exynos_bus_ops_edev(set_event);
> > >
> > > +static int exynos_bus_next_id(void)
> > > +{
> > > +   static DEFINE_IDA(exynos_bus_icc_ida);
> > > +
> > > +   return ida_alloc(&exynos_bus_icc_ida, GFP_KERNEL);
> > > +}
> > > +
> > >  static int exynos_bus_get_event(struct exynos_bus *bus,
> > >                             struct devfreq_event_data *edata)
> > >  {
> > > @@ -171,6 +189,38 @@ static void exynos_bus_passive_exit(struct device *dev)
> > >     clk_disable_unprepare(bus->clk);
> > >  }
> > >
> > > +static int exynos_bus_icc_set(struct icc_node *src, struct icc_node *dst)
> > > +{
> > > +   struct exynos_bus *src_bus = src->data, *dst_bus = dst->data;
> > > +   s32 src_freq = icc_units_to_khz(src->avg_bw);
> > > +   s32 dst_freq = icc_units_to_khz(dst->avg_bw);
> > > +
> > > +   dev_pm_qos_update_request(&src_bus->qos_req, src_freq);
> >
> > Have to check the return value.
> > If return error, show the waring with dev_warn.
>
> OK, I will change it to:
>
> ret = dev_pm_qos_update_request(&src_bus->qos_req, src_freq);
> if (ret < 0) {
>         dev_warn(src_bus->dev, "failed to update PM QoS request");
>         return ret;

If you return right after, better to use dev_err.
If you use dev_warn, just show the warning message without return.

> }
>
> > > +   dev_pm_qos_update_request(&dst_bus->qos_req, dst_freq);
> >
> > ditto.
>
> OK (same as above).

ditto.

>
> > > +
> > > +   return 0;
> > > +}
> > > +
> > > +static int exynos_bus_icc_aggregate(struct icc_node *node, u32 tag, u32 avg_bw,
> > > +                               u32 peak_bw, u32 *agg_avg, u32 *agg_peak)
> > > +{
> > > +   *agg_avg += avg_bw;
> > > +   *agg_peak = max(*agg_peak, peak_bw);
> > > +
> > > +   return 0;
> > > +}
> > > +
> > > +static struct icc_node *exynos_bus_icc_xlate(struct of_phandle_args *spec,
> > > +                                        void *data)
> > > +{
> > > +   struct exynos_bus *bus = data;
> > > +
> > > +   if (spec->np != bus->dev->of_node)
> > > +           return ERR_PTR(-EINVAL);
> > > +
> > > +   return bus->node;
> > > +}
> > > +
> > >  static int exynos_bus_parent_parse_of(struct device_node *np,
> > >                                     struct exynos_bus *bus)
> > >  {
> > > @@ -366,6 +416,101 @@ static int exynos_bus_profile_init_passive(struct exynos_bus *bus,
> > >     return 0;
> > >  }
> > >
> > > +static int exynos_bus_icc_connect(struct exynos_bus *bus)
> > > +{
> > > +   struct device_node *np = bus->dev->of_node;
> > > +   struct devfreq *parent_devfreq;
> > > +   struct icc_node *parent_node = NULL;
> > > +   struct of_phandle_args args;
> > > +   int ret = 0;
> > > +
> > > +   parent_devfreq = devfreq_get_devfreq_by_phandle(bus->dev, 0);
> > > +   if (!IS_ERR(parent_devfreq)) {
> > > +           struct exynos_bus *parent_bus;
> > > +
> > > +           parent_bus = dev_get_drvdata(parent_devfreq->dev.parent);
> > > +           parent_node = parent_bus->node;
> > > +   } else {
> > > +           /* Look for parent in DT */
> > > +           int num = of_count_phandle_with_args(np, "parent",
> > > +                                                "#interconnect-cells");
> > > +           if (num != 1)
> > > +                   goto out; /* 'parent' is optional */
> > > +
> > > +           ret = of_parse_phandle_with_args(np, "parent",
> > > +                                            "#interconnect-cells",
> > > +                                            0, &args);
> >
> >
> > Actually, I agree your approach. I think that it is very useful
> > and necessary to guarantee the PM QoS requirements between devices.
> >
> > But,
> > As I already commented, I'm not sure that the "parent" property
> > is proper for only this driver. If possible, you better to get
> > the parent phandle through other way like OF graph.
> >
> > If you suggest the standard way to make the tree between
> > the exynos-bus, I'll agree.
>
> As I commented in the answer to patch 08, I will use the
> 'exynos,interconnect-parent-node' property for bus_display,
> bus_leftbus and bus_dmc.

OK.

>
> > Also, for interconnect path, you have to add the connection
> > between 'bus_display' and 'bus_leftbus' regardless
> > of the existing 'devfreq' property.
> > - bus_display - bus_leftbus - bus_dmc
> >
> > > +           if (ret < 0)
> > > +                   goto out;
> > > +
> > > +           of_node_put(args.np);
> > > +
> > > +           parent_node = of_icc_get_from_provider(&args);
> > > +           if (IS_ERR(parent_node)) {
> > > +                   /* May be -EPROBE_DEFER */
> > > +                   ret = PTR_ERR(parent_node);
> > > +                   goto out;
> > > +           }
> > > +   }
> > > +
> > > +   ret = icc_link_create(bus->node, parent_node->id);
> > > +
> > > +out:
> > > +   return ret;
> > > +}
> > > +
> > > +static int exynos_bus_icc_init(struct exynos_bus *bus)
> > > +{
> > > +   struct device *dev = bus->dev;
> > > +   struct icc_provider *provider = &bus->provider;
> > > +   struct icc_node *node;
> > > +   int id, ret;
> > > +
> > > +   /* Initialize the interconnect provider */
> > > +   provider->set = exynos_bus_icc_set;
> > > +   provider->aggregate = exynos_bus_icc_aggregate;
> > > +   provider->xlate = exynos_bus_icc_xlate;
> > > +   provider->dev = dev;
> > > +   provider->data = bus;
> > > +
> > > +   ret = icc_provider_add(provider);
> > > +   if (ret < 0)
> > > +           goto out;
> >
> > Return error without goto because there is no any requirement
> > to free the resource before.
>
> OK.
>
> > > +
> > > +   ret = id = exynos_bus_next_id();
> > > +   if (ret < 0)
> > > +           goto err_node;
> > > +
> > > +   node = icc_node_create(id);
> > > +   if (IS_ERR(node)) {
> > > +           ret = PTR_ERR(node);
> > > +           goto err_node;
> > > +   }
> > > +
> > > +   bus->node = node;
> > > +   node->name = dev->of_node->name;
> > > +   node->data = bus;
> > > +   icc_node_add(node, provider);
> > > +
> > > +   ret = exynos_bus_icc_connect(bus);
> > > +   if (ret < 0)
> > > +           goto err_connect;
> > > +
> > > +   ret = dev_pm_qos_add_request(bus->devfreq->dev.parent, &bus->qos_req,
> >
> > Check whether this line is over 80 char.
>
> It looks like 77 columns to me.
>
> >
> > > +                                DEV_PM_QOS_MIN_FREQUENCY, 0);
> >
> >       Check the return value.
>
> OK.
>
> >
> > > +
> > > +out:
> >
> > Remove this goto due to not necessary.
> >
> > > +   return ret;
> >
> >       return 0;
>
> OK.
>
> Please also note that this function as well as exynos_bus_icc_connect()
> will
> slightly change in v3 due to the changes regarding DT properties.
>
> >
> > > +
> > > +err_connect:
> > > +   icc_node_del(node);
> > > +   icc_node_destroy(id);
> > > +err_node:
> > > +   icc_provider_del(provider);
> > > +
> > > +   return ret;
> > > +}
> > > +
> > >  static int exynos_bus_probe(struct platform_device *pdev)
> > >  {
> > >     struct device *dev = &pdev->dev;
> > > @@ -415,6 +560,14 @@ static int exynos_bus_probe(struct platform_device *pdev)
> > >     if (ret < 0)
> > >             goto err;
> > >
> > > +   /*
> > > +    * Initialize interconnect provider. A return value of -ENOTSUPP means
> > > +    * that CONFIG_INTERCONNECT is disabled.
> > > +    */
> > > +   ret = exynos_bus_icc_init(bus);
> > > +   if (ret < 0 && ret != -ENOTSUPP)
> > > +           goto err;
> >
> > Print error message.
> >       dev_err(dev, "failed to initialize the interconnect provider");
>
> OK.
>
> >
> > > +
> > >     max_state = bus->devfreq->profile->max_state;
> > >     min_freq = (bus->devfreq->profile->freq_table[0] / 1000);
> > >     max_freq = (bus->devfreq->profile->freq_table[max_state - 1] / 1000);
> > >
> >
> >
>
> Best regards,
> --
> Artur Świgoń
> Samsung R&D Institute Poland
> Samsung Electronics
>
>

Artur Świgoń Dec. 18, 2019, 10:48 a.m. UTC | #8

On Wed, 2019-12-18 at 19:39 +0900, Chanwoo Choi wrote:
> Hi,
> 
> 2019년 12월 18일 (수) 오후 7:19, Artur Świgoń <[e-mail]>님이 작성:
> > 
> > Hi,
> > 
> > Thank you for the review.
> > 
> > On Mon, 2019-12-16 at 09:44 +0900, Chanwoo Choi wrote:
> > > Hi,
> > > 
> > > On 9/19/19 11:22 PM, Artur Świgoń wrote:
> > > > From: Artur Świgoń <[e-mail]>
> > > > 
> > > > This patch adds interconnect functionality to the exynos-bus devfreq
> > > > driver.
> > > > 
> > > > The SoC topology is a graph (or, more specifically, a tree) and most of
> > > > its edges are taken from the devfreq parent-child hierarchy (cf.
> > > > Documentation/devicetree/bindings/devfreq/exynos-bus.txt). Due to
> > > > unspecified relative probing order, -EPROBE_DEFER may be propagated to
> > > > guarantee that a child is probed before its parent.
> > > > 
> > > > Each bus is now an interconnect provider and an interconnect node as well
> > > > (cf. Documentation/interconnect/interconnect.rst), i.e. every bus registers
> > > > itself as a node. Node IDs are not hardcoded but rather assigned at
> > > > runtime, in probing order (subject to the above-mentioned exception
> > > > regarding relative order). This approach allows for using this driver with
> > > > various Exynos SoCs.
> > > > 
> > > > Frequencies requested via the interconnect API for a given node are
> > > > propagated to devfreq using dev_pm_qos_update_request(). Please note that
> > > > it is not an error when CONFIG_INTERCONNECT is 'n', in which case all
> > > > interconnect API functions are no-op.
> > > > 
> > > > Signed-off-by: Artur Świgoń <[e-mail]>
> > > > ---
> > > >  drivers/devfreq/exynos-bus.c | 153 +++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 153 insertions(+)
> > > > 
> > > > diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
> > > > index 8d44810cac69..e0232202720d 100644
> > > > --- a/drivers/devfreq/exynos-bus.c
> > > > +++ b/drivers/devfreq/exynos-bus.c
> > > > @@ -14,14 +14,19 @@
> > > >  #include <linux/devfreq-event.h>
> > > >  #include <linux/device.h>
> > > >  #include <linux/export.h>
> > > > +#include <linux/idr.h>
> > > > +#include <linux/interconnect-provider.h>
> > > >  #include <linux/module.h>
> > > >  #include <linux/of.h>
> > > >  #include <linux/pm_opp.h>
> > > > +#include <linux/pm_qos.h>
> > > >  #include <linux/platform_device.h>
> > > >  #include <linux/regulator/consumer.h>
> > > > 
> > > >  #define DEFAULT_SATURATION_RATIO   40
> > > > 
> > > > +#define icc_units_to_khz(x) ((x) / 8)
> > > 
> > > icc_units_to_khz() -> kpbs_to_khz()
> > 
> > OK
> > 
> > > > +
> > > >  struct exynos_bus {
> > > >     struct device *dev;
> > > > 
> > > > @@ -35,6 +40,12 @@ struct exynos_bus {
> > > >     struct opp_table *opp_table;
> > > >     struct clk *clk;
> > > >     unsigned int ratio;
> > > > +
> > > > +   /* One provider per bus, one node per provider */
> > > > +   struct icc_provider provider;
> > > > +   struct icc_node *node;
> > > > +
> > > > +   struct dev_pm_qos_request qos_req;
> > > >  };
> > > > 
> > > >  /*
> > > > @@ -59,6 +70,13 @@ exynos_bus_ops_edev(enable_edev);
> > > >  exynos_bus_ops_edev(disable_edev);
> > > >  exynos_bus_ops_edev(set_event);
> > > > 
> > > > +static int exynos_bus_next_id(void)
> > > > +{
> > > > +   static DEFINE_IDA(exynos_bus_icc_ida);
> > > > +
> > > > +   return ida_alloc(&exynos_bus_icc_ida, GFP_KERNEL);
> > > > +}
> > > > +
> > > >  static int exynos_bus_get_event(struct exynos_bus *bus,
> > > >                             struct devfreq_event_data *edata)
> > > >  {
> > > > @@ -171,6 +189,38 @@ static void exynos_bus_passive_exit(struct device *dev)
> > > >     clk_disable_unprepare(bus->clk);
> > > >  }
> > > > 
> > > > +static int exynos_bus_icc_set(struct icc_node *src, struct icc_node *dst)
> > > > +{
> > > > +   struct exynos_bus *src_bus = src->data, *dst_bus = dst->data;
> > > > +   s32 src_freq = icc_units_to_khz(src->avg_bw);
> > > > +   s32 dst_freq = icc_units_to_khz(dst->avg_bw);
> > > > +
> > > > +   dev_pm_qos_update_request(&src_bus->qos_req, src_freq);
> > > 
> > > Have to check the return value.
> > > If return error, show the waring with dev_warn.
> > 
> > OK, I will change it to:
> > 
> > ret = dev_pm_qos_update_request(&src_bus->qos_req, src_freq);
> > if (ret < 0) {
> >         dev_warn(src_bus->dev, "failed to update PM QoS request");
> >         return ret;
> 
> If you return right after, better to use dev_err.
> If you use dev_warn, just show the warning message without return.

OK, I will use dev_err().

> > }
> > 
> > > > +   dev_pm_qos_update_request(&dst_bus->qos_req, dst_freq);
> > > 
> > > ditto.
> > 
> > OK (same as above).
> 
> ditto.
> 
> > 
> > > > +
> > > > +   return 0;
> > > > +}
> > > > +
> > > > +static int exynos_bus_icc_aggregate(struct icc_node *node, u32 tag, u32 avg_bw,
> > > > +                               u32 peak_bw, u32 *agg_avg, u32 *agg_peak)
> > > > +{
> > > > +   *agg_avg += avg_bw;
> > > > +   *agg_peak = max(*agg_peak, peak_bw);
> > > > +
> > > > +   return 0;
> > > > +}
> > > > +
> > > > +static struct icc_node *exynos_bus_icc_xlate(struct of_phandle_args *spec,
> > > > +                                        void *data)
> > > > +{
> > > > +   struct exynos_bus *bus = data;
> > > > +
> > > > +   if (spec->np != bus->dev->of_node)
> > > > +           return ERR_PTR(-EINVAL);
> > > > +
> > > > +   return bus->node;
> > > > +}
> > > > +
> > > >  static int exynos_bus_parent_parse_of(struct device_node *np,
> > > >                                     struct exynos_bus *bus)
> > > >  {
> > > > @@ -366,6 +416,101 @@ static int exynos_bus_profile_init_passive(struct exynos_bus *bus,
> > > >     return 0;
> > > >  }
> > > > 
> > > > +static int exynos_bus_icc_connect(struct exynos_bus *bus)
> > > > +{
> > > > +   struct device_node *np = bus->dev->of_node;
> > > > +   struct devfreq *parent_devfreq;
> > > > +   struct icc_node *parent_node = NULL;
> > > > +   struct of_phandle_args args;
> > > > +   int ret = 0;
> > > > +
> > > > +   parent_devfreq = devfreq_get_devfreq_by_phandle(bus->dev, 0);
> > > > +   if (!IS_ERR(parent_devfreq)) {
> > > > +           struct exynos_bus *parent_bus;
> > > > +
> > > > +           parent_bus = dev_get_drvdata(parent_devfreq->dev.parent);
> > > > +           parent_node = parent_bus->node;
> > > > +   } else {
> > > > +           /* Look for parent in DT */
> > > > +           int num = of_count_phandle_with_args(np, "parent",
> > > > +                                                "#interconnect-cells");
> > > > +           if (num != 1)
> > > > +                   goto out; /* 'parent' is optional */
> > > > +
> > > > +           ret = of_parse_phandle_with_args(np, "parent",
> > > > +                                            "#interconnect-cells",
> > > > +                                            0, &args);
> > > 
> > > 
> > > Actually, I agree your approach. I think that it is very useful
> > > and necessary to guarantee the PM QoS requirements between devices.
> > > 
> > > But,
> > > As I already commented, I'm not sure that the "parent" property
> > > is proper for only this driver. If possible, you better to get
> > > the parent phandle through other way like OF graph.
> > > 
> > > If you suggest the standard way to make the tree between
> > > the exynos-bus, I'll agree.
> > 
> > As I commented in the answer to patch 08, I will use the
> > 'exynos,interconnect-parent-node' property for bus_display,
> > bus_leftbus and bus_dmc.
> 
> OK.
> 
> > 
> > > Also, for interconnect path, you have to add the connection
> > > between 'bus_display' and 'bus_leftbus' regardless
> > > of the existing 'devfreq' property.
> > > - bus_display - bus_leftbus - bus_dmc
> > > 
> > > > +           if (ret < 0)
> > > > +                   goto out;
> > > > +
> > > > +           of_node_put(args.np);
> > > > +
> > > > +           parent_node = of_icc_get_from_provider(&args);
> > > > +           if (IS_ERR(parent_node)) {
> > > > +                   /* May be -EPROBE_DEFER */
> > > > +                   ret = PTR_ERR(parent_node);
> > > > +                   goto out;
> > > > +           }
> > > > +   }
> > > > +
> > > > +   ret = icc_link_create(bus->node, parent_node->id);
> > > > +
> > > > +out:
> > > > +   return ret;
> > > > +}
> > > > +
> > > > +static int exynos_bus_icc_init(struct exynos_bus *bus)
> > > > +{
> > > > +   struct device *dev = bus->dev;
> > > > +   struct icc_provider *provider = &bus->provider;
> > > > +   struct icc_node *node;
> > > > +   int id, ret;
> > > > +
> > > > +   /* Initialize the interconnect provider */
> > > > +   provider->set = exynos_bus_icc_set;
> > > > +   provider->aggregate = exynos_bus_icc_aggregate;
> > > > +   provider->xlate = exynos_bus_icc_xlate;
> > > > +   provider->dev = dev;
> > > > +   provider->data = bus;
> > > > +
> > > > +   ret = icc_provider_add(provider);
> > > > +   if (ret < 0)
> > > > +           goto out;
> > > 
> > > Return error without goto because there is no any requirement
> > > to free the resource before.
> > 
> > OK.
> > 
> > > > +
> > > > +   ret = id = exynos_bus_next_id();
> > > > +   if (ret < 0)
> > > > +           goto err_node;
> > > > +
> > > > +   node = icc_node_create(id);
> > > > +   if (IS_ERR(node)) {
> > > > +           ret = PTR_ERR(node);
> > > > +           goto err_node;
> > > > +   }
> > > > +
> > > > +   bus->node = node;
> > > > +   node->name = dev->of_node->name;
> > > > +   node->data = bus;
> > > > +   icc_node_add(node, provider);
> > > > +
> > > > +   ret = exynos_bus_icc_connect(bus);
> > > > +   if (ret < 0)
> > > > +           goto err_connect;
> > > > +
> > > > +   ret = dev_pm_qos_add_request(bus->devfreq->dev.parent, &bus->qos_req,
> > > 
> > > Check whether this line is over 80 char.
> > 
> > It looks like 77 columns to me.
> > 
> > > 
> > > > +                                DEV_PM_QOS_MIN_FREQUENCY, 0);
> > > 
> > >       Check the return value.
> > 
> > OK.
> > 
> > > 
> > > > +
> > > > +out:
> > > 
> > > Remove this goto due to not necessary.
> > > 
> > > > +   return ret;
> > > 
> > >       return 0;
> > 
> > OK.
> > 
> > Please also note that this function as well as exynos_bus_icc_connect()
> > will
> > slightly change in v3 due to the changes regarding DT properties.
> > 
> > > 
> > > > +
> > > > +err_connect:
> > > > +   icc_node_del(node);
> > > > +   icc_node_destroy(id);
> > > > +err_node:
> > > > +   icc_provider_del(provider);
> > > > +
> > > > +   return ret;
> > > > +}
> > > > +
> > > >  static int exynos_bus_probe(struct platform_device *pdev)
> > > >  {
> > > >     struct device *dev = &pdev->dev;
> > > > @@ -415,6 +560,14 @@ static int exynos_bus_probe(struct platform_device *pdev)
> > > >     if (ret < 0)
> > > >             goto err;
> > > > 
> > > > +   /*
> > > > +    * Initialize interconnect provider. A return value of -ENOTSUPP means
> > > > +    * that CONFIG_INTERCONNECT is disabled.
> > > > +    */
> > > > +   ret = exynos_bus_icc_init(bus);
> > > > +   if (ret < 0 && ret != -ENOTSUPP)
> > > > +           goto err;
> > > 
> > > Print error message.
> > >       dev_err(dev, "failed to initialize the interconnect provider");
> > 
> > OK.
> > 
> > > 
> > > > +
> > > >     max_state = bus->devfreq->profile->max_state;
> > > >     min_freq = (bus->devfreq->profile->freq_table[0] / 1000);
> > > >     max_freq = (bus->devfreq->profile->freq_table[max_state - 1] / 1000);
> > > >

Chanwoo Choi Dec. 18, 2019, 11:08 a.m. UTC | #9

Hi,

If possible, I'd like to apply these patches to v5.6-rc1.
It is very useful feature to support the QoS for user device using memory bus.

2019년 12월 18일 (수) 오후 7:48, Artur Świgoń <[e-mail]>님이 작성:
>
> On Wed, 2019-12-18 at 19:39 +0900, Chanwoo Choi wrote:
> > Hi,
> >
> > 2019년 12월 18일 (수) 오후 7:19, Artur Świgoń <[e-mail]>님이 작성:
> > >
> > > Hi,
> > >
> > > Thank you for the review.
> > >
> > > On Mon, 2019-12-16 at 09:44 +0900, Chanwoo Choi wrote:
> > > > Hi,
> > > >
> > > > On 9/19/19 11:22 PM, Artur Świgoń wrote:
> > > > > From: Artur Świgoń <[e-mail]>
> > > > >
> > > > > This patch adds interconnect functionality to the exynos-bus devfreq
> > > > > driver.
> > > > >
> > > > > The SoC topology is a graph (or, more specifically, a tree) and most of
> > > > > its edges are taken from the devfreq parent-child hierarchy (cf.
> > > > > Documentation/devicetree/bindings/devfreq/exynos-bus.txt). Due to
> > > > > unspecified relative probing order, -EPROBE_DEFER may be propagated to
> > > > > guarantee that a child is probed before its parent.
> > > > >
> > > > > Each bus is now an interconnect provider and an interconnect node as well
> > > > > (cf. Documentation/interconnect/interconnect.rst), i.e. every bus registers
> > > > > itself as a node. Node IDs are not hardcoded but rather assigned at
> > > > > runtime, in probing order (subject to the above-mentioned exception
> > > > > regarding relative order). This approach allows for using this driver with
> > > > > various Exynos SoCs.
> > > > >
> > > > > Frequencies requested via the interconnect API for a given node are
> > > > > propagated to devfreq using dev_pm_qos_update_request(). Please note that
> > > > > it is not an error when CONFIG_INTERCONNECT is 'n', in which case all
> > > > > interconnect API functions are no-op.
> > > > >
> > > > > Signed-off-by: Artur Świgoń <[e-mail]>
> > > > > ---
> > > > >  drivers/devfreq/exynos-bus.c | 153 +++++++++++++++++++++++++++++++++++
> > > > >  1 file changed, 153 insertions(+)
> > > > >
> > > > > diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
> > > > > index 8d44810cac69..e0232202720d 100644
> > > > > --- a/drivers/devfreq/exynos-bus.c
> > > > > +++ b/drivers/devfreq/exynos-bus.c
> > > > > @@ -14,14 +14,19 @@
> > > > >  #include <linux/devfreq-event.h>
> > > > >  #include <linux/device.h>
> > > > >  #include <linux/export.h>
> > > > > +#include <linux/idr.h>
> > > > > +#include <linux/interconnect-provider.h>
> > > > >  #include <linux/module.h>
> > > > >  #include <linux/of.h>
> > > > >  #include <linux/pm_opp.h>
> > > > > +#include <linux/pm_qos.h>
> > > > >  #include <linux/platform_device.h>
> > > > >  #include <linux/regulator/consumer.h>
> > > > >
> > > > >  #define DEFAULT_SATURATION_RATIO   40
> > > > >
> > > > > +#define icc_units_to_khz(x) ((x) / 8)
> > > >
> > > > icc_units_to_khz() -> kpbs_to_khz()
> > >
> > > OK
> > >
> > > > > +
> > > > >  struct exynos_bus {
> > > > >     struct device *dev;
> > > > >
> > > > > @@ -35,6 +40,12 @@ struct exynos_bus {
> > > > >     struct opp_table *opp_table;
> > > > >     struct clk *clk;
> > > > >     unsigned int ratio;
> > > > > +
> > > > > +   /* One provider per bus, one node per provider */
> > > > > +   struct icc_provider provider;
> > > > > +   struct icc_node *node;
> > > > > +
> > > > > +   struct dev_pm_qos_request qos_req;
> > > > >  };
> > > > >
> > > > >  /*
> > > > > @@ -59,6 +70,13 @@ exynos_bus_ops_edev(enable_edev);
> > > > >  exynos_bus_ops_edev(disable_edev);
> > > > >  exynos_bus_ops_edev(set_event);
> > > > >
> > > > > +static int exynos_bus_next_id(void)
> > > > > +{
> > > > > +   static DEFINE_IDA(exynos_bus_icc_ida);
> > > > > +
> > > > > +   return ida_alloc(&exynos_bus_icc_ida, GFP_KERNEL);
> > > > > +}
> > > > > +
> > > > >  static int exynos_bus_get_event(struct exynos_bus *bus,
> > > > >                             struct devfreq_event_data *edata)
> > > > >  {
> > > > > @@ -171,6 +189,38 @@ static void exynos_bus_passive_exit(struct device *dev)
> > > > >     clk_disable_unprepare(bus->clk);
> > > > >  }
> > > > >
> > > > > +static int exynos_bus_icc_set(struct icc_node *src, struct icc_node *dst)
> > > > > +{
> > > > > +   struct exynos_bus *src_bus = src->data, *dst_bus = dst->data;
> > > > > +   s32 src_freq = icc_units_to_khz(src->avg_bw);
> > > > > +   s32 dst_freq = icc_units_to_khz(dst->avg_bw);
> > > > > +
> > > > > +   dev_pm_qos_update_request(&src_bus->qos_req, src_freq);
> > > >
> > > > Have to check the return value.
> > > > If return error, show the waring with dev_warn.
> > >
> > > OK, I will change it to:
> > >
> > > ret = dev_pm_qos_update_request(&src_bus->qos_req, src_freq);
> > > if (ret < 0) {
> > >         dev_warn(src_bus->dev, "failed to update PM QoS request");
> > >         return ret;
> >
> > If you return right after, better to use dev_err.
> > If you use dev_warn, just show the warning message without return.
>
> OK, I will use dev_err().
>
> > > }
> > >
> > > > > +   dev_pm_qos_update_request(&dst_bus->qos_req, dst_freq);
> > > >
> > > > ditto.
> > >
> > > OK (same as above).
> >
> > ditto.
> >
> > >
> > > > > +
> > > > > +   return 0;
> > > > > +}
> > > > > +
> > > > > +static int exynos_bus_icc_aggregate(struct icc_node *node, u32 tag, u32 avg_bw,
> > > > > +                               u32 peak_bw, u32 *agg_avg, u32 *agg_peak)
> > > > > +{
> > > > > +   *agg_avg += avg_bw;
> > > > > +   *agg_peak = max(*agg_peak, peak_bw);
> > > > > +
> > > > > +   return 0;
> > > > > +}
> > > > > +
> > > > > +static struct icc_node *exynos_bus_icc_xlate(struct of_phandle_args *spec,
> > > > > +                                        void *data)
> > > > > +{
> > > > > +   struct exynos_bus *bus = data;
> > > > > +
> > > > > +   if (spec->np != bus->dev->of_node)
> > > > > +           return ERR_PTR(-EINVAL);
> > > > > +
> > > > > +   return bus->node;
> > > > > +}
> > > > > +
> > > > >  static int exynos_bus_parent_parse_of(struct device_node *np,
> > > > >                                     struct exynos_bus *bus)
> > > > >  {
> > > > > @@ -366,6 +416,101 @@ static int exynos_bus_profile_init_passive(struct exynos_bus *bus,
> > > > >     return 0;
> > > > >  }
> > > > >
> > > > > +static int exynos_bus_icc_connect(struct exynos_bus *bus)
> > > > > +{
> > > > > +   struct device_node *np = bus->dev->of_node;
> > > > > +   struct devfreq *parent_devfreq;
> > > > > +   struct icc_node *parent_node = NULL;
> > > > > +   struct of_phandle_args args;
> > > > > +   int ret = 0;
> > > > > +
> > > > > +   parent_devfreq = devfreq_get_devfreq_by_phandle(bus->dev, 0);
> > > > > +   if (!IS_ERR(parent_devfreq)) {
> > > > > +           struct exynos_bus *parent_bus;
> > > > > +
> > > > > +           parent_bus = dev_get_drvdata(parent_devfreq->dev.parent);
> > > > > +           parent_node = parent_bus->node;
> > > > > +   } else {
> > > > > +           /* Look for parent in DT */
> > > > > +           int num = of_count_phandle_with_args(np, "parent",
> > > > > +                                                "#interconnect-cells");
> > > > > +           if (num != 1)
> > > > > +                   goto out; /* 'parent' is optional */
> > > > > +
> > > > > +           ret = of_parse_phandle_with_args(np, "parent",
> > > > > +                                            "#interconnect-cells",
> > > > > +                                            0, &args);
> > > >
> > > >
> > > > Actually, I agree your approach. I think that it is very useful
> > > > and necessary to guarantee the PM QoS requirements between devices.
> > > >
> > > > But,
> > > > As I already commented, I'm not sure that the "parent" property
> > > > is proper for only this driver. If possible, you better to get
> > > > the parent phandle through other way like OF graph.
> > > >
> > > > If you suggest the standard way to make the tree between
> > > > the exynos-bus, I'll agree.
> > >
> > > As I commented in the answer to patch 08, I will use the
> > > 'exynos,interconnect-parent-node' property for bus_display,
> > > bus_leftbus and bus_dmc.
> >
> > OK.
> >
> > >
> > > > Also, for interconnect path, you have to add the connection
> > > > between 'bus_display' and 'bus_leftbus' regardless
> > > > of the existing 'devfreq' property.
> > > > - bus_display - bus_leftbus - bus_dmc
> > > >
> > > > > +           if (ret < 0)
> > > > > +                   goto out;
> > > > > +
> > > > > +           of_node_put(args.np);
> > > > > +
> > > > > +           parent_node = of_icc_get_from_provider(&args);
> > > > > +           if (IS_ERR(parent_node)) {
> > > > > +                   /* May be -EPROBE_DEFER */
> > > > > +                   ret = PTR_ERR(parent_node);
> > > > > +                   goto out;
> > > > > +           }
> > > > > +   }
> > > > > +
> > > > > +   ret = icc_link_create(bus->node, parent_node->id);
> > > > > +
> > > > > +out:
> > > > > +   return ret;
> > > > > +}
> > > > > +
> > > > > +static int exynos_bus_icc_init(struct exynos_bus *bus)
> > > > > +{
> > > > > +   struct device *dev = bus->dev;
> > > > > +   struct icc_provider *provider = &bus->provider;
> > > > > +   struct icc_node *node;
> > > > > +   int id, ret;
> > > > > +
> > > > > +   /* Initialize the interconnect provider */
> > > > > +   provider->set = exynos_bus_icc_set;
> > > > > +   provider->aggregate = exynos_bus_icc_aggregate;
> > > > > +   provider->xlate = exynos_bus_icc_xlate;
> > > > > +   provider->dev = dev;
> > > > > +   provider->data = bus;
> > > > > +
> > > > > +   ret = icc_provider_add(provider);
> > > > > +   if (ret < 0)
> > > > > +           goto out;
> > > >
> > > > Return error without goto because there is no any requirement
> > > > to free the resource before.
> > >
> > > OK.
> > >
> > > > > +
> > > > > +   ret = id = exynos_bus_next_id();
> > > > > +   if (ret < 0)
> > > > > +           goto err_node;
> > > > > +
> > > > > +   node = icc_node_create(id);
> > > > > +   if (IS_ERR(node)) {
> > > > > +           ret = PTR_ERR(node);
> > > > > +           goto err_node;
> > > > > +   }
> > > > > +
> > > > > +   bus->node = node;
> > > > > +   node->name = dev->of_node->name;
> > > > > +   node->data = bus;
> > > > > +   icc_node_add(node, provider);
> > > > > +
> > > > > +   ret = exynos_bus_icc_connect(bus);
> > > > > +   if (ret < 0)
> > > > > +           goto err_connect;
> > > > > +
> > > > > +   ret = dev_pm_qos_add_request(bus->devfreq->dev.parent, &bus->qos_req,
> > > >
> > > > Check whether this line is over 80 char.
> > >
> > > It looks like 77 columns to me.
> > >
> > > >
> > > > > +                                DEV_PM_QOS_MIN_FREQUENCY, 0);
> > > >
> > > >       Check the return value.
> > >
> > > OK.
> > >
> > > >
> > > > > +
> > > > > +out:
> > > >
> > > > Remove this goto due to not necessary.
> > > >
> > > > > +   return ret;
> > > >
> > > >       return 0;
> > >
> > > OK.
> > >
> > > Please also note that this function as well as exynos_bus_icc_connect()
> > > will
> > > slightly change in v3 due to the changes regarding DT properties.
> > >
> > > >
> > > > > +
> > > > > +err_connect:
> > > > > +   icc_node_del(node);
> > > > > +   icc_node_destroy(id);
> > > > > +err_node:
> > > > > +   icc_provider_del(provider);
> > > > > +
> > > > > +   return ret;
> > > > > +}
> > > > > +
> > > > >  static int exynos_bus_probe(struct platform_device *pdev)
> > > > >  {
> > > > >     struct device *dev = &pdev->dev;
> > > > > @@ -415,6 +560,14 @@ static int exynos_bus_probe(struct platform_device *pdev)
> > > > >     if (ret < 0)
> > > > >             goto err;
> > > > >
> > > > > +   /*
> > > > > +    * Initialize interconnect provider. A return value of -ENOTSUPP means
> > > > > +    * that CONFIG_INTERCONNECT is disabled.
> > > > > +    */
> > > > > +   ret = exynos_bus_icc_init(bus);
> > > > > +   if (ret < 0 && ret != -ENOTSUPP)
> > > > > +           goto err;
> > > >
> > > > Print error message.
> > > >       dev_err(dev, "failed to initialize the interconnect provider");
> > >
> > > OK.
> > >
> > > >
> > > > > +
> > > > >     max_state = bus->devfreq->profile->max_state;
> > > > >     min_freq = (bus->devfreq->profile->freq_table[0] / 1000);
> > > > >     max_freq = (bus->devfreq->profile->freq_table[max_state - 1] / 1000);
> > > > >
>
> --
> Artur Świgoń
> Samsung R&D Institute Poland
> Samsung Electronics
>
>

Patch

diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
index 8d44810cac69..e0232202720d 100644
--- a/drivers/devfreq/exynos-bus.c
+++ b/drivers/devfreq/exynos-bus.c
@@ -14,14 +14,19 @@ 
 #include <linux/devfreq-event.h>
 #include <linux/device.h>
 #include <linux/export.h>
+#include <linux/idr.h>
+#include <linux/interconnect-provider.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/pm_opp.h>
+#include <linux/pm_qos.h>
 #include <linux/platform_device.h>
 #include <linux/regulator/consumer.h>
 
 #define DEFAULT_SATURATION_RATIO	40
 
+#define icc_units_to_khz(x) ((x) / 8)
+
 struct exynos_bus {
 	struct device *dev;
 
@@ -35,6 +40,12 @@  struct exynos_bus {
 	struct opp_table *opp_table;
 	struct clk *clk;
 	unsigned int ratio;
+
+	/* One provider per bus, one node per provider */
+	struct icc_provider provider;
+	struct icc_node *node;
+
+	struct dev_pm_qos_request qos_req;
 };
 
 /*
@@ -59,6 +70,13 @@  exynos_bus_ops_edev(enable_edev);
 exynos_bus_ops_edev(disable_edev);
 exynos_bus_ops_edev(set_event);
 
+static int exynos_bus_next_id(void)
+{
+	static DEFINE_IDA(exynos_bus_icc_ida);
+
+	return ida_alloc(&exynos_bus_icc_ida, GFP_KERNEL);
+}
+
 static int exynos_bus_get_event(struct exynos_bus *bus,
 				struct devfreq_event_data *edata)
 {
@@ -171,6 +189,38 @@  static void exynos_bus_passive_exit(struct device *dev)
 	clk_disable_unprepare(bus->clk);
 }
 
+static int exynos_bus_icc_set(struct icc_node *src, struct icc_node *dst)
+{
+	struct exynos_bus *src_bus = src->data, *dst_bus = dst->data;
+	s32 src_freq = icc_units_to_khz(src->avg_bw);
+	s32 dst_freq = icc_units_to_khz(dst->avg_bw);
+
+	dev_pm_qos_update_request(&src_bus->qos_req, src_freq);
+	dev_pm_qos_update_request(&dst_bus->qos_req, dst_freq);
+
+	return 0;
+}
+
+static int exynos_bus_icc_aggregate(struct icc_node *node, u32 tag, u32 avg_bw,
+				    u32 peak_bw, u32 *agg_avg, u32 *agg_peak)
+{
+	*agg_avg += avg_bw;
+	*agg_peak = max(*agg_peak, peak_bw);
+
+	return 0;
+}
+
+static struct icc_node *exynos_bus_icc_xlate(struct of_phandle_args *spec,
+					     void *data)
+{
+	struct exynos_bus *bus = data;
+
+	if (spec->np != bus->dev->of_node)
+		return ERR_PTR(-EINVAL);
+
+	return bus->node;
+}
+
 static int exynos_bus_parent_parse_of(struct device_node *np,
 					struct exynos_bus *bus)
 {
@@ -366,6 +416,101 @@  static int exynos_bus_profile_init_passive(struct exynos_bus *bus,
 	return 0;
 }
 
+static int exynos_bus_icc_connect(struct exynos_bus *bus)
+{
+	struct device_node *np = bus->dev->of_node;
+	struct devfreq *parent_devfreq;
+	struct icc_node *parent_node = NULL;
+	struct of_phandle_args args;
+	int ret = 0;
+
+	parent_devfreq = devfreq_get_devfreq_by_phandle(bus->dev, 0);
+	if (!IS_ERR(parent_devfreq)) {
+		struct exynos_bus *parent_bus;
+
+		parent_bus = dev_get_drvdata(parent_devfreq->dev.parent);
+		parent_node = parent_bus->node;
+	} else {
+		/* Look for parent in DT */
+		int num = of_count_phandle_with_args(np, "parent",
+						     "#interconnect-cells");
+		if (num != 1)
+			goto out; /* 'parent' is optional */
+
+		ret = of_parse_phandle_with_args(np, "parent",
+						 "#interconnect-cells",
+						 0, &args);
+		if (ret < 0)
+			goto out;
+
+		of_node_put(args.np);
+
+		parent_node = of_icc_get_from_provider(&args);
+		if (IS_ERR(parent_node)) {
+			/* May be -EPROBE_DEFER */
+			ret = PTR_ERR(parent_node);
+			goto out;
+		}
+	}
+
+	ret = icc_link_create(bus->node, parent_node->id);
+
+out:
+	return ret;
+}
+
+static int exynos_bus_icc_init(struct exynos_bus *bus)
+{
+	struct device *dev = bus->dev;
+	struct icc_provider *provider = &bus->provider;
+	struct icc_node *node;
+	int id, ret;
+
+	/* Initialize the interconnect provider */
+	provider->set = exynos_bus_icc_set;
+	provider->aggregate = exynos_bus_icc_aggregate;
+	provider->xlate = exynos_bus_icc_xlate;
+	provider->dev = dev;
+	provider->data = bus;
+
+	ret = icc_provider_add(provider);
+	if (ret < 0)
+		goto out;
+
+	ret = id = exynos_bus_next_id();
+	if (ret < 0)
+		goto err_node;
+
+	node = icc_node_create(id);
+	if (IS_ERR(node)) {
+		ret = PTR_ERR(node);
+		goto err_node;
+	}
+
+	bus->node = node;
+	node->name = dev->of_node->name;
+	node->data = bus;
+	icc_node_add(node, provider);
+
+	ret = exynos_bus_icc_connect(bus);
+	if (ret < 0)
+		goto err_connect;
+
+	ret = dev_pm_qos_add_request(bus->devfreq->dev.parent, &bus->qos_req,
+				     DEV_PM_QOS_MIN_FREQUENCY, 0);
+
+out:
+	return ret;
+
+err_connect:
+	icc_node_del(node);
+	icc_node_destroy(id);
+err_node:
+	icc_provider_del(provider);
+
+	return ret;
+}
+
 static int exynos_bus_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -415,6 +560,14 @@  static int exynos_bus_probe(struct platform_device *pdev)
 	if (ret < 0)
 		goto err;
 
+	/*
+	 * Initialize interconnect provider. A return value of -ENOTSUPP means
+	 * that CONFIG_INTERCONNECT is disabled.
+	 */
+	ret = exynos_bus_icc_init(bus);
+	if (ret < 0 && ret != -ENOTSUPP)
+		goto err;
+
 	max_state = bus->devfreq->profile->max_state;
 	min_freq = (bus->devfreq->profile->freq_table[0] / 1000);
 	max_freq = (bus->devfreq->profile->freq_table[max_state - 1] / 1000);