* [U-Boot] [RFC PATCH 1/7] pinctrl: add pinctrl framework
2015-07-15 8:16 [U-Boot] [RFC PATCH 0/7] DM: pinctrl: Another implemtation of pinctrl framework Masahiro Yamada
@ 2015-07-15 8:16 ` Masahiro Yamada
2015-07-15 8:29 ` Masahiro Yamada
` (3 more replies)
2015-07-15 8:16 ` [U-Boot] [RFC PATCH 2/7] pinctrl: UniPhier: add UniPhier pinctrl core support Masahiro Yamada
` (5 subsequent siblings)
6 siblings, 4 replies; 18+ messages in thread
From: Masahiro Yamada @ 2015-07-15 8:16 UTC (permalink / raw)
To: u-boot
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 <yamada.masahiro@socionext.com>
---
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
diff --git a/drivers/Kconfig b/drivers/Kconfig
index c7e526c..c696036 100644
--- a/drivers/Kconfig
+++ b/drivers/Kconfig
@@ -28,6 +28,8 @@ source "drivers/i2c/Kconfig"
source "drivers/spi/Kconfig"
+source "drivers/pinctrl/Kconfig"
+
source "drivers/gpio/Kconfig"
source "drivers/power/Kconfig"
diff --git a/drivers/Makefile b/drivers/Makefile
index 280b6d1..7e5ae84 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -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/
diff --git a/drivers/core/device.c b/drivers/core/device.c
index 46bf388..4e94a4f 100644
--- a/drivers/core/device.c
+++ b/drivers/core/device.c
@@ -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;
+ ret = pinctrl_init(dev);
+ if (ret)
+ goto fail;
+
ret = uclass_pre_probe_device(dev);
if (ret)
goto fail;
diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
new file mode 100644
index 0000000..cae4d55
--- /dev/null
+++ b/drivers/pinctrl/Kconfig
@@ -0,0 +1,2 @@
+config PINCTRL
+ bool
diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
new file mode 100644
index 0000000..a775a4c
--- /dev/null
+++ b/drivers/pinctrl/Makefile
@@ -0,0 +1 @@
+obj-y += pinctrl-uclass.o
diff --git a/drivers/pinctrl/pinctrl-uclass.c b/drivers/pinctrl/pinctrl-uclass.c
new file mode 100644
index 0000000..68e4842
--- /dev/null
+++ b/drivers/pinctrl/pinctrl-uclass.c
@@ -0,0 +1,353 @@
+/*
+ * Copyright (C) 2015 Masahiro Yamada <yamada.masahiro@socionext.com>
+ *
+ * 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 {
+ struct list_head node;
+ const char *name;
+ int index;
+ struct list_head settings;
+};
+
+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)
+{
+ 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);
+ 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);
+ if (!value)
+ return -EINVAL;
+
+ if (!strcmp(name, "pins") || !strcmp(name, "groups") ||
+ !strcmp(name, "phandle") || !strcmp(name, "linux,phandle"))
+ continue;
+
+ 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;
+
+ pctldev_node = config_node;
+ for (;;) {
+ pctldev_node = fdt_parent_offset(fdt, pctldev_node);
+ 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;
+
+ list_for_each_entry_safe(setting, n, &state->settings, node)
+ free(setting);
+
+ free(state);
+ }
+
+ *pstate = state;
+
+ return ret;
+}
+#endif
+
+int pinctrl_set_state(struct udevice *dev, const char *state_name)
+{
+ 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;
+ }
+
+ return pinctrl_select_state(state);
+ }
+
+#ifdef CONFIG_OF_CONTROL
+ /*
+ * if the state was not found in the linked list,
+ * search the device tree
+ */
+ 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",
+};
diff --git a/include/dm/device.h b/include/dm/device.h
index 18296bb..05d4e40 100644
--- a/include/dm/device.h
+++ b/include/dm/device.h
@@ -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 */
diff --git a/include/dm/pinctrl.h b/include/dm/pinctrl.h
new file mode 100644
index 0000000..95925ca
--- /dev/null
+++ b/include/dm/pinctrl.h
@@ -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);
+int pinctrl_init(struct udevice *dev);
+#else
+static inline int pinctrl_set_state(struct udevice *dev,
+ const char *state_name)
+{
+ return 0;
+}
+
+static inline int pinctrl_init(struct udevice *dev)
+{
+ return 0;
+}
+#endif
+
+#endif /* __PINCTRL_H */
diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
index c7310d7..546f7fb 100644
--- a/include/dm/uclass-id.h
+++ b/include/dm/uclass-id.h
@@ -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 */
UCLASS_PMIC, /* PMIC I/O device */
UCLASS_REGULATOR, /* Regulator device */
UCLASS_RTC, /* Real time clock device */
--
1.9.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* [U-Boot] [RFC PATCH 1/7] pinctrl: add pinctrl framework
2015-07-15 8:16 ` [U-Boot] [RFC PATCH 1/7] pinctrl: add " Masahiro Yamada
@ 2015-07-15 8:29 ` Masahiro Yamada
2015-07-15 8:53 ` Albert ARIBAUD
` (2 subsequent siblings)
3 siblings, 0 replies; 18+ messages in thread
From: Masahiro Yamada @ 2015-07-15 8:29 UTC (permalink / raw)
To: u-boot
Memory footprint analysis:
This uclass support increased 2.5KB on my board (ARM).
Additional 1-2KB would be necessary to implement your
low-level driver.
^ permalink raw reply [flat|nested] 18+ messages in thread* [U-Boot] [RFC PATCH 1/7] pinctrl: add pinctrl framework
2015-07-15 8:16 ` [U-Boot] [RFC PATCH 1/7] pinctrl: add " Masahiro Yamada
2015-07-15 8:29 ` Masahiro Yamada
@ 2015-07-15 8:53 ` Albert ARIBAUD
2015-07-15 9:02 ` Masahiro Yamada
2015-07-18 14:37 ` Simon Glass
2015-07-30 2:06 ` Simon Glass
3 siblings, 1 reply; 18+ messages in thread
From: Albert ARIBAUD @ 2015-07-15 8:53 UTC (permalink / raw)
To: u-boot
Hello Masahiro,
On Wed, 15 Jul 2015 17:16:16 +0900, Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
> Now, a simple pinctrl patch is being proposed by Simon.
> http://patchwork.ozlabs.org/patch/487801/
>
> In the design above, as you see, the uclass is just like a wrapper layer
> to invoke .request and .get_periph_id of low-level drivers.
> In other words, it is Do-It-Yourself thing, so it is up to you how to identify
> which peripheral is being handled in your .get_periph_id().
>
> And here is one example for how a low-level pinctrl driver could be implemented.
> http://patchwork.ozlabs.org/patch/487874/
>
> As you see in the thread, honestly, I do not like this approach.
>
> It is true that you can implement .get_periph_id in your driver
> better than parsing "interrupts" properties, but I guess
> many drivers would follow the rockchip implmentation because
> no helpful infrastructure is provided by the uclass (at least now).
>
> Device trees describe hardwares in a way independent of software
> that they are used with. So, identical device trees can be (should be)
> used with U-Boot as well as Linux or whatever.
>
> Thus, I want the pinctrl can be controllable by device trees in the same way
> of Linux, that is, by parsing "pinctrl-names" and "pinctrl-<N>" properties.
>
> Of course, it would be possible to do it in my own .get_periph_id,
> but "pinctrl-names" and "pinctrl-<N>" are too generic to be done in each
> low-level driver.
>
> In this series, I'd like to propose to support it in the uclass, so that
> we can easily reuse device trees for pinctrl.
> Please put it on the table for discussion.
>
> Let me explain how it works.
>
> The basic idea is pretty much like Linux, but it has been much simplified
> because full-support of the Linux's pinctrl is too much a burden for a boot-loader.
>
> Device Tree
> -----------
>
> To use pinctrl from each peripheral, add some properties in the device node.
>
> "pinctrl-names" is a list of pin states. The "default" state is mandatory,
> and it would probably be enough for U-Boot. But, in order to show how it works,
> say the example device supports two states: "default" and "sleep".
> In this case, the properties should be like this.
>
> pinctrl-names = "default", "sleep";
>
> And then, add as many "pinctrl-<N>" properties as the number of states.
>
> pinctrl-0 = <phandle to default config node>;
> pinctrl-1 = <phandle to sleep config node>;
>
> Here, pinctrl-0, pinctrl-1 corresponds to "default", "sleep", respectively.
>
> The config nodes are (direct or indirect) children of a pinctrl device.
>
> To sum up, the device tree would be like this:
>
> foo {
> compatible = "...";
> reg = <...>;
> pinctrl-names = "default", "sleep";
> pinctrl-0 = <&foo_default_pin>;
> pinctrl-1 = <&foo_sleep_pin>;
> ...
> };
>
> pinctrl {
> compatible = "...";
> reg = <...>;
> foo_default_pin: foo_default {
> groups = "...";
> functions = "...";
> };
> foo_sleep_pin: foo_sleep {
> groups = "...";
> functions = "...";
> };
> };
>
> API
> ---
>
>
> To set a device into a particular pin state, call
> int pinctrl_set_state(struct udevice *dev, const char *state_name).
>
> For example, if you want to set the foo device into the sleep state,
> you can do like this:
>
> struct udevice *foo_dev;
>
> (device_get or whatever)
>
> pinctrl_set_state(foo_dev, "sleep");
>
> When each device is probed, pinctrl_init() is invoked,
> which initializes some pinctrl-specific parameters and set it into "default"
> pin state. Because it is automatically done by the core of driver model,
> when a device is probed, its pins are in the "default" state.
>
> Implementation of low-level driver
> ----------------------------------
>
> Currently, two methods are supported in the pinctrl operation:
> struct pinctrl_ops {
> int (*pinmux_set) (struct udevice *dev, const char *group,
> const char *function);
> int (*pinconf_set) (struct udevice *dev, const char *group,
> const char *conf_param, unsigned conf_arg);
> };
>
> They are used to change pin-mux, pin-conf, respectively.
>
> If the pin-config node for the target pin-state is like this,
> i2c_default_pin: i2c_default {
> groups = "i2c-0a";
> functions = "i2c-0";
> };
>
> Your pinmux_set() is called with "i2c-0a" for the group and "i2c-0"
> for the function.
>
> It is totally up to you what you do for each group & function.
>
> Difference from Linux pinctrl (limitation)
> -----------------------------------------
>
> As you can see in pinctrl drivers in Linux (drivers/pinctrl/*),
> the drivers usually contain huge tables for pins, groups, functions,...
> But I do not want to force that for U-Boot.
>
> In Linux, "group" represents a group of pins although a group sometimes
> consists of a signle pin (like GPIO). Pin-muxing is available only against
> groups, while pin-conf is supported for both pins and groups.
>
> In contrast, in U-Boot, "pins" and "groups" are handled exactly in the same way,
> so you can use either to specify the target of pin-mux or pin-conf.
>
> Pin/group tables are not required for U-boot pinctrl drivers, so
> we never know which pins belong to which group, function, that is,
> we can not avoid conflicts on pins.
>
> For example, let's say some pins are shared between UART0 and I2C0.
> In this case, Linux pinctrl does not allow to use them simultaneously,
> while U-boot pinctrl does not (can not) care about it.
>
> If you want to use multiple functions that share the same pins,
> you must make sure pin-muxing is correctly set with pinctrl_set_state().
>
> TODO
> ----
>
> [1] Pinctrl drivers are usually used with device trees (or ACPI), but
> not all the boards in U-boot support device tree.
> I will add board file support (plat data) later.
> (we will need some function to register pin settings array)
>
> [2] SPL support is not completed. Tweak Kconfig and Makefile a little.
> This should be easy.
>
> [3] Currently, properties other than "function" are assumed
> as pin-conf parameters. Perhaps, should we filter out
> unsupported properties? A table for supported properties
> such "bias-pull-up", "driver-strength", etc. ?
>
> [4] For "function", "groups" should be able to be omitted.
>
> Comments are welcome.
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).
Amicalement,
--
Albert.
^ permalink raw reply [flat|nested] 18+ messages in thread* [U-Boot] [RFC PATCH 1/7] pinctrl: add pinctrl framework
2015-07-15 8:53 ` Albert ARIBAUD
@ 2015-07-15 9:02 ` Masahiro Yamada
2015-07-15 9:08 ` Masahiro Yamada
0 siblings, 1 reply; 18+ messages in thread
From: Masahiro Yamada @ 2015-07-15 9:02 UTC (permalink / raw)
To: u-boot
2015-07-15 17:53 GMT+09:00 Albert ARIBAUD <albert.u.boot@aribaud.net>:
> Hello Masahiro,
>
> On Wed, 15 Jul 2015 17:16:16 +0900, Masahiro Yamada
> <yamada.masahiro@socionext.com> wrote:
>
>> Now, a simple pinctrl patch is being proposed by Simon.
>> http://patchwork.ozlabs.org/patch/487801/
>>
>> In the design above, as you see, the uclass is just like a wrapper layer
>> to invoke .request and .get_periph_id of low-level drivers.
>> In other words, it is Do-It-Yourself thing, so it is up to you how to identify
>> which peripheral is being handled in your .get_periph_id().
>>
>> And here is one example for how a low-level pinctrl driver could be implemented.
>> http://patchwork.ozlabs.org/patch/487874/
>>
>> As you see in the thread, honestly, I do not like this approach.
>>
>> It is true that you can implement .get_periph_id in your driver
>> better than parsing "interrupts" properties, but I guess
>> many drivers would follow the rockchip implmentation because
>> no helpful infrastructure is provided by the uclass (at least now).
>>
>> Device trees describe hardwares in a way independent of software
>> that they are used with. So, identical device trees can be (should be)
>> used with U-Boot as well as Linux or whatever.
>>
>> Thus, I want the pinctrl can be controllable by device trees in the same way
>> of Linux, that is, by parsing "pinctrl-names" and "pinctrl-<N>" properties.
>>
>> Of course, it would be possible to do it in my own .get_periph_id,
>> but "pinctrl-names" and "pinctrl-<N>" are too generic to be done in each
>> low-level driver.
>>
>> In this series, I'd like to propose to support it in the uclass, so that
>> we can easily reuse device trees for pinctrl.
>> Please put it on the table for discussion.
>>
>> Let me explain how it works.
>>
>> The basic idea is pretty much like Linux, but it has been much simplified
>> because full-support of the Linux's pinctrl is too much a burden for a boot-loader.
>>
>> Device Tree
>> -----------
>>
>> To use pinctrl from each peripheral, add some properties in the device node.
>>
>> "pinctrl-names" is a list of pin states. The "default" state is mandatory,
>> and it would probably be enough for U-Boot. But, in order to show how it works,
>> say the example device supports two states: "default" and "sleep".
>> In this case, the properties should be like this.
>>
>> pinctrl-names = "default", "sleep";
>>
>> And then, add as many "pinctrl-<N>" properties as the number of states.
>>
>> pinctrl-0 = <phandle to default config node>;
>> pinctrl-1 = <phandle to sleep config node>;
>>
>> Here, pinctrl-0, pinctrl-1 corresponds to "default", "sleep", respectively.
>>
>> The config nodes are (direct or indirect) children of a pinctrl device.
>>
>> To sum up, the device tree would be like this:
>>
>> foo {
>> compatible = "...";
>> reg = <...>;
>> pinctrl-names = "default", "sleep";
>> pinctrl-0 = <&foo_default_pin>;
>> pinctrl-1 = <&foo_sleep_pin>;
>> ...
>> };
>>
>> pinctrl {
>> compatible = "...";
>> reg = <...>;
>> foo_default_pin: foo_default {
>> groups = "...";
>> functions = "...";
>> };
>> foo_sleep_pin: foo_sleep {
>> groups = "...";
>> functions = "...";
>> };
>> };
>>
>> API
>> ---
>>
>>
>> To set a device into a particular pin state, call
>> int pinctrl_set_state(struct udevice *dev, const char *state_name).
>>
>> For example, if you want to set the foo device into the sleep state,
>> you can do like this:
>>
>> struct udevice *foo_dev;
>>
>> (device_get or whatever)
>>
>> pinctrl_set_state(foo_dev, "sleep");
>>
>> When each device is probed, pinctrl_init() is invoked,
>> which initializes some pinctrl-specific parameters and set it into "default"
>> pin state. Because it is automatically done by the core of driver model,
>> when a device is probed, its pins are in the "default" state.
>>
>> Implementation of low-level driver
>> ----------------------------------
>>
>> Currently, two methods are supported in the pinctrl operation:
>> struct pinctrl_ops {
>> int (*pinmux_set) (struct udevice *dev, const char *group,
>> const char *function);
>> int (*pinconf_set) (struct udevice *dev, const char *group,
>> const char *conf_param, unsigned conf_arg);
>> };
>>
>> They are used to change pin-mux, pin-conf, respectively.
>>
>> If the pin-config node for the target pin-state is like this,
>> i2c_default_pin: i2c_default {
>> groups = "i2c-0a";
>> functions = "i2c-0";
>> };
>>
>> Your pinmux_set() is called with "i2c-0a" for the group and "i2c-0"
>> for the function.
>>
>> It is totally up to you what you do for each group & function.
>>
>> Difference from Linux pinctrl (limitation)
>> -----------------------------------------
>>
>> As you can see in pinctrl drivers in Linux (drivers/pinctrl/*),
>> the drivers usually contain huge tables for pins, groups, functions,...
>> But I do not want to force that for U-Boot.
>>
>> In Linux, "group" represents a group of pins although a group sometimes
>> consists of a signle pin (like GPIO). Pin-muxing is available only against
>> groups, while pin-conf is supported for both pins and groups.
>>
>> In contrast, in U-Boot, "pins" and "groups" are handled exactly in the same way,
>> so you can use either to specify the target of pin-mux or pin-conf.
>>
>> Pin/group tables are not required for U-boot pinctrl drivers, so
>> we never know which pins belong to which group, function, that is,
>> we can not avoid conflicts on pins.
>>
>> For example, let's say some pins are shared between UART0 and I2C0.
>> In this case, Linux pinctrl does not allow to use them simultaneously,
>> while U-boot pinctrl does not (can not) care about it.
>>
>> If you want to use multiple functions that share the same pins,
>> you must make sure pin-muxing is correctly set with pinctrl_set_state().
>>
>> TODO
>> ----
>>
>> [1] Pinctrl drivers are usually used with device trees (or ACPI), but
>> not all the boards in U-boot support device tree.
>> I will add board file support (plat data) later.
>> (we will need some function to register pin settings array)
>>
>> [2] SPL support is not completed. Tweak Kconfig and Makefile a little.
>> This should be easy.
>>
>> [3] Currently, properties other than "function" are assumed
>> as pin-conf parameters. Perhaps, should we filter out
>> unsupported properties? A table for supported properties
>> such "bias-pull-up", "driver-strength", etc. ?
>>
>> [4] For "function", "groups" should be able to be omitted.
>>
>> Comments are welcome.
>
> 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).
No worry.
This is a RFC and it will never be applied as is.
Of course, I will tidy up description into more summarized form
and consider adding detailed info in README
if I get positive options for this series.
Anyway, let's see how it goes.
--
Best Regards
Masahiro Yamada
^ permalink raw reply [flat|nested] 18+ messages in thread
* [U-Boot] [RFC PATCH 1/7] pinctrl: add pinctrl framework
2015-07-15 8:16 ` [U-Boot] [RFC PATCH 1/7] pinctrl: add " Masahiro Yamada
2015-07-15 8:29 ` Masahiro Yamada
2015-07-15 8:53 ` Albert ARIBAUD
@ 2015-07-18 14:37 ` Simon Glass
2015-07-22 14:24 ` Simon Glass
2015-07-30 2:06 ` Simon Glass
3 siblings, 1 reply; 18+ messages in thread
From: Simon Glass @ 2015-07-18 14:37 UTC (permalink / raw)
To: u-boot
Hi Masahiro,
On 15 July 2015 at 02:16, Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> Now, a simple pinctrl patch is being proposed by Simon.
> http://patchwork.ozlabs.org/patch/487801/
>
> In the design above, as you see, the uclass is just like a wrapper layer
> to invoke .request and .get_periph_id of low-level drivers.
> In other words, it is Do-It-Yourself thing, so it is up to you how to identify
> which peripheral is being handled in your .get_periph_id().
>
> And here is one example for how a low-level pinctrl driver could be implemented.
> http://patchwork.ozlabs.org/patch/487874/
>
> As you see in the thread, honestly, I do not like this approach.
>
> It is true that you can implement .get_periph_id in your driver
> better than parsing "interrupts" properties, but I guess
> many drivers would follow the rockchip implmentation because
> no helpful infrastructure is provided by the uclass (at least now).
>
> Device trees describe hardwares in a way independent of software
> that they are used with. So, identical device trees can be (should be)
> used with U-Boot as well as Linux or whatever.
>
> Thus, I want the pinctrl can be controllable by device trees in the same way
> of Linux, that is, by parsing "pinctrl-names" and "pinctrl-<N>" properties.
>
> Of course, it would be possible to do it in my own .get_periph_id,
> but "pinctrl-names" and "pinctrl-<N>" are too generic to be done in each
> low-level driver.
>
> In this series, I'd like to propose to support it in the uclass, so that
> we can easily reuse device trees for pinctrl.
> Please put it on the table for discussion.
>
> Let me explain how it works.
>
> The basic idea is pretty much like Linux, but it has been much simplified
> because full-support of the Linux's pinctrl is too much a burden for a boot-loader.
>
> Device Tree
> -----------
>
> To use pinctrl from each peripheral, add some properties in the device node.
>
> "pinctrl-names" is a list of pin states. The "default" state is mandatory,
> and it would probably be enough for U-Boot. But, in order to show how it works,
> say the example device supports two states: "default" and "sleep".
> In this case, the properties should be like this.
>
> pinctrl-names = "default", "sleep";
>
> And then, add as many "pinctrl-<N>" properties as the number of states.
>
> pinctrl-0 = <phandle to default config node>;
> pinctrl-1 = <phandle to sleep config node>;
>
> Here, pinctrl-0, pinctrl-1 corresponds to "default", "sleep", respectively.
>
> The config nodes are (direct or indirect) children of a pinctrl device.
>
> To sum up, the device tree would be like this:
>
> foo {
> compatible = "...";
> reg = <...>;
> pinctrl-names = "default", "sleep";
> pinctrl-0 = <&foo_default_pin>;
> pinctrl-1 = <&foo_sleep_pin>;
> ...
> };
>
> pinctrl {
> compatible = "...";
> reg = <...>;
> foo_default_pin: foo_default {
> groups = "...";
> functions = "...";
> };
> foo_sleep_pin: foo_sleep {
> groups = "...";
> functions = "...";
> };
> };
>
> API
> ---
>
>
> To set a device into a particular pin state, call
> int pinctrl_set_state(struct udevice *dev, const char *state_name).
>
> For example, if you want to set the foo device into the sleep state,
> you can do like this:
>
> struct udevice *foo_dev;
>
> (device_get or whatever)
>
> pinctrl_set_state(foo_dev, "sleep");
>
> When each device is probed, pinctrl_init() is invoked,
> which initializes some pinctrl-specific parameters and set it into "default"
> pin state. Because it is automatically done by the core of driver model,
> when a device is probed, its pins are in the "default" state.
>
> Implementation of low-level driver
> ----------------------------------
>
> Currently, two methods are supported in the pinctrl operation:
> struct pinctrl_ops {
> int (*pinmux_set) (struct udevice *dev, const char *group,
> const char *function);
> int (*pinconf_set) (struct udevice *dev, const char *group,
> const char *conf_param, unsigned conf_arg);
> };
>
> They are used to change pin-mux, pin-conf, respectively.
>
> If the pin-config node for the target pin-state is like this,
> i2c_default_pin: i2c_default {
> groups = "i2c-0a";
> functions = "i2c-0";
> };
>
> Your pinmux_set() is called with "i2c-0a" for the group and "i2c-0"
> for the function.
>
> It is totally up to you what you do for each group & function.
>
> Difference from Linux pinctrl (limitation)
> -----------------------------------------
>
> As you can see in pinctrl drivers in Linux (drivers/pinctrl/*),
> the drivers usually contain huge tables for pins, groups, functions,...
> But I do not want to force that for U-Boot.
>
> In Linux, "group" represents a group of pins although a group sometimes
> consists of a signle pin (like GPIO). Pin-muxing is available only against
> groups, while pin-conf is supported for both pins and groups.
>
> In contrast, in U-Boot, "pins" and "groups" are handled exactly in the same way,
> so you can use either to specify the target of pin-mux or pin-conf.
>
> Pin/group tables are not required for U-boot pinctrl drivers, so
> we never know which pins belong to which group, function, that is,
> we can not avoid conflicts on pins.
>
> For example, let's say some pins are shared between UART0 and I2C0.
> In this case, Linux pinctrl does not allow to use them simultaneously,
> while U-boot pinctrl does not (can not) care about it.
>
> If you want to use multiple functions that share the same pins,
> you must make sure pin-muxing is correctly set with pinctrl_set_state().
>
> TODO
> ----
>
> [1] Pinctrl drivers are usually used with device trees (or ACPI), but
> not all the boards in U-boot support device tree.
> I will add board file support (plat data) later.
> (we will need some function to register pin settings array)
>
> [2] SPL support is not completed. Tweak Kconfig and Makefile a little.
> This should be easy.
>
> [3] Currently, properties other than "function" are assumed
> as pin-conf parameters. Perhaps, should we filter out
> unsupported properties? A table for supported properties
> such "bias-pull-up", "driver-strength", etc. ?
>
> [4] For "function", "groups" should be able to be omitted.
>
> Comments are welcome.
(long pause for thought)
I would certainly like to have this in U-Boot. I think for most uses
it is the right thing to do. However I'm not sure it is sufficient.
For SPL we are limited on code size / space. In that case we often
need to do a little pinmuxing but we cannot always afford the space of
this implementation. The device tree for pinmux is pretty large for
some SoCs quite apart from the extra code. I found this recently with
Rockchip which has a 32KB limit.
The nice thing about my simple implementation is that it can be used
from SPL efficiently. For example:
static void soc_spi_pinmux(enum periph_id id, int flags)
{
switch (id) {
case PERIPH_ID_SPI0:
writel(...)
break;
case PERIPH_ID_SPI1:
writel(...)
break;
}
}
static void soc_i2c_pinmux(enum periph_id id, int flags)
{
case PERIPH_ID_I2C0:
writel(...)
break;
case PERIPH_ID_I2C1:
writel(...)
break;
}
}
static void soc_set_pinmux(enum_periph_id id, int flags)
{
switch (id) {
#ifdef CONFIG_SPI
case PERIPH_ID_SPI0:
case PERIPH_ID_SPI0:
soc_i2c_pinmux(id, flags);
break;
#endif
#ifdef CONFIG_SPI
case PERIPH_ID_I2C0:
case PERIPH_ID_I2C0:
soc_i2c_pinmux(id, flags);
break;
#endif
}
In SPL we can do something like:
soc_i2c_pinmux(PERIPH_ID_I2C0, 0);
to set up I2C port 0 with default pin settings. This is about as
efficient as you can get.
With your full implementation we have no such option.
So I think your implementation makes more sense in general, but mine
is better for SPL.
Can we combine the two? What do you think about allowing the uclass to
provide both interfaces? You can then use the full pinctl one, but
implement a simple one also. This could be achieved simply by adding
two more methods to the uclass.
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
^ permalink raw reply [flat|nested] 18+ messages in thread* [U-Boot] [RFC PATCH 1/7] pinctrl: add pinctrl framework
2015-07-18 14:37 ` Simon Glass
@ 2015-07-22 14:24 ` Simon Glass
2015-07-23 4:06 ` Masahiro Yamada
0 siblings, 1 reply; 18+ messages in thread
From: Simon Glass @ 2015-07-22 14:24 UTC (permalink / raw)
To: u-boot
Hi Masahiro,
On 18 July 2015 at 08:37, Simon Glass <sjg@chromium.org> wrote:
> Hi Masahiro,
>
> On 15 July 2015 at 02:16, Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>> Now, a simple pinctrl patch is being proposed by Simon.
>> http://patchwork.ozlabs.org/patch/487801/
>>
>> In the design above, as you see, the uclass is just like a wrapper layer
>> to invoke .request and .get_periph_id of low-level drivers.
>> In other words, it is Do-It-Yourself thing, so it is up to you how to identify
>> which peripheral is being handled in your .get_periph_id().
>>
>> And here is one example for how a low-level pinctrl driver could be implemented.
>> http://patchwork.ozlabs.org/patch/487874/
>>
>> As you see in the thread, honestly, I do not like this approach.
>>
>> It is true that you can implement .get_periph_id in your driver
>> better than parsing "interrupts" properties, but I guess
>> many drivers would follow the rockchip implmentation because
>> no helpful infrastructure is provided by the uclass (at least now).
>>
>> Device trees describe hardwares in a way independent of software
>> that they are used with. So, identical device trees can be (should be)
>> used with U-Boot as well as Linux or whatever.
>>
>> Thus, I want the pinctrl can be controllable by device trees in the same way
>> of Linux, that is, by parsing "pinctrl-names" and "pinctrl-<N>" properties.
>>
>> Of course, it would be possible to do it in my own .get_periph_id,
>> but "pinctrl-names" and "pinctrl-<N>" are too generic to be done in each
>> low-level driver.
>>
>> In this series, I'd like to propose to support it in the uclass, so that
>> we can easily reuse device trees for pinctrl.
>> Please put it on the table for discussion.
>>
>> Let me explain how it works.
>>
>> The basic idea is pretty much like Linux, but it has been much simplified
>> because full-support of the Linux's pinctrl is too much a burden for a boot-loader.
>>
>> Device Tree
>> -----------
>>
>> To use pinctrl from each peripheral, add some properties in the device node.
>>
>> "pinctrl-names" is a list of pin states. The "default" state is mandatory,
>> and it would probably be enough for U-Boot. But, in order to show how it works,
>> say the example device supports two states: "default" and "sleep".
>> In this case, the properties should be like this.
>>
>> pinctrl-names = "default", "sleep";
>>
>> And then, add as many "pinctrl-<N>" properties as the number of states.
>>
>> pinctrl-0 = <phandle to default config node>;
>> pinctrl-1 = <phandle to sleep config node>;
>>
>> Here, pinctrl-0, pinctrl-1 corresponds to "default", "sleep", respectively.
>>
>> The config nodes are (direct or indirect) children of a pinctrl device.
>>
>> To sum up, the device tree would be like this:
>>
>> foo {
>> compatible = "...";
>> reg = <...>;
>> pinctrl-names = "default", "sleep";
>> pinctrl-0 = <&foo_default_pin>;
>> pinctrl-1 = <&foo_sleep_pin>;
>> ...
>> };
>>
>> pinctrl {
>> compatible = "...";
>> reg = <...>;
>> foo_default_pin: foo_default {
>> groups = "...";
>> functions = "...";
>> };
>> foo_sleep_pin: foo_sleep {
>> groups = "...";
>> functions = "...";
>> };
>> };
>>
>> API
>> ---
>>
>>
>> To set a device into a particular pin state, call
>> int pinctrl_set_state(struct udevice *dev, const char *state_name).
>>
>> For example, if you want to set the foo device into the sleep state,
>> you can do like this:
>>
>> struct udevice *foo_dev;
>>
>> (device_get or whatever)
>>
>> pinctrl_set_state(foo_dev, "sleep");
>>
>> When each device is probed, pinctrl_init() is invoked,
>> which initializes some pinctrl-specific parameters and set it into "default"
>> pin state. Because it is automatically done by the core of driver model,
>> when a device is probed, its pins are in the "default" state.
>>
>> Implementation of low-level driver
>> ----------------------------------
>>
>> Currently, two methods are supported in the pinctrl operation:
>> struct pinctrl_ops {
>> int (*pinmux_set) (struct udevice *dev, const char *group,
>> const char *function);
>> int (*pinconf_set) (struct udevice *dev, const char *group,
>> const char *conf_param, unsigned conf_arg);
>> };
>>
>> They are used to change pin-mux, pin-conf, respectively.
>>
>> If the pin-config node for the target pin-state is like this,
>> i2c_default_pin: i2c_default {
>> groups = "i2c-0a";
>> functions = "i2c-0";
>> };
>>
>> Your pinmux_set() is called with "i2c-0a" for the group and "i2c-0"
>> for the function.
>>
>> It is totally up to you what you do for each group & function.
>>
>> Difference from Linux pinctrl (limitation)
>> -----------------------------------------
>>
>> As you can see in pinctrl drivers in Linux (drivers/pinctrl/*),
>> the drivers usually contain huge tables for pins, groups, functions,...
>> But I do not want to force that for U-Boot.
>>
>> In Linux, "group" represents a group of pins although a group sometimes
>> consists of a signle pin (like GPIO). Pin-muxing is available only against
>> groups, while pin-conf is supported for both pins and groups.
>>
>> In contrast, in U-Boot, "pins" and "groups" are handled exactly in the same way,
>> so you can use either to specify the target of pin-mux or pin-conf.
>>
>> Pin/group tables are not required for U-boot pinctrl drivers, so
>> we never know which pins belong to which group, function, that is,
>> we can not avoid conflicts on pins.
>>
>> For example, let's say some pins are shared between UART0 and I2C0.
>> In this case, Linux pinctrl does not allow to use them simultaneously,
>> while U-boot pinctrl does not (can not) care about it.
>>
>> If you want to use multiple functions that share the same pins,
>> you must make sure pin-muxing is correctly set with pinctrl_set_state().
>>
>> TODO
>> ----
>>
>> [1] Pinctrl drivers are usually used with device trees (or ACPI), but
>> not all the boards in U-boot support device tree.
>> I will add board file support (plat data) later.
>> (we will need some function to register pin settings array)
>>
>> [2] SPL support is not completed. Tweak Kconfig and Makefile a little.
>> This should be easy.
>>
>> [3] Currently, properties other than "function" are assumed
>> as pin-conf parameters. Perhaps, should we filter out
>> unsupported properties? A table for supported properties
>> such "bias-pull-up", "driver-strength", etc. ?
>>
>> [4] For "function", "groups" should be able to be omitted.
>>
>> Comments are welcome.
>
> (long pause for thought)
>
> I would certainly like to have this in U-Boot. I think for most uses
> it is the right thing to do. However I'm not sure it is sufficient.
>
> For SPL we are limited on code size / space. In that case we often
> need to do a little pinmuxing but we cannot always afford the space of
> this implementation. The device tree for pinmux is pretty large for
> some SoCs quite apart from the extra code. I found this recently with
> Rockchip which has a 32KB limit.
>
> The nice thing about my simple implementation is that it can be used
> from SPL efficiently. For example:
>
> static void soc_spi_pinmux(enum periph_id id, int flags)
> {
> switch (id) {
> case PERIPH_ID_SPI0:
> writel(...)
> break;
> case PERIPH_ID_SPI1:
> writel(...)
> break;
> }
> }
>
> static void soc_i2c_pinmux(enum periph_id id, int flags)
> {
> case PERIPH_ID_I2C0:
> writel(...)
> break;
> case PERIPH_ID_I2C1:
> writel(...)
> break;
> }
> }
>
> static void soc_set_pinmux(enum_periph_id id, int flags)
> {
> switch (id) {
> #ifdef CONFIG_SPI
> case PERIPH_ID_SPI0:
> case PERIPH_ID_SPI0:
> soc_i2c_pinmux(id, flags);
> break;
> #endif
> #ifdef CONFIG_SPI
> case PERIPH_ID_I2C0:
> case PERIPH_ID_I2C0:
> soc_i2c_pinmux(id, flags);
> break;
> #endif
> }
>
> In SPL we can do something like:
>
> soc_i2c_pinmux(PERIPH_ID_I2C0, 0);
>
> to set up I2C port 0 with default pin settings. This is about as
> efficient as you can get.
>
> With your full implementation we have no such option.
>
> So I think your implementation makes more sense in general, but mine
> is better for SPL.
>
> Can we combine the two? What do you think about allowing the uclass to
> provide both interfaces? You can then use the full pinctl one, but
> implement a simple one also. This could be achieved simply by adding
> two more methods to the uclass.
If you are OK with this then I'll have a try at basing my pinctrl
patches on top of your series. I think it can be made to work.
>
> BTW I've been meaning to discuss how to deal with SPL config as it is
> painful in this area, but I'll start a new thread for that.
>
> Regards,
> Simon
Regards,
Simon
^ permalink raw reply [flat|nested] 18+ messages in thread* [U-Boot] [RFC PATCH 1/7] pinctrl: add pinctrl framework
2015-07-22 14:24 ` Simon Glass
@ 2015-07-23 4:06 ` Masahiro Yamada
0 siblings, 0 replies; 18+ messages in thread
From: Masahiro Yamada @ 2015-07-23 4:06 UTC (permalink / raw)
To: u-boot
Hi Simon,
Sorry for being away from this a while.
2015-07-22 23:24 GMT+09:00 Simon Glass <sjg@chromium.org>:
> Hi Masahiro,
>
> On 18 July 2015 at 08:37, Simon Glass <sjg@chromium.org> wrote:
>> Hi Masahiro,
>>
>> On 15 July 2015 at 02:16, Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>>> Now, a simple pinctrl patch is being proposed by Simon.
>>> http://patchwork.ozlabs.org/patch/487801/
>>>
>>> In the design above, as you see, the uclass is just like a wrapper layer
>>> to invoke .request and .get_periph_id of low-level drivers.
>>> In other words, it is Do-It-Yourself thing, so it is up to you how to identify
>>> which peripheral is being handled in your .get_periph_id().
>>>
>>> And here is one example for how a low-level pinctrl driver could be implemented.
>>> http://patchwork.ozlabs.org/patch/487874/
>>>
>>> As you see in the thread, honestly, I do not like this approach.
>>>
>>> It is true that you can implement .get_periph_id in your driver
>>> better than parsing "interrupts" properties, but I guess
>>> many drivers would follow the rockchip implmentation because
>>> no helpful infrastructure is provided by the uclass (at least now).
>>>
>>> Device trees describe hardwares in a way independent of software
>>> that they are used with. So, identical device trees can be (should be)
>>> used with U-Boot as well as Linux or whatever.
>>>
>>> Thus, I want the pinctrl can be controllable by device trees in the same way
>>> of Linux, that is, by parsing "pinctrl-names" and "pinctrl-<N>" properties.
>>>
>>> Of course, it would be possible to do it in my own .get_periph_id,
>>> but "pinctrl-names" and "pinctrl-<N>" are too generic to be done in each
>>> low-level driver.
>>>
>>> In this series, I'd like to propose to support it in the uclass, so that
>>> we can easily reuse device trees for pinctrl.
>>> Please put it on the table for discussion.
>>>
>>> Let me explain how it works.
>>>
>>> The basic idea is pretty much like Linux, but it has been much simplified
>>> because full-support of the Linux's pinctrl is too much a burden for a boot-loader.
>>>
>>> Device Tree
>>> -----------
>>>
>>> To use pinctrl from each peripheral, add some properties in the device node.
>>>
>>> "pinctrl-names" is a list of pin states. The "default" state is mandatory,
>>> and it would probably be enough for U-Boot. But, in order to show how it works,
>>> say the example device supports two states: "default" and "sleep".
>>> In this case, the properties should be like this.
>>>
>>> pinctrl-names = "default", "sleep";
>>>
>>> And then, add as many "pinctrl-<N>" properties as the number of states.
>>>
>>> pinctrl-0 = <phandle to default config node>;
>>> pinctrl-1 = <phandle to sleep config node>;
>>>
>>> Here, pinctrl-0, pinctrl-1 corresponds to "default", "sleep", respectively.
>>>
>>> The config nodes are (direct or indirect) children of a pinctrl device.
>>>
>>> To sum up, the device tree would be like this:
>>>
>>> foo {
>>> compatible = "...";
>>> reg = <...>;
>>> pinctrl-names = "default", "sleep";
>>> pinctrl-0 = <&foo_default_pin>;
>>> pinctrl-1 = <&foo_sleep_pin>;
>>> ...
>>> };
>>>
>>> pinctrl {
>>> compatible = "...";
>>> reg = <...>;
>>> foo_default_pin: foo_default {
>>> groups = "...";
>>> functions = "...";
>>> };
>>> foo_sleep_pin: foo_sleep {
>>> groups = "...";
>>> functions = "...";
>>> };
>>> };
>>>
>>> API
>>> ---
>>>
>>>
>>> To set a device into a particular pin state, call
>>> int pinctrl_set_state(struct udevice *dev, const char *state_name).
>>>
>>> For example, if you want to set the foo device into the sleep state,
>>> you can do like this:
>>>
>>> struct udevice *foo_dev;
>>>
>>> (device_get or whatever)
>>>
>>> pinctrl_set_state(foo_dev, "sleep");
>>>
>>> When each device is probed, pinctrl_init() is invoked,
>>> which initializes some pinctrl-specific parameters and set it into "default"
>>> pin state. Because it is automatically done by the core of driver model,
>>> when a device is probed, its pins are in the "default" state.
>>>
>>> Implementation of low-level driver
>>> ----------------------------------
>>>
>>> Currently, two methods are supported in the pinctrl operation:
>>> struct pinctrl_ops {
>>> int (*pinmux_set) (struct udevice *dev, const char *group,
>>> const char *function);
>>> int (*pinconf_set) (struct udevice *dev, const char *group,
>>> const char *conf_param, unsigned conf_arg);
>>> };
>>>
>>> They are used to change pin-mux, pin-conf, respectively.
>>>
>>> If the pin-config node for the target pin-state is like this,
>>> i2c_default_pin: i2c_default {
>>> groups = "i2c-0a";
>>> functions = "i2c-0";
>>> };
>>>
>>> Your pinmux_set() is called with "i2c-0a" for the group and "i2c-0"
>>> for the function.
>>>
>>> It is totally up to you what you do for each group & function.
>>>
>>> Difference from Linux pinctrl (limitation)
>>> -----------------------------------------
>>>
>>> As you can see in pinctrl drivers in Linux (drivers/pinctrl/*),
>>> the drivers usually contain huge tables for pins, groups, functions,...
>>> But I do not want to force that for U-Boot.
>>>
>>> In Linux, "group" represents a group of pins although a group sometimes
>>> consists of a signle pin (like GPIO). Pin-muxing is available only against
>>> groups, while pin-conf is supported for both pins and groups.
>>>
>>> In contrast, in U-Boot, "pins" and "groups" are handled exactly in the same way,
>>> so you can use either to specify the target of pin-mux or pin-conf.
>>>
>>> Pin/group tables are not required for U-boot pinctrl drivers, so
>>> we never know which pins belong to which group, function, that is,
>>> we can not avoid conflicts on pins.
>>>
>>> For example, let's say some pins are shared between UART0 and I2C0.
>>> In this case, Linux pinctrl does not allow to use them simultaneously,
>>> while U-boot pinctrl does not (can not) care about it.
>>>
>>> If you want to use multiple functions that share the same pins,
>>> you must make sure pin-muxing is correctly set with pinctrl_set_state().
>>>
>>> TODO
>>> ----
>>>
>>> [1] Pinctrl drivers are usually used with device trees (or ACPI), but
>>> not all the boards in U-boot support device tree.
>>> I will add board file support (plat data) later.
>>> (we will need some function to register pin settings array)
>>>
>>> [2] SPL support is not completed. Tweak Kconfig and Makefile a little.
>>> This should be easy.
>>>
>>> [3] Currently, properties other than "function" are assumed
>>> as pin-conf parameters. Perhaps, should we filter out
>>> unsupported properties? A table for supported properties
>>> such "bias-pull-up", "driver-strength", etc. ?
>>>
>>> [4] For "function", "groups" should be able to be omitted.
>>>
>>> Comments are welcome.
>>
>> (long pause for thought)
>>
>> I would certainly like to have this in U-Boot. I think for most uses
>> it is the right thing to do. However I'm not sure it is sufficient.
>>
>> For SPL we are limited on code size / space. In that case we often
>> need to do a little pinmuxing but we cannot always afford the space of
>> this implementation. The device tree for pinmux is pretty large for
>> some SoCs quite apart from the extra code. I found this recently with
>> Rockchip which has a 32KB limit.
>>
>> The nice thing about my simple implementation is that it can be used
>> from SPL efficiently. For example:
>>
>> static void soc_spi_pinmux(enum periph_id id, int flags)
>> {
>> switch (id) {
>> case PERIPH_ID_SPI0:
>> writel(...)
>> break;
>> case PERIPH_ID_SPI1:
>> writel(...)
>> break;
>> }
>> }
>>
>> static void soc_i2c_pinmux(enum periph_id id, int flags)
>> {
>> case PERIPH_ID_I2C0:
>> writel(...)
>> break;
>> case PERIPH_ID_I2C1:
>> writel(...)
>> break;
>> }
>> }
>>
>> static void soc_set_pinmux(enum_periph_id id, int flags)
>> {
>> switch (id) {
>> #ifdef CONFIG_SPI
>> case PERIPH_ID_SPI0:
>> case PERIPH_ID_SPI0:
>> soc_i2c_pinmux(id, flags);
>> break;
>> #endif
>> #ifdef CONFIG_SPI
>> case PERIPH_ID_I2C0:
>> case PERIPH_ID_I2C0:
>> soc_i2c_pinmux(id, flags);
>> break;
>> #endif
>> }
>>
>> In SPL we can do something like:
>>
>> soc_i2c_pinmux(PERIPH_ID_I2C0, 0);
>>
>> to set up I2C port 0 with default pin settings. This is about as
>> efficient as you can get.
>>
>> With your full implementation we have no such option.
>>
>> So I think your implementation makes more sense in general, but mine
>> is better for SPL.
>>
>> Can we combine the two? What do you think about allowing the uclass to
>> provide both interfaces? You can then use the full pinctl one, but
>> implement a simple one also. This could be achieved simply by adding
>> two more methods to the uclass.
>
> If you are OK with this then I'll have a try at basing my pinctrl
> patches on top of your series. I think it can be made to work.
I know this RFC is missing some items.
OK, we can have a trial to see what needs to be brushed up.
Yours handles simple peripheral IDs to not have impact on the memory footprint,
while mine handles strings as they are given by device tree nodes.
Maybe we need some conversion callbacks?
>>
>> 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.
>>
OK, i will try this.
--
Best Regards
Masahiro Yamada
^ permalink raw reply [flat|nested] 18+ messages in thread
* [U-Boot] [RFC PATCH 1/7] pinctrl: add pinctrl framework
2015-07-15 8:16 ` [U-Boot] [RFC PATCH 1/7] pinctrl: add " Masahiro Yamada
` (2 preceding siblings ...)
2015-07-18 14:37 ` Simon Glass
@ 2015-07-30 2:06 ` Simon Glass
2015-07-30 5:16 ` Masahiro Yamada
3 siblings, 1 reply; 18+ messages in thread
From: Simon Glass @ 2015-07-30 2:06 UTC (permalink / raw)
To: u-boot
Hi Masahiro,
On 15 July 2015 at 02:16, Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>
> Now, a simple pinctrl patch is being proposed by Simon.
> http://patchwork.ozlabs.org/patch/487801/
>
> In the design above, as you see, the uclass is just like a wrapper layer
> to invoke .request and .get_periph_id of low-level drivers.
> In other words, it is Do-It-Yourself thing, so it is up to you how to identify
> which peripheral is being handled in your .get_periph_id().
>
> And here is one example for how a low-level pinctrl driver could be implemented.
> http://patchwork.ozlabs.org/patch/487874/
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.
>
> 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)
I don't think we should support that. People should have to use the
simple version for SPL.
>
> [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 <yamada.masahiro@socionext.com>
> ---
> 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
>
> diff --git a/drivers/Kconfig b/drivers/Kconfig
> index c7e526c..c696036 100644
> --- a/drivers/Kconfig
> +++ b/drivers/Kconfig
> @@ -28,6 +28,8 @@ source "drivers/i2c/Kconfig"
>
> source "drivers/spi/Kconfig"
>
> +source "drivers/pinctrl/Kconfig"
> +
> source "drivers/gpio/Kconfig"
>
> source "drivers/power/Kconfig"
> diff --git a/drivers/Makefile b/drivers/Makefile
> index 280b6d1..7e5ae84 100644
> --- a/drivers/Makefile
> +++ b/drivers/Makefile
> @@ -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/
> diff --git a/drivers/core/device.c b/drivers/core/device.c
> index 46bf388..4e94a4f 100644
> --- a/drivers/core/device.c
> +++ b/drivers/core/device.c
> @@ -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;
>
> + 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.
> ret = uclass_pre_probe_device(dev);
> if (ret)
> goto fail;
> diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
> new file mode 100644
> index 0000000..cae4d55
> --- /dev/null
> +++ b/drivers/pinctrl/Kconfig
> @@ -0,0 +1,2 @@
> +config PINCTRL
> + bool
> diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
> new file mode 100644
> index 0000000..a775a4c
> --- /dev/null
> +++ b/drivers/pinctrl/Makefile
> @@ -0,0 +1 @@
> +obj-y += pinctrl-uclass.o
> diff --git a/drivers/pinctrl/pinctrl-uclass.c b/drivers/pinctrl/pinctrl-uclass.c
> new file mode 100644
> index 0000000..68e4842
> --- /dev/null
> +++ b/drivers/pinctrl/pinctrl-uclass.c
> @@ -0,0 +1,353 @@
> +/*
> + * Copyright (C) 2015 Masahiro Yamada <yamada.masahiro@socionext.com>
> + *
> + * 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 {
> + struct list_head node;
> + const char *name;
> + int index;
> + struct list_head settings;
> +};
> +
Comments
> +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)
> +{
> + 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?
> + 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);
> + 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...
> +
> + 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;
> +
> + 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.
> + 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;
> +
> + 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?
> + }
> +
> + *pstate = state;
> +
> + 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.
> +{
> + 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;
> + }
> +
> + return pinctrl_select_state(state);
> + }
> +
> +#ifdef CONFIG_OF_CONTROL
I think we can assumed this is defined.
> + /*
> + * if the state was not found in the linked list,
> + * search the device tree
Why not populate the table at start-up?
> + */
> + 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",
> +};
> diff --git a/include/dm/device.h b/include/dm/device.h
> index 18296bb..05d4e40 100644
> --- a/include/dm/device.h
> +++ b/include/dm/device.h
> @@ -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 */
> diff --git a/include/dm/pinctrl.h b/include/dm/pinctrl.h
> new file mode 100644
> index 0000000..95925ca
> --- /dev/null
> +++ b/include/dm/pinctrl.h
> @@ -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);
> +int pinctrl_init(struct udevice *dev);
> +#else
> +static inline int pinctrl_set_state(struct udevice *dev,
> + const char *state_name)
> +{
> + return 0;
return -ENOSYS?
> +}
> +
> +static inline int pinctrl_init(struct udevice *dev)
> +{
> + return 0;
> +}
> +#endif
> +
> +#endif /* __PINCTRL_H */
> diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
> index c7310d7..546f7fb 100644
> --- a/include/dm/uclass-id.h
> +++ b/include/dm/uclass-id.h
> @@ -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!
> UCLASS_PMIC, /* PMIC I/O device */
> UCLASS_REGULATOR, /* Regulator device */
> UCLASS_RTC, /* Real time clock device */
> --
> 1.9.1
>
Regards,
Simon
^ permalink raw reply [flat|nested] 18+ messages in thread* [U-Boot] [RFC PATCH 1/7] pinctrl: add pinctrl framework
2015-07-30 2:06 ` Simon Glass
@ 2015-07-30 5:16 ` Masahiro Yamada
2015-07-30 15:16 ` Simon Glass
0 siblings, 1 reply; 18+ messages in thread
From: Masahiro Yamada @ 2015-07-30 5:16 UTC (permalink / raw)
To: u-boot
Hi Simon,
2015-07-30 11:06 GMT+09:00 Simon Glass <sjg@chromium.org>:
> Hi Masahiro,
>
> On 15 July 2015 at 02:16, Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>>
>> Now, a simple pinctrl patch is being proposed by Simon.
>> http://patchwork.ozlabs.org/patch/487801/
>>
>> In the design above, as you see, the uclass is just like a wrapper layer
>> to invoke .request and .get_periph_id of low-level drivers.
>> In other words, it is Do-It-Yourself thing, so it is up to you how to identify
>> which peripheral is being handled in your .get_periph_id().
>>
>> And here is one example for how a low-level pinctrl driver could be implemented.
>> http://patchwork.ozlabs.org/patch/487874/
>
> 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
OK, I will do it.
Now I am pressed, so hopefully I will have some time this weekend.
> 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.
>>
>> 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)
>
> 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.
>>
>> [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 <yamada.masahiro@socionext.com>
>> ---
>> 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
>>
>> diff --git a/drivers/Kconfig b/drivers/Kconfig
>> index c7e526c..c696036 100644
>> --- a/drivers/Kconfig
>> +++ b/drivers/Kconfig
>> @@ -28,6 +28,8 @@ source "drivers/i2c/Kconfig"
>>
>> source "drivers/spi/Kconfig"
>>
>> +source "drivers/pinctrl/Kconfig"
>> +
>> source "drivers/gpio/Kconfig"
>>
>> source "drivers/power/Kconfig"
>> diff --git a/drivers/Makefile b/drivers/Makefile
>> index 280b6d1..7e5ae84 100644
>> --- a/drivers/Makefile
>> +++ b/drivers/Makefile
>> @@ -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/
>> diff --git a/drivers/core/device.c b/drivers/core/device.c
>> index 46bf388..4e94a4f 100644
>> --- a/drivers/core/device.c
>> +++ b/drivers/core/device.c
>> @@ -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;
>>
>> + 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?
>> ret = uclass_pre_probe_device(dev);
>> if (ret)
>> goto fail;
>> diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
>> new file mode 100644
>> index 0000000..cae4d55
>> --- /dev/null
>> +++ b/drivers/pinctrl/Kconfig
>> @@ -0,0 +1,2 @@
>> +config PINCTRL
>> + bool
>> diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
>> new file mode 100644
>> index 0000000..a775a4c
>> --- /dev/null
>> +++ b/drivers/pinctrl/Makefile
>> @@ -0,0 +1 @@
>> +obj-y += pinctrl-uclass.o
>> diff --git a/drivers/pinctrl/pinctrl-uclass.c b/drivers/pinctrl/pinctrl-uclass.c
>> new file mode 100644
>> index 0000000..68e4842
>> --- /dev/null
>> +++ b/drivers/pinctrl/pinctrl-uclass.c
>> @@ -0,0 +1,353 @@
>> +/*
>> + * Copyright (C) 2015 Masahiro Yamada <yamada.masahiro@socionext.com>
>> + *
>> + * 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 {
>> + struct list_head node;
>> + const char *name;
>> + int index;
>> + struct list_head settings;
>> +};
>> +
>
> Comments
OK.
>> +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)
>> +{
>> + 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.
>> + 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);
>> + 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.
>> +
>> + 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;
>> +
>> + 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.
>> + 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;
>> +
>> + 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.
>> + }
>> +
>> + *pstate = state;
>> +
>> + 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.
>> +{
>> + 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;
>> + }
>> +
>> + 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".
>> + /*
>> + * 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.
>> + */
>> + 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",
>> +};
>> diff --git a/include/dm/device.h b/include/dm/device.h
>> index 18296bb..05d4e40 100644
>> --- a/include/dm/device.h
>> +++ b/include/dm/device.h
>> @@ -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 */
>> diff --git a/include/dm/pinctrl.h b/include/dm/pinctrl.h
>> new file mode 100644
>> index 0000000..95925ca
>> --- /dev/null
>> +++ b/include/dm/pinctrl.h
>> @@ -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);
>> +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.
>> +}
>> +
>> +static inline int pinctrl_init(struct udevice *dev)
>> +{
>> + return 0;
>> +}
>> +#endif
>> +
>> +#endif /* __PINCTRL_H */
>> diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
>> index c7310d7..546f7fb 100644
>> --- a/include/dm/uclass-id.h
>> +++ b/include/dm/uclass-id.h
>> @@ -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?
--
Best Regards
Masahiro Yamada
^ permalink raw reply [flat|nested] 18+ messages in thread* [U-Boot] [RFC PATCH 1/7] pinctrl: add pinctrl framework
2015-07-30 5:16 ` Masahiro Yamada
@ 2015-07-30 15:16 ` Simon Glass
0 siblings, 0 replies; 18+ messages in thread
From: Simon Glass @ 2015-07-30 15:16 UTC (permalink / raw)
To: u-boot
Hi Masahiro,
On 29 July 2015 at 23:16, Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> Hi Simon,
>
>
> 2015-07-30 11:06 GMT+09:00 Simon Glass <sjg@chromium.org>:
>> Hi Masahiro,
>>
>> On 15 July 2015 at 02:16, Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>>>
>>> Now, a simple pinctrl patch is being proposed by Simon.
>>> http://patchwork.ozlabs.org/patch/487801/
>>>
>>> In the design above, as you see, the uclass is just like a wrapper layer
>>> to invoke .request and .get_periph_id of low-level drivers.
>>> In other words, it is Do-It-Yourself thing, so it is up to you how to identify
>>> which peripheral is being handled in your .get_periph_id().
>>>
>>> And here is one example for how a low-level pinctrl driver could be implemented.
>>> http://patchwork.ozlabs.org/patch/487874/
>>
>> 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
>
> OK, I will do it.
>
> Now I am pressed, so hopefully I will have some time this weekend.
>
>
>> 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.
I don't think it is that important. Much more important is to use the
same bindings which I understand you already support.
My question is really about understanding the limitation and whether
it can be resolved (as an option, adding lots of extra code, etc.) or
not? We can address it later, or not. I think it's excellent that you
have come up with something reasonable efficient and we don't want to
mess that up.
>
>
>
>>>
>>> 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)
>>
>> 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.
>
OK. Well I suppose if someone has a very large SPL and is OK with
using the full version, that would be fine. But what I really mean is
that the full version should require DT IMO.
>
>
>
>
>>>
>>> [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 <yamada.masahiro@socionext.com>
>>> ---
>>> 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
>>>
>>> diff --git a/drivers/Kconfig b/drivers/Kconfig
>>> index c7e526c..c696036 100644
>>> --- a/drivers/Kconfig
>>> +++ b/drivers/Kconfig
>>> @@ -28,6 +28,8 @@ source "drivers/i2c/Kconfig"
>>>
>>> source "drivers/spi/Kconfig"
>>>
>>> +source "drivers/pinctrl/Kconfig"
>>> +
>>> source "drivers/gpio/Kconfig"
>>>
>>> source "drivers/power/Kconfig"
>>> diff --git a/drivers/Makefile b/drivers/Makefile
>>> index 280b6d1..7e5ae84 100644
>>> --- a/drivers/Makefile
>>> +++ b/drivers/Makefile
>>> @@ -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/
>>> diff --git a/drivers/core/device.c b/drivers/core/device.c
>>> index 46bf388..4e94a4f 100644
>>> --- a/drivers/core/device.c
>>> +++ b/drivers/core/device.c
>>> @@ -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;
>>>
>>> + 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?
>
I was thinking that the pinctrl uclass API could call a function to
populate the data if it is not there. Thus this could be done only
when a pin is requested. Until then each struct udevice would just
have a NULL pointer.
>
>
>
>
>>> ret = uclass_pre_probe_device(dev);
>>> if (ret)
>>> goto fail;
>>> diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
>>> new file mode 100644
>>> index 0000000..cae4d55
>>> --- /dev/null
>>> +++ b/drivers/pinctrl/Kconfig
>>> @@ -0,0 +1,2 @@
>>> +config PINCTRL
>>> + bool
>>> diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
>>> new file mode 100644
>>> index 0000000..a775a4c
>>> --- /dev/null
>>> +++ b/drivers/pinctrl/Makefile
>>> @@ -0,0 +1 @@
>>> +obj-y += pinctrl-uclass.o
>>> diff --git a/drivers/pinctrl/pinctrl-uclass.c b/drivers/pinctrl/pinctrl-uclass.c
>>> new file mode 100644
>>> index 0000000..68e4842
>>> --- /dev/null
>>> +++ b/drivers/pinctrl/pinctrl-uclass.c
>>> @@ -0,0 +1,353 @@
>>> +/*
>>> + * Copyright (C) 2015 Masahiro Yamada <yamada.masahiro@socionext.com>
>>> + *
>>> + * 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 {
>>> + struct list_head node;
>>> + const char *name;
>>> + int index;
>>> + struct list_head settings;
>>> +};
>>> +
>>
>> Comments
>
> OK.
>
>>> +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)
>>> +{
>>> + 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.
>
OK. But we should not printf() in drivers - debug() is OK. If you go
with doing this lazily then this function will not be called from
device_probe(). But if device_probe() gets this error it could ignore
it- perhaps another error number could be used instead so that it is
less common than -ENOSYS. It is better I think to pass the error up
the stack rather than suppressing it in the low levels - that's really
hard to debug.
>
>
>
>>> + 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);
>>> + 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.
>
>
I even wonder whether you need this table, or could/should just scan
the node as you need it each time. Probably that would be too
inefficient, but if we are only making these calls once on each driver
probe it could be an option.
>
>
>>> +
>>> + 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;
>>> +
>>> + 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.
So the parent of a pinconf node is always a pinctrl device. I think
this is a bit of a funny case since normally a device tree node has a
struct udevice. Here we have a phandle pointing to a subnode of that
device. Even with GPIO we create a separate device for each bank to
avoid this problem - then the phandles each point to a struct
udevice's of_offset.
We could handle this by using an unflattened tree, or even a separate
structure that knows about phandles. But that seems like a separate
task. Maybe the pinctrl driver could scan all its nodes and put the
phandles into a table - i.e. a list of all the pins it manages? That
would not be unreasonable although it might be cleaner to use a struct
udevice for each (but much less efficient!). What do you think?
Perhaps if move this into a function like pinctrl_find_phandle(int
phandle) which returns the struct udevice of the relevant pinctrl
device then we could worry about optimisation later?
Do we need the loop? I think we should require that the immediate
parent is a pinctrl device and drop the loop.
>
>
>
>
>
>>> + 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;
>>> +
>>> + 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.
>
>
>
>>> + }
>>> +
>>> + *pstate = state;
>>> +
>>> + 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.
>
>
>>> +{
>>> + 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;
>>> + }
>>> +
>>> + 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".
>
>
>>> + /*
>>> + * 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.
>
OK I see. Yes your way makes good sense and lazy init is a great idea
for pinctrl.
>
>
>>> + */
>>> + 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",
>>> +};
>>> diff --git a/include/dm/device.h b/include/dm/device.h
>>> index 18296bb..05d4e40 100644
>>> --- a/include/dm/device.h
>>> +++ b/include/dm/device.h
>>> @@ -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 */
>>> diff --git a/include/dm/pinctrl.h b/include/dm/pinctrl.h
>>> new file mode 100644
>>> index 0000000..95925ca
>>> --- /dev/null
>>> +++ b/include/dm/pinctrl.h
>>> @@ -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);
>>> +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.
Perhaps we can catch this error there and deal with it as 'not
implemented, OK that's fine, contine'.
>
>
>>> +}
>>> +
>>> +static inline int pinctrl_init(struct udevice *dev)
>>> +{
>>> + return 0;
>>> +}
>>> +#endif
>>> +
>>> +#endif /* __PINCTRL_H */
>>> diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
>>> index c7310d7..546f7fb 100644
>>> --- a/include/dm/uclass-id.h
>>> +++ b/include/dm/uclass-id.h
>>> @@ -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?
I just mean it repeats the symbol. Something like 'Pin multiplexing
and configuration' would be better.
Regards,
Simon
^ permalink raw reply [flat|nested] 18+ messages in thread
* [U-Boot] [RFC PATCH 2/7] pinctrl: UniPhier: add UniPhier pinctrl core support
2015-07-15 8:16 [U-Boot] [RFC PATCH 0/7] DM: pinctrl: Another implemtation of pinctrl framework Masahiro Yamada
2015-07-15 8:16 ` [U-Boot] [RFC PATCH 1/7] pinctrl: add " Masahiro Yamada
@ 2015-07-15 8:16 ` Masahiro Yamada
2015-07-15 8:16 ` [U-Boot] [RFC PATCH 3/7] pinctrl: UniPhier: add UniPhier PH1-LD4 pinctrl driver Masahiro Yamada
` (4 subsequent siblings)
6 siblings, 0 replies; 18+ messages in thread
From: Masahiro Yamada @ 2015-07-15 8:16 UTC (permalink / raw)
To: u-boot
The core support for the pinctrl drivers for all the UniPhier SoCs.
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---
drivers/pinctrl/Kconfig | 2 +
drivers/pinctrl/Makefile | 2 +
drivers/pinctrl/uniphier/Kconfig | 7 ++
drivers/pinctrl/uniphier/Makefile | 1 +
drivers/pinctrl/uniphier/pinctrl-uniphier-core.c | 127 +++++++++++++++++++++++
drivers/pinctrl/uniphier/pinctrl-uniphier.h | 51 +++++++++
6 files changed, 190 insertions(+)
create mode 100644 drivers/pinctrl/uniphier/Kconfig
create mode 100644 drivers/pinctrl/uniphier/Makefile
create mode 100644 drivers/pinctrl/uniphier/pinctrl-uniphier-core.c
create mode 100644 drivers/pinctrl/uniphier/pinctrl-uniphier.h
diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index cae4d55..758b5c7 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -1,2 +1,4 @@
config PINCTRL
bool
+
+source "drivers/pinctrl/uniphier/Kconfig"
diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
index a775a4c..2af6887 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -1 +1,3 @@
obj-y += pinctrl-uclass.o
+
+obj-$(CONFIG_ARCH_UNIPHIER) += uniphier/
diff --git a/drivers/pinctrl/uniphier/Kconfig b/drivers/pinctrl/uniphier/Kconfig
new file mode 100644
index 0000000..d458394
--- /dev/null
+++ b/drivers/pinctrl/uniphier/Kconfig
@@ -0,0 +1,7 @@
+if ARCH_UNIPHIER
+
+config PINCTRL_UNIPHIER_CORE
+ bool
+ select PINCTRL
+
+endif
diff --git a/drivers/pinctrl/uniphier/Makefile b/drivers/pinctrl/uniphier/Makefile
new file mode 100644
index 0000000..748aa1b
--- /dev/null
+++ b/drivers/pinctrl/uniphier/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_PINCTRL_UNIPHIER_CORE) += pinctrl-uniphier-core.o
diff --git a/drivers/pinctrl/uniphier/pinctrl-uniphier-core.c b/drivers/pinctrl/uniphier/pinctrl-uniphier-core.c
new file mode 100644
index 0000000..6436a30
--- /dev/null
+++ b/drivers/pinctrl/uniphier/pinctrl-uniphier-core.c
@@ -0,0 +1,127 @@
+/*
+ * Copyright (C) 2015 Masahiro Yamada <yamada.masahiro@socionext.com>
+ *
+ * SPDX-License-Identifier: GPL-2.0+
+ */
+
+#include <common.h>
+#include <mapmem.h>
+#include <linux/io.h>
+#include <linux/err.h>
+#include <dm/device.h>
+#include <dm/pinctrl.h>
+
+#include "pinctrl-uniphier.h"
+
+static void uniphier_pinmux_set_one(struct udevice *dev,
+ const struct uniphier_mux *mux)
+{
+ struct uniphier_pinctrl_priv *priv = dev_get_priv(dev);
+ unsigned mux_bits = priv->socdata->mux_bits;
+ unsigned reg_stride = priv->socdata->reg_stride;
+ unsigned pin = mux->pin;
+ unsigned muxval = mux->muxval;
+ unsigned reg, reg_end, shift, mask;
+ u32 tmp;
+
+ reg = UNIPHIER_PINCTRL_PINMUX_BASE + pin * mux_bits / 32 * reg_stride;
+ reg_end = reg + reg_stride;
+ shift = pin * mux_bits % 32;
+ mask = (1U << mux_bits) - 1;
+
+ /*
+ * If reg_stride is greater than 4, the MSB of each pinsel shall be
+ * stored in the offset+4.
+ */
+ for (; reg < reg_end; reg += 4) {
+ tmp = readl(priv->base + reg);
+ tmp &= ~(mask << shift);
+ tmp |= (mask & muxval) << shift;
+ writel(tmp, priv->base + reg);
+
+ muxval >>= mux_bits;
+ }
+
+ if (priv->socdata->load_pinctrl)
+ writel(1, priv->base + UNIPHIER_PINCTRL_LOAD_PINMUX);
+}
+
+static int uniphier_pinmux_set(struct udevice *dev, const char *group,
+ const char *function)
+{
+ struct uniphier_pinctrl_priv *priv = dev_get_priv(dev);
+ const struct uniphier_pinmux_data *pinmux_data =
+ priv->socdata->pinmux_data;
+ const unsigned num_pinmux_data = priv->socdata->num_pinmux_data;
+ const struct uniphier_mux *mux;
+ unsigned nmux;
+ int i;
+
+ for (i = 0; i < num_pinmux_data; i++) {
+ if (strcmp(pinmux_data[i].function, function))
+ continue;
+
+ if (!group || !strcmp(pinmux_data[i].group, group))
+ break;
+ }
+
+ if (i >= num_pinmux_data) {
+ printf("unsupported pinmux: group = %s, functions = %s\n",
+ group, function);
+ return -EINVAL;
+ }
+
+ mux = pinmux_data[i].muxdata;
+ nmux = pinmux_data[i].num_muxdata;
+
+ for (i = 0; i < nmux; i++)
+ uniphier_pinmux_set_one(dev, &mux[i]);
+
+ return 0;
+}
+
+static int uniphier_pinconf_set(struct udevice *dev, const char *group,
+ const char *conf_param, unsigned conf_arg)
+{
+ /* not implemented yet: just debug message */
+ printf("%s: pinconf_set, group = %s, conf_param = %s, conf_arg = %d\n",
+ dev->name, group, conf_param, conf_arg);
+
+ return 0;
+}
+
+const struct pinctrl_ops uniphier_pinctrl_ops = {
+ .pinmux_set = uniphier_pinmux_set,
+ .pinconf_set = uniphier_pinconf_set,
+};
+
+int uniphier_pinctrl_probe(struct udevice *dev,
+ struct uniphier_pinctrl_socdata *socdata)
+{
+ struct uniphier_pinctrl_priv *priv = dev_get_priv(dev);
+ DECLARE_GLOBAL_DATA_PTR;
+ fdt_addr_t addr;
+ fdt_size_t size;
+
+ addr = fdtdec_get_addr_size(gd->fdt_blob, dev->of_offset, "reg",
+ &size);
+ if (addr == FDT_ADDR_T_NONE)
+ return -EINVAL;
+
+ priv->base = map_sysmem(addr, size);
+ if (!priv->base)
+ return -ENOMEM;
+
+ priv->socdata = socdata;
+
+ return 0;
+}
+
+int uniphier_pinctrl_remove(struct udevice *dev)
+{
+ struct uniphier_pinctrl_priv *priv = dev_get_priv(dev);
+
+ unmap_sysmem(priv->base);
+
+ return 0;
+}
diff --git a/drivers/pinctrl/uniphier/pinctrl-uniphier.h b/drivers/pinctrl/uniphier/pinctrl-uniphier.h
new file mode 100644
index 0000000..681158b
--- /dev/null
+++ b/drivers/pinctrl/uniphier/pinctrl-uniphier.h
@@ -0,0 +1,51 @@
+#ifndef __PINCTRL_UNIPHIER_H__
+#define __PINCTRL_UNIPHIER_H__
+
+#include <linux/kernel.h>
+#include <linux/types.h>
+
+#define UNIPHIER_PINCTRL_PINMUX_BASE 0x0
+#define UNIPHIER_PINCTRL_LOAD_PINMUX 0x700
+#define UNIPHIER_PINCTRL_IECTRL 0xd00
+
+struct uniphier_mux {
+ unsigned pin;
+ unsigned muxval;
+};
+
+struct uniphier_pinmux_data {
+ const char *group;
+ const char *function;
+ const struct uniphier_mux *muxdata;
+ unsigned num_muxdata;
+};
+
+struct uniphier_pinctrl_socdata {
+ const struct uniphier_pinmux_data *pinmux_data;
+ unsigned num_pinmux_data;
+ unsigned mux_bits;
+ unsigned reg_stride;
+ bool load_pinctrl;
+};
+
+#define UNIPHIER_PINMUX_DATA(grp, func) \
+ { \
+ .group = #grp, \
+ .function = #func, \
+ .muxdata = grp##_mux, \
+ .num_muxdata = ARRAY_SIZE(grp##_mux), \
+ }
+
+struct uniphier_pinctrl_priv {
+ void __iomem *base;
+ struct uniphier_pinctrl_socdata *socdata;
+};
+
+extern const struct pinctrl_ops uniphier_pinctrl_ops;
+
+int uniphier_pinctrl_probe(struct udevice *dev,
+ struct uniphier_pinctrl_socdata *socdata);
+
+int uniphier_pinctrl_remove(struct udevice *dev);
+
+#endif /* __PINCTRL_UNIPHIER_H__ */
--
1.9.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* [U-Boot] [RFC PATCH 3/7] pinctrl: UniPhier: add UniPhier PH1-LD4 pinctrl driver
2015-07-15 8:16 [U-Boot] [RFC PATCH 0/7] DM: pinctrl: Another implemtation of pinctrl framework Masahiro Yamada
2015-07-15 8:16 ` [U-Boot] [RFC PATCH 1/7] pinctrl: add " Masahiro Yamada
2015-07-15 8:16 ` [U-Boot] [RFC PATCH 2/7] pinctrl: UniPhier: add UniPhier pinctrl core support Masahiro Yamada
@ 2015-07-15 8:16 ` Masahiro Yamada
2015-07-15 8:16 ` [U-Boot] [RFC PATCH 4/7] pinctrl: UniPhier: add UniPhier PH1-Pro4 " Masahiro Yamada
` (3 subsequent siblings)
6 siblings, 0 replies; 18+ messages in thread
From: Masahiro Yamada @ 2015-07-15 8:16 UTC (permalink / raw)
To: u-boot
Add pinmux support for UniPhier PH1-LD4 SoC.
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---
drivers/pinctrl/uniphier/Kconfig | 4 ++
drivers/pinctrl/uniphier/Makefile | 2 +
drivers/pinctrl/uniphier/pinctrl-ph1-ld4.c | 69 ++++++++++++++++++++++++++++++
3 files changed, 75 insertions(+)
create mode 100644 drivers/pinctrl/uniphier/pinctrl-ph1-ld4.c
diff --git a/drivers/pinctrl/uniphier/Kconfig b/drivers/pinctrl/uniphier/Kconfig
index d458394..5c25923 100644
--- a/drivers/pinctrl/uniphier/Kconfig
+++ b/drivers/pinctrl/uniphier/Kconfig
@@ -4,4 +4,8 @@ config PINCTRL_UNIPHIER_CORE
bool
select PINCTRL
+config PINCTRL_UNIPHIER_PH1_LD4
+ bool "UniPhier PH1-LD4 SoC pinctrl driver"
+ select PINCTRL_UNIPHIER_CORE
+
endif
diff --git a/drivers/pinctrl/uniphier/Makefile b/drivers/pinctrl/uniphier/Makefile
index 748aa1b..b4bd042 100644
--- a/drivers/pinctrl/uniphier/Makefile
+++ b/drivers/pinctrl/uniphier/Makefile
@@ -1 +1,3 @@
obj-$(CONFIG_PINCTRL_UNIPHIER_CORE) += pinctrl-uniphier-core.o
+
+obj-$(CONFIG_PINCTRL_UNIPHIER_PH1_LD4) += pinctrl-ph1-ld4.o
diff --git a/drivers/pinctrl/uniphier/pinctrl-ph1-ld4.c b/drivers/pinctrl/uniphier/pinctrl-ph1-ld4.c
new file mode 100644
index 0000000..66759f2
--- /dev/null
+++ b/drivers/pinctrl/uniphier/pinctrl-ph1-ld4.c
@@ -0,0 +1,69 @@
+/*
+ * Copyright (C) 2015 Masahiro Yamada <yamada.masahiro@socionext.com>
+ *
+ * SPDX-License-Identifier: GPL-2.0+
+ */
+
+#include <dm/device.h>
+#include <dm/pinctrl.h>
+
+#include "pinctrl-uniphier.h"
+
+static const struct uniphier_mux i2c0_mux[] = {{102, 0}, {103, 0}};
+static const struct uniphier_mux i2c1_mux[] = {{104, 0}, {105, 0}};
+static const struct uniphier_mux i2c2_mux[] = {{108, 2}, {109, 2}};
+static const struct uniphier_mux i2c3_mux[] = {{108, 3}, {109, 3}};
+static const struct uniphier_mux uart0_mux[] = {{85, 1}, {88, 1}};
+static const struct uniphier_mux uart1_mux[] = {{115, 13}, {116, 13}};
+static const struct uniphier_mux uart1b_mux[] = {{69, 23}, {70, 23}};
+static const struct uniphier_mux uart2_mux[] = {{128, 13}, {129, 13}};
+static const struct uniphier_mux uart3_mux[] = {{110, 1}, {111, 1}};
+static const struct uniphier_mux usb0_mux[] = {{53, 0}, {54, 0}};
+static const struct uniphier_mux usb1_mux[] = {{55, 0}, {56, 0}};
+static const struct uniphier_mux usb2_mux[] = {{155, 4}, {156, 4}};
+static const struct uniphier_mux usb2b_mux[] = {{67, 23}, {68, 23}};
+
+static const struct uniphier_pinmux_data ph1_ld4_pinmux_data[] = {
+ UNIPHIER_PINMUX_DATA(i2c0, i2c0),
+ UNIPHIER_PINMUX_DATA(i2c1, i2c1),
+ UNIPHIER_PINMUX_DATA(i2c2, i2c2),
+ UNIPHIER_PINMUX_DATA(i2c3, i2c3),
+ UNIPHIER_PINMUX_DATA(uart0, uart0),
+ UNIPHIER_PINMUX_DATA(uart1, uart1),
+ UNIPHIER_PINMUX_DATA(uart1b, uart1),
+ UNIPHIER_PINMUX_DATA(uart2, uart2),
+ UNIPHIER_PINMUX_DATA(uart3, uart3),
+ UNIPHIER_PINMUX_DATA(usb0, usb0),
+ UNIPHIER_PINMUX_DATA(usb1, usb1),
+ UNIPHIER_PINMUX_DATA(usb2, usb2),
+ UNIPHIER_PINMUX_DATA(usb2b, usb2),
+};
+
+static struct uniphier_pinctrl_socdata ph1_ld4_pinctrl_socdata = {
+ .pinmux_data = ph1_ld4_pinmux_data,
+ .num_pinmux_data = ARRAY_SIZE(ph1_ld4_pinmux_data),
+ .mux_bits = 8,
+ .reg_stride = 4,
+ .load_pinctrl = false,
+};
+
+static int ph1_ld4_pinctrl_probe(struct udevice *dev)
+{
+ return uniphier_pinctrl_probe(dev, &ph1_ld4_pinctrl_socdata);
+}
+
+static const struct udevice_id ph1_ld4_pinctrl_match[] = {
+ { .compatible = "socionext,ph1-ld4-pinctrl" },
+ { /* sentinel */ }
+};
+
+U_BOOT_DRIVER(ph1_ld4_pinctrl) = {
+ .name = "ph1-ld4-pinctrl",
+ .id = UCLASS_PINCTRL,
+ .of_match = of_match_ptr(ph1_ld4_pinctrl_match),
+ .probe = ph1_ld4_pinctrl_probe,
+ .remove = uniphier_pinctrl_remove,
+ .priv_auto_alloc_size = sizeof(struct uniphier_pinctrl_priv),
+ .ops = &uniphier_pinctrl_ops,
+ .flags = DM_FLAG_PRE_RELOC,
+};
--
1.9.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* [U-Boot] [RFC PATCH 4/7] pinctrl: UniPhier: add UniPhier PH1-Pro4 pinctrl driver
2015-07-15 8:16 [U-Boot] [RFC PATCH 0/7] DM: pinctrl: Another implemtation of pinctrl framework Masahiro Yamada
` (2 preceding siblings ...)
2015-07-15 8:16 ` [U-Boot] [RFC PATCH 3/7] pinctrl: UniPhier: add UniPhier PH1-LD4 pinctrl driver Masahiro Yamada
@ 2015-07-15 8:16 ` Masahiro Yamada
2015-07-15 8:16 ` [U-Boot] [RFC PATCH 5/7] pinctrl: UniPhier: add UniPhier PH1-sLD8 " Masahiro Yamada
` (2 subsequent siblings)
6 siblings, 0 replies; 18+ messages in thread
From: Masahiro Yamada @ 2015-07-15 8:16 UTC (permalink / raw)
To: u-boot
Add pinmux support for UniPhier PH1-Pro4 SoC.
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---
drivers/pinctrl/uniphier/Kconfig | 4 ++
drivers/pinctrl/uniphier/Makefile | 1 +
drivers/pinctrl/uniphier/pinctrl-ph1-pro4.c | 69 +++++++++++++++++++++++++++++
3 files changed, 74 insertions(+)
create mode 100644 drivers/pinctrl/uniphier/pinctrl-ph1-pro4.c
diff --git a/drivers/pinctrl/uniphier/Kconfig b/drivers/pinctrl/uniphier/Kconfig
index 5c25923..38c2f5e 100644
--- a/drivers/pinctrl/uniphier/Kconfig
+++ b/drivers/pinctrl/uniphier/Kconfig
@@ -8,4 +8,8 @@ config PINCTRL_UNIPHIER_PH1_LD4
bool "UniPhier PH1-LD4 SoC pinctrl driver"
select PINCTRL_UNIPHIER_CORE
+config PINCTRL_UNIPHIER_PH1_PRO4
+ bool "UniPhier PH1-Pro4 SoC pinctrl driver"
+ select PINCTRL_UNIPHIER_CORE
+
endif
diff --git a/drivers/pinctrl/uniphier/Makefile b/drivers/pinctrl/uniphier/Makefile
index b4bd042..b1b597e 100644
--- a/drivers/pinctrl/uniphier/Makefile
+++ b/drivers/pinctrl/uniphier/Makefile
@@ -1,3 +1,4 @@
obj-$(CONFIG_PINCTRL_UNIPHIER_CORE) += pinctrl-uniphier-core.o
obj-$(CONFIG_PINCTRL_UNIPHIER_PH1_LD4) += pinctrl-ph1-ld4.o
+obj-$(CONFIG_PINCTRL_UNIPHIER_PH1_PRO4) += pinctrl-ph1-pro4.o
diff --git a/drivers/pinctrl/uniphier/pinctrl-ph1-pro4.c b/drivers/pinctrl/uniphier/pinctrl-ph1-pro4.c
new file mode 100644
index 0000000..ec4322e
--- /dev/null
+++ b/drivers/pinctrl/uniphier/pinctrl-ph1-pro4.c
@@ -0,0 +1,69 @@
+/*
+ * Copyright (C) 2015 Masahiro Yamada <yamada.masahiro@socionext.com>
+ *
+ * SPDX-License-Identifier: GPL-2.0+
+ */
+
+#include <dm/device.h>
+#include <dm/pinctrl.h>
+
+#include "pinctrl-uniphier.h"
+
+static const struct uniphier_mux i2c0_mux[] = {{142, 0}, {143, 0}};
+static const struct uniphier_mux i2c1_mux[] = {{144, 0}, {145, 0}};
+static const struct uniphier_mux i2c2_mux[] = {{146, 0}, {147, 0}};
+static const struct uniphier_mux i2c3_mux[] = {{148, 0}, {149, 0}};
+static const struct uniphier_mux i2c6_mux[] = {{308, 6}, {309, 6}};
+static const struct uniphier_mux uart0_mux[] = {{127, 0}, {128, 0}};
+static const struct uniphier_mux uart1_mux[] = {{129, 0}, {130, 0}};
+static const struct uniphier_mux uart2_mux[] = {{131, 0}, {132, 0}};
+static const struct uniphier_mux uart3_mux[] = {{88, 2}, {89, 2}};
+static const struct uniphier_mux usb0_mux[] = {{180, 0}, {181, 0}};
+static const struct uniphier_mux usb1_mux[] = {{182, 0}, {183, 0}};
+static const struct uniphier_mux usb2_mux[] = {{184, 0}, {185, 0}};
+static const struct uniphier_mux usb3_mux[] = {{186, 0}, {187, 0}};
+
+static const struct uniphier_pinmux_data ph1_pro4_pinmux_data[] = {
+ UNIPHIER_PINMUX_DATA(i2c0, i2c0),
+ UNIPHIER_PINMUX_DATA(i2c1, i2c1),
+ UNIPHIER_PINMUX_DATA(i2c2, i2c2),
+ UNIPHIER_PINMUX_DATA(i2c3, i2c3),
+ UNIPHIER_PINMUX_DATA(i2c6, i2c6),
+ UNIPHIER_PINMUX_DATA(uart0, uart0),
+ UNIPHIER_PINMUX_DATA(uart1, uart1),
+ UNIPHIER_PINMUX_DATA(uart2, uart2),
+ UNIPHIER_PINMUX_DATA(uart3, uart3),
+ UNIPHIER_PINMUX_DATA(usb0, usb0),
+ UNIPHIER_PINMUX_DATA(usb1, usb1),
+ UNIPHIER_PINMUX_DATA(usb2, usb2),
+ UNIPHIER_PINMUX_DATA(usb3, usb3),
+};
+
+static struct uniphier_pinctrl_socdata ph1_pro4_pinctrl_socdata = {
+ .pinmux_data = ph1_pro4_pinmux_data,
+ .num_pinmux_data = ARRAY_SIZE(ph1_pro4_pinmux_data),
+ .mux_bits = 4,
+ .reg_stride = 8,
+ .load_pinctrl = true,
+};
+
+static int ph1_pro4_pinctrl_probe(struct udevice *dev)
+{
+ return uniphier_pinctrl_probe(dev, &ph1_pro4_pinctrl_socdata);
+}
+
+static const struct udevice_id ph1_pro4_pinctrl_match[] = {
+ { .compatible = "socionext,ph1-pro4-pinctrl" },
+ { /* sentinel */ }
+};
+
+U_BOOT_DRIVER(ph1_pro4_pinctrl) = {
+ .name = "ph1-pro4-pinctrl",
+ .id = UCLASS_PINCTRL,
+ .of_match = of_match_ptr(ph1_pro4_pinctrl_match),
+ .probe = ph1_pro4_pinctrl_probe,
+ .remove = uniphier_pinctrl_remove,
+ .priv_auto_alloc_size = sizeof(struct uniphier_pinctrl_priv),
+ .ops = &uniphier_pinctrl_ops,
+ .flags = DM_FLAG_PRE_RELOC,
+};
--
1.9.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* [U-Boot] [RFC PATCH 5/7] pinctrl: UniPhier: add UniPhier PH1-sLD8 pinctrl driver
2015-07-15 8:16 [U-Boot] [RFC PATCH 0/7] DM: pinctrl: Another implemtation of pinctrl framework Masahiro Yamada
` (3 preceding siblings ...)
2015-07-15 8:16 ` [U-Boot] [RFC PATCH 4/7] pinctrl: UniPhier: add UniPhier PH1-Pro4 " Masahiro Yamada
@ 2015-07-15 8:16 ` Masahiro Yamada
2015-07-15 8:16 ` [U-Boot] [RFC PATCH 6/7] ARM: dts: UniPhier: add nodes for pinctrl devices and pin configs Masahiro Yamada
2015-07-15 8:16 ` [U-Boot] [RFC PATCH 7/7] ARM: select pinctrl drivers from Kconfig Masahiro Yamada
6 siblings, 0 replies; 18+ messages in thread
From: Masahiro Yamada @ 2015-07-15 8:16 UTC (permalink / raw)
To: u-boot
Add pinmux support for UniPhier PH1-sLD8 SoC.
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---
drivers/pinctrl/uniphier/Kconfig | 4 ++
drivers/pinctrl/uniphier/Makefile | 1 +
drivers/pinctrl/uniphier/pinctrl-ph1-sld8.c | 65 +++++++++++++++++++++++++++++
3 files changed, 70 insertions(+)
create mode 100644 drivers/pinctrl/uniphier/pinctrl-ph1-sld8.c
diff --git a/drivers/pinctrl/uniphier/Kconfig b/drivers/pinctrl/uniphier/Kconfig
index 38c2f5e..005a16d 100644
--- a/drivers/pinctrl/uniphier/Kconfig
+++ b/drivers/pinctrl/uniphier/Kconfig
@@ -12,4 +12,8 @@ config PINCTRL_UNIPHIER_PH1_PRO4
bool "UniPhier PH1-Pro4 SoC pinctrl driver"
select PINCTRL_UNIPHIER_CORE
+config PINCTRL_UNIPHIER_PH1_SLD8
+ bool "UniPhier PH1-sLD8 SoC pinctrl driver"
+ select PINCTRL_UNIPHIER_CORE
+
endif
diff --git a/drivers/pinctrl/uniphier/Makefile b/drivers/pinctrl/uniphier/Makefile
index b1b597e..3349fff 100644
--- a/drivers/pinctrl/uniphier/Makefile
+++ b/drivers/pinctrl/uniphier/Makefile
@@ -2,3 +2,4 @@ obj-$(CONFIG_PINCTRL_UNIPHIER_CORE) += pinctrl-uniphier-core.o
obj-$(CONFIG_PINCTRL_UNIPHIER_PH1_LD4) += pinctrl-ph1-ld4.o
obj-$(CONFIG_PINCTRL_UNIPHIER_PH1_PRO4) += pinctrl-ph1-pro4.o
+obj-$(CONFIG_PINCTRL_UNIPHIER_PH1_SLD8) += pinctrl-ph1-sld8.o
diff --git a/drivers/pinctrl/uniphier/pinctrl-ph1-sld8.c b/drivers/pinctrl/uniphier/pinctrl-ph1-sld8.c
new file mode 100644
index 0000000..4a08886
--- /dev/null
+++ b/drivers/pinctrl/uniphier/pinctrl-ph1-sld8.c
@@ -0,0 +1,65 @@
+/*
+ * Copyright (C) 2015 Masahiro Yamada <yamada.masahiro@socionext.com>
+ *
+ * SPDX-License-Identifier: GPL-2.0+
+ */
+
+#include <dm/device.h>
+#include <dm/pinctrl.h>
+
+#include "pinctrl-uniphier.h"
+
+static const struct uniphier_mux i2c0_mux[] = {{102, 0}, {103, 0}};
+static const struct uniphier_mux i2c1_mux[] = {{104, 0}, {105, 0}};
+static const struct uniphier_mux i2c2_mux[] = {{108, 2}, {109, 2}};
+static const struct uniphier_mux i2c3_mux[] = {{108, 3}, {109, 3}};
+static const struct uniphier_mux uart0_mux[] = {{70, 3}, {71, 3}};
+static const struct uniphier_mux uart1_mux[] = {{114, 0}, {115, 0}};
+static const struct uniphier_mux uart2_mux[] = {{112, 1}, {113, 1}};
+static const struct uniphier_mux uart3_mux[] = {{110, 1}, {111, 1}};
+static const struct uniphier_mux usb0_mux[] = {{41, 0}, {42, 0}};
+static const struct uniphier_mux usb1_mux[] = {{43, 0}, {44, 0}};
+static const struct uniphier_mux usb2_mux[] = {{114, 1}, {115, 1}};
+
+static const struct uniphier_pinmux_data ph1_sld8_pinmux_data[] = {
+ UNIPHIER_PINMUX_DATA(i2c0, i2c0),
+ UNIPHIER_PINMUX_DATA(i2c1, i2c1),
+ UNIPHIER_PINMUX_DATA(i2c2, i2c2),
+ UNIPHIER_PINMUX_DATA(i2c3, i2c3),
+ UNIPHIER_PINMUX_DATA(uart0, uart0),
+ UNIPHIER_PINMUX_DATA(uart1, uart1),
+ UNIPHIER_PINMUX_DATA(uart2, uart2),
+ UNIPHIER_PINMUX_DATA(uart3, uart3),
+ UNIPHIER_PINMUX_DATA(usb0, usb0),
+ UNIPHIER_PINMUX_DATA(usb1, usb1),
+ UNIPHIER_PINMUX_DATA(usb2, usb2),
+};
+
+static struct uniphier_pinctrl_socdata ph1_sld8_pinctrl_socdata = {
+ .pinmux_data = ph1_sld8_pinmux_data,
+ .num_pinmux_data = ARRAY_SIZE(ph1_sld8_pinmux_data),
+ .mux_bits = 8,
+ .reg_stride = 4,
+ .load_pinctrl = false,
+};
+
+static int ph1_sld8_pinctrl_probe(struct udevice *dev)
+{
+ return uniphier_pinctrl_probe(dev, &ph1_sld8_pinctrl_socdata);
+}
+
+static const struct udevice_id ph1_sld8_pinctrl_match[] = {
+ { .compatible = "socionext,ph1-sld8-pinctrl" },
+ { /* sentinel */ }
+};
+
+U_BOOT_DRIVER(ph1_sld8_pinctrl) = {
+ .name = "ph1-sld8-pinctrl",
+ .id = UCLASS_PINCTRL,
+ .of_match = of_match_ptr(ph1_sld8_pinctrl_match),
+ .probe = ph1_sld8_pinctrl_probe,
+ .remove = uniphier_pinctrl_remove,
+ .priv_auto_alloc_size = sizeof(struct uniphier_pinctrl_priv),
+ .ops = &uniphier_pinctrl_ops,
+ .flags = DM_FLAG_PRE_RELOC,
+};
--
1.9.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* [U-Boot] [RFC PATCH 6/7] ARM: dts: UniPhier: add nodes for pinctrl devices and pin configs
2015-07-15 8:16 [U-Boot] [RFC PATCH 0/7] DM: pinctrl: Another implemtation of pinctrl framework Masahiro Yamada
` (4 preceding siblings ...)
2015-07-15 8:16 ` [U-Boot] [RFC PATCH 5/7] pinctrl: UniPhier: add UniPhier PH1-sLD8 " Masahiro Yamada
@ 2015-07-15 8:16 ` Masahiro Yamada
2015-07-15 8:16 ` [U-Boot] [RFC PATCH 7/7] ARM: select pinctrl drivers from Kconfig Masahiro Yamada
6 siblings, 0 replies; 18+ messages in thread
From: Masahiro Yamada @ 2015-07-15 8:16 UTC (permalink / raw)
To: u-boot
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---
arch/arm/dts/uniphier-ph1-ld4.dtsi | 30 ++++++++++++++
arch/arm/dts/uniphier-ph1-pro4.dtsi | 34 ++++++++++++++++
arch/arm/dts/uniphier-ph1-sld8.dtsi | 30 ++++++++++++++
arch/arm/dts/uniphier-pinctrl.dtsi | 80 +++++++++++++++++++++++++++++++++++++
4 files changed, 174 insertions(+)
create mode 100644 arch/arm/dts/uniphier-pinctrl.dtsi
diff --git a/arch/arm/dts/uniphier-ph1-ld4.dtsi b/arch/arm/dts/uniphier-ph1-ld4.dtsi
index 5a1d10c..356f637 100644
--- a/arch/arm/dts/uniphier-ph1-ld4.dtsi
+++ b/arch/arm/dts/uniphier-ph1-ld4.dtsi
@@ -47,6 +47,8 @@
compatible = "socionext,uniphier-uart";
status = "disabled";
reg = <0x54006800 0x20>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_uart0>;
clock-frequency = <36864000>;
};
@@ -54,6 +56,8 @@
compatible = "socionext,uniphier-uart";
status = "disabled";
reg = <0x54006900 0x20>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_uart1>;
clock-frequency = <36864000>;
};
@@ -61,6 +65,8 @@
compatible = "socionext,uniphier-uart";
status = "disabled";
reg = <0x54006a00 0x20>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_uart2>;
clock-frequency = <36864000>;
};
@@ -68,6 +74,8 @@
compatible = "socionext,uniphier-uart";
status = "disabled";
reg = <0x54006b00 0x20>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_uart3>;
clock-frequency = <36864000>;
};
@@ -84,6 +92,8 @@
#address-cells = <1>;
#size-cells = <0>;
reg = <0x58400000 0x40>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_i2c0>;
clock-frequency = <100000>;
status = "disabled";
};
@@ -93,6 +103,8 @@
#address-cells = <1>;
#size-cells = <0>;
reg = <0x58480000 0x40>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_i2c1>;
clock-frequency = <100000>;
status = "disabled";
};
@@ -102,6 +114,8 @@
#address-cells = <1>;
#size-cells = <0>;
reg = <0x58500000 0x40>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_i2c2>;
clock-frequency = <100000>;
status = "disabled";
};
@@ -111,6 +125,8 @@
#address-cells = <1>;
#size-cells = <0>;
reg = <0x58580000 0x40>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_i2c3>;
clock-frequency = <100000>;
status = "disabled";
};
@@ -137,18 +153,30 @@
compatible = "socionext,uniphier-ehci", "generic-ehci";
status = "disabled";
reg = <0x5a800100 0x100>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_usb0>;
};
usb1: usb at 5a810100 {
compatible = "socionext,uniphier-ehci", "generic-ehci";
status = "disabled";
reg = <0x5a810100 0x100>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_usb1>;
};
usb2: usb at 5a820100 {
compatible = "socionext,uniphier-ehci", "generic-ehci";
status = "disabled";
reg = <0x5a820100 0x100>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_usb2>;
+ };
+
+ pinctrl: pinctrl at 5f801000 {
+ compatible = "socionext,ph1-ld4-pinctrl",
+ "syscon";
+ reg = <0x5f801000 0xe00>;
};
timer at 60000200 {
@@ -180,3 +208,5 @@
};
};
};
+
+/include/ "uniphier-pinctrl.dtsi"
diff --git a/arch/arm/dts/uniphier-ph1-pro4.dtsi b/arch/arm/dts/uniphier-ph1-pro4.dtsi
index 88fc48c..fa006fd 100644
--- a/arch/arm/dts/uniphier-ph1-pro4.dtsi
+++ b/arch/arm/dts/uniphier-ph1-pro4.dtsi
@@ -54,6 +54,8 @@
compatible = "socionext,uniphier-uart";
status = "disabled";
reg = <0x54006800 0x20>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_uart0>;
clock-frequency = <73728000>;
};
@@ -61,6 +63,8 @@
compatible = "socionext,uniphier-uart";
status = "disabled";
reg = <0x54006900 0x20>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_uart1>;
clock-frequency = <73728000>;
};
@@ -68,6 +72,8 @@
compatible = "socionext,uniphier-uart";
status = "disabled";
reg = <0x54006a00 0x20>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_uart2>;
clock-frequency = <73728000>;
};
@@ -75,6 +81,8 @@
compatible = "socionext,uniphier-uart";
status = "disabled";
reg = <0x54006b00 0x20>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_uart3>;
clock-frequency = <73728000>;
};
@@ -91,6 +99,8 @@
#address-cells = <1>;
#size-cells = <0>;
reg = <0x58780000 0x80>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_i2c0>;
clock-frequency = <100000>;
status = "disabled";
};
@@ -100,6 +110,8 @@
#address-cells = <1>;
#size-cells = <0>;
reg = <0x58781000 0x80>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_i2c1>;
clock-frequency = <100000>;
status = "disabled";
};
@@ -109,6 +121,8 @@
#address-cells = <1>;
#size-cells = <0>;
reg = <0x58782000 0x80>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_i2c2>;
clock-frequency = <100000>;
status = "disabled";
};
@@ -118,6 +132,8 @@
#address-cells = <1>;
#size-cells = <0>;
reg = <0x58783000 0x80>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_i2c3>;
clock-frequency = <100000>;
status = "disabled";
};
@@ -138,6 +154,8 @@
#address-cells = <1>;
#size-cells = <0>;
reg = <0x58786000 0x80>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_i2c6>;
clock-frequency = <400000>;
status = "ok";
};
@@ -170,24 +188,38 @@
compatible = "socionext,uniphier-ehci", "generic-ehci";
status = "disabled";
reg = <0x5a800100 0x100>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_usb2>;
};
usb3: usb at 5a810100 {
compatible = "socionext,uniphier-ehci", "generic-ehci";
status = "disabled";
reg = <0x5a810100 0x100>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_usb3>;
+ };
+
+ pinctrl: pinctrl at 5f801000 {
+ compatible = "socionext,ph1-pro4-pinctrl",
+ "syscon";
+ reg = <0x5f801000 0xe00>;
};
usb0: usb at 65a00000 {
compatible = "socionext,uniphier-xhci", "generic-xhci";
status = "disabled";
reg = <0x65a00000 0x100>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_usb0>;
};
usb1: usb at 65c00000 {
compatible = "socionext,uniphier-xhci", "generic-xhci";
status = "disabled";
reg = <0x65c00000 0x100>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_usb1>;
};
timer at 60000200 {
@@ -219,3 +251,5 @@
};
};
};
+
+/include/ "uniphier-pinctrl.dtsi"
diff --git a/arch/arm/dts/uniphier-ph1-sld8.dtsi b/arch/arm/dts/uniphier-ph1-sld8.dtsi
index f8d42bb..023d3f2 100644
--- a/arch/arm/dts/uniphier-ph1-sld8.dtsi
+++ b/arch/arm/dts/uniphier-ph1-sld8.dtsi
@@ -47,6 +47,8 @@
compatible = "socionext,uniphier-uart";
status = "disabled";
reg = <0x54006800 0x20>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_uart0>;
clock-frequency = <80000000>;
};
@@ -54,6 +56,8 @@
compatible = "socionext,uniphier-uart";
status = "disabled";
reg = <0x54006900 0x20>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_uart1>;
clock-frequency = <80000000>;
};
@@ -61,6 +65,8 @@
compatible = "socionext,uniphier-uart";
status = "disabled";
reg = <0x54006a00 0x20>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_uart2>;
clock-frequency = <80000000>;
};
@@ -68,6 +74,8 @@
compatible = "socionext,uniphier-uart";
status = "disabled";
reg = <0x54006b00 0x20>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_uart3>;
clock-frequency = <80000000>;
};
@@ -84,6 +92,8 @@
#address-cells = <1>;
#size-cells = <0>;
reg = <0x58400000 0x40>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_i2c0>;
clock-frequency = <100000>;
status = "disabled";
};
@@ -93,6 +103,8 @@
#address-cells = <1>;
#size-cells = <0>;
reg = <0x58480000 0x40>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_i2c1>;
clock-frequency = <100000>;
status = "disabled";
};
@@ -102,6 +114,8 @@
#address-cells = <1>;
#size-cells = <0>;
reg = <0x58500000 0x40>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_i2c2>;
clock-frequency = <100000>;
status = "disabled";
};
@@ -111,6 +125,8 @@
#address-cells = <1>;
#size-cells = <0>;
reg = <0x58580000 0x40>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_i2c3>;
clock-frequency = <100000>;
status = "disabled";
};
@@ -137,18 +153,30 @@
compatible = "socionext,uniphier-ehci", "generic-ehci";
status = "disabled";
reg = <0x5a800100 0x100>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_usb0>;
};
usb1: usb at 5a810100 {
compatible = "socionext,uniphier-ehci", "generic-ehci";
status = "disabled";
reg = <0x5a810100 0x100>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_usb1>;
};
usb2: usb at 5a820100 {
compatible = "socionext,uniphier-ehci", "generic-ehci";
status = "disabled";
reg = <0x5a820100 0x100>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_usb2>;
+ };
+
+ pinctrl: pinctrl at 5f801000 {
+ compatible = "socionext,ph1-sld8-pinctrl",
+ "syscon";
+ reg = <0x5f801000 0xe00>;
};
timer at 60000200 {
@@ -180,3 +208,5 @@
};
};
};
+
+/include/ "uniphier-pinctrl.dtsi"
diff --git a/arch/arm/dts/uniphier-pinctrl.dtsi b/arch/arm/dts/uniphier-pinctrl.dtsi
new file mode 100644
index 0000000..8f9aa01
--- /dev/null
+++ b/arch/arm/dts/uniphier-pinctrl.dtsi
@@ -0,0 +1,80 @@
+/*
+ * Device Tree Source for UniPhier SoCs pinctrl settings
+ *
+ * Copyright (C) 2015 Masahiro Yamada <yamada.masahiro@socionext.com>
+ *
+ * SPDX-License-Identifier: GPL-2.0+ X11
+ */
+
+&pinctrl {
+ pinctrl_i2c0: i2c0_grp {
+ groups = "i2c0";
+ function = "i2c0";
+ pull-down;
+ };
+
+ pinctrl_i2c1: i2c1_grp {
+ groups = "i2c1";
+ function = "i2c1";
+ };
+
+ pinctrl_i2c2: i2c2_grp {
+ groups = "i2c2";
+ function = "i2c2";
+ };
+
+ pinctrl_i2c3: i2c3_grp {
+ groups = "i2c3";
+ function = "i2c3";
+ };
+
+ pinctrl_i2c5: i2c5_grp {
+ groups = "i2c5";
+ function = "i2c5";
+ };
+
+ pinctrl_i2c6: i2c6_grp {
+ groups = "i2c6";
+ function = "i2c6";
+ };
+
+ pinctrl_uart0: uart0_grp {
+ groups = "uart0";
+ function = "uart0";
+ };
+
+ pinctrl_uart1: uart1_grp {
+ groups = "uart1";
+ function = "uart1";
+ };
+
+ pinctrl_uart2: uart2_grp {
+ groups = "uart2";
+ function = "uart2";
+ };
+
+ pinctrl_uart3: uart3_grp {
+ groups = "uart3";
+ function = "uart3";
+ };
+
+ pinctrl_usb0: usb0_grp {
+ groups = "usb0";
+ function = "usb0";
+ };
+
+ pinctrl_usb1: usb1_grp {
+ groups = "usb1";
+ function = "usb1";
+ };
+
+ pinctrl_usb2: usb2_grp {
+ groups = "usb2";
+ function = "usb2";
+ };
+
+ pinctrl_usb3: usb3_grp {
+ groups = "usb3";
+ function = "usb3";
+ };
+};
--
1.9.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* [U-Boot] [RFC PATCH 7/7] ARM: select pinctrl drivers from Kconfig
2015-07-15 8:16 [U-Boot] [RFC PATCH 0/7] DM: pinctrl: Another implemtation of pinctrl framework Masahiro Yamada
` (5 preceding siblings ...)
2015-07-15 8:16 ` [U-Boot] [RFC PATCH 6/7] ARM: dts: UniPhier: add nodes for pinctrl devices and pin configs Masahiro Yamada
@ 2015-07-15 8:16 ` Masahiro Yamada
6 siblings, 0 replies; 18+ messages in thread
From: Masahiro Yamada @ 2015-07-15 8:16 UTC (permalink / raw)
To: u-boot
The pinctrl driver is tightly integrated with the each SoC,
so it is reasonable to select it from Kconfig.
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---
arch/arm/mach-uniphier/Kconfig | 3 +++
1 file changed, 3 insertions(+)
diff --git a/arch/arm/mach-uniphier/Kconfig b/arch/arm/mach-uniphier/Kconfig
index 3820bca..57e1c42 100644
--- a/arch/arm/mach-uniphier/Kconfig
+++ b/arch/arm/mach-uniphier/Kconfig
@@ -17,6 +17,7 @@ config MACH_PH1_SLD3
config MACH_PH1_LD4
bool "PH1-LD4"
+ select PINCTRL_UNIPHIER_PH1_LD4
config MACH_PH1_PRO4TV
bool "PH1-Pro4TV"
@@ -25,9 +26,11 @@ config MACH_PH1_PRO4TV
config MACH_PH1_PRO4
bool "PH1-Pro4"
select UNIPHIER_SMP
+ select PINCTRL_UNIPHIER_PH1_PRO4
config MACH_PH1_SLD8
bool "PH1-sLD8"
+ select PINCTRL_UNIPHIER_PH1_SLD8
config MACH_PH1_PRO5
bool "PH1-Pro5"
--
1.9.1
^ permalink raw reply related [flat|nested] 18+ messages in thread