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

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

[Sylwester Nawrocki]

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

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

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

Changes since v6:
 - the interconnect consumer DT bindings are now used to describe dependencies
   of the interconnects (samsung,exynos-bus nodes),
 - bus-width property replaced with samsung,data-clk-ratio,
 - adaptation to recent changes in the interconnect code
   (of_icc_get_from_provider(), icc_node_add()).

The series has been tested on Odroid U3 board. It is based on v5.10-rc1.

--
Regards,
Sylwester


Changes since v5:
 - addition of "bus-width: DT property, which specifies data width
   of the interconnect bus (patches 1...2/6),
 - addition of synchronization of the interconnect bandwidth setting
   with VSYNC (patch 6/6).

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

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

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


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


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

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

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

--
2.7.4

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

[Sylwester Nawrocki]

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

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

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

Changes since v6:
 - the interconnect consumer DT bindings are now used to describe dependencies
   of the interconnects (samsung,exynos-bus nodes),
 - bus-width property replaced with samsung,data-clk-ratio,
 - adaptation to recent changes in the interconnect code
   (of_icc_get_from_provider(), icc_node_add()).

The series has been tested on Odroid U3 board. It is based on v5.10-rc1.

--
Regards,
Sylwester


Changes since v5:
 - addition of "bus-width: DT property, which specifies data width
   of the interconnect bus (patches 1...2/6),
 - addition of synchronization of the interconnect bandwidth setting
   with VSYNC (patch 6/6).

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

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

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


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


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

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

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

--
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v7 1/6] dt-bindings: devfreq: Add documentation for the interconnect properties

[Sylwester Nawrocki]

Add documentation for new optional properties in the exynos bus nodes:
interconnects, #interconnect-cells, samsung,data-clock-ratio.
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 v7:
 - bus-width property replaced with samsung,data-clock-ratio,
 - the interconnect consumer bindings used instead of vendor specific
   properties

Changes for v6:
 - added dts example of bus hierarchy definition and the interconnect
   consumer,
 - added new bus-width property.

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

diff --git a/Documentation/devicetree/bindings/devfreq/exynos-bus.txt b/Documentation/devicetree/bindings/devfreq/exynos-bus.txt
index e71f752..e34175c 100644
--- a/Documentation/devicetree/bindings/devfreq/exynos-bus.txt
+++ b/Documentation/devicetree/bindings/devfreq/exynos-bus.txt
@@ -51,6 +51,16 @@ 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):
+- #interconnect-cells: should be 0.
+- interconnects: as documented in ../interconnect.txt, describes a path
+  at the higher level interconnects used by this interconnect provider.
+  If this interconnect provider is a parent of a top level interconnect
+  provider the property contains only one phandle.
+
+- samsung,data-clock-ratio: ratio of the data troughput in B/s to minimum data
+   clock frequency in Hz, default value is 8 when this property is missing.
+
 Detailed correlation between sub-blocks and power line according to Exynos SoC:
 - In case of Exynos3250, there are two power line as following:
 	VDD_MIF |--- DMC
@@ -135,7 +145,7 @@ Detailed correlation between sub-blocks and power line according to Exynos SoC:
 		|--- PERIC (Fixed clock rate)
 		|--- FSYS  (Fixed clock rate)
 
-Example1:
+Example 1:
 	Show the AXI buses of Exynos3250 SoC. Exynos3250 divides the buses to
 	power line (regulator). The MIF (Memory Interface) AXI bus is used to
 	transfer data between DRAM and CPU and uses the VDD_MIF regulator.
@@ -184,7 +194,7 @@ Example1:
 	|L5   |200000 |200000  |400000 |300000 |       ||1000000 |
 	----------------------------------------------------------
 
-Example2 :
+Example 2:
 	The bus of DMC (Dynamic Memory Controller) block in exynos3250.dtsi
 	is listed below:
 
@@ -419,3 +429,57 @@ Example2 :
 		devfreq = <&bus_leftbus>;
 		status = "okay";
 	};
+
+Example 3:
+	An interconnect path "bus_display -- bus_leftbus -- bus_dmc" on
+	Exynos4412 SoC with video mixer as an interconnect consumer device.
+
+	soc {
+		bus_dmc: bus_dmc {
+			compatible = "samsung,exynos-bus";
+			clocks = <&clock CLK_DIV_DMC>;
+			clock-names = "bus";
+			operating-points-v2 = <&bus_dmc_opp_table>;
+			samsung,data-clock-ratio = <4>;
+			#interconnect-cells = <0>;
+		};
+
+		bus_leftbus: bus_leftbus {
+			compatible = "samsung,exynos-bus";
+			clocks = <&clock CLK_DIV_GDL>;
+			clock-names = "bus";
+			operating-points-v2 = <&bus_leftbus_opp_table>;
+			#interconnect-cells = <0>;
+			interconnects = <&bus_dmc>;
+		};
+
+		bus_display: bus_display {
+			compatible = "samsung,exynos-bus";
+			clocks = <&clock CLK_ACLK160>;
+			clock-names = "bus";
+			operating-points-v2 = <&bus_display_opp_table>;
+			#interconnect-cells = <0>;
+			interconnects = <&bus_leftbus &bus_dmc>;
+		};
+
+		bus_dmc_opp_table: opp_table1 {
+			compatible = "operating-points-v2";
+			/* ... */
+		}
+
+		bus_leftbus_opp_table: opp_table3 {
+			compatible = "operating-points-v2";
+			/* ... */
+		};
+
+		bus_display_opp_table: opp_table4 {
+			compatible = "operating-points-v2";
+			/* .. */
+		};
+
+		&mixer {
+			compatible = "samsung,exynos4212-mixer";
+			interconnects = <&bus_display &bus_dmc>;
+			/* ... */
+		};
+	};
-- 
2.7.4

[PATCH v7 1/6] dt-bindings: devfreq: Add documentation for the interconnect properties

[Sylwester Nawrocki]

Add documentation for new optional properties in the exynos bus nodes:
interconnects, #interconnect-cells, samsung,data-clock-ratio.
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 v7:
 - bus-width property replaced with samsung,data-clock-ratio,
 - the interconnect consumer bindings used instead of vendor specific
   properties

Changes for v6:
 - added dts example of bus hierarchy definition and the interconnect
   consumer,
 - added new bus-width property.

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

diff --git a/Documentation/devicetree/bindings/devfreq/exynos-bus.txt b/Documentation/devicetree/bindings/devfreq/exynos-bus.txt
index e71f752..e34175c 100644
--- a/Documentation/devicetree/bindings/devfreq/exynos-bus.txt
+++ b/Documentation/devicetree/bindings/devfreq/exynos-bus.txt
@@ -51,6 +51,16 @@ 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):
+- #interconnect-cells: should be 0.
+- interconnects: as documented in ../interconnect.txt, describes a path
+  at the higher level interconnects used by this interconnect provider.
+  If this interconnect provider is a parent of a top level interconnect
+  provider the property contains only one phandle.
+
+- samsung,data-clock-ratio: ratio of the data troughput in B/s to minimum data
+   clock frequency in Hz, default value is 8 when this property is missing.
+
 Detailed correlation between sub-blocks and power line according to Exynos SoC:
 - In case of Exynos3250, there are two power line as following:
 	VDD_MIF |--- DMC
@@ -135,7 +145,7 @@ Detailed correlation between sub-blocks and power line according to Exynos SoC:
 		|--- PERIC (Fixed clock rate)
 		|--- FSYS  (Fixed clock rate)
 
-Example1:
+Example 1:
 	Show the AXI buses of Exynos3250 SoC. Exynos3250 divides the buses to
 	power line (regulator). The MIF (Memory Interface) AXI bus is used to
 	transfer data between DRAM and CPU and uses the VDD_MIF regulator.
@@ -184,7 +194,7 @@ Example1:
 	|L5   |200000 |200000  |400000 |300000 |       ||1000000 |
 	----------------------------------------------------------
 
-Example2 :
+Example 2:
 	The bus of DMC (Dynamic Memory Controller) block in exynos3250.dtsi
 	is listed below:
 
@@ -419,3 +429,57 @@ Example2 :
 		devfreq = <&bus_leftbus>;
 		status = "okay";
 	};
+
+Example 3:
+	An interconnect path "bus_display -- bus_leftbus -- bus_dmc" on
+	Exynos4412 SoC with video mixer as an interconnect consumer device.
+
+	soc {
+		bus_dmc: bus_dmc {
+			compatible = "samsung,exynos-bus";
+			clocks = <&clock CLK_DIV_DMC>;
+			clock-names = "bus";
+			operating-points-v2 = <&bus_dmc_opp_table>;
+			samsung,data-clock-ratio = <4>;
+			#interconnect-cells = <0>;
+		};
+
+		bus_leftbus: bus_leftbus {
+			compatible = "samsung,exynos-bus";
+			clocks = <&clock CLK_DIV_GDL>;
+			clock-names = "bus";
+			operating-points-v2 = <&bus_leftbus_opp_table>;
+			#interconnect-cells = <0>;
+			interconnects = <&bus_dmc>;
+		};
+
+		bus_display: bus_display {
+			compatible = "samsung,exynos-bus";
+			clocks = <&clock CLK_ACLK160>;
+			clock-names = "bus";
+			operating-points-v2 = <&bus_display_opp_table>;
+			#interconnect-cells = <0>;
+			interconnects = <&bus_leftbus &bus_dmc>;
+		};
+
+		bus_dmc_opp_table: opp_table1 {
+			compatible = "operating-points-v2";
+			/* ... */
+		}
+
+		bus_leftbus_opp_table: opp_table3 {
+			compatible = "operating-points-v2";
+			/* ... */
+		};
+
+		bus_display_opp_table: opp_table4 {
+			compatible = "operating-points-v2";
+			/* .. */
+		};
+
+		&mixer {
+			compatible = "samsung,exynos4212-mixer";
+			interconnects = <&bus_display &bus_dmc>;
+			/* ... */
+		};
+	};
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v7 1/6] dt-bindings: devfreq: Add documentation for the interconnect properties

[Krzysztof Kozlowski]

On Fri, Oct 30, 2020 at 01:51:44PM +0100, Sylwester Nawrocki wrote:
> Add documentation for new optional properties in the exynos bus nodes:
> interconnects, #interconnect-cells, samsung,data-clock-ratio.
> 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 v7:
>  - bus-width property replaced with samsung,data-clock-ratio,
>  - the interconnect consumer bindings used instead of vendor specific
>    properties
> 
> Changes for v6:
>  - added dts example of bus hierarchy definition and the interconnect
>    consumer,
>  - added new bus-width property.
> 
> Changes for v5:
>  - exynos,interconnect-parent-node renamed to samsung,interconnect-parent
> ---
>  .../devicetree/bindings/devfreq/exynos-bus.txt     | 68 +++++++++++++++++++++-
>  1 file changed, 66 insertions(+), 2 deletions(-)
> 

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

Best regards,
Krzysztof

Re: [PATCH v7 1/6] dt-bindings: devfreq: Add documentation for the interconnect properties

[Krzysztof Kozlowski]

On Fri, Oct 30, 2020 at 01:51:44PM +0100, Sylwester Nawrocki wrote:
> Add documentation for new optional properties in the exynos bus nodes:
> interconnects, #interconnect-cells, samsung,data-clock-ratio.
> 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 v7:
>  - bus-width property replaced with samsung,data-clock-ratio,
>  - the interconnect consumer bindings used instead of vendor specific
>    properties
> 
> Changes for v6:
>  - added dts example of bus hierarchy definition and the interconnect
>    consumer,
>  - added new bus-width property.
> 
> Changes for v5:
>  - exynos,interconnect-parent-node renamed to samsung,interconnect-parent
> ---
>  .../devicetree/bindings/devfreq/exynos-bus.txt     | 68 +++++++++++++++++++++-
>  1 file changed, 66 insertions(+), 2 deletions(-)
> 

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

Best regards,
Krzysztof
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v7 1/6] dt-bindings: devfreq: Add documentation for the interconnect properties

[Chanwoo Choi]

Hi Sylwester,

This patch contains one typo.

On 10/30/20 9:51 PM, Sylwester Nawrocki wrote:
> Add documentation for new optional properties in the exynos bus nodes:
> interconnects, #interconnect-cells, samsung,data-clock-ratio.
> 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 v7:
>  - bus-width property replaced with samsung,data-clock-ratio,
>  - the interconnect consumer bindings used instead of vendor specific
>    properties
> 
> Changes for v6:
>  - added dts example of bus hierarchy definition and the interconnect
>    consumer,
>  - added new bus-width property.
> 
> Changes for v5:
>  - exynos,interconnect-parent-node renamed to samsung,interconnect-parent
> ---
>  .../devicetree/bindings/devfreq/exynos-bus.txt     | 68 +++++++++++++++++++++-
>  1 file changed, 66 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/devfreq/exynos-bus.txt b/Documentation/devicetree/bindings/devfreq/exynos-bus.txt
> index e71f752..e34175c 100644
> --- a/Documentation/devicetree/bindings/devfreq/exynos-bus.txt
> +++ b/Documentation/devicetree/bindings/devfreq/exynos-bus.txt
> @@ -51,6 +51,16 @@ 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):
> +- #interconnect-cells: should be 0.
> +- interconnects: as documented in ../interconnect.txt, describes a path
> +  at the higher level interconnects used by this interconnect provider.
> +  If this interconnect provider is a parent of a top level interconnect
> +  provider the property contains only one phandle.
> +
> +- samsung,data-clock-ratio: ratio of the data troughput in B/s to minimum data

s/troughput/throughput

> +   clock frequency in Hz, default value is 8 when this property is missing.
> +
>  Detailed correlation between sub-blocks and power line according to Exynos SoC:
>  - In case of Exynos3250, there are two power line as following:
>  	VDD_MIF |--- DMC
> @@ -135,7 +145,7 @@ Detailed correlation between sub-blocks and power line according to Exynos SoC:
>  		|--- PERIC (Fixed clock rate)
>  		|--- FSYS  (Fixed clock rate)
>  
> -Example1:
> +Example 1:
>  	Show the AXI buses of Exynos3250 SoC. Exynos3250 divides the buses to
>  	power line (regulator). The MIF (Memory Interface) AXI bus is used to
>  	transfer data between DRAM and CPU and uses the VDD_MIF regulator.
> @@ -184,7 +194,7 @@ Example1:
>  	|L5   |200000 |200000  |400000 |300000 |       ||1000000 |
>  	----------------------------------------------------------
>  
> -Example2 :
> +Example 2:
>  	The bus of DMC (Dynamic Memory Controller) block in exynos3250.dtsi
>  	is listed below:
>  
> @@ -419,3 +429,57 @@ Example2 :
>  		devfreq = <&bus_leftbus>;
>  		status = "okay";
>  	};
> +
> +Example 3:
> +	An interconnect path "bus_display -- bus_leftbus -- bus_dmc" on
> +	Exynos4412 SoC with video mixer as an interconnect consumer device.
> +
> +	soc {
> +		bus_dmc: bus_dmc {
> +			compatible = "samsung,exynos-bus";
> +			clocks = <&clock CLK_DIV_DMC>;
> +			clock-names = "bus";
> +			operating-points-v2 = <&bus_dmc_opp_table>;
> +			samsung,data-clock-ratio = <4>;
> +			#interconnect-cells = <0>;
> +		};
> +
> +		bus_leftbus: bus_leftbus {
> +			compatible = "samsung,exynos-bus";
> +			clocks = <&clock CLK_DIV_GDL>;
> +			clock-names = "bus";
> +			operating-points-v2 = <&bus_leftbus_opp_table>;
> +			#interconnect-cells = <0>;
> +			interconnects = <&bus_dmc>;
> +		};
> +
> +		bus_display: bus_display {
> +			compatible = "samsung,exynos-bus";
> +			clocks = <&clock CLK_ACLK160>;
> +			clock-names = "bus";
> +			operating-points-v2 = <&bus_display_opp_table>;
> +			#interconnect-cells = <0>;
> +			interconnects = <&bus_leftbus &bus_dmc>;

About adding two icc_node, I queried you on patch2.

> +		};
> +
> +		bus_dmc_opp_table: opp_table1 {
> +			compatible = "operating-points-v2";
> +			/* ... */
> +		}
> +
> +		bus_leftbus_opp_table: opp_table3 {
> +			compatible = "operating-points-v2";
> +			/* ... */
> +		};
> +
> +		bus_display_opp_table: opp_table4 {
> +			compatible = "operating-points-v2";
> +			/* .. */
> +		};
> +
> +		&mixer {
> +			compatible = "samsung,exynos4212-mixer";
> +			interconnects = <&bus_display &bus_dmc>;
> +			/* ... */
> +		};
> +	};
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v7 1/6] dt-bindings: devfreq: Add documentation for the interconnect properties

[Chanwoo Choi]

Hi Sylwester,

This patch contains one typo.

On 10/30/20 9:51 PM, Sylwester Nawrocki wrote:
> Add documentation for new optional properties in the exynos bus nodes:
> interconnects, #interconnect-cells, samsung,data-clock-ratio.
> 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 v7:
>  - bus-width property replaced with samsung,data-clock-ratio,
>  - the interconnect consumer bindings used instead of vendor specific
>    properties
> 
> Changes for v6:
>  - added dts example of bus hierarchy definition and the interconnect
>    consumer,
>  - added new bus-width property.
> 
> Changes for v5:
>  - exynos,interconnect-parent-node renamed to samsung,interconnect-parent
> ---
>  .../devicetree/bindings/devfreq/exynos-bus.txt     | 68 +++++++++++++++++++++-
>  1 file changed, 66 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/devfreq/exynos-bus.txt b/Documentation/devicetree/bindings/devfreq/exynos-bus.txt
> index e71f752..e34175c 100644
> --- a/Documentation/devicetree/bindings/devfreq/exynos-bus.txt
> +++ b/Documentation/devicetree/bindings/devfreq/exynos-bus.txt
> @@ -51,6 +51,16 @@ 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):
> +- #interconnect-cells: should be 0.
> +- interconnects: as documented in ../interconnect.txt, describes a path
> +  at the higher level interconnects used by this interconnect provider.
> +  If this interconnect provider is a parent of a top level interconnect
> +  provider the property contains only one phandle.
> +
> +- samsung,data-clock-ratio: ratio of the data troughput in B/s to minimum data

s/troughput/throughput

> +   clock frequency in Hz, default value is 8 when this property is missing.
> +
>  Detailed correlation between sub-blocks and power line according to Exynos SoC:
>  - In case of Exynos3250, there are two power line as following:
>  	VDD_MIF |--- DMC
> @@ -135,7 +145,7 @@ Detailed correlation between sub-blocks and power line according to Exynos SoC:
>  		|--- PERIC (Fixed clock rate)
>  		|--- FSYS  (Fixed clock rate)
>  
> -Example1:
> +Example 1:
>  	Show the AXI buses of Exynos3250 SoC. Exynos3250 divides the buses to
>  	power line (regulator). The MIF (Memory Interface) AXI bus is used to
>  	transfer data between DRAM and CPU and uses the VDD_MIF regulator.
> @@ -184,7 +194,7 @@ Example1:
>  	|L5   |200000 |200000  |400000 |300000 |       ||1000000 |
>  	----------------------------------------------------------
>  
> -Example2 :
> +Example 2:
>  	The bus of DMC (Dynamic Memory Controller) block in exynos3250.dtsi
>  	is listed below:
>  
> @@ -419,3 +429,57 @@ Example2 :
>  		devfreq = <&bus_leftbus>;
>  		status = "okay";
>  	};
> +
> +Example 3:
> +	An interconnect path "bus_display -- bus_leftbus -- bus_dmc" on
> +	Exynos4412 SoC with video mixer as an interconnect consumer device.
> +
> +	soc {
> +		bus_dmc: bus_dmc {
> +			compatible = "samsung,exynos-bus";
> +			clocks = <&clock CLK_DIV_DMC>;
> +			clock-names = "bus";
> +			operating-points-v2 = <&bus_dmc_opp_table>;
> +			samsung,data-clock-ratio = <4>;
> +			#interconnect-cells = <0>;
> +		};
> +
> +		bus_leftbus: bus_leftbus {
> +			compatible = "samsung,exynos-bus";
> +			clocks = <&clock CLK_DIV_GDL>;
> +			clock-names = "bus";
> +			operating-points-v2 = <&bus_leftbus_opp_table>;
> +			#interconnect-cells = <0>;
> +			interconnects = <&bus_dmc>;
> +		};
> +
> +		bus_display: bus_display {
> +			compatible = "samsung,exynos-bus";
> +			clocks = <&clock CLK_ACLK160>;
> +			clock-names = "bus";
> +			operating-points-v2 = <&bus_display_opp_table>;
> +			#interconnect-cells = <0>;
> +			interconnects = <&bus_leftbus &bus_dmc>;

About adding two icc_node, I queried you on patch2.

> +		};
> +
> +		bus_dmc_opp_table: opp_table1 {
> +			compatible = "operating-points-v2";
> +			/* ... */
> +		}
> +
> +		bus_leftbus_opp_table: opp_table3 {
> +			compatible = "operating-points-v2";
> +			/* ... */
> +		};
> +
> +		bus_display_opp_table: opp_table4 {
> +			compatible = "operating-points-v2";
> +			/* .. */
> +		};
> +
> +		&mixer {
> +			compatible = "samsung,exynos4212-mixer";
> +			interconnects = <&bus_display &bus_dmc>;
> +			/* ... */
> +		};
> +	};
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Sylwester Nawrocki]

This patch adds a generic interconnect driver for Exynos SoCs in order
to provide interconnect functionality for each "samsung,exynos-bus"
compatible device.

The SoC topology is a graph (or more specifically, a tree) and its
edges are specified using the 'samsung,interconnect-parent' in the
DT. Due to unspecified relative probing order, -EPROBE_DEFER may be
propagated to ensure that the parent is probed before its children.

Each bus is now an interconnect provider and an interconnect node as
well (cf. Documentation/interconnect/interconnect.rst), i.e. every bus
registers itself as a node. Node IDs are not hardcoded but rather
assigned dynamically at runtime. This approach allows for using this
driver with various Exynos SoCs.

Frequencies requested via the interconnect API for a given node are
propagated to devfreq using dev_pm_qos_update_request(). Please note
that it is not an error when CONFIG_INTERCONNECT is 'n', in which
case all interconnect API functions are no-op.

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

Signed-off-by: Artur Świgoń <a.swigon@samsung.com>
Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
---
Changes for v7:
 - adjusted to the DT property changes: "interconnects" instead
   of "samsung,interconnect-parent", "samsung,data-clk-ratio"
   instead of "bus-width",
 - adaptation to of_icc_get_from_provider() function changes
   in v5.10-rc1.

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

Changes for v5:
 - adjust to renamed exynos,interconnect-parent-node property,
 - use automatically generated platform device id as the interconect
   node id instead of a now unavailable devfreq->id field,
 - add icc_ prefix to some variables to make the code more self-commenting,
 - use icc_nodes_remove() instead of icc_node_del() + icc_node_destroy(),
 - adjust to exynos,interconnect-parent-node property rename to
   samsung,interconnect-parent,
 - converted to a separate platform driver in drivers/interconnect.

---
 drivers/interconnect/Kconfig         |   1 +
 drivers/interconnect/Makefile        |   1 +
 drivers/interconnect/exynos/Kconfig  |   6 ++
 drivers/interconnect/exynos/Makefile |   4 +
 drivers/interconnect/exynos/exynos.c | 198 +++++++++++++++++++++++++++++++++++
 5 files changed, 210 insertions(+)
 create mode 100644 drivers/interconnect/exynos/Kconfig
 create mode 100644 drivers/interconnect/exynos/Makefile
 create mode 100644 drivers/interconnect/exynos/exynos.c

diff --git a/drivers/interconnect/Kconfig b/drivers/interconnect/Kconfig
index 5b7204e..eca6eda 100644
--- a/drivers/interconnect/Kconfig
+++ b/drivers/interconnect/Kconfig
@@ -11,6 +11,7 @@ menuconfig INTERCONNECT
 
 if INTERCONNECT
 
+source "drivers/interconnect/exynos/Kconfig"
 source "drivers/interconnect/imx/Kconfig"
 source "drivers/interconnect/qcom/Kconfig"
 
diff --git a/drivers/interconnect/Makefile b/drivers/interconnect/Makefile
index d203520..665538d 100644
--- a/drivers/interconnect/Makefile
+++ b/drivers/interconnect/Makefile
@@ -4,5 +4,6 @@ CFLAGS_core.o				:= -I$(src)
 icc-core-objs				:= core.o bulk.o
 
 obj-$(CONFIG_INTERCONNECT)		+= icc-core.o
+obj-$(CONFIG_INTERCONNECT_EXYNOS)	+= exynos/
 obj-$(CONFIG_INTERCONNECT_IMX)		+= imx/
 obj-$(CONFIG_INTERCONNECT_QCOM)		+= qcom/
diff --git a/drivers/interconnect/exynos/Kconfig b/drivers/interconnect/exynos/Kconfig
new file mode 100644
index 0000000..e51e52e
--- /dev/null
+++ b/drivers/interconnect/exynos/Kconfig
@@ -0,0 +1,6 @@
+# SPDX-License-Identifier: GPL-2.0-only
+config INTERCONNECT_EXYNOS
+	tristate "Exynos generic interconnect driver"
+	depends on ARCH_EXYNOS || COMPILE_TEST
+	help
+	  Generic interconnect driver for Exynos SoCs.
diff --git a/drivers/interconnect/exynos/Makefile b/drivers/interconnect/exynos/Makefile
new file mode 100644
index 0000000..e19d1df
--- /dev/null
+++ b/drivers/interconnect/exynos/Makefile
@@ -0,0 +1,4 @@
+# SPDX-License-Identifier: GPL-2.0
+exynos-interconnect-objs		:= exynos.o
+
+obj-$(CONFIG_INTERCONNECT_EXYNOS)	+= exynos-interconnect.o
diff --git a/drivers/interconnect/exynos/exynos.c b/drivers/interconnect/exynos/exynos.c
new file mode 100644
index 0000000..772d1fc
--- /dev/null
+++ b/drivers/interconnect/exynos/exynos.c
@@ -0,0 +1,198 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Exynos generic interconnect provider driver
+ *
+ * Copyright (c) 2020 Samsung Electronics Co., Ltd.
+ *
+ * Authors: Artur Świgoń <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>
+#include <linux/slab.h>
+
+#define EXYNOS_ICC_DEFAULT_BUS_CLK_RATIO	8
+
+struct exynos_icc_priv {
+	struct device *dev;
+
+	/* One interconnect node per provider */
+	struct icc_provider provider;
+	struct icc_node *node;
+
+	struct dev_pm_qos_request qos_req;
+	u32 bus_clk_ratio;
+};
+
+static struct icc_node *exynos_icc_get_parent(struct device_node *np)
+{
+	struct of_phandle_args args;
+	struct icc_node_data *icc_node_data;
+	struct icc_node *icc_node;
+	int num, ret;
+
+	num = of_count_phandle_with_args(np, "interconnects",
+					 "#interconnect-cells");
+	if (num < 1)
+		return NULL; /* parent nodes are optional */
+
+	/* Get the interconnect target node */
+	ret = of_parse_phandle_with_args(np, "interconnects",
+					"#interconnect-cells", 0, &args);
+	if (ret < 0)
+		return ERR_PTR(ret);
+
+	icc_node_data = of_icc_get_from_provider(&args);
+	of_node_put(args.np);
+
+	if (IS_ERR(icc_node_data))
+		return ERR_CAST(icc_node_data);
+
+	icc_node = icc_node_data->node;
+	kfree(icc_node_data);
+
+	return icc_node;
+}
+
+static int exynos_generic_icc_set(struct icc_node *src, struct icc_node *dst)
+{
+	struct exynos_icc_priv *src_priv = src->data, *dst_priv = dst->data;
+	s32 src_freq = max(src->avg_bw, src->peak_bw) / src_priv->bus_clk_ratio;
+	s32 dst_freq = max(dst->avg_bw, dst->peak_bw) / dst_priv->bus_clk_ratio;
+	int ret;
+
+	ret = dev_pm_qos_update_request(&src_priv->qos_req, src_freq);
+	if (ret < 0) {
+		dev_err(src_priv->dev, "failed to update PM QoS of %s (src)\n",
+			src->name);
+		return ret;
+	}
+
+	ret = dev_pm_qos_update_request(&dst_priv->qos_req, dst_freq);
+	if (ret < 0) {
+		dev_err(dst_priv->dev, "failed to update PM QoS of %s (dst)\n",
+			dst->name);
+		return ret;
+	}
+
+	return 0;
+}
+
+static struct icc_node *exynos_generic_icc_xlate(struct of_phandle_args *spec,
+						 void *data)
+{
+	struct exynos_icc_priv *priv = data;
+
+	if (spec->np != priv->dev->parent->of_node)
+		return ERR_PTR(-EINVAL);
+
+	return priv->node;
+}
+
+static int exynos_generic_icc_remove(struct platform_device *pdev)
+{
+	struct exynos_icc_priv *priv = platform_get_drvdata(pdev);
+	struct icc_node *parent_node, *node = priv->node;
+
+	parent_node = exynos_icc_get_parent(priv->dev->parent->of_node);
+	if (parent_node && !IS_ERR(parent_node))
+		icc_link_destroy(node, parent_node);
+
+	icc_nodes_remove(&priv->provider);
+	icc_provider_del(&priv->provider);
+
+	return 0;
+}
+
+static int exynos_generic_icc_probe(struct platform_device *pdev)
+{
+	struct device *bus_dev = pdev->dev.parent;
+	struct exynos_icc_priv *priv;
+	struct icc_provider *provider;
+	struct icc_node *icc_node, *icc_parent_node;
+	int ret;
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->dev = &pdev->dev;
+	platform_set_drvdata(pdev, priv);
+
+	provider = &priv->provider;
+
+	provider->set = exynos_generic_icc_set;
+	provider->aggregate = icc_std_aggregate;
+	provider->xlate = exynos_generic_icc_xlate;
+	provider->dev = bus_dev;
+	provider->inter_set = true;
+	provider->data = priv;
+
+	ret = icc_provider_add(provider);
+	if (ret < 0)
+		return ret;
+
+	icc_node = icc_node_create(pdev->id);
+	if (IS_ERR(icc_node)) {
+		ret = PTR_ERR(icc_node);
+		goto err_prov_del;
+	}
+
+	priv->node = icc_node;
+	icc_node->name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%pOFn",
+					bus_dev->of_node);
+	if (of_property_read_u32(bus_dev->of_node, "samsung,data-clock-ratio",
+				 &priv->bus_clk_ratio))
+		priv->bus_clk_ratio = EXYNOS_ICC_DEFAULT_BUS_CLK_RATIO;
+
+	/*
+	 * Register a PM QoS request for the parent (devfreq) device.
+	 */
+	ret = dev_pm_qos_add_request(bus_dev, &priv->qos_req,
+				     DEV_PM_QOS_MIN_FREQUENCY, 0);
+	if (ret < 0)
+		goto err_node_del;
+
+	icc_node->data = priv;
+	icc_node_add(icc_node, provider);
+
+	icc_parent_node = exynos_icc_get_parent(bus_dev->of_node);
+	if (IS_ERR(icc_parent_node)) {
+		ret = PTR_ERR(icc_parent_node);
+		goto err_pmqos_del;
+	}
+	if (icc_parent_node) {
+		ret = icc_link_create(icc_node, icc_parent_node->id);
+		if (ret < 0)
+			goto err_pmqos_del;
+	}
+
+	return 0;
+
+err_pmqos_del:
+	dev_pm_qos_remove_request(&priv->qos_req);
+err_node_del:
+	icc_nodes_remove(provider);
+err_prov_del:
+	icc_provider_del(provider);
+	return ret;
+}
+
+static struct platform_driver exynos_generic_icc_driver = {
+	.driver = {
+		.name = "exynos-generic-icc",
+	},
+	.probe = exynos_generic_icc_probe,
+	.remove = exynos_generic_icc_remove,
+};
+module_platform_driver(exynos_generic_icc_driver);
+
+MODULE_DESCRIPTION("Exynos generic interconnect driver");
+MODULE_AUTHOR("Artur Świgoń <a.swigon@samsung.com>");
+MODULE_AUTHOR("Sylwester Nawrocki <s.nawrocki@samsung.com>");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:exynos-generic-icc");
-- 
2.7.4

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

[Sylwester Nawrocki]

This patch adds a generic interconnect driver for Exynos SoCs in order
to provide interconnect functionality for each "samsung,exynos-bus"
compatible device.

The SoC topology is a graph (or more specifically, a tree) and its
edges are specified using the 'samsung,interconnect-parent' in the
DT. Due to unspecified relative probing order, -EPROBE_DEFER may be
propagated to ensure that the parent is probed before its children.

Each bus is now an interconnect provider and an interconnect node as
well (cf. Documentation/interconnect/interconnect.rst), i.e. every bus
registers itself as a node. Node IDs are not hardcoded but rather
assigned dynamically at runtime. This approach allows for using this
driver with various Exynos SoCs.

Frequencies requested via the interconnect API for a given node are
propagated to devfreq using dev_pm_qos_update_request(). Please note
that it is not an error when CONFIG_INTERCONNECT is 'n', in which
case all interconnect API functions are no-op.

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

Signed-off-by: Artur Świgoń <a.swigon@samsung.com>
Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
---
Changes for v7:
 - adjusted to the DT property changes: "interconnects" instead
   of "samsung,interconnect-parent", "samsung,data-clk-ratio"
   instead of "bus-width",
 - adaptation to of_icc_get_from_provider() function changes
   in v5.10-rc1.

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

Changes for v5:
 - adjust to renamed exynos,interconnect-parent-node property,
 - use automatically generated platform device id as the interconect
   node id instead of a now unavailable devfreq->id field,
 - add icc_ prefix to some variables to make the code more self-commenting,
 - use icc_nodes_remove() instead of icc_node_del() + icc_node_destroy(),
 - adjust to exynos,interconnect-parent-node property rename to
   samsung,interconnect-parent,
 - converted to a separate platform driver in drivers/interconnect.

---
 drivers/interconnect/Kconfig         |   1 +
 drivers/interconnect/Makefile        |   1 +
 drivers/interconnect/exynos/Kconfig  |   6 ++
 drivers/interconnect/exynos/Makefile |   4 +
 drivers/interconnect/exynos/exynos.c | 198 +++++++++++++++++++++++++++++++++++
 5 files changed, 210 insertions(+)
 create mode 100644 drivers/interconnect/exynos/Kconfig
 create mode 100644 drivers/interconnect/exynos/Makefile
 create mode 100644 drivers/interconnect/exynos/exynos.c

diff --git a/drivers/interconnect/Kconfig b/drivers/interconnect/Kconfig
index 5b7204e..eca6eda 100644
--- a/drivers/interconnect/Kconfig
+++ b/drivers/interconnect/Kconfig
@@ -11,6 +11,7 @@ menuconfig INTERCONNECT
 
 if INTERCONNECT
 
+source "drivers/interconnect/exynos/Kconfig"
 source "drivers/interconnect/imx/Kconfig"
 source "drivers/interconnect/qcom/Kconfig"
 
diff --git a/drivers/interconnect/Makefile b/drivers/interconnect/Makefile
index d203520..665538d 100644
--- a/drivers/interconnect/Makefile
+++ b/drivers/interconnect/Makefile
@@ -4,5 +4,6 @@ CFLAGS_core.o				:= -I$(src)
 icc-core-objs				:= core.o bulk.o
 
 obj-$(CONFIG_INTERCONNECT)		+= icc-core.o
+obj-$(CONFIG_INTERCONNECT_EXYNOS)	+= exynos/
 obj-$(CONFIG_INTERCONNECT_IMX)		+= imx/
 obj-$(CONFIG_INTERCONNECT_QCOM)		+= qcom/
diff --git a/drivers/interconnect/exynos/Kconfig b/drivers/interconnect/exynos/Kconfig
new file mode 100644
index 0000000..e51e52e
--- /dev/null
+++ b/drivers/interconnect/exynos/Kconfig
@@ -0,0 +1,6 @@
+# SPDX-License-Identifier: GPL-2.0-only
+config INTERCONNECT_EXYNOS
+	tristate "Exynos generic interconnect driver"
+	depends on ARCH_EXYNOS || COMPILE_TEST
+	help
+	  Generic interconnect driver for Exynos SoCs.
diff --git a/drivers/interconnect/exynos/Makefile b/drivers/interconnect/exynos/Makefile
new file mode 100644
index 0000000..e19d1df
--- /dev/null
+++ b/drivers/interconnect/exynos/Makefile
@@ -0,0 +1,4 @@
+# SPDX-License-Identifier: GPL-2.0
+exynos-interconnect-objs		:= exynos.o
+
+obj-$(CONFIG_INTERCONNECT_EXYNOS)	+= exynos-interconnect.o
diff --git a/drivers/interconnect/exynos/exynos.c b/drivers/interconnect/exynos/exynos.c
new file mode 100644
index 0000000..772d1fc
--- /dev/null
+++ b/drivers/interconnect/exynos/exynos.c
@@ -0,0 +1,198 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Exynos generic interconnect provider driver
+ *
+ * Copyright (c) 2020 Samsung Electronics Co., Ltd.
+ *
+ * Authors: Artur Świgoń <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>
+#include <linux/slab.h>
+
+#define EXYNOS_ICC_DEFAULT_BUS_CLK_RATIO	8
+
+struct exynos_icc_priv {
+	struct device *dev;
+
+	/* One interconnect node per provider */
+	struct icc_provider provider;
+	struct icc_node *node;
+
+	struct dev_pm_qos_request qos_req;
+	u32 bus_clk_ratio;
+};
+
+static struct icc_node *exynos_icc_get_parent(struct device_node *np)
+{
+	struct of_phandle_args args;
+	struct icc_node_data *icc_node_data;
+	struct icc_node *icc_node;
+	int num, ret;
+
+	num = of_count_phandle_with_args(np, "interconnects",
+					 "#interconnect-cells");
+	if (num < 1)
+		return NULL; /* parent nodes are optional */
+
+	/* Get the interconnect target node */
+	ret = of_parse_phandle_with_args(np, "interconnects",
+					"#interconnect-cells", 0, &args);
+	if (ret < 0)
+		return ERR_PTR(ret);
+
+	icc_node_data = of_icc_get_from_provider(&args);
+	of_node_put(args.np);
+
+	if (IS_ERR(icc_node_data))
+		return ERR_CAST(icc_node_data);
+
+	icc_node = icc_node_data->node;
+	kfree(icc_node_data);
+
+	return icc_node;
+}
+
+static int exynos_generic_icc_set(struct icc_node *src, struct icc_node *dst)
+{
+	struct exynos_icc_priv *src_priv = src->data, *dst_priv = dst->data;
+	s32 src_freq = max(src->avg_bw, src->peak_bw) / src_priv->bus_clk_ratio;
+	s32 dst_freq = max(dst->avg_bw, dst->peak_bw) / dst_priv->bus_clk_ratio;
+	int ret;
+
+	ret = dev_pm_qos_update_request(&src_priv->qos_req, src_freq);
+	if (ret < 0) {
+		dev_err(src_priv->dev, "failed to update PM QoS of %s (src)\n",
+			src->name);
+		return ret;
+	}
+
+	ret = dev_pm_qos_update_request(&dst_priv->qos_req, dst_freq);
+	if (ret < 0) {
+		dev_err(dst_priv->dev, "failed to update PM QoS of %s (dst)\n",
+			dst->name);
+		return ret;
+	}
+
+	return 0;
+}
+
+static struct icc_node *exynos_generic_icc_xlate(struct of_phandle_args *spec,
+						 void *data)
+{
+	struct exynos_icc_priv *priv = data;
+
+	if (spec->np != priv->dev->parent->of_node)
+		return ERR_PTR(-EINVAL);
+
+	return priv->node;
+}
+
+static int exynos_generic_icc_remove(struct platform_device *pdev)
+{
+	struct exynos_icc_priv *priv = platform_get_drvdata(pdev);
+	struct icc_node *parent_node, *node = priv->node;
+
+	parent_node = exynos_icc_get_parent(priv->dev->parent->of_node);
+	if (parent_node && !IS_ERR(parent_node))
+		icc_link_destroy(node, parent_node);
+
+	icc_nodes_remove(&priv->provider);
+	icc_provider_del(&priv->provider);
+
+	return 0;
+}
+
+static int exynos_generic_icc_probe(struct platform_device *pdev)
+{
+	struct device *bus_dev = pdev->dev.parent;
+	struct exynos_icc_priv *priv;
+	struct icc_provider *provider;
+	struct icc_node *icc_node, *icc_parent_node;
+	int ret;
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->dev = &pdev->dev;
+	platform_set_drvdata(pdev, priv);
+
+	provider = &priv->provider;
+
+	provider->set = exynos_generic_icc_set;
+	provider->aggregate = icc_std_aggregate;
+	provider->xlate = exynos_generic_icc_xlate;
+	provider->dev = bus_dev;
+	provider->inter_set = true;
+	provider->data = priv;
+
+	ret = icc_provider_add(provider);
+	if (ret < 0)
+		return ret;
+
+	icc_node = icc_node_create(pdev->id);
+	if (IS_ERR(icc_node)) {
+		ret = PTR_ERR(icc_node);
+		goto err_prov_del;
+	}
+
+	priv->node = icc_node;
+	icc_node->name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%pOFn",
+					bus_dev->of_node);
+	if (of_property_read_u32(bus_dev->of_node, "samsung,data-clock-ratio",
+				 &priv->bus_clk_ratio))
+		priv->bus_clk_ratio = EXYNOS_ICC_DEFAULT_BUS_CLK_RATIO;
+
+	/*
+	 * Register a PM QoS request for the parent (devfreq) device.
+	 */
+	ret = dev_pm_qos_add_request(bus_dev, &priv->qos_req,
+				     DEV_PM_QOS_MIN_FREQUENCY, 0);
+	if (ret < 0)
+		goto err_node_del;
+
+	icc_node->data = priv;
+	icc_node_add(icc_node, provider);
+
+	icc_parent_node = exynos_icc_get_parent(bus_dev->of_node);
+	if (IS_ERR(icc_parent_node)) {
+		ret = PTR_ERR(icc_parent_node);
+		goto err_pmqos_del;
+	}
+	if (icc_parent_node) {
+		ret = icc_link_create(icc_node, icc_parent_node->id);
+		if (ret < 0)
+			goto err_pmqos_del;
+	}
+
+	return 0;
+
+err_pmqos_del:
+	dev_pm_qos_remove_request(&priv->qos_req);
+err_node_del:
+	icc_nodes_remove(provider);
+err_prov_del:
+	icc_provider_del(provider);
+	return ret;
+}
+
+static struct platform_driver exynos_generic_icc_driver = {
+	.driver = {
+		.name = "exynos-generic-icc",
+	},
+	.probe = exynos_generic_icc_probe,
+	.remove = exynos_generic_icc_remove,
+};
+module_platform_driver(exynos_generic_icc_driver);
+
+MODULE_DESCRIPTION("Exynos generic interconnect driver");
+MODULE_AUTHOR("Artur Świgoń <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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Krzysztof Kozlowski]

On Fri, Oct 30, 2020 at 01:51:45PM +0100, Sylwester Nawrocki wrote:
> This patch adds a generic interconnect driver for Exynos SoCs in order
> to provide interconnect functionality for each "samsung,exynos-bus"
> compatible device.
> 
> The SoC topology is a graph (or more specifically, a tree) and its
> edges are specified using the 'samsung,interconnect-parent' in the
> DT. Due to unspecified relative probing order, -EPROBE_DEFER may be
> propagated to ensure that the parent is probed before its children.
> 
> Each bus is now an interconnect provider and an interconnect node as
> well (cf. Documentation/interconnect/interconnect.rst), i.e. every bus
> registers itself as a node. Node IDs are not hardcoded but rather
> assigned dynamically at runtime. This approach allows for using this
> driver with various Exynos SoCs.
> 
> Frequencies requested via the interconnect API for a given node are
> propagated to devfreq using dev_pm_qos_update_request(). Please note
> that it is not an error when CONFIG_INTERCONNECT is 'n', in which
> case all interconnect API functions are no-op.
> 
> The bus-width DT property is to determine the interconnect data
> width and traslate requested bandwidth to clock frequency for each
> bus.
> 
> Signed-off-by: Artur Świgoń <a.swigon@samsung.com>
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> ---
> Changes for v7:
>  - adjusted to the DT property changes: "interconnects" instead
>    of "samsung,interconnect-parent", "samsung,data-clk-ratio"
>    instead of "bus-width",
>  - adaptation to of_icc_get_from_provider() function changes
>    in v5.10-rc1.
> 
> Changes for v6:
>  - corrected of_node dereferencing in exynos_icc_get_parent()
>    function,
>  - corrected initialization of icc_node->name so as to avoid
>    direct of_node->name dereferencing,
>  - added parsing of bus-width DT property.
> 
> Changes for v5:
>  - adjust to renamed exynos,interconnect-parent-node property,
>  - use automatically generated platform device id as the interconect
>    node id instead of a now unavailable devfreq->id field,
>  - add icc_ prefix to some variables to make the code more self-commenting,
>  - use icc_nodes_remove() instead of icc_node_del() + icc_node_destroy(),
>  - adjust to exynos,interconnect-parent-node property rename to
>    samsung,interconnect-parent,
>  - converted to a separate platform driver in drivers/interconnect.
> 
> ---
>  drivers/interconnect/Kconfig         |   1 +
>  drivers/interconnect/Makefile        |   1 +
>  drivers/interconnect/exynos/Kconfig  |   6 ++
>  drivers/interconnect/exynos/Makefile |   4 +
>  drivers/interconnect/exynos/exynos.c | 198 +++++++++++++++++++++++++++++++++++

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

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

Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On Fri, Oct 30, 2020 at 01:51:45PM +0100, Sylwester Nawrocki wrote:
> This patch adds a generic interconnect driver for Exynos SoCs in order
> to provide interconnect functionality for each "samsung,exynos-bus"
> compatible device.
> 
> The SoC topology is a graph (or more specifically, a tree) and its
> edges are specified using the 'samsung,interconnect-parent' in the
> DT. Due to unspecified relative probing order, -EPROBE_DEFER may be
> propagated to ensure that the parent is probed before its children.
> 
> Each bus is now an interconnect provider and an interconnect node as
> well (cf. Documentation/interconnect/interconnect.rst), i.e. every bus
> registers itself as a node. Node IDs are not hardcoded but rather
> assigned dynamically at runtime. This approach allows for using this
> driver with various Exynos SoCs.
> 
> Frequencies requested via the interconnect API for a given node are
> propagated to devfreq using dev_pm_qos_update_request(). Please note
> that it is not an error when CONFIG_INTERCONNECT is 'n', in which
> case all interconnect API functions are no-op.
> 
> The bus-width DT property is to determine the interconnect data
> width and traslate requested bandwidth to clock frequency for each
> bus.
> 
> Signed-off-by: Artur Świgoń <a.swigon@samsung.com>
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> ---
> Changes for v7:
>  - adjusted to the DT property changes: "interconnects" instead
>    of "samsung,interconnect-parent", "samsung,data-clk-ratio"
>    instead of "bus-width",
>  - adaptation to of_icc_get_from_provider() function changes
>    in v5.10-rc1.
> 
> Changes for v6:
>  - corrected of_node dereferencing in exynos_icc_get_parent()
>    function,
>  - corrected initialization of icc_node->name so as to avoid
>    direct of_node->name dereferencing,
>  - added parsing of bus-width DT property.
> 
> Changes for v5:
>  - adjust to renamed exynos,interconnect-parent-node property,
>  - use automatically generated platform device id as the interconect
>    node id instead of a now unavailable devfreq->id field,
>  - add icc_ prefix to some variables to make the code more self-commenting,
>  - use icc_nodes_remove() instead of icc_node_del() + icc_node_destroy(),
>  - adjust to exynos,interconnect-parent-node property rename to
>    samsung,interconnect-parent,
>  - converted to a separate platform driver in drivers/interconnect.
> 
> ---
>  drivers/interconnect/Kconfig         |   1 +
>  drivers/interconnect/Makefile        |   1 +
>  drivers/interconnect/exynos/Kconfig  |   6 ++
>  drivers/interconnect/exynos/Makefile |   4 +
>  drivers/interconnect/exynos/exynos.c | 198 +++++++++++++++++++++++++++++++++++

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

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

Best regards,
Krzysztof


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Sylwester Nawrocki]

On 31.10.2020 13:17, Krzysztof Kozlowski wrote:
> On Fri, Oct 30, 2020 at 01:51:45PM +0100, Sylwester Nawrocki wrote:
>> This patch adds a generic interconnect driver for Exynos SoCs in order
>> to provide interconnect functionality for each "samsung,exynos-bus"
>> compatible device.

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

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

Sure, I will rename the directory.

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

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

-- 
Regards,
Sylwester

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

[Sylwester Nawrocki]

On 31.10.2020 13:17, Krzysztof Kozlowski wrote:
> On Fri, Oct 30, 2020 at 01:51:45PM +0100, Sylwester Nawrocki wrote:
>> This patch adds a generic interconnect driver for Exynos SoCs in order
>> to provide interconnect functionality for each "samsung,exynos-bus"
>> compatible device.

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

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

Sure, I will rename the directory.

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

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

-- 
Regards,
Sylwester
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Georgi Djakov]

Hi Sylwester,

Thank you for refreshing the patchset!

On 10/30/20 14:51, Sylwester Nawrocki wrote:
> This patch adds a generic interconnect driver for Exynos SoCs in order
> to provide interconnect functionality for each "samsung,exynos-bus"
> compatible device.
> 
> The SoC topology is a graph (or more specifically, a tree) and its
> edges are specified using the 'samsung,interconnect-parent' in the
> DT. Due to unspecified relative probing order, -EPROBE_DEFER may be
> propagated to ensure that the parent is probed before its children.
> 
> Each bus is now an interconnect provider and an interconnect node as
> well (cf. Documentation/interconnect/interconnect.rst), i.e. every bus
> registers itself as a node. Node IDs are not hardcoded but rather
> assigned dynamically at runtime. This approach allows for using this
> driver with various Exynos SoCs.
> 
> Frequencies requested via the interconnect API for a given node are
> propagated to devfreq using dev_pm_qos_update_request(). Please note
> that it is not an error when CONFIG_INTERCONNECT is 'n', in which
> case all interconnect API functions are no-op.
> 
> The bus-width DT property is to determine the interconnect data
> width and traslate requested bandwidth to clock frequency for each
> bus.
> 
> Signed-off-by: Artur Świgoń <a.swigon@samsung.com>
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> ---
> Changes for v7:
>  - adjusted to the DT property changes: "interconnects" instead
>    of "samsung,interconnect-parent", "samsung,data-clk-ratio"
>    instead of "bus-width",
>  - adaptation to of_icc_get_from_provider() function changes
>    in v5.10-rc1.
> 
> Changes for v6:
>  - corrected of_node dereferencing in exynos_icc_get_parent()
>    function,
>  - corrected initialization of icc_node->name so as to avoid
>    direct of_node->name dereferencing,
>  - added parsing of bus-width DT property.
> 
> Changes for v5:
>  - adjust to renamed exynos,interconnect-parent-node property,
>  - use automatically generated platform device id as the interconect
>    node id instead of a now unavailable devfreq->id field,
>  - add icc_ prefix to some variables to make the code more self-commenting,
>  - use icc_nodes_remove() instead of icc_node_del() + icc_node_destroy(),
>  - adjust to exynos,interconnect-parent-node property rename to
>    samsung,interconnect-parent,
>  - converted to a separate platform driver in drivers/interconnect.
> 
> ---
>  drivers/interconnect/Kconfig         |   1 +
>  drivers/interconnect/Makefile        |   1 +
>  drivers/interconnect/exynos/Kconfig  |   6 ++
>  drivers/interconnect/exynos/Makefile |   4 +
>  drivers/interconnect/exynos/exynos.c | 198 +++++++++++++++++++++++++++++++++++
>  5 files changed, 210 insertions(+)
>  create mode 100644 drivers/interconnect/exynos/Kconfig
>  create mode 100644 drivers/interconnect/exynos/Makefile
>  create mode 100644 drivers/interconnect/exynos/exynos.c
> 
[..]
> +
> +static struct platform_driver exynos_generic_icc_driver = {
> +	.driver = {
> +		.name = "exynos-generic-icc",

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

Thanks,
Georgi

> +	},
> +	.probe = exynos_generic_icc_probe,
> +	.remove = exynos_generic_icc_remove,
> +};
> +module_platform_driver(exynos_generic_icc_driver);
> +
> +MODULE_DESCRIPTION("Exynos generic interconnect driver");
> +MODULE_AUTHOR("Artur Świgoń <a.swigon@samsung.com>");
> +MODULE_AUTHOR("Sylwester Nawrocki <s.nawrocki@samsung.com>");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:exynos-generic-icc");
> 

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

[Georgi Djakov]

Hi Sylwester,

Thank you for refreshing the patchset!

On 10/30/20 14:51, Sylwester Nawrocki wrote:
> This patch adds a generic interconnect driver for Exynos SoCs in order
> to provide interconnect functionality for each "samsung,exynos-bus"
> compatible device.
> 
> The SoC topology is a graph (or more specifically, a tree) and its
> edges are specified using the 'samsung,interconnect-parent' in the
> DT. Due to unspecified relative probing order, -EPROBE_DEFER may be
> propagated to ensure that the parent is probed before its children.
> 
> Each bus is now an interconnect provider and an interconnect node as
> well (cf. Documentation/interconnect/interconnect.rst), i.e. every bus
> registers itself as a node. Node IDs are not hardcoded but rather
> assigned dynamically at runtime. This approach allows for using this
> driver with various Exynos SoCs.
> 
> Frequencies requested via the interconnect API for a given node are
> propagated to devfreq using dev_pm_qos_update_request(). Please note
> that it is not an error when CONFIG_INTERCONNECT is 'n', in which
> case all interconnect API functions are no-op.
> 
> The bus-width DT property is to determine the interconnect data
> width and traslate requested bandwidth to clock frequency for each
> bus.
> 
> Signed-off-by: Artur Świgoń <a.swigon@samsung.com>
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> ---
> Changes for v7:
>  - adjusted to the DT property changes: "interconnects" instead
>    of "samsung,interconnect-parent", "samsung,data-clk-ratio"
>    instead of "bus-width",
>  - adaptation to of_icc_get_from_provider() function changes
>    in v5.10-rc1.
> 
> Changes for v6:
>  - corrected of_node dereferencing in exynos_icc_get_parent()
>    function,
>  - corrected initialization of icc_node->name so as to avoid
>    direct of_node->name dereferencing,
>  - added parsing of bus-width DT property.
> 
> Changes for v5:
>  - adjust to renamed exynos,interconnect-parent-node property,
>  - use automatically generated platform device id as the interconect
>    node id instead of a now unavailable devfreq->id field,
>  - add icc_ prefix to some variables to make the code more self-commenting,
>  - use icc_nodes_remove() instead of icc_node_del() + icc_node_destroy(),
>  - adjust to exynos,interconnect-parent-node property rename to
>    samsung,interconnect-parent,
>  - converted to a separate platform driver in drivers/interconnect.
> 
> ---
>  drivers/interconnect/Kconfig         |   1 +
>  drivers/interconnect/Makefile        |   1 +
>  drivers/interconnect/exynos/Kconfig  |   6 ++
>  drivers/interconnect/exynos/Makefile |   4 +
>  drivers/interconnect/exynos/exynos.c | 198 +++++++++++++++++++++++++++++++++++
>  5 files changed, 210 insertions(+)
>  create mode 100644 drivers/interconnect/exynos/Kconfig
>  create mode 100644 drivers/interconnect/exynos/Makefile
>  create mode 100644 drivers/interconnect/exynos/exynos.c
> 
[..]
> +
> +static struct platform_driver exynos_generic_icc_driver = {
> +	.driver = {
> +		.name = "exynos-generic-icc",

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

Thanks,
Georgi

> +	},
> +	.probe = exynos_generic_icc_probe,
> +	.remove = exynos_generic_icc_remove,
> +};
> +module_platform_driver(exynos_generic_icc_driver);
> +
> +MODULE_DESCRIPTION("Exynos generic interconnect driver");
> +MODULE_AUTHOR("Artur Świgoń <a.swigon@samsung.com>");
> +MODULE_AUTHOR("Sylwester Nawrocki <s.nawrocki@samsung.com>");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:exynos-generic-icc");
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Chanwoo Choi]

Hi Sylwester,

On 10/30/20 9:51 PM, Sylwester Nawrocki wrote:
> This patch adds a generic interconnect driver for Exynos SoCs in order
> to provide interconnect functionality for each "samsung,exynos-bus"
> compatible device.
> 
> The SoC topology is a graph (or more specifically, a tree) and its
> edges are specified using the 'samsung,interconnect-parent' in the

samsung,interconnect-parent -> interconnects?


> DT. Due to unspecified relative probing order, -EPROBE_DEFER may be
> propagated to ensure that the parent is probed before its children.
> 
> Each bus is now an interconnect provider and an interconnect node as
> well (cf. Documentation/interconnect/interconnect.rst), i.e. every bus
> registers itself as a node. Node IDs are not hardcoded but rather
> assigned dynamically at runtime. This approach allows for using this
> driver with various Exynos SoCs.
> 
> Frequencies requested via the interconnect API for a given node are
> propagated to devfreq using dev_pm_qos_update_request(). Please note
> that it is not an error when CONFIG_INTERCONNECT is 'n', in which
> case all interconnect API functions are no-op.
> 
> The bus-width DT property is to determine the interconnect data
> width and traslate requested bandwidth to clock frequency for each
> bus.
> 
> Signed-off-by: Artur Świgoń <a.swigon@samsung.com>
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> ---
> Changes for v7:
>  - adjusted to the DT property changes: "interconnects" instead
>    of "samsung,interconnect-parent", "samsung,data-clk-ratio"
>    instead of "bus-width",
>  - adaptation to of_icc_get_from_provider() function changes
>    in v5.10-rc1.
> 
> Changes for v6:
>  - corrected of_node dereferencing in exynos_icc_get_parent()
>    function,
>  - corrected initialization of icc_node->name so as to avoid
>    direct of_node->name dereferencing,
>  - added parsing of bus-width DT property.
> 
> Changes for v5:
>  - adjust to renamed exynos,interconnect-parent-node property,
>  - use automatically generated platform device id as the interconect
>    node id instead of a now unavailable devfreq->id field,
>  - add icc_ prefix to some variables to make the code more self-commenting,
>  - use icc_nodes_remove() instead of icc_node_del() + icc_node_destroy(),
>  - adjust to exynos,interconnect-parent-node property rename to
>    samsung,interconnect-parent,
>  - converted to a separate platform driver in drivers/interconnect.
> 
> ---
>  drivers/interconnect/Kconfig         |   1 +
>  drivers/interconnect/Makefile        |   1 +
>  drivers/interconnect/exynos/Kconfig  |   6 ++
>  drivers/interconnect/exynos/Makefile |   4 +
>  drivers/interconnect/exynos/exynos.c | 198 +++++++++++++++++++++++++++++++++++
>  5 files changed, 210 insertions(+)
>  create mode 100644 drivers/interconnect/exynos/Kconfig
>  create mode 100644 drivers/interconnect/exynos/Makefile
>  create mode 100644 drivers/interconnect/exynos/exynos.c
> 
> diff --git a/drivers/interconnect/Kconfig b/drivers/interconnect/Kconfig
> index 5b7204e..eca6eda 100644
> --- a/drivers/interconnect/Kconfig
> +++ b/drivers/interconnect/Kconfig
> @@ -11,6 +11,7 @@ menuconfig INTERCONNECT
>  
>  if INTERCONNECT
>  
> +source "drivers/interconnect/exynos/Kconfig"
>  source "drivers/interconnect/imx/Kconfig"
>  source "drivers/interconnect/qcom/Kconfig"
>  
> diff --git a/drivers/interconnect/Makefile b/drivers/interconnect/Makefile
> index d203520..665538d 100644
> --- a/drivers/interconnect/Makefile
> +++ b/drivers/interconnect/Makefile
> @@ -4,5 +4,6 @@ CFLAGS_core.o				:= -I$(src)
>  icc-core-objs				:= core.o bulk.o
>  
>  obj-$(CONFIG_INTERCONNECT)		+= icc-core.o
> +obj-$(CONFIG_INTERCONNECT_EXYNOS)	+= exynos/
>  obj-$(CONFIG_INTERCONNECT_IMX)		+= imx/
>  obj-$(CONFIG_INTERCONNECT_QCOM)		+= qcom/
> diff --git a/drivers/interconnect/exynos/Kconfig b/drivers/interconnect/exynos/Kconfig
> new file mode 100644
> index 0000000..e51e52e
> --- /dev/null
> +++ b/drivers/interconnect/exynos/Kconfig
> @@ -0,0 +1,6 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +config INTERCONNECT_EXYNOS
> +	tristate "Exynos generic interconnect driver"
> +	depends on ARCH_EXYNOS || COMPILE_TEST
> +	help
> +	  Generic interconnect driver for Exynos SoCs.
> diff --git a/drivers/interconnect/exynos/Makefile b/drivers/interconnect/exynos/Makefile
> new file mode 100644
> index 0000000..e19d1df
> --- /dev/null
> +++ b/drivers/interconnect/exynos/Makefile
> @@ -0,0 +1,4 @@
> +# SPDX-License-Identifier: GPL-2.0
> +exynos-interconnect-objs		:= exynos.o
> +
> +obj-$(CONFIG_INTERCONNECT_EXYNOS)	+= exynos-interconnect.o
> diff --git a/drivers/interconnect/exynos/exynos.c b/drivers/interconnect/exynos/exynos.c
> new file mode 100644
> index 0000000..772d1fc
> --- /dev/null
> +++ b/drivers/interconnect/exynos/exynos.c
> @@ -0,0 +1,198 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Exynos generic interconnect provider driver
> + *
> + * Copyright (c) 2020 Samsung Electronics Co., Ltd.
> + *
> + * Authors: Artur Świgoń <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>
> +#include <linux/slab.h>
> +
> +#define EXYNOS_ICC_DEFAULT_BUS_CLK_RATIO	8
> +
> +struct exynos_icc_priv {
> +	struct device *dev;
> +
> +	/* One interconnect node per provider */
> +	struct icc_provider provider;
> +	struct icc_node *node;
> +
> +	struct dev_pm_qos_request qos_req;
> +	u32 bus_clk_ratio;
> +};
> +
> +static struct icc_node *exynos_icc_get_parent(struct device_node *np)
> +{
> +	struct of_phandle_args args;
> +	struct icc_node_data *icc_node_data;
> +	struct icc_node *icc_node;
> +	int num, ret;
> +
> +	num = of_count_phandle_with_args(np, "interconnects",
> +					 "#interconnect-cells");
> +	if (num < 1)
> +		return NULL; /* parent nodes are optional */
> +
> +	/* Get the interconnect target node */
> +	ret = of_parse_phandle_with_args(np, "interconnects",
> +					"#interconnect-cells", 0, &args);
> +	if (ret < 0)
> +		return ERR_PTR(ret);
> +
> +	icc_node_data = of_icc_get_from_provider(&args);
> +	of_node_put(args.np);
> +
> +	if (IS_ERR(icc_node_data))
> +		return ERR_CAST(icc_node_data);
> +
> +	icc_node = icc_node_data->node;
> +	kfree(icc_node_data);
> +
> +	return icc_node;
> +}

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

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

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


> +
> +static int exynos_generic_icc_set(struct icc_node *src, struct icc_node *dst)
> +{
> +	struct exynos_icc_priv *src_priv = src->data, *dst_priv = dst->data;
> +	s32 src_freq = max(src->avg_bw, src->peak_bw) / src_priv->bus_clk_ratio;
> +	s32 dst_freq = max(dst->avg_bw, dst->peak_bw) / dst_priv->bus_clk_ratio;
> +	int ret;
> +
> +	ret = dev_pm_qos_update_request(&src_priv->qos_req, src_freq);
> +	if (ret < 0) {
> +		dev_err(src_priv->dev, "failed to update PM QoS of %s (src)\n",
> +			src->name);
> +		return ret;
> +	}
> +
> +	ret = dev_pm_qos_update_request(&dst_priv->qos_req, dst_freq);
> +	if (ret < 0) {
> +		dev_err(dst_priv->dev, "failed to update PM QoS of %s (dst)\n",
> +			dst->name);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static struct icc_node *exynos_generic_icc_xlate(struct of_phandle_args *spec,
> +						 void *data)
> +{
> +	struct exynos_icc_priv *priv = data;
> +
> +	if (spec->np != priv->dev->parent->of_node)
> +		return ERR_PTR(-EINVAL);
> +
> +	return priv->node;
> +}
> +
> +static int exynos_generic_icc_remove(struct platform_device *pdev)
> +{
> +	struct exynos_icc_priv *priv = platform_get_drvdata(pdev);
> +	struct icc_node *parent_node, *node = priv->node;
> +
> +	parent_node = exynos_icc_get_parent(priv->dev->parent->of_node);
> +	if (parent_node && !IS_ERR(parent_node))
> +		icc_link_destroy(node, parent_node);
> +
> +	icc_nodes_remove(&priv->provider);
> +	icc_provider_del(&priv->provider);
> +
> +	return 0;
> +}
> +
> +static int exynos_generic_icc_probe(struct platform_device *pdev)
> +{
> +	struct device *bus_dev = pdev->dev.parent;
> +	struct exynos_icc_priv *priv;
> +	struct icc_provider *provider;
> +	struct icc_node *icc_node, *icc_parent_node;
> +	int ret;
> +
> +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->dev = &pdev->dev;
> +	platform_set_drvdata(pdev, priv);
> +
> +	provider = &priv->provider;
> +
> +	provider->set = exynos_generic_icc_set;
> +	provider->aggregate = icc_std_aggregate;
> +	provider->xlate = exynos_generic_icc_xlate;
> +	provider->dev = bus_dev;
> +	provider->inter_set = true;
> +	provider->data = priv;
> +
> +	ret = icc_provider_add(provider);
> +	if (ret < 0)
> +		return ret;
> +
> +	icc_node = icc_node_create(pdev->id);
> +	if (IS_ERR(icc_node)) {
> +		ret = PTR_ERR(icc_node);
> +		goto err_prov_del;
> +	}
> +
> +	priv->node = icc_node;
> +	icc_node->name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%pOFn",
> +					bus_dev->of_node);
> +	if (of_property_read_u32(bus_dev->of_node, "samsung,data-clock-ratio",
> +				 &priv->bus_clk_ratio))
> +		priv->bus_clk_ratio = EXYNOS_ICC_DEFAULT_BUS_CLK_RATIO;
> +
> +	/*
> +	 * Register a PM QoS request for the parent (devfreq) device.
> +	 */
> +	ret = dev_pm_qos_add_request(bus_dev, &priv->qos_req,
> +				     DEV_PM_QOS_MIN_FREQUENCY, 0);
> +	if (ret < 0)
> +		goto err_node_del;
> +
> +	icc_node->data = priv;
> +	icc_node_add(icc_node, provider);
> +
> +	icc_parent_node = exynos_icc_get_parent(bus_dev->of_node);
> +	if (IS_ERR(icc_parent_node)) {
> +		ret = PTR_ERR(icc_parent_node);
> +		goto err_pmqos_del;
> +	}
> +	if (icc_parent_node) {
> +		ret = icc_link_create(icc_node, icc_parent_node->id);
> +		if (ret < 0)
> +			goto err_pmqos_del;
> +	}
> +
> +	return 0;
> +
> +err_pmqos_del:
> +	dev_pm_qos_remove_request(&priv->qos_req);
> +err_node_del:
> +	icc_nodes_remove(provider);
> +err_prov_del:
> +	icc_provider_del(provider);
> +	return ret;
> +}
> +
> +static struct platform_driver exynos_generic_icc_driver = {
> +	.driver = {
> +		.name = "exynos-generic-icc",
> +	},
> +	.probe = exynos_generic_icc_probe,
> +	.remove = exynos_generic_icc_remove,
> +};
> +module_platform_driver(exynos_generic_icc_driver);
> +
> +MODULE_DESCRIPTION("Exynos generic interconnect driver");
> +MODULE_AUTHOR("Artur Świgoń <a.swigon@samsung.com>");
> +MODULE_AUTHOR("Sylwester Nawrocki <s.nawrocki@samsung.com>");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:exynos-generic-icc");
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

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

[Chanwoo Choi]

Hi Sylwester,

On 10/30/20 9:51 PM, Sylwester Nawrocki wrote:
> This patch adds a generic interconnect driver for Exynos SoCs in order
> to provide interconnect functionality for each "samsung,exynos-bus"
> compatible device.
> 
> The SoC topology is a graph (or more specifically, a tree) and its
> edges are specified using the 'samsung,interconnect-parent' in the

samsung,interconnect-parent -> interconnects?


> DT. Due to unspecified relative probing order, -EPROBE_DEFER may be
> propagated to ensure that the parent is probed before its children.
> 
> Each bus is now an interconnect provider and an interconnect node as
> well (cf. Documentation/interconnect/interconnect.rst), i.e. every bus
> registers itself as a node. Node IDs are not hardcoded but rather
> assigned dynamically at runtime. This approach allows for using this
> driver with various Exynos SoCs.
> 
> Frequencies requested via the interconnect API for a given node are
> propagated to devfreq using dev_pm_qos_update_request(). Please note
> that it is not an error when CONFIG_INTERCONNECT is 'n', in which
> case all interconnect API functions are no-op.
> 
> The bus-width DT property is to determine the interconnect data
> width and traslate requested bandwidth to clock frequency for each
> bus.
> 
> Signed-off-by: Artur Świgoń <a.swigon@samsung.com>
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> ---
> Changes for v7:
>  - adjusted to the DT property changes: "interconnects" instead
>    of "samsung,interconnect-parent", "samsung,data-clk-ratio"
>    instead of "bus-width",
>  - adaptation to of_icc_get_from_provider() function changes
>    in v5.10-rc1.
> 
> Changes for v6:
>  - corrected of_node dereferencing in exynos_icc_get_parent()
>    function,
>  - corrected initialization of icc_node->name so as to avoid
>    direct of_node->name dereferencing,
>  - added parsing of bus-width DT property.
> 
> Changes for v5:
>  - adjust to renamed exynos,interconnect-parent-node property,
>  - use automatically generated platform device id as the interconect
>    node id instead of a now unavailable devfreq->id field,
>  - add icc_ prefix to some variables to make the code more self-commenting,
>  - use icc_nodes_remove() instead of icc_node_del() + icc_node_destroy(),
>  - adjust to exynos,interconnect-parent-node property rename to
>    samsung,interconnect-parent,
>  - converted to a separate platform driver in drivers/interconnect.
> 
> ---
>  drivers/interconnect/Kconfig         |   1 +
>  drivers/interconnect/Makefile        |   1 +
>  drivers/interconnect/exynos/Kconfig  |   6 ++
>  drivers/interconnect/exynos/Makefile |   4 +
>  drivers/interconnect/exynos/exynos.c | 198 +++++++++++++++++++++++++++++++++++
>  5 files changed, 210 insertions(+)
>  create mode 100644 drivers/interconnect/exynos/Kconfig
>  create mode 100644 drivers/interconnect/exynos/Makefile
>  create mode 100644 drivers/interconnect/exynos/exynos.c
> 
> diff --git a/drivers/interconnect/Kconfig b/drivers/interconnect/Kconfig
> index 5b7204e..eca6eda 100644
> --- a/drivers/interconnect/Kconfig
> +++ b/drivers/interconnect/Kconfig
> @@ -11,6 +11,7 @@ menuconfig INTERCONNECT
>  
>  if INTERCONNECT
>  
> +source "drivers/interconnect/exynos/Kconfig"
>  source "drivers/interconnect/imx/Kconfig"
>  source "drivers/interconnect/qcom/Kconfig"
>  
> diff --git a/drivers/interconnect/Makefile b/drivers/interconnect/Makefile
> index d203520..665538d 100644
> --- a/drivers/interconnect/Makefile
> +++ b/drivers/interconnect/Makefile
> @@ -4,5 +4,6 @@ CFLAGS_core.o				:= -I$(src)
>  icc-core-objs				:= core.o bulk.o
>  
>  obj-$(CONFIG_INTERCONNECT)		+= icc-core.o
> +obj-$(CONFIG_INTERCONNECT_EXYNOS)	+= exynos/
>  obj-$(CONFIG_INTERCONNECT_IMX)		+= imx/
>  obj-$(CONFIG_INTERCONNECT_QCOM)		+= qcom/
> diff --git a/drivers/interconnect/exynos/Kconfig b/drivers/interconnect/exynos/Kconfig
> new file mode 100644
> index 0000000..e51e52e
> --- /dev/null
> +++ b/drivers/interconnect/exynos/Kconfig
> @@ -0,0 +1,6 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +config INTERCONNECT_EXYNOS
> +	tristate "Exynos generic interconnect driver"
> +	depends on ARCH_EXYNOS || COMPILE_TEST
> +	help
> +	  Generic interconnect driver for Exynos SoCs.
> diff --git a/drivers/interconnect/exynos/Makefile b/drivers/interconnect/exynos/Makefile
> new file mode 100644
> index 0000000..e19d1df
> --- /dev/null
> +++ b/drivers/interconnect/exynos/Makefile
> @@ -0,0 +1,4 @@
> +# SPDX-License-Identifier: GPL-2.0
> +exynos-interconnect-objs		:= exynos.o
> +
> +obj-$(CONFIG_INTERCONNECT_EXYNOS)	+= exynos-interconnect.o
> diff --git a/drivers/interconnect/exynos/exynos.c b/drivers/interconnect/exynos/exynos.c
> new file mode 100644
> index 0000000..772d1fc
> --- /dev/null
> +++ b/drivers/interconnect/exynos/exynos.c
> @@ -0,0 +1,198 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Exynos generic interconnect provider driver
> + *
> + * Copyright (c) 2020 Samsung Electronics Co., Ltd.
> + *
> + * Authors: Artur Świgoń <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>
> +#include <linux/slab.h>
> +
> +#define EXYNOS_ICC_DEFAULT_BUS_CLK_RATIO	8
> +
> +struct exynos_icc_priv {
> +	struct device *dev;
> +
> +	/* One interconnect node per provider */
> +	struct icc_provider provider;
> +	struct icc_node *node;
> +
> +	struct dev_pm_qos_request qos_req;
> +	u32 bus_clk_ratio;
> +};
> +
> +static struct icc_node *exynos_icc_get_parent(struct device_node *np)
> +{
> +	struct of_phandle_args args;
> +	struct icc_node_data *icc_node_data;
> +	struct icc_node *icc_node;
> +	int num, ret;
> +
> +	num = of_count_phandle_with_args(np, "interconnects",
> +					 "#interconnect-cells");
> +	if (num < 1)
> +		return NULL; /* parent nodes are optional */
> +
> +	/* Get the interconnect target node */
> +	ret = of_parse_phandle_with_args(np, "interconnects",
> +					"#interconnect-cells", 0, &args);
> +	if (ret < 0)
> +		return ERR_PTR(ret);
> +
> +	icc_node_data = of_icc_get_from_provider(&args);
> +	of_node_put(args.np);
> +
> +	if (IS_ERR(icc_node_data))
> +		return ERR_CAST(icc_node_data);
> +
> +	icc_node = icc_node_data->node;
> +	kfree(icc_node_data);
> +
> +	return icc_node;
> +}

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

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

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


> +
> +static int exynos_generic_icc_set(struct icc_node *src, struct icc_node *dst)
> +{
> +	struct exynos_icc_priv *src_priv = src->data, *dst_priv = dst->data;
> +	s32 src_freq = max(src->avg_bw, src->peak_bw) / src_priv->bus_clk_ratio;
> +	s32 dst_freq = max(dst->avg_bw, dst->peak_bw) / dst_priv->bus_clk_ratio;
> +	int ret;
> +
> +	ret = dev_pm_qos_update_request(&src_priv->qos_req, src_freq);
> +	if (ret < 0) {
> +		dev_err(src_priv->dev, "failed to update PM QoS of %s (src)\n",
> +			src->name);
> +		return ret;
> +	}
> +
> +	ret = dev_pm_qos_update_request(&dst_priv->qos_req, dst_freq);
> +	if (ret < 0) {
> +		dev_err(dst_priv->dev, "failed to update PM QoS of %s (dst)\n",
> +			dst->name);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static struct icc_node *exynos_generic_icc_xlate(struct of_phandle_args *spec,
> +						 void *data)
> +{
> +	struct exynos_icc_priv *priv = data;
> +
> +	if (spec->np != priv->dev->parent->of_node)
> +		return ERR_PTR(-EINVAL);
> +
> +	return priv->node;
> +}
> +
> +static int exynos_generic_icc_remove(struct platform_device *pdev)
> +{
> +	struct exynos_icc_priv *priv = platform_get_drvdata(pdev);
> +	struct icc_node *parent_node, *node = priv->node;
> +
> +	parent_node = exynos_icc_get_parent(priv->dev->parent->of_node);
> +	if (parent_node && !IS_ERR(parent_node))
> +		icc_link_destroy(node, parent_node);
> +
> +	icc_nodes_remove(&priv->provider);
> +	icc_provider_del(&priv->provider);
> +
> +	return 0;
> +}
> +
> +static int exynos_generic_icc_probe(struct platform_device *pdev)
> +{
> +	struct device *bus_dev = pdev->dev.parent;
> +	struct exynos_icc_priv *priv;
> +	struct icc_provider *provider;
> +	struct icc_node *icc_node, *icc_parent_node;
> +	int ret;
> +
> +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->dev = &pdev->dev;
> +	platform_set_drvdata(pdev, priv);
> +
> +	provider = &priv->provider;
> +
> +	provider->set = exynos_generic_icc_set;
> +	provider->aggregate = icc_std_aggregate;
> +	provider->xlate = exynos_generic_icc_xlate;
> +	provider->dev = bus_dev;
> +	provider->inter_set = true;
> +	provider->data = priv;
> +
> +	ret = icc_provider_add(provider);
> +	if (ret < 0)
> +		return ret;
> +
> +	icc_node = icc_node_create(pdev->id);
> +	if (IS_ERR(icc_node)) {
> +		ret = PTR_ERR(icc_node);
> +		goto err_prov_del;
> +	}
> +
> +	priv->node = icc_node;
> +	icc_node->name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%pOFn",
> +					bus_dev->of_node);
> +	if (of_property_read_u32(bus_dev->of_node, "samsung,data-clock-ratio",
> +				 &priv->bus_clk_ratio))
> +		priv->bus_clk_ratio = EXYNOS_ICC_DEFAULT_BUS_CLK_RATIO;
> +
> +	/*
> +	 * Register a PM QoS request for the parent (devfreq) device.
> +	 */
> +	ret = dev_pm_qos_add_request(bus_dev, &priv->qos_req,
> +				     DEV_PM_QOS_MIN_FREQUENCY, 0);
> +	if (ret < 0)
> +		goto err_node_del;
> +
> +	icc_node->data = priv;
> +	icc_node_add(icc_node, provider);
> +
> +	icc_parent_node = exynos_icc_get_parent(bus_dev->of_node);
> +	if (IS_ERR(icc_parent_node)) {
> +		ret = PTR_ERR(icc_parent_node);
> +		goto err_pmqos_del;
> +	}
> +	if (icc_parent_node) {
> +		ret = icc_link_create(icc_node, icc_parent_node->id);
> +		if (ret < 0)
> +			goto err_pmqos_del;
> +	}
> +
> +	return 0;
> +
> +err_pmqos_del:
> +	dev_pm_qos_remove_request(&priv->qos_req);
> +err_node_del:
> +	icc_nodes_remove(provider);
> +err_prov_del:
> +	icc_provider_del(provider);
> +	return ret;
> +}
> +
> +static struct platform_driver exynos_generic_icc_driver = {
> +	.driver = {
> +		.name = "exynos-generic-icc",
> +	},
> +	.probe = exynos_generic_icc_probe,
> +	.remove = exynos_generic_icc_remove,
> +};
> +module_platform_driver(exynos_generic_icc_driver);
> +
> +MODULE_DESCRIPTION("Exynos generic interconnect driver");
> +MODULE_AUTHOR("Artur Świgoń <a.swigon@samsung.com>");
> +MODULE_AUTHOR("Sylwester Nawrocki <s.nawrocki@samsung.com>");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:exynos-generic-icc");
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Sylwester Nawrocki]

On 03.11.2020 10:37, Chanwoo Choi wrote:
> On 10/30/20 9:51 PM, Sylwester Nawrocki wrote:
>> This patch adds a generic interconnect driver for Exynos SoCs in order
>> to provide interconnect functionality for each "samsung,exynos-bus"
>> compatible device.
>>
>> The SoC topology is a graph (or more specifically, a tree) and its
>> edges are specified using the 'samsung,interconnect-parent' in the
> 
> samsung,interconnect-parent -> interconnects?

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

I've changed the sentence to:
"The SoC topology is a graph (or more specifically, a tree) and its
edges are described by specifying in the 'interconnects' property
the interconnect consumer path for each interconnect provider DT node."

>> DT. Due to unspecified relative probing order, -EPROBE_DEFER may be
>> propagated to ensure that the parent is probed before its children.
>>
>> Each bus is now an interconnect provider and an interconnect node as
>> well (cf. Documentation/interconnect/interconnect.rst), i.e. every bus
>> registers itself as a node. Node IDs are not hardcoded but rather
>> assigned dynamically at runtime. This approach allows for using this
>> driver with various Exynos SoCs.
>>
>> Frequencies requested via the interconnect API for a given node are
>> propagated to devfreq using dev_pm_qos_update_request(). Please note
>> that it is not an error when CONFIG_INTERCONNECT is 'n', in which
>> case all interconnect API functions are no-op.
>>
>> The bus-width DT property is to determine the interconnect data
>> width and traslate requested bandwidth to clock frequency for each
>> bus.
>>
>> Signed-off-by: Artur Świgoń <a.swigon@samsung.com>
>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>

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

>> +struct exynos_icc_priv {
>> +	struct device *dev;
>> +
>> +	/* One interconnect node per provider */
>> +	struct icc_provider provider;
>> +	struct icc_node *node;
>> +
>> +	struct dev_pm_qos_request qos_req;
>> +	u32 bus_clk_ratio;
>> +};
>> +
>> +static struct icc_node *exynos_icc_get_parent(struct device_node *np)
>> +{
>> +	struct of_phandle_args args;
>> +	struct icc_node_data *icc_node_data;
>> +	struct icc_node *icc_node;
>> +	int num, ret;
>> +
>> +	num = of_count_phandle_with_args(np, "interconnects",
>> +					 "#interconnect-cells");
>> +	if (num < 1)
>> +		return NULL; /* parent nodes are optional */
>> +
>> +	/* Get the interconnect target node */
>> +	ret = of_parse_phandle_with_args(np, "interconnects",
>> +					"#interconnect-cells", 0, &args);
>> +	if (ret < 0)
>> +		return ERR_PTR(ret);
>> +
>> +	icc_node_data = of_icc_get_from_provider(&args);
>> +	of_node_put(args.np);
>> +
>> +	if (IS_ERR(icc_node_data))
>> +		return ERR_CAST(icc_node_data);
>> +
>> +	icc_node = icc_node_data->node;
>> +	kfree(icc_node_data);
>> +
>> +	return icc_node;
>> +}
> 
> I have a question about exynos_icc_get_parent().
> As I checked, this function returns the only one icc_node
> as parent node. But, bus_display dt node in the exynos4412.dtsi
> specifies the two interconnect node as following with bus_leftbus, bus_dmc,
> 
> When I checked the return value of exynos_icc_get_parent()
> during probing for bus_display device, exynos_icc_get_parent() function
> only returns 'bus_leftbus' icc_node. Do you need to add two phandle
> of icc node?

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

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

-- 
Regards,
Sylwester

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

[Sylwester Nawrocki]

On 03.11.2020 10:37, Chanwoo Choi wrote:
> On 10/30/20 9:51 PM, Sylwester Nawrocki wrote:
>> This patch adds a generic interconnect driver for Exynos SoCs in order
>> to provide interconnect functionality for each "samsung,exynos-bus"
>> compatible device.
>>
>> The SoC topology is a graph (or more specifically, a tree) and its
>> edges are specified using the 'samsung,interconnect-parent' in the
> 
> samsung,interconnect-parent -> interconnects?

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

I've changed the sentence to:
"The SoC topology is a graph (or more specifically, a tree) and its
edges are described by specifying in the 'interconnects' property
the interconnect consumer path for each interconnect provider DT node."

>> DT. Due to unspecified relative probing order, -EPROBE_DEFER may be
>> propagated to ensure that the parent is probed before its children.
>>
>> Each bus is now an interconnect provider and an interconnect node as
>> well (cf. Documentation/interconnect/interconnect.rst), i.e. every bus
>> registers itself as a node. Node IDs are not hardcoded but rather
>> assigned dynamically at runtime. This approach allows for using this
>> driver with various Exynos SoCs.
>>
>> Frequencies requested via the interconnect API for a given node are
>> propagated to devfreq using dev_pm_qos_update_request(). Please note
>> that it is not an error when CONFIG_INTERCONNECT is 'n', in which
>> case all interconnect API functions are no-op.
>>
>> The bus-width DT property is to determine the interconnect data
>> width and traslate requested bandwidth to clock frequency for each
>> bus.
>>
>> Signed-off-by: Artur Świgoń <a.swigon@samsung.com>
>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>

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

>> +struct exynos_icc_priv {
>> +	struct device *dev;
>> +
>> +	/* One interconnect node per provider */
>> +	struct icc_provider provider;
>> +	struct icc_node *node;
>> +
>> +	struct dev_pm_qos_request qos_req;
>> +	u32 bus_clk_ratio;
>> +};
>> +
>> +static struct icc_node *exynos_icc_get_parent(struct device_node *np)
>> +{
>> +	struct of_phandle_args args;
>> +	struct icc_node_data *icc_node_data;
>> +	struct icc_node *icc_node;
>> +	int num, ret;
>> +
>> +	num = of_count_phandle_with_args(np, "interconnects",
>> +					 "#interconnect-cells");
>> +	if (num < 1)
>> +		return NULL; /* parent nodes are optional */
>> +
>> +	/* Get the interconnect target node */
>> +	ret = of_parse_phandle_with_args(np, "interconnects",
>> +					"#interconnect-cells", 0, &args);
>> +	if (ret < 0)
>> +		return ERR_PTR(ret);
>> +
>> +	icc_node_data = of_icc_get_from_provider(&args);
>> +	of_node_put(args.np);
>> +
>> +	if (IS_ERR(icc_node_data))
>> +		return ERR_CAST(icc_node_data);
>> +
>> +	icc_node = icc_node_data->node;
>> +	kfree(icc_node_data);
>> +
>> +	return icc_node;
>> +}
> 
> I have a question about exynos_icc_get_parent().
> As I checked, this function returns the only one icc_node
> as parent node. But, bus_display dt node in the exynos4412.dtsi
> specifies the two interconnect node as following with bus_leftbus, bus_dmc,
> 
> When I checked the return value of exynos_icc_get_parent()
> during probing for bus_display device, exynos_icc_get_parent() function
> only returns 'bus_leftbus' icc_node. Do you need to add two phandle
> of icc node?

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

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

-- 
Regards,
Sylwester
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Chanwoo Choi]

Hi Sylwester,

On Tue, Nov 3, 2020 at 8:32 PM Sylwester Nawrocki
<s.nawrocki@samsung.com> wrote:
>
> On 03.11.2020 10:37, Chanwoo Choi wrote:
> > On 10/30/20 9:51 PM, Sylwester Nawrocki wrote:
> >> This patch adds a generic interconnect driver for Exynos SoCs in order
> >> to provide interconnect functionality for each "samsung,exynos-bus"
> >> compatible device.
> >>
> >> The SoC topology is a graph (or more specifically, a tree) and its
> >> edges are specified using the 'samsung,interconnect-parent' in the
> >
> > samsung,interconnect-parent -> interconnects?
>
> Yes, I will rephrase the whole commit message as it's a bit outdated now.
>
> I've changed the sentence to:
> "The SoC topology is a graph (or more specifically, a tree) and its
> edges are described by specifying in the 'interconnects' property
> the interconnect consumer path for each interconnect provider DT node."
>
> >> DT. Due to unspecified relative probing order, -EPROBE_DEFER may be
> >> propagated to ensure that the parent is probed before its children.
> >>
> >> Each bus is now an interconnect provider and an interconnect node as
> >> well (cf. Documentation/interconnect/interconnect.rst), i.e. every bus
> >> registers itself as a node. Node IDs are not hardcoded but rather
> >> assigned dynamically at runtime. This approach allows for using this
> >> driver with various Exynos SoCs.
> >>
> >> Frequencies requested via the interconnect API for a given node are
> >> propagated to devfreq using dev_pm_qos_update_request(). Please note
> >> that it is not an error when CONFIG_INTERCONNECT is 'n', in which
> >> case all interconnect API functions are no-op.
> >>
> >> The bus-width DT property is to determine the interconnect data
> >> width and traslate requested bandwidth to clock frequency for each
> >> bus.
> >>
> >> Signed-off-by: Artur Świgoń <a.swigon@samsung.com>
> >> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
>
> >> +++ b/drivers/interconnect/exynos/exynos.c
>
> >> +struct exynos_icc_priv {
> >> +    struct device *dev;
> >> +
> >> +    /* One interconnect node per provider */
> >> +    struct icc_provider provider;
> >> +    struct icc_node *node;
> >> +
> >> +    struct dev_pm_qos_request qos_req;
> >> +    u32 bus_clk_ratio;
> >> +};
> >> +
> >> +static struct icc_node *exynos_icc_get_parent(struct device_node *np)
> >> +{
> >> +    struct of_phandle_args args;
> >> +    struct icc_node_data *icc_node_data;
> >> +    struct icc_node *icc_node;
> >> +    int num, ret;
> >> +
> >> +    num = of_count_phandle_with_args(np, "interconnects",
> >> +                                     "#interconnect-cells");
> >> +    if (num < 1)
> >> +            return NULL; /* parent nodes are optional */
> >> +
> >> +    /* Get the interconnect target node */
> >> +    ret = of_parse_phandle_with_args(np, "interconnects",
> >> +                                    "#interconnect-cells", 0, &args);
> >> +    if (ret < 0)
> >> +            return ERR_PTR(ret);
> >> +
> >> +    icc_node_data = of_icc_get_from_provider(&args);
> >> +    of_node_put(args.np);
> >> +
> >> +    if (IS_ERR(icc_node_data))
> >> +            return ERR_CAST(icc_node_data);
> >> +
> >> +    icc_node = icc_node_data->node;
> >> +    kfree(icc_node_data);
> >> +
> >> +    return icc_node;
> >> +}
> >
> > I have a question about exynos_icc_get_parent().
> > As I checked, this function returns the only one icc_node
> > as parent node. But, bus_display dt node in the exynos4412.dtsi
> > specifies the two interconnect node as following with bus_leftbus, bus_dmc,
> >
> > When I checked the return value of exynos_icc_get_parent()
> > during probing for bus_display device, exynos_icc_get_parent() function
> > only returns 'bus_leftbus' icc_node. Do you need to add two phandle
> > of icc node?
>
> Yes, as we use the interconnect consumer bindings we need to specify a path,
> i.e. a <initiator, target> pair. When the provider node initializes it will
> link itself to that path. Currently the provider driver uses just the first
> phandle.

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

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

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



-- 
Best Regards,
Chanwoo Choi

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

[Chanwoo Choi]

Hi Sylwester,

On Tue, Nov 3, 2020 at 8:32 PM Sylwester Nawrocki
<s.nawrocki@samsung.com> wrote:
>
> On 03.11.2020 10:37, Chanwoo Choi wrote:
> > On 10/30/20 9:51 PM, Sylwester Nawrocki wrote:
> >> This patch adds a generic interconnect driver for Exynos SoCs in order
> >> to provide interconnect functionality for each "samsung,exynos-bus"
> >> compatible device.
> >>
> >> The SoC topology is a graph (or more specifically, a tree) and its
> >> edges are specified using the 'samsung,interconnect-parent' in the
> >
> > samsung,interconnect-parent -> interconnects?
>
> Yes, I will rephrase the whole commit message as it's a bit outdated now.
>
> I've changed the sentence to:
> "The SoC topology is a graph (or more specifically, a tree) and its
> edges are described by specifying in the 'interconnects' property
> the interconnect consumer path for each interconnect provider DT node."
>
> >> DT. Due to unspecified relative probing order, -EPROBE_DEFER may be
> >> propagated to ensure that the parent is probed before its children.
> >>
> >> Each bus is now an interconnect provider and an interconnect node as
> >> well (cf. Documentation/interconnect/interconnect.rst), i.e. every bus
> >> registers itself as a node. Node IDs are not hardcoded but rather
> >> assigned dynamically at runtime. This approach allows for using this
> >> driver with various Exynos SoCs.
> >>
> >> Frequencies requested via the interconnect API for a given node are
> >> propagated to devfreq using dev_pm_qos_update_request(). Please note
> >> that it is not an error when CONFIG_INTERCONNECT is 'n', in which
> >> case all interconnect API functions are no-op.
> >>
> >> The bus-width DT property is to determine the interconnect data
> >> width and traslate requested bandwidth to clock frequency for each
> >> bus.
> >>
> >> Signed-off-by: Artur Świgoń <a.swigon@samsung.com>
> >> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
>
> >> +++ b/drivers/interconnect/exynos/exynos.c
>
> >> +struct exynos_icc_priv {
> >> +    struct device *dev;
> >> +
> >> +    /* One interconnect node per provider */
> >> +    struct icc_provider provider;
> >> +    struct icc_node *node;
> >> +
> >> +    struct dev_pm_qos_request qos_req;
> >> +    u32 bus_clk_ratio;
> >> +};
> >> +
> >> +static struct icc_node *exynos_icc_get_parent(struct device_node *np)
> >> +{
> >> +    struct of_phandle_args args;
> >> +    struct icc_node_data *icc_node_data;
> >> +    struct icc_node *icc_node;
> >> +    int num, ret;
> >> +
> >> +    num = of_count_phandle_with_args(np, "interconnects",
> >> +                                     "#interconnect-cells");
> >> +    if (num < 1)
> >> +            return NULL; /* parent nodes are optional */
> >> +
> >> +    /* Get the interconnect target node */
> >> +    ret = of_parse_phandle_with_args(np, "interconnects",
> >> +                                    "#interconnect-cells", 0, &args);
> >> +    if (ret < 0)
> >> +            return ERR_PTR(ret);
> >> +
> >> +    icc_node_data = of_icc_get_from_provider(&args);
> >> +    of_node_put(args.np);
> >> +
> >> +    if (IS_ERR(icc_node_data))
> >> +            return ERR_CAST(icc_node_data);
> >> +
> >> +    icc_node = icc_node_data->node;
> >> +    kfree(icc_node_data);
> >> +
> >> +    return icc_node;
> >> +}
> >
> > I have a question about exynos_icc_get_parent().
> > As I checked, this function returns the only one icc_node
> > as parent node. But, bus_display dt node in the exynos4412.dtsi
> > specifies the two interconnect node as following with bus_leftbus, bus_dmc,
> >
> > When I checked the return value of exynos_icc_get_parent()
> > during probing for bus_display device, exynos_icc_get_parent() function
> > only returns 'bus_leftbus' icc_node. Do you need to add two phandle
> > of icc node?
>
> Yes, as we use the interconnect consumer bindings we need to specify a path,
> i.e. a <initiator, target> pair. When the provider node initializes it will
> link itself to that path. Currently the provider driver uses just the first
> phandle.

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

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

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



-- 
Best Regards,
Chanwoo Choi
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Sylwester Nawrocki]

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

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

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

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

-- 
Regards,
Sylwester

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

[Sylwester Nawrocki]

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

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

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

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

-- 
Regards,
Sylwester
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v7 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 v7, v6:
 - none.

Changes for v5:
 - new patch.
---
 drivers/devfreq/exynos-bus.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
index 1e684a4..ee300ee 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);
 }
@@ -432,6 +437,18 @@ static int exynos_bus_probe(struct platform_device *pdev)
 	if (ret < 0)
 		goto err;
 
+	/* Create child platform device for the interconnect provider */
+	if (of_get_property(dev->of_node, "#interconnect-cells", NULL)) {
+		    bus->icc_pdev = platform_device_register_data(
+						dev, "exynos-generic-icc",
+						PLATFORM_DEVID_AUTO, NULL, 0);
+
+		    if (IS_ERR(bus->icc_pdev)) {
+			    ret = PTR_ERR(bus->icc_pdev);
+			    goto err;
+		    }
+	}
+
 	max_state = bus->devfreq->profile->max_state;
 	min_freq = (bus->devfreq->profile->freq_table[0] / 1000);
 	max_freq = (bus->devfreq->profile->freq_table[max_state - 1] / 1000);
-- 
2.7.4

[PATCH v7 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 v7, v6:
 - none.

Changes for v5:
 - new patch.
---
 drivers/devfreq/exynos-bus.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
index 1e684a4..ee300ee 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);
 }
@@ -432,6 +437,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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Krzysztof Kozlowski]

On Fri, Oct 30, 2020 at 01:51:46PM +0100, Sylwester Nawrocki 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 v7, v6:
>  - none.
> 
> Changes for v5:
>  - new patch.
> ---
>  drivers/devfreq/exynos-bus.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 

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

Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On Fri, Oct 30, 2020 at 01:51:46PM +0100, Sylwester Nawrocki 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 v7, v6:
>  - none.
> 
> Changes for v5:
>  - new patch.
> ---
>  drivers/devfreq/exynos-bus.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 

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

Best regards,
Krzysztof
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Chanwoo Choi]

Hi Sylwester,

On 10/30/20 9:51 PM, Sylwester Nawrocki 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 v7, v6:
>  - none.
> 
> Changes for v5:
>  - new patch.
> ---
>  drivers/devfreq/exynos-bus.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
> index 1e684a4..ee300ee 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);
>  }
> @@ -432,6 +437,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);
> 

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

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

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

[Chanwoo Choi]

Hi Sylwester,

On 10/30/20 9:51 PM, Sylwester Nawrocki 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 v7, v6:
>  - none.
> 
> Changes for v5:
>  - new patch.
> ---
>  drivers/devfreq/exynos-bus.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
> index 1e684a4..ee300ee 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);
>  }
> @@ -432,6 +437,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);
> 

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

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Chanwoo Choi]

Hi Sylwester,

On 10/30/20 9:51 PM, Sylwester Nawrocki 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 v7, v6:
>  - none.
> 
> Changes for v5:
>  - new patch.
> ---
>  drivers/devfreq/exynos-bus.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)

We don't need to  add 'select INTERCONNECT_EXYNOS' in Kconfig?
Do you want to remain it as optional according to user?

> 
> diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
> index 1e684a4..ee300ee 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);
>  }
> @@ -432,6 +437,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);
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

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

[Chanwoo Choi]

Hi Sylwester,

On 10/30/20 9:51 PM, Sylwester Nawrocki 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 v7, v6:
>  - none.
> 
> Changes for v5:
>  - new patch.
> ---
>  drivers/devfreq/exynos-bus.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)

We don't need to  add 'select INTERCONNECT_EXYNOS' in Kconfig?
Do you want to remain it as optional according to user?

> 
> diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
> index 1e684a4..ee300ee 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);
>  }
> @@ -432,6 +437,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);
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Sylwester Nawrocki]

Hi Chanwoo,

On 03.11.2020 11:45, Chanwoo Choi wrote:
> On 10/30/20 9:51 PM, Sylwester Nawrocki 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>

>>  drivers/devfreq/exynos-bus.c | 17 +++++++++++++++++

> We don't need to  add 'select INTERCONNECT_EXYNOS' in Kconfig?

I think by doing so we could run into some dependency issues.

> Do you want to remain it as optional according to user?
Yes, I would prefer to keep it selectable through defconfig. 
Currently it's only needed by one 32-bit ARM board.

-- 
Regards,
Sylwester

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

[Sylwester Nawrocki]

Hi Chanwoo,

On 03.11.2020 11:45, Chanwoo Choi wrote:
> On 10/30/20 9:51 PM, Sylwester Nawrocki 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>

>>  drivers/devfreq/exynos-bus.c | 17 +++++++++++++++++

> We don't need to  add 'select INTERCONNECT_EXYNOS' in Kconfig?

I think by doing so we could run into some dependency issues.

> Do you want to remain it as optional according to user?
Yes, I would prefer to keep it selectable through defconfig. 
Currently it's only needed by one 32-bit ARM board.

-- 
Regards,
Sylwester
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Krzysztof Kozlowski]

On Tue, 3 Nov 2020 at 13:32, Sylwester Nawrocki <s.nawrocki@samsung.com> wrote:
>
> Hi Chanwoo,
>
> On 03.11.2020 11:45, Chanwoo Choi wrote:
> > On 10/30/20 9:51 PM, Sylwester Nawrocki 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>
>
> >>  drivers/devfreq/exynos-bus.c | 17 +++++++++++++++++
>
> > We don't need to  add 'select INTERCONNECT_EXYNOS' in Kconfig?
>
> I think by doing so we could run into some dependency issues.
>
> > Do you want to remain it as optional according to user?
> Yes, I would prefer to keep it selectable through defconfig.
> Currently it's only needed by one 32-bit ARM board.

I am fine with it as it is really optional.

You could consider then "imply" but then you would need to check for
dependencies (the same as with select).

Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On Tue, 3 Nov 2020 at 13:32, Sylwester Nawrocki <s.nawrocki@samsung.com> wrote:
>
> Hi Chanwoo,
>
> On 03.11.2020 11:45, Chanwoo Choi wrote:
> > On 10/30/20 9:51 PM, Sylwester Nawrocki 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>
>
> >>  drivers/devfreq/exynos-bus.c | 17 +++++++++++++++++
>
> > We don't need to  add 'select INTERCONNECT_EXYNOS' in Kconfig?
>
> I think by doing so we could run into some dependency issues.
>
> > Do you want to remain it as optional according to user?
> Yes, I would prefer to keep it selectable through defconfig.
> Currently it's only needed by one 32-bit ARM board.

I am fine with it as it is really optional.

You could consider then "imply" but then you would need to check for
dependencies (the same as with select).

Best regards,
Krzysztof
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Chanwoo Choi]

Hi Sylwester,

On Tue, Nov 3, 2020 at 9:32 PM Sylwester Nawrocki
<s.nawrocki@samsung.com> wrote:
>
> Hi Chanwoo,
>
> On 03.11.2020 11:45, Chanwoo Choi wrote:
> > On 10/30/20 9:51 PM, Sylwester Nawrocki 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>
>
> >>  drivers/devfreq/exynos-bus.c | 17 +++++++++++++++++
>
> > We don't need to  add 'select INTERCONNECT_EXYNOS' in Kconfig?
>
> I think by doing so we could run into some dependency issues.
>
> > Do you want to remain it as optional according to user?
> Yes, I would prefer to keep it selectable through defconfig.
> Currently it's only needed by one 32-bit ARM board.

OK.

-- 
Best Regards,
Chanwoo Choi

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

[Chanwoo Choi]

Hi Sylwester,

On Tue, Nov 3, 2020 at 9:32 PM Sylwester Nawrocki
<s.nawrocki@samsung.com> wrote:
>
> Hi Chanwoo,
>
> On 03.11.2020 11:45, Chanwoo Choi wrote:
> > On 10/30/20 9:51 PM, Sylwester Nawrocki 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>
>
> >>  drivers/devfreq/exynos-bus.c | 17 +++++++++++++++++
>
> > We don't need to  add 'select INTERCONNECT_EXYNOS' in Kconfig?
>
> I think by doing so we could run into some dependency issues.
>
> > Do you want to remain it as optional according to user?
> Yes, I would prefer to keep it selectable through defconfig.
> Currently it's only needed by one 32-bit ARM board.

OK.

-- 
Best Regards,
Chanwoo Choi
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v7 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:
 - interconnects: to declare connections between nodes in order to
   guarantee PM QoS requirements between nodes,
 - #interconnect-cells: required by the interconnect framework,
 - samsung,data-clk-ratio: which allows to specify minimum data clock
   frequency corresponding to requested bandwidth for each bus.

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

Signed-off-by: Artur Świgoń <a.swigon@samsung.com>
Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
---
Changes for v7:
 - adjusted to the DT property changes: "interconnects" instead
   of "samsung,interconnect-parent", "samsung,data-clk-ratio"
   instead of "bus-width".

Changes for v6:
 - added bus-width property in bus_dmc node.

Changes for v5:
 - adjust to renamed exynos,interconnect-parent-node property,
 - add properties in common exynos4412.dtsi file rather than
   in Odroid specific odroid4412-odroid-common.dtsi.
---
 arch/arm/boot/dts/exynos4412.dtsi | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm/boot/dts/exynos4412.dtsi b/arch/arm/boot/dts/exynos4412.dtsi
index e76881d..4999e68 100644
--- a/arch/arm/boot/dts/exynos4412.dtsi
+++ b/arch/arm/boot/dts/exynos4412.dtsi
@@ -383,6 +383,8 @@
 			clocks = <&clock CLK_DIV_DMC>;
 			clock-names = "bus";
 			operating-points-v2 = <&bus_dmc_opp_table>;
+			samsung,data-clock-ratio = <4>;
+			#interconnect-cells = <0>;
 			status = "disabled";
 		};
 
@@ -452,6 +454,8 @@
 			clocks = <&clock CLK_DIV_GDL>;
 			clock-names = "bus";
 			operating-points-v2 = <&bus_leftbus_opp_table>;
+			interconnects = <&bus_dmc>;
+			#interconnect-cells = <0>;
 			status = "disabled";
 		};
 
@@ -468,6 +472,8 @@
 			clocks = <&clock CLK_ACLK160>;
 			clock-names = "bus";
 			operating-points-v2 = <&bus_display_opp_table>;
+			interconnects = <&bus_leftbus &bus_dmc>;
+			#interconnect-cells = <0>;
 			status = "disabled";
 		};
 
-- 
2.7.4

[PATCH v7 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:
 - interconnects: to declare connections between nodes in order to
   guarantee PM QoS requirements between nodes,
 - #interconnect-cells: required by the interconnect framework,
 - samsung,data-clk-ratio: which allows to specify minimum data clock
   frequency corresponding to requested bandwidth for each bus.

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

Signed-off-by: Artur Świgoń <a.swigon@samsung.com>
Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
---
Changes for v7:
 - adjusted to the DT property changes: "interconnects" instead
   of "samsung,interconnect-parent", "samsung,data-clk-ratio"
   instead of "bus-width".

Changes for v6:
 - added bus-width property in bus_dmc node.

Changes for v5:
 - adjust to renamed exynos,interconnect-parent-node property,
 - add properties in common exynos4412.dtsi file rather than
   in Odroid specific odroid4412-odroid-common.dtsi.
---
 arch/arm/boot/dts/exynos4412.dtsi | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm/boot/dts/exynos4412.dtsi b/arch/arm/boot/dts/exynos4412.dtsi
index e76881d..4999e68 100644
--- a/arch/arm/boot/dts/exynos4412.dtsi
+++ b/arch/arm/boot/dts/exynos4412.dtsi
@@ -383,6 +383,8 @@
 			clocks = <&clock CLK_DIV_DMC>;
 			clock-names = "bus";
 			operating-points-v2 = <&bus_dmc_opp_table>;
+			samsung,data-clock-ratio = <4>;
+			#interconnect-cells = <0>;
 			status = "disabled";
 		};
 
@@ -452,6 +454,8 @@
 			clocks = <&clock CLK_DIV_GDL>;
 			clock-names = "bus";
 			operating-points-v2 = <&bus_leftbus_opp_table>;
+			interconnects = <&bus_dmc>;
+			#interconnect-cells = <0>;
 			status = "disabled";
 		};
 
@@ -468,6 +472,8 @@
 			clocks = <&clock CLK_ACLK160>;
 			clock-names = "bus";
 			operating-points-v2 = <&bus_display_opp_table>;
+			interconnects = <&bus_leftbus &bus_dmc>;
+			#interconnect-cells = <0>;
 			status = "disabled";
 		};
 
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v7 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 v7, v6, 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 4999e68..d07739e 100644
--- a/arch/arm/boot/dts/exynos4412.dtsi
+++ b/arch/arm/boot/dts/exynos4412.dtsi
@@ -779,6 +779,7 @@
 	clock-names = "mixer", "hdmi", "sclk_hdmi", "vp";
 	clocks = <&clock CLK_MIXER>, <&clock CLK_HDMI>,
 		 <&clock CLK_SCLK_HDMI>, <&clock CLK_VP>;
+	interconnects = <&bus_display &bus_dmc>;
 };
 
 &pmu {
-- 
2.7.4

[PATCH v7 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 v7, v6, 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 4999e68..d07739e 100644
--- a/arch/arm/boot/dts/exynos4412.dtsi
+++ b/arch/arm/boot/dts/exynos4412.dtsi
@@ -779,6 +779,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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Sylwester Nawrocki]

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

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

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

Co-developed-by: Artur Świgoń <a.swigon@samsung.com>
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
---
Changes for v7:
 - fixed incorrect setting of the ICC bandwidth when the mixer is
   disabled, now the bandwidth is set explicitly to 0 in such case.

Changes for v6:
 - the icc_set_bw() call is now only done when calculated value for
   a crtc changes, this avoids unnecessary calls per each video frame
 - added synchronization of the interconnect bandwidth setting with
   the mixer VSYNC in order to avoid buffer underflow when we lower
   the interconnect bandwidth when the hardware still operates with
   previous mode settings that require higher bandwidth. This fixed
   IOMMU faults observed e.g. during switching from two planes to
   a single plane operation.

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

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

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

[Sylwester Nawrocki]

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

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

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

Co-developed-by: Artur Świgoń <a.swigon@samsung.com>
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
---
Changes for v7:
 - fixed incorrect setting of the ICC bandwidth when the mixer is
   disabled, now the bandwidth is set explicitly to 0 in such case.

Changes for v6:
 - the icc_set_bw() call is now only done when calculated value for
   a crtc changes, this avoids unnecessary calls per each video frame
 - added synchronization of the interconnect bandwidth setting with
   the mixer VSYNC in order to avoid buffer underflow when we lower
   the interconnect bandwidth when the hardware still operates with
   previous mode settings that require higher bandwidth. This fixed
   IOMMU faults observed e.g. during switching from two planes to
   a single plane operation.

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

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Krzysztof Kozlowski]

On Fri, Oct 30, 2020 at 01:51:49PM +0100, Sylwester Nawrocki wrote:
> This patch adds interconnect support to exynos-mixer. The mixer works
> the same as before when CONFIG_INTERCONNECT is 'n'.
> 
> For proper operation of the video mixer block we need to ensure the
> interconnect busses like DMC or LEFTBUS provide enough bandwidth so
> as to avoid DMA buffer underruns in the mixer block. I.e we need to
> prevent those busses from operating in low perfomance OPPs when
> the mixer is running.
> In this patch the bus bandwidth request is done through the interconnect
> API, the bandwidth value is calculated from selected DRM mode, i.e.
> video plane width, height, refresh rate and pixel format.
> 
> The bandwidth setting is synchronized with VSYNC when we are switching
> to lower bandwidth. This is required to ensure enough bandwidth for
> the device since new settings are normally being applied in the hardware
> synchronously with VSYNC.
> 
> Co-developed-by: Artur Świgoń <a.swigon@samsung.com>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> ---
> Changes for v7:
>  - fixed incorrect setting of the ICC bandwidth when the mixer is
>    disabled, now the bandwidth is set explicitly to 0 in such case.
> 
> Changes for v6:
>  - the icc_set_bw() call is now only done when calculated value for
>    a crtc changes, this avoids unnecessary calls per each video frame
>  - added synchronization of the interconnect bandwidth setting with
>    the mixer VSYNC in order to avoid buffer underflow when we lower
>    the interconnect bandwidth when the hardware still operates with
>    previous mode settings that require higher bandwidth. This fixed
>    IOMMU faults observed e.g. during switching from two planes to
>    a single plane operation.
> 
> Changes for v5:
>  - renamed soc_path variable to icc_path
> ---
>  drivers/gpu/drm/exynos/exynos_mixer.c | 146 ++++++++++++++++++++++++++++++++--
>  1 file changed, 138 insertions(+), 8 deletions(-)
> 

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

Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On Fri, Oct 30, 2020 at 01:51:49PM +0100, Sylwester Nawrocki wrote:
> This patch adds interconnect support to exynos-mixer. The mixer works
> the same as before when CONFIG_INTERCONNECT is 'n'.
> 
> For proper operation of the video mixer block we need to ensure the
> interconnect busses like DMC or LEFTBUS provide enough bandwidth so
> as to avoid DMA buffer underruns in the mixer block. I.e we need to
> prevent those busses from operating in low perfomance OPPs when
> the mixer is running.
> In this patch the bus bandwidth request is done through the interconnect
> API, the bandwidth value is calculated from selected DRM mode, i.e.
> video plane width, height, refresh rate and pixel format.
> 
> The bandwidth setting is synchronized with VSYNC when we are switching
> to lower bandwidth. This is required to ensure enough bandwidth for
> the device since new settings are normally being applied in the hardware
> synchronously with VSYNC.
> 
> Co-developed-by: Artur Świgoń <a.swigon@samsung.com>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> ---
> Changes for v7:
>  - fixed incorrect setting of the ICC bandwidth when the mixer is
>    disabled, now the bandwidth is set explicitly to 0 in such case.
> 
> Changes for v6:
>  - the icc_set_bw() call is now only done when calculated value for
>    a crtc changes, this avoids unnecessary calls per each video frame
>  - added synchronization of the interconnect bandwidth setting with
>    the mixer VSYNC in order to avoid buffer underflow when we lower
>    the interconnect bandwidth when the hardware still operates with
>    previous mode settings that require higher bandwidth. This fixed
>    IOMMU faults observed e.g. during switching from two planes to
>    a single plane operation.
> 
> Changes for v5:
>  - renamed soc_path variable to icc_path
> ---
>  drivers/gpu/drm/exynos/exynos_mixer.c | 146 ++++++++++++++++++++++++++++++++--
>  1 file changed, 138 insertions(+), 8 deletions(-)
> 

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

Best regards,
Krzysztof
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Krzysztof Kozlowski]

On Fri, Oct 30, 2020 at 01:51:49PM +0100, Sylwester Nawrocki wrote:
> This patch adds interconnect support to exynos-mixer. The mixer works
> the same as before when CONFIG_INTERCONNECT is 'n'.
> 
> For proper operation of the video mixer block we need to ensure the
> interconnect busses like DMC or LEFTBUS provide enough bandwidth so
> as to avoid DMA buffer underruns in the mixer block. I.e we need to
> prevent those busses from operating in low perfomance OPPs when
> the mixer is running.
> In this patch the bus bandwidth request is done through the interconnect
> API, the bandwidth value is calculated from selected DRM mode, i.e.
> video plane width, height, refresh rate and pixel format.
> 
> The bandwidth setting is synchronized with VSYNC when we are switching
> to lower bandwidth. This is required to ensure enough bandwidth for
> the device since new settings are normally being applied in the hardware
> synchronously with VSYNC.
> 
> Co-developed-by: Artur Świgoń <a.swigon@samsung.com>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> ---
> Changes for v7:
>  - fixed incorrect setting of the ICC bandwidth when the mixer is
>    disabled, now the bandwidth is set explicitly to 0 in such case.
> 
> Changes for v6:
>  - the icc_set_bw() call is now only done when calculated value for
>    a crtc changes, this avoids unnecessary calls per each video frame
>  - added synchronization of the interconnect bandwidth setting with
>    the mixer VSYNC in order to avoid buffer underflow when we lower
>    the interconnect bandwidth when the hardware still operates with
>    previous mode settings that require higher bandwidth. This fixed
>    IOMMU faults observed e.g. during switching from two planes to
>    a single plane operation.
> 
> Changes for v5:
>  - renamed soc_path variable to icc_path
> ---
>  drivers/gpu/drm/exynos/exynos_mixer.c | 146 ++++++++++++++++++++++++++++++++--
>  1 file changed, 138 insertions(+), 8 deletions(-)


[...]

> @@ -1223,19 +1330,33 @@ static int mixer_probe(struct platform_device *pdev)
>  	struct device *dev = &pdev->dev;
>  	const struct mixer_drv_data *drv;
>  	struct mixer_context *ctx;
> +	struct icc_path *path;
>  	int ret;
>  
> +	/*
> +	 * Returns NULL if CONFIG_INTERCONNECT is disabled.

You could add here:
or if "interconnects" property does not exist.

Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On Fri, Oct 30, 2020 at 01:51:49PM +0100, Sylwester Nawrocki wrote:
> This patch adds interconnect support to exynos-mixer. The mixer works
> the same as before when CONFIG_INTERCONNECT is 'n'.
> 
> For proper operation of the video mixer block we need to ensure the
> interconnect busses like DMC or LEFTBUS provide enough bandwidth so
> as to avoid DMA buffer underruns in the mixer block. I.e we need to
> prevent those busses from operating in low perfomance OPPs when
> the mixer is running.
> In this patch the bus bandwidth request is done through the interconnect
> API, the bandwidth value is calculated from selected DRM mode, i.e.
> video plane width, height, refresh rate and pixel format.
> 
> The bandwidth setting is synchronized with VSYNC when we are switching
> to lower bandwidth. This is required to ensure enough bandwidth for
> the device since new settings are normally being applied in the hardware
> synchronously with VSYNC.
> 
> Co-developed-by: Artur Świgoń <a.swigon@samsung.com>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> ---
> Changes for v7:
>  - fixed incorrect setting of the ICC bandwidth when the mixer is
>    disabled, now the bandwidth is set explicitly to 0 in such case.
> 
> Changes for v6:
>  - the icc_set_bw() call is now only done when calculated value for
>    a crtc changes, this avoids unnecessary calls per each video frame
>  - added synchronization of the interconnect bandwidth setting with
>    the mixer VSYNC in order to avoid buffer underflow when we lower
>    the interconnect bandwidth when the hardware still operates with
>    previous mode settings that require higher bandwidth. This fixed
>    IOMMU faults observed e.g. during switching from two planes to
>    a single plane operation.
> 
> Changes for v5:
>  - renamed soc_path variable to icc_path
> ---
>  drivers/gpu/drm/exynos/exynos_mixer.c | 146 ++++++++++++++++++++++++++++++++--
>  1 file changed, 138 insertions(+), 8 deletions(-)


[...]

> @@ -1223,19 +1330,33 @@ static int mixer_probe(struct platform_device *pdev)
>  	struct device *dev = &pdev->dev;
>  	const struct mixer_drv_data *drv;
>  	struct mixer_context *ctx;
> +	struct icc_path *path;
>  	int ret;
>  
> +	/*
> +	 * Returns NULL if CONFIG_INTERCONNECT is disabled.

You could add here:
or if "interconnects" property does not exist.

Best regards,
Krzysztof
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Sylwester Nawrocki]

On 31.10.2020 13:47, Krzysztof Kozlowski wrote:
>> @@ -1223,19 +1330,33 @@ static int mixer_probe(struct platform_device *pdev)
>>  	struct device *dev = &pdev->dev;
>>  	const struct mixer_drv_data *drv;
>>  	struct mixer_context *ctx;
>> +	struct icc_path *path;
>>  	int ret;
>>  
>> +	/*
>> +	 * Returns NULL if CONFIG_INTERCONNECT is disabled.

> You could add here:
> or if "interconnects" property does not exist.

Right, thanks for pointing out, I will update that comment.

-- 
Regards,
Sylwester

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

[Sylwester Nawrocki]

On 31.10.2020 13:47, Krzysztof Kozlowski wrote:
>> @@ -1223,19 +1330,33 @@ static int mixer_probe(struct platform_device *pdev)
>>  	struct device *dev = &pdev->dev;
>>  	const struct mixer_drv_data *drv;
>>  	struct mixer_context *ctx;
>> +	struct icc_path *path;
>>  	int ret;
>>  
>> +	/*
>> +	 * Returns NULL if CONFIG_INTERCONNECT is disabled.

> You could add here:
> or if "interconnects" property does not exist.

Right, thanks for pointing out, I will update that comment.

-- 
Regards,
Sylwester
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Chanwoo Choi]

Hi Sylwester,

When I tested this patchset on Odroid-U3,
After setting 0 bps by interconnect[1][2],
the frequency of devfreq devs sustain the high frequency
according to the pm qos request.

So, I try to find the cause of this situation.
In result, it seems that interconnect exynos driver
updates the pm qos request to devfreq device
during the kernel booting. Do you know why the exynos
interconnect driver request the pm qos during probe
without the mixer request?

PS. The passive governor has a bug related to PM_QOS interface.
So, I posted the patch[4].


[1] interconnect_graph
root@localhost:~# cat /sys/kernel/debug/interconnect/interconnect_graph 
digraph {
        rankdir = LR
        node [shape = record]
        subgraph cluster_1 {
                label = "soc:bus_dmc"
                "2:bus_dmc" [label="2:bus_dmc
                        |avg_bw=0kBps
                        |peak_bw=0kBps"]
        }
        subgraph cluster_2 {
                label = "soc:bus_leftbus"
                "3:bus_leftbus" [label="3:bus_leftbus
                        |avg_bw=0kBps
                        |peak_bw=0kBps"]
        }
        subgraph cluster_3 {
                label = "soc:bus_display"
                "4:bus_display" [label="4:bus_display
                        |avg_bw=0kBps
                        |peak_bw=0kBps"]
        }
        "3:bus_leftbus" -> "2:bus_dmc"
        "4:bus_display" -> "3:bus_leftbus"


[2] interconnect_summary
root@localhost:~# cat /sys/kernel/debug/interconnect/interconnect_summary 
 node                                  tag          avg         peak
--------------------------------------------------------------------
bus_dmc                                               0            0
  12c10000.mixer                         0            0            0
bus_leftbus                                           0            0
  12c10000.mixer                         0            0            0
bus_display                                           0            0
  12c10000.mixer                         0            0            0


[3] devfreq_summary
root@localhost:~# cat /sys/kernel/debug/devfreq/devfreq_summary 
dev                            parent_dev                     governor        timer      polling_ms  cur_freq_Hz  min_freq_Hz  max_freq_Hz
------------------------------ ------------------------------ --------------- ---------- ---------- ------------ ------------ ------------
soc:bus_dmc                    null                           simple_ondemand deferrable         50    400000000    400000000    400000000
soc:bus_acp                    soc:bus_dmc                    passive         null                0    267000000    100000000    267000000
soc:bus_c2c                    soc:bus_dmc                    passive         null                0    400000000    100000000    400000000
soc:bus_leftbus                null                           simple_ondemand deferrable         50    200000000    200000000    200000000
soc:bus_rightbus               soc:bus_leftbus                passive         null                0    200000000    100000000    200000000
soc:bus_display                soc:bus_leftbus                passive         null                0    200000000    200000000    200000000
soc:bus_fsys                   soc:bus_leftbus                passive         null                0    134000000    100000000    134000000
soc:bus_peri                   soc:bus_leftbus                passive         null                0    100000000     50000000    100000000
soc:bus_mfc                    soc:bus_leftbus                passive         null                0    200000000    100000000    200000000


[4] PM / devfreq: passive: Update frequency when start governor
https://patchwork.kernel.org/project/linux-pm/patch/20201103070646.18687-1-cw00.choi@samsung.com/


On 10/30/20 9:51 PM, 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 aallows to avoid issues like the one discussed
> in thread [1].
> 
> This patch series adds implementation of the interconnect provider per each
> "samsung,exynos-bus" compatible DT node, with one interconnect node per
> provider.  The interconnect code which was previously added as a part of
> the devfreq driver has been converted to a separate platform driver.
> In the devfreq a corresponding virtual child platform device is registered.
> Integration of devfreq and interconnect frameworks is achieved through
> the PM QoS API.
> 
> A sample interconnect consumer for exynos-mixer is added in patches 5/6,
> 6/6, it is currently added only for exynos4412 and allows to address the
> mixer DMA underrun error issues [1].
> 
> Changes since v6:
>  - the interconnect consumer DT bindings are now used to describe dependencies
>    of the interconnects (samsung,exynos-bus nodes),
>  - bus-width property replaced with samsung,data-clk-ratio,
>  - adaptation to recent changes in the interconnect code
>    (of_icc_get_from_provider(), icc_node_add()).
> 
> The series has been tested on Odroid U3 board. It is based on v5.10-rc1.
> 
> --
> Regards,
> Sylwester
> 
> 
> Changes since v5:
>  - addition of "bus-width: DT property, which specifies data width
>    of the interconnect bus (patches 1...2/6),
>  - addition of synchronization of the interconnect bandwidth setting
>    with VSYNC (patch 6/6).
> 
> Changes since v3 [4] (v4 skipped to align with patchset [1]), detailed
> changes are listed in each patch:
>  - conversion to a separate interconnect (platform) driver,
>  - an update of the DT binding documenting new optional properties:
>    #interconnect-cells, samsung,interconnect-parent in "samsung,exynos-bus"
>    nodes,
>  - new DT properties added to the SoC, rather than to the board specific
>    files.
> 
> Changes since v2 [5]:
>  - Use icc_std_aggregate().
>  - Implement a different modification of apply_constraints() in
>    drivers/interconnect/core.c (patch 03).
>  - Use 'exynos,interconnect-parent-node' in the DT instead of
>    'devfreq'/'parent', depending on the bus.
>  - Rebase on DT patches that deprecate the 'devfreq' DT property.
>  - Improve error handling, including freeing generated IDs on failure.
>  - Remove exynos_bus_icc_connect() and add exynos_bus_icc_get_parent().
> 
> Changes since v1 [6]:
>  - Rebase on coupled regulators patches.
>  - Use dev_pm_qos_*() API instead of overriding frequency in
>    exynos_bus_target().
>  - Use IDR for node ID allocation.
>  - Reverse order of multiplication and division in
>    mixer_set_memory_bandwidth() (patch 07) to avoid integer overflow.
> 
> 
> References:
> [1] https://patchwork.kernel.org/patch/10861757/ (original issue)
> [2] https://protect2.fireeye.com/v1/url?k=383efc40-67a5c559-383f770f-000babff3793-a505fcd0b7477e5e&q=1&e=ad8ffb9f-f90b-49a7-a3df-2ab066a8c4ee&u=https%3A%2F%2Fwww.spinics.net%2Flists%2Flinux-samsung-soc%2Fmsg70014.html
> [3] https://protect2.fireeye.com/v1/url?k=13f0c488-4c6bfd91-13f14fc7-000babff3793-98a59bf1c5c6f1fb&q=1&e=ad8ffb9f-f90b-49a7-a3df-2ab066a8c4ee&u=https%3A%2F%2Fwww.spinics.net%2Flists%2Farm-kernel%2Fmsg810722.html
> [4] https://lore.kernel.org/linux-pm/20191220115653.6487-1-a.swigon@samsung.com
> [5] https://patchwork.kernel.org/cover/11054417/ (v1 of this RFC)
> [6] https://patchwork.kernel.org/cover/11152595/ (v2 of this RFC)
> 
> 
> Artur Świgoń (1):
>   ARM: dts: exynos: Add interconnects to Exynos4412 mixer
> 
> Sylwester Nawrocki (5):
>   dt-bindings: devfreq: Add documentation for the interconnect
>     properties
>   interconnect: Add generic interconnect driver for Exynos SoCs
>   PM / devfreq: exynos-bus: Add registration of interconnect child
>     device
>   ARM: dts: exynos: Add interconnect properties to Exynos4412 bus nodes
>   drm: exynos: mixer: Add interconnect support
> 
>  .../devicetree/bindings/devfreq/exynos-bus.txt     |  68 ++++++-
>  arch/arm/boot/dts/exynos4412.dtsi                  |   7 +
>  drivers/devfreq/exynos-bus.c                       |  17 ++
>  drivers/gpu/drm/exynos/exynos_mixer.c              | 146 ++++++++++++++-
>  drivers/interconnect/Kconfig                       |   1 +
>  drivers/interconnect/Makefile                      |   1 +
>  drivers/interconnect/exynos/Kconfig                |   6 +
>  drivers/interconnect/exynos/Makefile               |   4 +
>  drivers/interconnect/exynos/exynos.c               | 198 +++++++++++++++++++++
>  9 files changed, 438 insertions(+), 10 deletions(-)
>  create mode 100644 drivers/interconnect/exynos/Kconfig
>  create mode 100644 drivers/interconnect/exynos/Makefile
>  create mode 100644 drivers/interconnect/exynos/exynos.c
> 
> --
> 2.7.4
> 
> 
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

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

[Chanwoo Choi]

Hi Sylwester,

When I tested this patchset on Odroid-U3,
After setting 0 bps by interconnect[1][2],
the frequency of devfreq devs sustain the high frequency
according to the pm qos request.

So, I try to find the cause of this situation.
In result, it seems that interconnect exynos driver
updates the pm qos request to devfreq device
during the kernel booting. Do you know why the exynos
interconnect driver request the pm qos during probe
without the mixer request?

PS. The passive governor has a bug related to PM_QOS interface.
So, I posted the patch[4].


[1] interconnect_graph
root@localhost:~# cat /sys/kernel/debug/interconnect/interconnect_graph 
digraph {
        rankdir = LR
        node [shape = record]
        subgraph cluster_1 {
                label = "soc:bus_dmc"
                "2:bus_dmc" [label="2:bus_dmc
                        |avg_bw=0kBps
                        |peak_bw=0kBps"]
        }
        subgraph cluster_2 {
                label = "soc:bus_leftbus"
                "3:bus_leftbus" [label="3:bus_leftbus
                        |avg_bw=0kBps
                        |peak_bw=0kBps"]
        }
        subgraph cluster_3 {
                label = "soc:bus_display"
                "4:bus_display" [label="4:bus_display
                        |avg_bw=0kBps
                        |peak_bw=0kBps"]
        }
        "3:bus_leftbus" -> "2:bus_dmc"
        "4:bus_display" -> "3:bus_leftbus"


[2] interconnect_summary
root@localhost:~# cat /sys/kernel/debug/interconnect/interconnect_summary 
 node                                  tag          avg         peak
--------------------------------------------------------------------
bus_dmc                                               0            0
  12c10000.mixer                         0            0            0
bus_leftbus                                           0            0
  12c10000.mixer                         0            0            0
bus_display                                           0            0
  12c10000.mixer                         0            0            0


[3] devfreq_summary
root@localhost:~# cat /sys/kernel/debug/devfreq/devfreq_summary 
dev                            parent_dev                     governor        timer      polling_ms  cur_freq_Hz  min_freq_Hz  max_freq_Hz
------------------------------ ------------------------------ --------------- ---------- ---------- ------------ ------------ ------------
soc:bus_dmc                    null                           simple_ondemand deferrable         50    400000000    400000000    400000000
soc:bus_acp                    soc:bus_dmc                    passive         null                0    267000000    100000000    267000000
soc:bus_c2c                    soc:bus_dmc                    passive         null                0    400000000    100000000    400000000
soc:bus_leftbus                null                           simple_ondemand deferrable         50    200000000    200000000    200000000
soc:bus_rightbus               soc:bus_leftbus                passive         null                0    200000000    100000000    200000000
soc:bus_display                soc:bus_leftbus                passive         null                0    200000000    200000000    200000000
soc:bus_fsys                   soc:bus_leftbus                passive         null                0    134000000    100000000    134000000
soc:bus_peri                   soc:bus_leftbus                passive         null                0    100000000     50000000    100000000
soc:bus_mfc                    soc:bus_leftbus                passive         null                0    200000000    100000000    200000000


[4] PM / devfreq: passive: Update frequency when start governor
https://patchwork.kernel.org/project/linux-pm/patch/20201103070646.18687-1-cw00.choi@samsung.com/


On 10/30/20 9:51 PM, 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 aallows to avoid issues like the one discussed
> in thread [1].
> 
> This patch series adds implementation of the interconnect provider per each
> "samsung,exynos-bus" compatible DT node, with one interconnect node per
> provider.  The interconnect code which was previously added as a part of
> the devfreq driver has been converted to a separate platform driver.
> In the devfreq a corresponding virtual child platform device is registered.
> Integration of devfreq and interconnect frameworks is achieved through
> the PM QoS API.
> 
> A sample interconnect consumer for exynos-mixer is added in patches 5/6,
> 6/6, it is currently added only for exynos4412 and allows to address the
> mixer DMA underrun error issues [1].
> 
> Changes since v6:
>  - the interconnect consumer DT bindings are now used to describe dependencies
>    of the interconnects (samsung,exynos-bus nodes),
>  - bus-width property replaced with samsung,data-clk-ratio,
>  - adaptation to recent changes in the interconnect code
>    (of_icc_get_from_provider(), icc_node_add()).
> 
> The series has been tested on Odroid U3 board. It is based on v5.10-rc1.
> 
> --
> Regards,
> Sylwester
> 
> 
> Changes since v5:
>  - addition of "bus-width: DT property, which specifies data width
>    of the interconnect bus (patches 1...2/6),
>  - addition of synchronization of the interconnect bandwidth setting
>    with VSYNC (patch 6/6).
> 
> Changes since v3 [4] (v4 skipped to align with patchset [1]), detailed
> changes are listed in each patch:
>  - conversion to a separate interconnect (platform) driver,
>  - an update of the DT binding documenting new optional properties:
>    #interconnect-cells, samsung,interconnect-parent in "samsung,exynos-bus"
>    nodes,
>  - new DT properties added to the SoC, rather than to the board specific
>    files.
> 
> Changes since v2 [5]:
>  - Use icc_std_aggregate().
>  - Implement a different modification of apply_constraints() in
>    drivers/interconnect/core.c (patch 03).
>  - Use 'exynos,interconnect-parent-node' in the DT instead of
>    'devfreq'/'parent', depending on the bus.
>  - Rebase on DT patches that deprecate the 'devfreq' DT property.
>  - Improve error handling, including freeing generated IDs on failure.
>  - Remove exynos_bus_icc_connect() and add exynos_bus_icc_get_parent().
> 
> Changes since v1 [6]:
>  - Rebase on coupled regulators patches.
>  - Use dev_pm_qos_*() API instead of overriding frequency in
>    exynos_bus_target().
>  - Use IDR for node ID allocation.
>  - Reverse order of multiplication and division in
>    mixer_set_memory_bandwidth() (patch 07) to avoid integer overflow.
> 
> 
> References:
> [1] https://patchwork.kernel.org/patch/10861757/ (original issue)
> [2] https://protect2.fireeye.com/v1/url?k=383efc40-67a5c559-383f770f-000babff3793-a505fcd0b7477e5e&q=1&e=ad8ffb9f-f90b-49a7-a3df-2ab066a8c4ee&u=https%3A%2F%2Fwww.spinics.net%2Flists%2Flinux-samsung-soc%2Fmsg70014.html
> [3] https://protect2.fireeye.com/v1/url?k=13f0c488-4c6bfd91-13f14fc7-000babff3793-98a59bf1c5c6f1fb&q=1&e=ad8ffb9f-f90b-49a7-a3df-2ab066a8c4ee&u=https%3A%2F%2Fwww.spinics.net%2Flists%2Farm-kernel%2Fmsg810722.html
> [4] https://lore.kernel.org/linux-pm/20191220115653.6487-1-a.swigon@samsung.com
> [5] https://patchwork.kernel.org/cover/11054417/ (v1 of this RFC)
> [6] https://patchwork.kernel.org/cover/11152595/ (v2 of this RFC)
> 
> 
> Artur Świgoń (1):
>   ARM: dts: exynos: Add interconnects to Exynos4412 mixer
> 
> Sylwester Nawrocki (5):
>   dt-bindings: devfreq: Add documentation for the interconnect
>     properties
>   interconnect: Add generic interconnect driver for Exynos SoCs
>   PM / devfreq: exynos-bus: Add registration of interconnect child
>     device
>   ARM: dts: exynos: Add interconnect properties to Exynos4412 bus nodes
>   drm: exynos: mixer: Add interconnect support
> 
>  .../devicetree/bindings/devfreq/exynos-bus.txt     |  68 ++++++-
>  arch/arm/boot/dts/exynos4412.dtsi                  |   7 +
>  drivers/devfreq/exynos-bus.c                       |  17 ++
>  drivers/gpu/drm/exynos/exynos_mixer.c              | 146 ++++++++++++++-
>  drivers/interconnect/Kconfig                       |   1 +
>  drivers/interconnect/Makefile                      |   1 +
>  drivers/interconnect/exynos/Kconfig                |   6 +
>  drivers/interconnect/exynos/Makefile               |   4 +
>  drivers/interconnect/exynos/exynos.c               | 198 +++++++++++++++++++++
>  9 files changed, 438 insertions(+), 10 deletions(-)
>  create mode 100644 drivers/interconnect/exynos/Kconfig
>  create mode 100644 drivers/interconnect/exynos/Makefile
>  create mode 100644 drivers/interconnect/exynos/exynos.c
> 
> --
> 2.7.4
> 
> 
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Georgi Djakov]

Hi Chanwoo and Sylwester,

On 11/3/20 09:54, Chanwoo Choi wrote:
> Hi Sylwester,
> 
> When I tested this patchset on Odroid-U3,
> After setting 0 bps by interconnect[1][2],
> the frequency of devfreq devs sustain the high frequency
> according to the pm qos request.
> 
> So, I try to find the cause of this situation.
> In result, it seems that interconnect exynos driver
> updates the pm qos request to devfreq device
> during the kernel booting. Do you know why the exynos
> interconnect driver request the pm qos during probe
> without the mixer request?

That's probably because of the sync_state support, that was introduced
recently. The icc_sync_state callback needs to be added to the driver
(i just left a comment on that patch), and then check again if it works.

The idea of the sync_state is that there could be multiple users of a
path and we must wait for all consumers to tell their bandwidth needs.
Otherwise the first consumer may lower the bandwidth or disable a path
needed for another consumer (driver), which has not probed yet. So we
maintain a floor bandwidth until everyone has probed. By default the floor
bandwidth is INT_MAX, but can be overridden by implementing the get_bw()
callback.

Thanks,
Georgi

> 
> PS. The passive governor has a bug related to PM_QOS interface.
> So, I posted the patch[4].
> 
> 
> [1] interconnect_graph
> root@localhost:~# cat /sys/kernel/debug/interconnect/interconnect_graph 
> digraph {
>         rankdir = LR
>         node [shape = record]
>         subgraph cluster_1 {
>                 label = "soc:bus_dmc"
>                 "2:bus_dmc" [label="2:bus_dmc
>                         |avg_bw=0kBps
>                         |peak_bw=0kBps"]
>         }
>         subgraph cluster_2 {
>                 label = "soc:bus_leftbus"
>                 "3:bus_leftbus" [label="3:bus_leftbus
>                         |avg_bw=0kBps
>                         |peak_bw=0kBps"]
>         }
>         subgraph cluster_3 {
>                 label = "soc:bus_display"
>                 "4:bus_display" [label="4:bus_display
>                         |avg_bw=0kBps
>                         |peak_bw=0kBps"]
>         }
>         "3:bus_leftbus" -> "2:bus_dmc"
>         "4:bus_display" -> "3:bus_leftbus"
> 
> 
> [2] interconnect_summary
> root@localhost:~# cat /sys/kernel/debug/interconnect/interconnect_summary 
>  node                                  tag          avg         peak
> --------------------------------------------------------------------
> bus_dmc                                               0            0
>   12c10000.mixer                         0            0            0
> bus_leftbus                                           0            0
>   12c10000.mixer                         0            0            0
> bus_display                                           0            0
>   12c10000.mixer                         0            0            0
> 
> 
> [3] devfreq_summary
> root@localhost:~# cat /sys/kernel/debug/devfreq/devfreq_summary 
> dev                            parent_dev                     governor        timer      polling_ms  cur_freq_Hz  min_freq_Hz  max_freq_Hz
> ------------------------------ ------------------------------ --------------- ---------- ---------- ------------ ------------ ------------
> soc:bus_dmc                    null                           simple_ondemand deferrable         50    400000000    400000000    400000000
> soc:bus_acp                    soc:bus_dmc                    passive         null                0    267000000    100000000    267000000
> soc:bus_c2c                    soc:bus_dmc                    passive         null                0    400000000    100000000    400000000
> soc:bus_leftbus                null                           simple_ondemand deferrable         50    200000000    200000000    200000000
> soc:bus_rightbus               soc:bus_leftbus                passive         null                0    200000000    100000000    200000000
> soc:bus_display                soc:bus_leftbus                passive         null                0    200000000    200000000    200000000
> soc:bus_fsys                   soc:bus_leftbus                passive         null                0    134000000    100000000    134000000
> soc:bus_peri                   soc:bus_leftbus                passive         null                0    100000000     50000000    100000000
> soc:bus_mfc                    soc:bus_leftbus                passive         null                0    200000000    100000000    200000000
> 
> 
> [4] PM / devfreq: passive: Update frequency when start governor
> https://patchwork.kernel.org/project/linux-pm/patch/20201103070646.18687-1-cw00.choi@samsung.com/
> 
> 

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

[Georgi Djakov]

Hi Chanwoo and Sylwester,

On 11/3/20 09:54, Chanwoo Choi wrote:
> Hi Sylwester,
> 
> When I tested this patchset on Odroid-U3,
> After setting 0 bps by interconnect[1][2],
> the frequency of devfreq devs sustain the high frequency
> according to the pm qos request.
> 
> So, I try to find the cause of this situation.
> In result, it seems that interconnect exynos driver
> updates the pm qos request to devfreq device
> during the kernel booting. Do you know why the exynos
> interconnect driver request the pm qos during probe
> without the mixer request?

That's probably because of the sync_state support, that was introduced
recently. The icc_sync_state callback needs to be added to the driver
(i just left a comment on that patch), and then check again if it works.

The idea of the sync_state is that there could be multiple users of a
path and we must wait for all consumers to tell their bandwidth needs.
Otherwise the first consumer may lower the bandwidth or disable a path
needed for another consumer (driver), which has not probed yet. So we
maintain a floor bandwidth until everyone has probed. By default the floor
bandwidth is INT_MAX, but can be overridden by implementing the get_bw()
callback.

Thanks,
Georgi

> 
> PS. The passive governor has a bug related to PM_QOS interface.
> So, I posted the patch[4].
> 
> 
> [1] interconnect_graph
> root@localhost:~# cat /sys/kernel/debug/interconnect/interconnect_graph 
> digraph {
>         rankdir = LR
>         node [shape = record]
>         subgraph cluster_1 {
>                 label = "soc:bus_dmc"
>                 "2:bus_dmc" [label="2:bus_dmc
>                         |avg_bw=0kBps
>                         |peak_bw=0kBps"]
>         }
>         subgraph cluster_2 {
>                 label = "soc:bus_leftbus"
>                 "3:bus_leftbus" [label="3:bus_leftbus
>                         |avg_bw=0kBps
>                         |peak_bw=0kBps"]
>         }
>         subgraph cluster_3 {
>                 label = "soc:bus_display"
>                 "4:bus_display" [label="4:bus_display
>                         |avg_bw=0kBps
>                         |peak_bw=0kBps"]
>         }
>         "3:bus_leftbus" -> "2:bus_dmc"
>         "4:bus_display" -> "3:bus_leftbus"
> 
> 
> [2] interconnect_summary
> root@localhost:~# cat /sys/kernel/debug/interconnect/interconnect_summary 
>  node                                  tag          avg         peak
> --------------------------------------------------------------------
> bus_dmc                                               0            0
>   12c10000.mixer                         0            0            0
> bus_leftbus                                           0            0
>   12c10000.mixer                         0            0            0
> bus_display                                           0            0
>   12c10000.mixer                         0            0            0
> 
> 
> [3] devfreq_summary
> root@localhost:~# cat /sys/kernel/debug/devfreq/devfreq_summary 
> dev                            parent_dev                     governor        timer      polling_ms  cur_freq_Hz  min_freq_Hz  max_freq_Hz
> ------------------------------ ------------------------------ --------------- ---------- ---------- ------------ ------------ ------------
> soc:bus_dmc                    null                           simple_ondemand deferrable         50    400000000    400000000    400000000
> soc:bus_acp                    soc:bus_dmc                    passive         null                0    267000000    100000000    267000000
> soc:bus_c2c                    soc:bus_dmc                    passive         null                0    400000000    100000000    400000000
> soc:bus_leftbus                null                           simple_ondemand deferrable         50    200000000    200000000    200000000
> soc:bus_rightbus               soc:bus_leftbus                passive         null                0    200000000    100000000    200000000
> soc:bus_display                soc:bus_leftbus                passive         null                0    200000000    200000000    200000000
> soc:bus_fsys                   soc:bus_leftbus                passive         null                0    134000000    100000000    134000000
> soc:bus_peri                   soc:bus_leftbus                passive         null                0    100000000     50000000    100000000
> soc:bus_mfc                    soc:bus_leftbus                passive         null                0    200000000    100000000    200000000
> 
> 
> [4] PM / devfreq: passive: Update frequency when start governor
> https://patchwork.kernel.org/project/linux-pm/patch/20201103070646.18687-1-cw00.choi@samsung.com/
> 
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Chanwoo Choi]

Hi Georgi,

On 11/3/20 5:29 PM, Georgi Djakov wrote:
> Hi Chanwoo and Sylwester,
> 
> On 11/3/20 09:54, Chanwoo Choi wrote:
>> Hi Sylwester,
>>
>> When I tested this patchset on Odroid-U3,
>> After setting 0 bps by interconnect[1][2],
>> the frequency of devfreq devs sustain the high frequency
>> according to the pm qos request.
>>
>> So, I try to find the cause of this situation.
>> In result, it seems that interconnect exynos driver
>> updates the pm qos request to devfreq device
>> during the kernel booting. Do you know why the exynos
>> interconnect driver request the pm qos during probe
>> without the mixer request?
> 
> That's probably because of the sync_state support, that was introduced
> recently. The icc_sync_state callback needs to be added to the driver
> (i just left a comment on that patch), and then check again if it works.
> 
> The idea of the sync_state is that there could be multiple users of a
> path and we must wait for all consumers to tell their bandwidth needs.
> Otherwise the first consumer may lower the bandwidth or disable a path
> needed for another consumer (driver), which has not probed yet. So we
> maintain a floor bandwidth until everyone has probed. By default the floor
> bandwidth is INT_MAX, but can be overridden by implementing the get_bw()
> callback.

Thanks for guide. I tested it with your comment of patch2.
It is well working without problem as I mentioned previously.

I caught the reset operation of PM QoS requested from interconnect
on kernel log. In result, after completed the kernel booting,
there is no pm qos request if hdmi cable is not connected.

[Test Result]
1. Set 622080 Bps with HDMI cable

root@localhost:~# cat /sys/kernel/debug/devfreq/devfreq_summary;
dev                            parent_dev                     governor        timer      polling_ms  cur_freq_Hz  min_freq_Hz  max_freq_Hz
------------------------------ ------------------------------ --------------- ---------- ---------- ------------ ------------ ------------
soc:bus_dmc                    null                           simple_ondemand deferrable         50    400000000    155520000    400000000
soc:bus_acp                    soc:bus_dmc                    passive         null                0    267000000    100000000    267000000
soc:bus_c2c                    soc:bus_dmc                    passive         null                0    400000000    100000000    400000000
soc:bus_leftbus                null                           simple_ondemand deferrable         50    200000000    100000000    200000000
soc:bus_rightbus               soc:bus_leftbus                passive         null                0    200000000    100000000    200000000
soc:bus_display                soc:bus_leftbus                passive         null                0    200000000    160000000    200000000
soc:bus_fsys                   soc:bus_leftbus                passive         null                0    134000000    100000000    134000000
soc:bus_peri                   soc:bus_leftbus                passive         null                0    100000000     50000000    100000000
soc:bus_mfc                    soc:bus_leftbus                passive         null                0    200000000    100000000    200000000
root@localhost:~# cat /sys/kernel/debug/interconnect/interconnect_graph;
digraph {
        rankdir = LR
        node [shape = record]
        subgraph cluster_1 {
                label = "soc:bus_dmc"
                "2:bus_dmc" [label="2:bus_dmc
                        |avg_bw=622080kBps
                        |peak_bw=622080kBps"]
        }
        subgraph cluster_2 {
                label = "soc:bus_leftbus"
                "3:bus_leftbus" [label="3:bus_leftbus
                        |avg_bw=622080kBps
                        |peak_bw=622080kBps"]
        }
        subgraph cluster_3 {
                label = "soc:bus_display"
                "4:bus_display" [label="4:bus_display
                        |avg_bw=622080kBps
                        |peak_bw=622080kBps"]
        }
        "3:bus_leftbus" -> "2:bus_dmc"
        "4:bus_display" -> "3:bus_leftbus"
}root@localhost:~# cat /sys/kernel/debug/interconnect/interconnect_summary;
 node                                  tag          avg         peak
--------------------------------------------------------------------
bus_dmc                                          622080       622080
  12c10000.mixer                         0       622080       622080
bus_leftbus                                      622080       622080
  12c10000.mixer                         0       622080       622080
bus_display                                      622080       622080
  12c10000.mixer                         0       622080       622080



2. Set 0Bps without HDMI cable
root@localhost:~# cat /sys/kernel/debug/devfreq/devfreq_summary;
dev                            parent_dev                     governor        timer      polling_ms  cur_freq_Hz  min_freq_Hz  max_freq_Hz
------------------------------ ------------------------------ --------------- ---------- ---------- ------------ ------------ ------------
soc:bus_dmc                    null                           simple_ondemand deferrable         50    100000000    100000000    400000000
soc:bus_acp                    soc:bus_dmc                    passive         null                0    100000000    100000000    267000000
soc:bus_c2c                    soc:bus_dmc                    passive         null                0    100000000    100000000    400000000
soc:bus_leftbus                null                           simple_ondemand deferrable         50    100000000    100000000    200000000
soc:bus_rightbus               soc:bus_leftbus                passive         null                0    100000000    100000000    200000000
soc:bus_display                soc:bus_leftbus                passive         null                0    160000000    160000000    200000000
soc:bus_fsys                   soc:bus_leftbus                passive         null                0    100000000    100000000    134000000
soc:bus_peri                   soc:bus_leftbus                passive         null                0     50000000     50000000    100000000
soc:bus_mfc                    soc:bus_leftbus                passive         null                0    100000000    100000000    200000000
root@localhost:~# cat /sys/kernel/debug/interconnect/interconnect_graph;
digraph {
        rankdir = LR
        node [shape = record]
        subgraph cluster_1 {
                label = "soc:bus_dmc"
                "2:bus_dmc" [label="2:bus_dmc
                        |avg_bw=0kBps
                        |peak_bw=0kBps"]
        }
        subgraph cluster_2 {
                label = "soc:bus_leftbus"
                "3:bus_leftbus" [label="3:bus_leftbus
                        |avg_bw=0kBps
                        |peak_bw=0kBps"]
        }
        subgraph cluster_3 {
                label = "soc:bus_display"
                "4:bus_display" [label="4:bus_display
                        |avg_bw=0kBps
                        |peak_bw=0kBps"]
        }
        "3:bus_leftbus" -> "2:bus_dmc"
        "4:bus_display" -> "3:bus_leftbus"
}root@localhost:~# cat /sys/kernel/debug/interconnect/interconnect_summary;
 node                                  tag          avg         peak
--------------------------------------------------------------------
bus_dmc                                               0            0
  12c10000.mixer                         0            0            0
bus_leftbus                                           0            0
  12c10000.mixer                         0            0            0
bus_display                                           0            0
  12c10000.mixer                         0            0            0


Thanks,
Chanwoo Choi

> 
> Thanks,
> Georgi
> 
>>
>> PS. The passive governor has a bug related to PM_QOS interface.
>> So, I posted the patch[4].
>>
>>
>> [1] interconnect_graph
>> root@localhost:~# cat /sys/kernel/debug/interconnect/interconnect_graph 
>> digraph {
>>         rankdir = LR
>>         node [shape = record]
>>         subgraph cluster_1 {
>>                 label = "soc:bus_dmc"
>>                 "2:bus_dmc" [label="2:bus_dmc
>>                         |avg_bw=0kBps
>>                         |peak_bw=0kBps"]
>>         }
>>         subgraph cluster_2 {
>>                 label = "soc:bus_leftbus"
>>                 "3:bus_leftbus" [label="3:bus_leftbus
>>                         |avg_bw=0kBps
>>                         |peak_bw=0kBps"]
>>         }
>>         subgraph cluster_3 {
>>                 label = "soc:bus_display"
>>                 "4:bus_display" [label="4:bus_display
>>                         |avg_bw=0kBps
>>                         |peak_bw=0kBps"]
>>         }
>>         "3:bus_leftbus" -> "2:bus_dmc"
>>         "4:bus_display" -> "3:bus_leftbus"
>>
>>
>> [2] interconnect_summary
>> root@localhost:~# cat /sys/kernel/debug/interconnect/interconnect_summary 
>>  node                                  tag          avg         peak
>> --------------------------------------------------------------------
>> bus_dmc                                               0            0
>>   12c10000.mixer                         0            0            0
>> bus_leftbus                                           0            0
>>   12c10000.mixer                         0            0            0
>> bus_display                                           0            0
>>   12c10000.mixer                         0            0            0
>>
>>
>> [3] devfreq_summary
>> root@localhost:~# cat /sys/kernel/debug/devfreq/devfreq_summary 
>> dev                            parent_dev                     governor        timer      polling_ms  cur_freq_Hz  min_freq_Hz  max_freq_Hz
>> ------------------------------ ------------------------------ --------------- ---------- ---------- ------------ ------------ ------------
>> soc:bus_dmc                    null                           simple_ondemand deferrable         50    400000000    400000000    400000000
>> soc:bus_acp                    soc:bus_dmc                    passive         null                0    267000000    100000000    267000000
>> soc:bus_c2c                    soc:bus_dmc                    passive         null                0    400000000    100000000    400000000
>> soc:bus_leftbus                null                           simple_ondemand deferrable         50    200000000    200000000    200000000
>> soc:bus_rightbus               soc:bus_leftbus                passive         null                0    200000000    100000000    200000000
>> soc:bus_display                soc:bus_leftbus                passive         null                0    200000000    200000000    200000000
>> soc:bus_fsys                   soc:bus_leftbus                passive         null                0    134000000    100000000    134000000
>> soc:bus_peri                   soc:bus_leftbus                passive         null                0    100000000     50000000    100000000
>> soc:bus_mfc                    soc:bus_leftbus                passive         null                0    200000000    100000000    200000000
>>
>>
>> [4] PM / devfreq: passive: Update frequency when start governor
>> https://patchwork.kernel.org/project/linux-pm/patch/20201103070646.18687-1-cw00.choi@samsung.com/
>>
>>
> 
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

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

[Chanwoo Choi]

Hi Georgi,

On 11/3/20 5:29 PM, Georgi Djakov wrote:
> Hi Chanwoo and Sylwester,
> 
> On 11/3/20 09:54, Chanwoo Choi wrote:
>> Hi Sylwester,
>>
>> When I tested this patchset on Odroid-U3,
>> After setting 0 bps by interconnect[1][2],
>> the frequency of devfreq devs sustain the high frequency
>> according to the pm qos request.
>>
>> So, I try to find the cause of this situation.
>> In result, it seems that interconnect exynos driver
>> updates the pm qos request to devfreq device
>> during the kernel booting. Do you know why the exynos
>> interconnect driver request the pm qos during probe
>> without the mixer request?
> 
> That's probably because of the sync_state support, that was introduced
> recently. The icc_sync_state callback needs to be added to the driver
> (i just left a comment on that patch), and then check again if it works.
> 
> The idea of the sync_state is that there could be multiple users of a
> path and we must wait for all consumers to tell their bandwidth needs.
> Otherwise the first consumer may lower the bandwidth or disable a path
> needed for another consumer (driver), which has not probed yet. So we
> maintain a floor bandwidth until everyone has probed. By default the floor
> bandwidth is INT_MAX, but can be overridden by implementing the get_bw()
> callback.

Thanks for guide. I tested it with your comment of patch2.
It is well working without problem as I mentioned previously.

I caught the reset operation of PM QoS requested from interconnect
on kernel log. In result, after completed the kernel booting,
there is no pm qos request if hdmi cable is not connected.

[Test Result]
1. Set 622080 Bps with HDMI cable

root@localhost:~# cat /sys/kernel/debug/devfreq/devfreq_summary;
dev                            parent_dev                     governor        timer      polling_ms  cur_freq_Hz  min_freq_Hz  max_freq_Hz
------------------------------ ------------------------------ --------------- ---------- ---------- ------------ ------------ ------------
soc:bus_dmc                    null                           simple_ondemand deferrable         50    400000000    155520000    400000000
soc:bus_acp                    soc:bus_dmc                    passive         null                0    267000000    100000000    267000000
soc:bus_c2c                    soc:bus_dmc                    passive         null                0    400000000    100000000    400000000
soc:bus_leftbus                null                           simple_ondemand deferrable         50    200000000    100000000    200000000
soc:bus_rightbus               soc:bus_leftbus                passive         null                0    200000000    100000000    200000000
soc:bus_display                soc:bus_leftbus                passive         null                0    200000000    160000000    200000000
soc:bus_fsys                   soc:bus_leftbus                passive         null                0    134000000    100000000    134000000
soc:bus_peri                   soc:bus_leftbus                passive         null                0    100000000     50000000    100000000
soc:bus_mfc                    soc:bus_leftbus                passive         null                0    200000000    100000000    200000000
root@localhost:~# cat /sys/kernel/debug/interconnect/interconnect_graph;
digraph {
        rankdir = LR
        node [shape = record]
        subgraph cluster_1 {
                label = "soc:bus_dmc"
                "2:bus_dmc" [label="2:bus_dmc
                        |avg_bw=622080kBps
                        |peak_bw=622080kBps"]
        }
        subgraph cluster_2 {
                label = "soc:bus_leftbus"
                "3:bus_leftbus" [label="3:bus_leftbus
                        |avg_bw=622080kBps
                        |peak_bw=622080kBps"]
        }
        subgraph cluster_3 {
                label = "soc:bus_display"
                "4:bus_display" [label="4:bus_display
                        |avg_bw=622080kBps
                        |peak_bw=622080kBps"]
        }
        "3:bus_leftbus" -> "2:bus_dmc"
        "4:bus_display" -> "3:bus_leftbus"
}root@localhost:~# cat /sys/kernel/debug/interconnect/interconnect_summary;
 node                                  tag          avg         peak
--------------------------------------------------------------------
bus_dmc                                          622080       622080
  12c10000.mixer                         0       622080       622080
bus_leftbus                                      622080       622080
  12c10000.mixer                         0       622080       622080
bus_display                                      622080       622080
  12c10000.mixer                         0       622080       622080



2. Set 0Bps without HDMI cable
root@localhost:~# cat /sys/kernel/debug/devfreq/devfreq_summary;
dev                            parent_dev                     governor        timer      polling_ms  cur_freq_Hz  min_freq_Hz  max_freq_Hz
------------------------------ ------------------------------ --------------- ---------- ---------- ------------ ------------ ------------
soc:bus_dmc                    null                           simple_ondemand deferrable         50    100000000    100000000    400000000
soc:bus_acp                    soc:bus_dmc                    passive         null                0    100000000    100000000    267000000
soc:bus_c2c                    soc:bus_dmc                    passive         null                0    100000000    100000000    400000000
soc:bus_leftbus                null                           simple_ondemand deferrable         50    100000000    100000000    200000000
soc:bus_rightbus               soc:bus_leftbus                passive         null                0    100000000    100000000    200000000
soc:bus_display                soc:bus_leftbus                passive         null                0    160000000    160000000    200000000
soc:bus_fsys                   soc:bus_leftbus                passive         null                0    100000000    100000000    134000000
soc:bus_peri                   soc:bus_leftbus                passive         null                0     50000000     50000000    100000000
soc:bus_mfc                    soc:bus_leftbus                passive         null                0    100000000    100000000    200000000
root@localhost:~# cat /sys/kernel/debug/interconnect/interconnect_graph;
digraph {
        rankdir = LR
        node [shape = record]
        subgraph cluster_1 {
                label = "soc:bus_dmc"
                "2:bus_dmc" [label="2:bus_dmc
                        |avg_bw=0kBps
                        |peak_bw=0kBps"]
        }
        subgraph cluster_2 {
                label = "soc:bus_leftbus"
                "3:bus_leftbus" [label="3:bus_leftbus
                        |avg_bw=0kBps
                        |peak_bw=0kBps"]
        }
        subgraph cluster_3 {
                label = "soc:bus_display"
                "4:bus_display" [label="4:bus_display
                        |avg_bw=0kBps
                        |peak_bw=0kBps"]
        }
        "3:bus_leftbus" -> "2:bus_dmc"
        "4:bus_display" -> "3:bus_leftbus"
}root@localhost:~# cat /sys/kernel/debug/interconnect/interconnect_summary;
 node                                  tag          avg         peak
--------------------------------------------------------------------
bus_dmc                                               0            0
  12c10000.mixer                         0            0            0
bus_leftbus                                           0            0
  12c10000.mixer                         0            0            0
bus_display                                           0            0
  12c10000.mixer                         0            0            0


Thanks,
Chanwoo Choi

> 
> Thanks,
> Georgi
> 
>>
>> PS. The passive governor has a bug related to PM_QOS interface.
>> So, I posted the patch[4].
>>
>>
>> [1] interconnect_graph
>> root@localhost:~# cat /sys/kernel/debug/interconnect/interconnect_graph 
>> digraph {
>>         rankdir = LR
>>         node [shape = record]
>>         subgraph cluster_1 {
>>                 label = "soc:bus_dmc"
>>                 "2:bus_dmc" [label="2:bus_dmc
>>                         |avg_bw=0kBps
>>                         |peak_bw=0kBps"]
>>         }
>>         subgraph cluster_2 {
>>                 label = "soc:bus_leftbus"
>>                 "3:bus_leftbus" [label="3:bus_leftbus
>>                         |avg_bw=0kBps
>>                         |peak_bw=0kBps"]
>>         }
>>         subgraph cluster_3 {
>>                 label = "soc:bus_display"
>>                 "4:bus_display" [label="4:bus_display
>>                         |avg_bw=0kBps
>>                         |peak_bw=0kBps"]
>>         }
>>         "3:bus_leftbus" -> "2:bus_dmc"
>>         "4:bus_display" -> "3:bus_leftbus"
>>
>>
>> [2] interconnect_summary
>> root@localhost:~# cat /sys/kernel/debug/interconnect/interconnect_summary 
>>  node                                  tag          avg         peak
>> --------------------------------------------------------------------
>> bus_dmc                                               0            0
>>   12c10000.mixer                         0            0            0
>> bus_leftbus                                           0            0
>>   12c10000.mixer                         0            0            0
>> bus_display                                           0            0
>>   12c10000.mixer                         0            0            0
>>
>>
>> [3] devfreq_summary
>> root@localhost:~# cat /sys/kernel/debug/devfreq/devfreq_summary 
>> dev                            parent_dev                     governor        timer      polling_ms  cur_freq_Hz  min_freq_Hz  max_freq_Hz
>> ------------------------------ ------------------------------ --------------- ---------- ---------- ------------ ------------ ------------
>> soc:bus_dmc                    null                           simple_ondemand deferrable         50    400000000    400000000    400000000
>> soc:bus_acp                    soc:bus_dmc                    passive         null                0    267000000    100000000    267000000
>> soc:bus_c2c                    soc:bus_dmc                    passive         null                0    400000000    100000000    400000000
>> soc:bus_leftbus                null                           simple_ondemand deferrable         50    200000000    200000000    200000000
>> soc:bus_rightbus               soc:bus_leftbus                passive         null                0    200000000    100000000    200000000
>> soc:bus_display                soc:bus_leftbus                passive         null                0    200000000    200000000    200000000
>> soc:bus_fsys                   soc:bus_leftbus                passive         null                0    134000000    100000000    134000000
>> soc:bus_peri                   soc:bus_leftbus                passive         null                0    100000000     50000000    100000000
>> soc:bus_mfc                    soc:bus_leftbus                passive         null                0    200000000    100000000    200000000
>>
>>
>> [4] PM / devfreq: passive: Update frequency when start governor
>> https://patchwork.kernel.org/project/linux-pm/patch/20201103070646.18687-1-cw00.choi@samsung.com/
>>
>>
> 
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Sylwester Nawrocki]

Hi Chanwoo, Georgi

On 03.11.2020 09:53, Chanwoo Choi wrote:
> On 11/3/20 5:29 PM, Georgi Djakov wrote:
>> On 11/3/20 09:54, Chanwoo Choi wrote:

>>> When I tested this patchset on Odroid-U3,
>>> After setting 0 bps by interconnect[1][2],
>>> the frequency of devfreq devs sustain the high frequency
>>> according to the pm qos request.
>>>
>>> So, I try to find the cause of this situation.
>>> In result, it seems that interconnect exynos driver
>>> updates the pm qos request to devfreq device
>>> during the kernel booting. Do you know why the exynos
>>> interconnect driver request the pm qos during probe
>>> without the mixer request?
>>
>> That's probably because of the sync_state support, that was introduced
>> recently. The icc_sync_state callback needs to be added to the driver
>> (i just left a comment on that patch), and then check again if it works.
>>
>> The idea of the sync_state is that there could be multiple users of a
>> path and we must wait for all consumers to tell their bandwidth needs.
>> Otherwise the first consumer may lower the bandwidth or disable a path
>> needed for another consumer (driver), which has not probed yet. So we
>> maintain a floor bandwidth until everyone has probed. By default the floor
>> bandwidth is INT_MAX, but can be overridden by implementing the get_bw()
>> callback.

Thanks for detailed explanation Georgi.

> Thanks for guide. I tested it with your comment of patch2.
> It is well working without problem as I mentioned previously.
> 
> I caught the reset operation of PM QoS requested from interconnect
> on kernel log. In result, after completed the kernel booting,
> there is no pm qos request if hdmi cable is not connected.

Thanks for the bug report Chanwoo, it's related to the sync_state
feature as you guys already figured out.  I had to reorder some code 
in the interconnect driver probe() to avoid some issues, 
i.e. to register PM QoS request before icc_node_add() call but 
I forgot to check initial state of the bus frequencies.

I thought the get_bw implementation might be needed but the default
behaviour seems fine, the PM QoS derived bus frequencies will be 
clamped in the devfreq to valid OPP values.

Chanwoo, in order to set the bandwidth to 0 we could also just blank 
the display. Below are some of the commands I use for testing.

# blank display (disable the mixer entirely)
echo 4 > /sys/devices/platform/exynos-drm/graphics/fb0/blank

# unblank display
echo 0 > /sys/devices/platform/exynos-drm/graphics/fb0/blank

# modetest with 2 planes (higher bandwidth test)
./modetest -s 47:1920x1080 -P 45:1920x1080 -v

-- 
Regards,
Sylwester

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

[Sylwester Nawrocki]

Hi Chanwoo, Georgi

On 03.11.2020 09:53, Chanwoo Choi wrote:
> On 11/3/20 5:29 PM, Georgi Djakov wrote:
>> On 11/3/20 09:54, Chanwoo Choi wrote:

>>> When I tested this patchset on Odroid-U3,
>>> After setting 0 bps by interconnect[1][2],
>>> the frequency of devfreq devs sustain the high frequency
>>> according to the pm qos request.
>>>
>>> So, I try to find the cause of this situation.
>>> In result, it seems that interconnect exynos driver
>>> updates the pm qos request to devfreq device
>>> during the kernel booting. Do you know why the exynos
>>> interconnect driver request the pm qos during probe
>>> without the mixer request?
>>
>> That's probably because of the sync_state support, that was introduced
>> recently. The icc_sync_state callback needs to be added to the driver
>> (i just left a comment on that patch), and then check again if it works.
>>
>> The idea of the sync_state is that there could be multiple users of a
>> path and we must wait for all consumers to tell their bandwidth needs.
>> Otherwise the first consumer may lower the bandwidth or disable a path
>> needed for another consumer (driver), which has not probed yet. So we
>> maintain a floor bandwidth until everyone has probed. By default the floor
>> bandwidth is INT_MAX, but can be overridden by implementing the get_bw()
>> callback.

Thanks for detailed explanation Georgi.

> Thanks for guide. I tested it with your comment of patch2.
> It is well working without problem as I mentioned previously.
> 
> I caught the reset operation of PM QoS requested from interconnect
> on kernel log. In result, after completed the kernel booting,
> there is no pm qos request if hdmi cable is not connected.

Thanks for the bug report Chanwoo, it's related to the sync_state
feature as you guys already figured out.  I had to reorder some code 
in the interconnect driver probe() to avoid some issues, 
i.e. to register PM QoS request before icc_node_add() call but 
I forgot to check initial state of the bus frequencies.

I thought the get_bw implementation might be needed but the default
behaviour seems fine, the PM QoS derived bus frequencies will be 
clamped in the devfreq to valid OPP values.

Chanwoo, in order to set the bandwidth to 0 we could also just blank 
the display. Below are some of the commands I use for testing.

# blank display (disable the mixer entirely)
echo 4 > /sys/devices/platform/exynos-drm/graphics/fb0/blank

# unblank display
echo 0 > /sys/devices/platform/exynos-drm/graphics/fb0/blank

# modetest with 2 planes (higher bandwidth test)
./modetest -s 47:1920x1080 -P 45:1920x1080 -v

-- 
Regards,
Sylwester
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Chanwoo Choi]

Hi Sylwester,

On 11/3/20 7:12 PM, Sylwester Nawrocki wrote:
> Hi Chanwoo, Georgi
> 
> On 03.11.2020 09:53, Chanwoo Choi wrote:
>> On 11/3/20 5:29 PM, Georgi Djakov wrote:
>>> On 11/3/20 09:54, Chanwoo Choi wrote:
> 
>>>> When I tested this patchset on Odroid-U3,
>>>> After setting 0 bps by interconnect[1][2],
>>>> the frequency of devfreq devs sustain the high frequency
>>>> according to the pm qos request.
>>>>
>>>> So, I try to find the cause of this situation.
>>>> In result, it seems that interconnect exynos driver
>>>> updates the pm qos request to devfreq device
>>>> during the kernel booting. Do you know why the exynos
>>>> interconnect driver request the pm qos during probe
>>>> without the mixer request?
>>>
>>> That's probably because of the sync_state support, that was introduced
>>> recently. The icc_sync_state callback needs to be added to the driver
>>> (i just left a comment on that patch), and then check again if it works.
>>>
>>> The idea of the sync_state is that there could be multiple users of a
>>> path and we must wait for all consumers to tell their bandwidth needs.
>>> Otherwise the first consumer may lower the bandwidth or disable a path
>>> needed for another consumer (driver), which has not probed yet. So we
>>> maintain a floor bandwidth until everyone has probed. By default the floor
>>> bandwidth is INT_MAX, but can be overridden by implementing the get_bw()
>>> callback.
> 
> Thanks for detailed explanation Georgi.
> 
>> Thanks for guide. I tested it with your comment of patch2.
>> It is well working without problem as I mentioned previously.
>>
>> I caught the reset operation of PM QoS requested from interconnect
>> on kernel log. In result, after completed the kernel booting,
>> there is no pm qos request if hdmi cable is not connected.
> 
> Thanks for the bug report Chanwoo, it's related to the sync_state
> feature as you guys already figured out.  I had to reorder some code 
> in the interconnect driver probe() to avoid some issues, 
> i.e. to register PM QoS request before icc_node_add() call but 
> I forgot to check initial state of the bus frequencies.
> 
> I thought the get_bw implementation might be needed but the default
> behaviour seems fine, the PM QoS derived bus frequencies will be 
> clamped in the devfreq to valid OPP values.
> 
> Chanwoo, in order to set the bandwidth to 0 we could also just blank 
> the display. Below are some of the commands I use for testing.
> 
> # blank display (disable the mixer entirely)
> echo 4 > /sys/devices/platform/exynos-drm/graphics/fb0/blank
> 
> # unblank display
> echo 0 > /sys/devices/platform/exynos-drm/graphics/fb0/blank
> 
> # modetest with 2 planes (higher bandwidth test)
> ./modetest -s 47:1920x1080 -P 45:1920x1080 -v
> 

Thanks for the test guide.

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

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

[Chanwoo Choi]

Hi Sylwester,

On 11/3/20 7:12 PM, Sylwester Nawrocki wrote:
> Hi Chanwoo, Georgi
> 
> On 03.11.2020 09:53, Chanwoo Choi wrote:
>> On 11/3/20 5:29 PM, Georgi Djakov wrote:
>>> On 11/3/20 09:54, Chanwoo Choi wrote:
> 
>>>> When I tested this patchset on Odroid-U3,
>>>> After setting 0 bps by interconnect[1][2],
>>>> the frequency of devfreq devs sustain the high frequency
>>>> according to the pm qos request.
>>>>
>>>> So, I try to find the cause of this situation.
>>>> In result, it seems that interconnect exynos driver
>>>> updates the pm qos request to devfreq device
>>>> during the kernel booting. Do you know why the exynos
>>>> interconnect driver request the pm qos during probe
>>>> without the mixer request?
>>>
>>> That's probably because of the sync_state support, that was introduced
>>> recently. The icc_sync_state callback needs to be added to the driver
>>> (i just left a comment on that patch), and then check again if it works.
>>>
>>> The idea of the sync_state is that there could be multiple users of a
>>> path and we must wait for all consumers to tell their bandwidth needs.
>>> Otherwise the first consumer may lower the bandwidth or disable a path
>>> needed for another consumer (driver), which has not probed yet. So we
>>> maintain a floor bandwidth until everyone has probed. By default the floor
>>> bandwidth is INT_MAX, but can be overridden by implementing the get_bw()
>>> callback.
> 
> Thanks for detailed explanation Georgi.
> 
>> Thanks for guide. I tested it with your comment of patch2.
>> It is well working without problem as I mentioned previously.
>>
>> I caught the reset operation of PM QoS requested from interconnect
>> on kernel log. In result, after completed the kernel booting,
>> there is no pm qos request if hdmi cable is not connected.
> 
> Thanks for the bug report Chanwoo, it's related to the sync_state
> feature as you guys already figured out.  I had to reorder some code 
> in the interconnect driver probe() to avoid some issues, 
> i.e. to register PM QoS request before icc_node_add() call but 
> I forgot to check initial state of the bus frequencies.
> 
> I thought the get_bw implementation might be needed but the default
> behaviour seems fine, the PM QoS derived bus frequencies will be 
> clamped in the devfreq to valid OPP values.
> 
> Chanwoo, in order to set the bandwidth to 0 we could also just blank 
> the display. Below are some of the commands I use for testing.
> 
> # blank display (disable the mixer entirely)
> echo 4 > /sys/devices/platform/exynos-drm/graphics/fb0/blank
> 
> # unblank display
> echo 0 > /sys/devices/platform/exynos-drm/graphics/fb0/blank
> 
> # modetest with 2 planes (higher bandwidth test)
> ./modetest -s 47:1920x1080 -P 45:1920x1080 -v
> 

Thanks for the test guide.

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel