[PATCH 0/3] Renesas RZ PFC and GPIO driver

[PATCH 0/3] Renesas RZ PFC and GPIO driver

[Jacopo Mondi]

Hello,
   these patches are the result of a squash and forward porting of Magnus'
PFC and GPIO driver from
git://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-drivers.git tree
genmai-gpio-and-pfc branch to kernel v4.10

The PFC and GPIO driver have been forward ported to v4.10-rc1 with minor
fixes and applied on top of renesas-driver master branch.

As I cannot locally verify most of the peripherals functionalities enabled
by the creation of PFC and GPIO driver for RZ SoC series, testing performed
by patches original authors has to be trusted.

Some possible fixes and functional expansion for the PFC driver will follow in
an RFC series to be applied on top of this one.

Thanks
   j


Magnus Damm (3):
  pinctrl: sh-pfc: Add r7s72100 PFC driver
  gpio: gpio-rz: GPIO driver for Renesas RZ series
  arm: dts: r7s72100: Add peripherals nodes

 .../bindings/pinctrl/renesas,pfc-pinctrl.txt       |   1 +
 arch/arm/boot/dts/r7s72100-genmai.dts              |  51 ++
 arch/arm/boot/dts/r7s72100.dtsi                    | 151 ++++++
 drivers/gpio/Kconfig                               |   6 +
 drivers/gpio/Makefile                              |   1 +
 drivers/gpio/gpio-rz.c                             | 212 +++++++++
 drivers/pinctrl/sh-pfc/Kconfig                     |   5 +
 drivers/pinctrl/sh-pfc/Makefile                    |   1 +
 drivers/pinctrl/sh-pfc/core.c                      |   9 +
 drivers/pinctrl/sh-pfc/pfc-r7s72100.c              | 529 +++++++++++++++++++++
 drivers/pinctrl/sh-pfc/sh_pfc.h                    |   9 +-
 11 files changed, 972 insertions(+), 3 deletions(-)
 create mode 100644 drivers/gpio/gpio-rz.c
 create mode 100644 drivers/pinctrl/sh-pfc/pfc-r7s72100.c

-- 
2.7.4

[PATCH 1/3] pinctrl: sh-pfc: Add r7s72100 PFC driver

[Jacopo Mondi]

From: Magnus Damm <damm@opensource.se>

Squash commits in Geert's renesas-driver/genmai-gpio-and-pfc branch that
add support for r7s72100 PFC.
This squash combines commits for Magnus' original driver and minor fixes
to forward-port it to a more recent kernel (v4.10)

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

-----------------------------------------------------------------------

pinctrl: sh-pfc: Rework _GP_GPIO, introduce _GP_GPIO32

Rework _GP_GPIO to take banksize as argument, introduce
_GP_GPIO32 for SoCs with 32-bit GPIO banks.

Signed-off-by: Magnus Damm <damm@opensource.se>

pinctrl: sh-pfc: r7s72100 base support

Add r7s72100 PINCTRL support via sh-pfc V2. At this point the code
provides enough support to allow use together with the gpio-rz
driver. Incremental per-device patches will in the future be
submitted on top of this base patch to allow PINCTRL enablement
for each individual device.

Signed-off-by: Magnus Damm <damm@opensource.se>

pinctrl: sh-pfc: r7s72100 single pin macros

Introduce macros that allow description of one pin per line.
Compared to the other ways of doing this, using this style
we can compresses the description of each pin from 9 to 1 line.

Signed-off-by: Magnus Damm <damm@opensource.se>

pinctrl: sh-pfc: r7s72100 SCIF support

Add support for SCIF functions SCK, TXD, RXD, CTS and RTS to the
r7s72100 PINCTRL code. There are two possible pins that can be used
for TXD (Port 3 Pin 0 Function 6 and Port 3 Pin 1 Function 4) and
because of that are pins broken out into separate functions.

Signed-off-by: Magnus Damm <damm@opensource.se>

[REBASED] pinctrl: sh-pfc: r7s72100 binding docs

Add compatible string for r7s72100.

Signed-off-by: Magnus Damm <damm@opensource.se>

pinctrl: r7s72100: add riic groups

Tested RIIC2 on a genmai board. Other riic groups are untested but seem
trivial enough to be added.

Signed-off-by: Wolfram Sang <wsa@sang-engineering.com>
Acked-by: Magnus Damm <damm@opensource.se>

[PATCH] HACK: make magnus new driver work

On Wed, Dec 18, 2013 at 10:31:57PM +0100, Wolfram Sang wrote:
> From: Wolfram Sang <wsa@sang-engineering.com>
>
> Tested RIIC2 on a genmai board. Other riic groups are untested but seem
> trivial enough to be added.
>
> Signed-off-by: Wolfram Sang <wsa@sang-engineering.com>
> Acked-by: Magnus Damm <damm@opensource.se>
> ---
>
> V2: keep sorting alphabetical
>
> Note: With the current PFC driver as posted by Magnus, it needs another patch
> to work. Yet, I think this is a seperate PFC issue which needs to be sorted out
> seperately and shouldn't affect these declarations. I'll add the needed patch
> as a response to this mail.

From: Wolfram Sang <wsa@the-dreams.de>
Subject: [PATCH] HACK: make magnus new driver work

Signed-off-by: Wolfram Sang <wsa@the-dreams.de>

pinctl: sh-pfc: r7s72100: Ethernet support

Signed-off-by: Simon Horman <horms+renesas@verge.net.au>

[WIP] eth pfc comments from Laurent

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

pinctrl: sh-pfc: r7s72100: Add RSPI support

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

pinctrl: sh-pfc: Port to v4.10-rc1

Port of Magnus' driver from v3.18 to v4.10-rc1

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 .../bindings/pinctrl/renesas,pfc-pinctrl.txt       |   1 +
 drivers/pinctrl/sh-pfc/Kconfig                     |   5 +
 drivers/pinctrl/sh-pfc/Makefile                    |   1 +
 drivers/pinctrl/sh-pfc/core.c                      |   9 +
 drivers/pinctrl/sh-pfc/pfc-r7s72100.c              | 529 +++++++++++++++++++++
 drivers/pinctrl/sh-pfc/sh_pfc.h                    |   9 +-
 6 files changed, 551 insertions(+), 3 deletions(-)
 create mode 100644 drivers/pinctrl/sh-pfc/pfc-r7s72100.c

diff --git a/Documentation/devicetree/bindings/pinctrl/renesas,pfc-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/renesas,pfc-pinctrl.txt
index 13df949..bd6629b 100644
--- a/Documentation/devicetree/bindings/pinctrl/renesas,pfc-pinctrl.txt
+++ b/Documentation/devicetree/bindings/pinctrl/renesas,pfc-pinctrl.txt
@@ -11,6 +11,7 @@ Required Properties:
 
   - compatible: should be one of the following.
     - "renesas,pfc-emev2": for EMEV2 (EMMA Mobile EV2) compatible pin-controller.
+    - "renesas,pfc-r7s72100": for R7S72100 (RZ/A1H) compatible pin-controller.
     - "renesas,pfc-r8a73a4": for R8A73A4 (R-Mobile APE6) compatible pin-controller.
     - "renesas,pfc-r8a7740": for R8A7740 (R-Mobile A1) compatible pin-controller.
     - "renesas,pfc-r8a7778": for R8A7778 (R-Mobile M1) compatible pin-controller.
diff --git a/drivers/pinctrl/sh-pfc/Kconfig b/drivers/pinctrl/sh-pfc/Kconfig
index 07eca54..e4ccae2 100644
--- a/drivers/pinctrl/sh-pfc/Kconfig
+++ b/drivers/pinctrl/sh-pfc/Kconfig
@@ -24,6 +24,11 @@ config PINCTRL_PFC_EMEV2
 	depends on ARCH_EMEV2
 	select PINCTRL_SH_PFC
 
+config PINCTRL_PFC_R7S72100
+	def_bool y
+	depends on ARCH_R7S72100
+	select PINCTRL_SH_PFC
+
 config PINCTRL_PFC_R8A73A4
 	def_bool y
 	depends on ARCH_R8A73A4
diff --git a/drivers/pinctrl/sh-pfc/Makefile b/drivers/pinctrl/sh-pfc/Makefile
index 2dda8c6..b953392 100644
--- a/drivers/pinctrl/sh-pfc/Makefile
+++ b/drivers/pinctrl/sh-pfc/Makefile
@@ -1,6 +1,7 @@
 obj-$(CONFIG_PINCTRL_SH_PFC)	+= core.o pinctrl.o
 obj-$(CONFIG_PINCTRL_SH_PFC_GPIO)	+= gpio.o
 obj-$(CONFIG_PINCTRL_PFC_EMEV2)	+= pfc-emev2.o
+obj-$(CONFIG_PINCTRL_PFC_R7S72100)	+= pfc-r7s72100.o
 obj-$(CONFIG_PINCTRL_PFC_R8A73A4)	+= pfc-r8a73a4.o
 obj-$(CONFIG_PINCTRL_PFC_R8A7740)	+= pfc-r8a7740.o
 obj-$(CONFIG_PINCTRL_PFC_R8A7778)	+= pfc-r8a7778.o
diff --git a/drivers/pinctrl/sh-pfc/core.c b/drivers/pinctrl/sh-pfc/core.c
index cf80ce1..bb6bab1 100644
--- a/drivers/pinctrl/sh-pfc/core.c
+++ b/drivers/pinctrl/sh-pfc/core.c
@@ -473,6 +473,12 @@ static const struct of_device_id sh_pfc_of_table[] = {
 		.data = &emev2_pinmux_info,
 	},
 #endif
+#ifdef CONFIG_PINCTRL_PFC_R7S72100
+	{
+		.compatible = "renesas,pfc-r7s72100",
+		.data = &r7s72100_pinmux_info,
+	},
+#endif
 #ifdef CONFIG_PINCTRL_PFC_R8A73A4
 	{
 		.compatible = "renesas,pfc-r8a73a4",
@@ -626,6 +632,9 @@ static int sh_pfc_probe(struct platform_device *pdev)
 }
 
 static const struct platform_device_id sh_pfc_id_table[] = {
+#ifdef CONFIG_PINCTRL_PFC_R7S72100
+	{ "pfc-r7s72100", (kernel_ulong_t)&r7s72100_pinmux_info },
+#endif
 #ifdef CONFIG_PINCTRL_PFC_SH7203
 	{ "pfc-sh7203", (kernel_ulong_t)&sh7203_pinmux_info },
 #endif
diff --git a/drivers/pinctrl/sh-pfc/pfc-r7s72100.c b/drivers/pinctrl/sh-pfc/pfc-r7s72100.c
new file mode 100644
index 0000000..72e1dff
--- /dev/null
+++ b/drivers/pinctrl/sh-pfc/pfc-r7s72100.c
@@ -0,0 +1,529 @@
+/*
+ * R7S72100 processor support
+ *
+ * Copyright (C) 2013  Renesas Electronics Corporation
+ * Copyright (C) 2013  Magnus Damm
+ * Copyright (C) 2012  Renesas Solutions Corp.
+ * Copyright (C) 2012  Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; version 2 of the
+ * License.
+ */
+
+#include <linux/kernel.h>
+
+#include "core.h"
+#include "sh_pfc.h"
+
+#define CPU_ALL_PORT(fn, sfx)						\
+	PORT_GP_16(0, fn, sfx), PORT_GP_16(1, fn, sfx),			\
+	PORT_GP_16(2, fn, sfx), PORT_GP_16(3, fn, sfx),			\
+	PORT_GP_16(4, fn, sfx), PORT_GP_16(5, fn, sfx),			\
+	PORT_GP_16(6, fn, sfx), PORT_GP_16(7, fn, sfx),			\
+	PORT_GP_16(8, fn, sfx), PORT_GP_16(9, fn, sfx),			\
+	PORT_GP_16(10, fn, sfx), PORT_GP_16(11, fn, sfx),			\
+	PORT_GP_16(12, fn, sfx)
+
+enum {
+	PINMUX_RESERVED = 0,
+
+	PINMUX_DATA_BEGIN,
+	GP_ALL(DATA),
+	PINMUX_DATA_END,
+
+	PINMUX_FUNCTION_BEGIN,
+	GP_ALL(PMC_0), GP_ALL(PMC_1),
+	GP_ALL(PFC_0), GP_ALL(PFC_1),
+	GP_ALL(PFCE_0), GP_ALL(PFCE_1),
+	GP_ALL(PFCAE_0), GP_ALL(PFCAE_1),
+	GP_ALL(PIBC_0), GP_ALL(PIBC_1),
+	GP_ALL(PBDC_0), GP_ALL(PBDC_1),
+	GP_ALL(PIPC_0), GP_ALL(PIPC_1),
+	PINMUX_FUNCTION_END,
+
+	PINMUX_MARK_BEGIN,
+	GP_ALL(MARK_FN1), GP_ALL(MARK_FN2), GP_ALL(MARK_FN3), GP_ALL(MARK_FN4),
+	GP_ALL(MARK_FN5), GP_ALL(MARK_FN6), GP_ALL(MARK_FN7), GP_ALL(MARK_FN8),
+	PINMUX_MARK_END,
+};
+
+#define _P_ALL(n) CPU_ALL_PORT(n, unused)
+
+#define _P_GPIO(bank, _pin, _name, sfx, cfg) \
+	_GP_GPIO(16, bank, _pin, _name, sfx, cfg)
+
+#define _P_DATA(bank, pin, name, sfx, cfg)					\
+	PINMUX_DATA(name##_DATA, name##_PMC_0,		\
+		    name##_PIBC_1, name##_PBDC_1)
+
+#define _P_FN(n, fn, pfcae, pfce, pfc)					\
+	PINMUX_DATA(n##_MARK_FN##fn, n##_PMC_1,		\
+		    n##_PFCAE_##pfcae, n##_PFCE_##pfce, n##_PFC_##pfc)
+
+#define _P_MARK_FN1(bank, pin, name, sfx, cfg) _P_FN(name, 1, 0, 0, 0)
+#define _P_MARK_FN2(bank, pin, name, sfx, cfg) _P_FN(name, 2, 0, 0, 1)
+#define _P_MARK_FN3(bank, pin, name, sfx, cfg) _P_FN(name, 3, 0, 1, 0)
+#define _P_MARK_FN4(bank, pin, name, sfx, cfg) _P_FN(name, 4, 0, 1, 1)
+#define _P_MARK_FN5(bank, pin, name, sfx, cfg) _P_FN(name, 5, 1, 0, 0)
+#define _P_MARK_FN6(bank, pin, name, sfx, cfg) _P_FN(name, 6, 1, 0, 1)
+#define _P_MARK_FN7(bank, pin, name, sfx, cfg) _P_FN(name, 7, 1, 1, 0)
+#define _P_MARK_FN8(bank, pin, name, sfx, cfg) _P_FN(name, 8, 1, 1, 1)
+
+static const u16 pinmux_data[] = {
+	_P_ALL(_P_DATA), /* PINMUX_DATA(P_M_N_DATA, P_M_N_PMC_0)... */
+	_P_ALL(_P_MARK_FN1), _P_ALL(_P_MARK_FN2),
+	_P_ALL(_P_MARK_FN3), _P_ALL(_P_MARK_FN4),
+	_P_ALL(_P_MARK_FN5), _P_ALL(_P_MARK_FN6),
+	_P_ALL(_P_MARK_FN7), _P_ALL(_P_MARK_FN8),
+};
+
+static struct sh_pfc_pin pinmux_pins[] = {
+	_P_ALL(_P_GPIO),
+};
+
+#define RZ_PORT_PIN(bank, pin) (((bank) * 16) + (pin))
+
+#define __RZ_STR(pfx, hw, bank, pin, sfx)		\
+	pfx##_##hw##_p##bank##_##pin####sfx
+
+#define RZ_PIN_AND_MUX(pfx, hw, bank, pin, fn)				\
+static const unsigned int __RZ_STR(pfx, hw, bank, pin, _pins)[] = {	\
+	RZ_PORT_PIN(bank, pin),						\
+};									\
+static const unsigned int __RZ_STR(pfx, hw, bank, pin, _mux)[] = {	\
+	GP_##bank##_##pin##_MARK_FN##fn,					\
+};
+
+#define RZ_PMX_GROUP(pfx, hw, bank, pin, fn) \
+	SH_PFC_PIN_GROUP(pfx##_##hw##_p##bank##_##pin),
+
+#define __RZ_GROUPS(x) #x
+
+#define RZ_GROUPS(pfx, hw, bank, pin, fn) \
+	__RZ_GROUPS(pfx##_##hw##_p##bank##_##pin),
+
+#define RIIC0(fn)			\
+	fn(riic0, scl, 1, 0, 1)		\
+	fn(riic0, sda, 1, 1, 1)
+
+#define RIIC1(fn)			\
+	fn(riic1, scl, 1, 2, 1)		\
+	fn(riic1, sda, 1, 3, 1)
+
+#define RIIC2(fn)			\
+	fn(riic2, scl, 1, 4, 1)		\
+	fn(riic2, sda, 1, 5, 1)
+
+#define RIIC3(fn)			\
+	fn(riic3, scl, 1, 6, 1)		\
+	fn(riic3, sda, 1, 7, 1)
+
+RIIC0(RZ_PIN_AND_MUX)
+RIIC1(RZ_PIN_AND_MUX)
+RIIC2(RZ_PIN_AND_MUX)
+RIIC3(RZ_PIN_AND_MUX)
+
+#define RSPI0(fn)			\
+	fn(rspi0, rspck,  2, 12, 2)	\
+	fn(rspi0, ssl0,   2, 13, 2)	\
+	fn(rspi0, mosi,   2, 14, 2)	\
+	fn(rspi0, miso,   2, 15, 2)	\
+	fn(rspi0, rspck,  7, 15, 2)	\
+	fn(rspi0, ssl0,   8,  0, 2)	\
+	fn(rspi0, mosi,   8,  1, 2)	\
+	fn(rspi0, miso,   8,  2, 2)	\
+	fn(rspi0, rspck, 10, 12, 4)	\
+	fn(rspi0, ssl0,  10, 13, 4)	\
+	fn(rspi0, mosi,  10, 14, 4)	\
+	fn(rspi0, miso,  10, 15, 4)	\
+
+#define RSPI1(fn)			\
+	fn(rspi1, rspck,  4,  4, 2)	\
+	fn(rspi1, ssl0,   4,  5, 2)	\
+	fn(rspi1, mosi,   4,  6, 2)	\
+	fn(rspi1, miso,   4,  7, 2)	\
+	fn(rspi1, rspck,  6,  4, 7)	\
+	fn(rspi1, ssl0,   6,  5, 7)	\
+	fn(rspi1, mosi,   6,  6, 7)	\
+	fn(rspi1, miso,   6,  7, 7)	\
+	fn(rspi1, rspck, 11, 12, 2)	\
+	fn(rspi1, ssl0,  11, 13, 2)	\
+	fn(rspi1, mosi,  11, 14, 2)	\
+	fn(rspi1, miso,  11, 15, 2)	\
+
+#define RSPI2(fn)			\
+	fn(rspi2, rspck,  8,  3, 3)	\
+	fn(rspi2, ssl0,   8,  4, 3)	\
+	fn(rspi2, mosi,   8,  5, 3)	\
+	fn(rspi2, miso,   8,  6, 3)	\
+	fn(rspi2, rspck,  8, 14, 5)	\
+	fn(rspi2, ssl0,   8, 15, 5)	\
+	fn(rspi2, mosi,   9,  0, 5)	\
+	fn(rspi2, miso,   9,  1, 5)	\
+
+#define RSPI3(fn)			\
+	fn(rspi3, rspck,  3,  0, 8)	\
+	fn(rspi3, ssl0,   3,  1, 8)	\
+	fn(rspi3, mosi,   3,  2, 8)	\
+	fn(rspi3, miso,   3,  3, 8)	\
+	fn(rspi3, rspck,  5,  0, 8)	\
+	fn(rspi3, ssl0,   5,  1, 8)	\
+	fn(rspi3, mosi,   5,  2, 8)	\
+	fn(rspi3, miso,   5,  3, 8)	\
+
+#define RSPI4(fn)			\
+	fn(rspi4, rspck,  2,  8, 8)	\
+	fn(rspi4, ssl0,   2,  9, 8)	\
+	fn(rspi4, mosi,   2, 10, 8)	\
+	fn(rspi4, miso,   2, 11, 8)	\
+	fn(rspi4, rspck,  4,  0, 7)	\
+	fn(rspi4, ssl0,   4,  1, 7)	\
+	fn(rspi4, mosi,   4,  2, 7)	\
+	fn(rspi4, miso,   4,  3, 7)	\
+
+RSPI0(RZ_PIN_AND_MUX)
+RSPI1(RZ_PIN_AND_MUX)
+RSPI2(RZ_PIN_AND_MUX)
+RSPI3(RZ_PIN_AND_MUX)
+RSPI4(RZ_PIN_AND_MUX)
+
+#define SCIF0(fn)			\
+	fn(scif0, clk, 2, 13, 6)	\
+	fn(scif0, txd, 2, 14, 6)	\
+	fn(scif0, rxd, 2, 15, 6)	\
+	fn(scif0, clk, 4, 8, 7)		\
+	fn(scif0, txd, 4, 9, 7)		\
+	fn(scif0, rxd, 4, 10, 7)	\
+	fn(scif0, clk, 6, 8, 5)		\
+	fn(scif0, txd, 6, 9, 5)		\
+	fn(scif0, rxd, 6, 10, 5)
+
+#define SCIF1(fn)			\
+	fn(scif1, cts, 2, 3, 6)		\
+	fn(scif1, clk, 2, 4, 6)		\
+	fn(scif1, txd, 2, 5, 6)		\
+	fn(scif1, rxd, 2, 6, 6)		\
+	fn(scif1, rts, 2, 7, 6)		\
+	fn(scif1, clk, 4, 11, 7)	\
+	fn(scif1, txd, 4, 12, 7)	\
+	fn(scif1, rxd, 4, 13, 7)	\
+	fn(scif1, clk, 6, 11, 5)	\
+	fn(scif1, txd, 6, 12, 5)	\
+	fn(scif1, rxd, 6, 13, 5)	\
+	fn(scif1, clk, 9, 2, 4)		\
+	fn(scif1, txd, 9, 3, 4)		\
+	fn(scif1, rxd, 9, 4, 4)		\
+	fn(scif1, cts, 9, 5, 4)		\
+	fn(scif1, rts, 9, 6, 4)
+
+#define SCIF2(fn)			\
+	fn(scif2, clk, 3, 0, 4)		\
+	fn(scif2, txd, 3, 1, 4)		\
+	fn(scif2, rxd, 3, 2, 4)		\
+	fn(scif2, txd, 3, 0, 6)		\
+	fn(scif2, clk, 4, 1, 5)		\
+	fn(scif2, txd, 4, 2, 5)		\
+	fn(scif2, rxd, 4, 3, 5)		\
+	fn(scif2, txd, 4, 14, 7)	\
+	fn(scif2, rxd, 4, 15, 7)	\
+	fn(scif2, txd, 6, 2, 7)		\
+	fn(scif2, rxd, 6, 3, 7)		\
+	fn(scif2, clk, 8, 3, 7)		\
+	fn(scif2, rxd, 8, 4, 7)		\
+	fn(scif2, txd, 8, 6, 7)
+
+#define SCIF3(fn)			\
+	fn(scif3, clk, 3, 4, 7)		\
+	fn(scif3, txd, 3, 5, 7)		\
+	fn(scif3, rxd, 3, 6, 7)		\
+	fn(scif3, clk, 5, 2, 5)		\
+	fn(scif3, txd, 5, 3, 5)		\
+	fn(scif3, rxd, 5, 4, 5)		\
+	fn(scif3, rxd, 6, 0, 7)		\
+	fn(scif3, txd, 6, 1, 7)		\
+	fn(scif3, txd, 8, 8, 7)		\
+	fn(scif3, rxd, 8, 9, 7)
+
+#define SCIF4(fn)			\
+	fn(scif4, txd, 5, 0, 5)		\
+	fn(scif4, rxd, 5, 1, 5)		\
+	fn(scif4, clk, 7, 0, 4)		\
+	fn(scif4, txd, 7, 1, 4)		\
+	fn(scif4, rxd, 7, 2, 4)		\
+	fn(scif4, txd, 8, 14, 7)	\
+	fn(scif4, rxd, 8, 15, 7)
+
+#define SCIF5(fn)			\
+	fn(scif5, cts, 6, 3, 5)		\
+	fn(scif5, rts, 6, 4, 5)		\
+	fn(scif5, clk, 6, 5, 5)		\
+	fn(scif5, txd, 6, 6, 5)		\
+	fn(scif5, rxd, 6, 7, 5)		\
+	fn(scif5, cts, 7, 15, 4)	\
+	fn(scif5, clk, 8, 0, 4)		\
+	fn(scif5, txd, 8, 1, 4)		\
+	fn(scif5, rxd, 8, 2, 4)		\
+	fn(scif5, rts, 8, 3, 4)		\
+	fn(scif5, rxd, 8, 11, 5)	\
+	fn(scif5, clk, 8, 12, 5)	\
+	fn(scif5, txd, 8, 13, 5)	\
+	fn(scif5, cts, 11, 7, 3)	\
+	fn(scif5, rts, 11, 8, 3)	\
+	fn(scif5, clk, 11, 9, 3)	\
+	fn(scif5, txd, 11, 10, 3)	\
+	fn(scif5, rxd, 11, 11, 3)
+
+#define SCIF6(fn)			\
+	fn(scif6, txd, 5, 6, 5)		\
+	fn(scif6, rxd, 5, 7, 5)		\
+	fn(scif6, clk, 6, 13, 4)	\
+	fn(scif6, txd, 6, 14, 4)	\
+	fn(scif6, rxd, 6, 15, 4)	\
+	fn(scif6, clk, 11, 0, 4)	\
+	fn(scif6, txd, 11, 1, 4)	\
+	fn(scif6, rxd, 11, 2, 4)
+
+#define SCIF7(fn)			\
+	fn(scif7, clk, 7, 3, 4)		\
+	fn(scif7, txd, 7, 4, 4)		\
+	fn(scif7, rxd, 7, 5, 4)		\
+	fn(scif7, cts, 7, 6, 4)		\
+	fn(scif7, rts, 7, 7, 4)
+
+SCIF0(RZ_PIN_AND_MUX)
+SCIF1(RZ_PIN_AND_MUX)
+SCIF2(RZ_PIN_AND_MUX)
+SCIF3(RZ_PIN_AND_MUX)
+SCIF4(RZ_PIN_AND_MUX)
+SCIF5(RZ_PIN_AND_MUX)
+SCIF6(RZ_PIN_AND_MUX)
+SCIF7(RZ_PIN_AND_MUX)
+
+#define ETHERNET(fn)			\
+	fn(ethernet, col,    1,  3, 3)		\
+	fn(ethernet, col,    1, 14, 4)		\
+	fn(ethernet, int,    1, 15, 1)		\
+	fn(ethernet, txclk,  2,  0, 2)		\
+	fn(ethernet, txer,   2,  1, 2)		\
+	fn(ethernet, txen,   2,  2, 2)		\
+	fn(ethernet, txcrs,  2,  3, 2)		\
+	fn(ethernet, txd0,   2,  4, 2)		\
+	fn(ethernet, txd1,   2,  5, 2)		\
+	fn(ethernet, txd2,   2,  6, 2)		\
+	fn(ethernet, txd3,   2,  7, 2)		\
+	fn(ethernet, rxd0,   2,  8, 2)		\
+	fn(ethernet, rxd1,   2,  9, 2)		\
+	fn(ethernet, rxd2,   2, 10, 2)		\
+	fn(ethernet, rxd3,   2, 11, 2)		\
+	fn(ethernet, txclk,  3,  0, 2)		\
+	fn(ethernet, txer,   3,  1, 2)		\
+	fn(ethernet, txen,   3,  2, 2)		\
+	fn(ethernet, mdio,   3,  3, 2)		\
+	fn(ethernet, rxclk,  3,  4, 2)		\
+	fn(ethernet, rxer,   3,  5, 2)		\
+	fn(ethernet, rxdv,   3,  6, 2)		\
+	fn(ethernet, mdc,    5,  9, 2)		\
+	fn(ethernet, mdc,    7,  0, 3)		\
+	fn(ethernet, txclk,  7,  1, 3)		\
+	fn(ethernet, txer,   7,  2, 3)		\
+	fn(ethernet, txen,   7,  3, 3)		\
+	fn(ethernet, txd0,   7,  4, 3)		\
+	fn(ethernet, txd1,   7,  5, 3)		\
+	fn(ethernet, txd2,   7,  6, 3)		\
+	fn(ethernet, txd3,   7,  7, 3)		\
+	fn(ethernet, rxd0,   7,  9, 3)		\
+	fn(ethernet, rxd1,   7, 10, 3)		\
+	fn(ethernet, rxd2,   7, 11, 2)		\
+	fn(ethernet, rxd3,   7, 12, 3)		\
+	fn(ethernet, mdio,   7, 13, 3)		\
+	fn(ethernet, crs,    7, 14, 3)		\
+	fn(ethernet, rxclk,  7, 15, 3)		\
+	fn(ethernet, rxer,   8,  0, 3)		\
+	fn(ethernet, rxd,    8,  1, 3)		\
+	fn(ethernet, col,    8,  7, 5)		\
+	fn(ethernet, txclk, 10,  0, 4)		\
+	fn(ethernet, txer,  10,  1, 4)		\
+	fn(ethernet, txen,  10,  2, 4)		\
+	fn(ethernet, crs,   10,  3, 4)		\
+	fn(ethernet, txd0,  10,  4, 4)		\
+	fn(ethernet, txd1,  10,  5, 4)		\
+	fn(ethernet, txd2,  10,  6, 4)		\
+	fn(ethernet, txd3,  10,  7, 4)		\
+	fn(ethernet, txd0,  10,  8, 4)		\
+	fn(ethernet, txd1,  10,  9, 4)		\
+	fn(ethernet, txd2,  10, 10, 4)		\
+	fn(ethernet, txd3,  10, 11, 4)		\
+
+ETHERNET(RZ_PIN_AND_MUX)
+
+static const struct sh_pfc_pin_group pinmux_groups[] = {
+	RIIC0(RZ_PMX_GROUP)
+	RIIC1(RZ_PMX_GROUP)
+	RIIC2(RZ_PMX_GROUP)
+	RIIC3(RZ_PMX_GROUP)
+	RSPI0(RZ_PMX_GROUP)
+	RSPI1(RZ_PMX_GROUP)
+	RSPI2(RZ_PMX_GROUP)
+	RSPI3(RZ_PMX_GROUP)
+	RSPI4(RZ_PMX_GROUP)
+	SCIF0(RZ_PMX_GROUP)
+	SCIF1(RZ_PMX_GROUP)
+	SCIF2(RZ_PMX_GROUP)
+	SCIF3(RZ_PMX_GROUP)
+	SCIF4(RZ_PMX_GROUP)
+	SCIF5(RZ_PMX_GROUP)
+	SCIF6(RZ_PMX_GROUP)
+	SCIF7(RZ_PMX_GROUP)
+	ETHERNET(RZ_PMX_GROUP)
+};
+
+static const char * const riic0_groups[] = {
+	RIIC0(RZ_GROUPS)
+};
+
+static const char * const riic1_groups[] = {
+	RIIC1(RZ_GROUPS)
+};
+
+static const char * const riic2_groups[] = {
+	RIIC2(RZ_GROUPS)
+};
+
+static const char * const riic3_groups[] = {
+	RIIC3(RZ_GROUPS)
+};
+
+static const char * const rspi0_groups[] = {
+	RSPI0(RZ_GROUPS)
+};
+
+static const char * const rspi1_groups[] = {
+	RSPI1(RZ_GROUPS)
+};
+
+static const char * const rspi2_groups[] = {
+	RSPI2(RZ_GROUPS)
+};
+
+static const char * const rspi3_groups[] = {
+	RSPI3(RZ_GROUPS)
+};
+
+static const char * const rspi4_groups[] = {
+	RSPI4(RZ_GROUPS)
+};
+
+static const char * const scif0_groups[] = {
+	SCIF0(RZ_GROUPS)
+};
+
+static const char * const scif1_groups[] = {
+	SCIF1(RZ_GROUPS)
+};
+
+static const char * const scif2_groups[] = {
+	SCIF2(RZ_GROUPS)
+};
+
+static const char * const scif3_groups[] = {
+	SCIF3(RZ_GROUPS)
+};
+
+static const char * const scif4_groups[] = {
+	SCIF4(RZ_GROUPS)
+};
+
+static const char * const scif5_groups[] = {
+	SCIF5(RZ_GROUPS)
+};
+
+static const char * const scif6_groups[] = {
+	SCIF6(RZ_GROUPS)
+};
+
+static const char * const scif7_groups[] = {
+	SCIF7(RZ_GROUPS)
+};
+
+static const char * const ethernet_groups[] = {
+	ETHERNET(RZ_GROUPS)
+};
+
+static const struct sh_pfc_function pinmux_functions[] = {
+	SH_PFC_FUNCTION(riic0),
+	SH_PFC_FUNCTION(riic1),
+	SH_PFC_FUNCTION(riic2),
+	SH_PFC_FUNCTION(riic3),
+	SH_PFC_FUNCTION(rspi0),
+	SH_PFC_FUNCTION(rspi1),
+	SH_PFC_FUNCTION(rspi2),
+	SH_PFC_FUNCTION(rspi3),
+	SH_PFC_FUNCTION(rspi4),
+	SH_PFC_FUNCTION(scif0),
+	SH_PFC_FUNCTION(scif1),
+	SH_PFC_FUNCTION(scif2),
+	SH_PFC_FUNCTION(scif3),
+	SH_PFC_FUNCTION(scif4),
+	SH_PFC_FUNCTION(scif5),
+	SH_PFC_FUNCTION(scif6),
+	SH_PFC_FUNCTION(scif7),
+	SH_PFC_FUNCTION(ethernet),
+};
+
+#define PFC_REG(idx, name, reg)						\
+	{ PINMUX_CFG_REG(__stringify(name), reg, 16, 1) {		\
+		GP_##idx##_15_##name##_0, GP_##idx##_15_##name##_1,	\
+		GP_##idx##_14_##name##_0, GP_##idx##_14_##name##_1,	\
+		GP_##idx##_13_##name##_0, GP_##idx##_13_##name##_1,	\
+		GP_##idx##_12_##name##_0, GP_##idx##_12_##name##_1,	\
+		GP_##idx##_11_##name##_0, GP_##idx##_11_##name##_1,	\
+		GP_##idx##_10_##name##_0, GP_##idx##_10_##name##_1,	\
+		GP_##idx##_9_##name##_0, GP_##idx##_9_##name##_1,		\
+		GP_##idx##_8_##name##_0, GP_##idx##_8_##name##_1,		\
+		GP_##idx##_7_##name##_0, GP_##idx##_7_##name##_1,		\
+		GP_##idx##_6_##name##_0, GP_##idx##_6_##name##_1,		\
+		GP_##idx##_5_##name##_0, GP_##idx##_5_##name##_1,		\
+		GP_##idx##_4_##name##_0, GP_##idx##_4_##name##_1,		\
+		GP_##idx##_3_##name##_0, GP_##idx##_3_##name##_1,		\
+		GP_##idx##_2_##name##_0, GP_##idx##_2_##name##_1,		\
+		GP_##idx##_1_##name##_0, GP_##idx##_1_##name##_1,		\
+		GP_##idx##_0_##name##_0, GP_##idx##_0_##name##_1 }	\
+	}
+
+#define PFC_REGS(idx)						\
+	PFC_REG(idx, PMC, (0xfcfe3400 + (idx * 4))),		\
+	PFC_REG(idx, PFC, (0xfcfe3500 + (idx * 4))),		\
+	PFC_REG(idx, PFCE, (0xfcfe3600 + (idx * 4))),		\
+	PFC_REG(idx, PFCAE, (0xfcfe3a00 + (idx * 4))),		\
+	PFC_REG(idx, PIBC, (0xfcfe7000 + (idx * 4))),		\
+	PFC_REG(idx, PBDC, (0xfcfe7100 + (idx * 4))),		\
+	PFC_REG(idx, PIPC, (0xfcfe7200 + (idx * 4)))
+
+static struct pinmux_cfg_reg pinmux_config_regs[] = {
+	PFC_REGS(0), PFC_REGS(1), PFC_REGS(2), PFC_REGS(3),
+	PFC_REGS(4), PFC_REGS(5), PFC_REGS(6), PFC_REGS(7),
+	PFC_REGS(8), PFC_REGS(9), PFC_REGS(10), PFC_REGS(11),
+	PFC_REG(12, PMC, 0xfcfe7b40),
+	PFC_REG(12, PIBC, 0xfcfe7f00),
+	{ },
+};
+
+const struct sh_pfc_soc_info r7s72100_pinmux_info = {
+	.name = "r7s72100_pfc",
+
+	.function = { PINMUX_FUNCTION_BEGIN, PINMUX_FUNCTION_END },
+
+	.pins = pinmux_pins,
+	.nr_pins = ARRAY_SIZE(pinmux_pins),
+	.groups = pinmux_groups,
+	.nr_groups = ARRAY_SIZE(pinmux_groups),
+	.functions = pinmux_functions,
+	.nr_functions = ARRAY_SIZE(pinmux_functions),
+
+	.cfg_regs = pinmux_config_regs,
+
+	.pinmux_data = pinmux_data,
+	.pinmux_data_size = ARRAY_SIZE(pinmux_data),
+};
diff --git a/drivers/pinctrl/sh-pfc/sh_pfc.h b/drivers/pinctrl/sh-pfc/sh_pfc.h
index e42cc7a..95a07c9 100644
--- a/drivers/pinctrl/sh-pfc/sh_pfc.h
+++ b/drivers/pinctrl/sh-pfc/sh_pfc.h
@@ -257,6 +257,7 @@ struct sh_pfc_soc_info {
 };
 
 extern const struct sh_pfc_soc_info emev2_pinmux_info;
+extern const struct sh_pfc_soc_info r7s72100_pinmux_info;
 extern const struct sh_pfc_soc_info r8a73a4_pinmux_info;
 extern const struct sh_pfc_soc_info r8a7740_pinmux_info;
 extern const struct sh_pfc_soc_info r8a7778_pinmux_info;
@@ -484,14 +485,16 @@ extern const struct sh_pfc_soc_info shx3_pinmux_info;
 #define GP_ALL(str)			CPU_ALL_PORT(_GP_ALL, str)
 
 /* PINMUX_GPIO_GP_ALL - Expand to a list of sh_pfc_pin entries */
-#define _GP_GPIO(bank, _pin, _name, sfx, cfg)				\
+#define _GP_GPIO(banksize, bank, _pin, _name, sfx, cfg)				\
 	{								\
-		.pin = (bank * 32) + _pin,				\
+		.pin = (bank * banksize) + _pin,			\
 		.name = __stringify(_name),				\
 		.enum_id = _name##_DATA,				\
 		.configs = cfg,						\
 	}
-#define PINMUX_GPIO_GP_ALL()		CPU_ALL_PORT(_GP_GPIO, unused)
+#define _GP_GPIO32(bank, _pin, _name, sfx, cfg)\
+  _GP_GPIO(32, bank, _pin, _name, sfx, cfg)
+#define PINMUX_GPIO_GP_ALL()		CPU_ALL_PORT(_GP_GPIO32, unused)
 
 /* PINMUX_DATA_GP_ALL -  Expand to a list of name_DATA, name_FN marks */
 #define _GP_DATA(bank, pin, name, sfx, cfg)	PINMUX_DATA(name##_DATA, name##_FN)
-- 
2.7.4

Re: [PATCH 1/3] pinctrl: sh-pfc: Add r7s72100 PFC driver

[Geert Uytterhoeven]

Hi Jacopo,

On Mon, Jan 9, 2017 at 8:31 PM, Jacopo Mondi <jacopo+renesas@jmondi.org> wrote:
> From: Magnus Damm <damm@opensource.se>
>
> Squash commits in Geert's renesas-driver/genmai-gpio-and-pfc branch that
> add support for r7s72100 PFC.
> This squash combines commits for Magnus' original driver and minor fixes
> to forward-port it to a more recent kernel (v4.10)
>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

> --- a/drivers/pinctrl/sh-pfc/core.c
> +++ b/drivers/pinctrl/sh-pfc/core.c
> @@ -473,6 +473,12 @@ static const struct of_device_id sh_pfc_of_table[] = {
>                 .data = &emev2_pinmux_info,
>         },
>  #endif
> +#ifdef CONFIG_PINCTRL_PFC_R7S72100
> +       {
> +               .compatible = "renesas,pfc-r7s72100",
> +               .data = &r7s72100_pinmux_info,
> +       },
> +#endif
>  #ifdef CONFIG_PINCTRL_PFC_R8A73A4
>         {
>                 .compatible = "renesas,pfc-r8a73a4",
> @@ -626,6 +632,9 @@ static int sh_pfc_probe(struct platform_device *pdev)
>  }
>
>  static const struct platform_device_id sh_pfc_id_table[] = {
> +#ifdef CONFIG_PINCTRL_PFC_R7S72100
> +       { "pfc-r7s72100", (kernel_ulong_t)&r7s72100_pinmux_info },
> +#endif

The above chunk should be removed, as now r7s72100 always uses DT.

>  #ifdef CONFIG_PINCTRL_PFC_SH7203
>         { "pfc-sh7203", (kernel_ulong_t)&sh7203_pinmux_info },
>  #endif

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

[PATCH 2/3] gpio: gpio-rz: GPIO driver for Renesas RZ series

[Jacopo Mondi]

From: Magnus Damm <damm@opensource.se>

This commit combines Magnus' original driver and minor fixes to
forward-port it to a more recent kernel version (v4.10)

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

---------------------------------------------------------------
gpio: Renesas RZ GPIO driver V3

This patch adds a GPIO driver for the RZ series of SoCs from
Renesas. The V3 of the driver requires DT to be used.

The hardware allows control of GPIOs in blocks of up to 16 pins.
Interrupts are not included in this hardware block, if interrupts
are needed then the PFC needs to be configured to a IRQ pin function
which is handled by the GIC hardware.

Tested with yet-to-be-posted DT devices on r7s72100 and Genmai using
LEDs, DIP switches and I2C bitbang.

Signed-off-by: Magnus Damm <damm@opensource.se>

gpio: gpio-rz: Port to v4.10-rc1

Fix invalid return value in gpio remove function and change gpiochip's
"dev" field name to "parent" to compile driver against v4.10

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/gpio/Kconfig   |   6 ++
 drivers/gpio/Makefile  |   1 +
 drivers/gpio/gpio-rz.c | 212 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 219 insertions(+)
 create mode 100644 drivers/gpio/gpio-rz.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index d5d3654..e9ad7b4 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -369,6 +369,12 @@ config GPIO_RCAR
 	help
 	  Say yes here to support GPIO on Renesas R-Car SoCs.
 
+config GPIO_RZ
+	tristate "Renesas RZ GPIO"
+	depends on ARM
+	help
+	  Say yes here to support GPIO on Renesas RZ SoCs.
+
 config GPIO_SPEAR_SPICS
 	bool "ST SPEAr13xx SPI Chip Select as GPIO support"
 	depends on PLAT_SPEAR
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index a7676b8..f0b2713 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -96,6 +96,7 @@ obj-$(CONFIG_GPIO_PXA)		+= gpio-pxa.o
 obj-$(CONFIG_GPIO_RC5T583)	+= gpio-rc5t583.o
 obj-$(CONFIG_GPIO_RDC321X)	+= gpio-rdc321x.o
 obj-$(CONFIG_GPIO_RCAR)		+= gpio-rcar.o
+obj-$(CONFIG_GPIO_RZ)		+= gpio-rz.o
 obj-$(CONFIG_ARCH_SA1100)	+= gpio-sa1100.o
 obj-$(CONFIG_GPIO_SCH)		+= gpio-sch.o
 obj-$(CONFIG_GPIO_SCH311X)	+= gpio-sch311x.o
diff --git a/drivers/gpio/gpio-rz.c b/drivers/gpio/gpio-rz.c
new file mode 100644
index 0000000..b48fa05
--- /dev/null
+++ b/drivers/gpio/gpio-rz.c
@@ -0,0 +1,212 @@
+/*
+ * RZ GPIO Support - Ports
+ *
+ *  Copyright (C) 2013 Magnus Damm
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; version 2 of the
+ * License.
+ */
+
+#include <linux/bitops.h>
+#include <linux/err.h>
+#include <linux/gpio.h>
+#include <linux/io.h>
+#include <linux/ioport.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/pinctrl/consumer.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+
+#define RZ_GPIOS_PER_PORT 16
+
+enum { REG_PSR, REG_PPR, REG_PMSR, REG_NR };
+
+struct rz_gpio_priv {
+	void __iomem *io[REG_NR];
+	struct gpio_chip gpio_chip;
+};
+
+static inline u32 rz_gpio_read_ppr(struct rz_gpio_priv *p, int offs)
+{
+	unsigned long msk = BIT(offs % RZ_GPIOS_PER_PORT);
+	unsigned int offset = (offs / RZ_GPIOS_PER_PORT) * 4;
+
+	return ioread32(p->io[REG_PPR] + offset) & msk;
+}
+
+static inline void rz_gpio_write(struct rz_gpio_priv *p, int reg, int offs,
+				 bool value)
+{
+	unsigned long msk = BIT(offs % RZ_GPIOS_PER_PORT);
+	unsigned int offset = (offs / RZ_GPIOS_PER_PORT) * 4;
+
+	/* upper 16 bits contain mask and lower 16 actual value */
+	iowrite32(value ? (msk | (msk << 16)) : (msk << 16),
+		  p->io[reg] + offset);
+}
+
+static inline struct rz_gpio_priv *gpio_to_priv(struct gpio_chip *chip)
+{
+	return container_of(chip, struct rz_gpio_priv, gpio_chip);
+}
+
+static int rz_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
+{
+	/* Set bit in PM register via PMSR to disable output */
+	rz_gpio_write(gpio_to_priv(chip), REG_PMSR, offset, true);
+	return 0;
+}
+
+static int rz_gpio_get(struct gpio_chip *chip, unsigned offset)
+{
+	/* Get bit from PPR register to determine pin state */
+	return rz_gpio_read_ppr(gpio_to_priv(chip), offset);
+}
+
+static void rz_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
+{
+	/* Set bit in P register via PSR to control output */
+	rz_gpio_write(gpio_to_priv(chip), REG_PSR, offset, !!value);
+}
+
+static int rz_gpio_direction_output(struct gpio_chip *chip, unsigned offset,
+				   int value)
+{
+	/* Write GPIO value to output before selecting output mode of pin */
+	rz_gpio_set(chip, offset, value);
+
+	/* Clear bit in PM register via PMSR to enable output */
+	rz_gpio_write(gpio_to_priv(chip), REG_PMSR, offset, false);
+	return 0;
+}
+
+static int rz_gpio_request(struct gpio_chip *chip, unsigned offset)
+{
+	return pinctrl_request_gpio(chip->base + offset);
+}
+
+static void rz_gpio_free(struct gpio_chip *chip, unsigned offset)
+{
+	pinctrl_free_gpio(chip->base + offset);
+
+	/* Set the GPIO as an input to ensure that the next GPIO request won't
+	* drive the GPIO pin as an output.
+	*/
+	rz_gpio_direction_input(chip, offset);
+}
+
+static int rz_gpio_probe(struct platform_device *pdev)
+{
+	struct rz_gpio_priv *p;
+	struct resource *io[3];
+	struct gpio_chip *gpio_chip;
+	struct device_node *np = pdev->dev.of_node;
+	struct of_phandle_args args;
+	unsigned int k, nr;
+	int ret;
+
+	p = devm_kzalloc(&pdev->dev, sizeof(*p), GFP_KERNEL);
+	if (!p) {
+		dev_err(&pdev->dev, "failed to allocate driver data\n");
+		return -ENOMEM;
+	}
+
+	for (k = 0; k < REG_NR; k++)
+		io[k] = platform_get_resource(pdev, IORESOURCE_MEM, k);
+
+	/* In case of 3 resources PSR, PPR and PMSR order is expected */
+	if (io[REG_PSR] && io[REG_PPR] && io[REG_PMSR]) {
+		nr = REG_NR;
+	} else {
+		/* A single resource is also acceptable (PPR only) */
+		if (io[0] && !io[1] && !io[2]) {
+			nr = 1;
+		} else {
+			dev_err(&pdev->dev, "missing IOMEM\n");
+			return -EINVAL;
+		}
+	}
+
+	for (k = 0; k < nr; k++) {
+		p->io[k] = devm_ioremap_resource(&pdev->dev, io[k]);
+		if (IS_ERR(p->io[k]))
+			return PTR_ERR(p->io[k]);
+	}
+
+	/* If only 1 resource is available it must be PPR */
+	if (nr == 1) {
+		io[REG_PPR] = io[0];
+		p->io[REG_PPR] = p->io[0];
+		io[0] = NULL;
+		p->io[0] = NULL;
+	}
+
+	ret = of_parse_phandle_with_fixed_args(np, "gpio-ranges", 3, 0, &args);
+
+	gpio_chip = &p->gpio_chip;
+	gpio_chip->direction_input = rz_gpio_direction_input;
+	gpio_chip->get = rz_gpio_get;
+	gpio_chip->direction_output = rz_gpio_direction_output;
+	gpio_chip->set = rz_gpio_set;
+	gpio_chip->request = rz_gpio_request;
+	gpio_chip->free = rz_gpio_free;
+	gpio_chip->label = dev_name(&pdev->dev);
+	gpio_chip->parent = &pdev->dev;
+	gpio_chip->owner = THIS_MODULE;
+	gpio_chip->base = -1;
+	gpio_chip->ngpio = ret == 0 ? args.args[2] : RZ_GPIOS_PER_PORT;
+
+	ret = gpiochip_add(gpio_chip);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to add GPIO controller\n");
+		return ret;
+	}
+
+	dev_info(&pdev->dev, "driving %d GPIOs\n", gpio_chip->ngpio);
+	return 0;
+}
+
+static int rz_gpio_remove(struct platform_device *pdev)
+{
+	struct rz_gpio_priv *p = platform_get_drvdata(pdev);
+
+	gpiochip_remove(&p->gpio_chip);
+	return 0;
+}
+
+static const struct of_device_id rz_gpio_dt_ids[] = {
+	{ .compatible = "renesas,gpio-rz", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, rz_gpio_dt_ids);
+
+static struct platform_driver rz_gpio_device_driver = {
+	.probe		= rz_gpio_probe,
+	.remove		= rz_gpio_remove,
+	.driver		= {
+		.name	= "gpio_rz",
+		.of_match_table = rz_gpio_dt_ids,
+		.owner		= THIS_MODULE,
+	}
+};
+
+static int __init rz_gpio_init(void)
+{
+	return platform_driver_register(&rz_gpio_device_driver);
+}
+postcore_initcall(rz_gpio_init);
+
+static void __exit rz_gpio_exit(void)
+{
+	platform_driver_unregister(&rz_gpio_device_driver);
+}
+module_exit(rz_gpio_exit);
+
+MODULE_AUTHOR("Magnus Damm");
+MODULE_DESCRIPTION("Renesas RZ Port GPIO Driver");
+MODULE_LICENSE("GPL v2");
-- 
2.7.4

Re: [PATCH 2/3] gpio: gpio-rz: GPIO driver for Renesas RZ series

[Linus Walleij]

On Mon, Jan 9, 2017 at 8:31 PM, Jacopo Mondi <jacopo+renesas@jmondi.org> wrote:

> From: Magnus Damm <damm@opensource.se>
>
> This commit combines Magnus' original driver and minor fixes to
> forward-port it to a more recent kernel version (v4.10)
>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>
> ---------------------------------------------------------------
> gpio: Renesas RZ GPIO driver V3
>
> This patch adds a GPIO driver for the RZ series of SoCs from
> Renesas. The V3 of the driver requires DT to be used.
>
> The hardware allows control of GPIOs in blocks of up to 16 pins.
> Interrupts are not included in this hardware block, if interrupts
> are needed then the PFC needs to be configured to a IRQ pin function
> which is handled by the GIC hardware.
>
> Tested with yet-to-be-posted DT devices on r7s72100 and Genmai using
> LEDs, DIP switches and I2C bitbang.
>
> Signed-off-by: Magnus Damm <damm@opensource.se>
>
> gpio: gpio-rz: Port to v4.10-rc1
>
> Fix invalid return value in gpio remove function and change gpiochip's
> "dev" field name to "parent" to compile driver against v4.10
>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

This commit message looks seriously mangled, please make it look like a regular
one with all signed offs at the end.

> +config GPIO_RZ
> +       tristate "Renesas RZ GPIO"
> +       depends on ARM

ARM really? Not some Renesas or so?
Include || COMPILE_TEST?

> +#include <linux/bitops.h>
> +#include <linux/err.h>
> +#include <linux/gpio.h>

No. Use only
#include <linux/gpio/driver.h>

> +static inline struct rz_gpio_priv *gpio_to_priv(struct gpio_chip *chip)
> +{
> +       return container_of(chip, struct rz_gpio_priv, gpio_chip);
> +}

Please get rid of this and use gpiochip_get_data()
after adding the chip with devm_gpiochip_add_data()
supplying the data you need.

Look at other GPIO drivers for examples.

> +static int rz_gpio_get(struct gpio_chip *chip, unsigned offset)
> +{
> +       /* Get bit from PPR register to determine pin state */
> +       return rz_gpio_read_ppr(gpio_to_priv(chip), offset);

return !!rz_gpio_read_ppr(gpio_to_priv(chip), offset);

To clamp to bool.

> +static void rz_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
> +{
> +       /* Set bit in P register via PSR to control output */
> +       rz_gpio_write(gpio_to_priv(chip), REG_PSR, offset, !!value);
> +}

Ironically you can trust the value to be 0/1 here. Check the
gpiolib.

> +static int rz_gpio_request(struct gpio_chip *chip, unsigned offset)
> +{
> +       return pinctrl_request_gpio(chip->base + offset);
> +}

What about just using gpiochip_generic_request instead?

> +static void rz_gpio_free(struct gpio_chip *chip, unsigned offset)
> +{
> +       pinctrl_free_gpio(chip->base + offset);
> +
> +       /* Set the GPIO as an input to ensure that the next GPIO request won't
> +       * drive the GPIO pin as an output.
> +       */
> +       rz_gpio_direction_input(chip, offset);
> +}

Very close to gpiochip_generic_free()

Maybe we should actually set lines as input in the
generic routine!

> +       gpio_chip = &p->gpio_chip;
> +       gpio_chip->direction_input = rz_gpio_direction_input;
> +       gpio_chip->get = rz_gpio_get;
> +       gpio_chip->direction_output = rz_gpio_direction_output;

Please implement .get_direction() as well.

> +       gpio_chip->set = rz_gpio_set;
> +       gpio_chip->request = rz_gpio_request;
> +       gpio_chip->free = rz_gpio_free;
> +       gpio_chip->label = dev_name(&pdev->dev);
> +       gpio_chip->parent = &pdev->dev;
> +       gpio_chip->owner = THIS_MODULE;
> +       gpio_chip->base = -1;
> +       gpio_chip->ngpio = ret == 0 ? args.args[2] : RZ_GPIOS_PER_PORT;
> +
> +       ret = gpiochip_add(gpio_chip);

Use devm_gpiochip_add_data() providing struct rz_gpio_priv * as
data.

> +static int rz_gpio_remove(struct platform_device *pdev)
> +{
> +       struct rz_gpio_priv *p = platform_get_drvdata(pdev);
> +
> +       gpiochip_remove(&p->gpio_chip);
> +       return 0;
> +}

And with the devm_gpiochip_add_data() you don't even need
this remove().

Yours,
Linus Walleij

Re: [PATCH 2/3] gpio: gpio-rz: GPIO driver for Renesas RZ series

[jacopo mondi]

Hi Linus,
     thanks for review

On 11/01/2017 15:55, Linus Walleij wrote:
> On Mon, Jan 9, 2017 at 8:31 PM, Jacopo Mondi <jacopo+renesas@jmondi.org> wrote:

>
>> +static void rz_gpio_free(struct gpio_chip *chip, unsigned offset)
>> +{
>> +       pinctrl_free_gpio(chip->base + offset);
>> +
>> +       /* Set the GPIO as an input to ensure that the next GPIO request won't
>> +       * drive the GPIO pin as an output.
>> +       */
>> +       rz_gpio_direction_input(chip, offset);
>> +}
>
> Very close to gpiochip_generic_free()
>
> Maybe we should actually set lines as input in the
> generic routine!
>

Is it ok then to simply substitute "pinctrl_free_gpio" with 
"gpiochip_generic_free" and keep rz_gpio_direction_input here?


>> +       gpio_chip = &p->gpio_chip;
>> +       gpio_chip->direction_input = rz_gpio_direction_input;
>> +       gpio_chip->get = rz_gpio_get;
>> +       gpio_chip->direction_output = rz_gpio_direction_output;
>
> Please implement .get_direction() as well.
>

That's funny how a simple addition like this one would require several 
changes (possibly even in the dts ABI defined by this driver).

This is more a question for the original driver author I hope is 
listening here:

To read the pin direction I would need to add one more register to the 
"reg" property range provided in the DTS.
This is of course doable, but I would have two questions here:
- why did you chose to use PMSR register instead of reading/writing 
directly to PM? Am I missing something?
- wouldn't it be better to just receive the port base address from the 
"reg" property and offset from that instead of having the 3 register 
addresses specified in the dts?

See, the dts now looks like this:

reg = <0xfcfe3100 0x4>, /* PSR */
       <0xfcfe3200 0x2>, /* PPR */
       <0xfcfe3800 0x4>; /* PMSR */

Shouldn't we just receive the gpiochip base address and the offset as we 
like?

Thanks
    j

RE: [PATCH 2/3] gpio: gpio-rz: GPIO driver for Renesas RZ series

[Chris Brandt]

Hi Jacopo,

On Thursday, January 12, 2017, jacopo mondi wrote:
> To read the pin direction I would need to add one more register to the
> "reg" property range provided in the DTS.
> This is of course doable, but I would have two questions here:
> - why did you chose to use PMSR register instead of reading/writing
> directly to PM? Am I missing something?


I guess you can still use the PMSR register to get the current direction
of the I/O pin. According to the HW manual:

"When reading, the higher 16 bits are read as 0000H. The lower 16 bits
 are read as the value of the PMn register."


While the PMSR register is a cool idea of having a special register that
can change a single port pin direction without doing a read-modify-write
by the CPU, it doesn't seem to be a standard option in Renesas SoCs (well,
except for the older NEC V850 parts where this PFC HW came from).
So a more traditional method of just using the Pmn register seems to be
more compatible across Renesas SoCs.


Chris

Re: [PATCH 2/3] gpio: gpio-rz: GPIO driver for Renesas RZ series

[jacopo mondi]

Hi Chris,

On 12/01/2017 15:39, Chris Brandt wrote:
> Hi Jacopo,
>
> On Thursday, January 12, 2017, jacopo mondi wrote:
>> To read the pin direction I would need to add one more register to the
>> "reg" property range provided in the DTS.
>> This is of course doable, but I would have two questions here:
>> - why did you chose to use PMSR register instead of reading/writing
>> directly to PM? Am I missing something?
>
>
> I guess you can still use the PMSR register to get the current direction
> of the I/O pin. According to the HW manual:
>
> "When reading, the higher 16 bits are read as 0000H. The lower 16 bits
>  are read as the value of the PMn register."
>

Oh! That's weird I don't have that statement in my manual (public 
version R01UH0403EJ0060 Rev.0.60)

The only description for PMSR I have is:

"This register provides an alternative method for writing data to the 
PMn register.
The higher 16 bits of the PMSRn register specify whether data can be 
written to the PMn.PMnm bit specified by the
lower 16 bits of the PMSRn register."

So I assumed it was one direction only

>
> While the PMSR register is a cool idea of having a special register that
> can change a single port pin direction without doing a read-modify-write
> by the CPU, it doesn't seem to be a standard option in Renesas SoCs (well,
> except for the older NEC V850 parts where this PFC HW came from).
> So a more traditional method of just using the Pmn register seems to be
> more compatible across Renesas SoCs.
>
>
> Chris
>

RE: [PATCH 2/3] gpio: gpio-rz: GPIO driver for Renesas RZ series

[Chris Brandt]

Hi Jacopo,

On Thursday, January 12, 2017, jacopo mondi wrote:
> Oh! That's weird I don't have that statement in my manual (public version
> R01UH0403EJ0060 Rev.0.60)

If you go to www.renesas.com and search for "RZ/A1H User Manual", you will
see that a new version 3.0 manual was recently posted. This new version contains
many new descriptions and precautions. It also now includes the SDHI chapter
which previously was always omitted because of legal reasons.

There is a separate manual for the RZ/A1L. The peripherals are all the same,
but the function pinouts are different because the packages are smaller. Hence,
all the existing upstream drivers for RZ/A1H work fine for RZ/A1M and RZ/A1L
without modification...except the pins comes out on different ports...and
the sh-pfc driver won't work well in this situation.


Chris

[PATCH 3/3] arm: dts: r7s72100: Add peripherals nodes

[Jacopo Mondi]

From: Magnus Damm <damm@opensource.se>

This is a squash of several commits, adding peripherals groups
configuration to r7s72100 device tree, and enabling some of them on
Genmai evaluation board

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

-------------------------------------------------------------------

[REBASED] ARM: shmobile: r7s72100 GPIO and PINCTRL device nodes

Add support for r7s72100 PFC and GPIO device nodes port0 -> port11
and jtagport0.

Signed-off-by: Magnus Damm <damm@opensource.se>

[REBASED] ARM: shmobile: Genmai SCIF2 PINCTRL configuration

Configure the r7s72100 PINCTRL hardware and select pin function
for the SCIF2 serial console.

Signed-off-by: Magnus Damm <damm@opensource.se>
Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

[REBASED] ARM: shmobile: Genmai LED1 and LED2 support

Add support for Genmai board LED1 and LED2 via gpio-leds.

Signed-off-by: Magnus Damm <damm@opensource.se>
Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

ARM: shmobile: Genmai I2C-over-GPIO support

Add support for the Genmai I2C bus hooked up to P1_5 and P1_4 using
the i2c-gpio driver. On the bus sits a 24c128 EEPRROM.

Signed-off-by: Magnus Damm <damm@opensource.se>

[REBASED] arm: shmobile: genmai: adapt dts to use native i2c driver

Switch from the gpio-driver to the shiny new native driver. Tested by
accessing the eeprom on the genmai board.

Signed-off-by: Wolfram Sang <wsa@sang-engineering.com>
Acked-by: Magnus Damm <damm@opensource.se>

[REBASED] ARM: shmobile: r7s72100: Add ethernet PFC node to DT

Signed-off-by: Simon Horman <horms+renesas@verge.net.au>

[BLOCKED] ARM: shmobile: genmai reference dts: Add pinctrl for RSPI

Add pinctrl for the existing rspi4 node on Genmai.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

[WIP] genmai sh_eth dts fixup
---
 arch/arm/boot/dts/r7s72100-genmai.dts |  51 ++++++++++++
 arch/arm/boot/dts/r7s72100.dtsi       | 151 ++++++++++++++++++++++++++++++++++
 2 files changed, 202 insertions(+)

diff --git a/arch/arm/boot/dts/r7s72100-genmai.dts b/arch/arm/boot/dts/r7s72100-genmai.dts
index 118a8e2..59bccd3 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>
 
 / {
 	model = "Genmai";
@@ -34,6 +35,17 @@
 		#address-cells = <1>;
 		#size-cells = <1>;
 	};
+
+	leds {
+		compatible = "gpio-leds";
+		led1 {
+			gpios = <&port4 10 GPIO_ACTIVE_LOW>;
+		};
+		led2 {
+			gpios = <&port4 11 GPIO_ACTIVE_LOW>;
+		};
+	};
+
 };
 
 &extal_clk {
@@ -59,6 +71,45 @@
 	};
 };
 
+&pfc {
+	pinctrl-0 = <&ethernet_pins &rspi4_pins &scif2_pins>;
+	pinctrl-names = "default";
+
+	ethernet_pins: ethernet {
+		renesas,groups = "ethernet_rxdv_p3_6",
+				 "ethernet_rxer_p3_5",
+				 "ethernet_rxclk_p3_4",
+				 "ethernet_mdio_p3_3",
+				 "ethernet_rxd3_p2_11",
+				 "ethernet_rxd2_p2_10",
+				 "ethernet_rxd1_p2_9",
+				 "ethernet_rxd0_p2_8",
+				 "ethernet_txd3_p2_7",
+				 "ethernet_txd2_p2_6",
+				 "ethernet_txd1_p2_5",
+				 "ethernet_txd0_p2_4",
+				 "ethernet_txcrs_p2_3",
+				 "ethernet_txen_p2_2",
+				 "ethernet_txer_p2_1",
+				 "ethernet_txclk_p2_0",
+				 "ethernet_mdc_p5_9",
+				 "ethernet_col_p1_14",
+				 "ethernet_int_p1_15";
+		renesas,function = "ethernet";
+	};
+
+	rspi4_pins: spi4 {
+		renesas,groups = "rspi4_rspck_p4_0", "rspi4_ssl0_p4_1",
+				 "rspi4_mosi_p4_2", "rspi4_miso_p4_3";
+		renesas,function = "rspi4";
+	};
+
+	scif2_pins: serial2 {
+		renesas,groups = "scif2_txd_p3_0", "scif2_rxd_p3_2";
+		renesas,function = "scif2";
+	};
+};
+
 &scif2 {
 	status = "okay";
 };
diff --git a/arch/arm/boot/dts/r7s72100.dtsi b/arch/arm/boot/dts/r7s72100.dtsi
index 3dd427d..47bfd47 100644
--- a/arch/arm/boot/dts/r7s72100.dtsi
+++ b/arch/arm/boot/dts/r7s72100.dtsi
@@ -20,6 +20,19 @@
 	#size-cells = <1>;
 
 	aliases {
+		gpio0 = &port0;
+		gpio1 = &port1;
+		gpio2 = &port2;
+		gpio3 = &port3;
+		gpio4 = &port4;
+		gpio5 = &port5;
+		gpio6 = &port6;
+		gpio7 = &port7;
+		gpio8 = &port8;
+		gpio9 = &port9;
+		gpio10 = &port10;
+		gpio11 = &port11;
+		gpio12 = &jtagport0;
 		i2c0 = &i2c0;
 		i2c1 = &i2c1;
 		i2c2 = &i2c2;
@@ -359,6 +372,144 @@
 			<0xe8202000 0x1000>;
 	};
 
+	pfc: pfc@fcfe3300 {
+		compatible = "renesas,pfc-r7s72100";
+		reg = <0xfcfe3300 0x400>, /* PM, PMC, PFC, PFCE */
+		  	  <0xfcfe3a00 0x100>, /* PFCAE */
+			  <0xfcfe7000 0x300>, /* PIBC, PBDC, PIPC */
+			  <0xfcfe7b40 0x04>, /* JPMC */
+			  <0xfcfe7b90 0x04>, /* JPMCSR */
+			  <0xfcfe7f00 0x04>; /* JPIBC */
+	};
+
+	port0: gpio@fcfe3100 {
+		compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz";
+		reg = <0xfcfe3100 0x4>, /* PSR */
+		  	  <0xfcfe3200 0x2>, /* PPR */
+			  <0xfcfe3800 0x4>; /* PMSR */
+		#gpio-cells = <2>;
+		gpio-controller;
+		gpio-ranges = <&pfc 0 0 6>;
+	};
+
+	port1: gpio@fcfe3104 {
+		compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz";
+		reg = <0xfcfe3104 0x4>, /* PSR */
+		  	  <0xfcfe3204 0x2>, /* PPR */
+			  <0xfcfe3804 0x4>; /* PMSR */
+		#gpio-cells = <2>;
+		gpio-controller;
+		gpio-ranges = <&pfc 0 16 16>;
+	};
+
+	port2: gpio@fcfe3108 {
+		compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz";
+		reg = <0xfcfe3108 0x4>, /* PSR */
+		  	  <0xfcfe3208 0x2>, /* PPR */
+			  <0xfcfe3808 0x4>; /* PMSR */
+		#gpio-cells = <2>;
+		gpio-controller;
+		gpio-ranges = <&pfc 0 32 16>;
+	};
+
+	port3: gpio@fcfe310c {
+		compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz";
+		reg = <0xfcfe310c 0x4>, /* PSR */
+		  	  <0xfcfe320c 0x2>, /* PPR */
+			  <0xfcfe380c 0x4>; /* PMSR */
+		#gpio-cells = <2>;
+		gpio-controller;
+		gpio-ranges = <&pfc 0 48 16>;
+	};
+
+	port4: gpio@fcfe3110 {
+		compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz";
+		reg = <0xfcfe3110 0x4>, /* PSR */
+		  	  <0xfcfe3210 0x2>, /* PPR */
+			  <0xfcfe3810 0x4>; /* PMSR */
+		#gpio-cells = <2>;
+		gpio-controller;
+		gpio-ranges = <&pfc 0 64 16>;
+	};
+
+	port5: gpio@fcfe3114 {
+		compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz";
+		reg = <0xfcfe3114 0x4>, /* PSR */
+		  	  <0xfcfe3214 0x2>, /* PPR */
+			  <0xfcfe3814 0x4>; /* PMSR */
+		#gpio-cells = <2>;
+		gpio-controller;
+		gpio-ranges = <&pfc 0 80 11>;
+	};
+
+	port6: gpio@fcfe3118 {
+		compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz";
+		reg = <0xfcfe3118 0x4>, /* PSR */
+		  	  <0xfcfe3218 0x2>, /* PPR */
+			  <0xfcfe3818 0x4>; /* PMSR */
+		#gpio-cells = <2>;
+		gpio-controller;
+		gpio-ranges = <&pfc 0 96 16>;
+	};
+
+	port7: gpio@fcfe311c {
+		compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz";
+		reg = <0xfcfe311c 0x4>, /* PSR */
+		  	  <0xfcfe321c 0x2>, /* PPR */
+			  <0xfcfe381c 0x4>; /* PMSR */
+		#gpio-cells = <2>;
+		gpio-controller;
+		gpio-ranges = <&pfc 0 112 16>;
+	};
+
+	port8: gpio@fcfe3120 {
+		compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz";
+		reg = <0xfcfe3120 0x4>, /* PSR */
+		  	  <0xfcfe3220 0x2>, /* PPR */
+			  <0xfcfe3820 0x4>; /* PMSR */
+		#gpio-cells = <2>;
+		gpio-controller;
+		gpio-ranges = <&pfc 0 128 16>;
+	};
+
+	port9: gpio@fcfe3124 {
+		compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz";
+		reg = <0xfcfe3124 0x4>, /* PSR */
+		  	  <0xfcfe3224 0x2>, /* PPR */
+			  <0xfcfe3824 0x4>; /* PMSR */
+		#gpio-cells = <2>;
+		gpio-controller;
+		gpio-ranges = <&pfc 0 144 8>;
+	};
+
+	port10: gpio@fcfe3128 {
+		compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz";
+		reg = <0xfcfe3128 0x4>, /* PSR */
+		  	  <0xfcfe3228 0x2>, /* PPR */
+			  <0xfcfe3828 0x4>; /* PMSR */
+		#gpio-cells = <2>;
+		gpio-controller;
+		gpio-ranges = <&pfc 0 160 16>;
+	};
+
+	port11: gpio@fcfe312c {
+		compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz";
+		reg = <0xfcfe312c 0x4>, /* PSR */
+		  	  <0xfcfe322c 0x2>, /* PPR */
+			  <0xfcfe382c 0x4>; /* PMSR */
+		#gpio-cells = <2>;
+		gpio-controller;
+		gpio-ranges = <&pfc 0 176 16>;
+	};
+
+	jtagport0: gpio@fcfe7b20 {
+		compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz";
+		reg = <0xfcfe7b20 0x2>; /* JPPR */
+		#gpio-cells = <2>;
+		gpio-controller;
+		gpio-ranges = <&pfc 0 192 2>;
+	};
+
 	i2c0: i2c@fcfee000 {
 		#address-cells = <1>;
 		#size-cells = <0>;
-- 
2.7.4

Re: [PATCH 3/3] arm: dts: r7s72100: Add peripherals nodes

[Geert Uytterhoeven]

Hi Jacopo,

On Mon, Jan 9, 2017 at 8:31 PM, Jacopo Mondi <jacopo+renesas@jmondi.org> wrote:
> From: Magnus Damm <damm@opensource.se>
>
> This is a squash of several commits, adding peripherals groups
> configuration to r7s72100 device tree, and enabling some of them on
> Genmai evaluation board
>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Thanks for the rework!

>  arch/arm/boot/dts/r7s72100-genmai.dts |  51 ++++++++++++
>  arch/arm/boot/dts/r7s72100.dtsi       | 151 ++++++++++++++++++++++++++++++++++

This path should be split in multiple parts:
  - Add the pfc node to r7s72100.dtsi,
  - Add the gpio nodes to r7s72100.dtsi,
  - 4 patches for r7s72100-genmai.dts, adding support for LEDs, SCIF,
    Ethernet, and SPI.

> --- a/arch/arm/boot/dts/r7s72100.dtsi
> +++ b/arch/arm/boot/dts/r7s72100.dtsi
> @@ -20,6 +20,19 @@
>         #size-cells = <1>;
>
>         aliases {
> +               gpio0 = &port0;
> +               gpio1 = &port1;
> +               gpio2 = &port2;
> +               gpio3 = &port3;
> +               gpio4 = &port4;
> +               gpio5 = &port5;
> +               gpio6 = &port6;
> +               gpio7 = &port7;
> +               gpio8 = &port8;
> +               gpio9 = &port9;
> +               gpio10 = &port10;
> +               gpio11 = &port11;
> +               gpio12 = &jtagport0;

Please remove this hunk.
GPIO aliases are deprecated, and I don't think the driver uses them.

>                 i2c0 = &i2c0;
>                 i2c1 = &i2c1;
>                 i2c2 = &i2c2;

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

Re: [PATCH 3/3] arm: dts: r7s72100: Add peripherals nodes

[Laurent Pinchart]

Hi Geert,

On Tuesday 10 Jan 2017 16:07:01 Geert Uytterhoeven wrote:
> On Mon, Jan 9, 2017 at 8:31 PM, Jacopo Mondi wrote:
> > From: Magnus Damm <damm@opensource.se>
> > 
> > This is a squash of several commits, adding peripherals groups
> > configuration to r7s72100 device tree, and enabling some of them on
> > Genmai evaluation board
> > 
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> 
> Thanks for the rework!
> 
> >  arch/arm/boot/dts/r7s72100-genmai.dts |  51 ++++++++++++
> >  arch/arm/boot/dts/r7s72100.dtsi       | 151 +++++++++++++++++++++++++++++
>
> This path should be split in multiple parts:
>   - Add the pfc node to r7s72100.dtsi,
>   - Add the gpio nodes to r7s72100.dtsi,
>   - 4 patches for r7s72100-genmai.dts, adding support for LEDs, SCIF,
>     Ethernet, and SPI.

I can agree about the .dtsi/.dts split, but isn't this going a bit overboard ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/3] arm: dts: r7s72100: Add peripherals nodes

[Geert Uytterhoeven]

Hi Laurent,

On Tue, Jan 10, 2017 at 8:58 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Tuesday 10 Jan 2017 16:07:01 Geert Uytterhoeven wrote:
>> On Mon, Jan 9, 2017 at 8:31 PM, Jacopo Mondi wrote:
>> > From: Magnus Damm <damm@opensource.se>
>> >
>> > This is a squash of several commits, adding peripherals groups
>> > configuration to r7s72100 device tree, and enabling some of them on
>> > Genmai evaluation board
>> >
>> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>>
>> Thanks for the rework!
>>
>> >  arch/arm/boot/dts/r7s72100-genmai.dts |  51 ++++++++++++
>> >  arch/arm/boot/dts/r7s72100.dtsi       | 151 +++++++++++++++++++++++++++++
>>
>> This path should be split in multiple parts:
>>   - Add the pfc node to r7s72100.dtsi,
>>   - Add the gpio nodes to r7s72100.dtsi,
>>   - 4 patches for r7s72100-genmai.dts, adding support for LEDs, SCIF,
>>     Ethernet, and SPI.
>
> I can agree about the .dtsi/.dts split, but isn't this going a bit overboard ?

Let's find out what Simon's opinion is...

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

Re: [PATCH 3/3] arm: dts: r7s72100: Add peripherals nodes

[Simon Horman]

On Tue, Jan 10, 2017 at 09:58:19PM +0200, Laurent Pinchart wrote:
> Hi Geert,
> 
> On Tuesday 10 Jan 2017 16:07:01 Geert Uytterhoeven wrote:
> > On Mon, Jan 9, 2017 at 8:31 PM, Jacopo Mondi wrote:
> > > From: Magnus Damm <damm@opensource.se>
> > > 
> > > This is a squash of several commits, adding peripherals groups
> > > configuration to r7s72100 device tree, and enabling some of them on
> > > Genmai evaluation board
> > > 
> > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > 
> > Thanks for the rework!
> > 
> > >  arch/arm/boot/dts/r7s72100-genmai.dts |  51 ++++++++++++
> > >  arch/arm/boot/dts/r7s72100.dtsi       | 151 +++++++++++++++++++++++++++++
> >
> > This path should be split in multiple parts:
> >   - Add the pfc node to r7s72100.dtsi,
> >   - Add the gpio nodes to r7s72100.dtsi,
> >   - 4 patches for r7s72100-genmai.dts, adding support for LEDs, SCIF,
> >     Ethernet, and SPI.
> 
> I can agree about the .dtsi/.dts split, but isn't this going a bit overboard ?

I would like the split so that different patches touch different files
to be made.

I am willing to be flexible regarding adding more than one IP block in a
single patch if the patches would otherwise be very small and unlikely to
lead to breakage.

>From my PoV a key motivation for splitting things up is to make it easier
to selectively revert or backport individual features. I personally don't
have much cause to do either on a fine-grained basis of late. So I'm happy
to consider being more flexible with regards to patch granularity.

Re: [PATCH 3/3] arm: dts: r7s72100: Add peripherals nodes

[Laurent Pinchart]

Hi Simon,

On Wednesday 11 Jan 2017 11:33:17 Simon Horman wrote:
> On Tue, Jan 10, 2017 at 09:58:19PM +0200, Laurent Pinchart wrote:
> > On Tuesday 10 Jan 2017 16:07:01 Geert Uytterhoeven wrote:
> >> On Mon, Jan 9, 2017 at 8:31 PM, Jacopo Mondi wrote:
> >>> From: Magnus Damm <damm@opensource.se>
> >>> 
> >>> This is a squash of several commits, adding peripherals groups
> >>> configuration to r7s72100 device tree, and enabling some of them on
> >>> Genmai evaluation board
> >>> 
> >>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> >> 
> >> Thanks for the rework!
> >> 
> >>>  arch/arm/boot/dts/r7s72100-genmai.dts |  51 ++++++++++++
> >>>  arch/arm/boot/dts/r7s72100.dtsi       | 151 ++++++++++++++++++++++++++
> >> 
> >> This path should be split in multiple parts:
> >>   - Add the pfc node to r7s72100.dtsi,
> >>   - Add the gpio nodes to r7s72100.dtsi,
> >>   - 4 patches for r7s72100-genmai.dts, adding support for LEDs, SCIF,
> >>     Ethernet, and SPI.
> > 
> > I can agree about the .dtsi/.dts split, but isn't this going a bit
> > overboard ?
>
> I would like the split so that different patches touch different files
> to be made.

That's usually what I do, at least when it comes to device tree files. When 
reworking core code in a subsystem patches often have to touch multiple files, 
but that's different.

> I am willing to be flexible regarding adding more than one IP block in a
> single patch if the patches would otherwise be very small and unlikely to
> lead to breakage.

Splitting the GPIO and PFC nodes in two patches would be fine with me, but 
given that they're tightly related, I think it makes more sense to keep them 
in one patch in this particular case.

> From my PoV a key motivation for splitting things up is to make it easier
> to selectively revert or backport individual features. I personally don't
> have much cause to do either on a fine-grained basis of late. So I'm happy
> to consider being more flexible with regards to patch granularity.

I usually try to split addition of unrelated IP cores in multiple patches. On 
the other hand, when adding multiple IP cores in one series, they often end up 
one next to the other in the source, creating conflicts if you try to backport 
selectively. I don't think that's ideal either.

In this particular case, the changes to arch/arm/boot/dts/r7s72100-genmai.dts 
are twofold :

- add a GPIO LEDs node
- configure pinctrl for the ethernet, spi and scif devices

We could split that in two patches, but I probably wouldn't split the last one 
in three patches.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/3] arm: dts: r7s72100: Add peripherals nodes

[Simon Horman]

On Wed, Jan 11, 2017 at 12:55:23PM +0200, Laurent Pinchart wrote:
> Hi Simon,
> 
> On Wednesday 11 Jan 2017 11:33:17 Simon Horman wrote:
> > On Tue, Jan 10, 2017 at 09:58:19PM +0200, Laurent Pinchart wrote:
> > > On Tuesday 10 Jan 2017 16:07:01 Geert Uytterhoeven wrote:
> > >> On Mon, Jan 9, 2017 at 8:31 PM, Jacopo Mondi wrote:
> > >>> From: Magnus Damm <damm@opensource.se>
> > >>> 
> > >>> This is a squash of several commits, adding peripherals groups
> > >>> configuration to r7s72100 device tree, and enabling some of them on
> > >>> Genmai evaluation board
> > >>> 
> > >>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > >> 
> > >> Thanks for the rework!
> > >> 
> > >>>  arch/arm/boot/dts/r7s72100-genmai.dts |  51 ++++++++++++
> > >>>  arch/arm/boot/dts/r7s72100.dtsi       | 151 ++++++++++++++++++++++++++
> > >> 
> > >> This path should be split in multiple parts:
> > >>   - Add the pfc node to r7s72100.dtsi,
> > >>   - Add the gpio nodes to r7s72100.dtsi,
> > >>   - 4 patches for r7s72100-genmai.dts, adding support for LEDs, SCIF,
> > >>     Ethernet, and SPI.
> > > 
> > > I can agree about the .dtsi/.dts split, but isn't this going a bit
> > > overboard ?
> >
> > I would like the split so that different patches touch different files
> > to be made.
> 
> That's usually what I do, at least when it comes to device tree files. When 
> reworking core code in a subsystem patches often have to touch multiple files, 
> but that's different.
> 
> > I am willing to be flexible regarding adding more than one IP block in a
> > single patch if the patches would otherwise be very small and unlikely to
> > lead to breakage.
> 
> Splitting the GPIO and PFC nodes in two patches would be fine with me, but 
> given that they're tightly related, I think it makes more sense to keep them 
> in one patch in this particular case.
> 
> > From my PoV a key motivation for splitting things up is to make it easier
> > to selectively revert or backport individual features. I personally don't
> > have much cause to do either on a fine-grained basis of late. So I'm happy
> > to consider being more flexible with regards to patch granularity.
> 
> I usually try to split addition of unrelated IP cores in multiple patches. On 
> the other hand, when adding multiple IP cores in one series, they often end up 
> one next to the other in the source, creating conflicts if you try to backport 
> selectively. I don't think that's ideal either.
> 
> In this particular case, the changes to arch/arm/boot/dts/r7s72100-genmai.dts 
> are twofold :
> 
> - add a GPIO LEDs node
> - configure pinctrl for the ethernet, spi and scif devices
> 
> We could split that in two patches, but I probably wouldn't split the
> last one in three patches.

I agree that makes sense.

Re: [PATCH 3/3] arm: dts: r7s72100: Add peripherals nodes

[jacopo mondi]

Hi Geert,

On 10/01/2017 16:07, Geert Uytterhoeven wrote:
> Hi Jacopo,
>
> On Mon, Jan 9, 2017 at 8:31 PM, Jacopo Mondi <jacopo+renesas@jmondi.org> wrote:
>> From: Magnus Damm <damm@opensource.se>
>>
>> This is a squash of several commits, adding peripherals groups
>> configuration to r7s72100 device tree, and enabling some of them on
>> Genmai evaluation board
>>
>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>
> Thanks for the rework!
>
>>  arch/arm/boot/dts/r7s72100-genmai.dts |  51 ++++++++++++
>>  arch/arm/boot/dts/r7s72100.dtsi       | 151 ++++++++++++++++++++++++++++++++++
>
> This path should be split in multiple parts:
>   - Add the pfc node to r7s72100.dtsi,
>   - Add the gpio nodes to r7s72100.dtsi,
>   - 4 patches for r7s72100-genmai.dts, adding support for LEDs, SCIF,
>     Ethernet, and SPI.
>

I can easily separate the last 4 patches, but I don't feel like breaking 
in 2 parts the original patch that adds both pfc and gpio nodes to the 
.dtsi file.

This will look like:
	- Add pfc and gpio nodes to r7s72100.dtsi
	- 4 patches to add LEDs, SCIF, ETH and SPI to r7s72100-genmai.dts

Hope this is ok

Thanks
    j



>> --- a/arch/arm/boot/dts/r7s72100.dtsi
>> +++ b/arch/arm/boot/dts/r7s72100.dtsi
>> @@ -20,6 +20,19 @@
>>         #size-cells = <1>;
>>
>>         aliases {
>> +               gpio0 = &port0;
>> +               gpio1 = &port1;
>> +               gpio2 = &port2;
>> +               gpio3 = &port3;
>> +               gpio4 = &port4;
>> +               gpio5 = &port5;
>> +               gpio6 = &port6;
>> +               gpio7 = &port7;
>> +               gpio8 = &port8;
>> +               gpio9 = &port9;
>> +               gpio10 = &port10;
>> +               gpio11 = &port11;
>> +               gpio12 = &jtagport0;
>
> Please remove this hunk.
> GPIO aliases are deprecated, and I don't think the driver uses them.
>
>>                 i2c0 = &i2c0;
>>                 i2c1 = &i2c1;
>>                 i2c2 = &i2c2;
>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> 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
>

Re: [PATCH 3/3] arm: dts: r7s72100: Add peripherals nodes

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Monday 09 Jan 2017 20:31:58 Jacopo Mondi wrote:
> From: Magnus Damm <damm@opensource.se>
> 
> This is a squash of several commits, adding peripherals groups
> configuration to r7s72100 device tree, and enabling some of them on
> Genmai evaluation board
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> 
> -------------------------------------------------------------------
> 
> [REBASED] ARM: shmobile: r7s72100 GPIO and PINCTRL device nodes
> 
> Add support for r7s72100 PFC and GPIO device nodes port0 -> port11
> and jtagport0.
> 
> Signed-off-by: Magnus Damm <damm@opensource.se>
> 
> [REBASED] ARM: shmobile: Genmai SCIF2 PINCTRL configuration
> 
> Configure the r7s72100 PINCTRL hardware and select pin function
> for the SCIF2 serial console.
> 
> Signed-off-by: Magnus Damm <damm@opensource.se>
> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> [REBASED] ARM: shmobile: Genmai LED1 and LED2 support
> 
> Add support for Genmai board LED1 and LED2 via gpio-leds.
> 
> Signed-off-by: Magnus Damm <damm@opensource.se>
> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> ARM: shmobile: Genmai I2C-over-GPIO support
> 
> Add support for the Genmai I2C bus hooked up to P1_5 and P1_4 using
> the i2c-gpio driver. On the bus sits a 24c128 EEPRROM.
> 
> Signed-off-by: Magnus Damm <damm@opensource.se>
> 
> [REBASED] arm: shmobile: genmai: adapt dts to use native i2c driver
> 
> Switch from the gpio-driver to the shiny new native driver. Tested by
> accessing the eeprom on the genmai board.
> 
> Signed-off-by: Wolfram Sang <wsa@sang-engineering.com>
> Acked-by: Magnus Damm <damm@opensource.se>
> 
> [REBASED] ARM: shmobile: r7s72100: Add ethernet PFC node to DT
> 
> Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
> 
> [BLOCKED] ARM: shmobile: genmai reference dts: Add pinctrl for RSPI
> 
> Add pinctrl for the existing rspi4 node on Genmai.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> 
> [WIP] genmai sh_eth dts fixup
> ---
>  arch/arm/boot/dts/r7s72100-genmai.dts |  51 ++++++++++++
>  arch/arm/boot/dts/r7s72100.dtsi       | 151 +++++++++++++++++++++++++++++++
>  2 files changed, 202 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/r7s72100-genmai.dts
> b/arch/arm/boot/dts/r7s72100-genmai.dts index 118a8e2..59bccd3 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>
> 
>  / {
>  	model = "Genmai";
> @@ -34,6 +35,17 @@
>  		#address-cells = <1>;
>  		#size-cells = <1>;
>  	};
> +
> +	leds {
> +		compatible = "gpio-leds";
> +		led1 {
> +			gpios = <&port4 10 GPIO_ACTIVE_LOW>;
> +		};
> +		led2 {
> +			gpios = <&port4 11 GPIO_ACTIVE_LOW>;
> +		};
> +	};
> +
>  };
> 
>  &extal_clk {
> @@ -59,6 +71,45 @@
>  	};
>  };
> 
> +&pfc {
> +	pinctrl-0 = <&ethernet_pins &rspi4_pins &scif2_pins>;
> +	pinctrl-names = "default";

Shouldn't you add these properties to the ethernet, spi4 and scif2 nodes 
instead ?

> +
> +	ethernet_pins: ethernet {
> +		renesas,groups = "ethernet_rxdv_p3_6",
> +				 "ethernet_rxer_p3_5",
> +				 "ethernet_rxclk_p3_4",
> +				 "ethernet_mdio_p3_3",
> +				 "ethernet_rxd3_p2_11",
> +				 "ethernet_rxd2_p2_10",
> +				 "ethernet_rxd1_p2_9",
> +				 "ethernet_rxd0_p2_8",
> +				 "ethernet_txd3_p2_7",
> +				 "ethernet_txd2_p2_6",
> +				 "ethernet_txd1_p2_5",
> +				 "ethernet_txd0_p2_4",
> +				 "ethernet_txcrs_p2_3",
> +				 "ethernet_txen_p2_2",
> +				 "ethernet_txer_p2_1",
> +				 "ethernet_txclk_p2_0",
> +				 "ethernet_mdc_p5_9",
> +				 "ethernet_col_p1_14",
> +				 "ethernet_int_p1_15";
> +		renesas,function = "ethernet";
> +	};
> +
> +	rspi4_pins: spi4 {
> +		renesas,groups = "rspi4_rspck_p4_0", "rspi4_ssl0_p4_1",
> +				 "rspi4_mosi_p4_2", "rspi4_miso_p4_3";
> +		renesas,function = "rspi4";
> +	};
> +
> +	scif2_pins: serial2 {
> +		renesas,groups = "scif2_txd_p3_0", "scif2_rxd_p3_2";
> +		renesas,function = "scif2";
> +	};
> +};
> +
>  &scif2 {
>  	status = "okay";
>  };
> diff --git a/arch/arm/boot/dts/r7s72100.dtsi
> b/arch/arm/boot/dts/r7s72100.dtsi index 3dd427d..47bfd47 100644
> --- a/arch/arm/boot/dts/r7s72100.dtsi
> +++ b/arch/arm/boot/dts/r7s72100.dtsi
> @@ -20,6 +20,19 @@
>  	#size-cells = <1>;
> 
>  	aliases {
> +		gpio0 = &port0;
> +		gpio1 = &port1;
> +		gpio2 = &port2;
> +		gpio3 = &port3;
> +		gpio4 = &port4;
> +		gpio5 = &port5;
> +		gpio6 = &port6;
> +		gpio7 = &port7;
> +		gpio8 = &port8;
> +		gpio9 = &port9;
> +		gpio10 = &port10;
> +		gpio11 = &port11;
> +		gpio12 = &jtagport0;
>  		i2c0 = &i2c0;
>  		i2c1 = &i2c1;
>  		i2c2 = &i2c2;
> @@ -359,6 +372,144 @@
>  			<0xe8202000 0x1000>;
>  	};
> 
> +	pfc: pfc@fcfe3300 {
> +		compatible = "renesas,pfc-r7s72100";
> +		reg = <0xfcfe3300 0x400>, /* PM, PMC, PFC, PFCE */
> +		  	  <0xfcfe3a00 0x100>, /* PFCAE */
> +			  <0xfcfe7000 0x300>, /* PIBC, PBDC, PIPC */
> +			  <0xfcfe7b40 0x04>, /* JPMC */
> +			  <0xfcfe7b90 0x04>, /* JPMCSR */
> +			  <0xfcfe7f00 0x04>; /* JPIBC */
> +	};
> +
> +	port0: gpio@fcfe3100 {
> +		compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz";
> +		reg = <0xfcfe3100 0x4>, /* PSR */
> +		  	  <0xfcfe3200 0x2>, /* PPR */
> +			  <0xfcfe3800 0x4>; /* PMSR */
> +		#gpio-cells = <2>;
> +		gpio-controller;
> +		gpio-ranges = <&pfc 0 0 6>;
> +	};
> +
> +	port1: gpio@fcfe3104 {
> +		compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz";
> +		reg = <0xfcfe3104 0x4>, /* PSR */
> +		  	  <0xfcfe3204 0x2>, /* PPR */
> +			  <0xfcfe3804 0x4>; /* PMSR */
> +		#gpio-cells = <2>;
> +		gpio-controller;
> +		gpio-ranges = <&pfc 0 16 16>;
> +	};
> +
> +	port2: gpio@fcfe3108 {
> +		compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz";
> +		reg = <0xfcfe3108 0x4>, /* PSR */
> +		  	  <0xfcfe3208 0x2>, /* PPR */
> +			  <0xfcfe3808 0x4>; /* PMSR */
> +		#gpio-cells = <2>;
> +		gpio-controller;
> +		gpio-ranges = <&pfc 0 32 16>;
> +	};
> +
> +	port3: gpio@fcfe310c {
> +		compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz";
> +		reg = <0xfcfe310c 0x4>, /* PSR */
> +		  	  <0xfcfe320c 0x2>, /* PPR */
> +			  <0xfcfe380c 0x4>; /* PMSR */
> +		#gpio-cells = <2>;
> +		gpio-controller;
> +		gpio-ranges = <&pfc 0 48 16>;
> +	};
> +
> +	port4: gpio@fcfe3110 {
> +		compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz";
> +		reg = <0xfcfe3110 0x4>, /* PSR */
> +		  	  <0xfcfe3210 0x2>, /* PPR */
> +			  <0xfcfe3810 0x4>; /* PMSR */
> +		#gpio-cells = <2>;
> +		gpio-controller;
> +		gpio-ranges = <&pfc 0 64 16>;
> +	};
> +
> +	port5: gpio@fcfe3114 {
> +		compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz";
> +		reg = <0xfcfe3114 0x4>, /* PSR */
> +		  	  <0xfcfe3214 0x2>, /* PPR */
> +			  <0xfcfe3814 0x4>; /* PMSR */
> +		#gpio-cells = <2>;
> +		gpio-controller;
> +		gpio-ranges = <&pfc 0 80 11>;
> +	};
> +
> +	port6: gpio@fcfe3118 {
> +		compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz";
> +		reg = <0xfcfe3118 0x4>, /* PSR */
> +		  	  <0xfcfe3218 0x2>, /* PPR */
> +			  <0xfcfe3818 0x4>; /* PMSR */
> +		#gpio-cells = <2>;
> +		gpio-controller;
> +		gpio-ranges = <&pfc 0 96 16>;
> +	};
> +
> +	port7: gpio@fcfe311c {
> +		compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz";
> +		reg = <0xfcfe311c 0x4>, /* PSR */
> +		  	  <0xfcfe321c 0x2>, /* PPR */
> +			  <0xfcfe381c 0x4>; /* PMSR */
> +		#gpio-cells = <2>;
> +		gpio-controller;
> +		gpio-ranges = <&pfc 0 112 16>;
> +	};
> +
> +	port8: gpio@fcfe3120 {
> +		compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz";
> +		reg = <0xfcfe3120 0x4>, /* PSR */
> +		  	  <0xfcfe3220 0x2>, /* PPR */
> +			  <0xfcfe3820 0x4>; /* PMSR */
> +		#gpio-cells = <2>;
> +		gpio-controller;
> +		gpio-ranges = <&pfc 0 128 16>;
> +	};
> +
> +	port9: gpio@fcfe3124 {
> +		compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz";
> +		reg = <0xfcfe3124 0x4>, /* PSR */
> +		  	  <0xfcfe3224 0x2>, /* PPR */
> +			  <0xfcfe3824 0x4>; /* PMSR */
> +		#gpio-cells = <2>;
> +		gpio-controller;
> +		gpio-ranges = <&pfc 0 144 8>;
> +	};
> +
> +	port10: gpio@fcfe3128 {
> +		compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz";
> +		reg = <0xfcfe3128 0x4>, /* PSR */
> +		  	  <0xfcfe3228 0x2>, /* PPR */
> +			  <0xfcfe3828 0x4>; /* PMSR */
> +		#gpio-cells = <2>;
> +		gpio-controller;
> +		gpio-ranges = <&pfc 0 160 16>;
> +	};
> +
> +	port11: gpio@fcfe312c {
> +		compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz";
> +		reg = <0xfcfe312c 0x4>, /* PSR */
> +		  	  <0xfcfe322c 0x2>, /* PPR */
> +			  <0xfcfe382c 0x4>; /* PMSR */
> +		#gpio-cells = <2>;
> +		gpio-controller;
> +		gpio-ranges = <&pfc 0 176 16>;
> +	};
> +
> +	jtagport0: gpio@fcfe7b20 {
> +		compatible = "renesas,gpio-r7s72100", "renesas,gpio-rz";
> +		reg = <0xfcfe7b20 0x2>; /* JPPR */
> +		#gpio-cells = <2>;
> +		gpio-controller;
> +		gpio-ranges = <&pfc 0 192 2>;
> +	};
> +
>  	i2c0: i2c@fcfee000 {
>  		#address-cells = <1>;
>  		#size-cells = <0>;

-- 
Regards,

Laurent Pinchart

Re: [PATCH 0/3] Renesas RZ PFC and GPIO driver

[Laurent Pinchart]

Hi Jacopo,

On Monday 09 Jan 2017 20:31:55 Jacopo Mondi wrote:
> Hello,
>    these patches are the result of a squash and forward porting of Magnus'
> PFC and GPIO driver from
> git://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-drivers.git tree
> genmai-gpio-and-pfc branch to kernel v4.10
> 
> The PFC and GPIO driver have been forward ported to v4.10-rc1 with minor
> fixes and applied on top of renesas-driver master branch.
> 
> As I cannot locally verify most of the peripherals functionalities enabled
> by the creation of PFC and GPIO driver for RZ SoC series, testing performed
> by patches original authors has to be trusted.
> 
> Some possible fixes and functional expansion for the PFC driver will follow
> in an RFC series to be applied on top of this one.

Pin control on the Renesas RZ chips is performed per pin instead of per 
function (but unfortunately with the various bits of configuration split 
across a bunch of registers, otherwise we could have used pinctrl-single). 
This gives us an opportunity to move away from the sh-pfc awful (but more or 
less needed for the R-Car family) architecture and implement something much, 
much cleaner without all those obscure data tables and macros, with per-pin 
configuration. Shouldn't we rejoice and embrace that opportunity ?

> Magnus Damm (3):
>   pinctrl: sh-pfc: Add r7s72100 PFC driver
>   gpio: gpio-rz: GPIO driver for Renesas RZ series
>   arm: dts: r7s72100: Add peripherals nodes
> 
>  .../bindings/pinctrl/renesas,pfc-pinctrl.txt       |   1 +
>  arch/arm/boot/dts/r7s72100-genmai.dts              |  51 ++
>  arch/arm/boot/dts/r7s72100.dtsi                    | 151 ++++++
>  drivers/gpio/Kconfig                               |   6 +
>  drivers/gpio/Makefile                              |   1 +
>  drivers/gpio/gpio-rz.c                             | 212 +++++++++
>  drivers/pinctrl/sh-pfc/Kconfig                     |   5 +
>  drivers/pinctrl/sh-pfc/Makefile                    |   1 +
>  drivers/pinctrl/sh-pfc/core.c                      |   9 +
>  drivers/pinctrl/sh-pfc/pfc-r7s72100.c              | 529 ++++++++++++++++++
>  drivers/pinctrl/sh-pfc/sh_pfc.h                    |   9 +-
>  11 files changed, 972 insertions(+), 3 deletions(-)
>  create mode 100644 drivers/gpio/gpio-rz.c
>  create mode 100644 drivers/pinctrl/sh-pfc/pfc-r7s72100.c

-- 
Regards,

Laurent Pinchart

RE: [PATCH 0/3] Renesas RZ PFC and GPIO driver

[Chris Brandt]

On Monday, January 09, 2017, Laurent Pinchart wrote:
> Pin control on the Renesas RZ chips is performed per pin instead of per
> function (but unfortunately with the various bits of configuration split
> across a bunch of registers, otherwise we could have used pinctrl-single).

I will say this...'spoilier alert'...you're not going to see this particular
PFC HW much anymore in the RZA series.

It will still be a 'per pin control' like the existing RZ/A1, but the bits
will not be spread out over a bunch of registers. So something like
pinctrl-single should work.

No idea about the future of RZG series.


> This gives us an opportunity to move away from the sh-pfc awful (but more
> or less needed for the R-Car family) architecture and implement something
> much, much cleaner without all those obscure data tables and macros, with
> per-pin configuration. Shouldn't we rejoice and embrace that opportunity ?

Honestly, that driver structure confuses the heck out of me. I guess you have
to understand the restrictions of the R-Car PFC HW to really appreciate it.

Of course I like the idea of a simple PFC for Renesas SoCs, of course the
existing RZ/A1 would be (hopefully) the worst case scenario.

I assume to make a one-size-fits-all per-pin driver, instead of filling out
structures and enums, you'd have to have callback functions that basically you
pass "set pin X to function #3"...which basically brings you back to what the
core of the pinctrl system does now any (if I understand it correctly).

Chris

Re: [PATCH 0/3] Renesas RZ PFC and GPIO driver

[Laurent Pinchart]

Hi Chris,

On Monday 09 Jan 2017 21:08:49 Chris Brandt wrote:
> On Monday, January 09, 2017, Laurent Pinchart wrote:
> > Pin control on the Renesas RZ chips is performed per pin instead of per
> > function (but unfortunately with the various bits of configuration split
> > across a bunch of registers, otherwise we could have used pinctrl-single).
> 
> I will say this...'spoilier alert'...you're not going to see this particular
> PFC HW much anymore in the RZA series.
> 
> It will still be a 'per pin control' like the existing RZ/A1, but the bits
> will not be spread out over a bunch of registers. So something like
> pinctrl-single should work.

Both the existing RZ/A model and the pinctrl model are in my opinion 
improvements over the RZ/G and R-Car models. I don't care much about whether 
pinctrl-single can be used, but we really need hardware architectures that 
don't require those huge data tables.

> No idea about the future of RZG series.

I'll cross my fingers.

> > This gives us an opportunity to move away from the sh-pfc awful (but more
> > or less needed for the R-Car family) architecture and implement something
> > much, much cleaner without all those obscure data tables and macros, with
> > per-pin configuration. Shouldn't we rejoice and embrace that opportunity ?
> 
> Honestly, that driver structure confuses the heck out of me. I guess you
> have to understand the restrictions of the R-Car PFC HW to really
> appreciate it.

No, you can't appreciate it, no matter what :-)

> Of course I like the idea of a simple PFC for Renesas SoCs, of course the
> existing RZ/A1 would be (hopefully) the worst case scenario.
> 
> I assume to make a one-size-fits-all per-pin driver, instead of filling out
> structures and enums, you'd have to have callback functions that basically
> you pass "set pin X to function #3"...which basically brings you back to
> what the core of the pinctrl system does now any (if I understand it
> correctly).

It's more complex than that. Both the pin-based and group-based models have 
their pros and cons, and the pinctrl API is some kind of mix that allows both 
options.

-- 
Regards,

Laurent Pinchart

RE: [PATCH 0/3] Renesas RZ PFC and GPIO driver

[Chris Brandt]

Hi Laurent,

On Monday, January 09, 2017, Laurent Pinchart wrote:
> Both the existing RZ/A model and the pinctrl model are in my opinion
> improvements over the RZ/G and R-Car models. I don't care much about
> whether pinctrl-single can be used, but we really need hardware
> architectures that don't require those huge data tables.

I can definitely agree to that.


> It's more complex than that. Both the pin-based and group-based models
> have their pros and cons, and the pinctrl API is some kind of mix that
> allows both options.

Here is my general question: Which of these 2 approaches are better?

A) In the DT, the user ask "enable Ethernet please" and magic happens in
   the pfc driver.

B) In the DT, the user looks up the correct pin/function assignments in
   the SoC Hardware Manual and manually spells out what they need.

R-Car looks more like A. I've been using a driver that looks more like B.

For most drivers (USB, MMC, SDHI, etc..,), I'm happy when 'magic happens'
and I don't really have to open a HW manual at all.
But, for something like setting up the PFC when someone gets a shiny new
board, making people actually open a HW manual seems acceptable to me.



Chris

Re: [PATCH 0/3] Renesas RZ PFC and GPIO driver

[Laurent Pinchart]

Hi Chris,

On Monday 09 Jan 2017 23:53:39 Chris Brandt wrote:
> On Monday, January 09, 2017, Laurent Pinchart wrote:
> > Both the existing RZ/A model and the pinctrl model are in my opinion
> > improvements over the RZ/G and R-Car models. I don't care much about
> > whether pinctrl-single can be used, but we really need hardware
> > architectures that don't require those huge data tables.
> 
> I can definitely agree to that.
> 
> > It's more complex than that. Both the pin-based and group-based models
> > have their pros and cons, and the pinctrl API is some kind of mix that
> > allows both options.
> 
> Here is my general question: Which of these 2 approaches are better?
> 
> A) In the DT, the user ask "enable Ethernet please" and magic happens in
>    the pfc driver.
> 
> B) In the DT, the user looks up the correct pin/function assignments in
>    the SoC Hardware Manual and manually spells out what they need.
> 
> R-Car looks more like A. I've been using a driver that looks more like B.
> 
> For most drivers (USB, MMC, SDHI, etc..,), I'm happy when 'magic happens'
> and I don't really have to open a HW manual at all.
> But, for something like setting up the PFC when someone gets a shiny new
> board, making people actually open a HW manual seems acceptable to me.

>From a user point of view, option A looks better to me. However, it has two 
drawbacks:

- Through deciding what pin groups we make available we create a DT ABI that 
will make it difficult to fix mistakes in case the groups are not fine-grained 
enough.

- The data tables in C code are large, and we end up compiling many of them in 
multi-platform kernel, significantly increasing the kernel size.

I would thus favour option B.

-- 
Regards,

Laurent Pinchart

RE: [PATCH 0/3] Renesas RZ PFC and GPIO driver

[Chris Brandt]

Hi Laurent,

On Monday, January 09, 2017, Laurent Pinchart wrote:
> From a user point of view, option A looks better to me. However, it has
> two
> drawbacks:
> 
> - Through deciding what pin groups we make available we create a DT ABI
> that will make it difficult to fix mistakes in case the groups are not
> fine-grained enough.
> 
> - The data tables in C code are large, and we end up compiling many of
> them in multi-platform kernel, significantly increasing the kernel size.
> 
> I would thus favour option B.

I'm going to have a good read through the pinctrl documentation in the kernel
and have a look at the pinctrl-single driver and see if I can come up with
something that looks like option B.

Also, there are lots of drivers in the pinctrl directory, so I'll have a look
at what other SoCs vendors are doing to see if there are any good ideas there.

I can tell you that some of the other Renesas SoCs in pinctrl/sh-pfc like sh7757
and sh7724 are also per pin function type parts and might have worked better
with an option B type driver.
NOTE: I'm not saying we update those old crusty parts, but rather the R-Car
      PFC might be more non-standard.

Regards,
Chris

Re: [PATCH 0/3] Renesas RZ PFC and GPIO driver

[jacopo mondi]

Hi Laurent, Chris,

On 10/01/2017 02:28, Laurent Pinchart wrote:
> Hi Chris,
>
> On Monday 09 Jan 2017 23:53:39 Chris Brandt wrote:
>> On Monday, January 09, 2017, Laurent Pinchart wrote:
>>> Both the existing RZ/A model and the pinctrl model are in my opinion
>>> improvements over the RZ/G and R-Car models. I don't care much about
>>> whether pinctrl-single can be used, but we really need hardware
>>> architectures that don't require those huge data tables.
>>
>> I can definitely agree to that.
>>
>>> It's more complex than that. Both the pin-based and group-based models
>>> have their pros and cons, and the pinctrl API is some kind of mix that
>>> allows both options.
>>
>> Here is my general question: Which of these 2 approaches are better?
>>
>> A) In the DT, the user ask "enable Ethernet please" and magic happens in
>>    the pfc driver.
>>
>> B) In the DT, the user looks up the correct pin/function assignments in
>>    the SoC Hardware Manual and manually spells out what they need.
>>
>> R-Car looks more like A. I've been using a driver that looks more like B.
>>
>> For most drivers (USB, MMC, SDHI, etc..,), I'm happy when 'magic happens'
>> and I don't really have to open a HW manual at all.
>> But, for something like setting up the PFC when someone gets a shiny new
>> board, making people actually open a HW manual seems acceptable to me.
>
> From a user point of view, option A looks better to me. However, it has two
> drawbacks:
>
> - Through deciding what pin groups we make available we create a DT ABI that
> will make it difficult to fix mistakes in case the groups are not fine-grained
> enough.
>
> - The data tables in C code are large, and we end up compiling many of them in
> multi-platform kernel, significantly increasing the kernel size.
>
> I would thus favour option B.
>

That would be saner, I agree.

And a much saner way of doing this would be, in my understanding, not 
using the group-based sh-pfc drivers used for R-Car and back it up with 
a pin-based equivalent, where to hook drivers for each specific hardware.

Looks like pinmux-single, yes, but that driver bets on the ability to 
set pin functions and configurations with a write to a single register 
while, at least for RZ/A, the same is scattered among several registers 
(I may be wrong on the single register requirement for pinctrl-single 
though)

So, I guess what direction to take depends on whether or not we're going 
to see more hardware with a per-pin configuration that would benefit 
from this new rz-pfc driver (it seems so) and if this justify splitting 
sh-pfc in two, a group-based one for R-Car devices (and all devices 
there already) and a new pin-based one.

Or maybe we can tie pin-based configuration in sh-pfc and it's me not 
seeing how to do that.

Thanks
    j

RE: [PATCH 0/3] Renesas RZ PFC and GPIO driver

[Chris Brandt]

Hi Jacopo,

On Tuesday, January 10, 2017, Jacopo Mondi wrote:
> So, I guess what direction to take depends on whether or not we're going
> to see more hardware with a per-pin configuration that would benefit from
> this new rz-pfc driver (it seems so) and if this justify splitting sh-pfc
> in two, a group-based one for R-Car devices (and all devices there
> already) and a new pin-based one.

Well, since I'm the one with the "renesas.com" email address, I'll try to
see if I can figure out if the R-Car PFC can be changed to something else for
R-Car Gen4. Same goes for RZ/G. Sometimes the chip designers just keep doing
the same thing over and over again because they don't get any feedback from
the SW guys.


> Or maybe we can tie pin-based configuration in sh-pfc and it's me not
> seeing how to do that.

At some point....we do need to leave the "sh-" name behind as I don't really
see much of a future for SH4 devices. Using something like pinctrl-renesas
is probably a better name. Although, anyone who's watched the semiconductor
business over the years knows that names never stay the same. (sigh)


Chris

Re: [PATCH 0/3] Renesas RZ PFC and GPIO driver

[Simon Horman]

On Wed, Jan 11, 2017 at 12:46:31AM +0000, Chris Brandt wrote:
> Hi Jacopo,
> 
> On Tuesday, January 10, 2017, Jacopo Mondi wrote:
> > So, I guess what direction to take depends on whether or not we're going
> > to see more hardware with a per-pin configuration that would benefit from
> > this new rz-pfc driver (it seems so) and if this justify splitting sh-pfc
> > in two, a group-based one for R-Car devices (and all devices there
> > already) and a new pin-based one.
> 
> Well, since I'm the one with the "renesas.com" email address, I'll try to
> see if I can figure out if the R-Car PFC can be changed to something else for
> R-Car Gen4. Same goes for RZ/G. Sometimes the chip designers just keep doing
> the same thing over and over again because they don't get any feedback from
> the SW guys.
> 
> 
> > Or maybe we can tie pin-based configuration in sh-pfc and it's me not
> > seeing how to do that.
> 
> At some point....we do need to leave the "sh-" name behind as I don't really
> see much of a future for SH4 devices. Using something like pinctrl-renesas
> is probably a better name. Although, anyone who's watched the semiconductor
> business over the years knows that names never stay the same. (sigh)

FWIW I agree "renesas-" makes more sense than "sh-" for all Renesas ARM
based SoCs.

Re: [PATCH 0/3] Renesas RZ PFC and GPIO driver

[Laurent Pinchart]

Hi Jacopo,

On Tuesday 10 Jan 2017 23:32:27 jacopo mondi wrote:
> On 10/01/2017 02:28, Laurent Pinchart wrote:
> > On Monday 09 Jan 2017 23:53:39 Chris Brandt wrote:
> >> On Monday, January 09, 2017, Laurent Pinchart wrote:
> >>> Both the existing RZ/A model and the pinctrl model are in my opinion
> >>> improvements over the RZ/G and R-Car models. I don't care much about
> >>> whether pinctrl-single can be used, but we really need hardware
> >>> architectures that don't require those huge data tables.
> >> 
> >> I can definitely agree to that.
> >> 
> >>> It's more complex than that. Both the pin-based and group-based models
> >>> have their pros and cons, and the pinctrl API is some kind of mix that
> >>> allows both options.
> >> 
> >> Here is my general question: Which of these 2 approaches are better?
> >> 
> >> A) In the DT, the user ask "enable Ethernet please" and magic happens in
> >>    the pfc driver.
> >> 
> >> B) In the DT, the user looks up the correct pin/function assignments in
> >>    the SoC Hardware Manual and manually spells out what they need.
> >> 
> >> R-Car looks more like A. I've been using a driver that looks more like B.
> >> 
> >> For most drivers (USB, MMC, SDHI, etc..,), I'm happy when 'magic happens'
> >> and I don't really have to open a HW manual at all.
> >> But, for something like setting up the PFC when someone gets a shiny new
> >> board, making people actually open a HW manual seems acceptable to me.
> > 
> > From a user point of view, option A looks better to me. However, it has
> > two drawbacks:
> > 
> > - Through deciding what pin groups we make available we create a DT ABI
> > that will make it difficult to fix mistakes in case the groups are not
> > fine-grained enough.
> > 
> > - The data tables in C code are large, and we end up compiling many of
> > them in multi-platform kernel, significantly increasing the kernel size.
> > 
> > I would thus favour option B.
> 
> That would be saner, I agree.
> 
> And a much saner way of doing this would be, in my understanding, not
> using the group-based sh-pfc drivers used for R-Car and back it up with
> a pin-based equivalent, where to hook drivers for each specific hardware.

We can't really do that for the existing SoCs as the concept of groups is 
present in the hardware. Not only do we need to select per-pin functions, but 
there are also register bits that perform pin muxing for whole groups. If this 
changes in future generations of the SoCs we then could do away with the data 
tables, but for now we're stuck with them.

> Looks like pinmux-single, yes, but that driver bets on the ability to
> set pin functions and configurations with a write to a single register
> while, at least for RZ/A, the same is scattered among several registers
> (I may be wrong on the single register requirement for pinctrl-single
> though)
> 
> So, I guess what direction to take depends on whether or not we're going
> to see more hardware with a per-pin configuration that would benefit
> from this new rz-pfc driver (it seems so) and if this justify splitting
> sh-pfc in two, a group-based one for R-Car devices (and all devices
> there already) and a new pin-based one.
> 
> Or maybe we can tie pin-based configuration in sh-pfc and it's me not
> seeing how to do that.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 0/3] Renesas RZ PFC and GPIO driver

[jacopo mondi]

Hi Laurent,

On 11/01/2017 12:22, Laurent Pinchart wrote:
> Hi Jacopo,
>

[snip]

>>>>
>>>> Here is my general question: Which of these 2 approaches are better?
>>>>
>>>> A) In the DT, the user ask "enable Ethernet please" and magic happens in
>>>>    the pfc driver.
>>>>
>>>> B) In the DT, the user looks up the correct pin/function assignments in
>>>>    the SoC Hardware Manual and manually spells out what they need.
>>>>
>>>> R-Car looks more like A. I've been using a driver that looks more like B.
>>>>
>>>> For most drivers (USB, MMC, SDHI, etc..,), I'm happy when 'magic happens'
>>>> and I don't really have to open a HW manual at all.
>>>> But, for something like setting up the PFC when someone gets a shiny new
>>>> board, making people actually open a HW manual seems acceptable to me.
>>>
>>> From a user point of view, option A looks better to me. However, it has
>>> two drawbacks:
>>>
>>> - Through deciding what pin groups we make available we create a DT ABI
>>> that will make it difficult to fix mistakes in case the groups are not
>>> fine-grained enough.
>>>
>>> - The data tables in C code are large, and we end up compiling many of
>>> them in multi-platform kernel, significantly increasing the kernel size.
>>>
>>> I would thus favour option B.
>>
>> That would be saner, I agree.
>>
>> And a much saner way of doing this would be, in my understanding, not
>> using the group-based sh-pfc drivers used for R-Car and back it up with
>> a pin-based equivalent, where to hook drivers for each specific hardware.
>
> We can't really do that for the existing SoCs as the concept of groups is
> present in the hardware. Not only do we need to select per-pin functions, but
> there are also register bits that perform pin muxing for whole groups. If this
> changes in future generations of the SoCs we then could do away with the data
> tables, but for now we're stuck with them.
>

Well, for the RZ/A platform "groups" are really pins themselves for what 
I see in dts and pinctrl driver.

-------------------------------------------------------------------
Eg. SCIF2

From: arch/arm/boot/dts/r7s72100-genmai.dts
	scif2_pins: serial2 {
		renesas,groups = "scif2_txd_p3_0", "scif2_rxd_p3_2";
		renesas,function = "scif2";
	};

From: drivers/pinctrl/sh-pfc/pfc-r7s72100.c
#define SCIF2(fn)			\
	fn(scif2, clk, 3, 0, 4)		\
	fn(scif2, txd, 3, 1, 4)		\
	*fn(scif2, rxd, 3, 2, 4)	\*
	*fn(scif2, txd, 3, 0, 6)	\*
	fn(scif2, clk, 4, 1, 5)		\
	fn(scif2, txd, 4, 2, 5)		\
	fn(scif2, rxd, 4, 3, 5)		\
	fn(scif2, txd, 4, 14, 7)	\
	fn(scif2, rxd, 4, 15, 7)	\
	fn(scif2, txd, 6, 2, 7)		\
	fn(scif2, rxd, 6, 3, 7)		\
	fn(scif2, clk, 8, 3, 7)		\
	fn(scif2, rxd, 8, 4, 7)		\
	fn(scif2, txd, 8, 6, 7)
-------------------------------------------------------------------

It seems to me that "renesas,function" is used to identify all "groups" 
(aka pins here) that can be made to work in SCIF2 mode (the 14 pins 
above) and "renesas,groups" identifies which pins to configure for the 
desired function (the alternate function number is the last entry of the 
"fn(function, name, bank,  pin, function)" macro).

Long story short: this ends up writing a bunch of registers for each 
single "group" (pin)

-------------------------------------------------------------------
Eg.

scif2_rxd_p3_2 configuration as function #4 writes registers

PMC3[2] = 1, PFC3[2] = 1, PFCE3[2] = 1, PFCAE3[2] = 0

scif2_txd_p3_0 configuration as function #6 writes registers

PMC3[2] = 1, PFC3[2] = 1, PFCE3[2] = 0, PFCAE3[2] = 1

And no global group configuration register has to be set, as it happens 
instead for R-Car series.
-------------------------------------------------------------------

As shown in the RFC series "pinctrl: sh-pfc: r7s72100: Add IO mode 
selection" I sent out along with this one, this model is not flexible 
enough, and the attempt I proposed to extend it to support additional 
pin configuration options ends up multiplying the already gigantic macro 
tables.

I was wondering if that should not be be turned into something more 
similar to the pinctrl-single defined ABI. In that case it would look 
like this (please excuse the pseduo-dts syntax here)

  	scif2_pins: serial2 {
		renesas,pin = <3, 2, MUX_MODE4, PIN_CONFIG_OPTIONS,
			       3, 0, MUX_MODE6, PIN_CONFIG_OPTIONS>
	};

I'm under the impression that pinctrl-single would almost do, if not for 
the write function bit, that for OMAP platform results in a write to a 
single register, while (for RZ/A at least) we have to spread it on a 
bunch of different registers.

Also, Chris has some examples of this in his BSPs iirc, so I guess there 
is code for the RZ series out there already implementing this per-pin ABI...

Sorry for the long email, hope it did not frustrate anyone.
    j


>> Looks like pinmux-single, yes, but that driver bets on the ability to
>> set pin functions and configurations with a write to a single register
>> while, at least for RZ/A, the same is scattered among several registers
>> (I may be wrong on the single register requirement for pinctrl-single
>> though)
>>
>> So, I guess what direction to take depends on whether or not we're going
>> to see more hardware with a per-pin configuration that would benefit
>> from this new rz-pfc driver (it seems so) and if this justify splitting
>> sh-pfc in two, a group-based one for R-Car devices (and all devices
>> there already) and a new pin-based one.
>>
>> Or maybe we can tie pin-based configuration in sh-pfc and it's me not
>> seeing how to do that.
>

Re: [PATCH 0/3] Renesas RZ PFC and GPIO driver

[Laurent Pinchart]

Hi Jacopo,

On Wednesday 11 Jan 2017 16:15:30 jacopo mondi wrote:
> On 11/01/2017 12:22, Laurent Pinchart wrote:
> > Hi Jacopo,
> 
> [snip]
> 
> >>>> Here is my general question: Which of these 2 approaches are better?
> >>>> 
> >>>> A) In the DT, the user ask "enable Ethernet please" and magic happens
> >>>>    in the pfc driver.
> >>>> 
> >>>> B) In the DT, the user looks up the correct pin/function assignments in
> >>>>    the SoC Hardware Manual and manually spells out what they need.
> >>>> 
> >>>> R-Car looks more like A. I've been using a driver that looks more like
> >>>> B.
> >>>> 
> >>>> For most drivers (USB, MMC, SDHI, etc..,), I'm happy when 'magic
> >>>> happens' and I don't really have to open a HW manual at all.
> >>>> But, for something like setting up the PFC when someone gets a shiny
> >>>> new board, making people actually open a HW manual seems acceptable to
> >>>> me.
> >>> 
> >>> From a user point of view, option A looks better to me. However, it has
> >>> two drawbacks:
> >>> 
> >>> - Through deciding what pin groups we make available we create a DT ABI
> >>> that will make it difficult to fix mistakes in case the groups are not
> >>> fine-grained enough.
> >>> 
> >>> - The data tables in C code are large, and we end up compiling many of
> >>> them in multi-platform kernel, significantly increasing the kernel size.
> >>> 
> >>> I would thus favour option B.
> >> 
> >> That would be saner, I agree.
> >> 
> >> And a much saner way of doing this would be, in my understanding, not
> >> using the group-based sh-pfc drivers used for R-Car and back it up with
> >> a pin-based equivalent, where to hook drivers for each specific hardware.
> > 
> > We can't really do that for the existing SoCs as the concept of groups is
> > present in the hardware. Not only do we need to select per-pin functions,
> > but there are also register bits that perform pin muxing for whole
> > groups. If this changes in future generations of the SoCs we then could
> > do away with the data tables, but for now we're stuck with them.
> 
> Well, for the RZ/A platform "groups" are really pins themselves for what
> I see in dts and pinctrl driver.

On RZ/A, yes. My comment was related to the R-Car Gen2 and Gen3 families.

> -------------------------------------------------------------------
> Eg. SCIF2
> 
> From: arch/arm/boot/dts/r7s72100-genmai.dts
> 	scif2_pins: serial2 {
> 		renesas,groups = "scif2_txd_p3_0", "scif2_rxd_p3_2";
> 		renesas,function = "scif2";
> 	};
> 
> From: drivers/pinctrl/sh-pfc/pfc-r7s72100.c
> #define SCIF2(fn)			\
> 	fn(scif2, clk, 3, 0, 4)		\
> 	fn(scif2, txd, 3, 1, 4)		\
> 	*fn(scif2, rxd, 3, 2, 4)	\*
> 	*fn(scif2, txd, 3, 0, 6)	\*
> 	fn(scif2, clk, 4, 1, 5)		\
> 	fn(scif2, txd, 4, 2, 5)		\
> 	fn(scif2, rxd, 4, 3, 5)		\
> 	fn(scif2, txd, 4, 14, 7)	\
> 	fn(scif2, rxd, 4, 15, 7)	\
> 	fn(scif2, txd, 6, 2, 7)		\
> 	fn(scif2, rxd, 6, 3, 7)		\
> 	fn(scif2, clk, 8, 3, 7)		\
> 	fn(scif2, rxd, 8, 4, 7)		\
> 	fn(scif2, txd, 8, 6, 7)
> -------------------------------------------------------------------
> 
> It seems to me that "renesas,function" is used to identify all "groups"
> (aka pins here) that can be made to work in SCIF2 mode (the 14 pins
> above) and "renesas,groups" identifies which pins to configure for the
> desired function (the alternate function number is the last entry of the
> "fn(function, name, bank,  pin, function)" macro).
> 
> Long story short: this ends up writing a bunch of registers for each
> single "group" (pin)
> 
> -------------------------------------------------------------------
> Eg.
> 
> scif2_rxd_p3_2 configuration as function #4 writes registers
> 
> PMC3[2] = 1, PFC3[2] = 1, PFCE3[2] = 1, PFCAE3[2] = 0
> 
> scif2_txd_p3_0 configuration as function #6 writes registers
> 
> PMC3[2] = 1, PFC3[2] = 1, PFCE3[2] = 0, PFCAE3[2] = 1
> 
> And no global group configuration register has to be set, as it happens
> instead for R-Car series.
> -------------------------------------------------------------------
> 
> As shown in the RFC series "pinctrl: sh-pfc: r7s72100: Add IO mode
> selection" I sent out along with this one, this model is not flexible
> enough, and the attempt I proposed to extend it to support additional
> pin configuration options ends up multiplying the already gigantic macro
> tables.
> 
> I was wondering if that should not be be turned into something more
> similar to the pinctrl-single defined ABI. In that case it would look
> like this (please excuse the pseduo-dts syntax here)
> 
>   	scif2_pins: serial2 {
> 		renesas,pin = <3, 2, MUX_MODE4, PIN_CONFIG_OPTIONS,
> 			       3, 0, MUX_MODE6, PIN_CONFIG_OPTIONS>

This should just be "pin", not "renesas,pin".

> 	};

Yes, I believe that should be done, especially given that the datasheet for 
the SoC is public. For other SoCs the current PFC driver model exposes the 
information needed to configure pin muxing even if you can't access the 
datasheet, which is an option that I'd hate losing.

> I'm under the impression that pinctrl-single would almost do, if not for
> the write function bit, that for OMAP platform results in a write to a
> single register, while (for RZ/A at least) we have to spread it on a
> bunch of different registers.

That's correct, but a pinctrl-single-like driver could be implemented 
relatively easily without all the data tables. We might still need some data 
if we want to validate DT entries though, for instance to store the valid pin 
config and/or options for each pin.

> Also, Chris has some examples of this in his BSPs iirc, so I guess there
> is code for the RZ series out there already implementing this per-pin ABI...
> 
> Sorry for the long email, hope it did not frustrate anyone.
> 
> >> Looks like pinmux-single, yes, but that driver bets on the ability to
> >> set pin functions and configurations with a write to a single register
> >> while, at least for RZ/A, the same is scattered among several registers
> >> (I may be wrong on the single register requirement for pinctrl-single
> >> though)
> >> 
> >> So, I guess what direction to take depends on whether or not we're going
> >> to see more hardware with a per-pin configuration that would benefit
> >> from this new rz-pfc driver (it seems so) and if this justify splitting
> >> sh-pfc in two, a group-based one for R-Car devices (and all devices
> >> there already) and a new pin-based one.
> >> 
> >> Or maybe we can tie pin-based configuration in sh-pfc and it's me not
> >> seeing how to do that.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 0/3] Renesas RZ PFC and GPIO driver

[jacopo mondi]

Hi Laurent,

On 11/01/2017 17:31, Laurent Pinchart wrote:
> Hi Jacopo,

[snip]

>>
>> I was wondering if that should not be be turned into something more
>> similar to the pinctrl-single defined ABI. In that case it would look
>> like this (please excuse the pseduo-dts syntax here)
>>
>>   	scif2_pins: serial2 {
>> 		renesas,pin = <3, 2, MUX_MODE4, PIN_CONFIG_OPTIONS,
>> 			       3, 0, MUX_MODE6, PIN_CONFIG_OPTIONS>
>
> This should just be "pin", not "renesas,pin".
>
>> 	};
>
> Yes, I believe that should be done, especially given that the datasheet for
> the SoC is public. For other SoCs the current PFC driver model exposes the
> information needed to configure pin muxing even if you can't access the
> datasheet, which is an option that I'd hate losing.
>

Just to point out that this proposed change won't affect the existing 
R-Car devices, but would apply to RZ/A-like devices currently not 
supported and which may appear in future (and according to Chris, to 
some old SH ones which no one wants to touch ;P )

I'm thinking about something like

drivers/pinctrl/sh-pfc/ (which would be better re-named as rcar-pfc)
drivers/pinctrl/rz-pfc/

But that's a big change that may upset some people here :)

>> I'm under the impression that pinctrl-single would almost do, if not for
>> the write function bit, that for OMAP platform results in a write to a
>> single register, while (for RZ/A at least) we have to spread it on a
>> bunch of different registers.
>
> That's correct, but a pinctrl-single-like driver could be implemented
> relatively easily without all the data tables. We might still need some data
> if we want to validate DT entries though, for instance to store the valid pin
> config and/or options for each pin.
>

A big difference from what pinctrl-single does and what the table-based 
approach does is that it creates groups, pins and functions at run-time, 
parsing the dts...
There's indeed quite some code there for doing this, but it does not 
perform any validation of what the dts describes.
Maybe we can skip all that part (parsing and run-time generation of 
groups, pins and function) using tables, and adopt a pin-oriented ABI 
that would allow us to specify in the dts the function and the per-pin 
configuration that would look more natural for this kind of devices.

As a final note: maybe we need to decide what to do with this series, 
people is spending time reviewing it, and I've just sent v2 out. If we 
want to go in a different direction, we should stop here :)

Thanks
    j

RE: [PATCH 0/3] Renesas RZ PFC and GPIO driver

[Chris Brandt]

Hi Jacopo and Laurent,

On Wednesday, January 11, 2017, jacopo mondi wrote:
> A big difference from what pinctrl-single does and what the table-based
> approach does is that it creates groups, pins and functions at run-time,
> parsing the dts...

See, this makes me think that this will chew up even more system memory.

The thing about the RZ/A1 series is that it has internal RAM and we use
that along with XIP_KERNEL in order to build systems that do not require
external RAM.
So in the end, even if we can get sh-pfc to functionally work for RZ/A1,
it would be pretty sad if the device driver that uses the most system
memory is one that is really only used at boot time just to change some
pin configurations.


> As a final note: maybe we need to decide what to do with this series,
> people is spending time reviewing it, and I've just sent v2 out. If we
> want to go in a different direction, we should stop here :)

Personally, for the RZ/A series, I don't see the sh-pfc driver as a long
term solution. If people need something in the kernel today to general
testing, that's one thing. But if someone starts laying out a board
with any differences from the Renesas reference design, they might be
in for a rude awaking when trying to put together a BSP for their board.


If people really want this sh-pcf based driver upstreamed, I will try
to do some testing on my boards to make sure it at least does what it
is supposed to do. (wait for V3 patch set???)

Even so, I still plan on mocking up a new driver with the ideas from
this discussion (create a renesas-pfc.c) as a long term solution for
the RZ/A series. Whether that new driver architecture is worth upstreaming
or not, I guess we'll have to wait and see.

Cheers

Chris