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

[RESEND PATCH v3 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.

The driver now register two separate banks (per instance), one for
input pins (the first) and one for output direction. This eliminates
the need for specifying the direction as part of the gpio
arguments. The (fixed) direction is thus inherent from the gpio
handle.

v3 changes:
- Renamed all usage of "mchp" abbrevation with "microchip".
- Split the in/output directions into (two) separate banks.
- Eliminated the bindings include file (from above)
- Changed SPDX license to "GPL-2.0-or-later"
- Change -ENOTSUPP to -EOPNOTSUPP
- Minor type/symbol naming changes

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       | 127 ++++
 MAINTAINERS                                   |   1 +
 arch/arm64/boot/dts/microchip/sparx5.dtsi     |  91 +++
 .../boot/dts/microchip/sparx5_pcb125.dts      |   5 +
 .../dts/microchip/sparx5_pcb134_board.dtsi    | 262 +++++++
 .../dts/microchip/sparx5_pcb135_board.dtsi    |  57 ++
 drivers/pinctrl/Kconfig                       |  18 +
 drivers/pinctrl/Makefile                      |   1 +
 drivers/pinctrl/pinctrl-microchip-sgpio.c     | 676 ++++++++++++++++++
 9 files changed, 1238 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/microchip,sparx5-sgpio.yaml
 create mode 100644 drivers/pinctrl/pinctrl-microchip-sgpio.c

--
2.25.1

[RESEND PATCH v3 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       | 127 ++++++++++++++++++
 1 file changed, 127 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/microchip,sparx5-sgpio.yaml

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..e3618ed28165
--- /dev/null
+++ b/Documentation/devicetree/bindings/pinctrl/microchip,sparx5-sgpio.yaml
@@ -0,0 +1,127 @@
+# 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@[0-9a-f]+$"
+
+  compatible:
+    enum:
+      - microchip,sparx5-sgpio
+      - mscc,ocelot-sgpio
+      - mscc,luton-sgpio
+
+  "#address-cells":
+    const: 1
+
+  "#size-cells":
+    const: 0
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    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
+
+patternProperties:
+  "^gpio-(port|controller)@[01]$":
+    type: object
+    properties:
+      compatible:
+        const: microchip,sparx5-sgpio-bank
+
+      reg:
+        maxItems: 1
+
+      gpio-controller: true
+
+      '#gpio-cells':
+        const: 3
+
+      ngpios:
+        minimum: 1
+        maximum: 128
+
+    required:
+      - compatible
+      - reg
+      - gpio-controller
+      - '#gpio-cells'
+      - ngpios
+
+    additionalProperties: false
+
+additionalProperties: false
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - microchip,sgpio-port-ranges
+  - "#address-cells"
+  - "#size-cells"
+
+examples:
+  - |
+    sgpio2: gpio@1101059c {
+    	#address-cells = <1>;
+    	#size-cells = <0>;
+    	compatible = "microchip,sparx5-sgpio";
+    	clocks = <&sys_clk>;
+    	pinctrl-0 = <&sgpio2_pins>;
+    	pinctrl-names = "default";
+    	reg = <0x1101059c 0x100>;
+        microchip,sgpio-port-ranges = <0 0 16 18 28 31>;
+        microchip,sgpio-frequency = <25000000>;
+    	sgpio_in2: gpio-controller@0 {
+            reg = <0>;
+            compatible = "microchip,sparx5-sgpio-bank";
+            gpio-controller;
+            #gpio-cells = <3>;
+            ngpios = <96>;
+    	};
+    	sgpio_out2: gpio-controller@1 {
+            compatible = "microchip,sparx5-sgpio-bank";
+            reg = <1>;
+            gpio-controller;
+            #gpio-cells = <3>;
+            ngpios = <96>;
+    	};
+    };
--
2.25.1

[RESEND PATCH v3 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                   |  18 +
 drivers/pinctrl/Makefile                  |   1 +
 drivers/pinctrl/pinctrl-microchip-sgpio.c | 676 ++++++++++++++++++++++
 4 files changed, 696 insertions(+)
 create mode 100644 drivers/pinctrl/pinctrl-microchip-sgpio.c

diff --git a/MAINTAINERS b/MAINTAINERS
index cc70e3ab428b..a8cc028b84d0 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-microchip-sgpio.c
 N:	sparx5

 ARM/MIOA701 MACHINE SUPPORT
diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index 8828613c4e0e..1b3527d4c3c8 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -404,6 +404,24 @@ config PINCTRL_OCELOT
 	select OF_GPIO
 	select REGMAP_MMIO

+config PINCTRL_MICROCHIP_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 serial GPIO interface used on Microsemi and
+	  Microchip SoC's. 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..347b0e015960 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_MICROCHIP_SGPIO)	+= pinctrl-microchip-sgpio.o
 obj-$(CONFIG_PINCTRL_EQUILIBRIUM)   += pinctrl-equilibrium.o

 obj-y				+= actions/
diff --git a/drivers/pinctrl/pinctrl-microchip-sgpio.c b/drivers/pinctrl/pinctrl-microchip-sgpio.c
new file mode 100644
index 000000000000..ccee60bc9f77
--- /dev/null
+++ b/drivers/pinctrl/pinctrl-microchip-sgpio.c
@@ -0,0 +1,676 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * 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 "core.h"
+#include "pinconf.h"
+#include "pinmux.h"
+
+#define SGPIO_BITS_PER_WORD	32
+#define SGPIO_MAX_BITS		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 sgpio_properties {
+	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 SGPIO_M_CFG_SIO_AUTO_REPEAT(p)	  ((p)->properties->auto_repeat)
+#define SGPIO_F_CFG_SIO_PORT_WIDTH(p, x)  __BF_PREP((p)->properties->port_width, x)
+#define SGPIO_M_CFG_SIO_PORT_WIDTH(p)	  ((p)->properties->port_width)
+#define SGPIO_F_CLOCK_SIO_CLK_FREQ(p, x)  __BF_PREP((p)->properties->clk_freq, x)
+#define SGPIO_M_CLOCK_SIO_CLK_FREQ(p)     ((p)->properties->clk_freq)
+#define SGPIO_F_PORT_CFG_BIT_SOURCE(p, x) __BF_PREP((p)->properties->bit_source, x)
+#define SGPIO_X_PORT_CFG_BIT_SOURCE(p, x) __BF_GET((p)->properties->bit_source, x)
+
+const struct sgpio_properties properties_luton = {
+	.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 sgpio_properties properties_ocelot = {
+	.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 sgpio_properties properties_sparx5 = {
+	.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 sgpio_priv {
+	struct device *dev;
+	/* 2 banks: IN/OUT */
+	struct {
+		struct gpio_chip gpio;
+		struct pinctrl_desc pctl_desc;
+		struct pinctrl_dev *pctl_dev;
+	} bank[2];
+	u32 bitcount;
+	u32 ports;
+	u32 clock;
+	u32 __iomem *regs;
+	const struct sgpio_properties *properties;
+};
+
+struct sgpio_port_addr {
+	u8 port;
+	u8 bit;
+};
+
+static inline bool sgpio_pctl_is_input(struct sgpio_priv *priv,
+				       struct pinctrl_dev *pctl)
+{
+	/* 1st index is input */
+	return pctl == priv->bank[0].pctl_dev;
+}
+
+static inline bool sgpio_gpio_is_input(struct sgpio_priv *priv,
+				       struct gpio_chip *gc)
+{
+	/* 1st index is input */
+	return gc == &priv->bank[0].gpio;
+}
+
+static inline void sgpio_pin_to_addr(struct sgpio_priv *priv, int pin,
+				     struct sgpio_port_addr *addr)
+{
+	addr->port = pin / priv->bitcount;
+	addr->bit = pin % priv->bitcount;
+}
+
+static inline u32 sgpio_readl(struct sgpio_priv *priv, u32 rno, u32 off)
+{
+	u32 __iomem *reg = &priv->regs[priv->properties->regoff[rno] + off];
+
+	return readl(reg);
+}
+
+static inline void sgpio_writel(struct sgpio_priv *priv,
+				u32 val, u32 rno, u32 off)
+{
+	u32 __iomem *reg = &priv->regs[priv->properties->regoff[rno] + off];
+
+	writel(val, reg);
+}
+
+static inline void sgpio_clrsetbits(struct sgpio_priv *priv,
+				    u32 rno, u32 off, u32 clear, u32 set)
+{
+	u32 __iomem *reg = &priv->regs[priv->properties->regoff[rno] + off];
+	u32 val = readl(reg);
+
+	val &= ~clear;
+	val |= set;
+
+	writel(val, reg);
+}
+
+static void sgpio_output_set(struct sgpio_priv *priv,
+			     struct 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,
+			 SGPIO_F_PORT_CFG_BIT_SOURCE(priv, mask),
+			 SGPIO_F_PORT_CFG_BIT_SOURCE(priv, value));
+}
+
+static int sgpio_output_get(struct sgpio_priv *priv,
+			    struct sgpio_port_addr *addr)
+{
+	u32 portval = sgpio_readl(priv, REG_PORT_CONFIG, addr->port);
+	int ret;
+
+	ret = SGPIO_X_PORT_CFG_BIT_SOURCE(priv, portval);
+	ret = !!(ret & (3 << (3 * addr->bit)));
+
+	return ret;
+}
+
+static int sgpio_input_get(struct sgpio_priv *priv,
+			   struct 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 sgpio_priv *priv = pinctrl_dev_get_drvdata(pctldev);
+	bool is_input = sgpio_pctl_is_input(priv, pctldev);
+	struct 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 = is_input;
+		break;
+
+	case PIN_CONFIG_OUTPUT_ENABLE:
+		val = !is_input;
+		break;
+
+	case PIN_CONFIG_OUTPUT:
+		if (is_input)
+			return -EINVAL;
+		val = sgpio_output_get(priv, &addr);
+		break;
+
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	*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 sgpio_priv *priv = pinctrl_dev_get_drvdata(pctldev);
+	bool is_input = sgpio_pctl_is_input(priv, pctldev);
+	struct 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 (is_input)
+				return -EINVAL;
+			sgpio_output_set(priv, &addr, arg);
+			break;
+
+		default:
+			err = -EOPNOTSUPP;
+		}
+	}
+
+	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 sgpio_priv *priv = pinctrl_dev_get_drvdata(pctldev);
+
+	if (input != sgpio_pctl_is_input(priv, pctldev)) {
+		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 sgpio_priv *priv = pinctrl_dev_get_drvdata(pctldev);
+	struct 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 sgpio_priv *priv = pinctrl_dev_get_drvdata(pctldev);
+	int bank = !sgpio_pctl_is_input(priv, pctldev);
+
+	return priv->bank[bank].pctl_desc.npins;
+}
+
+static const char *sgpio_pctl_get_group_name(struct pinctrl_dev *pctldev,
+					     unsigned int group)
+{
+	struct sgpio_priv *priv = pinctrl_dev_get_drvdata(pctldev);
+	int bank = !sgpio_pctl_is_input(priv, pctldev);
+
+	return priv->bank[bank].pctl_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 sgpio_priv *priv = pinctrl_dev_get_drvdata(pctldev);
+	int bank = !sgpio_pctl_is_input(priv, pctldev);
+
+	*pins = &priv->bank[bank].pctl_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 int microchip_sgpio_direction_input(struct gpio_chip *gc, unsigned int gpio)
+{
+	struct sgpio_priv *priv = gpiochip_get_data(gc);
+
+	/* Fixed-position function */
+	return sgpio_gpio_is_input(priv, gc) ? 0 : -EINVAL;
+}
+
+static int microchip_sgpio_direction_output(struct gpio_chip *gc,
+				       unsigned int gpio, int value)
+{
+	struct sgpio_priv *priv = gpiochip_get_data(gc);
+	bool is_input = sgpio_gpio_is_input(priv, gc);
+	struct sgpio_port_addr addr;
+
+	sgpio_pin_to_addr(priv, gpio, &addr);
+
+	/* Fixed-position function */
+	if (is_input)
+		return -EINVAL;
+
+	sgpio_output_set(priv, &addr, value);
+
+	return 0;
+}
+
+static int microchip_sgpio_get_direction(struct gpio_chip *gc, unsigned int gpio)
+{
+	struct sgpio_priv *priv = gpiochip_get_data(gc);
+
+	return sgpio_gpio_is_input(priv, gc); /* 0=out, 1=in */
+}
+
+static void microchip_sgpio_set_value(struct gpio_chip *gc,
+				unsigned int gpio, int value)
+{
+	microchip_sgpio_direction_output(gc, gpio, value);
+}
+
+static int microchip_sgpio_get_value(struct gpio_chip *gc, unsigned int gpio)
+{
+	struct sgpio_priv *priv = gpiochip_get_data(gc);
+	struct sgpio_port_addr addr;
+	int ret;
+
+	sgpio_pin_to_addr(priv, gpio, &addr);
+
+	if (sgpio_gpio_is_input(priv, gc))
+		ret = sgpio_input_get(priv, &addr);
+	else
+		ret = sgpio_output_get(priv, &addr);
+
+	return ret;
+}
+
+static int microchip_sgpio_of_xlate(struct gpio_chip *gc,
+			       const struct of_phandle_args *gpiospec,
+			       u32 *flags)
+{
+	struct sgpio_priv *priv = gpiochip_get_data(gc);
+	int pin;
+
+	if (gpiospec->args[0] > SGPIO_BITS_PER_WORD ||
+	    gpiospec->args[1] > priv->bitcount)
+		return -EINVAL;
+
+	pin = gpiospec->args[1] + (gpiospec->args[0] * priv->bitcount);
+
+	if (pin > gc->ngpio)
+		return -EINVAL;
+
+	if (flags)
+		*flags = gpiospec->args[2];
+
+	return pin;
+}
+
+static int microchip_sgpio_get_ports(struct sgpio_priv *priv)
+{
+	struct device *dev = priv->dev;
+	struct device_node *np = dev->of_node;
+	int i, ret;
+	u32 range_params[64];
+
+	/* 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 >= SGPIO_BITS_PER_WORD) {
+			dev_err(dev, "Ill-formed port-range [%d:%d]\n",
+				start, end);
+		}
+		priv->ports |= GENMASK(end, start);
+	}
+
+	return 0;
+}
+
+static int microchip_sgpio_register_bank(struct device *dev,
+					 struct sgpio_priv *priv,
+					 struct fwnode_handle *fwnode,
+					 int bank)
+{
+	struct pinctrl_dev *pctl;
+	struct pinctrl_pin_desc *pins;
+	struct pinctrl_desc *pctl_desc;
+	struct gpio_chip *gc;
+	bool is_input = (bank == 0);
+	int ret;
+	u32 ngpios;
+
+	if (fwnode_property_read_u32(fwnode, "ngpios", &ngpios)) {
+		dev_info(dev, "failed to get number of gpios for bank%d\n",
+			 bank);
+		ngpios = 64;
+	}
+
+	priv->bitcount = ngpios / SGPIO_BITS_PER_WORD;
+	if (priv->bitcount > SGPIO_MAX_BITS) {
+		dev_err(dev, "Bit width exceeds maximum (%d)\n",
+			SGPIO_MAX_BITS);
+		return -EINVAL;
+	}
+
+	pctl_desc = &priv->bank[bank].pctl_desc;
+	pctl_desc->name = devm_kasprintf(dev, GFP_KERNEL, "%s-%sput",
+					 dev_name(dev),
+					 is_input ? "in" : "out");
+	pctl_desc->pctlops = &sgpio_pctl_ops;
+	pctl_desc->pmxops = &sgpio_pmx_ops;
+	pctl_desc->confops = &sgpio_confops;
+	pctl_desc->owner = THIS_MODULE;
+
+	pins = devm_kzalloc(dev, sizeof(*pins)*ngpios, GFP_KERNEL);
+	if (pins) {
+		int i;
+		char *p, *names;
+
+		names = devm_kzalloc(dev, PIN_NAM_SZ*ngpios, GFP_KERNEL);
+
+		if (!names)
+			return -ENOMEM;
+
+		pctl_desc->npins = ngpios;
+		pctl_desc->pins = pins;
+
+		for (p = names, i = 0; i < ngpios; i++, p += PIN_NAM_SZ) {
+			struct sgpio_port_addr addr;
+
+			sgpio_pin_to_addr(priv, i, &addr);
+			snprintf(p, PIN_NAM_SZ, "SGPIO_%c_p%db%d",
+				 is_input ? 'I' : 'O',
+				 addr.port, addr.bit);
+			pins[i].number = i;
+			pins[i].name = p;
+		}
+	} else
+		return -ENOMEM;
+
+	pctl = devm_pinctrl_register(dev, pctl_desc, priv);
+	if (IS_ERR(pctl)) {
+		dev_err(dev, "Failed to register pinctrl\n");
+		return PTR_ERR(pctl);
+	}
+
+	gc			= &priv->bank[bank].gpio;
+	gc->label		= pctl_desc->name;
+	gc->parent		= dev;
+	gc->of_node		= to_of_node(fwnode);
+	gc->owner		= THIS_MODULE;
+	gc->get_direction	= microchip_sgpio_get_direction;
+	gc->direction_input	= microchip_sgpio_direction_input;
+	gc->direction_output	= microchip_sgpio_direction_output;
+	gc->get			= microchip_sgpio_get_value;
+	gc->set			= microchip_sgpio_set_value;
+	gc->request		= gpiochip_generic_request;
+	gc->free		= gpiochip_generic_free;
+	gc->of_xlate		= microchip_sgpio_of_xlate;
+	gc->of_gpio_n_cells     = 3;
+	gc->base		= -1;
+	gc->ngpio		= ngpios;
+
+	ret = devm_gpiochip_add_data(dev, gc, priv);
+	if (ret == 0)
+		dev_info(pctl->dev, "Registered %d GPIOs\n", ngpios);
+	else
+		dev_err(pctl->dev, "Failed to register: ret %d\n", ret);
+
+	priv->bank[bank].pctl_dev = pctl;
+
+	return ret;
+}
+
+static int microchip_sgpio_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct fwnode_handle *fwnode;
+	struct sgpio_priv *priv;
+	int div_clock = 0, ret, port;
+	u32 val;
+	struct resource *regs;
+	struct clk *clk;
+	int i, nbanks;
+
+	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->properties = of_device_get_match_data(dev);
+
+	/* Get rest of device properties */
+	ret = microchip_sgpio_get_ports(priv);
+	if (ret)
+		return ret;
+
+	nbanks = device_get_child_node_count(dev);
+	if (nbanks != 2) {
+		dev_err(dev, "Must have 2 banks (have %d)\n", nbanks);
+		return -EINVAL;
+	}
+
+	i = 0;
+	device_for_each_child_node(dev, fwnode) {
+		ret = microchip_sgpio_register_bank(dev, priv, fwnode, i++);
+		if (ret)
+			return ret;
+	}
+
+	if (priv->bank[0].gpio.ngpio != priv->bank[0].gpio.ngpio) {
+		dev_err(dev, "Banks must have same GPIO count\n");
+		return -EINVAL;
+	}
+
+	sgpio_clrsetbits(priv, REG_SIO_CONFIG, 0,
+			 SGPIO_M_CFG_SIO_PORT_WIDTH(priv),
+			 SGPIO_F_CFG_SIO_PORT_WIDTH(priv, priv->bitcount - 1) |
+			 SGPIO_M_CFG_SIO_AUTO_REPEAT(priv));
+	val = max(2U, div_clock / priv->clock);
+	sgpio_clrsetbits(priv, REG_SIO_CLOCK, 0,
+			 SGPIO_M_CLOCK_SIO_CLK_FREQ(priv),
+			 SGPIO_F_CLOCK_SIO_CLK_FREQ(priv, val));
+
+	for (port = 0; port < SGPIO_BITS_PER_WORD; port++)
+		sgpio_writel(priv, 0, REG_PORT_CONFIG, port);
+	sgpio_writel(priv, priv->ports, REG_PORT_ENABLE, 0);
+
+	return 0;
+}
+
+static const struct of_device_id microchip_sgpio_gpio_of_match[] = {
+	{
+		.compatible = "microchip,sparx5-sgpio",
+		.data = &properties_sparx5,
+	}, {
+		.compatible = "mscc,luton-sgpio",
+		.data = &properties_luton,
+	}, {
+		.compatible = "mscc,ocelot-sgpio",
+		.data = &properties_ocelot,
+	}, {
+		/* sentinel */
+	}
+};
+
+static struct platform_driver microchip_sgpio_pinctrl_driver = {
+	.driver = {
+		.name = "pinctrl-microchip-sgpio",
+		.of_match_table = microchip_sgpio_gpio_of_match,
+		.suppress_bind_attrs = true,
+	},
+	.probe = microchip_sgpio_probe,
+};
+builtin_platform_driver(microchip_sgpio_pinctrl_driver);
--
2.25.1

[RESEND PATCH v3 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     |  91 ++++++
 .../boot/dts/microchip/sparx5_pcb125.dts      |   5 +
 .../dts/microchip/sparx5_pcb134_board.dtsi    | 262 ++++++++++++++++++
 .../dts/microchip/sparx5_pcb135_board.dtsi    |  57 ++++
 4 files changed, 415 insertions(+)

diff --git a/arch/arm64/boot/dts/microchip/sparx5.dtsi b/arch/arm64/boot/dts/microchip/sparx5.dtsi
index fc1c7807249f..326ace6d35a3 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,81 @@ emmc_pins: emmc-pins {
 			};
 		};

+		sgpio0: gpio@61101036c {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			compatible = "microchip,sparx5-sgpio";
+			status = "disabled";
+			clocks = <&sys_clk>;
+			pinctrl-0 = <&sgpio0_pins>;
+			pinctrl-names = "default";
+			reg = <0x6 0x1101036c 0x100>;
+			sgpio_in0: gpio-controller@0 {
+				compatible = "microchip,sparx5-sgpio-bank";
+				reg = <0>;
+				gpio-controller;
+				#gpio-cells = <3>;
+				ngpios = <96>;
+			};
+			sgpio_out0: gpio-controller@1 {
+				compatible = "microchip,sparx5-sgpio-bank";
+				reg = <1>;
+				gpio-controller;
+				#gpio-cells = <3>;
+				ngpios = <96>;
+			};
+		};
+
+		sgpio1: gpio@611010484 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			compatible = "microchip,sparx5-sgpio";
+			status = "disabled";
+			clocks = <&sys_clk>;
+			pinctrl-0 = <&sgpio1_pins>;
+			pinctrl-names = "default";
+			reg = <0x6 0x11010484 0x100>;
+			sgpio_in1: gpio-controller@0 {
+				compatible = "microchip,sparx5-sgpio-bank";
+				reg = <0>;
+				gpio-controller;
+				#gpio-cells = <3>;
+				ngpios = <96>;
+			};
+			sgpio_out1: gpio-controller@1 {
+				compatible = "microchip,sparx5-sgpio-bank";
+				reg = <1>;
+				gpio-controller;
+				#gpio-cells = <3>;
+				ngpios = <96>;
+			};
+		};
+
+		sgpio2: gpio@61101059c {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			compatible = "microchip,sparx5-sgpio";
+			status = "disabled";
+			clocks = <&sys_clk>;
+			pinctrl-0 = <&sgpio2_pins>;
+			pinctrl-names = "default";
+			reg = <0x6 0x1101059c 0x100>;
+			sgpio_in2: gpio-controller@0 {
+				reg = <0>;
+				compatible = "microchip,sparx5-sgpio-bank";
+				gpio-controller;
+				#gpio-cells = <3>;
+				ngpios = <96>;
+			};
+			sgpio_out2: gpio-controller@1 {
+				compatible = "microchip,sparx5-sgpio-bank";
+				reg = <1>;
+				gpio-controller;
+				#gpio-cells = <3>;
+				ngpios = <96>;
+			};
+		};
+
 		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..9571f14ba51e 100644
--- a/arch/arm64/boot/dts/microchip/sparx5_pcb134_board.dtsi
+++ b/arch/arm64/boot/dts/microchip/sparx5_pcb134_board.dtsi
@@ -36,6 +36,242 @@ gpio-restart {
 		gpios = <&gpio 37 GPIO_ACTIVE_LOW>;
 		priority = <200>;
 	};
+
+	leds {
+		compatible = "gpio-leds";
+		led@0 {
+			label = "twr0:green";
+			gpios = <&sgpio_out0 8 0 GPIO_ACTIVE_LOW>;
+		};
+		led@1 {
+			label = "twr0:yellow";
+			gpios = <&sgpio_out0 8 1 GPIO_ACTIVE_LOW>;
+		};
+		led@2 {
+			label = "twr1:green";
+			gpios = <&sgpio_out0 9 0 GPIO_ACTIVE_LOW>;
+		};
+		led@3 {
+			label = "twr1:yellow";
+			gpios = <&sgpio_out0 9 1 GPIO_ACTIVE_LOW>;
+		};
+		led@4 {
+			label = "twr2:green";
+			gpios = <&sgpio_out0 10 0 GPIO_ACTIVE_LOW>;
+		};
+		led@5 {
+			label = "twr2:yellow";
+			gpios = <&sgpio_out0 10 1 GPIO_ACTIVE_LOW>;
+		};
+		led@6 {
+			label = "twr3:green";
+			gpios = <&sgpio_out0 11 0 GPIO_ACTIVE_LOW>;
+		};
+		led@7 {
+			label = "twr3:yellow";
+			gpios = <&sgpio_out0 11 1 GPIO_ACTIVE_LOW>;
+		};
+		led@8 {
+			label = "eth12:green";
+			gpios = <&sgpio_out0 12 0 GPIO_ACTIVE_HIGH>;
+			default-state = "off";
+		};
+		led@9 {
+			label = "eth12:yellow";
+			gpios = <&sgpio_out0 12 1 GPIO_ACTIVE_HIGH>;
+			default-state = "off";
+		};
+		led@10 {
+			label = "eth13:green";
+			gpios = <&sgpio_out0 13 0 GPIO_ACTIVE_HIGH>;
+			default-state = "off";
+		};
+		led@11 {
+			label = "eth13:yellow";
+			gpios = <&sgpio_out0 13 1 GPIO_ACTIVE_HIGH>;
+			default-state = "off";
+		};
+		led@12 {
+			label = "eth14:green";
+			gpios = <&sgpio_out0 14 0 GPIO_ACTIVE_HIGH>;
+			default-state = "off";
+		};
+		led@13 {
+			label = "eth14:yellow";
+			gpios = <&sgpio_out0 14 1 GPIO_ACTIVE_HIGH>;
+			default-state = "off";
+		};
+		led@14 {
+			label = "eth15:green";
+			gpios = <&sgpio_out0 15 0 GPIO_ACTIVE_HIGH>;
+			default-state = "off";
+		};
+		led@15 {
+			label = "eth15:yellow";
+			gpios = <&sgpio_out0 15 1 GPIO_ACTIVE_HIGH>;
+			default-state = "off";
+		};
+		led@16 {
+			label = "eth48:green";
+			gpios = <&sgpio_out1 16 0 GPIO_ACTIVE_HIGH>;
+			default-state = "off";
+		};
+		led@17 {
+			label = "eth48:yellow";
+			gpios = <&sgpio_out1 16 1 GPIO_ACTIVE_HIGH>;
+			default-state = "off";
+		};
+		led@18 {
+			label = "eth49:green";
+			gpios = <&sgpio_out1 17 0 GPIO_ACTIVE_HIGH>;
+			default-state = "off";
+		};
+		led@19 {
+			label = "eth49:yellow";
+			gpios = <&sgpio_out1 17 1 GPIO_ACTIVE_HIGH>;
+			default-state = "off";
+		};
+		led@20 {
+			label = "eth50:green";
+			gpios = <&sgpio_out1 18 0 GPIO_ACTIVE_HIGH>;
+			default-state = "off";
+		};
+		led@21 {
+			label = "eth50:yellow";
+			gpios = <&sgpio_out1 18 1 GPIO_ACTIVE_HIGH>;
+			default-state = "off";
+		};
+		led@22 {
+			label = "eth51:green";
+			gpios = <&sgpio_out1 19 0 GPIO_ACTIVE_HIGH>;
+			default-state = "off";
+		};
+		led@23 {
+			label = "eth51:yellow";
+			gpios = <&sgpio_out1 19 1 GPIO_ACTIVE_HIGH>;
+			default-state = "off";
+		};
+		led@24 {
+			label = "eth52:green";
+			gpios = <&sgpio_out1 20 0 GPIO_ACTIVE_HIGH>;
+			default-state = "off";
+		};
+		led@25 {
+			label = "eth52:yellow";
+			gpios = <&sgpio_out1 20 1 GPIO_ACTIVE_HIGH>;
+			default-state = "off";
+		};
+		led@26 {
+			label = "eth53:green";
+			gpios = <&sgpio_out1 21 0 GPIO_ACTIVE_HIGH>;
+			default-state = "off";
+		};
+		led@27 {
+			label = "eth53:yellow";
+			gpios = <&sgpio_out1 21 1 GPIO_ACTIVE_HIGH>;
+			default-state = "off";
+		};
+		led@28 {
+			label = "eth54:green";
+			gpios = <&sgpio_out1 22 0 GPIO_ACTIVE_HIGH>;
+			default-state = "off";
+		};
+		led@29 {
+			label = "eth54:yellow";
+			gpios = <&sgpio_out1 22 1 GPIO_ACTIVE_HIGH>;
+			default-state = "off";
+		};
+		led@30 {
+			label = "eth55:green";
+			gpios = <&sgpio_out1 23 0 GPIO_ACTIVE_HIGH>;
+			default-state = "off";
+		};
+		led@31 {
+			label = "eth55:yellow";
+			gpios = <&sgpio_out1 23 1 GPIO_ACTIVE_HIGH>;
+			default-state = "off";
+		};
+		led@32 {
+			label = "eth56:green";
+			gpios = <&sgpio_out1 24 0 GPIO_ACTIVE_HIGH>;
+			default-state = "off";
+		};
+		led@33 {
+			label = "eth56:yellow";
+			gpios = <&sgpio_out1 24 1 GPIO_ACTIVE_HIGH>;
+			default-state = "off";
+		};
+		led@34 {
+			label = "eth57:green";
+			gpios = <&sgpio_out1 25 0 GPIO_ACTIVE_HIGH>;
+			default-state = "off";
+		};
+		led@35 {
+			label = "eth57:yellow";
+			gpios = <&sgpio_out1 25 1 GPIO_ACTIVE_HIGH>;
+			default-state = "off";
+		};
+		led@36 {
+			label = "eth58:green";
+			gpios = <&sgpio_out1 26 0 GPIO_ACTIVE_HIGH>;
+			default-state = "off";
+		};
+		led@37 {
+			label = "eth58:yellow";
+			gpios = <&sgpio_out1 26 1 GPIO_ACTIVE_HIGH>;
+			default-state = "off";
+		};
+		led@38 {
+			label = "eth59:green";
+			gpios = <&sgpio_out1 27 0 GPIO_ACTIVE_HIGH>;
+			default-state = "off";
+		};
+		led@39 {
+			label = "eth59:yellow";
+			gpios = <&sgpio_out1 27 1 GPIO_ACTIVE_HIGH>;
+			default-state = "off";
+		};
+		led@40 {
+			label = "eth60:green";
+			gpios = <&sgpio_out1 28 0 GPIO_ACTIVE_HIGH>;
+			default-state = "off";
+		};
+		led@41 {
+			label = "eth60:yellow";
+			gpios = <&sgpio_out1 28 1 GPIO_ACTIVE_HIGH>;
+			default-state = "off";
+		};
+		led@42 {
+			label = "eth61:green";
+			gpios = <&sgpio_out1 29 0 GPIO_ACTIVE_HIGH>;
+			default-state = "off";
+		};
+		led@43 {
+			label = "eth61:yellow";
+			gpios = <&sgpio_out1 29 1 GPIO_ACTIVE_HIGH>;
+			default-state = "off";
+		};
+		led@44 {
+			label = "eth62:green";
+			gpios = <&sgpio_out1 30 0 GPIO_ACTIVE_HIGH>;
+			default-state = "off";
+		};
+		led@45 {
+			label = "eth62:yellow";
+			gpios = <&sgpio_out1 30 1 GPIO_ACTIVE_HIGH>;
+			default-state = "off";
+		};
+		led@46 {
+			label = "eth63:green";
+			gpios = <&sgpio_out1 31 0 GPIO_ACTIVE_HIGH>;
+			default-state = "off";
+		};
+		led@47 {
+			label = "eth63:yellow";
+			gpios = <&sgpio_out1 31 1 GPIO_ACTIVE_HIGH>;
+			default-state = "off";
+		};
+	};
 };

 &spi0 {
@@ -54,6 +290,32 @@ spi-flash@9 {
 	};
 };

+&sgpio0 {
+	status = "okay";
+	microchip,sgpio-port-ranges = <8 15>;
+};
+
+&sgpio_in0 {
+	ngpios = <64>;
+};
+
+&sgpio_out0 {
+	ngpios = <64>;
+};
+
+&sgpio1 {
+	status = "okay";
+	microchip,sgpio-port-ranges = <24 31>;
+};
+
+&sgpio_in1 {
+	ngpios = <64>;
+};
+
+&sgpio_out1 {
+	ngpios = <64>;
+};
+
 &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..c85e4d6900b6 100644
--- a/arch/arm64/boot/dts/microchip/sparx5_pcb135_board.dtsi
+++ b/arch/arm64/boot/dts/microchip/sparx5_pcb135_board.dtsi
@@ -20,6 +20,50 @@ gpio-restart {
 		gpios = <&gpio 37 GPIO_ACTIVE_LOW>;
 		priority = <200>;
 	};
+
+	leds {
+		compatible = "gpio-leds";
+		led@0 {
+			label = "eth60:yellow";
+			gpios = <&sgpio_out1 28 0 GPIO_ACTIVE_LOW>;
+			default-state = "off";
+		};
+		led@1 {
+			label = "eth60:green";
+			gpios = <&sgpio_out1 28 1 GPIO_ACTIVE_LOW>;
+			default-state = "off";
+		};
+		led@2 {
+			label = "eth61:yellow";
+			gpios = <&sgpio_out1 29 0 GPIO_ACTIVE_LOW>;
+			default-state = "off";
+		};
+		led@3 {
+			label = "eth61:green";
+			gpios = <&sgpio_out1 29 1 GPIO_ACTIVE_LOW>;
+			default-state = "off";
+		};
+		led@4 {
+			label = "eth62:yellow";
+			gpios = <&sgpio_out1 30 0 GPIO_ACTIVE_LOW>;
+			default-state = "off";
+		};
+		led@5 {
+			label = "eth62:green";
+			gpios = <&sgpio_out1 30 1 GPIO_ACTIVE_LOW>;
+			default-state = "off";
+		};
+		led@6 {
+			label = "eth63:yellow";
+			gpios = <&sgpio_out1 31 0 GPIO_ACTIVE_LOW>;
+			default-state = "off";
+		};
+		led@7 {
+			label = "eth63:green";
+			gpios = <&sgpio_out1 31 1 GPIO_ACTIVE_LOW>;
+			default-state = "off";
+		};
+	};
 };

 &gpio {
@@ -67,6 +111,19 @@ spi-flash@9 {
 	};
 };

+&sgpio1 {
+	status = "okay";
+	microchip,sgpio-port-ranges = <24 31>;
+};
+
+&sgpio_in1 {
+	ngpios = <64>;
+};
+
+&sgpio_out1 {
+	ngpios = <64>;
+};
+
 &axi {
 	i2c0_imux: i2c0-imux@0 {
 		compatible = "i2c-mux-pinctrl";
--
2.25.1

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

[Rob Herring]

On Tue, Oct 06, 2020 at 04:25:30PM +0200, Lars Povlsen wrote:
> 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       | 127 ++++++++++++++++++
>  1 file changed, 127 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pinctrl/microchip,sparx5-sgpio.yaml
> 
> 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..e3618ed28165
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pinctrl/microchip,sparx5-sgpio.yaml
> @@ -0,0 +1,127 @@
> +# 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@[0-9a-f]+$"
> +
> +  compatible:
> +    enum:
> +      - microchip,sparx5-sgpio
> +      - mscc,ocelot-sgpio
> +      - mscc,luton-sgpio
> +
> +  "#address-cells":
> +    const: 1
> +
> +  "#size-cells":
> +    const: 0
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    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
> +
> +patternProperties:
> +  "^gpio-(port|controller)@[01]$":

gpio@... is correct here as the node is a gpio-controller (no, we're 
not consistent).

> +    type: object
> +    properties:
> +      compatible:
> +        const: microchip,sparx5-sgpio-bank
> +
> +      reg:
> +        maxItems: 1
> +
> +      gpio-controller: true
> +
> +      '#gpio-cells':
> +        const: 3
> +
> +      ngpios:
> +        minimum: 1
> +        maximum: 128
> +
> +    required:
> +      - compatible
> +      - reg
> +      - gpio-controller
> +      - '#gpio-cells'
> +      - ngpios
> +
> +    additionalProperties: false
> +
> +additionalProperties: false
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - microchip,sgpio-port-ranges
> +  - "#address-cells"
> +  - "#size-cells"
> +
> +examples:
> +  - |
> +    sgpio2: gpio@1101059c {
> +    	#address-cells = <1>;
> +    	#size-cells = <0>;
> +    	compatible = "microchip,sparx5-sgpio";
> +    	clocks = <&sys_clk>;
> +    	pinctrl-0 = <&sgpio2_pins>;
> +    	pinctrl-names = "default";
> +    	reg = <0x1101059c 0x100>;
> +        microchip,sgpio-port-ranges = <0 0 16 18 28 31>;

Since it's tuples, do:

<0 0>, <16 18>, <28 31>

> +        microchip,sgpio-frequency = <25000000>;

Some whitespace issues here.


> +    	sgpio_in2: gpio-controller@0 {
> +            reg = <0>;
> +            compatible = "microchip,sparx5-sgpio-bank";
> +            gpio-controller;
> +            #gpio-cells = <3>;
> +            ngpios = <96>;
> +    	};
> +    	sgpio_out2: gpio-controller@1 {
> +            compatible = "microchip,sparx5-sgpio-bank";
> +            reg = <1>;
> +            gpio-controller;
> +            #gpio-cells = <3>;
> +            ngpios = <96>;
> +    	};
> +    };
> --
> 2.25.1

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

[Lars Povlsen]

Hi Rob!

Rob Herring writes:

> On Tue, Oct 06, 2020 at 04:25:30PM +0200, Lars Povlsen wrote:
>> 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       | 127 ++++++++++++++++++
>>  1 file changed, 127 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/pinctrl/microchip,sparx5-sgpio.yaml
>>
>> 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..e3618ed28165
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pinctrl/microchip,sparx5-sgpio.yaml
>> @@ -0,0 +1,127 @@
>> +# 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@[0-9a-f]+$"
>> +
>> +  compatible:
>> +    enum:
>> +      - microchip,sparx5-sgpio
>> +      - mscc,ocelot-sgpio
>> +      - mscc,luton-sgpio
>> +
>> +  "#address-cells":
>> +    const: 1
>> +
>> +  "#size-cells":
>> +    const: 0
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  clocks:
>> +    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
>> +
>> +patternProperties:
>> +  "^gpio-(port|controller)@[01]$":
>
> gpio@... is correct here as the node is a gpio-controller (no, we're
> not consistent).

OK, fine by me.

>
>> +    type: object
>> +    properties:
>> +      compatible:
>> +        const: microchip,sparx5-sgpio-bank
>> +
>> +      reg:
>> +        maxItems: 1
>> +
>> +      gpio-controller: true
>> +
>> +      '#gpio-cells':
>> +        const: 3
>> +
>> +      ngpios:
>> +        minimum: 1
>> +        maximum: 128
>> +
>> +    required:
>> +      - compatible
>> +      - reg
>> +      - gpio-controller
>> +      - '#gpio-cells'
>> +      - ngpios
>> +
>> +    additionalProperties: false
>> +
>> +additionalProperties: false
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - clocks
>> +  - microchip,sgpio-port-ranges
>> +  - "#address-cells"
>> +  - "#size-cells"
>> +
>> +examples:
>> +  - |
>> +    sgpio2: gpio@1101059c {
>> +     #address-cells = <1>;
>> +     #size-cells = <0>;
>> +     compatible = "microchip,sparx5-sgpio";
>> +     clocks = <&sys_clk>;
>> +     pinctrl-0 = <&sgpio2_pins>;
>> +     pinctrl-names = "default";
>> +     reg = <0x1101059c 0x100>;
>> +        microchip,sgpio-port-ranges = <0 0 16 18 28 31>;
>
> Since it's tuples, do:
>
> <0 0>, <16 18>, <28 31>

Yes, that will add clarity.

>
>> +        microchip,sgpio-frequency = <25000000>;
>
> Some whitespace issues here.
>

Will fix that.

>
>> +     sgpio_in2: gpio-controller@0 {
>> +            reg = <0>;
>> +            compatible = "microchip,sparx5-sgpio-bank";
>> +            gpio-controller;
>> +            #gpio-cells = <3>;
>> +            ngpios = <96>;
>> +     };
>> +     sgpio_out2: gpio-controller@1 {
>> +            compatible = "microchip,sparx5-sgpio-bank";
>> +            reg = <1>;
>> +            gpio-controller;
>> +            #gpio-cells = <3>;
>> +            ngpios = <96>;
>> +     };
>> +    };
>> --
>> 2.25.1

Thank you for your comments, I will refresh the series shortly.

---Lars

-- 
Lars Povlsen,
Microchip

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

[Linus Walleij]

Hi Lars!

Thanks for the update, this looks much improved!

Some further hints at things I saw here:

On Tue, Oct 6, 2020 at 4:25 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>

> +       /* 2 banks: IN/OUT */
> +       struct {
> +               struct gpio_chip gpio;
> +               struct pinctrl_desc pctl_desc;
> +               struct pinctrl_dev *pctl_dev;
> +       } bank[2];

Is it really good to use index [0,1] to refer to these?
Isnt it easier to do something like:

struct sgpio_bank {
         struct gpio_chip gpio;
         struct pinctrl_desc pctl_desc;
         struct pinctrl_dev *pctl_dev;
};

struct sgpio_priv {
        (...)
        struct sgpio_bank in;
        struct sgpio_bank out;
        (...)
};

This way you I think the code becomes clearer.

> +static inline bool sgpio_pctl_is_input(struct sgpio_priv *priv,
> +                                      struct pinctrl_dev *pctl)
> +{
> +       /* 1st index is input */
> +       return pctl == priv->bank[0].pctl_dev;
> +}
> +
> +static inline bool sgpio_gpio_is_input(struct sgpio_priv *priv,
> +                                      struct gpio_chip *gc)
> +{
> +       /* 1st index is input */
> +       return gc == &priv->bank[0].gpio;
> +}

Isn't it easier to just add a

bool is_input;

to the struct gpio_bank?

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

These accessors are somewhat re-implementing regmap-mmio, especially
the clrsetbits. I would consider just creating a regmap for each
struct sgpio_bank and manipulate the register through that.
(Not any requirement just a suggestion.)

> +static void sgpio_output_set(struct sgpio_priv *priv,
> +                            struct sgpio_port_addr *addr,
> +                            int value)
> +{
> +       u32 mask = 3 << (3 * addr->bit);
> +       value = (value & 3) << (3 * addr->bit);

I would spell it out a bit so it becomes clear what is going in
and use the bits helpers.

value & 3 doesn't make much sense since value here is always
0 or 1. Thus if value is 1 it in effect becomes value = 1 << (3 * addr->bit)
so with the above helper bit:

unsigned int bit = 3 * addr->bit;
u32 mask = GENMASK(bit+1, bit);
u32 val = BIT(bit);

This way it becomes clear that you set the output usin the lower
of the two bits.

Also note that I use val rather than value to avoid overwriting
the parameter: it is legal but confusing. Let the compiler optimize
register use.

> +static int sgpio_output_get(struct sgpio_priv *priv,
> +                           struct sgpio_port_addr *addr)
> +{
> +       u32 portval = sgpio_readl(priv, REG_PORT_CONFIG, addr->port);
> +       int ret;
> +
> +       ret = SGPIO_X_PORT_CFG_BIT_SOURCE(priv, portval);
> +       ret = !!(ret & (3 << (3 * addr->bit)));

Is the output direction really controlled by both bits?

ret = !!(ret & (BIT(3 * addr->bit))); ?

> +static int microchip_sgpio_get_direction(struct gpio_chip *gc, unsigned int gpio)
> +{
> +       struct sgpio_priv *priv = gpiochip_get_data(gc);
> +
> +       return sgpio_gpio_is_input(priv, gc); /* 0=out, 1=in */

You can use the #defines from <linux/gpio/driver.h> if you want to be explicit:

return sgpio_gpio_is_input(priv, gc) ? GPIO_LINE_DIRECTION_IN :
GPIO_LINE_DIRECTION_OUT;

> +static int microchip_sgpio_of_xlate(struct gpio_chip *gc,
> +                              const struct of_phandle_args *gpiospec,
> +                              u32 *flags)
> +{
> +       struct sgpio_priv *priv = gpiochip_get_data(gc);
> +       int pin;
> +
> +       if (gpiospec->args[0] > SGPIO_BITS_PER_WORD ||
> +           gpiospec->args[1] > priv->bitcount)
> +               return -EINVAL;
> +
> +       pin = gpiospec->args[1] + (gpiospec->args[0] * priv->bitcount);
> +
> +       if (pin > gc->ngpio)
> +               return -EINVAL;
> +
> +       if (flags)
> +               *flags = gpiospec->args[2];
> +
> +       return pin;
> +}

I'm still not convinced that you need the flags in args[2].

Just using the default of_gpio_simple_xlate() with one flag
should be fine. But I will go and review the bindings to figure
this out.

> +       gc->of_xlate            = microchip_sgpio_of_xlate;
> +       gc->of_gpio_n_cells     = 3;

So I'm sceptical to this.

Why can't you just use the pin index in cell 0 directly
and avoid cell 1?

Yours,
Linus Walleij

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

[Lars Povlsen]

Linus Walleij writes:

> Hi Lars!
>
> Thanks for the update, this looks much improved!

Glad you like it! It's been a difficult birth...

>
> Some further hints at things I saw here:
>
> On Tue, Oct 6, 2020 at 4:25 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>
>
>> +       /* 2 banks: IN/OUT */
>> +       struct {
>> +               struct gpio_chip gpio;
>> +               struct pinctrl_desc pctl_desc;
>> +               struct pinctrl_dev *pctl_dev;
>> +       } bank[2];
>
> Is it really good to use index [0,1] to refer to these?
> Isnt it easier to do something like:
>
> struct sgpio_bank {
>          struct gpio_chip gpio;
>          struct pinctrl_desc pctl_desc;
>          struct pinctrl_dev *pctl_dev;
> };
>
> struct sgpio_priv {
>         (...)
>         struct sgpio_bank in;
>         struct sgpio_bank out;
>         (...)
> };
>
> This way you I think the code becomes clearer.
>

I have done the change as suggested, and I think your right - looks
better. Also helped loose the "is_input" helper functions.

>> +static inline bool sgpio_pctl_is_input(struct sgpio_priv *priv,
>> +                                      struct pinctrl_dev *pctl)
>> +{
>> +       /* 1st index is input */
>> +       return pctl == priv->bank[0].pctl_dev;
>> +}
>> +
>> +static inline bool sgpio_gpio_is_input(struct sgpio_priv *priv,
>> +                                      struct gpio_chip *gc)
>> +{
>> +       /* 1st index is input */
>> +       return gc == &priv->bank[0].gpio;
>> +}
>
> Isn't it easier to just add a
>
> bool is_input;
>
> to the struct gpio_bank?

Yes, did that.

>
>> +static inline u32 sgpio_readl(struct sgpio_priv *priv, u32 rno, u32 off)
>> +{
>> +       u32 __iomem *reg = &priv->regs[priv->properties->regoff[rno] + off];
>> +
>> +       return readl(reg);
>> +}
>> +
>> +static inline void sgpio_writel(struct sgpio_priv *priv,
>> +                               u32 val, u32 rno, u32 off)
>> +{
>> +       u32 __iomem *reg = &priv->regs[priv->properties->regoff[rno] + off];
>> +
>> +       writel(val, reg);
>> +}
>> +
>> +static inline void sgpio_clrsetbits(struct sgpio_priv *priv,
>> +                                   u32 rno, u32 off, u32 clear, u32 set)
>> +{
>> +       u32 __iomem *reg = &priv->regs[priv->properties->regoff[rno] + off];
>> +       u32 val = readl(reg);
>> +
>> +       val &= ~clear;
>> +       val |= set;
>> +
>> +       writel(val, reg);
>> +}
>
> These accessors are somewhat re-implementing regmap-mmio, especially
> the clrsetbits. I would consider just creating a regmap for each
> struct sgpio_bank and manipulate the register through that.
> (Not any requirement just a suggestion.)
>

Humm, not sure if these few functions warrants using regmap. I'll have a
closer look.

>> +static void sgpio_output_set(struct sgpio_priv *priv,
>> +                            struct sgpio_port_addr *addr,
>> +                            int value)
>> +{
>> +       u32 mask = 3 << (3 * addr->bit);
>> +       value = (value & 3) << (3 * addr->bit);
>
> I would spell it out a bit so it becomes clear what is going in
> and use the bits helpers.
>
> value & 3 doesn't make much sense since value here is always
> 0 or 1. Thus if value is 1 it in effect becomes value = 1 << (3 * addr->bit)
> so with the above helper bit:
>
> unsigned int bit = 3 * addr->bit;
> u32 mask = GENMASK(bit+1, bit);
> u32 val = BIT(bit);
>
> This way it becomes clear that you set the output usin the lower
> of the two bits.
>
> Also note that I use val rather than value to avoid overwriting
> the parameter: it is legal but confusing. Let the compiler optimize
> register use.
>

I will change as suggested.

>> +static int sgpio_output_get(struct sgpio_priv *priv,
>> +                           struct sgpio_port_addr *addr)
>> +{
>> +       u32 portval = sgpio_readl(priv, REG_PORT_CONFIG, addr->port);
>> +       int ret;
>> +
>> +       ret = SGPIO_X_PORT_CFG_BIT_SOURCE(priv, portval);
>> +       ret = !!(ret & (3 << (3 * addr->bit)));
>
> Is the output direction really controlled by both bits?
>
> ret = !!(ret & (BIT(3 * addr->bit))); ?
>

The 3 bits are actually "mode" not direction. The direction is fixed as
described earlier.

0: Forced 0
1: Forced 1
2: Blink mode 0
3: Blink mode 1
4: Link activity blink mode 0
5: Link activity blink mode 1

But you are still right the (forced) _value_ can still be read using
just the first bit. I will change to using just the first bit.

>> +static int microchip_sgpio_get_direction(struct gpio_chip *gc, unsigned int gpio)
>> +{
>> +       struct sgpio_priv *priv = gpiochip_get_data(gc);
>> +
>> +       return sgpio_gpio_is_input(priv, gc); /* 0=out, 1=in */
>
> You can use the #defines from <linux/gpio/driver.h> if you want to be explicit:
>
> return sgpio_gpio_is_input(priv, gc) ? GPIO_LINE_DIRECTION_IN :
> GPIO_LINE_DIRECTION_OUT;
>

Yes, good suggestion. Also using bank->is_input now.

>> +static int microchip_sgpio_of_xlate(struct gpio_chip *gc,
>> +                              const struct of_phandle_args *gpiospec,
>> +                              u32 *flags)
>> +{
>> +       struct sgpio_priv *priv = gpiochip_get_data(gc);
>> +       int pin;
>> +
>> +       if (gpiospec->args[0] > SGPIO_BITS_PER_WORD ||
>> +           gpiospec->args[1] > priv->bitcount)
>> +               return -EINVAL;
>> +
>> +       pin = gpiospec->args[1] + (gpiospec->args[0] * priv->bitcount);
>> +
>> +       if (pin > gc->ngpio)
>> +               return -EINVAL;
>> +
>> +       if (flags)
>> +               *flags = gpiospec->args[2];
>> +
>> +       return pin;
>> +}
>
> I'm still not convinced that you need the flags in args[2].
>
> Just using the default of_gpio_simple_xlate() with one flag
> should be fine. But I will go and review the bindings to figure
> this out.
>

Ok, I just assumed the std GPIO flags in args[2] were kind of mandatory,
I mean polarity would be needed?

F.ex. a GPIO (led) looks like this now:

    led@0 {
            label = "eth60:yellow";
            gpios = <&sgpio_out1 28 1 GPIO_ACTIVE_LOW>;
            default-state = "off";
    };

>> +       gc->of_xlate            = microchip_sgpio_of_xlate;
>> +       gc->of_gpio_n_cells     = 3;
>
> So I'm sceptical to this.
>
> Why can't you just use the pin index in cell 0 directly
> and avoid cell 1?
>

You scepticism has surfaced before :-). The (now) 2 indices relates to
how the hardware address signals.

Each signal/pin is addressed by port, bit number and direction. We now
have the direction encoded in the bank/phandle.

While it would be possible to just produce a linear range of (32 *
width), hardware designs and documentation use pXXbY (as "p28b1" above),
making the cross reference much better using the 2 indices when
specifying a pin (as opposed to using f.ex. value "60" in the example).

Hope this helps.

Thank you for your comments, I will refresh later today.

---Lars

> Yours,
> Linus Walleij

-- 
Lars Povlsen,
Microchip

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

[Linus Walleij]

Hi Lars,

I'm overall mostly happy with the latest posting (not the one I respond to here)

On Thu, Oct 8, 2020 at 12:57 PM Lars Povlsen <lars.povlsen@microchip.com> wrote:
> > On Tue, Oct 6, 2020 at 4:25 PM Lars Povlsen <lars.povlsen@microchip.com> wrote:

> >> +       gc->of_xlate            = microchip_sgpio_of_xlate;
> >> +       gc->of_gpio_n_cells     = 3;
> >
> > So I'm sceptical to this.
> >
> > Why can't you just use the pin index in cell 0 directly
> > and avoid cell 1?
> >
>
> You scepticism has surfaced before :-). The (now) 2 indices relates to
> how the hardware address signals.
>
> Each signal/pin is addressed by port, bit number and direction. We now
> have the direction encoded in the bank/phandle.

I'm sorry but I just don't get it, I suppose. To me it is pretty
straight-forward
that the cells indicate the pin and then the flags. I do understand you
need the port at all, since this is implicit from the reg property
of the DT node. Are these two different things?

Yours,
Linus Walleij

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

[Lars Povlsen]

Linus Walleij writes:

> Hi Lars,
>
> I'm overall mostly happy with the latest posting (not the one I respond to here)

I'm glad we're getting there :-)

>
> On Thu, Oct 8, 2020 at 12:57 PM Lars Povlsen <lars.povlsen@microchip.com> wrote:
>> > On Tue, Oct 6, 2020 at 4:25 PM Lars Povlsen <lars.povlsen@microchip.com> wrote:
>
>> >> +       gc->of_xlate            = microchip_sgpio_of_xlate;
>> >> +       gc->of_gpio_n_cells     = 3;
>> >
>> > So I'm sceptical to this.
>> >
>> > Why can't you just use the pin index in cell 0 directly
>> > and avoid cell 1?
>> >
>>
>> You scepticism has surfaced before :-). The (now) 2 indices relates to
>> how the hardware address signals.
>>
>> Each signal/pin is addressed by port, bit number and direction. We now
>> have the direction encoded in the bank/phandle.
>
> I'm sorry but I just don't get it, I suppose. To me it is pretty
> straight-forward
> that the cells indicate the pin and then the flags. I do understand you
> need the port at all, since this is implicit from the reg property
> of the DT node. Are these two different things?

I responded to this in your comments to the DT bindings.

I just for got to offer to add a description for "#gpio-cells", I see
that's missing. That should make it "crystal clear" - I hope!

Something like:

--- a/Documentation/devicetree/bindings/pinctrl/microchip,sparx5-sgpio.yaml
+++ b/Documentation/devicetree/bindings/pinctrl/microchip,sparx5-sgpio.yaml
@@ -86,10 +86,17 @@ patternProperties:
       gpio-controller: true

       '#gpio-cells':
+        description: |
+         Specifies the pin (port and bit) and flags. Note that the
+         SGIO pin is defined by *2* numbers, a port number between 0
+         and 31, and a bit index, 0 to 3. The maximum bit number is
+         controlled indirectly by the "ngpios" property: (ngpios/32).
         const: 3

       ngpios:
-        minimum: 1
+        description: The numbers of GPIO's exposed. This must be a
+          multiple of 32.
+        minimum: 32
         maximum: 128

     required:

Would that be adequate, or should this also be added as a comment in
microchip_sgpio_of_xlate()?

Like:

    +       /* Note that the SGIO pin is defined by *2* numbers, a port
    +        * number between 0 and 31, and a bit index, 0 to 3.
    +        */
            if (gpiospec->args[0] > SGPIO_BITS_PER_WORD ||
                        gpiospec->args[1] > priv->bitcount)
                                        return -EINVAL;

I hope we can put this one to bed...

---Lars

>
> Yours,
> Linus Walleij

-- 
Lars Povlsen,
Microchip