LinuxLists.cc - [RFC PATCH 00/11] Simple QoS for exynos-bus driver using interconnect

[permalink] [raw]

Subject: [RFC PATCH 10/11] arm: dts: exynos: Add interconnects to Exynos4412 mixer

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ń <[email protected]>
---
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 a70a671acacd..faaec6c40412 100644
--- a/arch/arm/boot/dts/exynos4412.dtsi
+++ b/arch/arm/boot/dts/exynos4412.dtsi
@@ -789,6 +789,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

2019-07-23 23:09:02

[permalink] [raw]

Subject: [RFC PATCH 07/11] icc: Relax condition in apply_constraints()

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 2556fd6d1863..bb55565d190c 100644
--- a/drivers/interconnect/core.c
+++ b/drivers/interconnect/core.c
@@ -219,11 +219,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

2019-07-23 23:09:06

[permalink] [raw]

Subject: [RFC PATCH 08/11] arm: dts: exynos: Add parents and #interconnect-cells to Exynos4412

This patch adds two fields tp the Exynos4412 DTS:
- parent: to declare connections between nodes that are not in a
parent-child relation in devfreq;
- #interconnect-cells: required by the interconnect framework.

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

Signed-off-by: Artur Świgoń <[email protected]>
---
arch/arm/boot/dts/exynos4412-odroid-common.dtsi | 1 +
arch/arm/boot/dts/exynos4412.dtsi | 9 +++++++++
2 files changed, 10 insertions(+)

diff --git a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
index ea55f377d17c..bdd61ae86103 100644
--- a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
+++ b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
@@ -106,6 +106,7 @@
&bus_leftbus {
devfreq-events = <&ppmu_leftbus_3>, <&ppmu_rightbus_3>;
vdd-supply = <&buck3_reg>;
+ parent = <&bus_dmc>;
status = "okay";
};

diff --git a/arch/arm/boot/dts/exynos4412.dtsi b/arch/arm/boot/dts/exynos4412.dtsi
index d20db2dfe8e2..a70a671acacd 100644
--- a/arch/arm/boot/dts/exynos4412.dtsi
+++ b/arch/arm/boot/dts/exynos4412.dtsi
@@ -390,6 +390,7 @@
clocks = <&clock CLK_DIV_DMC>;
clock-names = "bus";
operating-points-v2 = <&bus_dmc_opp_table>;
+ #interconnect-cells = <0>;
status = "disabled";
};

@@ -398,6 +399,7 @@
clocks = <&clock CLK_DIV_ACP>;
clock-names = "bus";
operating-points-v2 = <&bus_acp_opp_table>;
+ #interconnect-cells = <0>;
status = "disabled";
};

@@ -406,6 +408,7 @@
clocks = <&clock CLK_DIV_C2C>;
clock-names = "bus";
operating-points-v2 = <&bus_dmc_opp_table>;
+ #interconnect-cells = <0>;
status = "disabled";
};

@@ -459,6 +462,7 @@
clocks = <&clock CLK_DIV_GDL>;
clock-names = "bus";
operating-points-v2 = <&bus_leftbus_opp_table>;
+ #interconnect-cells = <0>;
status = "disabled";
};

@@ -467,6 +471,7 @@
clocks = <&clock CLK_DIV_GDR>;
clock-names = "bus";
operating-points-v2 = <&bus_leftbus_opp_table>;
+ #interconnect-cells = <0>;
status = "disabled";
};

@@ -475,6 +480,7 @@
clocks = <&clock CLK_ACLK160>;
clock-names = "bus";
operating-points-v2 = <&bus_display_opp_table>;
+ #interconnect-cells = <0>;
status = "disabled";
};

@@ -483,6 +489,7 @@
clocks = <&clock CLK_ACLK133>;
clock-names = "bus";
operating-points-v2 = <&bus_fsys_opp_table>;
+ #interconnect-cells = <0>;
status = "disabled";
};

@@ -491,6 +498,7 @@
clocks = <&clock CLK_ACLK100>;
clock-names = "bus";
operating-points-v2 = <&bus_peri_opp_table>;
+ #interconnect-cells = <0>;
status = "disabled";
};

@@ -499,6 +507,7 @@
clocks = <&clock CLK_SCLK_MFC>;
clock-names = "bus";
operating-points-v2 = <&bus_leftbus_opp_table>;
+ #interconnect-cells = <0>;
status = "disabled";
};

--
2.17.1

2019-07-23 23:09:07

[permalink] [raw]

Subject: [RFC PATCH 04/11] devfreq: exynos-bus: Clean up code

This patch adds minor improvements to the exynos-bus driver.

Signed-off-by: Artur Świgoń <[email protected]>
---
drivers/devfreq/exynos-bus.c | 49 ++++++++++++++++--------------------
1 file changed, 22 insertions(+), 27 deletions(-)

diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
index 4bb83b945bf7..412511ca7703 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
#define DEFAULT_VOLTAGE_TOLERANCE 2
@@ -256,7 +255,7 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
struct exynos_bus *bus)
{
struct device *dev = bus->dev;
- int i, ret, count, size;
+ int i, ret, count;

/* Get the regulator to provide each bus with the power */
bus->regulator = devm_regulator_get(dev, "vdd");
@@ -283,8 +282,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;
@@ -388,7 +386,7 @@ static int exynos_bus_profile_init(struct exynos_bus *bus,
ondemand_data = devm_kzalloc(dev, sizeof(*ondemand_data), GFP_KERNEL);
if (!ondemand_data) {
ret = -ENOMEM;
- goto err;
+ goto out;
}
ondemand_data->upthreshold = 40;
ondemand_data->downdifferential = 5;
@@ -400,14 +398,14 @@ static int exynos_bus_profile_init(struct exynos_bus *bus,
if (IS_ERR(bus->devfreq)) {
dev_err(dev, "failed to add devfreq device\n");
ret = PTR_ERR(bus->devfreq);
- goto err;
+ goto out;
}

/* Register opp_notifier to catch the change of OPP */
ret = devm_devfreq_register_opp_notifier(dev, bus->devfreq);
if (ret < 0) {
dev_err(dev, "failed to register opp notifier\n");
- goto err;
+ goto out;
}

/*
@@ -417,16 +415,16 @@ 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;
+ goto out;
}

ret = exynos_bus_set_event(bus);
if (ret < 0) {
dev_err(dev, "failed to set event to devfreq-event devices\n");
- goto err;
+ goto out;
}

-err:
+out:
return ret;
}

@@ -446,27 +444,28 @@ static int exynos_bus_profile_init_passive(struct exynos_bus *bus,
parent_devfreq = devfreq_get_devfreq_by_phandle(dev, 0);
if (IS_ERR(parent_devfreq)) {
ret = -EPROBE_DEFER;
- goto err;
+ goto out;
}

passive_data = devm_kzalloc(dev, sizeof(*passive_data), GFP_KERNEL);
if (!passive_data) {
ret = -ENOMEM;
- goto err;
+ goto out;
}
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;
+ goto out;
}

-err:
+out:
return ret;
}

@@ -484,11 +483,11 @@ 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);

/* Parse the device-tree to get the resource information */
@@ -502,7 +501,7 @@ static int exynos_bus_probe(struct platform_device *pdev)
goto err;
}

- node = of_parse_phandle(dev->of_node, "devfreq", 0);
+ node = of_parse_phandle(np, "devfreq", 0);
if (node) {
of_node_put(node);
ret = exynos_bus_profile_init_passive(bus, profile);
@@ -547,12 +546,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)
@@ -561,12 +558,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

2019-07-24 19:27:44

[permalink] [raw]

Subject: Re: [RFC PATCH 04/11] devfreq: exynos-bus: Clean up code

On Tue, Jul 23, 2019 at 02:20:09PM +0200, Artur Świgoń wrote:
> This patch adds minor improvements to the exynos-bus driver.
>
> Signed-off-by: Artur Świgoń <[email protected]>
> ---
> drivers/devfreq/exynos-bus.c | 49 ++++++++++++++++--------------------
> 1 file changed, 22 insertions(+), 27 deletions(-)

Reviewed-by: Krzysztof Kozlowski <[email protected]>

Best regards,
Krzysztof

2019-07-24 19:27:58

[permalink] [raw]

Subject: Re: [RFC PATCH 00/11] Simple QoS for exynos-bus driver using interconnect

On Tue, Jul 23, 2019 at 02:20:05PM +0200, 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:

Nice work! Good to see proper solution :)

Best regards,
Krzysztof

2019-07-24 20:38:17

[permalink] [raw]

Subject: Re: [RFC PATCH 08/11] arm: dts: exynos: Add parents and #interconnect-cells to Exynos4412

On Tue, Jul 23, 2019 at 02:20:13PM +0200, Artur Świgoń wrote:
> This patch adds two fields tp the Exynos4412 DTS:

tp->to

> - parent: to declare connections between nodes that are not in a
> parent-child relation in devfreq;

Is it a standard property?
The explanation needs some improvement... why are you adding parent to a
devices which are not child-parent?

Best regards,
Krzysztof

> - #interconnect-cells: required by the interconnect framework.
>
> Please note that #interconnect-cells is always zero and node IDs are not
> hardcoded anywhere.
>
> Signed-off-by: Artur Świgoń <[email protected]>
> ---
> arch/arm/boot/dts/exynos4412-odroid-common.dtsi | 1 +
> arch/arm/boot/dts/exynos4412.dtsi | 9 +++++++++
> 2 files changed, 10 insertions(+)
>
> diff --git a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> index ea55f377d17c..bdd61ae86103 100644
> --- a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> +++ b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> @@ -106,6 +106,7 @@
> &bus_leftbus {
> devfreq-events = <&ppmu_leftbus_3>, <&ppmu_rightbus_3>;
> vdd-supply = <&buck3_reg>;
> + parent = <&bus_dmc>;
> status = "okay";
> };
>
> diff --git a/arch/arm/boot/dts/exynos4412.dtsi b/arch/arm/boot/dts/exynos4412.dtsi
> index d20db2dfe8e2..a70a671acacd 100644
> --- a/arch/arm/boot/dts/exynos4412.dtsi
> +++ b/arch/arm/boot/dts/exynos4412.dtsi
> @@ -390,6 +390,7 @@
> clocks = <&clock CLK_DIV_DMC>;
> clock-names = "bus";
> operating-points-v2 = <&bus_dmc_opp_table>;
> + #interconnect-cells = <0>;
> status = "disabled";
> };
>
> @@ -398,6 +399,7 @@
> clocks = <&clock CLK_DIV_ACP>;
> clock-names = "bus";
> operating-points-v2 = <&bus_acp_opp_table>;
> + #interconnect-cells = <0>;
> status = "disabled";
> };
>
> @@ -406,6 +408,7 @@
> clocks = <&clock CLK_DIV_C2C>;
> clock-names = "bus";
> operating-points-v2 = <&bus_dmc_opp_table>;
> + #interconnect-cells = <0>;
> status = "disabled";
> };
>
> @@ -459,6 +462,7 @@
> clocks = <&clock CLK_DIV_GDL>;
> clock-names = "bus";
> operating-points-v2 = <&bus_leftbus_opp_table>;
> + #interconnect-cells = <0>;
> status = "disabled";
> };
>
> @@ -467,6 +471,7 @@
> clocks = <&clock CLK_DIV_GDR>;
> clock-names = "bus";
> operating-points-v2 = <&bus_leftbus_opp_table>;
> + #interconnect-cells = <0>;
> status = "disabled";
> };
>
> @@ -475,6 +480,7 @@
> clocks = <&clock CLK_ACLK160>;
> clock-names = "bus";
> operating-points-v2 = <&bus_display_opp_table>;
> + #interconnect-cells = <0>;
> status = "disabled";
> };
>
> @@ -483,6 +489,7 @@
> clocks = <&clock CLK_ACLK133>;
> clock-names = "bus";
> operating-points-v2 = <&bus_fsys_opp_table>;
> + #interconnect-cells = <0>;
> status = "disabled";
> };
>
> @@ -491,6 +498,7 @@
> clocks = <&clock CLK_ACLK100>;
> clock-names = "bus";
> operating-points-v2 = <&bus_peri_opp_table>;
> + #interconnect-cells = <0>;
> status = "disabled";
> };
>
> @@ -499,6 +507,7 @@
> clocks = <&clock CLK_SCLK_MFC>;
> clock-names = "bus";
> operating-points-v2 = <&bus_leftbus_opp_table>;
> + #interconnect-cells = <0>;
> status = "disabled";
> };
>
> --
> 2.17.1
>

2019-07-25 14:04:11

[permalink] [raw]

Subject: Re: [RFC PATCH 04/11] devfreq: exynos-bus: Clean up code

2019년 7월 24일 (수) 오전 8:07, Artur Świgoń <[email protected]>님이 작성:
>
> This patch adds minor improvements to the exynos-bus driver.
>
> Signed-off-by: Artur Świgoń <[email protected]>
> ---
> drivers/devfreq/exynos-bus.c | 49 ++++++++++++++++--------------------
> 1 file changed, 22 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
> index 4bb83b945bf7..412511ca7703 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
> #define DEFAULT_VOLTAGE_TOLERANCE 2
> @@ -256,7 +255,7 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
> struct exynos_bus *bus)
> {
> struct device *dev = bus->dev;
> - int i, ret, count, size;
> + int i, ret, count;
>
> /* Get the regulator to provide each bus with the power */
> bus->regulator = devm_regulator_get(dev, "vdd");
> @@ -283,8 +282,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;
> @@ -388,7 +386,7 @@ static int exynos_bus_profile_init(struct exynos_bus *bus,
> ondemand_data = devm_kzalloc(dev, sizeof(*ondemand_data), GFP_KERNEL);
> if (!ondemand_data) {
> ret = -ENOMEM;
> - goto err;
> + goto out;
> }
> ondemand_data->upthreshold = 40;
> ondemand_data->downdifferential = 5;
> @@ -400,14 +398,14 @@ static int exynos_bus_profile_init(struct exynos_bus *bus,
> if (IS_ERR(bus->devfreq)) {
> dev_err(dev, "failed to add devfreq device\n");
> ret = PTR_ERR(bus->devfreq);
> - goto err;
> + goto out;
> }
>
> /* Register opp_notifier to catch the change of OPP */
> ret = devm_devfreq_register_opp_notifier(dev, bus->devfreq);
> if (ret < 0) {
> dev_err(dev, "failed to register opp notifier\n");
> - goto err;
> + goto out;
> }
>
> /*
> @@ -417,16 +415,16 @@ 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;
> + goto out;
> }
>
> ret = exynos_bus_set_event(bus);
> if (ret < 0) {
> dev_err(dev, "failed to set event to devfreq-event devices\n");
> - goto err;
> + goto out;
> }
>
> -err:
> +out:
> return ret;
> }
>
> @@ -446,27 +444,28 @@ static int exynos_bus_profile_init_passive(struct exynos_bus *bus,
> parent_devfreq = devfreq_get_devfreq_by_phandle(dev, 0);
> if (IS_ERR(parent_devfreq)) {
> ret = -EPROBE_DEFER;
> - goto err;
> + goto out;
> }
>
> passive_data = devm_kzalloc(dev, sizeof(*passive_data), GFP_KERNEL);
> if (!passive_data) {
> ret = -ENOMEM;
> - goto err;
> + goto out;
> }
> 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;
> + goto out;
> }
>
> -err:
> +out:
> return ret;
> }
>
> @@ -484,11 +483,11 @@ 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);
>
> /* Parse the device-tree to get the resource information */
> @@ -502,7 +501,7 @@ static int exynos_bus_probe(struct platform_device *pdev)
> goto err;
> }
>
> - node = of_parse_phandle(dev->of_node, "devfreq", 0);
> + node = of_parse_phandle(np, "devfreq", 0);
> if (node) {
> of_node_put(node);
> ret = exynos_bus_profile_init_passive(bus, profile);
> @@ -547,12 +546,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)
> @@ -561,12 +558,10 @@ static int exynos_bus_suspend(struct device *dev)
> int ret;
>

NACK.
> 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
>

NACK.

As I already commented, It has never any benefit.
I think that it is not usful. Please drop it.

--
Best Regards,
Chanwoo Choi

2019-07-25 17:50:33

[permalink] [raw]

Subject: Re: [RFC PATCH 08/11] arm: dts: exynos: Add parents and #interconnect-cells to Exynos4412

2019년 7월 24일 (수) 오전 8:07, Artur Świgoń <[email protected]>님이 작성:
>
> This patch adds two fields tp the Exynos4412 DTS:
> - parent: to declare connections between nodes that are not in a
> parent-child relation in devfreq;
> - #interconnect-cells: required by the interconnect framework.
>
> Please note that #interconnect-cells is always zero and node IDs are not
> hardcoded anywhere.
>
> Signed-off-by: Artur Świgoń <[email protected]>
> ---
> arch/arm/boot/dts/exynos4412-odroid-common.dtsi | 1 +
> arch/arm/boot/dts/exynos4412.dtsi | 9 +++++++++
> 2 files changed, 10 insertions(+)
>
> diff --git a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> index ea55f377d17c..bdd61ae86103 100644
> --- a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> +++ b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> @@ -106,6 +106,7 @@
> &bus_leftbus {
> devfreq-events = <&ppmu_leftbus_3>, <&ppmu_rightbus_3>;
> vdd-supply = <&buck3_reg>;
> + parent = <&bus_dmc>;

It is wrong. 'bus_leftbus' has not any h/w dependency of 'bus_dmc'
and 'bus_leftbus' is not child of 'bus_dmc'.

Even it there are some PMQoS requirement between them,
it it not proper to tie both 'bus_leftbus' and 'bus_dmc'.

> status = "okay";
> };
>
> diff --git a/arch/arm/boot/dts/exynos4412.dtsi b/arch/arm/boot/dts/exynos4412.dtsi
> index d20db2dfe8e2..a70a671acacd 100644
> --- a/arch/arm/boot/dts/exynos4412.dtsi
> +++ b/arch/arm/boot/dts/exynos4412.dtsi
> @@ -390,6 +390,7 @@
> clocks = <&clock CLK_DIV_DMC>;
> clock-names = "bus";
> operating-points-v2 = <&bus_dmc_opp_table>;
> + #interconnect-cells = <0>;
> status = "disabled";
> };
>
> @@ -398,6 +399,7 @@
> clocks = <&clock CLK_DIV_ACP>;
> clock-names = "bus";
> operating-points-v2 = <&bus_acp_opp_table>;
> + #interconnect-cells = <0>;
> status = "disabled";
> };
>
> @@ -406,6 +408,7 @@
> clocks = <&clock CLK_DIV_C2C>;
> clock-names = "bus";
> operating-points-v2 = <&bus_dmc_opp_table>;
> + #interconnect-cells = <0>;
> status = "disabled";
> };
>
> @@ -459,6 +462,7 @@
> clocks = <&clock CLK_DIV_GDL>;
> clock-names = "bus";
> operating-points-v2 = <&bus_leftbus_opp_table>;
> + #interconnect-cells = <0>;
> status = "disabled";
> };
>
> @@ -467,6 +471,7 @@
> clocks = <&clock CLK_DIV_GDR>;
> clock-names = "bus";
> operating-points-v2 = <&bus_leftbus_opp_table>;
> + #interconnect-cells = <0>;
> status = "disabled";
> };
>
> @@ -475,6 +480,7 @@
> clocks = <&clock CLK_ACLK160>;
> clock-names = "bus";
> operating-points-v2 = <&bus_display_opp_table>;
> + #interconnect-cells = <0>;
> status = "disabled";
> };
>
> @@ -483,6 +489,7 @@
> clocks = <&clock CLK_ACLK133>;
> clock-names = "bus";
> operating-points-v2 = <&bus_fsys_opp_table>;
> + #interconnect-cells = <0>;
> status = "disabled";
> };
>
> @@ -491,6 +498,7 @@
> clocks = <&clock CLK_ACLK100>;
> clock-names = "bus";
> operating-points-v2 = <&bus_peri_opp_table>;
> + #interconnect-cells = <0>;
> status = "disabled";
> };
>
> @@ -499,6 +507,7 @@
> clocks = <&clock CLK_SCLK_MFC>;
> clock-names = "bus";
> operating-points-v2 = <&bus_leftbus_opp_table>;
> + #interconnect-cells = <0>;
> status = "disabled";
> };
>
> --
> 2.17.1
>

--
Best Regards,
Chanwoo Choi

2019-07-26 10:48:40

[permalink] [raw]

Subject: Re: [RFC PATCH 04/11] devfreq: exynos-bus: Clean up code

On Thu, 25 Jul 2019 at 14:51, Chanwoo Choi <[email protected]> wrote:
>
> 2019년 7월 24일 (수) 오전 8:07, Artur Świgoń <[email protected]>님이 작성:
> >
> > This patch adds minor improvements to the exynos-bus driver.
> >
> > Signed-off-by: Artur Świgoń <[email protected]>
> > ---
> > drivers/devfreq/exynos-bus.c | 49 ++++++++++++++++--------------------
> > 1 file changed, 22 insertions(+), 27 deletions(-)
> >
> > diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
> > index 4bb83b945bf7..412511ca7703 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
> > #define DEFAULT_VOLTAGE_TOLERANCE 2
> > @@ -256,7 +255,7 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
> > struct exynos_bus *bus)
> > {
> > struct device *dev = bus->dev;
> > - int i, ret, count, size;
> > + int i, ret, count;
> >
> > /* Get the regulator to provide each bus with the power */
> > bus->regulator = devm_regulator_get(dev, "vdd");
> > @@ -283,8 +282,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;
> > @@ -388,7 +386,7 @@ static int exynos_bus_profile_init(struct exynos_bus *bus,
> > ondemand_data = devm_kzalloc(dev, sizeof(*ondemand_data), GFP_KERNEL);
> > if (!ondemand_data) {
> > ret = -ENOMEM;
> > - goto err;
> > + goto out;
> > }
> > ondemand_data->upthreshold = 40;
> > ondemand_data->downdifferential = 5;
> > @@ -400,14 +398,14 @@ static int exynos_bus_profile_init(struct exynos_bus *bus,
> > if (IS_ERR(bus->devfreq)) {
> > dev_err(dev, "failed to add devfreq device\n");
> > ret = PTR_ERR(bus->devfreq);
> > - goto err;
> > + goto out;
> > }
> >
> > /* Register opp_notifier to catch the change of OPP */
> > ret = devm_devfreq_register_opp_notifier(dev, bus->devfreq);
> > if (ret < 0) {
> > dev_err(dev, "failed to register opp notifier\n");
> > - goto err;
> > + goto out;
> > }
> >
> > /*
> > @@ -417,16 +415,16 @@ 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;
> > + goto out;
> > }
> >
> > ret = exynos_bus_set_event(bus);
> > if (ret < 0) {
> > dev_err(dev, "failed to set event to devfreq-event devices\n");
> > - goto err;
> > + goto out;
> > }
> >
> > -err:
> > +out:
> > return ret;
> > }
> >
> > @@ -446,27 +444,28 @@ static int exynos_bus_profile_init_passive(struct exynos_bus *bus,
> > parent_devfreq = devfreq_get_devfreq_by_phandle(dev, 0);
> > if (IS_ERR(parent_devfreq)) {
> > ret = -EPROBE_DEFER;
> > - goto err;
> > + goto out;
> > }
> >
> > passive_data = devm_kzalloc(dev, sizeof(*passive_data), GFP_KERNEL);
> > if (!passive_data) {
> > ret = -ENOMEM;
> > - goto err;
> > + goto out;
> > }
> > 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;
> > + goto out;
> > }
> >
> > -err:
> > +out:
> > return ret;
> > }
> >
> > @@ -484,11 +483,11 @@ 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);
> >
> > /* Parse the device-tree to get the resource information */
> > @@ -502,7 +501,7 @@ static int exynos_bus_probe(struct platform_device *pdev)
> > goto err;
> > }
> >
> > - node = of_parse_phandle(dev->of_node, "devfreq", 0);
> > + node = of_parse_phandle(np, "devfreq", 0);
> > if (node) {
> > of_node_put(node);
> > ret = exynos_bus_profile_init_passive(bus, profile);
> > @@ -547,12 +546,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)
> > @@ -561,12 +558,10 @@ static int exynos_bus_suspend(struct device *dev)
> > int ret;
> >
>
> NACK.

Instead of simple nack you should explain what is wrong with proposed
approach. The existing code could be improved and this patch clearly
improves the readability. "err" label is confusing if it is used for
correct exit path. The last "if() return" block is subjective - but
then explain exactly why you prefer one over another. No just "nack"
for peoples contributions.

> > 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
> >
>
> NACK.
>
> As I already commented, It has never any benefit.
> I think that it is not usful. Please drop it.

The benefit for me is clear - makes the code easier to understand.

Best regards,
Krzysztof

2019-07-26 11:08:43

[permalink] [raw]

Subject: Re: [RFC PATCH 04/11] devfreq: exynos-bus: Clean up code

On 19. 7. 26. 오후 7:45, Krzysztof Kozlowski wrote:
> On Thu, 25 Jul 2019 at 14:51, Chanwoo Choi <[email protected]> wrote:
>>
>> 2019년 7월 24일 (수) 오전 8:07, Artur Świgoń <[email protected]>님이 작성:
>>>
>>> This patch adds minor improvements to the exynos-bus driver.
>>>
>>> Signed-off-by: Artur Świgoń <[email protected]>
>>> ---
>>> drivers/devfreq/exynos-bus.c | 49 ++++++++++++++++--------------------
>>> 1 file changed, 22 insertions(+), 27 deletions(-)
>>>
>>> diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
>>> index 4bb83b945bf7..412511ca7703 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
>>> #define DEFAULT_VOLTAGE_TOLERANCE 2
>>> @@ -256,7 +255,7 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
>>> struct exynos_bus *bus)
>>> {
>>> struct device *dev = bus->dev;
>>> - int i, ret, count, size;
>>> + int i, ret, count;
>>>
>>> /* Get the regulator to provide each bus with the power */
>>> bus->regulator = devm_regulator_get(dev, "vdd");
>>> @@ -283,8 +282,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;
>>> @@ -388,7 +386,7 @@ static int exynos_bus_profile_init(struct exynos_bus *bus,
>>> ondemand_data = devm_kzalloc(dev, sizeof(*ondemand_data), GFP_KERNEL);
>>> if (!ondemand_data) {
>>> ret = -ENOMEM;
>>> - goto err;
>>> + goto out;
>>> }
>>> ondemand_data->upthreshold = 40;
>>> ondemand_data->downdifferential = 5;
>>> @@ -400,14 +398,14 @@ static int exynos_bus_profile_init(struct exynos_bus *bus,
>>> if (IS_ERR(bus->devfreq)) {
>>> dev_err(dev, "failed to add devfreq device\n");
>>> ret = PTR_ERR(bus->devfreq);
>>> - goto err;
>>> + goto out;
>>> }
>>>
>>> /* Register opp_notifier to catch the change of OPP */
>>> ret = devm_devfreq_register_opp_notifier(dev, bus->devfreq);
>>> if (ret < 0) {
>>> dev_err(dev, "failed to register opp notifier\n");
>>> - goto err;
>>> + goto out;
>>> }
>>>
>>> /*
>>> @@ -417,16 +415,16 @@ 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;
>>> + goto out;
>>> }
>>>
>>> ret = exynos_bus_set_event(bus);
>>> if (ret < 0) {
>>> dev_err(dev, "failed to set event to devfreq-event devices\n");
>>> - goto err;
>>> + goto out;
>>> }
>>>
>>> -err:
>>> +out:
>>> return ret;
>>> }
>>>
>>> @@ -446,27 +444,28 @@ static int exynos_bus_profile_init_passive(struct exynos_bus *bus,
>>> parent_devfreq = devfreq_get_devfreq_by_phandle(dev, 0);
>>> if (IS_ERR(parent_devfreq)) {
>>> ret = -EPROBE_DEFER;
>>> - goto err;
>>> + goto out;
>>> }
>>>
>>> passive_data = devm_kzalloc(dev, sizeof(*passive_data), GFP_KERNEL);
>>> if (!passive_data) {
>>> ret = -ENOMEM;
>>> - goto err;
>>> + goto out;
>>> }
>>> 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;
>>> + goto out;
>>> }
>>>
>>> -err:
>>> +out:
>>> return ret;
>>> }
>>>
>>> @@ -484,11 +483,11 @@ 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);
>>>
>>> /* Parse the device-tree to get the resource information */
>>> @@ -502,7 +501,7 @@ static int exynos_bus_probe(struct platform_device *pdev)
>>> goto err;
>>> }
>>>
>>> - node = of_parse_phandle(dev->of_node, "devfreq", 0);
>>> + node = of_parse_phandle(np, "devfreq", 0);
>>> if (node) {
>>> of_node_put(node);
>>> ret = exynos_bus_profile_init_passive(bus, profile);
>>> @@ -547,12 +546,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)
>>> @@ -561,12 +558,10 @@ static int exynos_bus_suspend(struct device *dev)
>>> int ret;
>>>
>>
>> NACK.
>
> Instead of simple nack you should explain what is wrong with proposed
> approach. The existing code could be improved and this patch clearly
> improves the readability. "err" label is confusing if it is used for
> correct exit path. The last "if() return" block is subjective - but
> then explain exactly why you prefer one over another. No just "nack"
> for peoples contributions.

OK. Sorry for my lack comment.

Actually, I prefer 'err' instead of 'out' because jump to 'err'
statement point when failed to call some functions. In my case,
'err' is proper without any problem.

>
>>> 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;
>>> - }

When happen error by function call, I think that have to print
the error log, just return or jump some point like 'err'
for restoring the previous state.

But, in this case, even if happen the error, the exception handling
of exynos_bus_disable_edev() just prints the error log without return.

I knew that this it is possible because the exynos_bus_disable_edev(bus)
was called at the end of function. But, I want to keep the same style
for exception handling if there are no any critical benefits.

>>>
>>> - return 0;
>>> + return ret;
>>> }
>>> #endif
>>>
>>> --
>>> 2.17.1
>>>
>>
>> NACK.
>>
>> As I already commented, It has never any benefit.
>> I think that it is not usful. Please drop it.
>
> The benefit for me is clear - makes the code easier to understand.
>
> Best regards,
> Krzysztof
>
>

--
Best Regards,
Chanwoo Choi
Samsung Electronics

2019-07-26 12:49:15

by Marek Szyprowski

[permalink] [raw]

Subject: Re: [RFC PATCH 08/11] arm: dts: exynos: Add parents and #interconnect-cells to Exynos4412

Hi

On 2019-07-25 15:13, Chanwoo Choi wrote:
> 2019년 7월 24일 (수) 오전 8:07, Artur Świgoń <[email protected]>님이 작성:
>> This patch adds two fields tp the Exynos4412 DTS:
>> - parent: to declare connections between nodes that are not in a
>> parent-child relation in devfreq;
>> - #interconnect-cells: required by the interconnect framework.
>>
>> Please note that #interconnect-cells is always zero and node IDs are not
>> hardcoded anywhere.
>>
>> Signed-off-by: Artur Świgoń <[email protected]>
>> ---
>> arch/arm/boot/dts/exynos4412-odroid-common.dtsi | 1 +
>> arch/arm/boot/dts/exynos4412.dtsi | 9 +++++++++
>> 2 files changed, 10 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
>> index ea55f377d17c..bdd61ae86103 100644
>> --- a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
>> +++ b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
>> @@ -106,6 +106,7 @@
>> &bus_leftbus {
>> devfreq-events = <&ppmu_leftbus_3>, <&ppmu_rightbus_3>;
>> vdd-supply = <&buck3_reg>;
>> + parent = <&bus_dmc>;
> It is wrong. 'bus_leftbus' has not any h/w dependency of 'bus_dmc'
> and 'bus_leftbus' is not child of 'bus_dmc'.
>
> Even it there are some PMQoS requirement between them,
> it it not proper to tie both 'bus_leftbus' and 'bus_dmc'.

There is strict dependency between them. DMC bus running at frequency
lower than left (or right) bus really doesn't make much sense, because
it will limit the left bus performance. This dependency should be
modeled somehow.

>> status = "okay";
>> };
>>
>> diff --git a/arch/arm/boot/dts/exynos4412.dtsi b/arch/arm/boot/dts/exynos4412.dtsi
>> index d20db2dfe8e2..a70a671acacd 100644
>> --- a/arch/arm/boot/dts/exynos4412.dtsi
>> +++ b/arch/arm/boot/dts/exynos4412.dtsi
>> @@ -390,6 +390,7 @@
>> clocks = <&clock CLK_DIV_DMC>;
>> clock-names = "bus";
>> operating-points-v2 = <&bus_dmc_opp_table>;
>> + #interconnect-cells = <0>;
>> status = "disabled";
>> };
>>
>> @@ -398,6 +399,7 @@
>> clocks = <&clock CLK_DIV_ACP>;
>> clock-names = "bus";
>> operating-points-v2 = <&bus_acp_opp_table>;
>> + #interconnect-cells = <0>;
>> status = "disabled";
>> };
>>
>> @@ -406,6 +408,7 @@
>> clocks = <&clock CLK_DIV_C2C>;
>> clock-names = "bus";
>> operating-points-v2 = <&bus_dmc_opp_table>;
>> + #interconnect-cells = <0>;
>> status = "disabled";
>> };
>>
>> @@ -459,6 +462,7 @@
>> clocks = <&clock CLK_DIV_GDL>;
>> clock-names = "bus";
>> operating-points-v2 = <&bus_leftbus_opp_table>;
>> + #interconnect-cells = <0>;
>> status = "disabled";
>> };
>>
>> @@ -467,6 +471,7 @@
>> clocks = <&clock CLK_DIV_GDR>;
>> clock-names = "bus";
>> operating-points-v2 = <&bus_leftbus_opp_table>;
>> + #interconnect-cells = <0>;
>> status = "disabled";
>> };
>>
>> @@ -475,6 +480,7 @@
>> clocks = <&clock CLK_ACLK160>;
>> clock-names = "bus";
>> operating-points-v2 = <&bus_display_opp_table>;
>> + #interconnect-cells = <0>;
>> status = "disabled";
>> };
>>
>> @@ -483,6 +489,7 @@
>> clocks = <&clock CLK_ACLK133>;
>> clock-names = "bus";
>> operating-points-v2 = <&bus_fsys_opp_table>;
>> + #interconnect-cells = <0>;
>> status = "disabled";
>> };
>>
>> @@ -491,6 +498,7 @@
>> clocks = <&clock CLK_ACLK100>;
>> clock-names = "bus";
>> operating-points-v2 = <&bus_peri_opp_table>;
>> + #interconnect-cells = <0>;
>> status = "disabled";
>> };
>>
>> @@ -499,6 +507,7 @@
>> clocks = <&clock CLK_SCLK_MFC>;
>> clock-names = "bus";
>> operating-points-v2 = <&bus_leftbus_opp_table>;
>> + #interconnect-cells = <0>;
>> status = "disabled";
>> };
>>
>> --
>> 2.17.1
>>
>
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland

2019-07-29 01:20:50

[permalink] [raw]

Subject: Re: [RFC PATCH 08/11] arm: dts: exynos: Add parents and #interconnect-cells to Exynos4412

Hi,

On 19. 7. 26. 오후 9:02, Marek Szyprowski wrote:
> Hi
>
> On 2019-07-25 15:13, Chanwoo Choi wrote:
>> 2019년 7월 24일 (수) 오전 8:07, Artur Świgoń <[email protected]>님이 작성:
>>> This patch adds two fields tp the Exynos4412 DTS:
>>> - parent: to declare connections between nodes that are not in a
>>> parent-child relation in devfreq;
>>> - #interconnect-cells: required by the interconnect framework.
>>>
>>> Please note that #interconnect-cells is always zero and node IDs are not
>>> hardcoded anywhere.
>>>
>>> Signed-off-by: Artur Świgoń <[email protected]>
>>> ---
>>> arch/arm/boot/dts/exynos4412-odroid-common.dtsi | 1 +
>>> arch/arm/boot/dts/exynos4412.dtsi | 9 +++++++++
>>> 2 files changed, 10 insertions(+)
>>>
>>> diff --git a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
>>> index ea55f377d17c..bdd61ae86103 100644
>>> --- a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
>>> +++ b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
>>> @@ -106,6 +106,7 @@
>>> &bus_leftbus {
>>> devfreq-events = <&ppmu_leftbus_3>, <&ppmu_rightbus_3>;
>>> vdd-supply = <&buck3_reg>;
>>> + parent = <&bus_dmc>;
>> It is wrong. 'bus_leftbus' has not any h/w dependency of 'bus_dmc'
>> and 'bus_leftbus' is not child of 'bus_dmc'.
>>
>> Even it there are some PMQoS requirement between them,
>> it it not proper to tie both 'bus_leftbus' and 'bus_dmc'.
>
> There is strict dependency between them. DMC bus running at frequency
> lower than left (or right) bus really doesn't make much sense, because
> it will limit the left bus performance. This dependency should be
> modeled somehow.

I misunderstood new 'parent' prototype as the existing 'devfreq' property.
I didn't understand why use the 'devfreq' property because 'bus_dmc' and
'bus_leftbus' don't share the power line. Please ignore my previous comment.

Basically, I agree that it is necessary to support the QoS requirement
between buses or if possible, between bus and gpu.

To support the interconnect between bus_dmc, bus_leftbus and bus_display,
it used the either 'devfreq' or 'parent' properties to connect them.

In order to catch the meaning of 'devfreq' and 'parent' properties,
If possible, it would be separate the usage role of between 'devfreq'
or 'parent' properties. Because it is possible to connect the 'buses'
with only using 'parent' property if all buses in the path uses
the devfreq governors except for 'passive' governor.

- If 'devfreq' property is used between buses,
it indicates that there are requirement of shading of power line.
- If 'parent' property is used between buses,
it indicates that there are requirement of interconnect connection.

>
>>> status = "okay";
>>> };
>>>
>>> diff --git a/arch/arm/boot/dts/exynos4412.dtsi b/arch/arm/boot/dts/exynos4412.dtsi
>>> index d20db2dfe8e2..a70a671acacd 100644
>>> --- a/arch/arm/boot/dts/exynos4412.dtsi
>>> +++ b/arch/arm/boot/dts/exynos4412.dtsi
>>> @@ -390,6 +390,7 @@
>>> clocks = <&clock CLK_DIV_DMC>;
>>> clock-names = "bus";
>>> operating-points-v2 = <&bus_dmc_opp_table>;
>>> + #interconnect-cells = <0>;
>>> status = "disabled";
>>> };
>>>
>>> @@ -398,6 +399,7 @@
>>> clocks = <&clock CLK_DIV_ACP>;
>>> clock-names = "bus";
>>> operating-points-v2 = <&bus_acp_opp_table>;
>>> + #interconnect-cells = <0>;
>>> status = "disabled";
>>> };
>>>
>>> @@ -406,6 +408,7 @@
>>> clocks = <&clock CLK_DIV_C2C>;
>>> clock-names = "bus";
>>> operating-points-v2 = <&bus_dmc_opp_table>;
>>> + #interconnect-cells = <0>;
>>> status = "disabled";
>>> };
>>>
>>> @@ -459,6 +462,7 @@
>>> clocks = <&clock CLK_DIV_GDL>;
>>> clock-names = "bus";
>>> operating-points-v2 = <&bus_leftbus_opp_table>;
>>> + #interconnect-cells = <0>;
>>> status = "disabled";
>>> };
>>>
>>> @@ -467,6 +471,7 @@
>>> clocks = <&clock CLK_DIV_GDR>;
>>> clock-names = "bus";
>>> operating-points-v2 = <&bus_leftbus_opp_table>;
>>> + #interconnect-cells = <0>;
>>> status = "disabled";
>>> };
>>>
>>> @@ -475,6 +480,7 @@
>>> clocks = <&clock CLK_ACLK160>;
>>> clock-names = "bus";
>>> operating-points-v2 = <&bus_display_opp_table>;
>>> + #interconnect-cells = <0>;
>>> status = "disabled";
>>> };
>>>
>>> @@ -483,6 +489,7 @@
>>> clocks = <&clock CLK_ACLK133>;
>>> clock-names = "bus";
>>> operating-points-v2 = <&bus_fsys_opp_table>;
>>> + #interconnect-cells = <0>;
>>> status = "disabled";
>>> };
>>>
>>> @@ -491,6 +498,7 @@
>>> clocks = <&clock CLK_ACLK100>;
>>> clock-names = "bus";
>>> operating-points-v2 = <&bus_peri_opp_table>;
>>> + #interconnect-cells = <0>;
>>> status = "disabled";
>>> };
>>>
>>> @@ -499,6 +507,7 @@
>>> clocks = <&clock CLK_SCLK_MFC>;
>>> clock-names = "bus";
>>> operating-points-v2 = <&bus_leftbus_opp_table>;
>>> + #interconnect-cells = <0>;
>>> status = "disabled";
>>> };
>>>
>>> --
>>> 2.17.1
>>>
>>
> Best regards
>

--
Best Regards,
Chanwoo Choi
Samsung Electronics

2019-07-31 13:04:13

[permalink] [raw]

Subject: Re: [RFC PATCH 08/11] arm: dts: exynos: Add parents and #interconnect-cells to Exynos4412

On Wed, 2019-07-24 at 21:24 +0200, Krzysztof Kozlowski wrote:
> On Tue, Jul 23, 2019 at 02:20:13PM +0200, Artur Świgoń wrote:
> > This patch adds two fields tp the Exynos4412 DTS:
>
> tp->to

Fixed. Thanks for catching the typo :)

> > - parent: to declare connections between nodes that are not in a
> > parent-child relation in devfreq;
>
> Is it a standard property?
> The explanation needs some improvement... why are you adding parent to a
> devices which are not child-parent?

This is not a standard property. I actually wanted to call it 'neighbor' at first. If you take a look at [1] and search for 'Exynos4x12', you will see that there are two power lines, and each has exactly one parent block (the rest are modelled in devfreq as its children). In [2], the parent of each child is indicated using the 'devfreq' property, e.g.,

&bus_display {
devfreq = <&bus_leftbus>;
status = "okay";
};

The single piece missing to make this topology a connected graph (for
interconnect QoS purposes) is the 'parent' property I proposed in this patch.
bus_leftbus is dependent on bus_dmc, therefore using the term 'parent' is
justified IMHO.

The explanation could be improved by adding what Chanwoo Choi <
[email protected]> expressed in a parallel reply to this patch:
> - If 'devfreq' property is used between buses,
> it indicates that there are requirement of sharing of power line.
> - If 'parent' property is used between buses,
> it indicates that there are requirement of interconnect connection.

[1] Documentation/devicetree/bindings/devfreq/exynos-bus.txt
[2] arch/arm/boot/dts/exynos4412-odroid-common.dtsi (subject of this patch)

Best regards,
--
Artur Świgoń
Samsung R&D Institute Poland
Samsung Electronics

2019-08-13 06:16:01

[permalink] [raw]

Subject: Re: [RFC PATCH 00/11] Simple QoS for exynos-bus driver using interconnect

Hi Artur.

The patch1-4 in this series depend on other patches[1] on mainline.
On next v2 version, please make some patches based on patches[1]
in order to prevent the merge conflict.

[1] [RESEND PATCH v5 0/4] add coupled regulators for Exynos5422/5800
- https://lkml.org/lkml/2019/8/8/217

Also, as I commented, we better to discuss it before sending the v2.

On 19. 7. 23. 오후 9:20, 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 comes down to one
> extra line in the devfreq target() callback, which selects either the
> frequency calculated by the devfreq governor, or the one requested with the
> interconnect API, whichever is higher. 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.
>
> ---
> 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
>
> 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
> icc: Export of_icc_get_from_provider()
> icc: Relax requirement in of_icc_get_from_provider()
> icc: 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 | 296 ++++++++++++++----
> drivers/gpu/drm/exynos/exynos_mixer.c | 68 +++-
> drivers/interconnect/core.c | 12 +-
> include/linux/interconnect-provider.h | 6 +
> 6 files changed, 314 insertions(+), 79 deletions(-)
>

--
Best Regards,
Chanwoo Choi
Samsung Electronics

2019-08-13 06:16:59