[4/7] pinctrl: zynq: Document DT binding

Commit Message

Soren Brinkmann Nov. 3, 2014, 7:05 p.m. UTC

Add documentation for the devicetree binding for the Zynq pincontroller.

Signed-off-by: Soren Brinkmann <[e-mail]>
---
 .../bindings/pinctrl/xlnx,zynq-pinctrl.txt         | 90 ++++++++++++++++++++++
 1 file changed, 90 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/xlnx,zynq-pinctrl.txt

Comments

Andreas Färber Nov. 5, 2014, 3:35 a.m. UTC | #1

Am 03.11.2014 um 20:05 schrieb Soren Brinkmann:
> Add documentation for the devicetree binding for the Zynq pincontroller.
> 
> Signed-off-by: Soren Brinkmann <[e-mail]>
> ---
>  .../bindings/pinctrl/xlnx,zynq-pinctrl.txt         | 90 ++++++++++++++++++++++
>  1 file changed, 90 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pinctrl/xlnx,zynq-pinctrl.txt
> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/xlnx,zynq-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/xlnx,zynq-pinctrl.txt
> new file mode 100644
> index 000000000000..86a86b644e6c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pinctrl/xlnx,zynq-pinctrl.txt
> @@ -0,0 +1,90 @@
> +	Binding for Xilinx Zynq Pinctrl
> +
> +Required properties:
> +- compatible: "xlnx,zynq-pinctrl"
> +- syscon: phandle to SLCR
> +- reg: Offset and length of pinctrl space in SLCR
> +
> +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".
> +
> +Zynq's pin configuration nodes act as a container for an abitrary number of

"arbitrary"

> +subnodes. Each of these subnodes represents some desired configuration for a
> +pin, a group, or a list of pins or groups. This configuration can include the
> +mux function to select on those pin(s)/group(s), and various pin configuration
> +parameters, such as pull-up, slew rate, etc.
> +
> +The name of each subnode is not important; all subnodes should be enumerated
> +and processed purely based on their content.
> +
> +Each subnode only affects those parameters that are explicitly listed. In
> +other words, a subnode that lists a mux function but no pin configuration
> +parameters implies no information about any pin configuration parameters.
> +Similarly, a pin subnode that describes a pullup parameter implies no
> +information about e.g. the mux function.
> +
> +
> +The following generic properties as defined in pinctrl-bindings.txt are valid
> +to specify in a pin configuration subnode:
> + groups, pins, function, bias-disable, bias-high-impedance, bias-pull-up,
> + slew-rate, low-power-disable, low-power-enable
> +
> + Valid arguments for 'slew-rate' are '0' and '1' to select between slow and fast
> + respectively.
> +
> + Valid values for groups are:
> +   ethernet0_0_grp, ethernet1_0_grp, mdio0_0_grp, mdio1_0_grp,
> +   qspi0_0_grp, qspi1_0_grp, qspi_fbclk, qspi_cs1_grp, spi0_0_grp,
> +   spi0_1_grp - spi0_2_grp, spi1_0_grp - spi1_3_grp, sdio0_0_grp - sdio0_2_grp,
> +   sdio1_0_grp - sdio1_3_grp, sdio0_emio_wp, sdio0_emio_cd, sdio1_emio_wp,
> +   sdio1_emio_cd, smc0_nor, smc0_nor_cs1_grp, smc0_nor_addr25_grp, smc0_nand,
> +   can0_0_grp - can0_10_grp, can1_0_grp - can1_11_grp, uart0_0_grp - uart0_10_grp,
> +   uart1_0_grp - uart1_11_grp, i2c0_0_grp - i2c0_10_grp, i2c1_0_grp - i2c1_10_grp,
> +   ttc0_0_grp - ttc0_2_grp, ttc1_0_grp - ttc1_2_grp, swdt0_0_grp - swdt0_4_grp,
> +   gpio0_0_grp - gpio0_53_grp
> +
> + Valid values for pins are:
> +   MIO0 - MIO53

What about the four EMIO_SD* pins?

> +
> + Valid values for function are:
> +   ethernet0, ethernet1, mdio0, mdio1, qspi0, qspi1, qspi_fbclk, qspi_cs1,
> +   spi0, spi1, sdio0, sdio0_pc, sdio0_cd, sdio0_wp,
> +   sdio1, sdio1_pc, sdio1_cd, sdio1_wp,
> +   smc0_nor, smc0_nor_cs1, smc0_nor_addr25, smc0_nand, can0, can1, uart0, uart1,
> +   i2c0, i2c1, ttc0, ttc1, swdt0, gpio0
> +
> +The following driver-specific properties as defined here are valid to specify in
> +a pin configuration subnode:
> + - io-standard: Configure the pin to use the selected IO standard according to
> +   this mapping:
> +    1: LVCMOS18
> +    2: LVCMOS25
> +    3: LVCMOS33
> +    4: HSTL
> +
> +Example:
> +	pinctrl0: pinctrl@700 {
> +		compatible = "xlnx,pinctrl-zynq";
> +		reg = <0x700 0x200>;
> +		syscon = <&slcr>;
> +
> +		pinctrl_uart1_default: pinctrl-uart1-default {

Nit: Is it really necessary to prefix subnodes of "pinctrl@700" with
"pinctrl-", here and in .dts?

> +			common {
> +				groups = "uart1_10_grp";
> +				function = "uart1";
> +				slew-rate = <0>;
> +				io-standard = <1>;
> +			};
> +
> +			rx {
> +				pins = "MIO49";
> +				bias-high-impedance;
> +			};
> +
> +			tx {
> +				pins = "MIO48";
> +				bias-disable;
> +			};
> +		};
> +	};

Otherwise looks good.

Cheers,
Andreas

Soren Brinkmann Nov. 5, 2014, 5:07 p.m. UTC | #2

On Wed, 2014-11-05 at 04:35AM +0100, Andreas Färber wrote:
> Am 03.11.2014 um 20:05 schrieb Soren Brinkmann:
> > Add documentation for the devicetree binding for the Zynq pincontroller.
> > 
> > Signed-off-by: Soren Brinkmann <[e-mail]>
> > ---
> >  .../bindings/pinctrl/xlnx,zynq-pinctrl.txt         | 90 ++++++++++++++++++++++
> >  1 file changed, 90 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/pinctrl/xlnx,zynq-pinctrl.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/pinctrl/xlnx,zynq-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/xlnx,zynq-pinctrl.txt
> > new file mode 100644
> > index 000000000000..86a86b644e6c
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pinctrl/xlnx,zynq-pinctrl.txt
> > @@ -0,0 +1,90 @@
> > +	Binding for Xilinx Zynq Pinctrl
> > +
> > +Required properties:
> > +- compatible: "xlnx,zynq-pinctrl"
> > +- syscon: phandle to SLCR
> > +- reg: Offset and length of pinctrl space in SLCR
> > +
> > +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".
> > +
> > +Zynq's pin configuration nodes act as a container for an abitrary number of
> 
> "arbitrary"

I'll fix that.

> 
> > +subnodes. Each of these subnodes represents some desired configuration for a
> > +pin, a group, or a list of pins or groups. This configuration can include the
> > +mux function to select on those pin(s)/group(s), and various pin configuration
> > +parameters, such as pull-up, slew rate, etc.
> > +
> > +The name of each subnode is not important; all subnodes should be enumerated
> > +and processed purely based on their content.
> > +
> > +Each subnode only affects those parameters that are explicitly listed. In
> > +other words, a subnode that lists a mux function but no pin configuration
> > +parameters implies no information about any pin configuration parameters.
> > +Similarly, a pin subnode that describes a pullup parameter implies no
> > +information about e.g. the mux function.
> > +
> > +
> > +The following generic properties as defined in pinctrl-bindings.txt are valid
> > +to specify in a pin configuration subnode:
> > + groups, pins, function, bias-disable, bias-high-impedance, bias-pull-up,
> > + slew-rate, low-power-disable, low-power-enable
> > +
> > + Valid arguments for 'slew-rate' are '0' and '1' to select between slow and fast
> > + respectively.
> > +
> > + Valid values for groups are:
> > +   ethernet0_0_grp, ethernet1_0_grp, mdio0_0_grp, mdio1_0_grp,
> > +   qspi0_0_grp, qspi1_0_grp, qspi_fbclk, qspi_cs1_grp, spi0_0_grp,
> > +   spi0_1_grp - spi0_2_grp, spi1_0_grp - spi1_3_grp, sdio0_0_grp - sdio0_2_grp,
> > +   sdio1_0_grp - sdio1_3_grp, sdio0_emio_wp, sdio0_emio_cd, sdio1_emio_wp,
> > +   sdio1_emio_cd, smc0_nor, smc0_nor_cs1_grp, smc0_nor_addr25_grp, smc0_nand,
> > +   can0_0_grp - can0_10_grp, can1_0_grp - can1_11_grp, uart0_0_grp - uart0_10_grp,
> > +   uart1_0_grp - uart1_11_grp, i2c0_0_grp - i2c0_10_grp, i2c1_0_grp - i2c1_10_grp,
> > +   ttc0_0_grp - ttc0_2_grp, ttc1_0_grp - ttc1_2_grp, swdt0_0_grp - swdt0_4_grp,
> > +   gpio0_0_grp - gpio0_53_grp
> > +
> > + Valid values for pins are:
> > +   MIO0 - MIO53
> 
> What about the four EMIO_SD* pins?

You can't do any pinconf on those and the config_(set|get) return some
error code when you pass anything but the 54 MIO pins to those
functions. So, they are actually not valid for PINs. They are only used
as group for pinmuxing where the respective group needs to be used.

> 
> > +
> > + Valid values for function are:
> > +   ethernet0, ethernet1, mdio0, mdio1, qspi0, qspi1, qspi_fbclk, qspi_cs1,
> > +   spi0, spi1, sdio0, sdio0_pc, sdio0_cd, sdio0_wp,
> > +   sdio1, sdio1_pc, sdio1_cd, sdio1_wp,
> > +   smc0_nor, smc0_nor_cs1, smc0_nor_addr25, smc0_nand, can0, can1, uart0, uart1,
> > +   i2c0, i2c1, ttc0, ttc1, swdt0, gpio0
> > +
> > +The following driver-specific properties as defined here are valid to specify in
> > +a pin configuration subnode:
> > + - io-standard: Configure the pin to use the selected IO standard according to
> > +   this mapping:
> > +    1: LVCMOS18
> > +    2: LVCMOS25
> > +    3: LVCMOS33
> > +    4: HSTL
> > +
> > +Example:
> > +	pinctrl0: pinctrl@700 {
> > +		compatible = "xlnx,pinctrl-zynq";
> > +		reg = <0x700 0x200>;
> > +		syscon = <&slcr>;
> > +
> > +		pinctrl_uart1_default: pinctrl-uart1-default {
> 
> Nit: Is it really necessary to prefix subnodes of "pinctrl@700" with
> "pinctrl-", here and in .dts?

Guess not. Is there any best practice for naming pinctrl-state nodes?
I might shorten it.

> 
> > +			common {
> > +				groups = "uart1_10_grp";
> > +				function = "uart1";
> > +				slew-rate = <0>;
> > +				io-standard = <1>;
> > +			};
> > +
> > +			rx {
> > +				pins = "MIO49";
> > +				bias-high-impedance;
> > +			};
> > +
> > +			tx {
> > +				pins = "MIO48";
> > +				bias-disable;
> > +			};
> > +		};
> > +	};
> 
> Otherwise looks good.

Thanks for reviewing.

	Sören
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Linus Walleij Nov. 11, 2014, 3 p.m. UTC | #3

On Mon, Nov 3, 2014 at 8:05 PM, Soren Brinkmann
<[e-mail]> wrote:

> Add documentation for the devicetree binding for the Zynq pincontroller.
>
> Signed-off-by: Soren Brinkmann <[e-mail]>
(...)
> +Example:
> +       pinctrl0: pinctrl@700 {
> +               compatible = "xlnx,pinctrl-zynq";
> +               reg = <0x700 0x200>;
> +               syscon = <&slcr>;
> +
> +               pinctrl_uart1_default: pinctrl-uart1-default {
> +                       common {
> +                               groups = "uart1_10_grp";
> +                               function = "uart1";
> +                               slew-rate = <0>;
> +                               io-standard = <1>;
> +                       };

I don't really like that you mix multiplexing and config in the
same node. I would prefer if the generic bindings say we have
muxing nodes and config nodes, and those are disparate.

Can't you just split this:

common-mux {
    groups = "uart1_10_grp";
    function = "uart1";
};

common-config {
    groups = "uart1_10_grp";
    slew-rate = <0>;
    io-standard = <1>;
};

That way we can identify nodes as mux nodes (have "function")
or config nodes (have "groups" or "pins" but not "function") which
I think makes things easier to read.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Soren Brinkmann Nov. 12, 2014, 6:53 p.m. UTC | #4

On Tue, 2014-11-11 at 04:00PM +0100, Linus Walleij wrote:
> On Mon, Nov 3, 2014 at 8:05 PM, Soren Brinkmann
> <[e-mail]> wrote:
> 
> > Add documentation for the devicetree binding for the Zynq pincontroller.
> >
> > Signed-off-by: Soren Brinkmann <[e-mail]>
> (...)
> > +Example:
> > +       pinctrl0: pinctrl@700 {
> > +               compatible = "xlnx,pinctrl-zynq";
> > +               reg = <0x700 0x200>;
> > +               syscon = <&slcr>;
> > +
> > +               pinctrl_uart1_default: pinctrl-uart1-default {
> > +                       common {
> > +                               groups = "uart1_10_grp";
> > +                               function = "uart1";
> > +                               slew-rate = <0>;
> > +                               io-standard = <1>;
> > +                       };
> 
> I don't really like that you mix multiplexing and config in the
> same node. I would prefer if the generic bindings say we have
> muxing nodes and config nodes, and those are disparate.
> 
> Can't you just split this:
> 
> common-mux {
>     groups = "uart1_10_grp";
>     function = "uart1";
> };
> 
> common-config {
>     groups = "uart1_10_grp";
>     slew-rate = <0>;
>     io-standard = <1>;
> };
> 
> That way we can identify nodes as mux nodes (have "function")
> or config nodes (have "groups" or "pins" but not "function") which
> I think makes things easier to read.

I think such separation is not required by the bindings currently and
the parser assumes everything can be present in any node.
Can we add that requirement to the generic bindings without breaking
current users? I think it would make the implementation a little easier.
	
	Thanks,
	Sören
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Linus Walleij Nov. 27, 2014, 1:10 p.m. UTC | #5

On Wed, Nov 12, 2014 at 7:53 PM, Sören Brinkmann
<[e-mail]> wrote:
> On Tue, 2014-11-11 at 04:00PM +0100, Linus Walleij wrote:

>> I don't really like that you mix multiplexing and config in the
>> same node. I would prefer if the generic bindings say we have
>> muxing nodes and config nodes, and those are disparate.
>>
>> Can't you just split this:
>>
>> common-mux {
>>     groups = "uart1_10_grp";
>>     function = "uart1";
>> };
>>
>> common-config {
>>     groups = "uart1_10_grp";
>>     slew-rate = <0>;
>>     io-standard = <1>;
>> };
>>
>> That way we can identify nodes as mux nodes (have "function")
>> or config nodes (have "groups" or "pins" but not "function") which
>> I think makes things easier to read.
>
> I think such separation is not required by the bindings currently and
> the parser assumes everything can be present in any node.

The bindings say:

== Generic pin multiplexing node content ==

pin multiplexing nodes:

function                - the mux function to select
groups                  - the list of groups to select with this function

Example:

state_0_node_a {
        function = "uart0";
        groups = "u0rxtx", "u0rtscts";
};
state_1_node_a {
        function = "spi0";
        groups = "spi0pins";
};


== Generic pin configuration node content ==

(...)
Supported generic properties are:

pins                    - the list of pins that properties in the node
                          apply to (either this or "group" has to be
                          specified)
group                   - the group to apply the properties to, if the driver
                          supports configuration of whole groups rather than
                          individual pins (either this or "pins" has to be
                          specified)

It is not explicit that they have to be separate nodes but if needed
we can state that more clearly.

> Can we add that requirement to the generic bindings without breaking
> current users? I think it would make the implementation a little easier.

I think so.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/Documentation/devicetree/bindings/pinctrl/xlnx,zynq-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/xlnx,zynq-pinctrl.txt
new file mode 100644
index 000000000000..86a86b644e6c
--- /dev/null
+++ b/Documentation/devicetree/bindings/pinctrl/xlnx,zynq-pinctrl.txt
@@ -0,0 +1,90 @@ 
+	Binding for Xilinx Zynq Pinctrl
+
+Required properties:
+- compatible: "xlnx,zynq-pinctrl"
+- syscon: phandle to SLCR
+- reg: Offset and length of pinctrl space in SLCR
+
+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".
+
+Zynq's pin configuration nodes act as a container for an abitrary number of
+subnodes. Each of these subnodes represents some desired configuration for a
+pin, a group, or a list of pins or groups. This configuration can include the
+mux function to select on those pin(s)/group(s), and various pin configuration
+parameters, such as pull-up, slew rate, etc.
+
+The name of each subnode is not important; all subnodes should be enumerated
+and processed purely based on their content.
+
+Each subnode only affects those parameters that are explicitly listed. In
+other words, a subnode that lists a mux function but no pin configuration
+parameters implies no information about any pin configuration parameters.
+Similarly, a pin subnode that describes a pullup parameter implies no
+information about e.g. the mux function.
+
+
+The following generic properties as defined in pinctrl-bindings.txt are valid
+to specify in a pin configuration subnode:
+ groups, pins, function, bias-disable, bias-high-impedance, bias-pull-up,
+ slew-rate, low-power-disable, low-power-enable
+
+ Valid arguments for 'slew-rate' are '0' and '1' to select between slow and fast
+ respectively.
+
+ Valid values for groups are:
+   ethernet0_0_grp, ethernet1_0_grp, mdio0_0_grp, mdio1_0_grp,
+   qspi0_0_grp, qspi1_0_grp, qspi_fbclk, qspi_cs1_grp, spi0_0_grp,
+   spi0_1_grp - spi0_2_grp, spi1_0_grp - spi1_3_grp, sdio0_0_grp - sdio0_2_grp,
+   sdio1_0_grp - sdio1_3_grp, sdio0_emio_wp, sdio0_emio_cd, sdio1_emio_wp,
+   sdio1_emio_cd, smc0_nor, smc0_nor_cs1_grp, smc0_nor_addr25_grp, smc0_nand,
+   can0_0_grp - can0_10_grp, can1_0_grp - can1_11_grp, uart0_0_grp - uart0_10_grp,
+   uart1_0_grp - uart1_11_grp, i2c0_0_grp - i2c0_10_grp, i2c1_0_grp - i2c1_10_grp,
+   ttc0_0_grp - ttc0_2_grp, ttc1_0_grp - ttc1_2_grp, swdt0_0_grp - swdt0_4_grp,
+   gpio0_0_grp - gpio0_53_grp
+
+ Valid values for pins are:
+   MIO0 - MIO53
+
+ Valid values for function are:
+   ethernet0, ethernet1, mdio0, mdio1, qspi0, qspi1, qspi_fbclk, qspi_cs1,
+   spi0, spi1, sdio0, sdio0_pc, sdio0_cd, sdio0_wp,
+   sdio1, sdio1_pc, sdio1_cd, sdio1_wp,
+   smc0_nor, smc0_nor_cs1, smc0_nor_addr25, smc0_nand, can0, can1, uart0, uart1,
+   i2c0, i2c1, ttc0, ttc1, swdt0, gpio0
+
+The following driver-specific properties as defined here are valid to specify in
+a pin configuration subnode:
+ - io-standard: Configure the pin to use the selected IO standard according to
+   this mapping:
+    1: LVCMOS18
+    2: LVCMOS25
+    3: LVCMOS33
+    4: HSTL
+
+Example:
+	pinctrl0: pinctrl@700 {
+		compatible = "xlnx,pinctrl-zynq";
+		reg = <0x700 0x200>;
+		syscon = <&slcr>;
+
+		pinctrl_uart1_default: pinctrl-uart1-default {
+			common {
+				groups = "uart1_10_grp";
+				function = "uart1";
+				slew-rate = <0>;
+				io-standard = <1>;
+			};
+
+			rx {
+				pins = "MIO49";
+				bias-high-impedance;
+			};
+
+			tx {
+				pins = "MIO48";
+				bias-disable;
+			};
+		};
+	};