Raspberry Pi firmware driver
Subsystems (5)
cover Raspberry Pi firmware driver
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
PATCH 1/3 dt/bindings: Add binding for the Raspberry Pi firmware driver
--- .../devicetree/bindings/arm/bcm/raspberrypi,firmware.txt | 14 ++++++++++++++ 1 file changed, 14 insertions(+) create mode 100644 Documentation/devicetree/bindings/arm/bcm/raspberrypi,firmware.txt
Documentation/devicetree/bindings/arm/bcm/raspberrypi,firmware.txt new file +14 -0
@@ -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> +};
Signature · 1 line
2.1.4
PATCH 1/3 dt/bindings: Add binding for the Raspberry Pi firmware driver
Quoted 12 lines · Eric Anholt
Signed-off-by Eric Anholt <[email protected]>--- .../devicetree/bindings/arm/bcm/raspberrypi,firmware.txt | 14 ++++++++++++++ 1 file changed, 14 insertions(+) create mode 100644 Documentation/devicetree/bindings/arm/bcm/raspberrypi,firmware.txtDocumentation/devicetree/bindings/arm/bcm/raspberrypi,firmware.txt new file +0
-0
Documentation/devicetree/bindings/arm/bcm/raspberrypi,firmware.txt line 7
Quoted 7 lines
Documentation/devicetree/bindings/arm/bcm/raspberrypi,firmware.txt new file +7
-0@@ -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?
Quoted 6 lines
Documentation/devicetree/bindings/arm/bcm/raspberrypi,firmware.txt new file +6
-0+Example: + +firmware { + compatible = "rasbperrypi,firmware"; + mboxes = <&mailbox 0> +};
PATCH 1/3 dt/bindings: Add binding for the Raspberry Pi firmware driver
In reply to Eric AnholtOn 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.
Documentation/devicetree/bindings/arm/bcm/raspberrypi,firmware.txt line 7
Quoted 7 lines
Documentation/devicetree/bindings/arm/bcm/raspberrypi,firmware.txt +5
-0+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.
PATCH 2/3 ARM: bcm2835: Add the Raspberry Pi firmware driver
In reply to Eric AnholtThis 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.
--- 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
arch/arm/mach-bcm/Kconfig +2 -0
@@ -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.
arch/arm/mach-bcm/Makefile +1 -0
@@ -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
arch/arm/mach-bcm/raspberrypi-firmware.c new file +285 -0
285 lines not on this page · raw message
include/dt-bindings/arm/raspberrypi-firmware-power.h new file +16 -0
@@ -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
include/soc/bcm2835/raspberrypi-mailbox-property.h new file +110 -0
110 lines not on this page · raw message
Signature · 1 line
2.1.4
PATCH 2/3 ARM: bcm2835: Add the Raspberry Pi firmware driver
Quoted 9 lines · 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 <[email protected]>--- 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.
Quoted 11 lines
include/soc/bcm2835/raspberrypi-mailbox-property.h new file +5
-0@@ -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
include/soc/bcm2835/raspberrypi-mailbox-property.h line 15
Quoted 10 lines
include/soc/bcm2835/raspberrypi-mailbox-property.h new file +10
-0+* 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.
Quoted 7 lines
include/soc/bcm2835/raspberrypi-mailbox-property.h new file +7
-0+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. +*/
include/soc/bcm2835/raspberrypi-mailbox-property.h line 33
Quoted 10 lines
include/soc/bcm2835/raspberrypi-mailbox-property.h new file +10
-0+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.
Quoted 4 lines
include/soc/bcm2835/raspberrypi-mailbox-property.h new file +4
-0+enum raspberrypi_firmware_property_tag { + raspberrypi_firmware_property_end =0 , + raspberrypi_firmware_get_firmware_revision =0x00000001 , +
include/soc/bcm2835/raspberrypi-mailbox-property.h line 48
Quoted 10 lines
include/soc/bcm2835/raspberrypi-mailbox-property.h new file +10
-0+ 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
PATCH 2/3 ARM: bcm2835: Add the Raspberry Pi firmware driver
Quoted 5 lines · Arnd Bergmann
Quoted 4 lines · 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.
Quoted 7 lines
Quoted 5 lines
Signed-off-by Eric Anholt <[email protected]>--- arch/arm/mach-bcm/Kconfig | 2 + arch/arm/mach-bcm/Makefile | 1 + arch/arm/mach-bcm/raspberrypi-firmware.c | 285 +++++++++++++++++++++
drivers/firmwaremaybe?
Sounds fine to me.
Quoted 1 line
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.
Quoted 15 lines
Quoted 15 lines
include/soc/bcm2835/raspberrypi-mailbox-property.h new file +9
-0@@ -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>
Quoted 8 lines
include/soc/bcm2835/raspberrypi-mailbox-property.h line 15
Quoted 6 lines
include/soc/bcm2835/raspberrypi-mailbox-property.h new file +6
-0+ +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_CAPSfor constants, even when they are in an enum.
Will do.
Quoted 13 lines
Quoted 13 lines
include/soc/bcm2835/raspberrypi-mailbox-property.h new file +13
-0+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
Quoted 8 lines
include/soc/bcm2835/raspberrypi-mailbox-property.h line 33
Quoted 4 lines
include/soc/bcm2835/raspberrypi-mailbox-property.h new file +4
-0+* 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.
Quoted 2 lines
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.
Quoted 9 lines
Quoted 9 lines
include/soc/bcm2835/raspberrypi-mailbox-property.h new file +9
-0+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 ,
Quoted 8 lines
include/soc/bcm2835/raspberrypi-mailbox-property.h line 48
Quoted 5 lines
include/soc/bcm2835/raspberrypi-mailbox-property.h new file +5
-0+ 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>
PATCH 2/3 ARM: bcm2835: Add the Raspberry Pi firmware driver
In reply to Eric AnholtQuoted 9 lines · Eric Anholt
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 <[email protected]>--- arch/arm/mach-bcm/Kconfig | 2 + arch/arm/mach-bcm/Makefile | 1 + arch/arm/mach-bcm/raspberrypi-firmware.c | 285 +++++++++++++++++++++
drivers/firmware ?
Quoted 6 lines
.../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
PATCH 2/3 ARM: bcm2835: Add the Raspberry Pi firmware driver
In reply to Eric AnholtDon't forget to CC LKML.
Quoted 15 lines
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 <[email protected]>--- 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
PATCH 2/3 ARM: bcm2835: Add the Raspberry Pi firmware driver
In reply to Eric AnholtQuoted 13 lines · Eric Anholt
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.
arch/arm/mach-bcm/raspberrypi-firmware.c +7
-0+/* +* 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.
arch/arm/mach-bcm/raspberrypi-firmware.c line 88
Quoted 10 lines
arch/arm/mach-bcm/raspberrypi-firmware.c +10
-0+*/ +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.
Quoted 1 line
+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.
arch/arm/mach-bcm/raspberrypi-firmware.c line 231
Quoted 13 lines
arch/arm/mach-bcm/raspberrypi-firmware.c +12
-0+{ + 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...
arch/arm/mach-bcm/raspberrypi-firmware.c line 261
Quoted 6 lines
arch/arm/mach-bcm/raspberrypi-firmware.c +6
-0+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; ?
PATCH 2/3 ARM: bcm2835: Add the Raspberry Pi firmware driver
Quoted 14 lines · Stephen Warren
arch/arm/mach-bcm/raspberrypi-firmware.c line 231
Quoted 11 lines · Eric Anholt
arch/arm/mach-bcm/raspberrypi-firmware.c new file +11
-0+ 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);
Quoted 3 lines
I would have hoped that
mbox_request_channel()returned -EPROBE_DEFER itself. It really should...
Yes, it should. Let me look into it.
Thanks.
PATCH 2/3 ARM: bcm2835: Add the Raspberry Pi firmware driver
Quoted 15 lines · Jassi Brar
Quoted 14 lines · Stephen Warren
Quoted 11 lines · Eric Anholt
arch/arm/mach-bcm/raspberrypi-firmware.c new file +11
-0+ 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);
Quoted 5 lines
Quoted 3 lines
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.
PATCH 2/3 ARM: bcm2835: Add the Raspberry Pi firmware driver
In reply to Stephen WarrenQuoted 28 lines · Stephen Warren
Quoted 13 lines · Eric Anholt
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.
arch/arm/mach-bcm/raspberrypi-firmware.c +7
-0+/* +* 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. arch/arm/mach-bcm/raspberrypi-firmware.c line 88
Quoted 10 lines
arch/arm/mach-bcm/raspberrypi-firmware.c +10
-0+*/ +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:
Quoted 6 lines
* 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.
Quoted 4 lines
Quoted 1 line
+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.
Quoted 11 lines
I would have hoped that
mbox_request_channel()returned -EPROBE_DEFER itself. It really should...arch/arm/mach-bcm/raspberrypi-firmware.c line 261
Quoted 6 lines
arch/arm/mach-bcm/raspberrypi-firmware.c +6
-0+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>
PATCH 2/3 ARM: bcm2835: Add the Raspberry Pi firmware driver
Quoted 37 lines · Eric Anholt
Quoted 28 lines · Stephen Warren
Quoted 13 lines · Eric Anholt
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.
arch/arm/mach-bcm/raspberrypi-firmware.c +7
-0+/* +* 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. arch/arm/mach-bcm/raspberrypi-firmware.c line 88
Quoted 10 lines
arch/arm/mach-bcm/raspberrypi-firmware.c +10
-0+*/ +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:
Quoted 6 lines
* 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.
Quoted 6 lines
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_devicepassed 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.
PATCH 2/3 ARM: bcm2835: Add the Raspberry Pi firmware driver
Quoted 45 lines · Stephen Warren
Quoted 37 lines · Eric Anholt
Quoted 28 lines · Stephen Warren
Quoted 13 lines · Eric Anholt
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.
arch/arm/mach-bcm/raspberrypi-firmware.c +7
-0+/* +* 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. arch/arm/mach-bcm/raspberrypi-firmware.c line 88
Quoted 10 lines
arch/arm/mach-bcm/raspberrypi-firmware.c +10
-0+*/ +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:
Quoted 6 lines
* 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.
Quoted 6 lines
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_devicepassed 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.
Quoted 4 lines
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>
PATCH 2/3 ARM: bcm2835: Add the Raspberry Pi firmware driver
Quoted 52 lines · Eric Anholt
Quoted 45 lines · Stephen Warren
Quoted 37 lines · Eric Anholt
Quoted 28 lines · Stephen Warren
Quoted 13 lines · Eric Anholt
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.
arch/arm/mach-bcm/raspberrypi-firmware.c +7
-0+/* +* 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. arch/arm/mach-bcm/raspberrypi-firmware.c line 88
Quoted 10 lines
arch/arm/mach-bcm/raspberrypi-firmware.c +10
-0+*/ +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:
Quoted 6 lines
* 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.
Quoted 6 lines
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_devicepassed 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.Quoted 4 lines
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.
Quoted 5 lines
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.ctegra_ahb_enable_smmu()
Consumer:drivers/iommu/tegra-smmu.ctegra_smmu_ahb_enable()
(called from tegra_smmu_probe() in the same file)
PATCH 2/3 ARM: bcm2835: Add the Raspberry Pi firmware driver
Quoted 71 lines · Stephen Warren
Quoted 52 lines · Eric Anholt
Quoted 45 lines · Stephen Warren
Quoted 37 lines · Eric Anholt
Quoted 28 lines · Stephen Warren
Quoted 13 lines · Eric Anholt
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.
arch/arm/mach-bcm/raspberrypi-firmware.c +7
-0+/* +* 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. arch/arm/mach-bcm/raspberrypi-firmware.c line 88
Quoted 10 lines
arch/arm/mach-bcm/raspberrypi-firmware.c +10
-0+*/ +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:
Quoted 6 lines
* 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.
Quoted 6 lines
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_devicepassed 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.Quoted 4 lines
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.
Quoted 5 lines
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->providerreferences 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:
Quoted 8 lines
Provider:
drivers/amba/tegra-ahb.ctegra_ahb_enable_smmu()Consumer:
drivers/iommu/tegra-smmu.ctegra_smmu_ahb_enable()
(called fromtegra_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>
PATCH 2/3 ARM: bcm2835: Add the Raspberry Pi firmware driver
Quoted 77 lines · Eric Anholt
Quoted 75 lines · Stephen Warren
Quoted 52 lines · Eric Anholt
Quoted 45 lines · Stephen Warren
Quoted 37 lines · Eric Anholt
Quoted 28 lines · Stephen Warren
Quoted 13 lines · Eric Anholt
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.
arch/arm/mach-bcm/raspberrypi-firmware.c +7
-0+/* +* 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. arch/arm/mach-bcm/raspberrypi-firmware.c
Quoted 10 lines
arch/arm/mach-bcm/raspberrypi-firmware.c +10
-0+*/ +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:
Quoted 6 lines
* 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.
Quoted 6 lines
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_devicepassed 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.Quoted 4 lines
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.
Quoted 5 lines
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->providerreferences 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.ctegra_ahb_enable_smmu()
Quoted 8 lines
Quoted 4 lines
Consumer:
drivers/iommu/tegra-smmu.ctegra_smmu_ahb_enable()
(called fromtegra_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.
Quoted 5 lines
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.
PATCH 3/3 ARM: bcm2835: Add the necessary firmware driver information to the DT
In reply to Eric AnholtThis 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).
--- arch/arm/boot/dts/bcm2835-rpi.dtsi | 13 +++++++++++++ arch/arm/boot/dts/bcm2835.dtsi | 2 +- 2 files changed, 14 insertions(+), 1 deletion(-)
arch/arm/boot/dts/bcm2835-rpi.dtsi +13 -0
@@ -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>; +};
arch/arm/boot/dts/bcm2835.dtsi +1 -1
@@ -148,7 +148,7 @@ status = "disabled"; }; - usb at 7e980000 { + usb: usb at 7e980000 { compatible = "brcm,bcm2835-usb"; reg = <0x7e980000 0x10000>; interrupts = <1 9>;
Signature · 1 line
2.1.4
PATCH 3/3 ARM: bcm2835: Add the necessary firmware driver information to the DT
Quoted 9 lines · Eric Anholt
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 <[email protected]>--- arch/arm/boot/dts/bcm2835-rpi.dtsi | 13 +++++++++++++ arch/arm/boot/dts/bcm2835.dtsi | 2 +- 2 files changed, 14 insertions(+), 1 deletion(-)
Quoted 45 lines
arch/arm/boot/dts/bcm2835-rpi.dtsi +13
-0@@ -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>; +};arch/arm/boot/dts/bcm2835.dtsi +1
-1@@ -148,7 +148,7 @@ status = "disabled"; }; - usb at 7e980000 { + usb: usb at 7e980000 { compatible = "brcm,bcm2835-usb"; reg = <0x7e980000 0x10000>; interrupts = <1 9>;
PATCH 3/3 ARM: bcm2835: Add the necessary firmware driver information to the DT
In reply to Eric Anholtarch/arm/boot/dts/bcm2835-rpi.dtsi line 64
Quoted 9 lines · Eric Anholt
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).
arch/arm/boot/dts/bcm2835-rpi.dtsi +3
-0+&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?
PATCH 3/3 ARM: bcm2835: Add the necessary firmware driver information to the DT
Quoted 7 lines · Stephen Warren
Quoted 5 lines · Eric Anholt
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).
arch/arm/boot/dts/bcm2835-rpi.dtsi +0
-0
Quoted 7 lines
arch/arm/boot/dts/bcm2835-rpi.dtsi line 64
Quoted 3 lines
+&usb {
arch/arm/boot/dts/bcm2835-rpi.dtsi +2
-0+ 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 bydrivers/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>
PATCH 3/3 ARM: bcm2835: Add the necessary firmware driver information to the DT
Quoted 17 lines · Eric Anholt
Quoted 7 lines · Stephen Warren
Quoted 5 lines · Eric Anholt
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).
arch/arm/boot/dts/bcm2835-rpi.dtsi +0
-0Quoted 7 lines
arch/arm/boot/dts/bcm2835-rpi.dtsi line 64
Quoted 3 lines
+&usb {
arch/arm/boot/dts/bcm2835-rpi.dtsi +2
-0+ 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?
Quoted 3 lines
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).