[v8,2/7] interconnect: Add generic interconnect driver for Exynos SoCs

Commit Message

Sylwester Nawrocki/Kernel \(PLT\) /SRPOL/Staff Engineer/Samsung Electronics Nov. 4, 2020, 10:36 a.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 described by specifying in the 'interconnects' property
the interconnect consumer path for each interconnect provider DT node.

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 hard coded 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.

The samsung,data-clk-ratio DT property is used to specify the ratio
of the interconect bandwidth to the minimum data clock frequency
for each bus.

Due to unspecified relative probing order, -EPROBE_DEFER may be
propagated to ensure that the parent is probed before its children.

Signed-off-by: Artur Świgoń <[e-mail]>
Signed-off-by: Sylwester Nawrocki <[e-mail]>
---
Changes for v8:
 - renamed drivers/interconnect/exynos to drivers/interconnect/samsung,
 - added missing driver sync_state callback assignment.

Changes for v7:
 - adjusted to the DT property changes: "interconnects" instead
   of "samsung,interconnect-parent", "samsung,data-clk-ratio"
   instead of "bus-width",
 - adaptation to of_icc_get_from_provider() function changes
   in v5.10-rc1.

Changes for v6:
 - corrected of_node dereferencing in exynos_icc_get_parent()
   function,
 - corrected initialization of icc_node->name so as to avoid
   direct of_node->name dereferencing,
 - added parsing of bus-width DT property.

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/samsung/Kconfig  |  13 +++
 drivers/interconnect/samsung/Makefile |   4 +
 drivers/interconnect/samsung/exynos.c | 199 ++++++++++++++++++++++++++++++++++
 5 files changed, 218 insertions(+)
 create mode 100644 drivers/interconnect/samsung/Kconfig
 create mode 100644 drivers/interconnect/samsung/Makefile
 create mode 100644 drivers/interconnect/samsung/exynos.c

Comments

Chanwoo Choi Nov. 4, 2020, 12:28 p.m. UTC | #1

Hi Sylwester,

On Wed, Nov 4, 2020 at 7:37 PM Sylwester Nawrocki
<[e-mail]> 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 described by specifying in the 'interconnects' property
> the interconnect consumer path for each interconnect provider DT node.
>
> 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 hard coded 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.
>
> The samsung,data-clk-ratio DT property is used to specify the ratio
> of the interconect bandwidth to the minimum data clock frequency
> for each bus.
>
> Due to unspecified relative probing order, -EPROBE_DEFER may be
> propagated to ensure that the parent is probed before its children.
>
> Signed-off-by: Artur Świgoń <[e-mail]>
> Signed-off-by: Sylwester Nawrocki <[e-mail]>
> ---
> Changes for v8:
>  - renamed drivers/interconnect/exynos to drivers/interconnect/samsung,
>  - added missing driver sync_state callback assignment.
>
> Changes for v7:
>  - adjusted to the DT property changes: "interconnects" instead
>    of "samsung,interconnect-parent", "samsung,data-clk-ratio"
>    instead of "bus-width",
>  - adaptation to of_icc_get_from_provider() function changes
>    in v5.10-rc1.
>
> Changes for v6:
>  - corrected of_node dereferencing in exynos_icc_get_parent()
>    function,
>  - corrected initialization of icc_node->name so as to avoid
>    direct of_node->name dereferencing,
>  - added parsing of bus-width DT property.
>
> 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/samsung/Kconfig  |  13 +++
>  drivers/interconnect/samsung/Makefile |   4 +
>  drivers/interconnect/samsung/exynos.c | 199 ++++++++++++++++++++++++++++++++++
>  5 files changed, 218 insertions(+)
>  create mode 100644 drivers/interconnect/samsung/Kconfig
>  create mode 100644 drivers/interconnect/samsung/Makefile
>  create mode 100644 drivers/interconnect/samsung/exynos.c
>
> diff --git a/drivers/interconnect/Kconfig b/drivers/interconnect/Kconfig
> index 5b7204e..d637a89 100644
> --- a/drivers/interconnect/Kconfig
> +++ b/drivers/interconnect/Kconfig
> @@ -13,5 +13,6 @@ if INTERCONNECT
>
>  source "drivers/interconnect/imx/Kconfig"
>  source "drivers/interconnect/qcom/Kconfig"
> +source "drivers/interconnect/samsung/Kconfig"
>
>  endif
> diff --git a/drivers/interconnect/Makefile b/drivers/interconnect/Makefile
> index d203520..c2f9e9d 100644
> --- a/drivers/interconnect/Makefile
> +++ b/drivers/interconnect/Makefile
> @@ -6,3 +6,4 @@ icc-core-objs                           := core.o bulk.o
>  obj-$(CONFIG_INTERCONNECT)             += icc-core.o
>  obj-$(CONFIG_INTERCONNECT_IMX)         += imx/
>  obj-$(CONFIG_INTERCONNECT_QCOM)                += qcom/
> +obj-$(CONFIG_INTERCONNECT_SAMSUNG)     += samsung/
> \ No newline at end of file
> diff --git a/drivers/interconnect/samsung/Kconfig b/drivers/interconnect/samsung/Kconfig
> new file mode 100644
> index 0000000..508ed64
> --- /dev/null
> +++ b/drivers/interconnect/samsung/Kconfig
> @@ -0,0 +1,13 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +config INTERCONNECT_SAMSUNG
> +       bool "Samsung interconnect drivers"
> +       depends on ARCH_EXYNOS || COMPILE_TEST
> +       help
> +         Interconnect drivers for Samsung SoCs.
> +
> +
> +config INTERCONNECT_EXYNOS
> +       tristate "Exynos generic interconnect driver"
> +       depends on INTERCONNECT_SAMSUNG
> +       help
> +         Generic interconnect driver for Exynos SoCs.
> diff --git a/drivers/interconnect/samsung/Makefile b/drivers/interconnect/samsung/Makefile
> new file mode 100644
> index 0000000..e19d1df
> --- /dev/null
> +++ b/drivers/interconnect/samsung/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/samsung/exynos.c b/drivers/interconnect/samsung/exynos.c
> new file mode 100644
> index 0000000..6559d8c
> --- /dev/null
> +++ b/drivers/interconnect/samsung/exynos.c
> @@ -0,0 +1,199 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Exynos generic interconnect provider driver
> + *
> + * Copyright (c) 2020 Samsung Electronics Co., Ltd.
> + *
> + * Authors: Artur Świgoń <[e-mail]>
> + *          Sylwester Nawrocki <[e-mail]>
> + */
> +#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>
> +#include <linux/slab.h>
> +
> +#define EXYNOS_ICC_DEFAULT_BUS_CLK_RATIO       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;
> +       u32 bus_clk_ratio;
> +};
> +
> +static struct icc_node *exynos_icc_get_parent(struct device_node *np)
> +{
> +       struct of_phandle_args args;
> +       struct icc_node_data *icc_node_data;
> +       struct icc_node *icc_node;
> +       int num, ret;
> +
> +       num = of_count_phandle_with_args(np, "interconnects",
> +                                        "#interconnect-cells");
> +       if (num < 1)
> +               return NULL; /* parent nodes are optional */
> +
> +       /* Get the interconnect target node */
> +       ret = of_parse_phandle_with_args(np, "interconnects",
> +                                       "#interconnect-cells", 0, &args);
> +       if (ret < 0)
> +               return ERR_PTR(ret);
> +
> +       icc_node_data = of_icc_get_from_provider(&args);
> +       of_node_put(args.np);
> +
> +       if (IS_ERR(icc_node_data))
> +               return ERR_CAST(icc_node_data);
> +
> +       icc_node = icc_node_data->node;
> +       kfree(icc_node_data);
> +
> +       return icc_node;
> +}
> +
> +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 = max(src->avg_bw, src->peak_bw) / src_priv->bus_clk_ratio;
> +       s32 dst_freq = max(dst->avg_bw, dst->peak_bw) / dst_priv->bus_clk_ratio;
> +       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 (src)\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 (dst)\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 = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%pOFn",
> +                                       bus_dev->of_node);
> +       if (of_property_read_u32(bus_dev->of_node, "samsung,data-clock-ratio",
> +                                &priv->bus_clk_ratio))
> +               priv->bus_clk_ratio = EXYNOS_ICC_DEFAULT_BUS_CLK_RATIO;
> +
> +       /*
> +        * Register a PM QoS request for the parent (devfreq) device.
> +        */
> +       ret = dev_pm_qos_add_request(bus_dev, &priv->qos_req,
> +                                    DEV_PM_QOS_MIN_FREQUENCY, 0);
> +       if (ret < 0)
> +               goto err_node_del;
> +
> +       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_pmqos_del;
> +       }
> +       if (icc_parent_node) {
> +               ret = icc_link_create(icc_node, icc_parent_node->id);
> +               if (ret < 0)
> +                       goto err_pmqos_del;
> +       }
> +
> +       return 0;
> +
> +err_pmqos_del:
> +       dev_pm_qos_remove_request(&priv->qos_req);
> +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",
> +               .sync_state = icc_sync_state,
> +       },
> +       .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ń <[e-mail]>");
> +MODULE_AUTHOR("Sylwester Nawrocki <[e-mail]>");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:exynos-generic-icc");
> --
> 2.7.4
>
> _______________________________________________
> dri-devel mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Reviewed-by: Chanwoo Choi <[e-mail]>

Krzysztof Kozlowski Nov. 4, 2020, 12:37 p.m. UTC | #2

On Wed, Nov 04, 2020 at 11:36:52AM +0100, 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 described by specifying in the 'interconnects' property
> the interconnect consumer path for each interconnect provider DT node.
> 
> 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 hard coded 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.
> 
> The samsung,data-clk-ratio DT property is used to specify the ratio
> of the interconect bandwidth to the minimum data clock frequency
> for each bus.
> 
> Due to unspecified relative probing order, -EPROBE_DEFER may be
> propagated to ensure that the parent is probed before its children.
> 
> Signed-off-by: Artur Świgoń <[e-mail]>
> Signed-off-by: Sylwester Nawrocki <[e-mail]>
> ---
> Changes for v8:
>  - renamed drivers/interconnect/exynos to drivers/interconnect/samsung,
>  - added missing driver sync_state callback assignment.
> 
> Changes for v7:
>  - adjusted to the DT property changes: "interconnects" instead
>    of "samsung,interconnect-parent", "samsung,data-clk-ratio"
>    instead of "bus-width",
>  - adaptation to of_icc_get_from_provider() function changes
>    in v5.10-rc1.
> 
> Changes for v6:
>  - corrected of_node dereferencing in exynos_icc_get_parent()
>    function,
>  - corrected initialization of icc_node->name so as to avoid
>    direct of_node->name dereferencing,
>  - added parsing of bus-width DT property.
> 
> 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/samsung/Kconfig  |  13 +++
>  drivers/interconnect/samsung/Makefile |   4 +
>  drivers/interconnect/samsung/exynos.c | 199 ++++++++++++++++++++++++++++++++++
>  5 files changed, 218 insertions(+)
>  create mode 100644 drivers/interconnect/samsung/Kconfig
>  create mode 100644 drivers/interconnect/samsung/Makefile
>  create mode 100644 drivers/interconnect/samsung/exynos.c
> 
> diff --git a/drivers/interconnect/Kconfig b/drivers/interconnect/Kconfig
> index 5b7204e..d637a89 100644
> --- a/drivers/interconnect/Kconfig
> +++ b/drivers/interconnect/Kconfig
> @@ -13,5 +13,6 @@ if INTERCONNECT
>  
>  source "drivers/interconnect/imx/Kconfig"
>  source "drivers/interconnect/qcom/Kconfig"
> +source "drivers/interconnect/samsung/Kconfig"
>  
>  endif
> diff --git a/drivers/interconnect/Makefile b/drivers/interconnect/Makefile
> index d203520..c2f9e9d 100644
> --- a/drivers/interconnect/Makefile
> +++ b/drivers/interconnect/Makefile
> @@ -6,3 +6,4 @@ icc-core-objs				:= core.o bulk.o
>  obj-$(CONFIG_INTERCONNECT)		+= icc-core.o
>  obj-$(CONFIG_INTERCONNECT_IMX)		+= imx/
>  obj-$(CONFIG_INTERCONNECT_QCOM)		+= qcom/
> +obj-$(CONFIG_INTERCONNECT_SAMSUNG)	+= samsung/
> \ No newline at end of file

This needs a fix.

> diff --git a/drivers/interconnect/samsung/Kconfig b/drivers/interconnect/samsung/Kconfig
> new file mode 100644
> index 0000000..508ed64
> --- /dev/null
> +++ b/drivers/interconnect/samsung/Kconfig
> @@ -0,0 +1,13 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +config INTERCONNECT_SAMSUNG
> +	bool "Samsung interconnect drivers"

"Samsung SoC interconnect drivers"

> +	depends on ARCH_EXYNOS || COMPILE_TEST

Don't the depend on INTERCONNECT?

> +	help
> +	  Interconnect drivers for Samsung SoCs.
> +
> +

One line break

> +config INTERCONNECT_EXYNOS
> +	tristate "Exynos generic interconnect driver"
> +	depends on INTERCONNECT_SAMSUNG

How about:
default y if ARCH_EXYNOS

> +	help
> +	  Generic interconnect driver for Exynos SoCs.
> diff --git a/drivers/interconnect/samsung/Makefile b/drivers/interconnect/samsung/Makefile
> new file mode 100644
> index 0000000..e19d1df
> --- /dev/null
> +++ b/drivers/interconnect/samsung/Makefile
> @@ -0,0 +1,4 @@
> +# SPDX-License-Identifier: GPL-2.0
> +exynos-interconnect-objs		:= exynos.o

What is this line for?

Best regards,
Krzysztof


> +
> +obj-$(CONFIG_INTERCONNECT_EXYNOS)	+= exynos-interconnect.o
> diff --git a/drivers/interconnect/samsung/exynos.c b/drivers/interconnect/samsung/exynos.c
> new file mode 100644
> index 0000000..6559d8c

Sylwester Nawrocki/Kernel \(PLT\) /SRPOL/Staff Engineer/Samsung Electronics Nov. 4, 2020, 1:22 p.m. UTC | #3

On 04.11.2020 13:37, Krzysztof Kozlowski wrote:
> On Wed, Nov 04, 2020 at 11:36:52AM +0100, Sylwester Nawrocki wrote:

>> diff --git a/drivers/interconnect/Makefile b/drivers/interconnect/Makefile
>> index d203520..c2f9e9d 100644
>> --- a/drivers/interconnect/Makefile
>> +++ b/drivers/interconnect/Makefile
>> @@ -6,3 +6,4 @@ icc-core-objs				:= core.o bulk.o
>>  obj-$(CONFIG_INTERCONNECT)		+= icc-core.o
>>  obj-$(CONFIG_INTERCONNECT_IMX)		+= imx/
>>  obj-$(CONFIG_INTERCONNECT_QCOM)		+= qcom/
>> +obj-$(CONFIG_INTERCONNECT_SAMSUNG)	+= samsung/
>> \ No newline at end of file
> 
> This needs a fix.

Corrected, thanks for pointing out.
 
>> diff --git a/drivers/interconnect/samsung/Kconfig b/drivers/interconnect/samsung/Kconfig
>> new file mode 100644
>> index 0000000..508ed64
>> --- /dev/null
>> +++ b/drivers/interconnect/samsung/Kconfig
>> @@ -0,0 +1,13 @@
>> +# SPDX-License-Identifier: GPL-2.0-only
>> +config INTERCONNECT_SAMSUNG
>> +	bool "Samsung interconnect drivers"
> 
> "Samsung SoC interconnect drivers"

Changed.

>> +	depends on ARCH_EXYNOS || COMPILE_TEST
> 
> Don't the depend on INTERCONNECT?

This file gets included only if INTERCONNECT is enabled, see
higher level Kconfig file.
 
>> +	help
>> +	  Interconnect drivers for Samsung SoCs.
>> +
>> +
> 
> One line break

Fixed.

>> +config INTERCONNECT_EXYNOS
>> +	tristate "Exynos generic interconnect driver"
>> +	depends on INTERCONNECT_SAMSUNG
> 
> How about:
> default y if ARCH_EXYNOS

OK, added.

>> +	help
>> +	  Generic interconnect driver for Exynos SoCs.
>> diff --git a/drivers/interconnect/samsung/Makefile b/drivers/interconnect/samsung/Makefile
>> new file mode 100644
>> index 0000000..e19d1df
>> --- /dev/null
>> +++ b/drivers/interconnect/samsung/Makefile
>> @@ -0,0 +1,4 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +exynos-interconnect-objs		:= exynos.o
> 
> What is this line for?
 
That allows to change the module name, so it's exynos-interconnect.ko
rather than just exynos.c. It's done similarly for other SoCs in 
the subsystem.

>> +obj-$(CONFIG_INTERCONNECT_EXYNOS)	+= exynos-interconnect.o

Krzysztof Kozlowski Nov. 4, 2020, 2:06 p.m. UTC | #4

On Wed, Nov 04, 2020 at 02:22:37PM +0100, Sylwester Nawrocki wrote:
> On 04.11.2020 13:37, Krzysztof Kozlowski wrote:
> > On Wed, Nov 04, 2020 at 11:36:52AM +0100, Sylwester Nawrocki wrote:
> 
> >> diff --git a/drivers/interconnect/Makefile b/drivers/interconnect/Makefile
> >> index d203520..c2f9e9d 100644
> >> --- a/drivers/interconnect/Makefile
> >> +++ b/drivers/interconnect/Makefile
> >> @@ -6,3 +6,4 @@ icc-core-objs				:= core.o bulk.o
> >>  obj-$(CONFIG_INTERCONNECT)		+= icc-core.o
> >>  obj-$(CONFIG_INTERCONNECT_IMX)		+= imx/
> >>  obj-$(CONFIG_INTERCONNECT_QCOM)		+= qcom/
> >> +obj-$(CONFIG_INTERCONNECT_SAMSUNG)	+= samsung/
> >> \ No newline at end of file
> > 
> > This needs a fix.
> 
> Corrected, thanks for pointing out.
>  
> >> diff --git a/drivers/interconnect/samsung/Kconfig b/drivers/interconnect/samsung/Kconfig
> >> new file mode 100644
> >> index 0000000..508ed64
> >> --- /dev/null
> >> +++ b/drivers/interconnect/samsung/Kconfig
> >> @@ -0,0 +1,13 @@
> >> +# SPDX-License-Identifier: GPL-2.0-only
> >> +config INTERCONNECT_SAMSUNG
> >> +	bool "Samsung interconnect drivers"
> > 
> > "Samsung SoC interconnect drivers"
> 
> Changed.
> 
> >> +	depends on ARCH_EXYNOS || COMPILE_TEST
> > 
> > Don't the depend on INTERCONNECT?
> 
> This file gets included only if INTERCONNECT is enabled, see
> higher level Kconfig file.

I missed the include part, looks good.

>  
> >> +	help
> >> +	  Interconnect drivers for Samsung SoCs.
> >> +
> >> +
> > 
> > One line break
> 
> Fixed.
> 
> >> +config INTERCONNECT_EXYNOS
> >> +	tristate "Exynos generic interconnect driver"
> >> +	depends on INTERCONNECT_SAMSUNG
> > 
> > How about:
> > default y if ARCH_EXYNOS
> 
> OK, added.
> 
> >> +	help
> >> +	  Generic interconnect driver for Exynos SoCs.
> >> diff --git a/drivers/interconnect/samsung/Makefile b/drivers/interconnect/samsung/Makefile
> >> new file mode 100644
> >> index 0000000..e19d1df
> >> --- /dev/null
> >> +++ b/drivers/interconnect/samsung/Makefile
> >> @@ -0,0 +1,4 @@
> >> +# SPDX-License-Identifier: GPL-2.0
> >> +exynos-interconnect-objs		:= exynos.o
> > 
> > What is this line for?
>  
> That allows to change the module name, so it's exynos-interconnect.ko
> rather than just exynos.c. It's done similarly for other SoCs in 
> the subsystem.

Thanks, makes sense.

Best regards,
Krzysztof

Patch

diff --git a/drivers/interconnect/Kconfig b/drivers/interconnect/Kconfig
index 5b7204e..d637a89 100644
--- a/drivers/interconnect/Kconfig
+++ b/drivers/interconnect/Kconfig
@@ -13,5 +13,6 @@  if INTERCONNECT
 
 source "drivers/interconnect/imx/Kconfig"
 source "drivers/interconnect/qcom/Kconfig"
+source "drivers/interconnect/samsung/Kconfig"
 
 endif
diff --git a/drivers/interconnect/Makefile b/drivers/interconnect/Makefile
index d203520..c2f9e9d 100644
--- a/drivers/interconnect/Makefile
+++ b/drivers/interconnect/Makefile
@@ -6,3 +6,4 @@  icc-core-objs				:= core.o bulk.o
 obj-$(CONFIG_INTERCONNECT)		+= icc-core.o
 obj-$(CONFIG_INTERCONNECT_IMX)		+= imx/
 obj-$(CONFIG_INTERCONNECT_QCOM)		+= qcom/
+obj-$(CONFIG_INTERCONNECT_SAMSUNG)	+= samsung/
\ No newline at end of file
diff --git a/drivers/interconnect/samsung/Kconfig b/drivers/interconnect/samsung/Kconfig
new file mode 100644
index 0000000..508ed64
--- /dev/null
+++ b/drivers/interconnect/samsung/Kconfig
@@ -0,0 +1,13 @@ 
+# SPDX-License-Identifier: GPL-2.0-only
+config INTERCONNECT_SAMSUNG
+	bool "Samsung interconnect drivers"
+	depends on ARCH_EXYNOS || COMPILE_TEST
+	help
+	  Interconnect drivers for Samsung SoCs.
+
+
+config INTERCONNECT_EXYNOS
+	tristate "Exynos generic interconnect driver"
+	depends on INTERCONNECT_SAMSUNG
+	help
+	  Generic interconnect driver for Exynos SoCs.
diff --git a/drivers/interconnect/samsung/Makefile b/drivers/interconnect/samsung/Makefile
new file mode 100644
index 0000000..e19d1df
--- /dev/null
+++ b/drivers/interconnect/samsung/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/samsung/exynos.c b/drivers/interconnect/samsung/exynos.c
new file mode 100644
index 0000000..6559d8c
--- /dev/null
+++ b/drivers/interconnect/samsung/exynos.c
@@ -0,0 +1,199 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Exynos generic interconnect provider driver
+ *
+ * Copyright (c) 2020 Samsung Electronics Co., Ltd.
+ *
+ * Authors: Artur Świgoń <[e-mail]>
+ *          Sylwester Nawrocki <[e-mail]>
+ */
+#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>
+#include <linux/slab.h>
+
+#define EXYNOS_ICC_DEFAULT_BUS_CLK_RATIO	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;
+	u32 bus_clk_ratio;
+};
+
+static struct icc_node *exynos_icc_get_parent(struct device_node *np)
+{
+	struct of_phandle_args args;
+	struct icc_node_data *icc_node_data;
+	struct icc_node *icc_node;
+	int num, ret;
+
+	num = of_count_phandle_with_args(np, "interconnects",
+					 "#interconnect-cells");
+	if (num < 1)
+		return NULL; /* parent nodes are optional */
+
+	/* Get the interconnect target node */
+	ret = of_parse_phandle_with_args(np, "interconnects",
+					"#interconnect-cells", 0, &args);
+	if (ret < 0)
+		return ERR_PTR(ret);
+
+	icc_node_data = of_icc_get_from_provider(&args);
+	of_node_put(args.np);
+
+	if (IS_ERR(icc_node_data))
+		return ERR_CAST(icc_node_data);
+
+	icc_node = icc_node_data->node;
+	kfree(icc_node_data);
+
+	return icc_node;
+}
+
+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 = max(src->avg_bw, src->peak_bw) / src_priv->bus_clk_ratio;
+	s32 dst_freq = max(dst->avg_bw, dst->peak_bw) / dst_priv->bus_clk_ratio;
+	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 (src)\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 (dst)\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 = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%pOFn",
+					bus_dev->of_node);
+	if (of_property_read_u32(bus_dev->of_node, "samsung,data-clock-ratio",
+				 &priv->bus_clk_ratio))
+		priv->bus_clk_ratio = EXYNOS_ICC_DEFAULT_BUS_CLK_RATIO;
+
+	/*
+	 * Register a PM QoS request for the parent (devfreq) device.
+	 */
+	ret = dev_pm_qos_add_request(bus_dev, &priv->qos_req,
+				     DEV_PM_QOS_MIN_FREQUENCY, 0);
+	if (ret < 0)
+		goto err_node_del;
+
+	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_pmqos_del;
+	}
+	if (icc_parent_node) {
+		ret = icc_link_create(icc_node, icc_parent_node->id);
+		if (ret < 0)
+			goto err_pmqos_del;
+	}
+
+	return 0;
+
+err_pmqos_del:
+	dev_pm_qos_remove_request(&priv->qos_req);
+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",
+		.sync_state = icc_sync_state,
+	},
+	.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ń <[e-mail]>");
+MODULE_AUTHOR("Sylwester Nawrocki <[e-mail]>");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:exynos-generic-icc");