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

[Stephen Boyd <sboyd@codeaurora.org>]

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.

>
> 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?

> +
> +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.

> +}
> +
> +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.

> +
> +	/* Update BIT(0) of SPM_CTL to enable/disable the SPM */

This comment could be replaced with code that's more english-like.

    #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?

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