linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RESEND PATCH v2 0/3] Broadcom AVS CPUfreq driver
@ 2016-09-30 21:55 Markus Mayer
  2016-09-30 21:55 ` [RESEND PATCH v2 1/3] dt: cpufreq: brcm: New binding document for brcmstb-avs-cpufreq Markus Mayer
                   ` (3 more replies)
  0 siblings, 4 replies; 16+ messages in thread
From: Markus Mayer @ 2016-09-30 21:55 UTC (permalink / raw)
  To: Rob Herring, Mark Rutland, Rafael J . Wysocki, Viresh Kumar
  Cc: Broadcom Kernel List, Device Tree List, Power Management List,
	Linux Kernel Mailing List, Markus Mayer

This series contains the CPUfreq driver for Broadcom SoCs that use "AVS
Firmware" for voltage and frequency scaling. All voltage and frequency
transitions are performed by the firmware and are therefore hidden from
Linux.

The driver provides a standard CPUfreq interface to other kernel
components and to userland on the one hand and communicates with the
AVS co-processor on the other.

Communication between the two processors is via shared mailbox
registers and interrupts (ARM -> AVS to tell the firmware that there is
a command to process and AVS -> ARM to tell the driver that a command
finished executing).

lkml.org seems to be down for me. Here are patchwork links to the original
series:

https://patchwork.kernel.org/patch/9278119/
https://patchwork.kernel.org/patch/9278121/
https://patchwork.kernel.org/patch/9278127/

Changes from v1:
    - renamed binding document 
    - rewrote the introduction of the binding document
    - created a new driver documentation file that contains Linux specific
      information that was previously part of the binding document
    - renamed the driver (and related config options) to include a reference
      to "STB", since this implementation is primarily intended for use on
      set-top boxes
    - improved comments
    - updated function __map_region()
    - updated struct private_data
    - added code to unmap memory regions in the error and exit paths
    - added new sysfs property to report frequency directly from the
      co-processor register

Markus Mayer (3):
  dt: cpufreq: brcm: New binding document for brcmstb-avs-cpufreq
  cpufreq: brcmstb-avs-cpufreq: AVS CPUfreq driver for Broadcom STB SoCs
  cpufreq: brcmstb-avs-cpufreq: add debugfs support

 Documentation/cpu-freq/brcmstb-avs-cpufreq.txt     |   27 +
 .../bindings/cpufreq/brcm,stb-avs-cpu-freq.txt     |   77 ++
 MAINTAINERS                                        |    9 +
 drivers/cpufreq/Kconfig.arm                        |   10 +
 drivers/cpufreq/Makefile                           |    1 +
 drivers/cpufreq/brcmstb-avs-cpufreq.c              | 1026 ++++++++++++++++++++
 6 files changed, 1150 insertions(+)
 create mode 100644 Documentation/cpu-freq/brcmstb-avs-cpufreq.txt
 create mode 100644 Documentation/devicetree/bindings/cpufreq/brcm,stb-avs-cpu-freq.txt
 create mode 100644 drivers/cpufreq/brcmstb-avs-cpufreq.c

-- 
2.7.4

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [RESEND PATCH v2 1/3] dt: cpufreq: brcm: New binding document for brcmstb-avs-cpufreq
  2016-09-30 21:55 [RESEND PATCH v2 0/3] Broadcom AVS CPUfreq driver Markus Mayer
@ 2016-09-30 21:55 ` Markus Mayer
  2016-10-05  3:27   ` Viresh Kumar
  2016-09-30 21:56 ` [RESEND PATCH v2 2/3] cpufreq: brcmstb-avs-cpufreq: AVS CPUfreq driver for Broadcom STB SoCs Markus Mayer
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 16+ messages in thread
From: Markus Mayer @ 2016-09-30 21:55 UTC (permalink / raw)
  To: Rob Herring, Mark Rutland, Rafael J . Wysocki, Viresh Kumar
  Cc: Broadcom Kernel List, Device Tree List, Power Management List,
	Linux Kernel Mailing List, Markus Mayer

Add the binding document for the new brcmstb-avs-cpufreq driver.

Signed-off-by: Markus Mayer <mmayer@broadcom.com>
---
 .../bindings/cpufreq/brcm,stb-avs-cpu-freq.txt     | 77 ++++++++++++++++++++++
 MAINTAINERS                                        |  7 ++
 2 files changed, 84 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/cpufreq/brcm,stb-avs-cpu-freq.txt

diff --git a/Documentation/devicetree/bindings/cpufreq/brcm,stb-avs-cpu-freq.txt b/Documentation/devicetree/bindings/cpufreq/brcm,stb-avs-cpu-freq.txt
new file mode 100644
index 0000000..d9ad3d3
--- /dev/null
+++ b/Documentation/devicetree/bindings/cpufreq/brcm,stb-avs-cpu-freq.txt
@@ -0,0 +1,77 @@
+Broadcom STB AVS CPUfreq driver bindings
+========================================
+
+A total of three DT nodes are required. One node (brcm,avs-cpu-data-mem)
+references the mailbox register used to communicate with the AVS CPU[1]. The
+second node (brcm,avs-cpu-l2-intr) is required to trigger an interrupt on
+the AVS CPU. The interrupt tells the AVS CPU that it needs to process a
+command sent to it by a driver. Interrupting the AVS CPU is mandatory for
+commands to be processed.
+
+The interface also requires a reference to the AVS host interrrupt
+controller, so a driver can react to interrupts generated by the AVS CPU
+whenever a command has been processed. See [2] for more information on the
+brcm,l2-intc node.
+
+[1] The AVS CPU is an independent co-processor that runs proprietary
+firmware. On some SoCs, this firmware supports DFS and DVFS in addition to
+Adaptive Voltage Scaling.
+
+[2] Documentation/devicetree/bindings/interrupt-controller/brcm,l2-intc.txt
+
+
+Node brcm,avs-cpu-data-mem
+--------------------------
+
+Required properties:
+- compatible: Sould be one of: brcm,avs-cpu-data-mem,
+              brcm,bcm7271-avs-cpu-data-mem or brcm,bcm7268-avs-cpu-data-mem
+- reg: Specifies base physical address and size of the registers.
+- interrupts: The interrupt that the AVS CPU will use to interrupt the host
+              when a command completed.
+- interrupt-parent: The interrupt controller the above interrupt is routed
+                    through.
+- interrupt-names: The name of the interrupt used to interrupt the host.
+
+Optional properties:
+- None
+
+Node brcm,avs-cpu-l2-intr
+-------------------------
+
+Required properties:
+- compatible: Sould be one of: brcm,avs-cpu-l2-intr,
+              brcm,bcm7271-avs-cpu-l2-intr or brcm,bcm7268-avs-cpu-l2-intr
+- reg: Specifies base physical address and size of the registers.
+
+Optional properties:
+- None
+
+
+Example
+=======
+
+	avs_host_l2_intc: interrupt-controller@f04d1200 {
+		#interrupt-cells = <1>;
+		compatible = "brcm,l2-intc";
+		interrupt-parent = <&intc>;
+		reg = <0xf04d1200 0x48>;
+		interrupt-controller;
+		interrupts = <0x0 0x19 0x0>;
+		interrupt-names = "avs";
+	};
+
+	avs-cpu-data-mem@f04c4000 {
+		compatible = "brcm,bcm7271-avs-cpu-data-mem",
+				"brcm,avs-cpu-data-mem";
+		reg = <0xf04c4000 0x60>;
+		interrupts = <0x1a>;
+		interrupt-parent = <&avs_host_l2_intc>;
+		interrupt-names = "sw_intr";
+	};
+
+	avs-cpu-l2-intr@f04d1100 {
+		compatible = "brcm,bcm7271-avs-cpu-l2-intr",
+				"brcm,avs-cpu-l2-intr";
+		reg = <0xf04d1100 0x10>;
+	};
diff --git a/MAINTAINERS b/MAINTAINERS
index 20bb1d0..557f839 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2689,6 +2689,13 @@ L:	bcm-kernel-feedback-list@broadcom.com
 S:	Maintained
 F:	drivers/mtd/nand/brcmnand/
 
+BROADCOM STB AVS CPUFREQ DRIVER
+M:	Markus Mayer <mmayer@broadcom.com>
+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
+
 BROADCOM SPECIFIC AMBA DRIVER (BCMA)
 M:	Rafał Miłecki <zajec5@gmail.com>
 L:	linux-wireless@vger.kernel.org
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [RESEND PATCH v2 2/3] cpufreq: brcmstb-avs-cpufreq: AVS CPUfreq driver for Broadcom STB SoCs
  2016-09-30 21:55 [RESEND PATCH v2 0/3] Broadcom AVS CPUfreq driver Markus Mayer
  2016-09-30 21:55 ` [RESEND PATCH v2 1/3] dt: cpufreq: brcm: New binding document for brcmstb-avs-cpufreq Markus Mayer
@ 2016-09-30 21:56 ` Markus Mayer
  2016-10-05  3:25   ` Viresh Kumar
  2016-09-30 21:56 ` [RESEND PATCH v2 3/3] cpufreq: brcmstb-avs-cpufreq: add debugfs support Markus Mayer
  2016-10-03  3:29 ` [RESEND PATCH v2 0/3] Broadcom AVS CPUfreq driver Viresh Kumar
  3 siblings, 1 reply; 16+ messages in thread
From: Markus Mayer @ 2016-09-30 21:56 UTC (permalink / raw)
  To: Rob Herring, Mark Rutland, Rafael J . Wysocki, Viresh Kumar
  Cc: Broadcom Kernel List, Device Tree List, Power Management List,
	Linux Kernel Mailing List, Markus Mayer

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@broadcom.com>
---
 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.
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 <zajec5@gmail.com>
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>
+#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[])
+{
+	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);
+	/* Give the caller a chance to retry if AVS is busy. */
+	if (i >= AVS_LOOP_LIMIT) {
+		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)
+		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);
+	/* 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);
+	/* 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)
+		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);
+	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;
+}
+
+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;
+
+	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;
+
+	/*
+	 * 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;
+
+	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)) {
+		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);
+
+	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@broadcom.com>");
+MODULE_DESCRIPTION("CPUfreq driver for Broadcom AVS");
+MODULE_LICENSE("GPL");
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [RESEND PATCH v2 3/3] cpufreq: brcmstb-avs-cpufreq: add debugfs support
  2016-09-30 21:55 [RESEND PATCH v2 0/3] Broadcom AVS CPUfreq driver Markus Mayer
  2016-09-30 21:55 ` [RESEND PATCH v2 1/3] dt: cpufreq: brcm: New binding document for brcmstb-avs-cpufreq Markus Mayer
  2016-09-30 21:56 ` [RESEND PATCH v2 2/3] cpufreq: brcmstb-avs-cpufreq: AVS CPUfreq driver for Broadcom STB SoCs Markus Mayer
@ 2016-09-30 21:56 ` Markus Mayer
  2016-10-05  3:27   ` Viresh Kumar
  2016-10-03  3:29 ` [RESEND PATCH v2 0/3] Broadcom AVS CPUfreq driver Viresh Kumar
  3 siblings, 1 reply; 16+ messages in thread
From: Markus Mayer @ 2016-09-30 21:56 UTC (permalink / raw)
  To: Rob Herring, Mark Rutland, Rafael J . Wysocki, Viresh Kumar
  Cc: Broadcom Kernel List, Device Tree List, Power Management List,
	Linux Kernel Mailing List, Markus Mayer

In order to aid debugging, we add a debugfs interface to the driver
that allows direct interaction with the AVS co-processor.

The debugfs interface provides a means for reading all and writing some
of the mailbox registers directly from the shell prompt and enables a
user to execute the communications protocol between ARM CPU and AVS CPU
step-by-step.

This interface should be used for debugging purposes only.

Signed-off-by: Markus Mayer <mmayer@broadcom.com>
---
 drivers/cpufreq/brcmstb-avs-cpufreq.c | 319 ++++++++++++++++++++++++++++++++++
 1 file changed, 319 insertions(+)

diff --git a/drivers/cpufreq/brcmstb-avs-cpufreq.c b/drivers/cpufreq/brcmstb-avs-cpufreq.c
index ddf5dd1..5d3c789 100644
--- a/drivers/cpufreq/brcmstb-avs-cpufreq.c
+++ b/drivers/cpufreq/brcmstb-avs-cpufreq.c
@@ -27,6 +27,12 @@
 #include <linux/pm_opp.h>
 #include <linux/slab.h>
 
+#ifdef CONFIG_ARM_BRCM_AVS_CPUFREQ_DEBUG
+#include <linux/ctype.h>
+#include <linux/debugfs.h>
+#include <linux/uaccess.h>
+#endif
+
 /* Max number of arguments AVS calls take */
 #define AVS_MAX_CMD_ARGS	4
 /*
@@ -154,11 +160,88 @@ struct private_data {
 	struct device_node *base_np;
 	struct device_node *intr_base_np;
 	struct device *dev;
+#ifdef CONFIG_ARM_BRCM_AVS_CPUFREQ_DEBUG
+	struct dentry *debugfs;
+#endif
 	struct completion done;
 	struct semaphore sem;
 	struct pmap pmap;
 };
 
+#ifdef CONFIG_ARM_BRCM_AVS_CPUFREQ_DEBUG
+
+enum debugfs_format {
+	DEBUGFS_NORMAL,
+	DEBUGFS_FLOAT,
+	DEBUGFS_REV,
+};
+
+struct debugfs_data {
+	struct debugfs_entry *entry;
+	struct private_data *priv;
+};
+
+struct debugfs_entry {
+	char *name;
+	u32 offset;
+	fmode_t mode;
+	enum debugfs_format format;
+};
+
+#define DEBUGFS_ENTRY(name, mode, format)	{ \
+	#name, AVS_MBOX_##name, mode, format \
+}
+
+/*
+ * These are used for debugfs only. Otherwise we use AVS_MBOX_PARAM() directly.
+ */
+#define AVS_MBOX_PARAM1		AVS_MBOX_PARAM(0)
+#define AVS_MBOX_PARAM2		AVS_MBOX_PARAM(1)
+#define AVS_MBOX_PARAM3		AVS_MBOX_PARAM(2)
+#define AVS_MBOX_PARAM4		AVS_MBOX_PARAM(3)
+
+/*
+ * This table stores the name, access permissions and offset for each hardware
+ * register and is used to generate debugfs entries.
+ */
+static struct debugfs_entry debugfs_entries[] = {
+	DEBUGFS_ENTRY(COMMAND, S_IWUSR, DEBUGFS_NORMAL),
+	DEBUGFS_ENTRY(STATUS, S_IWUSR, DEBUGFS_NORMAL),
+	DEBUGFS_ENTRY(VOLTAGE0, 0, DEBUGFS_FLOAT),
+	DEBUGFS_ENTRY(TEMP0, 0, DEBUGFS_FLOAT),
+	DEBUGFS_ENTRY(PV0, 0, DEBUGFS_FLOAT),
+	DEBUGFS_ENTRY(MV0, 0, DEBUGFS_FLOAT),
+	DEBUGFS_ENTRY(PARAM1, S_IWUSR, DEBUGFS_NORMAL),
+	DEBUGFS_ENTRY(PARAM2, S_IWUSR, DEBUGFS_NORMAL),
+	DEBUGFS_ENTRY(PARAM3, S_IWUSR, DEBUGFS_NORMAL),
+	DEBUGFS_ENTRY(PARAM4, S_IWUSR, DEBUGFS_NORMAL),
+	DEBUGFS_ENTRY(REVISION, 0, DEBUGFS_REV),
+	DEBUGFS_ENTRY(PSTATE, 0, DEBUGFS_NORMAL),
+	DEBUGFS_ENTRY(HEARTBEAT, 0, DEBUGFS_NORMAL),
+	DEBUGFS_ENTRY(MAGIC, S_IWUSR, DEBUGFS_NORMAL),
+	DEBUGFS_ENTRY(SIGMA_HVT, 0, DEBUGFS_NORMAL),
+	DEBUGFS_ENTRY(SIGMA_SVT, 0, DEBUGFS_NORMAL),
+	DEBUGFS_ENTRY(VOLTAGE1, 0, DEBUGFS_FLOAT),
+	DEBUGFS_ENTRY(TEMP1, 0, DEBUGFS_FLOAT),
+	DEBUGFS_ENTRY(PV1, 0, DEBUGFS_FLOAT),
+	DEBUGFS_ENTRY(MV1, 0, DEBUGFS_FLOAT),
+	DEBUGFS_ENTRY(FREQUENCY, 0, DEBUGFS_NORMAL),
+};
+
+static int brcm_avs_target_index(struct cpufreq_policy *, unsigned int);
+
+static char *__strtolower(char *s)
+{
+	char *p;
+
+	for (p = s; *p; p++)
+		*p = tolower(*p);
+
+	return s;
+}
+
+#endif /* CONFIG_ARM_BRCM_AVS_CPUFREQ_DEBUG */
+
 static void __iomem *__map_region(const char *name, struct device_node **node)
 {
 	struct device_node *np;
@@ -405,6 +488,239 @@ brcm_avs_get_freq_table(struct device *dev, struct private_data *priv)
 	return table;
 }
 
+#ifdef CONFIG_ARM_BRCM_AVS_CPUFREQ_DEBUG
+
+#define MANT(x)	(unsigned int)(abs((x)) / 1000)
+#define FRAC(x)	(unsigned int)(abs((x)) - abs((x)) / 1000 * 1000)
+
+static int brcm_avs_debug_show(struct seq_file *s, void *data)
+{
+	struct debugfs_data *dbgfs = s->private;
+	void __iomem *base;
+	u32 val, offset;
+
+	if (!dbgfs) {
+		seq_puts(s, "No device pointer\n");
+		return 0;
+	}
+
+	base = dbgfs->priv->base;
+	offset = dbgfs->entry->offset;
+	val = readl(base + offset);
+	switch (dbgfs->entry->format) {
+	case DEBUGFS_NORMAL:
+		seq_printf(s, "%u\n", val);
+		break;
+	case DEBUGFS_FLOAT:
+		seq_printf(s, "%d.%03d\n", MANT(val), FRAC(val));
+		break;
+	case DEBUGFS_REV:
+		seq_printf(s, "%c.%c.%c.%c\n", (val >> 24 & 0xff),
+			(val >> 16 & 0xff), (val >> 8 & 0xff),
+			val & 0xff);
+		break;
+	}
+	seq_printf(s, "0x%08x\n", val);
+
+	return 0;
+}
+
+#undef MANT
+#undef FRAC
+
+static ssize_t brcm_avs_seq_write(struct file *file, const char __user *buf,
+	size_t size, loff_t *ppos)
+{
+	struct seq_file *s = file->private_data;
+	struct debugfs_data *dbgfs = s->private;
+	struct private_data *priv = dbgfs->priv;
+	void __iomem *base, *avs_intr_base;
+	bool use_issue_command = false;
+	unsigned long val, offset;
+	char str[128];
+	int ret;
+	char *str_ptr = str;
+
+	if (size >= sizeof(str))
+		return -E2BIG;
+
+	memset(str, 0, sizeof(str));
+	ret = copy_from_user(str, buf, size);
+	if (ret)
+		return ret;
+
+	base = priv->base;
+	avs_intr_base = priv->avs_intr_base;
+	offset = dbgfs->entry->offset;
+	/*
+	 * Special case writing to "command" entry only: if the string starts
+	 * with a 'c', we use the driver's __issue_avs_command() function.
+	 * Otherwise, we perform a raw write. This should allow testing of raw
+	 * access as well as using the higher level function. (Raw access
+	 * doesn't clear the firmware return status after issuing the command.)
+	 */
+	if (str_ptr[0] == 'c' && offset == AVS_MBOX_COMMAND) {
+		use_issue_command = true;
+		str_ptr++;
+	}
+	if (kstrtoul(str_ptr, 0, &val) != 0)
+		return -EINVAL;
+
+	/*
+	 * Setting the P-state is a special case. We need to update the CPU
+	 * frequency we report.
+	 */
+	if (val == AVS_CMD_SET_PSTATE) {
+		struct cpufreq_policy *policy;
+		unsigned int pstate;
+
+		policy = cpufreq_cpu_get(smp_processor_id());
+		/* Read back the P-state we are about to set */
+		pstate = readl(base + AVS_MBOX_PARAM(0));
+		if (use_issue_command) {
+			ret = brcm_avs_target_index(policy, pstate);
+			return ret ? ret : size;
+		}
+		policy->cur = policy->freq_table[pstate].frequency;
+	}
+
+	if (use_issue_command) {
+		ret = __issue_avs_command(priv, val, false, NULL);
+	} else {
+		/* Locking here is not perfect, but is only for debug. */
+		ret = down_interruptible(&priv->sem);
+		if (ret)
+			return ret;
+
+		writel(val, base + offset);
+		/* We have to wake up the firmware to process a command. */
+		if (offset == AVS_MBOX_COMMAND)
+			writel(AVS_CPU_L2_INT_MASK,
+				avs_intr_base + AVS_CPU_L2_SET0);
+		up(&priv->sem);
+	}
+
+
+	return ret ? ret : size;
+}
+
+static struct debugfs_entry *__find_debugfs_entry(const char *name)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(debugfs_entries); i++)
+		if (strcasecmp(debugfs_entries[i].name, name) == 0)
+			return &debugfs_entries[i];
+
+	return NULL;
+}
+
+static int brcm_avs_debug_open(struct inode *inode, struct file *file)
+{
+	struct debugfs_data *data;
+	fmode_t fmode;
+	int ret;
+
+	/*
+	 * seq_open(), which is called by single_open(), clears "write" access.
+	 * We need write access to some files, so we preserve our access mode
+	 * and restore it.
+	 */
+	fmode = file->f_mode;
+	/*
+	 * Check access permissions even for root. We don't want to be writing
+	 * to read-only registers. Access for regular users has already been
+	 * checked by the VFS layer.
+	 */
+	if ((fmode & FMODE_WRITER) && !(inode->i_mode & S_IWUSR))
+		return -EACCES;
+
+	data = kmalloc(sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+	/*
+	 * We use the same file system operations for all our debug files. To
+	 * produce specific output, we look up the file name upon opening a
+	 * debugfs entry and map it to a memory offset. This offset is then used
+	 * in the generic "show" function to read a specific register.
+	 */
+	data->entry = __find_debugfs_entry(file->f_path.dentry->d_iname);
+	data->priv = inode->i_private;
+
+	ret = single_open(file, brcm_avs_debug_show, data);
+	if (ret)
+		kfree(data);
+	file->f_mode = fmode;
+
+	return ret;
+}
+
+static int brcm_avs_debug_release(struct inode *inode, struct file *file)
+{
+	struct seq_file *seq_priv = file->private_data;
+	struct debugfs_data *data = seq_priv->private;
+
+	kfree(data);
+	return single_release(inode, file);
+}
+
+static const struct file_operations brcm_avs_debug_ops = {
+	.open		= brcm_avs_debug_open,
+	.read		= seq_read,
+	.write		= brcm_avs_seq_write,
+	.llseek		= seq_lseek,
+	.release	= brcm_avs_debug_release,
+};
+
+static void brcm_avs_cpufreq_debug_init(struct platform_device *pdev)
+{
+	struct private_data *priv = platform_get_drvdata(pdev);
+	struct dentry *dir;
+	int i;
+
+	if (!priv)
+		return;
+
+	dir = debugfs_create_dir(BRCM_AVS_CPUFREQ_NAME, NULL);
+	if (IS_ERR_OR_NULL(dir))
+		return;
+	priv->debugfs = dir;
+
+	for (i = 0; i < ARRAY_SIZE(debugfs_entries); i++) {
+		/*
+		 * The DEBUGFS_ENTRY macro generates uppercase strings. We
+		 * convert them to lowercase before creating the debugfs
+		 * entries.
+		 */
+		char *entry = __strtolower(debugfs_entries[i].name);
+		fmode_t mode = debugfs_entries[i].mode;
+
+		if (!debugfs_create_file(entry, S_IFREG | S_IRUGO | mode,
+				dir, priv, &brcm_avs_debug_ops)) {
+			priv->debugfs = NULL;
+			debugfs_remove_recursive(dir);
+			break;
+		}
+	}
+}
+
+static void brcm_avs_cpufreq_debug_exit(struct platform_device *pdev)
+{
+	struct private_data *priv = platform_get_drvdata(pdev);
+
+	if (priv && priv->debugfs) {
+		debugfs_remove_recursive(priv->debugfs);
+		priv->debugfs = NULL;
+	}
+}
+
+#else
+
+static void brcm_avs_cpufreq_debug_init(struct platform_device *pdev) {}
+static void brcm_avs_cpufreq_debug_exit(struct platform_device *pdev) {}
+
+#endif /* CONFIG_ARM_BRCM_AVS_CPUFREQ_DEBUG */
+
 /*
  * To ensure the right firmware is running we need to
  *    - check the MAGIC matches what we expect
@@ -677,12 +993,15 @@ static int brcm_avs_cpufreq_probe(struct platform_device *pdev)
 
 	brcm_avs_driver.driver_data = pdev;
 	ret = cpufreq_register_driver(&brcm_avs_driver);
+	if (!ret)
+		brcm_avs_cpufreq_debug_init(pdev);
 
 	return ret;
 }
 
 static int brcm_avs_cpufreq_remove(struct platform_device *pdev)
 {
+	brcm_avs_cpufreq_debug_exit(pdev);
 	return cpufreq_unregister_driver(&brcm_avs_driver);
 }
 
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [RESEND PATCH v2 0/3] Broadcom AVS CPUfreq driver
  2016-09-30 21:55 [RESEND PATCH v2 0/3] Broadcom AVS CPUfreq driver Markus Mayer
                   ` (2 preceding siblings ...)
  2016-09-30 21:56 ` [RESEND PATCH v2 3/3] cpufreq: brcmstb-avs-cpufreq: add debugfs support Markus Mayer
@ 2016-10-03  3:29 ` Viresh Kumar
  3 siblings, 0 replies; 16+ messages in thread
From: Viresh Kumar @ 2016-10-03  3:29 UTC (permalink / raw)
  To: Markus Mayer
  Cc: Rob Herring, Mark Rutland, Rafael J . Wysocki,
	Broadcom Kernel List, Device Tree List, Power Management List,
	Linux Kernel Mailing List

On 30-09-16, 14:55, Markus Mayer wrote:
> This series contains the CPUfreq driver for Broadcom SoCs that use "AVS
> Firmware" for voltage and frequency scaling. All voltage and frequency
> transitions are performed by the firmware and are therefore hidden from
> Linux.
> 
> The driver provides a standard CPUfreq interface to other kernel
> components and to userland on the one hand and communicates with the
> AVS co-processor on the other.
> 
> Communication between the two processors is via shared mailbox
> registers and interrupts (ARM -> AVS to tell the firmware that there is
> a command to process and AVS -> ARM to tell the driver that a command
> finished executing).
> 
> lkml.org seems to be down for me. Here are patchwork links to the original
> series:

My fault really. I wanted to review it earlier but couldn't :(

I should be doing it this week though.

-- 
viresh

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [RESEND PATCH v2 2/3] cpufreq: brcmstb-avs-cpufreq: AVS CPUfreq driver for Broadcom STB SoCs
  2016-09-30 21:56 ` [RESEND PATCH v2 2/3] cpufreq: brcmstb-avs-cpufreq: AVS CPUfreq driver for Broadcom STB SoCs Markus Mayer
@ 2016-10-05  3:25   ` Viresh Kumar
  2016-10-05 21:04     ` Markus Mayer
  0 siblings, 1 reply; 16+ messages in thread
From: Viresh Kumar @ 2016-10-05  3:25 UTC (permalink / raw)
  To: Markus Mayer
  Cc: Rob Herring, Mark Rutland, Rafael J . Wysocki,
	Broadcom Kernel List, Device Tree List, Power Management List,
	Linux Kernel Mailing List

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@broadcom.com>
> ---
>  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 <zajec5@gmail.com>
> 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@broadcom.com>");
> +MODULE_DESCRIPTION("CPUfreq driver for Broadcom AVS");
> +MODULE_LICENSE("GPL");
> -- 
> 2.7.4

-- 
viresh

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [RESEND PATCH v2 3/3] cpufreq: brcmstb-avs-cpufreq: add debugfs support
  2016-09-30 21:56 ` [RESEND PATCH v2 3/3] cpufreq: brcmstb-avs-cpufreq: add debugfs support Markus Mayer
@ 2016-10-05  3:27   ` Viresh Kumar
  0 siblings, 0 replies; 16+ messages in thread
From: Viresh Kumar @ 2016-10-05  3:27 UTC (permalink / raw)
  To: Markus Mayer
  Cc: Rob Herring, Mark Rutland, Rafael J . Wysocki,
	Broadcom Kernel List, Device Tree List, Power Management List,
	Linux Kernel Mailing List

On 30-09-16, 14:56, Markus Mayer wrote:
> In order to aid debugging, we add a debugfs interface to the driver
> that allows direct interaction with the AVS co-processor.
> 
> The debugfs interface provides a means for reading all and writing some
> of the mailbox registers directly from the shell prompt and enables a
> user to execute the communications protocol between ARM CPU and AVS CPU
> step-by-step.
> 
> This interface should be used for debugging purposes only.
> 
> Signed-off-by: Markus Mayer <mmayer@broadcom.com>
> ---
>  drivers/cpufreq/brcmstb-avs-cpufreq.c | 319 ++++++++++++++++++++++++++++++++++
>  1 file changed, 319 insertions(+)

I am not sure how much useful this is going to be, but I don't have
any concerns against this as well.

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [RESEND PATCH v2 1/3] dt: cpufreq: brcm: New binding document for brcmstb-avs-cpufreq
  2016-09-30 21:55 ` [RESEND PATCH v2 1/3] dt: cpufreq: brcm: New binding document for brcmstb-avs-cpufreq Markus Mayer
@ 2016-10-05  3:27   ` Viresh Kumar
  0 siblings, 0 replies; 16+ messages in thread
From: Viresh Kumar @ 2016-10-05  3:27 UTC (permalink / raw)
  To: Markus Mayer
  Cc: Rob Herring, Mark Rutland, Rafael J . Wysocki,
	Broadcom Kernel List, Device Tree List, Power Management List,
	Linux Kernel Mailing List

On 30-09-16, 14:55, Markus Mayer wrote:
> Add the binding document for the new brcmstb-avs-cpufreq driver.
> 
> Signed-off-by: Markus Mayer <mmayer@broadcom.com>
> ---
>  .../bindings/cpufreq/brcm,stb-avs-cpu-freq.txt     | 77 ++++++++++++++++++++++
>  MAINTAINERS                                        |  7 ++
>  2 files changed, 84 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/cpufreq/brcm,stb-avs-cpu-freq.txt

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [RESEND PATCH v2 2/3] cpufreq: brcmstb-avs-cpufreq: AVS CPUfreq driver for Broadcom STB SoCs
  2016-10-05  3:25   ` Viresh Kumar
@ 2016-10-05 21:04     ` Markus Mayer
  2016-10-06  4:01       ` Viresh Kumar
  0 siblings, 1 reply; 16+ messages in thread
From: Markus Mayer @ 2016-10-05 21:04 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: Rob Herring, Mark Rutland, Rafael J . Wysocki,
	Broadcom Kernel List, Device Tree List, Power Management List,
	Linux Kernel Mailing List

On 4 October 2016 at 20:25, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> 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@broadcom.com>
>> ---
>>  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..

The reason I created a separate file was because Rob didn't want Linux
specifics in a binding document (https://lkml.org/lkml/2016/8/16/403).

I am okay sticking it into a comment in the driver.

>> 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 <zajec5@gmail.com>
>> 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.

Done.

>> +#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.

Done (all instances).

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

Done.

>> +     /* Give the caller a chance to retry if AVS is busy. */
>> +     if (i >= AVS_LOOP_LIMIT) {
>
> Isn't == sufficient here ?

Yes, it is. Changed to "==".

>> +             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..

Done.

>> +             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...

Done.

>> +     /* 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.

Done.

>> +     /* 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 {} ..

Done.

>> +             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.

Done, as mentioned above.

>> +     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.

Fixed. Changed to read from the hardware.

>> +}
>> +
>> +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.

Fixed.

>> +
>> +     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.

Done.

>> +
>> +     /*
>> +      * 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?

Is there an easy way for me to know via the framework whether init is
being called for the first time vs. init is being called on a
different core after a previous attempt to initialize on another core
failed?

I could use a driver-global variable for the driver to remember if it
has been initialized, but that seems a bit hacky.

>> +
>> +     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 ?

Fixed. Moved it up right after mapping priv->base.

>> +             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 ?

You  mean "platform_set_drvdata(pdev, NULL)"? Added to
brcm_avs_cpu_init() and brcm_avs_cpufreq_remove().

>> +
>> +     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@broadcom.com>");
>> +MODULE_DESCRIPTION("CPUfreq driver for Broadcom AVS");
>> +MODULE_LICENSE("GPL");
>> --
>> 2.7.4
>
> --
> viresh

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [RESEND PATCH v2 2/3] cpufreq: brcmstb-avs-cpufreq: AVS CPUfreq driver for Broadcom STB SoCs
  2016-10-05 21:04     ` Markus Mayer
@ 2016-10-06  4:01       ` Viresh Kumar
  2016-10-06 14:51         ` Markus Mayer
  0 siblings, 1 reply; 16+ messages in thread
From: Viresh Kumar @ 2016-10-06  4:01 UTC (permalink / raw)
  To: Markus Mayer
  Cc: Rob Herring, Mark Rutland, Rafael J . Wysocki,
	Broadcom Kernel List, Device Tree List, Power Management List,
	Linux Kernel Mailing List

Thanks for accepting all the comments :)

On 05-10-16, 14:04, Markus Mayer wrote:
> Is there an easy way for me to know via the framework whether init is
> being called for the first time vs. init is being called on a
> different core after a previous attempt to initialize on another core
> failed?
> 
> I could use a driver-global variable for the driver to remember if it
> has been initialized, but that seems a bit hacky.

You don't really need to have any special code here, specially for the case that
may never get hit. For example, if we fail to initialize something for CPU0,
cpufreq core will try calling this routine for other CPUs as well. I don't think
there is anything wrong in letting cpufreq core trying that. Why stop it or
return early? It wouldn't happen normally, unless there is a bug in there.

-- 
viresh

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [RESEND PATCH v2 2/3] cpufreq: brcmstb-avs-cpufreq: AVS CPUfreq driver for Broadcom STB SoCs
  2016-10-06  4:01       ` Viresh Kumar
@ 2016-10-06 14:51         ` Markus Mayer
  2016-10-06 21:04           ` Markus Mayer
  2016-10-07  3:33           ` Viresh Kumar
  0 siblings, 2 replies; 16+ messages in thread
From: Markus Mayer @ 2016-10-06 14:51 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: Rob Herring, Mark Rutland, Rafael J . Wysocki,
	Broadcom Kernel List, Device Tree List, Power Management List,
	Linux Kernel Mailing List

On 5 October 2016 at 21:01, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> Thanks for accepting all the comments :)
>
> On 05-10-16, 14:04, Markus Mayer wrote:
>> Is there an easy way for me to know via the framework whether init is
>> being called for the first time vs. init is being called on a
>> different core after a previous attempt to initialize on another core
>> failed?
>>
>> I could use a driver-global variable for the driver to remember if it
>> has been initialized, but that seems a bit hacky.
>
> You don't really need to have any special code here, specially for the case that
> may never get hit. For example, if we fail to initialize something for CPU0,
> cpufreq core will try calling this routine for other CPUs as well. I don't think
> there is anything wrong in letting cpufreq core trying that. Why stop it or
> return early? It wouldn't happen normally, unless there is a bug in there.

During early development, when the driver couldn't fully register, I
would see the init() function called four times, i.e. once for each
core. If the first call succeeded, that was it. It would only get
called once. But if it failed, all cores would try to register. And I
wanted to avoid spilling the same error message four times.

I'll look at that again. It may have had something to do with how the
driver worked back then. If it doesn't happen anymore, I'll just get
rid of this code.

Thanks,
-Markus

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [RESEND PATCH v2 2/3] cpufreq: brcmstb-avs-cpufreq: AVS CPUfreq driver for Broadcom STB SoCs
  2016-10-06 14:51         ` Markus Mayer
@ 2016-10-06 21:04           ` Markus Mayer
  2016-10-07  3:41             ` Viresh Kumar
  2016-10-07  3:33           ` Viresh Kumar
  1 sibling, 1 reply; 16+ messages in thread
From: Markus Mayer @ 2016-10-06 21:04 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: Rob Herring, Mark Rutland, Rafael J . Wysocki,
	Broadcom Kernel List, Device Tree List, Power Management List,
	Linux Kernel Mailing List

On Thu, Oct 06, 2016 at 07:51:59AM -0700, Markus Mayer wrote:
> On 5 October 2016 at 21:01, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > Thanks for accepting all the comments :)

Unfortunately, I'll have to take back one agreed-upon change.

In this piece of code, brcm_avs_is_firmware_loaded() has to come after
requesting the IRQ.


	priv->base = __map_region(BRCM_AVS_CPU_DATA);
	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);
	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))
		return 0;

The reason being that we need to be ready to receive interrupts from the
co-processor to tell us the GET_PMAP command completed.
brcm_avs_is_firmware_loaded() issues the GET_PMAP command to ensure the
firmware supports it. If I try to run brcm_avs_is_firmware_loaded() before
setting up interrupts, I get a timeout in the driver -- because it never
receives the interrupt saying the command is done.

And I have to check the firmware supports the GET_PMAP command, because it
is possible for somebody to choose to run a firmware that does not support
DVFS, even though the hardware would support it.

> >> Is there an easy way for me to know via the framework whether init is
> >> being called for the first time vs. init is being called on a
> >> different core after a previous attempt to initialize on another core
> >> failed?
> >>
> >> I could use a driver-global variable for the driver to remember if it
> >> has been initialized, but that seems a bit hacky.
> >
> > You don't really need to have any special code here, specially for the case that
> > may never get hit. For example, if we fail to initialize something for CPU0,
> > cpufreq core will try calling this routine for other CPUs as well. I don't think
> > there is anything wrong in letting cpufreq core trying that. Why stop it or
> > return early? It wouldn't happen normally, unless there is a bug in there.
> 
> During early development, when the driver couldn't fully register, I
> would see the init() function called four times, i.e. once for each
> core. If the first call succeeded, that was it. It would only get
> called once. But if it failed, all cores would try to register. And I
> wanted to avoid spilling the same error message four times.
> 
> I'll look at that again. It may have had something to do with how the
> driver worked back then. If it doesn't happen anymore, I'll just get
> rid of this code.

Okay, I looked some more and compared it to what cpufreq-dt is doing to
initialize. That lead me onto the right track. They simply do things they
only want done once via the driver's probe function rather than CPUfreq's
init function. So, I broke my initialization code up into two parts. 
Everything up to checking if the firmware is loaded is now being called from
brcm_avs_cpufreq_probe(). The frequency table code is still being called
from brcm_avs_cpufreq_init().

That means that if the initial hardware checks fail, it won't try again.

Regards,
-Markus

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [RESEND PATCH v2 2/3] cpufreq: brcmstb-avs-cpufreq: AVS CPUfreq driver for Broadcom STB SoCs
  2016-10-06 14:51         ` Markus Mayer
  2016-10-06 21:04           ` Markus Mayer
@ 2016-10-07  3:33           ` Viresh Kumar
  1 sibling, 0 replies; 16+ messages in thread
From: Viresh Kumar @ 2016-10-07  3:33 UTC (permalink / raw)
  To: Markus Mayer
  Cc: Rob Herring, Mark Rutland, Rafael J . Wysocki,
	Broadcom Kernel List, Device Tree List, Power Management List,
	Linux Kernel Mailing List

On 06-10-16, 07:51, Markus Mayer wrote:
> During early development, when the driver couldn't fully register, I
> would see the init() function called four times, i.e. once for each
> core. If the first call succeeded, that was it. It would only get
> called once. But if it failed, all cores would try to register. And I
> wanted to avoid spilling the same error message four times.

This is the current behavior of the cpufreq core.

> I'll look at that again. It may have had something to do with how the
> driver worked back then. If it doesn't happen anymore, I'll just get
> rid of this code.

I don't think so, but again, we don't need to have special code to save few
lines which get printed only in the rare case of errors.

-- 
viresh

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [RESEND PATCH v2 2/3] cpufreq: brcmstb-avs-cpufreq: AVS CPUfreq driver for Broadcom STB SoCs
  2016-10-06 21:04           ` Markus Mayer
@ 2016-10-07  3:41             ` Viresh Kumar
  0 siblings, 0 replies; 16+ messages in thread
From: Viresh Kumar @ 2016-10-07  3:41 UTC (permalink / raw)
  To: Markus Mayer
  Cc: Rob Herring, Mark Rutland, Rafael J . Wysocki,
	Broadcom Kernel List, Device Tree List, Power Management List,
	Linux Kernel Mailing List

On 06-10-16, 14:04, Markus Mayer wrote:
> Unfortunately, I'll have to take back one agreed-upon change.
> 
> In this piece of code, brcm_avs_is_firmware_loaded() has to come after
> requesting the IRQ.
> 
> 
> 	priv->base = __map_region(BRCM_AVS_CPU_DATA);
> 	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);
> 	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))
> 		return 0;
> 
> The reason being that we need to be ready to receive interrupts from the
> co-processor to tell us the GET_PMAP command completed.
> brcm_avs_is_firmware_loaded() issues the GET_PMAP command to ensure the
> firmware supports it. If I try to run brcm_avs_is_firmware_loaded() before
> setting up interrupts, I get a timeout in the driver -- because it never
> receives the interrupt saying the command is done.
> 
> And I have to check the firmware supports the GET_PMAP command, because it
> is possible for somebody to choose to run a firmware that does not support
> DVFS, even though the hardware would support it.

Fair enough.

> Okay, I looked some more and compared it to what cpufreq-dt is doing to
> initialize. That lead me onto the right track. They simply do things they
> only want done once via the driver's probe function rather than CPUfreq's
> init function. So, I broke my initialization code up into two parts. 

I wanted to suggest that earlier, but then didn't do it :)

> Everything up to checking if the firmware is loaded is now being called from
> brcm_avs_cpufreq_probe(). The frequency table code is still being called
> from brcm_avs_cpufreq_init().

Looks fine.

> That means that if the initial hardware checks fail, it won't try again.

Yeah, but if init fails for some reason, then it will get called again for other
CPUs. Which is of course the right thing IMO.

-- 
viresh

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [RESEND PATCH v2 0/3] Broadcom AVS CPUfreq driver
  2016-10-07 23:20 Markus Mayer
@ 2016-10-07 23:22 ` Markus Mayer
  0 siblings, 0 replies; 16+ messages in thread
From: Markus Mayer @ 2016-10-07 23:22 UTC (permalink / raw)
  To: Rafael J . Wysocki, Viresh Kumar, Rob Herring, Mark Rutland
  Cc: Broadcom Kernel List, Device Tree List, Power Management List,
	Linux Kernel Mailing List, Markus Mayer

My apologies. Please ignore this e-mail. Proper series coming up shortly.

Regards,
-Markus

On 7 October 2016 at 16:20, Markus Mayer <mmayer@broadcom.com> wrote:
> This series contains the CPUfreq driver for Broadcom SoCs that use "AVS
> Firmware" for voltage and frequency scaling. All voltage and frequency
> transitions are performed by the firmware and are therefore hidden from
> Linux.
>
> The driver provides a standard CPUfreq interface to other kernel
> components and to userland on the one hand and communicates with the
> AVS co-processor on the other.
>
> Communication between the two processors is via shared mailbox
> registers and interrupts (ARM -> AVS to tell the firmware that there is
> a command to process and AVS -> ARM to tell the driver that a command
> finished executing).
>
> lkml.org seems to be down for me. Here are patchwork links to the original
> series:
>
> https://patchwork.kernel.org/patch/9278119/
> https://patchwork.kernel.org/patch/9278121/
> https://patchwork.kernel.org/patch/9278127/
>
> Changes from v1:
>     - renamed binding document
>     - rewrote the introduction of the binding document
>     - created a new driver documentation file that contains Linux specific
>       information that was previously part of the binding document
>     - renamed the driver (and related config options) to include a reference
>       to "STB", since this implementation is primarily intended for use on
>       set-top boxes
>     - improved comments
>     - updated function __map_region()
>     - updated struct private_data
>     - added code to unmap memory regions in the error and exit paths
>     - added new sysfs property to report frequency directly from the
>       co-processor register
>
> Markus Mayer (3):
>   dt: cpufreq: brcm: New binding document for brcmstb-avs-cpufreq
>   cpufreq: brcmstb-avs-cpufreq: AVS CPUfreq driver for Broadcom STB SoCs
>   cpufreq: brcmstb-avs-cpufreq: add debugfs support
>
>  Documentation/cpu-freq/brcmstb-avs-cpufreq.txt     |   27 +
>  .../bindings/cpufreq/brcm,stb-avs-cpu-freq.txt     |   77 ++
>  MAINTAINERS                                        |    9 +
>  drivers/cpufreq/Kconfig.arm                        |   10 +
>  drivers/cpufreq/Makefile                           |    1 +
>  drivers/cpufreq/brcmstb-avs-cpufreq.c              | 1026 ++++++++++++++++++++
>  6 files changed, 1150 insertions(+)
>  create mode 100644 Documentation/cpu-freq/brcmstb-avs-cpufreq.txt
>  create mode 100644 Documentation/devicetree/bindings/cpufreq/brcm,stb-avs-cpu-freq.txt
>  create mode 100644 drivers/cpufreq/brcmstb-avs-cpufreq.c
>
> --
> 2.7.4
>

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [RESEND PATCH v2 0/3] Broadcom AVS CPUfreq driver
@ 2016-10-07 23:20 Markus Mayer
  2016-10-07 23:22 ` Markus Mayer
  0 siblings, 1 reply; 16+ messages in thread
From: Markus Mayer @ 2016-10-07 23:20 UTC (permalink / raw)
  To: Rafael J . Wysocki, Viresh Kumar, Rob Herring, Mark Rutland
  Cc: Broadcom Kernel List, Device Tree List, Power Management List,
	Linux Kernel Mailing List, Markus Mayer

This series contains the CPUfreq driver for Broadcom SoCs that use "AVS
Firmware" for voltage and frequency scaling. All voltage and frequency
transitions are performed by the firmware and are therefore hidden from
Linux.

The driver provides a standard CPUfreq interface to other kernel
components and to userland on the one hand and communicates with the
AVS co-processor on the other.

Communication between the two processors is via shared mailbox
registers and interrupts (ARM -> AVS to tell the firmware that there is
a command to process and AVS -> ARM to tell the driver that a command
finished executing).

lkml.org seems to be down for me. Here are patchwork links to the original
series:

https://patchwork.kernel.org/patch/9278119/
https://patchwork.kernel.org/patch/9278121/
https://patchwork.kernel.org/patch/9278127/

Changes from v1:
    - renamed binding document 
    - rewrote the introduction of the binding document
    - created a new driver documentation file that contains Linux specific
      information that was previously part of the binding document
    - renamed the driver (and related config options) to include a reference
      to "STB", since this implementation is primarily intended for use on
      set-top boxes
    - improved comments
    - updated function __map_region()
    - updated struct private_data
    - added code to unmap memory regions in the error and exit paths
    - added new sysfs property to report frequency directly from the
      co-processor register

Markus Mayer (3):
  dt: cpufreq: brcm: New binding document for brcmstb-avs-cpufreq
  cpufreq: brcmstb-avs-cpufreq: AVS CPUfreq driver for Broadcom STB SoCs
  cpufreq: brcmstb-avs-cpufreq: add debugfs support

 Documentation/cpu-freq/brcmstb-avs-cpufreq.txt     |   27 +
 .../bindings/cpufreq/brcm,stb-avs-cpu-freq.txt     |   77 ++
 MAINTAINERS                                        |    9 +
 drivers/cpufreq/Kconfig.arm                        |   10 +
 drivers/cpufreq/Makefile                           |    1 +
 drivers/cpufreq/brcmstb-avs-cpufreq.c              | 1026 ++++++++++++++++++++
 6 files changed, 1150 insertions(+)
 create mode 100644 Documentation/cpu-freq/brcmstb-avs-cpufreq.txt
 create mode 100644 Documentation/devicetree/bindings/cpufreq/brcm,stb-avs-cpu-freq.txt
 create mode 100644 drivers/cpufreq/brcmstb-avs-cpufreq.c

-- 
2.7.4

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2016-10-07 23:26 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-30 21:55 [RESEND PATCH v2 0/3] Broadcom AVS CPUfreq driver Markus Mayer
2016-09-30 21:55 ` [RESEND PATCH v2 1/3] dt: cpufreq: brcm: New binding document for brcmstb-avs-cpufreq Markus Mayer
2016-10-05  3:27   ` Viresh Kumar
2016-09-30 21:56 ` [RESEND PATCH v2 2/3] cpufreq: brcmstb-avs-cpufreq: AVS CPUfreq driver for Broadcom STB SoCs Markus Mayer
2016-10-05  3:25   ` Viresh Kumar
2016-10-05 21:04     ` Markus Mayer
2016-10-06  4:01       ` Viresh Kumar
2016-10-06 14:51         ` Markus Mayer
2016-10-06 21:04           ` Markus Mayer
2016-10-07  3:41             ` Viresh Kumar
2016-10-07  3:33           ` Viresh Kumar
2016-09-30 21:56 ` [RESEND PATCH v2 3/3] cpufreq: brcmstb-avs-cpufreq: add debugfs support Markus Mayer
2016-10-05  3:27   ` Viresh Kumar
2016-10-03  3:29 ` [RESEND PATCH v2 0/3] Broadcom AVS CPUfreq driver Viresh Kumar
2016-10-07 23:20 Markus Mayer
2016-10-07 23:22 ` Markus Mayer

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).