[PATCH v2 1/2] gpio: dt-bindings: add brcm,bcm6345-gpio bindings

[PATCH v2 1/2] gpio: dt-bindings: add brcm,bcm6345-gpio bindings

[Álvaro Fernández Rojas]

This patch adds the device tree bindings for the Broadcom's BCM6345
memory-mapped GPIO controllers.

The gpios will be supported by gpio-mmio code of the
GPIO generic library.

Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
---
 v2: add improvements suggested by Jonas Gorski:
  - use native-endian instead of big-endian.
  - use gpio alias instead of gpio0.
  - be less dramatic with newer Broadcom SoCs.
  - reorder patches.

 .../devicetree/bindings/gpio/brcm,bcm6345-gpio.txt | 46 ++++++++++++++++++++++
 1 file changed, 46 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/brcm,bcm6345-gpio.txt

diff --git a/Documentation/devicetree/bindings/gpio/brcm,bcm6345-gpio.txt b/Documentation/devicetree/bindings/gpio/brcm,bcm6345-gpio.txt
new file mode 100644
index 0000000..e785314
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/brcm,bcm6345-gpio.txt
@@ -0,0 +1,46 @@
+Bindings for the Broadcom's brcm,bcm6345-gpio memory-mapped GPIO controllers.
+
+These bindings can be used on any BCM63xx SoC. However, BCM6338 and BCM6345
+are the only ones which don't need a pinctrl driver.
+BCM6338 have 8-bit data and dirout registers, where GPIO state can be read
+and/or written, and the direction changed from input to output.
+BCM6345 have 16-bit data and dirout registers, where GPIO state can be read
+and/or written, and the direction changed from input to output.
+
+Required properties:
+	- compatible: should be "brcm,bcm6345-gpio"
+	- reg-names: must contain
+		"dat" - data register
+		"dirout" - direction (output) register
+	- reg: address + size pairs describing the GPIO register sets;
+		order must correspond with the order of entries in reg-names
+	- #gpio-cells: must be set to 2. The first cell is the pin number and
+			the second cell is used to specify the gpio polarity:
+			0 = active high
+			1 = active low
+	- gpio-controller: Marks the device node as a gpio controller.
+
+Optional properties:
+	- native-endian: use native endian memory.
+
+Examples:
+	- BCM6338:
+	gpio: gpio-controller@fffe0407 {
+		compatible = "brcm,bcm6345-gpio";
+		reg-names = "dirout", "dat";
+		reg = <0xfffe0407 1>, <0xfffe040f 1>;
+
+		#gpio-cells = <2>;
+		gpio-controller;
+	};
+
+	- BCM6345:
+	gpio: gpio-controller@fffe0406 {
+		compatible = "brcm,bcm6345-gpio";
+		reg-names = "dirout", "dat";
+		reg = <0xfffe0406 2>, <0xfffe040a 2>;
+		native-endian;
+
+		#gpio-cells = <2>;
+		gpio-controller;
+	};
-- 
2.1.4

Re: [PATCH v2 1/2] gpio: dt-bindings: add brcm,bcm6345-gpio bindings

[Linus Walleij]

All devicetree binding patches must be sent to the devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
mailing list so include them on subsequent posts of this patch.

On Wed, Aug 3, 2016 at 2:05 PM, Álvaro Fernández Rojas
<noltari-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> This patch adds the device tree bindings for the Broadcom's BCM6345
> memory-mapped GPIO controllers.
>
> The gpios will be supported by gpio-mmio code of the
> GPIO generic library.
>
> Signed-off-by: Álvaro Fernández Rojas <noltari-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  v2: add improvements suggested by Jonas Gorski:
>   - use native-endian instead of big-endian.

What is this? Does it come from some other existing binding?
I was feeling native endian is the default unless LE or BE is
explicitly specified...

> +Required properties:
> +       - compatible: should be "brcm,bcm6345-gpio"
> +       - reg-names: must contain
> +               "dat" - data register
> +               "dirout" - direction (output) register

I don't like this and don't understand why you can't just cover
all GPIO registers with a single reg property.

> +       - reg: address + size pairs describing the GPIO register sets;
> +               order must correspond with the order of entries in reg-names
> +       - #gpio-cells: must be set to 2. The first cell is the pin number and
> +                       the second cell is used to specify the gpio polarity:
> +                       0 = active high
> +                       1 = active low
> +       - gpio-controller: Marks the device node as a gpio controller.

Reference the standard bindings in gpio.txt for cells and controller.

> +Optional properties:
> +       - native-endian: use native endian memory.

That is weird. Explain why this is needed.

> +       - BCM6345:
> +       gpio: gpio-controller@fffe0406 {
> +               compatible = "brcm,bcm6345-gpio";
> +               reg-names = "dirout", "dat";
> +               reg = <0xfffe0406 2>, <0xfffe040a 2>;

Also I do not understand this at all. Why pick out two 16bit registers?
Surely the rest of the registers at 0xfffe0400 must be GPIO registers
as well? Are they not?

If they are, cover them all with a single reg property.

If they are not all GPIO registers, use MFD syscon for this and access
the constituent parts through that.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/2] gpio: dt-bindings: add brcm,bcm6345-gpio bindings

[Linus Walleij]

All devicetree binding patches must be sent to the devicetree@vger.kernel.org
mailing list so include them on subsequent posts of this patch.

On Wed, Aug 3, 2016 at 2:05 PM, Álvaro Fernández Rojas
<noltari@gmail.com> wrote:

> This patch adds the device tree bindings for the Broadcom's BCM6345
> memory-mapped GPIO controllers.
>
> The gpios will be supported by gpio-mmio code of the
> GPIO generic library.
>
> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
> ---
>  v2: add improvements suggested by Jonas Gorski:
>   - use native-endian instead of big-endian.

What is this? Does it come from some other existing binding?
I was feeling native endian is the default unless LE or BE is
explicitly specified...

> +Required properties:
> +       - compatible: should be "brcm,bcm6345-gpio"
> +       - reg-names: must contain
> +               "dat" - data register
> +               "dirout" - direction (output) register

I don't like this and don't understand why you can't just cover
all GPIO registers with a single reg property.

> +       - reg: address + size pairs describing the GPIO register sets;
> +               order must correspond with the order of entries in reg-names
> +       - #gpio-cells: must be set to 2. The first cell is the pin number and
> +                       the second cell is used to specify the gpio polarity:
> +                       0 = active high
> +                       1 = active low
> +       - gpio-controller: Marks the device node as a gpio controller.

Reference the standard bindings in gpio.txt for cells and controller.

> +Optional properties:
> +       - native-endian: use native endian memory.

That is weird. Explain why this is needed.

> +       - BCM6345:
> +       gpio: gpio-controller@fffe0406 {
> +               compatible = "brcm,bcm6345-gpio";
> +               reg-names = "dirout", "dat";
> +               reg = <0xfffe0406 2>, <0xfffe040a 2>;

Also I do not understand this at all. Why pick out two 16bit registers?
Surely the rest of the registers at 0xfffe0400 must be GPIO registers
as well? Are they not?

If they are, cover them all with a single reg property.

If they are not all GPIO registers, use MFD syscon for this and access
the constituent parts through that.

Yours,
Linus Walleij

Re: [PATCH v2 1/2] gpio: dt-bindings: add brcm,bcm6345-gpio bindings

[Álvaro Fernández Rojas]

Hi Linus,

El 11/08/2016 a las 13:45, Linus Walleij escribió:
> All devicetree binding patches must be sent to the devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> mailing list so include them on subsequent posts of this patch.
> 
> On Wed, Aug 3, 2016 at 2:05 PM, Álvaro Fernández Rojas
> <noltari-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 
>> This patch adds the device tree bindings for the Broadcom's BCM6345
>> memory-mapped GPIO controllers.
>>
>> The gpios will be supported by gpio-mmio code of the
>> GPIO generic library.
>>
>> Signed-off-by: Álvaro Fernández Rojas <noltari-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>>  v2: add improvements suggested by Jonas Gorski:
>>   - use native-endian instead of big-endian.
> 
> What is this? Does it come from some other existing binding?
> I was feeling native endian is the default unless LE or BE is
> explicitly specified...
Not really, even if the kernel is configured as big endian, either native-endian or big-endian are needed.
The default is little-endian if nothing else is specified:
https://github.com/torvalds/linux/blob/master/drivers/of/base.c#L598

> 
>> +Required properties:
>> +       - compatible: should be "brcm,bcm6345-gpio"
>> +       - reg-names: must contain
>> +               "dat" - data register
>> +               "dirout" - direction (output) register
> 
> I don't like this and don't understand why you can't just cover
> all GPIO registers with a single reg property.
Because we want to add a simple gpio driver for these old SoCs with the newer ones having a different and more complex gpio/pinctrl driver.

> 
>> +       - reg: address + size pairs describing the GPIO register sets;
>> +               order must correspond with the order of entries in reg-names
>> +       - #gpio-cells: must be set to 2. The first cell is the pin number and
>> +                       the second cell is used to specify the gpio polarity:
>> +                       0 = active high
>> +                       1 = active low
>> +       - gpio-controller: Marks the device node as a gpio controller.
> 
> Reference the standard bindings in gpio.txt for cells and controller.
Sure, I just copy & pasted this from the other gpio mmio driver:
https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/gpio/wd%2Cmbl-gpio.txt#L17

> 
>> +Optional properties:
>> +       - native-endian: use native endian memory.
> 
> That is weird. Explain why this is needed.
(explained above)

> 
>> +       - BCM6345:
>> +       gpio: gpio-controller@fffe0406 {
>> +               compatible = "brcm,bcm6345-gpio";
>> +               reg-names = "dirout", "dat";
>> +               reg = <0xfffe0406 2>, <0xfffe040a 2>;
> 
> Also I do not understand this at all. Why pick out two 16bit registers?
> Surely the rest of the registers at 0xfffe0400 must be GPIO registers
> as well? Are they not?
This is the layout for BCM6345:
https://gitlab.labs.nic.cz/turris/openwrt/blob/8bfda76892a39c033b5abf2c17035a444fffad08/target/linux/brcm63xx-2.6/files/include/asm-mips/mach-bcm963xx/6345_map_part.h#L141
And this is the layout for BCM6338:
https://gitlab.labs.nic.cz/turris/openwrt/blob/8bfda76892a39c033b5abf2c17035a444fffad08/target/linux/brcm63xx-2.6/files/include/asm-mips/mach-bcm963xx/6338_map_part.h#L202

> 
> If they are, cover them all with a single reg property.
> 
> If they are not all GPIO registers, use MFD syscon for this and access
> the constituent parts through that.
I don't think that's the case since we're using a generic mmio binding.

> 
> Yours,
> Linus Walleij
> 

Regards,
Álvaro.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/2] gpio: dt-bindings: add brcm,bcm6345-gpio bindings

[Álvaro Fernández Rojas]

Hi Linus,

El 11/08/2016 a las 13:45, Linus Walleij escribió:
> All devicetree binding patches must be sent to the devicetree@vger.kernel.org
> mailing list so include them on subsequent posts of this patch.
> 
> On Wed, Aug 3, 2016 at 2:05 PM, Álvaro Fernández Rojas
> <noltari@gmail.com> wrote:
> 
>> This patch adds the device tree bindings for the Broadcom's BCM6345
>> memory-mapped GPIO controllers.
>>
>> The gpios will be supported by gpio-mmio code of the
>> GPIO generic library.
>>
>> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
>> ---
>>  v2: add improvements suggested by Jonas Gorski:
>>   - use native-endian instead of big-endian.
> 
> What is this? Does it come from some other existing binding?
> I was feeling native endian is the default unless LE or BE is
> explicitly specified...
Not really, even if the kernel is configured as big endian, either native-endian or big-endian are needed.
The default is little-endian if nothing else is specified:
https://github.com/torvalds/linux/blob/master/drivers/of/base.c#L598

> 
>> +Required properties:
>> +       - compatible: should be "brcm,bcm6345-gpio"
>> +       - reg-names: must contain
>> +               "dat" - data register
>> +               "dirout" - direction (output) register
> 
> I don't like this and don't understand why you can't just cover
> all GPIO registers with a single reg property.
Because we want to add a simple gpio driver for these old SoCs with the newer ones having a different and more complex gpio/pinctrl driver.

> 
>> +       - reg: address + size pairs describing the GPIO register sets;
>> +               order must correspond with the order of entries in reg-names
>> +       - #gpio-cells: must be set to 2. The first cell is the pin number and
>> +                       the second cell is used to specify the gpio polarity:
>> +                       0 = active high
>> +                       1 = active low
>> +       - gpio-controller: Marks the device node as a gpio controller.
> 
> Reference the standard bindings in gpio.txt for cells and controller.
Sure, I just copy & pasted this from the other gpio mmio driver:
https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/gpio/wd%2Cmbl-gpio.txt#L17

> 
>> +Optional properties:
>> +       - native-endian: use native endian memory.
> 
> That is weird. Explain why this is needed.
(explained above)

> 
>> +       - BCM6345:
>> +       gpio: gpio-controller@fffe0406 {
>> +               compatible = "brcm,bcm6345-gpio";
>> +               reg-names = "dirout", "dat";
>> +               reg = <0xfffe0406 2>, <0xfffe040a 2>;
> 
> Also I do not understand this at all. Why pick out two 16bit registers?
> Surely the rest of the registers at 0xfffe0400 must be GPIO registers
> as well? Are they not?
This is the layout for BCM6345:
https://gitlab.labs.nic.cz/turris/openwrt/blob/8bfda76892a39c033b5abf2c17035a444fffad08/target/linux/brcm63xx-2.6/files/include/asm-mips/mach-bcm963xx/6345_map_part.h#L141
And this is the layout for BCM6338:
https://gitlab.labs.nic.cz/turris/openwrt/blob/8bfda76892a39c033b5abf2c17035a444fffad08/target/linux/brcm63xx-2.6/files/include/asm-mips/mach-bcm963xx/6338_map_part.h#L202

> 
> If they are, cover them all with a single reg property.
> 
> If they are not all GPIO registers, use MFD syscon for this and access
> the constituent parts through that.
I don't think that's the case since we're using a generic mmio binding.

> 
> Yours,
> Linus Walleij
> 

Regards,
Álvaro.

Re: [PATCH v2 1/2] gpio: dt-bindings: add brcm,bcm6345-gpio bindings

[Linus Walleij]

On Wed, Aug 3, 2016 at 2:05 PM, Álvaro Fernández Rojas
<noltari@gmail.com> wrote:

> This patch adds the device tree bindings for the Broadcom's BCM6345
> memory-mapped GPIO controllers.
>
> The gpios will be supported by gpio-mmio code of the
> GPIO generic library.
>
> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>

Turns out my remarks on the bindings and driver were stupid.

Patch applied.

Yours,
Linus Walleij