Hi Masahiro, On 18 July 2015 at 08:37, Simon Glass <s...@chromium.org> wrote: > Hi Masahiro, > > On 15 July 2015 at 02:16, Masahiro Yamada <yamada.masah...@socionext.com> > wrote: >> Now, a simple pinctrl patch is being proposed by Simon. >> http://patchwork.ozlabs.org/patch/487801/ >> >> In the design above, as you see, the uclass is just like a wrapper layer >> to invoke .request and .get_periph_id of low-level drivers. >> In other words, it is Do-It-Yourself thing, so it is up to you how to >> identify >> which peripheral is being handled in your .get_periph_id(). >> >> And here is one example for how a low-level pinctrl driver could be >> implemented. >> http://patchwork.ozlabs.org/patch/487874/ >> >> As you see in the thread, honestly, I do not like this approach. >> >> It is true that you can implement .get_periph_id in your driver >> better than parsing "interrupts" properties, but I guess >> many drivers would follow the rockchip implmentation because >> no helpful infrastructure is provided by the uclass (at least now). >> >> Device trees describe hardwares in a way independent of software >> that they are used with. So, identical device trees can be (should be) >> used with U-Boot as well as Linux or whatever. >> >> Thus, I want the pinctrl can be controllable by device trees in the same way >> of Linux, that is, by parsing "pinctrl-names" and "pinctrl-<N>" properties. >> >> Of course, it would be possible to do it in my own .get_periph_id, >> but "pinctrl-names" and "pinctrl-<N>" are too generic to be done in each >> low-level driver. >> >> In this series, I'd like to propose to support it in the uclass, so that >> we can easily reuse device trees for pinctrl. >> Please put it on the table for discussion. >> >> Let me explain how it works. >> >> The basic idea is pretty much like Linux, but it has been much simplified >> because full-support of the Linux's pinctrl is too much a burden for a >> boot-loader. >> >> Device Tree >> ----------- >> >> To use pinctrl from each peripheral, add some properties in the device node. >> >> "pinctrl-names" is a list of pin states. The "default" state is mandatory, >> and it would probably be enough for U-Boot. But, in order to show how it >> works, >> say the example device supports two states: "default" and "sleep". >> In this case, the properties should be like this. >> >> pinctrl-names = "default", "sleep"; >> >> And then, add as many "pinctrl-<N>" properties as the number of states. >> >> pinctrl-0 = <phandle to default config node>; >> pinctrl-1 = <phandle to sleep config node>; >> >> Here, pinctrl-0, pinctrl-1 corresponds to "default", "sleep", respectively. >> >> The config nodes are (direct or indirect) children of a pinctrl device. >> >> To sum up, the device tree would be like this: >> >> foo { >> compatible = "..."; >> reg = <...>; >> pinctrl-names = "default", "sleep"; >> pinctrl-0 = <&foo_default_pin>; >> pinctrl-1 = <&foo_sleep_pin>; >> ... >> }; >> >> pinctrl { >> compatible = "..."; >> reg = <...>; >> foo_default_pin: foo_default { >> groups = "..."; >> functions = "..."; >> }; >> foo_sleep_pin: foo_sleep { >> groups = "..."; >> functions = "..."; >> }; >> }; >> >> API >> --- >> >> >> To set a device into a particular pin state, call >> int pinctrl_set_state(struct udevice *dev, const char *state_name). >> >> For example, if you want to set the foo device into the sleep state, >> you can do like this: >> >> struct udevice *foo_dev; >> >> (device_get or whatever) >> >> pinctrl_set_state(foo_dev, "sleep"); >> >> When each device is probed, pinctrl_init() is invoked, >> which initializes some pinctrl-specific parameters and set it into "default" >> pin state. Because it is automatically done by the core of driver model, >> when a device is probed, its pins are in the "default" state. >> >> Implementation of low-level driver >> ---------------------------------- >> >> Currently, two methods are supported in the pinctrl operation: >> struct pinctrl_ops { >> int (*pinmux_set) (struct udevice *dev, const char *group, >> const char *function); >> int (*pinconf_set) (struct udevice *dev, const char *group, >> const char *conf_param, unsigned conf_arg); >> }; >> >> They are used to change pin-mux, pin-conf, respectively. >> >> If the pin-config node for the target pin-state is like this, >> i2c_default_pin: i2c_default { >> groups = "i2c-0a"; >> functions = "i2c-0"; >> }; >> >> Your pinmux_set() is called with "i2c-0a" for the group and "i2c-0" >> for the function. >> >> It is totally up to you what you do for each group & function. >> >> Difference from Linux pinctrl (limitation) >> ----------------------------------------- >> >> As you can see in pinctrl drivers in Linux (drivers/pinctrl/*), >> the drivers usually contain huge tables for pins, groups, functions,... >> But I do not want to force that for U-Boot. >> >> In Linux, "group" represents a group of pins although a group sometimes >> consists of a signle pin (like GPIO). Pin-muxing is available only against >> groups, while pin-conf is supported for both pins and groups. >> >> In contrast, in U-Boot, "pins" and "groups" are handled exactly in the same >> way, >> so you can use either to specify the target of pin-mux or pin-conf. >> >> Pin/group tables are not required for U-boot pinctrl drivers, so >> we never know which pins belong to which group, function, that is, >> we can not avoid conflicts on pins. >> >> For example, let's say some pins are shared between UART0 and I2C0. >> In this case, Linux pinctrl does not allow to use them simultaneously, >> while U-boot pinctrl does not (can not) care about it. >> >> If you want to use multiple functions that share the same pins, >> you must make sure pin-muxing is correctly set with pinctrl_set_state(). >> >> TODO >> ---- >> >> [1] Pinctrl drivers are usually used with device trees (or ACPI), but >> not all the boards in U-boot support device tree. >> I will add board file support (plat data) later. >> (we will need some function to register pin settings array) >> >> [2] SPL support is not completed. Tweak Kconfig and Makefile a little. >> This should be easy. >> >> [3] Currently, properties other than "function" are assumed >> as pin-conf parameters. Perhaps, should we filter out >> unsupported properties? A table for supported properties >> such "bias-pull-up", "driver-strength", etc. ? >> >> [4] For "function", "groups" should be able to be omitted. >> >> Comments are welcome. > > (long pause for thought) > > I would certainly like to have this in U-Boot. I think for most uses > it is the right thing to do. However I'm not sure it is sufficient. > > For SPL we are limited on code size / space. In that case we often > need to do a little pinmuxing but we cannot always afford the space of > this implementation. The device tree for pinmux is pretty large for > some SoCs quite apart from the extra code. I found this recently with > Rockchip which has a 32KB limit. > > The nice thing about my simple implementation is that it can be used > from SPL efficiently. For example: > > static void soc_spi_pinmux(enum periph_id id, int flags) > { > switch (id) { > case PERIPH_ID_SPI0: > writel(...) > break; > case PERIPH_ID_SPI1: > writel(...) > break; > } > } > > static void soc_i2c_pinmux(enum periph_id id, int flags) > { > case PERIPH_ID_I2C0: > writel(...) > break; > case PERIPH_ID_I2C1: > writel(...) > break; > } > } > > static void soc_set_pinmux(enum_periph_id id, int flags) > { > switch (id) { > #ifdef CONFIG_SPI > case PERIPH_ID_SPI0: > case PERIPH_ID_SPI0: > soc_i2c_pinmux(id, flags); > break; > #endif > #ifdef CONFIG_SPI > case PERIPH_ID_I2C0: > case PERIPH_ID_I2C0: > soc_i2c_pinmux(id, flags); > break; > #endif > } > > In SPL we can do something like: > > soc_i2c_pinmux(PERIPH_ID_I2C0, 0); > > to set up I2C port 0 with default pin settings. This is about as > efficient as you can get. > > With your full implementation we have no such option. > > So I think your implementation makes more sense in general, but mine > is better for SPL. > > Can we combine the two? What do you think about allowing the uclass to > provide both interfaces? You can then use the full pinctl one, but > implement a simple one also. This could be achieved simply by adding > two more methods to the uclass.
If you are OK with this then I'll have a try at basing my pinctrl patches on top of your series. I think it can be made to work. > > BTW I've been meaning to discuss how to deal with SPL config as it is > painful in this area, but I'll start a new thread for that. > > Regards, > Simon Regards, Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot