On Thu, Oct 11, 2018 at 02:50:51AM +0530, Raju P.L.S.S.S.N wrote:
> RPMH based targets require that the sleep and wake state request votes
> be sent during system low power mode entry. The votes help reduce the
> power consumption when the AP is not using them. The votes sent by the
> clients are cached in RPMH controller and needs to be flushed when the
> last cpu enters low power mode. So add cpu power domain using Linux
> generic power domain infrastructure to perform necessary tasks as part
> of domain power down.
>
> Suggested-by: Lina Iyer <ilina@xxxxxxxxxxxxxx>
> Signed-off-by: Raju P.L.S.S.S.N <rplsssn@xxxxxxxxxxxxxx>
> ---
> drivers/soc/qcom/Kconfig | 9 ++++
> drivers/soc/qcom/Makefile | 1 +
> drivers/soc/qcom/cpu_pd.c | 104 ++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 114 insertions(+)
> create mode 100644 drivers/soc/qcom/cpu_pd.c
>
> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> index ba79b60..91e8b3b 100644
> --- a/drivers/soc/qcom/Kconfig
> +++ b/drivers/soc/qcom/Kconfig
> @@ -95,6 +95,7 @@ config QCOM_RMTFS_MEM
> config QCOM_RPMH
> bool "Qualcomm RPM-Hardened (RPMH) Communication"
> depends on ARCH_QCOM && ARM64 && OF || COMPILE_TEST
> + select QCOM_CPU_PD
> help
> Support for communication with the hardened-RPM blocks in
> Qualcomm Technologies Inc (QTI) SoCs. RPMH communication uses an
> @@ -102,6 +103,14 @@ config QCOM_RPMH
> of hardware components aggregate requests for these resources and
> help apply the aggregated state on the resource.
>
> +config QCOM_CPU_PD
> + bool "Qualcomm cpu power domain driver"
> + depends on QCOM_RPMH && PM_GENERIC_DOMAINS || COMPILE_TEST
> + help
> + Support for QCOM platform cpu power management to perform tasks
> + necessary while application processor votes for deeper modes so that
> + the firmware can enter SoC level low power modes to save power.
> +
> 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 f25b54c..57a1b0e 100644
> --- a/drivers/soc/qcom/Makefile
> +++ b/drivers/soc/qcom/Makefile
> @@ -12,6 +12,7 @@ obj-$(CONFIG_QCOM_RMTFS_MEM) += rmtfs_mem.o
> obj-$(CONFIG_QCOM_RPMH) += qcom_rpmh.o
> qcom_rpmh-y += rpmh-rsc.o
> qcom_rpmh-y += rpmh.o
> +obj-$(CONFIG_QCOM_CPU_PD) += cpu_pd.o
> obj-$(CONFIG_QCOM_SMD_RPM) += smd-rpm.o
> obj-$(CONFIG_QCOM_SMEM) += smem.o
> obj-$(CONFIG_QCOM_SMEM_STATE) += smem_state.o
> diff --git a/drivers/soc/qcom/cpu_pd.c b/drivers/soc/qcom/cpu_pd.c
> new file mode 100644
> index 0000000..565c510
> --- /dev/null
> +++ b/drivers/soc/qcom/cpu_pd.c
> @@ -0,0 +1,104 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
> + */
> +
> +#include <linux/of_platform.h>
> +#include <linux/pm_domain.h>
> +#include <linux/slab.h>
> +
> +#include <soc/qcom/rpmh.h>
> +
> +static struct device *cpu_pd_dev;
> +
This doesn't scale if you have 2 instances.
> +static int cpu_pd_power_off(struct generic_pm_domain *domain)
> +{
> + if (rpmh_ctrlr_idle(cpu_pd_dev)) {
How is this expected to compile ? I couldn't find any instance of this.
> + /* Flush the sleep/wake sets */
> + rpmh_flush(cpu_pd_dev);
So it's just flushing the pending requests on the controller. The function
implementation carries a note that it's assumed to be called only from
system PM and we may call it in cpu idle path here. Is that fine ?
If so, may be the comment needs to be dropped.
Also, where exactly this voting for CPU is happening in this path ?
> + } else {
> + pr_debug("rpmh controller is busy\n");
> + return -EBUSY;
> + }
> +
> + return 0;
> +}
> +
> +static int cpu_pm_domain_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct device_node *np = dev->of_node;
> + struct generic_pm_domain *cpu_pd;
> + int ret = -EINVAL, cpu;
> +
> + if (!np) {
> + dev_err(dev, "device tree node not found\n");
> + return -ENODEV;
> + }
> +
> + if (!of_find_property(np, "#power-domain-cells", NULL)) {
> + pr_err("power-domain-cells not found\n");
> + return -ENODEV;
> + }
> +
> + cpu_pd_dev = &pdev->dev;
> + if (IS_ERR_OR_NULL(cpu_pd_dev))
Isn't this too late to check ? You would have crashed on dev->of_node.
So sounds pretty useless
> + return PTR_ERR(cpu_pd_dev);
> +
> + cpu_pd = devm_kzalloc(dev, sizeof(*cpu_pd), GFP_KERNEL);
> + if (!cpu_pd)
> + return -ENOMEM;
> +
> + cpu_pd->name = kasprintf(GFP_KERNEL, "%s", np->name);
> + if (!cpu_pd->name)
> + goto free_cpu_pd;
> + cpu_pd->name = kbasename(cpu_pd->name);
> + cpu_pd->power_off = cpu_pd_power_off;
If some kind of voting is done in off, why is there nothing to take care
of that in pd_power_on if it's per EL(linux/hyp/secure).
--
Regards,
Sudeep
