Subject: [PATCH v13 03/10] qcom: spm: Add Subsystem Power Manager driver

From: [email-masked] (Lina Iyer)
Date: Mon, 1 Dec 2014 11:57:05 -0700

On Thu, Nov 27 2014 at 08:01 -0700, Lorenzo Pieralisi wrote:
>On Thu, Nov 27, 2014 at 05:24:07AM +0000, Lina Iyer wrote:
>
>[...]
>
>> +static int spm_set_low_power_mode(enum pm_sleep_mode mode)
>> +{
>> +       struct spm_driver_data *drv = per_cpu(cpu_spm_drv,
>> +                                               smp_processor_id());
>> +       u32 start_index;
>> +       u32 ctl_val;
>> +
>> +       if (!drv)
>> +               return -ENXIO;
>> +
>> +       start_index = drv->reg_data->start_index[mode];
>> +
>> +       ctl_val = spm_register_read(drv, SPM_REG_SPM_CTL);
>> +       ctl_val &= ~(SPM_CTL_INDEX << SPM_CTL_INDEX_SHIFT);
>> +       ctl_val |= start_index << SPM_CTL_INDEX_SHIFT;
>> +       ctl_val |= SPM_CTL_EN;
>> +       spm_register_write(drv, SPM_REG_SPM_CTL, ctl_val);
>> +
>> +       /* Ensure we have written the start address */
>> +       wmb();
>
>Can you explain please what this wmb is meant to achieve ? If it is there
>to make sure the write made it to the SPM that's not a proper way
>to achieve it, the barrier ensures ordering, not write completion.
>
A write completion would be great, but assuming that the context switch
using SCM or WFI could ensure the flush, then ordering is probably good
enough for this.

Is my understanding incorrect?

>> +
>> +       return 0;
>> +}
>> +
>> +static int qcom_pm_collapse(unsigned long int unused)
>> +{
>> +       int ret;
>> +       u32 flag;
>> +       int cpu = smp_processor_id();
>> +
>> +       ret = scm_set_warm_boot_addr(cpu_resume, cpu);
>> +       if (ret)
>> +               return ret;
>> +
>> +       flag = SCM_L2_ON & SCM_FLUSH_FLAG_MASK;
>> +       scm_call_atomic1(SCM_SVC_BOOT, SCM_CMD_TERMINATE_PC, flag);
>> +
>> +       /*
>> +        * Returns here only if there was a pending interrupt and we did not
>> +        * power down as a result.
>> +        */
>> +       return 0;
>
>I know the return value is changed by the cpu_suspend implementation to
>signal error when not returning from cpu_resume, but you should not
>return 0 anyway.
>
Sure. Will do.
>> +}
>> +
>> +static int qcom_cpu_standby(void *unused)
>> +{
>> +       int ret;
>> +
>> +       ret = spm_set_low_power_mode(PM_SLEEP_MODE_STBY);
>> +       if (ret)
>> +               return ret;
>> +
>> +       cpu_do_idle();
>> +
>> +       return 0;
>> +}
>> +
>> +static int qcom_cpu_spc(void *unused)
>> +{      int ret;
>> +
>> +       ret = spm_set_low_power_mode(PM_SLEEP_MODE_STBY);
>> +       if (ret)
>> +               return ret;
>> +
>> +       cpu_pm_enter();
>> +       cpu_suspend(0, qcom_pm_collapse);
>
>You must stash the return value and propagate it.
>
Sure. Will do.
>> +       cpu_pm_exit();
>> +
>> +       return 0;
>> +}
>> +
>> +static struct spm_driver_data *spm_get_drv(struct platform_device *pdev,
>> +               int *spm_cpu)
>> +{
>> +       struct spm_driver_data *drv = NULL;
>> +       struct device_node *cpu_node, *saw_node;
>> +       int cpu;
>> +       bool found = false;
>> +
>> +       for_each_possible_cpu(cpu) {
>> +               cpu_node = of_cpu_device_node_get(cpu);
>> +               if (!cpu_node)
>> +                       continue;
>> +               saw_node = of_parse_phandle(cpu_node, "qcom,saw", 0);
>
>+               if (saw_node) {
>+                       if (saw_node == pdev->dev.of_node)
>+                               found = true;
>+                       of_node_put(saw_node);
>+               }
>
>can be:
>		found = (saw_node == pdev->dev.of_node);
>		of_node_put(saw_node);
>
Sure.

>Thanks,
>Lorenzo
>
>> +               of_node_put(cpu_node);
>> +               if (found)
>> +                       break;
>> +       }
>> +
>> +       if (found) {
>> +               drv = devm_kzalloc(&pdev->dev, sizeof(*drv), GFP_KERNEL);
>> +               if (drv)
>> +                       *spm_cpu = cpu;
>> +       }
>> +
>> +       return drv;
>> +}
>> +
>> +static const struct of_device_id spm_match_table[] = {
>> +       { .compatible = "qcom,msm8974-saw2-v2.1-cpu",
>> +         .data = &spm_reg_8974_8084_cpu },
>> +       { .compatible = "qcom,apq8084-saw2-v2.1-cpu",
>> +         .data = &spm_reg_8974_8084_cpu },
>> +       { .compatible = "qcom,apq8064-saw2-v1.1-cpu",
>> +         .data = &spm_reg_8064_cpu },
>> +       { },
>> +};
>> +
>> +static const struct qcom_cpu_pm_ops lpm_ops = {
>> +       .standby = qcom_cpu_standby,
>> +       .spc = qcom_cpu_spc,
>> +};
>> +
>> +static int spm_dev_probe(struct platform_device *pdev)
>> +{
>> +       struct spm_driver_data *drv;
>> +       struct resource *res;
>> +       const struct of_device_id *match_id;
>> +       void __iomem *addr;
>> +       int cpu = -EINVAL;
>> +       static bool cpuidle_drv_init;
>> +       const struct platform_device_info qcom_cpuidle_info = {
>> +               .name   = "qcom_cpuidle",
>> +               .id     = -1,
>> +               .data = &lpm_ops,
>> +               .size_data = sizeof(lpm_ops),
>> +       };
>> +
>> +       drv = spm_get_drv(pdev, &cpu);
>> +       if (!drv || cpu < 0)
>> +               return -EINVAL;
>> +
>> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +       drv->reg_base = devm_ioremap_resource(&pdev->dev, res);
>> +       if (IS_ERR(drv->reg_base))
>> +               return PTR_ERR(drv->reg_base);
>> +
>> +       match_id = of_match_node(spm_match_table, pdev->dev.of_node);
>> +       if (!match_id)
>> +               return -ENODEV;
>> +
>> +       drv->reg_data = match_id->data;
>> +
>> +       /* Write the SPM sequences first.. */
>> +       addr = drv->reg_base + drv->reg_data->reg_offset[SPM_REG_SEQ_ENTRY];
>> +       __iowrite32_copy(addr, drv->reg_data->seq,
>> +                       ARRAY_SIZE(drv->reg_data->seq) / 4);
>> +
>> +       /*
>> +        * ..and then the control registers.
>> +        * On some SoC's if the control registers are written first and if the
>> +        * CPU was held in reset, the reset signal could trigger the SPM state
>> +        * machine, before the sequences are completely written.
>> +        */
>> +       spm_register_write(drv, SPM_REG_CFG, drv->reg_data->spm_cfg);
>> +       spm_register_write(drv, SPM_REG_DLY, drv->reg_data->spm_dly);
>> +       spm_register_write(drv, SPM_REG_PMIC_DLY, drv->reg_data->pmic_dly);
>> +
>> +       spm_register_write(drv, SPM_REG_PMIC_DATA_0,
>> +                               drv->reg_data->pmic_data[0]);
>> +       spm_register_write(drv, SPM_REG_PMIC_DATA_1,
>> +                               drv->reg_data->pmic_data[1]);
>> +
>> +       /*
>> +        * Ensure all observers see the above register writes before the
>> +        * cpuidle driver is allowed to use the SPM.
>> +        */
>> +       wmb();
>> +       per_cpu(cpu_spm_drv, cpu) = drv;
>> +
>> +       if (!cpuidle_drv_init) {
>> +               platform_device_register_full(&qcom_cpuidle_info);
>> +               cpuidle_drv_init = true;
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +static struct platform_driver spm_driver = {
>> +       .probe = spm_dev_probe,
>> +       .driver = {
>> +               .name = "saw",
>> +               .of_match_table = spm_match_table,
>> +       },
>> +};
>> +
>> +module_platform_driver(spm_driver);
>> +
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_DESCRIPTION("SAW power controller driver");
>> +MODULE_ALIAS("platform:saw");
>> diff --git a/include/soc/qcom/pm.h b/include/soc/qcom/pm.h
>> new file mode 100644
>> index 0000000..d9a56d7
>> --- /dev/null
>> +++ b/include/soc/qcom/pm.h
>> @@ -0,0 +1,31 @@
>> +/*
>> + * Copyright (c) 2009-2014, The Linux Foundation. All rights reserved.
>> + *
>> + * This software is licensed under the terms of the GNU General Public
>> + * License version 2, as published by the Free Software Foundation, and
>> + * may be copied, distributed, and modified under those terms.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + */
>> +
>> +#ifndef __QCOM_PM_H
>> +#define __QCOM_PM_H
>> +
>> +enum pm_sleep_mode {
>> +       PM_SLEEP_MODE_STBY,
>> +       PM_SLEEP_MODE_RET,
>> +       PM_SLEEP_MODE_SPC,
>> +       PM_SLEEP_MODE_PC,
>> +       PM_SLEEP_MODE_NR,
>> +};
>> +
>> +struct qcom_cpu_pm_ops {
>> +       int (*standby)(void *data);
>> +       int (*spc)(void *data);
>> +};
>> +
>> +#endif  /* __QCOM_PM_H */
>> --
>> 2.1.0
>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>
>--
>To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
>the body of a message to majordomo at vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html