[PATCH v2 0/3] pinctrl/GPIO driver for Apple SoCs

[PATCH v2 0/3] pinctrl/GPIO driver for Apple SoCs

[Joey Gouly]

Hi all,

Here is the v2 patchset for the Apple pinctrl/GPIO driver.

I'll be away for a week, and won't be able to reply to e-mails, but I
wanted to send this out now to keep progressing and maybe people can test with
the updates.

Changes since v1 [1]:
  - Removed USE_*_GENERIC_FN defines
  - Use apple,npins instead of gpio-ranges + dt-binding commit
  - Use _relaxed accesors
  - Use per-irq context data + gpiolib core commit
  - Moved some fields from apple_gpio_pinctrl to be local variables in 
    apple_gpio_pinctrl_probe
  - Simplify the register shadowing, by simply mirroring the entire
    value of the register.
  - Ran checkpatch.pl / clang-format to fix style issues.

There is a branch here with the driver:
  https://gitlab.arm.com/linux-arm/jg-open/-/commits/pinctrl_apple_v2

Thanks,
Joey

note: For those that have been testing this with PCIe etc, you will
probably want to also include the last commit in the following branch, as I
dropped the clock references in the code (due to the switch to power domains):
https://gitlab.arm.com/linux-arm/jg-open/-/commits/pinctrl_apple_v2_clock

[1]
https://lore.kernel.org/linux-gpio/20210921222956.40719-1-joey.gouly@arm.com/

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

Joey Gouly (1):
  dt-bindings: pinctrl: Add apple,npins property to apple,pinctrl

Marc Zyngier (1):
  gpio: Allow per-parent interrupt data

Stan Skowronek (1):
  pinctrl: add pinctrl/GPIO driver for Apple SoCs

 .../bindings/pinctrl/apple,pinctrl.yaml       |   4 +
 MAINTAINERS                                   |   1 +
 drivers/gpio/gpiolib.c                        |   9 +-
 drivers/pinctrl/Kconfig                       |  16 +
 drivers/pinctrl/Makefile                      |   1 +
 drivers/pinctrl/pinctrl-apple-gpio.c          | 561 ++++++++++++++++++
 include/linux/gpio/driver.h                   |  19 +-
 7 files changed, 607 insertions(+), 4 deletions(-)
 create mode 100644 drivers/pinctrl/pinctrl-apple-gpio.c

-- 
2.17.1

[PATCH v2 1/3] gpio: Allow per-parent interrupt data

[Joey Gouly]

From: Marc Zyngier <maz@kernel.org>

The core gpiolib code is able to deal with multiple interrupt parents
for a single gpio irqchip. It however only allows a single piece
of data to be conveyed to all flow handlers (either the gpio_chip
or some other, driver-specific data).

This means that drivers have to go through some interesting dance
to find the correct context, something that isn't great in interrupt
context (see aebdc8abc9db86e2bd33070fc2f961012fff74b4 for a prime
example).

Instead, offer an optional way for a pinctrl/gpio driver to provide
an array of pointers which gets used to provide the correct context
to the flow handler.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 drivers/gpio/gpiolib.c      |  9 +++++++--
 include/linux/gpio/driver.h | 19 +++++++++++++++++--
 2 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index d1b9b721218f..abfbf546d159 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1534,9 +1534,14 @@ static int gpiochip_add_irqchip(struct gpio_chip *gc,
 	}
 
 	if (gc->irq.parent_handler) {
-		void *data = gc->irq.parent_handler_data ?: gc;
-
 		for (i = 0; i < gc->irq.num_parents; i++) {
+			void *data;
+
+			if (gc->irq.per_parent_data)
+				data = gc->irq.parent_handler_data_array[i];
+			else
+				data = gc->irq.parent_handler_data ?: gc;
+
 			/*
 			 * The parent IRQ chip is already using the chip_data
 			 * for this IRQ chip, so our callbacks simply use the
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index a0f9901dcae6..a673a359e20b 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -168,11 +168,18 @@ struct gpio_irq_chip {
 
 	/**
 	 * @parent_handler_data:
+	 * @parent_handler_data_array:
 	 *
 	 * Data associated, and passed to, the handler for the parent
-	 * interrupt.
+	 * interrupt. Can either be a single pointer if @per_parent_data
+	 * is false, or an array of @num_parents pointers otherwise.  If
+	 * @per_parent_data is true, @parent_handler_data_array cannot be
+	 * NULL.
 	 */
-	void *parent_handler_data;
+	union {
+		void *parent_handler_data;
+		void **parent_handler_data_array;
+	};
 
 	/**
 	 * @num_parents:
@@ -203,6 +210,14 @@ struct gpio_irq_chip {
 	 */
 	bool threaded;
 
+	/**
+	 * @per_parent_data:
+	 *
+	 * True if parent_handler_data_array describes a @num_parents
+	 * sized array to be used as parent data.
+	 */
+	bool per_parent_data;
+
 	/**
 	 * @init_hw: optional routine to initialize hardware before
 	 * an IRQ chip will be added. This is quite useful when
-- 
2.17.1

Re: [PATCH v2 1/3] gpio: Allow per-parent interrupt data

[Linus Walleij]

On Fri, Oct 1, 2021 at 9:12 PM Joey Gouly <joey.gouly@arm.com> wrote:

> From: Marc Zyngier <maz@kernel.org>
>
> The core gpiolib code is able to deal with multiple interrupt parents
> for a single gpio irqchip. It however only allows a single piece
> of data to be conveyed to all flow handlers (either the gpio_chip
> or some other, driver-specific data).
>
> This means that drivers have to go through some interesting dance
> to find the correct context, something that isn't great in interrupt
> context (see aebdc8abc9db86e2bd33070fc2f961012fff74b4 for a prime
> example).
>
> Instead, offer an optional way for a pinctrl/gpio driver to provide
> an array of pointers which gets used to provide the correct context
> to the flow handler.
>
> Signed-off-by: Marc Zyngier <maz@kernel.org>

This works for me, bonus for the elegant use of union
here to make the array optional.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH v2 2/3] dt-bindings: pinctrl: Add apple,npins property to apple,pinctrl

[Joey Gouly]

This property is used to describe the total number of pins on this
particular pinctrl hardware block.

Signed-off-by: Joey Gouly <joey.gouly@arm.com>
---
 Documentation/devicetree/bindings/pinctrl/apple,pinctrl.yaml | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/pinctrl/apple,pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/apple,pinctrl.yaml
index d50571affd1f..cdd8cb454e92 100644
--- a/Documentation/devicetree/bindings/pinctrl/apple,pinctrl.yaml
+++ b/Documentation/devicetree/bindings/pinctrl/apple,pinctrl.yaml
@@ -34,6 +34,9 @@ properties:
   gpio-ranges:
     maxItems: 1
 
+  apple,npins:
+    maxItems: 1
+
   interrupts:
     description: One interrupt for each of the (up to 7) interrupt
       groups supported by the controller sorted by interrupt group
@@ -86,6 +89,7 @@ examples:
         gpio-controller;
         #gpio-cells = <2>;
         gpio-ranges = <&pinctrl 0 0 212>;
+        apple,npins = <212>;
 
         interrupt-controller;
         interrupt-parent = <&aic>;
-- 
2.17.1

Re: [PATCH v2 2/3] dt-bindings: pinctrl: Add apple,npins property to apple,pinctrl

[Mark Kettenis]

> From: Joey Gouly <joey.gouly@arm.com>
> Date: Fri, 1 Oct 2021 20:12:08 +0100
> 
> This property is used to describe the total number of pins on this
> particular pinctrl hardware block.
> 
> Signed-off-by: Joey Gouly <joey.gouly@arm.com>
> ---
>  Documentation/devicetree/bindings/pinctrl/apple,pinctrl.yaml | 4 ++++
>  1 file changed, 4 insertions(+)

Adding does property doesn't break the U-Boot driver, so I'm ok with
this.  This should probably be a required property though.

> diff --git a/Documentation/devicetree/bindings/pinctrl/apple,pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/apple,pinctrl.yaml
> index d50571affd1f..cdd8cb454e92 100644
> --- a/Documentation/devicetree/bindings/pinctrl/apple,pinctrl.yaml
> +++ b/Documentation/devicetree/bindings/pinctrl/apple,pinctrl.yaml
> @@ -34,6 +34,9 @@ properties:
>    gpio-ranges:
>      maxItems: 1
>  
> +  apple,npins:
> +    maxItems: 1
> +
>    interrupts:
>      description: One interrupt for each of the (up to 7) interrupt
>        groups supported by the controller sorted by interrupt group
> @@ -86,6 +89,7 @@ examples:
>          gpio-controller;
>          #gpio-cells = <2>;
>          gpio-ranges = <&pinctrl 0 0 212>;
> +        apple,npins = <212>;
>  
>          interrupt-controller;
>          interrupt-parent = <&aic>;
> -- 
> 2.17.1
> 
> 

Re: [PATCH v2 2/3] dt-bindings: pinctrl: Add apple,npins property to apple,pinctrl

[Rob Herring]

On Fri, 01 Oct 2021 20:12:08 +0100, Joey Gouly wrote:
> This property is used to describe the total number of pins on this
> particular pinctrl hardware block.
> 
> Signed-off-by: Joey Gouly <joey.gouly@arm.com>
> ---
>  Documentation/devicetree/bindings/pinctrl/apple,pinctrl.yaml | 4 ++++
>  1 file changed, 4 insertions(+)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/pinctrl/apple,pinctrl.yaml: properties:apple,npins: 'description' is a required property
	hint: Vendor specific properties must have a type and description unless they have a defined, common suffix.
	from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/pinctrl/apple,pinctrl.yaml: properties:apple,npins: 'oneOf' conditional failed, one must be fixed:
	'type' is a required property
		hint: A vendor boolean property can use "type: boolean"
	Additional properties are not allowed ('maxItems' was unexpected)
		hint: A vendor boolean property can use "type: boolean"
	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/pinctrl/apple,pinctrl.yaml: properties:apple,npins: 'oneOf' conditional failed, one must be fixed:
		'enum' is a required property
		'const' is a required property
		hint: A vendor string property with exact values has an implicit type
		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/pinctrl/apple,pinctrl.yaml: properties:apple,npins: 'oneOf' conditional failed, one must be fixed:
		'$ref' is a required property
		'allOf' is a required property
		hint: A vendor property needs a $ref to types.yaml
		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
	hint: Vendor specific properties must have a type and description unless they have a defined, common suffix.
	from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/pinctrl/apple,pinctrl.yaml: ignoring, error in schema: properties: apple,npins
warning: no schema found in file: ./Documentation/devicetree/bindings/pinctrl/apple,pinctrl.yaml
Documentation/devicetree/bindings/pinctrl/apple,pinctrl.example.dt.yaml:0:0: /example-0/soc/pinctrl@23c100000: failed to match any schema with compatible: ['apple,t8103-pinctrl', 'apple,pinctrl']
Documentation/devicetree/bindings/pinctrl/apple,pinctrl.example.dt.yaml:0:0: /example-0/soc/pinctrl@23c100000: failed to match any schema with compatible: ['apple,t8103-pinctrl', 'apple,pinctrl']

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/1535492

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

Re: [PATCH v2 2/3] dt-bindings: pinctrl: Add apple,npins property to apple,pinctrl

[Linus Walleij]

On Fri, Oct 1, 2021 at 9:12 PM Joey Gouly <joey.gouly@arm.com> wrote:

> This property is used to describe the total number of pins on this
> particular pinctrl hardware block.
>
> Signed-off-by: Joey Gouly <joey.gouly@arm.com>

Fair enough!
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

> +  apple,npins
> +    maxItems: 1

Add some smallish description like "the number of pins in this pin
controller instance" when you resend, but no big deal.

Yours,
Linus Walleij

[PATCH v2 3/3] pinctrl: add pinctrl/GPIO driver for Apple SoCs

[Joey Gouly]

From: Stan Skowronek <stan@corellium.com>

This driver adds support for the pinctrl / GPIO hardware found
on some Apple SoCs.

Co-developed-by: Joey Gouly <joey.gouly@arm.com>
Signed-off-by: Joey Gouly <joey.gouly@arm.com>
Signed-off-by: Stan Skowronek <stan@corellium.com>
---
 MAINTAINERS                          |   1 +
 drivers/pinctrl/Kconfig              |  16 +
 drivers/pinctrl/Makefile             |   1 +
 drivers/pinctrl/pinctrl-apple-gpio.c | 561 +++++++++++++++++++++++++++
 4 files changed, 579 insertions(+)
 create mode 100644 drivers/pinctrl/pinctrl-apple-gpio.c

diff --git a/MAINTAINERS b/MAINTAINERS
index ca6d6fde85cf..e83e992b8ada 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1722,6 +1722,7 @@ F:	Documentation/devicetree/bindings/interrupt-controller/apple,aic.yaml
 F:	Documentation/devicetree/bindings/pinctrl/apple,pinctrl.yaml
 F:	arch/arm64/boot/dts/apple/
 F:	drivers/irqchip/irq-apple-aic.c
+F:	drivers/pinctrl/pinctrl-apple-gpio.c
 F:	include/dt-bindings/interrupt-controller/apple-aic.h
 F:	include/dt-bindings/pinctrl/apple.h
 
diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index 31921108e456..6a961d5f8726 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -31,6 +31,22 @@ config DEBUG_PINCTRL
 	help
 	  Say Y here to add some extra checks and diagnostics to PINCTRL calls.
 
+config PINCTRL_APPLE_GPIO
+	tristate "Apple SoC GPIO pin controller driver"
+	depends on ARCH_APPLE
+	select PINMUX
+	select GPIOLIB
+	select GPIOLIB_IRQCHIP
+	select GENERIC_PINCTRL_GROUPS
+	select GENERIC_PINMUX_FUNCTIONS
+	select OF_GPIO
+	help
+	  This is the driver for the GPIO controller found on Apple ARM SoCs,
+	  including M1.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called pinctrl-apple-gpio.
+
 config PINCTRL_ARTPEC6
 	bool "Axis ARTPEC-6 pin controller driver"
 	depends on MACH_ARTPEC6
diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
index 200073bcc2c1..5e63de2ffcf4 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -8,6 +8,7 @@ obj-$(CONFIG_PINMUX)		+= pinmux.o
 obj-$(CONFIG_PINCONF)		+= pinconf.o
 obj-$(CONFIG_OF)		+= devicetree.o
 obj-$(CONFIG_GENERIC_PINCONF)	+= pinconf-generic.o
+obj-$(CONFIG_PINCTRL_APPLE_GPIO) += pinctrl-apple-gpio.o
 obj-$(CONFIG_PINCTRL_ARTPEC6)	+= pinctrl-artpec6.o
 obj-$(CONFIG_PINCTRL_AS3722)	+= pinctrl-as3722.o
 obj-$(CONFIG_PINCTRL_AXP209)	+= pinctrl-axp209.o
diff --git a/drivers/pinctrl/pinctrl-apple-gpio.c b/drivers/pinctrl/pinctrl-apple-gpio.c
new file mode 100644
index 000000000000..cfec37e3627a
--- /dev/null
+++ b/drivers/pinctrl/pinctrl-apple-gpio.c
@@ -0,0 +1,561 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Apple SoC pinctrl+GPIO+external IRQ driver
+ *
+ * Copyright (C) 2021 The Asahi Linux Contributors
+ * Copyright (C) 2020 Corellium LLC
+ *
+ * Based on: pinctrl-pistachio.c
+ * Copyright (C) 2014 Imagination Technologies Ltd.
+ * Copyright (C) 2014 Google, Inc.
+ */
+
+#include <dt-bindings/pinctrl/apple.h>
+#include <linux/clk.h>
+#include <linux/gpio/driver.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/pinctrl/pinmux.h>
+#include <linux/platform_device.h>
+
+#include "pinctrl-utils.h"
+#include "core.h"
+#include "pinmux.h"
+
+struct apple_gpio_irq_data {
+	struct apple_gpio_pinctrl *pctl;
+	int irqgrp;
+};
+
+struct apple_gpio_pinctrl {
+	struct device *dev;
+	struct pinctrl_dev *pctldev;
+
+	// Shadow the pin values, the REG_GPIOx_DATA bit can read back stale values.
+	u32 *pin_shadow;
+
+	void __iomem *base;
+	unsigned int nirqgrps;
+
+	struct pinctrl_desc pinctrl_desc;
+	struct gpio_chip gpio_chip;
+	struct irq_chip irq_chip;
+};
+
+#define REG_GPIO(x)          (4 * (x))
+#define REG_GPIOx_DATA       BIT(0)
+#define REG_GPIOx_MODE_MASK  GENMASK(3, 1)
+#define REG_GPIOx_OUT        1
+#define REG_GPIOx_IN_IRQ_HI  2
+#define REG_GPIOx_IN_IRQ_LO  3
+#define REG_GPIOx_IN_IRQ_UP  4
+#define REG_GPIOx_IN_IRQ_DN  5
+#define REG_GPIOx_IN_IRQ_ANY 6
+#define REG_GPIOx_IN_IRQ_OFF 7
+#define REG_GPIOx_PERIPH     BIT(5)
+#define REG_GPIOx_CFG_DONE   BIT(9)
+#define REG_GPIOx_GRP_MASK   GENMASK(18, 16)
+#define REG_IRQ(g, x)        (0x800 + 0x40 * (g) + 4 * ((x) >> 5))
+
+// No locking needed to mask/unmask IRQs as the interrupt mode is per pin-register.
+static void apple_gpio_set_reg(struct apple_gpio_pinctrl *pctl,
+			       unsigned int pin, uint32_t clr, uint32_t set)
+{
+	void __iomem *ppin = pctl->base + REG_GPIO(pin);
+	uint32_t prev, cfg;
+
+	prev = pctl->pin_shadow[pin];
+	cfg = (prev & ~clr) | set;
+
+	if (!(prev & REG_GPIOx_CFG_DONE))
+		writel_relaxed(cfg & ~REG_GPIOx_CFG_DONE, ppin);
+	writel_relaxed(cfg, ppin);
+	pctl->pin_shadow[pin] = cfg;
+}
+
+static uint32_t apple_gpio_get_reg(struct apple_gpio_pinctrl *pctl,
+				   unsigned int pin)
+{
+	return readl_relaxed(pctl->base + REG_GPIO(pin));
+}
+
+/* Pin controller functions */
+
+static int apple_gpio_dt_node_to_map(struct pinctrl_dev *pctldev,
+				     struct device_node *node,
+				     struct pinctrl_map **map,
+				     unsigned *num_maps)
+{
+	unsigned reserved_maps;
+	struct apple_gpio_pinctrl *pctl;
+	u32 pinfunc, pin, func;
+	int num_pins, i, ret;
+	const char *group_name;
+	const char *function_name;
+
+	*map = NULL;
+	*num_maps = 0;
+	reserved_maps = 0;
+
+	pctl = pinctrl_dev_get_drvdata(pctldev);
+
+	ret = of_property_count_u32_elems(node, "pinmux");
+	if (ret <= 0) {
+		dev_err(pctl->dev,
+			"missing or empty pinmux property in node %pOFn.\n",
+			node);
+		return -EINVAL;
+	}
+
+	num_pins = ret;
+
+	ret = pinctrl_utils_reserve_map(pctldev, map, &reserved_maps, num_maps,
+					num_pins);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < num_pins; i++) {
+		ret = of_property_read_u32_index(node, "pinmux", i, &pinfunc);
+		if (ret)
+			goto free_map;
+
+		pin = APPLE_PIN(pinfunc);
+		func = APPLE_FUNC(pinfunc);
+
+		if (func >= pinmux_generic_get_function_count(pctldev)) {
+			ret = -EINVAL;
+			goto free_map;
+		}
+
+		group_name = pinctrl_generic_get_group_name(pctldev, pin);
+		function_name =
+			pinmux_generic_get_function_name(pctl->pctldev, func);
+		ret = pinctrl_utils_add_map_mux(pctl->pctldev, map,
+						&reserved_maps, num_maps,
+						group_name, function_name);
+		if (ret)
+			goto free_map;
+	}
+
+free_map:
+	if (ret < 0) {
+		pinctrl_utils_free_map(pctldev, *map, *num_maps);
+		return ret;
+	}
+
+	return 0;
+}
+
+static const struct pinctrl_ops apple_gpio_pinctrl_ops = {
+	.get_groups_count = pinctrl_generic_get_group_count,
+	.get_group_name = pinctrl_generic_get_group_name,
+	.get_group_pins = pinctrl_generic_get_group_pins,
+	.dt_node_to_map = apple_gpio_dt_node_to_map,
+	.dt_free_map = pinctrl_utils_free_map,
+};
+
+/* Pin multiplexer functions */
+
+static int apple_gpio_pinmux_enable(struct pinctrl_dev *pctldev, unsigned func,
+				    unsigned group)
+{
+	struct apple_gpio_pinctrl *pctl = pinctrl_dev_get_drvdata(pctldev);
+
+	if (func)
+		apple_gpio_set_reg(pctl, group, 0,
+				   REG_GPIOx_PERIPH | REG_GPIOx_CFG_DONE);
+	else
+		apple_gpio_set_reg(pctl, group, REG_GPIOx_PERIPH,
+				   REG_GPIOx_CFG_DONE);
+
+	return 0;
+}
+
+static const struct pinmux_ops apple_gpio_pinmux_ops = {
+	.get_functions_count = pinmux_generic_get_function_count,
+	.get_function_name = pinmux_generic_get_function_name,
+	.get_function_groups = pinmux_generic_get_function_groups,
+	.set_mux = apple_gpio_pinmux_enable,
+	.strict = true,
+};
+
+/* GPIO chip functions */
+
+static int apple_gpio_gpio_get_direction(struct gpio_chip *chip,
+					 unsigned int offset)
+{
+	struct apple_gpio_pinctrl *pctl = gpiochip_get_data(chip);
+
+	return (FIELD_GET(REG_GPIOx_MODE_MASK, pctl->pin_shadow[offset]) ==
+		REG_GPIOx_OUT) ?
+		       GPIO_LINE_DIRECTION_OUT :
+			     GPIO_LINE_DIRECTION_IN;
+}
+
+static int apple_gpio_gpio_get(struct gpio_chip *chip, unsigned offset)
+{
+	struct apple_gpio_pinctrl *pctl = gpiochip_get_data(chip);
+	uint32_t reg;
+
+	if (FIELD_GET(REG_GPIOx_MODE_MASK, pctl->pin_shadow[offset]) ==
+	    REG_GPIOx_OUT) {
+		return pctl->pin_shadow[offset] & REG_GPIOx_DATA;
+	}
+
+	reg = apple_gpio_get_reg(pctl, offset);
+	return !!(reg & REG_GPIOx_DATA);
+}
+
+static void apple_gpio_gpio_set(struct gpio_chip *chip, unsigned int offset,
+				int value)
+{
+	struct apple_gpio_pinctrl *pctl = gpiochip_get_data(chip);
+
+	apple_gpio_set_reg(pctl, offset, REG_GPIOx_DATA,
+			   REG_GPIOx_CFG_DONE | (value & REG_GPIOx_DATA));
+}
+
+static int apple_gpio_gpio_direction_input(struct gpio_chip *chip,
+					   unsigned int offset)
+{
+	struct apple_gpio_pinctrl *pctl = gpiochip_get_data(chip);
+
+	apple_gpio_set_reg(pctl, offset, REG_GPIOx_MODE_MASK | REG_GPIOx_DATA,
+			   FIELD_PREP(REG_GPIOx_MODE_MASK,
+				      REG_GPIOx_IN_IRQ_OFF) |
+				   REG_GPIOx_CFG_DONE);
+	return 0;
+}
+
+static int apple_gpio_gpio_direction_output(struct gpio_chip *chip,
+					    unsigned int offset, int value)
+{
+	struct apple_gpio_pinctrl *pctl = gpiochip_get_data(chip);
+
+	apple_gpio_set_reg(pctl, offset, REG_GPIOx_PERIPH | REG_GPIOx_DATA,
+			   FIELD_PREP(REG_GPIOx_MODE_MASK, REG_GPIOx_OUT) |
+				   (value & REG_GPIOx_DATA) |
+				   REG_GPIOx_CFG_DONE);
+	return 0;
+}
+
+/* IRQ chip functions */
+
+static void apple_gpio_gpio_irq_ack(struct irq_data *data)
+{
+	struct apple_gpio_pinctrl *pctl =
+		gpiochip_get_data(irq_data_get_irq_chip_data(data));
+	unsigned int irqgrp = FIELD_GET(REG_GPIOx_GRP_MASK,
+					apple_gpio_get_reg(pctl, data->hwirq));
+
+	writel(BIT(data->hwirq & 31),
+	       pctl->base + REG_IRQ(irqgrp, data->hwirq));
+}
+
+static int apple_gpio_irq_type(unsigned int type)
+{
+	switch (type & IRQ_TYPE_SENSE_MASK) {
+	case IRQ_TYPE_EDGE_RISING:
+		return REG_GPIOx_IN_IRQ_UP;
+	case IRQ_TYPE_EDGE_FALLING:
+		return REG_GPIOx_IN_IRQ_DN;
+	case IRQ_TYPE_EDGE_BOTH:
+		return REG_GPIOx_IN_IRQ_ANY;
+	case IRQ_TYPE_LEVEL_HIGH:
+		return REG_GPIOx_IN_IRQ_HI;
+	case IRQ_TYPE_LEVEL_LOW:
+		return REG_GPIOx_IN_IRQ_LO;
+	default:
+		return -EINVAL;
+	}
+}
+
+static void apple_gpio_gpio_irq_mask(struct irq_data *data)
+{
+	struct apple_gpio_pinctrl *pctl =
+		gpiochip_get_data(irq_data_get_irq_chip_data(data));
+	apple_gpio_set_reg(pctl, data->hwirq, REG_GPIOx_MODE_MASK,
+			   FIELD_PREP(REG_GPIOx_MODE_MASK,
+				      REG_GPIOx_IN_IRQ_OFF) |
+				   REG_GPIOx_CFG_DONE);
+}
+
+static void apple_gpio_gpio_irq_unmask(struct irq_data *data)
+{
+	struct apple_gpio_pinctrl *pctl =
+		gpiochip_get_data(irq_data_get_irq_chip_data(data));
+	u32 irqtype = apple_gpio_irq_type(irqd_get_trigger_type(data));
+
+	if (WARN_ON(irqtype < 0))
+		return;
+	apple_gpio_set_reg(pctl, data->hwirq, REG_GPIOx_MODE_MASK,
+			   FIELD_PREP(REG_GPIOx_MODE_MASK, irqtype) |
+				   REG_GPIOx_CFG_DONE);
+}
+
+static unsigned int apple_gpio_gpio_irq_startup(struct irq_data *data)
+{
+	struct gpio_chip *chip = irq_data_get_irq_chip_data(data);
+	struct apple_gpio_pinctrl *pctl = gpiochip_get_data(chip);
+
+	apple_gpio_set_reg(pctl, data->hwirq, REG_GPIOx_GRP_MASK,
+			   FIELD_PREP(REG_GPIOx_GRP_MASK, 0));
+
+	apple_gpio_gpio_direction_input(chip, data->hwirq);
+	apple_gpio_gpio_irq_unmask(data);
+
+	return 0;
+}
+
+static int apple_gpio_gpio_irq_set_type(struct irq_data *data,
+					unsigned int type)
+{
+	struct apple_gpio_pinctrl *pctl =
+		gpiochip_get_data(irq_data_get_irq_chip_data(data));
+	u32 irqtype = apple_gpio_irq_type(type);
+
+	if (irqtype < 0)
+		return irqtype;
+
+	irqd_set_trigger_type(data, type);
+
+	apple_gpio_set_reg(pctl, data->hwirq, REG_GPIOx_MODE_MASK,
+			   FIELD_PREP(REG_GPIOx_MODE_MASK, irqtype) |
+				   REG_GPIOx_CFG_DONE);
+
+	if (type & IRQ_TYPE_LEVEL_MASK)
+		irq_set_handler_locked(data, handle_level_irq);
+	else
+		irq_set_handler_locked(data, handle_edge_irq);
+	return 0;
+}
+
+static void apple_gpio_gpio_irq_handler(struct irq_desc *desc)
+{
+	struct apple_gpio_irq_data *data = irq_desc_get_handler_data(desc);
+	struct apple_gpio_pinctrl *pctl = data->pctl;
+	struct gpio_chip *gc = &pctl->gpio_chip;
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	unsigned int irqgrp = data->irqgrp;
+	unsigned int pinh, pinl;
+	unsigned long pending;
+
+	chained_irq_enter(chip, desc);
+	for (pinh = 0; pinh < gc->ngpio; pinh += 32) {
+		pending = readl(pctl->base + REG_IRQ(irqgrp, pinh));
+		for_each_set_bit(pinl, &pending, 32)
+			generic_handle_domain_irq(gc->irq.domain, pinh + pinl);
+	}
+	chained_irq_exit(chip, desc);
+}
+
+/* Probe & register */
+
+static int apple_gpio_gpio_register(struct apple_gpio_pinctrl *pctl)
+{
+	struct device_node *node = pctl->dev->of_node;
+	struct gpio_irq_chip *girq;
+	struct apple_gpio_irq_data **irq_data;
+	int i;
+	int ret;
+
+	if (!of_find_property(node, "gpio-controller", NULL)) {
+		dev_err(pctl->dev,
+			"Apple GPIO must have 'gpio-controller' property.\n");
+		return -ENODEV;
+	}
+
+	pctl->gpio_chip.label = dev_name(pctl->dev);
+	pctl->gpio_chip.request = gpiochip_generic_request;
+	pctl->gpio_chip.free = gpiochip_generic_free;
+	pctl->gpio_chip.get_direction = apple_gpio_gpio_get_direction;
+	pctl->gpio_chip.direction_input = apple_gpio_gpio_direction_input;
+	pctl->gpio_chip.direction_output = apple_gpio_gpio_direction_output;
+	pctl->gpio_chip.get = apple_gpio_gpio_get;
+	pctl->gpio_chip.set = apple_gpio_gpio_set;
+	pctl->gpio_chip.base = -1;
+	pctl->gpio_chip.ngpio = pctl->pinctrl_desc.npins;
+	pctl->gpio_chip.parent = pctl->dev;
+	pctl->gpio_chip.of_node = node;
+
+	if (of_property_read_bool(node, "interrupt-controller")) {
+		ret = platform_irq_count(to_platform_device(pctl->dev));
+		if (ret < 0)
+			return ret;
+
+		pctl->nirqgrps = ret;
+
+		pctl->irq_chip.name = "Apple-GPIO";
+		pctl->irq_chip.irq_startup = apple_gpio_gpio_irq_startup;
+		pctl->irq_chip.irq_ack = apple_gpio_gpio_irq_ack;
+		pctl->irq_chip.irq_mask = apple_gpio_gpio_irq_mask;
+		pctl->irq_chip.irq_unmask = apple_gpio_gpio_irq_unmask;
+		pctl->irq_chip.irq_set_type = apple_gpio_gpio_irq_set_type;
+
+		girq = &pctl->gpio_chip.irq;
+		girq->chip = &pctl->irq_chip;
+		girq->parent_handler = apple_gpio_gpio_irq_handler;
+		girq->num_parents = pctl->nirqgrps;
+
+		girq->parents = kmalloc_array(
+			pctl->nirqgrps, sizeof(*girq->parents), GFP_KERNEL);
+		if (!girq->parents)
+			return -ENOMEM;
+
+		for (i = 0; i < pctl->nirqgrps; i++) {
+			ret = platform_get_irq(to_platform_device(pctl->dev),
+					       i);
+			if (ret < 0) {
+				return dev_err_probe(pctl->dev, ret,
+						     "Failed to map IRQ %d\n",
+						     i);
+			}
+			girq->parents[i] = ret;
+		}
+
+		irq_data = kmalloc_array(pctl->nirqgrps, sizeof(*irq_data),
+					 GFP_KERNEL);
+		if (!irq_data) {
+			kfree(girq->parents);
+			return -ENOMEM;
+		}
+		for (i = 0; i < pctl->nirqgrps; i++) {
+			irq_data[i] = devm_kzalloc(
+				pctl->dev, sizeof(*irq_data[i]), GFP_KERNEL);
+			irq_data[i]->pctl = pctl;
+			irq_data[i]->irqgrp = i;
+		}
+
+		girq->parent_handler_data_array = (void **)irq_data;
+		girq->per_parent_data = true;
+		girq->default_type = IRQ_TYPE_NONE;
+		girq->handler = handle_level_irq;
+	}
+
+	ret = devm_gpiochip_add_data(pctl->dev, &pctl->gpio_chip, pctl);
+
+	if (of_property_read_bool(node, "interrupt-controller")) {
+		kfree(girq->parents);
+		kfree(irq_data);
+	}
+
+	return ret;
+}
+
+static int apple_gpio_pinctrl_probe(struct platform_device *pdev)
+{
+	struct apple_gpio_pinctrl *pctl;
+	struct pinctrl_pin_desc *pins;
+	unsigned int npins;
+	const char **pin_names;
+	unsigned int *pin_nums;
+	unsigned int i, reg;
+	int res;
+
+	pctl = devm_kzalloc(&pdev->dev, sizeof(*pctl), GFP_KERNEL);
+	if (!pctl)
+		return -ENOMEM;
+	pctl->dev = &pdev->dev;
+	dev_set_drvdata(&pdev->dev, pctl);
+
+	if (of_property_read_u32(pdev->dev.of_node, "apple,npins", &npins)) {
+		dev_err(&pdev->dev, "gpio-ranges property not found\n");
+		npins = 512;
+		return -EINVAL;
+	}
+
+	pins = devm_kmalloc_array(&pdev->dev, npins, sizeof(pins[0]),
+				  GFP_KERNEL);
+	if (!pins)
+		return -ENOMEM;
+	pin_names = devm_kmalloc_array(&pdev->dev, npins, sizeof(pin_names[0]),
+				       GFP_KERNEL);
+	if (!pin_names)
+		return -ENOMEM;
+	pin_nums = devm_kmalloc_array(&pdev->dev, npins, sizeof(pin_nums[0]),
+				      GFP_KERNEL);
+	if (!pin_nums)
+		return -ENOMEM;
+	pctl->pin_shadow = devm_kmalloc_array(
+		&pdev->dev, npins, sizeof(pctl->pin_shadow[0]), GFP_KERNEL);
+	if (!pctl->pin_shadow)
+		return -ENOMEM;
+
+	pctl->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(pctl->base))
+		return PTR_ERR(pctl->base);
+
+	for (i = 0; i < npins; i++) {
+		reg = apple_gpio_get_reg(pctl, i);
+		pctl->pin_shadow[i] = reg;
+		pins[i].number = i;
+		pins[i].name =
+			devm_kasprintf(&pdev->dev, GFP_KERNEL, "PIN%u", i);
+		pins[i].drv_data = pctl;
+		pin_names[i] = pins[i].name;
+		pin_nums[i] = i;
+	}
+
+	pctl->pinctrl_desc.name = dev_name(pctl->dev);
+	pctl->pinctrl_desc.pins = pins;
+	pctl->pinctrl_desc.npins = npins;
+	pctl->pinctrl_desc.pctlops = &apple_gpio_pinctrl_ops;
+	pctl->pinctrl_desc.pmxops = &apple_gpio_pinmux_ops;
+
+	pctl->pctldev =
+		devm_pinctrl_register(&pdev->dev, &pctl->pinctrl_desc, pctl);
+	if (IS_ERR(pctl->pctldev)) {
+		dev_err(&pdev->dev, "Failed to register pinctrl device.\n");
+		return PTR_ERR(pctl->pctldev);
+	}
+
+	for (i = 0; i < npins; i++) {
+		res = pinctrl_generic_add_group(pctl->pctldev, pins[i].name,
+						pin_nums + i, 1, pctl);
+		if (res < 0) {
+			dev_err(pctl->dev, "Failed to register group.");
+			return res;
+		}
+	}
+
+	res = pinmux_generic_add_function(pctl->pctldev, "gpio", pin_names,
+					  npins, pctl);
+	if (res < 0) {
+		dev_err(pctl->dev, "Failed to register function.");
+		return res;
+	}
+
+	res = pinmux_generic_add_function(pctl->pctldev, "periph", pin_names,
+					  npins, pctl);
+	if (res < 0) {
+		dev_err(pctl->dev, "Failed to register function.");
+		return res;
+	}
+
+	return apple_gpio_gpio_register(pctl);
+}
+
+static const struct of_device_id apple_gpio_pinctrl_of_match[] = {
+	{ .compatible = "apple,t8103-pinctrl", },
+	{ .compatible = "apple,pinctrl", },
+	{ }
+};
+
+static struct platform_driver apple_gpio_pinctrl_driver = {
+	.driver = {
+		.name = "apple-gpio-pinctrl",
+		.of_match_table = apple_gpio_pinctrl_of_match,
+		.suppress_bind_attrs = true,
+	},
+	.probe = apple_gpio_pinctrl_probe,
+};
+module_platform_driver(apple_gpio_pinctrl_driver);
+
+MODULE_DESCRIPTION("Apple pinctrl/GPIO driver");
+MODULE_AUTHOR("Stan Skowronek <stan@corellium.com>");
+MODULE_AUTHOR("Joey Gouly <joey.gouly@arm.com>");
+MODULE_LICENSE("GPL v2");
-- 
2.17.1

Re: [PATCH v2 3/3] pinctrl: add pinctrl/GPIO driver for Apple SoCs

[Linus Walleij]

Hi Joey!

over all this driver is much improved and using a lot of stock functions
in the pin control core and getting really clean and compact.
I have one major nit below:

On Fri, Oct 1, 2021 at 9:12 PM Joey Gouly <joey.gouly@arm.com> wrote:

> +struct apple_gpio_pinctrl {
> +       // Shadow the pin values, the REG_GPIOx_DATA bit can read back stale values.
> +       u32 *pin_shadow;
(...)
> +// No locking needed to mask/unmask IRQs as the interrupt mode is per pin-register.
> +static void apple_gpio_set_reg(struct apple_gpio_pinctrl *pctl,
> +                              unsigned int pin, uint32_t clr, uint32_t set)
> +{
> +       void __iomem *ppin = pctl->base + REG_GPIO(pin);
> +       uint32_t prev, cfg;
> +
> +       prev = pctl->pin_shadow[pin];
> +       cfg = (prev & ~clr) | set;
> +
> +       if (!(prev & REG_GPIOx_CFG_DONE))
> +               writel_relaxed(cfg & ~REG_GPIOx_CFG_DONE, ppin);
> +       writel_relaxed(cfg, ppin);
> +       pctl->pin_shadow[pin] = cfg;
> +}

Are you not simply reinventing regmap-mmio here?

Keeping shadows of registers including write-only registers
is exactly what regmap does.

Check it out, if in doubt consult Mark Brown, I'm pretty
sure we can add what you need to regmap if it is missing.

> +static uint32_t apple_gpio_get_reg(struct apple_gpio_pinctrl *pctl,
> +                                  unsigned int pin)
> +{
> +       return readl_relaxed(pctl->base + REG_GPIO(pin));
> +}

If you use regmap-mmio I am pretty sure this will also
return the right value for the shadowed registers which it
currently does not IIUC.

> +               apple_gpio_set_reg(pctl, group, 0,
> +                                  REG_GPIOx_PERIPH | REG_GPIOx_CFG_DONE);
> +       else
> +               apple_gpio_set_reg(pctl, group, REG_GPIOx_PERIPH,
> +                                  REG_GPIOx_CFG_DONE);

I think all calls to apple_gpio_set_reg() could be replaced with
regmap_update_bits() or similar.

Yours,
Linus Walleij

Re: [PATCH v2 3/3] pinctrl: add pinctrl/GPIO driver for Apple SoCs

[Hector Martin]

On 02/10/2021 04.12, Joey Gouly wrote:
> +#define REG_GPIO(x)          (4 * (x))
> +#define REG_GPIOx_DATA       BIT(0)
> +#define REG_GPIOx_MODE_MASK  GENMASK(3, 1)
> +#define REG_GPIOx_OUT        1
> +#define REG_GPIOx_IN_IRQ_HI  2
> +#define REG_GPIOx_IN_IRQ_LO  3
> +#define REG_GPIOx_IN_IRQ_UP  4
> +#define REG_GPIOx_IN_IRQ_DN  5
> +#define REG_GPIOx_IN_IRQ_ANY 6
> +#define REG_GPIOx_IN_IRQ_OFF 7
> +#define REG_GPIOx_PERIPH     BIT(5)
> +#define REG_GPIOx_CFG_DONE   BIT(9)
> +#define REG_GPIOx_GRP_MASK   GENMASK(18, 16)
> +#define REG_IRQ(g, x)        (0x800 + 0x40 * (g) + 4 * ((x) >> 5))

Can we update these defines with the correct definitions and names we 
figured out the other day and add the missing ones? We now know a bunch 
of these are wrong (e.g. CFG_DONE is INPUT_ENABLE, PERIPH should be two 
bits, we're missing pull-up control, drive strength, schmitt trigger and 
lock bits). Even if we don't implement all the features in the driver 
yet, we should have all the register bit defines for documentation 
purposes at least.

> +	if (!(prev & REG_GPIOx_CFG_DONE))
> +		writel_relaxed(cfg & ~REG_GPIOx_CFG_DONE, ppin);
> +	writel_relaxed(cfg, ppin);

We already determined this dance doesn't make any sense; if we want to 
change the pin config before enabling the input buffer (whether this 
serves any purpose at all is an open question) then that should be 
handled in the upper code responsible for enabling/disabling the input 
buffer, not in the core register wrappers.

> +	if (func)
> +		apple_gpio_set_reg(pctl, group, 0,
> +				   REG_GPIOx_PERIPH | REG_GPIOx_CFG_DONE);
> +	else
> +		apple_gpio_set_reg(pctl, group, REG_GPIOx_PERIPH,
> +				   REG_GPIOx_CFG_DONE);

Func is two bits (4 functions) :)

> +static void apple_gpio_gpio_set(struct gpio_chip *chip, unsigned int offset,
> +				int value)
> +{
> +	struct apple_gpio_pinctrl *pctl = gpiochip_get_data(chip);
> +
> +	apple_gpio_set_reg(pctl, offset, REG_GPIOx_DATA,
> +			   REG_GPIOx_CFG_DONE | (value & REG_GPIOx_DATA));
> +}

`value ? REG_GPIOx_DATA : 0` please, otherwise this makes assumptions 
about value always being 1 and REG_GPIOx_DATA being the LSB.

Also as we now know, REG_GPIOx_CFG_DONE is nonsense and doesn't belong 
here. Let's drop the cargo cult and drive the hardware based on how it 
works, not how macOS or Corellium decided to do things.

> +static int apple_gpio_gpio_direction_input(struct gpio_chip *chip,
> +					   unsigned int offset)
> +{
> +	struct apple_gpio_pinctrl *pctl = gpiochip_get_data(chip);
> +
> +	apple_gpio_set_reg(pctl, offset, REG_GPIOx_MODE_MASK | REG_GPIOx_DATA,
> +			   FIELD_PREP(REG_GPIOx_MODE_MASK,
> +				      REG_GPIOx_IN_IRQ_OFF) |
> +				   REG_GPIOx_CFG_DONE);

Is hardcoding IRQ_OFF correct here? Shouldn't this be getting the 
intended IRQ state from somewhere, or is it always guaranteed that that 
gets set later?

> +static int apple_gpio_gpio_direction_output(struct gpio_chip *chip,
> +					    unsigned int offset, int value)
> +{
> +	struct apple_gpio_pinctrl *pctl = gpiochip_get_data(chip);
> +
> +	apple_gpio_set_reg(pctl, offset, REG_GPIOx_PERIPH | REG_GPIOx_DATA,
> +			   FIELD_PREP(REG_GPIOx_MODE_MASK, REG_GPIOx_OUT) |
> +				   (value & REG_GPIOx_DATA) |
> +				   REG_GPIOx_CFG_DONE);

I actually wonder if we should even bother turning on the input buffer 
for output pins, given we're shadowing the value anyway. Seems 
unnecessary and might save a few nanowatts.

Also, why is this clearing the peripheral (yet direction_input isn't)?

> +static void apple_gpio_gpio_irq_mask(struct irq_data *data)
> +{
> +	struct apple_gpio_pinctrl *pctl =
> +		gpiochip_get_data(irq_data_get_irq_chip_data(data));
> +	apple_gpio_set_reg(pctl, data->hwirq, REG_GPIOx_MODE_MASK,
> +			   FIELD_PREP(REG_GPIOx_MODE_MASK,
> +				      REG_GPIOx_IN_IRQ_OFF) |
> +				   REG_GPIOx_CFG_DONE);
> +}

-REG_GPIOx_CFG_DONE please

> +
> +static void apple_gpio_gpio_irq_unmask(struct irq_data *data)
> +{
> +	struct apple_gpio_pinctrl *pctl =
> +		gpiochip_get_data(irq_data_get_irq_chip_data(data));
> +	u32 irqtype = apple_gpio_irq_type(irqd_get_trigger_type(data));
> +
> +	if (WARN_ON(irqtype < 0))
> +		return;
> +	apple_gpio_set_reg(pctl, data->hwirq, REG_GPIOx_MODE_MASK,
> +			   FIELD_PREP(REG_GPIOx_MODE_MASK, irqtype) |
> +				   REG_GPIOx_CFG_DONE);

Ditto

> +static unsigned int apple_gpio_gpio_irq_startup(struct irq_data *data)
> +{
> +	struct gpio_chip *chip = irq_data_get_irq_chip_data(data);
> +	struct apple_gpio_pinctrl *pctl = gpiochip_get_data(chip);
> +
> +	apple_gpio_set_reg(pctl, data->hwirq, REG_GPIOx_GRP_MASK,
> +			   FIELD_PREP(REG_GPIOx_GRP_MASK, 0));

I guess we're only using a single IRQ group right now?

The driver structure looks good (though see the regmap suggestion from 
Linus). Let's just get the actual hardware part right. I didn't spend a 
couple hours poking register bits with a multimeter, a scope, and a 
breadboard for nothing ;)

-- 
Hector Martin (marcan@marcan.st)
Public Key: https://mrcn.st/pub

Re: [PATCH v2 3/3] pinctrl: add pinctrl/GPIO driver for Apple SoCs

[Joey Gouly]

Hi!

On Mon, Oct 04, 2021 at 12:33:15PM +0900, Hector Martin wrote:
> On 02/10/2021 04.12, Joey Gouly wrote:
> > +#define REG_GPIO(x)          (4 * (x))
> > +#define REG_GPIOx_DATA       BIT(0)
> > +#define REG_GPIOx_MODE_MASK  GENMASK(3, 1)
> > +#define REG_GPIOx_OUT        1
> > +#define REG_GPIOx_IN_IRQ_HI  2
> > +#define REG_GPIOx_IN_IRQ_LO  3
> > +#define REG_GPIOx_IN_IRQ_UP  4
> > +#define REG_GPIOx_IN_IRQ_DN  5
> > +#define REG_GPIOx_IN_IRQ_ANY 6
> > +#define REG_GPIOx_IN_IRQ_OFF 7
> > +#define REG_GPIOx_PERIPH     BIT(5)
> > +#define REG_GPIOx_CFG_DONE   BIT(9)
> > +#define REG_GPIOx_GRP_MASK   GENMASK(18, 16)
> > +#define REG_IRQ(g, x)        (0x800 + 0x40 * (g) + 4 * ((x) >> 5))
> 
> Can we update these defines with the correct definitions and names we
> figured out the other day and add the missing ones? We now know a bunch of
> these are wrong (e.g. CFG_DONE is INPUT_ENABLE, PERIPH should be two bits,
> we're missing pull-up control, drive strength, schmitt trigger and lock
> bits). Even if we don't implement all the features in the driver yet, we
> should have all the register bit defines for documentation purposes at
> least.

Done.

> 
> > +	if (!(prev & REG_GPIOx_CFG_DONE))
> > +		writel_relaxed(cfg & ~REG_GPIOx_CFG_DONE, ppin);
> > +	writel_relaxed(cfg, ppin);
> 
> We already determined this dance doesn't make any sense; if we want to
> change the pin config before enabling the input buffer (whether this serves
> any purpose at all is an open question) then that should be handled in the
> upper code responsible for enabling/disabling the input buffer, not in the
> core register wrappers.

Removed this.

[...]

> > +static void apple_gpio_gpio_set(struct gpio_chip *chip, unsigned int offset,
> > +				int value)
> > +{
> > +	struct apple_gpio_pinctrl *pctl = gpiochip_get_data(chip);
> > +
> > +	apple_gpio_set_reg(pctl, offset, REG_GPIOx_DATA,
> > +			   REG_GPIOx_CFG_DONE | (value & REG_GPIOx_DATA));
> > +}
> 
> `value ? REG_GPIOx_DATA : 0` please, otherwise this makes assumptions about
> value always being 1 and REG_GPIOx_DATA being the LSB.

I like that.

> > +static int apple_gpio_gpio_direction_input(struct gpio_chip *chip,
> > +					   unsigned int offset)
> > +{
> > +	struct apple_gpio_pinctrl *pctl = gpiochip_get_data(chip);
> > +
> > +	apple_gpio_set_reg(pctl, offset, REG_GPIOx_MODE_MASK | REG_GPIOx_DATA,
> > +			   FIELD_PREP(REG_GPIOx_MODE_MASK,
> > +				      REG_GPIOx_IN_IRQ_OFF) |
> > +				   REG_GPIOx_CFG_DONE);
> 
> Is hardcoding IRQ_OFF correct here? Shouldn't this be getting the intended
> IRQ state from somewhere, or is it always guaranteed that that gets set
> later?

As far as I can tell, the only way to setup an IRQ on a pin/gpio goes
via the apple_gpio_irq_startup function, which calls
`apple_gpio_gpio_direction_input` and then `apple_gpio_gpio_irq_unmask`
which sets the correct IRQ_ value.

> 
> > +static int apple_gpio_gpio_direction_output(struct gpio_chip *chip,
> > +					    unsigned int offset, int value)
> > +{
> > +	struct apple_gpio_pinctrl *pctl = gpiochip_get_data(chip);
> > +
> > +	apple_gpio_set_reg(pctl, offset, REG_GPIOx_PERIPH | REG_GPIOx_DATA,
> > +			   FIELD_PREP(REG_GPIOx_MODE_MASK, REG_GPIOx_OUT) |
> > +				   (value & REG_GPIOx_DATA) |
> > +				   REG_GPIOx_CFG_DONE);
> 
> I actually wonder if we should even bother turning on the input buffer for
> output pins, given we're shadowing the value anyway. Seems unnecessary and
> might save a few nanowatts.
> 
> Also, why is this clearing the peripheral (yet direction_input isn't)?

I've removed setting the input buffer here (CFG_DONE, or INPUT_ENABLE as
it is now named in v3).

That was just an oversight, I'm clearing it in direction_input too now.

[...]

> > +static unsigned int apple_gpio_gpio_irq_startup(struct irq_data *data)
> > +{
> > +	struct gpio_chip *chip = irq_data_get_irq_chip_data(data);
> > +	struct apple_gpio_pinctrl *pctl = gpiochip_get_data(chip);
> > +
> > +	apple_gpio_set_reg(pctl, data->hwirq, REG_GPIOx_GRP_MASK,
> > +			   FIELD_PREP(REG_GPIOx_GRP_MASK, 0));
> 
> I guess we're only using a single IRQ group right now?
Yep. The irq handler should already work with different groups. So to
support different groups we'd have to get the group number here somehow.

Thanks,
Joey