[v3,3/8] qcom: spm: Add Subsystem Power Manager driver (SAW2)

Message ID 1408400614-45419-4-git-send-email-lina.iyer@linaro.org
State New
Headers show

Commit Message

Lina Iyer Aug. 18, 2014, 10:23 p.m.
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.

Signed-off-by: Praveen Chidambaram <pchidamb@codeaurora.org>
Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
---
 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

Comments

Kumar Gala Aug. 19, 2014, 2:07 p.m. | #1
On Aug 18, 2014, at 5:23 PM, Lina Iyer <lina.iyer@linaro.org> 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@codeaurora.org>
> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
> ---
> 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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lina Iyer Aug. 19, 2014, 3:28 p.m. | #2
On Tue, Aug 19, 2014 at 09:07:51AM -0500, Kumar Gala wrote:
>
>On Aug 18, 2014, at 5:23 PM, Lina Iyer <lina.iyer@linaro.org> 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?
>
Commit message? The start address is just the register's physical
address.
I can definitely add information about the sequences.

>>
>> Signed-off-by: Praveen Chidambaram <pchidamb@codeaurora.org>
>> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
>> ---
>> 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.
This patch is not useful at all without the spm-devices.c patch. Hence,
the separate patches. 
>
>> 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"
>> +d
>> +#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?
They could be, but they could change with each rev of hardware. Any
advantage to the #define other than the memory.
>
>> +
>> +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
>
OK. Its just the building of the address to the position in the
register.
>> +	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.
>
:) Well this file is full of barriers. Barriers + __raw_writel instead
of writel helps club writes togethers and prevents unnecessary flush,
until we need to.
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +void msm_spm_drv_flush_seq_entry(struct msm_spm_driver_data *dev)
>
>Can this be static?
>
Need this externally in spm-devices.c.
More comment below.

>> +{
>> +	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.
>
OK.
>> +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?

I presume, you are talking about the msm_spm_drv_load_shadow() here.
Yes, we could do that. This just seemed cleaner, since we already have a
nice SPM packet around, with all the relevant data. Any advantages, that
I am failing to see?

>>
>> +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?
>
Its used to populate the SPM sequences by spm-devices.c

>> +};
>> +
>> +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 ?
>
Yes. SAW is SPM - AVS Wrapper. The other part of SAW is currently not
implemented, the platform data would hold both and the driver-data would
hold only the SPM aspect of it.
I do agree, the justification doesnt hold water much. I will try and see
if I can use the same structure.

>> +
>> +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?
>
SPM devices may need to flush the sequences in some cpus separately
before setting up the control register.

>> +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@vger.kernel.org
>> 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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
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,
+};
+
+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;
+	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();
+	}
+
+	return 0;
+}
+
+void msm_spm_drv_flush_seq_entry(struct msm_spm_driver_data *dev)
+{
+	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();
+}
+
+/**
+ * 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();
+	msm_spm_drv_load_shadow(dev, MSM_SPM_REG_SAW2_SPM_STS);
+
+	return 0;
+}
+
+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);
+}
+
+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;
+};
+
+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;
+};
+
+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);
+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 */