Rasperry Pi clock support

Rasperry Pi clock support

[Eric Anholt]

I'm also avoiding requests to set rate/state when they're a no-op, as
apparently no-op changes to pixel clock break the firmware-configured
display setup.  It *shouldn't* change anything, but I've confirmed
that setting the CPU clock works fine, and after a couple of weeks of
prodding pixel clock now, I'm at a loss for what's going on with it.

Rasperry Pi clock support

[Eric Anholt]

I'm also avoiding requests to set rate/state when they're a no-op, as
apparently no-op changes to pixel clock break the firmware-configured
display setup.  It *shouldn't* change anything, but I've confirmed
that setting the CPU clock works fine, and after a couple of weeks of
prodding pixel clock now, I'm at a loss for what's going on with it.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Rasperry Pi clock support

[Eric Anholt]

I'm also avoiding requests to set rate/state when they're a no-op, as
apparently no-op changes to pixel clock break the firmware-configured
display setup.  It *shouldn't* change anything, but I've confirmed
that setting the CPU clock works fine, and after a couple of weeks of
prodding pixel clock now, I'm at a loss for what's going on with it.

[PATCH v5 1/3] clk: bcm2835: Add binding docs 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>
Acked-by: Stephen Warren <swarren@wwwdotorg.org>
Acked-by: Michael Turquette <mturquette@baylibre.com>
---

v2: Include the dt-bindings header in this commit instead of the next
    one.  Make the clock indices match the firmware clock IDs.  Rename
    the binding's compat string.  Move the firmware phandle to be
    under a vendor-specific namespace.
v3: Mention 'clk' in the subject instead of the more generic
    dt/bindings.

 .../clock/raspberrypi,bcm2835-firmware-clocks.txt  | 25 ++++++++++++++++++++++
 include/dt-bindings/clk/raspberrypi.h              | 23 ++++++++++++++++++++
 2 files changed, 48 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/raspberrypi,bcm2835-firmware-clocks.txt
 create mode 100644 include/dt-bindings/clk/raspberrypi.h

diff --git a/Documentation/devicetree/bindings/clock/raspberrypi,bcm2835-firmware-clocks.txt b/Documentation/devicetree/bindings/clock/raspberrypi,bcm2835-firmware-clocks.txt
new file mode 100644
index 0000000..0972602
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/raspberrypi,bcm2835-firmware-clocks.txt
@@ -0,0 +1,25 @@
+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,bcm2835-firmware-clocks"
+
+- #clock-cells:		Shall have value <1>.  The permitted clock-specifier
+			  values can be found in
+			  include/dt-bindings/clk/raspberrypi.h.
+
+- raspberrypi,firmware:	Phandle to the firmware driver node.
+
+Example:
+
+firmware_clocks: firmware-clocks {
+	compatible = "raspberrypi,bcm2835-firmware-clocks";
+	#clock-cells = <1>;
+	raspberrypi,firmware = <&firmware>;
+};
diff --git a/include/dt-bindings/clk/raspberrypi.h b/include/dt-bindings/clk/raspberrypi.h
new file mode 100644
index 0000000..ceec90f
--- /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	1
+#define RPI_CLOCK_UART0	2
+#define RPI_CLOCK_ARM	3
+#define RPI_CLOCK_CORE	4
+#define RPI_CLOCK_V3D	5
+#define RPI_CLOCK_H264	6
+#define RPI_CLOCK_ISP	7
+#define RPI_CLOCK_SDRAM	8
+#define RPI_CLOCK_PIXEL	9
+#define RPI_CLOCK_PWM	10
+
+#endif
-- 
2.1.4

[PATCH v5 1/3] clk: bcm2835: Add binding docs 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-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org>
Acked-by: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
Acked-by: Michael Turquette <mturquette-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
---

v2: Include the dt-bindings header in this commit instead of the next
    one.  Make the clock indices match the firmware clock IDs.  Rename
    the binding's compat string.  Move the firmware phandle to be
    under a vendor-specific namespace.
v3: Mention 'clk' in the subject instead of the more generic
    dt/bindings.

 .../clock/raspberrypi,bcm2835-firmware-clocks.txt  | 25 ++++++++++++++++++++++
 include/dt-bindings/clk/raspberrypi.h              | 23 ++++++++++++++++++++
 2 files changed, 48 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/raspberrypi,bcm2835-firmware-clocks.txt
 create mode 100644 include/dt-bindings/clk/raspberrypi.h

diff --git a/Documentation/devicetree/bindings/clock/raspberrypi,bcm2835-firmware-clocks.txt b/Documentation/devicetree/bindings/clock/raspberrypi,bcm2835-firmware-clocks.txt
new file mode 100644
index 0000000..0972602
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/raspberrypi,bcm2835-firmware-clocks.txt
@@ -0,0 +1,25 @@
+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,bcm2835-firmware-clocks"
+
+- #clock-cells:		Shall have value <1>.  The permitted clock-specifier
+			  values can be found in
+			  include/dt-bindings/clk/raspberrypi.h.
+
+- raspberrypi,firmware:	Phandle to the firmware driver node.
+
+Example:
+
+firmware_clocks: firmware-clocks {
+	compatible = "raspberrypi,bcm2835-firmware-clocks";
+	#clock-cells = <1>;
+	raspberrypi,firmware = <&firmware>;
+};
diff --git a/include/dt-bindings/clk/raspberrypi.h b/include/dt-bindings/clk/raspberrypi.h
new file mode 100644
index 0000000..ceec90f
--- /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	1
+#define RPI_CLOCK_UART0	2
+#define RPI_CLOCK_ARM	3
+#define RPI_CLOCK_CORE	4
+#define RPI_CLOCK_V3D	5
+#define RPI_CLOCK_H264	6
+#define RPI_CLOCK_ISP	7
+#define RPI_CLOCK_SDRAM	8
+#define RPI_CLOCK_PIXEL	9
+#define RPI_CLOCK_PWM	10
+
+#endif
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5 1/3] clk: bcm2835: Add binding docs 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>
Acked-by: Stephen Warren <swarren@wwwdotorg.org>
Acked-by: Michael Turquette <mturquette@baylibre.com>
---

v2: Include the dt-bindings header in this commit instead of the next
    one.  Make the clock indices match the firmware clock IDs.  Rename
    the binding's compat string.  Move the firmware phandle to be
    under a vendor-specific namespace.
v3: Mention 'clk' in the subject instead of the more generic
    dt/bindings.

 .../clock/raspberrypi,bcm2835-firmware-clocks.txt  | 25 ++++++++++++++++++++++
 include/dt-bindings/clk/raspberrypi.h              | 23 ++++++++++++++++++++
 2 files changed, 48 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/raspberrypi,bcm2835-firmware-clocks.txt
 create mode 100644 include/dt-bindings/clk/raspberrypi.h

diff --git a/Documentation/devicetree/bindings/clock/raspberrypi,bcm2835-firmware-clocks.txt b/Documentation/devicetree/bindings/clock/raspberrypi,bcm2835-firmware-clocks.txt
new file mode 100644
index 0000000..0972602
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/raspberrypi,bcm2835-firmware-clocks.txt
@@ -0,0 +1,25 @@
+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,bcm2835-firmware-clocks"
+
+- #clock-cells:		Shall have value <1>.  The permitted clock-specifier
+			  values can be found in
+			  include/dt-bindings/clk/raspberrypi.h.
+
+- raspberrypi,firmware:	Phandle to the firmware driver node.
+
+Example:
+
+firmware_clocks: firmware-clocks {
+	compatible = "raspberrypi,bcm2835-firmware-clocks";
+	#clock-cells = <1>;
+	raspberrypi,firmware = <&firmware>;
+};
diff --git a/include/dt-bindings/clk/raspberrypi.h b/include/dt-bindings/clk/raspberrypi.h
new file mode 100644
index 0000000..ceec90f
--- /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	1
+#define RPI_CLOCK_UART0	2
+#define RPI_CLOCK_ARM	3
+#define RPI_CLOCK_CORE	4
+#define RPI_CLOCK_V3D	5
+#define RPI_CLOCK_H264	6
+#define RPI_CLOCK_ISP	7
+#define RPI_CLOCK_SDRAM	8
+#define RPI_CLOCK_PIXEL	9
+#define RPI_CLOCK_PWM	10
+
+#endif
-- 
2.1.4

Re: [PATCH v5 1/3] clk: bcm2835: Add binding docs for the Raspberry Pi clock provider

[Eric Anholt]

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

Eric Anholt <eric@anholt.net> writes:

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

Gordon from the Raspberry Pi Foundation just asked me "what do you mean,
you can't access the clocks from the ARM?"  I'd been assured I couldn't
by another developer (who was originally going to write a Linux driver
for this), and my own testing had also indicated I couldn't, but after a
new round of hacking together some tests, I see things that look a lot
like clockman registers.

Looks like we're going to get a native driver, instead.

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

Re: [PATCH v5 1/3] clk: bcm2835: Add binding docs for the Raspberry Pi clock provider

[Eric Anholt]

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

Eric Anholt <eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org> writes:

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

Gordon from the Raspberry Pi Foundation just asked me "what do you mean,
you can't access the clocks from the ARM?"  I'd been assured I couldn't
by another developer (who was originally going to write a Linux driver
for this), and my own testing had also indicated I couldn't, but after a
new round of hacking together some tests, I see things that look a lot
like clockman registers.

Looks like we're going to get a native driver, instead.

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

[PATCH v5 1/3] clk: bcm2835: Add binding docs for the Raspberry Pi clock provider

[Eric Anholt]

Eric Anholt <eric@anholt.net> writes:

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

Gordon from the Raspberry Pi Foundation just asked me "what do you mean,
you can't access the clocks from the ARM?"  I'd been assured I couldn't
by another developer (who was originally going to write a Linux driver
for this), and my own testing had also indicated I couldn't, but after a
new round of hacking together some tests, I see things that look a lot
like clockman registers.

Looks like we're going to get a native driver, instead.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150824/8ff89ce0/attachment.sig>

[PATCH v5 2/3] clk: 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.

v3: Update for new rpi firmware API.  Update the compatible string.
    Make the firmware handle be under a vendor-namespaced property.
    Make the DT indices match the firmware clock IDs.  Move the driver
    under the firmware driver's Kconfig, since it requires it.  Move a
    container_of() from 2 callers into the callee.

v4: Update commit message to mention subsystem.  Add #defines for a
    couple of bitfield checks, and use the bitfield check for
    rpi_clk_is_on().  Return an error when appropriate in
    rpi_clk_is_on(), and don't return specific rpi_firmware_property()
    errors in other places when we were only returning -EINVAL.  Add
    some comment text describing how return values work from
    the firmware.

v5: Restructure as a platform_driver on suggestion from Michael
    Turquette.  This works around the -EPROBE_DEFER problem, so now we
    can register all of our clocks at probe time.  Store the current
    clock rate in the struct, and avoid asking for no-op changes.  It
    appears that a no-op change of the pixel clock while the clock is
    being used (at boot time when the KMS driver calls
    clk_prepare_enable()) actually breaks the display.  Dropped
    Stephen's ack since this is a pretty significant rewrite.

 drivers/clk/Makefile          |   1 +
 drivers/clk/clk-raspberrypi.c | 286 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 287 insertions(+)
 create mode 100644 drivers/clk/clk-raspberrypi.c

diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index c4cf075..ad0a4a5 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_RASPBERRYPI_FIRMWARE)	+= 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..a65a028
--- /dev/null
+++ b/drivers/clk/clk-raspberrypi.c
@@ -0,0 +1,286 @@
+/*
+ * 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 <linux/clk-provider.h>
+#include <linux/module.h>
+#include <dt-bindings/clk/raspberrypi.h>
+#include <soc/bcm2835/raspberrypi-firmware.h>
+
+#define RPI_FIRMWARE_CLOCK_STATE_ENABLED		(1 << 0)
+#define RPI_FIRMWARE_CLOCK_STATE_ERROR			(1 << 1)
+#define RPI_FIRMWARE_SET_CLOCK_RATE_ERROR		0
+
+static const struct {
+	const char *name;
+	int flags;
+} rpi_clocks[] = {
+	[RPI_CLOCK_EMMC] = { "emmc", CLK_IS_ROOT },
+	[RPI_CLOCK_UART0] = { "uart0", CLK_IS_ROOT },
+	[RPI_CLOCK_ARM] = { "arm", CLK_IS_ROOT | CLK_IGNORE_UNUSED },
+	[RPI_CLOCK_CORE] = { "core", CLK_IS_ROOT | CLK_IGNORE_UNUSED },
+	[RPI_CLOCK_V3D] = { "v3d", CLK_IS_ROOT },
+	[RPI_CLOCK_H264] = { "h264", CLK_IS_ROOT },
+	[RPI_CLOCK_ISP] = { "isp", CLK_IS_ROOT },
+	[RPI_CLOCK_SDRAM] = { "sdram", CLK_IS_ROOT | CLK_IGNORE_UNUSED },
+	[RPI_CLOCK_PIXEL] = { "pixel", CLK_IS_ROOT | CLK_IGNORE_UNUSED },
+	[RPI_CLOCK_PWM] = { "pwm", CLK_IS_ROOT },
+};
+
+struct rpi_firmware_clock {
+	/* Clock definitions in our static struct. */
+	const char *name;
+	int flags;
+
+	/* The rest are filled in at init time. */
+	struct clk_hw hw;
+	struct device *dev;
+	struct rpi_firmware *firmware;
+	uint32_t last_rate;
+	int id;
+};
+
+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->id;
+	packet[1] = 0;
+	ret = rpi_firmware_property(rpi_clk->firmware,
+				    RPI_FIRMWARE_GET_CLOCK_STATE,
+				    &packet, sizeof(packet));
+	/*
+	 * The second packet field has the new clock state returned in
+	 * the low bit, or an error flag in the second bit.
+	 */
+	if (ret || (packet[1] & RPI_FIRMWARE_CLOCK_STATE_ERROR)) {
+		dev_err(rpi_clk->dev, "Failed to get clock state\n");
+		return ret ? ret : -EINVAL;
+	}
+
+	return packet[1] & RPI_FIRMWARE_CLOCK_STATE_ENABLED;
+}
+
+static int rpi_clk_set_state(struct clk_hw *hw, bool on)
+{
+	struct rpi_firmware_clock *rpi_clk =
+		container_of(hw, struct rpi_firmware_clock, hw);
+	u32 packet[2];
+	int ret;
+
+	if (on == (rpi_clk->last_rate != 0))
+		return 0;
+
+	packet[0] = rpi_clk->id;
+	packet[1] = on;
+	ret = rpi_firmware_property(rpi_clk->firmware,
+				    RPI_FIRMWARE_SET_CLOCK_STATE,
+				    &packet, sizeof(packet));
+	/*
+	 * The second packet field has the new clock state returned in
+	 * the low bit, or an error flag in the second bit.
+	 */
+	if (ret || (packet[1] & RPI_FIRMWARE_CLOCK_STATE_ERROR)) {
+		dev_err(rpi_clk->dev, "Failed to set clock state\n");
+		return ret ? ret : -EINVAL;
+	}
+
+	return 0;
+}
+
+static int rpi_clk_on(struct clk_hw *hw)
+{
+	return rpi_clk_set_state(hw, true);
+}
+
+static void rpi_clk_off(struct clk_hw *hw)
+{
+	rpi_clk_set_state(hw, 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->id;
+	packet[1] = 0;
+	ret = rpi_firmware_property(rpi_clk->firmware,
+				    RPI_FIRMWARE_GET_CLOCK_RATE,
+				    &packet, sizeof(packet));
+	/*
+	 * Note that the second packet field returns 0 on an unknown
+	 * clock error, which would also be a reasonable value for a
+	 * clock that's off.
+	 */
+	if (ret) {
+		dev_err(rpi_clk->dev, "Failed to get clock rate\n");
+		return 0;
+	}
+
+	rpi_clk->last_rate = packet[1];
+
+	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;
+
+	if (rate == rpi_clk->last_rate)
+		return 0;
+
+	packet[0] = rpi_clk->id;
+	packet[1] = rate;
+	ret = rpi_firmware_property(rpi_clk->firmware,
+				    RPI_FIRMWARE_SET_CLOCK_RATE,
+				    &packet, sizeof(packet));
+	/*
+	 * The second packet field has the new clock rate returned, or
+	 * 0 on error.
+	 */
+	if (ret || packet[1] == RPI_FIRMWARE_SET_CLOCK_RATE_ERROR) {
+		dev_err(rpi_clk->dev, "Failed to set clock rate\n");
+		return ret;
+	}
+
+	rpi_clk->last_rate = packet[1];
+
+	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 int rpi_clk_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *firmware_node;
+	struct rpi_firmware *firmware;
+	struct clk_init_data init;
+	int i;
+	struct clk_onecell_data *onecell;
+
+	firmware_node = of_parse_phandle(dev->of_node,
+					 "raspberrypi,firmware", 0);
+	if (!firmware_node) {
+		dev_err(dev, "Missing firmware node\n");
+		return -ENODEV;
+	}
+	firmware = rpi_firmware_get(firmware_node);
+	if (!firmware)
+		return -EPROBE_DEFER;
+
+	onecell = devm_kmalloc(dev, sizeof(*onecell), GFP_KERNEL);
+	if (!onecell)
+		return -ENOMEM;
+	onecell->clk_num = ARRAY_SIZE(rpi_clocks);
+	onecell->clks = devm_kzalloc(dev, sizeof(*onecell->clks), GFP_KERNEL);
+	if (!onecell->clks)
+		return -ENOMEM;
+
+	memset(&init, 0, sizeof(init));
+	init.ops = &rpi_clk_ops;
+
+	for (i = 0; i < ARRAY_SIZE(rpi_clocks); i++) {
+		struct rpi_firmware_clock *rpi_clk;
+
+		if (!rpi_clocks[i].name)
+			continue;
+
+		rpi_clk = devm_kzalloc(dev, sizeof(*rpi_clk), GFP_KERNEL);
+		if (!rpi_clk)
+			return -ENOMEM;
+
+		rpi_clk->name = rpi_clocks[i].name;
+		rpi_clk->firmware = firmware;
+		rpi_clk->dev = dev;
+		rpi_clk->hw.init = &init;
+		rpi_clk->id = i;
+		init.name = rpi_clocks[i].name;
+		init.flags = rpi_clocks[i].flags;
+
+		onecell->clks[i] = devm_clk_register(&pdev->dev, &rpi_clk->hw);
+		if (IS_ERR(onecell->clks[i]))
+			return PTR_ERR(onecell->clks[i]);
+
+		/* Get the current clock rate/state, to avoid extra on
+		 * to on transitions at boot.
+		 */
+		rpi_clk_get_rate(&rpi_clk->hw, 0);
+	}
+
+	return of_clk_add_provider(pdev->dev.of_node, of_clk_src_onecell_get,
+				   onecell);
+}
+
+static int rpi_clk_remove(struct platform_device *pdev)
+{
+	of_clk_del_provider(pdev->dev.of_node);
+
+	return 0;
+}
+
+static const struct of_device_id rpi_clk_of_match[] = {
+	{ .compatible = "raspberrypi,bcm2835-firmware-clocks", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, rpi_clk_of_match);
+
+static struct platform_driver rpi_clk_driver = {
+	.driver = {
+		.name = "raspberrypi-clk",
+		.of_match_table = rpi_clk_of_match,
+	},
+	.probe		= rpi_clk_probe,
+	.remove		= rpi_clk_remove,
+};
+module_platform_driver(rpi_clk_driver);
+
+MODULE_AUTHOR("Eric Anholt <eric@anholt.net>");
+MODULE_DESCRIPTION("Raspberry Pi clock driver");
+MODULE_LICENSE("GPL v2");
-- 
2.1.4

[PATCH v5 2/3] clk: 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.

v3: Update for new rpi firmware API.  Update the compatible string.
    Make the firmware handle be under a vendor-namespaced property.
    Make the DT indices match the firmware clock IDs.  Move the driver
    under the firmware driver's Kconfig, since it requires it.  Move a
    container_of() from 2 callers into the callee.

v4: Update commit message to mention subsystem.  Add #defines for a
    couple of bitfield checks, and use the bitfield check for
    rpi_clk_is_on().  Return an error when appropriate in
    rpi_clk_is_on(), and don't return specific rpi_firmware_property()
    errors in other places when we were only returning -EINVAL.  Add
    some comment text describing how return values work from
    the firmware.

v5: Restructure as a platform_driver on suggestion from Michael
    Turquette.  This works around the -EPROBE_DEFER problem, so now we
    can register all of our clocks at probe time.  Store the current
    clock rate in the struct, and avoid asking for no-op changes.  It
    appears that a no-op change of the pixel clock while the clock is
    being used (at boot time when the KMS driver calls
    clk_prepare_enable()) actually breaks the display.  Dropped
    Stephen's ack since this is a pretty significant rewrite.

 drivers/clk/Makefile          |   1 +
 drivers/clk/clk-raspberrypi.c | 286 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 287 insertions(+)
 create mode 100644 drivers/clk/clk-raspberrypi.c

diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index c4cf075..ad0a4a5 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_RASPBERRYPI_FIRMWARE)	+= 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..a65a028
--- /dev/null
+++ b/drivers/clk/clk-raspberrypi.c
@@ -0,0 +1,286 @@
+/*
+ * 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 <linux/clk-provider.h>
+#include <linux/module.h>
+#include <dt-bindings/clk/raspberrypi.h>
+#include <soc/bcm2835/raspberrypi-firmware.h>
+
+#define RPI_FIRMWARE_CLOCK_STATE_ENABLED		(1 << 0)
+#define RPI_FIRMWARE_CLOCK_STATE_ERROR			(1 << 1)
+#define RPI_FIRMWARE_SET_CLOCK_RATE_ERROR		0
+
+static const struct {
+	const char *name;
+	int flags;
+} rpi_clocks[] = {
+	[RPI_CLOCK_EMMC] = { "emmc", CLK_IS_ROOT },
+	[RPI_CLOCK_UART0] = { "uart0", CLK_IS_ROOT },
+	[RPI_CLOCK_ARM] = { "arm", CLK_IS_ROOT | CLK_IGNORE_UNUSED },
+	[RPI_CLOCK_CORE] = { "core", CLK_IS_ROOT | CLK_IGNORE_UNUSED },
+	[RPI_CLOCK_V3D] = { "v3d", CLK_IS_ROOT },
+	[RPI_CLOCK_H264] = { "h264", CLK_IS_ROOT },
+	[RPI_CLOCK_ISP] = { "isp", CLK_IS_ROOT },
+	[RPI_CLOCK_SDRAM] = { "sdram", CLK_IS_ROOT | CLK_IGNORE_UNUSED },
+	[RPI_CLOCK_PIXEL] = { "pixel", CLK_IS_ROOT | CLK_IGNORE_UNUSED },
+	[RPI_CLOCK_PWM] = { "pwm", CLK_IS_ROOT },
+};
+
+struct rpi_firmware_clock {
+	/* Clock definitions in our static struct. */
+	const char *name;
+	int flags;
+
+	/* The rest are filled in@init time. */
+	struct clk_hw hw;
+	struct device *dev;
+	struct rpi_firmware *firmware;
+	uint32_t last_rate;
+	int id;
+};
+
+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->id;
+	packet[1] = 0;
+	ret = rpi_firmware_property(rpi_clk->firmware,
+				    RPI_FIRMWARE_GET_CLOCK_STATE,
+				    &packet, sizeof(packet));
+	/*
+	 * The second packet field has the new clock state returned in
+	 * the low bit, or an error flag in the second bit.
+	 */
+	if (ret || (packet[1] & RPI_FIRMWARE_CLOCK_STATE_ERROR)) {
+		dev_err(rpi_clk->dev, "Failed to get clock state\n");
+		return ret ? ret : -EINVAL;
+	}
+
+	return packet[1] & RPI_FIRMWARE_CLOCK_STATE_ENABLED;
+}
+
+static int rpi_clk_set_state(struct clk_hw *hw, bool on)
+{
+	struct rpi_firmware_clock *rpi_clk =
+		container_of(hw, struct rpi_firmware_clock, hw);
+	u32 packet[2];
+	int ret;
+
+	if (on == (rpi_clk->last_rate != 0))
+		return 0;
+
+	packet[0] = rpi_clk->id;
+	packet[1] = on;
+	ret = rpi_firmware_property(rpi_clk->firmware,
+				    RPI_FIRMWARE_SET_CLOCK_STATE,
+				    &packet, sizeof(packet));
+	/*
+	 * The second packet field has the new clock state returned in
+	 * the low bit, or an error flag in the second bit.
+	 */
+	if (ret || (packet[1] & RPI_FIRMWARE_CLOCK_STATE_ERROR)) {
+		dev_err(rpi_clk->dev, "Failed to set clock state\n");
+		return ret ? ret : -EINVAL;
+	}
+
+	return 0;
+}
+
+static int rpi_clk_on(struct clk_hw *hw)
+{
+	return rpi_clk_set_state(hw, true);
+}
+
+static void rpi_clk_off(struct clk_hw *hw)
+{
+	rpi_clk_set_state(hw, 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->id;
+	packet[1] = 0;
+	ret = rpi_firmware_property(rpi_clk->firmware,
+				    RPI_FIRMWARE_GET_CLOCK_RATE,
+				    &packet, sizeof(packet));
+	/*
+	 * Note that the second packet field returns 0 on an unknown
+	 * clock error, which would also be a reasonable value for a
+	 * clock that's off.
+	 */
+	if (ret) {
+		dev_err(rpi_clk->dev, "Failed to get clock rate\n");
+		return 0;
+	}
+
+	rpi_clk->last_rate = packet[1];
+
+	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;
+
+	if (rate == rpi_clk->last_rate)
+		return 0;
+
+	packet[0] = rpi_clk->id;
+	packet[1] = rate;
+	ret = rpi_firmware_property(rpi_clk->firmware,
+				    RPI_FIRMWARE_SET_CLOCK_RATE,
+				    &packet, sizeof(packet));
+	/*
+	 * The second packet field has the new clock rate returned, or
+	 * 0 on error.
+	 */
+	if (ret || packet[1] == RPI_FIRMWARE_SET_CLOCK_RATE_ERROR) {
+		dev_err(rpi_clk->dev, "Failed to set clock rate\n");
+		return ret;
+	}
+
+	rpi_clk->last_rate = packet[1];
+
+	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 int rpi_clk_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *firmware_node;
+	struct rpi_firmware *firmware;
+	struct clk_init_data init;
+	int i;
+	struct clk_onecell_data *onecell;
+
+	firmware_node = of_parse_phandle(dev->of_node,
+					 "raspberrypi,firmware", 0);
+	if (!firmware_node) {
+		dev_err(dev, "Missing firmware node\n");
+		return -ENODEV;
+	}
+	firmware = rpi_firmware_get(firmware_node);
+	if (!firmware)
+		return -EPROBE_DEFER;
+
+	onecell = devm_kmalloc(dev, sizeof(*onecell), GFP_KERNEL);
+	if (!onecell)
+		return -ENOMEM;
+	onecell->clk_num = ARRAY_SIZE(rpi_clocks);
+	onecell->clks = devm_kzalloc(dev, sizeof(*onecell->clks), GFP_KERNEL);
+	if (!onecell->clks)
+		return -ENOMEM;
+
+	memset(&init, 0, sizeof(init));
+	init.ops = &rpi_clk_ops;
+
+	for (i = 0; i < ARRAY_SIZE(rpi_clocks); i++) {
+		struct rpi_firmware_clock *rpi_clk;
+
+		if (!rpi_clocks[i].name)
+			continue;
+
+		rpi_clk = devm_kzalloc(dev, sizeof(*rpi_clk), GFP_KERNEL);
+		if (!rpi_clk)
+			return -ENOMEM;
+
+		rpi_clk->name = rpi_clocks[i].name;
+		rpi_clk->firmware = firmware;
+		rpi_clk->dev = dev;
+		rpi_clk->hw.init = &init;
+		rpi_clk->id = i;
+		init.name = rpi_clocks[i].name;
+		init.flags = rpi_clocks[i].flags;
+
+		onecell->clks[i] = devm_clk_register(&pdev->dev, &rpi_clk->hw);
+		if (IS_ERR(onecell->clks[i]))
+			return PTR_ERR(onecell->clks[i]);
+
+		/* Get the current clock rate/state, to avoid extra on
+		 * to on transitions at boot.
+		 */
+		rpi_clk_get_rate(&rpi_clk->hw, 0);
+	}
+
+	return of_clk_add_provider(pdev->dev.of_node, of_clk_src_onecell_get,
+				   onecell);
+}
+
+static int rpi_clk_remove(struct platform_device *pdev)
+{
+	of_clk_del_provider(pdev->dev.of_node);
+
+	return 0;
+}
+
+static const struct of_device_id rpi_clk_of_match[] = {
+	{ .compatible = "raspberrypi,bcm2835-firmware-clocks", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, rpi_clk_of_match);
+
+static struct platform_driver rpi_clk_driver = {
+	.driver = {
+		.name = "raspberrypi-clk",
+		.of_match_table = rpi_clk_of_match,
+	},
+	.probe		= rpi_clk_probe,
+	.remove		= rpi_clk_remove,
+};
+module_platform_driver(rpi_clk_driver);
+
+MODULE_AUTHOR("Eric Anholt <eric@anholt.net>");
+MODULE_DESCRIPTION("Raspberry Pi clock driver");
+MODULE_LICENSE("GPL v2");
-- 
2.1.4

Re: [PATCH v5 2/3] clk: Add a Raspberry Pi-specific clock driver.

[Stephen Warren]

On 08/13/2015 05:05 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.

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

> +static const struct {
> +	const char *name;
> +	int flags;
> +} rpi_clocks[] = {
> +	[RPI_CLOCK_EMMC] = { "emmc", CLK_IS_ROOT },
> +	[RPI_CLOCK_UART0] = { "uart0", CLK_IS_ROOT },
> +	[RPI_CLOCK_ARM] = { "arm", CLK_IS_ROOT | CLK_IGNORE_UNUSED },
> +	[RPI_CLOCK_CORE] = { "core", CLK_IS_ROOT | CLK_IGNORE_UNUSED },
> +	[RPI_CLOCK_V3D] = { "v3d", CLK_IS_ROOT },
> +	[RPI_CLOCK_H264] = { "h264", CLK_IS_ROOT },
> +	[RPI_CLOCK_ISP] = { "isp", CLK_IS_ROOT },
> +	[RPI_CLOCK_SDRAM] = { "sdram", CLK_IS_ROOT | CLK_IGNORE_UNUSED },
> +	[RPI_CLOCK_PIXEL] = { "pixel", CLK_IS_ROOT | CLK_IGNORE_UNUSED },
> +	[RPI_CLOCK_PWM] = { "pwm", CLK_IS_ROOT },
> +};
> +
> +struct rpi_firmware_clock {
> +	/* Clock definitions in our static struct. */
> +	const char *name;
> +	int flags;

Are these duplicates of the values in rpi_clocks[]? Why not just store a
pointer to or index of the entry in that array?

> +static int rpi_clk_set_state(struct clk_hw *hw, bool on)
> +{
> +	struct rpi_firmware_clock *rpi_clk =
> +		container_of(hw, struct rpi_firmware_clock, hw);
> +	u32 packet[2];
> +	int ret;
> +
> +	if (on == (rpi_clk->last_rate != 0))
> +		return 0;

The overloading of last_rate to represent both rate information and
on/off status is slightly confusing. I would have expected this function
to clear last_rate to 0 when switching the clock off, and some specific
rate when turning a clock on. Is there some guarantee that the clock
core will always call recalc_rate() at certain times, thus ensuring that
last_rate is always accurate?

Wouldn't it be simpler to let last_rate always represent that actual
rate, and have a separate last_on or is_on field to represent the
enable/disable state?

> +static unsigned long rpi_clk_get_rate(struct clk_hw *hw,
> +				      unsigned long parent_rate)
...
> +	rpi_clk->last_rate = packet[1];

Since this is a query API, I wouldn't have expected it to have
side-effects like this. Don't we know what rate the clock runs at based
on the firmware's response in set_rate()?

> +static int rpi_clk_probe(struct platform_device *pdev)

> +	onecell = devm_kmalloc(dev, sizeof(*onecell), GFP_KERNEL);
> +	if (!onecell)
> +		return -ENOMEM;
> +	onecell->clk_num = ARRAY_SIZE(rpi_clocks);
> +	onecell->clks = devm_kzalloc(dev, sizeof(*onecell->clks), GFP_KERNEL);

Don't you need to multiply the size by ARRAY_SIZE(rpi_clocks)? I assume
onecell->clks is an array with one entry per each of onecell->clk_num?
Yes, the for loop right after that allocation confirms this.

Re: [PATCH v5 2/3] clk: Add a Raspberry Pi-specific clock driver.

[Stephen Warren]

On 08/13/2015 05:05 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.

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

> +static const struct {
> +	const char *name;
> +	int flags;
> +} rpi_clocks[] = {
> +	[RPI_CLOCK_EMMC] = { "emmc", CLK_IS_ROOT },
> +	[RPI_CLOCK_UART0] = { "uart0", CLK_IS_ROOT },
> +	[RPI_CLOCK_ARM] = { "arm", CLK_IS_ROOT | CLK_IGNORE_UNUSED },
> +	[RPI_CLOCK_CORE] = { "core", CLK_IS_ROOT | CLK_IGNORE_UNUSED },
> +	[RPI_CLOCK_V3D] = { "v3d", CLK_IS_ROOT },
> +	[RPI_CLOCK_H264] = { "h264", CLK_IS_ROOT },
> +	[RPI_CLOCK_ISP] = { "isp", CLK_IS_ROOT },
> +	[RPI_CLOCK_SDRAM] = { "sdram", CLK_IS_ROOT | CLK_IGNORE_UNUSED },
> +	[RPI_CLOCK_PIXEL] = { "pixel", CLK_IS_ROOT | CLK_IGNORE_UNUSED },
> +	[RPI_CLOCK_PWM] = { "pwm", CLK_IS_ROOT },
> +};
> +
> +struct rpi_firmware_clock {
> +	/* Clock definitions in our static struct. */
> +	const char *name;
> +	int flags;

Are these duplicates of the values in rpi_clocks[]? Why not just store a
pointer to or index of the entry in that array?

> +static int rpi_clk_set_state(struct clk_hw *hw, bool on)
> +{
> +	struct rpi_firmware_clock *rpi_clk =
> +		container_of(hw, struct rpi_firmware_clock, hw);
> +	u32 packet[2];
> +	int ret;
> +
> +	if (on == (rpi_clk->last_rate != 0))
> +		return 0;

The overloading of last_rate to represent both rate information and
on/off status is slightly confusing. I would have expected this function
to clear last_rate to 0 when switching the clock off, and some specific
rate when turning a clock on. Is there some guarantee that the clock
core will always call recalc_rate() at certain times, thus ensuring that
last_rate is always accurate?

Wouldn't it be simpler to let last_rate always represent that actual
rate, and have a separate last_on or is_on field to represent the
enable/disable state?

> +static unsigned long rpi_clk_get_rate(struct clk_hw *hw,
> +				      unsigned long parent_rate)
...
> +	rpi_clk->last_rate = packet[1];

Since this is a query API, I wouldn't have expected it to have
side-effects like this. Don't we know what rate the clock runs at based
on the firmware's response in set_rate()?

> +static int rpi_clk_probe(struct platform_device *pdev)

> +	onecell = devm_kmalloc(dev, sizeof(*onecell), GFP_KERNEL);
> +	if (!onecell)
> +		return -ENOMEM;
> +	onecell->clk_num = ARRAY_SIZE(rpi_clocks);
> +	onecell->clks = devm_kzalloc(dev, sizeof(*onecell->clks), GFP_KERNEL);

Don't you need to multiply the size by ARRAY_SIZE(rpi_clocks)? I assume
onecell->clks is an array with one entry per each of onecell->clk_num?
Yes, the for loop right after that allocation confirms this.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5 2/3] clk: Add a Raspberry Pi-specific clock driver.

[Stephen Warren]

On 08/13/2015 05:05 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.

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

> +static const struct {
> +	const char *name;
> +	int flags;
> +} rpi_clocks[] = {
> +	[RPI_CLOCK_EMMC] = { "emmc", CLK_IS_ROOT },
> +	[RPI_CLOCK_UART0] = { "uart0", CLK_IS_ROOT },
> +	[RPI_CLOCK_ARM] = { "arm", CLK_IS_ROOT | CLK_IGNORE_UNUSED },
> +	[RPI_CLOCK_CORE] = { "core", CLK_IS_ROOT | CLK_IGNORE_UNUSED },
> +	[RPI_CLOCK_V3D] = { "v3d", CLK_IS_ROOT },
> +	[RPI_CLOCK_H264] = { "h264", CLK_IS_ROOT },
> +	[RPI_CLOCK_ISP] = { "isp", CLK_IS_ROOT },
> +	[RPI_CLOCK_SDRAM] = { "sdram", CLK_IS_ROOT | CLK_IGNORE_UNUSED },
> +	[RPI_CLOCK_PIXEL] = { "pixel", CLK_IS_ROOT | CLK_IGNORE_UNUSED },
> +	[RPI_CLOCK_PWM] = { "pwm", CLK_IS_ROOT },
> +};
> +
> +struct rpi_firmware_clock {
> +	/* Clock definitions in our static struct. */
> +	const char *name;
> +	int flags;

Are these duplicates of the values in rpi_clocks[]? Why not just store a
pointer to or index of the entry in that array?

> +static int rpi_clk_set_state(struct clk_hw *hw, bool on)
> +{
> +	struct rpi_firmware_clock *rpi_clk =
> +		container_of(hw, struct rpi_firmware_clock, hw);
> +	u32 packet[2];
> +	int ret;
> +
> +	if (on == (rpi_clk->last_rate != 0))
> +		return 0;

The overloading of last_rate to represent both rate information and
on/off status is slightly confusing. I would have expected this function
to clear last_rate to 0 when switching the clock off, and some specific
rate when turning a clock on. Is there some guarantee that the clock
core will always call recalc_rate() at certain times, thus ensuring that
last_rate is always accurate?

Wouldn't it be simpler to let last_rate always represent that actual
rate, and have a separate last_on or is_on field to represent the
enable/disable state?

> +static unsigned long rpi_clk_get_rate(struct clk_hw *hw,
> +				      unsigned long parent_rate)
...
> +	rpi_clk->last_rate = packet[1];

Since this is a query API, I wouldn't have expected it to have
side-effects like this. Don't we know what rate the clock runs at based
on the firmware's response in set_rate()?

> +static int rpi_clk_probe(struct platform_device *pdev)

> +	onecell = devm_kmalloc(dev, sizeof(*onecell), GFP_KERNEL);
> +	if (!onecell)
> +		return -ENOMEM;
> +	onecell->clk_num = ARRAY_SIZE(rpi_clocks);
> +	onecell->clks = devm_kzalloc(dev, sizeof(*onecell->clks), GFP_KERNEL);

Don't you need to multiply the size by ARRAY_SIZE(rpi_clocks)? I assume
onecell->clks is an array with one entry per each of onecell->clk_num?
Yes, the for loop right after that allocation confirms this.

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

[Eric Anholt]

Signed-off-by: Eric Anholt <eric@anholt.net>
Acked-by: Lee Jones <lee@kernel.org>
---

v2: Rename our compat string to mention bcm2835, and make our firmware
    phandle be under a vendor-namespaced property.

v3: Squashed in the patches to reference the other clocks, to avoid
    regressions now that we register all clocks at boot.  Dropped
    Stephen's ack, since he hadn't acked the new EMMC change yet.

 arch/arm/boot/dts/bcm2835-rpi.dtsi | 18 ++++++++++++++++++
 arch/arm/boot/dts/bcm2835.dtsi     |  3 +--
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/arch/arm/boot/dts/bcm2835-rpi.dtsi b/arch/arm/boot/dts/bcm2835-rpi.dtsi
index ab5474e..5d370cb 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"
 
 / {
@@ -20,6 +21,12 @@
 			compatible = "raspberrypi,bcm2835-firmware";
 			mboxes = <&mailbox>;
 		};
+
+		firmware_clocks: firmware-clocks {
+			compatible = "raspberrypi,bcm2835-firmware-clocks";
+			#clock-cells = <1>;
+			raspberrypi,firmware = <&firmware>;
+		};
 	};
 };
 
@@ -55,4 +62,15 @@
 &sdhci {
 	status = "okay";
 	bus-width = <4>;
+	clocks = <&firmware_clocks RPI_CLOCK_EMMC>;
+};
+
+&uart0 {
+	clocks = <&firmware_clocks RPI_CLOCK_UART0>,
+		 <&firmware_clocks RPI_CLOCK_CORE>;
+	clock-names = "uartclk", "apb_pclk";
+};
+
+&spi {
+	clocks = <&firmware_clocks RPI_CLOCK_CORE>;
 };
diff --git a/arch/arm/boot/dts/bcm2835.dtsi b/arch/arm/boot/dts/bcm2835.dtsi
index 301c73f..5be2862 100644
--- a/arch/arm/boot/dts/bcm2835.dtsi
+++ b/arch/arm/boot/dts/bcm2835.dtsi
@@ -92,11 +92,10 @@
 			#interrupt-cells = <2>;
 		};
 
-		uart@7e201000 {
+		uart0: uart@7e201000 {
 			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 v5 3/3] ARM: bcm2835: Add DT for the firmware clocks driver.

[Eric Anholt]

Signed-off-by: Eric Anholt <eric@anholt.net>
Acked-by: Lee Jones <lee@kernel.org>
---

v2: Rename our compat string to mention bcm2835, and make our firmware
    phandle be under a vendor-namespaced property.

v3: Squashed in the patches to reference the other clocks, to avoid
    regressions now that we register all clocks at boot.  Dropped
    Stephen's ack, since he hadn't acked the new EMMC change yet.

 arch/arm/boot/dts/bcm2835-rpi.dtsi | 18 ++++++++++++++++++
 arch/arm/boot/dts/bcm2835.dtsi     |  3 +--
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/arch/arm/boot/dts/bcm2835-rpi.dtsi b/arch/arm/boot/dts/bcm2835-rpi.dtsi
index ab5474e..5d370cb 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"
 
 / {
@@ -20,6 +21,12 @@
 			compatible = "raspberrypi,bcm2835-firmware";
 			mboxes = <&mailbox>;
 		};
+
+		firmware_clocks: firmware-clocks {
+			compatible = "raspberrypi,bcm2835-firmware-clocks";
+			#clock-cells = <1>;
+			raspberrypi,firmware = <&firmware>;
+		};
 	};
 };
 
@@ -55,4 +62,15 @@
 &sdhci {
 	status = "okay";
 	bus-width = <4>;
+	clocks = <&firmware_clocks RPI_CLOCK_EMMC>;
+};
+
+&uart0 {
+	clocks = <&firmware_clocks RPI_CLOCK_UART0>,
+		 <&firmware_clocks RPI_CLOCK_CORE>;
+	clock-names = "uartclk", "apb_pclk";
+};
+
+&spi {
+	clocks = <&firmware_clocks RPI_CLOCK_CORE>;
 };
diff --git a/arch/arm/boot/dts/bcm2835.dtsi b/arch/arm/boot/dts/bcm2835.dtsi
index 301c73f..5be2862 100644
--- a/arch/arm/boot/dts/bcm2835.dtsi
+++ b/arch/arm/boot/dts/bcm2835.dtsi
@@ -92,11 +92,10 @@
 			#interrupt-cells = <2>;
 		};
 
-		uart at 7e201000 {
+		uart0: uart at 7e201000 {
 			compatible = "brcm,bcm2835-pl011", "arm,pl011", "arm,primecell";
 			reg = <0x7e201000 0x1000>;
 			interrupts = <2 25>;
-			clock-frequency = <3000000>;
 			arm,primecell-periphid = <0x00241011>;
 		};
 
-- 
2.1.4

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

[Stephen Warren]

On 08/13/2015 05:05 PM, Eric Anholt wrote:
> Signed-off-by: Eric Anholt <eric@anholt.net>
> Acked-by: Lee Jones <lee@kernel.org>

A patch description might be nice, although admittedly the subject seems
clear enough.

> diff --git a/arch/arm/boot/dts/bcm2835-rpi.dtsi b/arch/arm/boot/dts/bcm2835-rpi.dtsi

> +		firmware_clocks: firmware-clocks {
> +			compatible = "raspberrypi,bcm2835-firmware-clocks";
> +			#clock-cells = <1>;
> +			raspberrypi,firmware = <&firmware>;
> +		};

No need to resend for this, but just a background note: DT node names
usually contain a device type not a device identity, so "clocks" not
"firmware-clocks" would be typical.

This patch,
Acked-by: Stephen Warren <swarren@wwwdotorg.org>

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

[Stephen Warren]

On 08/13/2015 05:05 PM, Eric Anholt wrote:
> Signed-off-by: Eric Anholt <eric@anholt.net>
> Acked-by: Lee Jones <lee@kernel.org>

A patch description might be nice, although admittedly the subject seems
clear enough.

> diff --git a/arch/arm/boot/dts/bcm2835-rpi.dtsi b/arch/arm/boot/dts/bcm2835-rpi.dtsi

> +		firmware_clocks: firmware-clocks {
> +			compatible = "raspberrypi,bcm2835-firmware-clocks";
> +			#clock-cells = <1>;
> +			raspberrypi,firmware = <&firmware>;
> +		};

No need to resend for this, but just a background note: DT node names
usually contain a device type not a device identity, so "clocks" not
"firmware-clocks" would be typical.

This patch,
Acked-by: Stephen Warren <swarren@wwwdotorg.org>

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

[Lee Jones]

On Fri, 14 Aug 2015, Stephen Warren wrote:

> On 08/13/2015 05:05 PM, Eric Anholt wrote:
> > Signed-off-by: Eric Anholt <eric@anholt.net>
> > Acked-by: Lee Jones <lee@kernel.org>
> 
> A patch description might be nice, although admittedly the subject seems
> clear enough.

Agreed that the subject is clear enough on this occasion.

> > diff --git a/arch/arm/boot/dts/bcm2835-rpi.dtsi b/arch/arm/boot/dts/bcm2835-rpi.dtsi
> 
> > +		firmware_clocks: firmware-clocks {
> > +			compatible = "raspberrypi,bcm2835-firmware-clocks";
> > +			#clock-cells = <1>;
> > +			raspberrypi,firmware = <&firmware>;
> > +		};
> 
> No need to resend for this, but just a background note: DT node names
> usually contain a device type not a device identity, so "clocks" not
> "firmware-clocks" would be typical.

Please fix this up when you re-send the set.

> This patch,
> Acked-by: Stephen Warren <swarren@wwwdotorg.org>

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

[Lee Jones]

On Fri, 14 Aug 2015, Stephen Warren wrote:

> On 08/13/2015 05:05 PM, Eric Anholt wrote:
> > Signed-off-by: Eric Anholt <eric@anholt.net>
> > Acked-by: Lee Jones <lee@kernel.org>
> 
> A patch description might be nice, although admittedly the subject seems
> clear enough.

Agreed that the subject is clear enough on this occasion.

> > diff --git a/arch/arm/boot/dts/bcm2835-rpi.dtsi b/arch/arm/boot/dts/bcm2835-rpi.dtsi
> 
> > +		firmware_clocks: firmware-clocks {
> > +			compatible = "raspberrypi,bcm2835-firmware-clocks";
> > +			#clock-cells = <1>;
> > +			raspberrypi,firmware = <&firmware>;
> > +		};
> 
> No need to resend for this, but just a background note: DT node names
> usually contain a device type not a device identity, so "clocks" not
> "firmware-clocks" would be typical.

Please fix this up when you re-send the set.

> This patch,
> Acked-by: Stephen Warren <swarren@wwwdotorg.org>