On Aug 18, 2014, at 5:23 PM, Lina Iyer <lina.iyer@xxxxxxxxxx> 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.
>
> Allow drivers to configure the idle mode for the cores in the SPM start
> address register.
What is the ‘start address’?
Can we add anything about ‘start address’ and the sequences in the commit message?
>
> Signed-off-by: Praveen Chidambaram <pchidamb@xxxxxxxxxxxxxx>
> Signed-off-by: Lina Iyer <lina.iyer@xxxxxxxxxx>
> ---
> drivers/soc/qcom/Makefile | 1 +
> drivers/soc/qcom/spm.c | 176 ++++++++++++++++++++++++++++++++++++++++++
> drivers/soc/qcom/spm_driver.h | 85 ++++++++++++++++++++
> 3 files changed, 262 insertions(+)
> create mode 100644 drivers/soc/qcom/spm.c
> create mode 100644 drivers/soc/qcom/spm_driver.h
>
> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
> index 70d52ed..20b329f 100644
> --- a/drivers/soc/qcom/Makefile
> +++ b/drivers/soc/qcom/Makefile
> @@ -1,3 +1,4 @@
> obj-$(CONFIG_QCOM_GSBI) += qcom_gsbi.o
> +obj-$(CONFIG_QCOM_PM) += spm.o
> CFLAGS_scm.o :=$(call as-instr,.arch_extension sec,-DREQUIRES_SEC=1)
> obj-$(CONFIG_QCOM_SCM) += scm.o scm-boot.o
We should have the Kconfig that adds QCOM_PM as part of this patch so it actual is buildable.
> diff --git a/drivers/soc/qcom/spm.c b/drivers/soc/qcom/spm.c
> new file mode 100644
> index 0000000..3e1de43
> --- /dev/null
> +++ b/drivers/soc/qcom/spm.c
> @@ -0,0 +1,176 @@
> +/* Copyright (c) 2011-2014, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/delay.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/slab.h>
> +
> +#include "spm_driver.h"
> +
> +#define NUM_SPM_ENTRY 32
> +
> +static uint32_t msm_spm_reg_offsets_saw2_v2_1[MSM_SPM_REG_NR] = {
> + [MSM_SPM_REG_SAW2_SECURE] = 0x00,
> + [MSM_SPM_REG_SAW2_ID] = 0x04,
> + [MSM_SPM_REG_SAW2_CFG] = 0x08,
> + [MSM_SPM_REG_SAW2_SPM_STS] = 0x0C,
> + [MSM_SPM_REG_SAW2_AVS_STS] = 0x10,
> + [MSM_SPM_REG_SAW2_PMIC_STS] = 0x14,
> + [MSM_SPM_REG_SAW2_RST] = 0x18,
> + [MSM_SPM_REG_SAW2_VCTL] = 0x1C,
> + [MSM_SPM_REG_SAW2_AVS_CTL] = 0x20,
> + [MSM_SPM_REG_SAW2_AVS_LIMIT] = 0x24,
> + [MSM_SPM_REG_SAW2_AVS_DLY] = 0x28,
> + [MSM_SPM_REG_SAW2_AVS_HYSTERESIS] = 0x2C,
> + [MSM_SPM_REG_SAW2_SPM_CTL] = 0x30,
> + [MSM_SPM_REG_SAW2_SPM_DLY] = 0x34,
> + [MSM_SPM_REG_SAW2_PMIC_DATA_0] = 0x40,
> + [MSM_SPM_REG_SAW2_PMIC_DATA_1] = 0x44,
> + [MSM_SPM_REG_SAW2_PMIC_DATA_2] = 0x48,
> + [MSM_SPM_REG_SAW2_PMIC_DATA_3] = 0x4C,
> + [MSM_SPM_REG_SAW2_PMIC_DATA_4] = 0x50,
> + [MSM_SPM_REG_SAW2_PMIC_DATA_5] = 0x54,
> + [MSM_SPM_REG_SAW2_PMIC_DATA_6] = 0x58,
> + [MSM_SPM_REG_SAW2_PMIC_DATA_7] = 0x5C,
> + [MSM_SPM_REG_SAW2_SEQ_ENTRY] = 0x80,
> + [MSM_SPM_REG_SAW2_VERSION] = 0xFD0,
> +};
Why do we need an array for these offsets, can’t they just be #defines?
> +
> +static void msm_spm_drv_flush_shadow(struct msm_spm_driver_data *dev,
> + unsigned int reg_index)
> +{
> + __raw_writel(dev->reg_shadow[reg_index],
> + dev->reg_base_addr + dev->reg_offsets[reg_index]);
> +}
> +
> +static void msm_spm_drv_load_shadow(struct msm_spm_driver_data *dev,
> + unsigned int reg_index)
> +{
> + dev->reg_shadow[reg_index] = __raw_readl(dev->reg_base_addr +
> + dev->reg_offsets[reg_index]);
> +}
> +
> +static inline void msm_spm_drv_set_start_addr(
> + struct msm_spm_driver_data *dev, uint32_t addr)
> +{
> + addr &= 0x7F;
> + addr <<= 4;
possibly worth a comment why we are shifting addr
> + dev->reg_shadow[MSM_SPM_REG_SAW2_SPM_CTL] &= 0xFFFFF80F;
> + dev->reg_shadow[MSM_SPM_REG_SAW2_SPM_CTL] |= addr;
> +}
> +
> +inline int msm_spm_drv_set_spm_enable(
> + struct msm_spm_driver_data *dev, bool enable)
> +{
> + uint32_t value = enable ? 0x01 : 0x00;
> +
> + if ((dev->reg_shadow[MSM_SPM_REG_SAW2_SPM_CTL] & 0x01) ^ value) {
> + dev->reg_shadow[MSM_SPM_REG_SAW2_SPM_CTL] &= ~0x1;
> + dev->reg_shadow[MSM_SPM_REG_SAW2_SPM_CTL] |= value;
> + msm_spm_drv_flush_shadow(dev, MSM_SPM_REG_SAW2_SPM_CTL);
> + wmb();
typically barriers should have comments about why they are needed.
> + }
> +
> + return 0;
> +}
> +
> +void msm_spm_drv_flush_seq_entry(struct msm_spm_driver_data *dev)
Can this be static?
> +{
> + int i;
> +
> + for (i = 0; i < NUM_SPM_ENTRY; i++) {
> + __raw_writel(dev->reg_seq_entry_shadow[i],
> + dev->reg_base_addr
> + + dev->reg_offsets[MSM_SPM_REG_SAW2_SEQ_ENTRY]
> + + 4 * i);
> + }
> + mb();
again, comment about barrier usage.
> +}
> +
> +/**
> + * msm_spm_drv_write_seq_data - Load the SPM register with sequences
> + *
> + * @dev - The SPM device whose sequences to be programmed
> + * @cmd - The byte array
> + * @offset - The last written byte position, to continue from.
> + */
> +int msm_spm_drv_write_seq_data(struct msm_spm_driver_data *dev,
> + uint8_t *cmd, uint32_t *offset)
> +{
> + uint32_t cmd_w;
> + uint32_t offset_w = *offset / 4;
> + uint8_t last_cmd;
> +
> + while (1) {
> + int i;
> +
> + cmd_w = 0;
> + last_cmd = 0;
> + cmd_w = dev->reg_seq_entry_shadow[offset_w];
> +
> + for (i = (*offset % 4); i < 4; i++) {
> + last_cmd = *(cmd++);
> + cmd_w |= last_cmd << (i * 8);
> + (*offset)++;
> + if (last_cmd == 0x0f)
> + break;
> + }
> +
> + dev->reg_seq_entry_shadow[offset_w++] = cmd_w;
> + if (last_cmd == 0x0f)
> + break;
> + }
> +
> + return 0;
> +}
> +
> +int msm_spm_drv_set_low_power_mode(struct msm_spm_driver_data *dev,
> + uint32_t addr)
> +{
> +
> + msm_spm_drv_set_start_addr(dev, addr);
> + msm_spm_drv_flush_shadow(dev, MSM_SPM_REG_SAW2_SPM_CTL);
> + wmb();
comment on barrier
> + msm_spm_drv_load_shadow(dev, MSM_SPM_REG_SAW2_SPM_STS);
> +
> + return 0;
> +}
> +
We should add a comment about what reinit is for.
> +void msm_spm_drv_reinit(struct msm_spm_driver_data *dev)
> +{
> + int i;
> +
> + msm_spm_drv_flush_seq_entry(dev);
> +
> + for (i = MSM_SPM_REG_NR_INITIALIZE + 1; i < MSM_SPM_REG_NR; i++)
> + msm_spm_drv_load_shadow(dev, i);
> +}
> +
can we have the caller either use msm_spm_driver_data or just pass in reg_base_addr & reg_init_values as args to the function?
>
> +int msm_spm_drv_init(struct msm_spm_driver_data *dev,
> + struct msm_spm_platform_data *data)
> +{
> + dev->reg_base_addr = data->reg_base_addr;
> + memcpy(dev->reg_shadow, data->reg_init_values,
> + sizeof(data->reg_init_values));
> + dev->reg_offsets = msm_spm_reg_offsets_saw2_v2_1;
> + dev->reg_seq_entry_shadow = kcalloc(NUM_SPM_ENTRY,
> + sizeof(*dev->reg_seq_entry_shadow),
> + GFP_KERNEL);
> + if (!dev->reg_seq_entry_shadow)
> + return -ENOMEM;
> +
> + return 0;
> +}
> diff --git a/drivers/soc/qcom/spm_driver.h b/drivers/soc/qcom/spm_driver.h
> new file mode 100644
> index 0000000..6ff69bb
> --- /dev/null
> +++ b/drivers/soc/qcom/spm_driver.h
> @@ -0,0 +1,85 @@
> +/* Copyright (c) 2011-2014, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +#ifndef __QCOM_SPM_DRIVER_H
> +#define __QCOM_SPM_DRIVER_H
> +
> +enum {
> + MSM_SPM_REG_SAW2_CFG,
> + MSM_SPM_REG_SAW2_AVS_CTL,
> + MSM_SPM_REG_SAW2_AVS_HYSTERESIS,
> + MSM_SPM_REG_SAW2_SPM_CTL,
> + MSM_SPM_REG_SAW2_PMIC_DLY,
> + MSM_SPM_REG_SAW2_AVS_LIMIT,
> + MSM_SPM_REG_SAW2_AVS_DLY,
> + MSM_SPM_REG_SAW2_SPM_DLY,
> + MSM_SPM_REG_SAW2_PMIC_DATA_0,
> + MSM_SPM_REG_SAW2_PMIC_DATA_1,
> + MSM_SPM_REG_SAW2_PMIC_DATA_2,
> + MSM_SPM_REG_SAW2_PMIC_DATA_3,
> + MSM_SPM_REG_SAW2_PMIC_DATA_4,
> + MSM_SPM_REG_SAW2_PMIC_DATA_5,
> + MSM_SPM_REG_SAW2_PMIC_DATA_6,
> + MSM_SPM_REG_SAW2_PMIC_DATA_7,
> + MSM_SPM_REG_SAW2_RST,
> +
> + MSM_SPM_REG_NR_INITIALIZE = MSM_SPM_REG_SAW2_RST,
> +
> + MSM_SPM_REG_SAW2_ID,
> + MSM_SPM_REG_SAW2_SECURE,
> + MSM_SPM_REG_SAW2_STS0,
> + MSM_SPM_REG_SAW2_STS1,
> + MSM_SPM_REG_SAW2_STS2,
> + MSM_SPM_REG_SAW2_VCTL,
> + MSM_SPM_REG_SAW2_SEQ_ENTRY,
> + MSM_SPM_REG_SAW2_SPM_STS,
> + MSM_SPM_REG_SAW2_AVS_STS,
> + MSM_SPM_REG_SAW2_PMIC_STS,
> + MSM_SPM_REG_SAW2_VERSION,
> +
> + MSM_SPM_REG_NR,
> +};
> +
> +struct msm_spm_seq_entry {
> + uint32_t mode;
> + uint8_t *cmd;
> +};
> +
> +struct msm_spm_platform_data {
> + void __iomem *reg_base_addr;
> + uint32_t reg_init_values[MSM_SPM_REG_NR_INITIALIZE];
> +
> + uint32_t num_modes;
> + struct msm_spm_seq_entry *modes;
Are modes used? is msm_spm_seq_entry used anywhere?
> +};
> +
> +struct msm_spm_driver_data {
> + void __iomem *reg_base_addr;
> + uint32_t reg_shadow[MSM_SPM_REG_NR];
> + uint32_t *reg_seq_entry_shadow;
> + uint32_t *reg_offsets;
> + uint32_t num_spm_entry;
> +};
Is there a reason we need both platform_dta & driver_data ?
> +
> +int msm_spm_drv_init(struct msm_spm_driver_data *dev,
> + struct msm_spm_platform_data *data);
> +void msm_spm_drv_reinit(struct msm_spm_driver_data *dev);
> +int msm_spm_drv_set_low_power_mode(struct msm_spm_driver_data *dev,
> + uint32_t addr);
> +int msm_spm_drv_write_seq_data(struct msm_spm_driver_data *dev,
> + uint8_t *cmd, uint32_t *offset);
> +void msm_spm_drv_flush_seq_entry(struct msm_spm_driver_data *dev);
does flush need to be exposed?
> +int msm_spm_drv_set_spm_enable(struct msm_spm_driver_data *dev,
> + bool enable);
> +void msm_spm_reinit(void);
> +int msm_spm_init(struct msm_spm_platform_data *data, int nr_devs);
> +
> +#endif /* __QCOM_SPM_DRIVER_H */
> --
> 1.9.1
>
> --
> 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
--
Employee of Qualcomm Innovation Center, Inc.
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
