Raspberry Pi mailbox drivers
Subsystems (6)
This series can also be found at:
https://github.com/anholt/linux/tree/rpi-mailbox
The generic mailbox stuff was seen on the list last October:
http://marc.info/?l=linux-arm-kernel&m=141415695904815&w=2
which got review comments and then, as far as I can see, never got resubmitted. I think I've dealt with those comments except for the following:
- Kconfig should say "Y or M" instead of "Y"
It looks like the majority behavior in Kconfig is to just say "Y" generally, even for module things.
- Alphabetizing Makefile / Kconfig additions
In the time since the submission, everything else has been added in submission order rather than alphabetizing, so I'm following that.
- Authorship (Lubomir or Craig) on the patch
Lubomir's patch (the one being reviewed) said it was squash of Craig's onto previous work by Lubomir. Craig's patch
(http://marc.info/?l=linux-arm-kernel&m=137907209606316&w=2) looked like it was a replacement of a previous driver by Lubomir with a rewrite onto the generic mailbox support. Enough got rewritten in Craig's rework that I could easily see his name going on the patch, but I think it's a derivative of Lubomir's. However, Lubomir did go on to do some significant cleanup of Craig's work, as well. I've left Lubomir's name on the patch and made the MODULE_AUTHOR be him. If Craig is bothered by this, I'd be happy to try to work something out, but this is one of those places where a certain style of development is hard to fairly represent given the way we like history to happen in the kernel tree.
My two driver bits haven't been seen on the list before. I did read some other drivers in the process of writing them (particularly the downstream Raspberry Pi vcio.c and power.c). I've tested the property channel with some ad-hoc code to dump a few properties, and the results look good.
I haven't added a MAINTAINERS entry for this series. I'm personally happy with the maintainers being the 2835 arch maintainers, but I'd also be willing to add myself as a maintainer if that was desired. I should also be able to answer questions about the firmware behavior, which would probably be useful to anyone trying to build more drivers using this code.
A message that is not in the archive (sent off-list or not yet received): 1425329684-23968-1-git-send-email-eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org
PATCH 1/10 dt/bindings: Add binding for BCM2835 mailbox driver
From: Lubomir Rintel <[email protected]>
v2: Split into a separate patch for submitting to the devicetree list.
Consistently start node docs with a capital letter. device's address in the example shouldn't have "0x". Drop machine-specific interrupt numbers from the docs. (changes by anholt).
--- .../devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) create mode 100644 Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt
Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt new file +19 -0
@@ -0,0 +1,19 @@ +Broadcom BCM2835 VideoCore mailbox IPC + +Required properties: + +- compatible : Should be "brcm,bcm2835-mbox" +- reg : Specifies base physical address and size of the registers. +- interrupts : The interrupt number. See + bindings/interrupt-controller/brcm,bcm2835-armctrl-ic.txt +- #mbox-cells : Specifies the number of cells needed to encode a + mailbox channel. The value shall be 1. + +Example: + +mailbox: mailbox@7e00b800 { + compatible = "brcm,bcm2835-mbox"; + reg = <0x7e00b880 0x40>; + interrupts = <0 1>; + #mbox-cells = <1>; +};
Signature · 1 line
2.1.4
A message that is not in the archive (sent off-list or not yet received): 1425329684-23968-2-git-send-email-eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org
Re: [PATCH 01/10] dt/bindings: Add binding for BCM2835 mailbox driver
Quoted 3 lines · Eric Anholt
From: Lubomir Rintel <[email protected]>
v2: Split into a separate patch for submitting to the devicetree list.
When you say that you've split them, do you mean each DT doc, as I don't think this is the way to go. I'm happy to listen to other people's opinions, but I think all of the .../mailbox/brcm,bcm2835- files should be amalgamated.
Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt
Quoted 17 lines
Consistently start node docs with a capital letter. device's address in the example shouldn't have "0x". Drop machine-specific interrupt numbers from the docs. (changes by anholt).
Signed-off-by Lubomir Rintel <[email protected]>Signed-off-by Craig McGeachie <slapdau-/[email protected]>Signed-off-by Eric Anholt <[email protected]>--- .../devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) create mode 100644 Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txtDocumentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt new file +0
-0
Rename these files to conform to the current naming convention. In -next we currently have 'altera-mailbox.txt' and 'omap-mailbox.txt', so 'bcm2835-mbox.txt' seems appropriate.
Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt line 10
Quoted 11 lines
Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt new file +10
-0@@ -0,0 +1,19 @@ +Broadcom BCM2835 VideoCore mailbox IPC + +Required properties: + +- compatible : Should be "brcm,bcm2835-mbox" +- reg : Specifies base physical address and size of the registers. +- interrupts : The interrupt number. See + bindings/interrupt-controller/brcm,bcm2835-armctrl-ic.txt +- #mbox-cells : Specifies the number of cells needed to encode a + mailbox channel. The value shall be 1.
Binding documents are much easier to read if the property names and descriptions are seperated with tabs.
- compatible : Should be "brcm,bcm2835-mbox"
- reg : Specifies base physical address and size of the registers
- interrupts : The interrupt number
[See ../interrupt-controller/brcm,bcm2835-armctrl-ic.txt]
- #mbox-cells : Specifies the number of cells needed to encode a
mailbox channel. The value shall be 1
... don't you think? Also notice the consistency of no '.'s and the bracketing of the [See ...] statement.
Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt line 19
Quoted 8 lines
Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt new file +8
-0+Example: + +mailbox: mailbox@7e00b800 { + compatible = "brcm,bcm2835-mbox"; + reg = <0x7e00b880 0x40>; + interrupts = <0 1>; + #mbox-cells = <1>; +};
It would be good to see the client examples here as well. Please consider pulling in brcm,bcm2835-mbox-power.txt and brcm,bcm2835-mbox-property.txt. --
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/10] dt/bindings: Add binding for BCM2835 mailbox driver
Quoted 10 lines · Lee Jones
Quoted 3 lines · Eric Anholt
From: Lubomir Rintel <[email protected]>
v2: Split into a separate patch for submitting to the devicetree list.
When you say that you've split them, do you mean each DT doc, as I don't think this is the way to go. I'm happy to listen to other people's opinions, but I think all of the .../mailbox/brcm,bcm2835- files should be amalgamated.
I just meant that I split this patch out of the patch that followed (because of review feeback and Documentation/devicetree/bindings/submitting-patches.txt rules).
Quoted 5 lines
Quoted 5 lines
--- .../devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) create mode 100644 Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt
Quoted 9 lines
Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt
Quoted 5 lines
Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt new file +0
-0Rename these files to conform to the current naming convention. In -next we currently have 'altera-mailbox.txt' and 'omap-mailbox.txt', so 'bcm2835-mbox.txt' seems appropriate.
Will do.
Quoted 18 lines
Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt line 10
Quoted 11 lines
Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt new file +10
-0@@ -0,0 +1,19 @@ +Broadcom BCM2835 VideoCore mailbox IPC + +Required properties: + +- compatible : Should be "brcm,bcm2835-mbox" +- reg : Specifies base physical address and size of the registers. +- interrupts : The interrupt number. See + bindings/interrupt-controller/brcm,bcm2835-armctrl-ic.txt +- #mbox-cells : Specifies the number of cells needed to encode a + mailbox channel. The value shall be 1.Binding documents are much easier to read if the property names and descriptions are seperated with tabs.
- compatible : Should be "brcm,bcm2835-mbox"
- reg : Specifies base physical address and size of the registers
- interrupts : The interrupt number
Quoted 6 lines
[See ../interrupt-controller/brcm,bcm2835-armctrl-ic.txt]- #mbox-cells : Specifies the number of cells needed to encode a
mailbox channel. The value shall be 1... don't you think? Also notice the consistency of no '.'s and the bracketing of the [See ...] statement.
The tree seems inconsistent on formatting of these files, but I like your suggestion for nicer formatting so I'll do it.
Quoted 11 lines
Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt line 19
Quoted 8 lines
Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt new file +8
-0+Example: + +mailbox: mailbox@7e00b800 { + compatible = "brcm,bcm2835-mbox"; + reg = <0x7e00b880 0x40>; + interrupts = <0 1>; + #mbox-cells = <1>; +};It would be good to see the client examples here as well. Please consider pulling in brcm,bcm2835-mbox-power.txt and brcm,bcm2835-mbox-property.txt.
Oh, so have those two just smashed into this file as one set of documentation for everything to do with mailbox on bcm2835? That seems good to me. When I was adding the client drivers, the fact that the other brcm file was named after the compatible string made me generate new files under then new compatible strings, but the other drivers already in the tree obviously aren't formatted that way.
A message that is not in the archive (sent off-list or not yet received): 87vbih98za.fsf-omZaPlIz5HhaEpDpdNBo/[email protected]
Re: [PATCH 01/10] dt/bindings: Add binding for BCM2835 mailbox driver
Quoted 8 lines · Eric Anholt
Quoted 6 lines · Lee Jones
Quoted 4 lines · Eric Anholt
From: Lubomir Rintel <[email protected]>
v2: Split into a separate patch for submitting to the devicetree list.
...
Quoted 1 line
Quoted 1 line
Quoted 1 line
---
Generally, the changelog should go below the --- since most people don't want to see the changelog committed into the source.
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
.../devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt| 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) create mode 100644Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt
diff --git a/Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt b/Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
new file mode 100644 index 0000000..f5741a0 ---
/dev/null+++b/Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt
Rename these files to conform to the current naming convention. In
Quoted 4 lines
Quoted 2 lines
-next we currently have 'altera-mailbox.txt' and 'omap-mailbox.txt', so 'bcm2835-mbox.txt' seems appropriate.
Will do.
I believe all the current bcm2835 bindings use the compatible value as the filename. I personally prefer this to picking a different "random" name for the filenames. It means you only have to name the thing once, and then use the same value for the compatible property and binding document.
Quoted 8 lines
Quoted 7 lines
Quoted 3 lines
+Example: + +mailbox: mailbox@7e00b800 { + compatible =
"brcm,bcm2835-mbox"; + reg = <0x7e00b8800x40>; + interrupts =
<0 1>; + #mbox-cells = <1>; +};It would be good to see the client examples here as well. Please consider pulling in brcm,bcm2835-mbox-power.txt and brcm,bcm2835-mbox-property.txt.
Quoted 7 lines
Oh, so have those two just smashed into this file as one set of documentation for everything to do with mailbox on bcm2835? That seems good to me. When I was adding the client drivers, the fact that the other brcm file was named after the compatible string made me generate new files under then new compatible strings, but the other drivers already in the tree obviously aren't formatted that way.
The HW mailbox seems like a different process to the upper-layer protocols/message formats running over the top of it. Sure right now the Pi has a single firmware, but do all bcm2835-based devices share the same firmware? Is so, we'd be warranted in lumping the HW and firmware protocol together, but I rather wonder whether e.g. the bcm2835-based Roku uses the same firmware protocol?
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): [email protected]
Re: [PATCH 01/10] dt/bindings: Add binding for BCM2835 mailbox driver
Quoted 7 lines · Stephen Warren
Quoted 6 lines · Eric Anholt
Quoted 4 lines · Lee Jones
Quoted 2 lines · Eric Anholt
From: Lubomir Rintel <[email protected]>
Quoted 7 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
v2: Split into a separate patch for submitting to the devicetree list.
...
Quoted 1 line
Quoted 1 line
Quoted 1 line
---
Generally, the changelog should go below the --- since most people don't want to see the changelog committed into the source.
Huh. I'm coming from drivers/gpu/drm/ (and non-kernel communities) where people are expected to keep changelog in the commit message. I'll strip it out of this series.
Quoted 21 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
.../devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt| 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) create mode 100644Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txtdiff --git
a/Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txtb/Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txtQuoted 6 lines
Quoted 6 lines
Quoted 6 lines
new file mode 100644 index 0000000..f5741a0 ---
/dev/null+++b/Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txtRename these files to conform to the current naming convention. In
Quoted 3 lines
Quoted 2 lines
-next we currently have 'altera-mailbox.txt' and 'omap-mailbox.txt', so 'bcm2835-mbox.txt' seems appropriate.
Quoted 7 lines
Quoted 1 line
Will do.
I believe all the current bcm2835 bindings use the compatible value as the filename. I personally prefer this to picking a different "random" name for the filenames. It means you only have to name the thing once, and then use the same value for the compatible property and binding document.
"git grep brcm | grep 2835" thinks that we're quite confused on whether you put the "brcm," in the filename or not. And then there's some that don't follow either convention:
watchdog/brcm,bcm2835-pm-wdog.txt: compatible = "brcm,bcm2835-pm-wdt";pwm/pwm-bcm2835.txt: compatible = "brcm,bcm2835-pwm";rng/brcm,bcm2835.txt: compatible = "brcm,bcm2835-rng";
Quoted 16 lines
Quoted 8 lines
Quoted 7 lines
Quoted 3 lines
+Example: + +mailbox: mailbox@7e00b800 { + compatible =
"brcm,bcm2835-mbox"; + reg = <0x7e00b8800x40>; + interrupts =
<0 1>; + #mbox-cells = <1>; +};It would be good to see the client examples here as well. Please consider pulling in brcm,bcm2835-mbox-power.txt and brcm,bcm2835-mbox-property.txt.
Quoted 7 lines
Oh, so have those two just smashed into this file as one set of documentation for everything to do with mailbox on bcm2835? That seems good to me. When I was adding the client drivers, the fact that the other brcm file was named after the compatible string made me generate new files under then new compatible strings, but the other drivers already in the tree obviously aren't formatted that way.
Quoted 6 lines
The HW mailbox seems like a different process to the upper-layer protocols/message formats running over the top of it. Sure right now the Pi has a single firmware, but do all bcm2835-based devices share the same firmware? Is so, we'd be warranted in lumping the HW and firmware protocol together, but I rather wonder whether e.g. the bcm2835-based Roku uses the same firmware protocol?
I don't know the answer to that one, and I've only got RPi firmware myself.
Ultimately, my hope is to get enough merged that we can basically get off the firmware, other than the bits of clock management that are only doable from the VPU.
Re: [PATCH 01/10] dt/bindings: Add binding for BCM2835 mailbox driver
In reply to a message not in the archiveQuoted 20 lines · Stephen Warren
Quoted 11 lines · Eric Anholt
Quoted 8 lines · Lee Jones
Quoted 3 lines · Eric Anholt
+Example: + +mailbox: mailbox@7e00b800 { + compatible =
"brcm,bcm2835-mbox"; + reg = <0x7e00b8800x40>; + interrupts =
<0 1>; + #mbox-cells = <1>; +};It would be good to see the client examples here as well. Please consider pulling in brcm,bcm2835-mbox-power.txt and brcm,bcm2835-mbox-property.txt.
Quoted 7 lines
Oh, so have those two just smashed into this file as one set of documentation for everything to do with mailbox on bcm2835? That seems good to me. When I was adding the client drivers, the fact that the other brcm file was named after the compatible string made me generate new files under then new compatible strings, but the other drivers already in the tree obviously aren't formatted that way.
Quoted 6 lines
The HW mailbox seems like a different process to the upper-layer protocols/message formats running over the top of it. Sure right now the Pi has a single firmware, but do all bcm2835-based devices share the same firmware? Is so, we'd be warranted in lumping the HW and firmware protocol together, but I rather wonder whether e.g. the bcm2835-based Roku uses the same firmware protocol?
I've confirmed: While their firmware would have been forked from the same source, there's no reason to expect their shipped firmware protocols (like my power domain or property channel drivers use) to match RPi's.
The power-domain rework ended up not working out -- it needs the power-domain part of device/base to support throwing -EPROBE_DEFER if the driver hasn't probed yet, unless we're willing to just bake in the power domain driver in static init ordering. Device base maintainers weren't excited about my patch for -EPROBE_DEFER, because then a new DT would mean we start failing to probe the USB driver in an older kernel, which whould be a regression in the case that the user had U-Boot setting up USB for them.
Meanwhile, it looks like the thing being done in the firmware for USB is not just a power domain, but also configuration of the USB PHY. This led me to wonder if I could just write native support for the PHY and the power domains instead of asking the firmware to do it, but this is going to take more investigation.
As a result, I'm going to shelve my power domain and property channel work for the moment, and just submit the core mailbox support that I feel pretty confident about for now.
A message that is not in the archive (sent off-list or not yet received): 87egothkaf.fsf-omZaPlIz5HhaEpDpdNBo/[email protected]
Re: [PATCH 01/10] dt/bindings: Add binding for BCM2835 mailbox driver
On 03/12/2015 05:23 PM, Eric Anholt wrote:
...
Quoted 3 lines
The power-domain rework ended up not working out -- it needs the power-domain part of device/base to support throwing -EPROBE_DEFER if the driver hasn't probed yet,
That makese sense.
Quoted 2 lines
unless we're willing to just bake in the power domain driver in static init ordering.
Indeed, that's quite unlikely to be acceptable.
Quoted 5 lines
Device base maintainers weren't excited about my patch for -EPROBE_DEFER, because then a new DT would mean we start failing to probe the USB driver in an older kernel, which whould be a regression in the case that the user had U-Boot setting up USB for them.
The main ABI issue is that old DTs should work with new kernels. The other way around is nice, but certainly not as strict a requirement. I don't think that should block a change. Do you have a link to the thread; I don't think I noticed it.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): [email protected]
Re: [PATCH 01/10] dt/bindings: Add binding for BCM2835 mailbox driver
Quoted 11 lines · Stephen Warren
Quoted 5 lines · Eric Anholt
Device base maintainers weren't excited about my patch for -EPROBE_DEFER, because then a new DT would mean we start failing to probe the USB driver in an older kernel, which whould be a regression in the case that the user had U-Boot setting up USB for them.
The main ABI issue is that old DTs should work with new kernels. The other way around is nice, but certainly not as strict a requirement. I don't think that should block a change. Do you have a link to the thread; I don't think I noticed it.
A message that is not in the archive (sent off-list or not yet received): 87vbhzl9t1.fsf-omZaPlIz5HhaEpDpdNBo/[email protected]
Re: [PATCH 01/10] dt/bindings: Add binding for BCM2835 mailbox driver
Quoted 9 lines · Eric Anholt
Quoted 7 lines · Stephen Warren
Quoted 5 lines · Eric Anholt
Device base maintainers weren't excited about my patch for -EPROBE_DEFER, because then a new DT would mean we start failing to probe the USB driver in an older kernel, which whould be a regression in the case that the user had U-Boot setting up USB for them.
Quoted 6 lines
Quoted 4 lines
The main ABI issue is that old DTs should work with new kernels. The other way around is nice, but certainly not as strict a requirement. I don't think that should block a change. Do you have a link to the thread; I don't think I noticed it.
Hmm. I think that in general, the Linux kernel has already chosen deferred probe as the ordering mechanism over fixed init levels, which have demonstrably been unable to solve all ordering problems in the past. As Kevin pointed out, new DT with old kernel isn't the most important use-case. The only other objection I saw was Ulf not wanting to introduce another "probe scenario", but I guess he changed his mind since he outlines the steps to make deferred probe work in a later email.
I guess/hope the way forward is to either wait for PM domains to work with deferred probe, or push that work forward (i.e. implement part
(1) mentioned in Ulf's Mar 16 email).
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH 3/10 ARM: bcm2835: Add the mailbox to the device tree.
In reply to a message not in the archive--- arch/arm/boot/dts/bcm2835.dtsi | 7 +++++++ 1 file changed, 7 insertions(+)
arch/arm/boot/dts/bcm2835.dtsi +7 -0
@@ -60,6 +60,13 @@ reg = <0x7e104000 0x10>; }; + mailbox: mailbox@7e00b800 { + compatible = "brcm,bcm2835-mbox"; + reg = <0x7e00b880 0x40>; + interrupts = <0 1>; + #mbox-cells = <1>; + }; + gpio: gpio@7e200000 { compatible = "brcm,bcm2835-gpio"; reg = <0x7e200000 0xb4>;
Signature · 1 line
2.1.4
A message that is not in the archive (sent off-list or not yet received): 1425329684-23968-4-git-send-email-eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org
Lee Jones
Acked-byRe: [PATCH 03/10] ARM: bcm2835: Add the mailbox to the device tree.
Quoted 4 lines · Eric Anholt
Signed-off-by Eric Anholt <[email protected]>--- arch/arm/boot/dts/bcm2835.dtsi | 7 +++++++ 1 file changed, 7 insertions(+)
Quoted 8 lines
arch/arm/boot/dts/bcm2835.dtsi +0
-0@@ -60,6 +60,13 @@ reg = <0x7e104000 0x10>; };
arch/arm/boot/dts/bcm2835.dtsi line 72
Quoted 10 lines
arch/arm/boot/dts/bcm2835.dtsi +7
-0+ mailbox: mailbox@7e00b800 { + compatible = "brcm,bcm2835-mbox"; + reg = <0x7e00b880 0x40>; + interrupts = <0 1>; + #mbox-cells = <1>; + }; + gpio: gpio@7e200000 { compatible = "brcm,bcm2835-gpio"; reg = <0x7e200000 0xb4>;
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH 4/10 dt/bindings: Add binding for BCM2835 mailbox power channel driver
In reply to a message not in the archiveI was tempted to have the mailbox power channel support just be in the 2835 mailbox driver itself, but mbox_request_channel() wants its device to have the "mboxes" node, and that appears to be only intended for mailbox clients, not controllers.
--- .../bindings/mailbox/brcm,bcm2835-mbox-power.txt | 14 ++++++++++++++ 1 file changed, 14 insertions(+) create mode 100644 Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox-power.txt
Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox-power.txt new file +14 -0
@@ -0,0 +1,14 @@ +Broadcom BCM2835 VideoCore mailbox power channel IPC + +Required properties: + +- compatible : Should be "brcm,bcm2835-mbox-power" +- mboxes: Single-entry list which specifies which mailbox controller + and channel is the power channel. + +Example: + +mailbox-power { + compatible = "brcm,bcm2835-mbox-power"; + mboxes = <&mailbox 0> +};
Signature · 6 lines
2.1.4 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): 1425329684-23968-5-git-send-email-eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org
Re: [PATCH 04/10] dt/bindings: Add binding for BCM2835 mailbox power channel driver
Quoted 11 lines · Eric Anholt
I was tempted to have the mailbox power channel support just be in the 2835 mailbox driver itself, but
mbox_request_channel()wants its device to have the "mboxes" node, and that appears to be only intended for mailbox clients, not controllers.Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox-power.txt +4
-0+Broadcom BCM2835 VideoCore mailbox power channel IPC + +Required properties: +
Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox-power.txt line 14
Quoted 10 lines
Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox-power.txt +10
-0+- compatible : Should be "brcm,bcm2835-mbox-power" +- mboxes: Single-entry list which specifies which mailbox controller + and channel is the power channel. + +Example: + +mailbox-power { + compatible = "brcm,bcm2835-mbox-power"; + mboxes = <&mailbox 0> +};
I think the driver for this device should provide its services to other drivers using some standard in-kernel API (power domains, regulators?), which in turn would presumably have some subsystem-level DT binding.
So, I would expect this DT node to implement the server-side of that binding, and probably have a property like "#power-domain-cells = <1>", and the client nodes to "consume" this service via a property like "power-domain = <&mbox_power N>" where N is the ID of the client's power domain within the firmware protocol.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH 5/10 ARM: bcm2835: Add the mailbox power channel driver.
In reply to a message not in the archiveThis just enables the power to the USB controller, so that DWC2 can initialize.
The downstream tree has an interface to this channel for tracking enables from multiple clients, except it doesn't have any clients as far as I can see. For now, just make the simplest thing that gets USB working.
--- drivers/mailbox/Makefile | 1 + drivers/mailbox/bcm2835-mailbox-power.c | 127 ++++++++++++++++++++++++++++++++ 2 files changed, 128 insertions(+) create mode 100644 drivers/mailbox/bcm2835-mailbox-power.c
drivers/mailbox/Makefile +1 -0
@@ -11,3 +11,4 @@ obj-$(CONFIG_PCC) += pcc.o obj-$(CONFIG_ALTERA_MBOX) += mailbox-altera.o obj-$(CONFIG_BCM2835_MBOX) += bcm2835-mailbox.o +obj-$(CONFIG_BCM2835_MBOX) += bcm2835-mailbox-power.o
drivers/mailbox/bcm2835-mailbox-power.c new file +127 -0
127 lines not on this page · raw message
Signature · 6 lines
2.1.4 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): 1425329684-23968-6-git-send-email-eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org
Re: [PATCH 05/10] ARM: bcm2835: Add the mailbox power channel driver.
Quoted 10 lines · On Monday 02 March 2015 12:54:39 Eric Anholt wrote:
drivers/mailbox/bcm2835-mailbox-power.c new file +9
-0+/* +* Submits a set of concatenated tags to the VPU firmware through the +* mailbox power 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 bcm_mbox_power_tag_header for the per-tag structure. +*/ +static int bcm_mbox_set_power (uint32_t power_enables)
drivers/mailbox/bcm2835-mailbox-power.c line 60
Quoted 10 lines
drivers/mailbox/bcm2835-mailbox-power.c new file +10
-0+{ +int ret; + +reinit_completion (&mbox_power->c); + ret =mbox_send_message (mbox_power->chan, + (void *)(power_enables <<BCM_MBOX_DATA_SHIFT )); +if (ret >=0 ) +wait_for_completion (&mbox_power->c); + +return ret;
Please don't abuse the pointer argument to send an integer.
Instead, pass a pointer to the data argument as intended. As this is a recurring problem, we may want to add a different interface to pass fixed-length inline data, maybe
mbox_send_message_u32(struct mbox_chan *chan, u32 msg);
and possibly add a length argument to the existing mbox_send_message.
Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 05/10] ARM: bcm2835: Add the mailbox power channel driver.
Quoted 24 lines · Arnd Bergmann
Quoted 10 lines · On Monday 02 March 2015 12:54:39 Eric Anholt wrote:
drivers/mailbox/bcm2835-mailbox-power.c new file +9
-0+/* +* Submits a set of concatenated tags to the VPU firmware through the +* mailbox power 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 bcm_mbox_power_tag_header for the per-tag structure. +*/ +static int bcm_mbox_set_power (uint32_t power_enables)drivers/mailbox/bcm2835-mailbox-power.c line 60
Quoted 10 lines
drivers/mailbox/bcm2835-mailbox-power.c new file +10
-0+{ +int ret; + +reinit_completion (&mbox_power->c); + ret =mbox_send_message (mbox_power->chan, + (void *)(power_enables <<BCM_MBOX_DATA_SHIFT )); +if (ret >=0 ) +wait_for_completion (&mbox_power->c); + +return ret;Please don't abuse the pointer argument to send an integer.
Quoted 7 lines
Instead, pass a pointer to the data argument as intended. As this is a recurring problem, we may want to add a different interface to pass fixed-length inline data, maybe
mbox_send_message_u32(struct mbox_chan *chan, u32 msg);and possibly add a length argument to the existing
mbox_send_message.
Good point. I've adjusted this code to do this (and to use "msg" instead of "mssg" -- is "mssg" supposed to mean something besides an unusual abbreviation of "message"?), and since it was the same number of lines of code, I don't think it makes sense to define a new API. I suspect this awkward style just arose from following the lead of the omap driver.
Re: [PATCH 05/10] ARM: bcm2835: Add the mailbox power channel driver.
In reply to a message not in the archiveQuoted 10 lines · Eric Anholt
This just enables the power to the USB controller, so that DWC2 can initialize.
The downstream tree has an interface to this channel for tracking enables from multiple clients, except it doesn't have any clients as far as I can see. For now, just make the simplest thing that gets USB working.
drivers/mailbox/Makefile | 1 + drivers/mailbox/bcm2835-mailbox-power.c | 127 ++++++++++++++++++++++++++++++++
Along the lines of my comments on the previous patch, I would expect this driver to show up within the directory for the subsystem/API that it implements (power domains, regulators?)
drivers/mailbox/bcm2835-mailbox-power.c
Quoted 3 lines
drivers/mailbox/bcm2835-mailbox-power.c +1
-0+#define BCM_POWER_USB (1 << 3)
I'd expect that to be encoded in DT, in the manner I mentioned in reply to patch 4, so that this driver can work for arbitrary clients.
Quoted 1 line
+#define
BCM_MBOX_DATA_SHIFT4
I'd expect that to be defined in some public header that's part of patch 2, so that clients don't have to duplicate the definition.
Quoted 10 lines
drivers/mailbox/bcm2835-mailbox-power.c +10
-0+/* +* Submits a set of concatenated tags to the VPU firmware through the +* mailbox power 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 bcm_mbox_power_tag_header for the per-tag structure. +*/ +static int bcm_mbox_set_power (uint32_t power_enables) +{
drivers/mailbox/bcm2835-mailbox-power.c line 61
Quoted 10 lines
drivers/mailbox/bcm2835-mailbox-power.c +10
-0+int ret; + +reinit_completion (&mbox_power->c); + ret =mbox_send_message (mbox_power->chan, + (void *)(power_enables <<BCM_MBOX_DATA_SHIFT )); +if (ret >=0 ) +wait_for_completion (&mbox_power->c); + +return ret; +}
The comment sounds more like the property mailbox interface/channel, whereas the code appears to be using the non-property channel. In particular, the code only appears to be sending one "tag"/message, without the header or ending tag mentioned in the comment.
drivers/mailbox/bcm2835-mailbox-power.c line 71
Quoted 9 lines
drivers/mailbox/bcm2835-mailbox-power.c +9
-0+static int bcm2835_mbox_power_probe (struct platform_device *pdev) +{ +struct device *dev = &pdev->dev; +int ret =0 ; + + mbox_power =devm_kzalloc (dev,sizeof (*mbox_power), +GFP_KERNEL ); +if (!mbox_power) { +dev_err (dev,"Failed to allocate device memory\n" );
Duplicate error message.
drivers/mailbox/bcm2835-mailbox-power.c line 94
Quoted 5 lines
drivers/mailbox/bcm2835-mailbox-power.c +5
-0+/* Enable power to the USB phy. */ +if (IS_ENABLED (CONFIG_USB_DWC2 )) { +bcm_mbox_set_power (BCM_POWER_USB ); + bcm2835_mbox_power_initialized = true; + }
Hmm. Shouldn't the DWC2 driver make a call to do that?
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH 6/10 ARM: bcm2835: Add the mailbox power channel to the device tree.
In reply to a message not in the archive--- arch/arm/boot/dts/bcm2835.dtsi | 5 +++++ 1 file changed, 5 insertions(+)
arch/arm/boot/dts/bcm2835.dtsi +5 -0
@@ -67,6 +67,11 @@ #mbox-cells = <1>; }; + mailbox-power { + compatible = "brcm,bcm2835-mbox-power"; + mboxes = <&mailbox 0>; + }; + gpio: gpio@7e200000 { compatible = "brcm,bcm2835-gpio"; reg = <0x7e200000 0xb4>;
Signature · 1 line
2.1.4
PATCH 7/10 usb: Make sure that DWC2 initializes after the power channel mailbox driver.
In reply to a message not in the archiveThis gets USB working on the Raspberry Pi without relying on U-Boot to send the power message for us. Without it, you would get warnings about fifo sizes and "dwc2_core_reset() HANG! Soft Reset GRSTCTL=80000001" leading to a failed probe.
--- drivers/mailbox/bcm2835-mailbox-power.c | 5 +++++ drivers/usb/dwc2/platform.c | 20 ++++++++++++++++++++ 2 files changed, 25 insertions(+)
drivers/mailbox/bcm2835-mailbox-power.c +5 -0
@@ -31,6 +31,9 @@ struct bcm_mbox_power {uint32_t response; }; +bool bcm2835_mbox_power_initialized; +EXPORT_SYMBOL_GPL (bcm2835_mbox_power_initialized); +struct bcm_mbox_power *mbox_power;static void response_callback (struct mbox_client *cl,void *mssg)
@@ -92,6 +95,7 @@ static int bcm2835_mbox_power_probe(struct platform_device *pdev)bcm_mbox_set_power (BCM_POWER_USB ); bcm2835_mbox_power_initialized = true; } + bcm2835_mbox_power_initialized = true;return ret; }
@@ -99,6 +103,7 @@ static int bcm2835_mbox_power_probe(struct platform_device *pdev)static int bcm2835_mbox_power_remove (struct platform_device *pdev) {bcm_mbox_set_power (0 ); + bcm2835_mbox_power_initialized = false;mbox_free_channel (mbox_power->chan);
drivers/usb/dwc2/platform.c +20 -0
@@ -136,6 +136,20 @@ static const struct of_device_id dwc2_of_match_table[] = { };MODULE_DEVICE_TABLE (of, dwc2_of_match_table); +extern bool bcm2835_mbox_power_initialized; + +bool bcm2835_usb_power_enabled (void ) +{ +#ifdef CONFIG_BCM2835_MBOX +return bcm2835_mbox_power_initialized; +#else +/* Assume that somebody else has enabled the power for us +* (like U-Boot). +*/ +return true; +#endif +} +/** * dwc2_driver_probe() - Called when the DWC_otg core is bound to the DWC_otg * driver
@@ -175,6 +189,12 @@ static int dwc2_driver_probe(struct platform_device *dev) defparams.dma_desc_enable =0 ; } +/* Make sure the platform driver for enabling the power has +* initialized before we do. +*/ +if (params == ¶ms_bcm2835 && !bcm2835_usb_power_enabled ()) +return -EPROBE_DEFER ; + hsotg =devm_kzalloc (&dev->dev,sizeof (*hsotg),GFP_KERNEL );if (!hsotg)return -ENOMEM ;
Signature · 6 lines
2.1.4 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): 1425329684-23968-8-git-send-email-eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org
Re: [PATCH 07/10] usb: Make sure that DWC2 initializes after the power channel mailbox driver.
Quoted 17 lines · Eric Anholt
This gets USB working on the Raspberry Pi without relying on U-Boot to send the power message for us. Without it, you would get warnings about fifo sizes and "
dwc2_core_reset()HANG! Soft Reset GRSTCTL=80000001" leading to a failed probe.Signed-off-by Eric Anholt <[email protected]>Cc John Youn <johnyoun-HKixBCOQz3hWk0Htik3J/[email protected]>--- drivers/mailbox/bcm2835-mailbox-power.c | 5 +++++ drivers/usb/dwc2/platform.c | 20 ++++++++++++++++++++ 2 files changed, 25 insertions(+)drivers/mailbox/bcm2835-mailbox-power.c +0
-0
drivers/mailbox/bcm2835-mailbox-power.c struct bcm_mbox_power { · line 35
Quoted 5 lines
drivers/mailbox/bcm2835-mailbox-power.c +2
-0@@ -31,6 +31,9 @@ struct bcm_mbox_power {uint32_t response; }; +bool bcm2835_mbox_power_initialized; +EXPORT_SYMBOL_GPL (bcm2835_mbox_power_initialized);
This is pretty ugly. What is it used for? Is there any way you can use an API for it instead?
Quoted 24 lines
drivers/mailbox/bcm2835-mailbox-power.c +2
-0struct bcm_mbox_power *mbox_power;static void response_callback (struct mbox_client *cl,void *mssg)@@ -92,6 +95,7 @@ static int bcm2835_mbox_power_probe(struct platform_device *pdev)bcm_mbox_set_power (BCM_POWER_USB ); bcm2835_mbox_power_initialized = true; } + bcm2835_mbox_power_initialized = true;return ret; }@@ -99,6 +103,7 @@ static int bcm2835_mbox_power_probe(struct platform_device *pdev)static int bcm2835_mbox_power_remove (struct platform_device *pdev) {bcm_mbox_set_power (0 ); + bcm2835_mbox_power_initialized = false;mbox_free_channel (mbox_power->chan);drivers/usb/dwc2/platform.c +0
-0
drivers/usb/dwc2/platform.c static const struct of_device_id dwc2_of_match_table[] = { · line 139
Quoted 4 lines
drivers/usb/dwc2/platform.c +1
-0@@ -136,6 +136,20 @@ static const struct of_device_id dwc2_of_match_table[] = { };MODULE_DEVICE_TABLE (of, dwc2_of_match_table); +extern bool bcm2835_mbox_power_initialized;
Ughhh, *shudder*.
drivers/usb/dwc2/platform.c static const struct of_device_id dwc2_of_match_table[] = { · line 143
Quoted 3 lines
drivers/usb/dwc2/platform.c +3
-0+bool bcm2835_usb_power_enabled (void ) +{ +#ifdef CONFIG_BCM2835_MBOX
Please don't put #ifdeffery in *.c files. If you have to match on CONFIG_* options, please do so using inlines in a header file somewhere.
Quoted 13 lines
drivers/usb/dwc2/platform.c +9
-0+return bcm2835_mbox_power_initialized; +#else +/* Assume that somebody else has enabled the power for us +* (like U-Boot). +*/ +return true; +#endif +} +/** * dwc2_driver_probe() - Called when the DWC_otg core is bound to the DWC_otg * driver
drivers/usb/dwc2/platform.c static int dwc2_driver_probe(struct platform_device *dev) · line 194
Quoted 6 lines
drivers/usb/dwc2/platform.c +3
-0@@ -175,6 +189,12 @@ static int dwc2_driver_probe(struct platform_device *dev) defparams.dma_desc_enable =0 ; } +/* Make sure the platform driver for enabling the power has +* initialized before we do. +*/
This multi-line comment is unconventional.
See: Documentation/CodingStyle
drivers/usb/dwc2/platform.c static int dwc2_driver_probe(struct platform_device *dev) · line 196
Quoted 2 lines
drivers/usb/dwc2/platform.c +2
-0+if (params == ¶ms_bcm2835 && !bcm2835_usb_power_enabled ()) +return -EPROBE_DEFER ;
Why don't you cut out the middle man and get rid of all of these awful exports?
include/linux/platform/bcm2835.h
#ifdef CONFIG_BCM2835_MBOX
static inline bool bcm2835_is_usb_powered()
{
return bcm2835_mbox_is_usb_powered();
}
#else
static inline bool bcm2835_is_usb_powered()
{
/*
* If BCM2835 isn't using its Mailbox(es), the
* bootloader is charged with powering up USB.
*/
return true;
}#endif
drivers/usb/dwc2/platform.c static int dwc2_driver_probe(struct platform_device *dev) · line 200
Quoted 3 lines
drivers/usb/dwc2/platform.c +0
-0hsotg =devm_kzalloc (&dev->dev,sizeof (*hsotg),GFP_KERNEL );if (!hsotg)return -ENOMEM ;
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH v2 7/10 usb: Make sure that DWC2 initializes after the power channel mailbox driver.
This gets USB working on the Raspberry Pi without relying on U-Boot to send the power message for us. Without it, you would get warnings about fifo sizes and "dwc2_core_reset() HANG! Soft Reset GRSTCTL=80000001" leading to a failed probe.
v2: Make the export of the has-the-mailbox-driver-loaded bool be
through a function call defined in include/linux/platform/
---
This would be the first include file in include/linux/platform/. Is that the right place? I see a few arch/arm/mach-*/include/mach/platform.h files -- would that or arch/arm/mach-bcm/include/mach/bcm2835-platform.h possibly be the right place instead?
Also, Lee, since you mostly typed the contents of that header, should your SoB go on this?
drivers/mailbox/bcm2835-mailbox-power.c | 11 +++++++++++
drivers/usb/dwc2/platform.c | 8 ++++++++ include/linux/platform/bcm2835.h | 18 ++++++++++++++++++ 3 files changed, 37 insertions(+) create mode 100644 include/linux/platform/bcm2835.h
drivers/mailbox/bcm2835-mailbox-power.c +11 -0
@@ -19,6 +19,7 @@#include <linux/mailbox_client.h> #include <linux/module.h> #include <linux/platform_device.h> +#include <linux/platform/bcm2835.h> #define BCM_POWER_USB (1 << 3)#define BCM_MBOX_DATA_SHIFT 4
@@ -31,6 +32,14 @@ struct bcm_mbox_power {uint32_t response; }; +static bool bcm2835_mbox_power_initialized; + +bool bcm2835_mbox_is_usb_powered (void ) +{ +return bcm2835_mbox_power_initialized; +} +EXPORT_SYMBOL_GPL (bcm2835_mbox_is_usb_powered); +struct bcm_mbox_power *mbox_power;static void response_callback (struct mbox_client *cl,void *mssg)
@@ -92,6 +101,7 @@ static int bcm2835_mbox_power_probe(struct platform_device *pdev)bcm_mbox_set_power (BCM_POWER_USB ); bcm2835_mbox_power_initialized = true; } + bcm2835_mbox_power_initialized = true;return ret; }
@@ -99,6 +109,7 @@ static int bcm2835_mbox_power_probe(struct platform_device *pdev)static int bcm2835_mbox_power_remove (struct platform_device *pdev) {bcm_mbox_set_power (0 ); + bcm2835_mbox_power_initialized = false;mbox_free_channel (mbox_power->chan);
drivers/usb/dwc2/platform.c +8 -0
@@ -42,6 +42,7 @@#include <linux/of_device.h> #include <linux/mutex.h> #include <linux/platform_device.h> +#include <linux/platform/bcm2835.h> #include <linux/usb/of.h>
@@ -175,6 +176,13 @@ static int dwc2_driver_probe(struct platform_device *dev) defparams.dma_desc_enable =0 ; } +/* +* Make sure the platform driver for enabling the power has +* initialized before we do. +*/ +if (params == ¶ms_bcm2835 && !bcm2835_is_usb_powered ()) +return -EPROBE_DEFER ; + hsotg =devm_kzalloc (&dev->dev,sizeof (*hsotg),GFP_KERNEL );if (!hsotg)return -ENOMEM ;
include/linux/platform/bcm2835.h new file +18 -0
@@ -0,0 +1,18 @@ +#ifdef CONFIG_BCM2835_MBOX +bool bcm2835_mbox_is_usb_powered (void ); + +static inline bool bcm2835_is_usb_powered (void ) +{ +return bcm2835_mbox_is_usb_powered (); +} +#else +static inline bool bcm2835_is_usb_powered (void ) +{ +/* +* If BCM2835 isn't using its Mailbox driver, the bootloader +* is charged with powering up USB. +*/ +return true; +} +#endif +
Signature · 6 lines
2.1.4 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): 1425414778-30820-1-git-send-email-eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org
Re: [PATCH 07/10 v2] usb: Make sure that DWC2 initializes after the power channel mailbox driver.
Quoted 10 lines · Eric Anholt
This gets USB working on the Raspberry Pi without relying on U-Boot to send the power message for us. Without it, you would get warnings about fifo sizes and "
dwc2_core_reset()HANG! Soft Reset GRSTCTL=80000001" leading to a failed probe.v2: Make the export of the has-the-mailbox-driver-loaded bool be
through a function call defined ininclude/linux/platform/Signed-off-by Eric Anholt <[email protected]>Cc John Youn <johnyoun-HKixBCOQz3hWk0Htik3J/[email protected]>
Quoted 7 lines
---
This would be the first include file in
include/linux/platform/. Is that the right place? I see a fewarch/arm/mach-*/include/mach/platform.hfiles -- would that orarch/arm/mach-bcm/include/mach/bcm2835-platform.hpossibly be the right place instead?
include/linux/soc might be better?
Even better would be to make this work through a standard kernel subsystem API. Then, dwc2's probe would do something like:
// probe:
pd = power_domain_get(&pdev->dev, "power-domain"); if (IS_ERR(pd))
return PTR_ERR(pd);
// perhaps in probe, or in PM callbacks, etc.
ret = power_domain_on(pd);
That hypothetical function will go out to DT, find the power-domain property, look at the DT phandle in the property's value, search for a power domain provider that was instantiated from the DT node the phandle points at, and obtain a handle to it.
The benefits of this approach are:
a) Uses a standard API rather than something custom.
b) power_domain_get() will return ERR_PTR(-EPROBE_DEFERRED) if the "power domain provider (driver) instantiated from the DT node the phandle points at" has not yet probed. In turn this will prevent DWC2 from probing until the power domain can be turned on. The driver core will call DWC2's probe over and over until it returns something other than -EPROBE_DEFERRED (i.e. success, or some other failure).
drivers/usb/dwc2/platform.c static int dwc2_driver_probe(struct platform_device *dev) · line 184
Quoted 12 lines
drivers/usb/dwc2/platform.c +6
-0@@ -175,6 +176,13 @@ static int dwc2_driver_probe(struct platform_device *dev) defparams.dma_desc_enable =0 ; } +/* +* Make sure the platform driver for enabling the power has +* initialized before we do. +*/ +if (params == ¶ms_bcm2835 && !bcm2835_is_usb_powered ()) +return -EPROBE_DEFER ;
This should be something more like:
if dt_property_exists("power-domain") {
// The power_domain_get code I wrote above
}
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 07/10] usb: Make sure that DWC2 initializes after the power channel mailbox driver.
In reply to Lee JonesQuoted 8 lines · Lee Jones
Quoted 6 lines · Eric Anholt
This gets USB working on the Raspberry Pi without relying on U-Boot to send the power message for us. Without it, you would get warnings about fifo sizes and "
dwc2_core_reset()HANG! Soft Reset GRSTCTL=80000001" leading to a failed probe.drivers/usb/dwc2/platform.c +0
-0
Quoted 7 lines
drivers/usb/dwc2/platform.c static const struct of_device_id dwc2_of_match_table[] = { · line 143
Quoted 3 lines
drivers/usb/dwc2/platform.c +3
-0+bool bcm2835_usb_power_enabled (void ) +{ +#ifdef CONFIG_BCM2835_MBOX Please don't put #ifdeffery in *.c files. If you have to match on CONFIG_* options, please do so using inlines in a header file somewhere.
I haven't heard of that restriction before. I'm sure there are many ifdefs in C files in the kernel. What benefit does moving the ifdefs into headers have?
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): [email protected]
Re: [PATCH 07/10] usb: Make sure that DWC2 initializes after the power channel mailbox driver.
Quoted 17 lines · On Tuesday 03 March 2015 20:17:24 Stephen Warren wrote:
Quoted 8 lines · Lee Jones
Quoted 6 lines · Eric Anholt
This gets USB working on the Raspberry Pi without relying on U-Boot to send the power message for us. Without it, you would get warnings about fifo sizes and "
dwc2_core_reset()HANG! Soft Reset GRSTCTL=80000001" leading to a failed probe.drivers/usb/dwc2/platform.c +0
-0Quoted 7 lines
drivers/usb/dwc2/platform.c static const struct of_device_id dwc2_of_match_table[] = { · line 143
Quoted 3 lines
drivers/usb/dwc2/platform.c +3
-0+bool bcm2835_usb_power_enabled (void ) +{ +#ifdef CONFIG_BCM2835_MBOX Please don't put #ifdeffery in *.c files. If you have to match on CONFIG_* options, please do so using inlines in a header file somewhere.
Quoted 4 lines
I haven't heard of that restriction before. I'm sure there are many ifdefs in C files in the kernel. What benefit does moving the ifdefs into headers have?
It's the common convention.
However, I don't think this should depend on the specific mailbox implementation at all. The whole point of the mailbox abstraction is to shield the drivers from the details, and you can rely on the right mailbox driver being used based on the phandle that is used in the dt.
Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH 8/10 dt/bindings: Add binding for BCM2835 mailbox property channel driver
In reply to a message not in the archiveI was tempted to have the mailbox property channel support just be in the 2835 mailbox driver itself, but mbox_request_channel() wants its device to have the "mboxes" node, and that appears to be only intended for mailbox clients, not controllers.
--- .../bindings/mailbox/brcm,bcm2835-mbox-property.txt | 14 ++++++++++++++ 1 file changed, 14 insertions(+) create mode 100644 Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox-property.txt
Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox-property.txt new file +14 -0
@@ -0,0 +1,14 @@ +Broadcom BCM2835 VideoCore mailbox property channel IPC + +Required properties: + +- compatible : Should be "brcm,bcm2835-mbox-property" +- mboxes: Single-entry list which specifies which mailbox controller + and channel is the property channel. + +Example: + +mailbox-property { + compatible = "brcm,bcm2835-mbox-property"; + mboxes = <&mailbox 8> +};
Signature · 1 line
2.1.4
A message that is not in the archive (sent off-list or not yet received): 1425329684-23968-9-git-send-email-eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org
Re: [PATCH 08/10] dt/bindings: Add binding for BCM2835 mailbox property channel driver
Quoted 4 lines · Eric Anholt
I was tempted to have the mailbox property channel support just be in the 2835 mailbox driver itself, but
mbox_request_channel()wants its device to have the "mboxes" node, and that appears to be only intended for mailbox clients, not controllers.
This is more of a particular format/protocol of messages you can send over the mailbox HW than a device. I wonder if it actually makes sense to represent it in DT as a device at all?
If we do represent this as a device in DT, shouldn't it also look like a mailbox device so that other drivers (clock, display, ...) can bind to it and send messages using the mailbox API?
I might have expected to just add property support directly into the basic bcm2835 mailbox driver itself. Perhaps some attempt might be made to isolate the HW register level access in one file/driver, and expose both the regular and property mailbox protocols as a higher level that uses the low-level mailbox functionality? The concept of the lower 4 bits of mailbox data being a channel ID might belong in the higher protocol level rather than the lower HW layer?
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): [email protected]
Re: [PATCH 08/10] dt/bindings: Add binding for BCM2835 mailbox property channel driver
Quoted 6 lines · Stephen Warren
Quoted 4 lines · Eric Anholt
I was tempted to have the mailbox property channel support just be in the 2835 mailbox driver itself, but
mbox_request_channel()wants its device to have the "mboxes" node, and that appears to be only intended for mailbox clients, not controllers.
Quoted 7 lines
This is more of a particular format/protocol of messages you can send over the mailbox HW than a device. I wonder if it actually makes sense to represent it in DT as a device at all?
If we do represent this as a device in DT, shouldn't it also look like a mailbox device so that other drivers (clock, display, ...) can bind to it and send messages using the mailbox API?
I don't think it makes sense as a mailbox device. mailbox devices can only have one client per channel, while there's no reason to have that limitation on the property channel. You could imagine having the individual tags be channels, except that again there's no reason to have the one-client limitation, but more importantly the indivudual messages to the firmware are composed of N tags.
Quoted 7 lines
I might have expected to just add property support directly into the basic bcm2835 mailbox driver itself. Perhaps some attempt might be made to isolate the HW register level access in one file/driver, and expose both the regular and property mailbox protocols as a higher level that uses the low-level mailbox functionality? The concept of the lower 4 bits of mailbox data being a channel ID might belong in the higher protocol level rather than the lower HW layer?
Yes, the lower 4 bits being the channel is firmware protocol.
The reason I didn't have it in the same device as the mailbox is that you need the channel reference in order to set up a mailbox client, and I didn't think it made sense to have the mailbox device dt reference itself.
The higher-level interface I think might make sense would be a send_data replacement that took your device and mbox channel index, and a u32 of data (low 4 bits unused), and synchronously returned a u32 of response. It would do the client setup/send/wait_for_completion/teardown for you.
I'm skeptical of putting much more work into mailboxes on this platform, though. All we've got for channels are:
0: power (present in this series)
1: fb (Won't be used in Linux)
2: unused
3: VCHIQ (I've heard skepticism that the kernel community would accept this
interface)
4: unused
5: unused
6: unused
7: settings (not sure if there's anything here not exposed in properties) 8: property (present in this series)
9-15: unused
A message that is not in the archive (sent off-list or not yet received): 87wq2v6x69.fsf-omZaPlIz5HhaEpDpdNBo/[email protected]
Re: [PATCH 08/10] dt/bindings: Add binding for BCM2835 mailbox property channel driver
Quoted 17 lines · Eric Anholt
Quoted 7 lines · Stephen Warren
Quoted 5 lines · Eric Anholt
I was tempted to have the mailbox property channel support just be in the 2835 mailbox driver itself, but
mbox_request_channel()wants its device to have the "mboxes" node, and that appears to be only intended for mailbox clients, not controllers.Quoted 7 lines
This is more of a particular format/protocol of messages you can send over the mailbox HW than a device. I wonder if it actually makes sense to represent it in DT as a device at all?
If we do represent this as a device in DT, shouldn't it also look like a mailbox device so that other drivers (clock, display, ...) can bind to it and send messages using the mailbox API?
Quoted 6 lines
I don't think it makes sense as a mailbox device. mailbox devices can only have one client per channel, while there's no reason to have that limitation on the property channel. You could imagine having the individual tags be channels, except that again there's no reason to have the one-client limitation, but more importantly the indivudual messages to the firmware are composed of N tags.
My inclination would be to structure everything as:
a) A mailbox driver/device. This doesn't implement any of the protocol code. The device appears in DT, since it's real HW.
b) A RPi firmware protocol implementation. This is the only code that talks directly to the mailbox driver. This would implement support for all aspects of the RPi firmware protocol; both the non-property and property channels; they're just different aspects of the one RPi firmware.
I guess there could be a DT node that represents the existence of the RPi firmware. It would be a bit of a "virtual device" rather than an actual piece of physical HW, but since firmware is an interface that the SW interacts with, I guess a DT node makes sense.
This DT node would contain a property pointing at the physical mailbox provider, so it could send messages.
c) Client drivers (clocks, power domains, ...) All client drivers talk to (b) not (a). The DT nodes for this code might contain a property that points at (b), but the API from c->b would likely be something custom rather than the mailbox API, since the RPi firmware protocol implements something custom rather than standard.
I'd be interested to hear opinions from people much more familiar with other mailbox HW and protocol drivers. Perhaps they're all lumped together on other platforms?
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): [email protected]
Re: [PATCH 08/10] dt/bindings: Add binding for BCM2835 mailbox property channel driver
Quoted 25 lines · Stephen Warren
Quoted 17 lines · Eric Anholt
Quoted 7 lines · Stephen Warren
Quoted 5 lines · Eric Anholt
I was tempted to have the mailbox property channel support just be in the 2835 mailbox driver itself, but
mbox_request_channel()wants its device to have the "mboxes" node, and that appears to be only intended for mailbox clients, not controllers.Quoted 7 lines
This is more of a particular format/protocol of messages you can send over the mailbox HW than a device. I wonder if it actually makes sense to represent it in DT as a device at all?
If we do represent this as a device in DT, shouldn't it also look like a mailbox device so that other drivers (clock, display, ...) can bind to it and send messages using the mailbox API?
Quoted 6 lines
I don't think it makes sense as a mailbox device. mailbox devices can only have one client per channel, while there's no reason to have that limitation on the property channel. You could imagine having the individual tags be channels, except that again there's no reason to have the one-client limitation, but more importantly the indivudual messages to the firmware are composed of N tags.
Quoted 5 lines
My inclination would be to structure everything as:
a) A mailbox driver/device. This doesn't implement any of the protocol code. The device appears in DT, since it's real HW.
Exactly.
Quoted 5 lines
b) A RPi firmware protocol implementation. This is the only code that talks directly to the mailbox driver. This would implement support for all aspects of the RPi firmware protocol; both the non-property and property channels; they're just different aspects of the one RPi firmware.
Yes, I usually call that the 'server' driver - a gobal client driver that serves other client subsystems.
Quoted 8 lines
I guess there could be a DT node that represents the existence of the RPi firmware. It would be a bit of a "virtual device" rather than an actual piece of physical HW, but since firmware is an interface that the SW interacts with, I guess a DT node makes sense.
This DT node would contain a property pointing at the physical mailbox provider, so it could send messages.
Yes, as long as a device has one physical resource IRQ, register, clock I think it has the right to be in DT.
It will be a physical mailbox channel, so it qualifies.
Quoted 6 lines
c) Client drivers (clocks, power domains, ...) All client drivers talk to (b) not (a). The DT nodes for this code might contain a property that points at (b), but the API from
c->bwould likely be something custom rather than the mailbox API, since the RPi firmware protocol implements something custom rather than standard.
Yes, c<->b protocol will be Linux specific. b<->a will be pre-determined by the remote firmware.
I haven't had the time to look deeply but from quick overview:
1) bcm2835-mailbox-power.c and bcm2835-mailbox-property.c are client drivers and shouldn't reside in drivers/mailbox/
2) The bcm2835-mailbox-power should probably be a part of platform's USB support - which should try to call remote to enable power upon init. Or even better, let the 'server' driver (b) manage that mailbox as well, if it could be used for other purposes on some other platform.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH 9/10 ARM: bcm2835: Add the mailbox property channel driver.
In reply to a message not in the archiveMany of the operations with the firmware are done through this mailbox channel pair with a specific packet format. Notably, it's used for clock control, which is apparently not actually totally possible to do from the ARM side (some regs aren't addressable). I need clock control for the VC4 DRM driver, to turn on the 3D engine.
--- drivers/mailbox/Makefile | 1 + drivers/mailbox/bcm2835-mailbox-property.c | 172 +++++++++++++++++++++++ include/linux/mailbox/bcm2835-mailbox-property.h | 106 ++++++++++++++ 3 files changed, 279 insertions(+) create mode 100644 drivers/mailbox/bcm2835-mailbox-property.c create mode 100644 include/linux/mailbox/bcm2835-mailbox-property.h
drivers/mailbox/Makefile +1 -0
@@ -12,3 +12,4 @@ obj-$(CONFIG_ALTERA_MBOX) += mailbox-altera.o obj-$(CONFIG_BCM2835_MBOX) += bcm2835-mailbox.o obj-$(CONFIG_BCM2835_MBOX) += bcm2835-mailbox-power.o +obj-$(CONFIG_BCM2835_MBOX) += bcm2835-mailbox-property.o
drivers/mailbox/bcm2835-mailbox-property.c new file +172 -0
172 lines not on this page · raw message
include/linux/mailbox/bcm2835-mailbox-property.h new file +106 -0
106 lines not on this page · raw message
Signature · 7 lines
2.1.4 _______________________________________________ linux-rpi-kernel mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-rpi-kernel
A message that is not in the archive (sent off-list or not yet received): 1425329684-23968-10-git-send-email-eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org
Re: [PATCH 09/10] ARM: bcm2835: Add the mailbox property channel driver.
drivers/mailbox/bcm2835-mailbox-property.c line 82
Quoted 12 lines · Eric Anholt
Many of the operations with the firmware are done through this mailbox channel pair with a specific packet format. Notably, it's used for clock control, which is apparently not actually totally possible to do from the ARM side (some regs aren't addressable). I need clock control for the VC4 DRM driver, to turn on the 3D engine.
drivers/mailbox/bcm2835-mailbox-property.c +4
-0+int bcm_mbox_property (void *data,size_t tag_size) + buf =dma_alloc_coherent (NULL ,PAGE_ALIGN (size), &bus_addr,GFP_ATOMIC ); +if (!buf) +return -ENOMEM ;
Can't the driver (this one or the client) maintain some persistent buffer rather than allocating/freeing a new one each time. It seems like the alloc/free might introduce quite some overhead?
drivers/mailbox/bcm2835-mailbox-property.c line 90
Quoted 4 lines
drivers/mailbox/bcm2835-mailbox-property.c +4
-0+writel (size, buf); +writel (bcm_mbox_status_request, buf +4 ); +memcpy_toio (buf +8 , data, tag_size); +writel (bcm_mbox_property_end, buf + size -4 );
Since this is just a regular chunk of RAM, can't the code just use regular memory writes and memcpy()?
drivers/mailbox/bcm2835-mailbox-property.c line 124
Quoted 9 lines
drivers/mailbox/bcm2835-mailbox-property.c +9
-0+static int bcm2835_mbox_property_probe (struct platform_device *pdev) +{ +struct device *dev = &pdev->dev; +int ret =0 ; + + mbox_property =devm_kzalloc (dev,sizeof (*mbox_property), +GFP_KERNEL ); +if (!mbox_property) { +dev_err (dev,"Failed to allocate device memory\n" );
Duplicate error message.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): [email protected]
Re: [PATCH 09/10] ARM: bcm2835: Add the mailbox property channel driver.
Quoted 10 lines · Stephen Warren
Quoted 8 lines · Eric Anholt
Many of the operations with the firmware are done through this mailbox channel pair with a specific packet format. Notably, it's used for clock control, which is apparently not actually totally possible to do from the ARM side (some regs aren't addressable). I need clock control for the VC4 DRM driver, to turn on the 3D engine.
drivers/mailbox/bcm2835-mailbox-property.c +1
-0+int bcm_mbox_property (void *data,size_t tag_size)
Quoted 7 lines
drivers/mailbox/bcm2835-mailbox-property.c line 82
Quoted 3 lines
drivers/mailbox/bcm2835-mailbox-property.c +3
-0+ buf =dma_alloc_coherent (NULL ,PAGE_ALIGN (size), &bus_addr,GFP_ATOMIC ); +if (!buf) +return -ENOMEM ;Can't the driver (this one or the client) maintain some persistent buffer rather than allocating/freeing a new one each time. It seems like the alloc/free might introduce quite some overhead?
The size of the buffer is arbitrary (up to 1MB), the frequency of requessts is low, and the hardware's pretty starved for RAM. However, we're probably only ever going to see single page allocations, so the RAM cost is probably low and the allocation time is probably also correspondingly low (not like when I was trying to do 256k allocations in the vc4 driver. ouch).
Quoted 7 lines
drivers/mailbox/bcm2835-mailbox-property.c line 90
Quoted 4 lines
drivers/mailbox/bcm2835-mailbox-property.c +4
-0+writel (size, buf); +writel (bcm_mbox_status_request, buf +4 ); +memcpy_toio (buf +8 , data, tag_size); +writel (bcm_mbox_property_end, buf + size -4 );Since this is just a regular chunk of RAM, can't the code just use regular memory writes and
memcpy()?
will wmb() guarantee that the compiler won't optimize out my write to buf[0] and corresponding read from buf[0]? I was originally treating it as RAM and using volatile, but then reread the old "don't use volatile" doc.
PATCH 9/10 ARM: bcm2835: Add the mailbox property channel driver.
Quoted 22 lines · Eric Anholt
Quoted 12 lines · Stephen Warren
Quoted 10 lines · Eric Anholt
Many of the operations with the firmware are done through this mailbox channel pair with a specific packet format. Notably, it's used for clock control, which is apparently not actually totally possible to do from the ARM side (some regs aren't addressable). I need clock control for the VC4 DRM driver, to turn on the 3D engine.
(quoted diff) +0
-0
b/drivers/mailbox/bcm2835-mailbox-property.c+intbcm_mbox_property(void *data, size_t tag_size)Quoted 7 lines
Quoted 2 lines
(quoted diff) +1
-0+ buf = dma_alloc_coherent(NULL, PAGE_ALIGN(size), &bus_addr,
GFP_ATOMIC); + if (!buf) + return-ENOMEM;Can't the driver (this one or the client) maintain some persistent buffer rather than allocating/freeing a new one each time. It seems like the alloc/free might introduce quite some overhead?
Quoted 6 lines
The size of the buffer is arbitrary (up to 1MB), the frequency of requessts is low, and the hardware's pretty starved for RAM. However, we're probably only ever going to see single page allocations, so the RAM cost is probably low and the allocation time is probably also correspondingly low (not like when I was trying to do 256k allocations in the vc4 driver. ouch).
OK. Since this is an implementation detail with no effect on DT ABI, it would be easy to change later if it did turn out to be an issue.
Quoted 11 lines
Quoted 6 lines
Quoted 3 lines
(quoted diff) +1
-0+ writel(size, buf); + writel(bcm_mbox_status_request, buf +4); +
memcpy_toio(buf + 8, data, tag_size); +writel(bcm_mbox_property_end, buf + size - 4);Since this is just a regular chunk of RAM, can't the code just use regular memory writes and
memcpy()?will
wmb()guarantee that the compiler won't optimize out my write to buf[0] and corresponding read from buf[0]? I was originally treating it as RAM and using volatile, but then reread the old "don't use volatile" doc.
Yes, all of the barrier implementations boil down to one of:
#define isb(x) __asm__ __volatile__ ("mcr p15, 0, %0, c7, c5, 4" \
: : "r" (0) : "memory")
#define dsb(x) __asm__ __volatile__ ("mcr p15, 0, %0, c7, c10, 4" \
: : "r" (0) : "memory")
#define dmb(x) __asm__ __volatile__ ("mcr p15, 0, %0, c7, c10, 5" \
: : "r" (0) : "memory")The final :"memory" tells the compiler that the __asm__ statement may change memory, so the compiler isn't allowed to "cache" anything over the statement.
PATCH 10/10 ARM: bcm2835: Add the mailbox property channel to the device tree.
In reply to a message not in the archive--- arch/arm/boot/dts/bcm2835.dtsi | 5 +++++ 1 file changed, 5 insertions(+)
arch/arm/boot/dts/bcm2835.dtsi +5 -0
@@ -72,6 +72,11 @@ mboxes = <&mailbox 0>; }; + mailbox-property { + compatible = "brcm,bcm2835-mbox-property"; + mboxes = <&mailbox 8>; + }; + gpio: gpio@7e200000 { compatible = "brcm,bcm2835-gpio"; reg = <0x7e200000 0xb4>;
Signature · 1 line
2.1.4
PATCH 2/10 mailbox: Enable BCM2835 mailbox support
In reply to Eric AnholtFrom: Lubomir Rintel <[email protected]>
Implement BCM2835 mailbox support as a device registered with the general purpose mailbox framework. Implementation based on commits by Lubomir Rintel [1], Suman Anna and Jassi Brar [2] on which to base the implementation.
[1] http://lists.infradead.org/pipermail/linux-rpi-kernel/2013-April/000528.html
[2] http://lists.infradead.org/pipermail/linux-rpi-kernel/2013-May/000546.html
v2: Squashed Craig's work for review, carried over to new version of
Mailbox framework (changes by Lubomir)
v3: Fix multi-line comment style. Refer to the documentation by
filename. Only declare one MODULE_AUTHOR. Alphabetize includes. Drop some excessive dev_dbg()s (changes by anholt).
--- drivers/mailbox/Kconfig | 8 ++ drivers/mailbox/Makefile | 2 + drivers/mailbox/bcm2835-mailbox.c | 284 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 294 insertions(+) create mode 100644 drivers/mailbox/bcm2835-mailbox.c
drivers/mailbox/Kconfig +8 -0
@@ -51,4 +51,12 @@ config ALTERA_MBOX An implementation of the Altera Mailbox soft core. It is used to send message between processors. Say Y here if you want to use the Altera mailbox support. + +config BCM2835_MBOX + tristate "BCM2835 Mailbox" + depends on ARCH_BCM2835 + help + An implementation of the BCM2385 Mailbox. It is used to invoke + the services of the Videocore. Say Y here if you want to use the + BCM2835 Mailbox. endif
drivers/mailbox/Makefile +2 -0
@@ -9,3 +9,5 @@ obj-$(CONFIG_OMAP2PLUS_MBOX) += omap-mailbox.o obj-$(CONFIG_PCC) += pcc.o obj-$(CONFIG_ALTERA_MBOX) += mailbox-altera.o + +obj-$(CONFIG_BCM2835_MBOX) += bcm2835-mailbox.o
drivers/mailbox/bcm2835-mailbox.c new file +284 -0
284 lines not on this page · raw message
Signature · 1 line
2.1.4
PATCH 2/10 mailbox: Enable BCM2835 mailbox support
Quoted 14 lines · Eric Anholt
From: Lubomir Rintel <[email protected]>
Implement BCM2835 mailbox support as a device registered with the general purpose mailbox framework. Implementation based on commits by Lubomir Rintel [1], Suman Anna and Jassi Brar [2] on which to base the implementation.
drivers/mailbox/bcm2835-mailbox.c +5
-0+/* Mailboxes */ +#define ARM_0_MAIL0 0x00 +#define ARM_0_MAIL1 0x20 + +/*
drivers/mailbox/bcm2835-mailbox.c line 51
Quoted 10 lines
drivers/mailbox/bcm2835-mailbox.c +10
-0+* Mailbox registers. We basically only support mailbox 0 & 1. We +* deliver to the VC in mailbox 1, it delivers to us in mailbox 0. See +* BCM2835-ARM-Peripherals.pdf section 1.3 for an explanation about +* the placement of memory barriers. +*/ +#define MAIL0_RD (ARM_0_MAIL0 + 0x00) +#define MAIL0_POL (ARM_0_MAIL0 + 0x10) +#define MAIL0_STA (ARM_0_MAIL0 + 0x18) +#define MAIL0_CNF (ARM_0_MAIL0 + 0x1C) +#define MAIL1_WRT (ARM_0_MAIL1 + 0x00)
That implies there are more mailboxes. I wonder if we should parameterize which to use via some DT properties? I guess we can defer that though; we can default to the current values and add properties later if we want to use something else.
Quoted 8 lines
drivers/mailbox/bcm2835-mailbox.c +8
-0+static irqreturn_t bcm2835_mbox_irq (int irq,void *dev_id) +{ +struct bcm2835_mbox *mbox = (struct bcm2835_mbox *) dev_id; +struct device *dev = mbox->dev; + +while (!(readl (mbox->regs +MAIL0_STA ) &ARM_MS_EMPTY )) { +u32 msg =readl (mbox->regs +MAIL0_RD ); +unsigned int chan =MBOX_CHAN (msg);
drivers/mailbox/bcm2835-mailbox.c line 103
Quoted 10 lines
drivers/mailbox/bcm2835-mailbox.c +10
-0+ +if (!mbox->channel[chan].started) { +dev_err (dev,"Reply on stopped channel %d\n" , chan); +continue ; + } +dev_dbg (dev,"Reply 0x%08X\n" , msg); +mbox_chan_received_data (mbox->channel[chan].link, + (void *)MBOX_DATA28 (msg)); + } +rmb ();/* Finished last mailbox read. */
I know the PDF mentioned in the comment earlier in the patch says to put in barriers between accesses to different peripherals, which this seems compliant with, but I don't see quite what this barrier achieves. I think the PDF is talking generalities, not imposing a rule that must be blindly followed. Besides, if there's a context-switch you can't actually implement the rules the PDF suggests. What read operation is this barrier attempting to ensure happens after reading all mailbox messages and any associated DRAM buffer?
If any barrier is needed, shouldn't it be between the mailbox read and the processing of that message (which could at least in some cases read an SDRAM buffer). So, the producer would do roughly:
p1) Fill in DRAM buffer
p2) Write memory barrier so the MBOX write happens after the above p3) Send mbox message to tell the consumer to process the buffer
... and the consumer:
c1) Read MBOX register to know which DRAM buffer to handle c2) rmb() to make sure we read from the DRAM buffer after the MBOX read c3) Read the DRAM buffer
Even then, since (c3) is data-dependent on (c1), I don't think the rmb() in (c2) there actually does anything useful.
drivers/mailbox/bcm2835-mailbox.c line 115
Quoted 9 lines
drivers/mailbox/bcm2835-mailbox.c +9
-0+static int bcm2835_send_data (struct mbox_chan *link,void *data) +{ +struct bcm2835_channel *chan =to_channel (link); +struct bcm2835_mbox *mbox = chan->mbox; +int ret =0 ; + +if (!chan->started) +return -ENODEV ; +spin_lock (&mbox->lock);
Is it safe to read chan->started without the channel lock held?
drivers/mailbox/bcm2835-mailbox.c
Quoted 2 lines
drivers/mailbox/bcm2835-mailbox.c +2
-0+if (readl (mbox->regs +MAIL0_STA ) &ARM_MS_FULL ) { +rmb ();/* Finished last mailbox read. */
This also doesn't seem useful?
drivers/mailbox/bcm2835-mailbox.c line 122
Quoted 5 lines
drivers/mailbox/bcm2835-mailbox.c +5
-0+ ret = -EBUSY ; + goto end; + } +wmb ();/* About to write to the mail box. */ +writel (MBOX_MSG (chan->chan_num, (u32 ) data), mbox->regs +MAIL1_WRT );
This one I agree with, at least if MBOX messages contain pointers to DRAM buffers.
Quoted 4 lines
drivers/mailbox/bcm2835-mailbox.c +4
-0+static int bcm2835_startup (struct mbox_chan *link) +{ +struct bcm2835_channel *chan =to_channel (link); +
drivers/mailbox/bcm2835-mailbox.c line 143
Quoted 10 lines
drivers/mailbox/bcm2835-mailbox.c +10
-0+ chan->started = true; +return 0 ; +} + +static void bcm2835_shutdown (struct mbox_chan *link) +{ +struct bcm2835_channel *chan =to_channel (link); + + chan->started = false; +}
Don't we need to hold chan->lock when adjusting chan->started? Or is start/stop intended to be asynchronous to any operations currently in progress on the channel?
drivers/mailbox/bcm2835-mailbox.c line 155
Quoted 11 lines
drivers/mailbox/bcm2835-mailbox.c +11
-0+static bool bcm2835_last_tx_done (struct mbox_chan *link) +{ +struct bcm2835_channel *chan =to_channel (link); +struct bcm2835_mbox *mbox = chan->mbox; +bool ret; + +if (!chan->started) +return false; +spin_lock (&mbox->lock); + ret = !(readl (mbox->regs +MAIL0_STA ) &ARM_MS_FULL ); +rmb ();/* Finished last mailbox read. */
That barrier doesn't seem useful?
What are the semantics of "tx done"? This seems to be testing that the TX mailbox isn't completely full, which is more about whether the consumer side is backed up rather than whether our producer-side TX operations are done.
If the idea is to wait for the consumer to have consumed everything in our TX direction, shouldn't this check for empty not !full?
drivers/mailbox/bcm2835-mailbox.c line 191
Quoted 6 lines
drivers/mailbox/bcm2835-mailbox.c +5
-0+static int request_mailbox_irq (struct bcm2835_mbox *mbox) + if (irq <= 0) { +dev_err (dev,"Can't get IRQ number for mailbox\n" ); +return -ENODEV ; + }
I expect devm_request_irq() checkes that condition.
drivers/mailbox/bcm2835-mailbox.c line 195
Quoted 4 lines
drivers/mailbox/bcm2835-mailbox.c +4
-0+ ret =devm_request_irq (dev, irq, bcm2835_mbox_irq,0 ,dev_name (dev), + mbox); +if (ret) { +dev_err (dev,"Failed to register a mailbox IRQ handler\n" );
Printing ret might be useful to know why.
Are you sure devm_request_irq() is appropriate? The IRQ handler will be unregistered *after* bcm2835_mbox_remove() is called, and I think without any guarantee re: the order that other devm_ allocations are cleaned up. If bcm2835_mbox_remove() can't guarantee that no IRQ will fire after it exits, then bcm2835_mbox_irq() might just get called after some allocations are torn down, thus causing the IRQ handler to touch free'd memory.
Both request_mailbox_iomem and request_mailbox_irq are small enough they're typically written inline into the main probe() function.
drivers/mailbox/bcm2835-mailbox.c line 210
Quoted 5 lines
drivers/mailbox/bcm2835-mailbox.c +4
-0+static int bcm2835_mbox_probe (struct platform_device *pdev) + mbox =devm_kzalloc (dev,sizeof (*mbox),GFP_KERNEL ); +if (mbox ==NULL ) { +dev_err (dev,"Failed to allocate mailbox memory\n" );
devm_kzalloc() already prints an error, so no need to add another here, even if it does nicely document the fact that you remembered error messages:-)
drivers/mailbox/bcm2835-mailbox.c line 232
Quoted 8 lines
drivers/mailbox/bcm2835-mailbox.c +8
-0+ mbox->controller.txdone_poll = true; + mbox->controller.txpoll_period =5 ; + mbox->controller.ops = &bcm2835_mbox_chan_ops; + mbox->controller.dev = dev; + mbox->controller.num_chans =MBOX_CHAN_COUNT ; + mbox->controller.chans =devm_kzalloc (dev, +sizeof (struct mbox_chan ) *MBOX_CHAN_COUNT , +GFP_KERNEL );
It'd be common to say "sizeof(*mbox->controller.chans) so the type can't mismatch what's being assigned to.
drivers/mailbox/bcm2835-mailbox.c line 234
Quoted 2 lines
drivers/mailbox/bcm2835-mailbox.c +2
-0+if (!mbox->controller.chans) { +dev_err (dev,"Failed to alloc mbox_chans\n" );
Same comment about error messages here.
drivers/mailbox/bcm2835-mailbox.c line 252
Quoted 3 lines
drivers/mailbox/bcm2835-mailbox.c +3
-0+/* Enable the interrupt on data reception */ +writel (ARM_MC_IHAVEDATAIRQEN , mbox->regs +MAIL0_CNF ); +dev_info (dev,"mailbox enabled\n" );
There's no interrupt for "TX space available"? Oh well. I guess that's why mbox->controller.txdone_poll = true.
PATCH 2/10 mailbox: Enable BCM2835 mailbox support
Quoted 9 lines · On Tuesday 03 March 2015 20:03:13 Stephen Warren wrote:
Quoted 9 lines
drivers/mailbox/bcm2835-mailbox.c new file +9
-0+ +/* +* Mailbox registers. We basically only support mailbox 0 & 1. We +* deliver to the VC in mailbox 1, it delivers to us in mailbox 0. See +* BCM2835-ARM-Peripherals.pdf section 1.3 for an explanation about +* the placement of memory barriers. +*/ +#define MAIL0_RD (ARM_0_MAIL0 + 0x00) +#define MAIL0_POL (ARM_0_MAIL0 + 0x10)
Quoted 8 lines
drivers/mailbox/bcm2835-mailbox.c line 51
Quoted 3 lines
drivers/mailbox/bcm2835-mailbox.c new file +3
-0+#define MAIL0_STA (ARM_0_MAIL0 + 0x18) +#define MAIL0_CNF (ARM_0_MAIL0 + 0x1C) +#define MAIL1_WRT (ARM_0_MAIL1 + 0x00)That implies there are more mailboxes. I wonder if we should parameterize which to use via some DT properties? I guess we can defer that though; we can default to the current values and add properties later if we want to use something else.
How about changing #mbox-cells to <2> and using the first cell to identify the mailbox and the second to identify the channel?
The binding isn't very clear on the meaning of the one argument cell for the mailbox reference, but I assume it's used for the mailbox channel rather than the mailbox id.
Arnd
PATCH 2/10 mailbox: Enable BCM2835 mailbox support
In reply to Stephen WarrenQuoted 22 lines · Stephen Warren
Quoted 21 lines · Eric Anholt
From: Lubomir Rintel <[email protected]>
Implement BCM2835 mailbox support as a device registered with the general purpose mailbox framework. Implementation based on commits by Lubomir Rintel [1], Suman Anna and Jassi Brar [2] on which to base the implementation.
drivers/mailbox/bcm2835-mailbox.c +12
-0+/* Mailboxes */ +#define ARM_0_MAIL0 0x00 +#define ARM_0_MAIL1 0x20 + +/* +* Mailbox registers. We basically only support mailbox 0 & 1. We +* deliver to the VC in mailbox 1, it delivers to us in mailbox 0. See +* BCM2835-ARM-Peripherals.pdf section 1.3 for an explanation about +* the placement of memory barriers. +*/ +#define MAIL0_RD (ARM_0_MAIL0 + 0x00) +#define MAIL0_POL (ARM_0_MAIL0 + 0x10)
Quoted 8 lines
drivers/mailbox/bcm2835-mailbox.c
Quoted 3 lines
drivers/mailbox/bcm2835-mailbox.c +3
-0+#define MAIL0_STA (ARM_0_MAIL0 + 0x18) +#define MAIL0_CNF (ARM_0_MAIL0 + 0x1C) +#define MAIL1_WRT (ARM_0_MAIL1 + 0x00)That implies there are more mailboxes. I wonder if we should parameterize which to use via some DT properties? I guess we can defer that though; we can default to the current values and add properties later if we want to use something else.
Yeah, until there's something to talk to in another mailbox, this seems fine.
Quoted 19 lines
Quoted 8 lines
drivers/mailbox/bcm2835-mailbox.c +8
-0+static irqreturn_t bcm2835_mbox_irq (int irq,void *dev_id) +{ +struct bcm2835_mbox *mbox = (struct bcm2835_mbox *) dev_id; +struct device *dev = mbox->dev; + +while (!(readl (mbox->regs +MAIL0_STA ) &ARM_MS_EMPTY )) { +u32 msg =readl (mbox->regs +MAIL0_RD ); +unsigned int chan =MBOX_CHAN (msg);drivers/mailbox/bcm2835-mailbox.c
Quoted 10 lines
drivers/mailbox/bcm2835-mailbox.c +10
-0+ +if (!mbox->channel[chan].started) { +dev_err (dev,"Reply on stopped channel %d\n" , chan); +continue ; + } +dev_dbg (dev,"Reply 0x%08X\n" , msg); +mbox_chan_received_data (mbox->channel[chan].link, + (void *)MBOX_DATA28 (msg)); + } +rmb ();/* Finished last mailbox read. */
Quoted 8 lines
I know the PDF mentioned in the comment earlier in the patch says to put in barriers between accesses to different peripherals, which this seems compliant with, but I don't see quite what this barrier achieves. I think the PDF is talking generalities, not imposing a rule that must be blindly followed. Besides, if there's a context-switch you can't actually implement the rules the PDF suggests. What read operation is this barrier attempting to ensure happens after reading all mailbox messages and any associated DRAM buffer?
Looking at this bit of code in particular:
"As interrupts can appear anywhere in the code so you should safeguard those. If an interrupt routine reads from a peripheral the routine should start with a memory read barrier. If an interrupt routine writes to a peripheral the routine should end with a memory write barrier."
So it seems like the IRQ handler at least wants:
drivers/mailbox/bcm2835-mailbox.c +7 -1
@@ -88,6 +88,13 @@ static irqreturn_t bcm2835_mbox_irq(int irq, void *dev_id)struct bcm2835_mbox *mbox = (struct bcm2835_mbox *) dev_id;struct device *dev = mbox->dev; +/* +* BCM2835-ARM-Peripherals.pdf says "If an interrupt routine +* reads from a peripheral the routine should start with a +* memory read barrier." +*/ +rmb (); +while (!(readl (mbox->regs +MAIL0_STA ) &ARM_MS_EMPTY )) {u32 msg =readl (mbox->regs +MAIL0_RD );unsigned int chan =MBOX_CHAN (msg);
@@ -100,7 +107,6 @@ static irqreturn_t bcm2835_mbox_irq(int irq, void *dev_id)mbox_chan_received_data (mbox->channel[chan].link, (void *)MBOX_DATA28 (msg)); } -rmb ();/* Finished last mailbox read. */ return IRQ_HANDLED ; }
Quoted 8 lines
If any barrier is needed, shouldn't it be between the mailbox read and the processing of that message (which could at least in some cases read an SDRAM buffer). So, the producer would do roughly:
p1) Fill in DRAM buffer
p2) Write memory barrier so the MBOX write happens after the above p3) Send mbox message to tell the consumer to process the buffer
Quoted 8 lines
... and the consumer:
c1) Read MBOX register to know which DRAM buffer to handle c2)
rmb()to make sure we read from the DRAM buffer after the MBOX read c3) Read the DRAM bufferEven then, since (c3) is data-dependent on (c1), I don't think the
rmb()in (c2) there actually does anything useful.
I'm not sure if this is already covered by "The GPU has special logic to cope with data arriving out-of-order", but I think I agree we should probably do it for safety.
Quoted 11 lines
drivers/mailbox/bcm2835-mailbox.c
Quoted 9 lines
drivers/mailbox/bcm2835-mailbox.c +9
-0+static int bcm2835_send_data (struct mbox_chan *link,void *data) +{ +struct bcm2835_channel *chan =to_channel (link); +struct bcm2835_mbox *mbox = chan->mbox; +int ret =0 ; + +if (!chan->started) +return -ENODEV ; +spin_lock (&mbox->lock);Is it safe to read
chan->startedwithout the channel lock held?
The channel's lock is held by our caller (msg_submit() of mailbox.c).
Quoted 4 lines
drivers/mailbox/bcm2835-mailbox.c
Quoted 2 lines
drivers/mailbox/bcm2835-mailbox.c +2
-0+if (readl (mbox->regs +MAIL0_STA ) &ARM_MS_FULL ) { +rmb ();/* Finished last mailbox read. */ This also doesn't seem useful?
This (and the next one) seems to fall under:
"You should place:
? A memory write barrier before the first write to a peripheral. ? A memory read barrier after the last read of a peripheral.
It is not required to put a memory barrier instruction after each read or write access. Only at those places in the code where it is possible that a peripheral read or write may be followed by a read or write of a different peripheral. This is normally at the entry and exit points of the peripheral service code."
Quoted 10 lines
Quoted 10 lines
drivers/mailbox/bcm2835-mailbox.c +10
-0+static int bcm2835_startup (struct mbox_chan *link) +{ +struct bcm2835_channel *chan =to_channel (link); + + chan->started = true; +return 0 ; +} + +static void bcm2835_shutdown (struct mbox_chan *link) +{
Quoted 8 lines
drivers/mailbox/bcm2835-mailbox.c
Quoted 4 lines
drivers/mailbox/bcm2835-mailbox.c +4
-0+struct bcm2835_channel *chan =to_channel (link); + + chan->started = false; +}Don't we need to hold
chan->lockwhen adjustingchan->started? Or is start/stop intended to be asynchronous to any operations currently in progress on the channel?
Only one client can be on a channel at a time, which is controlled by con_mutex at channel request time, and these hooks are when the client appears/disappears.
The started check in the irq handler is necessary so that we drop any stray mbox messages instead of NULL dereffing in mbox_chan_received_data(). I think the check in bcm2846_send_data() could just be dropped (we know we have a client if a client is trying to send a message). I haven't followed quite what bcm2835_last_tx_done() is doing.
Quoted 5 lines
Quoted 5 lines
drivers/mailbox/bcm2835-mailbox.c +5
-0+static bool bcm2835_last_tx_done (struct mbox_chan *link) +{ +struct bcm2835_channel *chan =to_channel (link); +struct bcm2835_mbox *mbox = chan->mbox; +bool ret;
Quoted 8 lines
drivers/mailbox/bcm2835-mailbox.c
Quoted 6 lines
drivers/mailbox/bcm2835-mailbox.c +6
-0+ +if (!chan->started) +return false; +spin_lock (&mbox->lock); + ret = !(readl (mbox->regs +MAIL0_STA ) &ARM_MS_FULL ); +rmb ();/* Finished last mailbox read. */ That barrier doesn't seem useful?
Same barrier comment.
Quoted 4 lines
What are the semantics of "tx done"? This seems to be testing that the TX mailbox isn't completely full, which is more about whether the consumer side is backed up rather than whether our producer-side TX operations are done.
Take a look at poll_txdone() -- it does look like we're doing the right thing here, just that the method would be better named as "ready_to_send" or something.
Quoted 8 lines
drivers/mailbox/bcm2835-mailbox.c
Quoted 6 lines
drivers/mailbox/bcm2835-mailbox.c +5
-0+static int request_mailbox_irq (struct bcm2835_mbox *mbox) + if (irq <= 0) { +dev_err (dev,"Can't get IRQ number for mailbox\n" ); +return -ENODEV ; + }I expect
devm_request_irq()checkes that condition.
Chasing things down, it looks like you'd get a silent error, but then we've already got a whine if devm_request_irq fails anyway.
Quoted 6 lines
drivers/mailbox/bcm2835-mailbox.c
Quoted 4 lines
drivers/mailbox/bcm2835-mailbox.c +4
-0+ ret =devm_request_irq (dev, irq, bcm2835_mbox_irq,0 ,dev_name (dev), + mbox); +if (ret) { +dev_err (dev,"Failed to register a mailbox IRQ handler\n" );Printing ret might be useful to know why.
Yeah.
Quoted 7 lines
Are you sure
devm_request_irq()is appropriate? The IRQ handler will be unregistered *after*bcm2835_mbox_remove()is called, and I think without any guarantee re: the order that otherdevm_allocations are cleaned up. Ifbcm2835_mbox_remove()can't guarantee that no IRQ will fire after it exits, thenbcm2835_mbox_irq()might just get called after some allocations are torn down, thus causing the IRQ handler to touch free'd memory.
It looks like we should
writel(0, mbox->regs + MAIL0_CNF);
in the unload.
Quoted 2 lines
Both
request_mailbox_iomemandrequest_mailbox_irqare small enough they're typically written inline into the mainprobe()function.
Sounds good.
Quoted 9 lines
drivers/mailbox/bcm2835-mailbox.c
Quoted 5 lines
drivers/mailbox/bcm2835-mailbox.c +4
-0+static int bcm2835_mbox_probe (struct platform_device *pdev) + mbox =devm_kzalloc (dev,sizeof (*mbox),GFP_KERNEL ); +if (mbox ==NULL ) { +dev_err (dev,"Failed to allocate mailbox memory\n" );
devm_kzalloc()already prints an error, so no need to add another here, even if it does nicely document the fact that you remembered error messages:-)
Wait, it does? I couldn't find where it would, when I was looking into the checkpatch warnings.
Quoted 11 lines
drivers/mailbox/bcm2835-mailbox.c
Quoted 8 lines
drivers/mailbox/bcm2835-mailbox.c +8
-0+ mbox->controller.txdone_poll = true; + mbox->controller.txpoll_period =5 ; + mbox->controller.ops = &bcm2835_mbox_chan_ops; + mbox->controller.dev = dev; + mbox->controller.num_chans =MBOX_CHAN_COUNT ; + mbox->controller.chans =devm_kzalloc (dev, +sizeof (struct mbox_chan ) *MBOX_CHAN_COUNT , +GFP_KERNEL );It'd be common to say "
sizeof(*mbox->controller.chans)so the type can't mismatch what's being assigned to.
Agreed.
-------------- 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/20150304/55955f1c/attachment.sig>
PATCH 2/10 mailbox: Enable BCM2835 mailbox support
Quoted 35 lines · Eric Anholt
Quoted 24 lines · Stephen Warren
Quoted 13 lines · Eric Anholt
From: Lubomir Rintel <[email protected]>
Implement BCM2835 mailbox support as a device registered with the general purpose mailbox framework. Implementation based on commits by Lubomir Rintel [1], Suman Anna and Jassi Brar [2] on which to base the implementation.
drivers/mailbox/bcm2835-mailbox.c +2
-0+static irqreturn_t bcm2835_mbox_irq(int irq, void *dev_id) +{ + struct bcm2835_mbox *mbox = (struct bcm2835_mbox *) dev_id; +struct device *dev =
mbox->dev; + + while (!(readl(mbox->regs +Quoted 8 lines
MAIL0_STA) &ARM_MS_EMPTY)) { + u32 msg = readl(mbox->regs +MAIL0_RD); + unsigned int chan =MBOX_CHAN(msg); + + if
(!mbox->channel[chan].started) { + dev_err(dev, "Reply on
stopped channel %d\n", chan); + continue; + } +dev_dbg(dev, "Reply 0x%08X\n", msg); +
mbox_chan_received_data(mbox->channel[chan].link, + (void *)MBOX_DATA28(msg)); + } +rmb(); /* Finished last mailbox read.
*/I know the PDF mentioned in the comment earlier in the patch says
Quoted 8 lines
to put in barriers between accesses to different peripherals, which this seems compliant with, but I don't see quite what this barrier achieves. I think the PDF is talking generalities, not imposing a rule that must be blindly followed. Besides, if there's a context-switch you can't actually implement the rules the PDF suggests. What read operation is this barrier attempting to ensure happens after reading all mailbox messages and any associated DRAM buffer?
Quoted 7 lines
Looking at this bit of code in particular:
"As interrupts can appear anywhere in the code so you should safeguard those. If an interrupt routine reads from a peripheral the routine should start with a memory read barrier. If an interrupt routine writes to a peripheral the routine should end with a memory write barrier."
I can see that doing that would be safe, but I don't think following those rules is actually necessary in the general case. Following those rules would cause lots of unnecessary barriers in code.
Barriers shouldn't be used arbitrarily at the start/end of ISRs, but rather at specific chosen locations in the code that actually need to enforce some kind of memory ordering.
For example, if the CPU writes a buffer to RAM, then programs a DMA peripheral to read from that RAM buffer, you'd need to make sure the RAM writes were visible to the DMA controller before kicking it off. That's often achieved by wmb() between the last write to the memory buffer (and perhaps some cache management) and the write to the DMA controller to kick off the DMA read.
Not all ISRs will have any kind of interaction between DMA peripherals and peripheral registers. For example, an I2C driver that only gets data into/out-of the I2C controller HW via CPU PIO register reads/writes rather than via DMA to/from RAM likely wouldn't ever need any barriers.
Quoted 17 lines
So it seems like the IRQ handler at least wants:
(quoted diff) +0
-0
b/drivers/mailbox/bcm2835-mailbox.cindex 604beb7..7e528f6 100644 ---a/drivers/mailbox/bcm2835-mailbox.c+++b/drivers/mailbox/bcm2835-mailbox.c@@ -88,6 +88,13 @@ staticirqreturn_tbcm2835_mbox_irq(int irq, void *dev_id)structbcm2835_mbox*mbox = (structbcm2835_mbox*)dev_id; struct device *dev =mbox->dev;(quoted diff) +1
-0+ /* + * BCM2835-ARM-Peripherals.pdf says "If an interruptroutine + * reads from a peripheral the routine should start with
a + * memory read barrier." + */ +rmb(); + while
(!(readl(mbox->regs +MAIL0_STA) &ARM_MS_EMPTY)) { u32 msg =readl(mbox->regs + MAIL0_RD); unsigned int chan =MBOX_CHAN(msg);(quoted diff) +0
-0@@ -100,7 +107,6 @@ static irqreturn_t bcm2835_mbox_irq(int irq, void *dev_id) mbox_chan_received_data(mbox->channel[chan].link,
Quoted 2 lines
(void *)
MBOX_DATA28(msg)); } -rmb(); /* Finished last mailbox
read. */ returnIRQ_HANDLED; }
In this case, I don't think neither the original barrier is needed, nor the extra one you added.
PATCH 2/10 mailbox: Enable BCM2835 mailbox support
Quoted 44 lines · Stephen Warren
Quoted 35 lines · Eric Anholt
Quoted 24 lines · Stephen Warren
Quoted 13 lines · Eric Anholt
From: Lubomir Rintel <[email protected]>
Implement BCM2835 mailbox support as a device registered with the general purpose mailbox framework. Implementation based on commits by Lubomir Rintel [1], Suman Anna and Jassi Brar [2] on which to base the implementation.
drivers/mailbox/bcm2835-mailbox.c +2
-0+static irqreturn_t bcm2835_mbox_irq(int irq, void *dev_id) +{ + struct bcm2835_mbox *mbox = (struct bcm2835_mbox *) dev_id; +struct device *dev =
mbox->dev; + + while (!(readl(mbox->regs +Quoted 8 lines
MAIL0_STA) &ARM_MS_EMPTY)) { + u32 msg = readl(mbox->regs +MAIL0_RD); + unsigned int chan =MBOX_CHAN(msg); + + if
(!mbox->channel[chan].started) { + dev_err(dev, "Reply on
stopped channel %d\n", chan); + continue; + } +dev_dbg(dev, "Reply 0x%08X\n", msg); +
mbox_chan_received_data(mbox->channel[chan].link, + (void *)MBOX_DATA28(msg)); + } +rmb(); /* Finished last mailbox read.
*/I know the PDF mentioned in the comment earlier in the patch says
Quoted 8 lines
to put in barriers between accesses to different peripherals, which this seems compliant with, but I don't see quite what this barrier achieves. I think the PDF is talking generalities, not imposing a rule that must be blindly followed. Besides, if there's a context-switch you can't actually implement the rules the PDF suggests. What read operation is this barrier attempting to ensure happens after reading all mailbox messages and any associated DRAM buffer?
Quoted 7 lines
Looking at this bit of code in particular:
"As interrupts can appear anywhere in the code so you should safeguard those. If an interrupt routine reads from a peripheral the routine should start with a memory read barrier. If an interrupt routine writes to a peripheral the routine should end with a memory write barrier."
Quoted 7 lines
I can see that doing that would be safe, but I don't think following those rules is actually necessary in the general case. Following those rules would cause lots of unnecessary barriers in code.
Barriers shouldn't be used arbitrarily at the start/end of ISRs, but rather at specific chosen locations in the code that actually need to enforce some kind of memory ordering.
According to the architecture docs, if you don't RMB at the start of your ISR, then the following timeline could get the wrong values:
some other device driver our isr
------------------------ -------rmb()
read from device
read from device examine value read exit isr
examine value raed.
The ISR could get the device driver's value. This is made explicit in footnote 2 on page 7.
Quoted 10 lines
Quoted 10 lines
So it seems like the IRQ handler at least wants:
(quoted diff) +0
-0
b/drivers/mailbox/bcm2835-mailbox.cindex 604beb7..7e528f6 100644 ---a/drivers/mailbox/bcm2835-mailbox.c+++b/drivers/mailbox/bcm2835-mailbox.c@@ -88,6 +88,13 @@ staticirqreturn_tbcm2835_mbox_irq(int irq, void *dev_id)structbcm2835_mbox*mbox = (structbcm2835_mbox*)dev_id; struct device *dev =mbox->dev;
Quoted 12 lines
Quoted 9 lines
(quoted diff) +1
-0+ /* + * BCM2835-ARM-Peripherals.pdf says "If an interruptroutine + * reads from a peripheral the routine should start with
a + * memory read barrier." + */ +rmb(); + while
(!(readl(mbox->regs +MAIL0_STA) &ARM_MS_EMPTY)) { u32 msg =readl(mbox->regs + MAIL0_RD); unsigned int chan =MBOX_CHAN(msg);(quoted diff) +0
-0@@ -100,7 +107,6 @@ static irqreturn_t bcm2835_mbox_irq(int irq, void *dev_id) mbox_chan_received_data(mbox->channel[chan].link,(void *)
MBOX_DATA28(msg)); } -rmb(); /* Finished last mailbox
read. */ returnIRQ_HANDLED; }In this case, I don't think neither the original barrier is needed, nor the extra one you added.
Your formatting got completely destroyed here.
-------------- 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/20150306/6a66dddf/attachment.sig>
PATCH 2/10 mailbox: Enable BCM2835 mailbox support
Quoted 50 lines · Eric Anholt
Quoted 48 lines · Stephen Warren
Quoted 35 lines · Eric Anholt
Quoted 24 lines · Stephen Warren
Quoted 13 lines · Eric Anholt
From: Lubomir Rintel <[email protected]>
Implement BCM2835 mailbox support as a device registered with the general purpose mailbox framework. Implementation based on commits by Lubomir Rintel [1], Suman Anna and Jassi Brar [2] on which to base the implementation.
drivers/mailbox/bcm2835-mailbox.c +2
-0+static irqreturn_t bcm2835_mbox_irq(int irq, void *dev_id) +{ + struct bcm2835_mbox *mbox = (struct bcm2835_mbox *) dev_id; +struct device *dev =
mbox->dev; + + while (!(readl(mbox->regs +Quoted 8 lines
MAIL0_STA) &ARM_MS_EMPTY)) { + u32 msg = readl(mbox->regs +MAIL0_RD); + unsigned int chan =MBOX_CHAN(msg); + + if
(!mbox->channel[chan].started) { + dev_err(dev, "Reply on
stopped channel %d\n", chan); + continue; + } +dev_dbg(dev, "Reply 0x%08X\n", msg); +
mbox_chan_received_data(mbox->channel[chan].link, + (void *)MBOX_DATA28(msg)); + } +rmb(); /* Finished last mailbox read.
*/I know the PDF mentioned in the comment earlier in the patch says
Quoted 8 lines
to put in barriers between accesses to different peripherals, which this seems compliant with, but I don't see quite what this barrier achieves. I think the PDF is talking generalities, not imposing a rule that must be blindly followed. Besides, if there's a context-switch you can't actually implement the rules the PDF suggests. What read operation is this barrier attempting to ensure happens after reading all mailbox messages and any associated DRAM buffer?
Quoted 7 lines
Looking at this bit of code in particular:
"As interrupts can appear anywhere in the code so you should safeguard those. If an interrupt routine reads from a peripheral the routine should start with a memory read barrier. If an interrupt routine writes to a peripheral the routine should end with a memory write barrier."
I can see that doing that would be safe, but I don't think following those rules is actually necessary in the general case. Following those rules would cause lots of unnecessary barriers in code.
Quoted 6 lines
Quoted 3 lines
Barriers shouldn't be used arbitrarily at the start/end of ISRs, but rather at specific chosen locations in the code that actually need to enforce some kind of memory ordering.
According to the architecture docs, if you don't RMB at the start of your ISR, then the following timeline could get the wrong values:
Which architecture doc and section/... specifically?
Quoted 11 lines
some other device driver our isr
------------------------ -------rmb()
read from device
read from device examine value read exit isr
examine value raed.The ISR could get the device driver's value. This is made explicit in footnote 2 on page 7.
Are you saying that the "read from device" in the right -hand "our isr" column could end up returning the value actually read during "read from device" in the left-hand "some other device driver" column, or vice-versa?
That doesn't make any sense.
Barriers are about ensuring that accesses happen (are visible or complete) in the desired order, not about ensuring that the results of two unrelated reads don't get swapped.
PATCH 2/10 mailbox: Enable BCM2835 mailbox support
Quoted 52 lines · Stephen Warren
Quoted 51 lines · Eric Anholt
Quoted 49 lines · Stephen Warren
Quoted 36 lines · Eric Anholt
Quoted 24 lines · Stephen Warren
Quoted 14 lines · Eric Anholt
From: Lubomir Rintel <[email protected]>
Implement BCM2835 mailbox support as a device registered with the general purpose mailbox framework. Implementation based on commits by Lubomir Rintel [1], Suman Anna and Jassi Brar [2] on which to base the implementation.
drivers/mailbox/bcm2835-mailbox.c +2
-0+static irqreturn_t bcm2835_mbox_irq(int irq, void *dev_id) +{ + struct bcm2835_mbox *mbox = (struct bcm2835_mbox *) dev_id; +struct device *dev =
mbox->dev; + + while (!(readl(mbox->regs +MAIL0_STA) &ARM_MS_EMPTY)) { + u32 msg = readl(mbox->regs +Quoted 8 lines
MAIL0_RD); + unsigned int chan =MBOX_CHAN(msg); + + if
(!mbox->channel[chan].started) { + dev_err(dev, "Reply on
stopped channel %d\n", chan); + continue; + } +dev_dbg(dev, "Reply 0x%08X\n", msg); + mbox_chan_received_data(mbox->channel[chan].link, + (void *)MBOX_DATA28(msg)); + } +rmb(); /* Finished last mailbox read.
*/Quoted 9 lines
I know the PDF mentioned in the comment earlier in the patch says to put in barriers between accesses to different peripherals, which this seems compliant with, but I don't see quite what this barrier achieves. I think the PDF is talking generalities, not imposing a rule that must be blindly followed. Besides, if there's a context-switch you can't actually implement the rules the PDF suggests. What read operation is this barrier attempting to ensure happens after reading all mailbox messages and any associated DRAM buffer?
Quoted 7 lines
Looking at this bit of code in particular:
"As interrupts can appear anywhere in the code so you should safeguard those. If an interrupt routine reads from a peripheral the routine should start with a memory read barrier. If an interrupt routine writes to a peripheral the routine should end with a memory write barrier."
I can see that doing that would be safe, but I don't think following those rules is actually necessary in the general case. Following those rules would cause lots of unnecessary barriers in code.
Quoted 8 lines
Quoted 6 lines
Quoted 3 lines
Barriers shouldn't be used arbitrarily at the start/end of ISRs, but rather at specific chosen locations in the code that actually need to enforce some kind of memory ordering.
According to the architecture docs, if you don't RMB at the start of your ISR, then the following timeline could get the wrong values:
Which architecture doc and section/... specifically?
Ah, this is BCM2835-ARM-Peripherals.pdf still. I found the footnote you mentioned.
Quoted 16 lines
Quoted 11 lines
some other device driver our isr
------------------------ -------rmb()
read from device
read from device examine value read exit isr
examine value raed.The ISR could get the device driver's value. This is made explicit in footnote 2 on page 7.
Are you saying that the "read from device" in the right -hand "our isr" column could end up returning the value actually read during "read from device" in the left-hand "some other device driver" column, or vice-versa?
Quoted 5 lines
That doesn't make any sense.
Barriers are about ensuring that accesses happen (are visible or complete) in the desired order, not about ensuring that the results of two unrelated reads don't get swapped.
Ah. I've been thinking about the typical uses of barriers to ensure ordering of transactions on a standards-compliant ARM CPU.
It was pointed out on IRC that perhaps the barriers are to work around a CPU/SoC bug. In which case, if the CPU/bus really can swap results, then I can see how barriers might be required and how they might solve the problem. If this is the real reason, it sounds truly massively scary.
I think this deserves a complete description in each driver file and in the commit description for any commit that introduces these barriers. Otherwise someone else will just want to rip them out.
I'm also concerned that we need to add barriers in many more places than just that start/end of ISRs. For example, what if the ISR for the mailbox ends up calling back into some other driver that goes and touches a different HW module. Now we need to put barriers either at the start/end of that callback function, or immediately before/after we call that callback function. This has potential to need barriers in a whole bunch of places one wouldn't expect.
I sure hope this was fixed on bcm2836, or that the ordering issue appears separately within each CPU core and there's no interaction between multiple CPU cores. Otherwise, two separate pieces of code running on two separate CPU cores are going to have trouble when they start touching different peripherals.
PATCH 2/10 mailbox: Enable BCM2835 mailbox support
In reply to Stephen WarrenQuoted 22 lines · Stephen Warren
Quoted 21 lines · Eric Anholt
From: Lubomir Rintel <[email protected]>
Implement BCM2835 mailbox support as a device registered with the general purpose mailbox framework. Implementation based on commits by Lubomir Rintel [1], Suman Anna and Jassi Brar [2] on which to base the implementation.
drivers/mailbox/bcm2835-mailbox.c +12
-0+/* Mailboxes */ +#define ARM_0_MAIL0 0x00 +#define ARM_0_MAIL1 0x20 + +/* +* Mailbox registers. We basically only support mailbox 0 & 1. We +* deliver to the VC in mailbox 1, it delivers to us in mailbox 0. See +* BCM2835-ARM-Peripherals.pdf section 1.3 for an explanation about +* the placement of memory barriers. +*/ +#define MAIL0_RD (ARM_0_MAIL0 + 0x00) +#define MAIL0_POL (ARM_0_MAIL0 + 0x10)
Quoted 8 lines
drivers/mailbox/bcm2835-mailbox.c line 51
Quoted 3 lines
drivers/mailbox/bcm2835-mailbox.c +3
-0+#define MAIL0_STA (ARM_0_MAIL0 + 0x18) +#define MAIL0_CNF (ARM_0_MAIL0 + 0x1C) +#define MAIL1_WRT (ARM_0_MAIL1 + 0x00)That implies there are more mailboxes. I wonder if we should parameterize which to use via some DT properties? I guess we can defer that though; we can default to the current values and add properties later if we want to use something else.
BCM2835-ARM-Peripherals.pdf:
"Default the interrupts from doorbell 0,1 and mailbox 0 go to the ARM this means that these resources should be written by the GPU and read by the ARM. The opposite holds for doorbells 2, 3 and mailbox 1."
I don't see any references to more mailboxes than 0 and 1.
-------------- 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/20150304/0aa1d5b5/attachment.sig>