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

[Lina Iyer <lina.iyer@linaro.org>]

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.

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.

>
>>
>> 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.
>
>> +
>> +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?
>
>> +}
>> +
>> +static inline void set_start_addr(struct msm_spm_driver_data *drv,
>> +		uint32_t addr)
>> +{
>> +	/* Update bits 10:4 in the SPM CTL register */
>> +	addr &= 0x7F;
>> +	addr <<= 4;
>> +	drv->reg_shadow[MSM_SPM_REG_SAW2_SPM_CTL] &= 0xFFFFF80F;
>> +	drv->reg_shadow[MSM_SPM_REG_SAW2_SPM_CTL] |= addr;
>> +}
>> +
>> +int msm_spm_drv_set_low_power_mode(struct msm_spm_driver_data *drv,
>> +		uint32_t mode)
>> +{
>> +	int i;
>> +	uint32_t start_addr = 0;
>> +
>> +	for (i = 0; i < drv->num_modes; i++) {
>> +		if (drv->modes[i].mode == mode) {
>> +			start_addr = drv->modes[i].start_addr;
>> +			break;
>> +		}
>> +	}
>> +
>> +	if (i == drv->num_modes)
>> +		return -EINVAL;
>> +
>> +	set_start_addr(drv, start_addr);
>> +	flush_shadow(drv, MSM_SPM_REG_SAW2_SPM_CTL);
>> +	/* Barrier to ensure we have written the start address */
>> +	wmb();
>> +
>> +	/* Update our shadow with the status changes, if any */
>> +	load_shadow(drv, MSM_SPM_REG_SAW2_SPM_STS);
>> +
>> +	return 0;
>> +}
>> +
>> +int msm_spm_drv_set_spm_enable(struct msm_spm_driver_data *drv, bool enable)
>> +{
>> +	uint32_t value = enable ? 0x01 : 0x00;
>
>I would prefer u32/u8 instead of uint32_t. Makes lines shorter and
>easier to read.
>
Hmm.. okay

>> +
>> +	/* Update BIT(0) of SPM_CTL to enable/disable the SPM */
>
>This comment could be replaced with code that's more english-like.
Ok.

>
>    #define SPM_CTL_ENABLE BIT(0)
>
>    if ((drv->reg_shadow[MSM_SPM_REG_SAW2_SPM_CTL] & SPM_CTL_ENABLE) !=
>value)
>
>> +	if ((drv->reg_shadow[MSM_SPM_REG_SAW2_SPM_CTL] & 0x01) ^ value) {
>> +		/* Clear the existing value  and update */
>> +		drv->reg_shadow[MSM_SPM_REG_SAW2_SPM_CTL] &= ~0x1;
>> +		drv->reg_shadow[MSM_SPM_REG_SAW2_SPM_CTL] |= value;
>> +		flush_shadow(drv, MSM_SPM_REG_SAW2_SPM_CTL);
>> +		/* Ensure we have enabled/disabled before returning */
>> +		wmb();
>> +	}
>> +
>> +	return 0;
>
>Why have a return value at all?
>
Well, it was intended to be an export function. Made sense that way that the
caller may expect an error. if the hardware was not intialized. I
removed that code here, but did not take out the return value.

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