Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1623132 > unrolled thread

[RFC 1/2] dt-bindings: add mmio-based syscon mux controller DT bindings

Back to article view | Back to linux.kernel

Contents

  [RFC 1/2] dt-bindings: add mmio-based syscon mux controller DT bindings Philipp Zabel <p.zabel@pengutronix.de> - 2017-04-13 17:50 +0200
    Re: [RFC 1/2] dt-bindings: add mmio-based syscon mux controller DT  bindings Steve Longerbeam <slongerbeam@gmail.com> - 2017-04-14 03:10 +0200
      Re: [RFC 1/2] dt-bindings: add mmio-based syscon mux controller DT  bindings Philipp Zabel <p.zabel@pengutronix.de> - 2017-04-19 13:50 +0200
    Re: [RFC 1/2] dt-bindings: add mmio-based syscon mux controller DT  bindings Philipp Zabel <p.zabel@pengutronix.de> - 2017-04-18 10:20 +0200
      Re: [RFC 1/2] dt-bindings: add mmio-based syscon mux controller DT  bindings Sakari Ailus <sakari.ailus@iki.fi> - 2017-04-18 12:10 +0200
        Re: [RFC 1/2] dt-bindings: add mmio-based syscon mux controller DT  bindings Pavel Machek <pavel@ucw.cz> - 2017-04-18 12:40 +0200
          Re: [RFC 1/2] dt-bindings: add mmio-based syscon mux controller DT  bindings Sakari Ailus <sakari.ailus@iki.fi> - 2017-04-18 13:00 +0200
            Re: [RFC 1/2] dt-bindings: add mmio-based syscon mux controller DT  bindings Pavel Machek <pavel@ucw.cz> - 2017-04-18 14:00 +0200
        Re: [RFC 1/2] dt-bindings: add mmio-based syscon mux controller DT  bindings Philipp Zabel <p.zabel@pengutronix.de> - 2017-04-18 13:00 +0200
    Re: [RFC 1/2] dt-bindings: add mmio-based syscon mux controller DT  bindings Rob Herring <robh@kernel.org> - 2017-04-20 00:10 +0200
      Re: [RFC 1/2] dt-bindings: add mmio-based syscon mux controller DT  bindings Philipp Zabel <p.zabel@pengutronix.de> - 2017-04-20 10:20 +0200
        Re: [RFC 1/2] dt-bindings: add mmio-based syscon mux controller DT  bindings Philipp Zabel <p.zabel@pengutronix.de> - 2017-04-20 15:10 +0200
        Re: [RFC 1/2] dt-bindings: add mmio-based syscon mux controller DT  bindings Rob Herring <robh@kernel.org> - 2017-04-20 15:50 +0200
      Re: [RFC 1/2] dt-bindings: add mmio-based syscon mux controller DT  bindings Philipp Zabel <p.zabel@pengutronix.de> - 2017-04-20 17:00 +0200

#1623132 — [RFC 1/2] dt-bindings: add mmio-based syscon mux controller DT bindings

This adds device tree binding documentation for mmio-based syscon
multiplexers controlled by a single bitfield in a syscon register
range.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 Documentation/devicetree/bindings/mux/mmio-mux.txt | 56 ++++++++++++++++++++++
 1 file changed, 56 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mux/mmio-mux.txt

diff --git a/Documentation/devicetree/bindings/mux/mmio-mux.txt b/Documentation/devicetree/bindings/mux/mmio-mux.txt
new file mode 100644
index 0000000000000..11d96f5d98583
--- /dev/null
+++ b/Documentation/devicetree/bindings/mux/mmio-mux.txt
@@ -0,0 +1,56 @@
+MMIO bitfield-based multiplexer controller bindings
+
+Define a syscon bitfield to be used to control a multiplexer. The parent
+device tree node must be a syscon node to provide register access.
+
+Required properties:
+- compatible : "gpio-mux"
+- reg : register base of the register containing the control bitfield
+- bit-mask : bitmask of the control bitfield in the control register
+- bit-shift : bit offset of the control bitfield in the control register
+- #mux-control-cells : <0>
+* Standard mux-controller bindings as decribed in mux-controller.txt
+
+Optional properties:
+- idle-state : if present, the state the mux will have when idle. The
+	       special state MUX_IDLE_AS_IS is the default.
+
+The multiplexer state is defined as the value of the bitfield described
+by the reg, bit-mask, and bit-shift properties, accessed through the parent
+syscon.
+
+Example:
+
+	syscon {
+		compatible = "syscon";
+
+		mux: mux-controller@3 {
+			compatible = "mmio-mux";
+			reg = <0x3>;
+			bit-mask = <0x1>;
+			bit-shift = <5>;
+			#mux-control-cells = <0>;
+		};
+	};
+
+	video-mux {
+		compatible = "video-mux";
+		mux-controls = <&mux>;
+
+		ports {
+			/* input 0 */
+			port@0 {
+				reg = <0>;
+			};
+
+			/* input 1 */
+			port@1 {
+				reg = <1>;
+			};
+
+			/* output */
+			port@2 {
+				reg = <2>;
+			};
+		};
+	};
-- 
2.11.0

[toc] | [next] | [standalone]

#1623449 — Re: [RFC 1/2] dt-bindings: add mmio-based syscon mux controller DT bindings

On 04/13/2017 08:48 AM, Philipp Zabel wrote:
> This adds device tree binding documentation for mmio-based syscon
> multiplexers controlled by a single bitfield in a syscon register
> range.

Nice! (you beat me to it, I was about to embark on this myself :)

Looks good to me, just some minor comments below.

>
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
>   Documentation/devicetree/bindings/mux/mmio-mux.txt | 56 ++++++++++++++++++++++
>   1 file changed, 56 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/mux/mmio-mux.txt
>
> diff --git a/Documentation/devicetree/bindings/mux/mmio-mux.txt b/Documentation/devicetree/bindings/mux/mmio-mux.txt
> new file mode 100644
> index 0000000000000..11d96f5d98583
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mux/mmio-mux.txt
> @@ -0,0 +1,56 @@
> +MMIO bitfield-based multiplexer controller bindings
> +
> +Define a syscon bitfield to be used to control a multiplexer. The parent

I think "Define a register bitfield to be used ..." is more clear.

> +device tree node must be a syscon node to provide register access.
> +
> +Required properties:
> +- compatible : "gpio-mux"

Er, "mmio-mux"

> +- reg : register base of the register containing the control bitfield
> +- bit-mask : bitmask of the control bitfield in the control register
> +- bit-shift : bit offset of the control bitfield in the control register
> +- #mux-control-cells : <0>
> +* Standard mux-controller bindings as decribed in mux-controller.txt
> +
> +Optional properties:
> +- idle-state : if present, the state the mux will have when idle. The
> +	       special state MUX_IDLE_AS_IS is the default.
> +
> +The multiplexer state is defined as the value of the bitfield described
> +by the reg, bit-mask, and bit-shift properties, accessed through the parent
> +syscon.
> +
> +Example:
> +
> +	syscon {
> +		compatible = "syscon";
> +
> +		mux: mux-controller@3 {
> +			compatible = "mmio-mux";
> +			reg = <0x3>;
> +			bit-mask = <0x1>;
> +			bit-shift = <5>;
> +			#mux-control-cells = <0>;
> +		};
> +	};
> +
> +	video-mux {

I like this as an example consumer of a mmio-mux, but just
the same some might argue this doesn't really fit here.

Steve

> +		compatible = "video-mux";
> +		mux-controls = <&mux>;
> +
> +		ports {
> +			/* input 0 */
> +			port@0 {
> +				reg = <0>;
> +			};
> +
> +			/* input 1 */
> +			port@1 {
> +				reg = <1>;
> +			};
> +
> +			/* output */
> +			port@2 {
> +				reg = <2>;
> +			};
> +		};
> +	};

[toc] | [prev] | [next] | [standalone]

#1626041 — Re: [RFC 1/2] dt-bindings: add mmio-based syscon mux controller DT bindings

On Thu, 2017-04-13 at 18:03 -0700, Steve Longerbeam wrote:
> 
> On 04/13/2017 08:48 AM, Philipp Zabel wrote:
> > This adds device tree binding documentation for mmio-based syscon
> > multiplexers controlled by a single bitfield in a syscon register
> > range.
> 
> Nice! (you beat me to it, I was about to embark on this myself :)
> 
> Looks good to me, just some minor comments below.
> 
[...]
> > +Define a syscon bitfield to be used to control a multiplexer. The parent
> I think "Define a register bitfield to be used ..." is more clear.
[...]
> > +- compatible : "gpio-mux"
> Er, "mmio-mux"

I'll change those, thanks.

[...]
> > +Example:
> > +
> > +	syscon {
> > +		compatible = "syscon";
> > +
> > +		mux: mux-controller@3 {
> > +			compatible = "mmio-mux";
> > +			reg = <0x3>;
> > +			bit-mask = <0x1>;
> > +			bit-shift = <5>;
> > +			#mux-control-cells = <0>;
> > +		};
> > +	};
> > +
> > +	video-mux {
> 
> I like this as an example consumer of a mmio-mux, but just
> the same some might argue this doesn't really fit here.

I don't think this is a problem, of course assuming that this video-mux
binding will actually come into existence.

regards
Philipp

[toc] | [prev] | [next] | [standalone]

#1625128 — Re: [RFC 1/2] dt-bindings: add mmio-based syscon mux controller DT bindings

On Thu, 2017-04-13 at 17:48 +0200, Philipp Zabel wrote:
> This adds device tree binding documentation for mmio-based syscon
> multiplexers controlled by a single bitfield in a syscon register
> range.
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
>  Documentation/devicetree/bindings/mux/mmio-mux.txt | 56 ++++++++++++++++++++++
>  1 file changed, 56 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mux/mmio-mux.txt
> 
> diff --git a/Documentation/devicetree/bindings/mux/mmio-mux.txt b/Documentation/devicetree/bindings/mux/mmio-mux.txt
> new file mode 100644
> index 0000000000000..11d96f5d98583
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mux/mmio-mux.txt
> @@ -0,0 +1,56 @@
> +MMIO bitfield-based multiplexer controller bindings
> +
> +Define a syscon bitfield to be used to control a multiplexer. The parent
> +device tree node must be a syscon node to provide register access.
> +
> +Required properties:
> +- compatible : "gpio-mux"
> +- reg : register base of the register containing the control bitfield
> +- bit-mask : bitmask of the control bitfield in the control register
> +- bit-shift : bit offset of the control bitfield in the control register
> +- #mux-control-cells : <0>
> +* Standard mux-controller bindings as decribed in mux-controller.txt
> +
> +Optional properties:
> +- idle-state : if present, the state the mux will have when idle. The
> +	       special state MUX_IDLE_AS_IS is the default.
> +
> +The multiplexer state is defined as the value of the bitfield described
> +by the reg, bit-mask, and bit-shift properties, accessed through the parent
> +syscon.
> +
> +Example:
> +
> +	syscon {
> +		compatible = "syscon";
> +
> +		mux: mux-controller@3 {
> +			compatible = "mmio-mux";
> +			reg = <0x3>;
> +			bit-mask = <0x1>;
> +			bit-shift = <5>;
> +			#mux-control-cells = <0>;
> +		};
> +	};
> +
> +	video-mux {
> +		compatible = "video-mux";
> +		mux-controls = <&mux>;
> +
> +		ports {
> +			/* input 0 */
> +			port@0 {
> +				reg = <0>;
> +			};
> +
> +			/* input 1 */
> +			port@1 {
> +				reg = <1>;
> +			};
> +
> +			/* output */
> +			port@2 {
> +				reg = <2>;
> +			};
> +		};
> +	};

So Pavel (added to Cc:) suggested to merge these into one node for the
video mux, as really we are describing a single hardware entity that
happens to be multiplexing multiple video buses into one:

	syscon {
		compatible = "syscon";

		/* video multiplexer */
		mux: mux-controller@3 {
			compatible = "video-mmio-mux";
			reg = <0x3>;
			bit-mask = <0x1>;
			bit-shift = <5>;
			#mux-control-cells = <0>;
 
			mux-controls = <&mux>;

			ports {
				/* input 0 */
				port@0 {
					reg = <0>;
				};

				/* input 1 */
				port@1 {
					reg = <1>;
				};

				/* output */
				port@2 {
					reg = <2>;
				};
			};
		};
	};

That would not touch on this "general purpose" mmio-mux binding itself,
but would make it necessary to add a separate "video-mmio-mux" and a
"video-gpio-mux" binding that mirror the "mmio-mux" and "gpio-mux"
bindings but add the OF-graph connections.

Also I think in this case the self-referencing mux-controls property
would be superfluous, as the driver binding to this node is expected to
control the mux according to activation of the links described by the
OF-graph bindings.

regards
Philipp

[toc] | [prev] | [next] | [standalone]

#1625206 — Re: [RFC 1/2] dt-bindings: add mmio-based syscon mux controller DT bindings

Hi Philipp,

On Tue, Apr 18, 2017 at 10:19:04AM +0200, Philipp Zabel wrote:
> On Thu, 2017-04-13 at 17:48 +0200, Philipp Zabel wrote:
> > This adds device tree binding documentation for mmio-based syscon
> > multiplexers controlled by a single bitfield in a syscon register
> > range.
> > 
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > ---
> >  Documentation/devicetree/bindings/mux/mmio-mux.txt | 56 ++++++++++++++++++++++
> >  1 file changed, 56 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/mux/mmio-mux.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/mux/mmio-mux.txt b/Documentation/devicetree/bindings/mux/mmio-mux.txt
> > new file mode 100644
> > index 0000000000000..11d96f5d98583
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mux/mmio-mux.txt
> > @@ -0,0 +1,56 @@
> > +MMIO bitfield-based multiplexer controller bindings
> > +
> > +Define a syscon bitfield to be used to control a multiplexer. The parent
> > +device tree node must be a syscon node to provide register access.
> > +
> > +Required properties:
> > +- compatible : "gpio-mux"
> > +- reg : register base of the register containing the control bitfield
> > +- bit-mask : bitmask of the control bitfield in the control register
> > +- bit-shift : bit offset of the control bitfield in the control register
> > +- #mux-control-cells : <0>
> > +* Standard mux-controller bindings as decribed in mux-controller.txt
> > +
> > +Optional properties:
> > +- idle-state : if present, the state the mux will have when idle. The
> > +	       special state MUX_IDLE_AS_IS is the default.
> > +
> > +The multiplexer state is defined as the value of the bitfield described
> > +by the reg, bit-mask, and bit-shift properties, accessed through the parent
> > +syscon.
> > +
> > +Example:
> > +
> > +	syscon {
> > +		compatible = "syscon";
> > +
> > +		mux: mux-controller@3 {
> > +			compatible = "mmio-mux";
> > +			reg = <0x3>;
> > +			bit-mask = <0x1>;
> > +			bit-shift = <5>;
> > +			#mux-control-cells = <0>;
> > +		};
> > +	};
> > +
> > +	video-mux {
> > +		compatible = "video-mux";
> > +		mux-controls = <&mux>;
> > +
> > +		ports {
> > +			/* input 0 */
> > +			port@0 {
> > +				reg = <0>;
> > +			};
> > +
> > +			/* input 1 */
> > +			port@1 {
> > +				reg = <1>;
> > +			};
> > +
> > +			/* output */
> > +			port@2 {
> > +				reg = <2>;
> > +			};
> > +		};
> > +	};
> 
> So Pavel (added to Cc:) suggested to merge these into one node for the
> video mux, as really we are describing a single hardware entity that
> happens to be multiplexing multiple video buses into one:

Two drivers will be needed in a way or another to disconnect the dependency
between the video switch driver and the MUX implementation. Are there ways
to do that cleanly other than having two devices?

And if there are two devices, shouldn't the video switch device be a child
of the MUX device? I think it'd be odd to have it hanging around in a
completely unrelated part of the device tree.

> 
> 	syscon {
> 		compatible = "syscon";
> 
> 		/* video multiplexer */
> 		mux: mux-controller@3 {
> 			compatible = "video-mmio-mux";
> 			reg = <0x3>;
> 			bit-mask = <0x1>;
> 			bit-shift = <5>;
> 			#mux-control-cells = <0>;
>  
> 			mux-controls = <&mux>;
> 
> 			ports {
> 				/* input 0 */
> 				port@0 {
> 					reg = <0>;
> 				};
> 
> 				/* input 1 */
> 				port@1 {
> 					reg = <1>;
> 				};
> 
> 				/* output */
> 				port@2 {
> 					reg = <2>;
> 				};
> 			};
> 		};
> 	};
> 
> That would not touch on this "general purpose" mmio-mux binding itself,
> but would make it necessary to add a separate "video-mmio-mux" and a
> "video-gpio-mux" binding that mirror the "mmio-mux" and "gpio-mux"
> bindings but add the OF-graph connections.
> 
> Also I think in this case the self-referencing mux-controls property
> would be superfluous, as the driver binding to this node is expected to
> control the mux according to activation of the links described by the
> OF-graph bindings.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

[toc] | [prev] | [next] | [standalone]

#1625225 — Re: [RFC 1/2] dt-bindings: add mmio-based syscon mux controller DT bindings

[Multipart message — attachments visible in raw view] — view raw

On Tue 2017-04-18 13:08:41, Sakari Ailus wrote:
> Hi Philipp,
> 
> On Tue, Apr 18, 2017 at 10:19:04AM +0200, Philipp Zabel wrote:
> > On Thu, 2017-04-13 at 17:48 +0200, Philipp Zabel wrote:
> > > This adds device tree binding documentation for mmio-based syscon
> > > multiplexers controlled by a single bitfield in a syscon register
> > > range.
> > > 
> > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > > ---
> > >  Documentation/devicetree/bindings/mux/mmio-mux.txt | 56 ++++++++++++++++++++++
> > >  1 file changed, 56 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/mux/mmio-mux.txt
> > > 
> > > diff --git a/Documentation/devicetree/bindings/mux/mmio-mux.txt b/Documentation/devicetree/bindings/mux/mmio-mux.txt
> > > new file mode 100644
> > > index 0000000000000..11d96f5d98583
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/mux/mmio-mux.txt
> > > @@ -0,0 +1,56 @@
> > > +MMIO bitfield-based multiplexer controller bindings
> > > +
> > > +Define a syscon bitfield to be used to control a multiplexer. The parent
> > > +device tree node must be a syscon node to provide register access.
> > > +
> > > +Required properties:
> > > +- compatible : "gpio-mux"
> > > +- reg : register base of the register containing the control bitfield
> > > +- bit-mask : bitmask of the control bitfield in the control register
> > > +- bit-shift : bit offset of the control bitfield in the control register
> > > +- #mux-control-cells : <0>
> > > +* Standard mux-controller bindings as decribed in mux-controller.txt
> > > +
> > > +Optional properties:
> > > +- idle-state : if present, the state the mux will have when idle. The
> > > +	       special state MUX_IDLE_AS_IS is the default.
> > > +
> > > +The multiplexer state is defined as the value of the bitfield described
> > > +by the reg, bit-mask, and bit-shift properties, accessed through the parent
> > > +syscon.
> > > +
> > > +Example:
> > > +
> > > +	syscon {
> > > +		compatible = "syscon";
> > > +
> > > +		mux: mux-controller@3 {
> > > +			compatible = "mmio-mux";
> > > +			reg = <0x3>;
> > > +			bit-mask = <0x1>;
> > > +			bit-shift = <5>;
> > > +			#mux-control-cells = <0>;
> > > +		};
> > > +	};
> > > +
> > > +	video-mux {
> > > +		compatible = "video-mux";
> > > +		mux-controls = <&mux>;
> > > +
> > > +		ports {
> > > +			/* input 0 */
> > > +			port@0 {
> > > +				reg = <0>;
> > > +			};
> > > +
> > > +			/* input 1 */
> > > +			port@1 {
> > > +				reg = <1>;
> > > +			};
> > > +
> > > +			/* output */
> > > +			port@2 {
> > > +				reg = <2>;
> > > +			};
> > > +		};
> > > +	};
> > 
> > So Pavel (added to Cc:) suggested to merge these into one node for the
> > video mux, as really we are describing a single hardware entity that
> > happens to be multiplexing multiple video buses into one:
> 
> Two drivers will be needed in a way or another to disconnect the dependency
> between the video switch driver and the MUX implementation. Are there ways
> to do that cleanly other than having two devices?

Yes.

Device tree describes hardware, not the driver structure.

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [prev] | [next] | [standalone]

#1625249 — Re: [RFC 1/2] dt-bindings: add mmio-based syscon mux controller DT bindings

On Tue, Apr 18, 2017 at 12:34:53PM +0200, Pavel Machek wrote:
> On Tue 2017-04-18 13:08:41, Sakari Ailus wrote:
> > Hi Philipp,
> > 
> > On Tue, Apr 18, 2017 at 10:19:04AM +0200, Philipp Zabel wrote:
> > > On Thu, 2017-04-13 at 17:48 +0200, Philipp Zabel wrote:
> > > > This adds device tree binding documentation for mmio-based syscon
> > > > multiplexers controlled by a single bitfield in a syscon register
> > > > range.
> > > > 
> > > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > > > ---
> > > >  Documentation/devicetree/bindings/mux/mmio-mux.txt | 56 ++++++++++++++++++++++
> > > >  1 file changed, 56 insertions(+)
> > > >  create mode 100644 Documentation/devicetree/bindings/mux/mmio-mux.txt
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/mux/mmio-mux.txt b/Documentation/devicetree/bindings/mux/mmio-mux.txt
> > > > new file mode 100644
> > > > index 0000000000000..11d96f5d98583
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/mux/mmio-mux.txt
> > > > @@ -0,0 +1,56 @@
> > > > +MMIO bitfield-based multiplexer controller bindings
> > > > +
> > > > +Define a syscon bitfield to be used to control a multiplexer. The parent
> > > > +device tree node must be a syscon node to provide register access.
> > > > +
> > > > +Required properties:
> > > > +- compatible : "gpio-mux"
> > > > +- reg : register base of the register containing the control bitfield
> > > > +- bit-mask : bitmask of the control bitfield in the control register
> > > > +- bit-shift : bit offset of the control bitfield in the control register
> > > > +- #mux-control-cells : <0>
> > > > +* Standard mux-controller bindings as decribed in mux-controller.txt
> > > > +
> > > > +Optional properties:
> > > > +- idle-state : if present, the state the mux will have when idle. The
> > > > +	       special state MUX_IDLE_AS_IS is the default.
> > > > +
> > > > +The multiplexer state is defined as the value of the bitfield described
> > > > +by the reg, bit-mask, and bit-shift properties, accessed through the parent
> > > > +syscon.
> > > > +
> > > > +Example:
> > > > +
> > > > +	syscon {
> > > > +		compatible = "syscon";
> > > > +
> > > > +		mux: mux-controller@3 {
> > > > +			compatible = "mmio-mux";
> > > > +			reg = <0x3>;
> > > > +			bit-mask = <0x1>;
> > > > +			bit-shift = <5>;
> > > > +			#mux-control-cells = <0>;
> > > > +		};
> > > > +	};
> > > > +
> > > > +	video-mux {
> > > > +		compatible = "video-mux";
> > > > +		mux-controls = <&mux>;
> > > > +
> > > > +		ports {
> > > > +			/* input 0 */
> > > > +			port@0 {
> > > > +				reg = <0>;
> > > > +			};
> > > > +
> > > > +			/* input 1 */
> > > > +			port@1 {
> > > > +				reg = <1>;
> > > > +			};
> > > > +
> > > > +			/* output */
> > > > +			port@2 {
> > > > +				reg = <2>;
> > > > +			};
> > > > +		};
> > > > +	};
> > > 
> > > So Pavel (added to Cc:) suggested to merge these into one node for the
> > > video mux, as really we are describing a single hardware entity that
> > > happens to be multiplexing multiple video buses into one:
> > 
> > Two drivers will be needed in a way or another to disconnect the dependency
> > between the video switch driver and the MUX implementation. Are there ways
> > to do that cleanly other than having two devices?
> 
> Yes.
> 
> Device tree describes hardware, not the driver structure.

I think you you could view the MUX control as a device, too, and that's
separate from the actual video switch.

This isn't really related to the video switch as much as it's got to do with
the MUX framework, so having Peter's opinion here would be very helpful.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

[toc] | [prev] | [next] | [standalone]

#1625286 — Re: [RFC 1/2] dt-bindings: add mmio-based syscon mux controller DT bindings

[Multipart message — attachments visible in raw view] — view raw

On Tue 2017-04-18 13:55:40, Sakari Ailus wrote:
> On Tue, Apr 18, 2017 at 12:34:53PM +0200, Pavel Machek wrote:
> > On Tue 2017-04-18 13:08:41, Sakari Ailus wrote:
> > > Hi Philipp,
> > > 
> > > On Tue, Apr 18, 2017 at 10:19:04AM +0200, Philipp Zabel wrote:
> > > > On Thu, 2017-04-13 at 17:48 +0200, Philipp Zabel wrote:
> > > > > This adds device tree binding documentation for mmio-based syscon
> > > > > multiplexers controlled by a single bitfield in a syscon register
> > > > > range.
> > > > > 
> > > > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > > > > ---
> > > > >  Documentation/devicetree/bindings/mux/mmio-mux.txt | 56 ++++++++++++++++++++++
> > > > >  1 file changed, 56 insertions(+)
> > > > >  create mode 100644 Documentation/devicetree/bindings/mux/mmio-mux.txt
> > > > > 
> > > > > diff --git a/Documentation/devicetree/bindings/mux/mmio-mux.txt b/Documentation/devicetree/bindings/mux/mmio-mux.txt
> > > > > new file mode 100644
> > > > > index 0000000000000..11d96f5d98583
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/mux/mmio-mux.txt
> > > > > @@ -0,0 +1,56 @@
> > > > > +MMIO bitfield-based multiplexer controller bindings
> > > > > +
> > > > > +Define a syscon bitfield to be used to control a multiplexer. The parent
> > > > > +device tree node must be a syscon node to provide register access.
> > > > > +
> > > > > +Required properties:
> > > > > +- compatible : "gpio-mux"
> > > > > +- reg : register base of the register containing the control bitfield
> > > > > +- bit-mask : bitmask of the control bitfield in the control register
> > > > > +- bit-shift : bit offset of the control bitfield in the control register
> > > > > +- #mux-control-cells : <0>
> > > > > +* Standard mux-controller bindings as decribed in mux-controller.txt
> > > > > +
> > > > > +Optional properties:
> > > > > +- idle-state : if present, the state the mux will have when idle. The
> > > > > +	       special state MUX_IDLE_AS_IS is the default.
> > > > > +
> > > > > +The multiplexer state is defined as the value of the bitfield described
> > > > > +by the reg, bit-mask, and bit-shift properties, accessed through the parent
> > > > > +syscon.
> > > > > +
> > > > > +Example:
> > > > > +
> > > > > +	syscon {
> > > > > +		compatible = "syscon";
> > > > > +
> > > > > +		mux: mux-controller@3 {
> > > > > +			compatible = "mmio-mux";
> > > > > +			reg = <0x3>;
> > > > > +			bit-mask = <0x1>;
> > > > > +			bit-shift = <5>;
> > > > > +			#mux-control-cells = <0>;
> > > > > +		};
> > > > > +	};
> > > > > +
> > > > > +	video-mux {
> > > > > +		compatible = "video-mux";
> > > > > +		mux-controls = <&mux>;
> > > > > +
> > > > > +		ports {
> > > > > +			/* input 0 */
> > > > > +			port@0 {
> > > > > +				reg = <0>;
> > > > > +			};
> > > > > +
> > > > > +			/* input 1 */
> > > > > +			port@1 {
> > > > > +				reg = <1>;
> > > > > +			};
> > > > > +
> > > > > +			/* output */
> > > > > +			port@2 {
> > > > > +				reg = <2>;
> > > > > +			};
> > > > > +		};
> > > > > +	};
> > > > 
> > > > So Pavel (added to Cc:) suggested to merge these into one node for the
> > > > video mux, as really we are describing a single hardware entity that
> > > > happens to be multiplexing multiple video buses into one:
> > > 
> > > Two drivers will be needed in a way or another to disconnect the dependency
> > > between the video switch driver and the MUX implementation. Are there ways
> > > to do that cleanly other than having two devices?
> > 
> > Yes.
> > 
> > Device tree describes hardware, not the driver structure.
> 
> I think you you could view the MUX control as a device, too, and that's
> separate from the actual video switch.

Actually, I believe what matters here is hardware. There's some chip,
somewhere, that does the switching, and the device tree should should
basically describe that switch.


									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [prev] | [next] | [standalone]

#1625248 — Re: [RFC 1/2] dt-bindings: add mmio-based syscon mux controller DT bindings

On Tue, 2017-04-18 at 13:08 +0300, Sakari Ailus wrote:
> Hi Philipp,
> 
> On Tue, Apr 18, 2017 at 10:19:04AM +0200, Philipp Zabel wrote:
> > On Thu, 2017-04-13 at 17:48 +0200, Philipp Zabel wrote:
> > > This adds device tree binding documentation for mmio-based syscon
> > > multiplexers controlled by a single bitfield in a syscon register
> > > range.
> > > 
> > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > > ---
> > >  Documentation/devicetree/bindings/mux/mmio-mux.txt | 56 ++++++++++++++++++++++
> > >  1 file changed, 56 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/mux/mmio-mux.txt
> > > 
> > > diff --git a/Documentation/devicetree/bindings/mux/mmio-mux.txt b/Documentation/devicetree/bindings/mux/mmio-mux.txt
> > > new file mode 100644
> > > index 0000000000000..11d96f5d98583
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/mux/mmio-mux.txt
> > > @@ -0,0 +1,56 @@
> > > +MMIO bitfield-based multiplexer controller bindings
> > > +
> > > +Define a syscon bitfield to be used to control a multiplexer. The parent
> > > +device tree node must be a syscon node to provide register access.
> > > +
> > > +Required properties:
> > > +- compatible : "gpio-mux"
> > > +- reg : register base of the register containing the control bitfield
> > > +- bit-mask : bitmask of the control bitfield in the control register
> > > +- bit-shift : bit offset of the control bitfield in the control register
> > > +- #mux-control-cells : <0>
> > > +* Standard mux-controller bindings as decribed in mux-controller.txt
> > > +
> > > +Optional properties:
> > > +- idle-state : if present, the state the mux will have when idle. The
> > > +	       special state MUX_IDLE_AS_IS is the default.
> > > +
> > > +The multiplexer state is defined as the value of the bitfield described
> > > +by the reg, bit-mask, and bit-shift properties, accessed through the parent
> > > +syscon.
> > > +
> > > +Example:
> > > +
> > > +	syscon {
> > > +		compatible = "syscon";
> > > +
> > > +		mux: mux-controller@3 {
> > > +			compatible = "mmio-mux";
> > > +			reg = <0x3>;
> > > +			bit-mask = <0x1>;
> > > +			bit-shift = <5>;
> > > +			#mux-control-cells = <0>;
> > > +		};
> > > +	};
> > > +
> > > +	video-mux {
> > > +		compatible = "video-mux";
> > > +		mux-controls = <&mux>;
> > > +
> > > +		ports {
> > > +			/* input 0 */
> > > +			port@0 {
> > > +				reg = <0>;
> > > +			};
> > > +
> > > +			/* input 1 */
> > > +			port@1 {
> > > +				reg = <1>;
> > > +			};
> > > +
> > > +			/* output */
> > > +			port@2 {
> > > +				reg = <2>;
> > > +			};
> > > +		};
> > > +	};
> > 
> > So Pavel (added to Cc:) suggested to merge these into one node for the
> > video mux, as really we are describing a single hardware entity that
> > happens to be multiplexing multiple video buses into one:
> 
> Two drivers will be needed in a way or another to disconnect the dependency
> between the video switch driver and the MUX implementation. Are there ways
> to do that cleanly other than having two devices?

We are talking about the device tree bindings, drivers and devices
shouldn't factor into it yet. A video-mmio-mux driver could very well
create a mmio-mux platform device internally, if necessary. Or it could
just use the same library functions that the mmio-mux driver uses,
without creating a second device.

> And if there are two devices, shouldn't the video switch device be a child
> of the MUX device? I think it'd be odd to have it hanging around in a
> completely unrelated part of the device tree.

That boils down to whether you consider the connection between mux
controller and mux to be resource usage that should be described via a
phandle reference, like pwms, gpios, clocks, and so on, or whether you
consider it to be control flow in the device tree sense, which should be
described as a parent-child relationship of the nodes. The mux framework
is designed around the former.

We could embrace this and consider a syscon region that contains
multiple mux bitfields as one mux controller that controls multiple
muxes, with a binding similar to, for example, the
reset/ti-syscon-reset.txt bindings:

	syscon {
		compatible = "syscon";

		mux: mux-controller@4 {
			compatible = "mmio-mux";

			/* register, bit shift, bit mask */
			mux-bits = <0x4 19 0x1>, /* 0: CSI0 mux */
				   <0x4 20 0x1>; /* 1: CSI1 mux */
			#mux-control-cells = <1>;
		};
	};

	/* somewhere else */

	csi0-mux {
		compatible = "video-mux";
		mux-controls = <&mux 0>;

		ports {
			/* ... */
		};
	};

	csi1-mux {
		compatible = "video-mux";
		mux-controls = <&mux 1>;

		ports {
			/* ... */
		};
	};

regards
Philipp

[toc] | [prev] | [next] | [standalone]

#1626830 — Re: [RFC 1/2] dt-bindings: add mmio-based syscon mux controller DT bindings

On Thu, Apr 13, 2017 at 05:48:11PM +0200, Philipp Zabel wrote:
> This adds device tree binding documentation for mmio-based syscon
> multiplexers controlled by a single bitfield in a syscon register
> range.
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
>  Documentation/devicetree/bindings/mux/mmio-mux.txt | 56 ++++++++++++++++++++++
>  1 file changed, 56 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mux/mmio-mux.txt
> 
> diff --git a/Documentation/devicetree/bindings/mux/mmio-mux.txt b/Documentation/devicetree/bindings/mux/mmio-mux.txt
> new file mode 100644
> index 0000000000000..11d96f5d98583
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mux/mmio-mux.txt
> @@ -0,0 +1,56 @@
> +MMIO bitfield-based multiplexer controller bindings
> +
> +Define a syscon bitfield to be used to control a multiplexer. The parent
> +device tree node must be a syscon node to provide register access.
> +
> +Required properties:
> +- compatible : "gpio-mux"

?

> +- reg : register base of the register containing the control bitfield
> +- bit-mask : bitmask of the control bitfield in the control register
> +- bit-shift : bit offset of the control bitfield in the control register
> +- #mux-control-cells : <0>
> +* Standard mux-controller bindings as decribed in mux-controller.txt
> +
> +Optional properties:
> +- idle-state : if present, the state the mux will have when idle. The
> +	       special state MUX_IDLE_AS_IS is the default.
> +
> +The multiplexer state is defined as the value of the bitfield described
> +by the reg, bit-mask, and bit-shift properties, accessed through the parent
> +syscon.
> +
> +Example:
> +
> +	syscon {
> +		compatible = "syscon";
> +
> +		mux: mux-controller@3 {
> +			compatible = "mmio-mux";
> +			reg = <0x3>;
> +			bit-mask = <0x1>;
> +			bit-shift = <5>;

This pattern doesn't scale once you have multiple fields @ addr 3. I 
also don't really think a node per register field in DT really scales.

I think the parent should be declared as a mux controller instead. You 
could encode the mux addr and bit position in the mux cells.

> +			#mux-control-cells = <0>;
> +		};
> +	};
> +
> +	video-mux {
> +		compatible = "video-mux";
> +		mux-controls = <&mux>;

The mux binding was largely defined for a single control controling 
multiple muxes. This doesn't really fit that, but I guess this is an 
improvement over a custom syscon phandle.

> +
> +		ports {
> +			/* input 0 */
> +			port@0 {
> +				reg = <0>;
> +			};
> +
> +			/* input 1 */
> +			port@1 {
> +				reg = <1>;
> +			};
> +
> +			/* output */
> +			port@2 {
> +				reg = <2>;
> +			};
> +		};
> +	};
> -- 
> 2.11.0
> 

[toc] | [prev] | [next] | [standalone]

#1627185 — Re: [RFC 1/2] dt-bindings: add mmio-based syscon mux controller DT bindings

Hi Rob,

On Wed, 2017-04-19 at 17:09 -0500, Rob Herring wrote:
> On Thu, Apr 13, 2017 at 05:48:11PM +0200, Philipp Zabel wrote:
> > This adds device tree binding documentation for mmio-based syscon
> > multiplexers controlled by a single bitfield in a syscon register
> > range.
> > 
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > ---
> >  Documentation/devicetree/bindings/mux/mmio-mux.txt | 56 ++++++++++++++++++++++
> >  1 file changed, 56 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/mux/mmio-mux.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/mux/mmio-mux.txt b/Documentation/devicetree/bindings/mux/mmio-mux.txt
> > new file mode 100644
> > index 0000000000000..11d96f5d98583
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mux/mmio-mux.txt
> > @@ -0,0 +1,56 @@
> > +MMIO bitfield-based multiplexer controller bindings
> > +
> > +Define a syscon bitfield to be used to control a multiplexer. The parent
> > +device tree node must be a syscon node to provide register access.
> > +
> > +Required properties:
> > +- compatible : "gpio-mux"
> 
> ?
>
> > +- reg : register base of the register containing the control bitfield
> > +- bit-mask : bitmask of the control bitfield in the control register
> > +- bit-shift : bit offset of the control bitfield in the control register
> > +- #mux-control-cells : <0>
> > +* Standard mux-controller bindings as decribed in mux-controller.txt
> > +
> > +Optional properties:
> > +- idle-state : if present, the state the mux will have when idle. The
> > +	       special state MUX_IDLE_AS_IS is the default.
> > +
> > +The multiplexer state is defined as the value of the bitfield described
> > +by the reg, bit-mask, and bit-shift properties, accessed through the parent
> > +syscon.
> > +
> > +Example:
> > +
> > +	syscon {
> > +		compatible = "syscon";
> > +
> > +		mux: mux-controller@3 {
> > +			compatible = "mmio-mux";
> > +			reg = <0x3>;
> > +			bit-mask = <0x1>;
> > +			bit-shift = <5>;
> 
> This pattern doesn't scale once you have multiple fields @ addr 3. I 
> also don't really think a node per register field in DT really scales.

Thanks, I have been a bit uneasy with the separate per-bitfield mux
controller node, so I'm eager to agree. But thit makes me unsure how to
best represent the information that is spelled out above.

> I think the parent should be declared as a mux controller instead.

The syscon node itself should be the mux controller? Would you expect
the mmio-mux driver bind to the syscon node, or should the mux framework
handle creation of the mux controls in this case (i.e. does the syscon
node get a "mmio-mux" added to its compatible list)?

> You could encode the mux addr and bit position in the mux cells.

What about the bit-mask / bitfield width? Just add a cell for it?

	gpr: syscon {
		compatible = "mmio-mux", "syscon", "simple-mfd";
		#mux-control-cells = <3>;

		video-mux {
			compatible = "video-mux";
			/* register 0x3, bits [6:5] */
			mux-controls = <&gpr 0x3 5 0x3>;

			ports {
				/* ports 0..5 */
			};
		};
	};

Or maybe using MSB and LSB would be better to read?

		video-mux {
			/* register 0x3, bits [6:5] */
			mux-control = <&gpr 0x3 6 5>;

			ports {
				/* ports 0..5 */
			};
		};

> > +			#mux-control-cells = <0>;
> > +		};
> > +	};
> > +
> > +	video-mux {
> > +		compatible = "video-mux";
> > +		mux-controls = <&mux>;
> 
> The mux binding was largely defined for a single control controling 
> multiple muxes. This doesn't really fit that, but I guess this is an 
> improvement over a custom syscon phandle.

What I especially like about the mux-controls property is that would
allow me to use the gpio-mux driver (or any other mux controller)
instead of having to code variants of the video-mux for all possible
control schemes.

regards
Philipp

[toc] | [prev] | [next] | [standalone]

#1627427 — Re: [RFC 1/2] dt-bindings: add mmio-based syscon mux controller DT bindings

On Thu, 2017-04-20 at 13:57 +0200, Peter Rosin wrote:
> On 2017-04-20 10:14, Philipp Zabel wrote:
> > Hi Rob,
> > 
> > On Wed, 2017-04-19 at 17:09 -0500, Rob Herring wrote:
> >> On Thu, Apr 13, 2017 at 05:48:11PM +0200, Philipp Zabel wrote:
> >>> This adds device tree binding documentation for mmio-based syscon
> >>> multiplexers controlled by a single bitfield in a syscon register
> >>> range.
> >>>
> >>> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> >>> ---
> >>>  Documentation/devicetree/bindings/mux/mmio-mux.txt | 56 ++++++++++++++++++++++
> >>>  1 file changed, 56 insertions(+)
> >>>  create mode 100644 Documentation/devicetree/bindings/mux/mmio-mux.txt
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/mux/mmio-mux.txt b/Documentation/devicetree/bindings/mux/mmio-mux.txt
> >>> new file mode 100644
> >>> index 0000000000000..11d96f5d98583
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/mux/mmio-mux.txt
> >>> @@ -0,0 +1,56 @@
> >>> +MMIO bitfield-based multiplexer controller bindings
> >>> +
> >>> +Define a syscon bitfield to be used to control a multiplexer. The parent
> >>> +device tree node must be a syscon node to provide register access.
> >>> +
> >>> +Required properties:
> >>> +- compatible : "gpio-mux"
> >>
> >> ?
> >>
> >>> +- reg : register base of the register containing the control bitfield
> >>> +- bit-mask : bitmask of the control bitfield in the control register
> >>> +- bit-shift : bit offset of the control bitfield in the control register
> >>> +- #mux-control-cells : <0>
> >>> +* Standard mux-controller bindings as decribed in mux-controller.txt
> >>> +
> >>> +Optional properties:
> >>> +- idle-state : if present, the state the mux will have when idle. The
> >>> +	       special state MUX_IDLE_AS_IS is the default.
> >>> +
> >>> +The multiplexer state is defined as the value of the bitfield described
> >>> +by the reg, bit-mask, and bit-shift properties, accessed through the parent
> >>> +syscon.
> >>> +
> >>> +Example:
> >>> +
> >>> +	syscon {
> >>> +		compatible = "syscon";
> >>> +
> >>> +		mux: mux-controller@3 {
> >>> +			compatible = "mmio-mux";
> >>> +			reg = <0x3>;
> >>> +			bit-mask = <0x1>;
> >>> +			bit-shift = <5>;
> >>
> >> This pattern doesn't scale once you have multiple fields @ addr 3. I 
> >> also don't really think a node per register field in DT really scales.
> > 
> > Thanks, I have been a bit uneasy with the separate per-bitfield mux
> > controller node, so I'm eager to agree. But thit makes me unsure how to
> > best represent the information that is spelled out above.
> > 
> >> I think the parent should be declared as a mux controller instead.
> > 
> > The syscon node itself should be the mux controller? Would you expect
> > the mmio-mux driver bind to the syscon node, or should the mux framework
> > handle creation of the mux controls in this case (i.e. does the syscon
> > node get a "mmio-mux" added to its compatible list)?
> > 
> >> You could encode the mux addr and bit position in the mux cells.
> > 
> > What about the bit-mask / bitfield width? Just add a cell for it?
> > 
> > 	gpr: syscon {
> > 		compatible = "mmio-mux", "syscon", "simple-mfd";
> > 		#mux-control-cells = <3>;
> > 
> > 		video-mux {
> > 			compatible = "video-mux";
> > 			/* register 0x3, bits [6:5] */
> > 			mux-controls = <&gpr 0x3 5 0x3>;
> > 
> > 			ports {
> > 				/* ports 0..5 */
> > 			};
> > 		};
> > 	};
> > 
> > Or maybe using MSB and LSB would be better to read?
> > 
> > 		video-mux {
> > 			/* register 0x3, bits [6:5] */
> > 			mux-control = <&gpr 0x3 6 5>;
> > 
> > 			ports {
> > 				/* ports 0..5 */
> > 			};
> > 		};
> 
> Why do you need three values for one register+field? The shift can be
> implied from the mask, if the mask is pre-shifted. I.e. specifying a
> mask of 0x60 in this case. What I'm I missing?

As long as we have <= 32-bit hardware registers, that would work.
The question then is if things like
	mux-control = <&gpr 0x04 0x00300000>;
are considered readable/reviewable enough. And what happens when we get
64-bit general purpose registers containing muxes? Also a binding like
this would allow non-contiguous bit masks.
The reason I suggested using bit-shift in the first place was that there
are already other bindings using "bit-shift" or "reg-shift" properties.

regards
Philipp

[toc] | [prev] | [next] | [standalone]

#1627471 — Re: [RFC 1/2] dt-bindings: add mmio-based syscon mux controller DT bindings

On Thu, Apr 20, 2017 at 10:14:08AM +0200, Philipp Zabel wrote:
> Hi Rob,
> 
> On Wed, 2017-04-19 at 17:09 -0500, Rob Herring wrote:
> > On Thu, Apr 13, 2017 at 05:48:11PM +0200, Philipp Zabel wrote:
> > > This adds device tree binding documentation for mmio-based syscon
> > > multiplexers controlled by a single bitfield in a syscon register
> > > range.
> > > 
> > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > > ---
> > >  Documentation/devicetree/bindings/mux/mmio-mux.txt | 56 ++++++++++++++++++++++
> > >  1 file changed, 56 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/mux/mmio-mux.txt
> > > 
> > > diff --git a/Documentation/devicetree/bindings/mux/mmio-mux.txt b/Documentation/devicetree/bindings/mux/mmio-mux.txt
> > > new file mode 100644
> > > index 0000000000000..11d96f5d98583
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/mux/mmio-mux.txt
> > > @@ -0,0 +1,56 @@
> > > +MMIO bitfield-based multiplexer controller bindings
> > > +
> > > +Define a syscon bitfield to be used to control a multiplexer. The parent
> > > +device tree node must be a syscon node to provide register access.
> > > +
> > > +Required properties:
> > > +- compatible : "gpio-mux"
> > 
> > ?
> >
> > > +- reg : register base of the register containing the control bitfield
> > > +- bit-mask : bitmask of the control bitfield in the control register
> > > +- bit-shift : bit offset of the control bitfield in the control register
> > > +- #mux-control-cells : <0>
> > > +* Standard mux-controller bindings as decribed in mux-controller.txt
> > > +
> > > +Optional properties:
> > > +- idle-state : if present, the state the mux will have when idle. The
> > > +	       special state MUX_IDLE_AS_IS is the default.
> > > +
> > > +The multiplexer state is defined as the value of the bitfield described
> > > +by the reg, bit-mask, and bit-shift properties, accessed through the parent
> > > +syscon.
> > > +
> > > +Example:
> > > +
> > > +	syscon {
> > > +		compatible = "syscon";
> > > +
> > > +		mux: mux-controller@3 {
> > > +			compatible = "mmio-mux";
> > > +			reg = <0x3>;
> > > +			bit-mask = <0x1>;
> > > +			bit-shift = <5>;
> > 
> > This pattern doesn't scale once you have multiple fields @ addr 3. I 
> > also don't really think a node per register field in DT really scales.
> 
> Thanks, I have been a bit uneasy with the separate per-bitfield mux
> controller node, so I'm eager to agree. But thit makes me unsure how to
> best represent the information that is spelled out above.
> 
> > I think the parent should be declared as a mux controller instead.
> 
> The syscon node itself should be the mux controller? Would you expect
> the mmio-mux driver bind to the syscon node, or should the mux framework
> handle creation of the mux controls in this case (i.e. does the syscon
> node get a "mmio-mux" added to its compatible list)?

Using compatibles here doesn't scale either. If we had 30 functions in 
a syscon, we'd have 30 different compatibles. The syscon node should 
have a specific compatible which implies it is a mux controller (as does 
#mux-controll-cells). So I would expect either you add compatibles to 
the mmio-mux driver or you have a specific driver for the syscon that 
in turn registers a mux device. Either way, it doesn't affect the binding.

> 
> > You could encode the mux addr and bit position in the mux cells.
> 
> What about the bit-mask / bitfield width? Just add a cell for it?

As Peter said, just a shifted mask should be enough.

> 	gpr: syscon {
> 		compatible = "mmio-mux", "syscon", "simple-mfd";
> 		#mux-control-cells = <3>;
> 
> 		video-mux {
> 			compatible = "video-mux";
> 			/* register 0x3, bits [6:5] */
> 			mux-controls = <&gpr 0x3 5 0x3>;
> 
> 			ports {
> 				/* ports 0..5 */
> 			};
> 		};
> 	};
> 
> Or maybe using MSB and LSB would be better to read?
> 
> 		video-mux {
> 			/* register 0x3, bits [6:5] */
> 			mux-control = <&gpr 0x3 6 5>;
> 
> 			ports {
> 				/* ports 0..5 */
> 			};
> 		};
> 
> > > +			#mux-control-cells = <0>;
> > > +		};
> > > +	};
> > > +
> > > +	video-mux {
> > > +		compatible = "video-mux";
> > > +		mux-controls = <&mux>;
> > 
> > The mux binding was largely defined for a single control controling 
> > multiple muxes. This doesn't really fit that, but I guess this is an 
> > improvement over a custom syscon phandle.
> 
> What I especially like about the mux-controls property is that would
> allow me to use the gpio-mux driver (or any other mux controller)
> instead of having to code variants of the video-mux for all possible
> control schemes.

Yes, that's true.

Rob

[toc] | [prev] | [next] | [standalone]

#1627569 — Re: [RFC 1/2] dt-bindings: add mmio-based syscon mux controller DT bindings

On Thu, 2017-04-20 at 16:13 +0200, Peter Rosin wrote:
> On 2017-04-20 15:32, Peter Rosin wrote:
> > On 2017-04-20 00:09, Rob Herring wrote:
> >> On Thu, Apr 13, 2017 at 05:48:11PM +0200, Philipp Zabel wrote:
> >>> This adds device tree binding documentation for mmio-based syscon
> >>> multiplexers controlled by a single bitfield in a syscon register
> >>> range.
> >>>
> >>> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> >>> ---
> >>>  Documentation/devicetree/bindings/mux/mmio-mux.txt | 56 ++++++++++++++++++++++
> >>>  1 file changed, 56 insertions(+)
> >>>  create mode 100644 Documentation/devicetree/bindings/mux/mmio-mux.txt
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/mux/mmio-mux.txt b/Documentation/devicetree/bindings/mux/mmio-mux.txt
> >>> new file mode 100644
> >>> index 0000000000000..11d96f5d98583
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/mux/mmio-mux.txt
> >>> @@ -0,0 +1,56 @@
> >>> +MMIO bitfield-based multiplexer controller bindings
> >>> +
> >>> +Define a syscon bitfield to be used to control a multiplexer. The parent
> >>> +device tree node must be a syscon node to provide register access.
> >>> +
> >>> +Required properties:
> >>> +- compatible : "gpio-mux"
> >>
> >> ?
> >>
> >>> +- reg : register base of the register containing the control bitfield
> >>> +- bit-mask : bitmask of the control bitfield in the control register
> >>> +- bit-shift : bit offset of the control bitfield in the control register
> >>> +- #mux-control-cells : <0>
> >>> +* Standard mux-controller bindings as decribed in mux-controller.txt
> >>> +
> >>> +Optional properties:
> >>> +- idle-state : if present, the state the mux will have when idle. The
> >>> +	       special state MUX_IDLE_AS_IS is the default.
> >>> +
> >>> +The multiplexer state is defined as the value of the bitfield described
> >>> +by the reg, bit-mask, and bit-shift properties, accessed through the parent
> >>> +syscon.
> >>> +
> >>> +Example:
> >>> +
> >>> +	syscon {
> >>> +		compatible = "syscon";
> >>> +
> >>> +		mux: mux-controller@3 {
> >>> +			compatible = "mmio-mux";
> >>> +			reg = <0x3>;
> >>> +			bit-mask = <0x1>;
> >>> +			bit-shift = <5>;
> >>
> >> This pattern doesn't scale once you have multiple fields @ addr 3. I 
> >> also don't really think a node per register field in DT really scales.
> >>
> >> I think the parent should be declared as a mux controller instead. You 
> >> could encode the mux addr and bit position in the mux cells.
> > 
> > But then you need to create mux controllers on demand. I have not
> > succeeded in doing that while also following the rules of the driver
> > model. I had severe problems with life-time issues when I tried.
> > I would like to see code before embarking on this path, and I'm
> > apparently not the one writing it...
> > 
> > So, either you meant that, or that the parent node should somehow
> > specify the possible mux controllers up front so that they can be
> > pre-created and ready when the consumers request them. But if you
> > do that, you can just refer to them by some enumeration from the
> > mux consumers instead of by some convoluted reg+field notation.
> 
> Ok, thinking some more about this. Sorry for spamming and replying to
> self...
> 
> How about:
> 
>  	syscon {
>  		compatible = "syscon", "simple-mfd";
> 
> 		mux: mux-controllers {
> 			compatible = "mmio-mux";
> 	 		#mux-control-cells = <1>;
> 
> 			/* three mux controllers, one at reg 3 bits 0:2,
> 			 * one at reg 3 bits 5:6 and one at reg 7 bit 3.
> 			 */
> 			mux-reg-masks = <0x3 0x07>, <0x3 0x60>, <0x7 0x08>;
> 			idle-state = <7>, <MUX_IDLE_AS_IS>, <0>;
> 		};
> 
>  
>  		video-mux {
>  			compatible = "video-mux";
>  			mux-controls = <&mux 1>; /* i.e. reg 3 bits 5:6 */
>  
>  			ports {
>  				/* ports 0..5 */
>  			};
>  		};
>  	};
> 
> Optionally using some 64-bit safe 3-value encoding of the register fields
> in the mux-reg-masks binding...

I would prefer this to putting the registers and bit masks into the
phandle cells. The i.MX6Q/D GPR muxes could look like this:

	gpr: iomuxc-gpr@020e0000 {
		compatible = "fsl,imx6q-iomuxc-gpr", "syscon";
		reg = <0x020e0000 0x38>;

		mux: mux-controllers {
			compatible = "mmio-mux";
			#mux-control-cells = <1>;

			/* This list is not complete */
			mux-reg-masks = <0x04 0x00080000>, /* MIPI_IPU1_MUX */
					<0x04 0x00100000>, /* MIPI_IPU2_MUX */
					<0x0c 0x0000000c>, /* HDMI_MUX_CTL */
					<0x0c 0x000000c0>, /* LVDS0_MUX_CTL */
					<0x0c 0x0000030c>, /* LVDS1_MUX_CTL */
					<0x28 0x00000003>, /* DCIC1_MUX_CTL */
					<0x28 0x0000000c>; /* DCIC2_MUX_CTL */
		};

		ipu1_csi0_mux {
			compatible = "video-mux";
			mux-controls = <&mux 0>;
			/* ... */
		};

		ipu2_csi1_mux {
			compatible = "video-mux";
			mux-controls = <&mux 1>;
			/* ... */
		};
	};

and for i.MX6DL/S:

	gpr: iomuxc-gpr@20e0000 {
		compatible = "fsl,imx6q-iomuxc-gpr", "syscon";
		reg = <0x020e0000 0x38>;

		mux: mux-controllers {
			compatible = "mmio-mux";
			#mux-control-cells = <1>;

			mux-reg-masks = <0x34 0x00000007>, /* IPU_CSI0_MUX */
					<0x34 0x00000038>, /* IPU_CSI1_MUX */
					<0x0c 0x0000000c>, /* HDMI_MUX_CTL */
					<0x0c 0x000000c0>, /* LVDS0_MUX_CTL */
					<0x0c 0x0000030c>, /* LVDS1_MUX_CTL */
					<0x28 0x00000003>, /* DCIC1_MUX_CTL */
					<0x28 0x0000000c>; /* DCIC2_MUX_CTL */
		};

		ipu1_csi0_mux {
			compatible = "video-mux";
			mux-controls = <&mux 0>;
			/* ... */
		};

		ipu1_csi1_mux {
			compatible = "video-mux";
			mux-controls = <&mux 1>;
			/* ... */
		};
	};

regards
Philipp

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web