The following patchset adds interconnect[1][2] framework support to the
exynos-bus devfreq driver. Extending the devfreq driver with interconnect
capabilities started as a response to the issue referenced in [3]. The
patches can be subdivided into four logical groups:
(a) Refactoring the existing devfreq driver in order to improve readability
and accommodate for adding new code (patches 01--04/11).
(b) Tweaking the interconnect framework to support the exynos-bus use case
(patches 05--07/11). Exporting of_icc_get_from_provider() allows us to
avoid hardcoding every single graph edge in the DT or driver source, and
relaxing the requirement contained in that function removes the need to
provide dummy node IDs in the DT. Adjusting the logic in
apply_constraints() (drivers/interconnect/core.c) accounts for the fact
that every bus is a separate entity and therefore a separate interconnect
provider, albeit constituting a part of a larger hierarchy.
(c) Implementing interconnect providers in the exynos-bus devfreq driver
and adding required DT properties for one selected platform, namely
Exynos4412 (patches 08--09/11). Due to the fact that this aims to be a
generic driver for various Exynos SoCs, node IDs are generated dynamically
rather than hardcoded. This has been determined to be a simpler approach,
but depends on changes described in (b).
(d) Implementing a sample interconnect consumer for exynos-mixer targeted
at the issue referenced in [3], again with DT info only for Exynos4412
(patches 10--11/11).
Integration of devfreq and interconnect functionalities is achieved by
using dev_pm_qos_*() API[5]. All new code works equally well when
CONFIG_INTERCONNECT is 'n' (as in exynos_defconfig) in which case all
interconnect API functions are no-ops.
This patchset depends on [5].
--- Changes since v1 [6]:
* Rebase on [4] (coupled regulators).
* Rebase on [5] (dev_pm_qos for devfreq).
* Use dev_pm_qos_*() API[5] instead of overriding frequency in
exynos_bus_target().
* Use IDR for node ID allocation.
* Avoid goto in functions extracted in patches 01 & 02 (cf. patch 04).
* Reverse order of multiplication and division in
mixer_set_memory_bandwidth() (patch 11) 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/11083663/ (coupled regulators; merged)
[5] https://patchwork.kernel.org/cover/11149497/ (dev_pm_qos for devfreq)
[6] https://patchwork.kernel.org/cover/11054417/ (v1 of this RFC)
Artur Świgoń (10):
devfreq: exynos-bus: Extract exynos_bus_profile_init()
devfreq: exynos-bus: Extract exynos_bus_profile_init_passive()
devfreq: exynos-bus: Change goto-based logic to if-else logic
devfreq: exynos-bus: Clean up code
interconnect: Export of_icc_get_from_provider()
interconnect: Relax requirement in of_icc_get_from_provider()
interconnect: Relax condition in apply_constraints()
arm: dts: exynos: Add parents and #interconnect-cells to 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 | 1 +
arch/arm/boot/dts/exynos4412.dtsi | 10 +
drivers/devfreq/exynos-bus.c | 319 +++++++++++++-----
drivers/gpu/drm/exynos/exynos_mixer.c | 71 +++-
drivers/interconnect/core.c | 12 +-
include/linux/interconnect-provider.h | 6 +
6 files changed, 327 insertions(+), 92 deletions(-)
--
2.17.1
From: Artur Świgoń <[email protected]>
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ń <[email protected]>
Acked-by: Krzysztof Kozlowski <[email protected]>
---
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 95850700e367..f357c3a78858 100644
--- a/drivers/interconnect/core.c
+++ b/drivers/interconnect/core.c
@@ -284,7 +284,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
From: Artur Świgoń <[email protected]>
The exynos-bus devfreq driver is extended with interconnect functionality
by a subsequent patch. This patch removes a check from the interconnect
framework that prevents interconnect from working on exynos-bus, in which
every bus is a separate interconnect provider.
Signed-off-by: Artur Świgoń <[email protected]>
---
drivers/interconnect/core.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
index f357c3a78858..e8243665d5ba 100644
--- a/drivers/interconnect/core.c
+++ b/drivers/interconnect/core.c
@@ -224,11 +224,8 @@ static int apply_constraints(struct icc_path *path)
for (i = 0; i < path->num_nodes; i++) {
next = path->reqs[i].node;
- /*
- * 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) {
prev = next;
continue;
}
--
2.17.1
From: Artur Świgoń <[email protected]>
This patch adds minor improvements to the exynos-bus driver.
Signed-off-by: Artur Świgoń <[email protected]>
Reviewed-by: Krzysztof Kozlowski <[email protected]>
---
drivers/devfreq/exynos-bus.c | 66 ++++++++++++++----------------------
1 file changed, 25 insertions(+), 41 deletions(-)
diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
index 60ad4319fd80..8d44810cac69 100644
--- a/drivers/devfreq/exynos-bus.c
+++ b/drivers/devfreq/exynos-bus.c
@@ -15,11 +15,10 @@
#include <linux/device.h>
#include <linux/export.h>
#include <linux/module.h>
-#include <linux/of_device.h>
+#include <linux/of.h>
#include <linux/pm_opp.h>
#include <linux/platform_device.h>
#include <linux/regulator/consumer.h>
-#include <linux/slab.h>
#define DEFAULT_SATURATION_RATIO 40
@@ -178,7 +177,7 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
struct device *dev = bus->dev;
struct opp_table *opp_table;
const char *vdd = "vdd";
- int i, ret, count, size;
+ int i, ret, count;
opp_table = dev_pm_opp_set_regulators(dev, &vdd, 1);
if (IS_ERR(opp_table)) {
@@ -201,8 +200,7 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
}
bus->edev_count = count;
- size = sizeof(*bus->edev) * count;
- bus->edev = devm_kzalloc(dev, size, GFP_KERNEL);
+ bus->edev = devm_kcalloc(dev, count, sizeof(*bus->edev), GFP_KERNEL);
if (!bus->edev) {
ret = -ENOMEM;
goto err_regulator;
@@ -301,10 +299,9 @@ static int exynos_bus_profile_init(struct exynos_bus *bus,
profile->exit = exynos_bus_exit;
ondemand_data = devm_kzalloc(dev, sizeof(*ondemand_data), GFP_KERNEL);
- if (!ondemand_data) {
- ret = -ENOMEM;
- goto err;
- }
+ if (!ondemand_data)
+ return -ENOMEM;
+
ondemand_data->upthreshold = 40;
ondemand_data->downdifferential = 5;
@@ -314,8 +311,7 @@ static int exynos_bus_profile_init(struct exynos_bus *bus,
ondemand_data);
if (IS_ERR(bus->devfreq)) {
dev_err(dev, "failed to add devfreq device\n");
- ret = PTR_ERR(bus->devfreq);
- goto err;
+ return PTR_ERR(bus->devfreq);
}
/*
@@ -325,16 +321,13 @@ static int exynos_bus_profile_init(struct exynos_bus *bus,
ret = exynos_bus_enable_edev(bus);
if (ret < 0) {
dev_err(dev, "failed to enable devfreq-event devices\n");
- goto err;
+ return ret;
}
ret = exynos_bus_set_event(bus);
- if (ret < 0) {
+ if (ret < 0)
dev_err(dev, "failed to set event to devfreq-event devices\n");
- goto err;
- }
-err:
return ret;
}
@@ -344,7 +337,6 @@ static int exynos_bus_profile_init_passive(struct exynos_bus *bus,
struct device *dev = bus->dev;
struct devfreq_passive_data *passive_data;
struct devfreq *parent_devfreq;
- int ret = 0;
/* Initialize the struct profile and governor data for passive device */
profile->target = exynos_bus_target;
@@ -352,30 +344,26 @@ static int exynos_bus_profile_init_passive(struct exynos_bus *bus,
/* Get the instance of parent devfreq device */
parent_devfreq = devfreq_get_devfreq_by_phandle(dev, 0);
- if (IS_ERR(parent_devfreq)) {
- ret = -EPROBE_DEFER;
- goto err;
- }
+ if (IS_ERR(parent_devfreq))
+ return -EPROBE_DEFER;
passive_data = devm_kzalloc(dev, sizeof(*passive_data), GFP_KERNEL);
- if (!passive_data) {
- ret = -ENOMEM;
- goto err;
- }
+ if (!passive_data)
+ return -ENOMEM;
+
passive_data->parent = parent_devfreq;
/* Add devfreq device for exynos bus with passive governor */
- bus->devfreq = devm_devfreq_add_device(dev, profile, DEVFREQ_GOV_PASSIVE,
+ bus->devfreq = devm_devfreq_add_device(dev, profile,
+ DEVFREQ_GOV_PASSIVE,
passive_data);
if (IS_ERR(bus->devfreq)) {
dev_err(dev,
"failed to add devfreq dev with passive governor\n");
- ret = PTR_ERR(bus->devfreq);
- goto err;
+ return PTR_ERR(bus->devfreq);
}
-err:
- return ret;
+ return 0;
}
static int exynos_bus_probe(struct platform_device *pdev)
@@ -393,18 +381,18 @@ static int exynos_bus_probe(struct platform_device *pdev)
return -EINVAL;
}
- bus = devm_kzalloc(&pdev->dev, sizeof(*bus), GFP_KERNEL);
+ bus = devm_kzalloc(dev, sizeof(*bus), GFP_KERNEL);
if (!bus)
return -ENOMEM;
mutex_init(&bus->lock);
- bus->dev = &pdev->dev;
+ bus->dev = dev;
platform_set_drvdata(pdev, bus);
profile = devm_kzalloc(dev, sizeof(*profile), GFP_KERNEL);
if (!profile)
return -ENOMEM;
- node = of_parse_phandle(dev->of_node, "devfreq", 0);
+ node = of_parse_phandle(np, "devfreq", 0);
if (node) {
of_node_put(node);
passive = true;
@@ -461,12 +449,10 @@ static int exynos_bus_resume(struct device *dev)
int ret;
ret = exynos_bus_enable_edev(bus);
- if (ret < 0) {
+ if (ret < 0)
dev_err(dev, "failed to enable the devfreq-event devices\n");
- return ret;
- }
- return 0;
+ return ret;
}
static int exynos_bus_suspend(struct device *dev)
@@ -475,12 +461,10 @@ static int exynos_bus_suspend(struct device *dev)
int ret;
ret = exynos_bus_disable_edev(bus);
- if (ret < 0) {
+ if (ret < 0)
dev_err(dev, "failed to disable the devfreq-event devices\n");
- return ret;
- }
- return 0;
+ return ret;
}
#endif
--
2.17.1
Hi Artur,
On 19. 9. 19. 오후 11:22, Artur Świgoń wrote:
> From: Artur Świgoń <[email protected]>
>
> This patch adds minor improvements to the exynos-bus driver.
>
> Signed-off-by: Artur Świgoń <[email protected]>
> Reviewed-by: Krzysztof Kozlowski <[email protected]>
> ---
> drivers/devfreq/exynos-bus.c | 66 ++++++++++++++----------------------
> 1 file changed, 25 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
> index 60ad4319fd80..8d44810cac69 100644
> --- a/drivers/devfreq/exynos-bus.c
> +++ b/drivers/devfreq/exynos-bus.c
> @@ -15,11 +15,10 @@
> #include <linux/device.h>
> #include <linux/export.h>
> #include <linux/module.h>
> -#include <linux/of_device.h>
> +#include <linux/of.h>
> #include <linux/pm_opp.h>
> #include <linux/platform_device.h>
> #include <linux/regulator/consumer.h>
> -#include <linux/slab.h>
>
> #define DEFAULT_SATURATION_RATIO 40
>
> @@ -178,7 +177,7 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
> struct device *dev = bus->dev;
> struct opp_table *opp_table;
> const char *vdd = "vdd";
> - int i, ret, count, size;
> + int i, ret, count;
>
> opp_table = dev_pm_opp_set_regulators(dev, &vdd, 1);
> if (IS_ERR(opp_table)) {
> @@ -201,8 +200,7 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
> }
> bus->edev_count = count;
>
> - size = sizeof(*bus->edev) * count;
> - bus->edev = devm_kzalloc(dev, size, GFP_KERNEL);
> + bus->edev = devm_kcalloc(dev, count, sizeof(*bus->edev), GFP_KERNEL);
> if (!bus->edev) {
> ret = -ENOMEM;
> goto err_regulator;
> @@ -301,10 +299,9 @@ static int exynos_bus_profile_init(struct exynos_bus *bus,
> profile->exit = exynos_bus_exit;
>
> ondemand_data = devm_kzalloc(dev, sizeof(*ondemand_data), GFP_KERNEL);
> - if (!ondemand_data) {
> - ret = -ENOMEM;
> - goto err;
> - }
> + if (!ondemand_data)
> + return -ENOMEM;
> +
> ondemand_data->upthreshold = 40;
> ondemand_data->downdifferential = 5;
>
> @@ -314,8 +311,7 @@ static int exynos_bus_profile_init(struct exynos_bus *bus,
> ondemand_data);
> if (IS_ERR(bus->devfreq)) {
> dev_err(dev, "failed to add devfreq device\n");
> - ret = PTR_ERR(bus->devfreq);
> - goto err;
> + return PTR_ERR(bus->devfreq);
> }
>
> /*
> @@ -325,16 +321,13 @@ static int exynos_bus_profile_init(struct exynos_bus *bus,
> ret = exynos_bus_enable_edev(bus);
> if (ret < 0) {
> dev_err(dev, "failed to enable devfreq-event devices\n");
> - goto err;
> + return ret;
> }
>
> ret = exynos_bus_set_event(bus);
> - if (ret < 0) {
> + if (ret < 0)
> dev_err(dev, "failed to set event to devfreq-event devices\n");
> - goto err;
Instead of removing 'goto err', just return err as I commented[1] on v1.
[1] https://lkml.org/lkml/2019/7/26/331
> - }
>
> -err:
> return ret;
And you just keep 'return ret' or you can change it as 'return 0'.
> }
>
> @@ -344,7 +337,6 @@ static int exynos_bus_profile_init_passive(struct exynos_bus *bus,
> struct device *dev = bus->dev;
> struct devfreq_passive_data *passive_data;
> struct devfreq *parent_devfreq;
> - int ret = 0;
>
> /* Initialize the struct profile and governor data for passive device */
> profile->target = exynos_bus_target;
> @@ -352,30 +344,26 @@ static int exynos_bus_profile_init_passive(struct exynos_bus *bus,
>
> /* Get the instance of parent devfreq device */
> parent_devfreq = devfreq_get_devfreq_by_phandle(dev, 0);
> - if (IS_ERR(parent_devfreq)) {
> - ret = -EPROBE_DEFER;
> - goto err;
> - }
> + if (IS_ERR(parent_devfreq))
> + return -EPROBE_DEFER;
>
> passive_data = devm_kzalloc(dev, sizeof(*passive_data), GFP_KERNEL);
> - if (!passive_data) {
> - ret = -ENOMEM;
> - goto err;
> - }
> + if (!passive_data)
> + return -ENOMEM;
> +
> passive_data->parent = parent_devfreq;
>
> /* Add devfreq device for exynos bus with passive governor */
> - bus->devfreq = devm_devfreq_add_device(dev, profile, DEVFREQ_GOV_PASSIVE,
> + bus->devfreq = devm_devfreq_add_device(dev, profile,
> + DEVFREQ_GOV_PASSIVE,
> passive_data);
> if (IS_ERR(bus->devfreq)) {
> dev_err(dev,
> "failed to add devfreq dev with passive governor\n");
> - ret = PTR_ERR(bus->devfreq);
> - goto err;
> + return PTR_ERR(bus->devfreq);
> }
>
> -err:
> - return ret;
> + return 0;
> }
>
> static int exynos_bus_probe(struct platform_device *pdev)
> @@ -393,18 +381,18 @@ static int exynos_bus_probe(struct platform_device *pdev)
> return -EINVAL;
> }
>
> - bus = devm_kzalloc(&pdev->dev, sizeof(*bus), GFP_KERNEL);
> + bus = devm_kzalloc(dev, sizeof(*bus), GFP_KERNEL);
> if (!bus)
> return -ENOMEM;
> mutex_init(&bus->lock);
> - bus->dev = &pdev->dev;
> + bus->dev = dev;
> platform_set_drvdata(pdev, bus);
>
> profile = devm_kzalloc(dev, sizeof(*profile), GFP_KERNEL);
> if (!profile)
> return -ENOMEM;
>
> - node = of_parse_phandle(dev->of_node, "devfreq", 0);
> + node = of_parse_phandle(np, "devfreq", 0);
> if (node) {
> of_node_put(node);
> passive = true;
> @@ -461,12 +449,10 @@ static int exynos_bus_resume(struct device *dev)
> int ret;
>
> ret = exynos_bus_enable_edev(bus);
> - if (ret < 0) {
> + if (ret < 0)
> dev_err(dev, "failed to enable the devfreq-event devices\n");
> - return ret;
Keep the 'return ret' if error happen as I commented[1] on v1.
[1] https://lkml.org/lkml/2019/7/26/331
> - }
>
> - return 0;
> + return ret;
And you just keep 'return 0' or you can change it as 'return ret'.
> }
>
> static int exynos_bus_suspend(struct device *dev)
> @@ -475,12 +461,10 @@ static int exynos_bus_suspend(struct device *dev)
> int ret;
>
> ret = exynos_bus_disable_edev(bus);
> - if (ret < 0) {
> + if (ret < 0)
> dev_err(dev, "failed to disable the devfreq-event devices\n");
> - return ret;
Keep the 'return ret' if error happen as I commented[1] on v1.
[1] https://lkml.org/lkml/2019/7/26/331
> - }
>
> - return 0;
> + return ret;
And you just keep 'return 0' or you can change it as 'return ret'.
> }
> #endif
>
>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
Hi Artur,
On v1, I mentioned that we need to discuss how to change
the v2 for this. But, I have not received any reply from you on v1.
And, without your reply from v1, you just send v2.
I think that it is not proper development sequence.
I have spent many times to review your patches
and also I'll review your patches. You have to take care
the reply of reviewer and and keep the basic rule
of mailing contribution for discussion.
On 19. 9. 19. 오후 11:22, Artur Świgoń wrote:
> The following patchset adds interconnect[1][2] framework support to the
> exynos-bus devfreq driver. Extending the devfreq driver with interconnect
> capabilities started as a response to the issue referenced in [3]. The
> patches can be subdivided into four logical groups:
>
> (a) Refactoring the existing devfreq driver in order to improve readability
> and accommodate for adding new code (patches 01--04/11).
>
> (b) Tweaking the interconnect framework to support the exynos-bus use case
> (patches 05--07/11). Exporting of_icc_get_from_provider() allows us to
> avoid hardcoding every single graph edge in the DT or driver source, and
> relaxing the requirement contained in that function removes the need to
> provide dummy node IDs in the DT. Adjusting the logic in
> apply_constraints() (drivers/interconnect/core.c) accounts for the fact
> that every bus is a separate entity and therefore a separate interconnect
> provider, albeit constituting a part of a larger hierarchy.
>
> (c) Implementing interconnect providers in the exynos-bus devfreq driver
> and adding required DT properties for one selected platform, namely
> Exynos4412 (patches 08--09/11). Due to the fact that this aims to be a
> generic driver for various Exynos SoCs, node IDs are generated dynamically
> rather than hardcoded. This has been determined to be a simpler approach,
> but depends on changes described in (b).
>
> (d) Implementing a sample interconnect consumer for exynos-mixer targeted
> at the issue referenced in [3], again with DT info only for Exynos4412
> (patches 10--11/11).
>
> Integration of devfreq and interconnect functionalities is achieved by
> using dev_pm_qos_*() API[5]. All new code works equally well when
> CONFIG_INTERCONNECT is 'n' (as in exynos_defconfig) in which case all
> interconnect API functions are no-ops.
>
> This patchset depends on [5].
>
> --- Changes since v1 [6]:
> * Rebase on [4] (coupled regulators).
> * Rebase on [5] (dev_pm_qos for devfreq).
> * Use dev_pm_qos_*() API[5] instead of overriding frequency in
> exynos_bus_target().
> * Use IDR for node ID allocation.
> * Avoid goto in functions extracted in patches 01 & 02 (cf. patch 04).
> * Reverse order of multiplication and division in
> mixer_set_memory_bandwidth() (patch 11) 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/11083663/ (coupled regulators; merged)
> [5] https://patchwork.kernel.org/cover/11149497/ (dev_pm_qos for devfreq)
> [6] https://patchwork.kernel.org/cover/11054417/ (v1 of this RFC)
>
> Artur Świgoń (10):
> devfreq: exynos-bus: Extract exynos_bus_profile_init()
> devfreq: exynos-bus: Extract exynos_bus_profile_init_passive()
> devfreq: exynos-bus: Change goto-based logic to if-else logic
> devfreq: exynos-bus: Clean up code
> interconnect: Export of_icc_get_from_provider()
> interconnect: Relax requirement in of_icc_get_from_provider()
> interconnect: Relax condition in apply_constraints()
> arm: dts: exynos: Add parents and #interconnect-cells to 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 | 1 +
> arch/arm/boot/dts/exynos4412.dtsi | 10 +
> drivers/devfreq/exynos-bus.c | 319 +++++++++++++-----
> drivers/gpu/drm/exynos/exynos_mixer.c | 71 +++-
> drivers/interconnect/core.c | 12 +-
> include/linux/interconnect-provider.h | 6 +
> 6 files changed, 327 insertions(+), 92 deletions(-)
>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
Hi Artur,
I tried to just build this patch on mainline kernel or linux-next.
But, when I applied them, merge conflict happens. You didn't develop
them on latest version. Please rebase them based on latest mainline kernel.
On 19. 9. 20. 오전 10:07, Chanwoo Choi wrote:
> Hi Artur,
>
> On v1, I mentioned that we need to discuss how to change
> the v2 for this. But, I have not received any reply from you on v1.
> And, without your reply from v1, you just send v2.
>
> I think that it is not proper development sequence.
> I have spent many times to review your patches
> and also I'll review your patches. You have to take care
> the reply of reviewer and and keep the basic rule
> of mailing contribution for discussion.
>
> On 19. 9. 19. 오후 11:22, Artur Świgoń wrote:
>> The following patchset adds interconnect[1][2] framework support to the
>> exynos-bus devfreq driver. Extending the devfreq driver with interconnect
>> capabilities started as a response to the issue referenced in [3]. The
>> patches can be subdivided into four logical groups:
>>
>> (a) Refactoring the existing devfreq driver in order to improve readability
>> and accommodate for adding new code (patches 01--04/11).
>>
>> (b) Tweaking the interconnect framework to support the exynos-bus use case
>> (patches 05--07/11). Exporting of_icc_get_from_provider() allows us to
>> avoid hardcoding every single graph edge in the DT or driver source, and
>> relaxing the requirement contained in that function removes the need to
>> provide dummy node IDs in the DT. Adjusting the logic in
>> apply_constraints() (drivers/interconnect/core.c) accounts for the fact
>> that every bus is a separate entity and therefore a separate interconnect
>> provider, albeit constituting a part of a larger hierarchy.
>>
>> (c) Implementing interconnect providers in the exynos-bus devfreq driver
>> and adding required DT properties for one selected platform, namely
>> Exynos4412 (patches 08--09/11). Due to the fact that this aims to be a
>> generic driver for various Exynos SoCs, node IDs are generated dynamically
>> rather than hardcoded. This has been determined to be a simpler approach,
>> but depends on changes described in (b).
>>
>> (d) Implementing a sample interconnect consumer for exynos-mixer targeted
>> at the issue referenced in [3], again with DT info only for Exynos4412
>> (patches 10--11/11).
>>
>> Integration of devfreq and interconnect functionalities is achieved by
>> using dev_pm_qos_*() API[5]. All new code works equally well when
>> CONFIG_INTERCONNECT is 'n' (as in exynos_defconfig) in which case all
>> interconnect API functions are no-ops.
>>
>> This patchset depends on [5].
>>
>> --- Changes since v1 [6]:
>> * Rebase on [4] (coupled regulators).
>> * Rebase on [5] (dev_pm_qos for devfreq).
>> * Use dev_pm_qos_*() API[5] instead of overriding frequency in
>> exynos_bus_target().
>> * Use IDR for node ID allocation.
>> * Avoid goto in functions extracted in patches 01 & 02 (cf. patch 04).
>> * Reverse order of multiplication and division in
>> mixer_set_memory_bandwidth() (patch 11) 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/11083663/ (coupled regulators; merged)
>> [5] https://patchwork.kernel.org/cover/11149497/ (dev_pm_qos for devfreq)
>> [6] https://patchwork.kernel.org/cover/11054417/ (v1 of this RFC)
>>
>> Artur Świgoń (10):
>> devfreq: exynos-bus: Extract exynos_bus_profile_init()
>> devfreq: exynos-bus: Extract exynos_bus_profile_init_passive()
>> devfreq: exynos-bus: Change goto-based logic to if-else logic
>> devfreq: exynos-bus: Clean up code
>> interconnect: Export of_icc_get_from_provider()
>> interconnect: Relax requirement in of_icc_get_from_provider()
>> interconnect: Relax condition in apply_constraints()
>> arm: dts: exynos: Add parents and #interconnect-cells to 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 | 1 +
>> arch/arm/boot/dts/exynos4412.dtsi | 10 +
>> drivers/devfreq/exynos-bus.c | 319 +++++++++++++-----
>> drivers/gpu/drm/exynos/exynos_mixer.c | 71 +++-
>> drivers/interconnect/core.c | 12 +-
>> include/linux/interconnect-provider.h | 6 +
>> 6 files changed, 327 insertions(+), 92 deletions(-)
>>
>
>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
Hi,
On Fri, 2019-09-20 at 11:14 +0900, Chanwoo Choi wrote:
> Hi Artur,
>
> I tried to just build this patch on mainline kernel or linux-next.
> But, when I applied them, merge conflict happens. You didn't develop
> them on latest version. Please rebase them based on latest mainline kernel.
I developed on top of next-20190918 on which I applied
https://patchwork.kernel.org/cover/11149497/ as I mentioned in the cover
letter. The dev_pm_qos patches and my RFC have just cleanly rebased together on
top of next-20190920. Could you check if you have the dev_pm_qos patches (v5,
the version number is missing in this one; link above) and if so, where does the
conflict appear?
> On 19. 9. 20. 오전 10:07, Chanwoo Choi wrote:
> > Hi Artur,
> >
> > On v1, I mentioned that we need to discuss how to change
> > the v2 for this. But, I have not received any reply from you on v1.
> > And, without your reply from v1, you just send v2.
> >
> > I think that it is not proper development sequence.
> > I have spent many times to review your patches
> > and also I'll review your patches. You have to take care
> > the reply of reviewer and and keep the basic rule
> > of mailing contribution for discussion.
> >
> > On 19. 9. 19. 오후 11:22, Artur Świgoń wrote:
> > > The following patchset adds interconnect[1][2] framework support to the
> > > exynos-bus devfreq driver. Extending the devfreq driver with interconnect
> > > capabilities started as a response to the issue referenced in [3]. The
> > > patches can be subdivided into four logical groups:
> > >
> > > (a) Refactoring the existing devfreq driver in order to improve readability
> > > and accommodate for adding new code (patches 01--04/11).
> > >
> > > (b) Tweaking the interconnect framework to support the exynos-bus use case
> > > (patches 05--07/11). Exporting of_icc_get_from_provider() allows us to
> > > avoid hardcoding every single graph edge in the DT or driver source, and
> > > relaxing the requirement contained in that function removes the need to
> > > provide dummy node IDs in the DT. Adjusting the logic in
> > > apply_constraints() (drivers/interconnect/core.c) accounts for the fact
> > > that every bus is a separate entity and therefore a separate interconnect
> > > provider, albeit constituting a part of a larger hierarchy.
> > >
> > > (c) Implementing interconnect providers in the exynos-bus devfreq driver
> > > and adding required DT properties for one selected platform, namely
> > > Exynos4412 (patches 08--09/11). Due to the fact that this aims to be a
> > > generic driver for various Exynos SoCs, node IDs are generated dynamically
> > > rather than hardcoded. This has been determined to be a simpler approach,
> > > but depends on changes described in (b).
> > >
> > > (d) Implementing a sample interconnect consumer for exynos-mixer targeted
> > > at the issue referenced in [3], again with DT info only for Exynos4412
> > > (patches 10--11/11).
> > >
> > > Integration of devfreq and interconnect functionalities is achieved by
> > > using dev_pm_qos_*() API[5]. All new code works equally well when
> > > CONFIG_INTERCONNECT is 'n' (as in exynos_defconfig) in which case all
> > > interconnect API functions are no-ops.
> > >
> > > This patchset depends on [5].
> > >
> > > --- Changes since v1 [6]:
> > > * Rebase on [4] (coupled regulators).
> > > * Rebase on [5] (dev_pm_qos for devfreq).
> > > * Use dev_pm_qos_*() API[5] instead of overriding frequency in
> > > exynos_bus_target().
> > > * Use IDR for node ID allocation.
> > > * Avoid goto in functions extracted in patches 01 & 02 (cf. patch 04).
> > > * Reverse order of multiplication and division in
> > > mixer_set_memory_bandwidth() (patch 11) 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/11083663/ (coupled regulators; merged)
> > > [5] https://patchwork.kernel.org/cover/11149497/ (dev_pm_qos for devfreq)
> > > [6] https://patchwork.kernel.org/cover/11054417/ (v1 of this RFC)
> > >
> > > Artur Świgoń (10):
> > > devfreq: exynos-bus: Extract exynos_bus_profile_init()
> > > devfreq: exynos-bus: Extract exynos_bus_profile_init_passive()
> > > devfreq: exynos-bus: Change goto-based logic to if-else logic
> > > devfreq: exynos-bus: Clean up code
> > > interconnect: Export of_icc_get_from_provider()
> > > interconnect: Relax requirement in of_icc_get_from_provider()
> > > interconnect: Relax condition in apply_constraints()
> > > arm: dts: exynos: Add parents and #interconnect-cells to 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 | 1 +
> > > arch/arm/boot/dts/exynos4412.dtsi | 10 +
> > > drivers/devfreq/exynos-bus.c | 319 +++++++++++++-----
> > > drivers/gpu/drm/exynos/exynos_mixer.c | 71 +++-
> > > drivers/interconnect/core.c | 12 +-
> > > include/linux/interconnect-provider.h | 6 +
> > > 6 files changed, 327 insertions(+), 92 deletions(-)
> > >
> >
> >
>
>
--
Artur Świgoń
Samsung R&D Institute Poland
Samsung Electronics
Hi,
On 19. 9. 25. 오후 2:47, Artur Świgoń wrote:
> Hi,
>
> On Fri, 2019-09-20 at 11:14 +0900, Chanwoo Choi wrote:
>> Hi Artur,
>>
>> I tried to just build this patch on mainline kernel or linux-next.
>> But, when I applied them, merge conflict happens. You didn't develop
>> them on latest version. Please rebase them based on latest mainline kernel.
>
> I developed on top of next-20190918 on which I applied
> https://patchwork.kernel.org/cover/11149497/ as I mentioned in the cover
> letter. The dev_pm_qos patches and my RFC have just cleanly rebased together on
> top of next-20190920. Could you check if you have the dev_pm_qos patches (v5,
> the version number is missing in this one; link above) and if so, where does the
> conflict appear?
I faced on the merge conflict of drivers/devfreq/exynos-bus.c.
I think that It is not related to to dev_pm_qos patch.
Maybe, Kamil's patches[1] changed the many things of exynos-bus.c
If your test branch doesn't contain following patches,
you need to rebase your patches based on latest mainline kernel
from Linus Torvald.
[1] https://patchwork.kernel.org/cover/11083663/
- [RESEND PATCH v5 0/4] add coupled regulators for Exynos5422/5800
Today, I tried to apply these patch again based on latest mainline kernel.
The merge conflict happen still.
- merge conflict log
Applying: devfreq: exynos-bus: Extract exynos_bus_profile_init()
error: patch failed: drivers/devfreq/exynos-bus.c:334
error: drivers/devfreq/exynos-bus.c: patch does not apply
Patch failed at 0001 devfreq: exynos-bus: Extract exynos_bus_profile_init()
>
>> On 19. 9. 20. 오전 10:07, Chanwoo Choi wrote:
>>> Hi Artur,
>>>
>>> On v1, I mentioned that we need to discuss how to change
>>> the v2 for this. But, I have not received any reply from you on v1.
>>> And, without your reply from v1, you just send v2.
>>>
>>> I think that it is not proper development sequence.
>>> I have spent many times to review your patches
>>> and also I'll review your patches. You have to take care
>>> the reply of reviewer and and keep the basic rule
>>> of mailing contribution for discussion.
>>>
>>> On 19. 9. 19. 오후 11:22, Artur Świgoń wrote:
>>>> The following patchset adds interconnect[1][2] framework support to the
>>>> exynos-bus devfreq driver. Extending the devfreq driver with interconnect
>>>> capabilities started as a response to the issue referenced in [3]. The
>>>> patches can be subdivided into four logical groups:
>>>>
>>>> (a) Refactoring the existing devfreq driver in order to improve readability
>>>> and accommodate for adding new code (patches 01--04/11).
>>>>
>>>> (b) Tweaking the interconnect framework to support the exynos-bus use case
>>>> (patches 05--07/11). Exporting of_icc_get_from_provider() allows us to
>>>> avoid hardcoding every single graph edge in the DT or driver source, and
>>>> relaxing the requirement contained in that function removes the need to
>>>> provide dummy node IDs in the DT. Adjusting the logic in
>>>> apply_constraints() (drivers/interconnect/core.c) accounts for the fact
>>>> that every bus is a separate entity and therefore a separate interconnect
>>>> provider, albeit constituting a part of a larger hierarchy.
>>>>
>>>> (c) Implementing interconnect providers in the exynos-bus devfreq driver
>>>> and adding required DT properties for one selected platform, namely
>>>> Exynos4412 (patches 08--09/11). Due to the fact that this aims to be a
>>>> generic driver for various Exynos SoCs, node IDs are generated dynamically
>>>> rather than hardcoded. This has been determined to be a simpler approach,
>>>> but depends on changes described in (b).
>>>>
>>>> (d) Implementing a sample interconnect consumer for exynos-mixer targeted
>>>> at the issue referenced in [3], again with DT info only for Exynos4412
>>>> (patches 10--11/11).
>>>>
>>>> Integration of devfreq and interconnect functionalities is achieved by
>>>> using dev_pm_qos_*() API[5]. All new code works equally well when
>>>> CONFIG_INTERCONNECT is 'n' (as in exynos_defconfig) in which case all
>>>> interconnect API functions are no-ops.
>>>>
>>>> This patchset depends on [5].
>>>>
>>>> --- Changes since v1 [6]:
>>>> * Rebase on [4] (coupled regulators).
>>>> * Rebase on [5] (dev_pm_qos for devfreq).
>>>> * Use dev_pm_qos_*() API[5] instead of overriding frequency in
>>>> exynos_bus_target().
>>>> * Use IDR for node ID allocation.
>>>> * Avoid goto in functions extracted in patches 01 & 02 (cf. patch 04).
>>>> * Reverse order of multiplication and division in
>>>> mixer_set_memory_bandwidth() (patch 11) 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/11083663/ (coupled regulators; merged)
>>>> [5] https://patchwork.kernel.org/cover/11149497/ (dev_pm_qos for devfreq)
>>>> [6] https://patchwork.kernel.org/cover/11054417/ (v1 of this RFC)
>>>>
>>>> Artur Świgoń (10):
>>>> devfreq: exynos-bus: Extract exynos_bus_profile_init()
>>>> devfreq: exynos-bus: Extract exynos_bus_profile_init_passive()
>>>> devfreq: exynos-bus: Change goto-based logic to if-else logic
>>>> devfreq: exynos-bus: Clean up code
>>>> interconnect: Export of_icc_get_from_provider()
>>>> interconnect: Relax requirement in of_icc_get_from_provider()
>>>> interconnect: Relax condition in apply_constraints()
>>>> arm: dts: exynos: Add parents and #interconnect-cells to 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 | 1 +
>>>> arch/arm/boot/dts/exynos4412.dtsi | 10 +
>>>> drivers/devfreq/exynos-bus.c | 319 +++++++++++++-----
>>>> drivers/gpu/drm/exynos/exynos_mixer.c | 71 +++-
>>>> drivers/interconnect/core.c | 12 +-
>>>> include/linux/interconnect-provider.h | 6 +
>>>> 6 files changed, 327 insertions(+), 92 deletions(-)
>>>>
>>>
>>>
>>
>>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
On Wed, 2019-09-25 at 15:12 +0900, Chanwoo Choi wrote:
> Hi,
>
> On 19. 9. 25. 오후 2:47, Artur Świgoń wrote:
> > Hi,
> >
> > On Fri, 2019-09-20 at 11:14 +0900, Chanwoo Choi wrote:
> > > Hi Artur,
> > >
> > > I tried to just build this patch on mainline kernel or linux-next.
> > > But, when I applied them, merge conflict happens. You didn't develop
> > > them on latest version. Please rebase them based on latest mainline kernel.
> >
> > I developed on top of next-20190918 on which I applied
> > https://patchwork.kernel.org/cover/11149497/ as I mentioned in the cover
> > letter. The dev_pm_qos patches and my RFC have just cleanly rebased together on
> > top of next-20190920. Could you check if you have the dev_pm_qos patches (v5,
> > the version number is missing in this one; link above) and if so, where does the
> > conflict appear?
>
> I faced on the merge conflict of drivers/devfreq/exynos-bus.c.
> I think that It is not related to to dev_pm_qos patch.
I think that it is actually related to the specific version of dev_pm_qos (v5) that
I used because patch 08/08 of dev_pm_qos series modifies exynos_bus_probe() in
drivers/devfreq/exynos-bus.c (https://patchwork.kernel.org/patch/11149507/).
I will rebase the next RFC (v3) on latest dev_pm_qos patches from Leonard and the
latest Linux-next kernel.
> Maybe, Kamil's patches[1] changed the many things of exynos-bus.c
> If your test branch doesn't contain following patches,
> you need to rebase your patches based on latest mainline kernel
> from Linus Torvald.
> [1] https://patchwork.kernel.org/cover/11083663/
> - [RESEND PATCH v5 0/4] add coupled regulators for Exynos5422/5800
Yes, requiring Kamil's patches is one of the changes in this RFC (v2), since they
are already merged.
> Today, I tried to apply these patch again based on latest mainline kernel.
> The merge conflict happen still.
>
> - merge conflict log
> Applying: devfreq: exynos-bus: Extract exynos_bus_profile_init()
> error: patch failed: drivers/devfreq/exynos-bus.c:334
> error: drivers/devfreq/exynos-bus.c: patch does not apply
> Patch failed at 0001 devfreq: exynos-bus: Extract exynos_bus_profile_init()
>
>
> >
> > > On 19. 9. 20. 오전 10:07, Chanwoo Choi wrote:
> > > > Hi Artur,
> > > >
> > > > On v1, I mentioned that we need to discuss how to change
> > > > the v2 for this. But, I have not received any reply from you on v1.
> > > > And, without your reply from v1, you just send v2.
> > > >
> > > > I think that it is not proper development sequence.
> > > > I have spent many times to review your patches
> > > > and also I'll review your patches. You have to take care
> > > > the reply of reviewer and and keep the basic rule
> > > > of mailing contribution for discussion.
> > > >
> > > > On 19. 9. 19. 오후 11:22, Artur Świgoń wrote:
> > > > > The following patchset adds interconnect[1][2] framework support to the
> > > > > exynos-bus devfreq driver. Extending the devfreq driver with interconnect
> > > > > capabilities started as a response to the issue referenced in [3]. The
> > > > > patches can be subdivided into four logical groups:
> > > > >
> > > > > (a) Refactoring the existing devfreq driver in order to improve readability
> > > > > and accommodate for adding new code (patches 01--04/11).
> > > > >
> > > > > (b) Tweaking the interconnect framework to support the exynos-bus use case
> > > > > (patches 05--07/11). Exporting of_icc_get_from_provider() allows us to
> > > > > avoid hardcoding every single graph edge in the DT or driver source, and
> > > > > relaxing the requirement contained in that function removes the need to
> > > > > provide dummy node IDs in the DT. Adjusting the logic in
> > > > > apply_constraints() (drivers/interconnect/core.c) accounts for the fact
> > > > > that every bus is a separate entity and therefore a separate interconnect
> > > > > provider, albeit constituting a part of a larger hierarchy.
> > > > >
> > > > > (c) Implementing interconnect providers in the exynos-bus devfreq driver
> > > > > and adding required DT properties for one selected platform, namely
> > > > > Exynos4412 (patches 08--09/11). Due to the fact that this aims to be a
> > > > > generic driver for various Exynos SoCs, node IDs are generated dynamically
> > > > > rather than hardcoded. This has been determined to be a simpler approach,
> > > > > but depends on changes described in (b).
> > > > >
> > > > > (d) Implementing a sample interconnect consumer for exynos-mixer targeted
> > > > > at the issue referenced in [3], again with DT info only for Exynos4412
> > > > > (patches 10--11/11).
> > > > >
> > > > > Integration of devfreq and interconnect functionalities is achieved by
> > > > > using dev_pm_qos_*() API[5]. All new code works equally well when
> > > > > CONFIG_INTERCONNECT is 'n' (as in exynos_defconfig) in which case all
> > > > > interconnect API functions are no-ops.
> > > > >
> > > > > This patchset depends on [5].
> > > > >
> > > > > --- Changes since v1 [6]:
> > > > > * Rebase on [4] (coupled regulators).
> > > > > * Rebase on [5] (dev_pm_qos for devfreq).
> > > > > * Use dev_pm_qos_*() API[5] instead of overriding frequency in
> > > > > exynos_bus_target().
> > > > > * Use IDR for node ID allocation.
> > > > > * Avoid goto in functions extracted in patches 01 & 02 (cf. patch 04).
> > > > > * Reverse order of multiplication and division in
> > > > > mixer_set_memory_bandwidth() (patch 11) 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/11083663/ (coupled regulators; merged)
> > > > > [5] https://patchwork.kernel.org/cover/11149497/ (dev_pm_qos for devfreq)
> > > > > [6] https://patchwork.kernel.org/cover/11054417/ (v1 of this RFC)
> > > > >
> > > > > Artur Świgoń (10):
> > > > > devfreq: exynos-bus: Extract exynos_bus_profile_init()
> > > > > devfreq: exynos-bus: Extract exynos_bus_profile_init_passive()
> > > > > devfreq: exynos-bus: Change goto-based logic to if-else logic
> > > > > devfreq: exynos-bus: Clean up code
> > > > > interconnect: Export of_icc_get_from_provider()
> > > > > interconnect: Relax requirement in of_icc_get_from_provider()
> > > > > interconnect: Relax condition in apply_constraints()
> > > > > arm: dts: exynos: Add parents and #interconnect-cells to 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 | 1 +
> > > > > arch/arm/boot/dts/exynos4412.dtsi | 10 +
> > > > > drivers/devfreq/exynos-bus.c | 319 +++++++++++++-----
> > > > > drivers/gpu/drm/exynos/exynos_mixer.c | 71 +++-
> > > > > drivers/interconnect/core.c | 12 +-
> > > > > include/linux/interconnect-provider.h | 6 +
> > > > > 6 files changed, 327 insertions(+), 92 deletions(-)
> > > > >
Hi,
On 19. 9. 25. 오후 3:37, Artur Świgoń wrote:
> On Wed, 2019-09-25 at 15:12 +0900, Chanwoo Choi wrote:
>> Hi,
>>
>> On 19. 9. 25. 오후 2:47, Artur Świgoń wrote:
>>> Hi,
>>>
>>> On Fri, 2019-09-20 at 11:14 +0900, Chanwoo Choi wrote:
>>>> Hi Artur,
>>>>
>>>> I tried to just build this patch on mainline kernel or linux-next.
>>>> But, when I applied them, merge conflict happens. You didn't develop
>>>> them on latest version. Please rebase them based on latest mainline kernel.
>>>
>>> I developed on top of next-20190918 on which I applied
>>> https://patchwork.kernel.org/cover/11149497/ as I mentioned in the cover
>>> letter. The dev_pm_qos patches and my RFC have just cleanly rebased together on
>>> top of next-20190920. Could you check if you have the dev_pm_qos patches (v5,
>>> the version number is missing in this one; link above) and if so, where does the
>>> conflict appear?
>>
>> I faced on the merge conflict of drivers/devfreq/exynos-bus.c.
>> I think that It is not related to to dev_pm_qos patch.
>
> I think that it is actually related to the specific version of dev_pm_qos (v5) that
> I used because patch 08/08 of dev_pm_qos series modifies exynos_bus_probe() in
> drivers/devfreq/exynos-bus.c (https://patchwork.kernel.org/patch/11149507/).
>
> I will rebase the next RFC (v3) on latest dev_pm_qos patches from Leonard and the
> latest Linux-next kernel.
My mistake. I only checked the Leonard's latest patches(v8)
which doesn't contain this patch. OK. I'll try again. Thanks.
[1] https://patchwork.kernel.org/patch/11149507/
- PM / devfreq: Move opp notifier registration to core
>
>> Maybe, Kamil's patches[1] changed the many things of exynos-bus.c
>> If your test branch doesn't contain following patches,
>> you need to rebase your patches based on latest mainline kernel
>> from Linus Torvald.
>> [1] https://patchwork.kernel.org/cover/11083663/
>> - [RESEND PATCH v5 0/4] add coupled regulators for Exynos5422/5800
>
> Yes, requiring Kamil's patches is one of the changes in this RFC (v2), since they
> are already merged.
>
>> Today, I tried to apply these patch again based on latest mainline kernel.
>> The merge conflict happen still.
>>
>> - merge conflict log
>> Applying: devfreq: exynos-bus: Extract exynos_bus_profile_init()
>> error: patch failed: drivers/devfreq/exynos-bus.c:334
>> error: drivers/devfreq/exynos-bus.c: patch does not apply
>> Patch failed at 0001 devfreq: exynos-bus: Extract exynos_bus_profile_init()
>>
>>
>>>
>>>> On 19. 9. 20. 오전 10:07, Chanwoo Choi wrote:
>>>>> Hi Artur,
>>>>>
>>>>> On v1, I mentioned that we need to discuss how to change
>>>>> the v2 for this. But, I have not received any reply from you on v1.
>>>>> And, without your reply from v1, you just send v2.
>>>>>
>>>>> I think that it is not proper development sequence.
>>>>> I have spent many times to review your patches
>>>>> and also I'll review your patches. You have to take care
>>>>> the reply of reviewer and and keep the basic rule
>>>>> of mailing contribution for discussion.
>>>>>
>>>>> On 19. 9. 19. 오후 11:22, Artur Świgoń wrote:
>>>>>> The following patchset adds interconnect[1][2] framework support to the
>>>>>> exynos-bus devfreq driver. Extending the devfreq driver with interconnect
>>>>>> capabilities started as a response to the issue referenced in [3]. The
>>>>>> patches can be subdivided into four logical groups:
>>>>>>
>>>>>> (a) Refactoring the existing devfreq driver in order to improve readability
>>>>>> and accommodate for adding new code (patches 01--04/11).
>>>>>>
>>>>>> (b) Tweaking the interconnect framework to support the exynos-bus use case
>>>>>> (patches 05--07/11). Exporting of_icc_get_from_provider() allows us to
>>>>>> avoid hardcoding every single graph edge in the DT or driver source, and
>>>>>> relaxing the requirement contained in that function removes the need to
>>>>>> provide dummy node IDs in the DT. Adjusting the logic in
>>>>>> apply_constraints() (drivers/interconnect/core.c) accounts for the fact
>>>>>> that every bus is a separate entity and therefore a separate interconnect
>>>>>> provider, albeit constituting a part of a larger hierarchy.
>>>>>>
>>>>>> (c) Implementing interconnect providers in the exynos-bus devfreq driver
>>>>>> and adding required DT properties for one selected platform, namely
>>>>>> Exynos4412 (patches 08--09/11). Due to the fact that this aims to be a
>>>>>> generic driver for various Exynos SoCs, node IDs are generated dynamically
>>>>>> rather than hardcoded. This has been determined to be a simpler approach,
>>>>>> but depends on changes described in (b).
>>>>>>
>>>>>> (d) Implementing a sample interconnect consumer for exynos-mixer targeted
>>>>>> at the issue referenced in [3], again with DT info only for Exynos4412
>>>>>> (patches 10--11/11).
>>>>>>
>>>>>> Integration of devfreq and interconnect functionalities is achieved by
>>>>>> using dev_pm_qos_*() API[5]. All new code works equally well when
>>>>>> CONFIG_INTERCONNECT is 'n' (as in exynos_defconfig) in which case all
>>>>>> interconnect API functions are no-ops.
>>>>>>
>>>>>> This patchset depends on [5].
>>>>>>
>>>>>> --- Changes since v1 [6]:
>>>>>> * Rebase on [4] (coupled regulators).
>>>>>> * Rebase on [5] (dev_pm_qos for devfreq).
>>>>>> * Use dev_pm_qos_*() API[5] instead of overriding frequency in
>>>>>> exynos_bus_target().
>>>>>> * Use IDR for node ID allocation.
>>>>>> * Avoid goto in functions extracted in patches 01 & 02 (cf. patch 04).
>>>>>> * Reverse order of multiplication and division in
>>>>>> mixer_set_memory_bandwidth() (patch 11) 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/11083663/ (coupled regulators; merged)
>>>>>> [5] https://patchwork.kernel.org/cover/11149497/ (dev_pm_qos for devfreq)
>>>>>> [6] https://patchwork.kernel.org/cover/11054417/ (v1 of this RFC)
>>>>>>
>>>>>> Artur Świgoń (10):
>>>>>> devfreq: exynos-bus: Extract exynos_bus_profile_init()
>>>>>> devfreq: exynos-bus: Extract exynos_bus_profile_init_passive()
>>>>>> devfreq: exynos-bus: Change goto-based logic to if-else logic
>>>>>> devfreq: exynos-bus: Clean up code
>>>>>> interconnect: Export of_icc_get_from_provider()
>>>>>> interconnect: Relax requirement in of_icc_get_from_provider()
>>>>>> interconnect: Relax condition in apply_constraints()
>>>>>> arm: dts: exynos: Add parents and #interconnect-cells to 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 | 1 +
>>>>>> arch/arm/boot/dts/exynos4412.dtsi | 10 +
>>>>>> drivers/devfreq/exynos-bus.c | 319 +++++++++++++-----
>>>>>> drivers/gpu/drm/exynos/exynos_mixer.c | 71 +++-
>>>>>> drivers/interconnect/core.c | 12 +-
>>>>>> include/linux/interconnect-provider.h | 6 +
>>>>>> 6 files changed, 327 insertions(+), 92 deletions(-)
>>>>>>
>
>
>
>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
Hi,
On Fri, 2019-09-20 at 11:22 +0900, Chanwoo Choi wrote:
> Hi Artur,
>
> On 19. 9. 19. 오후 11:22, Artur Świgoń wrote:
> > From: Artur Świgoń <[email protected]>
> >
> > This patch adds minor improvements to the exynos-bus driver.
> >
> > Signed-off-by: Artur Świgoń <[email protected]>
> > Reviewed-by: Krzysztof Kozlowski <[email protected]>
> > ---
> > drivers/devfreq/exynos-bus.c | 66 ++++++++++++++----------------------
> > 1 file changed, 25 insertions(+), 41 deletions(-)
> >
> > diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
> > index 60ad4319fd80..8d44810cac69 100644
> > --- a/drivers/devfreq/exynos-bus.c
> > +++ b/drivers/devfreq/exynos-bus.c
> > @@ -15,11 +15,10 @@
> > #include <linux/device.h>
> > #include <linux/export.h>
> > #include <linux/module.h>
> > -#include <linux/of_device.h>
> > +#include <linux/of.h>
> > #include <linux/pm_opp.h>
> > #include <linux/platform_device.h>
> > #include <linux/regulator/consumer.h>
> > -#include <linux/slab.h>
> >
> > #define DEFAULT_SATURATION_RATIO 40
> >
> > @@ -178,7 +177,7 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
> > struct device *dev = bus->dev;
> > struct opp_table *opp_table;
> > const char *vdd = "vdd";
> > - int i, ret, count, size;
> > + int i, ret, count;
> >
> > opp_table = dev_pm_opp_set_regulators(dev, &vdd, 1);
> > if (IS_ERR(opp_table)) {
> > @@ -201,8 +200,7 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
> > }
> > bus->edev_count = count;
> >
> > - size = sizeof(*bus->edev) * count;
> > - bus->edev = devm_kzalloc(dev, size, GFP_KERNEL);
> > + bus->edev = devm_kcalloc(dev, count, sizeof(*bus->edev), GFP_KERNEL);
> > if (!bus->edev) {
> > ret = -ENOMEM;
> > goto err_regulator;
> > @@ -301,10 +299,9 @@ static int exynos_bus_profile_init(struct exynos_bus *bus,
> > profile->exit = exynos_bus_exit;
> >
> > ondemand_data = devm_kzalloc(dev, sizeof(*ondemand_data), GFP_KERNEL);
> > - if (!ondemand_data) {
> > - ret = -ENOMEM;
> > - goto err;
> > - }
> > + if (!ondemand_data)
> > + return -ENOMEM;
> > +
> > ondemand_data->upthreshold = 40;
> > ondemand_data->downdifferential = 5;
> >
> > @@ -314,8 +311,7 @@ static int exynos_bus_profile_init(struct exynos_bus *bus,
> > ondemand_data);
> > if (IS_ERR(bus->devfreq)) {
> > dev_err(dev, "failed to add devfreq device\n");
> > - ret = PTR_ERR(bus->devfreq);
> > - goto err;
> > + return PTR_ERR(bus->devfreq);
> > }
> >
> > /*
> > @@ -325,16 +321,13 @@ static int exynos_bus_profile_init(struct exynos_bus *bus,
> > ret = exynos_bus_enable_edev(bus);
> > if (ret < 0) {
> > dev_err(dev, "failed to enable devfreq-event devices\n");
> > - goto err;
> > + return ret;
> > }
> >
> > ret = exynos_bus_set_event(bus);
> > - if (ret < 0) {
> > + if (ret < 0)
> > dev_err(dev, "failed to set event to devfreq-event devices\n");
> > - goto err;
>
> Instead of removing 'goto err', just return err as I commented[1] on v1.
> [1] https://lkml.org/lkml/2019/7/26/331
>
> > - }
> >
> > -err:
> > return ret;
>
> And you just keep 'return ret' or you can change it as 'return 0'.
OK, I went for:
ret = exynos_bus_set_event(bus);
if (ret < 0) {
dev_err(dev, "failed to set event to devfreq-event devices\n");
return ret;
}
return 0;
> > }
> >
> > @@ -344,7 +337,6 @@ static int exynos_bus_profile_init_passive(struct exynos_bus *bus,
> > struct device *dev = bus->dev;
> > struct devfreq_passive_data *passive_data;
> > struct devfreq *parent_devfreq;
> > - int ret = 0;
> >
> > /* Initialize the struct profile and governor data for passive device */
> > profile->target = exynos_bus_target;
> > @@ -352,30 +344,26 @@ static int exynos_bus_profile_init_passive(struct exynos_bus *bus,
> >
> > /* Get the instance of parent devfreq device */
> > parent_devfreq = devfreq_get_devfreq_by_phandle(dev, 0);
> > - if (IS_ERR(parent_devfreq)) {
> > - ret = -EPROBE_DEFER;
> > - goto err;
> > - }
> > + if (IS_ERR(parent_devfreq))
> > + return -EPROBE_DEFER;
> >
> > passive_data = devm_kzalloc(dev, sizeof(*passive_data), GFP_KERNEL);
> > - if (!passive_data) {
> > - ret = -ENOMEM;
> > - goto err;
> > - }
> > + if (!passive_data)
> > + return -ENOMEM;
> > +
> > passive_data->parent = parent_devfreq;
> >
> > /* Add devfreq device for exynos bus with passive governor */
> > - bus->devfreq = devm_devfreq_add_device(dev, profile, DEVFREQ_GOV_PASSIVE,
> > + bus->devfreq = devm_devfreq_add_device(dev, profile,
> > + DEVFREQ_GOV_PASSIVE,
> > passive_data);
> > if (IS_ERR(bus->devfreq)) {
> > dev_err(dev,
> > "failed to add devfreq dev with passive governor\n");
> > - ret = PTR_ERR(bus->devfreq);
> > - goto err;
> > + return PTR_ERR(bus->devfreq);
> > }
> >
> > -err:
> > - return ret;
> > + return 0;
> > }
> >
> > static int exynos_bus_probe(struct platform_device *pdev)
> > @@ -393,18 +381,18 @@ static int exynos_bus_probe(struct platform_device *pdev)
> > return -EINVAL;
> > }
> >
> > - bus = devm_kzalloc(&pdev->dev, sizeof(*bus), GFP_KERNEL);
> > + bus = devm_kzalloc(dev, sizeof(*bus), GFP_KERNEL);
> > if (!bus)
> > return -ENOMEM;
> > mutex_init(&bus->lock);
> > - bus->dev = &pdev->dev;
> > + bus->dev = dev;
> > platform_set_drvdata(pdev, bus);
> >
> > profile = devm_kzalloc(dev, sizeof(*profile), GFP_KERNEL);
> > if (!profile)
> > return -ENOMEM;
> >
> > - node = of_parse_phandle(dev->of_node, "devfreq", 0);
> > + node = of_parse_phandle(np, "devfreq", 0);
> > if (node) {
> > of_node_put(node);
> > passive = true;
> > @@ -461,12 +449,10 @@ static int exynos_bus_resume(struct device *dev)
> > int ret;
> >
> > ret = exynos_bus_enable_edev(bus);
> > - if (ret < 0) {
> > + if (ret < 0)
> > dev_err(dev, "failed to enable the devfreq-event devices\n");
> > - return ret;
>
> Keep the 'return ret' if error happen as I commented[1] on v1.
> [1] https://lkml.org/lkml/2019/7/26/331
>
> > - }
> >
> > - return 0;
> > + return ret;
>
> And you just keep 'return 0' or you can change it as 'return ret'.
OK, I kept the original code:
ret = exynos_bus_enable_edev(bus);
if (ret < 0) {
dev_err(dev, "failed to enable the devfreq-event devices\n");
return ret;
}
return 0;
> > }
> >
> > static int exynos_bus_suspend(struct device *dev)
> > @@ -475,12 +461,10 @@ static int exynos_bus_suspend(struct device *dev)
> > int ret;
> >
> > ret = exynos_bus_disable_edev(bus);
> > - if (ret < 0) {
> > + if (ret < 0)
> > dev_err(dev, "failed to disable the devfreq-event devices\n");
> > - return ret;
>
> Keep the 'return ret' if error happen as I commented[1] on v1.
> [1] https://lkml.org/lkml/2019/7/26/331
OK, I kept the original code.
> > - }
> >
> > - return 0;
> > + return ret;
>
> And you just keep 'return 0' or you can change it as 'return ret'.
>
> > }
> > #endif
> >
> >
>
>
Best regards,
--
Artur Świgoń
Samsung R&D Institute Poland
Samsung Electronics