LinuxLists.cc - [PATCH v5 00/10] Renesas RZ/A1 pin and gpio controller

[permalink] [raw]

Subject: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable

Add bi-directional and output-enable pin configuration properties.

bi-directional allows to specify when a pin shall operate in input and
output mode at the same time. This is particularly useful in platforms
where input and output buffers have to be manually enabled.

output-enable is just syntactic sugar to specify that a pin shall
operate in output mode, ignoring the provided argument.
This pairs with input-enable pin configuration option.

Signed-off-by: Jacopo Mondi <[email protected]>
---
Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt | 2 ++
drivers/pinctrl/pinconf-generic.c | 3 +++
include/linux/pinctrl/pinconf-generic.h | 3 +++
3 files changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
index bf3f7b0..f2ed458 100644
--- a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
+++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
@@ -222,6 +222,7 @@ bias-bus-hold - latch weakly
bias-pull-up - pull up the pin
bias-pull-down - pull down the pin
bias-pull-pin-default - use pin-default pull state
+bi-directional - pin supports simultaneous input/output operations
drive-push-pull - drive actively high and low
drive-open-drain - drive with open drain
drive-open-source - drive with open source
@@ -234,6 +235,7 @@ input-debounce - debounce mode with debound time X
power-source - select between different power supplies
low-power-enable - enable low power mode
low-power-disable - disable low power mode
+output-enable - enable output on pin regardless of output value
output-low - set the pin to output mode with low level
output-high - set the pin to output mode with high level
slew-rate - set the slew rate
diff --git a/drivers/pinctrl/pinconf-generic.c b/drivers/pinctrl/pinconf-generic.c
index ce3335a..03e6808 100644
--- a/drivers/pinctrl/pinconf-generic.c
+++ b/drivers/pinctrl/pinconf-generic.c
@@ -35,6 +35,7 @@ static const struct pin_config_item conf_items[] = {
PCONFDUMP(PIN_CONFIG_BIAS_PULL_PIN_DEFAULT,
"input bias pull to pin specific state", NULL, false),
PCONFDUMP(PIN_CONFIG_BIAS_PULL_UP, "input bias pull up", NULL, false),
+ PCONFDUMP(PIN_CONFIG_BIDIRECTIONAL, "bi-directional pin operations", NULL, false),
PCONFDUMP(PIN_CONFIG_DRIVE_OPEN_DRAIN, "output drive open drain", NULL, false),
PCONFDUMP(PIN_CONFIG_DRIVE_OPEN_SOURCE, "output drive open source", NULL, false),
PCONFDUMP(PIN_CONFIG_DRIVE_PUSH_PULL, "output drive push pull", NULL, false),
@@ -160,6 +161,7 @@ static const struct pinconf_generic_params dt_params[] = {
{ "bias-pull-up", PIN_CONFIG_BIAS_PULL_UP, 1 },
{ "bias-pull-pin-default", PIN_CONFIG_BIAS_PULL_PIN_DEFAULT, 1 },
{ "bias-pull-down", PIN_CONFIG_BIAS_PULL_DOWN, 1 },
+ { "bi-directional", PIN_CONFIG_BIDIRECTIONAL, 1 },
{ "drive-open-drain", PIN_CONFIG_DRIVE_OPEN_DRAIN, 0 },
{ "drive-open-source", PIN_CONFIG_DRIVE_OPEN_SOURCE, 0 },
{ "drive-push-pull", PIN_CONFIG_DRIVE_PUSH_PULL, 0 },
@@ -172,6 +174,7 @@ static const struct pinconf_generic_params dt_params[] = {
{ "input-schmitt-enable", PIN_CONFIG_INPUT_SCHMITT_ENABLE, 1 },
{ "low-power-disable", PIN_CONFIG_LOW_POWER_MODE, 0 },
{ "low-power-enable", PIN_CONFIG_LOW_POWER_MODE, 1 },
+ { "output-enable", PIN_CONFIG_OUTPUT, 1, },
{ "output-high", PIN_CONFIG_OUTPUT, 1, },
{ "output-low", PIN_CONFIG_OUTPUT, 0, },
{ "power-source", PIN_CONFIG_POWER_SOURCE, 0 },
diff --git a/include/linux/pinctrl/pinconf-generic.h b/include/linux/pinctrl/pinconf-generic.h
index 7620eb1..279e3c5 100644
--- a/include/linux/pinctrl/pinconf-generic.h
+++ b/include/linux/pinctrl/pinconf-generic.h
@@ -42,6 +42,8 @@
* @PIN_CONFIG_BIAS_PULL_UP: the pin will be pulled up (usually with high
* impedance to VDD). If the argument is != 0 pull-up is enabled,
* if it is 0, pull-up is total, i.e. the pin is connected to VDD.
+ * @PIN_CONFIG_BIDIRECTIONAL: the pin will be configured to allow simultaneous
+ * input and output operations.
* @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. Setting this
@@ -96,6 +98,7 @@ enum pin_config_param {
PIN_CONFIG_BIAS_PULL_DOWN,
PIN_CONFIG_BIAS_PULL_PIN_DEFAULT,
PIN_CONFIG_BIAS_PULL_UP,
+ PIN_CONFIG_BIDIRECTIONAL,
PIN_CONFIG_DRIVE_OPEN_DRAIN,
PIN_CONFIG_DRIVE_OPEN_SOURCE,
PIN_CONFIG_DRIVE_PUSH_PULL,
--
2.7.4

2017-04-27 08:20:57

[permalink] [raw]

Subject: [PATCH v5 03/10] pinctrl: Renesas RZ/A1 pin and gpio controller

Add combined gpio and pin controller driver for Renesas RZ/A1
r7s72100 SoC.

Signed-off-by: Jacopo Mondi <[email protected]>
---
drivers/pinctrl/Kconfig | 11 +
drivers/pinctrl/Makefile | 1 +
drivers/pinctrl/pinctrl-rza1.c | 995 +++++++++++++++++++++++++++++++++++++++++
3 files changed, 1007 insertions(+)
create mode 100644 drivers/pinctrl/pinctrl-rza1.c

diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index 8f8c2af..8eb84a9 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -163,6 +163,17 @@ config PINCTRL_ROCKCHIP
select GENERIC_IRQ_CHIP
select MFD_SYSCON

+config PINCTRL_RZA1
+ bool "Renesas RZ/A1 gpio and pinctrl driver"
+ depends on OF
+ depends on ARCH_R7S72100 || COMPILE_TEST
+ select GPIOLIB
+ select GENERIC_PINCTRL_GROUPS
+ select GENERIC_PINMUX_FUNCTIONS
+ select GENERIC_PINCONF
+ help
+ This selects pinctrl driver for Renesas RZ/A1 platforms.
+
config PINCTRL_SINGLE
tristate "One-register-per-pin type device tree based pinctrl driver"
depends on OF
diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
index a251f43..0c2328d2 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -24,6 +24,7 @@ obj-$(CONFIG_PINCTRL_PALMAS) += pinctrl-palmas.o
obj-$(CONFIG_PINCTRL_PIC32) += pinctrl-pic32.o
obj-$(CONFIG_PINCTRL_PISTACHIO) += pinctrl-pistachio.o
obj-$(CONFIG_PINCTRL_ROCKCHIP) += pinctrl-rockchip.o
+obj-$(CONFIG_PINCTRL_RZA1) += pinctrl-rza1.o
obj-$(CONFIG_PINCTRL_SINGLE) += pinctrl-single.o
obj-$(CONFIG_PINCTRL_SIRF) += sirf/
obj-$(CONFIG_PINCTRL_SX150X) += pinctrl-sx150x.o
diff --git a/drivers/pinctrl/pinctrl-rza1.c b/drivers/pinctrl/pinctrl-rza1.c
new file mode 100644
index 0000000..b0f044d
--- /dev/null
+++ b/drivers/pinctrl/pinctrl-rza1.c
@@ -0,0 +1,995 @@
+/*
+ * Combined GPIO and pin controller support for Renesas RZ/A1 (r7s72100) SoC
+ *
+ * Copyright (C) 2017 Jacopo Mondi
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+/*
+ * This pincontroller/gpio combined driver support Renesas devices of RZ/A1
+ * family.
+ * This includes SoCs which are sub- or super- sets of this particular line,
+ * as RZ/A1H (r7s721000), RZ/A1M (r7s721010) and RZ/A1L (r7s721020) are.
+ */
+
+#include <linux/bitops.h>
+#include <linux/err.h>
+#include <linux/gpio/driver.h>
+#include <linux/init.h>
+#include <linux/ioport.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_device.h>
+#include <linux/pinctrl/pinconf-generic.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/pinctrl/pinmux.h>
+#include <linux/slab.h>
+
+#include "core.h"
+#include "devicetree.h"
+#include "pinconf.h"
+#include "pinmux.h"
+
+#define DRIVER_NAME "pinctrl-rza1"
+
+#define RZA1_P_REG 0x0000
+#define RZA1_PPR_REG 0x0200
+#define RZA1_PM_REG 0x0300
+#define RZA1_PMC_REG 0x0400
+#define RZA1_PFC_REG 0x0500
+#define RZA1_PFCE_REG 0x0600
+#define RZA1_PFCEA_REG 0x0a00
+#define RZA1_PIBC_REG 0x4000
+#define RZA1_PBDC_REG 0x4100
+#define RZA1_PIPC_REG 0x4200
+
+#define RZA1_ADDR(mem, reg, port) ((mem) + (reg) + ((port) * 4))
+
+#define RZA1_NPORTS 12
+#define RZA1_PINS_PER_PORT 16
+#define RZA1_NPINS (RZA1_PINS_PER_PORT * RZA1_NPORTS)
+#define RZA1_PIN_ID_TO_PORT(id) ((id) / RZA1_PINS_PER_PORT)
+#define RZA1_PIN_ID_TO_PIN(id) ((id) % RZA1_PINS_PER_PORT)
+
+/*
+ * Use 16 lower bits [15:0] for pin identifier
+ * Use 16 higher bits [31:16] for pin mux function
+ */
+#define MUX_PIN_ID_MASK GENMASK(15, 0)
+#define MUX_FUNC_MASK GENMASK(31, 16)
+
+#define MUX_FUNC_OFFS 16
+#define MUX_FUNC(pinconf) \
+ ((pinconf & MUX_FUNC_MASK) >> MUX_FUNC_OFFS)
+#define MUX_FUNC_PFC_MASK BIT(0)
+#define MUX_FUNC_PFCE_MASK BIT(1)
+#define MUX_FUNC_PFCEA_MASK BIT(2)
+
+/* Pin mux flags: translated from pinconf standard properties */
+#define MUX_FLAGS_BIDIR BIT(0)
+#define MUX_FLAGS_SWIO_INPUT BIT(1)
+#define MUX_FLAGS_SWIO_OUTPUT BIT(2)
+
+/**
+ * rza1_mux_conf - describes a pin multiplexing operation
+ *
+ * @id: the pin identifier from 0 to RZA1_NPINS
+ * @port: the port where pin sits on
+ * @pin: pin id
+ * @mux_func: alternate function id number
+ * @mux_flags: alternate function flags
+ * @value: output value to set the pin to
+ */
+struct rza1_mux_conf {
+ u16 id;
+ u8 port;
+ u8 pin;
+ u8 mux_func;
+ u8 mux_flags;
+ u8 value;
+};
+
+/**
+ * rza1_port - describes a pin port
+ *
+ * This is mostly useful to lock register writes per-bank and not globally.
+ *
+ * @lock: protect access to HW registers
+ * @id: port number
+ * @base: logical address base
+ * @pins: pins sitting on this port
+ */
+struct rza1_port {
+ spinlock_t lock;
+ unsigned int id;
+ void __iomem *base;
+ struct pinctrl_pin_desc *pins;
+};
+
+/**
+ * rza1_pinctrl - RZ pincontroller device
+ *
+ * @dev: parent device structure
+ * @mutex: protect [pinctrl|pinmux]_generic functions
+ * @base: logical address base
+ * @nports: number of pin controller ports
+ * @ports: pin controller banks
+ * @pins: pin array for pinctrl core
+ * @desc: pincontroller desc for pinctrl core
+ * @pctl: pinctrl device
+ */
+struct rza1_pinctrl {
+ struct device *dev;
+
+ struct mutex mutex;
+
+ void __iomem *base;
+
+ unsigned int nport;
+ struct rza1_port *ports;
+
+ struct pinctrl_pin_desc *pins;
+ struct pinctrl_desc desc;
+ struct pinctrl_dev *pctl;
+};
+
+/* ----------------------------------------------------------------------------
+ * RZ/A1 SoC operations
+ */
+
+/**
+ * rza1_set_bit() - un-locked set/clear a single bit in pin configuration
+ * registers
+ */
+static inline void rza1_set_bit(struct rza1_port *port, unsigned int reg,
+ unsigned int bit, bool set)
+{
+ void __iomem *mem = RZA1_ADDR(port->base, reg, port->id);
+ u16 val = ioread16(mem);
+
+ if (set)
+ val |= BIT(bit);
+ else
+ val &= ~BIT(bit);
+
+ iowrite16(val, mem);
+}
+
+static inline unsigned int rza1_get_bit(struct rza1_port *port,
+ unsigned int reg, unsigned int bit)
+{
+ void __iomem *mem = RZA1_ADDR(port->base, reg, port->id);
+
+ return ioread16(mem) & BIT(bit);
+}
+
+/**
+ * rza1_pin_reset() - reset a pin to default initial state
+ *
+ * Reset pin state disabling input buffer and bi-directional control,
+ * and configure it as input port.
+ * Note that pin is now configured with direction as input but with input
+ * buffer disabled. This implies the pin value cannot be read in this state.
+ *
+ * @port: port where pin sits on
+ * @pin: pin offset
+ */
+static void rza1_pin_reset(struct rza1_port *port, unsigned int pin)
+{
+ unsigned long irqflags;
+
+ spin_lock_irqsave(&port->lock, irqflags);
+ rza1_set_bit(port, RZA1_PIBC_REG, pin, 0);
+ rza1_set_bit(port, RZA1_PBDC_REG, pin, 0);
+
+ rza1_set_bit(port, RZA1_PM_REG, pin, 1);
+ rza1_set_bit(port, RZA1_PMC_REG, pin, 0);
+ rza1_set_bit(port, RZA1_PIPC_REG, pin, 0);
+ spin_unlock_irqrestore(&port->lock, irqflags);
+}
+
+static inline int rza1_pin_get_direction(struct rza1_port *port,
+ unsigned int pin)
+{
+ unsigned long irqflags;
+ int input;
+
+ spin_lock_irqsave(&port->lock, irqflags);
+ input = rza1_get_bit(port, RZA1_PM_REG, pin);
+ spin_unlock_irqrestore(&port->lock, irqflags);
+
+ return !!input;
+}
+
+/**
+ * rza1_pin_set_direction() - set I/O direction on a pin in port mode
+ *
+ * When running in output port mode keep PBDC enabled to allow reading the
+ * pin value from PPR.
+ *
+ * @port: port where pin sits on
+ * @pin: pin offset
+ * @input: input enable/disable flag
+ */
+static inline void rza1_pin_set_direction(struct rza1_port *port,
+ unsigned int pin, bool input)
+{
+ unsigned long irqflags;
+
+ spin_lock_irqsave(&port->lock, irqflags);
+
+ rza1_set_bit(port, RZA1_PIBC_REG, pin, 1);
+ if (input) {
+ rza1_set_bit(port, RZA1_PM_REG, pin, 1);
+ rza1_set_bit(port, RZA1_PBDC_REG, pin, 0);
+ } else {
+ rza1_set_bit(port, RZA1_PM_REG, pin, 0);
+ rza1_set_bit(port, RZA1_PBDC_REG, pin, 1);
+ }
+
+ spin_unlock_irqrestore(&port->lock, irqflags);
+}
+
+static inline void rza1_pin_set(struct rza1_port *port, unsigned int pin,
+ unsigned int value)
+{
+ unsigned long irqflags;
+
+ spin_lock_irqsave(&port->lock, irqflags);
+ rza1_set_bit(port, RZA1_P_REG, pin, !!value);
+ spin_unlock_irqrestore(&port->lock, irqflags);
+}
+
+static inline int rza1_pin_get(struct rza1_port *port, unsigned int pin)
+{
+ unsigned long irqflags;
+ int val;
+
+ spin_lock_irqsave(&port->lock, irqflags);
+ val = rza1_get_bit(port, RZA1_PPR_REG, pin);
+ spin_unlock_irqrestore(&port->lock, irqflags);
+
+ return val;
+}
+
+/**
+ * rza1_pin_mux_single() - configure pin multiplexing on a single pin
+ *
+ * @pinctrl: RZ/A1 pin controller device
+ * @mux_conf: pin multiplexing descriptor
+ */
+static int rza1_pin_mux_single(struct rza1_pinctrl *rza1_pctl,
+ struct rza1_mux_conf *mux_conf)
+{
+ struct rza1_port *port = &rza1_pctl->ports[mux_conf->port];
+ unsigned int pin = mux_conf->pin;
+ u8 mux_func = mux_conf->mux_func - 1;
+ u8 mux_flags = mux_conf->mux_flags;
+
+ rza1_pin_reset(port, pin);
+
+ if (mux_flags & MUX_FLAGS_BIDIR)
+ rza1_set_bit(port, RZA1_PBDC_REG, pin, 1);
+
+ /*
+ * Enable alternate function mode and select it.
+ *
+ * Be careful here: the pin mux sub-nodes in device tree
+ * enumerate alternate functions from 1 to 8;
+ * subtract 1 before using macros to match registers configuration
+ * which expects numbers from 0 to 7 instead.
+ *
+ * ----------------------------------------------------
+ * Alternate mode selection table:
+ *
+ * PMC PFC PFCE PFCAE (mux_func - 1)
+ * 1 0 0 0 0
+ * 1 1 0 0 1
+ * 1 0 1 0 2
+ * 1 1 1 0 3
+ * 1 0 0 1 4
+ * 1 1 0 1 5
+ * 1 0 1 1 6
+ * 1 1 1 1 7
+ * ----------------------------------------------------
+ */
+ rza1_set_bit(port, RZA1_PFC_REG, pin, mux_func & MUX_FUNC_PFC_MASK);
+ rza1_set_bit(port, RZA1_PFCE_REG, pin, mux_func & MUX_FUNC_PFCE_MASK);
+ rza1_set_bit(port, RZA1_PFCEA_REG, pin, mux_func & MUX_FUNC_PFCEA_MASK);
+
+ /*
+ * All alternate functions except a few need PIPCn = 1.
+ * If PIPCn has to stay disabled (SW IO mode), configure PMn according
+ * to I/O direction specified by pin configuration -after- PMC has been
+ * set to one.
+ */
+ if (!(mux_flags & (MUX_FLAGS_SWIO_INPUT | MUX_FLAGS_SWIO_OUTPUT)))
+ rza1_set_bit(port, RZA1_PIPC_REG, pin, 1);
+
+ rza1_set_bit(port, RZA1_PMC_REG, pin, 1);
+ rza1_set_bit(port, RZA1_PM_REG, pin, mux_flags & MUX_FLAGS_SWIO_INPUT);
+
+ return 0;
+}
+
+/* ----------------------------------------------------------------------------
+ * gpio operations
+ */
+
+/**
+ * rza1_gpio_request() - configure pin in port mode
+ *
+ * Configure a pin as gpio (port mode).
+ * After reset, the pin is in input mode with input buffer disabled.
+ * To use the pin as input or output, set_direction shall be called first
+ *
+ * @chip: gpio chip where the gpio sits on
+ * @gpio: gpio offset
+ */
+static int rza1_gpio_request(struct gpio_chip *chip, unsigned int gpio)
+{
+ struct rza1_port *port = gpiochip_get_data(chip);
+
+ rza1_pin_reset(port, gpio);
+
+ return 0;
+}
+
+/**
+ * rza1_gpio_disable_free() - reset a pin
+ *
+ * Surprisingly, disable_free a gpio, is equivalent to request it.
+ * Reset pin to port mode, with input buffer disabled. This overwrites all
+ * port direction settings applied with set_direction
+ *
+ * @chip: gpio chip where the gpio sits on
+ * @gpio: gpio offset
+ */
+static void rza1_gpio_free(struct gpio_chip *chip, unsigned int gpio)
+{
+ struct rza1_port *port = gpiochip_get_data(chip);
+
+ rza1_pin_reset(port, gpio);
+}
+
+static int rza1_gpio_get_direction(struct gpio_chip *chip, unsigned int gpio)
+{
+ struct rza1_port *port = gpiochip_get_data(chip);
+
+ return rza1_pin_get_direction(port, gpio);
+}
+
+static int rza1_gpio_direction_input(struct gpio_chip *chip,
+ unsigned int gpio)
+{
+ struct rza1_port *port = gpiochip_get_data(chip);
+
+ rza1_pin_set_direction(port, gpio, true);
+
+ return 0;
+}
+
+static int rza1_gpio_direction_output(struct gpio_chip *chip,
+ unsigned int gpio,
+ int value)
+{
+ struct rza1_port *port = gpiochip_get_data(chip);
+
+ /* Set value before driving pin direction */
+ rza1_pin_set(port, gpio, value);
+ rza1_pin_set_direction(port, gpio, false);
+
+ return 0;
+}
+
+/**
+ * rza1_gpio_get() - read a gpio pin value
+ *
+ * Read gpio pin value through PPR register.
+ * Requires bi-directional mode to work when reading the value of a pin
+ * in output mode
+ *
+ * @chip: gpio chip where the gpio sits on
+ * @gpio: gpio offset
+ */
+static int rza1_gpio_get(struct gpio_chip *chip, unsigned int gpio)
+{
+ struct rza1_port *port = gpiochip_get_data(chip);
+
+ return rza1_pin_get(port, gpio);
+}
+
+static void rza1_gpio_set(struct gpio_chip *chip, unsigned int gpio,
+ int value)
+{
+ struct rza1_port *port = gpiochip_get_data(chip);
+
+ rza1_pin_set(port, gpio, value);
+}
+
+struct gpio_chip rza1_gpiochip_template = {
+ .request = rza1_gpio_request,
+ .free = rza1_gpio_free,
+ .get_direction = rza1_gpio_get_direction,
+ .direction_input = rza1_gpio_direction_input,
+ .direction_output = rza1_gpio_direction_output,
+ .get = rza1_gpio_get,
+ .set = rza1_gpio_set,
+};
+/* ----------------------------------------------------------------------------
+ * pinctrl operations
+ */
+
+/**
+ * rza1_dt_node_pin_count() - Count number of pins in a dt node or in all its
+ * children sub-nodes
+ *
+ * @np: device tree node to parse
+ */
+static int rza1_dt_node_pin_count(struct device_node *np)
+{
+ struct device_node *child;
+ struct property *of_pins;
+ unsigned int npins;
+
+ of_pins = of_find_property(np, "pinmux", NULL);
+ if (of_pins)
+ return of_pins->length / sizeof(u32);
+
+ npins = 0;
+ for_each_child_of_node(np, child) {
+ of_pins = of_find_property(child, "pinmux", NULL);
+ if (!of_pins)
+ return -EINVAL;
+
+ npins += of_pins->length / sizeof(u32);
+ }
+
+ return npins;
+}
+
+/**
+ * rza1_parse_pmx_function() - parse a pin mux sub-node
+ *
+ * @rza1_pctl: RZ/A1 pin controller device
+ * @np: of pmx sub-node
+ * @mux_confs: array of pin mux configurations to fill with parsed info
+ * @grpins: array of pin ids to mux
+ */
+static int rza1_parse_pinmux_node(struct rza1_pinctrl *rza1_pctl,
+ struct device_node *np,
+ struct rza1_mux_conf *mux_confs,
+ unsigned int *grpins)
+{
+ struct pinctrl_dev *pctldev = rza1_pctl->pctl;
+ char const *prop_name = "pinmux";
+ unsigned long *pin_configs;
+ unsigned int npin_configs;
+ struct property *of_pins;
+ unsigned int npins;
+ u8 pinmux_flags;
+ unsigned int i;
+ int ret;
+
+ of_pins = of_find_property(np, prop_name, NULL);
+ if (!of_pins) {
+ dev_err(rza1_pctl->dev, "Missing %s property\n", prop_name);
+ return -ENOENT;
+ }
+ npins = of_pins->length / sizeof(u32);
+
+ /*
+ * Collect pin configuration properties: they apply to all pins in
+ * this sub-node
+ */
+ ret = pinconf_generic_parse_dt_config(np, pctldev, &pin_configs,
+ &npin_configs);
+ if (ret) {
+ dev_err(rza1_pctl->dev,
+ "Unable to parse pin configuration options for %s\n",
+ np->name);
+ return ret;
+ }
+
+ /*
+ * Create a mask with pinmux flags from pin configuration;
+ * only allow a single flag to be set (the first encountered one)
+ */
+ pinmux_flags = 0;
+ for (i = 0; i < npin_configs && pinmux_flags == 0; i++)
+ switch (PIN_CONF_UNPACK_PARAM(pin_configs[i])) {
+ case PIN_CONFIG_BIDIRECTIONAL:
+ pinmux_flags |= MUX_FLAGS_BIDIR;
+ break;
+ case PIN_CONFIG_INPUT_ENABLE:
+ pinmux_flags |= MUX_FLAGS_SWIO_INPUT;
+ break;
+ case PIN_CONFIG_OUTPUT:
+ pinmux_flags |= MUX_FLAGS_SWIO_OUTPUT;
+ break;
+ }
+
+ kfree(pin_configs);
+
+ /* Collect pin positions and their mux settings. */
+ for (i = 0; i < npins; ++i) {
+ u32 of_pinconf;
+ struct rza1_mux_conf *mux_conf = &mux_confs[i];
+
+ ret = of_property_read_u32_index(np, prop_name, i, &of_pinconf);
+ if (ret)
+ return ret;
+
+ mux_conf->id = of_pinconf & MUX_PIN_ID_MASK;
+ mux_conf->port = RZA1_PIN_ID_TO_PORT(mux_conf->id);
+ mux_conf->pin = RZA1_PIN_ID_TO_PIN(mux_conf->id);
+ mux_conf->mux_func = MUX_FUNC(of_pinconf);
+ mux_conf->mux_flags = pinmux_flags;
+
+ if (mux_conf->port >= RZA1_NPORTS ||
+ mux_conf->pin >= RZA1_PINS_PER_PORT) {
+ dev_err(rza1_pctl->dev,
+ "Wrong port %u pin %u for %s property\n",
+ mux_conf->port, mux_conf->pin, prop_name);
+ return -EINVAL;
+ }
+
+ grpins[i] = mux_conf->id;
+ }
+
+ return npins;
+}
+
+/**
+ * rza1_dt_node_to_map() - map a pin mux node to a function/group
+ *
+ * Parse and register a pin mux function.
+ *
+ * @pctldev: pin controller device
+ * @np: device tree node to parse
+ * @map: pointer to pin map (output)
+ * @num_maps: number of collected maps (output)
+ */
+static int rza1_dt_node_to_map(struct pinctrl_dev *pctldev,
+ struct device_node *np,
+ struct pinctrl_map **map,
+ unsigned int *num_maps)
+{
+ struct rza1_pinctrl *rza1_pctl = pinctrl_dev_get_drvdata(pctldev);
+ struct rza1_mux_conf *mux_confs, *mux_conf;
+ unsigned int *grpins, *grpin;
+ struct device_node *child;
+ const char *grpname;
+ const char **fngrps;
+ int ret, npins;
+
+ npins = rza1_dt_node_pin_count(np);
+ if (npins < 0) {
+ dev_err(rza1_pctl->dev, "invalid pinmux node structure\n");
+ return -EINVAL;
+ }
+
+ /*
+ * Functions are made of 1 group only;
+ * in fact, functions and groups are identical for this pin controller
+ * except that functions carry an array of per-pin mux configuration
+ * settings.
+ */
+ mux_confs = devm_kcalloc(rza1_pctl->dev, npins, sizeof(*mux_confs),
+ GFP_KERNEL);
+ grpins = devm_kcalloc(rza1_pctl->dev, npins, sizeof(*grpins),
+ GFP_KERNEL);
+ fngrps = devm_kzalloc(rza1_pctl->dev, sizeof(*fngrps), GFP_KERNEL);
+
+ if (!mux_confs || !grpins || !fngrps)
+ return -ENOMEM;
+
+ /*
+ * Parse the pinmux node.
+ * If the node does not contain "pinmux" property (-ENOENT)
+ * that property shall be specified in all its children sub-nodes.
+ */
+ mux_conf = &mux_confs[0];
+ grpin = &grpins[0];
+
+ ret = rza1_parse_pinmux_node(rza1_pctl, np, mux_conf, grpin);
+ if (ret == -ENOENT)
+ for_each_child_of_node(np, child) {
+ ret = rza1_parse_pinmux_node(rza1_pctl, child, mux_conf,
+ grpin);
+ if (ret < 0)
+ return ret;
+
+ grpin += ret;
+ mux_conf += ret;
+ }
+ else if (ret < 0)
+ return ret;
+
+ /* Register pin group and function name to pinctrl_generic */
+ grpname = np->name;
+ fngrps[0] = grpname;
+
+ mutex_lock(&rza1_pctl->mutex);
+ ret = pinctrl_generic_add_group(pctldev, grpname, grpins, npins,
+ NULL);
+ if (ret) {
+ mutex_unlock(&rza1_pctl->mutex);
+ return ret;
+ }
+
+ ret = pinmux_generic_add_function(pctldev, grpname, fngrps, 1,
+ mux_confs);
+ if (ret)
+ goto remove_group;
+ mutex_unlock(&rza1_pctl->mutex);
+
+ dev_info(rza1_pctl->dev, "Parsed function and group %s with %d pins\n",
+ grpname, npins);
+
+ /* Create map where to retrieve function and mux settings from */
+ *num_maps = 0;
+ *map = kzalloc(sizeof(**map), GFP_KERNEL);
+ if (!*map) {
+ ret = -ENOMEM;
+ goto remove_function;
+ }
+
+ (*map)->type = PIN_MAP_TYPE_MUX_GROUP;
+ (*map)->data.mux.group = np->name;
+ (*map)->data.mux.function = np->name;
+ *num_maps = 1;
+
+ return 0;
+
+remove_function:
+ mutex_lock(&rza1_pctl->mutex);
+ pinmux_generic_remove_last_function(pctldev);
+
+remove_group:
+ pinctrl_generic_remove_last_group(pctldev);
+ mutex_unlock(&rza1_pctl->mutex);
+
+ dev_info(rza1_pctl->dev, "Unable to parse function and group %s\n",
+ grpname);
+
+ return ret;
+}
+
+static void rza1_dt_free_map(struct pinctrl_dev *pctldev,
+ struct pinctrl_map *map, unsigned int num_maps)
+{
+ kfree(map);
+}
+
+static const struct pinctrl_ops rza1_pinctrl_ops = {
+ .get_groups_count = pinctrl_generic_get_group_count,
+ .get_group_name = pinctrl_generic_get_group_name,
+ .get_group_pins = pinctrl_generic_get_group_pins,
+ .dt_node_to_map = rza1_dt_node_to_map,
+ .dt_free_map = rza1_dt_free_map,
+};
+
+/* ----------------------------------------------------------------------------
+ * pinmux operations
+ */
+
+/**
+ * rza1_set_mux() - retrieve pins from a group and apply their mux settings
+ *
+ * @pctldev: pin controller device
+ * @selector: function selector
+ * @group: group selector
+ */
+static int rza1_set_mux(struct pinctrl_dev *pctldev, unsigned int selector,
+ unsigned int group)
+{
+ struct rza1_pinctrl *rza1_pctl = pinctrl_dev_get_drvdata(pctldev);
+ struct rza1_mux_conf *mux_confs;
+ struct function_desc *func;
+ struct group_desc *grp;
+ int i;
+
+ grp = pinctrl_generic_get_group(pctldev, group);
+ if (!grp)
+ return -EINVAL;
+
+ func = pinmux_generic_get_function(pctldev, selector);
+ if (!func)
+ return -EINVAL;
+
+ mux_confs = (struct rza1_mux_conf *)func->data;
+ for (i = 0; i < grp->num_pins; ++i) {
+ int ret;
+
+ ret = rza1_pin_mux_single(rza1_pctl, &mux_confs[i]);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+
+struct pinmux_ops rza1_pinmux_ops = {
+ .get_functions_count = pinmux_generic_get_function_count,
+ .get_function_name = pinmux_generic_get_function_name,
+ .get_function_groups = pinmux_generic_get_function_groups,
+ .set_mux = rza1_set_mux,
+ .strict = true,
+};
+
+/* ----------------------------------------------------------------------------
+ * RZ/A1 pin controller driver operations
+ */
+
+static unsigned int rza1_count_gpio_chips(struct device_node *np)
+{
+ struct device_node *child;
+ unsigned int count = 0;
+
+ for_each_child_of_node(np, child) {
+ if (!of_property_read_bool(child, "gpio-controller"))
+ continue;
+
+ count++;
+ }
+
+ return count;
+}
+
+/**
+ * rza1_parse_gpiochip() - parse and register a gpio chip and pin range
+ *
+ * The gpio controller subnode shall provide a "gpio-ranges" list property as
+ * defined by gpio device tree binding documentation.
+ *
+ * @rza1_pctl: RZ/A1 pin controller device
+ * @np: of gpio-controller node
+ * @chip: gpio chip to register to gpiolib
+ * @range: pin range to register to pinctrl core
+ */
+static int rza1_parse_gpiochip(struct rza1_pinctrl *rza1_pctl,
+ struct device_node *np,
+ struct gpio_chip *chip,
+ struct pinctrl_gpio_range *range)
+{
+ const char *list_name = "gpio-ranges";
+ struct of_phandle_args of_args;
+ unsigned int gpioport;
+ u32 pinctrl_base;
+ int ret;
+
+ ret = of_parse_phandle_with_fixed_args(np, list_name, 3, 0, &of_args);
+ if (ret) {
+ dev_err(rza1_pctl->dev, "Unable to parse %s list property\n",
+ list_name);
+ return ret;
+ }
+
+ /*
+ * Find out on which port this gpio-chip maps to by inspecting the
+ * second argument of the "gpio-ranges" property.
+ */
+ pinctrl_base = of_args.args[1];
+ gpioport = RZA1_PIN_ID_TO_PORT(pinctrl_base);
+ if (gpioport > RZA1_NPORTS) {
+ dev_err(rza1_pctl->dev,
+ "Invalid values in property %s\n", list_name);
+ return -EINVAL;
+ }
+
+ *chip = rza1_gpiochip_template;
+ chip->base = -1;
+ chip->label = devm_kasprintf(rza1_pctl->dev, GFP_KERNEL, "%s-%u",
+ np->name, gpioport);
+ chip->ngpio = of_args.args[2];
+ chip->of_node = np;
+ chip->parent = rza1_pctl->dev;
+
+ range->id = gpioport;
+ range->name = chip->label;
+ range->pin_base = range->base = pinctrl_base;
+ range->npins = of_args.args[2];
+ range->gc = chip;
+
+ ret = devm_gpiochip_add_data(rza1_pctl->dev, chip,
+ &rza1_pctl->ports[gpioport]);
+ if (ret)
+ return ret;
+
+ pinctrl_add_gpio_range(rza1_pctl->pctl, range);
+
+ dev_info(rza1_pctl->dev, "Parsed gpiochip %s with %d pins\n",
+ chip->label, chip->ngpio);
+
+ return 0;
+}
+
+/**
+ * rza1_gpio_register() - parse DT to collect gpio-chips and gpio-ranges
+ *
+ * @rza1_pctl: RZ/A1 pin controller device
+ */
+static int rza1_gpio_register(struct rza1_pinctrl *rza1_pctl)
+{
+ struct device_node *np = rza1_pctl->dev->of_node;
+ struct pinctrl_gpio_range *gpio_ranges;
+ struct gpio_chip *gpio_chips;
+ struct device_node *child;
+ unsigned int ngpiochips;
+ unsigned int i;
+ int ret;
+
+ ngpiochips = rza1_count_gpio_chips(np);
+ if (ngpiochips == 0) {
+ dev_dbg(rza1_pctl->dev, "No gpiochip registered\n");
+ return 0;
+ }
+
+ gpio_chips = devm_kcalloc(rza1_pctl->dev, ngpiochips,
+ sizeof(*gpio_chips), GFP_KERNEL);
+ gpio_ranges = devm_kcalloc(rza1_pctl->dev, ngpiochips,
+ sizeof(*gpio_ranges), GFP_KERNEL);
+ if (!gpio_chips || !gpio_ranges)
+ return -ENOMEM;
+
+ i = 0;
+ for_each_child_of_node(np, child) {
+ if (!of_property_read_bool(child, "gpio-controller"))
+ continue;
+
+ ret = rza1_parse_gpiochip(rza1_pctl, child, &gpio_chips[i],
+ &gpio_ranges[i]);
+ if (ret)
+ goto gpiochip_remove;
+
+ ++i;
+ }
+
+ dev_info(rza1_pctl->dev, "Registered %u gpio controllers\n", i);
+
+ return 0;
+
+gpiochip_remove:
+ for (; i > 0; i--)
+ devm_gpiochip_remove(rza1_pctl->dev, &gpio_chips[i - 1]);
+
+ return ret;
+}
+
+/**
+ * rza1_pinctrl_register() - Enumerate pins, ports and gpiochips; register
+ * them to pinctrl and gpio cores.
+ *
+ * @rza1_pctl: RZ/A1 pin controller device
+ */
+static int rza1_pinctrl_register(struct rza1_pinctrl *rza1_pctl)
+{
+ struct pinctrl_pin_desc *pins;
+ struct rza1_port *ports;
+ unsigned int i;
+ int ret;
+
+ pins = devm_kcalloc(rza1_pctl->dev, RZA1_NPINS, sizeof(*pins),
+ GFP_KERNEL);
+ ports = devm_kcalloc(rza1_pctl->dev, RZA1_NPORTS, sizeof(*ports),
+ GFP_KERNEL);
+ if (!pins || !ports)
+ return -ENOMEM;
+
+ rza1_pctl->pins = pins;
+ rza1_pctl->desc.pins = pins;
+ rza1_pctl->desc.npins = RZA1_NPINS;
+ rza1_pctl->ports = ports;
+
+ for (i = 0; i < RZA1_NPINS; ++i) {
+ unsigned int pin = RZA1_PIN_ID_TO_PIN(i);
+ unsigned int port = RZA1_PIN_ID_TO_PORT(i);
+
+ pins[i].number = i;
+ pins[i].name = devm_kasprintf(rza1_pctl->dev, GFP_KERNEL,
+ "P%u-%u", port, pin);
+
+ if (i % RZA1_PINS_PER_PORT == 0) {
+ /*
+ * Setup ports;
+ * they provide per-port lock and logical base address.
+ */
+ unsigned int port_id = RZA1_PIN_ID_TO_PORT(i);
+
+ ports[port_id].id = port_id;
+ ports[port_id].base = rza1_pctl->base;
+ ports[port_id].pins = &pins[i];
+ spin_lock_init(&ports[port_id].lock);
+ }
+ }
+
+ ret = devm_pinctrl_register_and_init(rza1_pctl->dev, &rza1_pctl->desc,
+ rza1_pctl, &rza1_pctl->pctl);
+ if (ret) {
+ dev_err(rza1_pctl->dev,
+ "RZ/A1 pin controller registration failed\n");
+ return ret;
+ }
+
+ ret = pinctrl_enable(rza1_pctl->pctl);
+ if (ret) {
+ dev_err(rza1_pctl->dev,
+ "RZ/A1 pin controller failed to start\n");
+ return ret;
+ }
+
+ ret = rza1_gpio_register(rza1_pctl);
+ if (ret) {
+ dev_err(rza1_pctl->dev, "RZ/A1 GPIO registration failed\n");
+ return ret;
+ }
+
+ return 0;
+}
+
+static int rza1_pinctrl_probe(struct platform_device *pdev)
+{
+ struct rza1_pinctrl *rza1_pctl;
+ struct resource *res;
+ int ret;
+
+ rza1_pctl = devm_kzalloc(&pdev->dev, sizeof(*rza1_pctl), GFP_KERNEL);
+ if (!rza1_pctl)
+ return -ENOMEM;
+
+ rza1_pctl->dev = &pdev->dev;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (ret)
+ return -ENODEV;
+
+ rza1_pctl->base = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(rza1_pctl->base))
+ return PTR_ERR(rza1_pctl->base);
+
+ mutex_init(&rza1_pctl->mutex);
+
+ platform_set_drvdata(pdev, rza1_pctl);
+
+ rza1_pctl->desc.name = DRIVER_NAME;
+ rza1_pctl->desc.pctlops = &rza1_pinctrl_ops;
+ rza1_pctl->desc.pmxops = &rza1_pinmux_ops;
+ rza1_pctl->desc.owner = THIS_MODULE;
+
+ ret = rza1_pinctrl_register(rza1_pctl);
+ if (ret)
+ return ret;
+
+ dev_info(&pdev->dev,
+ "RZ/A1 pin controller and gpio successfully registered\n");
+
+ return 0;
+}
+
+static const struct of_device_id rza1_pinctrl_of_match[] = {
+ { .compatible = "renesas,r7s72100-ports", },
+ { }
+};
+
+static struct platform_driver rza1_pinctrl_driver = {
+ .driver = {
+ .name = DRIVER_NAME,
+ .of_match_table = rza1_pinctrl_of_match,
+ },
+ .probe = rza1_pinctrl_probe,
+};
+
+static int __init rza1_pinctrl_init(void)
+{
+ return platform_driver_register(&rza1_pinctrl_driver);
+}
+core_initcall(rza1_pinctrl_init);
+
+MODULE_AUTHOR("Jacopo Mondi <[email protected]");
+MODULE_DESCRIPTION("Pin and gpio controller driver for Reneas RZ/A1 SoC");
+MODULE_LICENSE("GPL v2");
--
2.7.4

2017-04-27 08:21:22

[permalink] [raw]

Subject: [PATCH v5 04/10] dt-bindings: pinctrl: Add RZ/A1 bindings doc

Add device tree bindings documentation for Renesas RZ/A1 gpio and pin
controller.

Signed-off-by: Jacopo Mondi <[email protected]>
---
.../bindings/pinctrl/renesas,rza1-pinctrl.txt | 219 +++++++++++++++++++++
1 file changed, 219 insertions(+)
create mode 100644 Documentation/devicetree/bindings/pinctrl/renesas,rza1-pinctrl.txt

diff --git a/Documentation/devicetree/bindings/pinctrl/renesas,rza1-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/renesas,rza1-pinctrl.txt
new file mode 100644
index 0000000..5a1106d
--- /dev/null
+++ b/Documentation/devicetree/bindings/pinctrl/renesas,rza1-pinctrl.txt
@@ -0,0 +1,219 @@
+Renesas RZ/A1 combined Pin and GPIO controller
+
+The Renesas SoCs of the RZ/A1 family feature a combined Pin and GPIO controller,
+named "Ports" in the hardware reference manual.
+Pin multiplexing and GPIO configuration is performed on a per-pin basis
+writing configuration values to per-port register sets.
+Each "port" features up to 16 pins, each of them configurable for GPIO
+function (port mode) or in alternate function mode.
+Up to 8 different alternate function modes exist for each single pin.
+
+Pin controller node
+-------------------
+
+Required properties:
+ - compatible
+ this shall be "renesas,r7s72100-ports".
+
+ - reg
+ address base and length of the memory area where the pin controller
+ hardware is mapped to.
+
+Example:
+Pin controller node for RZ/A1H SoC (r7s72100)
+
+pinctrl: pin-controller@fcfe3000 {
+ compatible = "renesas,r7s72100-ports";
+
+ reg = <0xfcfe3000 0x4230>;
+};
+
+Sub-nodes
+---------
+
+The child nodes of the pin controller node describe a pin multiplexing
+function or a GPIO controller alternatively.
+
+- Pin multiplexing sub-nodes:
+ A pin multiplexing sub-node describes how to configure a set of
+ (or a single) pin in some desired alternate function mode.
+ A single sub-node may define several pin configurations.
+ Some alternate functions require special pin configuration flags to be
+ supplied along with the alternate function configuration number.
+ When the hardware reference manual specifies a pin function to be either
+ "bi-directional" or "software IO driven", use the generic properties from
+ the <include/linux/pinctrl/pinconf_generic.h> header file to instruct the
+ pin controller to perform the desired pin configuration operations.
+ Please refer to pinctrl-bindings.txt to get to know more on generic
+ pin properties usage.
+
+ The allowed generic formats for a pin multiplexing sub-node are the
+ following ones:
+
+ node-1 {
+ pinmux = <PIN_ID_AND_MUX>, <PIN_ID_AND_MUX>, ... ;
+ GENERIC_PINCONFIG;
+ };
+
+ node-2 {
+ sub-node-1 {
+ pinmux = <PIN_ID_AND_MUX>, <PIN_ID_AND_MUX>, ... ;
+ GENERIC_PINCONFIG;
+ };
+
+ sub-node-2 {
+ pinmux = <PIN_ID_AND_MUX>, <PIN_ID_AND_MUX>, ... ;
+ GENERIC_PINCONFIG;
+ };
+
+ ...
+
+ sub-node-n {
+ pinmux = <PIN_ID_AND_MUX>, <PIN_ID_AND_MUX>, ... ;
+ GENERIC_PINCONFIG;
+ };
+ };
+
+ Use the second format when pins part of the same logical group need to have
+ different generic pin configuration flags applied.
+
+ Client sub-nodes shall refer to pin multiplexing sub-nodes using the phandle
+ of the most external one.
+
+ Eg.
+
+ client-1 {
+ ...
+ pinctrl-0 = <&node-1>;
+ ...
+ };
+
+ client-2 {
+ ...
+ pinctrl-0 = <&node-2>;
+ ...
+ };
+
+ Required properties:
+ - pinmux:
+ integer array representing pin number and pin multiplexing configuration.
+ When a pin has to be configured in alternate function mode, use this
+ property to identify the pin by its global index, and provide its
+ alternate function configuration number along with it.
+ When multiple pins are required to be configured as part of the same
+ alternate function they shall be specified as members of the same
+ argument list of a single "pinmux" property.
+ Helper macros to ease assembling the pin index from its position
+ (port where it sits on and pin number) and alternate function identifier
+ are provided by the pin controller header file at:
+ <include/dt-bindings/pinctrl/r7s72100-pinctrl.h>
+ Integers values in "pinmux" argument list are assembled as:
+ ((PORT * 16 + PIN) | MUX_FUNC << 16)
+
+ Optional generic properties:
+ - bi-directional:
+ for pins requiring bi-directional operations.
+ - input-enable:
+ for pins requiring software driven IO input operations.
+ - output-enable:
+ for pins requiring software driven IO output operations.
+
+ The hardware reference manual specifies when a pin has to be configured to
+ work in bi-directional mode and when the IO direction has to be specified
+ by software.
+
+ Example:
+ A serial communication interface with a TX output pin and an RX input pin.
+
+ &pinctrl {
+ scif2_pins: serial2 {
+ pinmux = <RZA1_PINMUX(3, 0, 6)>, <RZA1_PINMUX(3, 2, 4)>;
+ };
+ };
+
+ Pin #0 on port #3 is configured as alternate function #6.
+ Pin #2 on port #3 is configured as alternate function #4.
+
+ Example 2:
+ I2c master: both SDA and SCL pins need bi-directional operations
+
+ &pinctrl {
+ i2c2_pins: i2c2 {
+ pinmux = <RZA1_PINMUX(1, 4, 1)>, <RZA1_PINMUX(1, 5, 1)>;
+ bi-directional;
+ };
+ };
+
+ Pin #4 on port #1 is configured as alternate function #1.
+ Pin #5 on port #1 is configured as alternate function #1.
+ Both need to work in bi-directional mode.
+
+ Example 3:
+ Multi-function timer input and output compare pins.
+ Configure TIOC0A as software driven input and TIOC0B as software driven
+ output.
+
+ &pinctrl {
+ tioc0_pins: tioc0 {
+ tioc0_input_pins {
+ pinumx = <RZA1_PINMUX(4, 0, 2)>;
+ input-enable;
+ };
+
+ tioc0_output_pins {
+ pinmux = <RZA1_PINMUX(4, 1, 1)>;
+ output-enable;
+ };
+ };
+ };
+
+
+ &tioc0 {
+ ...
+ pinctrl-0 = <&tioc0_pins>;
+ ...
+ };
+
+ Pin #0 on port #4 is configured as alternate function #2 with IO direction
+ specified by software as input.
+ Pin #1 on port #4 is configured as alternate function #1 with IO direction
+ specified by software as output.
+
+- GPIO controller sub-nodes:
+ Each port of the r7s72100 pin controller hardware is itself a GPIO controller.
+ Different SoCs have different numbers of available pins per port, but
+ generally speaking, each of them can be configured in GPIO ("port") mode
+ on this hardware.
+ Describe GPIO controllers using sub-nodes with the following properties.
+
+ Required properties:
+ - gpio-controller
+ empty property as defined by the GPIO bindings documentation.
+ - #gpio-cells
+ number of cells required to identify and configure a GPIO.
+ Shall be 2.
+ - gpio-ranges
+ Describes a GPIO controller specifying its specific pin base, the pin
+ base in the global pin numbering space, and the number of controlled
+ pins, as defined by the GPIO bindings documentation. Refer to
+ Documentation/devicetree/bindings/gpio/gpio.txt file for a more detailed
+ description.
+
+ Example:
+ A GPIO controller node, controlling 16 pins indexed from 0.
+ The GPIO controller base in the global pin indexing space is pin 48, thus
+ pins [0 - 15] on this controller map to pins [48 - 63] in the global pin
+ indexing space.
+
+ port3: gpio-3 {
+ gpio-controller;
+ #gpio-cells = <2>;
+ gpio-ranges = <&pinctrl 0 48 16>;
+ };
+
+ A device node willing to use pins controlled by this GPIO controller, shall
+ refer to it as follows:
+
+ led1 {
+ gpios = <&port3 10 GPIO_ACTIVE_LOW>;
+ };
--
2.7.4

2017-04-27 08:21:35

[permalink] [raw]

Subject: [PATCH v5 08/10] arm: dts: genmai: Add RIIC2 pin group

Add pin configuration subnode for RIIC2 interface.

Signed-off-by: Jacopo Mondi <[email protected]>
Reviewed-by: Geert Uytterhoeven <[email protected]>
---
arch/arm/boot/dts/r7s72100-genmai.dts | 9 +++++++++
1 file changed, 9 insertions(+)

diff --git a/arch/arm/boot/dts/r7s72100-genmai.dts b/arch/arm/boot/dts/r7s72100-genmai.dts
index c28d74b..9add1b6 100644
--- a/arch/arm/boot/dts/r7s72100-genmai.dts
+++ b/arch/arm/boot/dts/r7s72100-genmai.dts
@@ -43,6 +43,12 @@
/* P3_0 as TxD2; P3_2 as RxD2 */
pinmux = <RZA1_PINMUX(3, 0, 6)>, <RZA1_PINMUX(3, 2, 4)>;
};
+
+ i2c2_pins: i2c2 {
+ /* RIIC2: P1_4 as SCL, P1_5 as SDA */
+ pinmux = <RZA1_PINMUX(1, 4, 1)>, <RZA1_PINMUX(1, 5, 1)>;
+ bi-directional;
+ };
};

&extal_clk {
@@ -61,6 +67,9 @@
status = "okay";
clock-frequency = <400000>;

+ pinctrl-names = "default";
+ pinctrl-0 = <&i2c2_pins>;
+
eeprom@50 {
compatible = "renesas,24c128";
reg = <0x50>;
--
2.7.4

2017-04-27 08:22:11

[permalink] [raw]

Subject: [PATCH v5 10/10] arm: dts: genmai: Add ethernet pin group

Add pin configuration subnode for ETHER ethernet controller.

Signed-off-by: Jacopo Mondi <[email protected]>
---
arch/arm/boot/dts/r7s72100-genmai.dts | 41 +++++++++++++++++++++++++++++++++++
1 file changed, 41 insertions(+)

diff --git a/arch/arm/boot/dts/r7s72100-genmai.dts b/arch/arm/boot/dts/r7s72100-genmai.dts
index f7c512e..328f4c9 100644
--- a/arch/arm/boot/dts/r7s72100-genmai.dts
+++ b/arch/arm/boot/dts/r7s72100-genmai.dts
@@ -63,6 +63,34 @@
pinmux = <RZA1_PINMUX(1, 4, 1)>, <RZA1_PINMUX(1, 5, 1)>;
bi-directional;
};
+
+ ether_pins: ether {
+ pins {
+ /* Ethernet on Ports 1,2,3,5 */
+ pinmux = <RZA1_PINMUX(1, 14, 4)>,/* P1_14 = ET_COL */
+ <RZA1_PINMUX(5, 9, 2)>, /* P5_9 = ET_MDC */
+ <RZA1_PINMUX(3, 4, 2)>, /* P3_4 = ET_RXCLK */
+ <RZA1_PINMUX(3, 5, 2)>, /* P3_5 = ET_RXER */
+ <RZA1_PINMUX(3, 6, 2)>, /* P3_6 = ET_RXDV */
+ <RZA1_PINMUX(2, 0, 2)>, /* P2_0 = ET_TXCLK */
+ <RZA1_PINMUX(2, 1, 2)>, /* P2_1 = ET_TXER */
+ <RZA1_PINMUX(2, 2, 2)>, /* P2_2 = ET_TXEN */
+ <RZA1_PINMUX(2, 3, 2)>, /* P2_3 = ET_CRS */
+ <RZA1_PINMUX(2, 4, 2)>, /* P2_4 = ET_TXD0 */
+ <RZA1_PINMUX(2, 5, 2)>, /* P2_5 = ET_TXD1 */
+ <RZA1_PINMUX(2, 6, 2)>, /* P2_6 = ET_TXD2 */
+ <RZA1_PINMUX(2, 7, 2)>, /* P2_7 = ET_TXD3 */
+ <RZA1_PINMUX(2, 8, 2)>, /* P2_8 = ET_RXD0 */
+ <RZA1_PINMUX(2, 9, 2)>, /* P2_9 = ET_RXD1 */
+ <RZA1_PINMUX(2, 10, 2)>,/* P2_10 = ET_RXD2 */
+ <RZA1_PINMUX(2, 11, 2)>;/* P2_11 = ET_RXD3 */
+ };
+
+ pins_bidir {
+ pinmux = <RZA1_PINMUX(3, 3, 2)>;/* P3_3 = ET_MDIO */
+ bi-directional;
+ };
+ };
};

&extal_clk {
@@ -77,6 +105,19 @@
status = "okay";
};

+&ether {
+ pinctrl-names = "default";
+ pinctrl-0 = <&ether_pins>;
+
+ status = "okay";
+
+ renesas,no-ether-link;
+ phy-handle = <&phy0>;
+ phy0: ethernet-phy@0 {
+ reg = <0>;
+ };
+};
+
&i2c2 {
status = "okay";
clock-frequency = <400000>;
--
2.7.4

2017-04-27 08:21:13

[permalink] [raw]

Subject: [PATCH v5 05/10] arm: dts: dt-bindings: Add Renesas RZ/A1 pinctrl header

Add dt-bindings for Renesas r7s72100 pin controller header file.

Signed-off-by: Jacopo Mondi <[email protected]>
---
include/dt-bindings/pinctrl/r7s72100-pinctrl.h | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
create mode 100644 include/dt-bindings/pinctrl/r7s72100-pinctrl.h

diff --git a/include/dt-bindings/pinctrl/r7s72100-pinctrl.h b/include/dt-bindings/pinctrl/r7s72100-pinctrl.h
new file mode 100644
index 0000000..6b609fe
--- /dev/null
+++ b/include/dt-bindings/pinctrl/r7s72100-pinctrl.h
@@ -0,0 +1,16 @@
+/*
+ * Defines macros and constants for Renesas RZ/A1 pin controller pin
+ * muxing functions.
+ */
+#ifndef __DT_BINDINGS_PINCTRL_RENESAS_RZA1_H
+#define __DT_BINDINGS_PINCTRL_RENESAS_RZA1_H
+
+#define RZA1_PINS_PER_PORT 16
+
+/*
+ * Create the pin index from its bank and position numbers and store in
+ * the upper 16 bits the alternate function identifier
+ */
+#define RZA1_PINMUX(b, p, f) ((b) * RZA1_PINS_PER_PORT + (p) | (f << 16))
+
+#endif /* __DT_BINDINGS_PINCTRL_RENESAS_RZA1_H */
--
2.7.4

2017-04-27 08:21:27

[permalink] [raw]

Subject: [PATCH v5 07/10] arm: dts: genmai: Add SCIF2 pin group

Add pin configuration subnode for SCIF2 serial debug interface.

Signed-off-by: Jacopo Mondi <[email protected]>
Reviewed-by: Geert Uytterhoeven <[email protected]>
---
arch/arm/boot/dts/r7s72100-genmai.dts | 12 ++++++++++++
1 file changed, 12 insertions(+)

diff --git a/arch/arm/boot/dts/r7s72100-genmai.dts b/arch/arm/boot/dts/r7s72100-genmai.dts
index 118a8e2..c28d74b 100644
--- a/arch/arm/boot/dts/r7s72100-genmai.dts
+++ b/arch/arm/boot/dts/r7s72100-genmai.dts
@@ -11,6 +11,7 @@

/dts-v1/;
#include "r7s72100.dtsi"
+#include <dt-bindings/pinctrl/r7s72100-pinctrl.h>

/ {
model = "Genmai";
@@ -36,6 +37,14 @@
};
};

+&pinctrl {
+
+ scif2_pins: serial2 {
+ /* P3_0 as TxD2; P3_2 as RxD2 */
+ pinmux = <RZA1_PINMUX(3, 0, 6)>, <RZA1_PINMUX(3, 2, 4)>;
+ };
+};
+
&extal_clk {
clock-frequency = <13330000>;
};
@@ -60,6 +69,9 @@
};

&scif2 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&scif2_pins>;
+
status = "okay";
};

--
2.7.4

2017-04-27 08:22:07

[permalink] [raw]

Subject: [PATCH v5 09/10] arm: dts: genmai: Add user led device nodes

Add device nodes for user leds on Genmai board.

Signed-off-by: Jacopo Mondi <[email protected]>
Reviewed-by: Geert Uytterhoeven <[email protected]>
---
arch/arm/boot/dts/r7s72100-genmai.dts | 14 ++++++++++++++
1 file changed, 14 insertions(+)

diff --git a/arch/arm/boot/dts/r7s72100-genmai.dts b/arch/arm/boot/dts/r7s72100-genmai.dts
index 9add1b6..f7c512e 100644
--- a/arch/arm/boot/dts/r7s72100-genmai.dts
+++ b/arch/arm/boot/dts/r7s72100-genmai.dts
@@ -11,6 +11,7 @@

/dts-v1/;
#include "r7s72100.dtsi"
+#include <dt-bindings/gpio/gpio.h>
#include <dt-bindings/pinctrl/r7s72100-pinctrl.h>

/ {
@@ -35,6 +36,19 @@
#address-cells = <1>;
#size-cells = <1>;
};
+
+ leds {
+ status = "okay";
+ compatible = "gpio-leds";
+
+ led1 {
+ gpios = <&port4 10 GPIO_ACTIVE_LOW>;
+ };
+
+ led2 {
+ gpios = <&port4 11 GPIO_ACTIVE_LOW>;
+ };
+ };
};

&pinctrl {
--
2.7.4

2017-04-27 08:23:12

[permalink] [raw]

Subject: [PATCH v5 06/10] arm: dts: r7s72100: Add pin controller node

Add pin controller node with 12 gpio controller sub-nodes to
r7s72100 dtsi.

Signed-off-by: Jacopo Mondi <[email protected]>
Reviewed-by: Geert Uytterhoeven <[email protected]>
---
arch/arm/boot/dts/r7s72100.dtsi | 78 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 78 insertions(+)

diff --git a/arch/arm/boot/dts/r7s72100.dtsi b/arch/arm/boot/dts/r7s72100.dtsi
index b8aa256..9342ff4 100644
--- a/arch/arm/boot/dts/r7s72100.dtsi
+++ b/arch/arm/boot/dts/r7s72100.dtsi
@@ -180,6 +180,84 @@
};
};

+ pinctrl: pin-controller@fcfe3000 {
+ compatible = "renesas,r7s72100-ports";
+
+ reg = <0xfcfe3000 0x4230>;
+
+ port0: gpio-0 {
+ gpio-controller;
+ #gpio-cells = <2>;
+ gpio-ranges = <&pinctrl 0 0 6>;
+ };
+
+ port1: gpio-1 {
+ gpio-controller;
+ #gpio-cells = <2>;
+ gpio-ranges = <&pinctrl 0 16 16>;
+ };
+
+ port2: gpio-2 {
+ gpio-controller;
+ #gpio-cells = <2>;
+ gpio-ranges = <&pinctrl 0 32 16>;
+ };
+
+ port3: gpio-3 {
+ gpio-controller;
+ #gpio-cells = <2>;
+ gpio-ranges = <&pinctrl 0 48 16>;
+ };
+
+ port4: gpio-4 {
+ gpio-controller;
+ #gpio-cells = <2>;
+ gpio-ranges = <&pinctrl 0 64 16>;
+ };
+
+ port5: gpio-5 {
+ gpio-controller;
+ #gpio-cells = <2>;
+ gpio-ranges = <&pinctrl 0 80 11>;
+ };
+
+ port6: gpio-6 {
+ gpio-controller;
+ #gpio-cells = <2>;
+ gpio-ranges = <&pinctrl 0 96 16>;
+ };
+
+ port7: gpio-7 {
+ gpio-controller;
+ #gpio-cells = <2>;
+ gpio-ranges = <&pinctrl 0 112 16>;
+ };
+
+ port8: gpio-8 {
+ gpio-controller;
+ #gpio-cells = <2>;
+ gpio-ranges = <&pinctrl 0 128 16>;
+ };
+
+ port9: gpio-9 {
+ gpio-controller;
+ #gpio-cells = <2>;
+ gpio-ranges = <&pinctrl 0 144 8>;
+ };
+
+ port10: gpio-10 {
+ gpio-controller;
+ #gpio-cells = <2>;
+ gpio-ranges = <&pinctrl 0 160 16>;
+ };
+
+ port11: gpio-11 {
+ gpio-controller;
+ #gpio-cells = <2>;
+ gpio-ranges = <&pinctrl 0 176 16>;
+ };
+ };
+
scif0: serial@e8007000 {
compatible = "renesas,scif-r7s72100", "renesas,scif";
reg = <0xe8007000 64>;
--
2.7.4

2017-04-27 08:20:40

[permalink] [raw]

Subject: [PATCH v5 02/10] pinctrl: generic: Add macros to unpack properties

Add PIN_CONF_UNPACK_PARAM and PIN_CONF_UNPACK_ARGS macros useful to
unpack generic properties and their arguments

Signed-off-by: Jacopo Mondi <[email protected]>
---
include/linux/pinctrl/pinconf-generic.h | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/linux/pinctrl/pinconf-generic.h b/include/linux/pinctrl/pinconf-generic.h
index 279e3c5..2cd2a03 100644
--- a/include/linux/pinctrl/pinconf-generic.h
+++ b/include/linux/pinctrl/pinconf-generic.h
@@ -118,7 +118,9 @@ enum pin_config_param {
/*
* Helpful configuration macro to be used in tables etc.
*/
-#define PIN_CONF_PACKED(p, a) ((a << 8) | ((unsigned long) p & 0xffUL))
+#define PIN_CONF_PACKED(p, a) (((a) << 8) | ((unsigned long) (p) & 0xffUL))
+#define PIN_CONF_UNPACK_PARAM(c) ((c) & 0xffUL)
+#define PIN_CONF_UNPACK_ARGS(c) ((c) >> 8)

/*
* The following inlines stuffs a configuration parameter and data value
--
2.7.4

2017-04-27 08:37:22

[permalink] [raw]

Subject: Re: [PATCH v5 02/10] pinctrl: generic: Add macros to unpack properties

On Thu, Apr 27, 2017 at 10:19 AM, Jacopo Mondi
<[email protected]> wrote:
> Add PIN_CONF_UNPACK_PARAM and PIN_CONF_UNPACK_ARGS macros useful to
> unpack generic properties and their arguments
>
> Signed-off-by: Jacopo Mondi <[email protected]>

Reviewed-by: Geert Uytterhoeven <[email protected]>

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2017-04-27 08:37:45

[permalink] [raw]

Subject: Re: [PATCH v5 03/10] pinctrl: Renesas RZ/A1 pin and gpio controller

On Thu, Apr 27, 2017 at 10:19 AM, Jacopo Mondi
<[email protected]> wrote:
> Add combined gpio and pin controller driver for Renesas RZ/A1
> r7s72100 SoC.
>
> Signed-off-by: Jacopo Mondi <[email protected]>

Reviewed-by: Geert Uytterhoeven <[email protected]>

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2017-04-27 08:38:04

[permalink] [raw]

Subject: Re: [PATCH v5 04/10] dt-bindings: pinctrl: Add RZ/A1 bindings doc

On Thu, Apr 27, 2017 at 10:19 AM, Jacopo Mondi
<[email protected]> wrote:
> Add device tree bindings documentation for Renesas RZ/A1 gpio and pin
> controller.
>
> Signed-off-by: Jacopo Mondi <[email protected]>

Reviewed-by: Geert Uytterhoeven <[email protected]>

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2017-04-27 08:38:51

[permalink] [raw]

Subject: Re: [PATCH v5 05/10] arm: dts: dt-bindings: Add Renesas RZ/A1 pinctrl header

On Thu, Apr 27, 2017 at 10:19 AM, Jacopo Mondi
<[email protected]> wrote:
> Add dt-bindings for Renesas r7s72100 pin controller header file.
>
> Signed-off-by: Jacopo Mondi <[email protected]>

Reviewed-by: Geert Uytterhoeven <[email protected]>

> --- /dev/null
> +++ b/include/dt-bindings/pinctrl/r7s72100-pinctrl.h
> @@ -0,0 +1,16 @@

> +#define RZA1_PINMUX(b, p, f) ((b) * RZA1_PINS_PER_PORT + (p) | (f << 16))

... | (f) << 16)

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2017-04-27 08:42:13

[permalink] [raw]

Subject: Re: [PATCH v5 00/10] Renesas RZ/A1 pin and gpio controller

Hi Jacopo,

On Thu, Apr 27, 2017 at 10:19 AM, Jacopo Mondi
<[email protected]> wrote:
> this is 5th round of gpio/pincontroller for RZ/A1 devices.
>
> I have updated the pin controller driver to use the newly introduced
> "pinctrl_enable()" function.
> This is required since v4.11-rc7 as otherwise, as reported by Chris Brandt,
> the pin controller does not start.
>
> I have incorporated your comments on the device tree bindings documentation,
> and added to pinctrl-generic.h header file two macros to unpack generic
> properties and their arguments.
>
> Tested with SCIF, RIIC, ETHER and gpio-leds on Genmai board.

Thanks for the update!

> Jacopo Mondi (10):
> pinctrl: generic: Add bi-directional and output-enable

Already applied by LinusW.

> pinctrl: generic: Add macros to unpack properties

LinusW: do you want me to queue this together with the driver for v4.13,
or will you take this single patch for v4.12?

> pinctrl: Renesas RZ/A1 pin and gpio controller
> dt-bindings: pinctrl: Add RZ/A1 bindings doc

Will queue in sh-pfc-for-v4.13.

> arm: dts: dt-bindings: Add Renesas RZ/A1 pinctrl header
> arm: dts: r7s72100: Add pin controller node
> arm: dts: genmai: Add SCIF2 pin group
> arm: dts: genmai: Add RIIC2 pin group
> arm: dts: genmai: Add user led device nodes
> arm: dts: genmai: Add ethernet pin group

These are for Simon.

Does applying the DTS changes before the driver introduce regressions?
If no, Simon can queue them for v4.13.
If yes, they'll have to wait for v4.14.

Thanks!

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2017-04-27 09:56:48

[permalink] [raw]

Subject: Re: [PATCH v5 10/10] arm: dts: genmai: Add ethernet pin group

Hi Jacopo,

On Thu, Apr 27, 2017 at 10:19 AM, Jacopo Mondi
<[email protected]> wrote:
> Add pin configuration subnode for ETHER ethernet controller.
>
> Signed-off-by: Jacopo Mondi <[email protected]>

Reviewed-by: Geert Uytterhoeven <[email protected]>

> --- a/arch/arm/boot/dts/r7s72100-genmai.dts
> +++ b/arch/arm/boot/dts/r7s72100-genmai.dts

> @@ -77,6 +105,19 @@
> status = "okay";
> };
>
> +&ether {
> + pinctrl-names = "default";
> + pinctrl-0 = <&ether_pins>;
> +
> + status = "okay";
> +
> + renesas,no-ether-link;
> + phy-handle = <&phy0>;
> + phy0: ethernet-phy@0 {
> + reg = <0>;

Shouldn't the interrupt (connected to P1_15) be described?

> + };
> +};

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2017-04-27 10:48:57

[permalink] [raw]

Subject: RE: [PATCH v5 10/10] arm: dts: genmai: Add ethernet pin group

Hi Geert,

On Thursday, April 27, 2017, Geert Uytterhoeven wrote:
> > +&ether {
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&ether_pins>;
> > +
> > + status = "okay";
> > +
> > + renesas,no-ether-link;
> > + phy-handle = <&phy0>;
> > + phy0: ethernet-phy@0 {
> > + reg = <0>;
>
> Shouldn't the interrupt (connected to P1_15) be described?

That interrupt pin from the PHY is not used. It did not need to be connected.

Chris

2017-04-27 14:56:24

[permalink] [raw]

Subject: Re: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable

On Thu, Apr 27, 2017 at 11:19 AM, Jacopo Mondi
<[email protected]> wrote:
> Add bi-directional and output-enable pin configuration properties.
>
> bi-directional allows to specify when a pin shall operate in input and
> output mode at the same time. This is particularly useful in platforms
> where input and output buffers have to be manually enabled.
>
> output-enable is just syntactic sugar to specify that a pin shall
> operate in output mode, ignoring the provided argument.
> This pairs with input-enable pin configuration option.

For me it looks like you are trying to alias open-drain + bias or
alike. Don't actually see the benefit of it.

> Signed-off-by: Jacopo Mondi <[email protected]>
> ---
> Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt | 2 ++
> drivers/pinctrl/pinconf-generic.c | 3 +++
> include/linux/pinctrl/pinconf-generic.h | 3 +++
> 3 files changed, 8 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
> index bf3f7b0..f2ed458 100644
> --- a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
> +++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
> @@ -222,6 +222,7 @@ bias-bus-hold - latch weakly
> bias-pull-up - pull up the pin
> bias-pull-down - pull down the pin
> bias-pull-pin-default - use pin-default pull state
> +bi-directional - pin supports simultaneous input/output operations
> drive-push-pull - drive actively high and low
> drive-open-drain - drive with open drain
> drive-open-source - drive with open source
> @@ -234,6 +235,7 @@ input-debounce - debounce mode with debound time X
> power-source - select between different power supplies
> low-power-enable - enable low power mode
> low-power-disable - disable low power mode
> +output-enable - enable output on pin regardless of output value
> output-low - set the pin to output mode with low level
> output-high - set the pin to output mode with high level
> slew-rate - set the slew rate
> diff --git a/drivers/pinctrl/pinconf-generic.c b/drivers/pinctrl/pinconf-generic.c
> index ce3335a..03e6808 100644
> --- a/drivers/pinctrl/pinconf-generic.c
> +++ b/drivers/pinctrl/pinconf-generic.c
> @@ -35,6 +35,7 @@ static const struct pin_config_item conf_items[] = {
> PCONFDUMP(PIN_CONFIG_BIAS_PULL_PIN_DEFAULT,
> "input bias pull to pin specific state", NULL, false),
> PCONFDUMP(PIN_CONFIG_BIAS_PULL_UP, "input bias pull up", NULL, false),
> + PCONFDUMP(PIN_CONFIG_BIDIRECTIONAL, "bi-directional pin operations", NULL, false),
> PCONFDUMP(PIN_CONFIG_DRIVE_OPEN_DRAIN, "output drive open drain", NULL, false),
> PCONFDUMP(PIN_CONFIG_DRIVE_OPEN_SOURCE, "output drive open source", NULL, false),
> PCONFDUMP(PIN_CONFIG_DRIVE_PUSH_PULL, "output drive push pull", NULL, false),
> @@ -160,6 +161,7 @@ static const struct pinconf_generic_params dt_params[] = {
> { "bias-pull-up", PIN_CONFIG_BIAS_PULL_UP, 1 },
> { "bias-pull-pin-default", PIN_CONFIG_BIAS_PULL_PIN_DEFAULT, 1 },
> { "bias-pull-down", PIN_CONFIG_BIAS_PULL_DOWN, 1 },
> + { "bi-directional", PIN_CONFIG_BIDIRECTIONAL, 1 },
> { "drive-open-drain", PIN_CONFIG_DRIVE_OPEN_DRAIN, 0 },
> { "drive-open-source", PIN_CONFIG_DRIVE_OPEN_SOURCE, 0 },
> { "drive-push-pull", PIN_CONFIG_DRIVE_PUSH_PULL, 0 },
> @@ -172,6 +174,7 @@ static const struct pinconf_generic_params dt_params[] = {
> { "input-schmitt-enable", PIN_CONFIG_INPUT_SCHMITT_ENABLE, 1 },
> { "low-power-disable", PIN_CONFIG_LOW_POWER_MODE, 0 },
> { "low-power-enable", PIN_CONFIG_LOW_POWER_MODE, 1 },
> + { "output-enable", PIN_CONFIG_OUTPUT, 1, },
> { "output-high", PIN_CONFIG_OUTPUT, 1, },
> { "output-low", PIN_CONFIG_OUTPUT, 0, },
> { "power-source", PIN_CONFIG_POWER_SOURCE, 0 },
> diff --git a/include/linux/pinctrl/pinconf-generic.h b/include/linux/pinctrl/pinconf-generic.h
> index 7620eb1..279e3c5 100644
> --- a/include/linux/pinctrl/pinconf-generic.h
> +++ b/include/linux/pinctrl/pinconf-generic.h
> @@ -42,6 +42,8 @@
> * @PIN_CONFIG_BIAS_PULL_UP: the pin will be pulled up (usually with high
> * impedance to VDD). If the argument is != 0 pull-up is enabled,
> * if it is 0, pull-up is total, i.e. the pin is connected to VDD.
> + * @PIN_CONFIG_BIDIRECTIONAL: the pin will be configured to allow simultaneous
> + * input and output operations.
> * @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. Setting this
> @@ -96,6 +98,7 @@ enum pin_config_param {
> PIN_CONFIG_BIAS_PULL_DOWN,
> PIN_CONFIG_BIAS_PULL_PIN_DEFAULT,
> PIN_CONFIG_BIAS_PULL_UP,
> + PIN_CONFIG_BIDIRECTIONAL,
> PIN_CONFIG_DRIVE_OPEN_DRAIN,
> PIN_CONFIG_DRIVE_OPEN_SOURCE,
> PIN_CONFIG_DRIVE_PUSH_PULL,
> --
> 2.7.4
>

--
With Best Regards,
Andy Shevchenko

2017-04-28 05:19:56

[permalink] [raw]

Subject: Re: [PATCH v5 05/10] arm: dts: dt-bindings: Add Renesas RZ/A1 pinctrl header

On Thu, Apr 27, 2017 at 10:38:39AM +0200, Geert Uytterhoeven wrote:
> On Thu, Apr 27, 2017 at 10:19 AM, Jacopo Mondi
> <[email protected]> wrote:
> > Add dt-bindings for Renesas r7s72100 pin controller header file.
> >
> > Signed-off-by: Jacopo Mondi <[email protected]>
>
> Reviewed-by: Geert Uytterhoeven <[email protected]>

Thanks, I have queued this up.

2017-04-28 05:21:50

[permalink] [raw]

Subject: Re: [PATCH v5 07/10] arm: dts: genmai: Add SCIF2 pin group

On Thu, Apr 27, 2017 at 10:19:51AM +0200, Jacopo Mondi wrote:
> Add pin configuration subnode for SCIF2 serial debug interface.
>
> Signed-off-by: Jacopo Mondi <[email protected]>
> Reviewed-by: Geert Uytterhoeven <[email protected]>

As the dt-bindings (documentation) has been acked by Geert I'd be happy
to queue up this and other "arm: dts: genmai" DT patches in this series
which do not have any outstanding review comments. Is it safe to do so
without the PFC driver patches in place?

> ---
> arch/arm/boot/dts/r7s72100-genmai.dts | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/arch/arm/boot/dts/r7s72100-genmai.dts b/arch/arm/boot/dts/r7s72100-genmai.dts
> index 118a8e2..c28d74b 100644
> --- a/arch/arm/boot/dts/r7s72100-genmai.dts
> +++ b/arch/arm/boot/dts/r7s72100-genmai.dts
> @@ -11,6 +11,7 @@
>
> /dts-v1/;
> #include "r7s72100.dtsi"
> +#include <dt-bindings/pinctrl/r7s72100-pinctrl.h>
>
> / {
> model = "Genmai";
> @@ -36,6 +37,14 @@
> };
> };
>
> +&pinctrl {
> +
> + scif2_pins: serial2 {
> + /* P3_0 as TxD2; P3_2 as RxD2 */
> + pinmux = <RZA1_PINMUX(3, 0, 6)>, <RZA1_PINMUX(3, 2, 4)>;
> + };
> +};
> +
> &extal_clk {
> clock-frequency = <13330000>;
> };
> @@ -60,6 +69,9 @@
> };
>
> &scif2 {
> + pinctrl-names = "default";
> + pinctrl-0 = <&scif2_pins>;
> +
> status = "okay";
> };
>
> --
> 2.7.4
>

2017-04-28 05:22:23

[permalink] [raw]

Subject: Re: [PATCH v5 10/10] arm: dts: genmai: Add ethernet pin group

On Thu, Apr 27, 2017 at 10:48:45AM +0000, Chris Brandt wrote:
> Hi Geert,
>
> On Thursday, April 27, 2017, Geert Uytterhoeven wrote:
> > > +&ether {
> > > + pinctrl-names = "default";
> > > + pinctrl-0 = <&ether_pins>;
> > > +
> > > + status = "okay";
> > > +
> > > + renesas,no-ether-link;
> > > + phy-handle = <&phy0>;
> > > + phy0: ethernet-phy@0 {
> > > + reg = <0>;
> >
> > Shouldn't the interrupt (connected to P1_15) be described?
>
>
> That interrupt pin from the PHY is not used. It did not need to be connected.

So things are fine as above or should I expect to see v6?

2017-04-28 05:23:02

[permalink] [raw]

Subject: Re: [PATCH v5 00/10] Renesas RZ/A1 pin and gpio controller

On Thu, Apr 27, 2017 at 10:42:02AM +0200, Geert Uytterhoeven wrote:
> Hi Jacopo,
>
> On Thu, Apr 27, 2017 at 10:19 AM, Jacopo Mondi
> <[email protected]> wrote:
> > this is 5th round of gpio/pincontroller for RZ/A1 devices.
> >
> > I have updated the pin controller driver to use the newly introduced
> > "pinctrl_enable()" function.
> > This is required since v4.11-rc7 as otherwise, as reported by Chris Brandt,
> > the pin controller does not start.
> >
> > I have incorporated your comments on the device tree bindings documentation,
> > and added to pinctrl-generic.h header file two macros to unpack generic
> > properties and their arguments.
> >
> > Tested with SCIF, RIIC, ETHER and gpio-leds on Genmai board.
>
> Thanks for the update!
>
> > Jacopo Mondi (10):
> > pinctrl: generic: Add bi-directional and output-enable
>
> Already applied by LinusW.
>
> > pinctrl: generic: Add macros to unpack properties
>
> LinusW: do you want me to queue this together with the driver for v4.13,
> or will you take this single patch for v4.12?
>
> > pinctrl: Renesas RZ/A1 pin and gpio controller
> > dt-bindings: pinctrl: Add RZ/A1 bindings doc
>
> Will queue in sh-pfc-for-v4.13.
>
> > arm: dts: dt-bindings: Add Renesas RZ/A1 pinctrl header
> > arm: dts: r7s72100: Add pin controller node
> > arm: dts: genmai: Add SCIF2 pin group
> > arm: dts: genmai: Add RIIC2 pin group
> > arm: dts: genmai: Add user led device nodes
> > arm: dts: genmai: Add ethernet pin group
>
> These are for Simon.
>
> Does applying the DTS changes before the driver introduce regressions?
> If no, Simon can queue them for v4.13.
> If yes, they'll have to wait for v4.14.

That is my question too.

2017-04-28 07:18:52

[permalink] [raw]

Subject: Re: [PATCH v5 10/10] arm: dts: genmai: Add ethernet pin group

Hi Chris,

On Thu, Apr 27, 2017 at 12:48 PM, Chris Brandt <[email protected]> wrote:
> On Thursday, April 27, 2017, Geert Uytterhoeven wrote:
>> > +&ether {
>> > + pinctrl-names = "default";
>> > + pinctrl-0 = <&ether_pins>;
>> > +
>> > + status = "okay";
>> > +
>> > + renesas,no-ether-link;
>> > + phy-handle = <&phy0>;
>> > + phy0: ethernet-phy@0 {
>> > + reg = <0>;
>>
>> Shouldn't the interrupt (connected to P1_15) be described?
>
> That interrupt pin from the PHY is not used. It did not need to be connected.

But it is connected, according to the schematics.

DT describes hardware, not software limitations.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2017-04-28 07:25:19

[permalink] [raw]

Subject: Re: [PATCH v5 00/10] Renesas RZ/A1 pin and gpio controller

Hi Geert, Simon,

On Thu, Apr 27, 2017 at 10:42:02AM +0200, Geert Uytterhoeven wrote:
> Hi Jacopo,
>
> On Thu, Apr 27, 2017 at 10:19 AM, Jacopo Mondi
> <[email protected]> wrote:
> > this is 5th round of gpio/pincontroller for RZ/A1 devices.
> >
> > I have updated the pin controller driver to use the newly introduced
> > "pinctrl_enable()" function.
> > This is required since v4.11-rc7 as otherwise, as reported by Chris Brandt,
> > the pin controller does not start.
> >
> > I have incorporated your comments on the device tree bindings documentation,
> > and added to pinctrl-generic.h header file two macros to unpack generic
> > properties and their arguments.
> >
> > Tested with SCIF, RIIC, ETHER and gpio-leds on Genmai board.
>
> Thanks for the update!
>
> > Jacopo Mondi (10):
> > pinctrl: generic: Add bi-directional and output-enable
>
> Already applied by LinusW.
>
> > pinctrl: generic: Add macros to unpack properties
>
> LinusW: do you want me to queue this together with the driver for v4.13,
> or will you take this single patch for v4.12?
>
> > pinctrl: Renesas RZ/A1 pin and gpio controller
> > dt-bindings: pinctrl: Add RZ/A1 bindings doc
>
> Will queue in sh-pfc-for-v4.13.
>
> > arm: dts: dt-bindings: Add Renesas RZ/A1 pinctrl header
> > arm: dts: r7s72100: Add pin controller node
> > arm: dts: genmai: Add SCIF2 pin group
> > arm: dts: genmai: Add RIIC2 pin group
> > arm: dts: genmai: Add user led device nodes
> > arm: dts: genmai: Add ethernet pin group
>
> These are for Simon.
>
> Does applying the DTS changes before the driver introduce regressions?
> If no, Simon can queue them for v4.13.
> If yes, they'll have to wait for v4.14.
>

I tried reverting patch [03/10] which introduces the driver, and I
lose serial output on Genmai. So I guess the dts patches have to be
taken after the driver has been merged.

One question: why can't they be taken at the same time? (eg. for
v4.13?)

Thanks
j

> Thanks!
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
> -- Linus Torvalds

2017-04-28 07:30:57

[permalink] [raw]

Subject: Re: [PATCH v5 00/10] Renesas RZ/A1 pin and gpio controller

Hi Jacopo,

On Fri, Apr 28, 2017 at 9:24 AM, jmondi <[email protected]> wrote:
> On Thu, Apr 27, 2017 at 10:42:02AM +0200, Geert Uytterhoeven wrote:
>> On Thu, Apr 27, 2017 at 10:19 AM, Jacopo Mondi
>> <[email protected]> wrote:
>> > this is 5th round of gpio/pincontroller for RZ/A1 devices.
>> >
>> > I have updated the pin controller driver to use the newly introduced
>> > "pinctrl_enable()" function.
>> > This is required since v4.11-rc7 as otherwise, as reported by Chris Brandt,
>> > the pin controller does not start.
>> >
>> > I have incorporated your comments on the device tree bindings documentation,
>> > and added to pinctrl-generic.h header file two macros to unpack generic
>> > properties and their arguments.
>> >
>> > Tested with SCIF, RIIC, ETHER and gpio-leds on Genmai board.
>>
>> Thanks for the update!
>>
>> > Jacopo Mondi (10):
>> > pinctrl: generic: Add bi-directional and output-enable
>>
>> Already applied by LinusW.
>>
>> > pinctrl: generic: Add macros to unpack properties
>>
>> LinusW: do you want me to queue this together with the driver for v4.13,
>> or will you take this single patch for v4.12?
>>
>> > pinctrl: Renesas RZ/A1 pin and gpio controller
>> > dt-bindings: pinctrl: Add RZ/A1 bindings doc
>>
>> Will queue in sh-pfc-for-v4.13.
>>
>> > arm: dts: dt-bindings: Add Renesas RZ/A1 pinctrl header
>> > arm: dts: r7s72100: Add pin controller node
>> > arm: dts: genmai: Add SCIF2 pin group
>> > arm: dts: genmai: Add RIIC2 pin group
>> > arm: dts: genmai: Add user led device nodes
>> > arm: dts: genmai: Add ethernet pin group
>>
>> These are for Simon.
>>
>> Does applying the DTS changes before the driver introduce regressions?
>> If no, Simon can queue them for v4.13.
>> If yes, they'll have to wait for v4.14.
>>
>
> I tried reverting patch [03/10] which introduces the driver, and I
> lose serial output on Genmai. So I guess the dts patches have to be
> taken after the driver has been merged.
>
> One question: why can't they be taken at the same time? (eg. for
> v4.13?)

Because the driver goes in through me and LinusW, while the DTS goes in
through Simon and arm-soc.

If Simon applies the DTS patches, it will regress genmai in renesas-devel
until the release of v4.13-rc1.

An immutable branch would solve that, but for adding PFC we usuable just
postpone the DT part by one cycle.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2017-04-28 07:31:45

[permalink] [raw]

Subject: Re: [PATCH v5 00/10] Renesas RZ/A1 pin and gpio controller

On Fri, Apr 28, 2017 at 09:24:57AM +0200, jmondi wrote:
> Hi Geert, Simon,
>
> On Thu, Apr 27, 2017 at 10:42:02AM +0200, Geert Uytterhoeven wrote:
> > Hi Jacopo,
> >
> > On Thu, Apr 27, 2017 at 10:19 AM, Jacopo Mondi
> > <[email protected]> wrote:
> > > this is 5th round of gpio/pincontroller for RZ/A1 devices.
> > >
> > > I have updated the pin controller driver to use the newly introduced
> > > "pinctrl_enable()" function.
> > > This is required since v4.11-rc7 as otherwise, as reported by Chris Brandt,
> > > the pin controller does not start.
> > >
> > > I have incorporated your comments on the device tree bindings documentation,
> > > and added to pinctrl-generic.h header file two macros to unpack generic
> > > properties and their arguments.
> > >
> > > Tested with SCIF, RIIC, ETHER and gpio-leds on Genmai board.
> >
> > Thanks for the update!
> >
> > > Jacopo Mondi (10):
> > > pinctrl: generic: Add bi-directional and output-enable
> >
> > Already applied by LinusW.
> >
> > > pinctrl: generic: Add macros to unpack properties
> >
> > LinusW: do you want me to queue this together with the driver for v4.13,
> > or will you take this single patch for v4.12?
> >
> > > pinctrl: Renesas RZ/A1 pin and gpio controller
> > > dt-bindings: pinctrl: Add RZ/A1 bindings doc
> >
> > Will queue in sh-pfc-for-v4.13.
> >
> > > arm: dts: dt-bindings: Add Renesas RZ/A1 pinctrl header
> > > arm: dts: r7s72100: Add pin controller node
> > > arm: dts: genmai: Add SCIF2 pin group
> > > arm: dts: genmai: Add RIIC2 pin group
> > > arm: dts: genmai: Add user led device nodes
> > > arm: dts: genmai: Add ethernet pin group
> >
> > These are for Simon.
> >
> > Does applying the DTS changes before the driver introduce regressions?
> > If no, Simon can queue them for v4.13.
> > If yes, they'll have to wait for v4.14.
> >
>
> I tried reverting patch [03/10] which introduces the driver, and I
> lose serial output on Genmai. So I guess the dts patches have to be
> taken after the driver has been merged.
>
> One question: why can't they be taken at the same time? (eg. for
> v4.13?)

The problem is that the changes go through different trees. It may,
however, be possible to arrange for both sets of changes to go into v4.13
by negotiating with the ARM-SoC maintainers.

2017-04-28 08:16:35

[permalink] [raw]

Subject: Re: [PATCH v5 02/10] pinctrl: generic: Add macros to unpack properties

On Thu, Apr 27, 2017 at 10:19 AM, Jacopo Mondi
<[email protected]> wrote:

> Add PIN_CONF_UNPACK_PARAM and PIN_CONF_UNPACK_ARGS macros useful to
> unpack generic properties and their arguments
>
> Signed-off-by: Jacopo Mondi <[email protected]>

(...)

/*
* Helpful configuration macro to be used in tables etc.

Then this should say "macros" rather than "macro".

> -#define PIN_CONF_PACKED(p, a) ((a << 8) | ((unsigned long) p & 0xffUL))
> +#define PIN_CONF_PACKED(p, a) (((a) << 8) | ((unsigned long) (p) & 0xffUL))

Also adding some extra parantheses I see.

> +#define PIN_CONF_UNPACK_PARAM(c) ((c) & 0xffUL)
> +#define PIN_CONF_UNPACK_ARGS(c) ((c) >> 8)

But why.

I have these two static inlines just below your new macros:

static inline enum pin_config_param pinconf_to_config_param(unsigned
long config)
{
return (enum pin_config_param) (config & 0xffUL);
}

static inline u32 pinconf_to_config_argument(unsigned long config)
{
return (u32) ((config >> 8) & 0xffffffUL);
}

Why can't you use this in your code instead of macros?

We generally prefer static inlines over macros because they are easier
to read.

Yours,
Linus Walleij

2017-04-28 08:32:38

[permalink] [raw]

Subject: Re: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable

On Thu, Apr 27, 2017 at 4:56 PM, Andy Shevchenko
<[email protected]> wrote:
> On Thu, Apr 27, 2017 at 11:19 AM, Jacopo Mondi
> <[email protected]> wrote:
>> Add bi-directional and output-enable pin configuration properties.
>>
>> bi-directional allows to specify when a pin shall operate in input and
>> output mode at the same time. This is particularly useful in platforms
>> where input and output buffers have to be manually enabled.
>>
>> output-enable is just syntactic sugar to specify that a pin shall
>> operate in output mode, ignoring the provided argument.
>> This pairs with input-enable pin configuration option.
>
> For me it looks like you are trying to alias open-drain + bias or
> alike. Don't actually see the benefit of it.

Andy is bringing up a valid point. And I remember asking about
this before.

What does "bi-directional" really mean, electrically speaking?

Does is just mean open drain and/or open source actually?
(See Documentation/gpio/driver.txt for an explanation of
how open drain/source works.)

When you set an output without setting a value, what happens
electrically?

Isn't this bias-high-impedance / High-Z?

Hopefully you can find the answer from Renesas hardware dept.

You can certainly call it whatever the datasheet calls it
in your driver #defines but for the DT bindings we would
ideally have the physical world things.

Yours,
Linus Walleij

2017-04-28 08:51:11

[permalink] [raw]

Subject: Re: [PATCH v5 10/10] arm: dts: genmai: Add ethernet pin group

On Thu, Apr 27, 2017 at 10:19 AM, Jacopo Mondi
<[email protected]> wrote:

> Add pin configuration subnode for ETHER ethernet controller.
>
> Signed-off-by: Jacopo Mondi <[email protected]>
(...)
> + pins_bidir {
> + pinmux = <RZA1_PINMUX(3, 3, 2)>;/* P3_3 = ET_MDIO */
> + bi-directional;
> + };

So I'm against merging this until someone explains what "bi-directional"
actually means, electrically speaking. What happens physically on this pin?

I think this just means open drain.

It is dangerous to merge things we don't understand.

Surely someone inside Renesas can answer this question.

Yours,
Linus Walleij

2017-04-28 10:06:47

[permalink] [raw]

Subject: Re: [PATCH v5 02/10] pinctrl: generic: Add macros to unpack properties

On Fri, Apr 28, 2017 at 10:16 AM, Linus Walleij
<[email protected]> wrote:
> On Thu, Apr 27, 2017 at 10:19 AM, Jacopo Mondi
>> +#define PIN_CONF_UNPACK_PARAM(c) ((c) & 0xffUL)
>> +#define PIN_CONF_UNPACK_ARGS(c) ((c) >> 8)
>
> But why.
>
> I have these two static inlines just below your new macros:
>
> static inline enum pin_config_param pinconf_to_config_param(unsigned
> long config)
> {
> return (enum pin_config_param) (config & 0xffUL);
> }
>
> static inline u32 pinconf_to_config_argument(unsigned long config)
> {
> return (u32) ((config >> 8) & 0xffffffUL);
> }

Cool, need...more...context...in...patches ;-)

> Why can't you use this in your code instead of macros?
>
> We generally prefer static inlines over macros because they are easier
> to read.

Sure.

Thanks for noticing!

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2017-04-28 10:10:05

[permalink] [raw]

Subject: Re: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable

Hi Linus,

On Fri, Apr 28, 2017 at 10:32 AM, Linus Walleij
<[email protected]> wrote:
> On Thu, Apr 27, 2017 at 4:56 PM, Andy Shevchenko
> <[email protected]> wrote:
>> On Thu, Apr 27, 2017 at 11:19 AM, Jacopo Mondi
>> <[email protected]> wrote:
>>> Add bi-directional and output-enable pin configuration properties.
>>>
>>> bi-directional allows to specify when a pin shall operate in input and
>>> output mode at the same time. This is particularly useful in platforms
>>> where input and output buffers have to be manually enabled.
>>>
>>> output-enable is just syntactic sugar to specify that a pin shall
>>> operate in output mode, ignoring the provided argument.
>>> This pairs with input-enable pin configuration option.
>>
>> For me it looks like you are trying to alias open-drain + bias or
>> alike. Don't actually see the benefit of it.
>
> Andy is bringing up a valid point. And I remember asking about
> this before.
>
> What does "bi-directional" really mean, electrically speaking?
>
> Does is just mean open drain and/or open source actually?
> (See Documentation/gpio/driver.txt for an explanation of
> how open drain/source works.)
>
> When you set an output without setting a value, what happens
> electrically?
>
> Isn't this bias-high-impedance / High-Z?
>
> Hopefully you can find the answer from Renesas hardware dept.
>
> You can certainly call it whatever the datasheet calls it
> in your driver #defines but for the DT bindings we would
> ideally have the physical world things.

FWIW, you have already applied v4.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2017-04-28 12:07:18

[permalink] [raw]

Subject: RE: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable

On Friday, April 28, 2017, Linus Walleij wrote:
> > For me it looks like you are trying to alias open-drain + bias or
> > alike. Don't actually see the benefit of it.
>
> Andy is bringing up a valid point. And I remember asking about this before.
>
> What does "bi-directional" really mean, electrically speaking?
>
> Does is just mean open drain and/or open source actually?
> (See Documentation/gpio/driver.txt for an explanation of how open
> drain/source works.)
>
> When you set an output without setting a value, what happens electrically?
>
> Isn't this bias-high-impedance / High-Z?
>
> Hopefully you can find the answer from Renesas hardware dept.
>
> You can certainly call it whatever the datasheet calls it in your driver
> #defines but for the DT bindings we would ideally have the physical world
> things.

The main reason is this pin controller is too dumb to do what it's supposed to with 1 register setting.

Take the SDHI data pins. You send AND receive data over those pins (and they are not open drain). The issue is that the PFC HW that enables the connections between the SDHI IP block and the I/O pad buffers can only enable one path/signal/direction to the buffer enables (in or out). So for a pin that needs both directions, the PFC enables output and the "bidirectional register" is used to enable the input buffer as well.
In the RZ/A1 HW manual you can kind of see that in 54.18 Port Control Logical Diagram (but that wasn't obvious to me at first).

# side note, the way the registers are arranged is also ridiculous in my opinion. I'm not a fan of this particular IP.

The good news is the RZ/A1 is the only chip series I've seen with this PFC IP (I can't even figure out where it came from internally). And as far as I know, it will not appear in any other RZ series chips.

Chris

2017-04-28 12:15:54

[permalink] [raw]

Subject: Re: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable

On Fri, Apr 28, 2017 at 3:07 PM, Chris Brandt <[email protected]> wrote:
> On Friday, April 28, 2017, Linus Walleij wrote:
>> > For me it looks like you are trying to alias open-drain + bias or
>> > alike. Don't actually see the benefit of it.
>>
>> Andy is bringing up a valid point. And I remember asking about this before.
>>
>> What does "bi-directional" really mean, electrically speaking?

> Take the SDHI data pins. You send AND receive data over those pins (and they are not open drain).

Can you point to schematics and electrical characteristics of such buffer?
(Yes, I can imagine one case where it's possible to have an
"automatic" switch based on which current is bigger output of your
side or remote's. But! I would like to see actual specifications to
prove this or otherwise.)

> The issue is that the PFC HW that enables the connections between the SDHI IP block and the I/O pad buffers can only enable one path/signal/direction to the buffer enables (in or out). So for a pin that needs both directions, the PFC enables output and the "bidirectional register" is used to enable the input buffer as well.

> In the RZ/A1 HW manual you can kind of see that in 54.18 Port Control Logical Diagram (but that wasn't obvious to me at first).

Please, post a link to it or copy essential parts.
I'm quite skeptical that cheap hardware can implement something more
costable than simplest open-source / open-drain + bias.

--
With Best Regards,
Andy Shevchenko

2017-04-28 12:49:22

[permalink] [raw]

Subject: Re: [PATCH v5 02/10] pinctrl: generic: Add macros to unpack properties

Hi Linus,

On Fri, Apr 28, 2017 at 10:16:22AM +0200, Linus Walleij wrote:
> On Thu, Apr 27, 2017 at 10:19 AM, Jacopo Mondi
> <[email protected]> wrote:
>
> > Add PIN_CONF_UNPACK_PARAM and PIN_CONF_UNPACK_ARGS macros useful to
> > unpack generic properties and their arguments
> >
> > Signed-off-by: Jacopo Mondi <[email protected]>
>
> (...)
>
> /*
> * Helpful configuration macro to be used in tables etc.
>
> Then this should say "macros" rather than "macro".
>
> > -#define PIN_CONF_PACKED(p, a) ((a << 8) | ((unsigned long) p & 0xffUL))
> > +#define PIN_CONF_PACKED(p, a) (((a) << 8) | ((unsigned long) (p) & 0xffUL))
>
> Also adding some extra parantheses I see.
>
> > +#define PIN_CONF_UNPACK_PARAM(c) ((c) & 0xffUL)
> > +#define PIN_CONF_UNPACK_ARGS(c) ((c) >> 8)
>
> But why.
>
> I have these two static inlines just below your new macros:
>
> static inline enum pin_config_param pinconf_to_config_param(unsigned
> long config)
> {
> return (enum pin_config_param) (config & 0xffUL);
> }
>
> static inline u32 pinconf_to_config_argument(unsigned long config)
> {
> return (u32) ((config >> 8) & 0xffffffUL);
> }
>
> Why can't you use this in your code instead of macros?
>
> We generally prefer static inlines over macros because they are easier
> to read.
>

Right. I haven't noticed them.
I'll drop this patch, sorry for noise

> Yours,
> Linus Walleij

2017-04-28 13:18:18

[permalink] [raw]

Subject: RE: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable

On Friday, April 28, 2017, Andy Shevchenko wrote:
> > In the RZ/A1 HW manual you can kind of see that in 54.18 Port Control
> Logical Diagram (but that wasn't obvious to me at first).
>
> Please, post a link to it or copy essential parts.

This board the RZ/A1 GENMAI board.
https://www.renesas.com/en-us/products/software-tools/boards-and-kits/evaluation-demo-solution-boards/genmai-cpu-board-rtk772100bc00000br.html

The schematic is included in the "User's manual"
https://www.renesas.com/en-us/doc/products/tool/doc/003/r20ut2596ej_r7s72100evum.pdf

The RZ/A1H Hardware manual is here:
https://www.renesas.com/en-us/document/hw-manual?hwLayerShowFlg=true&prdLayerId=186374&layerName=RZ%252FA1H&coronrService=document-prd-search&hwDocUrl=%2Fen-us%2Fdoc%2Fproducts%2Fmpumcu%2Fdoc%2Frz%2Fr01uh0403ej0300_rz_a1h.pdf&hashKey=54f335753742b5add524d4725b7242e6

Chapter 54 is the port/pin controller.

"54.18 Port Control Logical Diagram" is the diagram I was talking about. Note that is says "Note: This figure shows the logic for reference, not the circuit."

"54.3.13 Port Bidirection Control Register (PBDCn)" is the magic register needed to get some pins to work.

> I'm quite skeptical that cheap hardware can implement something more
> costable than simplest open-source / open-drain + bias

I don't think this is an open-source / open-drain + bias issue. It's a "the internal signal paths are not getting hooked up correctly" issue.

Regardless, on this part, we needed a way to flag that some pins when put in some function modes needed 'an extra register setting'. At first we tried to sneak that info in with a simple #define in the pin/pinmux DT node properties. But, Linus didn't want it there so we had to make up a new generic property called "bi-directional".

What is your end goal here? Get "bi-directional" changed to something else?

Chris

2017-04-28 13:50:50

[permalink] [raw]

Subject: RE: [PATCH v5 10/10] arm: dts: genmai: Add ethernet pin group

On Friday, April 28, 2017, Linus Walleij wrote:
> > Add pin configuration subnode for ETHER ethernet controller.
> >
> > Signed-off-by: Jacopo Mondi <[email protected]>
> (...)
> > + pins_bidir {
> > + pinmux = <RZA1_PINMUX(3, 3, 2)>;/* P3_3 =
> ET_MDIO */
> > + bi-directional;
> > + };
>
> So I'm against merging this until someone explains what "bi-directional"
> actually means, electrically speaking. What happens physically on this
> pin?
>
> I think this just means open drain.
>
> It is dangerous to merge things we don't understand.
>
> Surely someone inside Renesas can answer this question.

I don't think this has anything to do with open drain because you need it for any pin that the peripheral IP block needs to transmit and receive over the same line, regardless of if it's a SDHI, I2C, Ethernet MDIO, etc...
It's more about of allowing the internal IP block signals to get hooked up to the IO pad signals.

Chris

2017-04-28 14:49:32

[permalink] [raw]

Subject: RE: [PATCH v5 10/10] arm: dts: genmai: Add ethernet pin group

Hi Geert,

On Friday, April 28, 2017, Geert Uytterhoeven wrote:
> >> Shouldn't the interrupt (connected to P1_15) be described?
> >
> > That interrupt pin from the PHY is not used. It did not need to be
> connected.
>
> But it is connected, according to the schematics.

P1_15 can be configured as:
* GPIO_IN
* AN7
* AVB_CAPTURE

So...I guess you would 'describe' it as an GPIO-input.

> DT describes hardware, not software limitations.

Describing things is fine, but the kernel code takes the DT and then start configuring things based on it.

For example, on the RSK board, that line is connected to P4_14. P4_14 can be configured as IRQ6...but IRQ6 comes out in 8 different pin choices, and I might want to use one of those other choices, so I don't want to describe P4_14 as an interrupt.

If it was just describing that "pin 15 of the PHY chip is tied to pin Y19 of the RZ/A1H", that's fine because it's hardware connection that's not going to change.
But...the DT also defines the pin muxing...which is a software decision (do I want to get interrupt or just manually poll or simple ignore it).
This is the part of the whole "DT is for hardware description only" that doesn't really make sense to me.

Chris

2017-04-28 14:53:07

[permalink] [raw]

Subject: Re: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable

On Fri, Apr 28, 2017 at 4:18 PM, Chris Brandt <[email protected]> wrote:
> On Friday, April 28, 2017, Andy Shevchenko wrote:
>> > In the RZ/A1 HW manual you can kind of see that in 54.18 Port Control
>> Logical Diagram (but that wasn't obvious to me at first).
>>
>> Please, post a link to it or copy essential parts.

> The schematic is included in the "User's manual"
> https://www.renesas.com/en-us/doc/products/tool/doc/003/r20ut2596ej_r7s72100evum.pdf

Not that one I would like to see...

> The RZ/A1H Hardware manual is here:
> https://www.renesas.com/en-us/document/hw-manual?hwLayerShowFlg=true&prdLayerId=186374&layerName=RZ%252FA1H&coronrService=document-prd-search&hwDocUrl=%2Fen-us%2Fdoc%2Fproducts%2Fmpumcu%2Fdoc%2Frz%2Fr01uh0403ej0300_rz_a1h.pdf&hashKey=54f335753742b5add524d4725b7242e6
>
> Chapter 54 is the port/pin controller.
>
> "54.18 Port Control Logical Diagram" is the diagram I was talking about. Note that is says "Note: This figure shows the logic for reference, not the circuit."
>
> "54.3.13 Port Bidirection Control Register (PBDCn)" is the magic register needed to get some pins to work.

This is useful. Thanks.

>> I'm quite skeptical that cheap hardware can implement something more
>> costable than simplest open-source / open-drain + bias
>
> I don't think this is an open-source / open-drain + bias issue. It's a "the internal signal paths are not getting hooked up correctly" issue.

Had you read the following, esp. Note there?

* @PIN_CONFIG_INPUT_ENABLE: enable the pin's input. Note that this does not
* affect the pin's ability to drive output. 1 enables input, 0 disables
* input.

For me manual is clearly tells about this settings:
"This register enables or disables the input buffer while the output
buffer is enabled."

> Regardless, on this part, we needed a way to flag that some pins when put in some function modes needed 'an extra register setting'. At first we tried to sneak that info in with a simple #define in the pin/pinmux DT node properties. But, Linus didn't want it there so we had to make up a new generic property called "bi-directional".
>
> What is your end goal here? Get "bi-directional" changed to something else?

My goal is to reduce amount of (useless) entities. See Occam's razor
for details.

Linus, for me it looks like better to revert that change, until we
will have clear picture why existing configuration parameters can't
work.

--
With Best Regards,
Andy Shevchenko

2017-04-28 15:16:41

[permalink] [raw]

Subject: RE: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable

On Friday, April 28, 2017, Andy Shevchenko wrote:
> Had you read the following, esp. Note there?
>
> * @PIN_CONFIG_INPUT_ENABLE: enable the pin's input. Note that this does
> not
> * affect the pin's ability to drive output. 1 enables input, 0
> disables
> * input.
>
> For me manual is clearly tells about this settings:
> "This register enables or disables the input buffer while the output
> buffer is enabled."

But, then if we use "input-enable" to get bi-directional functionality, now we need something to replace what we were using "input-enable" for.
We were using "input-enable" to signal when the pin function that we set also needs to be forcible set to input by the software (once again, because the HW is not smart enough to do it on its own), but is different than the bi-directional functionality (ie, a different register setting).

So, if we replace "bi-directional" with "input-enable" (since logically internally that is what is going on), what do we use for the special pins that the HW manual says "hey, you need to manually set these pins to input with SW because the pin selection HW can't do it correctly)". Note that we added a enable-output for the same reason.
See RZ/A1H HW Manual section "Table 54.7 Alternative Functions that PIPCn.PIPCnm Bit Should be Set to 0"

Chris

2017-04-28 15:37:41

[permalink] [raw]

Subject: Re: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable

On Fri, Apr 28, 2017 at 6:16 PM, Chris Brandt <[email protected]> wrote:
> On Friday, April 28, 2017, Andy Shevchenko wrote:
>> Had you read the following, esp. Note there?
>>
>> * @PIN_CONFIG_INPUT_ENABLE: enable the pin's input. Note that this does
>> not
>> * affect the pin's ability to drive output. 1 enables input, 0
>> disables
>> * input.
>>
>> For me manual is clearly tells about this settings:
>> "This register enables or disables the input buffer while the output
>> buffer is enabled."
>
>
> But, then if we use "input-enable" to get bi-directional functionality,

It seems you are missing the point from electrical prospective.
Standard pin buffers (electrically) means input buffer and output
buffer that are driven independently in most cases.

Here is one example:
https://electronics.stackexchange.com/questions/96932/internal-circuitry-of-io-ports-in-mcu/96953#96953

(That's what I asked before as a schematic)

> now we need something to replace what we were using "input-enable" for.

No.

> We were using "input-enable" to signal when the pin function that we set also needs to be forcible set to input by the software (once again, because the HW is not smart enough to do it on its own), but is different than the bi-directional functionality (ie, a different register setting).

You are trying to introduce an abstraction, called BiDi, which is
*not* a separate thing from a set of pin properties.

> So, if we replace "bi-directional" with "input-enable" (since logically internally that is what is going on), what do we use for the special pins that the HW manual says "hey, you need to manually set these pins to input with SW because the pin selection HW can't do it correctly)".

Yes.
BiDi is an alias to output + input enable + other pin configuration
parameters (a set depends on real HW needs).

> Note that we added a enable-output for the same reason.
> See RZ/A1H HW Manual section "Table 54.7 Alternative Functions that PIPCn.PIPCnm Bit Should be Set to 0"

Perhaps needs to be revisited as well.

P.S. It looks like more and more software engineers are going far from
real hardware when developing drivers...

--
With Best Regards,
Andy Shevchenko

2017-04-28 16:23:27

[permalink] [raw]

Subject: Re: [PATCH v5 02/10] pinctrl: generic: Add macros to unpack properties

On Fri, Apr 28, 2017 at 11:16 AM, Linus Walleij
<[email protected]> wrote:
> On Thu, Apr 27, 2017 at 10:19 AM, Jacopo Mondi
> <[email protected]> wrote:

> But why.
>
> I have these two static inlines just below your new macros:

+1.

> We generally prefer static inlines over macros because they are easier
> to read.

Not only. It adds type checking as well AFAIUC.

--
With Best Regards,
Andy Shevchenko

2017-04-28 16:46:29

[permalink] [raw]

Subject: RE: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable

On Friday, April 28, 2017, Andy Shevchenko wrote:
> > We were using "input-enable" to signal when the pin function that we set
> also needs to be forcible set to input by the software (once again,
> because the HW is not smart enough to do it on its own), but is different
> than the bi-directional functionality (ie, a different register setting).
>
> You are trying to introduce an abstraction, called BiDi, which is
> *not* a separate thing from a set of pin properties.

Note, I'm talking about 2 different issues we had:

1) Pins that need input and output buffers enabled during normal use. We created "bi-directional" for that.

2) For whatever reason, the HW manual points out that the PFC hardware can't really automatically set buffers enables correctly for some pin instances, and we have to manually assign the pin as input or output using another register. For that, we were using "input-enable" and "output-enable".

For #2:

> > Note that we added a enable-output for the same reason.
> > See RZ/A1H HW Manual section "Table 54.7 Alternative Functions that
> PIPCn.PIPCnm Bit Should be Set to 0"
>
> Perhaps needs to be revisited as well.

Sorry, we didn't 'add' anything new. The property "output-enable", (ie, PIN_CONFIG_OUTPUT) already existed and describes what we are doing in the case for output.

But, we still have the issue that we have 2 cases that need the input enabled, but they are not the same situation, so we can't just use "input-enable" for both.

My only suggestion is (and I'm not sure this is possible in the driver):

"input-enable" : case #2 where you need the pin to be forced as an input
"output-enable" : case #2 where you need the pin to be forced as an output
"input-enable" + "output-enable" : case #1 (replaces "bi-directional").

For example:

i2c2_pins: i2c2 {
pinmux = <RZA1_PINMUX(1, 4, 1)>, <RZA1_PINMUX(1, 5, 1)>;
input-enable;
output-enable;
};

So in the SW driver, if we see both, that will signal to us what is going on and what to do about it (as in, set the bi-directional register and not the input direction register).

Thoughts?

Chris

2017-04-28 21:03:22

by Rob Herring (Arm)

[permalink] [raw]

Subject: Re: [PATCH v5 04/10] dt-bindings: pinctrl: Add RZ/A1 bindings doc

On Thu, Apr 27, 2017 at 10:19:48AM +0200, Jacopo Mondi wrote:
> Add device tree bindings documentation for Renesas RZ/A1 gpio and pin
> controller.
>
> Signed-off-by: Jacopo Mondi <[email protected]>
> ---
> .../bindings/pinctrl/renesas,rza1-pinctrl.txt | 219 +++++++++++++++++++++
> 1 file changed, 219 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/pinctrl/renesas,rza1-pinctrl.txt

Acked-by: Rob Herring <[email protected]>

2017-06-09 07:27:02

by Dong Aisheng

[permalink] [raw]

Subject: Re: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable

Hi Linus & j,

On Mon, May 29, 2017 at 6:42 PM, jmondi <[email protected]> wrote:
> Hi Linus,
>
> On Mon, May 29, 2017 at 10:45:44AM +0200, Linus Walleij wrote:
>> On Tue, May 23, 2017 at 8:37 PM, jmondi <[email protected]> wrote:
>>
>> >> I did not follow too much.
>> >> But it seems IMX7ULP/Vybrid to be also a fan of generic
>> >> output-enable/input-enable
>> >> property.
>> >>
>> >> See:
>> >> Figure 5-2. GPIO PAD in Page 241
>> >> http://www.nxp.com/assets/documents/data/en/reference-manuals/VFXXXRM.pdf
>> >>
>> >> It has separate register bits to control input buffer enable and
>> >> output buffer enable
>> >> and we need set it property for GPIO function.
>> >
>> > As it seems we have another user for 'output-enable' here, what if we just
>> > add that one to the generic bindings properties list, and we keep
>> > 'bi-directional' (which seems to be the most debated property we have
>> > added) out of generic properties?
>> >
>> > We can handle 'bi-directional' pins with static tables in our pin
>> > controller driver and not have it anywhere in DT.
>>
>> This sounds like a viable approach.
>>
>> I just want to know if "output-enable" is the right name?
>> "output-buffer-enable"?
>
> Great! Thanks!
>
> On naming: if we need "output-buffer-enable" should we add
> "input-buffer-enable" as well?
>
> Currently we are using "input-enable" to pair with "output-enable",
> but as you said, just "output-enable" when "output-high" and
> "output-low" are there already seems a bit confusing.
> At the same time "input-buffer-enable" seems to actually be just
> electrically equivalent to "input-enable", so adding it is a bit of a
> waste as well.
>
> I see three options here:
>
> 1) Add "output-buffer-enable" and "input-buffer-enable"
> we end up with
> "output-high"
> "output-low"
> "input-enable"
> "output-buffer-enable"
> "input-buffer-enable"
>
> 2) Add "output-buffer-enable" only
> we end up with
> "output-high"
> "output-low"
> "input-enable"
> "output-buffer-enable"
>
> Binding may be confusing as in one case we use "output-buffer-enable"
> while in the other "input-enable"
>
> 3) Add "output-enable" only
> "output-high"
> "output-low"
> "input-enable"
> "output-enable"
>
> As you, I don't like "output-enable" that much but it pairs better with
> "input-enable".
>
> I'll let you and DT people decide on this, as it's really an ABI definition
> problem and you have better judgment there.
>

What's the final decision of this?

I saw the following revert patch in pinctrl-next but did not see a successive
patch to add output-enable back?

IMX7ULP pinctrl driver is pending on this because it needs use both
input-enable and output-enable if we want to make them generic property.

commit b4d2ea2af95cb77e2f320e24da526280d4aa2f6b
Author: Linus Walleij <[email protected]>
Date: Mon May 8 10:48:21 2017 +0200

Revert "pinctrl: generic: Add bi-directional and output-enable"

This reverts commit 8c58f1a7a4b6d1d723bf25fef9d842d5a11200d0.

It turns out that applying these generic properties was
premature: the properties used in the driver using this
are of unclear electrical nature and the subject need to
be discussed.

Signed-off-by: Linus Walleij <[email protected]>

Regards
Dong Aisheng

>>
>> > I see commit 42d5a11200d0[1] has not been reverted yet as Andy asked
>> > in some previous email.
>>
>> I'm just overloaded. I sent that revert to Torvalds today.
>
> Thank you. Didn't want to put pressure ;)
>>
>> > I can send another version of that patch with
>> > only 'output-enable' if you wish.
>>
>> That's what we want.
>>
>> > Once we reach consesus, I can then send v6 of our pin controller driver
>> > based on that.
>>
>> OK sounds like a plan.
>>
>> Sorry for the mess, I'm just trying to get this right :/
>
> Not a mess, and thanks for your effort in maintaining all of this
>
> Thanks
> j
>>
>> Yours,
>> Linus Walleij

2017-06-09 07:50:40

[permalink] [raw]

Subject: Re: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable

Hi Dong,

On Fri, Jun 09, 2017 at 03:26:57PM +0800, Dong Aisheng wrote:
> Hi Linus & j,
>
> >>
> >> I just want to know if "output-enable" is the right name?
> >> "output-buffer-enable"?
> >
> > Great! Thanks!
> >
> > On naming: if we need "output-buffer-enable" should we add
> > "input-buffer-enable" as well?
> >
> > Currently we are using "input-enable" to pair with "output-enable",
> > but as you said, just "output-enable" when "output-high" and
> > "output-low" are there already seems a bit confusing.
> > At the same time "input-buffer-enable" seems to actually be just
> > electrically equivalent to "input-enable", so adding it is a bit of a
> > waste as well.
> >
> > I see three options here:
> >
> > 1) Add "output-buffer-enable" and "input-buffer-enable"
> > we end up with
> > "output-high"
> > "output-low"
> > "input-enable"
> > "output-buffer-enable"
> > "input-buffer-enable"
> >
> > 2) Add "output-buffer-enable" only
> > we end up with
> > "output-high"
> > "output-low"
> > "input-enable"
> > "output-buffer-enable"
> >
> > Binding may be confusing as in one case we use "output-buffer-enable"
> > while in the other "input-enable"
> >
> > 3) Add "output-enable" only
> > "output-high"
> > "output-low"
> > "input-enable"
> > "output-enable"
> >
> > As you, I don't like "output-enable" that much but it pairs better with
> > "input-enable".
> >
> > I'll let you and DT people decide on this, as it's really an ABI definition
> > problem and you have better judgment there.
> >
>
> What's the final decision of this?

I admit a was buying a bit of time and post-poned the gentle ping for
any final word on this. But since you're asking I'll second your
question :)

>
> I saw the following revert patch in pinctrl-next but did not see a successive
> patch to add output-enable back?
>

Still waiting to have a feedback on which properties to add, that's
why I have not sent anything yet.

Thanks
j

> IMX7ULP pinctrl driver is pending on this because it needs use both
> input-enable and output-enable if we want to make them generic property.
>
> commit b4d2ea2af95cb77e2f320e24da526280d4aa2f6b
> Author: Linus Walleij <[email protected]>
> Date: Mon May 8 10:48:21 2017 +0200
>
> Revert "pinctrl: generic: Add bi-directional and output-enable"
>
> This reverts commit 8c58f1a7a4b6d1d723bf25fef9d842d5a11200d0.
>
> It turns out that applying these generic properties was
> premature: the properties used in the driver using this
> are of unclear electrical nature and the subject need to
> be discussed.
>
> Signed-off-by: Linus Walleij <[email protected]>
>
> Regards
> Dong Aisheng
>
> >>
> >> > I see commit 42d5a11200d0[1] has not been reverted yet as Andy asked
> >> > in some previous email.
> >>
> >> I'm just overloaded. I sent that revert to Torvalds today.
> >
> > Thank you. Didn't want to put pressure ;)
> >>
> >> > I can send another version of that patch with
> >> > only 'output-enable' if you wish.
> >>
> >> That's what we want.
> >>
> >> > Once we reach consesus, I can then send v6 of our pin controller driver
> >> > based on that.
> >>
> >> OK sounds like a plan.
> >>
> >> Sorry for the mess, I'm just trying to get this right :/
> >
> > Not a mess, and thanks for your effort in maintaining all of this
> >
> > Thanks
> > j
> >>
> >> Yours,
> >> Linus Walleij

2017-06-11 21:45:54

[permalink] [raw]

Subject: Re: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable

On Fri, Jun 9, 2017 at 9:50 AM, jmondi <[email protected]> wrote:
> On Fri, Jun 09, 2017 at 03:26:57PM +0800, Dong Aisheng wrote:

>> > I see three options here:
>> >
>> > 1) Add "output-buffer-enable" and "input-buffer-enable"
>> > we end up with
>> > "output-high"
>> > "output-low"
>> > "input-enable"
>> > "output-buffer-enable"
>> > "input-buffer-enable"
>> >
>> > 2) Add "output-buffer-enable" only
>> > we end up with
>> > "output-high"
>> > "output-low"
>> > "input-enable"
>> > "output-buffer-enable"
>> >
>> > Binding may be confusing as in one case we use "output-buffer-enable"
>> > while in the other "input-enable"
>> >
>> > 3) Add "output-enable" only
>> > "output-high"
>> > "output-low"
>> > "input-enable"
>> > "output-enable"
>> >
>> > As you, I don't like "output-enable" that much but it pairs better with
>> > "input-enable".
>> >
>> > I'll let you and DT people decide on this, as it's really an ABI definition
>> > problem and you have better judgment there.
>> >
>>
>> What's the final decision of this?
>
> I admit a was buying a bit of time and post-poned the gentle ping for
> any final word on this. But since you're asking I'll second your
> question :)

I suspect it is time to quote
Documentation/process/management-style.rst
(Torvalds):

1) Decisions

Everybody thinks managers make decisions, and that decision-making is
important. The bigger and more painful the decision, the bigger the
manager must be to make it. That's very deep and obvious, but it's not
actually true.

The name of the game is to **avoid** having to make a decision. In
particular, if somebody tells you "choose (a) or (b), we really need you
to decide on this", you're in trouble as a manager. The people you
manage had better know the details better than you, so if they come to
you for a technical decision, you're screwed. You're clearly not
competent to make that decision for them.

(It goes on, it's the best part of the entire Documentation/* dir in my
opinion, please take the time to read it in full.)

So: what do you guys, using this feature, and Andy, who raised serious
concerns, think is the right binding? That is what *I* need to know.

Yours,
Linus Walleij

2017-06-12 09:44:41

[permalink] [raw]

Subject: Re: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable

Hi Linus,

On Sun, Jun 11, 2017 at 11:45:49PM +0200, Linus Walleij wrote:
> On Fri, Jun 9, 2017 at 9:50 AM, jmondi <[email protected]> wrote:
> > On Fri, Jun 09, 2017 at 03:26:57PM +0800, Dong Aisheng wrote:
>
> >> > I see three options here:
> >> >
> >> > 1) Add "output-buffer-enable" and "input-buffer-enable"
> >> > we end up with
> >> > "output-high"
> >> > "output-low"
> >> > "input-enable"
> >> > "output-buffer-enable"
> >> > "input-buffer-enable"
> >> >
> >> > 2) Add "output-buffer-enable" only
> >> > we end up with
> >> > "output-high"
> >> > "output-low"
> >> > "input-enable"
> >> > "output-buffer-enable"
> >> >
> >> > Binding may be confusing as in one case we use "output-buffer-enable"
> >> > while in the other "input-enable"
> >> >
> >> > 3) Add "output-enable" only
> >> > "output-high"
> >> > "output-low"
> >> > "input-enable"
> >> > "output-enable"
> >> >
> >> > As you, I don't like "output-enable" that much but it pairs better with
> >> > "input-enable".
> >> >
> >> > I'll let you and DT people decide on this, as it's really an ABI definition
> >> > problem and you have better judgment there.
> >> >
> >>
> >> What's the final decision of this?
> >
> > I admit a was buying a bit of time and post-poned the gentle ping for
> > any final word on this. But since you're asking I'll second your
> > question :)
>
> I suspect it is time to quote
> Documentation/process/management-style.rst
> (Torvalds):
>
> 1) Decisions
>
> Everybody thinks managers make decisions, and that decision-making is
> important. The bigger and more painful the decision, the bigger the
> manager must be to make it. That's very deep and obvious, but it's not
> actually true.
>
> The name of the game is to **avoid** having to make a decision. In
> particular, if somebody tells you "choose (a) or (b), we really need you
> to decide on this", you're in trouble as a manager. The people you
> manage had better know the details better than you, so if they come to
> you for a technical decision, you're screwed. You're clearly not
> competent to make that decision for them.
>
> (It goes on, it's the best part of the entire Documentation/* dir in my
> opinion, please take the time to read it in full.)
>
> So: what do you guys, using this feature, and Andy, who raised serious
> concerns, think is the right binding? That is what *I* need to know.

Fair enough :)

I'll try to keep this short: I don't like "output-enable", and at the
same time I don't think "output-high" and "output-low" fit well for
this purpose, as they electrically means something different from what
our (and IMX) use case is: enabling/disabling input/output
buffers internal to pin controller/gpio block HW and not driving a value
there.

This seems clear to me from the "GPIO mode pitfalls" section of
pinctrl.txt documentation examples and from the fact that generic bindings
did not expose an "output" flag because if you drive an output line, you
reasonably either drive it high or low.

Unfortunately I cannot convince myself that the same reasons apply
to the input use case. Enabling input on a pin implies the pinctrl/gpio driver
has to enable any input buffer required to use that pin as a properly
working input line, and enabling an input buffer implies being able to sense
the line value from there, so I don't see that much use for "input-buffer-enable"
alone.

So, even if bindings could look a bit weird as there won't be a direct
matching between properties names used to enable input/output buffers,
my vote is to add "output-buffer-enable" only, and keep using the
already there "input-enable" properties for the input use case.

Thanks
j

>
> Yours,
> Linus Walleij

2017-06-13 06:25:14

by Dong Aisheng

[permalink] [raw]

Subject: Re: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable

On Mon, Jun 12, 2017 at 5:44 PM, jmondi <[email protected]> wrote:
> Hi Linus,
>
> On Sun, Jun 11, 2017 at 11:45:49PM +0200, Linus Walleij wrote:
>> On Fri, Jun 9, 2017 at 9:50 AM, jmondi <[email protected]> wrote:
>> > On Fri, Jun 09, 2017 at 03:26:57PM +0800, Dong Aisheng wrote:
>>
>> >> > I see three options here:
>> >> >
>> >> > 1) Add "output-buffer-enable" and "input-buffer-enable"
>> >> > we end up with
>> >> > "output-high"
>> >> > "output-low"
>> >> > "input-enable"
>> >> > "output-buffer-enable"
>> >> > "input-buffer-enable"
>> >> >
>> >> > 2) Add "output-buffer-enable" only
>> >> > we end up with
>> >> > "output-high"
>> >> > "output-low"
>> >> > "input-enable"
>> >> > "output-buffer-enable"
>> >> >
>> >> > Binding may be confusing as in one case we use "output-buffer-enable"
>> >> > while in the other "input-enable"
>> >> >
>> >> > 3) Add "output-enable" only
>> >> > "output-high"
>> >> > "output-low"
>> >> > "input-enable"
>> >> > "output-enable"
>> >> >
>> >> > As you, I don't like "output-enable" that much but it pairs better with
>> >> > "input-enable".
>> >> >
>> >> > I'll let you and DT people decide on this, as it's really an ABI definition
>> >> > problem and you have better judgment there.
>> >> >
>> >>
>> >> What's the final decision of this?
>> >
>> > I admit a was buying a bit of time and post-poned the gentle ping for
>> > any final word on this. But since you're asking I'll second your
>> > question :)
>>
>> I suspect it is time to quote
>> Documentation/process/management-style.rst
>> (Torvalds):
>>
>> 1) Decisions
>>
>> Everybody thinks managers make decisions, and that decision-making is
>> important. The bigger and more painful the decision, the bigger the
>> manager must be to make it. That's very deep and obvious, but it's not
>> actually true.
>>
>> The name of the game is to **avoid** having to make a decision. In
>> particular, if somebody tells you "choose (a) or (b), we really need you
>> to decide on this", you're in trouble as a manager. The people you
>> manage had better know the details better than you, so if they come to
>> you for a technical decision, you're screwed. You're clearly not
>> competent to make that decision for them.
>>
>> (It goes on, it's the best part of the entire Documentation/* dir in my
>> opinion, please take the time to read it in full.)
>>
>> So: what do you guys, using this feature, and Andy, who raised serious
>> concerns, think is the right binding? That is what *I* need to know.
>
> Fair enough :)
>
> I'll try to keep this short: I don't like "output-enable", and at the
> same time I don't think "output-high" and "output-low" fit well for
> this purpose, as they electrically means something different from what
> our (and IMX) use case is: enabling/disabling input/output
> buffers internal to pin controller/gpio block HW and not driving a value
> there.
>
> This seems clear to me from the "GPIO mode pitfalls" section of
> pinctrl.txt documentation examples and from the fact that generic bindings
> did not expose an "output" flag because if you drive an output line, you
> reasonably either drive it high or low.
>
> Unfortunately I cannot convince myself that the same reasons apply
> to the input use case. Enabling input on a pin implies the pinctrl/gpio driver
> has to enable any input buffer required to use that pin as a properly
> working input line, and enabling an input buffer implies being able to sense
> the line value from there, so I don't see that much use for "input-buffer-enable"
> alone.
>
> So, even if bindings could look a bit weird as there won't be a direct
> matching between properties names used to enable input/output buffers,
> my vote is to add "output-buffer-enable" only, and keep using the
> already there "input-enable" properties for the input use case.
>

Yes, it may be a bit weird.
I'm not pad internal details expert and can't tell much difference between
output-enable and output-buffer-enable.
I just feel a bit confuse if only using output-buffer-enable.

If enable both input and output, it becomes:
pinctrl_xxx: gpios_xxx_grp {
pins = <
ULP1_PAD_PTD0__PTD0
>;
input-enable;
output-buffer-enable;
bias-pull-up;
};

How about still use output-enable in pairs to input-enable but explain more
in comments?
Aslo update 'input-enable' comment to 'enable input buffer'.
e.g.
diff --git a/drivers/pinctrl/pinconf-generic.c
b/drivers/pinctrl/pinconf-generic.c
index 720a19f..96c83a4 100644
--- a/drivers/pinctrl/pinconf-generic.c
+++ b/drivers/pinctrl/pinconf-generic.c
@@ -172,6 +172,7 @@ static const struct pinconf_generic_params dt_params[] = {
{ "input-schmitt-enable", PIN_CONFIG_INPUT_SCHMITT_ENABLE, 1 },
{ "low-power-disable", PIN_CONFIG_LOW_POWER_MODE, 0 },
{ "low-power-enable", PIN_CONFIG_LOW_POWER_MODE, 1 },
+ { "output-enable", PIN_CONFIG_OUTPUT_ENABLE, 1 },
{ "output-high", PIN_CONFIG_OUTPUT, 1, },
{ "output-low", PIN_CONFIG_OUTPUT, 0, },
{ "power-source", PIN_CONFIG_POWER_SOURCE, 0 },
diff --git a/include/linux/pinctrl/pinconf-generic.h
b/include/linux/pinctrl/pinconf-generic.h
index 7620eb1..d30f4fe 100644
--- a/include/linux/pinctrl/pinconf-generic.h
+++ b/include/linux/pinctrl/pinconf-generic.h
@@ -59,9 +59,9 @@
* which means it will wait for signals to settle when reading inputs. The
* argument gives the debounce time in usecs. Setting the
* argument to zero turns debouncing off.
- * @PIN_CONFIG_INPUT_ENABLE: enable the pin's input. Note that this does not
- * affect the pin's ability to drive output. 1 enables input, 0 disables
- * input.
+ * @PIN_CONFIG_INPUT_ENABLE: enable the pin's input buffer. Note
that this does
+ * not affect the pin's ability to drive output.
+ * 1 enables input, 0 disables input.
* @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
@@ -73,6 +73,9 @@
* operation, if several modes of operation are supported these can be
* passed in the argument on a custom form, else just use argument 1
* to indicate low power mode, argument 0 turns low power mode off.
+ * @PIN_CONFIG_OUTPUT_ENABLE: only enable the pin's output buffer, not driving
+ * a value.
+ * 1 enables output buffer, 0 disables output buffer.
* @PIN_CONFIG_OUTPUT: this will configure the pin as an output. Use argument
* 1 to indicate high level, argument 0 to indicate low level. (Please
* see Documentation/pinctrl.txt, section "GPIO mode pitfalls" for a

Or
invent both input-buffer-enable and output-buffer-enable and
deprecated input-enable?

Andy,
how about your comments?

Regards
Dong Aisheng

> Thanks
> j
>
>>
>> Yours,
>> Linus Walleij

2017-06-15 11:11:33