[PATCH v2 1/4] pinctrl: rockchip: Set wake_enabled

[PATCH v2 1/4] pinctrl: rockchip: Set wake_enabled

[Doug Anderson]

The rockchip pinctrl driver uses irq_gc_set_wake() but doesn't setup
the .wake_enabled member.  That means that we can never actually use a
pin for wakeup.  When "irq_set_irq_wake()" tries to call through it
will always get a failure from set_irq_wake_real() and will then set
wake_depth to 0.  Assuming you can resume you'll later get an error
message about "Unbalanced IRQ x wake disable".

Signed-off-by: Doug Anderson <dianders@chromium.org>
Tested-by: Chris Zhong <zyw@rock-chips.com>
---
Changes in v2: None

 drivers/pinctrl/pinctrl-rockchip.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c
index 016f457..230d8f3 100644
--- a/drivers/pinctrl/pinctrl-rockchip.c
+++ b/drivers/pinctrl/pinctrl-rockchip.c
@@ -1563,6 +1563,7 @@ static int rockchip_interrupts_register(struct platform_device *pdev,
 		gc->chip_types[0].chip.irq_unmask = irq_gc_mask_set_bit;
 		gc->chip_types[0].chip.irq_set_wake = irq_gc_set_wake;
 		gc->chip_types[0].chip.irq_set_type = rockchip_irq_set_type;
+		gc->wake_enabled = IRQ_MSK(bank->nr_pins);
 
 		irq_set_handler_data(bank->irq, bank);
 		irq_set_chained_handler(bank->irq, rockchip_irq_demux);
-- 
2.1.0.rc2.206.gedb03e5

[PATCH v2 2/4] pinctrl: rockchip: Don't call pinctrl_gpio_direction_output() in pin_config_set()

[Doug Anderson]

The Rockchip pinctrl driver was calling
rockchip_gpio_direction_output() in the pin_config_set() callback.
This was just a shortcut for:
* rockchip_gpio_set()
* pinctrl_gpio_direction_output()

Unfortunately it's not so good to call pinctrl_gpio_direction_output()
from pin_config_set().  Specifically when initting hogs you'll get an
error.

Let's refactor a little so we can call
_rockchip_pmx_gpio_set_direction() directly.

Signed-off-by: Doug Anderson <dianders@chromium.org>
Tested-by: Chris Zhong <zyw@rock-chips.com>
---
Changes in v2:
- The 'flags' variable is now in the locking patch.
- Fix confusion between 'pin' and 'offset'.

 drivers/pinctrl/pinctrl-rockchip.c | 41 +++++++++++++++++++++++---------------
 1 file changed, 25 insertions(+), 16 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c
index 230d8f3..8a3c582 100644
--- a/drivers/pinctrl/pinctrl-rockchip.c
+++ b/drivers/pinctrl/pinctrl-rockchip.c
@@ -856,22 +856,14 @@ static int rockchip_pmx_set(struct pinctrl_dev *pctldev, unsigned selector,
  * leads to this function call (via the pinctrl_gpio_direction_{input|output}()
  * function called from the gpiolib interface).
  */
-static int rockchip_pmx_gpio_set_direction(struct pinctrl_dev *pctldev,
-					      struct pinctrl_gpio_range *range,
-					      unsigned offset, bool input)
+static int _rockchip_pmx_gpio_set_direction(struct gpio_chip *chip,
+					    int pin, bool input)
 {
-	struct rockchip_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
 	struct rockchip_pin_bank *bank;
-	struct gpio_chip *chip;
-	int pin, ret;
+	int ret;
 	u32 data;
 
-	chip = range->gc;
 	bank = gc_to_pin_bank(chip);
-	pin = offset - chip->base;
-
-	dev_dbg(info->dev, "gpio_direction for pin %u as %s-%d to %s\n",
-		 offset, range->name, pin, input ? "input" : "output");
 
 	ret = rockchip_set_mux(bank, pin, RK_FUNC_GPIO);
 	if (ret < 0)
@@ -888,6 +880,23 @@ static int rockchip_pmx_gpio_set_direction(struct pinctrl_dev *pctldev,
 	return 0;
 }
 
+static int rockchip_pmx_gpio_set_direction(struct pinctrl_dev *pctldev,
+					      struct pinctrl_gpio_range *range,
+					      unsigned offset, bool input)
+{
+	struct rockchip_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
+	struct gpio_chip *chip;
+	int pin;
+
+	chip = range->gc;
+	pin = offset - chip->base;
+	dev_dbg(info->dev, "gpio_direction for pin %u as %s-%d to %s\n",
+		 offset, range->name, pin, input ? "input" : "output");
+
+	return _rockchip_pmx_gpio_set_direction(chip, offset - chip->base,
+						input);
+}
+
 static const struct pinmux_ops rockchip_pmx_ops = {
 	.get_functions_count	= rockchip_pmx_get_funcs_count,
 	.get_function_name	= rockchip_pmx_get_func_name,
@@ -917,8 +926,7 @@ static bool rockchip_pinconf_pull_valid(struct rockchip_pin_ctrl *ctrl,
 	return false;
 }
 
-static int rockchip_gpio_direction_output(struct gpio_chip *gc,
-					  unsigned offset, int value);
+static void rockchip_gpio_set(struct gpio_chip *gc, unsigned offset, int value);
 static int rockchip_gpio_get(struct gpio_chip *gc, unsigned offset);
 
 /* set the pin config settings for a specified pin */
@@ -959,9 +967,10 @@ static int rockchip_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
 				return rc;
 			break;
 		case PIN_CONFIG_OUTPUT:
-			rc = rockchip_gpio_direction_output(&bank->gpio_chip,
-							    pin - bank->pin_base,
-							    arg);
+			rockchip_gpio_set(&bank->gpio_chip,
+					  pin - bank->pin_base, arg);
+			rc = _rockchip_pmx_gpio_set_direction(&bank->gpio_chip,
+					  pin - bank->pin_base, false);
 			if (rc)
 				return rc;
 			break;
-- 
2.1.0.rc2.206.gedb03e5

[PATCH v2 3/4] pinctrl: rockchip: Parse pin groups before calling pinctrl_register()

[Doug Anderson]

Just like in (529301c pinctrl: samsung: Parse pin groups before
calling pinctrl_register()), Rockchip also needs to parse pin groups
earlier to make hogs work.

Signed-off-by: Doug Anderson <dianders@chromium.org>
Tested-by: Chris Zhong <zyw@rock-chips.com>
---
Changes in v2: None

 drivers/pinctrl/pinctrl-rockchip.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c
index 8a3c582..6c14f6c 100644
--- a/drivers/pinctrl/pinctrl-rockchip.c
+++ b/drivers/pinctrl/pinctrl-rockchip.c
@@ -1262,6 +1262,10 @@ static int rockchip_pinctrl_register(struct platform_device *pdev,
 		}
 	}
 
+	ret = rockchip_pinctrl_parse_dt(pdev, info);
+	if (ret)
+		return ret;
+
 	info->pctl_dev = pinctrl_register(ctrldesc, &pdev->dev, info);
 	if (!info->pctl_dev) {
 		dev_err(&pdev->dev, "could not register pinctrl driver\n");
@@ -1279,12 +1283,6 @@ static int rockchip_pinctrl_register(struct platform_device *pdev,
 		pinctrl_add_gpio_range(info->pctl_dev, &pin_bank->grange);
 	}
 
-	ret = rockchip_pinctrl_parse_dt(pdev, info);
-	if (ret) {
-		pinctrl_unregister(info->pctl_dev);
-		return ret;
-	}
-
 	return 0;
 }
 
-- 
2.1.0.rc2.206.gedb03e5

[PATCH v2 4/4] pinctrl: rockchip: Protect read-modify-write with the spinlock

[Doug Anderson]

There were a few instances where the rockchip pinctrl driver would do
read-modify-write with no spinlock.  Add a spinlock for these cases.

Signed-off-by: Doug Anderson <dianders@chromium.org>
---
Changes in v2:
- Made sure to release the lock in an error condition.

 drivers/pinctrl/pinctrl-rockchip.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c
index 6c14f6c..59a5461 100644
--- a/drivers/pinctrl/pinctrl-rockchip.c
+++ b/drivers/pinctrl/pinctrl-rockchip.c
@@ -861,6 +861,7 @@ static int _rockchip_pmx_gpio_set_direction(struct gpio_chip *chip,
 {
 	struct rockchip_pin_bank *bank;
 	int ret;
+	unsigned long flags;
 	u32 data;
 
 	bank = gc_to_pin_bank(chip);
@@ -869,6 +870,8 @@ static int _rockchip_pmx_gpio_set_direction(struct gpio_chip *chip,
 	if (ret < 0)
 		return ret;
 
+	spin_lock_irqsave(&bank->slock, flags);
+
 	data = readl_relaxed(bank->reg_base + GPIO_SWPORT_DDR);
 	/* set bit to 1 for output, 0 for input */
 	if (!input)
@@ -877,6 +880,8 @@ static int _rockchip_pmx_gpio_set_direction(struct gpio_chip *chip,
 		data &= ~BIT(pin);
 	writel_relaxed(data, bank->reg_base + GPIO_SWPORT_DDR);
 
+	spin_unlock_irqrestore(&bank->slock, flags);
+
 	return 0;
 }
 
@@ -1394,6 +1399,7 @@ static void rockchip_irq_demux(unsigned int irq, struct irq_desc *desc)
 	u32 polarity = 0, data = 0;
 	u32 pend;
 	bool edge_changed = false;
+	unsigned long flags;
 
 	dev_dbg(bank->drvdata->dev, "got irq for bank %s\n", bank->name);
 
@@ -1439,10 +1445,14 @@ static void rockchip_irq_demux(unsigned int irq, struct irq_desc *desc)
 
 	if (bank->toggle_edge_mode && edge_changed) {
 		/* Interrupt params should only be set with ints disabled */
+		spin_lock_irqsave(&bank->slock, flags);
+
 		data = readl_relaxed(bank->reg_base + GPIO_INTEN);
 		writel_relaxed(0, bank->reg_base + GPIO_INTEN);
 		writel(polarity, bank->reg_base + GPIO_INT_POLARITY);
 		writel(data, bank->reg_base + GPIO_INTEN);
+
+		spin_unlock_irqrestore(&bank->slock, flags);
 	}
 
 	chained_irq_exit(chip, desc);
@@ -1456,6 +1466,7 @@ static int rockchip_irq_set_type(struct irq_data *d, unsigned int type)
 	u32 polarity;
 	u32 level;
 	u32 data;
+	unsigned long flags;
 	int ret;
 
 	/* make sure the pin is configured as gpio input */
@@ -1463,15 +1474,20 @@ static int rockchip_irq_set_type(struct irq_data *d, unsigned int type)
 	if (ret < 0)
 		return ret;
 
+	spin_lock_irqsave(&bank->slock, flags);
+
 	data = readl_relaxed(bank->reg_base + GPIO_SWPORT_DDR);
 	data &= ~mask;
 	writel_relaxed(data, bank->reg_base + GPIO_SWPORT_DDR);
 
+	spin_unlock_irqrestore(&bank->slock, flags);
+
 	if (type & IRQ_TYPE_EDGE_BOTH)
 		__irq_set_handler_locked(d->irq, handle_edge_irq);
 	else
 		__irq_set_handler_locked(d->irq, handle_level_irq);
 
+	spin_lock_irqsave(&bank->slock, flags);
 	irq_gc_lock(gc);
 
 	level = readl_relaxed(gc->reg_base + GPIO_INTTYPE_LEVEL);
@@ -1514,6 +1530,7 @@ static int rockchip_irq_set_type(struct irq_data *d, unsigned int type)
 		break;
 	default:
 		irq_gc_unlock(gc);
+		spin_unlock_irqrestore(&bank->slock, flags);
 		return -EINVAL;
 	}
 
@@ -1521,6 +1538,7 @@ static int rockchip_irq_set_type(struct irq_data *d, unsigned int type)
 	writel_relaxed(polarity, gc->reg_base + GPIO_INT_POLARITY);
 
 	irq_gc_unlock(gc);
+	spin_unlock_irqrestore(&bank->slock, flags);
 
 	return 0;
 }
-- 
2.1.0.rc2.206.gedb03e5

[PATCH v2 3/4] pinctrl: rockchip: Parse pin groups before calling pinctrl_register()

[Heiko Stübner]

Am Dienstag, 21. Oktober 2014, 10:47:34 schrieb Doug Anderson:
> Just like in (529301c pinctrl: samsung: Parse pin groups before
> calling pinctrl_register()), Rockchip also needs to parse pin groups
> earlier to make hogs work.
> 
> Signed-off-by: Doug Anderson <dianders@chromium.org>
> Tested-by: Chris Zhong <zyw@rock-chips.com>

Reviewed-by: Heiko Stuebner <heiko@sntech.de>

> ---
> Changes in v2: None
> 
>  drivers/pinctrl/pinctrl-rockchip.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pinctrl/pinctrl-rockchip.c
> b/drivers/pinctrl/pinctrl-rockchip.c index 8a3c582..6c14f6c 100644
> --- a/drivers/pinctrl/pinctrl-rockchip.c
> +++ b/drivers/pinctrl/pinctrl-rockchip.c
> @@ -1262,6 +1262,10 @@ static int rockchip_pinctrl_register(struct
> platform_device *pdev, }
>  	}
> 
> +	ret = rockchip_pinctrl_parse_dt(pdev, info);
> +	if (ret)
> +		return ret;
> +
>  	info->pctl_dev = pinctrl_register(ctrldesc, &pdev->dev, info);
>  	if (!info->pctl_dev) {
>  		dev_err(&pdev->dev, "could not register pinctrl driver\n");
> @@ -1279,12 +1283,6 @@ static int rockchip_pinctrl_register(struct
> platform_device *pdev, pinctrl_add_gpio_range(info->pctl_dev,
> &pin_bank->grange);
>  	}
> 
> -	ret = rockchip_pinctrl_parse_dt(pdev, info);
> -	if (ret) {
> -		pinctrl_unregister(info->pctl_dev);
> -		return ret;
> -	}
> -
>  	return 0;
>  }

[PATCH v2 4/4] pinctrl: rockchip: Protect read-modify-write with the spinlock

[Heiko Stübner]

Am Dienstag, 21. Oktober 2014, 10:47:35 schrieb Doug Anderson:
> There were a few instances where the rockchip pinctrl driver would do
> read-modify-write with no spinlock.  Add a spinlock for these cases.
> 
> Signed-off-by: Doug Anderson <dianders@chromium.org>

Reviewed-by: Heiko Stuebner <heiko@sntech.de>

thanks for catching those

> ---
> Changes in v2:
> - Made sure to release the lock in an error condition.
> 
>  drivers/pinctrl/pinctrl-rockchip.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/pinctrl/pinctrl-rockchip.c
> b/drivers/pinctrl/pinctrl-rockchip.c index 6c14f6c..59a5461 100644
> --- a/drivers/pinctrl/pinctrl-rockchip.c
> +++ b/drivers/pinctrl/pinctrl-rockchip.c
> @@ -861,6 +861,7 @@ static int _rockchip_pmx_gpio_set_direction(struct
> gpio_chip *chip, {
>  	struct rockchip_pin_bank *bank;
>  	int ret;
> +	unsigned long flags;
>  	u32 data;
> 
>  	bank = gc_to_pin_bank(chip);
> @@ -869,6 +870,8 @@ static int _rockchip_pmx_gpio_set_direction(struct
> gpio_chip *chip, if (ret < 0)
>  		return ret;
> 
> +	spin_lock_irqsave(&bank->slock, flags);
> +
>  	data = readl_relaxed(bank->reg_base + GPIO_SWPORT_DDR);
>  	/* set bit to 1 for output, 0 for input */
>  	if (!input)
> @@ -877,6 +880,8 @@ static int _rockchip_pmx_gpio_set_direction(struct
> gpio_chip *chip, data &= ~BIT(pin);
>  	writel_relaxed(data, bank->reg_base + GPIO_SWPORT_DDR);
> 
> +	spin_unlock_irqrestore(&bank->slock, flags);
> +
>  	return 0;
>  }
> 
> @@ -1394,6 +1399,7 @@ static void rockchip_irq_demux(unsigned int irq,
> struct irq_desc *desc) u32 polarity = 0, data = 0;
>  	u32 pend;
>  	bool edge_changed = false;
> +	unsigned long flags;
> 
>  	dev_dbg(bank->drvdata->dev, "got irq for bank %s\n", bank->name);
> 
> @@ -1439,10 +1445,14 @@ static void rockchip_irq_demux(unsigned int irq,
> struct irq_desc *desc)
> 
>  	if (bank->toggle_edge_mode && edge_changed) {
>  		/* Interrupt params should only be set with ints disabled */
> +		spin_lock_irqsave(&bank->slock, flags);
> +
>  		data = readl_relaxed(bank->reg_base + GPIO_INTEN);
>  		writel_relaxed(0, bank->reg_base + GPIO_INTEN);
>  		writel(polarity, bank->reg_base + GPIO_INT_POLARITY);
>  		writel(data, bank->reg_base + GPIO_INTEN);
> +
> +		spin_unlock_irqrestore(&bank->slock, flags);
>  	}
> 
>  	chained_irq_exit(chip, desc);
> @@ -1456,6 +1466,7 @@ static int rockchip_irq_set_type(struct irq_data *d,
> unsigned int type) u32 polarity;
>  	u32 level;
>  	u32 data;
> +	unsigned long flags;
>  	int ret;
> 
>  	/* make sure the pin is configured as gpio input */
> @@ -1463,15 +1474,20 @@ static int rockchip_irq_set_type(struct irq_data *d,
> unsigned int type) if (ret < 0)
>  		return ret;
> 
> +	spin_lock_irqsave(&bank->slock, flags);
> +
>  	data = readl_relaxed(bank->reg_base + GPIO_SWPORT_DDR);
>  	data &= ~mask;
>  	writel_relaxed(data, bank->reg_base + GPIO_SWPORT_DDR);
> 
> +	spin_unlock_irqrestore(&bank->slock, flags);
> +
>  	if (type & IRQ_TYPE_EDGE_BOTH)
>  		__irq_set_handler_locked(d->irq, handle_edge_irq);
>  	else
>  		__irq_set_handler_locked(d->irq, handle_level_irq);
> 
> +	spin_lock_irqsave(&bank->slock, flags);
>  	irq_gc_lock(gc);
> 
>  	level = readl_relaxed(gc->reg_base + GPIO_INTTYPE_LEVEL);
> @@ -1514,6 +1530,7 @@ static int rockchip_irq_set_type(struct irq_data *d,
> unsigned int type) break;
>  	default:
>  		irq_gc_unlock(gc);
> +		spin_unlock_irqrestore(&bank->slock, flags);
>  		return -EINVAL;
>  	}
> 
> @@ -1521,6 +1538,7 @@ static int rockchip_irq_set_type(struct irq_data *d,
> unsigned int type) writel_relaxed(polarity, gc->reg_base +
> GPIO_INT_POLARITY);
> 
>  	irq_gc_unlock(gc);
> +	spin_unlock_irqrestore(&bank->slock, flags);
> 
>  	return 0;
>  }

[PATCH v2 2/4] pinctrl: rockchip: Don't call pinctrl_gpio_direction_output() in pin_config_set()

[Heiko Stübner]

Am Dienstag, 21. Oktober 2014, 10:47:33 schrieb Doug Anderson:
> The Rockchip pinctrl driver was calling
> rockchip_gpio_direction_output() in the pin_config_set() callback.
> This was just a shortcut for:
> * rockchip_gpio_set()
> * pinctrl_gpio_direction_output()
> 
> Unfortunately it's not so good to call pinctrl_gpio_direction_output()
> from pin_config_set().  Specifically when initting hogs you'll get an
> error.
> 
> Let's refactor a little so we can call
> _rockchip_pmx_gpio_set_direction() directly.
> 
> Signed-off-by: Doug Anderson <dianders@chromium.org>
> Tested-by: Chris Zhong <zyw@rock-chips.com>

Reviewed-by: Heiko Stuebner <heiko@sntech.de>

> ---
> Changes in v2:
> - The 'flags' variable is now in the locking patch.
> - Fix confusion between 'pin' and 'offset'.
> 
>  drivers/pinctrl/pinctrl-rockchip.c | 41
> +++++++++++++++++++++++--------------- 1 file changed, 25 insertions(+), 16
> deletions(-)
> 
> diff --git a/drivers/pinctrl/pinctrl-rockchip.c
> b/drivers/pinctrl/pinctrl-rockchip.c index 230d8f3..8a3c582 100644
> --- a/drivers/pinctrl/pinctrl-rockchip.c
> +++ b/drivers/pinctrl/pinctrl-rockchip.c
> @@ -856,22 +856,14 @@ static int rockchip_pmx_set(struct pinctrl_dev
> *pctldev, unsigned selector, * leads to this function call (via the
> pinctrl_gpio_direction_{input|output}() * function called from the gpiolib
> interface).
>   */
> -static int rockchip_pmx_gpio_set_direction(struct pinctrl_dev *pctldev,
> -					      struct pinctrl_gpio_range *range,
> -					      unsigned offset, bool input)
> +static int _rockchip_pmx_gpio_set_direction(struct gpio_chip *chip,
> +					    int pin, bool input)
>  {
> -	struct rockchip_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
>  	struct rockchip_pin_bank *bank;
> -	struct gpio_chip *chip;
> -	int pin, ret;
> +	int ret;
>  	u32 data;
> 
> -	chip = range->gc;
>  	bank = gc_to_pin_bank(chip);
> -	pin = offset - chip->base;
> -
> -	dev_dbg(info->dev, "gpio_direction for pin %u as %s-%d to %s\n",
> -		 offset, range->name, pin, input ? "input" : "output");
> 
>  	ret = rockchip_set_mux(bank, pin, RK_FUNC_GPIO);
>  	if (ret < 0)
> @@ -888,6 +880,23 @@ static int rockchip_pmx_gpio_set_direction(struct
> pinctrl_dev *pctldev, return 0;
>  }
> 
> +static int rockchip_pmx_gpio_set_direction(struct pinctrl_dev *pctldev,
> +					      struct pinctrl_gpio_range *range,
> +					      unsigned offset, bool input)
> +{
> +	struct rockchip_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
> +	struct gpio_chip *chip;
> +	int pin;
> +
> +	chip = range->gc;
> +	pin = offset - chip->base;
> +	dev_dbg(info->dev, "gpio_direction for pin %u as %s-%d to %s\n",
> +		 offset, range->name, pin, input ? "input" : "output");
> +
> +	return _rockchip_pmx_gpio_set_direction(chip, offset - chip->base,
> +						input);
> +}
> +
>  static const struct pinmux_ops rockchip_pmx_ops = {
>  	.get_functions_count	= rockchip_pmx_get_funcs_count,
>  	.get_function_name	= rockchip_pmx_get_func_name,
> @@ -917,8 +926,7 @@ static bool rockchip_pinconf_pull_valid(struct
> rockchip_pin_ctrl *ctrl, return false;
>  }
> 
> -static int rockchip_gpio_direction_output(struct gpio_chip *gc,
> -					  unsigned offset, int value);
> +static void rockchip_gpio_set(struct gpio_chip *gc, unsigned offset, int
> value); static int rockchip_gpio_get(struct gpio_chip *gc, unsigned
> offset);
> 
>  /* set the pin config settings for a specified pin */
> @@ -959,9 +967,10 @@ static int rockchip_pinconf_set(struct pinctrl_dev
> *pctldev, unsigned int pin, return rc;
>  			break;
>  		case PIN_CONFIG_OUTPUT:
> -			rc = rockchip_gpio_direction_output(&bank->gpio_chip,
> -							    pin - bank->pin_base,
> -							    arg);
> +			rockchip_gpio_set(&bank->gpio_chip,
> +					  pin - bank->pin_base, arg);
> +			rc = _rockchip_pmx_gpio_set_direction(&bank->gpio_chip,
> +					  pin - bank->pin_base, false);
>  			if (rc)
>  				return rc;
>  			break;

[PATCH v2 1/4] pinctrl: rockchip: Set wake_enabled

[Heiko Stübner]

Am Dienstag, 21. Oktober 2014, 10:47:32 schrieb Doug Anderson:
> The rockchip pinctrl driver uses irq_gc_set_wake() but doesn't setup
> the .wake_enabled member.  That means that we can never actually use a
> pin for wakeup.  When "irq_set_irq_wake()" tries to call through it
> will always get a failure from set_irq_wake_real() and will then set
> wake_depth to 0.  Assuming you can resume you'll later get an error
> message about "Unbalanced IRQ x wake disable".

The change itself looks reasonable. But now being able to read the docs for 
it, it doesn't look like all gpios are able to wake the system.

On the rk3288 it seems to be only the pins from gpio0 that can do this 
(similar for different banks on the other Rockchip SoCs) - see PMU_WAKEUP_CFG0 
and PMU_WAKEUP_CFG1[1].

So I guess we'll need something more eloquent to handle this.


Heiko

> 
> Signed-off-by: Doug Anderson <dianders@chromium.org>
> Tested-by: Chris Zhong <zyw@rock-chips.com>
> ---
> Changes in v2: None
> 
>  drivers/pinctrl/pinctrl-rockchip.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/pinctrl/pinctrl-rockchip.c
> b/drivers/pinctrl/pinctrl-rockchip.c index 016f457..230d8f3 100644
> --- a/drivers/pinctrl/pinctrl-rockchip.c
> +++ b/drivers/pinctrl/pinctrl-rockchip.c
> @@ -1563,6 +1563,7 @@ static int rockchip_interrupts_register(struct
> platform_device *pdev, gc->chip_types[0].chip.irq_unmask =
> irq_gc_mask_set_bit;
>  		gc->chip_types[0].chip.irq_set_wake = irq_gc_set_wake;
>  		gc->chip_types[0].chip.irq_set_type = rockchip_irq_set_type;
> +		gc->wake_enabled = IRQ_MSK(bank->nr_pins);
> 
>  		irq_set_handler_data(bank->irq, bank);
>  		irq_set_chained_handler(bank->irq, rockchip_irq_demux);

[PATCH v2 1/4] pinctrl: rockchip: Set wake_enabled

[Doug Anderson]

Heiko,

On Thu, Oct 23, 2014 at 9:43 AM, Heiko St?bner <heiko@sntech.de> wrote:
> Am Dienstag, 21. Oktober 2014, 10:47:32 schrieb Doug Anderson:
>> The rockchip pinctrl driver uses irq_gc_set_wake() but doesn't setup
>> the .wake_enabled member.  That means that we can never actually use a
>> pin for wakeup.  When "irq_set_irq_wake()" tries to call through it
>> will always get a failure from set_irq_wake_real() and will then set
>> wake_depth to 0.  Assuming you can resume you'll later get an error
>> message about "Unbalanced IRQ x wake disable".
>
> The change itself looks reasonable. But now being able to read the docs for
> it, it doesn't look like all gpios are able to wake the system.
>
> On the rk3288 it seems to be only the pins from gpio0 that can do this
> (similar for different banks on the other Rockchip SoCs) - see PMU_WAKEUP_CFG0
> and PMU_WAKEUP_CFG1[1].
>
> So I guess we'll need something more eloquent to handle this.

I think long term we're going to need something more elegant, yes.
...but it turns out that as long as you're not in the low, low power
state that all pins can wake up the system.

Check out "Table 4-5 Power Domain Status Summary in all Work Mode".
In Mode 3 (called "sleep") all GPIOs can wake the system up.  This is
the mode that Chris's current suspend/resume patch uses (actually, it
doesn't quite get all the way to that mode yet, but that's the
target).  It would be ideal if we could get to Mode 4 (called
"poweroff"), but when I talked to Rockchip they were a little hesitant
about promising that it would work.

For one thing: I'm also not 100% certain that today's boards are
designed to handle Mode 4.  You need to make sure that anything
connected to a pin other that GPIO0 can handle whatever state the CPU
leaves those pins in mode 4.  You also need to make extra certain that
all wakeup pins are on GPIO0 (wasn't the case with the first board I
worked with).

Also: I think you're supposed to turn off VDD_LOG in mode 4.  See the
line that says "The power off mode turns off the power of all VD_LOGIC
externally."  On the schematic I'm looking at right now (based on
rk808 EVB) I think there is no way to turn off VDD_LOG (without
killing RAM).


...my thought was to take this simple patch and eventually work something out.


NOTE: One unresolved thing with the current series (this series +
Chris's) is that pretty much any interrupt can wake up the system.
Even typing on the UART seems to do it.  Somehow we're not masking
interrupts in a way that prevents this, but I haven't tracked it down
yet.  I don't think it's related to this patch.

[PATCH v2 1/4] pinctrl: rockchip: Set wake_enabled

[Heiko Stübner]

Am Donnerstag, 23. Oktober 2014, 09:55:27 schrieb Doug Anderson:
> Heiko,
> 
> On Thu, Oct 23, 2014 at 9:43 AM, Heiko St?bner <heiko@sntech.de> wrote:
> > Am Dienstag, 21. Oktober 2014, 10:47:32 schrieb Doug Anderson:
> >> The rockchip pinctrl driver uses irq_gc_set_wake() but doesn't setup
> >> the .wake_enabled member.  That means that we can never actually use a
> >> pin for wakeup.  When "irq_set_irq_wake()" tries to call through it
> >> will always get a failure from set_irq_wake_real() and will then set
> >> wake_depth to 0.  Assuming you can resume you'll later get an error
> >> message about "Unbalanced IRQ x wake disable".
> > 
> > The change itself looks reasonable. But now being able to read the docs
> > for
> > it, it doesn't look like all gpios are able to wake the system.
> > 
> > On the rk3288 it seems to be only the pins from gpio0 that can do this
> > (similar for different banks on the other Rockchip SoCs) - see
> > PMU_WAKEUP_CFG0 and PMU_WAKEUP_CFG1[1].
> > 
> > So I guess we'll need something more eloquent to handle this.
> 
> I think long term we're going to need something more elegant, yes.
> ...but it turns out that as long as you're not in the low, low power
> state that all pins can wake up the system.
> 
> Check out "Table 4-5 Power Domain Status Summary in all Work Mode".
> In Mode 3 (called "sleep") all GPIOs can wake the system up.  This is
> the mode that Chris's current suspend/resume patch uses (actually, it
> doesn't quite get all the way to that mode yet, but that's the
> target).  It would be ideal if we could get to Mode 4 (called
> "poweroff"), but when I talked to Rockchip they were a little hesitant
> about promising that it would work.

You're right ... didn't read far enough it seems, so this patch also

Reviewed-by: Heiko Stuebner <heiko@sntech.de>



> NOTE: One unresolved thing with the current series (this series +
> Chris's) is that pretty much any interrupt can wake up the system.
> Even typing on the UART seems to do it.  Somehow we're not masking
> interrupts in a way that prevents this, but I haven't tracked it down
> yet.  I don't think it's related to this patch.

I guess the interrupts that aren't wakeup sources should then get masked when 
going to sleep?




Heiko

[PATCH v2 3/4] pinctrl: rockchip: Parse pin groups before calling pinctrl_register()

[Linus Walleij]

On Thu, Oct 23, 2014 at 6:26 PM, Heiko St?bner <heiko@sntech.de> wrote:
> Am Dienstag, 21. Oktober 2014, 10:47:34 schrieb Doug Anderson:
>> Just like in (529301c pinctrl: samsung: Parse pin groups before
>> calling pinctrl_register()), Rockchip also needs to parse pin groups
>> earlier to make hogs work.
>>
>> Signed-off-by: Doug Anderson <dianders@chromium.org>
>> Tested-by: Chris Zhong <zyw@rock-chips.com>
>
> Reviewed-by: Heiko Stuebner <heiko@sntech.de>

Heiko, can you collect this for a pull request?

Yours,
Linus Walleij

[PATCH v2 1/4] pinctrl: rockchip: Set wake_enabled

[Heiko Stübner]

Am Dienstag, 21. Oktober 2014, 10:47:32 schrieb Doug Anderson:
> The rockchip pinctrl driver uses irq_gc_set_wake() but doesn't setup
> the .wake_enabled member.  That means that we can never actually use a
> pin for wakeup.  When "irq_set_irq_wake()" tries to call through it
> will always get a failure from set_irq_wake_real() and will then set
> wake_depth to 0.  Assuming you can resume you'll later get an error
> message about "Unbalanced IRQ x wake disable".
> 
> Signed-off-by: Doug Anderson <dianders@chromium.org>
> Tested-by: Chris Zhong <zyw@rock-chips.com>

I've added all 4 patches to a v3.19-pinctrl/next branch.

As I'm still looking at the suspend pinctrl changes I'll wait for this before 
I send this to LinusW.


Heiko