Raspberry Pi DT clocks series

Raspberry Pi DT clocks series

[Eric Anholt]

Here's a series to add a real clock provider on Raspberry Pi.
Previously, we've been using a mix of fixed clocks from clk-bcm2835.c
(though some of them failed to get used by their intended consumers),
and fixed-clock nodes in the DT.

This driver gives us the ability to enable/disable our clocks, and
also lets us get and set frequency values.  This will let us build a
cpufreq driver (Andrea Merello has done some work on this front, and
has a couple of options available), and get correct frequencies on the
Raspberry Pi 2 without hardcoding new values.

This branch can be found at:

https://github.com/anholt/linux/tree/rpi-dt-clocks

which also has a patch for the MMC clock which is not included yet due
to a bug in the sdhci driver.  The series applies on top of
rpi-mailbox, whose code is currently out for review:

http://lists.infradead.org/pipermail/linux-rpi-kernel/2015-May/001762.html

[PATCH 1/7] dt/bindings: Add binding for the Raspberry Pi clock provider

[Eric Anholt]

The hardware clocks are not controllable by the ARM, so we have to
make requests to the firmware to do so from the VPU side.  This will
let us replace fixed clocks in our DT with actual clock control (and
correct frequency information).

Signed-off-by: Eric Anholt <eric@anholt.net>
---
 .../bindings/clock/raspberrypi,firmware-clocks.txt | 24 ++++++++++++++++++++++
 1 file changed, 24 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/raspberrypi,firmware-clocks.txt

diff --git a/Documentation/devicetree/bindings/clock/raspberrypi,firmware-clocks.txt b/Documentation/devicetree/bindings/clock/raspberrypi,firmware-clocks.txt
new file mode 100644
index 0000000..9e1f21b
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/raspberrypi,firmware-clocks.txt
@@ -0,0 +1,24 @@
+Raspberry Pi firmware clock provider.
+
+The Raspberry Pi architecture doesn't provide direct access to the
+CLOCKMAN peripheral from the ARM side, so Linux has to make requests
+to the VPU firmware to program them.
+
+This binding uses the common clock binding:
+Documentation/devicetree/bindings/clock/clock-bindings.txt
+
+Required properties:
+- compatible:	Should be "raspberrypi,firmware-clocks"
+
+- #clock-cells:	Shall have value <1>.  The permitted clock-specifier values
+		  can be found in include/dt-bindings/clk/raspberrypi.h.
+
+- firmware:	Phandle to the firmware driver node.
+
+Example:
+
+firmware_clocks: firmware-clocks {
+	compatible = "raspberrypi,firmware-clocks";
+	#clock-cells = <1>;
+	firmware = <&firmware>;
+};
-- 
2.1.4

[PATCH 2/7] ARM: bcm2835: Add a Raspberry Pi-specific clock driver.

[Eric Anholt]

Unfortunately, the clock manager's registers are not accessible by the
ARM, so we have to request that the firmware modify our clocks for us.

This driver only registers the clocks at the point they are requested
by a client driver.  This is partially to support returning
-EPROBE_DEFER when the firmware driver isn't supported yet, but it
also avoids issues with disabling "unused" clocks due to them not yet
being connected to their consumers in the DT.

Signed-off-by: Eric Anholt <eric@anholt.net>
---
 drivers/clk/Makefile                  |   1 +
 drivers/clk/clk-raspberrypi.c         | 241 ++++++++++++++++++++++++++++++++++
 include/dt-bindings/clk/raspberrypi.h |  23 ++++
 3 files changed, 265 insertions(+)
 create mode 100644 drivers/clk/clk-raspberrypi.c
 create mode 100644 include/dt-bindings/clk/raspberrypi.h

diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index 3d00c25..6a5dafa 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -20,6 +20,7 @@ obj-$(CONFIG_MACH_ASM9260)		+= clk-asm9260.o
 obj-$(CONFIG_COMMON_CLK_AXI_CLKGEN)	+= clk-axi-clkgen.o
 obj-$(CONFIG_ARCH_AXXIA)		+= clk-axm5516.o
 obj-$(CONFIG_ARCH_BCM2835)		+= clk-bcm2835.o
+obj-$(CONFIG_ARCH_BCM2835)		+= clk-raspberrypi.o
 obj-$(CONFIG_COMMON_CLK_CDCE706)	+= clk-cdce706.o
 obj-$(CONFIG_ARCH_CLPS711X)		+= clk-clps711x.o
 obj-$(CONFIG_ARCH_EFM32)		+= clk-efm32gg.o
diff --git a/drivers/clk/clk-raspberrypi.c b/drivers/clk/clk-raspberrypi.c
new file mode 100644
index 0000000..5745875
--- /dev/null
+++ b/drivers/clk/clk-raspberrypi.c
@@ -0,0 +1,241 @@
+/*
+ * Copyright © 2015 Broadcom
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+/*
+ * Implements a clock provider for the clocks controlled by the
+ * firmware on Raspberry Pi.
+ *
+ * These clocks are controlled by the CLOCKMAN peripheral in the
+ * hardware, but the ARM doesn't have access to the registers for
+ * them.  As a result, we have to call into the firmware to get it to
+ * enable, disable, and set their frequencies.
+ *
+ * We don't have an interface for getting the set of frequencies
+ * available from the hardware.  We can request a min/max, but other
+ * than that we have to request a frequency and take what it gives us.
+ */
+
+#include <dt-bindings/clk/raspberrypi.h>
+#include <linux/clk-provider.h>
+#include <soc/bcm2835/raspberrypi-firmware-property.h>
+
+struct rpi_firmware_clock {
+	/* Clock definitions in our static struct. */
+	int clock_id;
+	const char *name;
+
+	/* The rest are filled in at init time. */
+	struct clk_hw hw;
+	struct device *dev;
+	struct device_node *firmware_node;
+};
+
+static struct rpi_firmware_clock rpi_clocks[] = {
+	[RPI_CLOCK_EMMC] = { 1, "emmc" },
+	[RPI_CLOCK_UART0] = { 2, "uart0" },
+	[RPI_CLOCK_ARM] = { 3, "arm" },
+	[RPI_CLOCK_CORE] = { 4, "core" },
+	[RPI_CLOCK_V3D] = { 5, "v3d" },
+	[RPI_CLOCK_H264] = { 6, "h264" },
+	[RPI_CLOCK_ISP] = { 7, "isp" },
+	[RPI_CLOCK_SDRAM] = { 8, "sdram" },
+	[RPI_CLOCK_PIXEL] = { 9, "pixel" },
+	[RPI_CLOCK_PWM] = { 10, "pwm" },
+};
+
+static int rpi_clk_is_on(struct clk_hw *hw)
+{
+	struct rpi_firmware_clock *rpi_clk =
+		container_of(hw, struct rpi_firmware_clock, hw);
+	u32 packet[2];
+	int ret;
+
+	packet[0] = rpi_clk->clock_id;
+	packet[1] = 0;
+	ret = rpi_firmware_property(rpi_clk->firmware_node,
+				    RPI_FIRMWARE_GET_CLOCK_STATE,
+				    &packet, sizeof(packet));
+	if (ret) {
+		dev_err(rpi_clk->dev, "Failed to get clock state\n");
+		return 0;
+	}
+
+	return packet[1] != 0;
+}
+
+static int rpi_clk_set_state(struct rpi_firmware_clock *rpi_clk, bool on)
+{
+	u32 packet[2];
+	int ret;
+
+	packet[0] = rpi_clk->clock_id;
+	packet[1] = on;
+	ret = rpi_firmware_property(rpi_clk->firmware_node,
+				    RPI_FIRMWARE_SET_CLOCK_STATE,
+				    &packet, sizeof(packet));
+	if (ret || (packet[1] & (1 << 1))) {
+		dev_err(rpi_clk->dev, "Failed to set clock state\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int rpi_clk_on(struct clk_hw *hw)
+{
+	struct rpi_firmware_clock *rpi_clk =
+		container_of(hw, struct rpi_firmware_clock, hw);
+
+	return rpi_clk_set_state(rpi_clk, true);
+}
+
+static void rpi_clk_off(struct clk_hw *hw)
+{
+	struct rpi_firmware_clock *rpi_clk =
+		container_of(hw, struct rpi_firmware_clock, hw);
+
+	rpi_clk_set_state(rpi_clk, false);
+}
+
+static unsigned long rpi_clk_get_rate(struct clk_hw *hw,
+					      unsigned long parent_rate)
+{
+	struct rpi_firmware_clock *rpi_clk =
+		container_of(hw, struct rpi_firmware_clock, hw);
+	u32 packet[2];
+	int ret;
+
+	packet[0] = rpi_clk->clock_id;
+	packet[1] = 0;
+	ret = rpi_firmware_property(rpi_clk->firmware_node,
+				    RPI_FIRMWARE_GET_CLOCK_RATE,
+				    &packet, sizeof(packet));
+	if (ret) {
+		dev_err(rpi_clk->dev, "Failed to get clock rate\n");
+		return 0;
+	}
+
+	return packet[1];
+}
+
+static int rpi_clk_set_rate(struct clk_hw *hw,
+			    unsigned long rate, unsigned long parent_rate)
+{
+	struct rpi_firmware_clock *rpi_clk =
+		container_of(hw, struct rpi_firmware_clock, hw);
+	u32 packet[2];
+	int ret;
+
+	packet[0] = rpi_clk->clock_id;
+	packet[1] = rate;
+	ret = rpi_firmware_property(rpi_clk->firmware_node,
+				    RPI_FIRMWARE_SET_CLOCK_RATE,
+				    &packet, sizeof(packet));
+	if (ret) {
+		dev_err(rpi_clk->dev, "Failed to set clock rate\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static long rpi_clk_round_rate(struct clk_hw *hw, unsigned long rate,
+			       unsigned long *parent_rate)
+{
+	/*
+	 * The firmware will end up rounding our rate to something,
+	 * but we don't have an interface for it.  Just return the
+	 * requested value, and it'll get updated after the clock gets
+	 * set.
+	 */
+	return rate;
+}
+
+static struct clk_ops rpi_clk_ops = {
+	.is_prepared = rpi_clk_is_on,
+	.prepare = rpi_clk_on,
+	.unprepare = rpi_clk_off,
+	.recalc_rate = rpi_clk_get_rate,
+	.set_rate = rpi_clk_set_rate,
+	.round_rate = rpi_clk_round_rate,
+};
+
+DEFINE_MUTEX(delayed_clock_init);
+static struct clk *rpi_firmware_delayed_get_clk(struct of_phandle_args *clkspec,
+						void *_data)
+{
+	struct device_node *of_node = _data;
+	struct platform_device *pdev = of_find_device_by_node(of_node);
+	struct device *dev = &pdev->dev;
+	struct device_node *firmware_node;
+	struct clk_init_data init;
+	struct rpi_firmware_clock *rpi_clk;
+	struct clk *ret_clk;
+	int ret;
+
+	if (clkspec->args_count != 1) {
+		dev_err(dev, "clock phandle should have 1 argument\n");
+		return ERR_PTR(-ENODEV);
+	}
+
+	if (clkspec->args[0] >= ARRAY_SIZE(rpi_clocks)) {
+		dev_err(dev, "clock phandle index %d too large\n",
+			clkspec->args[0]);
+		return ERR_PTR(-ENODEV);
+	}
+
+	rpi_clk = &rpi_clocks[clkspec->args[0]];
+
+	firmware_node = of_parse_phandle(of_node, "firmware", 0);
+	if (!firmware_node) {
+		dev_err(dev, "%s: Missing firmware node\n", rpi_clk->name);
+		return ERR_PTR(-ENODEV);
+	}
+
+	/* Try a no-op transaction to see if the driver is loaded yet. */
+	ret = rpi_firmware_property_list(firmware_node, NULL, 0);
+	if (ret)
+		return ERR_PTR(ret);
+
+	mutex_lock(&delayed_clock_init);
+	if (rpi_clk->hw.clk) {
+		mutex_unlock(&delayed_clock_init);
+		return rpi_clk->hw.clk;
+	}
+	memset(&init, 0, sizeof(init));
+	init.ops = &rpi_clk_ops;
+
+	rpi_clk->firmware_node = firmware_node;
+	rpi_clk->dev = dev;
+	rpi_clk->hw.init = &init;
+	init.name = rpi_clk->name;
+	init.flags = CLK_IS_ROOT;
+
+	ret_clk = clk_register(dev, &rpi_clk->hw);
+	mutex_unlock(&delayed_clock_init);
+	if (!IS_ERR(ret_clk))
+		dev_info(dev, "registered %s clock\n", rpi_clk->name);
+	else {
+		dev_err(dev, "%s clock failed to init: %ld\n", rpi_clk->name,
+			PTR_ERR(ret_clk));
+	}
+	return ret_clk;
+}
+
+void __init rpi_firmware_init_clock_provider(struct device_node *node)
+{
+	/* We delay construction of our struct clks until get time,
+	 * because we need to be able to return -EPROBE_DEFER if the
+	 * firmware driver isn't up yet.  clk core doesn't support
+	 * re-probing on -EPROBE_DEFER, but callers of clk_get can.
+	 */
+	of_clk_add_provider(node, rpi_firmware_delayed_get_clk, node);
+}
+
+CLK_OF_DECLARE(rpi_firmware_clocks, "raspberrypi,firmware-clocks",
+	       rpi_firmware_init_clock_provider);
diff --git a/include/dt-bindings/clk/raspberrypi.h b/include/dt-bindings/clk/raspberrypi.h
new file mode 100644
index 0000000..c9fa85c
--- /dev/null
+++ b/include/dt-bindings/clk/raspberrypi.h
@@ -0,0 +1,23 @@
+#/*
+ *  Copyright © 2015 Broadcom
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef _DT_BINDINGS_CLK_RASPBERRYPI_H
+#define _DT_BINDINGS_CLK_RASPBERRYPI_H
+
+#define RPI_CLOCK_EMMC	0
+#define RPI_CLOCK_UART0	1
+#define RPI_CLOCK_ARM	2
+#define RPI_CLOCK_CORE	3
+#define RPI_CLOCK_V3D	4
+#define RPI_CLOCK_H264	5
+#define RPI_CLOCK_ISP	6
+#define RPI_CLOCK_SDRAM	7
+#define RPI_CLOCK_PIXEL	8
+#define RPI_CLOCK_PWM	9
+
+#endif
-- 
2.1.4

[PATCH 3/7] ARM: bcm2835: Add DT for the firmware clocks driver.

[Eric Anholt]

Signed-off-by: Eric Anholt <eric@anholt.net>
---
 arch/arm/boot/dts/bcm2835-rpi.dtsi | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm/boot/dts/bcm2835-rpi.dtsi b/arch/arm/boot/dts/bcm2835-rpi.dtsi
index ace33f6..ac5f84c 100644
--- a/arch/arm/boot/dts/bcm2835-rpi.dtsi
+++ b/arch/arm/boot/dts/bcm2835-rpi.dtsi
@@ -20,6 +20,12 @@
 			compatible = "raspberrypi,firmware";
 			mboxes = <&mailbox>;
 		};
+
+		firmware_clocks: firmware-clocks {
+			compatible = "raspberrypi,firmware-clocks";
+			#clock-cells = <1>;
+			firmware = <&firmware>;
+		};
 	};
 };
 
-- 
2.1.4

[PATCH 4/7] ARM: bcm2835: Drop never-used clock-frequency property of uart0.

[Eric Anholt]

This appears to have been copy-and-paste from another serial driver's
DT.  The driver has never used this value -- instead, the pl011 driver
is getting the fixed 20201000.uart clock from clk-bcm2835.c.

Signed-off-by: Eric Anholt <eric@anholt.net>
---
 arch/arm/boot/dts/bcm2835.dtsi | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/arm/boot/dts/bcm2835.dtsi b/arch/arm/boot/dts/bcm2835.dtsi
index 301c73f..32f5830 100644
--- a/arch/arm/boot/dts/bcm2835.dtsi
+++ b/arch/arm/boot/dts/bcm2835.dtsi
@@ -96,7 +96,6 @@
 			compatible = "brcm,bcm2835-pl011", "arm,pl011", "arm,primecell";
 			reg = <0x7e201000 0x1000>;
 			interrupts = <2 25>;
-			clock-frequency = <3000000>;
 			arm,primecell-periphid = <0x00241011>;
 		};
 
-- 
2.1.4

[PATCH 5/7] ARM: bcm2835: Drop the fixed sys_pclk.

[Eric Anholt]

Nothing uses it, and I can't find any evidence that anything ever has.

Signed-off-by: Eric Anholt <eric@anholt.net>
---
 drivers/clk/clk-bcm2835.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/clk/clk-bcm2835.c b/drivers/clk/clk-bcm2835.c
index 6b950ca..dd295e4 100644
--- a/drivers/clk/clk-bcm2835.c
+++ b/drivers/clk/clk-bcm2835.c
@@ -32,11 +32,6 @@ void __init bcm2835_init_clocks(void)
 	struct clk *clk;
 	int ret;
 
-	clk = clk_register_fixed_rate(NULL, "sys_pclk", NULL, CLK_IS_ROOT,
-					250000000);
-	if (IS_ERR(clk))
-		pr_err("sys_pclk not registered\n");
-
 	clk = clk_register_fixed_rate(NULL, "apb_pclk", NULL, CLK_IS_ROOT,
 					126000000);
 	if (IS_ERR(clk))
-- 
2.1.4

[PATCH 6/7] ARM: bcm2835: Use the RPi firmware clocks for uart.

[Eric Anholt]

This gets us a correct apb_pclk, which previously was accidentally
using the "20201000.uart" clock from clk-bcm2835.c, due to the
fallback clk_get_sys() path.

Signed-off-by: Eric Anholt <eric@anholt.net>
---
 arch/arm/boot/dts/bcm2835-rpi.dtsi | 7 +++++++
 arch/arm/boot/dts/bcm2835.dtsi     | 2 +-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/bcm2835-rpi.dtsi b/arch/arm/boot/dts/bcm2835-rpi.dtsi
index ac5f84c..53285a3 100644
--- a/arch/arm/boot/dts/bcm2835-rpi.dtsi
+++ b/arch/arm/boot/dts/bcm2835-rpi.dtsi
@@ -1,3 +1,4 @@
+#include <dt-bindings/clk/raspberrypi.h>
 #include "bcm2835.dtsi"
 
 / {
@@ -62,3 +63,9 @@
 	status = "okay";
 	bus-width = <4>;
 };
+
+&uart0 {
+	clocks = <&firmware_clocks RPI_CLOCK_UART0>,
+		 <&firmware_clocks RPI_CLOCK_CORE>;
+	clock-names = "uartclk", "apb_pclk";
+};
diff --git a/arch/arm/boot/dts/bcm2835.dtsi b/arch/arm/boot/dts/bcm2835.dtsi
index 32f5830..5be2862 100644
--- a/arch/arm/boot/dts/bcm2835.dtsi
+++ b/arch/arm/boot/dts/bcm2835.dtsi
@@ -92,7 +92,7 @@
 			#interrupt-cells = <2>;
 		};
 
-		uart@7e201000 {
+		uart0: uart@7e201000 {
 			compatible = "brcm,bcm2835-pl011", "arm,pl011", "arm,primecell";
 			reg = <0x7e201000 0x1000>;
 			interrupts = <2 25>;
-- 
2.1.4

[PATCH 7/7] ARM: bcm2835: Tie SPI clock to the core clock rate.

[Eric Anholt]

We were previously using a fixed clock declared in the 2835 DT, but
it's actually the core clock, and it might not be the same if you had
adjusted it using the firmware's config.txt.

Signed-off-by: Eric Anholt <eric@anholt.net>
---

This is the only patch in the series I haven't really tested, since I
don't have any SPI devices.

 arch/arm/boot/dts/bcm2835-rpi.dtsi | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm/boot/dts/bcm2835-rpi.dtsi b/arch/arm/boot/dts/bcm2835-rpi.dtsi
index 53285a3..994c8e7 100644
--- a/arch/arm/boot/dts/bcm2835-rpi.dtsi
+++ b/arch/arm/boot/dts/bcm2835-rpi.dtsi
@@ -69,3 +69,7 @@
 		 <&firmware_clocks RPI_CLOCK_CORE>;
 	clock-names = "uartclk", "apb_pclk";
 };
+
+&spi {
+	clocks = <&firmware_clocks RPI_CLOCK_CORE>;
+};
-- 
2.1.4

Re: [PATCH 2/7] ARM: bcm2835: Add a Raspberry Pi-specific clock driver.

[Baruch Siach]

Hi Eric,

On Mon, May 18, 2015 at 12:43:34PM -0700, Eric Anholt wrote:
> +DEFINE_MUTEX(delayed_clock_init);

Static?

baruch

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

[PATCH 2/7 v2] ARM: bcm2835: Add a Raspberry Pi-specific clock driver.

[Eric Anholt]

Unfortunately, the clock manager's registers are not accessible by the
ARM, so we have to request that the firmware modify our clocks for us.

This driver only registers the clocks at the point they are requested
by a client driver.  This is partially to support returning
-EPROBE_DEFER when the firmware driver isn't supported yet, but it
also avoids issues with disabling "unused" clocks due to them not yet
being connected to their consumers in the DT.

Signed-off-by: Eric Anholt <eric@anholt.net>
---

v2: Declare the mutex static (from review by Baruch Siach), merge
    description and copyright comments.

 drivers/clk/Makefile                  |   1 +
 drivers/clk/clk-raspberrypi.c         | 239 ++++++++++++++++++++++++++++++++++
 include/dt-bindings/clk/raspberrypi.h |  23 ++++
 3 files changed, 263 insertions(+)
 create mode 100644 drivers/clk/clk-raspberrypi.c
 create mode 100644 include/dt-bindings/clk/raspberrypi.h

diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index 3d00c25..6a5dafa 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -20,6 +20,7 @@ obj-$(CONFIG_MACH_ASM9260)		+= clk-asm9260.o
 obj-$(CONFIG_COMMON_CLK_AXI_CLKGEN)	+= clk-axi-clkgen.o
 obj-$(CONFIG_ARCH_AXXIA)		+= clk-axm5516.o
 obj-$(CONFIG_ARCH_BCM2835)		+= clk-bcm2835.o
+obj-$(CONFIG_ARCH_BCM2835)		+= clk-raspberrypi.o
 obj-$(CONFIG_COMMON_CLK_CDCE706)	+= clk-cdce706.o
 obj-$(CONFIG_ARCH_CLPS711X)		+= clk-clps711x.o
 obj-$(CONFIG_ARCH_EFM32)		+= clk-efm32gg.o
diff --git a/drivers/clk/clk-raspberrypi.c b/drivers/clk/clk-raspberrypi.c
new file mode 100644
index 0000000..b9d8f44
--- /dev/null
+++ b/drivers/clk/clk-raspberrypi.c
@@ -0,0 +1,239 @@
+/*
+ * Implements a clock provider for the clocks controlled by the
+ * firmware on Raspberry Pi.
+ *
+ * These clocks are controlled by the CLOCKMAN peripheral in the
+ * hardware, but the ARM doesn't have access to the registers for
+ * them.  As a result, we have to call into the firmware to get it to
+ * enable, disable, and set their frequencies.
+ *
+ * We don't have an interface for getting the set of frequencies
+ * available from the hardware.  We can request a min/max, but other
+ * than that we have to request a frequency and take what it gives us.
+ *
+ * Copyright © 2015 Broadcom
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <dt-bindings/clk/raspberrypi.h>
+#include <linux/clk-provider.h>
+#include <soc/bcm2835/raspberrypi-firmware-property.h>
+
+struct rpi_firmware_clock {
+	/* Clock definitions in our static struct. */
+	int clock_id;
+	const char *name;
+
+	/* The rest are filled in at init time. */
+	struct clk_hw hw;
+	struct device *dev;
+	struct device_node *firmware_node;
+};
+
+static struct rpi_firmware_clock rpi_clocks[] = {
+	[RPI_CLOCK_EMMC] = { 1, "emmc" },
+	[RPI_CLOCK_UART0] = { 2, "uart0" },
+	[RPI_CLOCK_ARM] = { 3, "arm" },
+	[RPI_CLOCK_CORE] = { 4, "core" },
+	[RPI_CLOCK_V3D] = { 5, "v3d" },
+	[RPI_CLOCK_H264] = { 6, "h264" },
+	[RPI_CLOCK_ISP] = { 7, "isp" },
+	[RPI_CLOCK_SDRAM] = { 8, "sdram" },
+	[RPI_CLOCK_PIXEL] = { 9, "pixel" },
+	[RPI_CLOCK_PWM] = { 10, "pwm" },
+};
+
+static int rpi_clk_is_on(struct clk_hw *hw)
+{
+	struct rpi_firmware_clock *rpi_clk =
+		container_of(hw, struct rpi_firmware_clock, hw);
+	u32 packet[2];
+	int ret;
+
+	packet[0] = rpi_clk->clock_id;
+	packet[1] = 0;
+	ret = rpi_firmware_property(rpi_clk->firmware_node,
+				    RPI_FIRMWARE_GET_CLOCK_STATE,
+				    &packet, sizeof(packet));
+	if (ret) {
+		dev_err(rpi_clk->dev, "Failed to get clock state\n");
+		return 0;
+	}
+
+	return packet[1] != 0;
+}
+
+static int rpi_clk_set_state(struct rpi_firmware_clock *rpi_clk, bool on)
+{
+	u32 packet[2];
+	int ret;
+
+	packet[0] = rpi_clk->clock_id;
+	packet[1] = on;
+	ret = rpi_firmware_property(rpi_clk->firmware_node,
+				    RPI_FIRMWARE_SET_CLOCK_STATE,
+				    &packet, sizeof(packet));
+	if (ret || (packet[1] & (1 << 1))) {
+		dev_err(rpi_clk->dev, "Failed to set clock state\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int rpi_clk_on(struct clk_hw *hw)
+{
+	struct rpi_firmware_clock *rpi_clk =
+		container_of(hw, struct rpi_firmware_clock, hw);
+
+	return rpi_clk_set_state(rpi_clk, true);
+}
+
+static void rpi_clk_off(struct clk_hw *hw)
+{
+	struct rpi_firmware_clock *rpi_clk =
+		container_of(hw, struct rpi_firmware_clock, hw);
+
+	rpi_clk_set_state(rpi_clk, false);
+}
+
+static unsigned long rpi_clk_get_rate(struct clk_hw *hw,
+					      unsigned long parent_rate)
+{
+	struct rpi_firmware_clock *rpi_clk =
+		container_of(hw, struct rpi_firmware_clock, hw);
+	u32 packet[2];
+	int ret;
+
+	packet[0] = rpi_clk->clock_id;
+	packet[1] = 0;
+	ret = rpi_firmware_property(rpi_clk->firmware_node,
+				    RPI_FIRMWARE_GET_CLOCK_RATE,
+				    &packet, sizeof(packet));
+	if (ret) {
+		dev_err(rpi_clk->dev, "Failed to get clock rate\n");
+		return 0;
+	}
+
+	return packet[1];
+}
+
+static int rpi_clk_set_rate(struct clk_hw *hw,
+			    unsigned long rate, unsigned long parent_rate)
+{
+	struct rpi_firmware_clock *rpi_clk =
+		container_of(hw, struct rpi_firmware_clock, hw);
+	u32 packet[2];
+	int ret;
+
+	packet[0] = rpi_clk->clock_id;
+	packet[1] = rate;
+	ret = rpi_firmware_property(rpi_clk->firmware_node,
+				    RPI_FIRMWARE_SET_CLOCK_RATE,
+				    &packet, sizeof(packet));
+	if (ret) {
+		dev_err(rpi_clk->dev, "Failed to set clock rate\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static long rpi_clk_round_rate(struct clk_hw *hw, unsigned long rate,
+			       unsigned long *parent_rate)
+{
+	/*
+	 * The firmware will end up rounding our rate to something,
+	 * but we don't have an interface for it.  Just return the
+	 * requested value, and it'll get updated after the clock gets
+	 * set.
+	 */
+	return rate;
+}
+
+static struct clk_ops rpi_clk_ops = {
+	.is_prepared = rpi_clk_is_on,
+	.prepare = rpi_clk_on,
+	.unprepare = rpi_clk_off,
+	.recalc_rate = rpi_clk_get_rate,
+	.set_rate = rpi_clk_set_rate,
+	.round_rate = rpi_clk_round_rate,
+};
+
+static DEFINE_MUTEX(delayed_clock_init);
+static struct clk *rpi_firmware_delayed_get_clk(struct of_phandle_args *clkspec,
+						void *_data)
+{
+	struct device_node *of_node = _data;
+	struct platform_device *pdev = of_find_device_by_node(of_node);
+	struct device *dev = &pdev->dev;
+	struct device_node *firmware_node;
+	struct clk_init_data init;
+	struct rpi_firmware_clock *rpi_clk;
+	struct clk *ret_clk;
+	int ret;
+
+	if (clkspec->args_count != 1) {
+		dev_err(dev, "clock phandle should have 1 argument\n");
+		return ERR_PTR(-ENODEV);
+	}
+
+	if (clkspec->args[0] >= ARRAY_SIZE(rpi_clocks)) {
+		dev_err(dev, "clock phandle index %d too large\n",
+			clkspec->args[0]);
+		return ERR_PTR(-ENODEV);
+	}
+
+	rpi_clk = &rpi_clocks[clkspec->args[0]];
+
+	firmware_node = of_parse_phandle(of_node, "firmware", 0);
+	if (!firmware_node) {
+		dev_err(dev, "%s: Missing firmware node\n", rpi_clk->name);
+		return ERR_PTR(-ENODEV);
+	}
+
+	/* Try a no-op transaction to see if the driver is loaded yet. */
+	ret = rpi_firmware_property_list(firmware_node, NULL, 0);
+	if (ret)
+		return ERR_PTR(ret);
+
+	mutex_lock(&delayed_clock_init);
+	if (rpi_clk->hw.clk) {
+		mutex_unlock(&delayed_clock_init);
+		return rpi_clk->hw.clk;
+	}
+	memset(&init, 0, sizeof(init));
+	init.ops = &rpi_clk_ops;
+
+	rpi_clk->firmware_node = firmware_node;
+	rpi_clk->dev = dev;
+	rpi_clk->hw.init = &init;
+	init.name = rpi_clk->name;
+	init.flags = CLK_IS_ROOT;
+
+	ret_clk = clk_register(dev, &rpi_clk->hw);
+	mutex_unlock(&delayed_clock_init);
+	if (!IS_ERR(ret_clk))
+		dev_info(dev, "registered %s clock\n", rpi_clk->name);
+	else {
+		dev_err(dev, "%s clock failed to init: %ld\n", rpi_clk->name,
+			PTR_ERR(ret_clk));
+	}
+	return ret_clk;
+}
+
+void __init rpi_firmware_init_clock_provider(struct device_node *node)
+{
+	/* We delay construction of our struct clks until get time,
+	 * because we need to be able to return -EPROBE_DEFER if the
+	 * firmware driver isn't up yet.  clk core doesn't support
+	 * re-probing on -EPROBE_DEFER, but callers of clk_get can.
+	 */
+	of_clk_add_provider(node, rpi_firmware_delayed_get_clk, node);
+}
+
+CLK_OF_DECLARE(rpi_firmware_clocks, "raspberrypi,firmware-clocks",
+	       rpi_firmware_init_clock_provider);
diff --git a/include/dt-bindings/clk/raspberrypi.h b/include/dt-bindings/clk/raspberrypi.h
new file mode 100644
index 0000000..c9fa85c
--- /dev/null
+++ b/include/dt-bindings/clk/raspberrypi.h
@@ -0,0 +1,23 @@
+#/*
+ *  Copyright © 2015 Broadcom
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef _DT_BINDINGS_CLK_RASPBERRYPI_H
+#define _DT_BINDINGS_CLK_RASPBERRYPI_H
+
+#define RPI_CLOCK_EMMC	0
+#define RPI_CLOCK_UART0	1
+#define RPI_CLOCK_ARM	2
+#define RPI_CLOCK_CORE	3
+#define RPI_CLOCK_V3D	4
+#define RPI_CLOCK_H264	5
+#define RPI_CLOCK_ISP	6
+#define RPI_CLOCK_SDRAM	7
+#define RPI_CLOCK_PIXEL	8
+#define RPI_CLOCK_PWM	9
+
+#endif
-- 
2.1.4

Re: [PATCH 1/7] dt/bindings: Add binding for the Raspberry Pi clock provider

[Stephen Warren]

On 05/18/2015 01:43 PM, Eric Anholt wrote:
> The hardware clocks are not controllable by the ARM, so we have to
> make requests to the firmware to do so from the VPU side.  This will
> let us replace fixed clocks in our DT with actual clock control (and
> correct frequency information).

> diff --git a/Documentation/devicetree/bindings/clock/raspberrypi,firmware-clocks.txt b/Documentation/devicetree/bindings/clock/raspberrypi,firmware-clocks.txt

> +Required properties:
...
> +- #clock-cells:	Shall have value <1>.  The permitted clock-specifier values
> +		  can be found in include/dt-bindings/clk/raspberrypi.h.

That file is explicitly part of the binding; any driver that implements
this binding needs to use the same values. As such, that file would
typically be part of the patch that adds the binding doc, not part of
the patch that implements the driver.

No need to respin the series just because of that though.

> +- firmware:	Phandle to the firmware driver node.

"firmware" is rather generic. I'd suggest a vendor prefix for the
property name; raspberrypi,firmware.

Re: [PATCH 2/7] ARM: bcm2835: Add a Raspberry Pi-specific clock driver.

[Stephen Warren]

On 05/18/2015 01:43 PM, Eric Anholt wrote:
> Unfortunately, the clock manager's registers are not accessible by the
> ARM, so we have to request that the firmware modify our clocks for us.
> 
> This driver only registers the clocks at the point they are requested
> by a client driver.  This is partially to support returning
> -EPROBE_DEFER when the firmware driver isn't supported yet, but it
> also avoids issues with disabling "unused" clocks due to them not yet
> being connected to their consumers in the DT.

It looks like you forgot to CC the clock subsystem maintainers:

M:      Mike Turquette <mturquette@linaro.org>
M:      Stephen Boyd <sboyd@codeaurora.org>

The description above sounds like a neat solution, but has the
disadvantage that the clocks without a client won't show up in debugfs.
I wonder if the clock maintainers know of a better way?

> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile

>  obj-$(CONFIG_ARCH_BCM2835)		+= clk-bcm2835.o
> +obj-$(CONFIG_ARCH_BCM2835)		+= clk-raspberrypi.o

Shouldn't this replace the old legacy code in clk-bcm2835.c?

I wonder if a separate Kconfig symbol is warranted for the clock driver.
Looking at the context in the patch, it's common not to do that though.

> diff --git a/drivers/clk/clk-raspberrypi.c b/drivers/clk/clk-raspberrypi.c

> +struct rpi_firmware_clock {
> +	/* Clock definitions in our static struct. */
> +	int clock_id;

Why not just use the raw HW IDs (from the mailbox protocol) as the ID in
DT and the driver? The only thing that would need to change is to add a
error check for "clkspec->args[0] == 0" in
rpi_firmware_delayed_get_clk(). The advantage would be that
include/dt-bindings/clk/raspberrypi.h would exactly match the firmware
protocol, and hence be easily correlated with the documentation.

> +static int rpi_clk_set_rate(struct clk_hw *hw,
> +			    unsigned long rate, unsigned long parent_rate)
> +{
> +	struct rpi_firmware_clock *rpi_clk =
> +		container_of(hw, struct rpi_firmware_clock, hw);
> +	u32 packet[2];
> +	int ret;
> +
> +	packet[0] = rpi_clk->clock_id;
> +	packet[1] = rate;
> +	ret = rpi_firmware_property(rpi_clk->firmware_node,

The docs indicate there's a third word here "skip setting turbo". Is
that optional?

https://github.com/raspberrypi/firmware/wiki/Mailbox-property-interface

> +static long rpi_clk_round_rate(struct clk_hw *hw, unsigned long rate,
> +			       unsigned long *parent_rate)
> +{
> +	/*
> +	 * The firmware will end up rounding our rate to something,
> +	 * but we don't have an interface for it.  Just return the
> +	 * requested value, and it'll get updated after the clock gets
> +	 * set.
> +	 */
> +	return rate;
> +}

Hmm. I wonder if that will confuse things; I thought the purpose of
round_rate() was so code could know exactly what would happen ahead of time?

> +static struct clk *rpi_firmware_delayed_get_clk(struct of_phandle_args *clkspec,
> +						void *_data)

> +	rpi_clk = &rpi_clocks[clkspec->args[0]];
> +
> +	firmware_node = of_parse_phandle(of_node, "firmware", 0);
> +	if (!firmware_node) {
> +		dev_err(dev, "%s: Missing firmware node\n", rpi_clk->name);
> +		return ERR_PTR(-ENODEV);
> +	}
> +
> +	/* Try a no-op transaction to see if the driver is loaded yet. */
> +	ret = rpi_firmware_property_list(firmware_node, NULL, 0);
> +	if (ret)
> +		return ERR_PTR(ret);

I would move all that into this driver's probe().

> +	init.flags = CLK_IS_ROOT;

Is it possible to add clock parent information to the driver, so the
clocks are all hooked together into the correct tree, rather than all
looking like root clocks?

One of the many reasons I didn't do anything FW-wise for the kernel was
the hope that such information would be forthcoming, and hence we could
have complete kernel drivers.

> +void __init rpi_firmware_init_clock_provider(struct device_node *node)
> +{
> +	/* We delay construction of our struct clks until get time,
> +	 * because we need to be able to return -EPROBE_DEFER if the
> +	 * firmware driver isn't up yet.  clk core doesn't support
> +	 * re-probing on -EPROBE_DEFER, but callers of clk_get can.

Oh, that's nasty. What would it take to fix the clock core to support
deferred probe? It really should.

> +CLK_OF_DECLARE(rpi_firmware_clocks, "raspberrypi,firmware-clocks",
> +	       rpi_firmware_init_clock_provider);

Oh, I guess the same comment as for the firmware node applies re: the
over-genericity of the DT compatible value applies here too. Perhaps
raspberrypi,bcm2835-firmware-clocks to match Lee's proposal for the
firmware node?

Re: [PATCH 5/7] ARM: bcm2835: Drop the fixed sys_pclk.

[Stephen Warren]

On 05/18/2015 01:43 PM, Eric Anholt wrote:
> Nothing uses it, and I can't find any evidence that anything ever has.

Does the clock actually exist though? If it does, it seems reasonable to
keep it.

Re: [PATCH 2/7] ARM: bcm2835: Add a Raspberry Pi-specific clock driver.

[Stephen Boyd]

On 05/28, Stephen Warren wrote:
> On 05/18/2015 01:43 PM, Eric Anholt wrote:
> > Unfortunately, the clock manager's registers are not accessible by the
> > ARM, so we have to request that the firmware modify our clocks for us.
> > 
> > This driver only registers the clocks at the point they are requested
> > by a client driver.  This is partially to support returning
> > -EPROBE_DEFER when the firmware driver isn't supported yet, but it
> > also avoids issues with disabling "unused" clocks due to them not yet
> > being connected to their consumers in the DT.
> 
> It looks like you forgot to CC the clock subsystem maintainers:
> 
> M:      Mike Turquette <mturquette@linaro.org>
> M:      Stephen Boyd <sboyd@codeaurora.org>
> 

Thanks, except I don't have the full patch context here to review
the patch.

> The description above sounds like a neat solution, but has the
> disadvantage that the clocks without a client won't show up in debugfs.
> I wonder if the clock maintainers know of a better way?

Can you mark them as CLK_IGNORE_UNUSED? The probe defer problem
has a solution in sight (see more below).

> > +static long rpi_clk_round_rate(struct clk_hw *hw, unsigned long rate,
> > +			       unsigned long *parent_rate)
> > +{
> > +	/*
> > +	 * The firmware will end up rounding our rate to something,
> > +	 * but we don't have an interface for it.  Just return the
> > +	 * requested value, and it'll get updated after the clock gets
> > +	 * set.
> > +	 */
> > +	return rate;
> > +}
> 
> Hmm. I wonder if that will confuse things; I thought the purpose of
> round_rate() was so code could know exactly what would happen ahead of time?

We have to do the same thing on Qualcomm platforms for FW
controlled clocks. We don't have any insight to what the rate is
going to be. We can only ask the firmware to set a rate and the
firmware guarantees it will be at least that rate.

> 
> > +	init.flags = CLK_IS_ROOT;
> 
> Is it possible to add clock parent information to the driver, so the
> clocks are all hooked together into the correct tree, rather than all
> looking like root clocks?
> 
> One of the many reasons I didn't do anything FW-wise for the kernel was
> the hope that such information would be forthcoming, and hence we could
> have complete kernel drivers.
> 
> > +void __init rpi_firmware_init_clock_provider(struct device_node *node)
> > +{
> > +	/* We delay construction of our struct clks until get time,
> > +	 * because we need to be able to return -EPROBE_DEFER if the
> > +	 * firmware driver isn't up yet.  clk core doesn't support
> > +	 * re-probing on -EPROBE_DEFER, but callers of clk_get can.
> 
> Oh, that's nasty. What would it take to fix the clock core to support
> deferred probe? It really should.

There are patches to support probe defer from clk_get() but they
stalled because sunxi is needs to keep clocks on from their
providing driver (termed "critical clocks"). If we can resolve
the "critical clocks" thing then we should be able to support
probe defer, unless we find other users of orphaned clk
pointers.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 5/7] ARM: bcm2835: Drop the fixed sys_pclk.

[Eric Anholt]

[-- Attachment #1: Type: text/plain, Size: 383 bytes --]

Stephen Warren <swarren@wwwdotorg.org> writes:

> On 05/18/2015 01:43 PM, Eric Anholt wrote:
>> Nothing uses it, and I can't find any evidence that anything ever has.
>
> Does the clock actually exist though? If it does, it seems reasonable to
> keep it.

Yes, it's kept in the form of the core clock in the RPi driver, and the
clock-frequency values in spi and i2c in bcm2835.dtsi.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH 2/7] ARM: bcm2835: Add a Raspberry Pi-specific clock driver.

[Eric Anholt]

[-- Attachment #1: Type: text/plain, Size: 1039 bytes --]

Stephen Warren <swarren@wwwdotorg.org> writes:

> On 05/18/2015 01:43 PM, Eric Anholt wrote:
>> +	init.flags = CLK_IS_ROOT;
>
> Is it possible to add clock parent information to the driver, so the
> clocks are all hooked together into the correct tree, rather than all
> looking like root clocks?
>
> One of the many reasons I didn't do anything FW-wise for the kernel was
> the hope that such information would be forthcoming, and hence we could
> have complete kernel drivers.

As far as I can tell, none of these clocks are the parent of any other.

There's a layer of PLLs, then clockman almost always just dividing off
of those.  In response to a clockman request (which is what this
property tag quickly maps to), only PLLH ever gets changed as a result
of RPI_CLOCK_PIXEL.  PLLH feeds the HDMI and VEC (SDTV).  We don't have
an SDTV clock for us to control through these interfaces, so there are
no conflicts that I can see.  Only two clockman clocks I see divide off
of on other clockman clocks, and we don't have access to those.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH 2/7] ARM: bcm2835: Add a Raspberry Pi-specific clock driver.

[Eric Anholt]

[-- Attachment #1: Type: text/plain, Size: 2850 bytes --]

Stephen Boyd <sboyd@codeaurora.org> writes:

> On 05/28, Stephen Warren wrote:
>> On 05/18/2015 01:43 PM, Eric Anholt wrote:
>> > Unfortunately, the clock manager's registers are not accessible by the
>> > ARM, so we have to request that the firmware modify our clocks for us.
>> > 
>> > This driver only registers the clocks at the point they are requested
>> > by a client driver.  This is partially to support returning
>> > -EPROBE_DEFER when the firmware driver isn't supported yet, but it
>> > also avoids issues with disabling "unused" clocks due to them not yet
>> > being connected to their consumers in the DT.
>> 
>> It looks like you forgot to CC the clock subsystem maintainers:
>> 
>> M:      Mike Turquette <mturquette@linaro.org>
>> M:      Stephen Boyd <sboyd@codeaurora.org>
>> 
>
> Thanks, except I don't have the full patch context here to review
> the patch.
>
>> The description above sounds like a neat solution, but has the
>> disadvantage that the clocks without a client won't show up in debugfs.
>> I wonder if the clock maintainers know of a better way?
>
> Can you mark them as CLK_IGNORE_UNUSED? The probe defer problem
> has a solution in sight (see more below).

>> > +	init.flags = CLK_IS_ROOT;
>> 
>> Is it possible to add clock parent information to the driver, so the
>> clocks are all hooked together into the correct tree, rather than all
>> looking like root clocks?
>> 
>> One of the many reasons I didn't do anything FW-wise for the kernel was
>> the hope that such information would be forthcoming, and hence we could
>> have complete kernel drivers.
>> 
>> > +void __init rpi_firmware_init_clock_provider(struct device_node *node)
>> > +{
>> > +	/* We delay construction of our struct clks until get time,
>> > +	 * because we need to be able to return -EPROBE_DEFER if the
>> > +	 * firmware driver isn't up yet.  clk core doesn't support
>> > +	 * re-probing on -EPROBE_DEFER, but callers of clk_get can.
>> 
>> Oh, that's nasty. What would it take to fix the clock core to support
>> deferred probe? It really should.
>
> There are patches to support probe defer from clk_get() but they
> stalled because sunxi is needs to keep clocks on from their
> providing driver (termed "critical clocks"). If we can resolve
> the "critical clocks" thing then we should be able to support
> probe defer, unless we find other users of orphaned clk
> pointers.

Great!  I'm certainly happy to switch to a normal registration of all my
clocks once -EPROBE_DEFER works from the clock provider init.

If those patches aren't landing this release, that also gives us a
release to wire up all the clock consumers in the DT before we get hit
by stable DT ABI, so we'll be able to give a nice limited set of
CLOCK_IGNORE_UNUSED in the flags when we transition.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH 2/7] ARM: bcm2835: Add a Raspberry Pi-specific clock driver.

[Eric Anholt]

[-- Attachment #1: Type: text/plain, Size: 3644 bytes --]

Stephen Warren <swarren@wwwdotorg.org> writes:

> On 05/18/2015 01:43 PM, Eric Anholt wrote:
>>  obj-$(CONFIG_ARCH_BCM2835)		+= clk-bcm2835.o
>> +obj-$(CONFIG_ARCH_BCM2835)		+= clk-raspberrypi.o
>
> Shouldn't this replace the old legacy code in clk-bcm2835.c?

I don't think we can, because of DT backwards compat (Sigh).
>
> I wonder if a separate Kconfig symbol is warranted for the clock driver.
> Looking at the context in the patch, it's common not to do that though.

I've moved it under RASPBERRYPI_FIRMWARE, since it needs that to build.

>> diff --git a/drivers/clk/clk-raspberrypi.c b/drivers/clk/clk-raspberrypi.c
>
>> +struct rpi_firmware_clock {
>> +	/* Clock definitions in our static struct. */
>> +	int clock_id;
>
> Why not just use the raw HW IDs (from the mailbox protocol) as the ID in
> DT and the driver? The only thing that would need to change is to add a
> error check for "clkspec->args[0] == 0" in
> rpi_firmware_delayed_get_clk(). The advantage would be that
> include/dt-bindings/clk/raspberrypi.h would exactly match the firmware
> protocol, and hence be easily correlated with the documentation.

Done.

>> +static int rpi_clk_set_rate(struct clk_hw *hw,
>> +			    unsigned long rate, unsigned long parent_rate)
>> +{
>> +	struct rpi_firmware_clock *rpi_clk =
>> +		container_of(hw, struct rpi_firmware_clock, hw);
>> +	u32 packet[2];
>> +	int ret;
>> +
>> +	packet[0] = rpi_clk->clock_id;
>> +	packet[1] = rate;
>> +	ret = rpi_firmware_property(rpi_clk->firmware_node,
>
> The docs indicate there's a third word here "skip setting turbo". Is
> that optional?
>
> https://github.com/raspberrypi/firmware/wiki/Mailbox-property-interface

Yeah, it's optional based on the buffer size specified in the packet.

>> +static long rpi_clk_round_rate(struct clk_hw *hw, unsigned long rate,
>> +			       unsigned long *parent_rate)
>> +{
>> +	/*
>> +	 * The firmware will end up rounding our rate to something,
>> +	 * but we don't have an interface for it.  Just return the
>> +	 * requested value, and it'll get updated after the clock gets
>> +	 * set.
>> +	 */
>> +	return rate;
>> +}
>
> Hmm. I wonder if that will confuse things; I thought the purpose of
> round_rate() was so code could know exactly what would happen ahead of time?

Well, we don't have the ability.  Things seem to work, anyway.
Certainly better than just living with fixed clocks for everything like
we are today.

>> +static struct clk *rpi_firmware_delayed_get_clk(struct
>> of_phandle_args *clkspec, + void *_data)
>
>> +	rpi_clk = &rpi_clocks[clkspec->args[0]];
>> +
>> +	firmware_node = of_parse_phandle(of_node, "firmware", 0);
>> +	if (!firmware_node) {
>> +		dev_err(dev, "%s: Missing firmware node\n", rpi_clk->name);
>> +		return ERR_PTR(-ENODEV);
>> +	}
>> +
>> +	/* Try a no-op transaction to see if the driver is loaded yet. */
>> +	ret = rpi_firmware_property_list(firmware_node, NULL, 0);
>> +	if (ret)
>> +		return ERR_PTR(ret);
>
> I would move all that into this driver's probe().

We can't move all this into the driver's probe, because this is where
we're returning -EPROBE_DEFER.  We could potentially do just the phandle
parse up front and allocate some memory to pass it and our own device
node to this function through the _data arg, but I don't see much point.

>> +CLK_OF_DECLARE(rpi_firmware_clocks, "raspberrypi,firmware-clocks",
>> +	       rpi_firmware_init_clock_provider);
>
> Oh, I guess the same comment as for the firmware node applies re: the
> over-genericity of the DT compatible value applies here too. Perhaps
> raspberrypi,bcm2835-firmware-clocks to match Lee's proposal for the
> firmware node?

Done.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH 2/7] ARM: bcm2835: Add a Raspberry Pi-specific clock driver.

[Stephen Warren]

On 05/29/2015 03:02 PM, Eric Anholt wrote:
> Stephen Warren <swarren@wwwdotorg.org> writes:
> 
>> On 05/18/2015 01:43 PM, Eric Anholt wrote:

>>> +static struct clk *rpi_firmware_delayed_get_clk(struct 
>>> of_phandle_args *clkspec, + void *_data)
>> 
>>> +	rpi_clk = &rpi_clocks[clkspec->args[0]]; + +	firmware_node =
>>> of_parse_phandle(of_node, "firmware", 0); +	if (!firmware_node)
>>> { +		dev_err(dev, "%s: Missing firmware node\n",
>>> rpi_clk->name); +		return ERR_PTR(-ENODEV); +	} + +	/* Try a
>>> no-op transaction to see if the driver is loaded yet. */ +	ret
>>> = rpi_firmware_property_list(firmware_node, NULL, 0); +	if
>>> (ret) +		return ERR_PTR(ret);
>> 
>> I would move all that into this driver's probe().
> 
> We can't move all this into the driver's probe, because this is
> where we're returning -EPROBE_DEFER.  We could potentially do just
> the phandle parse up front and allocate some memory to pass it and
> our own device node to this function through the _data arg, but I
> don't see much point.

Well, once the clock core correctly supports deferred probe, that can
be moved.

Aside from that, I think all your other replies to my replies in this
thread/series make sense.