RPi firmware driver v2

RPi firmware driver v2

[Eric Anholt]

Here's a new version of the firmware driver.  This series applies on
top of for-rpi-next from Lee Jones's tree:

git://git.kernel.org/pub/scm/linux/kernel/git/rpi/linux-rpi.git

But it won't actually probe without merging in mailbox-for-next:

https://git.linaro.org/landing-teams/working/fujitsu/integration.git

I've dropped the pm-domains code for now, since it wasn't doing
anything useful for us yet without the drivers/base support.  However,
this code has been useful for building a proper clk provider for the
Pi, which can be found at:

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

Andrea Merello apparently got a cpufreq driver working on top of the
non-DT version of this clk provider:

https://github.com/andreamerello/linux-mach-bcm/tree/bcm2836-cpufreq

I think we might manage with a generic dt-cpufreq implementation,
though.

[PATCH 1/3 v2] dt/bindings: Add binding for the Raspberry Pi firmware driver

[Eric Anholt]

This driver will provide support for calls into the firmware that will
be used by other drivers like cpufreq and vc4.

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

v2: Improve commit message, point to mailbox.txt for how mboxes work.

 .../devicetree/bindings/arm/bcm/raspberrypi,firmware.txt | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/bcm/raspberrypi,firmware.txt

diff --git a/Documentation/devicetree/bindings/arm/bcm/raspberrypi,firmware.txt b/Documentation/devicetree/bindings/arm/bcm/raspberrypi,firmware.txt
new file mode 100644
index 0000000..33b0043
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/bcm/raspberrypi,firmware.txt
@@ -0,0 +1,16 @@
+Raspberry Pi VideoCore firmware driver
+
+Required properties:
+
+- compatible:		Should be "rasbperrypi,firmware"
+- mboxes:		Single-entry list which specifies which mailbox
+			  controller and channel is used.  See
+			  Documentation/devicetree/bindings/mailbox/mailbox.txt
+			  for the semantics of this property
+
+Example:
+
+firmware {
+	compatible = "rasbperrypi,firmware";
+	mboxes = <&mailbox>;
+};
-- 
2.1.4

[PATCH 2/3 v2] ARM: bcm2835: Add the Raspberry Pi firmware driver

[Eric Anholt]

This gives us a function for making mailbox property channel requests
of the firmware, which is most notable in that it will let us get and
set clock rates.

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

v2: Drop power-domains stuff for now since we don't have the driver
    core support to make it useful.  Move to drivers/firmware/.
    Capitalize the enums.  De-global the firmware variable.  Use the
    firmware device to allocate our DMA buffer, so that the dma-ranges
    DT property gets respected.  Simplify the property tag transaction
    interface even more, leaving a multi-tag interface still
    available.  For conciseness, rename "raspberrypi" to "rpi" on all
    functions/enums/structs, and the "firmware" variable to "fw".
    Print when the driver is probed successfully, since debugging
    -EPROBE_DEFER handling is such a big part of bcm2835 development.
    Drop -EBUSY mailbox handling since the mailbox core has been fixed
    to return -EPROBE_DEFER in -next.

Note that I don't think I've done what srwarren wanted for
-EPROBE_DEFER, because I'm not clear what he wants.  I think he might
just be asking for a function that does:

/*
 * Returns 0 if the firmware device is probed and available, otherwise
 * -EPROBE_DEFER.
 */
int rpi_firmware_get(struct device_node *firmware_node)
{
	struct platform_device *pdev = of_find_device_by_node(of_node);
	if (!platform_get_drvdata(pdev))
		return -EPROBE_DEFER;
	return 0;
}
EXPORT_SYMBOL(rpi_firmware_get)

If that's all, I'm happy to add it.

Note that a client could currently do this:

	ret = rpi_firmware_property_list(firmware_node, NULL, 0);

in exchange for a bit of overhead in the case that it's actually probed already.


 drivers/firmware/Makefile                          |   1 +
 drivers/firmware/raspberrypi.c                     | 224 +++++++++++++++++++++
 .../soc/bcm2835/raspberrypi-firmware-property.h    | 114 +++++++++++
 3 files changed, 339 insertions(+)
 create mode 100644 drivers/firmware/raspberrypi.c
 create mode 100644 include/soc/bcm2835/raspberrypi-firmware-property.h

diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
index 3fdd391..41ced28 100644
--- a/drivers/firmware/Makefile
+++ b/drivers/firmware/Makefile
@@ -13,6 +13,7 @@ obj-$(CONFIG_ISCSI_IBFT)	+= iscsi_ibft.o
 obj-$(CONFIG_FIRMWARE_MEMMAP)	+= memmap.o
 obj-$(CONFIG_QCOM_SCM)		+= qcom_scm.o
 CFLAGS_qcom_scm.o :=$(call as-instr,.arch_extension sec,-DREQUIRES_SEC=1)
+obj-$(CONFIG_BCM2835_MBOX)	+= raspberrypi.o
 
 obj-$(CONFIG_GOOGLE_FIRMWARE)	+= google/
 obj-$(CONFIG_EFI)		+= efi/
diff --git a/drivers/firmware/raspberrypi.c b/drivers/firmware/raspberrypi.c
new file mode 100644
index 0000000..61bde1b
--- /dev/null
+++ b/drivers/firmware/raspberrypi.c
@@ -0,0 +1,224 @@
+/*
+ *  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.
+ */
+
+/*
+ * Defines interfaces for interacting wtih the Raspberry Pi firmware's
+ * property channel.
+ */
+
+#include <linux/dma-mapping.h>
+#include <linux/mailbox_client.h>
+#include <linux/module.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <soc/bcm2835/raspberrypi-firmware-property.h>
+
+#define MBOX_MSG(chan, data28)		(((data28) & ~0xf) | ((chan) & 0xf))
+#define MBOX_CHAN(msg)			((msg) & 0xf)
+#define MBOX_DATA28(msg)		((msg) & ~0xf)
+#define MBOX_CHAN_PROPERTY		8
+
+struct rpi_firmware {
+	struct mbox_client cl;
+	struct mbox_chan *chan; /* The property channel. */
+	struct completion c;
+	u32 enabled;
+};
+
+static DEFINE_MUTEX(transaction_lock);
+
+static void response_callback(struct mbox_client *cl, void *msg)
+{
+	struct rpi_firmware *fw = container_of(cl, struct rpi_firmware, cl);
+	complete(&fw->c);
+}
+
+/*
+ * Sends a request to the firmware through the BCM2835 mailbox driver,
+ * and synchronously waits for the reply.
+ */
+static int
+rpi_firmware_transaction(struct rpi_firmware *fw, u32 chan, u32 data)
+{
+	u32 message = MBOX_MSG(chan, data);
+	int ret;
+
+	WARN_ON(data & 0xf);
+
+	mutex_lock(&transaction_lock);
+	reinit_completion(&fw->c);
+	ret = mbox_send_message(fw->chan, &message);
+	if (ret >= 0) {
+		wait_for_completion(&fw->c);
+		ret = 0;
+	} else {
+		dev_err(fw->cl.dev, "mbox_send_message returned %d\n", ret);
+	}
+	mutex_unlock(&transaction_lock);
+
+	return ret;
+}
+
+/*
+ * Submits a set of concatenated tags to the VPU firmware through the
+ * mailbox property interface.
+ *
+ * The buffer header and the ending tag are added by this function and
+ * don't need to be supplied, just the actual tags for your operation.
+ * See struct rpi_firmware_property_tag_header for the per-tag
+ * structure.
+ */
+int rpi_firmware_property_list(struct device_node *of_node,
+			       void *data, size_t tag_size)
+{
+	struct platform_device *pdev = of_find_device_by_node(of_node);
+	struct rpi_firmware *fw = platform_get_drvdata(pdev);
+	size_t size = tag_size + 12;
+	u32 *buf;
+	dma_addr_t bus_addr;
+	int ret = 0;
+
+	if (!fw)
+		return -EPROBE_DEFER;
+
+	/* Packets are processed a dword at a time. */
+	if (size & 3)
+		return -EINVAL;
+
+	buf = dma_alloc_coherent(fw->cl.dev, PAGE_ALIGN(size), &bus_addr,
+				 GFP_ATOMIC);
+	if (!buf)
+		return -ENOMEM;
+
+	/* The firmware will error out without parsing in this case. */
+	WARN_ON(size >= 1024 * 1024);
+
+	buf[0] = size;
+	buf[1] = RPI_FIRMWARE_STATUS_REQUEST;
+	memcpy(&buf[2], data, tag_size);
+	buf[size / 4 - 1] = RPI_FIRMWARE_PROPERTY_END;
+	wmb();
+
+	ret = rpi_firmware_transaction(fw, MBOX_CHAN_PROPERTY, bus_addr);
+
+	rmb();
+	memcpy(data, &buf[2], tag_size);
+	if (ret == 0 && buf[1] != RPI_FIRMWARE_STATUS_SUCCESS) {
+		/*
+		 * The tag name here might not be the one causing the
+		 * error, if there were multiple tags in the request.
+		 * But single-tag is the most common, so go with it.
+		 */
+		dev_err(fw->cl.dev, "Request 0x%08x returned status 0x%08x\n",
+			buf[2], buf[1]);
+		ret = -EINVAL;
+	}
+
+	dma_free_coherent(NULL, PAGE_ALIGN(size), buf, bus_addr);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(rpi_firmware_property_list);
+
+/*
+ * Submits a single tag to the VPU firmware through the mailbox
+ * property interface.
+ *
+ * This is a convenience wrapper around
+ * rpi_firmware_property_list() to avoid some of the
+ * boilerplate in property calls.
+ */
+int rpi_firmware_property(struct device_node *of_node,
+			  u32 tag, void *tag_data, size_t buf_size)
+{
+	/* Single tags are very small (generally 8 bytes), so the
+	 * stack should be safe.
+	 */
+	u8 data[buf_size + sizeof(struct rpi_firmware_property_tag_header)];
+	struct rpi_firmware_property_tag_header *header =
+		(struct rpi_firmware_property_tag_header *)data;
+	int ret;
+
+	header->tag = tag;
+	header->buf_size = buf_size;
+	header->req_resp_size = 0;
+	memcpy(data + sizeof(struct rpi_firmware_property_tag_header),
+	       tag_data, buf_size);
+
+	ret = rpi_firmware_property_list(of_node, &data, sizeof(data));
+	memcpy(tag_data,
+	       data + sizeof(struct rpi_firmware_property_tag_header),
+	       buf_size);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(rpi_firmware_property);
+
+static int rpi_firmware_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	int ret = 0;
+	struct rpi_firmware *fw;
+
+	fw = devm_kzalloc(dev, sizeof(*fw), GFP_KERNEL);
+	if (!fw)
+		return -ENOMEM;
+
+	fw->cl.dev = dev;
+	fw->cl.rx_callback = response_callback;
+	fw->cl.tx_block = true;
+
+	fw->chan = mbox_request_channel(&fw->cl, 0);
+	if (IS_ERR(fw->chan)) {
+		ret = PTR_ERR(fw->chan);
+		/* An -EBUSY from the core means it couldn't find our
+		 * channel, because the mailbox driver hadn't
+		 * registered yet.
+		 */
+		if (ret != -EPROBE_DEFER)
+			dev_err(dev, "Failed to get mbox channel: %d\n", ret);
+		return ret;
+	}
+
+	init_completion(&fw->c);
+
+	dev_info(dev, "Firmware driver\n");
+	platform_set_drvdata(pdev, fw);
+
+	return 0;
+}
+
+static int rpi_firmware_remove(struct platform_device *pdev)
+{
+	struct rpi_firmware *fw = platform_get_drvdata(pdev);
+
+	mbox_free_channel(fw->chan);
+
+	return 0;
+}
+
+static const struct of_device_id rpi_firmware_of_match[] = {
+	{ .compatible = "raspberrypi,firmware", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, rpi_firmware_of_match);
+
+static struct platform_driver rpi_firmware_driver = {
+	.driver = {
+		.name = "raspberrypi-firmware",
+		.owner = THIS_MODULE,
+		.of_match_table = rpi_firmware_of_match,
+	},
+	.probe		= rpi_firmware_probe,
+	.remove		= rpi_firmware_remove,
+};
+module_platform_driver(rpi_firmware_driver);
+
+MODULE_AUTHOR("Eric Anholt <eric@anholt.net>");
+MODULE_DESCRIPTION("Raspberry Pi firmware driver");
+MODULE_LICENSE("GPL v2");
diff --git a/include/soc/bcm2835/raspberrypi-firmware-property.h b/include/soc/bcm2835/raspberrypi-firmware-property.h
new file mode 100644
index 0000000..2557cab
--- /dev/null
+++ b/include/soc/bcm2835/raspberrypi-firmware-property.h
@@ -0,0 +1,114 @@
+/*
+ *  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/types.h>
+#include <linux/of_device.h>
+
+enum rpi_firmware_property_status {
+	RPI_FIRMWARE_STATUS_REQUEST = 0,
+	RPI_FIRMWARE_STATUS_SUCCESS = 0x80000000,
+	RPI_FIRMWARE_STATUS_ERROR =   0x80000001,
+};
+
+struct rpi_firmware_property_tag_header {
+	/* One of enum_mbox_property_tag. */
+	u32 tag;
+
+	/* The number of bytes in the value buffer following this
+	 * struct.
+	 */
+	u32 buf_size;
+
+	/*
+	 * On submit, the length of the request (though it doesn't
+	 * appear to be currently used by the firmware).  On return,
+	 * the length of the response (always 4 byte aligned), with
+	 * the low bit set.
+	 */
+	u32 req_resp_size;
+};
+
+enum rpi_firmware_property_tag {
+	RPI_FIRMWARE_PROPERTY_END =                           0,
+	RPI_FIRMWARE_GET_FIRMWARE_REVISION =                  0x00000001,
+
+	RPI_FIRMWARE_SET_CURSOR_INFO =                        0x00008010,
+	RPI_FIRMWARE_SET_CURSOR_STATE =                       0x00008011,
+
+	RPI_FIRMWARE_GET_BOARD_MODEL =                        0x00010001,
+	RPI_FIRMWARE_GET_BOARD_REVISION =                     0x00010002,
+	RPI_FIRMWARE_GET_BOARD_MAC_ADDRESS =                  0x00010003,
+	RPI_FIRMWARE_GET_BOARD_SERIAL =                       0x00010004,
+	RPI_FIRMWARE_GET_ARM_MEMORY =                         0x00010005,
+	RPI_FIRMWARE_GET_VC_MEMORY =                          0x00010006,
+	RPI_FIRMWARE_GET_CLOCKS =                             0x00010007,
+	RPI_FIRMWARE_GET_POWER_STATE =                        0x00020001,
+	RPI_FIRMWARE_GET_TIMING =                             0x00020002,
+	RPI_FIRMWARE_SET_POWER_STATE =                        0x00028001,
+	RPI_FIRMWARE_GET_CLOCK_STATE =                        0x00030001,
+	RPI_FIRMWARE_GET_CLOCK_RATE =                         0x00030002,
+	RPI_FIRMWARE_GET_VOLTAGE =                            0x00030003,
+	RPI_FIRMWARE_GET_MAX_CLOCK_RATE =                     0x00030004,
+	RPI_FIRMWARE_GET_MAX_VOLTAGE =                        0x00030005,
+	RPI_FIRMWARE_GET_TEMPERATURE =                        0x00030006,
+	RPI_FIRMWARE_GET_MIN_CLOCK_RATE =                     0x00030007,
+	RPI_FIRMWARE_GET_MIN_VOLTAGE =                        0x00030008,
+	RPI_FIRMWARE_GET_TURBO =                              0x00030009,
+	RPI_FIRMWARE_GET_MAX_TEMPERATURE =                    0x0003000a,
+	RPI_FIRMWARE_ALLOCATE_MEMORY =                        0x0003000c,
+	RPI_FIRMWARE_LOCK_MEMORY =                            0x0003000d,
+	RPI_FIRMWARE_UNLOCK_MEMORY =                          0x0003000e,
+	RPI_FIRMWARE_RELEASE_MEMORY =                         0x0003000f,
+	RPI_FIRMWARE_EXECUTE_CODE =                           0x00030010,
+	RPI_FIRMWARE_EXECUTE_QPU =                            0x00030011,
+	RPI_FIRMWARE_SET_ENABLE_QPU =                         0x00030012,
+	RPI_FIRMWARE_GET_DISPMANX_RESOURCE_MEM_HANDLE =       0x00030014,
+	RPI_FIRMWARE_GET_EDID_BLOCK =                         0x00030020,
+	RPI_FIRMWARE_SET_CLOCK_STATE =                        0x00038001,
+	RPI_FIRMWARE_SET_CLOCK_RATE =                         0x00038002,
+	RPI_FIRMWARE_SET_VOLTAGE =                            0x00038003,
+	RPI_FIRMWARE_SET_TURBO =                              0x00038009,
+
+	/* Dispmanx TAGS */
+	RPI_FIRMWARE_FRAMEBUFFER_ALLOCATE =                   0x00040001,
+	RPI_FIRMWARE_FRAMEBUFFER_BLANK =                      0x00040002,
+	RPI_FIRMWARE_FRAMEBUFFER_GET_PHYSICAL_WIDTH_HEIGHT =  0x00040003,
+	RPI_FIRMWARE_FRAMEBUFFER_GET_VIRTUAL_WIDTH_HEIGHT =   0x00040004,
+	RPI_FIRMWARE_FRAMEBUFFER_GET_DEPTH =                  0x00040005,
+	RPI_FIRMWARE_FRAMEBUFFER_GET_PIXEL_ORDER =            0x00040006,
+	RPI_FIRMWARE_FRAMEBUFFER_GET_ALPHA_MODE =             0x00040007,
+	RPI_FIRMWARE_FRAMEBUFFER_GET_PITCH =                  0x00040008,
+	RPI_FIRMWARE_FRAMEBUFFER_GET_VIRTUAL_OFFSET =         0x00040009,
+	RPI_FIRMWARE_FRAMEBUFFER_GET_OVERSCAN =               0x0004000a,
+	RPI_FIRMWARE_FRAMEBUFFER_GET_PALETTE =                0x0004000b,
+	RPI_FIRMWARE_FRAMEBUFFER_RELEASE =                    0x00048001,
+	RPI_FIRMWARE_FRAMEBUFFER_TEST_PHYSICAL_WIDTH_HEIGHT = 0x00044003,
+	RPI_FIRMWARE_FRAMEBUFFER_TEST_VIRTUAL_WIDTH_HEIGHT =  0x00044004,
+	RPI_FIRMWARE_FRAMEBUFFER_TEST_DEPTH =                 0x00044005,
+	RPI_FIRMWARE_FRAMEBUFFER_TEST_PIXEL_ORDER =           0x00044006,
+	RPI_FIRMWARE_FRAMEBUFFER_TEST_ALPHA_MODE =            0x00044007,
+	RPI_FIRMWARE_FRAMEBUFFER_TEST_VIRTUAL_OFFSET =        0x00044009,
+	RPI_FIRMWARE_FRAMEBUFFER_TEST_OVERSCAN =              0x0004400a,
+	RPI_FIRMWARE_FRAMEBUFFER_TEST_PALETTE =               0x0004400b,
+	RPI_FIRMWARE_FRAMEBUFFER_SET_PHYSICAL_WIDTH_HEIGHT =  0x00048003,
+	RPI_FIRMWARE_FRAMEBUFFER_SET_VIRTUAL_WIDTH_HEIGHT =   0x00048004,
+	RPI_FIRMWARE_FRAMEBUFFER_SET_DEPTH =                  0x00048005,
+	RPI_FIRMWARE_FRAMEBUFFER_SET_PIXEL_ORDER =            0x00048006,
+	RPI_FIRMWARE_FRAMEBUFFER_SET_ALPHA_MODE =             0x00048007,
+	RPI_FIRMWARE_FRAMEBUFFER_SET_VIRTUAL_OFFSET =         0x00048009,
+	RPI_FIRMWARE_FRAMEBUFFER_SET_OVERSCAN =               0x0004800a,
+	RPI_FIRMWARE_FRAMEBUFFER_SET_PALETTE =                0x0004800b,
+
+	RPI_FIRMWARE_GET_COMMAND_LINE =                       0x00050001,
+	RPI_FIRMWARE_GET_DMA_CHANNELS =                       0x00060001,
+};
+
+int rpi_firmware_property(struct device_node *of_node,
+			  u32 tag, void *data, size_t len);
+int rpi_firmware_property_list(struct device_node *of_node,
+			       void *data, size_t tag_size);
-- 
2.1.4

[PATCH 3/3 v2] ARM: bcm2835: Add the firmware driver information to the RPi DT

[Eric Anholt]

Signed-off-by: Eric Anholt <eric@anholt.net>
Acked-by: Lee Jones <lee@kernel.org> (previous version with pm-domains)
---

v2: Drop pm-domains stuff since I've dropped it from the firmware
    driver for now, until we get drivers/base fixed.

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

diff --git a/arch/arm/boot/dts/bcm2835-rpi.dtsi b/arch/arm/boot/dts/bcm2835-rpi.dtsi
index 46780bb..ace33f6 100644
--- a/arch/arm/boot/dts/bcm2835-rpi.dtsi
+++ b/arch/arm/boot/dts/bcm2835-rpi.dtsi
@@ -14,6 +14,13 @@
 			linux,default-trigger = "heartbeat";
 		};
 	};
+
+	soc {
+		firmware: firmware {
+			compatible = "raspberrypi,firmware";
+			mboxes = <&mailbox>;
+		};
+	};
 };
 
 &gpio {
-- 
2.1.4

Re: [PATCH 1/3 v2] dt/bindings: Add binding for the Raspberry Pi firmware driver

[Lee Jones]

On Wed, 13 May 2015, Eric Anholt wrote:

> This driver will provide support for calls into the firmware that will
> be used by other drivers like cpufreq and vc4.
> 
> Signed-off-by: Eric Anholt <eric@anholt.net>
> ---
> 
> v2: Improve commit message, point to mailbox.txt for how mboxes work.
> 
>  .../devicetree/bindings/arm/bcm/raspberrypi,firmware.txt | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/arm/bcm/raspberrypi,firmware.txt
> 
> diff --git a/Documentation/devicetree/bindings/arm/bcm/raspberrypi,firmware.txt b/Documentation/devicetree/bindings/arm/bcm/raspberrypi,firmware.txt
> new file mode 100644
> index 0000000..33b0043
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/bcm/raspberrypi,firmware.txt
> @@ -0,0 +1,16 @@
> +Raspberry Pi VideoCore firmware driver
> +
> +Required properties:
> +
> +- compatible:		Should be "rasbperrypi,firmware"
> +- mboxes:		Single-entry list which specifies which mailbox
> +			  controller and channel is used.  See
> +			  Documentation/devicetree/bindings/mailbox/mailbox.txt
> +			  for the semantics of this property

That's not what it looks like to me.  There is no mention of channels
in a 0 cell property.  Keep it simple.  How about:

Phandle to the firmware device's Mailbox.
(See: ../mailbox/mailbox.txt for more information)

> +Example:
> +
> +firmware {
> +	compatible = "rasbperrypi,firmware";
> +	mboxes = <&mailbox>;
> +};

Re: [PATCH 1/3 v2] dt/bindings: Add binding for the Raspberry Pi firmware driver

[Eric Anholt]

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

Lee Jones <lee@kernel.org> writes:

> On Wed, 13 May 2015, Eric Anholt wrote:
>
>> This driver will provide support for calls into the firmware that will
>> be used by other drivers like cpufreq and vc4.
>> 
>> Signed-off-by: Eric Anholt <eric@anholt.net>
>> ---
>> 
>> v2: Improve commit message, point to mailbox.txt for how mboxes work.
>> 
>>  .../devicetree/bindings/arm/bcm/raspberrypi,firmware.txt | 16 ++++++++++++++++
>>  1 file changed, 16 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/arm/bcm/raspberrypi,firmware.txt
>> 
>> diff --git a/Documentation/devicetree/bindings/arm/bcm/raspberrypi,firmware.txt b/Documentation/devicetree/bindings/arm/bcm/raspberrypi,firmware.txt
>> new file mode 100644
>> index 0000000..33b0043
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/arm/bcm/raspberrypi,firmware.txt
>> @@ -0,0 +1,16 @@
>> +Raspberry Pi VideoCore firmware driver
>> +
>> +Required properties:
>> +
>> +- compatible:		Should be "rasbperrypi,firmware"
>> +- mboxes:		Single-entry list which specifies which mailbox
>> +			  controller and channel is used.  See
>> +			  Documentation/devicetree/bindings/mailbox/mailbox.txt
>> +			  for the semantics of this property
>
> That's not what it looks like to me.  There is no mention of channels
> in a 0 cell property.  Keep it simple.  How about:
>
> Phandle to the firmware device's Mailbox.
> (See: ../mailbox/mailbox.txt for more information)

Sounds good to me.

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

Re: [PATCH 1/3 v2] dt/bindings: Add binding for the Raspberry Pi firmware driver

[Noralf Trønnes]

Den 13.05.2015 21:00, skrev Eric Anholt:
> This driver will provide support for calls into the firmware that will
> be used by other drivers like cpufreq and vc4.
>
> Signed-off-by: Eric Anholt <eric@anholt.net>
> ---
>
> v2: Improve commit message, point to mailbox.txt for how mboxes work.
>
>   .../devicetree/bindings/arm/bcm/raspberrypi,firmware.txt | 16 ++++++++++++++++
>   1 file changed, 16 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/arm/bcm/raspberrypi,firmware.txt
>
> diff --git a/Documentation/devicetree/bindings/arm/bcm/raspberrypi,firmware.txt b/Documentation/devicetree/bindings/arm/bcm/raspberrypi,firmware.txt
> new file mode 100644
> index 0000000..33b0043
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/bcm/raspberrypi,firmware.txt
> @@ -0,0 +1,16 @@
> +Raspberry Pi VideoCore firmware driver
> +
> +Required properties:
> +
> +- compatible:		Should be "rasbperrypi,firmware"
> +- mboxes:		Single-entry list which specifies which mailbox
> +			  controller and channel is used.  See
> +			  Documentation/devicetree/bindings/mailbox/mailbox.txt
> +			  for the semantics of this property
> +
> +Example:
> +
> +firmware {
> +	compatible = "rasbperrypi,firmware";

Typo: rasbperrypi -> raspberrypi

> +	mboxes = <&mailbox>;
> +};

Re: [PATCH 2/3 v2] ARM: bcm2835: Add the Raspberry Pi firmware driver

[Noralf Trønnes]

Den 13.05.2015 21:00, skrev Eric Anholt:
> This gives us a function for making mailbox property channel requests
> of the firmware, which is most notable in that it will let us get and
> set clock rates.
>
> Signed-off-by: Eric Anholt <eric@anholt.net>
> ---
>
> v2: Drop power-domains stuff for now since we don't have the driver
>      core support to make it useful.  Move to drivers/firmware/.
>      Capitalize the enums.  De-global the firmware variable.  Use the
>      firmware device to allocate our DMA buffer, so that the dma-ranges
>      DT property gets respected.  Simplify the property tag transaction
>      interface even more, leaving a multi-tag interface still
>      available.  For conciseness, rename "raspberrypi" to "rpi" on all
>      functions/enums/structs, and the "firmware" variable to "fw".
>      Print when the driver is probed successfully, since debugging
>      -EPROBE_DEFER handling is such a big part of bcm2835 development.
>      Drop -EBUSY mailbox handling since the mailbox core has been fixed
>      to return -EPROBE_DEFER in -next.
>
> Note that I don't think I've done what srwarren wanted for
> -EPROBE_DEFER, because I'm not clear what he wants.  I think he might
> just be asking for a function that does:
>
> /*
>   * Returns 0 if the firmware device is probed and available, otherwise
>   * -EPROBE_DEFER.
>   */
> int rpi_firmware_get(struct device_node *firmware_node)
> {
> 	struct platform_device *pdev = of_find_device_by_node(of_node);
> 	if (!platform_get_drvdata(pdev))
> 		return -EPROBE_DEFER;
> 	return 0;
> }
> EXPORT_SYMBOL(rpi_firmware_get)
>
> If that's all, I'm happy to add it.
>
> Note that a client could currently do this:
>
> 	ret = rpi_firmware_property_list(firmware_node, NULL, 0);
>
> in exchange for a bit of overhead in the case that it's actually probed already.
>
[...]
> diff --git a/drivers/firmware/raspberrypi.c b/drivers/firmware/raspberrypi.c
[...]
> +int rpi_firmware_property_list(struct device_node *of_node,
> +			       void *data, size_t tag_size)
> +{
> +	struct platform_device *pdev = of_find_device_by_node(of_node);
> +	struct rpi_firmware *fw = platform_get_drvdata(pdev);
> +	size_t size = tag_size + 12;
> +	u32 *buf;
> +	dma_addr_t bus_addr;
> +	int ret = 0;
> +
> +	if (!fw)
> +		return -EPROBE_DEFER;
> +
> +	/* Packets are processed a dword at a time. */
> +	if (size & 3)
> +		return -EINVAL;
> +
> +	buf = dma_alloc_coherent(fw->cl.dev, PAGE_ALIGN(size), &bus_addr,
[...]
> +	dma_free_coherent(NULL, PAGE_ALIGN(size), buf, bus_addr);

Should probably pass the device when freeing as well.

> +static int rpi_firmware_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	int ret = 0;
> +	struct rpi_firmware *fw;
> +
> +	fw = devm_kzalloc(dev, sizeof(*fw), GFP_KERNEL);
> +	if (!fw)
> +		return -ENOMEM;
> +
> +	fw->cl.dev = dev;
> +	fw->cl.rx_callback = response_callback;
> +	fw->cl.tx_block = true;
> +
> +	fw->chan = mbox_request_channel(&fw->cl, 0);
> +	if (IS_ERR(fw->chan)) {

Definition of ret can be move here.

> +		ret = PTR_ERR(fw->chan);
> +		/* An -EBUSY from the core means it couldn't find our
> +		 * channel, because the mailbox driver hadn't
> +		 * registered yet.
> +		 */

You forgot to remove this comment.

> +		if (ret != -EPROBE_DEFER)
> +			dev_err(dev, "Failed to get mbox channel: %d\n", ret);
> +		return ret;
> +	}

Why not turn the comments into kernel-doc comments:
(Thunderbird converts my tabs into spaces, sorry about that)

/**
  * struct rpi_firmware_property_tag_header - Firmware property tag header
  * @tag:        One of enum_mbox_property_tag.
  * @buf_size:   The number of bytes in the value buffer following this 
struct.
  * @req_resp_size: On submit, the length of the request (though it doesn't
  *                 appear to be currently used by the firmware).  On 
return,
  *                 the length of the response (always 4 byte aligned), with
  *                 the low bit set.
  */
struct rpi_firmware_property_tag_header {
         u32 tag;
         u32 buf_size;
         u32 req_resp_size;
};


/**
  * rpi_firmware_property_list - Submit firmware property list
  * @of_node:    Pointer to the firmware Device Tree node.
  * @data:       Buffer holding tags.
  * @tag_size:   Size of tags buffer.
  *
  * Submits a set of concatenated tags to the VPU firmware through the
  * mailbox property interface.
  *
  * The buffer header and the ending tag are added by this function and
  * don't need to be supplied, just the actual tags for your operation.
  * See struct rpi_firmware_property_tag_header for the per-tag
  * structure.
  */
int rpi_firmware_property_list(struct device_node *of_node,
                                void *data, size_t tag_size)


/**
  * rpi_firmware_property - Submit single firmware property
  * @of_node:    Pointer to the firmware Device Tree node.
  * @tag:        One of enum_mbox_property_tag.
  * @tag_data:   Tag data buffer.
  * @buf_size:   Buffer size.
  *
  * Submits a single tag to the VPU firmware through the mailbox
  * property interface.
  *
  * This is a convenience wrapper around
  * rpi_firmware_property_list() to avoid some of the
  * boilerplate in property calls.
  */
int rpi_firmware_property(struct device_node *of_node,
                           u32 tag, void *tag_data, size_t buf_size)

Re: [PATCH 2/3 v2] ARM: bcm2835: Add the Raspberry Pi firmware driver

[Noralf Trønnes]

Den 13.05.2015 21:00, skrev Eric Anholt:
> This gives us a function for making mailbox property channel requests
> of the firmware, which is most notable in that it will let us get and
> set clock rates.
>
> Signed-off-by: Eric Anholt <eric@anholt.net>
> ---

[...]

> +/*
> + * Submits a single tag to the VPU firmware through the mailbox
> + * property interface.
> + *
> + * This is a convenience wrapper around
> + * rpi_firmware_property_list() to avoid some of the
> + * boilerplate in property calls.
> + */
> +int rpi_firmware_property(struct device_node *of_node,
> +			  u32 tag, void *tag_data, size_t buf_size)

To use the firmware property functions, I need a DT node pointer.
Since Device Tree is dynamic now, should I fetch the firmware node
each time, or should I do that in probe and store the node pointer?

Device Tree:
         firmware: firmware {
             compatible = "raspberrypi,firmware";
         };

         thermal {
             compatible = "brcm,bcm2835-thermal";
             firmware = <&firmware>;
         };

Rewritten (not tested) function from downstream bcm2835-thermal driver:

static int bcm2835_get_temp_or_max(struct thermal_zone_device *thermal_dev,
                                    unsigned long *temp, unsigned tag_id)
{
     struct device *dev = <get device somehow>;
     struct device_node *np;
     struct {
         u32 id;
         u32 val;
     } tag_buf;

     np = of_parse_phandle(dev->of_node, "firmware", 0);
     if (!np)
         return -EINVAL;

     ret = rpi_firmware_property(np, tag_id, &tag_buf, sizeof(tag_buf));
     if (ret)
         return ret;

     *temp = val;

     return 0;
}

Re: [PATCH 2/3 v2] ARM: bcm2835: Add the Raspberry Pi firmware driver

[Eric Anholt]

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

Noralf Trønnes <noralf@tronnes.org> writes:

> Den 13.05.2015 21:00, skrev Eric Anholt:
>> This gives us a function for making mailbox property channel requests
>> of the firmware, which is most notable in that it will let us get and
>> set clock rates.
>>
>> Signed-off-by: Eric Anholt <eric@anholt.net>
>> ---
>
> [...]
>
>> +/*
>> + * Submits a single tag to the VPU firmware through the mailbox
>> + * property interface.
>> + *
>> + * This is a convenience wrapper around
>> + * rpi_firmware_property_list() to avoid some of the
>> + * boilerplate in property calls.
>> + */
>> +int rpi_firmware_property(struct device_node *of_node,
>> +			  u32 tag, void *tag_data, size_t buf_size)
>
> To use the firmware property functions, I need a DT node pointer.
> Since Device Tree is dynamic now, should I fetch the firmware node
> each time, or should I do that in probe and store the node pointer?
>
> Device Tree:
>          firmware: firmware {
>              compatible = "raspberrypi,firmware";
>          };
>
>          thermal {
>              compatible = "brcm,bcm2835-thermal";
>              firmware = <&firmware>;
>          };

I'm doing it in probe in my clients -- I don't see why the firmware
device's node would change.

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

[PATCH 1/3 v3] dt/bindings: Add binding for the Raspberry Pi firmware driver

[Eric Anholt]

This driver will provide support for calls into the firmware that will
be used by other drivers like cpufreq and vc4.
---

v2: Improve commit message, point to mailbox.txt for how mboxes work.
v3: Use Lee's suggestion for mailbox phandle docs, fix spelling of
    "raspberry".

 .../devicetree/bindings/arm/bcm/raspberrypi,firmware.txt   | 14 ++++++++++++++
 1 file changed, 14 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/bcm/raspberrypi,firmware.txt

diff --git a/Documentation/devicetree/bindings/arm/bcm/raspberrypi,firmware.txt b/Documentation/devicetree/bindings/arm/bcm/raspberrypi,firmware.txt
new file mode 100644
index 0000000..dbf60f9
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/bcm/raspberrypi,firmware.txt
@@ -0,0 +1,14 @@
+Raspberry Pi VideoCore firmware driver
+
+Required properties:
+
+- compatible:		Should be "rasbperrypi,firmware"
+- mboxes:		Phandle to the firmware device's Mailbox.
+			  (See: ../mailbox/mailbox.txt for more information)
+
+Example:
+
+firmware {
+	compatible = "raspberrypi,firmware";
+	mboxes = <&mailbox>;
+};
-- 
2.1.4

[PATCH 2/3 v3] ARM: bcm2835: Add the Raspberry Pi firmware driver

[Eric Anholt]

This gives us a function for making mailbox property channel requests
of the firmware, which is most notable in that it will let us get and
set clock rates.

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

v2: Drop power-domains stuff for now since we don't have the driver
    core support to make it useful.  Move to drivers/firmware/.
    Capitalize the enums.  De-global the firmware variable.  Use the
    firmware device to allocate our DMA buffer, so that the dma-ranges
    DT property gets respected.  Simplify the property tag transaction
    interface even more, leaving a multi-tag interface still
    available.  For conciseness, rename "raspberrypi" to "rpi" on all
    functions/enums/structs, and the "firmware" variable to "fw".
    Print when the driver is probed successfully, since debugging
    -EPROBE_DEFER handling is such a big part of bcm2835 development.
    Drop -EBUSY mailbox handling since the mailbox core has been fixed
    to return -EPROBE_DEFER in -next.

v3: Use kernel-doc style for big comments (from Noralf), drop stale
    comment, use "dev" when freeing DMA as well.

 drivers/firmware/Makefile                          |   1 +
 drivers/firmware/raspberrypi.c                     | 230 +++++++++++++++++++++
 .../soc/bcm2835/raspberrypi-firmware-property.h    | 112 ++++++++++
 3 files changed, 343 insertions(+)
 create mode 100644 drivers/firmware/raspberrypi.c
 create mode 100644 include/soc/bcm2835/raspberrypi-firmware-property.h

diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
index 3fdd391..41ced28 100644
--- a/drivers/firmware/Makefile
+++ b/drivers/firmware/Makefile
@@ -13,6 +13,7 @@ obj-$(CONFIG_ISCSI_IBFT)	+= iscsi_ibft.o
 obj-$(CONFIG_FIRMWARE_MEMMAP)	+= memmap.o
 obj-$(CONFIG_QCOM_SCM)		+= qcom_scm.o
 CFLAGS_qcom_scm.o :=$(call as-instr,.arch_extension sec,-DREQUIRES_SEC=1)
+obj-$(CONFIG_BCM2835_MBOX)	+= raspberrypi.o
 
 obj-$(CONFIG_GOOGLE_FIRMWARE)	+= google/
 obj-$(CONFIG_EFI)		+= efi/
diff --git a/drivers/firmware/raspberrypi.c b/drivers/firmware/raspberrypi.c
new file mode 100644
index 0000000..d612815
--- /dev/null
+++ b/drivers/firmware/raspberrypi.c
@@ -0,0 +1,230 @@
+/*
+ *  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.
+ */
+
+/*
+ * Defines interfaces for interacting wtih the Raspberry Pi firmware's
+ * property channel.
+ */
+
+#include <linux/dma-mapping.h>
+#include <linux/mailbox_client.h>
+#include <linux/module.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <soc/bcm2835/raspberrypi-firmware-property.h>
+
+#define MBOX_MSG(chan, data28)		(((data28) & ~0xf) | ((chan) & 0xf))
+#define MBOX_CHAN(msg)			((msg) & 0xf)
+#define MBOX_DATA28(msg)		((msg) & ~0xf)
+#define MBOX_CHAN_PROPERTY		8
+
+struct rpi_firmware {
+	struct mbox_client cl;
+	struct mbox_chan *chan; /* The property channel. */
+	struct completion c;
+	u32 enabled;
+};
+
+static DEFINE_MUTEX(transaction_lock);
+
+static void response_callback(struct mbox_client *cl, void *msg)
+{
+	struct rpi_firmware *fw = container_of(cl, struct rpi_firmware, cl);
+	complete(&fw->c);
+}
+
+/*
+ * Sends a request to the firmware through the BCM2835 mailbox driver,
+ * and synchronously waits for the reply.
+ */
+static int
+rpi_firmware_transaction(struct rpi_firmware *fw, u32 chan, u32 data)
+{
+	u32 message = MBOX_MSG(chan, data);
+	int ret;
+
+	WARN_ON(data & 0xf);
+
+	mutex_lock(&transaction_lock);
+	reinit_completion(&fw->c);
+	ret = mbox_send_message(fw->chan, &message);
+	if (ret >= 0) {
+		wait_for_completion(&fw->c);
+		ret = 0;
+	} else {
+		dev_err(fw->cl.dev, "mbox_send_message returned %d\n", ret);
+	}
+	mutex_unlock(&transaction_lock);
+
+	return ret;
+}
+
+/**
+ * rpi_firmware_property_list - Submit firmware property list
+ * @of_node:	Pointer to the firmware Device Tree node.
+ * @data:	Buffer holding tags.
+ * @tag_size:	Size of tags buffer.
+ *
+ * Submits a set of concatenated tags to the VPU firmware through the
+ * mailbox property interface.
+ *
+ * The buffer header and the ending tag are added by this function and
+ * don't need to be supplied, just the actual tags for your operation.
+ * See struct rpi_firmware_property_tag_header for the per-tag
+ * structure.
+ */
+int rpi_firmware_property_list(struct device_node *of_node,
+			       void *data, size_t tag_size)
+{
+	struct platform_device *pdev = of_find_device_by_node(of_node);
+	struct rpi_firmware *fw = platform_get_drvdata(pdev);
+	size_t size = tag_size + 12;
+	u32 *buf;
+	dma_addr_t bus_addr;
+	int ret = 0;
+
+	if (!fw)
+		return -EPROBE_DEFER;
+
+	/* Packets are processed a dword at a time. */
+	if (size & 3)
+		return -EINVAL;
+
+	buf = dma_alloc_coherent(fw->cl.dev, PAGE_ALIGN(size), &bus_addr,
+				 GFP_ATOMIC);
+	if (!buf)
+		return -ENOMEM;
+
+	/* The firmware will error out without parsing in this case. */
+	WARN_ON(size >= 1024 * 1024);
+
+	buf[0] = size;
+	buf[1] = RPI_FIRMWARE_STATUS_REQUEST;
+	memcpy(&buf[2], data, tag_size);
+	buf[size / 4 - 1] = RPI_FIRMWARE_PROPERTY_END;
+	wmb();
+
+	ret = rpi_firmware_transaction(fw, MBOX_CHAN_PROPERTY, bus_addr);
+
+	rmb();
+	memcpy(data, &buf[2], tag_size);
+	if (ret == 0 && buf[1] != RPI_FIRMWARE_STATUS_SUCCESS) {
+		/*
+		 * The tag name here might not be the one causing the
+		 * error, if there were multiple tags in the request.
+		 * But single-tag is the most common, so go with it.
+		 */
+		dev_err(fw->cl.dev, "Request 0x%08x returned status 0x%08x\n",
+			buf[2], buf[1]);
+		ret = -EINVAL;
+	}
+
+	dma_free_coherent(fw->cl.dev, PAGE_ALIGN(size), buf, bus_addr);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(rpi_firmware_property_list);
+
+/**
+ * rpi_firmware_property - Submit single firmware property
+ * @of_node:	Pointer to the firmware Device Tree node.
+ * @tag:	One of enum_mbox_property_tag.
+ * @tag_data:	Tag data buffer.
+ * @buf_size:	Buffer size.
+ *
+ * Submits a single tag to the VPU firmware through the mailbox
+ * property interface.
+ *
+ * This is a convenience wrapper around
+ * rpi_firmware_property_list() to avoid some of the
+ * boilerplate in property calls.
+ */
+int rpi_firmware_property(struct device_node *of_node,
+			  u32 tag, void *tag_data, size_t buf_size)
+{
+	/* Single tags are very small (generally 8 bytes), so the
+	 * stack should be safe.
+	 */
+	u8 data[buf_size + sizeof(struct rpi_firmware_property_tag_header)];
+	struct rpi_firmware_property_tag_header *header =
+		(struct rpi_firmware_property_tag_header *)data;
+	int ret;
+
+	header->tag = tag;
+	header->buf_size = buf_size;
+	header->req_resp_size = 0;
+	memcpy(data + sizeof(struct rpi_firmware_property_tag_header),
+	       tag_data, buf_size);
+
+	ret = rpi_firmware_property_list(of_node, &data, sizeof(data));
+	memcpy(tag_data,
+	       data + sizeof(struct rpi_firmware_property_tag_header),
+	       buf_size);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(rpi_firmware_property);
+
+static int rpi_firmware_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct rpi_firmware *fw;
+
+	fw = devm_kzalloc(dev, sizeof(*fw), GFP_KERNEL);
+	if (!fw)
+		return -ENOMEM;
+
+	fw->cl.dev = dev;
+	fw->cl.rx_callback = response_callback;
+	fw->cl.tx_block = true;
+
+	fw->chan = mbox_request_channel(&fw->cl, 0);
+	if (IS_ERR(fw->chan)) {
+		int ret = PTR_ERR(fw->chan);
+		if (ret != -EPROBE_DEFER)
+			dev_err(dev, "Failed to get mbox channel: %d\n", ret);
+		return ret;
+	}
+
+	init_completion(&fw->c);
+
+	dev_info(dev, "Firmware driver\n");
+	platform_set_drvdata(pdev, fw);
+
+	return 0;
+}
+
+static int rpi_firmware_remove(struct platform_device *pdev)
+{
+	struct rpi_firmware *fw = platform_get_drvdata(pdev);
+
+	mbox_free_channel(fw->chan);
+
+	return 0;
+}
+
+static const struct of_device_id rpi_firmware_of_match[] = {
+	{ .compatible = "raspberrypi,firmware", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, rpi_firmware_of_match);
+
+static struct platform_driver rpi_firmware_driver = {
+	.driver = {
+		.name = "raspberrypi-firmware",
+		.owner = THIS_MODULE,
+		.of_match_table = rpi_firmware_of_match,
+	},
+	.probe		= rpi_firmware_probe,
+	.remove		= rpi_firmware_remove,
+};
+module_platform_driver(rpi_firmware_driver);
+
+MODULE_AUTHOR("Eric Anholt <eric@anholt.net>");
+MODULE_DESCRIPTION("Raspberry Pi firmware driver");
+MODULE_LICENSE("GPL v2");
diff --git a/include/soc/bcm2835/raspberrypi-firmware-property.h b/include/soc/bcm2835/raspberrypi-firmware-property.h
new file mode 100644
index 0000000..b007e92
--- /dev/null
+++ b/include/soc/bcm2835/raspberrypi-firmware-property.h
@@ -0,0 +1,112 @@
+/*
+ *  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/types.h>
+#include <linux/of_device.h>
+
+enum rpi_firmware_property_status {
+	RPI_FIRMWARE_STATUS_REQUEST = 0,
+	RPI_FIRMWARE_STATUS_SUCCESS = 0x80000000,
+	RPI_FIRMWARE_STATUS_ERROR =   0x80000001,
+};
+
+/**
+ * struct rpi_firmware_property_tag_header - Firmware property tag header
+ * @tag:		One of enum_mbox_property_tag.
+ * @buf_size:		The number of bytes in the value buffer following this
+ *			struct.
+ * @req_resp_size:	On submit, the length of the request (though it doesn't
+ *			appear to be currently used by the firmware).  On return,
+ *			the length of the response (always 4 byte aligned), with
+ *			the low bit set.
+ */
+struct rpi_firmware_property_tag_header {
+	u32 tag;
+	u32 buf_size;
+	u32 req_resp_size;
+};
+
+enum rpi_firmware_property_tag {
+	RPI_FIRMWARE_PROPERTY_END =                           0,
+	RPI_FIRMWARE_GET_FIRMWARE_REVISION =                  0x00000001,
+
+	RPI_FIRMWARE_SET_CURSOR_INFO =                        0x00008010,
+	RPI_FIRMWARE_SET_CURSOR_STATE =                       0x00008011,
+
+	RPI_FIRMWARE_GET_BOARD_MODEL =                        0x00010001,
+	RPI_FIRMWARE_GET_BOARD_REVISION =                     0x00010002,
+	RPI_FIRMWARE_GET_BOARD_MAC_ADDRESS =                  0x00010003,
+	RPI_FIRMWARE_GET_BOARD_SERIAL =                       0x00010004,
+	RPI_FIRMWARE_GET_ARM_MEMORY =                         0x00010005,
+	RPI_FIRMWARE_GET_VC_MEMORY =                          0x00010006,
+	RPI_FIRMWARE_GET_CLOCKS =                             0x00010007,
+	RPI_FIRMWARE_GET_POWER_STATE =                        0x00020001,
+	RPI_FIRMWARE_GET_TIMING =                             0x00020002,
+	RPI_FIRMWARE_SET_POWER_STATE =                        0x00028001,
+	RPI_FIRMWARE_GET_CLOCK_STATE =                        0x00030001,
+	RPI_FIRMWARE_GET_CLOCK_RATE =                         0x00030002,
+	RPI_FIRMWARE_GET_VOLTAGE =                            0x00030003,
+	RPI_FIRMWARE_GET_MAX_CLOCK_RATE =                     0x00030004,
+	RPI_FIRMWARE_GET_MAX_VOLTAGE =                        0x00030005,
+	RPI_FIRMWARE_GET_TEMPERATURE =                        0x00030006,
+	RPI_FIRMWARE_GET_MIN_CLOCK_RATE =                     0x00030007,
+	RPI_FIRMWARE_GET_MIN_VOLTAGE =                        0x00030008,
+	RPI_FIRMWARE_GET_TURBO =                              0x00030009,
+	RPI_FIRMWARE_GET_MAX_TEMPERATURE =                    0x0003000a,
+	RPI_FIRMWARE_ALLOCATE_MEMORY =                        0x0003000c,
+	RPI_FIRMWARE_LOCK_MEMORY =                            0x0003000d,
+	RPI_FIRMWARE_UNLOCK_MEMORY =                          0x0003000e,
+	RPI_FIRMWARE_RELEASE_MEMORY =                         0x0003000f,
+	RPI_FIRMWARE_EXECUTE_CODE =                           0x00030010,
+	RPI_FIRMWARE_EXECUTE_QPU =                            0x00030011,
+	RPI_FIRMWARE_SET_ENABLE_QPU =                         0x00030012,
+	RPI_FIRMWARE_GET_DISPMANX_RESOURCE_MEM_HANDLE =       0x00030014,
+	RPI_FIRMWARE_GET_EDID_BLOCK =                         0x00030020,
+	RPI_FIRMWARE_SET_CLOCK_STATE =                        0x00038001,
+	RPI_FIRMWARE_SET_CLOCK_RATE =                         0x00038002,
+	RPI_FIRMWARE_SET_VOLTAGE =                            0x00038003,
+	RPI_FIRMWARE_SET_TURBO =                              0x00038009,
+
+	/* Dispmanx TAGS */
+	RPI_FIRMWARE_FRAMEBUFFER_ALLOCATE =                   0x00040001,
+	RPI_FIRMWARE_FRAMEBUFFER_BLANK =                      0x00040002,
+	RPI_FIRMWARE_FRAMEBUFFER_GET_PHYSICAL_WIDTH_HEIGHT =  0x00040003,
+	RPI_FIRMWARE_FRAMEBUFFER_GET_VIRTUAL_WIDTH_HEIGHT =   0x00040004,
+	RPI_FIRMWARE_FRAMEBUFFER_GET_DEPTH =                  0x00040005,
+	RPI_FIRMWARE_FRAMEBUFFER_GET_PIXEL_ORDER =            0x00040006,
+	RPI_FIRMWARE_FRAMEBUFFER_GET_ALPHA_MODE =             0x00040007,
+	RPI_FIRMWARE_FRAMEBUFFER_GET_PITCH =                  0x00040008,
+	RPI_FIRMWARE_FRAMEBUFFER_GET_VIRTUAL_OFFSET =         0x00040009,
+	RPI_FIRMWARE_FRAMEBUFFER_GET_OVERSCAN =               0x0004000a,
+	RPI_FIRMWARE_FRAMEBUFFER_GET_PALETTE =                0x0004000b,
+	RPI_FIRMWARE_FRAMEBUFFER_RELEASE =                    0x00048001,
+	RPI_FIRMWARE_FRAMEBUFFER_TEST_PHYSICAL_WIDTH_HEIGHT = 0x00044003,
+	RPI_FIRMWARE_FRAMEBUFFER_TEST_VIRTUAL_WIDTH_HEIGHT =  0x00044004,
+	RPI_FIRMWARE_FRAMEBUFFER_TEST_DEPTH =                 0x00044005,
+	RPI_FIRMWARE_FRAMEBUFFER_TEST_PIXEL_ORDER =           0x00044006,
+	RPI_FIRMWARE_FRAMEBUFFER_TEST_ALPHA_MODE =            0x00044007,
+	RPI_FIRMWARE_FRAMEBUFFER_TEST_VIRTUAL_OFFSET =        0x00044009,
+	RPI_FIRMWARE_FRAMEBUFFER_TEST_OVERSCAN =              0x0004400a,
+	RPI_FIRMWARE_FRAMEBUFFER_TEST_PALETTE =               0x0004400b,
+	RPI_FIRMWARE_FRAMEBUFFER_SET_PHYSICAL_WIDTH_HEIGHT =  0x00048003,
+	RPI_FIRMWARE_FRAMEBUFFER_SET_VIRTUAL_WIDTH_HEIGHT =   0x00048004,
+	RPI_FIRMWARE_FRAMEBUFFER_SET_DEPTH =                  0x00048005,
+	RPI_FIRMWARE_FRAMEBUFFER_SET_PIXEL_ORDER =            0x00048006,
+	RPI_FIRMWARE_FRAMEBUFFER_SET_ALPHA_MODE =             0x00048007,
+	RPI_FIRMWARE_FRAMEBUFFER_SET_VIRTUAL_OFFSET =         0x00048009,
+	RPI_FIRMWARE_FRAMEBUFFER_SET_OVERSCAN =               0x0004800a,
+	RPI_FIRMWARE_FRAMEBUFFER_SET_PALETTE =                0x0004800b,
+
+	RPI_FIRMWARE_GET_COMMAND_LINE =                       0x00050001,
+	RPI_FIRMWARE_GET_DMA_CHANNELS =                       0x00060001,
+};
+
+int rpi_firmware_property(struct device_node *of_node,
+			  u32 tag, void *data, size_t len);
+int rpi_firmware_property_list(struct device_node *of_node,
+			       void *data, size_t tag_size);
-- 
2.1.4

Re: [PATCH 2/3 v2] ARM: bcm2835: Add the Raspberry Pi firmware driver

[Lee Jones]

Few nits, nothing major.

> This gives us a function for making mailbox property channel requests
> of the firmware, which is most notable in that it will let us get and
> set clock rates.
> 
> Signed-off-by: Eric Anholt <eric@anholt.net>
> ---
> 
> v2: Drop power-domains stuff for now since we don't have the driver
>     core support to make it useful.  Move to drivers/firmware/.
>     Capitalize the enums.  De-global the firmware variable.  Use the
>     firmware device to allocate our DMA buffer, so that the dma-ranges
>     DT property gets respected.  Simplify the property tag transaction
>     interface even more, leaving a multi-tag interface still
>     available.  For conciseness, rename "raspberrypi" to "rpi" on all
>     functions/enums/structs, and the "firmware" variable to "fw".
>     Print when the driver is probed successfully, since debugging
>     -EPROBE_DEFER handling is such a big part of bcm2835 development.
>     Drop -EBUSY mailbox handling since the mailbox core has been fixed
>     to return -EPROBE_DEFER in -next.
> 
> Note that I don't think I've done what srwarren wanted for
> -EPROBE_DEFER, because I'm not clear what he wants.  I think he might
> just be asking for a function that does:
> 
> /*
>  * Returns 0 if the firmware device is probed and available, otherwise
>  * -EPROBE_DEFER.
>  */
> int rpi_firmware_get(struct device_node *firmware_node)
> {
> 	struct platform_device *pdev = of_find_device_by_node(of_node);
> 	if (!platform_get_drvdata(pdev))
> 		return -EPROBE_DEFER;
> 	return 0;
> }
> EXPORT_SYMBOL(rpi_firmware_get)
> 
> If that's all, I'm happy to add it.
> 
> Note that a client could currently do this:
> 
> 	ret = rpi_firmware_property_list(firmware_node, NULL, 0);
> 
> in exchange for a bit of overhead in the case that it's actually probed already.
> 
> 
>  drivers/firmware/Makefile                          |   1 +
>  drivers/firmware/raspberrypi.c                     | 224 +++++++++++++++++++++
>  .../soc/bcm2835/raspberrypi-firmware-property.h    | 114 +++++++++++
>  3 files changed, 339 insertions(+)
>  create mode 100644 drivers/firmware/raspberrypi.c
>  create mode 100644 include/soc/bcm2835/raspberrypi-firmware-property.h
> 
> diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
> index 3fdd391..41ced28 100644
> --- a/drivers/firmware/Makefile
> +++ b/drivers/firmware/Makefile
> @@ -13,6 +13,7 @@ obj-$(CONFIG_ISCSI_IBFT)	+= iscsi_ibft.o
>  obj-$(CONFIG_FIRMWARE_MEMMAP)	+= memmap.o
>  obj-$(CONFIG_QCOM_SCM)		+= qcom_scm.o
>  CFLAGS_qcom_scm.o :=$(call as-instr,.arch_extension sec,-DREQUIRES_SEC=1)
> +obj-$(CONFIG_BCM2835_MBOX)	+= raspberrypi.o
>  
>  obj-$(CONFIG_GOOGLE_FIRMWARE)	+= google/
>  obj-$(CONFIG_EFI)		+= efi/
> diff --git a/drivers/firmware/raspberrypi.c b/drivers/firmware/raspberrypi.c
> new file mode 100644
> index 0000000..61bde1b
> --- /dev/null
> +++ b/drivers/firmware/raspberrypi.c
> @@ -0,0 +1,224 @@
> +/*
> + *  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.
> + */
> +
> +/*
> + * Defines interfaces for interacting wtih the Raspberry Pi firmware's
> + * property channel.
> + */

No need to separate this from the header comment above.

> +#include <linux/dma-mapping.h>
> +#include <linux/mailbox_client.h>
> +#include <linux/module.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <soc/bcm2835/raspberrypi-firmware-property.h>
> +
> +#define MBOX_MSG(chan, data28)		(((data28) & ~0xf) | ((chan) & 0xf))
> +#define MBOX_CHAN(msg)			((msg) & 0xf)
> +#define MBOX_DATA28(msg)		((msg) & ~0xf)
> +#define MBOX_CHAN_PROPERTY		8
> +
> +struct rpi_firmware {
> +	struct mbox_client cl;
> +	struct mbox_chan *chan; /* The property channel. */
> +	struct completion c;
> +	u32 enabled;
> +};
> +
> +static DEFINE_MUTEX(transaction_lock);
> +
> +static void response_callback(struct mbox_client *cl, void *msg)
> +{
> +	struct rpi_firmware *fw = container_of(cl, struct rpi_firmware, cl);
> +	complete(&fw->c);
> +}
> +
> +/*
> + * Sends a request to the firmware through the BCM2835 mailbox driver,
> + * and synchronously waits for the reply.
> + */
> +static int
> +rpi_firmware_transaction(struct rpi_firmware *fw, u32 chan, u32 data)
> +{
> +	u32 message = MBOX_MSG(chan, data);
> +	int ret;
> +
> +	WARN_ON(data & 0xf);
> +
> +	mutex_lock(&transaction_lock);
> +	reinit_completion(&fw->c);
> +	ret = mbox_send_message(fw->chan, &message);
> +	if (ret >= 0) {
> +		wait_for_completion(&fw->c);
> +		ret = 0;
> +	} else {
> +		dev_err(fw->cl.dev, "mbox_send_message returned %d\n", ret);
> +	}
> +	mutex_unlock(&transaction_lock);
> +
> +	return ret;
> +}
> +
> +/*
> + * Submits a set of concatenated tags to the VPU firmware through the
> + * mailbox property interface.
> + *
> + * The buffer header and the ending tag are added by this function and
> + * don't need to be supplied, just the actual tags for your operation.
> + * See struct rpi_firmware_property_tag_header for the per-tag
> + * structure.
> + */
> +int rpi_firmware_property_list(struct device_node *of_node,
> +			       void *data, size_t tag_size)
> +{
> +	struct platform_device *pdev = of_find_device_by_node(of_node);
> +	struct rpi_firmware *fw = platform_get_drvdata(pdev);
> +	size_t size = tag_size + 12;
> +	u32 *buf;
> +	dma_addr_t bus_addr;
> +	int ret = 0;

No need to re-initialise.

> +	if (!fw)
> +		return -EPROBE_DEFER;
> +
> +	/* Packets are processed a dword at a time. */
> +	if (size & 3)
> +		return -EINVAL;
> +
> +	buf = dma_alloc_coherent(fw->cl.dev, PAGE_ALIGN(size), &bus_addr,
> +				 GFP_ATOMIC);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	/* The firmware will error out without parsing in this case. */
> +	WARN_ON(size >= 1024 * 1024);
> +
> +	buf[0] = size;
> +	buf[1] = RPI_FIRMWARE_STATUS_REQUEST;
> +	memcpy(&buf[2], data, tag_size);
> +	buf[size / 4 - 1] = RPI_FIRMWARE_PROPERTY_END;
> +	wmb();
> +
> +	ret = rpi_firmware_transaction(fw, MBOX_CHAN_PROPERTY, bus_addr);
> +
> +	rmb();
> +	memcpy(data, &buf[2], tag_size);
> +	if (ret == 0 && buf[1] != RPI_FIRMWARE_STATUS_SUCCESS) {
> +		/*
> +		 * The tag name here might not be the one causing the
> +		 * error, if there were multiple tags in the request.
> +		 * But single-tag is the most common, so go with it.
> +		 */
> +		dev_err(fw->cl.dev, "Request 0x%08x returned status 0x%08x\n",
> +			buf[2], buf[1]);
> +		ret = -EINVAL;
> +	}
> +
> +	dma_free_coherent(NULL, PAGE_ALIGN(size), buf, bus_addr);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(rpi_firmware_property_list);
> +
> +/*
> + * Submits a single tag to the VPU firmware through the mailbox
> + * property interface.
> + *
> + * This is a convenience wrapper around
> + * rpi_firmware_property_list() to avoid some of the
> + * boilerplate in property calls.
> + */
> +int rpi_firmware_property(struct device_node *of_node,
> +			  u32 tag, void *tag_data, size_t buf_size)
> +{
> +	/* Single tags are very small (generally 8 bytes), so the
> +	 * stack should be safe.
> +	 */
> +	u8 data[buf_size + sizeof(struct rpi_firmware_property_tag_header)];
> +	struct rpi_firmware_property_tag_header *header =
> +		(struct rpi_firmware_property_tag_header *)data;
> +	int ret;
> +
> +	header->tag = tag;
> +	header->buf_size = buf_size;
> +	header->req_resp_size = 0;
> +	memcpy(data + sizeof(struct rpi_firmware_property_tag_header),
> +	       tag_data, buf_size);
> +
> +	ret = rpi_firmware_property_list(of_node, &data, sizeof(data));
> +	memcpy(tag_data,
> +	       data + sizeof(struct rpi_firmware_property_tag_header),
> +	       buf_size);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(rpi_firmware_property);
> +
> +static int rpi_firmware_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	int ret = 0;
> +	struct rpi_firmware *fw;
> +
> +	fw = devm_kzalloc(dev, sizeof(*fw), GFP_KERNEL);
> +	if (!fw)
> +		return -ENOMEM;
> +
> +	fw->cl.dev = dev;
> +	fw->cl.rx_callback = response_callback;
> +	fw->cl.tx_block = true;
> +
> +	fw->chan = mbox_request_channel(&fw->cl, 0);
> +	if (IS_ERR(fw->chan)) {
> +		ret = PTR_ERR(fw->chan);
> +		/* An -EBUSY from the core means it couldn't find our
> +		 * channel, because the mailbox driver hadn't
> +		 * registered yet.
> +		 */
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(dev, "Failed to get mbox channel: %d\n", ret);
> +		return ret;
> +	}
> +
> +	init_completion(&fw->c);
> +
> +	dev_info(dev, "Firmware driver\n");

We don't normally print unless data (such as version/revision number)
has been acquired from the h/w.

> +	platform_set_drvdata(pdev, fw);
> +
> +	return 0;
> +}
> +
> +static int rpi_firmware_remove(struct platform_device *pdev)
> +{
> +	struct rpi_firmware *fw = platform_get_drvdata(pdev);
> +
> +	mbox_free_channel(fw->chan);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id rpi_firmware_of_match[] = {
> +	{ .compatible = "raspberrypi,firmware", },

"firmware" sounds very generic.  Is there any other information you
can use to make it more specific, in order to future-proof the
addition of new incarnations?  I'm thinking "bcm2835-firmware" for
instance.

> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, rpi_firmware_of_match);
> +
> +static struct platform_driver rpi_firmware_driver = {
> +	.driver = {
> +		.name = "raspberrypi-firmware",
> +		.owner = THIS_MODULE,

Remove this, it's taken care of elsewhere.

> +		.of_match_table = rpi_firmware_of_match,
> +	},
> +	.probe		= rpi_firmware_probe,
> +	.remove		= rpi_firmware_remove,
> +};
> +module_platform_driver(rpi_firmware_driver);
> +
> +MODULE_AUTHOR("Eric Anholt <eric@anholt.net>");
> +MODULE_DESCRIPTION("Raspberry Pi firmware driver");
> +MODULE_LICENSE("GPL v2");

[PATCH 2/3 v4] ARM: bcm2835: Add the Raspberry Pi firmware driver

[Eric Anholt]

This gives us a function for making mailbox property channel requests
of the firmware, which is most notable in that it will let us get and
set clock rates.

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

v2: Drop power-domains stuff for now since we don't have the driver
    core support to make it useful.  Move to drivers/firmware/.
    Capitalize the enums.  De-global the firmware variable.  Use the
    firmware device to allocate our DMA buffer, so that the dma-ranges
    DT property gets respected.  Simplify the property tag transaction
    interface even more, leaving a multi-tag interface still
    available.  For conciseness, rename "raspberrypi" to "rpi" on all
    functions/enums/structs, and the "firmware" variable to "fw".
    Print when the driver is probed successfully, since debugging
    -EPROBE_DEFER handling is such a big part of bcm2835 development.
    Drop -EBUSY mailbox handling since the mailbox core has been fixed
    to return -EPROBE_DEFER in -next.

v3: Use kernel-doc style for big comments (from Noralf), drop stale
    comment, use "dev" when freeing DMA as well.

v4: Move description comment into copyright comment, drop a dead
    initialization of "ret", and print the firmware revision at probe
    time.

 drivers/firmware/Makefile                          |   1 +
 drivers/firmware/raspberrypi.c                     | 248 +++++++++++++++++++++
 .../soc/bcm2835/raspberrypi-firmware-property.h    | 112 ++++++++++
 3 files changed, 361 insertions(+)
 create mode 100644 drivers/firmware/raspberrypi.c
 create mode 100644 include/soc/bcm2835/raspberrypi-firmware-property.h

diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
index 3fdd391..41ced28 100644
--- a/drivers/firmware/Makefile
+++ b/drivers/firmware/Makefile
@@ -13,6 +13,7 @@ obj-$(CONFIG_ISCSI_IBFT)	+= iscsi_ibft.o
 obj-$(CONFIG_FIRMWARE_MEMMAP)	+= memmap.o
 obj-$(CONFIG_QCOM_SCM)		+= qcom_scm.o
 CFLAGS_qcom_scm.o :=$(call as-instr,.arch_extension sec,-DREQUIRES_SEC=1)
+obj-$(CONFIG_BCM2835_MBOX)	+= raspberrypi.o
 
 obj-$(CONFIG_GOOGLE_FIRMWARE)	+= google/
 obj-$(CONFIG_EFI)		+= efi/
diff --git a/drivers/firmware/raspberrypi.c b/drivers/firmware/raspberrypi.c
new file mode 100644
index 0000000..e121467
--- /dev/null
+++ b/drivers/firmware/raspberrypi.c
@@ -0,0 +1,248 @@
+/*
+ * Defines interfaces for interacting wtih the Raspberry Pi firmware's
+ * property channel.
+ *
+ * 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/dma-mapping.h>
+#include <linux/mailbox_client.h>
+#include <linux/module.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <soc/bcm2835/raspberrypi-firmware-property.h>
+
+#define MBOX_MSG(chan, data28)		(((data28) & ~0xf) | ((chan) & 0xf))
+#define MBOX_CHAN(msg)			((msg) & 0xf)
+#define MBOX_DATA28(msg)		((msg) & ~0xf)
+#define MBOX_CHAN_PROPERTY		8
+
+struct rpi_firmware {
+	struct mbox_client cl;
+	struct mbox_chan *chan; /* The property channel. */
+	struct completion c;
+	u32 enabled;
+};
+
+static DEFINE_MUTEX(transaction_lock);
+
+static void response_callback(struct mbox_client *cl, void *msg)
+{
+	struct rpi_firmware *fw = container_of(cl, struct rpi_firmware, cl);
+	complete(&fw->c);
+}
+
+/*
+ * Sends a request to the firmware through the BCM2835 mailbox driver,
+ * and synchronously waits for the reply.
+ */
+static int
+rpi_firmware_transaction(struct rpi_firmware *fw, u32 chan, u32 data)
+{
+	u32 message = MBOX_MSG(chan, data);
+	int ret;
+
+	WARN_ON(data & 0xf);
+
+	mutex_lock(&transaction_lock);
+	reinit_completion(&fw->c);
+	ret = mbox_send_message(fw->chan, &message);
+	if (ret >= 0) {
+		wait_for_completion(&fw->c);
+		ret = 0;
+	} else {
+		dev_err(fw->cl.dev, "mbox_send_message returned %d\n", ret);
+	}
+	mutex_unlock(&transaction_lock);
+
+	return ret;
+}
+
+/**
+ * rpi_firmware_property_list - Submit firmware property list
+ * @of_node:	Pointer to the firmware Device Tree node.
+ * @data:	Buffer holding tags.
+ * @tag_size:	Size of tags buffer.
+ *
+ * Submits a set of concatenated tags to the VPU firmware through the
+ * mailbox property interface.
+ *
+ * The buffer header and the ending tag are added by this function and
+ * don't need to be supplied, just the actual tags for your operation.
+ * See struct rpi_firmware_property_tag_header for the per-tag
+ * structure.
+ */
+int rpi_firmware_property_list(struct device_node *of_node,
+			       void *data, size_t tag_size)
+{
+	struct platform_device *pdev = of_find_device_by_node(of_node);
+	struct rpi_firmware *fw = platform_get_drvdata(pdev);
+	size_t size = tag_size + 12;
+	u32 *buf;
+	dma_addr_t bus_addr;
+	int ret;
+
+	if (!fw)
+		return -EPROBE_DEFER;
+
+	/* Packets are processed a dword at a time. */
+	if (size & 3)
+		return -EINVAL;
+
+	buf = dma_alloc_coherent(fw->cl.dev, PAGE_ALIGN(size), &bus_addr,
+				 GFP_ATOMIC);
+	if (!buf)
+		return -ENOMEM;
+
+	/* The firmware will error out without parsing in this case. */
+	WARN_ON(size >= 1024 * 1024);
+
+	buf[0] = size;
+	buf[1] = RPI_FIRMWARE_STATUS_REQUEST;
+	memcpy(&buf[2], data, tag_size);
+	buf[size / 4 - 1] = RPI_FIRMWARE_PROPERTY_END;
+	wmb();
+
+	ret = rpi_firmware_transaction(fw, MBOX_CHAN_PROPERTY, bus_addr);
+
+	rmb();
+	memcpy(data, &buf[2], tag_size);
+	if (ret == 0 && buf[1] != RPI_FIRMWARE_STATUS_SUCCESS) {
+		/*
+		 * The tag name here might not be the one causing the
+		 * error, if there were multiple tags in the request.
+		 * But single-tag is the most common, so go with it.
+		 */
+		dev_err(fw->cl.dev, "Request 0x%08x returned status 0x%08x\n",
+			buf[2], buf[1]);
+		ret = -EINVAL;
+	}
+
+	dma_free_coherent(fw->cl.dev, PAGE_ALIGN(size), buf, bus_addr);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(rpi_firmware_property_list);
+
+/**
+ * rpi_firmware_property - Submit single firmware property
+ * @of_node:	Pointer to the firmware Device Tree node.
+ * @tag:	One of enum_mbox_property_tag.
+ * @tag_data:	Tag data buffer.
+ * @buf_size:	Buffer size.
+ *
+ * Submits a single tag to the VPU firmware through the mailbox
+ * property interface.
+ *
+ * This is a convenience wrapper around
+ * rpi_firmware_property_list() to avoid some of the
+ * boilerplate in property calls.
+ */
+int rpi_firmware_property(struct device_node *of_node,
+			  u32 tag, void *tag_data, size_t buf_size)
+{
+	/* Single tags are very small (generally 8 bytes), so the
+	 * stack should be safe.
+	 */
+	u8 data[buf_size + sizeof(struct rpi_firmware_property_tag_header)];
+	struct rpi_firmware_property_tag_header *header =
+		(struct rpi_firmware_property_tag_header *)data;
+	int ret;
+
+	header->tag = tag;
+	header->buf_size = buf_size;
+	header->req_resp_size = 0;
+	memcpy(data + sizeof(struct rpi_firmware_property_tag_header),
+	       tag_data, buf_size);
+
+	ret = rpi_firmware_property_list(of_node, &data, sizeof(data));
+	memcpy(tag_data,
+	       data + sizeof(struct rpi_firmware_property_tag_header),
+	       buf_size);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(rpi_firmware_property);
+
+static void
+rpi_firmware_print_firmware_revision(struct device *dev)
+{
+	u32 packet;
+	int ret = rpi_firmware_property(dev->of_node,
+					RPI_FIRMWARE_GET_FIRMWARE_REVISION,
+					&packet, sizeof(packet));
+
+	if (ret == 0) {
+		struct tm tm;
+
+		time_to_tm(packet, 0, &tm);
+
+		dev_info(dev,
+			 "Attached to firmware from %04ld-%02d-%02d %02d:%02d\n",
+			 tm.tm_year + 1900, tm.tm_mon + 1, tm.tm_mday,
+			 tm.tm_hour, tm.tm_min);
+	}
+}
+
+static int rpi_firmware_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct rpi_firmware *fw;
+
+	fw = devm_kzalloc(dev, sizeof(*fw), GFP_KERNEL);
+	if (!fw)
+		return -ENOMEM;
+
+	fw->cl.dev = dev;
+	fw->cl.rx_callback = response_callback;
+	fw->cl.tx_block = true;
+
+	fw->chan = mbox_request_channel(&fw->cl, 0);
+	if (IS_ERR(fw->chan)) {
+		int ret = PTR_ERR(fw->chan);
+		if (ret != -EPROBE_DEFER)
+			dev_err(dev, "Failed to get mbox channel: %d\n", ret);
+		return ret;
+	}
+
+	init_completion(&fw->c);
+
+	platform_set_drvdata(pdev, fw);
+
+	rpi_firmware_print_firmware_revision(dev);
+
+	return 0;
+}
+
+static int rpi_firmware_remove(struct platform_device *pdev)
+{
+	struct rpi_firmware *fw = platform_get_drvdata(pdev);
+
+	mbox_free_channel(fw->chan);
+
+	return 0;
+}
+
+static const struct of_device_id rpi_firmware_of_match[] = {
+	{ .compatible = "raspberrypi,firmware", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, rpi_firmware_of_match);
+
+static struct platform_driver rpi_firmware_driver = {
+	.driver = {
+		.name = "raspberrypi-firmware",
+		.of_match_table = rpi_firmware_of_match,
+	},
+	.probe		= rpi_firmware_probe,
+	.remove		= rpi_firmware_remove,
+};
+module_platform_driver(rpi_firmware_driver);
+
+MODULE_AUTHOR("Eric Anholt <eric@anholt.net>");
+MODULE_DESCRIPTION("Raspberry Pi firmware driver");
+MODULE_LICENSE("GPL v2");
diff --git a/include/soc/bcm2835/raspberrypi-firmware-property.h b/include/soc/bcm2835/raspberrypi-firmware-property.h
new file mode 100644
index 0000000..b007e92
--- /dev/null
+++ b/include/soc/bcm2835/raspberrypi-firmware-property.h
@@ -0,0 +1,112 @@
+/*
+ *  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/types.h>
+#include <linux/of_device.h>
+
+enum rpi_firmware_property_status {
+	RPI_FIRMWARE_STATUS_REQUEST = 0,
+	RPI_FIRMWARE_STATUS_SUCCESS = 0x80000000,
+	RPI_FIRMWARE_STATUS_ERROR =   0x80000001,
+};
+
+/**
+ * struct rpi_firmware_property_tag_header - Firmware property tag header
+ * @tag:		One of enum_mbox_property_tag.
+ * @buf_size:		The number of bytes in the value buffer following this
+ *			struct.
+ * @req_resp_size:	On submit, the length of the request (though it doesn't
+ *			appear to be currently used by the firmware).  On return,
+ *			the length of the response (always 4 byte aligned), with
+ *			the low bit set.
+ */
+struct rpi_firmware_property_tag_header {
+	u32 tag;
+	u32 buf_size;
+	u32 req_resp_size;
+};
+
+enum rpi_firmware_property_tag {
+	RPI_FIRMWARE_PROPERTY_END =                           0,
+	RPI_FIRMWARE_GET_FIRMWARE_REVISION =                  0x00000001,
+
+	RPI_FIRMWARE_SET_CURSOR_INFO =                        0x00008010,
+	RPI_FIRMWARE_SET_CURSOR_STATE =                       0x00008011,
+
+	RPI_FIRMWARE_GET_BOARD_MODEL =                        0x00010001,
+	RPI_FIRMWARE_GET_BOARD_REVISION =                     0x00010002,
+	RPI_FIRMWARE_GET_BOARD_MAC_ADDRESS =                  0x00010003,
+	RPI_FIRMWARE_GET_BOARD_SERIAL =                       0x00010004,
+	RPI_FIRMWARE_GET_ARM_MEMORY =                         0x00010005,
+	RPI_FIRMWARE_GET_VC_MEMORY =                          0x00010006,
+	RPI_FIRMWARE_GET_CLOCKS =                             0x00010007,
+	RPI_FIRMWARE_GET_POWER_STATE =                        0x00020001,
+	RPI_FIRMWARE_GET_TIMING =                             0x00020002,
+	RPI_FIRMWARE_SET_POWER_STATE =                        0x00028001,
+	RPI_FIRMWARE_GET_CLOCK_STATE =                        0x00030001,
+	RPI_FIRMWARE_GET_CLOCK_RATE =                         0x00030002,
+	RPI_FIRMWARE_GET_VOLTAGE =                            0x00030003,
+	RPI_FIRMWARE_GET_MAX_CLOCK_RATE =                     0x00030004,
+	RPI_FIRMWARE_GET_MAX_VOLTAGE =                        0x00030005,
+	RPI_FIRMWARE_GET_TEMPERATURE =                        0x00030006,
+	RPI_FIRMWARE_GET_MIN_CLOCK_RATE =                     0x00030007,
+	RPI_FIRMWARE_GET_MIN_VOLTAGE =                        0x00030008,
+	RPI_FIRMWARE_GET_TURBO =                              0x00030009,
+	RPI_FIRMWARE_GET_MAX_TEMPERATURE =                    0x0003000a,
+	RPI_FIRMWARE_ALLOCATE_MEMORY =                        0x0003000c,
+	RPI_FIRMWARE_LOCK_MEMORY =                            0x0003000d,
+	RPI_FIRMWARE_UNLOCK_MEMORY =                          0x0003000e,
+	RPI_FIRMWARE_RELEASE_MEMORY =                         0x0003000f,
+	RPI_FIRMWARE_EXECUTE_CODE =                           0x00030010,
+	RPI_FIRMWARE_EXECUTE_QPU =                            0x00030011,
+	RPI_FIRMWARE_SET_ENABLE_QPU =                         0x00030012,
+	RPI_FIRMWARE_GET_DISPMANX_RESOURCE_MEM_HANDLE =       0x00030014,
+	RPI_FIRMWARE_GET_EDID_BLOCK =                         0x00030020,
+	RPI_FIRMWARE_SET_CLOCK_STATE =                        0x00038001,
+	RPI_FIRMWARE_SET_CLOCK_RATE =                         0x00038002,
+	RPI_FIRMWARE_SET_VOLTAGE =                            0x00038003,
+	RPI_FIRMWARE_SET_TURBO =                              0x00038009,
+
+	/* Dispmanx TAGS */
+	RPI_FIRMWARE_FRAMEBUFFER_ALLOCATE =                   0x00040001,
+	RPI_FIRMWARE_FRAMEBUFFER_BLANK =                      0x00040002,
+	RPI_FIRMWARE_FRAMEBUFFER_GET_PHYSICAL_WIDTH_HEIGHT =  0x00040003,
+	RPI_FIRMWARE_FRAMEBUFFER_GET_VIRTUAL_WIDTH_HEIGHT =   0x00040004,
+	RPI_FIRMWARE_FRAMEBUFFER_GET_DEPTH =                  0x00040005,
+	RPI_FIRMWARE_FRAMEBUFFER_GET_PIXEL_ORDER =            0x00040006,
+	RPI_FIRMWARE_FRAMEBUFFER_GET_ALPHA_MODE =             0x00040007,
+	RPI_FIRMWARE_FRAMEBUFFER_GET_PITCH =                  0x00040008,
+	RPI_FIRMWARE_FRAMEBUFFER_GET_VIRTUAL_OFFSET =         0x00040009,
+	RPI_FIRMWARE_FRAMEBUFFER_GET_OVERSCAN =               0x0004000a,
+	RPI_FIRMWARE_FRAMEBUFFER_GET_PALETTE =                0x0004000b,
+	RPI_FIRMWARE_FRAMEBUFFER_RELEASE =                    0x00048001,
+	RPI_FIRMWARE_FRAMEBUFFER_TEST_PHYSICAL_WIDTH_HEIGHT = 0x00044003,
+	RPI_FIRMWARE_FRAMEBUFFER_TEST_VIRTUAL_WIDTH_HEIGHT =  0x00044004,
+	RPI_FIRMWARE_FRAMEBUFFER_TEST_DEPTH =                 0x00044005,
+	RPI_FIRMWARE_FRAMEBUFFER_TEST_PIXEL_ORDER =           0x00044006,
+	RPI_FIRMWARE_FRAMEBUFFER_TEST_ALPHA_MODE =            0x00044007,
+	RPI_FIRMWARE_FRAMEBUFFER_TEST_VIRTUAL_OFFSET =        0x00044009,
+	RPI_FIRMWARE_FRAMEBUFFER_TEST_OVERSCAN =              0x0004400a,
+	RPI_FIRMWARE_FRAMEBUFFER_TEST_PALETTE =               0x0004400b,
+	RPI_FIRMWARE_FRAMEBUFFER_SET_PHYSICAL_WIDTH_HEIGHT =  0x00048003,
+	RPI_FIRMWARE_FRAMEBUFFER_SET_VIRTUAL_WIDTH_HEIGHT =   0x00048004,
+	RPI_FIRMWARE_FRAMEBUFFER_SET_DEPTH =                  0x00048005,
+	RPI_FIRMWARE_FRAMEBUFFER_SET_PIXEL_ORDER =            0x00048006,
+	RPI_FIRMWARE_FRAMEBUFFER_SET_ALPHA_MODE =             0x00048007,
+	RPI_FIRMWARE_FRAMEBUFFER_SET_VIRTUAL_OFFSET =         0x00048009,
+	RPI_FIRMWARE_FRAMEBUFFER_SET_OVERSCAN =               0x0004800a,
+	RPI_FIRMWARE_FRAMEBUFFER_SET_PALETTE =                0x0004800b,
+
+	RPI_FIRMWARE_GET_COMMAND_LINE =                       0x00050001,
+	RPI_FIRMWARE_GET_DMA_CHANNELS =                       0x00060001,
+};
+
+int rpi_firmware_property(struct device_node *of_node,
+			  u32 tag, void *data, size_t len);
+int rpi_firmware_property_list(struct device_node *of_node,
+			       void *data, size_t tag_size);
-- 
2.1.4

Re: [PATCH 1/3 v3] dt/bindings: Add binding for the Raspberry Pi firmware driver

[Stephen Warren]

On 05/18/2015 11:59 AM, Eric Anholt wrote:
> This driver will provide support for calls into the firmware that will
> be used by other drivers like cpufreq and vc4.

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

(although Lee's comment on patch 2/3 about the property name being
rather generic does ring true)

Re: [PATCH 2/3 v2] ARM: bcm2835: Add the Raspberry Pi firmware driver

[Stephen Warren]

On 05/13/2015 01:00 PM, Eric Anholt wrote:
> This gives us a function for making mailbox property channel requests
> of the firmware, which is most notable in that it will let us get and
> set clock rates.
...
> Note that I don't think I've done what srwarren wanted for
> -EPROBE_DEFER, because I'm not clear what he wants.  I think he might
> just be asking for a function that does:
> 
> /*
>  * Returns 0 if the firmware device is probed and available, otherwise
>  * -EPROBE_DEFER.
>  */
> int rpi_firmware_get(struct device_node *firmware_node)
> {
> 	struct platform_device *pdev = of_find_device_by_node(of_node);
> 	if (!platform_get_drvdata(pdev))
> 		return -EPROBE_DEFER;
> 	return 0;
> }
> EXPORT_SYMBOL(rpi_firmware_get)
> 
> If that's all, I'm happy to add it.

Yes, there definitely needs to be something that clients can call at
probe() time to make sure the firmware driver is there already. That
check is quite different from actually sending a request to the FW, so
I'd certainly expect a separate function for that.

Re: [PATCH 2/3 v4] ARM: bcm2835: Add the Raspberry Pi firmware driver

[Stephen Warren]

On 05/28/2015 12:33 PM, Eric Anholt wrote:
> This gives us a function for making mailbox property channel requests
> of the firmware, which is most notable in that it will let us get and
> set clock rates.

This thread was rather hard to follow since updated versions of patches
were sent "in response" to the original posting rather than as a new
thread. Generally it's best to post a complete copy of each new version
of the series, and as a completely standalone thread.

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

> +obj-$(CONFIG_BCM2835_MBOX)	+= raspberrypi.o

I think this warrants its own Kconfig option that depends on _MBOX.

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

> +int rpi_firmware_property_list(struct device_node *of_node,
> +			       void *data, size_t tag_size)
> +{
> +	struct platform_device *pdev = of_find_device_by_node(of_node);

I would expect the of_node -> pdev mapping to happen at client device
probe time. Simplest for this driver would be if the
client-probe-time-mapping function returned the "struct rpi_firmware"
and the client passed that to this function.

> +	struct rpi_firmware *fw = platform_get_drvdata(pdev);
> +	size_t size = tag_size + 12;
> +	u32 *buf;
> +	dma_addr_t bus_addr;
> +	int ret;
> +
> +	if (!fw)
> +		return -EPROBE_DEFER;

That return code should only be used in functions called at probe time.
While I do see the expectation is that clients call this function once
at probe time and hence guarantee to see -EPROBE_DEFER at probe time (if
at all), it's rather odd for a function that's intended to be called at
times other that probe() to ever have the possibility of returning
-EPROBE_DEFER. If somehow that values gets returned at another time,
it's going to be rather confusing to the client.

> +	buf[0] = size;
> +	buf[1] = RPI_FIRMWARE_STATUS_REQUEST;
> +	memcpy(&buf[2], data, tag_size);
> +	buf[size / 4 - 1] = RPI_FIRMWARE_PROPERTY_END;

Out of curiosity, why not require the client to format the entire
message itself? In theory at least, the client could submit a whole
"string" of tags at once, so it really seems like the client should be
constructing the whole message.

Still, this is something that's easy to modify later without affecting
much (and in particular, has zero impact on DT), so feel free to ignore
this for now.

...
> +	if (ret == 0 && buf[1] != RPI_FIRMWARE_STATUS_SUCCESS) {
> +		/*
> +		 * The tag name here might not be the one causing the
> +		 * error, if there were multiple tags in the request.
> +		 * But single-tag is the most common, so go with it.
> +		 */
> +		dev_err(fw->cl.dev, "Request 0x%08x returned status 0x%08x\n",
> +			buf[2], buf[1]);
> +		ret = -EINVAL;
> +	}

If this driver were solely a transport, and required the client driver
to construct the whole message and parse the whole response, the gotcha
that's documented above would be solved; the client would know exactly
which set of tags to look at for errors.

Alternatively, iterating over all the tags is quite simple, since each
one has a size field and there's a sentinel tag at the end. That
enhancement could also be added later though.

> +rpi_firmware_print_firmware_revision(struct device *dev)
> +{
> +	u32 packet;
> +	int ret = rpi_firmware_property(dev->of_node,
> +					RPI_FIRMWARE_GET_FIRMWARE_REVISION,
> +					&packet, sizeof(packet));
...
> +		time_to_tm(packet, 0, &tm);

Oh, the revision value is a time? Is so, it'd be nice if the
documentation could be updated to state this:

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

Re: [PATCH 3/3 v2] ARM: bcm2835: Add the firmware driver information to the RPi DT

[Stephen Warren]

On 05/13/2015 01:00 PM, Eric Anholt wrote:
> Signed-off-by: Eric Anholt <eric@anholt.net>

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

Re: [PATCH 2/3 v4] ARM: bcm2835: Add the Raspberry Pi firmware driver

[Noralf Trønnes]

Den 28.05.2015 23:28, skrev Stephen Warren:
> On 05/28/2015 12:33 PM, Eric Anholt wrote:
>> This gives us a function for making mailbox property channel requests
>> of the firmware, which is most notable in that it will let us get and
>> set clock rates.
...
>> +int rpi_firmware_property_list(struct device_node *of_node,
>> +			       void *data, size_t tag_size)
>> +{
>> +	struct platform_device *pdev = of_find_device_by_node(of_node);
> I would expect the of_node -> pdev mapping to happen at client device
> probe time. Simplest for this driver would be if the
> client-probe-time-mapping function returned the "struct rpi_firmware"
> and the client passed that to this function.

What if the firmware driver/device is removed or reloaded?
In that case the client has an invalid pointer. It wouldn't know about 
the change.

Re: [PATCH 2/3 v4] ARM: bcm2835: Add the Raspberry Pi firmware driver

[Noralf Trønnes]

Den 28.05.2015 20:33, skrev Eric Anholt:
> This gives us a function for making mailbox property channel requests
> of the firmware, which is most notable in that it will let us get and
> set clock rates.
>
> Signed-off-by: Eric Anholt <eric@anholt.net>
...
> +static int rpi_firmware_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct rpi_firmware *fw;
> +
> +	fw = devm_kzalloc(dev, sizeof(*fw), GFP_KERNEL);
> +	if (!fw)
> +		return -ENOMEM;
> +
> +	fw->cl.dev = dev;
> +	fw->cl.rx_callback = response_callback;
> +	fw->cl.tx_block = true;
> +
> +	fw->chan = mbox_request_channel(&fw->cl, 0);
> +	if (IS_ERR(fw->chan)) {
> +		int ret = PTR_ERR(fw->chan);
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(dev, "Failed to get mbox channel: %d\n", ret);
> +		return ret;
> +	}
> +
> +	init_completion(&fw->c);
> +
> +	platform_set_drvdata(pdev, fw);
> +
> +	rpi_firmware_print_firmware_revision(dev);
> +
> +	return 0;
> +}
> +
> +static int rpi_firmware_remove(struct platform_device *pdev)
> +{
> +	struct rpi_firmware *fw = platform_get_drvdata(pdev);
> +
> +	mbox_free_channel(fw->chan);

I guess driver data has to be reset here:
platform_set_drvdata(pdev, NULL);

> +
> +	return 0;
> +}

Re: [PATCH 2/3 v4] ARM: bcm2835: Add the Raspberry Pi firmware driver

[Stephen Warren]

On 05/28/2015 03:39 PM, Noralf Trønnes wrote:
> 
> Den 28.05.2015 23:28, skrev Stephen Warren:
>> On 05/28/2015 12:33 PM, Eric Anholt wrote:
>>> This gives us a function for making mailbox property channel requests
>>> of the firmware, which is most notable in that it will let us get and
>>> set clock rates.
> ...
>>> +int rpi_firmware_property_list(struct device_node *of_node,
>>> +                   void *data, size_t tag_size)
>>> +{
>>> +    struct platform_device *pdev = of_find_device_by_node(of_node);
>> I would expect the of_node -> pdev mapping to happen at client device
>> probe time. Simplest for this driver would be if the
>> client-probe-time-mapping function returned the "struct rpi_firmware"
>> and the client passed that to this function.
> 
> What if the firmware driver/device is removed or reloaded?
> In that case the client has an invalid pointer. It wouldn't know about
> the change.

A provider can't be removed if it has clients. So, you'd have to remove
all clients first, then remove the provider. This may require adding
some code to increment/decrement the module use count when clients look
up the provider and are themselves removed.

Re: [PATCH 2/3 v4] ARM: bcm2835: Add the Raspberry Pi firmware driver

[Stephen Warren]

On 05/28/2015 03:42 PM, Noralf Trønnes wrote:
> 
> Den 28.05.2015 20:33, skrev Eric Anholt:
>> This gives us a function for making mailbox property channel requests
>> of the firmware, which is most notable in that it will let us get and
>> set clock rates.

>> +static int rpi_firmware_remove(struct platform_device *pdev)
>> +{
>> +    struct rpi_firmware *fw = platform_get_drvdata(pdev);
>> +
>> +    mbox_free_channel(fw->chan);
> 
> I guess driver data has to be reset here:
> platform_set_drvdata(pdev, NULL);

The value of the drvdata is irrelevant before the device has probed, or
after it has been removed. Hence, drivers should not manually clear
drvdata. (Many drivers used to, but that code is being purged out).

Re: [PATCH 2/3 v4] ARM: bcm2835: Add the Raspberry Pi firmware driver

[Eric Anholt]

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

Stephen Warren <swarren@wwwdotorg.org> writes:

> On 05/28/2015 12:33 PM, Eric Anholt wrote:
>> This gives us a function for making mailbox property channel requests
>> of the firmware, which is most notable in that it will let us get and
>> set clock rates.
>
> This thread was rather hard to follow since updated versions of patches
> were sent "in response" to the original posting rather than as a new
> thread. Generally it's best to post a complete copy of each new version
> of the series, and as a completely standalone thread.

Will do.

>> diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
>
>> +obj-$(CONFIG_BCM2835_MBOX)	+= raspberrypi.o
>
> I think this warrants its own Kconfig option that depends on _MBOX.

Done, called it RASPBERRPI_FIRMWARE

>> diff --git a/drivers/firmware/raspberrypi.c b/drivers/firmware/raspberrypi.c
>
>> +int rpi_firmware_property_list(struct device_node *of_node,
>> +			       void *data, size_t tag_size)
>> +{
>> +	struct platform_device *pdev = of_find_device_by_node(of_node);
>
> I would expect the of_node -> pdev mapping to happen at client device
> probe time. Simplest for this driver would be if the
> client-probe-time-mapping function returned the "struct rpi_firmware"
> and the client passed that to this function.

Done.

>> +	struct rpi_firmware *fw = platform_get_drvdata(pdev);
>> +	size_t size = tag_size + 12;
>> +	u32 *buf;
>> +	dma_addr_t bus_addr;
>> +	int ret;
>> +
>> +	if (!fw)
>> +		return -EPROBE_DEFER;
>
> That return code should only be used in functions called at probe time.
> While I do see the expectation is that clients call this function once
> at probe time and hence guarantee to see -EPROBE_DEFER at probe time (if
> at all), it's rather odd for a function that's intended to be called at
> times other that probe() to ever have the possibility of returning
> -EPROBE_DEFER. If somehow that values gets returned at another time,
> it's going to be rather confusing to the client.

Dropped now that I'm passing fw in.

>> +	buf[0] = size;
>> +	buf[1] = RPI_FIRMWARE_STATUS_REQUEST;
>> +	memcpy(&buf[2], data, tag_size);
>> +	buf[size / 4 - 1] = RPI_FIRMWARE_PROPERTY_END;
>
> Out of curiosity, why not require the client to format the entire
> message itself? In theory at least, the client could submit a whole
> "string" of tags at once, so it really seems like the client should be
> constructing the whole message.

Because it's something extra for the client to screw up?  I mean, if we
were going for super high performance, we'd have the client asking us
for a DMA buffer to write into, and they'd write the whole thing, and
submit that back to us.  But we don't need that, because the property
channel is used so rarely.

I briefly used multi-tag in a not-for-upstream KMS-using-the-firmware
branch, but it turned out the FB MBOX channel was strictly better (once
I worked around a fw bug).

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

Re: [PATCH 2/3 v4] ARM: bcm2835: Add the Raspberry Pi firmware driver

[Eric Anholt]

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

Noralf Trønnes <noralf@tronnes.org> writes:

> Den 28.05.2015 20:33, skrev Eric Anholt:
>> This gives us a function for making mailbox property channel requests
>> of the firmware, which is most notable in that it will let us get and
>> set clock rates.
>>
>> Signed-off-by: Eric Anholt <eric@anholt.net>
> ...
>> +static int rpi_firmware_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct rpi_firmware *fw;
>> +
>> +	fw = devm_kzalloc(dev, sizeof(*fw), GFP_KERNEL);
>> +	if (!fw)
>> +		return -ENOMEM;
>> +
>> +	fw->cl.dev = dev;
>> +	fw->cl.rx_callback = response_callback;
>> +	fw->cl.tx_block = true;
>> +
>> +	fw->chan = mbox_request_channel(&fw->cl, 0);
>> +	if (IS_ERR(fw->chan)) {
>> +		int ret = PTR_ERR(fw->chan);
>> +		if (ret != -EPROBE_DEFER)
>> +			dev_err(dev, "Failed to get mbox channel: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	init_completion(&fw->c);
>> +
>> +	platform_set_drvdata(pdev, fw);
>> +
>> +	rpi_firmware_print_firmware_revision(dev);
>> +
>> +	return 0;
>> +}
>> +
>> +static int rpi_firmware_remove(struct platform_device *pdev)
>> +{
>> +	struct rpi_firmware *fw = platform_get_drvdata(pdev);
>> +
>> +	mbox_free_channel(fw->chan);
>
> I guess driver data has to be reset here:
> platform_set_drvdata(pdev, NULL);

Fixed.

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

Re: [PATCH 2/3 v2] ARM: bcm2835: Add the Raspberry Pi firmware driver

[Noralf Trønnes]

Den 28.05.2015 23:17, skrev Stephen Warren:
> On 05/13/2015 01:00 PM, Eric Anholt wrote:
>> This gives us a function for making mailbox property channel requests
>> of the firmware, which is most notable in that it will let us get and
>> set clock rates.
> ...
>> Note that I don't think I've done what srwarren wanted for
>> -EPROBE_DEFER, because I'm not clear what he wants.  I think he might
>> just be asking for a function that does:
>>
>> /*
>>   * Returns 0 if the firmware device is probed and available, otherwise
>>   * -EPROBE_DEFER.
>>   */
>> int rpi_firmware_get(struct device_node *firmware_node)
>> {
>> 	struct platform_device *pdev = of_find_device_by_node(of_node);
>> 	if (!platform_get_drvdata(pdev))

of_find_device_by_node() can return NULL if the device can't be found.
platform_get_drvdata() can't handle a NULL pointer.

>> 		return -EPROBE_DEFER;
>> 	return 0;
>> }
>> EXPORT_SYMBOL(rpi_firmware_get)
>>
>> If that's all, I'm happy to add it.
> Yes, there definitely needs to be something that clients can call at
> probe() time to make sure the firmware driver is there already. That
> check is quite different from actually sending a request to the FW, so
> I'd certainly expect a separate function for that.

A try_module_get() here would make sure this module won't go away while a
client relies on it. For that we would need a rpi_firmware_put() as well.

Re: [PATCH 2/3 v4] ARM: bcm2835: Add the Raspberry Pi firmware driver

[Noralf Trønnes]

Den 29.05.2015 00:38, skrev Eric Anholt:
> Noralf Trønnes <noralf@tronnes.org> writes:
>
>> Den 28.05.2015 20:33, skrev Eric Anholt:
>>> This gives us a function for making mailbox property channel requests
>>> of the firmware, which is most notable in that it will let us get and
>>> set clock rates.
>>>
>>> Signed-off-by: Eric Anholt <eric@anholt.net>
>> ...
>>> +static int rpi_firmware_probe(struct platform_device *pdev)
>>> +{
>>> +	struct device *dev = &pdev->dev;
>>> +	struct rpi_firmware *fw;
>>> +
>>> +	fw = devm_kzalloc(dev, sizeof(*fw), GFP_KERNEL);
>>> +	if (!fw)
>>> +		return -ENOMEM;
>>> +
>>> +	fw->cl.dev = dev;
>>> +	fw->cl.rx_callback = response_callback;
>>> +	fw->cl.tx_block = true;
>>> +
>>> +	fw->chan = mbox_request_channel(&fw->cl, 0);
>>> +	if (IS_ERR(fw->chan)) {
>>> +		int ret = PTR_ERR(fw->chan);
>>> +		if (ret != -EPROBE_DEFER)
>>> +			dev_err(dev, "Failed to get mbox channel: %d\n", ret);
>>> +		return ret;
>>> +	}
>>> +
>>> +	init_completion(&fw->c);
>>> +
>>> +	platform_set_drvdata(pdev, fw);
>>> +
>>> +	rpi_firmware_print_firmware_revision(dev);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int rpi_firmware_remove(struct platform_device *pdev)
>>> +{
>>> +	struct rpi_firmware *fw = platform_get_drvdata(pdev);
>>> +
>>> +	mbox_free_channel(fw->chan);
>> I guess driver data has to be reset here:
>> platform_set_drvdata(pdev, NULL);
> Fixed.
>

I was wrong about this one as Stephen pointed out.
My point became mute now that rpi_firmware_property_list() doesn't look 
up driver data anymore.