From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stephen Boyd Subject: Re: [PATCH v4 3/8] qcom: spm: Add Subsystem Power Manager driver (SAW2) Date: Tue, 19 Aug 2014 19:01:53 -0700 Message-ID: <53F40191.5080106@codeaurora.org> References: <1408486537-6358-1-git-send-email-lina.iyer@linaro.org> <1408486537-6358-4-git-send-email-lina.iyer@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Return-path: Received: from smtp.codeaurora.org ([198.145.11.231]:36179 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751733AbaHTCBz (ORCPT ); Tue, 19 Aug 2014 22:01:55 -0400 In-Reply-To: <1408486537-6358-4-git-send-email-lina.iyer@linaro.org> Sender: linux-arm-msm-owner@vger.kernel.org List-Id: linux-arm-msm@vger.kernel.org To: Lina Iyer Cc: daniel.lezcano@linaro.org, khilman@linaro.org, davidb@codeaurora.org, galak@codeaurora.org, linux-arm-msm@vger.kernel.org, lorenzo.pieralisi@arm.com, msivasub@codeaurora.org, Praveen Chidambaram 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 > Signed-off-by: Lina Iyer 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