[PATCH 0/2] pinctrl: Add RZ/A2 pin and gpio driver

[PATCH 0/2] pinctrl: Add RZ/A2 pin and gpio driver

[Chris Brandt]

The pin controller in the RZ/A2 is nothing like the pin controller in
the RZ/A1. That's a good thing! This pin controller is much more simple
and easier to configure.

So, this driver is faily simple (I hope).


Chris Brandt (2):
  pinctrl: Add RZ/A2 pin and gpio controller
  dt-bindings: pinctrl: Add RZ/A2 pinctrl and GPIO

 .../bindings/pinctrl/renesas,rza2-pinctrl.txt      |  76 +++
 drivers/pinctrl/Kconfig                            |  11 +
 drivers/pinctrl/Makefile                           |   1 +
 drivers/pinctrl/pinctrl-rza2.c                     | 519 +++++++++++++++++++++
 include/dt-bindings/pinctrl/r7s9210-pinctrl.h      |  47 ++
 5 files changed, 654 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/renesas,rza2-pinctrl.txt
 create mode 100644 drivers/pinctrl/pinctrl-rza2.c
 create mode 100644 include/dt-bindings/pinctrl/r7s9210-pinctrl.h

-- 
2.16.1

[PATCH 1/2] pinctrl: Add RZ/A2 pin and gpio controller

[Chris Brandt]

Adds support for the pin and gpio controller found in R7S9210 (RZ/A2) SoCs.

Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
---
 drivers/pinctrl/Kconfig        |  11 +
 drivers/pinctrl/Makefile       |   1 +
 drivers/pinctrl/pinctrl-rza2.c | 519 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 531 insertions(+)
 create mode 100644 drivers/pinctrl/pinctrl-rza2.c

diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index 978b2ed4d014..5819e0c9c3a8 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -195,6 +195,17 @@ config PINCTRL_RZA1
 	help
 	  This selects pinctrl driver for Renesas RZ/A1 platforms.
 
+config PINCTRL_RZA2
+	bool "Renesas RZ/A2 gpio and pinctrl driver"
+	depends on OF
+	depends on ARCH_R7S9210 || COMPILE_TEST
+	select GPIOLIB
+	select GENERIC_PINCTRL_GROUPS
+	select GENERIC_PINMUX_FUNCTIONS
+	select GENERIC_PINCONF
+	help
+	  This selects pinctrl driver for Renesas RZ/A2 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 8e127bd8610f..9354f0c2044c 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -27,6 +27,7 @@ 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_RZA2)	+= pinctrl-rza2.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-rza2.c b/drivers/pinctrl/pinctrl-rza2.c
new file mode 100644
index 000000000000..db8d5a3cf2ea
--- /dev/null
+++ b/drivers/pinctrl/pinctrl-rza2.c
@@ -0,0 +1,519 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Combined GPIO and pin controller support for Renesas RZ/A2 (R7S72100) SoC
+ *
+ * Copyright (C) 2018 Chris Brandt
+ */
+
+/*
+ * This pin controller/gpio combined driver supports Renesas devices of RZ/A2
+ * family.
+ */
+
+#include <linux/gpio.h>
+#include <linux/bitops.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/pinctrl/pinmux.h>
+
+#include "core.h"
+#include "pinmux.h"
+
+#define DRIVER_NAME		"pinctrl-rza2"
+
+#define RZA2_NPORTS		(PM + 1)
+#define RZA2_PINS_PER_PORT	8
+#define RZA2_NPINS		(RZA2_PINS_PER_PORT * RZA2_NPORTS)
+#define RZA2_PIN_ID_TO_PORT(id)	((id) / RZA2_PINS_PER_PORT)
+#define RZA2_PIN_ID_TO_PIN(id)	((id) % RZA2_PINS_PER_PORT)
+
+/*
+ * Use 16 lower bits [15:0] for pin identifier
+ * Use 16 higher bits [31:16] for pin mux function
+ */
+#define MUX_PIN_ID_MASK		GENMASK(15, 0)
+#define MUX_FUNC_MASK		GENMASK(31, 16)
+#define MUX_FUNC_OFFS		16
+#define MUX_FUNC(pinconf)	((pinconf & MUX_FUNC_MASK) >> MUX_FUNC_OFFS)
+#define MUX_FUNC(pinconf)	((pinconf & MUX_FUNC_MASK) >> MUX_FUNC_OFFS)
+
+
+enum pfc_pin_port_name {P0, P1, P2, P3, P4, P5, P6, P7, P8, P9, PA, PB, PC, PD,
+			PE, PF, PG, PH,	    PJ, PK, PL, PM};
+static const char port_names[] = "0123456789ABCDEFGHJKLM";
+
+struct rza2_pinctrl_priv {
+	struct device *dev;
+	void __iomem *base;
+
+	struct pinctrl_pin_desc *pins;
+	struct pinctrl_desc desc;
+	struct pinctrl_dev *pctl;
+};
+
+#define PDR_BASE(b)	(b + 0x0000)	/* 16-bit, 2 bytes apart */
+#define PODR_BASE(b)	(b + 0x0040)	/* 8-bit, 1 byte apart */
+#define PIDR_BASE(b)	(b + 0x0060)	/* 8-bit, 1 byte apart */
+#define PMR_BASE(b)	(b + 0x0080)	/* 8-bit, 1 byte apart */
+#define DSCR_BASE(b)	(b + 0x0140)	/* 16-bit, 2 bytes apart */
+#define PFS_BASE(b)	(b + 0x0200)	/* 8-bit, 8 bytes apart */
+#define PWPR(b)		(b + 0x02FF)	/* 8-bit */
+#define PFENET(b)	(b + 0x0820)	/* 8-bit */
+#define PPOC(b)		(b + 0x0900)	/* 32-bit */
+#define PHMOMO(b)	(b + 0x0980)	/* 32-bit */
+#define PMODEPFS(b)	(b + 0x09C0)	/* 32-bit */
+#define PCKIO(b)	(b + 0x09D0)	/* 8-bit */
+
+void rza2_set_pin_function(void __iomem *pfc_base, u8 port, u8 pin, u8 func)
+{
+	u8 reg8;
+	u16 reg16;
+	u16 mask16;
+
+	/* Set pin to 'Non-use (Hi-z input protection)'  */
+	reg16 = readw(PDR_BASE(pfc_base) + (port * 2));
+	mask16 = 0x03 << (pin * 2);
+	reg16 &= ~mask16;
+	writew(reg16, PDR_BASE((pfc_base)) + port * 2);
+
+	/* Temporarily switch to GPIO */
+	reg8 = readb(PMR_BASE(pfc_base) + port);
+	reg8 &= ~BIT(pin);
+	writeb(reg8, PMR_BASE(pfc_base) + port);
+
+	/* PFS Register Write Protect : OFF */
+	writeb(0x00, PWPR(pfc_base));	/* B0WI=0, PFSWE=0 */
+	writeb(0x40, PWPR(pfc_base));	/* B0WI=0, PFSWE=1 */
+
+	/* Set Pin function (interrupt disabled, ISEL=0) */
+	writeb(func, PFS_BASE(pfc_base) + (port * 8) + pin);
+
+	/* PFS Register Write Protect : ON */
+	writeb(0x00, PWPR(pfc_base));	/* B0WI=0, PFSWE=0 */
+	writeb(0x80, PWPR(pfc_base));	/* B0WI=1, PFSWE=0 */
+
+	/* Port Mode  : Peripheral module pin functions */
+	reg8 = readb(PMR_BASE(pfc_base) + port);
+	reg8 |= BIT(pin);
+	writeb(reg8, PMR_BASE(pfc_base) + port);
+}
+
+static void rza2_pin_to_gpio(void __iomem *pfc_base, u8 port, u8 pin, u8 dir)
+{
+	u16 reg16;
+	u16 mask16;
+
+	reg16 = readw(PDR_BASE(pfc_base) + (port * 2));
+	mask16 = 0x03 << (pin * 2);
+	reg16 &= ~mask16;
+
+	if (dir == GPIOF_DIR_IN)
+		reg16 |= 2 << (pin * 2);	// pin as input
+	else
+		reg16 |= 3 << (pin * 2);	// pin as output
+
+	writew(reg16, PDR_BASE((pfc_base)) + port * 2);
+}
+
+static int rza2_chip_get_direction(struct gpio_chip *chip, unsigned int offset)
+{
+	struct rza2_pinctrl_priv *priv = gpiochip_get_data(chip);
+	u8 port = offset / 8;
+	u8 pin = offset % 8;
+	u16 reg16;
+
+	reg16 = readw(PDR_BASE(priv->base) + (port * 2));
+	reg16 = (reg16 >> (pin * 2)) & 0x03;
+
+	if (reg16 == 3)
+		return GPIOF_DIR_OUT;
+
+	if (reg16 == 2)
+		return GPIOF_DIR_IN;
+
+	/*
+	 * This GPIO controller has a default Hi-Z state that is not input or
+	 * output, so force the pin to input now.
+	 */
+	rza2_pin_to_gpio(priv->base, port, pin, GPIOF_DIR_IN);
+	return GPIOF_DIR_IN;
+}
+
+static int rza2_chip_direction_input(struct gpio_chip *chip,
+				     unsigned int offset)
+{
+	struct rza2_pinctrl_priv *priv = gpiochip_get_data(chip);
+	u8 port = offset / 8;
+	u8 pin = offset % 8;
+
+	rza2_pin_to_gpio(priv->base, port, pin, GPIOF_DIR_IN);
+
+	return 0;
+}
+
+static int rza2_chip_direction_output(struct gpio_chip *chip,
+				      unsigned int offset, int val)
+{
+	struct rza2_pinctrl_priv *priv = gpiochip_get_data(chip);
+	u8 port = offset / 8;
+	u8 pin = offset % 8;
+
+	rza2_pin_to_gpio(priv->base, port, pin, GPIOF_DIR_OUT);
+
+	return 0;
+}
+
+static int rza2_chip_get(struct gpio_chip *chip, unsigned int offset)
+{
+	struct rza2_pinctrl_priv *priv = gpiochip_get_data(chip);
+	u8 port = offset / 8;
+	u8 pin = offset % 8;
+
+	return (readb(PIDR_BASE(priv->base) + port) >> pin) & 1;
+}
+
+static void rza2_chip_set(struct gpio_chip *chip, unsigned int offset,
+			  int value)
+{
+	struct rza2_pinctrl_priv *priv = gpiochip_get_data(chip);
+	u8 port = offset / 8;
+	u8 pin = offset % 8;
+	u8 new_value = readb(PODR_BASE(priv->base) + port);
+
+	if (value)
+		new_value |= (1 << pin);
+	else
+		new_value &= ~(1 << pin);
+
+	writeb(new_value, PODR_BASE(priv->base) + port);
+}
+
+static const char * const rza2_gpio_names[] = {
+	"P0_0", "P0_1", "P0_2", "P0_3", "P0_4", "P0_5", "P0_6", "P0_7",
+	"P1_0", "P1_1", "P1_2", "P1_3", "P1_4", "P1_5", "P1_6", "P1_7",
+	"P2_0", "P2_1", "P2_2", "P2_3", "P2_4", "P2_5", "P2_6", "P2_7",
+	"P3_0", "P3_1", "P3_2", "P3_3", "P3_4", "P3_5", "P3_6", "P3_7",
+	"P4_0", "P4_1", "P4_2", "P4_3", "P4_4", "P4_5", "P4_6", "P4_7",
+	"P5_0", "P5_1", "P5_2", "P5_3", "P5_4", "P5_5", "P5_6", "P5_7",
+	"P6_0", "P6_1", "P6_2", "P6_3", "P6_4", "P6_5", "P6_6", "P6_7",
+	"P7_0", "P7_1", "P7_2", "P7_3", "P7_4", "P7_5", "P7_6", "P7_7",
+	"P8_0", "P8_1", "P8_2", "P8_3", "P8_4", "P8_5", "P8_6", "P8_7",
+	"P9_0", "P9_1", "P9_2", "P9_3", "P9_4", "P9_5", "P9_6", "P9_7",
+	"PA_0", "PA_1", "PA_2", "PA_3", "PA_4", "PA_5", "PA_6", "PA_7",
+	"PB_0", "PB_1", "PB_2", "PB_3", "PB_4", "PB_5", "PB_6", "PB_7",
+	"PC_0", "PC_1", "PC_2", "PC_3", "PC_4", "PC_5", "PC_6", "PC_7",
+	"PD_0", "PD_1", "PD_2", "PD_3", "PD_4", "PD_5", "PD_6", "PD_7",
+	"PE_0", "PE_1", "PE_2", "PE_3", "PE_4", "PE_5", "PE_6", "PE_7",
+	"PF_0", "PF_1", "PF_2", "PF_3", "P0_4", "PF_5", "PF_6", "PF_7",
+	"PG_0", "PG_1", "PG_2", "P0_3", "PG_4", "PG_5", "PG_6", "PG_7",
+	"PH_0", "PH_1", "PH_2", "PH_3", "PH_4", "PH_5", "PH_6", "PH_7",
+	/* port I does not exist */
+	"PJ_0", "PJ_1", "PJ_2", "PJ_3", "PJ_4", "PJ_5", "PJ_6", "PJ_7",
+	"PK_0", "PK_1", "PK_2", "PK_3", "PK_4", "PK_5", "PK_6", "PK_7",
+	"PL_0", "PL_1", "PL_2", "PL_3", "PL_4", "PL_5", "PL_6", "PL_7",
+	"PM_0", "PM_1", "PM_2", "PM_3", "PM_4", "PM_5", "PM_6", "PM_7",
+};
+
+static struct gpio_chip chip = {
+	.names = rza2_gpio_names,
+	.base = -1,
+	.ngpio = RZA2_NPINS,
+	.get_direction = rza2_chip_get_direction,
+	.direction_input = rza2_chip_direction_input,
+	.direction_output = rza2_chip_direction_output,
+	.get = rza2_chip_get,
+	.set = rza2_chip_set,
+};
+
+struct pinctrl_gpio_range gpio_range;
+
+static int rza2_gpio_register(struct rza2_pinctrl_priv *priv)
+{
+	struct device_node *np = priv->dev->of_node;
+	struct of_phandle_args of_args;
+	int ret;
+
+	if (!of_property_read_bool(np, "gpio-controller")) {
+		dev_info(priv->dev, "No gpio controller registered\n");
+		return 0;
+	}
+
+	chip.label = devm_kasprintf(priv->dev, GFP_KERNEL, "%pOFn", np);
+	chip.of_node = np;
+	chip.parent = priv->dev;
+
+	ret = of_parse_phandle_with_fixed_args(np, "gpio-ranges", 3, 0,
+					       &of_args);
+	if (ret) {
+		dev_err(priv->dev, "Unable to parse gpio-ranges\n");
+		return ret;
+	}
+
+	gpio_range.id = of_args.args[0];
+	gpio_range.name = chip.label;
+	gpio_range.pin_base = gpio_range.base = of_args.args[1];
+	gpio_range.npins = of_args.args[2];
+	gpio_range.gc = &chip;
+
+	/* Register our gpio chip with gpiolib */
+	ret = devm_gpiochip_add_data(priv->dev, &chip, priv);
+	if (ret)
+		return ret;
+
+	/* Register pin range with pinctrl core */
+	pinctrl_add_gpio_range(priv->pctl, &gpio_range);
+
+	dev_dbg(priv->dev, "Registered gpio controller\n");
+
+	return 0;
+}
+
+static int rza2_pinctrl_register(struct rza2_pinctrl_priv *priv)
+{
+	struct pinctrl_pin_desc *pins;
+	unsigned int i;
+	int ret;
+
+	pins = devm_kcalloc(priv->dev, RZA2_NPINS, sizeof(*pins), GFP_KERNEL);
+	if (!pins)
+		return -ENOMEM;
+
+	priv->pins = pins;
+	priv->desc.pins = pins;
+	priv->desc.npins = RZA2_NPINS;
+
+	for (i = 0; i < RZA2_NPINS; i++) {
+		unsigned int pin = RZA2_PIN_ID_TO_PIN(i);
+		unsigned int port = RZA2_PIN_ID_TO_PORT(i);
+
+		pins[i].number = i;
+		pins[i].name = devm_kasprintf(priv->dev, GFP_KERNEL, "P%c_%u",
+					      port_names[port], pin);
+	}
+
+	ret = devm_pinctrl_register_and_init(priv->dev, &priv->desc, priv,
+					     &priv->pctl);
+	if (ret) {
+		dev_err(priv->dev, "pinctrl registration failed\n");
+		return ret;
+	}
+
+	ret = pinctrl_enable(priv->pctl);
+	if (ret) {
+		dev_err(priv->dev, "pinctrl enable failed\n");
+		return ret;
+	}
+
+	ret = rza2_gpio_register(priv);
+	if (ret) {
+		dev_err(priv->dev, "GPIO registration failed\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+/*
+ * For each DT node, create a single pin mapping. That pin mapping will only
+ * contain a single group of pins, and that group of pins will only have a
+ * single function that can be selected.
+ */
+static int rza2_dt_node_to_map(struct pinctrl_dev *pctldev,
+			       struct device_node *np,
+			       struct pinctrl_map **map,
+			       unsigned int *num_maps)
+{
+	struct rza2_pinctrl_priv *priv = pinctrl_dev_get_drvdata(pctldev);
+	struct property *of_pins;
+	int i;
+	unsigned int *pins;
+	unsigned int *psel_val;
+	const char **pin_fn;
+	int ret, npins;
+	int gsel, fsel;
+
+	/* Find out how many pins to map */
+	of_pins = of_find_property(np, "pinmux", NULL);
+	if (!of_pins) {
+		dev_info(priv->dev, "Missing pinmux property\n");
+		return -ENOENT;
+	}
+	npins = of_pins->length / sizeof(u32);
+
+	pins = devm_kcalloc(priv->dev, npins, sizeof(*pins), GFP_KERNEL);
+	psel_val = devm_kcalloc(priv->dev, npins, sizeof(*psel_val),
+				GFP_KERNEL);
+	pin_fn = devm_kzalloc(priv->dev, sizeof(*pin_fn), GFP_KERNEL);
+	if (!pins || !psel_val || !pin_fn)
+		return -ENOMEM;
+
+	/* Collect pin locations and mux settings from DT properties */
+	for (i = 0; i < npins; ++i) {
+		u32 value;
+
+		ret = of_property_read_u32_index(np, "pinmux", i, &value);
+		if (ret)
+			return ret;
+		pins[i] = value & MUX_PIN_ID_MASK;
+		psel_val[i] = MUX_FUNC(value);
+	}
+
+	/* Register a single pin group listing all the pins we read from DT */
+	gsel = pinctrl_generic_add_group(pctldev, np->name, pins, npins, NULL);
+	if (gsel < 0)
+		return gsel;
+
+	/*
+	 * Register a single group function where the 'data' is an array PSEL
+	 * register values read from DT.
+	 */
+	pin_fn[0] = np->name;
+	fsel = pinmux_generic_add_function(pctldev, np->name, pin_fn, 1,
+					   psel_val);
+	if (fsel < 0) {
+		ret = fsel;
+		goto remove_group;
+	}
+
+	dev_dbg(priv->dev, "Parsed %s with %d pins\n", np->name, npins);
+
+	/* Create map where to retrieve function and mux settings from */
+	*num_maps = 0;
+	*map = kzalloc(sizeof(**map), GFP_KERNEL);
+	if (!*map) {
+		ret = -ENOMEM;
+		goto remove_function;
+	}
+
+	(*map)->type = PIN_MAP_TYPE_MUX_GROUP;
+	(*map)->data.mux.group = np->name;
+	(*map)->data.mux.function = np->name;
+	*num_maps = 1;
+
+	return 0;
+
+remove_function:
+	pinmux_generic_remove_function(pctldev, fsel);
+
+remove_group:
+	pinctrl_generic_remove_group(pctldev, gsel);
+
+	dev_info(priv->dev, "Unable to parse DT node %s\n", np->name);
+
+	return ret;
+}
+
+static void rza2_dt_free_map(struct pinctrl_dev *pctldev,
+			     struct pinctrl_map *map, unsigned int num_maps)
+{
+	kfree(map);
+}
+
+static const struct pinctrl_ops rza2_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		= rza2_dt_node_to_map,
+	.dt_free_map		= rza2_dt_free_map,
+};
+
+static int rza2_set_mux(struct pinctrl_dev *pctldev, unsigned int selector,
+			   unsigned int group)
+{
+	struct rza2_pinctrl_priv *priv = pinctrl_dev_get_drvdata(pctldev);
+	struct function_desc *func;
+	struct group_desc *grp;
+	int i;
+	unsigned int *psel_val;
+
+	grp = pinctrl_generic_get_group(pctldev, group);
+	if (!grp)
+		return -EINVAL;
+
+	func = pinmux_generic_get_function(pctldev, selector);
+	if (!func)
+		return -EINVAL;
+
+	psel_val = func->data;
+
+	for (i = 0; i < grp->num_pins; ++i) {
+		dev_dbg(priv->dev, "Setting P%c_%d to PSEL=%d\n",
+			port_names[RZA2_PIN_ID_TO_PORT(grp->pins[i])],
+			RZA2_PIN_ID_TO_PIN(grp->pins[i]),
+			psel_val[i]);
+		rza2_set_pin_function(
+			priv->base,
+			RZA2_PIN_ID_TO_PORT(grp->pins[i]),
+			RZA2_PIN_ID_TO_PIN(grp->pins[i]),
+			psel_val[i]);
+	}
+
+	return 0;
+}
+
+static const struct pinmux_ops rza2_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		= rza2_set_mux,
+	.strict			= true,
+};
+
+static int rza2_pinctrl_probe(struct platform_device *pdev)
+{
+	struct rza2_pinctrl_priv *priv;
+	struct resource *res;
+	int ret;
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->dev = &pdev->dev;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	priv->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(priv->base))
+		return PTR_ERR(priv->base);
+
+	platform_set_drvdata(pdev, priv);
+
+	priv->desc.name		= DRIVER_NAME;
+	priv->desc.pctlops	= &rza2_pinctrl_ops;
+	priv->desc.pmxops	= &rza2_pinmux_ops;
+	priv->desc.owner	= THIS_MODULE;
+
+	ret = rza2_pinctrl_register(priv);
+	if (ret)
+		return ret;
+
+	pr_info("RZ/A2 pin controller registered\n");
+
+	return 0;
+}
+
+static const struct of_device_id rza2_pinctrl_of_match[] = {
+	{
+		.compatible	= "renesas,r7s9210-pinctrl",
+	},
+	{ /* sentinel */ }
+};
+
+static struct platform_driver rza2_pinctrl_driver = {
+	.driver = {
+		.name = DRIVER_NAME,
+		.of_match_table = rza2_pinctrl_of_match,
+	},
+	.probe = rza2_pinctrl_probe,
+};
+
+static int __init rza2_pinctrl_init(void)
+{
+	return platform_driver_register(&rza2_pinctrl_driver);
+}
+core_initcall(rza2_pinctrl_init);
+
+
+MODULE_AUTHOR("Chris Brandt <chris.brandt@renesas.com>");
+MODULE_DESCRIPTION("Pin and gpio controller driver for RZ/A2 SoC");
+MODULE_LICENSE("GPL v2");
-- 
2.16.1

Re: [PATCH 1/2] pinctrl: Add RZ/A2 pin and gpio controller

[jacopo mondi]

[-- Attachment #1: Type: text/plain, Size: 20199 bytes --]

Hi Chris,
   thanks for the patches.

On Fri, Oct 05, 2018 at 10:09:50AM -0500, Chris Brandt wrote:
> Adds support for the pin and gpio controller found in R7S9210 (RZ/A2) SoCs.
>
> Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
> ---
>  drivers/pinctrl/Kconfig        |  11 +
>  drivers/pinctrl/Makefile       |   1 +
>  drivers/pinctrl/pinctrl-rza2.c | 519 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 531 insertions(+)
>  create mode 100644 drivers/pinctrl/pinctrl-rza2.c
>
> diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
> index 978b2ed4d014..5819e0c9c3a8 100644
> --- a/drivers/pinctrl/Kconfig
> +++ b/drivers/pinctrl/Kconfig
> @@ -195,6 +195,17 @@ config PINCTRL_RZA1
>  	help
>  	  This selects pinctrl driver for Renesas RZ/A1 platforms.
>
> +config PINCTRL_RZA2
> +	bool "Renesas RZ/A2 gpio and pinctrl driver"
> +	depends on OF
> +	depends on ARCH_R7S9210 || COMPILE_TEST
> +	select GPIOLIB
> +	select GENERIC_PINCTRL_GROUPS
> +	select GENERIC_PINMUX_FUNCTIONS
> +	select GENERIC_PINCONF
> +	help
> +	  This selects pinctrl driver for Renesas RZ/A2 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 8e127bd8610f..9354f0c2044c 100644
> --- a/drivers/pinctrl/Makefile
> +++ b/drivers/pinctrl/Makefile
> @@ -27,6 +27,7 @@ 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_RZA2)	+= pinctrl-rza2.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-rza2.c b/drivers/pinctrl/pinctrl-rza2.c
> new file mode 100644
> index 000000000000..db8d5a3cf2ea
> --- /dev/null
> +++ b/drivers/pinctrl/pinctrl-rza2.c
> @@ -0,0 +1,519 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Combined GPIO and pin controller support for Renesas RZ/A2 (R7S72100) SoC

R7S9210

> + *
> + * Copyright (C) 2018 Chris Brandt
> + */
> +
> +/*
> + * This pin controller/gpio combined driver supports Renesas devices of RZ/A2
> + * family.
> + */
> +
> +#include <linux/gpio.h>
> +#include <linux/bitops.h>

Headers sorted please :)

> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/pinctrl/pinmux.h>
> +
> +#include "core.h"
> +#include "pinmux.h"
> +
> +#define DRIVER_NAME		"pinctrl-rza2"
> +
> +#define RZA2_NPORTS		(PM + 1)

Why not making this part of the enum itself?

> +#define RZA2_PINS_PER_PORT	8
> +#define RZA2_NPINS		(RZA2_PINS_PER_PORT * RZA2_NPORTS)
> +#define RZA2_PIN_ID_TO_PORT(id)	((id) / RZA2_PINS_PER_PORT)
> +#define RZA2_PIN_ID_TO_PIN(id)	((id) % RZA2_PINS_PER_PORT)
> +
> +/*
> + * Use 16 lower bits [15:0] for pin identifier
> + * Use 16 higher bits [31:16] for pin mux function
> + */
> +#define MUX_PIN_ID_MASK		GENMASK(15, 0)
> +#define MUX_FUNC_MASK		GENMASK(31, 16)
> +#define MUX_FUNC_OFFS		16
> +#define MUX_FUNC(pinconf)	((pinconf & MUX_FUNC_MASK) >> MUX_FUNC_OFFS)
> +#define MUX_FUNC(pinconf)	((pinconf & MUX_FUNC_MASK) >> MUX_FUNC_OFFS)

Seems like the same line to me.
Also, double empty line.

> +
> +
> +enum pfc_pin_port_name {P0, P1, P2, P3, P4, P5, P6, P7, P8, P9, PA, PB, PC, PD,
> +			PE, PF, PG, PH,	    PJ, PK, PL, PM};
weird spacing                          ^^^^

> +static const char port_names[] = "0123456789ABCDEFGHJKLM";
> +
> +struct rza2_pinctrl_priv {
> +	struct device *dev;
> +	void __iomem *base;
> +
> +	struct pinctrl_pin_desc *pins;
> +	struct pinctrl_desc desc;
> +	struct pinctrl_dev *pctl;
> +};
> +
> +#define PDR_BASE(b)	(b + 0x0000)	/* 16-bit, 2 bytes apart */

() around macros parameters when used:
                        ((b) + 0x0000)
                        which is just (b) by the way :)

Also, please prefix all defines with RZA2_ not to pollute the global
symbols space.

> +#define PODR_BASE(b)	(b + 0x0040)	/* 8-bit, 1 byte apart */
> +#define PIDR_BASE(b)	(b + 0x0060)	/* 8-bit, 1 byte apart */
> +#define PMR_BASE(b)	(b + 0x0080)	/* 8-bit, 1 byte apart */
> +#define DSCR_BASE(b)	(b + 0x0140)	/* 16-bit, 2 bytes apart */
> +#define PFS_BASE(b)	(b + 0x0200)	/* 8-bit, 8 bytes apart */

Here you define registers address bases, but instead of computing
everytime the port-pin dedicated register address (I'm thinking about
PFS specifically but it applies to others), would you consider providing
accessors helpers to write the offset computations process just once here?

Eg.
#define RZA2_PFS(_b, _port, _pin) (RZA2_PFS_BASE(_b) + (_port) * 8 + (_pin))

Same for other register which needs offset calculation based on port
and pin values.

> +#define PWPR(b)		(b + 0x02FF)	/* 8-bit */
> +#define PFENET(b)	(b + 0x0820)	/* 8-bit */
> +#define PPOC(b)		(b + 0x0900)	/* 32-bit */
> +#define PHMOMO(b)	(b + 0x0980)	/* 32-bit */
> +#define PMODEPFS(b)	(b + 0x09C0)	/* 32-bit */
> +#define PCKIO(b)	(b + 0x09D0)	/* 8-bit */

> +
> +void rza2_set_pin_function(void __iomem *pfc_base, u8 port, u8 pin, u8 func)
static ?

> +{
> +	u8 reg8;
> +	u16 reg16;
> +	u16 mask16;
> +
> +	/* Set pin to 'Non-use (Hi-z input protection)'  */
> +	reg16 = readw(PDR_BASE(pfc_base) + (port * 2));

as in example,

        reg16 = readw(RZA2_PDR(pfc_base), port));

it's nicer and isolates the offset calculations, which differs
from one register to another (the 'x bytes apart' comment you added in
the registers definitions represents this, right? )


> +	mask16 = 0x03 << (pin * 2);
> +	reg16 &= ~mask16;
> +	writew(reg16, PDR_BASE((pfc_base)) + port * 2);
> +
> +	/* Temporarily switch to GPIO */
> +	reg8 = readb(PMR_BASE(pfc_base) + port);
> +	reg8 &= ~BIT(pin);
> +	writeb(reg8, PMR_BASE(pfc_base) + port);
> +
> +	/* PFS Register Write Protect : OFF */
> +	writeb(0x00, PWPR(pfc_base));	/* B0WI=0, PFSWE=0 */
> +	writeb(0x40, PWPR(pfc_base));	/* B0WI=0, PFSWE=1 */
> +
> +	/* Set Pin function (interrupt disabled, ISEL=0) */
> +	writeb(func, PFS_BASE(pfc_base) + (port * 8) + pin);
> +
> +	/* PFS Register Write Protect : ON */
> +	writeb(0x00, PWPR(pfc_base));	/* B0WI=0, PFSWE=0 */
> +	writeb(0x80, PWPR(pfc_base));	/* B0WI=1, PFSWE=0 */
> +
> +	/* Port Mode  : Peripheral module pin functions */
> +	reg8 = readb(PMR_BASE(pfc_base) + port);
> +	reg8 |= BIT(pin);
> +	writeb(reg8, PMR_BASE(pfc_base) + port);
> +}
> +
> +static void rza2_pin_to_gpio(void __iomem *pfc_base, u8 port, u8 pin, u8 dir)
> +{
> +	u16 reg16;
> +	u16 mask16;
> +
> +	reg16 = readw(PDR_BASE(pfc_base) + (port * 2));
> +	mask16 = 0x03 << (pin * 2);
> +	reg16 &= ~mask16;
> +
> +	if (dir == GPIOF_DIR_IN)
> +		reg16 |= 2 << (pin * 2);	// pin as input

C++ comments style!

> +	else
> +		reg16 |= 3 << (pin * 2);	// pin as output

Also, you are using those '2' and '3' values here and there in the
code, as they represent the pin 'status. Would you consider making
a define for them? Actually, for all of them

#define RZA2_PIN_STATE_HIGHZ    0x00
#define RZA2_PIN_STATE_INPUT    0x02
#define RZA2_PIN_STATE_OUTPUT   0x03

> +
> +	writew(reg16, PDR_BASE((pfc_base)) + port * 2);
> +}
> +
> +static int rza2_chip_get_direction(struct gpio_chip *chip, unsigned int offset)
> +{
> +	struct rza2_pinctrl_priv *priv = gpiochip_get_data(chip);
> +	u8 port = offset / 8;
> +	u8 pin = offset % 8;
> +	u16 reg16;
> +
> +	reg16 = readw(PDR_BASE(priv->base) + (port * 2));
> +	reg16 = (reg16 >> (pin * 2)) & 0x03;
> +
> +	if (reg16 == 3)
> +		return GPIOF_DIR_OUT;
> +
> +	if (reg16 == 2)
> +		return GPIOF_DIR_IN;
> +
> +	/*
> +	 * This GPIO controller has a default Hi-Z state that is not input or
> +	 * output, so force the pin to input now.
> +	 */
I wonder if it is fine for the .get_direction callback to change the
pin state.
> +	rza2_pin_to_gpio(priv->base, port, pin, GPIOF_DIR_IN);

Empty line before return is nice (here and in other places).

I'll stop here with comments on the driver, as I should have a look at
bindings before. I'll comment there too and the get back to this.

Thanks
   j

> +	return GPIOF_DIR_IN;

> +}
> +
> +static int rza2_chip_direction_input(struct gpio_chip *chip,
> +				     unsigned int offset)
> +{
> +	struct rza2_pinctrl_priv *priv = gpiochip_get_data(chip);
> +	u8 port = offset / 8;
> +	u8 pin = offset % 8;
> +
> +	rza2_pin_to_gpio(priv->base, port, pin, GPIOF_DIR_IN);
> +
> +	return 0;
> +}
> +
> +static int rza2_chip_direction_output(struct gpio_chip *chip,
> +				      unsigned int offset, int val)
> +{
> +	struct rza2_pinctrl_priv *priv = gpiochip_get_data(chip);
> +	u8 port = offset / 8;
> +	u8 pin = offset % 8;
> +
> +	rza2_pin_to_gpio(priv->base, port, pin, GPIOF_DIR_OUT);
> +
> +	return 0;
> +}
> +
> +static int rza2_chip_get(struct gpio_chip *chip, unsigned int offset)
> +{
> +	struct rza2_pinctrl_priv *priv = gpiochip_get_data(chip);
> +	u8 port = offset / 8;
> +	u8 pin = offset % 8;
> +
> +	return (readb(PIDR_BASE(priv->base) + port) >> pin) & 1;
> +}
> +
> +static void rza2_chip_set(struct gpio_chip *chip, unsigned int offset,
> +			  int value)
> +{
> +	struct rza2_pinctrl_priv *priv = gpiochip_get_data(chip);
> +	u8 port = offset / 8;
> +	u8 pin = offset % 8;
> +	u8 new_value = readb(PODR_BASE(priv->base) + port);
> +
> +	if (value)
> +		new_value |= (1 << pin);
> +	else
> +		new_value &= ~(1 << pin);
> +
> +	writeb(new_value, PODR_BASE(priv->base) + port);
> +}
> +
> +static const char * const rza2_gpio_names[] = {
> +	"P0_0", "P0_1", "P0_2", "P0_3", "P0_4", "P0_5", "P0_6", "P0_7",
> +	"P1_0", "P1_1", "P1_2", "P1_3", "P1_4", "P1_5", "P1_6", "P1_7",
> +	"P2_0", "P2_1", "P2_2", "P2_3", "P2_4", "P2_5", "P2_6", "P2_7",
> +	"P3_0", "P3_1", "P3_2", "P3_3", "P3_4", "P3_5", "P3_6", "P3_7",
> +	"P4_0", "P4_1", "P4_2", "P4_3", "P4_4", "P4_5", "P4_6", "P4_7",
> +	"P5_0", "P5_1", "P5_2", "P5_3", "P5_4", "P5_5", "P5_6", "P5_7",
> +	"P6_0", "P6_1", "P6_2", "P6_3", "P6_4", "P6_5", "P6_6", "P6_7",
> +	"P7_0", "P7_1", "P7_2", "P7_3", "P7_4", "P7_5", "P7_6", "P7_7",
> +	"P8_0", "P8_1", "P8_2", "P8_3", "P8_4", "P8_5", "P8_6", "P8_7",
> +	"P9_0", "P9_1", "P9_2", "P9_3", "P9_4", "P9_5", "P9_6", "P9_7",
> +	"PA_0", "PA_1", "PA_2", "PA_3", "PA_4", "PA_5", "PA_6", "PA_7",
> +	"PB_0", "PB_1", "PB_2", "PB_3", "PB_4", "PB_5", "PB_6", "PB_7",
> +	"PC_0", "PC_1", "PC_2", "PC_3", "PC_4", "PC_5", "PC_6", "PC_7",
> +	"PD_0", "PD_1", "PD_2", "PD_3", "PD_4", "PD_5", "PD_6", "PD_7",
> +	"PE_0", "PE_1", "PE_2", "PE_3", "PE_4", "PE_5", "PE_6", "PE_7",
> +	"PF_0", "PF_1", "PF_2", "PF_3", "P0_4", "PF_5", "PF_6", "PF_7",
> +	"PG_0", "PG_1", "PG_2", "P0_3", "PG_4", "PG_5", "PG_6", "PG_7",
> +	"PH_0", "PH_1", "PH_2", "PH_3", "PH_4", "PH_5", "PH_6", "PH_7",
> +	/* port I does not exist */
> +	"PJ_0", "PJ_1", "PJ_2", "PJ_3", "PJ_4", "PJ_5", "PJ_6", "PJ_7",
> +	"PK_0", "PK_1", "PK_2", "PK_3", "PK_4", "PK_5", "PK_6", "PK_7",
> +	"PL_0", "PL_1", "PL_2", "PL_3", "PL_4", "PL_5", "PL_6", "PL_7",
> +	"PM_0", "PM_1", "PM_2", "PM_3", "PM_4", "PM_5", "PM_6", "PM_7",
> +};
> +
> +static struct gpio_chip chip = {
> +	.names = rza2_gpio_names,
> +	.base = -1,
> +	.ngpio = RZA2_NPINS,
> +	.get_direction = rza2_chip_get_direction,
> +	.direction_input = rza2_chip_direction_input,
> +	.direction_output = rza2_chip_direction_output,
> +	.get = rza2_chip_get,
> +	.set = rza2_chip_set,
> +};
> +
> +struct pinctrl_gpio_range gpio_range;
> +
> +static int rza2_gpio_register(struct rza2_pinctrl_priv *priv)
> +{
> +	struct device_node *np = priv->dev->of_node;
> +	struct of_phandle_args of_args;
> +	int ret;
> +
> +	if (!of_property_read_bool(np, "gpio-controller")) {
> +		dev_info(priv->dev, "No gpio controller registered\n");
> +		return 0;
> +	}
> +
> +	chip.label = devm_kasprintf(priv->dev, GFP_KERNEL, "%pOFn", np);
> +	chip.of_node = np;
> +	chip.parent = priv->dev;
> +
> +	ret = of_parse_phandle_with_fixed_args(np, "gpio-ranges", 3, 0,
> +					       &of_args);
> +	if (ret) {
> +		dev_err(priv->dev, "Unable to parse gpio-ranges\n");
> +		return ret;
> +	}
> +
> +	gpio_range.id = of_args.args[0];
> +	gpio_range.name = chip.label;
> +	gpio_range.pin_base = gpio_range.base = of_args.args[1];
> +	gpio_range.npins = of_args.args[2];
> +	gpio_range.gc = &chip;
> +
> +	/* Register our gpio chip with gpiolib */
> +	ret = devm_gpiochip_add_data(priv->dev, &chip, priv);
> +	if (ret)
> +		return ret;
> +
> +	/* Register pin range with pinctrl core */
> +	pinctrl_add_gpio_range(priv->pctl, &gpio_range);
> +
> +	dev_dbg(priv->dev, "Registered gpio controller\n");
> +
> +	return 0;
> +}
> +
> +static int rza2_pinctrl_register(struct rza2_pinctrl_priv *priv)
> +{
> +	struct pinctrl_pin_desc *pins;
> +	unsigned int i;
> +	int ret;
> +
> +	pins = devm_kcalloc(priv->dev, RZA2_NPINS, sizeof(*pins), GFP_KERNEL);
> +	if (!pins)
> +		return -ENOMEM;
> +
> +	priv->pins = pins;
> +	priv->desc.pins = pins;
> +	priv->desc.npins = RZA2_NPINS;
> +
> +	for (i = 0; i < RZA2_NPINS; i++) {
> +		unsigned int pin = RZA2_PIN_ID_TO_PIN(i);
> +		unsigned int port = RZA2_PIN_ID_TO_PORT(i);
> +
> +		pins[i].number = i;
> +		pins[i].name = devm_kasprintf(priv->dev, GFP_KERNEL, "P%c_%u",
> +					      port_names[port], pin);
> +	}
> +
> +	ret = devm_pinctrl_register_and_init(priv->dev, &priv->desc, priv,
> +					     &priv->pctl);
> +	if (ret) {
> +		dev_err(priv->dev, "pinctrl registration failed\n");
> +		return ret;
> +	}
> +
> +	ret = pinctrl_enable(priv->pctl);
> +	if (ret) {
> +		dev_err(priv->dev, "pinctrl enable failed\n");
> +		return ret;
> +	}
> +
> +	ret = rza2_gpio_register(priv);
> +	if (ret) {
> +		dev_err(priv->dev, "GPIO registration failed\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +/*
> + * For each DT node, create a single pin mapping. That pin mapping will only
> + * contain a single group of pins, and that group of pins will only have a
> + * single function that can be selected.
> + */
> +static int rza2_dt_node_to_map(struct pinctrl_dev *pctldev,
> +			       struct device_node *np,
> +			       struct pinctrl_map **map,
> +			       unsigned int *num_maps)
> +{
> +	struct rza2_pinctrl_priv *priv = pinctrl_dev_get_drvdata(pctldev);
> +	struct property *of_pins;
> +	int i;
> +	unsigned int *pins;
> +	unsigned int *psel_val;
> +	const char **pin_fn;
> +	int ret, npins;
> +	int gsel, fsel;
> +
> +	/* Find out how many pins to map */
> +	of_pins = of_find_property(np, "pinmux", NULL);
> +	if (!of_pins) {
> +		dev_info(priv->dev, "Missing pinmux property\n");
> +		return -ENOENT;
> +	}
> +	npins = of_pins->length / sizeof(u32);
> +
> +	pins = devm_kcalloc(priv->dev, npins, sizeof(*pins), GFP_KERNEL);
> +	psel_val = devm_kcalloc(priv->dev, npins, sizeof(*psel_val),
> +				GFP_KERNEL);
> +	pin_fn = devm_kzalloc(priv->dev, sizeof(*pin_fn), GFP_KERNEL);
> +	if (!pins || !psel_val || !pin_fn)
> +		return -ENOMEM;
> +
> +	/* Collect pin locations and mux settings from DT properties */
> +	for (i = 0; i < npins; ++i) {
> +		u32 value;
> +
> +		ret = of_property_read_u32_index(np, "pinmux", i, &value);
> +		if (ret)
> +			return ret;
> +		pins[i] = value & MUX_PIN_ID_MASK;
> +		psel_val[i] = MUX_FUNC(value);
> +	}
> +
> +	/* Register a single pin group listing all the pins we read from DT */
> +	gsel = pinctrl_generic_add_group(pctldev, np->name, pins, npins, NULL);
> +	if (gsel < 0)
> +		return gsel;
> +
> +	/*
> +	 * Register a single group function where the 'data' is an array PSEL
> +	 * register values read from DT.
> +	 */
> +	pin_fn[0] = np->name;
> +	fsel = pinmux_generic_add_function(pctldev, np->name, pin_fn, 1,
> +					   psel_val);
> +	if (fsel < 0) {
> +		ret = fsel;
> +		goto remove_group;
> +	}
> +
> +	dev_dbg(priv->dev, "Parsed %s with %d pins\n", np->name, npins);
> +
> +	/* Create map where to retrieve function and mux settings from */
> +	*num_maps = 0;
> +	*map = kzalloc(sizeof(**map), GFP_KERNEL);
> +	if (!*map) {
> +		ret = -ENOMEM;
> +		goto remove_function;
> +	}
> +
> +	(*map)->type = PIN_MAP_TYPE_MUX_GROUP;
> +	(*map)->data.mux.group = np->name;
> +	(*map)->data.mux.function = np->name;
> +	*num_maps = 1;
> +
> +	return 0;
> +
> +remove_function:
> +	pinmux_generic_remove_function(pctldev, fsel);
> +
> +remove_group:
> +	pinctrl_generic_remove_group(pctldev, gsel);
> +
> +	dev_info(priv->dev, "Unable to parse DT node %s\n", np->name);
> +
> +	return ret;
> +}
> +
> +static void rza2_dt_free_map(struct pinctrl_dev *pctldev,
> +			     struct pinctrl_map *map, unsigned int num_maps)
> +{
> +	kfree(map);
> +}
> +
> +static const struct pinctrl_ops rza2_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		= rza2_dt_node_to_map,
> +	.dt_free_map		= rza2_dt_free_map,
> +};
> +
> +static int rza2_set_mux(struct pinctrl_dev *pctldev, unsigned int selector,
> +			   unsigned int group)
> +{
> +	struct rza2_pinctrl_priv *priv = pinctrl_dev_get_drvdata(pctldev);
> +	struct function_desc *func;
> +	struct group_desc *grp;
> +	int i;
> +	unsigned int *psel_val;
> +
> +	grp = pinctrl_generic_get_group(pctldev, group);
> +	if (!grp)
> +		return -EINVAL;
> +
> +	func = pinmux_generic_get_function(pctldev, selector);
> +	if (!func)
> +		return -EINVAL;
> +
> +	psel_val = func->data;
> +
> +	for (i = 0; i < grp->num_pins; ++i) {
> +		dev_dbg(priv->dev, "Setting P%c_%d to PSEL=%d\n",
> +			port_names[RZA2_PIN_ID_TO_PORT(grp->pins[i])],
> +			RZA2_PIN_ID_TO_PIN(grp->pins[i]),
> +			psel_val[i]);
> +		rza2_set_pin_function(
> +			priv->base,
> +			RZA2_PIN_ID_TO_PORT(grp->pins[i]),
> +			RZA2_PIN_ID_TO_PIN(grp->pins[i]),
> +			psel_val[i]);
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct pinmux_ops rza2_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		= rza2_set_mux,
> +	.strict			= true,
> +};
> +
> +static int rza2_pinctrl_probe(struct platform_device *pdev)
> +{
> +	struct rza2_pinctrl_priv *priv;
> +	struct resource *res;
> +	int ret;
> +
> +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->dev = &pdev->dev;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	priv->base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(priv->base))
> +		return PTR_ERR(priv->base);
> +
> +	platform_set_drvdata(pdev, priv);
> +
> +	priv->desc.name		= DRIVER_NAME;
> +	priv->desc.pctlops	= &rza2_pinctrl_ops;
> +	priv->desc.pmxops	= &rza2_pinmux_ops;
> +	priv->desc.owner	= THIS_MODULE;
> +
> +	ret = rza2_pinctrl_register(priv);
> +	if (ret)
> +		return ret;
> +
> +	pr_info("RZ/A2 pin controller registered\n");
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id rza2_pinctrl_of_match[] = {
> +	{
> +		.compatible	= "renesas,r7s9210-pinctrl",
> +	},
> +	{ /* sentinel */ }
> +};
> +
> +static struct platform_driver rza2_pinctrl_driver = {
> +	.driver = {
> +		.name = DRIVER_NAME,
> +		.of_match_table = rza2_pinctrl_of_match,
> +	},
> +	.probe = rza2_pinctrl_probe,
> +};
> +
> +static int __init rza2_pinctrl_init(void)
> +{
> +	return platform_driver_register(&rza2_pinctrl_driver);
> +}
> +core_initcall(rza2_pinctrl_init);
> +
> +
> +MODULE_AUTHOR("Chris Brandt <chris.brandt@renesas.com>");
> +MODULE_DESCRIPTION("Pin and gpio controller driver for RZ/A2 SoC");
> +MODULE_LICENSE("GPL v2");
> --
> 2.16.1
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

RE: [PATCH 1/2] pinctrl: Add RZ/A2 pin and gpio controller

[Chris Brandt]

Hi Jacopo,

>    thanks for the patches.
Thanks for the review!

On Thursday, October 18, 2018, jacopo mondi wrote:

> > + * Combined GPIO and pin controller support for Renesas RZ/A2
> (R7S72100) SoC
> 
> R7S9210

Thanks.
hmm,  I wonder what pinctrl driver I copied that from... ;)

> > +#include <linux/gpio.h>
> > +#include <linux/bitops.h>
> 
> Headers sorted please :)

OK, sorted.


> > +#define RZA2_NPORTS		(PM + 1)
> 
> Why not making this part of the enum itself?

Good idea.

> > +#define MUX_FUNC(pinconf)	((pinconf & MUX_FUNC_MASK) >> MUX_FUNC_OFFS)
> > +#define MUX_FUNC(pinconf)	((pinconf & MUX_FUNC_MASK) >> MUX_FUNC_OFFS)
> 
> Seems like the same line to me.
> Also, double empty line.

Oops, thanks.


> > +enum pfc_pin_port_name {P0, P1, P2, P3, P4, P5, P6, P7, P8, P9, PA, PB,
> PC, PD,
> > +			PE, PF, PG, PH,	    PJ, PK, PL, PM};
> weird spacing                   ^^^^

I was doing that to point out that there is no "I" port (they skipped 
over it). But if you didn't realize that, then it is not obvious, so I'll 
just format it normally.




> > +static const char port_names[] = "0123456789ABCDEFGHJKLM";
> > +
> > +struct rza2_pinctrl_priv {
> > +	struct device *dev;
> > +	void __iomem *base;
> > +
> > +	struct pinctrl_pin_desc *pins;
> > +	struct pinctrl_desc desc;
> > +	struct pinctrl_dev *pctl;
> > +};
> > +
> > +#define PDR_BASE(b)	(b + 0x0000)	/* 16-bit, 2 bytes apart */
> 
> () around macros parameters when used:
>                         ((b) + 0x0000)
>                         which is just (b) by the way :)

OK.
The "+ 0x0000" was more for documentation reason.

> Also, please prefix all defines with RZA2_ not to pollute the global
> symbols space.


OK.

> > +#define PODR_BASE(b)	(b + 0x0040)	/* 8-bit, 1 byte apart */
> > +#define PIDR_BASE(b)	(b + 0x0060)	/* 8-bit, 1 byte apart */
> > +#define PMR_BASE(b)	(b + 0x0080)	/* 8-bit, 1 byte apart */
> > +#define DSCR_BASE(b)	(b + 0x0140)	/* 16-bit, 2 bytes apart */
> > +#define PFS_BASE(b)	(b + 0x0200)	/* 8-bit, 8 bytes apart */
> 
> Here you define registers address bases, but instead of computing
> everytime the port-pin dedicated register address (I'm thinking about
> PFS specifically but it applies to others), would you consider providing
> accessors helpers to write the offset computations process just once here?
> 
> Eg.
> #define RZA2_PFS(_b, _port, _pin) (RZA2_PFS_BASE(_b) + (_port) * 8 +
> (_pin))
> 
> Same for other register which needs offset calculation based on port
> and pin values.

I like that idea. Thanks.


> > +#define PWPR(b)		(b + 0x02FF)	/* 8-bit */
> > +#define PFENET(b)	(b + 0x0820)	/* 8-bit */
> > +#define PPOC(b)		(b + 0x0900)	/* 32-bit */
> > +#define PHMOMO(b)	(b + 0x0980)	/* 32-bit */
> > +#define PMODEPFS(b)	(b + 0x09C0)	/* 32-bit */
> > +#define PCKIO(b)	(b + 0x09D0)	/* 8-bit */
> 
> > +
> > +void rza2_set_pin_function(void __iomem *pfc_base, u8 port, u8 pin, u8
> func)
> static ?

I actually left that as global for a reason (because for some testing, I
need a way to set pin directly in a non-standard way).
But, I might as well make it static and just use a different hack for my
testing.

> > +	/* Set pin to 'Non-use (Hi-z input protection)'  */
> > +	reg16 = readw(PDR_BASE(pfc_base) + (port * 2));
> 
> as in example,
> 
>         reg16 = readw(RZA2_PDR(pfc_base), port));
> 
> it's nicer and isolates the offset calculations, which differs
> from one register to another (the 'x bytes apart' comment you added in
> the registers definitions represents this, right? )

Yup, that's what my code looks like now after the change :)

> > +	if (dir == GPIOF_DIR_IN)
> > +		reg16 |= 2 << (pin * 2);	// pin as input
> 
> C++ comments style!

OK, I changed it.

However....
I can't find anywhere it says that // is not proper.

For example, just look at checkpatch.pl:

$ grep -m1  allow_c99_comments  scripts/checkpatch.pl
my $allow_c99_comments = 1;

Also every file has this as the very first line:
// SPDX-License-Identifier: GPL-2.0

;)


> > +	else
> > +		reg16 |= 3 << (pin * 2);	// pin as output
> 
> Also, you are using those '2' and '3' values here and there in the
> code, as they represent the pin 'status. Would you consider making
> a define for them? Actually, for all of them
> 
> #define RZA2_PIN_STATE_HIGHZ    0x00
> #define RZA2_PIN_STATE_INPUT    0x02
> #define RZA2_PIN_STATE_OUTPUT   0x03

OK.


> > +	/*
> > +	 * This GPIO controller has a default Hi-Z state that is not input
> or
> > +	 * output, so force the pin to input now.
> > +	 */
> I wonder if it is fine for the .get_direction callback to change the
> pin state.

I see no other option. I can only return "input" or "output", but Hi-Z 
is neither one of those.
The .get_direction is the first one to get called after assigning a pin.
If I return that it's an 'input', it will think it does not have to call
.direction_input, and then the pin doesn't work.


> > +	rza2_pin_to_gpio(priv->base, port, pin, GPIOF_DIR_IN);
> 
> Empty line before return is nice (here and in other places).

True. I added it.

> I'll stop here with comments on the driver, as I should have a look at
> bindings before. I'll comment there too and the get back to this.

OK. I've made all these changes.

I see just sent some more comments, so I'll have a look at them first 
before I send a V2.

Thanks,
Chris

[PATCH 2/2] dt-bindings: pinctrl: Add RZ/A2 pinctrl and GPIO

[Chris Brandt]

Add device tree binding documentation and header file for Renesas R7S9210
(RZ/A2) SoCs.

Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
---
 .../bindings/pinctrl/renesas,rza2-pinctrl.txt      | 76 ++++++++++++++++++++++
 include/dt-bindings/pinctrl/r7s9210-pinctrl.h      | 47 +++++++++++++
 2 files changed, 123 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/renesas,rza2-pinctrl.txt
 create mode 100644 include/dt-bindings/pinctrl/r7s9210-pinctrl.h

diff --git a/Documentation/devicetree/bindings/pinctrl/renesas,rza2-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/renesas,rza2-pinctrl.txt
new file mode 100644
index 000000000000..5f338054f493
--- /dev/null
+++ b/Documentation/devicetree/bindings/pinctrl/renesas,rza2-pinctrl.txt
@@ -0,0 +1,76 @@
+Renesas RZ/A2 combined Pin and GPIO controller
+
+The Renesas SoCs of the RZ/A2 series feature a combined Pin and GPIO controller.
+Pin multiplexing and GPIO configuration is performed on a per-pin basis.
+Each port features up to 8 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: should be:
+    - "renesas,r7s9210-pinctrl": for RZ/A2M
+
+  - reg
+    address base and length of the memory area where the pin controller
+    hardware is mapped to.
+
+Optional properties:
+  - gpio-controller
+    Include this in order to enable GPIO functionality. When included, both
+    gpio_cells and gpio_ranges are then required.
+  - #gpio-cells
+    Must be 2
+  - gpio-ranges
+    Expresses the total number GPIO ports/pins in this SoC
+
+
+Example: Pin controller node for RZ/A2M SoC (r7s9210)
+
+	pinctrl: pin-controller@fcffe000 {
+		compatible = "renesas,r7s9210-pinctrl";
+		reg = <0xfcffe000 0x9D1>;
+
+		gpio-controller;
+		#gpio-cells = <2>;
+		gpio-ranges = <&pinctrl 0 0 176>;
+	};
+
+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.
+  The values for the pinmux properties are a combination of port name, pin
+  number and the desired function index. Use the RZA2_PINMUX macro located
+  in include/dt-bindings/pinctrl/r7s9210-pinctrl.h to easily define these.
+  For assigning GPIO pins, use the macro RZA2_PIN_ID also in r7s9210-pinctrl.h
+  to express the desired port pin.
+
+  Example: Board specific pins configuration
+
+	&pinctrl {
+		/* Serial Console */
+		scif4_pins: serial4 {
+			pinmux = <RZA2_PINMUX(P9, 0, 4)>,	/* TxD4 */
+				 <RZA2_PINMUX(P9, 1, 4)>;	/* RxD4 */
+		};
+	};
+
+  Example: Assigning a GPIO:
+
+	leds {
+		status = "okay";
+		compatible = "gpio-leds";
+
+		led0 {
+			/* P6_0 */
+			gpios = <&pinctrl RZA2_PIN_ID(P6, 0) GPIO_ACTIVE_HIGH>;
+		};
+	};
diff --git a/include/dt-bindings/pinctrl/r7s9210-pinctrl.h b/include/dt-bindings/pinctrl/r7s9210-pinctrl.h
new file mode 100644
index 000000000000..39ac74ba520b
--- /dev/null
+++ b/include/dt-bindings/pinctrl/r7s9210-pinctrl.h
@@ -0,0 +1,47 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Defines macros and constants for Renesas RZ/A2 pin controller pin
+ * muxing functions.
+ */
+#ifndef __DT_BINDINGS_PINCTRL_RENESAS_RZA2_H
+#define __DT_BINDINGS_PINCTRL_RENESAS_RZA2_H
+
+#define RZA2_PINS_PER_PORT	8
+
+/* Port names as labeled in the Hardware Manual */
+#define P0 0
+#define P1 1
+#define P2 2
+#define P3 3
+#define P4 4
+#define P5 5
+#define P6 6
+#define P7 7
+#define P8 8
+#define P9 9
+#define PA 10
+#define PB 11
+#define PC 12
+#define PD 13
+#define PE 14
+#define PF 15
+#define PG 16
+#define PH 17
+/* No I */
+#define PJ 18
+#define PK 19
+#define PL 20
+#define PM 21
+
+/*
+ * Create the pin index from its bank and position numbers and store in
+ * the upper 8 bits the alternate function identifier
+ */
+#define RZA2_PINMUX(b, p, f)	((b) * RZA2_PINS_PER_PORT + (p) | (f << 16))
+
+/*
+ * Convert a port and pin label to its global pin index
+ */
+ #define RZA2_PIN_ID(port, pin)	((port) * RZA2_PINS_PER_PORT + (pin))
+
+#endif /* __DT_BINDINGS_PINCTRL_RENESAS_RZA2_H */
-- 
2.16.1

Re: [PATCH 2/2] dt-bindings: pinctrl: Add RZ/A2 pinctrl and GPIO

[Rob Herring]

On Fri, Oct 05, 2018 at 10:09:51AM -0500, Chris Brandt wrote:
> Add device tree binding documentation and header file for Renesas R7S9210
> (RZ/A2) SoCs.
> 
> Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
> ---
>  .../bindings/pinctrl/renesas,rza2-pinctrl.txt      | 76 ++++++++++++++++++++++
>  include/dt-bindings/pinctrl/r7s9210-pinctrl.h      | 47 +++++++++++++
>  2 files changed, 123 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pinctrl/renesas,rza2-pinctrl.txt
>  create mode 100644 include/dt-bindings/pinctrl/r7s9210-pinctrl.h
> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/renesas,rza2-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/renesas,rza2-pinctrl.txt
> new file mode 100644
> index 000000000000..5f338054f493
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pinctrl/renesas,rza2-pinctrl.txt
> @@ -0,0 +1,76 @@
> +Renesas RZ/A2 combined Pin and GPIO controller
> +
> +The Renesas SoCs of the RZ/A2 series feature a combined Pin and GPIO controller.
> +Pin multiplexing and GPIO configuration is performed on a per-pin basis.
> +Each port features up to 8 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: should be:
> +    - "renesas,r7s9210-pinctrl": for RZ/A2M
> +
> +  - reg
> +    address base and length of the memory area where the pin controller
> +    hardware is mapped to.
> +
> +Optional properties:
> +  - gpio-controller
> +    Include this in order to enable GPIO functionality. When included, both
> +    gpio_cells and gpio_ranges are then required.
> +  - #gpio-cells
> +    Must be 2
> +  - gpio-ranges
> +    Expresses the total number GPIO ports/pins in this SoC

Are these really optional? I guess in theory a board could use no GPIOs, 
but that seems unlikely. 

> +
> +
> +Example: Pin controller node for RZ/A2M SoC (r7s9210)
> +
> +	pinctrl: pin-controller@fcffe000 {
> +		compatible = "renesas,r7s9210-pinctrl";
> +		reg = <0xfcffe000 0x9D1>;
> +
> +		gpio-controller;
> +		#gpio-cells = <2>;
> +		gpio-ranges = <&pinctrl 0 0 176>;
> +	};
> +
> +Sub-nodes
> +---------
> +
> +The child nodes of the pin controller node describe a pin multiplexing
> +function or a GPIO controller alternatively.

But the parent is already a GPIO controller. This needs to be fully 
defined.

> +
> +- 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.
> +  The values for the pinmux properties are a combination of port name, pin
> +  number and the desired function index. Use the RZA2_PINMUX macro located
> +  in include/dt-bindings/pinctrl/r7s9210-pinctrl.h to easily define these.
> +  For assigning GPIO pins, use the macro RZA2_PIN_ID also in r7s9210-pinctrl.h
> +  to express the desired port pin.
> +
> +  Example: Board specific pins configuration
> +
> +	&pinctrl {
> +		/* Serial Console */
> +		scif4_pins: serial4 {
> +			pinmux = <RZA2_PINMUX(P9, 0, 4)>,	/* TxD4 */
> +				 <RZA2_PINMUX(P9, 1, 4)>;	/* RxD4 */
> +		};
> +	};
> +
> +  Example: Assigning a GPIO:
> +
> +	leds {
> +		status = "okay";
> +		compatible = "gpio-leds";
> +
> +		led0 {
> +			/* P6_0 */
> +			gpios = <&pinctrl RZA2_PIN_ID(P6, 0) GPIO_ACTIVE_HIGH>;
> +		};
> +	};

RE: [PATCH 2/2] dt-bindings: pinctrl: Add RZ/A2 pinctrl and GPIO

[Chris Brandt]

On Tuesday, October 16, 2018, Rob Herring wrote:
> > +Optional properties:
> > +  - gpio-controller
> > +    Include this in order to enable GPIO functionality. When included,
> both
> > +    gpio_cells and gpio_ranges are then required.
> > +  - #gpio-cells
> > +    Must be 2
> > +  - gpio-ranges
> > +    Expresses the total number GPIO ports/pins in this SoC
> 
> Are these really optional? I guess in theory a board could use no GPIOs,
> but that seems unlikely.

They are 'optional' in the sense that if you don't include them in the 
DT, the driver still loads (just without any GPIO, but pinctrl still 
works). So, I was just documenting that fact.

If you think I should just move these to required, let me know an I'm 
fine with that. (as in, DT documents HW, not software)


> > +Sub-nodes
> > +---------
> > +
> > +The child nodes of the pin controller node describe a pin multiplexing
> > +function or a GPIO controller alternatively.
> 
> But the parent is already a GPIO controller. This needs to be fully
> defined.

Now that I read this, I think my wording was off (I was borrowing text 
for other files).

How about this:

The child nodes of the pin controller designate pins to be used for
specific peripheral functions or as GPIO.


Chris

Re: [PATCH 2/2] dt-bindings: pinctrl: Add RZ/A2 pinctrl and GPIO

[Rob Herring]

On Tue, Oct 16, 2018 at 7:53 PM Chris Brandt <Chris.Brandt@renesas.com> wrote:
>
> On Tuesday, October 16, 2018, Rob Herring wrote:
> > > +Optional properties:
> > > +  - gpio-controller
> > > +    Include this in order to enable GPIO functionality. When included,
> > both
> > > +    gpio_cells and gpio_ranges are then required.
> > > +  - #gpio-cells
> > > +    Must be 2
> > > +  - gpio-ranges
> > > +    Expresses the total number GPIO ports/pins in this SoC
> >
> > Are these really optional? I guess in theory a board could use no GPIOs,
> > but that seems unlikely.
>
> They are 'optional' in the sense that if you don't include them in the
> DT, the driver still loads (just without any GPIO, but pinctrl still
> works). So, I was just documenting that fact.
>
> If you think I should just move these to required, let me know an I'm
> fine with that. (as in, DT documents HW, not software)

I do. There's no implicit requirement that the s/w has to support it.

> > > +Sub-nodes
> > > +---------
> > > +
> > > +The child nodes of the pin controller node describe a pin multiplexing
> > > +function or a GPIO controller alternatively.
> >
> > But the parent is already a GPIO controller. This needs to be fully
> > defined.
>
> Now that I read this, I think my wording was off (I was borrowing text
> for other files).
>
> How about this:
>
> The child nodes of the pin controller designate pins to be used for
> specific peripheral functions or as GPIO.

Sure.

Re: [PATCH 2/2] dt-bindings: pinctrl: Add RZ/A2 pinctrl and GPIO

[jacopo mondi]

[-- Attachment #1: Type: text/plain, Size: 4598 bytes --]

Hi Chris,

On Tue, Oct 16, 2018 at 05:47:00PM -0500, Rob Herring wrote:
> On Fri, Oct 05, 2018 at 10:09:51AM -0500, Chris Brandt wrote:
> > Add device tree binding documentation and header file for Renesas R7S9210
> > (RZ/A2) SoCs.
> >
> > Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
> > ---
> >  .../bindings/pinctrl/renesas,rza2-pinctrl.txt      | 76 ++++++++++++++++++++++
> >  include/dt-bindings/pinctrl/r7s9210-pinctrl.h      | 47 +++++++++++++
> >  2 files changed, 123 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/pinctrl/renesas,rza2-pinctrl.txt
> >  create mode 100644 include/dt-bindings/pinctrl/r7s9210-pinctrl.h
> >
> > diff --git a/Documentation/devicetree/bindings/pinctrl/renesas,rza2-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/renesas,rza2-pinctrl.txt
> > new file mode 100644
> > index 000000000000..5f338054f493
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pinctrl/renesas,rza2-pinctrl.txt
> > @@ -0,0 +1,76 @@
> > +Renesas RZ/A2 combined Pin and GPIO controller
> > +
> > +The Renesas SoCs of the RZ/A2 series feature a combined Pin and GPIO controller.
> > +Pin multiplexing and GPIO configuration is performed on a per-pin basis.
> > +Each port features up to 8 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: should be:
> > +    - "renesas,r7s9210-pinctrl": for RZ/A2M
> > +
> > +  - reg
> > +    address base and length of the memory area where the pin controller
> > +    hardware is mapped to.
> > +
> > +Optional properties:
> > +  - gpio-controller
> > +    Include this in order to enable GPIO functionality. When included, both
> > +    gpio_cells and gpio_ranges are then required.
> > +  - #gpio-cells
> > +    Must be 2
> > +  - gpio-ranges
> > +    Expresses the total number GPIO ports/pins in this SoC
>
> Are these really optional? I guess in theory a board could use no GPIOs,
> but that seems unlikely.

Here you define bindings that allows you to have only one
gpio-controller node for the whole system.
With RZ/A1 we have a gpio-controller sub-node for each port. It's
true though that you have a lot of ports and few pins per port, but
to refer to a gpio you have to index the gpio in the whole pin space
with RZA1_PIN_ID():

	gpios = <&pinctrl RZA2_PIN_ID(P6, 0) GPIO_ACTIVE_HIGH>;

While I think this is nicer:
        gpios = <&port6 0 GPIO_ACTIVE_HIGH>;

Having gpio-controller sub-nodes also allows you
to specify a 'ngpios' property for each port (or do all ports have 8
pins? If I read Table 51.1 right they don't..), and when RZ/A2x will
come and has different pins per port it's
easy for developers to identify the differences (but this
depends on the package too, so it's not that easy as I'm putting it
here probably)

What do you think?

Thanks
   j

> > +
> > +
> > +Example: Pin controller node for RZ/A2M SoC (r7s9210)
> > +
> > +	pinctrl: pin-controller@fcffe000 {
> > +		compatible = "renesas,r7s9210-pinctrl";
> > +		reg = <0xfcffe000 0x9D1>;
> > +
> > +		gpio-controller;
> > +		#gpio-cells = <2>;
> > +		gpio-ranges = <&pinctrl 0 0 176>;
> > +	};
> > +
> > +Sub-nodes
> > +---------
> > +
> > +The child nodes of the pin controller node describe a pin multiplexing
> > +function or a GPIO controller alternatively.
>
> But the parent is already a GPIO controller. This needs to be fully
> defined.
>
> > +
> > +- 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.
> > +  The values for the pinmux properties are a combination of port name, pin
> > +  number and the desired function index. Use the RZA2_PINMUX macro located
> > +  in include/dt-bindings/pinctrl/r7s9210-pinctrl.h to easily define these.
> > +  For assigning GPIO pins, use the macro RZA2_PIN_ID also in r7s9210-pinctrl.h
> > +  to express the desired port pin.
> > +
> > +  Example: Board specific pins configuration
> > +
> > +	&pinctrl {
> > +		/* Serial Console */
> > +		scif4_pins: serial4 {
> > +			pinmux = <RZA2_PINMUX(P9, 0, 4)>,	/* TxD4 */
> > +				 <RZA2_PINMUX(P9, 1, 4)>;	/* RxD4 */
> > +		};
> > +	};
> > +
> > +  Example: Assigning a GPIO:
> > +
> > +	leds {
> > +		status = "okay";
> > +		compatible = "gpio-leds";
> > +
> > +		led0 {
> > +			/* P6_0 */
> > +			gpios = <&pinctrl RZA2_PIN_ID(P6, 0) GPIO_ACTIVE_HIGH>;
> > +		};
> > +	};

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

RE: [PATCH 2/2] dt-bindings: pinctrl: Add RZ/A2 pinctrl and GPIO

[Chris Brandt]

Hi Jacopo,

On Thursday, October 18, 2018, jacopo mondi wrote:
> Here you define bindings that allows you to have only one
> gpio-controller node for the whole system.

Correct. Since DT describes HW, we do only have one gpio-controller for 
the entire chip. It is one piece of hardware, not many tiny pieces of 
hardware.

> With RZ/A1 we have a gpio-controller sub-node for each port.

As I started to write this driver, I think the RZ/A1 driver is a little 
bit strange. It is also one gpio-ctonroller HW. But, I'm not about to go
suggesting we change the RZ/A1 driver any time soon.


> It's
> true though that you have a lot of ports and few pins per port, but
> to refer to a gpio you have to index the gpio in the whole pin space
> with RZA1_PIN_ID():
> 
> 	gpios = <&pinctrl RZA2_PIN_ID(P6, 0) GPIO_ACTIVE_HIGH>;
> 
> While I think this is nicer:
>         gpios = <&port6 0 GPIO_ACTIVE_HIGH>;
 
Both seem just as easy to use.
The difference is I don't want lots and lots of device tree sub nodes 
that are not really needed.

Technically, the GPIO controller is a single piece of HW that you index 
into it what pin you want to control. The 'indexing' is basically 
registers and bits addresses. But, it is still one big controller (not many 
smaller controllers like channels of SPI, UART or I2C).


> Having gpio-controller sub-nodes also allows you
> to specify a 'ngpios' property for each port (or do all ports have 8
> pins? If I read Table 51.1 right they don't..),

The "controller" (actual HW) controls 8-pins per port. In the RZ/A2M, 
not all pins are hooked up to actual pin pads. And that also change with 
different packages like you mentioned.

> and when RZ/A2x will
> come and has different pins per port it's
> easy for developers to identify the differences (but this
> depends on the package too, so it's not that easy as I'm putting it
> here probably)

Yes, as you start to guess about different packages, an RZ/A2x, or even 
a RZ/A3, and all the different combinations, it gets complicated.

However, what will always be true is that this pin controller HW, no
matter what SoC it is in, will always have the ability to control 8 pins
per port. So you just have to specify how many ports it has, and then
that will be a correct description of the hardware.

Chris

Re: [PATCH 2/2] dt-bindings: pinctrl: Add RZ/A2 pinctrl and GPIO

[jacopo mondi]

[-- Attachment #1: Type: text/plain, Size: 5043 bytes --]

Hi Chris,

On Fri, Oct 05, 2018 at 10:09:51AM -0500, Chris Brandt wrote:
> Add device tree binding documentation and header file for Renesas R7S9210
> (RZ/A2) SoCs.
>
> Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
> ---
>  .../bindings/pinctrl/renesas,rza2-pinctrl.txt      | 76 ++++++++++++++++++++++
>  include/dt-bindings/pinctrl/r7s9210-pinctrl.h      | 47 +++++++++++++
>  2 files changed, 123 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pinctrl/renesas,rza2-pinctrl.txt
>  create mode 100644 include/dt-bindings/pinctrl/r7s9210-pinctrl.h
>
> diff --git a/Documentation/devicetree/bindings/pinctrl/renesas,rza2-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/renesas,rza2-pinctrl.txt
> new file mode 100644
> index 000000000000..5f338054f493
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pinctrl/renesas,rza2-pinctrl.txt
> @@ -0,0 +1,76 @@
> +Renesas RZ/A2 combined Pin and GPIO controller
> +
> +The Renesas SoCs of the RZ/A2 series feature a combined Pin and GPIO controller.
> +Pin multiplexing and GPIO configuration is performed on a per-pin basis.
> +Each port features up to 8 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: should be:
> +    - "renesas,r7s9210-pinctrl": for RZ/A2M
> +
> +  - reg
> +    address base and length of the memory area where the pin controller
> +    hardware is mapped to.
> +
> +Optional properties:
> +  - gpio-controller
> +    Include this in order to enable GPIO functionality. When included, both
> +    gpio_cells and gpio_ranges are then required.
> +  - #gpio-cells
> +    Must be 2
> +  - gpio-ranges
> +    Expresses the total number GPIO ports/pins in this SoC

I have some concerns here, I'll reply to Rob on this
> +
> +
> +Example: Pin controller node for RZ/A2M SoC (r7s9210)
> +
> +	pinctrl: pin-controller@fcffe000 {
> +		compatible = "renesas,r7s9210-pinctrl";
> +		reg = <0xfcffe000 0x9D1>;
> +
> +		gpio-controller;
> +		#gpio-cells = <2>;
> +		gpio-ranges = <&pinctrl 0 0 176>;
> +	};
> +
> +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.
> +  The values for the pinmux properties are a combination of port name, pin
> +  number and the desired function index. Use the RZA2_PINMUX macro located
> +  in include/dt-bindings/pinctrl/r7s9210-pinctrl.h to easily define these.
> +  For assigning GPIO pins, use the macro RZA2_PIN_ID also in r7s9210-pinctrl.h
> +  to express the desired port pin.
> +
> +  Example: Board specific pins configuration
> +
> +	&pinctrl {
> +		/* Serial Console */
> +		scif4_pins: serial4 {
> +			pinmux = <RZA2_PINMUX(P9, 0, 4)>,	/* TxD4 */
> +				 <RZA2_PINMUX(P9, 1, 4)>;	/* RxD4 */
> +		};
> +	};
> +
> +  Example: Assigning a GPIO:
> +
> +	leds {
> +		status = "okay";
> +		compatible = "gpio-leds";
> +
> +		led0 {
> +			/* P6_0 */
> +			gpios = <&pinctrl RZA2_PIN_ID(P6, 0) GPIO_ACTIVE_HIGH>;
> +		};
> +	};

I think you should list the required properties ('pinmux') and the pin
configuration flags the hardware supports. From a quick look to the
manual I only see a configurable drive strength, but I might have
missed something.

> diff --git a/include/dt-bindings/pinctrl/r7s9210-pinctrl.h b/include/dt-bindings/pinctrl/r7s9210-pinctrl.h
> new file mode 100644
> index 000000000000..39ac74ba520b
> --- /dev/null
> +++ b/include/dt-bindings/pinctrl/r7s9210-pinctrl.h
> @@ -0,0 +1,47 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Defines macros and constants for Renesas RZ/A2 pin controller pin
> + * muxing functions.
> + */
> +#ifndef __DT_BINDINGS_PINCTRL_RENESAS_RZA2_H
> +#define __DT_BINDINGS_PINCTRL_RENESAS_RZA2_H
> +
> +#define RZA2_PINS_PER_PORT	8
> +
> +/* Port names as labeled in the Hardware Manual */
> +#define P0 0
> +#define P1 1
> +#define P2 2
> +#define P3 3
> +#define P4 4
> +#define P5 5
> +#define P6 6
> +#define P7 7
> +#define P8 8
> +#define P9 9
> +#define PA 10
> +#define PB 11
> +#define PC 12
> +#define PD 13
> +#define PE 14
> +#define PF 15
> +#define PG 16
> +#define PH 17
> +/* No I */
> +#define PJ 18
> +#define PK 19
> +#define PL 20
> +#define PM 21
> +
> +/*
> + * Create the pin index from its bank and position numbers and store in
> + * the upper 8 bits the alternate function identifier
> + */
> +#define RZA2_PINMUX(b, p, f)	((b) * RZA2_PINS_PER_PORT + (p) | (f << 16))
> +
> +/*
> + * Convert a port and pin label to its global pin index
> + */
> + #define RZA2_PIN_ID(port, pin)	((port) * RZA2_PINS_PER_PORT + (pin))

Why not just RZA2_PIN() :) ?

Thanks
   j

> +
> +#endif /* __DT_BINDINGS_PINCTRL_RENESAS_RZA2_H */
> --
> 2.16.1
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

RE: [PATCH 2/2] dt-bindings: pinctrl: Add RZ/A2 pinctrl and GPIO

[Chris Brandt]

Hi Jacopo,

On Thursday, October 18, 2018, jacopo mondi wrote:
> > +  Example: Assigning a GPIO:
> > +
> > +	leds {
> > +		status = "okay";
> > +		compatible = "gpio-leds";
> > +
> > +		led0 {
> > +			/* P6_0 */
> > +			gpios = <&pinctrl RZA2_PIN_ID(P6, 0) GPIO_ACTIVE_HIGH>;
> > +		};
> > +	};
> 
> I think you should list the required properties ('pinmux') and the pin
> configuration flags the hardware supports.

OK, I can list pinmux.

> From a quick look to the
> manual I only see a configurable drive strength, but I might have
> missed something.

Yes, some pin can have different drive strength, but not all pins. Also,
some part are not clear about the drive strength.
So for this initial release, I do not want to be trying to describe 
hardware that I don't completely understand yet.

> > +/*
> > + * Convert a port and pin label to its global pin index
> > + */
> > + #define RZA2_PIN_ID(port, pin)	((port) * RZA2_PINS_PER_PORT + (pin))
> 
> Why not just RZA2_PIN() :) ?

OK.
I don't mind a shorter name.


Chris