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@xxxxxxxxxxxxxx>
> Signed-off-by: Lina Iyer <lina.iyer@xxxxxxxxxx>
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
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
