* [PATCH v2] pinctrl: add a generic pin config interface
@ 2011-11-11 8:31 Linus Walleij
2011-11-11 11:26 ` Thomas Abraham
2011-11-14 19:44 ` Stephen Warren
0 siblings, 2 replies; 12+ messages in thread
From: Linus Walleij @ 2011-11-11 8:31 UTC (permalink / raw)
To: linux-kernel, Stephen Warren
Cc: Grant Likely, Barry Song, Shawn Guo, Linus Walleij
From: Linus Walleij <linus.walleij@linaro.org>
This add per-pin and per-group pin config interfaces for biasing,
driving and other such electronic properties. The intention is
clearly to enumerate all things you can do with pins, hoping that
these are enumerable.
ChangeLog v1->v2:
- Clear split of terminology: we now have pin controllers, and
those may support two interfaces using vtables: pin
multiplexing and pin configuration.
- Break out pin configuration to its own C file, controllers may
implement only config without mux, and vice versa, so keep each
sub-functionality of pin controllers separate. Introduce
CONFIG_PINCONF in Kconfig.
- Implement some core logic around pin configuration in the
pinconf.c file.
- Remove UNKNOWN config states, these were just surplus baggage.
- Remove FLOAT config state - HIGH_IMPEDANCE should be enough for
everyone.
- PIN_CONFIG_POWER_SOURCE added to handle switching the power
supply for the pin logic between different sources
- Explicit DISABLE config enums to turn schmitt-trigger,
wakeup etc OFF.
- Update documentation to reflect all the recent reasoning.
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
Documentation/pinctrl.txt | 93 ++++++++++-
drivers/pinctrl/Kconfig | 5 +-
drivers/pinctrl/Makefile | 1 +
drivers/pinctrl/core.c | 18 ++
drivers/pinctrl/core.h | 10 ++
drivers/pinctrl/pinconf.c | 332 +++++++++++++++++++++++++++++++++++++++
drivers/pinctrl/pinconf.h | 34 ++++
include/linux/pinctrl/pinconf.h | 193 +++++++++++++++++++++++
include/linux/pinctrl/pinctrl.h | 10 +-
9 files changed, 685 insertions(+), 11 deletions(-)
create mode 100644 drivers/pinctrl/pinconf.c
create mode 100644 drivers/pinctrl/pinconf.h
create mode 100644 include/linux/pinctrl/pinconf.h
diff --git a/Documentation/pinctrl.txt b/Documentation/pinctrl.txt
index ffef900..ad04c5e 100644
--- a/Documentation/pinctrl.txt
+++ b/Documentation/pinctrl.txt
@@ -7,12 +7,9 @@ This subsystem deals with:
- Multiplexing of pins, pads, fingers (etc) see below for details
-The intention is to also deal with:
-
-- Software-controlled biasing and driving mode specific pins, such as
- pull-up/down, open drain etc, load capacitance configuration when controlled
- by software, etc.
-
+- Configuration of pins, pads, fingers (etc), such as software-controlled
+ biasing and driving mode specific pins, such as pull-up/down, open drain,
+ load capacitance etc.
Top-level interface
===================
@@ -88,6 +85,11 @@ int __init foo_probe(void)
pr_err("could not register foo pin driver\n");
}
+To enable the pinctrl subsystem and the subgroups for PINMUX and PINCONF and
+selected drivers, you need to select them from your machine's Kconfig entry,
+since these are so tightly integrated with the machines they are used on.
+See for example arch/arm/mach-u300/Kconfig for an example.
+
Pins usually have fancier names than this. You can find these in the dataheet
for your chip. Notice that the core pinctrl.h file provides a fancy macro
called PINCTRL_PIN() to create the struct entries. As you can see I enumerated
@@ -193,6 +195,85 @@ structure, for example specific register ranges associated with each group
and so on.
+Pin configuration
+=================
+
+Pins can sometimes be software-configured in an various ways, mostly related
+to their electronic properties when used as inputs or outputs. For example you
+may be able to make an output pin high impedance, or "tristate" meaning it is
+effectively disconnected. You may be able to connect an input pin to VDD or GND
+using a certain resistor value - pull up and pull down - so that the pin has a
+stable value when nothing is driving the rail it is connected to, or when it's
+unconnected.
+
+The pin control system supports an interface partly abstracting these
+properties while leaving the details to the pin control driver. We assume that
+the things a controller may want configure are enumerable, and thus the
+parameters such as PIN_CONFIG_BIAS_PULL_UP are defined by the core, whereas
+the arguments to the parameter may need to be on a custom format only
+understandable by the driver. However we strive to use SI-derived entities for
+these where applicable, which means the core may step in and do sematic
+analysis of the passed values in select cases.
+
+For example, a driver can do this:
+
+ret = pin_config(128, PIN_CONFIG_BIAS_PULL_UP, 100000);
+
+To pull up a pin to VDD with a 100KOhm resistor. The driver implements
+callbacks for changing pin configuration in the pin controller ops like this:
+
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/pinctrl/pinconf.h>
+
+int foo_pin_config (struct pinctrl_dev *pctldev,
+ const struct pin_config *conf,
+ unsigned pin,
+ enum pin_config_param param,
+ unsigned long data)
+{
+ switch (param) {
+ case PIN_CONFIG_BIAS_PULL_UP:
+ ...
+}
+
+int foo_pin_config_group (struct pinctrl_dev *pctldev,
+ unsigned selector,
+ enum pin_config_param param,
+ unsigned long data)
+{
+ ...
+}
+
+static struct pinconf_ops foo_pconf_ops = {
+ pin_config = foo_pin_config,
+ pin_config_group = foo_pin_config_group,
+};
+
+/* Pin config operations are handled by some pin controller */
+static struct pinctrl_desc foo_desc = {
+ ...
+ .confops = &foo_pconf_ops,
+};
+
+The pin config core keeps track of the pin state, since not all hardware
+support reading this out in a sane way. The current state is the logical sum
+of all applied configurations and can be inspected in debugfs. There is
+an optional hook in the pinconf_ops, .pin_get_initial_config() which can
+read out the initial state of each pin as it is registered, if there is need
+for this on your system. This way the pin config core can keep track of the
+state of each pin at any time. Notice that the .pin_config() callback pass
+the current state of the pin as an argument to the driver so that it can
+exploit the current state of each pin if need be.
+
+Since some controllers have special logic for handling entire groups of pins
+they can exploit the special whole-group pin control function. The
+pin_config_group() callback is allowed to return the error code -EAGAIN,
+for groups it does not want to handle, or if it just wants to do some
+group-level handling and then fall through to iterate over all pins, in which
+case each individual pin will be treated by separate pin_config() calls as
+well.
+
+
Interaction with the GPIO subsystem
===================================
diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index e17e2f8..2c9be64 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -12,7 +12,10 @@ menu "Pin controllers"
depends on PINCTRL
config PINMUX
- bool "Support pinmux controllers"
+ bool "Support pin multiplexing controllers"
+
+config PINCONF
+ bool "Support pin configuration controllers"
config DEBUG_PINCTRL
bool "Debug PINCTRL calls"
diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
index bdc548a..dfc145b 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -4,5 +4,6 @@ ccflags-$(CONFIG_DEBUG_PINMUX) += -DDEBUG
obj-$(CONFIG_PINCTRL) += core.o
obj-$(CONFIG_PINMUX) += pinmux.o
+obj-$(CONFIG_PINCONF) += pinconf.o
obj-$(CONFIG_PINMUX_SIRF) += pinmux-sirf.o
obj-$(CONFIG_PINMUX_U300) += pinmux-u300.o
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index 9970590..bfb2b89 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -28,6 +28,7 @@
#include <linux/pinctrl/machine.h>
#include "core.h"
#include "pinmux.h"
+#include "pinconf.h"
/* Global list of pin control devices */
static DEFINE_MUTEX(pinctrldev_list_mutex);
@@ -149,6 +150,7 @@ static int pinctrl_register_one_pin(struct pinctrl_dev *pctldev,
unsigned number, const char *name)
{
struct pin_desc *pindesc;
+ int ret;
pindesc = pin_desc_get(pctldev, number);
if (pindesc != NULL) {
@@ -160,6 +162,7 @@ static int pinctrl_register_one_pin(struct pinctrl_dev *pctldev,
pindesc = kzalloc(sizeof(*pindesc), GFP_KERNEL);
if (pindesc == NULL)
return -ENOMEM;
+
spin_lock_init(&pindesc->lock);
/* Set owner */
@@ -168,6 +171,10 @@ static int pinctrl_register_one_pin(struct pinctrl_dev *pctldev,
/* Copy basic pin info */
pindesc->name = name;
+ ret = pinconf_init_config(pctldev, pindesc, number);
+ if (ret)
+ return ret;
+
spin_lock(&pctldev->pin_desc_tree_lock);
radix_tree_insert(&pctldev->pin_desc_tree, number, pindesc);
spin_unlock(&pctldev->pin_desc_tree_lock);
@@ -315,6 +322,7 @@ int pinctrl_get_group_selector(struct pinctrl_dev *pctldev,
return -EINVAL;
}
+
#ifdef CONFIG_DEBUG_FS
static int pinctrl_pins_show(struct seq_file *s, void *what)
@@ -545,6 +553,16 @@ struct pinctrl_dev *pinctrl_register(struct pinctrl_desc *pctldesc,
}
}
+ /* If we're implementing pinconfig, check the ops for sanity */
+ if (pctldesc->confops) {
+ ret = pinconf_check_ops(pctldesc->confops);
+ if (ret) {
+ pr_err("%s pin config ops lacks necessary functions\n",
+ pctldesc->name);
+ return NULL;
+ }
+ }
+
pctldev = kzalloc(sizeof(struct pinctrl_dev), GFP_KERNEL);
if (pctldev == NULL)
return NULL;
diff --git a/drivers/pinctrl/core.h b/drivers/pinctrl/core.h
index dcc6d68..012026e 100644
--- a/drivers/pinctrl/core.h
+++ b/drivers/pinctrl/core.h
@@ -9,6 +9,10 @@
* License terms: GNU General Public License (GPL) version 2
*/
+#include <linux/pinctrl/pinconf.h>
+
+struct pinctrl_gpio_range;
+
/**
* struct pinctrl_dev - pin control class device
* @node: node to include this pin controller in the global pin controller list
@@ -52,6 +56,8 @@ struct pinctrl_dev {
* @mux_requested: whether the pin is already requested by pinmux or not
* @mux_function: a named muxing function for the pin that will be passed to
* subdrivers and shown in debugfs etc
+ * @config_lock: a lock to protect the pin configuration portions
+ * @pin_configs: a list of configuration settings for this pin
*/
struct pin_desc {
struct pinctrl_dev *pctldev;
@@ -61,6 +67,10 @@ struct pin_desc {
#ifdef CONFIG_PINMUX
const char *mux_function;
#endif
+#ifdef CONFIG_PINCONF
+ struct mutex config_lock;
+ struct pin_config config;
+#endif
};
struct pinctrl_dev *get_pinctrl_dev_from_dev(struct device *dev,
diff --git a/drivers/pinctrl/pinconf.c b/drivers/pinctrl/pinconf.c
new file mode 100644
index 0000000..324626d
--- /dev/null
+++ b/drivers/pinctrl/pinconf.c
@@ -0,0 +1,332 @@
+/*
+ * Core driver for the pin config portions of the pin control subsystem
+ *
+ * Copyright (C) 2011 ST-Ericsson SA
+ * Written on behalf of Linaro for ST-Ericsson
+ * Based on bits of regulator core, gpio core and clk core
+ *
+ * Author: Linus Walleij <linus.walleij@linaro.org>
+ *
+ * License terms: GNU General Public License (GPL) version 2
+ */
+#define pr_fmt(fmt) "pinconfig core: " fmt
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/device.h>
+#include <linux/slab.h>
+#include <linux/err.h>
+#include <linux/list.h>
+#include <linux/mutex.h>
+#include <linux/debugfs.h>
+#include <linux/seq_file.h>
+#include <linux/pinctrl/machine.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/pinctrl/pinconf.h>
+#include "core.h"
+
+static void pinconf_update_state(struct pin_config *conf,
+ enum pin_config_param param,
+ unsigned long data)
+{
+ switch (param) {
+ case PIN_CONFIG_BIAS_DISABLE:
+ case PIN_CONFIG_BIAS_HIGH_IMPEDANCE:
+ case PIN_CONFIG_BIAS_PULL_UP:
+ case PIN_CONFIG_BIAS_PULL_DOWN:
+ case PIN_CONFIG_BIAS_HIGH:
+ case PIN_CONFIG_BIAS_GROUND:
+ /* These are mutually exclusive */
+ conf->bias_param = param;
+ conf->bias_data = data;
+ break;
+ case PIN_CONFIG_DRIVE_PUSH_PULL:
+ case PIN_CONFIG_DRIVE_OPEN_DRAIN:
+ case PIN_CONFIG_DRIVE_OPEN_SOURCE:
+ case PIN_CONFIG_DRIVE_OFF:
+ conf->drive_param = param;
+ conf->drive_data = data;
+ break;
+ case PIN_CONFIG_INPUT_SCHMITT:
+ conf->schmitt = true;
+ break;
+ case PIN_CONFIG_INPUT_SCHMITT_OFF:
+ conf->schmitt = false;
+ break;
+ case PIN_CONFIG_SLEW_RATE_RISING:
+ conf->slewrate_rising = data;
+ break;
+ case PIN_CONFIG_SLEW_RATE_FALLING:
+ conf->slewrate_falling = data;
+ break;
+ case PIN_CONFIG_LOAD_CAPACITANCE:
+ conf->load_capacitance = data;
+ break;
+ case PIN_CONFIG_POWER_SOURCE:
+ conf->power_source = data;
+ break;
+ case PIN_CONFIG_LOW_POWER_MODE:
+ conf->low_power = true;
+ break;
+ case PIN_CONFIG_NORMAL_POWER_MODE:
+ conf->low_power = false;
+ break;
+ case PIN_CONFIG_WAKEUP_ENABLE:
+ conf->wakeup = true;
+ break;
+ case PIN_CONFIG_WAKEUP_DISABLE:
+ conf->wakeup = false;
+ break;
+ default:
+ /* TODO: Error? Custom? */
+ break;
+ }
+}
+
+int pin_config(struct pinctrl_dev *pctldev, int pin,
+ enum pin_config_param param, unsigned long data)
+{
+ const struct pinconf_ops *ops = pctldev->desc->confops;
+ struct pin_desc *desc;
+ struct pin_config *conf;
+ int ret;
+
+ desc = pin_desc_get(pctldev, pin);
+ if (desc == NULL) {
+ dev_err(&pctldev->dev, "tried to configure unregistered pin\n");
+ return -EINVAL;
+ }
+
+ conf = &desc->config;
+
+ if (!ops || !ops->pin_config) {
+ dev_err(&pctldev->dev, "cannot configure pin, missing "
+ "config function in driver\n");
+ return -EINVAL;
+ }
+
+ ret = ops->pin_config(pctldev, conf, pin, param, data);
+ if (ret) {
+ dev_err(&pctldev->dev,
+ "unable to set pin configuration on pin %d\n", pin);
+ return ret;
+ }
+
+ pinconf_update_state(conf, param, data);
+
+ return 0;
+}
+
+int pin_config_group(struct pinctrl_dev *pctldev, const char *pin_group,
+ enum pin_config_param param, unsigned long data)
+{
+ const struct pinctrl_ops *pctlops = pctldev->desc->pctlops;
+ const struct pinconf_ops *ops = pctldev->desc->confops;
+ int selector;
+ const unsigned *pins;
+ unsigned num_pins;
+ int ret;
+ int i;
+
+ if (!ops || (!ops->pin_config_group && !ops->pin_config)) {
+ dev_err(&pctldev->dev, "cannot configure pin group, missing "
+ "config function in driver\n");
+ return -EINVAL;
+ }
+
+ selector = pinctrl_get_group_selector(pctldev, pin_group);
+ if (selector < 0)
+ return selector;
+
+ ret = pctlops->get_group_pins(pctldev, selector, &pins, &num_pins);
+ if (ret) {
+ dev_err(&pctldev->dev, "cannot configure pin group, error "
+ "getting pins\n");
+ return ret;
+ }
+
+ /*
+ * If the pin controller supports handling entire groups we use that
+ * capability.
+ */
+ if (ops->pin_config_group) {
+ ret = ops->pin_config_group(pctldev, selector, param, data);
+
+ /* Success, update per-pin state */
+ if (ret == 0) {
+ for (i = 0; i < num_pins; i++) {
+ struct pin_desc *desc;
+
+ desc = pin_desc_get(pctldev, pins[i]);
+ if (desc == NULL) {
+ dev_err(&pctldev->dev, "error updating state\n");
+ return -EINVAL;
+ }
+ pinconf_update_state(&desc->config, param, data);
+ }
+ }
+ /*
+ * If the pin controller prefer that a certain group be handled
+ * pin-by-pin as well, it returns -EAGAIN.
+ */
+ if (ret != -EAGAIN)
+ return ret;
+ }
+
+ /*
+ * If the controller cannot handle entire groups, we configure each pin
+ * individually.
+ */
+ for (i = 0; i < num_pins; i++) {
+ ret = pin_config(pctldev, pins[i], param, data);
+ if (ret < 0)
+ return ret;
+ }
+
+ return 0;
+}
+
+int pinconf_check_ops(const struct pinconf_ops *ops)
+{
+ /* We have to be able to config the pins in SOME way */
+ if (!ops->pin_config_group && !ops->pin_config)
+ return -EINVAL;
+ return 0;
+}
+
+int pinconf_init_config(struct pinctrl_dev *pctldev, struct pin_desc *desc,
+ unsigned number)
+{
+ const struct pinconf_ops *ops = pctldev->desc->confops;
+ struct pin_config *conf = &desc->config;
+ int ret;
+
+ /* Retrieve initial pin config if the driver supports it */
+ if (ops->pin_get_initial_config) {
+ ret = ops->pin_get_initial_config(pctldev, conf, number);
+ if (!ret)
+ dev_err(&pctldev->dev, "unable to get initial config "
+ "for pin %d (%s)\n", number, desc->name);
+ return ret;
+ } else {
+ /* Put them all in assumed states then */
+ conf->bias_param = PIN_CONFIG_BIAS_DISABLE;
+ conf->drive_param = PIN_CONFIG_DRIVE_OFF;
+ }
+
+ return 0;
+}
+
+#ifdef CONFIG_DEBUG_FS
+
+static void pinconf_dump_pin(struct seq_file *s, struct pin_config *conf)
+{
+ switch (conf->bias_param) {
+ case PIN_CONFIG_BIAS_DISABLE:
+ seq_puts(s, "bias disabled ");
+ break;
+ case PIN_CONFIG_BIAS_HIGH_IMPEDANCE:
+ seq_puts(s, "bias high impedance ");
+ break;
+ case PIN_CONFIG_BIAS_PULL_UP:
+ seq_puts(s, "bias pull up ");
+ break;
+ case PIN_CONFIG_BIAS_PULL_DOWN:
+ seq_puts(s, "bias pull down ");
+ break;
+ case PIN_CONFIG_BIAS_HIGH:
+ seq_puts(s, "bias high ");
+ break;
+ case PIN_CONFIG_BIAS_GROUND:
+ seq_puts(s, "bias ground ");
+ break;
+ default:
+ break;
+ }
+
+ switch (conf->drive_param) {
+ case PIN_CONFIG_DRIVE_PUSH_PULL:
+ seq_puts(s, "drive push/pull ");
+ break;
+ case PIN_CONFIG_DRIVE_OPEN_DRAIN:
+ seq_puts(s, "drive open drain ");
+ break;
+ case PIN_CONFIG_DRIVE_OPEN_SOURCE:
+ seq_puts(s, "drive open source ");
+ break;
+ case PIN_CONFIG_DRIVE_OFF:
+ seq_puts(s, "drive off ");
+ break;
+ default:
+ break;
+ }
+
+ if (conf->schmitt)
+ seq_puts(s, "schmitt trigger ");
+ if (conf->slewrate_rising)
+ seq_printf(s, "slewrate rising %lu ns ",
+ conf->slewrate_rising);
+ if (conf->slewrate_falling)
+ seq_printf(s, "slewrate falling %lu ns ",
+ conf->slewrate_falling);
+ if (conf->load_capacitance)
+ seq_printf(s, "load capacitance %lu pF ",
+ conf->load_capacitance);
+ /* Don't print power source, we don't know about the argument */
+ if (conf->low_power)
+ seq_puts(s, "low power mode ");
+}
+
+static int pinconf_pins_show(struct seq_file *s, void *what)
+{
+ struct pinctrl_dev *pctldev = s->private;
+ unsigned pin;
+
+ seq_puts(s, "Pin config settings per pin\n");
+ seq_puts(s, "Format: pin (name): pinmux setting array\n");
+
+ /* The highest pin number need to be included in the loop, thus <= */
+ for (pin = 0; pin <= pctldev->desc->maxpin; pin++) {
+ struct pin_desc *desc;
+
+ desc = pin_desc_get(pctldev, pin);
+ /* Pin space may be sparse */
+ if (desc == NULL)
+ continue;
+
+ seq_printf(s, "pin %d (%s): ", pin,
+ desc->name ? desc->name : "unnamed");
+
+ mutex_lock(&desc->config_lock);
+ pinconf_dump_pin(s, &desc->config);
+ mutex_unlock(&desc->config_lock);
+
+ seq_printf(s, "\n");
+ }
+
+ return 0;
+}
+
+static int pinconf_pins_open(struct inode *inode, struct file *file)
+{
+ return single_open(file, pinconf_pins_show, inode->i_private);
+}
+
+static const struct file_operations pinconf_pins_ops = {
+ .open = pinconf_pins_open,
+ .read = seq_read,
+ .llseek = seq_lseek,
+ .release = single_release,
+};
+
+void pinconf_init_device_debugfs(struct dentry *devroot,
+ struct pinctrl_dev *pctldev)
+{
+ debugfs_create_file("pinconf-pins", S_IFREG | S_IRUGO,
+ devroot, pctldev, &pinconf_pins_ops);
+}
+
+#else
+
+#endif
diff --git a/drivers/pinctrl/pinconf.h b/drivers/pinctrl/pinconf.h
new file mode 100644
index 0000000..a7b57b1
--- /dev/null
+++ b/drivers/pinctrl/pinconf.h
@@ -0,0 +1,34 @@
+/*
+ * Internal interface between the core pin control system and the
+ * pin config portions
+ *
+ * Copyright (C) 2011 ST-Ericsson SA
+ * Written on behalf of Linaro for ST-Ericsson
+ * Based on bits of regulator core, gpio core and clk core
+ *
+ * Author: Linus Walleij <linus.walleij@linaro.org>
+ *
+ * License terms: GNU General Public License (GPL) version 2
+ */
+
+#ifdef CONFIG_PINCONF
+
+int pinconf_check_ops(const struct pinconf_ops *ops);
+int pinconf_init_config(struct pinctrl_dev *pctldev, struct pin_desc *desc,
+ unsigned number);
+
+#else
+
+static inline int pinconf_check_ops(const struct pinconf_ops *ops)
+{
+ return 0;
+}
+
+static inline int pinconf_init_config(struct pinctrl_dev *pctldev,
+ struct pin_desc *desc,
+ unsigned number)
+{
+ return 0;
+}
+
+#endif
diff --git a/include/linux/pinctrl/pinconf.h b/include/linux/pinctrl/pinconf.h
new file mode 100644
index 0000000..6b5163d
--- /dev/null
+++ b/include/linux/pinctrl/pinconf.h
@@ -0,0 +1,193 @@
+/*
+ * Interface the pinconfig portions of the pinctrl subsystem
+ *
+ * Copyright (C) 2011 ST-Ericsson SA
+ * Written on behalf of Linaro for ST-Ericsson
+ * This interface is used in the core to keep track of pins.
+ *
+ * Author: Linus Walleij <linus.walleij@linaro.org>
+ *
+ * License terms: GNU General Public License (GPL) version 2
+ */
+#ifndef __LINUX_PINCTRL_PINCONF_H
+#define __LINUX_PINCTRL_PINCONF_H
+
+/**
+ * enum pin_config_param - possible pin configuration parameters
+ * @PIN_CONFIG_BIAS_DISABLE: disable any pin bias on the pin, a
+ * transition from say pull-up to pull-down implies that you disable
+ * pull-up in the process, this setting disables all biasing
+ * @PIN_CONFIG_BIAS_HIGH_IMPEDANCE: the pin will be set to a high impedance
+ * mode, also know as "third-state" (tristate) or "high-Z" or "floating".
+ * On output pins this effectively disconnects the pin, which is useful
+ * if for example some other pin is going to drive the signal connected
+ * to it for a while. Pins used for input are usually always high
+ * impedance.
+ * @PIN_CONFIG_BIAS_PULL_UP: the pin will be pulled up (usually with high
+ * impedance to VDD), if the controller supports specifying a certain
+ * pull-up resistance, this is given as an argument (in Ohms) when
+ * setting this parameter
+ * @PIN_CONFIG_BIAS_PULL_DOWN: the pin will be pulled down (usually with high
+ * impedance to GROUND), if the controller supports specifying a certain
+ * pull-down resistance, this is given as an argument (in Ohms) when
+ * setting this parameter
+ * @PIN_CONFIG_BIAS_HIGH: the pin will be wired high, connected to VDD
+ * @PIN_CONFIG_BIAS_GROUND: the pin will be grounded, connected to GROUND
+ * @PIN_CONFIG_DRIVE_PUSH_PULL: the pin will be driven actively high and
+ * low, this is the most typical case and is typically achieved with two
+ * active transistors on the output. If the pin can support different
+ * drive strengths for push/pull, the strength is given on a custom format
+ * as argument when setting pins to this mode
+ * @PIN_CONFIG_DRIVE_OPEN_DRAIN: the pin will be driven with open drain (open
+ * collector) which means it is usually wired with other output ports
+ * which are then pulled up with an external resistor. If the pin can
+ * support different drive strengths for the open drain pin, the strength
+ * is given on a custom format as argument when setting pins to this mode
+ * @PIN_CONFIG_DRIVE_OPEN_SOURCE: the pin will be driven with open drain
+ * (open emitter) which is the same as open drain mutatis mutandis but
+ * pulled to ground. If the pin can support different drive strengths for
+ * the open drain pin, the strength is given on a custom format as
+ * argument when setting pins to this mode
+ * @PIN_CONFIG_DRIVE_OFF: the pin is set to inactive drive mode, off
+ * @PIN_CONFIG_INPUT_SCHMITT: this will configure an input pin to run in
+ * schmitt-trigger mode. If the schmitt-trigger has adjustable hysteresis,
+ * the threshold value is given on a custom format as argument when
+ * setting pins to this mode
+ * @PIN_CONFIG_INPUT_SCHMITT_OFF: disables schmitt-trigger mode
+ * @PIN_CONFIG_SLEW_RATE_RISING: this will configure the slew rate for rising
+ * signals on the pin. The argument gives the rise time in nanoseconds.
+ * You may want to adjust slew rates so that signal edges don't get too
+ * steep, causing disturbances in surrounding electronics for example.
+ * @PIN_CONFIG_SLEW_RATE_FALLING: this will configure the slew rate for falling
+ * signals on the pin. The argument gives the fall time in nanoseconds
+ * @PIN_CONFIG_LOAD_CAPACITANCE: some pins have inductive characteristics and
+ * will deform waveforms when signals are transmitted on them, by
+ * applying a load capacitance, the waveform can be rectified. The
+ * argument gives the load capacitance in picofarad (pF)
+ * @PIN_CONFIG_POWER_SOURCE: if the pin can select between different power
+ * supplies, the argument to this parameter (on a custom format) tells
+ * the driver which alternative power source to use
+ * @PIN_CONFIG_LOW_POWER_MODE: this will configure the pin for low power
+ * operation
+ * @PIN_CONFIG_NORMAL_POWER_MODE: returns pin from low power mode
+ * @PIN_CONFIG_WAKEUP_ENABLE: this will configure an input pin such that if a
+ * signal transition arrives at the pin when the pin controller/system
+ * is sleeping, it will wake up the system
+ * @PIN_CONFIG_WAKEUP_DISABLE: will disable a pin from the wakeup enable state
+ * set by the previous parameter
+ * @PIN_CONFIG_END: this is the last enumerator for pin configurations, if
+ * you need to pass in custom configurations to the pin controller, use
+ * PIN_CONFIG_END+1 as the base offset
+ */
+enum pin_config_param {
+ PIN_CONFIG_BIAS_DISABLE,
+ PIN_CONFIG_BIAS_HIGH_IMPEDANCE,
+ PIN_CONFIG_BIAS_PULL_UP,
+ PIN_CONFIG_BIAS_PULL_DOWN,
+ PIN_CONFIG_BIAS_HIGH,
+ PIN_CONFIG_BIAS_GROUND,
+ PIN_CONFIG_DRIVE_PUSH_PULL,
+ PIN_CONFIG_DRIVE_OPEN_DRAIN,
+ PIN_CONFIG_DRIVE_OPEN_SOURCE,
+ PIN_CONFIG_DRIVE_OFF,
+ PIN_CONFIG_INPUT_SCHMITT,
+ PIN_CONFIG_INPUT_SCHMITT_OFF,
+ PIN_CONFIG_SLEW_RATE_RISING,
+ PIN_CONFIG_SLEW_RATE_FALLING,
+ PIN_CONFIG_LOAD_CAPACITANCE,
+ PIN_CONFIG_POWER_SOURCE,
+ PIN_CONFIG_LOW_POWER_MODE,
+ PIN_CONFIG_NORMAL_POWER_MODE,
+ PIN_CONFIG_WAKEUP_ENABLE,
+ PIN_CONFIG_WAKEUP_DISABLE,
+ PIN_CONFIG_END,
+};
+
+/**
+ * struct pin_config - configuration state holder for a single config of a pin
+ * @bias_param: bias configuration parameter
+ * @bias_data: configuration data for the parameter
+ * @schmitt: input is in schmitt-trigger mode
+ * @slewrate_rising: rising edge slew rate
+ * @slewrate_falling: falling edge slew rate
+ * @load_capacitance: load capacitane of the pin
+ * @power_source: selected power source for the pin
+ * @low_power: pin is in low power mode
+ * @wakeup: pin will wake up system when sleeping
+ *
+ * This holds one configuration item for one pin, a pin may have several such
+ * configurations since it may be configured for several non-conflicting modes
+ * simultaneously.
+ */
+struct pin_config {
+ enum pin_config_param bias_param;
+ unsigned long bias_data;
+ enum pin_config_param drive_param;
+ unsigned long drive_data;
+ bool schmitt;
+ unsigned long slewrate_rising;
+ unsigned long slewrate_falling;
+ unsigned long load_capacitance;
+ unsigned long power_source;
+ bool low_power;
+ bool wakeup;
+};
+
+#ifdef CONFIG_PINCONF
+
+struct pinctrl_dev;
+
+/**
+ * struct pinconf_ops - pin config operations, to be implemented by
+ * pin configuration capable drivers.
+ * @pin_get_initial_config: called to get the initial config of each pin
+ * if you cannot read out the configuration of your pins at startup, just
+ * leave this as NULL
+ * @pin_config: configure an individual pin
+ * @pin_config_group: configure all pins in a group
+ * @pin_config_dbg_show: optional debugfs display hook that will provide
+ * per-device info for a certain pin in debugfs
+ */
+struct pinconf_ops {
+ int (*pin_get_initial_config) (struct pinctrl_dev *pctldev,
+ struct pin_config *conf,
+ unsigned pin);
+ int (*pin_config) (struct pinctrl_dev *pctldev,
+ const struct pin_config *conf,
+ unsigned pin,
+ enum pin_config_param param,
+ unsigned long data);
+ int (*pin_config_group) (struct pinctrl_dev *pctldev,
+ unsigned selector,
+ enum pin_config_param param,
+ unsigned long data);
+ void (*pin_config_dbg_show) (struct pinctrl_dev *pctldev,
+ struct seq_file *s,
+ unsigned offset);
+};
+
+
+extern int pin_config(struct pinctrl_dev *pctldev, int pin,
+ enum pin_config_param param, unsigned long data);
+extern int pin_config_group(struct pinctrl_dev *pctldev, const char *pin_group,
+ enum pin_config_param param, unsigned long data);
+
+#else
+
+static inline int pin_config(struct pinctrl_dev *pctldev, int pin,
+ enum pin_config_param param, unsigned long data)
+{
+ return 0;
+}
+
+static inline int pin_config_group(struct pinctrl_dev *pctldev,
+ const char *pin_group,
+ enum pin_config_param param,
+ unsigned long data)
+{
+ return 0;
+}
+
+#endif
+
+#endif /* __LINUX_PINCTRL_PINCONF_H */
diff --git a/include/linux/pinctrl/pinctrl.h b/include/linux/pinctrl/pinctrl.h
index 04c0110..d366540 100644
--- a/include/linux/pinctrl/pinctrl.h
+++ b/include/linux/pinctrl/pinctrl.h
@@ -21,6 +21,7 @@
struct pinctrl_dev;
struct pinmux_ops;
+struct pinconf_ops;
struct gpio_chip;
/**
@@ -95,7 +96,9 @@ struct pinctrl_ops {
* but may be equal to npins if you have no holes in the pin range.
* @pctlops: pin control operation vtable, to support global concepts like
* grouping of pins, this is optional.
- * @pmxops: pinmux operation vtable, if you support pinmuxing in your driver
+ * @pmxops: pinmux operations vtable, if you support pinmuxing in your driver
+ * @confops: pin config operations vtable, if you support pin configuration in
+ * your driver
* @owner: module providing the pin controller, used for refcounting
*/
struct pinctrl_desc {
@@ -105,6 +108,7 @@ struct pinctrl_desc {
unsigned int maxpin;
struct pinctrl_ops *pctlops;
struct pinmux_ops *pmxops;
+ struct pinconf_ops *confops;
struct module *owner;
};
@@ -121,9 +125,7 @@ extern const char *pinctrl_dev_get_name(struct pinctrl_dev *pctldev);
extern void *pinctrl_dev_get_drvdata(struct pinctrl_dev *pctldev);
#else
-struct pinctrl_dev;
-
-/* Sufficiently stupid default function when pinctrl is not in use */
+/* Sufficiently stupid default functions when pinctrl is not in use */
static inline bool pin_is_valid(struct pinctrl_dev *pctldev, int pin)
{
return pin >= 0;
--
1.7.3.2
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2] pinctrl: add a generic pin config interface
2011-11-11 8:31 [PATCH v2] pinctrl: add a generic pin config interface Linus Walleij
@ 2011-11-11 11:26 ` Thomas Abraham
2011-11-14 9:36 ` Linus Walleij
2011-11-14 19:44 ` Stephen Warren
1 sibling, 1 reply; 12+ messages in thread
From: Thomas Abraham @ 2011-11-11 11:26 UTC (permalink / raw)
To: Linus Walleij
Cc: linux-kernel, Stephen Warren, Grant Likely, Barry Song,
Shawn Guo, Linus Walleij
Hi Linus,
On 11 November 2011 14:01, Linus Walleij <linus.walleij@stericsson.com> wrote:
> From: Linus Walleij <linus.walleij@linaro.org>
>
> This add per-pin and per-group pin config interfaces for biasing,
> driving and other such electronic properties. The intention is
> clearly to enumerate all things you can do with pins, hoping that
> these are enumerable.
>
> ChangeLog v1->v2:
> - Clear split of terminology: we now have pin controllers, and
> those may support two interfaces using vtables: pin
> multiplexing and pin configuration.
> - Break out pin configuration to its own C file, controllers may
> implement only config without mux, and vice versa, so keep each
> sub-functionality of pin controllers separate. Introduce
> CONFIG_PINCONF in Kconfig.
> - Implement some core logic around pin configuration in the
> pinconf.c file.
> - Remove UNKNOWN config states, these were just surplus baggage.
> - Remove FLOAT config state - HIGH_IMPEDANCE should be enough for
> everyone.
> - PIN_CONFIG_POWER_SOURCE added to handle switching the power
> supply for the pin logic between different sources
> - Explicit DISABLE config enums to turn schmitt-trigger,
> wakeup etc OFF.
> - Update documentation to reflect all the recent reasoning.
>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> Documentation/pinctrl.txt | 93 ++++++++++-
> drivers/pinctrl/Kconfig | 5 +-
> drivers/pinctrl/Makefile | 1 +
> drivers/pinctrl/core.c | 18 ++
> drivers/pinctrl/core.h | 10 ++
> drivers/pinctrl/pinconf.c | 332 +++++++++++++++++++++++++++++++++++++++
> drivers/pinctrl/pinconf.h | 34 ++++
> include/linux/pinctrl/pinconf.h | 193 +++++++++++++++++++++++
> include/linux/pinctrl/pinctrl.h | 10 +-
> 9 files changed, 685 insertions(+), 11 deletions(-)
> create mode 100644 drivers/pinctrl/pinconf.c
> create mode 100644 drivers/pinctrl/pinconf.h
> create mode 100644 include/linux/pinctrl/pinconf.h
>
> diff --git a/Documentation/pinctrl.txt b/Documentation/pinctrl.txt
> index ffef900..ad04c5e 100644
> --- a/Documentation/pinctrl.txt
> +++ b/Documentation/pinctrl.txt
> @@ -7,12 +7,9 @@ This subsystem deals with:
>
[...]
> +The pin control system supports an interface partly abstracting these
> +properties while leaving the details to the pin control driver. We assume that
> +the things a controller may want configure are enumerable, and thus the
> +parameters such as PIN_CONFIG_BIAS_PULL_UP are defined by the core, whereas
> +the arguments to the parameter may need to be on a custom format only
> +understandable by the driver. However we strive to use SI-derived entities for
> +these where applicable, which means the core may step in and do sematic
> +analysis of the passed values in select cases.
> +
> +For example, a driver can do this:
> +
> +ret = pin_config(128, PIN_CONFIG_BIAS_PULL_UP, 100000);
struct pinctrl_dev* is required as the first parameter in this example.
> +
> +To pull up a pin to VDD with a 100KOhm resistor. The driver implements
> +callbacks for changing pin configuration in the pin controller ops like this:
[...]
> diff --git a/include/linux/pinctrl/pinconf.h b/include/linux/pinctrl/pinconf.h
> new file mode 100644
> index 0000000..6b5163d
> --- /dev/null
> +++ b/include/linux/pinctrl/pinconf.h
> @@ -0,0 +1,193 @@
> +/*
> + * Interface the pinconfig portions of the pinctrl subsystem
> + *
> + * Copyright (C) 2011 ST-Ericsson SA
> + * Written on behalf of Linaro for ST-Ericsson
> + * This interface is used in the core to keep track of pins.
> + *
> + * Author: Linus Walleij <linus.walleij@linaro.org>
> + *
> + * License terms: GNU General Public License (GPL) version 2
> + */
> +#ifndef __LINUX_PINCTRL_PINCONF_H
> +#define __LINUX_PINCTRL_PINCONF_H
> +
> +/**
> + * enum pin_config_param - possible pin configuration parameters
> + * @PIN_CONFIG_BIAS_DISABLE: disable any pin bias on the pin, a
> + * transition from say pull-up to pull-down implies that you disable
> + * pull-up in the process, this setting disables all biasing
> + * @PIN_CONFIG_BIAS_HIGH_IMPEDANCE: the pin will be set to a high impedance
> + * mode, also know as "third-state" (tristate) or "high-Z" or "floating".
> + * On output pins this effectively disconnects the pin, which is useful
> + * if for example some other pin is going to drive the signal connected
> + * to it for a while. Pins used for input are usually always high
> + * impedance.
> + * @PIN_CONFIG_BIAS_PULL_UP: the pin will be pulled up (usually with high
> + * impedance to VDD), if the controller supports specifying a certain
> + * pull-up resistance, this is given as an argument (in Ohms) when
> + * setting this parameter
> + * @PIN_CONFIG_BIAS_PULL_DOWN: the pin will be pulled down (usually with high
> + * impedance to GROUND), if the controller supports specifying a certain
> + * pull-down resistance, this is given as an argument (in Ohms) when
> + * setting this parameter
> + * @PIN_CONFIG_BIAS_HIGH: the pin will be wired high, connected to VDD
> + * @PIN_CONFIG_BIAS_GROUND: the pin will be grounded, connected to GROUND
> + * @PIN_CONFIG_DRIVE_PUSH_PULL: the pin will be driven actively high and
> + * low, this is the most typical case and is typically achieved with two
> + * active transistors on the output. If the pin can support different
> + * drive strengths for push/pull, the strength is given on a custom format
> + * as argument when setting pins to this mode
> + * @PIN_CONFIG_DRIVE_OPEN_DRAIN: the pin will be driven with open drain (open
> + * collector) which means it is usually wired with other output ports
> + * which are then pulled up with an external resistor. If the pin can
> + * support different drive strengths for the open drain pin, the strength
> + * is given on a custom format as argument when setting pins to this mode
> + * @PIN_CONFIG_DRIVE_OPEN_SOURCE: the pin will be driven with open drain
> + * (open emitter) which is the same as open drain mutatis mutandis but
> + * pulled to ground. If the pin can support different drive strengths for
> + * the open drain pin, the strength is given on a custom format as
> + * argument when setting pins to this mode
> + * @PIN_CONFIG_DRIVE_OFF: the pin is set to inactive drive mode, off
> + * @PIN_CONFIG_INPUT_SCHMITT: this will configure an input pin to run in
> + * schmitt-trigger mode. If the schmitt-trigger has adjustable hysteresis,
> + * the threshold value is given on a custom format as argument when
> + * setting pins to this mode
> + * @PIN_CONFIG_INPUT_SCHMITT_OFF: disables schmitt-trigger mode
> + * @PIN_CONFIG_SLEW_RATE_RISING: this will configure the slew rate for rising
> + * signals on the pin. The argument gives the rise time in nanoseconds.
> + * You may want to adjust slew rates so that signal edges don't get too
> + * steep, causing disturbances in surrounding electronics for example.
> + * @PIN_CONFIG_SLEW_RATE_FALLING: this will configure the slew rate for falling
> + * signals on the pin. The argument gives the fall time in nanoseconds
> + * @PIN_CONFIG_LOAD_CAPACITANCE: some pins have inductive characteristics and
> + * will deform waveforms when signals are transmitted on them, by
> + * applying a load capacitance, the waveform can be rectified. The
> + * argument gives the load capacitance in picofarad (pF)
> + * @PIN_CONFIG_POWER_SOURCE: if the pin can select between different power
> + * supplies, the argument to this parameter (on a custom format) tells
> + * the driver which alternative power source to use
> + * @PIN_CONFIG_LOW_POWER_MODE: this will configure the pin for low power
> + * operation
> + * @PIN_CONFIG_NORMAL_POWER_MODE: returns pin from low power mode
> + * @PIN_CONFIG_WAKEUP_ENABLE: this will configure an input pin such that if a
> + * signal transition arrives at the pin when the pin controller/system
> + * is sleeping, it will wake up the system
> + * @PIN_CONFIG_WAKEUP_DISABLE: will disable a pin from the wakeup enable state
> + * set by the previous parameter
> + * @PIN_CONFIG_END: this is the last enumerator for pin configurations, if
> + * you need to pass in custom configurations to the pin controller, use
> + * PIN_CONFIG_END+1 as the base offset
> + */
> +enum pin_config_param {
> + PIN_CONFIG_BIAS_DISABLE,
> + PIN_CONFIG_BIAS_HIGH_IMPEDANCE,
> + PIN_CONFIG_BIAS_PULL_UP,
> + PIN_CONFIG_BIAS_PULL_DOWN,
> + PIN_CONFIG_BIAS_HIGH,
> + PIN_CONFIG_BIAS_GROUND,
> + PIN_CONFIG_DRIVE_PUSH_PULL,
> + PIN_CONFIG_DRIVE_OPEN_DRAIN,
> + PIN_CONFIG_DRIVE_OPEN_SOURCE,
> + PIN_CONFIG_DRIVE_OFF,
> + PIN_CONFIG_INPUT_SCHMITT,
> + PIN_CONFIG_INPUT_SCHMITT_OFF,
> + PIN_CONFIG_SLEW_RATE_RISING,
> + PIN_CONFIG_SLEW_RATE_FALLING,
> + PIN_CONFIG_LOAD_CAPACITANCE,
> + PIN_CONFIG_POWER_SOURCE,
> + PIN_CONFIG_LOW_POWER_MODE,
> + PIN_CONFIG_NORMAL_POWER_MODE,
> + PIN_CONFIG_WAKEUP_ENABLE,
> + PIN_CONFIG_WAKEUP_DISABLE,
> + PIN_CONFIG_END,
> +};
Samsung parts have a 'drive strength' config option for the pads. The
drive strength is represented as 1x, 2x, 4x, etc .. depending on the
SoC. The config param that can be used to represent drive strength in
this case could be PIN_CONFIG_POWER_SOURCE. Or should there be another
config param for representing drive strength? Otherwise, the above
config param options are sufficient for all existing Samsung SoC's.
> +
> +/**
> + * struct pin_config - configuration state holder for a single config of a pin
> + * @bias_param: bias configuration parameter
> + * @bias_data: configuration data for the parameter
> + * @schmitt: input is in schmitt-trigger mode
> + * @slewrate_rising: rising edge slew rate
> + * @slewrate_falling: falling edge slew rate
> + * @load_capacitance: load capacitane of the pin
> + * @power_source: selected power source for the pin
> + * @low_power: pin is in low power mode
> + * @wakeup: pin will wake up system when sleeping
> + *
> + * This holds one configuration item for one pin, a pin may have several such
> + * configurations since it may be configured for several non-conflicting modes
> + * simultaneously.
> + */
> +struct pin_config {
> + enum pin_config_param bias_param;
> + unsigned long bias_data;
> + enum pin_config_param drive_param;
> + unsigned long drive_data;
> + bool schmitt;
> + unsigned long slewrate_rising;
> + unsigned long slewrate_falling;
> + unsigned long load_capacitance;
> + unsigned long power_source;
> + bool low_power;
> + bool wakeup;
> +};
Most of these might remain unused since not all SoC's would have all
the possible config options. Is it required to keep pin controller
subsystem aware of individual pin config settings. Or should this be
handled by individual pinctrl drivers?
There is a API by name pin_config below. Should this struct or the API
name be changed so that they are different?
> +
> +#ifdef CONFIG_PINCONF
> +
> +struct pinctrl_dev;
> +
> +/**
> + * struct pinconf_ops - pin config operations, to be implemented by
> + * pin configuration capable drivers.
> + * @pin_get_initial_config: called to get the initial config of each pin
> + * if you cannot read out the configuration of your pins at startup, just
> + * leave this as NULL
> + * @pin_config: configure an individual pin
> + * @pin_config_group: configure all pins in a group
> + * @pin_config_dbg_show: optional debugfs display hook that will provide
> + * per-device info for a certain pin in debugfs
> + */
> +struct pinconf_ops {
> + int (*pin_get_initial_config) (struct pinctrl_dev *pctldev,
> + struct pin_config *conf,
> + unsigned pin);
> + int (*pin_config) (struct pinctrl_dev *pctldev,
> + const struct pin_config *conf,
> + unsigned pin,
> + enum pin_config_param param,
> + unsigned long data);
> + int (*pin_config_group) (struct pinctrl_dev *pctldev,
> + unsigned selector,
> + enum pin_config_param param,
> + unsigned long data);
> + void (*pin_config_dbg_show) (struct pinctrl_dev *pctldev,
> + struct seq_file *s,
> + unsigned offset);
> +};
> +
> +
> +extern int pin_config(struct pinctrl_dev *pctldev, int pin,
> + enum pin_config_param param, unsigned long data);
> +extern int pin_config_group(struct pinctrl_dev *pctldev, const char *pin_group,
> + enum pin_config_param param, unsigned long data);
> +
> +#else
> +
> +static inline int pin_config(struct pinctrl_dev *pctldev, int pin,
> + enum pin_config_param param, unsigned long data)
> +{
> + return 0;
> +}
> +
> +static inline int pin_config_group(struct pinctrl_dev *pctldev,
> + const char *pin_group,
> + enum pin_config_param param,
> + unsigned long data)
> +{
> + return 0;
> +}
> +
> +#endif
> +
> +#endif /* __LINUX_PINCTRL_PINCONF_H */
[...]
Thanks for this patch. I will try it on Samsung exynos along with the
pinctrl driver for exynos.
Regards,
Thomas.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] pinctrl: add a generic pin config interface
2011-11-11 11:26 ` Thomas Abraham
@ 2011-11-14 9:36 ` Linus Walleij
2011-11-14 14:24 ` Thomas Abraham
0 siblings, 1 reply; 12+ messages in thread
From: Linus Walleij @ 2011-11-14 9:36 UTC (permalink / raw)
To: Thomas Abraham
Cc: Linus Walleij, linux-kernel, Stephen Warren, Grant Likely,
Barry Song, Shawn Guo
On Fri, Nov 11, 2011 at 12:26 PM, Thomas Abraham
<thomas.abraham@linaro.org> wrote:
> [Me]
>> +ret = pin_config(128, PIN_CONFIG_BIAS_PULL_UP, 100000);
>
> struct pinctrl_dev* is required as the first parameter in this example.
Thanks, fixed it.
>> +enum pin_config_param {
>> + PIN_CONFIG_BIAS_DISABLE,
>> + PIN_CONFIG_BIAS_HIGH_IMPEDANCE,
>> + PIN_CONFIG_BIAS_PULL_UP,
>> + PIN_CONFIG_BIAS_PULL_DOWN,
>> + PIN_CONFIG_BIAS_HIGH,
>> + PIN_CONFIG_BIAS_GROUND,
>> + PIN_CONFIG_DRIVE_PUSH_PULL,
>> + PIN_CONFIG_DRIVE_OPEN_DRAIN,
>> + PIN_CONFIG_DRIVE_OPEN_SOURCE,
>> + PIN_CONFIG_DRIVE_OFF,
>> + PIN_CONFIG_INPUT_SCHMITT,
>> + PIN_CONFIG_INPUT_SCHMITT_OFF,
>> + PIN_CONFIG_SLEW_RATE_RISING,
>> + PIN_CONFIG_SLEW_RATE_FALLING,
>> + PIN_CONFIG_LOAD_CAPACITANCE,
>> + PIN_CONFIG_POWER_SOURCE,
>> + PIN_CONFIG_LOW_POWER_MODE,
>> + PIN_CONFIG_NORMAL_POWER_MODE,
>> + PIN_CONFIG_WAKEUP_ENABLE,
>> + PIN_CONFIG_WAKEUP_DISABLE,
>> + PIN_CONFIG_END,
>> +};
>
> Samsung parts have a 'drive strength' config option for the pads. The
> drive strength is represented as 1x, 2x, 4x, etc .. depending on the
> SoC. The config param that can be used to represent drive strength in
> this case could be PIN_CONFIG_POWER_SOURCE. Or should there be another
> config param for representing drive strength? Otherwise, the above
> config param options are sufficient for all existing Samsung SoC's.
I strongly suspect that you want to use
PIN_CONFIG_DRIVE_PUSH_PULL with a custom argument
representing the drive strength as "data" passed in the call.
Moste "strong" driving is done by push/pull (the others are just oddities)
and if you want to specify a number representing drive strength, the
data parameter is there to do exactly that. For this drive mode the
parameter does not have defined semantics, so you can interpret it
the way you need in your driver.
That said, 1x, 2x, 4x aren't exactly scientific - what does this really
represent, speaking electronics?
On some forum I found this:
http://www.edaboard.com/thread74140.html
gate drive strength
The symbols, 1X, 2X, 3X...etc in an ASIC flow in used for
convinience. What it means is a gate with 2X drive strength
will have the same rise/fall time while driving a capacitance
of 2C farads as that of a gate with X drive strength driving a
capacitance of C farads. You can look at the schematic and
see that gates with 2X drive strengths have approximately
twice the widths on output pull up/pull down trasistors as
comapred to the same gate with 1X drive strength.
Similarly definitions for 3X, 4X etc.
This is to ensure that ratio of width to load capacitance
remains constant, thus resulting in the same transition
times. It is desired that the transition times in an ASIC
chip be within a certain limit (DRV critereon). This will
be met if the following rule is followed -
If 1X drive strength is sufficient to drive a load of C farads
(ie, transition time is satisfactory) then 2X drive is
satisfactory to drive loads between C farads and 2C farads,
3X drive is sufficient to drive loads between 2C and 3C
farads....etc.
In practice is seems: 2x = 2 transistors in driver stage, 4x =
4 transistors in driver stage etc.
Maybe we should simply standardize the semantics of the
data argument to PIN_CONFIG_DRIVE_PUSH_PULL
to mean "x times nominal load capacitance with equal
rise/fall times"?
Alternatively we could use the
PIN_CONFIG_SLEW_RATE_RISING and
PIN_CONFIG_SLEW_RATE_FALLING for this,
however there I defined the semantics to be in
nanoseconds, assuming that electronics that can control
this have more delicate amplifiers, and in this case I
think it is more about limiting the rise/fall time by shunting
in resistances in the circuit.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] pinctrl: add a generic pin config interface
2011-11-14 9:36 ` Linus Walleij
@ 2011-11-14 14:24 ` Thomas Abraham
0 siblings, 0 replies; 12+ messages in thread
From: Thomas Abraham @ 2011-11-14 14:24 UTC (permalink / raw)
To: Linus Walleij
Cc: Linus Walleij, linux-kernel, Stephen Warren, Grant Likely,
Barry Song, Shawn Guo
On 14 November 2011 15:06, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Fri, Nov 11, 2011 at 12:26 PM, Thomas Abraham
> <thomas.abraham@linaro.org> wrote:
>
[...]
>> Samsung parts have a 'drive strength' config option for the pads. The
>> drive strength is represented as 1x, 2x, 4x, etc .. depending on the
>> SoC. The config param that can be used to represent drive strength in
>> this case could be PIN_CONFIG_POWER_SOURCE. Or should there be another
>> config param for representing drive strength? Otherwise, the above
>> config param options are sufficient for all existing Samsung SoC's.
>
> I strongly suspect that you want to use
> PIN_CONFIG_DRIVE_PUSH_PULL with a custom argument
> representing the drive strength as "data" passed in the call.
>
> Moste "strong" driving is done by push/pull (the others are just oddities)
> and if you want to specify a number representing drive strength, the
> data parameter is there to do exactly that. For this drive mode the
> parameter does not have defined semantics, so you can interpret it
> the way you need in your driver.
Ok.
>
> That said, 1x, 2x, 4x aren't exactly scientific - what does this really
> represent, speaking electronics?
I have not attempted to understand that yet.
>
> On some forum I found this:
> http://www.edaboard.com/thread74140.html
Thanks for the link. This was very informative.
[...]
> In practice is seems: 2x = 2 transistors in driver stage, 4x =
> 4 transistors in driver stage etc.
>
> Maybe we should simply standardize the semantics of the
> data argument to PIN_CONFIG_DRIVE_PUSH_PULL
> to mean "x times nominal load capacitance with equal
> rise/fall times"?
Ok. This should be fine for Exynos platforms. But it might not seem
intuitive to use PIN_CONFIG_DRIVE_PUSH_PULL for driver strength.
Probably, this might need additional documentation.
>
> Alternatively we could use the
> PIN_CONFIG_SLEW_RATE_RISING and
> PIN_CONFIG_SLEW_RATE_FALLING for this,
> however there I defined the semantics to be in
> nanoseconds, assuming that electronics that can control
> this have more delicate amplifiers, and in this case I
> think it is more about limiting the rise/fall time by shunting
> in resistances in the circuit.
Ok. As everybody might not understand the electronics behind the
driver strength, it would be difficult to use RATE_RISING and
RATE_FALLING config options. Maybe we should have a simpler config
option that is easy to understand and program for drive strength.
Thank you Linus.
Regards,
Thomas.
>
> Yours,
> Linus Walleij
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [PATCH v2] pinctrl: add a generic pin config interface
2011-11-11 8:31 [PATCH v2] pinctrl: add a generic pin config interface Linus Walleij
2011-11-11 11:26 ` Thomas Abraham
@ 2011-11-14 19:44 ` Stephen Warren
2011-11-17 13:26 ` Linus Walleij
1 sibling, 1 reply; 12+ messages in thread
From: Stephen Warren @ 2011-11-14 19:44 UTC (permalink / raw)
To: Linus Walleij, linux-kernel
Cc: Grant Likely, Barry Song, Shawn Guo, Linus Walleij
Linus Walleij wrote at Friday, November 11, 2011 1:31 AM:
> This add per-pin and per-group pin config interfaces for biasing,
> driving and other such electronic properties. The intention is
> clearly to enumerate all things you can do with pins, hoping that
> these are enumerable.
...
> diff --git a/include/linux/pinctrl/pinconf.h b/include/linux/pinctrl/pinconf.h
...
> + * enum pin_config_param - possible pin configuration parameters
I'm still not entirely convinced that abstracting all these minutiae is
productive.
I assert that drivers should not be directly manipulating configuration
parameters. Instead, they should be requesting some generic named state,
and some board-provided data should be mapping that named state to the set
of configuration values to apply. Assuming that's true, then there doesn't
need to be any generic way of naming/describing what those configuration
values are, since only the SoC-specific pinctrl driver and board-specific
mapping table will ever deal with the individual values.
> + * @PIN_CONFIG_BIAS_DISABLE: disable any pin bias on the pin, a
> + * transition from say pull-up to pull-down implies that you disable
> + * pull-up in the process, this setting disables all biasing
> + * @PIN_CONFIG_BIAS_HIGH_IMPEDANCE: the pin will be set to a high impedance
> + * mode, also know as "third-state" (tristate) or "high-Z" or "floating".
> + * On output pins this effectively disconnects the pin, which is useful
> + * if for example some other pin is going to drive the signal connected
> + * to it for a while. Pins used for input are usually always high
> + * impedance.
> + * @PIN_CONFIG_BIAS_PULL_UP: the pin will be pulled up (usually with high
> + * impedance to VDD), if the controller supports specifying a certain
> + * pull-up resistance, this is given as an argument (in Ohms) when
> + * setting this parameter
> + * @PIN_CONFIG_BIAS_PULL_DOWN: the pin will be pulled down (usually with high
> + * impedance to GROUND), if the controller supports specifying a certain
> + * pull-down resistance, this is given as an argument (in Ohms) when
> + * setting this parameter
I agree that its unlikely to be common to enable both pull-up and pull-
down at the same time, but why should the PIN_CONFIG_ abstraction actively
prohibit it? An Soc could easily be designed with a bit to enable each,
and hence allow enabling both at once. Perhaps with configurable pull
strengths, this could even be used to create a poor-man's A-D converter,
or perhaps it could be used to create a reference voltage for something.
While I admit this /is/ pretty unlikely to happen in practice, this is
one of the reasons I'm not convinced that abstracting all this is a great
idea; if the abstraction isn't general enough, it might prohibit us from
representing unforeseen features in the future.
> + * @PIN_CONFIG_BIAS_HIGH: the pin will be wired high, connected to VDD
> + * @PIN_CONFIG_BIAS_GROUND: the pin will be grounded, connected to GROUND
> + * @PIN_CONFIG_DRIVE_PUSH_PULL: the pin will be driven actively high and
> + * low, this is the most typical case and is typically achieved with two
> + * active transistors on the output. If the pin can support different
> + * drive strengths for push/pull, the strength is given on a custom format
> + * as argument when setting pins to this mode
If the drive-strength argument is custom, this vastly reduces the
usefulness of creating this single abstraction; sure a driver could
specify PUSH_PULL, but if it then wanted to configure the strength, but
couldn't because there was no standardized way of representing that, it
seems pointless to create the common abstraction for PUSH_PULL in the
first place; what is it achieving? In fact, if a driver just wants to
toggle between PUSH_PULL/OPEN_DRAIN, what value should it provide for
the value if there's no standardization?
> + * @PIN_CONFIG_DRIVE_OPEN_DRAIN: the pin will be driven with open drain (open
> + * collector) which means it is usually wired with other output ports
> + * which are then pulled up with an external resistor. If the pin can
> + * support different drive strengths for the open drain pin, the strength
> + * is given on a custom format as argument when setting pins to this mode
> + * @PIN_CONFIG_DRIVE_OPEN_SOURCE: the pin will be driven with open drain
> + * (open emitter) which is the same as open drain mutatis mutandis but
I think it'd be better to spell this out, rather than make non-latin-speakers
go look that up, and still perhaps not know /what/ the appropriate changes to
the description actually are.
> + * pulled to ground. If the pin can support different drive strengths for
> + * the open drain pin, the strength is given on a custom format as
> + * argument when setting pins to this mode
> + * @PIN_CONFIG_DRIVE_OFF: the pin is set to inactive drive mode, off
> + * @PIN_CONFIG_INPUT_SCHMITT: this will configure an input pin to run in
> + * schmitt-trigger mode. If the schmitt-trigger has adjustable hysteresis,
> + * the threshold value is given on a custom format as argument when
> + * setting pins to this mode
> + * @PIN_CONFIG_INPUT_SCHMITT_OFF: disables schmitt-trigger mode
Shouldn't that be just PIN_CONFIG_INPUT_SCHMITT with a boolean parameter
to indicate whether it's enabled?
> + * @PIN_CONFIG_SLEW_RATE_RISING: this will configure the slew rate for rising
> + * signals on the pin. The argument gives the rise time in nanoseconds.
> + * You may want to adjust slew rates so that signal edges don't get too
> + * steep, causing disturbances in surrounding electronics for example.
> + * @PIN_CONFIG_SLEW_RATE_FALLING: this will configure the slew rate for falling
> + * signals on the pin. The argument gives the fall time in nanoseconds
In order to specify rates, you'd also need to define what load capacitance
was being driven; rate isn't an independent value. I wonder if a standard
load inductance would also need to be specified.
Tegra's slew rates are not specified in nanoSeconds. Now it may be that
for a given load capacitance, each rate does indeed map to a specific
rise time. However, the documentation isn't written that way. Getting the
documentation changed to specify times simply isn't going to happen; it's
hard enough to just get the documentation in the first place for some of
the pinmux stuff. In fact, even calculating what those times are probably
isn't possible right now (for me at least, and the low-level pad designers
probably have better things to do).
> + * @PIN_CONFIG_LOAD_CAPACITANCE: some pins have inductive characteristics and
That should say "capacitive"; inductance is something else AIUI.
> + * will deform waveforms when signals are transmitted on them, by
> + * applying a load capacitance, the waveform can be rectified. The
> + * argument gives the load capacitance in picofarad (pF)
> + * @PIN_CONFIG_POWER_SOURCE: if the pin can select between different power
> + * supplies, the argument to this parameter (on a custom format) tells
> + * the driver which alternative power source to use
> + * @PIN_CONFIG_LOW_POWER_MODE: this will configure the pin for low power
> + * operation
> + * @PIN_CONFIG_NORMAL_POWER_MODE: returns pin from low power mode
Is this meant to represent Tegra's "low power mode" configuration? That's
a four-level value on Tegra, so having two PIN_CONFIG values doesn't make
sense; we'd have to use the value for e.g. LOW_POWER_MODE to represent the
desired HW value, and ignore NORMAL_POWER_MODE, I think.
> + * @PIN_CONFIG_WAKEUP_ENABLE: this will configure an input pin such that if a
> + * signal transition arrives at the pin when the pin controller/system
> + * is sleeping, it will wake up the system
> + * @PIN_CONFIG_WAKEUP_DISABLE: will disable a pin from the wakeup enable state
> + * set by the previous parameter
> + * @PIN_CONFIG_END: this is the last enumerator for pin configurations, if
> + * you need to pass in custom configurations to the pin controller, use
> + * PIN_CONFIG_END+1 as the base offset
> + */
> +enum pin_config_param {
> + PIN_CONFIG_BIAS_DISABLE,
...
> + PIN_CONFIG_END,
> +};
> +
> +/**
> + * struct pin_config - configuration state holder for a single config of a pin
> + * @bias_param: bias configuration parameter
> + * @bias_data: configuration data for the parameter
> + * @schmitt: input is in schmitt-trigger mode
> + * @slewrate_rising: rising edge slew rate
> + * @slewrate_falling: falling edge slew rate
> + * @load_capacitance: load capacitane of the pin
> + * @power_source: selected power source for the pin
> + * @low_power: pin is in low power mode
> + * @wakeup: pin will wake up system when sleeping
> + *
> + * This holds one configuration item for one pin, a pin may have several such
> + * configurations since it may be configured for several non-conflicting modes
> + * simultaneously.
> + */
> +struct pin_config {
> + enum pin_config_param bias_param;
> + unsigned long bias_data;
> + enum pin_config_param drive_param;
> + unsigned long drive_data;
> + bool schmitt;
> + unsigned long slewrate_rising;
> + unsigned long slewrate_falling;
> + unsigned long load_capacitance;
> + unsigned long power_source;
> + bool low_power;
> + bool wakeup;
> +};
Not all SoCs support all those options. On Tegra, different groups
support different subsets of those options. In the pin_get_initial_config()
API below, how does a pinctrl driver indicate unknown or not-applicable
for individual fields?
> +/**
> + * struct pinconf_ops - pin config operations, to be implemented by
> + * pin configuration capable drivers.
> + * @pin_get_initial_config: called to get the initial config of each pin
> + * if you cannot read out the configuration of your pins at startup, just
> + * leave this as NULL
> + * @pin_config: configure an individual pin
> + * @pin_config_group: configure all pins in a group
> + * @pin_config_dbg_show: optional debugfs display hook that will provide
> + * per-device info for a certain pin in debugfs
> + */
> +struct pinconf_ops {
> + int (*pin_get_initial_config) (struct pinctrl_dev *pctldev,
> + struct pin_config *conf,
> + unsigned pin);
> + int (*pin_config) (struct pinctrl_dev *pctldev,
> + const struct pin_config *conf,
> + unsigned pin,
> + enum pin_config_param param,
> + unsigned long data);
> + int (*pin_config_group) (struct pinctrl_dev *pctldev,
> + unsigned selector,
> + enum pin_config_param param,
> + unsigned long data);
> + void (*pin_config_dbg_show) (struct pinctrl_dev *pctldev,
> + struct seq_file *s,
> + unsigned offset);
> +};
There was some discussion about having to call pin_config_group many
Times to set up e.g. 5 different parameters on a single group. Should
pin_config_group accept a list of param/data pairs, or a struct pin_config
instead? The latter would also need some NA/don't-change indication for
each field.
--
nvpublic
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] pinctrl: add a generic pin config interface
2011-11-14 19:44 ` Stephen Warren
@ 2011-11-17 13:26 ` Linus Walleij
2011-11-18 22:32 ` Stephen Warren
0 siblings, 1 reply; 12+ messages in thread
From: Linus Walleij @ 2011-11-17 13:26 UTC (permalink / raw)
To: Stephen Warren
Cc: Linus Walleij, linux-kernel, Grant Likely, Barry Song, Shawn Guo
Hi Stephen,
as usual the most thought-provoking review comments come from nVidia,
many thanks for your time!
On Mon, Nov 14, 2011 at 8:44 PM, Stephen Warren <swarren@nvidia.com> wrote:
> Linus Walleij wrote at Friday, November 11, 2011 1:31 AM:
>> + * enum pin_config_param - possible pin configuration parameters
>
> I'm still not entirely convinced that abstracting all these minutiae is
> productive.
We'll see where we end up ... I have similar fears about *not* doing
it and ending up with some message passing core of too loose or
none semantics. So bear with me for some patch iterations...
> I assert that drivers should not be directly manipulating configuration
> parameters. Instead, they should be requesting some generic named state,
> and some board-provided data should be mapping that named state to the set
> of configuration values to apply. Assuming that's true, then there doesn't
> need to be any generic way of naming/describing what those configuration
> values are, since only the SoC-specific pinctrl driver and board-specific
> mapping table will ever deal with the individual values.
But that is not about this enum.
This enum is a way to describe states for the pins, which is also
the interface between the pinconf core and the individual drivers,
i.e. even with what you say above these enums are useful for
parameters passed in the functions in struct pinconf_ops.
If I rephrased the above to say that these two functions:
extern int pin_config(struct pinctrl_dev *pctldev, int pin,
enum pin_config_param param, unsigned long data);
extern int pin_config_group(struct pinctrl_dev *pctldev, const char *pin_group,
enum pin_config_param param, unsigned long data);
Should not be part of the public API available to the drivers and
platforms, then I'm game. That's a very valid point. Fixing that
will require some upfront code (again ... hehe)
I'll post some V3 with this current scheme though, and think about
how to proceed with abstracting the group states and getting
rid of the above for V4.
>> + * @PIN_CONFIG_BIAS_DISABLE: disable any pin bias on the pin, a
>> + * transition from say pull-up to pull-down implies that you disable
>> + * pull-up in the process, this setting disables all biasing
>> + * @PIN_CONFIG_BIAS_HIGH_IMPEDANCE: the pin will be set to a high impedance
>> + * mode, also know as "third-state" (tristate) or "high-Z" or "floating".
>> + * On output pins this effectively disconnects the pin, which is useful
>> + * if for example some other pin is going to drive the signal connected
>> + * to it for a while. Pins used for input are usually always high
>> + * impedance.
>> + * @PIN_CONFIG_BIAS_PULL_UP: the pin will be pulled up (usually with high
>> + * impedance to VDD), if the controller supports specifying a certain
>> + * pull-up resistance, this is given as an argument (in Ohms) when
>> + * setting this parameter
>> + * @PIN_CONFIG_BIAS_PULL_DOWN: the pin will be pulled down (usually with high
>> + * impedance to GROUND), if the controller supports specifying a certain
>> + * pull-down resistance, this is given as an argument (in Ohms) when
>> + * setting this parameter
>
> I agree that its unlikely to be common to enable both pull-up and pull-
> down at the same time, but why should the PIN_CONFIG_ abstraction actively
> prohibit it?
I've read the above and fail to see why that would be prohibited
from the enum alone.
In practice it does not work however since I track the state like this:
struct pin_config {
enum pin_config_param bias_param;
unsigned long bias_data;
(...)
};
I'd opt for leaving it like this and have the first driver that wants
to set multiple bias configs simultaneously refactor it to make
that possible, in the process proving me wrong on the account
that this is not a useful feature.
> An Soc could easily be designed with a bit to enable each,
> and hence allow enabling both at once. Perhaps with configurable pull
> strengths, this could even be used to create a poor-man's A-D converter,
> or perhaps it could be used to create a reference voltage for something.
OK the day someone needs to do this it can be refactored,
I don't think this needs to be part of the upfront design.
At that point drivers/staging/iio/adc will probably be available
in drivers/* and it can be modelled using the proper abstraction
for these registers anyway.
> While I admit this /is/ pretty unlikely to happen in practice, this is
> one of the reasons I'm not convinced that abstracting all this is a great
> idea; if the abstraction isn't general enough, it might prohibit us from
> representing unforeseen features in the future.
There will always be unforeseen use cases, what I want to avoid
is coding something that is hard to amend for new use cases
down the road. What I want to make sure is that we have an
API that can support what we have in the kernel today, and
avoid lock-ins where possible.
>> + * @PIN_CONFIG_BIAS_HIGH: the pin will be wired high, connected to VDD
>> + * @PIN_CONFIG_BIAS_GROUND: the pin will be grounded, connected to GROUND
>> + * @PIN_CONFIG_DRIVE_PUSH_PULL: the pin will be driven actively high and
>> + * low, this is the most typical case and is typically achieved with two
>> + * active transistors on the output. If the pin can support different
>> + * drive strengths for push/pull, the strength is given on a custom format
>> + * as argument when setting pins to this mode
>
> If the drive-strength argument is custom, this vastly reduces the
> usefulness of creating this single abstraction; sure a driver could
> specify PUSH_PULL, but if it then wanted to configure the strength, but
> couldn't because there was no standardized way of representing that, it
> seems pointless to create the common abstraction for PUSH_PULL in the
> first place; what is it achieving? In fact, if a driver just wants to
> toggle between PUSH_PULL/OPEN_DRAIN, what value should it provide for
> the value if there's no standardization?
As discussed in the other thread with Thomas the way this is done
for most systems is in drive stage multiples vs nominal load
impedance (usually capacitive, but we don't need to specify that) for one drive
stage as described in this forum thread:
http://www.edaboard.com/thread74140.html
Which means the argument should be number of drive stages.
So I'll just standardize on that and see if it sticks. For something like
push/pull CMOS or TTL totempole it seems to hit the nail on the head
atleast, if the electronics people come up with more exotic drive stages
it should probably not use the push/pull denominator anyway.
So, point taken. Will standardize.
>> + * @PIN_CONFIG_DRIVE_OPEN_DRAIN: the pin will be driven with open drain (open
>> + * collector) which means it is usually wired with other output ports
>> + * which are then pulled up with an external resistor. If the pin can
>> + * support different drive strengths for the open drain pin, the strength
>> + * is given on a custom format as argument when setting pins to this mode
>> + * @PIN_CONFIG_DRIVE_OPEN_SOURCE: the pin will be driven with open drain
>> + * (open emitter) which is the same as open drain mutatis mutandis but
>
> I think it'd be better to spell this out, rather than make non-latin-speakers
> go look that up, and still perhaps not know /what/ the appropriate changes to
> the description actually are.
OK. I was too infatuated with my own language games...
I guess for the intended audience using the form of s/foo/bar/g is more
understandable, but I'll spell it out.
>> + * @PIN_CONFIG_INPUT_SCHMITT: this will configure an input pin to run in
>> + * schmitt-trigger mode. If the schmitt-trigger has adjustable hysteresis,
>> + * the threshold value is given on a custom format as argument when
>> + * setting pins to this mode
>> + * @PIN_CONFIG_INPUT_SCHMITT_OFF: disables schmitt-trigger mode
>
> Shouldn't that be just PIN_CONFIG_INPUT_SCHMITT with a boolean parameter
> to indicate whether it's enabled?
To match the PIN_CONFIG_DRIVE_OFF this is more logical I think,
there is no shortage of enums.
>> + * @PIN_CONFIG_SLEW_RATE_RISING: this will configure the slew rate for rising
>> + * signals on the pin. The argument gives the rise time in nanoseconds.
>> + * You may want to adjust slew rates so that signal edges don't get too
>> + * steep, causing disturbances in surrounding electronics for example.
>> + * @PIN_CONFIG_SLEW_RATE_FALLING: this will configure the slew rate for falling
>> + * signals on the pin. The argument gives the fall time in nanoseconds
>
> In order to specify rates, you'd also need to define what load capacitance
> was being driven; rate isn't an independent value. I wonder if a standard
> load inductance would also need to be specified.
>
> Tegra's slew rates are not specified in nanoSeconds. Now it may be that
> for a given load capacitance, each rate does indeed map to a specific
> rise time. However, the documentation isn't written that way. Getting the
> documentation changed to specify times simply isn't going to happen; it's
> hard enough to just get the documentation in the first place for some of
> the pinmux stuff. In fact, even calculating what those times are probably
> isn't possible right now (for me at least, and the low-level pad designers
> probably have better things to do).
OK so how is the slew rate specified in the specs you have?
Is it just some magic number? If it turns out all platforms only have
unspecified magic numbers for slew rate settings we'd better
just leave this argument as custom then.
>> + * @PIN_CONFIG_LOAD_CAPACITANCE: some pins have inductive characteristics and
>
> That should say "capacitive"; inductance is something else AIUI.
No actually not. See below:
>> + * will deform waveforms when signals are transmitted on them, by
>> + * applying a load capacitance, the waveform can be rectified. The
>> + * argument gives the load capacitance in picofarad (pF)
So the pin (or the wire connected to it) has inductive properties,
and you compensate by adding load capacitance.
I'll reword to make this clear...
>> + * @PIN_CONFIG_LOW_POWER_MODE: this will configure the pin for low power
>> + * operation
>> + * @PIN_CONFIG_NORMAL_POWER_MODE: returns pin from low power mode
>
> Is this meant to represent Tegra's "low power mode" configuration? That's
> a four-level value on Tegra,
OK I added that the argument to low power mode can contain a custom power
state enumerator.
> so having two PIN_CONFIG values doesn't make
> sense; we'd have to use the value for e.g. LOW_POWER_MODE to represent the
> desired HW value, and ignore NORMAL_POWER_MODE, I think.
NORMAL_POWER_MODE is intended to return the pin from any low-power
mode in analogy with say BIAS_DISABLE or DRIVE_OFF, SCHMITT_OFF
so it's to make the code symmetric and readable, not to make the minimal
number of enumerators. I think this is better syntax than say, specifying that a
zero argument to LOW_POWER_MODE means "low power mode off"
>> +/**
>> + * struct pin_config - configuration state holder for a single config of a pin
>> + * @bias_param: bias configuration parameter
>> + * @bias_data: configuration data for the parameter
>> + * @schmitt: input is in schmitt-trigger mode
>> + * @slewrate_rising: rising edge slew rate
>> + * @slewrate_falling: falling edge slew rate
>> + * @load_capacitance: load capacitane of the pin
>> + * @power_source: selected power source for the pin
>> + * @low_power: pin is in low power mode
>> + * @wakeup: pin will wake up system when sleeping
>> + *
>> + * This holds one configuration item for one pin, a pin may have several such
>> + * configurations since it may be configured for several non-conflicting modes
>> + * simultaneously.
>> + */
>> +struct pin_config {
>> + enum pin_config_param bias_param;
>> + unsigned long bias_data;
>> + enum pin_config_param drive_param;
>> + unsigned long drive_data;
>> + bool schmitt;
>> + unsigned long slewrate_rising;
>> + unsigned long slewrate_falling;
>> + unsigned long load_capacitance;
>> + unsigned long power_source;
>> + bool low_power;
>> + bool wakeup;
>> +};
>
> Not all SoCs support all those options. On Tegra, different groups
> support different subsets of those options. In the pin_get_initial_config()
> API below, how does a pinctrl driver indicate unknown or not-applicable
> for individual fields?
Currently the fields contain default values until filled in with
wither get_initial_configuration() of by configuration calls.
I've toyed with the idea of adding per-pin capabilities, like a flag
for each supported configuration option as another field of
struct pinctrl_pin_desc say:
struct pinctrl_pin_desc {
unsigned number;
const char *name;
#ifdef CONFIG_PINCONF
enum pin_config_param *supported_params;
unsigned num_supported_params;
#endif
};
So a controller would declare supported params per pin like:
static enum pin_config_param myparams[] = {
PIN_CONFIG_BIAS_DISABLE,
PIN_CONFIG_BIAS_HIGH_IMPEDANCE,
PIN_CONFIG_DRIVE_PUSH_PULL,
PIN_CONFIG_DRIVE_OFF,
};
struct pinctrl_pin_desc mypins[] = {
{
.number = 1,
.name = "foo",
.supported_params = myparams;
.num_supported_params = ARRAY_SIZE(myparams),
},
...
}
I'm a bit hesitant since it may be overdesigned, overkill,
and the little footprint conscience I still have inside me does not like
the idea either and would prefer to trust driver writers to do things
right and return proper errors for illegal configurations without full
constraints on everything.
But it's a loose opinion and I may be wrong. I haven't seen any
other pin config driver in the kernel that tries to prevent you from
doing things you cannot do though.
>> +/**
>> + * struct pinconf_ops - pin config operations, to be implemented by
>> + * pin configuration capable drivers.
>> + * @pin_get_initial_config: called to get the initial config of each pin
>> + * if you cannot read out the configuration of your pins at startup, just
>> + * leave this as NULL
>> + * @pin_config: configure an individual pin
>> + * @pin_config_group: configure all pins in a group
>> + * @pin_config_dbg_show: optional debugfs display hook that will provide
>> + * per-device info for a certain pin in debugfs
>> + */
>> +struct pinconf_ops {
>> + int (*pin_get_initial_config) (struct pinctrl_dev *pctldev,
>> + struct pin_config *conf,
>> + unsigned pin);
>> + int (*pin_config) (struct pinctrl_dev *pctldev,
>> + const struct pin_config *conf,
>> + unsigned pin,
>> + enum pin_config_param param,
>> + unsigned long data);
>> + int (*pin_config_group) (struct pinctrl_dev *pctldev,
>> + unsigned selector,
>> + enum pin_config_param param,
>> + unsigned long data);
>> + void (*pin_config_dbg_show) (struct pinctrl_dev *pctldev,
>> + struct seq_file *s,
>> + unsigned offset);
>> +};
>
> There was some discussion about having to call pin_config_group many
> Times to set up e.g. 5 different parameters on a single group. Should
> pin_config_group accept a list of param/data pairs, or a struct pin_config
> instead? The latter would also need some NA/don't-change indication for
> each field.
struct pin_config was mainly thought of as a state container, bolting
a lot of "changed" fields on it seems kludgy.
So both pin_config() and pin_config_group() should rather take a list
of config params + data.
Which sort of brings the discussion to the point of whether the
passed configuration element should rather be a tuple than two
discrete arguments everywhere. Which folds back into the infrastructure
for providing the per-group state mapping etc described in the beginning
of the mail, since this would likely be the basic configuration atom, so
I need to think a bit about that and try to come up with something we
can use going forward.
Thanks!
Linus Walleij
^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [PATCH v2] pinctrl: add a generic pin config interface
2011-11-17 13:26 ` Linus Walleij
@ 2011-11-18 22:32 ` Stephen Warren
2011-11-21 19:29 ` Linus Walleij
0 siblings, 1 reply; 12+ messages in thread
From: Stephen Warren @ 2011-11-18 22:32 UTC (permalink / raw)
To: Linus Walleij
Cc: Linus Walleij, linux-kernel, Grant Likely, Barry Song, Shawn Guo
Linus Walleij wrote at Thursday, November 17, 2011 6:26 AM:
> Hi Stephen,
>
> as usual the most thought-provoking review comments come from nVidia,
> many thanks for your time!
Thanks for listening:-)
> On Mon, Nov 14, 2011 at 8:44 PM, Stephen Warren <swarren@nvidia.com> wrote:
> > Linus Walleij wrote at Friday, November 11, 2011 1:31 AM:
> >> + * enum pin_config_param - possible pin configuration parameters
> >
> > I'm still not entirely convinced that abstracting all these minutiae is
> > productive.
>
> We'll see where we end up ... I have similar fears about *not* doing
> it and ending up with some message passing core of too loose or
> none semantics. So bear with me for some patch iterations...
>
> > I assert that drivers should not be directly manipulating configuration
> > parameters. Instead, they should be requesting some generic named state,
> > and some board-provided data should be mapping that named state to the set
> > of configuration values to apply. Assuming that's true, then there doesn't
> > need to be any generic way of naming/describing what those configuration
> > values are, since only the SoC-specific pinctrl driver and board-specific
> > mapping table will ever deal with the individual values.
>
> But that is not about this enum.
>
> This enum is a way to describe states for the pins, which is also
> the interface between the pinconf core and the individual drivers,
> i.e. even with what you say above these enums are useful for
> parameters passed in the functions in struct pinconf_ops.
Yes, whether to expose a pin config API and whether to define a
standardized config enum (possibly only for a pinctrl core <-> pinctrl
driver interface) are indeed two separate things.
> If I rephrased the above to say that these two functions:
>
> extern int pin_config(struct pinctrl_dev *pctldev, int pin,
> enum pin_config_param param, unsigned long data);
> extern int pin_config_group(struct pinctrl_dev *pctldev, const char *pin_group,
> enum pin_config_param param, unsigned long data);
>
> Should not be part of the public API available to the drivers and
> platforms, then I'm game. That's a very valid point. Fixing that
> will require some upfront code (again ... hehe)
>
> I'll post some V3 with this current scheme though, and think about
> how to proceed with abstracting the group states and getting
> rid of the above for V4.
OK.
The primary thing I want to understand is this:
Assuming we did remove the "public" pin_config APIs, what's the problem
that having a defined enum pin_config_param is solving?
If drivers are deciding what config params and values to set, then I do
see the need for standardized naming; drivers need to be generic across
SoCs.
However, if there is no driver-facing config API, then the data flow is:
* (SoC-specific) board file (or device-tree) provides some data to the
pinctrl core.
* At some appropriate time, pinmux core passes that data to the pinctrl
driver.
I assert that:
* The pinctrl core need not use, understand, or modify the provided data
in any way.
* The data only needs to make sense the SoC-specific pinctrl driver that's
used on the board that originally passed the data to the pinctrl core.
Assuming all that, I see no value in an abstraction; there's no problem
to solve, since no part of the code that handles the values ever has to
both interpret those values and be generic across multiple SoCs.
> >> + * @PIN_CONFIG_BIAS_DISABLE: disable any pin bias on the pin, a
> >> + * transition from say pull-up to pull-down implies that you disable
> >> + * pull-up in the process, this setting disables all biasing
> >> + * @PIN_CONFIG_BIAS_HIGH_IMPEDANCE: the pin will be set to a high impedance
> >> + * mode, also know as "third-state" (tristate) or "high-Z" or "floating".
> >> + * On output pins this effectively disconnects the pin, which is useful
> >> + * if for example some other pin is going to drive the signal connected
> >> + * to it for a while. Pins used for input are usually always high
> >> + * impedance.
> >> + * @PIN_CONFIG_BIAS_PULL_UP: the pin will be pulled up (usually with high
> >> + * impedance to VDD), if the controller supports specifying a certain
> >> + * pull-up resistance, this is given as an argument (in Ohms) when
> >> + * setting this parameter
> >> + * @PIN_CONFIG_BIAS_PULL_DOWN: the pin will be pulled down (usually with high
> >> + * impedance to GROUND), if the controller supports specifying a certain
> >> + * pull-down resistance, this is given as an argument (in Ohms) when
> >> + * setting this parameter
> >
> > I agree that its unlikely to be common to enable both pull-up and pull-
> > down at the same time, but why should the PIN_CONFIG_ abstraction actively
> > prohibit it?
>
> I've read the above and fail to see why that would be prohibited
> from the enum alone.
>
> In practice it does not work however since I track the state like this:
>
> struct pin_config {
> enum pin_config_param bias_param;
> unsigned long bias_data;
> (...)
> };
Right, that was exactly my point.
Logically, pull-up is one thing you can control, and pull-down is another.
As such, they should have different PIN_CONFIG values, and each have their
own Boolean value.
HIGH_IMPEDANCE isn't really a bias option, it's a lack of a drive option.
> I'd opt for leaving it like this and have the first driver that wants
> to set multiple bias configs simultaneously refactor it to make
> that possible, in the process proving me wrong on the account
> that this is not a useful feature.
>
> > An Soc could easily be designed with a bit to enable each,
> > and hence allow enabling both at once. Perhaps with configurable pull
> > strengths, this could even be used to create a poor-man's A-D converter,
> > or perhaps it could be used to create a reference voltage for something.
>
> OK the day someone needs to do this it can be refactored,
> I don't think this needs to be part of the upfront design.
> At that point drivers/staging/iio/adc will probably be available
> in drivers/* and it can be modelled using the proper abstraction
> for these registers anyway.
One potential issue here; this will make it more difficult to create a
device-tree binding, since the binding needs to be a fixed ABI, whereas
if we were just talking about an in-kernel structure, we could modify it
all we want without as much backwards compatibility.
> >> + * @PIN_CONFIG_BIAS_HIGH: the pin will be wired high, connected to VDD
> >> + * @PIN_CONFIG_BIAS_GROUND: the pin will be grounded, connected to GROUND
> >> + * @PIN_CONFIG_DRIVE_PUSH_PULL: the pin will be driven actively high and
> >> + * low, this is the most typical case and is typically achieved with two
> >> + * active transistors on the output. If the pin can support different
> >> + * drive strengths for push/pull, the strength is given on a custom format
> >> + * as argument when setting pins to this mode
> >
> > If the drive-strength argument is custom, this vastly reduces the
> > usefulness of creating this single abstraction; sure a driver could
> > specify PUSH_PULL, but if it then wanted to configure the strength, but
> > couldn't because there was no standardized way of representing that, it
> > seems pointless to create the common abstraction for PUSH_PULL in the
> > first place; what is it achieving? In fact, if a driver just wants to
> > toggle between PUSH_PULL/OPEN_DRAIN, what value should it provide for
> > the value if there's no standardization?
>
> As discussed in the other thread with Thomas the way this is done
> for most systems is in drive stage multiples vs nominal load
> impedance (usually capacitive, but we don't need to specify that) for one drive
> stage as described in this forum thread:
> http://www.edaboard.com/thread74140.html
>
> Which means the argument should be number of drive stages.
That thread is just one person's assertions. I have no way to judge
if they're really globally true or not. I'd be hesitant to based any
decisions on one random unauthenticated thread on some random forum.
I wonder why Tegra's driver strengths are 1, 1/2, 1/4, 1/8 if they're
Implemented like that forum thread says. While I suppose it's entirely
possibly there are an extra 1, 1, 2, 4 transistors for each level, it
seems more likely that there would be 1 extra transistor per level
giving drive strengths of 1, 3/4, 1/2, 1/4. However, I'm just randomly
conjecturing here...
> >> + * @PIN_CONFIG_INPUT_SCHMITT: this will configure an input pin to run in
> >> + * schmitt-trigger mode. If the schmitt-trigger has adjustable hysteresis,
> >> + * the threshold value is given on a custom format as argument when
> >> + * setting pins to this mode
> >> + * @PIN_CONFIG_INPUT_SCHMITT_OFF: disables schmitt-trigger mode
> >
> > Shouldn't that be just PIN_CONFIG_INPUT_SCHMITT with a boolean parameter
> > to indicate whether it's enabled?
>
> To match the PIN_CONFIG_DRIVE_OFF this is more logical I think,
> there is no shortage of enums.
Sure, but given Schmitt is a single logical thing that can be turned on
and off, and the API is a param/value style API, surely the param should
be "is Schmitt enabled" and the value "on/off" or "yes/no".
>
> >> + * @PIN_CONFIG_SLEW_RATE_RISING: this will configure the slew rate for rising
> >> + * signals on the pin. The argument gives the rise time in nanoseconds.
> >> + * You may want to adjust slew rates so that signal edges don't get too
> >> + * steep, causing disturbances in surrounding electronics for example.
> >> + * @PIN_CONFIG_SLEW_RATE_FALLING: this will configure the slew rate for falling
> >> + * signals on the pin. The argument gives the fall time in nanoseconds
> >
> > In order to specify rates, you'd also need to define what load capacitance
> > was being driven; rate isn't an independent value. I wonder if a standard
> > load inductance would also need to be specified.
> >
> > Tegra's slew rates are not specified in nanoSeconds. Now it may be that
> > for a given load capacitance, each rate does indeed map to a specific
> > rise time. However, the documentation isn't written that way. Getting the
> > documentation changed to specify times simply isn't going to happen; it's
> > hard enough to just get the documentation in the first place for some of
> > the pinmux stuff. In fact, even calculating what those times are probably
> > isn't possible right now (for me at least, and the low-level pad designers
> > probably have better things to do).
>
> OK so how is the slew rate specified in the specs you have?
> Is it just some magic number? If it turns out all platforms only have
> unspecified magic numbers for slew rate settings we'd better
> just leave this argument as custom then.
Yes. The exact text is e.g.:
DRVDN_SLWR - Driver Output Rising Edge Slew 2-bit control code. Code 11
is least slewing of signal, code 00 is highest slewing of the signal.
> >> + * @PIN_CONFIG_LOAD_CAPACITANCE: some pins have inductive characteristics and
> >
> > That should say "capacitive"; inductance is something else AIUI.
>
> No actually not. See below:
>
> >> + * will deform waveforms when signals are transmitted on them, by
> >> + * applying a load capacitance, the waveform can be rectified. The
> >> + * argument gives the load capacitance in picofarad (pF)
>
> So the pin (or the wire connected to it) has inductive properties,
> and you compensate by adding load capacitance.
I don't think that's correct.
Any electrical device has both some intrinsic capacitance and inductance.
These are orthogonal fundamental properties.
Now, it's entirely possible that if your device's inductance is high (or
perhaps for other reasons too), you explicitly add some capacitors to
absorb spikes that occur when switching the device on or off, but that
doesn't make inductance part of the definition of capacitance.
> >> + * @PIN_CONFIG_LOW_POWER_MODE: this will configure the pin for low power
> >> + * operation
> >> + * @PIN_CONFIG_NORMAL_POWER_MODE: returns pin from low power mode
> >
> > Is this meant to represent Tegra's "low power mode" configuration? That's
> > a four-level value on Tegra,
>
> OK I added that the argument to low power mode can contain a custom power
> state enumerator.
>
> > so having two PIN_CONFIG values doesn't make
> > sense; we'd have to use the value for e.g. LOW_POWER_MODE to represent the
> > desired HW value, and ignore NORMAL_POWER_MODE, I think.
>
> NORMAL_POWER_MODE is intended to return the pin from any low-power
> mode in analogy with say BIAS_DISABLE or DRIVE_OFF, SCHMITT_OFF
> so it's to make the code symmetric and readable, not to make the minimal
> number of enumerators. I think this is better syntax than say, specifying that a
> zero argument to LOW_POWER_MODE means "low power mode off"
Maybe this is representing something different than what Tegra's LPMD
bits are then.
On Tegra, this isn't a kind of "turn off and consume less power" toggle,
but rather a way of configuring the pin while it's active; it's a value
0..3 (on Tegra20 at least) that interacts with the other drive strength
and slew rate properties and affects overall active pin performance.
> > There was some discussion about having to call pin_config_group many
> > Times to set up e.g. 5 different parameters on a single group. Should
> > pin_config_group accept a list of param/data pairs, or a struct pin_config
> > instead? The latter would also need some NA/don't-change indication for
> > each field.
>
> struct pin_config was mainly thought of as a state container, bolting
> a lot of "changed" fields on it seems kludgy.
>
> So both pin_config() and pin_config_group() should rather take a list
> of config params + data.
A list sounds good.
--
nvpublic
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] pinctrl: add a generic pin config interface
2011-11-18 22:32 ` Stephen Warren
@ 2011-11-21 19:29 ` Linus Walleij
2011-11-21 23:22 ` Stephen Warren
0 siblings, 1 reply; 12+ messages in thread
From: Linus Walleij @ 2011-11-21 19:29 UTC (permalink / raw)
To: Stephen Warren
Cc: Linus Walleij, linux-kernel, Grant Likely, Barry Song, Shawn Guo,
Mark Brown
On Fri, Nov 18, 2011 at 11:32 PM, Stephen Warren <swarren@nvidia.com> wrote:
> Linus Walleij wrote at Thursday, November 17, 2011 6:26 AM:
> The primary thing I want to understand is this:
>
> Assuming we did remove the "public" pin_config APIs, what's the problem
> that having a defined enum pin_config_param is solving?
>
> If drivers are deciding what config params and values to set, then I do
> see the need for standardized naming; drivers need to be generic across
> SoCs.
>
> However, if there is no driver-facing config API, then the data flow is:
>
> * (SoC-specific) board file (or device-tree) provides some data to the
> pinctrl core.
>
> * At some appropriate time, pinmux core passes that data to the pinctrl
> driver.
>
> I assert that:
>
> * The pinctrl core need not use, understand, or modify the provided data
> in any way.
>
> * The data only needs to make sense the SoC-specific pinctrl driver that's
> used on the board that originally passed the data to the pinctrl core.
>
> Assuming all that, I see no value in an abstraction; there's no problem
> to solve, since no part of the code that handles the values ever has to
> both interpret those values and be generic across multiple SoCs.
Yes that is true if the problem space is only to pass driver-specific
information to specific drivers. I.e. if the subsystem shall be
only some mediator and mechanics with no deeper knowledge
about what't going on. But I don't see things that way, this is not
an extended drivers/base/* for me, but a subsystem that should
know what it is doing.
Thinking about it I think the reason I feel so strongly for this is
more about understanding the abstractions from the outside.
Say I am a pinmux author for TI and I learned a set of pin config
options and how to use them, then I need to go over and fix up
some stuff in the S3C driver as part of some refactoring or so.
If we have a common well-defined terminology, you will
understand the code in the other driver quickly, say you
know what PIN_CONFIG_BIAS_HIGH_IMPEDANCE means.
If one driver calls the same thing TI_OMAP_HIGHZ_CONF
and the other calls it S3C_TRISTATE_SETUP that
becomes a burden to understand all differences. And as
a subsystem maintainer I *have* to understand and be
able to patch around in all drivers. Without common
terminology I will easily find myself not understanding the
different implementations. I will only know that "it's doing
some magic on pin X".
So basically it has a value for code maintenance. I also
want to encode some specific electronic attributes to each
state to achieve some correspondance with the thing that
would appear in textbooks about this kind of stuff.
So it in some way is a construction of a common language
for this subsystem and then the reason is the same as
for why I don't write all code comments in Swedish - I
need others to understand it.
I made a similar comment on the devicetree list some days
ago - I am not aware of the ambitions or ideas behind
DT, but if the things inside some specific dts file is
supposed to be generally understood we cannot just have
a bunch of custom values for everything. The DTS file
may also need to see outside maintenance, all custom
content won't work IMO. Defining pin configs in the DTS
file using these enumerators make them more
understandable to any developer as I see it.
As I read some of the paragraphs below I see the push to
route around all-custom configuration values as stemming
partly from lacking documentation, and I do not think it is
proper to design an entire subsystem as per the principle
"we don't really know what we're doing here", as if I am
randomly poking in some values in some register because
some other guy told me they should have that value. I don't
see other subsystems designed very much like that.
For the U300 and Nomadik controllers I think I pretty much
know what the supported values are, so these enumerators
will be useful across these two machines at least.
> Logically, pull-up is one thing you can control, and pull-down is another.
> As such, they should have different PIN_CONFIG values, and each have their
> own Boolean value.
Yes and they will be controllable each, what I'm after here is
whether the core should allow both to be set at the same
time, since it knows this is generally a bad idea. So I mean
the core should understand what it is doing, not just respond
to configuration requests and leave all understanding to the
individual drivers.
I would compare to how the regulator subsystem infers the
allowed voltage range for a certain regulator from the
regulator, rail and list of consumer supply limits. It has
understanding of the voltages it tries to set.
> HIGH_IMPEDANCE isn't really a bias option, it's a lack of a drive option.
Hm, do you mean that PIN_CONFIG_BIAS_DISABLE
and PIN_CONFIG_BIAS_HIGH_IMPEDANCE should be merged
into one option?
>> OK the day someone needs to do this it can be refactored,
>> I don't think this needs to be part of the upfront design.
>> At that point drivers/staging/iio/adc will probably be available
>> in drivers/* and it can be modelled using the proper abstraction
>> for these registers anyway.
>
> One potential issue here; this will make it more difficult to create a
> device-tree binding, since the binding needs to be a fixed ABI, whereas
> if we were just talking about an in-kernel structure, we could modify it
> all we want without as much backwards compatibility.
But that must apply to *any* subsystem which wants to change the
way configuration in which data is to be passed in, not unique to
pin control really? Is DT only for things that have fixed ABI?
Since this is a kernel-internal API I smell a violation of
Documentation/stable_api_nonsense.txt ...
So in that case maybe pincontrol is not mature for DT bindings
yet. Maybe that is something for mature subsystems like
I2C, memory-mapped IO, IRQ numbers, even GPIO ... etc
for the time being.
In the powerpc world it is a common custom to update the device
tree(s) in arch/powerpc/boot/dts/* and drivers or platform code
in the same patch even. (C.f. commit
0b5cf10691eb2c95a9126bf25f5e084d83d5d743) I take it that
this needs to be allowed also on ARM.
I think at one point in a conference in Prague it was agreed that
DTS files will be kept inside the kernel for the initial submissions,
and at the same time it was said that this was precisely because
bindings will likely be changing quite a bit.
If ARM DTS files are indeed broken out of the kernel git,
well they will just have to be kept in sync I believe?
>> As discussed in the other thread with Thomas the way this is done
>> for most systems is in drive stage multiples vs nominal load
>> impedance (usually capacitive, but we don't need to specify that) for one drive
>> stage as described in this forum thread:
>> http://www.edaboard.com/thread74140.html
>>
>> Which means the argument should be number of drive stages.
>
> That thread is just one person's assertions. I have no way to judge
> if they're really globally true or not. I'd be hesitant to based any
> decisions on one random unauthenticated thread on some random forum.
Well I'm trying to check in text books. But how many ways of
designing push/pull can there be (practical, not theoretical)?
> I wonder why Tegra's driver strengths are 1, 1/2, 1/4, 1/8 if they're
> Implemented like that forum thread says. While I suppose it's entirely
> possibly there are an extra 1, 1, 2, 4 transistors for each level, it
> seems more likely that there would be 1 extra transistor per level
> giving drive strengths of 1, 3/4, 1/2, 1/4. However, I'm just randomly
> conjecturing here...
Hm. Since the terminology seems related I would *guess* something
like 1 = 8 transistors (4 up 4 down), 3/4 = 6 transistors (3 up 3 down),
1/2 = 4 transistors (2 up 2 down), 1/4 = 2 transistors (1 up 1 down).
So the number would be "number of driving stages as related
to the maximum available" and the maximum is 4 driving stages.
So then it would indeed make sense to define the parameter
argument as "number of active driving stages" so it becomes:
# driving stages S3C Tegra
1 1x 1/4
2 2x 1/2
3 N/A 3/4
4 4x 1
8 8x N/A
Atleast it makes logical sense.
I'm a bit worried if we're writing drivers for hardware where the
documentation is so sparse that we don't really know what
we're doing... How do you know how to select between say the
1/2 and 1/4 setting today? The subsystem will have a hard time
to compensate for lack of engineering documentation whatever
it comes to :-(
>> > Shouldn't that be just PIN_CONFIG_INPUT_SCHMITT with a boolean parameter
>> > to indicate whether it's enabled?
>>
>> To match the PIN_CONFIG_DRIVE_OFF this is more logical I think,
>> there is no shortage of enums.
>
> Sure, but given Schmitt is a single logical thing that can be turned on
> and off, and the API is a param/value style API, surely the param should
> be "is Schmitt enabled" and the value "on/off" or "yes/no".
OK I'll change it.
I also defined low power mode 0 to mean normal power mode
(low power mode off) and changed
PIN_CONFIG_WAKEUP_ENABLE/DISABLE to just
PIN_CONFIG_WAKEUP with arguments 1 for enable
and 0 for disable.
Should we also join PIN_CONFIG_BIAS_DISABLE and
PIN_CONFIG_BIAS_HIGH_IMPEDANCE (as was hinted above)
and possibly CONFIG_DRIVE_OFF could be rewritten as
"any of PIN_CONFIG_DRIVE_* with an argument of zero".
>> > In order to specify rates, you'd also need to define what load capacitance
>> > was being driven; rate isn't an independent value. I wonder if a standard
>> > load inductance would also need to be specified.
>> >
>> > Tegra's slew rates are not specified in nanoSeconds. Now it may be that
>> > for a given load capacitance, each rate does indeed map to a specific
>> > rise time. However, the documentation isn't written that way. Getting the
>> > documentation changed to specify times simply isn't going to happen; it's
>> > hard enough to just get the documentation in the first place for some of
>> > the pinmux stuff. In fact, even calculating what those times are probably
>> > isn't possible right now (for me at least, and the low-level pad designers
>> > probably have better things to do).
>>
>> OK so how is the slew rate specified in the specs you have?
>> Is it just some magic number? If it turns out all platforms only have
>> unspecified magic numbers for slew rate settings we'd better
>> just leave this argument as custom then.
>
> Yes. The exact text is e.g.:
>
> DRVDN_SLWR - Driver Output Rising Edge Slew 2-bit control code. Code 11
> is least slewing of signal, code 00 is highest slewing of the signal.
So when you attach some electronics to this SoC, how do you
choose which value to poke in there? Trial-and-error?
Again I think we need to understand the things we're trying
to program. That documentation is lacking is bad, but do we
really want to design the subsystem around lacking documentation
instead of deeper understanding?
The Nomadik GPIO is described like this in the manual for
AP9500 used in the Snowball (page 775)
http://www.stericsson.com/developers/DM00030004_AP9500_reference_manual_rev1.pdf
"The GPIO1B_LOWEMI register is used to control the IO pad
current slew rate. When LOWEMI= ‘1’, the normal slew rate is
slashed to half of its normal value, us enabling Low EMI mode. In
normal case, LOWEMI=’0’."
So I wouldn't know much about rise and fall times either. But
"slashed in half" is pretty clear, so I guess the argument might
be formulated as "fractions of maximum slew rate", so if the slew
rate can be contolled in 8 steps the argument 7 gives 7/8
slew rate, argument 2 gives 2/8 = 1/4 slew rate compared to
nominal etc. This is similar to the proportional number of
driving stages suggested for PIN_CONFIG_DRIVE_*
>> >> + * @PIN_CONFIG_LOAD_CAPACITANCE: some pins have inductive characteristics and
>> >
>> > That should say "capacitive"; inductance is something else AIUI.
>>
>> No actually not. See below:
>>
>> >> + * will deform waveforms when signals are transmitted on them, by
>> >> + * applying a load capacitance, the waveform can be rectified. The
>> >> + * argument gives the load capacitance in picofarad (pF)
>>
>> So the pin (or the wire connected to it) has inductive properties,
>> and you compensate by adding load capacitance.
>
> I don't think that's correct.
>
> Any electrical device has both some intrinsic capacitance and inductance.
> These are orthogonal fundamental properties.
Yes I think I have understood so far atleast :-)
> Now, it's entirely possible that if your device's inductance is high (or
> perhaps for other reasons too), you explicitly add some capacitors to
> absorb spikes that occur when switching the device on or off, but that
> doesn't make inductance part of the definition of capacitance.
I think I have misunderstood this thing, or don't understand it
properly, so I'll remove it as it's not needed for now. I though it
was there to compensate for ringing, but it may be that the
datasheet is just expressing drive strengths 1x and 2x in some
odd way.
The same datasheet had another interesting pin property too:
debounce time :-) OMAP also seems to have that,
and an associate debounce time.
>> > so having two PIN_CONFIG values doesn't make
>> > sense; we'd have to use the value for e.g. LOW_POWER_MODE to represent the
>> > desired HW value, and ignore NORMAL_POWER_MODE, I think.
>>
>> NORMAL_POWER_MODE is intended to return the pin from any low-power
>> mode in analogy with say BIAS_DISABLE or DRIVE_OFF, SCHMITT_OFF
>> so it's to make the code symmetric and readable, not to make the minimal
>> number of enumerators. I think this is better syntax than say, specifying that a
>> zero argument to LOW_POWER_MODE means "low power mode off"
>
> Maybe this is representing something different than what Tegra's LPMD
> bits are then.
>
> On Tegra, this isn't a kind of "turn off and consume less power" toggle,
> but rather a way of configuring the pin while it's active; it's a value
> 0..3 (on Tegra20 at least) that interacts with the other drive strength
> and slew rate properties and affects overall active pin performance.
Hm, I don't follow this quite... what is it then? How do you select
the apropriate value in your code?
>> > There was some discussion about having to call pin_config_group many
>> > Times to set up e.g. 5 different parameters on a single group. Should
>> > pin_config_group accept a list of param/data pairs, or a struct pin_config
>> > instead? The latter would also need some NA/don't-change indication for
>> > each field.
>>
>> struct pin_config was mainly thought of as a state container, bolting
>> a lot of "changed" fields on it seems kludgy.
>>
>> So both pin_config() and pin_config_group() should rather take a list
>> of config params + data.
>
> A list sounds good.
I'm going back and forth on this.
But I'll code up something with tuple-lists to the config functions and
see what it looks like, just as good to try it out.
Thanks,
Linus Walleij
^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [PATCH v2] pinctrl: add a generic pin config interface
2011-11-21 19:29 ` Linus Walleij
@ 2011-11-21 23:22 ` Stephen Warren
2011-11-22 13:59 ` Mark Brown
0 siblings, 1 reply; 12+ messages in thread
From: Stephen Warren @ 2011-11-21 23:22 UTC (permalink / raw)
To: Linus Walleij
Cc: Linus Walleij, linux-kernel, Grant Likely, Barry Song, Shawn Guo,
Mark Brown
Linus Walleij wrote at Monday, November 21, 2011 12:29 PM:
> On Fri, Nov 18, 2011 at 11:32 PM, Stephen Warren <swarren@nvidia.com> wrote:
...
> > The primary thing I want to understand is this:
> >
> > Assuming we did remove the "public" pin_config APIs, what's the problem
> > that having a defined enum pin_config_param is solving?
> >
> > If drivers are deciding what config params and values to set, then I do
> > see the need for standardized naming; drivers need to be generic across
> > SoCs.
> >
> > However, if there is no driver-facing config API, then the data flow is:
> >
> > * (SoC-specific) board file (or device-tree) provides some data to the
> > pinctrl core.
> >
> > * At some appropriate time, pinmux core passes that data to the pinctrl
> > driver.
> >
> > I assert that:
> >
> > * The pinctrl core need not use, understand, or modify the provided data
> > in any way.
> >
> > * The data only needs to make sense the SoC-specific pinctrl driver that's
> > used on the board that originally passed the data to the pinctrl core.
> >
> > Assuming all that, I see no value in an abstraction; there's no problem
> > to solve, since no part of the code that handles the values ever has to
> > both interpret those values and be generic across multiple SoCs.
>
> Yes that is true if the problem space is only to pass driver-specific
> information to specific drivers. I.e. if the subsystem shall be
> only some mediator and mechanics with no deeper knowledge
> about what't going on. But I don't see things that way, this is not
> an extended drivers/base/* for me, but a subsystem that should
> know what it is doing.
>
> Thinking about it I think the reason I feel so strongly for this is
> more about understanding the abstractions from the outside.
> Say I am a pinmux author for TI and I learned a set of pin config
> options and how to use them, then I need to go over and fix up
> some stuff in the S3C driver as part of some refactoring or so.
For basic config params, that might work out.
But the differences between the more esoteric config params are such that
nobody will ever be able to casually understand another SoC's complete
set of config options with or without a subsystem-defined config param
abstraction; a thorough reading of the SoC manual would be required.
Equally, I'm not sure the case you mention is what we should be optimizing
for. The people who deal with these options most will be FAEs (Field
Application Engineers), PCB designers, ... who intimately understand
the details of the SoC they're working with. They'll be well-versed in
the SoC-specific naming of all these properties. Forcing them to map all
that knowledge into an all-encompassing abstraction is only going to make
their life harder, and lead to mistakes.
Perhaps the more basic stuff like tri-state vss drive vs. open-drain or
pull-up enabled or pull-down enable can be standardized, but a region of
the config param namespace reserved for vendor extensions? That way, stuff
that really is likely to be common can be, but the other stuff which
honestly I don't think matters that much from a standardization perspective
can be kept SoC-specific.
> I also
> want to encode some specific electronic attributes to each
> state to achieve some correspondance with the thing that
> would appear in textbooks about this kind of stuff.
Why does that benefit anyone? Surely most people will take a SoC manual,
work out the settings they need using the terminology of that manual,
and attempt to set them. I don't think it's the domain of the subsystem
to teach people about electronics.
> So it in some way is a construction of a common language
> for this subsystem and then the reason is the same as
> for why I don't write all code comments in Swedish - I
> need others to understand it.
Why would an engineer working on SoC A need to converse with an engineer
working on SoC B about the detailed settings of SoC B?
Well, admittedly we're doing exactly that here, because you want to define
an abstraction that works across many SoCs, and hence are trying to
understand them all to do this. However, if you don't aim to create such
an abstraction, you won't need to understand them all, and there won't be
any language issue.
> I made a similar comment on the devicetree list some days
> ago - I am not aware of the ambitions or ideas behind
> DT, but if the things inside some specific dts file is
> supposed to be generally understood we cannot just have
> a bunch of custom values for everything. The DTS file
> may also need to see outside maintenance, all custom
> content won't work IMO.
> Defining pin configs in the DTS
> file using these enumerators make them more
> understandable to any developer as I see it.
I don't think so; I'm going to fill in the DTS file based on reading the
SoC manual and board schematic. That's all vendor-specific terminology.
Having to map that to an abstraction only makes it harder.
I want to point out again that simply having an abstraction isn't going
to stop anyone having to learn the intimate details of what all those
abstract values really do in the hardware. That's still a must. And as
such, I don't think the common language helps with understanding.
> As I read some of the paragraphs below I see the push to
> route around all-custom configuration values as stemming
> partly from lacking documentation, and I do not think it is
> proper to design an entire subsystem as per the principle
> "we don't really know what we're doing here", as if I am
> randomly poking in some values in some register because
> some other guy told me they should have that value. I don't
> see other subsystems designed very much like that.
All the fields are documented to the extent that I think anyone working
on Tegra would care about. The documentation issue is more that you'd
like the abstraction to define each config param to have a specific
unit, and hence each SoC's description has to include details of what
units each parameter has in order to convert the values to whatever units
the abstraction is defined to be in.
Put another way: Documentation has nothing to do with whether I want an
abstraction, just to do with whether I think the values of config params
can be in a specific unit defined by the abstraction.
(At least, I think... I've probably contra-dicted myself somewhere I've
written so much:-)
> > Logically, pull-up is one thing you can control, and pull-down is another.
> > As such, they should have different PIN_CONFIG values, and each have their
> > own Boolean value.
>
> Yes and they will be controllable each, what I'm after here is
> whether the core should allow both to be set at the same
> time, since it knows this is generally a bad idea. So I mean
> the core should understand what it is doing, not just respond
> to configuration requests and leave all understanding to the
> individual drivers.
Why should the core enforce policy (though shalln't not both pull up
and down)? I think a general rule in the kernel is to not implement
policy, and let user-space dictate that (well, in this case, board files
or device-tree files, but still at least not the core of the subsystem)
> I would compare to how the regulator subsystem infers the
> allowed voltage range for a certain regulator from the
> regulator, rail and list of consumer supply limits. It has
> understanding of the voltages it tries to set.
That's true, but I think that a regulator outputs a voltage is a much
better defined and consistent concept than some of the pin config values.
> > HIGH_IMPEDANCE isn't really a bias option, it's a lack of a drive option.
>
> Hm, do you mean that PIN_CONFIG_BIAS_DISABLE
> and PIN_CONFIG_BIAS_HIGH_IMPEDANCE should be merged
> into one option?
They're probably the same thing yes, and the same as PIN_CONFIG_DRIVE_OFF
with none of the following four options (which are each presumably
independent Booleans) enabled:
PIN_CONFIG_BIAS_PULL_UP
PIN_CONFIG_BIAS_PULL_DOWN
PIN_CONFIG_BIAS_HIGH
PIN_CONFIG_BIAS_GROUND
> >> OK the day someone needs to do this it can be refactored,
> >> I don't think this needs to be part of the upfront design.
> >> At that point drivers/staging/iio/adc will probably be available
> >> in drivers/* and it can be modelled using the proper abstraction
> >> for these registers anyway.
> >
> > One potential issue here; this will make it more difficult to create a
> > device-tree binding, since the binding needs to be a fixed ABI, whereas
> > if we were just talking about an in-kernel structure, we could modify it
> > all we want without as much backwards compatibility.
>
> But that must apply to *any* subsystem which wants to change the
> way configuration in which data is to be passed in, not unique to
> pin control really? Is DT only for things that have fixed ABI?
> Since this is a kernel-internal API I smell a violation of
> Documentation/stable_api_nonsense.txt ...
The kernel has a stable API (ABI) to user-space. That document applies
to intra-kernel APIs only. DT is the equivalent of an API to user-space
in this context, since once I've created a DT and installed it on a
device, I must be able to run an arbitrary (future) kernel using the
data from that DT.
So, the DT ABI doesn't have to be fixed in stone (it can be extensible),
but any enhancements to it must be backwards compatible.
So, you can add new fields to the DT schema, and if they're missing, just
assume a value that makes everything work like it did before you modified
the schema.
Or, you could make a significant change and somehow version the content
of the DT, and have the kernel be able to parse both versions, just like
you can introduce new IOCTLs and keep the old one around). Still, doing
this is pretty heavy-handed.
As a specific example, if we start off with param PIN_CONFIG_BIAS_PULL
that has enumerated values UP, DOWN, NONE, then the DT binding might be:
{ pin-pull = "up"; ... }
{ pin-pull = "down"; ... }
{ ... } // missing property means none
In kernel, we can very easily change this to two properties
PIN_CONFIG_BIAS_PULL_UP and PIN_CONFIG_BIAS_PULL_UP, each a Boolean.
However adjusting the DT binding to represent the new combination would
be a little challenging; we could easily parse the first two options
into setting the new Boolean value to true, but how to represent the
new legal option of having both; perhaps:
{ pin-pull = "updown"; ... }
Yuck!
It would have been far better to have defined the binding using
independent Boolean properties in the first place:
{ pin-pull-up; } // property without value is "true"
{ pin-pull-down; }
{ pin-pull-up; pin-pull-down; }
So that's my reasoning for *if* we have an abstraction, we want to get
what's in that abstraction correct up-front.
> So in that case maybe pincontrol is not mature for DT bindings
> yet. Maybe that is something for mature subsystems like
> I2C, memory-mapped IO, IRQ numbers, even GPIO ... etc
> for the time being.
Hmm, well I need a binding pretty soon in order to allow Tegra-booting-
with-DT to not rely on the now legacy board file pinmux setup code...
> In the powerpc world it is a common custom to update the device
> tree(s) in arch/powerpc/boot/dts/* and drivers or platform code
> in the same patch even. (C.f. commit
> 0b5cf10691eb2c95a9126bf25f5e084d83d5d743) I take it that
> this needs to be allowed also on ARM.
Yes, both could be updated at once. However, we still need backwards
compatibility. Just because *.dts is in the kernel, doesn't mean we can
break compatibility.
> I think at one point in a conference in Prague it was agreed that
> DTS files will be kept inside the kernel for the initial submissions,
> and at the same time it was said that this was precisely because
> bindings will likely be changing quite a bit.
>
> If ARM DTS files are indeed broken out of the kernel git,
> well they will just have to be kept in sync I believe?
I /think/ that was the documentation of the bindings; the schema for
the .dts files, not the files themselves, but I may be wrong.
But either way, there's nothing incorrect about somebody using an out-
of-tree .dts file to boot their system, and we can't assume we'll be
able to keep that .dts file in sync with the kernel.
> >> As discussed in the other thread with Thomas the way this is done
> >> for most systems is in drive stage multiples vs nominal load
> >> impedance (usually capacitive, but we don't need to specify that) for one drive
> >> stage as described in this forum thread:
> >> http://www.edaboard.com/thread74140.html
> >>
> >> Which means the argument should be number of drive stages.
> >
> > That thread is just one person's assertions. I have no way to judge
> > if they're really globally true or not. I'd be hesitant to based any
> > decisions on one random unauthenticated thread on some random forum.
>
> Well I'm trying to check in text books. But how many ways of
> designing push/pull can there be (practical, not theoretical)?
I have no idea at that level of ASIC design. Given neither of us do,
I'd suggest we don't rely on obtaining such knowledge.
> > I wonder why Tegra's driver strengths are 1, 1/2, 1/4, 1/8 if they're
> > Implemented like that forum thread says. While I suppose it's entirely
> > possibly there are an extra 1, 1, 2, 4 transistors for each level, it
> > seems more likely that there would be 1 extra transistor per level
> > giving drive strengths of 1, 3/4, 1/2, 1/4. However, I'm just randomly
> > conjecturing here...
>
> Hm. Since the terminology seems related I would *guess* something
> like 1 = 8 transistors (4 up 4 down), 3/4 = 6 transistors (3 up 3 down),
> 1/2 = 4 transistors (2 up 2 down), 1/4 = 2 transistors (1 up 1 down).
>
> So the number would be "number of driving stages as related
> to the maximum available" and the maximum is 4 driving stages.
>
> So then it would indeed make sense to define the parameter
> argument as "number of active driving stages" so it becomes:
>
> # driving stages S3C Tegra
> 1 1x 1/4
> 2 2x 1/2
> 3 N/A 3/4
> 4 4x 1
> 8 8x N/A
>
> Atleast it makes logical sense.
That's entirely possible. My point is that while the numbers may work
out that way, that's only correlation not causation. I have no idea how
many transistors Tegra has per pad, and I don't and shouldn't have to
care; what I care about is that a board designer or FAE wants to pick
"drive strength option 3", because that's what they validated.
> I'm a bit worried if we're writing drivers for hardware where the
> documentation is so sparse that we don't really know what
> we're doing... How do you know how to select between say the
> 1/2 and 1/4 setting today? The subsystem will have a hard time
> to compensate for lack of engineering documentation whatever
> it comes to :-(
I believe either simulations or practical measurement are used to
determine which option to select (e.g. based on minimal ringing, timing
spec conformance, etc.), and the SW engineers simply make sure they
program the value they're told to. While I certainly do like to understand
everything fully, there are some things that I just let go, and let
someone decide for me. Almost all of the esoteric settings are the domain
of HW engineers, and it's their responsibility to fix things if there is
system instability; kernel engineers shouldn't be too overly concerned I
think.
> >> > Shouldn't that be just PIN_CONFIG_INPUT_SCHMITT with a boolean parameter
> >> > to indicate whether it's enabled?
> >>
> >> To match the PIN_CONFIG_DRIVE_OFF this is more logical I think,
> >> there is no shortage of enums.
> >
> > Sure, but given Schmitt is a single logical thing that can be turned on
> > and off, and the API is a param/value style API, surely the param should
> > be "is Schmitt enabled" and the value "on/off" or "yes/no".
>
> OK I'll change it.
>
> I also defined low power mode 0 to mean normal power mode
> (low power mode off) and changed
> PIN_CONFIG_WAKEUP_ENABLE/DISABLE to just
> PIN_CONFIG_WAKEUP with arguments 1 for enable
> and 0 for disable.
>
> Should we also join PIN_CONFIG_BIAS_DISABLE and
> PIN_CONFIG_BIAS_HIGH_IMPEDANCE (as was hinted above)
> and possibly CONFIG_DRIVE_OFF could be rewritten as
> "any of PIN_CONFIG_DRIVE_* with an argument of zero".
s/any/all/ I think. But again I think DRIVE_OFF is what the other two mean,
more than the other way around.
> >> > In order to specify rates, you'd also need to define what load capacitance
> >> > was being driven; rate isn't an independent value. I wonder if a standard
> >> > load inductance would also need to be specified.
> >> >
> >> > Tegra's slew rates are not specified in nanoSeconds. Now it may be that
> >> > for a given load capacitance, each rate does indeed map to a specific
> >> > rise time. However, the documentation isn't written that way. Getting the
> >> > documentation changed to specify times simply isn't going to happen; it's
> >> > hard enough to just get the documentation in the first place for some of
> >> > the pinmux stuff. In fact, even calculating what those times are probably
> >> > isn't possible right now (for me at least, and the low-level pad designers
> >> > probably have better things to do).
> >>
> >> OK so how is the slew rate specified in the specs you have?
> >> Is it just some magic number? If it turns out all platforms only have
> >> unspecified magic numbers for slew rate settings we'd better
> >> just leave this argument as custom then.
> >
> > Yes. The exact text is e.g.:
> >
> > DRVDN_SLWR - Driver Output Rising Edge Slew 2-bit control code. Code 11
> > is least slewing of signal, code 00 is highest slewing of the signal.
>
> So when you attach some electronics to this SoC, how do you
> choose which value to poke in there? Trial-and-error?
> Again I think we need to understand the things we're trying
> to program. That documentation is lacking is bad, but do we
> really want to design the subsystem around lacking documentation
> instead of deeper understanding?
I addressed this above, but just to repeat: I wouldn't chose any of the
values; it's a HW engineer's responsibility because it's their knob to
tweak. Yes, I imagine sometimes it is trial-and-error.
> The Nomadik GPIO is described like this in the manual for
> AP9500 used in the Snowball (page 775)
> http://www.stericsson.com/developers/DM00030004_AP9500_reference_manual_rev1.pdf
>
> "The GPIO1B_LOWEMI register is used to control the IO pad
> current slew rate. When LOWEMI= '1', the normal slew rate is
> slashed to half of its normal value, us enabling Low EMI mode. In
> normal case, LOWEMI='0'."
>
> So I wouldn't know much about rise and fall times either. But
> "slashed in half" is pretty clear, so I guess the argument might
> be formulated as "fractions of maximum slew rate", so if the slew
> rate can be contolled in 8 steps the argument 7 gives 7/8
> slew rate, argument 2 gives 2/8 = 1/4 slew rate compared to
> nominal etc. This is similar to the proportional number of
> driving stages suggested for PIN_CONFIG_DRIVE_*
Expressing this as a fraction/percentage of max rate might work. But
Then what do you do when the documentation just says low/mid/high? You
could measure it yourself (a hassle), or obtain more documentation (a
hassle), or ...? But again, why do you really care; this isn't a knob
that you're expected to just arbitrarily tweak from software.
> >> > so having two PIN_CONFIG values doesn't make
> >> > sense; we'd have to use the value for e.g. LOW_POWER_MODE to represent the
> >> > desired HW value, and ignore NORMAL_POWER_MODE, I think.
> >>
> >> NORMAL_POWER_MODE is intended to return the pin from any low-power
> >> mode in analogy with say BIAS_DISABLE or DRIVE_OFF, SCHMITT_OFF
> >> so it's to make the code symmetric and readable, not to make the minimal
> >> number of enumerators. I think this is better syntax than say, specifying that a
> >> zero argument to LOW_POWER_MODE means "low power mode off"
> >
> > Maybe this is representing something different than what Tegra's LPMD
> > bits are then.
> >
> > On Tegra, this isn't a kind of "turn off and consume less power" toggle,
> > but rather a way of configuring the pin while it's active; it's a value
> > 0..3 (on Tegra20 at least) that interacts with the other drive strength
> > and slew rate properties and affects overall active pin performance.
>
> Hm, I don't follow this quite... what is it then? How do you select
> the apropriate value in your code?
I don't; a HW engineer would tell me how to configure it (or I leave it at
default).
--
nvpublic
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] pinctrl: add a generic pin config interface
2011-11-21 23:22 ` Stephen Warren
@ 2011-11-22 13:59 ` Mark Brown
2011-11-24 14:19 ` Linus Walleij
0 siblings, 1 reply; 12+ messages in thread
From: Mark Brown @ 2011-11-22 13:59 UTC (permalink / raw)
To: Stephen Warren
Cc: Linus Walleij, Linus Walleij, linux-kernel, Grant Likely,
Barry Song, Shawn Guo
On Mon, Nov 21, 2011 at 03:22:59PM -0800, Stephen Warren wrote:
> Equally, I'm not sure the case you mention is what we should be optimizing
> for. The people who deal with these options most will be FAEs (Field
> Application Engineers), PCB designers, ... who intimately understand
> the details of the SoC they're working with. They'll be well-versed in
> the SoC-specific naming of all these properties. Forcing them to map all
> that knowledge into an all-encompassing abstraction is only going to make
> their life harder, and lead to mistakes.
I'd second this - in my experience detailed setup for these settings
usually comes from hardware engineers in the form of "set register X to
value Y". Having to decode the sematics does make things that little
bit harder.
> > I would compare to how the regulator subsystem infers the
> > allowed voltage range for a certain regulator from the
> > regulator, rail and list of consumer supply limits. It has
> > understanding of the voltages it tries to set.
> That's true, but I think that a regulator outputs a voltage is a much
> better defined and consistent concept than some of the pin config values.
Yes, and we do have some other settings like the modes where the
definition is *much* more shaky and partly as a result the setting is
rarely used (though with a lot of these things the hardware is making
manual selection obsolete).
> > I'm a bit worried if we're writing drivers for hardware where the
> > documentation is so sparse that we don't really know what
> > we're doing... How do you know how to select between say the
> > 1/2 and 1/4 setting today? The subsystem will have a hard time
> > to compensate for lack of engineering documentation whatever
> > it comes to :-(
> I believe either simulations or practical measurement are used to
> determine which option to select (e.g. based on minimal ringing, timing
> spec conformance, etc.), and the SW engineers simply make sure they
> program the value they're told to. While I certainly do like to understand
> everything fully, there are some things that I just let go, and let
> someone decide for me. Almost all of the esoteric settings are the domain
> of HW engineers, and it's their responsibility to fix things if there is
> system instability; kernel engineers shouldn't be too overly concerned I
> think.
This is pretty much it - usually one has a fairly good idea where to go
by default but when actively tuning things usually it's due to some
board specifics where things like the PCB play a role and measurements
are needed to decide what's going on. These systems are incredibly
complex and physical measurement is an ikportant part of the process.
> > > On Tegra, this isn't a kind of "turn off and consume less power" toggle,
> > > but rather a way of configuring the pin while it's active; it's a value
> > > 0..3 (on Tegra20 at least) that interacts with the other drive strength
> > > and slew rate properties and affects overall active pin performance.
> > Hm, I don't follow this quite... what is it then? How do you select
> > the apropriate value in your code?
> I don't; a HW engineer would tell me how to configure it (or I leave it at
> default).
Or that decision may happen as a result of a dialogue between the
engineers working on the project, trading off between software and
hardware behaviours.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] pinctrl: add a generic pin config interface
2011-11-22 13:59 ` Mark Brown
@ 2011-11-24 14:19 ` Linus Walleij
2011-11-24 15:18 ` Mark Brown
0 siblings, 1 reply; 12+ messages in thread
From: Linus Walleij @ 2011-11-24 14:19 UTC (permalink / raw)
To: Mark Brown
Cc: Stephen Warren, Linus Walleij, linux-kernel, Grant Likely,
Barry Song, Shawn Guo
On Tue, Nov 22, 2011 at 2:59 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Mon, Nov 21, 2011 at 03:22:59PM -0800, Stephen Warren wrote:
>> Equally, I'm not sure the case you mention is what we should be optimizing
>> for. The people who deal with these options most will be FAEs (Field
>> Application Engineers), PCB designers, ... who intimately understand
>> the details of the SoC they're working with. They'll be well-versed in
>> the SoC-specific naming of all these properties. Forcing them to map all
>> that knowledge into an all-encompassing abstraction is only going to make
>> their life harder, and lead to mistakes.
>
> I'd second this - in my experience detailed setup for these settings
> usually comes from hardware engineers in the form of "set register X to
> value Y". Having to decode the sematics does make things that little
> bit harder.
I'm a bit reluctant, since the above is sometimes referred to as
"throw over the wall engineering" and is generally considered a bad
way of doing things.
But now that you are two smart guys telling me this I gues I'll
just back out a bit, let's say I make the generic pin configs
optional, select HAVE_GENERIC_PINCONF for those who
are convenient with it, will that fit the bill?
(Generics will be a separate patch anyway.)
> [Stephen]
>> [Me]
>>> I also
>>> want to encode some specific electronic attributes to each
>>> state to achieve some correspondance with the thing that
>>> would appear in textbooks about this kind of stuff.
>>
>> Why does that benefit anyone? Surely most people will take a SoC manual,
>> work out the settings they need using the terminology of that manual,
>> and attempt to set them. I don't think it's the domain of the subsystem
>> to teach people about electronics.
You mean for this subsystem I suppose.
Whereas for the regulator subsystem it is indeed necessary
to understand both software and electronics.
As well as for many things in drivers/staging/iio.
I'm not convinced that pins are all that different, but I'll
attempt to make that knowledge lib optional.
> [Stephen]
>> Why would an engineer working on SoC A need to converse with an engineer
>> working on SoC B about the detailed settings of SoC B?
In Linaro, which is my current project office, this is like, what
we're doing all the time. Not just related to pinconfig. Sure at some
level we can cut out details but well, that level is undefined.
> [Stephen]
>> [Me]
>>> Well I'm trying to check in text books. But how many ways of
>>> designing push/pull can there be (practical, not theoretical)?
>>
>>I have no idea at that level of ASIC design. Given neither of us do,
>>I'd suggest we don't rely on obtaining such knowledge.
Well, actually SoC ASIC construction was my major subject when I
left university, so I do think I can obtain that knowledge, my
main problem right now is finding some old textbooks and talking
to the right people.
This slide from the textbook "Digital Integrated Circuits" for example
touches on some of the basic concepts of pad driver stages mixed up
with some other topics:
http://bwrc.eecs.berkeley.edu/icbook/Slides/chapter9.ppt
I have asked some academic contacts to help me out with
reading lists.
(Maybe I'm taking this too seriously I don't know.)
>>> [Me]
>> [Stephen]
> [Mark]
>> > Hm, I don't follow this quite... what is it then? How do you select
>> > the apropriate value in your code?
>
>> I don't; a HW engineer would tell me how to configure it (or I leave it at
>> default).
>
> Or that decision may happen as a result of a dialogue between the
> engineers working on the project, trading off between software and
> hardware behaviours.
That is less throw over the wall so I like the sound of that :-)
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] pinctrl: add a generic pin config interface
2011-11-24 14:19 ` Linus Walleij
@ 2011-11-24 15:18 ` Mark Brown
0 siblings, 0 replies; 12+ messages in thread
From: Mark Brown @ 2011-11-24 15:18 UTC (permalink / raw)
To: Linus Walleij
Cc: Stephen Warren, Linus Walleij, linux-kernel, Grant Likely,
Barry Song, Shawn Guo
On Thu, Nov 24, 2011 at 03:19:39PM +0100, Linus Walleij wrote:
> On Tue, Nov 22, 2011 at 2:59 PM, Mark Brown
> > I'd second this - in my experience detailed setup for these settings
> > usually comes from hardware engineers in the form of "set register X to
> > value Y". Having to decode the sematics does make things that little
> > bit harder.
> I'm a bit reluctant, since the above is sometimes referred to as
> "throw over the wall engineering" and is generally considered a bad
> way of doing things.
Well, another way of describing the above situation would be the
software engineers going to the hardware engineer and saying that
they've got ringing on a signal (or something) and the hardware
engineer coming back with a suggested fix.
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2011-11-24 15:18 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-11-11 8:31 [PATCH v2] pinctrl: add a generic pin config interface Linus Walleij
2011-11-11 11:26 ` Thomas Abraham
2011-11-14 9:36 ` Linus Walleij
2011-11-14 14:24 ` Thomas Abraham
2011-11-14 19:44 ` Stephen Warren
2011-11-17 13:26 ` Linus Walleij
2011-11-18 22:32 ` Stephen Warren
2011-11-21 19:29 ` Linus Walleij
2011-11-21 23:22 ` Stephen Warren
2011-11-22 13:59 ` Mark Brown
2011-11-24 14:19 ` Linus Walleij
2011-11-24 15:18 ` Mark Brown
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).