RE: [RFC PATCH v3 2/5] pinctrl: add dt binding support for pinmuxmappings

[Dong Aisheng-B29396]

> -----Original Message-----
> From: Stephen Warren [mailto:swarren@xxxxxxxxxx]
> Sent: Friday, January 06, 2012 7:38 AM
> To: Dong Aisheng-B29396; linux-kernel@xxxxxxxxxxxxxxx
> Cc: linus.walleij@xxxxxxxxxxxxxx; s.hauer@xxxxxxxxxxxxxx;
> rob.herring@xxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx;
> kernel@xxxxxxxxxxxxxx; cjb@xxxxxxxxxx; devicetree-discuss@xxxxxxxxxxxxxxxx
> Subject: RE: [RFC PATCH v3 2/5] pinctrl: add dt binding support for pinmux
> mappings
> Importance: High
> 
> Dong Aisheng-B29396 wrote at Tuesday, December 27, 2011 7:41 AM:
> > Stephen Warren wrote at Sunday, December 25, 2011 11:37 AM:
> > > Dong Aisheng-B29396 wrote at Thursday, December 22, 2011 1:18 AM:
> > > > Stephen Warren wrote at Wednesday, December 21, 2011 8:39 AM:
> > > > > Dong Aisheng wrote at Tuesday, December 20, 2011 10:41 AM:
> > > > > > This patch provies a common API for driver to use to register
> > > > > > pinmux mappings from dts file. It is needed for dt support.
> > > > >
> > > > > Dong,
> > > > >
> > > > > Thanks for providing a concrete binding to get discussion
> > > > > started. I had intended to do this myself, but got side-tracked on other
> things.
> > > > >
> > > > > I'll comment exclusively on the documentation in this patch,
> > > > > since any changes there will require the code to be changed.
> ...
> > > > > b) The top-level node ("imx6q-sabreauto-map" below) is not tied
> > > > > to any particular piece of hardware (no compatible flag, not reg
> > > > > property etc.)
> > > >
> > > > My original purpose was that it was not a hw device node, it was
> > > > just created for containing pin map information and referenced by
> > > > a phandle in iomuxc deivce node
> > > > like:
> > > >
> > > > iomuxc@020e0000 {
> > > >         compatible = "fsl,imx6q-iomuxc";
> > > >         reg = <0x020e0000 0x4000>;
> > > >         fsl,pinmux-map = <&pinmux>;
> > > > 	  ...
> > > > }
> > > >
> > > > I could also make it as a sub node of iomuxc, but original design
> > > > is that sub nodes of Ioxmuc are already representing each pinmux functions.
> > > > To avoid conflict, I put the imx6q-sabreauto-map node out of
> > > > iomuxc since a phandle (fsl,pinmux-map ) does the same work well.
> > >
> > > You could have nested sub-nodes, say something like:
> > >
> > > iomuxc@020e0000 {
> > >     compatible = "fsl,imx6q-iomuxc";
> > >     reg = <0x020e0000 0x4000>;
> > >     fsl,pins {
> > >         ... // fsl-specific pin properties
> > >     };
> > >     fsl,groups {
> > >         ... // fsl-specific group properties
> > >     };
> > >     fsl,functions {
> > >         ... // fsl-specific mux function properties
> > >     };
> > >     pinmux-usage {
> > >         // standardized pinmux "table" properties
> > >         sd4 { // pin group name
> > >             function = "sdio4"; // this function active on it
> > >             ...
> > >         };
> > >         ...
> > >     }
> > >     ...
> > > }
> > >
> > Yes, I could do that.
> > The extra effort is that we have to manually exclude one pinmux-usage
> > node in pinmux driver since originally i take the child node count of
> > iomuxc as the function count since all child nodes are functions, that
> > why I firstly took the mapping node out of iomuxc, in addition the old
> > way i used seems to be more brief and clear.
> >
> > I tried adding pinmux-usage as a sub node of iomuxc and got two issues:
> > Taking imx6q as an example:
> >
> > iomuxc@020e0000 {
> >         uart4 {
> >                 func-name = "uart4";
> >                 grp-name = "uart4grp";
> >                 grp-pins = <107 108>;
> >                 num-pins = <2>;
> >                 grp-mux = <4 4>;
> >                 num-mux = <2>;
> >         };
> >
> >         sd4 {
> >                 func-name = "sd4";
> >                 grp-name = "sd4grp";
> >                 grp-pins = <170 171 180 181 182 183 184 185 186 187>;
> >                 num-pins = <10>;
> >                 grp-mux = <0 0 1 1 1 1 1 1 1 1>;
> >                 num-mux = <10>;
> >         };
> >
> >         pinmux-mapping {
> >                 uart4 {
> >                         function = "uart4";
> >                         dev = <&uart4>;
> >                 };
> >
> >                 usdhc4 {
> > /*			    ctrl-dev-name = "20e0000.iomuxc"; */
> >                         function = "sd4";
> >                         dev = <&usdhc4>;
> >                 };
> >         };
> > };
> >
> > If we remove ctrl-dev-name and get it from its parent node in drivers
> > as you suggested, first, since this work will be done in the common
> > API (pinmux_of_register_mappings Requires pass the pinmux mapping node
> > to it for construct the mapping table) It will introduce a restriction
> > that all platforms should define this node under their pinctrl device
> > node.
> 
> This seems reasonable to me; the pinmux controller would be defining the mux
> options/... that it's providing to the rest of the DT.
> 
> > Second, it seems we cannot get its parent node device or device name
> > in driver (can only get node Name which is not sufficient for
> > construct the pin map table required by pinctrl core) and current
> > device tree subsystem seems do not support get its associated device from a
> device node.
> > We may not be able to construct ctrl-dev-name or ctrl-dev for struct
> pinmux_map.
> > I'm not sure if I missed something, if missed please let me know.
> > To support it, we may need to add support for converting device node
> > to device. Not sure it's applicable since I still have not tried it.
> 
> That functionality is pretty easy to implement; it already exists for other
> subsystems that refer to parent/controller/... nodes using phandles.
> for example, take a look at the implementation of of_node_to_gpiochip() in
> drivers/of/gpio.c, which calls gpiochip_find() with a custom match function that
> simply compares the gpio_chip's of_node field. Once you have the chip, you have
> the device, and can store that in the constructed mapping table, or call
> dev_name() on it. I assume there's something similar for interrupts.
> 
Yes, that's indeed a way.
Thanks for the info.

> ...
> > > > The same question applies to clock and regulator: does each device
> > > > need Define its clock and regulator properties?
> > >
> > > Yes, in those cases I believe each device does contain a phandle to
> > > the resources it uses.
> >
> > I was ever thought putting a phandle of pinmux function in each
> > device, Then pinmux mapping table seems not need anymore. Like:
> >
> > usdhc4: usdhc@0219c000 { /* uSDHC4 */
> >         compatible = "fsl,imx6q-usdhc";
> > 	....
> >         pinmux = <&pinmux-sd4>;
> > };
> >
> > iomuxc@020e0000 {
> >         pinmux-sd4 : sd4 {
> >                 func-name = "sd4";
> >                 grp-name = "sd4grp";
> >                 grp-pins = <170 171 180 181 182 183 184 185 186 187>;
> >                 num-pins = <10>;
> >                 grp-mux = <0 0 1 1 1 1 1 1 1 1>;
> >                 num-mux = <10>;
> >         };
> >
> > It is a pure hw point of view to define node.
> > And it may need to implement a of_pinmux_get.
> 
> > But what about the pin maps without device associated?
> 
> Indeed; that's why I'd tend towards defining a table of pinmux usage in the
> pinmux node, and having other devices refer to that table.
>
Currently we still prefer to use device node relationship to reflect the pinmux
map if we can since as you said pinmux map is little depending on the pinctrl
subsystem implementation.
And I'm trying to do it now.
 
> Still, if the pinmux definitions are in the device nodes, we could simply make
> the pinmux controller have such a definition itself too, for the "system hog"
> case.
> 
Yes, that way I think is like:
iomuxc@020e0000 {
        pinctrl_uart4: uart4 {
                grp-pins = <107 108>;
                grp-mux = <4 4>;
		   hog_on_boot;
        };
}
I'm still trying to find a way to support it in pinctrl core.
Maybe we could do it in the next step and we want to implement pinmux binding first.

Regards
Dong Aisheng

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/