[PATCH v2 0/3] pinctrl: Adding support for Microchip/Microsemi serial GPIO controller

[PATCH v2 0/3] pinctrl: Adding support for Microchip/Microsemi serial GPIO controller

[Lars Povlsen]

The series add support for the serial GPIO controller used by
Microchip Sparx5, as well as (MSCC) ocelot/jaguar2 SoCs.

This is the second revision of the patch series.

v2 changes:
- Adds both in and output modes.
- Use direct adressing of the individual banks (#gpio-cells = <4>),
  also osoleting need for addressing macros in bindings include file.
- Property 'microchip,sgpio-ports' (uint32, bitmask) replaced by
  proper range set (array of [start,end]) 'microchip,sgpio-port-ranges'.
- Fixes whitespace issues in Kconfig file

Lars Povlsen (3):
  dt-bindings: pinctrl: Add bindings for pinctrl-mchp-sgpio driver
  pinctrl: pinctrl-mchp-sgpio: Add pinctrl driver for Microsemi Serial
    GPIO
  arm64: dts: sparx5: Add SGPIO devices

 .../pinctrl/microchip,sparx5-sgpio.yaml       |  88 +++
 MAINTAINERS                                   |   1 +
 arch/arm64/boot/dts/microchip/sparx5.dtsi     |  52 ++
 .../boot/dts/microchip/sparx5_pcb125.dts      |   5 +
 .../dts/microchip/sparx5_pcb134_board.dtsi    |   5 +
 .../dts/microchip/sparx5_pcb135_board.dtsi    |   5 +
 drivers/pinctrl/Kconfig                       |  17 +
 drivers/pinctrl/Makefile                      |   1 +
 drivers/pinctrl/pinctrl-mchp-sgpio.c          | 642 ++++++++++++++++++
 include/dt-bindings/gpio/mchp-sgpio.h         |  19 +
 10 files changed, 835 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/microchip,sparx5-sgpio.yaml
 create mode 100644 drivers/pinctrl/pinctrl-mchp-sgpio.c
 create mode 100644 include/dt-bindings/gpio/mchp-sgpio.h

--
2.27.0

[PATCH v2 1/3] dt-bindings: pinctrl: Add bindings for pinctrl-mchp-sgpio driver

[Lars Povlsen]

This adds DT bindings for the Microsemi/Microchip SGPIO controller,
bindings microchip,sparx5-sgpio, mscc,ocelot-sgpio and
mscc,luton-sgpio.

Signed-off-by: Lars Povlsen <lars.povlsen@microchip.com>
---
 .../pinctrl/microchip,sparx5-sgpio.yaml       | 88 +++++++++++++++++++
 include/dt-bindings/gpio/mchp-sgpio.h         | 19 ++++
 2 files changed, 107 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/microchip,sparx5-sgpio.yaml
 create mode 100644 include/dt-bindings/gpio/mchp-sgpio.h

diff --git a/Documentation/devicetree/bindings/pinctrl/microchip,sparx5-sgpio.yaml b/Documentation/devicetree/bindings/pinctrl/microchip,sparx5-sgpio.yaml
new file mode 100644
index 000000000000..25a50ba9e893
--- /dev/null
+++ b/Documentation/devicetree/bindings/pinctrl/microchip,sparx5-sgpio.yaml
@@ -0,0 +1,88 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pinctrl/microchip,sparx5-sgpio.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Microsemi/Microchip Serial GPIO controller
+
+maintainers:
+  - Lars Povlsen <lars.povlsen@microchip.com>
+
+description: |
+  By using a serial interface, the SIO controller significantly extend
+  the number of available GPIOs with a minimum number of additional
+  pins on the device. The primary purpose of the SIO controllers is to
+  connect control signals from SFP modules and to act as an LED
+  controller.
+
+properties:
+  $nodename:
+    pattern: "gpio"
+
+  compatible:
+    enum:
+      - microchip,sparx5-sgpio
+      - mscc,ocelot-sgpio
+      - mscc,luton-sgpio
+
+  clocks:
+    maxItems: 1
+
+  gpio-controller: true
+
+  '#gpio-cells':
+    description: GPIO consumers must specify four arguments, first the
+      port number, then the bit number, then a input/output flag and
+      finally the GPIO flags (from include/dt-bindings/gpio/gpio.h).
+      The dt-bindings/gpio/mchp-sgpio.h file define manifest constants
+      PIN_INPUT and PIN_OUTPUT.
+    const: 4
+
+  gpio-ranges:
+    maxItems: 1
+
+  microchip,sgpio-port-ranges:
+    description: This is a sequence of tuples, defining intervals of
+      enabled ports in the serial input stream. The enabled ports must
+      match the hardware configuration in order for signals to be
+      properly written/read to/from the controller holding
+      registers. Being tuples, then number of arguments must be
+      even. The tuples mast be ordered (low, high) and are
+      inclusive. Arguments must be between 0 and 31.
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+    minItems: 2
+    maxItems: 64
+
+  microchip,sgpio-frequency:
+    description: The sgpio controller frequency (Hz). This dictates
+      the serial bitstream speed, which again affects the latency in
+      getting control signals back and forth between external shift
+      registers. The speed must be no larger than half the system
+      clock, and larger than zero.
+    $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 1
+    default: 12500000
+
+required:
+  - compatible
+  - clocks
+  - gpio-controller
+  - '#gpio-cells'
+  - gpio-ranges
+  - microchip,sgpio-port-ranges
+
+examples:
+  - |
+    sgpio2: gpio@1101036c {
+        compatible = "microchip,sparx5-sgpio";
+        pinctrl-0 = <&sgpio0_pins>;
+        pinctrl-names = "default";
+        reg = <0x1101036c 0x100>;
+        clocks = <&sys_clk>;
+        gpio-controller;
+        gpio-ranges = <&sgpio0 0 0 192>;
+        #gpio-cells = <4>;
+        microchip,sgpio-port-ranges = <0 0 16 18 28 31>;
+        microchip,sgpio-frequency = <25000000>;
+    };
diff --git a/include/dt-bindings/gpio/mchp-sgpio.h b/include/dt-bindings/gpio/mchp-sgpio.h
new file mode 100644
index 000000000000..61b8aa75d870
--- /dev/null
+++ b/include/dt-bindings/gpio/mchp-sgpio.h
@@ -0,0 +1,19 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * This header provides constants for binding the Microchip SGPIO
+ * driver.
+ *
+ */
+
+#ifndef _DT_BINDINGS_GPIO_MCHP_SGPIO_H
+#define _DT_BINDINGS_GPIO_MCHP_SGPIO_H
+
+#include <dt-bindings/gpio/gpio.h>
+
+/* mchp-sgpio specific pin type defines */
+#undef PIN_OUTPUT
+#undef PIN_INPUT
+#define PIN_OUTPUT	0
+#define PIN_INPUT	1
+
+#endif
--
2.27.0

[PATCH v2 2/3] pinctrl: pinctrl-mchp-sgpio: Add pinctrl driver for Microsemi Serial GPIO

[Lars Povlsen]

This adds a pinctrl driver for the Microsemi/Microchip Serial GPIO
(SGPIO) device used in various SoC's.

Signed-off-by: Lars Povlsen <lars.povlsen@microchip.com>
---
 MAINTAINERS                          |   1 +
 drivers/pinctrl/Kconfig              |  17 +
 drivers/pinctrl/Makefile             |   1 +
 drivers/pinctrl/pinctrl-mchp-sgpio.c | 642 +++++++++++++++++++++++++++
 4 files changed, 661 insertions(+)
 create mode 100644 drivers/pinctrl/pinctrl-mchp-sgpio.c

diff --git a/MAINTAINERS b/MAINTAINERS
index cc70e3ab428b..f147af7f3e0d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2137,6 +2137,7 @@ M:	Microchip Linux Driver Support <UNGLinuxDriver@microchip.com>
 L:	linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)
 S:	Supported
 F:	arch/arm64/boot/dts/microchip/
+F:	drivers/pinctrl/pinctrl-mchp-sgpio.c
 N:	sparx5
 
 ARM/MIOA701 MACHINE SUPPORT
diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index 8828613c4e0e..e109a1f9e296 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -404,6 +404,23 @@ config PINCTRL_OCELOT
 	select OF_GPIO
 	select REGMAP_MMIO
 
+config PINCTRL_MCHP_SGPIO
+	bool "Pinctrl driver for Microsemi/Microchip Serial GPIO"
+	depends on OF
+	depends on HAS_IOMEM
+	select GPIOLIB
+	select GENERIC_PINCONF
+	select GENERIC_PINCTRL_GROUPS
+	select GENERIC_PINMUX_FUNCTIONS
+	select OF_GPIO
+	help
+	  Support for the VCoreIII SoC serial GPIO device. By using a
+	  serial interface, the SIO controller significantly extends
+	  the number of available GPIOs with a minimum number of
+	  additional pins on the device. The primary purpose of the
+	  SIO controller is to connect control signals from SFP
+	  modules and to act as an LED controller.
+
 source "drivers/pinctrl/actions/Kconfig"
 source "drivers/pinctrl/aspeed/Kconfig"
 source "drivers/pinctrl/bcm/Kconfig"
diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
index 1731b2154df9..626e685bf9ca 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -49,6 +49,7 @@ obj-$(CONFIG_PINCTRL_ZYNQ)	+= pinctrl-zynq.o
 obj-$(CONFIG_PINCTRL_INGENIC)	+= pinctrl-ingenic.o
 obj-$(CONFIG_PINCTRL_RK805)	+= pinctrl-rk805.o
 obj-$(CONFIG_PINCTRL_OCELOT)	+= pinctrl-ocelot.o
+obj-$(CONFIG_PINCTRL_MCHP_SGPIO) += pinctrl-mchp-sgpio.o
 obj-$(CONFIG_PINCTRL_EQUILIBRIUM)   += pinctrl-equilibrium.o
 
 obj-y				+= actions/
diff --git a/drivers/pinctrl/pinctrl-mchp-sgpio.c b/drivers/pinctrl/pinctrl-mchp-sgpio.c
new file mode 100644
index 000000000000..336ca9995088
--- /dev/null
+++ b/drivers/pinctrl/pinctrl-mchp-sgpio.c
@@ -0,0 +1,642 @@
+// SPDX-License-Identifier: (GPL-2.0 OR MIT)
+/*
+ * Microsemi/Microchip SoCs serial gpio driver
+ *
+ * Author: <lars.povlsen@microchip.com>
+ *
+ * Copyright (c) 2020 Microchip Technology Inc. and its subsidiaries.
+ */
+
+#include <linux/gpio/driver.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/of_device.h>
+#include <linux/of_irq.h>
+#include <linux/of_platform.h>
+#include <linux/clk.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/pinctrl/pinmux.h>
+#include <linux/pinctrl/pinconf.h>
+#include <linux/pinctrl/pinconf-generic.h>
+#include <linux/platform_device.h>
+
+#include <dt-bindings/gpio/mchp-sgpio.h>
+
+#include "core.h"
+#include "pinconf.h"
+#include "pinmux.h"
+
+#define MCHP_SGPIOS_PER_BANK	32
+#define MCHP_SGPIO_BANK_DEPTH	4
+
+#define PIN_NAM_SZ	(sizeof("SGPIO_D_pXXbY")+1)
+
+enum {
+	REG_INPUT_DATA,
+	REG_PORT_CONFIG,
+	REG_PORT_ENABLE,
+	REG_SIO_CONFIG,
+	REG_SIO_CLOCK,
+	MAXREG
+};
+
+struct mchp_sgpio_props {
+	const char *label;
+	u8 regoff[MAXREG];
+	u32 auto_repeat;
+	u32 port_width;
+	u32 clk_freq;
+	u32 bit_source;
+};
+
+#define __shf(x)		(__builtin_ffsll(x) - 1)
+#define __BF_PREP(bf, x)	(bf & ((x) << __shf(bf)))
+#define __BF_GET(bf, x)		(((x & bf) >> __shf(bf)))
+
+#define MCHP_M_CFG_SIO_AUTO_REPEAT(p)	 ((p)->props->auto_repeat)
+#define MCHP_F_CFG_SIO_PORT_WIDTH(p, x)	 __BF_PREP((p)->props->port_width, x)
+#define MCHP_M_CFG_SIO_PORT_WIDTH(p)	 ((p)->props->port_width)
+#define MCHP_F_CLOCK_SIO_CLK_FREQ(p, x)	 __BF_PREP((p)->props->clk_freq, x)
+#define MCHP_M_CLOCK_SIO_CLK_FREQ(p)	 ((p)->props->clk_freq)
+#define MCHP_F_PORT_CFG_BIT_SOURCE(p, x) __BF_PREP((p)->props->bit_source, x)
+#define MCHP_X_PORT_CFG_BIT_SOURCE(p, x) __BF_GET((p)->props->bit_source, x)
+
+const struct mchp_sgpio_props props_luton = {
+	.label = "luton-sgpio",
+	.regoff = { 0x00, 0x09, 0x29, 0x2a, 0x2b },
+	.auto_repeat = BIT(5),
+	.port_width  = GENMASK(3, 2),
+	.clk_freq    = GENMASK(11, 0),
+	.bit_source  = GENMASK(11, 0),
+};
+
+const struct mchp_sgpio_props props_ocelot = {
+	.label = "ocelot-sgpio",
+	.regoff = { 0x00, 0x06, 0x26, 0x04, 0x05 },
+	.auto_repeat = BIT(10),
+	.port_width  = GENMASK(8, 7),
+	.clk_freq    = GENMASK(19, 8),
+	.bit_source  = GENMASK(23, 12),
+};
+
+const struct mchp_sgpio_props props_sparx5 = {
+	.label = "sparx5-sgpio",
+	.regoff = { 0x00, 0x06, 0x26, 0x04, 0x05 },
+	.auto_repeat = BIT(6),
+	.port_width  = GENMASK(4, 3),
+	.clk_freq    = GENMASK(19, 8),
+	.bit_source  = GENMASK(23, 12),
+};
+
+static const char * const functions[] = { "gpio" };
+
+struct mchp_sgpio_priv {
+	struct device *dev;
+	struct pinctrl_dev *pctl;
+	struct gpio_chip gpio;
+	u32 ngpios;
+	u32 bitcount;
+	u32 ports;
+	u32 clock;
+	u32 __iomem *regs;
+	struct pinctrl_desc *desc;
+	const struct mchp_sgpio_props *props;
+};
+
+struct mchp_sgpio_port_addr {
+	u8 port;
+	u8 bit;
+	bool input;
+};
+
+static inline bool sgpio_is_input(struct mchp_sgpio_priv *priv, int pin)
+{
+	/* First half is default input, then output */
+	return pin < (priv->ngpios / 2);
+}
+
+static inline void sgpio_pin_to_addr(struct mchp_sgpio_priv *priv, int pin,
+	struct mchp_sgpio_port_addr *addr)
+{
+	addr->bit = pin % priv->bitcount;
+	addr->port = (pin / priv->bitcount) % MCHP_SGPIOS_PER_BANK;
+	addr->input = sgpio_is_input(priv, pin);
+}
+
+static inline u32 sgpio_readl(struct mchp_sgpio_priv *priv, u32 rno, u32 off)
+{
+	u32 __iomem *reg = &priv->regs[priv->props->regoff[rno] + off];
+
+	return readl(reg);
+}
+
+static inline void sgpio_writel(struct mchp_sgpio_priv *priv,
+				u32 val, u32 rno, u32 off)
+{
+	u32 __iomem *reg = &priv->regs[priv->props->regoff[rno] + off];
+
+	writel(val, reg);
+}
+
+static inline void sgpio_clrsetbits(struct mchp_sgpio_priv *priv,
+				    u32 rno, u32 off, u32 clear, u32 set)
+{
+	u32 __iomem *reg = &priv->regs[priv->props->regoff[rno] + off];
+	u32 val = readl(reg);
+
+	val &= ~clear;
+	val |= set;
+
+	writel(val, reg);
+}
+
+static void sgpio_output_set(struct mchp_sgpio_priv *priv,
+			     struct mchp_sgpio_port_addr *addr,
+			     int value)
+{
+	u32 mask = 3 << (3 * addr->bit);
+
+	value = (value & 3) << (3 * addr->bit);
+	sgpio_clrsetbits(priv, REG_PORT_CONFIG, addr->port,
+			 MCHP_F_PORT_CFG_BIT_SOURCE(priv, mask),
+			 MCHP_F_PORT_CFG_BIT_SOURCE(priv, value));
+}
+
+static int sgpio_output_get(struct mchp_sgpio_priv *priv,
+			    struct mchp_sgpio_port_addr *addr)
+{
+	u32 portval = sgpio_readl(priv, REG_PORT_CONFIG, addr->port);
+	int ret;
+
+	ret = MCHP_X_PORT_CFG_BIT_SOURCE(priv, portval);
+	ret = !!(ret & (3 << (3 * addr->bit)));
+
+	return ret;
+}
+
+static int sgpio_input_get(struct mchp_sgpio_priv *priv,
+			   struct mchp_sgpio_port_addr *addr)
+{
+	int ret;
+
+	ret = !!(sgpio_readl(priv, REG_INPUT_DATA, addr->bit) &
+		 BIT(addr->port));
+
+	return ret;
+}
+
+static int sgpio_pinconf_get(struct pinctrl_dev *pctldev,
+			     unsigned int pin, unsigned long *config)
+{
+	struct mchp_sgpio_priv *priv = pinctrl_dev_get_drvdata(pctldev);
+	struct mchp_sgpio_port_addr addr;
+	u32 param = pinconf_to_config_param(*config);
+	int val;
+
+	sgpio_pin_to_addr(priv, pin, &addr);
+
+	switch (param) {
+	case PIN_CONFIG_INPUT_ENABLE:
+		val = addr.input;
+		break;
+
+	case PIN_CONFIG_OUTPUT_ENABLE:
+		val = !addr.input;
+		break;
+
+	case PIN_CONFIG_OUTPUT:
+		if (addr.input)
+			return -EINVAL;
+		val = sgpio_output_get(priv, &addr);
+		break;
+
+	default:
+		return -ENOTSUPP;
+	}
+
+	*config = pinconf_to_config_packed(param, val);
+
+	return 0;
+}
+
+static int sgpio_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
+			     unsigned long *configs, unsigned int num_configs)
+{
+	struct mchp_sgpio_priv *priv = pinctrl_dev_get_drvdata(pctldev);
+	struct mchp_sgpio_port_addr addr;
+	u32 param, arg;
+	int cfg, err = 0;
+
+	sgpio_pin_to_addr(priv, pin, &addr);
+
+	for (cfg = 0; cfg < num_configs; cfg++) {
+		param = pinconf_to_config_param(configs[cfg]);
+		arg = pinconf_to_config_argument(configs[cfg]);
+
+		switch (param) {
+		case PIN_CONFIG_OUTPUT:
+			if (addr.input)
+				return -EINVAL;
+			sgpio_output_set(priv, &addr, arg);
+			break;
+
+		default:
+			err = -ENOTSUPP;
+		}
+	}
+
+	return err;
+}
+
+static const struct pinconf_ops sgpio_confops = {
+	.is_generic = true,
+	.pin_config_get = sgpio_pinconf_get,
+	.pin_config_set = sgpio_pinconf_set,
+	.pin_config_config_dbg_show = pinconf_generic_dump_config,
+};
+
+static int sgpio_get_functions_count(struct pinctrl_dev *pctldev)
+{
+	return 1;
+}
+
+static const char *sgpio_get_function_name(struct pinctrl_dev *pctldev,
+					   unsigned int function)
+{
+	return functions[0];
+}
+
+static int sgpio_get_function_groups(struct pinctrl_dev *pctldev,
+				      unsigned int function,
+				      const char *const **groups,
+				      unsigned *const num_groups)
+{
+	*groups  = functions;
+	*num_groups = ARRAY_SIZE(functions);
+
+	return 0;
+}
+
+static int sgpio_pinmux_set_mux(struct pinctrl_dev *pctldev,
+				unsigned int selector, unsigned int group)
+{
+	return 0;
+}
+
+static int sgpio_gpio_set_direction(struct pinctrl_dev *pctldev,
+				    struct pinctrl_gpio_range *range,
+				    unsigned int pin, bool input)
+{
+	struct mchp_sgpio_priv *priv = pinctrl_dev_get_drvdata(pctldev);
+
+	if (input != sgpio_is_input(priv, pin)) {
+		dev_err(priv->dev, "Pin %d direction as %s is not possible\n",
+			pin, input ? "input" : "output");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int sgpio_gpio_request_enable(struct pinctrl_dev *pctldev,
+				     struct pinctrl_gpio_range *range,
+				     unsigned int offset)
+{
+	struct mchp_sgpio_priv *priv = pinctrl_dev_get_drvdata(pctldev);
+	struct mchp_sgpio_port_addr addr;
+
+	sgpio_pin_to_addr(priv, offset, &addr);
+
+	if ((priv->ports & BIT(addr.port)) == 0) {
+		dev_warn(priv->dev, "%s: Request port %d for pin %d is not activated\n",
+			 __func__, addr.port, offset);
+	}
+
+	return 0;
+}
+
+static const struct pinmux_ops sgpio_pmx_ops = {
+	.get_functions_count = sgpio_get_functions_count,
+	.get_function_name = sgpio_get_function_name,
+	.get_function_groups = sgpio_get_function_groups,
+	.set_mux = sgpio_pinmux_set_mux,
+	.gpio_set_direction = sgpio_gpio_set_direction,
+	.gpio_request_enable = sgpio_gpio_request_enable,
+};
+
+static int sgpio_pctl_get_groups_count(struct pinctrl_dev *pctldev)
+{
+	struct mchp_sgpio_priv *priv = pinctrl_dev_get_drvdata(pctldev);
+
+	return priv->desc->npins;
+}
+
+static const char *sgpio_pctl_get_group_name(struct pinctrl_dev *pctldev,
+					      unsigned int group)
+{
+	struct mchp_sgpio_priv *priv = pinctrl_dev_get_drvdata(pctldev);
+
+	return priv->desc->pins[group].name;
+}
+
+static int sgpio_pctl_get_group_pins(struct pinctrl_dev *pctldev,
+				      unsigned int group,
+				      const unsigned int **pins,
+				      unsigned int *num_pins)
+{
+	struct mchp_sgpio_priv *priv = pinctrl_dev_get_drvdata(pctldev);
+
+	*pins = &priv->desc->pins[group].number;
+	*num_pins = 1;
+
+	return 0;
+}
+
+static const struct pinctrl_ops sgpio_pctl_ops = {
+	.get_groups_count = sgpio_pctl_get_groups_count,
+	.get_group_name = sgpio_pctl_get_group_name,
+	.get_group_pins = sgpio_pctl_get_group_pins,
+	.dt_node_to_map = pinconf_generic_dt_node_to_map_pin,
+	.dt_free_map = pinconf_generic_dt_free_map,
+};
+
+static struct pinctrl_desc sgpio_desc = {
+	.name = "sgpio-pinctrl",
+	.pctlops = &sgpio_pctl_ops,
+	.pmxops = &sgpio_pmx_ops,
+	.confops = &sgpio_confops,
+	.owner = THIS_MODULE,
+};
+
+static int mchp_sgpio_direction_input(struct gpio_chip *gc, unsigned int gpio)
+{
+	struct mchp_sgpio_priv *priv = gpiochip_get_data(gc);
+
+	/* Fixed-position function */
+	return sgpio_is_input(priv, gpio) ? 0 : -EINVAL;
+}
+
+static int mchp_sgpio_direction_output(struct gpio_chip *gc,
+				       unsigned int gpio, int value)
+{
+	struct mchp_sgpio_priv *priv = gpiochip_get_data(gc);
+	struct mchp_sgpio_port_addr addr;
+
+	sgpio_pin_to_addr(priv, gpio, &addr);
+
+	/* Fixed-position function */
+	if (addr.input)
+		return -EINVAL;
+
+	sgpio_output_set(priv, &addr, value);
+
+	return 0;
+}
+
+static int mchp_sgpio_get_direction(struct gpio_chip *gc, unsigned int gpio)
+{
+	struct mchp_sgpio_priv *priv = gpiochip_get_data(gc);
+
+	return sgpio_is_input(priv, gpio); /* 0=out, 1=in */
+}
+
+static void mchp_sgpio_set_value(struct gpio_chip *gc,
+				unsigned int gpio, int value)
+{
+	mchp_sgpio_direction_output(gc, gpio, value);
+}
+
+static int mchp_sgpio_get_value(struct gpio_chip *gc, unsigned int gpio)
+{
+	struct mchp_sgpio_priv *priv = gpiochip_get_data(gc);
+	struct mchp_sgpio_port_addr addr;
+	int ret;
+
+	sgpio_pin_to_addr(priv, gpio, &addr);
+
+	if (sgpio_is_input(priv, gpio))
+		ret = sgpio_input_get(priv, &addr);
+	else
+		ret = sgpio_output_get(priv, &addr);
+
+	return ret;
+}
+
+static int mchp_sgpio_get_ports(struct mchp_sgpio_priv *priv)
+{
+	struct device *dev = priv->dev;
+	struct device_node *np = dev->of_node;
+	struct of_phandle_args pinspec;
+	int count, i, ret;
+	u32 range_params[64];
+
+	/* Calculate bitcount */
+	for (count = 0, i = 0;; i++) {
+		ret = of_parse_phandle_with_fixed_args(np, "gpio-ranges", 3,
+						       i, &pinspec);
+		if (ret)
+			break;
+		count += pinspec.args[2];
+	}
+
+	if (count == 0 ||
+	    count % MCHP_SGPIOS_PER_BANK ||
+	    count > (MCHP_SGPIO_BANK_DEPTH * MCHP_SGPIOS_PER_BANK * 2)) {
+		dev_err(dev, "Invalid gpio count %d\n", count);
+		return -EINVAL;
+	}
+	priv->ngpios = count;
+	priv->bitcount = count / MCHP_SGPIOS_PER_BANK / 2;
+
+	/* Calculate port mask */
+	ret = of_property_read_variable_u32_array(np,
+						  "microchip,sgpio-port-ranges",
+						  range_params,
+						  2,
+						  ARRAY_SIZE(range_params));
+	if (ret < 0 || ret % 2) {
+		dev_err(dev, "%s port range\n",
+			ret == -EINVAL ? "Missing" : "Invalid");
+		return ret;
+	}
+	for (i = 0; i < ret; i += 2) {
+		int start, end;
+
+		start = range_params[i];
+		end = range_params[i + 1];
+		if (start > end || end >= MCHP_SGPIOS_PER_BANK) {
+			dev_err(dev, "Ill-formed port-range [%d:%d]\n",
+				start, end);
+		}
+		priv->ports |= GENMASK(end, start);
+	}
+
+	dev_dbg(dev, "probe: gpios = %d, ports = 0x%08x, bit-count = %d\n",
+		priv->ngpios, priv->ports, priv->bitcount);
+
+	return 0;
+}
+
+static int mchp_sgpio_of_xlate(struct gpio_chip *gc,
+			       const struct of_phandle_args *gpiospec,
+			       u32 *flags)
+{
+	struct mchp_sgpio_priv *priv = gpiochip_get_data(gc);
+	int pin, base;
+
+	if (gpiospec->args[0] > MCHP_SGPIOS_PER_BANK ||
+	    gpiospec->args[1] > priv->bitcount)
+		return -EINVAL;
+	base = priv->bitcount * gpiospec->args[0];
+	pin = base + gpiospec->args[1];
+	/* Add to 2nd half of output range if output */
+	if (gpiospec->args[2] == PIN_OUTPUT)
+		pin += (priv->ngpios / 2);
+
+	if (pin > gc->ngpio)
+		return -EINVAL;
+
+	if (flags)
+		*flags = gpiospec->args[3];
+
+	return pin;
+}
+
+static int mchp_sgpio_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct mchp_sgpio_priv *priv;
+	int div_clock = 0, ret, port;
+	u32 val;
+	struct resource *regs;
+	struct clk *clk;
+	struct pinctrl_pin_desc *pins;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->dev = dev;
+
+
+	/* Get clock */
+	clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(clk)) {
+		dev_err(dev, "Failed to get clock\n");
+		return PTR_ERR(clk);
+	}
+	div_clock = clk_get_rate(clk);
+	if (device_property_read_u32(dev, "microchip,sgpio-frequency",
+				     &priv->clock))
+		priv->clock = 12500000;
+	if (priv->clock <= 0 || priv->clock > div_clock) {
+		dev_err(dev, "Invalid frequency %d\n", priv->clock);
+		return -EINVAL;
+	}
+
+	/* Get register map */
+	regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	priv->regs = devm_ioremap_resource(dev, regs);
+	if (IS_ERR(priv->regs))
+		return PTR_ERR(priv->regs);
+	priv->props = of_device_get_match_data(dev);
+
+	/* Get rest of device properties */
+	ret = mchp_sgpio_get_ports(priv);
+	if (ret)
+		return ret;
+
+	pins = devm_kzalloc(dev, sizeof(*pins)*priv->ngpios, GFP_KERNEL);
+	if (pins) {
+		int i;
+		char *p, *names;
+
+		names = devm_kzalloc(dev, PIN_NAM_SZ*priv->ngpios, GFP_KERNEL);
+
+		if (!names)
+			return -ENOMEM;
+
+		sgpio_desc.npins = priv->ngpios;
+		sgpio_desc.pins = pins;
+
+		for (p = names, i = 0; i < priv->ngpios; i++, p += PIN_NAM_SZ) {
+			struct mchp_sgpio_port_addr addr;
+
+			sgpio_pin_to_addr(priv, i, &addr);
+			snprintf(p, PIN_NAM_SZ, "SGPIO_%c_p%db%d",
+				 addr.input ? 'I' : 'O',
+				 addr.port, addr.bit);
+			pins[i].number = i;
+			pins[i].name = p;
+		}
+	} else
+		return -ENOMEM;
+
+	priv->desc = &sgpio_desc;
+	priv->pctl = devm_pinctrl_register(&pdev->dev, priv->desc, priv);
+	if (IS_ERR(priv->pctl)) {
+		dev_err(&pdev->dev, "Failed to register pinctrl\n");
+		return PTR_ERR(priv->pctl);
+	}
+
+	priv->gpio.label		= priv->props->label;
+	priv->gpio.parent		= dev;
+	priv->gpio.of_node		= dev->of_node;
+	priv->gpio.owner		= THIS_MODULE;
+	priv->gpio.get_direction	= mchp_sgpio_get_direction;
+	priv->gpio.direction_input	= mchp_sgpio_direction_input;
+	priv->gpio.direction_output	= mchp_sgpio_direction_output;
+	priv->gpio.get			= mchp_sgpio_get_value,
+	priv->gpio.set			= mchp_sgpio_set_value;
+	priv->gpio.request		= gpiochip_generic_request;
+	priv->gpio.free			= gpiochip_generic_free;
+	priv->gpio.of_xlate		= mchp_sgpio_of_xlate;
+	priv->gpio.of_gpio_n_cells      = 4;
+	priv->gpio.base			= -1;
+	priv->gpio.ngpio		= priv->ngpios;
+
+	sgpio_clrsetbits(priv, REG_SIO_CONFIG, 0,
+			 MCHP_M_CFG_SIO_PORT_WIDTH(priv),
+			 MCHP_F_CFG_SIO_PORT_WIDTH(priv, priv->bitcount - 1) |
+			 MCHP_M_CFG_SIO_AUTO_REPEAT(priv));
+	val = max(2, div_clock / priv->clock);
+	sgpio_clrsetbits(priv, REG_SIO_CLOCK, 0,
+			 MCHP_M_CLOCK_SIO_CLK_FREQ(priv),
+			 MCHP_F_CLOCK_SIO_CLK_FREQ(priv, val));
+
+	for (port = 0; port < MCHP_SGPIOS_PER_BANK; port++)
+		sgpio_writel(priv, 0, REG_PORT_CONFIG, port);
+	sgpio_writel(priv, priv->ports, REG_PORT_ENABLE, 0);
+
+	ret = devm_gpiochip_add_data(dev, &priv->gpio, priv);
+	if (ret == 0)
+		dev_info(dev, "Registered %d GPIOs\n", priv->ngpios);
+	else
+		dev_err(dev, "Failed to register: ret %d\n", ret);
+	return ret;
+}
+
+static const struct of_device_id mchp_sgpio_gpio_of_match[] = {
+	{
+		.compatible = "microchip,sparx5-sgpio",
+		.data = &props_sparx5,
+	}, {
+		.compatible = "mscc,luton-sgpio",
+		.data = &props_luton,
+	}, {
+		.compatible = "mscc,ocelot-sgpio",
+		.data = &props_ocelot,
+	}, {
+		/* sentinel */
+	}
+};
+
+static struct platform_driver mchp_sgpio_pinctrl_driver = {
+	.driver = {
+		.name = "pinctrl-mchp-sgpio",
+		.of_match_table = mchp_sgpio_gpio_of_match,
+		.suppress_bind_attrs = true,
+	},
+	.probe = mchp_sgpio_probe,
+};
+builtin_platform_driver(mchp_sgpio_pinctrl_driver);
-- 
2.27.0

[PATCH v2 3/3] arm64: dts: sparx5: Add SGPIO devices

[Lars Povlsen]

This adds SGPIO devices for the Sparx5 SoC and configures it for the
applicable reference boards.

Signed-off-by: Lars Povlsen <lars.povlsen@microchip.com>
---
 arch/arm64/boot/dts/microchip/sparx5.dtsi     | 52 +++++++++++++++++++
 .../boot/dts/microchip/sparx5_pcb125.dts      |  5 ++
 .../dts/microchip/sparx5_pcb134_board.dtsi    |  5 ++
 .../dts/microchip/sparx5_pcb135_board.dtsi    |  5 ++
 4 files changed, 67 insertions(+)

diff --git a/arch/arm64/boot/dts/microchip/sparx5.dtsi b/arch/arm64/boot/dts/microchip/sparx5.dtsi
index 5408486b4d3b..3cbf8824a545 100644
--- a/arch/arm64/boot/dts/microchip/sparx5.dtsi
+++ b/arch/arm64/boot/dts/microchip/sparx5.dtsi
@@ -232,6 +232,22 @@ si2_pins: si2-pins {
 				function = "si2";
 			};
 
+			sgpio0_pins: sgpio-pins {
+				pins = "GPIO_0", "GPIO_1", "GPIO_2", "GPIO_3";
+				function = "sg0";
+			};
+
+			sgpio1_pins: sgpio1-pins {
+				pins = "GPIO_4", "GPIO_5", "GPIO_12", "GPIO_13";
+				function = "sg1";
+			};
+
+			sgpio2_pins: sgpio2-pins {
+				pins = "GPIO_30", "GPIO_31", "GPIO_32",
+				       "GPIO_33";
+				function = "sg2";
+			};
+
 			uart_pins: uart-pins {
 				pins = "GPIO_10", "GPIO_11";
 				function = "uart";
@@ -262,6 +278,42 @@ emmc_pins: emmc-pins {
 			};
 		};
 
+		sgpio0: gpio@61101036c {
+			compatible = "microchip,sparx5-sgpio";
+			status = "disabled";
+			clocks = <&sys_clk>;
+			pinctrl-0 = <&sgpio0_pins>;
+			pinctrl-names = "default";
+			reg = <0x6 0x1101036c 0x100>;
+			gpio-controller;
+			gpio-ranges = <&sgpio0 0 0 192>;
+			#gpio-cells = <4>;
+		};
+
+		sgpio1: gpio@611010484 {
+			compatible = "microchip,sparx5-sgpio";
+			status = "disabled";
+			clocks = <&sys_clk>;
+			pinctrl-0 = <&sgpio1_pins>;
+			pinctrl-names = "default";
+			reg = <0x6 0x11010484 0x100>;
+			gpio-controller;
+			gpio-ranges = <&sgpio1 0 0 192>;
+			#gpio-cells = <4>;
+		};
+
+		sgpio2: gpio@61101059c {
+			compatible = "microchip,sparx5-sgpio";
+			status = "disabled";
+			clocks = <&sys_clk>;
+			pinctrl-0 = <&sgpio2_pins>;
+			pinctrl-names = "default";
+			reg = <0x6 0x1101059c 0x100>;
+			gpio-controller;
+			gpio-ranges = <&sgpio2 0 0 192>;
+			#gpio-cells = <4>;
+		};
+
 		i2c0: i2c@600101000 {
 			compatible = "snps,designware-i2c";
 			status = "disabled";
diff --git a/arch/arm64/boot/dts/microchip/sparx5_pcb125.dts b/arch/arm64/boot/dts/microchip/sparx5_pcb125.dts
index 6b2da7c7520c..9baa085d7861 100644
--- a/arch/arm64/boot/dts/microchip/sparx5_pcb125.dts
+++ b/arch/arm64/boot/dts/microchip/sparx5_pcb125.dts
@@ -69,6 +69,11 @@ spi-flash@9 {
 	};
 };
 
+&sgpio0 {
+	status = "okay";
+	microchip,sgpio-port-ranges = <0 23>;
+};
+
 &i2c1 {
 	status = "okay";
 };
diff --git a/arch/arm64/boot/dts/microchip/sparx5_pcb134_board.dtsi b/arch/arm64/boot/dts/microchip/sparx5_pcb134_board.dtsi
index 35984785d611..65336be31fd9 100644
--- a/arch/arm64/boot/dts/microchip/sparx5_pcb134_board.dtsi
+++ b/arch/arm64/boot/dts/microchip/sparx5_pcb134_board.dtsi
@@ -54,6 +54,11 @@ spi-flash@9 {
 	};
 };
 
+&sgpio2 {
+	status = "okay";
+        microchip,sgpio-port-ranges = <0 0 11 31>;
+};
+
 &gpio {
 	i2cmux_pins_i: i2cmux-pins-i {
 	       pins = "GPIO_16", "GPIO_17", "GPIO_18", "GPIO_19",
diff --git a/arch/arm64/boot/dts/microchip/sparx5_pcb135_board.dtsi b/arch/arm64/boot/dts/microchip/sparx5_pcb135_board.dtsi
index 7de66806b14b..5ea2d0910c2b 100644
--- a/arch/arm64/boot/dts/microchip/sparx5_pcb135_board.dtsi
+++ b/arch/arm64/boot/dts/microchip/sparx5_pcb135_board.dtsi
@@ -67,6 +67,11 @@ spi-flash@9 {
 	};
 };
 
+&sgpio2 {
+	status = "okay";
+	microchip,sgpio-port-ranges = <0 0 16 18 28 31>;
+};
+
 &axi {
 	i2c0_imux: i2c0-imux@0 {
 		compatible = "i2c-mux-pinctrl";
-- 
2.27.0

Re: [PATCH v2 1/3] dt-bindings: pinctrl: Add bindings for pinctrl-mchp-sgpio driver

[Linus Walleij]

Hi Lars,

thanks for your patch!

On Thu, Sep 3, 2020 at 3:35 PM Lars Povlsen <lars.povlsen@microchip.com> wrote:

> This adds DT bindings for the Microsemi/Microchip SGPIO controller,

What I do not understand is why this GPIO controller is placed in the
bindings of the pin controllers? Do you plan to add pin control
properties to the bindings in the future?

> +description: |
> +  By using a serial interface, the SIO controller significantly extend
> +  the number of available GPIOs with a minimum number of additional
> +  pins on the device. The primary purpose of the SIO controllers is to
> +  connect control signals from SFP modules and to act as an LED
> +  controller.

This doesn't sound like it will ever be pin control?

> +  gpio-controller: true
> +
> +  '#gpio-cells':
> +    description: GPIO consumers must specify four arguments, first the
> +      port number, then the bit number, then a input/output flag and
> +      finally the GPIO flags (from include/dt-bindings/gpio/gpio.h).
> +      The dt-bindings/gpio/mchp-sgpio.h file define manifest constants
> +      PIN_INPUT and PIN_OUTPUT.
> +    const: 4

I do not follow this new third input/output flag at all.

- If it is a property of the hardware, it is something the driver should
  handle by determining which hardware it is from the compatible
  string.

- If it is a configuration it should be turned into something that is generic
  and useful for *all* GPIO controllers. If it is pin config it should use
  the pinconf bindings rather than shortcuts like this, but I think it is
  something the driver can do as an effect of the pin being requested
  as input or output in the operating system, depending on who the
  consumer is. Linux for example has GPIOD_OUT_LOW,
  GPIOD_OUT_HIGH, GPIOD_IN, GPIOD_ASIS...

- Is it not just a hog? We have bindings for those.

> +  microchip,sgpio-port-ranges:
> +    description: This is a sequence of tuples, defining intervals of
> +      enabled ports in the serial input stream. The enabled ports must
> +      match the hardware configuration in order for signals to be
> +      properly written/read to/from the controller holding
> +      registers. Being tuples, then number of arguments must be
> +      even. The tuples mast be ordered (low, high) and are
> +      inclusive. Arguments must be between 0 and 31.
> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> +    minItems: 2
> +    maxItems: 64

And you are *absolutely sure* that you can't just figure this out
from the compatible string? Or add a few compatible strings for
the existing variants?

> +  microchip,sgpio-frequency:
> +    description: The sgpio controller frequency (Hz). This dictates
> +      the serial bitstream speed, which again affects the latency in
> +      getting control signals back and forth between external shift
> +      registers. The speed must be no larger than half the system
> +      clock, and larger than zero.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 1
> +    default: 12500000

I understand why you need this binding now, OK.

> +/* mchp-sgpio specific pin type defines */
> +#undef PIN_OUTPUT
> +#undef PIN_INPUT
> +#define PIN_OUTPUT     0
> +#define PIN_INPUT      1

I'm not a fan of this. It seems like something that should be set in
response to the gpiochip callbacks .direction_input and
.direction_output callbacks.

Yours,
Linus Walleij

Re: [PATCH v2 2/3] pinctrl: pinctrl-mchp-sgpio: Add pinctrl driver for Microsemi Serial GPIO

[Linus Walleij]

On Thu, Sep 3, 2020 at 3:35 PM Lars Povlsen <lars.povlsen@microchip.com> wrote:

> This adds a pinctrl driver for the Microsemi/Microchip Serial GPIO
> (SGPIO) device used in various SoC's.
>
> Signed-off-by: Lars Povlsen <lars.povlsen@microchip.com>
(...)

> diff --git a/drivers/pinctrl/pinctrl-mchp-sgpio.c b/drivers/pinctrl/pinctrl-mchp-sgpio.c

Can we just spell it out
pinctrl-microchip-sgpio.c ?

The abbreviation seems arbitrary and unnecessary.

I do see that this chip is using the pinctrl abstractions (very nicely)
and should be under drivers/pinctrl/*.

Sadly it doesn't mean the bindings need to be in pinctrl ... unless you
plan to add pinctrl bindings later.

> +config PINCTRL_MCHP_SGPIO
> +       bool "Pinctrl driver for Microsemi/Microchip Serial GPIO"
> +       depends on OF
> +       depends on HAS_IOMEM
> +       select GPIOLIB
> +       select GENERIC_PINCONF
> +       select GENERIC_PINCTRL_GROUPS
> +       select GENERIC_PINMUX_FUNCTIONS

Nice use of these abstractions!

> +// SPDX-License-Identifier: (GPL-2.0 OR MIT)

What's up with this OR MIT? I'd like Kate to OK this.

> +#define MCHP_SGPIOS_PER_BANK   32
> +#define MCHP_SGPIO_BANK_DEPTH  4
> +
> +#define PIN_NAM_SZ     (sizeof("SGPIO_D_pXXbY")+1)
> +
> +enum {
> +       REG_INPUT_DATA,
> +       REG_PORT_CONFIG,
> +       REG_PORT_ENABLE,
> +       REG_SIO_CONFIG,
> +       REG_SIO_CLOCK,
> +       MAXREG
> +};
> +
> +struct mchp_sgpio_props {

Just call it struct microchip_gpio_variant it is easier to read and
does not abbreviate randomly, also it is a per-variant set of properties
so calling it variant is more to the point.

> +struct mchp_sgpio_priv {

I would just spell it out struct microchip_sgpio, it is implicit that
the struct is private since it is defined in a c file.

> +struct mchp_sgpio_port_addr {

struct microchip_sgpio_port_addr

(Admittedly this is a bit about taste.)

> +static inline void sgpio_writel(struct mchp_sgpio_priv *priv,
> +                               u32 val, u32 rno, u32 off)
> +{
> +       u32 __iomem *reg = &priv->regs[priv->props->regoff[rno] + off];
> +
> +       writel(val, reg);
> +}
> +
> +static inline void sgpio_clrsetbits(struct mchp_sgpio_priv *priv,
> +                                   u32 rno, u32 off, u32 clear, u32 set)
> +{
> +       u32 __iomem *reg = &priv->regs[priv->props->regoff[rno] + off];
> +       u32 val = readl(reg);
> +
> +       val &= ~clear;
> +       val |= set;
> +
> +       writel(val, reg);
> +}

This looks like a reimplementation of regmap_update_bits for a bit,
have you considered just using regmap? It's pretty simple.

> +static int mchp_sgpio_direction_input(struct gpio_chip *gc, unsigned int gpio)
> +{
> +       struct mchp_sgpio_priv *priv = gpiochip_get_data(gc);
> +
> +       /* Fixed-position function */
> +       return sgpio_is_input(priv, gpio) ? 0 : -EINVAL;
> +}
> +
> +static int mchp_sgpio_direction_output(struct gpio_chip *gc,
> +                                      unsigned int gpio, int value)
> +{
> +       struct mchp_sgpio_priv *priv = gpiochip_get_data(gc);
> +       struct mchp_sgpio_port_addr addr;
> +
> +       sgpio_pin_to_addr(priv, gpio, &addr);
> +
> +       /* Fixed-position function */
> +       if (addr.input)
> +               return -EINVAL;
> +
> +       sgpio_output_set(priv, &addr, value);
> +
> +       return 0;
> +}

This looks like the right way to handle this!

> +static int mchp_sgpio_of_xlate(struct gpio_chip *gc,
> +                              const struct of_phandle_args *gpiospec,
> +                              u32 *flags)
> +{
> +       struct mchp_sgpio_priv *priv = gpiochip_get_data(gc);
> +       int pin, base;
> +
> +       if (gpiospec->args[0] > MCHP_SGPIOS_PER_BANK ||
> +           gpiospec->args[1] > priv->bitcount)
> +               return -EINVAL;
> +       base = priv->bitcount * gpiospec->args[0];
> +       pin = base + gpiospec->args[1];
> +       /* Add to 2nd half of output range if output */
> +       if (gpiospec->args[2] == PIN_OUTPUT)
> +               pin += (priv->ngpios / 2);
> +
> +       if (pin > gc->ngpio)
> +               return -EINVAL;
> +
> +       if (flags)
> +               *flags = gpiospec->args[3];
> +
> +       return pin;
> +}

I don't like this. I would certainly prefer the driver to just use standard
GPIO bindings. I do not understand why this is necessary.

If for nothing else, there should be a big comment explaining this.

The only real problem I have with the driver is this extra flag tagged onto
all the GPIOs, this seems unnecessary, and something the hardware
driver should already know from the compatible string.

Yours,
Linus Walleij

Re: [PATCH v2 1/3] dt-bindings: pinctrl: Add bindings for pinctrl-mchp-sgpio driver

[Lars Povlsen]

Linus Walleij writes:

> Hi Lars,
>
> thanks for your patch!

You're welcome - thank you for you taking time to review it!

>
> On Thu, Sep 3, 2020 at 3:35 PM Lars Povlsen <lars.povlsen@microchip.com> wrote:
>
>> This adds DT bindings for the Microsemi/Microchip SGPIO controller,
>
> What I do not understand is why this GPIO controller is placed in the
> bindings of the pin controllers? Do you plan to add pin control
> properties to the bindings in the future?

I have made provisions for some of the generic pinconf parameters, and
since the controller also has support for some alternate modes like
(syncronized) blink at various rates, I thought I better add it as
pinctrl straight away.

>
>> +description: |
>> +  By using a serial interface, the SIO controller significantly extend
>> +  the number of available GPIOs with a minimum number of additional
>> +  pins on the device. The primary purpose of the SIO controllers is to
>> +  connect control signals from SFP modules and to act as an LED
>> +  controller.
>
> This doesn't sound like it will ever be pin control?

above.

>
>> +  gpio-controller: true
>> +
>> +  '#gpio-cells':
>> +    description: GPIO consumers must specify four arguments, first the
>> +      port number, then the bit number, then a input/output flag and
>> +      finally the GPIO flags (from include/dt-bindings/gpio/gpio.h).
>> +      The dt-bindings/gpio/mchp-sgpio.h file define manifest constants
>> +      PIN_INPUT and PIN_OUTPUT.
>> +    const: 4
>
> I do not follow this new third input/output flag at all.
>

Its actually a sort of bank address, since the individual "pins" are
unidirectional.

The PIN_INPUT/PIN_OUTPUT is defined in similar fashion in other pinctrl
binding header files... I can drop the define and use, but as it will be
used to address individual pins, I think it adds to readability.

Like this (excerpts from a DT with a switchdev driver using SFP's and
LED's on sgpio):

/{
	leds {
		compatible = "gpio-leds";
		led@0 {
			label = "eth60:yellow";
			gpios = <&sgpio1 28 0 PIN_OUTPUT GPIO_ACTIVE_LOW>;
			default-state = "off";
		};
		...
	};
};

&axi {
	sfp_eth60: sfp-eth60 {
		compatible	   = "sff,sfp";
		i2c-bus            = <&i2c152>;
		tx-disable-gpios   = <&sgpio2 28 0 PIN_OUTPUT GPIO_ACTIVE_LOW>;
		rate-select0-gpios = <&sgpio2 28 1 PIN_OUTPUT GPIO_ACTIVE_HIGH>;
		los-gpios          = <&sgpio2 28 0 PIN_INPUT GPIO_ACTIVE_HIGH>;
		mod-def0-gpios     = <&sgpio2 28 1 PIN_INPUT GPIO_ACTIVE_LOW>;
		tx-fault-gpios     = <&sgpio2 28 2 PIN_INPUT GPIO_ACTIVE_HIGH>;
	};
	...
};
                
> - If it is a property of the hardware, it is something the driver should
>   handle by determining which hardware it is from the compatible
>   string.
>
> - If it is a configuration it should be turned into something that is generic
>   and useful for *all* GPIO controllers. If it is pin config it should use
>   the pinconf bindings rather than shortcuts like this, but I think it is
>   something the driver can do as an effect of the pin being requested
>   as input or output in the operating system, depending on who the
>   consumer is. Linux for example has GPIOD_OUT_LOW,
>   GPIOD_OUT_HIGH, GPIOD_IN, GPIOD_ASIS...
>
> - Is it not just a hog? We have bindings for those.

I hope the above shed some light on this.

>
>> +  microchip,sgpio-port-ranges:
>> +    description: This is a sequence of tuples, defining intervals of
>> +      enabled ports in the serial input stream. The enabled ports must
>> +      match the hardware configuration in order for signals to be
>> +      properly written/read to/from the controller holding
>> +      registers. Being tuples, then number of arguments must be
>> +      even. The tuples mast be ordered (low, high) and are
>> +      inclusive. Arguments must be between 0 and 31.
>> +    $ref: /schemas/types.yaml#/definitions/uint32-array
>> +    minItems: 2
>> +    maxItems: 64
>
> And you are *absolutely sure* that you can't just figure this out
> from the compatible string? Or add a few compatible strings for
> the existing variants?
>

Yes, this really needs to be configured for each board individually -
and cant be probed. It defines how the bitstream to/from the shift
registers is constructed/demuxed.

>> +  microchip,sgpio-frequency:
>> +    description: The sgpio controller frequency (Hz). This dictates
>> +      the serial bitstream speed, which again affects the latency in
>> +      getting control signals back and forth between external shift
>> +      registers. The speed must be no larger than half the system
>> +      clock, and larger than zero.
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    minimum: 1
>> +    default: 12500000
>
> I understand why you need this binding now, OK.
>
>> +/* mchp-sgpio specific pin type defines */
>> +#undef PIN_OUTPUT
>> +#undef PIN_INPUT
>> +#define PIN_OUTPUT     0
>> +#define PIN_INPUT      1
>
> I'm not a fan of this. It seems like something that should be set in
> response to the gpiochip callbacks .direction_input and
> .direction_output callbacks.
>

As I tried to explain above, its a part of the pin address - aka bank
selector - whether your are accessing the input or the output side. And
since the directions have totally different - and concurrent - use, they
need to be individually addressed, not "configured".

In the example presented, sgpio2-p28b0 IN is loss-of-signal, and the
OUT is the sfp tx-disable control.

> Yours,
> Linus Walleij

---Lars

-- 
Lars Povlsen,
Microchip

Re: [PATCH v2 2/3] pinctrl: pinctrl-mchp-sgpio: Add pinctrl driver for Microsemi Serial GPIO

[Lars Povlsen]

Linus Walleij writes:

> On Thu, Sep 3, 2020 at 3:35 PM Lars Povlsen <lars.povlsen@microchip.com> wrote:
>
>> This adds a pinctrl driver for the Microsemi/Microchip Serial GPIO
>> (SGPIO) device used in various SoC's.
>>
>> Signed-off-by: Lars Povlsen <lars.povlsen@microchip.com>
> (...)
>
>> diff --git a/drivers/pinctrl/pinctrl-mchp-sgpio.c b/drivers/pinctrl/pinctrl-mchp-sgpio.c
>
> Can we just spell it out
> pinctrl-microchip-sgpio.c ?
>
> The abbreviation seems arbitrary and unnecessary.

Well, not completely arbitrary, but maybe unnecessary... I'll change
that. I'll also change that for any symbols/defines off course.

>
> I do see that this chip is using the pinctrl abstractions (very nicely)
> and should be under drivers/pinctrl/*.
>
> Sadly it doesn't mean the bindings need to be in pinctrl ... unless you
> plan to add pinctrl bindings later.
>
>> +config PINCTRL_MCHP_SGPIO
>> +       bool "Pinctrl driver for Microsemi/Microchip Serial GPIO"
>> +       depends on OF
>> +       depends on HAS_IOMEM
>> +       select GPIOLIB
>> +       select GENERIC_PINCONF
>> +       select GENERIC_PINCTRL_GROUPS
>> +       select GENERIC_PINMUX_FUNCTIONS
>
> Nice use of these abstractions!

Thanks!

>
>> +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
>
> What's up with this OR MIT? I'd like Kate to OK this.
>
>> +#define MCHP_SGPIOS_PER_BANK   32
>> +#define MCHP_SGPIO_BANK_DEPTH  4
>> +
>> +#define PIN_NAM_SZ     (sizeof("SGPIO_D_pXXbY")+1)
>> +
>> +enum {
>> +       REG_INPUT_DATA,
>> +       REG_PORT_CONFIG,
>> +       REG_PORT_ENABLE,
>> +       REG_SIO_CONFIG,
>> +       REG_SIO_CLOCK,
>> +       MAXREG
>> +};
>> +
>> +struct mchp_sgpio_props {
>
> Just call it struct microchip_gpio_variant it is easier to read and
> does not abbreviate randomly, also it is a per-variant set of properties
> so calling it variant is more to the point.
>

Fine.

>> +struct mchp_sgpio_priv {
>
> I would just spell it out struct microchip_sgpio, it is implicit that
> the struct is private since it is defined in a c file.
>

Fine.

>> +struct mchp_sgpio_port_addr {
>
> struct microchip_sgpio_port_addr
>
> (Admittedly this is a bit about taste.)
>
>> +static inline void sgpio_writel(struct mchp_sgpio_priv *priv,
>> +                               u32 val, u32 rno, u32 off)
>> +{
>> +       u32 __iomem *reg = &priv->regs[priv->props->regoff[rno] + off];
>> +
>> +       writel(val, reg);
>> +}
>> +
>> +static inline void sgpio_clrsetbits(struct mchp_sgpio_priv *priv,
>> +                                   u32 rno, u32 off, u32 clear, u32 set)
>> +{
>> +       u32 __iomem *reg = &priv->regs[priv->props->regoff[rno] + off];
>> +       u32 val = readl(reg);
>> +
>> +       val &= ~clear;
>> +       val |= set;
>> +
>> +       writel(val, reg);
>> +}
>
> This looks like a reimplementation of regmap_update_bits for a bit,
> have you considered just using regmap? It's pretty simple.
>

Well, the registers are not in a regmap, so I did not consider that. The
utility function also serves to abstract the variant register address.

>> +static int mchp_sgpio_direction_input(struct gpio_chip *gc, unsigned int gpio)
>> +{
>> +       struct mchp_sgpio_priv *priv = gpiochip_get_data(gc);
>> +
>> +       /* Fixed-position function */
>> +       return sgpio_is_input(priv, gpio) ? 0 : -EINVAL;
>> +}
>> +
>> +static int mchp_sgpio_direction_output(struct gpio_chip *gc,
>> +                                      unsigned int gpio, int value)
>> +{
>> +       struct mchp_sgpio_priv *priv = gpiochip_get_data(gc);
>> +       struct mchp_sgpio_port_addr addr;
>> +
>> +       sgpio_pin_to_addr(priv, gpio, &addr);
>> +
>> +       /* Fixed-position function */
>> +       if (addr.input)
>> +               return -EINVAL;
>> +
>> +       sgpio_output_set(priv, &addr, value);
>> +
>> +       return 0;
>> +}
>
> This looks like the right way to handle this!

I'm glad you think so...

>
>> +static int mchp_sgpio_of_xlate(struct gpio_chip *gc,
>> +                              const struct of_phandle_args *gpiospec,
>> +                              u32 *flags)
>> +{
>> +       struct mchp_sgpio_priv *priv = gpiochip_get_data(gc);
>> +       int pin, base;
>> +
>> +       if (gpiospec->args[0] > MCHP_SGPIOS_PER_BANK ||
>> +           gpiospec->args[1] > priv->bitcount)
>> +               return -EINVAL;
>> +       base = priv->bitcount * gpiospec->args[0];
>> +       pin = base + gpiospec->args[1];
>> +       /* Add to 2nd half of output range if output */
>> +       if (gpiospec->args[2] == PIN_OUTPUT)
>> +               pin += (priv->ngpios / 2);
>> +
>> +       if (pin > gc->ngpio)
>> +               return -EINVAL;
>> +
>> +       if (flags)
>> +               *flags = gpiospec->args[3];
>> +
>> +       return pin;
>> +}
>
> I don't like this. I would certainly prefer the driver to just use standard
> GPIO bindings. I do not understand why this is necessary.
>
> If for nothing else, there should be a big comment explaining this.
>
> The only real problem I have with the driver is this extra flag tagged onto
> all the GPIOs, this seems unnecessary, and something the hardware
> driver should already know from the compatible string.

I hope my previous comments have cleared this up.

>
> Yours,
> Linus Walleij

Thank you for your time and comments!

---Lars

-- 
Lars Povlsen,
Microchip

Re: [PATCH v2 1/3] dt-bindings: pinctrl: Add bindings for pinctrl-mchp-sgpio driver

[Linus Walleij]

Hi Lars,

thanks for working on this!

On Sun, Sep 13, 2020 at 9:11 PM Lars Povlsen <lars.povlsen@microchip.com> wrote:

> > What I do not understand is why this GPIO controller is placed in the
> > bindings of the pin controllers? Do you plan to add pin control
> > properties to the bindings in the future?
>
> I have made provisions for some of the generic pinconf parameters, and
> since the controller also has support for some alternate modes like
> (syncronized) blink at various rates, I thought I better add it as
> pinctrl straight away.

OK fair enough let's keep the bindings here.

BTW the latter function sounds like some kind of PWM?

> >> +  gpio-controller: true
> >> +
> >> +  '#gpio-cells':
> >> +    description: GPIO consumers must specify four arguments, first the
> >> +      port number, then the bit number, then a input/output flag and
> >> +      finally the GPIO flags (from include/dt-bindings/gpio/gpio.h).
> >> +      The dt-bindings/gpio/mchp-sgpio.h file define manifest constants
> >> +      PIN_INPUT and PIN_OUTPUT.
> >> +    const: 4
> >
> > I do not follow this new third input/output flag at all.
>
> Its actually a sort of bank address, since the individual "pins" are
> unidirectional.

I'm a bit confused here...
The standard advice for any "banked" GPIOs is to represent
each "bank" as a separate node (with a corresponding gpio_chip
in the Linux kernel). Then you can just use the standard
bindings to pick a line from one of these nodes.

> The PIN_INPUT/PIN_OUTPUT is defined in similar fashion in other pinctrl
> binding header files... I can drop the define and use, but as it will be
> used to address individual pins, I think it adds to readability.

Hmmm. What makes these names expecially confusing is the
Documentation/devicetree/bindings/pinctrl/pincfg-node.yaml defines:
input-enable
input-disable
output-enable
output-high
output-low

In the Linux kernel further there is:
include/linux/pinctrl/pinconf-generic.h that defines:
PIN_CONFIG_INPUT_ENABLE
PIN_CONFIG_OUTPUT_ENABLE
PIN_CONFIG_OUTPUT

Since you are using the pin control framework this gets really
hard to hash out.

I don't really understand why it is needed.

> Like this (excerpts from a DT with a switchdev driver using SFP's and
> LED's on sgpio):
>
> /{
>         leds {
>                 compatible = "gpio-leds";
>                 led@0 {
>                         label = "eth60:yellow";
>                         gpios = <&sgpio1 28 0 PIN_OUTPUT GPIO_ACTIVE_LOW>;
>                         default-state = "off";
>                 };
>                 ...
>         };
> };

If what you intend to achieve is to make the GPIO come up in output mode,
you can either just have the driver do that as needed by the consumer.
If you absolutely have to do it in the device tree, then implement
pin control (pin config) and have it something like this:

leds {
        compatible = "gpio-leds";
        pinctrl-names = "default";
        pinctrl-0 = <&my_led_pinctrl>;
        led@0 {
                label = "eth60:yellow";
                gpios = <&sgpio1 28 GPIO_ACTIVE_LOW>;
                default-state = "off";
        };
        ...

        my_led_pinctrl: pinctrl-led {
                pins = "gpio95"; // Just an example way of referring to the pin
                bias-disable;
                output-enable;
        };
};

> >> +  microchip,sgpio-port-ranges:
> >> +    description: This is a sequence of tuples, defining intervals of
> >> +      enabled ports in the serial input stream. The enabled ports must
> >> +      match the hardware configuration in order for signals to be
> >> +      properly written/read to/from the controller holding
> >> +      registers. Being tuples, then number of arguments must be
> >> +      even. The tuples mast be ordered (low, high) and are
> >> +      inclusive. Arguments must be between 0 and 31.
> >> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> >> +    minItems: 2
> >> +    maxItems: 64
> >
> > And you are *absolutely sure* that you can't just figure this out
> > from the compatible string? Or add a few compatible strings for
> > the existing variants?
>
> Yes, this really needs to be configured for each board individually -
> and cant be probed. It defines how the bitstream to/from the shift
> registers is constructed/demuxed.

And you have considered the option of simply letting the driver
check which board we are then? The property at the very
top of the device tree.

if (of_machine_is_compatible("my_board")) {
    ....
} else if (of_machine_is_compatible("my_other_board")) {
    ....
}

So that you simply use the board compatible string to determine
this?

> >> +/* mchp-sgpio specific pin type defines */
> >> +#undef PIN_OUTPUT
> >> +#undef PIN_INPUT
> >> +#define PIN_OUTPUT     0
> >> +#define PIN_INPUT      1
> >
> > I'm not a fan of this. It seems like something that should be set in
> > response to the gpiochip callbacks .direction_input and
> > .direction_output callbacks.
> >
>
> As I tried to explain above, its a part of the pin address - aka bank
> selector - whether your are accessing the input or the output side. And
> since the directions have totally different - and concurrent - use, they
> need to be individually addressed, not "configured".
>
> In the example presented, sgpio2-p28b0 IN is loss-of-signal, and the
> OUT is the sfp tx-disable control.

I suspect the proper way to do it is to create one node for
the input side and one node for the output side and also create
two different gpio chips in the kernel.

my-device {
       compatible = "my-device";
       gpioin: input-gpio {
           ....
       };
       gpioout: output-gpio {
           ....
       };
};

Note: I didn't think over the naming in this example.

You will need code in your driver to parse the subnodes and
populate two gpio_chips.

Yours,
Linus Walleij

Re: [PATCH v2 1/3] dt-bindings: pinctrl: Add bindings for pinctrl-mchp-sgpio driver

[Lars Povlsen]

Linus Walleij writes:

> Hi Lars,
>
> thanks for working on this!
>
> On Sun, Sep 13, 2020 at 9:11 PM Lars Povlsen <lars.povlsen@microchip.com> wrote:
>
>> > What I do not understand is why this GPIO controller is placed in the
>> > bindings of the pin controllers? Do you plan to add pin control
>> > properties to the bindings in the future?
>>
>> I have made provisions for some of the generic pinconf parameters, and
>> since the controller also has support for some alternate modes like
>> (syncronized) blink at various rates, I thought I better add it as
>> pinctrl straight away.
>
> OK fair enough let's keep the bindings here.
>
> BTW the latter function sounds like some kind of PWM?

Yes, it has PWM functionality as well.

>
>> >> +  gpio-controller: true
>> >> +
>> >> +  '#gpio-cells':
>> >> +    description: GPIO consumers must specify four arguments, first the
>> >> +      port number, then the bit number, then a input/output flag and
>> >> +      finally the GPIO flags (from include/dt-bindings/gpio/gpio.h).
>> >> +      The dt-bindings/gpio/mchp-sgpio.h file define manifest constants
>> >> +      PIN_INPUT and PIN_OUTPUT.
>> >> +    const: 4
>> >
>> > I do not follow this new third input/output flag at all.
>>
>> Its actually a sort of bank address, since the individual "pins" are
>> unidirectional.
>
> I'm a bit confused here...
> The standard advice for any "banked" GPIOs is to represent
> each "bank" as a separate node (with a corresponding gpio_chip
> in the Linux kernel). Then you can just use the standard
> bindings to pick a line from one of these nodes.

Yes, that seems to be a good model.

>
>> The PIN_INPUT/PIN_OUTPUT is defined in similar fashion in other pinctrl
>> binding header files... I can drop the define and use, but as it will be
>> used to address individual pins, I think it adds to readability.
>
> Hmmm. What makes these names expecially confusing is the
> Documentation/devicetree/bindings/pinctrl/pincfg-node.yaml defines:
> input-enable
> input-disable
> output-enable
> output-high
> output-low
>
> In the Linux kernel further there is:
> include/linux/pinctrl/pinconf-generic.h that defines:
> PIN_CONFIG_INPUT_ENABLE
> PIN_CONFIG_OUTPUT_ENABLE
> PIN_CONFIG_OUTPUT
>
> Since you are using the pin control framework this gets really
> hard to hash out.
>

Yes, as the pins are fixed-function, the "input-enable", "input-disable"
and "output-enable" are not really useful.

> I don't really understand why it is needed.
>
>> Like this (excerpts from a DT with a switchdev driver using SFP's and
>> LED's on sgpio):
>>
>> /{
>>         leds {
>>                 compatible = "gpio-leds";
>>                 led@0 {
>>                         label = "eth60:yellow";
>>                         gpios = <&sgpio1 28 0 PIN_OUTPUT GPIO_ACTIVE_LOW>;
>>                         default-state = "off";
>>                 };
>>                 ...
>>         };
>> };
>
> If what you intend to achieve is to make the GPIO come up in output mode,
> you can either just have the driver do that as needed by the consumer.
> If you absolutely have to do it in the device tree, then implement
> pin control (pin config) and have it something like this:
>
> leds {
>         compatible = "gpio-leds";
>         pinctrl-names = "default";
>         pinctrl-0 = <&my_led_pinctrl>;
>         led@0 {
>                 label = "eth60:yellow";
>                 gpios = <&sgpio1 28 GPIO_ACTIVE_LOW>;
>                 default-state = "off";
>         };
>         ...
>
>         my_led_pinctrl: pinctrl-led {
>                 pins = "gpio95"; // Just an example way of referring to the pin
>                 bias-disable;
>                 output-enable;
>         };
> };

No, the PIN_OUTPUT is purely for adressing. But as you suggested, I'll
split the into separate nodes. That will eliminate the "PIN_OUTPUT" and
the bindings header.

>
>> >> +  microchip,sgpio-port-ranges:
>> >> +    description: This is a sequence of tuples, defining intervals of
>> >> +      enabled ports in the serial input stream. The enabled ports must
>> >> +      match the hardware configuration in order for signals to be
>> >> +      properly written/read to/from the controller holding
>> >> +      registers. Being tuples, then number of arguments must be
>> >> +      even. The tuples mast be ordered (low, high) and are
>> >> +      inclusive. Arguments must be between 0 and 31.
>> >> +    $ref: /schemas/types.yaml#/definitions/uint32-array
>> >> +    minItems: 2
>> >> +    maxItems: 64
>> >
>> > And you are *absolutely sure* that you can't just figure this out
>> > from the compatible string? Or add a few compatible strings for
>> > the existing variants?
>>
>> Yes, this really needs to be configured for each board individually -
>> and cant be probed. It defines how the bitstream to/from the shift
>> registers is constructed/demuxed.
>
> And you have considered the option of simply letting the driver
> check which board we are then? The property at the very
> top of the device tree.
>
> if (of_machine_is_compatible("my_board")) {
>     ....
> } else if (of_machine_is_compatible("my_other_board")) {
>     ....
> }

No, board-specific code is undesireable, as our customers should be able
to design own boards without driver changes.

>
> So that you simply use the board compatible string to determine
> this?
>
>> >> +/* mchp-sgpio specific pin type defines */
>> >> +#undef PIN_OUTPUT
>> >> +#undef PIN_INPUT
>> >> +#define PIN_OUTPUT     0
>> >> +#define PIN_INPUT      1
>> >
>> > I'm not a fan of this. It seems like something that should be set in
>> > response to the gpiochip callbacks .direction_input and
>> > .direction_output callbacks.
>> >
>>
>> As I tried to explain above, its a part of the pin address - aka bank
>> selector - whether your are accessing the input or the output side. And
>> since the directions have totally different - and concurrent - use, they
>> need to be individually addressed, not "configured".
>>
>> In the example presented, sgpio2-p28b0 IN is loss-of-signal, and the
>> OUT is the sfp tx-disable control.
>
> I suspect the proper way to do it is to create one node for
> the input side and one node for the output side and also create
> two different gpio chips in the kernel.
>
> my-device {
>        compatible = "my-device";
>        gpioin: input-gpio {
>            ....
>        };
>        gpioout: output-gpio {
>            ....
>        };
> };
>
> Note: I didn't think over the naming in this example.
>
> You will need code in your driver to parse the subnodes and
> populate two gpio_chips.

Yes, I will modify the driver to use separate nodes for each direction.

Thank you for your comments, it is highly appreciated.

---Lars

>
> Yours,
> Linus Walleij

-- 
Lars Povlsen,
Microchip