Hi Sylwester,

On Sat, May 30, 2020 at 1:32 AM Sylwester Nawrocki s.nawrocki@samsung.com wrote:

Add documentation for new optional properties in the exynos bus nodes: samsung,interconnect-parent, #interconnect-cells. These properties allow to specify the SoC interconnect structure which then allows the interconnect consumer devices to request specific bandwidth requirements.

Signed-off-by: Artur Świgoń a.swigon@samsung.com Signed-off-by: Sylwester Nawrocki s.nawrocki@samsung.com

---

Changes for v5:

• exynos,interconnect-parent-node renamed to samsung,interconnect-parent

---

Documentation/devicetree/bindings/devfreq/exynos-bus.txt | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/devfreq/exynos-bus.txt b/Documentation/devicetree/bindings/devfreq/exynos-bus.txt index e71f752..e0d2daa 100644 --- a/Documentation/devicetree/bindings/devfreq/exynos-bus.txt +++ b/Documentation/devicetree/bindings/devfreq/exynos-bus.txt @@ -51,6 +51,11 @@ Optional properties only for parent bus device:

• exynos,saturation-ratio: the percentage value which is used to calibrate the performance count against total cycle count.

+Optional properties for interconnect functionality (QoS frequency constraints): +- samsung,interconnect-parent: phandle to the parent interconnect node; for

• passive devices should point to same node as the exynos,parent-bus property.

+- #interconnect-cells: should be 0

Detailed correlation between sub-blocks and power line according to Exynos SoC:

• In case of Exynos3250, there are two power line as following: VDD_MIF |--- DMC

@@ -185,8 +190,9 @@ Example1:

Example2 :

•   The bus of DMC (Dynamic Memory Controller) block in exynos3250.dtsi
  

•   is listed below:
  

•   The bus of DMC (Dynamic Memory Controller) block in exynos3250.dtsi is
  

•   listed below. An interconnect path "bus_lcd0 -- bus_leftbus -- bus_dmc"
  

•   is defined for demonstration purposes.
  
    bus_dmc: bus_dmc {
            compatible = "samsung,exynos-bus";
  

@@ -376,12 +382,15 @@ Example2 : &bus_dmc { devfreq-events = <&ppmu_dmc0_3>, <&ppmu_dmc1_3>; vdd-supply = <&buck1_reg>; /* VDD_MIF */

•           #interconnect-cells = <0>;
            status = "okay";
    };
  
    &bus_leftbus {
            devfreq-events = <&ppmu_leftbus_3>, <&ppmu_rightbus_3>;
            vdd-supply = <&buck3_reg>;
  

•           samsung,interconnect-parent = <&bus_dmc>;
  

•           #interconnect-cells = <0>;
            status = "okay";
    };
  
  

@@ -392,6 +401,8 @@ Example2 :

    &bus_lcd0 {
            devfreq = <&bus_leftbus>;

•           samsung,interconnect-parent = <&bus_leftbus>;
  

•           #interconnect-cells = <0>;
            status = "okay";
    };
  
  

-- 2.7.4

If you add the usage example like the mixer device of patch5 to this dt-binding document, I think it is very beneficial and more helpful for user of exynos-bus/exynos-generic-icc.

Acked-by: Chanwoo Choi cw00.choi@samsung.com

-- Best Regards, Chanwoo Choi Samsung Electronics

Cc: Rob, devicetree ML

On 29.05.2020 18:31, Sylwester Nawrocki wrote:

Add documentation for new optional properties in the exynos bus nodes: samsung,interconnect-parent, #interconnect-cells. These properties allow to specify the SoC interconnect structure which then allows the interconnect consumer devices to request specific bandwidth requirements.

Signed-off-by: Artur Świgoń a.swigon@samsung.com Signed-off-by: Sylwester Nawrocki s.nawrocki@samsung.com

---

Changes for v5:

• exynos,interconnect-parent-node renamed to samsung,interconnect-parent

---

Documentation/devicetree/bindings/devfreq/exynos-bus.txt | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/devfreq/exynos-bus.txt b/Documentation/devicetree/bindings/devfreq/exynos-bus.txt index e71f752..e0d2daa 100644 --- a/Documentation/devicetree/bindings/devfreq/exynos-bus.txt +++ b/Documentation/devicetree/bindings/devfreq/exynos-bus.txt @@ -51,6 +51,11 @@ Optional properties only for parent bus device:

• exynos,saturation-ratio: the percentage value which is used to calibrate the performance count against total cycle count.

+Optional properties for interconnect functionality (QoS frequency constraints): +- samsung,interconnect-parent: phandle to the parent interconnect node; for

• passive devices should point to same node as the exynos,parent-bus property.

+- #interconnect-cells: should be 0

Detailed correlation between sub-blocks and power line according to Exynos SoC:

• In case of Exynos3250, there are two power line as following: VDD_MIF |--- DMC

@@ -185,8 +190,9 @@ Example1:

Example2 :

• The bus of DMC (Dynamic Memory Controller) block in exynos3250.dtsi
• is listed below:

• The bus of DMC (Dynamic Memory Controller) block in exynos3250.dtsi is

• listed below. An interconnect path "bus_lcd0 -- bus_leftbus -- bus_dmc"

• is defined for demonstration purposes.

  bus_dmc: bus_dmc { compatible = "samsung,exynos-bus";

@@ -376,12 +382,15 @@ Example2 : &bus_dmc { devfreq-events = <&ppmu_dmc0_3>, <&ppmu_dmc1_3>; vdd-supply = <&buck1_reg>; /* VDD_MIF */

• #interconnect-cells = <0>;
  

  status = "okay"; };

  &bus_leftbus { devfreq-events = <&ppmu_leftbus_3>, <&ppmu_rightbus_3>; vdd-supply = <&buck3_reg>;

• samsung,interconnect-parent = <&bus_dmc>;
  

• #interconnect-cells = <0>;
  

  status = "okay"; };

@@ -392,6 +401,8 @@ Example2 :

&bus_lcd0 { devfreq = <&bus_leftbus>;

• samsung,interconnect-parent = <&bus_leftbus>;
  

• #interconnect-cells = <0>;
  

  status = "okay"; };

-- 2.7.4

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.

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.

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.

Hi Sylwester,

On Sat, May 30, 2020 at 1:33 AM Sylwester Nawrocki s.nawrocki@samsung.com wrote:

This patch adds registration of a child platform device for the exynos interconnect driver. It is assumed that the interconnect provider will only be needed when #interconnect-cells property is present in the bus DT node, hence the child device will be created only when such a property is present.

Signed-off-by: Sylwester Nawrocki s.nawrocki@samsung.com

Changes for v5:

• new patch.

---

drivers/devfreq/exynos-bus.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)

diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c index 8fa8eb5..856e37d 100644 --- a/drivers/devfreq/exynos-bus.c +++ b/drivers/devfreq/exynos-bus.c @@ -24,6 +24,7 @@

struct exynos_bus { struct device *dev;

•   struct platform_device *icc_pdev;
  
    struct devfreq *devfreq;
    struct devfreq_event_dev **edev;
  

@@ -156,6 +157,8 @@ static void exynos_bus_exit(struct device *dev) if (ret < 0) dev_warn(dev, "failed to disable the devfreq-event devices\n");

•   platform_device_unregister(bus->icc_pdev);
  

•   dev_pm_opp_of_remove_table(dev);
    clk_disable_unprepare(bus->clk);
    if (bus->opp_table) {
  

@@ -168,6 +171,8 @@ static void exynos_bus_passive_exit(struct device *dev) { struct exynos_bus *bus = dev_get_drvdata(dev);

•   platform_device_unregister(bus->icc_pdev);
  

•   dev_pm_opp_of_remove_table(dev);
    clk_disable_unprepare(bus->clk);
  

} @@ -431,6 +436,18 @@ static int exynos_bus_probe(struct platform_device *pdev) if (ret < 0) goto err;

•   /* Create child platform device for the interconnect provider */
  

•   if (of_get_property(dev->of_node, "#interconnect-cells", NULL)) {
  

•               bus->icc_pdev = platform_device_register_data(
  

•                                           dev, "exynos-generic-icc",
  

•                                           PLATFORM_DEVID_AUTO, NULL, 0);
  

•               if (IS_ERR(bus->icc_pdev)) {
  

•                       ret = PTR_ERR(bus->icc_pdev);
  

•                       goto err;
  

•               }
  

•   }
  

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

-- 2.7.4

It looks like very similar like the registering the interconnect device of imx-bus.c and I already reviewed and agreed this approach.

Acked-by: Chanwoo Choi cw00.choi@samsung.com

nitpick: IMHO, I think that 'exynos-icc' is proper and simple without 'generic' word. If we need to add new icc compatible int the future, we will add 'exynosXXXX-icc' new compatible. But, I'm not forcing it. just opinion. Anyway, I agree this approach.

Hi Sylwester,

On 6/1/20 7:04 PM, Sylwester Nawrocki wrote:

Cc: Rob, devicetree ML

On 31.05.2020 01:57, Chanwoo Choi wrote:

...

On Sat, May 30, 2020 at 1:33 AM Sylwester Nawrocki s.nawrocki@samsung.com wrote:

...

This patch adds registration of a child platform device for the exynos interconnect driver. It is assumed that the interconnect provider will only be needed when #interconnect-cells property is present in the bus DT node, hence the child device will be created only when such a property is present.

Signed-off-by: Sylwester Nawrocki s.nawrocki@samsung.com

Changes for v5:

• new patch.

---

drivers/devfreq/exynos-bus.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)

diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c index 8fa8eb5..856e37d 100644 --- a/drivers/devfreq/exynos-bus.c +++ b/drivers/devfreq/exynos-bus.c @@ -24,6 +24,7 @@

struct exynos_bus { struct device *dev;

•   struct platform_device *icc_pdev;
  
    struct devfreq *devfreq;
    struct devfreq_event_dev **edev;
  

@@ -156,6 +157,8 @@ static void exynos_bus_exit(struct device *dev) if (ret < 0) dev_warn(dev, "failed to disable the devfreq-event devices\n");

•   platform_device_unregister(bus->icc_pdev);
  

•   dev_pm_opp_of_remove_table(dev);
    clk_disable_unprepare(bus->clk);
    if (bus->opp_table) {
  

@@ -168,6 +171,8 @@ static void exynos_bus_passive_exit(struct device *dev) { struct exynos_bus *bus = dev_get_drvdata(dev);

•   platform_device_unregister(bus->icc_pdev);
  

•   dev_pm_opp_of_remove_table(dev);
    clk_disable_unprepare(bus->clk);
  

} @@ -431,6 +436,18 @@ static int exynos_bus_probe(struct platform_device *pdev) if (ret < 0) goto err;

•   /* Create child platform device for the interconnect provider */
  

•   if (of_get_property(dev->of_node, "#interconnect-cells", NULL)) {
  

•               bus->icc_pdev = platform_device_register_data(
  

•                                           dev, "exynos-generic-icc",
  

•                                           PLATFORM_DEVID_AUTO, NULL, 0);
  

•               if (IS_ERR(bus->icc_pdev)) {
  

•                       ret = PTR_ERR(bus->icc_pdev);
  

•                       goto err;
  

•               }
  

•   }
  

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

-- 2.7.4

It looks like very similar like the registering the interconnect device of imx-bus.c and I already reviewed and agreed this approach.

Acked-by: Chanwoo Choi cw00.choi@samsung.com

nitpick: IMHO, I think that 'exynos-icc' is proper and simple without 'generic' word. If we need to add new icc compatible int the future, we will add 'exynosXXXX-icc' new compatible. But, I'm not forcing it. just opinion. Anyway, I agree this approach.

Thanks for review. I will change the name to exynos-icc in next version, as I commented at other patch, it is not part of any DT binding, it is just for device/driver matching between devfreq and interconnect.

Thanks. I have not any objection to use either 'exynos-generic-icc' or 'exynos-icc'. It is just my opinion. And on next version, please add linux-pm mailing list to Cc.

Hi Sylwester,

On Sat, May 30, 2020 at 1:33 AM Sylwester Nawrocki s.nawrocki@samsung.com wrote:

This patch adds the following properties for Exynos4412 interconnect bus nodes:

• samsung,interconnect-parent: to declare connections between nodes in order to guarantee PM QoS requirements between nodes;
• #interconnect-cells: required by the interconnect framework.

Note that #interconnect-cells is always zero and node IDs are not hardcoded anywhere.

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,
• add properties in common exynos4412.dtsi file rather than in Odroid specific odroid4412-odroid-common.dtsi.

---

arch/arm/boot/dts/exynos4412.dtsi | 5 +++++ 1 file changed, 5 insertions(+)

diff --git a/arch/arm/boot/dts/exynos4412.dtsi b/arch/arm/boot/dts/exynos4412.dtsi index 4886894..a7496d3 100644 --- a/arch/arm/boot/dts/exynos4412.dtsi +++ b/arch/arm/boot/dts/exynos4412.dtsi @@ -381,6 +381,7 @@ clocks = <&clock CLK_DIV_DMC>; clock-names = "bus"; operating-points-v2 = <&bus_dmc_opp_table>;

•                   #interconnect-cells = <0>;
                    status = "disabled";
            };
  
  

@@ -450,6 +451,8 @@ clocks = <&clock CLK_DIV_GDL>; clock-names = "bus"; operating-points-v2 = <&bus_leftbus_opp_table>;

•                   samsung,interconnect-parent = <&bus_dmc>;
  

•                   #interconnect-cells = <0>;
                    status = "disabled";
            };
  
  

@@ -466,6 +469,8 @@ clocks = <&clock CLK_ACLK160>; clock-names = "bus"; operating-points-v2 = <&bus_display_opp_table>;

•                   samsung,interconnect-parent = <&bus_leftbus>;
  

•                   #interconnect-cells = <0>;
                    status = "disabled";
            };
  
  

-- 2.7.4

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

Hi Sylwester,

On Sat, May 30, 2020 at 1:33 AM Sylwester Nawrocki s.nawrocki@samsung.com wrote:

From: Artur Świgoń a.swigon@samsung.com

This patch adds an 'interconnects' property to Exynos4412 DTS in order to declare the interconnect path used by the mixer. Please note that the 'interconnect-names' property is not needed when there is only one path in 'interconnects', in which case calling of_icc_get() with a NULL name simply returns the right path.

Signed-off-by: Artur Świgoń a.swigon@samsung.com Reviewed-by: Chanwoo Choi cw00.choi@samsung.com

---

Changes for v5:

• none.

---

arch/arm/boot/dts/exynos4412.dtsi | 1 + 1 file changed, 1 insertion(+)

diff --git a/arch/arm/boot/dts/exynos4412.dtsi b/arch/arm/boot/dts/exynos4412.dtsi index a7496d3..eee86d2 100644 --- a/arch/arm/boot/dts/exynos4412.dtsi +++ b/arch/arm/boot/dts/exynos4412.dtsi @@ -776,6 +776,7 @@ clock-names = "mixer", "hdmi", "sclk_hdmi", "vp"; clocks = <&clock CLK_MIXER>, <&clock CLK_HDMI>, <&clock CLK_SCLK_HDMI>, <&clock CLK_VP>;

•   interconnects = <&bus_display &bus_dmc>;
  

I think it is really good and necessary in order to support the minimum bandwidth. Until now, I had to add the additional code to support for this same purpose into product code.

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

Hi Sylwester,

On 5/30/20 1:32 AM, Sylwester Nawrocki wrote:

From: Marek Szyprowski m.szyprowski@samsung.com

This patch adds interconnect support to exynos-mixer. The mixer works the same as before when CONFIG_INTERCONNECT is 'n'.

For proper operation of the video mixer block we need to ensure the interconnect busses like DMC or LEFTBUS provide enough bandwidth so as to avoid DMA buffer underruns in the mixer block. i.e we need to prevent those busses from operating in low perfomance OPPs when the mixer is running. In this patch the bus bandwidth request is done through the interconnect API, the bandiwidth value is calculated from selected DRM mode, i.e. video plane width, height, refresh rate and pixel format.

Co-developed-by: Artur Świgoń a.swigon@samsung.com Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com [s.nawrocki: renamed soc_path variable to icc_path, edited commit desc.] Signed-off-by: Sylwester Nawrocki s.nawrocki@samsung.com

---

Changes for v5:

• renamed soc_path variable to icc_path

---

drivers/gpu/drm/exynos/exynos_mixer.c | 73 ++++++++++++++++++++++++++++++++--- 1 file changed, 68 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c index 21b726b..bdae683 100644 --- a/drivers/gpu/drm/exynos/exynos_mixer.c +++ b/drivers/gpu/drm/exynos/exynos_mixer.c @@ -13,6 +13,7 @@ #include <linux/component.h> #include <linux/delay.h> #include <linux/i2c.h> +#include <linux/interconnect.h> #include <linux/interrupt.h> #include <linux/irq.h> #include <linux/kernel.h> @@ -98,6 +99,7 @@ struct mixer_context { struct exynos_drm_crtc *crtc; struct exynos_drm_plane planes[MIXER_WIN_NR]; unsigned long flags;

• struct icc_path *icc_path;

  int irq; void __iomem *mixer_regs;

@@ -934,6 +936,42 @@ static void mixer_disable_vblank(struct exynos_drm_crtc *crtc) mixer_reg_writemask(mixer_ctx, MXR_INT_EN, 0, MXR_INT_EN_VSYNC); }

+static void mixer_set_memory_bandwidth(struct exynos_drm_crtc *crtc) +{

• struct drm_display_mode *mode = &crtc->base.state->adjusted_mode;
• struct mixer_context *ctx = crtc->ctx;
• unsigned long bw, bandwidth = 0;
• u32 avg_bw, peak_bw;
• int i, j, sub;
• if (!ctx->icc_path)

• return;
  

• for (i = 0; i < MIXER_WIN_NR; i++) {

• struct drm_plane *plane = &ctx->planes[i].base;
  

• const struct drm_format_info *format;
  

• if (plane->state && plane->state->crtc && plane->state->fb) {
  

• 	format = plane->state->fb->format;
  

• 	bw = mode->hdisplay * mode->vdisplay *
  

• 					drm_mode_vrefresh(mode);
  

• 	if (mode->flags & DRM_MODE_FLAG_INTERLACE)
  

• 		bw /= 2;
  

• 	for (j = 0; j < format->num_planes; j++) {
  

• 		sub = j ? (format->vsub * format->hsub) : 1;
  

• 		bandwidth += format->cpp[j] * bw / sub;
  

First of all, I agree this approach.

Could you please add more detailed comments for understadning about this calculation? As you commented, it seems that the final bandwidth contains the width/height/refresh rate and pixel format. If you add one real example, it will be very helpful.

(snip)