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

From: Kumar Gala <galak@codeaurora.org>
To: Lina Iyer <lina.iyer@linaro.org>
Cc: daniel.lezcano@linaro.org, khilman@linaro.org,
	sboyd@codeaurora.org, davidb@codeaurora.org,
	linux-arm-msm@vger.kernel.org, lorenzo.pieralisi@arm.com,
	msivasub@codeauorora.org,
	Praveen Chidambaram <pchidamb@codeaurora.org>
Subject: Re: [PATCH v3 3/8] qcom: spm: Add Subsystem Power Manager driver (SAW2)
Date: Tue, 19 Aug 2014 09:07:51 -0500	[thread overview]
Message-ID: <6B9F0B5A-9784-442E-9336-A9F894A72A5D@codeaurora.org> (raw)
In-Reply-To: <1408400614-45419-4-git-send-email-lina.iyer@linaro.org>

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

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