Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753052AbcJEDZR (ORCPT ); Tue, 4 Oct 2016 23:25:17 -0400 Received: from mail-pf0-f175.google.com ([209.85.192.175]:34618 "EHLO mail-pf0-f175.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751223AbcJEDZO (ORCPT ); Tue, 4 Oct 2016 23:25:14 -0400 Date: Wed, 5 Oct 2016 08:55:09 +0530 From: Viresh Kumar To: Markus Mayer Cc: Rob Herring , Mark Rutland , "Rafael J . Wysocki" , Broadcom Kernel List , Device Tree List , Power Management List , Linux Kernel Mailing List Subject: Re: [RESEND PATCH v2 2/3] cpufreq: brcmstb-avs-cpufreq: AVS CPUfreq driver for Broadcom STB SoCs Message-ID: <20161005032509.GG4664@vireshk-i7> References: <1475272561-8446-1-git-send-email-mmayer@broadcom.com> <1475272561-8446-3-git-send-email-mmayer@broadcom.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1475272561-8446-3-git-send-email-mmayer@broadcom.com> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 26517 Lines: 876 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 > --- > 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@broadcom.com > L: linux-pm@vger.kernel.org > 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 > 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 Since you have kept the rest in ascending order, it might be better to put this one after io.h. > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +/* 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, "\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, "\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, "\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 "); > +MODULE_DESCRIPTION("CPUfreq driver for Broadcom AVS"); > +MODULE_LICENSE("GPL"); > -- > 2.7.4 -- viresh