[PATCH v3 0/7] Add powerdomain driver for corners on msm8996/sdm845

[PATCH v3 0/7] Add powerdomain driver for corners on msm8996/sdm845

[Rajendra Nayak]

Changes in v3:
* Bindings split into seperate patches
* Bindings updated to remove duplicate OPP table phandles
* DT headers defining macros for Power domain indexes and OPP levels
* Optimisations to use rpmh_write_async() whereever applicable
* Fixed up handling of ACTIVE_ONLY/WAKE_ONLY/SLEEP voting for RPMh
* Fixed the vlvl to hlvl conversions in set_performance
* Other minor fixes based on review of v2
* TODO: This series does not handle the case where all VDD_MX votes
should be higher than VDD_CX from APPs, as pointed out
by David Collins in v2. This needs support at genpd to propogate performance
state up the parents, if we model these as Parent/Child to handle the
interdependency.

Changes in v2:
* added a powerdomain driver for sdm845 which supports communicating to RPMh
* dropped the changes to sdhc driver to move over to using OPP
as there is active discussion on using OPP as the interface vs
handling all of it in clock drivers
* Other minor binding updates based on review of v1

With performance state support for genpd/OPP merged, this is an effort
to model a powerdomain driver to communicate corner/level
values for qualcomm platforms to RPM (Remote Power Manager) and RPMh.

Rajendra Nayak (7):
  dt-bindings: power: Add qcom rpm power domain driver bindings
  soc: qcom: rpmpd: Add a Power domain driver to model corners
  soc: qcom: rpmpd: Add support for get/set performance state
  arm64: dts: msm8996: Add rpmpd device node
  dt-bindings: power: Add qcom rpmh power domain driver bindings
  soc: qcom: Add RPMh Power domain driver
  soc: qcom: rpmpd/rpmhpd: Add a max vote on all corners at init

 .../devicetree/bindings/power/qcom,rpmhpd.txt |  65 +++
 .../devicetree/bindings/power/qcom,rpmpd.txt  |  49 ++
 arch/arm64/boot/dts/qcom/msm8996.dtsi         |  34 ++
 drivers/soc/qcom/Kconfig                      |  18 +
 drivers/soc/qcom/Makefile                     |   2 +
 drivers/soc/qcom/rpmhpd.c                     | 437 ++++++++++++++++++
 drivers/soc/qcom/rpmpd.c                      | 356 ++++++++++++++
 include/dt-bindings/power/qcom-rpmhpd.h       |  31 ++
 include/dt-bindings/power/qcom-rpmpd.h        |  16 +
 9 files changed, 1008 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/qcom,rpmhpd.txt
 create mode 100644 Documentation/devicetree/bindings/power/qcom,rpmpd.txt
 create mode 100644 drivers/soc/qcom/rpmhpd.c
 create mode 100644 drivers/soc/qcom/rpmpd.c
 create mode 100644 include/dt-bindings/power/qcom-rpmhpd.h
 create mode 100644 include/dt-bindings/power/qcom-rpmpd.h

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v3 1/7] dt-bindings: power: Add qcom rpm power domain driver bindings

[Rajendra Nayak]

Add DT bindings to describe the rpm power domains found on Qualcomm
Technologies, Inc. SoCs. These power domains communicate a performance
state to RPM, which then translates it into corresponding voltage on a
PMIC rail.

Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 .../devicetree/bindings/power/qcom,rpmpd.txt  | 49 +++++++++++++++++++
 1 file changed, 49 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/qcom,rpmpd.txt

diff --git a/Documentation/devicetree/bindings/power/qcom,rpmpd.txt b/Documentation/devicetree/bindings/power/qcom,rpmpd.txt
new file mode 100644
index 000000000000..61a0e2687940
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/qcom,rpmpd.txt
@@ -0,0 +1,49 @@
+Qualcomm RPM Power domains
+
+For RPM Power domains, we communicate a performance state to RPM
+which then translates it into a corresponding voltage on a rail
+
+Required Properties:
+ - compatible: Should be one of the following
+	* qcom,msm8996-rpmpd: RPM Power domain for the msm8996 family of SoC
+ - power-domain-cells: number of cells in Power domain specifier
+	must be 1.
+ - operating-points-v2: Phandle to the OPP table for the Power domain.
+	Refer to Documentation/devicetree/bindings/power/power_domain.txt
+	and Documentation/devicetree/bindings/opp/qcom-opp.txt for more details
+
+Example:
+
+	rpmpd: power-controller {
+		compatible = "qcom,msm8996-rpmpd";
+		#power-domain-cells = <1>;
+		operating-points-v2 = <&rpmpd_opp_table>;
+	};
+
+	rpmpd_opp_table: opp-table {
+		compatible = "operating-points-v2-qcom-level";
+
+		rpmpd_opp1: opp1 {
+			qcom,level = <1>;
+		};
+
+		rpmpd_opp2: opp2 {
+			qcom,level = <2>;
+		};
+
+		rpmpd_opp3: opp3 {
+			qcom,level = <3>;
+		};
+
+		rpmpd_opp4: opp4 {
+			qcom,level = <4>;
+		};
+
+		rpmpd_opp5: opp5 {
+			qcom,level = <5>;
+		};
+
+		rpmpd_opp6: opp6 {
+			qcom,level = <6>;
+		};
+	};
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v3 2/7] soc: qcom: rpmpd: Add a Power domain driver to model corners

[Rajendra Nayak]

The Power domains for corners just pass the performance state set by the
consumers to the RPM (Remote Power manager) which then takes care
of setting the appropriate voltage on the corresponding rails to
meet the performance needs.

We add all Power domain data needed on msm8996 here. This driver can easily
be extended by adding data for other qualcomm SoCs as well.

Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/soc/qcom/Kconfig               |   9 +
 drivers/soc/qcom/Makefile              |   1 +
 drivers/soc/qcom/rpmpd.c               | 301 +++++++++++++++++++++++++
 include/dt-bindings/power/qcom-rpmpd.h |  16 ++
 4 files changed, 327 insertions(+)
 create mode 100644 drivers/soc/qcom/rpmpd.c
 create mode 100644 include/dt-bindings/power/qcom-rpmpd.h

diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index 9dc02f390ba3..5c54931a7b99 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -74,6 +74,15 @@ config QCOM_RMTFS_MEM
 
 	  Say y here if you intend to boot the modem remoteproc.
 
+config QCOM_RPMPD
+	tristate "Qualcomm RPM Power domain driver"
+	depends on MFD_QCOM_RPM && QCOM_SMD_RPM
+	help
+	  QCOM RPM Power domain driver to support power-domains with
+	  performance states. The driver communicates a performance state
+	  value to RPM which then translates it into corresponding voltage
+	  for the voltage rail.
+
 config QCOM_SMEM
 	tristate "Qualcomm Shared Memory Manager (SMEM)"
 	depends on ARCH_QCOM
diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index 19dcf957cb3a..9550c170de93 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -15,3 +15,4 @@ obj-$(CONFIG_QCOM_SMP2P)	+= smp2p.o
 obj-$(CONFIG_QCOM_SMSM)	+= smsm.o
 obj-$(CONFIG_QCOM_WCNSS_CTRL) += wcnss_ctrl.o
 obj-$(CONFIG_QCOM_APR) += apr.o
+obj-$(CONFIG_QCOM_RPMPD) += rpmpd.o
diff --git a/drivers/soc/qcom/rpmpd.c b/drivers/soc/qcom/rpmpd.c
new file mode 100644
index 000000000000..c5e7a91f278c
--- /dev/null
+++ b/drivers/soc/qcom/rpmpd.c
@@ -0,0 +1,301 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2017-2018, The Linux Foundation. All rights reserved. */
+
+#include <linux/err.h>
+#include <linux/export.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/pm_domain.h>
+#include <linux/mfd/qcom_rpm.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/soc/qcom/smd-rpm.h>
+
+#include <dt-bindings/mfd/qcom-rpm.h>
+#include <dt-bindings/power/qcom-rpmpd.h>
+
+#define domain_to_rpmpd(domain) container_of(domain, struct rpmpd, pd)
+
+/* Resource types */
+#define RPMPD_SMPA 0x61706d73
+#define RPMPD_LDOA 0x616f646c
+
+/* Operation Keys */
+#define KEY_CORNER		0x6e726f63 /* corn */
+#define KEY_ENABLE		0x6e657773 /* swen */
+#define KEY_FLOOR_CORNER	0x636676   /* vfc */
+
+#define DEFINE_RPMPD_CORN_SMPA(_platform, _name, _active, r_id)		\
+	static struct rpmpd _platform##_##_active;			\
+	static struct rpmpd _platform##_##_name = {			\
+		.pd = {	.name = #_name,	},				\
+		.peer = &_platform##_##_active,				\
+		.res_type = RPMPD_SMPA,					\
+		.res_id = r_id,						\
+		.key = KEY_CORNER,					\
+	};								\
+	static struct rpmpd _platform##_##_active = {			\
+		.pd = { .name = #_active, },				\
+		.peer = &_platform##_##_name,				\
+		.active_only = true,					\
+		.res_type = RPMPD_SMPA,					\
+		.res_id = r_id,						\
+		.key = KEY_CORNER,					\
+	}
+
+#define DEFINE_RPMPD_CORN_LDOA(_platform, _name, r_id)			\
+	static struct rpmpd _platform##_##_name = {			\
+		.pd = { .name = #_name, },				\
+		.res_type = RPMPD_LDOA,					\
+		.res_id = r_id,						\
+		.key = KEY_CORNER,					\
+	}
+
+#define DEFINE_RPMPD_VFC(_platform, _name, r_id, r_type)		\
+	static struct rpmpd _platform##_##_name = {			\
+		.pd = { .name = #_name, },				\
+		.res_type = r_type,					\
+		.res_id = r_id,						\
+		.key = KEY_FLOOR_CORNER,				\
+	}
+
+#define DEFINE_RPMPD_VFC_SMPA(_platform, _name, r_id)			\
+	DEFINE_RPMPD_VFC(_platform, _name, r_id, RPMPD_SMPA)
+
+#define DEFINE_RPMPD_VFC_LDOA(_platform, _name, r_id)			\
+	DEFINE_RPMPD_VFC(_platform, _name, r_id, RPMPD_LDOA)
+
+struct rpmpd_req {
+	__le32 key;
+	__le32 nbytes;
+	__le32 value;
+};
+
+struct rpmpd {
+	struct generic_pm_domain pd;
+	struct rpmpd *peer;
+	const bool active_only;
+	unsigned int corner;
+	bool enabled;
+	const char *res_name;
+	const int res_type;
+	const int res_id;
+	struct qcom_smd_rpm *rpm;
+	__le32 key;
+};
+
+struct rpmpd_desc {
+	struct rpmpd **rpmpds;
+	size_t num_pds;
+};
+
+static DEFINE_MUTEX(rpmpd_lock);
+
+/* msm8996 RPM Power domains */
+DEFINE_RPMPD_CORN_SMPA(msm8996, vddcx, vddcx_ao, 1);
+DEFINE_RPMPD_CORN_SMPA(msm8996, vddmx, vddmx_ao, 2);
+DEFINE_RPMPD_CORN_LDOA(msm8996, vddsscx, 26);
+
+DEFINE_RPMPD_VFC_SMPA(msm8996, vddcx_vfc, 1);
+DEFINE_RPMPD_VFC_LDOA(msm8996, vddsscx_vfc, 26);
+
+static struct rpmpd *msm8996_rpmpds[] = {
+	[MSM8996_VDDCX] =	&msm8996_vddcx,
+	[MSM8996_VDDCX_AO] =	&msm8996_vddcx_ao,
+	[MSM8996_VDDCX_VFC] =	&msm8996_vddcx_vfc,
+	[MSM8996_VDDMX] =	&msm8996_vddmx,
+	[MSM8996_VDDMX_AO] =	&msm8996_vddmx_ao,
+	[MSM8996_VDDSSCX] =	&msm8996_vddsscx,
+	[MSM8996_VDDSSCX_VFC] =	&msm8996_vddsscx_vfc,
+};
+
+static const struct rpmpd_desc msm8996_desc = {
+	.rpmpds = msm8996_rpmpds,
+	.num_pds = ARRAY_SIZE(msm8996_rpmpds),
+};
+
+static const struct of_device_id rpmpd_match_table[] = {
+	{ .compatible = "qcom,msm8996-rpmpd", .data = &msm8996_desc },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, rpmpd_match_table);
+
+static int rpmpd_send_enable(struct rpmpd *pd, bool enable)
+{
+	struct rpmpd_req req = {
+		.key = KEY_ENABLE,
+		.nbytes = cpu_to_le32(sizeof(u32)),
+		.value = cpu_to_le32(enable),
+	};
+
+	return qcom_rpm_smd_write(pd->rpm, QCOM_RPM_ACTIVE_STATE, pd->res_type,
+				  pd->res_id, &req, sizeof(req));
+}
+
+static int rpmpd_send_corner(struct rpmpd *pd, int state, unsigned int corner)
+{
+	struct rpmpd_req req = {
+		.key = pd->key,
+		.nbytes = cpu_to_le32(sizeof(u32)),
+		.value = cpu_to_le32(corner),
+	};
+
+	return qcom_rpm_smd_write(pd->rpm, state, pd->res_type, pd->res_id,
+				  &req, sizeof(req));
+};
+
+static void to_active_sleep(struct rpmpd *pd, unsigned int corner,
+			    unsigned int *active, unsigned int *sleep)
+{
+	*active = corner;
+
+	if (pd->active_only)
+		*sleep = 0;
+	else
+		*sleep = *active;
+}
+
+static int rpmpd_aggregate_corner(struct rpmpd *pd)
+{
+	int ret;
+	struct rpmpd *peer = pd->peer;
+	unsigned int active_corner, sleep_corner;
+	unsigned int this_active_corner = 0, this_sleep_corner = 0;
+	unsigned int peer_active_corner = 0, peer_sleep_corner = 0;
+
+	to_active_sleep(pd, pd->corner, &this_active_corner, &this_sleep_corner);
+
+	if (peer && peer->enabled)
+		to_active_sleep(peer, peer->corner, &peer_active_corner,
+				&peer_sleep_corner);
+
+	active_corner = max(this_active_corner, peer_active_corner);
+
+	ret = rpmpd_send_corner(pd, QCOM_RPM_ACTIVE_STATE, active_corner);
+	if (ret)
+		return ret;
+
+	sleep_corner = max(this_sleep_corner, peer_sleep_corner);
+
+	return rpmpd_send_corner(pd, QCOM_RPM_SLEEP_STATE, sleep_corner);
+}
+
+static int rpmpd_power_on(struct generic_pm_domain *domain)
+{
+	int ret;
+	struct rpmpd *pd = domain_to_rpmpd(domain);
+
+	mutex_lock(&rpmpd_lock);
+
+	ret = rpmpd_send_enable(pd, true);
+	if (ret)
+		goto out;
+
+	pd->enabled = true;
+
+	if (pd->corner)
+		ret = rpmpd_aggregate_corner(pd);
+
+out:
+	mutex_unlock(&rpmpd_lock);
+
+	return ret;
+}
+
+static int rpmpd_power_off(struct generic_pm_domain *domain)
+{
+	int ret;
+	struct rpmpd *pd = domain_to_rpmpd(domain);
+
+	mutex_lock(&rpmpd_lock);
+
+	ret = rpmpd_send_enable(pd, false);
+	if (!ret)
+		pd->enabled = false;
+
+	mutex_unlock(&rpmpd_lock);
+
+	return ret;
+}
+
+static int rpmpd_probe(struct platform_device *pdev)
+{
+	int i;
+	size_t num;
+	struct genpd_onecell_data *data;
+	struct qcom_smd_rpm *rpm;
+	struct rpmpd **rpmpds;
+	const struct rpmpd_desc *desc;
+
+	rpm = dev_get_drvdata(pdev->dev.parent);
+	if (!rpm) {
+		dev_err(&pdev->dev, "Unable to retrieve handle to RPM\n");
+		return -ENODEV;
+	}
+
+	desc = of_device_get_match_data(&pdev->dev);
+	if (!desc)
+		return -EINVAL;
+
+	rpmpds = desc->rpmpds;
+	num = desc->num_pds;
+
+	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->domains = devm_kcalloc(&pdev->dev, num, sizeof(*data->domains),
+				     GFP_KERNEL);
+	data->num_domains = num;
+
+	for (i = 0; i < num; i++) {
+		if (!rpmpds[i]) {
+			dev_warn(&pdev->dev, "rpmpds[] with empty entry at index=%d\n",
+				 i);
+			continue;
+		}
+
+		rpmpds[i]->rpm = rpm;
+		rpmpds[i]->pd.power_off = rpmpd_power_off;
+		rpmpds[i]->pd.power_on = rpmpd_power_on;
+		pm_genpd_init(&rpmpds[i]->pd, NULL, true);
+
+		data->domains[i] = &rpmpds[i]->pd;
+	}
+
+	return of_genpd_add_provider_onecell(pdev->dev.of_node, data);
+}
+
+static int rpmpd_remove(struct platform_device *pdev)
+{
+	of_genpd_del_provider(pdev->dev.of_node);
+	return 0;
+}
+
+static struct platform_driver rpmpd_driver = {
+	.driver = {
+		.name = "qcom-rpmpd",
+		.of_match_table = rpmpd_match_table,
+	},
+	.probe = rpmpd_probe,
+	.remove = rpmpd_remove,
+};
+
+static int __init rpmpd_init(void)
+{
+	return platform_driver_register(&rpmpd_driver);
+}
+core_initcall(rpmpd_init);
+
+static void __exit rpmpd_exit(void)
+{
+	platform_driver_unregister(&rpmpd_driver);
+}
+module_exit(rpmpd_exit);
+
+MODULE_DESCRIPTION("Qualcomm Technologies, Inc. RPM Power Domain Driver");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:qcom-rpmpd");
diff --git a/include/dt-bindings/power/qcom-rpmpd.h b/include/dt-bindings/power/qcom-rpmpd.h
new file mode 100644
index 000000000000..59f47317b81f
--- /dev/null
+++ b/include/dt-bindings/power/qcom-rpmpd.h
@@ -0,0 +1,16 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright (c) 2018, The Linux Foundation. All rights reserved. */
+
+#ifndef _DT_BINDINGS_POWER_QCOM_RPMPD_H
+#define _DT_BINDINGS_POWER_QCOM_RPMPD_H
+
+/* MSM8996 Power Domain Indexes */
+#define MSM8996_VDDCX		0
+#define MSM8996_VDDCX_AO	1
+#define MSM8996_VDDCX_VFC	2
+#define MSM8996_VDDMX		3
+#define MSM8996_VDDMX_AO	4
+#define MSM8996_VDDSSCX		5
+#define MSM8996_VDDSSCX_VFC	6
+
+#endif
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v3 2/7] soc: qcom: rpmpd: Add a Power domain driver to model corners

[Ulf Hansson]

[...]

> +static int rpmpd_probe(struct platform_device *pdev)
> +{
> +       int i;
> +       size_t num;
> +       struct genpd_onecell_data *data;
> +       struct qcom_smd_rpm *rpm;
> +       struct rpmpd **rpmpds;
> +       const struct rpmpd_desc *desc;
> +
> +       rpm = dev_get_drvdata(pdev->dev.parent);
> +       if (!rpm) {
> +               dev_err(&pdev->dev, "Unable to retrieve handle to RPM\n");
> +               return -ENODEV;
> +       }
> +
> +       desc = of_device_get_match_data(&pdev->dev);
> +       if (!desc)
> +               return -EINVAL;
> +
> +       rpmpds = desc->rpmpds;
> +       num = desc->num_pds;
> +
> +       data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> +       if (!data)
> +               return -ENOMEM;
> +
> +       data->domains = devm_kcalloc(&pdev->dev, num, sizeof(*data->domains),
> +                                    GFP_KERNEL);
> +       data->num_domains = num;
> +
> +       for (i = 0; i < num; i++) {
> +               if (!rpmpds[i]) {
> +                       dev_warn(&pdev->dev, "rpmpds[] with empty entry at index=%d\n",
> +                                i);
> +                       continue;
> +               }
> +
> +               rpmpds[i]->rpm = rpm;
> +               rpmpds[i]->pd.power_off = rpmpd_power_off;
> +               rpmpds[i]->pd.power_on = rpmpd_power_on;
> +               pm_genpd_init(&rpmpds[i]->pd, NULL, true);
> +
> +               data->domains[i] = &rpmpds[i]->pd;
> +       }
> +
> +       return of_genpd_add_provider_onecell(pdev->dev.of_node, data);
> +}
> +
> +static int rpmpd_remove(struct platform_device *pdev)
> +{
> +       of_genpd_del_provider(pdev->dev.of_node);

In principle what you need, additionally to the above, is something like this:

while (!IS_ERR(of_genpd_remove_last(pdev->dev.of_node));

Then in the ->probe() function, use dev_set_drvdata() to store the
pointers for the allocated data, such you can use dev_get_drvdata() to
get them back here to kfree() them.

However, running the ->remove() path for this driver doesn't really
make sense. As we can't remove a genpd that has devices attached to it
anyways.

Perhaps make this a built in driver, if possible?

[...]

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

Kind regards
Uffe

Re: [PATCH v3 2/7] soc: qcom: rpmpd: Add a Power domain driver to model corners

[Rob Herring]

On Tue, Jun 12, 2018 at 10:10:47AM +0530, Rajendra Nayak wrote:
> The Power domains for corners just pass the performance state set by the
> consumers to the RPM (Remote Power manager) which then takes care
> of setting the appropriate voltage on the corresponding rails to
> meet the performance needs.
> 
> We add all Power domain data needed on msm8996 here. This driver can easily
> be extended by adding data for other qualcomm SoCs as well.
> 
> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/soc/qcom/Kconfig               |   9 +
>  drivers/soc/qcom/Makefile              |   1 +
>  drivers/soc/qcom/rpmpd.c               | 301 +++++++++++++++++++++++++

>  include/dt-bindings/power/qcom-rpmpd.h |  16 ++

Acked-by: Rob Herring <robh@kernel.org>

>  4 files changed, 327 insertions(+)
>  create mode 100644 drivers/soc/qcom/rpmpd.c
>  create mode 100644 include/dt-bindings/power/qcom-rpmpd.h

[PATCH v3 3/7] soc: qcom: rpmpd: Add support for get/set performance state

[Rajendra Nayak]

Add support for the .set_performace_state() and .opp_to_performance_state()
callbacks in the rpmpd driver.

Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/soc/qcom/rpmpd.c | 46 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/drivers/soc/qcom/rpmpd.c b/drivers/soc/qcom/rpmpd.c
index c5e7a91f278c..53209454c1f3 100644
--- a/drivers/soc/qcom/rpmpd.c
+++ b/drivers/soc/qcom/rpmpd.c
@@ -12,6 +12,7 @@
 #include <linux/of.h>
 #include <linux/of_device.h>
 #include <linux/platform_device.h>
+#include <linux/pm_opp.h>
 #include <linux/soc/qcom/smd-rpm.h>
 
 #include <dt-bindings/mfd/qcom-rpm.h>
@@ -28,6 +29,8 @@
 #define KEY_ENABLE		0x6e657773 /* swen */
 #define KEY_FLOOR_CORNER	0x636676   /* vfc */
 
+#define MAX_RPMPD_STATE		6
+
 #define DEFINE_RPMPD_CORN_SMPA(_platform, _name, _active, r_id)		\
 	static struct rpmpd _platform##_##_active;			\
 	static struct rpmpd _platform##_##_name = {			\
@@ -221,6 +224,47 @@ static int rpmpd_power_off(struct generic_pm_domain *domain)
 	return ret;
 }
 
+static int rpmpd_set_performance(struct generic_pm_domain *domain,
+				 unsigned int state)
+{
+	int ret = 0;
+	struct rpmpd *pd = domain_to_rpmpd(domain);
+
+	mutex_lock(&rpmpd_lock);
+
+	if (state > MAX_RPMPD_STATE)
+		goto out;
+
+	pd->corner = state;
+
+	if (!pd->enabled && (pd->key != KEY_FLOOR_CORNER))
+		goto out;
+
+	ret = rpmpd_aggregate_corner(pd);
+
+out:
+	mutex_unlock(&rpmpd_lock);
+
+	return ret;
+}
+
+static unsigned int rpmpd_get_performance(struct generic_pm_domain *genpd,
+					  struct dev_pm_opp *opp)
+{
+	struct device_node *np;
+	unsigned int corner = 0;
+
+	np = dev_pm_opp_get_of_node(opp);
+	if (of_property_read_u32(np, "qcom,level", &corner)) {
+		pr_err("%s: missing 'qcom,level' property\n", __func__);
+		return 0;
+	}
+
+	of_node_put(np);
+
+	return corner;
+}
+
 static int rpmpd_probe(struct platform_device *pdev)
 {
 	int i;
@@ -261,6 +305,8 @@ static int rpmpd_probe(struct platform_device *pdev)
 		rpmpds[i]->rpm = rpm;
 		rpmpds[i]->pd.power_off = rpmpd_power_off;
 		rpmpds[i]->pd.power_on = rpmpd_power_on;
+		rpmpds[i]->pd.set_performance_state = rpmpd_set_performance;
+		rpmpds[i]->pd.opp_to_performance_state = rpmpd_get_performance;
 		pm_genpd_init(&rpmpds[i]->pd, NULL, true);
 
 		data->domains[i] = &rpmpds[i]->pd;
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v3 4/7] arm64: dts: msm8996: Add rpmpd device node

[Rajendra Nayak]

Add rpmpd device node and its OPP table

Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 arch/arm64/boot/dts/qcom/msm8996.dtsi | 34 +++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/msm8996.dtsi b/arch/arm64/boot/dts/qcom/msm8996.dtsi
index 8c7f9ca25b53..404a08630ccd 100644
--- a/arch/arm64/boot/dts/qcom/msm8996.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8996.dtsi
@@ -308,6 +308,40 @@
 				#clock-cells = <1>;
 			};
 
+			rpmpd: power-controller {
+				compatible = "qcom,msm8996-rpmpd";
+				#power-domain-cells = <1>;
+				operating-points-v2 = <&rpmpd_opp_table>;
+			};
+
+			rpmpd_opp_table: opp-table {
+				compatible = "operating-points-v2-qcom-level";
+
+				rpmpd_opp1: opp1 {
+					qcom,level = <1>;
+				};
+
+				rpmpd_opp2: opp2 {
+					qcom,level = <2>;
+				};
+
+				rpmpd_opp3: opp3 {
+					qcom,level = <3>;
+				};
+
+				rpmpd_opp4: opp4 {
+					qcom,level = <4>;
+				};
+
+				rpmpd_opp5: opp5 {
+					qcom,level = <5>;
+				};
+
+				rpmpd_opp6: opp6 {
+					qcom,level = <6>;
+				};
+			};
+
 			pm8994-regulators {
 				compatible = "qcom,rpm-pm8994-regulators";
 
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v3 5/7] dt-bindings: power: Add qcom rpmh power domain driver bindings

[Rajendra Nayak]

Add DT bindings to describe the rpmh powerdomains found on Qualcomm
Technologies, Inc. SoCs. These power domains communicate a performance
state to RPMh, which then translates it into corresponding voltage on
a PMIC rail.

Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
---
 .../devicetree/bindings/power/qcom,rpmhpd.txt | 65 +++++++++++++++++++
 1 file changed, 65 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/qcom,rpmhpd.txt

diff --git a/Documentation/devicetree/bindings/power/qcom,rpmhpd.txt b/Documentation/devicetree/bindings/power/qcom,rpmhpd.txt
new file mode 100644
index 000000000000..41ef7afa6b24
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/qcom,rpmhpd.txt
@@ -0,0 +1,65 @@
+Qualcomm RPMh Power domains
+
+For RPMh Power domains, we communicate a performance state to RPMh
+which then translates it into a corresponding voltage on a rail
+
+Required Properties:
+ - compatible: Should be one of the following
+	* qcom,sdm845-rpmhpd: RPMh Power domain for the sdm845 family of SoC
+ - power-domain-cells: number of cells in power domain specifier
+	must be 1
+ - operating-points-v2: Phandle to the OPP table for the power-domain.
+	Refer to Documentation/devicetree/bindings/power/power_domain.txt
+	and Documentation/devicetree/bindings/opp/qcom-opp.txt for more details
+
+Example:
+
+	rpmhpd: power-controller {
+		compatible = "qcom,sdm845-rpmhpd";
+		#power-domain-cells = <1>;
+		operating-points-v2 = <&rpmhpd_opp_table>;
+	};
+
+	rpmhpd_opp_table: opp-table {
+		compatible = "operating-points-v2-qcom-level";
+
+		rpmhpd_opp_ret: opp1 {
+			qcom-level = <16>;
+		};
+
+		rpmhpd_opp_min_svs: opp2 {
+			qcom-level = <48>;
+		};
+
+		rpmhpd_opp_low_svs: opp3 {
+			qcom-level = <64>;
+		};
+
+		rpmhpd_opp_svs: opp4 {
+			qcom-level = <128>;
+		};
+
+		rpmhpd_opp_svs_l1: opp5 {
+			qcom-level = <192>;
+		};
+
+		rpmhpd_opp_nom: opp6 {
+			qcom-level = <256>;
+		};
+
+		rpmhpd_opp_nom_l1: opp7 {
+			qcom-level = <320>;
+		};
+
+		rpmhpd_opp_nom_l2: opp8 {
+			qcom-level = <336>;
+		};
+
+		rpmhpd_opp_turbo: opp9 {
+			qcom-level = <384>;
+		};
+
+		rpmhpd_opp_turbo_l1: opp10 {
+			qcom-level = <416>;
+		};
+	};
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v3 5/7] dt-bindings: power: Add qcom rpmh power domain driver bindings

[Bjorn Andersson]

On Mon 11 Jun 21:40 PDT 2018, Rajendra Nayak wrote:

> Add DT bindings to describe the rpmh powerdomains found on Qualcomm
> Technologies, Inc. SoCs. These power domains communicate a performance
> state to RPMh, which then translates it into corresponding voltage on
> a PMIC rail.
> 
> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
> ---
>  .../devicetree/bindings/power/qcom,rpmhpd.txt | 65 +++++++++++++++++++
>  1 file changed, 65 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power/qcom,rpmhpd.txt
> 
> diff --git a/Documentation/devicetree/bindings/power/qcom,rpmhpd.txt b/Documentation/devicetree/bindings/power/qcom,rpmhpd.txt
> new file mode 100644
> index 000000000000..41ef7afa6b24
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/qcom,rpmhpd.txt
> @@ -0,0 +1,65 @@
> +Qualcomm RPMh Power domains
> +
> +For RPMh Power domains, we communicate a performance state to RPMh
> +which then translates it into a corresponding voltage on a rail
> +
> +Required Properties:
> + - compatible: Should be one of the following
> +	* qcom,sdm845-rpmhpd: RPMh Power domain for the sdm845 family of SoC

Afaict this binding is identical to the one introduced in patch 1, so I
would suggest that you just add the compatible there.

Regards,
Bjorn

> + - power-domain-cells: number of cells in power domain specifier
> +	must be 1
> + - operating-points-v2: Phandle to the OPP table for the power-domain.
> +	Refer to Documentation/devicetree/bindings/power/power_domain.txt
> +	and Documentation/devicetree/bindings/opp/qcom-opp.txt for more details
> +
> +Example:
> +
> +	rpmhpd: power-controller {
> +		compatible = "qcom,sdm845-rpmhpd";
> +		#power-domain-cells = <1>;
> +		operating-points-v2 = <&rpmhpd_opp_table>;
> +	};
> +
> +	rpmhpd_opp_table: opp-table {
> +		compatible = "operating-points-v2-qcom-level";
> +
> +		rpmhpd_opp_ret: opp1 {
> +			qcom-level = <16>;
> +		};
> +
> +		rpmhpd_opp_min_svs: opp2 {
> +			qcom-level = <48>;
> +		};
> +
> +		rpmhpd_opp_low_svs: opp3 {
> +			qcom-level = <64>;
> +		};
> +
> +		rpmhpd_opp_svs: opp4 {
> +			qcom-level = <128>;
> +		};
> +
> +		rpmhpd_opp_svs_l1: opp5 {
> +			qcom-level = <192>;
> +		};
> +
> +		rpmhpd_opp_nom: opp6 {
> +			qcom-level = <256>;
> +		};
> +
> +		rpmhpd_opp_nom_l1: opp7 {
> +			qcom-level = <320>;
> +		};
> +
> +		rpmhpd_opp_nom_l2: opp8 {
> +			qcom-level = <336>;
> +		};
> +
> +		rpmhpd_opp_turbo: opp9 {
> +			qcom-level = <384>;
> +		};
> +
> +		rpmhpd_opp_turbo_l1: opp10 {
> +			qcom-level = <416>;
> +		};
> +	};
> -- 
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation
> 

Re: [PATCH v3 5/7] dt-bindings: power: Add qcom rpmh power domain driver bindings

[Rajendra Nayak]

On 06/12/2018 11:09 AM, Bjorn Andersson wrote:
> On Mon 11 Jun 21:40 PDT 2018, Rajendra Nayak wrote:
> 
>> Add DT bindings to describe the rpmh powerdomains found on Qualcomm
>> Technologies, Inc. SoCs. These power domains communicate a performance
>> state to RPMh, which then translates it into corresponding voltage on
>> a PMIC rail.
>>
>> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
>> ---
>>  .../devicetree/bindings/power/qcom,rpmhpd.txt | 65 +++++++++++++++++++
>>  1 file changed, 65 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/power/qcom,rpmhpd.txt
>>
>> diff --git a/Documentation/devicetree/bindings/power/qcom,rpmhpd.txt b/Documentation/devicetree/bindings/power/qcom,rpmhpd.txt
>> new file mode 100644
>> index 000000000000..41ef7afa6b24
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/power/qcom,rpmhpd.txt
>> @@ -0,0 +1,65 @@
>> +Qualcomm RPMh Power domains
>> +
>> +For RPMh Power domains, we communicate a performance state to RPMh
>> +which then translates it into a corresponding voltage on a rail
>> +
>> +Required Properties:
>> + - compatible: Should be one of the following
>> +	* qcom,sdm845-rpmhpd: RPMh Power domain for the sdm845 family of SoC
> 
> Afaict this binding is identical to the one introduced in patch 1, so I
> would suggest that you just add the compatible there.

Sure, makes sense. thanks.

> 
> Regards,
> Bjorn
> 
>> + - power-domain-cells: number of cells in power domain specifier
>> +	must be 1
>> + - operating-points-v2: Phandle to the OPP table for the power-domain.
>> +	Refer to Documentation/devicetree/bindings/power/power_domain.txt
>> +	and Documentation/devicetree/bindings/opp/qcom-opp.txt for more details
>> +
>> +Example:
>> +
>> +	rpmhpd: power-controller {
>> +		compatible = "qcom,sdm845-rpmhpd";
>> +		#power-domain-cells = <1>;
>> +		operating-points-v2 = <&rpmhpd_opp_table>;
>> +	};
>> +
>> +	rpmhpd_opp_table: opp-table {
>> +		compatible = "operating-points-v2-qcom-level";
>> +
>> +		rpmhpd_opp_ret: opp1 {
>> +			qcom-level = <16>;
>> +		};
>> +
>> +		rpmhpd_opp_min_svs: opp2 {
>> +			qcom-level = <48>;
>> +		};
>> +
>> +		rpmhpd_opp_low_svs: opp3 {
>> +			qcom-level = <64>;
>> +		};
>> +
>> +		rpmhpd_opp_svs: opp4 {
>> +			qcom-level = <128>;
>> +		};
>> +
>> +		rpmhpd_opp_svs_l1: opp5 {
>> +			qcom-level = <192>;
>> +		};
>> +
>> +		rpmhpd_opp_nom: opp6 {
>> +			qcom-level = <256>;
>> +		};
>> +
>> +		rpmhpd_opp_nom_l1: opp7 {
>> +			qcom-level = <320>;
>> +		};
>> +
>> +		rpmhpd_opp_nom_l2: opp8 {
>> +			qcom-level = <336>;
>> +		};
>> +
>> +		rpmhpd_opp_turbo: opp9 {
>> +			qcom-level = <384>;
>> +		};
>> +
>> +		rpmhpd_opp_turbo_l1: opp10 {
>> +			qcom-level = <416>;
>> +		};
>> +	};
>> -- 
>> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
>> of Code Aurora Forum, hosted by The Linux Foundation
>>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH v3 5/7] dt-bindings: power: Add qcom rpmh power domain driver bindings

[David Collins]

Hello Rajendra,

On 06/11/2018 09:40 PM, Rajendra Nayak wrote:
> Add DT bindings to describe the rpmh powerdomains found on Qualcomm

s/powerdomains/power domains/

> Technologies, Inc. SoCs. These power domains communicate a performance
> state to RPMh, which then translates it into corresponding voltage on
> a PMIC rail.
> 
> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
> ---
>  .../devicetree/bindings/power/qcom,rpmhpd.txt | 65 +++++++++++++++++++
>  1 file changed, 65 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power/qcom,rpmhpd.txt

include/dt-bindings/power/qcom-rpmhpd.h from patch 6/7 should be moved to
this patch.

> 
> diff --git a/Documentation/devicetree/bindings/power/qcom,rpmhpd.txt b/Documentation/devicetree/bindings/power/qcom,rpmhpd.txt
> new file mode 100644
> index 000000000000..41ef7afa6b24
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/qcom,rpmhpd.txt
> @@ -0,0 +1,65 @@
> +Qualcomm RPMh Power domains
> +
> +For RPMh Power domains, we communicate a performance state to RPMh
> +which then translates it into a corresponding voltage on a rail
> +
> +Required Properties:
> + - compatible: Should be one of the following
> +	* qcom,sdm845-rpmhpd: RPMh Power domain for the sdm845 family of SoC
> + - power-domain-cells: number of cells in power domain specifier
> +	must be 1
> + - operating-points-v2: Phandle to the OPP table for the power-domain.
> +	Refer to Documentation/devicetree/bindings/power/power_domain.txt
> +	and Documentation/devicetree/bindings/opp/qcom-opp.txt for more details

Could you please mention here that qcom,level properties in the associated
opp-table should use the RPMH_REGULATOR_LEVEL_* constants?  RPMh ARC
resources depend upon the RPMH_REGULATOR_LEVEL_* constants to provide a
mapping of levels supported by hardware.

> +Example:

Could you please add this here?

#include <dt-bindings/power/qcom-rpmhpd.h>

> +
> +	rpmhpd: power-controller {
> +		compatible = "qcom,sdm845-rpmhpd";
> +		#power-domain-cells = <1>;
> +		operating-points-v2 = <&rpmhpd_opp_table>;
> +	};
> +
> +	rpmhpd_opp_table: opp-table {
> +		compatible = "operating-points-v2-qcom-level";
> +
> +		rpmhpd_opp_ret: opp1 {
> +			qcom-level = <16>;

As per qcom-opp.txt, 'qcom,level' should be used, not 'qcom-level'.

Where is the qcom-opp.txt patch?  It isn't part of the v3 patch series but
was in the v2 series [1].

Could you please change this to be the following?

    qcom,level = <RPMH_REGULATOR_LEVEL_RETENTION>;

Also, please use the level constants for all other subnodes in this
example as well.

> +		};
> +
> +		rpmhpd_opp_min_svs: opp2 {
> +			qcom-level = <48>;
> +		};
> +
> +		rpmhpd_opp_low_svs: opp3 {
> +			qcom-level = <64>;
> +		};
> +
> +		rpmhpd_opp_svs: opp4 {
> +			qcom-level = <128>;
> +		};
> +
> +		rpmhpd_opp_svs_l1: opp5 {
> +			qcom-level = <192>;
> +		};
> +
> +		rpmhpd_opp_nom: opp6 {
> +			qcom-level = <256>;
> +		};
> +
> +		rpmhpd_opp_nom_l1: opp7 {
> +			qcom-level = <320>;
> +		};
> +
> +		rpmhpd_opp_nom_l2: opp8 {
> +			qcom-level = <336>;
> +		};
> +
> +		rpmhpd_opp_turbo: opp9 {
> +			qcom-level = <384>;
> +		};
> +
> +		rpmhpd_opp_turbo_l1: opp10 {
> +			qcom-level = <416>;
> +		};
> +	};

Could you please add an example consumer DT node as well which uses
"SDM845 Power Domain Indexes" from qcom-rpmhpd.h?  It isn't clear how a
specific power domain (e.g. SDM845_CX) is specified from the consumer
side.  It also isn't clear how the consumer specifies a mapping for the
power domain levels that it will be using.

Thanks,
David

[1]: https://lkml.org/lkml/2018/5/25/210

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

Re: [PATCH v3 5/7] dt-bindings: power: Add qcom rpmh power domain driver bindings

[Rajendra Nayak]

Hi David,

On 06/14/2018 03:42 AM, David Collins wrote:
> Hello Rajendra,
> 
> On 06/11/2018 09:40 PM, Rajendra Nayak wrote:
>> Add DT bindings to describe the rpmh powerdomains found on Qualcomm
> 
> s/powerdomains/power domains/
> 
>> Technologies, Inc. SoCs. These power domains communicate a performance
>> state to RPMh, which then translates it into corresponding voltage on
>> a PMIC rail.
>>
>> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
>> ---
>>  .../devicetree/bindings/power/qcom,rpmhpd.txt | 65 +++++++++++++++++++
>>  1 file changed, 65 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/power/qcom,rpmhpd.txt
> 
> include/dt-bindings/power/qcom-rpmhpd.h from patch 6/7 should be moved to
> this patch.

right, Rob mentioned this too, I will move it in v4.

> 
>>
>> diff --git a/Documentation/devicetree/bindings/power/qcom,rpmhpd.txt b/Documentation/devicetree/bindings/power/qcom,rpmhpd.txt
>> new file mode 100644
>> index 000000000000..41ef7afa6b24
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/power/qcom,rpmhpd.txt
>> @@ -0,0 +1,65 @@
>> +Qualcomm RPMh Power domains
>> +
>> +For RPMh Power domains, we communicate a performance state to RPMh
>> +which then translates it into a corresponding voltage on a rail
>> +
>> +Required Properties:
>> + - compatible: Should be one of the following
>> +	* qcom,sdm845-rpmhpd: RPMh Power domain for the sdm845 family of SoC
>> + - power-domain-cells: number of cells in power domain specifier
>> +	must be 1
>> + - operating-points-v2: Phandle to the OPP table for the power-domain.
>> +	Refer to Documentation/devicetree/bindings/power/power_domain.txt
>> +	and Documentation/devicetree/bindings/opp/qcom-opp.txt for more details
> 
> Could you please mention here that qcom,level properties in the associated
> opp-table should use the RPMH_REGULATOR_LEVEL_* constants?  RPMh ARC
> resources depend upon the RPMH_REGULATOR_LEVEL_* constants to provide a
> mapping of levels supported by hardware.
> 
>> +Example:
> 
> Could you please add this here?
> 
> #include <dt-bindings/power/qcom-rpmhpd.h>

I will, I wasn't sure its okay to reference a kernel include file in a DT
binding documentation. But looking around it seems like its common practice.

> 
>> +
>> +	rpmhpd: power-controller {
>> +		compatible = "qcom,sdm845-rpmhpd";
>> +		#power-domain-cells = <1>;
>> +		operating-points-v2 = <&rpmhpd_opp_table>;
>> +	};
>> +
>> +	rpmhpd_opp_table: opp-table {
>> +		compatible = "operating-points-v2-qcom-level";
>> +
>> +		rpmhpd_opp_ret: opp1 {
>> +			qcom-level = <16>;
> 
> As per qcom-opp.txt, 'qcom,level' should be used, not 'qcom-level'.

d'oh! I just keep getting this wrong.

> 
> Where is the qcom-opp.txt patch?  It isn't part of the v3 patch series but
> was in the v2 series [1].

Oops, looks like I accidentally dropped it in v3 :(

> 
> Could you please change this to be the following?
> 
>     qcom,level = <RPMH_REGULATOR_LEVEL_RETENTION>;
> 
> Also, please use the level constants for all other subnodes in this
> example as well.
> 
>> +		};
>> +
>> +		rpmhpd_opp_min_svs: opp2 {
>> +			qcom-level = <48>;
>> +		};
>> +
>> +		rpmhpd_opp_low_svs: opp3 {
>> +			qcom-level = <64>;
>> +		};
>> +
>> +		rpmhpd_opp_svs: opp4 {
>> +			qcom-level = <128>;
>> +		};
>> +
>> +		rpmhpd_opp_svs_l1: opp5 {
>> +			qcom-level = <192>;
>> +		};
>> +
>> +		rpmhpd_opp_nom: opp6 {
>> +			qcom-level = <256>;
>> +		};
>> +
>> +		rpmhpd_opp_nom_l1: opp7 {
>> +			qcom-level = <320>;
>> +		};
>> +
>> +		rpmhpd_opp_nom_l2: opp8 {
>> +			qcom-level = <336>;
>> +		};
>> +
>> +		rpmhpd_opp_turbo: opp9 {
>> +			qcom-level = <384>;
>> +		};
>> +
>> +		rpmhpd_opp_turbo_l1: opp10 {
>> +			qcom-level = <416>;
>> +		};
>> +	};
> 
> Could you please add an example consumer DT node as well which uses
> "SDM845 Power Domain Indexes" from qcom-rpmhpd.h?  It isn't clear how a
> specific power domain (e.g. SDM845_CX) is specified from the consumer
> side.  It also isn't clear how the consumer specifies a mapping for the
> power domain levels that it will be using.

I can add an example consumer with a power-domains property pointing to
the phandle and index (as is general practice)

For specifying the power domain levels, I am not quite sure what the approach
we would use. One way is for consumers to use OPP bindings, but that wasn't
liked by some and we now have plans to stuff it all within the clock driver code.
In which case I expect we would just maintain internal mapping tables for clock
frequencies/power domain levels so nothing comes in from DT, or maybe it will
come in from DT, i just don't know.

I can certainly describe the OPP way a consumer could map to a power domain level,
but I am not sure how the clock bindings if any would be at this point to handle this.

regards,
Rajendra

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH v3 5/7] dt-bindings: power: Add qcom rpmh power domain driver bindings

[Viresh Kumar]

On 14-06-18, 11:56, Rajendra Nayak wrote:
> On 06/14/2018 03:42 AM, David Collins wrote:
> > Could you please add an example consumer DT node as well which uses
> > "SDM845 Power Domain Indexes" from qcom-rpmhpd.h?  It isn't clear how a
> > specific power domain (e.g. SDM845_CX) is specified from the consumer
> > side.  It also isn't clear how the consumer specifies a mapping for the
> > power domain levels that it will be using.
> 
> I can add an example consumer with a power-domains property pointing to
> the phandle and index (as is general practice)
> 
> For specifying the power domain levels, I am not quite sure what the approach
> we would use. One way is for consumers to use OPP bindings, but that wasn't
> liked by some and we now have plans to stuff it all within the clock driver code.

Even in that case the information should come from DT somehow. So the consumer
doesn't need an OPP table for itself, but it can/should have the "required-opps"
property which points to an entry in the genpd OPP table.

> In which case I expect we would just maintain internal mapping tables for clock
> frequencies/power domain levels so nothing comes in from DT, or maybe it will
> come in from DT, i just don't know.
> 
> I can certainly describe the OPP way a consumer could map to a power domain level,
> but I am not sure how the clock bindings if any would be at this point to handle this.

-- 
viresh

[PATCH v3 6/7] soc: qcom: Add RPMh Power domain driver

[Rajendra Nayak]

The RPMh Power domain driver aggregates the corner votes from various
consumers for the ARC resources and communicates it to RPMh.

We also add data for all power domains on sdm845 SoC as part of the patch.
The driver can be extended to support other SoCs which support RPMh

Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
---
 drivers/soc/qcom/Kconfig                |   9 +
 drivers/soc/qcom/Makefile               |   1 +
 drivers/soc/qcom/rpmhpd.c               | 427 ++++++++++++++++++++++++
 include/dt-bindings/power/qcom-rpmhpd.h |  31 ++
 4 files changed, 468 insertions(+)
 create mode 100644 drivers/soc/qcom/rpmhpd.c
 create mode 100644 include/dt-bindings/power/qcom-rpmhpd.h

diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index 5c54931a7b99..7cb7eba2b997 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -74,6 +74,15 @@ config QCOM_RMTFS_MEM
 
 	  Say y here if you intend to boot the modem remoteproc.
 
+config QCOM_RPMHPD
+	tristate "Qualcomm RPMh Power domain driver"
+	depends on QCOM_RPMH && QCOM_COMMAND_DB
+	help
+	  QCOM RPMh Power domain driver to support power-domains with
+	  performance states. The driver communicates a performance state
+	  value to RPMh which then translates it into corresponding voltage
+	  for the voltage rail.
+
 config QCOM_RPMPD
 	tristate "Qualcomm RPM Power domain driver"
 	depends on MFD_QCOM_RPM && QCOM_SMD_RPM
diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index 9550c170de93..499513f63bef 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -16,3 +16,4 @@ obj-$(CONFIG_QCOM_SMSM)	+= smsm.o
 obj-$(CONFIG_QCOM_WCNSS_CTRL) += wcnss_ctrl.o
 obj-$(CONFIG_QCOM_APR) += apr.o
 obj-$(CONFIG_QCOM_RPMPD) += rpmpd.o
+obj-$(CONFIG_QCOM_RPMHPD) += rpmhpd.o
diff --git a/drivers/soc/qcom/rpmhpd.c b/drivers/soc/qcom/rpmhpd.c
new file mode 100644
index 000000000000..7083ec1590ff
--- /dev/null
+++ b/drivers/soc/qcom/rpmhpd.c
@@ -0,0 +1,427 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2018, The Linux Foundation. All rights reserved.*/
+
+#include <linux/err.h>
+#include <linux/export.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/pm_domain.h>
+#include <linux/slab.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/pm_opp.h>
+#include <soc/qcom/cmd-db.h>
+#include <soc/qcom/rpmh.h>
+#include <dt-bindings/power/qcom-rpmhpd.h>
+
+#define domain_to_rpmhpd(domain) container_of(domain, struct rpmhpd, pd)
+
+#define DEFINE_RPMHPD_AO(_platform, _name, _active)			\
+	static struct rpmhpd _platform##_##_active;			\
+	static struct rpmhpd _platform##_##_name = {			\
+		.pd = {	.name = #_name,	},				\
+		.peer = &_platform##_##_active,				\
+		.res_name = #_name".lvl",				\
+		.valid_state_mask = (BIT(RPMH_ACTIVE_ONLY_STATE) |	\
+					BIT(RPMH_WAKE_ONLY_STATE) |	\
+					BIT(RPMH_SLEEP_STATE)),		\
+	};								\
+	static struct rpmhpd _platform##_##_active = {			\
+		.pd = { .name = #_active, },				\
+		.peer = &_platform##_##_name,				\
+		.active_only = true,					\
+		.res_name = #_name".lvl",				\
+		.valid_state_mask = (BIT(RPMH_ACTIVE_ONLY_STATE) |	\
+					BIT(RPMH_WAKE_ONLY_STATE) |	\
+					BIT(RPMH_SLEEP_STATE)),		\
+	}
+
+#define DEFINE_RPMHPD(_platform, _name)					\
+	static struct rpmhpd _platform##_##_name = {			\
+		.pd = { .name = #_name, },				\
+		.res_name = #_name".lvl",				\
+		.valid_state_mask = BIT(RPMH_ACTIVE_ONLY_STATE),	\
+	}
+
+/*
+ * This is the number of bytes used for each command DB aux data entry of an
+ * ARC resource.
+ */
+#define RPMH_ARC_LEVEL_SIZE	2
+#define RPMH_ARC_MAX_LEVELS	16
+
+struct rpmhpd {
+	struct device	*dev;
+	struct generic_pm_domain pd;
+	struct rpmhpd	*peer;
+	const bool	active_only;
+	unsigned int	corner;
+	unsigned int	active_corner;
+	u32		level[RPMH_ARC_MAX_LEVELS];
+	int		level_count;
+	bool		enabled;
+	const char	*res_name;
+	u32		addr;
+	u8		valid_state_mask;
+};
+
+struct rpmhpd_desc {
+	struct rpmhpd **rpmhpds;
+	size_t num_pds;
+};
+
+static DEFINE_MUTEX(rpmhpd_lock);
+
+/* sdm845 RPMh Power domains */
+DEFINE_RPMHPD(sdm845, ebi);
+DEFINE_RPMHPD_AO(sdm845, mx, mx_ao);
+DEFINE_RPMHPD_AO(sdm845, cx, cx_ao);
+DEFINE_RPMHPD(sdm845, lmx);
+DEFINE_RPMHPD(sdm845, lcx);
+DEFINE_RPMHPD(sdm845, gfx);
+DEFINE_RPMHPD(sdm845, mss);
+
+static struct rpmhpd *sdm845_rpmhpds[] = {
+	[SDM845_EBI] = &sdm845_ebi,
+	[SDM845_MX] = &sdm845_mx,
+	[SDM845_MX_AO] = &sdm845_mx_ao,
+	[SDM845_CX] = &sdm845_cx,
+	[SDM845_CX_AO] = &sdm845_cx_ao,
+	[SDM845_LMX] = &sdm845_lmx,
+	[SDM845_LCX] = &sdm845_lcx,
+	[SDM845_GFX] = &sdm845_gfx,
+	[SDM845_MSS] = &sdm845_mss,
+};
+
+static const struct rpmhpd_desc sdm845_desc = {
+	.rpmhpds = sdm845_rpmhpds,
+	.num_pds = ARRAY_SIZE(sdm845_rpmhpds),
+};
+
+static const struct of_device_id rpmhpd_match_table[] = {
+	{ .compatible = "qcom,sdm845-rpmhpd", .data = &sdm845_desc },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, rpmhpd_match_table);
+
+static int rpmhpd_send_corner(struct rpmhpd *pd, int state,
+			      unsigned int corner, bool sync)
+{
+	struct tcs_cmd cmd = {
+		.addr = pd->addr,
+		.data = corner,
+	};
+
+	if (sync)
+		return rpmh_write(pd->dev, state, &cmd, 1);
+	else
+		return rpmh_write_async(pd->dev, state, &cmd, 1);
+}
+
+static int rpmhpd_send_corner_sync(struct rpmhpd *pd, int state,
+				   unsigned int corner)
+{
+	return rpmhpd_send_corner(pd, state, corner, true);
+}
+
+static int rpmhpd_send_corner_async(struct rpmhpd *pd, int state,
+				    unsigned int corner)
+{
+	return rpmhpd_send_corner(pd, state, corner, false);
+};
+
+static void to_active_sleep(struct rpmhpd *pd, unsigned int corner,
+			    unsigned int *active, unsigned int *sleep)
+{
+	*active = corner;
+
+	if (pd->active_only)
+		*sleep = 0;
+	else
+		*sleep = *active;
+}
+
+/*
+ * This function is used to aggregate the votes across the active only
+ * resources and its peers. The aggregated votes are send to RPMh as
+ * ACTIVE_ONLY votes (which take effect immediately), as WAKE_ONLY votes
+ * (applied by RPMh on system wakeup) and as SLEEP votes (applied by RPMh
+ * on system sleep).
+ * We send ACTIVE_ONLY votes for resources without any peers. For others,
+ * which have an active only peer, all 3 Votes are sent.
+ */
+static int rpmhpd_aggregate_corner(struct rpmhpd *pd, unsigned int corner)
+{
+	int ret = -EINVAL;
+	struct rpmhpd *peer = pd->peer;
+	unsigned int active_corner, sleep_corner;
+	unsigned int this_active_corner = 0, this_sleep_corner = 0;
+	unsigned int peer_active_corner = 0, peer_sleep_corner = 0;
+
+	to_active_sleep(pd, corner, &this_active_corner, &this_sleep_corner);
+
+	if (peer && peer->enabled)
+		to_active_sleep(peer, peer->corner, &peer_active_corner,
+				&peer_sleep_corner);
+
+	active_corner = max(this_active_corner, peer_active_corner);
+
+	if (pd->valid_state_mask & BIT(RPMH_ACTIVE_ONLY_STATE)) {
+		/*
+		 * Wait for an ack only when we are increasing the
+		 * perf state of the power domain
+		 */
+		if (active_corner > pd->active_corner)
+			ret = rpmhpd_send_corner_sync(pd,
+						      RPMH_ACTIVE_ONLY_STATE,
+						      active_corner);
+		else
+			ret = rpmhpd_send_corner_async(pd,
+						       RPMH_ACTIVE_ONLY_STATE,
+						       active_corner);
+		if (ret)
+			return ret;
+		pd->active_corner = active_corner;
+		if (peer)
+			peer->active_corner = active_corner;
+	}
+
+	if (pd->valid_state_mask & BIT(RPMH_WAKE_ONLY_STATE)) {
+		ret = rpmhpd_send_corner_async(pd, RPMH_WAKE_ONLY_STATE,
+					       active_corner);
+		if (ret)
+			return ret;
+	}
+
+	sleep_corner = max(this_sleep_corner, peer_sleep_corner);
+
+	if (pd->valid_state_mask & BIT(RPMH_SLEEP_STATE))
+		ret = rpmhpd_send_corner_async(pd, RPMH_SLEEP_STATE,
+					       sleep_corner);
+
+	return ret;
+}
+
+static int rpmhpd_power_on(struct generic_pm_domain *domain)
+{
+	struct rpmhpd *pd = domain_to_rpmhpd(domain);
+	int ret = 0;
+
+	mutex_lock(&rpmhpd_lock);
+
+	pd->enabled = true;
+
+	if (pd->corner)
+		ret = rpmhpd_aggregate_corner(pd, pd->corner);
+
+	mutex_unlock(&rpmhpd_lock);
+
+	return ret;
+}
+
+static int rpmhpd_power_off(struct generic_pm_domain *domain)
+{
+	struct rpmhpd *pd = domain_to_rpmhpd(domain);
+	int ret = 0;
+
+	mutex_lock(&rpmhpd_lock);
+
+	if (pd->level[0] == 0)
+		ret = rpmhpd_aggregate_corner(pd, 0);
+
+	if (!ret)
+		pd->enabled = false;
+
+	mutex_unlock(&rpmhpd_lock);
+
+	return ret;
+}
+
+static int rpmhpd_set_performance(struct generic_pm_domain *domain,
+				  unsigned int state)
+{
+	struct rpmhpd *pd = domain_to_rpmhpd(domain);
+	int ret = 0, i;
+
+	mutex_lock(&rpmhpd_lock);
+
+	for (i = 0; i < pd->level_count; i++)
+		if (state <= pd->level[i])
+			break;
+
+	if (i == pd->level_count) {
+		ret = -EINVAL;
+		dev_err(pd->dev, "invalid state=%u for domain %s",
+			state, pd->pd.name);
+			goto out;
+	}
+
+	pd->corner = i;
+
+	if (!pd->enabled)
+		goto out;
+
+	ret = rpmhpd_aggregate_corner(pd, i);
+out:
+	mutex_unlock(&rpmhpd_lock);
+
+	return ret;
+}
+
+static unsigned int rpmhpd_get_performance(struct generic_pm_domain *genpd,
+					   struct dev_pm_opp *opp)
+{
+	struct device_node *np;
+	unsigned int corner = 0;
+
+	np = dev_pm_opp_get_of_node(opp);
+	if (of_property_read_u32(np, "qcom,level", &corner)) {
+		pr_err("%s: missing 'qcom,level' property\n", __func__);
+		return 0;
+	}
+
+	of_node_put(np);
+
+	return corner;
+}
+
+static int rpmhpd_update_level_mapping(struct rpmhpd *rpmhpd)
+{
+	int i, j, len, ret;
+	u8 buf[RPMH_ARC_MAX_LEVELS * RPMH_ARC_LEVEL_SIZE];
+
+	len = cmd_db_read_aux_data_len(rpmhpd->res_name);
+	if (len <= 0)
+		return len;
+
+	if (len > RPMH_ARC_MAX_LEVELS * RPMH_ARC_LEVEL_SIZE)
+		return -EINVAL;
+
+	ret = cmd_db_read_aux_data(rpmhpd->res_name, buf, len);
+	if (ret < 0)
+		return ret;
+
+	rpmhpd->level_count = len / RPMH_ARC_LEVEL_SIZE;
+
+	for (i = 0; i < rpmhpd->level_count; i++) {
+		rpmhpd->level[i] = 0;
+		for (j = 0; j < RPMH_ARC_LEVEL_SIZE; j++)
+			rpmhpd->level[i] |=
+				buf[i * RPMH_ARC_LEVEL_SIZE + j] << (8 * j);
+
+		/*
+		 * The AUX data may be zero padded.  These 0 valued entries at
+		 * the end of the map must be ignored.
+		 */
+		if (i > 0 && rpmhpd->level[i] == 0) {
+			rpmhpd->level_count = i;
+			break;
+		}
+		pr_dbg("%s: ARC hlvl=%2d --> vlvl=%4u\n", rpmhpd->res_name, i,
+		       rpmhpd->level[i]);
+	}
+
+	return 0;
+}
+
+static int rpmhpd_probe(struct platform_device *pdev)
+{
+	int i, ret;
+	size_t num;
+	struct genpd_onecell_data *data;
+	struct rpmhpd **rpmhpds;
+	const struct rpmhpd_desc *desc;
+
+	desc = of_device_get_match_data(&pdev->dev);
+	if (!desc)
+		return -EINVAL;
+
+	rpmhpds = desc->rpmhpds;
+	num = desc->num_pds;
+
+	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->domains = devm_kcalloc(&pdev->dev, num, sizeof(*data->domains),
+				     GFP_KERNEL);
+	data->num_domains = num;
+
+	ret = cmd_db_ready();
+	if (ret) {
+		if (ret != -EPROBE_DEFER)
+			dev_err(&pdev->dev, "Command DB unavailable, ret=%d\n",
+				ret);
+		return ret;
+	}
+
+	for (i = 0; i < num; i++) {
+		if (!rpmhpds[i]) {
+			dev_warn(&pdev->dev, "rpmhpds[] with empty entry at index=%d\n",
+				 i);
+			continue;
+		}
+
+		rpmhpds[i]->dev = &pdev->dev;
+		rpmhpds[i]->addr = cmd_db_read_addr(rpmhpds[i]->res_name);
+		if (!rpmhpds[i]->addr) {
+			dev_err(&pdev->dev, "Could not find RPMh address for resource %s\n",
+				rpmhpds[i]->res_name);
+			return -ENODEV;
+		}
+
+		ret = cmd_db_read_slave_id(rpmhpds[i]->res_name);
+		if (ret != CMD_DB_HW_ARC) {
+			dev_err(&pdev->dev, "RPMh slave ID mismatch\n");
+			return -EINVAL;
+		}
+
+		ret = rpmhpd_update_level_mapping(rpmhpds[i]);
+		if (ret)
+			return ret;
+
+		rpmhpds[i]->pd.power_off = rpmhpd_power_off;
+		rpmhpds[i]->pd.power_on = rpmhpd_power_on;
+		rpmhpds[i]->pd.set_performance_state = rpmhpd_set_performance;
+		rpmhpds[i]->pd.opp_to_performance_state = rpmhpd_get_performance;
+		pm_genpd_init(&rpmhpds[i]->pd, NULL, true);
+
+		data->domains[i] = &rpmhpds[i]->pd;
+	}
+
+	return of_genpd_add_provider_onecell(pdev->dev.of_node, data);
+}
+
+static int rpmhpd_remove(struct platform_device *pdev)
+{
+	of_genpd_del_provider(pdev->dev.of_node);
+	return 0;
+}
+
+static struct platform_driver rpmhpd_driver = {
+	.driver = {
+		.name = "qcom-rpmhpd",
+		.of_match_table = rpmhpd_match_table,
+	},
+	.probe = rpmhpd_probe,
+	.remove = rpmhpd_remove,
+};
+
+static int __init rpmhpd_init(void)
+{
+	return platform_driver_register(&rpmhpd_driver);
+}
+core_initcall(rpmhpd_init);
+
+static void __exit rpmhpd_exit(void)
+{
+	platform_driver_unregister(&rpmhpd_driver);
+}
+module_exit(rpmhpd_exit);
+
+MODULE_DESCRIPTION("Qualcomm Technologies, Inc. RPMh Power Domain Driver");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:qcom-rpmhpd");
diff --git a/include/dt-bindings/power/qcom-rpmhpd.h b/include/dt-bindings/power/qcom-rpmhpd.h
new file mode 100644
index 000000000000..b01ae2452603
--- /dev/null
+++ b/include/dt-bindings/power/qcom-rpmhpd.h
@@ -0,0 +1,31 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright (c) 2018, The Linux Foundation. All rights reserved. */
+
+#ifndef _DT_BINDINGS_POWER_QCOM_RPMHPD_H
+#define _DT_BINDINGS_POWER_QCOM_RPMHPD_H
+
+/* SDM845 Power Domain Indexes */
+#define SDM845_EBI	0
+#define SDM845_MX	1
+#define SDM845_MX_AO	2
+#define SDM845_CX	3
+#define SDM845_CX_AO	4
+#define SDM845_LMX	5
+#define SDM845_LCX	6
+#define SDM845_GFX	7
+#define SDM845_MSS	8
+
+/* SDM845 Power Domain performance levels */
+#define RPMH_REGULATOR_LEVEL_OFF	0
+#define RPMH_REGULATOR_LEVEL_RETENTION	16
+#define RPMH_REGULATOR_LEVEL_MIN_SVS	48
+#define RPMH_REGULATOR_LEVEL_LOW_SVS	64
+#define RPMH_REGULATOR_LEVEL_SVS	128
+#define RPMH_REGULATOR_LEVEL_SVS_L1	192
+#define RPMH_REGULATOR_LEVEL_NOM	256
+#define RPMH_REGULATOR_LEVEL_NOM_L1	320
+#define RPMH_REGULATOR_LEVEL_NOM_L2	336
+#define RPMH_REGULATOR_LEVEL_TURBO	384
+#define RPMH_REGULATOR_LEVEL_TURBO_L1	416
+
+#endif
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v3 6/7] soc: qcom: Add RPMh Power domain driver

[Ulf Hansson]

On 12 June 2018 at 06:40, Rajendra Nayak <rnayak@codeaurora.org> wrote:
> The RPMh Power domain driver aggregates the corner votes from various
> consumers for the ARC resources and communicates it to RPMh.
>
> We also add data for all power domains on sdm845 SoC as part of the patch.
> The driver can be extended to support other SoCs which support RPMh
>
> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

[...]

> +
> +static int rpmhpd_remove(struct platform_device *pdev)
> +{
> +       of_genpd_del_provider(pdev->dev.of_node);

Similar comment as I had for patch2.

> +       return 0;
> +}
> +
> +static struct platform_driver rpmhpd_driver = {
> +       .driver = {
> +               .name = "qcom-rpmhpd",
> +               .of_match_table = rpmhpd_match_table,
> +       },
> +       .probe = rpmhpd_probe,
> +       .remove = rpmhpd_remove,
> +};

[...]

Kind regards
Uffe

Re: [PATCH v3 6/7] soc: qcom: Add RPMh Power domain driver

[Rob Herring]

On Tue, Jun 12, 2018 at 10:10:51AM +0530, Rajendra Nayak wrote:
> The RPMh Power domain driver aggregates the corner votes from various
> consumers for the ARC resources and communicates it to RPMh.
> 
> We also add data for all power domains on sdm845 SoC as part of the patch.
> The driver can be extended to support other SoCs which support RPMh
> 
> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
> ---
>  drivers/soc/qcom/Kconfig                |   9 +
>  drivers/soc/qcom/Makefile               |   1 +
>  drivers/soc/qcom/rpmhpd.c               | 427 ++++++++++++++++++++++++
>  include/dt-bindings/power/qcom-rpmhpd.h |  31 ++

These includes should go with the binding.

>  4 files changed, 468 insertions(+)
>  create mode 100644 drivers/soc/qcom/rpmhpd.c
>  create mode 100644 include/dt-bindings/power/qcom-rpmhpd.h
> 
> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> index 5c54931a7b99..7cb7eba2b997 100644
> --- a/drivers/soc/qcom/Kconfig
> +++ b/drivers/soc/qcom/Kconfig
> @@ -74,6 +74,15 @@ config QCOM_RMTFS_MEM
>  
>  	  Say y here if you intend to boot the modem remoteproc.
>  
> +config QCOM_RPMHPD
> +	tristate "Qualcomm RPMh Power domain driver"
> +	depends on QCOM_RPMH && QCOM_COMMAND_DB
> +	help
> +	  QCOM RPMh Power domain driver to support power-domains with
> +	  performance states. The driver communicates a performance state
> +	  value to RPMh which then translates it into corresponding voltage
> +	  for the voltage rail.
> +
>  config QCOM_RPMPD
>  	tristate "Qualcomm RPM Power domain driver"
>  	depends on MFD_QCOM_RPM && QCOM_SMD_RPM
> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
> index 9550c170de93..499513f63bef 100644
> --- a/drivers/soc/qcom/Makefile
> +++ b/drivers/soc/qcom/Makefile
> @@ -16,3 +16,4 @@ obj-$(CONFIG_QCOM_SMSM)	+= smsm.o
>  obj-$(CONFIG_QCOM_WCNSS_CTRL) += wcnss_ctrl.o
>  obj-$(CONFIG_QCOM_APR) += apr.o
>  obj-$(CONFIG_QCOM_RPMPD) += rpmpd.o
> +obj-$(CONFIG_QCOM_RPMHPD) += rpmhpd.o
> diff --git a/drivers/soc/qcom/rpmhpd.c b/drivers/soc/qcom/rpmhpd.c
> new file mode 100644
> index 000000000000..7083ec1590ff
> --- /dev/null
> +++ b/drivers/soc/qcom/rpmhpd.c
> @@ -0,0 +1,427 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2018, The Linux Foundation. All rights reserved.*/
> +
> +#include <linux/err.h>
> +#include <linux/export.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/pm_domain.h>
> +#include <linux/slab.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_opp.h>
> +#include <soc/qcom/cmd-db.h>
> +#include <soc/qcom/rpmh.h>
> +#include <dt-bindings/power/qcom-rpmhpd.h>
> +
> +#define domain_to_rpmhpd(domain) container_of(domain, struct rpmhpd, pd)
> +
> +#define DEFINE_RPMHPD_AO(_platform, _name, _active)			\
> +	static struct rpmhpd _platform##_##_active;			\
> +	static struct rpmhpd _platform##_##_name = {			\
> +		.pd = {	.name = #_name,	},				\
> +		.peer = &_platform##_##_active,				\
> +		.res_name = #_name".lvl",				\
> +		.valid_state_mask = (BIT(RPMH_ACTIVE_ONLY_STATE) |	\
> +					BIT(RPMH_WAKE_ONLY_STATE) |	\
> +					BIT(RPMH_SLEEP_STATE)),		\
> +	};								\
> +	static struct rpmhpd _platform##_##_active = {			\
> +		.pd = { .name = #_active, },				\
> +		.peer = &_platform##_##_name,				\
> +		.active_only = true,					\
> +		.res_name = #_name".lvl",				\
> +		.valid_state_mask = (BIT(RPMH_ACTIVE_ONLY_STATE) |	\
> +					BIT(RPMH_WAKE_ONLY_STATE) |	\
> +					BIT(RPMH_SLEEP_STATE)),		\
> +	}
> +
> +#define DEFINE_RPMHPD(_platform, _name)					\
> +	static struct rpmhpd _platform##_##_name = {			\
> +		.pd = { .name = #_name, },				\
> +		.res_name = #_name".lvl",				\
> +		.valid_state_mask = BIT(RPMH_ACTIVE_ONLY_STATE),	\
> +	}
> +
> +/*
> + * This is the number of bytes used for each command DB aux data entry of an
> + * ARC resource.
> + */
> +#define RPMH_ARC_LEVEL_SIZE	2
> +#define RPMH_ARC_MAX_LEVELS	16
> +
> +struct rpmhpd {
> +	struct device	*dev;
> +	struct generic_pm_domain pd;
> +	struct rpmhpd	*peer;
> +	const bool	active_only;
> +	unsigned int	corner;
> +	unsigned int	active_corner;
> +	u32		level[RPMH_ARC_MAX_LEVELS];
> +	int		level_count;
> +	bool		enabled;
> +	const char	*res_name;
> +	u32		addr;
> +	u8		valid_state_mask;
> +};
> +
> +struct rpmhpd_desc {
> +	struct rpmhpd **rpmhpds;
> +	size_t num_pds;
> +};
> +
> +static DEFINE_MUTEX(rpmhpd_lock);
> +
> +/* sdm845 RPMh Power domains */
> +DEFINE_RPMHPD(sdm845, ebi);
> +DEFINE_RPMHPD_AO(sdm845, mx, mx_ao);
> +DEFINE_RPMHPD_AO(sdm845, cx, cx_ao);
> +DEFINE_RPMHPD(sdm845, lmx);
> +DEFINE_RPMHPD(sdm845, lcx);
> +DEFINE_RPMHPD(sdm845, gfx);
> +DEFINE_RPMHPD(sdm845, mss);
> +
> +static struct rpmhpd *sdm845_rpmhpds[] = {
> +	[SDM845_EBI] = &sdm845_ebi,
> +	[SDM845_MX] = &sdm845_mx,
> +	[SDM845_MX_AO] = &sdm845_mx_ao,
> +	[SDM845_CX] = &sdm845_cx,
> +	[SDM845_CX_AO] = &sdm845_cx_ao,
> +	[SDM845_LMX] = &sdm845_lmx,
> +	[SDM845_LCX] = &sdm845_lcx,
> +	[SDM845_GFX] = &sdm845_gfx,
> +	[SDM845_MSS] = &sdm845_mss,
> +};
> +
> +static const struct rpmhpd_desc sdm845_desc = {
> +	.rpmhpds = sdm845_rpmhpds,
> +	.num_pds = ARRAY_SIZE(sdm845_rpmhpds),
> +};
> +
> +static const struct of_device_id rpmhpd_match_table[] = {
> +	{ .compatible = "qcom,sdm845-rpmhpd", .data = &sdm845_desc },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, rpmhpd_match_table);
> +
> +static int rpmhpd_send_corner(struct rpmhpd *pd, int state,
> +			      unsigned int corner, bool sync)
> +{
> +	struct tcs_cmd cmd = {
> +		.addr = pd->addr,
> +		.data = corner,
> +	};
> +
> +	if (sync)
> +		return rpmh_write(pd->dev, state, &cmd, 1);
> +	else
> +		return rpmh_write_async(pd->dev, state, &cmd, 1);
> +}
> +
> +static int rpmhpd_send_corner_sync(struct rpmhpd *pd, int state,
> +				   unsigned int corner)
> +{
> +	return rpmhpd_send_corner(pd, state, corner, true);
> +}
> +
> +static int rpmhpd_send_corner_async(struct rpmhpd *pd, int state,
> +				    unsigned int corner)
> +{
> +	return rpmhpd_send_corner(pd, state, corner, false);
> +};
> +
> +static void to_active_sleep(struct rpmhpd *pd, unsigned int corner,
> +			    unsigned int *active, unsigned int *sleep)
> +{
> +	*active = corner;
> +
> +	if (pd->active_only)
> +		*sleep = 0;
> +	else
> +		*sleep = *active;
> +}
> +
> +/*
> + * This function is used to aggregate the votes across the active only
> + * resources and its peers. The aggregated votes are send to RPMh as
> + * ACTIVE_ONLY votes (which take effect immediately), as WAKE_ONLY votes
> + * (applied by RPMh on system wakeup) and as SLEEP votes (applied by RPMh
> + * on system sleep).
> + * We send ACTIVE_ONLY votes for resources without any peers. For others,
> + * which have an active only peer, all 3 Votes are sent.
> + */
> +static int rpmhpd_aggregate_corner(struct rpmhpd *pd, unsigned int corner)
> +{
> +	int ret = -EINVAL;
> +	struct rpmhpd *peer = pd->peer;
> +	unsigned int active_corner, sleep_corner;
> +	unsigned int this_active_corner = 0, this_sleep_corner = 0;
> +	unsigned int peer_active_corner = 0, peer_sleep_corner = 0;
> +
> +	to_active_sleep(pd, corner, &this_active_corner, &this_sleep_corner);
> +
> +	if (peer && peer->enabled)
> +		to_active_sleep(peer, peer->corner, &peer_active_corner,
> +				&peer_sleep_corner);
> +
> +	active_corner = max(this_active_corner, peer_active_corner);
> +
> +	if (pd->valid_state_mask & BIT(RPMH_ACTIVE_ONLY_STATE)) {
> +		/*
> +		 * Wait for an ack only when we are increasing the
> +		 * perf state of the power domain
> +		 */
> +		if (active_corner > pd->active_corner)
> +			ret = rpmhpd_send_corner_sync(pd,
> +						      RPMH_ACTIVE_ONLY_STATE,
> +						      active_corner);
> +		else
> +			ret = rpmhpd_send_corner_async(pd,
> +						       RPMH_ACTIVE_ONLY_STATE,
> +						       active_corner);
> +		if (ret)
> +			return ret;
> +		pd->active_corner = active_corner;
> +		if (peer)
> +			peer->active_corner = active_corner;
> +	}
> +
> +	if (pd->valid_state_mask & BIT(RPMH_WAKE_ONLY_STATE)) {
> +		ret = rpmhpd_send_corner_async(pd, RPMH_WAKE_ONLY_STATE,
> +					       active_corner);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	sleep_corner = max(this_sleep_corner, peer_sleep_corner);
> +
> +	if (pd->valid_state_mask & BIT(RPMH_SLEEP_STATE))
> +		ret = rpmhpd_send_corner_async(pd, RPMH_SLEEP_STATE,
> +					       sleep_corner);
> +
> +	return ret;
> +}
> +
> +static int rpmhpd_power_on(struct generic_pm_domain *domain)
> +{
> +	struct rpmhpd *pd = domain_to_rpmhpd(domain);
> +	int ret = 0;
> +
> +	mutex_lock(&rpmhpd_lock);
> +
> +	pd->enabled = true;
> +
> +	if (pd->corner)
> +		ret = rpmhpd_aggregate_corner(pd, pd->corner);
> +
> +	mutex_unlock(&rpmhpd_lock);
> +
> +	return ret;
> +}
> +
> +static int rpmhpd_power_off(struct generic_pm_domain *domain)
> +{
> +	struct rpmhpd *pd = domain_to_rpmhpd(domain);
> +	int ret = 0;
> +
> +	mutex_lock(&rpmhpd_lock);
> +
> +	if (pd->level[0] == 0)
> +		ret = rpmhpd_aggregate_corner(pd, 0);
> +
> +	if (!ret)
> +		pd->enabled = false;
> +
> +	mutex_unlock(&rpmhpd_lock);
> +
> +	return ret;
> +}
> +
> +static int rpmhpd_set_performance(struct generic_pm_domain *domain,
> +				  unsigned int state)
> +{
> +	struct rpmhpd *pd = domain_to_rpmhpd(domain);
> +	int ret = 0, i;
> +
> +	mutex_lock(&rpmhpd_lock);
> +
> +	for (i = 0; i < pd->level_count; i++)
> +		if (state <= pd->level[i])
> +			break;
> +
> +	if (i == pd->level_count) {
> +		ret = -EINVAL;
> +		dev_err(pd->dev, "invalid state=%u for domain %s",
> +			state, pd->pd.name);
> +			goto out;
> +	}
> +
> +	pd->corner = i;
> +
> +	if (!pd->enabled)
> +		goto out;
> +
> +	ret = rpmhpd_aggregate_corner(pd, i);
> +out:
> +	mutex_unlock(&rpmhpd_lock);
> +
> +	return ret;
> +}
> +
> +static unsigned int rpmhpd_get_performance(struct generic_pm_domain *genpd,
> +					   struct dev_pm_opp *opp)
> +{
> +	struct device_node *np;
> +	unsigned int corner = 0;
> +
> +	np = dev_pm_opp_get_of_node(opp);
> +	if (of_property_read_u32(np, "qcom,level", &corner)) {
> +		pr_err("%s: missing 'qcom,level' property\n", __func__);
> +		return 0;
> +	}
> +
> +	of_node_put(np);
> +
> +	return corner;
> +}
> +
> +static int rpmhpd_update_level_mapping(struct rpmhpd *rpmhpd)
> +{
> +	int i, j, len, ret;
> +	u8 buf[RPMH_ARC_MAX_LEVELS * RPMH_ARC_LEVEL_SIZE];
> +
> +	len = cmd_db_read_aux_data_len(rpmhpd->res_name);
> +	if (len <= 0)
> +		return len;
> +
> +	if (len > RPMH_ARC_MAX_LEVELS * RPMH_ARC_LEVEL_SIZE)
> +		return -EINVAL;
> +
> +	ret = cmd_db_read_aux_data(rpmhpd->res_name, buf, len);
> +	if (ret < 0)
> +		return ret;
> +
> +	rpmhpd->level_count = len / RPMH_ARC_LEVEL_SIZE;
> +
> +	for (i = 0; i < rpmhpd->level_count; i++) {
> +		rpmhpd->level[i] = 0;
> +		for (j = 0; j < RPMH_ARC_LEVEL_SIZE; j++)
> +			rpmhpd->level[i] |=
> +				buf[i * RPMH_ARC_LEVEL_SIZE + j] << (8 * j);
> +
> +		/*
> +		 * The AUX data may be zero padded.  These 0 valued entries at
> +		 * the end of the map must be ignored.
> +		 */
> +		if (i > 0 && rpmhpd->level[i] == 0) {
> +			rpmhpd->level_count = i;
> +			break;
> +		}
> +		pr_dbg("%s: ARC hlvl=%2d --> vlvl=%4u\n", rpmhpd->res_name, i,
> +		       rpmhpd->level[i]);
> +	}
> +
> +	return 0;
> +}
> +
> +static int rpmhpd_probe(struct platform_device *pdev)
> +{
> +	int i, ret;
> +	size_t num;
> +	struct genpd_onecell_data *data;
> +	struct rpmhpd **rpmhpds;
> +	const struct rpmhpd_desc *desc;
> +
> +	desc = of_device_get_match_data(&pdev->dev);
> +	if (!desc)
> +		return -EINVAL;
> +
> +	rpmhpds = desc->rpmhpds;
> +	num = desc->num_pds;
> +
> +	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	data->domains = devm_kcalloc(&pdev->dev, num, sizeof(*data->domains),
> +				     GFP_KERNEL);
> +	data->num_domains = num;
> +
> +	ret = cmd_db_ready();
> +	if (ret) {
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(&pdev->dev, "Command DB unavailable, ret=%d\n",
> +				ret);
> +		return ret;
> +	}
> +
> +	for (i = 0; i < num; i++) {
> +		if (!rpmhpds[i]) {
> +			dev_warn(&pdev->dev, "rpmhpds[] with empty entry at index=%d\n",
> +				 i);
> +			continue;
> +		}
> +
> +		rpmhpds[i]->dev = &pdev->dev;
> +		rpmhpds[i]->addr = cmd_db_read_addr(rpmhpds[i]->res_name);
> +		if (!rpmhpds[i]->addr) {
> +			dev_err(&pdev->dev, "Could not find RPMh address for resource %s\n",
> +				rpmhpds[i]->res_name);
> +			return -ENODEV;
> +		}
> +
> +		ret = cmd_db_read_slave_id(rpmhpds[i]->res_name);
> +		if (ret != CMD_DB_HW_ARC) {
> +			dev_err(&pdev->dev, "RPMh slave ID mismatch\n");
> +			return -EINVAL;
> +		}
> +
> +		ret = rpmhpd_update_level_mapping(rpmhpds[i]);
> +		if (ret)
> +			return ret;
> +
> +		rpmhpds[i]->pd.power_off = rpmhpd_power_off;
> +		rpmhpds[i]->pd.power_on = rpmhpd_power_on;
> +		rpmhpds[i]->pd.set_performance_state = rpmhpd_set_performance;
> +		rpmhpds[i]->pd.opp_to_performance_state = rpmhpd_get_performance;
> +		pm_genpd_init(&rpmhpds[i]->pd, NULL, true);
> +
> +		data->domains[i] = &rpmhpds[i]->pd;
> +	}
> +
> +	return of_genpd_add_provider_onecell(pdev->dev.of_node, data);
> +}
> +
> +static int rpmhpd_remove(struct platform_device *pdev)
> +{
> +	of_genpd_del_provider(pdev->dev.of_node);
> +	return 0;
> +}
> +
> +static struct platform_driver rpmhpd_driver = {
> +	.driver = {
> +		.name = "qcom-rpmhpd",
> +		.of_match_table = rpmhpd_match_table,
> +	},
> +	.probe = rpmhpd_probe,
> +	.remove = rpmhpd_remove,
> +};
> +
> +static int __init rpmhpd_init(void)
> +{
> +	return platform_driver_register(&rpmhpd_driver);
> +}
> +core_initcall(rpmhpd_init);
> +
> +static void __exit rpmhpd_exit(void)
> +{
> +	platform_driver_unregister(&rpmhpd_driver);
> +}
> +module_exit(rpmhpd_exit);
> +
> +MODULE_DESCRIPTION("Qualcomm Technologies, Inc. RPMh Power Domain Driver");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:qcom-rpmhpd");
> diff --git a/include/dt-bindings/power/qcom-rpmhpd.h b/include/dt-bindings/power/qcom-rpmhpd.h
> new file mode 100644
> index 000000000000..b01ae2452603
> --- /dev/null
> +++ b/include/dt-bindings/power/qcom-rpmhpd.h
> @@ -0,0 +1,31 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright (c) 2018, The Linux Foundation. All rights reserved. */
> +
> +#ifndef _DT_BINDINGS_POWER_QCOM_RPMHPD_H
> +#define _DT_BINDINGS_POWER_QCOM_RPMHPD_H
> +
> +/* SDM845 Power Domain Indexes */
> +#define SDM845_EBI	0
> +#define SDM845_MX	1
> +#define SDM845_MX_AO	2
> +#define SDM845_CX	3
> +#define SDM845_CX_AO	4
> +#define SDM845_LMX	5
> +#define SDM845_LCX	6
> +#define SDM845_GFX	7
> +#define SDM845_MSS	8
> +
> +/* SDM845 Power Domain performance levels */
> +#define RPMH_REGULATOR_LEVEL_OFF	0
> +#define RPMH_REGULATOR_LEVEL_RETENTION	16
> +#define RPMH_REGULATOR_LEVEL_MIN_SVS	48
> +#define RPMH_REGULATOR_LEVEL_LOW_SVS	64
> +#define RPMH_REGULATOR_LEVEL_SVS	128
> +#define RPMH_REGULATOR_LEVEL_SVS_L1	192
> +#define RPMH_REGULATOR_LEVEL_NOM	256
> +#define RPMH_REGULATOR_LEVEL_NOM_L1	320
> +#define RPMH_REGULATOR_LEVEL_NOM_L2	336
> +#define RPMH_REGULATOR_LEVEL_TURBO	384
> +#define RPMH_REGULATOR_LEVEL_TURBO_L1	416
> +
> +#endif
> -- 
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation
> 
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 6/7] soc: qcom: Add RPMh Power domain driver

[Rajendra Nayak]

On 06/13/2018 12:36 AM, Rob Herring wrote:
> On Tue, Jun 12, 2018 at 10:10:51AM +0530, Rajendra Nayak wrote:
>> The RPMh Power domain driver aggregates the corner votes from various
>> consumers for the ARC resources and communicates it to RPMh.
>>
>> We also add data for all power domains on sdm845 SoC as part of the patch.
>> The driver can be extended to support other SoCs which support RPMh
>>
>> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
>> ---
>>  drivers/soc/qcom/Kconfig                |   9 +
>>  drivers/soc/qcom/Makefile               |   1 +
>>  drivers/soc/qcom/rpmhpd.c               | 427 ++++++++++++++++++++++++
>>  include/dt-bindings/power/qcom-rpmhpd.h |  31 ++
> 
> These includes should go with the binding.

will move them to the bindings patch when I resend, thanks.

> 
>>  4 files changed, 468 insertions(+)
>>  create mode 100644 drivers/soc/qcom/rpmhpd.c
>>  create mode 100644 include/dt-bindings/power/qcom-rpmhpd.h
>>
>> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
>> index 5c54931a7b99..7cb7eba2b997 100644
>> --- a/drivers/soc/qcom/Kconfig
>> +++ b/drivers/soc/qcom/Kconfig
>> @@ -74,6 +74,15 @@ config QCOM_RMTFS_MEM
>>  
>>  	  Say y here if you intend to boot the modem remoteproc.
>>  
>> +config QCOM_RPMHPD
>> +	tristate "Qualcomm RPMh Power domain driver"
>> +	depends on QCOM_RPMH && QCOM_COMMAND_DB
>> +	help
>> +	  QCOM RPMh Power domain driver to support power-domains with
>> +	  performance states. The driver communicates a performance state
>> +	  value to RPMh which then translates it into corresponding voltage
>> +	  for the voltage rail.
>> +
>>  config QCOM_RPMPD
>>  	tristate "Qualcomm RPM Power domain driver"
>>  	depends on MFD_QCOM_RPM && QCOM_SMD_RPM
>> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
>> index 9550c170de93..499513f63bef 100644
>> --- a/drivers/soc/qcom/Makefile
>> +++ b/drivers/soc/qcom/Makefile
>> @@ -16,3 +16,4 @@ obj-$(CONFIG_QCOM_SMSM)	+= smsm.o
>>  obj-$(CONFIG_QCOM_WCNSS_CTRL) += wcnss_ctrl.o
>>  obj-$(CONFIG_QCOM_APR) += apr.o
>>  obj-$(CONFIG_QCOM_RPMPD) += rpmpd.o
>> +obj-$(CONFIG_QCOM_RPMHPD) += rpmhpd.o
>> diff --git a/drivers/soc/qcom/rpmhpd.c b/drivers/soc/qcom/rpmhpd.c
>> new file mode 100644
>> index 000000000000..7083ec1590ff
>> --- /dev/null
>> +++ b/drivers/soc/qcom/rpmhpd.c
>> @@ -0,0 +1,427 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/* Copyright (c) 2018, The Linux Foundation. All rights reserved.*/
>> +
>> +#include <linux/err.h>
>> +#include <linux/export.h>
>> +#include <linux/init.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/mutex.h>
>> +#include <linux/pm_domain.h>
>> +#include <linux/slab.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pm_opp.h>
>> +#include <soc/qcom/cmd-db.h>
>> +#include <soc/qcom/rpmh.h>
>> +#include <dt-bindings/power/qcom-rpmhpd.h>
>> +
>> +#define domain_to_rpmhpd(domain) container_of(domain, struct rpmhpd, pd)
>> +
>> +#define DEFINE_RPMHPD_AO(_platform, _name, _active)			\
>> +	static struct rpmhpd _platform##_##_active;			\
>> +	static struct rpmhpd _platform##_##_name = {			\
>> +		.pd = {	.name = #_name,	},				\
>> +		.peer = &_platform##_##_active,				\
>> +		.res_name = #_name".lvl",				\
>> +		.valid_state_mask = (BIT(RPMH_ACTIVE_ONLY_STATE) |	\
>> +					BIT(RPMH_WAKE_ONLY_STATE) |	\
>> +					BIT(RPMH_SLEEP_STATE)),		\
>> +	};								\
>> +	static struct rpmhpd _platform##_##_active = {			\
>> +		.pd = { .name = #_active, },				\
>> +		.peer = &_platform##_##_name,				\
>> +		.active_only = true,					\
>> +		.res_name = #_name".lvl",				\
>> +		.valid_state_mask = (BIT(RPMH_ACTIVE_ONLY_STATE) |	\
>> +					BIT(RPMH_WAKE_ONLY_STATE) |	\
>> +					BIT(RPMH_SLEEP_STATE)),		\
>> +	}
>> +
>> +#define DEFINE_RPMHPD(_platform, _name)					\
>> +	static struct rpmhpd _platform##_##_name = {			\
>> +		.pd = { .name = #_name, },				\
>> +		.res_name = #_name".lvl",				\
>> +		.valid_state_mask = BIT(RPMH_ACTIVE_ONLY_STATE),	\
>> +	}
>> +
>> +/*
>> + * This is the number of bytes used for each command DB aux data entry of an
>> + * ARC resource.
>> + */
>> +#define RPMH_ARC_LEVEL_SIZE	2
>> +#define RPMH_ARC_MAX_LEVELS	16
>> +
>> +struct rpmhpd {
>> +	struct device	*dev;
>> +	struct generic_pm_domain pd;
>> +	struct rpmhpd	*peer;
>> +	const bool	active_only;
>> +	unsigned int	corner;
>> +	unsigned int	active_corner;
>> +	u32		level[RPMH_ARC_MAX_LEVELS];
>> +	int		level_count;
>> +	bool		enabled;
>> +	const char	*res_name;
>> +	u32		addr;
>> +	u8		valid_state_mask;
>> +};
>> +
>> +struct rpmhpd_desc {
>> +	struct rpmhpd **rpmhpds;
>> +	size_t num_pds;
>> +};
>> +
>> +static DEFINE_MUTEX(rpmhpd_lock);
>> +
>> +/* sdm845 RPMh Power domains */
>> +DEFINE_RPMHPD(sdm845, ebi);
>> +DEFINE_RPMHPD_AO(sdm845, mx, mx_ao);
>> +DEFINE_RPMHPD_AO(sdm845, cx, cx_ao);
>> +DEFINE_RPMHPD(sdm845, lmx);
>> +DEFINE_RPMHPD(sdm845, lcx);
>> +DEFINE_RPMHPD(sdm845, gfx);
>> +DEFINE_RPMHPD(sdm845, mss);
>> +
>> +static struct rpmhpd *sdm845_rpmhpds[] = {
>> +	[SDM845_EBI] = &sdm845_ebi,
>> +	[SDM845_MX] = &sdm845_mx,
>> +	[SDM845_MX_AO] = &sdm845_mx_ao,
>> +	[SDM845_CX] = &sdm845_cx,
>> +	[SDM845_CX_AO] = &sdm845_cx_ao,
>> +	[SDM845_LMX] = &sdm845_lmx,
>> +	[SDM845_LCX] = &sdm845_lcx,
>> +	[SDM845_GFX] = &sdm845_gfx,
>> +	[SDM845_MSS] = &sdm845_mss,
>> +};
>> +
>> +static const struct rpmhpd_desc sdm845_desc = {
>> +	.rpmhpds = sdm845_rpmhpds,
>> +	.num_pds = ARRAY_SIZE(sdm845_rpmhpds),
>> +};
>> +
>> +static const struct of_device_id rpmhpd_match_table[] = {
>> +	{ .compatible = "qcom,sdm845-rpmhpd", .data = &sdm845_desc },
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(of, rpmhpd_match_table);
>> +
>> +static int rpmhpd_send_corner(struct rpmhpd *pd, int state,
>> +			      unsigned int corner, bool sync)
>> +{
>> +	struct tcs_cmd cmd = {
>> +		.addr = pd->addr,
>> +		.data = corner,
>> +	};
>> +
>> +	if (sync)
>> +		return rpmh_write(pd->dev, state, &cmd, 1);
>> +	else
>> +		return rpmh_write_async(pd->dev, state, &cmd, 1);
>> +}
>> +
>> +static int rpmhpd_send_corner_sync(struct rpmhpd *pd, int state,
>> +				   unsigned int corner)
>> +{
>> +	return rpmhpd_send_corner(pd, state, corner, true);
>> +}
>> +
>> +static int rpmhpd_send_corner_async(struct rpmhpd *pd, int state,
>> +				    unsigned int corner)
>> +{
>> +	return rpmhpd_send_corner(pd, state, corner, false);
>> +};
>> +
>> +static void to_active_sleep(struct rpmhpd *pd, unsigned int corner,
>> +			    unsigned int *active, unsigned int *sleep)
>> +{
>> +	*active = corner;
>> +
>> +	if (pd->active_only)
>> +		*sleep = 0;
>> +	else
>> +		*sleep = *active;
>> +}
>> +
>> +/*
>> + * This function is used to aggregate the votes across the active only
>> + * resources and its peers. The aggregated votes are send to RPMh as
>> + * ACTIVE_ONLY votes (which take effect immediately), as WAKE_ONLY votes
>> + * (applied by RPMh on system wakeup) and as SLEEP votes (applied by RPMh
>> + * on system sleep).
>> + * We send ACTIVE_ONLY votes for resources without any peers. For others,
>> + * which have an active only peer, all 3 Votes are sent.
>> + */
>> +static int rpmhpd_aggregate_corner(struct rpmhpd *pd, unsigned int corner)
>> +{
>> +	int ret = -EINVAL;
>> +	struct rpmhpd *peer = pd->peer;
>> +	unsigned int active_corner, sleep_corner;
>> +	unsigned int this_active_corner = 0, this_sleep_corner = 0;
>> +	unsigned int peer_active_corner = 0, peer_sleep_corner = 0;
>> +
>> +	to_active_sleep(pd, corner, &this_active_corner, &this_sleep_corner);
>> +
>> +	if (peer && peer->enabled)
>> +		to_active_sleep(peer, peer->corner, &peer_active_corner,
>> +				&peer_sleep_corner);
>> +
>> +	active_corner = max(this_active_corner, peer_active_corner);
>> +
>> +	if (pd->valid_state_mask & BIT(RPMH_ACTIVE_ONLY_STATE)) {
>> +		/*
>> +		 * Wait for an ack only when we are increasing the
>> +		 * perf state of the power domain
>> +		 */
>> +		if (active_corner > pd->active_corner)
>> +			ret = rpmhpd_send_corner_sync(pd,
>> +						      RPMH_ACTIVE_ONLY_STATE,
>> +						      active_corner);
>> +		else
>> +			ret = rpmhpd_send_corner_async(pd,
>> +						       RPMH_ACTIVE_ONLY_STATE,
>> +						       active_corner);
>> +		if (ret)
>> +			return ret;
>> +		pd->active_corner = active_corner;
>> +		if (peer)
>> +			peer->active_corner = active_corner;
>> +	}
>> +
>> +	if (pd->valid_state_mask & BIT(RPMH_WAKE_ONLY_STATE)) {
>> +		ret = rpmhpd_send_corner_async(pd, RPMH_WAKE_ONLY_STATE,
>> +					       active_corner);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	sleep_corner = max(this_sleep_corner, peer_sleep_corner);
>> +
>> +	if (pd->valid_state_mask & BIT(RPMH_SLEEP_STATE))
>> +		ret = rpmhpd_send_corner_async(pd, RPMH_SLEEP_STATE,
>> +					       sleep_corner);
>> +
>> +	return ret;
>> +}
>> +
>> +static int rpmhpd_power_on(struct generic_pm_domain *domain)
>> +{
>> +	struct rpmhpd *pd = domain_to_rpmhpd(domain);
>> +	int ret = 0;
>> +
>> +	mutex_lock(&rpmhpd_lock);
>> +
>> +	pd->enabled = true;
>> +
>> +	if (pd->corner)
>> +		ret = rpmhpd_aggregate_corner(pd, pd->corner);
>> +
>> +	mutex_unlock(&rpmhpd_lock);
>> +
>> +	return ret;
>> +}
>> +
>> +static int rpmhpd_power_off(struct generic_pm_domain *domain)
>> +{
>> +	struct rpmhpd *pd = domain_to_rpmhpd(domain);
>> +	int ret = 0;
>> +
>> +	mutex_lock(&rpmhpd_lock);
>> +
>> +	if (pd->level[0] == 0)
>> +		ret = rpmhpd_aggregate_corner(pd, 0);
>> +
>> +	if (!ret)
>> +		pd->enabled = false;
>> +
>> +	mutex_unlock(&rpmhpd_lock);
>> +
>> +	return ret;
>> +}
>> +
>> +static int rpmhpd_set_performance(struct generic_pm_domain *domain,
>> +				  unsigned int state)
>> +{
>> +	struct rpmhpd *pd = domain_to_rpmhpd(domain);
>> +	int ret = 0, i;
>> +
>> +	mutex_lock(&rpmhpd_lock);
>> +
>> +	for (i = 0; i < pd->level_count; i++)
>> +		if (state <= pd->level[i])
>> +			break;
>> +
>> +	if (i == pd->level_count) {
>> +		ret = -EINVAL;
>> +		dev_err(pd->dev, "invalid state=%u for domain %s",
>> +			state, pd->pd.name);
>> +			goto out;
>> +	}
>> +
>> +	pd->corner = i;
>> +
>> +	if (!pd->enabled)
>> +		goto out;
>> +
>> +	ret = rpmhpd_aggregate_corner(pd, i);
>> +out:
>> +	mutex_unlock(&rpmhpd_lock);
>> +
>> +	return ret;
>> +}
>> +
>> +static unsigned int rpmhpd_get_performance(struct generic_pm_domain *genpd,
>> +					   struct dev_pm_opp *opp)
>> +{
>> +	struct device_node *np;
>> +	unsigned int corner = 0;
>> +
>> +	np = dev_pm_opp_get_of_node(opp);
>> +	if (of_property_read_u32(np, "qcom,level", &corner)) {
>> +		pr_err("%s: missing 'qcom,level' property\n", __func__);
>> +		return 0;
>> +	}
>> +
>> +	of_node_put(np);
>> +
>> +	return corner;
>> +}
>> +
>> +static int rpmhpd_update_level_mapping(struct rpmhpd *rpmhpd)
>> +{
>> +	int i, j, len, ret;
>> +	u8 buf[RPMH_ARC_MAX_LEVELS * RPMH_ARC_LEVEL_SIZE];
>> +
>> +	len = cmd_db_read_aux_data_len(rpmhpd->res_name);
>> +	if (len <= 0)
>> +		return len;
>> +
>> +	if (len > RPMH_ARC_MAX_LEVELS * RPMH_ARC_LEVEL_SIZE)
>> +		return -EINVAL;
>> +
>> +	ret = cmd_db_read_aux_data(rpmhpd->res_name, buf, len);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	rpmhpd->level_count = len / RPMH_ARC_LEVEL_SIZE;
>> +
>> +	for (i = 0; i < rpmhpd->level_count; i++) {
>> +		rpmhpd->level[i] = 0;
>> +		for (j = 0; j < RPMH_ARC_LEVEL_SIZE; j++)
>> +			rpmhpd->level[i] |=
>> +				buf[i * RPMH_ARC_LEVEL_SIZE + j] << (8 * j);
>> +
>> +		/*
>> +		 * The AUX data may be zero padded.  These 0 valued entries at
>> +		 * the end of the map must be ignored.
>> +		 */
>> +		if (i > 0 && rpmhpd->level[i] == 0) {
>> +			rpmhpd->level_count = i;
>> +			break;
>> +		}
>> +		pr_dbg("%s: ARC hlvl=%2d --> vlvl=%4u\n", rpmhpd->res_name, i,
>> +		       rpmhpd->level[i]);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int rpmhpd_probe(struct platform_device *pdev)
>> +{
>> +	int i, ret;
>> +	size_t num;
>> +	struct genpd_onecell_data *data;
>> +	struct rpmhpd **rpmhpds;
>> +	const struct rpmhpd_desc *desc;
>> +
>> +	desc = of_device_get_match_data(&pdev->dev);
>> +	if (!desc)
>> +		return -EINVAL;
>> +
>> +	rpmhpds = desc->rpmhpds;
>> +	num = desc->num_pds;
>> +
>> +	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
>> +	if (!data)
>> +		return -ENOMEM;
>> +
>> +	data->domains = devm_kcalloc(&pdev->dev, num, sizeof(*data->domains),
>> +				     GFP_KERNEL);
>> +	data->num_domains = num;
>> +
>> +	ret = cmd_db_ready();
>> +	if (ret) {
>> +		if (ret != -EPROBE_DEFER)
>> +			dev_err(&pdev->dev, "Command DB unavailable, ret=%d\n",
>> +				ret);
>> +		return ret;
>> +	}
>> +
>> +	for (i = 0; i < num; i++) {
>> +		if (!rpmhpds[i]) {
>> +			dev_warn(&pdev->dev, "rpmhpds[] with empty entry at index=%d\n",
>> +				 i);
>> +			continue;
>> +		}
>> +
>> +		rpmhpds[i]->dev = &pdev->dev;
>> +		rpmhpds[i]->addr = cmd_db_read_addr(rpmhpds[i]->res_name);
>> +		if (!rpmhpds[i]->addr) {
>> +			dev_err(&pdev->dev, "Could not find RPMh address for resource %s\n",
>> +				rpmhpds[i]->res_name);
>> +			return -ENODEV;
>> +		}
>> +
>> +		ret = cmd_db_read_slave_id(rpmhpds[i]->res_name);
>> +		if (ret != CMD_DB_HW_ARC) {
>> +			dev_err(&pdev->dev, "RPMh slave ID mismatch\n");
>> +			return -EINVAL;
>> +		}
>> +
>> +		ret = rpmhpd_update_level_mapping(rpmhpds[i]);
>> +		if (ret)
>> +			return ret;
>> +
>> +		rpmhpds[i]->pd.power_off = rpmhpd_power_off;
>> +		rpmhpds[i]->pd.power_on = rpmhpd_power_on;
>> +		rpmhpds[i]->pd.set_performance_state = rpmhpd_set_performance;
>> +		rpmhpds[i]->pd.opp_to_performance_state = rpmhpd_get_performance;
>> +		pm_genpd_init(&rpmhpds[i]->pd, NULL, true);
>> +
>> +		data->domains[i] = &rpmhpds[i]->pd;
>> +	}
>> +
>> +	return of_genpd_add_provider_onecell(pdev->dev.of_node, data);
>> +}
>> +
>> +static int rpmhpd_remove(struct platform_device *pdev)
>> +{
>> +	of_genpd_del_provider(pdev->dev.of_node);
>> +	return 0;
>> +}
>> +
>> +static struct platform_driver rpmhpd_driver = {
>> +	.driver = {
>> +		.name = "qcom-rpmhpd",
>> +		.of_match_table = rpmhpd_match_table,
>> +	},
>> +	.probe = rpmhpd_probe,
>> +	.remove = rpmhpd_remove,
>> +};
>> +
>> +static int __init rpmhpd_init(void)
>> +{
>> +	return platform_driver_register(&rpmhpd_driver);
>> +}
>> +core_initcall(rpmhpd_init);
>> +
>> +static void __exit rpmhpd_exit(void)
>> +{
>> +	platform_driver_unregister(&rpmhpd_driver);
>> +}
>> +module_exit(rpmhpd_exit);
>> +
>> +MODULE_DESCRIPTION("Qualcomm Technologies, Inc. RPMh Power Domain Driver");
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_ALIAS("platform:qcom-rpmhpd");
>> diff --git a/include/dt-bindings/power/qcom-rpmhpd.h b/include/dt-bindings/power/qcom-rpmhpd.h
>> new file mode 100644
>> index 000000000000..b01ae2452603
>> --- /dev/null
>> +++ b/include/dt-bindings/power/qcom-rpmhpd.h
>> @@ -0,0 +1,31 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/* Copyright (c) 2018, The Linux Foundation. All rights reserved. */
>> +
>> +#ifndef _DT_BINDINGS_POWER_QCOM_RPMHPD_H
>> +#define _DT_BINDINGS_POWER_QCOM_RPMHPD_H
>> +
>> +/* SDM845 Power Domain Indexes */
>> +#define SDM845_EBI	0
>> +#define SDM845_MX	1
>> +#define SDM845_MX_AO	2
>> +#define SDM845_CX	3
>> +#define SDM845_CX_AO	4
>> +#define SDM845_LMX	5
>> +#define SDM845_LCX	6
>> +#define SDM845_GFX	7
>> +#define SDM845_MSS	8
>> +
>> +/* SDM845 Power Domain performance levels */
>> +#define RPMH_REGULATOR_LEVEL_OFF	0
>> +#define RPMH_REGULATOR_LEVEL_RETENTION	16
>> +#define RPMH_REGULATOR_LEVEL_MIN_SVS	48
>> +#define RPMH_REGULATOR_LEVEL_LOW_SVS	64
>> +#define RPMH_REGULATOR_LEVEL_SVS	128
>> +#define RPMH_REGULATOR_LEVEL_SVS_L1	192
>> +#define RPMH_REGULATOR_LEVEL_NOM	256
>> +#define RPMH_REGULATOR_LEVEL_NOM_L1	320
>> +#define RPMH_REGULATOR_LEVEL_NOM_L2	336
>> +#define RPMH_REGULATOR_LEVEL_TURBO	384
>> +#define RPMH_REGULATOR_LEVEL_TURBO_L1	416
>> +
>> +#endif
>> -- 
>> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
>> of Code Aurora Forum, hosted by The Linux Foundation
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe devicetree" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH v3 6/7] soc: qcom: Add RPMh Power domain driver

[Matthias Kaehlcke]

Hi,

On Tue, Jun 12, 2018 at 10:10:51AM +0530, Rajendra Nayak wrote:
> The RPMh Power domain driver aggregates the corner votes from various
> consumers for the ARC resources and communicates it to RPMh.
> 
> We also add data for all power domains on sdm845 SoC as part of the patch.
> The driver can be extended to support other SoCs which support RPMh
> 
> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
> ---
>  drivers/soc/qcom/Kconfig                |   9 +
>  drivers/soc/qcom/Makefile               |   1 +
>  drivers/soc/qcom/rpmhpd.c               | 427 ++++++++++++++++++++++++
>  include/dt-bindings/power/qcom-rpmhpd.h |  31 ++
>  4 files changed, 468 insertions(+)
>  create mode 100644 drivers/soc/qcom/rpmhpd.c
>  create mode 100644 include/dt-bindings/power/qcom-rpmhpd.h
> 
> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> index 5c54931a7b99..7cb7eba2b997 100644
> --- a/drivers/soc/qcom/Kconfig
> +++ b/drivers/soc/qcom/Kconfig
> @@ -74,6 +74,15 @@ config QCOM_RMTFS_MEM
>  
>  	  Say y here if you intend to boot the modem remoteproc.
>  
> +config QCOM_RPMHPD
> +	tristate "Qualcomm RPMh Power domain driver"
> +	depends on QCOM_RPMH && QCOM_COMMAND_DB
> +	help
> +	  QCOM RPMh Power domain driver to support power-domains with
> +	  performance states. The driver communicates a performance state
> +	  value to RPMh which then translates it into corresponding voltage
> +	  for the voltage rail.
> +
>  config QCOM_RPMPD
>  	tristate "Qualcomm RPM Power domain driver"
>  	depends on MFD_QCOM_RPM && QCOM_SMD_RPM
> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
> index 9550c170de93..499513f63bef 100644
> --- a/drivers/soc/qcom/Makefile
> +++ b/drivers/soc/qcom/Makefile
> @@ -16,3 +16,4 @@ obj-$(CONFIG_QCOM_SMSM)	+= smsm.o
>  obj-$(CONFIG_QCOM_WCNSS_CTRL) += wcnss_ctrl.o
>  obj-$(CONFIG_QCOM_APR) += apr.o
>  obj-$(CONFIG_QCOM_RPMPD) += rpmpd.o
> +obj-$(CONFIG_QCOM_RPMHPD) += rpmhpd.o
> diff --git a/drivers/soc/qcom/rpmhpd.c b/drivers/soc/qcom/rpmhpd.c
> new file mode 100644
> index 000000000000..7083ec1590ff
> --- /dev/null
> +++ b/drivers/soc/qcom/rpmhpd.c
> @@ -0,0 +1,427 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2018, The Linux Foundation. All rights reserved.*/
> +
> +#include <linux/err.h>
> +#include <linux/export.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/pm_domain.h>
> +#include <linux/slab.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_opp.h>
> +#include <soc/qcom/cmd-db.h>
> +#include <soc/qcom/rpmh.h>
> +#include <dt-bindings/power/qcom-rpmhpd.h>
> +
> +#define domain_to_rpmhpd(domain) container_of(domain, struct rpmhpd, pd)
> +
> +#define DEFINE_RPMHPD_AO(_platform, _name, _active)			\
> +	static struct rpmhpd _platform##_##_active;			\
> +	static struct rpmhpd _platform##_##_name = {			\
> +		.pd = {	.name = #_name,	},				\
> +		.peer = &_platform##_##_active,				\
> +		.res_name = #_name".lvl",				\
> +		.valid_state_mask = (BIT(RPMH_ACTIVE_ONLY_STATE) |	\
> +					BIT(RPMH_WAKE_ONLY_STATE) |	\
> +					BIT(RPMH_SLEEP_STATE)),		\
> +	};								\
> +	static struct rpmhpd _platform##_##_active = {			\
> +		.pd = { .name = #_active, },				\
> +		.peer = &_platform##_##_name,				\
> +		.active_only = true,					\
> +		.res_name = #_name".lvl",				\
> +		.valid_state_mask = (BIT(RPMH_ACTIVE_ONLY_STATE) |	\
> +					BIT(RPMH_WAKE_ONLY_STATE) |	\
> +					BIT(RPMH_SLEEP_STATE)),		\
> +	}
> +
> +#define DEFINE_RPMHPD(_platform, _name)					\
> +	static struct rpmhpd _platform##_##_name = {			\
> +		.pd = { .name = #_name, },				\
> +		.res_name = #_name".lvl",				\
> +		.valid_state_mask = BIT(RPMH_ACTIVE_ONLY_STATE),	\
> +	}

Perhaps describe briefly the concept of an AO (active-only) and a peer
domain. It is not necessarily evident why an AO domain would have
WAKE_ONLY and SLEEP_ONLY as valid states, while the non-AO domain has
ACTIVE_ONLY as it's only state.

> +/*
> + * This function is used to aggregate the votes across the active only
> + * resources and its peers. The aggregated votes are send to RPMh as

s/send/sent/

> + * ACTIVE_ONLY votes (which take effect immediately), as WAKE_ONLY votes
> + * (applied by RPMh on system wakeup) and as SLEEP votes (applied by RPMh
> + * on system sleep).
> + * We send ACTIVE_ONLY votes for resources without any peers. For others,
> + * which have an active only peer, all 3 Votes are sent.

s/Votes/votes/

> +static int rpmhpd_probe(struct platform_device *pdev)
> +{
> +	int i, ret;
> +	size_t num;

nit: naming this num_pds would slightly improve readability (e.g. make
it evident that the for loop iterates over the PDs).

> +	struct genpd_onecell_data *data;
> +	struct rpmhpd **rpmhpds;
> +	const struct rpmhpd_desc *desc;
> +
> +	desc = of_device_get_match_data(&pdev->dev);
> +	if (!desc)
> +		return -EINVAL;
> +
> +	rpmhpds = desc->rpmhpds;
> +	num = desc->num_pds;
> +
> +	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	data->domains = devm_kcalloc(&pdev->dev, num, sizeof(*data->domains),
> +				     GFP_KERNEL);

Check for NULL?

Thanks

Matthias

Re: [PATCH v3 6/7] soc: qcom: Add RPMh Power domain driver

[Rajendra Nayak]

On 06/13/2018 01:12 AM, Matthias Kaehlcke wrote:
> Hi,
> 
> On Tue, Jun 12, 2018 at 10:10:51AM +0530, Rajendra Nayak wrote:
>> The RPMh Power domain driver aggregates the corner votes from various
>> consumers for the ARC resources and communicates it to RPMh.
>>
>> We also add data for all power domains on sdm845 SoC as part of the patch.
>> The driver can be extended to support other SoCs which support RPMh
>>
>> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
>> ---
>>  drivers/soc/qcom/Kconfig                |   9 +
>>  drivers/soc/qcom/Makefile               |   1 +
>>  drivers/soc/qcom/rpmhpd.c               | 427 ++++++++++++++++++++++++
>>  include/dt-bindings/power/qcom-rpmhpd.h |  31 ++
>>  4 files changed, 468 insertions(+)
>>  create mode 100644 drivers/soc/qcom/rpmhpd.c
>>  create mode 100644 include/dt-bindings/power/qcom-rpmhpd.h
>>
>> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
>> index 5c54931a7b99..7cb7eba2b997 100644
>> --- a/drivers/soc/qcom/Kconfig
>> +++ b/drivers/soc/qcom/Kconfig
>> @@ -74,6 +74,15 @@ config QCOM_RMTFS_MEM
>>  
>>  	  Say y here if you intend to boot the modem remoteproc.
>>  
>> +config QCOM_RPMHPD
>> +	tristate "Qualcomm RPMh Power domain driver"
>> +	depends on QCOM_RPMH && QCOM_COMMAND_DB
>> +	help
>> +	  QCOM RPMh Power domain driver to support power-domains with
>> +	  performance states. The driver communicates a performance state
>> +	  value to RPMh which then translates it into corresponding voltage
>> +	  for the voltage rail.
>> +
>>  config QCOM_RPMPD
>>  	tristate "Qualcomm RPM Power domain driver"
>>  	depends on MFD_QCOM_RPM && QCOM_SMD_RPM
>> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
>> index 9550c170de93..499513f63bef 100644
>> --- a/drivers/soc/qcom/Makefile
>> +++ b/drivers/soc/qcom/Makefile
>> @@ -16,3 +16,4 @@ obj-$(CONFIG_QCOM_SMSM)	+= smsm.o
>>  obj-$(CONFIG_QCOM_WCNSS_CTRL) += wcnss_ctrl.o
>>  obj-$(CONFIG_QCOM_APR) += apr.o
>>  obj-$(CONFIG_QCOM_RPMPD) += rpmpd.o
>> +obj-$(CONFIG_QCOM_RPMHPD) += rpmhpd.o
>> diff --git a/drivers/soc/qcom/rpmhpd.c b/drivers/soc/qcom/rpmhpd.c
>> new file mode 100644
>> index 000000000000..7083ec1590ff
>> --- /dev/null
>> +++ b/drivers/soc/qcom/rpmhpd.c
>> @@ -0,0 +1,427 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/* Copyright (c) 2018, The Linux Foundation. All rights reserved.*/
>> +
>> +#include <linux/err.h>
>> +#include <linux/export.h>
>> +#include <linux/init.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/mutex.h>
>> +#include <linux/pm_domain.h>
>> +#include <linux/slab.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pm_opp.h>
>> +#include <soc/qcom/cmd-db.h>
>> +#include <soc/qcom/rpmh.h>
>> +#include <dt-bindings/power/qcom-rpmhpd.h>
>> +
>> +#define domain_to_rpmhpd(domain) container_of(domain, struct rpmhpd, pd)
>> +
>> +#define DEFINE_RPMHPD_AO(_platform, _name, _active)			\
>> +	static struct rpmhpd _platform##_##_active;			\
>> +	static struct rpmhpd _platform##_##_name = {			\
>> +		.pd = {	.name = #_name,	},				\
>> +		.peer = &_platform##_##_active,				\
>> +		.res_name = #_name".lvl",				\
>> +		.valid_state_mask = (BIT(RPMH_ACTIVE_ONLY_STATE) |	\
>> +					BIT(RPMH_WAKE_ONLY_STATE) |	\
>> +					BIT(RPMH_SLEEP_STATE)),		\
>> +	};								\
>> +	static struct rpmhpd _platform##_##_active = {			\
>> +		.pd = { .name = #_active, },				\
>> +		.peer = &_platform##_##_name,				\
>> +		.active_only = true,					\
>> +		.res_name = #_name".lvl",				\
>> +		.valid_state_mask = (BIT(RPMH_ACTIVE_ONLY_STATE) |	\
>> +					BIT(RPMH_WAKE_ONLY_STATE) |	\
>> +					BIT(RPMH_SLEEP_STATE)),		\
>> +	}
>> +
>> +#define DEFINE_RPMHPD(_platform, _name)					\
>> +	static struct rpmhpd _platform##_##_name = {			\
>> +		.pd = { .name = #_name, },				\
>> +		.res_name = #_name".lvl",				\
>> +		.valid_state_mask = BIT(RPMH_ACTIVE_ONLY_STATE),	\
>> +	}
> 
> Perhaps describe briefly the concept of an AO (active-only) and a peer
> domain. It is not necessarily evident why an AO domain would have
> WAKE_ONLY and SLEEP_ONLY as valid states, while the non-AO domain has
> ACTIVE_ONLY as it's only state.

Sure, I'll add in more comments to describe this.

> 
>> +/*
>> + * This function is used to aggregate the votes across the active only
>> + * resources and its peers. The aggregated votes are send to RPMh as
> 
> s/send/sent/
> 
>> + * ACTIVE_ONLY votes (which take effect immediately), as WAKE_ONLY votes
>> + * (applied by RPMh on system wakeup) and as SLEEP votes (applied by RPMh
>> + * on system sleep).
>> + * We send ACTIVE_ONLY votes for resources without any peers. For others,
>> + * which have an active only peer, all 3 Votes are sent.
> 
> s/Votes/votes/
> 
>> +static int rpmhpd_probe(struct platform_device *pdev)
>> +{
>> +	int i, ret;
>> +	size_t num;
> 
> nit: naming this num_pds would slightly improve readability (e.g. make
> it evident that the for loop iterates over the PDs).

sure, will change

> 
>> +	struct genpd_onecell_data *data;
>> +	struct rpmhpd **rpmhpds;
>> +	const struct rpmhpd_desc *desc;
>> +
>> +	desc = of_device_get_match_data(&pdev->dev);
>> +	if (!desc)
>> +		return -EINVAL;
>> +
>> +	rpmhpds = desc->rpmhpds;
>> +	num = desc->num_pds;
>> +
>> +	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
>> +	if (!data)
>> +		return -ENOMEM;
>> +
>> +	data->domains = devm_kcalloc(&pdev->dev, num, sizeof(*data->domains),
>> +				     GFP_KERNEL);
> 
> Check for NULL?

yes, indeed, thanks for the review. I will fix all these up with a v4.


-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH v3 6/7] soc: qcom: Add RPMh Power domain driver

[David Collins]

Hello Rajendra,

On 06/11/2018 09:40 PM, Rajendra Nayak wrote:
> The RPMh Power domain driver aggregates the corner votes from various
> consumers for the ARC resources and communicates it to RPMh.
> 
> We also add data for all power domains on sdm845 SoC as part of the patch.
> The driver can be extended to support other SoCs which support RPMh
> 
> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
> ---
>  drivers/soc/qcom/Kconfig                |   9 +
>  drivers/soc/qcom/Makefile               |   1 +
>  drivers/soc/qcom/rpmhpd.c               | 427 ++++++++++++++++++++++++
>  include/dt-bindings/power/qcom-rpmhpd.h |  31 ++
>  4 files changed, 468 insertions(+)
>  create mode 100644 drivers/soc/qcom/rpmhpd.c
>  create mode 100644 include/dt-bindings/power/qcom-rpmhpd.h

This DT header file should be included in a DT binding patch that is
separate from the driver patch.


> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> index 5c54931a7b99..7cb7eba2b997 100644
> --- a/drivers/soc/qcom/Kconfig
> +++ b/drivers/soc/qcom/Kconfig
> @@ -74,6 +74,15 @@ config QCOM_RMTFS_MEM
>  
>  	  Say y here if you intend to boot the modem remoteproc.
>  
> +config QCOM_RPMHPD
> +	tristate "Qualcomm RPMh Power domain driver"
> +	depends on QCOM_RPMH && QCOM_COMMAND_DB
> +	help
> +	  QCOM RPMh Power domain driver to support power-domains with
> +	  performance states. The driver communicates a performance state
> +	  value to RPMh which then translates it into corresponding voltage
> +	  for the voltage rail.
> +
>  config QCOM_RPMPD
>  	tristate "Qualcomm RPM Power domain driver"
>  	depends on MFD_QCOM_RPM && QCOM_SMD_RPM
> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
> index 9550c170de93..499513f63bef 100644
> --- a/drivers/soc/qcom/Makefile
> +++ b/drivers/soc/qcom/Makefile
> @@ -16,3 +16,4 @@ obj-$(CONFIG_QCOM_SMSM)	+= smsm.o
>  obj-$(CONFIG_QCOM_WCNSS_CTRL) += wcnss_ctrl.o
>  obj-$(CONFIG_QCOM_APR) += apr.o
>  obj-$(CONFIG_QCOM_RPMPD) += rpmpd.o
> +obj-$(CONFIG_QCOM_RPMHPD) += rpmhpd.o
> diff --git a/drivers/soc/qcom/rpmhpd.c b/drivers/soc/qcom/rpmhpd.c
> new file mode 100644
> index 000000000000..7083ec1590ff
> --- /dev/null
> +++ b/drivers/soc/qcom/rpmhpd.c
> @@ -0,0 +1,427 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2018, The Linux Foundation. All rights reserved.*/
> +
> +#include <linux/err.h>
> +#include <linux/export.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/pm_domain.h>
> +#include <linux/slab.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_opp.h>
> +#include <soc/qcom/cmd-db.h>
> +#include <soc/qcom/rpmh.h>
> +#include <dt-bindings/power/qcom-rpmhpd.h>
> +
> +#define domain_to_rpmhpd(domain) container_of(domain, struct rpmhpd, pd)
> +
> +#define DEFINE_RPMHPD_AO(_platform, _name, _active)			\
> +	static struct rpmhpd _platform##_##_active;			\
> +	static struct rpmhpd _platform##_##_name = {			\
> +		.pd = {	.name = #_name,	},				\
> +		.peer = &_platform##_##_active,				\
> +		.res_name = #_name".lvl",				\
> +		.valid_state_mask = (BIT(RPMH_ACTIVE_ONLY_STATE) |	\
> +					BIT(RPMH_WAKE_ONLY_STATE) |	\
> +					BIT(RPMH_SLEEP_STATE)),		\
> +	};								\
> +	static struct rpmhpd _platform##_##_active = {			\
> +		.pd = { .name = #_active, },				\
> +		.peer = &_platform##_##_name,				\
> +		.active_only = true,					\
> +		.res_name = #_name".lvl",				\
> +		.valid_state_mask = (BIT(RPMH_ACTIVE_ONLY_STATE) |	\
> +					BIT(RPMH_WAKE_ONLY_STATE) |	\
> +					BIT(RPMH_SLEEP_STATE)),		\> +	}
> +
> +#define DEFINE_RPMHPD(_platform, _name)					\
> +	static struct rpmhpd _platform##_##_name = {			\
> +		.pd = { .name = #_name, },				\
> +		.res_name = #_name".lvl",				\
> +		.valid_state_mask = BIT(RPMH_ACTIVE_ONLY_STATE),	\
> +	}
> +
> +/*
> + * This is the number of bytes used for each command DB aux data entry of an
> + * ARC resource.
> + */
> +#define RPMH_ARC_LEVEL_SIZE	2
> +#define RPMH_ARC_MAX_LEVELS	16
> +


Would you mind adding a kernel-doc comment for here for struct rpmhpd?  I
think that would make the code clearer.  It would be good to mention the
numbering spaces for 'corner' and 'level' elements as well as the usage of
'peer' and 'active_only' elements.

> +struct rpmhpd {
> +	struct device	*dev;
> +	struct generic_pm_domain pd;
> +	struct rpmhpd	*peer;
> +	const bool	active_only;
> +	unsigned int	corner;
> +	unsigned int	active_corner> +	u32		level[RPMH_ARC_MAX_LEVELS];
> +	int		level_count;
> +	bool		enabled;
> +	const char	*res_name;
> +	u32		addr;
> +	u8		valid_state_mask;
> +};
> +
> +struct rpmhpd_desc {
> +	struct rpmhpd **rpmhpds;
> +	size_t num_pds;
> +};
> +
> +static DEFINE_MUTEX(rpmhpd_lock);
> +
> +/* sdm845 RPMh Power domains */
> +DEFINE_RPMHPD(sdm845, ebi);
> +DEFINE_RPMHPD_AO(sdm845, mx, mx_ao);
> +DEFINE_RPMHPD_AO(sdm845, cx, cx_ao);
> +DEFINE_RPMHPD(sdm845, lmx);
> +DEFINE_RPMHPD(sdm845, lcx);
> +DEFINE_RPMHPD(sdm845, gfx);
> +DEFINE_RPMHPD(sdm845, mss);
> +
> +static struct rpmhpd *sdm845_rpmhpds[] = {
> +	[SDM845_EBI] = &sdm845_ebi,
> +	[SDM845_MX] = &sdm845_mx,
> +	[SDM845_MX_AO] = &sdm845_mx_ao,
> +	[SDM845_CX] = &sdm845_cx,
> +	[SDM845_CX_AO] = &sdm845_cx_ao,
> +	[SDM845_LMX] = &sdm845_lmx,
> +	[SDM845_LCX] = &sdm845_lcx,
> +	[SDM845_GFX] = &sdm845_gfx,
> +	[SDM845_MSS] = &sdm845_mss,
> +};
> +
> +static const struct rpmhpd_desc sdm845_desc = {
> +	.rpmhpds = sdm845_rpmhpds,
> +	.num_pds = ARRAY_SIZE(sdm845_rpmhpds),
> +};
> +
> +static const struct of_device_id rpmhpd_match_table[] = {
> +	{ .compatible = "qcom,sdm845-rpmhpd", .data = &sdm845_desc },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, rpmhpd_match_table);
> +
> +static int rpmhpd_send_corner(struct rpmhpd *pd, int state,
> +			      unsigned int corner, bool sync)
> +{
> +	struct tcs_cmd cmd = {
> +		.addr = pd->addr,
> +		.data = corner,
> +	};
> +
> +	if (sync)
> +		return rpmh_write(pd->dev, state, &cmd, 1);
> +	else
> +		return rpmh_write_async(pd->dev, state, &cmd, 1);
> +}
> +
> +static int rpmhpd_send_corner_sync(struct rpmhpd *pd, int state,
> +				   unsigned int corner)
> +{
> +	return rpmhpd_send_corner(pd, state, corner, true);
> +}
> +
> +static int rpmhpd_send_corner_async(struct rpmhpd *pd, int state,
> +				    unsigned int corner)
> +{
> +	return rpmhpd_send_corner(pd, state, corner, false);
> +};

I'm not sure about the need for rpmhpd_send_corner_sync() and
rpmhpd_send_corner_async().  They are adding lines that aren't strictly
needed since rpmhpd_send_corner() could be called directly instead.  Doing
that could actually save some more lines in rpmhpd_aggregate_corner()
below as 'active_corner > pd->active_corner' could be passed as the 'sync'
argument so that the if statement isn't needed.  However, I also see the
utility in not having a magic bool in the calls below.  Let's see if other
reviewers have a preference about it one way or the other.


> +static void to_active_sleep(struct rpmhpd *pd, unsigned int corner,
> +			    unsigned int *active, unsigned int *sleep)
> +{
> +	*active = corner;
> +
> +	if (pd->active_only)
> +		*sleep = 0;
> +	else
> +		*sleep = *active;
> +}
> +
> +/*
> + * This function is used to aggregate the votes across the active only
> + * resources and its peers. The aggregated votes are send to RPMh as
> + * ACTIVE_ONLY votes (which take effect immediately), as WAKE_ONLY votes
> + * (applied by RPMh on system wakeup) and as SLEEP votes (applied by RPMh
> + * on system sleep).
> + * We send ACTIVE_ONLY votes for resources without any peers. For others,
> + * which have an active only peer, all 3 Votes are sent.
> + */
> +static int rpmhpd_aggregate_corner(struct rpmhpd *pd, unsigned int corner)
> +{
> +	int ret = -EINVAL;
> +	struct rpmhpd *peer = pd->peer;
> +	unsigned int active_corner, sleep_corner;
> +	unsigned int this_active_corner = 0, this_sleep_corner = 0;
> +	unsigned int peer_active_corner = 0, peer_sleep_corner = 0;
> +
> +	to_active_sleep(pd, corner, &this_active_corner, &this_sleep_corner);
> +
> +	if (peer && peer->enabled)
> +		to_active_sleep(peer, peer->corner, &peer_active_corner,
> +				&peer_sleep_corner);
> +
> +	active_corner = max(this_active_corner, peer_active_corner);
> +
> +	if (pd->valid_state_mask & BIT(RPMH_ACTIVE_ONLY_STATE)) {

This condition will always be true, so this check can be removed.


> +		/*
> +		 * Wait for an ack only when we are increasing the
> +		 * perf state of the power domain
> +		 */
> +		if (active_corner > pd->active_corner)
> +			ret = rpmhpd_send_corner_sync(pd,
> +						      RPMH_ACTIVE_ONLY_STATE,
> +						      active_corner);
> +		else
> +			ret = rpmhpd_send_corner_async(pd,
> +						       RPMH_ACTIVE_ONLY_STATE,
> +						       active_corner);
> +		if (ret)
> +			return ret;
> +		pd->active_corner = active_corner;
> +		if (peer)
> +			peer->active_corner = active_corner;
> +	}
> +
> +	if (pd->valid_state_mask & BIT(RPMH_WAKE_ONLY_STATE)) {

This check and the one below could be changed to simply:

    if (pd->peer) {

That way, the valid_state_mask element can be removed from struct rpmhpd
and the two if blocks can be consolidated together.  I think that
valid_state_mask is making the code more confusing at this point than it
is at verbosely describing the aggregation semantics.


> +		ret = rpmhpd_send_corner_async(pd, RPMH_WAKE_ONLY_STATE,
> +					       active_corner);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	sleep_corner = max(this_sleep_corner, peer_sleep_corner);
> +
> +	if (pd->valid_state_mask & BIT(RPMH_SLEEP_STATE))
> +		ret = rpmhpd_send_corner_async(pd, RPMH_SLEEP_STATE,
> +					       sleep_corner);
> +
> +	return ret;
> +}
> +
> +static int rpmhpd_power_on(struct generic_pm_domain *domain)
> +{
> +	struct rpmhpd *pd = domain_to_rpmhpd(domain);
> +	int ret = 0;
> +
> +	mutex_lock(&rpmhpd_lock);
> +
> +	pd->enabled = true;

It would probably be better to remove this line and add the following
after the rpmhpd_aggregate_corner() call:

if (!ret)
	pd->enabled = true;

Only the peer 'enabled' value is checked in rpmhpd_aggregate_corner() so
architecturally, it doesn't matter if the value is configured before or
after the call.


> +
> +	if (pd->corner)
> +		ret = rpmhpd_aggregate_corner(pd, pd->corner);
> +
> +	mutex_unlock(&rpmhpd_lock);
> +
> +	return ret;
> +}
> +
> +static int rpmhpd_power_off(struct generic_pm_domain *domain)
> +{
> +	struct rpmhpd *pd = domain_to_rpmhpd(domain);
> +	int ret = 0;
> +
> +	mutex_lock(&rpmhpd_lock);
> +
> +	if (pd->level[0] == 0)
> +		ret = rpmhpd_aggregate_corner(pd, 0);

I'm not sure that we want to have the 'pd->level[0] == 0' check,
especially when considering aggregation with the peer pd.  I understand
its intention to try to keep enable state and level setting orthogonal.
However, as it stands now, the final request sent to hardware would differ
depending upon the order of calls.  Consider the following example.

Initial state:
pd->level[0] == 0
pd->corner = 5, pd->enabled = true, pd->active_only = false
pd->peer->corner = 7, pd->peer->enabled = true, pd->peer->active_only = true

Outstanding requests:
RPMH_ACTIVE_ONLY_STATE = 7, RPMH_WAKE_ONLY_STATE = 7, RPMH_SLEEP_STATE = 5

Case A:
	1. set pd->corner = 6
		--> new value request: RPMH_SLEEP_STATE = 6
		--> duplicate value requests: RPMH_ACTIVE_ONLY_STATE = 7,
			RPMH_WAKE_ONLY_STATE = 7
	2. power_off pd->peer
		--> no requests

	Final state:
	RPMH_ACTIVE_ONLY_STATE = 7
	RPMH_WAKE_ONLY_STATE = 7
	RPMH_SLEEP_STATE = 6

Case B:
	1. power_off pd->peer
		--> no requests
	2. set pd->corner = 6
		--> new value requests: RPMH_ACTIVE_ONLY_STATE = 6,
		       RPMH_WAKE_ONLY_STATE = 6, RPMH_SLEEP_STATE = 6

	Final state:
	RPMH_ACTIVE_ONLY_STATE = 6
	RPMH_WAKE_ONLY_STATE = 6
	RPMH_SLEEP_STATE = 6

Without the check, Linux would vote for the lowest supported level when
power_off is called.  This seems semantically reasonable given that the
consumer is ok with the power domain going fully off and that would be the
closest that we can get.


> +
> +	if (!ret)
> +		pd->enabled = false;
> +
> +	mutex_unlock(&rpmhpd_lock);
> +
> +	return ret;
> +}
> +
> +static int rpmhpd_set_performance(struct generic_pm_domain *domain,
> +				  unsigned int state)

The code might be a bit more readable if 'state' is changed to 'level'.

Also, is there a particular reason that this is named
rpmhpd_set_performance() instead of rpmhpd_set_performance_state()?


> +{
> +	struct rpmhpd *pd = domain_to_rpmhpd(domain);
> +	int ret = 0, i;
> +
> +	mutex_lock(&rpmhpd_lock);
> +
> +	for (i = 0; i < pd->level_count; i++)
> +		if (state <= pd->level[i])
> +			break;
> +
> +	if (i == pd->level_count) {
> +		ret = -EINVAL;
> +		dev_err(pd->dev, "invalid state=%u for domain %s",
> +			state, pd->pd.name);
> +			goto out;

One level of indentation should be removed from this line.


> +	}
> +
> +	pd->corner = i;
> +
> +	if (!pd->enabled)
> +		goto out;
> +
> +	ret = rpmhpd_aggregate_corner(pd, i);

Would it be worthwhile to roll back the pd->corner value in the case of an
error?


> +out:
> +	mutex_unlock(&rpmhpd_lock);
> +
> +	return ret;
> +}
> +
> +static unsigned int rpmhpd_get_performance(struct generic_pm_domain *genpd,
> +					   struct dev_pm_opp *opp)

Is there a particular reason that this is named rpmhpd_get_performance()
instead of rpmhpd_get_performance_state()?


> +{
> +	struct device_node *np;
> +	unsigned int corner = 0;

Please change 'corner' to 'level' for consistency.  In this driver "level"
values are in the vlvl RPMH_REGULATOR_LEVEL_* numbering space and "corner"
values are in the hlvl 0-15 numbering space.


> +
> +	np = dev_pm_opp_get_of_node(opp);
> +	if (of_property_read_u32(np, "qcom,level", &corner)) {
> +		pr_err("%s: missing 'qcom,level' property\n", __func__);
> +		return 0;
> +	}
> +
> +	of_node_put(np);
> +
> +	return corner;
> +}
> +
> +static int rpmhpd_update_level_mapping(struct rpmhpd *rpmhpd)
> +{
> +	int i, j, len, ret;
> +	u8 buf[RPMH_ARC_MAX_LEVELS * RPMH_ARC_LEVEL_SIZE];

Minor: It might look better to list buf[] first.


> +
> +	len = cmd_db_read_aux_data_len(rpmhpd->res_name);
> +	if (len <= 0)
> +		return len;
> +
> +	if (len > RPMH_ARC_MAX_LEVELS * RPMH_ARC_LEVEL_SIZE)
> +		return -EINVAL;

'else if' could be used here.


> +
> +	ret = cmd_db_read_aux_data(rpmhpd->res_name, buf, len);
> +	if (ret < 0)
> +		return ret;
> +
> +	rpmhpd->level_count = len / RPMH_ARC_LEVEL_SIZE;
> +
> +	for (i = 0; i < rpmhpd->level_count; i++) {
> +		rpmhpd->level[i] = 0;
> +		for (j = 0; j < RPMH_ARC_LEVEL_SIZE; j++)
> +			rpmhpd->level[i] |=
> +				buf[i * RPMH_ARC_LEVEL_SIZE + j] << (8 * j);
> +
> +		/*
> +		 * The AUX data may be zero padded.  These 0 valued entries at
> +		 * the end of the map must be ignored.
> +		 */
> +		if (i > 0 && rpmhpd->level[i] == 0) {
> +			rpmhpd->level_count = i;
> +			break;
> +		}
> +		pr_dbg("%s: ARC hlvl=%2d --> vlvl=%4u\n", rpmhpd->res_name, i,
> +		       rpmhpd->level[i]);
> +	}
> +
> +	return 0;
> +}
> +
> +static int rpmhpd_probe(struct platform_device *pdev)
> +{
> +	int i, ret;
> +	size_t num;
> +	struct genpd_onecell_data *data;
> +	struct rpmhpd **rpmhpds;
> +	const struct rpmhpd_desc *desc;
> +
> +	desc = of_device_get_match_data(&pdev->dev);
> +	if (!desc)
> +		return -EINVAL;
> +
> +	rpmhpds = desc->rpmhpds;
> +	num = desc->num_pds;
> +
> +	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	data->domains = devm_kcalloc(&pdev->dev, num, sizeof(*data->domains),
> +				     GFP_KERNEL);
> +	data->num_domains = num;
> +
> +	ret = cmd_db_ready();
> +	if (ret) {
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(&pdev->dev, "Command DB unavailable, ret=%d\n",
> +				ret);
> +		return ret;
> +	}
> +
> +	for (i = 0; i < num; i++) {
> +		if (!rpmhpds[i]) {
> +			dev_warn(&pdev->dev, "rpmhpds[] with empty entry at index=%d\n",
> +				 i);

Minor: This could be simplified to:

dev_warn(&pdev->dev, "rpmhpds[%d] is empty\n", i);


> +			continue;
> +		}
> +
> +		rpmhpds[i]->dev = &pdev->dev;
> +		rpmhpds[i]->addr = cmd_db_read_addr(rpmhpds[i]->res_name);
> +		if (!rpmhpds[i]->addr) {
> +			dev_err(&pdev->dev, "Could not find RPMh address for resource %s\n",
> +				rpmhpds[i]->res_name);
> +			return -ENODEV;
> +		}
> +
> +		ret = cmd_db_read_slave_id(rpmhpds[i]->res_name);
> +		if (ret != CMD_DB_HW_ARC) {
> +			dev_err(&pdev->dev, "RPMh slave ID mismatch\n");
> +			return -EINVAL;
> +		}
> +
> +		ret = rpmhpd_update_level_mapping(rpmhpds[i]);
> +		if (ret)
> +			return ret;
> +
> +		rpmhpds[i]->pd.power_off = rpmhpd_power_off;
> +		rpmhpds[i]->pd.power_on = rpmhpd_power_on;
> +		rpmhpds[i]->pd.set_performance_state = rpmhpd_set_performance;
> +		rpmhpds[i]->pd.opp_to_performance_state = rpmhpd_get_performance;
> +		pm_genpd_init(&rpmhpds[i]->pd, NULL, true);
> +
> +		data->domains[i] = &rpmhpds[i]->pd;
> +	}
> +
> +	return of_genpd_add_provider_onecell(pdev->dev.of_node, data);
> +}
> +
> +static int rpmhpd_remove(struct platform_device *pdev)
> +{
> +	of_genpd_del_provider(pdev->dev.of_node);
> +	return 0;
> +}
> +
> +static struct platform_driver rpmhpd_driver = {
> +	.driver = {
> +		.name = "qcom-rpmhpd",
> +		.of_match_table = rpmhpd_match_table,
> +	},
> +	.probe = rpmhpd_probe,
> +	.remove = rpmhpd_remove,
> +};
> +
> +static int __init rpmhpd_init(void)
> +{
> +	return platform_driver_register(&rpmhpd_driver);
> +}
> +core_initcall(rpmhpd_init);
> +
> +static void __exit rpmhpd_exit(void)
> +{
> +	platform_driver_unregister(&rpmhpd_driver);
> +}
> +module_exit(rpmhpd_exit);
> +
> +MODULE_DESCRIPTION("Qualcomm Technologies, Inc. RPMh Power Domain Driver");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:qcom-rpmhpd");
> diff --git a/include/dt-bindings/power/qcom-rpmhpd.h b/include/dt-bindings/power/qcom-rpmhpd.h
> new file mode 100644
> index 000000000000..b01ae2452603
> --- /dev/null
> +++ b/include/dt-bindings/power/qcom-rpmhpd.h
> @@ -0,0 +1,31 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright (c) 2018, The Linux Foundation. All rights reserved. */
> +
> +#ifndef _DT_BINDINGS_POWER_QCOM_RPMHPD_H
> +#define _DT_BINDINGS_POWER_QCOM_RPMHPD_H
> +
> +/* SDM845 Power Domain Indexes */
> +#define SDM845_EBI	0
> +#define SDM845_MX	1
> +#define SDM845_MX_AO	2
> +#define SDM845_CX	3
> +#define SDM845_CX_AO	4
> +#define SDM845_LMX	5
> +#define SDM845_LCX	6
> +#define SDM845_GFX	7
> +#define SDM845_MSS	8
> +
> +/* SDM845 Power Domain performance levels */
> +#define RPMH_REGULATOR_LEVEL_OFF	0

Do you really want to specify 0 as a performance level?  This would allow
an OFF request to be sent by setting the performance state and without
disabling the power domain.  I would suggest removing it.

It will also lead to rpmhpd_get_performance() returning 0 in a non-error case.


> +#define RPMH_REGULATOR_LEVEL_RETENTION	16
> +#define RPMH_REGULATOR_LEVEL_MIN_SVS	48
> +#define RPMH_REGULATOR_LEVEL_LOW_SVS	64
> +#define RPMH_REGULATOR_LEVEL_SVS	128
> +#define RPMH_REGULATOR_LEVEL_SVS_L1	192
> +#define RPMH_REGULATOR_LEVEL_NOM	256
> +#define RPMH_REGULATOR_LEVEL_NOM_L1	320
> +#define RPMH_REGULATOR_LEVEL_NOM_L2	336
> +#define RPMH_REGULATOR_LEVEL_TURBO	384
> +#define RPMH_REGULATOR_LEVEL_TURBO_L1	416
> +
> +#endif

Thanks,
David

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

Re: [PATCH v3 6/7] soc: qcom: Add RPMh Power domain driver

[Rajendra Nayak]

Hi David,

On 06/14/2018 06:02 AM, David Collins wrote:
> Hello Rajendra,
> 
> On 06/11/2018 09:40 PM, Rajendra Nayak wrote:
>> The RPMh Power domain driver aggregates the corner votes from various
>> consumers for the ARC resources and communicates it to RPMh.
>>
>> We also add data for all power domains on sdm845 SoC as part of the patch.
>> The driver can be extended to support other SoCs which support RPMh
>>
>> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
>> ---
>>  drivers/soc/qcom/Kconfig                |   9 +
>>  drivers/soc/qcom/Makefile               |   1 +
>>  drivers/soc/qcom/rpmhpd.c               | 427 ++++++++++++++++++++++++
>>  include/dt-bindings/power/qcom-rpmhpd.h |  31 ++
>>  4 files changed, 468 insertions(+)
>>  create mode 100644 drivers/soc/qcom/rpmhpd.c
>>  create mode 100644 include/dt-bindings/power/qcom-rpmhpd.h
> 
> This DT header file should be included in a DT binding patch that is
> separate from the driver patch.
> 
> 
>> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
>> index 5c54931a7b99..7cb7eba2b997 100644
>> --- a/drivers/soc/qcom/Kconfig
>> +++ b/drivers/soc/qcom/Kconfig
>> @@ -74,6 +74,15 @@ config QCOM_RMTFS_MEM
>>  
>>  	  Say y here if you intend to boot the modem remoteproc.
>>  
>> +config QCOM_RPMHPD
>> +	tristate "Qualcomm RPMh Power domain driver"
>> +	depends on QCOM_RPMH && QCOM_COMMAND_DB
>> +	help
>> +	  QCOM RPMh Power domain driver to support power-domains with
>> +	  performance states. The driver communicates a performance state
>> +	  value to RPMh which then translates it into corresponding voltage
>> +	  for the voltage rail.
>> +
>>  config QCOM_RPMPD
>>  	tristate "Qualcomm RPM Power domain driver"
>>  	depends on MFD_QCOM_RPM && QCOM_SMD_RPM
>> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
>> index 9550c170de93..499513f63bef 100644
>> --- a/drivers/soc/qcom/Makefile
>> +++ b/drivers/soc/qcom/Makefile
>> @@ -16,3 +16,4 @@ obj-$(CONFIG_QCOM_SMSM)	+= smsm.o
>>  obj-$(CONFIG_QCOM_WCNSS_CTRL) += wcnss_ctrl.o
>>  obj-$(CONFIG_QCOM_APR) += apr.o
>>  obj-$(CONFIG_QCOM_RPMPD) += rpmpd.o
>> +obj-$(CONFIG_QCOM_RPMHPD) += rpmhpd.o
>> diff --git a/drivers/soc/qcom/rpmhpd.c b/drivers/soc/qcom/rpmhpd.c
>> new file mode 100644
>> index 000000000000..7083ec1590ff
>> --- /dev/null
>> +++ b/drivers/soc/qcom/rpmhpd.c
>> @@ -0,0 +1,427 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/* Copyright (c) 2018, The Linux Foundation. All rights reserved.*/
>> +
>> +#include <linux/err.h>
>> +#include <linux/export.h>
>> +#include <linux/init.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/mutex.h>
>> +#include <linux/pm_domain.h>
>> +#include <linux/slab.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pm_opp.h>
>> +#include <soc/qcom/cmd-db.h>
>> +#include <soc/qcom/rpmh.h>
>> +#include <dt-bindings/power/qcom-rpmhpd.h>
>> +
>> +#define domain_to_rpmhpd(domain) container_of(domain, struct rpmhpd, pd)
>> +
>> +#define DEFINE_RPMHPD_AO(_platform, _name, _active)			\
>> +	static struct rpmhpd _platform##_##_active;			\
>> +	static struct rpmhpd _platform##_##_name = {			\
>> +		.pd = {	.name = #_name,	},				\
>> +		.peer = &_platform##_##_active,				\
>> +		.res_name = #_name".lvl",				\
>> +		.valid_state_mask = (BIT(RPMH_ACTIVE_ONLY_STATE) |	\
>> +					BIT(RPMH_WAKE_ONLY_STATE) |	\
>> +					BIT(RPMH_SLEEP_STATE)),		\
>> +	};								\
>> +	static struct rpmhpd _platform##_##_active = {			\
>> +		.pd = { .name = #_active, },				\
>> +		.peer = &_platform##_##_name,				\
>> +		.active_only = true,					\
>> +		.res_name = #_name".lvl",				\
>> +		.valid_state_mask = (BIT(RPMH_ACTIVE_ONLY_STATE) |	\
>> +					BIT(RPMH_WAKE_ONLY_STATE) |	\
>> +					BIT(RPMH_SLEEP_STATE)),		\> +	}
>> +
>> +#define DEFINE_RPMHPD(_platform, _name)					\
>> +	static struct rpmhpd _platform##_##_name = {			\
>> +		.pd = { .name = #_name, },				\
>> +		.res_name = #_name".lvl",				\
>> +		.valid_state_mask = BIT(RPMH_ACTIVE_ONLY_STATE),	\
>> +	}
>> +
>> +/*
>> + * This is the number of bytes used for each command DB aux data entry of an
>> + * ARC resource.
>> + */
>> +#define RPMH_ARC_LEVEL_SIZE	2
>> +#define RPMH_ARC_MAX_LEVELS	16
>> +
> 
> 
> Would you mind adding a kernel-doc comment for here for struct rpmhpd?  I
> think that would make the code clearer.  It would be good to mention the
> numbering spaces for 'corner' and 'level' elements as well as the usage of
> 'peer' and 'active_only' elements.

yes, I will, there were comments from others as well that the need for 'peer'
and when to use 'active_only' etc wasn't clear.

> 
>> +struct rpmhpd {
>> +	struct device	*dev;
>> +	struct generic_pm_domain pd;
>> +	struct rpmhpd	*peer;
>> +	const bool	active_only;
>> +	unsigned int	corner;
>> +	unsigned int	active_corner> +	u32		level[RPMH_ARC_MAX_LEVELS];
>> +	int		level_count;
>> +	bool		enabled;
>> +	const char	*res_name;
>> +	u32		addr;
>> +	u8		valid_state_mask;
>> +};
>> +
>> +struct rpmhpd_desc {
>> +	struct rpmhpd **rpmhpds;
>> +	size_t num_pds;
>> +};
>> +
>> +static DEFINE_MUTEX(rpmhpd_lock);
>> +
>> +/* sdm845 RPMh Power domains */
>> +DEFINE_RPMHPD(sdm845, ebi);
>> +DEFINE_RPMHPD_AO(sdm845, mx, mx_ao);
>> +DEFINE_RPMHPD_AO(sdm845, cx, cx_ao);
>> +DEFINE_RPMHPD(sdm845, lmx);
>> +DEFINE_RPMHPD(sdm845, lcx);
>> +DEFINE_RPMHPD(sdm845, gfx);
>> +DEFINE_RPMHPD(sdm845, mss);
>> +
>> +static struct rpmhpd *sdm845_rpmhpds[] = {
>> +	[SDM845_EBI] = &sdm845_ebi,
>> +	[SDM845_MX] = &sdm845_mx,
>> +	[SDM845_MX_AO] = &sdm845_mx_ao,
>> +	[SDM845_CX] = &sdm845_cx,
>> +	[SDM845_CX_AO] = &sdm845_cx_ao,
>> +	[SDM845_LMX] = &sdm845_lmx,
>> +	[SDM845_LCX] = &sdm845_lcx,
>> +	[SDM845_GFX] = &sdm845_gfx,
>> +	[SDM845_MSS] = &sdm845_mss,
>> +};
>> +
>> +static const struct rpmhpd_desc sdm845_desc = {
>> +	.rpmhpds = sdm845_rpmhpds,
>> +	.num_pds = ARRAY_SIZE(sdm845_rpmhpds),
>> +};
>> +
>> +static const struct of_device_id rpmhpd_match_table[] = {
>> +	{ .compatible = "qcom,sdm845-rpmhpd", .data = &sdm845_desc },
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(of, rpmhpd_match_table);
>> +
>> +static int rpmhpd_send_corner(struct rpmhpd *pd, int state,
>> +			      unsigned int corner, bool sync)
>> +{
>> +	struct tcs_cmd cmd = {
>> +		.addr = pd->addr,
>> +		.data = corner,
>> +	};
>> +
>> +	if (sync)
>> +		return rpmh_write(pd->dev, state, &cmd, 1);
>> +	else
>> +		return rpmh_write_async(pd->dev, state, &cmd, 1);
>> +}
>> +
>> +static int rpmhpd_send_corner_sync(struct rpmhpd *pd, int state,
>> +				   unsigned int corner)
>> +{
>> +	return rpmhpd_send_corner(pd, state, corner, true);
>> +}
>> +
>> +static int rpmhpd_send_corner_async(struct rpmhpd *pd, int state,
>> +				    unsigned int corner)
>> +{
>> +	return rpmhpd_send_corner(pd, state, corner, false);
>> +};
> 
> I'm not sure about the need for rpmhpd_send_corner_sync() and
> rpmhpd_send_corner_async().  They are adding lines that aren't strictly
> needed since rpmhpd_send_corner() could be called directly instead.  Doing
> that could actually save some more lines in rpmhpd_aggregate_corner()
> below as 'active_corner > pd->active_corner' could be passed as the 'sync'
> argument so that the if statement isn't needed.  However, I also see the
> utility in not having a magic bool in the calls below.  Let's see if other
> reviewers have a preference about it one way or the other.

sure, I like what you are suggesting. I will change it unless someone else
complains.

> 
> 
>> +static void to_active_sleep(struct rpmhpd *pd, unsigned int corner,
>> +			    unsigned int *active, unsigned int *sleep)
>> +{
>> +	*active = corner;
>> +
>> +	if (pd->active_only)
>> +		*sleep = 0;
>> +	else
>> +		*sleep = *active;
>> +}
>> +
>> +/*
>> + * This function is used to aggregate the votes across the active only
>> + * resources and its peers. The aggregated votes are send to RPMh as
>> + * ACTIVE_ONLY votes (which take effect immediately), as WAKE_ONLY votes
>> + * (applied by RPMh on system wakeup) and as SLEEP votes (applied by RPMh
>> + * on system sleep).
>> + * We send ACTIVE_ONLY votes for resources without any peers. For others,
>> + * which have an active only peer, all 3 Votes are sent.
>> + */
>> +static int rpmhpd_aggregate_corner(struct rpmhpd *pd, unsigned int corner)
>> +{
>> +	int ret = -EINVAL;
>> +	struct rpmhpd *peer = pd->peer;
>> +	unsigned int active_corner, sleep_corner;
>> +	unsigned int this_active_corner = 0, this_sleep_corner = 0;
>> +	unsigned int peer_active_corner = 0, peer_sleep_corner = 0;
>> +
>> +	to_active_sleep(pd, corner, &this_active_corner, &this_sleep_corner);
>> +
>> +	if (peer && peer->enabled)
>> +		to_active_sleep(peer, peer->corner, &peer_active_corner,
>> +				&peer_sleep_corner);
>> +
>> +	active_corner = max(this_active_corner, peer_active_corner);
>> +
>> +	if (pd->valid_state_mask & BIT(RPMH_ACTIVE_ONLY_STATE)) {
> 
> This condition will always be true, so this check can be removed.
> 
> 
>> +		/*
>> +		 * Wait for an ack only when we are increasing the
>> +		 * perf state of the power domain
>> +		 */
>> +		if (active_corner > pd->active_corner)
>> +			ret = rpmhpd_send_corner_sync(pd,
>> +						      RPMH_ACTIVE_ONLY_STATE,
>> +						      active_corner);
>> +		else
>> +			ret = rpmhpd_send_corner_async(pd,
>> +						       RPMH_ACTIVE_ONLY_STATE,
>> +						       active_corner);
>> +		if (ret)
>> +			return ret;
>> +		pd->active_corner = active_corner;
>> +		if (peer)
>> +			peer->active_corner = active_corner;
>> +	}
>> +
>> +	if (pd->valid_state_mask & BIT(RPMH_WAKE_ONLY_STATE)) {
> 
> This check and the one below could be changed to simply:
> 
>     if (pd->peer) {
> 
> That way, the valid_state_mask element can be removed from struct rpmhpd
> and the two if blocks can be consolidated together.  I think that
> valid_state_mask is making the code more confusing at this point than it
> is at verbosely describing the aggregation semantics.

makes sense

> 
> 
>> +		ret = rpmhpd_send_corner_async(pd, RPMH_WAKE_ONLY_STATE,
>> +					       active_corner);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	sleep_corner = max(this_sleep_corner, peer_sleep_corner);
>> +
>> +	if (pd->valid_state_mask & BIT(RPMH_SLEEP_STATE))
>> +		ret = rpmhpd_send_corner_async(pd, RPMH_SLEEP_STATE,
>> +					       sleep_corner);
>> +
>> +	return ret;
>> +}
>> +
>> +static int rpmhpd_power_on(struct generic_pm_domain *domain)
>> +{
>> +	struct rpmhpd *pd = domain_to_rpmhpd(domain);
>> +	int ret = 0;
>> +
>> +	mutex_lock(&rpmhpd_lock);
>> +
>> +	pd->enabled = true;
> 
> It would probably be better to remove this line and add the following
> after the rpmhpd_aggregate_corner() call:
> 
> if (!ret)
> 	pd->enabled = true;
> 
> Only the peer 'enabled' value is checked in rpmhpd_aggregate_corner() so
> architecturally, it doesn't matter if the value is configured before or
> after the call.

agree, a failure to communicate with rpmh would then keep it in disabled state.
 
> 
> 
>> +
>> +	if (pd->corner)
>> +		ret = rpmhpd_aggregate_corner(pd, pd->corner);
>> +
>> +	mutex_unlock(&rpmhpd_lock);
>> +
>> +	return ret;
>> +}
>> +
>> +static int rpmhpd_power_off(struct generic_pm_domain *domain)
>> +{
>> +	struct rpmhpd *pd = domain_to_rpmhpd(domain);
>> +	int ret = 0;
>> +
>> +	mutex_lock(&rpmhpd_lock);
>> +
>> +	if (pd->level[0] == 0)
>> +		ret = rpmhpd_aggregate_corner(pd, 0);
> 
> I'm not sure that we want to have the 'pd->level[0] == 0' check,
> especially when considering aggregation with the peer pd.  I understand
> its intention to try to keep enable state and level setting orthogonal.
> However, as it stands now, the final request sent to hardware would differ
> depending upon the order of calls.  Consider the following example.
> 
> Initial state:
> pd->level[0] == 0
> pd->corner = 5, pd->enabled = true, pd->active_only = false
> pd->peer->corner = 7, pd->peer->enabled = true, pd->peer->active_only = true
> 
> Outstanding requests:
> RPMH_ACTIVE_ONLY_STATE = 7, RPMH_WAKE_ONLY_STATE = 7, RPMH_SLEEP_STATE = 5
> 
> Case A:
> 	1. set pd->corner = 6
> 		--> new value request: RPMH_SLEEP_STATE = 6
> 		--> duplicate value requests: RPMH_ACTIVE_ONLY_STATE = 7,
> 			RPMH_WAKE_ONLY_STATE = 7
> 	2. power_off pd->peer
> 		--> no requests

I am not sure why there would be no requests, since we do end up aggregating
with pd->peer->corner = 0.
So the final state would be

RPMH_ACTIVE_ONLY_STATE = max(6, 0) = 6
RPMH_WAKE_ONLY_STATE = 6
RPMH_SLEEP_STATE = max(6, 0) = 6

> 
> 	Final state:
> 	RPMH_ACTIVE_ONLY_STATE = 7
> 	RPMH_WAKE_ONLY_STATE = 7
> 	RPMH_SLEEP_STATE = 6
> 
> Case B:
> 	1. power_off pd->peer
> 		--> no requests

Here it would be again be aggregation based on pd->peer->corner = 0
so,
RPMH_ACTIVE_ONLY_STATE = max(5, 0) = 5
RPMH_WAKE_ONLY_STATE = 5
RPMH_SLEEP_STATE = max(5, 0) = 5

> 	2. set pd->corner = 6
> 		--> new value requests: RPMH_ACTIVE_ONLY_STATE = 6,
> 		       RPMH_WAKE_ONLY_STATE = 6, RPMH_SLEEP_STATE = 6
> 
> 	Final state:
> 	RPMH_ACTIVE_ONLY_STATE = 6
> 	RPMH_WAKE_ONLY_STATE = 6
> 	RPMH_SLEEP_STATE = 6

correct,
RPMH_ACTIVE_ONLY_STATE = max(6, 0) = 6
RPMH_WAKE_ONLY_STATE = 6
RPMH_SLEEP_STATE = max(6, 0) = 6

> 
> Without the check, Linux would vote for the lowest supported level when
> power_off is called.  This seems semantically reasonable given that the
> consumer is ok with the power domain going fully off and that would be the
> closest that we can get.

So are you suggesting I replace

>> +	if (pd->level[0] == 0)
>> +		ret = rpmhpd_aggregate_corner(pd, 0);

with

>> +	ret = rpmhpd_aggregate_corner(pd, pd->level[0]);

I can see what you said above makes sense but if its
> Initial state:
> pd->level[0] != 0

Was that what you meant?

I can't seem to see any ARC resources on 845 which seem to 
have a 'pd->level[0] != 0' but looks like thats certainly a
possibility we need to handle?

> 
> 
>> +
>> +	if (!ret)
>> +		pd->enabled = false;
>> +
>> +	mutex_unlock(&rpmhpd_lock);
>> +
>> +	return ret;
>> +}
>> +
>> +static int rpmhpd_set_performance(struct generic_pm_domain *domain,
>> +				  unsigned int state)
> 
> The code might be a bit more readable if 'state' is changed to 'level'.
> 
> Also, is there a particular reason that this is named
> rpmhpd_set_performance() instead of rpmhpd_set_performance_state()?

no, i will change both.

> 
> 
>> +{
>> +	struct rpmhpd *pd = domain_to_rpmhpd(domain);
>> +	int ret = 0, i;
>> +
>> +	mutex_lock(&rpmhpd_lock);
>> +
>> +	for (i = 0; i < pd->level_count; i++)
>> +		if (state <= pd->level[i])
>> +			break;
>> +
>> +	if (i == pd->level_count) {
>> +		ret = -EINVAL;
>> +		dev_err(pd->dev, "invalid state=%u for domain %s",
>> +			state, pd->pd.name);
>> +			goto out;
> 
> One level of indentation should be removed from this line.

right

> 
> 
>> +	}
>> +
>> +	pd->corner = i;
>> +
>> +	if (!pd->enabled)
>> +		goto out;
>> +
>> +	ret = rpmhpd_aggregate_corner(pd, i);
> 
> Would it be worthwhile to roll back the pd->corner value in the case of an
> error?

yes, makes sense

> 
> 
>> +out:
>> +	mutex_unlock(&rpmhpd_lock);
>> +
>> +	return ret;
>> +}
>> +
>> +static unsigned int rpmhpd_get_performance(struct generic_pm_domain *genpd,
>> +					   struct dev_pm_opp *opp)
> 
> Is there a particular reason that this is named rpmhpd_get_performance()
> instead of rpmhpd_get_performance_state()?

nop, will change

> 
> 
>> +{
>> +	struct device_node *np;
>> +	unsigned int corner = 0;
> 
> Please change 'corner' to 'level' for consistency.  In this driver "level"
> values are in the vlvl RPMH_REGULATOR_LEVEL_* numbering space and "corner"
> values are in the hlvl 0-15 numbering space.

right, i will change things to be more consistent and less confusing

> 
> 
>> +
>> +	np = dev_pm_opp_get_of_node(opp);
>> +	if (of_property_read_u32(np, "qcom,level", &corner)) {
>> +		pr_err("%s: missing 'qcom,level' property\n", __func__);
>> +		return 0;
>> +	}
>> +
>> +	of_node_put(np);
>> +
>> +	return corner;
>> +}
>> +
>> +static int rpmhpd_update_level_mapping(struct rpmhpd *rpmhpd)
>> +{
>> +	int i, j, len, ret;
>> +	u8 buf[RPMH_ARC_MAX_LEVELS * RPMH_ARC_LEVEL_SIZE];
> 
> Minor: It might look better to list buf[] first.

sure

> 
> 
>> +
>> +	len = cmd_db_read_aux_data_len(rpmhpd->res_name);
>> +	if (len <= 0)
>> +		return len;
>> +
>> +	if (len > RPMH_ARC_MAX_LEVELS * RPMH_ARC_LEVEL_SIZE)
>> +		return -EINVAL;
> 
> 'else if' could be used here.

okay

> 
> 
>> +
>> +	ret = cmd_db_read_aux_data(rpmhpd->res_name, buf, len);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	rpmhpd->level_count = len / RPMH_ARC_LEVEL_SIZE;
>> +
>> +	for (i = 0; i < rpmhpd->level_count; i++) {
>> +		rpmhpd->level[i] = 0;
>> +		for (j = 0; j < RPMH_ARC_LEVEL_SIZE; j++)
>> +			rpmhpd->level[i] |=
>> +				buf[i * RPMH_ARC_LEVEL_SIZE + j] << (8 * j);
>> +
>> +		/*
>> +		 * The AUX data may be zero padded.  These 0 valued entries at
>> +		 * the end of the map must be ignored.
>> +		 */
>> +		if (i > 0 && rpmhpd->level[i] == 0) {
>> +			rpmhpd->level_count = i;
>> +			break;
>> +		}
>> +		pr_dbg("%s: ARC hlvl=%2d --> vlvl=%4u\n", rpmhpd->res_name, i,
>> +		       rpmhpd->level[i]);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int rpmhpd_probe(struct platform_device *pdev)
>> +{
>> +	int i, ret;
>> +	size_t num;
>> +	struct genpd_onecell_data *data;
>> +	struct rpmhpd **rpmhpds;
>> +	const struct rpmhpd_desc *desc;
>> +
>> +	desc = of_device_get_match_data(&pdev->dev);
>> +	if (!desc)
>> +		return -EINVAL;
>> +
>> +	rpmhpds = desc->rpmhpds;
>> +	num = desc->num_pds;
>> +
>> +	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
>> +	if (!data)
>> +		return -ENOMEM;
>> +
>> +	data->domains = devm_kcalloc(&pdev->dev, num, sizeof(*data->domains),
>> +				     GFP_KERNEL);
>> +	data->num_domains = num;
>> +
>> +	ret = cmd_db_ready();
>> +	if (ret) {
>> +		if (ret != -EPROBE_DEFER)
>> +			dev_err(&pdev->dev, "Command DB unavailable, ret=%d\n",
>> +				ret);
>> +		return ret;
>> +	}
>> +
>> +	for (i = 0; i < num; i++) {
>> +		if (!rpmhpds[i]) {
>> +			dev_warn(&pdev->dev, "rpmhpds[] with empty entry at index=%d\n",
>> +				 i);
> 
> Minor: This could be simplified to:
> 
> dev_warn(&pdev->dev, "rpmhpds[%d] is empty\n", i);

will do

> 
> 
>> +			continue;
>> +		}
>> +
>> +		rpmhpds[i]->dev = &pdev->dev;
>> +		rpmhpds[i]->addr = cmd_db_read_addr(rpmhpds[i]->res_name);
>> +		if (!rpmhpds[i]->addr) {
>> +			dev_err(&pdev->dev, "Could not find RPMh address for resource %s\n",
>> +				rpmhpds[i]->res_name);
>> +			return -ENODEV;
>> +		}
>> +
>> +		ret = cmd_db_read_slave_id(rpmhpds[i]->res_name);
>> +		if (ret != CMD_DB_HW_ARC) {
>> +			dev_err(&pdev->dev, "RPMh slave ID mismatch\n");
>> +			return -EINVAL;
>> +		}
>> +
>> +		ret = rpmhpd_update_level_mapping(rpmhpds[i]);
>> +		if (ret)
>> +			return ret;
>> +
>> +		rpmhpds[i]->pd.power_off = rpmhpd_power_off;
>> +		rpmhpds[i]->pd.power_on = rpmhpd_power_on;
>> +		rpmhpds[i]->pd.set_performance_state = rpmhpd_set_performance;
>> +		rpmhpds[i]->pd.opp_to_performance_state = rpmhpd_get_performance;
>> +		pm_genpd_init(&rpmhpds[i]->pd, NULL, true);
>> +
>> +		data->domains[i] = &rpmhpds[i]->pd;
>> +	}
>> +
>> +	return of_genpd_add_provider_onecell(pdev->dev.of_node, data);
>> +}
>> +
>> +static int rpmhpd_remove(struct platform_device *pdev)
>> +{
>> +	of_genpd_del_provider(pdev->dev.of_node);
>> +	return 0;
>> +}
>> +
>> +static struct platform_driver rpmhpd_driver = {
>> +	.driver = {
>> +		.name = "qcom-rpmhpd",
>> +		.of_match_table = rpmhpd_match_table,
>> +	},
>> +	.probe = rpmhpd_probe,
>> +	.remove = rpmhpd_remove,
>> +};
>> +
>> +static int __init rpmhpd_init(void)
>> +{
>> +	return platform_driver_register(&rpmhpd_driver);
>> +}
>> +core_initcall(rpmhpd_init);
>> +
>> +static void __exit rpmhpd_exit(void)
>> +{
>> +	platform_driver_unregister(&rpmhpd_driver);
>> +}
>> +module_exit(rpmhpd_exit);
>> +
>> +MODULE_DESCRIPTION("Qualcomm Technologies, Inc. RPMh Power Domain Driver");
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_ALIAS("platform:qcom-rpmhpd");
>> diff --git a/include/dt-bindings/power/qcom-rpmhpd.h b/include/dt-bindings/power/qcom-rpmhpd.h
>> new file mode 100644
>> index 000000000000..b01ae2452603
>> --- /dev/null
>> +++ b/include/dt-bindings/power/qcom-rpmhpd.h
>> @@ -0,0 +1,31 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/* Copyright (c) 2018, The Linux Foundation. All rights reserved. */
>> +
>> +#ifndef _DT_BINDINGS_POWER_QCOM_RPMHPD_H
>> +#define _DT_BINDINGS_POWER_QCOM_RPMHPD_H
>> +
>> +/* SDM845 Power Domain Indexes */
>> +#define SDM845_EBI	0
>> +#define SDM845_MX	1
>> +#define SDM845_MX_AO	2
>> +#define SDM845_CX	3
>> +#define SDM845_CX_AO	4
>> +#define SDM845_LMX	5
>> +#define SDM845_LCX	6
>> +#define SDM845_GFX	7
>> +#define SDM845_MSS	8
>> +
>> +/* SDM845 Power Domain performance levels */
>> +#define RPMH_REGULATOR_LEVEL_OFF	0
> 
> Do you really want to specify 0 as a performance level?  This would allow
> an OFF request to be sent by setting the performance state and without
> disabling the power domain.  I would suggest removing it.
> 
> It will also lead to rpmhpd_get_performance() returning 0 in a non-error case.

yes, I'll drop it. Thanks again for taking a look at these patches.

thanks,
Rajendra


-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH v3 6/7] soc: qcom: Add RPMh Power domain driver

[David Collins]

Hello Rajendra,

On 06/13/2018 11:54 PM, Rajendra Nayak wrote:
> On 06/14/2018 06:02 AM, David Collins wrote:
>> On 06/11/2018 09:40 PM, Rajendra Nayak wrote:
...
>>> +static int rpmhpd_power_off(struct generic_pm_domain *domain)
>>> +{
>>> +	struct rpmhpd *pd = domain_to_rpmhpd(domain);
>>> +	int ret = 0;
>>> +
>>> +	mutex_lock(&rpmhpd_lock);
>>> +
>>> +	if (pd->level[0] == 0)
>>> +		ret = rpmhpd_aggregate_corner(pd, 0);
>>
>> I'm not sure that we want to have the 'pd->level[0] == 0' check,
>> especially when considering aggregation with the peer pd.  I understand
>> its intention to try to keep enable state and level setting orthogonal.
>> However, as it stands now, the final request sent to hardware would differ
>> depending upon the order of calls.  Consider the following example.
>>
>> Initial state:
>> pd->level[0] == 0
>> pd->corner = 5, pd->enabled = true, pd->active_only = false
>> pd->peer->corner = 7, pd->peer->enabled = true, pd->peer->active_only = true
>>
>> Outstanding requests:
>> RPMH_ACTIVE_ONLY_STATE = 7, RPMH_WAKE_ONLY_STATE = 7, RPMH_SLEEP_STATE = 5
>>
>> Case A:
>> 	1. set pd->corner = 6
>> 		--> new value request: RPMH_SLEEP_STATE = 6
>> 		--> duplicate value requests: RPMH_ACTIVE_ONLY_STATE = 7,
>> 			RPMH_WAKE_ONLY_STATE = 7
>> 	2. power_off pd->peer
>> 		--> no requests
> 
> I am not sure why there would be no requests, since we do end up aggregating
> with pd->peer->corner = 0.
> So the final state would be
> 
> RPMH_ACTIVE_ONLY_STATE = max(6, 0) = 6
> RPMH_WAKE_ONLY_STATE = 6
> RPMH_SLEEP_STATE = max(6, 0) = 6

Argh, my example was ruined by a one character typo.  I meant to have:

	Initial state:
	pd->level[0] != 0


>>
>> 	Final state:
>> 	RPMH_ACTIVE_ONLY_STATE = 7
>> 	RPMH_WAKE_ONLY_STATE = 7
>> 	RPMH_SLEEP_STATE = 6
>>
>> Case B:
>> 	1. power_off pd->peer
>> 		--> no requests
> 
> Here it would be again be aggregation based on pd->peer->corner = 0
> so,
> RPMH_ACTIVE_ONLY_STATE = max(5, 0) = 5
> RPMH_WAKE_ONLY_STATE = 5
> RPMH_SLEEP_STATE = max(5, 0) = 5
> 
>> 	2. set pd->corner = 6
>> 		--> new value requests: RPMH_ACTIVE_ONLY_STATE = 6,
>> 		       RPMH_WAKE_ONLY_STATE = 6, RPMH_SLEEP_STATE = 6
>>
>> 	Final state:
>> 	RPMH_ACTIVE_ONLY_STATE = 6
>> 	RPMH_WAKE_ONLY_STATE = 6
>> 	RPMH_SLEEP_STATE = 6
> 
> correct,
> RPMH_ACTIVE_ONLY_STATE = max(6, 0) = 6
> RPMH_WAKE_ONLY_STATE = 6
> RPMH_SLEEP_STATE = max(6, 0) = 6
> 
>>
>> Without the check, Linux would vote for the lowest supported level when
>> power_off is called.  This seems semantically reasonable given that the
>> consumer is ok with the power domain going fully off and that would be the
>> closest that we can get.
> 
> So are you suggesting I replace
> 
>>> +	if (pd->level[0] == 0)
>>> +		ret = rpmhpd_aggregate_corner(pd, 0);
> 
> with
> 
>>> +	ret = rpmhpd_aggregate_corner(pd, pd->level[0]);

Yes, this is the modification that I'm requesting.


> I can see what you said above makes sense but if its
>> Initial state:
>> pd->level[0] != 0
> 
> Was that what you meant?

Yes.


> I can't seem to see any ARC resources on 845 which seem to 
> have a 'pd->level[0] != 0' but looks like thats certainly a
> possibility we need to handle?

The command DB interface for ARC resources was designed to support the
situation of a power domain that could not be fully disabled and is
instead limited to some minimum level.

Thanks,
David

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

Re: [PATCH v3 6/7] soc: qcom: Add RPMh Power domain driver

[Ulf Hansson]

David, Rajendra,

On 14 June 2018 at 20:17, David Collins <collinsd@codeaurora.org> wrote:
> Hello Rajendra,
>
> On 06/13/2018 11:54 PM, Rajendra Nayak wrote:
>> On 06/14/2018 06:02 AM, David Collins wrote:
>>> On 06/11/2018 09:40 PM, Rajendra Nayak wrote:
> ...
>>>> +static int rpmhpd_power_off(struct generic_pm_domain *domain)
>>>> +{
>>>> +   struct rpmhpd *pd = domain_to_rpmhpd(domain);
>>>> +   int ret = 0;
>>>> +
>>>> +   mutex_lock(&rpmhpd_lock);
>>>> +
>>>> +   if (pd->level[0] == 0)
>>>> +           ret = rpmhpd_aggregate_corner(pd, 0);
>>>
>>> I'm not sure that we want to have the 'pd->level[0] == 0' check,
>>> especially when considering aggregation with the peer pd.  I understand
>>> its intention to try to keep enable state and level setting orthogonal.
>>> However, as it stands now, the final request sent to hardware would differ
>>> depending upon the order of calls.  Consider the following example.
>>>
>>> Initial state:
>>> pd->level[0] == 0
>>> pd->corner = 5, pd->enabled = true, pd->active_only = false
>>> pd->peer->corner = 7, pd->peer->enabled = true, pd->peer->active_only = true
>>>
>>> Outstanding requests:
>>> RPMH_ACTIVE_ONLY_STATE = 7, RPMH_WAKE_ONLY_STATE = 7, RPMH_SLEEP_STATE = 5
>>>
>>> Case A:
>>>      1. set pd->corner = 6
>>>              --> new value request: RPMH_SLEEP_STATE = 6
>>>              --> duplicate value requests: RPMH_ACTIVE_ONLY_STATE = 7,
>>>                      RPMH_WAKE_ONLY_STATE = 7
>>>      2. power_off pd->peer
>>>              --> no requests
>>
>> I am not sure why there would be no requests, since we do end up aggregating
>> with pd->peer->corner = 0.
>> So the final state would be
>>
>> RPMH_ACTIVE_ONLY_STATE = max(6, 0) = 6
>> RPMH_WAKE_ONLY_STATE = 6
>> RPMH_SLEEP_STATE = max(6, 0) = 6
>
> Argh, my example was ruined by a one character typo.  I meant to have:
>
>         Initial state:
>         pd->level[0] != 0
>
>
>>>
>>>      Final state:
>>>      RPMH_ACTIVE_ONLY_STATE = 7
>>>      RPMH_WAKE_ONLY_STATE = 7
>>>      RPMH_SLEEP_STATE = 6
>>>
>>> Case B:
>>>      1. power_off pd->peer
>>>              --> no requests
>>
>> Here it would be again be aggregation based on pd->peer->corner = 0
>> so,
>> RPMH_ACTIVE_ONLY_STATE = max(5, 0) = 5
>> RPMH_WAKE_ONLY_STATE = 5
>> RPMH_SLEEP_STATE = max(5, 0) = 5
>>
>>>      2. set pd->corner = 6
>>>              --> new value requests: RPMH_ACTIVE_ONLY_STATE = 6,
>>>                     RPMH_WAKE_ONLY_STATE = 6, RPMH_SLEEP_STATE = 6
>>>
>>>      Final state:
>>>      RPMH_ACTIVE_ONLY_STATE = 6
>>>      RPMH_WAKE_ONLY_STATE = 6
>>>      RPMH_SLEEP_STATE = 6
>>
>> correct,
>> RPMH_ACTIVE_ONLY_STATE = max(6, 0) = 6
>> RPMH_WAKE_ONLY_STATE = 6
>> RPMH_SLEEP_STATE = max(6, 0) = 6
>>
>>>
>>> Without the check, Linux would vote for the lowest supported level when
>>> power_off is called.  This seems semantically reasonable given that the
>>> consumer is ok with the power domain going fully off and that would be the
>>> closest that we can get.
>>
>> So are you suggesting I replace
>>
>>>> +   if (pd->level[0] == 0)
>>>> +           ret = rpmhpd_aggregate_corner(pd, 0);
>>
>> with
>>
>>>> +   ret = rpmhpd_aggregate_corner(pd, pd->level[0]);
>
> Yes, this is the modification that I'm requesting.
>
>
>> I can see what you said above makes sense but if its
>>> Initial state:
>>> pd->level[0] != 0
>>
>> Was that what you meant?
>
> Yes.

Apologize for jumping into the discussion.

I have a couple of ideas, that may simplify/improve things related to the above.

1) Would it be easier if genpd calls ->set_performance_state(0) before
it is about to call the ->power_off() callback? Actually Viresh wanted
that from the start, but I thought it was useless.

2) When device are becoming runtime suspended, the ->runtime_suspend()
callback of genpd is invoked (genpd_runtime_suspend()). However, in
there we don't care to re-evaluate the votes on the performance level,
but instead rely on the corresponding driver for the device to drop
the vote. I think it would be a good idea of managing this internally
in genpd instead, thus, depending on if the aggregated vote can be
decreased we should call  ->set_performance_state(new-vote). Of
course, once the device get runtime resumed, the votes needs to be
restored for the device.

What do you think?

[...]

Kind regards
Uffe

Re: [PATCH v3 6/7] soc: qcom: Add RPMh Power domain driver

[David Collins]

Hello Ulf,

On 06/15/2018 02:25 AM, Ulf Hansson wrote:
> On 14 June 2018 at 20:17, David Collins <collinsd@codeaurora.org> wrote:
>> On 06/13/2018 11:54 PM, Rajendra Nayak wrote:
>>> On 06/14/2018 06:02 AM, David Collins wrote:
>>>> On 06/11/2018 09:40 PM, Rajendra Nayak wrote:
>> ...
>>>>> +static int rpmhpd_power_off(struct generic_pm_domain *domain)
>>>>> +{
>>>>> +   struct rpmhpd *pd = domain_to_rpmhpd(domain);
>>>>> +   int ret = 0;
>>>>> +
>>>>> +   mutex_lock(&rpmhpd_lock);
>>>>> +
>>>>> +   if (pd->level[0] == 0)
>>>>> +           ret = rpmhpd_aggregate_corner(pd, 0);
>>>>
>>>> I'm not sure that we want to have the 'pd->level[0] == 0' check,
>>>> especially when considering aggregation with the peer pd.  I understand
>>>> its intention to try to keep enable state and level setting orthogonal.
>>>> However, as it stands now, the final request sent to hardware would differ
>>>> depending upon the order of calls.  Consider the following example.
>>>>
>>>> Initial state:
>>>> pd->level[0] == 0
>>>> pd->corner = 5, pd->enabled = true, pd->active_only = false
>>>> pd->peer->corner = 7, pd->peer->enabled = true, pd->peer->active_only = true
>>>>
>>>> Outstanding requests:
>>>> RPMH_ACTIVE_ONLY_STATE = 7, RPMH_WAKE_ONLY_STATE = 7, RPMH_SLEEP_STATE = 5
>>>>
>>>> Case A:
>>>>      1. set pd->corner = 6
>>>>              --> new value request: RPMH_SLEEP_STATE = 6
>>>>              --> duplicate value requests: RPMH_ACTIVE_ONLY_STATE = 7,
>>>>                      RPMH_WAKE_ONLY_STATE = 7
>>>>      2. power_off pd->peer
>>>>              --> no requests
>>>
>>> I am not sure why there would be no requests, since we do end up aggregating
>>> with pd->peer->corner = 0.
>>> So the final state would be
>>>
>>> RPMH_ACTIVE_ONLY_STATE = max(6, 0) = 6
>>> RPMH_WAKE_ONLY_STATE = 6
>>> RPMH_SLEEP_STATE = max(6, 0) = 6
>>
>> Argh, my example was ruined by a one character typo.  I meant to have:
>>
>>         Initial state:
>>         pd->level[0] != 0
>>
>>
>>>>
>>>>      Final state:
>>>>      RPMH_ACTIVE_ONLY_STATE = 7
>>>>      RPMH_WAKE_ONLY_STATE = 7
>>>>      RPMH_SLEEP_STATE = 6
>>>>
>>>> Case B:
>>>>      1. power_off pd->peer
>>>>              --> no requests
>>>
>>> Here it would be again be aggregation based on pd->peer->corner = 0
>>> so,
>>> RPMH_ACTIVE_ONLY_STATE = max(5, 0) = 5
>>> RPMH_WAKE_ONLY_STATE = 5
>>> RPMH_SLEEP_STATE = max(5, 0) = 5
>>>
>>>>      2. set pd->corner = 6
>>>>              --> new value requests: RPMH_ACTIVE_ONLY_STATE = 6,
>>>>                     RPMH_WAKE_ONLY_STATE = 6, RPMH_SLEEP_STATE = 6
>>>>
>>>>      Final state:
>>>>      RPMH_ACTIVE_ONLY_STATE = 6
>>>>      RPMH_WAKE_ONLY_STATE = 6
>>>>      RPMH_SLEEP_STATE = 6
>>>
>>> correct,
>>> RPMH_ACTIVE_ONLY_STATE = max(6, 0) = 6
>>> RPMH_WAKE_ONLY_STATE = 6
>>> RPMH_SLEEP_STATE = max(6, 0) = 6
>>>
>>>>
>>>> Without the check, Linux would vote for the lowest supported level when
>>>> power_off is called.  This seems semantically reasonable given that the
>>>> consumer is ok with the power domain going fully off and that would be the
>>>> closest that we can get.
>>>
>>> So are you suggesting I replace
>>>
>>>>> +   if (pd->level[0] == 0)
>>>>> +           ret = rpmhpd_aggregate_corner(pd, 0);
>>>
>>> with
>>>
>>>>> +   ret = rpmhpd_aggregate_corner(pd, pd->level[0]);
>>
>> Yes, this is the modification that I'm requesting.
>>
>>
>>> I can see what you said above makes sense but if its
>>>> Initial state:
>>>> pd->level[0] != 0
>>>
>>> Was that what you meant?
>>
>> Yes.
> 
> Apologize for jumping into the discussion.
> 
> I have a couple of ideas, that may simplify/improve things related to the above.
> 
> 1) Would it be easier if genpd calls ->set_performance_state(0) before
> it is about to call the ->power_off() callback? Actually Viresh wanted
> that from the start, but I thought it was useless.

This sounds like a relatively reasonable thing to do that falls in line
with the semantics of the API.  It would also necessitate genpd
aggregating performance state requests again when ->power_on() is called
so that ->set_performance_state() can be called with an appropriate value.

I think that the issue that I identified above should be solved outright
within the rpmhpd driver though.  It is a bug in the RPMh-specific active
+ sleep vs active-only aggregation logic.

The feature you are describing here seems more like a power optimization
that would help in the case of consumer disabling the power domain without
scaling down the performance level for a power domain that does not
support enable/disable.

Would this behavior in genpd be implemented per power domain or per consumer?


> 2) When device are becoming runtime suspended, the ->runtime_suspend()
> callback of genpd is invoked (genpd_runtime_suspend()). However, in
> there we don't care to re-evaluate the votes on the performance level,
> but instead rely on the corresponding driver for the device to drop
> the vote. I think it would be a good idea of managing this internally
> in genpd instead, thus, depending on if the aggregated vote can be
> decreased we should call  ->set_performance_state(new-vote). Of
> course, once the device get runtime resumed, the votes needs to be
> restored for the device.

This feature sounds a little risky.  Is it really safe for all cases for
the genpd framework to unilaterally make these kind of decisions for
consumers?  Could there be any interdependencies between resources that
consumers need to enforce that would not be possible if genpd handled
power state reduction automatically (for example two power domains with a
sequencing requirement between them)?

Thanks,
David

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

Re: [PATCH v3 6/7] soc: qcom: Add RPMh Power domain driver

[Ulf Hansson]

On 15 June 2018 at 23:46, David Collins <collinsd@codeaurora.org> wrote:
> Hello Ulf,
>
> On 06/15/2018 02:25 AM, Ulf Hansson wrote:
>> On 14 June 2018 at 20:17, David Collins <collinsd@codeaurora.org> wrote:
>>> On 06/13/2018 11:54 PM, Rajendra Nayak wrote:
>>>> On 06/14/2018 06:02 AM, David Collins wrote:
>>>>> On 06/11/2018 09:40 PM, Rajendra Nayak wrote:
>>> ...
>>>>>> +static int rpmhpd_power_off(struct generic_pm_domain *domain)
>>>>>> +{
>>>>>> +   struct rpmhpd *pd = domain_to_rpmhpd(domain);
>>>>>> +   int ret = 0;
>>>>>> +
>>>>>> +   mutex_lock(&rpmhpd_lock);
>>>>>> +
>>>>>> +   if (pd->level[0] == 0)
>>>>>> +           ret = rpmhpd_aggregate_corner(pd, 0);
>>>>>
>>>>> I'm not sure that we want to have the 'pd->level[0] == 0' check,
>>>>> especially when considering aggregation with the peer pd.  I understand
>>>>> its intention to try to keep enable state and level setting orthogonal.
>>>>> However, as it stands now, the final request sent to hardware would differ
>>>>> depending upon the order of calls.  Consider the following example.
>>>>>
>>>>> Initial state:
>>>>> pd->level[0] == 0
>>>>> pd->corner = 5, pd->enabled = true, pd->active_only = false
>>>>> pd->peer->corner = 7, pd->peer->enabled = true, pd->peer->active_only = true
>>>>>
>>>>> Outstanding requests:
>>>>> RPMH_ACTIVE_ONLY_STATE = 7, RPMH_WAKE_ONLY_STATE = 7, RPMH_SLEEP_STATE = 5
>>>>>
>>>>> Case A:
>>>>>      1. set pd->corner = 6
>>>>>              --> new value request: RPMH_SLEEP_STATE = 6
>>>>>              --> duplicate value requests: RPMH_ACTIVE_ONLY_STATE = 7,
>>>>>                      RPMH_WAKE_ONLY_STATE = 7
>>>>>      2. power_off pd->peer
>>>>>              --> no requests
>>>>
>>>> I am not sure why there would be no requests, since we do end up aggregating
>>>> with pd->peer->corner = 0.
>>>> So the final state would be
>>>>
>>>> RPMH_ACTIVE_ONLY_STATE = max(6, 0) = 6
>>>> RPMH_WAKE_ONLY_STATE = 6
>>>> RPMH_SLEEP_STATE = max(6, 0) = 6
>>>
>>> Argh, my example was ruined by a one character typo.  I meant to have:
>>>
>>>         Initial state:
>>>         pd->level[0] != 0
>>>
>>>
>>>>>
>>>>>      Final state:
>>>>>      RPMH_ACTIVE_ONLY_STATE = 7
>>>>>      RPMH_WAKE_ONLY_STATE = 7
>>>>>      RPMH_SLEEP_STATE = 6
>>>>>
>>>>> Case B:
>>>>>      1. power_off pd->peer
>>>>>              --> no requests
>>>>
>>>> Here it would be again be aggregation based on pd->peer->corner = 0
>>>> so,
>>>> RPMH_ACTIVE_ONLY_STATE = max(5, 0) = 5
>>>> RPMH_WAKE_ONLY_STATE = 5
>>>> RPMH_SLEEP_STATE = max(5, 0) = 5
>>>>
>>>>>      2. set pd->corner = 6
>>>>>              --> new value requests: RPMH_ACTIVE_ONLY_STATE = 6,
>>>>>                     RPMH_WAKE_ONLY_STATE = 6, RPMH_SLEEP_STATE = 6
>>>>>
>>>>>      Final state:
>>>>>      RPMH_ACTIVE_ONLY_STATE = 6
>>>>>      RPMH_WAKE_ONLY_STATE = 6
>>>>>      RPMH_SLEEP_STATE = 6
>>>>
>>>> correct,
>>>> RPMH_ACTIVE_ONLY_STATE = max(6, 0) = 6
>>>> RPMH_WAKE_ONLY_STATE = 6
>>>> RPMH_SLEEP_STATE = max(6, 0) = 6
>>>>
>>>>>
>>>>> Without the check, Linux would vote for the lowest supported level when
>>>>> power_off is called.  This seems semantically reasonable given that the
>>>>> consumer is ok with the power domain going fully off and that would be the
>>>>> closest that we can get.
>>>>
>>>> So are you suggesting I replace
>>>>
>>>>>> +   if (pd->level[0] == 0)
>>>>>> +           ret = rpmhpd_aggregate_corner(pd, 0);
>>>>
>>>> with
>>>>
>>>>>> +   ret = rpmhpd_aggregate_corner(pd, pd->level[0]);
>>>
>>> Yes, this is the modification that I'm requesting.
>>>
>>>
>>>> I can see what you said above makes sense but if its
>>>>> Initial state:
>>>>> pd->level[0] != 0
>>>>
>>>> Was that what you meant?
>>>
>>> Yes.
>>
>> Apologize for jumping into the discussion.
>>
>> I have a couple of ideas, that may simplify/improve things related to the above.
>>
>> 1) Would it be easier if genpd calls ->set_performance_state(0) before
>> it is about to call the ->power_off() callback? Actually Viresh wanted
>> that from the start, but I thought it was useless.
>
> This sounds like a relatively reasonable thing to do that falls in line
> with the semantics of the API.  It would also necessitate genpd
> aggregating performance state requests again when ->power_on() is called
> so that ->set_performance_state() can be called with an appropriate value.
>
> I think that the issue that I identified above should be solved outright
> within the rpmhpd driver though.  It is a bug in the RPMh-specific active
> + sleep vs active-only aggregation logic.
>
> The feature you are describing here seems more like a power optimization
> that would help in the case of consumer disabling the power domain without
> scaling down the performance level for a power domain that does not
> support enable/disable.

Right. Seems like this isn't needed then.

The genpd driver can just implement the callbacks instead.

>
> Would this behavior in genpd be implemented per power domain or per consumer?
>
>
>> 2) When device are becoming runtime suspended, the ->runtime_suspend()
>> callback of genpd is invoked (genpd_runtime_suspend()). However, in
>> there we don't care to re-evaluate the votes on the performance level,
>> but instead rely on the corresponding driver for the device to drop
>> the vote. I think it would be a good idea of managing this internally
>> in genpd instead, thus, depending on if the aggregated vote can be
>> decreased we should call  ->set_performance_state(new-vote). Of
>> course, once the device get runtime resumed, the votes needs to be
>> restored for the device.
>
> This feature sounds a little risky.  Is it really safe for all cases for
> the genpd framework to unilaterally make these kind of decisions for
> consumers?  Could there be any interdependencies between resources that
> consumers need to enforce that would not be possible if genpd handled
> power state reduction automatically (for example two power domains with a
> sequencing requirement between them)?

In regards to resource/device dependencies, those needs to be properly
manged anyway when using runtime PM. For that we have mechanism for
dealing with parent-childs and the so called device links for
functional dependencies.

For sequencing, that may be an issue, as currently we don't propagate
performance states hierarchically inside genpd. I know Viresh is
looking at addressing this, so perhaps we should come back to this
within that context instead.

Kind regards
Uffe

[PATCH v3 7/7] soc: qcom: rpmpd/rpmhpd: Add a max vote on all corners at init

[Rajendra Nayak]

As we move from no clients/consumers in kernel voting on corners,
to *some* voting and some not voting, we might end up in a situation
where the clients which remove votes can adversly impact others
who still don't have a way to vote.

To avoid this situation, have a max vote on all corners at init.
This should/can be removed once we have all clients moved to
be able to vote/unvote for themselves.

Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/soc/qcom/rpmhpd.c | 12 +++++++++++-
 drivers/soc/qcom/rpmpd.c  |  9 +++++++++
 2 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/soc/qcom/rpmhpd.c b/drivers/soc/qcom/rpmhpd.c
index 7083ec1590ff..3c753d33aeee 100644
--- a/drivers/soc/qcom/rpmhpd.c
+++ b/drivers/soc/qcom/rpmhpd.c
@@ -329,7 +329,7 @@ static int rpmhpd_update_level_mapping(struct rpmhpd *rpmhpd)
 
 static int rpmhpd_probe(struct platform_device *pdev)
 {
-	int i, ret;
+	int i, ret, max_level;
 	size_t num;
 	struct genpd_onecell_data *data;
 	struct rpmhpd **rpmhpds;
@@ -390,6 +390,16 @@ static int rpmhpd_probe(struct platform_device *pdev)
 		pm_genpd_init(&rpmhpds[i]->pd, NULL, true);
 
 		data->domains[i] = &rpmhpds[i]->pd;
+
+		/*
+		 * Until we have all consumers voting on corners
+		 * just vote the max corner on all PDs
+		 * This should ideally be *removed* once we have
+		 * all (most) consumers being able to vote
+		 */
+		max_level = rpmhpds[i]->level_count - 1;
+		rpmhpd_set_performance(&rpmhpds[i]->pd, rpmhpds[i]->level[max_level]);
+		rpmhpd_power_on(&rpmhpds[i]->pd);
 	}
 
 	return of_genpd_add_provider_onecell(pdev->dev.of_node, data);
diff --git a/drivers/soc/qcom/rpmpd.c b/drivers/soc/qcom/rpmpd.c
index 53209454c1f3..6440163305e3 100644
--- a/drivers/soc/qcom/rpmpd.c
+++ b/drivers/soc/qcom/rpmpd.c
@@ -310,6 +310,15 @@ static int rpmpd_probe(struct platform_device *pdev)
 		pm_genpd_init(&rpmpds[i]->pd, NULL, true);
 
 		data->domains[i] = &rpmpds[i]->pd;
+
+		/*
+		 * Until we have all consumers voting on corners
+		 * just vote the max corner on all PDs
+		 * This should ideally be *removed* once we have
+		 * all (most) consumers being able to vote
+		 */
+		rpmpd_set_performance(&rpmpds[i]->pd, MAX_RPMPD_STATE);
+		rpmpd_power_on(&rpmpds[i]->pd);
 	}
 
 	return of_genpd_add_provider_onecell(pdev->dev.of_node, data);
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v3 7/7] soc: qcom: rpmpd/rpmhpd: Add a max vote on all corners at init

[David Collins]

Hello Rajendra,

On 06/11/2018 09:40 PM, Rajendra Nayak wrote:
> As we move from no clients/consumers in kernel voting on corners,
> to *some* voting and some not voting, we might end up in a situation
> where the clients which remove votes can adversly impact others

s/adversly/adversely/

> who still don't have a way to vote.
> 
> To avoid this situation, have a max vote on all corners at init.
> This should/can be removed once we have all clients moved to
> be able to vote/unvote for themselves.

This change seems like a hack.  Do you intend for it to be merged and then
later reverted in Linus's tree?  Could it instead be implemented in a way
that does not require reverting and instead is enabled by some DT
property?  Alternatively, could this feature be added to the power domain
core since it is relatively generic?


> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/soc/qcom/rpmhpd.c | 12 +++++++++++-
>  drivers/soc/qcom/rpmpd.c  |  9 +++++++++
>  2 files changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/soc/qcom/rpmhpd.c b/drivers/soc/qcom/rpmhpd.c
> index 7083ec1590ff..3c753d33aeee 100644
> --- a/drivers/soc/qcom/rpmhpd.c
> +++ b/drivers/soc/qcom/rpmhpd.c
> @@ -329,7 +329,7 @@ static int rpmhpd_update_level_mapping(struct rpmhpd *rpmhpd)
>  
>  static int rpmhpd_probe(struct platform_device *pdev)
>  {
> -	int i, ret;
> +	int i, ret, max_level;
>  	size_t num;
>  	struct genpd_onecell_data *data;
>  	struct rpmhpd **rpmhpds;
> @@ -390,6 +390,16 @@ static int rpmhpd_probe(struct platform_device *pdev)
>  		pm_genpd_init(&rpmhpds[i]->pd, NULL, true);
>  
>  		data->domains[i] = &rpmhpds[i]->pd;
> +
> +		/*
> +		 * Until we have all consumers voting on corners
> +		 * just vote the max corner on all PDs
> +		 * This should ideally be *removed* once we have
> +		 * all (most) consumers being able to vote
> +		 */
> +		max_level = rpmhpds[i]->level_count - 1;
> +		rpmhpd_set_performance(&rpmhpds[i]->pd, rpmhpds[i]->level[max_level]);
> +		rpmhpd_power_on(&rpmhpds[i]->pd);

Clearly these calls will result in max level requests being sent for all
power domains at probe time.  However, it isn't clear that this will
actually help at runtime in these two cases:

1. A consumer enables and then disables a power domain.
	- It seems like the PD would just be disabled in this case.

2. A consumer sets a non-max performance state of a power domain.
	- It seems like the PD would just be set to the new lower
	  performance state since the max vote isn't known to the
	  PD core for aggregation purposes.

Thanks,
David

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

Re: [PATCH v3 7/7] soc: qcom: rpmpd/rpmhpd: Add a max vote on all corners at init

[Rajendra Nayak]

On 06/14/2018 03:58 AM, David Collins wrote:
> Hello Rajendra,
> 
> On 06/11/2018 09:40 PM, Rajendra Nayak wrote:
>> As we move from no clients/consumers in kernel voting on corners,
>> to *some* voting and some not voting, we might end up in a situation
>> where the clients which remove votes can adversly impact others
> 
> s/adversly/adversely/
> 
>> who still don't have a way to vote.
>>
>> To avoid this situation, have a max vote on all corners at init.
>> This should/can be removed once we have all clients moved to
>> be able to vote/unvote for themselves.
> 
> This change seems like a hack.  Do you intend for it to be merged and then
> later reverted in Linus's tree?  Could it instead be implemented in a way
> that does not require reverting and instead is enabled by some DT
> property?  Alternatively, could this feature be added to the power domain
> core since it is relatively generic?
> 
> 
>> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
>> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
>> ---
>>  drivers/soc/qcom/rpmhpd.c | 12 +++++++++++-
>>  drivers/soc/qcom/rpmpd.c  |  9 +++++++++
>>  2 files changed, 20 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/soc/qcom/rpmhpd.c b/drivers/soc/qcom/rpmhpd.c
>> index 7083ec1590ff..3c753d33aeee 100644
>> --- a/drivers/soc/qcom/rpmhpd.c
>> +++ b/drivers/soc/qcom/rpmhpd.c
>> @@ -329,7 +329,7 @@ static int rpmhpd_update_level_mapping(struct rpmhpd *rpmhpd)
>>  
>>  static int rpmhpd_probe(struct platform_device *pdev)
>>  {
>> -	int i, ret;
>> +	int i, ret, max_level;
>>  	size_t num;
>>  	struct genpd_onecell_data *data;
>>  	struct rpmhpd **rpmhpds;
>> @@ -390,6 +390,16 @@ static int rpmhpd_probe(struct platform_device *pdev)
>>  		pm_genpd_init(&rpmhpds[i]->pd, NULL, true);
>>  
>>  		data->domains[i] = &rpmhpds[i]->pd;
>> +
>> +		/*
>> +		 * Until we have all consumers voting on corners
>> +		 * just vote the max corner on all PDs
>> +		 * This should ideally be *removed* once we have
>> +		 * all (most) consumers being able to vote
>> +		 */
>> +		max_level = rpmhpds[i]->level_count - 1;
>> +		rpmhpd_set_performance(&rpmhpds[i]->pd, rpmhpds[i]->level[max_level]);
>> +		rpmhpd_power_on(&rpmhpds[i]->pd);
> 
> Clearly these calls will result in max level requests being sent for all
> power domains at probe time.  However, it isn't clear that this will
> actually help at runtime in these two cases:
> 
> 1. A consumer enables and then disables a power domain.
> 	- It seems like the PD would just be disabled in this case.
> 
> 2. A consumer sets a non-max performance state of a power domain.
> 	- It seems like the PD would just be set to the new lower
> 	  performance state since the max vote isn't known to the
> 	  PD core for aggregation purposes.

Yes, you are right. I certainly did not seem to have thought through this enough.

A need for something like this came up at a point where genpd could not deal with
devices with multiple power domains. So the concern at that point was that if some
consumers (which only need to vote on one corner) move to using this driver, while 
some others (which need to vote on multiple corners) cannot, we would end up breaking
them.

That does not seem to be true anymore since we do have patches from Ulf which support
having devices with multiple power domains attached which can be controlled individually.
So if some consumer voting makes some others break, they should just be fixed and patched
to vote as well. If all this gets handled centrally from within the clock drivers then we
most likely won't even end up with a situation like this.

I think I will just drop this one unless Stephen/Viresh still see an issue with some early
voters breaking others.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH v3 7/7] soc: qcom: rpmpd/rpmhpd: Add a max vote on all corners at init

[Viresh Kumar]

On 14-06-18, 12:05, Rajendra Nayak wrote:
> On 06/14/2018 03:58 AM, David Collins wrote:
> > Hello Rajendra,
> > 
> > On 06/11/2018 09:40 PM, Rajendra Nayak wrote:
> >> As we move from no clients/consumers in kernel voting on corners,
> >> to *some* voting and some not voting, we might end up in a situation
> >> where the clients which remove votes can adversly impact others
> > 
> > s/adversly/adversely/
> > 
> >> who still don't have a way to vote.
> >>
> >> To avoid this situation, have a max vote on all corners at init.
> >> This should/can be removed once we have all clients moved to
> >> be able to vote/unvote for themselves.
> > 
> > This change seems like a hack.  Do you intend for it to be merged and then
> > later reverted in Linus's tree?  Could it instead be implemented in a way
> > that does not require reverting and instead is enabled by some DT
> > property?  Alternatively, could this feature be added to the power domain
> > core since it is relatively generic?
> > 
> > 
> >> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
> >> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> >> ---
> >>  drivers/soc/qcom/rpmhpd.c | 12 +++++++++++-
> >>  drivers/soc/qcom/rpmpd.c  |  9 +++++++++
> >>  2 files changed, 20 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/soc/qcom/rpmhpd.c b/drivers/soc/qcom/rpmhpd.c
> >> index 7083ec1590ff..3c753d33aeee 100644
> >> --- a/drivers/soc/qcom/rpmhpd.c
> >> +++ b/drivers/soc/qcom/rpmhpd.c
> >> @@ -329,7 +329,7 @@ static int rpmhpd_update_level_mapping(struct rpmhpd *rpmhpd)
> >>  
> >>  static int rpmhpd_probe(struct platform_device *pdev)
> >>  {
> >> -	int i, ret;
> >> +	int i, ret, max_level;
> >>  	size_t num;
> >>  	struct genpd_onecell_data *data;
> >>  	struct rpmhpd **rpmhpds;
> >> @@ -390,6 +390,16 @@ static int rpmhpd_probe(struct platform_device *pdev)
> >>  		pm_genpd_init(&rpmhpds[i]->pd, NULL, true);
> >>  
> >>  		data->domains[i] = &rpmhpds[i]->pd;
> >> +
> >> +		/*
> >> +		 * Until we have all consumers voting on corners
> >> +		 * just vote the max corner on all PDs
> >> +		 * This should ideally be *removed* once we have
> >> +		 * all (most) consumers being able to vote
> >> +		 */
> >> +		max_level = rpmhpds[i]->level_count - 1;
> >> +		rpmhpd_set_performance(&rpmhpds[i]->pd, rpmhpds[i]->level[max_level]);
> >> +		rpmhpd_power_on(&rpmhpds[i]->pd);
> > 
> > Clearly these calls will result in max level requests being sent for all
> > power domains at probe time.  However, it isn't clear that this will
> > actually help at runtime in these two cases:
> > 
> > 1. A consumer enables and then disables a power domain.
> > 	- It seems like the PD would just be disabled in this case.

So instead of rpmhpd_power_on() we should be doing gepnd_power_on() or whatever
the API is, so the user count stays at 1.

> > 2. A consumer sets a non-max performance state of a power domain.
> > 	- It seems like the PD would just be set to the new lower
> > 	  performance state since the max vote isn't known to the
> > 	  PD core for aggregation purposes.

Right, and that's because the patch isn't implemented properly yet. I asked to
do a fake vote from some user with their dev structure, so the vote always
stays.

> Yes, you are right. I certainly did not seem to have thought through this enough.
> 
> A need for something like this came up at a point where genpd could not deal with
> devices with multiple power domains. So the concern at that point was that if some
> consumers (which only need to vote on one corner) move to using this driver, while 
> some others (which need to vote on multiple corners) cannot, we would end up breaking
> them.
> 
> That does not seem to be true anymore since we do have patches from Ulf which support
> having devices with multiple power domains attached which can be controlled individually.
> So if some consumer voting makes some others break, they should just be fixed and patched
> to vote as well. If all this gets handled centrally from within the clock drivers then we
> most likely won't even end up with a situation like this.
> 
> I think I will just drop this one unless Stephen/Viresh still see an issue with some early
> voters breaking others.

So what if the LCD/DDR/etc are getting used at boot and someone requests a lower
vote?  Wouldn't we just break ?

-- 
viresh

Re: [PATCH v3 7/7] soc: qcom: rpmpd/rpmhpd: Add a max vote on all corners at init

[Ulf Hansson]

On 19 June 2018 at 12:10, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 14-06-18, 12:05, Rajendra Nayak wrote:
>> On 06/14/2018 03:58 AM, David Collins wrote:
>> > Hello Rajendra,
>> >
>> > On 06/11/2018 09:40 PM, Rajendra Nayak wrote:
>> >> As we move from no clients/consumers in kernel voting on corners,
>> >> to *some* voting and some not voting, we might end up in a situation
>> >> where the clients which remove votes can adversly impact others
>> >
>> > s/adversly/adversely/
>> >
>> >> who still don't have a way to vote.
>> >>
>> >> To avoid this situation, have a max vote on all corners at init.
>> >> This should/can be removed once we have all clients moved to
>> >> be able to vote/unvote for themselves.
>> >
>> > This change seems like a hack.  Do you intend for it to be merged and then
>> > later reverted in Linus's tree?  Could it instead be implemented in a way
>> > that does not require reverting and instead is enabled by some DT
>> > property?  Alternatively, could this feature be added to the power domain
>> > core since it is relatively generic?
>> >
>> >
>> >> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
>> >> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
>> >> ---
>> >>  drivers/soc/qcom/rpmhpd.c | 12 +++++++++++-
>> >>  drivers/soc/qcom/rpmpd.c  |  9 +++++++++
>> >>  2 files changed, 20 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/drivers/soc/qcom/rpmhpd.c b/drivers/soc/qcom/rpmhpd.c
>> >> index 7083ec1590ff..3c753d33aeee 100644
>> >> --- a/drivers/soc/qcom/rpmhpd.c
>> >> +++ b/drivers/soc/qcom/rpmhpd.c
>> >> @@ -329,7 +329,7 @@ static int rpmhpd_update_level_mapping(struct rpmhpd *rpmhpd)
>> >>
>> >>  static int rpmhpd_probe(struct platform_device *pdev)
>> >>  {
>> >> -  int i, ret;
>> >> +  int i, ret, max_level;
>> >>    size_t num;
>> >>    struct genpd_onecell_data *data;
>> >>    struct rpmhpd **rpmhpds;
>> >> @@ -390,6 +390,16 @@ static int rpmhpd_probe(struct platform_device *pdev)
>> >>            pm_genpd_init(&rpmhpds[i]->pd, NULL, true);
>> >>
>> >>            data->domains[i] = &rpmhpds[i]->pd;
>> >> +
>> >> +          /*
>> >> +           * Until we have all consumers voting on corners
>> >> +           * just vote the max corner on all PDs
>> >> +           * This should ideally be *removed* once we have
>> >> +           * all (most) consumers being able to vote
>> >> +           */
>> >> +          max_level = rpmhpds[i]->level_count - 1;
>> >> +          rpmhpd_set_performance(&rpmhpds[i]->pd, rpmhpds[i]->level[max_level]);
>> >> +          rpmhpd_power_on(&rpmhpds[i]->pd);
>> >
>> > Clearly these calls will result in max level requests being sent for all
>> > power domains at probe time.  However, it isn't clear that this will
>> > actually help at runtime in these two cases:
>> >
>> > 1. A consumer enables and then disables a power domain.
>> >     - It seems like the PD would just be disabled in this case.
>
> So instead of rpmhpd_power_on() we should be doing gepnd_power_on() or whatever
> the API is, so the user count stays at 1.

There is no such API.

Instead a device needs to be attached to genpd and that's it. As long
as the device don't enables runtime PM and that the device gets
runtime suspended, genpd will remain powered on.

>
>> > 2. A consumer sets a non-max performance state of a power domain.
>> >     - It seems like the PD would just be set to the new lower
>> >       performance state since the max vote isn't known to the
>> >       PD core for aggregation purposes.
>
> Right, and that's because the patch isn't implemented properly yet. I asked to
> do a fake vote from some user with their dev structure, so the vote always
> stays.
>
>> Yes, you are right. I certainly did not seem to have thought through this enough.
>>
>> A need for something like this came up at a point where genpd could not deal with
>> devices with multiple power domains. So the concern at that point was that if some
>> consumers (which only need to vote on one corner) move to using this driver, while
>> some others (which need to vote on multiple corners) cannot, we would end up breaking
>> them.
>>
>> That does not seem to be true anymore since we do have patches from Ulf which support
>> having devices with multiple power domains attached which can be controlled individually.
>> So if some consumer voting makes some others break, they should just be fixed and patched
>> to vote as well. If all this gets handled centrally from within the clock drivers then we
>> most likely won't even end up with a situation like this.
>>
>> I think I will just drop this one unless Stephen/Viresh still see an issue with some early
>> voters breaking others.
>
> So what if the LCD/DDR/etc are getting used at boot and someone requests a lower
> vote?  Wouldn't we just break ?

Sounds like we need a way to manage votes for "boot constraints
performance levels". :-)

Anyway, to deal with this via the existing genpd APIs, we need to
attach a device to a genpd and then call
dev_pm_genpd_set_performance_state() on it. I guess at a late
initcall, the votes can be dropped and the device can be detached. Or
something along these lines.

Kind regards
Uffe

Re: [PATCH v3 7/7] soc: qcom: rpmpd/rpmhpd: Add a max vote on all corners at init

[Rajendra Nayak]

On 06/25/2018 02:27 PM, Ulf Hansson wrote:
> On 19 June 2018 at 12:10, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>> On 14-06-18, 12:05, Rajendra Nayak wrote:
>>> On 06/14/2018 03:58 AM, David Collins wrote:
>>>> Hello Rajendra,
>>>>
>>>> On 06/11/2018 09:40 PM, Rajendra Nayak wrote:
>>>>> As we move from no clients/consumers in kernel voting on corners,
>>>>> to *some* voting and some not voting, we might end up in a situation
>>>>> where the clients which remove votes can adversly impact others
>>>>
>>>> s/adversly/adversely/
>>>>
>>>>> who still don't have a way to vote.
>>>>>
>>>>> To avoid this situation, have a max vote on all corners at init.
>>>>> This should/can be removed once we have all clients moved to
>>>>> be able to vote/unvote for themselves.
>>>>
>>>> This change seems like a hack.  Do you intend for it to be merged and then
>>>> later reverted in Linus's tree?  Could it instead be implemented in a way
>>>> that does not require reverting and instead is enabled by some DT
>>>> property?  Alternatively, could this feature be added to the power domain
>>>> core since it is relatively generic?
>>>>
>>>>
>>>>> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
>>>>> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
>>>>> ---
>>>>>  drivers/soc/qcom/rpmhpd.c | 12 +++++++++++-
>>>>>  drivers/soc/qcom/rpmpd.c  |  9 +++++++++
>>>>>  2 files changed, 20 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/soc/qcom/rpmhpd.c b/drivers/soc/qcom/rpmhpd.c
>>>>> index 7083ec1590ff..3c753d33aeee 100644
>>>>> --- a/drivers/soc/qcom/rpmhpd.c
>>>>> +++ b/drivers/soc/qcom/rpmhpd.c
>>>>> @@ -329,7 +329,7 @@ static int rpmhpd_update_level_mapping(struct rpmhpd *rpmhpd)
>>>>>
>>>>>  static int rpmhpd_probe(struct platform_device *pdev)
>>>>>  {
>>>>> -  int i, ret;
>>>>> +  int i, ret, max_level;
>>>>>    size_t num;
>>>>>    struct genpd_onecell_data *data;
>>>>>    struct rpmhpd **rpmhpds;
>>>>> @@ -390,6 +390,16 @@ static int rpmhpd_probe(struct platform_device *pdev)
>>>>>            pm_genpd_init(&rpmhpds[i]->pd, NULL, true);
>>>>>
>>>>>            data->domains[i] = &rpmhpds[i]->pd;
>>>>> +
>>>>> +          /*
>>>>> +           * Until we have all consumers voting on corners
>>>>> +           * just vote the max corner on all PDs
>>>>> +           * This should ideally be *removed* once we have
>>>>> +           * all (most) consumers being able to vote
>>>>> +           */
>>>>> +          max_level = rpmhpds[i]->level_count - 1;
>>>>> +          rpmhpd_set_performance(&rpmhpds[i]->pd, rpmhpds[i]->level[max_level]);
>>>>> +          rpmhpd_power_on(&rpmhpds[i]->pd);
>>>>
>>>> Clearly these calls will result in max level requests being sent for all
>>>> power domains at probe time.  However, it isn't clear that this will
>>>> actually help at runtime in these two cases:
>>>>
>>>> 1. A consumer enables and then disables a power domain.
>>>>     - It seems like the PD would just be disabled in this case.
>>
>> So instead of rpmhpd_power_on() we should be doing gepnd_power_on() or whatever
>> the API is, so the user count stays at 1.
> 
> There is no such API.
> 
> Instead a device needs to be attached to genpd and that's it. As long
> as the device don't enables runtime PM and that the device gets
> runtime suspended, genpd will remain powered on.

Its more to do with keeping the power domains at a desired 'performance level'
than just keeping them on.

> 
>>
>>>> 2. A consumer sets a non-max performance state of a power domain.
>>>>     - It seems like the PD would just be set to the new lower
>>>>       performance state since the max vote isn't known to the
>>>>       PD core for aggregation purposes.
>>
>> Right, and that's because the patch isn't implemented properly yet. I asked to
>> do a fake vote from some user with their dev structure, so the vote always
>> stays.
>>
>>> Yes, you are right. I certainly did not seem to have thought through this enough.
>>>
>>> A need for something like this came up at a point where genpd could not deal with
>>> devices with multiple power domains. So the concern at that point was that if some
>>> consumers (which only need to vote on one corner) move to using this driver, while
>>> some others (which need to vote on multiple corners) cannot, we would end up breaking
>>> them.
>>>
>>> That does not seem to be true anymore since we do have patches from Ulf which support
>>> having devices with multiple power domains attached which can be controlled individually.
>>> So if some consumer voting makes some others break, they should just be fixed and patched
>>> to vote as well. If all this gets handled centrally from within the clock drivers then we
>>> most likely won't even end up with a situation like this.
>>>
>>> I think I will just drop this one unless Stephen/Viresh still see an issue with some early
>>> voters breaking others.
>>
>> So what if the LCD/DDR/etc are getting used at boot and someone requests a lower
>> vote?  Wouldn't we just break ?
> 
> Sounds like we need a way to manage votes for "boot constraints
> performance levels". :-)

Yes, I think we are mixing up whats needed for 'boot constraints' and what this
patch was meant to do.
Boot constraints is a generic problem not limited to power domains alone and this patch
isn't trying to solve that.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH v3 0/7] Add powerdomain driver for corners on msm8996/sdm845

[Ulf Hansson]

On 12 June 2018 at 06:40, Rajendra Nayak <rnayak@codeaurora.org> wrote:
> Changes in v3:
> * Bindings split into seperate patches
> * Bindings updated to remove duplicate OPP table phandles
> * DT headers defining macros for Power domain indexes and OPP levels
> * Optimisations to use rpmh_write_async() whereever applicable
> * Fixed up handling of ACTIVE_ONLY/WAKE_ONLY/SLEEP voting for RPMh
> * Fixed the vlvl to hlvl conversions in set_performance
> * Other minor fixes based on review of v2
> * TODO: This series does not handle the case where all VDD_MX votes
> should be higher than VDD_CX from APPs, as pointed out
> by David Collins in v2. This needs support at genpd to propogate performance
> state up the parents, if we model these as Parent/Child to handle the
> interdependency.
>
> Changes in v2:
> * added a powerdomain driver for sdm845 which supports communicating to RPMh
> * dropped the changes to sdhc driver to move over to using OPP
> as there is active discussion on using OPP as the interface vs
> handling all of it in clock drivers
> * Other minor binding updates based on review of v1
>
> With performance state support for genpd/OPP merged, this is an effort
> to model a powerdomain driver to communicate corner/level
> values for qualcomm platforms to RPM (Remote Power Manager) and RPMh.
>
> Rajendra Nayak (7):
>   dt-bindings: power: Add qcom rpm power domain driver bindings
>   soc: qcom: rpmpd: Add a Power domain driver to model corners
>   soc: qcom: rpmpd: Add support for get/set performance state
>   arm64: dts: msm8996: Add rpmpd device node
>   dt-bindings: power: Add qcom rpmh power domain driver bindings
>   soc: qcom: Add RPMh Power domain driver
>   soc: qcom: rpmpd/rpmhpd: Add a max vote on all corners at init
>
>  .../devicetree/bindings/power/qcom,rpmhpd.txt |  65 +++
>  .../devicetree/bindings/power/qcom,rpmpd.txt  |  49 ++
>  arch/arm64/boot/dts/qcom/msm8996.dtsi         |  34 ++
>  drivers/soc/qcom/Kconfig                      |  18 +
>  drivers/soc/qcom/Makefile                     |   2 +
>  drivers/soc/qcom/rpmhpd.c                     | 437 ++++++++++++++++++
>  drivers/soc/qcom/rpmpd.c                      | 356 ++++++++++++++
>  include/dt-bindings/power/qcom-rpmhpd.h       |  31 ++
>  include/dt-bindings/power/qcom-rpmpd.h        |  16 +
>  9 files changed, 1008 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power/qcom,rpmhpd.txt
>  create mode 100644 Documentation/devicetree/bindings/power/qcom,rpmpd.txt
>  create mode 100644 drivers/soc/qcom/rpmhpd.c
>  create mode 100644 drivers/soc/qcom/rpmpd.c
>  create mode 100644 include/dt-bindings/power/qcom-rpmhpd.h
>  create mode 100644 include/dt-bindings/power/qcom-rpmpd.h
>
> --
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation
>

For the series:

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

Kind regards
Uffe