[RFC PATCH v3 2/5] pinctrl: add dt binding support for pinmux mappings

[Dong Aisheng-B29396]

> -----Original Message-----
> From: sjg at google.com [mailto:sjg at google.com] On Behalf Of Simon Glass
> Sent: Monday, January 09, 2012 2:18 PM
> To: Shawn Guo
> Cc: Richard Zhao; Stephen Warren; linus.walleij at stericsson.com;
> s.hauer at pengutronix.de; linux-kernel at vger.kernel.org; rob.herring at calxeda.com;
> kernel at pengutronix.de; cjb at laptop.org; Dong Aisheng-B29396; devicetree-
> discuss at lists.ozlabs.org; linux-arm-kernel at lists.infradead.org; Dong Aisheng
> Subject: Re: [RFC PATCH v3 2/5] pinctrl: add dt binding support for pinmux
> mappings
> Importance: High
> 
> Hi,
> 
> [snip lots of discussion]
> 
> Someone copied me on this so I feel I can sneak in with a response. I have read
> this entire thread and implemented Tegra pinmux in U-boot, but not much else on
> the topic so have limited understanding, particular of the kernel pinmux setup,
> unfortunately. Sorry if I have the wrong end of the stick on some of this, but I
> hope my comments are useful.
> 
Thanks a lot for such long comments and share your design. :-)
I mainly comment on pinctrl stuff.

> 1. Symbolic names
> 
> > Stephen said:
> > It's a real pity that dtc doesn't have a way of doing named integer
> > constants, so this could be re-written as e.g.:
> > SoC .dtsi file:
> > /define/ TEGRA_MUX_FUNC_SD4 234
> 
> IMO this is essential for pinmux. Stephen has already done a patch which
> implements this feature. The alternative is bindings with lots of arbitrary
> numbers - lots of errors, lots of confusion, lots of pain. He has asked a few
> times on the device-tree list for this feature to be accepted. I believe it
> should be, and pinmux brough in with that in mind. Turning symbols into numbers
> is the role of the device tree compiler I think.
> 
> If Stephen's proposed grammar needs tweaking that is fine, but I think we are
> pushing the limits of a pure numerical representation here.
> 
> 
> 2. Stephen also said:
> 
> > We also need to consider compatibility and extensibility. A DT binding
> > is an ABI between the kernel and whatever is providing it, and so we
> > need to be able to maintain and extend it in a compatible fashion,
> > just like the user-space syscall ABI. Now it's true that the .dts
> > files are currently part of the kernel source tree and can be rev'd in
> > sync with kernel changes, but that may not always be the case moving forward.
> >
> > In other words, taking a board with an old device tree embedded into
> > Flash and running a new kernel using that device tree should work just
> > like running the original kernel did.
> 
> While this is true we should remember that SOCs will change and evolve, and we
> don't actually need a device tree for a Tegra 2 (say) to be able to boot a Tegra
> 3 kernel. So each time we create a new SOC in a family, we have the opportunity
> to update the fdt, potentially in an incompatible way, since we know that the
> old boot loader and new kernel are incompatible anyway.
> 
> Anyway we are looking at pinmux from a 2012 view of how SOCs do things. We can't
> predict what things will look like in the future. The device tree format is
> great for making things extensible, but there is an efficiency cost also.
> 
> Backwards compatibility can create a lot of problems with newer designs, so I
> don't think we should be slaves to it. We are backwards compatible because it
> aids understanding, is necessary for kernels to run across different boards,
> etc., not just for its own sake. If in the future we decide that the approach
> agreed now no longer servers the purpose and has too much baggage, we can change
> it. Similar things happen in the rest of the kernel.
> 
> Also IMO updating the kernel without the device tree doesn't make a huge amount
> of sense. Why not just update both? Really to me the device tree is almost like
> static data for the kernel. It is a worthy goal to keep it as static as possible,
> but if it changes I don' t think it is the end of civilisation.
> 
> 
> 3. Efficiency
> 
> There has been talk about using strings to link things. IMO this is inefficient
> and slow compared to integers and device trees already have phandles. Some
> platforms will care more than others, but if the reason for using strings is
> that the device tree format does not support symbolic representation of integers,
> then see 1 above.
> 
> 
> 4. Who bears the pain?
> 
> There is a lot of complexity in pinmux - if we continue to split the device tree
> into an SOC file and a board file then we should think about which one bears the
> brunt of the complexity. IMO it should be the SOC file. The SOC file is written
> by very clever engineers working for the SOC vendor. They are motivated to get
> their SOC working properly and for it to be easy for customers to access the
> glorious ground-breaking feature therein. On the other hand, the board file (to
> the extent it is not just copied from the vendor's example) is put together by
> people with little knowledge of the SOC, limited time to do the work and minimal
> interest in the wonders of the device tree structure.
> 
> With this in mind I think the pinmux complexity should mostly be in the SOC file.
> This includes the selection of valid pinmux options.
> After all, despite the many possible ways that functions could be brought out of
> the chip, only a certain few ways typically make sense.
> For example, an SDMMC peripheral needs clock and command pins - unless these are
> connected then nothing with work. Similarly there is no sense in having two
> data[0] pins brought out for the same SDMMC port.
> 
> In fact the SOC vendor generally has a good idea about the different options and
> mostly there are a small number of useful ones. So why don't we just enumerate
> these in the SOC file and let the board select what it wants in a single setting?
> 
Correct for us! :)

> 
> 5. Start at the top: function mux
> 
> I propose adding a new level at the top, a function mux. This basically routes
> functions to one of 2-3 predefined options. So UART1 can come out on 3 different
> sets of pins, SDMMC2 on two sets, etc.
Yes, that's like what current pinctrl subsystem does.
And I agreed with Stephen we could define it in soc dts file.

> These are recommended from the vendor and in common use (e.g. on reference
> designs), so should be easy to use.
> 
Yes, we really do not need to define all possible ones and most of them may not
Useful.

> Below the function mux is the group mux. Here we specify a list of pin groups
> and which function each group is assigned to. Together these
> 2-3 groups join to give us the pins that the function mux needs.
> 
> Below the pin groups are the pins. Some SOCs will have only one pin per group,
> some will have a group mux that really just moves entire functions around.
> 
> IMO the top level belongs with the board file. If the board designer wants to
> depart from common options they can suffer the pain of dropping down to the
> group level.
> 
> So at the board level it would be nice to do something like this:
> 
> sdhci at c8000400 {
> 	funcmux = <&sdmmc3-funcmux 0 0>;  /* funcmux phandle, config option, flags
> */ };
> 
> which means that we want this SDMMC port to use configuration 0 (out of perhaps
> 2-3 recommended options the SOC provides for bringing out this SDMMC function)
> and that there are no special flags required. Or:
> 
> emmc: sdhci at c8000600 {
> 	funcmux = <&sdmmc4-funcmux 1 OPT_SDMMC_8BIT>; }
> 
> which means for this port we want to use configuration 1 with 8-bit wide data.
> 
In my understanding a phandle to the 8bit sdmmc pingroup already works.
Why we really need this flag?

> Something like this is easy for the board files to deal with. It fits with the
> way reference designs are done (selecting from a few options). It allows flags
> to specify different options like 4-bit or 8-bit wide SDMMC which can affect
> what pinmux groups are affected.
> 
> Some boot loaders might even implement things only at the funcmux level, with
> hard-coded configuration of the actual pin groups in static data in their code,
> without reference to the device tree. This might allow them to operate with a
> vastly truncated device tree. They might be very motivated to do this if the
> full device tree consists of 20KB of strings :-)
> 
> 
> 6. Groups
> 
> Already done in the discussion prior to this email:
> 
> uart4func: func at 1 {
>        func-name = "uart4";
>        locations = <&bargrp &bazgrp>;
>        mux-value = <6 3>;
> };
> 
> but how above adding a flag cell and putting the locations and values into the
> same property, like:
> 
> uart4func: func at 1 {
>        func-name = "uart4";
>        locations = <&bargrp 6 0>
> 		<&bazgrp 3 SOME_FLAG>;
> };
> 
> 
> So the SDMMC example might have three options for SDMMC4: a 4-bit one, and two
> 8-bit ones (changing 'locations' to 'groups'):
> 
> sdmmc4-funcmux : sdmmc4_funcmux at 0 {
> 	setting at 0 {
> 		config = <0>;
> 		option = <OPT_SDMMC_8BIT>;
> 		groups = <&pingrp-atc FUNC_SDIO4 0> /* clk, cmd, data 1, 3, 5, 7 */
> 			<&pingrp-atd FUNC_SDIO4 0>; /* data 0, 2, 4, 6 */
> 	};
> 
> 	setting at 1 {
> 		config = <1>;
> 		option = <0>;
> 		groups = <&pingrp-atb FUNC_SDIO4 0>  /* clock, command */
> 			<&pingrp-gma FUNC_SDIO4 0>; /* data[3:0] */
> 	};
> 
> 	setting at 2 {
> 		config = <1>;
> 		option = <OPT_SDMMC_8BIT>;
> 		groups = <&pingrp-atb FUNC_SDIO4 0>  /* clock, command */
> 			<&pingrp-gma FUNC_SDIO4 0> /* data[3:0] */
> 			<&pingrp-gme FUNC_SDIO4 0>; /* data [7:4] */
> 	};
> };
> 
> The 'config' property refers to the second cell of the reference in the funcmux.
> The option property refers to the third cell. Essentially the vendor is
> recommending three possible options for SDMMC4 and the board can choose which
> one it likes.
> 
> The disadvantage is that the last two share the pingrp-atb setting, and the last
> just adds an extra group, but no matter. It is easy to understand.
> 
> A related problem is that the options must each be enumerated. Maybe that
> doesn't matter either for the moment.
> 
> Then for the groups, again already done above, but I am less clear on the exact
> binding that is arrived at above. Something like this?
> 
> pingrp-atb: pingrp_atb at 0 {
> 	locations = <&pin-i02 &pin-t07>;
> 	control = <&pinmux 0>;
If it's sub node of pin controller device, we may not need this.

> 	/* 4 options available, numbered 0 to 3 */
> 	functions = <FUNC_IDE   FUNC_NAND   FUNC_GMI   FUNC_SDIO4>;
This does not apply for IMX since the functions here are for each pin rather than
the group.

For IMX, it looks like:
iomuxc at 020e0000 {
        reg = <0x020e0000 0x4000>;
        pinmux-groups {
                uart4grp: group at 0 {
                        grp-name = "uart4grp";
                        grp-pins = <107 108>;
                        grp-mux = <4 4>;
                };

                sd4grp: group at 1 {
                        grp-name = "sd4grp";
                        grp-pins = <170 171 180 181 182 183 184 185 186 187>;
                        grp-mux = <0 0 1 1 1 1 1 1 1 1>;
                }
        }

        pinmux-functions {
                uart4func: func at 0 {
                        func-name = "uart4";
                        groups = <&uart4grp ..>;
                }

                sd4func: func at 1 {
                        func-name = "sd4";
                        groups = <&sd4grp ..>;
                }
        }
};

And not the pins in 'locations' can support all functions in 'functions' Property. 
It may be less flexible.
For IMX different pins in same group may support different type of functions.

> 	/* do we want to specify what the flags do here? I assume they are
>            globally understood by the pinmux system rather than specific to
> 	   each group */
> };
>
> 
> pingrp-atd: pingrp_atd at 0 {
> 	locations = <&pin-h01 &pin-h02 &pin-h03 &pin-h04>;
> 	control = <&pinmux 3>;  /* 3 is the pinmux controller's ID for the atd
> group */
> 	/* 4 options available, numbered 0 to 3 */
> 	functions = <FUNC_IDE   FUNC_NAND   FUNC_GMI   FUNC_SDIO4>;
> };
> 
> 
> (It is probably obvious, but FUNC_SDIO4 is not just the value 3. It might be 21
> - the pinmux code searches the list of 4 options for FUNC_SDIO4. The position it
> finds it in this case is 3, so that is the value written to &pinmux)
> 
> 
> 7. Pins
> 
> At this level I'm not sure if a device tree representation adds much.
> Maybe connect the GPIOs?
> 
I'm not sure if it's suitable since not all the pins of IMX are gpio pins.

> pin-i02: pin_i02 at 0 {
> 	name = "i02";
> 	gpio = <&gpio 66>;
> };
> 
> pin-t07: pin_t07 at 0 {
> 	name = "t07";
> 	gpio = <&gpio 167>;
> };
> 
> 
> 8. Flexibility
> 
> Someone asked how to deal with an LCD output with 16 bits each of which can be
> on two pins. Know you of such an SOC? If so, then it is still possible that the
> SOC would provide a few basic 'common' options at the funcmux level. But board
> designers may have to steel themselves and dive into the the pingroup level.
> 
> Where an SOC has no groups but only pins, then perhaps the funcmux can still be
> provided, but uses a 'pins' property instead of 'groups':
> 
> sdmmc4-funcmux : sdmmc4_funcmux at 0 {
>         ...
> 	setting at 2 {
> 		config = <1>;
> 		option = <OPT_SDMMC_8BIT>;
> 		pins = <&pin-i02 FUNC_SDIO4 0>  /* clock */
> 			<&pin-t07 FUNC_SDIO4 0>  /* command */
> 			<&pin-a00 FUNC_SDIO4 0>  /* data[0] */
> 			<&pin-a01 FUNC_SDIO4 0>  /* data[1] */
> 			<&pin-a02 FUNC_SDIO4 0>  /* data[2] */
> 			<&pin-a03 FUNC_SDIO4 0>;  /* data[3] */
> 			<&pin-a04 FUNC_SDIO4 0>;  /* data[4] */
> 			<&pin-a05 FUNC_SDIO4 0>;  /* data[5] */
> 			<&pin-a06 FUNC_SDIO4 0>;  /* data[6] */
> 			<&pin-a07 FUNC_SDIO4 0>;  /* data[7] */
> 	};
> };
> 
> IMO this sort of thing makes it even more desirable to have a high-level pinmux
> feature in the device tree. Again, the board designer can dive into pins by
> defining his own custom funcmux-level things, so no flexibility is lost.
> 
> Regards,
> Simon