[coreboot-gerrit] New patch to review for coreboot: 2f517cc tegra: Reorder GPIO register accesses to avoid glitching

[Isaac Christensen]

Isaac Christensen ([e-mail]) just uploaded a new patch set to gerrit, which you can find at http://review.coreboot.org/6868

-gerrit

commit 2f517ccb7b2cd51b1bab0811ae97ab1a4306e183
Author: Julius Werner 
Date:   Thu Oct 10 19:20:06 2013 -0700

tegra: Reorder GPIO register accesses to avoid glitching

This patch changes the order of GPIO and pinmux register accesses when
    setting GPIOs to make the operations as atomic as possible, and avoid
    output-to-output as well as special-function-to-input glitches under
    some circumstances. It also sets the TRISTATE pinmux for input GPIOs,
    since according to my understanding of the pinmux circuit this is the
    right thing to do.

Also changed all uintXX_t types to uXX, since I think we agreed on
    preferring that for all new code.

Old-Change-Id: I2e0156fbaa85e46e460b4494765a06e19f50d4e4
    Signed-off-by: Julius Werner 
    Reviewed-on: https://chromium-review.googlesource.com/172730
    Reviewed-by: Ronald Minnich 
    Reviewed-by: David Hendricks 
    (cherry picked from commit 61bedbf0f839e19b284d21af2ad10f2ff15e17d5)

tegra: Turn GPIO wrappers into macros to make them easier to write

The GPIO wrappers are already pretty nice, but you still end up writing
    everything twice since you need to specify the pinmux register:

gpio_output(GPIO_Q2, PINMUX_GPIO_Q2, 1);

Calculating the pinmux index from the GPIO isn't easy, but luckily the
    enums use a similar naming scheme. This patch changes the wrapper
    functions to macros that generate the corresponding pinmux name
    automatically.

Old-Change-Id: Id23855c5aec5f87b4201f22bac32503e72f83392
    Signed-off-by: Julius Werner 
    Reviewed-on: https://chromium-review.googlesource.com/172731
    Reviewed-by: Ronald Minnich 
    (cherry picked from commit 94550fdfa5a8005d2e6a313041de212ab7ac470c)

tegra: Change GPIO functions to allow variable arguments

This patch changes the GPIO API once again, this time back to functions.
    It introduces a new opaque type gpio_t that holds both the GPIO number
    and the corresponding pinmux number and can be constructed with a macro
    (e.g. 'gpio_output(GPIO(Q2), 1);' ). This makes the GPIO functions
    convenient to use, but also allows passing GPIOs around in variables and
    function parameters.

Old-Change-Id: I519c32b872b0c912046a3aaa070ef4e9699856ee
    Signed-off-by: Julius Werner 
    Reviewed-on: https://chromium-review.googlesource.com/172916
    Reviewed-by: Ronald Minnich 
    (cherry picked from commit e95ccd984f718a04b6067ff6ad5049a2cd74466d)

Squashed three gpio related commits for tegra.

Change-Id: Ibd3f2134d8473314ebe898ee21a8bf3630a9c8ad
    Signed-off-by: Isaac Christensen 
---
 src/soc/nvidia/tegra/gpio.c    | 210 ++++++++++++++++++++---------------------
 src/soc/nvidia/tegra/gpio.h    |  62 ++++++++----
 src/soc/nvidia/tegra124/gpio.h |   7 +-
 3 files changed, 147 insertions(+), 132 deletions(-)

diff --git a/src/soc/nvidia/tegra/gpio.c b/src/soc/nvidia/tegra/gpio.c
index d4b5bdd..0615320 100644
--- a/src/soc/nvidia/tegra/gpio.c
+++ b/src/soc/nvidia/tegra/gpio.c
@@ -26,41 +26,25 @@
 #include "gpio.h"
 #include "pinmux.h"

-static void gpio_input_common(int gpio_index, int pinmux_index,
-			      uint32_t pconfig)
+void __gpio_input(gpio_t gpio, u32 pull)
 {
-	pconfig |= PINMUX_INPUT_ENABLE;
-	gpio_set_int_enable(gpio_index, 0);
-	gpio_set_mode(gpio_index, GPIO_MODE_GPIO);
-	gpio_set_out_enable(gpio_index, 0);
-	pinmux_set_config(pinmux_index, pconfig);
-}
-
-void gpio_input(int gpio_index, int pinmux_index)
-{
-	gpio_input_common(gpio_index, pinmux_index, PINMUX_PULL_NONE);
-}
+	u32 pinmux_config = PINMUX_INPUT_ENABLE | PINMUX_TRISTATE | pull;

-void gpio_input_pullup(int gpio_index, int pinmux_index)
-{
-	gpio_input_common(gpio_index, pinmux_index, PINMUX_PULL_UP);
-}
-
-void gpio_input_pulldown(int gpio_index, int pinmux_index)
-{
-	gpio_input_common(gpio_index, pinmux_index, PINMUX_PULL_DOWN);
+	gpio_set_int_enable(gpio, 0);
+	gpio_set_out_enable(gpio, 0);
+	gpio_set_mode(gpio, GPIO_MODE_GPIO);
+	pinmux_set_config(gpio >> GPIO_PINMUX_SHIFT, pinmux_config);
 }

-void gpio_output(int gpio_index, int pinmux_index, int value)
+void gpio_output(gpio_t gpio, int value)
 {
-	uint32_t pconfig = PINMUX_PULL_NONE;
+	/* TODO: Set OPEN_DRAIN based on what pin it is? */

-	pinmux_set_config(pinmux_index, pconfig | PINMUX_TRISTATE);
-	gpio_set_int_enable(gpio_index, 0);
-	gpio_set_mode(gpio_index, GPIO_MODE_GPIO);
-	gpio_set_out_enable(gpio_index, 1);
-	gpio_set_out_value(gpio_index, value);
-	pinmux_set_config(pinmux_index, pconfig);
+	gpio_set_int_enable(gpio, 0);
+	gpio_set_out_value(gpio, value);
+	gpio_set_out_enable(gpio, 1);
+	gpio_set_mode(gpio, GPIO_MODE_GPIO);
+	pinmux_set_config(gpio >> GPIO_PINMUX_SHIFT, PINMUX_PULL_NONE);
 }

enum {
@@ -74,167 +58,173 @@ enum {

struct gpio_bank {
    // Values
-	uint32_t config[GPIO_PORTS_PER_BANK];
-	uint32_t out_enable[GPIO_PORTS_PER_BANK];
-	uint32_t out_value[GPIO_PORTS_PER_BANK];
-	uint32_t in_value[GPIO_PORTS_PER_BANK];
-	uint32_t int_status[GPIO_PORTS_PER_BANK];
-	uint32_t int_enable[GPIO_PORTS_PER_BANK];
-	uint32_t int_level[GPIO_PORTS_PER_BANK];
-	uint32_t int_clear[GPIO_PORTS_PER_BANK];
+	u32 config[GPIO_PORTS_PER_BANK];
+	u32 out_enable[GPIO_PORTS_PER_BANK];
+	u32 out_value[GPIO_PORTS_PER_BANK];
+	u32 in_value[GPIO_PORTS_PER_BANK];
+	u32 int_status[GPIO_PORTS_PER_BANK];
+	u32 int_enable[GPIO_PORTS_PER_BANK];
+	u32 int_level[GPIO_PORTS_PER_BANK];
+	u32 int_clear[GPIO_PORTS_PER_BANK];

// Masks
-	uint32_t config_mask[GPIO_PORTS_PER_BANK];
-	uint32_t out_enable_mask[GPIO_PORTS_PER_BANK];
-	uint32_t out_value_mask[GPIO_PORTS_PER_BANK];
-	uint32_t in_value_mask[GPIO_PORTS_PER_BANK];
-	uint32_t int_status_mask[GPIO_PORTS_PER_BANK];
-	uint32_t int_enable_mask[GPIO_PORTS_PER_BANK];
-	uint32_t int_level_mask[GPIO_PORTS_PER_BANK];
-	uint32_t int_clear_mask[GPIO_PORTS_PER_BANK];
+	u32 config_mask[GPIO_PORTS_PER_BANK];
+	u32 out_enable_mask[GPIO_PORTS_PER_BANK];
+	u32 out_value_mask[GPIO_PORTS_PER_BANK];
+	u32 in_value_mask[GPIO_PORTS_PER_BANK];
+	u32 int_status_mask[GPIO_PORTS_PER_BANK];
+	u32 int_enable_mask[GPIO_PORTS_PER_BANK];
+	u32 int_level_mask[GPIO_PORTS_PER_BANK];
+	u32 int_clear_mask[GPIO_PORTS_PER_BANK];
 };

static const struct gpio_bank *gpio_banks = (void *)TEGRA_GPIO_BASE;

-static uint32_t gpio_read_port(int index, size_t offset)
+static u32 gpio_read_port(int index, size_t offset)
 {
    int bank = index / GPIO_GPIOS_PER_BANK;
    int port = (index - bank * GPIO_GPIOS_PER_BANK) / GPIO_GPIOS_PER_PORT;

-	return read32((uint8_t *)&gpio_banks[bank] + offset +
-		      port * sizeof(uint32_t));
+	return read32((u8 *)&gpio_banks[bank] + offset +
+		      port * sizeof(u32));
 }

-static void gpio_write_port(int index, size_t offset,
-			    uint32_t mask, uint32_t value)
+static void gpio_write_port(int index, size_t offset, u32 mask, u32 value)
 {
    int bank = index / GPIO_GPIOS_PER_BANK;
    int port = (index - bank * GPIO_GPIOS_PER_BANK) / GPIO_GPIOS_PER_PORT;

-	uint32_t reg = read32((uint8_t *)&gpio_banks[bank] + offset +
-			      port * sizeof(uint32_t));
-	uint32_t new_reg = (reg & ~mask) | (value & mask);
+	u32 reg = read32((u8 *)&gpio_banks[bank] + offset +
+			      port * sizeof(u32));
+	u32 new_reg = (reg & ~mask) | (value & mask);

if (new_reg != reg) {
-		write32(new_reg, (uint8_t *)&gpio_banks[bank] + offset +
-			port * sizeof(uint32_t));
+		write32(new_reg, (u8 *)&gpio_banks[bank] + offset +
+			port * sizeof(u32));
    }
 }

-void gpio_set_mode(int gpio_index, enum gpio_mode mode)
+void gpio_set_mode(gpio_t gpio, enum gpio_mode mode)
 {
-	int bit = gpio_index % GPIO_GPIOS_PER_PORT;
-	gpio_write_port(gpio_index, offsetof(struct gpio_bank, config),
+	int bit = gpio % GPIO_GPIOS_PER_PORT;
+	gpio_write_port(gpio & ((1 << GPIO_PINMUX_SHIFT) - 1),
+			offsetof(struct gpio_bank, config),
    		1 << bit, mode ? (1 << bit) : 0);
 }

-int gpio_get_mode(int gpio_index)
+int gpio_get_mode(gpio_t gpio)
 {
-	int bit = gpio_index % GPIO_GPIOS_PER_PORT;
-	uint32_t port = gpio_read_port(gpio_index,
-				       offsetof(struct gpio_bank, config));
+	int bit = gpio % GPIO_GPIOS_PER_PORT;
+	u32 port = gpio_read_port(gpio & ((1 << GPIO_PINMUX_SHIFT) - 1),
+				  offsetof(struct gpio_bank, config));
    return (port & (1 << bit)) != 0;
 }

-void gpio_set_lock(int gpio_index)
+void gpio_set_lock(gpio_t gpio)
 {
-	int bit = gpio_index % GPIO_GPIOS_PER_PORT + GPIO_GPIOS_PER_PORT;
-	gpio_write_port(gpio_index, offsetof(struct gpio_bank, config),
+	int bit = gpio % GPIO_GPIOS_PER_PORT + GPIO_GPIOS_PER_PORT;
+	gpio_write_port(gpio & ((1 << GPIO_PINMUX_SHIFT) - 1),
+			offsetof(struct gpio_bank, config),
    		1 << bit, 1 << bit);
 }

-int gpio_get_lock(int gpio_index)
+int gpio_get_lock(gpio_t gpio)
 {
-	int bit = gpio_index % GPIO_GPIOS_PER_PORT + GPIO_GPIOS_PER_PORT;
-	uint32_t port = gpio_read_port(gpio_index,
-				       offsetof(struct gpio_bank, config));
+	int bit = gpio % GPIO_GPIOS_PER_PORT + GPIO_GPIOS_PER_PORT;
+	u32 port = gpio_read_port(gpio & ((1 << GPIO_PINMUX_SHIFT) - 1),
+				  offsetof(struct gpio_bank, config));
    return (port & (1 << bit)) != 0;
 }

-void gpio_set_out_enable(int gpio_index, int enable)
+void gpio_set_out_enable(gpio_t gpio, int enable)
 {
-	int bit = gpio_index % GPIO_GPIOS_PER_PORT;
-	gpio_write_port(gpio_index, offsetof(struct gpio_bank, out_enable),
+	int bit = gpio % GPIO_GPIOS_PER_PORT;
+	gpio_write_port(gpio & ((1 << GPIO_PINMUX_SHIFT) - 1),
+			offsetof(struct gpio_bank, out_enable),
    		1 << bit, enable ? (1 << bit) : 0);
 }

-int gpio_get_out_enable(int gpio_index)
+int gpio_get_out_enable(gpio_t gpio)
 {
-	int bit = gpio_index % GPIO_GPIOS_PER_PORT;
-	uint32_t port = gpio_read_port(gpio_index,
-				       offsetof(struct gpio_bank, out_enable));
+	int bit = gpio % GPIO_GPIOS_PER_PORT;
+	u32 port = gpio_read_port(gpio & ((1 << GPIO_PINMUX_SHIFT) - 1),
+				  offsetof(struct gpio_bank, out_enable));
    return (port & (1 << bit)) != 0;
 }

-void gpio_set_out_value(int gpio_index, int value)
+void gpio_set_out_value(gpio_t gpio, int value)
 {
-	int bit = gpio_index % GPIO_GPIOS_PER_PORT;
-	gpio_write_port(gpio_index, offsetof(struct gpio_bank, out_value),
+	int bit = gpio % GPIO_GPIOS_PER_PORT;
+	gpio_write_port(gpio & ((1 << GPIO_PINMUX_SHIFT) - 1),
+			offsetof(struct gpio_bank, out_value),
    		1 << bit, value ? (1 << bit) : 0);
 }

-int gpio_get_out_value(int gpio_index)
+int gpio_get_out_value(gpio_t gpio)
 {
-	int bit = gpio_index % GPIO_GPIOS_PER_PORT;
-	uint32_t port = gpio_read_port(gpio_index,
-				       offsetof(struct gpio_bank, out_value));
+	int bit = gpio % GPIO_GPIOS_PER_PORT;
+	u32 port = gpio_read_port(gpio & ((1 << GPIO_PINMUX_SHIFT) - 1),
+				  offsetof(struct gpio_bank, out_value));
    return (port & (1 << bit)) != 0;
 }

-int gpio_get_in_value(int gpio_index)
+int gpio_get_in_value(gpio_t gpio)
 {
-	int bit = gpio_index % GPIO_GPIOS_PER_PORT;
-	uint32_t port = gpio_read_port(gpio_index,
-				       offsetof(struct gpio_bank, in_value));
+	int bit = gpio % GPIO_GPIOS_PER_PORT;
+	u32 port = gpio_read_port(gpio & ((1 << GPIO_PINMUX_SHIFT) - 1),
+				  offsetof(struct gpio_bank, in_value));
    return (port & (1 << bit)) != 0;
 }

-int gpio_get_int_status(int gpio_index)
+int gpio_get_int_status(gpio_t gpio)
 {
-	int bit = gpio_index % GPIO_GPIOS_PER_PORT;
-	uint32_t port = gpio_read_port(gpio_index,
-				       offsetof(struct gpio_bank, int_status));
+	int bit = gpio % GPIO_GPIOS_PER_PORT;
+	u32 port = gpio_read_port(gpio & ((1 << GPIO_PINMUX_SHIFT) - 1),
+				  offsetof(struct gpio_bank, int_status));
    return (port & (1 << bit)) != 0;
 }

-void gpio_set_int_enable(int gpio_index, int enable)
+void gpio_set_int_enable(gpio_t gpio, int enable)
 {
-	int bit = gpio_index % GPIO_GPIOS_PER_PORT;
-	gpio_write_port(gpio_index, offsetof(struct gpio_bank, int_enable),
+	int bit = gpio % GPIO_GPIOS_PER_PORT;
+	gpio_write_port(gpio & ((1 << GPIO_PINMUX_SHIFT) - 1),
+			offsetof(struct gpio_bank, int_enable),
    		1 << bit, enable ? (1 << bit) : 0);
 }

-int gpio_get_int_enable(int gpio_index)
+int gpio_get_int_enable(gpio_t gpio)
 {
-	int bit = gpio_index % GPIO_GPIOS_PER_PORT;
-	uint32_t port = gpio_read_port(gpio_index,
-				       offsetof(struct gpio_bank, int_enable));
+	int bit = gpio % GPIO_GPIOS_PER_PORT;
+	u32 port = gpio_read_port(gpio & ((1 << GPIO_PINMUX_SHIFT) - 1),
+				  offsetof(struct gpio_bank, int_enable));
    return (port & (1 << bit)) != 0;
 }

-void gpio_set_int_level(int gpio_index, int high_rise, int edge, int delta)
+void gpio_set_int_level(gpio_t gpio, int high_rise, int edge, int delta)
 {
-	int bit = gpio_index % GPIO_GPIOS_PER_PORT;
-	uint32_t value = (high_rise ? (0x000001 << bit) : 0) |
+	int bit = gpio % GPIO_GPIOS_PER_PORT;
+	u32 value = (high_rise ? (0x000001 << bit) : 0) |
    		 (edge ? (0x000100 << bit) : 0) |
-			 (delta ? (0x010000 << bit) : 0);
-	gpio_write_port(gpio_index, offsetof(struct gpio_bank, config),
+			(delta ? (0x010000 << bit) : 0);
+	gpio_write_port(gpio & ((1 << GPIO_PINMUX_SHIFT) - 1),
+			offsetof(struct gpio_bank, config),
    		0x010101 << bit, value);
 }

-void gpio_get_int_level(int gpio_index, int *high_rise, int *edge, int *delta)
+void gpio_get_int_level(gpio_t gpio, int *high_rise, int *edge, int *delta)
 {
-	int bit = gpio_index % GPIO_GPIOS_PER_PORT;
-	uint32_t port = gpio_read_port(gpio_index,
-				       offsetof(struct gpio_bank, int_level));
+	int bit = gpio % GPIO_GPIOS_PER_PORT;
+	u32 port = gpio_read_port(gpio & ((1 << GPIO_PINMUX_SHIFT) - 1),
+				  offsetof(struct gpio_bank, int_level));
    *high_rise = ((port & (0x000001 << bit)) != 0);
    *edge = ((port & (0x000100 << bit)) != 0);
    *delta = ((port & (0x010000 << bit)) != 0);
 }

-void gpio_set_int_clear(int gpio_index)
+void gpio_set_int_clear(gpio_t gpio)
 {
-	int bit = gpio_index % GPIO_GPIOS_PER_PORT;
-	gpio_write_port(gpio_index, offsetof(struct gpio_bank, int_clear),
+	int bit = gpio % GPIO_GPIOS_PER_PORT;
+	gpio_write_port(gpio & ((1 << GPIO_PINMUX_SHIFT) - 1),
+			offsetof(struct gpio_bank, int_clear),
    		1 << bit, 1 << bit);
 }
diff --git a/src/soc/nvidia/tegra/gpio.h b/src/soc/nvidia/tegra/gpio.h
index b62dc90..546ea05 100644
--- a/src/soc/nvidia/tegra/gpio.h
+++ b/src/soc/nvidia/tegra/gpio.h
@@ -22,12 +22,34 @@

#include <stdint.h>

-/* Higher level functions for common GPIO configurations. */
+#include "pinmux.h"

-void gpio_input(int gpio_index, int pinmux_index);
-void gpio_input_pullup(int gpio_index, int pinmux_index);
-void gpio_input_pulldown(int gpio_index, int pinmux_index);
-void gpio_output(int gpio_index, int pinmux_index, int value);
+/* Wrapper type for GPIOs. Always use GPIO() macro to generate. */
+typedef u32 gpio_t;
+
+#define GPIO_PINMUX_SHIFT 16
+#define GPIO(name) ((gpio_t)(GPIO_##name##_INDEX | \
+			     (PINMUX_GPIO_##name << GPIO_PINMUX_SHIFT)))
+
+/* Higher level function wrappers for common GPIO configurations. */
+
+void gpio_output(gpio_t gpio, int value);
+void __gpio_input(gpio_t gpio, u32 pull);
+
+static inline void gpio_input(gpio_t gpio)
+{
+	__gpio_input(gpio, PINMUX_PULL_NONE);
+}
+
+static inline void gpio_input_pulldown(gpio_t gpio)
+{
+	__gpio_input(gpio, PINMUX_PULL_DOWN);
+}
+
+static inline void gpio_input_pullup(gpio_t gpio)
+{
+	__gpio_input(gpio, PINMUX_PULL_UP);
+}

/* Functions to modify specific GPIO control values. */

@@ -35,29 +57,29 @@ enum gpio_mode {
    GPIO_MODE_SPIO = 0,
    GPIO_MODE_GPIO = 1
 };
-void gpio_set_mode(int gpio_index, enum gpio_mode);
-int gpio_get_mode(int gpio_index);
+void gpio_set_mode(gpio_t gpio, enum gpio_mode);
+int gpio_get_mode(gpio_t gpio);

// Lock a GPIO with extreme caution since they can't be unlocked.
-void gpio_set_lock(int gpio_index);
-int gpio_get_lock(int gpio_index);
+void gpio_set_lock(gpio_t gpio);
+int gpio_get_lock(gpio_t gpio);

-void gpio_set_out_enable(int gpio_index, int enable);
-int gpio_get_out_enable(int gpio_index);
+void gpio_set_out_enable(gpio_t gpio, int enable);
+int gpio_get_out_enable(gpio_t gpio);

-void gpio_set_out_value(int gpio_index, int value);
-int gpio_get_out_value(int gpio_index);
+void gpio_set_out_value(gpio_t gpio, int value);
+int gpio_get_out_value(gpio_t gpio);

-int gpio_get_in_value(int gpio_index);
+int gpio_get_in_value(gpio_t gpio);

-int gpio_get_int_status(int gpio_index);
+int gpio_get_int_status(gpio_t gpio);

-void gpio_set_int_enable(int gpio_index, int enable);
-int gpio_get_int_enable(int gpio_index);
+void gpio_set_int_enable(gpio_t gpio, int enable);
+int gpio_get_int_enable(gpio_t gpio);

-void gpio_set_int_level(int gpio_index, int high_rise, int edge, int delta);
-void gpio_get_int_level(int gpio_index, int *high_rise, int *edge, int *delta);
+void gpio_set_int_level(gpio_t gpio, int high_rise, int edge, int delta);
+void gpio_get_int_level(gpio_t gpio, int *high_rise, int *edge, int *delta);

-void gpio_set_int_clear(int gpio_index);
+void gpio_set_int_clear(gpio_t gpio);

#endif	/* __SOC_NVIDIA_TEGRA_GPIO_H__ */
diff --git a/src/soc/nvidia/tegra124/gpio.h b/src/soc/nvidia/tegra124/gpio.h
index 83d727d..f7d1c30 100644
--- a/src/soc/nvidia/tegra124/gpio.h
+++ b/src/soc/nvidia/tegra124/gpio.h
@@ -23,11 +23,14 @@
 #include <soc/nvidia/tegra/gpio.h>
 #include <stdint.h>

+#include "pinmux.h"	/* for pinmux constants in GPIO macro */
+
 /* GPIO index constants. */

#define GPIO_PORT_CONSTANTS(port) \
-	GPIO_##port##0, GPIO_##port##1, GPIO_##port##2, GPIO_##port##3, \
-	GPIO_##port##4, GPIO_##port##5, GPIO_##port##6, GPIO_##port##7
+	GPIO_##port##0_INDEX, GPIO_##port##1_INDEX, GPIO_##port##2_INDEX, \
+	GPIO_##port##3_INDEX, GPIO_##port##4_INDEX, GPIO_##port##5_INDEX, \
+	GPIO_##port##6_INDEX, GPIO_##port##7_INDEX

enum {
    GPIO_PORT_CONSTANTS(A),