[v7,2/6] interconnect: Add generic interconnect driver for Exynos SoCs

Commit Message

Sylwester Nawrocki/Kernel \(PLT\) /SRPOL/Staff Engineer/Samsung Electronics Oct. 30, 2020, 12:51 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.

The bus-width DT property is to determine the interconnect data
width and traslate requested bandwidth to clock frequency for each
bus.

Signed-off-by: Artur Świgoń <[e-mail]>
Signed-off-by: Sylwester Nawrocki <[e-mail]>
---
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/exynos/Kconfig  |   6 ++
 drivers/interconnect/exynos/Makefile |   4 +
 drivers/interconnect/exynos/exynos.c | 198 +++++++++++++++++++++++++++++++++++
 5 files changed, 210 insertions(+)
 create mode 100644 drivers/interconnect/exynos/Kconfig
 create mode 100644 drivers/interconnect/exynos/Makefile
 create mode 100644 drivers/interconnect/exynos/exynos.c

Comments

Krzysztof Kozlowski Oct. 31, 2020, 12:17 p.m. UTC | #1

On Fri, Oct 30, 2020 at 01:51:45PM +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 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.
> 
> The bus-width DT property is to determine the interconnect data
> width and traslate requested bandwidth to clock frequency for each
> bus.
> 
> Signed-off-by: Artur Świgoń <[e-mail]>
> Signed-off-by: Sylwester Nawrocki <[e-mail]>
> ---
> 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/exynos/Kconfig  |   6 ++
>  drivers/interconnect/exynos/Makefile |   4 +
>  drivers/interconnect/exynos/exynos.c | 198 +++++++++++++++++++++++++++++++++++

How about naming the directory as "samsung"? I don't expect interconnect
drivers for the old Samsung S3C or S5P platforms, but it would be
consisteny with other names (memory, clk, pinctrl).

How about adding separate maintainers entry for the driver with you and
Artur (if he still works on this)?

Best regards,
Krzysztof

Sylwester Nawrocki/Kernel \(PLT\) /SRPOL/Staff Engineer/Samsung Electronics Nov. 2, 2020, 12:23 p.m. UTC | #2

On 31.10.2020 13:17, Krzysztof Kozlowski wrote:
> On Fri, Oct 30, 2020 at 01:51:45PM +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.

>> Signed-off-by: Artur Świgoń <[e-mail]>
>> Signed-off-by: Sylwester Nawrocki <[e-mail]>

>>  drivers/interconnect/Kconfig         |   1 +
>>  drivers/interconnect/Makefile        |   1 +
>>  drivers/interconnect/exynos/Kconfig  |   6 ++
>>  drivers/interconnect/exynos/Makefile |   4 +
>>  drivers/interconnect/exynos/exynos.c | 198 +++++++++++++++++++++++++++++++++++
> 
> How about naming the directory as "samsung"? I don't expect interconnect
> drivers for the old Samsung S3C or S5P platforms, but it would be
> consisteny with other names (memory, clk, pinctrl).

Sure, I will rename the directory.

> How about adding separate maintainers entry for the driver with you and
> Artur (if he still works on this)?

I'm not sure what's the preference in the subsystem, I'm going to add
a patch introducing such a maintainers entry as it might be helpful 
for reviews/testing.

Georgi Djakov Nov. 3, 2020, 8:11 a.m. UTC | #3

Hi Sylwester,

Thank you for refreshing the patchset!

On 10/30/20 14:51, 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.
> 
> The bus-width DT property is to determine the interconnect data
> width and traslate requested bandwidth to clock frequency for each
> bus.
> 
> Signed-off-by: Artur Świgoń <[e-mail]>
> Signed-off-by: Sylwester Nawrocki <[e-mail]>
> ---
> 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/exynos/Kconfig  |   6 ++
>  drivers/interconnect/exynos/Makefile |   4 +
>  drivers/interconnect/exynos/exynos.c | 198 +++++++++++++++++++++++++++++++++++
>  5 files changed, 210 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 platform_driver exynos_generic_icc_driver = {
> +	.driver = {
> +		.name = "exynos-generic-icc",

I think that you will need this:
		.sync_state = icc_sync_state,

Thanks,
Georgi

> +	},
> +	.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");
>

Chanwoo Choi Nov. 3, 2020, 9:37 a.m. UTC | #4

Hi Sylwester,

On 10/30/20 9:51 PM, 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

samsung,interconnect-parent -> interconnects?


> 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.
> 
> The bus-width DT property is to determine the interconnect data
> width and traslate requested bandwidth to clock frequency for each
> bus.
> 
> Signed-off-by: Artur Świgoń <[e-mail]>
> Signed-off-by: Sylwester Nawrocki <[e-mail]>
> ---
> 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/exynos/Kconfig  |   6 ++
>  drivers/interconnect/exynos/Makefile |   4 +
>  drivers/interconnect/exynos/exynos.c | 198 +++++++++++++++++++++++++++++++++++
>  5 files changed, 210 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 d203520..665538d 100644
> --- a/drivers/interconnect/Makefile
> +++ b/drivers/interconnect/Makefile
> @@ -4,5 +4,6 @@ CFLAGS_core.o				:= -I$(src)
>  icc-core-objs				:= core.o bulk.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..772d1fc
> --- /dev/null
> +++ b/drivers/interconnect/exynos/exynos.c
> @@ -0,0 +1,198 @@
> +// 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;
> +}

I have a question about exynos_icc_get_parent().
As I checked, this function returns the only one icc_node
as parent node. But, bus_display dt node in the exynos4412.dtsi
specifies the two interconnect node as following with bus_leftbus, bus_dmc,

When I checked the return value of exynos_icc_get_parent()
during probing for bus_display device, exynos_icc_get_parent() function
only returns 'bus_leftbus' icc_node. Do you need to add two phandle
of icc node?

diff --git a/arch/arm/boot/dts/exynos4412.dtsi b/arch/arm/boot/dts/exynos4412.dtsi
index d07739ec8740..9e4045ceb6ab 100644
--- a/arch/arm/boot/dts/exynos4412.dtsi
+++ b/arch/arm/boot/dts/exynos4412.dtsi
@@ -472,7 +472,7 @@
                        clocks = <&clock CLK_ACLK160>;
                        clock-names = "bus";
                        operating-points-v2 = <&bus_display_opp_table>;
                        interconnects = <&bus_leftbus &bus_dmc>;
                        #interconnect-cells = <0>;
                        status = "disabled";
                };


> +
> +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",
> +	},
> +	.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");
>

Sylwester Nawrocki/Kernel \(PLT\) /SRPOL/Staff Engineer/Samsung Electronics Nov. 3, 2020, 11:32 a.m. UTC | #5

On 03.11.2020 10:37, Chanwoo Choi wrote:
> On 10/30/20 9:51 PM, 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
> 
> samsung,interconnect-parent -> interconnects?

Yes, I will rephrase the whole commit message as it's a bit outdated now.

I've changed the sentence to:
"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."

>> 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.
>>
>> The bus-width DT property is to determine the interconnect data
>> width and traslate requested bandwidth to clock frequency for each
>> bus.
>>
>> Signed-off-by: Artur Świgoń <[e-mail]>
>> Signed-off-by: Sylwester Nawrocki <[e-mail]>

>> +++ b/drivers/interconnect/exynos/exynos.c

>> +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;
>> +}
> 
> I have a question about exynos_icc_get_parent().
> As I checked, this function returns the only one icc_node
> as parent node. But, bus_display dt node in the exynos4412.dtsi
> specifies the two interconnect node as following with bus_leftbus, bus_dmc,
> 
> When I checked the return value of exynos_icc_get_parent()
> during probing for bus_display device, exynos_icc_get_parent() function
> only returns 'bus_leftbus' icc_node. Do you need to add two phandle
> of icc node?

Yes, as we use the interconnect consumer bindings we need to specify a path,
i.e. a <initiator, target> pair. When the provider node initializes it will
link itself to that path. Currently the provider driver uses just the first 
phandle.

> +++ b/arch/arm/boot/dts/exynos4412.dtsi
> @@ -472,7 +472,7 @@
>                         clocks = <&clock CLK_ACLK160>;
>                         clock-names = "bus";
>                         operating-points-v2 = <&bus_display_opp_table>;
>                         interconnects = <&bus_leftbus &bus_dmc>;
>                         #interconnect-cells = <0>;
>                         status = "disabled";
>                 };

Chanwoo Choi Nov. 3, 2020, 2:12 p.m. UTC | #6

Hi Sylwester,

On Tue, Nov 3, 2020 at 8:32 PM Sylwester Nawrocki
<[e-mail]> wrote:
>
> On 03.11.2020 10:37, Chanwoo Choi wrote:
> > On 10/30/20 9:51 PM, 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
> >
> > samsung,interconnect-parent -> interconnects?
>
> Yes, I will rephrase the whole commit message as it's a bit outdated now.
>
> I've changed the sentence to:
> "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."
>
> >> 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.
> >>
> >> The bus-width DT property is to determine the interconnect data
> >> width and traslate requested bandwidth to clock frequency for each
> >> bus.
> >>
> >> Signed-off-by: Artur Świgoń <[e-mail]>
> >> Signed-off-by: Sylwester Nawrocki <[e-mail]>
>
> >> +++ b/drivers/interconnect/exynos/exynos.c
>
> >> +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;
> >> +}
> >
> > I have a question about exynos_icc_get_parent().
> > As I checked, this function returns the only one icc_node
> > as parent node. But, bus_display dt node in the exynos4412.dtsi
> > specifies the two interconnect node as following with bus_leftbus, bus_dmc,
> >
> > When I checked the return value of exynos_icc_get_parent()
> > during probing for bus_display device, exynos_icc_get_parent() function
> > only returns 'bus_leftbus' icc_node. Do you need to add two phandle
> > of icc node?
>
> Yes, as we use the interconnect consumer bindings we need to specify a path,
> i.e. a <initiator, target> pair. When the provider node initializes it will
> link itself to that path. Currently the provider driver uses just the first
> phandle.

As I knew, the interconnect consumer bindings use the two phandles
in the interconnect core as you commented. But, in case of this,
even if add two phandles with interconnect consumding binding style,
the exynos interconnect driver only uses the first phandle.

Instead, I think we better explain this case into a dt-binding
document for users.

> > +++ b/arch/arm/boot/dts/exynos4412.dtsi
> > @@ -472,7 +472,7 @@
> >                         clocks = <&clock CLK_ACLK160>;
> >                         clock-names = "bus";
> >                         operating-points-v2 = <&bus_display_opp_table>;
> >                         interconnects = <&bus_leftbus &bus_dmc>;
> >                         #interconnect-cells = <0>;
> >                         status = "disabled";
> >                 };
>
> --
> Regards,
> Sylwester
> _______________________________________________
> dri-devel mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

On 03.11.2020 15:12, Chanwoo Choi wrote:
>>> I have a question about exynos_icc_get_parent().
>>> As I checked, this function returns the only one icc_node
>>> as parent node. But, bus_display dt node in the exynos4412.dtsi
>>> specifies the two interconnect node as following with bus_leftbus, bus_dmc,
>>>
>>> When I checked the return value of exynos_icc_get_parent()
>>> during probing for bus_display device, exynos_icc_get_parent() function
>>> only returns 'bus_leftbus' icc_node. Do you need to add two phandle
>>> of icc node?
>> Yes, as we use the interconnect consumer bindings we need to specify a path,
>> i.e. a <initiator, target> pair. When the provider node initializes it will
>> link itself to that path. Currently the provider driver uses just the first
>> phandle.

> As I knew, the interconnect consumer bindings use the two phandles
> in the interconnect core as you commented. But, in case of this,
> even if add two phandles with interconnect consuming binding style,
> the exynos interconnect driver only uses the first phandle.
> 
> Instead, I think we better explain this case into a dt-binding
> document for users.

Fair enough, I'll try to improve the description, do you perhaps have 
any suggestions?

The DT binding reflects how the hardware structure looks like and the
fact that the driver currently uses only one of the phandles could be
considered an implementation detail.

Patch

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 d203520..665538d 100644
--- a/drivers/interconnect/Makefile
+++ b/drivers/interconnect/Makefile
@@ -4,5 +4,6 @@  CFLAGS_core.o				:= -I$(src)
 icc-core-objs				:= core.o bulk.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..772d1fc
--- /dev/null
+++ b/drivers/interconnect/exynos/exynos.c
@@ -0,0 +1,198 @@ 
+// 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",
+	},
+	.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");