drivers: pinctrl: Microchip XEC PINCTRL glitch fix

Glitches were observed if a GPIO pin was configured by
ROM to a non-default state and then Zephyr PINCTRL
reconfigured the pin. The fix involves using the correct
PINCTRL YAML output enable and state flags. Reading the
current spin state and reflecting into new pin configuration
if the pin is output and the drive low/high properties are
not present. We also take advantage of GPIO hardware reflecing
the alternate output value in the parallel output bit before
enabling parallel output mode. Interpret boolean flags with
both enable and disable as do not touch if neither flag is
present. We give precedence to enable over disable if both
flags mistakenly appear. Note, PINCTRL always clears the
GPIO control input pad disable bit.

Signed-off-by: Manimaran A <[email protected]>

Author: Manimaran-A

boards/arm/mec172xevb_assy6906/mec172xevb_assy6906.dts

@@ -148,11 +148,13 @@
 
 &i2c00_scl_gpio004 {
 	drive-open-drain;
+	output-enable;
 	output-high;
 };
 
 &i2c00_sda_gpio003 {
 	drive-open-drain;
+	output-enable;
 	output-high;
 };
 
@@ -181,11 +183,13 @@
 
 &i2c01_scl_gpio131 {
 	drive-open-drain;
+	output-enable;
 	output-high;
 };
 
 &i2c01_sda_gpio130 {
 	drive-open-drain;
+	output-enable;
 	output-high;
 };
 
@@ -198,11 +202,13 @@
 
 &i2c07_scl_gpio013 {
 	drive-open-drain;
+	output-enable;
 	output-high;
 };
 
 &i2c07_sda_gpio012 {
 	drive-open-drain;
+	output-enable;
 	output-high;
 };
 

boards/arm/mec172xmodular_assy6930/mec172xmodular_assy6930.dts

@@ -158,11 +158,13 @@
 
 &i2c00_scl_gpio004 {
 	drive-open-drain;
+	output-enable;
 	output-high;
 };
 
 &i2c00_sda_gpio003 {
 	drive-open-drain;
+	output-enable;
 	output-high;
 };
 
@@ -175,11 +177,13 @@
 
 &i2c01_scl_gpio131 {
 	drive-open-drain;
+	output-enable;
 	output-high;
 };
 
 &i2c01_sda_gpio130 {
 	drive-open-drain;
+	output-enable;
 	output-high;
 };
 
@@ -192,11 +196,13 @@
 
 &i2c07_scl_gpio013 {
 	drive-open-drain;
+	output-enable;
 	output-high;
 };
 
 &i2c07_sda_gpio012 {
 	drive-open-drain;
+	output-enable;
 	output-high;
 };
 

drivers/pinctrl/pinctrl_mchp_xec.c

@@ -12,56 +12,88 @@
 #include <zephyr/drivers/pinctrl.h>
 #include <soc.h>
 
-/* Microchip XEC: each GPIO pin has two 32-bit control register.
+/*
+ * Microchip XEC: each GPIO pin has two 32-bit control register.
  * The first 32-bit register contains all pin features except
  * slew rate and driver strength in the second control register.
  * We compute the register index from the beginning of the GPIO
  * control address space which is the same range of the PINCTRL
- * parent node.
+ * parent node. A zero value in the PINCTRL pinmux field means
+ * do not touch.
  */
-
 static void config_drive_slew(struct gpio_regs * const regs, uint32_t idx, uint32_t conf)
 {
-	uint32_t slew = conf & (MCHP_XEC_OSPEEDR_MASK << MCHP_XEC_OSPEEDR_POS);
-	uint32_t drvstr = conf & (MCHP_XEC_ODRVSTR_MASK << MCHP_XEC_ODRVSTR_POS);
-	uint32_t val = 0;
-	uint32_t mask = 0;
-
-	if (slew != MCHP_XEC_OSPEEDR_NO_CHG) {
-		mask |= MCHP_GPIO_CTRL2_SLEW_MASK;
-		if (slew == MCHP_XEC_OSPEEDR_FAST) {
+	uint32_t slew = (conf >> MCHP_XEC_SLEW_RATE_POS) & MCHP_XEC_SLEW_RATE_MSK0;
+	uint32_t drvstr = (conf >> MCHP_XEC_DRV_STR_POS) & MCHP_XEC_DRV_STR_MSK0;
+	uint32_t msk = 0, val = 0;
+
+	if (slew) {
+		msk |= MCHP_GPIO_CTRL2_SLEW_MASK;
+		/* slow slew value is 0 */
+		if (slew == MCHP_XEC_SLEW_RATE_FAST0) {
 			val |= MCHP_GPIO_CTRL2_SLEW_FAST;
 		}
 	}
-	if (drvstr != MCHP_XEC_ODRVSTR_NO_CHG) {
-		mask |= MCHP_GPIO_CTRL2_DRV_STR_MASK;
-		val |= (drvstr << MCHP_GPIO_CTRL2_DRV_STR_POS);
+
+	if (drvstr) {
+		msk |= MCHP_GPIO_CTRL2_DRV_STR_MASK;
+		/* drive strength values are 0 based */
+		val |= ((drvstr - 1u) << MCHP_GPIO_CTRL2_DRV_STR_POS);
 	}
 
-	if (!mask) {
+	if (!msk) {
 		return;
 	}
 
-	regs->CTRL2[idx] = (regs->CTRL2[idx] & ~mask) | (val & mask);
+	regs->CTRL2[idx] = (regs->CTRL2[idx] & ~msk) | (val & msk);
 }
 
-/* Configure pin by writing GPIO Control and Control2 registers.
- * NOTE: Disable alternate output feature since the GPIO driver does.
- * While alternate output is enabled (default state of pin) HW does not
- * ignores writes to the parallel output bit for the pin. To set parallel
- * output value we must keep pin direction as input, set alternate output
- * disable, program pin value to parallel output bit, and then disable
- * alternate output mode.
+/*
+ * Internal pulls feature:
+ * None, weak pull-up, weak pull-down, or repeater mode (both pulls enabled).
+ * We do not touch this field unless one or more of the DT booleans are set.
+ * If the no-bias boolean is set then disable internal pulls.
+ * If pull up and/or down is set enable the respective pull or both for what
+ * MCHP calls repeater(keeper) mode.
+ */
+static uint32_t prog_pud(uint32_t pcr1, uint32_t conf)
+{
+	if (conf & BIT(MCHP_XEC_NO_PUD_POS)) {
+		pcr1 &= ~(MCHP_GPIO_CTRL_PUD_MASK);
+		pcr1 |= MCHP_GPIO_CTRL_PUD_NONE;
+		return pcr1;
+	}
+
+	if (conf & (BIT(MCHP_XEC_PU_POS) | BIT(MCHP_XEC_PD_POS))) {
+		pcr1 &= ~(MCHP_GPIO_CTRL_PUD_MASK);
+		if (conf & BIT(MCHP_XEC_PU_POS)) {
+			pcr1 |= MCHP_GPIO_CTRL_PUD_PU;
+		}
+		if (conf & BIT(MCHP_XEC_PD_POS)) {
+			pcr1 |= MCHP_GPIO_CTRL_PUD_PD;
+		}
+	}
+
+	return pcr1;
+}
+
+/*
+ * DT enable booleans take precedence over disable booleans.
+ * We initially clear alternate output disable allowing us to set output state
+ * in the control register. Hardware sets output state bit in both control and
+ * parallel output register bits. Alternate output disable only controls which
+ * register bit is writable by the EC. We also clear the input pad disable
+ * bit because we need the input pin state and we don't know if the requested
+ * alternate function is input or bi-directional.
+ * Note 1: hardware allows input and output to be simultaneously enabled.
+ * Note 2: hardware interrupt detection is only on the input path.
  */
 static int xec_config_pin(uint32_t portpin, uint32_t conf, uint32_t altf)
 {
 	struct gpio_regs * const regs = (struct gpio_regs * const)DT_INST_REG_ADDR(0);
 	uint32_t port = MCHP_XEC_PINMUX_PORT(portpin);
 	uint32_t pin = (uint32_t)MCHP_XEC_PINMUX_PIN(portpin);
-	uint32_t msk = MCHP_GPIO_CTRL_AOD_MASK;
-	uint32_t val = MCHP_GPIO_CTRL_AOD_DIS;
-	uint32_t idx = 0u;
-	uint32_t temp = 0u;
+	uint32_t idx = 0u, pcr1 = 0u;
 
 	if (port >= NUM_MCHP_GPIO_PORTS) {
 		return -EINVAL;
@@ -72,81 +104,84 @@ static int xec_config_pin(uint32_t portpin, uint32_t conf, uint32_t altf)
 
 	config_drive_slew(regs, idx, conf);
 
-	/* default input pad enabled, buffer type push-pull, no internal pulls,
-	 * and invert polarity normal.
-	 */
-	msk |= (BIT(MCHP_GPIO_CTRL_INPAD_DIS_POS) | MCHP_GPIO_CTRL_BUFT_MASK |
-			MCHP_GPIO_CTRL_PUD_MASK | MCHP_GPIO_CTRL_MUX_MASK
-			| BIT(MCHP_GPIO_CTRL_POL_POS));
+	/* Clear alternate output disable and input pad disable */
+	regs->CTRL[idx] &= ~(BIT(MCHP_GPIO_CTRL_AOD_POS) | BIT(MCHP_GPIO_CTRL_INPAD_DIS_POS));
+	pcr1 = regs->CTRL[idx]; /* current configuration including pin input state */
+	pcr1 = regs->CTRL[idx]; /* read multiple times to allow propagation from pad */
+	pcr1 = regs->CTRL[idx]; /* Is this necessary? */
 
-	if (conf & BIT(MCHP_XEC_PIN_LOW_POWER_POS)) {
-		msk |= MCHP_GPIO_CTRL_PWRG_MASK;
-		val |= MCHP_GPIO_CTRL_PWRG_OFF;
-	}
+	pcr1 = prog_pud(pcr1, conf);
 
-	temp = (conf & MCHP_XEC_PUPDR_MASK) >> MCHP_XEC_PUPDR_POS;
-	switch (temp) {
-	case MCHP_XEC_PULL_UP:
-		val |= MCHP_GPIO_CTRL_PUD_PU;
-		break;
-	case MCHP_XEC_PULL_DOWN:
-		val |= MCHP_GPIO_CTRL_PUD_PD;
-		break;
-	case MCHP_XEC_REPEATER:
-		val |= MCHP_GPIO_CTRL_PUD_RPT;
-		break;
-	default:
-		val |= MCHP_GPIO_CTRL_PUD_NONE;
-		break;
+	/* Touch output enable. We always enable input */
+	if (conf & BIT(MCHP_XEC_OUT_DIS_POS)) {
+		pcr1 &= ~(MCHP_GPIO_CTRL_DIR_OUTPUT);
+	}
+	if (conf & BIT(MCHP_XEC_OUT_EN_POS)) {
+		pcr1 |= MCHP_GPIO_CTRL_DIR_OUTPUT;
 	}
 
-	if ((conf >> MCHP_XEC_OTYPER_POS) & MCHP_XEC_OTYPER_MASK) {
-		val |= MCHP_GPIO_CTRL_BUFT_OPENDRAIN;
+	/* Touch output state? Bit can be set even if the direction is input only */
+	if (conf & BIT(MCHP_XEC_OUT_LO_POS)) {
+		pcr1 &= ~BIT(MCHP_GPIO_CTRL_OUTVAL_POS);
+	}
+	if (conf & BIT(MCHP_XEC_OUT_HI_POS)) {
+		pcr1 |= BIT(MCHP_GPIO_CTRL_OUTVAL_POS);
 	}
 
-	if (conf & MCHP_XEC_FUNC_INV_MSK) {
-		val |= BIT(MCHP_GPIO_CTRL_POL_POS);
+	/* Touch output buffer type? */
+	if (conf & BIT(MCHP_XEC_PUSH_PULL_POS)) {
+		pcr1 &= ~(MCHP_GPIO_CTRL_BUFT_OPENDRAIN);
+	}
+	if (conf & BIT(MCHP_XEC_OPEN_DRAIN_POS)) {
+		pcr1 |= MCHP_GPIO_CTRL_BUFT_OPENDRAIN;
 	}
 
-	regs->CTRL[idx] = (regs->CTRL[idx] & ~msk) | val;
+	/* Always touch power gate */
+	pcr1 &= ~MCHP_GPIO_CTRL_PWRG_MASK;
+	if (conf & BIT(MCHP_XEC_PIN_LOW_POWER_POS)) {
+		pcr1 |= MCHP_GPIO_CTRL_PWRG_OFF;
+	} else {
+		pcr1 |= MCHP_GPIO_CTRL_PWRG_VTR_IO;
+	}
 
-	temp = (conf >> MCHP_XEC_OVAL_POS) & MCHP_XEC_OVAL_MASK;
-	if (temp) {
-		if (temp == MCHP_XEC_OVAL_DRV_HIGH) {
-			regs->PAROUT[port] |= BIT(pin);
-		} else {
-			regs->PAROUT[port] &= ~BIT(pin);
-		}
+	/* Always touch MUX (alternate function) */
+	pcr1 &= ~MCHP_GPIO_CTRL_MUX_MASK;
+	pcr1 |= (uint32_t)((altf & MCHP_GPIO_CTRL_MUX_MASK0) << MCHP_GPIO_CTRL_MUX_POS);
 
-		regs->CTRL[idx] |= MCHP_GPIO_CTRL_DIR_OUTPUT;
+	/* Always touch invert of alternate function. Need another bit to avoid touching */
+	if (conf & BIT(MCHP_XEC_FUNC_INV_POS)) {
+		pcr1 |= BIT(MCHP_GPIO_CTRL_POL_POS);
+	} else {
+		pcr1 &= ~BIT(MCHP_GPIO_CTRL_POL_POS);
 	}
 
-	val = (uint32_t)((altf & MCHP_GPIO_CTRL_MUX_MASK0) << MCHP_GPIO_CTRL_MUX_POS);
-	regs->CTRL[idx] |= val;
+	/* output state set in control & parallel regs */
+	regs->CTRL[idx] = pcr1;
+	/* make output state in control read-only in control and read-write in parallel reg */
+	regs->CTRL[idx] = pcr1 | BIT(MCHP_GPIO_CTRL_AOD_POS);
 
 	return 0;
 }
 
 int pinctrl_configure_pins(const pinctrl_soc_pin_t *pins, uint8_t pin_cnt,
 			   uintptr_t reg)
 {
-	uint32_t portpin, mux, cfg, func;
+	uint32_t portpin, pinmux, func;
 	int ret;
 
 	ARG_UNUSED(reg);
 
 	for (uint8_t i = 0U; i < pin_cnt; i++) {
-		mux = pins[i].pinmux;
+		pinmux = pins[i];
 
-		func = MCHP_XEC_PINMUX_FUNC(mux);
+		func = MCHP_XEC_PINMUX_FUNC(pinmux);
 		if (func >= MCHP_AFMAX) {
 			return -EINVAL;
 		}
 
-		cfg = pins[i].pincfg;
-		portpin = MEC_XEC_PINMUX_PORT_PIN(mux);
+		portpin = MEC_XEC_PINMUX_PORT_PIN(pinmux);
 
-		ret = xec_config_pin(portpin, cfg, func);
+		ret = xec_config_pin(portpin, pinmux, func);
 		if (ret < 0) {
 			return ret;
 		}

dts/bindings/pinctrl/microchip,xec-pinctrl.yaml

@@ -83,6 +83,8 @@ child-binding:
         - drive-push-pull
         - drive-open-drain
         - low-power-enable
+        - output-disable
+        - output-enable
         - output-high
         - output-low
 
@@ -94,8 +96,9 @@ child-binding:
 
     slew-rate:
       type: string
-      default: "low-speed"
+      default: "no-change"
       enum:
+        - "no-change"
         - "low-speed"
         - "high-speed"
       description: |
@@ -106,8 +109,9 @@ child-binding:
 
     drive-strength:
       type: string
-      default: "1x"
+      default: "no-change"
       enum:
+        - "no-change"
         - "1x"
         - "2x"
         - "4x"