[PATCH RFC v6 0/6] Exynos: Simple QoS for exynos-bus using interconnect

[PATCH RFC v6 0/6] Exynos: Simple QoS for exynos-bus using interconnect

[Sylwester Nawrocki]

This patchset adds interconnect API support for the Exynos SoC "samsung,
exynos-bus" compatible devices, which already have their corresponding
exynos-bus driver in the devfreq subsystem.  Complementing the devfreq
driver with an interconnect functionality allows to ensure the QoS
requirements of devices accessing the system memory (e.g. video processing
devices) are fulfilled and aallows to avoid issues like the one discussed
in thread [1].

This patch series adds implementation of the interconnect provider per each
"samsung,exynos-bus" compatible DT node, with one interconnect node per
provider.  The interconnect code which was previously added as a part of
the devfreq driver has been converted to a separate platform driver.
In the devfreq a corresponding virtual child platform device is registered.
Integration of devfreq and interconnect frameworks is achieved through
the PM QoS API.

A sample interconnect consumer for exynos-mixer is added in patches 5/6,
6/6, it is currently added only for exynos4412 and allows to address the
mixer DMA underrun error issues [1].

Main changes since v5 [3] is an addition of "bus-width: DT property, which
specifies data width of the interconnect bus (patches 1...2/6 and addition
of synchronization of the interconnect bandwidth setting with VSYNC
(patch 6/6).

The series has been tested on Odroid U3 board. It is based on icc-next
branch, which already includes required patches [2]:
 26c205e interconnect: Allow inter-provider pairs to be configured
 0db4ee05 interconnect: Relax requirement in of_icc_get_from_provider()
 6998a7c interconnect: Export of_icc_get_from_provider()

--
Regards,
Sylwester


Changes since v3 [4] (v4 skipped to align with patchset [1]), detailed
changes are listed in each patch:
 - conversion to a separate interconnect (platform) driver,
 - an update of the DT binding documenting new optional properties:
   #interconnect-cells, samsung,interconnect-parent in "samsung,exynos-bus"
   nodes,
 - new DT properties added to the SoC, rather than to the board specific
   files.

Changes since v2 [5]:
 - Use icc_std_aggregate().
 - Implement a different modification of apply_constraints() in
   drivers/interconnect/core.c (patch 03).
 - Use 'exynos,interconnect-parent-node' in the DT instead of
   'devfreq'/'parent', depending on the bus.
 - Rebase on DT patches that deprecate the 'devfreq' DT property.
 - Improve error handling, including freeing generated IDs on failure.
 - Remove exynos_bus_icc_connect() and add exynos_bus_icc_get_parent().

Changes since v1 [6]:
 - Rebase on coupled regulators patches.
 - Use dev_pm_qos_*() API instead of overriding frequency in
   exynos_bus_target().
 - Use IDR for node ID allocation.
 - Reverse order of multiplication and division in
   mixer_set_memory_bandwidth() (patch 07) to avoid integer overflow.


References:
[1] https://patchwork.kernel.org/patch/10861757/ (original issue)
[2] https://www.spinics.net/lists/linux-samsung-soc/msg70014.html
[3] https://www.spinics.net/lists/arm-kernel/msg810722.html
[4] https://lore.kernel.org/linux-pm/20191220115653.6487-1-a.swigon@samsung.com
[5] https://patchwork.kernel.org/cover/11054417/ (v1 of this RFC)
[6] https://patchwork.kernel.org/cover/11152595/ (v2 of this RFC)


Artur Świgoń (1):
  ARM: dts: exynos: Add interconnects to Exynos4412 mixer

Sylwester Nawrocki (5):
  dt-bindings: exynos-bus: Add documentation for interconnect properties
  interconnect: Add generic interconnect driver for Exynos SoCs
  PM / devfreq: exynos-bus: Add registration of interconnect child
    device
  ARM: dts: exynos: Add interconnect properties to Exynos4412 bus nodes
  drm: exynos: mixer: Add interconnect support

 .../devicetree/bindings/devfreq/exynos-bus.txt     |  68 +++++++-
 arch/arm/boot/dts/exynos4412.dtsi                  |   7 +
 drivers/devfreq/exynos-bus.c                       |  17 ++
 drivers/gpu/drm/exynos/exynos_mixer.c              | 150 +++++++++++++++-
 drivers/interconnect/Kconfig                       |   1 +
 drivers/interconnect/Makefile                      |   1 +
 drivers/interconnect/exynos/Kconfig                |   6 +
 drivers/interconnect/exynos/Makefile               |   4 +
 drivers/interconnect/exynos/exynos.c               | 192 +++++++++++++++++++++
 9 files changed, 436 insertions(+), 10 deletions(-)
 create mode 100644 drivers/interconnect/exynos/Kconfig
 create mode 100644 drivers/interconnect/exynos/Makefile
 create mode 100644 drivers/interconnect/exynos/exynos.c

--
2.7.4

[PATCH RFC v6 1/6] dt-bindings: exynos-bus: Add documentation for interconnect properties

[Sylwester Nawrocki]

Add documentation for new optional properties in the exynos bus nodes:
samsung,interconnect-parent, #interconnect-cells, bus-width.
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 v6:
 - added dts example of bus hierarchy definition and the interconnect
   consumer,
 - added new bus-width property.

Changes for v5:
 - exynos,interconnect-parent-node renamed to samsung,interconnect-parent
---
 .../devicetree/bindings/devfreq/exynos-bus.txt     | 68 +++++++++++++++++++++-
 1 file changed, 66 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/devfreq/exynos-bus.txt b/Documentation/devicetree/bindings/devfreq/exynos-bus.txt
index e71f752..4035e3e 100644
--- a/Documentation/devicetree/bindings/devfreq/exynos-bus.txt
+++ b/Documentation/devicetree/bindings/devfreq/exynos-bus.txt
@@ -51,6 +51,13 @@ 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.
+- bus-width: the interconnect bus width in bits, default value is 8 when this
+  property is missing.
+
 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
@@ -135,7 +142,7 @@ Detailed correlation between sub-blocks and power line according to Exynos SoC:
 		|--- PERIC (Fixed clock rate)
 		|--- FSYS  (Fixed clock rate)
 
-Example1:
+Example 1:
 	Show the AXI buses of Exynos3250 SoC. Exynos3250 divides the buses to
 	power line (regulator). The MIF (Memory Interface) AXI bus is used to
 	transfer data between DRAM and CPU and uses the VDD_MIF regulator.
@@ -184,7 +191,7 @@ Example1:
 	|L5   |200000 |200000  |400000 |300000 |       ||1000000 |
 	----------------------------------------------------------
 
-Example2 :
+Example 2:
 	The bus of DMC (Dynamic Memory Controller) block in exynos3250.dtsi
 	is listed below:
 
@@ -419,3 +426,60 @@ Example2 :
 		devfreq = <&bus_leftbus>;
 		status = "okay";
 	};
+
+Example 3:
+	An interconnect path "bus_display -- bus_leftbus -- bus_dmc" on
+	Exynos4412 SoC with video mixer as an interconnect consumer device.
+
+	soc {
+		bus_dmc: bus_dmc {
+			compatible = "samsung,exynos-bus";
+			clocks = <&clock CLK_DIV_DMC>;
+			clock-names = "bus";
+			operating-points-v2 = <&bus_dmc_opp_table>;
+			bus-width = <4>;
+			#interconnect-cells = <0>;
+			status = "disabled";
+		};
+
+		bus_leftbus: bus_leftbus {
+			compatible = "samsung,exynos-bus";
+			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";
+		};
+
+		bus_display: bus_display {
+			compatible = "samsung,exynos-bus";
+			clocks = <&clock CLK_ACLK160>;
+			clock-names = "bus";
+			operating-points-v2 = <&bus_display_opp_table>;
+			samsung,interconnect-parent = <&bus_leftbus>;
+			#interconnect-cells = <0>;
+			status = "disabled";
+		};
+
+		bus_dmc_opp_table: opp_table1 {
+			compatible = "operating-points-v2";
+			/* ... */
+		}
+
+		bus_leftbus_opp_table: opp_table3 {
+			compatible = "operating-points-v2";
+			/* ... */
+		};
+
+		bus_display_opp_table: opp_table4 {
+			compatible = "operating-points-v2";
+			/* .. */
+		};
+
+		&mixer {
+			compatible = "samsung,exynos4212-mixer";
+			interconnects = <&bus_display &bus_dmc>;
+			/* ... */
+		};
+	};
-- 
2.7.4

[PATCH RFC v6 2/6] interconnect: Add generic interconnect driver for Exynos SoCs

[Sylwester Nawrocki]

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ń <a.swigon@samsung.com>
Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
---
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,
 - use IS_ERR_OR_NULL in exynos_icc_genberic_remove().

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 | 192 +++++++++++++++++++++++++++++++++++
 5 files changed, 204 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..6c47a62
--- /dev/null
+++ b/drivers/interconnect/exynos/exynos.c
@@ -0,0 +1,192 @@
+// 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 EXYNOS_ICC_DEFAULT_BUS_WIDTH	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_width;
+};
+
+static struct icc_node *exynos_icc_get_parent(struct device_node *np)
+{
+	struct of_phandle_args args;
+	struct icc_node *icc_np;
+	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);
+
+	icc_np = of_icc_get_from_provider(&args);
+	of_node_put(args.np);
+
+	return icc_np;
+}
+
+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_width;
+	s32 dst_freq = max(dst->avg_bw, dst->peak_bw) / dst_priv->bus_width;
+	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 (!IS_ERR_OR_NULL(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, "bus-width",
+				 &priv->bus_width))
+		priv->bus_width = EXYNOS_ICC_DEFAULT_BUS_WIDTH;
+
+	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

[PATCH RFC v6 3/6] PM / devfreq: exynos-bus: Add registration of interconnect child device

[Sylwester Nawrocki]

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>
Acked-by: Chanwoo Choi <cw00.choi@samsung.com>
---
Changes for v6:
 - none.

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

[PATCH RFC v6 4/6] ARM: dts: exynos: Add interconnect properties to Exynos4412 bus nodes

[Sylwester Nawrocki]

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,
 - bus-width: the bus width in bits, required to properly derive
   minimum bus clock frequency from requested bandwidth for each
   bus.

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>
Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
---
Changes for v6:
 - added bus-width property in bus_dmc node.

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 | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm/boot/dts/exynos4412.dtsi b/arch/arm/boot/dts/exynos4412.dtsi
index 4886894..24529d4 100644
--- a/arch/arm/boot/dts/exynos4412.dtsi
+++ b/arch/arm/boot/dts/exynos4412.dtsi
@@ -381,6 +381,8 @@
 			clocks = <&clock CLK_DIV_DMC>;
 			clock-names = "bus";
 			operating-points-v2 = <&bus_dmc_opp_table>;
+			bus-width = <4>;
+			#interconnect-cells = <0>;
 			status = "disabled";
 		};

@@ -450,6 +452,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 +470,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

[PATCH RFC v6 5/6] ARM: dts: exynos: Add interconnects to Exynos4412 mixer

[Sylwester Nawrocki]

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, v6:
 - 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 24529d4..e0be193 100644
--- a/arch/arm/boot/dts/exynos4412.dtsi
+++ b/arch/arm/boot/dts/exynos4412.dtsi
@@ -777,6 +777,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>;
 };

 &pmu {
--
2.7.4

[PATCH RFC v6 6/6] drm: exynos: mixer: Add interconnect support

[Sylwester Nawrocki]

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 bandwidth value is calculated from selected DRM mode, i.e.
video plane width, height, refresh rate and pixel format.

The bandwidth setting is synchronized with VSYNC when we are switching
to lower bandwidth. This is required to ensure enough bandwidth for
the device since new settings are normally being applied in the hardware
synchronously with VSYNC.

Co-developed-by: Artur Świgoń <a.swigon@samsung.com>
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
---
Changes for v6:
 - the icc_set_bw() call is now only done when calculated value for
   a crtc changes, this avoids unnecessary calls per each video frame
 - added synchronization of the interconnect bandwidth setting with
   the mixer VSYNC in order to avoid buffer underflow when we lower
   the interconnect bandwidth when the hardware still operates with
   previous mode settings that require higher bandwidth. This fixed
   IOMMU faults observed e.g. during switching from two planes to
   a single plane operation.

Changes for v5:
 - renamed soc_path variable to icc_path
---
 drivers/gpu/drm/exynos/exynos_mixer.c | 150 ++++++++++++++++++++++++++++++++--
 1 file changed, 142 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
index c7e2e2e..f7babf8 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>
@@ -73,6 +74,7 @@ enum mixer_flag_bits {
 	MXR_BIT_INTERLACE,
 	MXR_BIT_VP_ENABLED,
 	MXR_BIT_HAS_SCLK,
+	MXR_BIT_REQUEST_BW,
 };
 
 static const uint32_t mixer_formats[] = {
@@ -99,6 +101,13 @@ struct mixer_context {
 	struct exynos_drm_plane	planes[MIXER_WIN_NR];
 	unsigned long		flags;
 
+	struct icc_path		*icc_path;
+	/* memory bandwidth on the interconnect bus in B/s */
+	unsigned long		icc_bandwidth;
+	/* mutex protecting @icc_bandwidth and serializing access to @icc_path */
+	struct mutex 		icc_lock;
+	struct work_struct	work;
+
 	int			irq;
 	void __iomem		*mixer_regs;
 	void __iomem		*vp_regs;
@@ -754,6 +763,9 @@ static irqreturn_t mixer_irq_handler(int irq, void *arg)
 		val |= MXR_INT_CLEAR_VSYNC;
 		val &= ~MXR_INT_STATUS_VSYNC;
 
+		if (test_and_clear_bit(MXR_BIT_REQUEST_BW, &ctx->flags))
+			schedule_work(&ctx->work);
+
 		/* interlace scan need to check shadow register */
 		if (test_bit(MXR_BIT_INTERLACE, &ctx->flags)
 		    && !mixer_is_synced(ctx))
@@ -934,6 +946,78 @@ static void mixer_disable_vblank(struct exynos_drm_crtc *crtc)
 	mixer_reg_writemask(mixer_ctx, MXR_INT_EN, 0, MXR_INT_EN_VSYNC);
 }
 
+/**
+ * mixer_get_memory_bandwidth - calculate memory bandwidth for current crtc mode
+ * @crtc: a crtc with DRM mode to calculate the bandwidth for
+ *
+ * Return: memory bandwidth in B/s
+ *
+ * This function returns memory bandwidth calculated as a sum of amount of data
+ * per second for each plane. The calculation is based on maximum possible pixel
+ * resolution for a plane so as to avoid different bandwidth request per each
+ * video frame. The formula used for calculation for each plane is:
+ *
+ * bw = width * height * frame_rate / interlace / (hor_subs * vert_subs)
+ *
+ * where:
+ *  - width, height - (DRM mode) video frame width and height in pixels,
+ *  - frame_rate - DRM mode frame refresh rate,
+ *  - interlace: 1 - in case of progressive and 2 in case of interlaced video,
+ *  - hor_subs, vert_subs - accordingly horizontal and vertical pixel
+ *    subsampling for a plane.
+ */
+static unsigned int mixer_get_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;
+	int i, j, sub;
+
+	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;
+			}
+		}
+	}
+
+	return bandwidth;
+}
+
+static void mixer_set_icc_bandwidth(struct mixer_context *ctx)
+{
+	unsigned long bandwidth;
+	u32 avg_bw, peak_bw;
+
+	mutex_lock(&ctx->icc_lock);
+
+	/* add 20% safety margin */
+	bandwidth = ctx->icc_bandwidth / 4 * 5;
+
+	avg_bw = peak_bw = Bps_to_icc(bandwidth);
+	icc_set_bw(ctx->icc_path, avg_bw, peak_bw);
+
+	mutex_unlock(&ctx->icc_lock);
+
+	dev_dbg(ctx->dev, "safe bandwidth %lu Bps\n", bandwidth);
+}
+
+static void mixer_icc_request_fn(struct work_struct *work)
+{
+	struct mixer_context *ctx = container_of(work, struct mixer_context,
+						 work);
+	mixer_set_icc_bandwidth(ctx);
+}
+
 static void mixer_atomic_begin(struct exynos_drm_crtc *crtc)
 {
 	struct mixer_context *ctx = crtc->ctx;
@@ -980,12 +1064,34 @@ static void mixer_disable_plane(struct exynos_drm_crtc *crtc,
 
 static void mixer_atomic_flush(struct exynos_drm_crtc *crtc)
 {
-	struct mixer_context *mixer_ctx = crtc->ctx;
+	struct mixer_context *ctx = crtc->ctx;
+	int bw, prev_bw;
 
-	if (!test_bit(MXR_BIT_POWERED, &mixer_ctx->flags))
+	if (!test_bit(MXR_BIT_POWERED, &ctx->flags))
 		return;
 
-	mixer_enable_sync(mixer_ctx);
+	/*
+	 * Request necessary bandwidth on the interconnects. If new
+	 * bandwidth is greater than current value set the new value
+	 * immediately. Otherwise request lower bandwidth only after
+	 * VSYNC, after the HW has actually switched to new video
+	 * frame settings.
+	 */
+	if (ctx->icc_path) {
+		bw = mixer_get_memory_bandwidth(crtc);
+
+		mutex_lock(&ctx->icc_lock);
+		prev_bw = ctx->icc_bandwidth;
+		ctx->icc_bandwidth = bw;
+		mutex_unlock(&ctx->icc_lock);
+
+		if (bw > prev_bw)
+			mixer_set_icc_bandwidth(ctx);
+		else if (bw < prev_bw)
+			set_bit(MXR_BIT_REQUEST_BW, &ctx->flags);
+	}
+
+	mixer_enable_sync(ctx);
 	exynos_crtc_handle_event(crtc);
 }
 
@@ -1036,6 +1142,10 @@ static void mixer_atomic_disable(struct exynos_drm_crtc *crtc)
 
 	pm_runtime_put(ctx->dev);
 
+	cancel_work_sync(&ctx->work);
+	ctx->icc_bandwidth = 0;
+	mixer_set_icc_bandwidth(ctx);
+
 	clear_bit(MXR_BIT_POWERED, &ctx->flags);
 }
 
@@ -1210,6 +1320,7 @@ static void mixer_unbind(struct device *dev, struct device *master, void *data)
 {
 	struct mixer_context *ctx = dev_get_drvdata(dev);
 
+	cancel_work_sync(&ctx->work);
 	mixer_ctx_remove(ctx);
 }
 
@@ -1223,19 +1334,33 @@ static int mixer_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	const struct mixer_drv_data *drv;
 	struct mixer_context *ctx;
+	struct icc_path *path;
 	int ret;
 
+	/*
+	 * Returns NULL if CONFIG_INTERCONNECT is disabled.
+	 * May return ERR_PTR(-EPROBE_DEFER).
+	 */
+	path = of_icc_get(dev, NULL);
+	if (IS_ERR(path))
+		return PTR_ERR(path);
+
 	ctx = devm_kzalloc(&pdev->dev, sizeof(*ctx), GFP_KERNEL);
 	if (!ctx) {
 		DRM_DEV_ERROR(dev, "failed to alloc mixer context.\n");
-		return -ENOMEM;
+		ret = -ENOMEM;
+		goto err;
 	}
 
 	drv = of_device_get_match_data(dev);
 
+	INIT_WORK(&ctx->work, mixer_icc_request_fn);
+	mutex_init(&ctx->icc_lock);
+
 	ctx->pdev = pdev;
 	ctx->dev = dev;
 	ctx->mxr_ver = drv->version;
+	ctx->icc_path = path;
 
 	if (drv->is_vp_enabled)
 		__set_bit(MXR_BIT_VP_ENABLED, &ctx->flags);
@@ -1247,17 +1372,26 @@ static int mixer_probe(struct platform_device *pdev)
 	pm_runtime_enable(dev);
 
 	ret = component_add(&pdev->dev, &mixer_component_ops);
-	if (ret)
+	if (ret) {
 		pm_runtime_disable(dev);
-
+		goto err;
+	}
+	return 0;
+err:
+	icc_put(path);
 	return ret;
 }
 
 static int mixer_remove(struct platform_device *pdev)
 {
-	pm_runtime_disable(&pdev->dev);
+	struct device *dev = &pdev->dev;
+	struct mixer_context *ctx = dev_get_drvdata(dev);
+
+	pm_runtime_disable(dev);
+
+	component_del(dev, &mixer_component_ops);
 
-	component_del(&pdev->dev, &mixer_component_ops);
+	icc_put(ctx->icc_path);
 
 	return 0;
 }
-- 
2.7.4

Re: [PATCH RFC v6 1/6] dt-bindings: exynos-bus: Add documentation for interconnect properties

[Chanwoo Choi]

Hi Sylwester,


On 7/3/20 1:37 AM, Sylwester Nawrocki wrote:
> Add documentation for new optional properties in the exynos bus nodes:
> samsung,interconnect-parent, #interconnect-cells, bus-width.
> 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 v6:
>  - added dts example of bus hierarchy definition and the interconnect
>    consumer,
>  - added new bus-width property.
> 
> Changes for v5:
>  - exynos,interconnect-parent-node renamed to samsung,interconnect-parent
> ---
>  .../devicetree/bindings/devfreq/exynos-bus.txt     | 68 +++++++++++++++++++++-
>  1 file changed, 66 insertions(+), 2 deletions(-)

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


(snip)

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH RFC v6 1/6] dt-bindings: exynos-bus: Add documentation for interconnect properties

[Rob Herring]

On Thu, Jul 02, 2020 at 06:37:19PM +0200, Sylwester Nawrocki wrote:
> Add documentation for new optional properties in the exynos bus nodes:
> samsung,interconnect-parent, #interconnect-cells, bus-width.
> 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 v6:
>  - added dts example of bus hierarchy definition and the interconnect
>    consumer,
>  - added new bus-width property.
> 
> Changes for v5:
>  - exynos,interconnect-parent-node renamed to samsung,interconnect-parent
> ---
>  .../devicetree/bindings/devfreq/exynos-bus.txt     | 68 +++++++++++++++++++++-
>  1 file changed, 66 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/devfreq/exynos-bus.txt b/Documentation/devicetree/bindings/devfreq/exynos-bus.txt
> index e71f752..4035e3e 100644
> --- a/Documentation/devicetree/bindings/devfreq/exynos-bus.txt
> +++ b/Documentation/devicetree/bindings/devfreq/exynos-bus.txt
> @@ -51,6 +51,13 @@ 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.

Adding vendor specific properties for a common binding defeats the 
point.

> +- #interconnect-cells: should be 0.
> +- bus-width: the interconnect bus width in bits, default value is 8 when this
> +  property is missing.

Your bus is 8-bits or 4-bits as the example?

> +
>  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
> @@ -135,7 +142,7 @@ Detailed correlation between sub-blocks and power line according to Exynos SoC:
>  		|--- PERIC (Fixed clock rate)
>  		|--- FSYS  (Fixed clock rate)
>  
> -Example1:
> +Example 1:
>  	Show the AXI buses of Exynos3250 SoC. Exynos3250 divides the buses to
>  	power line (regulator). The MIF (Memory Interface) AXI bus is used to
>  	transfer data between DRAM and CPU and uses the VDD_MIF regulator.
> @@ -184,7 +191,7 @@ Example1:
>  	|L5   |200000 |200000  |400000 |300000 |       ||1000000 |
>  	----------------------------------------------------------
>  
> -Example2 :
> +Example 2:
>  	The bus of DMC (Dynamic Memory Controller) block in exynos3250.dtsi
>  	is listed below:
>  
> @@ -419,3 +426,60 @@ Example2 :
>  		devfreq = <&bus_leftbus>;
>  		status = "okay";
>  	};
> +
> +Example 3:
> +	An interconnect path "bus_display -- bus_leftbus -- bus_dmc" on
> +	Exynos4412 SoC with video mixer as an interconnect consumer device.
> +
> +	soc {
> +		bus_dmc: bus_dmc {
> +			compatible = "samsung,exynos-bus";
> +			clocks = <&clock CLK_DIV_DMC>;
> +			clock-names = "bus";
> +			operating-points-v2 = <&bus_dmc_opp_table>;
> +			bus-width = <4>;
> +			#interconnect-cells = <0>;
> +			status = "disabled";
> +		};
> +
> +		bus_leftbus: bus_leftbus {
> +			compatible = "samsung,exynos-bus";
> +			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";
> +		};
> +
> +		bus_display: bus_display {
> +			compatible = "samsung,exynos-bus";
> +			clocks = <&clock CLK_ACLK160>;
> +			clock-names = "bus";
> +			operating-points-v2 = <&bus_display_opp_table>;
> +			samsung,interconnect-parent = <&bus_leftbus>;
> +			#interconnect-cells = <0>;
> +			status = "disabled";
> +		};
> +
> +		bus_dmc_opp_table: opp_table1 {
> +			compatible = "operating-points-v2";
> +			/* ... */
> +		}
> +
> +		bus_leftbus_opp_table: opp_table3 {
> +			compatible = "operating-points-v2";
> +			/* ... */
> +		};
> +
> +		bus_display_opp_table: opp_table4 {
> +			compatible = "operating-points-v2";
> +			/* .. */
> +		};
> +
> +		&mixer {
> +			compatible = "samsung,exynos4212-mixer";
> +			interconnects = <&bus_display &bus_dmc>;
> +			/* ... */
> +		};
> +	};
> -- 
> 2.7.4
> 

Re: [PATCH RFC v6 1/6] dt-bindings: exynos-bus: Add documentation for interconnect properties

[Sylwester Nawrocki]

On 09.07.2020 23:04, Rob Herring wrote:
> On Thu, Jul 02, 2020 at 06:37:19PM +0200, Sylwester Nawrocki wrote:
>> Add documentation for new optional properties in the exynos bus nodes:
>> samsung,interconnect-parent, #interconnect-cells, bus-width.
>> 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>

>> ---
>>  .../devicetree/bindings/devfreq/exynos-bus.txt     | 68 +++++++++++++++++++++-
>>  1 file changed, 66 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/devfreq/exynos-bus.txt b/Documentation/devicetree/bindings/devfreq/exynos-bus.txt
>> index e71f752..4035e3e 100644
>> --- a/Documentation/devicetree/bindings/devfreq/exynos-bus.txt
>> +++ b/Documentation/devicetree/bindings/devfreq/exynos-bus.txt
>> @@ -51,6 +51,13 @@ 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.
> 
> Adding vendor specific properties for a common binding defeats the 
> point.

Should we make it then a common interconnect-parent property? Perhaps allowing
also a second cell after the phandle to indicate the target interconnect id?

Currently the links are only being defined in drivers, I'm not sure if we want 
to go that direction and extend the interconnect binding so it is possible
to define any link between the nodes.

With the samsung,interconnect-parent property there was an assumption that
each DT node ("samsung,exynos-bus" compatible) corresponds to an interconnect 
provider with single interconnect node. The source and destination node id 
for the link were unspecified and dynamically allocated by the driver.

I guess we don't want a property that would contain pairs of the interconnect 
node specifiers (phandle + interconnect id) to define all links, since usually 
additional data is required per each link.

Then perhaps we could make the new interconnect-parent property applicable
only to DT nodes with #interconnect-cells == 0, i.e. valid only in such DT 
nodes?

>> +- #interconnect-cells: should be 0.
>> +- bus-width: the interconnect bus width in bits, default value is 8 when this
>> +  property is missing.
> 
> Your bus is 8-bits or 4-bits as the example?
Bus width might not be a good term for the intended purpose of that property.
It has been added to specify minimum bus clock rate required for given data
throughput. After checking the documentation again the AXI bus width is
actually 128 bits everywhere for instance. The example defines data path 
leftbus <- dmc <- (memory) and for leftbus we have bus-width=<8> and for dmc 
bus-width=<4>. 

Perhaps it's better to use a vendor specific property instead, e.g.
samsung, data-clock-ratio?

-- 
Thanks,
Sylwester

Re: [PATCH RFC v6 1/6] dt-bindings: exynos-bus: Add documentation for interconnect properties

[Sylwester Nawrocki]

On 30.07.2020 14:28, Sylwester Nawrocki wrote:
> On 09.07.2020 23:04, Rob Herring wrote:
>> On Thu, Jul 02, 2020 at 06:37:19PM +0200, Sylwester Nawrocki wrote:
>>> Add documentation for new optional properties in the exynos bus nodes:
>>> samsung,interconnect-parent, #interconnect-cells, bus-width.
>>> 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>

>>> --- a/Documentation/devicetree/bindings/devfreq/exynos-bus.txt
>>> +++ b/Documentation/devicetree/bindings/devfreq/exynos-bus.txt
>>> @@ -51,6 +51,13 @@ 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.

>> Adding vendor specific properties for a common binding defeats the 
>> point.

Actually we could do without any new property if we used existing interconnect
consumers binding to specify linking between the provider nodes. I think those
exynos-bus nodes could well be considered both the interconnect providers 
and consumers. The example would then be something along the lines 
(yes, I know the bus node naming needs to be fixed):

	soc {
		bus_dmc: bus_dmc {
			compatible = "samsung,exynos-bus";
			/* ... */
			samsung,data-clock-ratio = <4>;
			#interconnect-cells = <0>;
		};

		bus_leftbus: bus_leftbus {
			compatible = "samsung,exynos-bus";
			/* ... */
			interconnects = <&bus_leftbus &bus_dmc>;
			#interconnect-cells = <0>;
		};

		bus_display: bus_display {
			compatible = "samsung,exynos-bus";
			/* ... */
			interconnects = <&bus_display &bus_leftbus>;
			#interconnect-cells = <0>;
		};


		&mixer {
			compatible = "samsung,exynos4212-mixer";
			interconnects = <&bus_display &bus_dmc>;
			/* ... */
		};
	};

What do you think, Georgi, Rob?

-- 
Regards
Sylwester

Re: [PATCH RFC v6 1/6] dt-bindings: exynos-bus: Add documentation for interconnect properties

[Georgi Djakov]

Hi Sylwester,

On 8/28/20 17:49, Sylwester Nawrocki wrote:
> On 30.07.2020 14:28, Sylwester Nawrocki wrote:
>> On 09.07.2020 23:04, Rob Herring wrote:
>>> On Thu, Jul 02, 2020 at 06:37:19PM +0200, Sylwester Nawrocki wrote:
>>>> Add documentation for new optional properties in the exynos bus nodes:
>>>> samsung,interconnect-parent, #interconnect-cells, bus-width.
>>>> 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>
> 
>>>> --- a/Documentation/devicetree/bindings/devfreq/exynos-bus.txt
>>>> +++ b/Documentation/devicetree/bindings/devfreq/exynos-bus.txt
>>>> @@ -51,6 +51,13 @@ 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.
> 
>>> Adding vendor specific properties for a common binding defeats the 
>>> point.
> 
> Actually we could do without any new property if we used existing interconnect
> consumers binding to specify linking between the provider nodes. I think those
> exynos-bus nodes could well be considered both the interconnect providers 
> and consumers. The example would then be something along the lines 
> (yes, I know the bus node naming needs to be fixed):
> 
> 	soc {
> 		bus_dmc: bus_dmc {
> 			compatible = "samsung,exynos-bus";
> 			/* ... */
> 			samsung,data-clock-ratio = <4>;
> 			#interconnect-cells = <0>;
> 		};
> 
> 		bus_leftbus: bus_leftbus {
> 			compatible = "samsung,exynos-bus";
> 			/* ... */
> 			interconnects = <&bus_leftbus &bus_dmc>;
> 			#interconnect-cells = <0>;
> 		};
> 
> 		bus_display: bus_display {
> 			compatible = "samsung,exynos-bus";
> 			/* ... */
> 			interconnects = <&bus_display &bus_leftbus>;

Hmm, bus_display being a consumer of itself is a bit odd? Did you mean:
 			interconnects = <&bus_dmc &bus_leftbus>;

> 			#interconnect-cells = <0>;
> 		};
> 
> 
> 		&mixer {
> 			compatible = "samsung,exynos4212-mixer";
> 			interconnects = <&bus_display &bus_dmc>;
> 			/* ... */
> 		};
> 	};
> 
> What do you think, Georgi, Rob?

I can't understand the above example with bus_display being it's own consumer.
This seems strange to me. Could you please clarify it?

Otherwise the interconnect consumer DT bindings are already well established
and i don't see anything preventing a node to be both consumer and provider.
So this should be okay in general.

Thanks,
Georgi

Re: [PATCH RFC v6 1/6] dt-bindings: exynos-bus: Add documentation for interconnect properties

[Sylwester Nawrocki]

Hi Georgi,

On 09.09.2020 11:07, Georgi Djakov wrote:
> On 8/28/20 17:49, Sylwester Nawrocki wrote:
>> On 30.07.2020 14:28, Sylwester Nawrocki wrote:
>>> On 09.07.2020 23:04, Rob Herring wrote:
>>>> On Thu, Jul 02, 2020 at 06:37:19PM +0200, Sylwester Nawrocki wrote:
>>>>> Add documentation for new optional properties in the exynos bus nodes:
>>>>> samsung,interconnect-parent, #interconnect-cells, bus-width.
>>>>> 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>
>>
>>>>> --- a/Documentation/devicetree/bindings/devfreq/exynos-bus.txt
>>>>> +++ b/Documentation/devicetree/bindings/devfreq/exynos-bus.txt

>>>>> +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.
>>
>>>> Adding vendor specific properties for a common binding defeats the 
>>>> point.
>>
>> Actually we could do without any new property if we used existing interconnect
>> consumers binding to specify linking between the provider nodes. I think those
>> exynos-bus nodes could well be considered both the interconnect providers 
>> and consumers. The example would then be something along the lines 
>> (yes, I know the bus node naming needs to be fixed):
>>
>> 	soc {
>> 		bus_dmc: bus_dmc {
>> 			compatible = "samsung,exynos-bus";
>> 			/* ... */
>> 			samsung,data-clock-ratio = <4>;
>> 			#interconnect-cells = <0>;
>> 		};
>>
>> 		bus_leftbus: bus_leftbus {
>> 			compatible = "samsung,exynos-bus";
>> 			/* ... */
>> 			interconnects = <&bus_leftbus &bus_dmc>;
>> 			#interconnect-cells = <0>;
>> 		};
>>
>> 		bus_display: bus_display {
>> 			compatible = "samsung,exynos-bus";
>> 			/* ... */
>> 			interconnects = <&bus_display &bus_leftbus>;
> 
> Hmm, bus_display being a consumer of itself is a bit odd? Did you mean:
>  			interconnects = <&bus_dmc &bus_leftbus>;

Might be, but we would need to swap the phandles so <source, destination>
order is maintained, i.e. interconnects = <&bus_leftbus &bus_dmc>;

My intention here was to describe the 'bus_display -> bus_leftbus' part 
of data path 'bus_display -> bus_leftbus -> bus_dmc', bus_display is
really a consumer of 'bus_leftbus -> bus_dmc' path.

I'm not sure if it is allowed to specify only single phandle (and 
interconnect provider specifier) in the interconnect property, that would
be needed for the bus_leftbus node to define bus_dmc as the interconnect 
destination port. There seems to be such a use case in arch/arm64/boot/
dts/allwinner/sun50i-a64.dtsi. 

>> 			#interconnect-cells = <0>;
>> 		};
>>
>>
>> 		&mixer {
>> 			compatible = "samsung,exynos4212-mixer";
>> 			interconnects = <&bus_display &bus_dmc>;
>> 			/* ... */
>> 		};
>> 	};
>>
>> What do you think, Georgi, Rob?
> 
> I can't understand the above example with bus_display being it's own consumer.
> This seems strange to me. Could you please clarify it?

> Otherwise the interconnect consumer DT bindings are already well established
> and i don't see anything preventing a node to be both consumer and provider.
> So this should be okay in general.

Thanks, below is an updated example according to your suggestions. 
Does it look better now?

---------------------------8<------------------------------
soc {
	bus_dmc: bus_dmc {
		compatible = "samsung,exynos-bus";
		/* ... */
		samsung,data-clock-ratio = <4>;
		#interconnect-cells = <0>;
	};

	bus_leftbus: bus_leftbus {
		compatible = "samsung,exynos-bus";
		/* ... */
		interconnects = <&bus_dmc>;
		#interconnect-cells = <0>;
	};

	bus_display: bus_display {
		compatible = "samsung,exynos-bus";
		/* ... */
		interconnects = <&bus_leftbus &bus_dmc>;
		#interconnect-cells = <0>;
	};

	&mixer {
		compatible = "samsung,exynos4212-mixer";
		interconnects = <&bus_display &bus_dmc>;
		/* ... */
	};
};
---------------------------8<------------------------------

-- 
Regards,
Sylwester

Re: [PATCH RFC v6 1/6] dt-bindings: exynos-bus: Add documentation for interconnect properties

[Georgi Djakov]

Hi Sylwester,

On 9/9/20 17:47, Sylwester Nawrocki wrote:
> Hi Georgi,
> 
> On 09.09.2020 11:07, Georgi Djakov wrote:
>> On 8/28/20 17:49, Sylwester Nawrocki wrote:
>>> On 30.07.2020 14:28, Sylwester Nawrocki wrote:
>>>> On 09.07.2020 23:04, Rob Herring wrote:
>>>>> On Thu, Jul 02, 2020 at 06:37:19PM +0200, Sylwester Nawrocki wrote:
>>>>>> Add documentation for new optional properties in the exynos bus nodes:
>>>>>> samsung,interconnect-parent, #interconnect-cells, bus-width.
>>>>>> 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>
>>>
>>>>>> --- a/Documentation/devicetree/bindings/devfreq/exynos-bus.txt
>>>>>> +++ b/Documentation/devicetree/bindings/devfreq/exynos-bus.txt
> 
>>>>>> +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.
>>>
>>>>> Adding vendor specific properties for a common binding defeats the 
>>>>> point.
>>>
>>> Actually we could do without any new property if we used existing interconnect
>>> consumers binding to specify linking between the provider nodes. I think those
>>> exynos-bus nodes could well be considered both the interconnect providers 
>>> and consumers. The example would then be something along the lines 
>>> (yes, I know the bus node naming needs to be fixed):
>>>
>>> 	soc {
>>> 		bus_dmc: bus_dmc {
>>> 			compatible = "samsung,exynos-bus";
>>> 			/* ... */
>>> 			samsung,data-clock-ratio = <4>;
>>> 			#interconnect-cells = <0>;
>>> 		};
>>>
>>> 		bus_leftbus: bus_leftbus {
>>> 			compatible = "samsung,exynos-bus";
>>> 			/* ... */
>>> 			interconnects = <&bus_leftbus &bus_dmc>;
>>> 			#interconnect-cells = <0>;
>>> 		};
>>>
>>> 		bus_display: bus_display {
>>> 			compatible = "samsung,exynos-bus";
>>> 			/* ... */
>>> 			interconnects = <&bus_display &bus_leftbus>;
>>
>> Hmm, bus_display being a consumer of itself is a bit odd? Did you mean:
>>  			interconnects = <&bus_dmc &bus_leftbus>;
> 
> Might be, but we would need to swap the phandles so <source, destination>
> order is maintained, i.e. interconnects = <&bus_leftbus &bus_dmc>;

Ok, i see. Thanks for clarifying! Currently the "interconnects" property is
defined as a pair of initiator and target nodes. You can keep it also as
interconnects = <&bus_display &bus_dmc>, but you will need to figure out
during probe that there is another node in the middle and defer. I assume
that if a provider is also an interconnect consumer, we will link it to
whatever nodes are specified in the "interconnects" property?

> My intention here was to describe the 'bus_display -> bus_leftbus' part 
> of data path 'bus_display -> bus_leftbus -> bus_dmc', bus_display is
> really a consumer of 'bus_leftbus -> bus_dmc' path.
>
> I'm not sure if it is allowed to specify only single phandle (and 
> interconnect provider specifier) in the interconnect property, that would
> be needed for the bus_leftbus node to define bus_dmc as the interconnect 
> destination port. There seems to be such a use case in arch/arm64/boot/
> dts/allwinner/sun50i-a64.dtsi.

In the general case you have to specify pairs. The "dma-mem" is a reserved
name for devices that perform DMA through another bus with separate address
translation rules.

>>> 			#interconnect-cells = <0>;
>>> 		};
>>>
>>>
>>> 		&mixer {
>>> 			compatible = "samsung,exynos4212-mixer";
>>> 			interconnects = <&bus_display &bus_dmc>;
>>> 			/* ... */
>>> 		};
>>> 	};
>>>
>>> What do you think, Georgi, Rob?
>>
>> I can't understand the above example with bus_display being it's own consumer.
>> This seems strange to me. Could you please clarify it?
> 
>> Otherwise the interconnect consumer DT bindings are already well established
>> and i don't see anything preventing a node to be both consumer and provider.
>> So this should be okay in general.
> 
> Thanks, below is an updated example according to your suggestions. 
> Does it look better now?
> 
> ---------------------------8<------------------------------
> soc {
> 	bus_dmc: bus_dmc {
> 		compatible = "samsung,exynos-bus";
> 		/* ... */
> 		samsung,data-clock-ratio = <4>;
> 		#interconnect-cells = <0>;
> 	};
> 
> 	bus_leftbus: bus_leftbus {
> 		compatible = "samsung,exynos-bus";
> 		/* ... */
> 		interconnects = <&bus_dmc>;
> 		#interconnect-cells = <0>;
> 	};
> 
> 	bus_display: bus_display {
> 		compatible = "samsung,exynos-bus";
> 		/* ... */
> 		interconnects = <&bus_leftbus &bus_dmc>;
> 		#interconnect-cells = <0>;
> 	};
> 
> 	&mixer {
> 		compatible = "samsung,exynos4212-mixer";
> 		interconnects = <&bus_display &bus_dmc>;
> 		/* ... */
> 	};
> };
> ---------------------------8<------------------------------

It's difficult to have a common way to describe all the different kinds of
topologies in DT, as some SoCs are very complex, having multi-tiered topologies
with hundreds of nodes, with multiple links between them etc. Currently, the
idea is to have the topology as platform data, but i guess that you want to
avoid this. I hope that we will be able to describe simpler topologies in DT in
the future, but we don't have such support in the framework yet.

So maybe we could try your proposal and see how it will work for exynos.

Thanks,
Georgi

Re: [PATCH RFC v6 1/6] dt-bindings: exynos-bus: Add documentation for interconnect properties

[Sylwester Nawrocki]

Hi Georgi,

On 15.09.2020 23:40, Georgi Djakov wrote:
> On 9/9/20 17:47, Sylwester Nawrocki wrote:
>> On 09.09.2020 11:07, Georgi Djakov wrote:
>>> On 8/28/20 17:49, Sylwester Nawrocki wrote:
>>>> On 30.07.2020 14:28, Sylwester Nawrocki wrote:
>>>>> On 09.07.2020 23:04, Rob Herring wrote:
>>>>>> On Thu, Jul 02, 2020 at 06:37:19PM +0200, Sylwester Nawrocki wrote:
>>>>>>> Add documentation for new optional properties in the exynos bus nodes:
>>>>>>> samsung,interconnect-parent, #interconnect-cells, bus-width.
>>>>>>> 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>

>>>>>>> +++ b/Documentation/devicetree/bindings/devfreq/exynos-bus.txt

>>>> Actually we could do without any new property if we used existing interconnect
>>>> consumers binding to specify linking between the provider nodes. I think those
>>>> exynos-bus nodes could well be considered both the interconnect providers 
>>>> and consumers. The example would then be something along the lines 
>>>> (yes, I know the bus node naming needs to be fixed):
>>>>
>>>> 	soc {
>>>> 		bus_dmc: bus_dmc {
>>>> 			compatible = "samsung,exynos-bus";
>>>> 			/* ... */
>>>> 			samsung,data-clock-ratio = <4>;
>>>> 			#interconnect-cells = <0>;
>>>> 		};
>>>>
>>>> 		bus_leftbus: bus_leftbus {
>>>> 			compatible = "samsung,exynos-bus";
>>>> 			/* ... */
>>>> 			interconnects = <&bus_leftbus &bus_dmc>;
>>>> 			#interconnect-cells = <0>;
>>>> 		};
>>>>
>>>> 		bus_display: bus_display {
>>>> 			compatible = "samsung,exynos-bus";
>>>> 			/* ... */
>>>> 			interconnects = <&bus_display &bus_leftbus>;
>>>
>>> Hmm, bus_display being a consumer of itself is a bit odd? Did you mean:
>>>  			interconnects = <&bus_dmc &bus_leftbus>;
>>
>> Might be, but we would need to swap the phandles so <source, destination>
>> order is maintained, i.e. interconnects = <&bus_leftbus &bus_dmc>;
> 
> Ok, i see. Thanks for clarifying! Currently the "interconnects" property is
> defined as a pair of initiator and target nodes. You can keep it also as
> interconnects = <&bus_display &bus_dmc>, but you will need to figure out
> during probe that there is another node in the middle and defer. I assume
> that if a provider is also an interconnect consumer, we will link it to
> whatever nodes are specified in the "interconnects" property?

My apologies for the delay.

Yes, the "interconnect" property would be used (only) to determine what
links should be created.

>> My intention here was to describe the 'bus_display -> bus_leftbus' part 
>> of data path 'bus_display -> bus_leftbus -> bus_dmc', bus_display is
>> really a consumer of 'bus_leftbus -> bus_dmc' path.
>>
>> I'm not sure if it is allowed to specify only single phandle (and 
>> interconnect provider specifier) in the interconnect property, that would
>> be needed for the bus_leftbus node to define bus_dmc as the interconnect 
>> destination port. There seems to be such a use case in arch/arm64/boot/
>> dts/allwinner/sun50i-a64.dtsi.
> 
> In the general case you have to specify pairs. The "dma-mem" is a reserved
> name for devices that perform DMA through another bus with separate address
> translation rules.

I see, thanks for clarifying.

>>> I can't understand the above example with bus_display being it's own consumer.
>>> This seems strange to me. Could you please clarify it?
>>
>>> Otherwise the interconnect consumer DT bindings are already well established
>>> and i don't see anything preventing a node to be both consumer and provider.
>>> So this should be okay in general.
>>
>> Thanks, below is an updated example according to your suggestions. 
>> Does it look better now?
>>
>> ---------------------------8<------------------------------
>> soc {
>> 	bus_dmc: bus_dmc {
>> 		compatible = "samsung,exynos-bus";
>> 		/* ... */
>> 		samsung,data-clock-ratio = <4>;
>> 		#interconnect-cells = <0>;
>> 	};
>>
>> 	bus_leftbus: bus_leftbus {
>> 		compatible = "samsung,exynos-bus";
>> 		/* ... */
>> 		interconnects = <&bus_dmc>;
>> 		#interconnect-cells = <0>;
>> 	};
>>
>> 	bus_display: bus_display {
>> 		compatible = "samsung,exynos-bus";
>> 		/* ... */
>> 		interconnects = <&bus_leftbus &bus_dmc>;
>> 		#interconnect-cells = <0>;
>> 	};
>>
>> 	&mixer {
>> 		compatible = "samsung,exynos4212-mixer";
>> 		interconnects = <&bus_display &bus_dmc>;
>> 		/* ... */
>> 	};
>> };
>> ---------------------------8<------------------------------
> 
> It's difficult to have a common way to describe all the different kinds of
> topologies in DT, as some SoCs are very complex, having multi-tiered topologies
> with hundreds of nodes, with multiple links between them etc. Currently, the
> idea is to have the topology as platform data, but i guess that you want to
> avoid this. I hope that we will be able to describe simpler topologies in DT in
> the future, but we don't have such support in the framework yet.
> 
> So maybe we could try your proposal and see how it will work for exynos.

I suppose for any new Exynos SoCs with more complex interconnects exposed
for software control an approach with topology definition as platform data 
would also be used. The generic interconnect driver would likely only be 
used on existing platforms where the interconnects are somewhat abstracted 
by the devfreq.   

-- 
Regards,
Sylwester