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

[Daniel Lezcano <daniel.lezcano@linaro.org>]

On 12/01/2014 07:50 PM, Lina Iyer wrote:
> On Thu, Nov 27 2014 at 01:52 -0700, Daniel Lezcano wrote:
>> On 11/27/2014 06:24 AM, Lina Iyer wrote:
>
>> +    static bool cpuidle_drv_init;
>>
>>        ^^^^^^^^^
>>
>> As already said in a previous comment, please find a way to remove that.
>>
> I will look into it. Stephen and I wanted the cpuidle driver to be
> probed only after any of the SPMs are ready. And possibly, only for that
> cpu, for which the SPM has been probed.
>
> To achieve the SPM -> CPUIDLE Device relation, I havent found a good way
> to do that. Without using CPUIDLE_MULTIPLE_DRIVERS, initializing each
> cpuidle device, separate from the cpuidle driver, requires that I update
> the cpuidle_driver->cpumask after probing each SPM device, to allow for
> only one driver and cpuidle devices only for the probed cpus. Using the
> cpuidle_register_driver(), resets the drv->refcnt in
> __cpuidle_driver_init.
>
> I may need something like this in the else clause of
> CPUIDLE_MULTIPLE_DRIVERS -
>
> static inline struct cpuidle_driver *__cpuidle_get_cpu_driver(int cpu)
> {
>         struct cpuidle_driver *drv = cpuidle_curr_driver;
>
>         if (drv && !cpumask_test_cpu(cpu, drv->cpumask))
>                  drv = NULL;
>
>         return drv;
> }

You don't have to deal with the drv->cpumask and 
CPUIDLE_MULTIPLE_DRIVERS. This field gives the list of the cpus the 
driver will have to potentially handle.

Just register the driver at the first place with the platform device by 
using cpuidle_register_driver(drv), I suggest somewhere else than the 
spm code.

The underlying code will do:

         if (!drv->cpumask)
                 drv->cpumask = (struct cpumask *)cpu_possible_mask;

Until the cpuidle device is not initialized for a specific cpu, the 
cpuidle code will have no effect for this cpu. If you register the 
driver but without registering the cpuidle devices that will result in 
nothing more than a structure initialized but inoperative.

For each SPM being probed:

struct cpuidle_device *dev = &per_cpu(cpuidle_dev, cpu);
dev->cpu = cpu;
cpuidle_register_device(dev);

Note cpuidle_dev is exported via cpuidle.h, so you don't have to 
redeclare a per cpu cpuidle device by your own.

So rephrasing all that:

(1) in cpuidle-qcom register the driver
(2) in the spm code register the device for each spm probe

(1) being called before (2).

If after that in the code you still have a "static bool 
cpuidle_drv_init", then I guess we have a problem in the init sequence 
somewhere else.

> void cpuidle_update_cpumask(struct cpumask *mask) {
>         struct cpuidle_driver *drv;
>
>         spin_lock(&cpuidle_driver_lock);
>         drv = cpuidle_get_driver();
>      if (drv)
>          drv->cpumask = mask ?: cpu_possible_mask;
>         spin_unlock(&cpuidle_driver_lock);
> }
>
> With that, I could register cpuidle driver the first time when the mask
> changes
> from empty and consequent updates would just update the cpumask. (I am not
> sure, if I missed anything in this change). It just seemed far too
> invasive at
> this time, in lieu of the static bool.
>
>>> +    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;
>>
>> As already said in a previous comment, it is not possible to have "cpu
>> < 0" with "drv != NULL", so except I am missing something the test
>> should be:
>>
>>     if (!drv)
>>         return -EINVAL;
>>
> Sorry, done.
>
>> There is something wrong with the init sequence. Don't you find weird
>> you have to backward search for the cpu belonging to the pdev each
>> time the probe function is called ?
>>
>>
> Well, it was that or the SPM device node pointing to the CPU that it
> references. It seems more common to have an iterator than the doubly
> linked device nodes. I dont have a strong preference either way, just
> chose the way that made device nodes easier.
>
>>> +
>>> +    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 */
>>>
>>
>>
>> --
>> <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
>>
>> Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
>> <http://twitter.com/#!/linaroorg> Twitter |
>> <http://www.linaro.org/linaro-blog/> Blog
>>


-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog