[PATCH v2 0/6] Add powerdomain driver for corners on msm8996/sdm845

[PATCH v2 0/6] Add powerdomain driver for corners on msm8996/sdm845

[Rajendra Nayak]

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 merged, and with some more
patches to add support for them in the OPP layer [1] in linux-next,
this is an effort to model a powerdomain driver to communicate corner/level
values for qualcomm platforms to RPM (Remote Power Manager) and RPMh.

The series is based on linux-next as it depends on OPP updates and SMD845
specific patches which are all in linux-next. It also depends on the RPMH
communication patches [2] for the sdm845 rpmhpd driver.

[1] https://lwn.net/Articles/742136/
[2] https://lkml.org/lkml/2018/5/9/729

Rajendra Nayak (6):
  soc: qcom: rpmpd: Add a powerdomain driver to model corners
  dt-bindings: opp: Introduce qcom-opp bindings
  soc: qcom: rpmpd: Add support for get/set performance state
  arm64: dts: msm8996: Add rpmpd device node
  soc: qcom: rpmh powerdomain driver
  soc: qcom: rpmpd/rpmhpd: Add a max vote on all corners at init

 .../devicetree/bindings/opp/qcom-opp.txt      |  25 ++
 .../devicetree/bindings/power/qcom,rpmhpd.txt |  65 +++
 .../devicetree/bindings/power/qcom,rpmpd.txt  |  55 +++
 arch/arm64/boot/dts/qcom/msm8996.dtsi         |  40 ++
 drivers/soc/qcom/Kconfig                      |  18 +
 drivers/soc/qcom/Makefile                     |   2 +
 drivers/soc/qcom/rpmhpd.c                     | 369 ++++++++++++++++++
 drivers/soc/qcom/rpmpd.c                      | 354 +++++++++++++++++
 8 files changed, 928 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/opp/qcom-opp.txt
 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

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

[PATCH v2 1/6] soc: qcom: rpmpd: Add a powerdomain driver to model corners

[Rajendra Nayak]

The powerdomains 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 powerdomain 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>
---
 .../devicetree/bindings/power/qcom,rpmpd.txt  |  55 ++++
 drivers/soc/qcom/Kconfig                      |   9 +
 drivers/soc/qcom/Makefile                     |   1 +
 drivers/soc/qcom/rpmpd.c                      | 299 ++++++++++++++++++
 4 files changed, 364 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/qcom,rpmpd.txt
 create mode 100644 drivers/soc/qcom/rpmpd.c

diff --git a/Documentation/devicetree/bindings/power/qcom,rpmpd.txt b/Documentation/devicetree/bindings/power/qcom,rpmpd.txt
new file mode 100644
index 000000000000..68f620a2af0d
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/qcom,rpmpd.txt
@@ -0,0 +1,55 @@
+Qualcomm RPM Powerdomains
+
+* For RPM powerdomains, 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 Powerdomain 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>,
+				      <&rpmpd_opp_table>,
+				      <&rpmpd_opp_table>,
+				      <&rpmpd_opp_table>,
+				      <&rpmpd_opp_table>,
+				      <&rpmpd_opp_table>;
+	};
+
+	rpmpd_opp_table: opp-table {
+		compatible = "operating-points-v2-qcom-level", "operating-points-v2";
+
+		rpmpd_opp1: opp@1 {
+			qcom,level = <1>;
+		};
+
+		rpmpd_opp2: opp@2 {
+			qcom,level = <2>;
+		};
+
+		rpmpd_opp3: opp@3 {
+			qcom,level = <3>;
+		};
+
+		rpmpd_opp4: opp@4 {
+			qcom,level = <4>;
+		};
+
+		rpmpd_opp5: opp@5 {
+			qcom,level = <5>;
+		};
+
+		rpmpd_opp6: opp@6 {
+			qcom,level = <6>;
+		};
+	};
diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index 9dc02f390ba3..a7a405178967 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 Powerdomain driver"
+	depends on MFD_QCOM_RPM && QCOM_SMD_RPM
+	help
+	  QCOM RPM powerdomain driver to support powerdomain 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..df6427d43414
--- /dev/null
+++ b/drivers/soc/qcom/rpmpd.c
@@ -0,0 +1,299 @@
+// 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>
+
+#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 long 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 powerdomains */
+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[] = {
+	[0] = &msm8996_vddcx,
+	[1] = &msm8996_vddcx_ao,
+	[2] = &msm8996_vddcx_vfc,
+	[3] = &msm8996_vddmx,
+	[4] = &msm8996_vddmx_ao,
+	[5] = &msm8996_vddsscx,
+	[6] = &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 long corner,
+			    unsigned long *active, unsigned long *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 long active_corner, sleep_corner;
+	unsigned long this_corner = 0, this_sleep_corner = 0;
+	unsigned long peer_corner = 0, peer_sleep_corner = 0;
+
+	to_active_sleep(pd, pd->corner, &this_corner, &this_sleep_corner);
+
+	if (peer && peer->enabled)
+		to_active_sleep(peer, peer->corner, &peer_corner,
+				&peer_sleep_corner);
+
+	active_corner = max(this_corner, peer_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])
+			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 RPM Power Domain Driver");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:qcom-rpmpd");
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v2 1/6] soc: qcom: rpmpd: Add a powerdomain driver to model corners

[Ulf Hansson]

On 25 May 2018 at 12:01, Rajendra Nayak <rnayak@codeaurora.org> wrote:
> The powerdomains 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 powerdomain 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>
> ---
>  .../devicetree/bindings/power/qcom,rpmpd.txt  |  55 ++++

Please split DT doc changes into separate patches, to simplify review.

>  drivers/soc/qcom/Kconfig                      |   9 +
>  drivers/soc/qcom/Makefile                     |   1 +
>  drivers/soc/qcom/rpmpd.c                      | 299 ++++++++++++++++++
>  4 files changed, 364 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power/qcom,rpmpd.txt
>  create mode 100644 drivers/soc/qcom/rpmpd.c
>

[...]

> +++ b/drivers/soc/qcom/rpmpd.c
> @@ -0,0 +1,299 @@
> +// 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>
> +
> +#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 long 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 powerdomains */
> +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[] = {
> +       [0] = &msm8996_vddcx,
> +       [1] = &msm8996_vddcx_ao,
> +       [2] = &msm8996_vddcx_vfc,
> +       [3] = &msm8996_vddmx,
> +       [4] = &msm8996_vddmx_ao,
> +       [5] = &msm8996_vddsscx,
> +       [6] = &msm8996_vddsscx_vfc,
> +};

It's not my call, but honestly the above all macros makes the code
less readable.

Anyway, I think you should convert to allocate these structs
dynamically from the heap (kzalloc/kcalloc), instead of statically as
above.

> +
> +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_aggregate_corner(struct rpmpd *pd)
> +{

Isn't the aggregation of the performance states in genpd sufficient
for your case?

I guess this is SoC specific and needed anyways, but then could you
perhaps add a few comments about what goes on here?

> +       int ret;
> +       struct rpmpd *peer = pd->peer;
> +       unsigned long active_corner, sleep_corner;
> +       unsigned long this_corner = 0, this_sleep_corner = 0;
> +       unsigned long peer_corner = 0, peer_sleep_corner = 0;
> +
> +       to_active_sleep(pd, pd->corner, &this_corner, &this_sleep_corner);
> +
> +       if (peer && peer->enabled)
> +               to_active_sleep(peer, peer->corner, &peer_corner,
> +                               &peer_sleep_corner);
> +
> +       active_corner = max(this_corner, peer_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_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])
> +                       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);

Question: Is there no hierarchical topology of the PM domains. No
genpd subdomains?

> +
> +               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 RPM Power Domain Driver");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:qcom-rpmpd");
> --
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation
>

Besides the minor things above, this looks good to me.

Kind regards
Uffe

Re: [PATCH v2 1/6] soc: qcom: rpmpd: Add a powerdomain driver to model corners

[Rajendra Nayak]

On 05/30/2018 02:47 PM, Ulf Hansson wrote:
> On 25 May 2018 at 12:01, Rajendra Nayak <rnayak@codeaurora.org> wrote:
>> The powerdomains 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 powerdomain 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>
>> ---
>>  .../devicetree/bindings/power/qcom,rpmpd.txt  |  55 ++++
> 
> Please split DT doc changes into separate patches, to simplify review.

yes, i will split it when I resend the series.

> 
>>  drivers/soc/qcom/Kconfig                      |   9 +
>>  drivers/soc/qcom/Makefile                     |   1 +
>>  drivers/soc/qcom/rpmpd.c                      | 299 ++++++++++++++++++
>>  4 files changed, 364 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/power/qcom,rpmpd.txt
>>  create mode 100644 drivers/soc/qcom/rpmpd.c
>>
> 
> [...]
> 
>> +++ b/drivers/soc/qcom/rpmpd.c
>> @@ -0,0 +1,299 @@
>> +// 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>
>> +
>> +#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 long 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 powerdomains */
>> +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[] = {
>> +       [0] = &msm8996_vddcx,
>> +       [1] = &msm8996_vddcx_ao,
>> +       [2] = &msm8996_vddcx_vfc,
>> +       [3] = &msm8996_vddmx,
>> +       [4] = &msm8996_vddmx_ao,
>> +       [5] = &msm8996_vddsscx,
>> +       [6] = &msm8996_vddsscx_vfc,
>> +};
> 
> It's not my call, but honestly the above all macros makes the code
> less readable.

This is all static data per SoC. The macros will keep the new additions
needed for every new SoC to a minimal. Currently this supports only
msm8996.

> 
> Anyway, I think you should convert to allocate these structs
> dynamically from the heap (kzalloc/kcalloc), instead of statically as
> above.
> 
>> +
>> +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_aggregate_corner(struct rpmpd *pd)
>> +{
> 
> Isn't the aggregation of the performance states in genpd sufficient
> for your case?
> 
> I guess this is SoC specific and needed anyways, but then could you
> perhaps add a few comments about what goes on here?

Yes, this is SoC specific aggregation for active and sleep votes.
i will add comments to clarify this is different from the aggregation
done at the framework level.

> 
>> +       int ret;
>> +       struct rpmpd *peer = pd->peer;
>> +       unsigned long active_corner, sleep_corner;
>> +       unsigned long this_corner = 0, this_sleep_corner = 0;
>> +       unsigned long peer_corner = 0, peer_sleep_corner = 0;
>> +
>> +       to_active_sleep(pd, pd->corner, &this_corner, &this_sleep_corner);
>> +
>> +       if (peer && peer->enabled)
>> +               to_active_sleep(peer, peer->corner, &peer_corner,
>> +                               &peer_sleep_corner);
>> +
>> +       active_corner = max(this_corner, peer_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_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])
>> +                       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);
> 
> Question: Is there no hierarchical topology of the PM domains. No
> genpd subdomains?

The hierarchy if any is all handled by the remote core (RPM in this case).
For Linux its just a flat view.

> 
>> +
>> +               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 RPM Power Domain Driver");
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_ALIAS("platform:qcom-rpmpd");
>> --
>> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
>> of Code Aurora Forum, hosted by The Linux Foundation
>>
> 
> Besides the minor things above, this looks good to me.

thanks,
Rajendra


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

Re: [PATCH v2 1/6] soc: qcom: rpmpd: Add a powerdomain driver to model corners

[Ulf Hansson]

[...]

>>> +
>>> +static DEFINE_MUTEX(rpmpd_lock);
>>> +
>>> +/* msm8996 RPM powerdomains */
>>> +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[] = {
>>> +       [0] = &msm8996_vddcx,
>>> +       [1] = &msm8996_vddcx_ao,
>>> +       [2] = &msm8996_vddcx_vfc,
>>> +       [3] = &msm8996_vddmx,
>>> +       [4] = &msm8996_vddmx_ao,
>>> +       [5] = &msm8996_vddsscx,
>>> +       [6] = &msm8996_vddsscx_vfc,
>>> +};
>>
>> It's not my call, but honestly the above all macros makes the code
>> less readable.
>
> This is all static data per SoC. The macros will keep the new additions
> needed for every new SoC to a minimal. Currently this supports only
> msm8996.

Right, that's fine then.

>
>>
>> Anyway, I think you should convert to allocate these structs
>> dynamically from the heap (kzalloc/kcalloc), instead of statically as
>> above.

However, I assume this is still doable!?

[...]

>>> +       for (i = 0; i < num; i++) {
>>> +               if (!rpmpds[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);
>>
>> Question: Is there no hierarchical topology of the PM domains. No
>> genpd subdomains?
>
> The hierarchy if any is all handled by the remote core (RPM in this case).
> For Linux its just a flat view.

Okay, thanks for clarifying!

Kind regards
Uffe

Re: [PATCH v2 1/6] soc: qcom: rpmpd: Add a powerdomain driver to model corners

[Rajendra Nayak]

On 05/30/2018 06:14 PM, Ulf Hansson wrote:
> [...]
> 
>>>> +
>>>> +static DEFINE_MUTEX(rpmpd_lock);
>>>> +
>>>> +/* msm8996 RPM powerdomains */
>>>> +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[] = {
>>>> +       [0] = &msm8996_vddcx,
>>>> +       [1] = &msm8996_vddcx_ao,
>>>> +       [2] = &msm8996_vddcx_vfc,
>>>> +       [3] = &msm8996_vddmx,
>>>> +       [4] = &msm8996_vddmx_ao,
>>>> +       [5] = &msm8996_vddsscx,
>>>> +       [6] = &msm8996_vddsscx_vfc,
>>>> +};
>>>
>>> It's not my call, but honestly the above all macros makes the code
>>> less readable.
>>
>> This is all static data per SoC. The macros will keep the new additions
>> needed for every new SoC to a minimal. Currently this supports only
>> msm8996.
> 
> Right, that's fine then.
> 
>>
>>>
>>> Anyway, I think you should convert to allocate these structs
>>> dynamically from the heap (kzalloc/kcalloc), instead of statically as
>>> above.
> 
> However, I assume this is still doable!?

Perhaps it is, but is there any specific advantage of constructing these structures
dynamically vs statically, given they are static data?
Most other powerdomain/clock/regulator drivers I see do it statically, and thats
what I followed.

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

Re: [PATCH v2 1/6] soc: qcom: rpmpd: Add a powerdomain driver to model corners

[Ulf Hansson]

On 31 May 2018 at 06:20, Rajendra Nayak <rnayak@codeaurora.org> wrote:
>
>
> On 05/30/2018 06:14 PM, Ulf Hansson wrote:
>> [...]
>>
>>>>> +
>>>>> +static DEFINE_MUTEX(rpmpd_lock);
>>>>> +
>>>>> +/* msm8996 RPM powerdomains */
>>>>> +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[] = {
>>>>> +       [0] = &msm8996_vddcx,
>>>>> +       [1] = &msm8996_vddcx_ao,
>>>>> +       [2] = &msm8996_vddcx_vfc,
>>>>> +       [3] = &msm8996_vddmx,
>>>>> +       [4] = &msm8996_vddmx_ao,
>>>>> +       [5] = &msm8996_vddsscx,
>>>>> +       [6] = &msm8996_vddsscx_vfc,
>>>>> +};
>>>>
>>>> It's not my call, but honestly the above all macros makes the code
>>>> less readable.
>>>
>>> This is all static data per SoC. The macros will keep the new additions
>>> needed for every new SoC to a minimal. Currently this supports only
>>> msm8996.
>>
>> Right, that's fine then.
>>
>>>
>>>>
>>>> Anyway, I think you should convert to allocate these structs
>>>> dynamically from the heap (kzalloc/kcalloc), instead of statically as
>>>> above.
>>
>> However, I assume this is still doable!?
>
> Perhaps it is, but is there any specific advantage of constructing these structures
> dynamically vs statically, given they are static data?

Well, I was just thinking that the genpd struct has grown quite big.

> Most other powerdomain/clock/regulator drivers I see do it statically, and thats
> what I followed.

Right, so forget it and keep it as is.

Kind regards
Uffe

Re: [PATCH v2 1/6] soc: qcom: rpmpd: Add a powerdomain driver to model corners

[David Collins]

Hello Rajendra,

On 05/30/2018 03:14 AM, Rajendra Nayak wrote:
> On 05/30/2018 02:47 PM, Ulf Hansson wrote:
>> On 25 May 2018 at 12:01, Rajendra Nayak <rnayak@codeaurora.org> wrote:
...
>>> +               pm_genpd_init(&rpmpds[i]->pd, NULL, true);
>>
>> Question: Is there no hierarchical topology of the PM domains. No
>> genpd subdomains?
> 
> The hierarchy if any is all handled by the remote core (RPM in this case).
> For Linux its just a flat view.

There is one special case that we'll need to handle somehow.  The APPS
vlvl request for VDD_MX needs to be greater than or equal to the vlvl
request for VDD_CX.  Can you please add the necessary code to achieve
this?  RPMh hardware doesn't handle this hardware requirement due to
concerns about modem use case latency.

Please note that this is handled in a somewhat hacky manner [1] with the
downstream rpmh-regulator driver by specifying VDD_MX as the parent of
VDD_CX and VDD_MX_AO as the parent of VDD_CX_AO with a dropout voltage of
-1.  That way, enabling CX causes MX to be enabled and voltage level
requests are propagated from CX to MX (the -1 is ignored because it is
rounded up within the sparse vlvl numbering space).

Thanks,
David

[1]:
https://source.codeaurora.org/quic/la/kernel/msm-4.9/tree/arch/arm64/boot/dts/qcom/sdm845-regulator.dtsi?h=msm-4.9#n135

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

Re: [PATCH v2 1/6] soc: qcom: rpmpd: Add a powerdomain driver to model corners

[Rajendra Nayak]

Hi David,

On 05/30/2018 11:57 PM, David Collins wrote:
> Hello Rajendra,
> 
> On 05/30/2018 03:14 AM, Rajendra Nayak wrote:
>> On 05/30/2018 02:47 PM, Ulf Hansson wrote:
>>> On 25 May 2018 at 12:01, Rajendra Nayak <rnayak@codeaurora.org> wrote:
> ...
>>>> +               pm_genpd_init(&rpmpds[i]->pd, NULL, true);
>>>
>>> Question: Is there no hierarchical topology of the PM domains. No
>>> genpd subdomains?
>>
>> The hierarchy if any is all handled by the remote core (RPM in this case).
>> For Linux its just a flat view.
> 
> There is one special case that we'll need to handle somehow.  The APPS
> vlvl request for VDD_MX needs to be greater than or equal to the vlvl
> request for VDD_CX.  Can you please add the necessary code to achieve
> this?  RPMh hardware doesn't handle this hardware requirement due to
> concerns about modem use case latency.

Sure, I'll take a look at it.

> 
> Please note that this is handled in a somewhat hacky manner [1] with the
> downstream rpmh-regulator driver by specifying VDD_MX as the parent of
> VDD_CX and VDD_MX_AO as the parent of VDD_CX_AO with a dropout voltage of
> -1.  That way, enabling CX causes MX to be enabled and voltage level
> requests are propagated from CX to MX (the -1 is ignored because it is
> rounded up within the sparse vlvl numbering space).

I can't see how else to handle this but with a fake parent/child relation,
which also means we might need support to propagate performance states for
power domains up the parents, which I think was initially supported but
later dropped since we thought this wasn't needed for now.
We might need to take a re-look at it to support this usecase.

thanks,
Rajendra

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

Re: [PATCH v2 1/6] soc: qcom: rpmpd: Add a powerdomain driver to model corners

[Rob Herring]

On Fri, May 25, 2018 at 03:31:16PM +0530, Rajendra Nayak wrote:
> The powerdomains 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 powerdomain 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>
> ---
>  .../devicetree/bindings/power/qcom,rpmpd.txt  |  55 ++++
>  drivers/soc/qcom/Kconfig                      |   9 +
>  drivers/soc/qcom/Makefile                     |   1 +
>  drivers/soc/qcom/rpmpd.c                      | 299 ++++++++++++++++++
>  4 files changed, 364 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power/qcom,rpmpd.txt
>  create mode 100644 drivers/soc/qcom/rpmpd.c
> 
> diff --git a/Documentation/devicetree/bindings/power/qcom,rpmpd.txt b/Documentation/devicetree/bindings/power/qcom,rpmpd.txt
> new file mode 100644
> index 000000000000..68f620a2af0d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/qcom,rpmpd.txt
> @@ -0,0 +1,55 @@
> +Qualcomm RPM Powerdomains
> +
> +* For RPM powerdomains, 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 Powerdomain 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>,
> +				      <&rpmpd_opp_table>,
> +				      <&rpmpd_opp_table>,
> +				      <&rpmpd_opp_table>,
> +				      <&rpmpd_opp_table>,
> +				      <&rpmpd_opp_table>;
> +	};
> +
> +	rpmpd_opp_table: opp-table {
> +		compatible = "operating-points-v2-qcom-level", "operating-points-v2";
> +
> +		rpmpd_opp1: opp@1 {

unit-address without reg property is not valid.

> +			qcom,level = <1>;

Is this the only property? If so, I don't see the point in trying to use 
operating-points-v2 here.

Rob

Re: [PATCH v2 1/6] soc: qcom: rpmpd: Add a powerdomain driver to model corners

[Rajendra Nayak]

On 05/31/2018 08:57 AM, Rob Herring wrote:
> On Fri, May 25, 2018 at 03:31:16PM +0530, Rajendra Nayak wrote:
>> The powerdomains 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 powerdomain 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>
>> ---
>>  .../devicetree/bindings/power/qcom,rpmpd.txt  |  55 ++++
>>  drivers/soc/qcom/Kconfig                      |   9 +
>>  drivers/soc/qcom/Makefile                     |   1 +
>>  drivers/soc/qcom/rpmpd.c                      | 299 ++++++++++++++++++
>>  4 files changed, 364 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/power/qcom,rpmpd.txt
>>  create mode 100644 drivers/soc/qcom/rpmpd.c
>>
>> diff --git a/Documentation/devicetree/bindings/power/qcom,rpmpd.txt b/Documentation/devicetree/bindings/power/qcom,rpmpd.txt
>> new file mode 100644
>> index 000000000000..68f620a2af0d
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/power/qcom,rpmpd.txt
>> @@ -0,0 +1,55 @@
>> +Qualcomm RPM Powerdomains
>> +
>> +* For RPM powerdomains, 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 Powerdomain 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>,
>> +				      <&rpmpd_opp_table>,
>> +				      <&rpmpd_opp_table>,
>> +				      <&rpmpd_opp_table>,
>> +				      <&rpmpd_opp_table>,
>> +				      <&rpmpd_opp_table>;
>> +	};
>> +
>> +	rpmpd_opp_table: opp-table {
>> +		compatible = "operating-points-v2-qcom-level", "operating-points-v2";
>> +
>> +		rpmpd_opp1: opp@1 {
> 
> unit-address without reg property is not valid.
> 
>> +			qcom,level = <1>;
> 
> Is this the only property? If so, I don't see the point in trying to use 
> operating-points-v2 here.

Thanks, I'll drop the unit address above and the compatible here.

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

[PATCH v2 2/6] dt-bindings: opp: Introduce qcom-opp bindings

[Rajendra Nayak]

On Qualcomm platforms, an OPP node needs to describe an
additional level/corner value that is then communicated to
a remote microprocessor by the CPU, which then takes some
actions (like adjusting voltage values across various rails)
based on the value passed.

Describe these bindings in the qcom-opp bindings document.

Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 .../devicetree/bindings/opp/qcom-opp.txt      | 25 +++++++++++++++++++
 1 file changed, 25 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/opp/qcom-opp.txt

diff --git a/Documentation/devicetree/bindings/opp/qcom-opp.txt b/Documentation/devicetree/bindings/opp/qcom-opp.txt
new file mode 100644
index 000000000000..db4d970c7ec7
--- /dev/null
+++ b/Documentation/devicetree/bindings/opp/qcom-opp.txt
@@ -0,0 +1,25 @@
+Qualcomm OPP bindings to descibe OPP nodes with corner/level values
+
+OPP tables for devices on Qualcomm platforms require an additional
+platform specific corner/level value to be specified.
+This value is passed on to the RPM (Resource Power Manager) by
+the CPU, which then takes the necessary actions to set a voltage
+rail to an appropriate voltage based on the value passed.
+
+The bindings are based on top of the operating-points-v2 bindings
+described in Documentation/devicetree/bindings/opp/opp.txt
+Additional properties are described below.
+
+* OPP Table Node
+
+Required properties:
+- compatible: Allow OPPs to express their compatibility. It should be:
+  "operating-points-v2-qcom-level"
+
+* OPP Node
+
+Required properties:
+- qcom,level: On Qualcomm platforms an OPP node can describe a positive value
+representing a corner/level that's communicated with a remote microprocessor
+(usually called the RPM) which then translates it into a certain voltage on
+a voltage rail.
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v2 2/6] dt-bindings: opp: Introduce qcom-opp bindings

[David Collins]

Hello Rajendra,

On 05/25/2018 03:01 AM, Rajendra Nayak wrote:
> On Qualcomm platforms, an OPP node needs to describe an

s/Qualcomm/Qualcomm Technologies, Inc./


> additional level/corner value that is then communicated to
> a remote microprocessor by the CPU, which then takes some
> actions (like adjusting voltage values across various rails)
> based on the value passed.
> 
> Describe these bindings in the qcom-opp bindings document.
> 
> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> Reviewed-by: Rob Herring <robh@kernel.org>
> ---
>  .../devicetree/bindings/opp/qcom-opp.txt      | 25 +++++++++++++++++++
>  1 file changed, 25 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/opp/qcom-opp.txt
> 
> diff --git a/Documentation/devicetree/bindings/opp/qcom-opp.txt b/Documentation/devicetree/bindings/opp/qcom-opp.txt
> new file mode 100644
> index 000000000000..db4d970c7ec7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/opp/qcom-opp.txt
> @@ -0,0 +1,25 @@
> +Qualcomm OPP bindings to descibe OPP nodes with corner/level values

s/Qualcomm/Qualcomm Technologies, Inc./ ?

s/descibe/describe/


> +
> +OPP tables for devices on Qualcomm platforms require an additional

s/Qualcomm/Qualcomm Technologies, Inc./


> +platform specific corner/level value to be specified.
> +This value is passed on to the RPM (Resource Power Manager) by

Do you want to mention RPMh here as well?


> +the CPU, which then takes the necessary actions to set a voltage
> +rail to an appropriate voltage based on the value passed.
> +
> +The bindings are based on top of the operating-points-v2 bindings
> +described in Documentation/devicetree/bindings/opp/opp.txt
> +Additional properties are described below.
> +
> +* OPP Table Node
> +
> +Required properties:
> +- compatible: Allow OPPs to express their compatibility. It should be:
> +  "operating-points-v2-qcom-level"
> +
> +* OPP Node
> +
> +Required properties:
> +- qcom,level: On Qualcomm platforms an OPP node can describe a positive value

s/Qualcomm/Qualcomm Technologies, Inc./


> +representing a corner/level that's communicated with a remote microprocessor
> +(usually called the RPM) which then translates it into a certain voltage on
> +a voltage rail.

Should these lines be indented 2 spaces as is the case for the compatible
property above?

Could you add an example for both RPM and RPMh in this binding file?

Thanks,
David

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

Re: [PATCH v2 2/6] dt-bindings: opp: Introduce qcom-opp bindings

[Rajendra Nayak]

On 05/26/2018 04:03 AM, David Collins wrote:
> Hello Rajendra,
> 
> On 05/25/2018 03:01 AM, Rajendra Nayak wrote:
>> On Qualcomm platforms, an OPP node needs to describe an
> 
> s/Qualcomm/Qualcomm Technologies, Inc./
> 
> 

[] ..

> 
> s/Qualcomm/Qualcomm Technologies, Inc./ ?
> 
> s/descibe/describe/
> 
> 
>> +
>> +OPP tables for devices on Qualcomm platforms require an additional
> 
> s/Qualcomm/Qualcomm Technologies, Inc./
> 
> 
>> +platform specific corner/level value to be specified.
>> +This value is passed on to the RPM (Resource Power Manager) by
> 
> Do you want to mention RPMh here as well?
> 
> 

[] ..

>> +
>> +Required properties:
>> +- qcom,level: On Qualcomm platforms an OPP node can describe a positive value
> 
> s/Qualcomm/Qualcomm Technologies, Inc./
> 
> 
>> +representing a corner/level that's communicated with a remote microprocessor
>> +(usually called the RPM) which then translates it into a certain voltage on
>> +a voltage rail.
> 
> Should these lines be indented 2 spaces as is the case for the compatible
> property above?
> 
> Could you add an example for both RPM and RPMh in this binding file?

Thanks David for the review. I will fix all this up when I respin.

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

[PATCH v2 3/6] 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 df6427d43414..ff3d910f388c 100644
--- a/drivers/soc/qcom/rpmpd.c
+++ b/drivers/soc/qcom/rpmpd.c
@@ -14,6 +14,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>
@@ -29,6 +30,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 = {			\
@@ -222,6 +225,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;
@@ -259,6 +303,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 v2 4/6] 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 | 40 +++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/msm8996.dtsi b/arch/arm64/boot/dts/qcom/msm8996.dtsi
index 37b7152cb064..e1bc86b2a230 100644
--- a/arch/arm64/boot/dts/qcom/msm8996.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8996.dtsi
@@ -308,6 +308,46 @@
 				#clock-cells = <1>;
 			};
 
+			rpmpd: power-controller {
+				compatible = "qcom,msm8996-rpmpd";
+				#power-domain-cells = <1>;
+				operating-points-v2 = <&rpmpd_opp_table>, /* cx */
+						      <&rpmpd_opp_table>, /* cx_ao */
+						      <&rpmpd_opp_table>, /* cx_vfc */
+						      <&rpmpd_opp_table>, /* mx */
+						      <&rpmpd_opp_table>, /* mx_ao */
+						      <&rpmpd_opp_table>, /* sscx */
+						      <&rpmpd_opp_table>; /* sscx_vfc */
+			};
+
+			rpmpd_opp_table: opp-table {
+				compatible = "operating-points-v2-qcom-level", "operating-points-v2";
+
+				rpmpd_opp1: opp@1 {
+					qcom,level = <1>;
+				};
+
+				rpmpd_opp2: opp@2 {
+					qcom,level = <2>;
+				};
+
+				rpmpd_opp3: opp@3 {
+					qcom,level = <3>;
+				};
+
+				rpmpd_opp4: opp@4 {
+					qcom,level = <4>;
+				};
+
+				rpmpd_opp5: opp@5 {
+					qcom,level = <5>;
+				};
+
+				rpmpd_opp6: opp@6 {
+					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 v2 5/6] soc: qcom: rpmh powerdomain driver

[Rajendra Nayak]

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

We also add data for all powerdomains on sdm845 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>
---
 .../devicetree/bindings/power/qcom,rpmhpd.txt |  65 ++++
 drivers/soc/qcom/Kconfig                      |   9 +
 drivers/soc/qcom/Makefile                     |   1 +
 drivers/soc/qcom/rpmhpd.c                     | 360 ++++++++++++++++++
 4 files changed, 435 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/qcom,rpmhpd.txt
 create mode 100644 drivers/soc/qcom/rpmhpd.c

diff --git a/Documentation/devicetree/bindings/power/qcom,rpmhpd.txt b/Documentation/devicetree/bindings/power/qcom,rpmhpd.txt
new file mode 100644
index 000000000000..c1fa986c12ee
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/qcom,rpmhpd.txt
@@ -0,0 +1,65 @@
+Qualcomm RPMh Powerdomains
+
+* For RPMh powerdomains, 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 powerdomain 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>,
+				      <&rpmhpd_opp_table>,
+				      <&rpmhpd_opp_table>,
+				      <&rpmhpd_opp_table>,
+				      <&rpmhpd_opp_table>,
+				      <&rpmhpd_opp_table>,
+				      <&rpmhpd_opp_table>,
+				      <&rpmhpd_opp_table>;
+	};
+
+	rpmhpd_opp_table: opp-table {
+		compatible = "operating-points-v2-qcom-level", "operating-points-v2";
+
+		rpmhpd_opp1: opp@1 {
+			qcom-corner = <16>;
+		};
+
+		rpmhpd_opp2: opp@2 {
+			qcom-corner = <48>;
+		};
+
+		rpmhpd_opp3: opp@3 {
+			qcom-corner = <64>;
+		};
+
+		rpmhpd_opp4: opp@4 {
+			qcom-corner = <128>;
+		};
+
+		rpmhpd_opp5: opp@5 {
+			qcom-corner = <192>;
+		};
+
+		rpmhpd_opp6: opp@6 {
+			qcom-corner = <256>;
+		};
+
+		rpmhpd_opp7: opp@7 {
+			qcom-corner = <320>;
+		};
+
+		rpmhpd_opp8: opp@8 {
+			qcom-corner = <416>;
+		};
+	};
diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index a7a405178967..1faed239701d 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 Powerdomain driver"
+	depends on QCOM_RPMH && QCOM_COMMAND_DB
+	help
+	  QCOM RPMh powerdomain driver to support powerdomain 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 Powerdomain 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..a79f094ad326
--- /dev/null
+++ b/drivers/soc/qcom/rpmhpd.c
@@ -0,0 +1,360 @@
+// 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>
+
+#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",				\
+	};								\
+	static struct rpmhpd _platform##_##_active = {			\
+		.pd = { .name = #_active, },				\
+		.peer = &_platform##_##_name,				\
+		.active_only = true,					\
+		.res_name = #_name".lvl",				\
+	}
+
+#define DEFINE_RPMHPD(_platform, _name)					\
+	static struct rpmhpd _platform##_##_name = {			\
+		.pd = { .name = #_name, },				\
+		.res_name = #_name".lvl",				\
+	}
+
+/*
+ * 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 long corner;
+	u32	level[RPMH_ARC_MAX_LEVELS];
+	int	level_count;
+	bool enabled;
+	const char *res_name;
+	u32	addr;
+};
+
+struct rpmhpd_desc {
+	struct rpmhpd **rpmhpds;
+	size_t num_pds;
+};
+
+static DEFINE_MUTEX(rpmhpd_lock);
+
+/* sdm845 RPMh powerdomains */
+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[] = {
+	[0] = &sdm845_ebi,
+	[1] = &sdm845_mx,
+	[2] = &sdm845_mx_ao,
+	[3] = &sdm845_cx,
+	[4] = &sdm845_cx_ao,
+	[5] = &sdm845_lmx,
+	[6] = &sdm845_lcx,
+	[7] = &sdm845_gfx,
+	[8] = &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)
+{
+	struct tcs_cmd cmd = {
+		.addr = pd->addr,
+		.data = corner,
+	};
+
+	return rpmh_write(pd->dev, state, &cmd, 1);
+};
+
+static void to_active_sleep(struct rpmhpd *pd, unsigned long corner,
+			    unsigned long *active, unsigned long *sleep)
+{
+	*active = corner;
+
+	if (pd->active_only)
+		*sleep = 0;
+	else
+		*sleep = *active;
+}
+
+static int rpmhpd_aggregate_corner(struct rpmhpd *pd)
+{
+	int ret;
+	struct rpmhpd *peer = pd->peer;
+	unsigned long active_corner, sleep_corner;
+	unsigned long this_corner = 0, this_sleep_corner = 0;
+	unsigned long peer_corner = 0, peer_sleep_corner = 0;
+
+	to_active_sleep(pd, pd->corner, &this_corner, &this_sleep_corner);
+
+	if (peer && peer->enabled)
+		to_active_sleep(peer, peer->corner, &peer_corner,
+				&peer_sleep_corner);
+
+	active_corner = max(this_corner, peer_corner);
+
+	ret = rpmhpd_send_corner(pd, RPMH_ACTIVE_ONLY_STATE, active_corner);
+	if (ret)
+		return ret;
+
+	sleep_corner = max(this_sleep_corner, peer_sleep_corner);
+
+	return rpmhpd_send_corner(pd, RPMH_SLEEP_STATE, sleep_corner);
+}
+
+static int rpmhpd_power_on(struct generic_pm_domain *domain)
+{
+	int ret = 0;
+	struct rpmhpd *pd = domain_to_rpmhpd(domain);
+
+	mutex_lock(&rpmhpd_lock);
+
+	pd->enabled = true;
+
+	if (pd->corner)
+		ret = rpmhpd_aggregate_corner(pd);
+
+	mutex_unlock(&rpmhpd_lock);
+
+	return ret;
+}
+
+static int rpmhpd_power_off(struct generic_pm_domain *domain)
+{
+	int ret;
+	struct rpmhpd *pd = domain_to_rpmhpd(domain);
+
+	mutex_lock(&rpmhpd_lock);
+
+	if (pd->level[0] == 0) {
+		ret = rpmhpd_send_corner(pd, RPMH_ACTIVE_ONLY_STATE, 0);
+		if (ret)
+			return ret;
+
+		ret = rpmhpd_send_corner(pd, RPMH_SLEEP_STATE, 0);
+		if (ret)
+			return ret;
+	}
+
+	pd->enabled = false;
+
+	mutex_unlock(&rpmhpd_lock);
+
+	return 0;
+}
+
+static int rpmhpd_set_performance(struct generic_pm_domain *domain,
+			     unsigned int state)
+{
+	int ret = 0;
+	struct rpmhpd *pd = domain_to_rpmhpd(domain);
+
+	mutex_lock(&rpmhpd_lock);
+
+	pd->corner = state;
+
+	if (!pd->enabled)
+		goto out;
+
+	ret = rpmhpd_aggregate_corner(pd);
+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)
+{
+	u8 *buf;
+	int i, j, len, ret;
+
+	len = cmd_db_read_aux_data_len(rpmhpd->res_name);
+	if (len <= 0)
+		return len;
+
+	buf = kzalloc(len, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	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++) {
+		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]);
+	}
+
+	kfree(buf);
+	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])
+			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 RPMh Power Domain Driver");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:qcom-rpmhpd");
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v2 5/6] soc: qcom: rpmh powerdomain driver

[David Collins]

Hello Rajendra,

On 05/25/2018 03:01 AM, Rajendra Nayak wrote:
> The RPMh powerdomain driver aggregates the corner votes from various

s/powerdomain/power domain/

This applies to all instances in this patch.  "Power domain" seems to be
the preferred spelling in the kernel.


> consumers for the ARC resources and communicates it to RPMh.
> 
> We also add data for all powerdomains on sdm845 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>
> ---
>  .../devicetree/bindings/power/qcom,rpmhpd.txt |  65 ++++
>  drivers/soc/qcom/Kconfig                      |   9 +
>  drivers/soc/qcom/Makefile                     |   1 +
>  drivers/soc/qcom/rpmhpd.c                     | 360 ++++++++++++++++++
>  4 files changed, 435 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power/qcom,rpmhpd.txt
>  create mode 100644 drivers/soc/qcom/rpmhpd.c
...
> +++ b/Documentation/devicetree/bindings/power/qcom,rpmhpd.txt

I think that this binding documentation should be in a patch separate from
the driver.


> @@ -0,0 +1,65 @@
> +Qualcomm RPMh Powerdomains

s/Qualcomm/Qualcomm Technologies, Inc./


> +
> +* For RPMh powerdomains, we communicate a performance state to RPMh

Does this line need to start with '*'?


> +which then translates it into a corresponding voltage on a rail
> +
> +Required Properties:
> + - compatible: Should be one of the following
> +	* qcom,sdm845-rpmhpd: RPMh powerdomain 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>,
> +				      <&rpmhpd_opp_table>,
> +				      <&rpmhpd_opp_table>,
> +				      <&rpmhpd_opp_table>,
> +				      <&rpmhpd_opp_table>,
> +				      <&rpmhpd_opp_table>,
> +				      <&rpmhpd_opp_table>,
> +				      <&rpmhpd_opp_table>;

Can this be changed to simply:
  operating-points-v2 = <&rpmhpd_opp_table>;

The opp binding documentation [1] states that this should be ok:

    If only one phandle is available, then the same OPP table will be used
    for all power domains provided by the power domain provider.


> +	};
> +
> +	rpmhpd_opp_table: opp-table {
> +		compatible = "operating-points-v2-qcom-level", "operating-points-v2";
> +
> +		rpmhpd_opp1: opp@1 {

Is there any significance to the 1 through 8 values in these OPP table
nodes?  If not, then could this be changed to something like:

rpmhpd_retention: opp@16 {
...
rpmhpd_turbo_l1: opp@416 {


> +			qcom-corner = <16>;

s/qcom-corner/qcom,level/g


> +		};
> +
> +		rpmhpd_opp2: opp@2 {
> +			qcom-corner = <48>;
> +		};
> +
> +		rpmhpd_opp3: opp@3 {
> +			qcom-corner = <64>;
> +		};
> +
> +		rpmhpd_opp4: opp@4 {
> +			qcom-corner = <128>;
> +		};
> +
> +		rpmhpd_opp5: opp@5 {
> +			qcom-corner = <192>;
> +		};
> +
> +		rpmhpd_opp6: opp@6 {
> +			qcom-corner = <256>;
> +		};
> +
> +		rpmhpd_opp7: opp@7 {
> +			qcom-corner = <320>;
> +		};

Can you please add 336 and 384 to your example?  384 at least should be
present as it corresponds to the Turbo level which all supplies support.


> +		rpmhpd_opp8: opp@8 {
> +			qcom-corner = <416>;
> +		};
> +	};

How are consumers of these power domains supposed to identify which domain
within <&rpmhpd> to use (e.g. VDD_CX vs VDD_MX)?  If the answer is a plain
integer index, then could you please add per-platform #define constants in
a DT header file which explicitly define the meaning for each index?

How do consumers of these power domains identify which level they want to
set for a specific power domain (e.g. Nominal vs Turbo)?

Would it be helpful to provide a DT header file with #define constants for
the cross-platform sparse level mapping?  This is done in [2] for the
downstream rpmh-regulator driver that handles ARC managed regulators.


Would it be ok to add some consumer DT nodes in this binding file example
so that it is clear how consumers interact with the rpmhpd?


> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> index a7a405178967..1faed239701d 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 Powerdomain driver"

s/Qualcomm/Qualcomm Technologies, Inc./


> +	depends on QCOM_RPMH && QCOM_COMMAND_DB
> +	help
> +	  QCOM RPMh powerdomain driver to support powerdomain 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 Powerdomain 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..a79f094ad326
> --- /dev/null
> +++ b/drivers/soc/qcom/rpmhpd.c
> @@ -0,0 +1,360 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
> + */

Minor: The copyright comment could be made into a single line comment.


> +
> +#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>
> +
> +#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",				\
> +	};								\
> +	static struct rpmhpd _platform##_##_active = {			\
> +		.pd = { .name = #_active, },				\
> +		.peer = &_platform##_##_name,				\
> +		.active_only = true,					\
> +		.res_name = #_name".lvl",				\
> +	}
> +
> +#define DEFINE_RPMHPD(_platform, _name)					\
> +	static struct rpmhpd _platform##_##_name = {			\
> +		.pd = { .name = #_name, },				\
> +		.res_name = #_name".lvl",				\
> +	}
> +
> +/*
> + * 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 long corner;

Does this actually need to be unsigned long?  It looks like unsigned int
is being passed in from rpmhpd_set_performance().  Also, hlvl values sent
to RPMh will only every be in the range 0 - 15.

If you change the type here, then can you also please change long to int
in to_active_sleep() and rpmhpd_aggregate_corner() below?


> +	u32	level[RPMH_ARC_MAX_LEVELS];
> +	int	level_count;
> +	bool enabled;
> +	const char *res_name;
> +	u32	addr;
> +};

Can you please indent the fields of this struct to the same column with tabs?


> +
> +struct rpmhpd_desc {
> +	struct rpmhpd **rpmhpds;
> +	size_t num_pds;
> +};

This struct could be removed and the per-platform arrays could instead be
NULL terminated.

> +
> +static DEFINE_MUTEX(rpmhpd_lock);
> +
> +/* sdm845 RPMh powerdomains */
> +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[] = {
> +	[0] = &sdm845_ebi,

If you are going to explicitly index these elements, then can you please
use #define constants from a DT header file that specifies meaningful
names?  The existing 0-8 indexing is no better than implicit indexing.


> +	[1] = &sdm845_mx,
> +	[2] = &sdm845_mx_ao,
> +	[3] = &sdm845_cx,
> +	[4] = &sdm845_cx_ao,
> +	[5] = &sdm845_lmx,
> +	[6] = &sdm845_lcx,
> +	[7] = &sdm845_gfx,
> +	[8] = &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)
> +{
> +	struct tcs_cmd cmd = {
> +		.addr = pd->addr,
> +		.data = corner,
> +	};
> +
> +	return rpmh_write(pd->dev, state, &cmd, 1);

This can be optimized by calling rpmh_write_async() whenever the corner
being sent is smaller than the last value sent.  That way, no time is
wasted waiting for an ACK when decreasing voltage.  Would you mind adding
the necessary check and previous request caching for this?


> +};
> +
> +static void to_active_sleep(struct rpmhpd *pd, unsigned long corner,
> +			    unsigned long *active, unsigned long *sleep)
> +{
> +	*active = corner;
> +
> +	if (pd->active_only)
> +		*sleep = 0;
> +	else
> +		*sleep = *active;
> +}
> +
> +static int rpmhpd_aggregate_corner(struct rpmhpd *pd)
> +{
> +	int ret;
> +	struct rpmhpd *peer = pd->peer;
> +	unsigned long active_corner, sleep_corner;
> +	unsigned long this_corner = 0, this_sleep_corner = 0;
> +	unsigned long peer_corner = 0, peer_sleep_corner = 0;

s/this_corner/this_active_corner/
s/peer_corner/peer_active_corner/

This is more consistent and I think that it makes the code a little more
readable.


> +
> +	to_active_sleep(pd, pd->corner, &this_corner, &this_sleep_corner);
> +
> +	if (peer && peer->enabled)
> +		to_active_sleep(peer, peer->corner, &peer_corner,
> +				&peer_sleep_corner);
> +
> +	active_corner = max(this_corner, peer_corner);
> +
> +	ret = rpmhpd_send_corner(pd, RPMH_ACTIVE_ONLY_STATE, active_corner);
> +	if (ret)
> +		return ret;
> +
> +	sleep_corner = max(this_sleep_corner, peer_sleep_corner);
> +
> +	return rpmhpd_send_corner(pd, RPMH_SLEEP_STATE, sleep_corner);
> +}

This aggregation function as well as the rpmhpd_send_corner() calls below
are not sufficient for RPMh.  There are 3 states that must all be used:
RPMH_ACTIVE_ONLY_STATE, RPMH_WAKE_ONLY_STATE, and RPMH_SLEEP_STATE.  The
naming is somewhat confusing as rpmhpd is defining a different concept of
active-only.

For power domains without active-only or peers, only
RPMH_ACTIVE_ONLY_STATE should be used.  This instructs RPMh to issue the
request immediately.

For power domains with active-only, requests will need to be made for all
three.  active_corner would be sent for both RPMH_ACTIVE_ONLY_STATE (so
that the request takes effect immediately) and RPMH_WAKE_ONLY_STATE (so
that the request is inserted into the wake TCS).  sleep_corner would be
sent for RPMH_SLEEP_STATE (so that the request is inserted into the sleep
TCS).

You can see how this is handled in the RPMh clock driver in patch [3].

You may be able to get away with using only RPMH_SLEEP_STATE and
RPMH_ACTIVE_ONLY_STATE assuming that you issue the RPMH_SLEEP_STATE
request first due to the rpmh driver caching behavior added in the
cache_rpm_request() function in [4].  However, could you please confirm
with Lina that this usage will continue to work in the future?  I'm not
sure what guarantees are made at the rpmh API level.


> +
> +static int rpmhpd_power_on(struct generic_pm_domain *domain)
> +{
> +	int ret = 0;
> +	struct rpmhpd *pd = domain_to_rpmhpd(domain);

Minor: It might look a little nicer to list 'pd' definition first amongst
the local variables in this function as well as those below.


> +
> +	mutex_lock(&rpmhpd_lock);
> +
> +	pd->enabled = true;
> +
> +	if (pd->corner)
> +		ret = rpmhpd_aggregate_corner(pd);
> +
> +	mutex_unlock(&rpmhpd_lock);
> +
> +	return ret;
> +}
> +
> +static int rpmhpd_power_off(struct generic_pm_domain *domain)
> +{
> +	int ret;
> +	struct rpmhpd *pd = domain_to_rpmhpd(domain);
> +
> +	mutex_lock(&rpmhpd_lock);
> +
> +	if (pd->level[0] == 0) {
> +		ret = rpmhpd_send_corner(pd, RPMH_ACTIVE_ONLY_STATE, 0);
> +		if (ret)
> +			return ret;
> +
> +		ret = rpmhpd_send_corner(pd, RPMH_SLEEP_STATE, 0);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	pd->enabled = false;
> +
> +	mutex_unlock(&rpmhpd_lock);
> +
> +	return 0;
> +}
> +
> +static int rpmhpd_set_performance(struct generic_pm_domain *domain,
> +			     unsigned int state)
> +{
> +	int ret = 0;
> +	struct rpmhpd *pd = domain_to_rpmhpd(domain);
> +
> +	mutex_lock(&rpmhpd_lock);
> +
> +	pd->corner = state;

What is the numbering space for 'state'?  I assume that it is a vlvl value
corresponding to qcom,level in a DT OPP table node.  If so, additional
logic is required.

When using RPMh, the platform and supply independent vlvl sparse numbering
space is used by consumers so that they can always have consistent values.
 However, the actual requests sent to RPMh ARC must be in the hlvl
numbering space (i.e. 0 - 15(max)).  In the case of this driver, the
acceptable hlvl values for a given power domain are 0 to pd->level_count - 1.

I suspect that you need to add something like this here:

int i;

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;


Note that a given power domain will likely not support all of the vlvl
values listed in the DT OPP table nodes.


> +
> +	if (!pd->enabled)
> +		goto out;
> +
> +	ret = rpmhpd_aggregate_corner(pd);
> +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;

Why return 0 instead of an error?


> +	}
> +
> +	of_node_put(np);
> +
> +	return corner;
> +}

Is there an API to determine the currently operating performance state of
a given power domain?  Is this information accessible from userspace?  We
will definitely need this for general debugging.


> +
> +static int rpmhpd_update_level_mapping(struct rpmhpd *rpmhpd)
> +{
> +	u8 *buf;

This could be changed to the following in order to remove the need for
kzalloc() and kfree() calls below:

u8 buf[RPMH_ARC_MAX_LEVELS * RPMH_ARC_LEVEL_SIZE];


> +	int i, j, len, ret;
> +
> +	len = cmd_db_read_aux_data_len(rpmhpd->res_name);
> +	if (len <= 0)
> +		return len;
> +

A check like this is needed here:

if (len > RPMH_ARC_MAX_LEVELS * RPMH_ARC_LEVEL_SIZE)
	return -EINVAL;


> +	buf = kzalloc(len, GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	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++) {

If you make buf a fixed size array, then rpmhpd->level[i] = 0; is needed
here or a memset() outside of the for loop.


> +		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]);
> +	}
> +
> +	kfree(buf);
> +	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])
> +			continue;

Why is this check needed?


> +
> +		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 RPMh Power Domain Driver");

s/Qualcomm/Qualcomm Technologies, Inc./


> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:qcom-rpmhpd");

Thanks,
David

[1]:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/opp/opp.txt?h=v4.17-rc6#n49
[2]:
https://source.codeaurora.org/quic/la/kernel/msm-4.9/tree/include/dt-bindings/regulator/qcom,rpmh-regulator.h?h=msm-4.9
[3]: https://lkml.org/lkml/2018/5/9/54
[4]: https://lkml.org/lkml/2018/5/24/536

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

Re: [PATCH v2 5/6] soc: qcom: rpmh powerdomain driver

[Rajendra Nayak]

Hi David,

On 05/26/2018 06:38 AM, David Collins wrote:
> Hello Rajendra,
> 
> On 05/25/2018 03:01 AM, Rajendra Nayak wrote:
>> The RPMh powerdomain driver aggregates the corner votes from various
> 
> s/powerdomain/power domain/
> 
> This applies to all instances in this patch.  "Power domain" seems to be
> the preferred spelling in the kernel.

thanks, will change in all applicable places.

> 
> 
>> consumers for the ARC resources and communicates it to RPMh.
>>
>> We also add data for all powerdomains on sdm845 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>
>> ---
>>  .../devicetree/bindings/power/qcom,rpmhpd.txt |  65 ++++
>>  drivers/soc/qcom/Kconfig                      |   9 +
>>  drivers/soc/qcom/Makefile                     |   1 +
>>  drivers/soc/qcom/rpmhpd.c                     | 360 ++++++++++++++++++
>>  4 files changed, 435 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/power/qcom,rpmhpd.txt
>>  create mode 100644 drivers/soc/qcom/rpmhpd.c
> ...
>> +++ b/Documentation/devicetree/bindings/power/qcom,rpmhpd.txt
> 
> I think that this binding documentation should be in a patch separate from
> the driver.

yes, will split it when I repost

> 
> 
>> @@ -0,0 +1,65 @@
>> +Qualcomm RPMh Powerdomains
> 
> s/Qualcomm/Qualcomm Technologies, Inc./
> 
> 
>> +
>> +* For RPMh powerdomains, we communicate a performance state to RPMh
> 
> Does this line need to start with '*'?

No, will remove it

> 
> 
>> +which then translates it into a corresponding voltage on a rail
>> +
>> +Required Properties:
>> + - compatible: Should be one of the following
>> +	* qcom,sdm845-rpmhpd: RPMh powerdomain 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>,
>> +				      <&rpmhpd_opp_table>,
>> +				      <&rpmhpd_opp_table>,
>> +				      <&rpmhpd_opp_table>,
>> +				      <&rpmhpd_opp_table>,
>> +				      <&rpmhpd_opp_table>,
>> +				      <&rpmhpd_opp_table>,
>> +				      <&rpmhpd_opp_table>;
> 
> Can this be changed to simply:
>   operating-points-v2 = <&rpmhpd_opp_table>;
> 
> The opp binding documentation [1] states that this should be ok:
> 
>     If only one phandle is available, then the same OPP table will be used
>     for all power domains provided by the power domain provider.

thanks, I mentioned this to Viresh but didn't realize he fixed it up.
Will remove the redundant entries.

> 
> 
>> +	};
>> +
>> +	rpmhpd_opp_table: opp-table {
>> +		compatible = "operating-points-v2-qcom-level", "operating-points-v2";
>> +
>> +		rpmhpd_opp1: opp@1 {
> 
> Is there any significance to the 1 through 8 values in these OPP table
> nodes?  If not, then could this be changed to something like:
> 
> rpmhpd_retention: opp@16 {
> ...
> rpmhpd_turbo_l1: opp@416 {
> 
> 
>> +			qcom-corner = <16>;
> 
> s/qcom-corner/qcom,level/g

Thanks, I'll fix these up.
> 
> 
>> +		};
>> +
>> +		rpmhpd_opp2: opp@2 {
>> +			qcom-corner = <48>;
>> +		};
>> +
>> +		rpmhpd_opp3: opp@3 {
>> +			qcom-corner = <64>;
>> +		};
>> +
>> +		rpmhpd_opp4: opp@4 {
>> +			qcom-corner = <128>;
>> +		};
>> +
>> +		rpmhpd_opp5: opp@5 {
>> +			qcom-corner = <192>;
>> +		};
>> +
>> +		rpmhpd_opp6: opp@6 {
>> +			qcom-corner = <256>;
>> +		};
>> +
>> +		rpmhpd_opp7: opp@7 {
>> +			qcom-corner = <320>;
>> +		};
> 
> Can you please add 336 and 384 to your example?  384 at least should be
> present as it corresponds to the Turbo level which all supplies support.

will do.

> 
> 
>> +		rpmhpd_opp8: opp@8 {
>> +			qcom-corner = <416>;
>> +		};
>> +	};
> 
> How are consumers of these power domains supposed to identify which domain
> within <&rpmhpd> to use (e.g. VDD_CX vs VDD_MX)?  If the answer is a plain
> integer index, then could you please add per-platform #define constants in
> a DT header file which explicitly define the meaning for each index?
> 
> How do consumers of these power domains identify which level they want to
> set for a specific power domain (e.g. Nominal vs Turbo)?
> 
> Would it be helpful to provide a DT header file with #define constants for
> the cross-platform sparse level mapping?  This is done in [2] for the
> downstream rpmh-regulator driver that handles ARC managed regulators.
> 
> 
> Would it be ok to add some consumer DT nodes in this binding file example
> so that it is clear how consumers interact with the rpmhpd?

yes, I'll add the header for indexes and performance levels.

> 
> 
>> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
>> index a7a405178967..1faed239701d 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 Powerdomain driver"
> 
> s/Qualcomm/Qualcomm Technologies, Inc./

All other config options in qcom/Kconfig use 'Qualcomm XYZ feature'
for the comment. Maybe I will leave it that way for consistency?

[]...

>> +
>> +struct rpmhpd {
>> +	struct device *dev;
>> +	struct generic_pm_domain pd;
>> +	struct rpmhpd *peer;
>> +	const bool active_only;
>> +	unsigned long corner;
> 
> Does this actually need to be unsigned long?  It looks like unsigned int
> is being passed in from rpmhpd_set_performance().  Also, hlvl values sent
> to RPMh will only every be in the range 0 - 15.
> 
> If you change the type here, then can you also please change long to int
> in to_active_sleep() and rpmhpd_aggregate_corner() below?

yes, there's no need for an unsigned long, will change it

> 
> 
>> +	u32	level[RPMH_ARC_MAX_LEVELS];
>> +	int	level_count;
>> +	bool enabled;
>> +	const char *res_name;
>> +	u32	addr;
>> +};
> 
> Can you please indent the fields of this struct to the same column with tabs?

will do.

> 
> 
>> +
>> +struct rpmhpd_desc {
>> +	struct rpmhpd **rpmhpds;
>> +	size_t num_pds;
>> +};
> 
> This struct could be removed and the per-platform arrays could instead be
> NULL terminated.

Yes, but I would prefer it this way unless you have strong objections.
Just makes it easier to do the allocations at probe for genpd_onecell_data structures.

>> +
>> +static DEFINE_MUTEX(rpmhpd_lock);
>> +
>> +/* sdm845 RPMh powerdomains */
>> +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[] = {
>> +	[0] = &sdm845_ebi,
> 
> If you are going to explicitly index these elements, then can you please
> use #define constants from a DT header file that specifies meaningful
> names?  The existing 0-8 indexing is no better than implicit indexing.

yes, I will use the macros from the header.

> 
> 
>> +	[1] = &sdm845_mx,
>> +	[2] = &sdm845_mx_ao,
>> +	[3] = &sdm845_cx,
>> +	[4] = &sdm845_cx_ao,
>> +	[5] = &sdm845_lmx,
>> +	[6] = &sdm845_lcx,
>> +	[7] = &sdm845_gfx,
>> +	[8] = &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)
>> +{
>> +	struct tcs_cmd cmd = {
>> +		.addr = pd->addr,
>> +		.data = corner,
>> +	};
>> +
>> +	return rpmh_write(pd->dev, state, &cmd, 1);
> 
> This can be optimized by calling rpmh_write_async() whenever the corner
> being sent is smaller than the last value sent.  That way, no time is
> wasted waiting for an ACK when decreasing voltage.  Would you mind adding
> the necessary check and previous request caching for this?

thanks, looks useful, I will do the necessary changes.

> 
> 
>> +};
>> +
>> +static void to_active_sleep(struct rpmhpd *pd, unsigned long corner,
>> +			    unsigned long *active, unsigned long *sleep)
>> +{
>> +	*active = corner;
>> +
>> +	if (pd->active_only)
>> +		*sleep = 0;
>> +	else
>> +		*sleep = *active;
>> +}
>> +
>> +static int rpmhpd_aggregate_corner(struct rpmhpd *pd)
>> +{
>> +	int ret;
>> +	struct rpmhpd *peer = pd->peer;
>> +	unsigned long active_corner, sleep_corner;
>> +	unsigned long this_corner = 0, this_sleep_corner = 0;
>> +	unsigned long peer_corner = 0, peer_sleep_corner = 0;
> 
> s/this_corner/this_active_corner/
> s/peer_corner/peer_active_corner/
> 
> This is more consistent and I think that it makes the code a little more
> readable.

sure

> 
> 
>> +
>> +	to_active_sleep(pd, pd->corner, &this_corner, &this_sleep_corner);
>> +
>> +	if (peer && peer->enabled)
>> +		to_active_sleep(peer, peer->corner, &peer_corner,
>> +				&peer_sleep_corner);
>> +
>> +	active_corner = max(this_corner, peer_corner);
>> +
>> +	ret = rpmhpd_send_corner(pd, RPMH_ACTIVE_ONLY_STATE, active_corner);
>> +	if (ret)
>> +		return ret;
>> +
>> +	sleep_corner = max(this_sleep_corner, peer_sleep_corner);
>> +
>> +	return rpmhpd_send_corner(pd, RPMH_SLEEP_STATE, sleep_corner);
>> +}
> 
> This aggregation function as well as the rpmhpd_send_corner() calls below
> are not sufficient for RPMh.  There are 3 states that must all be used:
> RPMH_ACTIVE_ONLY_STATE, RPMH_WAKE_ONLY_STATE, and RPMH_SLEEP_STATE.  The
> naming is somewhat confusing as rpmhpd is defining a different concept of
> active-only.
> 
> For power domains without active-only or peers, only
> RPMH_ACTIVE_ONLY_STATE should be used.  This instructs RPMh to issue the
> request immediately.
> 
> For power domains with active-only, requests will need to be made for all
> three.  active_corner would be sent for both RPMH_ACTIVE_ONLY_STATE (so
> that the request takes effect immediately) and RPMH_WAKE_ONLY_STATE (so
> that the request is inserted into the wake TCS).  sleep_corner would be
> sent for RPMH_SLEEP_STATE (so that the request is inserted into the sleep
> TCS).
> 
> You can see how this is handled in the RPMh clock driver in patch [3].

Thanks, I'll take another look at this and fix this up

> 
> You may be able to get away with using only RPMH_SLEEP_STATE and
> RPMH_ACTIVE_ONLY_STATE assuming that you issue the RPMH_SLEEP_STATE
> request first due to the rpmh driver caching behavior added in the
> cache_rpm_request() function in [4].  However, could you please confirm
> with Lina that this usage will continue to work in the future?  I'm not
> sure what guarantees are made at the rpmh API level.

sure, I'll check it up

> 
> 
>> +
>> +static int rpmhpd_power_on(struct generic_pm_domain *domain)
>> +{
>> +	int ret = 0;
>> +	struct rpmhpd *pd = domain_to_rpmhpd(domain);
> 
> Minor: It might look a little nicer to list 'pd' definition first amongst
> the local variables in this function as well as those below.

okay

> 
> 
>> +
>> +	mutex_lock(&rpmhpd_lock);
>> +
>> +	pd->enabled = true;
>> +
>> +	if (pd->corner)
>> +		ret = rpmhpd_aggregate_corner(pd);
>> +
>> +	mutex_unlock(&rpmhpd_lock);
>> +
>> +	return ret;
>> +}
>> +
>> +static int rpmhpd_power_off(struct generic_pm_domain *domain)
>> +{
>> +	int ret;
>> +	struct rpmhpd *pd = domain_to_rpmhpd(domain);
>> +
>> +	mutex_lock(&rpmhpd_lock);
>> +
>> +	if (pd->level[0] == 0) {
>> +		ret = rpmhpd_send_corner(pd, RPMH_ACTIVE_ONLY_STATE, 0);
>> +		if (ret)
>> +			return ret;
>> +
>> +		ret = rpmhpd_send_corner(pd, RPMH_SLEEP_STATE, 0);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	pd->enabled = false;
>> +
>> +	mutex_unlock(&rpmhpd_lock);
>> +
>> +	return 0;
>> +}
>> +
>> +static int rpmhpd_set_performance(struct generic_pm_domain *domain,
>> +			     unsigned int state)
>> +{
>> +	int ret = 0;
>> +	struct rpmhpd *pd = domain_to_rpmhpd(domain);
>> +
>> +	mutex_lock(&rpmhpd_lock);
>> +
>> +	pd->corner = state;
> 
> What is the numbering space for 'state'?  I assume that it is a vlvl value
> corresponding to qcom,level in a DT OPP table node.  If so, additional
> logic is required.
> 
> When using RPMh, the platform and supply independent vlvl sparse numbering
> space is used by consumers so that they can always have consistent values.
>  However, the actual requests sent to RPMh ARC must be in the hlvl
> numbering space (i.e. 0 - 15(max)).  In the case of this driver, the
> acceptable hlvl values for a given power domain are 0 to pd->level_count - 1.
> 
> I suspect that you need to add something like this here:
> 
> int i;
> 
> 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;
> 
> 
> Note that a given power domain will likely not support all of the vlvl
> values listed in the DT OPP table nodes.

yes indeed :/ I will add the translation bits here for vlvl to hlvl which is
needed before communicating with RPMh

> 
> 
>> +
>> +	if (!pd->enabled)
>> +		goto out;
>> +
>> +	ret = rpmhpd_aggregate_corner(pd);
>> +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;
> 
> Why return 0 instead of an error?

The caller expects a 0 for failure

> 
> 
>> +	}
>> +
>> +	of_node_put(np);
>> +
>> +	return corner;
>> +}
> 
> Is there an API to determine the currently operating performance state of
> a given power domain?  Is this information accessible from userspace?  We
> will definitely need this for general debugging.

A quick look shows me its not. I agree its a necessary feature for debug.
I will add a patch to expose it via debugfs

> 
> 
>> +
>> +static int rpmhpd_update_level_mapping(struct rpmhpd *rpmhpd)
>> +{
>> +	u8 *buf;
> 
> This could be changed to the following in order to remove the need for
> kzalloc() and kfree() calls below:
> 
> u8 buf[RPMH_ARC_MAX_LEVELS * RPMH_ARC_LEVEL_SIZE];
> 
> 
>> +	int i, j, len, ret;
>> +
>> +	len = cmd_db_read_aux_data_len(rpmhpd->res_name);
>> +	if (len <= 0)
>> +		return len;
>> +
> 
> A check like this is needed here:
> 
> if (len > RPMH_ARC_MAX_LEVELS * RPMH_ARC_LEVEL_SIZE)
> 	return -EINVAL;
> 
> 
>> +	buf = kzalloc(len, GFP_KERNEL);
>> +	if (!buf)
>> +		return -ENOMEM;
>> +
>> +	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++) {
> 
> If you make buf a fixed size array, then rpmhpd->level[i] = 0; is needed
> here or a memset() outside of the for loop.

Okay, I;ll move buf to a fixed size array instead

> 
> 
>> +		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]);
>> +	}
>> +
>> +	kfree(buf);
>> +	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])
>> +			continue;
> 
> Why is this check needed?

Just to check/ignore if there are any holes.
maybe I should atleast throw a warning instead of silently ignoring it?

> 
> 
>> +
>> +		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 RPMh Power Domain Driver");
> 
> s/Qualcomm/Qualcomm Technologies, Inc./

will do

> 
> 
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_ALIAS("platform:qcom-rpmhpd");
> 
> Thanks,
> David

Thanks David for taking time to review.

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

Re: [PATCH v2 5/6] soc: qcom: rpmh powerdomain driver

[David Collins]

Hello Rajendra,

On 05/29/2018 03:19 AM, Rajendra Nayak wrote:
> On 05/26/2018 06:38 AM, David Collins wrote:
>> On 05/25/2018 03:01 AM, Rajendra Nayak wrote:
>>> The RPMh powerdomain driver aggregates the corner votes from various
...
>>> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
>>> index a7a405178967..1faed239701d 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 Powerdomain driver"
>>
>> s/Qualcomm/Qualcomm Technologies, Inc./
> 
> All other config options in qcom/Kconfig use 'Qualcomm XYZ feature'
> for the comment. Maybe I will leave it that way for consistency?

I don't have a strong opinion about it.  I just want the legal folks to be
happy.  I'm fine with whatever achieves that goal.


>>> +
>>> +struct rpmhpd_desc {
>>> +	struct rpmhpd **rpmhpds;
>>> +	size_t num_pds;
>>> +};
>>
>> This struct could be removed and the per-platform arrays could instead be
>> NULL terminated.
> 
> Yes, but I would prefer it this way unless you have strong objections.
> Just makes it easier to do the allocations at probe for genpd_onecell_data structures.

I'm fine if you keep it as-is.  I mentioned the alternative because
Stephen had requested the same modification on my qcom-rpmh-regulator
driver patch [1].  Other reviewers may care about this point.


>> Is there an API to determine the currently operating performance state of
>> a given power domain?  Is this information accessible from userspace?  We
>> will definitely need this for general debugging.
> 
> A quick look shows me its not. I agree its a necessary feature for debug.
> I will add a patch to expose it via debugfs

Thanks


>>> +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])
>>> +			continue;
>>
>> Why is this check needed?
> 
> Just to check/ignore if there are any holes.
> maybe I should atleast throw a warning instead of silently ignoring it?

A warning message might be a good idea if this condition should ever be
reached but also doesn't necessarily imply that probing must be ceased.
It looks like of_genpd_add_provider_onecell() ignores the NULL initialized
data->domains[i] values so it should be safe to leave the holes in and not
decrement num_domains accordingly.

Take care,
David

[1]: https://lkml.org/lkml/2018/3/21/681

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

Re: [PATCH v2 5/6] soc: qcom: rpmh powerdomain driver

[Rajendra Nayak]

[]...

>>> +Required Properties:
>>> + - compatible: Should be one of the following
>>> +	* qcom,sdm845-rpmhpd: RPMh powerdomain 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>,
>>> +				      <&rpmhpd_opp_table>,
>>> +				      <&rpmhpd_opp_table>,
>>> +				      <&rpmhpd_opp_table>,
>>> +				      <&rpmhpd_opp_table>,
>>> +				      <&rpmhpd_opp_table>,
>>> +				      <&rpmhpd_opp_table>,
>>> +				      <&rpmhpd_opp_table>;
>>
>> Can this be changed to simply:
>>   operating-points-v2 = <&rpmhpd_opp_table>;
>>
>> The opp binding documentation [1] states that this should be ok:
>>
>>     If only one phandle is available, then the same OPP table will be used
>>     for all power domains provided by the power domain provider.
> 
> thanks, I mentioned this to Viresh but didn't realize he fixed it up.
> Will remove the redundant entries.

Looks like the kernel implementation does not handle this yet, and I get
an error adding the OPP tables for the powerdomains if I just specify
a single OPP table phandle.

Viresh, is this expected with the latest patches in linux-next?

It would be good if I can specify just one phandle instead of coping
the same phandle n times. 

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

Re: [PATCH v2 5/6] soc: qcom: rpmh powerdomain driver

[Viresh Kumar]

On 30-05-18, 14:25, Rajendra Nayak wrote:
> []...
> 
> >>> +Required Properties:
> >>> + - compatible: Should be one of the following
> >>> +	* qcom,sdm845-rpmhpd: RPMh powerdomain 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>,
> >>> +				      <&rpmhpd_opp_table>,
> >>> +				      <&rpmhpd_opp_table>,
> >>> +				      <&rpmhpd_opp_table>,
> >>> +				      <&rpmhpd_opp_table>,
> >>> +				      <&rpmhpd_opp_table>,
> >>> +				      <&rpmhpd_opp_table>,
> >>> +				      <&rpmhpd_opp_table>;
> >>
> >> Can this be changed to simply:
> >>   operating-points-v2 = <&rpmhpd_opp_table>;
> >>
> >> The opp binding documentation [1] states that this should be ok:
> >>
> >>     If only one phandle is available, then the same OPP table will be used
> >>     for all power domains provided by the power domain provider.
> > 
> > thanks, I mentioned this to Viresh but didn't realize he fixed it up.
> > Will remove the redundant entries.
> 
> Looks like the kernel implementation does not handle this yet, and I get
> an error adding the OPP tables for the powerdomains if I just specify
> a single OPP table phandle.
> 
> Viresh, is this expected with the latest patches in linux-next?
> 
> It would be good if I can specify just one phandle instead of coping
> the same phandle n times. 

Please try this untested hunk:

diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index 6d15f05bfc28..7af0ddec936b 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -554,11 +554,24 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_of_add_table);
 int dev_pm_opp_of_add_table_indexed(struct device *dev, int index)
 {
        struct device_node *opp_np;
-       int ret;
+       int ret, count;
 
+again:
        opp_np = _opp_of_get_opp_desc_node(dev->of_node, index);
-       if (!opp_np)
+       if (!opp_np) {
+               /*
+                * If only one phandle is present, then the same OPP table
+                * applies for all index requests.
+                */
+               count = of_count_phandle_with_args(dev->of_node,
+                                                  "operating-points-v2", NULL);
+               if (count == 1 && index) {
+                       index = 0;
+                       goto again;
+               }
+
                return -ENODEV;
+       }
 
        ret = _of_add_opp_table_v2(dev, opp_np);
        of_node_put(opp_np);

Re: [PATCH v2 5/6] soc: qcom: rpmh powerdomain driver

[Rajendra Nayak]

On 05/30/2018 03:14 PM, Viresh Kumar wrote:
> On 30-05-18, 14:25, Rajendra Nayak wrote:
>> []...
>>
>>>>> +Required Properties:
>>>>> + - compatible: Should be one of the following
>>>>> +	* qcom,sdm845-rpmhpd: RPMh powerdomain 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>,
>>>>> +				      <&rpmhpd_opp_table>,
>>>>> +				      <&rpmhpd_opp_table>,
>>>>> +				      <&rpmhpd_opp_table>,
>>>>> +				      <&rpmhpd_opp_table>,
>>>>> +				      <&rpmhpd_opp_table>,
>>>>> +				      <&rpmhpd_opp_table>,
>>>>> +				      <&rpmhpd_opp_table>;
>>>>
>>>> Can this be changed to simply:
>>>>   operating-points-v2 = <&rpmhpd_opp_table>;
>>>>
>>>> The opp binding documentation [1] states that this should be ok:
>>>>
>>>>     If only one phandle is available, then the same OPP table will be used
>>>>     for all power domains provided by the power domain provider.
>>>
>>> thanks, I mentioned this to Viresh but didn't realize he fixed it up.
>>> Will remove the redundant entries.
>>
>> Looks like the kernel implementation does not handle this yet, and I get
>> an error adding the OPP tables for the powerdomains if I just specify
>> a single OPP table phandle.
>>
>> Viresh, is this expected with the latest patches in linux-next?
>>
>> It would be good if I can specify just one phandle instead of coping
>> the same phandle n times. 
> 
> Please try this untested hunk:

yes, seems to work.

> diff --git a/drivers/opp/of.c b/drivers/opp/of.c
> index 6d15f05bfc28..7af0ddec936b 100644
> --- a/drivers/opp/of.c
> +++ b/drivers/opp/of.c
> @@ -554,11 +554,24 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_of_add_table);
>  int dev_pm_opp_of_add_table_indexed(struct device *dev, int index)
>  {
>         struct device_node *opp_np;
> -       int ret;
> +       int ret, count;
>  
> +again:
>         opp_np = _opp_of_get_opp_desc_node(dev->of_node, index);
> -       if (!opp_np)
> +       if (!opp_np) {
> +               /*
> +                * If only one phandle is present, then the same OPP table
> +                * applies for all index requests.
> +                */
> +               count = of_count_phandle_with_args(dev->of_node,
> +                                                  "operating-points-v2", NULL);
> +               if (count == 1 && index) {
> +                       index = 0;
> +                       goto again;
> +               }
> +
>                 return -ENODEV;
> +       }
>  
>         ret = _of_add_opp_table_v2(dev, opp_np);
>         of_node_put(opp_np);
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" 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 v2 5/6] soc: qcom: rpmh powerdomain driver

[Rajendra Nayak]

Hi David,

On 05/26/2018 06:38 AM, David Collins wrote:
> 
>> +	[1] = &sdm845_mx,
>> +	[2] = &sdm845_mx_ao,
>> +	[3] = &sdm845_cx,
>> +	[4] = &sdm845_cx_ao,
>> +	[5] = &sdm845_lmx,
>> +	[6] = &sdm845_lcx,
>> +	[7] = &sdm845_gfx,
>> +	[8] = &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)
>> +{
>> +	struct tcs_cmd cmd = {
>> +		.addr = pd->addr,
>> +		.data = corner,
>> +	};
>> +
>> +	return rpmh_write(pd->dev, state, &cmd, 1);
> This can be optimized by calling rpmh_write_async() whenever the corner
> being sent is smaller than the last value sent.  That way, no time is
> wasted waiting for an ACK when decreasing voltage.  Would you mind adding
> the necessary check and previous request caching for this?

is it safe to assume all sleep votes can be sent as async always? and only
active votes could be sync/async depending on the last value sent?

regards,
Rajendra

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

Re: [PATCH v2 5/6] soc: qcom: rpmh powerdomain driver

[David Collins]

Hello Rajendra,

On 06/01/2018 01:48 AM, Rajendra Nayak wrote:
> On 05/26/2018 06:38 AM, David Collins wrote:
>>
>>> +	[1] = &sdm845_mx,
>>> +	[2] = &sdm845_mx_ao,
>>> +	[3] = &sdm845_cx,
>>> +	[4] = &sdm845_cx_ao,
>>> +	[5] = &sdm845_lmx,
>>> +	[6] = &sdm845_lcx,
>>> +	[7] = &sdm845_gfx,
>>> +	[8] = &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)
>>> +{
>>> +	struct tcs_cmd cmd = {
>>> +		.addr = pd->addr,
>>> +		.data = corner,
>>> +	};
>>> +
>>> +	return rpmh_write(pd->dev, state, &cmd, 1);
>> This can be optimized by calling rpmh_write_async() whenever the corner
>> being sent is smaller than the last value sent.  That way, no time is
>> wasted waiting for an ACK when decreasing voltage.  Would you mind adding
>> the necessary check and previous request caching for this?
> 
> is it safe to assume all sleep votes can be sent as async always? and only
> active votes could be sync/async depending on the last value sent?

Yes, rpmh_write_async() can always be used for RPMH_SLEEP_STATE and
RPMH_WAKE_ONLY_STATE.  The rpmh_write() vs rpmh_write_async() distinction
only matters for RPMH_ACTIVE_ONLY_STATE.

Take care,
David

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

Re: [PATCH v2 5/6] soc: qcom: rpmh powerdomain driver

[Lina Iyer]

On Fri, May 25 2018 at 19:08 -0600, David Collins wrote:
>Hello Rajendra,
>
>On 05/25/2018 03:01 AM, Rajendra Nayak wrote:
>> +
>> +	to_active_sleep(pd, pd->corner, &this_corner, &this_sleep_corner);
>> +
>> +	if (peer && peer->enabled)
>> +		to_active_sleep(peer, peer->corner, &peer_corner,
>> +				&peer_sleep_corner);
>> +
>> +	active_corner = max(this_corner, peer_corner);
>> +
>> +	ret = rpmhpd_send_corner(pd, RPMH_ACTIVE_ONLY_STATE, active_corner);
>> +	if (ret)
>> +		return ret;
>> +
>> +	sleep_corner = max(this_sleep_corner, peer_sleep_corner);
>> +
>> +	return rpmhpd_send_corner(pd, RPMH_SLEEP_STATE, sleep_corner);
>> +}
>
>This aggregation function as well as the rpmhpd_send_corner() calls below
>are not sufficient for RPMh.  There are 3 states that must all be used:
>RPMH_ACTIVE_ONLY_STATE, RPMH_WAKE_ONLY_STATE, and RPMH_SLEEP_STATE.  The
>naming is somewhat confusing as rpmhpd is defining a different concept of
>active-only.
>
>For power domains without active-only or peers, only
>RPMH_ACTIVE_ONLY_STATE should be used.  This instructs RPMh to issue the
>request immediately.
>
>For power domains with active-only, requests will need to be made for all
>three.  active_corner would be sent for both RPMH_ACTIVE_ONLY_STATE (so
>that the request takes effect immediately) and RPMH_WAKE_ONLY_STATE (so
>that the request is inserted into the wake TCS).  sleep_corner would be
>sent for RPMH_SLEEP_STATE (so that the request is inserted into the sleep
>TCS).
>
>You can see how this is handled in the RPMh clock driver in patch [3].
>
>You may be able to get away with using only RPMH_SLEEP_STATE and
>RPMH_ACTIVE_ONLY_STATE assuming that you issue the RPMH_SLEEP_STATE
>request first due to the rpmh driver caching behavior added in the
>cache_rpm_request() function in [4].  However, could you please confirm
>with Lina that this usage will continue to work in the future?  I'm not
>sure what guarantees are made at the rpmh API level.
>
We expect to cache all active values into wake if there was a sleep
value already defined. Expect to continue this behavior in the future
as well. But it would be safer for you to send sleep and wake votes in
addition to active votes. It shouldn't add too much of an overhead.

-- Lina

Re: [PATCH v2 5/6] soc: qcom: rpmh powerdomain driver

[Rob Herring]

On Fri, May 25, 2018 at 03:31:20PM +0530, Rajendra Nayak wrote:
> The RPMh powerdomain driver aggregates the corner votes from various
> consumers for the ARC resources and communicates it to RPMh.
> 
> We also add data for all powerdomains on sdm845 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>
> ---
>  .../devicetree/bindings/power/qcom,rpmhpd.txt |  65 ++++
>  drivers/soc/qcom/Kconfig                      |   9 +
>  drivers/soc/qcom/Makefile                     |   1 +
>  drivers/soc/qcom/rpmhpd.c                     | 360 ++++++++++++++++++
>  4 files changed, 435 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power/qcom,rpmhpd.txt
>  create mode 100644 drivers/soc/qcom/rpmhpd.c
> 
> diff --git a/Documentation/devicetree/bindings/power/qcom,rpmhpd.txt b/Documentation/devicetree/bindings/power/qcom,rpmhpd.txt
> new file mode 100644
> index 000000000000..c1fa986c12ee
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/qcom,rpmhpd.txt
> @@ -0,0 +1,65 @@
> +Qualcomm RPMh Powerdomains
> +
> +* For RPMh powerdomains, 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 powerdomain 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>,
> +				      <&rpmhpd_opp_table>,
> +				      <&rpmhpd_opp_table>,
> +				      <&rpmhpd_opp_table>,
> +				      <&rpmhpd_opp_table>,
> +				      <&rpmhpd_opp_table>,
> +				      <&rpmhpd_opp_table>,
> +				      <&rpmhpd_opp_table>;
> +	};
> +
> +	rpmhpd_opp_table: opp-table {
> +		compatible = "operating-points-v2-qcom-level", "operating-points-v2";
> +
> +		rpmhpd_opp1: opp@1 {
> +			qcom-corner = <16>;

Is it corner or level?

> +		};
> +
> +		rpmhpd_opp2: opp@2 {
> +			qcom-corner = <48>;
> +		};
> +
> +		rpmhpd_opp3: opp@3 {
> +			qcom-corner = <64>;
> +		};
> +
> +		rpmhpd_opp4: opp@4 {
> +			qcom-corner = <128>;
> +		};
> +
> +		rpmhpd_opp5: opp@5 {
> +			qcom-corner = <192>;
> +		};
> +
> +		rpmhpd_opp6: opp@6 {
> +			qcom-corner = <256>;
> +		};
> +
> +		rpmhpd_opp7: opp@7 {
> +			qcom-corner = <320>;
> +		};
> +
> +		rpmhpd_opp8: opp@8 {
> +			qcom-corner = <416>;
> +		};
> +	};

Re: [PATCH v2 5/6] soc: qcom: rpmh powerdomain driver

[Rajendra Nayak]

On 05/31/2018 09:01 AM, Rob Herring wrote:
> On Fri, May 25, 2018 at 03:31:20PM +0530, Rajendra Nayak wrote:
>> The RPMh powerdomain driver aggregates the corner votes from various
>> consumers for the ARC resources and communicates it to RPMh.
>>
>> We also add data for all powerdomains on sdm845 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>
>> ---
>>  .../devicetree/bindings/power/qcom,rpmhpd.txt |  65 ++++
>>  drivers/soc/qcom/Kconfig                      |   9 +
>>  drivers/soc/qcom/Makefile                     |   1 +
>>  drivers/soc/qcom/rpmhpd.c                     | 360 ++++++++++++++++++
>>  4 files changed, 435 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/power/qcom,rpmhpd.txt
>>  create mode 100644 drivers/soc/qcom/rpmhpd.c
>>
>> diff --git a/Documentation/devicetree/bindings/power/qcom,rpmhpd.txt b/Documentation/devicetree/bindings/power/qcom,rpmhpd.txt
>> new file mode 100644
>> index 000000000000..c1fa986c12ee
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/power/qcom,rpmhpd.txt
>> @@ -0,0 +1,65 @@
>> +Qualcomm RPMh Powerdomains
>> +
>> +* For RPMh powerdomains, 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 powerdomain 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>,
>> +				      <&rpmhpd_opp_table>,
>> +				      <&rpmhpd_opp_table>,
>> +				      <&rpmhpd_opp_table>,
>> +				      <&rpmhpd_opp_table>,
>> +				      <&rpmhpd_opp_table>,
>> +				      <&rpmhpd_opp_table>,
>> +				      <&rpmhpd_opp_table>;
>> +	};
>> +
>> +	rpmhpd_opp_table: opp-table {
>> +		compatible = "operating-points-v2-qcom-level", "operating-points-v2";
>> +
>> +		rpmhpd_opp1: opp@1 {
>> +			qcom-corner = <16>;
> 
> Is it corner or level?

It should be level, I will fix it up when I respin. thanks.

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

[PATCH v2 6/6] 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 | 9 +++++++++
 drivers/soc/qcom/rpmpd.c  | 9 +++++++++
 2 files changed, 18 insertions(+)

diff --git a/drivers/soc/qcom/rpmhpd.c b/drivers/soc/qcom/rpmhpd.c
index a79f094ad326..933f7b2d9249 100644
--- a/drivers/soc/qcom/rpmhpd.c
+++ b/drivers/soc/qcom/rpmhpd.c
@@ -323,6 +323,15 @@ 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
+		 */
+		rpmhpd_power_on(&rpmhpds[i]->pd);
+		rpmhpd_set_performance(&rpmhpds[i]->pd, rpmhpds[i]->level_count - 1);
 	}
 
 	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 ff3d910f388c..ff5da19beef3 100644
--- a/drivers/soc/qcom/rpmpd.c
+++ b/drivers/soc/qcom/rpmpd.c
@@ -308,6 +308,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