[PATCH v3 2/2] PM / devfreq: Add devfreq driver for interconnect bandwidth voting

[PATCH v3 2/2] PM / devfreq: Add devfreq driver for interconnect bandwidth voting

[Saravana Kannan]

This driver registers itself as a devfreq device that allows devfreq
governors to make bandwidth votes for an interconnect path. This allows
applying various policies for different interconnect paths using devfreq
governors.

Example uses:
* Use the devfreq performance governor to set the CPU to DDR interconnect
  path for maximum performance.
* Use the devfreq performance governor to set the GPU to DDR interconnect
  path for maximum performance.
* Use the CPU frequency to device frequency mapping governor to scale the
  DDR frequency based on the needs of the CPUs' current frequency.

Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
---
 Documentation/devicetree/bindings/devfreq/icbw.txt |  21 ++++
 drivers/devfreq/Kconfig                            |  13 +++
 drivers/devfreq/Makefile                           |   1 +
 drivers/devfreq/devfreq_icbw.c                     | 116 +++++++++++++++++++++
 4 files changed, 151 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/devfreq/icbw.txt
 create mode 100644 drivers/devfreq/devfreq_icbw.c

diff --git a/Documentation/devicetree/bindings/devfreq/icbw.txt b/Documentation/devicetree/bindings/devfreq/icbw.txt
new file mode 100644
index 0000000..36cf045
--- /dev/null
+++ b/Documentation/devicetree/bindings/devfreq/icbw.txt
@@ -0,0 +1,21 @@
+Interconnect bandwidth device
+
+icbw is a device that represents an interconnect path that connects two
+devices. This device is typically used to vote for BW requirements between
+two devices. Eg: CPU to DDR, GPU to DDR, etc
+
+Required properties:
+- compatible:		Must be "devfreq-icbw"
+- interconnects:	Pairs of phandles and interconnect provider specifier
+			to denote the edge source and destination ports of
+			the interconnect path. See also:
+		Documentation/devicetree/bindings/interconnect/interconnect.txt
+- interconnect-names:	Must have one entry with the name "path".
+
+Example:
+
+	qcom,cpubw {
+		compatible = "devfreq-icbw";
+		interconnects = <&snoc MASTER_APSS_1 &bimc SLAVE_EBI_CH0>;
+		interconnect-names = "path";
+	};
diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
index 3d9ae68..590370e 100644
--- a/drivers/devfreq/Kconfig
+++ b/drivers/devfreq/Kconfig
@@ -121,6 +121,19 @@ config ARM_RK3399_DMC_DEVFREQ
           It sets the frequency for the memory controller and reads the usage counts
           from hardware.
 
+config DEVFREQ_ICBW
+	bool "DEVFREQ device for making bandwidth votes on interconnect paths"
+	select DEVFREQ_GOV_PERFORMANCE
+	select DEVFREQ_GOV_POWERSAVE
+	select DEVFREQ_GOV_USERSPACE
+	default n
+	help
+	  Different devfreq governors use this devfreq device to make
+	  bandwidth votes for interconnect paths between different devices
+	  (Eg: CPU to DDR, GPU to DDR, etc). This driver provides a generic
+	  interface so that the devfreq governors can be shared across SoCs
+	  and architectures.
+
 source "drivers/devfreq/event/Kconfig"
 
 endif # PM_DEVFREQ
diff --git a/drivers/devfreq/Makefile b/drivers/devfreq/Makefile
index e9dc3e9..b302c5cf 100644
--- a/drivers/devfreq/Makefile
+++ b/drivers/devfreq/Makefile
@@ -12,6 +12,7 @@ obj-$(CONFIG_DEVFREQ_GOV_CPUFREQ_MAP)	+= governor_cpufreq_map.o
 obj-$(CONFIG_ARM_EXYNOS_BUS_DEVFREQ)	+= exynos-bus.o
 obj-$(CONFIG_ARM_RK3399_DMC_DEVFREQ)	+= rk3399_dmc.o
 obj-$(CONFIG_ARM_TEGRA_DEVFREQ)		+= tegra-devfreq.o
+obj-$(CONFIG_DEVFREQ_ICBW)		+= devfreq_icbw.o
 
 # DEVFREQ Event Drivers
 obj-$(CONFIG_PM_DEVFREQ_EVENT)		+= event/
diff --git a/drivers/devfreq/devfreq_icbw.c b/drivers/devfreq/devfreq_icbw.c
new file mode 100644
index 0000000..231fb21
--- /dev/null
+++ b/drivers/devfreq/devfreq_icbw.c
@@ -0,0 +1,116 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2013-2014, 2018, The Linux Foundation. All rights reserved.
+ */
+
+#define pr_fmt(fmt) "icbw: " fmt
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/err.h>
+#include <linux/errno.h>
+#include <linux/mutex.h>
+#include <linux/devfreq.h>
+#include <linux/platform_device.h>
+#include <linux/of.h>
+#include <linux/interconnect.h>
+
+struct dev_data {
+	struct icc_path *path;
+	u32 cur_ab;
+	u32 cur_pb;
+	unsigned long gov_ab;
+	struct devfreq *df;
+	struct devfreq_dev_profile dp;
+};
+
+static int icbw_target(struct device *dev, unsigned long *freq, u32 flags)
+{
+	struct dev_data *d = dev_get_drvdata(dev);
+	int ret;
+	u32 new_pb = *freq, new_ab = d->gov_ab;
+
+	if (d->cur_pb == new_pb && d->cur_ab == new_ab)
+		return 0;
+
+	dev_dbg(dev, "BW KBps: AB: %u PB: %u\n", new_ab, new_pb);
+
+	ret = icc_set(d->path, new_ab, new_pb);
+	if (ret) {
+		dev_err(dev, "bandwidth request failed (%d)\n", ret);
+	} else {
+		d->cur_pb = new_pb;
+		d->cur_ab = new_ab;
+	}
+
+	return ret;
+}
+
+static int icbw_get_dev_status(struct device *dev,
+				struct devfreq_dev_status *stat)
+{
+	struct dev_data *d = dev_get_drvdata(dev);
+
+	stat->private_data = &d->gov_ab;
+	return 0;
+}
+
+static int devfreq_icbw_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct dev_data *d;
+	struct devfreq_dev_profile *p;
+
+	d = devm_kzalloc(dev, sizeof(*d), GFP_KERNEL);
+	if (!d)
+		return -ENOMEM;
+	dev_set_drvdata(dev, d);
+
+	p = &d->dp;
+	p->polling_ms = 50;
+	p->target = icbw_target;
+	p->get_dev_status = icbw_get_dev_status;
+
+	d->path = of_icc_get(dev, "path");
+	if (IS_ERR(d->path)) {
+		dev_err(dev, "Unable to register interconnect path\n");
+		return -ENODEV;
+	}
+
+	d->df = devfreq_add_device(dev, p, "performance", NULL);
+	if (IS_ERR(d->df)) {
+		icc_put(d->path);
+		return PTR_ERR(d->df);
+	}
+
+	return 0;
+}
+
+static int devfreq_icbw_remove(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct dev_data *d = dev_get_drvdata(dev);
+
+	devfreq_remove_device(d->df);
+	icc_put(d->path);
+	return 0;
+}
+
+static const struct of_device_id icbw_match_table[] = {
+	{ .compatible = "devfreq-icbw" },
+	{}
+};
+
+static struct platform_driver icbw_driver = {
+	.probe = devfreq_icbw_probe,
+	.remove = devfreq_icbw_remove,
+	.driver = {
+		.name = "devfreq-icbw",
+		.of_match_table = icbw_match_table,
+	},
+};
+
+module_platform_driver(icbw_driver);
+MODULE_DESCRIPTION("Interconnect bandwidth voting driver");
+MODULE_LICENSE("GPL v2");
-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

RE: [PATCH v3 2/2] PM / devfreq: Add devfreq driver for interconnect bandwidth voting

[MyungJoo Ham]

>This driver registers itself as a devfreq device that allows devfreq
>governors to make bandwidth votes for an interconnect path. This allows
>applying various policies for different interconnect paths using devfreq
>governors.
>

First of all, the name, "devfreq_icbw", is not appropriate for a
devfreq device driver. It confuses; it looks like a part of the
framework itself.

>diff --git a/drivers/devfreq/devfreq_icbw.c b/drivers/devfreq/devfreq_icbw.c
>new file mode 100644
>index 0000000..231fb21
>--- /dev/null
>+++ b/drivers/devfreq/devfreq_icbw.c
>@@ -0,0 +1,116 @@
>+// SPDX-License-Identifier: GPL-2.0
>+/*
>+ * Copyright (c) 2013-2014, 2018, The Linux Foundation. All rights reserved.
>+ */
>+
>+#define pr_fmt(fmt) "icbw: " fmt
>+
>+#include <linux/kernel.h>
>+#include <linux/module.h>
>+#include <linux/init.h>
>+#include <linux/err.h>
>+#include <linux/errno.h>
>+#include <linux/mutex.h>
>+#include <linux/devfreq.h>
>+#include <linux/platform_device.h>
>+#include <linux/of.h>
>+#include <linux/interconnect.h>

Where can I find this file?

>+
>+struct dev_data {
>+	struct icc_path *path;
>+	u32 cur_ab;
>+	u32 cur_pb;
>+	unsigned long gov_ab;
>+	struct devfreq *df;
>+	struct devfreq_dev_profile dp;
>+};
>+
>+static int icbw_target(struct device *dev, unsigned long *freq, u32 flags)
>+{
>+	struct dev_data *d = dev_get_drvdata(dev);
>+	int ret;
>+	u32 new_pb = *freq, new_ab = d->gov_ab;
>+
>+	if (d->cur_pb == new_pb && d->cur_ab == new_ab)
>+		return 0;
>+
>+	dev_dbg(dev, "BW KBps: AB: %u PB: %u\n", new_ab, new_pb);
>+
>+	ret = icc_set(d->path, new_ab, new_pb);

I'm not sure if icc_set is available.


Cheers,
MyungJoo

Re: [PATCH v3 2/2] PM / devfreq: Add devfreq driver for interconnect bandwidth voting

[Georgi Djakov]

Hi MyungJoo,

On 08/02/2018 01:13 PM, MyungJoo Ham wrote:
>> This driver registers itself as a devfreq device that allows devfreq
>> governors to make bandwidth votes for an interconnect path. This allows
>> applying various policies for different interconnect paths using devfreq
>> governors.
>>
> 
> First of all, the name, "devfreq_icbw", is not appropriate for a
> devfreq device driver. It confuses; it looks like a part of the
> framework itself.
> 
>> diff --git a/drivers/devfreq/devfreq_icbw.c b/drivers/devfreq/devfreq_icbw.c
>> new file mode 100644
>> index 0000000..231fb21
>> --- /dev/null
>> +++ b/drivers/devfreq/devfreq_icbw.c
>> @@ -0,0 +1,116 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2013-2014, 2018, The Linux Foundation. All rights reserved.
>> + */
>> +
>> +#define pr_fmt(fmt) "icbw: " fmt
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/init.h>
>> +#include <linux/err.h>
>> +#include <linux/errno.h>
>> +#include <linux/mutex.h>
>> +#include <linux/devfreq.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/of.h>
>> +#include <linux/interconnect.h>
> 
> Where can I find this file?

It is part of the On-chip interconnect API, which is currently under
review and is available here: https://lkml.org/lkml/2018/7/31/647

Thanks,
Georgi

Re: [PATCH v3 2/2] PM / devfreq: Add devfreq driver for interconnect bandwidth voting

[skannan]

On 2018-08-02 03:13, MyungJoo Ham wrote:
>> This driver registers itself as a devfreq device that allows devfreq
>> governors to make bandwidth votes for an interconnect path. This 
>> allows
>> applying various policies for different interconnect paths using 
>> devfreq
>> governors.
>> 
> 
> First of all, the name, "devfreq_icbw", is not appropriate for a
> devfreq device driver. It confuses; it looks like a part of the
> framework itself.
> 
>> diff --git a/drivers/devfreq/devfreq_icbw.c 
>> b/drivers/devfreq/devfreq_icbw.c
>> new file mode 100644
>> index 0000000..231fb21
>> --- /dev/null
>> +++ b/drivers/devfreq/devfreq_icbw.c
>> @@ -0,0 +1,116 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2013-2014, 2018, The Linux Foundation. All rights 
>> reserved.
>> + */
>> +
>> +#define pr_fmt(fmt) "icbw: " fmt
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/init.h>
>> +#include <linux/err.h>
>> +#include <linux/errno.h>
>> +#include <linux/mutex.h>
>> +#include <linux/devfreq.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/of.h>
>> +#include <linux/interconnect.h>
> 
> Where can I find this file?

Sorry, meant to mention this in the email specific portion of the commit 
text. This is on top of the interconnect framework series that Georgi 
has been working on. linux-pm should have those patches.


>> +
>> +struct dev_data {
>> +	struct icc_path *path;
>> +	u32 cur_ab;
>> +	u32 cur_pb;
>> +	unsigned long gov_ab;
>> +	struct devfreq *df;
>> +	struct devfreq_dev_profile dp;
>> +};
>> +
>> +static int icbw_target(struct device *dev, unsigned long *freq, u32 
>> flags)
>> +{
>> +	struct dev_data *d = dev_get_drvdata(dev);
>> +	int ret;
>> +	u32 new_pb = *freq, new_ab = d->gov_ab;
>> +
>> +	if (d->cur_pb == new_pb && d->cur_ab == new_ab)
>> +		return 0;
>> +
>> +	dev_dbg(dev, "BW KBps: AB: %u PB: %u\n", new_ab, new_pb);
>> +
>> +	ret = icc_set(d->path, new_ab, new_pb);
> 
> I'm not sure if icc_set is available.

Yeah, it's available on that patch series.

-Saravana

Re: [PATCH v3 2/2] PM / devfreq: Add devfreq driver for interconnect bandwidth voting

[Rob Herring]

On Wed, Aug 01, 2018 at 05:57:42PM -0700, Saravana Kannan wrote:
> This driver registers itself as a devfreq device that allows devfreq
> governors to make bandwidth votes for an interconnect path. This allows
> applying various policies for different interconnect paths using devfreq
> governors.
> 
> Example uses:
> * Use the devfreq performance governor to set the CPU to DDR interconnect
>   path for maximum performance.
> * Use the devfreq performance governor to set the GPU to DDR interconnect
>   path for maximum performance.
> * Use the CPU frequency to device frequency mapping governor to scale the
>   DDR frequency based on the needs of the CPUs' current frequency.
> 
> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
> ---
>  Documentation/devicetree/bindings/devfreq/icbw.txt |  21 ++++

Please make bindings separate a patch.

>  drivers/devfreq/Kconfig                            |  13 +++
>  drivers/devfreq/Makefile                           |   1 +
>  drivers/devfreq/devfreq_icbw.c                     | 116 +++++++++++++++++++++
>  4 files changed, 151 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/devfreq/icbw.txt
>  create mode 100644 drivers/devfreq/devfreq_icbw.c
> 
> diff --git a/Documentation/devicetree/bindings/devfreq/icbw.txt b/Documentation/devicetree/bindings/devfreq/icbw.txt
> new file mode 100644
> index 0000000..36cf045
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/devfreq/icbw.txt
> @@ -0,0 +1,21 @@
> +Interconnect bandwidth device
> +
> +icbw is a device that represents an interconnect path that connects two
> +devices. This device is typically used to vote for BW requirements between
> +two devices. Eg: CPU to DDR, GPU to DDR, etc

I'm pretty sure this doesn't represent a h/w device. This usage doesn't 
encourage me to accept the interconnects binding either.

> +
> +Required properties:
> +- compatible:		Must be "devfreq-icbw"
> +- interconnects:	Pairs of phandles and interconnect provider specifier
> +			to denote the edge source and destination ports of
> +			the interconnect path. See also:
> +		Documentation/devicetree/bindings/interconnect/interconnect.txt
> +- interconnect-names:	Must have one entry with the name "path".

That's pretty useless...

> +
> +Example:
> +
> +	qcom,cpubw {

Someone in QCom please broadcast to stop using qcom,foo for node names. 
It is amazing how consistent you all are. If only folks were as 
consistent in reading 
Documentation/devicetree/bindings/submitting-patches.txt.

Rob

Re: [PATCH v3 2/2] PM / devfreq: Add devfreq driver for interconnect bandwidth voting

[skannan]

On 2018-08-07 09:51, Rob Herring wrote:
> On Wed, Aug 01, 2018 at 05:57:42PM -0700, Saravana Kannan wrote:
>> This driver registers itself as a devfreq device that allows devfreq
>> governors to make bandwidth votes for an interconnect path. This 
>> allows
>> applying various policies for different interconnect paths using 
>> devfreq
>> governors.
>> 
>> Example uses:
>> * Use the devfreq performance governor to set the CPU to DDR 
>> interconnect
>>   path for maximum performance.
>> * Use the devfreq performance governor to set the GPU to DDR 
>> interconnect
>>   path for maximum performance.
>> * Use the CPU frequency to device frequency mapping governor to scale 
>> the
>>   DDR frequency based on the needs of the CPUs' current frequency.
>> 
>> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
>> ---
>>  Documentation/devicetree/bindings/devfreq/icbw.txt |  21 ++++
> 
> Please make bindings separate a patch.

Yeah, I was aware of that. I just wanted to give some context in the v1 
of this patch (I wasn't expecting this to merge as is).

>>  drivers/devfreq/Kconfig                            |  13 +++
>>  drivers/devfreq/Makefile                           |   1 +
>>  drivers/devfreq/devfreq_icbw.c                     | 116 
>> +++++++++++++++++++++
>>  4 files changed, 151 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/devfreq/icbw.txt
>>  create mode 100644 drivers/devfreq/devfreq_icbw.c
>> 
>> diff --git a/Documentation/devicetree/bindings/devfreq/icbw.txt 
>> b/Documentation/devicetree/bindings/devfreq/icbw.txt
>> new file mode 100644
>> index 0000000..36cf045
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/devfreq/icbw.txt
>> @@ -0,0 +1,21 @@
>> +Interconnect bandwidth device
>> +
>> +icbw is a device that represents an interconnect path that connects 
>> two
>> +devices. This device is typically used to vote for BW requirements 
>> between
>> +two devices. Eg: CPU to DDR, GPU to DDR, etc
> 
> I'm pretty sure this doesn't represent a h/w device. This usage doesn't
> encourage me to accept the interconnects binding either.

Hasn't the DT rules moved past "only HW devices" in DT? Logical devices 
are still allowed in Linux DT bindings?

Having said that, this is explicitly representing a real HW path and the 
ability to control its performance.

>> +
>> +Required properties:
>> +- compatible:		Must be "devfreq-icbw"
>> +- interconnects:	Pairs of phandles and interconnect provider 
>> specifier
>> +			to denote the edge source and destination ports of
>> +			the interconnect path. See also:
>> +		Documentation/devicetree/bindings/interconnect/interconnect.txt
>> +- interconnect-names:	Must have one entry with the name "path".
> 
> That's pretty useless...

True. But the current DT binding for interconnect consumer bindings 
needs a interconnect name to use the of_* API. I'm open to switching to 
an index based API if one is provided.

>> +
>> +Example:
>> +
>> +	qcom,cpubw {
> 
> Someone in QCom please broadcast to stop using qcom,foo for node names.
> It is amazing how consistent you all are. If only folks were as
> consistent in reading
> Documentation/devicetree/bindings/submitting-patches.txt.

Sorry :(

Thanks,
Saravana

Re: [PATCH v3 2/2] PM / devfreq: Add devfreq driver for interconnect bandwidth voting

[Georgi Djakov]

Hi Saravana,

On 08/02/2018 03:57 AM, Saravana Kannan wrote:
> This driver registers itself as a devfreq device that allows devfreq
> governors to make bandwidth votes for an interconnect path. This allows
> applying various policies for different interconnect paths using devfreq
> governors.
> 
> Example uses:
> * Use the devfreq performance governor to set the CPU to DDR interconnect
>   path for maximum performance.
> * Use the devfreq performance governor to set the GPU to DDR interconnect
>   path for maximum performance.
> * Use the CPU frequency to device frequency mapping governor to scale the
>   DDR frequency based on the needs of the CPUs' current frequency.

Usually CPUs and GPUs have dedicated cpufreq/devfreq drivers and i was
wondering if the interconnect support could be put into these drivers
directly?

> 
> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
> ---
>  Documentation/devicetree/bindings/devfreq/icbw.txt |  21 ++++
>  drivers/devfreq/Kconfig                            |  13 +++
>  drivers/devfreq/Makefile                           |   1 +
>  drivers/devfreq/devfreq_icbw.c                     | 116 +++++++++++++++++++++
>  4 files changed, 151 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/devfreq/icbw.txt
>  create mode 100644 drivers/devfreq/devfreq_icbw.c
> 
> diff --git a/Documentation/devicetree/bindings/devfreq/icbw.txt b/Documentation/devicetree/bindings/devfreq/icbw.txt
> new file mode 100644
> index 0000000..36cf045
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/devfreq/icbw.txt
> @@ -0,0 +1,21 @@
> +Interconnect bandwidth device
> +
> +icbw is a device that represents an interconnect path that connects two
> +devices. This device is typically used to vote for BW requirements between
> +two devices. Eg: CPU to DDR, GPU to DDR, etc
> +
> +Required properties:
> +- compatible:		Must be "devfreq-icbw"
> +- interconnects:	Pairs of phandles and interconnect provider specifier
> +			to denote the edge source and destination ports of
> +			the interconnect path. See also:
> +		Documentation/devicetree/bindings/interconnect/interconnect.txt
> +- interconnect-names:	Must have one entry with the name "path".
> +
> +Example:
> +
> +	qcom,cpubw {
> +		compatible = "devfreq-icbw";
> +		interconnects = <&snoc MASTER_APSS_1 &bimc SLAVE_EBI_CH0>;
> +		interconnect-names = "path";

interconnect-names is optional when there is only a single path.

> +	};
> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
> index 3d9ae68..590370e 100644
> --- a/drivers/devfreq/Kconfig
> +++ b/drivers/devfreq/Kconfig
> @@ -121,6 +121,19 @@ config ARM_RK3399_DMC_DEVFREQ
>            It sets the frequency for the memory controller and reads the usage counts
>            from hardware.
>  
> +config DEVFREQ_ICBW
> +	bool "DEVFREQ device for making bandwidth votes on interconnect paths"

Can this be a module?

> +	select DEVFREQ_GOV_PERFORMANCE
> +	select DEVFREQ_GOV_POWERSAVE
> +	select DEVFREQ_GOV_USERSPACE
> +	default n

There's no need to specify this default. It is 'n' by default anyway.
Also maybe you want to add something like:
	depends on INTERCONNECT=y

Thanks,
Georgi

> +	help
> +	  Different devfreq governors use this devfreq device to make
> +	  bandwidth votes for interconnect paths between different devices
> +	  (Eg: CPU to DDR, GPU to DDR, etc). This driver provides a generic
> +	  interface so that the devfreq governors can be shared across SoCs
> +	  and architectures.
> +
>  source "drivers/devfreq/event/Kconfig"

Re: [PATCH v3 2/2] PM / devfreq: Add devfreq driver for interconnect bandwidth voting

[Sibi Sankar]

Hi Georgi,

This driver uses of_icc_get which is very likely to fail if it probes
before the interconnect provider. Would it be possible for icc_get to
return/differentiate both -EPROBE_DEFER and other errors to prevent the
driver to continually probe defer if the path doesn't actually exist
or just error out if it probes before the interconnect provider.

On 2018-08-23 18:30, Georgi Djakov wrote:
> Hi Saravana,
> 
> On 08/02/2018 03:57 AM, Saravana Kannan wrote:
>> This driver registers itself as a devfreq device that allows devfreq
>> governors to make bandwidth votes for an interconnect path. This 
>> allows
>> applying various policies for different interconnect paths using 
>> devfreq
>> governors.
>> 
>> Example uses:
>> * Use the devfreq performance governor to set the CPU to DDR 
>> interconnect
>>   path for maximum performance.
>> * Use the devfreq performance governor to set the GPU to DDR 
>> interconnect
>>   path for maximum performance.
>> * Use the CPU frequency to device frequency mapping governor to scale 
>> the
>>   DDR frequency based on the needs of the CPUs' current frequency.
> 
> Usually CPUs and GPUs have dedicated cpufreq/devfreq drivers and i was
> wondering if the interconnect support could be put into these drivers
> directly?
> 
>> 
>> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
>> ---
>>  Documentation/devicetree/bindings/devfreq/icbw.txt |  21 ++++
>>  drivers/devfreq/Kconfig                            |  13 +++
>>  drivers/devfreq/Makefile                           |   1 +
>>  drivers/devfreq/devfreq_icbw.c                     | 116 
>> +++++++++++++++++++++
>>  4 files changed, 151 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/devfreq/icbw.txt
>>  create mode 100644 drivers/devfreq/devfreq_icbw.c
>> 
>> diff --git a/Documentation/devicetree/bindings/devfreq/icbw.txt 
>> b/Documentation/devicetree/bindings/devfreq/icbw.txt
>> new file mode 100644
>> index 0000000..36cf045
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/devfreq/icbw.txt
>> @@ -0,0 +1,21 @@
>> +Interconnect bandwidth device
>> +
>> +icbw is a device that represents an interconnect path that connects 
>> two
>> +devices. This device is typically used to vote for BW requirements 
>> between
>> +two devices. Eg: CPU to DDR, GPU to DDR, etc
>> +
>> +Required properties:
>> +- compatible:		Must be "devfreq-icbw"
>> +- interconnects:	Pairs of phandles and interconnect provider 
>> specifier
>> +			to denote the edge source and destination ports of
>> +			the interconnect path. See also:
>> +		Documentation/devicetree/bindings/interconnect/interconnect.txt
>> +- interconnect-names:	Must have one entry with the name "path".
>> +
>> +Example:
>> +
>> +	qcom,cpubw {
>> +		compatible = "devfreq-icbw";
>> +		interconnects = <&snoc MASTER_APSS_1 &bimc SLAVE_EBI_CH0>;
>> +		interconnect-names = "path";
> 
> interconnect-names is optional when there is only a single path.
> 
>> +	};
>> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
>> index 3d9ae68..590370e 100644
>> --- a/drivers/devfreq/Kconfig
>> +++ b/drivers/devfreq/Kconfig
>> @@ -121,6 +121,19 @@ config ARM_RK3399_DMC_DEVFREQ
>>            It sets the frequency for the memory controller and reads 
>> the usage counts
>>            from hardware.
>> 
>> +config DEVFREQ_ICBW
>> +	bool "DEVFREQ device for making bandwidth votes on interconnect 
>> paths"
> 
> Can this be a module?
> 
>> +	select DEVFREQ_GOV_PERFORMANCE
>> +	select DEVFREQ_GOV_POWERSAVE
>> +	select DEVFREQ_GOV_USERSPACE
>> +	default n
> 
> There's no need to specify this default. It is 'n' by default anyway.
> Also maybe you want to add something like:
> 	depends on INTERCONNECT=y
> 
> Thanks,
> Georgi
> 
>> +	help
>> +	  Different devfreq governors use this devfreq device to make
>> +	  bandwidth votes for interconnect paths between different devices
>> +	  (Eg: CPU to DDR, GPU to DDR, etc). This driver provides a generic
>> +	  interface so that the devfreq governors can be shared across SoCs
>> +	  and architectures.
>> +
>>  source "drivers/devfreq/event/Kconfig"

-- 
-- Sibi Sankar --
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project.

Re: [PATCH v3 2/2] PM / devfreq: Add devfreq driver for interconnect bandwidth voting

[Sibi Sankar]

Hi Saravana,

On 2018-08-02 06:27, Saravana Kannan wrote:
> This driver registers itself as a devfreq device that allows devfreq
> governors to make bandwidth votes for an interconnect path. This allows
> applying various policies for different interconnect paths using 
> devfreq
> governors.
> 
> Example uses:
> * Use the devfreq performance governor to set the CPU to DDR 
> interconnect
>   path for maximum performance.
> * Use the devfreq performance governor to set the GPU to DDR 
> interconnect
>   path for maximum performance.
> * Use the CPU frequency to device frequency mapping governor to scale 
> the
>   DDR frequency based on the needs of the CPUs' current frequency.
> 
> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
> ---
>  Documentation/devicetree/bindings/devfreq/icbw.txt |  21 ++++
>  drivers/devfreq/Kconfig                            |  13 +++
>  drivers/devfreq/Makefile                           |   1 +
>  drivers/devfreq/devfreq_icbw.c                     | 116 
> +++++++++++++++++++++
>  4 files changed, 151 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/devfreq/icbw.txt
>  create mode 100644 drivers/devfreq/devfreq_icbw.c
> 
> diff --git a/Documentation/devicetree/bindings/devfreq/icbw.txt
> b/Documentation/devicetree/bindings/devfreq/icbw.txt
> new file mode 100644
> index 0000000..36cf045
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/devfreq/icbw.txt
> @@ -0,0 +1,21 @@
> +Interconnect bandwidth device
> +
> +icbw is a device that represents an interconnect path that connects 
> two
> +devices. This device is typically used to vote for BW requirements 
> between
> +two devices. Eg: CPU to DDR, GPU to DDR, etc
> +
> +Required properties:
> +- compatible:		Must be "devfreq-icbw"
> +- interconnects:	Pairs of phandles and interconnect provider specifier
> +			to denote the edge source and destination ports of
> +			the interconnect path. See also:
> +		Documentation/devicetree/bindings/interconnect/interconnect.txt
> +- interconnect-names:	Must have one entry with the name "path".
> +
> +Example:
> +
> +	qcom,cpubw {
> +		compatible = "devfreq-icbw";
> +		interconnects = <&snoc MASTER_APSS_1 &bimc SLAVE_EBI_CH0>;
> +		interconnect-names = "path";
> +	};
> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
> index 3d9ae68..590370e 100644
> --- a/drivers/devfreq/Kconfig
> +++ b/drivers/devfreq/Kconfig
> @@ -121,6 +121,19 @@ config ARM_RK3399_DMC_DEVFREQ
>            It sets the frequency for the memory controller and reads
> the usage counts
>            from hardware.
> 
> +config DEVFREQ_ICBW
> +	bool "DEVFREQ device for making bandwidth votes on interconnect 
> paths"
> +	select DEVFREQ_GOV_PERFORMANCE
> +	select DEVFREQ_GOV_POWERSAVE
> +	select DEVFREQ_GOV_USERSPACE
> +	default n
> +	help
> +	  Different devfreq governors use this devfreq device to make
> +	  bandwidth votes for interconnect paths between different devices
> +	  (Eg: CPU to DDR, GPU to DDR, etc). This driver provides a generic
> +	  interface so that the devfreq governors can be shared across SoCs
> +	  and architectures.
> +
>  source "drivers/devfreq/event/Kconfig"
> 
>  endif # PM_DEVFREQ
> diff --git a/drivers/devfreq/Makefile b/drivers/devfreq/Makefile
> index e9dc3e9..b302c5cf 100644
> --- a/drivers/devfreq/Makefile
> +++ b/drivers/devfreq/Makefile
> @@ -12,6 +12,7 @@ obj-$(CONFIG_DEVFREQ_GOV_CPUFREQ_MAP)	+=
> governor_cpufreq_map.o
>  obj-$(CONFIG_ARM_EXYNOS_BUS_DEVFREQ)	+= exynos-bus.o
>  obj-$(CONFIG_ARM_RK3399_DMC_DEVFREQ)	+= rk3399_dmc.o
>  obj-$(CONFIG_ARM_TEGRA_DEVFREQ)		+= tegra-devfreq.o
> +obj-$(CONFIG_DEVFREQ_ICBW)		+= devfreq_icbw.o
> 
>  # DEVFREQ Event Drivers
>  obj-$(CONFIG_PM_DEVFREQ_EVENT)		+= event/
> diff --git a/drivers/devfreq/devfreq_icbw.c 
> b/drivers/devfreq/devfreq_icbw.c
> new file mode 100644
> index 0000000..231fb21
> --- /dev/null
> +++ b/drivers/devfreq/devfreq_icbw.c
> @@ -0,0 +1,116 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2013-2014, 2018, The Linux Foundation. All rights 
> reserved.
> + */
> +
> +#define pr_fmt(fmt) "icbw: " fmt
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/err.h>
> +#include <linux/errno.h>
> +#include <linux/mutex.h>
> +#include <linux/devfreq.h>
> +#include <linux/platform_device.h>
> +#include <linux/of.h>
> +#include <linux/interconnect.h>
> +
> +struct dev_data {
> +	struct icc_path *path;
> +	u32 cur_ab;
> +	u32 cur_pb;
> +	unsigned long gov_ab;
> +	struct devfreq *df;
> +	struct devfreq_dev_profile dp;
> +};
> +
> +static int icbw_target(struct device *dev, unsigned long *freq, u32 
> flags)
> +{
> +	struct dev_data *d = dev_get_drvdata(dev);
> +	int ret;
> +	u32 new_pb = *freq, new_ab = d->gov_ab;
> +
> +	if (d->cur_pb == new_pb && d->cur_ab == new_ab)
> +		return 0;
> +
> +	dev_dbg(dev, "BW KBps: AB: %u PB: %u\n", new_ab, new_pb);
> +
> +	ret = icc_set(d->path, new_ab, new_pb);
> +	if (ret) {
> +		dev_err(dev, "bandwidth request failed (%d)\n", ret);
> +	} else {
> +		d->cur_pb = new_pb;
> +		d->cur_ab = new_ab;
> +	}
> +
> +	return ret;
> +}
> +
> +static int icbw_get_dev_status(struct device *dev,
> +				struct devfreq_dev_status *stat)
> +{
> +	struct dev_data *d = dev_get_drvdata(dev);
> +
> +	stat->private_data = &d->gov_ab;
> +	return 0;
> +}
> +
> +static int devfreq_icbw_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct dev_data *d;
> +	struct devfreq_dev_profile *p;
> +
> +	d = devm_kzalloc(dev, sizeof(*d), GFP_KERNEL);
> +	if (!d)
> +		return -ENOMEM;
> +	dev_set_drvdata(dev, d);
> +
> +	p = &d->dp;
> +	p->polling_ms = 50;
> +	p->target = icbw_target;
> +	p->get_dev_status = icbw_get_dev_status;
> +
> +	d->path = of_icc_get(dev, "path");
> +	if (IS_ERR(d->path)) {
> +		dev_err(dev, "Unable to register interconnect path\n");
> +		return -ENODEV;
> +	}

The probe fails if the provider is not registered yet. Worked around it 
using -EPROBE_DEFER
but this should probably come from of_icc_get itself.

> +
> +	d->df = devfreq_add_device(dev, p, "performance", NULL);
> +	if (IS_ERR(d->df)) {
> +		icc_put(d->path);
> +		return PTR_ERR(d->df);
> +	}

devfreq_add_device will fail with -EINVAL for set_table_freq, 
find_available_min_freq and
find_available_max_freq. If you end up working your way around it, I 
still ended up getting
multiple errors like "Couldn't update frequency transition information" 
after probing.

> +
> +	return 0;
> +}
> +
> +static int devfreq_icbw_remove(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct dev_data *d = dev_get_drvdata(dev);
> +
> +	devfreq_remove_device(d->df);
> +	icc_put(d->path);
> +	return 0;
> +}
> +
> +static const struct of_device_id icbw_match_table[] = {
> +	{ .compatible = "devfreq-icbw" },
> +	{}
> +};
> +
> +static struct platform_driver icbw_driver = {
> +	.probe = devfreq_icbw_probe,
> +	.remove = devfreq_icbw_remove,
> +	.driver = {
> +		.name = "devfreq-icbw",
> +		.of_match_table = icbw_match_table,
> +	},
> +};
> +
> +module_platform_driver(icbw_driver);
> +MODULE_DESCRIPTION("Interconnect bandwidth voting driver");
> +MODULE_LICENSE("GPL v2");

-- 
-- Sibi Sankar --
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project.