Re: [RESEND PATCH v2 2/3] cpufreq: brcmstb-avs-cpufreq: AVS CPUfreq driver for Broadcom STB SoCs
From: Viresh Kumar
Date: Tue Oct 04 2016 - 23:25:37 EST
On 30-09-16, 14:56, Markus Mayer wrote:
> This driver supports voltage and frequency scaling on Broadcom STB SoCs
> using AVS firmware with DFS and DVFS support.
>
> Actual frequency or voltage scaling is done exclusively by the AVS
> firmware. The driver merely provides a standard CPUfreq interface to
> other kernel components and userland, and instructs the AVS firmware to
> perform frequency or voltage changes on its behalf.
>
> Signed-off-by: Markus Mayer <mmayer@xxxxxxxxxxxx>
> ---
> Documentation/cpu-freq/brcmstb-avs-cpufreq.txt | 27 +
> MAINTAINERS | 2 +
> drivers/cpufreq/Kconfig.arm | 10 +
> drivers/cpufreq/Makefile | 1 +
> drivers/cpufreq/brcmstb-avs-cpufreq.c | 707 +++++++++++++++++++++++++
> 5 files changed, 747 insertions(+)
> create mode 100644 Documentation/cpu-freq/brcmstb-avs-cpufreq.txt
> create mode 100644 drivers/cpufreq/brcmstb-avs-cpufreq.c
>
> diff --git a/Documentation/cpu-freq/brcmstb-avs-cpufreq.txt b/Documentation/cpu-freq/brcmstb-avs-cpufreq.txt
> new file mode 100644
> index 0000000..c6503e1
> --- /dev/null
> +++ b/Documentation/cpu-freq/brcmstb-avs-cpufreq.txt
> @@ -0,0 +1,27 @@
> +Broadcom STB AVS CPUfreq driver
> +===============================
> +
> +"AVS" is the name of a firmware developed at Broadcom. It derives its
> +name from the technique called "Adaptive Voltage Scaling". Adaptive
> +voltage scaling was the original purpose of this firmware. The AVS
> +firmware still supports "AVS mode", where all it does is adaptive
> +voltage scaling. However, on some newer Broadcom SoCs, the AVS
> +Firmware, despite its unchanged name, also supports DFS mode and DVFS
> +mode.
> +
> +In the context of this document and the related driver, "AVS" by itself
> +always means the Broadcom firmware and never refers to the technique
> +called "Adaptive Voltage Scaling".
> +
> +The Broadcom STB AVS CPUfreq driver provides voltage and frequency
> +scaling on Broadcom SoCs using AVS firmware with support for DFS and
> +DVFS. The AVS firmware is running on its own co-processor. The driver
> +supports both uniprocessor (UP) and symmetric multiprocessor (SMP)
> +systems which share clock and voltage across all CPUs.
> +
> +Actual voltage and frequency scaling is done solely by the AVS firmware.
> +This driver does not change frequency or voltage itself. It provides a
> +standard CPUfreq interface to the rest of the kernel and to userland. It
> +interfaces with the AVS firmware to effect the requested changes and to
> +report back the current system status in a way that is expected by existing
> +tools.
Do we really need this file? Can't part of it go to the binding
document? I am not sure I see any useful information here which is
mandatory. If the DT file doesn't work well, a big comment at the top
of the driver's .c file should do..
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 557f839..708b2bc 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2695,6 +2695,8 @@ M: bcm-kernel-feedback-list@xxxxxxxxxxxx
> L: linux-pm@xxxxxxxxxxxxxxx
> S: Maintained
> F: Documentation/devicetree/bindings/cpufreq/brcm,stb-avs-cpu-freq.txt
> +F: Documentation/cpu-freq/brcmstb-avs-cpufreq.txt
> +F: drivers/cpufreq/brcm-avs-cpufreq.c
>
> BROADCOM SPECIFIC AMBA DRIVER (BCMA)
> M: RafaÅ MiÅecki <zajec5@xxxxxxxxx>
> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> index d89b8af..4177f45 100644
> --- a/drivers/cpufreq/Kconfig.arm
> +++ b/drivers/cpufreq/Kconfig.arm
> @@ -12,6 +12,16 @@ config ARM_BIG_LITTLE_CPUFREQ
> help
> This enables the Generic CPUfreq driver for ARM big.LITTLE platforms.
>
> +config ARM_BRCMSTB_AVS_CPUFREQ
> + tristate "Broadcom STB AVS CPUfreq driver"
> + depends on ARCH_BRCMSTB || COMPILE_TEST
> + help
> + Some Broadcom STB SoCs use a co-processor running proprietary firmware
> + ("AVS") to handle voltage and frequency scaling. This driver provides
> + a standard CPUfreq interface to to the firmware.
> +
> + Say Y, if you have a Broadcom SoC with AVS support for DFS or DVFS.
> +
> config ARM_DT_BL_CPUFREQ
> tristate "Generic probing via DT for ARM big LITTLE CPUfreq driver"
> depends on ARM_BIG_LITTLE_CPUFREQ && OF
> diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
> index 0a9b6a09..ac54f64 100644
> --- a/drivers/cpufreq/Makefile
> +++ b/drivers/cpufreq/Makefile
> @@ -51,6 +51,7 @@ obj-$(CONFIG_ARM_BIG_LITTLE_CPUFREQ) += arm_big_little.o
> # LITTLE drivers, so that it is probed last.
> obj-$(CONFIG_ARM_DT_BL_CPUFREQ) += arm_big_little_dt.o
>
> +obj-$(CONFIG_ARM_BRCMSTB_AVS_CPUFREQ) += brcmstb-avs-cpufreq.o
> obj-$(CONFIG_ARCH_DAVINCI) += davinci-cpufreq.o
> obj-$(CONFIG_UX500_SOC_DB8500) += dbx500-cpufreq.o
> obj-$(CONFIG_ARM_EXYNOS5440_CPUFREQ) += exynos5440-cpufreq.o
> diff --git a/drivers/cpufreq/brcmstb-avs-cpufreq.c b/drivers/cpufreq/brcmstb-avs-cpufreq.c
> new file mode 100644
> index 0000000..ddf5dd1
> --- /dev/null
> +++ b/drivers/cpufreq/brcmstb-avs-cpufreq.c
> @@ -0,0 +1,707 @@
> +/*
> + * CPU frequency scaling for Broadcom SoCs with AVS firmware that
> + * supports DVS or DVFS
> + *
> + * Copyright (c) 2016 Broadcom
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation version 2.
> + *
> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
> + * kind, whether express or implied; without even the implied warranty
> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/kernel.h>
Since you have kept the rest in ascending order, it might be better to
put this one after io.h.
> +#include <linux/cpufreq.h>
> +#include <linux/cpu.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_opp.h>
> +#include <linux/slab.h>
> +
> +/* Max number of arguments AVS calls take */
> +#define AVS_MAX_CMD_ARGS 4
> +/*
> + * This macro is used to generate AVS parameter register offsets. For
> + * x >= AVS_MAX_CMD_ARGS, it returns 0 to protect against accidental memory
> + * access outside of the parameter range. (Offset 0 is the first parameter.)
> + */
> +#define AVS_PARAM_MULT(x) ((x) < AVS_MAX_CMD_ARGS ? (x) : 0)
> +
> +/* AVS Mailbox Register offsets */
> +#define AVS_MBOX_COMMAND 0x00
> +#define AVS_MBOX_STATUS 0x04
> +#define AVS_MBOX_VOLTAGE0 0x08
> +#define AVS_MBOX_TEMP0 0x0c
> +#define AVS_MBOX_PV0 0x10
> +#define AVS_MBOX_MV0 0x14
> +#define AVS_MBOX_PARAM(x) (0x18 + AVS_PARAM_MULT(x) * sizeof(u32))
> +#define AVS_MBOX_REVISION 0x28
> +#define AVS_MBOX_PSTATE 0x2c
> +#define AVS_MBOX_HEARTBEAT 0x30
> +#define AVS_MBOX_MAGIC 0x34
> +#define AVS_MBOX_SIGMA_HVT 0x38
> +#define AVS_MBOX_SIGMA_SVT 0x3c
> +#define AVS_MBOX_VOLTAGE1 0x40
> +#define AVS_MBOX_TEMP1 0x44
> +#define AVS_MBOX_PV1 0x48
> +#define AVS_MBOX_MV1 0x4c
> +#define AVS_MBOX_FREQUENCY 0x50
> +
> +/* AVS Commands */
> +#define AVS_CMD_AVAILABLE 0x00
> +#define AVS_CMD_DISABLE 0x10
> +#define AVS_CMD_ENABLE 0x11
> +#define AVS_CMD_S2_ENTER 0x12
> +#define AVS_CMD_S2_EXIT 0x13
> +#define AVS_CMD_BBM_ENTER 0x14
> +#define AVS_CMD_BBM_EXIT 0x15
> +#define AVS_CMD_S3_ENTER 0x16
> +#define AVS_CMD_S3_EXIT 0x17
> +#define AVS_CMD_BALANCE 0x18
> +/* PMAP and P-STATE commands */
> +#define AVS_CMD_GET_PMAP 0x30
> +#define AVS_CMD_SET_PMAP 0x31
> +#define AVS_CMD_GET_PSTATE 0x40
> +#define AVS_CMD_SET_PSTATE 0x41
> +
> +/* Different modes AVS supports (for GET_PMAP/SET_PMAP) */
> +#define AVS_MODE_AVS 0x0
> +#define AVS_MODE_DFS 0x1
> +#define AVS_MODE_DVS 0x2
> +#define AVS_MODE_DVFS 0x3
> +
> +/*
> + * PMAP parameter p1
> + * unused:31-24, mdiv_p0:23-16, unused:15-14, pdiv:13-10 , ndiv_int:9-0
> + */
> +#define NDIV_INT_SHIFT 0
> +#define NDIV_INT_MASK 0x3ff
> +#define PDIV_SHIFT 10
> +#define PDIV_MASK 0xf
> +#define MDIV_P0_SHIFT 16
> +#define MDIV_P0_MASK 0xff
> +/*
> + * PMAP parameter p2
> + * mdiv_p4:31-24, mdiv_p3:23-16, mdiv_p2:15:8, mdiv_p1:7:0
> + */
> +#define MDIV_P1_SHIFT 0
> +#define MDIV_P1_MASK 0xff
> +#define MDIV_P2_SHIFT 8
> +#define MDIV_P2_MASK 0xff
> +#define MDIV_P3_SHIFT 16
> +#define MDIV_P3_MASK 0xff
> +#define MDIV_P4_SHIFT 24
> +#define MDIV_P4_MASK 0xff
> +
> +/* Different P-STATES AVS supports (for GET_PSTATE/SET_PSTATE) */
> +#define AVS_PSTATE_P0 0x0
> +#define AVS_PSTATE_P1 0x1
> +#define AVS_PSTATE_P2 0x2
> +#define AVS_PSTATE_P3 0x3
> +#define AVS_PSTATE_P4 0x4
> +#define AVS_PSTATE_MAX AVS_PSTATE_P4
> +
> +/* CPU L2 Interrupt Controller Registers */
> +#define AVS_CPU_L2_SET0 0x04
> +#define AVS_CPU_L2_INT_MASK BIT(31)
> +
> +/* AVS Command Status Values */
> +#define AVS_STATUS_CLEAR 0x00
> +/* Command/notification accepted */
> +#define AVS_STATUS_SUCCESS 0xf0
> +/* Command/notification rejected */
> +#define AVS_STATUS_FAILURE 0xff
> +/* Invalid command/notification (unknown) */
> +#define AVS_STATUS_INVALID 0xf1
> +/* Non-AVS modes are not supported */
> +#define AVS_STATUS_NO_SUPP 0xf2
> +/* Cannot set P-State until P-Map supplied */
> +#define AVS_STATUS_NO_MAP 0xf3
> +/* Cannot change P-Map after initial P-Map set */
> +#define AVS_STATUS_MAP_SET 0xf4
> +/* Max AVS status; higher numbers are used for debugging */
> +#define AVS_STATUS_MAX 0xff
> +
> +/* Other AVS related constants */
> +#define AVS_LOOP_LIMIT 10000
> +#define AVS_TIMEOUT 300 /* in ms; expected completion is < 10ms */
> +#define AVS_FIRMWARE_MAGIC 0xa11600d1
> +
> +#define BRCM_AVS_CPUFREQ_NAME "brcmstb-avs-cpufreq"
> +#define BRCM_AVS_CPU_DATA "brcm,avs-cpu-data-mem"
> +#define BRCM_AVS_CPU_INTR "brcm,avs-cpu-l2-intr"
> +#define BRCM_AVS_HOST_INTR "sw_intr"
> +
> +struct pmap {
> + unsigned int mode;
> + unsigned int p1;
> + unsigned int p2;
> + unsigned int state;
> +};
> +
> +struct private_data {
> + void __iomem *base;
> + void __iomem *avs_intr_base;
> + struct device_node *base_np;
> + struct device_node *intr_base_np;
> + struct device *dev;
> + struct completion done;
> + struct semaphore sem;
> + struct pmap pmap;
> +};
> +
> +static void __iomem *__map_region(const char *name, struct device_node **node)
> +{
> + struct device_node *np;
> +
> + np = of_find_compatible_node(NULL, NULL, name);
> + if (!np)
> + return NULL;
> +
> + if (node)
> + *node = np;
> +
> + return of_iomap(np, 0);
> +}
> +
> +static int __issue_avs_command(struct private_data *priv, int cmd, bool is_send,
> + u32 args[])
checkpatch --strict will ask you to align the u32 to the opening ( in
the above line.
> +{
> + unsigned long time_left = msecs_to_jiffies(AVS_TIMEOUT);
> + void __iomem *base = priv->base;
> + unsigned int i;
> + int ret;
> + u32 val;
> +
> + ret = down_interruptible(&priv->sem);
> + if (ret)
> + return ret;
> +
> + /*
> + * Make sure no other command is currently running: cmd is 0 if AVS
> + * co-processor is idle. Due to the guard above, we should almost never
> + * have to wait here.
> + */
> + for (i = 0, val = 1; val != 0 && i < AVS_LOOP_LIMIT; i++)
> + val = readl(base + AVS_MBOX_COMMAND);
Please add a blank line here..
> + /* Give the caller a chance to retry if AVS is busy. */
> + if (i >= AVS_LOOP_LIMIT) {
Isn't == sufficient here ?
> + ret = -EAGAIN;
> + goto out;
> + }
> +
> + /* Clear status before we begin. */
> + writel(AVS_STATUS_CLEAR, base + AVS_MBOX_STATUS);
> +
> + /* We need to send arguments for this command. */
> + if (args && is_send)
Though its not compulsory as per C coding standards, but its better to
use {} for multi-line body..
> + for (i = 0; i < AVS_MAX_CMD_ARGS; i++)
> + writel(args[i], base + AVS_MBOX_PARAM(i));
> +
> + /* Protect from spurious interrupts. */
> + reinit_completion(&priv->done);
Blank line here...
> + /* Now issue the command & tell firmware to wake up to process it. */
> + writel(cmd, base + AVS_MBOX_COMMAND);
> + writel(AVS_CPU_L2_INT_MASK, priv->avs_intr_base + AVS_CPU_L2_SET0);
and here would make it easily readable IMO.
> + /* Wait for AVS co-processor to finish processing the command. */
> + time_left = wait_for_completion_timeout(&priv->done, time_left);
> +
> + /*
> + * If the AVS status is not in the expected range, it means AVS didn't
> + * complete our command in time, and we return an error. Also, if there
> + * is no "time left", we timed out waiting for the interrupt.
> + */
> + val = readl(base + AVS_MBOX_STATUS);
> + if (time_left == 0 || val == 0 || val > AVS_STATUS_MAX) {
> + dev_err(priv->dev, "AVS command %#x didn't complete in time\n",
> + cmd);
> + dev_err(priv->dev, " Time left: %u ms, AVS status: %#x\n",
> + jiffies_to_msecs(time_left), val);
> + ret = -ETIMEDOUT;
> + goto out;
> + }
> +
> + /* This command returned arguments, so we read them back. */
> + if (args && !is_send)
Please use {} ..
> + for (i = 0; i < AVS_MAX_CMD_ARGS; i++)
> + args[i] = readl(base + AVS_MBOX_PARAM(i));
> +
> + /* Clear status to tell AVS co-processor we are done. */
> + writel(AVS_STATUS_CLEAR, base + AVS_MBOX_STATUS);
> +
> + /* Convert firmware errors to errno's as much as possible. */
> + switch (val) {
> + case AVS_STATUS_INVALID:
> + ret = -EINVAL;
> + break;
> + case AVS_STATUS_NO_SUPP:
> + ret = -ENOTSUPP;
> + break;
> + case AVS_STATUS_NO_MAP:
> + ret = -ENOENT;
> + break;
> + case AVS_STATUS_MAP_SET:
> + ret = -EEXIST;
> + break;
> + case AVS_STATUS_FAILURE:
> + ret = -EIO;
> + break;
> + }
> +
> +out:
> + up(&priv->sem);
> +
> + return ret;
> +}
> +
> +static irqreturn_t irq_handler(int irq, void *data)
> +{
> + struct private_data *priv = data;
> +
> + /* AVS command completed execution. Wake up __issue_avs_command(). */
> + complete(&priv->done);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static char *brcm_avs_mode_to_string(unsigned int mode)
> +{
> + switch (mode) {
> + case AVS_MODE_AVS:
> + return "AVS";
> + case AVS_MODE_DFS:
> + return "DFS";
> + case AVS_MODE_DVS:
> + return "DVS";
> + case AVS_MODE_DVFS:
> + return "DVFS";
> + }
> + return NULL;
> +}
> +
> +static void brcm_avs_parse_p1(u32 p1, unsigned int *mdiv_p0, unsigned int *pdiv,
> + unsigned int *ndiv)
> +{
> + *mdiv_p0 = (p1 >> MDIV_P0_SHIFT) & MDIV_P0_MASK;
> + *pdiv = (p1 >> PDIV_SHIFT) & PDIV_MASK;
> + *ndiv = (p1 >> NDIV_INT_SHIFT) & NDIV_INT_MASK;
> +}
> +
> +static void brcm_avs_parse_p2(u32 p2, unsigned int *mdiv_p1,
> + unsigned int *mdiv_p2, unsigned int *mdiv_p3,
> + unsigned int *mdiv_p4)
> +{
> + *mdiv_p4 = (p2 >> MDIV_P4_SHIFT) & MDIV_P4_MASK;
> + *mdiv_p3 = (p2 >> MDIV_P3_SHIFT) & MDIV_P3_MASK;
> + *mdiv_p2 = (p2 >> MDIV_P2_SHIFT) & MDIV_P2_MASK;
> + *mdiv_p1 = (p2 >> MDIV_P1_SHIFT) & MDIV_P1_MASK;
> +}
> +
> +static int brcm_avs_get_pmap(struct private_data *priv, struct pmap *pmap)
> +{
> + u32 args[AVS_MAX_CMD_ARGS];
> + int ret;
> +
> + ret = __issue_avs_command(priv, AVS_CMD_GET_PMAP, false, args);
> + if (ret || !pmap)
> + return ret;
> +
> + pmap->mode = args[0];
> + pmap->p1 = args[1];
> + pmap->p2 = args[2];
> + pmap->state = args[3];
> +
> + return 0;
> +}
> +
> +static int brcm_avs_set_pmap(struct private_data *priv, struct pmap *pmap)
> +{
> + u32 args[AVS_MAX_CMD_ARGS];
> +
> + args[0] = pmap->mode;
> + args[1] = pmap->p1;
> + args[2] = pmap->p2;
> + args[3] = pmap->state;
> +
> + return __issue_avs_command(priv, AVS_CMD_SET_PMAP, true, args);
> +}
> +
> +static int brcm_avs_get_pstate(struct private_data *priv, unsigned int *pstate)
> +{
> + u32 args[AVS_MAX_CMD_ARGS];
> + int ret;
> +
> + ret = __issue_avs_command(priv, AVS_CMD_GET_PSTATE, false, args);
> + if (ret)
> + return ret;
> + *pstate = args[0];
> +
> + return 0;
> +}
> +
> +static int brcm_avs_set_pstate(struct private_data *priv, unsigned int pstate)
> +{
> + u32 args[AVS_MAX_CMD_ARGS];
> +
> + args[0] = pstate;
> + return __issue_avs_command(priv, AVS_CMD_SET_PSTATE, true, args);
> +}
> +
> +static unsigned long brcm_avs_get_voltage(void __iomem *base)
> +{
> + return readl(base + AVS_MBOX_VOLTAGE1);
> +}
> +
> +static unsigned long brcm_avs_get_frequency(void __iomem *base)
> +{
> + return readl(base + AVS_MBOX_FREQUENCY) * 1000; /* in kHz */
> +}
> +
> +/*
> + * We determine which frequencies are supported by cycling through all P-states
> + * and reading back what frequency we are running at for each P-state.
> + */
> +static struct cpufreq_frequency_table *
> +brcm_avs_get_freq_table(struct device *dev, struct private_data *priv)
> +{
> + struct cpufreq_frequency_table *table;
> + unsigned int pstate;
> + int i, ret;
> +
> + /* Remember P-state for later */
> + ret = brcm_avs_get_pstate(priv, &pstate);
> + if (ret)
> + return ERR_PTR(ret);
> +
> + table = devm_kzalloc(dev, (AVS_PSTATE_MAX + 1) * sizeof(*table),
> + GFP_KERNEL);
Same alignment issue here as well. Please fix that for all the
patches.
> + if (!table)
> + return ERR_PTR(-ENOMEM);
> +
> + for (i = AVS_PSTATE_P0; i <= AVS_PSTATE_MAX; i++) {
> + ret = brcm_avs_set_pstate(priv, i);
> + if (ret)
> + return ERR_PTR(ret);
> + table[i].frequency = brcm_avs_get_frequency(priv->base);
> + table[i].driver_data = i;
> + }
> + table[i].frequency = CPUFREQ_TABLE_END;
> + table[i].driver_data = i;
> +
> + /* Restore P-state */
> + ret = brcm_avs_set_pstate(priv, pstate);
> + if (ret)
> + return ERR_PTR(ret);
> +
> + return table;
> +}
> +
> +/*
> + * To ensure the right firmware is running we need to
> + * - check the MAGIC matches what we expect
> + * - brcm_avs_get_pmap() doesn't return -ENOTSUPP or -EINVAL
> + */
> +static bool brcm_avs_is_firmware_loaded(struct private_data *priv)
> +{
> + u32 magic;
> + int rc;
> +
> + rc = brcm_avs_get_pmap(priv, NULL);
> + magic = readl(priv->base + AVS_MBOX_MAGIC);
> +
> + return (magic == AVS_FIRMWARE_MAGIC) && (rc != -ENOTSUPP) &&
> + (rc != -EINVAL);
> +}
> +
> +static unsigned int brcm_avs_cpufreq_get(unsigned int cpu)
> +{
> + struct cpufreq_policy *policy = cpufreq_cpu_get(cpu);
> +
> + return policy->cur;
Can't you read the value right from the hardware? The purpose of this
routine is to get the exact frequency the hardware is running at. And
there are cases when software/hardware aren't in sync.
> +}
> +
> +static int brcm_avs_target_index(struct cpufreq_policy *policy,
> + unsigned int index)
> +{
> + int ret;
> +
> + ret = brcm_avs_set_pstate(policy->driver_data,
> + policy->freq_table[index].driver_data);
> + if (ret)
> + return ret;
> +
> + policy->cur = policy->freq_table[index].frequency;
This isn't supposed to be done by the drivers. Core handles it by
itself.
> +
> + return 0;
> +}
> +
> +static int brcm_avs_suspend(struct cpufreq_policy *policy)
> +{
> + struct private_data *priv = policy->driver_data;
> +
> + return brcm_avs_get_pmap(priv, &priv->pmap);
> +}
> +
> +static int brcm_avs_resume(struct cpufreq_policy *policy)
> +{
> + struct private_data *priv = policy->driver_data;
> + int ret;
> +
> + ret = brcm_avs_set_pmap(priv, &priv->pmap);
> + if (ret == -EEXIST) {
> + struct platform_device *pdev = cpufreq_get_driver_data();
> + struct device *dev = &pdev->dev;
> +
> + dev_warn(dev, "PMAP was already set\n");
> + ret = 0;
> + }
> +
> + return ret;
> +}
> +
> +static int brcm_avs_cpu_init(struct cpufreq_policy *policy)
> +{
> + struct cpufreq_frequency_table *freq_table;
> + struct platform_device *pdev;
> + struct private_data *priv;
> + struct device *dev;
> + int host_irq;
> + int ret;
Maybe merge above two lines.
> +
> + /*
> + * Register on CPU 0 only. If this fails, there is no reason to try on
> + * other cores, since it would just fail again.
> + */
> + if (policy->cpu > 0)
> + return -ENODEV;
This is not the right way of doing it. What if CPU 0 is offline while
this function gets called?
> +
> + pdev = cpufreq_get_driver_data();
> + dev = &pdev->dev;
> +
> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + priv->dev = dev;
> + policy->driver_data = priv;
> + sema_init(&priv->sem, 1);
> + init_completion(&priv->done);
> + platform_set_drvdata(pdev, priv);
> +
> + priv->base = __map_region(BRCM_AVS_CPU_DATA, &priv->base_np);
> + if (!priv->base) {
> + dev_err(dev, "Couldn't find property %s in device tree.\n",
> + BRCM_AVS_CPU_DATA);
> + return -ENOENT;
> + }
> +
> + priv->avs_intr_base = __map_region(BRCM_AVS_CPU_INTR,
> + &priv->intr_base_np);
> + if (!priv->avs_intr_base) {
> + dev_err(dev, "Couldn't find property %s in device tree.\n",
> + BRCM_AVS_CPU_INTR);
> + ret = -ENOENT;
> + goto unmap_base;
> + }
> +
> + host_irq = platform_get_irq_byname(pdev, BRCM_AVS_HOST_INTR);
> + if (host_irq < 0) {
> + dev_err(dev, "Couldn't find interrupt %s -- %d\n",
> + BRCM_AVS_HOST_INTR, host_irq);
> + ret = host_irq;
> + goto unmap_intr_base;
> + }
> + ret = devm_request_irq(dev, host_irq, irq_handler, IRQF_TRIGGER_RISING,
> + BRCM_AVS_HOST_INTR, priv);
> + if (ret) {
> + dev_err(dev, "IRQ request failed: %s (%d) -- %d\n",
> + BRCM_AVS_HOST_INTR, host_irq, ret);
> + goto unmap_intr_base;
> + }
> +
> + if (!brcm_avs_is_firmware_loaded(priv)) {
Shouldn't this be checked before requesting irqs ?
> + dev_err(dev,
> + "AVS firmware is not loaded or doesn't support DVFS\n");
> + ret = -ENODEV;
> + goto unmap_intr_base;
> + }
> +
> + freq_table = brcm_avs_get_freq_table(dev, priv);
> + if (IS_ERR(freq_table)) {
> + dev_err(dev, "Couldn't determine frequency table (%ld).\n",
> + PTR_ERR(freq_table));
> + ret = PTR_ERR(freq_table);
> + goto unmap_intr_base;
> + }
> +
> + ret = cpufreq_table_validate_and_show(policy, freq_table);
> + if (ret) {
> + dev_err(dev, "invalid frequency table: %d\n", ret);
> + goto unmap_intr_base;
> + }
> +
> + /* All cores share the same clock and thus the same policy. */
> + cpumask_setall(policy->cpus);
> +
> + ret = __issue_avs_command(priv, AVS_CMD_ENABLE, false, NULL);
> + if (!ret) {
> + unsigned int pstate;
> +
> + ret = brcm_avs_get_pstate(priv, &pstate);
> + if (!ret) {
> + policy->cur = freq_table[pstate].frequency;
> + dev_info(dev, "registered\n");
> + goto out;
> + }
> + }
> + dev_err(dev, "couldn't initialize driver (%d)\n", ret);
> +
> +unmap_intr_base:
> + iounmap(priv->avs_intr_base);
> + of_node_put(priv->intr_base_np);
> +unmap_base:
> + iounmap(priv->base);
> + of_node_put(priv->base_np);
> +out:
> + return ret;
> +}
> +
> +static int brcm_avs_cpu_exit(struct cpufreq_policy *policy)
> +{
> + struct private_data *priv = policy->driver_data;
> +
> + iounmap(priv->base);
> + iounmap(priv->avs_intr_base);
> + of_node_put(priv->base_np);
> + of_node_put(priv->intr_base_np);
What about clearing platform-data which you have set earlier ?
> +
> + return 0;
> +}
> +
> +static ssize_t show_brcm_avs_pstate(struct cpufreq_policy *policy, char *buf)
> +{
> + struct private_data *priv = policy->driver_data;
> + unsigned int pstate;
> +
> + if (brcm_avs_get_pstate(priv, &pstate))
> + return sprintf(buf, "<unknown>\n");
> +
> + return sprintf(buf, "%u\n", pstate);
> +}
> +
> +static ssize_t show_brcm_avs_mode(struct cpufreq_policy *policy, char *buf)
> +{
> + struct private_data *priv = policy->driver_data;
> + struct pmap pmap;
> +
> + if (brcm_avs_get_pmap(priv, &pmap))
> + return sprintf(buf, "<unknown>\n");
> +
> + return sprintf(buf, "%s %u\n", brcm_avs_mode_to_string(pmap.mode),
> + pmap.mode);
> +}
> +
> +static ssize_t show_brcm_avs_pmap(struct cpufreq_policy *policy, char *buf)
> +{
> + unsigned int mdiv_p0, mdiv_p1, mdiv_p2, mdiv_p3, mdiv_p4;
> + struct private_data *priv = policy->driver_data;
> + unsigned int ndiv, pdiv;
> + struct pmap pmap;
> +
> + if (brcm_avs_get_pmap(priv, &pmap))
> + return sprintf(buf, "<unknown>\n");
> +
> + brcm_avs_parse_p1(pmap.p1, &mdiv_p0, &pdiv, &ndiv);
> + brcm_avs_parse_p2(pmap.p2, &mdiv_p1, &mdiv_p2, &mdiv_p3, &mdiv_p4);
> +
> + return sprintf(buf, "0x%08x 0x%08x %u %u %u %u %u %u %u\n",
> + pmap.p1, pmap.p2, ndiv, pdiv, mdiv_p0, mdiv_p1, mdiv_p2,
> + mdiv_p3, mdiv_p4);
> +}
> +
> +static ssize_t show_brcm_avs_voltage(struct cpufreq_policy *policy, char *buf)
> +{
> + struct private_data *priv = policy->driver_data;
> +
> + return sprintf(buf, "0x%08lx\n", brcm_avs_get_voltage(priv->base));
> +}
> +
> +static ssize_t show_brcm_avs_frequency(struct cpufreq_policy *policy, char *buf)
> +{
> + struct private_data *priv = policy->driver_data;
> +
> + return sprintf(buf, "0x%08lx\n", brcm_avs_get_frequency(priv->base));
> +}
> +
> +cpufreq_freq_attr_ro(brcm_avs_pstate);
> +cpufreq_freq_attr_ro(brcm_avs_mode);
> +cpufreq_freq_attr_ro(brcm_avs_pmap);
> +cpufreq_freq_attr_ro(brcm_avs_voltage);
> +cpufreq_freq_attr_ro(brcm_avs_frequency);
> +
> +struct freq_attr *brcm_avs_cpufreq_attr[] = {
> + &cpufreq_freq_attr_scaling_available_freqs,
> + &brcm_avs_pstate,
> + &brcm_avs_mode,
> + &brcm_avs_pmap,
> + &brcm_avs_voltage,
> + &brcm_avs_frequency,
> + NULL
> +};
> +
> +static struct cpufreq_driver brcm_avs_driver = {
> + .flags = CPUFREQ_NEED_INITIAL_FREQ_CHECK,
> + .verify = cpufreq_generic_frequency_table_verify,
> + .target_index = brcm_avs_target_index,
> + .get = brcm_avs_cpufreq_get,
> + .suspend = brcm_avs_suspend,
> + .resume = brcm_avs_resume,
> + .init = brcm_avs_cpu_init,
> + .exit = brcm_avs_cpu_exit,
> + .name = BRCM_AVS_CPUFREQ_NAME,
> + .attr = brcm_avs_cpufreq_attr,
> +};
> +
> +static int brcm_avs_cpufreq_probe(struct platform_device *pdev)
> +{
> + int ret;
> +
> + brcm_avs_driver.driver_data = pdev;
> + ret = cpufreq_register_driver(&brcm_avs_driver);
> +
> + return ret;
> +}
> +
> +static int brcm_avs_cpufreq_remove(struct platform_device *pdev)
> +{
> + return cpufreq_unregister_driver(&brcm_avs_driver);
> +}
> +
> +static const struct of_device_id brcm_avs_cpufreq_match[] = {
> + { .compatible = BRCM_AVS_CPU_DATA },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, brcm_avs_cpufreq_match);
> +
> +static struct platform_driver brcm_avs_cpufreq_platdrv = {
> + .driver = {
> + .name = BRCM_AVS_CPUFREQ_NAME,
> + .of_match_table = brcm_avs_cpufreq_match,
> + },
> + .probe = brcm_avs_cpufreq_probe,
> + .remove = brcm_avs_cpufreq_remove,
> +};
> +module_platform_driver(brcm_avs_cpufreq_platdrv);
> +
> +MODULE_AUTHOR("Markus Mayer <mmayer@xxxxxxxxxxxx>");
> +MODULE_DESCRIPTION("CPUfreq driver for Broadcom AVS");
> +MODULE_LICENSE("GPL");
> --
> 2.7.4
--
viresh