[PATCH v4 3/8] qcom: spm: Add Subsystem Power Manager driver (SAW2)

[PATCH v4 3/8] qcom: spm: Add Subsystem Power Manager driver (SAW2)

[Stephen Boyd]

On 08/19/14 20:24, Lina Iyer wrote:
> On Tue, Aug 19, 2014 at 07:01:53PM -0700, Stephen Boyd wrote:
>> On 08/19/14 15:15, Lina Iyer wrote:
>>> SPM is a hardware block that controls the peripheral logic surrounding
>>> the application cores (cpu/l$). When the core executes WFI instruction,
>>> the SPM takes over the putting the core in low power state as
>>> configured. The wake up for the SPM is an interrupt at the GIC, which
>>> then completes the rest of low power mode sequence and brings the core
>>> out of low power mode.
>>>
>>> The SPM has a set of control registers that configure the SPMs
>>> individually based on the type of the core and the runtime conditions.
>>> SPM is a finite state machine block to which a sequence is provided and
>>> it interprets the bytes  and executes them in sequence. Each low power
>>> mode that the core can enter into is provided to the SPM as a sequence.
>>>
>>> Configure the SPM to set the core (cpu or L2) into its low power mode,
>>> the index of the first command in the sequence is set in the SPM_CTL
>>> register. When the core executes ARM wfi instruction, it triggers the
>>> SPM state machine to start executing from that index. The SPM state
>>> machine waits until the interrupt occurs and starts executing the rest
>>> of the sequence until it hits the end of the sequence. The end of the
>>> sequence jumps the core out of its low power mode.
>>>
>>> Signed-off-by: Praveen Chidambaram <pchidamb@codeaurora.org>
>>> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
>>
>> Why isn't this code combined with the cpuidle driver in qcom-cpuidle.c,
>> spm-devices.c, and qcom-pm.c? All these files are interacting with the
>> same hardware, I'm confused why we have to have 4 different files and
>> all these different patches to support this. Basically patches 3, 4, 6
>> and 7 should be one file under drivers/cpuidle/ named qcom-cpuidle or
>> saw-cpuidle.
>
> They all interact with the one hardware, you are right about that. But
> each
> of these have a responsibility and provide certain functionality that
> builds up into the idle framework.
> Let me explain - The hardware driver spm.c doesnt care what the code
> calling the driver, intends to do. All it knows is to write to right
> register. And it can write to only one SPM block. There are multiple SPM
> blocks which need to be managed and thats provided by spm-devices. The
> cpuidle framework or the hotplug framework needs to do the same thing.
> The common functionality between idle and hotplug is abstraced out in
> msm-pm.c.  platsmp.c and cpuidle-qcom.c both would need the same
> functionality.

I can see the end result in the downstream vendor tree and it isn't
pretty. The code winds through a handful of different files. The spm.c
file is basically just a bunch of functions to read and write to an SPM.
By itself it's pretty much worthless and doesn't stand on its own.
spm-devices is where we would actually probe a driver for the device
that is read/written in spm.c. At the least, these two files should be
combined into one driver.

Right now just to support cpuidle it seems that it could all be in one
file. When we get to hotplug, why don't we use cpuidle_play_dead() on
ARM so that this cpuidle driver can configure the SPM for the deepest
idle state and power off the CPU? The only problem I see is that we
would need another hook for the cpu_kill() op so that we can wait for
the state machine to finish bringing down the CPU. That would remove the
need for msm-pm.c?

This would handle the cpuidle_play_dead() part. Alternatively we call
cpuidle_play_dead() from the cpu_die() hook in the platform layer, but
I'd prefer we call it directly in arch code if we can.

diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
index 7c4fada440f0..fef953ecde22 100644
--- a/arch/arm/kernel/smp.c
+++ b/arch/arm/kernel/smp.c
@@ -26,6 +26,7 @@
 #include <linux/completion.h>
 #include <linux/cpufreq.h>
 #include <linux/irq_work.h>
+#include <linux/cpuidle.h>
 
 #include <linux/atomic.h>
 #include <asm/smp.h>
@@ -290,8 +291,9 @@ void __ref cpu_die(void)
         * The return path should not be used for platforms which can
         * power off the CPU.
         */
-       if (smp_ops.cpu_die)
-               smp_ops.cpu_die(cpu);
+       if (cpuidle_play_dead())
+               if (smp_ops.cpu_die)
+                       smp_ops.cpu_die(cpu);
 
        pr_warn("CPU%u: smp_ops.cpu_die() returned, trying to resuscitate\n",
                cpu);


>
> The SPM is not simple register write. It needs quite a bit of
> configuration and to make it functional and then you may do register
> writes.  SPM also provides voltage control functionality, which again
> has a lot of support code that need to ensure the hardware is in the
> correct state before you do that one register write to set the voltage.
> Again, this and other functionality add up to a whole lot of code to be
> clumped up in qcom-cpuidle.c and duplicated for hotplug again.  It is
> beneficial to have them this way. Bear with me, as I build up this
> framework to support the idle and hotplug idle framework.
>

Yes I'm not quite sure how we would handle adding the regulator code. In
some cases each CPU's SAW is controlling a regulator and in other cases
there's only the L2 SAW controlling a regulator. We would need these
regulators to exist even if we don't support cpuidle, so we'd need to
register it somehow. I was wondering if we shouldn't be using the
register in the SAW to change the voltage, instead we should go directly
to the PMIC and write the regulator driver on top of the PMIC interface.
The spm code could then look at those regulators to figure out what
voltage the supply is configured to so that it can be programmed into
the SPM and used during any power restoring activities. I'm not sure if
we can even write the PMIC registers though, or if those registers are
locked down thus requiring us to use the SPM interface to change
voltages. It would be nice to split this out somehow though. Another
idea would be to make some shim driver that creates two child devices
for the regulator and cpuidle drivers and have the shim driver make a
regmap that both drivers use.

>>
>>>
>>> diff --git a/drivers/soc/qcom/spm.c b/drivers/soc/qcom/spm.c
>>> new file mode 100644
>>> index 0000000..19b79d0
>>> --- /dev/null
>>> +++ b/drivers/soc/qcom/spm.c
>>> @@ -0,0 +1,195 @@
>> [..]
>>> +
>>> +static void flush_shadow(struct msm_spm_driver_data *drv,
>>> +        unsigned int reg_index)
>>> +{
>>> +    __raw_writel(drv->reg_shadow[reg_index],
>>> +            drv->reg_base_addr + drv->reg_offsets[reg_index]);
>>> +}
>>
>> What's the use of the "shadow" functionality? Why can't we just read and
>> write the registers directly without having to go through a register
>> cache?
> Helps speed up reads and write, when you have multiple writes. Also, is
> an excellent mechanism to know the state SPM was configured to, in the
> event of a mishap.

Huh? How can writing something into memory and then writing it to the
device be faster than writing it directly to the device? If we need to
know how the SPM was configured I assume we would know by printks or by
inspecting the code. I still don't see a reason for this.

>>
>>> +
>>> +static void load_shadow(struct msm_spm_driver_data *drv,
>>> +        unsigned int reg_index)
>>> +{
>>> +    drv->reg_shadow[reg_index] = __raw_readl(drv->reg_base_addr +
>>> +                        drv->reg_offsets[reg_index]);
>>
>> Please use readl_relaxed/writel_relaxed. The raw accessors don't
>> byte-swap and fail horribly for big-endian kernels.
> OK. But does it matter that the SoC the code is expected to run is
> little-endian?

big-endian kernels work on these platforms. Nobody is extensively
testing it but putting down more roadblocks to using it isn't helpful.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

[PATCH v4 3/8] qcom: spm: Add Subsystem Power Manager driver (SAW2)

[Lina Iyer]

On Wed, Aug 20, 2014 at 05:25:49PM -0700, Stephen Boyd wrote:
>On 08/19/14 20:24, Lina Iyer wrote:
>> On Tue, Aug 19, 2014 at 07:01:53PM -0700, Stephen Boyd wrote:
>>> On 08/19/14 15:15, Lina Iyer wrote:
>>>> SPM is a hardware block that controls the peripheral logic surrounding
>>>> the application cores (cpu/l$). When the core executes WFI instruction,
>>>> the SPM takes over the putting the core in low power state as
>>>> configured. The wake up for the SPM is an interrupt at the GIC, which
>>>> then completes the rest of low power mode sequence and brings the core
>>>> out of low power mode.
>>>>
>>>> The SPM has a set of control registers that configure the SPMs
>>>> individually based on the type of the core and the runtime conditions.
>>>> SPM is a finite state machine block to which a sequence is provided and
>>>> it interprets the bytes  and executes them in sequence. Each low power
>>>> mode that the core can enter into is provided to the SPM as a sequence.
>>>>
>>>> Configure the SPM to set the core (cpu or L2) into its low power mode,
>>>> the index of the first command in the sequence is set in the SPM_CTL
>>>> register. When the core executes ARM wfi instruction, it triggers the
>>>> SPM state machine to start executing from that index. The SPM state
>>>> machine waits until the interrupt occurs and starts executing the rest
>>>> of the sequence until it hits the end of the sequence. The end of the
>>>> sequence jumps the core out of its low power mode.
>>>>
>>>> Signed-off-by: Praveen Chidambaram <pchidamb@codeaurora.org>
>>>> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
>>>
>>> Why isn't this code combined with the cpuidle driver in qcom-cpuidle.c,
>>> spm-devices.c, and qcom-pm.c? All these files are interacting with the
>>> same hardware, I'm confused why we have to have 4 different files and
>>> all these different patches to support this. Basically patches 3, 4, 6
>>> and 7 should be one file under drivers/cpuidle/ named qcom-cpuidle or
>>> saw-cpuidle.
>>
>> They all interact with the one hardware, you are right about that. But
>> each
>> of these have a responsibility and provide certain functionality that
>> builds up into the idle framework.
>> Let me explain - The hardware driver spm.c doesnt care what the code
>> calling the driver, intends to do. All it knows is to write to right
>> register. And it can write to only one SPM block. There are multiple SPM
>> blocks which need to be managed and thats provided by spm-devices. The
>> cpuidle framework or the hotplug framework needs to do the same thing.
>> The common functionality between idle and hotplug is abstraced out in
>> msm-pm.c.  platsmp.c and cpuidle-qcom.c both would need the same
>> functionality.
>
>I can see the end result in the downstream vendor tree and it isn't
>pretty. The code winds through a handful of different files. The spm.c
>file is basically just a bunch of functions to read and write to an SPM.

>By itself it's pretty much worthless and doesn't stand on its own.

I wouldnt put it that way. It abstracts a whole of device functionality
away from rest of the code. If look at the history of the driver in the
downstream, you would undertand that multiple version of SPM exists and
at some point, they all were supported by the same code base. The spm.c
is valuable that way in abstracting the bit manipulations away from
managing the device. Different version manipulate the same registers
differently and I can guarantee that this wont be the last of the h/w
revision either. A separate file helps keep the implementations clean
and manage different versions of SPM better.

>spm-devices is where we would actually probe a driver for the device
>that is read/written in spm.c. At the least, these two files should be
>combined into one driver.
>
Thats very simplistic approach. Reiterating, what I mention earlier, the
driver when clubbed, would get complicated and real huge, very soon.

>Right now just to support cpuidle it seems that it could all be in one
>file. When we get to hotplug, why don't we use cpuidle_play_dead() on
>ARM so that this cpuidle driver can configure the SPM for the deepest
>idle state and power off the CPU? The only problem I see is that we
>would need another hook for the cpu_kill() op so that we can wait for
>the state machine to finish bringing down the CPU. That would remove the
>need for msm-pm.c?

I will explore the cpuidle_play_dead() to see how much we can reuse
there.

This file is at its very infancy, only switches the low power mode to the
right function, but wait until you have to handle two different kinds of
processors - Krait and vanilla ARM cores using the same code base for
the same vendor. And the many corner cases you need to solve. What we do
if we remove msm-pm.c at this point is build up all the code in cpuidle
and when it comes to differenciating and handle corner cases, we will
split the file again to abstract things better.
If you had followed the previous versions of this patch, you would
realize, that I had pruned a whole lot of code. Removing this file would
make putting those code back really difficult and the driver needs all that
code.

Downstream is dictated by the milliamp of power saving you can squeeze
out of the SoC and it is inevitable that we would have to handle atleast
some of these usecases. Broad spectrum generic low power mode, barely
gets the debug boards running. If our goal is to have upstream code
any close to being as functional as downstream, you would need to do a
multitude of things and clubbing them all in one procedural file does
not help the effort. The cover letter explains how the hierarchy is and
the patches will evolve the hierarchy to full functionality.

>This would handle the cpuidle_play_dead() part. Alternatively we call
>cpuidle_play_dead() from the cpu_die() hook in the platform layer, but
>I'd prefer we call it directly in arch code if we can.
>
>diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
>index 7c4fada440f0..fef953ecde22 100644
>--- a/arch/arm/kernel/smp.c
>+++ b/arch/arm/kernel/smp.c
>@@ -26,6 +26,7 @@
> #include <linux/completion.h>
> #include <linux/cpufreq.h>
> #include <linux/irq_work.h>
>+#include <linux/cpuidle.h>
>
> #include <linux/atomic.h>
> #include <asm/smp.h>
>@@ -290,8 +291,9 @@ void __ref cpu_die(void)
>         * The return path should not be used for platforms which can
>         * power off the CPU.
>         */
>-       if (smp_ops.cpu_die)
>-               smp_ops.cpu_die(cpu);
>+       if (cpuidle_play_dead())
>+               if (smp_ops.cpu_die)
>+                       smp_ops.cpu_die(cpu);
>
>        pr_warn("CPU%u: smp_ops.cpu_die() returned, trying to resuscitate\n",
>                cpu);
>
>
>>
>> The SPM is not simple register write. It needs quite a bit of
>> configuration and to make it functional and then you may do register
>> writes.  SPM also provides voltage control functionality, which again
>> has a lot of support code that need to ensure the hardware is in the
>> correct state before you do that one register write to set the voltage.
>> Again, this and other functionality add up to a whole lot of code to be
>> clumped up in qcom-cpuidle.c and duplicated for hotplug again.  It is
>> beneficial to have them this way. Bear with me, as I build up this
>> framework to support the idle and hotplug idle framework.
>>
>
>Yes I'm not quite sure how we would handle adding the regulator code. In
>some cases each CPU's SAW is controlling a regulator and in other cases
>there's only the L2 SAW controlling a regulator. We would need these
>regulators to exist even if we don't support cpuidle, so we'd need to
>register it somehow. I was wondering if we shouldn't be using the
>register in the SAW to change the voltage, instead we should go directly
>to the PMIC and write the regulator driver on top of the PMIC interface.
>The spm code could then look at those regulators to figure out what
>voltage the supply is configured to so that it can be programmed into
>the SPM and used during any power restoring activities. I'm not sure if
>we can even write the PMIC registers though, or if those registers are
>locked down thus requiring us to use the SPM interface to change
>voltages. It would be nice to split this out somehow though. Another
>idea would be to make some shim driver that creates two child devices
>for the regulator and cpuidle drivers and have the shim driver make a
>regmap that both drivers use.
>
There is not just one SPM in the system. If you can split between the
regulator and idle functionality of the same hardware, imaging having
10s of such SPMs that you need to manage. Like you mention, the
functionality of an instance of SPM varies depending on the SoC
architecture. And to keep a track of which SPM changes the voltage is
not functionality that you want the regulator code to remember.  Today
you might get by, bit banging or I2C or SPMI write to the PMIC, but it
inevitable in the future and it would unacceptably inefficient to do so.

>>>>
>>>> diff --git a/drivers/soc/qcom/spm.c b/drivers/soc/qcom/spm.c
>>>> new file mode 100644
>>>> index 0000000..19b79d0
>>>> --- /dev/null
>>>> +++ b/drivers/soc/qcom/spm.c
>>>> @@ -0,0 +1,195 @@
>>> [..]
>>>> +
>>>> +static void flush_shadow(struct msm_spm_driver_data *drv,
>>>> +        unsigned int reg_index)
>>>> +{
>>>> +    __raw_writel(drv->reg_shadow[reg_index],
>>>> +            drv->reg_base_addr + drv->reg_offsets[reg_index]);
>>>> +}
>>>
>>> What's the use of the "shadow" functionality? Why can't we just read and
>>> write the registers directly without having to go through a register
>>> cache?
>> Helps speed up reads and write, when you have multiple writes. Also, is
>> an excellent mechanism to know the state SPM was configured to, in the
>> event of a mishap.
>
>Huh? How can writing something into memory and then writing it to the
>device be faster than writing it directly to the device? If we need to
>know how the SPM was configured I assume we would know by printks or by
>inspecting the code. I still don't see a reason for this.
>
It is if you dont flush after every write. There are explicit flushes to
help flush the whole register set, whenever many elements in the set are
modified. printks/traces are not very helpful when the log buffer is
not completely written in the power down/up path. When you dont
have a running kernel, looking at system state and predicting what could
have happened to crash the system or not jump back into warmboot entry
point, is very tough. This is even more bare than the bare minimum of
code that you need to debug and maintain a PM driver on a production device.

>>>> +
>>>> +static void load_shadow(struct msm_spm_driver_data *drv,
>>>> +        unsigned int reg_index)
>>>> +{
>>>> +    drv->reg_shadow[reg_index] = __raw_readl(drv->reg_base_addr +
>>>> +                        drv->reg_offsets[reg_index]);
>>>
>>> Please use readl_relaxed/writel_relaxed. The raw accessors don't
>>> byte-swap and fail horribly for big-endian kernels.
>> OK. But does it matter that the SoC the code is expected to run is
>> little-endian?
>
>big-endian kernels work on these platforms. Nobody is extensively
>testing it but putting down more roadblocks to using it isn't helpful.
>
Fair enough. I will change the functions.

Thanks for your time.

Lina

>-- 
>Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
>hosted by The Linux Foundation
>

[PATCH v4 4/8] qcom: spm-devices: Add SPM device manager for the SoC

[Stephen Boyd]

On 08/25/14 17:31, Lina Iyer wrote:
> On Mon, Aug 25, 2014 at 04:40:33PM -0700, Stephen Boyd wrote:
>> On 08/19/14 15:15, Lina Iyer wrote:
>>> diff --git a/Documentation/devicetree/bindings/arm/msm/spm.txt
>>> b/Documentation/devicetree/bindings/arm/msm/spm.txt
>>> new file mode 100644
>>> index 0000000..318e024
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/arm/msm/spm.txt
>>
>> We already have a binding document for SAW. Please add stuff there
>> because SPM is just a component of SAW.
>>
> I agree that SPM is just a component of the SAW. But the document there
> seems to indicate regulator details, totally unrelated to the actual SAW
> hardware functionality.

Huh? The SAW binding should be extended with whatever properties are
necessary. Probably the only thing we need is the delays. Everything
else can be determined from the compatible?

SAW has a connection to the PMIC, does it not? If it isn't directly
connected we can come up with a different name for the node, but just
because the node name in the example is misleading doesn't mean we
should completely disregard what we already have.

>
>>> @@ -0,0 +1,47 @@
>>> +* Subsystem Power Manager (SAW2)
>>> +
>>> +S4 generation of MSMs have SPM hardware blocks to control the
>>> Application
>>> +Processor Sub-System power. These SPM blocks run individual state
>>> machine
>>> +to determine what the core (L2 or Krait/Scorpion) would do when the
>>> WFI
>>> +instruction is executed by the core.
>>> +
>>> +The devicetree representation of the SPM block should be:
>>> +
>>> +Required properties
>>> +
>>> +- compatible: Could be one of -
>>> +        "qcom,spm-v2.1"
>>> +        "qcom,spm-v3.0"
>>> +- reg: The physical address and the size of the SPM's memory mapped
>>> registers
>>> +- qcom,cpu: phandle for the CPU that the SPM block is attached to.
>>> +    This field is required on only for SPMs that control the CPU.
>>
>> We have a phandle from the CPU/L2 to the SAW, so this isn't necessary.
>>
> Sorry, I dont understand. Care to explain pls? Its necessary to know
> what SPM instance controls which CPU or L2, so as to pick the right SPM
> device to configure.

We have a phandle in the CPU nodes pointing to the SAW that is
associated with that CPU (qcom,saw). SPM is a part of a SAW. Thus there
is no need for this qcom,cpu property once the SAW node is used.
Instead, we can search the CPU and cache nodes for a qcom,saw that
matches the of_node for the platform device we're probing.

>
>>> +- qcom,saw2-cfg: SAW2 configuration register
>>
>> Why? Compatible should indicate where this register is.
>>
> There are multiple versions of saw handled by the same driver and
> distinguished by the version register. These SAW revisions differ in the
> register offset organization. The variable holds the value to be
> configured in the saw2-cfg register. I will update the documentation to
> be more clear.
>>> +- qcom,saw2-spm-ctl: The SPM control register
>>
>> Why? Compatible should indicate where this register is.
>>
> See above. 

Ah this is more register jam table in DT? cfg should probably be
described in desired clock rate and then the driver can figure out the
value by multiplying that the input clock rate. spm-ctl looks like it's
usually used to describe "enable", which seems rather obvious. Why isn't
the driver always writing the enable bit (bit 0)?

>
>>> +- qcom,saw2-spm-dly: Provides the values for the SPM delay command
>>> in the SPM
>>> +    sequence
>>
>> This is actually three values packed into one register for three
>> different selectable delays, right? We don't typically do register jam
>> tables in DT. Perhaps it should be split out into 3 different
>> properties. Or maybe it shouldn't be specified in DT at all and should
>> be determined algorithmically from the command sequences? From what I
>> can tell most of the sequences don't even use these delays.
>>
> Not at all sequences use the delays. These cannot be determined
> algorithmatically, They may be added to the sequence for changes in
> hardware. Let me revisit the sequences to see if they need to be set
> with the current sequence in use.
>

I was thinking perhaps these should be more structured binary blobs that
indicate the delays that would be necessary in the first 3 bytes or
something and then the command sequence after that.

        <delay1> <delay2> <delay3> <sequence>

Or perhaps

        <num delays=N> <delayN-1> <delayN> <sequence>

and then the code would parse these first few bytes and compress them
into 3 values that are written into the register.

BTW, I wonder if these sequences should be firmware blobs? Or at least,
different files that we then /incbin/ into the final DT blob (if DT
reviewers approve putting blobs like this into the kernel, Cc'ed the DT
list just in case).

>
>
>>> +
>>> +Optional properties
>>> +
>>> +- qcom,saw2-spm-cmd-wfi: The WFI command sequence
>>> +- qcom,saw2-spm-cmd-ret: The Retention command sequence
>>> +- qcom,saw2-spm-cmd-spc: The Standalone PC command sequence
>>> +- qcom,saw2-spm-cmd-pc: The Power Collapse command sequence. This
>>> sequence may
>>> +    turn off other SoC components.
>>> +- qcom,saw2-spm-cmd-gdhs: GDHS (Globally Distributed Head Switch)
>>> command
>>> +    sequence. This sequence will retain the memory but turn off the
>>> logic.
>>
>> I wonder if these should be properties of the idle states? That way the
>> driver isn't searching for them by name in DT, instead it knows what
>> state is associated with what sequence that the SPM needs to have
>> programmed.
>>
> I see the relation you are seeing. But its not a property of the idle
> state. Its an SoC specific property that the idle uses to indicate a
> state. Better off lying here. I doubt there would be a good support for
> holding SoC specific stuff in the ARM idle-states nodes.
>
>

What isn't specifically related to the 1) idle state and 2) CPU/L2/etc.
that the idle state is used in? I would think that by pointing to
different idle states from different CPU nodes we could cover all cases.
What is the SoC specific stuff in here?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

[PATCH v4 4/8] qcom: spm-devices: Add SPM device manager for the SoC

[Lina Iyer]

On Mon, Aug 25, 2014 at 07:17:15PM -0700, Stephen Boyd wrote:
> On 08/25/14 17:31, Lina Iyer wrote:
>> On Mon, Aug 25, 2014 at 04:40:33PM -0700, Stephen Boyd wrote:
>>> On 08/19/14 15:15, Lina Iyer wrote:
>>>> diff --git a/Documentation/devicetree/bindings/arm/msm/spm.txt
>>>> b/Documentation/devicetree/bindings/arm/msm/spm.txt
>>>> new file mode 100644
>>>> index 0000000..318e024
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/arm/msm/spm.txt
>>> 
>>> We already have a binding document for SAW. Please add stuff there
>>> because SPM is just a component of SAW.
>>> 
>> I agree that SPM is just a component of the SAW. But the document there
>> seems to indicate regulator details, totally unrelated to the actual SAW
>> hardware functionality.
> 
> Huh? The SAW binding should be extended with whatever properties are
> necessary. Probably the only thing we need is the delays. Everything
> else can be determined from the compatible?
> 
Thats not true. There are many voltage related properties that are yet
to come. Not everything else can be determined from the compatible flag.
The idea behind having SPM nodes in the DT is that different SoCs may
have the same version of SPM but may have to talk to different
components. The delays, the PMIC register and the PMIC data all change
with the components that SPM communicates.

> SAW has a connection to the PMIC, does it not? If it isn't directly
> connected we can come up with a different name for the node, but just
> because the node name in the example is misleading doesn't mean we
> should completely disregard what we already have.
> 
Yes the SAW has a connection to the PMIC. But what the nodes represent
are just regulator nodes and have no bearing on the SAW. You could use
SPMI/I2C writes to convey the same value without having to go through
SAW using those nodes. I have no reasonable understanding as to why they
are called SAW regulators in the first place. They are just regulators.
The mechanism of using SPM to communicate with the PMIC does not exist
in the kernel and is not part of those nodes. So adding to that
confusion wasn't a wise choice for me.

>> 
>>>> @@ -0,0 +1,47 @@
>>>> +* Subsystem Power Manager (SAW2)
>>>> +
>>>> +S4 generation of MSMs have SPM hardware blocks to control the
>>>> Application
>>>> +Processor Sub-System power. These SPM blocks run individual state
>>>> machine
>>>> +to determine what the core (L2 or Krait/Scorpion) would do when the
>>>> WFI
>>>> +instruction is executed by the core.
>>>> +
>>>> +The devicetree representation of the SPM block should be:
>>>> +
>>>> +Required properties
>>>> +
>>>> +- compatible: Could be one of -
>>>> +        "qcom,spm-v2.1"
>>>> +        "qcom,spm-v3.0"
>>>> +- reg: The physical address and the size of the SPM's memory mapped
>>>> registers
>>>> +- qcom,cpu: phandle for the CPU that the SPM block is attached to.
>>>> +    This field is required on only for SPMs that control the CPU.
>>> 
>>> We have a phandle from the CPU/L2 to the SAW, so this isn't necessary.
>>> 
>> Sorry, I dont understand. Care to explain pls? Its necessary to know
>> what SPM instance controls which CPU or L2, so as to pick the right SPM
>> device to configure.
> 
> We have a phandle in the CPU nodes pointing to the SAW that is
> associated with that CPU (qcom,saw). SPM is a part of a SAW. Thus there
> is no need for this qcom,cpu property once the SAW node is used.
> Instead, we can search the CPU and cache nodes for a qcom,saw that
> matches the of_node for the platform device we're probing.
> 
I agree thats doable. The cpu node can point to the SPM node or the
otherway as it is done here. I dont have a strong preference eitherway.
Lets resolve the earlier and we may have a solution for this, in our
hands.

>> 
>>>> +- qcom,saw2-cfg: SAW2 configuration register
>>> 
>>> Why? Compatible should indicate where this register is.
>>> 
>> There are multiple versions of saw handled by the same driver and
>> distinguished by the version register. These SAW revisions differ in the
>> register offset organization. The variable holds the value to be
>> configured in the saw2-cfg register. I will update the documentation to
>> be more clear.
>>>> +- qcom,saw2-spm-ctl: The SPM control register
>>> 
>>> Why? Compatible should indicate where this register is.
>>> 
>> See above.
> 
> Ah this is more register jam table in DT? cfg should probably be
> described in desired clock rate and then the driver can figure out the
> value by multiplying that the input clock rate. spm-ctl looks like it's
> usually used to describe "enable", which seems rather obvious. Why isn't
> the driver always writing the enable bit (bit 0)?
> 
Kumar already had suggested that. I changed the code, but forgot to
update the documentation, which I noticed and amended in my local tree.
Waiting for some more comments and some changes in cpuidle stuff before
I submit the next rev. Thanks.
That said, figuring out the input clock rate and the multiplication is
still unnecessary. There are not much options on the hardware to change
clock rate and different parameters that will work well on a SoC. SPMs
are generic state machines and are designed to support multiple QCOM
SoCs and hence the configuration options. They dont generally run at
different clock rates and different configurations on the same SoCs.

>> 
>>>> +- qcom,saw2-spm-dly: Provides the values for the SPM delay command
>>>> in the SPM
>>>> +    sequence
>>> 
>>> This is actually three values packed into one register for three
>>> different selectable delays, right? We don't typically do register jam
>>> tables in DT. Perhaps it should be split out into 3 different
>>> properties. Or maybe it shouldn't be specified in DT at all and should
>>> be determined algorithmically from the command sequences? From what I
>>> can tell most of the sequences don't even use these delays.
>>> 
>> Not at all sequences use the delays. These cannot be determined
>> algorithmatically, They may be added to the sequence for changes in
>> hardware. Let me revisit the sequences to see if they need to be set
>> with the current sequence in use.
>> 
> 
> I was thinking perhaps these should be more structured binary blobs that
> indicate the delays that would be necessary in the first 3 bytes or
> something and then the command sequence after that.
> 
>        <delay1> <delay2> <delay3> <sequence>
> 
> Or perhaps
> 
>        <num delays=N> <delayN-1> <delayN> <sequence>
> 
> and then the code would parse these first few bytes and compress them
> into 3 values that are written into the register.
> 
Phew..A lot of splits and merge to get the SPM configured. The hardware
specs dictate a certain value, most of the time you just go with that
for that SoC and dont meddle around with these values. Given that, I
wonder how much benefit do we get by these reorganizations and
interpretations? Dont get me wrong, I am not resisting change. I just am
trying to find justification for making changes downstream as well.

> BTW, I wonder if these sequences should be firmware blobs? Or at least,
> different files that we then /incbin/ into the final DT blob (if DT
> reviewers approve putting blobs like this into the kernel, Cc'ed the DT
> list just in case).
> 
I dint know better. Hence this place, I am not sure the
advantages/disadvantages of putting the sequences in firmware blobs. Let
me hunt for some information.

>> 
>> 
>>>> +
>>>> +Optional properties
>>>> +
>>>> +- qcom,saw2-spm-cmd-wfi: The WFI command sequence
>>>> +- qcom,saw2-spm-cmd-ret: The Retention command sequence
>>>> +- qcom,saw2-spm-cmd-spc: The Standalone PC command sequence
>>>> +- qcom,saw2-spm-cmd-pc: The Power Collapse command sequence. This
>>>> sequence may
>>>> +    turn off other SoC components.
>>>> +- qcom,saw2-spm-cmd-gdhs: GDHS (Globally Distributed Head Switch)
>>>> command
>>>> +    sequence. This sequence will retain the memory but turn off the
>>>> logic.
>>> 
>>> I wonder if these should be properties of the idle states? That way the
>>> driver isn't searching for them by name in DT, instead it knows what
>>> state is associated with what sequence that the SPM needs to have
>>> programmed.
>>> 
>> I see the relation you are seeing. But its not a property of the idle
>> state. Its an SoC specific property that the idle uses to indicate a
>> state. Better off lying here. I doubt there would be a good support for
>> holding SoC specific stuff in the ARM idle-states nodes.
>> 
>> 
> 
> What isn't specifically related to the 1) idle state and 2) CPU/L2/etc.
> that the idle state is used in? I would think that by pointing to
> different idle states from different CPU nodes we could cover all cases.
> What is the SoC specific stuff in here?
> 
SPM is specific to SoC. I am not sure if people are open to pointing to
SPM and its sequences from a node thats generic to all ARM cores. If
thats allowed, I will be open to add these to the idle-state nodes, just
not sure what it buys us. There is no commonality to other vendor's
SoCs.

> -- 
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> hosted by The Linux Foundation
>