devicetree.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Re: [RFC PATCH v5 0/6] Exynos: Simple QoS for exynos-bus using interconnect
       [not found] ` <20200529163200.18031-1-s.nawrocki@samsung.com>
@ 2020-06-01  8:17   ` Sylwester Nawrocki
       [not found]   ` <CGME20200529163219eucas1p2d127fe3936921f53f6fe7902e7d14a3e@eucas1p2.samsung.com>
                     ` (2 subsequent siblings)
  3 siblings, 0 replies; 6+ messages in thread
From: Sylwester Nawrocki @ 2020-06-01  8:17 UTC (permalink / raw)
  To: georgi.djakov, cw00.choi, krzk
  Cc: a.swigon, myungjoo.ham, inki.dae, sw0312.kim, b.zolnierkie,
	m.szyprowski, linux-kernel, linux-samsung-soc, dri-devel,
	linux-arm-kernel, Rob Herring, devicetree

Cc: Rob, devicetree ML

On 29.05.2020 18:31, Sylwester Nawrocki wrote:
> This patchset adds interconnect API support for the Exynos SoC "samsung,
> exynos-bus" compatible devices, which already have their corresponding 
> exynos-bus driver in the devfreq subsystem.  Complementing the devfreq driver 
> with an interconnect functionality allows to ensure the QoS requirements 
> of devices accessing the system memory (e.g. video processing devices) 
> are fulfilled and to avoid issues like the one discussed in thread [1].
> 
> This patch series depends on 3 patches from Artur for the interconnect API 
> [2], which introduce following changes:
> 
>  - exporting of_icc_get_from_provider() to avoid hard coding every graph 
>    edge in the DT or driver source,
>  - relaxing the requirement on #interconnect-cells, so there is no need 
>    to provide dummy node IDs in the DT, 
>  - adding new field in struct icc_provider to explicitly allow configuring 
>    node pairs from two different providers.
> 
> This series adds implementation of interconnect provider per each "samsung,
> exynos-bus" compatible DT node, with one interconnect node per provider.
> The interconnect code which was previously added as a part of the devfreq
> driver has been converted to a separate platform driver.  In the devfreq 
> a corresponding virtual child platform device is registered.  Integration 
> of devfreq and interconnect frameworks is achieved through the PM QoS API.
> 
> A sample interconnect consumer for exynos-mixer is added in patches 5/6, 
> 6/6, it is currently added only for exynos4412 and allows to address the 
> mixer DMA underrun error issues [1].
> 
> The series has been tested on Odroid U3 board. It is based on icc-next 
> branch with devfreq-next branch merged and patches [2] applied.
> 
> --
> Regards,
> Sylwester
> 
> --
> Changes since v3 [3] (v4 skipped to align with patchset [1]), detailed 
> changes are listed at each patch:
>  - conversion to a separate interconnect (platform) driver,
>  - an update of the DT binding documenting new optional properties:
>    #interconnect-cells, samsung,interconnect-parent in "samsung,exynos-bus"
>    nodes,
>  - new DT properties added to the SoC, rather than to the board specific 
>    files.
> 
> Changes since v2 [5]:
>  - Use icc_std_aggregate().
>  - Implement a different modification of apply_constraints() in
>    drivers/interconnect/core.c (patch 03).
>  - Use 'exynos,interconnect-parent-node' in the DT instead of
>    'devfreq'/'parent', depending on the bus.
>  - Rebase on DT patches that deprecate the 'devfreq' DT property.
>  - Improve error handling, including freeing generated IDs on failure.
>  - Remove exynos_bus_icc_connect() and add exynos_bus_icc_get_parent().
> 
> Changes since v1 [4]:
>  - Rebase on coupled regulators patches.
>  - Use dev_pm_qos_*() API instead of overriding frequency in
>    exynos_bus_target().
>  - Use IDR for node ID allocation.
>  - Reverse order of multiplication and division in
>    mixer_set_memory_bandwidth() (patch 07) to avoid integer overflow.
> 
> 
> References:
> [1] https://patchwork.kernel.org/patch/10861757/ (original issue)
> [2] https://www.spinics.net/lists/linux-samsung-soc/msg70014.html
> [3] https://lore.kernel.org/linux-pm/20191220115653.6487-1-a.swigon@samsung.com
> [4] https://patchwork.kernel.org/cover/11054417/ (v1 of this RFC)
> [5] https://patchwork.kernel.org/cover/11152595/ (v2 of this RFC)
> 
> 
> Artur Świgoń (1):
>   ARM: dts: exynos: Add interconnects to Exynos4412 mixer
> 
> Marek Szyprowski (1):
>   drm: exynos: mixer: Add interconnect support
> 
> Sylwester Nawrocki (4):
>   dt-bindings: exynos-bus: Add documentation for interconnect properties
>   interconnect: Add generic interconnect driver for Exynos SoCs
>   PM / devfreq: exynos-bus: Add registration of interconnect child
>     device
>   ARM: dts: exynos: Add interconnect properties to Exynos4412 bus nodes
> 
>  .../devicetree/bindings/devfreq/exynos-bus.txt     |  15 +-
>  arch/arm/boot/dts/exynos4412.dtsi                  |   6 +
>  drivers/devfreq/exynos-bus.c                       |  17 ++
>  drivers/gpu/drm/exynos/exynos_mixer.c              |  73 +++++++-
>  drivers/interconnect/Kconfig                       |   1 +
>  drivers/interconnect/Makefile                      |   1 +
>  drivers/interconnect/exynos/Kconfig                |   6 +
>  drivers/interconnect/exynos/Makefile               |   4 +
>  drivers/interconnect/exynos/exynos.c               | 185 +++++++++++++++++++++
>  9 files changed, 301 insertions(+), 7 deletions(-)
>  create mode 100644 drivers/interconnect/exynos/Kconfig
>  create mode 100644 drivers/interconnect/exynos/Makefile
>  create mode 100644 drivers/interconnect/exynos/exynos.c


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [RFC PATCH v5 1/6] dt-bindings: exynos-bus: Add documentation for interconnect properties
       [not found]     ` <20200529163200.18031-2-s.nawrocki@samsung.com>
@ 2020-06-01  8:19       ` Sylwester Nawrocki
       [not found]       ` <CAGTfZH1yM0KRaEF5VTs2juTm+yrK9VqQZxWjdNf_ffjGHWPLsg@mail.gmail.com>
  1 sibling, 0 replies; 6+ messages in thread
From: Sylwester Nawrocki @ 2020-06-01  8:19 UTC (permalink / raw)
  To: georgi.djakov, cw00.choi, krzk
  Cc: a.swigon, myungjoo.ham, inki.dae, sw0312.kim, b.zolnierkie,
	m.szyprowski, linux-kernel, linux-samsung-soc, dri-devel,
	linux-arm-kernel, Rob Herring, devicetree

Cc: Rob, devicetree ML

On 29.05.2020 18:31, Sylwester Nawrocki wrote:
> Add documentation for new optional properties in the exynos bus nodes:
> samsung,interconnect-parent, #interconnect-cells.
> These properties allow to specify the SoC interconnect structure which
> then allows the interconnect consumer devices to request specific
> bandwidth requirements.
> 
> Signed-off-by: Artur Świgoń <a.swigon@samsung.com>
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> ---
> Changes for v5:
>  - exynos,interconnect-parent-node renamed to samsung,interconnect-parent
> ---
>  Documentation/devicetree/bindings/devfreq/exynos-bus.txt | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/devfreq/exynos-bus.txt b/Documentation/devicetree/bindings/devfreq/exynos-bus.txt
> index e71f752..e0d2daa 100644
> --- a/Documentation/devicetree/bindings/devfreq/exynos-bus.txt
> +++ b/Documentation/devicetree/bindings/devfreq/exynos-bus.txt
> @@ -51,6 +51,11 @@ Optional properties only for parent bus device:
>  - exynos,saturation-ratio: the percentage value which is used to calibrate
>  			the performance count against total cycle count.
> 
> +Optional properties for interconnect functionality (QoS frequency constraints):
> +- samsung,interconnect-parent: phandle to the parent interconnect node; for
> +  passive devices should point to same node as the exynos,parent-bus property.
> +- #interconnect-cells: should be 0
> +
>  Detailed correlation between sub-blocks and power line according to Exynos SoC:
>  - In case of Exynos3250, there are two power line as following:
>  	VDD_MIF |--- DMC
> @@ -185,8 +190,9 @@ Example1:
>  	----------------------------------------------------------
> 
>  Example2 :
> -	The bus of DMC (Dynamic Memory Controller) block in exynos3250.dtsi
> -	is listed below:
> +	The bus of DMC (Dynamic Memory Controller) block in exynos3250.dtsi is
> +	listed below. An interconnect path "bus_lcd0 -- bus_leftbus -- bus_dmc"
> +	is defined for demonstration purposes.
> 
>  	bus_dmc: bus_dmc {
>  		compatible = "samsung,exynos-bus";
> @@ -376,12 +382,15 @@ Example2 :
>  	&bus_dmc {
>  		devfreq-events = <&ppmu_dmc0_3>, <&ppmu_dmc1_3>;
>  		vdd-supply = <&buck1_reg>;	/* VDD_MIF */
> +		#interconnect-cells = <0>;
>  		status = "okay";
>  	};
> 
>  	&bus_leftbus {
>  		devfreq-events = <&ppmu_leftbus_3>, <&ppmu_rightbus_3>;
>  		vdd-supply = <&buck3_reg>;
> +		samsung,interconnect-parent = <&bus_dmc>;
> +		#interconnect-cells = <0>;
>  		status = "okay";
>  	};
> 
> @@ -392,6 +401,8 @@ Example2 :
> 
>  	&bus_lcd0 {
>  		devfreq = <&bus_leftbus>;
> +		samsung,interconnect-parent = <&bus_leftbus>;
> +		#interconnect-cells = <0>;
>  		status = "okay";
>  	};
> 
> --
> 2.7.4

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [RFC PATCH v5 1/6] dt-bindings: exynos-bus: Add documentation for interconnect properties
       [not found]       ` <CAGTfZH1yM0KRaEF5VTs2juTm+yrK9VqQZxWjdNf_ffjGHWPLsg@mail.gmail.com>
@ 2020-06-01  9:40         ` Sylwester Nawrocki
  0 siblings, 0 replies; 6+ messages in thread
From: Sylwester Nawrocki @ 2020-06-01  9:40 UTC (permalink / raw)
  To: Chanwoo Choi
  Cc: Georgi Djakov, Chanwoo Choi, Krzysztof Kozlowski,
	Artur Świgoń,
	MyungJoo Ham, inki.dae, Seung-Woo Kim, Bartlomiej Zolnierkiewicz,
	Marek Szyprowski, linux-kernel, linux-samsung-soc, dri-devel,
	linux-arm-kernel, Rob Herring, devicetree

Hi Chanwoo,

On 31.05.2020 02:01, Chanwoo Choi wrote:
> On Sat, May 30, 2020 at 1:32 AM Sylwester Nawrocki
> <s.nawrocki@samsung.com> wrote:
>>
>> Add documentation for new optional properties in the exynos bus nodes:
>> samsung,interconnect-parent, #interconnect-cells.
>> These properties allow to specify the SoC interconnect structure which
>> then allows the interconnect consumer devices to request specific
>> bandwidth requirements.
>>
>> Signed-off-by: Artur Świgoń <a.swigon@samsung.com>
>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
>> ---
>> Changes for v5:
>>  - exynos,interconnect-parent-node renamed to samsung,interconnect-parent
>> ---
>>  Documentation/devicetree/bindings/devfreq/exynos-bus.txt | 15 +++++++++++++--
>>  1 file changed, 13 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/devfreq/exynos-bus.txt b/Documentation/devicetree/bindings/devfreq/exynos-bus.txt
>> index e71f752..e0d2daa 100644
>> --- a/Documentation/devicetree/bindings/devfreq/exynos-bus.txt
>> +++ b/Documentation/devicetree/bindings/devfreq/exynos-bus.txt
>> @@ -51,6 +51,11 @@ Optional properties only for parent bus device:
>>  - exynos,saturation-ratio: the percentage value which is used to calibrate
>>                         the performance count against total cycle count.
>>
>> +Optional properties for interconnect functionality (QoS frequency constraints):
>> +- samsung,interconnect-parent: phandle to the parent interconnect node; for
>> +  passive devices should point to same node as the exynos,parent-bus property.
>> +- #interconnect-cells: should be 0
>> +
>>  Detailed correlation between sub-blocks and power line according to Exynos SoC:
>>  - In case of Exynos3250, there are two power line as following:
>>         VDD_MIF |--- DMC
>> @@ -185,8 +190,9 @@ Example1:
>>         ----------------------------------------------------------
>>
>>  Example2 :
>> -       The bus of DMC (Dynamic Memory Controller) block in exynos3250.dtsi
>> -       is listed below:
>> +       The bus of DMC (Dynamic Memory Controller) block in exynos3250.dtsi is
>> +       listed below. An interconnect path "bus_lcd0 -- bus_leftbus -- bus_dmc"
>> +       is defined for demonstration purposes.
>>
>>         bus_dmc: bus_dmc {
>>                 compatible = "samsung,exynos-bus";
>> @@ -376,12 +382,15 @@ Example2 :
>>         &bus_dmc {
>>                 devfreq-events = <&ppmu_dmc0_3>, <&ppmu_dmc1_3>;
>>                 vdd-supply = <&buck1_reg>;      /* VDD_MIF */
>> +               #interconnect-cells = <0>;
>>                 status = "okay";
>>         };
>>
>>         &bus_leftbus {
>>                 devfreq-events = <&ppmu_leftbus_3>, <&ppmu_rightbus_3>;
>>                 vdd-supply = <&buck3_reg>;
>> +               samsung,interconnect-parent = <&bus_dmc>;
>> +               #interconnect-cells = <0>;
>>                 status = "okay";
>>         };
>>
>> @@ -392,6 +401,8 @@ Example2 :
>>
>>         &bus_lcd0 {
>>                 devfreq = <&bus_leftbus>;
>> +               samsung,interconnect-parent = <&bus_leftbus>;
>> +               #interconnect-cells = <0>;
>>                 status = "okay";
>>         };
>>
>> --
>> 2.7.4
>>
> 
> If you add the usage example like the mixer device of patch5 to this
> dt-binding document,
> I think it is very beneficial and more helpful for user of
> exynos-bus/exynos-generic-icc.
> 
> Acked-by: Chanwoo Choi <cw00.choi@samsung.com>

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

-- 
Regards,
Sylwester

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [RFC PATCH v5 2/6] interconnect: Add generic interconnect driver for Exynos SoCs
       [not found]     ` <CAGTfZH3zvgOtME0U2hKjtqO49e0-Nw6MRxhw+z6Mfio-=FUwcQ@mail.gmail.com>
@ 2020-06-01  9:57       ` Sylwester Nawrocki
  0 siblings, 0 replies; 6+ messages in thread
From: Sylwester Nawrocki @ 2020-06-01  9:57 UTC (permalink / raw)
  To: Chanwoo Choi
  Cc: Georgi Djakov, Chanwoo Choi, Krzysztof Kozlowski,
	Artur Świgoń,
	MyungJoo Ham, inki.dae, Seung-Woo Kim, Bartlomiej Zolnierkiewicz,
	Marek Szyprowski, linux-kernel, linux-samsung-soc, dri-devel,
	linux-arm-kernel, Rob Herring, devicetree

Cc: Rob, devicetree ML

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

-- 
Thanks,
Sylwester

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [RFC PATCH v5 3/6] PM / devfreq: exynos-bus: Add registration of interconnect child device
       [not found]     ` <CAGTfZH1KC=jpQ5GXNtEf1cn7+WqXJdqbbVKmpxr8Snh4GEy8bA@mail.gmail.com>
@ 2020-06-01 10:04       ` Sylwester Nawrocki
  2020-06-02  0:50         ` Chanwoo Choi
  0 siblings, 1 reply; 6+ messages in thread
From: Sylwester Nawrocki @ 2020-06-01 10:04 UTC (permalink / raw)
  To: Chanwoo Choi
  Cc: Georgi Djakov, Chanwoo Choi, Krzysztof Kozlowski,
	Artur Świgoń,
	MyungJoo Ham, inki.dae, Seung-Woo Kim, Bartlomiej Zolnierkiewicz,
	Marek Szyprowski, linux-kernel, linux-samsung-soc, dri-devel,
	linux-arm-kernel, Rob Herring, devicetree

Cc: Rob, devicetree ML

On 31.05.2020 01:57, Chanwoo Choi wrote:
> On Sat, May 30, 2020 at 1:33 AM Sylwester Nawrocki
> <s.nawrocki@samsung.com> wrote:
>>
>> This patch adds registration of a child platform device for the exynos
>> interconnect driver. It is assumed that the interconnect provider will
>> only be needed when #interconnect-cells property is present in the bus
>> DT node, hence the child device will be created only when such a property
>> is present.
>>
>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
>>
>> Changes for v5:
>>  - new patch.
>> ---
>>  drivers/devfreq/exynos-bus.c | 17 +++++++++++++++++
>>  1 file changed, 17 insertions(+)
>>
>> diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
>> index 8fa8eb5..856e37d 100644
>> --- a/drivers/devfreq/exynos-bus.c
>> +++ b/drivers/devfreq/exynos-bus.c
>> @@ -24,6 +24,7 @@
>>
>>  struct exynos_bus {
>>         struct device *dev;
>> +       struct platform_device *icc_pdev;
>>
>>         struct devfreq *devfreq;
>>         struct devfreq_event_dev **edev;
>> @@ -156,6 +157,8 @@ static void exynos_bus_exit(struct device *dev)
>>         if (ret < 0)
>>                 dev_warn(dev, "failed to disable the devfreq-event devices\n");
>>
>> +       platform_device_unregister(bus->icc_pdev);
>> +
>>         dev_pm_opp_of_remove_table(dev);
>>         clk_disable_unprepare(bus->clk);
>>         if (bus->opp_table) {
>> @@ -168,6 +171,8 @@ static void exynos_bus_passive_exit(struct device *dev)
>>  {
>>         struct exynos_bus *bus = dev_get_drvdata(dev);
>>
>> +       platform_device_unregister(bus->icc_pdev);
>> +
>>         dev_pm_opp_of_remove_table(dev);
>>         clk_disable_unprepare(bus->clk);
>>  }
>> @@ -431,6 +436,18 @@ static int exynos_bus_probe(struct platform_device *pdev)
>>         if (ret < 0)
>>                 goto err;
>>
>> +       /* Create child platform device for the interconnect provider */
>> +       if (of_get_property(dev->of_node, "#interconnect-cells", NULL)) {
>> +                   bus->icc_pdev = platform_device_register_data(
>> +                                               dev, "exynos-generic-icc",
>> +                                               PLATFORM_DEVID_AUTO, NULL, 0);
>> +
>> +                   if (IS_ERR(bus->icc_pdev)) {
>> +                           ret = PTR_ERR(bus->icc_pdev);
>> +                           goto err;
>> +                   }
>> +       }
>> +
>>         max_state = bus->devfreq->profile->max_state;
>>         min_freq = (bus->devfreq->profile->freq_table[0] / 1000);
>>         max_freq = (bus->devfreq->profile->freq_table[max_state - 1] / 1000);
>> --
>> 2.7.4
>>
> 
> It looks like very similar like the registering the interconnect
> device of imx-bus.c
> and I already reviewed and agreed this approach.
> 
> Acked-by: Chanwoo Choi <cw00.choi@samsung.com>
> 
> nitpick: IMHO, I think that 'exynos-icc' is proper and simple without
> 'generic' word.
> If we need to add new icc compatible int the future, we will add
> 'exynosXXXX-icc' new compatible.
> But, I'm not forcing it. just opinion. Anyway, I agree this approach.

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


--
Thanks, 
Sylwester

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [RFC PATCH v5 3/6] PM / devfreq: exynos-bus: Add registration of interconnect child device
  2020-06-01 10:04       ` [RFC PATCH v5 3/6] PM / devfreq: exynos-bus: Add registration of interconnect child device Sylwester Nawrocki
@ 2020-06-02  0:50         ` Chanwoo Choi
  0 siblings, 0 replies; 6+ messages in thread
From: Chanwoo Choi @ 2020-06-02  0:50 UTC (permalink / raw)
  To: Sylwester Nawrocki, Chanwoo Choi
  Cc: Georgi Djakov, Krzysztof Kozlowski, Artur Świgoń,
	MyungJoo Ham, inki.dae, Seung-Woo Kim, Bartlomiej Zolnierkiewicz,
	Marek Szyprowski, linux-kernel, linux-samsung-soc, dri-devel,
	linux-arm-kernel, Rob Herring, devicetree

Hi Sylwester,

On 6/1/20 7:04 PM, Sylwester Nawrocki wrote:
> Cc: Rob, devicetree ML
> 
> On 31.05.2020 01:57, Chanwoo Choi wrote:
>> On Sat, May 30, 2020 at 1:33 AM Sylwester Nawrocki
>> <s.nawrocki@samsung.com> wrote:
>>>
>>> This patch adds registration of a child platform device for the exynos
>>> interconnect driver. It is assumed that the interconnect provider will
>>> only be needed when #interconnect-cells property is present in the bus
>>> DT node, hence the child device will be created only when such a property
>>> is present.
>>>
>>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
>>>
>>> Changes for v5:
>>>  - new patch.
>>> ---
>>>  drivers/devfreq/exynos-bus.c | 17 +++++++++++++++++
>>>  1 file changed, 17 insertions(+)
>>>
>>> diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
>>> index 8fa8eb5..856e37d 100644
>>> --- a/drivers/devfreq/exynos-bus.c
>>> +++ b/drivers/devfreq/exynos-bus.c
>>> @@ -24,6 +24,7 @@
>>>
>>>  struct exynos_bus {
>>>         struct device *dev;
>>> +       struct platform_device *icc_pdev;
>>>
>>>         struct devfreq *devfreq;
>>>         struct devfreq_event_dev **edev;
>>> @@ -156,6 +157,8 @@ static void exynos_bus_exit(struct device *dev)
>>>         if (ret < 0)
>>>                 dev_warn(dev, "failed to disable the devfreq-event devices\n");
>>>
>>> +       platform_device_unregister(bus->icc_pdev);
>>> +
>>>         dev_pm_opp_of_remove_table(dev);
>>>         clk_disable_unprepare(bus->clk);
>>>         if (bus->opp_table) {
>>> @@ -168,6 +171,8 @@ static void exynos_bus_passive_exit(struct device *dev)
>>>  {
>>>         struct exynos_bus *bus = dev_get_drvdata(dev);
>>>
>>> +       platform_device_unregister(bus->icc_pdev);
>>> +
>>>         dev_pm_opp_of_remove_table(dev);
>>>         clk_disable_unprepare(bus->clk);
>>>  }
>>> @@ -431,6 +436,18 @@ static int exynos_bus_probe(struct platform_device *pdev)
>>>         if (ret < 0)
>>>                 goto err;
>>>
>>> +       /* Create child platform device for the interconnect provider */
>>> +       if (of_get_property(dev->of_node, "#interconnect-cells", NULL)) {
>>> +                   bus->icc_pdev = platform_device_register_data(
>>> +                                               dev, "exynos-generic-icc",
>>> +                                               PLATFORM_DEVID_AUTO, NULL, 0);
>>> +
>>> +                   if (IS_ERR(bus->icc_pdev)) {
>>> +                           ret = PTR_ERR(bus->icc_pdev);
>>> +                           goto err;
>>> +                   }
>>> +       }
>>> +
>>>         max_state = bus->devfreq->profile->max_state;
>>>         min_freq = (bus->devfreq->profile->freq_table[0] / 1000);
>>>         max_freq = (bus->devfreq->profile->freq_table[max_state - 1] / 1000);
>>> --
>>> 2.7.4
>>>
>>
>> It looks like very similar like the registering the interconnect
>> device of imx-bus.c
>> and I already reviewed and agreed this approach.
>>
>> Acked-by: Chanwoo Choi <cw00.choi@samsung.com>
>>
>> nitpick: IMHO, I think that 'exynos-icc' is proper and simple without
>> 'generic' word.
>> If we need to add new icc compatible int the future, we will add
>> 'exynosXXXX-icc' new compatible.
>> But, I'm not forcing it. just opinion. Anyway, I agree this approach.
> 
> Thanks for review. I will change the name to exynos-icc in next version, 
> as I commented at other patch, it is not part of any DT binding, 
> it is just for device/driver matching between devfreq and interconnect.

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

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2020-06-02  0:40 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <CGME20200529163213eucas1p1ac148f9238214ac84f3d0cc199c4398b@eucas1p1.samsung.com>
     [not found] ` <20200529163200.18031-1-s.nawrocki@samsung.com>
2020-06-01  8:17   ` [RFC PATCH v5 0/6] Exynos: Simple QoS for exynos-bus using interconnect Sylwester Nawrocki
     [not found]   ` <CGME20200529163219eucas1p2d127fe3936921f53f6fe7902e7d14a3e@eucas1p2.samsung.com>
     [not found]     ` <20200529163200.18031-2-s.nawrocki@samsung.com>
2020-06-01  8:19       ` [RFC PATCH v5 1/6] dt-bindings: exynos-bus: Add documentation for interconnect properties Sylwester Nawrocki
     [not found]       ` <CAGTfZH1yM0KRaEF5VTs2juTm+yrK9VqQZxWjdNf_ffjGHWPLsg@mail.gmail.com>
2020-06-01  9:40         ` Sylwester Nawrocki
     [not found]   ` <20200529163200.18031-3-s.nawrocki@samsung.com>
     [not found]     ` <CAGTfZH3zvgOtME0U2hKjtqO49e0-Nw6MRxhw+z6Mfio-=FUwcQ@mail.gmail.com>
2020-06-01  9:57       ` [RFC PATCH v5 2/6] interconnect: Add generic interconnect driver for Exynos SoCs Sylwester Nawrocki
     [not found]   ` <20200529163200.18031-4-s.nawrocki@samsung.com>
     [not found]     ` <CAGTfZH1KC=jpQ5GXNtEf1cn7+WqXJdqbbVKmpxr8Snh4GEy8bA@mail.gmail.com>
2020-06-01 10:04       ` [RFC PATCH v5 3/6] PM / devfreq: exynos-bus: Add registration of interconnect child device Sylwester Nawrocki
2020-06-02  0:50         ` Chanwoo Choi

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).