* 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).