[RFC PATCH v3 2/5] pinctrl: add dt binding support for pinmux mappings
Richard Zhao
richard.zhao at linaro.org
Sun Jan 8 07:51:59 EST 2012
On Sat, Jan 07, 2012 at 09:54:48PM +0800, Shawn Guo wrote:
> On Fri, Jan 06, 2012 at 10:03:07AM -0800, Stephen Warren wrote:
> > I see now.
> >
> > I'd definitely be inclined to drop the num-pins and num-mux properties;
> > The values are just len(grp-pins)/4. You can still check that
> > len(grp-pins)==len(grp-mux) if you want to catch typos etc.
Maybe add a function of_u32_array_size? There' other drivers that need
to get array size.
> >
> +1
>
> > So, this does appear to be conflating the two things: The definition of
> > what pins are in a pingroup, and the mux function for a particular
> > setting of that pingroup. I think you need separate nodes for this.
> >
> At least for imx, we do not have mux function setting for pingroup.
> Instead, it only applies to individual pin.
I think it depends on function definition of pinmux driver. For the
imx example patch, it's one-to-one.
>
> > Without separate nodes, there will eventually be a lot of duplication.
> > A made-up example of the same uart4grp allowing either of two functions
> > uart3, uart4 to be muxed out onto it:
> >
> > aips-bus at 02000000 { /* AIPS1 */
> > iomuxc at 020e0000 {
> > pinctrl_uart4_3: uart4 at option_3 {
> > func-name = "uart3";
> > grp-name = "uart4grp";
>
> With phandle in dts reflecting the mapping, neither func-name nor
> grp-name should be needed, and both can just be dropped, IMO.
Maybe map-name can be dropped too if dev-name uses phandle.
>
> > grp-pins = <107 108>;
> > num-pins = <2>;
> > grp-mux = <3 3>;
> > num-mux = <2>;
> > };
> > pinctrl_uart4_4: uart4 at option_4 {
> > func-name = "uart4";
> > grp-name = "uart4grp";
> > grp-pins = <107 108>;
> > num-pins = <2>;
> > grp-mux = <3 3>;
> > num-mux = <2>;
> > };
> > }
> > };
> >
> > Now I understand that initially you aren't going to type out the complete
> > list of every available option into imx6q.dtsi because it's probably huge,
> > but the binding does need to allow you to do so without duplicating a lot
> > of data, because eventually you'll get boards that use a larger and larger
> > subset of all the options, so the number you need to represent at once in
> > imx6q.dtsi will grow.
If we don't want to lose flexibity, the pin group number will be huge
than you think. For example, 16bit display interface, has two alternatives
for every pin. The group number will be 2^16.
> >
> > So I think you need to model the IMX pinmux controller's bindings more on
> > how the pinctrl subsystem represents objects; separate definitions of pins,
> > groups of pins, functions, and board settings. Something more like:
> >
> > imx6q.dtsi:
> >
> > aips-bus at 02000000 { /* AIPS1 */
> > iomuxc at 020e0000 {
> > /* FIXME: Perhaps need pin nodes here to name them too */
>
> No, it's been listed in imx pinctrl driver.
>
> >
> > /* A node per group of pins. Each lists the group name, and
> > * the list of pins in the group */
> > foogrp: group at 100 {
> > grp-name = "foogrp";
> > grp-pins = <100 101>;
> > };
> > bargrp: group at 102 {
> > grp-name = "bargrp";
> > grp-pins = <102 103>;
> > };
> > bazgrp: group at 104 {
> > grp-name = "bargrp";
> > grp-pins = <104 105>;
> > };
>
> I agree that we should define pingroups in <soc>.dtsi, but the mux
> setting needs to be under the pingroup node too. See comment below ...
>
> > /* A node per function that can be muxed onto pin groups,
> > * each listing the function name, the set of groups it can
> > * be muxed onto, and the mux selector value to program into
> > * the groups' mux control register to select it */
> > uart3func: func at 0 {
> > func-name = "uart3";
> > /* Length of locations and mux-value must match */
> > locations = <&foogrp &bargrp>;
> > mux-value = <0 4>;
>
> This can be easily broken for imx. As the mux setting applies to
> individual pin rather than pingroup, it's very valid for foogrp to
> have pin 100 muxed on mode 0 while pin 101 on mode 1. That said,
> it's not necessarily true that we always have all the pins in
> particular pingroup muxed on the same setting for given function.
right.
>
> > };
> > uart4func: func at 1 {
> > func-name = "uart4";
> > locations = <&bargrp &bazgrp>;
> > mux-value = <6 3>;
> > };
>
> I prefer to have function node defined in <board>.dtsi, since it's
> all about defining phandle to the correct pingroup, which should be
> decided by board design.
group and function are one-to-one mapped for imx. So if you put function
in board dts, why not put pin group there too?
>
> > }
> > };
> >
> > Or, instead of separate locations and mux-value properties with matching
> > lengths, perhaps a node for each location:
> >
> > uart3func: func at 0 {
> > func-name = "uart3";
> > location at 0 {
> > location = <&foogrp>;
> > mux-value = <0>;
> > };
> > location at 1 {
> > location = <&bargrp>;
> > mux-value = <4>;
> > };
> > };
> >
> > That's more long-winded, but might be more easily extensible if we need
> > to add more properties later.
> >
> > Now in the board's .dts file, you need to specify for each device the
> > list of pinmux groups the device needs to use, and the function to
> > select for each group. Perhaps something like:
> >
> > board.dts:
> >
> > usdhc at 0219c000 { /* uSDHC4 */
> > fsl,card-wired;
> > status = "okay";
> > pinmux = <&foogrp &uart3func &bazgrp &uart4func>;
> > };
> >
> > I haven't convinced myself that's actually a good binding, but I think
> > it does represent the data required for muxing. Some potential issues
> > as before:
> >
> > * Do we need to add flags to each entry in the list; GPIO/interrupt do?
> >
> What's that for right now? I guess we can add it later when we see
> the need.
>
> > * Should "pinmux" be a node, and the configuration of each group be a
> > separate sub-node, so we can add more properties to each "table" entry
> > in the future, e.g. pin config parameters?
> >
> I do not think it's necessary. 'pinctrl' phandle works perfectly fine
> to me at least for now. How pinconf support should be added into
> pinctrl subsystem is still up in the air to me.
>
> > * For Tegra, I elected to put the definitions of pins, groups, and
> > functions into the driver rather than in the device tree.
>
> IMO, we do not want to do this for imx, as I'm scared of the size
> of Tegra pinctrl patches. If we go this way for imx, we will have
> even bigger patches.
>
> > This avoids
> > parsing a heck of a lot of data from device tree. That means there isn't
> > any per-function node that can be referred to by phandle. Does it make
> > sense to refer to groups and functions by string name or integer ID
> > instead of phandle? Perhaps:
> >
> > usdhc at 0219c000 { /* uSDHC4 */
> > fsl,card-wired;
> > status = "okay";
> > pinmux = {
> > group at 0 {
> > group = "foo";
> > function = "uart3";
> > /* You could add pin config options here too */
> > };
> > group at 1 {
> > group = "baz";
> > function = "uart4";
> > };
> > };
> > };
> >
> > I guess referring to things by name isn't that idiomatic for device tree.
> > Using integers here would be fine too, so long as dtc gets support for
> > named constants:
> >
> > imx6q.dtsi:
> >
> > /define/ IMX_PINGRP_FOO 0
> > /define/ IMX_PINGRP_BAR 1
> > /define/ IMX_PINGRP_BAZ 2
> > /define/ IMX_PINFUNC_UART3 0
> > /define/ IMX_PINFUNC_UART4 1
> > ...
> >
> > board .dts:
> >
> > usdhc at 0219c000 { /* uSDHC4 */
> > fsl,card-wired;
> > status = "okay";
> > pinmux = {
> > group at 0 {
> > group = <IMX_PINGRP_FOO>;
> > function = <IMX_PINFUNC_UART3>;
> > /* You could add pin config options here too */
> > };
> > group at 1 {
> > group = <IMX_PINGRP_BAZ>;
> > function = <IMX_PINFUNC_UART4>;
> > };
> > };
> > };
> >
> Doing this does not change the fact that this is bound with Linux
> driver details. That said, if the indexing of either pingrp array
> or pinfunc array changes in the driver, the binding is broken.
>
> --
> Regards,
> Shawn
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
More information about the linux-arm-kernel
mailing list