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

[RFC PATCH v5 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 to avoid issues like the one discussed in thread [1].

This patch series depends on 3 patches from Artur for the interconnect API 
[2], which introduce following changes:

 - exporting of_icc_get_from_provider() to avoid hard coding every graph 
   edge in the DT or driver source,
 - relaxing the requirement on #interconnect-cells, so there is no need 
   to provide dummy node IDs in the DT, 
 - adding new field in struct icc_provider to explicitly allow configuring 
   node pairs from two different providers.

This series adds implementation of 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].

The series has been tested on Odroid U3 board. It is based on icc-next 
branch with devfreq-next branch merged and patches [2] applied.

--
Regards,
Sylwester

--
Changes since v3 [3] (v4 skipped to align with patchset [1]), detailed 
changes are listed at 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 [4]:
 - 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://lore.kernel.org/linux-pm/20191220115653.6487-1-a.swigon@samsung.com
[4] https://patchwork.kernel.org/cover/11054417/ (v1 of this RFC)
[5] https://patchwork.kernel.org/cover/11152595/ (v2 of this RFC)


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

Marek Szyprowski (1):
  drm: exynos: mixer: Add interconnect support

Sylwester Nawrocki (4):
  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

 .../devicetree/bindings/devfreq/exynos-bus.txt     |  15 +-
 arch/arm/boot/dts/exynos4412.dtsi                  |   6 +
 drivers/devfreq/exynos-bus.c                       |  17 ++
 drivers/gpu/drm/exynos/exynos_mixer.c              |  73 +++++++-
 drivers/interconnect/Kconfig                       |   1 +
 drivers/interconnect/Makefile                      |   1 +
 drivers/interconnect/exynos/Kconfig                |   6 +
 drivers/interconnect/exynos/Makefile               |   4 +
 drivers/interconnect/exynos/exynos.c               | 185 +++++++++++++++++++++
 9 files changed, 301 insertions(+), 7 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

[RFC PATCH v5 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.
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

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

[Chanwoo Choi]

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

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

[Sylwester Nawrocki]

Hi Chanwoo,

On 31.05.2020 02:01, Chanwoo Choi wrote:
> 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>

Thanks for review. I will make sure the example includes a consumer
in next version. Will also mention ../interconnect/interconnect.txt
in description of the #interconnect-cells property.

-- 
Regards,
Sylwester

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

[Sylwester Nawrocki]

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

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

[Krzysztof Kozlowski]

On Fri, May 29, 2020 at 06:31:55PM +0200, 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(-)

Acked-by: Krzysztof Kozlowski <krzk@kernel.org>

Best regards,
Krzysztof

[RFC PATCH v5 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.

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

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

[Chanwoo Choi]

Hi Sylwester,

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

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

As I commented, How about changing the compatible name 'exynos-icc'
without 'generic'?
The 'exynos-icc' doesn't have the any specific version of Exynos SoC,
it think that it already contain the 'generic' meaning for Exynos SoC.

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

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

[Sylwester Nawrocki]

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.

-- 
Thanks,
Sylwester

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

[Krzysztof Kozlowski]

On Fri, May 29, 2020 at 06:31:56PM +0200, Sylwester Nawrocki wrote:
> This patch adds a generic interconnect driver for Exynos SoCs in order
> to provide interconnect functionality for each "samsung,exynos-bus"
> compatible device.
> 
> The SoC topology is a graph (or more specifically, a tree) and its
> edges are specified using the 'samsung,interconnect-parent' in the
> DT. Due to unspecified relative probing order, -EPROBE_DEFER may be
> propagated to ensure that the parent is probed before its children.
> 
> Each bus is now an interconnect provider and an interconnect node as
> well (cf. Documentation/interconnect/interconnect.rst), i.e. every bus
> registers itself as a node. Node IDs are not hardcoded but rather
> assigned dynamically at runtime. This approach allows for using this
> driver with various Exynos SoCs.
> 
> Frequencies requested via the interconnect API for a given node are
> propagated to devfreq using dev_pm_qos_update_request(). Please note
> that it is not an error when CONFIG_INTERCONNECT is 'n', in which
> case all interconnect API functions are no-op.
> 
> Signed-off-by: Artur Świgoń <a.swigon@samsung.com>
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> 
> Changes for v5:
>  - adjust to renamed exynos,interconnect-parent-node property,
>  - use automatically generated platform device id as the interconect
>    node id instead of a now unavailable devfreq->id field,
>  - add icc_ prefix to some variables to make the code more self-commenting,
>  - use icc_nodes_remove() instead of icc_node_del() + icc_node_destroy(),
>  - adjust to exynos,interconnect-parent-node property rename to
>    samsung,interconnect-parent,
>  - converted to a separate platform driver in drivers/interconnect.
> ---
>  drivers/interconnect/Kconfig         |   1 +
>  drivers/interconnect/Makefile        |   1 +
>  drivers/interconnect/exynos/Kconfig  |   6 ++
>  drivers/interconnect/exynos/Makefile |   4 +
>  drivers/interconnect/exynos/exynos.c | 185 +++++++++++++++++++++++++++++++++++
>  5 files changed, 197 insertions(+)
>  create mode 100644 drivers/interconnect/exynos/Kconfig
>  create mode 100644 drivers/interconnect/exynos/Makefile
>  create mode 100644 drivers/interconnect/exynos/exynos.c
> 
> diff --git a/drivers/interconnect/Kconfig b/drivers/interconnect/Kconfig
> index 5b7204e..eca6eda 100644
> --- a/drivers/interconnect/Kconfig
> +++ b/drivers/interconnect/Kconfig
> @@ -11,6 +11,7 @@ menuconfig INTERCONNECT
>  
>  if INTERCONNECT
>  
> +source "drivers/interconnect/exynos/Kconfig"
>  source "drivers/interconnect/imx/Kconfig"
>  source "drivers/interconnect/qcom/Kconfig"
>  
> diff --git a/drivers/interconnect/Makefile b/drivers/interconnect/Makefile
> index 4825c28..2ba1de6 100644
> --- a/drivers/interconnect/Makefile
> +++ b/drivers/interconnect/Makefile
> @@ -4,5 +4,6 @@ CFLAGS_core.o				:= -I$(src)
>  icc-core-objs				:= core.o
>  
>  obj-$(CONFIG_INTERCONNECT)		+= icc-core.o
> +obj-$(CONFIG_INTERCONNECT_EXYNOS)	+= exynos/
>  obj-$(CONFIG_INTERCONNECT_IMX)		+= imx/
>  obj-$(CONFIG_INTERCONNECT_QCOM)		+= qcom/
> diff --git a/drivers/interconnect/exynos/Kconfig b/drivers/interconnect/exynos/Kconfig
> new file mode 100644
> index 0000000..e51e52e
> --- /dev/null
> +++ b/drivers/interconnect/exynos/Kconfig
> @@ -0,0 +1,6 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +config INTERCONNECT_EXYNOS
> +	tristate "Exynos generic interconnect driver"
> +	depends on ARCH_EXYNOS || COMPILE_TEST
> +	help
> +	  Generic interconnect driver for Exynos SoCs.
> diff --git a/drivers/interconnect/exynos/Makefile b/drivers/interconnect/exynos/Makefile
> new file mode 100644
> index 0000000..e19d1df
> --- /dev/null
> +++ b/drivers/interconnect/exynos/Makefile
> @@ -0,0 +1,4 @@
> +# SPDX-License-Identifier: GPL-2.0
> +exynos-interconnect-objs		:= exynos.o
> +
> +obj-$(CONFIG_INTERCONNECT_EXYNOS)	+= exynos-interconnect.o
> diff --git a/drivers/interconnect/exynos/exynos.c b/drivers/interconnect/exynos/exynos.c
> new file mode 100644
> index 0000000..8278194
> --- /dev/null
> +++ b/drivers/interconnect/exynos/exynos.c
> @@ -0,0 +1,185 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Exynos generic interconnect provider driver
> + *
> + * Copyright (c) 2020 Samsung Electronics Co., Ltd.
> + *
> + * Authors: Artur Świgoń <a.swigon@samsung.com>
> + *          Sylwester Nawrocki <s.nawrocki@samsung.com>
> + */
> +#include <linux/device.h>
> +#include <linux/interconnect-provider.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_qos.h>
> +
> +#define kbps_to_khz(x) ((x) / 8)
> +
> +struct exynos_icc_priv {
> +	struct device *dev;
> +
> +	/* One interconnect node per provider */
> +	struct icc_provider provider;
> +	struct icc_node *node;
> +
> +	struct dev_pm_qos_request qos_req;
> +};
> +
> +static struct icc_node *exynos_icc_get_parent(struct device_node *np)
> +{
> +	struct of_phandle_args args;
> +	int num, ret;
> +
> +	num = of_count_phandle_with_args(np, "samsung,interconnect-parent",
> +					"#interconnect-cells");
> +	if (num != 1)
> +		return NULL; /* parent nodes are optional */
> +
> +	ret = of_parse_phandle_with_args(np, "samsung,interconnect-parent",
> +					"#interconnect-cells", 0, &args);
> +	if (ret < 0)
> +		return ERR_PTR(ret);
> +
> +	of_node_put(args.np);
> +
> +	return of_icc_get_from_provider(&args);

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

Best regards,
Krzysztof

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

[Sylwester Nawrocki]

On 02.06.2020 10:21, Krzysztof Kozlowski wrote:
>> +static struct icc_node *exynos_icc_get_parent(struct device_node *np)
>> +{
>> +	struct of_phandle_args args;
>> +	int num, ret;
>> +
>> +	num = of_count_phandle_with_args(np, "samsung,interconnect-parent",
>> +					"#interconnect-cells");
>> +	if (num != 1)
>> +		return NULL; /* parent nodes are optional */
>> +
>> +	ret = of_parse_phandle_with_args(np, "samsung,interconnect-parent",
>> +					"#interconnect-cells", 0, &args);
>> +	if (ret < 0)
>> +		return ERR_PTR(ret);
>> +
>> +	of_node_put(args.np);
>> +
>> +	return of_icc_get_from_provider(&args);

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

Indeed, thanks, I will amend that. It seems the node reference count decrementing
is often not done properly after existing calls to of_parse_phandle_with_args().

-- 
Thanks,
Sylwester

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

[Georgi Djakov]

Hi Sylwester,

Thanks for the patch and apologies for the delayed reply.

On 5/29/20 19:31, Sylwester Nawrocki wrote:
> This patch adds a generic interconnect driver for Exynos SoCs in order
> to provide interconnect functionality for each "samsung,exynos-bus"
> compatible device.
> 
> The SoC topology is a graph (or more specifically, a tree) and its
> edges are specified using the 'samsung,interconnect-parent' in the
> DT. Due to unspecified relative probing order, -EPROBE_DEFER may be
> propagated to ensure that the parent is probed before its children.
> 
> Each bus is now an interconnect provider and an interconnect node as
> well (cf. Documentation/interconnect/interconnect.rst), i.e. every bus
> registers itself as a node. Node IDs are not hardcoded but rather
> assigned dynamically at runtime. This approach allows for using this
> driver with various Exynos SoCs.
> 
> Frequencies requested via the interconnect API for a given node are
> propagated to devfreq using dev_pm_qos_update_request(). Please note
> that it is not an error when CONFIG_INTERCONNECT is 'n', in which
> case all interconnect API functions are no-op.
> 
> Signed-off-by: Artur Świgoń <a.swigon@samsung.com>
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> 
> Changes for v5:
>  - adjust to renamed exynos,interconnect-parent-node property,
>  - use automatically generated platform device id as the interconect
>    node id instead of a now unavailable devfreq->id field,
>  - add icc_ prefix to some variables to make the code more self-commenting,
>  - use icc_nodes_remove() instead of icc_node_del() + icc_node_destroy(),
>  - adjust to exynos,interconnect-parent-node property rename to
>    samsung,interconnect-parent,
>  - converted to a separate platform driver in drivers/i.nterconnect.
> ---
>  drivers/interconnect/Kconfig         |   1 +
>  drivers/interconnect/Makefile        |   1 +
>  drivers/interconnect/exynos/Kconfig  |   6 ++
>  drivers/interconnect/exynos/Makefile |   4 +
>  drivers/interconnect/exynos/exynos.c | 185 +++++++++++++++++++++++++++++++++++
>  5 files changed, 197 insertions(+)
>  create mode 100644 drivers/interconnect/exynos/Kconfig
>  create mode 100644 drivers/interconnect/exynos/Makefile
>  create mode 100644 drivers/interconnect/exynos/exynos.c
[..]
> +static struct icc_node *exynos_icc_get_parent(struct device_node *np)
> +{
> +	struct of_phandle_args args;
> +	int num, ret;
> +
> +	num = of_count_phandle_with_args(np, "samsung,interconnect-parent",
> +					"#interconnect-cells");
> +	if (num != 1)
> +		return NULL; /* parent nodes are optional */
> +
> +	ret = of_parse_phandle_with_args(np, "samsung,interconnect-parent",
> +					"#interconnect-cells", 0, &args);
> +	if (ret < 0)
> +		return ERR_PTR(ret);
> +
> +	of_node_put(args.np);
> +
> +	return of_icc_get_from_provider(&args);
> +}
> +
> +

Nit: multiple blank lines

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

Nit: !IS_ERR_OR_NULL?

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

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

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

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

Thanks,
Georgi

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

[Sylwester Nawrocki]

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.

-- 
Regards,
Sylwester

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

[Georgi Djakov]

Hi Sylwester,

On 7/2/20 15:01, Sylwester Nawrocki wrote:
> Hi Georgi,
> 
> On 01.07.2020 14:50, Georgi Djakov wrote:
>> Thanks for the patch and apologies for the delayed reply.
> 
> Thanks, no problem. It's actually just in time as I put that patchset
> aside for a while and was just about to post an update.
>  
>> On 5/29/20 19:31, Sylwester Nawrocki wrote:
>>> This patch adds a generic interconnect driver for Exynos SoCs in order
>>> to provide interconnect functionality for each "samsung,exynos-bus"
>>> compatible device.
>>>
>>> The SoC topology is a graph (or more specifically, a tree) and its
>>> edges are specified using the 'samsung,interconnect-parent' in the
>>> DT. Due to unspecified relative probing order, -EPROBE_DEFER may be
>>> propagated to ensure that the parent is probed before its children.
>>>
>>> Each bus is now an interconnect provider and an interconnect node as
>>> well (cf. Documentation/interconnect/interconnect.rst), i.e. every bus
>>> registers itself as a node. Node IDs are not hardcoded but rather
>>> assigned dynamically at runtime. This approach allows for using this
>>> driver with various Exynos SoCs.
>>>
>>> Frequencies requested via the interconnect API for a given node are
>>> propagated to devfreq using dev_pm_qos_update_request(). Please note
>>> that it is not an error when CONFIG_INTERCONNECT is 'n', in which
>>> case all interconnect API functions are no-op.
>>>
>>> Signed-off-by: Artur Świgoń <a.swigon@samsung.com>
>>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> 
>>> +static struct icc_node *exynos_icc_get_parent(struct device_node *np)
>>> +{
>>> +	struct of_phandle_args args;
>>> +	int num, ret;
>>> +
>>> +	num = of_count_phandle_with_args(np, "samsung,interconnect-parent",
>>> +					"#interconnect-cells");
>>> +	if (num != 1)
>>> +		return NULL; /* parent nodes are optional */
>>> +
>>> +	ret = of_parse_phandle_with_args(np, "samsung,interconnect-parent",
>>> +					"#interconnect-cells", 0, &args);
>>> +	if (ret < 0)
>>> +		return ERR_PTR(ret);
>>> +
>>> +	of_node_put(args.np);
>>> +
>>> +	return of_icc_get_from_provider(&args);
>>> +}
>>> +
>>> +
>>
>> Nit: multiple blank lines
> 
> Fixed.
> 
>> [..]
>>> +static struct icc_node *exynos_generic_icc_xlate(struct of_phandle_args *spec,
>>> +						 void *data)
>>> +{
>>> +	struct exynos_icc_priv *priv = data;
>>> +
>>> +	if (spec->np != priv->dev->parent->of_node)
>>> +		return ERR_PTR(-EINVAL);
>>> +
>>> +	return priv->node;
>>> +}
>>> +
>>> +static int exynos_generic_icc_remove(struct platform_device *pdev)
>>> +{
>>> +	struct exynos_icc_priv *priv = platform_get_drvdata(pdev);
>>> +	struct icc_node *parent_node, *node = priv->node;
>>> +
>>> +	parent_node = exynos_icc_get_parent(priv->dev->parent->of_node);
>>> +	if (parent_node && !IS_ERR(parent_node))
>>
>> Nit: !IS_ERR_OR_NULL?
> 
> It was left on purpose that way but I changed it now to IS_ERR_OR_NULL.

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

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

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

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

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

Thanks,
Georgi

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

[Sylwester Nawrocki]

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.

-- 
Regards,
Sylwester

[RFC PATCH v5 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>

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

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

[Chanwoo Choi]

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.

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

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

[Sylwester Nawrocki]

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, 
Sylwester

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

[Chanwoo Choi]

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.

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

[RFC PATCH v5 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.

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

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

[Chanwoo Choi]

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>

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

[RFC PATCH v5 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:
 - 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>;
 };
 
 &pmu {
-- 
2.7.4

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

[Chanwoo Choi]

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>

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

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

[Sylwester Nawrocki]

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;
+			}
+		}
+	}
+
+	/* add 20% safety margin */
+	bandwidth = bandwidth / 4 * 5;
+	dev_dbg(ctx->dev, "exynos-mixer: safe bandwidth %ld Bps\n", bandwidth);
+
+	avg_bw = peak_bw = Bps_to_icc(bandwidth);
+	icc_set_bw(ctx->icc_path, avg_bw, peak_bw);
+}
+
 static void mixer_atomic_begin(struct exynos_drm_crtc *crtc)
 {
 	struct mixer_context *ctx = crtc->ctx;
@@ -985,6 +1023,7 @@ static void mixer_atomic_flush(struct exynos_drm_crtc *crtc)
 	if (!test_bit(MXR_BIT_POWERED, &mixer_ctx->flags))
 		return;
 
+	mixer_set_memory_bandwidth(crtc);
 	mixer_enable_sync(mixer_ctx);
 	exynos_crtc_handle_event(crtc);
 }
@@ -1032,6 +1071,7 @@ static void mixer_atomic_disable(struct exynos_drm_crtc *crtc)
 	for (i = 0; i < MIXER_WIN_NR; i++)
 		mixer_disable_plane(crtc, &ctx->planes[i]);
 
+	mixer_set_memory_bandwidth(crtc);
 	exynos_drm_pipe_clk_enable(crtc, false);
 
 	pm_runtime_put(ctx->dev);
@@ -1223,12 +1263,22 @@ 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);
@@ -1236,6 +1286,7 @@ static int mixer_probe(struct platform_device *pdev)
 	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);
@@ -1245,17 +1296,29 @@ static int mixer_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, ctx);
 
 	ret = component_add(&pdev->dev, &mixer_component_ops);
-	if (!ret)
-		pm_runtime_enable(dev);
+	if (ret < 0)
+		goto err;
+
+	pm_runtime_enable(dev);
+
+	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: [RFC PATCH v5 6/6] drm: exynos: mixer: Add interconnect support

[Chanwoo Choi]

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)

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

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

[Sylwester Nawrocki]

Hi Chanwoo,

On 01.06.2020 09:58, Chanwoo Choi wrote:
> 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>

>>  drivers/gpu/drm/exynos/exynos_mixer.c | 73 ++++++++++++++++++++++++++++++++---
>>  1 file changed, 68 insertions(+), 5 deletions(-)
  
>> +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.

OK, I will improve the commit message and add a comment to the function.
As far as I understand this simply calculates amount of data in bytes that
needs to be read from the system memory per second for given video stream,
by summing values for each mixer window and for each color plane within 
a window.

Anyway, the patch will to be changed as after some more tests it seems
the calculation works for LEFTBUS but DMC clock on Odroid U3 needs to
be set to 400 MHz, rather than to at least 160 MHz. With any lower value 
the mixer underflow error interrupts are being triggered and eventually 
the IOMMU page fault occurs.

--
Regards,
Sylwester

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

[Sylwester Nawrocki]

Cc: Rob, devicetree ML

On 29.05.2020 18:31, Sylwester Nawrocki wrote:
> 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 to avoid issues like the one discussed in thread [1].
> 
> This patch series depends on 3 patches from Artur for the interconnect API 
> [2], which introduce following changes:
> 
>  - exporting of_icc_get_from_provider() to avoid hard coding every graph 
>    edge in the DT or driver source,
>  - relaxing the requirement on #interconnect-cells, so there is no need 
>    to provide dummy node IDs in the DT, 
>  - adding new field in struct icc_provider to explicitly allow configuring 
>    node pairs from two different providers.
> 
> This series adds implementation of 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].
> 
> The series has been tested on Odroid U3 board. It is based on icc-next 
> branch with devfreq-next branch merged and patches [2] applied.
> 
> --
> Regards,
> Sylwester
> 
> --
> Changes since v3 [3] (v4 skipped to align with patchset [1]), detailed 
> changes are listed at 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 [4]:
>  - 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://lore.kernel.org/linux-pm/20191220115653.6487-1-a.swigon@samsung.com
> [4] https://patchwork.kernel.org/cover/11054417/ (v1 of this RFC)
> [5] https://patchwork.kernel.org/cover/11152595/ (v2 of this RFC)
> 
> 
> Artur Świgoń (1):
>   ARM: dts: exynos: Add interconnects to Exynos4412 mixer
> 
> Marek Szyprowski (1):
>   drm: exynos: mixer: Add interconnect support
> 
> Sylwester Nawrocki (4):
>   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
> 
>  .../devicetree/bindings/devfreq/exynos-bus.txt     |  15 +-
>  arch/arm/boot/dts/exynos4412.dtsi                  |   6 +
>  drivers/devfreq/exynos-bus.c                       |  17 ++
>  drivers/gpu/drm/exynos/exynos_mixer.c              |  73 +++++++-
>  drivers/interconnect/Kconfig                       |   1 +
>  drivers/interconnect/Makefile                      |   1 +
>  drivers/interconnect/exynos/Kconfig                |   6 +
>  drivers/interconnect/exynos/Makefile               |   4 +
>  drivers/interconnect/exynos/exynos.c               | 185 +++++++++++++++++++++
>  9 files changed, 301 insertions(+), 7 deletions(-)
>  create mode 100644 drivers/interconnect/exynos/Kconfig
>  create mode 100644 drivers/interconnect/exynos/Makefile
>  create mode 100644 drivers/interconnect/exynos/exynos.c