This patch adds interconnect functionality to the exynos-bus devfreq
driver.
The SoC topology is a graph (or, more specifically, a tree) and its
edges are specified using the 'exynos,interconnect-parent-node' in the
DT. Due to unspecified relative probing order, -EPROBE_DEFER may be
propagated to ensure that the parent is probed before its children.
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ń <[email protected]>
---
drivers/devfreq/exynos-bus.c | 144 +++++++++++++++++++++++++++++++++++
1 file changed, 144 insertions(+)
diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
index 9fdb188915e8..694a9581dcdb 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 kbps_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;
};
/*
@@ -205,6 +216,39 @@ 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 = kbps_to_khz(src->avg_bw);
+ s32 dst_freq = kbps_to_khz(dst->avg_bw);
+ int ret;
+
+ ret = dev_pm_qos_update_request(&src_bus->qos_req, src_freq);
+ if (ret < 0) {
+ dev_err(src_bus->dev, "failed to update PM QoS request");
+ return ret;
+ }
+
+ ret = dev_pm_qos_update_request(&dst_bus->qos_req, dst_freq);
+ if (ret < 0) {
+ dev_err(dst_bus->dev, "failed to update PM QoS request");
+ return ret;
+ }
+
+ 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)
{
@@ -419,6 +463,96 @@ static int exynos_bus_profile_init_passive(struct exynos_bus *bus,
return 0;
}
+static struct icc_node *exynos_bus_icc_get_parent(struct exynos_bus *bus)
+{
+ struct device_node *np = bus->dev->of_node;
+ struct of_phandle_args args;
+ int num, ret;
+
+ num = of_count_phandle_with_args(np, "exynos,interconnect-parent-node",
+ "#interconnect-cells");
+ if (num != 1)
+ return NULL; /* parent nodes are optional */
+
+ ret = of_parse_phandle_with_args(np, "exynos,interconnect-parent-node",
+ "#interconnect-cells", 0, &args);
+ if (ret < 0)
+ return ERR_PTR(ret);
+
+ of_node_put(args.np);
+
+ return of_icc_get_from_provider(&args);
+}
+
+static int exynos_bus_icc_init(struct exynos_bus *bus)
+{
+ static DEFINE_IDA(ida);
+
+ struct device *dev = bus->dev;
+ struct icc_provider *provider = &bus->provider;
+ struct icc_node *node, *parent_node;
+ int id, ret;
+
+ /* Initialize the interconnect provider */
+ provider->set = exynos_bus_icc_set;
+ provider->aggregate = icc_std_aggregate;
+ provider->xlate = exynos_bus_icc_xlate;
+ provider->dev = dev;
+ provider->inter_set = true;
+ provider->data = bus;
+
+ ret = icc_provider_add(provider);
+ if (ret < 0)
+ return ret;
+
+ ret = id = ida_alloc(&ida, GFP_KERNEL);
+ if (ret < 0)
+ goto err_id;
+
+ 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);
+
+ parent_node = exynos_bus_icc_get_parent(bus);
+ if (IS_ERR(parent_node)) {
+ ret = PTR_ERR(parent_node);
+ goto err_parent;
+ }
+
+ if (parent_node) {
+ ret = icc_link_create(node, parent_node->id);
+ if (ret < 0)
+ goto err_parent;
+ }
+
+ ret = dev_pm_qos_add_request(bus->devfreq->dev.parent, &bus->qos_req,
+ DEV_PM_QOS_MIN_FREQUENCY, 0);
+ if (ret < 0)
+ goto err_request;
+
+ return 0;
+
+err_request:
+ if (parent_node)
+ icc_link_destroy(node, parent_node);
+err_parent:
+ icc_node_del(node);
+ icc_node_destroy(id);
+err_node:
+ ida_free(&ida, id);
+err_id:
+ icc_provider_del(provider);
+
+ return ret;
+}
+
static int exynos_bus_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
@@ -468,6 +602,16 @@ 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) {
+ dev_err(dev, "failed to initialize the interconnect provider");
+ 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);
--
2.17.1
Hi,
On Fri, Dec 20, 2019 at 9:02 PM Artur Świgoń <[email protected]> wrote:
>
> This patch adds interconnect functionality to the exynos-bus devfreq
> driver.
>
> The SoC topology is a graph (or, more specifically, a tree) and its
> edges are specified using the 'exynos,interconnect-parent-node' in the
> DT. Due to unspecified relative probing order, -EPROBE_DEFER may be
> propagated to ensure that the parent is probed before its children.
>
> 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ń <[email protected]>
> ---
> drivers/devfreq/exynos-bus.c | 144 +++++++++++++++++++++++++++++++++++
> 1 file changed, 144 insertions(+)
>
> diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
> index 9fdb188915e8..694a9581dcdb 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 kbps_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;
> };
>
> /*
> @@ -205,6 +216,39 @@ 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 = kbps_to_khz(src->avg_bw);
> + s32 dst_freq = kbps_to_khz(dst->avg_bw);
> + int ret;
> +
> + ret = dev_pm_qos_update_request(&src_bus->qos_req, src_freq);
> + if (ret < 0) {
> + dev_err(src_bus->dev, "failed to update PM QoS request");
To catch the correct error point, better to add 'src node' to error message
as following:
dev_err(src_bus->dev, "failed to update PM QoS of %s\n",
dev_name(src_bus->dev.parent))
> + return ret;
> + }
> +
> + ret = dev_pm_qos_update_request(&dst_bus->qos_req, dst_freq);
> + if (ret < 0) {
> + dev_err(dst_bus->dev, "failed to update PM QoS request");
ditto.
dev_err(src_bus->dev, "failed to update PM QoS of %s\n",
dev_name(dst_bus->dev.parent))
> + return ret;
> + }
> +
> + 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)
> {
> @@ -419,6 +463,96 @@ static int exynos_bus_profile_init_passive(struct exynos_bus *bus,
> return 0;
> }
>
> +static struct icc_node *exynos_bus_icc_get_parent(struct exynos_bus *bus)
> +{
> + struct device_node *np = bus->dev->of_node;
> + struct of_phandle_args args;
> + int num, ret;
> +
> + num = of_count_phandle_with_args(np, "exynos,interconnect-parent-node",
> + "#interconnect-cells");
> + if (num != 1)
> + return NULL; /* parent nodes are optional */
You better to add the comment before calling exynos_bus_icc_get_parent
and remove '/* parent nodes are optional */'. Actually, it is not enough
to understand the role of 'interconnect-parent-node' with '/* parent
nodes are optional */'.
And I add my opinion about this comment below.
And I expect that you will add the description and example for
'exynos,interconnect-parent-node' on exynos-bus dt-binding document.
> +
> + ret = of_parse_phandle_with_args(np, "exynos,interconnect-parent-node",
> + "#interconnect-cells", 0, &args);
> + if (ret < 0)
> + return ERR_PTR(ret);
> +
> + of_node_put(args.np);
> +
> + return of_icc_get_from_provider(&args);
> +}
> +
> +static int exynos_bus_icc_init(struct exynos_bus *bus)
> +{
> + static DEFINE_IDA(ida);
> +
> + struct device *dev = bus->dev;
> + struct icc_provider *provider = &bus->provider;
> + struct icc_node *node, *parent_node;
> + int id, ret;
> +
> + /* Initialize the interconnect provider */
> + provider->set = exynos_bus_icc_set;
> + provider->aggregate = icc_std_aggregate;
> + provider->xlate = exynos_bus_icc_xlate;
> + provider->dev = dev;
> + provider->inter_set = true;
> + provider->data = bus;
> +
> + ret = icc_provider_add(provider);
> + if (ret < 0)
> + return ret;
> +
> + ret = id = ida_alloc(&ida, GFP_KERNEL);
> + if (ret < 0)
> + goto err_id;
> +
> + 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);
> +
Better to add the following comment. If you add following comment
before calling exynos_bus_icc_get_parent, don't need to add the
same comment into exynos_bus_icc.
/* If interconnect parent node is not existing, don't use
interconnect feature */
> + parent_node = exynos_bus_icc_get_parent(bus);
> + if (IS_ERR(parent_node)) {
> + ret = PTR_ERR(parent_node);
> + goto err_parent;
> + }
> +
> + if (parent_node) {
Better to change this if statement as following:
else if (parent_node)
> + ret = icc_link_create(node, parent_node->id);
> + if (ret < 0)
> + goto err_parent;
> + }
> +
> + ret = dev_pm_qos_add_request(bus->devfreq->dev.parent, &bus->qos_req,
> + DEV_PM_QOS_MIN_FREQUENCY, 0);
Is it necessary if interconnect-parent-node is not existing?
> + if (ret < 0)
> + goto err_request;
> +
> + return 0;
> +
> +err_request:
> + if (parent_node)
> + icc_link_destroy(node, parent_node);
> +err_parent:
> + icc_node_del(node);
> + icc_node_destroy(id);
> +err_node:
> + ida_free(&ida, id);
> +err_id:
> + icc_provider_del(provider);
> +
> + return ret;
> +}
> +
> static int exynos_bus_probe(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> @@ -468,6 +602,16 @@ 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) {
> + dev_err(dev, "failed to initialize the interconnect provider");
> + 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);
> --
> 2.17.1
>
--
Best Regards,
Chanwoo Choi
On Sun, Dec 22, 2019 at 4:53 AM Chanwoo Choi <[email protected]> wrote:
>
> Hi,
>
> On Fri, Dec 20, 2019 at 9:02 PM Artur Świgoń <[email protected]> wrote:
> >
> > This patch adds interconnect functionality to the exynos-bus devfreq
> > driver.
> >
> > The SoC topology is a graph (or, more specifically, a tree) and its
> > edges are specified using the 'exynos,interconnect-parent-node' in the
> > DT. Due to unspecified relative probing order, -EPROBE_DEFER may be
> > propagated to ensure that the parent is probed before its children.
> >
> > 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ń <[email protected]>
> > ---
> > drivers/devfreq/exynos-bus.c | 144 +++++++++++++++++++++++++++++++++++
> > 1 file changed, 144 insertions(+)
> >
> > diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
> > index 9fdb188915e8..694a9581dcdb 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 kbps_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;
> > };
> >
> > /*
> > @@ -205,6 +216,39 @@ 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 = kbps_to_khz(src->avg_bw);
> > + s32 dst_freq = kbps_to_khz(dst->avg_bw);
> > + int ret;
> > +
> > + ret = dev_pm_qos_update_request(&src_bus->qos_req, src_freq);
> > + if (ret < 0) {
> > + dev_err(src_bus->dev, "failed to update PM QoS request");
>
> To catch the correct error point, better to add 'src node' to error message
> as following:
>
> dev_err(src_bus->dev, "failed to update PM QoS of %s\n",
> dev_name(src_bus->dev.parent))
>
> > + return ret;
> > + }
> > +
> > + ret = dev_pm_qos_update_request(&dst_bus->qos_req, dst_freq);
> > + if (ret < 0) {
> > + dev_err(dst_bus->dev, "failed to update PM QoS request");
>
> ditto.
>
> dev_err(src_bus->dev, "failed to update PM QoS of %s\n",
> dev_name(dst_bus->dev.parent))
>
>
> > + return ret;
> > + }
> > +
> > + 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)
> > {
> > @@ -419,6 +463,96 @@ static int exynos_bus_profile_init_passive(struct exynos_bus *bus,
> > return 0;
> > }
> >
> > +static struct icc_node *exynos_bus_icc_get_parent(struct exynos_bus *bus)
> > +{
> > + struct device_node *np = bus->dev->of_node;
> > + struct of_phandle_args args;
> > + int num, ret;
> > +
> > + num = of_count_phandle_with_args(np, "exynos,interconnect-parent-node",
> > + "#interconnect-cells");
> > + if (num != 1)
> > + return NULL; /* parent nodes are optional */
>
> You better to add the comment before calling exynos_bus_icc_get_parent
> and remove '/* parent nodes are optional */'. Actually, it is not enough
> to understand the role of 'interconnect-parent-node' with '/* parent
> nodes are optional */'.
> And I add my opinion about this comment below.
>
> And I expect that you will add the description and example for
> 'exynos,interconnect-parent-node' on exynos-bus dt-binding document.
>
> > +
> > + ret = of_parse_phandle_with_args(np, "exynos,interconnect-parent-node",
> > + "#interconnect-cells", 0, &args);
> > + if (ret < 0)
> > + return ERR_PTR(ret);
> > +
> > + of_node_put(args.np);
> > +
> > + return of_icc_get_from_provider(&args);
> > +}
> > +
> > +static int exynos_bus_icc_init(struct exynos_bus *bus)
> > +{
> > + static DEFINE_IDA(ida);
> > +
> > + struct device *dev = bus->dev;
> > + struct icc_provider *provider = &bus->provider;
> > + struct icc_node *node, *parent_node;
> > + int id, ret;
> > +
> > + /* Initialize the interconnect provider */
> > + provider->set = exynos_bus_icc_set;
> > + provider->aggregate = icc_std_aggregate;
> > + provider->xlate = exynos_bus_icc_xlate;
> > + provider->dev = dev;
> > + provider->inter_set = true;
> > + provider->data = bus;
> > +
> > + ret = icc_provider_add(provider);
> > + if (ret < 0)
> > + return ret;
> > +
> > + ret = id = ida_alloc(&ida, GFP_KERNEL);
> > + if (ret < 0)
> > + goto err_id;
> > +
> > + 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);
> > +
>
> Better to add the following comment. If you add following comment
> before calling exynos_bus_icc_get_parent, don't need to add the
> same comment into exynos_bus_icc.
> /* If interconnect parent node is not existing, don't use
> interconnect feature */
>
> > + parent_node = exynos_bus_icc_get_parent(bus);
> > + if (IS_ERR(parent_node)) {
> > + ret = PTR_ERR(parent_node);
> > + goto err_parent;
> > + }
> > +
> > + if (parent_node) {
>
> Better to change this if statement as following:
> else if (parent_node)
>
>
> > + ret = icc_link_create(node, parent_node->id);
> > + if (ret < 0)
> > + goto err_parent;
> > + }
> > +
> > + ret = dev_pm_qos_add_request(bus->devfreq->dev.parent, &bus->qos_req,
> > + DEV_PM_QOS_MIN_FREQUENCY, 0);
>
> Is it necessary if interconnect-parent-node is not existing?
I realized it is necessary for final destination node.
>
>
> > + if (ret < 0)
> > + goto err_request;
> > +
> > + return 0;
> > +
> > +err_request:
> > + if (parent_node)
> > + icc_link_destroy(node, parent_node);
> > +err_parent:
> > + icc_node_del(node);
> > + icc_node_destroy(id);
> > +err_node:
> > + ida_free(&ida, id);
> > +err_id:
> > + icc_provider_del(provider);
> > +
> > + return ret;
> > +}
> > +
> > static int exynos_bus_probe(struct platform_device *pdev)
> > {
> > struct device *dev = &pdev->dev;
> > @@ -468,6 +602,16 @@ 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) {
> > + dev_err(dev, "failed to initialize the interconnect provider");
> > + 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);
> > --
> > 2.17.1
> >
>
>
> --
> Best Regards,
> Chanwoo Choi
--
Best Regards,
Chanwoo Choi
Hi Artur,
On 12/20/19 13:56, Artur Świgoń wrote:
> This patch adds interconnect functionality to the exynos-bus devfreq
> driver.
>
> The SoC topology is a graph (or, more specifically, a tree) and its
> edges are specified using the 'exynos,interconnect-parent-node' in the
> DT. Due to unspecified relative probing order, -EPROBE_DEFER may be
> propagated to ensure that the parent is probed before its children.
>
> 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
Just to note that usually the provider consists of multiple nodes and each node
represents a single master or slave port on the AXI bus for example. I am not
sure whether this represents correctly the Exynos hardware, so it's up to
you.
> runtime, in probing order (subject to the above-mentioned exception
> regarding relative order). This approach allows for using this driver with
> various Exynos SoCs.
This sounds good. I am wondering whether such dynamic probing would be useful
for other platforms too. Then maybe it would make sense to even have a common DT
property, but we will see.
Is this going to be used only together with devfreq?
> 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.
How about the case where CONFIG_INTERCONNECT=m. Looks like the build will fail
if CONFIG_ARM_EXYNOS_BUS_DEVFREQ=y, so this dependency should be expressed in
Kconfig.
Thanks,
Georgi
>
> Signed-off-by: Artur Świgoń <[email protected]>
> ---
> drivers/devfreq/exynos-bus.c | 144 +++++++++++++++++++++++++++++++++++
> 1 file changed, 144 insertions(+)
>
> diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
> index 9fdb188915e8..694a9581dcdb 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 kbps_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;
> };
>
> /*
> @@ -205,6 +216,39 @@ 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 = kbps_to_khz(src->avg_bw);
> + s32 dst_freq = kbps_to_khz(dst->avg_bw);
> + int ret;
> +
> + ret = dev_pm_qos_update_request(&src_bus->qos_req, src_freq);
> + if (ret < 0) {
> + dev_err(src_bus->dev, "failed to update PM QoS request");
> + return ret;
> + }
> +
> + ret = dev_pm_qos_update_request(&dst_bus->qos_req, dst_freq);
> + if (ret < 0) {
> + dev_err(dst_bus->dev, "failed to update PM QoS request");
> + return ret;
> + }
> +
> + 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)
> {
> @@ -419,6 +463,96 @@ static int exynos_bus_profile_init_passive(struct exynos_bus *bus,
> return 0;
> }
>
> +static struct icc_node *exynos_bus_icc_get_parent(struct exynos_bus *bus)
> +{
> + struct device_node *np = bus->dev->of_node;
> + struct of_phandle_args args;
> + int num, ret;
> +
> + num = of_count_phandle_with_args(np, "exynos,interconnect-parent-node",
> + "#interconnect-cells");
> + if (num != 1)
> + return NULL; /* parent nodes are optional */
> +
> + ret = of_parse_phandle_with_args(np, "exynos,interconnect-parent-node",
> + "#interconnect-cells", 0, &args);
> + if (ret < 0)
> + return ERR_PTR(ret);
> +
> + of_node_put(args.np);
> +
> + return of_icc_get_from_provider(&args);
> +}
> +
> +static int exynos_bus_icc_init(struct exynos_bus *bus)
> +{
> + static DEFINE_IDA(ida);
> +
> + struct device *dev = bus->dev;
> + struct icc_provider *provider = &bus->provider;
> + struct icc_node *node, *parent_node;
> + int id, ret;
> +
> + /* Initialize the interconnect provider */
> + provider->set = exynos_bus_icc_set;
> + provider->aggregate = icc_std_aggregate;
> + provider->xlate = exynos_bus_icc_xlate;
> + provider->dev = dev;
> + provider->inter_set = true;
> + provider->data = bus;
> +
> + ret = icc_provider_add(provider);
> + if (ret < 0)
> + return ret;
> +
> + ret = id = ida_alloc(&ida, GFP_KERNEL);
> + if (ret < 0)
> + goto err_id;
> +
> + 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);
> +
> + parent_node = exynos_bus_icc_get_parent(bus);
> + if (IS_ERR(parent_node)) {
> + ret = PTR_ERR(parent_node);
> + goto err_parent;
> + }
> +
> + if (parent_node) {
> + ret = icc_link_create(node, parent_node->id);
> + if (ret < 0)
> + goto err_parent;
> + }
> +
> + ret = dev_pm_qos_add_request(bus->devfreq->dev.parent, &bus->qos_req,
> + DEV_PM_QOS_MIN_FREQUENCY, 0);
> + if (ret < 0)
> + goto err_request;
> +
> + return 0;
> +
> +err_request:
> + if (parent_node)
> + icc_link_destroy(node, parent_node);
> +err_parent:
> + icc_node_del(node);
> + icc_node_destroy(id);
> +err_node:
> + ida_free(&ida, id);
> +err_id:
> + icc_provider_del(provider);
> +
> + return ret;
> +}
> +
> static int exynos_bus_probe(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> @@ -468,6 +602,16 @@ 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) {
> + dev_err(dev, "failed to initialize the interconnect provider");
> + 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);
>
Hi Georgi,
On Wed, 2020-01-22 at 19:02 +0200, Georgi Djakov wrote:
> Hi Artur,
>
> On 12/20/19 13:56, Artur Świgoń wrote:
> > This patch adds interconnect functionality to the exynos-bus devfreq
> > driver.
> >
> > The SoC topology is a graph (or, more specifically, a tree) and its
> > edges are specified using the 'exynos,interconnect-parent-node' in the
> > DT. Due to unspecified relative probing order, -EPROBE_DEFER may be
> > propagated to ensure that the parent is probed before its children.
> >
> > 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
>
> Just to note that usually the provider consists of multiple nodes and each node
> represents a single master or slave port on the AXI bus for example. I am not
> sure whether this represents correctly the Exynos hardware, so it's up to
> you.
>
> > runtime, in probing order (subject to the above-mentioned exception
> > regarding relative order). This approach allows for using this driver with
> > various Exynos SoCs.
>
> This sounds good. I am wondering whether such dynamic probing would be useful
> for other platforms too. Then maybe it would make sense to even have a common DT
> property, but we will see.
>
> Is this going to be used only together with devfreq?
Yes, this functions solely as an extension to devfreq, hence the slightly
unusual architecture (one icc_provider/icc_node per devfreq).
(Compared to a singleton icc_provider, this approach yields less code with
a very simple xlate()).
With exactly one icc_node for every devfreq device, I think I will actually
reuse the devfreq ID (as seen in the device name, e.g. the "3" in "devfreq3")
for the node ID. The devfreq framework already does the dynamic numbering
thing that I do in this patch using IDR.
> > 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.
>
> How about the case where CONFIG_INTERCONNECT=m. Looks like the build will fail
> if CONFIG_ARM_EXYNOS_BUS_DEVFREQ=y, so this dependency should be expressed in
> Kconfig.
I think adding:
depends on INTERCONNECT || !INTERCONNECT
under ARM_EXYNOS_BUS_DEVFREQ does the trick.
> >
> > Signed-off-by: Artur Świgoń <[email protected]>
> > ---
> > drivers/devfreq/exynos-bus.c | 144 +++++++++++++++++++++++++++++++++++
> > 1 file changed, 144 insertions(+)
> >
> > diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
> > index 9fdb188915e8..694a9581dcdb 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 kbps_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;
> > };
> >
> > /*
> > @@ -205,6 +216,39 @@ 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 = kbps_to_khz(src->avg_bw);
> > + s32 dst_freq = kbps_to_khz(dst->avg_bw);
> > + int ret;
> > +
> > + ret = dev_pm_qos_update_request(&src_bus->qos_req, src_freq);
> > + if (ret < 0) {
> > + dev_err(src_bus->dev, "failed to update PM QoS request");
> > + return ret;
> > + }
> > +
> > + ret = dev_pm_qos_update_request(&dst_bus->qos_req, dst_freq);
> > + if (ret < 0) {
> > + dev_err(dst_bus->dev, "failed to update PM QoS request");
> > + return ret;
> > + }
> > +
> > + 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)
> > {
> > @@ -419,6 +463,96 @@ static int exynos_bus_profile_init_passive(struct exynos_bus *bus,
> > return 0;
> > }
> >
> > +static struct icc_node *exynos_bus_icc_get_parent(struct exynos_bus *bus)
> > +{
> > + struct device_node *np = bus->dev->of_node;
> > + struct of_phandle_args args;
> > + int num, ret;
> > +
> > + num = of_count_phandle_with_args(np, "exynos,interconnect-parent-node",
> > + "#interconnect-cells");
> > + if (num != 1)
> > + return NULL; /* parent nodes are optional */
> > +
> > + ret = of_parse_phandle_with_args(np, "exynos,interconnect-parent-node",
> > + "#interconnect-cells", 0, &args);
> > + if (ret < 0)
> > + return ERR_PTR(ret);
> > +
> > + of_node_put(args.np);
> > +
> > + return of_icc_get_from_provider(&args);
> > +}
> > +
> > +static int exynos_bus_icc_init(struct exynos_bus *bus)
> > +{
> > + static DEFINE_IDA(ida);
> > +
> > + struct device *dev = bus->dev;
> > + struct icc_provider *provider = &bus->provider;
> > + struct icc_node *node, *parent_node;
> > + int id, ret;
> > +
> > + /* Initialize the interconnect provider */
> > + provider->set = exynos_bus_icc_set;
> > + provider->aggregate = icc_std_aggregate;
> > + provider->xlate = exynos_bus_icc_xlate;
> > + provider->dev = dev;
> > + provider->inter_set = true;
> > + provider->data = bus;
> > +
> > + ret = icc_provider_add(provider);
> > + if (ret < 0)
> > + return ret;
> > +
> > + ret = id = ida_alloc(&ida, GFP_KERNEL);
> > + if (ret < 0)
> > + goto err_id;
> > +
> > + 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);
> > +
> > + parent_node = exynos_bus_icc_get_parent(bus);
> > + if (IS_ERR(parent_node)) {
> > + ret = PTR_ERR(parent_node);
> > + goto err_parent;
> > + }
> > +
> > + if (parent_node) {
> > + ret = icc_link_create(node, parent_node->id);
> > + if (ret < 0)
> > + goto err_parent;
> > + }
> > +
> > + ret = dev_pm_qos_add_request(bus->devfreq->dev.parent, &bus->qos_req,
> > + DEV_PM_QOS_MIN_FREQUENCY, 0);
> > + if (ret < 0)
> > + goto err_request;
> > +
> > + return 0;
> > +
> > +err_request:
> > + if (parent_node)
> > + icc_link_destroy(node, parent_node);
> > +err_parent:
> > + icc_node_del(node);
> > + icc_node_destroy(id);
> > +err_node:
> > + ida_free(&ida, id);
> > +err_id:
> > + icc_provider_del(provider);
> > +
> > + return ret;
> > +}
> > +
> > static int exynos_bus_probe(struct platform_device *pdev)
> > {
> > struct device *dev = &pdev->dev;
> > @@ -468,6 +602,16 @@ 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) {
> > + dev_err(dev, "failed to initialize the interconnect provider");
> > + 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ń
Samsung R&D Institute Poland
Samsung Electronics
Hi Artur,
On 1/24/20 13:22, Artur Świgoń wrote:
> Hi Georgi,
>
> On Wed, 2020-01-22 at 19:02 +0200, Georgi Djakov wrote:
>> Hi Artur,
>>
>> On 12/20/19 13:56, Artur Świgoń wrote:
>>> This patch adds interconnect functionality to the exynos-bus devfreq
>>> driver.
>>>
>>> The SoC topology is a graph (or, more specifically, a tree) and its
>>> edges are specified using the 'exynos,interconnect-parent-node' in the
>>> DT. Due to unspecified relative probing order, -EPROBE_DEFER may be
>>> propagated to ensure that the parent is probed before its children.
>>>
>>> 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
>>
>> Just to note that usually the provider consists of multiple nodes and each node
>> represents a single master or slave port on the AXI bus for example. I am not
>> sure whether this represents correctly the Exynos hardware, so it's up to
>> you.
>>
>>> runtime, in probing order (subject to the above-mentioned exception
>>> regarding relative order). This approach allows for using this driver with
>>> various Exynos SoCs.
>>
>> This sounds good. I am wondering whether such dynamic probing would be useful
>> for other platforms too. Then maybe it would make sense to even have a common DT
>> property, but we will see.
>>
>> Is this going to be used only together with devfreq?
>
> Yes, this functions solely as an extension to devfreq, hence the slightly
> unusual architecture (one icc_provider/icc_node per devfreq).
Ok, thanks for clarifying.
> (Compared to a singleton icc_provider, this approach yields less code with
> a very simple xlate()).
>
> With exactly one icc_node for every devfreq device, I think I will actually
> reuse the devfreq ID (as seen in the device name, e.g. the "3" in "devfreq3")
> for the node ID. The devfreq framework already does the dynamic numbering
> thing that I do in this patch using IDR.
>
Sounds good.
>>> 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.
>>
>> How about the case where CONFIG_INTERCONNECT=m. Looks like the build will fail
>> if CONFIG_ARM_EXYNOS_BUS_DEVFREQ=y, so this dependency should be expressed in
>> Kconfig.
>
> I think adding:
> depends on INTERCONNECT || !INTERCONNECT
Yes, exactly.
> under ARM_EXYNOS_BUS_DEVFREQ does the trick.
>
>>>
>>> Signed-off-by: Artur Świgoń <[email protected]>
>>> ---
>>> drivers/devfreq/exynos-bus.c | 144 +++++++++++++++++++++++++++++++++++
>>> 1 file changed, 144 insertions(+)
>>>
>>> diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
>>> index 9fdb188915e8..694a9581dcdb 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 kbps_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;
>>> };
>>>
>>> /*
>>> @@ -205,6 +216,39 @@ 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 = kbps_to_khz(src->avg_bw);
>>> + s32 dst_freq = kbps_to_khz(dst->avg_bw);
>>> + int ret;
>>> +
>>> + ret = dev_pm_qos_update_request(&src_bus->qos_req, src_freq);
>>> + if (ret < 0) {
>>> + dev_err(src_bus->dev, "failed to update PM QoS request");
>>> + return ret;
>>> + }
>>> +
>>> + ret = dev_pm_qos_update_request(&dst_bus->qos_req, dst_freq);
>>> + if (ret < 0) {
>>> + dev_err(dst_bus->dev, "failed to update PM QoS request");
>>> + return ret;
>>> + }
>>> +
>>> + 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)
>>> {
>>> @@ -419,6 +463,96 @@ static int exynos_bus_profile_init_passive(struct exynos_bus *bus,
>>> return 0;
>>> }
>>>
>>> +static struct icc_node *exynos_bus_icc_get_parent(struct exynos_bus *bus)
>>> +{
>>> + struct device_node *np = bus->dev->of_node;
>>> + struct of_phandle_args args;
>>> + int num, ret;
>>> +
>>> + num = of_count_phandle_with_args(np, "exynos,interconnect-parent-node",
>>> + "#interconnect-cells");
>>> + if (num != 1)
>>> + return NULL; /* parent nodes are optional */
>>> +
>>> + ret = of_parse_phandle_with_args(np, "exynos,interconnect-parent-node",
>>> + "#interconnect-cells", 0, &args);
>>> + if (ret < 0)
>>> + return ERR_PTR(ret);
>>> +
>>> + of_node_put(args.np);
>>> +
>>> + return of_icc_get_from_provider(&args);
>>> +}
>>> +
>>> +static int exynos_bus_icc_init(struct exynos_bus *bus)
>>> +{
>>> + static DEFINE_IDA(ida);
>>> +
>>> + struct device *dev = bus->dev;
>>> + struct icc_provider *provider = &bus->provider;
>>> + struct icc_node *node, *parent_node;
>>> + int id, ret;
>>> +
>>> + /* Initialize the interconnect provider */
>>> + provider->set = exynos_bus_icc_set;
>>> + provider->aggregate = icc_std_aggregate;
>>> + provider->xlate = exynos_bus_icc_xlate;
>>> + provider->dev = dev;
>>> + provider->inter_set = true;
>>> + provider->data = bus;
>>> +
>>> + ret = icc_provider_add(provider);
>>> + if (ret < 0)
>>> + return ret;
>>> +
>>> + ret = id = ida_alloc(&ida, GFP_KERNEL);
>>> + if (ret < 0)
>>> + goto err_id;
>>> +
>>> + 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);
>>> +
>>> + parent_node = exynos_bus_icc_get_parent(bus);
>>> + if (IS_ERR(parent_node)) {
>>> + ret = PTR_ERR(parent_node);
>>> + goto err_parent;
>>> + }
>>> +
>>> + if (parent_node) {
>>> + ret = icc_link_create(node, parent_node->id);
>>> + if (ret < 0)
>>> + goto err_parent;
>>> + }
>>> +
>>> + ret = dev_pm_qos_add_request(bus->devfreq->dev.parent, &bus->qos_req,
>>> + DEV_PM_QOS_MIN_FREQUENCY, 0);
>>> + if (ret < 0)
>>> + goto err_request;
>>> +
>>> + return 0;
>>> +
>>> +err_request:
>>> + if (parent_node)
>>> + icc_link_destroy(node, parent_node);
>>> +err_parent:
>>> + icc_node_del(node);
>>> + icc_node_destroy(id);
>>> +err_node:
>>> + ida_free(&ida, id);
>>> +err_id:
>>> + icc_provider_del(provider);
>>> +
>>> + return ret;
>>> +}
>>> +
>>> static int exynos_bus_probe(struct platform_device *pdev)
>>> {
>>> struct device *dev = &pdev->dev;
>>> @@ -468,6 +602,16 @@ 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) {
I have been also thinking that all the code that you add in this patch would fit
nicely into a separate interconnect provider driver. Then instead of the above
icc_init() you can create a sub-device (platform_device_register_data() maybe?).
The separate driver will be handling just the interconnect functionality and
could be enabled on top of this devfreq driver.
Thanks,
Georgi
P.S. I think that grouping all the related patches into a single patch-set would
make reviewing them easier. Then we can decide about the merge path of each.
>>> + dev_err(dev, "failed to initialize the interconnect provider");
>>> + 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);
>>>
>>
>>