[PATCH 0/5] pinctrl: cherryview: fixes and enhancements

[PATCH 0/5] pinctrl: cherryview: fixes and enhancements

[Dan O'Donovan]

This series includes a number of fixes and enhancements for the
Cherryview pinctrl/gpio driver, developed during integration on the
UP Board (based on the Intel X5-Z8350 "Cherry Trail" Atom SoC).

Of particular note is a workaround for a documented silicon bug which
causes data corruption of concurrent accesses to the GPIO controller
registers on this SoC.

Other patches include clean-up of checkpatch warnings, implementation
of additional pin config functions and options, and a fix for a loss
of pin config register state that can occur when switching in and out
of GPIO pin mode.

Dan O'Donovan (5):
  pinctrl: cherryview: convert bare unsigned to unsigned int
  pinctrl: cherryview: add option to set open-drain pin config
  pinctrl: cherryview: add handlers for pin_config_group_get/set
  pinctrl: cherryview: prevent concurrent access to GPIO controllers
  pinctrl: cherryview: restore padctrl1 reg when gpio is disabled

 drivers/pinctrl/intel/pinctrl-cherryview.c | 296 +++++++++++++++++++----------
 1 file changed, 196 insertions(+), 100 deletions(-)

-- 
2.1.4

[PATCH 1/5] pinctrl: cherryview: convert bare unsigned to unsigned int

[Dan O'Donovan]

Addresses checkpatch warnings such as the following for this driver:

 WARNING: Prefer 'unsigned int' to bare use of 'unsigned'

Signed-off-by: Dan O'Donovan <dan@emutex.com>
---
 drivers/pinctrl/intel/pinctrl-cherryview.c | 130 +++++++++++++++--------------
 1 file changed, 66 insertions(+), 64 deletions(-)

diff --git a/drivers/pinctrl/intel/pinctrl-cherryview.c b/drivers/pinctrl/intel/pinctrl-cherryview.c
index ac4f564..c79f4cc 100644
--- a/drivers/pinctrl/intel/pinctrl-cherryview.c
+++ b/drivers/pinctrl/intel/pinctrl-cherryview.c
@@ -75,7 +75,7 @@
  * @invert_oe: Invert OE for this pin
  */
 struct chv_alternate_function {
-	unsigned pin;
+	unsigned int pin;
 	u8 mode;
 	bool invert_oe;
 };
@@ -92,7 +92,7 @@ struct chv_alternate_function {
  */
 struct chv_pingroup {
 	const char *name;
-	const unsigned *pins;
+	const unsigned int *pins;
 	size_t npins;
 	struct chv_alternate_function altfunc;
 	const struct chv_alternate_function *overrides;
@@ -117,8 +117,8 @@ struct chv_function {
  * @npins: Number of pins in this range
  */
 struct chv_gpio_pinrange {
-	unsigned base;
-	unsigned npins;
+	unsigned int base;
+	unsigned int npins;
 };
 
 /**
@@ -175,7 +175,7 @@ struct chv_pinctrl {
 	struct gpio_chip chip;
 	void __iomem *regs;
 	raw_spinlock_t lock;
-	unsigned intr_lines[16];
+	unsigned int intr_lines[16];
 	const struct chv_community *community;
 	u32 saved_intmask;
 	struct chv_pin_context *saved_pin_context;
@@ -286,24 +286,26 @@ static const struct pinctrl_pin_desc southwest_pins[] = {
 	PINCTRL_PIN(97, "GP_SSP_2_TXD"),
 };
 
-static const unsigned southwest_fspi_pins[] = { 0, 1, 2, 3, 4, 5, 6, 7 };
-static const unsigned southwest_uart0_pins[] = { 16, 20 };
-static const unsigned southwest_uart1_pins[] = { 15, 16, 18, 20 };
-static const unsigned southwest_uart2_pins[] = { 17, 19, 21, 22 };
-static const unsigned southwest_i2c0_pins[] = { 61, 65 };
-static const unsigned southwest_hda_pins[] = { 30, 31, 32, 33, 34, 35, 36, 37 };
-static const unsigned southwest_lpe_pins[] = {
+static const unsigned int southwest_fspi_pins[] = { 0, 1, 2, 3, 4, 5, 6, 7 };
+static const unsigned int southwest_uart0_pins[] = { 16, 20 };
+static const unsigned int southwest_uart1_pins[] = { 15, 16, 18, 20 };
+static const unsigned int southwest_uart2_pins[] = { 17, 19, 21, 22 };
+static const unsigned int southwest_i2c0_pins[] = { 61, 65 };
+static const unsigned int southwest_hda_pins[] = {
+	30, 31, 32, 33, 34, 35, 36, 37,
+};
+static const unsigned int southwest_lpe_pins[] = {
 	30, 31, 32, 33, 34, 35, 36, 37, 92, 94, 96, 97,
 };
-static const unsigned southwest_i2c1_pins[] = { 60, 63 };
-static const unsigned southwest_i2c2_pins[] = { 62, 66 };
-static const unsigned southwest_i2c3_pins[] = { 64, 67 };
-static const unsigned southwest_i2c4_pins[] = { 46, 50 };
-static const unsigned southwest_i2c5_pins[] = { 45, 48 };
-static const unsigned southwest_i2c6_pins[] = { 47, 51 };
-static const unsigned southwest_i2c_nfc_pins[] = { 49, 52 };
-static const unsigned southwest_smbus_pins[] = { 79, 81, 82 };
-static const unsigned southwest_spi3_pins[] = { 76, 79, 80, 81, 82 };
+static const unsigned int southwest_i2c1_pins[] = { 60, 63 };
+static const unsigned int southwest_i2c2_pins[] = { 62, 66 };
+static const unsigned int southwest_i2c3_pins[] = { 64, 67 };
+static const unsigned int southwest_i2c4_pins[] = { 46, 50 };
+static const unsigned int southwest_i2c5_pins[] = { 45, 48 };
+static const unsigned int southwest_i2c6_pins[] = { 47, 51 };
+static const unsigned int southwest_i2c_nfc_pins[] = { 49, 52 };
+static const unsigned int southwest_smbus_pins[] = { 79, 81, 82 };
+static const unsigned int southwest_spi3_pins[] = { 76, 79, 80, 81, 82 };
 
 /* LPE I2S TXD pins need to have invert_oe set */
 static const struct chv_alternate_function southwest_lpe_altfuncs[] = {
@@ -588,17 +590,17 @@ static const struct pinctrl_pin_desc southeast_pins[] = {
 	PINCTRL_PIN(85, "SDMMC3_1P8_EN"),
 };
 
-static const unsigned southeast_pwm0_pins[] = { 5 };
-static const unsigned southeast_pwm1_pins[] = { 1 };
-static const unsigned southeast_sdmmc1_pins[] = {
+static const unsigned int southeast_pwm0_pins[] = { 5 };
+static const unsigned int southeast_pwm1_pins[] = { 1 };
+static const unsigned int southeast_sdmmc1_pins[] = {
 	16, 17, 20, 23, 24, 26, 63, 65, 67, 68, 69,
 };
-static const unsigned southeast_sdmmc2_pins[] = { 15, 18, 19, 21, 22, 25 };
-static const unsigned southeast_sdmmc3_pins[] = {
+static const unsigned int southeast_sdmmc2_pins[] = { 15, 18, 19, 21, 22, 25 };
+static const unsigned int southeast_sdmmc3_pins[] = {
 	30, 31, 32, 33, 34, 35, 78, 81, 85,
 };
-static const unsigned southeast_spi1_pins[] = { 60, 61, 62, 64, 66 };
-static const unsigned southeast_spi2_pins[] = { 2, 3, 4, 6, 7 };
+static const unsigned int southeast_spi1_pins[] = { 60, 61, 62, 64, 66 };
+static const unsigned int southeast_spi2_pins[] = { 2, 3, 4, 6, 7 };
 
 static const struct chv_pingroup southeast_groups[] = {
 	PIN_GROUP("pwm0_grp", southeast_pwm0_pins, 1, false),
@@ -657,11 +659,11 @@ static const struct chv_community *chv_communities[] = {
 	&southeast_community,
 };
 
-static void __iomem *chv_padreg(struct chv_pinctrl *pctrl, unsigned offset,
-				unsigned reg)
+static void __iomem *chv_padreg(struct chv_pinctrl *pctrl, unsigned int offset,
+				unsigned int reg)
 {
-	unsigned family_no = offset / MAX_FAMILY_PAD_GPIO_NO;
-	unsigned pad_no = offset % MAX_FAMILY_PAD_GPIO_NO;
+	unsigned int family_no = offset / MAX_FAMILY_PAD_GPIO_NO;
+	unsigned int pad_no = offset % MAX_FAMILY_PAD_GPIO_NO;
 
 	offset = FAMILY_PAD_REGS_OFF + FAMILY_PAD_REGS_SIZE * family_no +
 		 GPIO_REGS_SIZE * pad_no;
@@ -677,7 +679,7 @@ static void chv_writel(u32 value, void __iomem *reg)
 }
 
 /* When Pad Cfg is locked, driver can only change GPIOTXState or GPIORXState */
-static bool chv_pad_locked(struct chv_pinctrl *pctrl, unsigned offset)
+static bool chv_pad_locked(struct chv_pinctrl *pctrl, unsigned int offset)
 {
 	void __iomem *reg;
 
@@ -693,15 +695,15 @@ static int chv_get_groups_count(struct pinctrl_dev *pctldev)
 }
 
 static const char *chv_get_group_name(struct pinctrl_dev *pctldev,
-				      unsigned group)
+				      unsigned int group)
 {
 	struct chv_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
 
 	return pctrl->community->groups[group].name;
 }
 
-static int chv_get_group_pins(struct pinctrl_dev *pctldev, unsigned group,
-			      const unsigned **pins, unsigned *npins)
+static int chv_get_group_pins(struct pinctrl_dev *pctldev, unsigned int group,
+			      const unsigned int **pins, unsigned int *npins)
 {
 	struct chv_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
 
@@ -711,7 +713,7 @@ static int chv_get_group_pins(struct pinctrl_dev *pctldev, unsigned group,
 }
 
 static void chv_pin_dbg_show(struct pinctrl_dev *pctldev, struct seq_file *s,
-			     unsigned offset)
+			     unsigned int offset)
 {
 	struct chv_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
 	unsigned long flags;
@@ -758,7 +760,7 @@ static int chv_get_functions_count(struct pinctrl_dev *pctldev)
 }
 
 static const char *chv_get_function_name(struct pinctrl_dev *pctldev,
-					 unsigned function)
+					 unsigned int function)
 {
 	struct chv_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
 
@@ -766,9 +768,9 @@ static const char *chv_get_function_name(struct pinctrl_dev *pctldev,
 }
 
 static int chv_get_function_groups(struct pinctrl_dev *pctldev,
-				   unsigned function,
+				   unsigned int function,
 				   const char * const **groups,
-				   unsigned * const ngroups)
+				   unsigned int * const ngroups)
 {
 	struct chv_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
 
@@ -777,8 +779,8 @@ static int chv_get_function_groups(struct pinctrl_dev *pctldev,
 	return 0;
 }
 
-static int chv_pinmux_set_mux(struct pinctrl_dev *pctldev, unsigned function,
-			      unsigned group)
+static int chv_pinmux_set_mux(struct pinctrl_dev *pctldev,
+			      unsigned int function, unsigned int group)
 {
 	struct chv_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
 	const struct chv_pingroup *grp;
@@ -844,7 +846,7 @@ static int chv_pinmux_set_mux(struct pinctrl_dev *pctldev, unsigned function,
 
 static int chv_gpio_request_enable(struct pinctrl_dev *pctldev,
 				   struct pinctrl_gpio_range *range,
-				   unsigned offset)
+				   unsigned int offset)
 {
 	struct chv_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
 	unsigned long flags;
@@ -904,7 +906,7 @@ static int chv_gpio_request_enable(struct pinctrl_dev *pctldev,
 
 static void chv_gpio_disable_free(struct pinctrl_dev *pctldev,
 				  struct pinctrl_gpio_range *range,
-				  unsigned offset)
+				  unsigned int offset)
 {
 	struct chv_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
 	unsigned long flags;
@@ -922,7 +924,7 @@ static void chv_gpio_disable_free(struct pinctrl_dev *pctldev,
 
 static int chv_gpio_set_direction(struct pinctrl_dev *pctldev,
 				  struct pinctrl_gpio_range *range,
-				  unsigned offset, bool input)
+				  unsigned int offset, bool input)
 {
 	struct chv_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
 	void __iomem *reg = chv_padreg(pctrl, offset, CHV_PADCTRL0);
@@ -953,7 +955,7 @@ static const struct pinmux_ops chv_pinmux_ops = {
 	.gpio_set_direction = chv_gpio_set_direction,
 };
 
-static int chv_config_get(struct pinctrl_dev *pctldev, unsigned pin,
+static int chv_config_get(struct pinctrl_dev *pctldev, unsigned int pin,
 			  unsigned long *config)
 {
 	struct chv_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
@@ -1033,7 +1035,7 @@ static int chv_config_get(struct pinctrl_dev *pctldev, unsigned pin,
 	return 0;
 }
 
-static int chv_config_set_pull(struct chv_pinctrl *pctrl, unsigned pin,
+static int chv_config_set_pull(struct chv_pinctrl *pctrl, unsigned int pin,
 			       enum pin_config_param param, u16 arg)
 {
 	void __iomem *reg = chv_padreg(pctrl, pin, CHV_PADCTRL0);
@@ -1099,8 +1101,8 @@ static int chv_config_set_pull(struct chv_pinctrl *pctrl, unsigned pin,
 	return 0;
 }
 
-static int chv_config_set(struct pinctrl_dev *pctldev, unsigned pin,
-			  unsigned long *configs, unsigned nconfigs)
+static int chv_config_set(struct pinctrl_dev *pctldev, unsigned int pin,
+			  unsigned long *configs, unsigned int nconfigs)
 {
 	struct chv_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
 	enum pin_config_param param;
@@ -1147,13 +1149,13 @@ static struct pinctrl_desc chv_pinctrl_desc = {
 	.owner = THIS_MODULE,
 };
 
-static unsigned chv_gpio_offset_to_pin(struct chv_pinctrl *pctrl,
-				       unsigned offset)
+static unsigned int chv_gpio_offset_to_pin(struct chv_pinctrl *pctrl,
+					   unsigned int offset)
 {
 	return pctrl->community->pins[offset].number;
 }
 
-static int chv_gpio_get(struct gpio_chip *chip, unsigned offset)
+static int chv_gpio_get(struct gpio_chip *chip, unsigned int offset)
 {
 	struct chv_pinctrl *pctrl = gpiochip_get_data(chip);
 	int pin = chv_gpio_offset_to_pin(pctrl, offset);
@@ -1172,10 +1174,10 @@ static int chv_gpio_get(struct gpio_chip *chip, unsigned offset)
 	return !!(ctrl0 & CHV_PADCTRL0_GPIORXSTATE);
 }
 
-static void chv_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
+static void chv_gpio_set(struct gpio_chip *chip, unsigned int offset, int value)
 {
 	struct chv_pinctrl *pctrl = gpiochip_get_data(chip);
-	unsigned pin = chv_gpio_offset_to_pin(pctrl, offset);
+	unsigned int pin = chv_gpio_offset_to_pin(pctrl, offset);
 	unsigned long flags;
 	void __iomem *reg;
 	u32 ctrl0;
@@ -1195,10 +1197,10 @@ static void chv_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
 	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
 }
 
-static int chv_gpio_get_direction(struct gpio_chip *chip, unsigned offset)
+static int chv_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)
 {
 	struct chv_pinctrl *pctrl = gpiochip_get_data(chip);
-	unsigned pin = chv_gpio_offset_to_pin(pctrl, offset);
+	unsigned int pin = chv_gpio_offset_to_pin(pctrl, offset);
 	u32 ctrl0, direction;
 	unsigned long flags;
 
@@ -1212,13 +1214,13 @@ static int chv_gpio_get_direction(struct gpio_chip *chip, unsigned offset)
 	return direction != CHV_PADCTRL0_GPIOCFG_GPO;
 }
 
-static int chv_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
+static int chv_gpio_direction_input(struct gpio_chip *chip, unsigned int offset)
 {
 	return pinctrl_gpio_direction_input(chip->base + offset);
 }
 
-static int chv_gpio_direction_output(struct gpio_chip *chip, unsigned offset,
-				     int value)
+static int chv_gpio_direction_output(struct gpio_chip *chip,
+				     unsigned int offset, int value)
 {
 	chv_gpio_set(chip, offset, value);
 	return pinctrl_gpio_direction_output(chip->base + offset);
@@ -1286,7 +1288,7 @@ static void chv_gpio_irq_unmask(struct irq_data *d)
 	chv_gpio_irq_mask_unmask(d, false);
 }
 
-static unsigned chv_gpio_irq_startup(struct irq_data *d)
+static unsigned int chv_gpio_irq_startup(struct irq_data *d)
 {
 	/*
 	 * Check if the interrupt has been requested with 0 as triggering
@@ -1301,7 +1303,7 @@ static unsigned chv_gpio_irq_startup(struct irq_data *d)
 	if (irqd_get_trigger_type(d) == IRQ_TYPE_NONE) {
 		struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
 		struct chv_pinctrl *pctrl = gpiochip_get_data(gc);
-		unsigned offset = irqd_to_hwirq(d);
+		unsigned int offset = irqd_to_hwirq(d);
 		int pin = chv_gpio_offset_to_pin(pctrl, offset);
 		irq_flow_handler_t handler;
 		unsigned long flags;
@@ -1329,11 +1331,11 @@ static unsigned chv_gpio_irq_startup(struct irq_data *d)
 	return 0;
 }
 
-static int chv_gpio_irq_type(struct irq_data *d, unsigned type)
+static int chv_gpio_irq_type(struct irq_data *d, unsigned int type)
 {
 	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
 	struct chv_pinctrl *pctrl = gpiochip_get_data(gc);
-	unsigned offset = irqd_to_hwirq(d);
+	unsigned int offset = irqd_to_hwirq(d);
 	int pin = chv_gpio_offset_to_pin(pctrl, offset);
 	unsigned long flags;
 	u32 value;
@@ -1414,7 +1416,7 @@ static void chv_gpio_irq_handler(struct irq_desc *desc)
 
 	pending = readl(pctrl->regs + CHV_INTSTAT);
 	for_each_set_bit(intr_line, &pending, 16) {
-		unsigned irq, offset;
+		unsigned int irq, offset;
 
 		offset = pctrl->intr_lines[intr_line];
 		irq = irq_find_mapping(gc->irqdomain, offset);
-- 
2.1.4

[PATCH 2/5] pinctrl: cherryview: add option to set open-drain pin config

[Dan O'Donovan]

On some CHV platforms, we need an option to configure the
open-drain setting for these pins.  This adds support for the
PIN_CONFIG_DRIVE_PUSH_PULL and PIN_CONFIG_DRIVE_OPEN_DRAIN to
disable/enable open-drain mode for a specific pin.

Signed-off-by: Dan O'Donovan <dan@emutex.com>
---
 drivers/pinctrl/intel/pinctrl-cherryview.c | 33 ++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/drivers/pinctrl/intel/pinctrl-cherryview.c b/drivers/pinctrl/intel/pinctrl-cherryview.c
index c79f4cc..7e5545f 100644
--- a/drivers/pinctrl/intel/pinctrl-cherryview.c
+++ b/drivers/pinctrl/intel/pinctrl-cherryview.c
@@ -1101,6 +1101,27 @@ static int chv_config_set_pull(struct chv_pinctrl *pctrl, unsigned int pin,
 	return 0;
 }
 
+static int chv_config_set_oden(struct chv_pinctrl *pctrl, unsigned int pin,
+			       bool enable)
+{
+	void __iomem *reg = chv_padreg(pctrl, pin, CHV_PADCTRL1);
+	unsigned long flags;
+	u32 ctrl1;
+
+	raw_spin_lock_irqsave(&pctrl->lock, flags);
+	ctrl1 = readl(reg);
+
+	if (enable)
+		ctrl1 |= CHV_PADCTRL1_ODEN;
+	else
+		ctrl1 &= ~CHV_PADCTRL1_ODEN;
+
+	chv_writel(ctrl1, reg);
+	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
+
+	return 0;
+}
+
 static int chv_config_set(struct pinctrl_dev *pctldev, unsigned int pin,
 			  unsigned long *configs, unsigned int nconfigs)
 {
@@ -1125,6 +1146,18 @@ static int chv_config_set(struct pinctrl_dev *pctldev, unsigned int pin,
 				return ret;
 			break;
 
+		case PIN_CONFIG_DRIVE_PUSH_PULL:
+			ret = chv_config_set_oden(pctrl, pin, false);
+			if (ret)
+				return ret;
+			break;
+
+		case PIN_CONFIG_DRIVE_OPEN_DRAIN:
+			ret = chv_config_set_oden(pctrl, pin, true);
+			if (ret)
+				return ret;
+			break;
+
 		default:
 			return -ENOTSUPP;
 		}
-- 
2.1.4

[PATCH 3/5] pinctrl: cherryview: add handlers for pin_config_group_get/set

[Dan O'Donovan]

Pin config get/set handlers for pin groups were previously not
implemented by this driver.  The pin_config_group_set is
particularly useful for applying a common config setting to all
pins in a specified group with a single call, without the caller
needing to reference each individual pin by name.

Signed-off-by: Dan O'Donovan <dan@emutex.com>
---
 drivers/pinctrl/intel/pinctrl-cherryview.c | 42 ++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/drivers/pinctrl/intel/pinctrl-cherryview.c b/drivers/pinctrl/intel/pinctrl-cherryview.c
index 7e5545f..7df4b40 100644
--- a/drivers/pinctrl/intel/pinctrl-cherryview.c
+++ b/drivers/pinctrl/intel/pinctrl-cherryview.c
@@ -1169,10 +1169,52 @@ static int chv_config_set(struct pinctrl_dev *pctldev, unsigned int pin,
 	return 0;
 }
 
+static int chv_config_group_get(struct pinctrl_dev *pctldev,
+				unsigned int group,
+				unsigned long *config)
+{
+	const unsigned int *pins;
+	unsigned int npins;
+	int ret;
+
+	ret = chv_get_group_pins(pctldev, group, &pins, &npins);
+	if (ret)
+		return ret;
+
+	ret = chv_config_get(pctldev, pins[0], config);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int chv_config_group_set(struct pinctrl_dev *pctldev,
+				unsigned int group, unsigned long *configs,
+				unsigned int num_configs)
+{
+	const unsigned int *pins;
+	unsigned int npins;
+	int i, ret;
+
+	ret = chv_get_group_pins(pctldev, group, &pins, &npins);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < npins; i++) {
+		ret = chv_config_set(pctldev, pins[i], configs, num_configs);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
 static const struct pinconf_ops chv_pinconf_ops = {
 	.is_generic = true,
 	.pin_config_set = chv_config_set,
 	.pin_config_get = chv_config_get,
+	.pin_config_group_get = chv_config_group_get,
+	.pin_config_group_set = chv_config_group_set,
 };
 
 static struct pinctrl_desc chv_pinctrl_desc = {
-- 
2.1.4

[PATCH 4/5] pinctrl: cherryview: prevent concurrent access to GPIO controllers

[Dan O'Donovan]

Due to a silicon issue on the Atom X5-Z8000 "Cherry Trail" processor
series, a common lock must be used to prevent concurrent accesses
across the 4 GPIO controllers managed by this driver.

See Intel Atom Z8000 Processor Series Specification Update
(Rev. 005), errata #CHT34, for further information.

Signed-off-by: Dan O'Donovan <dan@emutex.com>
---
 drivers/pinctrl/intel/pinctrl-cherryview.c | 84 ++++++++++++++++--------------
 1 file changed, 46 insertions(+), 38 deletions(-)

diff --git a/drivers/pinctrl/intel/pinctrl-cherryview.c b/drivers/pinctrl/intel/pinctrl-cherryview.c
index 7df4b40..5064f93 100644
--- a/drivers/pinctrl/intel/pinctrl-cherryview.c
+++ b/drivers/pinctrl/intel/pinctrl-cherryview.c
@@ -160,7 +160,6 @@ struct chv_pin_context {
  * @pctldev: Pointer to the pin controller device
  * @chip: GPIO chip in this pin controller
  * @regs: MMIO registers
- * @lock: Lock to serialize register accesses
  * @intr_lines: Stores mapping between 16 HW interrupt wires and GPIO
  *		offset (in GPIO number space)
  * @community: Community this pinctrl instance represents
@@ -174,7 +173,6 @@ struct chv_pinctrl {
 	struct pinctrl_dev *pctldev;
 	struct gpio_chip chip;
 	void __iomem *regs;
-	raw_spinlock_t lock;
 	unsigned int intr_lines[16];
 	const struct chv_community *community;
 	u32 saved_intmask;
@@ -659,6 +657,17 @@ static const struct chv_community *chv_communities[] = {
 	&southeast_community,
 };
 
+/*
+ * Lock to serialize register accesses
+ *
+ * Due to a silicon issue, a shared lock must be used to prevent
+ * concurrent accesses across the 4 GPIO controllers.
+ *
+ * See Intel Atom Z8000 Processor Series Specification Update (Rev. 005),
+ * errata #CHT34, for further information.
+ */
+static DEFINE_RAW_SPINLOCK(chv_reg_lock);
+
 static void __iomem *chv_padreg(struct chv_pinctrl *pctrl, unsigned int offset,
 				unsigned int reg)
 {
@@ -720,13 +729,13 @@ static void chv_pin_dbg_show(struct pinctrl_dev *pctldev, struct seq_file *s,
 	u32 ctrl0, ctrl1;
 	bool locked;
 
-	raw_spin_lock_irqsave(&pctrl->lock, flags);
+	raw_spin_lock_irqsave(&chv_reg_lock, flags);
 
 	ctrl0 = readl(chv_padreg(pctrl, offset, CHV_PADCTRL0));
 	ctrl1 = readl(chv_padreg(pctrl, offset, CHV_PADCTRL1));
 	locked = chv_pad_locked(pctrl, offset);
 
-	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
+	raw_spin_unlock_irqrestore(&chv_reg_lock, flags);
 
 	if (ctrl0 & CHV_PADCTRL0_GPIOEN) {
 		seq_puts(s, "GPIO ");
@@ -789,14 +798,14 @@ static int chv_pinmux_set_mux(struct pinctrl_dev *pctldev,
 
 	grp = &pctrl->community->groups[group];
 
-	raw_spin_lock_irqsave(&pctrl->lock, flags);
+	raw_spin_lock_irqsave(&chv_reg_lock, flags);
 
 	/* Check first that the pad is not locked */
 	for (i = 0; i < grp->npins; i++) {
 		if (chv_pad_locked(pctrl, grp->pins[i])) {
 			dev_warn(pctrl->dev, "unable to set mode for locked pin %u\n",
 				 grp->pins[i]);
-			raw_spin_unlock_irqrestore(&pctrl->lock, flags);
+			raw_spin_unlock_irqrestore(&chv_reg_lock, flags);
 			return -EBUSY;
 		}
 	}
@@ -839,7 +848,7 @@ static int chv_pinmux_set_mux(struct pinctrl_dev *pctldev,
 			pin, altfunc->mode, altfunc->invert_oe ? "" : "not ");
 	}
 
-	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
+	raw_spin_unlock_irqrestore(&chv_reg_lock, flags);
 
 	return 0;
 }
@@ -853,13 +862,13 @@ static int chv_gpio_request_enable(struct pinctrl_dev *pctldev,
 	void __iomem *reg;
 	u32 value;
 
-	raw_spin_lock_irqsave(&pctrl->lock, flags);
+	raw_spin_lock_irqsave(&chv_reg_lock, flags);
 
 	if (chv_pad_locked(pctrl, offset)) {
 		value = readl(chv_padreg(pctrl, offset, CHV_PADCTRL0));
 		if (!(value & CHV_PADCTRL0_GPIOEN)) {
 			/* Locked so cannot enable */
-			raw_spin_unlock_irqrestore(&pctrl->lock, flags);
+			raw_spin_unlock_irqrestore(&chv_reg_lock, flags);
 			return -EBUSY;
 		}
 	} else {
@@ -899,7 +908,7 @@ static int chv_gpio_request_enable(struct pinctrl_dev *pctldev,
 		chv_writel(value, reg);
 	}
 
-	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
+	raw_spin_unlock_irqrestore(&chv_reg_lock, flags);
 
 	return 0;
 }
@@ -913,13 +922,13 @@ static void chv_gpio_disable_free(struct pinctrl_dev *pctldev,
 	void __iomem *reg;
 	u32 value;
 
-	raw_spin_lock_irqsave(&pctrl->lock, flags);
+	raw_spin_lock_irqsave(&chv_reg_lock, flags);
 
 	reg = chv_padreg(pctrl, offset, CHV_PADCTRL0);
 	value = readl(reg) & ~CHV_PADCTRL0_GPIOEN;
 	chv_writel(value, reg);
 
-	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
+	raw_spin_unlock_irqrestore(&chv_reg_lock, flags);
 }
 
 static int chv_gpio_set_direction(struct pinctrl_dev *pctldev,
@@ -931,7 +940,7 @@ static int chv_gpio_set_direction(struct pinctrl_dev *pctldev,
 	unsigned long flags;
 	u32 ctrl0;
 
-	raw_spin_lock_irqsave(&pctrl->lock, flags);
+	raw_spin_lock_irqsave(&chv_reg_lock, flags);
 
 	ctrl0 = readl(reg) & ~CHV_PADCTRL0_GPIOCFG_MASK;
 	if (input)
@@ -940,7 +949,7 @@ static int chv_gpio_set_direction(struct pinctrl_dev *pctldev,
 		ctrl0 |= CHV_PADCTRL0_GPIOCFG_GPO << CHV_PADCTRL0_GPIOCFG_SHIFT;
 	chv_writel(ctrl0, reg);
 
-	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
+	raw_spin_unlock_irqrestore(&chv_reg_lock, flags);
 
 	return 0;
 }
@@ -965,10 +974,10 @@ static int chv_config_get(struct pinctrl_dev *pctldev, unsigned int pin,
 	u16 arg = 0;
 	u32 term;
 
-	raw_spin_lock_irqsave(&pctrl->lock, flags);
+	raw_spin_lock_irqsave(&chv_reg_lock, flags);
 	ctrl0 = readl(chv_padreg(pctrl, pin, CHV_PADCTRL0));
 	ctrl1 = readl(chv_padreg(pctrl, pin, CHV_PADCTRL1));
-	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
+	raw_spin_unlock_irqrestore(&chv_reg_lock, flags);
 
 	term = (ctrl0 & CHV_PADCTRL0_TERM_MASK) >> CHV_PADCTRL0_TERM_SHIFT;
 
@@ -1042,7 +1051,7 @@ static int chv_config_set_pull(struct chv_pinctrl *pctrl, unsigned int pin,
 	unsigned long flags;
 	u32 ctrl0, pull;
 
-	raw_spin_lock_irqsave(&pctrl->lock, flags);
+	raw_spin_lock_irqsave(&chv_reg_lock, flags);
 	ctrl0 = readl(reg);
 
 	switch (param) {
@@ -1065,7 +1074,7 @@ static int chv_config_set_pull(struct chv_pinctrl *pctrl, unsigned int pin,
 			pull = CHV_PADCTRL0_TERM_20K << CHV_PADCTRL0_TERM_SHIFT;
 			break;
 		default:
-			raw_spin_unlock_irqrestore(&pctrl->lock, flags);
+			raw_spin_unlock_irqrestore(&chv_reg_lock, flags);
 			return -EINVAL;
 		}
 
@@ -1083,7 +1092,7 @@ static int chv_config_set_pull(struct chv_pinctrl *pctrl, unsigned int pin,
 			pull = CHV_PADCTRL0_TERM_20K << CHV_PADCTRL0_TERM_SHIFT;
 			break;
 		default:
-			raw_spin_unlock_irqrestore(&pctrl->lock, flags);
+			raw_spin_unlock_irqrestore(&chv_reg_lock, flags);
 			return -EINVAL;
 		}
 
@@ -1091,12 +1100,12 @@ static int chv_config_set_pull(struct chv_pinctrl *pctrl, unsigned int pin,
 		break;
 
 	default:
-		raw_spin_unlock_irqrestore(&pctrl->lock, flags);
+		raw_spin_unlock_irqrestore(&chv_reg_lock, flags);
 		return -EINVAL;
 	}
 
 	chv_writel(ctrl0, reg);
-	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
+	raw_spin_unlock_irqrestore(&chv_reg_lock, flags);
 
 	return 0;
 }
@@ -1108,7 +1117,7 @@ static int chv_config_set_oden(struct chv_pinctrl *pctrl, unsigned int pin,
 	unsigned long flags;
 	u32 ctrl1;
 
-	raw_spin_lock_irqsave(&pctrl->lock, flags);
+	raw_spin_lock_irqsave(&chv_reg_lock, flags);
 	ctrl1 = readl(reg);
 
 	if (enable)
@@ -1117,7 +1126,7 @@ static int chv_config_set_oden(struct chv_pinctrl *pctrl, unsigned int pin,
 		ctrl1 &= ~CHV_PADCTRL1_ODEN;
 
 	chv_writel(ctrl1, reg);
-	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
+	raw_spin_unlock_irqrestore(&chv_reg_lock, flags);
 
 	return 0;
 }
@@ -1237,9 +1246,9 @@ static int chv_gpio_get(struct gpio_chip *chip, unsigned int offset)
 	unsigned long flags;
 	u32 ctrl0, cfg;
 
-	raw_spin_lock_irqsave(&pctrl->lock, flags);
+	raw_spin_lock_irqsave(&chv_reg_lock, flags);
 	ctrl0 = readl(chv_padreg(pctrl, pin, CHV_PADCTRL0));
-	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
+	raw_spin_unlock_irqrestore(&chv_reg_lock, flags);
 
 	cfg = ctrl0 & CHV_PADCTRL0_GPIOCFG_MASK;
 	cfg >>= CHV_PADCTRL0_GPIOCFG_SHIFT;
@@ -1257,7 +1266,7 @@ static void chv_gpio_set(struct gpio_chip *chip, unsigned int offset, int value)
 	void __iomem *reg;
 	u32 ctrl0;
 
-	raw_spin_lock_irqsave(&pctrl->lock, flags);
+	raw_spin_lock_irqsave(&chv_reg_lock, flags);
 
 	reg = chv_padreg(pctrl, pin, CHV_PADCTRL0);
 	ctrl0 = readl(reg);
@@ -1269,7 +1278,7 @@ static void chv_gpio_set(struct gpio_chip *chip, unsigned int offset, int value)
 
 	chv_writel(ctrl0, reg);
 
-	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
+	raw_spin_unlock_irqrestore(&chv_reg_lock, flags);
 }
 
 static int chv_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)
@@ -1279,9 +1288,9 @@ static int chv_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)
 	u32 ctrl0, direction;
 	unsigned long flags;
 
-	raw_spin_lock_irqsave(&pctrl->lock, flags);
+	raw_spin_lock_irqsave(&chv_reg_lock, flags);
 	ctrl0 = readl(chv_padreg(pctrl, pin, CHV_PADCTRL0));
-	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
+	raw_spin_unlock_irqrestore(&chv_reg_lock, flags);
 
 	direction = ctrl0 & CHV_PADCTRL0_GPIOCFG_MASK;
 	direction >>= CHV_PADCTRL0_GPIOCFG_SHIFT;
@@ -1319,14 +1328,14 @@ static void chv_gpio_irq_ack(struct irq_data *d)
 	int pin = chv_gpio_offset_to_pin(pctrl, irqd_to_hwirq(d));
 	u32 intr_line;
 
-	raw_spin_lock(&pctrl->lock);
+	raw_spin_lock(&chv_reg_lock);
 
 	intr_line = readl(chv_padreg(pctrl, pin, CHV_PADCTRL0));
 	intr_line &= CHV_PADCTRL0_INTSEL_MASK;
 	intr_line >>= CHV_PADCTRL0_INTSEL_SHIFT;
 	chv_writel(BIT(intr_line), pctrl->regs + CHV_INTSTAT);
 
-	raw_spin_unlock(&pctrl->lock);
+	raw_spin_unlock(&chv_reg_lock);
 }
 
 static void chv_gpio_irq_mask_unmask(struct irq_data *d, bool mask)
@@ -1337,7 +1346,7 @@ static void chv_gpio_irq_mask_unmask(struct irq_data *d, bool mask)
 	u32 value, intr_line;
 	unsigned long flags;
 
-	raw_spin_lock_irqsave(&pctrl->lock, flags);
+	raw_spin_lock_irqsave(&chv_reg_lock, flags);
 
 	intr_line = readl(chv_padreg(pctrl, pin, CHV_PADCTRL0));
 	intr_line &= CHV_PADCTRL0_INTSEL_MASK;
@@ -1350,7 +1359,7 @@ static void chv_gpio_irq_mask_unmask(struct irq_data *d, bool mask)
 		value |= BIT(intr_line);
 	chv_writel(value, pctrl->regs + CHV_INTMASK);
 
-	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
+	raw_spin_unlock_irqrestore(&chv_reg_lock, flags);
 }
 
 static void chv_gpio_irq_mask(struct irq_data *d)
@@ -1384,7 +1393,7 @@ static unsigned int chv_gpio_irq_startup(struct irq_data *d)
 		unsigned long flags;
 		u32 intsel, value;
 
-		raw_spin_lock_irqsave(&pctrl->lock, flags);
+		raw_spin_lock_irqsave(&chv_reg_lock, flags);
 		intsel = readl(chv_padreg(pctrl, pin, CHV_PADCTRL0));
 		intsel &= CHV_PADCTRL0_INTSEL_MASK;
 		intsel >>= CHV_PADCTRL0_INTSEL_SHIFT;
@@ -1399,7 +1408,7 @@ static unsigned int chv_gpio_irq_startup(struct irq_data *d)
 			irq_set_handler_locked(d, handler);
 			pctrl->intr_lines[intsel] = offset;
 		}
-		raw_spin_unlock_irqrestore(&pctrl->lock, flags);
+		raw_spin_unlock_irqrestore(&chv_reg_lock, flags);
 	}
 
 	chv_gpio_irq_unmask(d);
@@ -1415,7 +1424,7 @@ static int chv_gpio_irq_type(struct irq_data *d, unsigned int type)
 	unsigned long flags;
 	u32 value;
 
-	raw_spin_lock_irqsave(&pctrl->lock, flags);
+	raw_spin_lock_irqsave(&chv_reg_lock, flags);
 
 	/*
 	 * Pins which can be used as shared interrupt are configured in
@@ -1464,7 +1473,7 @@ static int chv_gpio_irq_type(struct irq_data *d, unsigned int type)
 	else if (type & IRQ_TYPE_LEVEL_MASK)
 		irq_set_handler_locked(d, handle_level_irq);
 
-	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
+	raw_spin_unlock_irqrestore(&chv_reg_lock, flags);
 
 	return 0;
 }
@@ -1576,7 +1585,6 @@ static int chv_pinctrl_probe(struct platform_device *pdev)
 	if (i == ARRAY_SIZE(chv_communities))
 		return -ENODEV;
 
-	raw_spin_lock_init(&pctrl->lock);
 	pctrl->dev = &pdev->dev;
 
 #ifdef CONFIG_PM_SLEEP
-- 
2.1.4

[PATCH 5/5] pinctrl: cherryview: restore padctrl1 reg when gpio is disabled

[Dan O'Donovan]

chv_gpio_request_enable() clears some bits in the padctrl1 register
when GPIO mode is selected, but these bits are not restored by
chv_gpio_disable_free() when GPIO mode is unselected and this can
prevent other pin modes (e.g. I2C) from functioning correctly
thereafter on that pin.  This patch adds saving/restoring of those
bits.

Signed-off-by: Dan O'Donovan <dan@emutex.com>
---
 drivers/pinctrl/intel/pinctrl-cherryview.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/pinctrl/intel/pinctrl-cherryview.c b/drivers/pinctrl/intel/pinctrl-cherryview.c
index 5064f93..a75cc8c 100644
--- a/drivers/pinctrl/intel/pinctrl-cherryview.c
+++ b/drivers/pinctrl/intel/pinctrl-cherryview.c
@@ -177,6 +177,7 @@ struct chv_pinctrl {
 	const struct chv_community *community;
 	u32 saved_intmask;
 	struct chv_pin_context *saved_pin_context;
+	u32 saved_mux_padctrl1;
 };
 
 #define ALTERNATE_FUNCTION(p, m, i)		\
@@ -885,6 +886,7 @@ static int chv_gpio_request_enable(struct pinctrl_dev *pctldev,
 		/* Disable interrupt generation */
 		reg = chv_padreg(pctrl, offset, CHV_PADCTRL1);
 		value = readl(reg);
+		pctrl->saved_mux_padctrl1 = value;
 		value &= ~CHV_PADCTRL1_INTWAKECFG_MASK;
 		value &= ~CHV_PADCTRL1_INVRXTX_MASK;
 		chv_writel(value, reg);
@@ -928,6 +930,15 @@ static void chv_gpio_disable_free(struct pinctrl_dev *pctldev,
 	value = readl(reg) & ~CHV_PADCTRL0_GPIOEN;
 	chv_writel(value, reg);
 
+	/* Restore previous pad configuration */
+	reg = chv_padreg(pctrl, offset, CHV_PADCTRL1);
+	value = readl(reg);
+	value &= ~CHV_PADCTRL1_INTWAKECFG_MASK;
+	value |= pctrl->saved_mux_padctrl1 & CHV_PADCTRL1_INTWAKECFG_MASK;
+	value &= ~CHV_PADCTRL1_INVRXTX_MASK;
+	value |= pctrl->saved_mux_padctrl1 & CHV_PADCTRL1_INVRXTX_MASK;
+	chv_writel(value, reg);
+
 	raw_spin_unlock_irqrestore(&chv_reg_lock, flags);
 }
 
-- 
2.1.4

Re: [PATCH 1/5] pinctrl: cherryview: convert bare unsigned to unsigned int

[Mika Westerberg]

On Thu, Jun 02, 2016 at 10:55:39PM +0100, Dan O'Donovan wrote:
> Addresses checkpatch warnings such as the following for this driver:
> 
>  WARNING: Prefer 'unsigned int' to bare use of 'unsigned'

I don't know if this is useful. Things like this make backporting real
fixes harder. For new code, it makes sense but not for converting existing.

Re: [PATCH 4/5] pinctrl: cherryview: prevent concurrent access to GPIO controllers

[Mika Westerberg]

On Thu, Jun 02, 2016 at 10:55:42PM +0100, Dan O'Donovan wrote:
> Due to a silicon issue on the Atom X5-Z8000 "Cherry Trail" processor
> series, a common lock must be used to prevent concurrent accesses
> across the 4 GPIO controllers managed by this driver.
> 
> See Intel Atom Z8000 Processor Series Specification Update
> (Rev. 005), errata #CHT34, for further information.
> 
> Signed-off-by: Dan O'Donovan <dan@emutex.com>

This should be the first patch in the series. It should also be tagged
for stable.

> ---
>  drivers/pinctrl/intel/pinctrl-cherryview.c | 84 ++++++++++++++++--------------
>  1 file changed, 46 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/pinctrl/intel/pinctrl-cherryview.c b/drivers/pinctrl/intel/pinctrl-cherryview.c
> index 7df4b40..5064f93 100644
> --- a/drivers/pinctrl/intel/pinctrl-cherryview.c
> +++ b/drivers/pinctrl/intel/pinctrl-cherryview.c
> @@ -160,7 +160,6 @@ struct chv_pin_context {
>   * @pctldev: Pointer to the pin controller device
>   * @chip: GPIO chip in this pin controller
>   * @regs: MMIO registers
> - * @lock: Lock to serialize register accesses
>   * @intr_lines: Stores mapping between 16 HW interrupt wires and GPIO
>   *		offset (in GPIO number space)
>   * @community: Community this pinctrl instance represents
> @@ -174,7 +173,6 @@ struct chv_pinctrl {
>  	struct pinctrl_dev *pctldev;
>  	struct gpio_chip chip;
>  	void __iomem *regs;
> -	raw_spinlock_t lock;
>  	unsigned int intr_lines[16];
>  	const struct chv_community *community;
>  	u32 saved_intmask;
> @@ -659,6 +657,17 @@ static const struct chv_community *chv_communities[] = {
>  	&southeast_community,
>  };
>  
> +/*
> + * Lock to serialize register accesses
> + *
> + * Due to a silicon issue, a shared lock must be used to prevent
> + * concurrent accesses across the 4 GPIO controllers.
> + *
> + * See Intel Atom Z8000 Processor Series Specification Update (Rev. 005),
> + * errata #CHT34, for further information.
> + */
> +static DEFINE_RAW_SPINLOCK(chv_reg_lock);

Can we call it chv_lock instead?

Re: [PATCH 2/5] pinctrl: cherryview: add option to set open-drain pin config

[Mika Westerberg]

On Thu, Jun 02, 2016 at 10:55:40PM +0100, Dan O'Donovan wrote:
> On some CHV platforms, we need an option to configure the
> open-drain setting for these pins.  This adds support for the
> PIN_CONFIG_DRIVE_PUSH_PULL and PIN_CONFIG_DRIVE_OPEN_DRAIN to
> disable/enable open-drain mode for a specific pin.
> 
> Signed-off-by: Dan O'Donovan <dan@emutex.com>

These should come after the locking fix. Looks reasonable to me.

Re: [PATCH 5/5] pinctrl: cherryview: restore padctrl1 reg when gpio is disabled

[Mika Westerberg]

On Thu, Jun 02, 2016 at 10:55:43PM +0100, Dan O'Donovan wrote:
> chv_gpio_request_enable() clears some bits in the padctrl1 register
> when GPIO mode is selected, but these bits are not restored by
> chv_gpio_disable_free() when GPIO mode is unselected and this can
> prevent other pin modes (e.g. I2C) from functioning correctly
> thereafter on that pin.  This patch adds saving/restoring of those
> bits.

Not sure how useful this is. If you want to mux I2C out of pins (even if
they were previosly configured as GPIO), you should call pinctrl to do
that (or let the core to do that automatically). Expecting that certain
(possibly unknown state) is restored does seem fragile to me.

Also what happens if the pin was originally muxed as GPIO?

Re: [PATCH 0/5] pinctrl: cherryview: fixes and enhancements

[Linus Walleij]

On Thu, Jun 2, 2016 at 11:55 PM, Dan O'Donovan <dan@emutex.com> wrote:

> This series includes a number of fixes and enhancements for the
> Cherryview pinctrl/gpio driver, developed during integration on the
> UP Board (based on the Intel X5-Z8350 "Cherry Trail" Atom SoC).
>
> Of particular note is a workaround for a documented silicon bug which
> causes data corruption of concurrent accesses to the GPIO controller
> registers on this SoC.
>
> Other patches include clean-up of checkpatch warnings, implementation
> of additional pin config functions and options, and a fix for a loss
> of pin config register state that can occur when switching in and out
> of GPIO pin mode.

Nice, but waiting for a revised patch set fixing Mika's comments.

Yours,
Linus Walleij

Re: [PATCH 1/5] pinctrl: cherryview: convert bare unsigned to unsigned int

[Dan O'Donovan]

On 06/06/2016 11:28 AM, Mika Westerberg wrote:
> On Thu, Jun 02, 2016 at 10:55:39PM +0100, Dan O'Donovan wrote:
>> Addresses checkpatch warnings such as the following for this driver:
>>
>>  WARNING: Prefer 'unsigned int' to bare use of 'unsigned'
> I don't know if this is useful. Things like this make backporting real
> fixes harder. For new code, it makes sense but not for converting existing.
Fair point.  I'll drop that patch so in the next version.  Thanks!
> --
> To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/5] pinctrl: cherryview: prevent concurrent access to GPIO controllers

[Dan O'Donovan]

On 06/06/2016 11:31 AM, Mika Westerberg wrote:
> On Thu, Jun 02, 2016 at 10:55:42PM +0100, Dan O'Donovan wrote:
>> Due to a silicon issue on the Atom X5-Z8000 "Cherry Trail" processor
>> series, a common lock must be used to prevent concurrent accesses
>> across the 4 GPIO controllers managed by this driver.
>>
>> See Intel Atom Z8000 Processor Series Specification Update
>> (Rev. 005), errata #CHT34, for further information.
>>
>> Signed-off-by: Dan O'Donovan <dan@emutex.com>
> This should be the first patch in the series. It should also be tagged
> for stable.
Makes sense.  I'll submit a new patch set to address this and other comments.  Thanks!
>
>> ---
>>  drivers/pinctrl/intel/pinctrl-cherryview.c | 84 ++++++++++++++++--------------
>>  1 file changed, 46 insertions(+), 38 deletions(-)
>>
>> diff --git a/drivers/pinctrl/intel/pinctrl-cherryview.c b/drivers/pinctrl/intel/pinctrl-cherryview.c
>> index 7df4b40..5064f93 100644
>> --- a/drivers/pinctrl/intel/pinctrl-cherryview.c
>> +++ b/drivers/pinctrl/intel/pinctrl-cherryview.c
>> @@ -160,7 +160,6 @@ struct chv_pin_context {
>>   * @pctldev: Pointer to the pin controller device
>>   * @chip: GPIO chip in this pin controller
>>   * @regs: MMIO registers
>> - * @lock: Lock to serialize register accesses
>>   * @intr_lines: Stores mapping between 16 HW interrupt wires and GPIO
>>   *		offset (in GPIO number space)
>>   * @community: Community this pinctrl instance represents
>> @@ -174,7 +173,6 @@ struct chv_pinctrl {
>>  	struct pinctrl_dev *pctldev;
>>  	struct gpio_chip chip;
>>  	void __iomem *regs;
>> -	raw_spinlock_t lock;
>>  	unsigned int intr_lines[16];
>>  	const struct chv_community *community;
>>  	u32 saved_intmask;
>> @@ -659,6 +657,17 @@ static const struct chv_community *chv_communities[] = {
>>  	&southeast_community,
>>  };
>>  
>> +/*
>> + * Lock to serialize register accesses
>> + *
>> + * Due to a silicon issue, a shared lock must be used to prevent
>> + * concurrent accesses across the 4 GPIO controllers.
>> + *
>> + * See Intel Atom Z8000 Processor Series Specification Update (Rev. 005),
>> + * errata #CHT34, for further information.
>> + */
>> +static DEFINE_RAW_SPINLOCK(chv_reg_lock);
> Can we call it chv_lock instead?
Ok
> --
> To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/5] pinctrl: cherryview: restore padctrl1 reg when gpio is disabled

[Dan O'Donovan]

On 06/06/2016 11:40 AM, Mika Westerberg wrote:
> On Thu, Jun 02, 2016 at 10:55:43PM +0100, Dan O'Donovan wrote:
>> chv_gpio_request_enable() clears some bits in the padctrl1 register
>> when GPIO mode is selected, but these bits are not restored by
>> chv_gpio_disable_free() when GPIO mode is unselected and this can
>> prevent other pin modes (e.g. I2C) from functioning correctly
>> thereafter on that pin.  This patch adds saving/restoring of those
>> bits.
> Not sure how useful this is. If you want to mux I2C out of pins (even if
> they were previosly configured as GPIO), you should call pinctrl to do
> that (or let the core to do that automatically). Expecting that certain
> (possibly unknown state) is restored does seem fragile to me.
Perhaps my description of the change was misleading.  Consider this scenario:
 * chv_pinmux_set_mux() is invoked at start-up (triggered by registering a pin map), and this sets a pin to an alternate mode (e.g. I2C, PWM, whatever).
   - For some pins/modes, this may set the INVRXTX bits in the PADCTRL1 register.  These bits may have also been set early by the BIOS.
 * some time later, the user exports the pin via sysfs for use as a GPIO, which triggers chv_gpio_request_enable()
   - chv_gpio_request_enable() clears some bits in the PADCTRL1 register (including INVRXTX)
 * later again, the user unexports the GPIO pin, which triggers chv_gpio_disable_free().
   - this returns the pin to its previously-selected alternate mode (GPIO is disabled).  However, the INVRXTX bits are not restored here, so the alternate mode no longer works.

>From the way the driver is written (possibly influenced by the hardware design), it appears that this can be a valid usage scenario - i.e. the pin can optionally be set to an alternate mode initially, but the user can bring it in/out of GPIO mode thereafter.  But, because the entry and exit from GPIO mode is not "symmetrical", it loses some configuration that was previously set for the alternate mode.

Admittedly, I've only encountered this scenario in validation testing - I'm not sure if there is a real use-case that would require this - so I can certainly drop this patch if you feel that a fix isn't warranted here.
>
> Also what happens if the pin was originally muxed as GPIO?
This shouldn't make any difference, I think.  The change here is just attempting to make chv_gpio_disable_free() reverse the action of chv_gpio_request_enable() - by restoring PADCTRL register fields to their previous state (whatever that was) before chv_gpio_request_enable() was called.

Re: [PATCH 5/5] pinctrl: cherryview: restore padctrl1 reg when gpio is disabled

[Mika Westerberg]

On Thu, Jun 09, 2016 at 05:41:04PM +0100, Dan O'Donovan wrote:
> On 06/06/2016 11:40 AM, Mika Westerberg wrote:
> > On Thu, Jun 02, 2016 at 10:55:43PM +0100, Dan O'Donovan wrote:
> >> chv_gpio_request_enable() clears some bits in the padctrl1 register
> >> when GPIO mode is selected, but these bits are not restored by
> >> chv_gpio_disable_free() when GPIO mode is unselected and this can
> >> prevent other pin modes (e.g. I2C) from functioning correctly
> >> thereafter on that pin.  This patch adds saving/restoring of those
> >> bits.
> > Not sure how useful this is. If you want to mux I2C out of pins (even if
> > they were previosly configured as GPIO), you should call pinctrl to do
> > that (or let the core to do that automatically). Expecting that certain
> > (possibly unknown state) is restored does seem fragile to me.
> Perhaps my description of the change was misleading.  Consider this scenario:
>  * chv_pinmux_set_mux() is invoked at start-up (triggered by registering a pin map), and this sets a pin to an alternate mode (e.g. I2C, PWM, whatever).
>    - For some pins/modes, this may set the INVRXTX bits in the PADCTRL1 register.  These bits may have also been set early by the BIOS.
>  * some time later, the user exports the pin via sysfs for use as a GPIO, which triggers chv_gpio_request_enable()
>    - chv_gpio_request_enable() clears some bits in the PADCTRL1 register (including INVRXTX)
>  * later again, the user unexports the GPIO pin, which triggers chv_gpio_disable_free().
>    - this returns the pin to its previously-selected alternate mode (GPIO is disabled).  However, the INVRXTX bits are not restored here, so the alternate mode no longer works.

I think it is not quaranteed anywhere that the pin returns to its
previous state. Better to explicitly request muxing of the pin as
needed.

> >From the way the driver is written (possibly influenced by the hardware design), it appears that this can be a valid usage scenario - i.e. the pin can optionally be set to an alternate mode initially, but the user can bring it in/out of GPIO mode thereafter.  But, because the entry and exit from GPIO mode is not "symmetrical", it loses some configuration that was previously set for the alternate mode.
> 
> Admittedly, I've only encountered this scenario in validation testing - I'm not sure if there is a real use-case that would require this - so I can certainly drop this patch if you feel that a fix isn't warranted here.
> >
> > Also what happens if the pin was originally muxed as GPIO?
> This shouldn't make any difference, I think.  The change here is just
> attempting to make chv_gpio_disable_free() reverse the action of
> chv_gpio_request_enable() - by restoring PADCTRL register fields to
> their previous state (whatever that was) before
> chv_gpio_request_enable() was called.

If it ever was called in the first place.

Re: [PATCH 5/5] pinctrl: cherryview: restore padctrl1 reg when gpio is disabled

[Dan O'Donovan]

On 06/10/2016 07:00 AM, Mika Westerberg wrote:
> On Thu, Jun 09, 2016 at 05:41:04PM +0100, Dan O'Donovan wrote:
>> On 06/06/2016 11:40 AM, Mika Westerberg wrote:
>>> On Thu, Jun 02, 2016 at 10:55:43PM +0100, Dan O'Donovan wrote:
>>>> chv_gpio_request_enable() clears some bits in the padctrl1 register
>>>> when GPIO mode is selected, but these bits are not restored by
>>>> chv_gpio_disable_free() when GPIO mode is unselected and this can
>>>> prevent other pin modes (e.g. I2C) from functioning correctly
>>>> thereafter on that pin.  This patch adds saving/restoring of those
>>>> bits.
>>> Not sure how useful this is. If you want to mux I2C out of pins (even if
>>> they were previosly configured as GPIO), you should call pinctrl to do
>>> that (or let the core to do that automatically). Expecting that certain
>>> (possibly unknown state) is restored does seem fragile to me.
>> Perhaps my description of the change was misleading.  Consider this scenario:
>>  * chv_pinmux_set_mux() is invoked at start-up (triggered by registering a pin map), and this sets a pin to an alternate mode (e.g. I2C, PWM, whatever).
>>    - For some pins/modes, this may set the INVRXTX bits in the PADCTRL1 register.  These bits may have also been set early by the BIOS.
>>  * some time later, the user exports the pin via sysfs for use as a GPIO, which triggers chv_gpio_request_enable()
>>    - chv_gpio_request_enable() clears some bits in the PADCTRL1 register (including INVRXTX)
>>  * later again, the user unexports the GPIO pin, which triggers chv_gpio_disable_free().
>>    - this returns the pin to its previously-selected alternate mode (GPIO is disabled).  However, the INVRXTX bits are not restored here, so the alternate mode no longer works.
> I think it is not quaranteed anywhere that the pin returns to its
> previous state. Better to explicitly request muxing of the pin as
> needed.
>
>> >From the way the driver is written (possibly influenced by the hardware design), it appears that this can be a valid usage scenario - i.e. the pin can optionally be set to an alternate mode initially, but the user can bring it in/out of GPIO mode thereafter.  But, because the entry and exit from GPIO mode is not "symmetrical", it loses some configuration that was previously set for the alternate mode.
>>
>> Admittedly, I've only encountered this scenario in validation testing - I'm not sure if there is a real use-case that would require this - so I can certainly drop this patch if you feel that a fix isn't warranted here.
>>> Also what happens if the pin was originally muxed as GPIO?
>> This shouldn't make any difference, I think.  The change here is just
>> attempting to make chv_gpio_disable_free() reverse the action of
>> chv_gpio_request_enable() - by restoring PADCTRL register fields to
>> their previous state (whatever that was) before
>> chv_gpio_request_enable() was called.
> If it ever was called in the first place.
Ah!  I had assumed that chv_gpio_disable_free() would not be called unless chv_gpio_request_enable() had been called previously, but it seems that's not a safe assumption.  I'll just drop that patch so.  Thanks for the feedback!

[PATCH v2 0/3] pinctrl: cherryview: fixes and enhancements

[Dan O'Donovan]

This series includes a number of fixes and enhancements for the
Cherryview pinctrl/gpio driver, developed during integration on the
UP Board (based on the Intel X5-Z8350 "Cherry Trail" Atom SoC).

Patch 1 provides a workaround for a documented silicon bug which
causes data corruption of concurrent accesses to the GPIO controller
registers on this SoC.

The other patches implement additional pin config functions/options
for this driver.

Changes since v1:
 * Addressed review comments from Mika Westerberg:
   - Drop clean-up of checkpatch warnings (impacts back-porting)
   - Re-order series to put fixes ahead of enhancements
   - Rename a variable
   - Tag concurrency fix (patch 1) for stable
   - Drop fix restoring PADCTRL1 bits when GPIO disabled (rejected)

Dan O'Donovan (3):
  pinctrl: cherryview: prevent concurrent access to GPIO controllers
  pinctrl: cherryview: add option to set open-drain pin config
  pinctrl: cherryview: add handlers for pin_config_group_get/set

 drivers/pinctrl/intel/pinctrl-cherryview.c | 155 ++++++++++++++++++++++-------
 1 file changed, 119 insertions(+), 36 deletions(-)

-- 
2.1.4

[PATCH v2 1/3] pinctrl: cherryview: prevent concurrent access to GPIO controllers

[Dan O'Donovan]

Due to a silicon issue on the Atom X5-Z8000 "Cherry Trail" processor
series, a common lock must be used to prevent concurrent accesses
across the 4 GPIO controllers managed by this driver.

See Intel Atom Z8000 Processor Series Specification Update
(Rev. 005), errata #CHT34, for further information.

Signed-off-by: Dan O'Donovan <dan@emutex.com>
Cc: stable <stable@vger.kernel.org>
---
 drivers/pinctrl/intel/pinctrl-cherryview.c | 80 ++++++++++++++++--------------
 1 file changed, 44 insertions(+), 36 deletions(-)

diff --git a/drivers/pinctrl/intel/pinctrl-cherryview.c b/drivers/pinctrl/intel/pinctrl-cherryview.c
index ac4f564..bf65c94 100644
--- a/drivers/pinctrl/intel/pinctrl-cherryview.c
+++ b/drivers/pinctrl/intel/pinctrl-cherryview.c
@@ -160,7 +160,6 @@ struct chv_pin_context {
  * @pctldev: Pointer to the pin controller device
  * @chip: GPIO chip in this pin controller
  * @regs: MMIO registers
- * @lock: Lock to serialize register accesses
  * @intr_lines: Stores mapping between 16 HW interrupt wires and GPIO
  *		offset (in GPIO number space)
  * @community: Community this pinctrl instance represents
@@ -174,7 +173,6 @@ struct chv_pinctrl {
 	struct pinctrl_dev *pctldev;
 	struct gpio_chip chip;
 	void __iomem *regs;
-	raw_spinlock_t lock;
 	unsigned intr_lines[16];
 	const struct chv_community *community;
 	u32 saved_intmask;
@@ -657,6 +655,17 @@ static const struct chv_community *chv_communities[] = {
 	&southeast_community,
 };
 
+/*
+ * Lock to serialize register accesses
+ *
+ * Due to a silicon issue, a shared lock must be used to prevent
+ * concurrent accesses across the 4 GPIO controllers.
+ *
+ * See Intel Atom Z8000 Processor Series Specification Update (Rev. 005),
+ * errata #CHT34, for further information.
+ */
+static DEFINE_RAW_SPINLOCK(chv_lock);
+
 static void __iomem *chv_padreg(struct chv_pinctrl *pctrl, unsigned offset,
 				unsigned reg)
 {
@@ -718,13 +727,13 @@ static void chv_pin_dbg_show(struct pinctrl_dev *pctldev, struct seq_file *s,
 	u32 ctrl0, ctrl1;
 	bool locked;
 
-	raw_spin_lock_irqsave(&pctrl->lock, flags);
+	raw_spin_lock_irqsave(&chv_lock, flags);
 
 	ctrl0 = readl(chv_padreg(pctrl, offset, CHV_PADCTRL0));
 	ctrl1 = readl(chv_padreg(pctrl, offset, CHV_PADCTRL1));
 	locked = chv_pad_locked(pctrl, offset);
 
-	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
+	raw_spin_unlock_irqrestore(&chv_lock, flags);
 
 	if (ctrl0 & CHV_PADCTRL0_GPIOEN) {
 		seq_puts(s, "GPIO ");
@@ -787,14 +796,14 @@ static int chv_pinmux_set_mux(struct pinctrl_dev *pctldev, unsigned function,
 
 	grp = &pctrl->community->groups[group];
 
-	raw_spin_lock_irqsave(&pctrl->lock, flags);
+	raw_spin_lock_irqsave(&chv_lock, flags);
 
 	/* Check first that the pad is not locked */
 	for (i = 0; i < grp->npins; i++) {
 		if (chv_pad_locked(pctrl, grp->pins[i])) {
 			dev_warn(pctrl->dev, "unable to set mode for locked pin %u\n",
 				 grp->pins[i]);
-			raw_spin_unlock_irqrestore(&pctrl->lock, flags);
+			raw_spin_unlock_irqrestore(&chv_lock, flags);
 			return -EBUSY;
 		}
 	}
@@ -837,7 +846,7 @@ static int chv_pinmux_set_mux(struct pinctrl_dev *pctldev, unsigned function,
 			pin, altfunc->mode, altfunc->invert_oe ? "" : "not ");
 	}
 
-	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
+	raw_spin_unlock_irqrestore(&chv_lock, flags);
 
 	return 0;
 }
@@ -851,13 +860,13 @@ static int chv_gpio_request_enable(struct pinctrl_dev *pctldev,
 	void __iomem *reg;
 	u32 value;
 
-	raw_spin_lock_irqsave(&pctrl->lock, flags);
+	raw_spin_lock_irqsave(&chv_lock, flags);
 
 	if (chv_pad_locked(pctrl, offset)) {
 		value = readl(chv_padreg(pctrl, offset, CHV_PADCTRL0));
 		if (!(value & CHV_PADCTRL0_GPIOEN)) {
 			/* Locked so cannot enable */
-			raw_spin_unlock_irqrestore(&pctrl->lock, flags);
+			raw_spin_unlock_irqrestore(&chv_lock, flags);
 			return -EBUSY;
 		}
 	} else {
@@ -897,7 +906,7 @@ static int chv_gpio_request_enable(struct pinctrl_dev *pctldev,
 		chv_writel(value, reg);
 	}
 
-	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
+	raw_spin_unlock_irqrestore(&chv_lock, flags);
 
 	return 0;
 }
@@ -911,13 +920,13 @@ static void chv_gpio_disable_free(struct pinctrl_dev *pctldev,
 	void __iomem *reg;
 	u32 value;
 
-	raw_spin_lock_irqsave(&pctrl->lock, flags);
+	raw_spin_lock_irqsave(&chv_lock, flags);
 
 	reg = chv_padreg(pctrl, offset, CHV_PADCTRL0);
 	value = readl(reg) & ~CHV_PADCTRL0_GPIOEN;
 	chv_writel(value, reg);
 
-	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
+	raw_spin_unlock_irqrestore(&chv_lock, flags);
 }
 
 static int chv_gpio_set_direction(struct pinctrl_dev *pctldev,
@@ -929,7 +938,7 @@ static int chv_gpio_set_direction(struct pinctrl_dev *pctldev,
 	unsigned long flags;
 	u32 ctrl0;
 
-	raw_spin_lock_irqsave(&pctrl->lock, flags);
+	raw_spin_lock_irqsave(&chv_lock, flags);
 
 	ctrl0 = readl(reg) & ~CHV_PADCTRL0_GPIOCFG_MASK;
 	if (input)
@@ -938,7 +947,7 @@ static int chv_gpio_set_direction(struct pinctrl_dev *pctldev,
 		ctrl0 |= CHV_PADCTRL0_GPIOCFG_GPO << CHV_PADCTRL0_GPIOCFG_SHIFT;
 	chv_writel(ctrl0, reg);
 
-	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
+	raw_spin_unlock_irqrestore(&chv_lock, flags);
 
 	return 0;
 }
@@ -963,10 +972,10 @@ static int chv_config_get(struct pinctrl_dev *pctldev, unsigned pin,
 	u16 arg = 0;
 	u32 term;
 
-	raw_spin_lock_irqsave(&pctrl->lock, flags);
+	raw_spin_lock_irqsave(&chv_lock, flags);
 	ctrl0 = readl(chv_padreg(pctrl, pin, CHV_PADCTRL0));
 	ctrl1 = readl(chv_padreg(pctrl, pin, CHV_PADCTRL1));
-	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
+	raw_spin_unlock_irqrestore(&chv_lock, flags);
 
 	term = (ctrl0 & CHV_PADCTRL0_TERM_MASK) >> CHV_PADCTRL0_TERM_SHIFT;
 
@@ -1040,7 +1049,7 @@ static int chv_config_set_pull(struct chv_pinctrl *pctrl, unsigned pin,
 	unsigned long flags;
 	u32 ctrl0, pull;
 
-	raw_spin_lock_irqsave(&pctrl->lock, flags);
+	raw_spin_lock_irqsave(&chv_lock, flags);
 	ctrl0 = readl(reg);
 
 	switch (param) {
@@ -1063,7 +1072,7 @@ static int chv_config_set_pull(struct chv_pinctrl *pctrl, unsigned pin,
 			pull = CHV_PADCTRL0_TERM_20K << CHV_PADCTRL0_TERM_SHIFT;
 			break;
 		default:
-			raw_spin_unlock_irqrestore(&pctrl->lock, flags);
+			raw_spin_unlock_irqrestore(&chv_lock, flags);
 			return -EINVAL;
 		}
 
@@ -1081,7 +1090,7 @@ static int chv_config_set_pull(struct chv_pinctrl *pctrl, unsigned pin,
 			pull = CHV_PADCTRL0_TERM_20K << CHV_PADCTRL0_TERM_SHIFT;
 			break;
 		default:
-			raw_spin_unlock_irqrestore(&pctrl->lock, flags);
+			raw_spin_unlock_irqrestore(&chv_lock, flags);
 			return -EINVAL;
 		}
 
@@ -1089,12 +1098,12 @@ static int chv_config_set_pull(struct chv_pinctrl *pctrl, unsigned pin,
 		break;
 
 	default:
-		raw_spin_unlock_irqrestore(&pctrl->lock, flags);
+		raw_spin_unlock_irqrestore(&chv_lock, flags);
 		return -EINVAL;
 	}
 
 	chv_writel(ctrl0, reg);
-	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
+	raw_spin_unlock_irqrestore(&chv_lock, flags);
 
 	return 0;
 }
@@ -1160,9 +1169,9 @@ static int chv_gpio_get(struct gpio_chip *chip, unsigned offset)
 	unsigned long flags;
 	u32 ctrl0, cfg;
 
-	raw_spin_lock_irqsave(&pctrl->lock, flags);
+	raw_spin_lock_irqsave(&chv_lock, flags);
 	ctrl0 = readl(chv_padreg(pctrl, pin, CHV_PADCTRL0));
-	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
+	raw_spin_unlock_irqrestore(&chv_lock, flags);
 
 	cfg = ctrl0 & CHV_PADCTRL0_GPIOCFG_MASK;
 	cfg >>= CHV_PADCTRL0_GPIOCFG_SHIFT;
@@ -1180,7 +1189,7 @@ static void chv_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
 	void __iomem *reg;
 	u32 ctrl0;
 
-	raw_spin_lock_irqsave(&pctrl->lock, flags);
+	raw_spin_lock_irqsave(&chv_lock, flags);
 
 	reg = chv_padreg(pctrl, pin, CHV_PADCTRL0);
 	ctrl0 = readl(reg);
@@ -1192,7 +1201,7 @@ static void chv_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
 
 	chv_writel(ctrl0, reg);
 
-	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
+	raw_spin_unlock_irqrestore(&chv_lock, flags);
 }
 
 static int chv_gpio_get_direction(struct gpio_chip *chip, unsigned offset)
@@ -1202,9 +1211,9 @@ static int chv_gpio_get_direction(struct gpio_chip *chip, unsigned offset)
 	u32 ctrl0, direction;
 	unsigned long flags;
 
-	raw_spin_lock_irqsave(&pctrl->lock, flags);
+	raw_spin_lock_irqsave(&chv_lock, flags);
 	ctrl0 = readl(chv_padreg(pctrl, pin, CHV_PADCTRL0));
-	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
+	raw_spin_unlock_irqrestore(&chv_lock, flags);
 
 	direction = ctrl0 & CHV_PADCTRL0_GPIOCFG_MASK;
 	direction >>= CHV_PADCTRL0_GPIOCFG_SHIFT;
@@ -1242,14 +1251,14 @@ static void chv_gpio_irq_ack(struct irq_data *d)
 	int pin = chv_gpio_offset_to_pin(pctrl, irqd_to_hwirq(d));
 	u32 intr_line;
 
-	raw_spin_lock(&pctrl->lock);
+	raw_spin_lock(&chv_lock);
 
 	intr_line = readl(chv_padreg(pctrl, pin, CHV_PADCTRL0));
 	intr_line &= CHV_PADCTRL0_INTSEL_MASK;
 	intr_line >>= CHV_PADCTRL0_INTSEL_SHIFT;
 	chv_writel(BIT(intr_line), pctrl->regs + CHV_INTSTAT);
 
-	raw_spin_unlock(&pctrl->lock);
+	raw_spin_unlock(&chv_lock);
 }
 
 static void chv_gpio_irq_mask_unmask(struct irq_data *d, bool mask)
@@ -1260,7 +1269,7 @@ static void chv_gpio_irq_mask_unmask(struct irq_data *d, bool mask)
 	u32 value, intr_line;
 	unsigned long flags;
 
-	raw_spin_lock_irqsave(&pctrl->lock, flags);
+	raw_spin_lock_irqsave(&chv_lock, flags);
 
 	intr_line = readl(chv_padreg(pctrl, pin, CHV_PADCTRL0));
 	intr_line &= CHV_PADCTRL0_INTSEL_MASK;
@@ -1273,7 +1282,7 @@ static void chv_gpio_irq_mask_unmask(struct irq_data *d, bool mask)
 		value |= BIT(intr_line);
 	chv_writel(value, pctrl->regs + CHV_INTMASK);
 
-	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
+	raw_spin_unlock_irqrestore(&chv_lock, flags);
 }
 
 static void chv_gpio_irq_mask(struct irq_data *d)
@@ -1307,7 +1316,7 @@ static unsigned chv_gpio_irq_startup(struct irq_data *d)
 		unsigned long flags;
 		u32 intsel, value;
 
-		raw_spin_lock_irqsave(&pctrl->lock, flags);
+		raw_spin_lock_irqsave(&chv_lock, flags);
 		intsel = readl(chv_padreg(pctrl, pin, CHV_PADCTRL0));
 		intsel &= CHV_PADCTRL0_INTSEL_MASK;
 		intsel >>= CHV_PADCTRL0_INTSEL_SHIFT;
@@ -1322,7 +1331,7 @@ static unsigned chv_gpio_irq_startup(struct irq_data *d)
 			irq_set_handler_locked(d, handler);
 			pctrl->intr_lines[intsel] = offset;
 		}
-		raw_spin_unlock_irqrestore(&pctrl->lock, flags);
+		raw_spin_unlock_irqrestore(&chv_lock, flags);
 	}
 
 	chv_gpio_irq_unmask(d);
@@ -1338,7 +1347,7 @@ static int chv_gpio_irq_type(struct irq_data *d, unsigned type)
 	unsigned long flags;
 	u32 value;
 
-	raw_spin_lock_irqsave(&pctrl->lock, flags);
+	raw_spin_lock_irqsave(&chv_lock, flags);
 
 	/*
 	 * Pins which can be used as shared interrupt are configured in
@@ -1387,7 +1396,7 @@ static int chv_gpio_irq_type(struct irq_data *d, unsigned type)
 	else if (type & IRQ_TYPE_LEVEL_MASK)
 		irq_set_handler_locked(d, handle_level_irq);
 
-	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
+	raw_spin_unlock_irqrestore(&chv_lock, flags);
 
 	return 0;
 }
@@ -1499,7 +1508,6 @@ static int chv_pinctrl_probe(struct platform_device *pdev)
 	if (i == ARRAY_SIZE(chv_communities))
 		return -ENODEV;
 
-	raw_spin_lock_init(&pctrl->lock);
 	pctrl->dev = &pdev->dev;
 
 #ifdef CONFIG_PM_SLEEP
-- 
2.1.4

[PATCH v2 2/3] pinctrl: cherryview: add option to set open-drain pin config

[Dan O'Donovan]

On some CHV platforms, we need an option to configure the
open-drain setting for these pins.  This adds support for the
PIN_CONFIG_DRIVE_PUSH_PULL and PIN_CONFIG_DRIVE_OPEN_DRAIN to
disable/enable open-drain mode for a specific pin.

Signed-off-by: Dan O'Donovan <dan@emutex.com>
---
 drivers/pinctrl/intel/pinctrl-cherryview.c | 33 ++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/drivers/pinctrl/intel/pinctrl-cherryview.c b/drivers/pinctrl/intel/pinctrl-cherryview.c
index bf65c94..7b3f147 100644
--- a/drivers/pinctrl/intel/pinctrl-cherryview.c
+++ b/drivers/pinctrl/intel/pinctrl-cherryview.c
@@ -1108,6 +1108,27 @@ static int chv_config_set_pull(struct chv_pinctrl *pctrl, unsigned pin,
 	return 0;
 }
 
+static int chv_config_set_oden(struct chv_pinctrl *pctrl, unsigned int pin,
+			       bool enable)
+{
+	void __iomem *reg = chv_padreg(pctrl, pin, CHV_PADCTRL1);
+	unsigned long flags;
+	u32 ctrl1;
+
+	raw_spin_lock_irqsave(&chv_lock, flags);
+	ctrl1 = readl(reg);
+
+	if (enable)
+		ctrl1 |= CHV_PADCTRL1_ODEN;
+	else
+		ctrl1 &= ~CHV_PADCTRL1_ODEN;
+
+	chv_writel(ctrl1, reg);
+	raw_spin_unlock_irqrestore(&chv_lock, flags);
+
+	return 0;
+}
+
 static int chv_config_set(struct pinctrl_dev *pctldev, unsigned pin,
 			  unsigned long *configs, unsigned nconfigs)
 {
@@ -1132,6 +1153,18 @@ static int chv_config_set(struct pinctrl_dev *pctldev, unsigned pin,
 				return ret;
 			break;
 
+		case PIN_CONFIG_DRIVE_PUSH_PULL:
+			ret = chv_config_set_oden(pctrl, pin, false);
+			if (ret)
+				return ret;
+			break;
+
+		case PIN_CONFIG_DRIVE_OPEN_DRAIN:
+			ret = chv_config_set_oden(pctrl, pin, true);
+			if (ret)
+				return ret;
+			break;
+
 		default:
 			return -ENOTSUPP;
 		}
-- 
2.1.4

[PATCH v2 3/3] pinctrl: cherryview: add handlers for pin_config_group_get/set

[Dan O'Donovan]

Pin config get/set handlers for pin groups were previously not
implemented by this driver.  The pin_config_group_set is
particularly useful for applying a common config setting to all
pins in a specified group with a single call, without the caller
needing to reference each individual pin by name.

Signed-off-by: Dan O'Donovan <dan@emutex.com>
---
 drivers/pinctrl/intel/pinctrl-cherryview.c | 42 ++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/drivers/pinctrl/intel/pinctrl-cherryview.c b/drivers/pinctrl/intel/pinctrl-cherryview.c
index 7b3f147..5749a4ee 100644
--- a/drivers/pinctrl/intel/pinctrl-cherryview.c
+++ b/drivers/pinctrl/intel/pinctrl-cherryview.c
@@ -1176,10 +1176,52 @@ static int chv_config_set(struct pinctrl_dev *pctldev, unsigned pin,
 	return 0;
 }
 
+static int chv_config_group_get(struct pinctrl_dev *pctldev,
+				unsigned int group,
+				unsigned long *config)
+{
+	const unsigned int *pins;
+	unsigned int npins;
+	int ret;
+
+	ret = chv_get_group_pins(pctldev, group, &pins, &npins);
+	if (ret)
+		return ret;
+
+	ret = chv_config_get(pctldev, pins[0], config);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int chv_config_group_set(struct pinctrl_dev *pctldev,
+				unsigned int group, unsigned long *configs,
+				unsigned int num_configs)
+{
+	const unsigned int *pins;
+	unsigned int npins;
+	int i, ret;
+
+	ret = chv_get_group_pins(pctldev, group, &pins, &npins);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < npins; i++) {
+		ret = chv_config_set(pctldev, pins[i], configs, num_configs);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
 static const struct pinconf_ops chv_pinconf_ops = {
 	.is_generic = true,
 	.pin_config_set = chv_config_set,
 	.pin_config_get = chv_config_get,
+	.pin_config_group_get = chv_config_group_get,
+	.pin_config_group_set = chv_config_group_set,
 };
 
 static struct pinctrl_desc chv_pinctrl_desc = {
-- 
2.1.4

Re: [PATCH v2 0/3] pinctrl: cherryview: fixes and enhancements

[Mika Westerberg]

On Fri, Jun 10, 2016 at 01:23:33PM +0100, Dan O'Donovan wrote:
> This series includes a number of fixes and enhancements for the
> Cherryview pinctrl/gpio driver, developed during integration on the
> UP Board (based on the Intel X5-Z8350 "Cherry Trail" Atom SoC).
> 
> Patch 1 provides a workaround for a documented silicon bug which
> causes data corruption of concurrent accesses to the GPIO controller
> registers on this SoC.
> 
> The other patches implement additional pin config functions/options
> for this driver.
> 
> Changes since v1:
>  * Addressed review comments from Mika Westerberg:
>    - Drop clean-up of checkpatch warnings (impacts back-porting)
>    - Re-order series to put fixes ahead of enhancements
>    - Rename a variable
>    - Tag concurrency fix (patch 1) for stable
>    - Drop fix restoring PADCTRL1 bits when GPIO disabled (rejected)
> 
> Dan O'Donovan (3):
>   pinctrl: cherryview: prevent concurrent access to GPIO controllers
>   pinctrl: cherryview: add option to set open-drain pin config
>   pinctrl: cherryview: add handlers for pin_config_group_get/set

Looks good to me now, thanks.

For the whole series,

Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Re: [PATCH v2 1/3] pinctrl: cherryview: prevent concurrent access to GPIO controllers

[Linus Walleij]

On Fri, Jun 10, 2016 at 2:23 PM, Dan O'Donovan <dan@emutex.com> wrote:

> Due to a silicon issue on the Atom X5-Z8000 "Cherry Trail" processor
> series, a common lock must be used to prevent concurrent accesses
> across the 4 GPIO controllers managed by this driver.
>
> See Intel Atom Z8000 Processor Series Specification Update
> (Rev. 005), errata #CHT34, for further information.
>
> Signed-off-by: Dan O'Donovan <dan@emutex.com>
> Cc: stable <stable@vger.kernel.org>

Patch applied.

Does it *really* need to go into fixes right now or can it wait until
the next merge window?

Since the rest of the patches seem to depend on it it creates a problem
where some apply to the fixes branch etc and need special care, that
is why I'm asking. If it is super-important, I will fix it.

Yours,
Linus Walleij

Re: [PATCH v2 2/3] pinctrl: cherryview: add option to set open-drain pin config

[Linus Walleij]

On Fri, Jun 10, 2016 at 2:23 PM, Dan O'Donovan <dan@emutex.com> wrote:

> On some CHV platforms, we need an option to configure the
> open-drain setting for these pins.  This adds support for the
> PIN_CONFIG_DRIVE_PUSH_PULL and PIN_CONFIG_DRIVE_OPEN_DRAIN to
> disable/enable open-drain mode for a specific pin.
>
> Signed-off-by: Dan O'Donovan <dan@emutex.com>

Patch applied.

Yours,
Linus Walleij

Re: [PATCH v2 3/3] pinctrl: cherryview: add handlers for pin_config_group_get/set

[Linus Walleij]

On Fri, Jun 10, 2016 at 2:23 PM, Dan O'Donovan <dan@emutex.com> wrote:

> Pin config get/set handlers for pin groups were previously not
> implemented by this driver.  The pin_config_group_set is
> particularly useful for applying a common config setting to all
> pins in a specified group with a single call, without the caller
> needing to reference each individual pin by name.
>
> Signed-off-by: Dan O'Donovan <dan@emutex.com>

Patch applied.

Yours,
Linus Walleij

Re: [PATCH v2 1/3] pinctrl: cherryview: prevent concurrent access to GPIO controllers

[Dan O'Donovan]

On 06/13/2016 01:23 PM, Linus Walleij wrote:
> On Fri, Jun 10, 2016 at 2:23 PM, Dan O'Donovan <dan@emutex.com> wrote:
>
>> Due to a silicon issue on the Atom X5-Z8000 "Cherry Trail" processor
>> series, a common lock must be used to prevent concurrent accesses
>> across the 4 GPIO controllers managed by this driver.
>>
>> See Intel Atom Z8000 Processor Series Specification Update
>> (Rev. 005), errata #CHT34, for further information.
>>
>> Signed-off-by: Dan O'Donovan <dan@emutex.com>
>> Cc: stable <stable@vger.kernel.org>
> Patch applied.
>
> Does it *really* need to go into fixes right now or can it wait until
> the next merge window?
I have no objection to waiting until the next merge window for this.  Thanks Linus.
>
> Since the rest of the patches seem to depend on it it creates a problem
> where some apply to the fixes branch etc and need special care, that
> is why I'm asking. If it is super-important, I will fix it.
>
> Yours,
> Linus Walleij