All of lore.kernel.org
 help / color / mirror / Atom feed
From: Lina Iyer <lina.iyer@linaro.org>
To: Kumar Gala <galak@codeaurora.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:28:45 -0600	[thread overview]
Message-ID: <20140819152845.GD53916@ilina-mac.local> (raw)
In-Reply-To: <6B9F0B5A-9784-442E-9336-A9F894A72A5D@codeaurora.org>

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
>

  reply	other threads:[~2014-08-19 15:28 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-08-18 22:23 [PATCH v3 0/8] QCOM 8074 cpuidle driver Lina Iyer
2014-08-18 22:23 ` [PATCH v3 1/8] msm: scm: Move scm-boot files to drivers/soc and include/soc Lina Iyer
2014-08-27 17:18   ` Kevin Hilman
2014-08-27 18:57     ` Lina Iyer
2014-08-27 20:24       ` Kevin Hilman
2014-08-27 20:31         ` Lina Iyer
2014-08-18 22:23 ` [PATCH v3 2/8] msm: scm: Add SCM warmboot flags for quad core targets Lina Iyer
2014-08-18 22:23 ` [PATCH v3 3/8] qcom: spm: Add Subsystem Power Manager driver (SAW2) Lina Iyer
2014-08-19 14:07   ` Kumar Gala
2014-08-19 15:28     ` Lina Iyer [this message]
2014-08-18 22:23 ` [PATCH v3 4/8] qcom: spm-devices: Add SPM device manager for the SoC Lina Iyer
2014-08-19  9:29   ` Pramod Gurav
2014-08-19 14:51     ` Lina Iyer
2014-08-18 22:23 ` [PATCH v3 5/8] arm: dts: qcom: Add SPM device bindings for 8974 Lina Iyer
2014-08-18 22:23 ` [PATCH v3 6/8] qcom: msm-pm: Add cpu low power mode functions Lina Iyer
2014-08-18 22:23 ` [PATCH v3 7/8] qcom: cpuidle: Add cpuidle driver for QCOM cpus Lina Iyer
2014-08-19  7:41   ` Pramod Gurav
2014-08-19 14:54     ` Lina Iyer
2014-08-19 15:01       ` Pramod Gurav
2014-08-18 22:23 ` [PATCH v3 8/8] arm: dts: qcom: Add idle states device nodes for 8974 Lina Iyer
2014-08-19 15:30 ` [PATCH v3 0/8] QCOM 8074 cpuidle driver Lina Iyer

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20140819152845.GD53916@ilina-mac.local \
    --to=lina.iyer@linaro.org \
    --cc=daniel.lezcano@linaro.org \
    --cc=davidb@codeaurora.org \
    --cc=galak@codeaurora.org \
    --cc=khilman@linaro.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=lorenzo.pieralisi@arm.com \
    --cc=msivasub@codeauorora.org \
    --cc=pchidamb@codeaurora.org \
    --cc=sboyd@codeaurora.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.