LinuxLists.cc - [PATCH v2 0/7] Renesas RZ/A1 pin and gpio controller

[permalink] [raw]

Subject: [PATCH v2 7/7] arm: dts: genmai: Add user led device nodes

Add device nodes for user leds on Genmai board.

Signed-off-by: Jacopo Mondi <[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 30992b1..94b9ab7 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-03-20 16:27:23

[permalink] [raw]

Subject: [PATCH v2 6/7] arm: dts: genmai: Add RIIC2 pin group

Add pin configuration subnode for RIIC2 interface.

Signed-off-by: Jacopo Mondi <[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 569c86a..30992b1 100644
--- a/arch/arm/boot/dts/r7s72100-genmai.dts
+++ b/arch/arm/boot/dts/r7s72100-genmai.dts
@@ -44,6 +44,12 @@
renesas,pins = <PIN(3, 0) 6>,
<PIN(3, 2) 4>;
};
+
+ i2c2_pins: i2c2 {
+ /* RIIC2: P1_4 as SCL, P1_5 as SDA */
+ renesas,pins = <PIN(1, 4) (1 | BI_DIR)>,
+ <PIN(1, 5) (1 | BI_DIR)>;
+ };
};

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

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

2017-03-20 16:46:56

[permalink] [raw]

Subject: [PATCH v2 4/7] 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]>
---
arch/arm/boot/dts/r7s72100.dtsi | 80 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 80 insertions(+)

diff --git a/arch/arm/boot/dts/r7s72100.dtsi b/arch/arm/boot/dts/r7s72100.dtsi
index 614ba79..9eb8751f 100644
--- a/arch/arm/boot/dts/r7s72100.dtsi
+++ b/arch/arm/boot/dts/r7s72100.dtsi
@@ -183,6 +183,86 @@
};
};

+ pinctrl: pinctrl@fcfe3000 {
+ compatible = "renesas,r7s72100-ports";
+
+ #pinctrl-cells = <1>;
+
+ reg = <0xfcfe3000 0x4248>;
+
+ 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-03-20 16:46:53

[permalink] [raw]

Subject: [PATCH v2 1/7] 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 | 10 +
drivers/pinctrl/Makefile | 1 +
drivers/pinctrl/pinctrl-rza1.c | 963 +++++++++++++++++++++++++++++++++++++++++
3 files changed, 974 insertions(+)
create mode 100644 drivers/pinctrl/pinctrl-rza1.c

diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index 8f8c2af..c9b55b9 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -163,6 +163,16 @@ 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 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..d2f50ab
--- /dev/null
+++ b/drivers/pinctrl/pinctrl-rza1.c
@@ -0,0 +1,963 @@
+/*
+ * 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 (r7s721001) and RZ/A1L (r7s721002) 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 "pinmux.h"
+
+#define DRIVER_NAME "pinctrl-rza1"
+
+#define RZA1_PINMUX_OF_ARGS 2
+
+#define P_REG 0x0000
+#define PPR_REG 0x0200
+#define PM_REG 0x0300
+#define PMC_REG 0x0400
+#define PFC_REG 0x0500
+#define PFCE_REG 0x0600
+#define PFCEA_REG 0x0a00
+#define PIBC_REG 0x4000
+#define PBDC_REG 0x4100
+#define 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_TO_PORT(pin) ((pin) / RZA1_PINS_PER_PORT)
+#define RZA1_PIN_TO_OFFSET(pin) ((pin) % RZA1_PINS_PER_PORT)
+
+/*
+ * Be careful here: the pin configuration subnodes in device tree enumerates
+ * alternate functions from 1 to 8; subtract 1 before using macros so to match
+ * registers configuration which expects numbers from 0 to 7 instead.
+ */
+#define MUX_FUNC_OFFS 3
+#define MUX_FUNC_MASK (BIT(MUX_FUNC_OFFS) - 1)
+#define MUX_FUNC_PFC_MASK BIT(0)
+#define MUX_FUNC_PFCE_MASK BIT(1)
+#define MUX_FUNC_PFCEA_MASK BIT(2)
+#define MUX_CONF_BIDIR BIT(0)
+#define MUX_CONF_SWIO_INPUT BIT(1)
+#define MUX_CONF_SWIO_OUTPUT BIT(2)
+
+/**
+ * rza1_pin_conf - describes a pin position, id, mux config and output value
+ *
+ * Use uint32_t to match types used in of_device nodes argument lists.
+ *
+ * @id: the pin identifier from 0 to RZA1_NPINS
+ * @port: the port where pin sits on
+ * @offset: pin offset in the port
+ * @mux: alternate function configuration settings
+ * @value: output value to set the pin to
+ */
+struct rza1_pin_conf {
+ uint32_t id;
+ uint32_t port;
+ uint32_t offset;
+ uint32_t mux_conf;
+ uint32_t 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
+ * @ngpiochips: number of gpio chips
+ * @gpio_ranges: gpio ranges for pinctrl core
+ * @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;
+
+ unsigned int ngpiochips;
+
+ struct pinctrl_gpio_range *gpio_ranges;
+ 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(const struct rza1_port *port,
+ unsigned int reg, unsigned int offset,
+ bool set)
+{
+ void __iomem *mem = RZA1_ADDR(port->base, reg, port->id);
+ u16 val = ioread16(mem);
+
+ if (set)
+ val |= BIT(offset);
+ else
+ val &= ~BIT(offset);
+
+ iowrite16(val, mem);
+}
+
+static inline int rza1_get_bit(struct rza1_port *port,
+ unsigned int reg, unsigned int offset)
+{
+ void __iomem *mem = RZA1_ADDR(port->base, reg, port->id);
+
+ return ioread16(mem) & BIT(offset);
+}
+
+/**
+ * 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
+ * @offset: pin offset
+ */
+static void rza1_pin_reset(struct rza1_port *port,
+ unsigned int offset)
+{
+ spin_lock(&port->lock);
+ rza1_set_bit(port, PIBC_REG, offset, 0);
+ rza1_set_bit(port, PBDC_REG, offset, 0);
+
+ rza1_set_bit(port, PM_REG, offset, 1);
+ rza1_set_bit(port, PMC_REG, offset, 0);
+ rza1_set_bit(port, PIPC_REG, offset, 0);
+ spin_unlock(&port->lock);
+}
+
+static inline int rza1_pin_get_direction(struct rza1_port *port,
+ int offset)
+{
+ int input;
+
+ spin_lock(&port->lock);
+ input = rza1_get_bit(port, PM_REG, offset);
+ spin_unlock(&port->lock);
+
+ 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
+ * @offset: pin offset
+ * @input: input enable/disable flag
+ */
+static inline void rza1_pin_set_direction(struct rza1_port *port,
+ unsigned int offset,
+ bool input)
+{
+ spin_lock(&port->lock);
+
+ rza1_set_bit(port, PIBC_REG, offset, 1);
+ if (input) {
+ rza1_set_bit(port, PM_REG, offset, 1);
+ rza1_set_bit(port, PBDC_REG, offset, 0);
+ } else {
+ rza1_set_bit(port, PM_REG, offset, 0);
+ rza1_set_bit(port, PBDC_REG, offset, 1);
+ }
+
+ spin_unlock(&port->lock);
+}
+
+static inline void rza1_pin_set(struct rza1_port *port,
+ unsigned int offset, unsigned int value)
+{
+ spin_lock(&port->lock);
+ rza1_set_bit(port, P_REG, offset, !!value);
+ spin_unlock(&port->lock);
+}
+
+static inline int rza1_pin_get(struct rza1_port *port, unsigned int offset)
+{
+ int val;
+
+ spin_lock(&port->lock);
+ val = rza1_get_bit(port, PPR_REG, offset);
+ spin_unlock(&port->lock);
+
+ return val;
+}
+
+/**
+ * rza1_pin_conf_validate() - make sure a single bit it set in mux_conf mask
+ */
+static inline int rza1_pin_conf_validate(u8 mux_conf)
+{
+ do {
+ if (mux_conf & BIT(0))
+ break;
+ } while ((mux_conf >>= 1));
+
+ return (mux_conf >> 1);
+}
+
+/**
+ * rza1_alternate_function_conf() - configure pin in alternate function mode
+ *
+ * @pinctrl: RZ/A1 pin controller device
+ * @pin_conf: single pin configuration descriptor
+ */
+static int rza1_alternate_function_conf(struct rza1_pinctrl *rza1_pctl,
+ struct rza1_pin_conf *pin_conf)
+{
+ unsigned int offset = pin_conf->offset;
+ struct rza1_port *port = &rza1_pctl->ports[pin_conf->port];
+ u8 mux_mode = (pin_conf->mux_conf - 1) & MUX_FUNC_MASK;
+ u8 mux_conf = pin_conf->mux_conf >> MUX_FUNC_OFFS;
+ bool bidir_en = !!(mux_conf & MUX_CONF_BIDIR);
+ bool swio_in = !!(mux_conf & MUX_CONF_SWIO_INPUT);
+ bool swio_out = !!(mux_conf & MUX_CONF_SWIO_OUTPUT);
+
+ if (rza1_pin_conf_validate(mux_conf)) {
+ dev_err(rza1_pctl->dev,
+ "Invalid pin configuration for pin %u:%u",
+ port->id, offset);
+ return -EINVAL;
+ }
+
+ rza1_pin_reset(port, offset);
+
+ if (bidir_en)
+ rza1_set_bit(port, PBDC_REG, offset, 1);
+
+ /*
+ * Enable alternate function mode and select it.
+ *
+ * ----------------------------------------------------
+ * Alternate mode selection table:
+ *
+ * PMC PFC PFCE PFCAE mux_mode
+ * 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, PFC_REG, offset, mux_mode & MUX_FUNC_PFC_MASK);
+ rza1_set_bit(port, PFCE_REG, offset, mux_mode & MUX_FUNC_PFCE_MASK);
+ rza1_set_bit(port, PFCEA_REG, offset, mux_mode & MUX_FUNC_PFCEA_MASK);
+
+ /*
+ * All alternate functions except a few (4) 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 (!(swio_in || swio_out))
+ rza1_set_bit(port, PIPC_REG, offset, 1);
+
+ rza1_set_bit(port, PMC_REG, offset, 1);
+ rza1_set_bit(port, PM_REG, offset, swio_in);
+
+ return 0;
+}
+
+/* ----------------------------------------------------------------------------
+ * pinctrl operations
+ */
+
+/**
+ * rza1_dt_node_to_map() - map a node to a function/group map
+ *
+ * Functions and groups are collected and registered to pinctrl_generic
+ * during DT parsing routine.
+ *
+ * @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 group_desc *grp;
+ unsigned int grp_sel;
+
+ /*
+ * Find the group of this node and check if we need create
+ * config maps for pins.
+ */
+ grp_sel = pinctrl_get_group_selector(pctldev, np->name);
+ if (grp_sel < 0) {
+ dev_err(rza1_pctl->dev, "unable to find group for node %s\n",
+ np->name);
+ return -EINVAL;
+ }
+
+ grp = pinctrl_generic_get_group(pctldev, grp_sel);
+ if (!grp) {
+ dev_err(rza1_pctl->dev, "unable to find group for node %s\n",
+ np->name);
+ return -EINVAL;
+ }
+
+ *num_maps = 0;
+ *map = kzalloc(sizeof(**map), GFP_KERNEL);
+ if (!*map)
+ return -ENOMEM;
+
+ (*map)->type = PIN_MAP_TYPE_MUX_GROUP;
+ (*map)->data.mux.group = np->name;
+ (*map)->data.mux.function = np->name;
+ *num_maps = 1;
+
+ return 0;
+}
+
+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,
+};
+
+/* ----------------------------------------------------------------------------
+ * 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 offset)
+{
+ struct rza1_port *port = gpiochip_get_data(chip);
+
+ return rza1_pin_get_direction(port, offset);
+}
+
+static int rza1_gpio_direction_input(struct gpio_chip *chip,
+ unsigned int offset)
+{
+ struct rza1_port *port = gpiochip_get_data(chip);
+
+ rza1_pin_set_direction(port, offset, true);
+
+ return 0;
+}
+
+static int rza1_gpio_direction_output(struct gpio_chip *chip,
+ unsigned int offset,
+ int value)
+{
+ struct rza1_port *port = gpiochip_get_data(chip);
+
+ /* Set value before driving pin direction */
+ rza1_pin_set(port, offset, value);
+ rza1_pin_set_direction(port, offset, 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 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 offset)
+{
+ struct rza1_port *port = gpiochip_get_data(chip);
+
+ return rza1_pin_get(port, offset);
+}
+
+static void rza1_gpio_set(struct gpio_chip *chip, unsigned int offset,
+ int value)
+{
+ struct rza1_port *port = gpiochip_get_data(chip);
+
+ rza1_pin_set(port, offset, 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,
+};
+
+/* ----------------------------------------------------------------------------
+ * pinmux operations
+ */
+
+/**
+ * rza1_pinmux_set() - retrieve pins from a group and apply them mux settings
+ *
+ * @pctldev: pin controller device
+ * @selector: function selector
+ * @group: group selector
+ */
+static int rza1_pinmux_set(struct pinctrl_dev *pctldev, unsigned int selector,
+ unsigned int group)
+{
+ int i;
+ struct group_desc *grp;
+ struct function_desc *func;
+ struct rza1_pin_conf *pin_confs;
+ struct rza1_pinctrl *rza1_pctl = pinctrl_dev_get_drvdata(pctldev);
+
+ grp = pinctrl_generic_get_group(pctldev, group);
+ if (!grp)
+ return -EINVAL;
+
+ func = pinmux_generic_get_function(pctldev, selector);
+ if (!func)
+ return -EINVAL;
+
+ pin_confs = (struct rza1_pin_conf *)func->data;
+ for (i = 0; i < grp->num_pins; ++i) {
+ int ret;
+
+ ret = rza1_alternate_function_conf(rza1_pctl, &pin_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_pinmux_set,
+ .strict = true,
+};
+
+/* ----------------------------------------------------------------------------
+ * RZ/A1 pin controller driver operations
+ */
+
+static unsigned int rza1_count_gpio_chips(struct device_node *np)
+{
+ unsigned int count = 0;
+ struct device_node *child;
+
+ for_each_child_of_node(np, child) {
+ if (!of_property_read_bool(child, "gpio-controller"))
+ continue;
+
+ count++;
+ }
+
+ return count;
+}
+
+/**
+ * rza1_parse_pmx_function() - parse and register a pin mux function
+ *
+ * Pins for RZ SoC pin controller described by "renesas-pins" property.
+ *
+ * First argument in the list identifies the pin, while the second one
+ * describes the requested alternate function number and additional
+ * configuration parameter to be applied to the selected function.
+ *
+ * @rza1_pctl: RZ/A1 pin controller device
+ * @np: of pmx sub-node
+ */
+static int rza1_parse_pmx_function(struct rza1_pinctrl *rza1_pctl,
+ struct device_node *np)
+{
+ int ret;
+ int of_pins;
+ unsigned int i;
+ unsigned int *grpins;
+ const char *grpname;
+ const char **fngrps;
+ struct rza1_pin_conf *pin_confs;
+ struct pinctrl_dev *pctldev = rza1_pctl->pctl;
+ char const *prop_name = "renesas,pins";
+
+ of_pins = pinctrl_count_index_with_args(np, prop_name);
+ if (of_pins <= 0) {
+ dev_err(rza1_pctl->dev, "Missing %s property\n", prop_name);
+ return -ENOENT;
+ }
+
+ /*
+ * Functions are made of 1 group only;
+ * in facts, functions and groups are identical for this pin controller
+ * except that functions carry an array of per-pin configurations
+ * settings.
+ */
+ pin_confs = devm_kcalloc(rza1_pctl->dev, of_pins, sizeof(*pin_confs),
+ GFP_KERNEL);
+ grpins = devm_kcalloc(rza1_pctl->dev, of_pins, sizeof(*grpins),
+ GFP_KERNEL);
+ fngrps = devm_kzalloc(rza1_pctl->dev, sizeof(*fngrps), GFP_KERNEL);
+
+ if (!pin_confs || !grpins || !fngrps)
+ return -ENOMEM;
+
+ /* Collect pin positions and mux settings to store them in function */
+ for (i = 0; i < of_pins; ++i) {
+ struct rza1_pin_conf *pin_conf = &pin_confs[i];
+ struct of_phandle_args of_pins_args;
+
+ ret = pinctrl_parse_index_with_args(np, prop_name, i,
+ &of_pins_args);
+ if (ret)
+ return ret;
+
+ if (of_pins_args.args_count < RZA1_PINMUX_OF_ARGS) {
+ dev_err(rza1_pctl->dev,
+ "Wrong arguments number for %s property\n",
+ prop_name);
+ return -EINVAL;
+ }
+
+ /*
+ * This new pins configuration will be associated with a new
+ * function, and later used to set-up pin muxing
+ */
+ pin_conf->id = of_pins_args.args[0];
+ pin_conf->port = RZA1_PIN_TO_PORT(pin_conf->id);
+ pin_conf->offset = RZA1_PIN_TO_OFFSET(pin_conf->id);
+ pin_conf->mux_conf = of_pins_args.args[1];
+
+ if (pin_conf->port >= RZA1_NPORTS ||
+ pin_conf->offset >= RZA1_PINS_PER_PORT) {
+ dev_err(rza1_pctl->dev,
+ "Wrong port %u pin %u for %s property\n",
+ pin_conf->port, pin_conf->offset, prop_name);
+ return -EINVAL;
+ }
+
+ grpins[i] = pin_conf->id;
+ }
+
+ grpname = np->name;
+ fngrps[0] = grpname;
+
+ mutex_lock(&rza1_pctl->mutex);
+ ret = pinctrl_generic_add_group(pctldev, grpname, grpins, of_pins,
+ NULL);
+ if (ret) {
+ mutex_unlock(&rza1_pctl->mutex);
+ return ret;
+ }
+
+ ret = pinmux_generic_add_function(pctldev, grpname, fngrps, 1,
+ pin_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, of_pins);
+
+ return 0;
+
+remove_group:
+ dev_info(rza1_pctl->dev, "Unable to parse function and group %s\n",
+ grpname);
+ pinctrl_generic_remove_last_group(pctldev);
+ mutex_unlock(&rza1_pctl->mutex);
+
+ return ret;
+}
+
+/**
+ * rza1_remove_pmx_functions() - un-register pmx functions and groups
+ *
+ * @rza1_pctl: RZ/A1 pin controller device
+ */
+static void rza1_remove_pmx_functions(struct rza1_pinctrl *rza1_pctl)
+{
+ struct pinctrl_dev *pctldev = rza1_pctl->pctl;
+
+ mutex_lock(&rza1_pctl->mutex);
+ pinmux_generic_free_functions(pctldev);
+ mutex_unlock(&rza1_pctl->mutex);
+}
+
+/**
+ * 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.
+ * Gpio chips and pin ranges are here collected, but ranges are registered
+ * later, after pin controller has been registered too. Only gpiochips are
+ * registered here.
+ *
+ * @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)
+{
+ int ret;
+ u32 pinctrl_base;
+ unsigned int gpioport;
+ struct of_phandle_args of_args;
+ const char *list_name = "gpio-ranges";
+
+ of_parse_phandle_with_fixed_args(np, list_name, 3, 0, &of_args);
+
+ /*
+ * Find out on which port this gpio-chip maps to inspecting the second
+ * argument of "gpio-ranges" property.
+ */
+ pinctrl_base = of_args.args[1];
+ gpioport = RZA1_PIN_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 = kasprintf(GFP_KERNEL, "%s-%d", np->name, gpioport);
+ chip->ngpio = of_args.args[2];
+ chip->of_node = np;
+ chip->parent = rza1_pctl->dev;
+
+ range->id = gpioport;
+ range->name = kasprintf(GFP_KERNEL, "%s-%d", np->name, gpioport);
+ 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;
+
+ dev_info(rza1_pctl->dev, "Parsed gpiochip %s with %d pins\n",
+ chip->label, chip->ngpio);
+
+ return 0;
+}
+
+/**
+ * rza1_parse_dt() - parse DT to collect gpiochips and pmx functions
+ *
+ * @rza1_pctl: RZ/A1 pin controller device
+ */
+static int rza1_parse_dt(struct rza1_pinctrl *rza1_pctl)
+{
+ int ret;
+ unsigned int npmxfuncs;
+ unsigned int ngpiochips;
+ unsigned int i;
+ struct gpio_chip *gpio_chips;
+ struct pinctrl_gpio_range *gpio_ranges;
+ struct device_node *np = rza1_pctl->dev->of_node;
+ struct device_node *child;
+
+ ngpiochips = rza1_count_gpio_chips(np);
+ if (ngpiochips) {
+ dev_info(rza1_pctl->dev, "Registering %u gpio chips\n",
+ ngpiochips);
+
+ 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;
+ } else {
+ dev_dbg(rza1_pctl->dev, "No gpiochip registered\n");
+ gpio_ranges = NULL;
+ }
+
+ rza1_pctl->gpio_ranges = gpio_ranges;
+
+ i = 0;
+ npmxfuncs = 0;
+ for_each_child_of_node(np, child) {
+ if (of_property_read_bool(child, "gpio-controller")) {
+ /* Never get here if ngpiochips == 0 */
+ ret = rza1_parse_gpiochip(rza1_pctl, child,
+ &gpio_chips[i],
+ &gpio_ranges[i]);
+ if (ret)
+ goto gpio_pmx_unregister;
+
+ ++i;
+ } else {
+ ret = rza1_parse_pmx_function(rza1_pctl, child);
+ if (ret)
+ goto gpio_pmx_unregister;
+
+ ++npmxfuncs;
+ }
+ }
+
+ rza1_pctl->ngpiochips = i;
+
+ dev_info(rza1_pctl->dev,
+ "Registered %u gpio controllers and %u pin mux functions\n",
+ rza1_pctl->ngpiochips, npmxfuncs);
+
+ return 0;
+
+gpio_pmx_unregister:
+ for (; i > 0; i--)
+ devm_gpiochip_remove(rza1_pctl->dev, &gpio_chips[i - 1]);
+
+ rza1_remove_pmx_functions(rza1_pctl);
+
+ return ret;
+}
+
+/**
+ * rza1_pinctrl_register() - Enumerate pins, ports, gpiochips and functions and
+ * register to pinctrl and gpio cores
+ *
+ * @rza1_pctl: RZ/A1 pin controller device
+ */
+static int rza1_pinctrl_register(struct rza1_pinctrl *rza1_pctl)
+{
+ int ret;
+ unsigned int i;
+ struct rza1_port *ports;
+ struct pinctrl_pin_desc *pins;
+
+ 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 port = RZA1_PIN_TO_PORT(i);
+ unsigned int offset = RZA1_PIN_TO_OFFSET(i);
+
+ pins[i].number = i;
+ pins[i].name = kasprintf(GFP_KERNEL, "P%u-%u", port, offset);
+
+ if (i % RZA1_PINS_PER_PORT == 0) {
+ /*
+ * Setup ports;
+ * they provide per-port lock and logical base address.
+ */
+ unsigned int port_id = RZA1_PIN_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 = rza1_parse_dt(rza1_pctl);
+ if (ret) {
+ dev_err(rza1_pctl->dev, "RZ/A1 DT parsing failed\n");
+ return ret;
+ }
+
+ for (i = 0; i < rza1_pctl->ngpiochips; i++)
+ pinctrl_add_gpio_range(rza1_pctl->pctl,
+ &rza1_pctl->gpio_ranges[i]);
+
+ return 0;
+}
+
+static int rza1_pinctrl_probe(struct platform_device *pdev)
+{
+ int ret;
+ struct resource *res;
+ struct rza1_pinctrl *rza1_pctl;
+
+ 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-03-20 16:51:06

[permalink] [raw]

Subject: [PATCH v2 3/7] arm: dts: dt-bindings: Add Renesas RZ 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 | 36 ++++++++++++++++++++++++++
1 file changed, 36 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..98852d3
--- /dev/null
+++ b/include/dt-bindings/pinctrl/r7s72100-pinctrl.h
@@ -0,0 +1,36 @@
+/*
+ * 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 it's bank and position numbers */
+#define PIN(b, p) ((b) * RZA1_PINS_PER_PORT + (p))
+
+/*
+ * Flags to apply to alternate function configuration
+ * All of the following are mutually exclusive.
+ */
+
+/*
+ * Pin is bi-directional.
+ * Alternate function that need both input/outpu functionalities shall
+ * be configured as bidirectional.
+ * Eg. SDA/SCL pins of an I2c interface.
+ */
+#define BI_DIR (1 << 3)
+
+/*
+ * Flags used to ask software drive the pin I/O direction overriding the
+ * alternate function configuration.
+ * Some alternate function requires software to force I/O direction of a pin,
+ * ovverriding the designated one.
+ * Reference to the HW manual to know when this flag shall be used.
+ */
+#define SWIO_IN (1 << 4)
+#define SWIO_OUT (1 << 5)
+
+#endif
--
2.7.4

2017-03-20 16:50:58

[permalink] [raw]

Subject: [PATCH v2 5/7] arm: dts: genmai: Add SCIF2 pin group

Add pin configuration subnode for SCIF2 serial debug interface.

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

diff --git a/arch/arm/boot/dts/r7s72100-genmai.dts b/arch/arm/boot/dts/r7s72100-genmai.dts
index 118a8e2..569c86a 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,15 @@
};
};

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

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

--
2.7.4

2017-03-20 17:24:22

[permalink] [raw]

Subject: [PATCH v2 2/7] 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 | 144 +++++++++++++++++++++
1 file changed, 144 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..0474860
--- /dev/null
+++ b/Documentation/devicetree/bindings/pinctrl/renesas,rza1-pinctrl.txt
@@ -0,0 +1,144 @@
+Renesas RZ/A1 combined Pin and GPIO controller
+
+Renesas SoCs of RZ/A1 family feature a combined Pin and GPIO controller
+hardware 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".
+
+ - #pinctrl-cells
+ as defined by pinctrl-bindings.txt, this is the number
+ of cells (in addition to pin index) required to configure a single pin.
+ Shall be set to 1.
+
+ - reg
+ address base and length of the memory area where pin controller
+ hardware is mapped to.
+
+Example:
+Pin controller node for RZ/A1H SoC (r7s72100)
+
+pinctrl: pinctrl@fcfe3000 {
+ compatible = "renesas,r7s72100-ports";
+
+ #pinctrl-cells = <1>;
+
+ reg = <0xfcfe3000 0x4248>;
+};
+
+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 groups.
+
+ Required properties:
+ - renesas,pins
+ describes an array of pin multiplexing configurations.
+ 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 description along with it.
+ When multiple pins are required to be configured as part of the same
+ alternate function (odds are single-pin alternate functions exist) they
+ shall be specified as members of the same argument list of a single
+ "renesas-pins" property.
+ Helper macros to ease calculating the pin index from its position
+ (port where it sits on and pin offset), and alternate function
+ configuration options are provided in pin controller header file at:
+ include/dt-bindings/pinctrl/r7s72100-pinctrl.h
+
+ Example:
+ A serial communication interface with a TX output pin and a RX input pin.
+
+ &pinctrl {
+ scif2_pins: serial2 {
+ renesas,pins = <PIN(3, 0) 6>,
+ <PIN(3, 2) 4>;
+ };
+ }
+
+ Pin #0 on port #3 is configured in alternate function #6.
+ Pin #2 on port #3 is configured in alternate function #4.
+
+ Example 2:
+ I2c master: both SDA and SCL pins need bi-directional operations
+
+ &pinctrl {
+ i2c2_pins: i2c2 {
+ renesas,pins = <PIN(1, 4) (1 | BI_DIR)>,
+ <PIN(1, 5) (1 | BI_DIR)>;
+ };
+ }
+
+ Pin #4 on port #1 is configured in alternate function #1.
+ Pin #5 on port #1 is configured in alternate function #1.
+ Both need to work in bi-directional mode.
+
+ Example 3:
+ Multi-function timer input and output compare pins.
+ The hardware manual prescribes this pins to have input/output direction
+ specified by software. Configure TIOC0A as input and TIOC0B as output.
+
+ &pinctrl {
+ tioc0_pins: tioc0 {
+ renesas,pins = <PIN(4, 0) (2 | SWIO_IN)>,
+ <PIN(4, 1) (2 | SWIO_OUT)>;
+ };
+ }
+
+ Pin #0 on port #4 is configured in alternate function #2 with IO direction
+ specified by software as input.
+ Pin #1 on port #4 is configured in alternate function #1 with IO direction
+ specified by software as output.
+
+- GPIO controller sub-nodes:
+ Each port of r7s72100 pin controller hardware is itself a gpio controller.
+ Different SoCs have different number 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 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 gpio bindings documentation. Refer to this 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-03-22 10:27:09

[permalink] [raw]

Subject: Re: [PATCH v2 1/7] pinctrl: Renesas RZ/A1 pin and gpio controller

Hi Jacopo,

On Mon, Mar 20, 2017 at 5:14 PM, 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]>

Thanks for your patch!

> --- /dev/null
> +++ b/drivers/pinctrl/pinctrl-rza1.c

> +/*
> + * 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 (r7s721001) and RZ/A1L (r7s721002) are.

RZ/A1M = r7s721010, RZ/A1L = r7s721020

> +#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_TO_PORT(pin) ((pin) / RZA1_PINS_PER_PORT)
> +#define RZA1_PIN_TO_OFFSET(pin) ((pin) % RZA1_PINS_PER_PORT)
> +
> +/*
> + * Be careful here: the pin configuration subnodes in device tree enumerates

enumerate

> + * alternate functions from 1 to 8; subtract 1 before using macros so to match
> + * registers configuration which expects numbers from 0 to 7 instead.

register configuration

> + */
> +#define MUX_FUNC_OFFS 3
> +#define MUX_FUNC_MASK (BIT(MUX_FUNC_OFFS) - 1)
> +#define MUX_FUNC_PFC_MASK BIT(0)
> +#define MUX_FUNC_PFCE_MASK BIT(1)
> +#define MUX_FUNC_PFCEA_MASK BIT(2)
> +#define MUX_CONF_BIDIR BIT(0)
> +#define MUX_CONF_SWIO_INPUT BIT(1)
> +#define MUX_CONF_SWIO_OUTPUT BIT(2)
> +
> +/**
> + * rza1_pin_conf - describes a pin position, id, mux config and output value
> + *
> + * Use uint32_t to match types used in of_device nodes argument lists.
> + *
> + * @id: the pin identifier from 0 to RZA1_NPINS
> + * @port: the port where pin sits on
> + * @offset: pin offset in the port
> + * @mux: alternate function configuration settings
> + * @value: output value to set the pin to
> + */
> +struct rza1_pin_conf {
> + uint32_t id;
> + uint32_t port;
> + uint32_t offset;
> + uint32_t mux_conf;
> + uint32_t value;

In the kernel, we tend to use u32 instead of uint32_t.
But many of these fields can be smaller than 32 bits, right, saving some
memory (important when running with built-in RAM only).

> +/**
> + * 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
> + * @ngpiochips: number of gpio chips
> + * @gpio_ranges: gpio ranges for pinctrl core
> + * @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;
> +
> + unsigned int ngpiochips;
> +
> + struct pinctrl_gpio_range *gpio_ranges;
> + struct pinctrl_pin_desc *pins;
> + struct pinctrl_desc desc;
> + struct pinctrl_dev *pctl;

It's a good idea not to mix pointers and integers, as the pointers will
be 64-bit on 64-bit platforms, leading to gaps due to alignment rules.
Not super-important here, as (for now) this driver is meant for 32-bit SoCs.

> +/**
> + * rza1_set_bit() - un-locked set/clear a single bit in pin configuration
> + * registers
> + */
> +static inline void rza1_set_bit(const struct rza1_port *port,
> + unsigned int reg, unsigned int offset,
> + bool set)

I think "reg" and "set" still fit on the previous lines (many more cases
in other functions).

I'd call "offset" "bit" (and "reg" "offset"?)

> +{
> + void __iomem *mem = RZA1_ADDR(port->base, reg, port->id);

I think this would be easier to read without using the RZA1_ADDR() macro.

> + u16 val = ioread16(mem);
> +
> + if (set)
> + val |= BIT(offset);
> + else
> + val &= ~BIT(offset);
> +
> + iowrite16(val, mem);
> +}
> +
> +static inline int rza1_get_bit(struct rza1_port *port,
> + unsigned int reg, unsigned int offset)
> +{
> + void __iomem *mem = RZA1_ADDR(port->base, reg, port->id);
> +
> + return ioread16(mem) & BIT(offset);

Same comments as for rza1_set_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
> + * @offset: pin offset
> + */
> +static void rza1_pin_reset(struct rza1_port *port,
> + unsigned int offset)

The above fits on a single line.

"pin" instead of "offset"?

> +{
> + spin_lock(&port->lock);

spin_lock_irqsave()? (everywhere)

> + rza1_set_bit(port, PIBC_REG, offset, 0);
> + rza1_set_bit(port, PBDC_REG, offset, 0);
> +
> + rza1_set_bit(port, PM_REG, offset, 1);
> + rza1_set_bit(port, PMC_REG, offset, 0);
> + rza1_set_bit(port, PIPC_REG, offset, 0);
> + spin_unlock(&port->lock);

spin_unlock_irqrestore()? (everywhere)

> +}
> +
> +static inline int rza1_pin_get_direction(struct rza1_port *port,
> + int offset)

The above fits on a single line.

"pin" instead of "offset" (many more below)?

> +/**
> + * rza1_pin_conf_validate() - make sure a single bit it set in mux_conf mask

is set

> + */
> +static inline int rza1_pin_conf_validate(u8 mux_conf)
> +{
> + do {
> + if (mux_conf & BIT(0))
> + break;
> + } while ((mux_conf >>= 1));
> +
> + return (mux_conf >> 1);

Please study <linux/bitops.h> to find a better way to do this ;-)

> +/**
> + * rza1_gpio_get() - read a gpio pin value
> + *
> + * Read gpio pin value through PPR register.
> + * Requires bi-directional mode to work when reading value of a pin

the value

> + * in output mode

> +/**
> + * rza1_pinmux_set() - retrieve pins from a group and apply them mux settings

their

> + *
> + * @pctldev: pin controller device
> + * @selector: function selector
> + * @group: group selector
> + */
> +static int rza1_pinmux_set(struct pinctrl_dev *pctldev, unsigned int selector,
> + unsigned int group)
> +{
> + int i;
> + struct group_desc *grp;
> + struct function_desc *func;
> + struct rza1_pin_conf *pin_confs;
> + struct rza1_pinctrl *rza1_pctl = pinctrl_dev_get_drvdata(pctldev);

Reverse Christmas tree ordering (longest line first)?

> +/**
> + * rza1_parse_pmx_function() - parse and register a pin mux function
> + *
> + * Pins for RZ SoC pin controller described by "renesas-pins" property.
> + *
> + * First argument in the list identifies the pin, while the second one
> + * describes the requested alternate function number and additional
> + * configuration parameter to be applied to the selected function.
> + *
> + * @rza1_pctl: RZ/A1 pin controller device
> + * @np: of pmx sub-node
> + */
> +static int rza1_parse_pmx_function(struct rza1_pinctrl *rza1_pctl,
> + struct device_node *np)
> +{
> + int ret;
> + int of_pins;

npins?

> + unsigned int i;
> + unsigned int *grpins;
> + const char *grpname;
> + const char **fngrps;
> + struct rza1_pin_conf *pin_confs;
> + struct pinctrl_dev *pctldev = rza1_pctl->pctl;
> + char const *prop_name = "renesas,pins";

const char *prop_name

Reverse Christmas tree ordering (longest line first)? (more below)

> +
> + of_pins = pinctrl_count_index_with_args(np, prop_name);
> + if (of_pins <= 0) {
> + dev_err(rza1_pctl->dev, "Missing %s property\n", prop_name);
> + return -ENOENT;
> + }
> +
> + /*
> + * Functions are made of 1 group only;
> + * in facts, functions and groups are identical for this pin controller

in fact

> + * except that functions carry an array of per-pin configurations

configuration

> + * settings.
> + */
> + pin_confs = devm_kcalloc(rza1_pctl->dev, of_pins, sizeof(*pin_confs),
> + GFP_KERNEL);
> + grpins = devm_kcalloc(rza1_pctl->dev, of_pins, sizeof(*grpins),
> + GFP_KERNEL);
> + fngrps = devm_kzalloc(rza1_pctl->dev, sizeof(*fngrps), GFP_KERNEL);
> +
> + if (!pin_confs || !grpins || !fngrps)
> + return -ENOMEM;
> +
> + /* Collect pin positions and mux settings to store them in function */
> + for (i = 0; i < of_pins; ++i) {
> + struct rza1_pin_conf *pin_conf = &pin_confs[i];
> + struct of_phandle_args of_pins_args;
> +
> + ret = pinctrl_parse_index_with_args(np, prop_name, i,
> + &of_pins_args);
> + if (ret)
> + return ret;
> +
> + if (of_pins_args.args_count < RZA1_PINMUX_OF_ARGS) {
> + dev_err(rza1_pctl->dev,
> + "Wrong arguments number for %s property\n",

number of arguments

> +/**
> + * 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.
> + * Gpio chips and pin ranges are here collected, but ranges are registered
> + * later, after pin controller has been registered too. Only gpiochips are

after the pin controller

> + * registered here.
> + *
> + * @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)
> +{
> + int ret;
> + u32 pinctrl_base;
> + unsigned int gpioport;
> + struct of_phandle_args of_args;
> + const char *list_name = "gpio-ranges";
> +
> + of_parse_phandle_with_fixed_args(np, list_name, 3, 0, &of_args);

This function can fail.

> +
> + /*
> + * Find out on which port this gpio-chip maps to inspecting the second

by inspecting

> + * argument of "gpio-ranges" property.

the "gpio-ranges" property.

> + */
> + pinctrl_base = of_args.args[1];
> + gpioport = RZA1_PIN_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 = kasprintf(GFP_KERNEL, "%s-%d", np->name, gpioport);

devm_kasprintf()?

"%s-%u"

> + chip->ngpio = of_args.args[2];
> + chip->of_node = np;
> + chip->parent = rza1_pctl->dev;
> +
> + range->id = gpioport;
> + range->name = kasprintf(GFP_KERNEL, "%s-%d", np->name, gpioport);

Reuse chip->label?

> +/**
> + * rza1_pinctrl_register() - Enumerate pins, ports, gpiochips and functions and
> + * register to pinctrl and gpio cores
> + *
> + * @rza1_pctl: RZ/A1 pin controller device
> + */
> +static int rza1_pinctrl_register(struct rza1_pinctrl *rza1_pctl)
> +{

> + for (i = 0; i < RZA1_NPINS; ++i) {
> + unsigned int port = RZA1_PIN_TO_PORT(i);
> + unsigned int offset = RZA1_PIN_TO_OFFSET(i);
> +
> + pins[i].number = i;
> + pins[i].name = kasprintf(GFP_KERNEL, "P%u-%u", port, offset);

devm_kasprintf()

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-03-22 10:34:30

[permalink] [raw]

Subject: Re: [PATCH v2 2/7] dt-bindings: pinctrl: Add RZ/A1 bindings doc

Hi Jacopo,

On Mon, Mar 20, 2017 at 5:14 PM, Jacopo Mondi <[email protected]> wrote:
> Add device tree bindings documentation for Renesas RZ/A1 gpio and pin

for the Renesas ...

> controller.
>
> Signed-off-by: Jacopo Mondi <[email protected]>
> ---
> .../bindings/pinctrl/renesas,rza1-pinctrl.txt | 144 +++++++++++++++++++++
> 1 file changed, 144 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..0474860
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pinctrl/renesas,rza1-pinctrl.txt
> @@ -0,0 +1,144 @@
> +Renesas RZ/A1 combined Pin and GPIO controller
> +
> +Renesas SoCs of RZ/A1 family feature a combined Pin and GPIO controller

the RZ/A1 family

> +hardware controller, named "Ports" in the hardware reference manual.

bogus "hardware controller"

> +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 groups.
> +
> + Required properties:
> + - renesas,pins

Just "pins"?

> + describes an array of pin multiplexing configurations.
> + 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 description along with it.
> + When multiple pins are required to be configured as part of the same
> + alternate function (odds are single-pin alternate functions exist) they
> + shall be specified as members of the same argument list of a single
> + "renesas-pins" property.
> + Helper macros to ease calculating the pin index from its position
> + (port where it sits on and pin offset), and alternate function
> + configuration options are provided in pin controller header file at:

the pin ... file

> + include/dt-bindings/pinctrl/r7s72100-pinctrl.h

Hence I'd include that file in this patch, as it's part of the bindings.

> + Example:
> + A serial communication interface with a TX output pin and a RX input pin.

an RX

> +
> + &pinctrl {
> + scif2_pins: serial2 {
> + renesas,pins = <PIN(3, 0) 6>,
> + <PIN(3, 2) 4>;

Single line?

> + };
> + }
> +
> + Pin #0 on port #3 is configured in alternate function #6.
> + Pin #2 on port #3 is configured in alternate function #4.

as alternate function

> +
> + Example 2:
> + I2c master: both SDA and SCL pins need bi-directional operations
> +
> + &pinctrl {
> + i2c2_pins: i2c2 {
> + renesas,pins = <PIN(1, 4) (1 | BI_DIR)>,
> + <PIN(1, 5) (1 | BI_DIR)>;
> + };
> + }
> +
> + Pin #4 on port #1 is configured in alternate function #1.
> + Pin #5 on port #1 is configured in alternate function #1.

as alternate function

> + Both need to work in bi-directional mode.
> +
> + Example 3:
> + Multi-function timer input and output compare pins.
> + The hardware manual prescribes this pins to have input/output direction
> + specified by software. Configure TIOC0A as input and TIOC0B as output.
> +
> + &pinctrl {
> + tioc0_pins: tioc0 {
> + renesas,pins = <PIN(4, 0) (2 | SWIO_IN)>,
> + <PIN(4, 1) (2 | SWIO_OUT)>;
> + };
> + }
> +
> + Pin #0 on port #4 is configured in alternate function #2 with IO direction
> + specified by software as input.
> + Pin #1 on port #4 is configured in alternate function #1 with IO direction
> + specified by software as output.

as alternate function

> +- GPIO controller sub-nodes:
> + Each port of r7s72100 pin controller hardware is itself a gpio controller.

the r7s72100 pin controller hardware

> + Different SoCs have different number of available pins per port, but

numbers of

> + 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 gpio bindings documentation.

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 gpio bindings documentation. Refer to this file

the gpio bindings documentation

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-03-22 10:37:46

[permalink] [raw]

Subject: Re: [PATCH v2 3/7] arm: dts: dt-bindings: Add Renesas RZ pinctrl header

On Mon, Mar 20, 2017 at 5:14 PM, Jacopo Mondi <[email protected]> wrote:
> Add dt-bindings for Renesas r7s72100 pin controller header file.
>
> Signed-off-by: Jacopo Mondi <[email protected]>
> ---
> include/dt-bindings/pinctrl/r7s72100-pinctrl.h | 36 ++++++++++++++++++++++++++
> 1 file changed, 36 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..98852d3
> --- /dev/null
> +++ b/include/dt-bindings/pinctrl/r7s72100-pinctrl.h
> @@ -0,0 +1,36 @@
> +/*
> + * 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 it's bank and position numbers */

its bank

> +#define PIN(b, p) ((b) * RZA1_PINS_PER_PORT + (p))
> +
> +/*
> + * Flags to apply to alternate function configuration
> + * All of the following are mutually exclusive.
> + */
> +
> +/*
> + * Pin is bi-directional.
> + * Alternate function that need both input/outpu functionalities shall

An alternate function that needs ... output

> + * be configured as bidirectional.
> + * Eg. SDA/SCL pins of an I2c interface.
> + */
> +#define BI_DIR (1 << 3)
> +
> +/*
> + * Flags used to ask software drive the pin I/O direction overriding the

to drive

> + * alternate function configuration.
> + * Some alternate function requires software to force I/O direction of a pin,

functions require

> + * ovverriding the designated one.

overriding

> + * Reference to the HW manual to know when this flag shall be used.

Refer

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-03-22 13:12:16

[permalink] [raw]

Subject: Re: [PATCH v2 4/7] arm: dts: r7s72100: Add pin controller node

Hi Jacopo,

On Mon, Mar 20, 2017 at 5:14 PM, Jacopo Mondi <[email protected]> wrote:
> 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]>

> --- a/arch/arm/boot/dts/r7s72100.dtsi
> +++ b/arch/arm/boot/dts/r7s72100.dtsi
> @@ -183,6 +183,86 @@
> };
> };
>
> + pinctrl: pinctrl@fcfe3000 {
> + compatible = "renesas,r7s72100-ports";
> +
> + #pinctrl-cells = <1>;
> +
> + reg = <0xfcfe3000 0x4248>;

How did you get to 0x4248? I had expected 0x4230.
Not that it matters much, Linux rounds it up to a multiple of PAGE_SIZE anyway.

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-03-22 13:13:26

[permalink] [raw]

Subject: Re: [PATCH v2 5/7] arm: dts: genmai: Add SCIF2 pin group

On Mon, Mar 20, 2017 at 5:14 PM, Jacopo Mondi <[email protected]> wrote:
> Add pin configuration subnode for SCIF2 serial debug interface.
>
> 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-03-22 13:17:08

[permalink] [raw]

Subject: Re: [PATCH v2 6/7] arm: dts: genmai: Add RIIC2 pin group

On Mon, Mar 20, 2017 at 5:14 PM, Jacopo Mondi <[email protected]> wrote:
> Add pin configuration subnode for RIIC2 interface.
>
> 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-03-22 13:21:26

[permalink] [raw]

Subject: Re: [PATCH v2 2/7] dt-bindings: pinctrl: Add RZ/A1 bindings doc

On Wed, Mar 22, 2017 at 11:33 AM, Geert Uytterhoeven
<[email protected]> wrote:
> On Mon, Mar 20, 2017 at 5:14 PM, Jacopo Mondi <[email protected]> wrote:
>> Add device tree bindings documentation for Renesas RZ/A1 gpio and pin
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pinctrl/renesas,rza1-pinctrl.txt

ulating the pin index from its position
>> + (port where it sits on and pin offset), and alternate function
>> + configuration options are provided in pin controller header file at:
>
> the pin ... file
>
>> + include/dt-bindings/pinctrl/r7s72100-pinctrl.h
>
> Hence I'd include that file in this patch, as it's part of the bindings.

Ah, that would create a hard dependency between the DTS files and the
DT bindings, which typically go in through different trees.
As the driver replicates the definitions from the header, the include file
can go in with the DTS updates.

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-03-22 13:23:45

[permalink] [raw]

Subject: Re: [PATCH v2 7/7] arm: dts: genmai: Add user led device nodes

On Mon, Mar 20, 2017 at 5:14 PM, Jacopo Mondi <[email protected]> wrote:
> Add device nodes for user leds on Genmai board.
>
> 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-03-22 15:36:46

[permalink] [raw]

Subject: Re: [PATCH v2 2/7] dt-bindings: pinctrl: Add RZ/A1 bindings doc

Hi Geert,
thanks for reviews

On Wed, Mar 22, 2017 at 02:20:08PM +0100, Geert Uytterhoeven wrote:
> On Wed, Mar 22, 2017 at 11:33 AM, Geert Uytterhoeven
> <[email protected]> wrote:
> > On Mon, Mar 20, 2017 at 5:14 PM, Jacopo Mondi <[email protected]> wrote:
> >> Add device tree bindings documentation for Renesas RZ/A1 gpio and pin
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/pinctrl/renesas,rza1-pinctrl.txt
>
> ulating the pin index from its position
> >> + (port where it sits on and pin offset), and alternate function
> >> + configuration options are provided in pin controller header file at:
> >
> > the pin ... file
> >
> >> + include/dt-bindings/pinctrl/r7s72100-pinctrl.h
> >
> > Hence I'd include that file in this patch, as it's part of the bindings.
>
> Ah, that would create a hard dependency between the DTS files and the
> DT bindings, which typically go in through different trees.
> As the driver replicates the definitions from the header, the include file
> can go in with the DTS updates.
>

Sorry, got confused by multiple reviews here..
Are you suggesting to squash [03/07] in [04/07] here?

Thanks
j

> 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-03-22 15:50:16

[permalink] [raw]

Subject: Re: [PATCH v2 2/7] dt-bindings: pinctrl: Add RZ/A1 bindings doc

Hi Jacopo,

On Wed, Mar 22, 2017 at 4:36 PM, jacopo <[email protected]> wrote:
> On Wed, Mar 22, 2017 at 02:20:08PM +0100, Geert Uytterhoeven wrote:
>> On Wed, Mar 22, 2017 at 11:33 AM, Geert Uytterhoeven
>> <[email protected]> wrote:
>> > On Mon, Mar 20, 2017 at 5:14 PM, Jacopo Mondi <[email protected]> wrote:
>> >> Add device tree bindings documentation for Renesas RZ/A1 gpio and pin
>> >> --- /dev/null
>> >> +++ b/Documentation/devicetree/bindings/pinctrl/renesas,rza1-pinctrl.txt
>>
>> ulating the pin index from its position
>> >> + (port where it sits on and pin offset), and alternate function
>> >> + configuration options are provided in pin controller header file at:
>> >
>> > the pin ... file
>> >
>> >> + include/dt-bindings/pinctrl/r7s72100-pinctrl.h
>> >
>> > Hence I'd include that file in this patch, as it's part of the bindings.
>>
>> Ah, that would create a hard dependency between the DTS files and the
>> DT bindings, which typically go in through different trees.
>> As the driver replicates the definitions from the header, the include file
>> can go in with the DTS updates.
>>
>
> Sorry, got confused by multiple reviews here..
> Are you suggesting to squash [03/07] in [04/07] here?

No, I suggested to squash [03/07] into [02/07].

But upon second thought, that's not such a good idea, as it creates an
additional dependency.

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-03-22 18:10:40

by Chris Brandt

[permalink] [raw]

Subject: RE: [PATCH v2 0/7] Renesas RZ/A1 pin and gpio controller

Hi Jacopo,

On Monday, March 20, 2017, Jacopo Mondi wrote:
> Chris: it would be great if you could give this another spin on RSK board.

I tested these patches on an RZ/A1H RSK board after modifying the DT for
the RSK vs the GENMAI board.

The following worked fine:
* SCIF
* I2C
* SDHI
* Ethernet

I see Geert has already responded with mostly text (grammar) changes.
As for the "API" of the driver from a user perspective, I think it should
cover all the use cases of the peripherals.

For your reference, here was my pin config for the RSK board testing. Once
this driver is (hopefully) accepted, I will update the upstream rskrza1 DT.

--- RSK BOARD PIN SETUP ---

&pinctrl {

scif2_pins: serial2 {
/* P3_0 as TxD2; P3_2 as RxD2 */
renesas,pins = <PIN(3, 0) 6>,
<PIN(3, 2) 4>;
};

/* RIIC Ch 3 */
i2c3_pins: i2c3 {
/* RIIC3: P1_6 as SCL, P1_7 as SDA */
renesas,pins = <PIN(1, 6) (1 | BI_DIR)>,
<PIN(1, 7) (1 | BI_DIR)>;
};

/* SHDI ch1 on CN1 */
sdhi1_pins: sdhi1 {
/* SHDI ch1 on Port 3 */
renesas,pins = <PIN(3, 8) (7)>, /* SDHI1 CD */
<PIN(3, 9) (7)>, /* SDHI1 WP */
<PIN(3, 10) (7 | BI_DIR)>, /* SDHI1 DAT1 */
<PIN(3, 11) (7 | BI_DIR)>, /* SDHI1 DAT0 */
<PIN(3, 12) (7)>, /* SDHI1 CLK */
<PIN(3, 13) (7 | BI_DIR)>, /* SDHI1 CMD */
<PIN(3, 14) (7 | BI_DIR)>, /* SDHI1 DAT3 */
<PIN(3, 15) (7 | BI_DIR)>; /* SDHI1 DAT2 */
};

/* Ethernet */
ether_pins: ether {
/* Ethernet on Ports 1,2,3,5 */
renesas,pins = <PIN(1, 14) (4)>, /* P1_14 = ET_COL */
<PIN(5, 9) (2)>, /* P5_9 = ET_MDC */
<PIN(3, 3) (2 | BI_DIR)>, /* P3_3 = ET_MDIO (bi dir) */
<PIN(3, 4) (2)>, /* P3_4 = ET_RXCLK */
<PIN(3, 5) (2)>, /* P3_5 = ET_RXER */
<PIN(3, 6) (2)>, /* P3_6 = ET_RXDV */
<PIN(2, 0) (2)>, /* P2_0 = ET_TXCLK */
<PIN(2, 1) (2)>, /* P2_1 = ET_TXER */
<PIN(2, 2) (2)>, /* P2_2 = ET_TXEN */
<PIN(2, 3) (2)>, /* P2_3 = ET_CRS */
<PIN(2, 4) (2)>, /* P2_4 = ET_TXD0 */
<PIN(2, 5) (2)>, /* P2_5 = ET_TXD1 */
<PIN(2, 6) (2)>, /* P2_6 = ET_TXD2 */
<PIN(2, 7) (2)>, /* P2_7 = ET_TXD3 */
<PIN(2, 8) (2)>, /* P2_8 = ET_RXD0 */
<PIN(2, 9) (2)>, /* P2_9 = ET_RXD1 */
<PIN(2, 10) (2)>, /* P2_10 = ET_RXD2 */
<PIN(2, 11) (2)>; /* P2_11 = ET_RXD3 */
};
};

&i2c3 {
pinctrl-names = "default";
pinctrl-0 = <&i2c3_pins>;
};

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

&sdhi1 {
pinctrl-names = "default";
pinctrl-0 = <&sdhi1_pins>;
};

&ether {
pinctrl-names = "default";
pinctrl-0 = <&ether_pins>;
};

/ {
leds {
status = "okay";
compatible = "gpio-leds";

led1 {
gpios = <&port7 1 GPIO_ACTIVE_LOW>;
};
};
};

Thank you,
Chris

2017-03-23 14:20:14

[permalink] [raw]

Subject: Re: [PATCH v2 1/7] pinctrl: Renesas RZ/A1 pin and gpio controller

Hi Geert,
thanks for detailed review

On Wed, Mar 22, 2017 at 11:26:58AM +0100, Geert Uytterhoeven wrote:
> Hi Jacopo,
>
> On Mon, Mar 20, 2017 at 5:14 PM, 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]>
>
> Thanks for your patch!
>
> > --- /dev/null
> > +++ b/drivers/pinctrl/pinctrl-rza1.c
>
> > +/*
> > + * 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 (r7s721001) and RZ/A1L (r7s721002) are.
>
> RZ/A1M = r7s721010, RZ/A1L = r7s721020
>
> > +#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_TO_PORT(pin) ((pin) / RZA1_PINS_PER_PORT)
> > +#define RZA1_PIN_TO_OFFSET(pin) ((pin) % RZA1_PINS_PER_PORT)
> > +
> > +/*
> > + * Be careful here: the pin configuration subnodes in device tree enumerates
>
> enumerate
>
> > + * alternate functions from 1 to 8; subtract 1 before using macros so to match
> > + * registers configuration which expects numbers from 0 to 7 instead.
>
> register configuration
>
> > + */
> > +#define MUX_FUNC_OFFS 3
> > +#define MUX_FUNC_MASK (BIT(MUX_FUNC_OFFS) - 1)
> > +#define MUX_FUNC_PFC_MASK BIT(0)
> > +#define MUX_FUNC_PFCE_MASK BIT(1)
> > +#define MUX_FUNC_PFCEA_MASK BIT(2)
> > +#define MUX_CONF_BIDIR BIT(0)
> > +#define MUX_CONF_SWIO_INPUT BIT(1)
> > +#define MUX_CONF_SWIO_OUTPUT BIT(2)
> > +
> > +/**
> > + * rza1_pin_conf - describes a pin position, id, mux config and output value
> > + *
> > + * Use uint32_t to match types used in of_device nodes argument lists.
> > + *
> > + * @id: the pin identifier from 0 to RZA1_NPINS
> > + * @port: the port where pin sits on
> > + * @offset: pin offset in the port
> > + * @mux: alternate function configuration settings
> > + * @value: output value to set the pin to
> > + */
> > +struct rza1_pin_conf {
> > + uint32_t id;
> > + uint32_t port;
> > + uint32_t offset;
> > + uint32_t mux_conf;
> > + uint32_t value;
>
> In the kernel, we tend to use u32 instead of uint32_t.
> But many of these fields can be smaller than 32 bits, right, saving some
> memory (important when running with built-in RAM only).
>

I'll move these to u8 (and u16 where appropriate) and fix all the
above grammar comments

> > +/**
> > + * 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
> > + * @ngpiochips: number of gpio chips
> > + * @gpio_ranges: gpio ranges for pinctrl core
> > + * @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;
> > +
> > + unsigned int ngpiochips;
> > +
> > + struct pinctrl_gpio_range *gpio_ranges;
> > + struct pinctrl_pin_desc *pins;
> > + struct pinctrl_desc desc;
> > + struct pinctrl_dev *pctl;
>
> It's a good idea not to mix pointers and integers, as the pointers will
> be 64-bit on 64-bit platforms, leading to gaps due to alignment rules.
> Not super-important here, as (for now) this driver is meant for 32-bit SoCs.
>

I grouped this variables "logically", and even if I understand your
concerns, there will be a single "struct rza1_pinctrl" per driver
instance, so I hope we can tollerate some padding bytes in favour of more
clearness. Also, as you said, there are no 64-bits RZ platforms atm.

> > +/**
> > + * rza1_set_bit() - un-locked set/clear a single bit in pin configuration
> > + * registers
> > + */
> > +static inline void rza1_set_bit(const struct rza1_port *port,
> > + unsigned int reg, unsigned int offset,
> > + bool set)
>
> I think "reg" and "set" still fit on the previous lines (many more cases
> in other functions).
>
> I'd call "offset" "bit" (and "reg" "offset"?)

I'll s/offset/bit and s/offset/pin where appropriate

>
> > +{
> > + void __iomem *mem = RZA1_ADDR(port->base, reg, port->id);
>
> I think this would be easier to read without using the RZA1_ADDR() macro.
>

Don't know... I can change this if you prefer... there's nothing nasty
hidden behind this macro...

> > + u16 val = ioread16(mem);
> > +
> > + if (set)
> > + val |= BIT(offset);
> > + else
> > + val &= ~BIT(offset);
> > +
> > + iowrite16(val, mem);
> > +}
> > +
> > +static inline int rza1_get_bit(struct rza1_port *port,
> > + unsigned int reg, unsigned int offset)
> > +{
> > + void __iomem *mem = RZA1_ADDR(port->base, reg, port->id);
> > +
> > + return ioread16(mem) & BIT(offset);
>
> Same comments as for rza1_set_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
> > + * @offset: pin offset
> > + */
> > +static void rza1_pin_reset(struct rza1_port *port,
> > + unsigned int offset)
>
> The above fits on a single line.
>
> "pin" instead of "offset"?
>
> > +{
> > + spin_lock(&port->lock);
>
> spin_lock_irqsave()? (everywhere)
>

Quoting spinlock.txt:

----
If you have a case where you have to protect a data structure across
several CPU's and you want to use spinlocks you can potentially use
cheaper versions of the spinlocks. IFF you know that the spinlocks are
never used in interrupt handlers, you can use the non-irq versions:

spin_lock(&lock);
...
spin_unlock(&lock);
----

And there's nothing preventing an interrupt handling routine to
set/change a gpio value, so I'll change these to the _irqsave version, thanks.

> > + rza1_set_bit(port, PIBC_REG, offset, 0);
> > + rza1_set_bit(port, PBDC_REG, offset, 0);
> > +
> > + rza1_set_bit(port, PM_REG, offset, 1);
> > + rza1_set_bit(port, PMC_REG, offset, 0);
> > + rza1_set_bit(port, PIPC_REG, offset, 0);
> > + spin_unlock(&port->lock);
>
> spin_unlock_irqrestore()? (everywhere)
>
> > +}
> > +
> > +static inline int rza1_pin_get_direction(struct rza1_port *port,
> > + int offset)
>
> The above fits on a single line.
>
> "pin" instead of "offset" (many more below)?
>
> > +/**
> > + * rza1_pin_conf_validate() - make sure a single bit it set in mux_conf mask
>
> is set
>
> > + */
> > +static inline int rza1_pin_conf_validate(u8 mux_conf)
> > +{
> > + do {
> > + if (mux_conf & BIT(0))
> > + break;
> > + } while ((mux_conf >>= 1));
> > +
> > + return (mux_conf >> 1);
>
> Please study <linux/bitops.h> to find a better way to do this ;-)
>

Did my homework and I'm thinking of replacing this function with
something like:
if (ffs(mask) != fls(mask))) { }

Is this what you were suggesting?

> > +/**
> > + * rza1_gpio_get() - read a gpio pin value
> > + *
> > + * Read gpio pin value through PPR register.
> > + * Requires bi-directional mode to work when reading value of a pin
>
> the value
>
> > + * in output mode
>
>
> > +/**
> > + * rza1_pinmux_set() - retrieve pins from a group and apply them mux settings
>
> their
>
> > + *
> > + * @pctldev: pin controller device
> > + * @selector: function selector
> > + * @group: group selector
> > + */
> > +static int rza1_pinmux_set(struct pinctrl_dev *pctldev, unsigned int selector,
> > + unsigned int group)
> > +{
> > + int i;
> > + struct group_desc *grp;
> > + struct function_desc *func;
> > + struct rza1_pin_conf *pin_confs;
> > + struct rza1_pinctrl *rza1_pctl = pinctrl_dev_get_drvdata(pctldev);
>
> Reverse Christmas tree ordering (longest line first)?
>
>
>
> > +/**
> > + * rza1_parse_pmx_function() - parse and register a pin mux function
> > + *
> > + * Pins for RZ SoC pin controller described by "renesas-pins" property.
> > + *
> > + * First argument in the list identifies the pin, while the second one
> > + * describes the requested alternate function number and additional
> > + * configuration parameter to be applied to the selected function.
> > + *
> > + * @rza1_pctl: RZ/A1 pin controller device
> > + * @np: of pmx sub-node
> > + */
> > +static int rza1_parse_pmx_function(struct rza1_pinctrl *rza1_pctl,
> > + struct device_node *np)
> > +{
> > + int ret;
> > + int of_pins;
>
> npins?
>
> > + unsigned int i;
> > + unsigned int *grpins;
> > + const char *grpname;
> > + const char **fngrps;
> > + struct rza1_pin_conf *pin_confs;
> > + struct pinctrl_dev *pctldev = rza1_pctl->pctl;
> > + char const *prop_name = "renesas,pins";
>
> const char *prop_name
>
> Reverse Christmas tree ordering (longest line first)? (more below)

or const char const * to make sure also the content is never changed. But
that's an overkill maybe?

Thanks
j
>
> > +
> > + of_pins = pinctrl_count_index_with_args(np, prop_name);
> > + if (of_pins <= 0) {
> > + dev_err(rza1_pctl->dev, "Missing %s property\n", prop_name);
> > + return -ENOENT;
> > + }
> > +
> > + /*
> > + * Functions are made of 1 group only;
> > + * in facts, functions and groups are identical for this pin controller
>
> in fact
>
> > + * except that functions carry an array of per-pin configurations
>
> configuration
>
> > + * settings.
> > + */
> > + pin_confs = devm_kcalloc(rza1_pctl->dev, of_pins, sizeof(*pin_confs),
> > + GFP_KERNEL);
> > + grpins = devm_kcalloc(rza1_pctl->dev, of_pins, sizeof(*grpins),
> > + GFP_KERNEL);
> > + fngrps = devm_kzalloc(rza1_pctl->dev, sizeof(*fngrps), GFP_KERNEL);
> > +
> > + if (!pin_confs || !grpins || !fngrps)
> > + return -ENOMEM;
> > +
> > + /* Collect pin positions and mux settings to store them in function */
> > + for (i = 0; i < of_pins; ++i) {
> > + struct rza1_pin_conf *pin_conf = &pin_confs[i];
> > + struct of_phandle_args of_pins_args;
> > +
> > + ret = pinctrl_parse_index_with_args(np, prop_name, i,
> > + &of_pins_args);
> > + if (ret)
> > + return ret;
> > +
> > + if (of_pins_args.args_count < RZA1_PINMUX_OF_ARGS) {
> > + dev_err(rza1_pctl->dev,
> > + "Wrong arguments number for %s property\n",
>
> number of arguments
>
>
> > +/**
> > + * 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.
> > + * Gpio chips and pin ranges are here collected, but ranges are registered
> > + * later, after pin controller has been registered too. Only gpiochips are
>
> after the pin controller
>
> > + * registered here.
> > + *
> > + * @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)
> > +{
> > + int ret;
> > + u32 pinctrl_base;
> > + unsigned int gpioport;
> > + struct of_phandle_args of_args;
> > + const char *list_name = "gpio-ranges";
> > +
> > + of_parse_phandle_with_fixed_args(np, list_name, 3, 0, &of_args);
>
> This function can fail.
>
> > +
> > + /*
> > + * Find out on which port this gpio-chip maps to inspecting the second
>
> by inspecting
>
> > + * argument of "gpio-ranges" property.
>
> the "gpio-ranges" property.
>
> > + */
> > + pinctrl_base = of_args.args[1];
> > + gpioport = RZA1_PIN_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 = kasprintf(GFP_KERNEL, "%s-%d", np->name, gpioport);
>
> devm_kasprintf()?
>
> "%s-%u"
>
> > + chip->ngpio = of_args.args[2];
> > + chip->of_node = np;
> > + chip->parent = rza1_pctl->dev;
> > +
> > + range->id = gpioport;
> > + range->name = kasprintf(GFP_KERNEL, "%s-%d", np->name, gpioport);
>
> Reuse chip->label?
>
>
> > +/**
> > + * rza1_pinctrl_register() - Enumerate pins, ports, gpiochips and functions and
> > + * register to pinctrl and gpio cores
> > + *
> > + * @rza1_pctl: RZ/A1 pin controller device
> > + */
> > +static int rza1_pinctrl_register(struct rza1_pinctrl *rza1_pctl)
> > +{
>
> > + for (i = 0; i < RZA1_NPINS; ++i) {
> > + unsigned int port = RZA1_PIN_TO_PORT(i);
> > + unsigned int offset = RZA1_PIN_TO_OFFSET(i);
> > +
> > + pins[i].number = i;
> > + pins[i].name = kasprintf(GFP_KERNEL, "P%u-%u", port, offset);
>
> devm_kasprintf()
>
> 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-03-23 16:02:21

[permalink] [raw]

Subject: Re: [PATCH v2 2/7] dt-bindings: pinctrl: Add RZ/A1 bindings doc

Hi Geert,
thanks for review

On Wed, Mar 22, 2017 at 11:33:50AM +0100, Geert Uytterhoeven wrote:
> Hi Jacopo,
>
> On Mon, Mar 20, 2017 at 5:14 PM, Jacopo Mondi <[email protected]> wrote:
> > Add device tree bindings documentation for Renesas RZ/A1 gpio and pin
>
> for the Renesas ...
>
> > controller.
> >
> > Signed-off-by: Jacopo Mondi <[email protected]>
> > ---
> > .../bindings/pinctrl/renesas,rza1-pinctrl.txt | 144 +++++++++++++++++++++
> > 1 file changed, 144 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..0474860
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pinctrl/renesas,rza1-pinctrl.txt
> > @@ -0,0 +1,144 @@
> > +Renesas RZ/A1 combined Pin and GPIO controller
> > +
> > +Renesas SoCs of RZ/A1 family feature a combined Pin and GPIO controller
>
> the RZ/A1 family
>
> > +hardware controller, named "Ports" in the hardware reference manual.
>
> bogus "hardware controller"
>
> > +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 groups.
> > +
> > + Required properties:
> > + - renesas,pins
>
> Just "pins"?
>

You know, I've been thinking about this, bu the "pins" property
definition in pinctrl-bidings is the following one:

Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
---
- pins takes a list of pin names or IDs as a required argument. The
specific binding for the hardware defines:
- Whether the entries are integers or strings, and their
meaning.
---

And all examples there assume one "pin name" or "ID" per pin.

Now, we use 2 values per each pin (the pin ID and the alternate
function number), so to me this is different from what the generic
binding describes.
Also, pinctrl-single, and pinctrl-imx which have and ABI similar to
the one this driver define, use "pinctrl-single,pins" and "fsl,pins"
respectively as property names.
So either they have to be updated yet, or we should keep using
"renesas,pins" for our own defined ABI.

Maybe Linus or other pinctrl people can give some suggestion here.

Thanks
j

> > + describes an array of pin multiplexing configurations.
> > + 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 description along with it.
> > + When multiple pins are required to be configured as part of the same
> > + alternate function (odds are single-pin alternate functions exist) they
> > + shall be specified as members of the same argument list of a single
> > + "renesas-pins" property.
> > + Helper macros to ease calculating the pin index from its position
> > + (port where it sits on and pin offset), and alternate function
> > + configuration options are provided in pin controller header file at:
>
> the pin ... file
>
> > + include/dt-bindings/pinctrl/r7s72100-pinctrl.h
>
> Hence I'd include that file in this patch, as it's part of the bindings.
>
> > + Example:
> > + A serial communication interface with a TX output pin and a RX input pin.
>
> an RX
>
> > +
> > + &pinctrl {
> > + scif2_pins: serial2 {
> > + renesas,pins = <PIN(3, 0) 6>,
> > + <PIN(3, 2) 4>;
>
> Single line?
>
> > + };
> > + }
> > +
> > + Pin #0 on port #3 is configured in alternate function #6.
> > + Pin #2 on port #3 is configured in alternate function #4.
>
> as alternate function
>
> > +
> > + Example 2:
> > + I2c master: both SDA and SCL pins need bi-directional operations
> > +
> > + &pinctrl {
> > + i2c2_pins: i2c2 {
> > + renesas,pins = <PIN(1, 4) (1 | BI_DIR)>,
> > + <PIN(1, 5) (1 | BI_DIR)>;
> > + };
> > + }
> > +
> > + Pin #4 on port #1 is configured in alternate function #1.
> > + Pin #5 on port #1 is configured in alternate function #1.
>
> as alternate function
>
> > + Both need to work in bi-directional mode.
> > +
> > + Example 3:
> > + Multi-function timer input and output compare pins.
> > + The hardware manual prescribes this pins to have input/output direction
> > + specified by software. Configure TIOC0A as input and TIOC0B as output.
> > +
> > + &pinctrl {
> > + tioc0_pins: tioc0 {
> > + renesas,pins = <PIN(4, 0) (2 | SWIO_IN)>,
> > + <PIN(4, 1) (2 | SWIO_OUT)>;
> > + };
> > + }
> > +
> > + Pin #0 on port #4 is configured in alternate function #2 with IO direction
> > + specified by software as input.
> > + Pin #1 on port #4 is configured in alternate function #1 with IO direction
> > + specified by software as output.
>
> as alternate function
>
> > +- GPIO controller sub-nodes:
> > + Each port of r7s72100 pin controller hardware is itself a gpio controller.
>
> the r7s72100 pin controller hardware
>
> > + Different SoCs have different number of available pins per port, but
>
> numbers of
>
> > + 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 gpio bindings documentation.
>
> 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 gpio bindings documentation. Refer to this file
>
> the gpio bindings documentation
>
>
> 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-03-23 16:13:31

[permalink] [raw]

Subject: Re: [PATCH v2 4/7] arm: dts: r7s72100: Add pin controller node

Hi Geert,

On Wed, Mar 22, 2017 at 02:12:04PM +0100, Geert Uytterhoeven wrote:
> Hi Jacopo,
>
> On Mon, Mar 20, 2017 at 5:14 PM, Jacopo Mondi <[email protected]> wrote:
> > 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]>
>
> > --- a/arch/arm/boot/dts/r7s72100.dtsi
> > +++ b/arch/arm/boot/dts/r7s72100.dtsi
> > @@ -183,6 +183,86 @@
> > };
> > };
> >
> > + pinctrl: pinctrl@fcfe3000 {
> > + compatible = "renesas,r7s72100-ports";
> > +
> > + #pinctrl-cells = <1>;
> > +
> > + reg = <0xfcfe3000 0x4248>;
>
> How did you get to 0x4248? I had expected 0x4230.
> Not that it matters much, Linux rounds it up to a multiple of PAGE_SIZE anyway.
>

I guess I probably confused hex and dec values, as the correct
calculation here would have been (4200 + C * 4)
That 4248 value is what you get with (4200 + 12 * 4) so I probably
typed 12 in the calculator instead of C :)
I'll change this even if it does not make any difference..

> 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-03-28 09:46:36

[permalink] [raw]

Subject: Re: [PATCH v2 2/7] dt-bindings: pinctrl: Add RZ/A1 bindings doc

On Thu, Mar 23, 2017 at 5:02 PM, jacopo <[email protected]> wrote:

>> > + Required properties:
>> > + - renesas,pins
>>
>> Just "pins"?
>>
>
> You know, I've been thinking about this, bu the "pins" property
> definition in pinctrl-bidings is the following one:
>
> Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
> ---
> - pins takes a list of pin names or IDs as a required argument. The
> specific binding for the hardware defines:
> - Whether the entries are integers or strings, and their
> meaning.
> ---
>
> And all examples there assume one "pin name" or "ID" per pin.
>
> Now, we use 2 values per each pin (the pin ID and the alternate
> function number), so to me this is different from what the generic
> binding describes.
> Also, pinctrl-single, and pinctrl-imx which have and ABI similar to
> the one this driver define, use "pinctrl-single,pins" and "fsl,pins"
> respectively as property names.
> So either they have to be updated yet, or we should keep using
> "renesas,pins" for our own defined ABI.
>
> Maybe Linus or other pinctrl people can give some suggestion here.

To me as subsystem maintainer any "necessarily different" bindings
are just a big confusion for the head.

Since you're adding a new driver, try to stick to the generic bindings
even if it deviates from what you are used to for Renesas, because
even if it may be more work for you guys or make you annoyed that
now a certain Renesas is different from all other Renesas platforms,
for the community this makes things easier to maintain because
we can look at the driver and its bindings and say "ah I know this".

The fact that historically all the early adopters of pinctrl in device tree
have these funky custom bindings is unfortunate but just something
that we need to live with.

Yours,
Linus Walleij

2017-03-28 14:38:33

[permalink] [raw]

Subject: Re: [PATCH v2 2/7] dt-bindings: pinctrl: Add RZ/A1 bindings doc

Hi Linus,

On 2017-03-28 11:46, Linus Walleij wrote:
> On Thu, Mar 23, 2017 at 5:02 PM, jacopo <[email protected]> wrote:
>
>>> > + Required properties:
>>> > + - renesas,pins
>>>
>>> Just "pins"?
>>>
>>
>> You know, I've been thinking about this, bu the "pins" property
>> definition in pinctrl-bidings is the following one:
>>
>> Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
>> ---
>> - pins takes a list of pin names or IDs as a required argument. The
>> specific binding for the hardware defines:
>> - Whether the entries are integers or strings, and their
>> meaning.
>> ---
>>
>> And all examples there assume one "pin name" or "ID" per pin.
>>
>> Now, we use 2 values per each pin (the pin ID and the alternate
>> function number), so to me this is different from what the generic
>> binding describes.
>> Also, pinctrl-single, and pinctrl-imx which have and ABI similar to
>> the one this driver define, use "pinctrl-single,pins" and "fsl,pins"
>> respectively as property names.
>> So either they have to be updated yet, or we should keep using
>> "renesas,pins" for our own defined ABI.
>>
>> Maybe Linus or other pinctrl people can give some suggestion here.
>
> To me as subsystem maintainer any "necessarily different" bindings
> are just a big confusion for the head.
>

Understandable :)

> Since you're adding a new driver, try to stick to the generic bindings
> even if it deviates from what you are used to for Renesas, because
> even if it may be more work for you guys or make you annoyed that
> now a certain Renesas is different from all other Renesas platforms,
> for the community this makes things easier to maintain because
> we can look at the driver and its bindings and say "ah I know this".
>
> The fact that historically all the early adopters of pinctrl in device
> tree
> have these funky custom bindings is unfortunate but just something
> that we need to live with.
>

To avoid any confusion, please bear with me and clarify this once and
for all,
since I'm not certain I fully got you here.

Are you suggesting:

1) Use "pins" property with the currently implemented ABI (which
slightly differs
from the standard documented one as explained above. Not sure it is
fine overriding
it or not)

2) Use "pins" property and change our ABI to match the documented one:
one ID (integer
or string) per pin, not 2 as we're doing now.

Both solutions are easily implementable; 2) requires some more work to
make pin id, function number
and pin configuration fit in one single integer, but is achievable for
sure.

Thanks
j

> Yours,
> Linus Walleij

2017-03-29 02:30:48

[permalink] [raw]

Subject: Re: [PATCH v2 2/7] dt-bindings: pinctrl: Add RZ/A1 bindings doc

On Tue, Mar 28, 2017 at 4:38 PM, <[email protected]> wrote:

>> The fact that historically all the early adopters of pinctrl in device
>> tree
>> have these funky custom bindings is unfortunate but just something
>> that we need to live with.
>>
>
> To avoid any confusion, please bear with me and clarify this once and for
> all,
> since I'm not certain I fully got you here.
>
> Are you suggesting:
>
> 1) Use "pins" property with the currently implemented ABI (which slightly
> differs
> from the standard documented one as explained above. Not sure it is fine
> overriding
> it or not)

Correction: you should be using the property "pinmux", because you
are setting group and function at the same time.

See for example:
include/dt-bindings/pinctrl/mt65xx.h

And how that is used in:
arch/arm/boot/dts/mt2701-pinfunc.h
arch/arm/boot/dts/mt2701-evb.dts

The docs are here:
Documentation/devicetree/bindings/pinctrl/pinctrl-mt65xx.txt

I'm sorry that "pinmux" is not part of the generic documentation, it'd be
great if you would like to add it with a patch.

Yours,
Linus Walleij

2017-03-29 07:35:23

[permalink] [raw]

Subject: Re: [PATCH v2 2/7] dt-bindings: pinctrl: Add RZ/A1 bindings doc

Hi Linus,

On Wed, Mar 29, 2017 at 4:30 AM, Linus Walleij <[email protected]> wrote:
> On Tue, Mar 28, 2017 at 4:38 PM, <[email protected]> wrote:
>>> The fact that historically all the early adopters of pinctrl in device
>>> tree
>>> have these funky custom bindings is unfortunate but just something
>>> that we need to live with.
>>
>> To avoid any confusion, please bear with me and clarify this once and for
>> all,
>> since I'm not certain I fully got you here.
>>
>> Are you suggesting:
>>
>> 1) Use "pins" property with the currently implemented ABI (which slightly
>> differs
>> from the standard documented one as explained above. Not sure it is fine
>> overriding
>> it or not)
>
> Correction: you should be using the property "pinmux", because you
> are setting group and function at the same time.

OK.

> See for example:
> include/dt-bindings/pinctrl/mt65xx.h
>
> And how that is used in:
> arch/arm/boot/dts/mt2701-pinfunc.h
> arch/arm/boot/dts/mt2701-evb.dts
>
> The docs are here:
> Documentation/devicetree/bindings/pinctrl/pinctrl-mt65xx.txt

All of the above pack the information for a pin into a single 32-bit integer.
Is that what you want us to use, too?
Currently we use two integers: 1) pin index, and 2) function/flags combo.

> I'm sorry that "pinmux" is not part of the generic documentation, it'd be
> great if you would like to add it with a patch.

That would mean pinmux could be an array of single values, or tuples.
Is that OK?

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-03-29 10:15:18

[permalink] [raw]

Subject: Re: [PATCH v2 2/7] dt-bindings: pinctrl: Add RZ/A1 bindings doc

On Wed, Mar 29, 2017 at 9:35 AM, Geert Uytterhoeven
<[email protected]> wrote:
> Me:

>> See for example:
>> include/dt-bindings/pinctrl/mt65xx.h
>>
>> And how that is used in:
>> arch/arm/boot/dts/mt2701-pinfunc.h
>> arch/arm/boot/dts/mt2701-evb.dts
>>
>> The docs are here:
>> Documentation/devicetree/bindings/pinctrl/pinctrl-mt65xx.txt
>
> All of the above pack the information for a pin into a single 32-bit integer.
> Is that what you want us to use, too?
> Currently we use two integers: 1) pin index, and 2) function/flags combo.

I don't really know what you need, sorry. But some kind of figure, yes.
I would say whatever makes sense. 16+16 bits makes sense in most
combinatorial spaces does it not? If you split 32 bits in 16 bits for
pin and 16 bits for function, do you have more than 2^16 pins or 2^16
functions?

If you really do we may need to go for u64 but ... really? Is there
a rational reason for that other than "we did it like this first"?

I do not understand the notion of "flags" here. I hope that is not referring
to pin config, because I expect that to use the standard pin config
bindings outside of the pinmux value which should just define the
pin+function combo:

node {
pinmux = <PIN_NUMBER_PINMUX>;
GENERIC_PINCONFIG;
};

Example from Mediatek:

i2c1_pins_a: i2c1@0 {
pins {
pinmux = <MT8135_PIN_195_SDA1__FUNC_SDA1>,
<MT8135_PIN_196_SCL1__FUNC_SCL1>;
bias-pull-up = <55>;
};
};

So this allows for a combine pin+function number but pull ups,
bias etc are not baked into the thing, they have to be added on
separately with the generic bindings, which is nice and very readable.

Yours,
Linus Walleij

2017-03-29 11:21:40

[permalink] [raw]

Subject: Re: [PATCH v2 2/7] dt-bindings: pinctrl: Add RZ/A1 bindings doc

Hi Linus,

On Wed, Mar 29, 2017 at 12:15 PM, Linus Walleij
<[email protected]> wrote:
> On Wed, Mar 29, 2017 at 9:35 AM, Geert Uytterhoeven
> <[email protected]> wrote:
>>> See for example:
>>> include/dt-bindings/pinctrl/mt65xx.h
>>>
>>> And how that is used in:
>>> arch/arm/boot/dts/mt2701-pinfunc.h
>>> arch/arm/boot/dts/mt2701-evb.dts
>>>
>>> The docs are here:
>>> Documentation/devicetree/bindings/pinctrl/pinctrl-mt65xx.txt
>>
>> All of the above pack the information for a pin into a single 32-bit integer.
>> Is that what you want us to use, too?
>> Currently we use two integers: 1) pin index, and 2) function/flags combo.
>
> I don't really know what you need, sorry. But some kind of figure, yes.
> I would say whatever makes sense. 16+16 bits makes sense in most
> combinatorial spaces does it not? If you split 32 bits in 16 bits for
> pin and 16 bits for function, do you have more than 2^16 pins or 2^16
> functions?
>
> If you really do we may need to go for u64 but ... really? Is there
> a rational reason for that other than "we did it like this first"?
>
> I do not understand the notion of "flags" here. I hope that is not referring

Flags refers to BI_DIR, SWIO_IN, and SWIO_OUT, from
https://patchwork.kernel.org/patch/9643047/

32-bit should be enough to cover pins, function, and flags.

> to pin config, because I expect that to use the standard pin config
> bindings outside of the pinmux value which should just define the
> pin+function combo:
>
> node {
> pinmux = <PIN_NUMBER_PINMUX>;
> GENERIC_PINCONFIG;
> };
>
> Example from Mediatek:
>
> i2c1_pins_a: i2c1@0 {
> pins {
> pinmux = <MT8135_PIN_195_SDA1__FUNC_SDA1>,
> <MT8135_PIN_196_SCL1__FUNC_SCL1>;

If we follow this example, then we can list all combinations in
include/dt-bindings/pinctrl/r7s72100-pinctrl.h, instead of creating the value
by combining the bits using a macro where we need it in the DTS.

It's gonna be a long list, though...

> bias-pull-up = <55>;
> };
> };
>
> So this allows for a combine pin+function number but pull ups,
> bias etc are not baked into the thing, they have to be added on
> separately with the generic bindings, which is nice and very readable.

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-03-29 12:06:39

[permalink] [raw]

Subject: Re: [PATCH v2 2/7] dt-bindings: pinctrl: Add RZ/A1 bindings doc

Hi Geert, Linus

On Wed, Mar 29, 2017 at 01:20:39PM +0200, Geert Uytterhoeven wrote:
> Hi Linus,
>
> On Wed, Mar 29, 2017 at 12:15 PM, Linus Walleij
> <[email protected]> wrote:
> > On Wed, Mar 29, 2017 at 9:35 AM, Geert Uytterhoeven
> > <[email protected]> wrote:
> >>> See for example:
> >>> include/dt-bindings/pinctrl/mt65xx.h
> >>>
> >>> And how that is used in:
> >>> arch/arm/boot/dts/mt2701-pinfunc.h
> >>> arch/arm/boot/dts/mt2701-evb.dts
> >>>
> >>> The docs are here:
> >>> Documentation/devicetree/bindings/pinctrl/pinctrl-mt65xx.txt
> >>
> >> All of the above pack the information for a pin into a single 32-bit integer.
> >> Is that what you want us to use, too?
> >> Currently we use two integers: 1) pin index, and 2) function/flags combo.
> >
> > I don't really know what you need, sorry. But some kind of figure, yes.
> > I would say whatever makes sense. 16+16 bits makes sense in most
> > combinatorial spaces does it not? If you split 32 bits in 16 bits for
> > pin and 16 bits for function, do you have more than 2^16 pins or 2^16
> > functions?
> >
> > If you really do we may need to go for u64 but ... really? Is there
> > a rational reason for that other than "we did it like this first"?
> >
> > I do not understand the notion of "flags" here. I hope that is not referring
>
> Flags refers to BI_DIR, SWIO_IN, and SWIO_OUT, from
> https://patchwork.kernel.org/patch/9643047/
>
> 32-bit should be enough to cover pins, function, and flags.
>

Geert already replied, but to avoid any confusion I'll try to remove
from driver the use of "pin config" when referring to this three
flags, which are just additional informations the pin controller needs
to perform pin muxing properly. They're not related the standard pin
config properties (pull-up/down, bias etc.. actually our hardware does
not even support these natively)

> > to pin config, because I expect that to use the standard pin config
> > bindings outside of the pinmux value which should just define the
> > pin+function combo:
> >
> > node {
> > pinmux = <PIN_NUMBER_PINMUX>;
> > GENERIC_PINCONFIG;
> > };
> >
> > Example from Mediatek:
> >
> > i2c1_pins_a: i2c1@0 {
> > pins {
> > pinmux = <MT8135_PIN_195_SDA1__FUNC_SDA1>,
> > <MT8135_PIN_196_SCL1__FUNC_SCL1>;
>
> If we follow this example, then we can list all combinations in
> include/dt-bindings/pinctrl/r7s72100-pinctrl.h, instead of creating the value
> by combining the bits using a macro where we need it in the DTS.
>
> It's gonna be a long list, though...
>

I'm strongly in favour of something like
pinmux = <PIN(1, 4) | FUNC# | FLAGS>, .... ;

opposed to
pinmux = <PIN1_4_FUNC#_FLAGS>, ... ;

Not only because it will save use from having a loong list(*) of macros that has
to be kept up to date when/if new RZ hardware will arrive, but also
because of readability and simplicity for down-stream and BSP users.
Speaking of which, I would like to know what does Chris think of this.

Is there any strong preference between the two for you Linus?

Thanks
j

(*) 12 ports, 16 pins, 8 functions, 3 flags: you can do the math
yourselves and see this is going to be hard to maintain.

> > bias-pull-up = <55>;
> > };
> > };
> >
> > So this allows for a combine pin+function number but pull ups,
> > bias etc are not baked into the thing, they have to be added on
> > separately with the generic bindings, which is nice and very readable.
>
> 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-03-29 12:38:32

by Chris Brandt

[permalink] [raw]

Subject: RE: [PATCH v2 2/7] dt-bindings: pinctrl: Add RZ/A1 bindings doc

Adding my 2 cents here:

On Wednesday, March 29, 2017, jacopo wrote:
> > > If you really do we may need to go for u64 but ... really? Is there
> > > a rational reason for that other than "we did it like this first"?
> > >
> > > I do not understand the notion of "flags" here. I hope that is not
> > > referring
> >
> > Flags refers to BI_DIR, SWIO_IN, and SWIO_OUT, from
> > https://patchwork.kernel.org/patch/9643047/
> >
> > 32-bit should be enough to cover pins, function, and flags.
> >
>
> Geert already replied, but to avoid any confusion I'll try to remove from
> driver the use of "pin config" when referring to this three flags, which
> are just additional informations the pin controller needs to perform pin
> muxing properly. They're not related the standard pin config properties
> (pull-up/down, bias etc.. actually our hardware does not even support
> these natively)
>
> > > to pin config, because I expect that to use the standard pin config
> > > bindings outside of the pinmux value which should just define the
> > > pin+function combo:
> > >
> > > node {
> > > pinmux = <PIN_NUMBER_PINMUX>;
> > > GENERIC_PINCONFIG;
> > > };
> > >
> > > Example from Mediatek:
> > >
> > > i2c1_pins_a: i2c1@0 {
> > > pins {
> > > pinmux = <MT8135_PIN_195_SDA1__FUNC_SDA1>,
> > > <MT8135_PIN_196_SCL1__FUNC_SCL1>;
> >
> > If we follow this example, then we can list all combinations in
> > include/dt-bindings/pinctrl/r7s72100-pinctrl.h, instead of creating
> > the value by combining the bits using a macro where we need it in the
> DTS.
> >
> > It's gonna be a long list, though...
> >
>
> I'm strongly in favour of something like
> pinmux = <PIN(1, 4) | FUNC# | FLAGS>, .... ;
>
> opposed to
> pinmux = <PIN1_4_FUNC#_FLAGS>, ... ;

I agree. I like "<PIN(1, 4) | FUNC# | FLAGS>".

> Not only because it will save use from having a loong list(*) of macros
> that has to be kept up to date when/if new RZ hardware will arrive, but
> also because of readability and simplicity for down-stream and BSP users.
> Speaking of which, I would like to know what does Chris think of this.

The list of macros would be very long, especially against the different
packaging version of the RZ/A1 series. 11 ports, 16-pins for each port, 8 different
function options for each pin....2 different package/pin variations.

And at the end of the day....there is no benefit for the user over just using
a macro.

A little about "this controller" for the RZ/A1: In my opinion it's a one-shot usage.
I don't foresee future RZ/A SoCs using this exact pin controller.
The reason for the "FLAGS" is to work around a quirky hardware design (in my opinion).

However, future RZ/A SoCs will use a 'similar' type controller where each pin has 8 function
options (but the FLAGs won't be needed anymore...as far as I can see...).

So I foresee the DT interface staying more or less the same, but the underlying register
setup in the driver Will change (become more simple).

Now...if we can only convince the R-Car guys to move to a more simple pin-mux controller...

Chris

2017-03-29 13:04:53

[permalink] [raw]

Subject: Re: [PATCH v2 2/7] dt-bindings: pinctrl: Add RZ/A1 bindings doc

On Wed, Mar 29, 2017 at 1:20 PM, Geert Uytterhoeven
<[email protected]> wrote:

>> I do not understand the notion of "flags" here. I hope that is not referring
>
> Flags refers to BI_DIR, SWIO_IN, and SWIO_OUT, from
> https://patchwork.kernel.org/patch/9643047/

Aha I will go in and review that closer because it doesn't seem right.

Sorry for missing it.

>> i2c1_pins_a: i2c1@0 {
>> pins {
>> pinmux = <MT8135_PIN_195_SDA1__FUNC_SDA1>,
>> <MT8135_PIN_196_SCL1__FUNC_SCL1>;
>
> If we follow this example, then we can list all combinations in
> include/dt-bindings/pinctrl/r7s72100-pinctrl.h, instead of creating the value
> by combining the bits using a macro where we need it in the DTS.
>
> It's gonna be a long list, though...

Size is not the issue, readability is the issue. I don't see why you would
need to list "all combinations" since the trees go through the C preprocessor
so you can use macros and bit | OR to build them?

Yours,
Linus Walleij

2017-03-29 13:10:23