lkml.org 
[lkml]   [2018]   [Oct]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH RFC v1 4/8] drivers: qcom: cpu_pd: add cpu power domain support using genpd
    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@codeaurora.org>
    > Signed-off-by: Raju P.L.S.S.S.N <rplsssn@codeaurora.org>
    > ---
    > 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

    \
     
     \ /
      Last update: 2018-10-12 16:34    [W:4.194 / U:0.036 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site