| Message ID | 1374870345-8276-3-git-send-email-markus.mayer@linaro.org |
|---|---|
| State | New |
| Headers | show |
(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";
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
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?
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."
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.
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
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
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>;