[PATCH v7 1/7] qcom: spm: Add Subsystem Power Manager driver

Lina Iyer
2014-09-30T16:27:48Z

On Mon, Sep 29 2014 at 17:19 -0600, Stephen Boyd wrote:
>On 09/26/14 17:58, Lina Iyer wrote:

> Definition: shall contain "qcom,saw2". A more specific value should be
>> one of:
>> - "qcom,saw2-v1"
>> - "qcom,saw2-v1.1"
>> - "qcom,saw2-v2"
>> - "qcom,saw2-v2.1"
>> + "qcom,apq8064-saw2-v1.1-cpu"
>> + "qcom,msm8974-saw2-v2.1-cpu"
>> + "qcom,apq8084-saw2-v2.1-cpu"
>
>It's probably not good to remove the old compatibles. Just add more to
>the list. Please Cc dt reviewers on dt bindings.
>
You are right, I should not have removed them.
>>
>> - reg:
>> Usage: required
>> @@ -26,10 +25,9 @@ PROPERTIES
>> the register region. An optional second element specifies
>> the base address and size of the alias register region.
>>
>> -
>> Example:
>>
>> - regulator at 2099000 {
>> + saw at 2099000 {
>
>saw is not a standard thing. Hence the usage of regulator here. I agree
>when it doesn't directly control a regulator then it should be called
>something else, power-controller perhaps? I don't really see a need to
>change this example though.
>
I am okay with the name power controller.

>> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> +config QCOM_PM
>> + bool "Qualcomm Power Management"
>> + depends on PM && ARCH_QCOM
>
>Drop the PM dependency. There isn't any right? Honestly we don't want
>this type of option at all. We want an option for each driver introduced.
>
OK.

We want? Why? Thats just many config items that we have to configure to
build the kernel. If you know of a reason, please let me know.

[...]
> +
>> +/* SPM register data for 8974, 8084 */
>> +static const struct spm_reg_data spm_reg_8974_8084_cpu = {
>> + .reg[SPM_REG_CFG] = {0x08, 0x1},
>> + .reg[SPM_REG_SPM_STS] = {0x0C, 0x0},
>> + .reg[SPM_REG_PMIC_STS] = {0x14, 0x0},
>> + .reg[SPM_REG_VCTL] = {0x1C, 0x0},
>> + .reg[SPM_REG_SPM_CTL] = {0x30, 0x1},
>> + .reg[SPM_REG_DLY] = {0x34, 0x3C102800},
>> + .reg[SPM_REG_PMIC_DATA_0] = {0x40, 0x0},
>> + .reg[SPM_REG_PMIC_DATA_1] = {0x44, 0x0},
>> + .reg[SPM_REG_PMIC_DATA_2] = {0x48, 0x0},
>> + .reg[SPM_REG_SEQ_ENTRY_0] = {0x80, 0x000F0B03},
>> + .reg[SPM_REG_SEQ_ENTRY_1] = {0x84, 0xE8108020},
>> + .reg[SPM_REG_SEQ_ENTRY_2] = {0x88, 0xE83B035B},
>> + .reg[SPM_REG_SEQ_ENTRY_3] = {0x8C, 0x300B1082},
>> + .reg[SPM_REG_SEQ_ENTRY_4] = {0x90, 0x0F302606},
>
>Is this endian agnostic?
I dont think I considered that.

>We don't need this initial value table. The
>only thing that really is different is delay and seq entries.
I need the PMIC_DATA for supporting 8064. And the voltage control and
the status registers for verifying the changes. I looked at the common
functionality with SoC that I plan to support and used them here.

>The seq
>entries can be a byte array that gets written to the device in an endian
>agnostic fashion and the delay can be a different struct member.
Endianness is something I may need to think about. So for that purpose,
I may need to fashion this into sequences. I just removed a bunch of
code that did that. Made the driver a lot easy on the eyes.

>The
>register map can be per version of the spm (i.e. not per-soc) and that
>can be pointed to by the SoC data.
>
I thought about it, its just unnecessary bunch of data structures. This
is clearly a name, value pair and is much more readable.

>I really don't like setting the SPM_CTL register's enable bit to 1 with
>this table either. That should be done explicitly because it isn't
>"configuration" like the delays or the sequences are. It's a bit that
>will have some effect. It probably even needs to be cleared if we're
>reprogramming the SPM sequence in a scenario like kexec where the bit
>may already be set.
>
Fair enough.

>> + .reg[SPM_REG_SEQ_ENTRY_5] = {0x94, 0x0},
>> + .reg[SPM_REG_SEQ_ENTRY_6] = {0x98, 0x0},
>> + .reg[SPM_REG_SEQ_ENTRY_7] = {0x9C, 0x0},
>> +
>> + .start_addr[SPM_MODE_CLOCK_GATING] = 0,
>> + .start_addr[SPM_MODE_POWER_COLLAPSE] = 3,
>> +};
>> +
>> +static DEFINE_PER_CPU_SHARED_ALIGNED(struct spm_driver_data, cpu_spm_drv);
>> +
>> +/**
>> + * spm_set_low_power_mode() - Configure SPM start address for low power mode
>> + * @mode: SPM LPM mode to enter
>> + */
>> +int qcom_spm_set_low_power_mode(enum spm_mode mode)
>> +{
>> + struct spm_driver_data *drv = &__get_cpu_var(cpu_spm_drv);
>> + u32 start_addr = 0;
>
>Unnecessary assignment.
>
Done.
>> + u32 ctl_val;
>> +
>> + if (!drv || !drv->reg_data)
>> + return -ENXIO;
>
>Does this ever happen? Please remove.
>
Possible, if the idle driver makes a call, before the SPM is ready.
>> +
>> + start_addr = drv->reg_data->start_addr[mode];
>> +
>> + /* Update bits 10:4 in the SPM CTL register */
>
>This comment provides nothing that isn't evident from the code. Remove.
>
okay

> + for_each_possible_cpu(cpu) {
>> + cpu_node = of_get_cpu_node(cpu, NULL);
>> + if (!cpu_node)
>> + continue;
>> + saw_node = of_parse_phandle(cpu_node, "qcom,saw", 0);
>> + if (!saw_node)
>> + continue;
>> + if (saw_node == pdev->dev.of_node) {
>> + drv = &per_cpu(cpu_spm_drv, cpu);
>> + break;
>> + }
>
>Missing a couple of_node_put()s.
>
Argh. I saw them after I sent the patches. Thanks for pointing it out.

> +
>> +static struct platform_driver spm_driver = {
>> + .probe = spm_dev_probe,
>> + .driver = {
>> + .name = "spm",
>
>qcom-spm?
>
ok

>> +static int __init spm_driver_init(void)
>> +{
>> + return platform_driver_register(&spm_driver);
>> +}
>> +device_initcall(spm_driver_init);
>
>Why can't we support modules?
>
It just happens later than we would like.

>> diff --git a/include/soc/qcom/spm.h b/include/soc/qcom/spm.h
>> new file mode 100644
>> index 0000000..997abfc
>> --- /dev/null
>> +++ b/include/soc/qcom/spm.h

> +#endif /* __QCOM_SPM_H */
>
>It would be nice to not have this file.
>
Why?

Thanks for your time Stephen.

Lina