diff mbox

[v3,01/14] Documentation: dt/bindings: Document pinctrl-ingenic

Message ID 20170125185207.23902-2-paul@crapouillou.net (mailing list archive)
State New, archived
Headers show

Commit Message

Paul Cercueil Jan. 25, 2017, 6:51 p.m. UTC
This commit adds documentation for the devicetree bidings of the
pinctrl-ingenic driver, which handles pin configuration and pin
muxing of the Ingenic SoCs currently supported by the Linux kernel.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 .../bindings/pinctrl/ingenic,pinctrl.txt           | 77 ++++++++++++++++++++++
 1 file changed, 77 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/ingenic,pinctrl.txt

v2: Rewrote the documentation for the new pinctrl-ingenic driver
v3: No changes

Comments

Rob Herring Jan. 30, 2017, 8:36 p.m. UTC | #1
On Wed, Jan 25, 2017 at 07:51:54PM +0100, Paul Cercueil wrote:
> This commit adds documentation for the devicetree bidings of the
> pinctrl-ingenic driver, which handles pin configuration and pin
> muxing of the Ingenic SoCs currently supported by the Linux kernel.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>  .../bindings/pinctrl/ingenic,pinctrl.txt           | 77 ++++++++++++++++++++++
>  1 file changed, 77 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pinctrl/ingenic,pinctrl.txt
> 
> v2: Rewrote the documentation for the new pinctrl-ingenic driver
> v3: No changes
> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/ingenic,pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/ingenic,pinctrl.txt
> new file mode 100644
> index 000000000000..ead5b01ad471
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pinctrl/ingenic,pinctrl.txt
> @@ -0,0 +1,77 @@
> +Ingenic jz47xx pin controller
> +
> +Please refer to pinctrl-bindings.txt in this directory for details of the
> +common pinctrl bindings used by client devices, including the meaning of the
> +phrase "pin configuration node".
> +
> +For the jz47xx SoCs, pin control is tightly bound with GPIO ports. All pins may
> +be used as GPIOs, multiplexed device functions are configured within the
> +GPIO port configuration registers and it is typical to refer to pins using the
> +naming scheme "PxN" where x is a character identifying the GPIO port with
> +which the pin is associated and N is an integer from 0 to 31 identifying the
> +pin within that GPIO port. For example PA0 is the first pin in GPIO port A, and
> +PB31 is the last pin in GPIO port B. The jz4740 contains 4 GPIO ports, PA to
> +PD, for a total of 128 pins. The jz4780 contains 6 GPIO ports, PA to PF, for a
> +total of 192 pins.

From the overlapping register addresses in the examples and this 
description, it looks like the pinctrlr and gpio controller are 1 block. 
If so, then there should only be 1 node.

Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Paul Cercueil Jan. 31, 2017, 10:31 a.m. UTC | #2
Hi,

> From the overlapping register addresses in the examples and this
> description, it looks like the pinctrlr and gpio controller are 1 
> block.
> If so, then there should only be 1 node.

Well, that's what I had until Linus W. just told me to do the opposite:

> Just pull all these down two levels and make them one device
> each instead of having them inside the pin controller node
> like this.

-Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij Jan. 31, 2017, 1:09 p.m. UTC | #3
On Tue, Jan 31, 2017 at 11:31 AM, Paul Cercueil <paul@crapouillou.net> wrote:
> [Rob]:
>> From the overlapping register addresses in the examples and this
>> description, it looks like the pinctrlr and gpio controller are 1 block.
>> If so, then there should only be 1 node.
>
> Well, that's what I had until Linus W. just told me to do the opposite:
>
>> Just pull all these down two levels and make them one device
>> each instead of having them inside the pin controller node
>> like this.

I guess the argument is that they are in the same coherent memory
range so they should be one device node. That is how we handle
e.g. system controllers so it makes some sense.

So can the two GPIO controllers be modeled as two subnodes of
the pin controller then?

Subnodes are certainly OK, we have that for many other devices
such as interrupt controllers on PCI bridges and what not.

So when the probing of the pin controller is ready it can just
walk down and populate the GPIO subdevices with
of_platform_default_populate() or simply by registering the
device directly with platform_device_add_data() just like an
MFD device does?

This is nice because we want to use the standard gpio ranges
to map pins to GPIO lines.

I'm sorry about the unclarities here, but it's essentially an intrinsic
problem with GPIO that has been with us for years: do we model
each "bank" as a device or do we just register each bank as a
gpiochip, or do we even make one gpiochip to cover all the banks.
All solutions can be found in the kernel... also the different DT bindings:
one node for a whole slew of GPIO controllers, or seveal nodes
and I bet also several nodes for memory ranges in close proximity.

I don't know for sure what is the most elegant solution, we might
need to build some consensus here for the future so it doesn't
get to heterogeneous.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Paul Cercueil Feb. 9, 2017, 5:28 p.m. UTC | #4
Le 2017-01-31 14:09, Linus Walleij a écrit :
> On Tue, Jan 31, 2017 at 11:31 AM, Paul Cercueil <paul@crapouillou.net> 
> wrote:
>> [Rob]:
>>> From the overlapping register addresses in the examples and this
>>> description, it looks like the pinctrlr and gpio controller are 1 
>>> block.
>>> If so, then there should only be 1 node.
>> 
>> Well, that's what I had until Linus W. just told me to do the 
>> opposite:
>> 
>>> Just pull all these down two levels and make them one device
>>> each instead of having them inside the pin controller node
>>> like this.
> 
> I guess the argument is that they are in the same coherent memory
> range so they should be one device node. That is how we handle
> e.g. system controllers so it makes some sense.
> 
> So can the two GPIO controllers be modeled as two subnodes of
> the pin controller then?
> 
> Subnodes are certainly OK, we have that for many other devices
> such as interrupt controllers on PCI bridges and what not.
> 
> So when the probing of the pin controller is ready it can just
> walk down and populate the GPIO subdevices with
> of_platform_default_populate() or simply by registering the
> device directly with platform_device_add_data() just like an
> MFD device does?
> 
> This is nice because we want to use the standard gpio ranges
> to map pins to GPIO lines.
> 
> I'm sorry about the unclarities here, but it's essentially an intrinsic
> problem with GPIO that has been with us for years: do we model
> each "bank" as a device or do we just register each bank as a
> gpiochip, or do we even make one gpiochip to cover all the banks.
> All solutions can be found in the kernel... also the different DT 
> bindings:
> one node for a whole slew of GPIO controllers, or seveal nodes
> and I bet also several nodes for memory ranges in close proximity.
> 
> I don't know for sure what is the most elegant solution, we might
> need to build some consensus here for the future so it doesn't
> get to heterogeneous.
> 
> Yours,
> Linus Walleij

I was thinking that instead of having one pinctrl-ingenic instance 
covering
0x600 of register space, and 6 instances of gpio-ingenic having 0x100 
each,
I could just have 6 instances of pinctrl-ingenic, each one with an 
instance
of gpio-ingenic declared as a sub-node, each handling just 0x100 of 
memory space.

Then I can make pinctrl-ingenic and gpio-ingenic share a regmap (through 
syscon),
which would be a good idea anyway since the two drivers poke to the very 
same
registers (in theory not at the same time, but it's never safe to assume 
things
like this).

Problem is, that in that case the pin functions/groups (and 
ingenic,pull-ups)
would have to be in DTS because we would have 6 instances with different 
pin
groups, and I know you hate that.

Thoughts?
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij Feb. 20, 2017, 1:56 p.m. UTC | #5
On Thu, Feb 9, 2017 at 6:28 PM, Paul Cercueil <paul@crapouillou.net> wrote:

> I was thinking that instead of having one pinctrl-ingenic instance covering
> 0x600 of register space, and 6 instances of gpio-ingenic having 0x100 each,
> I could just have 6 instances of pinctrl-ingenic, each one with an instance
> of gpio-ingenic declared as a sub-node, each handling just 0x100 of memory
> space.

My head is spinning,  but I think I get it. What is wrong with the solution
I proposed with one pin control instance covering the whole 0x600 and with 6
subnodes of GPIO?

The GPIO nodes do not even have to have an address range associated with
them you know, that can be distributed out with regmap code accessing
the parent regmap.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Paul Cercueil Feb. 21, 2017, 11:20 a.m. UTC | #6
Le 2017-02-20 14:56, Linus Walleij a écrit :
> On Thu, Feb 9, 2017 at 6:28 PM, Paul Cercueil <paul@crapouillou.net> 
> wrote:
> 
>> I was thinking that instead of having one pinctrl-ingenic instance 
>> covering
>> 0x600 of register space, and 6 instances of gpio-ingenic having 0x100 
>> each,
>> I could just have 6 instances of pinctrl-ingenic, each one with an 
>> instance
>> of gpio-ingenic declared as a sub-node, each handling just 0x100 of 
>> memory
>> space.
> 
> My head is spinning,  but I think I get it. What is wrong with the 
> solution
> I proposed with one pin control instance covering the whole 0x600 and 
> with 6
> subnodes of GPIO?
> 
> The GPIO nodes do not even have to have an address range associated 
> with
> them you know, that can be distributed out with regmap code accessing
> the parent regmap.

OK, but then each GPIO chip 'X' still need to know its offset in the 
register
area, which is (pinctrl_base + X * 0x100).
What's the best way to pass that info to the driver? (I assume it's not 
with
a custom DT binding...).

Regards,
-Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij Feb. 23, 2017, 9:59 a.m. UTC | #7
On Tue, Feb 21, 2017 at 12:20 PM, Paul Cercueil <paul@crapouillou.net> wrote:
> Le 2017-02-20 14:56, Linus Walleij a écrit :
>>
>> On Thu, Feb 9, 2017 at 6:28 PM, Paul Cercueil <paul@crapouillou.net>
>> wrote:
>>
>>> I was thinking that instead of having one pinctrl-ingenic instance
>>> covering
>>> 0x600 of register space, and 6 instances of gpio-ingenic having 0x100
>>> each,
>>> I could just have 6 instances of pinctrl-ingenic, each one with an
>>> instance
>>> of gpio-ingenic declared as a sub-node, each handling just 0x100 of
>>> memory
>>> space.
>>
>>
>> My head is spinning,  but I think I get it. What is wrong with the
>> solution
>> I proposed with one pin control instance covering the whole 0x600 and with
>> 6
>> subnodes of GPIO?
>>
>> The GPIO nodes do not even have to have an address range associated with
>> them you know, that can be distributed out with regmap code accessing
>> the parent regmap.
>
>
> OK, but then each GPIO chip 'X' still need to know its offset in the
> register
> area, which is (pinctrl_base + X * 0x100).
> What's the best way to pass that info to the driver? (I assume it's not with
> a custom DT binding...).

I do not really understand what driver you are referring to.

If the pin controller node is overarching and spawning children for
the gpiochips, you use the design pattern from MFD to pass data
from parents to children, e.g.:

#include <linux/regmap.h>

pinctrl driver:
     struct regmap_config mapconf = {
                .reg_bits = 32,
                .val_bits = 32,
                .reg_stride = 4,
     };
    struct regmap *map;

    map = regmap_init_mmio(dev, base, &mapconf);
    if (IS_ERR(map))
          ....
    dev_set_drvdata(dev, map);
    of_populate_children(dev,)..
    (can also use platform_device_add_data() or "simple-bus" etc)

gpio subdrivers:
     struct regmap *map;

     map = dev_get_drvdata(dev->parent);

There are examples of drivers passing more complex things to
their children than a regmap, just put some struct in a <linux/*/*.h>
file and pass it with drvdata as per above.

PS i2c_set_drvdata(), platform_set_drvdata() are just aliases
for dev_set_drvdata().

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Paul Cercueil April 2, 2017, 8:42 p.m. UTC | #8
This is the v4 of my patchset that introduces the pinctrl-ingenic and
gpio-ingenic drivers. The drivers were rewritten based on the feedback
received on the v3 version.

The pinctrl-ingenic driver now contains the pinmux/pinconf information of
each compatible SoC, so the devicetree bindings have been simplified greatly.

The driver now uses regmap for accessing the registers. This regmap is shared
to optional instances of the gpio-ingenic driver, that are instanciated as
MFD cells of the pinctrl-ingenic driver.

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/pinctrl/ingenic,pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/ingenic,pinctrl.txt
new file mode 100644
index 000000000000..ead5b01ad471
--- /dev/null
+++ b/Documentation/devicetree/bindings/pinctrl/ingenic,pinctrl.txt
@@ -0,0 +1,77 @@ 
+Ingenic jz47xx pin controller
+
+Please refer to pinctrl-bindings.txt in this directory for details of the
+common pinctrl bindings used by client devices, including the meaning of the
+phrase "pin configuration node".
+
+For the jz47xx SoCs, pin control is tightly bound with GPIO ports. All pins may
+be used as GPIOs, multiplexed device functions are configured within the
+GPIO port configuration registers and it is typical to refer to pins using the
+naming scheme "PxN" where x is a character identifying the GPIO port with
+which the pin is associated and N is an integer from 0 to 31 identifying the
+pin within that GPIO port. For example PA0 is the first pin in GPIO port A, and
+PB31 is the last pin in GPIO port B. The jz4740 contains 4 GPIO ports, PA to
+PD, for a total of 128 pins. The jz4780 contains 6 GPIO ports, PA to PF, for a
+total of 192 pins.
+
+
+Pin controller node
+===================
+
+Required properties:
+- compatible: One of:
+  - "ingenic,jz4740-pinctrl"
+  - "ingenic,jz4780-pinctrl"
+
+Optional properties:
+- ingenic,pull-ups: A list of 32-bit bit fields, where each bit set tells the
+  driver that a pull-up resistor is available for this pin.
+  By default, the driver considers that all pins feature a pull-up resistor.
+- ingenic,pull-downs: A list of 32-bit bit fields, where each bit set tells
+  the driver that a pull-down resistor is available for this pin.
+  By default, the driver considers that all pins feature a pull-down
+  resistor.
+
+
+'functions' sub-node
+====================
+
+The 'functions' node will contain sub-nodes that correspond to pin function
+nodes, and no properties. Pin function nodes will contain sub-nodes that
+correspond to pin groups, and no properties.
+
+The names of the pin function nodes will end up being the available functions
+provided by the pinctrl driver.
+The names of the pin group nodes will end up being the available groups
+provided by the pinctrl driver.
+
+Required properties for pin groups:
+- ingenic,pins: <pin mode [pin mode ...]>;
+  where 'pin' is the number of the pin, and 'mode' is the function mode of the
+  pin that should be enabled for this group.
+
+
+Example:
+=======
+
+pinctrl: ingenic-pinctrl@10010000 {
+	compatible = "ingenic,jz4740-pinctrl";
+	reg = <0x10010000 0x400>;
+
+	ingenic,pull-ups   = <0xffffffff 0xffffffff 0xffffffff 0xdfffffff>;
+	ingenic,pull-downs = <0x00000000 0x00000000 0x00000000 0x00000000>;
+
+	functions {
+		mmc {
+			mmc-1bit {
+				/* CLK, CMD, D0 */
+				ingenic,pins = <0x69 0 0x68 0 0x6a 0>;
+			};
+
+			mmc-4bit {
+				/* D1, D2, D3 */
+				ingenic,pins = <0x6b 0 0x6c 0 0x6d 0>;
+			};
+		};
+	};
+};