* [RFC PATCH v3 0/7] PM / devfreq: Simple QoS for exynos-bus using interconnect [not found] <CGME20191220120140eucas1p14ad33c20882f8f48e02337ea16754d91@eucas1p1.samsung.com> @ 2019-12-20 11:56 ` Artur Świgoń [not found] ` <CGME20191220120141eucas1p11f5fa9d76d17e80e06199cb7a911c482@eucas1p1.samsung.com> ` (7 more replies) 0 siblings, 8 replies; 31+ messages in thread From: Artur Świgoń @ 2019-12-20 11:56 UTC (permalink / raw) To: devicetree, linux-arm-kernel, linux-samsung-soc, linux-kernel, linux-pm, dri-devel Cc: Artur Świgoń, cw00.choi, myungjoo.ham, inki.dae, sw0312.kim, georgi.djakov, leonard.crestez, m.szyprowski, b.zolnierkie, krzk The following patchset adds interconnect[1][2] framework support to the exynos-bus devfreq driver. Extending the devfreq driver with interconnect functionality started as a response to the issue referenced in [3]. The patches can be subdivided into three groups: (a) Tweaking the interconnect framework to support the exynos-bus use case (patches 01--03/07). Exporting of_icc_get_from_provider() allows to avoid hardcoding every single graph edge in the DT or driver source, and relaxing the requirement on #interconnect-cells removes the need to provide dummy node IDs in the DT. A new field in struct icc_provider is used to explicitly allow configuring node pairs from two different providers. (b) Implementing interconnect providers in the exynos-bus devfreq driver and adding required DT properties for one selected platform, namely Exynos4412 (patches 04--05/07). Due to the fact that this aims to be a generic driver for various Exynos SoCs, node IDs are generated dynamically (rather than hardcoded). (c) Implementing a sample interconnect consumer for exynos-mixer targeted at solving the issue referenced in [3], again with DT properties only for Exynos4412 (patches 06--07/07). Integration of devfreq and interconnect frameworks is achieved by using the dev_pm_qos_*() API. When CONFIG_INTERCONNECT is 'n' (such as in exynos_defconfig) all interconnect API functions are no-ops. This series depends on these three patches (merged into devfreq-next[6]): * https://patchwork.kernel.org/patch/11279087/ * https://patchwork.kernel.org/patch/11279093/ * https://patchwork.kernel.org/patch/11293765/ and on this series: * https://patchwork.kernel.org/cover/11304545/ (which does not apply cleanly on next-20191220, adding --exclude=arch/arm/boot/dts/exynos5422-odroid-core.dtsi to 'git am' is a quick workaround) --- 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. --- Artur Świgoń Samsung R&D Institute Poland Samsung Electronics --- References: [1] Documentation/interconnect/interconnect.rst [2] Documentation/devicetree/bindings/interconnect/interconnect.txt [3] https://patchwork.kernel.org/patch/10861757/ (original issue) [4] https://patchwork.kernel.org/cover/11054417/ (v1 of this RFC) [5] https://patchwork.kernel.org/cover/11152595/ (v2 of this RFC) [6] https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/linux.git/log/?h=devfreq-next Artur Świgoń (6): interconnect: Export of_icc_get_from_provider() interconnect: Relax requirement in of_icc_get_from_provider() interconnect: Allow inter-provider pairs to be configured arm: dts: exynos: Add interconnect bindings for Exynos4412 devfreq: exynos-bus: Add interconnect functionality to exynos-bus arm: dts: exynos: Add interconnects to Exynos4412 mixer Marek Szyprowski (1): drm: exynos: mixer: Add interconnect support .../boot/dts/exynos4412-odroid-common.dtsi | 5 + arch/arm/boot/dts/exynos4412.dtsi | 1 + drivers/devfreq/exynos-bus.c | 144 ++++++++++++++++++ drivers/gpu/drm/exynos/exynos_mixer.c | 71 ++++++++- drivers/interconnect/core.c | 16 +- include/linux/interconnect-provider.h | 8 + 6 files changed, 232 insertions(+), 13 deletions(-) -- 2.17.1 ^ permalink raw reply [flat|nested] 31+ messages in thread
[parent not found: <CGME20191220120141eucas1p11f5fa9d76d17e80e06199cb7a911c482@eucas1p1.samsung.com>]
* [RFC PATCH v3 1/7] interconnect: Export of_icc_get_from_provider() [not found] ` <CGME20191220120141eucas1p11f5fa9d76d17e80e06199cb7a911c482@eucas1p1.samsung.com> @ 2019-12-20 11:56 ` Artur Świgoń 0 siblings, 0 replies; 31+ messages in thread From: Artur Świgoń @ 2019-12-20 11:56 UTC (permalink / raw) To: devicetree, linux-arm-kernel, linux-samsung-soc, linux-kernel, linux-pm, dri-devel Cc: Artur Świgoń, cw00.choi, myungjoo.ham, inki.dae, sw0312.kim, georgi.djakov, leonard.crestez, m.szyprowski, b.zolnierkie, krzk This patch makes the above function public (for use in exynos-bus devfreq driver). Signed-off-by: Artur Świgoń <a.swigon@samsung.com> Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com> --- drivers/interconnect/core.c | 3 ++- include/linux/interconnect-provider.h | 6 ++++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c index 63c164264b73..e6035c199369 100644 --- a/drivers/interconnect/core.c +++ b/drivers/interconnect/core.c @@ -330,7 +330,7 @@ EXPORT_SYMBOL_GPL(of_icc_xlate_onecell); * Returns a valid pointer to struct icc_node on success or ERR_PTR() * on failure. */ -static struct icc_node *of_icc_get_from_provider(struct of_phandle_args *spec) +struct icc_node *of_icc_get_from_provider(struct of_phandle_args *spec) { struct icc_node *node = ERR_PTR(-EPROBE_DEFER); struct icc_provider *provider; @@ -349,6 +349,7 @@ static struct icc_node *of_icc_get_from_provider(struct of_phandle_args *spec) return node; } +EXPORT_SYMBOL_GPL(of_icc_get_from_provider); /** * of_icc_get() - get a path handle from a DT node based on name diff --git a/include/linux/interconnect-provider.h b/include/linux/interconnect-provider.h index 0c494534b4d3..cc965b8fab53 100644 --- a/include/linux/interconnect-provider.h +++ b/include/linux/interconnect-provider.h @@ -103,6 +103,7 @@ void icc_node_del(struct icc_node *node); int icc_nodes_remove(struct icc_provider *provider); int icc_provider_add(struct icc_provider *provider); int icc_provider_del(struct icc_provider *provider); +struct icc_node *of_icc_get_from_provider(struct of_phandle_args *spec); #else @@ -154,6 +155,11 @@ static inline int icc_provider_del(struct icc_provider *provider) return -ENOTSUPP; } +struct icc_node *of_icc_get_from_provider(struct of_phandle_args *spec) +{ + return ERR_PTR(-ENOTSUPP); +} + #endif /* CONFIG_INTERCONNECT */ #endif /* __LINUX_INTERCONNECT_PROVIDER_H */ -- 2.17.1 ^ permalink raw reply related [flat|nested] 31+ messages in thread
[parent not found: <CGME20191220120142eucas1p1f43c7a862d9c0faa72e14b21d7d697e9@eucas1p1.samsung.com>]
* [RFC PATCH v3 2/7] interconnect: Relax requirement in of_icc_get_from_provider() [not found] ` <CGME20191220120142eucas1p1f43c7a862d9c0faa72e14b21d7d697e9@eucas1p1.samsung.com> @ 2019-12-20 11:56 ` Artur Świgoń 2019-12-21 17:20 ` Chanwoo Choi 0 siblings, 1 reply; 31+ messages in thread From: Artur Świgoń @ 2019-12-20 11:56 UTC (permalink / raw) To: devicetree, linux-arm-kernel, linux-samsung-soc, linux-kernel, linux-pm, dri-devel Cc: Artur Świgoń, cw00.choi, myungjoo.ham, inki.dae, sw0312.kim, georgi.djakov, leonard.crestez, m.szyprowski, b.zolnierkie, krzk This patch relaxes the condition in of_icc_get_from_provider() so that it is no longer required to set #interconnect-cells = <1> in the DT. In case of the devfreq driver for exynos-bus, #interconnect-cells is always zero. Signed-off-by: Artur Świgoń <a.swigon@samsung.com> Acked-by: Krzysztof Kozlowski <krzk@kernel.org> --- drivers/interconnect/core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c index e6035c199369..74c68898a350 100644 --- a/drivers/interconnect/core.c +++ b/drivers/interconnect/core.c @@ -335,7 +335,7 @@ struct icc_node *of_icc_get_from_provider(struct of_phandle_args *spec) struct icc_node *node = ERR_PTR(-EPROBE_DEFER); struct icc_provider *provider; - if (!spec || spec->args_count != 1) + if (!spec) return ERR_PTR(-EINVAL); mutex_lock(&icc_lock); -- 2.17.1 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [RFC PATCH v3 2/7] interconnect: Relax requirement in of_icc_get_from_provider() 2019-12-20 11:56 ` [RFC PATCH v3 2/7] interconnect: Relax requirement in of_icc_get_from_provider() Artur Świgoń @ 2019-12-21 17:20 ` Chanwoo Choi 0 siblings, 0 replies; 31+ messages in thread From: Chanwoo Choi @ 2019-12-21 17:20 UTC (permalink / raw) To: Artur Świgoń Cc: devicetree, linux-arm-kernel, linux-samsung-soc, linux-kernel, Linux PM list, dri-devel, Chanwoo Choi, MyungJoo Ham, inki.dae, Seung-Woo Kim, Georgi Djakov, Leonard Crestez, Marek Szyprowski, Bartlomiej Zolnierkiewicz, Krzysztof Kozlowski Hi, On Fri, Dec 20, 2019 at 9:03 PM Artur Świgoń <a.swigon@samsung.com> wrote: > > This patch relaxes the condition in of_icc_get_from_provider() so that it > is no longer required to set #interconnect-cells = <1> in the DT. In case > of the devfreq driver for exynos-bus, #interconnect-cells is always zero. It doesn't contain why don't need to require it. If you add more detailed description, it is better to understand. > > Signed-off-by: Artur Świgoń <a.swigon@samsung.com> > Acked-by: Krzysztof Kozlowski <krzk@kernel.org> > --- > drivers/interconnect/core.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c > index e6035c199369..74c68898a350 100644 > --- a/drivers/interconnect/core.c > +++ b/drivers/interconnect/core.c > @@ -335,7 +335,7 @@ struct icc_node *of_icc_get_from_provider(struct of_phandle_args *spec) > struct icc_node *node = ERR_PTR(-EPROBE_DEFER); > struct icc_provider *provider; > > - if (!spec || spec->args_count != 1) > + if (!spec) > return ERR_PTR(-EINVAL); > > mutex_lock(&icc_lock); > -- > 2.17.1 > -- Best Regards, Chanwoo Choi ^ permalink raw reply [flat|nested] 31+ messages in thread
[parent not found: <CGME20191220120143eucas1p1c9b01ae8c2e4ecd70423ef9d8001536f@eucas1p1.samsung.com>]
* [RFC PATCH v3 3/7] interconnect: Allow inter-provider pairs to be configured [not found] ` <CGME20191220120143eucas1p1c9b01ae8c2e4ecd70423ef9d8001536f@eucas1p1.samsung.com> @ 2019-12-20 11:56 ` Artur Świgoń 2019-12-22 17:08 ` Chanwoo Choi 0 siblings, 1 reply; 31+ messages in thread From: Artur Świgoń @ 2019-12-20 11:56 UTC (permalink / raw) To: devicetree, linux-arm-kernel, linux-samsung-soc, linux-kernel, linux-pm, dri-devel Cc: Artur Świgoń, cw00.choi, myungjoo.ham, inki.dae, sw0312.kim, georgi.djakov, leonard.crestez, m.szyprowski, b.zolnierkie, krzk In the exynos-bus devfreq driver every bus is probed separately and is assigned a separate interconnect provider. However, the interconnect framework does not call the '->set' callback for pairs of nodes which belong to different providers. This patch adds support for a new boolean 'inter_set' field in struct icc_provider. Setting it to 'true' enables calling '->set' for inter-provider node pairs. All existing users of the interconnect framework allocate this structure with kzalloc, and are therefore unaffected. Signed-off-by: Artur Świgoń <a.swigon@samsung.com> --- drivers/interconnect/core.c | 11 +++++------ include/linux/interconnect-provider.h | 2 ++ 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c index 74c68898a350..a28bd0f8a497 100644 --- a/drivers/interconnect/core.c +++ b/drivers/interconnect/core.c @@ -259,23 +259,22 @@ static int aggregate_requests(struct icc_node *node) static int apply_constraints(struct icc_path *path) { struct icc_node *next, *prev = NULL; + struct icc_provider *p; int ret = -EINVAL; int i; for (i = 0; i < path->num_nodes; i++) { next = path->reqs[i].node; + p = next->provider; - /* - * Both endpoints should be valid master-slave pairs of the - * same interconnect provider that will be configured. - */ - if (!prev || next->provider != prev->provider) { + /* both endpoints should be valid master-slave pairs */ + if (!prev || (p != prev->provider && !p->inter_set)) { prev = next; continue; } /* set the constraints */ - ret = next->provider->set(prev, next); + ret = p->set(prev, next); if (ret) goto out; diff --git a/include/linux/interconnect-provider.h b/include/linux/interconnect-provider.h index cc965b8fab53..b6ae0ee686c5 100644 --- a/include/linux/interconnect-provider.h +++ b/include/linux/interconnect-provider.h @@ -41,6 +41,7 @@ struct icc_node *of_icc_xlate_onecell(struct of_phandle_args *spec, * @xlate: provider-specific callback for mapping nodes from phandle arguments * @dev: the device this interconnect provider belongs to * @users: count of active users + * @inter_set: whether inter-provider pairs will be configured with @set * @data: pointer to private data */ struct icc_provider { @@ -53,6 +54,7 @@ struct icc_provider { struct icc_node* (*xlate)(struct of_phandle_args *spec, void *data); struct device *dev; int users; + bool inter_set; void *data; }; -- 2.17.1 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [RFC PATCH v3 3/7] interconnect: Allow inter-provider pairs to be configured 2019-12-20 11:56 ` [RFC PATCH v3 3/7] interconnect: Allow inter-provider pairs to be configured Artur Świgoń @ 2019-12-22 17:08 ` Chanwoo Choi 0 siblings, 0 replies; 31+ messages in thread From: Chanwoo Choi @ 2019-12-22 17:08 UTC (permalink / raw) To: Artur Świgoń Cc: devicetree, linux-arm-kernel, linux-samsung-soc, linux-kernel, Linux PM list, dri-devel, Chanwoo Choi, MyungJoo Ham, inki.dae, Seung-Woo Kim, Georgi Djakov, Leonard Crestez, Marek Szyprowski, Bartlomiej Zolnierkiewicz, Krzysztof Kozlowski Hi, On Fri, Dec 20, 2019 at 9:03 PM Artur Świgoń <a.swigon@samsung.com> wrote: > > In the exynos-bus devfreq driver every bus is probed separately and is IMHO, the patch description should specify the more general cause why have to be changed. Actually, almost people might not know the 'exynos-bus'. So, firstly, you have to specify the general cause why this patch is necessary without 'exynos-bus' word and then add the real use-case with 'exynos-bus' example. > assigned a separate interconnect provider. However, the interconnect > framework does not call the '->set' callback for pairs of nodes which > belong to different providers. > > This patch adds support for a new boolean 'inter_set' field in struct > icc_provider. Setting it to 'true' enables calling '->set' for > inter-provider node pairs. All existing users of the interconnect > framework allocate this structure with kzalloc, and are therefore > unaffected. > > Signed-off-by: Artur Świgoń <a.swigon@samsung.com> > --- > drivers/interconnect/core.c | 11 +++++------ > include/linux/interconnect-provider.h | 2 ++ > 2 files changed, 7 insertions(+), 6 deletions(-) > > diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c > index 74c68898a350..a28bd0f8a497 100644 > --- a/drivers/interconnect/core.c > +++ b/drivers/interconnect/core.c > @@ -259,23 +259,22 @@ static int aggregate_requests(struct icc_node *node) > static int apply_constraints(struct icc_path *path) > { > struct icc_node *next, *prev = NULL; > + struct icc_provider *p; > int ret = -EINVAL; > int i; > > for (i = 0; i < path->num_nodes; i++) { > next = path->reqs[i].node; > + p = next->provider; > > - /* > - * Both endpoints should be valid master-slave pairs of the > - * same interconnect provider that will be configured. > - */ > - if (!prev || next->provider != prev->provider) { > + /* both endpoints should be valid master-slave pairs */ > + if (!prev || (p != prev->provider && !p->inter_set)) { > prev = next; > continue; > } > > /* set the constraints */ > - ret = next->provider->set(prev, next); > + ret = p->set(prev, next); > if (ret) > goto out; > > diff --git a/include/linux/interconnect-provider.h b/include/linux/interconnect-provider.h > index cc965b8fab53..b6ae0ee686c5 100644 > --- a/include/linux/interconnect-provider.h > +++ b/include/linux/interconnect-provider.h > @@ -41,6 +41,7 @@ struct icc_node *of_icc_xlate_onecell(struct of_phandle_args *spec, > * @xlate: provider-specific callback for mapping nodes from phandle arguments > * @dev: the device this interconnect provider belongs to > * @users: count of active users > + * @inter_set: whether inter-provider pairs will be configured with @set > * @data: pointer to private data > */ > struct icc_provider { > @@ -53,6 +54,7 @@ struct icc_provider { > struct icc_node* (*xlate)(struct of_phandle_args *spec, void *data); > struct device *dev; > int users; > + bool inter_set; > void *data; > }; > > -- > 2.17.1 > -- Best Regards, Chanwoo Choi ^ permalink raw reply [flat|nested] 31+ messages in thread
[parent not found: <CGME20191220120144eucas1p119ececf161a6d45a6a194e432bbbd1f9@eucas1p1.samsung.com>]
* [RFC PATCH v3 4/7] arm: dts: exynos: Add interconnect bindings for Exynos4412 [not found] ` <CGME20191220120144eucas1p119ececf161a6d45a6a194e432bbbd1f9@eucas1p1.samsung.com> @ 2019-12-20 11:56 ` Artur Świgoń 2019-12-21 20:00 ` Chanwoo Choi ` (2 more replies) 0 siblings, 3 replies; 31+ messages in thread From: Artur Świgoń @ 2019-12-20 11:56 UTC (permalink / raw) To: devicetree, linux-arm-kernel, linux-samsung-soc, linux-kernel, linux-pm, dri-devel Cc: Artur Świgoń, cw00.choi, myungjoo.ham, inki.dae, sw0312.kim, georgi.djakov, leonard.crestez, m.szyprowski, b.zolnierkie, krzk This patch adds the following properties to the Exynos4412 DT: - exynos,interconnect-parent-node: to declare connections between nodes in order to guarantee PM QoS requirements between nodes; - #interconnect-cells: required by the interconnect framework. Note that #interconnect-cells is always zero and node IDs are not hardcoded anywhere. Signed-off-by: Artur Świgoń <a.swigon@samsung.com> --- arch/arm/boot/dts/exynos4412-odroid-common.dtsi | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi index 4ce3d77a6704..d9d70eacfcaf 100644 --- a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi +++ b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi @@ -90,6 +90,7 @@ &bus_dmc { exynos,ppmu-device = <&ppmu_dmc0_3>, <&ppmu_dmc1_3>; vdd-supply = <&buck1_reg>; + #interconnect-cells = <0>; status = "okay"; }; @@ -106,6 +107,8 @@ &bus_leftbus { exynos,ppmu-device = <&ppmu_leftbus_3>, <&ppmu_rightbus_3>; vdd-supply = <&buck3_reg>; + exynos,interconnect-parent-node = <&bus_dmc>; + #interconnect-cells = <0>; status = "okay"; }; @@ -116,6 +119,8 @@ &bus_display { exynos,parent-bus = <&bus_leftbus>; + exynos,interconnect-parent-node = <&bus_leftbus>; + #interconnect-cells = <0>; status = "okay"; }; -- 2.17.1 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [RFC PATCH v3 4/7] arm: dts: exynos: Add interconnect bindings for Exynos4412 2019-12-20 11:56 ` [RFC PATCH v3 4/7] arm: dts: exynos: Add interconnect bindings for Exynos4412 Artur Świgoń @ 2019-12-21 20:00 ` Chanwoo Choi 2019-12-30 15:44 ` Krzysztof Kozlowski 2020-01-22 16:54 ` Georgi Djakov 2 siblings, 0 replies; 31+ messages in thread From: Chanwoo Choi @ 2019-12-21 20:00 UTC (permalink / raw) To: Artur Świgoń Cc: devicetree, linux-arm-kernel, linux-samsung-soc, linux-kernel, Linux PM list, dri-devel, Chanwoo Choi, MyungJoo Ham, inki.dae, Seung-Woo Kim, Georgi Djakov, Leonard Crestez, Marek Szyprowski, Bartlomiej Zolnierkiewicz, Krzysztof Kozlowski Hi, On Fri, Dec 20, 2019 at 9:02 PM Artur Świgoń <a.swigon@samsung.com> wrote: > > This patch adds the following properties to the Exynos4412 DT: > - exynos,interconnect-parent-node: to declare connections between > nodes in order to guarantee PM QoS requirements between nodes; > - #interconnect-cells: required by the interconnect framework. > > Note that #interconnect-cells is always zero and node IDs are not > hardcoded anywhere. > > Signed-off-by: Artur Świgoń <a.swigon@samsung.com> > --- > arch/arm/boot/dts/exynos4412-odroid-common.dtsi | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi > index 4ce3d77a6704..d9d70eacfcaf 100644 > --- a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi > +++ b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi > @@ -90,6 +90,7 @@ > &bus_dmc { > exynos,ppmu-device = <&ppmu_dmc0_3>, <&ppmu_dmc1_3>; > vdd-supply = <&buck1_reg>; > + #interconnect-cells = <0>; > status = "okay"; > }; > > @@ -106,6 +107,8 @@ > &bus_leftbus { > exynos,ppmu-device = <&ppmu_leftbus_3>, <&ppmu_rightbus_3>; > vdd-supply = <&buck3_reg>; > + exynos,interconnect-parent-node = <&bus_dmc>; > + #interconnect-cells = <0>; > status = "okay"; > }; > > @@ -116,6 +119,8 @@ > > &bus_display { > exynos,parent-bus = <&bus_leftbus>; > + exynos,interconnect-parent-node = <&bus_leftbus>; > + #interconnect-cells = <0>; > status = "okay"; > }; > > -- > 2.17.1 > Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com> I has not yet tested on target. I'll test it on next week and then reply the test result. -- Best Regards, Chanwoo Choi ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC PATCH v3 4/7] arm: dts: exynos: Add interconnect bindings for Exynos4412 2019-12-20 11:56 ` [RFC PATCH v3 4/7] arm: dts: exynos: Add interconnect bindings for Exynos4412 Artur Świgoń 2019-12-21 20:00 ` Chanwoo Choi @ 2019-12-30 15:44 ` Krzysztof Kozlowski 2019-12-31 7:18 ` Artur Świgoń 2020-01-22 16:54 ` Georgi Djakov 2 siblings, 1 reply; 31+ messages in thread From: Krzysztof Kozlowski @ 2019-12-30 15:44 UTC (permalink / raw) To: Artur Świgoń Cc: devicetree, linux-arm-kernel, linux-samsung-soc, linux-kernel, linux-pm, dri-devel, cw00.choi, myungjoo.ham, inki.dae, sw0312.kim, georgi.djakov, leonard.crestez, m.szyprowski, b.zolnierkie On Fri, Dec 20, 2019 at 12:56:50PM +0100, Artur Świgoń wrote: > This patch adds the following properties to the Exynos4412 DT: > - exynos,interconnect-parent-node: to declare connections between > nodes in order to guarantee PM QoS requirements between nodes; > - #interconnect-cells: required by the interconnect framework. > > Note that #interconnect-cells is always zero and node IDs are not > hardcoded anywhere. > > Signed-off-by: Artur Świgoń <a.swigon@samsung.com> > --- > arch/arm/boot/dts/exynos4412-odroid-common.dtsi | 5 +++++ > 1 file changed, 5 insertions(+) The order of patches is confusing. Patches 4 and 6 are split - do the depend on 5? I doubt but... Adjust the title to match the contents - you are not adding bindings but properties to bus nodes. Also the prefix is ARM: (look at recent commits). > > diff --git a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi > index 4ce3d77a6704..d9d70eacfcaf 100644 > --- a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi > +++ b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi > @@ -90,6 +90,7 @@ > &bus_dmc { > exynos,ppmu-device = <&ppmu_dmc0_3>, <&ppmu_dmc1_3>; > vdd-supply = <&buck1_reg>; > + #interconnect-cells = <0>; This does not look like property of Odroid but Exynos4412 or Exynos4. Best regards, Krzysztof ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC PATCH v3 4/7] arm: dts: exynos: Add interconnect bindings for Exynos4412 2019-12-30 15:44 ` Krzysztof Kozlowski @ 2019-12-31 7:18 ` Artur Świgoń 2019-12-31 9:22 ` Krzysztof Kozlowski 0 siblings, 1 reply; 31+ messages in thread From: Artur Świgoń @ 2019-12-31 7:18 UTC (permalink / raw) To: Krzysztof Kozlowski Cc: devicetree, linux-arm-kernel, linux-samsung-soc, linux-kernel, linux-pm, dri-devel, cw00.choi, myungjoo.ham, inki.dae, sw0312.kim, georgi.djakov, leonard.crestez, m.szyprowski, b.zolnierkie Hi, On Mon, 2019-12-30 at 16:44 +0100, Krzysztof Kozlowski wrote: > On Fri, Dec 20, 2019 at 12:56:50PM +0100, Artur Świgoń wrote: > > This patch adds the following properties to the Exynos4412 DT: > > - exynos,interconnect-parent-node: to declare connections between > > nodes in order to guarantee PM QoS requirements between nodes; > > - #interconnect-cells: required by the interconnect framework. > > > > Note that #interconnect-cells is always zero and node IDs are not > > hardcoded anywhere. > > > > Signed-off-by: Artur Świgoń <a.swigon@samsung.com> > > --- > > arch/arm/boot/dts/exynos4412-odroid-common.dtsi | 5 +++++ > > 1 file changed, 5 insertions(+) > > The order of patches is confusing. Patches 4 and 6 are split - do the > depend on 5? I doubt but... Let me elaborate: The order of the patches in this series is such that every subsequent patch adds some functionality (and, of course, applying patches one-by-one yields a working kernel at every step). Specifically for patches 04--07: -- patch 04 adds interconnect _provider_ properties for Exynos4412; -- patch 05 implements interconnect provider logic (depends on patch 04); -- patch 06 adds interconnect _consumer_ properties for Exynos4412 mixer; -- patch 07 implements interconnect consumer logic (depends on patches 05 & 06); My reasoning is that this order allows to e.g., merge the interconnect provider for exynos-bus and leave the consumers for later (not limited to the mixer). I hope this makes sense. > Adjust the title to match the contents - you are not adding bindings but > properties to bus nodes. Also the prefix is ARM: (look at recent > commits). OK. > > > > diff --git a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi > > index 4ce3d77a6704..d9d70eacfcaf 100644 > > --- a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi > > +++ b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi > > @@ -90,6 +90,7 @@ > > &bus_dmc { > > exynos,ppmu-device = <&ppmu_dmc0_3>, <&ppmu_dmc1_3>; > > vdd-supply = <&buck1_reg>; > > + #interconnect-cells = <0>; > > This does not look like property of Odroid but Exynos4412 or Exynos4. Strangely enough, this file is where the 'exynos,parent-bus' (aka. 'devfreq') properties are located (and everything in this RFC concerns devfreq). Regards, -- Artur Świgoń Samsung R&D Institute Poland Samsung Electronics ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC PATCH v3 4/7] arm: dts: exynos: Add interconnect bindings for Exynos4412 2019-12-31 7:18 ` Artur Świgoń @ 2019-12-31 9:22 ` Krzysztof Kozlowski 2019-12-31 9:41 ` Artur Świgoń 0 siblings, 1 reply; 31+ messages in thread From: Krzysztof Kozlowski @ 2019-12-31 9:22 UTC (permalink / raw) To: Artur Świgoń Cc: devicetree, linux-arm-kernel, linux-samsung-soc, linux-kernel, linux-pm, dri-devel, cw00.choi, myungjoo.ham, inki.dae, sw0312.kim, georgi.djakov, leonard.crestez, m.szyprowski, b.zolnierkie On Tue, Dec 31, 2019 at 08:18:01AM +0100, Artur Świgoń wrote: > Hi, > > On Mon, 2019-12-30 at 16:44 +0100, Krzysztof Kozlowski wrote: > > On Fri, Dec 20, 2019 at 12:56:50PM +0100, Artur Świgoń wrote: > > > This patch adds the following properties to the Exynos4412 DT: > > > - exynos,interconnect-parent-node: to declare connections between > > > nodes in order to guarantee PM QoS requirements between nodes; > > > - #interconnect-cells: required by the interconnect framework. > > > > > > Note that #interconnect-cells is always zero and node IDs are not > > > hardcoded anywhere. > > > > > > Signed-off-by: Artur Świgoń <a.swigon@samsung.com> > > > --- > > > arch/arm/boot/dts/exynos4412-odroid-common.dtsi | 5 +++++ > > > 1 file changed, 5 insertions(+) > > > > The order of patches is confusing. Patches 4 and 6 are split - do the > > depend on 5? I doubt but... > > Let me elaborate: > > The order of the patches in this series is such that every subsequent > patch adds some functionality (and, of course, applying patches one-by-one > yields a working kernel at every step). Specifically for patches 04--07: > > -- patch 04 adds interconnect _provider_ properties for Exynos4412; > -- patch 05 implements interconnect provider logic (depends on patch 04); > -- patch 06 adds interconnect _consumer_ properties for Exynos4412 mixer; > -- patch 07 implements interconnect consumer logic (depends on patches > 05 & 06); > > My reasoning is that this order allows to e.g., merge the interconnect > provider for exynos-bus and leave the consumers for later (not limited to > the mixer). I hope this makes sense. It is wrong. The driver should not depend on DTS changes because: 1. DTS always go through separate branch and tree, so last patch will have to wait up to 3 cycles (!!!), 2. You break backward compatibility. In certain cases dependency on DTS changes is ok: 1. Cleaning up deprecated properties, 2. Ignoring the backward compatibility for e.g. new platforms. None of these are applicable here. You need to rework it, put DTS changes at the end. This clearly shows that there is no wrong dependency. > > > Adjust the title to match the contents - you are not adding bindings but > > properties to bus nodes. Also the prefix is ARM: (look at recent > > commits). > > OK. > > > > > > > diff --git a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi > > > index 4ce3d77a6704..d9d70eacfcaf 100644 > > > --- a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi > > > +++ b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi > > > @@ -90,6 +90,7 @@ > > > &bus_dmc { > > > exynos,ppmu-device = <&ppmu_dmc0_3>, <&ppmu_dmc1_3>; > > > vdd-supply = <&buck1_reg>; > > > + #interconnect-cells = <0>; > > > > This does not look like property of Odroid but Exynos4412 or Exynos4. > > Strangely enough, this file is where the 'exynos,parent-bus' (aka. 'devfreq') > properties are located (and everything in this RFC concerns devfreq). I cannot find exynos,parent-bus in exynos4412-odroid-common.dtsi. Can you elaborate? Best regards, Krzysztof ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC PATCH v3 4/7] arm: dts: exynos: Add interconnect bindings for Exynos4412 2019-12-31 9:22 ` Krzysztof Kozlowski @ 2019-12-31 9:41 ` Artur Świgoń 2019-12-31 10:02 ` Krzysztof Kozlowski 0 siblings, 1 reply; 31+ messages in thread From: Artur Świgoń @ 2019-12-31 9:41 UTC (permalink / raw) To: Krzysztof Kozlowski Cc: devicetree, linux-arm-kernel, linux-samsung-soc, linux-kernel, linux-pm, dri-devel, cw00.choi, myungjoo.ham, inki.dae, sw0312.kim, georgi.djakov, leonard.crestez, m.szyprowski, b.zolnierkie On Tue, 2019-12-31 at 10:22 +0100, Krzysztof Kozlowski wrote: > On Tue, Dec 31, 2019 at 08:18:01AM +0100, Artur Świgoń wrote: > > Hi, > > > > On Mon, 2019-12-30 at 16:44 +0100, Krzysztof Kozlowski wrote: > > > On Fri, Dec 20, 2019 at 12:56:50PM +0100, Artur Świgoń wrote: > > > > This patch adds the following properties to the Exynos4412 DT: > > > > - exynos,interconnect-parent-node: to declare connections between > > > > nodes in order to guarantee PM QoS requirements between nodes; > > > > - #interconnect-cells: required by the interconnect framework. > > > > > > > > Note that #interconnect-cells is always zero and node IDs are not > > > > hardcoded anywhere. > > > > > > > > Signed-off-by: Artur Świgoń <a.swigon@samsung.com> > > > > --- > > > > arch/arm/boot/dts/exynos4412-odroid-common.dtsi | 5 +++++ > > > > 1 file changed, 5 insertions(+) > > > > > > The order of patches is confusing. Patches 4 and 6 are split - do the > > > depend on 5? I doubt but... > > > > Let me elaborate: > > > > The order of the patches in this series is such that every subsequent > > patch adds some functionality (and, of course, applying patches one-by-one > > yields a working kernel at every step). Specifically for patches 04--07: > > > > -- patch 04 adds interconnect _provider_ properties for Exynos4412; > > -- patch 05 implements interconnect provider logic (depends on patch 04); > > -- patch 06 adds interconnect _consumer_ properties for Exynos4412 mixer; > > -- patch 07 implements interconnect consumer logic (depends on patches > > 05 & 06); > > > > My reasoning is that this order allows to e.g., merge the interconnect > > provider for exynos-bus and leave the consumers for later (not limited to > > the mixer). I hope this makes sense. > > It is wrong. The driver should not depend on DTS changes because: > 1. DTS always go through separate branch and tree, so last patch > will have to wait up to 3 cycles (!!!), > 2. You break backward compatibility. It is up to the definition of "depends". The driver is _not_ broken without the DTS patches, but the interconnect functionality will not be available. The only requirement is that if we want to have a working interconnect consumer, there needs to be a working interconnet provider (and I used the word "depends" to specify what needs what in order to work as intended). I still think the order of these patches is the most logical one for someone reading this RFC as a whole. > In certain cases dependency on DTS changes is ok: > 1. Cleaning up deprecated properties, > 2. Ignoring the backward compatibility for e.g. new platforms. > > None of these are applicable here. > > You need to rework it, put DTS changes at the end. This clearly shows > that there is no wrong dependency. > > > > > > Adjust the title to match the contents - you are not adding bindings but > > > properties to bus nodes. Also the prefix is ARM: (look at recent > > > commits). > > > > OK. > > > > > > > > > > diff --git a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi > > > > index 4ce3d77a6704..d9d70eacfcaf 100644 > > > > --- a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi > > > > +++ b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi > > > > @@ -90,6 +90,7 @@ > > > > &bus_dmc { > > > > exynos,ppmu-device = <&ppmu_dmc0_3>, <&ppmu_dmc1_3>; > > > > vdd-supply = <&buck1_reg>; > > > > + #interconnect-cells = <0>; > > > > > > This does not look like property of Odroid but Exynos4412 or Exynos4. > > > > Strangely enough, this file is where the 'exynos,parent-bus' (aka. 'devfreq') > > properties are located (and everything in this RFC concerns devfreq). > > I cannot find exynos,parent-bus in exynos4412-odroid-common.dtsi. Can > you elaborate? Currently a name change is being made: 'devfreq' -> 'exynos,parent-bus' https://patchwork.kernel.org/patch/11304549/ (a dependency of this RFC; also available in devfreq-testing branch) Best regards, -- Artur Świgoń Samsung R&D Institute Poland Samsung Electronics ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC PATCH v3 4/7] arm: dts: exynos: Add interconnect bindings for Exynos4412 2019-12-31 9:41 ` Artur Świgoń @ 2019-12-31 10:02 ` Krzysztof Kozlowski 2019-12-31 10:23 ` Artur Świgoń 0 siblings, 1 reply; 31+ messages in thread From: Krzysztof Kozlowski @ 2019-12-31 10:02 UTC (permalink / raw) To: Artur Świgoń Cc: devicetree, linux-arm-kernel, linux-samsung-soc, linux-kernel, linux-pm, dri-devel, cw00.choi, myungjoo.ham, inki.dae, sw0312.kim, georgi.djakov, leonard.crestez, m.szyprowski, b.zolnierkie On Tue, Dec 31, 2019 at 10:41:47AM +0100, Artur Świgoń wrote: > On Tue, 2019-12-31 at 10:22 +0100, Krzysztof Kozlowski wrote: > > On Tue, Dec 31, 2019 at 08:18:01AM +0100, Artur Świgoń wrote: > > > Hi, > > > > > > On Mon, 2019-12-30 at 16:44 +0100, Krzysztof Kozlowski wrote: > > > > On Fri, Dec 20, 2019 at 12:56:50PM +0100, Artur Świgoń wrote: > > > > > This patch adds the following properties to the Exynos4412 DT: > > > > > - exynos,interconnect-parent-node: to declare connections between > > > > > nodes in order to guarantee PM QoS requirements between nodes; > > > > > - #interconnect-cells: required by the interconnect framework. > > > > > > > > > > Note that #interconnect-cells is always zero and node IDs are not > > > > > hardcoded anywhere. > > > > > > > > > > Signed-off-by: Artur Świgoń <a.swigon@samsung.com> > > > > > --- > > > > > arch/arm/boot/dts/exynos4412-odroid-common.dtsi | 5 +++++ > > > > > 1 file changed, 5 insertions(+) > > > > > > > > The order of patches is confusing. Patches 4 and 6 are split - do the > > > > depend on 5? I doubt but... > > > > > > Let me elaborate: > > > > > > The order of the patches in this series is such that every subsequent > > > patch adds some functionality (and, of course, applying patches one-by-one > > > yields a working kernel at every step). Specifically for patches 04--07: > > > > > > -- patch 04 adds interconnect _provider_ properties for Exynos4412; > > > -- patch 05 implements interconnect provider logic (depends on patch 04); > > > -- patch 06 adds interconnect _consumer_ properties for Exynos4412 mixer; > > > -- patch 07 implements interconnect consumer logic (depends on patches > > > 05 & 06); > > > > > > My reasoning is that this order allows to e.g., merge the interconnect > > > provider for exynos-bus and leave the consumers for later (not limited to > > > the mixer). I hope this makes sense. > > > > It is wrong. The driver should not depend on DTS changes because: > > 1. DTS always go through separate branch and tree, so last patch > > will have to wait up to 3 cycles (!!!), > > 2. You break backward compatibility. > > It is up to the definition of "depends". The driver is _not_ broken without > the DTS patches, but the interconnect functionality will not be available. > > The only requirement is that if we want to have a working interconnect > consumer, there needs to be a working interconnet provider (and I used > the word "depends" to specify what needs what in order to work as intended). > The order of patches should reflect first of all real dependency. Whether it compiles, works at all and does not break anything. Logical dependency of "when the feature will start working" is irrelevant to DTS because DTS goes in separate way and driver is independent of it. > I still think the order of these patches is the most logical one for someone > reading this RFC as a whole. I am sorry but it brings only confusion. DTS is orthogonal of the driver code. You could even post the patchset without DTS (although then it would raise questions where is the user of it, but still, you could). Further, DTS describes also hardware so you could send certain DTS patches without driver implementation to describe the hardware. Driver code and DTS are kind of different worlds so mixing them up for logical review does not really make any sense. Not mentioning it is different than most of other patches on mailing lists. BTW, it is the same as bindings which should (almost) always go first as separate patches. > > > In certain cases dependency on DTS changes is ok: > > 1. Cleaning up deprecated properties, > > 2. Ignoring the backward compatibility for e.g. new platforms. > > > > None of these are applicable here. > > > > You need to rework it, put DTS changes at the end. This clearly shows > > that there is no wrong dependency. > > > > > > > > > Adjust the title to match the contents - you are not adding bindings but > > > > properties to bus nodes. Also the prefix is ARM: (look at recent > > > > commits). > > > > > > OK. > > > > > > > > > > > > > diff --git a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi > > > > > index 4ce3d77a6704..d9d70eacfcaf 100644 > > > > > --- a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi > > > > > +++ b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi > > > > > @@ -90,6 +90,7 @@ > > > > > &bus_dmc { > > > > > exynos,ppmu-device = <&ppmu_dmc0_3>, <&ppmu_dmc1_3>; > > > > > vdd-supply = <&buck1_reg>; > > > > > + #interconnect-cells = <0>; > > > > > > > > This does not look like property of Odroid but Exynos4412 or Exynos4. > > > > > > Strangely enough, this file is where the 'exynos,parent-bus' (aka. 'devfreq') > > > properties are located (and everything in this RFC concerns devfreq). > > > > I cannot find exynos,parent-bus in exynos4412-odroid-common.dtsi. Can > > you elaborate? > > Currently a name change is being made: 'devfreq' -> 'exynos,parent-bus' > https://patchwork.kernel.org/patch/11304549/ > (a dependency of this RFC; also available in devfreq-testing branch) I see. That property also does not look like board (Odroid) specific so it should be moved to Exynos4412 DTSI. Best regards, Krzysztof ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC PATCH v3 4/7] arm: dts: exynos: Add interconnect bindings for Exynos4412 2019-12-31 10:02 ` Krzysztof Kozlowski @ 2019-12-31 10:23 ` Artur Świgoń 2019-12-31 10:38 ` Krzysztof Kozlowski 0 siblings, 1 reply; 31+ messages in thread From: Artur Świgoń @ 2019-12-31 10:23 UTC (permalink / raw) To: Krzysztof Kozlowski Cc: devicetree, linux-arm-kernel, linux-samsung-soc, linux-kernel, linux-pm, dri-devel, cw00.choi, myungjoo.ham, inki.dae, sw0312.kim, georgi.djakov, leonard.crestez, m.szyprowski, b.zolnierkie On Tue, 2019-12-31 at 11:02 +0100, Krzysztof Kozlowski wrote: > On Tue, Dec 31, 2019 at 10:41:47AM +0100, Artur Świgoń wrote: > > On Tue, 2019-12-31 at 10:22 +0100, Krzysztof Kozlowski wrote: > > > On Tue, Dec 31, 2019 at 08:18:01AM +0100, Artur Świgoń wrote: > > > > Hi, > > > > > > > > On Mon, 2019-12-30 at 16:44 +0100, Krzysztof Kozlowski wrote: > > > > > On Fri, Dec 20, 2019 at 12:56:50PM +0100, Artur Świgoń wrote: > > > > > > This patch adds the following properties to the Exynos4412 DT: > > > > > > - exynos,interconnect-parent-node: to declare connections between > > > > > > nodes in order to guarantee PM QoS requirements between nodes; > > > > > > - #interconnect-cells: required by the interconnect framework. > > > > > > > > > > > > Note that #interconnect-cells is always zero and node IDs are not > > > > > > hardcoded anywhere. > > > > > > > > > > > > Signed-off-by: Artur Świgoń <a.swigon@samsung.com> > > > > > > --- > > > > > > arch/arm/boot/dts/exynos4412-odroid-common.dtsi | 5 +++++ > > > > > > 1 file changed, 5 insertions(+) > > > > > > > > > > The order of patches is confusing. Patches 4 and 6 are split - do the > > > > > depend on 5? I doubt but... > > > > > > > > Let me elaborate: > > > > > > > > The order of the patches in this series is such that every subsequent > > > > patch adds some functionality (and, of course, applying patches one-by-one > > > > yields a working kernel at every step). Specifically for patches 04--07: > > > > > > > > -- patch 04 adds interconnect _provider_ properties for Exynos4412; > > > > -- patch 05 implements interconnect provider logic (depends on patch 04); > > > > -- patch 06 adds interconnect _consumer_ properties for Exynos4412 mixer; > > > > -- patch 07 implements interconnect consumer logic (depends on patches > > > > 05 & 06); > > > > > > > > My reasoning is that this order allows to e.g., merge the interconnect > > > > provider for exynos-bus and leave the consumers for later (not limited to > > > > the mixer). I hope this makes sense. > > > > > > It is wrong. The driver should not depend on DTS changes because: > > > 1. DTS always go through separate branch and tree, so last patch > > > will have to wait up to 3 cycles (!!!), > > > 2. You break backward compatibility. > > > > It is up to the definition of "depends". The driver is _not_ broken without > > the DTS patches, but the interconnect functionality will not be available. > > > > The only requirement is that if we want to have a working interconnect > > consumer, there needs to be a working interconnet provider (and I used > > the word "depends" to specify what needs what in order to work as intended). > > > > The order of patches should reflect first of all real dependency. > Whether it compiles, works at all and does not break anything. Logical > dependency of "when the feature will start working" is > irrelevant to DTS because DTS goes in separate way and driver is > independent of it. The order of patches does indeed reflect real dependency. I can also reorder them (preserving the dependencies) so that DTS patches go first in the series if this is the more preferred way. > > I still think the order of these patches is the most logical one for someone > > reading this RFC as a whole. > > I am sorry but it brings only confusion. DTS is orthogonal of the > driver code. You could even post the patchset without DTS (although then > it would raise questions where is the user of it, but still, you > could). > > Further, DTS describes also hardware so you could send certain DTS > patches without driver implementation to describe the hardware. > > Driver code and DTS are kind of different worlds so mixing them up for > logical review does not really make any sense. > > Not mentioning it is different than most of other patches on mailing > lists. > > BTW, it is the same as bindings which should (almost) always go first as > separate patches. Thanks for elaborating on this, I appreciate it. Regarding your original concern, patches 04 & 06 are separate for several reasons, one of which is that they are related to two different drivers (exynos-bus vs. exynos-mixer). > > > > > In certain cases dependency on DTS changes is ok: > > > 1. Cleaning up deprecated properties, > > > 2. Ignoring the backward compatibility for e.g. new platforms. > > > > > > None of these are applicable here. > > > > > > You need to rework it, put DTS changes at the end. This clearly shows > > > that there is no wrong dependency. > > > > > > > > > > > > Adjust the title to match the contents - you are not adding bindings but > > > > > properties to bus nodes. Also the prefix is ARM: (look at recent > > > > > commits). > > > > > > > > OK. > > > > > > > > > > > > > > > > diff --git a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi > > > > > > index 4ce3d77a6704..d9d70eacfcaf 100644 > > > > > > --- a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi > > > > > > +++ b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi > > > > > > @@ -90,6 +90,7 @@ > > > > > > &bus_dmc { > > > > > > exynos,ppmu-device = <&ppmu_dmc0_3>, <&ppmu_dmc1_3>; > > > > > > vdd-supply = <&buck1_reg>; > > > > > > + #interconnect-cells = <0>; > > > > > > > > > > This does not look like property of Odroid but Exynos4412 or Exynos4. > > > > > > > > Strangely enough, this file is where the 'exynos,parent-bus' (aka. 'devfreq') > > > > properties are located (and everything in this RFC concerns devfreq). > > > > > > I cannot find exynos,parent-bus in exynos4412-odroid-common.dtsi. Can > > > you elaborate? > > > > Currently a name change is being made: 'devfreq' -> 'exynos,parent-bus' > > https://patchwork.kernel.org/patch/11304549/ > > (a dependency of this RFC; also available in devfreq-testing branch) > > I see. That property also does not look like board (Odroid) specific so > it should be moved to Exynos4412 DTSI. Makes sense to me. Just from looking at the patch I referenced above, there is a significant level of code duplication between * arch/arm/boot/dts/exynos4412-itop-scp-core.dtsi * arch/arm/boot/dts/exynos4412-midas.dtsi * arch/arm/boot/dts/exynos4412-odroid-common.dtsi with relation to the devfreq*/exynos,* properties. -- Artur Świgoń Samsung R&D Institute Poland Samsung Electronics ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC PATCH v3 4/7] arm: dts: exynos: Add interconnect bindings for Exynos4412 2019-12-31 10:23 ` Artur Świgoń @ 2019-12-31 10:38 ` Krzysztof Kozlowski 2019-12-31 11:03 ` Artur Świgoń 0 siblings, 1 reply; 31+ messages in thread From: Krzysztof Kozlowski @ 2019-12-31 10:38 UTC (permalink / raw) To: Artur Świgoń Cc: devicetree, linux-arm-kernel, linux-samsung-soc, linux-kernel, linux-pm, dri-devel, Chanwoo Choi, myungjoo.ham, Inki Dae, Seung Woo Kim, georgi.djakov, leonard.crestez, Marek Szyprowski, Bartłomiej Żołnierkiewicz On Tue, 31 Dec 2019 at 11:23, Artur Świgoń <a.swigon@samsung.com> wrote: > > > > The order of patches should reflect first of all real dependency. > > Whether it compiles, works at all and does not break anything. Logical > > dependency of "when the feature will start working" is > > irrelevant to DTS because DTS goes in separate way and driver is > > independent of it. > > The order of patches does indeed reflect real dependency. I can also reorder > them (preserving the dependencies) so that DTS patches go first in the series > if this is the more preferred way. It looks wrong then. Driver should not depend on DTS. I cannot find the patch changing bindings (should be first in patchset) which could also point to this problem. It seems you added requirement for interconnect properties while it should be rather optional. > > > I still think the order of these patches is the most logical one for someone > > > reading this RFC as a whole. > > > > I am sorry but it brings only confusion. DTS is orthogonal of the > > driver code. You could even post the patchset without DTS (although then > > it would raise questions where is the user of it, but still, you > > could). > > > > Further, DTS describes also hardware so you could send certain DTS > > patches without driver implementation to describe the hardware. > > > > Driver code and DTS are kind of different worlds so mixing them up for > > logical review does not really make any sense. > > > > Not mentioning it is different than most of other patches on mailing > > lists. > > > > BTW, it is the same as bindings which should (almost) always go first as > > separate patches. > > Thanks for elaborating on this, I appreciate it. > Regarding your original concern, patches 04 & 06 are separate for several > reasons, one of which is that they are related to two different drivers > (exynos-bus vs. exynos-mixer). It's okay then (for them to be split). > > > > > > > > In certain cases dependency on DTS changes is ok: > > > > 1. Cleaning up deprecated properties, > > > > 2. Ignoring the backward compatibility for e.g. new platforms. > > > > > > > > None of these are applicable here. > > > > > > > > You need to rework it, put DTS changes at the end. This clearly shows > > > > that there is no wrong dependency. > > > > > > > > > > > > > > > Adjust the title to match the contents - you are not adding bindings but > > > > > > properties to bus nodes. Also the prefix is ARM: (look at recent > > > > > > commits). > > > > > > > > > > OK. > > > > > > > > > > > > > > > > > > > diff --git a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi > > > > > > > index 4ce3d77a6704..d9d70eacfcaf 100644 > > > > > > > --- a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi > > > > > > > +++ b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi > > > > > > > @@ -90,6 +90,7 @@ > > > > > > > &bus_dmc { > > > > > > > exynos,ppmu-device = <&ppmu_dmc0_3>, <&ppmu_dmc1_3>; > > > > > > > vdd-supply = <&buck1_reg>; > > > > > > > + #interconnect-cells = <0>; > > > > > > > > > > > > This does not look like property of Odroid but Exynos4412 or Exynos4. > > > > > > > > > > Strangely enough, this file is where the 'exynos,parent-bus' (aka. 'devfreq') > > > > > properties are located (and everything in this RFC concerns devfreq). > > > > > > > > I cannot find exynos,parent-bus in exynos4412-odroid-common.dtsi. Can > > > > you elaborate? > > > > > > Currently a name change is being made: 'devfreq' -> 'exynos,parent-bus' > > > https://patchwork.kernel.org/patch/11304549/ > > > (a dependency of this RFC; also available in devfreq-testing branch) > > > > I see. That property also does not look like board (Odroid) specific so > > it should be moved to Exynos4412 DTSI. > > Makes sense to me. Just from looking at the patch I referenced above, there is > a significant level of code duplication between > * arch/arm/boot/dts/exynos4412-itop-scp-core.dtsi > * arch/arm/boot/dts/exynos4412-midas.dtsi > * arch/arm/boot/dts/exynos4412-odroid-common.dtsi > with relation to the devfreq*/exynos,* properties. If you have in mind all the nodes with "status=okay", it's fine to duplicate them. Best regards, Krzysztof ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC PATCH v3 4/7] arm: dts: exynos: Add interconnect bindings for Exynos4412 2019-12-31 10:38 ` Krzysztof Kozlowski @ 2019-12-31 11:03 ` Artur Świgoń 0 siblings, 0 replies; 31+ messages in thread From: Artur Świgoń @ 2019-12-31 11:03 UTC (permalink / raw) To: Krzysztof Kozlowski Cc: devicetree, linux-arm-kernel, linux-samsung-soc, linux-kernel, linux-pm, dri-devel, Chanwoo Choi, myungjoo.ham, Inki Dae, Seung Woo Kim, georgi.djakov, leonard.crestez, Marek Szyprowski, Bartłomiej Żołnierkiewicz On Tue, 2019-12-31 at 11:38 +0100, Krzysztof Kozlowski wrote: > On Tue, 31 Dec 2019 at 11:23, Artur Świgoń <a.swigon@samsung.com> wrote: > > > > > > The order of patches should reflect first of all real dependency. > > > Whether it compiles, works at all and does not break anything. Logical > > > dependency of "when the feature will start working" is > > > irrelevant to DTS because DTS goes in separate way and driver is > > > independent of it. > > > > The order of patches does indeed reflect real dependency. I can also reorder > > them (preserving the dependencies) so that DTS patches go first in the series > > if this is the more preferred way. > > It looks wrong then. Driver should not depend on DTS. I cannot find > the patch changing bindings (should be first in patchset) which could > also point to this problem. > > It seems you added requirement for interconnect properties while it > should be rather optional. No, there is no requirement for interconnect properties (other than that it simply does not make any sense to use the interconnect driver code and not the DTS properties for it in the long run). In case of the exynos-bus driver (code: patch 05, DTS: patch 04) if the DTS properties ('exynos,interconnect-parent-node') are missing, the new code handles it gracefully returning NULL from exynos_bus_icc_get_parent() (it is not an error condition). In case of the exynos-mixer driver (code: patch 07, DTS: patch 06) if the DTS property ('interconnects') is missing, of_icc_get() returns NULL and the code does not try to set any contraints for a NULL path. Same thing happens if CONFIG_INTERCONNECT is 'n'. The only case when something breaks is when you try to use the interconnect consumer (implemented in patches 06 & 07) when there is no interconnect provider (patches 04 & 05), in which case of_icc_get() returns an error (since it cannot find a path). From what I understand, it probably makes sense to merge any interconnect consumers one cycle later than the provider. > > > > I still think the order of these patches is the most logical one for someone > > > > reading this RFC as a whole. > > > > > > I am sorry but it brings only confusion. DTS is orthogonal of the > > > driver code. You could even post the patchset without DTS (although then > > > it would raise questions where is the user of it, but still, you > > > could). > > > > > > Further, DTS describes also hardware so you could send certain DTS > > > patches without driver implementation to describe the hardware. > > > > > > Driver code and DTS are kind of different worlds so mixing them up for > > > logical review does not really make any sense. > > > > > > Not mentioning it is different than most of other patches on mailing > > > lists. > > > > > > BTW, it is the same as bindings which should (almost) always go first as > > > separate patches. > > > > Thanks for elaborating on this, I appreciate it. > > Regarding your original concern, patches 04 & 06 are separate for several > > reasons, one of which is that they are related to two different drivers > > (exynos-bus vs. exynos-mixer). > > It's okay then (for them to be split). > > > > > > > > > > > > In certain cases dependency on DTS changes is ok: > > > > > 1. Cleaning up deprecated properties, > > > > > 2. Ignoring the backward compatibility for e.g. new platforms. > > > > > > > > > > None of these are applicable here. > > > > > > > > > > You need to rework it, put DTS changes at the end. This clearly shows > > > > > that there is no wrong dependency. > > > > > > > > > > > > > > > > > > Adjust the title to match the contents - you are not adding bindings but > > > > > > > properties to bus nodes. Also the prefix is ARM: (look at recent > > > > > > > commits). > > > > > > > > > > > > OK. > > > > > > > > > > > > > > > > > > > > > > diff --git a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi > > > > > > > > index 4ce3d77a6704..d9d70eacfcaf 100644 > > > > > > > > --- a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi > > > > > > > > +++ b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi > > > > > > > > @@ -90,6 +90,7 @@ > > > > > > > > &bus_dmc { > > > > > > > > exynos,ppmu-device = <&ppmu_dmc0_3>, <&ppmu_dmc1_3>; > > > > > > > > vdd-supply = <&buck1_reg>; > > > > > > > > + #interconnect-cells = <0>; > > > > > > > > > > > > > > This does not look like property of Odroid but Exynos4412 or Exynos4. > > > > > > > > > > > > Strangely enough, this file is where the 'exynos,parent-bus' (aka. 'devfreq') > > > > > > properties are located (and everything in this RFC concerns devfreq). > > > > > > > > > > I cannot find exynos,parent-bus in exynos4412-odroid-common.dtsi. Can > > > > > you elaborate? > > > > > > > > Currently a name change is being made: 'devfreq' -> 'exynos,parent-bus' > > > > https://patchwork.kernel.org/patch/11304549/ > > > > (a dependency of this RFC; also available in devfreq-testing branch) > > > > > > I see. That property also does not look like board (Odroid) specific so > > > it should be moved to Exynos4412 DTSI. > > > > Makes sense to me. Just from looking at the patch I referenced above, there is > > a significant level of code duplication between > > * arch/arm/boot/dts/exynos4412-itop-scp-core.dtsi > > * arch/arm/boot/dts/exynos4412-midas.dtsi > > * arch/arm/boot/dts/exynos4412-odroid-common.dtsi > > with relation to the devfreq*/exynos,* properties. > > If you have in mind all the nodes with "status=okay", it's fine to > duplicate them. OK. Regards, -- Artur Świgoń Samsung R&D Institute Poland Samsung Electronics ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC PATCH v3 4/7] arm: dts: exynos: Add interconnect bindings for Exynos4412 2019-12-20 11:56 ` [RFC PATCH v3 4/7] arm: dts: exynos: Add interconnect bindings for Exynos4412 Artur Świgoń 2019-12-21 20:00 ` Chanwoo Choi 2019-12-30 15:44 ` Krzysztof Kozlowski @ 2020-01-22 16:54 ` Georgi Djakov 2020-01-24 11:22 ` Artur Świgoń 2 siblings, 1 reply; 31+ messages in thread From: Georgi Djakov @ 2020-01-22 16:54 UTC (permalink / raw) To: Artur Świgoń, devicetree, linux-arm-kernel, linux-samsung-soc, linux-kernel, linux-pm, dri-devel Cc: cw00.choi, myungjoo.ham, inki.dae, sw0312.kim, leonard.crestez, m.szyprowski, b.zolnierkie, krzk Hi Artur, Thank you for your continuous work on this. On 12/20/19 13:56, Artur Świgoń wrote: > This patch adds the following properties to the Exynos4412 DT: > - exynos,interconnect-parent-node: to declare connections between > nodes in order to guarantee PM QoS requirements between nodes; Is this DT property documented somewhere? I believe that there should be a patch to document it somewhere in Documentation/devicetree/bindings/ before using it. Thanks, Georgi > - #interconnect-cells: required by the interconnect framework. > > Note that #interconnect-cells is always zero and node IDs are not > hardcoded anywhere. > > Signed-off-by: Artur Świgoń <a.swigon@samsung.com> > --- > arch/arm/boot/dts/exynos4412-odroid-common.dtsi | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi > index 4ce3d77a6704..d9d70eacfcaf 100644 > --- a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi > +++ b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi > @@ -90,6 +90,7 @@ > &bus_dmc { > exynos,ppmu-device = <&ppmu_dmc0_3>, <&ppmu_dmc1_3>; > vdd-supply = <&buck1_reg>; > + #interconnect-cells = <0>; > status = "okay"; > }; > > @@ -106,6 +107,8 @@ > &bus_leftbus { > exynos,ppmu-device = <&ppmu_leftbus_3>, <&ppmu_rightbus_3>; > vdd-supply = <&buck3_reg>; > + exynos,interconnect-parent-node = <&bus_dmc>; > + #interconnect-cells = <0>; > status = "okay"; > }; > > @@ -116,6 +119,8 @@ > > &bus_display { > exynos,parent-bus = <&bus_leftbus>; > + exynos,interconnect-parent-node = <&bus_leftbus>; > + #interconnect-cells = <0>; > status = "okay"; > }; > > ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC PATCH v3 4/7] arm: dts: exynos: Add interconnect bindings for Exynos4412 2020-01-22 16:54 ` Georgi Djakov @ 2020-01-24 11:22 ` Artur Świgoń 0 siblings, 0 replies; 31+ messages in thread From: Artur Świgoń @ 2020-01-24 11:22 UTC (permalink / raw) To: Georgi Djakov, devicetree, linux-arm-kernel, linux-samsung-soc, linux-kernel, linux-pm, dri-devel Cc: cw00.choi, myungjoo.ham, inki.dae, sw0312.kim, leonard.crestez, m.szyprowski, b.zolnierkie, krzk Hi, On Wed, 2020-01-22 at 18:54 +0200, Georgi Djakov wrote: > Hi Artur, > > Thank you for your continuous work on this. > > On 12/20/19 13:56, Artur Świgoń wrote: > > This patch adds the following properties to the Exynos4412 DT: > > - exynos,interconnect-parent-node: to declare connections between > > nodes in order to guarantee PM QoS requirements between nodes; > > Is this DT property documented somewhere? I believe that there should be a patch > to document it somewhere in Documentation/devicetree/bindings/ before using it. It will be documented in Documentation/devicetree/bindings/devfreq/exynos-bus.txt in the next version. > > - #interconnect-cells: required by the interconnect framework. > > > > Note that #interconnect-cells is always zero and node IDs are not > > hardcoded anywhere. > > > > Signed-off-by: Artur Świgoń <a.swigon@samsung.com> > > --- > > arch/arm/boot/dts/exynos4412-odroid-common.dtsi | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi > > index 4ce3d77a6704..d9d70eacfcaf 100644 > > --- a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi > > +++ b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi > > @@ -90,6 +90,7 @@ > > &bus_dmc { > > exynos,ppmu-device = <&ppmu_dmc0_3>, <&ppmu_dmc1_3>; > > vdd-supply = <&buck1_reg>; > > + #interconnect-cells = <0>; > > status = "okay"; > > }; > > > > @@ -106,6 +107,8 @@ > > &bus_leftbus { > > exynos,ppmu-device = <&ppmu_leftbus_3>, <&ppmu_rightbus_3>; > > vdd-supply = <&buck3_reg>; > > + exynos,interconnect-parent-node = <&bus_dmc>; > > + #interconnect-cells = <0>; > > status = "okay"; > > }; > > > > @@ -116,6 +119,8 @@ > > > > &bus_display { > > exynos,parent-bus = <&bus_leftbus>; > > + exynos,interconnect-parent-node = <&bus_leftbus>; > > + #interconnect-cells = <0>; > > status = "okay"; > > }; -- Artur Świgoń Samsung R&D Institute Poland Samsung Electronics ^ permalink raw reply [flat|nested] 31+ messages in thread
[parent not found: <CGME20191220120145eucas1p295af63eed7b23982d8c49fcf875cec8c@eucas1p2.samsung.com>]
* [RFC PATCH v3 5/7] devfreq: exynos-bus: Add interconnect functionality to exynos-bus [not found] ` <CGME20191220120145eucas1p295af63eed7b23982d8c49fcf875cec8c@eucas1p2.samsung.com> @ 2019-12-20 11:56 ` Artur Świgoń 2019-12-21 19:53 ` Chanwoo Choi 2020-01-22 17:02 ` Georgi Djakov 0 siblings, 2 replies; 31+ messages in thread From: Artur Świgoń @ 2019-12-20 11:56 UTC (permalink / raw) To: devicetree, linux-arm-kernel, linux-samsung-soc, linux-kernel, linux-pm, dri-devel Cc: Artur Świgoń, cw00.choi, myungjoo.ham, inki.dae, sw0312.kim, georgi.djakov, leonard.crestez, m.szyprowski, b.zolnierkie, krzk This patch adds interconnect functionality to the exynos-bus devfreq driver. The SoC topology is a graph (or, more specifically, a tree) and its edges are specified using the 'exynos,interconnect-parent-node' 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 at runtime, in probing order (subject to the above-mentioned exception regarding relative order). 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> --- drivers/devfreq/exynos-bus.c | 144 +++++++++++++++++++++++++++++++++++ 1 file changed, 144 insertions(+) diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c index 9fdb188915e8..694a9581dcdb 100644 --- a/drivers/devfreq/exynos-bus.c +++ b/drivers/devfreq/exynos-bus.c @@ -14,14 +14,19 @@ #include <linux/devfreq-event.h> #include <linux/device.h> #include <linux/export.h> +#include <linux/idr.h> +#include <linux/interconnect-provider.h> #include <linux/module.h> #include <linux/of.h> #include <linux/pm_opp.h> +#include <linux/pm_qos.h> #include <linux/platform_device.h> #include <linux/regulator/consumer.h> #define DEFAULT_SATURATION_RATIO 40 +#define kbps_to_khz(x) ((x) / 8) + struct exynos_bus { struct device *dev; @@ -35,6 +40,12 @@ struct exynos_bus { struct opp_table *opp_table; struct clk *clk; unsigned int ratio; + + /* One provider per bus, one node per provider */ + struct icc_provider provider; + struct icc_node *node; + + struct dev_pm_qos_request qos_req; }; /* @@ -205,6 +216,39 @@ static void exynos_bus_passive_exit(struct device *dev) clk_disable_unprepare(bus->clk); } +static int exynos_bus_icc_set(struct icc_node *src, struct icc_node *dst) +{ + struct exynos_bus *src_bus = src->data, *dst_bus = dst->data; + s32 src_freq = kbps_to_khz(src->avg_bw); + s32 dst_freq = kbps_to_khz(dst->avg_bw); + int ret; + + ret = dev_pm_qos_update_request(&src_bus->qos_req, src_freq); + if (ret < 0) { + dev_err(src_bus->dev, "failed to update PM QoS request"); + return ret; + } + + ret = dev_pm_qos_update_request(&dst_bus->qos_req, dst_freq); + if (ret < 0) { + dev_err(dst_bus->dev, "failed to update PM QoS request"); + return ret; + } + + return 0; +} + +static struct icc_node *exynos_bus_icc_xlate(struct of_phandle_args *spec, + void *data) +{ + struct exynos_bus *bus = data; + + if (spec->np != bus->dev->of_node) + return ERR_PTR(-EINVAL); + + return bus->node; +} + static int exynos_bus_parent_parse_of(struct device_node *np, struct exynos_bus *bus) { @@ -419,6 +463,96 @@ static int exynos_bus_profile_init_passive(struct exynos_bus *bus, return 0; } +static struct icc_node *exynos_bus_icc_get_parent(struct exynos_bus *bus) +{ + struct device_node *np = bus->dev->of_node; + struct of_phandle_args args; + int num, ret; + + num = of_count_phandle_with_args(np, "exynos,interconnect-parent-node", + "#interconnect-cells"); + if (num != 1) + return NULL; /* parent nodes are optional */ + + ret = of_parse_phandle_with_args(np, "exynos,interconnect-parent-node", + "#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_bus_icc_init(struct exynos_bus *bus) +{ + static DEFINE_IDA(ida); + + struct device *dev = bus->dev; + struct icc_provider *provider = &bus->provider; + struct icc_node *node, *parent_node; + int id, ret; + + /* Initialize the interconnect provider */ + provider->set = exynos_bus_icc_set; + provider->aggregate = icc_std_aggregate; + provider->xlate = exynos_bus_icc_xlate; + provider->dev = dev; + provider->inter_set = true; + provider->data = bus; + + ret = icc_provider_add(provider); + if (ret < 0) + return ret; + + ret = id = ida_alloc(&ida, GFP_KERNEL); + if (ret < 0) + goto err_id; + + node = icc_node_create(id); + if (IS_ERR(node)) { + ret = PTR_ERR(node); + goto err_node; + } + + bus->node = node; + node->name = dev->of_node->name; + node->data = bus; + icc_node_add(node, provider); + + parent_node = exynos_bus_icc_get_parent(bus); + if (IS_ERR(parent_node)) { + ret = PTR_ERR(parent_node); + goto err_parent; + } + + if (parent_node) { + ret = icc_link_create(node, parent_node->id); + if (ret < 0) + goto err_parent; + } + + ret = dev_pm_qos_add_request(bus->devfreq->dev.parent, &bus->qos_req, + DEV_PM_QOS_MIN_FREQUENCY, 0); + if (ret < 0) + goto err_request; + + return 0; + +err_request: + if (parent_node) + icc_link_destroy(node, parent_node); +err_parent: + icc_node_del(node); + icc_node_destroy(id); +err_node: + ida_free(&ida, id); +err_id: + icc_provider_del(provider); + + return ret; +} + static int exynos_bus_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; @@ -468,6 +602,16 @@ static int exynos_bus_probe(struct platform_device *pdev) if (ret < 0) goto err; + /* + * Initialize interconnect provider. A return value of -ENOTSUPP means + * that CONFIG_INTERCONNECT is disabled. + */ + ret = exynos_bus_icc_init(bus); + if (ret < 0 && ret != -ENOTSUPP) { + dev_err(dev, "failed to initialize the interconnect provider"); + 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.17.1 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [RFC PATCH v3 5/7] devfreq: exynos-bus: Add interconnect functionality to exynos-bus 2019-12-20 11:56 ` [RFC PATCH v3 5/7] devfreq: exynos-bus: Add interconnect functionality to exynos-bus Artur Świgoń @ 2019-12-21 19:53 ` Chanwoo Choi 2019-12-21 19:55 ` Chanwoo Choi 2020-01-22 17:02 ` Georgi Djakov 1 sibling, 1 reply; 31+ messages in thread From: Chanwoo Choi @ 2019-12-21 19:53 UTC (permalink / raw) To: Artur Świgoń Cc: devicetree, linux-arm-kernel, linux-samsung-soc, linux-kernel, Linux PM list, dri-devel, Chanwoo Choi, MyungJoo Ham, inki.dae, Seung-Woo Kim, Georgi Djakov, Leonard Crestez, Marek Szyprowski, Bartlomiej Zolnierkiewicz, Krzysztof Kozlowski Hi, On Fri, Dec 20, 2019 at 9:02 PM Artur Świgoń <a.swigon@samsung.com> wrote: > > This patch adds interconnect functionality to the exynos-bus devfreq > driver. > > The SoC topology is a graph (or, more specifically, a tree) and its > edges are specified using the 'exynos,interconnect-parent-node' 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 at > runtime, in probing order (subject to the above-mentioned exception > regarding relative order). 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> > --- > drivers/devfreq/exynos-bus.c | 144 +++++++++++++++++++++++++++++++++++ > 1 file changed, 144 insertions(+) > > diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c > index 9fdb188915e8..694a9581dcdb 100644 > --- a/drivers/devfreq/exynos-bus.c > +++ b/drivers/devfreq/exynos-bus.c > @@ -14,14 +14,19 @@ > #include <linux/devfreq-event.h> > #include <linux/device.h> > #include <linux/export.h> > +#include <linux/idr.h> > +#include <linux/interconnect-provider.h> > #include <linux/module.h> > #include <linux/of.h> > #include <linux/pm_opp.h> > +#include <linux/pm_qos.h> > #include <linux/platform_device.h> > #include <linux/regulator/consumer.h> > > #define DEFAULT_SATURATION_RATIO 40 > > +#define kbps_to_khz(x) ((x) / 8) > + > struct exynos_bus { > struct device *dev; > > @@ -35,6 +40,12 @@ struct exynos_bus { > struct opp_table *opp_table; > struct clk *clk; > unsigned int ratio; > + > + /* One provider per bus, one node per provider */ > + struct icc_provider provider; > + struct icc_node *node; > + > + struct dev_pm_qos_request qos_req; > }; > > /* > @@ -205,6 +216,39 @@ static void exynos_bus_passive_exit(struct device *dev) > clk_disable_unprepare(bus->clk); > } > > +static int exynos_bus_icc_set(struct icc_node *src, struct icc_node *dst) > +{ > + struct exynos_bus *src_bus = src->data, *dst_bus = dst->data; > + s32 src_freq = kbps_to_khz(src->avg_bw); > + s32 dst_freq = kbps_to_khz(dst->avg_bw); > + int ret; > + > + ret = dev_pm_qos_update_request(&src_bus->qos_req, src_freq); > + if (ret < 0) { > + dev_err(src_bus->dev, "failed to update PM QoS request"); To catch the correct error point, better to add 'src node' to error message as following: dev_err(src_bus->dev, "failed to update PM QoS of %s\n", dev_name(src_bus->dev.parent)) > + return ret; > + } > + > + ret = dev_pm_qos_update_request(&dst_bus->qos_req, dst_freq); > + if (ret < 0) { > + dev_err(dst_bus->dev, "failed to update PM QoS request"); ditto. dev_err(src_bus->dev, "failed to update PM QoS of %s\n", dev_name(dst_bus->dev.parent)) > + return ret; > + } > + > + return 0; > +} > + > +static struct icc_node *exynos_bus_icc_xlate(struct of_phandle_args *spec, > + void *data) > +{ > + struct exynos_bus *bus = data; > + > + if (spec->np != bus->dev->of_node) > + return ERR_PTR(-EINVAL); > + > + return bus->node; > +} > + > static int exynos_bus_parent_parse_of(struct device_node *np, > struct exynos_bus *bus) > { > @@ -419,6 +463,96 @@ static int exynos_bus_profile_init_passive(struct exynos_bus *bus, > return 0; > } > > +static struct icc_node *exynos_bus_icc_get_parent(struct exynos_bus *bus) > +{ > + struct device_node *np = bus->dev->of_node; > + struct of_phandle_args args; > + int num, ret; > + > + num = of_count_phandle_with_args(np, "exynos,interconnect-parent-node", > + "#interconnect-cells"); > + if (num != 1) > + return NULL; /* parent nodes are optional */ You better to add the comment before calling exynos_bus_icc_get_parent and remove '/* parent nodes are optional */'. Actually, it is not enough to understand the role of 'interconnect-parent-node' with '/* parent nodes are optional */'. And I add my opinion about this comment below. And I expect that you will add the description and example for 'exynos,interconnect-parent-node' on exynos-bus dt-binding document. > + > + ret = of_parse_phandle_with_args(np, "exynos,interconnect-parent-node", > + "#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_bus_icc_init(struct exynos_bus *bus) > +{ > + static DEFINE_IDA(ida); > + > + struct device *dev = bus->dev; > + struct icc_provider *provider = &bus->provider; > + struct icc_node *node, *parent_node; > + int id, ret; > + > + /* Initialize the interconnect provider */ > + provider->set = exynos_bus_icc_set; > + provider->aggregate = icc_std_aggregate; > + provider->xlate = exynos_bus_icc_xlate; > + provider->dev = dev; > + provider->inter_set = true; > + provider->data = bus; > + > + ret = icc_provider_add(provider); > + if (ret < 0) > + return ret; > + > + ret = id = ida_alloc(&ida, GFP_KERNEL); > + if (ret < 0) > + goto err_id; > + > + node = icc_node_create(id); > + if (IS_ERR(node)) { > + ret = PTR_ERR(node); > + goto err_node; > + } > + > + bus->node = node; > + node->name = dev->of_node->name; > + node->data = bus; > + icc_node_add(node, provider); > + Better to add the following comment. If you add following comment before calling exynos_bus_icc_get_parent, don't need to add the same comment into exynos_bus_icc. /* If interconnect parent node is not existing, don't use interconnect feature */ > + parent_node = exynos_bus_icc_get_parent(bus); > + if (IS_ERR(parent_node)) { > + ret = PTR_ERR(parent_node); > + goto err_parent; > + } > + > + if (parent_node) { Better to change this if statement as following: else if (parent_node) > + ret = icc_link_create(node, parent_node->id); > + if (ret < 0) > + goto err_parent; > + } > + > + ret = dev_pm_qos_add_request(bus->devfreq->dev.parent, &bus->qos_req, > + DEV_PM_QOS_MIN_FREQUENCY, 0); Is it necessary if interconnect-parent-node is not existing? > + if (ret < 0) > + goto err_request; > + > + return 0; > + > +err_request: > + if (parent_node) > + icc_link_destroy(node, parent_node); > +err_parent: > + icc_node_del(node); > + icc_node_destroy(id); > +err_node: > + ida_free(&ida, id); > +err_id: > + icc_provider_del(provider); > + > + return ret; > +} > + > static int exynos_bus_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > @@ -468,6 +602,16 @@ static int exynos_bus_probe(struct platform_device *pdev) > if (ret < 0) > goto err; > > + /* > + * Initialize interconnect provider. A return value of -ENOTSUPP means > + * that CONFIG_INTERCONNECT is disabled. > + */ > + ret = exynos_bus_icc_init(bus); > + if (ret < 0 && ret != -ENOTSUPP) { > + dev_err(dev, "failed to initialize the interconnect provider"); > + 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.17.1 > -- Best Regards, Chanwoo Choi ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC PATCH v3 5/7] devfreq: exynos-bus: Add interconnect functionality to exynos-bus 2019-12-21 19:53 ` Chanwoo Choi @ 2019-12-21 19:55 ` Chanwoo Choi 0 siblings, 0 replies; 31+ messages in thread From: Chanwoo Choi @ 2019-12-21 19:55 UTC (permalink / raw) To: Artur Świgoń Cc: devicetree, linux-arm-kernel, linux-samsung-soc, linux-kernel, Linux PM list, dri-devel, Chanwoo Choi, MyungJoo Ham, inki.dae, Seung-Woo Kim, Georgi Djakov, Leonard Crestez, Marek Szyprowski, Bartlomiej Zolnierkiewicz, Krzysztof Kozlowski On Sun, Dec 22, 2019 at 4:53 AM Chanwoo Choi <chanwoo@kernel.org> wrote: > > Hi, > > On Fri, Dec 20, 2019 at 9:02 PM Artur Świgoń <a.swigon@samsung.com> wrote: > > > > This patch adds interconnect functionality to the exynos-bus devfreq > > driver. > > > > The SoC topology is a graph (or, more specifically, a tree) and its > > edges are specified using the 'exynos,interconnect-parent-node' 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 at > > runtime, in probing order (subject to the above-mentioned exception > > regarding relative order). 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> > > --- > > drivers/devfreq/exynos-bus.c | 144 +++++++++++++++++++++++++++++++++++ > > 1 file changed, 144 insertions(+) > > > > diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c > > index 9fdb188915e8..694a9581dcdb 100644 > > --- a/drivers/devfreq/exynos-bus.c > > +++ b/drivers/devfreq/exynos-bus.c > > @@ -14,14 +14,19 @@ > > #include <linux/devfreq-event.h> > > #include <linux/device.h> > > #include <linux/export.h> > > +#include <linux/idr.h> > > +#include <linux/interconnect-provider.h> > > #include <linux/module.h> > > #include <linux/of.h> > > #include <linux/pm_opp.h> > > +#include <linux/pm_qos.h> > > #include <linux/platform_device.h> > > #include <linux/regulator/consumer.h> > > > > #define DEFAULT_SATURATION_RATIO 40 > > > > +#define kbps_to_khz(x) ((x) / 8) > > + > > struct exynos_bus { > > struct device *dev; > > > > @@ -35,6 +40,12 @@ struct exynos_bus { > > struct opp_table *opp_table; > > struct clk *clk; > > unsigned int ratio; > > + > > + /* One provider per bus, one node per provider */ > > + struct icc_provider provider; > > + struct icc_node *node; > > + > > + struct dev_pm_qos_request qos_req; > > }; > > > > /* > > @@ -205,6 +216,39 @@ static void exynos_bus_passive_exit(struct device *dev) > > clk_disable_unprepare(bus->clk); > > } > > > > +static int exynos_bus_icc_set(struct icc_node *src, struct icc_node *dst) > > +{ > > + struct exynos_bus *src_bus = src->data, *dst_bus = dst->data; > > + s32 src_freq = kbps_to_khz(src->avg_bw); > > + s32 dst_freq = kbps_to_khz(dst->avg_bw); > > + int ret; > > + > > + ret = dev_pm_qos_update_request(&src_bus->qos_req, src_freq); > > + if (ret < 0) { > > + dev_err(src_bus->dev, "failed to update PM QoS request"); > > To catch the correct error point, better to add 'src node' to error message > as following: > > dev_err(src_bus->dev, "failed to update PM QoS of %s\n", > dev_name(src_bus->dev.parent)) > > > + return ret; > > + } > > + > > + ret = dev_pm_qos_update_request(&dst_bus->qos_req, dst_freq); > > + if (ret < 0) { > > + dev_err(dst_bus->dev, "failed to update PM QoS request"); > > ditto. > > dev_err(src_bus->dev, "failed to update PM QoS of %s\n", > dev_name(dst_bus->dev.parent)) > > > > + return ret; > > + } > > + > > + return 0; > > +} > > + > > +static struct icc_node *exynos_bus_icc_xlate(struct of_phandle_args *spec, > > + void *data) > > +{ > > + struct exynos_bus *bus = data; > > + > > + if (spec->np != bus->dev->of_node) > > + return ERR_PTR(-EINVAL); > > + > > + return bus->node; > > +} > > + > > static int exynos_bus_parent_parse_of(struct device_node *np, > > struct exynos_bus *bus) > > { > > @@ -419,6 +463,96 @@ static int exynos_bus_profile_init_passive(struct exynos_bus *bus, > > return 0; > > } > > > > +static struct icc_node *exynos_bus_icc_get_parent(struct exynos_bus *bus) > > +{ > > + struct device_node *np = bus->dev->of_node; > > + struct of_phandle_args args; > > + int num, ret; > > + > > + num = of_count_phandle_with_args(np, "exynos,interconnect-parent-node", > > + "#interconnect-cells"); > > + if (num != 1) > > + return NULL; /* parent nodes are optional */ > > You better to add the comment before calling exynos_bus_icc_get_parent > and remove '/* parent nodes are optional */'. Actually, it is not enough > to understand the role of 'interconnect-parent-node' with '/* parent > nodes are optional */'. > And I add my opinion about this comment below. > > And I expect that you will add the description and example for > 'exynos,interconnect-parent-node' on exynos-bus dt-binding document. > > > + > > + ret = of_parse_phandle_with_args(np, "exynos,interconnect-parent-node", > > + "#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_bus_icc_init(struct exynos_bus *bus) > > +{ > > + static DEFINE_IDA(ida); > > + > > + struct device *dev = bus->dev; > > + struct icc_provider *provider = &bus->provider; > > + struct icc_node *node, *parent_node; > > + int id, ret; > > + > > + /* Initialize the interconnect provider */ > > + provider->set = exynos_bus_icc_set; > > + provider->aggregate = icc_std_aggregate; > > + provider->xlate = exynos_bus_icc_xlate; > > + provider->dev = dev; > > + provider->inter_set = true; > > + provider->data = bus; > > + > > + ret = icc_provider_add(provider); > > + if (ret < 0) > > + return ret; > > + > > + ret = id = ida_alloc(&ida, GFP_KERNEL); > > + if (ret < 0) > > + goto err_id; > > + > > + node = icc_node_create(id); > > + if (IS_ERR(node)) { > > + ret = PTR_ERR(node); > > + goto err_node; > > + } > > + > > + bus->node = node; > > + node->name = dev->of_node->name; > > + node->data = bus; > > + icc_node_add(node, provider); > > + > > Better to add the following comment. If you add following comment > before calling exynos_bus_icc_get_parent, don't need to add the > same comment into exynos_bus_icc. > /* If interconnect parent node is not existing, don't use > interconnect feature */ > > > + parent_node = exynos_bus_icc_get_parent(bus); > > + if (IS_ERR(parent_node)) { > > + ret = PTR_ERR(parent_node); > > + goto err_parent; > > + } > > + > > + if (parent_node) { > > Better to change this if statement as following: > else if (parent_node) > > > > + ret = icc_link_create(node, parent_node->id); > > + if (ret < 0) > > + goto err_parent; > > + } > > + > > + ret = dev_pm_qos_add_request(bus->devfreq->dev.parent, &bus->qos_req, > > + DEV_PM_QOS_MIN_FREQUENCY, 0); > > Is it necessary if interconnect-parent-node is not existing? I realized it is necessary for final destination node. > > > > + if (ret < 0) > > + goto err_request; > > + > > + return 0; > > + > > +err_request: > > + if (parent_node) > > + icc_link_destroy(node, parent_node); > > +err_parent: > > + icc_node_del(node); > > + icc_node_destroy(id); > > +err_node: > > + ida_free(&ida, id); > > +err_id: > > + icc_provider_del(provider); > > + > > + return ret; > > +} > > + > > static int exynos_bus_probe(struct platform_device *pdev) > > { > > struct device *dev = &pdev->dev; > > @@ -468,6 +602,16 @@ static int exynos_bus_probe(struct platform_device *pdev) > > if (ret < 0) > > goto err; > > > > + /* > > + * Initialize interconnect provider. A return value of -ENOTSUPP means > > + * that CONFIG_INTERCONNECT is disabled. > > + */ > > + ret = exynos_bus_icc_init(bus); > > + if (ret < 0 && ret != -ENOTSUPP) { > > + dev_err(dev, "failed to initialize the interconnect provider"); > > + 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.17.1 > > > > > -- > Best Regards, > Chanwoo Choi -- Best Regards, Chanwoo Choi ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC PATCH v3 5/7] devfreq: exynos-bus: Add interconnect functionality to exynos-bus 2019-12-20 11:56 ` [RFC PATCH v3 5/7] devfreq: exynos-bus: Add interconnect functionality to exynos-bus Artur Świgoń 2019-12-21 19:53 ` Chanwoo Choi @ 2020-01-22 17:02 ` Georgi Djakov 2020-01-24 11:22 ` Artur Świgoń 1 sibling, 1 reply; 31+ messages in thread From: Georgi Djakov @ 2020-01-22 17:02 UTC (permalink / raw) To: Artur Świgoń, devicetree, linux-arm-kernel, linux-samsung-soc, linux-kernel, linux-pm, dri-devel Cc: cw00.choi, myungjoo.ham, inki.dae, sw0312.kim, leonard.crestez, m.szyprowski, b.zolnierkie, krzk Hi Artur, On 12/20/19 13:56, Artur Świgoń wrote: > This patch adds interconnect functionality to the exynos-bus devfreq > driver. > > The SoC topology is a graph (or, more specifically, a tree) and its > edges are specified using the 'exynos,interconnect-parent-node' 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 at Just to note that usually the provider consists of multiple nodes and each node represents a single master or slave port on the AXI bus for example. I am not sure whether this represents correctly the Exynos hardware, so it's up to you. > runtime, in probing order (subject to the above-mentioned exception > regarding relative order). This approach allows for using this driver with > various Exynos SoCs. This sounds good. I am wondering whether such dynamic probing would be useful for other platforms too. Then maybe it would make sense to even have a common DT property, but we will see. Is this going to be used only together with devfreq? > 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. How about the case where CONFIG_INTERCONNECT=m. Looks like the build will fail if CONFIG_ARM_EXYNOS_BUS_DEVFREQ=y, so this dependency should be expressed in Kconfig. Thanks, Georgi > > Signed-off-by: Artur Świgoń <a.swigon@samsung.com> > --- > drivers/devfreq/exynos-bus.c | 144 +++++++++++++++++++++++++++++++++++ > 1 file changed, 144 insertions(+) > > diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c > index 9fdb188915e8..694a9581dcdb 100644 > --- a/drivers/devfreq/exynos-bus.c > +++ b/drivers/devfreq/exynos-bus.c > @@ -14,14 +14,19 @@ > #include <linux/devfreq-event.h> > #include <linux/device.h> > #include <linux/export.h> > +#include <linux/idr.h> > +#include <linux/interconnect-provider.h> > #include <linux/module.h> > #include <linux/of.h> > #include <linux/pm_opp.h> > +#include <linux/pm_qos.h> > #include <linux/platform_device.h> > #include <linux/regulator/consumer.h> > > #define DEFAULT_SATURATION_RATIO 40 > > +#define kbps_to_khz(x) ((x) / 8) > + > struct exynos_bus { > struct device *dev; > > @@ -35,6 +40,12 @@ struct exynos_bus { > struct opp_table *opp_table; > struct clk *clk; > unsigned int ratio; > + > + /* One provider per bus, one node per provider */ > + struct icc_provider provider; > + struct icc_node *node; > + > + struct dev_pm_qos_request qos_req; > }; > > /* > @@ -205,6 +216,39 @@ static void exynos_bus_passive_exit(struct device *dev) > clk_disable_unprepare(bus->clk); > } > > +static int exynos_bus_icc_set(struct icc_node *src, struct icc_node *dst) > +{ > + struct exynos_bus *src_bus = src->data, *dst_bus = dst->data; > + s32 src_freq = kbps_to_khz(src->avg_bw); > + s32 dst_freq = kbps_to_khz(dst->avg_bw); > + int ret; > + > + ret = dev_pm_qos_update_request(&src_bus->qos_req, src_freq); > + if (ret < 0) { > + dev_err(src_bus->dev, "failed to update PM QoS request"); > + return ret; > + } > + > + ret = dev_pm_qos_update_request(&dst_bus->qos_req, dst_freq); > + if (ret < 0) { > + dev_err(dst_bus->dev, "failed to update PM QoS request"); > + return ret; > + } > + > + return 0; > +} > + > +static struct icc_node *exynos_bus_icc_xlate(struct of_phandle_args *spec, > + void *data) > +{ > + struct exynos_bus *bus = data; > + > + if (spec->np != bus->dev->of_node) > + return ERR_PTR(-EINVAL); > + > + return bus->node; > +} > + > static int exynos_bus_parent_parse_of(struct device_node *np, > struct exynos_bus *bus) > { > @@ -419,6 +463,96 @@ static int exynos_bus_profile_init_passive(struct exynos_bus *bus, > return 0; > } > > +static struct icc_node *exynos_bus_icc_get_parent(struct exynos_bus *bus) > +{ > + struct device_node *np = bus->dev->of_node; > + struct of_phandle_args args; > + int num, ret; > + > + num = of_count_phandle_with_args(np, "exynos,interconnect-parent-node", > + "#interconnect-cells"); > + if (num != 1) > + return NULL; /* parent nodes are optional */ > + > + ret = of_parse_phandle_with_args(np, "exynos,interconnect-parent-node", > + "#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_bus_icc_init(struct exynos_bus *bus) > +{ > + static DEFINE_IDA(ida); > + > + struct device *dev = bus->dev; > + struct icc_provider *provider = &bus->provider; > + struct icc_node *node, *parent_node; > + int id, ret; > + > + /* Initialize the interconnect provider */ > + provider->set = exynos_bus_icc_set; > + provider->aggregate = icc_std_aggregate; > + provider->xlate = exynos_bus_icc_xlate; > + provider->dev = dev; > + provider->inter_set = true; > + provider->data = bus; > + > + ret = icc_provider_add(provider); > + if (ret < 0) > + return ret; > + > + ret = id = ida_alloc(&ida, GFP_KERNEL); > + if (ret < 0) > + goto err_id; > + > + node = icc_node_create(id); > + if (IS_ERR(node)) { > + ret = PTR_ERR(node); > + goto err_node; > + } > + > + bus->node = node; > + node->name = dev->of_node->name; > + node->data = bus; > + icc_node_add(node, provider); > + > + parent_node = exynos_bus_icc_get_parent(bus); > + if (IS_ERR(parent_node)) { > + ret = PTR_ERR(parent_node); > + goto err_parent; > + } > + > + if (parent_node) { > + ret = icc_link_create(node, parent_node->id); > + if (ret < 0) > + goto err_parent; > + } > + > + ret = dev_pm_qos_add_request(bus->devfreq->dev.parent, &bus->qos_req, > + DEV_PM_QOS_MIN_FREQUENCY, 0); > + if (ret < 0) > + goto err_request; > + > + return 0; > + > +err_request: > + if (parent_node) > + icc_link_destroy(node, parent_node); > +err_parent: > + icc_node_del(node); > + icc_node_destroy(id); > +err_node: > + ida_free(&ida, id); > +err_id: > + icc_provider_del(provider); > + > + return ret; > +} > + > static int exynos_bus_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > @@ -468,6 +602,16 @@ static int exynos_bus_probe(struct platform_device *pdev) > if (ret < 0) > goto err; > > + /* > + * Initialize interconnect provider. A return value of -ENOTSUPP means > + * that CONFIG_INTERCONNECT is disabled. > + */ > + ret = exynos_bus_icc_init(bus); > + if (ret < 0 && ret != -ENOTSUPP) { > + dev_err(dev, "failed to initialize the interconnect provider"); > + 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); > ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC PATCH v3 5/7] devfreq: exynos-bus: Add interconnect functionality to exynos-bus 2020-01-22 17:02 ` Georgi Djakov @ 2020-01-24 11:22 ` Artur Świgoń 2020-01-24 12:32 ` Georgi Djakov 0 siblings, 1 reply; 31+ messages in thread From: Artur Świgoń @ 2020-01-24 11:22 UTC (permalink / raw) To: Georgi Djakov, devicetree, linux-arm-kernel, linux-samsung-soc, linux-kernel, linux-pm, dri-devel Cc: cw00.choi, myungjoo.ham, inki.dae, sw0312.kim, leonard.crestez, m.szyprowski, b.zolnierkie, krzk Hi Georgi, On Wed, 2020-01-22 at 19:02 +0200, Georgi Djakov wrote: > Hi Artur, > > On 12/20/19 13:56, Artur Świgoń wrote: > > This patch adds interconnect functionality to the exynos-bus devfreq > > driver. > > > > The SoC topology is a graph (or, more specifically, a tree) and its > > edges are specified using the 'exynos,interconnect-parent-node' 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 at > > Just to note that usually the provider consists of multiple nodes and each node > represents a single master or slave port on the AXI bus for example. I am not > sure whether this represents correctly the Exynos hardware, so it's up to > you. > > > runtime, in probing order (subject to the above-mentioned exception > > regarding relative order). This approach allows for using this driver with > > various Exynos SoCs. > > This sounds good. I am wondering whether such dynamic probing would be useful > for other platforms too. Then maybe it would make sense to even have a common DT > property, but we will see. > > Is this going to be used only together with devfreq? Yes, this functions solely as an extension to devfreq, hence the slightly unusual architecture (one icc_provider/icc_node per devfreq). (Compared to a singleton icc_provider, this approach yields less code with a very simple xlate()). With exactly one icc_node for every devfreq device, I think I will actually reuse the devfreq ID (as seen in the device name, e.g. the "3" in "devfreq3") for the node ID. The devfreq framework already does the dynamic numbering thing that I do in this patch using IDR. > > 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. > > How about the case where CONFIG_INTERCONNECT=m. Looks like the build will fail > if CONFIG_ARM_EXYNOS_BUS_DEVFREQ=y, so this dependency should be expressed in > Kconfig. I think adding: depends on INTERCONNECT || !INTERCONNECT under ARM_EXYNOS_BUS_DEVFREQ does the trick. > > > > Signed-off-by: Artur Świgoń <a.swigon@samsung.com> > > --- > > drivers/devfreq/exynos-bus.c | 144 +++++++++++++++++++++++++++++++++++ > > 1 file changed, 144 insertions(+) > > > > diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c > > index 9fdb188915e8..694a9581dcdb 100644 > > --- a/drivers/devfreq/exynos-bus.c > > +++ b/drivers/devfreq/exynos-bus.c > > @@ -14,14 +14,19 @@ > > #include <linux/devfreq-event.h> > > #include <linux/device.h> > > #include <linux/export.h> > > +#include <linux/idr.h> > > +#include <linux/interconnect-provider.h> > > #include <linux/module.h> > > #include <linux/of.h> > > #include <linux/pm_opp.h> > > +#include <linux/pm_qos.h> > > #include <linux/platform_device.h> > > #include <linux/regulator/consumer.h> > > > > #define DEFAULT_SATURATION_RATIO 40 > > > > +#define kbps_to_khz(x) ((x) / 8) > > + > > struct exynos_bus { > > struct device *dev; > > > > @@ -35,6 +40,12 @@ struct exynos_bus { > > struct opp_table *opp_table; > > struct clk *clk; > > unsigned int ratio; > > + > > + /* One provider per bus, one node per provider */ > > + struct icc_provider provider; > > + struct icc_node *node; > > + > > + struct dev_pm_qos_request qos_req; > > }; > > > > /* > > @@ -205,6 +216,39 @@ static void exynos_bus_passive_exit(struct device *dev) > > clk_disable_unprepare(bus->clk); > > } > > > > +static int exynos_bus_icc_set(struct icc_node *src, struct icc_node *dst) > > +{ > > + struct exynos_bus *src_bus = src->data, *dst_bus = dst->data; > > + s32 src_freq = kbps_to_khz(src->avg_bw); > > + s32 dst_freq = kbps_to_khz(dst->avg_bw); > > + int ret; > > + > > + ret = dev_pm_qos_update_request(&src_bus->qos_req, src_freq); > > + if (ret < 0) { > > + dev_err(src_bus->dev, "failed to update PM QoS request"); > > + return ret; > > + } > > + > > + ret = dev_pm_qos_update_request(&dst_bus->qos_req, dst_freq); > > + if (ret < 0) { > > + dev_err(dst_bus->dev, "failed to update PM QoS request"); > > + return ret; > > + } > > + > > + return 0; > > +} > > + > > +static struct icc_node *exynos_bus_icc_xlate(struct of_phandle_args *spec, > > + void *data) > > +{ > > + struct exynos_bus *bus = data; > > + > > + if (spec->np != bus->dev->of_node) > > + return ERR_PTR(-EINVAL); > > + > > + return bus->node; > > +} > > + > > static int exynos_bus_parent_parse_of(struct device_node *np, > > struct exynos_bus *bus) > > { > > @@ -419,6 +463,96 @@ static int exynos_bus_profile_init_passive(struct exynos_bus *bus, > > return 0; > > } > > > > +static struct icc_node *exynos_bus_icc_get_parent(struct exynos_bus *bus) > > +{ > > + struct device_node *np = bus->dev->of_node; > > + struct of_phandle_args args; > > + int num, ret; > > + > > + num = of_count_phandle_with_args(np, "exynos,interconnect-parent-node", > > + "#interconnect-cells"); > > + if (num != 1) > > + return NULL; /* parent nodes are optional */ > > + > > + ret = of_parse_phandle_with_args(np, "exynos,interconnect-parent-node", > > + "#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_bus_icc_init(struct exynos_bus *bus) > > +{ > > + static DEFINE_IDA(ida); > > + > > + struct device *dev = bus->dev; > > + struct icc_provider *provider = &bus->provider; > > + struct icc_node *node, *parent_node; > > + int id, ret; > > + > > + /* Initialize the interconnect provider */ > > + provider->set = exynos_bus_icc_set; > > + provider->aggregate = icc_std_aggregate; > > + provider->xlate = exynos_bus_icc_xlate; > > + provider->dev = dev; > > + provider->inter_set = true; > > + provider->data = bus; > > + > > + ret = icc_provider_add(provider); > > + if (ret < 0) > > + return ret; > > + > > + ret = id = ida_alloc(&ida, GFP_KERNEL); > > + if (ret < 0) > > + goto err_id; > > + > > + node = icc_node_create(id); > > + if (IS_ERR(node)) { > > + ret = PTR_ERR(node); > > + goto err_node; > > + } > > + > > + bus->node = node; > > + node->name = dev->of_node->name; > > + node->data = bus; > > + icc_node_add(node, provider); > > + > > + parent_node = exynos_bus_icc_get_parent(bus); > > + if (IS_ERR(parent_node)) { > > + ret = PTR_ERR(parent_node); > > + goto err_parent; > > + } > > + > > + if (parent_node) { > > + ret = icc_link_create(node, parent_node->id); > > + if (ret < 0) > > + goto err_parent; > > + } > > + > > + ret = dev_pm_qos_add_request(bus->devfreq->dev.parent, &bus->qos_req, > > + DEV_PM_QOS_MIN_FREQUENCY, 0); > > + if (ret < 0) > > + goto err_request; > > + > > + return 0; > > + > > +err_request: > > + if (parent_node) > > + icc_link_destroy(node, parent_node); > > +err_parent: > > + icc_node_del(node); > > + icc_node_destroy(id); > > +err_node: > > + ida_free(&ida, id); > > +err_id: > > + icc_provider_del(provider); > > + > > + return ret; > > +} > > + > > static int exynos_bus_probe(struct platform_device *pdev) > > { > > struct device *dev = &pdev->dev; > > @@ -468,6 +602,16 @@ static int exynos_bus_probe(struct platform_device *pdev) > > if (ret < 0) > > goto err; > > > > + /* > > + * Initialize interconnect provider. A return value of -ENOTSUPP means > > + * that CONFIG_INTERCONNECT is disabled. > > + */ > > + ret = exynos_bus_icc_init(bus); > > + if (ret < 0 && ret != -ENOTSUPP) { > > + dev_err(dev, "failed to initialize the interconnect provider"); > > + 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); > > > > -- Artur Świgoń Samsung R&D Institute Poland Samsung Electronics ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC PATCH v3 5/7] devfreq: exynos-bus: Add interconnect functionality to exynos-bus 2020-01-24 11:22 ` Artur Świgoń @ 2020-01-24 12:32 ` Georgi Djakov 0 siblings, 0 replies; 31+ messages in thread From: Georgi Djakov @ 2020-01-24 12:32 UTC (permalink / raw) To: Artur Świgoń, devicetree, linux-arm-kernel, linux-samsung-soc, linux-kernel, linux-pm, dri-devel Cc: cw00.choi, myungjoo.ham, inki.dae, sw0312.kim, leonard.crestez, m.szyprowski, b.zolnierkie, krzk Hi Artur, On 1/24/20 13:22, Artur Świgoń wrote: > Hi Georgi, > > On Wed, 2020-01-22 at 19:02 +0200, Georgi Djakov wrote: >> Hi Artur, >> >> On 12/20/19 13:56, Artur Świgoń wrote: >>> This patch adds interconnect functionality to the exynos-bus devfreq >>> driver. >>> >>> The SoC topology is a graph (or, more specifically, a tree) and its >>> edges are specified using the 'exynos,interconnect-parent-node' 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 at >> >> Just to note that usually the provider consists of multiple nodes and each node >> represents a single master or slave port on the AXI bus for example. I am not >> sure whether this represents correctly the Exynos hardware, so it's up to >> you. >> >>> runtime, in probing order (subject to the above-mentioned exception >>> regarding relative order). This approach allows for using this driver with >>> various Exynos SoCs. >> >> This sounds good. I am wondering whether such dynamic probing would be useful >> for other platforms too. Then maybe it would make sense to even have a common DT >> property, but we will see. >> >> Is this going to be used only together with devfreq? > > Yes, this functions solely as an extension to devfreq, hence the slightly > unusual architecture (one icc_provider/icc_node per devfreq). Ok, thanks for clarifying. > (Compared to a singleton icc_provider, this approach yields less code with > a very simple xlate()). > > With exactly one icc_node for every devfreq device, I think I will actually > reuse the devfreq ID (as seen in the device name, e.g. the "3" in "devfreq3") > for the node ID. The devfreq framework already does the dynamic numbering > thing that I do in this patch using IDR. > Sounds good. >>> 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. >> >> How about the case where CONFIG_INTERCONNECT=m. Looks like the build will fail >> if CONFIG_ARM_EXYNOS_BUS_DEVFREQ=y, so this dependency should be expressed in >> Kconfig. > > I think adding: > depends on INTERCONNECT || !INTERCONNECT Yes, exactly. > under ARM_EXYNOS_BUS_DEVFREQ does the trick. > >>> >>> Signed-off-by: Artur Świgoń <a.swigon@samsung.com> >>> --- >>> drivers/devfreq/exynos-bus.c | 144 +++++++++++++++++++++++++++++++++++ >>> 1 file changed, 144 insertions(+) >>> >>> diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c >>> index 9fdb188915e8..694a9581dcdb 100644 >>> --- a/drivers/devfreq/exynos-bus.c >>> +++ b/drivers/devfreq/exynos-bus.c >>> @@ -14,14 +14,19 @@ >>> #include <linux/devfreq-event.h> >>> #include <linux/device.h> >>> #include <linux/export.h> >>> +#include <linux/idr.h> >>> +#include <linux/interconnect-provider.h> >>> #include <linux/module.h> >>> #include <linux/of.h> >>> #include <linux/pm_opp.h> >>> +#include <linux/pm_qos.h> >>> #include <linux/platform_device.h> >>> #include <linux/regulator/consumer.h> >>> >>> #define DEFAULT_SATURATION_RATIO 40 >>> >>> +#define kbps_to_khz(x) ((x) / 8) >>> + >>> struct exynos_bus { >>> struct device *dev; >>> >>> @@ -35,6 +40,12 @@ struct exynos_bus { >>> struct opp_table *opp_table; >>> struct clk *clk; >>> unsigned int ratio; >>> + >>> + /* One provider per bus, one node per provider */ >>> + struct icc_provider provider; >>> + struct icc_node *node; >>> + >>> + struct dev_pm_qos_request qos_req; >>> }; >>> >>> /* >>> @@ -205,6 +216,39 @@ static void exynos_bus_passive_exit(struct device *dev) >>> clk_disable_unprepare(bus->clk); >>> } >>> >>> +static int exynos_bus_icc_set(struct icc_node *src, struct icc_node *dst) >>> +{ >>> + struct exynos_bus *src_bus = src->data, *dst_bus = dst->data; >>> + s32 src_freq = kbps_to_khz(src->avg_bw); >>> + s32 dst_freq = kbps_to_khz(dst->avg_bw); >>> + int ret; >>> + >>> + ret = dev_pm_qos_update_request(&src_bus->qos_req, src_freq); >>> + if (ret < 0) { >>> + dev_err(src_bus->dev, "failed to update PM QoS request"); >>> + return ret; >>> + } >>> + >>> + ret = dev_pm_qos_update_request(&dst_bus->qos_req, dst_freq); >>> + if (ret < 0) { >>> + dev_err(dst_bus->dev, "failed to update PM QoS request"); >>> + return ret; >>> + } >>> + >>> + return 0; >>> +} >>> + >>> +static struct icc_node *exynos_bus_icc_xlate(struct of_phandle_args *spec, >>> + void *data) >>> +{ >>> + struct exynos_bus *bus = data; >>> + >>> + if (spec->np != bus->dev->of_node) >>> + return ERR_PTR(-EINVAL); >>> + >>> + return bus->node; >>> +} >>> + >>> static int exynos_bus_parent_parse_of(struct device_node *np, >>> struct exynos_bus *bus) >>> { >>> @@ -419,6 +463,96 @@ static int exynos_bus_profile_init_passive(struct exynos_bus *bus, >>> return 0; >>> } >>> >>> +static struct icc_node *exynos_bus_icc_get_parent(struct exynos_bus *bus) >>> +{ >>> + struct device_node *np = bus->dev->of_node; >>> + struct of_phandle_args args; >>> + int num, ret; >>> + >>> + num = of_count_phandle_with_args(np, "exynos,interconnect-parent-node", >>> + "#interconnect-cells"); >>> + if (num != 1) >>> + return NULL; /* parent nodes are optional */ >>> + >>> + ret = of_parse_phandle_with_args(np, "exynos,interconnect-parent-node", >>> + "#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_bus_icc_init(struct exynos_bus *bus) >>> +{ >>> + static DEFINE_IDA(ida); >>> + >>> + struct device *dev = bus->dev; >>> + struct icc_provider *provider = &bus->provider; >>> + struct icc_node *node, *parent_node; >>> + int id, ret; >>> + >>> + /* Initialize the interconnect provider */ >>> + provider->set = exynos_bus_icc_set; >>> + provider->aggregate = icc_std_aggregate; >>> + provider->xlate = exynos_bus_icc_xlate; >>> + provider->dev = dev; >>> + provider->inter_set = true; >>> + provider->data = bus; >>> + >>> + ret = icc_provider_add(provider); >>> + if (ret < 0) >>> + return ret; >>> + >>> + ret = id = ida_alloc(&ida, GFP_KERNEL); >>> + if (ret < 0) >>> + goto err_id; >>> + >>> + node = icc_node_create(id); >>> + if (IS_ERR(node)) { >>> + ret = PTR_ERR(node); >>> + goto err_node; >>> + } >>> + >>> + bus->node = node; >>> + node->name = dev->of_node->name; >>> + node->data = bus; >>> + icc_node_add(node, provider); >>> + >>> + parent_node = exynos_bus_icc_get_parent(bus); >>> + if (IS_ERR(parent_node)) { >>> + ret = PTR_ERR(parent_node); >>> + goto err_parent; >>> + } >>> + >>> + if (parent_node) { >>> + ret = icc_link_create(node, parent_node->id); >>> + if (ret < 0) >>> + goto err_parent; >>> + } >>> + >>> + ret = dev_pm_qos_add_request(bus->devfreq->dev.parent, &bus->qos_req, >>> + DEV_PM_QOS_MIN_FREQUENCY, 0); >>> + if (ret < 0) >>> + goto err_request; >>> + >>> + return 0; >>> + >>> +err_request: >>> + if (parent_node) >>> + icc_link_destroy(node, parent_node); >>> +err_parent: >>> + icc_node_del(node); >>> + icc_node_destroy(id); >>> +err_node: >>> + ida_free(&ida, id); >>> +err_id: >>> + icc_provider_del(provider); >>> + >>> + return ret; >>> +} >>> + >>> static int exynos_bus_probe(struct platform_device *pdev) >>> { >>> struct device *dev = &pdev->dev; >>> @@ -468,6 +602,16 @@ static int exynos_bus_probe(struct platform_device *pdev) >>> if (ret < 0) >>> goto err; >>> >>> + /* >>> + * Initialize interconnect provider. A return value of -ENOTSUPP means >>> + * that CONFIG_INTERCONNECT is disabled. >>> + */ >>> + ret = exynos_bus_icc_init(bus); >>> + if (ret < 0 && ret != -ENOTSUPP) { I have been also thinking that all the code that you add in this patch would fit nicely into a separate interconnect provider driver. Then instead of the above icc_init() you can create a sub-device (platform_device_register_data() maybe?). The separate driver will be handling just the interconnect functionality and could be enabled on top of this devfreq driver. Thanks, Georgi P.S. I think that grouping all the related patches into a single patch-set would make reviewing them easier. Then we can decide about the merge path of each. >>> + dev_err(dev, "failed to initialize the interconnect provider"); >>> + 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); >>> >> >> ^ permalink raw reply [flat|nested] 31+ messages in thread
[parent not found: <CGME20191220120146eucas1p25dada01c315215d18bb8a15e3173b52c@eucas1p2.samsung.com>]
* [RFC PATCH v3 6/7] arm: dts: exynos: Add interconnects to Exynos4412 mixer [not found] ` <CGME20191220120146eucas1p25dada01c315215d18bb8a15e3173b52c@eucas1p2.samsung.com> @ 2019-12-20 11:56 ` Artur Świgoń 2019-12-21 20:08 ` Chanwoo Choi 0 siblings, 1 reply; 31+ messages in thread From: Artur Świgoń @ 2019-12-20 11:56 UTC (permalink / raw) To: devicetree, linux-arm-kernel, linux-samsung-soc, linux-kernel, linux-pm, dri-devel Cc: Artur Świgoń, cw00.choi, myungjoo.ham, inki.dae, sw0312.kim, georgi.djakov, leonard.crestez, m.szyprowski, b.zolnierkie, krzk 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> --- 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 48868947373e..13a679a9a107 100644 --- a/arch/arm/boot/dts/exynos4412.dtsi +++ b/arch/arm/boot/dts/exynos4412.dtsi @@ -771,6 +771,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.17.1 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [RFC PATCH v3 6/7] arm: dts: exynos: Add interconnects to Exynos4412 mixer 2019-12-20 11:56 ` [RFC PATCH v3 6/7] arm: dts: exynos: Add interconnects to Exynos4412 mixer Artur Świgoń @ 2019-12-21 20:08 ` Chanwoo Choi 0 siblings, 0 replies; 31+ messages in thread From: Chanwoo Choi @ 2019-12-21 20:08 UTC (permalink / raw) To: Artur Świgoń Cc: devicetree, linux-arm-kernel, linux-samsung-soc, linux-kernel, Linux PM list, dri-devel, Chanwoo Choi, MyungJoo Ham, inki.dae, Seung-Woo Kim, Georgi Djakov, Leonard Crestez, Marek Szyprowski, Bartlomiej Zolnierkiewicz, Krzysztof Kozlowski Hi, On Fri, Dec 20, 2019 at 9:02 PM Artur Świgoń <a.swigon@samsung.com> wrote: > > 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> > --- > 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 48868947373e..13a679a9a107 100644 > --- a/arch/arm/boot/dts/exynos4412.dtsi > +++ b/arch/arm/boot/dts/exynos4412.dtsi > @@ -771,6 +771,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.17.1 > Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com> -- Best Regards, Chanwoo Choi ^ permalink raw reply [flat|nested] 31+ messages in thread
[parent not found: <CGME20191220120146eucas1p22a7b0457be4f378b113f67dc25f2eba7@eucas1p2.samsung.com>]
* [RFC PATCH v3 7/7] drm: exynos: mixer: Add interconnect support [not found] ` <CGME20191220120146eucas1p22a7b0457be4f378b113f67dc25f2eba7@eucas1p2.samsung.com> @ 2019-12-20 11:56 ` Artur Świgoń 2019-12-21 20:15 ` Chanwoo Choi 2019-12-24 4:56 ` Inki Dae 0 siblings, 2 replies; 31+ messages in thread From: Artur Świgoń @ 2019-12-20 11:56 UTC (permalink / raw) To: devicetree, linux-arm-kernel, linux-samsung-soc, linux-kernel, linux-pm, dri-devel Cc: Marek Szyprowski, cw00.choi, myungjoo.ham, inki.dae, sw0312.kim, georgi.djakov, leonard.crestez, b.zolnierkie, krzk, Artur Świgoń From: Marek Szyprowski <m.szyprowski@samsung.com> This patch adds interconnect support to exynos-mixer. The mixer works the same as before when CONFIG_INTERCONNECT is 'n'. Co-developed-by: Artur Świgoń <a.swigon@samsung.com> Signed-off-by: Artur Świgoń <a.swigon@samsung.com> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> --- drivers/gpu/drm/exynos/exynos_mixer.c | 71 +++++++++++++++++++++++++-- 1 file changed, 66 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c index 6cfdb95fef2f..a7e7240a055f 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> @@ -97,6 +98,7 @@ struct mixer_context { struct exynos_drm_crtc *crtc; struct exynos_drm_plane planes[MIXER_WIN_NR]; unsigned long flags; + struct icc_path *soc_path; int irq; void __iomem *mixer_regs; @@ -931,6 +933,40 @@ static void mixer_disable_vblank(struct exynos_drm_crtc *crtc) mixer_reg_writemask(mixer_ctx, MXR_INT_EN, 0, MXR_INT_EN_VSYNC); } +static void mixer_set_memory_bandwidth(struct exynos_drm_crtc *crtc) +{ + struct drm_display_mode *mode = &crtc->base.state->adjusted_mode; + struct mixer_context *ctx = crtc->ctx; + unsigned long bw, bandwidth = 0; + int i, j, sub; + + if (!ctx->soc_path) + return; + + for (i = 0; i < MIXER_WIN_NR; i++) { + struct drm_plane *plane = &ctx->planes[i].base; + const struct drm_format_info *format; + + if (plane->state && plane->state->crtc && plane->state->fb) { + format = plane->state->fb->format; + bw = mode->hdisplay * mode->vdisplay * + drm_mode_vrefresh(mode); + if (mode->flags & DRM_MODE_FLAG_INTERLACE) + bw /= 2; + for (j = 0; j < format->num_planes; j++) { + sub = j ? (format->vsub * format->hsub) : 1; + bandwidth += format->cpp[j] * bw / sub; + } + } + } + + /* add 20% safety margin */ + bandwidth = bandwidth / 4 * 5; + + dev_dbg(ctx->dev, "exynos-mixer: safe bandwidth %ld Bps\n", bandwidth); + icc_set_bw(ctx->soc_path, Bps_to_icc(bandwidth), 0); +} + static void mixer_atomic_begin(struct exynos_drm_crtc *crtc) { struct mixer_context *ctx = crtc->ctx; @@ -982,6 +1018,7 @@ static void mixer_atomic_flush(struct exynos_drm_crtc *crtc) if (!test_bit(MXR_BIT_POWERED, &mixer_ctx->flags)) return; + mixer_set_memory_bandwidth(crtc); mixer_enable_sync(mixer_ctx); exynos_crtc_handle_event(crtc); } @@ -1029,6 +1066,7 @@ static void mixer_disable(struct exynos_drm_crtc *crtc) for (i = 0; i < MIXER_WIN_NR; i++) mixer_disable_plane(crtc, &ctx->planes[i]); + mixer_set_memory_bandwidth(crtc); exynos_drm_pipe_clk_enable(crtc, false); pm_runtime_put(ctx->dev); @@ -1220,12 +1258,22 @@ static int mixer_probe(struct platform_device *pdev) struct device *dev = &pdev->dev; const struct mixer_drv_data *drv; struct mixer_context *ctx; + struct icc_path *path; int ret; + /* + * Returns NULL if CONFIG_INTERCONNECT is disabled. + * May return ERR_PTR(-EPROBE_DEFER). + */ + path = of_icc_get(dev, NULL); + if (IS_ERR(path)) + return PTR_ERR(path); + ctx = devm_kzalloc(&pdev->dev, sizeof(*ctx), GFP_KERNEL); if (!ctx) { DRM_DEV_ERROR(dev, "failed to alloc mixer context.\n"); - return -ENOMEM; + ret = -ENOMEM; + goto err; } drv = of_device_get_match_data(dev); @@ -1233,6 +1281,7 @@ static int mixer_probe(struct platform_device *pdev) ctx->pdev = pdev; ctx->dev = dev; ctx->mxr_ver = drv->version; + ctx->soc_path = path; if (drv->is_vp_enabled) __set_bit(MXR_BIT_VP_ENABLED, &ctx->flags); @@ -1242,17 +1291,29 @@ static int mixer_probe(struct platform_device *pdev) platform_set_drvdata(pdev, ctx); ret = component_add(&pdev->dev, &mixer_component_ops); - if (!ret) - pm_runtime_enable(dev); + if (ret < 0) + goto err; + + pm_runtime_enable(dev); + + return 0; + +err: + icc_put(path); return ret; } static int mixer_remove(struct platform_device *pdev) { - pm_runtime_disable(&pdev->dev); + struct device *dev = &pdev->dev; + struct mixer_context *ctx = dev_get_drvdata(dev); + + pm_runtime_disable(dev); + + component_del(dev, &mixer_component_ops); - component_del(&pdev->dev, &mixer_component_ops); + icc_put(ctx->soc_path); return 0; } -- 2.17.1 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [RFC PATCH v3 7/7] drm: exynos: mixer: Add interconnect support 2019-12-20 11:56 ` [RFC PATCH v3 7/7] drm: exynos: mixer: Add interconnect support Artur Świgoń @ 2019-12-21 20:15 ` Chanwoo Choi 2019-12-24 4:56 ` Inki Dae 1 sibling, 0 replies; 31+ messages in thread From: Chanwoo Choi @ 2019-12-21 20:15 UTC (permalink / raw) To: Artur Świgoń Cc: devicetree, linux-arm-kernel, linux-samsung-soc, linux-kernel, Linux PM list, dri-devel, Marek Szyprowski, Chanwoo Choi, MyungJoo Ham, inki.dae, Seung-Woo Kim, Georgi Djakov, Leonard Crestez, Bartlomiej Zolnierkiewicz, Krzysztof Kozlowski Hi, On Fri, Dec 20, 2019 at 9:03 PM Artur Świgoń <a.swigon@samsung.com> wrote: > > From: Marek Szyprowski <m.szyprowski@samsung.com> > > This patch adds interconnect support to exynos-mixer. The mixer works > the same as before when CONFIG_INTERCONNECT is 'n'. The patch description doesn't include why interconnect is required and what to do. > > Co-developed-by: Artur Świgoń <a.swigon@samsung.com> > Signed-off-by: Artur Świgoń <a.swigon@samsung.com> > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > --- > drivers/gpu/drm/exynos/exynos_mixer.c | 71 +++++++++++++++++++++++++-- > 1 file changed, 66 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c > index 6cfdb95fef2f..a7e7240a055f 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> > @@ -97,6 +98,7 @@ struct mixer_context { > struct exynos_drm_crtc *crtc; > struct exynos_drm_plane planes[MIXER_WIN_NR]; > unsigned long flags; > + struct icc_path *soc_path; > > int irq; > void __iomem *mixer_regs; > @@ -931,6 +933,40 @@ static void mixer_disable_vblank(struct exynos_drm_crtc *crtc) > mixer_reg_writemask(mixer_ctx, MXR_INT_EN, 0, MXR_INT_EN_VSYNC); > } > > +static void mixer_set_memory_bandwidth(struct exynos_drm_crtc *crtc) > +{ > + struct drm_display_mode *mode = &crtc->base.state->adjusted_mode; > + struct mixer_context *ctx = crtc->ctx; > + unsigned long bw, bandwidth = 0; > + int i, j, sub; > + > + if (!ctx->soc_path) > + return; > + > + for (i = 0; i < MIXER_WIN_NR; i++) { > + struct drm_plane *plane = &ctx->planes[i].base; > + const struct drm_format_info *format; > + > + if (plane->state && plane->state->crtc && plane->state->fb) { > + format = plane->state->fb->format; > + bw = mode->hdisplay * mode->vdisplay * > + drm_mode_vrefresh(mode); > + if (mode->flags & DRM_MODE_FLAG_INTERLACE) > + bw /= 2; > + for (j = 0; j < format->num_planes; j++) { > + sub = j ? (format->vsub * format->hsub) : 1; > + bandwidth += format->cpp[j] * bw / sub; > + } > + } > + } > + > + /* add 20% safety margin */ > + bandwidth = bandwidth / 4 * 5; > + > + dev_dbg(ctx->dev, "exynos-mixer: safe bandwidth %ld Bps\n", bandwidth); > + icc_set_bw(ctx->soc_path, Bps_to_icc(bandwidth), 0); > +} I recommend that add the role of this function in order to guarantee the minimum bandwidth to prevent performance drop or h/w issue. > + > static void mixer_atomic_begin(struct exynos_drm_crtc *crtc) > { > struct mixer_context *ctx = crtc->ctx; > @@ -982,6 +1018,7 @@ static void mixer_atomic_flush(struct exynos_drm_crtc *crtc) > if (!test_bit(MXR_BIT_POWERED, &mixer_ctx->flags)) > return; > > + mixer_set_memory_bandwidth(crtc); > mixer_enable_sync(mixer_ctx); > exynos_crtc_handle_event(crtc); > } > @@ -1029,6 +1066,7 @@ static void mixer_disable(struct exynos_drm_crtc *crtc) > for (i = 0; i < MIXER_WIN_NR; i++) > mixer_disable_plane(crtc, &ctx->planes[i]); > > + mixer_set_memory_bandwidth(crtc); > exynos_drm_pipe_clk_enable(crtc, false); > > pm_runtime_put(ctx->dev); > @@ -1220,12 +1258,22 @@ static int mixer_probe(struct platform_device *pdev) > struct device *dev = &pdev->dev; > const struct mixer_drv_data *drv; > struct mixer_context *ctx; > + struct icc_path *path; > int ret; > > + /* > + * Returns NULL if CONFIG_INTERCONNECT is disabled. > + * May return ERR_PTR(-EPROBE_DEFER). > + */ > + path = of_icc_get(dev, NULL); > + if (IS_ERR(path)) > + return PTR_ERR(path); > + > ctx = devm_kzalloc(&pdev->dev, sizeof(*ctx), GFP_KERNEL); > if (!ctx) { > DRM_DEV_ERROR(dev, "failed to alloc mixer context.\n"); > - return -ENOMEM; > + ret = -ENOMEM; > + goto err; > } > > drv = of_device_get_match_data(dev); > @@ -1233,6 +1281,7 @@ static int mixer_probe(struct platform_device *pdev) > ctx->pdev = pdev; > ctx->dev = dev; > ctx->mxr_ver = drv->version; > + ctx->soc_path = path; > > if (drv->is_vp_enabled) > __set_bit(MXR_BIT_VP_ENABLED, &ctx->flags); > @@ -1242,17 +1291,29 @@ static int mixer_probe(struct platform_device *pdev) > platform_set_drvdata(pdev, ctx); > > ret = component_add(&pdev->dev, &mixer_component_ops); > - if (!ret) > - pm_runtime_enable(dev); > + if (ret < 0) > + goto err; > + > + pm_runtime_enable(dev); > + > + return 0; > + > +err: > + icc_put(path); > > return ret; > } > > static int mixer_remove(struct platform_device *pdev) > { > - pm_runtime_disable(&pdev->dev); > + struct device *dev = &pdev->dev; > + struct mixer_context *ctx = dev_get_drvdata(dev); > + > + pm_runtime_disable(dev); > + > + component_del(dev, &mixer_component_ops); > > - component_del(&pdev->dev, &mixer_component_ops); > + icc_put(ctx->soc_path); > > return 0; > } > -- > 2.17.1 > Basically, I agree this patch about using ICC feature to guarantee the minimum bandwidth. But, I don't have the knowledge of exynos-mixer.c. So, just I reviewed the part of ICC usage. After digging the exynos-mixer.c, I'll reply the reviewed tag. Thanks. -- Best Regards, Chanwoo Choi ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC PATCH v3 7/7] drm: exynos: mixer: Add interconnect support 2019-12-20 11:56 ` [RFC PATCH v3 7/7] drm: exynos: mixer: Add interconnect support Artur Świgoń 2019-12-21 20:15 ` Chanwoo Choi @ 2019-12-24 4:56 ` Inki Dae 2019-12-30 9:35 ` Artur Świgoń 1 sibling, 1 reply; 31+ messages in thread From: Inki Dae @ 2019-12-24 4:56 UTC (permalink / raw) To: Artur Świgoń, devicetree, linux-arm-kernel, linux-samsung-soc, linux-kernel, linux-pm, dri-devel Cc: Marek Szyprowski, cw00.choi, myungjoo.ham, sw0312.kim, georgi.djakov, leonard.crestez, b.zolnierkie, krzk Hi, 19. 12. 20. 오후 8:56에 Artur Świgoń 이(가) 쓴 글: > From: Marek Szyprowski <m.szyprowski@samsung.com> > > This patch adds interconnect support to exynos-mixer. The mixer works > the same as before when CONFIG_INTERCONNECT is 'n'. > > Co-developed-by: Artur Świgoń <a.swigon@samsung.com> > Signed-off-by: Artur Świgoń <a.swigon@samsung.com> > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > --- > drivers/gpu/drm/exynos/exynos_mixer.c | 71 +++++++++++++++++++++++++-- > 1 file changed, 66 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c > index 6cfdb95fef2f..a7e7240a055f 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> > @@ -97,6 +98,7 @@ struct mixer_context { > struct exynos_drm_crtc *crtc; > struct exynos_drm_plane planes[MIXER_WIN_NR]; > unsigned long flags; > + struct icc_path *soc_path; > > int irq; > void __iomem *mixer_regs; > @@ -931,6 +933,40 @@ static void mixer_disable_vblank(struct exynos_drm_crtc *crtc) > mixer_reg_writemask(mixer_ctx, MXR_INT_EN, 0, MXR_INT_EN_VSYNC); > } > > +static void mixer_set_memory_bandwidth(struct exynos_drm_crtc *crtc) > +{ > + struct drm_display_mode *mode = &crtc->base.state->adjusted_mode; > + struct mixer_context *ctx = crtc->ctx; > + unsigned long bw, bandwidth = 0; > + int i, j, sub; > + > + if (!ctx->soc_path) > + return; > + > + for (i = 0; i < MIXER_WIN_NR; i++) { > + struct drm_plane *plane = &ctx->planes[i].base; > + const struct drm_format_info *format; > + > + if (plane->state && plane->state->crtc && plane->state->fb) { > + format = plane->state->fb->format; > + bw = mode->hdisplay * mode->vdisplay * > + drm_mode_vrefresh(mode); > + if (mode->flags & DRM_MODE_FLAG_INTERLACE) > + bw /= 2; > + for (j = 0; j < format->num_planes; j++) { > + sub = j ? (format->vsub * format->hsub) : 1; > + bandwidth += format->cpp[j] * bw / sub; > + } > + } > + } > + > + /* add 20% safety margin */ > + bandwidth = bandwidth / 4 * 5; > + > + dev_dbg(ctx->dev, "exynos-mixer: safe bandwidth %ld Bps\n", bandwidth); > + icc_set_bw(ctx->soc_path, Bps_to_icc(bandwidth), 0); > +} > + > static void mixer_atomic_begin(struct exynos_drm_crtc *crtc) > { > struct mixer_context *ctx = crtc->ctx; > @@ -982,6 +1018,7 @@ static void mixer_atomic_flush(struct exynos_drm_crtc *crtc) > if (!test_bit(MXR_BIT_POWERED, &mixer_ctx->flags)) > return; > > + mixer_set_memory_bandwidth(crtc); > mixer_enable_sync(mixer_ctx); > exynos_crtc_handle_event(crtc); > } > @@ -1029,6 +1066,7 @@ static void mixer_disable(struct exynos_drm_crtc *crtc) > for (i = 0; i < MIXER_WIN_NR; i++) > mixer_disable_plane(crtc, &ctx->planes[i]); > > + mixer_set_memory_bandwidth(crtc); Your intention is to set peak and average bandwidth to 0 at disabling mixer device? Thanks, Inki Dae ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC PATCH v3 7/7] drm: exynos: mixer: Add interconnect support 2019-12-24 4:56 ` Inki Dae @ 2019-12-30 9:35 ` Artur Świgoń 0 siblings, 0 replies; 31+ messages in thread From: Artur Świgoń @ 2019-12-30 9:35 UTC (permalink / raw) To: Inki Dae, devicetree, linux-arm-kernel, linux-samsung-soc, linux-kernel, linux-pm, dri-devel Cc: Marek Szyprowski, cw00.choi, myungjoo.ham, sw0312.kim, georgi.djakov, leonard.crestez, b.zolnierkie, krzk Hi, On Tue, 2019-12-24 at 13:56 +0900, Inki Dae wrote: > Hi, > > 19. 12. 20. 오후 8:56에 Artur Świgoń 이(가) 쓴 글: > > From: Marek Szyprowski <m.szyprowski@samsung.com> > > > > This patch adds interconnect support to exynos-mixer. The mixer works > > the same as before when CONFIG_INTERCONNECT is 'n'. > > > > Co-developed-by: Artur Świgoń <a.swigon@samsung.com> > > Signed-off-by: Artur Świgoń <a.swigon@samsung.com> > > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > > --- > > drivers/gpu/drm/exynos/exynos_mixer.c | 71 +++++++++++++++++++++++++-- > > 1 file changed, 66 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c > > index 6cfdb95fef2f..a7e7240a055f 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> > > @@ -97,6 +98,7 @@ struct mixer_context { > > struct exynos_drm_crtc *crtc; > > struct exynos_drm_plane planes[MIXER_WIN_NR]; > > unsigned long flags; > > + struct icc_path *soc_path; > > > > int irq; > > void __iomem *mixer_regs; > > @@ -931,6 +933,40 @@ static void mixer_disable_vblank(struct exynos_drm_crtc *crtc) > > mixer_reg_writemask(mixer_ctx, MXR_INT_EN, 0, MXR_INT_EN_VSYNC); > > } > > > > +static void mixer_set_memory_bandwidth(struct exynos_drm_crtc *crtc) > > +{ > > + struct drm_display_mode *mode = &crtc->base.state->adjusted_mode; > > + struct mixer_context *ctx = crtc->ctx; > > + unsigned long bw, bandwidth = 0; > > + int i, j, sub; > > + > > + if (!ctx->soc_path) > > + return; > > + > > + for (i = 0; i < MIXER_WIN_NR; i++) { > > + struct drm_plane *plane = &ctx->planes[i].base; > > + const struct drm_format_info *format; > > + > > + if (plane->state && plane->state->crtc && plane->state->fb) { > > + format = plane->state->fb->format; > > + bw = mode->hdisplay * mode->vdisplay * > > + drm_mode_vrefresh(mode); > > + if (mode->flags & DRM_MODE_FLAG_INTERLACE) > > + bw /= 2; > > + for (j = 0; j < format->num_planes; j++) { > > + sub = j ? (format->vsub * format->hsub) : 1; > > + bandwidth += format->cpp[j] * bw / sub; > > + } > > + } > > + } > > + > > + /* add 20% safety margin */ > > + bandwidth = bandwidth / 4 * 5; > > + > > + dev_dbg(ctx->dev, "exynos-mixer: safe bandwidth %ld Bps\n", bandwidth); > > + icc_set_bw(ctx->soc_path, Bps_to_icc(bandwidth), 0); > > +} > > + > > static void mixer_atomic_begin(struct exynos_drm_crtc *crtc) > > { > > struct mixer_context *ctx = crtc->ctx; > > @@ -982,6 +1018,7 @@ static void mixer_atomic_flush(struct exynos_drm_crtc *crtc) > > if (!test_bit(MXR_BIT_POWERED, &mixer_ctx->flags)) > > return; > > > > + mixer_set_memory_bandwidth(crtc); > > mixer_enable_sync(mixer_ctx); > > exynos_crtc_handle_event(crtc); > > } > > @@ -1029,6 +1066,7 @@ static void mixer_disable(struct exynos_drm_crtc *crtc) > > for (i = 0; i < MIXER_WIN_NR; i++) > > mixer_disable_plane(crtc, &ctx->planes[i]); > > > + mixer_set_memory_bandwidth(crtc); > > Your intention is to set peak and average bandwidth to 0 at disabling mixer device? Yes. In general, setting the requested bandwidth to zero means "do not override the devfreq setting" because only constraints of type DEV_PM_QOS_MIN_FREQUENCY are used (cf. patch 05 of this series). I will make sure to reflect that in the commit message. Moreover, this RFC does not really make use of the peak bandwidth (yet). It is set to zero in this patch and ignored in patch 05 (cf. exynos_bus_icc_set()). Only the average bandwidth is translated to a minimum frequency constraint, overriding devfreq if necessary. A possible modification to mixer_set_memory_bandwidth() could be: - bandwidth = bandwidth / 4 * 5; + peak_bandwidth = bandwidth / 4 * 5; in mixer_set_memory_bandwidth() plus some additional logic in exynos_bus_icc_set(). Best regards, -- Artur Świgoń Samsung R&D Institute Poland Samsung Electronics ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC PATCH v3 0/7] PM / devfreq: Simple QoS for exynos-bus using interconnect 2019-12-20 11:56 ` [RFC PATCH v3 0/7] PM / devfreq: Simple QoS for exynos-bus using interconnect Artur Świgoń ` (6 preceding siblings ...) [not found] ` <CGME20191220120146eucas1p22a7b0457be4f378b113f67dc25f2eba7@eucas1p2.samsung.com> @ 2019-12-21 19:58 ` Chanwoo Choi 7 siblings, 0 replies; 31+ messages in thread From: Chanwoo Choi @ 2019-12-21 19:58 UTC (permalink / raw) To: Artur Świgoń Cc: devicetree, linux-arm-kernel, linux-samsung-soc, linux-kernel, Linux PM list, dri-devel, Chanwoo Choi, MyungJoo Ham, inki.dae, Seung-Woo Kim, Georgi Djakov, Leonard Crestez, Marek Szyprowski, Bartlomiej Zolnierkiewicz, Krzysztof Kozlowski Hi Artur, I agree this approach. On next version, please update exynos-bus dt-binding document with example. On Fri, Dec 20, 2019 at 9:01 PM Artur Świgoń <a.swigon@samsung.com> wrote: > > The following patchset adds interconnect[1][2] framework support to the > exynos-bus devfreq driver. Extending the devfreq driver with > interconnect functionality started as a response to the issue referenced > in [3]. The patches can be subdivided into three groups: > > (a) Tweaking the interconnect framework to support the exynos-bus use > case (patches 01--03/07). Exporting of_icc_get_from_provider() allows to > avoid hardcoding every single graph edge in the DT or driver source, and > relaxing the requirement on #interconnect-cells removes the need to > provide dummy node IDs in the DT. A new field in struct icc_provider is > used to explicitly allow configuring node pairs from two different > providers. > > (b) Implementing interconnect providers in the exynos-bus devfreq driver > and adding required DT properties for one selected platform, namely > Exynos4412 (patches 04--05/07). Due to the fact that this aims to be a > generic driver for various Exynos SoCs, node IDs are generated > dynamically (rather than hardcoded). > > (c) Implementing a sample interconnect consumer for exynos-mixer > targeted at solving the issue referenced in [3], again with DT > properties only for Exynos4412 (patches 06--07/07). > > Integration of devfreq and interconnect frameworks is achieved by using > the dev_pm_qos_*() API. When CONFIG_INTERCONNECT is 'n' (such as in > exynos_defconfig) all interconnect API functions are no-ops. > > This series depends on these three patches (merged into devfreq-next[6]): > * https://patchwork.kernel.org/patch/11279087/ > * https://patchwork.kernel.org/patch/11279093/ > * https://patchwork.kernel.org/patch/11293765/ > and on this series: > * https://patchwork.kernel.org/cover/11304545/ > (which does not apply cleanly on next-20191220, adding > --exclude=arch/arm/boot/dts/exynos5422-odroid-core.dtsi to 'git am' is a > quick workaround) > > --- > 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. > > --- > Artur Świgoń > Samsung R&D Institute Poland > Samsung Electronics > > --- > References: > [1] Documentation/interconnect/interconnect.rst > [2] Documentation/devicetree/bindings/interconnect/interconnect.txt > [3] https://patchwork.kernel.org/patch/10861757/ (original issue) > [4] https://patchwork.kernel.org/cover/11054417/ (v1 of this RFC) > [5] https://patchwork.kernel.org/cover/11152595/ (v2 of this RFC) > [6] https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/linux.git/log/?h=devfreq-next > > Artur Świgoń (6): > interconnect: Export of_icc_get_from_provider() > interconnect: Relax requirement in of_icc_get_from_provider() > interconnect: Allow inter-provider pairs to be configured > arm: dts: exynos: Add interconnect bindings for Exynos4412 > devfreq: exynos-bus: Add interconnect functionality to exynos-bus > arm: dts: exynos: Add interconnects to Exynos4412 mixer > > Marek Szyprowski (1): > drm: exynos: mixer: Add interconnect support > > .../boot/dts/exynos4412-odroid-common.dtsi | 5 + > arch/arm/boot/dts/exynos4412.dtsi | 1 + > drivers/devfreq/exynos-bus.c | 144 ++++++++++++++++++ > drivers/gpu/drm/exynos/exynos_mixer.c | 71 ++++++++- > drivers/interconnect/core.c | 16 +- > include/linux/interconnect-provider.h | 8 + > 6 files changed, 232 insertions(+), 13 deletions(-) > > -- > 2.17.1 > -- Best Regards, Chanwoo Choi ^ permalink raw reply [flat|nested] 31+ messages in thread
end of thread, other threads:[~2020-01-24 12:32 UTC | newest] Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <CGME20191220120140eucas1p14ad33c20882f8f48e02337ea16754d91@eucas1p1.samsung.com> 2019-12-20 11:56 ` [RFC PATCH v3 0/7] PM / devfreq: Simple QoS for exynos-bus using interconnect Artur Świgoń [not found] ` <CGME20191220120141eucas1p11f5fa9d76d17e80e06199cb7a911c482@eucas1p1.samsung.com> 2019-12-20 11:56 ` [RFC PATCH v3 1/7] interconnect: Export of_icc_get_from_provider() Artur Świgoń [not found] ` <CGME20191220120142eucas1p1f43c7a862d9c0faa72e14b21d7d697e9@eucas1p1.samsung.com> 2019-12-20 11:56 ` [RFC PATCH v3 2/7] interconnect: Relax requirement in of_icc_get_from_provider() Artur Świgoń 2019-12-21 17:20 ` Chanwoo Choi [not found] ` <CGME20191220120143eucas1p1c9b01ae8c2e4ecd70423ef9d8001536f@eucas1p1.samsung.com> 2019-12-20 11:56 ` [RFC PATCH v3 3/7] interconnect: Allow inter-provider pairs to be configured Artur Świgoń 2019-12-22 17:08 ` Chanwoo Choi [not found] ` <CGME20191220120144eucas1p119ececf161a6d45a6a194e432bbbd1f9@eucas1p1.samsung.com> 2019-12-20 11:56 ` [RFC PATCH v3 4/7] arm: dts: exynos: Add interconnect bindings for Exynos4412 Artur Świgoń 2019-12-21 20:00 ` Chanwoo Choi 2019-12-30 15:44 ` Krzysztof Kozlowski 2019-12-31 7:18 ` Artur Świgoń 2019-12-31 9:22 ` Krzysztof Kozlowski 2019-12-31 9:41 ` Artur Świgoń 2019-12-31 10:02 ` Krzysztof Kozlowski 2019-12-31 10:23 ` Artur Świgoń 2019-12-31 10:38 ` Krzysztof Kozlowski 2019-12-31 11:03 ` Artur Świgoń 2020-01-22 16:54 ` Georgi Djakov 2020-01-24 11:22 ` Artur Świgoń [not found] ` <CGME20191220120145eucas1p295af63eed7b23982d8c49fcf875cec8c@eucas1p2.samsung.com> 2019-12-20 11:56 ` [RFC PATCH v3 5/7] devfreq: exynos-bus: Add interconnect functionality to exynos-bus Artur Świgoń 2019-12-21 19:53 ` Chanwoo Choi 2019-12-21 19:55 ` Chanwoo Choi 2020-01-22 17:02 ` Georgi Djakov 2020-01-24 11:22 ` Artur Świgoń 2020-01-24 12:32 ` Georgi Djakov [not found] ` <CGME20191220120146eucas1p25dada01c315215d18bb8a15e3173b52c@eucas1p2.samsung.com> 2019-12-20 11:56 ` [RFC PATCH v3 6/7] arm: dts: exynos: Add interconnects to Exynos4412 mixer Artur Świgoń 2019-12-21 20:08 ` Chanwoo Choi [not found] ` <CGME20191220120146eucas1p22a7b0457be4f378b113f67dc25f2eba7@eucas1p2.samsung.com> 2019-12-20 11:56 ` [RFC PATCH v3 7/7] drm: exynos: mixer: Add interconnect support Artur Świgoń 2019-12-21 20:15 ` Chanwoo Choi 2019-12-24 4:56 ` Inki Dae 2019-12-30 9:35 ` Artur Świgoń 2019-12-21 19:58 ` [RFC PATCH v3 0/7] PM / devfreq: Simple QoS for exynos-bus using interconnect 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).