[RFC,v5,2/6] interconnect: Add generic interconnect driver for Exynos SoCs
diff mbox series

Message ID 20200529163200.18031-3-s.nawrocki@samsung.com
State Superseded
Headers show
Series
  • Exynos: Simple QoS for exynos-bus using interconnect
Related show

Commit Message

Sylwester Nawrocki May 29, 2020, 4:31 p.m. UTC
This patch adds a generic interconnect driver for Exynos SoCs in order
to provide interconnect functionality for each "samsung,exynos-bus"
compatible device.

The SoC topology is a graph (or more specifically, a tree) and its
edges are specified using the 'samsung,interconnect-parent' 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 dynamically at runtime. 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ń <a.swigon@samsung.com>
Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>

Changes for v5:
 - adjust to renamed exynos,interconnect-parent-node property,
 - use automatically generated platform device id as the interconect
   node id instead of a now unavailable devfreq->id field,
 - add icc_ prefix to some variables to make the code more self-commenting,
 - use icc_nodes_remove() instead of icc_node_del() + icc_node_destroy(),
 - adjust to exynos,interconnect-parent-node property rename to
   samsung,interconnect-parent,
 - converted to a separate platform driver in drivers/interconnect.
---
 drivers/interconnect/Kconfig         |   1 +
 drivers/interconnect/Makefile        |   1 +
 drivers/interconnect/exynos/Kconfig  |   6 ++
 drivers/interconnect/exynos/Makefile |   4 +
 drivers/interconnect/exynos/exynos.c | 185 +++++++++++++++++++++++++++++++++++
 5 files changed, 197 insertions(+)
 create mode 100644 drivers/interconnect/exynos/Kconfig
 create mode 100644 drivers/interconnect/exynos/Makefile
 create mode 100644 drivers/interconnect/exynos/exynos.c

Comments

Chanwoo Choi May 31, 2020, 12:13 a.m. UTC | #1
Hi Sylwester,

On Sat, May 30, 2020 at 1:34 AM Sylwester Nawrocki
<s.nawrocki@samsung.com> wrote:
>
> This patch adds a generic interconnect driver for Exynos SoCs in order
> to provide interconnect functionality for each "samsung,exynos-bus"
> compatible device.
>
> The SoC topology is a graph (or more specifically, a tree) and its
> edges are specified using the 'samsung,interconnect-parent' 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 dynamically at runtime. 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ń <a.swigon@samsung.com>
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
>
> Changes for v5:
>  - adjust to renamed exynos,interconnect-parent-node property,
>  - use automatically generated platform device id as the interconect
>    node id instead of a now unavailable devfreq->id field,
>  - add icc_ prefix to some variables to make the code more self-commenting,
>  - use icc_nodes_remove() instead of icc_node_del() + icc_node_destroy(),
>  - adjust to exynos,interconnect-parent-node property rename to
>    samsung,interconnect-parent,
>  - converted to a separate platform driver in drivers/interconnect.
> ---
>  drivers/interconnect/Kconfig         |   1 +
>  drivers/interconnect/Makefile        |   1 +
>  drivers/interconnect/exynos/Kconfig  |   6 ++
>  drivers/interconnect/exynos/Makefile |   4 +
>  drivers/interconnect/exynos/exynos.c | 185 +++++++++++++++++++++++++++++++++++
>  5 files changed, 197 insertions(+)
>  create mode 100644 drivers/interconnect/exynos/Kconfig
>  create mode 100644 drivers/interconnect/exynos/Makefile
>  create mode 100644 drivers/interconnect/exynos/exynos.c
>
> diff --git a/drivers/interconnect/Kconfig b/drivers/interconnect/Kconfig
> index 5b7204e..eca6eda 100644
> --- a/drivers/interconnect/Kconfig
> +++ b/drivers/interconnect/Kconfig
> @@ -11,6 +11,7 @@ menuconfig INTERCONNECT
>
>  if INTERCONNECT
>
> +source "drivers/interconnect/exynos/Kconfig"
>  source "drivers/interconnect/imx/Kconfig"
>  source "drivers/interconnect/qcom/Kconfig"
>
> diff --git a/drivers/interconnect/Makefile b/drivers/interconnect/Makefile
> index 4825c28..2ba1de6 100644
> --- a/drivers/interconnect/Makefile
> +++ b/drivers/interconnect/Makefile
> @@ -4,5 +4,6 @@ CFLAGS_core.o                           := -I$(src)
>  icc-core-objs                          := core.o
>
>  obj-$(CONFIG_INTERCONNECT)             += icc-core.o
> +obj-$(CONFIG_INTERCONNECT_EXYNOS)      += exynos/
>  obj-$(CONFIG_INTERCONNECT_IMX)         += imx/
>  obj-$(CONFIG_INTERCONNECT_QCOM)                += qcom/
> diff --git a/drivers/interconnect/exynos/Kconfig b/drivers/interconnect/exynos/Kconfig
> new file mode 100644
> index 0000000..e51e52e
> --- /dev/null
> +++ b/drivers/interconnect/exynos/Kconfig
> @@ -0,0 +1,6 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +config INTERCONNECT_EXYNOS
> +       tristate "Exynos generic interconnect driver"
> +       depends on ARCH_EXYNOS || COMPILE_TEST
> +       help
> +         Generic interconnect driver for Exynos SoCs.
> diff --git a/drivers/interconnect/exynos/Makefile b/drivers/interconnect/exynos/Makefile
> new file mode 100644
> index 0000000..e19d1df
> --- /dev/null
> +++ b/drivers/interconnect/exynos/Makefile
> @@ -0,0 +1,4 @@
> +# SPDX-License-Identifier: GPL-2.0
> +exynos-interconnect-objs               := exynos.o
> +
> +obj-$(CONFIG_INTERCONNECT_EXYNOS)      += exynos-interconnect.o
> diff --git a/drivers/interconnect/exynos/exynos.c b/drivers/interconnect/exynos/exynos.c
> new file mode 100644
> index 0000000..8278194
> --- /dev/null
> +++ b/drivers/interconnect/exynos/exynos.c
> @@ -0,0 +1,185 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Exynos generic interconnect provider driver
> + *
> + * Copyright (c) 2020 Samsung Electronics Co., Ltd.
> + *
> + * Authors: Artur Świgoń <a.swigon@samsung.com>
> + *          Sylwester Nawrocki <s.nawrocki@samsung.com>
> + */
> +#include <linux/device.h>
> +#include <linux/interconnect-provider.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_qos.h>
> +
> +#define kbps_to_khz(x) ((x) / 8)
> +
> +struct exynos_icc_priv {
> +       struct device *dev;
> +
> +       /* One interconnect node per provider */
> +       struct icc_provider provider;
> +       struct icc_node *node;
> +
> +       struct dev_pm_qos_request qos_req;
> +};
> +
> +static struct icc_node *exynos_icc_get_parent(struct device_node *np)
> +{
> +       struct of_phandle_args args;
> +       int num, ret;
> +
> +       num = of_count_phandle_with_args(np, "samsung,interconnect-parent",
> +                                       "#interconnect-cells");
> +       if (num != 1)
> +               return NULL; /* parent nodes are optional */
> +
> +       ret = of_parse_phandle_with_args(np, "samsung,interconnect-parent",
> +                                       "#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_generic_icc_set(struct icc_node *src, struct icc_node *dst)
> +{
> +       struct exynos_icc_priv *src_priv = src->data, *dst_priv = dst->data;
> +       s32 src_freq = kbps_to_khz(max(src->avg_bw, src->peak_bw));
> +       s32 dst_freq = kbps_to_khz(max(dst->avg_bw, dst->peak_bw));
> +       int ret;
> +
> +       ret = dev_pm_qos_update_request(&src_priv->qos_req, src_freq);
> +       if (ret < 0) {
> +               dev_err(src_priv->dev, "failed to update PM QoS of %s\n",
> +                       src->name);
> +               return ret;
> +       }
> +
> +       ret = dev_pm_qos_update_request(&dst_priv->qos_req, dst_freq);
> +       if (ret < 0) {
> +               dev_err(dst_priv->dev, "failed to update PM QoS of %s\n",
> +                       dst->name);
> +               return ret;
> +       }
> +
> +       return 0;
> +}
> +
> +static struct icc_node *exynos_generic_icc_xlate(struct of_phandle_args *spec,
> +                                                void *data)
> +{
> +       struct exynos_icc_priv *priv = data;
> +
> +       if (spec->np != priv->dev->parent->of_node)
> +               return ERR_PTR(-EINVAL);
> +
> +       return priv->node;
> +}
> +
> +static int exynos_generic_icc_remove(struct platform_device *pdev)
> +{
> +       struct exynos_icc_priv *priv = platform_get_drvdata(pdev);
> +       struct icc_node *parent_node, *node = priv->node;
> +
> +       parent_node = exynos_icc_get_parent(priv->dev->parent->of_node);
> +       if (parent_node && !IS_ERR(parent_node))
> +               icc_link_destroy(node, parent_node);
> +
> +       icc_nodes_remove(&priv->provider);
> +       icc_provider_del(&priv->provider);
> +
> +       return 0;
> +}
> +
> +static int exynos_generic_icc_probe(struct platform_device *pdev)
> +{
> +       struct device *bus_dev = pdev->dev.parent;
> +       struct exynos_icc_priv *priv;
> +       struct icc_provider *provider;
> +       struct icc_node *icc_node, *icc_parent_node;
> +       int ret;
> +
> +       priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> +       if (!priv)
> +               return -ENOMEM;
> +
> +       priv->dev = &pdev->dev;
> +       platform_set_drvdata(pdev, priv);
> +
> +       provider = &priv->provider;
> +
> +       provider->set = exynos_generic_icc_set;
> +       provider->aggregate = icc_std_aggregate;
> +       provider->xlate = exynos_generic_icc_xlate;
> +       provider->dev = bus_dev;
> +       provider->inter_set = true;
> +       provider->data = priv;
> +
> +       ret = icc_provider_add(provider);
> +       if (ret < 0)
> +               return ret;
> +
> +       icc_node = icc_node_create(pdev->id);
> +       if (IS_ERR(icc_node)) {
> +               ret = PTR_ERR(icc_node);
> +               goto err_prov_del;
> +       }
> +
> +       priv->node = icc_node;
> +       icc_node->name = bus_dev->of_node->name;
> +       icc_node->data = priv;
> +       icc_node_add(icc_node, provider);
> +
> +       icc_parent_node = exynos_icc_get_parent(bus_dev->of_node);
> +       if (IS_ERR(icc_parent_node)) {
> +               ret = PTR_ERR(icc_parent_node);
> +               goto err_node_del;
> +       }
> +       if (icc_parent_node) {
> +               ret = icc_link_create(icc_node, icc_parent_node->id);
> +               if (ret < 0)
> +                       goto err_node_del;
> +       }
> +
> +       /*
> +        * Register a PM QoS request for the bus device for which also devfreq
> +        * functionality is registered.
> +        */
> +       ret = dev_pm_qos_add_request(bus_dev, &priv->qos_req,
> +                                    DEV_PM_QOS_MIN_FREQUENCY, 0);
> +       if (ret < 0)
> +               goto err_link_destroy;
> +
> +       return 0;
> +
> +err_link_destroy:
> +       if (icc_parent_node)
> +               icc_link_destroy(icc_node, icc_parent_node);
> +err_node_del:
> +       icc_nodes_remove(provider);
> +err_prov_del:
> +       icc_provider_del(provider);
> +
> +       return ret;
> +}
> +
> +static struct platform_driver exynos_generic_icc_driver = {
> +       .driver = {
> +               .name = "exynos-generic-icc",
> +       },
> +       .probe = exynos_generic_icc_probe,
> +       .remove = exynos_generic_icc_remove,
> +};
> +module_platform_driver(exynos_generic_icc_driver);
> +
> +MODULE_DESCRIPTION("Exynos generic interconnect driver");
> +MODULE_AUTHOR("Artur Świgoń <a.swigon@samsung.com>");
> +MODULE_AUTHOR("Sylwester Nawrocki <s.nawrocki@samsung.com>");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:exynos-generic-icc");
> --
> 2.7.4
>

Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>

As I commented, How about changing the compatible name 'exynos-icc'
without 'generic'?
The 'exynos-icc' doesn't have the any specific version of Exynos SoC,
it think that it already contain the 'generic' meaning for Exynos SoC.
Sylwester Nawrocki June 1, 2020, 9:57 a.m. UTC | #2
Cc: Rob, devicetree ML

On 31.05.2020 02:13, Chanwoo Choi wrote:
> On Sat, May 30, 2020 at 1:34 AM Sylwester Nawrocki
> <s.nawrocki@samsung.com> wrote:
>>
>> This patch adds a generic interconnect driver for Exynos SoCs in order
>> to provide interconnect functionality for each "samsung,exynos-bus"
>> compatible device.
>>
>> The SoC topology is a graph (or more specifically, a tree) and its
>> edges are specified using the 'samsung,interconnect-parent' 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 dynamically at runtime. 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ń <a.swigon@samsung.com>
>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
>>
>> Changes for v5:
>>  - adjust to renamed exynos,interconnect-parent-node property,
>>  - use automatically generated platform device id as the interconect
>>    node id instead of a now unavailable devfreq->id field,
>>  - add icc_ prefix to some variables to make the code more self-commenting,
>>  - use icc_nodes_remove() instead of icc_node_del() + icc_node_destroy(),
>>  - adjust to exynos,interconnect-parent-node property rename to
>>    samsung,interconnect-parent,
>>  - converted to a separate platform driver in drivers/interconnect.
>> ---
>>  drivers/interconnect/Kconfig         |   1 +
>>  drivers/interconnect/Makefile        |   1 +
>>  drivers/interconnect/exynos/Kconfig  |   6 ++
>>  drivers/interconnect/exynos/Makefile |   4 +
>>  drivers/interconnect/exynos/exynos.c | 185 +++++++++++++++++++++++++++++++++++
>>  5 files changed, 197 insertions(+)
>>  create mode 100644 drivers/interconnect/exynos/Kconfig
>>  create mode 100644 drivers/interconnect/exynos/Makefile
>>  create mode 100644 drivers/interconnect/exynos/exynos.c
>>
>> diff --git a/drivers/interconnect/Kconfig b/drivers/interconnect/Kconfig
>> index 5b7204e..eca6eda 100644
>> --- a/drivers/interconnect/Kconfig
>> +++ b/drivers/interconnect/Kconfig
>> @@ -11,6 +11,7 @@ menuconfig INTERCONNECT
>>
>>  if INTERCONNECT
>>
>> +source "drivers/interconnect/exynos/Kconfig"
>>  source "drivers/interconnect/imx/Kconfig"
>>  source "drivers/interconnect/qcom/Kconfig"
>>
>> diff --git a/drivers/interconnect/Makefile b/drivers/interconnect/Makefile
>> index 4825c28..2ba1de6 100644
>> --- a/drivers/interconnect/Makefile
>> +++ b/drivers/interconnect/Makefile
>> @@ -4,5 +4,6 @@ CFLAGS_core.o                           := -I$(src)
>>  icc-core-objs                          := core.o
>>
>>  obj-$(CONFIG_INTERCONNECT)             += icc-core.o
>> +obj-$(CONFIG_INTERCONNECT_EXYNOS)      += exynos/
>>  obj-$(CONFIG_INTERCONNECT_IMX)         += imx/
>>  obj-$(CONFIG_INTERCONNECT_QCOM)                += qcom/
>> diff --git a/drivers/interconnect/exynos/Kconfig b/drivers/interconnect/exynos/Kconfig
>> new file mode 100644
>> index 0000000..e51e52e
>> --- /dev/null
>> +++ b/drivers/interconnect/exynos/Kconfig
>> @@ -0,0 +1,6 @@
>> +# SPDX-License-Identifier: GPL-2.0-only
>> +config INTERCONNECT_EXYNOS
>> +       tristate "Exynos generic interconnect driver"
>> +       depends on ARCH_EXYNOS || COMPILE_TEST
>> +       help
>> +         Generic interconnect driver for Exynos SoCs.
>> diff --git a/drivers/interconnect/exynos/Makefile b/drivers/interconnect/exynos/Makefile
>> new file mode 100644
>> index 0000000..e19d1df
>> --- /dev/null
>> +++ b/drivers/interconnect/exynos/Makefile
>> @@ -0,0 +1,4 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +exynos-interconnect-objs               := exynos.o
>> +
>> +obj-$(CONFIG_INTERCONNECT_EXYNOS)      += exynos-interconnect.o
>> diff --git a/drivers/interconnect/exynos/exynos.c b/drivers/interconnect/exynos/exynos.c
>> new file mode 100644
>> index 0000000..8278194
>> --- /dev/null
>> +++ b/drivers/interconnect/exynos/exynos.c
>> @@ -0,0 +1,185 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Exynos generic interconnect provider driver
>> + *
>> + * Copyright (c) 2020 Samsung Electronics Co., Ltd.
>> + *
>> + * Authors: Artur Świgoń <a.swigon@samsung.com>
>> + *          Sylwester Nawrocki <s.nawrocki@samsung.com>
>> + */
>> +#include <linux/device.h>
>> +#include <linux/interconnect-provider.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pm_qos.h>
>> +
>> +#define kbps_to_khz(x) ((x) / 8)
>> +
>> +struct exynos_icc_priv {
>> +       struct device *dev;
>> +
>> +       /* One interconnect node per provider */
>> +       struct icc_provider provider;
>> +       struct icc_node *node;
>> +
>> +       struct dev_pm_qos_request qos_req;
>> +};
>> +
>> +static struct icc_node *exynos_icc_get_parent(struct device_node *np)
>> +{
>> +       struct of_phandle_args args;
>> +       int num, ret;
>> +
>> +       num = of_count_phandle_with_args(np, "samsung,interconnect-parent",
>> +                                       "#interconnect-cells");
>> +       if (num != 1)
>> +               return NULL; /* parent nodes are optional */
>> +
>> +       ret = of_parse_phandle_with_args(np, "samsung,interconnect-parent",
>> +                                       "#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_generic_icc_set(struct icc_node *src, struct icc_node *dst)
>> +{
>> +       struct exynos_icc_priv *src_priv = src->data, *dst_priv = dst->data;
>> +       s32 src_freq = kbps_to_khz(max(src->avg_bw, src->peak_bw));
>> +       s32 dst_freq = kbps_to_khz(max(dst->avg_bw, dst->peak_bw));
>> +       int ret;
>> +
>> +       ret = dev_pm_qos_update_request(&src_priv->qos_req, src_freq);
>> +       if (ret < 0) {
>> +               dev_err(src_priv->dev, "failed to update PM QoS of %s\n",
>> +                       src->name);
>> +               return ret;
>> +       }
>> +
>> +       ret = dev_pm_qos_update_request(&dst_priv->qos_req, dst_freq);
>> +       if (ret < 0) {
>> +               dev_err(dst_priv->dev, "failed to update PM QoS of %s\n",
>> +                       dst->name);
>> +               return ret;
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +static struct icc_node *exynos_generic_icc_xlate(struct of_phandle_args *spec,
>> +                                                void *data)
>> +{
>> +       struct exynos_icc_priv *priv = data;
>> +
>> +       if (spec->np != priv->dev->parent->of_node)
>> +               return ERR_PTR(-EINVAL);
>> +
>> +       return priv->node;
>> +}
>> +
>> +static int exynos_generic_icc_remove(struct platform_device *pdev)
>> +{
>> +       struct exynos_icc_priv *priv = platform_get_drvdata(pdev);
>> +       struct icc_node *parent_node, *node = priv->node;
>> +
>> +       parent_node = exynos_icc_get_parent(priv->dev->parent->of_node);
>> +       if (parent_node && !IS_ERR(parent_node))
>> +               icc_link_destroy(node, parent_node);
>> +
>> +       icc_nodes_remove(&priv->provider);
>> +       icc_provider_del(&priv->provider);
>> +
>> +       return 0;
>> +}
>> +
>> +static int exynos_generic_icc_probe(struct platform_device *pdev)
>> +{
>> +       struct device *bus_dev = pdev->dev.parent;
>> +       struct exynos_icc_priv *priv;
>> +       struct icc_provider *provider;
>> +       struct icc_node *icc_node, *icc_parent_node;
>> +       int ret;
>> +
>> +       priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
>> +       if (!priv)
>> +               return -ENOMEM;
>> +
>> +       priv->dev = &pdev->dev;
>> +       platform_set_drvdata(pdev, priv);
>> +
>> +       provider = &priv->provider;
>> +
>> +       provider->set = exynos_generic_icc_set;
>> +       provider->aggregate = icc_std_aggregate;
>> +       provider->xlate = exynos_generic_icc_xlate;
>> +       provider->dev = bus_dev;
>> +       provider->inter_set = true;
>> +       provider->data = priv;
>> +
>> +       ret = icc_provider_add(provider);
>> +       if (ret < 0)
>> +               return ret;
>> +
>> +       icc_node = icc_node_create(pdev->id);
>> +       if (IS_ERR(icc_node)) {
>> +               ret = PTR_ERR(icc_node);
>> +               goto err_prov_del;
>> +       }
>> +
>> +       priv->node = icc_node;
>> +       icc_node->name = bus_dev->of_node->name;
>> +       icc_node->data = priv;
>> +       icc_node_add(icc_node, provider);
>> +
>> +       icc_parent_node = exynos_icc_get_parent(bus_dev->of_node);
>> +       if (IS_ERR(icc_parent_node)) {
>> +               ret = PTR_ERR(icc_parent_node);
>> +               goto err_node_del;
>> +       }
>> +       if (icc_parent_node) {
>> +               ret = icc_link_create(icc_node, icc_parent_node->id);
>> +               if (ret < 0)
>> +                       goto err_node_del;
>> +       }
>> +
>> +       /*
>> +        * Register a PM QoS request for the bus device for which also devfreq
>> +        * functionality is registered.
>> +        */
>> +       ret = dev_pm_qos_add_request(bus_dev, &priv->qos_req,
>> +                                    DEV_PM_QOS_MIN_FREQUENCY, 0);
>> +       if (ret < 0)
>> +               goto err_link_destroy;
>> +
>> +       return 0;
>> +
>> +err_link_destroy:
>> +       if (icc_parent_node)
>> +               icc_link_destroy(icc_node, icc_parent_node);
>> +err_node_del:
>> +       icc_nodes_remove(provider);
>> +err_prov_del:
>> +       icc_provider_del(provider);
>> +
>> +       return ret;
>> +}
>> +
>> +static struct platform_driver exynos_generic_icc_driver = {
>> +       .driver = {
>> +               .name = "exynos-generic-icc",
>> +       },
>> +       .probe = exynos_generic_icc_probe,
>> +       .remove = exynos_generic_icc_remove,
>> +};
>> +module_platform_driver(exynos_generic_icc_driver);
>> +
>> +MODULE_DESCRIPTION("Exynos generic interconnect driver");
>> +MODULE_AUTHOR("Artur Świgoń <a.swigon@samsung.com>");
>> +MODULE_AUTHOR("Sylwester Nawrocki <s.nawrocki@samsung.com>");
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_ALIAS("platform:exynos-generic-icc");
>> --
>> 2.7.4
>>
> 
> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
> 
> As I commented, How about changing the compatible name 'exynos-icc'
> without 'generic'?
> The 'exynos-icc' doesn't have the any specific version of Exynos SoC,
> it think that it already contain the 'generic' meaning for Exynos SoC.
 
Sure, I can change it to "exynos-icc". However please note it is just 
a name for the driver and its related virtual platform (sub)device that 
is created in the devfreq driver, which matches on the "samsung,exynos-bus"
compatible. Of course we could add any specific DT compatible to this driver
in future if needed.
Krzysztof Kozlowski June 2, 2020, 8:21 a.m. UTC | #3
On Fri, May 29, 2020 at 06:31:56PM +0200, Sylwester Nawrocki wrote:
> This patch adds a generic interconnect driver for Exynos SoCs in order
> to provide interconnect functionality for each "samsung,exynos-bus"
> compatible device.
> 
> The SoC topology is a graph (or more specifically, a tree) and its
> edges are specified using the 'samsung,interconnect-parent' 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 dynamically at runtime. 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ń <a.swigon@samsung.com>
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> 
> Changes for v5:
>  - adjust to renamed exynos,interconnect-parent-node property,
>  - use automatically generated platform device id as the interconect
>    node id instead of a now unavailable devfreq->id field,
>  - add icc_ prefix to some variables to make the code more self-commenting,
>  - use icc_nodes_remove() instead of icc_node_del() + icc_node_destroy(),
>  - adjust to exynos,interconnect-parent-node property rename to
>    samsung,interconnect-parent,
>  - converted to a separate platform driver in drivers/interconnect.
> ---
>  drivers/interconnect/Kconfig         |   1 +
>  drivers/interconnect/Makefile        |   1 +
>  drivers/interconnect/exynos/Kconfig  |   6 ++
>  drivers/interconnect/exynos/Makefile |   4 +
>  drivers/interconnect/exynos/exynos.c | 185 +++++++++++++++++++++++++++++++++++
>  5 files changed, 197 insertions(+)
>  create mode 100644 drivers/interconnect/exynos/Kconfig
>  create mode 100644 drivers/interconnect/exynos/Makefile
>  create mode 100644 drivers/interconnect/exynos/exynos.c
> 
> diff --git a/drivers/interconnect/Kconfig b/drivers/interconnect/Kconfig
> index 5b7204e..eca6eda 100644
> --- a/drivers/interconnect/Kconfig
> +++ b/drivers/interconnect/Kconfig
> @@ -11,6 +11,7 @@ menuconfig INTERCONNECT
>  
>  if INTERCONNECT
>  
> +source "drivers/interconnect/exynos/Kconfig"
>  source "drivers/interconnect/imx/Kconfig"
>  source "drivers/interconnect/qcom/Kconfig"
>  
> diff --git a/drivers/interconnect/Makefile b/drivers/interconnect/Makefile
> index 4825c28..2ba1de6 100644
> --- a/drivers/interconnect/Makefile
> +++ b/drivers/interconnect/Makefile
> @@ -4,5 +4,6 @@ CFLAGS_core.o				:= -I$(src)
>  icc-core-objs				:= core.o
>  
>  obj-$(CONFIG_INTERCONNECT)		+= icc-core.o
> +obj-$(CONFIG_INTERCONNECT_EXYNOS)	+= exynos/
>  obj-$(CONFIG_INTERCONNECT_IMX)		+= imx/
>  obj-$(CONFIG_INTERCONNECT_QCOM)		+= qcom/
> diff --git a/drivers/interconnect/exynos/Kconfig b/drivers/interconnect/exynos/Kconfig
> new file mode 100644
> index 0000000..e51e52e
> --- /dev/null
> +++ b/drivers/interconnect/exynos/Kconfig
> @@ -0,0 +1,6 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +config INTERCONNECT_EXYNOS
> +	tristate "Exynos generic interconnect driver"
> +	depends on ARCH_EXYNOS || COMPILE_TEST
> +	help
> +	  Generic interconnect driver for Exynos SoCs.
> diff --git a/drivers/interconnect/exynos/Makefile b/drivers/interconnect/exynos/Makefile
> new file mode 100644
> index 0000000..e19d1df
> --- /dev/null
> +++ b/drivers/interconnect/exynos/Makefile
> @@ -0,0 +1,4 @@
> +# SPDX-License-Identifier: GPL-2.0
> +exynos-interconnect-objs		:= exynos.o
> +
> +obj-$(CONFIG_INTERCONNECT_EXYNOS)	+= exynos-interconnect.o
> diff --git a/drivers/interconnect/exynos/exynos.c b/drivers/interconnect/exynos/exynos.c
> new file mode 100644
> index 0000000..8278194
> --- /dev/null
> +++ b/drivers/interconnect/exynos/exynos.c
> @@ -0,0 +1,185 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Exynos generic interconnect provider driver
> + *
> + * Copyright (c) 2020 Samsung Electronics Co., Ltd.
> + *
> + * Authors: Artur Świgoń <a.swigon@samsung.com>
> + *          Sylwester Nawrocki <s.nawrocki@samsung.com>
> + */
> +#include <linux/device.h>
> +#include <linux/interconnect-provider.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_qos.h>
> +
> +#define kbps_to_khz(x) ((x) / 8)
> +
> +struct exynos_icc_priv {
> +	struct device *dev;
> +
> +	/* One interconnect node per provider */
> +	struct icc_provider provider;
> +	struct icc_node *node;
> +
> +	struct dev_pm_qos_request qos_req;
> +};
> +
> +static struct icc_node *exynos_icc_get_parent(struct device_node *np)
> +{
> +	struct of_phandle_args args;
> +	int num, ret;
> +
> +	num = of_count_phandle_with_args(np, "samsung,interconnect-parent",
> +					"#interconnect-cells");
> +	if (num != 1)
> +		return NULL; /* parent nodes are optional */
> +
> +	ret = of_parse_phandle_with_args(np, "samsung,interconnect-parent",
> +					"#interconnect-cells", 0, &args);
> +	if (ret < 0)
> +		return ERR_PTR(ret);
> +
> +	of_node_put(args.np);
> +
> +	return of_icc_get_from_provider(&args);

I think of_node_put() should happen after of_icc_get_from_provider().

Best regards,
Krzysztof
Sylwester Nawrocki June 3, 2020, 9:24 a.m. UTC | #4
On 02.06.2020 10:21, Krzysztof Kozlowski wrote:
>> +static struct icc_node *exynos_icc_get_parent(struct device_node *np)
>> +{
>> +	struct of_phandle_args args;
>> +	int num, ret;
>> +
>> +	num = of_count_phandle_with_args(np, "samsung,interconnect-parent",
>> +					"#interconnect-cells");
>> +	if (num != 1)
>> +		return NULL; /* parent nodes are optional */
>> +
>> +	ret = of_parse_phandle_with_args(np, "samsung,interconnect-parent",
>> +					"#interconnect-cells", 0, &args);
>> +	if (ret < 0)
>> +		return ERR_PTR(ret);
>> +
>> +	of_node_put(args.np);
>> +
>> +	return of_icc_get_from_provider(&args);

> I think of_node_put() should happen after of_icc_get_from_provider().

Indeed, thanks, I will amend that. It seems the node reference count decrementing
is often not done properly after existing calls to of_parse_phandle_with_args().
Georgi Djakov July 1, 2020, 12:50 p.m. UTC | #5
Hi Sylwester,

Thanks for the patch and apologies for the delayed reply.

On 5/29/20 19:31, Sylwester Nawrocki wrote:
> This patch adds a generic interconnect driver for Exynos SoCs in order
> to provide interconnect functionality for each "samsung,exynos-bus"
> compatible device.
> 
> The SoC topology is a graph (or more specifically, a tree) and its
> edges are specified using the 'samsung,interconnect-parent' 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 dynamically at runtime. 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ń <a.swigon@samsung.com>
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> 
> Changes for v5:
>  - adjust to renamed exynos,interconnect-parent-node property,
>  - use automatically generated platform device id as the interconect
>    node id instead of a now unavailable devfreq->id field,
>  - add icc_ prefix to some variables to make the code more self-commenting,
>  - use icc_nodes_remove() instead of icc_node_del() + icc_node_destroy(),
>  - adjust to exynos,interconnect-parent-node property rename to
>    samsung,interconnect-parent,
>  - converted to a separate platform driver in drivers/i.nterconnect.
> ---
>  drivers/interconnect/Kconfig         |   1 +
>  drivers/interconnect/Makefile        |   1 +
>  drivers/interconnect/exynos/Kconfig  |   6 ++
>  drivers/interconnect/exynos/Makefile |   4 +
>  drivers/interconnect/exynos/exynos.c | 185 +++++++++++++++++++++++++++++++++++
>  5 files changed, 197 insertions(+)
>  create mode 100644 drivers/interconnect/exynos/Kconfig
>  create mode 100644 drivers/interconnect/exynos/Makefile
>  create mode 100644 drivers/interconnect/exynos/exynos.c
[..]
> +static struct icc_node *exynos_icc_get_parent(struct device_node *np)
> +{
> +	struct of_phandle_args args;
> +	int num, ret;
> +
> +	num = of_count_phandle_with_args(np, "samsung,interconnect-parent",
> +					"#interconnect-cells");
> +	if (num != 1)
> +		return NULL; /* parent nodes are optional */
> +
> +	ret = of_parse_phandle_with_args(np, "samsung,interconnect-parent",
> +					"#interconnect-cells", 0, &args);
> +	if (ret < 0)
> +		return ERR_PTR(ret);
> +
> +	of_node_put(args.np);
> +
> +	return of_icc_get_from_provider(&args);
> +}
> +
> +

Nit: multiple blank lines

[..]
> +static struct icc_node *exynos_generic_icc_xlate(struct of_phandle_args *spec,
> +						 void *data)
> +{
> +	struct exynos_icc_priv *priv = data;
> +
> +	if (spec->np != priv->dev->parent->of_node)
> +		return ERR_PTR(-EINVAL);
> +
> +	return priv->node;
> +}
> +
> +static int exynos_generic_icc_remove(struct platform_device *pdev)
> +{
> +	struct exynos_icc_priv *priv = platform_get_drvdata(pdev);
> +	struct icc_node *parent_node, *node = priv->node;
> +
> +	parent_node = exynos_icc_get_parent(priv->dev->parent->of_node);
> +	if (parent_node && !IS_ERR(parent_node))

Nit: !IS_ERR_OR_NULL?

> +		icc_link_destroy(node, parent_node);
> +
> +	icc_nodes_remove(&priv->provider);
> +	icc_provider_del(&priv->provider);
> +
> +	return 0;
> +}
> +
> +static int exynos_generic_icc_probe(struct platform_device *pdev)
> +{
> +	struct device *bus_dev = pdev->dev.parent;
> +	struct exynos_icc_priv *priv;
> +	struct icc_provider *provider;
> +	struct icc_node *icc_node, *icc_parent_node;
> +	int ret;
> +
> +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->dev = &pdev->dev;
> +	platform_set_drvdata(pdev, priv);
> +
> +	provider = &priv->provider;
> +
> +	provider->set = exynos_generic_icc_set;
> +	provider->aggregate = icc_std_aggregate;
> +	provider->xlate = exynos_generic_icc_xlate;
> +	provider->dev = bus_dev;
> +	provider->inter_set = true;
> +	provider->data = priv;
> +
> +	ret = icc_provider_add(provider);

Nit: Maybe it would be better to move this after the node is created. The
idea is to create the nodes first and add the provider when the topology is
populated. It's fine either way here, but i am planning to change this in
some of the existing provider drivers.

> +	if (ret < 0)
> +		return ret;
> +
> +	icc_node = icc_node_create(pdev->id);
> +	if (IS_ERR(icc_node)) {
> +		ret = PTR_ERR(icc_node);
> +		goto err_prov_del;
> +	}
> +
> +	priv->node = icc_node;
> +	icc_node->name = bus_dev->of_node->name;
> +	icc_node->data = priv;
> +	icc_node_add(icc_node, provider);
> +
> +	icc_parent_node = exynos_icc_get_parent(bus_dev->of_node);
> +	if (IS_ERR(icc_parent_node)) {
> +		ret = PTR_ERR(icc_parent_node);
> +		goto err_node_del;
> +	}
> +	if (icc_parent_node) {
> +		ret = icc_link_create(icc_node, icc_parent_node->id);
> +		if (ret < 0)
> +			goto err_node_del;
> +	}
> +
> +	/*
> +	 * Register a PM QoS request for the bus device for which also devfreq
> +	 * functionality is registered.
> +	 */
> +	ret = dev_pm_qos_add_request(bus_dev, &priv->qos_req,
> +				     DEV_PM_QOS_MIN_FREQUENCY, 0);
> +	if (ret < 0)
> +		goto err_link_destroy;
> +
> +	return 0;
> +
> +err_link_destroy:
> +	if (icc_parent_node)
> +		icc_link_destroy(icc_node, icc_parent_node);
> +err_node_del:
> +	icc_nodes_remove(provider);
> +err_prov_del:
> +	icc_provider_del(provider);
> +
> +	return ret;
> +}
> +
> +static struct platform_driver exynos_generic_icc_driver = {
> +	.driver = {
> +		.name = "exynos-generic-icc",
> +	},
> +	.probe = exynos_generic_icc_probe,
> +	.remove = exynos_generic_icc_remove,
> +};
> +module_platform_driver(exynos_generic_icc_driver);
> +
> +MODULE_DESCRIPTION("Exynos generic interconnect driver");
> +MODULE_AUTHOR("Artur Świgoń <a.swigon@samsung.com>");
> +MODULE_AUTHOR("Sylwester Nawrocki <s.nawrocki@samsung.com>");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:exynos-generic-icc");

All looks good to me, but it seems that the patch-set is not on
Rob's radar currently, so please re-send and CC the DT mailing list.

Thanks,
Georgi
Sylwester Nawrocki July 2, 2020, 12:01 p.m. UTC | #6
Hi Georgi,

On 01.07.2020 14:50, Georgi Djakov wrote:
> Thanks for the patch and apologies for the delayed reply.

Thanks, no problem. It's actually just in time as I put that patchset
aside for a while and was just about to post an update.
 
> On 5/29/20 19:31, Sylwester Nawrocki wrote:
>> This patch adds a generic interconnect driver for Exynos SoCs in order
>> to provide interconnect functionality for each "samsung,exynos-bus"
>> compatible device.
>>
>> The SoC topology is a graph (or more specifically, a tree) and its
>> edges are specified using the 'samsung,interconnect-parent' 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 dynamically at runtime. 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ń <a.swigon@samsung.com>
>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>

>> +static struct icc_node *exynos_icc_get_parent(struct device_node *np)
>> +{
>> +	struct of_phandle_args args;
>> +	int num, ret;
>> +
>> +	num = of_count_phandle_with_args(np, "samsung,interconnect-parent",
>> +					"#interconnect-cells");
>> +	if (num != 1)
>> +		return NULL; /* parent nodes are optional */
>> +
>> +	ret = of_parse_phandle_with_args(np, "samsung,interconnect-parent",
>> +					"#interconnect-cells", 0, &args);
>> +	if (ret < 0)
>> +		return ERR_PTR(ret);
>> +
>> +	of_node_put(args.np);
>> +
>> +	return of_icc_get_from_provider(&args);
>> +}
>> +
>> +
> 
> Nit: multiple blank lines

Fixed.

> [..]
>> +static struct icc_node *exynos_generic_icc_xlate(struct of_phandle_args *spec,
>> +						 void *data)
>> +{
>> +	struct exynos_icc_priv *priv = data;
>> +
>> +	if (spec->np != priv->dev->parent->of_node)
>> +		return ERR_PTR(-EINVAL);
>> +
>> +	return priv->node;
>> +}
>> +
>> +static int exynos_generic_icc_remove(struct platform_device *pdev)
>> +{
>> +	struct exynos_icc_priv *priv = platform_get_drvdata(pdev);
>> +	struct icc_node *parent_node, *node = priv->node;
>> +
>> +	parent_node = exynos_icc_get_parent(priv->dev->parent->of_node);
>> +	if (parent_node && !IS_ERR(parent_node))
> 
> Nit: !IS_ERR_OR_NULL?

It was left on purpose that way but I changed it now to IS_ERR_OR_NULL.

>> +		icc_link_destroy(node, parent_node);
>> +
>> +	icc_nodes_remove(&priv->provider);
>> +	icc_provider_del(&priv->provider);
>> +
>> +	return 0;
>> +}
>> +
>> +static int exynos_generic_icc_probe(struct platform_device *pdev)
>> +{
>> +	struct device *bus_dev = pdev->dev.parent;
>> +	struct exynos_icc_priv *priv;
>> +	struct icc_provider *provider;
>> +	struct icc_node *icc_node, *icc_parent_node;
>> +	int ret;
>> +
>> +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
>> +	if (!priv)
>> +		return -ENOMEM;
>> +
>> +	priv->dev = &pdev->dev;
>> +	platform_set_drvdata(pdev, priv);
>> +
>> +	provider = &priv->provider;
>> +
>> +	provider->set = exynos_generic_icc_set;
>> +	provider->aggregate = icc_std_aggregate;
>> +	provider->xlate = exynos_generic_icc_xlate;
>> +	provider->dev = bus_dev;
>> +	provider->inter_set = true;
>> +	provider->data = priv;
>> +
>> +	ret = icc_provider_add(provider);
> 
> Nit: Maybe it would be better to move this after the node is created. The
> idea is to create the nodes first and add the provider when the topology is
> populated. It's fine either way here, but i am planning to change this in
> some of the existing provider drivers.

OK, it makes the clean up path a bit less straightforward. And still we need 
to register the provider before calling icc_node_add().
I made a change as below.

--------------8<------------------
@@ -124,14 +123,14 @@ static int exynos_generic_icc_probe(struct platform_device *pdev)
 	provider->inter_set = true;
 	provider->data = priv;
 
+	icc_node = icc_node_create(pdev->id);
+	if (IS_ERR(icc_node))
+		return PTR_ERR(icc_node);
+
 	ret = icc_provider_add(provider);
-	if (ret < 0)
+	if (ret < 0) {
+		icc_node_destroy(icc_node->id);
 		return ret;
-
-	icc_node = icc_node_create(pdev->id);
-	if (IS_ERR(icc_node)) {
-		ret = PTR_ERR(icc_node);
-		goto err_prov_del;
 	}
 
 	priv->node = icc_node;
@@ -171,9 +170,7 @@ static int exynos_generic_icc_probe(struct platform_device *pdev)
 		icc_link_destroy(icc_node, icc_parent_node);
 err_node_del:
 	icc_nodes_remove(provider);
-err_prov_del:
 	icc_provider_del(provider);
-
 	return ret;
 }
--------------8<------------------


>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	icc_node = icc_node_create(pdev->id);
>> +	if (IS_ERR(icc_node)) {
>> +		ret = PTR_ERR(icc_node);
>> +		goto err_prov_del;
>> +	}
>> +
>> +	priv->node = icc_node;
>> +	icc_node->name = bus_dev->of_node->name;
>> +	icc_node->data = priv;
>> +	icc_node_add(icc_node, provider);
>> +
>> +	icc_parent_node = exynos_icc_get_parent(bus_dev->of_node);
>> +	if (IS_ERR(icc_parent_node)) {
>> +		ret = PTR_ERR(icc_parent_node);
>> +		goto err_node_del;
>> +	}
>> +	if (icc_parent_node) {
>> +		ret = icc_link_create(icc_node, icc_parent_node->id);
>> +		if (ret < 0)
>> +			goto err_node_del;
>> +	}
>> +
>> +	/*
>> +	 * Register a PM QoS request for the bus device for which also devfreq
>> +	 * functionality is registered.
>> +	 */
>> +	ret = dev_pm_qos_add_request(bus_dev, &priv->qos_req,
>> +				     DEV_PM_QOS_MIN_FREQUENCY, 0);
>> +	if (ret < 0)
>> +		goto err_link_destroy;
>> +
>> +	return 0;
>> +
>> +err_link_destroy:
>> +	if (icc_parent_node)
>> +		icc_link_destroy(icc_node, icc_parent_node);
>> +err_node_del:
>> +	icc_nodes_remove(provider);
>> +err_prov_del:
>> +	icc_provider_del(provider);
>> +
>> +	return ret;
>> +}

> All looks good to me, but it seems that the patch-set is not on
> Rob's radar currently, so please re-send and CC the DT mailing list.

Thanks, indeed I missed some mailing list when posting. I will make sure
Rob and DT ML list is on Cc, especially that I have added new "bus-width" 
property in v6.
Georgi Djakov July 2, 2020, 12:33 p.m. UTC | #7
Hi Sylwester,

On 7/2/20 15:01, Sylwester Nawrocki wrote:
> Hi Georgi,
> 
> On 01.07.2020 14:50, Georgi Djakov wrote:
>> Thanks for the patch and apologies for the delayed reply.
> 
> Thanks, no problem. It's actually just in time as I put that patchset
> aside for a while and was just about to post an update.
>  
>> On 5/29/20 19:31, Sylwester Nawrocki wrote:
>>> This patch adds a generic interconnect driver for Exynos SoCs in order
>>> to provide interconnect functionality for each "samsung,exynos-bus"
>>> compatible device.
>>>
>>> The SoC topology is a graph (or more specifically, a tree) and its
>>> edges are specified using the 'samsung,interconnect-parent' 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 dynamically at runtime. 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ń <a.swigon@samsung.com>
>>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> 
>>> +static struct icc_node *exynos_icc_get_parent(struct device_node *np)
>>> +{
>>> +	struct of_phandle_args args;
>>> +	int num, ret;
>>> +
>>> +	num = of_count_phandle_with_args(np, "samsung,interconnect-parent",
>>> +					"#interconnect-cells");
>>> +	if (num != 1)
>>> +		return NULL; /* parent nodes are optional */
>>> +
>>> +	ret = of_parse_phandle_with_args(np, "samsung,interconnect-parent",
>>> +					"#interconnect-cells", 0, &args);
>>> +	if (ret < 0)
>>> +		return ERR_PTR(ret);
>>> +
>>> +	of_node_put(args.np);
>>> +
>>> +	return of_icc_get_from_provider(&args);
>>> +}
>>> +
>>> +
>>
>> Nit: multiple blank lines
> 
> Fixed.
> 
>> [..]
>>> +static struct icc_node *exynos_generic_icc_xlate(struct of_phandle_args *spec,
>>> +						 void *data)
>>> +{
>>> +	struct exynos_icc_priv *priv = data;
>>> +
>>> +	if (spec->np != priv->dev->parent->of_node)
>>> +		return ERR_PTR(-EINVAL);
>>> +
>>> +	return priv->node;
>>> +}
>>> +
>>> +static int exynos_generic_icc_remove(struct platform_device *pdev)
>>> +{
>>> +	struct exynos_icc_priv *priv = platform_get_drvdata(pdev);
>>> +	struct icc_node *parent_node, *node = priv->node;
>>> +
>>> +	parent_node = exynos_icc_get_parent(priv->dev->parent->of_node);
>>> +	if (parent_node && !IS_ERR(parent_node))
>>
>> Nit: !IS_ERR_OR_NULL?
> 
> It was left on purpose that way but I changed it now to IS_ERR_OR_NULL.

Well, i have no strong opinion on that, it's up to you.

>>> +		icc_link_destroy(node, parent_node);
>>> +
>>> +	icc_nodes_remove(&priv->provider);
>>> +	icc_provider_del(&priv->provider);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int exynos_generic_icc_probe(struct platform_device *pdev)
>>> +{
>>> +	struct device *bus_dev = pdev->dev.parent;
>>> +	struct exynos_icc_priv *priv;
>>> +	struct icc_provider *provider;
>>> +	struct icc_node *icc_node, *icc_parent_node;
>>> +	int ret;
>>> +
>>> +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
>>> +	if (!priv)
>>> +		return -ENOMEM;
>>> +
>>> +	priv->dev = &pdev->dev;
>>> +	platform_set_drvdata(pdev, priv);
>>> +
>>> +	provider = &priv->provider;
>>> +
>>> +	provider->set = exynos_generic_icc_set;
>>> +	provider->aggregate = icc_std_aggregate;
>>> +	provider->xlate = exynos_generic_icc_xlate;
>>> +	provider->dev = bus_dev;
>>> +	provider->inter_set = true;
>>> +	provider->data = priv;
>>> +
>>> +	ret = icc_provider_add(provider);
>>
>> Nit: Maybe it would be better to move this after the node is created. The
>> idea is to create the nodes first and add the provider when the topology is
>> populated. It's fine either way here, but i am planning to change this in
>> some of the existing provider drivers.
> 
> OK, it makes the clean up path a bit less straightforward. And still we need 
> to register the provider before calling icc_node_add().
> I made a change as below.
> 
> --------------8<------------------
> @@ -124,14 +123,14 @@ static int exynos_generic_icc_probe(struct platform_device *pdev)
>  	provider->inter_set = true;
>  	provider->data = priv;
>  
> +	icc_node = icc_node_create(pdev->id);
> +	if (IS_ERR(icc_node))
> +		return PTR_ERR(icc_node);
> +
>  	ret = icc_provider_add(provider);
> -	if (ret < 0)
> +	if (ret < 0) {
> +		icc_node_destroy(icc_node->id);
>  		return ret;
> -
> -	icc_node = icc_node_create(pdev->id);
> -	if (IS_ERR(icc_node)) {
> -		ret = PTR_ERR(icc_node);
> -		goto err_prov_del;
>  	}
>  
>  	priv->node = icc_node;
> @@ -171,9 +170,7 @@ static int exynos_generic_icc_probe(struct platform_device *pdev)
>  		icc_link_destroy(icc_node, icc_parent_node);
>  err_node_del:
>  	icc_nodes_remove(provider);
> -err_prov_del:
>  	icc_provider_del(provider);
> -
>  	return ret;
>  }
> --------------8<------------------

Actually i need to post some patches first, so maybe keep it as is for now and
we will update it afterwards. Sorry for the confusion.

> 
> 
>>> +	if (ret < 0)
>>> +		return ret;
>>> +
>>> +	icc_node = icc_node_create(pdev->id);
>>> +	if (IS_ERR(icc_node)) {
>>> +		ret = PTR_ERR(icc_node);
>>> +		goto err_prov_del;
>>> +	}
>>> +
>>> +	priv->node = icc_node;
>>> +	icc_node->name = bus_dev->of_node->name;
>>> +	icc_node->data = priv;
>>> +	icc_node_add(icc_node, provider);
>>> +
>>> +	icc_parent_node = exynos_icc_get_parent(bus_dev->of_node);
>>> +	if (IS_ERR(icc_parent_node)) {
>>> +		ret = PTR_ERR(icc_parent_node);
>>> +		goto err_node_del;
>>> +	}
>>> +	if (icc_parent_node) {
>>> +		ret = icc_link_create(icc_node, icc_parent_node->id);
>>> +		if (ret < 0)
>>> +			goto err_node_del;
>>> +	}
>>> +
>>> +	/*
>>> +	 * Register a PM QoS request for the bus device for which also devfreq
>>> +	 * functionality is registered.
>>> +	 */
>>> +	ret = dev_pm_qos_add_request(bus_dev, &priv->qos_req,
>>> +				     DEV_PM_QOS_MIN_FREQUENCY, 0);
>>> +	if (ret < 0)
>>> +		goto err_link_destroy;
>>> +
>>> +	return 0;
>>> +
>>> +err_link_destroy:
>>> +	if (icc_parent_node)
>>> +		icc_link_destroy(icc_node, icc_parent_node);
>>> +err_node_del:
>>> +	icc_nodes_remove(provider);
>>> +err_prov_del:
>>> +	icc_provider_del(provider);
>>> +
>>> +	return ret;
>>> +}
> 
>> All looks good to me, but it seems that the patch-set is not on
>> Rob's radar currently, so please re-send and CC the DT mailing list.
> 
> Thanks, indeed I missed some mailing list when posting. I will make sure
> Rob and DT ML list is on Cc, especially that I have added new "bus-width" 
> property in v6.

Ok, good. I have been thinking about bus-width and we might want to make it
even a generic DT property if there are multiple platforms which want to
use it - maybe if the bus-width is the same across the whole interconnect
provider. But as most of the existing drivers have different bus-widths, i
haven't done it yet, but let's see and start a discussion.

Thanks,
Georgi
Sylwester Nawrocki July 2, 2020, 2:24 p.m. UTC | #8
On 02.07.2020 14:33, Georgi Djakov wrote:
> On 7/2/20 15:01, Sylwester Nawrocki wrote:
>> On 01.07.2020 14:50, Georgi Djakov wrote:
>>> On 5/29/20 19:31, Sylwester Nawrocki wrote:

>>>> +static int exynos_generic_icc_remove(struct platform_device *pdev)
>>>> +{
>>>> +	struct exynos_icc_priv *priv = platform_get_drvdata(pdev);
>>>> +	struct icc_node *parent_node, *node = priv->node;
>>>> +
>>>> +	parent_node = exynos_icc_get_parent(priv->dev->parent->of_node);
>>>> +	if (parent_node && !IS_ERR(parent_node))
>>>
>>> Nit: !IS_ERR_OR_NULL?
>>
>> It was left on purpose that way but I changed it now to IS_ERR_OR_NULL.
> 
> Well, i have no strong opinion on that, it's up to you.

I will leave it as you suggested, otherwise we are likely to see
"clean up" patches sooner or later.
 
>>>> +		icc_link_destroy(node, parent_node);
>>>> +
>>>> +	icc_nodes_remove(&priv->provider);
>>>> +	icc_provider_del(&priv->provider);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int exynos_generic_icc_probe(struct platform_device *pdev)
>>>> +{
>>>> +	struct device *bus_dev = pdev->dev.parent;
>>>> +	struct exynos_icc_priv *priv;
>>>> +	struct icc_provider *provider;
>>>> +	struct icc_node *icc_node, *icc_parent_node;
>>>> +	int ret;
>>>> +
>>>> +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
>>>> +	if (!priv)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	priv->dev = &pdev->dev;
>>>> +	platform_set_drvdata(pdev, priv);
>>>> +
>>>> +	provider = &priv->provider;
>>>> +
>>>> +	provider->set = exynos_generic_icc_set;
>>>> +	provider->aggregate = icc_std_aggregate;
>>>> +	provider->xlate = exynos_generic_icc_xlate;
>>>> +	provider->dev = bus_dev;
>>>> +	provider->inter_set = true;
>>>> +	provider->data = priv;
>>>> +
>>>> +	ret = icc_provider_add(provider);
>>>
>>> Nit: Maybe it would be better to move this after the node is created. The
>>> idea is to create the nodes first and add the provider when the topology is
>>> populated. It's fine either way here, but i am planning to change this in
>>> some of the existing provider drivers.
>>
>> OK, it makes the clean up path a bit less straightforward. And still we need 
>> to register the provider before calling icc_node_add().
>> I made a change as below.
>>
>> --------------8<------------------
>> @@ -124,14 +123,14 @@ static int exynos_generic_icc_probe(struct platform_device *pdev)
>>  	provider->inter_set = true;
>>  	provider->data = priv;
>>  
>> +	icc_node = icc_node_create(pdev->id);
>> +	if (IS_ERR(icc_node))
>> +		return PTR_ERR(icc_node);
>> +
>>  	ret = icc_provider_add(provider);
>> -	if (ret < 0)
>> +	if (ret < 0) {
>> +		icc_node_destroy(icc_node->id);
>>  		return ret;
>> -
>> -	icc_node = icc_node_create(pdev->id);
>> -	if (IS_ERR(icc_node)) {
>> -		ret = PTR_ERR(icc_node);
>> -		goto err_prov_del;
>>  	}
>>  
>>  	priv->node = icc_node;
>> @@ -171,9 +170,7 @@ static int exynos_generic_icc_probe(struct platform_device *pdev)
>>  		icc_link_destroy(icc_node, icc_parent_node);
>>  err_node_del:
>>  	icc_nodes_remove(provider);
>> -err_prov_del:
>>  	icc_provider_del(provider);
>> -
>>  	return ret;
>>  }
>> --------------8<------------------
> 
> Actually i need to post some patches first, so maybe keep it as is for now and
> we will update it afterwards. Sorry for the confusion.

OK, anyway this helped me to remove a memory leak, which was there since
icc_nodes_remove() was used before a call to icc_node_add() in order 
to free the node allocated with icc_node_create(), instead of 
icc_node_destroy().

>>> All looks good to me, but it seems that the patch-set is not on
>>> Rob's radar currently, so please re-send and CC the DT mailing list.
>>
>> Thanks, indeed I missed some mailing list when posting. I will make sure
>> Rob and DT ML list is on Cc, especially that I have added new "bus-width" 
>> property in v6.
> 
> Ok, good. I have been thinking about bus-width and we might want to make it
> even a generic DT property if there are multiple platforms which want to
> use it - maybe if the bus-width is the same across the whole interconnect
> provider. But as most of the existing drivers have different bus-widths, i
> haven't done it yet, but let's see and start a discussion.

I see, it sounds good to me. Having an array property to allow specifying
bus width for each node is probably not so good idea.

Patch
diff mbox series

diff --git a/drivers/interconnect/Kconfig b/drivers/interconnect/Kconfig
index 5b7204e..eca6eda 100644
--- a/drivers/interconnect/Kconfig
+++ b/drivers/interconnect/Kconfig
@@ -11,6 +11,7 @@  menuconfig INTERCONNECT
 
 if INTERCONNECT
 
+source "drivers/interconnect/exynos/Kconfig"
 source "drivers/interconnect/imx/Kconfig"
 source "drivers/interconnect/qcom/Kconfig"
 
diff --git a/drivers/interconnect/Makefile b/drivers/interconnect/Makefile
index 4825c28..2ba1de6 100644
--- a/drivers/interconnect/Makefile
+++ b/drivers/interconnect/Makefile
@@ -4,5 +4,6 @@  CFLAGS_core.o				:= -I$(src)
 icc-core-objs				:= core.o
 
 obj-$(CONFIG_INTERCONNECT)		+= icc-core.o
+obj-$(CONFIG_INTERCONNECT_EXYNOS)	+= exynos/
 obj-$(CONFIG_INTERCONNECT_IMX)		+= imx/
 obj-$(CONFIG_INTERCONNECT_QCOM)		+= qcom/
diff --git a/drivers/interconnect/exynos/Kconfig b/drivers/interconnect/exynos/Kconfig
new file mode 100644
index 0000000..e51e52e
--- /dev/null
+++ b/drivers/interconnect/exynos/Kconfig
@@ -0,0 +1,6 @@ 
+# SPDX-License-Identifier: GPL-2.0-only
+config INTERCONNECT_EXYNOS
+	tristate "Exynos generic interconnect driver"
+	depends on ARCH_EXYNOS || COMPILE_TEST
+	help
+	  Generic interconnect driver for Exynos SoCs.
diff --git a/drivers/interconnect/exynos/Makefile b/drivers/interconnect/exynos/Makefile
new file mode 100644
index 0000000..e19d1df
--- /dev/null
+++ b/drivers/interconnect/exynos/Makefile
@@ -0,0 +1,4 @@ 
+# SPDX-License-Identifier: GPL-2.0
+exynos-interconnect-objs		:= exynos.o
+
+obj-$(CONFIG_INTERCONNECT_EXYNOS)	+= exynos-interconnect.o
diff --git a/drivers/interconnect/exynos/exynos.c b/drivers/interconnect/exynos/exynos.c
new file mode 100644
index 0000000..8278194
--- /dev/null
+++ b/drivers/interconnect/exynos/exynos.c
@@ -0,0 +1,185 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Exynos generic interconnect provider driver
+ *
+ * Copyright (c) 2020 Samsung Electronics Co., Ltd.
+ *
+ * Authors: Artur Świgoń <a.swigon@samsung.com>
+ *          Sylwester Nawrocki <s.nawrocki@samsung.com>
+ */
+#include <linux/device.h>
+#include <linux/interconnect-provider.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pm_qos.h>
+
+#define kbps_to_khz(x) ((x) / 8)
+
+struct exynos_icc_priv {
+	struct device *dev;
+
+	/* One interconnect node per provider */
+	struct icc_provider provider;
+	struct icc_node *node;
+
+	struct dev_pm_qos_request qos_req;
+};
+
+static struct icc_node *exynos_icc_get_parent(struct device_node *np)
+{
+	struct of_phandle_args args;
+	int num, ret;
+
+	num = of_count_phandle_with_args(np, "samsung,interconnect-parent",
+					"#interconnect-cells");
+	if (num != 1)
+		return NULL; /* parent nodes are optional */
+
+	ret = of_parse_phandle_with_args(np, "samsung,interconnect-parent",
+					"#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_generic_icc_set(struct icc_node *src, struct icc_node *dst)
+{
+	struct exynos_icc_priv *src_priv = src->data, *dst_priv = dst->data;
+	s32 src_freq = kbps_to_khz(max(src->avg_bw, src->peak_bw));
+	s32 dst_freq = kbps_to_khz(max(dst->avg_bw, dst->peak_bw));
+	int ret;
+
+	ret = dev_pm_qos_update_request(&src_priv->qos_req, src_freq);
+	if (ret < 0) {
+		dev_err(src_priv->dev, "failed to update PM QoS of %s\n",
+			src->name);
+		return ret;
+	}
+
+	ret = dev_pm_qos_update_request(&dst_priv->qos_req, dst_freq);
+	if (ret < 0) {
+		dev_err(dst_priv->dev, "failed to update PM QoS of %s\n",
+			dst->name);
+		return ret;
+	}
+
+	return 0;
+}
+
+static struct icc_node *exynos_generic_icc_xlate(struct of_phandle_args *spec,
+						 void *data)
+{
+	struct exynos_icc_priv *priv = data;
+
+	if (spec->np != priv->dev->parent->of_node)
+		return ERR_PTR(-EINVAL);
+
+	return priv->node;
+}
+
+static int exynos_generic_icc_remove(struct platform_device *pdev)
+{
+	struct exynos_icc_priv *priv = platform_get_drvdata(pdev);
+	struct icc_node *parent_node, *node = priv->node;
+
+	parent_node = exynos_icc_get_parent(priv->dev->parent->of_node);
+	if (parent_node && !IS_ERR(parent_node))
+		icc_link_destroy(node, parent_node);
+
+	icc_nodes_remove(&priv->provider);
+	icc_provider_del(&priv->provider);
+
+	return 0;
+}
+
+static int exynos_generic_icc_probe(struct platform_device *pdev)
+{
+	struct device *bus_dev = pdev->dev.parent;
+	struct exynos_icc_priv *priv;
+	struct icc_provider *provider;
+	struct icc_node *icc_node, *icc_parent_node;
+	int ret;
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->dev = &pdev->dev;
+	platform_set_drvdata(pdev, priv);
+
+	provider = &priv->provider;
+
+	provider->set = exynos_generic_icc_set;
+	provider->aggregate = icc_std_aggregate;
+	provider->xlate = exynos_generic_icc_xlate;
+	provider->dev = bus_dev;
+	provider->inter_set = true;
+	provider->data = priv;
+
+	ret = icc_provider_add(provider);
+	if (ret < 0)
+		return ret;
+
+	icc_node = icc_node_create(pdev->id);
+	if (IS_ERR(icc_node)) {
+		ret = PTR_ERR(icc_node);
+		goto err_prov_del;
+	}
+
+	priv->node = icc_node;
+	icc_node->name = bus_dev->of_node->name;
+	icc_node->data = priv;
+	icc_node_add(icc_node, provider);
+
+	icc_parent_node = exynos_icc_get_parent(bus_dev->of_node);
+	if (IS_ERR(icc_parent_node)) {
+		ret = PTR_ERR(icc_parent_node);
+		goto err_node_del;
+	}
+	if (icc_parent_node) {
+		ret = icc_link_create(icc_node, icc_parent_node->id);
+		if (ret < 0)
+			goto err_node_del;
+	}
+
+	/*
+	 * Register a PM QoS request for the bus device for which also devfreq
+	 * functionality is registered.
+	 */
+	ret = dev_pm_qos_add_request(bus_dev, &priv->qos_req,
+				     DEV_PM_QOS_MIN_FREQUENCY, 0);
+	if (ret < 0)
+		goto err_link_destroy;
+
+	return 0;
+
+err_link_destroy:
+	if (icc_parent_node)
+		icc_link_destroy(icc_node, icc_parent_node);
+err_node_del:
+	icc_nodes_remove(provider);
+err_prov_del:
+	icc_provider_del(provider);
+
+	return ret;
+}
+
+static struct platform_driver exynos_generic_icc_driver = {
+	.driver = {
+		.name = "exynos-generic-icc",
+	},
+	.probe = exynos_generic_icc_probe,
+	.remove = exynos_generic_icc_remove,
+};
+module_platform_driver(exynos_generic_icc_driver);
+
+MODULE_DESCRIPTION("Exynos generic interconnect driver");
+MODULE_AUTHOR("Artur Świgoń <a.swigon@samsung.com>");
+MODULE_AUTHOR("Sylwester Nawrocki <s.nawrocki@samsung.com>");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:exynos-generic-icc");