* [PATCH 0/3] Raspberry Pi firmware driver
@ 2015-04-27 23:14 Eric Anholt
2015-04-27 23:14 ` [PATCH 1/3] dt/bindings: Add binding for the " Eric Anholt
` (2 more replies)
0 siblings, 3 replies; 23+ messages in thread
From: Eric Anholt @ 2015-04-27 23:14 UTC (permalink / raw)
To: linux-arm-kernel
This is mostly a squash/rename of my previous property and power
domain drivers, on top of the new non-channels mailbox driver I sent
earlier. I definitely like how much less boilerplate there is.
I've switched the power domains to just using the property request,
which has an obvious mapping to the on/off hooks we need to implement
(the firmware tracks which domains are currently requested by the
user).
The code can also be found at: https://github.com/anholt/linux/tree/rpi-mailbox
^ permalink raw reply [flat|nested] 23+ messages in thread* [PATCH 1/3] dt/bindings: Add binding for the Raspberry Pi firmware driver 2015-04-27 23:14 [PATCH 0/3] Raspberry Pi firmware driver Eric Anholt @ 2015-04-27 23:14 ` Eric Anholt 2015-04-28 9:34 ` Lee Jones 2015-04-29 1:42 ` Stephen Warren 2015-04-27 23:14 ` [PATCH 2/3] ARM: bcm2835: Add " Eric Anholt 2015-04-27 23:14 ` [PATCH 3/3] ARM: bcm2835: Add the necessary firmware driver information to the DT Eric Anholt 2 siblings, 2 replies; 23+ messages in thread From: Eric Anholt @ 2015-04-27 23:14 UTC (permalink / raw) To: linux-arm-kernel Signed-off-by: Eric Anholt <eric@anholt.net> --- .../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..a74c966 --- /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: Single-entry list which specifies which mailbox controller + and channel is used. + +Example: + +firmware { + compatible = "rasbperrypi,firmware"; + mboxes = <&mailbox 0> +}; -- 2.1.4 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 1/3] dt/bindings: Add binding for the Raspberry Pi firmware driver 2015-04-27 23:14 ` [PATCH 1/3] dt/bindings: Add binding for the " Eric Anholt @ 2015-04-28 9:34 ` Lee Jones 2015-04-29 1:42 ` Stephen Warren 1 sibling, 0 replies; 23+ messages in thread From: Lee Jones @ 2015-04-28 9:34 UTC (permalink / raw) To: linux-arm-kernel On Mon, 27 Apr 2015, Eric Anholt wrote: > Signed-off-by: Eric Anholt <eric@anholt.net> > --- > .../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..a74c966 > --- /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: Single-entry list which specifies which mailbox controller > + and channel is used. #power-domain-cells? > +Example: > + > +firmware { > + compatible = "rasbperrypi,firmware"; > + mboxes = <&mailbox 0> > +}; ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 1/3] dt/bindings: Add binding for the Raspberry Pi firmware driver 2015-04-27 23:14 ` [PATCH 1/3] dt/bindings: Add binding for the " Eric Anholt 2015-04-28 9:34 ` Lee Jones @ 2015-04-29 1:42 ` Stephen Warren 1 sibling, 0 replies; 23+ messages in thread From: Stephen Warren @ 2015-04-29 1:42 UTC (permalink / raw) To: linux-arm-kernel On 04/27/2015 05:14 PM, Eric Anholt wrote: This one really needs a patch description, to indicate what kind of thing this patch introduces a binding for. > diff --git a/Documentation/devicetree/bindings/arm/bcm/raspberrypi,firmware.txt b/Documentation/devicetree/bindings/arm/bcm/raspberrypi,firmware.txt > +Required properties: > + > +- compatible: Should be "rasbperrypi,firmware" > +- mboxes: Single-entry list which specifies which mailbox controller > + and channel is used. It'd be nice to mention that ../../mailbox/mailbox.txt defines the semantics of this property. ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 2/3] ARM: bcm2835: Add the Raspberry Pi firmware driver 2015-04-27 23:14 [PATCH 0/3] Raspberry Pi firmware driver Eric Anholt 2015-04-27 23:14 ` [PATCH 1/3] dt/bindings: Add binding for the " Eric Anholt @ 2015-04-27 23:14 ` Eric Anholt 2015-04-28 7:56 ` Arnd Bergmann ` (3 more replies) 2015-04-27 23:14 ` [PATCH 3/3] ARM: bcm2835: Add the necessary firmware driver information to the DT Eric Anholt 2 siblings, 4 replies; 23+ messages in thread From: Eric Anholt @ 2015-04-27 23:14 UTC (permalink / raw) To: linux-arm-kernel This gives us a function for making mailbox property channel requests of the firmware, and uses it to control the 3 power domains provided by the firmware. Signed-off-by: Eric Anholt <eric@anholt.net> --- arch/arm/mach-bcm/Kconfig | 2 + arch/arm/mach-bcm/Makefile | 1 + arch/arm/mach-bcm/raspberrypi-firmware.c | 285 +++++++++++++++++++++ .../dt-bindings/arm/raspberrypi-firmware-power.h | 16 ++ include/soc/bcm2835/raspberrypi-mailbox-property.h | 110 ++++++++ 5 files changed, 414 insertions(+) create mode 100644 arch/arm/mach-bcm/raspberrypi-firmware.c create mode 100644 include/dt-bindings/arm/raspberrypi-firmware-power.h create mode 100644 include/soc/bcm2835/raspberrypi-mailbox-property.h diff --git a/arch/arm/mach-bcm/Kconfig b/arch/arm/mach-bcm/Kconfig index 8b11f44..a907bf4 100644 --- a/arch/arm/mach-bcm/Kconfig +++ b/arch/arm/mach-bcm/Kconfig @@ -116,6 +116,8 @@ config ARCH_BCM2835 select CLKSRC_OF select PINCTRL select PINCTRL_BCM2835 + select PM_GENERIC_DOMAINS if PM + select PM_GENERIC_DOMAINS_OF if PM help This enables support for the Broadcom BCM2835 SoC. This SoC is used in the Raspberry Pi and Roku 2 devices. diff --git a/arch/arm/mach-bcm/Makefile b/arch/arm/mach-bcm/Makefile index 4c38674..bbdde34 100644 --- a/arch/arm/mach-bcm/Makefile +++ b/arch/arm/mach-bcm/Makefile @@ -33,6 +33,7 @@ endif # BCM2835 obj-$(CONFIG_ARCH_BCM2835) += board_bcm2835.o +obj-$(CONFIG_BCM2835_MBOX) += raspberrypi-firmware.o # BCM5301X obj-$(CONFIG_ARCH_BCM_5301X) += bcm_5301x.o diff --git a/arch/arm/mach-bcm/raspberrypi-firmware.c b/arch/arm/mach-bcm/raspberrypi-firmware.c new file mode 100644 index 0000000..194f05f --- /dev/null +++ b/arch/arm/mach-bcm/raspberrypi-firmware.c @@ -0,0 +1,285 @@ +/* + * 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, + * and registers some of those services with the kernel. + */ + +#include <dt-bindings/arm/raspberrypi-firmware-power.h> +#include <linux/dma-mapping.h> +#include <linux/mailbox_client.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/pm_domain.h> +#include <soc/bcm2835/raspberrypi-mailbox-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 raspberrypi_firmware { + struct device *dev; + struct genpd_onecell_data genpd_xlate; + struct mbox_client cl; + struct mbox_chan *chan; /* The property channel. */ + struct completion c; + u32 enabled; +}; + +static struct raspberrypi_firmware *firmware; +static DEFINE_MUTEX(transaction_lock); + +static void response_callback(struct mbox_client *cl, void *msg) +{ + complete(&firmware->c); +} + +/* + * Sends a request to the firmware through the BCM2835 mailbox driver, + * and synchronously waits for the reply. + */ +static int +raspberrypi_firmware_transaction(u32 chan, u32 data) +{ + u32 message = MBOX_MSG(chan, data); + int ret; + + WARN_ON(data & 0xf); + + mutex_lock(&transaction_lock); + reinit_completion(&firmware->c); + ret = mbox_send_message(firmware->chan, &message); + if (ret >= 0) { + wait_for_completion(&firmware->c); + ret = 0; + } else { + dev_err(firmware->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 raspberrypi_firmware_property_tag_header for the per-tag structure. + */ +int raspberrypi_firmware_property(void *data, size_t tag_size) +{ + size_t size = tag_size + 12; + u32 *buf; + dma_addr_t bus_addr; + int ret = 0; + + if (!firmware) + return -EPROBE_DEFER; + + /* Packets are processed a dword at a time. */ + if (size & 3) + return -EINVAL; + + buf = dma_alloc_coherent(NULL, 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] = raspberrypi_firmware_status_request; + memcpy(&buf[2], data, tag_size); + buf[size / 4 - 1] = raspberrypi_firmware_property_end; + wmb(); + + ret = raspberrypi_firmware_transaction(MBOX_CHAN_PROPERTY, bus_addr); + + rmb(); + memcpy(data, &buf[2], tag_size); + if (ret == 0 && buf[1] != raspberrypi_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(firmware->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(raspberrypi_firmware_property); + +struct raspberrypi_power_domain { + u32 domain; + struct generic_pm_domain base; +}; + +/* + * Asks the firmware to enable or disable power on a specific power + * domain. + */ +static int raspberrypi_firmware_set_power(uint32_t domain, bool on) +{ + struct { + struct raspberrypi_firmware_property_tag_header header; + u32 domain; + u32 on; + } packet; + int ret; + + memset(&packet, 0, sizeof(packet)); + packet.header.tag = raspberrypi_firmware_set_power_state; + packet.header.buf_size = 8; + packet.domain = domain; + packet.on = on; + ret = raspberrypi_firmware_property(&packet, sizeof(packet)); + if (!ret && !packet.on) + ret = -EINVAL; + return ret; +} + +static int raspberrypi_domain_off(struct generic_pm_domain *domain) +{ + struct raspberrypi_power_domain *raspberrpi_domain = + container_of(domain, struct raspberrypi_power_domain, base); + + return raspberrypi_firmware_set_power(raspberrpi_domain->domain, false); +} + +static int raspberrypi_domain_on(struct generic_pm_domain *domain) +{ + struct raspberrypi_power_domain *raspberrpi_domain = + container_of(domain, struct raspberrypi_power_domain, base); + + return raspberrypi_firmware_set_power(raspberrpi_domain->domain, true); +} + +struct raspberrypi_power_domain raspberrypi_power_domain_sdcard = { + .domain = 0, + .base = { + .name = "SDCARD", + .power_off = raspberrypi_domain_off, + .power_on = raspberrypi_domain_on, + } +}; + +struct raspberrypi_power_domain raspberrypi_power_domain_usb = { + .domain = 3, + .base = { + .name = "USB", + .power_off = raspberrypi_domain_off, + .power_on = raspberrypi_domain_on, + } +}; + +struct raspberrypi_power_domain raspberrypi_power_domain_dsi = { + .domain = 9, + .base = { + .name = "DSI", + .power_off = raspberrypi_domain_off, + .power_on = raspberrypi_domain_on, + } +}; + +static struct generic_pm_domain *raspberrypi_power_domains[] = { + [POWER_DOMAIN_SDCARD] = &raspberrypi_power_domain_sdcard.base, + [POWER_DOMAIN_USB] = &raspberrypi_power_domain_usb.base, + [POWER_DOMAIN_DSI] = &raspberrypi_power_domain_dsi.base, +}; + +static int raspberrypi_firmware_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + int ret = 0; + int i; + + firmware = devm_kzalloc(dev, sizeof(*firmware), GFP_KERNEL); + if (!firmware) + return -ENOMEM; + + firmware->cl.dev = dev; + firmware->cl.rx_callback = response_callback; + firmware->cl.tx_block = true; + + firmware->chan = mbox_request_channel(&firmware->cl, 0); + if (IS_ERR(firmware->chan)) { + ret = PTR_ERR(firmware->chan); + /* An -EBUSY from the core means it couldn't find our + * channel, because the mailbox driver hadn't + * registered yet. + */ + if (ret == -EBUSY) + ret = -EPROBE_DEFER; + else + dev_err(dev, "Failed to get mbox channel: %d\n", ret); + goto fail; + } + + init_completion(&firmware->c); + + platform_set_drvdata(pdev, firmware); + + firmware->genpd_xlate.domains = + raspberrypi_power_domains; + firmware->genpd_xlate.num_domains = + ARRAY_SIZE(raspberrypi_power_domains); + + for (i = 0; i < ARRAY_SIZE(raspberrypi_power_domains); i++) + pm_genpd_init(raspberrypi_power_domains[i], NULL, true); + + of_genpd_add_provider_onecell(dev->of_node, &firmware->genpd_xlate); + + return 0; + +fail: + firmware = NULL; + return ret; +} + +static int raspberrypi_firmware_remove(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + + of_genpd_del_provider(dev->of_node); + mbox_free_channel(firmware->chan); + + return 0; +} + +static const struct of_device_id raspberrypi_firmware_of_match[] = { + { .compatible = "raspberrypi,firmware", }, + {}, +}; +MODULE_DEVICE_TABLE(of, raspberrypi_firmware_of_match); + +static struct platform_driver raspberrypi_firmware_driver = { + .driver = { + .name = "raspberrypi-firmware", + .owner = THIS_MODULE, + .of_match_table = raspberrypi_firmware_of_match, + }, + .probe = raspberrypi_firmware_probe, + .remove = raspberrypi_firmware_remove, +}; +module_platform_driver(raspberrypi_firmware_driver); + +MODULE_AUTHOR("Eric Anholt <eric@anholt.net>"); +MODULE_DESCRIPTION("Raspberry Pi firmware driver"); +MODULE_LICENSE("GPL v2"); diff --git a/include/dt-bindings/arm/raspberrypi-firmware-power.h b/include/dt-bindings/arm/raspberrypi-firmware-power.h new file mode 100644 index 0000000..1e1ef3b --- /dev/null +++ b/include/dt-bindings/arm/raspberrypi-firmware-power.h @@ -0,0 +1,16 @@ +/* + * 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_ARM_BCM2835_MBOX_POWER_H +#define _DT_BINDINGS_ARM_BCM2835_MBOX_POWER_H + +#define POWER_DOMAIN_SDCARD 0 +#define POWER_DOMAIN_USB 1 +#define POWER_DOMAIN_DSI 2 + +#endif diff --git a/include/soc/bcm2835/raspberrypi-mailbox-property.h b/include/soc/bcm2835/raspberrypi-mailbox-property.h new file mode 100644 index 0000000..787bd75 --- /dev/null +++ b/include/soc/bcm2835/raspberrypi-mailbox-property.h @@ -0,0 +1,110 @@ +/* + * 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> + +enum raspberrypi_firmware_property_status { + raspberrypi_firmware_status_request = 0, + raspberrypi_firmware_status_success = 0x80000000, + raspberrypi_firmware_status_error = 0x80000001, +}; + +struct raspberrypi_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 raspberrypi_firmware_property_tag { + raspberrypi_firmware_property_end = 0, + raspberrypi_firmware_get_firmware_revision = 0x00000001, + + raspberrypi_firmware_set_cursor_info = 0x00008010, + raspberrypi_firmware_set_cursor_state = 0x00008011, + + raspberrypi_firmware_get_board_model = 0x00010001, + raspberrypi_firmware_get_board_revision = 0x00010002, + raspberrypi_firmware_get_board_mac_address = 0x00010003, + raspberrypi_firmware_get_board_serial = 0x00010004, + raspberrypi_firmware_get_arm_memory = 0x00010005, + raspberrypi_firmware_get_vc_memory = 0x00010006, + raspberrypi_firmware_get_clocks = 0x00010007, + raspberrypi_firmware_get_power_state = 0x00020001, + raspberrypi_firmware_get_timing = 0x00020002, + raspberrypi_firmware_set_power_state = 0x00028001, + raspberrypi_firmware_get_clock_state = 0x00030001, + raspberrypi_firmware_get_clock_rate = 0x00030002, + raspberrypi_firmware_get_voltage = 0x00030003, + raspberrypi_firmware_get_max_clock_rate = 0x00030004, + raspberrypi_firmware_get_max_voltage = 0x00030005, + raspberrypi_firmware_get_temperature = 0x00030006, + raspberrypi_firmware_get_min_clock_rate = 0x00030007, + raspberrypi_firmware_get_min_voltage = 0x00030008, + raspberrypi_firmware_get_turbo = 0x00030009, + raspberrypi_firmware_get_max_temperature = 0x0003000a, + raspberrypi_firmware_allocate_memory = 0x0003000c, + raspberrypi_firmware_lock_memory = 0x0003000d, + raspberrypi_firmware_unlock_memory = 0x0003000e, + raspberrypi_firmware_release_memory = 0x0003000f, + raspberrypi_firmware_execute_code = 0x00030010, + raspberrypi_firmware_execute_qpu = 0x00030011, + raspberrypi_firmware_set_enable_qpu = 0x00030012, + raspberrypi_firmware_get_dispmanx_resource_mem_handle = 0x00030014, + raspberrypi_firmware_get_edid_block = 0x00030020, + raspberrypi_firmware_set_clock_state = 0x00038001, + raspberrypi_firmware_set_clock_rate = 0x00038002, + raspberrypi_firmware_set_voltage = 0x00038003, + raspberrypi_firmware_set_turbo = 0x00038009, + + /* Dispmanx tags */ + raspberrypi_firmware_framebuffer_allocate = 0x00040001, + raspberrypi_firmware_framebuffer_blank = 0x00040002, + raspberrypi_firmware_framebuffer_get_physical_width_height = 0x00040003, + raspberrypi_firmware_framebuffer_get_virtual_width_height = 0x00040004, + raspberrypi_firmware_framebuffer_get_depth = 0x00040005, + raspberrypi_firmware_framebuffer_get_pixel_order = 0x00040006, + raspberrypi_firmware_framebuffer_get_alpha_mode = 0x00040007, + raspberrypi_firmware_framebuffer_get_pitch = 0x00040008, + raspberrypi_firmware_framebuffer_get_virtual_offset = 0x00040009, + raspberrypi_firmware_framebuffer_get_overscan = 0x0004000a, + raspberrypi_firmware_framebuffer_get_palette = 0x0004000b, + raspberrypi_firmware_framebuffer_release = 0x00048001, + raspberrypi_firmware_framebuffer_test_physical_width_height = 0x00044003, + raspberrypi_firmware_framebuffer_test_virtual_width_height = 0x00044004, + raspberrypi_firmware_framebuffer_test_depth = 0x00044005, + raspberrypi_firmware_framebuffer_test_pixel_order = 0x00044006, + raspberrypi_firmware_framebuffer_test_alpha_mode = 0x00044007, + raspberrypi_firmware_framebuffer_test_virtual_offset = 0x00044009, + raspberrypi_firmware_framebuffer_test_overscan = 0x0004400a, + raspberrypi_firmware_framebuffer_test_palette = 0x0004400b, + raspberrypi_firmware_framebuffer_set_physical_width_height = 0x00048003, + raspberrypi_firmware_framebuffer_set_virtual_width_height = 0x00048004, + raspberrypi_firmware_framebuffer_set_depth = 0x00048005, + raspberrypi_firmware_framebuffer_set_pixel_order = 0x00048006, + raspberrypi_firmware_framebuffer_set_alpha_mode = 0x00048007, + raspberrypi_firmware_framebuffer_set_virtual_offset = 0x00048009, + raspberrypi_firmware_framebuffer_set_overscan = 0x0004800a, + raspberrypi_firmware_framebuffer_set_palette = 0x0004800b, + + raspberrypi_firmware_get_command_line = 0x00050001, + raspberrypi_firmware_get_dma_channels = 0x00060001, +}; + +int raspberrypi_firmware_property(void *data, size_t tag_size); -- 2.1.4 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 2/3] ARM: bcm2835: Add the Raspberry Pi firmware driver 2015-04-27 23:14 ` [PATCH 2/3] ARM: bcm2835: Add " Eric Anholt @ 2015-04-28 7:56 ` Arnd Bergmann 2015-04-28 20:07 ` Eric Anholt 2015-04-28 9:27 ` Lee Jones ` (2 subsequent siblings) 3 siblings, 1 reply; 23+ messages in thread From: Arnd Bergmann @ 2015-04-28 7:56 UTC (permalink / raw) To: linux-arm-kernel On Monday 27 April 2015 16:14:08 Eric Anholt wrote: > This gives us a function for making mailbox property channel requests > of the firmware, and uses it to control the 3 power domains provided > by the firmware. > > Signed-off-by: Eric Anholt <eric@anholt.net> > --- > arch/arm/mach-bcm/Kconfig | 2 + > arch/arm/mach-bcm/Makefile | 1 + > arch/arm/mach-bcm/raspberrypi-firmware.c | 285 +++++++++++++++++++++ drivers/firmware maybe? Is this firmware the same one on the Roku 2? If so, it might need a better name. > diff --git a/include/soc/bcm2835/raspberrypi-mailbox-property.h b/include/soc/bcm2835/raspberrypi-mailbox-property.h > new file mode 100644 > index 0000000..787bd75 > --- /dev/null > +++ b/include/soc/bcm2835/raspberrypi-mailbox-property.h > @@ -0,0 +1,110 @@ > +/* > + * 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> > + > +enum raspberrypi_firmware_property_status { > + raspberrypi_firmware_status_request = 0, > + raspberrypi_firmware_status_success = 0x80000000, > + raspberrypi_firmware_status_error = 0x80000001, > +}; We tend to use ALL_CAPS for constants, even when they are in an enum. > +struct raspberrypi_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; > +}; If I read your code right, this structure is only used inside of the driver and not passed in from a device driver, so better move it inside of the firmware code. It might be best to do the same with the return codes above as well, and convert them into standard Linux calling conventions as well. > +enum raspberrypi_firmware_property_tag { > + raspberrypi_firmware_property_end = 0, > + raspberrypi_firmware_get_firmware_revision = 0x00000001, > + > + raspberrypi_firmware_set_cursor_info = 0x00008010, > + raspberrypi_firmware_set_cursor_state = 0x00008011, > + > + raspberrypi_firmware_get_board_model = 0x00010001, > + raspberrypi_firmware_get_board_revision = 0x00010002, > + raspberrypi_firmware_get_board_mac_address = 0x00010003, > + raspberrypi_firmware_get_board_serial = 0x00010004, > + raspberrypi_firmware_get_arm_memory = 0x00010005, > + raspberrypi_firmware_get_vc_memory = 0x00010006, > + raspberrypi_firmware_get_clocks = 0x00010007, How stable are these interfaces? Does the firmware guarantee to keep the ABI working, or might we need to version these? Arnd ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 2/3] ARM: bcm2835: Add the Raspberry Pi firmware driver 2015-04-28 7:56 ` Arnd Bergmann @ 2015-04-28 20:07 ` Eric Anholt 0 siblings, 0 replies; 23+ messages in thread From: Eric Anholt @ 2015-04-28 20:07 UTC (permalink / raw) To: linux-arm-kernel Arnd Bergmann <arnd@arndb.de> writes: > On Monday 27 April 2015 16:14:08 Eric Anholt wrote: >> This gives us a function for making mailbox property channel requests >> of the firmware, and uses it to control the 3 power domains provided >> by the firmware. >> >> Signed-off-by: Eric Anholt <eric@anholt.net> >> --- >> arch/arm/mach-bcm/Kconfig | 2 + >> arch/arm/mach-bcm/Makefile | 1 + >> arch/arm/mach-bcm/raspberrypi-firmware.c | 285 +++++++++++++++++++++ > > drivers/firmware maybe? Sounds fine to me. > Is this firmware the same one on the Roku 2? If so, it might need a better name. Nope. While they apparently start from the same firmware tree, it's quite diverged. >> diff --git a/include/soc/bcm2835/raspberrypi-mailbox-property.h b/include/soc/bcm2835/raspberrypi-mailbox-property.h >> new file mode 100644 >> index 0000000..787bd75 >> --- /dev/null >> +++ b/include/soc/bcm2835/raspberrypi-mailbox-property.h >> @@ -0,0 +1,110 @@ >> +/* >> + * 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> >> + >> +enum raspberrypi_firmware_property_status { >> + raspberrypi_firmware_status_request = 0, >> + raspberrypi_firmware_status_success = 0x80000000, >> + raspberrypi_firmware_status_error = 0x80000001, >> +}; > > We tend to use ALL_CAPS for constants, even when they are in an enum. Will do. >> +struct raspberrypi_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; >> +}; > > If I read your code right, this structure is only used inside of the driver and > not passed in from a device driver, so better move it inside of the firmware > code. In the code present so far, yes. However, the vc4 driver is also making property requests, and many of the still-downstream drivers need it too, I believe. > It might be best to do the same with the return codes above as well, and > convert them into standard Linux calling conventions as well. The raspberrypi_firmware_status_*? Those are getting interpreted and turned into standard Linux return values in raspberrypi_firmware_property(). I guess we could hide them inside that driver, but it seems to make sense to me to live here next to the other property channel interface enums. >> +enum raspberrypi_firmware_property_tag { >> + raspberrypi_firmware_property_end = 0, >> + raspberrypi_firmware_get_firmware_revision = 0x00000001, >> + >> + raspberrypi_firmware_set_cursor_info = 0x00008010, >> + raspberrypi_firmware_set_cursor_state = 0x00008011, >> + >> + raspberrypi_firmware_get_board_model = 0x00010001, >> + raspberrypi_firmware_get_board_revision = 0x00010002, >> + raspberrypi_firmware_get_board_mac_address = 0x00010003, >> + raspberrypi_firmware_get_board_serial = 0x00010004, >> + raspberrypi_firmware_get_arm_memory = 0x00010005, >> + raspberrypi_firmware_get_vc_memory = 0x00010006, >> + raspberrypi_firmware_get_clocks = 0x00010007, > > How stable are these interfaces? Does the firmware guarantee to keep the ABI > working, or might we need to version these? They're good about ABI, as far as I can tell. -------------- 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/20150428/93388c11/attachment-0001.sig> ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 2/3] ARM: bcm2835: Add the Raspberry Pi firmware driver 2015-04-27 23:14 ` [PATCH 2/3] ARM: bcm2835: Add " Eric Anholt 2015-04-28 7:56 ` Arnd Bergmann @ 2015-04-28 9:27 ` Lee Jones 2015-04-28 9:28 ` Lee Jones 2015-04-29 1:58 ` Stephen Warren 3 siblings, 0 replies; 23+ messages in thread From: Lee Jones @ 2015-04-28 9:27 UTC (permalink / raw) To: linux-arm-kernel On Mon, 27 Apr 2015, Eric Anholt wrote: > This gives us a function for making mailbox property channel requests > of the firmware, and uses it to control the 3 power domains provided > by the firmware. > > Signed-off-by: Eric Anholt <eric@anholt.net> > --- > arch/arm/mach-bcm/Kconfig | 2 + > arch/arm/mach-bcm/Makefile | 1 + > arch/arm/mach-bcm/raspberrypi-firmware.c | 285 +++++++++++++++++++++ drivers/firmware ? > .../dt-bindings/arm/raspberrypi-firmware-power.h | 16 ++ > include/soc/bcm2835/raspberrypi-mailbox-property.h | 110 ++++++++ > 5 files changed, 414 insertions(+) > create mode 100644 arch/arm/mach-bcm/raspberrypi-firmware.c > create mode 100644 include/dt-bindings/arm/raspberrypi-firmware-power.h > create mode 100644 include/soc/bcm2835/raspberrypi-mailbox-property.h ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 2/3] ARM: bcm2835: Add the Raspberry Pi firmware driver 2015-04-27 23:14 ` [PATCH 2/3] ARM: bcm2835: Add " Eric Anholt 2015-04-28 7:56 ` Arnd Bergmann 2015-04-28 9:27 ` Lee Jones @ 2015-04-28 9:28 ` Lee Jones 2015-04-29 1:58 ` Stephen Warren 3 siblings, 0 replies; 23+ messages in thread From: Lee Jones @ 2015-04-28 9:28 UTC (permalink / raw) To: linux-arm-kernel Don't forget to CC LKML. > This gives us a function for making mailbox property channel requests > of the firmware, and uses it to control the 3 power domains provided > by the firmware. > > Signed-off-by: Eric Anholt <eric@anholt.net> > --- > arch/arm/mach-bcm/Kconfig | 2 + > arch/arm/mach-bcm/Makefile | 1 + > arch/arm/mach-bcm/raspberrypi-firmware.c | 285 +++++++++++++++++++++ > .../dt-bindings/arm/raspberrypi-firmware-power.h | 16 ++ > include/soc/bcm2835/raspberrypi-mailbox-property.h | 110 ++++++++ > 5 files changed, 414 insertions(+) > create mode 100644 arch/arm/mach-bcm/raspberrypi-firmware.c > create mode 100644 include/dt-bindings/arm/raspberrypi-firmware-power.h > create mode 100644 include/soc/bcm2835/raspberrypi-mailbox-property.h ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 2/3] ARM: bcm2835: Add the Raspberry Pi firmware driver 2015-04-27 23:14 ` [PATCH 2/3] ARM: bcm2835: Add " Eric Anholt ` (2 preceding siblings ...) 2015-04-28 9:28 ` Lee Jones @ 2015-04-29 1:58 ` Stephen Warren 2015-04-29 2:14 ` Jassi Brar 2015-04-29 17:51 ` Eric Anholt 3 siblings, 2 replies; 23+ messages in thread From: Stephen Warren @ 2015-04-29 1:58 UTC (permalink / raw) To: linux-arm-kernel On 04/27/2015 05:14 PM, Eric Anholt wrote: > This gives us a function for making mailbox property channel requests > of the firmware, and uses it to control the 3 power domains provided > by the firmware. > diff --git a/arch/arm/mach-bcm/raspberrypi-firmware.c b/arch/arm/mach-bcm/raspberrypi-firmware.c > +/* > + * 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 raspberrypi_firmware_property_tag_header for the per-tag structure. > + */ > +int raspberrypi_firmware_property(void *data, size_t tag_size) > +{ > + size_t size = tag_size + 12; > + u32 *buf; > + dma_addr_t bus_addr; > + int ret = 0; > + > + if (!firmware) > + return -EPROBE_DEFER; I think it'd make more sense if the clients looked up the firmware driver via phandle at their probe time. This would mean: * No need for global "firmware", since clients could pass the firmware driver handle into this function. * Clients resolve deferred probe at their probe time. That way, they won't register themselves with subsystems asserting they can provide services, but find out they can't yet provide the service at that time. > +static int raspberrypi_firmware_probe(struct platform_device *pdev) BTW if you wanted, a s/raspberrypi/rpi/ in all symbols and filenames seems reasonable. I'm not really bothered either way. > +{ > + firmware->chan = mbox_request_channel(&firmware->cl, 0); > + if (IS_ERR(firmware->chan)) { > + ret = PTR_ERR(firmware->chan); > + /* An -EBUSY from the core means it couldn't find our > + * channel, because the mailbox driver hadn't > + * registered yet. > + */ > + if (ret == -EBUSY) > + ret = -EPROBE_DEFER; > + else > + dev_err(dev, "Failed to get mbox channel: %d\n", ret); I would have hoped that mbox_request_channel() returned -EPROBE_DEFER itself. It really should... > +static int raspberrypi_firmware_remove(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + > + of_genpd_del_provider(dev->of_node); > + mbox_free_channel(firmware->chan); firmware = NULL; ? ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 2/3] ARM: bcm2835: Add the Raspberry Pi firmware driver 2015-04-29 1:58 ` Stephen Warren @ 2015-04-29 2:14 ` Jassi Brar 2015-04-29 2:56 ` Jassi Brar 2015-04-29 17:51 ` Eric Anholt 1 sibling, 1 reply; 23+ messages in thread From: Jassi Brar @ 2015-04-29 2:14 UTC (permalink / raw) To: linux-arm-kernel On Wed, Apr 29, 2015 at 7:28 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 04/27/2015 05:14 PM, Eric Anholt wrote: > >> + firmware->chan = mbox_request_channel(&firmware->cl, 0); >> + if (IS_ERR(firmware->chan)) { >> + ret = PTR_ERR(firmware->chan); >> + /* An -EBUSY from the core means it couldn't find our >> + * channel, because the mailbox driver hadn't >> + * registered yet. >> + */ >> + if (ret == -EBUSY) >> + ret = -EPROBE_DEFER; >> + else >> + dev_err(dev, "Failed to get mbox channel: %d\n", ret); > > I would have hoped that mbox_request_channel() returned -EPROBE_DEFER > itself. It really should... > Yes, it should. Let me look into it. Thanks. ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 2/3] ARM: bcm2835: Add the Raspberry Pi firmware driver 2015-04-29 2:14 ` Jassi Brar @ 2015-04-29 2:56 ` Jassi Brar 0 siblings, 0 replies; 23+ messages in thread From: Jassi Brar @ 2015-04-29 2:56 UTC (permalink / raw) To: linux-arm-kernel On Wed, Apr 29, 2015 at 7:44 AM, Jassi Brar <jassisinghbrar@gmail.com> wrote: > On Wed, Apr 29, 2015 at 7:28 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: >> On 04/27/2015 05:14 PM, Eric Anholt wrote: >> >>> + firmware->chan = mbox_request_channel(&firmware->cl, 0); >>> + if (IS_ERR(firmware->chan)) { >>> + ret = PTR_ERR(firmware->chan); >>> + /* An -EBUSY from the core means it couldn't find our >>> + * channel, because the mailbox driver hadn't >>> + * registered yet. >>> + */ >>> + if (ret == -EBUSY) >>> + ret = -EPROBE_DEFER; >>> + else >>> + dev_err(dev, "Failed to get mbox channel: %d\n", ret); >> >> I would have hoped that mbox_request_channel() returned -EPROBE_DEFER >> itself. It really should... >> > Yes, it should. Let me look into it. > Turns out a patch doing that has already been submitted yesterday by Andrew Bresticker. ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 2/3] ARM: bcm2835: Add the Raspberry Pi firmware driver 2015-04-29 1:58 ` Stephen Warren 2015-04-29 2:14 ` Jassi Brar @ 2015-04-29 17:51 ` Eric Anholt 2015-04-29 23:47 ` Stephen Warren 1 sibling, 1 reply; 23+ messages in thread From: Eric Anholt @ 2015-04-29 17:51 UTC (permalink / raw) To: linux-arm-kernel Stephen Warren <swarren@wwwdotorg.org> writes: > On 04/27/2015 05:14 PM, Eric Anholt wrote: >> This gives us a function for making mailbox property channel requests >> of the firmware, and uses it to control the 3 power domains provided >> by the firmware. > >> diff --git a/arch/arm/mach-bcm/raspberrypi-firmware.c b/arch/arm/mach-bcm/raspberrypi-firmware.c > >> +/* >> + * 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 raspberrypi_firmware_property_tag_header for the per-tag structure. >> + */ >> +int raspberrypi_firmware_property(void *data, size_t tag_size) >> +{ >> + size_t size = tag_size + 12; >> + u32 *buf; >> + dma_addr_t bus_addr; >> + int ret = 0; >> + >> + if (!firmware) >> + return -EPROBE_DEFER; > > I think it'd make more sense if the clients looked up the firmware > driver via phandle at their probe time. This would mean: > > * No need for global "firmware", since clients could pass the firmware > driver handle into this function. > > * Clients resolve deferred probe at their probe time. That way, they > won't register themselves with subsystems asserting they can provide > services, but find out they can't yet provide the service at that time. The one client so far (vc4) was resolving deferred probe at its probe time, but not taking a reference on the firmware driver. I figure I'll have it do the phandle lookup and refcount -- do you still want the struct platform_device passed in here? If we de-global firmware, it's going to mean some faffing in the power domain side of things to find the device again, it seems. >> +static int raspberrypi_firmware_probe(struct platform_device *pdev) > > BTW if you wanted, a s/raspberrypi/rpi/ in all symbols and filenames > seems reasonable. I'm not really bothered either way. I lean slightly toward avoiding the abbreviation, but I could go either way as well. > I would have hoped that mbox_request_channel() returned -EPROBE_DEFER > itself. It really should... > >> +static int raspberrypi_firmware_remove(struct platform_device *pdev) >> +{ >> + struct device *dev = &pdev->dev; >> + >> + of_genpd_del_provider(dev->of_node); >> + mbox_free_channel(firmware->chan); > > firmware = NULL; ? Done. -------------- 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/20150429/eaf6b107/attachment.sig> ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 2/3] ARM: bcm2835: Add the Raspberry Pi firmware driver 2015-04-29 17:51 ` Eric Anholt @ 2015-04-29 23:47 ` Stephen Warren 2015-05-12 17:46 ` Eric Anholt 0 siblings, 1 reply; 23+ messages in thread From: Stephen Warren @ 2015-04-29 23:47 UTC (permalink / raw) To: linux-arm-kernel On 04/29/15 11:51, Eric Anholt wrote: > Stephen Warren <swarren@wwwdotorg.org> writes: > >> On 04/27/2015 05:14 PM, Eric Anholt wrote: >>> This gives us a function for making mailbox property channel requests >>> of the firmware, and uses it to control the 3 power domains provided >>> by the firmware. >> >>> diff --git a/arch/arm/mach-bcm/raspberrypi-firmware.c b/arch/arm/mach-bcm/raspberrypi-firmware.c >> >>> +/* >>> + * 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 raspberrypi_firmware_property_tag_header for the per-tag structure. >>> + */ >>> +int raspberrypi_firmware_property(void *data, size_t tag_size) >>> +{ >>> + size_t size = tag_size + 12; >>> + u32 *buf; >>> + dma_addr_t bus_addr; >>> + int ret = 0; >>> + >>> + if (!firmware) >>> + return -EPROBE_DEFER; >> >> I think it'd make more sense if the clients looked up the firmware >> driver via phandle at their probe time. This would mean: >> >> * No need for global "firmware", since clients could pass the firmware >> driver handle into this function. >> >> * Clients resolve deferred probe at their probe time. That way, they >> won't register themselves with subsystems asserting they can provide >> services, but find out they can't yet provide the service at that time. > > The one client so far (vc4) was resolving deferred probe at its probe > time, but not taking a reference on the firmware driver. I figure I'll > have it do the phandle lookup and refcount -- do you still want the > struct platform_device passed in here? If we de-global firmware, it's > going to mean some faffing in the power domain side of things to find > the device again, it seems. I think I'd expect the API in the firmware driver to require the client to pass the client DT node pointer plus a property name, and do all the lookup itself. That's what most DT resource lookup APIs in the kernel do now. ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 2/3] ARM: bcm2835: Add the Raspberry Pi firmware driver 2015-04-29 23:47 ` Stephen Warren @ 2015-05-12 17:46 ` Eric Anholt 2015-05-12 22:08 ` Stephen Warren 0 siblings, 1 reply; 23+ messages in thread From: Eric Anholt @ 2015-05-12 17:46 UTC (permalink / raw) To: linux-arm-kernel Stephen Warren <swarren@wwwdotorg.org> writes: > On 04/29/15 11:51, Eric Anholt wrote: >> Stephen Warren <swarren@wwwdotorg.org> writes: >> >>> On 04/27/2015 05:14 PM, Eric Anholt wrote: >>>> This gives us a function for making mailbox property channel requests >>>> of the firmware, and uses it to control the 3 power domains provided >>>> by the firmware. >>> >>>> diff --git a/arch/arm/mach-bcm/raspberrypi-firmware.c b/arch/arm/mach-bcm/raspberrypi-firmware.c >>> >>>> +/* >>>> + * 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 raspberrypi_firmware_property_tag_header for the per-tag structure. >>>> + */ >>>> +int raspberrypi_firmware_property(void *data, size_t tag_size) >>>> +{ >>>> + size_t size = tag_size + 12; >>>> + u32 *buf; >>>> + dma_addr_t bus_addr; >>>> + int ret = 0; >>>> + >>>> + if (!firmware) >>>> + return -EPROBE_DEFER; >>> >>> I think it'd make more sense if the clients looked up the firmware >>> driver via phandle at their probe time. This would mean: >>> >>> * No need for global "firmware", since clients could pass the firmware >>> driver handle into this function. >>> >>> * Clients resolve deferred probe at their probe time. That way, they >>> won't register themselves with subsystems asserting they can provide >>> services, but find out they can't yet provide the service at that time. >> >> The one client so far (vc4) was resolving deferred probe at its probe >> time, but not taking a reference on the firmware driver. I figure I'll >> have it do the phandle lookup and refcount -- do you still want the >> struct platform_device passed in here? If we de-global firmware, it's >> going to mean some faffing in the power domain side of things to find >> the device again, it seems. > > I think I'd expect the API in the firmware driver to require the client > to pass the client DT node pointer plus a property name, and do all the > lookup itself. That's what most DT resource lookup APIs in the kernel do > now. I've made this change, but it's not great -- the client has to know some details of how this driver is structured (that it sets the drvdata) in order to figure out whether the driver is loaded or not and return -EPROBE_DEFERRED. I couldn't find any other existing solutions than that in the tree. -------------- 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/20150512/8ec643cb/attachment.sig> ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 2/3] ARM: bcm2835: Add the Raspberry Pi firmware driver 2015-05-12 17:46 ` Eric Anholt @ 2015-05-12 22:08 ` Stephen Warren 2015-05-13 0:38 ` Eric Anholt 0 siblings, 1 reply; 23+ messages in thread From: Stephen Warren @ 2015-05-12 22:08 UTC (permalink / raw) To: linux-arm-kernel On 05/12/2015 11:46 AM, Eric Anholt wrote: > Stephen Warren <swarren@wwwdotorg.org> writes: > >> On 04/29/15 11:51, Eric Anholt wrote: >>> Stephen Warren <swarren@wwwdotorg.org> writes: >>> >>>> On 04/27/2015 05:14 PM, Eric Anholt wrote: >>>>> This gives us a function for making mailbox property channel requests >>>>> of the firmware, and uses it to control the 3 power domains provided >>>>> by the firmware. >>>> >>>>> diff --git a/arch/arm/mach-bcm/raspberrypi-firmware.c b/arch/arm/mach-bcm/raspberrypi-firmware.c >>>> >>>>> +/* >>>>> + * 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 raspberrypi_firmware_property_tag_header for the per-tag structure. >>>>> + */ >>>>> +int raspberrypi_firmware_property(void *data, size_t tag_size) >>>>> +{ >>>>> + size_t size = tag_size + 12; >>>>> + u32 *buf; >>>>> + dma_addr_t bus_addr; >>>>> + int ret = 0; >>>>> + >>>>> + if (!firmware) >>>>> + return -EPROBE_DEFER; >>>> >>>> I think it'd make more sense if the clients looked up the firmware >>>> driver via phandle at their probe time. This would mean: >>>> >>>> * No need for global "firmware", since clients could pass the firmware >>>> driver handle into this function. >>>> >>>> * Clients resolve deferred probe at their probe time. That way, they >>>> won't register themselves with subsystems asserting they can provide >>>> services, but find out they can't yet provide the service at that time. >>> >>> The one client so far (vc4) was resolving deferred probe at its probe >>> time, but not taking a reference on the firmware driver. I figure I'll >>> have it do the phandle lookup and refcount -- do you still want the >>> struct platform_device passed in here? If we de-global firmware, it's >>> going to mean some faffing in the power domain side of things to find >>> the device again, it seems. >> >> I think I'd expect the API in the firmware driver to require the client >> to pass the client DT node pointer plus a property name, and do all the >> lookup itself. That's what most DT resource lookup APIs in the kernel do >> now. > > I've made this change, but it's not great -- the client has to know some > details of how this driver is structured (that it sets the drvdata) in > order to figure out whether the driver is loaded or not and return > -EPROBE_DEFERRED. I couldn't find any other existing solutions than > that in the tree. The client shouldn't need to know that. I'd expect the client to pass a DT node pointer to the provider's driver, and that provider driver would know and isolate all the details about its own internals. The provider could return some kind of handler/... to the provider device if required. I would expect any subsystem that supports client->provider references in DT would have an example of this (e.g. IRQs, GPIOs, regulators, ...). However, the code for those can be rather complex to dive into for the first time. For a fairly simple standalone example, check out: Provider: drivers/amba/tegra-ahb.c tegra_ahb_enable_smmu() Consumer: drivers/iommu/tegra-smmu.c tegra_smmu_ahb_enable() (called from tegra_smmu_probe() in the same file) ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 2/3] ARM: bcm2835: Add the Raspberry Pi firmware driver 2015-05-12 22:08 ` Stephen Warren @ 2015-05-13 0:38 ` Eric Anholt 2015-05-13 18:28 ` Stephen Warren 0 siblings, 1 reply; 23+ messages in thread From: Eric Anholt @ 2015-05-13 0:38 UTC (permalink / raw) To: linux-arm-kernel Stephen Warren <swarren@wwwdotorg.org> writes: > On 05/12/2015 11:46 AM, Eric Anholt wrote: >> Stephen Warren <swarren@wwwdotorg.org> writes: >> >>> On 04/29/15 11:51, Eric Anholt wrote: >>>> Stephen Warren <swarren@wwwdotorg.org> writes: >>>> >>>>> On 04/27/2015 05:14 PM, Eric Anholt wrote: >>>>>> This gives us a function for making mailbox property channel requests >>>>>> of the firmware, and uses it to control the 3 power domains provided >>>>>> by the firmware. >>>>> >>>>>> diff --git a/arch/arm/mach-bcm/raspberrypi-firmware.c b/arch/arm/mach-bcm/raspberrypi-firmware.c >>>>> >>>>>> +/* >>>>>> + * 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 raspberrypi_firmware_property_tag_header for the per-tag structure. >>>>>> + */ >>>>>> +int raspberrypi_firmware_property(void *data, size_t tag_size) >>>>>> +{ >>>>>> + size_t size = tag_size + 12; >>>>>> + u32 *buf; >>>>>> + dma_addr_t bus_addr; >>>>>> + int ret = 0; >>>>>> + >>>>>> + if (!firmware) >>>>>> + return -EPROBE_DEFER; >>>>> >>>>> I think it'd make more sense if the clients looked up the firmware >>>>> driver via phandle at their probe time. This would mean: >>>>> >>>>> * No need for global "firmware", since clients could pass the firmware >>>>> driver handle into this function. >>>>> >>>>> * Clients resolve deferred probe at their probe time. That way, they >>>>> won't register themselves with subsystems asserting they can provide >>>>> services, but find out they can't yet provide the service at that time. >>>> >>>> The one client so far (vc4) was resolving deferred probe at its probe >>>> time, but not taking a reference on the firmware driver. I figure I'll >>>> have it do the phandle lookup and refcount -- do you still want the >>>> struct platform_device passed in here? If we de-global firmware, it's >>>> going to mean some faffing in the power domain side of things to find >>>> the device again, it seems. >>> >>> I think I'd expect the API in the firmware driver to require the client >>> to pass the client DT node pointer plus a property name, and do all the >>> lookup itself. That's what most DT resource lookup APIs in the kernel do >>> now. >> >> I've made this change, but it's not great -- the client has to know some >> details of how this driver is structured (that it sets the drvdata) in >> order to figure out whether the driver is loaded or not and return >> -EPROBE_DEFERRED. I couldn't find any other existing solutions than >> that in the tree. > > The client shouldn't need to know that. > > I'd expect the client to pass a DT node pointer to the provider's > driver, and that provider driver would know and isolate all the details > about its own internals. The provider could return some kind of > handler/... to the provider device if required. > > I would expect any subsystem that supports client->provider references > in DT would have an example of this (e.g. IRQs, GPIOs, regulators, ...). > However, the code for those can be rather complex to dive into for the > first time. For a fairly simple standalone example, check out: > > Provider: > drivers/amba/tegra-ahb.c > tegra_ahb_enable_smmu() > > Consumer: > drivers/iommu/tegra-smmu.c > tegra_smmu_ahb_enable() > (called from tegra_smmu_probe() in the same file) It's somewhat suspicious to me as an example of how -EPROBE_DEFER ought to work, that tegra_ahb_enable_smmu()'s return value isn't being checked by tegra_smmu_ahb_enable(). That said, this looks like this is "Yeah, just call into the producer driver to do setup, and if it returns -EPROBE_DEFER, then bail out and return -EPROBE_DEFER, yourself." That's what I was doing in this function and its consumers, that you commented on, only I wasn't passing in the dt node. Is that all you wanted changed? -------------- 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/20150512/1e75a572/attachment.sig> ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 2/3] ARM: bcm2835: Add the Raspberry Pi firmware driver 2015-05-13 0:38 ` Eric Anholt @ 2015-05-13 18:28 ` Stephen Warren 0 siblings, 0 replies; 23+ messages in thread From: Stephen Warren @ 2015-05-13 18:28 UTC (permalink / raw) To: linux-arm-kernel On 05/12/2015 06:38 PM, Eric Anholt wrote: > Stephen Warren <swarren@wwwdotorg.org> writes: > >> On 05/12/2015 11:46 AM, Eric Anholt wrote: >>> Stephen Warren <swarren@wwwdotorg.org> writes: >>> >>>> On 04/29/15 11:51, Eric Anholt wrote: >>>>> Stephen Warren <swarren@wwwdotorg.org> writes: >>>>> >>>>>> On 04/27/2015 05:14 PM, Eric Anholt wrote: >>>>>>> This gives us a function for making mailbox property channel requests >>>>>>> of the firmware, and uses it to control the 3 power domains provided >>>>>>> by the firmware. >>>>>> >>>>>>> diff --git a/arch/arm/mach-bcm/raspberrypi-firmware.c b/arch/arm/mach-bcm/raspberrypi-firmware.c >>>>>> >>>>>>> +/* >>>>>>> + * 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 raspberrypi_firmware_property_tag_header for the per-tag structure. >>>>>>> + */ >>>>>>> +int raspberrypi_firmware_property(void *data, size_t tag_size) >>>>>>> +{ >>>>>>> + size_t size = tag_size + 12; >>>>>>> + u32 *buf; >>>>>>> + dma_addr_t bus_addr; >>>>>>> + int ret = 0; >>>>>>> + >>>>>>> + if (!firmware) >>>>>>> + return -EPROBE_DEFER; >>>>>> >>>>>> I think it'd make more sense if the clients looked up the firmware >>>>>> driver via phandle at their probe time. This would mean: >>>>>> >>>>>> * No need for global "firmware", since clients could pass the firmware >>>>>> driver handle into this function. >>>>>> >>>>>> * Clients resolve deferred probe at their probe time. That way, they >>>>>> won't register themselves with subsystems asserting they can provide >>>>>> services, but find out they can't yet provide the service at that time. >>>>> >>>>> The one client so far (vc4) was resolving deferred probe at its probe >>>>> time, but not taking a reference on the firmware driver. I figure I'll >>>>> have it do the phandle lookup and refcount -- do you still want the >>>>> struct platform_device passed in here? If we de-global firmware, it's >>>>> going to mean some faffing in the power domain side of things to find >>>>> the device again, it seems. >>>> >>>> I think I'd expect the API in the firmware driver to require the client >>>> to pass the client DT node pointer plus a property name, and do all the >>>> lookup itself. That's what most DT resource lookup APIs in the kernel do >>>> now. >>> >>> I've made this change, but it's not great -- the client has to know some >>> details of how this driver is structured (that it sets the drvdata) in >>> order to figure out whether the driver is loaded or not and return >>> -EPROBE_DEFERRED. I couldn't find any other existing solutions than >>> that in the tree. >> >> The client shouldn't need to know that. >> >> I'd expect the client to pass a DT node pointer to the provider's >> driver, and that provider driver would know and isolate all the details >> about its own internals. The provider could return some kind of >> handler/... to the provider device if required. >> >> I would expect any subsystem that supports client->provider references >> in DT would have an example of this (e.g. IRQs, GPIOs, regulators, ...). >> However, the code for those can be rather complex to dive into for the >> first time. For a fairly simple standalone example, check out: >> >> Provider: >> drivers/amba/tegra-ahb.c >> tegra_ahb_enable_smmu() >> >> Consumer: >> drivers/iommu/tegra-smmu.c >> tegra_smmu_ahb_enable() >> (called from tegra_smmu_probe() in the same file) > > It's somewhat suspicious to me as an example of how -EPROBE_DEFER ought > to work, that tegra_ahb_enable_smmu()'s return value isn't being checked > by tegra_smmu_ahb_enable(). Oh that's certainly a bug, and needs to be fixed; both tegra_smmu_ahb_enable() and tegra_smmu_probe() should propagate any -EPROBE_DEFERRED out if they happen. I'll file a bug for that internally. > That said, this looks like this is "Yeah, just call into the producer > driver to do setup, and if it returns -EPROBE_DEFER, then bail out and > return -EPROBE_DEFER, yourself." That's what I was doing in this > function and its consumers, that you commented on, only I wasn't passing > in the dt node. Is that all you wanted changed? (very briefly looking at the quoted history above) I think the primary issue was the -EPROBE_DEFERRED in the patch is returned by a function that sends a message, rather than a function which looks up the provider. There's no general reason to believe that a message will be sent during a client driver's probe, so in general a client driver may only notice that the firmware provider is missing when the first message is sent, which is too late to defer the client driver's probe. However, any client should always look up any service provider during probe, and hence be able to defer its own probe. ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 3/3] ARM: bcm2835: Add the necessary firmware driver information to the DT 2015-04-27 23:14 [PATCH 0/3] Raspberry Pi firmware driver Eric Anholt 2015-04-27 23:14 ` [PATCH 1/3] dt/bindings: Add binding for the " Eric Anholt 2015-04-27 23:14 ` [PATCH 2/3] ARM: bcm2835: Add " Eric Anholt @ 2015-04-27 23:14 ` Eric Anholt 2015-04-28 9:33 ` Lee Jones 2015-04-29 1:44 ` Stephen Warren 2 siblings, 2 replies; 23+ messages in thread From: Eric Anholt @ 2015-04-27 23:14 UTC (permalink / raw) To: linux-arm-kernel This gets the USB power domain turned on so that DWC2 can probe, even if the bootloader didn't turn it on for us (which is the case if you use the RPi firmware without chainloading U-Boot). Signed-off-by: Eric Anholt <eric@anholt.net> --- arch/arm/boot/dts/bcm2835-rpi.dtsi | 13 +++++++++++++ arch/arm/boot/dts/bcm2835.dtsi | 2 +- 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/arch/arm/boot/dts/bcm2835-rpi.dtsi b/arch/arm/boot/dts/bcm2835-rpi.dtsi index 46780bb..fa9afd1 100644 --- a/arch/arm/boot/dts/bcm2835-rpi.dtsi +++ b/arch/arm/boot/dts/bcm2835-rpi.dtsi @@ -1,3 +1,4 @@ +#include <dt-bindings/arm/raspberrypi-firmware-power.h> #include "bcm2835.dtsi" / { @@ -14,6 +15,14 @@ linux,default-trigger = "heartbeat"; }; }; + + soc { + firmware: firmware { + compatible = "raspberrypi,firmware"; + mboxes = <&mailbox 0>; + #power-domain-cells = <1>; + }; + }; }; &gpio { @@ -49,3 +58,7 @@ status = "okay"; bus-width = <4>; }; + +&usb { + power-domains = <&firmware POWER_DOMAIN_USB>; +}; diff --git a/arch/arm/boot/dts/bcm2835.dtsi b/arch/arm/boot/dts/bcm2835.dtsi index 664aa25..5734650 100644 --- a/arch/arm/boot/dts/bcm2835.dtsi +++ b/arch/arm/boot/dts/bcm2835.dtsi @@ -148,7 +148,7 @@ status = "disabled"; }; - usb at 7e980000 { + usb: usb at 7e980000 { compatible = "brcm,bcm2835-usb"; reg = <0x7e980000 0x10000>; interrupts = <1 9>; -- 2.1.4 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 3/3] ARM: bcm2835: Add the necessary firmware driver information to the DT 2015-04-27 23:14 ` [PATCH 3/3] ARM: bcm2835: Add the necessary firmware driver information to the DT Eric Anholt @ 2015-04-28 9:33 ` Lee Jones 2015-04-29 1:44 ` Stephen Warren 1 sibling, 0 replies; 23+ messages in thread From: Lee Jones @ 2015-04-28 9:33 UTC (permalink / raw) To: linux-arm-kernel On Mon, 27 Apr 2015, Eric Anholt wrote: > This gets the USB power domain turned on so that DWC2 can probe, even > if the bootloader didn't turn it on for us (which is the case if you > use the RPi firmware without chainloading U-Boot). > > Signed-off-by: Eric Anholt <eric@anholt.net> > --- > arch/arm/boot/dts/bcm2835-rpi.dtsi | 13 +++++++++++++ > arch/arm/boot/dts/bcm2835.dtsi | 2 +- > 2 files changed, 14 insertions(+), 1 deletion(-) Acked-by: Lee Jones <lee@kernel.org> > diff --git a/arch/arm/boot/dts/bcm2835-rpi.dtsi b/arch/arm/boot/dts/bcm2835-rpi.dtsi > index 46780bb..fa9afd1 100644 > --- a/arch/arm/boot/dts/bcm2835-rpi.dtsi > +++ b/arch/arm/boot/dts/bcm2835-rpi.dtsi > @@ -1,3 +1,4 @@ > +#include <dt-bindings/arm/raspberrypi-firmware-power.h> > #include "bcm2835.dtsi" > > / { > @@ -14,6 +15,14 @@ > linux,default-trigger = "heartbeat"; > }; > }; > + > + soc { > + firmware: firmware { > + compatible = "raspberrypi,firmware"; > + mboxes = <&mailbox 0>; > + #power-domain-cells = <1>; > + }; > + }; > }; > > &gpio { > @@ -49,3 +58,7 @@ > status = "okay"; > bus-width = <4>; > }; > + > +&usb { > + power-domains = <&firmware POWER_DOMAIN_USB>; > +}; > diff --git a/arch/arm/boot/dts/bcm2835.dtsi b/arch/arm/boot/dts/bcm2835.dtsi > index 664aa25..5734650 100644 > --- a/arch/arm/boot/dts/bcm2835.dtsi > +++ b/arch/arm/boot/dts/bcm2835.dtsi > @@ -148,7 +148,7 @@ > status = "disabled"; > }; > > - usb at 7e980000 { > + usb: usb at 7e980000 { > compatible = "brcm,bcm2835-usb"; > reg = <0x7e980000 0x10000>; > interrupts = <1 9>; ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 3/3] ARM: bcm2835: Add the necessary firmware driver information to the DT 2015-04-27 23:14 ` [PATCH 3/3] ARM: bcm2835: Add the necessary firmware driver information to the DT Eric Anholt 2015-04-28 9:33 ` Lee Jones @ 2015-04-29 1:44 ` Stephen Warren 2015-04-29 16:29 ` Eric Anholt 1 sibling, 1 reply; 23+ messages in thread From: Stephen Warren @ 2015-04-29 1:44 UTC (permalink / raw) To: linux-arm-kernel On 04/27/2015 05:14 PM, Eric Anholt wrote: > This gets the USB power domain turned on so that DWC2 can probe, even > if the bootloader didn't turn it on for us (which is the case if you > use the RPi firmware without chainloading U-Boot). > diff --git a/arch/arm/boot/dts/bcm2835-rpi.dtsi b/arch/arm/boot/dts/bcm2835-rpi.dtsi > +&usb { > + power-domains = <&firmware POWER_DOMAIN_USB>; > +}; That property doesn't seem to be documented in Documentation/devicetree/bindings/usb/dwc2.txt (which defines compatible = "brcm,bcm2835-usb"). Is it implemented? ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 3/3] ARM: bcm2835: Add the necessary firmware driver information to the DT 2015-04-29 1:44 ` Stephen Warren @ 2015-04-29 16:29 ` Eric Anholt 2015-04-29 23:43 ` Stephen Warren 0 siblings, 1 reply; 23+ messages in thread From: Eric Anholt @ 2015-04-29 16:29 UTC (permalink / raw) To: linux-arm-kernel Stephen Warren <swarren@wwwdotorg.org> writes: > On 04/27/2015 05:14 PM, Eric Anholt wrote: >> This gets the USB power domain turned on so that DWC2 can probe, even >> if the bootloader didn't turn it on for us (which is the case if you >> use the RPi firmware without chainloading U-Boot). > >> diff --git a/arch/arm/boot/dts/bcm2835-rpi.dtsi b/arch/arm/boot/dts/bcm2835-rpi.dtsi > >> +&usb { >> + power-domains = <&firmware POWER_DOMAIN_USB>; >> +}; > > That property doesn't seem to be documented in > Documentation/devicetree/bindings/usb/dwc2.txt (which defines compatible > = "brcm,bcm2835-usb"). Is it implemented? power-domains is a generic property that's parsed by drivers/base/power/domain.c. See also Documentation/devicetree/bindings/power/power_domain.txt& -------------- 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/20150429/915b498e/attachment-0001.sig> ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 3/3] ARM: bcm2835: Add the necessary firmware driver information to the DT 2015-04-29 16:29 ` Eric Anholt @ 2015-04-29 23:43 ` Stephen Warren 0 siblings, 0 replies; 23+ messages in thread From: Stephen Warren @ 2015-04-29 23:43 UTC (permalink / raw) To: linux-arm-kernel On 04/29/15 10:29, Eric Anholt wrote: > Stephen Warren <swarren@wwwdotorg.org> writes: > >> On 04/27/2015 05:14 PM, Eric Anholt wrote: >>> This gets the USB power domain turned on so that DWC2 can probe, even >>> if the bootloader didn't turn it on for us (which is the case if you >>> use the RPi firmware without chainloading U-Boot). >> >>> diff --git a/arch/arm/boot/dts/bcm2835-rpi.dtsi b/arch/arm/boot/dts/bcm2835-rpi.dtsi >> >>> +&usb { >>> + power-domains = <&firmware POWER_DOMAIN_USB>; >>> +}; >> >> That property doesn't seem to be documented in >> Documentation/devicetree/bindings/usb/dwc2.txt (which defines compatible >> = "brcm,bcm2835-usb"). Is it implemented? > > power-domains is a generic property that's parsed by > drivers/base/power/domain.c. See also > Documentation/devicetree/bindings/power/power_domain.txt& Sure, but the binding for the node still needs to mention that it's acceptable for that property to exist (a/k/a that the USB binding inherits from or aggregates the power domains binding). Part of mentioning that would be a description of what the entries in the power-domains property mean (how many are legal, what each index represents). ^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2015-05-13 18:28 UTC | newest] Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-04-27 23:14 [PATCH 0/3] Raspberry Pi firmware driver Eric Anholt 2015-04-27 23:14 ` [PATCH 1/3] dt/bindings: Add binding for the " Eric Anholt 2015-04-28 9:34 ` Lee Jones 2015-04-29 1:42 ` Stephen Warren 2015-04-27 23:14 ` [PATCH 2/3] ARM: bcm2835: Add " Eric Anholt 2015-04-28 7:56 ` Arnd Bergmann 2015-04-28 20:07 ` Eric Anholt 2015-04-28 9:27 ` Lee Jones 2015-04-28 9:28 ` Lee Jones 2015-04-29 1:58 ` Stephen Warren 2015-04-29 2:14 ` Jassi Brar 2015-04-29 2:56 ` Jassi Brar 2015-04-29 17:51 ` Eric Anholt 2015-04-29 23:47 ` Stephen Warren 2015-05-12 17:46 ` Eric Anholt 2015-05-12 22:08 ` Stephen Warren 2015-05-13 0:38 ` Eric Anholt 2015-05-13 18:28 ` Stephen Warren 2015-04-27 23:14 ` [PATCH 3/3] ARM: bcm2835: Add the necessary firmware driver information to the DT Eric Anholt 2015-04-28 9:33 ` Lee Jones 2015-04-29 1:44 ` Stephen Warren 2015-04-29 16:29 ` Eric Anholt 2015-04-29 23:43 ` Stephen Warren
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.