DM: pinctrl: Another implemtation of pinctrl framework

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.

Hello Masahiro,

Quoted 171 lines · Masahiro Yamada

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.

Quoted 1 line

Comments are welcome.

A very general comment for now: the above is absolutely too long for a commit message. It should be a patman commit or series note, or better yet, a cover letter, or, better still, rewritten into a doc/README.* file (except for the comparison with Simon's patch, which should not be in a README but remain a commit note).

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.

Hi Masahiro,

Quoted 164 lines · Masahiro Yamada

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.

Quoted 8 lines

[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.

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

Hi Masahiro,

Quoted 242 lines · Simon Glass

Hi Masahiro,

Quoted 164 lines · Masahiro Yamada

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.

Quoted 8 lines

[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.

Quoted 4 lines

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.

Quoted 4 lines

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.

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.

Signed-off-by Masahiro Yamada <[email protected]>

---
 drivers/Kconfig                  |   2 +
 drivers/Makefile                 |   1 +
 drivers/core/device.c            |   5 +
 drivers/pinctrl/Kconfig          |   2 +
 drivers/pinctrl/Makefile         |   1 +
 drivers/pinctrl/pinctrl-uclass.c | 353 +++++++++++++++++++++++++++++++++++++++
 include/dm/device.h              |   3 +
 include/dm/pinctrl.h             |  28 ++++
 include/dm/uclass-id.h           |   1 +
 9 files changed, 396 insertions(+)
 create mode 100644 drivers/pinctrl/Kconfig
 create mode 100644 drivers/pinctrl/Makefile
 create mode 100644 drivers/pinctrl/pinctrl-uclass.c
 create mode 100644 include/dm/pinctrl.h

drivers/Kconfig +2 -0

index c7e526c..c696036 100644
@@ -28,6 +28,8 @@ source "drivers/i2c/Kconfig"
 
 source "drivers/spi/Kconfig"
 
+source "drivers/pinctrl/Kconfig"
+
 source "drivers/gpio/Kconfig"
 
 source "drivers/power/Kconfig"

drivers/Makefile +1 -0

index 280b6d1..7e5ae84 100644
@@ -4,6 +4,7 @@ obj-$(CONFIG_OF_CONTROL) += fdt/
 obj-$(CONFIG_BIOSEMU) += bios_emulator/
 obj-y += block/
 obj-$(CONFIG_BOOTCOUNT_LIMIT) += bootcount/
+obj-$(CONFIG_PINCTRL) += pinctrl/
 obj-$(CONFIG_CPU) += cpu/
 obj-y += crypto/
 obj-$(CONFIG_FPGA) += fpga/

drivers/core/device.c +1 -0

index 46bf388..4e94a4f 100644
@@ -15,6 +15,7 @@
 #include <dm/device.h>
 #include <dm/device-internal.h>
 #include <dm/lists.h>
+#include <dm/pinctrl.h>
 #include <dm/platdata.h>
 #include <dm/uclass.h>
 #include <dm/uclass-internal.h>

drivers/core/device.c +0 -0

        ret = uclass_pre_probe_device(dev);
        if (ret)
                goto fail;

drivers/pinctrl/Kconfig new file +2 -0

new file mode 100644
index 0000000..cae4d55
@@ -0,0 +1,2 @@
+config PINCTRL
+       bool

drivers/pinctrl/Makefile new file +1 -0

new file mode 100644
index 0000000..a775a4c
@@ -0,0 +1 @@
+obj-y                          += pinctrl-uclass.o

drivers/pinctrl/pinctrl-uclass.c new file +17 -0

new file mode 100644
index 0000000..68e4842
@@ -0,0 +1,353 @@
+/*
+ * Copyright (C) 2015  Masahiro Yamada <[email protected]>
+ *
+ * SPDX-License-Identifier:    GPL-2.0+
+ */
+
+#include <common.h>
+#include <libfdt.h>
+#include <linux/err.h>
+#include <linux/list.h>
+#include <dm/device.h>
+#include <dm/pinctrl.h>
+#include <dm/uclass.h>
+
+enum pinctrl_setting_type {
+       PIN_SETTING_TYPE_INVALID,
+       PIN_SETTING_TYPE_MUX,

drivers/pinctrl/pinctrl-uclass.c new file +7 -0

+struct pinctrl_setting {
+       struct list_head node;
+       enum pinctrl_setting_type type;
+       struct udevice *pctldev;
+       const char *group;
+       const char *func_or_conf;
+       unsigned conf_arg;

drivers/pinctrl/pinctrl-uclass.c new file +58 -0

+               return 0;
+       }
+
+       return ops->pinmux_set(pctldev, setting->group, setting->func_or_conf);
+}
+
+static int pinconf_enable_setting(struct pinctrl_setting *setting)
+{
+       struct udevice *pctldev = setting->pctldev;
+       const struct pinctrl_ops *ops = pctldev->driver->ops;
+
+       if (!ops->pinconf_set) {
+               printf("%s: pin_config_set is not defined.  skip.\n",
+                      pctldev->name);
+               return 0;
+       }
+
+       return ops->pinconf_set(pctldev, setting->group, setting->func_or_conf,
+                               setting->conf_arg);
+}
+
+static int pinctrl_select_state(struct pinctrl_state *state)
+{
+       struct pinctrl_setting *setting;
+       int ret;
+
+       list_for_each_entry(setting, &state->settings, node) {
+               switch (setting->type) {
+               case PIN_SETTING_TYPE_MUX:
+                       ret = pinmux_enable_setting(setting);
+                       break;
+               case PIN_SETTING_TYPE_CONFIG:
+                       ret = pinconf_enable_setting(setting);
+                       break;
+               default:
+                       ret = -EINVAL;
+                       break;
+               }
+
+               if (ret)
+                       return ret;
+       }
+
+       return 0;
+}
+
+#ifdef CONFIG_OF_CONTROL
+static int pinctrl_dt_subnode_to_setting_one_group(struct udevice *pctldev,
+                                                  const void *fdt, int node_offset,
+                                                  const char *group,
+                                                  struct list_head *settings)
+{
+       struct pinctrl_setting *setting;
+       const char *name;
+       const void *value;
+       int prop_offset, len;
+
+       for (prop_offset = fdt_first_property_offset(fdt, node_offset);

drivers/pinctrl/pinctrl-uclass.c new file +58 -0

+
+               setting = calloc(1, sizeof(*setting));
+               if (!setting)
+                       return -ENOMEM;
+
+               INIT_LIST_HEAD(&setting->node);
+               setting->pctldev = pctldev;
+               setting->group = group;
+
+               /* we treat properties other than "function" as pinconf */
+               if (!strcmp(name, "function")) {
+                       setting->type = PIN_SETTING_TYPE_MUX;
+                       setting->func_or_conf = value;
+               } else {
+                       setting->type = PIN_SETTING_TYPE_CONFIG;
+                       setting->func_or_conf = name;
+                       if (len > 0)
+                               setting->conf_arg =
+                                       fdt32_to_cpu(*(fdt32_t *)value);
+               }
+
+               list_add_tail(&setting->node, settings);
+       }
+
+       return 0;
+}
+
+static int pinctrl_dt_subnode_to_setting(struct udevice *pctldev,
+                                        const void *fdt, int node_offset,
+                                        struct list_head *settings)
+{
+       const char *subnode_target_type = "pins";
+       const char *group;
+       int strings_count, i, ret;
+
+       strings_count = fdt_count_strings(fdt, node_offset, subnode_target_type);
+       if (strings_count < 0) {
+               subnode_target_type = "groups";
+               strings_count = fdt_count_strings(fdt, node_offset,
+                                                 subnode_target_type);
+       }
+       if (strings_count < 0)
+               return -EINVAL;
+
+       for (i = 0; i < strings_count; i++) {
+               ret = fdt_get_string_index(fdt, node_offset,
+                                          subnode_target_type, i, &group);
+               if (ret < 0)
+                       return -EINVAL;
+               ret = pinctrl_dt_subnode_to_setting_one_group(pctldev, fdt,
+                                                             node_offset,
+                                                             group, settings);
+               if (ret < 0)
+                       return ret;
+       }
+
+       return 0;
+}

drivers/pinctrl/pinctrl-uclass.c new file +87 -0

+               if (pctldev_node < 0) {
+                       printf("could not find pctldev for node %s\n",
+                              fdt_get_name(fdt, config_node, NULL));
+                       return -EINVAL;
+               }
+               /* is this node pinctrl device? */
+               ret = uclass_get_device_by_of_offset(UCLASS_PINCTRL,
+                                                    pctldev_node, &pctldev);
+               if (ret == 0)
+                       break;
+       }
+
+       ret = pinctrl_dt_subnode_to_setting(pctldev, fdt, config_node,
+                                           settings);
+       if (ret < 0)
+               return ret;
+
+       for (offset = fdt_first_subnode(fdt, config_node);
+            offset > 0;
+            offset = fdt_next_subnode(fdt, offset)) {
+               ret = pinctrl_dt_subnode_to_setting(pctldev, fdt, offset,
+                                                   settings);
+               if (ret < 0)
+                       return ret;
+       }
+
+       return 0;
+}
+
+static int pinctrl_dt_to_state(struct udevice *dev,
+                              const char *state_name,
+                              int state_index,
+                              struct pinctrl_state **pstate)
+{
+       DECLARE_GLOBAL_DATA_PTR;
+       const void *fdt = gd->fdt_blob;
+       int node = dev->of_offset;
+       struct pinctrl_state *state;
+       char propname[32]; /* long enough */
+       const fdt32_t *list;
+       int size, config, ret;
+
+       if (state_name)
+               state_index = fdt_find_string(fdt, node, "pinctrl-names",
+                                             state_name);
+
+       if (state_index < 0)
+               return -EINVAL;
+
+       snprintf(propname, sizeof(propname), "pinctrl-%d", state_index);
+       list = fdt_getprop(fdt, node, propname, &size);
+       if (!list)
+               return -EINVAL;
+
+       /*
+        * no return code check: if "pinctrl-names" is not specified,
+        * only use index.  In this case, state_name is NULL.
+        */
+       fdt_get_string_index(fdt, node, "pinctrl-names", state_index,
+                            &state_name);
+
+       state = malloc(sizeof(*state));
+       if (!state)
+               return -ENOMEM;
+       INIT_LIST_HEAD(&state->node);
+       state->name = state_name;
+       state->index = state_index;
+       INIT_LIST_HEAD(&state->settings);
+
+       size /= sizeof(*list);
+
+       for (config = 0; config < size; config++) {
+               int phandle, config_node;
+
+               phandle = fdt32_to_cpu(*list++);
+
+               config_node = fdt_node_offset_by_phandle(fdt, phandle);
+               if (config_node < 0) {
+                       printf("prop %s index %d invalid phandle\n",
+                              propname, config);
+                       ret = -EINVAL;
+                       goto err;
+               }
+               ret = pinctrl_dt_to_setting_one_config(fdt, config_node,
+                                                      &state->settings);
+               if (ret)
+                       goto err;

drivers/pinctrl/pinctrl-uclass.c new file +18 -0

+{
+       struct pinctrl_state *state;
+       char *end;
+       int state_index;
+       int ret;
+       bool search_by_index = false;
+
+       if (!dev)
+               return -EINVAL;
+
+       state_index = simple_strtoul(state_name, &end, 10);
+       if (*end == 0) {
+               search_by_index = true;
+       }
+
+       list_for_each_entry(state, &dev->pinctrl_states, node) {
+               if (search_by_index) {
+                       if (state->index != state_index)

drivers/pinctrl/pinctrl-uclass.c new file +36 -0

+        */
+       if (search_by_index)
+               ret = pinctrl_dt_to_state(dev, NULL, state_index, &state);
+       else
+               ret = pinctrl_dt_to_state(dev, state_name, -1, &state);
+
+       if (ret == 0) {
+               list_add_tail(&state->node, &dev->pinctrl_states);
+               return pinctrl_select_state(state);
+       }
+#endif
+
+       /*
+        * TODO: Masahiro Yamada
+        * As not all the boards support Device Tree,
+        * add board file support here.
+        */
+
+       /* requested pin-state not found, ignore it */
+       return 0;
+}
+
+int pinctrl_init(struct udevice *dev)
+{
+       if (!dev)
+               return -EINVAL;
+
+       INIT_LIST_HEAD(&dev->pinctrl_states);
+
+       return pinctrl_set_state(dev, "default");
+}
+
+UCLASS_DRIVER(pinctrl) = {
+       .id = UCLASS_PINCTRL,
+       .name = "pinctrl",
+};

include/dm/device.h +3 -0

index 18296bb..05d4e40 100644
@@ -93,6 +93,9 @@ struct udevice {
        uint32_t flags;
        int req_seq;
        int seq;
+#ifdef CONFIG_PINCTRL
+       struct list_head pinctrl_states;
+#endif
 };
 
 /* Maximum sequence number supported */

include/dm/pinctrl.h new file +9 -0

new file mode 100644
index 0000000..95925ca
@@ -0,0 +1,28 @@
+#ifndef __PINCTRL_H
+#define __PINCTRL_H
+
+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);
+};

include/dm/pinctrl.h new file +9 -0

+}
+
+static inline int pinctrl_init(struct udevice *dev)
+{
+       return 0;
+}
+#endif
+
+#endif /* __PINCTRL_H */

include/dm/uclass-id.h +0 -0

index c7310d7..546f7fb 100644

include/dm/uclass-id.h +0 -1

        UCLASS_PMIC,            /* PMIC I/O device */
        UCLASS_REGULATOR,       /* Regulator device */
        UCLASS_RTC,             /* Real time clock device */
--

1.9.1

Hi Masahiro,

Quoted 11 lines · Masahiro Yamada

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/

I'm sending some comments on this series since I think you are planning to rework it. My plan is:

- you respin the series
- apply this series
- it should includes the 'simple' version added to the uclass and perhaps a CONFIG to select whether the full pinctrl is available (can be separate patches on top of what you have)
- work out what we are going to do about GPIO / pull-ups

Can/does this use exactly the same binding as Linux? Re the limitation about not detecting conflicts, why is that? It seems that code could be added for that, even if optional.

Quoted 146 lines

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
----

Quoted 4 lines

[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)

I don't think we should support that. People should have to use the simple version for SPL.

Quoted 71 lines

[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.

Signed-off-by Masahiro Yamada <[email protected]>

---
 drivers/Kconfig                  |   2 +
 drivers/Makefile                 |   1 +
 drivers/core/device.c            |   5 +
 drivers/pinctrl/Kconfig          |   2 +
 drivers/pinctrl/Makefile         |   1 +
 drivers/pinctrl/pinctrl-uclass.c | 353 +++++++++++++++++++++++++++++++++++++++
 include/dm/device.h              |   3 +
 include/dm/pinctrl.h             |  28 ++++
 include/dm/uclass-id.h           |   1 +
 9 files changed, 396 insertions(+)
 create mode 100644 drivers/pinctrl/Kconfig
 create mode 100644 drivers/pinctrl/Makefile
 create mode 100644 drivers/pinctrl/pinctrl-uclass.c
 create mode 100644 include/dm/pinctrl.h

drivers/Kconfig +2 -0

index c7e526c..c696036 100644
@@ -28,6 +28,8 @@ source "drivers/i2c/Kconfig"
 
 source "drivers/spi/Kconfig"
 
+source "drivers/pinctrl/Kconfig"
+
 source "drivers/gpio/Kconfig"
 
 source "drivers/power/Kconfig"

drivers/Makefile +1 -0

index 280b6d1..7e5ae84 100644
@@ -4,6 +4,7 @@ obj-$(CONFIG_OF_CONTROL) += fdt/
 obj-$(CONFIG_BIOSEMU) += bios_emulator/
 obj-y += block/
 obj-$(CONFIG_BOOTCOUNT_LIMIT) += bootcount/
+obj-$(CONFIG_PINCTRL) += pinctrl/
 obj-$(CONFIG_CPU) += cpu/
 obj-y += crypto/
 obj-$(CONFIG_FPGA) += fpga/

drivers/core/device.c +1 -0

index 46bf388..4e94a4f 100644
@@ -15,6 +15,7 @@
 #include <dm/device.h>
 #include <dm/device-internal.h>
 #include <dm/lists.h>
+#include <dm/pinctrl.h>
 #include <dm/platdata.h>
 #include <dm/uclass.h>
 #include <dm/uclass-internal.h>

@@ -268,6 +269,10 @@ int device_probe_child(struct udevice *dev, void *parent_priv)
 
        dev->flags |= DM_FLAG_ACTIVATED;
 

drivers/core/device.c int device_probe_child(struct udevice *dev, void *parent_priv) · line 275

Quoted 4 lines

drivers/core/device.c +4 -0

+       ret = pinctrl_init(dev);
+       if (ret)
+               goto fail;
+

I don't think there should be a pinctrl list on every device. Can we instead have a pointer which is initially NULL? Then we could lazily scan the pinctrl settings later when a request is made, and add a structure to the device then.

Quoted 45 lines

drivers/core/device.c +0 -0

        ret = uclass_pre_probe_device(dev);
        if (ret)
                goto fail;

drivers/pinctrl/Kconfig new file +2 -0

new file mode 100644
index 0000000..cae4d55
@@ -0,0 +1,2 @@
+config PINCTRL
+       bool

drivers/pinctrl/Makefile new file +1 -0

new file mode 100644
index 0000000..a775a4c
@@ -0,0 +1 @@
+obj-y                          += pinctrl-uclass.o

drivers/pinctrl/pinctrl-uclass.c new file +21 -0

new file mode 100644
index 0000000..68e4842
@@ -0,0 +1,353 @@
+/*
+ * Copyright (C) 2015  Masahiro Yamada <[email protected]>
+ *
+ * SPDX-License-Identifier:    GPL-2.0+
+ */
+
+#include <common.h>
+#include <libfdt.h>
+#include <linux/err.h>
+#include <linux/list.h>
+#include <dm/device.h>
+#include <dm/pinctrl.h>
+#include <dm/uclass.h>
+
+enum pinctrl_setting_type {
+       PIN_SETTING_TYPE_INVALID,
+       PIN_SETTING_TYPE_MUX,
+       PIN_SETTING_TYPE_CONFIG,
+};
+
+struct pinctrl_state {

drivers/pinctrl/pinctrl-uclass.c line 27

Quoted 6 lines

drivers/pinctrl/pinctrl-uclass.c new file +6 -0

+       struct list_head node;
+       const char *name;
+       int index;
+       struct list_head settings;
+};
+

Comments

Quoted 11 lines

drivers/pinctrl/pinctrl-uclass.c new file +11 -0

+struct pinctrl_setting {
+       struct list_head node;
+       enum pinctrl_setting_type type;
+       struct udevice *pctldev;
+       const char *group;
+       const char *func_or_conf;
+       unsigned conf_arg;
+};
+
+static int pinmux_enable_setting(struct pinctrl_setting *setting)
+{

drivers/pinctrl/pinctrl-uclass.c line 44

Quoted 6 lines

drivers/pinctrl/pinctrl-uclass.c new file +6 -0

+       struct udevice *pctldev = setting->pctldev;
+       const struct pinctrl_ops *ops = pctldev->driver->ops;
+
+       if (!ops->pinmux_set) {
+               printf("%s: pinmux_set is not defined.  skip.\n",
+                      pctldev->name);

Should this not be return -ENOENT or similar?

Quoted 62 lines

drivers/pinctrl/pinctrl-uclass.c new file +62 -0

+               return 0;
+       }
+
+       return ops->pinmux_set(pctldev, setting->group, setting->func_or_conf);
+}
+
+static int pinconf_enable_setting(struct pinctrl_setting *setting)
+{
+       struct udevice *pctldev = setting->pctldev;
+       const struct pinctrl_ops *ops = pctldev->driver->ops;
+
+       if (!ops->pinconf_set) {
+               printf("%s: pin_config_set is not defined.  skip.\n",
+                      pctldev->name);
+               return 0;
+       }
+
+       return ops->pinconf_set(pctldev, setting->group, setting->func_or_conf,
+                               setting->conf_arg);
+}
+
+static int pinctrl_select_state(struct pinctrl_state *state)
+{
+       struct pinctrl_setting *setting;
+       int ret;
+
+       list_for_each_entry(setting, &state->settings, node) {
+               switch (setting->type) {
+               case PIN_SETTING_TYPE_MUX:
+                       ret = pinmux_enable_setting(setting);
+                       break;
+               case PIN_SETTING_TYPE_CONFIG:
+                       ret = pinconf_enable_setting(setting);
+                       break;
+               default:
+                       ret = -EINVAL;
+                       break;
+               }
+
+               if (ret)
+                       return ret;
+       }
+
+       return 0;
+}
+
+#ifdef CONFIG_OF_CONTROL
+static int pinctrl_dt_subnode_to_setting_one_group(struct udevice *pctldev,
+                                                  const void *fdt, int node_offset,
+                                                  const char *group,
+                                                  struct list_head *settings)
+{
+       struct pinctrl_setting *setting;
+       const char *name;
+       const void *value;
+       int prop_offset, len;
+
+       for (prop_offset = fdt_first_property_offset(fdt, node_offset);
+            prop_offset > 0;
+            prop_offset = fdt_next_property_offset(fdt, prop_offset)) {
+
+               value = fdt_getprop_by_offset(fdt, prop_offset, &name, &len);

drivers/pinctrl/pinctrl-uclass.c line 112

Quoted 6 lines

drivers/pinctrl/pinctrl-uclass.c new file +6 -0

+               if (!value)
+                       return -EINVAL;
+
+               if (!strcmp(name, "pins") || !strcmp(name, "groups") ||
+                   !strcmp(name, "phandle") || !strcmp(name, "linux,phandle"))
+                   continue;

Is there a positive test we can use? I guess not as it can be anything...

Quoted 64 lines

drivers/pinctrl/pinctrl-uclass.c new file +64 -0

+
+               setting = calloc(1, sizeof(*setting));
+               if (!setting)
+                       return -ENOMEM;
+
+               INIT_LIST_HEAD(&setting->node);
+               setting->pctldev = pctldev;
+               setting->group = group;
+
+               /* we treat properties other than "function" as pinconf */
+               if (!strcmp(name, "function")) {
+                       setting->type = PIN_SETTING_TYPE_MUX;
+                       setting->func_or_conf = value;
+               } else {
+                       setting->type = PIN_SETTING_TYPE_CONFIG;
+                       setting->func_or_conf = name;
+                       if (len > 0)
+                               setting->conf_arg =
+                                       fdt32_to_cpu(*(fdt32_t *)value);
+               }
+
+               list_add_tail(&setting->node, settings);
+       }
+
+       return 0;
+}
+
+static int pinctrl_dt_subnode_to_setting(struct udevice *pctldev,
+                                        const void *fdt, int node_offset,
+                                        struct list_head *settings)
+{
+       const char *subnode_target_type = "pins";
+       const char *group;
+       int strings_count, i, ret;
+
+       strings_count = fdt_count_strings(fdt, node_offset, subnode_target_type);
+       if (strings_count < 0) {
+               subnode_target_type = "groups";
+               strings_count = fdt_count_strings(fdt, node_offset,
+                                                 subnode_target_type);
+       }
+       if (strings_count < 0)
+               return -EINVAL;
+
+       for (i = 0; i < strings_count; i++) {
+               ret = fdt_get_string_index(fdt, node_offset,
+                                          subnode_target_type, i, &group);
+               if (ret < 0)
+                       return -EINVAL;
+               ret = pinctrl_dt_subnode_to_setting_one_group(pctldev, fdt,
+                                                             node_offset,
+                                                             group, settings);
+               if (ret < 0)
+                       return ret;
+       }
+
+       return 0;
+}
+
+static int pinctrl_dt_to_setting_one_config(const void *fdt, int config_node,
+                                           struct list_head *settings)
+{
+       struct udevice *pctldev;
+       int pctldev_node, offset, ret;

drivers/pinctrl/pinctrl-uclass.c line 180

Quoted 4 lines

drivers/pinctrl/pinctrl-uclass.c new file +4 -0

+
+       pctldev_node = config_node;
+       for (;;) {
+               pctldev_node = fdt_parent_offset(fdt, pctldev_node);

()
This should use dev->parent->of_offset as fdt_parent_offset is dreadfully slow. Why is this going up through the tree? Could use a comment.

Quoted 92 lines

drivers/pinctrl/pinctrl-uclass.c new file +92 -0

+               if (pctldev_node < 0) {
+                       printf("could not find pctldev for node %s\n",
+                              fdt_get_name(fdt, config_node, NULL));
+                       return -EINVAL;
+               }
+               /* is this node pinctrl device? */
+               ret = uclass_get_device_by_of_offset(UCLASS_PINCTRL,
+                                                    pctldev_node, &pctldev);
+               if (ret == 0)
+                       break;
+       }
+
+       ret = pinctrl_dt_subnode_to_setting(pctldev, fdt, config_node,
+                                           settings);
+       if (ret < 0)
+               return ret;
+
+       for (offset = fdt_first_subnode(fdt, config_node);
+            offset > 0;
+            offset = fdt_next_subnode(fdt, offset)) {
+               ret = pinctrl_dt_subnode_to_setting(pctldev, fdt, offset,
+                                                   settings);
+               if (ret < 0)
+                       return ret;
+       }
+
+       return 0;
+}
+
+static int pinctrl_dt_to_state(struct udevice *dev,
+                              const char *state_name,
+                              int state_index,
+                              struct pinctrl_state **pstate)
+{
+       DECLARE_GLOBAL_DATA_PTR;
+       const void *fdt = gd->fdt_blob;
+       int node = dev->of_offset;
+       struct pinctrl_state *state;
+       char propname[32]; /* long enough */
+       const fdt32_t *list;
+       int size, config, ret;
+
+       if (state_name)
+               state_index = fdt_find_string(fdt, node, "pinctrl-names",
+                                             state_name);
+
+       if (state_index < 0)
+               return -EINVAL;
+
+       snprintf(propname, sizeof(propname), "pinctrl-%d", state_index);
+       list = fdt_getprop(fdt, node, propname, &size);
+       if (!list)
+               return -EINVAL;
+
+       /*
+        * no return code check: if "pinctrl-names" is not specified,
+        * only use index.  In this case, state_name is NULL.
+        */
+       fdt_get_string_index(fdt, node, "pinctrl-names", state_index,
+                            &state_name);
+
+       state = malloc(sizeof(*state));
+       if (!state)
+               return -ENOMEM;
+       INIT_LIST_HEAD(&state->node);
+       state->name = state_name;
+       state->index = state_index;
+       INIT_LIST_HEAD(&state->settings);
+
+       size /= sizeof(*list);
+
+       for (config = 0; config < size; config++) {
+               int phandle, config_node;
+
+               phandle = fdt32_to_cpu(*list++);
+
+               config_node = fdt_node_offset_by_phandle(fdt, phandle);
+               if (config_node < 0) {
+                       printf("prop %s index %d invalid phandle\n",
+                              propname, config);
+                       ret = -EINVAL;
+                       goto err;
+               }
+               ret = pinctrl_dt_to_setting_one_config(fdt, config_node,
+                                                      &state->settings);
+               if (ret)
+                       goto err;
+       }
+
+err:
+       if (ret < 0) {
+               struct pinctrl_setting *setting, *n;

drivers/pinctrl/pinctrl-uclass.c line 277

Quoted 5 lines

drivers/pinctrl/pinctrl-uclass.c new file +5 -0

+
+               list_for_each_entry_safe(setting, n, &state->settings, node)
+                       free(setting);
+
+               free(state);

Perhaps this code should go in its own function so you can use it when removing the device?

Quoted 4 lines

drivers/pinctrl/pinctrl-uclass.c new file +4 -0

+       }
+
+       *pstate = state;
+

drivers/pinctrl/pinctrl-uclass.c line 286

Quoted 5 lines

drivers/pinctrl/pinctrl-uclass.c new file +5 -0

+       return ret;
+}
+#endif
+
+int pinctrl_set_state(struct udevice *dev, const char *state_name)

This seems to name a name or an index. I think there should be two functions, one which takes a name and one which takes an index.

Quoted 22 lines

drivers/pinctrl/pinctrl-uclass.c new file +22 -0

+{
+       struct pinctrl_state *state;
+       char *end;
+       int state_index;
+       int ret;
+       bool search_by_index = false;
+
+       if (!dev)
+               return -EINVAL;
+
+       state_index = simple_strtoul(state_name, &end, 10);
+       if (*end == 0) {
+               search_by_index = true;
+       }
+
+       list_for_each_entry(state, &dev->pinctrl_states, node) {
+               if (search_by_index) {
+                       if (state->index != state_index)
+                               continue;
+               } else {
+                       if (strcmp(state->name, state_name))
+                               continue;

drivers/pinctrl/pinctrl-uclass.c line 314

Quoted 6 lines

drivers/pinctrl/pinctrl-uclass.c new file +6 -0

+               }
+
+               return pinctrl_select_state(state);
+       }
+
+#ifdef CONFIG_OF_CONTROL

I think we can assumed this is defined.

drivers/pinctrl/pinctrl-uclass.c line 317

Quoted 3 lines

drivers/pinctrl/pinctrl-uclass.c new file +3 -0

+       /*
+        * if the state was not found in the linked list,
+        * search the device tree

Why not populate the table at start-up?

Quoted 69 lines

drivers/pinctrl/pinctrl-uclass.c new file +36 -0

+        */
+       if (search_by_index)
+               ret = pinctrl_dt_to_state(dev, NULL, state_index, &state);
+       else
+               ret = pinctrl_dt_to_state(dev, state_name, -1, &state);
+
+       if (ret == 0) {
+               list_add_tail(&state->node, &dev->pinctrl_states);
+               return pinctrl_select_state(state);
+       }
+#endif
+
+       /*
+        * TODO: Masahiro Yamada
+        * As not all the boards support Device Tree,
+        * add board file support here.
+        */
+
+       /* requested pin-state not found, ignore it */
+       return 0;
+}
+
+int pinctrl_init(struct udevice *dev)
+{
+       if (!dev)
+               return -EINVAL;
+
+       INIT_LIST_HEAD(&dev->pinctrl_states);
+
+       return pinctrl_set_state(dev, "default");
+}
+
+UCLASS_DRIVER(pinctrl) = {
+       .id = UCLASS_PINCTRL,
+       .name = "pinctrl",
+};

include/dm/device.h +3 -0

index 18296bb..05d4e40 100644
@@ -93,6 +93,9 @@ struct udevice {
        uint32_t flags;
        int req_seq;
        int seq;
+#ifdef CONFIG_PINCTRL
+       struct list_head pinctrl_states;
+#endif
 };
 
 /* Maximum sequence number supported */

include/dm/pinctrl.h new file +13 -0

new file mode 100644
index 0000000..95925ca
@@ -0,0 +1,28 @@
+#ifndef __PINCTRL_H
+#define __PINCTRL_H
+
+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);
+};
+
+#if (!defined(CONFIG_SPL_BUILD) && defined(CONFIG_PINCTRL)) ||         \
+               (defined(CONFIG_SPL_BUILD) && defined(CONFIG_SPL_PINCTRL))
+int pinctrl_set_state(struct udevice *dev, const char *state_name);

include/dm/pinctrl.h line 19

Quoted 6 lines

include/dm/pinctrl.h new file +6 -0

+int pinctrl_init(struct udevice *dev);
+#else
+static inline int pinctrl_set_state(struct udevice *dev,
+                                   const char *state_name)
+{
+       return 0;

return -ENOSYS?

Quoted 12 lines

include/dm/pinctrl.h new file +9 -0

+}
+
+static inline int pinctrl_init(struct udevice *dev)
+{
+       return 0;
+}
+#endif
+
+#endif /* __PINCTRL_H */

include/dm/uclass-id.h +0 -1

index c7310d7..546f7fb 100644
--- a/include/dm/uclass-id.h

include/dm/uclass-id.h enum uclass_id { · line 42

Quoted 6 lines

+++ b/include/dm/uclass-id.h

include/dm/uclass-id.h +1 -0

@@ -39,6 +39,7 @@ enum uclass_id {
        UCLASS_PCH,             /* x86 platform controller hub */
        UCLASS_PCI,             /* PCI bus */
        UCLASS_PCI_GENERIC,     /* Generic PCI bus device */
+       UCLASS_PINCTRL,         /* Pinctrl device */

That's not a very useful comment!

Hi Simon,

2015-07-30 11:06 GMT+09:00 Simon Glass <[email protected]>:

Quoted 18 lines

Hi Masahiro,

Quoted 11 lines · Masahiro Yamada

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/

I'm sending some comments on this series since I think you are planning to rework it. My plan is:

Quoted 6 lines

- you respin the series
- apply this series
- it should includes the 'simple' version added to the uclass and perhaps a CONFIG to select whether the full pinctrl is available (can be separate patches on top of what you have)
- work out what we are going to do about GPIO / pull-ups

OK, I will do it.

Now I am pressed, so hopefully I will have some time this weekend.

Quoted 3 lines

Can/does this use exactly the same binding as Linux? Re the limitation about not detecting conflicts, why is that? It seems that code could be added for that, even if optional.

I think this series can use the same Generic-conf bindings as used in Linux. Each driver is still allowed to use its own special bindings. This version does not support it, but it should not be difficult.

To detect pin conflicts, the framework needs to know which groups each pin belongs to.

For example, pin 10 is shared between I2C, UART, and SPI,
pin 20 is shared between MMC and NAND, etc.

In other words, low level drivers must have big pin & group tables like they have in Linux.
Most of pinctrl drivers in Linux are over 1000 lines.
I thought it was too much for U-Boot.

To support full features, the pinctrl-uclass would get much bigger. Of course, we can make it optional with some CONFIG, but I wonder if it is really needed.

Quoted 154 lines

Quoted 153 lines

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)

Quoted 2 lines

I don't think we should support that. People should have to use the simple version for SPL.

OK. This is the limitation.

OF_CONTROL is required to use full-pinctrl.
Otherwise, only simple-pinctrol is available.

Quoted 75 lines

Quoted 75 lines

[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.

Signed-off-by Masahiro Yamada <[email protected]>

---
 drivers/Kconfig                  |   2 +
 drivers/Makefile                 |   1 +
 drivers/core/device.c            |   5 +
 drivers/pinctrl/Kconfig          |   2 +
 drivers/pinctrl/Makefile         |   1 +
 drivers/pinctrl/pinctrl-uclass.c | 353 +++++++++++++++++++++++++++++++++++++++
 include/dm/device.h              |   3 +
 include/dm/pinctrl.h             |  28 ++++
 include/dm/uclass-id.h           |   1 +
 9 files changed, 396 insertions(+)
 create mode 100644 drivers/pinctrl/Kconfig
 create mode 100644 drivers/pinctrl/Makefile
 create mode 100644 drivers/pinctrl/pinctrl-uclass.c
 create mode 100644 include/dm/pinctrl.h

drivers/Kconfig +2 -0

index c7e526c..c696036 100644
@@ -28,6 +28,8 @@ source "drivers/i2c/Kconfig"
 
 source "drivers/spi/Kconfig"
 
+source "drivers/pinctrl/Kconfig"
+
 source "drivers/gpio/Kconfig"
 
 source "drivers/power/Kconfig"

drivers/Makefile +1 -0

index 280b6d1..7e5ae84 100644
@@ -4,6 +4,7 @@ obj-$(CONFIG_OF_CONTROL) += fdt/
 obj-$(CONFIG_BIOSEMU) += bios_emulator/
 obj-y += block/
 obj-$(CONFIG_BOOTCOUNT_LIMIT) += bootcount/
+obj-$(CONFIG_PINCTRL) += pinctrl/
 obj-$(CONFIG_CPU) += cpu/
 obj-y += crypto/
 obj-$(CONFIG_FPGA) += fpga/

drivers/core/device.c +1 -0

index 46bf388..4e94a4f 100644
@@ -15,6 +15,7 @@
 #include <dm/device.h>
 #include <dm/device-internal.h>
 #include <dm/lists.h>
+#include <dm/pinctrl.h>
 #include <dm/platdata.h>
 #include <dm/uclass.h>
 #include <dm/uclass-internal.h>

@@ -268,6 +269,10 @@ int device_probe_child(struct udevice *dev, void *parent_priv)
 
        dev->flags |= DM_FLAG_ACTIVATED;
 

Quoted 9 lines

drivers/core/device.c int device_probe_child(struct udevice *dev, void *parent_priv) · line 275

Quoted 4 lines

drivers/core/device.c +4 -0

+       ret = pinctrl_init(dev);
+       if (ret)
+               goto fail;
+

I don't think there should be a pinctrl list on every device. Can we instead have a pointer which is initially NULL? Then we could lazily scan the pinctrl settings later when a request is made, and add a structure to the device then.

The pinctrl_init() bails out soon
if the pinconf node is missing from the device tree. I do not think it is a big over head.

If we delay as you suggest, when should it be invoked?

Should each low-level driver call it explicitly?

Quoted 49 lines

Quoted 49 lines

drivers/core/device.c +0 -0

        ret = uclass_pre_probe_device(dev);
        if (ret)
                goto fail;

drivers/pinctrl/Kconfig new file +2 -0

new file mode 100644
index 0000000..cae4d55
@@ -0,0 +1,2 @@
+config PINCTRL
+       bool

drivers/pinctrl/Makefile new file +1 -0

new file mode 100644
index 0000000..a775a4c
@@ -0,0 +1 @@
+obj-y                          += pinctrl-uclass.o

drivers/pinctrl/pinctrl-uclass.c new file +21 -0

new file mode 100644
index 0000000..68e4842
@@ -0,0 +1,353 @@
+/*
+ * Copyright (C) 2015  Masahiro Yamada <[email protected]>
+ *
+ * SPDX-License-Identifier:    GPL-2.0+
+ */
+
+#include <common.h>
+#include <libfdt.h>
+#include <linux/err.h>
+#include <linux/list.h>
+#include <dm/device.h>
+#include <dm/pinctrl.h>
+#include <dm/uclass.h>
+
+enum pinctrl_setting_type {
+       PIN_SETTING_TYPE_INVALID,
+       PIN_SETTING_TYPE_MUX,
+       PIN_SETTING_TYPE_CONFIG,
+};
+
+struct pinctrl_state {

Quoted 8 lines

drivers/pinctrl/pinctrl-uclass.c line 27

Quoted 6 lines

drivers/pinctrl/pinctrl-uclass.c new file +6 -0

+       struct list_head node;
+       const char *name;
+       int index;
+       struct list_head settings;
+};
+

Comments

OK.

Quoted 11 lines

Quoted 11 lines

drivers/pinctrl/pinctrl-uclass.c new file +11 -0

+struct pinctrl_setting {
+       struct list_head node;
+       enum pinctrl_setting_type type;
+       struct udevice *pctldev;
+       const char *group;
+       const char *func_or_conf;
+       unsigned conf_arg;
+};
+
+static int pinmux_enable_setting(struct pinctrl_setting *setting)
+{

Quoted 8 lines

drivers/pinctrl/pinctrl-uclass.c line 44

Quoted 6 lines

drivers/pinctrl/pinctrl-uclass.c new file +6 -0

+       struct udevice *pctldev = setting->pctldev;
+       const struct pinctrl_ops *ops = pctldev->driver->ops;
+
+       if (!ops->pinmux_set) {
+               printf("%s: pinmux_set is not defined.  skip.\n",
+                      pctldev->name);

Should this not be return -ENOENT or similar?

This return value is propagated to device_probe()

If a negative value is returned, probing itself fails.

I assume, if pinmux_set() is missing, probably pin-controlling is not necessary at all.

Quoted 65 lines

Quoted 65 lines

drivers/pinctrl/pinctrl-uclass.c new file +62 -0

+               return 0;
+       }
+
+       return ops->pinmux_set(pctldev, setting->group, setting->func_or_conf);
+}
+
+static int pinconf_enable_setting(struct pinctrl_setting *setting)
+{
+       struct udevice *pctldev = setting->pctldev;
+       const struct pinctrl_ops *ops = pctldev->driver->ops;
+
+       if (!ops->pinconf_set) {
+               printf("%s: pin_config_set is not defined.  skip.\n",
+                      pctldev->name);
+               return 0;
+       }
+
+       return ops->pinconf_set(pctldev, setting->group, setting->func_or_conf,
+                               setting->conf_arg);
+}
+
+static int pinctrl_select_state(struct pinctrl_state *state)
+{
+       struct pinctrl_setting *setting;
+       int ret;
+
+       list_for_each_entry(setting, &state->settings, node) {
+               switch (setting->type) {
+               case PIN_SETTING_TYPE_MUX:
+                       ret = pinmux_enable_setting(setting);
+                       break;
+               case PIN_SETTING_TYPE_CONFIG:
+                       ret = pinconf_enable_setting(setting);
+                       break;
+               default:
+                       ret = -EINVAL;
+                       break;
+               }
+
+               if (ret)
+                       return ret;
+       }
+
+       return 0;
+}
+
+#ifdef CONFIG_OF_CONTROL
+static int pinctrl_dt_subnode_to_setting_one_group(struct udevice *pctldev,
+                                                  const void *fdt, int node_offset,
+                                                  const char *group,
+                                                  struct list_head *settings)
+{
+       struct pinctrl_setting *setting;
+       const char *name;
+       const void *value;
+       int prop_offset, len;
+
+       for (prop_offset = fdt_first_property_offset(fdt, node_offset);
+            prop_offset > 0;
+            prop_offset = fdt_next_property_offset(fdt, prop_offset)) {
+
+               value = fdt_getprop_by_offset(fdt, prop_offset, &name, &len);

Quoted 8 lines

drivers/pinctrl/pinctrl-uclass.c line 112

Quoted 6 lines

drivers/pinctrl/pinctrl-uclass.c new file +6 -0

+               if (!value)
+                       return -EINVAL;
+
+               if (!strcmp(name, "pins") || !strcmp(name, "groups") ||
+                   !strcmp(name, "phandle") || !strcmp(name, "linux,phandle"))
+                   continue;

Is there a positive test we can use? I guess not as it can be anything...

Uh, this is too easily compromised...
Needs fixing.

Quoted 66 lines

Quoted 66 lines

drivers/pinctrl/pinctrl-uclass.c new file +64 -0

+
+               setting = calloc(1, sizeof(*setting));
+               if (!setting)
+                       return -ENOMEM;
+
+               INIT_LIST_HEAD(&setting->node);
+               setting->pctldev = pctldev;
+               setting->group = group;
+
+               /* we treat properties other than "function" as pinconf */
+               if (!strcmp(name, "function")) {
+                       setting->type = PIN_SETTING_TYPE_MUX;
+                       setting->func_or_conf = value;
+               } else {
+                       setting->type = PIN_SETTING_TYPE_CONFIG;
+                       setting->func_or_conf = name;
+                       if (len > 0)
+                               setting->conf_arg =
+                                       fdt32_to_cpu(*(fdt32_t *)value);
+               }
+
+               list_add_tail(&setting->node, settings);
+       }
+
+       return 0;
+}
+
+static int pinctrl_dt_subnode_to_setting(struct udevice *pctldev,
+                                        const void *fdt, int node_offset,
+                                        struct list_head *settings)
+{
+       const char *subnode_target_type = "pins";
+       const char *group;
+       int strings_count, i, ret;
+
+       strings_count = fdt_count_strings(fdt, node_offset, subnode_target_type);
+       if (strings_count < 0) {
+               subnode_target_type = "groups";
+               strings_count = fdt_count_strings(fdt, node_offset,
+                                                 subnode_target_type);
+       }
+       if (strings_count < 0)
+               return -EINVAL;
+
+       for (i = 0; i < strings_count; i++) {
+               ret = fdt_get_string_index(fdt, node_offset,
+                                          subnode_target_type, i, &group);
+               if (ret < 0)
+                       return -EINVAL;
+               ret = pinctrl_dt_subnode_to_setting_one_group(pctldev, fdt,
+                                                             node_offset,
+                                                             group, settings);
+               if (ret < 0)
+                       return ret;
+       }
+
+       return 0;
+}
+
+static int pinctrl_dt_to_setting_one_config(const void *fdt, int config_node,
+                                           struct list_head *settings)
+{
+       struct udevice *pctldev;
+       int pctldev_node, offset, ret;

Quoted 8 lines

drivers/pinctrl/pinctrl-uclass.c line 180

Quoted 4 lines

drivers/pinctrl/pinctrl-uclass.c new file +4 -0

+
+       pctldev_node = config_node;
+       for (;;) {
+               pctldev_node = fdt_parent_offset(fdt, pctldev_node);

()
This should use dev->parent->of_offset as fdt_parent_offset is dreadfully slow. Why is this going up through the tree? Could use a comment.

This is the only way to find the pinctrl device associated to the pinctrl consumers.

(1) Pinctrl consumers should have "pinctrl-0" property
(2) pinctrl-0 points to a phandle to its pinconf node.
(3) The parent of the pinconf node is the pinctrl device.

The biggest disadvantage of parsing DT as flattened is that we cannot directly find the parent node. The libfdt always parses DT from the beginning.

Unfortunately dev->parent->of_offset cannot be used either, because this node is not a device. It is not bound. I will try to look for a better way.

Quoted 97 lines

Quoted 97 lines

drivers/pinctrl/pinctrl-uclass.c new file +92 -0

+               if (pctldev_node < 0) {
+                       printf("could not find pctldev for node %s\n",
+                              fdt_get_name(fdt, config_node, NULL));
+                       return -EINVAL;
+               }
+               /* is this node pinctrl device? */
+               ret = uclass_get_device_by_of_offset(UCLASS_PINCTRL,
+                                                    pctldev_node, &pctldev);
+               if (ret == 0)
+                       break;
+       }
+
+       ret = pinctrl_dt_subnode_to_setting(pctldev, fdt, config_node,
+                                           settings);
+       if (ret < 0)
+               return ret;
+
+       for (offset = fdt_first_subnode(fdt, config_node);
+            offset > 0;
+            offset = fdt_next_subnode(fdt, offset)) {
+               ret = pinctrl_dt_subnode_to_setting(pctldev, fdt, offset,
+                                                   settings);
+               if (ret < 0)
+                       return ret;
+       }
+
+       return 0;
+}
+
+static int pinctrl_dt_to_state(struct udevice *dev,
+                              const char *state_name,
+                              int state_index,
+                              struct pinctrl_state **pstate)
+{
+       DECLARE_GLOBAL_DATA_PTR;
+       const void *fdt = gd->fdt_blob;
+       int node = dev->of_offset;
+       struct pinctrl_state *state;
+       char propname[32]; /* long enough */
+       const fdt32_t *list;
+       int size, config, ret;
+
+       if (state_name)
+               state_index = fdt_find_string(fdt, node, "pinctrl-names",
+                                             state_name);
+
+       if (state_index < 0)
+               return -EINVAL;
+
+       snprintf(propname, sizeof(propname), "pinctrl-%d", state_index);
+       list = fdt_getprop(fdt, node, propname, &size);
+       if (!list)
+               return -EINVAL;
+
+       /*
+        * no return code check: if "pinctrl-names" is not specified,
+        * only use index.  In this case, state_name is NULL.
+        */
+       fdt_get_string_index(fdt, node, "pinctrl-names", state_index,
+                            &state_name);
+
+       state = malloc(sizeof(*state));
+       if (!state)
+               return -ENOMEM;
+       INIT_LIST_HEAD(&state->node);
+       state->name = state_name;
+       state->index = state_index;
+       INIT_LIST_HEAD(&state->settings);
+
+       size /= sizeof(*list);
+
+       for (config = 0; config < size; config++) {
+               int phandle, config_node;
+
+               phandle = fdt32_to_cpu(*list++);
+
+               config_node = fdt_node_offset_by_phandle(fdt, phandle);
+               if (config_node < 0) {
+                       printf("prop %s index %d invalid phandle\n",
+                              propname, config);
+                       ret = -EINVAL;
+                       goto err;
+               }
+               ret = pinctrl_dt_to_setting_one_config(fdt, config_node,
+                                                      &state->settings);
+               if (ret)
+                       goto err;
+       }
+
+err:
+       if (ret < 0) {
+               struct pinctrl_setting *setting, *n;

Quoted 8 lines

drivers/pinctrl/pinctrl-uclass.c line 277

Quoted 5 lines

drivers/pinctrl/pinctrl-uclass.c new file +5 -0

+
+               list_for_each_entry_safe(setting, n, &state->settings, node)
+                       free(setting);
+
+               free(state);

Perhaps this code should go in its own function so you can use it when removing the device?

Right.
I did not think of removing path earnestly.

Quoted 4 lines

Quoted 4 lines

drivers/pinctrl/pinctrl-uclass.c new file +4 -0

+       }
+
+       *pstate = state;
+

Quoted 8 lines

drivers/pinctrl/pinctrl-uclass.c line 286

Quoted 5 lines

drivers/pinctrl/pinctrl-uclass.c new file +5 -0

+       return ret;
+}
+#endif
+
+int pinctrl_set_state(struct udevice *dev, const char *state_name)

This seems to name a name or an index. I think there should be two functions, one which takes a name and one which takes an index.

Sounds good.

Quoted 22 lines

Quoted 22 lines

drivers/pinctrl/pinctrl-uclass.c new file +22 -0

+{
+       struct pinctrl_state *state;
+       char *end;
+       int state_index;
+       int ret;
+       bool search_by_index = false;
+
+       if (!dev)
+               return -EINVAL;
+
+       state_index = simple_strtoul(state_name, &end, 10);
+       if (*end == 0) {
+               search_by_index = true;
+       }
+
+       list_for_each_entry(state, &dev->pinctrl_states, node) {
+               if (search_by_index) {
+                       if (state->index != state_index)
+                               continue;
+               } else {
+                       if (strcmp(state->name, state_name))
+                               continue;

Quoted 8 lines

drivers/pinctrl/pinctrl-uclass.c line 314

Quoted 6 lines

drivers/pinctrl/pinctrl-uclass.c new file +6 -0

+               }
+
+               return pinctrl_select_state(state);
+       }
+
+#ifdef CONFIG_OF_CONTROL

I think we can assumed this is defined.

OK, will move it to Kconfig's "depends on".

Quoted 5 lines

drivers/pinctrl/pinctrl-uclass.c line 317

Quoted 3 lines

drivers/pinctrl/pinctrl-uclass.c new file +3 -0

+       /*
+        * if the state was not found in the linked list,
+        * search the device tree

Why not populate the table at start-up?

To delay population, so that only the necessary pinctrl state is parsed.

It is possible to have multiple states like this:

pinctrl-names = "default", "state-foo", "state-bar";

At the probing stage, every device is put into "default" state.

At this moment, "state-foo", "state-bar" should not be populated as they might never be used.

When pinctrl_set_state(dev, "state-foo") is called, it is parsed.

Quoted 71 lines

Quoted 71 lines

drivers/pinctrl/pinctrl-uclass.c new file +36 -0

+        */
+       if (search_by_index)
+               ret = pinctrl_dt_to_state(dev, NULL, state_index, &state);
+       else
+               ret = pinctrl_dt_to_state(dev, state_name, -1, &state);
+
+       if (ret == 0) {
+               list_add_tail(&state->node, &dev->pinctrl_states);
+               return pinctrl_select_state(state);
+       }
+#endif
+
+       /*
+        * TODO: Masahiro Yamada
+        * As not all the boards support Device Tree,
+        * add board file support here.
+        */
+
+       /* requested pin-state not found, ignore it */
+       return 0;
+}
+
+int pinctrl_init(struct udevice *dev)
+{
+       if (!dev)
+               return -EINVAL;
+
+       INIT_LIST_HEAD(&dev->pinctrl_states);
+
+       return pinctrl_set_state(dev, "default");
+}
+
+UCLASS_DRIVER(pinctrl) = {
+       .id = UCLASS_PINCTRL,
+       .name = "pinctrl",
+};

include/dm/device.h +3 -0

index 18296bb..05d4e40 100644
@@ -93,6 +93,9 @@ struct udevice {
        uint32_t flags;
        int req_seq;
        int seq;
+#ifdef CONFIG_PINCTRL
+       struct list_head pinctrl_states;
+#endif
 };
 
 /* Maximum sequence number supported */

include/dm/pinctrl.h new file +13 -0

new file mode 100644
index 0000000..95925ca
@@ -0,0 +1,28 @@
+#ifndef __PINCTRL_H
+#define __PINCTRL_H
+
+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);
+};
+
+#if (!defined(CONFIG_SPL_BUILD) && defined(CONFIG_PINCTRL)) ||         \
+               (defined(CONFIG_SPL_BUILD) && defined(CONFIG_SPL_PINCTRL))
+int pinctrl_set_state(struct udevice *dev, const char *state_name);

Quoted 8 lines

include/dm/pinctrl.h line 19

Quoted 6 lines

include/dm/pinctrl.h new file +6 -0

+int pinctrl_init(struct udevice *dev);
+#else
+static inline int pinctrl_set_state(struct udevice *dev,
+                                   const char *state_name)
+{
+       return 0;

return -ENOSYS?

device_probe() fails.

Quoted 14 lines

Quoted 14 lines

include/dm/pinctrl.h new file +9 -0

+}
+
+static inline int pinctrl_init(struct udevice *dev)
+{
+       return 0;
+}
+#endif
+
+#endif /* __PINCTRL_H */

include/dm/uclass-id.h +0 -1

index c7310d7..546f7fb 100644
--- a/include/dm/uclass-id.h

Quoted 9 lines

include/dm/uclass-id.h enum uclass_id { · line 42

Quoted 6 lines

+++ b/include/dm/uclass-id.h

include/dm/uclass-id.h +1 -0

@@ -39,6 +39,7 @@ enum uclass_id {
        UCLASS_PCH,             /* x86 platform controller hub */
        UCLASS_PCI,             /* PCI bus */
        UCLASS_PCI_GENERIC,     /* Generic PCI bus device */
+       UCLASS_PINCTRL,         /* Pinctrl device */

That's not a very useful comment!

Why not?