[v3,2/3] ARM: bcm281xx: Device Tree bindings for GPIO driver

Message ID 1374870345-8276-3-git-send-email-markus.mayer@linaro.org
State New
Headers show

Commit Message

Markus Mayer July 26, 2013, 8:25 p.m.
Signed-off-by: Markus Mayer <markus.mayer@linaro.org>
Reviewed-by: Christian Daudt <csd@broadcom.com>
Reviewed-by: Tim Kryger <tim.kryger@linaro.org>
Reviewed-by: Matt Porter <matt.porter@linaro.org>
---
 .../devicetree/bindings/gpio/gpio-bcm-kona.txt     |   41 ++++++++++++++++++++
 arch/arm/boot/dts/bcm11351.dtsi                    |   16 ++++++++
 2 files changed, 57 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-bcm-kona.txt

Comments

Stephen Warren July 26, 2013, 10:08 p.m. | #1
(CC'ing the DT bindings maintainers hence quoting the binding in full)

On 07/26/2013 02:25 PM, Markus Mayer wrote:

Some kind of patch description is always useful.

>  .../devicetree/bindings/gpio/gpio-bcm-kona.txt     |   41 ++++++++++++++++++++
>  arch/arm/boot/dts/bcm11351.dtsi                    |   16 ++++++++

It's more usual to put the DT binding doc change and driver change in
one patch, then "make use of that" in a later separate patch which edits
*.dts and *.dtsi.

> diff --git a/Documentation/devicetree/bindings/gpio/gpio-bcm-kona.txt b/Documentation/devicetree/bindings/gpio/gpio-bcm-kona.txt

> +Broadcom Kona Family GPIO
> +-------------------------
> +
> +This GPIO driver is used in the following Broadcom SoCs:
> +  BCM11130, BCM11140, BCM11351, BCM28145, BCM28155
> +
> +The GPIO controller only supports edge, not level triggering.

add "... of interrupts"? This might not be worth mentioning; it's
clearly spelled out in the description of the interrupt cells below.

> +Required properties:
> +  - compatible: "brcm,kona-gpio"
> +  - reg: Physical base address and length of the controller's registers.
> +  - interrupts: The interrupt outputs from the controller.

How many entries are required? I notice there's more than 1 in the
example below.

> +  - #gpio-cells: Should be <2>. The first cell is the pin number, the second
> +    cell is used to specify optional parameters:
> +    - bit 0 specifies polarity (0 for normal, 1 for inverted)
> +  - #interrupt-cells: Should be <2>. The first cell is the GPIO number.
> +    The second cell is used to specify flags:
> +    - trigger type (bits[1:0]):
> +        1 = low-to-high edge triggered.
> +        2 = high-to-low edge triggered.
> +        3 = low-to-high or high-to-low edge triggered
> +        Valid values are 1, 2, 3
> +  - gpio-controller: Marks the device node as a GPIO controller.
> +  - interrupt-controller: Marks the device node as an interrupt controller.
> +
> +Example:
> +	gpio: gpio@35003000 {
> +		compatible = "brcm,kona-gpio";
> +		reg = <0x35003000 0x800>;
> +		interrupts =
> +		       <0x0 106 0x4
> +			0x0 115 0x4
> +			0x0 114 0x4
> +			0x0 113 0x4
> +			0x0 112 0x4
> +			0x0 111 0x4>;
> +		#gpio-cells = <2>;
> +		#interrupt-cells = <2>;
> +		gpio-controller;
> +		interrupt-controller;
> +	};

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

> +	gpio: gpio@35003000 {
> +		compatible = "brcm,kona-gpio";

In order to enable any later chip-specific quirking requirements, that
compatible property should both specify the IP block and the specific
chip it's included in, so I'd expect to see something like:

		compatible = "brcm,brcm11351-gpio", "brcm,kona-gpio";
Markus Mayer July 26, 2013, 10:51 p.m. | #2
On 26 July 2013 15:08, Stephen Warren <swarren@wwwdotorg.org> wrote:
> (CC'ing the DT bindings maintainers hence quoting the binding in full)
>
> On 07/26/2013 02:25 PM, Markus Mayer wrote:
>
> Some kind of patch description is always useful.

I will add something along the lines of "GPIO device tree bindings for
the Broadcom bcm281xx family of chips."

>>  .../devicetree/bindings/gpio/gpio-bcm-kona.txt     |   41 ++++++++++++++++++++
>>  arch/arm/boot/dts/bcm11351.dtsi                    |   16 ++++++++
>
> It's more usual to put the DT binding doc change and driver change in
> one patch, then "make use of that" in a later separate patch which edits
> *.dts and *.dtsi.

Sure. I have no issue moving the gpio-bcm-kona.txt into the other
patch, together with the code.

>> diff --git a/Documentation/devicetree/bindings/gpio/gpio-bcm-kona.txt b/Documentation/devicetree/bindings/gpio/gpio-bcm-kona.txt
>
>> +Broadcom Kona Family GPIO
>> +-------------------------
>> +
>> +This GPIO driver is used in the following Broadcom SoCs:
>> +  BCM11130, BCM11140, BCM11351, BCM28145, BCM28155
>> +
>> +The GPIO controller only supports edge, not level triggering.
>
> add "... of interrupts"? This might not be worth mentioning; it's
> clearly spelled out in the description of the interrupt cells below.

Added.

>> +Required properties:
>> +  - compatible: "brcm,kona-gpio"
>> +  - reg: Physical base address and length of the controller's registers.
>> +  - interrupts: The interrupt outputs from the controller.
>
> How many entries are required? I notice there's more than 1 in the
> example below.

You only "need" the ones you want to use, i.e. at least one. The board
supports up to 6. I can mention as much in the description.

[...]

>> diff --git a/arch/arm/boot/dts/bcm11351.dtsi b/arch/arm/boot/dts/bcm11351.dtsi
>
>> +     gpio: gpio@35003000 {
>> +             compatible = "brcm,kona-gpio";
>
> In order to enable any later chip-specific quirking requirements, that
> compatible property should both specify the IP block and the specific
> chip it's included in, so I'd expect to see something like:
>
>                 compatible = "brcm,brcm11351-gpio", "brcm,kona-gpio";

Added.

Thanks,
-Markus
Stephen Warren July 26, 2013, 11:09 p.m. | #3
On 07/26/2013 04:51 PM, Markus Mayer wrote:
> On 26 July 2013 15:08, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> (CC'ing the DT bindings maintainers hence quoting the binding in full)
>>
>> On 07/26/2013 02:25 PM, Markus Mayer wrote:

>>> diff --git a/Documentation/devicetree/bindings/gpio/gpio-bcm-kona.txt b/Documentation/devicetree/bindings/gpio/gpio-bcm-kona.txt

>>> +Required properties:
>>> +  - compatible: "brcm,kona-gpio"
>>> +  - reg: Physical base address and length of the controller's registers.
>>> +  - interrupts: The interrupt outputs from the controller.
>>
>> How many entries are required? I notice there's more than 1 in the
>> example below.
> 
> You only "need" the ones you want to use, i.e. at least one. The board
> supports up to 6. I can mention as much in the description.

Isn't this a property of the SoC's IP block, not of the board?
Markus Mayer July 26, 2013, 11:22 p.m. | #4
On 26 July 2013 16:09, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 07/26/2013 04:51 PM, Markus Mayer wrote:
>> On 26 July 2013 15:08, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>> (CC'ing the DT bindings maintainers hence quoting the binding in full)
>>>
>>> On 07/26/2013 02:25 PM, Markus Mayer wrote:
>
>>>> diff --git a/Documentation/devicetree/bindings/gpio/gpio-bcm-kona.txt b/Documentation/devicetree/bindings/gpio/gpio-bcm-kona.txt
>
>>>> +Required properties:
>>>> +  - compatible: "brcm,kona-gpio"
>>>> +  - reg: Physical base address and length of the controller's registers.
>>>> +  - interrupts: The interrupt outputs from the controller.
>>>
>>> How many entries are required? I notice there's more than 1 in the
>>> example below.
>>
>> You only "need" the ones you want to use, i.e. at least one. The board
>> supports up to 6. I can mention as much in the description.
>
> Isn't this a property of the SoC's IP block, not of the board?

Yes. I wasn't very precise with my statement.

What I added to the description is "Specify at least 1 GPIO interrupt.
The maximum number is 6."
Tim Kryger July 27, 2013, 12:13 a.m. | #5
On Fri, Jul 26, 2013 at 4:22 PM, Markus Mayer <markus.mayer@linaro.org> wrote:
> On 26 July 2013 16:09, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> Isn't this a property of the SoC's IP block, not of the board?
>
> Yes. I wasn't very precise with my statement.
>
> What I added to the description is "Specify at least 1 GPIO interrupt.
> The maximum number is 6."

The IP has a configurable number of banks, each of which has its own
interrupt.  If a particular SoC was configured to have six banks then
you need to list six interrupts in the DTS.  Perhaps it would help to
spell that out that relationship explicitly in the documentation
rather than talking about a minimum or maximum number of interrupts?
Also, you may want to mention that interrupts should to be ordered by
bank number.
Linus Walleij Aug. 16, 2013, 12:51 p.m. | #6
On Fri, Jul 26, 2013 at 10:25 PM, Markus Mayer <markus.mayer@linaro.org> wrote:

> --- a/arch/arm/boot/dts/bcm11351.dtsi
> +++ b/arch/arm/boot/dts/bcm11351.dtsi
> @@ -63,6 +63,22 @@
>                 clock-frequency = <32768>;
>         };
>
> +       gpio: gpio@35003000 {
> +               compatible = "brcm,kona-gpio";
> +               reg = <0x35003000 0x800>;
> +               interrupts =
> +                      <0x0 106 0x4
> +                       0x0 115 0x4
> +                       0x0 114 0x4
> +                       0x0 113 0x4
> +                       0x0 112 0x4
> +                       0x0 111 0x4>;
> +               #gpio-cells = <2>;
> +               #interrupt-cells = <2>;
> +               gpio-controller;
> +               interrupt-controller;
> +       };

Do this:

#include <dt-bindings//interrupt-controller/irq.h>
#include <dt-bindings/interrupt-controller/arm-gic.h>

Rewrite like this:

+               interrupts =
+                      <GIC_SPI 106 IRQ_TYPE_LEVEL_HIGH
+                       GIC_SPI 115 IRQ_TYPE_LEVEL_HIGH

As you can see we can now understand the dts file.

Possibly this should also be done in the examples.

Yours,
Linus Walleij
Markus Mayer Aug. 19, 2013, 6:41 p.m. | #7
On 16 August 2013 05:51, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Fri, Jul 26, 2013 at 10:25 PM, Markus Mayer <markus.mayer@linaro.org> wrote:
>
>> --- a/arch/arm/boot/dts/bcm11351.dtsi
>> +++ b/arch/arm/boot/dts/bcm11351.dtsi
>> @@ -63,6 +63,22 @@
>>                 clock-frequency = <32768>;
>>         };
>>
>> +       gpio: gpio@35003000 {
>> +               compatible = "brcm,kona-gpio";
>> +               reg = <0x35003000 0x800>;
>> +               interrupts =
>> +                      <0x0 106 0x4
>> +                       0x0 115 0x4
>> +                       0x0 114 0x4
>> +                       0x0 113 0x4
>> +                       0x0 112 0x4
>> +                       0x0 111 0x4>;
>> +               #gpio-cells = <2>;
>> +               #interrupt-cells = <2>;
>> +               gpio-controller;
>> +               interrupt-controller;
>> +       };
>
> Do this:
>
> #include <dt-bindings//interrupt-controller/irq.h>
> #include <dt-bindings/interrupt-controller/arm-gic.h>
>
> Rewrite like this:
>
> +               interrupts =
> +                      <GIC_SPI 106 IRQ_TYPE_LEVEL_HIGH
> +                       GIC_SPI 115 IRQ_TYPE_LEVEL_HIGH
>
> As you can see we can now understand the dts file.
>
> Possibly this should also be done in the examples.

Thanks for the feedback. This change will be part of the next revision
that I am about to send out (in dtsi & examples).

Regards,
-Markus

Patch

diff --git a/Documentation/devicetree/bindings/gpio/gpio-bcm-kona.txt b/Documentation/devicetree/bindings/gpio/gpio-bcm-kona.txt
new file mode 100644
index 0000000..08e9b5a
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-bcm-kona.txt
@@ -0,0 +1,41 @@ 
+Broadcom Kona Family GPIO
+-------------------------
+
+This GPIO driver is used in the following Broadcom SoCs:
+  BCM11130, BCM11140, BCM11351, BCM28145, BCM28155
+
+The GPIO controller only supports edge, not level triggering.
+
+Required properties:
+  - compatible: "brcm,kona-gpio"
+  - reg: Physical base address and length of the controller's registers.
+  - interrupts: The interrupt outputs from the controller.
+  - #gpio-cells: Should be <2>. The first cell is the pin number, the second
+    cell is used to specify optional parameters:
+    - bit 0 specifies polarity (0 for normal, 1 for inverted)
+  - #interrupt-cells: Should be <2>. The first cell is the GPIO number.
+    The second cell is used to specify flags:
+    - trigger type (bits[1:0]):
+        1 = low-to-high edge triggered.
+        2 = high-to-low edge triggered.
+        3 = low-to-high or high-to-low edge triggered
+        Valid values are 1, 2, 3
+  - gpio-controller: Marks the device node as a GPIO controller.
+  - interrupt-controller: Marks the device node as an interrupt controller.
+
+Example:
+	gpio: gpio@35003000 {
+		compatible = "brcm,kona-gpio";
+		reg = <0x35003000 0x800>;
+		interrupts =
+		       <0x0 106 0x4
+			0x0 115 0x4
+			0x0 114 0x4
+			0x0 113 0x4
+			0x0 112 0x4
+			0x0 111 0x4>;
+		#gpio-cells = <2>;
+		#interrupt-cells = <2>;
+		gpio-controller;
+		interrupt-controller;
+	};
diff --git a/arch/arm/boot/dts/bcm11351.dtsi b/arch/arm/boot/dts/bcm11351.dtsi
index c0cdf66..13aaa06 100644
--- a/arch/arm/boot/dts/bcm11351.dtsi
+++ b/arch/arm/boot/dts/bcm11351.dtsi
@@ -63,6 +63,22 @@ 
 		clock-frequency = <32768>;
 	};
 
+	gpio: gpio@35003000 {
+		compatible = "brcm,kona-gpio";
+		reg = <0x35003000 0x800>;
+		interrupts =
+		       <0x0 106 0x4
+			0x0 115 0x4
+			0x0 114 0x4
+			0x0 113 0x4
+			0x0 112 0x4
+			0x0 111 0x4>;
+		#gpio-cells = <2>;
+		#interrupt-cells = <2>;
+		gpio-controller;
+		interrupt-controller;
+	};
+
 	sdio0: sdio@0x3f180000 {
 		compatible = "bcm,kona-sdhci";
 		reg = <0x3f180000 0x10000>;