[RFC PATCH 0/4] pinctrl: rockchip: PREEMPT_RT_FULL fixes

[RFC PATCH 0/4] pinctrl: rockchip: PREEMPT_RT_FULL fixes

[John Keeping]

As described in Documentation/gpio/driver.txt, we should not be using
sleepable APIs in the irqchip implementation.  Since this includes the
regmap API, this patch series ends up moving the mux setup for IRQs into
an irq_bus_sync_unlock() handler which may result in the IRQ being
configured before the port has been muxed as a GPIO.

I've marked the series as RFC because I'm not sure if this is the best
way to accomplish this or if there is another approach that is cleaner.
Also, the first patch may not be correct on RK3399 because I originally
wrote the patch for RK3288 on top of v4.4 where all drive updates only
affect a single register.  We don't need locking in this case because
regmap_update_bits() takes a lock on the regmap internally, but if these
two registers need to be updated atomically then another lock will
be required here - slock cannot be used if it is converted to a raw
spinlock since with full RT preemption the regmap's spinlock may sleep.

John Keeping (4):
  pinctrl: rockchip: remove unnecessary locking
  pinctrl: rockchip: convert to raw spinlock
  pinctrl: rockchip: split out verification of mux settings
  pinctrl: rockchip: avoid hardirq-unsafe functions in irq_chip

 drivers/pinctrl/pinctrl-rockchip.c | 140 +++++++++++++++++++++++--------------
 1 file changed, 86 insertions(+), 54 deletions(-)

-- 
2.12.0.377.gf910686b23.dirty

[RFC PATCH 0/4] pinctrl: rockchip: PREEMPT_RT_FULL fixes

[John Keeping]

As described in Documentation/gpio/driver.txt, we should not be using
sleepable APIs in the irqchip implementation.  Since this includes the
regmap API, this patch series ends up moving the mux setup for IRQs into
an irq_bus_sync_unlock() handler which may result in the IRQ being
configured before the port has been muxed as a GPIO.

I've marked the series as RFC because I'm not sure if this is the best
way to accomplish this or if there is another approach that is cleaner.
Also, the first patch may not be correct on RK3399 because I originally
wrote the patch for RK3288 on top of v4.4 where all drive updates only
affect a single register.  We don't need locking in this case because
regmap_update_bits() takes a lock on the regmap internally, but if these
two registers need to be updated atomically then another lock will
be required here - slock cannot be used if it is converted to a raw
spinlock since with full RT preemption the regmap's spinlock may sleep.

John Keeping (4):
  pinctrl: rockchip: remove unnecessary locking
  pinctrl: rockchip: convert to raw spinlock
  pinctrl: rockchip: split out verification of mux settings
  pinctrl: rockchip: avoid hardirq-unsafe functions in irq_chip

 drivers/pinctrl/pinctrl-rockchip.c | 140 +++++++++++++++++++++++--------------
 1 file changed, 86 insertions(+), 54 deletions(-)

-- 
2.12.0.377.gf910686b23.dirty

[RFC PATCH 1/4] pinctrl: rockchip: remove unnecessary locking

[John Keeping]

regmap_update_bits does its own locking and everything else accessed
here is a local variable so there is no need to lock around it.

I originally wrote this on to of v4.4 which doesn't have the split
registers for drive strength, where some additional locking might be
necessary.  But I don't think it can be "slock" given that the following
patches will convert that to a raw spinlock and regmap uses a normal
spinlock internally.

Signed-off-by: John Keeping <john@metanate.com>
---
 drivers/pinctrl/pinctrl-rockchip.c | 20 +-------------------
 1 file changed, 1 insertion(+), 19 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c
index a838c8bb3129..1defe83a5c4d 100644
--- a/drivers/pinctrl/pinctrl-rockchip.c
+++ b/drivers/pinctrl/pinctrl-rockchip.c
@@ -649,7 +649,6 @@ static int rockchip_set_mux(struct rockchip_pin_bank *bank, int pin, int mux)
 	int iomux_num = (pin / 8);
 	struct regmap *regmap;
 	int reg, ret, mask, mux_type;
-	unsigned long flags;
 	u8 bit;
 	u32 data, rmask;
 
@@ -698,15 +697,11 @@ static int rockchip_set_mux(struct rockchip_pin_bank *bank, int pin, int mux)
 	if (ctrl->iomux_recalc && (mux_type & IOMUX_RECALCED))
 		ctrl->iomux_recalc(bank->bank_num, pin, &reg, &bit, &mask);
 
-	spin_lock_irqsave(&bank->slock, flags);
-
 	data = (mask << (bit + 16));
 	rmask = data | (data >> 16);
 	data |= (mux & mask) << bit;
 	ret = regmap_update_bits(regmap, reg, rmask, data);
 
-	spin_unlock_irqrestore(&bank->slock, flags);
-
 	return ret;
 }
 
@@ -1132,7 +1127,6 @@ static int rockchip_set_drive_perpin(struct rockchip_pin_bank *bank,
 	struct rockchip_pinctrl *info = bank->drvdata;
 	struct rockchip_pin_ctrl *ctrl = info->ctrl;
 	struct regmap *regmap;
-	unsigned long flags;
 	int reg, ret, i;
 	u32 data, rmask, rmask_bits, temp;
 	u8 bit;
@@ -1160,8 +1154,6 @@ static int rockchip_set_drive_perpin(struct rockchip_pin_bank *bank,
 		return ret;
 	}
 
-	spin_lock_irqsave(&bank->slock, flags);
-
 	switch (drv_type) {
 	case DRV_TYPE_IO_1V8_3V0_AUTO:
 	case DRV_TYPE_IO_3V3_ONLY:
@@ -1182,17 +1174,14 @@ static int rockchip_set_drive_perpin(struct rockchip_pin_bank *bank,
 			rmask = BIT(15) | BIT(31);
 			data |= BIT(31);
 			ret = regmap_update_bits(regmap, reg, rmask, data);
-			if (ret) {
-				spin_unlock_irqrestore(&bank->slock, flags);
+			if (ret)
 				return ret;
-			}
 
 			rmask = 0x3 | (0x3 << 16);
 			temp |= (0x3 << 16);
 			reg += 0x4;
 			ret = regmap_update_bits(regmap, reg, rmask, temp);
 
-			spin_unlock_irqrestore(&bank->slock, flags);
 			return ret;
 		case 18 ... 21:
 			/* setting fully enclosed in the second register */
@@ -1200,7 +1189,6 @@ static int rockchip_set_drive_perpin(struct rockchip_pin_bank *bank,
 			bit -= 16;
 			break;
 		default:
-			spin_unlock_irqrestore(&bank->slock, flags);
 			dev_err(info->dev, "unsupported bit: %d for pinctrl drive type: %d\n",
 				bit, drv_type);
 			return -EINVAL;
@@ -1212,7 +1200,6 @@ static int rockchip_set_drive_perpin(struct rockchip_pin_bank *bank,
 		rmask_bits = RK3288_DRV_BITS_PER_PIN;
 		break;
 	default:
-		spin_unlock_irqrestore(&bank->slock, flags);
 		dev_err(info->dev, "unsupported pinctrl drive type: %d\n",
 			drv_type);
 		return -EINVAL;
@@ -1224,7 +1211,6 @@ static int rockchip_set_drive_perpin(struct rockchip_pin_bank *bank,
 	data |= (ret << bit);
 
 	ret = regmap_update_bits(regmap, reg, rmask, data);
-	spin_unlock_irqrestore(&bank->slock, flags);
 
 	return ret;
 }
@@ -1336,16 +1322,12 @@ static int rockchip_set_pull(struct rockchip_pin_bank *bank,
 			return ret;
 		}
 
-		spin_lock_irqsave(&bank->slock, flags);
-
 		/* enable the write to the equivalent lower bits */
 		data = ((1 << RK3188_PULL_BITS_PER_PIN) - 1) << (bit + 16);
 		rmask = data | (data >> 16);
 		data |= (ret << bit);
 
 		ret = regmap_update_bits(regmap, reg, rmask, data);
-
-		spin_unlock_irqrestore(&bank->slock, flags);
 		break;
 	default:
 		dev_err(info->dev, "unsupported pinctrl type\n");
-- 
2.12.0.377.gf910686b23.dirty

[RFC PATCH 1/4] pinctrl: rockchip: remove unnecessary locking

[John Keeping]

regmap_update_bits does its own locking and everything else accessed
here is a local variable so there is no need to lock around it.

I originally wrote this on to of v4.4 which doesn't have the split
registers for drive strength, where some additional locking might be
necessary.  But I don't think it can be "slock" given that the following
patches will convert that to a raw spinlock and regmap uses a normal
spinlock internally.

Signed-off-by: John Keeping <john@metanate.com>
---
 drivers/pinctrl/pinctrl-rockchip.c | 20 +-------------------
 1 file changed, 1 insertion(+), 19 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c
index a838c8bb3129..1defe83a5c4d 100644
--- a/drivers/pinctrl/pinctrl-rockchip.c
+++ b/drivers/pinctrl/pinctrl-rockchip.c
@@ -649,7 +649,6 @@ static int rockchip_set_mux(struct rockchip_pin_bank *bank, int pin, int mux)
 	int iomux_num = (pin / 8);
 	struct regmap *regmap;
 	int reg, ret, mask, mux_type;
-	unsigned long flags;
 	u8 bit;
 	u32 data, rmask;
 
@@ -698,15 +697,11 @@ static int rockchip_set_mux(struct rockchip_pin_bank *bank, int pin, int mux)
 	if (ctrl->iomux_recalc && (mux_type & IOMUX_RECALCED))
 		ctrl->iomux_recalc(bank->bank_num, pin, &reg, &bit, &mask);
 
-	spin_lock_irqsave(&bank->slock, flags);
-
 	data = (mask << (bit + 16));
 	rmask = data | (data >> 16);
 	data |= (mux & mask) << bit;
 	ret = regmap_update_bits(regmap, reg, rmask, data);
 
-	spin_unlock_irqrestore(&bank->slock, flags);
-
 	return ret;
 }
 
@@ -1132,7 +1127,6 @@ static int rockchip_set_drive_perpin(struct rockchip_pin_bank *bank,
 	struct rockchip_pinctrl *info = bank->drvdata;
 	struct rockchip_pin_ctrl *ctrl = info->ctrl;
 	struct regmap *regmap;
-	unsigned long flags;
 	int reg, ret, i;
 	u32 data, rmask, rmask_bits, temp;
 	u8 bit;
@@ -1160,8 +1154,6 @@ static int rockchip_set_drive_perpin(struct rockchip_pin_bank *bank,
 		return ret;
 	}
 
-	spin_lock_irqsave(&bank->slock, flags);
-
 	switch (drv_type) {
 	case DRV_TYPE_IO_1V8_3V0_AUTO:
 	case DRV_TYPE_IO_3V3_ONLY:
@@ -1182,17 +1174,14 @@ static int rockchip_set_drive_perpin(struct rockchip_pin_bank *bank,
 			rmask = BIT(15) | BIT(31);
 			data |= BIT(31);
 			ret = regmap_update_bits(regmap, reg, rmask, data);
-			if (ret) {
-				spin_unlock_irqrestore(&bank->slock, flags);
+			if (ret)
 				return ret;
-			}
 
 			rmask = 0x3 | (0x3 << 16);
 			temp |= (0x3 << 16);
 			reg += 0x4;
 			ret = regmap_update_bits(regmap, reg, rmask, temp);
 
-			spin_unlock_irqrestore(&bank->slock, flags);
 			return ret;
 		case 18 ... 21:
 			/* setting fully enclosed in the second register */
@@ -1200,7 +1189,6 @@ static int rockchip_set_drive_perpin(struct rockchip_pin_bank *bank,
 			bit -= 16;
 			break;
 		default:
-			spin_unlock_irqrestore(&bank->slock, flags);
 			dev_err(info->dev, "unsupported bit: %d for pinctrl drive type: %d\n",
 				bit, drv_type);
 			return -EINVAL;
@@ -1212,7 +1200,6 @@ static int rockchip_set_drive_perpin(struct rockchip_pin_bank *bank,
 		rmask_bits = RK3288_DRV_BITS_PER_PIN;
 		break;
 	default:
-		spin_unlock_irqrestore(&bank->slock, flags);
 		dev_err(info->dev, "unsupported pinctrl drive type: %d\n",
 			drv_type);
 		return -EINVAL;
@@ -1224,7 +1211,6 @@ static int rockchip_set_drive_perpin(struct rockchip_pin_bank *bank,
 	data |= (ret << bit);
 
 	ret = regmap_update_bits(regmap, reg, rmask, data);
-	spin_unlock_irqrestore(&bank->slock, flags);
 
 	return ret;
 }
@@ -1336,16 +1322,12 @@ static int rockchip_set_pull(struct rockchip_pin_bank *bank,
 			return ret;
 		}
 
-		spin_lock_irqsave(&bank->slock, flags);
-
 		/* enable the write to the equivalent lower bits */
 		data = ((1 << RK3188_PULL_BITS_PER_PIN) - 1) << (bit + 16);
 		rmask = data | (data >> 16);
 		data |= (ret << bit);
 
 		ret = regmap_update_bits(regmap, reg, rmask, data);
-
-		spin_unlock_irqrestore(&bank->slock, flags);
 		break;
 	default:
 		dev_err(info->dev, "unsupported pinctrl type\n");
-- 
2.12.0.377.gf910686b23.dirty

Re: [RFC PATCH 1/4] pinctrl: rockchip: remove unnecessary locking

[Heiko Stuebner]

Am Montag, 13. März 2017, 18:38:10 CET schrieb John Keeping:
> regmap_update_bits does its own locking and everything else accessed
> here is a local variable so there is no need to lock around it.
> 
> I originally wrote this on to of v4.4 which doesn't have the split
> registers for drive strength, where some additional locking might be
> necessary.  But I don't think it can be "slock" given that the following
> patches will convert that to a raw spinlock and regmap uses a normal
> spinlock internally.
> 
> Signed-off-by: John Keeping <john@metanate.com>

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

I have a hard time remembering why these locks are in there in the first place. 
Not only has regmap its own locking, all the grf-registers handling iomux (and 
also the new schmitt triggers) are hiword-mask registers (meaning you enable 
bit x+16 to get write access to bit x), so there isn't locking needed to 
prevent concurrent register access in the first place.

I can only guess  this was meant as a safeguard for earlier socs (
- rk2928 - single-core Cortex-A9
- rk2918 Cortex-A8
- earlier ARM9) that hadn't these hiword-mask mechanisms yet.
I don't think we'll ever have support for those (they're old and nobody cares) 
but we can of course revisit this if at some point somebody is interested in 
affected socs.

Of course that is only true for the grf-based parts, not for the gpio 
registers.

> ---
>  drivers/pinctrl/pinctrl-rockchip.c | 20 +-------------------
>  1 file changed, 1 insertion(+), 19 deletions(-)
> 
> diff --git a/drivers/pinctrl/pinctrl-rockchip.c
> b/drivers/pinctrl/pinctrl-rockchip.c index a838c8bb3129..1defe83a5c4d
> 100644
> --- a/drivers/pinctrl/pinctrl-rockchip.c
> +++ b/drivers/pinctrl/pinctrl-rockchip.c
> @@ -649,7 +649,6 @@ static int rockchip_set_mux(struct rockchip_pin_bank
> *bank, int pin, int mux) int iomux_num = (pin / 8);
>  	struct regmap *regmap;
>  	int reg, ret, mask, mux_type;
> -	unsigned long flags;
>  	u8 bit;
>  	u32 data, rmask;
> 
> @@ -698,15 +697,11 @@ static int rockchip_set_mux(struct rockchip_pin_bank
> *bank, int pin, int mux) if (ctrl->iomux_recalc && (mux_type &
> IOMUX_RECALCED))
>  		ctrl->iomux_recalc(bank->bank_num, pin, &reg, &bit, &mask);
> 
> -	spin_lock_irqsave(&bank->slock, flags);
> -
>  	data = (mask << (bit + 16));
>  	rmask = data | (data >> 16);
>  	data |= (mux & mask) << bit;
>  	ret = regmap_update_bits(regmap, reg, rmask, data);
> 
> -	spin_unlock_irqrestore(&bank->slock, flags);
> -
>  	return ret;
>  }
> 
> @@ -1132,7 +1127,6 @@ static int rockchip_set_drive_perpin(struct
> rockchip_pin_bank *bank, struct rockchip_pinctrl *info = bank->drvdata;
>  	struct rockchip_pin_ctrl *ctrl = info->ctrl;
>  	struct regmap *regmap;
> -	unsigned long flags;
>  	int reg, ret, i;
>  	u32 data, rmask, rmask_bits, temp;
>  	u8 bit;
> @@ -1160,8 +1154,6 @@ static int rockchip_set_drive_perpin(struct
> rockchip_pin_bank *bank, return ret;
>  	}
> 
> -	spin_lock_irqsave(&bank->slock, flags);
> -
>  	switch (drv_type) {
>  	case DRV_TYPE_IO_1V8_3V0_AUTO:
>  	case DRV_TYPE_IO_3V3_ONLY:
> @@ -1182,17 +1174,14 @@ static int rockchip_set_drive_perpin(struct
> rockchip_pin_bank *bank, rmask = BIT(15) | BIT(31);
>  			data |= BIT(31);
>  			ret = regmap_update_bits(regmap, reg, rmask, data);
> -			if (ret) {
> -				spin_unlock_irqrestore(&bank->slock, flags);
> +			if (ret)
>  				return ret;
> -			}
> 
>  			rmask = 0x3 | (0x3 << 16);
>  			temp |= (0x3 << 16);
>  			reg += 0x4;
>  			ret = regmap_update_bits(regmap, reg, rmask, temp);
> 
> -			spin_unlock_irqrestore(&bank->slock, flags);
>  			return ret;
>  		case 18 ... 21:
>  			/* setting fully enclosed in the second register */
> @@ -1200,7 +1189,6 @@ static int rockchip_set_drive_perpin(struct
> rockchip_pin_bank *bank, bit -= 16;
>  			break;
>  		default:
> -			spin_unlock_irqrestore(&bank->slock, flags);
>  			dev_err(info->dev, "unsupported bit: %d for pinctrl drive type: %d
\n",
>  				bit, drv_type);
>  			return -EINVAL;
> @@ -1212,7 +1200,6 @@ static int rockchip_set_drive_perpin(struct
> rockchip_pin_bank *bank, rmask_bits = RK3288_DRV_BITS_PER_PIN;
>  		break;
>  	default:
> -		spin_unlock_irqrestore(&bank->slock, flags);
>  		dev_err(info->dev, "unsupported pinctrl drive type: %d\n",
>  			drv_type);
>  		return -EINVAL;
> @@ -1224,7 +1211,6 @@ static int rockchip_set_drive_perpin(struct
> rockchip_pin_bank *bank, data |= (ret << bit);
> 
>  	ret = regmap_update_bits(regmap, reg, rmask, data);
> -	spin_unlock_irqrestore(&bank->slock, flags);
> 
>  	return ret;
>  }
> @@ -1336,16 +1322,12 @@ static int rockchip_set_pull(struct
> rockchip_pin_bank *bank, return ret;
>  		}
> 
> -		spin_lock_irqsave(&bank->slock, flags);
> -
>  		/* enable the write to the equivalent lower bits */
>  		data = ((1 << RK3188_PULL_BITS_PER_PIN) - 1) << (bit + 16);
>  		rmask = data | (data >> 16);
>  		data |= (ret << bit);
> 
>  		ret = regmap_update_bits(regmap, reg, rmask, data);
> -
> -		spin_unlock_irqrestore(&bank->slock, flags);
>  		break;
>  	default:
>  		dev_err(info->dev, "unsupported pinctrl type\n");

Re: [RFC PATCH 1/4] pinctrl: rockchip: remove unnecessary locking

[Heiko Stuebner]

Am Montag, 13. März 2017, 18:38:10 CET schrieb John Keeping:
> regmap_update_bits does its own locking and everything else accessed
> here is a local variable so there is no need to lock around it.
> 
> I originally wrote this on to of v4.4 which doesn't have the split
> registers for drive strength, where some additional locking might be
> necessary.  But I don't think it can be "slock" given that the following
> patches will convert that to a raw spinlock and regmap uses a normal
> spinlock internally.
> 
> Signed-off-by: John Keeping <john@metanate.com>

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

I have a hard time remembering why these locks are in there in the first place. 
Not only has regmap its own locking, all the grf-registers handling iomux (and 
also the new schmitt triggers) are hiword-mask registers (meaning you enable 
bit x+16 to get write access to bit x), so there isn't locking needed to 
prevent concurrent register access in the first place.

I can only guess  this was meant as a safeguard for earlier socs (
- rk2928 - single-core Cortex-A9
- rk2918 Cortex-A8
- earlier ARM9) that hadn't these hiword-mask mechanisms yet.
I don't think we'll ever have support for those (they're old and nobody cares) 
but we can of course revisit this if at some point somebody is interested in 
affected socs.

Of course that is only true for the grf-based parts, not for the gpio 
registers.

> ---
>  drivers/pinctrl/pinctrl-rockchip.c | 20 +-------------------
>  1 file changed, 1 insertion(+), 19 deletions(-)
> 
> diff --git a/drivers/pinctrl/pinctrl-rockchip.c
> b/drivers/pinctrl/pinctrl-rockchip.c index a838c8bb3129..1defe83a5c4d
> 100644
> --- a/drivers/pinctrl/pinctrl-rockchip.c
> +++ b/drivers/pinctrl/pinctrl-rockchip.c
> @@ -649,7 +649,6 @@ static int rockchip_set_mux(struct rockchip_pin_bank
> *bank, int pin, int mux) int iomux_num = (pin / 8);
>  	struct regmap *regmap;
>  	int reg, ret, mask, mux_type;
> -	unsigned long flags;
>  	u8 bit;
>  	u32 data, rmask;
> 
> @@ -698,15 +697,11 @@ static int rockchip_set_mux(struct rockchip_pin_bank
> *bank, int pin, int mux) if (ctrl->iomux_recalc && (mux_type &
> IOMUX_RECALCED))
>  		ctrl->iomux_recalc(bank->bank_num, pin, &reg, &bit, &mask);
> 
> -	spin_lock_irqsave(&bank->slock, flags);
> -
>  	data = (mask << (bit + 16));
>  	rmask = data | (data >> 16);
>  	data |= (mux & mask) << bit;
>  	ret = regmap_update_bits(regmap, reg, rmask, data);
> 
> -	spin_unlock_irqrestore(&bank->slock, flags);
> -
>  	return ret;
>  }
> 
> @@ -1132,7 +1127,6 @@ static int rockchip_set_drive_perpin(struct
> rockchip_pin_bank *bank, struct rockchip_pinctrl *info = bank->drvdata;
>  	struct rockchip_pin_ctrl *ctrl = info->ctrl;
>  	struct regmap *regmap;
> -	unsigned long flags;
>  	int reg, ret, i;
>  	u32 data, rmask, rmask_bits, temp;
>  	u8 bit;
> @@ -1160,8 +1154,6 @@ static int rockchip_set_drive_perpin(struct
> rockchip_pin_bank *bank, return ret;
>  	}
> 
> -	spin_lock_irqsave(&bank->slock, flags);
> -
>  	switch (drv_type) {
>  	case DRV_TYPE_IO_1V8_3V0_AUTO:
>  	case DRV_TYPE_IO_3V3_ONLY:
> @@ -1182,17 +1174,14 @@ static int rockchip_set_drive_perpin(struct
> rockchip_pin_bank *bank, rmask = BIT(15) | BIT(31);
>  			data |= BIT(31);
>  			ret = regmap_update_bits(regmap, reg, rmask, data);
> -			if (ret) {
> -				spin_unlock_irqrestore(&bank->slock, flags);
> +			if (ret)
>  				return ret;
> -			}
> 
>  			rmask = 0x3 | (0x3 << 16);
>  			temp |= (0x3 << 16);
>  			reg += 0x4;
>  			ret = regmap_update_bits(regmap, reg, rmask, temp);
> 
> -			spin_unlock_irqrestore(&bank->slock, flags);
>  			return ret;
>  		case 18 ... 21:
>  			/* setting fully enclosed in the second register */
> @@ -1200,7 +1189,6 @@ static int rockchip_set_drive_perpin(struct
> rockchip_pin_bank *bank, bit -= 16;
>  			break;
>  		default:
> -			spin_unlock_irqrestore(&bank->slock, flags);
>  			dev_err(info->dev, "unsupported bit: %d for pinctrl drive type: %d
\n",
>  				bit, drv_type);
>  			return -EINVAL;
> @@ -1212,7 +1200,6 @@ static int rockchip_set_drive_perpin(struct
> rockchip_pin_bank *bank, rmask_bits = RK3288_DRV_BITS_PER_PIN;
>  		break;
>  	default:
> -		spin_unlock_irqrestore(&bank->slock, flags);
>  		dev_err(info->dev, "unsupported pinctrl drive type: %d\n",
>  			drv_type);
>  		return -EINVAL;
> @@ -1224,7 +1211,6 @@ static int rockchip_set_drive_perpin(struct
> rockchip_pin_bank *bank, data |= (ret << bit);
> 
>  	ret = regmap_update_bits(regmap, reg, rmask, data);
> -	spin_unlock_irqrestore(&bank->slock, flags);
> 
>  	return ret;
>  }
> @@ -1336,16 +1322,12 @@ static int rockchip_set_pull(struct
> rockchip_pin_bank *bank, return ret;
>  		}
> 
> -		spin_lock_irqsave(&bank->slock, flags);
> -
>  		/* enable the write to the equivalent lower bits */
>  		data = ((1 << RK3188_PULL_BITS_PER_PIN) - 1) << (bit + 16);
>  		rmask = data | (data >> 16);
>  		data |= (ret << bit);
> 
>  		ret = regmap_update_bits(regmap, reg, rmask, data);
> -
> -		spin_unlock_irqrestore(&bank->slock, flags);
>  		break;
>  	default:
>  		dev_err(info->dev, "unsupported pinctrl type\n");

[RFC PATCH 1/4] pinctrl: rockchip: remove unnecessary locking

[Heiko Stuebner]

Am Montag, 13. M?rz 2017, 18:38:10 CET schrieb John Keeping:
> regmap_update_bits does its own locking and everything else accessed
> here is a local variable so there is no need to lock around it.
> 
> I originally wrote this on to of v4.4 which doesn't have the split
> registers for drive strength, where some additional locking might be
> necessary.  But I don't think it can be "slock" given that the following
> patches will convert that to a raw spinlock and regmap uses a normal
> spinlock internally.
> 
> Signed-off-by: John Keeping <john@metanate.com>

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

I have a hard time remembering why these locks are in there in the first place. 
Not only has regmap its own locking, all the grf-registers handling iomux (and 
also the new schmitt triggers) are hiword-mask registers (meaning you enable 
bit x+16 to get write access to bit x), so there isn't locking needed to 
prevent concurrent register access in the first place.

I can only guess  this was meant as a safeguard for earlier socs (
- rk2928 - single-core Cortex-A9
- rk2918 Cortex-A8
- earlier ARM9) that hadn't these hiword-mask mechanisms yet.
I don't think we'll ever have support for those (they're old and nobody cares) 
but we can of course revisit this if at some point somebody is interested in 
affected socs.

Of course that is only true for the grf-based parts, not for the gpio 
registers.

> ---
>  drivers/pinctrl/pinctrl-rockchip.c | 20 +-------------------
>  1 file changed, 1 insertion(+), 19 deletions(-)
> 
> diff --git a/drivers/pinctrl/pinctrl-rockchip.c
> b/drivers/pinctrl/pinctrl-rockchip.c index a838c8bb3129..1defe83a5c4d
> 100644
> --- a/drivers/pinctrl/pinctrl-rockchip.c
> +++ b/drivers/pinctrl/pinctrl-rockchip.c
> @@ -649,7 +649,6 @@ static int rockchip_set_mux(struct rockchip_pin_bank
> *bank, int pin, int mux) int iomux_num = (pin / 8);
>  	struct regmap *regmap;
>  	int reg, ret, mask, mux_type;
> -	unsigned long flags;
>  	u8 bit;
>  	u32 data, rmask;
> 
> @@ -698,15 +697,11 @@ static int rockchip_set_mux(struct rockchip_pin_bank
> *bank, int pin, int mux) if (ctrl->iomux_recalc && (mux_type &
> IOMUX_RECALCED))
>  		ctrl->iomux_recalc(bank->bank_num, pin, &reg, &bit, &mask);
> 
> -	spin_lock_irqsave(&bank->slock, flags);
> -
>  	data = (mask << (bit + 16));
>  	rmask = data | (data >> 16);
>  	data |= (mux & mask) << bit;
>  	ret = regmap_update_bits(regmap, reg, rmask, data);
> 
> -	spin_unlock_irqrestore(&bank->slock, flags);
> -
>  	return ret;
>  }
> 
> @@ -1132,7 +1127,6 @@ static int rockchip_set_drive_perpin(struct
> rockchip_pin_bank *bank, struct rockchip_pinctrl *info = bank->drvdata;
>  	struct rockchip_pin_ctrl *ctrl = info->ctrl;
>  	struct regmap *regmap;
> -	unsigned long flags;
>  	int reg, ret, i;
>  	u32 data, rmask, rmask_bits, temp;
>  	u8 bit;
> @@ -1160,8 +1154,6 @@ static int rockchip_set_drive_perpin(struct
> rockchip_pin_bank *bank, return ret;
>  	}
> 
> -	spin_lock_irqsave(&bank->slock, flags);
> -
>  	switch (drv_type) {
>  	case DRV_TYPE_IO_1V8_3V0_AUTO:
>  	case DRV_TYPE_IO_3V3_ONLY:
> @@ -1182,17 +1174,14 @@ static int rockchip_set_drive_perpin(struct
> rockchip_pin_bank *bank, rmask = BIT(15) | BIT(31);
>  			data |= BIT(31);
>  			ret = regmap_update_bits(regmap, reg, rmask, data);
> -			if (ret) {
> -				spin_unlock_irqrestore(&bank->slock, flags);
> +			if (ret)
>  				return ret;
> -			}
> 
>  			rmask = 0x3 | (0x3 << 16);
>  			temp |= (0x3 << 16);
>  			reg += 0x4;
>  			ret = regmap_update_bits(regmap, reg, rmask, temp);
> 
> -			spin_unlock_irqrestore(&bank->slock, flags);
>  			return ret;
>  		case 18 ... 21:
>  			/* setting fully enclosed in the second register */
> @@ -1200,7 +1189,6 @@ static int rockchip_set_drive_perpin(struct
> rockchip_pin_bank *bank, bit -= 16;
>  			break;
>  		default:
> -			spin_unlock_irqrestore(&bank->slock, flags);
>  			dev_err(info->dev, "unsupported bit: %d for pinctrl drive type: %d
\n",
>  				bit, drv_type);
>  			return -EINVAL;
> @@ -1212,7 +1200,6 @@ static int rockchip_set_drive_perpin(struct
> rockchip_pin_bank *bank, rmask_bits = RK3288_DRV_BITS_PER_PIN;
>  		break;
>  	default:
> -		spin_unlock_irqrestore(&bank->slock, flags);
>  		dev_err(info->dev, "unsupported pinctrl drive type: %d\n",
>  			drv_type);
>  		return -EINVAL;
> @@ -1224,7 +1211,6 @@ static int rockchip_set_drive_perpin(struct
> rockchip_pin_bank *bank, data |= (ret << bit);
> 
>  	ret = regmap_update_bits(regmap, reg, rmask, data);
> -	spin_unlock_irqrestore(&bank->slock, flags);
> 
>  	return ret;
>  }
> @@ -1336,16 +1322,12 @@ static int rockchip_set_pull(struct
> rockchip_pin_bank *bank, return ret;
>  		}
> 
> -		spin_lock_irqsave(&bank->slock, flags);
> -
>  		/* enable the write to the equivalent lower bits */
>  		data = ((1 << RK3188_PULL_BITS_PER_PIN) - 1) << (bit + 16);
>  		rmask = data | (data >> 16);
>  		data |= (ret << bit);
> 
>  		ret = regmap_update_bits(regmap, reg, rmask, data);
> -
> -		spin_unlock_irqrestore(&bank->slock, flags);
>  		break;
>  	default:
>  		dev_err(info->dev, "unsupported pinctrl type\n");

[RFC PATCH 2/4] pinctrl: rockchip: convert to raw spinlock

[John Keeping]

This lock is used from rockchip_irq_set_type() which is part of the
irq_chip implementation and thus must use raw_spinlock_t as documented
in Documentation/gpio/driver.txt.

Signed-off-by: John Keeping <john@metanate.com>
---
 drivers/pinctrl/pinctrl-rockchip.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c
index 1defe83a5c4d..2f963aea64b2 100644
--- a/drivers/pinctrl/pinctrl-rockchip.c
+++ b/drivers/pinctrl/pinctrl-rockchip.c
@@ -163,7 +163,7 @@ struct rockchip_pin_bank {
 	struct irq_domain		*domain;
 	struct gpio_chip		gpio_chip;
 	struct pinctrl_gpio_range	grange;
-	spinlock_t			slock;
+	raw_spinlock_t			slock;
 	u32				toggle_edge_mode;
 };
 
@@ -1292,14 +1292,14 @@ static int rockchip_set_pull(struct rockchip_pin_bank *bank,
 
 	switch (ctrl->type) {
 	case RK2928:
-		spin_lock_irqsave(&bank->slock, flags);
+		raw_spin_lock_irqsave(&bank->slock, flags);
 
 		data = BIT(bit + 16);
 		if (pull == PIN_CONFIG_BIAS_DISABLE)
 			data |= BIT(bit);
 		ret = regmap_write(regmap, reg, data);
 
-		spin_unlock_irqrestore(&bank->slock, flags);
+		raw_spin_unlock_irqrestore(&bank->slock, flags);
 		break;
 	case RK1108:
 	case RK3188:
@@ -1433,7 +1433,7 @@ static int _rockchip_pmx_gpio_set_direction(struct gpio_chip *chip,
 		return ret;
 
 	clk_enable(bank->clk);
-	spin_lock_irqsave(&bank->slock, flags);
+	raw_spin_lock_irqsave(&bank->slock, flags);
 
 	data = readl_relaxed(bank->reg_base + GPIO_SWPORT_DDR);
 	/* set bit to 1 for output, 0 for input */
@@ -1443,7 +1443,7 @@ 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);
+	raw_spin_unlock_irqrestore(&bank->slock, flags);
 	clk_disable(bank->clk);
 
 	return 0;
@@ -1874,7 +1874,7 @@ static void rockchip_gpio_set(struct gpio_chip *gc, unsigned offset, int value)
 	u32 data;
 
 	clk_enable(bank->clk);
-	spin_lock_irqsave(&bank->slock, flags);
+	raw_spin_lock_irqsave(&bank->slock, flags);
 
 	data = readl(reg);
 	data &= ~BIT(offset);
@@ -1882,7 +1882,7 @@ static void rockchip_gpio_set(struct gpio_chip *gc, unsigned offset, int value)
 		data |= BIT(offset);
 	writel(data, reg);
 
-	spin_unlock_irqrestore(&bank->slock, flags);
+	raw_spin_unlock_irqrestore(&bank->slock, flags);
 	clk_disable(bank->clk);
 }
 
@@ -1994,7 +1994,7 @@ static void rockchip_irq_demux(struct irq_desc *desc)
 
 			data = readl_relaxed(bank->reg_base + GPIO_EXT_PORT);
 			do {
-				spin_lock_irqsave(&bank->slock, flags);
+				raw_spin_lock_irqsave(&bank->slock, flags);
 
 				polarity = readl_relaxed(bank->reg_base +
 							 GPIO_INT_POLARITY);
@@ -2005,7 +2005,7 @@ static void rockchip_irq_demux(struct irq_desc *desc)
 				writel(polarity,
 				       bank->reg_base + GPIO_INT_POLARITY);
 
-				spin_unlock_irqrestore(&bank->slock, flags);
+				raw_spin_unlock_irqrestore(&bank->slock, flags);
 
 				data_old = data;
 				data = readl_relaxed(bank->reg_base +
@@ -2036,20 +2036,20 @@ static int rockchip_irq_set_type(struct irq_data *d, unsigned int type)
 		return ret;
 
 	clk_enable(bank->clk);
-	spin_lock_irqsave(&bank->slock, flags);
+	raw_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);
+	raw_spin_unlock_irqrestore(&bank->slock, flags);
 
 	if (type & IRQ_TYPE_EDGE_BOTH)
 		irq_set_handler_locked(d, handle_edge_irq);
 	else
 		irq_set_handler_locked(d, handle_level_irq);
 
-	spin_lock_irqsave(&bank->slock, flags);
+	raw_spin_lock_irqsave(&bank->slock, flags);
 	irq_gc_lock(gc);
 
 	level = readl_relaxed(gc->reg_base + GPIO_INTTYPE_LEVEL);
@@ -2092,7 +2092,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);
+		raw_spin_unlock_irqrestore(&bank->slock, flags);
 		clk_disable(bank->clk);
 		return -EINVAL;
 	}
@@ -2101,7 +2101,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);
+	raw_spin_unlock_irqrestore(&bank->slock, flags);
 	clk_disable(bank->clk);
 
 	return 0;
@@ -2383,7 +2383,7 @@ static struct rockchip_pin_ctrl *rockchip_pinctrl_get_soc_data(
 	for (i = 0; i < ctrl->nr_banks; ++i, ++bank) {
 		int bank_pins = 0;
 
-		spin_lock_init(&bank->slock);
+		raw_spin_lock_init(&bank->slock);
 		bank->drvdata = d;
 		bank->pin_base = ctrl->nr_pins;
 		ctrl->nr_pins += bank->nr_pins;
-- 
2.12.0.377.gf910686b23.dirty

[RFC PATCH 2/4] pinctrl: rockchip: convert to raw spinlock

[John Keeping]

This lock is used from rockchip_irq_set_type() which is part of the
irq_chip implementation and thus must use raw_spinlock_t as documented
in Documentation/gpio/driver.txt.

Signed-off-by: John Keeping <john@metanate.com>
---
 drivers/pinctrl/pinctrl-rockchip.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c
index 1defe83a5c4d..2f963aea64b2 100644
--- a/drivers/pinctrl/pinctrl-rockchip.c
+++ b/drivers/pinctrl/pinctrl-rockchip.c
@@ -163,7 +163,7 @@ struct rockchip_pin_bank {
 	struct irq_domain		*domain;
 	struct gpio_chip		gpio_chip;
 	struct pinctrl_gpio_range	grange;
-	spinlock_t			slock;
+	raw_spinlock_t			slock;
 	u32				toggle_edge_mode;
 };
 
@@ -1292,14 +1292,14 @@ static int rockchip_set_pull(struct rockchip_pin_bank *bank,
 
 	switch (ctrl->type) {
 	case RK2928:
-		spin_lock_irqsave(&bank->slock, flags);
+		raw_spin_lock_irqsave(&bank->slock, flags);
 
 		data = BIT(bit + 16);
 		if (pull == PIN_CONFIG_BIAS_DISABLE)
 			data |= BIT(bit);
 		ret = regmap_write(regmap, reg, data);
 
-		spin_unlock_irqrestore(&bank->slock, flags);
+		raw_spin_unlock_irqrestore(&bank->slock, flags);
 		break;
 	case RK1108:
 	case RK3188:
@@ -1433,7 +1433,7 @@ static int _rockchip_pmx_gpio_set_direction(struct gpio_chip *chip,
 		return ret;
 
 	clk_enable(bank->clk);
-	spin_lock_irqsave(&bank->slock, flags);
+	raw_spin_lock_irqsave(&bank->slock, flags);
 
 	data = readl_relaxed(bank->reg_base + GPIO_SWPORT_DDR);
 	/* set bit to 1 for output, 0 for input */
@@ -1443,7 +1443,7 @@ 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);
+	raw_spin_unlock_irqrestore(&bank->slock, flags);
 	clk_disable(bank->clk);
 
 	return 0;
@@ -1874,7 +1874,7 @@ static void rockchip_gpio_set(struct gpio_chip *gc, unsigned offset, int value)
 	u32 data;
 
 	clk_enable(bank->clk);
-	spin_lock_irqsave(&bank->slock, flags);
+	raw_spin_lock_irqsave(&bank->slock, flags);
 
 	data = readl(reg);
 	data &= ~BIT(offset);
@@ -1882,7 +1882,7 @@ static void rockchip_gpio_set(struct gpio_chip *gc, unsigned offset, int value)
 		data |= BIT(offset);
 	writel(data, reg);
 
-	spin_unlock_irqrestore(&bank->slock, flags);
+	raw_spin_unlock_irqrestore(&bank->slock, flags);
 	clk_disable(bank->clk);
 }
 
@@ -1994,7 +1994,7 @@ static void rockchip_irq_demux(struct irq_desc *desc)
 
 			data = readl_relaxed(bank->reg_base + GPIO_EXT_PORT);
 			do {
-				spin_lock_irqsave(&bank->slock, flags);
+				raw_spin_lock_irqsave(&bank->slock, flags);
 
 				polarity = readl_relaxed(bank->reg_base +
 							 GPIO_INT_POLARITY);
@@ -2005,7 +2005,7 @@ static void rockchip_irq_demux(struct irq_desc *desc)
 				writel(polarity,
 				       bank->reg_base + GPIO_INT_POLARITY);
 
-				spin_unlock_irqrestore(&bank->slock, flags);
+				raw_spin_unlock_irqrestore(&bank->slock, flags);
 
 				data_old = data;
 				data = readl_relaxed(bank->reg_base +
@@ -2036,20 +2036,20 @@ static int rockchip_irq_set_type(struct irq_data *d, unsigned int type)
 		return ret;
 
 	clk_enable(bank->clk);
-	spin_lock_irqsave(&bank->slock, flags);
+	raw_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);
+	raw_spin_unlock_irqrestore(&bank->slock, flags);
 
 	if (type & IRQ_TYPE_EDGE_BOTH)
 		irq_set_handler_locked(d, handle_edge_irq);
 	else
 		irq_set_handler_locked(d, handle_level_irq);
 
-	spin_lock_irqsave(&bank->slock, flags);
+	raw_spin_lock_irqsave(&bank->slock, flags);
 	irq_gc_lock(gc);
 
 	level = readl_relaxed(gc->reg_base + GPIO_INTTYPE_LEVEL);
@@ -2092,7 +2092,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);
+		raw_spin_unlock_irqrestore(&bank->slock, flags);
 		clk_disable(bank->clk);
 		return -EINVAL;
 	}
@@ -2101,7 +2101,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);
+	raw_spin_unlock_irqrestore(&bank->slock, flags);
 	clk_disable(bank->clk);
 
 	return 0;
@@ -2383,7 +2383,7 @@ static struct rockchip_pin_ctrl *rockchip_pinctrl_get_soc_data(
 	for (i = 0; i < ctrl->nr_banks; ++i, ++bank) {
 		int bank_pins = 0;
 
-		spin_lock_init(&bank->slock);
+		raw_spin_lock_init(&bank->slock);
 		bank->drvdata = d;
 		bank->pin_base = ctrl->nr_pins;
 		ctrl->nr_pins += bank->nr_pins;
-- 
2.12.0.377.gf910686b23.dirty

Re: [RFC PATCH 2/4] pinctrl: rockchip: convert to raw spinlock

[Heiko Stuebner]

Am Montag, 13. März 2017, 18:38:11 CET schrieb John Keeping:
> This lock is used from rockchip_irq_set_type() which is part of the
> irq_chip implementation and thus must use raw_spinlock_t as documented
> in Documentation/gpio/driver.txt.
> 
> Signed-off-by: John Keeping <john@metanate.com>

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

> ---
>  drivers/pinctrl/pinctrl-rockchip.c | 30 +++++++++++++++---------------
>  1 file changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/pinctrl/pinctrl-rockchip.c
> b/drivers/pinctrl/pinctrl-rockchip.c index 1defe83a5c4d..2f963aea64b2
> 100644
> --- a/drivers/pinctrl/pinctrl-rockchip.c
> +++ b/drivers/pinctrl/pinctrl-rockchip.c
> @@ -163,7 +163,7 @@ struct rockchip_pin_bank {
>  	struct irq_domain		*domain;
>  	struct gpio_chip		gpio_chip;
>  	struct pinctrl_gpio_range	grange;
> -	spinlock_t			slock;
> +	raw_spinlock_t			slock;
>  	u32				toggle_edge_mode;
>  };
> 
> @@ -1292,14 +1292,14 @@ static int rockchip_set_pull(struct
> rockchip_pin_bank *bank,
> 
>  	switch (ctrl->type) {
>  	case RK2928:
> -		spin_lock_irqsave(&bank->slock, flags);
> +		raw_spin_lock_irqsave(&bank->slock, flags);
> 
>  		data = BIT(bit + 16);
>  		if (pull == PIN_CONFIG_BIAS_DISABLE)
>  			data |= BIT(bit);
>  		ret = regmap_write(regmap, reg, data);
> 
> -		spin_unlock_irqrestore(&bank->slock, flags);
> +		raw_spin_unlock_irqrestore(&bank->slock, flags);
>  		break;
>  	case RK1108:
>  	case RK3188:
> @@ -1433,7 +1433,7 @@ static int _rockchip_pmx_gpio_set_direction(struct
> gpio_chip *chip, return ret;
> 
>  	clk_enable(bank->clk);
> -	spin_lock_irqsave(&bank->slock, flags);
> +	raw_spin_lock_irqsave(&bank->slock, flags);
> 
>  	data = readl_relaxed(bank->reg_base + GPIO_SWPORT_DDR);
>  	/* set bit to 1 for output, 0 for input */
> @@ -1443,7 +1443,7 @@ 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);
> +	raw_spin_unlock_irqrestore(&bank->slock, flags);
>  	clk_disable(bank->clk);
> 
>  	return 0;
> @@ -1874,7 +1874,7 @@ static void rockchip_gpio_set(struct gpio_chip *gc,
> unsigned offset, int value) u32 data;
> 
>  	clk_enable(bank->clk);
> -	spin_lock_irqsave(&bank->slock, flags);
> +	raw_spin_lock_irqsave(&bank->slock, flags);
> 
>  	data = readl(reg);
>  	data &= ~BIT(offset);
> @@ -1882,7 +1882,7 @@ static void rockchip_gpio_set(struct gpio_chip *gc,
> unsigned offset, int value) data |= BIT(offset);
>  	writel(data, reg);
> 
> -	spin_unlock_irqrestore(&bank->slock, flags);
> +	raw_spin_unlock_irqrestore(&bank->slock, flags);
>  	clk_disable(bank->clk);
>  }
> 
> @@ -1994,7 +1994,7 @@ static void rockchip_irq_demux(struct irq_desc *desc)
> 
>  			data = readl_relaxed(bank->reg_base + GPIO_EXT_PORT);
>  			do {
> -				spin_lock_irqsave(&bank->slock, flags);
> +				raw_spin_lock_irqsave(&bank->slock, flags);
> 
>  				polarity = readl_relaxed(bank->reg_base +
>  							 GPIO_INT_POLARITY);
> @@ -2005,7 +2005,7 @@ static void rockchip_irq_demux(struct irq_desc *desc)
>  				writel(polarity,
>  				       bank->reg_base + GPIO_INT_POLARITY);
> 
> -				spin_unlock_irqrestore(&bank->slock, flags);
> +				raw_spin_unlock_irqrestore(&bank->slock, flags);
> 
>  				data_old = data;
>  				data = readl_relaxed(bank->reg_base +
> @@ -2036,20 +2036,20 @@ static int rockchip_irq_set_type(struct irq_data *d,
> unsigned int type) return ret;
> 
>  	clk_enable(bank->clk);
> -	spin_lock_irqsave(&bank->slock, flags);
> +	raw_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);
> +	raw_spin_unlock_irqrestore(&bank->slock, flags);
> 
>  	if (type & IRQ_TYPE_EDGE_BOTH)
>  		irq_set_handler_locked(d, handle_edge_irq);
>  	else
>  		irq_set_handler_locked(d, handle_level_irq);
> 
> -	spin_lock_irqsave(&bank->slock, flags);
> +	raw_spin_lock_irqsave(&bank->slock, flags);
>  	irq_gc_lock(gc);
> 
>  	level = readl_relaxed(gc->reg_base + GPIO_INTTYPE_LEVEL);
> @@ -2092,7 +2092,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);
> +		raw_spin_unlock_irqrestore(&bank->slock, flags);
>  		clk_disable(bank->clk);
>  		return -EINVAL;
>  	}
> @@ -2101,7 +2101,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);
> +	raw_spin_unlock_irqrestore(&bank->slock, flags);
>  	clk_disable(bank->clk);
> 
>  	return 0;
> @@ -2383,7 +2383,7 @@ static struct rockchip_pin_ctrl
> *rockchip_pinctrl_get_soc_data( for (i = 0; i < ctrl->nr_banks; ++i,
> ++bank) {
>  		int bank_pins = 0;
> 
> -		spin_lock_init(&bank->slock);
> +		raw_spin_lock_init(&bank->slock);
>  		bank->drvdata = d;
>  		bank->pin_base = ctrl->nr_pins;
>  		ctrl->nr_pins += bank->nr_pins;

Re: [RFC PATCH 2/4] pinctrl: rockchip: convert to raw spinlock

[Heiko Stuebner]

Am Montag, 13. März 2017, 18:38:11 CET schrieb John Keeping:
> This lock is used from rockchip_irq_set_type() which is part of the
> irq_chip implementation and thus must use raw_spinlock_t as documented
> in Documentation/gpio/driver.txt.
> 
> Signed-off-by: John Keeping <john@metanate.com>

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

> ---
>  drivers/pinctrl/pinctrl-rockchip.c | 30 +++++++++++++++---------------
>  1 file changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/pinctrl/pinctrl-rockchip.c
> b/drivers/pinctrl/pinctrl-rockchip.c index 1defe83a5c4d..2f963aea64b2
> 100644
> --- a/drivers/pinctrl/pinctrl-rockchip.c
> +++ b/drivers/pinctrl/pinctrl-rockchip.c
> @@ -163,7 +163,7 @@ struct rockchip_pin_bank {
>  	struct irq_domain		*domain;
>  	struct gpio_chip		gpio_chip;
>  	struct pinctrl_gpio_range	grange;
> -	spinlock_t			slock;
> +	raw_spinlock_t			slock;
>  	u32				toggle_edge_mode;
>  };
> 
> @@ -1292,14 +1292,14 @@ static int rockchip_set_pull(struct
> rockchip_pin_bank *bank,
> 
>  	switch (ctrl->type) {
>  	case RK2928:
> -		spin_lock_irqsave(&bank->slock, flags);
> +		raw_spin_lock_irqsave(&bank->slock, flags);
> 
>  		data = BIT(bit + 16);
>  		if (pull == PIN_CONFIG_BIAS_DISABLE)
>  			data |= BIT(bit);
>  		ret = regmap_write(regmap, reg, data);
> 
> -		spin_unlock_irqrestore(&bank->slock, flags);
> +		raw_spin_unlock_irqrestore(&bank->slock, flags);
>  		break;
>  	case RK1108:
>  	case RK3188:
> @@ -1433,7 +1433,7 @@ static int _rockchip_pmx_gpio_set_direction(struct
> gpio_chip *chip, return ret;
> 
>  	clk_enable(bank->clk);
> -	spin_lock_irqsave(&bank->slock, flags);
> +	raw_spin_lock_irqsave(&bank->slock, flags);
> 
>  	data = readl_relaxed(bank->reg_base + GPIO_SWPORT_DDR);
>  	/* set bit to 1 for output, 0 for input */
> @@ -1443,7 +1443,7 @@ 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);
> +	raw_spin_unlock_irqrestore(&bank->slock, flags);
>  	clk_disable(bank->clk);
> 
>  	return 0;
> @@ -1874,7 +1874,7 @@ static void rockchip_gpio_set(struct gpio_chip *gc,
> unsigned offset, int value) u32 data;
> 
>  	clk_enable(bank->clk);
> -	spin_lock_irqsave(&bank->slock, flags);
> +	raw_spin_lock_irqsave(&bank->slock, flags);
> 
>  	data = readl(reg);
>  	data &= ~BIT(offset);
> @@ -1882,7 +1882,7 @@ static void rockchip_gpio_set(struct gpio_chip *gc,
> unsigned offset, int value) data |= BIT(offset);
>  	writel(data, reg);
> 
> -	spin_unlock_irqrestore(&bank->slock, flags);
> +	raw_spin_unlock_irqrestore(&bank->slock, flags);
>  	clk_disable(bank->clk);
>  }
> 
> @@ -1994,7 +1994,7 @@ static void rockchip_irq_demux(struct irq_desc *desc)
> 
>  			data = readl_relaxed(bank->reg_base + GPIO_EXT_PORT);
>  			do {
> -				spin_lock_irqsave(&bank->slock, flags);
> +				raw_spin_lock_irqsave(&bank->slock, flags);
> 
>  				polarity = readl_relaxed(bank->reg_base +
>  							 GPIO_INT_POLARITY);
> @@ -2005,7 +2005,7 @@ static void rockchip_irq_demux(struct irq_desc *desc)
>  				writel(polarity,
>  				       bank->reg_base + GPIO_INT_POLARITY);
> 
> -				spin_unlock_irqrestore(&bank->slock, flags);
> +				raw_spin_unlock_irqrestore(&bank->slock, flags);
> 
>  				data_old = data;
>  				data = readl_relaxed(bank->reg_base +
> @@ -2036,20 +2036,20 @@ static int rockchip_irq_set_type(struct irq_data *d,
> unsigned int type) return ret;
> 
>  	clk_enable(bank->clk);
> -	spin_lock_irqsave(&bank->slock, flags);
> +	raw_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);
> +	raw_spin_unlock_irqrestore(&bank->slock, flags);
> 
>  	if (type & IRQ_TYPE_EDGE_BOTH)
>  		irq_set_handler_locked(d, handle_edge_irq);
>  	else
>  		irq_set_handler_locked(d, handle_level_irq);
> 
> -	spin_lock_irqsave(&bank->slock, flags);
> +	raw_spin_lock_irqsave(&bank->slock, flags);
>  	irq_gc_lock(gc);
> 
>  	level = readl_relaxed(gc->reg_base + GPIO_INTTYPE_LEVEL);
> @@ -2092,7 +2092,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);
> +		raw_spin_unlock_irqrestore(&bank->slock, flags);
>  		clk_disable(bank->clk);
>  		return -EINVAL;
>  	}
> @@ -2101,7 +2101,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);
> +	raw_spin_unlock_irqrestore(&bank->slock, flags);
>  	clk_disable(bank->clk);
> 
>  	return 0;
> @@ -2383,7 +2383,7 @@ static struct rockchip_pin_ctrl
> *rockchip_pinctrl_get_soc_data( for (i = 0; i < ctrl->nr_banks; ++i,
> ++bank) {
>  		int bank_pins = 0;
> 
> -		spin_lock_init(&bank->slock);
> +		raw_spin_lock_init(&bank->slock);
>  		bank->drvdata = d;
>  		bank->pin_base = ctrl->nr_pins;
>  		ctrl->nr_pins += bank->nr_pins;

[RFC PATCH 2/4] pinctrl: rockchip: convert to raw spinlock

[Heiko Stuebner]

Am Montag, 13. M?rz 2017, 18:38:11 CET schrieb John Keeping:
> This lock is used from rockchip_irq_set_type() which is part of the
> irq_chip implementation and thus must use raw_spinlock_t as documented
> in Documentation/gpio/driver.txt.
> 
> Signed-off-by: John Keeping <john@metanate.com>

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

> ---
>  drivers/pinctrl/pinctrl-rockchip.c | 30 +++++++++++++++---------------
>  1 file changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/pinctrl/pinctrl-rockchip.c
> b/drivers/pinctrl/pinctrl-rockchip.c index 1defe83a5c4d..2f963aea64b2
> 100644
> --- a/drivers/pinctrl/pinctrl-rockchip.c
> +++ b/drivers/pinctrl/pinctrl-rockchip.c
> @@ -163,7 +163,7 @@ struct rockchip_pin_bank {
>  	struct irq_domain		*domain;
>  	struct gpio_chip		gpio_chip;
>  	struct pinctrl_gpio_range	grange;
> -	spinlock_t			slock;
> +	raw_spinlock_t			slock;
>  	u32				toggle_edge_mode;
>  };
> 
> @@ -1292,14 +1292,14 @@ static int rockchip_set_pull(struct
> rockchip_pin_bank *bank,
> 
>  	switch (ctrl->type) {
>  	case RK2928:
> -		spin_lock_irqsave(&bank->slock, flags);
> +		raw_spin_lock_irqsave(&bank->slock, flags);
> 
>  		data = BIT(bit + 16);
>  		if (pull == PIN_CONFIG_BIAS_DISABLE)
>  			data |= BIT(bit);
>  		ret = regmap_write(regmap, reg, data);
> 
> -		spin_unlock_irqrestore(&bank->slock, flags);
> +		raw_spin_unlock_irqrestore(&bank->slock, flags);
>  		break;
>  	case RK1108:
>  	case RK3188:
> @@ -1433,7 +1433,7 @@ static int _rockchip_pmx_gpio_set_direction(struct
> gpio_chip *chip, return ret;
> 
>  	clk_enable(bank->clk);
> -	spin_lock_irqsave(&bank->slock, flags);
> +	raw_spin_lock_irqsave(&bank->slock, flags);
> 
>  	data = readl_relaxed(bank->reg_base + GPIO_SWPORT_DDR);
>  	/* set bit to 1 for output, 0 for input */
> @@ -1443,7 +1443,7 @@ 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);
> +	raw_spin_unlock_irqrestore(&bank->slock, flags);
>  	clk_disable(bank->clk);
> 
>  	return 0;
> @@ -1874,7 +1874,7 @@ static void rockchip_gpio_set(struct gpio_chip *gc,
> unsigned offset, int value) u32 data;
> 
>  	clk_enable(bank->clk);
> -	spin_lock_irqsave(&bank->slock, flags);
> +	raw_spin_lock_irqsave(&bank->slock, flags);
> 
>  	data = readl(reg);
>  	data &= ~BIT(offset);
> @@ -1882,7 +1882,7 @@ static void rockchip_gpio_set(struct gpio_chip *gc,
> unsigned offset, int value) data |= BIT(offset);
>  	writel(data, reg);
> 
> -	spin_unlock_irqrestore(&bank->slock, flags);
> +	raw_spin_unlock_irqrestore(&bank->slock, flags);
>  	clk_disable(bank->clk);
>  }
> 
> @@ -1994,7 +1994,7 @@ static void rockchip_irq_demux(struct irq_desc *desc)
> 
>  			data = readl_relaxed(bank->reg_base + GPIO_EXT_PORT);
>  			do {
> -				spin_lock_irqsave(&bank->slock, flags);
> +				raw_spin_lock_irqsave(&bank->slock, flags);
> 
>  				polarity = readl_relaxed(bank->reg_base +
>  							 GPIO_INT_POLARITY);
> @@ -2005,7 +2005,7 @@ static void rockchip_irq_demux(struct irq_desc *desc)
>  				writel(polarity,
>  				       bank->reg_base + GPIO_INT_POLARITY);
> 
> -				spin_unlock_irqrestore(&bank->slock, flags);
> +				raw_spin_unlock_irqrestore(&bank->slock, flags);
> 
>  				data_old = data;
>  				data = readl_relaxed(bank->reg_base +
> @@ -2036,20 +2036,20 @@ static int rockchip_irq_set_type(struct irq_data *d,
> unsigned int type) return ret;
> 
>  	clk_enable(bank->clk);
> -	spin_lock_irqsave(&bank->slock, flags);
> +	raw_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);
> +	raw_spin_unlock_irqrestore(&bank->slock, flags);
> 
>  	if (type & IRQ_TYPE_EDGE_BOTH)
>  		irq_set_handler_locked(d, handle_edge_irq);
>  	else
>  		irq_set_handler_locked(d, handle_level_irq);
> 
> -	spin_lock_irqsave(&bank->slock, flags);
> +	raw_spin_lock_irqsave(&bank->slock, flags);
>  	irq_gc_lock(gc);
> 
>  	level = readl_relaxed(gc->reg_base + GPIO_INTTYPE_LEVEL);
> @@ -2092,7 +2092,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);
> +		raw_spin_unlock_irqrestore(&bank->slock, flags);
>  		clk_disable(bank->clk);
>  		return -EINVAL;
>  	}
> @@ -2101,7 +2101,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);
> +	raw_spin_unlock_irqrestore(&bank->slock, flags);
>  	clk_disable(bank->clk);
> 
>  	return 0;
> @@ -2383,7 +2383,7 @@ static struct rockchip_pin_ctrl
> *rockchip_pinctrl_get_soc_data( for (i = 0; i < ctrl->nr_banks; ++i,
> ++bank) {
>  		int bank_pins = 0;
> 
> -		spin_lock_init(&bank->slock);
> +		raw_spin_lock_init(&bank->slock);
>  		bank->drvdata = d;
>  		bank->pin_base = ctrl->nr_pins;
>  		ctrl->nr_pins += bank->nr_pins;

Re: [RFC PATCH 2/4] pinctrl: rockchip: convert to raw spinlock

[Heiko Stuebner]

Am Mittwoch, 15. März 2017, 17:28:56 CET schrieb Heiko Stuebner:
> Am Montag, 13. März 2017, 18:38:11 CET schrieb John Keeping:
> > This lock is used from rockchip_irq_set_type() which is part of the
> > irq_chip implementation and thus must use raw_spinlock_t as documented
> > in Documentation/gpio/driver.txt.
> > 
> > Signed-off-by: John Keeping <john@metanate.com>
> 
> Looks good
> Reviewed-by: Heiko Stuebner <heiko@sntech.de>

I have to take that back.
This leaves the spin_lock in the schmitt trigger unconverted and produces 
compile issues.

So it either needs to be converted or removed - I guess it should be removed.

> 
> > ---
> > 
> >  drivers/pinctrl/pinctrl-rockchip.c | 30 +++++++++++++++---------------
> >  1 file changed, 15 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/pinctrl/pinctrl-rockchip.c
> > b/drivers/pinctrl/pinctrl-rockchip.c index 1defe83a5c4d..2f963aea64b2
> > 100644
> > --- a/drivers/pinctrl/pinctrl-rockchip.c
> > +++ b/drivers/pinctrl/pinctrl-rockchip.c
> > @@ -163,7 +163,7 @@ struct rockchip_pin_bank {
> > 
> >  	struct irq_domain		*domain;
> >  	struct gpio_chip		gpio_chip;
> >  	struct pinctrl_gpio_range	grange;
> > 
> > -	spinlock_t			slock;
> > +	raw_spinlock_t			slock;
> > 
> >  	u32				toggle_edge_mode;
> >  
> >  };
> > 
> > @@ -1292,14 +1292,14 @@ static int rockchip_set_pull(struct
> > rockchip_pin_bank *bank,
> > 
> >  	switch (ctrl->type) {
> > 
> >  	case RK2928:
> > -		spin_lock_irqsave(&bank->slock, flags);
> > +		raw_spin_lock_irqsave(&bank->slock, flags);
> > 
> >  		data = BIT(bit + 16);
> >  		if (pull == PIN_CONFIG_BIAS_DISABLE)
> >  		
> >  			data |= BIT(bit);
> >  		
> >  		ret = regmap_write(regmap, reg, data);
> > 
> > -		spin_unlock_irqrestore(&bank->slock, flags);
> > +		raw_spin_unlock_irqrestore(&bank->slock, flags);
> > 
> >  		break;
> >  	
> >  	case RK1108:
> > 
> >  	case RK3188:
> > @@ -1433,7 +1433,7 @@ static int _rockchip_pmx_gpio_set_direction(struct
> > gpio_chip *chip, return ret;
> > 
> >  	clk_enable(bank->clk);
> > 
> > -	spin_lock_irqsave(&bank->slock, flags);
> > +	raw_spin_lock_irqsave(&bank->slock, flags);
> > 
> >  	data = readl_relaxed(bank->reg_base + GPIO_SWPORT_DDR);
> >  	/* set bit to 1 for output, 0 for input */
> > 
> > @@ -1443,7 +1443,7 @@ 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);
> > +	raw_spin_unlock_irqrestore(&bank->slock, flags);
> > 
> >  	clk_disable(bank->clk);
> >  	
> >  	return 0;
> > 
> > @@ -1874,7 +1874,7 @@ static void rockchip_gpio_set(struct gpio_chip *gc,
> > unsigned offset, int value) u32 data;
> > 
> >  	clk_enable(bank->clk);
> > 
> > -	spin_lock_irqsave(&bank->slock, flags);
> > +	raw_spin_lock_irqsave(&bank->slock, flags);
> > 
> >  	data = readl(reg);
> >  	data &= ~BIT(offset);
> > 
> > @@ -1882,7 +1882,7 @@ static void rockchip_gpio_set(struct gpio_chip *gc,
> > unsigned offset, int value) data |= BIT(offset);
> > 
> >  	writel(data, reg);
> > 
> > -	spin_unlock_irqrestore(&bank->slock, flags);
> > +	raw_spin_unlock_irqrestore(&bank->slock, flags);
> > 
> >  	clk_disable(bank->clk);
> >  
> >  }
> > 
> > @@ -1994,7 +1994,7 @@ static void rockchip_irq_demux(struct irq_desc
> > *desc)
> > 
> >  			data = readl_relaxed(bank->reg_base + GPIO_EXT_PORT);
> >  			do {
> > 
> > -				spin_lock_irqsave(&bank->slock, flags);
> > +				raw_spin_lock_irqsave(&bank->slock, flags);
> > 
> >  				polarity = readl_relaxed(bank->reg_base +
> >  				
> >  							 GPIO_INT_POLARITY);
> > 
> > @@ -2005,7 +2005,7 @@ static void rockchip_irq_demux(struct irq_desc
> > *desc)
> > 
> >  				writel(polarity,
> >  				
> >  				       bank->reg_base + GPIO_INT_POLARITY);
> > 
> > -				spin_unlock_irqrestore(&bank->slock, flags);
> > +				raw_spin_unlock_irqrestore(&bank->slock, flags);
> > 
> >  				data_old = data;
> >  				data = readl_relaxed(bank->reg_base +
> > 
> > @@ -2036,20 +2036,20 @@ static int rockchip_irq_set_type(struct irq_data
> > *d, unsigned int type) return ret;
> > 
> >  	clk_enable(bank->clk);
> > 
> > -	spin_lock_irqsave(&bank->slock, flags);
> > +	raw_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);
> > +	raw_spin_unlock_irqrestore(&bank->slock, flags);
> > 
> >  	if (type & IRQ_TYPE_EDGE_BOTH)
> >  	
> >  		irq_set_handler_locked(d, handle_edge_irq);
> >  	
> >  	else
> >  	
> >  		irq_set_handler_locked(d, handle_level_irq);
> > 
> > -	spin_lock_irqsave(&bank->slock, flags);
> > +	raw_spin_lock_irqsave(&bank->slock, flags);
> > 
> >  	irq_gc_lock(gc);
> >  	
> >  	level = readl_relaxed(gc->reg_base + GPIO_INTTYPE_LEVEL);
> > 
> > @@ -2092,7 +2092,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);
> > +		raw_spin_unlock_irqrestore(&bank->slock, flags);
> > 
> >  		clk_disable(bank->clk);
> >  		return -EINVAL;
> >  	
> >  	}
> > 
> > @@ -2101,7 +2101,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);
> > +	raw_spin_unlock_irqrestore(&bank->slock, flags);
> > 
> >  	clk_disable(bank->clk);
> >  	
> >  	return 0;
> > 
> > @@ -2383,7 +2383,7 @@ static struct rockchip_pin_ctrl
> > *rockchip_pinctrl_get_soc_data( for (i = 0; i < ctrl->nr_banks; ++i,
> > ++bank) {
> > 
> >  		int bank_pins = 0;
> > 
> > -		spin_lock_init(&bank->slock);
> > +		raw_spin_lock_init(&bank->slock);
> > 
> >  		bank->drvdata = d;
> >  		bank->pin_base = ctrl->nr_pins;
> >  		ctrl->nr_pins += bank->nr_pins;

Re: [RFC PATCH 2/4] pinctrl: rockchip: convert to raw spinlock

[Heiko Stuebner]

Am Mittwoch, 15. März 2017, 17:28:56 CET schrieb Heiko Stuebner:
> Am Montag, 13. März 2017, 18:38:11 CET schrieb John Keeping:
> > This lock is used from rockchip_irq_set_type() which is part of the
> > irq_chip implementation and thus must use raw_spinlock_t as documented
> > in Documentation/gpio/driver.txt.
> > 
> > Signed-off-by: John Keeping <john@metanate.com>
> 
> Looks good
> Reviewed-by: Heiko Stuebner <heiko@sntech.de>

I have to take that back.
This leaves the spin_lock in the schmitt trigger unconverted and produces 
compile issues.

So it either needs to be converted or removed - I guess it should be removed.

> 
> > ---
> > 
> >  drivers/pinctrl/pinctrl-rockchip.c | 30 +++++++++++++++---------------
> >  1 file changed, 15 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/pinctrl/pinctrl-rockchip.c
> > b/drivers/pinctrl/pinctrl-rockchip.c index 1defe83a5c4d..2f963aea64b2
> > 100644
> > --- a/drivers/pinctrl/pinctrl-rockchip.c
> > +++ b/drivers/pinctrl/pinctrl-rockchip.c
> > @@ -163,7 +163,7 @@ struct rockchip_pin_bank {
> > 
> >  	struct irq_domain		*domain;
> >  	struct gpio_chip		gpio_chip;
> >  	struct pinctrl_gpio_range	grange;
> > 
> > -	spinlock_t			slock;
> > +	raw_spinlock_t			slock;
> > 
> >  	u32				toggle_edge_mode;
> >  
> >  };
> > 
> > @@ -1292,14 +1292,14 @@ static int rockchip_set_pull(struct
> > rockchip_pin_bank *bank,
> > 
> >  	switch (ctrl->type) {
> > 
> >  	case RK2928:
> > -		spin_lock_irqsave(&bank->slock, flags);
> > +		raw_spin_lock_irqsave(&bank->slock, flags);
> > 
> >  		data = BIT(bit + 16);
> >  		if (pull == PIN_CONFIG_BIAS_DISABLE)
> >  		
> >  			data |= BIT(bit);
> >  		
> >  		ret = regmap_write(regmap, reg, data);
> > 
> > -		spin_unlock_irqrestore(&bank->slock, flags);
> > +		raw_spin_unlock_irqrestore(&bank->slock, flags);
> > 
> >  		break;
> >  	
> >  	case RK1108:
> > 
> >  	case RK3188:
> > @@ -1433,7 +1433,7 @@ static int _rockchip_pmx_gpio_set_direction(struct
> > gpio_chip *chip, return ret;
> > 
> >  	clk_enable(bank->clk);
> > 
> > -	spin_lock_irqsave(&bank->slock, flags);
> > +	raw_spin_lock_irqsave(&bank->slock, flags);
> > 
> >  	data = readl_relaxed(bank->reg_base + GPIO_SWPORT_DDR);
> >  	/* set bit to 1 for output, 0 for input */
> > 
> > @@ -1443,7 +1443,7 @@ 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);
> > +	raw_spin_unlock_irqrestore(&bank->slock, flags);
> > 
> >  	clk_disable(bank->clk);
> >  	
> >  	return 0;
> > 
> > @@ -1874,7 +1874,7 @@ static void rockchip_gpio_set(struct gpio_chip *gc,
> > unsigned offset, int value) u32 data;
> > 
> >  	clk_enable(bank->clk);
> > 
> > -	spin_lock_irqsave(&bank->slock, flags);
> > +	raw_spin_lock_irqsave(&bank->slock, flags);
> > 
> >  	data = readl(reg);
> >  	data &= ~BIT(offset);
> > 
> > @@ -1882,7 +1882,7 @@ static void rockchip_gpio_set(struct gpio_chip *gc,
> > unsigned offset, int value) data |= BIT(offset);
> > 
> >  	writel(data, reg);
> > 
> > -	spin_unlock_irqrestore(&bank->slock, flags);
> > +	raw_spin_unlock_irqrestore(&bank->slock, flags);
> > 
> >  	clk_disable(bank->clk);
> >  
> >  }
> > 
> > @@ -1994,7 +1994,7 @@ static void rockchip_irq_demux(struct irq_desc
> > *desc)
> > 
> >  			data = readl_relaxed(bank->reg_base + GPIO_EXT_PORT);
> >  			do {
> > 
> > -				spin_lock_irqsave(&bank->slock, flags);
> > +				raw_spin_lock_irqsave(&bank->slock, flags);
> > 
> >  				polarity = readl_relaxed(bank->reg_base +
> >  				
> >  							 GPIO_INT_POLARITY);
> > 
> > @@ -2005,7 +2005,7 @@ static void rockchip_irq_demux(struct irq_desc
> > *desc)
> > 
> >  				writel(polarity,
> >  				
> >  				       bank->reg_base + GPIO_INT_POLARITY);
> > 
> > -				spin_unlock_irqrestore(&bank->slock, flags);
> > +				raw_spin_unlock_irqrestore(&bank->slock, flags);
> > 
> >  				data_old = data;
> >  				data = readl_relaxed(bank->reg_base +
> > 
> > @@ -2036,20 +2036,20 @@ static int rockchip_irq_set_type(struct irq_data
> > *d, unsigned int type) return ret;
> > 
> >  	clk_enable(bank->clk);
> > 
> > -	spin_lock_irqsave(&bank->slock, flags);
> > +	raw_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);
> > +	raw_spin_unlock_irqrestore(&bank->slock, flags);
> > 
> >  	if (type & IRQ_TYPE_EDGE_BOTH)
> >  	
> >  		irq_set_handler_locked(d, handle_edge_irq);
> >  	
> >  	else
> >  	
> >  		irq_set_handler_locked(d, handle_level_irq);
> > 
> > -	spin_lock_irqsave(&bank->slock, flags);
> > +	raw_spin_lock_irqsave(&bank->slock, flags);
> > 
> >  	irq_gc_lock(gc);
> >  	
> >  	level = readl_relaxed(gc->reg_base + GPIO_INTTYPE_LEVEL);
> > 
> > @@ -2092,7 +2092,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);
> > +		raw_spin_unlock_irqrestore(&bank->slock, flags);
> > 
> >  		clk_disable(bank->clk);
> >  		return -EINVAL;
> >  	
> >  	}
> > 
> > @@ -2101,7 +2101,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);
> > +	raw_spin_unlock_irqrestore(&bank->slock, flags);
> > 
> >  	clk_disable(bank->clk);
> >  	
> >  	return 0;
> > 
> > @@ -2383,7 +2383,7 @@ static struct rockchip_pin_ctrl
> > *rockchip_pinctrl_get_soc_data( for (i = 0; i < ctrl->nr_banks; ++i,
> > ++bank) {
> > 
> >  		int bank_pins = 0;
> > 
> > -		spin_lock_init(&bank->slock);
> > +		raw_spin_lock_init(&bank->slock);
> > 
> >  		bank->drvdata = d;
> >  		bank->pin_base = ctrl->nr_pins;
> >  		ctrl->nr_pins += bank->nr_pins;

[RFC PATCH 2/4] pinctrl: rockchip: convert to raw spinlock

[Heiko Stuebner]

Am Mittwoch, 15. M?rz 2017, 17:28:56 CET schrieb Heiko Stuebner:
> Am Montag, 13. M?rz 2017, 18:38:11 CET schrieb John Keeping:
> > This lock is used from rockchip_irq_set_type() which is part of the
> > irq_chip implementation and thus must use raw_spinlock_t as documented
> > in Documentation/gpio/driver.txt.
> > 
> > Signed-off-by: John Keeping <john@metanate.com>
> 
> Looks good
> Reviewed-by: Heiko Stuebner <heiko@sntech.de>

I have to take that back.
This leaves the spin_lock in the schmitt trigger unconverted and produces 
compile issues.

So it either needs to be converted or removed - I guess it should be removed.

> 
> > ---
> > 
> >  drivers/pinctrl/pinctrl-rockchip.c | 30 +++++++++++++++---------------
> >  1 file changed, 15 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/pinctrl/pinctrl-rockchip.c
> > b/drivers/pinctrl/pinctrl-rockchip.c index 1defe83a5c4d..2f963aea64b2
> > 100644
> > --- a/drivers/pinctrl/pinctrl-rockchip.c
> > +++ b/drivers/pinctrl/pinctrl-rockchip.c
> > @@ -163,7 +163,7 @@ struct rockchip_pin_bank {
> > 
> >  	struct irq_domain		*domain;
> >  	struct gpio_chip		gpio_chip;
> >  	struct pinctrl_gpio_range	grange;
> > 
> > -	spinlock_t			slock;
> > +	raw_spinlock_t			slock;
> > 
> >  	u32				toggle_edge_mode;
> >  
> >  };
> > 
> > @@ -1292,14 +1292,14 @@ static int rockchip_set_pull(struct
> > rockchip_pin_bank *bank,
> > 
> >  	switch (ctrl->type) {
> > 
> >  	case RK2928:
> > -		spin_lock_irqsave(&bank->slock, flags);
> > +		raw_spin_lock_irqsave(&bank->slock, flags);
> > 
> >  		data = BIT(bit + 16);
> >  		if (pull == PIN_CONFIG_BIAS_DISABLE)
> >  		
> >  			data |= BIT(bit);
> >  		
> >  		ret = regmap_write(regmap, reg, data);
> > 
> > -		spin_unlock_irqrestore(&bank->slock, flags);
> > +		raw_spin_unlock_irqrestore(&bank->slock, flags);
> > 
> >  		break;
> >  	
> >  	case RK1108:
> > 
> >  	case RK3188:
> > @@ -1433,7 +1433,7 @@ static int _rockchip_pmx_gpio_set_direction(struct
> > gpio_chip *chip, return ret;
> > 
> >  	clk_enable(bank->clk);
> > 
> > -	spin_lock_irqsave(&bank->slock, flags);
> > +	raw_spin_lock_irqsave(&bank->slock, flags);
> > 
> >  	data = readl_relaxed(bank->reg_base + GPIO_SWPORT_DDR);
> >  	/* set bit to 1 for output, 0 for input */
> > 
> > @@ -1443,7 +1443,7 @@ 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);
> > +	raw_spin_unlock_irqrestore(&bank->slock, flags);
> > 
> >  	clk_disable(bank->clk);
> >  	
> >  	return 0;
> > 
> > @@ -1874,7 +1874,7 @@ static void rockchip_gpio_set(struct gpio_chip *gc,
> > unsigned offset, int value) u32 data;
> > 
> >  	clk_enable(bank->clk);
> > 
> > -	spin_lock_irqsave(&bank->slock, flags);
> > +	raw_spin_lock_irqsave(&bank->slock, flags);
> > 
> >  	data = readl(reg);
> >  	data &= ~BIT(offset);
> > 
> > @@ -1882,7 +1882,7 @@ static void rockchip_gpio_set(struct gpio_chip *gc,
> > unsigned offset, int value) data |= BIT(offset);
> > 
> >  	writel(data, reg);
> > 
> > -	spin_unlock_irqrestore(&bank->slock, flags);
> > +	raw_spin_unlock_irqrestore(&bank->slock, flags);
> > 
> >  	clk_disable(bank->clk);
> >  
> >  }
> > 
> > @@ -1994,7 +1994,7 @@ static void rockchip_irq_demux(struct irq_desc
> > *desc)
> > 
> >  			data = readl_relaxed(bank->reg_base + GPIO_EXT_PORT);
> >  			do {
> > 
> > -				spin_lock_irqsave(&bank->slock, flags);
> > +				raw_spin_lock_irqsave(&bank->slock, flags);
> > 
> >  				polarity = readl_relaxed(bank->reg_base +
> >  				
> >  							 GPIO_INT_POLARITY);
> > 
> > @@ -2005,7 +2005,7 @@ static void rockchip_irq_demux(struct irq_desc
> > *desc)
> > 
> >  				writel(polarity,
> >  				
> >  				       bank->reg_base + GPIO_INT_POLARITY);
> > 
> > -				spin_unlock_irqrestore(&bank->slock, flags);
> > +				raw_spin_unlock_irqrestore(&bank->slock, flags);
> > 
> >  				data_old = data;
> >  				data = readl_relaxed(bank->reg_base +
> > 
> > @@ -2036,20 +2036,20 @@ static int rockchip_irq_set_type(struct irq_data
> > *d, unsigned int type) return ret;
> > 
> >  	clk_enable(bank->clk);
> > 
> > -	spin_lock_irqsave(&bank->slock, flags);
> > +	raw_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);
> > +	raw_spin_unlock_irqrestore(&bank->slock, flags);
> > 
> >  	if (type & IRQ_TYPE_EDGE_BOTH)
> >  	
> >  		irq_set_handler_locked(d, handle_edge_irq);
> >  	
> >  	else
> >  	
> >  		irq_set_handler_locked(d, handle_level_irq);
> > 
> > -	spin_lock_irqsave(&bank->slock, flags);
> > +	raw_spin_lock_irqsave(&bank->slock, flags);
> > 
> >  	irq_gc_lock(gc);
> >  	
> >  	level = readl_relaxed(gc->reg_base + GPIO_INTTYPE_LEVEL);
> > 
> > @@ -2092,7 +2092,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);
> > +		raw_spin_unlock_irqrestore(&bank->slock, flags);
> > 
> >  		clk_disable(bank->clk);
> >  		return -EINVAL;
> >  	
> >  	}
> > 
> > @@ -2101,7 +2101,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);
> > +	raw_spin_unlock_irqrestore(&bank->slock, flags);
> > 
> >  	clk_disable(bank->clk);
> >  	
> >  	return 0;
> > 
> > @@ -2383,7 +2383,7 @@ static struct rockchip_pin_ctrl
> > *rockchip_pinctrl_get_soc_data( for (i = 0; i < ctrl->nr_banks; ++i,
> > ++bank) {
> > 
> >  		int bank_pins = 0;
> > 
> > -		spin_lock_init(&bank->slock);
> > +		raw_spin_lock_init(&bank->slock);
> > 
> >  		bank->drvdata = d;
> >  		bank->pin_base = ctrl->nr_pins;
> >  		ctrl->nr_pins += bank->nr_pins;

Re: [RFC PATCH 2/4] pinctrl: rockchip: convert to raw spinlock

[Heiko Stuebner]

Am Mittwoch, 15. März 2017, 17:41:21 CET schrieb Heiko Stuebner:
> Am Mittwoch, 15. März 2017, 17:28:56 CET schrieb Heiko Stuebner:
> > Am Montag, 13. März 2017, 18:38:11 CET schrieb John Keeping:
> > > This lock is used from rockchip_irq_set_type() which is part of the
> > > irq_chip implementation and thus must use raw_spinlock_t as documented
> > > in Documentation/gpio/driver.txt.
> > > 
> > > Signed-off-by: John Keeping <john@metanate.com>
> > 
> > Looks good
> > Reviewed-by: Heiko Stuebner <heiko@sntech.de>
> 
> I have to take that back.
> This leaves the spin_lock in the schmitt trigger unconverted and produces
> compile issues.
> 
> So it either needs to be converted or removed - I guess it should be
> removed.

Sorry for spamming, but forgot to add, with that problem above fixed, this 
patch (and all others) can of course keep my Reviewed-by :-)


Heiko

> > > ---
> > > 
> > >  drivers/pinctrl/pinctrl-rockchip.c | 30 +++++++++++++++---------------
> > >  1 file changed, 15 insertions(+), 15 deletions(-)
> > > 
> > > diff --git a/drivers/pinctrl/pinctrl-rockchip.c
> > > b/drivers/pinctrl/pinctrl-rockchip.c index 1defe83a5c4d..2f963aea64b2
> > > 100644
> > > --- a/drivers/pinctrl/pinctrl-rockchip.c
> > > +++ b/drivers/pinctrl/pinctrl-rockchip.c
> > > @@ -163,7 +163,7 @@ struct rockchip_pin_bank {
> > > 
> > >  	struct irq_domain		*domain;
> > >  	struct gpio_chip		gpio_chip;
> > >  	struct pinctrl_gpio_range	grange;
> > > 
> > > -	spinlock_t			slock;
> > > +	raw_spinlock_t			slock;
> > > 
> > >  	u32				toggle_edge_mode;
> > >  
> > >  };
> > > 
> > > @@ -1292,14 +1292,14 @@ static int rockchip_set_pull(struct
> > > rockchip_pin_bank *bank,
> > > 
> > >  	switch (ctrl->type) {
> > > 
> > >  	case RK2928:
> > > -		spin_lock_irqsave(&bank->slock, flags);
> > > +		raw_spin_lock_irqsave(&bank->slock, flags);
> > > 
> > >  		data = BIT(bit + 16);
> > >  		if (pull == PIN_CONFIG_BIAS_DISABLE)
> > >  		
> > >  			data |= BIT(bit);
> > >  		
> > >  		ret = regmap_write(regmap, reg, data);
> > > 
> > > -		spin_unlock_irqrestore(&bank->slock, flags);
> > > +		raw_spin_unlock_irqrestore(&bank->slock, flags);
> > > 
> > >  		break;
> > >  	
> > >  	case RK1108:
> > > 
> > >  	case RK3188:
> > > @@ -1433,7 +1433,7 @@ static int _rockchip_pmx_gpio_set_direction(struct
> > > gpio_chip *chip, return ret;
> > > 
> > >  	clk_enable(bank->clk);
> > > 
> > > -	spin_lock_irqsave(&bank->slock, flags);
> > > +	raw_spin_lock_irqsave(&bank->slock, flags);
> > > 
> > >  	data = readl_relaxed(bank->reg_base + GPIO_SWPORT_DDR);
> > >  	/* set bit to 1 for output, 0 for input */
> > > 
> > > @@ -1443,7 +1443,7 @@ 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);
> > > +	raw_spin_unlock_irqrestore(&bank->slock, flags);
> > > 
> > >  	clk_disable(bank->clk);
> > >  	
> > >  	return 0;
> > > 
> > > @@ -1874,7 +1874,7 @@ static void rockchip_gpio_set(struct gpio_chip
> > > *gc,
> > > unsigned offset, int value) u32 data;
> > > 
> > >  	clk_enable(bank->clk);
> > > 
> > > -	spin_lock_irqsave(&bank->slock, flags);
> > > +	raw_spin_lock_irqsave(&bank->slock, flags);
> > > 
> > >  	data = readl(reg);
> > >  	data &= ~BIT(offset);
> > > 
> > > @@ -1882,7 +1882,7 @@ static void rockchip_gpio_set(struct gpio_chip
> > > *gc,
> > > unsigned offset, int value) data |= BIT(offset);
> > > 
> > >  	writel(data, reg);
> > > 
> > > -	spin_unlock_irqrestore(&bank->slock, flags);
> > > +	raw_spin_unlock_irqrestore(&bank->slock, flags);
> > > 
> > >  	clk_disable(bank->clk);
> > >  
> > >  }
> > > 
> > > @@ -1994,7 +1994,7 @@ static void rockchip_irq_demux(struct irq_desc
> > > *desc)
> > > 
> > >  			data = readl_relaxed(bank->reg_base + GPIO_EXT_PORT);
> > >  			do {
> > > 
> > > -				spin_lock_irqsave(&bank->slock, flags);
> > > +				raw_spin_lock_irqsave(&bank->slock, flags);
> > > 
> > >  				polarity = readl_relaxed(bank->reg_base +
> > >  				
> > >  							 GPIO_INT_POLARITY);
> > > 
> > > @@ -2005,7 +2005,7 @@ static void rockchip_irq_demux(struct irq_desc
> > > *desc)
> > > 
> > >  				writel(polarity,
> > >  				
> > >  				       bank->reg_base + GPIO_INT_POLARITY);
> > > 
> > > -				spin_unlock_irqrestore(&bank->slock, flags);
> > > +				raw_spin_unlock_irqrestore(&bank->slock, flags);
> > > 
> > >  				data_old = data;
> > >  				data = readl_relaxed(bank->reg_base +
> > > 
> > > @@ -2036,20 +2036,20 @@ static int rockchip_irq_set_type(struct irq_data
> > > *d, unsigned int type) return ret;
> > > 
> > >  	clk_enable(bank->clk);
> > > 
> > > -	spin_lock_irqsave(&bank->slock, flags);
> > > +	raw_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);
> > > +	raw_spin_unlock_irqrestore(&bank->slock, flags);
> > > 
> > >  	if (type & IRQ_TYPE_EDGE_BOTH)
> > >  	
> > >  		irq_set_handler_locked(d, handle_edge_irq);
> > >  	
> > >  	else
> > >  	
> > >  		irq_set_handler_locked(d, handle_level_irq);
> > > 
> > > -	spin_lock_irqsave(&bank->slock, flags);
> > > +	raw_spin_lock_irqsave(&bank->slock, flags);
> > > 
> > >  	irq_gc_lock(gc);
> > >  	
> > >  	level = readl_relaxed(gc->reg_base + GPIO_INTTYPE_LEVEL);
> > > 
> > > @@ -2092,7 +2092,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);
> > > +		raw_spin_unlock_irqrestore(&bank->slock, flags);
> > > 
> > >  		clk_disable(bank->clk);
> > >  		return -EINVAL;
> > >  	
> > >  	}
> > > 
> > > @@ -2101,7 +2101,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);
> > > +	raw_spin_unlock_irqrestore(&bank->slock, flags);
> > > 
> > >  	clk_disable(bank->clk);
> > >  	
> > >  	return 0;
> > > 
> > > @@ -2383,7 +2383,7 @@ static struct rockchip_pin_ctrl
> > > *rockchip_pinctrl_get_soc_data( for (i = 0; i < ctrl->nr_banks; ++i,
> > > ++bank) {
> > > 
> > >  		int bank_pins = 0;
> > > 
> > > -		spin_lock_init(&bank->slock);
> > > +		raw_spin_lock_init(&bank->slock);
> > > 
> > >  		bank->drvdata = d;
> > >  		bank->pin_base = ctrl->nr_pins;
> > >  		ctrl->nr_pins += bank->nr_pins;

[RFC PATCH 2/4] pinctrl: rockchip: convert to raw spinlock

[Heiko Stuebner]

Am Mittwoch, 15. M?rz 2017, 17:41:21 CET schrieb Heiko Stuebner:
> Am Mittwoch, 15. M?rz 2017, 17:28:56 CET schrieb Heiko Stuebner:
> > Am Montag, 13. M?rz 2017, 18:38:11 CET schrieb John Keeping:
> > > This lock is used from rockchip_irq_set_type() which is part of the
> > > irq_chip implementation and thus must use raw_spinlock_t as documented
> > > in Documentation/gpio/driver.txt.
> > > 
> > > Signed-off-by: John Keeping <john@metanate.com>
> > 
> > Looks good
> > Reviewed-by: Heiko Stuebner <heiko@sntech.de>
> 
> I have to take that back.
> This leaves the spin_lock in the schmitt trigger unconverted and produces
> compile issues.
> 
> So it either needs to be converted or removed - I guess it should be
> removed.

Sorry for spamming, but forgot to add, with that problem above fixed, this 
patch (and all others) can of course keep my Reviewed-by :-)


Heiko

> > > ---
> > > 
> > >  drivers/pinctrl/pinctrl-rockchip.c | 30 +++++++++++++++---------------
> > >  1 file changed, 15 insertions(+), 15 deletions(-)
> > > 
> > > diff --git a/drivers/pinctrl/pinctrl-rockchip.c
> > > b/drivers/pinctrl/pinctrl-rockchip.c index 1defe83a5c4d..2f963aea64b2
> > > 100644
> > > --- a/drivers/pinctrl/pinctrl-rockchip.c
> > > +++ b/drivers/pinctrl/pinctrl-rockchip.c
> > > @@ -163,7 +163,7 @@ struct rockchip_pin_bank {
> > > 
> > >  	struct irq_domain		*domain;
> > >  	struct gpio_chip		gpio_chip;
> > >  	struct pinctrl_gpio_range	grange;
> > > 
> > > -	spinlock_t			slock;
> > > +	raw_spinlock_t			slock;
> > > 
> > >  	u32				toggle_edge_mode;
> > >  
> > >  };
> > > 
> > > @@ -1292,14 +1292,14 @@ static int rockchip_set_pull(struct
> > > rockchip_pin_bank *bank,
> > > 
> > >  	switch (ctrl->type) {
> > > 
> > >  	case RK2928:
> > > -		spin_lock_irqsave(&bank->slock, flags);
> > > +		raw_spin_lock_irqsave(&bank->slock, flags);
> > > 
> > >  		data = BIT(bit + 16);
> > >  		if (pull == PIN_CONFIG_BIAS_DISABLE)
> > >  		
> > >  			data |= BIT(bit);
> > >  		
> > >  		ret = regmap_write(regmap, reg, data);
> > > 
> > > -		spin_unlock_irqrestore(&bank->slock, flags);
> > > +		raw_spin_unlock_irqrestore(&bank->slock, flags);
> > > 
> > >  		break;
> > >  	
> > >  	case RK1108:
> > > 
> > >  	case RK3188:
> > > @@ -1433,7 +1433,7 @@ static int _rockchip_pmx_gpio_set_direction(struct
> > > gpio_chip *chip, return ret;
> > > 
> > >  	clk_enable(bank->clk);
> > > 
> > > -	spin_lock_irqsave(&bank->slock, flags);
> > > +	raw_spin_lock_irqsave(&bank->slock, flags);
> > > 
> > >  	data = readl_relaxed(bank->reg_base + GPIO_SWPORT_DDR);
> > >  	/* set bit to 1 for output, 0 for input */
> > > 
> > > @@ -1443,7 +1443,7 @@ 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);
> > > +	raw_spin_unlock_irqrestore(&bank->slock, flags);
> > > 
> > >  	clk_disable(bank->clk);
> > >  	
> > >  	return 0;
> > > 
> > > @@ -1874,7 +1874,7 @@ static void rockchip_gpio_set(struct gpio_chip
> > > *gc,
> > > unsigned offset, int value) u32 data;
> > > 
> > >  	clk_enable(bank->clk);
> > > 
> > > -	spin_lock_irqsave(&bank->slock, flags);
> > > +	raw_spin_lock_irqsave(&bank->slock, flags);
> > > 
> > >  	data = readl(reg);
> > >  	data &= ~BIT(offset);
> > > 
> > > @@ -1882,7 +1882,7 @@ static void rockchip_gpio_set(struct gpio_chip
> > > *gc,
> > > unsigned offset, int value) data |= BIT(offset);
> > > 
> > >  	writel(data, reg);
> > > 
> > > -	spin_unlock_irqrestore(&bank->slock, flags);
> > > +	raw_spin_unlock_irqrestore(&bank->slock, flags);
> > > 
> > >  	clk_disable(bank->clk);
> > >  
> > >  }
> > > 
> > > @@ -1994,7 +1994,7 @@ static void rockchip_irq_demux(struct irq_desc
> > > *desc)
> > > 
> > >  			data = readl_relaxed(bank->reg_base + GPIO_EXT_PORT);
> > >  			do {
> > > 
> > > -				spin_lock_irqsave(&bank->slock, flags);
> > > +				raw_spin_lock_irqsave(&bank->slock, flags);
> > > 
> > >  				polarity = readl_relaxed(bank->reg_base +
> > >  				
> > >  							 GPIO_INT_POLARITY);
> > > 
> > > @@ -2005,7 +2005,7 @@ static void rockchip_irq_demux(struct irq_desc
> > > *desc)
> > > 
> > >  				writel(polarity,
> > >  				
> > >  				       bank->reg_base + GPIO_INT_POLARITY);
> > > 
> > > -				spin_unlock_irqrestore(&bank->slock, flags);
> > > +				raw_spin_unlock_irqrestore(&bank->slock, flags);
> > > 
> > >  				data_old = data;
> > >  				data = readl_relaxed(bank->reg_base +
> > > 
> > > @@ -2036,20 +2036,20 @@ static int rockchip_irq_set_type(struct irq_data
> > > *d, unsigned int type) return ret;
> > > 
> > >  	clk_enable(bank->clk);
> > > 
> > > -	spin_lock_irqsave(&bank->slock, flags);
> > > +	raw_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);
> > > +	raw_spin_unlock_irqrestore(&bank->slock, flags);
> > > 
> > >  	if (type & IRQ_TYPE_EDGE_BOTH)
> > >  	
> > >  		irq_set_handler_locked(d, handle_edge_irq);
> > >  	
> > >  	else
> > >  	
> > >  		irq_set_handler_locked(d, handle_level_irq);
> > > 
> > > -	spin_lock_irqsave(&bank->slock, flags);
> > > +	raw_spin_lock_irqsave(&bank->slock, flags);
> > > 
> > >  	irq_gc_lock(gc);
> > >  	
> > >  	level = readl_relaxed(gc->reg_base + GPIO_INTTYPE_LEVEL);
> > > 
> > > @@ -2092,7 +2092,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);
> > > +		raw_spin_unlock_irqrestore(&bank->slock, flags);
> > > 
> > >  		clk_disable(bank->clk);
> > >  		return -EINVAL;
> > >  	
> > >  	}
> > > 
> > > @@ -2101,7 +2101,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);
> > > +	raw_spin_unlock_irqrestore(&bank->slock, flags);
> > > 
> > >  	clk_disable(bank->clk);
> > >  	
> > >  	return 0;
> > > 
> > > @@ -2383,7 +2383,7 @@ static struct rockchip_pin_ctrl
> > > *rockchip_pinctrl_get_soc_data( for (i = 0; i < ctrl->nr_banks; ++i,
> > > ++bank) {
> > > 
> > >  		int bank_pins = 0;
> > > 
> > > -		spin_lock_init(&bank->slock);
> > > +		raw_spin_lock_init(&bank->slock);
> > > 
> > >  		bank->drvdata = d;
> > >  		bank->pin_base = ctrl->nr_pins;
> > >  		ctrl->nr_pins += bank->nr_pins;

Re: [RFC PATCH 2/4] pinctrl: rockchip: convert to raw spinlock

[John Keeping]

On Wed, 15 Mar 2017 17:41:21 +0100, Heiko Stuebner wrote:

> Am Mittwoch, 15. März 2017, 17:28:56 CET schrieb Heiko Stuebner:
> > Am Montag, 13. März 2017, 18:38:11 CET schrieb John Keeping:  
> > > This lock is used from rockchip_irq_set_type() which is part of the
> > > irq_chip implementation and thus must use raw_spinlock_t as documented
> > > in Documentation/gpio/driver.txt.
> > > 
> > > Signed-off-by: John Keeping <john@metanate.com>  
> > 
> > Looks good
> > Reviewed-by: Heiko Stuebner <heiko@sntech.de>  
> 
> I have to take that back.
> This leaves the spin_lock in the schmitt trigger unconverted and produces 
> compile issues.
> 
> So it either needs to be converted or removed - I guess it should be removed.

Yes, it looks like it should be removed - the patches adding that hadn't
been applied when I sent this out!

I'll rebase and add the schmitt trigger change to patch 1 in a day or
two, after giving a bit more time for reviews.

> > > ---
> > > 
> > >  drivers/pinctrl/pinctrl-rockchip.c | 30 +++++++++++++++---------------
> > >  1 file changed, 15 insertions(+), 15 deletions(-)
> > > 
> > > diff --git a/drivers/pinctrl/pinctrl-rockchip.c
> > > b/drivers/pinctrl/pinctrl-rockchip.c index 1defe83a5c4d..2f963aea64b2
> > > 100644
> > > --- a/drivers/pinctrl/pinctrl-rockchip.c
> > > +++ b/drivers/pinctrl/pinctrl-rockchip.c
> > > @@ -163,7 +163,7 @@ struct rockchip_pin_bank {
> > > 
> > >  	struct irq_domain		*domain;
> > >  	struct gpio_chip		gpio_chip;
> > >  	struct pinctrl_gpio_range	grange;
> > > 
> > > -	spinlock_t			slock;
> > > +	raw_spinlock_t			slock;
> > > 
> > >  	u32				toggle_edge_mode;
> > >  
> > >  };
> > > 
> > > @@ -1292,14 +1292,14 @@ static int rockchip_set_pull(struct
> > > rockchip_pin_bank *bank,
> > > 
> > >  	switch (ctrl->type) {
> > > 
> > >  	case RK2928:
> > > -		spin_lock_irqsave(&bank->slock, flags);
> > > +		raw_spin_lock_irqsave(&bank->slock, flags);
> > > 
> > >  		data = BIT(bit + 16);
> > >  		if (pull == PIN_CONFIG_BIAS_DISABLE)
> > >  		
> > >  			data |= BIT(bit);
> > >  		
> > >  		ret = regmap_write(regmap, reg, data);
> > > 
> > > -		spin_unlock_irqrestore(&bank->slock, flags);
> > > +		raw_spin_unlock_irqrestore(&bank->slock, flags);
> > > 
> > >  		break;
> > >  	
> > >  	case RK1108:
> > > 
> > >  	case RK3188:
> > > @@ -1433,7 +1433,7 @@ static int _rockchip_pmx_gpio_set_direction(struct
> > > gpio_chip *chip, return ret;
> > > 
> > >  	clk_enable(bank->clk);
> > > 
> > > -	spin_lock_irqsave(&bank->slock, flags);
> > > +	raw_spin_lock_irqsave(&bank->slock, flags);
> > > 
> > >  	data = readl_relaxed(bank->reg_base + GPIO_SWPORT_DDR);
> > >  	/* set bit to 1 for output, 0 for input */
> > > 
> > > @@ -1443,7 +1443,7 @@ 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);
> > > +	raw_spin_unlock_irqrestore(&bank->slock, flags);
> > > 
> > >  	clk_disable(bank->clk);
> > >  	
> > >  	return 0;
> > > 
> > > @@ -1874,7 +1874,7 @@ static void rockchip_gpio_set(struct gpio_chip *gc,
> > > unsigned offset, int value) u32 data;
> > > 
> > >  	clk_enable(bank->clk);
> > > 
> > > -	spin_lock_irqsave(&bank->slock, flags);
> > > +	raw_spin_lock_irqsave(&bank->slock, flags);
> > > 
> > >  	data = readl(reg);
> > >  	data &= ~BIT(offset);
> > > 
> > > @@ -1882,7 +1882,7 @@ static void rockchip_gpio_set(struct gpio_chip *gc,
> > > unsigned offset, int value) data |= BIT(offset);
> > > 
> > >  	writel(data, reg);
> > > 
> > > -	spin_unlock_irqrestore(&bank->slock, flags);
> > > +	raw_spin_unlock_irqrestore(&bank->slock, flags);
> > > 
> > >  	clk_disable(bank->clk);
> > >  
> > >  }
> > > 
> > > @@ -1994,7 +1994,7 @@ static void rockchip_irq_demux(struct irq_desc
> > > *desc)
> > > 
> > >  			data = readl_relaxed(bank->reg_base + GPIO_EXT_PORT);
> > >  			do {
> > > 
> > > -				spin_lock_irqsave(&bank->slock, flags);
> > > +				raw_spin_lock_irqsave(&bank->slock, flags);
> > > 
> > >  				polarity = readl_relaxed(bank->reg_base +
> > >  				
> > >  							 GPIO_INT_POLARITY);
> > > 
> > > @@ -2005,7 +2005,7 @@ static void rockchip_irq_demux(struct irq_desc
> > > *desc)
> > > 
> > >  				writel(polarity,
> > >  				
> > >  				       bank->reg_base + GPIO_INT_POLARITY);
> > > 
> > > -				spin_unlock_irqrestore(&bank->slock, flags);
> > > +				raw_spin_unlock_irqrestore(&bank->slock, flags);
> > > 
> > >  				data_old = data;
> > >  				data = readl_relaxed(bank->reg_base +
> > > 
> > > @@ -2036,20 +2036,20 @@ static int rockchip_irq_set_type(struct irq_data
> > > *d, unsigned int type) return ret;
> > > 
> > >  	clk_enable(bank->clk);
> > > 
> > > -	spin_lock_irqsave(&bank->slock, flags);
> > > +	raw_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);
> > > +	raw_spin_unlock_irqrestore(&bank->slock, flags);
> > > 
> > >  	if (type & IRQ_TYPE_EDGE_BOTH)
> > >  	
> > >  		irq_set_handler_locked(d, handle_edge_irq);
> > >  	
> > >  	else
> > >  	
> > >  		irq_set_handler_locked(d, handle_level_irq);
> > > 
> > > -	spin_lock_irqsave(&bank->slock, flags);
> > > +	raw_spin_lock_irqsave(&bank->slock, flags);
> > > 
> > >  	irq_gc_lock(gc);
> > >  	
> > >  	level = readl_relaxed(gc->reg_base + GPIO_INTTYPE_LEVEL);
> > > 
> > > @@ -2092,7 +2092,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);
> > > +		raw_spin_unlock_irqrestore(&bank->slock, flags);
> > > 
> > >  		clk_disable(bank->clk);
> > >  		return -EINVAL;
> > >  	
> > >  	}
> > > 
> > > @@ -2101,7 +2101,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);
> > > +	raw_spin_unlock_irqrestore(&bank->slock, flags);
> > > 
> > >  	clk_disable(bank->clk);
> > >  	
> > >  	return 0;
> > > 
> > > @@ -2383,7 +2383,7 @@ static struct rockchip_pin_ctrl
> > > *rockchip_pinctrl_get_soc_data( for (i = 0; i < ctrl->nr_banks; ++i,
> > > ++bank) {
> > > 
> > >  		int bank_pins = 0;
> > > 
> > > -		spin_lock_init(&bank->slock);
> > > +		raw_spin_lock_init(&bank->slock);
> > > 
> > >  		bank->drvdata = d;
> > >  		bank->pin_base = ctrl->nr_pins;
> > >  		ctrl->nr_pins += bank->nr_pins;  

[RFC PATCH 2/4] pinctrl: rockchip: convert to raw spinlock

[John Keeping]

On Wed, 15 Mar 2017 17:41:21 +0100, Heiko Stuebner wrote:

> Am Mittwoch, 15. M?rz 2017, 17:28:56 CET schrieb Heiko Stuebner:
> > Am Montag, 13. M?rz 2017, 18:38:11 CET schrieb John Keeping:  
> > > This lock is used from rockchip_irq_set_type() which is part of the
> > > irq_chip implementation and thus must use raw_spinlock_t as documented
> > > in Documentation/gpio/driver.txt.
> > > 
> > > Signed-off-by: John Keeping <john@metanate.com>  
> > 
> > Looks good
> > Reviewed-by: Heiko Stuebner <heiko@sntech.de>  
> 
> I have to take that back.
> This leaves the spin_lock in the schmitt trigger unconverted and produces 
> compile issues.
> 
> So it either needs to be converted or removed - I guess it should be removed.

Yes, it looks like it should be removed - the patches adding that hadn't
been applied when I sent this out!

I'll rebase and add the schmitt trigger change to patch 1 in a day or
two, after giving a bit more time for reviews.

> > > ---
> > > 
> > >  drivers/pinctrl/pinctrl-rockchip.c | 30 +++++++++++++++---------------
> > >  1 file changed, 15 insertions(+), 15 deletions(-)
> > > 
> > > diff --git a/drivers/pinctrl/pinctrl-rockchip.c
> > > b/drivers/pinctrl/pinctrl-rockchip.c index 1defe83a5c4d..2f963aea64b2
> > > 100644
> > > --- a/drivers/pinctrl/pinctrl-rockchip.c
> > > +++ b/drivers/pinctrl/pinctrl-rockchip.c
> > > @@ -163,7 +163,7 @@ struct rockchip_pin_bank {
> > > 
> > >  	struct irq_domain		*domain;
> > >  	struct gpio_chip		gpio_chip;
> > >  	struct pinctrl_gpio_range	grange;
> > > 
> > > -	spinlock_t			slock;
> > > +	raw_spinlock_t			slock;
> > > 
> > >  	u32				toggle_edge_mode;
> > >  
> > >  };
> > > 
> > > @@ -1292,14 +1292,14 @@ static int rockchip_set_pull(struct
> > > rockchip_pin_bank *bank,
> > > 
> > >  	switch (ctrl->type) {
> > > 
> > >  	case RK2928:
> > > -		spin_lock_irqsave(&bank->slock, flags);
> > > +		raw_spin_lock_irqsave(&bank->slock, flags);
> > > 
> > >  		data = BIT(bit + 16);
> > >  		if (pull == PIN_CONFIG_BIAS_DISABLE)
> > >  		
> > >  			data |= BIT(bit);
> > >  		
> > >  		ret = regmap_write(regmap, reg, data);
> > > 
> > > -		spin_unlock_irqrestore(&bank->slock, flags);
> > > +		raw_spin_unlock_irqrestore(&bank->slock, flags);
> > > 
> > >  		break;
> > >  	
> > >  	case RK1108:
> > > 
> > >  	case RK3188:
> > > @@ -1433,7 +1433,7 @@ static int _rockchip_pmx_gpio_set_direction(struct
> > > gpio_chip *chip, return ret;
> > > 
> > >  	clk_enable(bank->clk);
> > > 
> > > -	spin_lock_irqsave(&bank->slock, flags);
> > > +	raw_spin_lock_irqsave(&bank->slock, flags);
> > > 
> > >  	data = readl_relaxed(bank->reg_base + GPIO_SWPORT_DDR);
> > >  	/* set bit to 1 for output, 0 for input */
> > > 
> > > @@ -1443,7 +1443,7 @@ 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);
> > > +	raw_spin_unlock_irqrestore(&bank->slock, flags);
> > > 
> > >  	clk_disable(bank->clk);
> > >  	
> > >  	return 0;
> > > 
> > > @@ -1874,7 +1874,7 @@ static void rockchip_gpio_set(struct gpio_chip *gc,
> > > unsigned offset, int value) u32 data;
> > > 
> > >  	clk_enable(bank->clk);
> > > 
> > > -	spin_lock_irqsave(&bank->slock, flags);
> > > +	raw_spin_lock_irqsave(&bank->slock, flags);
> > > 
> > >  	data = readl(reg);
> > >  	data &= ~BIT(offset);
> > > 
> > > @@ -1882,7 +1882,7 @@ static void rockchip_gpio_set(struct gpio_chip *gc,
> > > unsigned offset, int value) data |= BIT(offset);
> > > 
> > >  	writel(data, reg);
> > > 
> > > -	spin_unlock_irqrestore(&bank->slock, flags);
> > > +	raw_spin_unlock_irqrestore(&bank->slock, flags);
> > > 
> > >  	clk_disable(bank->clk);
> > >  
> > >  }
> > > 
> > > @@ -1994,7 +1994,7 @@ static void rockchip_irq_demux(struct irq_desc
> > > *desc)
> > > 
> > >  			data = readl_relaxed(bank->reg_base + GPIO_EXT_PORT);
> > >  			do {
> > > 
> > > -				spin_lock_irqsave(&bank->slock, flags);
> > > +				raw_spin_lock_irqsave(&bank->slock, flags);
> > > 
> > >  				polarity = readl_relaxed(bank->reg_base +
> > >  				
> > >  							 GPIO_INT_POLARITY);
> > > 
> > > @@ -2005,7 +2005,7 @@ static void rockchip_irq_demux(struct irq_desc
> > > *desc)
> > > 
> > >  				writel(polarity,
> > >  				
> > >  				       bank->reg_base + GPIO_INT_POLARITY);
> > > 
> > > -				spin_unlock_irqrestore(&bank->slock, flags);
> > > +				raw_spin_unlock_irqrestore(&bank->slock, flags);
> > > 
> > >  				data_old = data;
> > >  				data = readl_relaxed(bank->reg_base +
> > > 
> > > @@ -2036,20 +2036,20 @@ static int rockchip_irq_set_type(struct irq_data
> > > *d, unsigned int type) return ret;
> > > 
> > >  	clk_enable(bank->clk);
> > > 
> > > -	spin_lock_irqsave(&bank->slock, flags);
> > > +	raw_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);
> > > +	raw_spin_unlock_irqrestore(&bank->slock, flags);
> > > 
> > >  	if (type & IRQ_TYPE_EDGE_BOTH)
> > >  	
> > >  		irq_set_handler_locked(d, handle_edge_irq);
> > >  	
> > >  	else
> > >  	
> > >  		irq_set_handler_locked(d, handle_level_irq);
> > > 
> > > -	spin_lock_irqsave(&bank->slock, flags);
> > > +	raw_spin_lock_irqsave(&bank->slock, flags);
> > > 
> > >  	irq_gc_lock(gc);
> > >  	
> > >  	level = readl_relaxed(gc->reg_base + GPIO_INTTYPE_LEVEL);
> > > 
> > > @@ -2092,7 +2092,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);
> > > +		raw_spin_unlock_irqrestore(&bank->slock, flags);
> > > 
> > >  		clk_disable(bank->clk);
> > >  		return -EINVAL;
> > >  	
> > >  	}
> > > 
> > > @@ -2101,7 +2101,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);
> > > +	raw_spin_unlock_irqrestore(&bank->slock, flags);
> > > 
> > >  	clk_disable(bank->clk);
> > >  	
> > >  	return 0;
> > > 
> > > @@ -2383,7 +2383,7 @@ static struct rockchip_pin_ctrl
> > > *rockchip_pinctrl_get_soc_data( for (i = 0; i < ctrl->nr_banks; ++i,
> > > ++bank) {
> > > 
> > >  		int bank_pins = 0;
> > > 
> > > -		spin_lock_init(&bank->slock);
> > > +		raw_spin_lock_init(&bank->slock);
> > > 
> > >  		bank->drvdata = d;
> > >  		bank->pin_base = ctrl->nr_pins;
> > >  		ctrl->nr_pins += bank->nr_pins;  

Re: [RFC PATCH 2/4] pinctrl: rockchip: convert to raw spinlock

[Heiko Stuebner]

Am Mittwoch, 15. März 2017, 16:59:00 CET schrieb John Keeping:
> On Wed, 15 Mar 2017 17:41:21 +0100, Heiko Stuebner wrote:
> > Am Mittwoch, 15. März 2017, 17:28:56 CET schrieb Heiko Stuebner:
> > > Am Montag, 13. März 2017, 18:38:11 CET schrieb John Keeping:
> > > > This lock is used from rockchip_irq_set_type() which is part of the
> > > > irq_chip implementation and thus must use raw_spinlock_t as documented
> > > > in Documentation/gpio/driver.txt.
> > > > 
> > > > Signed-off-by: John Keeping <john@metanate.com>
> > > 
> > > Looks good
> > > Reviewed-by: Heiko Stuebner <heiko@sntech.de>
> > 
> > I have to take that back.
> > This leaves the spin_lock in the schmitt trigger unconverted and produces
> > compile issues.
> > 
> > So it either needs to be converted or removed - I guess it should be
> > removed.
> Yes, it looks like it should be removed - the patches adding that hadn't
> been applied when I sent this out!
> 
> I'll rebase and add the schmitt trigger change to patch 1 in a day or
> two, after giving a bit more time for reviews.

I don't think you need to wait overly long. Linus seems happy, I'm happy and 
the schmitt issue in the open might prevent people from taking a closer look.


Heiko

[RFC PATCH 2/4] pinctrl: rockchip: convert to raw spinlock

[Heiko Stuebner]

Am Mittwoch, 15. M?rz 2017, 16:59:00 CET schrieb John Keeping:
> On Wed, 15 Mar 2017 17:41:21 +0100, Heiko Stuebner wrote:
> > Am Mittwoch, 15. M?rz 2017, 17:28:56 CET schrieb Heiko Stuebner:
> > > Am Montag, 13. M?rz 2017, 18:38:11 CET schrieb John Keeping:
> > > > This lock is used from rockchip_irq_set_type() which is part of the
> > > > irq_chip implementation and thus must use raw_spinlock_t as documented
> > > > in Documentation/gpio/driver.txt.
> > > > 
> > > > Signed-off-by: John Keeping <john@metanate.com>
> > > 
> > > Looks good
> > > Reviewed-by: Heiko Stuebner <heiko@sntech.de>
> > 
> > I have to take that back.
> > This leaves the spin_lock in the schmitt trigger unconverted and produces
> > compile issues.
> > 
> > So it either needs to be converted or removed - I guess it should be
> > removed.
> Yes, it looks like it should be removed - the patches adding that hadn't
> been applied when I sent this out!
> 
> I'll rebase and add the schmitt trigger change to patch 1 in a day or
> two, after giving a bit more time for reviews.

I don't think you need to wait overly long. Linus seems happy, I'm happy and 
the schmitt issue in the open might prevent people from taking a closer look.


Heiko

[RFC PATCH 3/4] pinctrl: rockchip: split out verification of mux settings

[John Keeping]

We need to avoid calling regmap functions from irq handlers, so the next
commit is going to move the call to rockchip_set_mux() into an
irq_bus_sync_unlock handler.  But we can't return an error from there so
we still need to check the settings from rockchip_irq_set_type() and we
will use this new rockchip_verify_mux() function from there.

Signed-off-by: John Keeping <john@metanate.com>
---
 drivers/pinctrl/pinctrl-rockchip.c | 46 +++++++++++++++++++++++++-------------
 1 file changed, 30 insertions(+), 16 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c
index 2f963aea64b2..52e70c4aef7c 100644
--- a/drivers/pinctrl/pinctrl-rockchip.c
+++ b/drivers/pinctrl/pinctrl-rockchip.c
@@ -629,6 +629,31 @@ static int rockchip_get_mux(struct rockchip_pin_bank *bank, int pin)
 	return ((val >> bit) & mask);
 }
 
+static int rockchip_verify_mux(struct rockchip_pin_bank *bank,
+			       int pin, int mux)
+{
+	struct rockchip_pinctrl *info = bank->drvdata;
+	int iomux_num = (pin / 8);
+
+	if (iomux_num > 3)
+		return -EINVAL;
+
+	if (bank->iomux[iomux_num].type & IOMUX_UNROUTED) {
+		dev_err(info->dev, "pin %d is unrouted\n", pin);
+		return -EINVAL;
+	}
+
+	if (bank->iomux[iomux_num].type & IOMUX_GPIO_ONLY) {
+		if (mux != RK_FUNC_GPIO) {
+			dev_err(info->dev,
+				"pin %d only supports a gpio mux\n", pin);
+			return -ENOTSUPP;
+		}
+	}
+
+	return 0;
+}
+
 /*
  * Set a new mux function for a pin.
  *
@@ -652,23 +677,12 @@ static int rockchip_set_mux(struct rockchip_pin_bank *bank, int pin, int mux)
 	u8 bit;
 	u32 data, rmask;
 
-	if (iomux_num > 3)
-		return -EINVAL;
-
-	if (bank->iomux[iomux_num].type & IOMUX_UNROUTED) {
-		dev_err(info->dev, "pin %d is unrouted\n", pin);
-		return -EINVAL;
-	}
+	ret = rockchip_verify_mux(bank, pin, mux);
+	if (ret < 0)
+		return ret;
 
-	if (bank->iomux[iomux_num].type & IOMUX_GPIO_ONLY) {
-		if (mux != RK_FUNC_GPIO) {
-			dev_err(info->dev,
-				"pin %d only supports a gpio mux\n", pin);
-			return -ENOTSUPP;
-		} else {
-			return 0;
-		}
-	}
+	if (bank->iomux[iomux_num].type & IOMUX_GPIO_ONLY)
+		return 0;
 
 	dev_dbg(info->dev, "setting mux of GPIO%d-%d to %d\n",
 						bank->bank_num, pin, mux);
-- 
2.12.0.377.gf910686b23.dirty

[RFC PATCH 3/4] pinctrl: rockchip: split out verification of mux settings

[John Keeping]

We need to avoid calling regmap functions from irq handlers, so the next
commit is going to move the call to rockchip_set_mux() into an
irq_bus_sync_unlock handler.  But we can't return an error from there so
we still need to check the settings from rockchip_irq_set_type() and we
will use this new rockchip_verify_mux() function from there.

Signed-off-by: John Keeping <john@metanate.com>
---
 drivers/pinctrl/pinctrl-rockchip.c | 46 +++++++++++++++++++++++++-------------
 1 file changed, 30 insertions(+), 16 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c
index 2f963aea64b2..52e70c4aef7c 100644
--- a/drivers/pinctrl/pinctrl-rockchip.c
+++ b/drivers/pinctrl/pinctrl-rockchip.c
@@ -629,6 +629,31 @@ static int rockchip_get_mux(struct rockchip_pin_bank *bank, int pin)
 	return ((val >> bit) & mask);
 }
 
+static int rockchip_verify_mux(struct rockchip_pin_bank *bank,
+			       int pin, int mux)
+{
+	struct rockchip_pinctrl *info = bank->drvdata;
+	int iomux_num = (pin / 8);
+
+	if (iomux_num > 3)
+		return -EINVAL;
+
+	if (bank->iomux[iomux_num].type & IOMUX_UNROUTED) {
+		dev_err(info->dev, "pin %d is unrouted\n", pin);
+		return -EINVAL;
+	}
+
+	if (bank->iomux[iomux_num].type & IOMUX_GPIO_ONLY) {
+		if (mux != RK_FUNC_GPIO) {
+			dev_err(info->dev,
+				"pin %d only supports a gpio mux\n", pin);
+			return -ENOTSUPP;
+		}
+	}
+
+	return 0;
+}
+
 /*
  * Set a new mux function for a pin.
  *
@@ -652,23 +677,12 @@ static int rockchip_set_mux(struct rockchip_pin_bank *bank, int pin, int mux)
 	u8 bit;
 	u32 data, rmask;
 
-	if (iomux_num > 3)
-		return -EINVAL;
-
-	if (bank->iomux[iomux_num].type & IOMUX_UNROUTED) {
-		dev_err(info->dev, "pin %d is unrouted\n", pin);
-		return -EINVAL;
-	}
+	ret = rockchip_verify_mux(bank, pin, mux);
+	if (ret < 0)
+		return ret;
 
-	if (bank->iomux[iomux_num].type & IOMUX_GPIO_ONLY) {
-		if (mux != RK_FUNC_GPIO) {
-			dev_err(info->dev,
-				"pin %d only supports a gpio mux\n", pin);
-			return -ENOTSUPP;
-		} else {
-			return 0;
-		}
-	}
+	if (bank->iomux[iomux_num].type & IOMUX_GPIO_ONLY)
+		return 0;
 
 	dev_dbg(info->dev, "setting mux of GPIO%d-%d to %d\n",
 						bank->bank_num, pin, mux);
-- 
2.12.0.377.gf910686b23.dirty

Re: [RFC PATCH 3/4] pinctrl: rockchip: split out verification of mux settings

[Heiko Stuebner]

Am Montag, 13. März 2017, 18:38:12 CET schrieb John Keeping:
> We need to avoid calling regmap functions from irq handlers, so the next
> commit is going to move the call to rockchip_set_mux() into an
> irq_bus_sync_unlock handler.  But we can't return an error from there so
> we still need to check the settings from rockchip_irq_set_type() and we
> will use this new rockchip_verify_mux() function from there.
> 
> Signed-off-by: John Keeping <john@metanate.com>

straightforward separation of the verification code
Reviewed-by: Heiko Stuebner <heiko@sntech.de>

[RFC PATCH 3/4] pinctrl: rockchip: split out verification of mux settings

[Heiko Stuebner]

Am Montag, 13. M?rz 2017, 18:38:12 CET schrieb John Keeping:
> We need to avoid calling regmap functions from irq handlers, so the next
> commit is going to move the call to rockchip_set_mux() into an
> irq_bus_sync_unlock handler.  But we can't return an error from there so
> we still need to check the settings from rockchip_irq_set_type() and we
> will use this new rockchip_verify_mux() function from there.
> 
> Signed-off-by: John Keeping <john@metanate.com>

straightforward separation of the verification code
Reviewed-by: Heiko Stuebner <heiko@sntech.de>

[RFC PATCH 4/4] pinctrl: rockchip: avoid hardirq-unsafe functions in irq_chip

[John Keeping]

With real-time preemption, regmap functions cannot be used in the
implementation of irq_chip since they use spinlocks which may sleep.

Move the setting of the mux for IRQs to an irq_bus_sync_unlock handler
where we are allowed to sleep.

Signed-off-by: John Keeping <john@metanate.com>
---
 drivers/pinctrl/pinctrl-rockchip.c | 44 ++++++++++++++++++++++++++++++++++----
 1 file changed, 40 insertions(+), 4 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c
index 52e70c4aef7c..80f7c23d12d4 100644
--- a/drivers/pinctrl/pinctrl-rockchip.c
+++ b/drivers/pinctrl/pinctrl-rockchip.c
@@ -143,6 +143,9 @@ struct rockchip_drv {
  * @gpio_chip: gpiolib chip
  * @grange: gpio range
  * @slock: spinlock for the gpio bank
+ * @irq_lock: bus lock for irq chip
+ * @new_irqs: newly configured irqs which must be muxed as GPIOs in
+ *	irq_bus_sync_unlock()
  */
 struct rockchip_pin_bank {
 	void __iomem			*reg_base;
@@ -165,6 +168,8 @@ struct rockchip_pin_bank {
 	struct pinctrl_gpio_range	grange;
 	raw_spinlock_t			slock;
 	u32				toggle_edge_mode;
+	struct mutex			irq_lock;
+	u32				new_irqs;
 };
 
 #define PIN_BANK(id, pins, label)			\
@@ -2045,11 +2050,12 @@ static int rockchip_irq_set_type(struct irq_data *d, unsigned int type)
 	int ret;
 
 	/* make sure the pin is configured as gpio input */
-	ret = rockchip_set_mux(bank, d->hwirq, RK_FUNC_GPIO);
+	ret = rockchip_verify_mux(bank, d->hwirq, RK_FUNC_GPIO);
 	if (ret < 0)
 		return ret;
 
-	clk_enable(bank->clk);
+	bank->new_irqs |= mask;
+
 	raw_spin_lock_irqsave(&bank->slock, flags);
 
 	data = readl_relaxed(bank->reg_base + GPIO_SWPORT_DDR);
@@ -2107,7 +2113,6 @@ static int rockchip_irq_set_type(struct irq_data *d, unsigned int type)
 	default:
 		irq_gc_unlock(gc);
 		raw_spin_unlock_irqrestore(&bank->slock, flags);
-		clk_disable(bank->clk);
 		return -EINVAL;
 	}
 
@@ -2116,7 +2121,6 @@ static int rockchip_irq_set_type(struct irq_data *d, unsigned int type)
 
 	irq_gc_unlock(gc);
 	raw_spin_unlock_irqrestore(&bank->slock, flags);
-	clk_disable(bank->clk);
 
 	return 0;
 }
@@ -2160,6 +2164,34 @@ static void rockchip_irq_gc_mask_set_bit(struct irq_data *d)
 	clk_disable(bank->clk);
 }
 
+static void rockchip_irq_bus_lock(struct irq_data *d)
+{
+	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+	struct rockchip_pin_bank *bank = gc->private;
+
+	clk_enable(bank->clk);
+	mutex_lock(&bank->irq_lock);
+}
+
+static void rockchip_irq_bus_sync_unlock(struct irq_data *d)
+{
+	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+	struct rockchip_pin_bank *bank = gc->private;
+
+	while (bank->new_irqs) {
+		unsigned int irq = __ffs(bank->new_irqs);
+		int ret;
+
+		ret = rockchip_set_mux(bank, irq, RK_FUNC_GPIO);
+		WARN_ON(ret < 0);
+
+		bank->new_irqs &= ~BIT(irq);
+	}
+
+	mutex_unlock(&bank->irq_lock);
+	clk_disable(bank->clk);
+}
+
 static int rockchip_interrupts_register(struct platform_device *pdev,
 						struct rockchip_pinctrl *info)
 {
@@ -2225,6 +2257,9 @@ static int rockchip_interrupts_register(struct platform_device *pdev,
 		gc->chip_types[0].chip.irq_suspend = rockchip_irq_suspend;
 		gc->chip_types[0].chip.irq_resume = rockchip_irq_resume;
 		gc->chip_types[0].chip.irq_set_type = rockchip_irq_set_type;
+		gc->chip_types[0].chip.irq_bus_lock = rockchip_irq_bus_lock;
+		gc->chip_types[0].chip.irq_bus_sync_unlock =
+						rockchip_irq_bus_sync_unlock;
 		gc->wake_enabled = IRQ_MSK(bank->nr_pins);
 
 		irq_set_chained_handler_and_data(bank->irq,
@@ -2398,6 +2433,7 @@ static struct rockchip_pin_ctrl *rockchip_pinctrl_get_soc_data(
 		int bank_pins = 0;
 
 		raw_spin_lock_init(&bank->slock);
+		mutex_init(&bank->irq_lock);
 		bank->drvdata = d;
 		bank->pin_base = ctrl->nr_pins;
 		ctrl->nr_pins += bank->nr_pins;
-- 
2.12.0.377.gf910686b23.dirty

[RFC PATCH 4/4] pinctrl: rockchip: avoid hardirq-unsafe functions in irq_chip

[John Keeping]

With real-time preemption, regmap functions cannot be used in the
implementation of irq_chip since they use spinlocks which may sleep.

Move the setting of the mux for IRQs to an irq_bus_sync_unlock handler
where we are allowed to sleep.

Signed-off-by: John Keeping <john@metanate.com>
---
 drivers/pinctrl/pinctrl-rockchip.c | 44 ++++++++++++++++++++++++++++++++++----
 1 file changed, 40 insertions(+), 4 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c
index 52e70c4aef7c..80f7c23d12d4 100644
--- a/drivers/pinctrl/pinctrl-rockchip.c
+++ b/drivers/pinctrl/pinctrl-rockchip.c
@@ -143,6 +143,9 @@ struct rockchip_drv {
  * @gpio_chip: gpiolib chip
  * @grange: gpio range
  * @slock: spinlock for the gpio bank
+ * @irq_lock: bus lock for irq chip
+ * @new_irqs: newly configured irqs which must be muxed as GPIOs in
+ *	irq_bus_sync_unlock()
  */
 struct rockchip_pin_bank {
 	void __iomem			*reg_base;
@@ -165,6 +168,8 @@ struct rockchip_pin_bank {
 	struct pinctrl_gpio_range	grange;
 	raw_spinlock_t			slock;
 	u32				toggle_edge_mode;
+	struct mutex			irq_lock;
+	u32				new_irqs;
 };
 
 #define PIN_BANK(id, pins, label)			\
@@ -2045,11 +2050,12 @@ static int rockchip_irq_set_type(struct irq_data *d, unsigned int type)
 	int ret;
 
 	/* make sure the pin is configured as gpio input */
-	ret = rockchip_set_mux(bank, d->hwirq, RK_FUNC_GPIO);
+	ret = rockchip_verify_mux(bank, d->hwirq, RK_FUNC_GPIO);
 	if (ret < 0)
 		return ret;
 
-	clk_enable(bank->clk);
+	bank->new_irqs |= mask;
+
 	raw_spin_lock_irqsave(&bank->slock, flags);
 
 	data = readl_relaxed(bank->reg_base + GPIO_SWPORT_DDR);
@@ -2107,7 +2113,6 @@ static int rockchip_irq_set_type(struct irq_data *d, unsigned int type)
 	default:
 		irq_gc_unlock(gc);
 		raw_spin_unlock_irqrestore(&bank->slock, flags);
-		clk_disable(bank->clk);
 		return -EINVAL;
 	}
 
@@ -2116,7 +2121,6 @@ static int rockchip_irq_set_type(struct irq_data *d, unsigned int type)
 
 	irq_gc_unlock(gc);
 	raw_spin_unlock_irqrestore(&bank->slock, flags);
-	clk_disable(bank->clk);
 
 	return 0;
 }
@@ -2160,6 +2164,34 @@ static void rockchip_irq_gc_mask_set_bit(struct irq_data *d)
 	clk_disable(bank->clk);
 }
 
+static void rockchip_irq_bus_lock(struct irq_data *d)
+{
+	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+	struct rockchip_pin_bank *bank = gc->private;
+
+	clk_enable(bank->clk);
+	mutex_lock(&bank->irq_lock);
+}
+
+static void rockchip_irq_bus_sync_unlock(struct irq_data *d)
+{
+	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+	struct rockchip_pin_bank *bank = gc->private;
+
+	while (bank->new_irqs) {
+		unsigned int irq = __ffs(bank->new_irqs);
+		int ret;
+
+		ret = rockchip_set_mux(bank, irq, RK_FUNC_GPIO);
+		WARN_ON(ret < 0);
+
+		bank->new_irqs &= ~BIT(irq);
+	}
+
+	mutex_unlock(&bank->irq_lock);
+	clk_disable(bank->clk);
+}
+
 static int rockchip_interrupts_register(struct platform_device *pdev,
 						struct rockchip_pinctrl *info)
 {
@@ -2225,6 +2257,9 @@ static int rockchip_interrupts_register(struct platform_device *pdev,
 		gc->chip_types[0].chip.irq_suspend = rockchip_irq_suspend;
 		gc->chip_types[0].chip.irq_resume = rockchip_irq_resume;
 		gc->chip_types[0].chip.irq_set_type = rockchip_irq_set_type;
+		gc->chip_types[0].chip.irq_bus_lock = rockchip_irq_bus_lock;
+		gc->chip_types[0].chip.irq_bus_sync_unlock =
+						rockchip_irq_bus_sync_unlock;
 		gc->wake_enabled = IRQ_MSK(bank->nr_pins);
 
 		irq_set_chained_handler_and_data(bank->irq,
@@ -2398,6 +2433,7 @@ static struct rockchip_pin_ctrl *rockchip_pinctrl_get_soc_data(
 		int bank_pins = 0;
 
 		raw_spin_lock_init(&bank->slock);
+		mutex_init(&bank->irq_lock);
 		bank->drvdata = d;
 		bank->pin_base = ctrl->nr_pins;
 		ctrl->nr_pins += bank->nr_pins;
-- 
2.12.0.377.gf910686b23.dirty

Re: [RFC PATCH 4/4] pinctrl: rockchip: avoid hardirq-unsafe functions in irq_chip

[Heiko Stuebner]

Am Montag, 13. März 2017, 18:38:13 CET schrieb John Keeping:
> With real-time preemption, regmap functions cannot be used in the
> implementation of irq_chip since they use spinlocks which may sleep.
> 
> Move the setting of the mux for IRQs to an irq_bus_sync_unlock handler
> where we are allowed to sleep.
> 
> Signed-off-by: John Keeping <john-HooS5bfzL4hWk0Htik3J/w@public.gmane.org>

Looks good
Reviewed-by: Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org>

> ---
>  drivers/pinctrl/pinctrl-rockchip.c | 44
> ++++++++++++++++++++++++++++++++++---- 1 file changed, 40 insertions(+), 4
> deletions(-)
> 
> diff --git a/drivers/pinctrl/pinctrl-rockchip.c
> b/drivers/pinctrl/pinctrl-rockchip.c index 52e70c4aef7c..80f7c23d12d4
> 100644
> --- a/drivers/pinctrl/pinctrl-rockchip.c
> +++ b/drivers/pinctrl/pinctrl-rockchip.c
> @@ -143,6 +143,9 @@ struct rockchip_drv {
>   * @gpio_chip: gpiolib chip
>   * @grange: gpio range
>   * @slock: spinlock for the gpio bank
> + * @irq_lock: bus lock for irq chip
> + * @new_irqs: newly configured irqs which must be muxed as GPIOs in
> + *	irq_bus_sync_unlock()
>   */
>  struct rockchip_pin_bank {
>  	void __iomem			*reg_base;
> @@ -165,6 +168,8 @@ struct rockchip_pin_bank {
>  	struct pinctrl_gpio_range	grange;
>  	raw_spinlock_t			slock;
>  	u32				toggle_edge_mode;
> +	struct mutex			irq_lock;
> +	u32				new_irqs;
>  };
> 
>  #define PIN_BANK(id, pins, label)			\
> @@ -2045,11 +2050,12 @@ static int rockchip_irq_set_type(struct irq_data *d,
> unsigned int type) int ret;
> 
>  	/* make sure the pin is configured as gpio input */
> -	ret = rockchip_set_mux(bank, d->hwirq, RK_FUNC_GPIO);
> +	ret = rockchip_verify_mux(bank, d->hwirq, RK_FUNC_GPIO);
>  	if (ret < 0)
>  		return ret;
> 
> -	clk_enable(bank->clk);
> +	bank->new_irqs |= mask;
> +
>  	raw_spin_lock_irqsave(&bank->slock, flags);
> 
>  	data = readl_relaxed(bank->reg_base + GPIO_SWPORT_DDR);
> @@ -2107,7 +2113,6 @@ static int rockchip_irq_set_type(struct irq_data *d,
> unsigned int type) default:
>  		irq_gc_unlock(gc);
>  		raw_spin_unlock_irqrestore(&bank->slock, flags);
> -		clk_disable(bank->clk);
>  		return -EINVAL;
>  	}
> 
> @@ -2116,7 +2121,6 @@ static int rockchip_irq_set_type(struct irq_data *d,
> unsigned int type)
> 
>  	irq_gc_unlock(gc);
>  	raw_spin_unlock_irqrestore(&bank->slock, flags);
> -	clk_disable(bank->clk);
> 
>  	return 0;
>  }
> @@ -2160,6 +2164,34 @@ static void rockchip_irq_gc_mask_set_bit(struct
> irq_data *d) clk_disable(bank->clk);
>  }
> 
> +static void rockchip_irq_bus_lock(struct irq_data *d)
> +{
> +	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
> +	struct rockchip_pin_bank *bank = gc->private;
> +
> +	clk_enable(bank->clk);
> +	mutex_lock(&bank->irq_lock);
> +}
> +
> +static void rockchip_irq_bus_sync_unlock(struct irq_data *d)
> +{
> +	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
> +	struct rockchip_pin_bank *bank = gc->private;
> +
> +	while (bank->new_irqs) {
> +		unsigned int irq = __ffs(bank->new_irqs);
> +		int ret;
> +
> +		ret = rockchip_set_mux(bank, irq, RK_FUNC_GPIO);
> +		WARN_ON(ret < 0);
> +
> +		bank->new_irqs &= ~BIT(irq);
> +	}
> +
> +	mutex_unlock(&bank->irq_lock);
> +	clk_disable(bank->clk);
> +}
> +
>  static int rockchip_interrupts_register(struct platform_device *pdev,
>  						struct rockchip_pinctrl *info)
>  {
> @@ -2225,6 +2257,9 @@ static int rockchip_interrupts_register(struct
> platform_device *pdev, gc->chip_types[0].chip.irq_suspend =
> rockchip_irq_suspend;
>  		gc->chip_types[0].chip.irq_resume = rockchip_irq_resume;
>  		gc->chip_types[0].chip.irq_set_type = rockchip_irq_set_type;
> +		gc->chip_types[0].chip.irq_bus_lock = rockchip_irq_bus_lock;
> +		gc->chip_types[0].chip.irq_bus_sync_unlock =
> +						rockchip_irq_bus_sync_unlock;
>  		gc->wake_enabled = IRQ_MSK(bank->nr_pins);
> 
>  		irq_set_chained_handler_and_data(bank->irq,
> @@ -2398,6 +2433,7 @@ static struct rockchip_pin_ctrl
> *rockchip_pinctrl_get_soc_data( int bank_pins = 0;
> 
>  		raw_spin_lock_init(&bank->slock);
> +		mutex_init(&bank->irq_lock);
>  		bank->drvdata = d;
>  		bank->pin_base = ctrl->nr_pins;
>  		ctrl->nr_pins += bank->nr_pins;

Re: [RFC PATCH 4/4] pinctrl: rockchip: avoid hardirq-unsafe functions in irq_chip

[Heiko Stuebner]

Am Montag, 13. März 2017, 18:38:13 CET schrieb John Keeping:
> With real-time preemption, regmap functions cannot be used in the
> implementation of irq_chip since they use spinlocks which may sleep.
> 
> Move the setting of the mux for IRQs to an irq_bus_sync_unlock handler
> where we are allowed to sleep.
> 
> Signed-off-by: John Keeping <john@metanate.com>

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

> ---
>  drivers/pinctrl/pinctrl-rockchip.c | 44
> ++++++++++++++++++++++++++++++++++---- 1 file changed, 40 insertions(+), 4
> deletions(-)
> 
> diff --git a/drivers/pinctrl/pinctrl-rockchip.c
> b/drivers/pinctrl/pinctrl-rockchip.c index 52e70c4aef7c..80f7c23d12d4
> 100644
> --- a/drivers/pinctrl/pinctrl-rockchip.c
> +++ b/drivers/pinctrl/pinctrl-rockchip.c
> @@ -143,6 +143,9 @@ struct rockchip_drv {
>   * @gpio_chip: gpiolib chip
>   * @grange: gpio range
>   * @slock: spinlock for the gpio bank
> + * @irq_lock: bus lock for irq chip
> + * @new_irqs: newly configured irqs which must be muxed as GPIOs in
> + *	irq_bus_sync_unlock()
>   */
>  struct rockchip_pin_bank {
>  	void __iomem			*reg_base;
> @@ -165,6 +168,8 @@ struct rockchip_pin_bank {
>  	struct pinctrl_gpio_range	grange;
>  	raw_spinlock_t			slock;
>  	u32				toggle_edge_mode;
> +	struct mutex			irq_lock;
> +	u32				new_irqs;
>  };
> 
>  #define PIN_BANK(id, pins, label)			\
> @@ -2045,11 +2050,12 @@ static int rockchip_irq_set_type(struct irq_data *d,
> unsigned int type) int ret;
> 
>  	/* make sure the pin is configured as gpio input */
> -	ret = rockchip_set_mux(bank, d->hwirq, RK_FUNC_GPIO);
> +	ret = rockchip_verify_mux(bank, d->hwirq, RK_FUNC_GPIO);
>  	if (ret < 0)
>  		return ret;
> 
> -	clk_enable(bank->clk);
> +	bank->new_irqs |= mask;
> +
>  	raw_spin_lock_irqsave(&bank->slock, flags);
> 
>  	data = readl_relaxed(bank->reg_base + GPIO_SWPORT_DDR);
> @@ -2107,7 +2113,6 @@ static int rockchip_irq_set_type(struct irq_data *d,
> unsigned int type) default:
>  		irq_gc_unlock(gc);
>  		raw_spin_unlock_irqrestore(&bank->slock, flags);
> -		clk_disable(bank->clk);
>  		return -EINVAL;
>  	}
> 
> @@ -2116,7 +2121,6 @@ static int rockchip_irq_set_type(struct irq_data *d,
> unsigned int type)
> 
>  	irq_gc_unlock(gc);
>  	raw_spin_unlock_irqrestore(&bank->slock, flags);
> -	clk_disable(bank->clk);
> 
>  	return 0;
>  }
> @@ -2160,6 +2164,34 @@ static void rockchip_irq_gc_mask_set_bit(struct
> irq_data *d) clk_disable(bank->clk);
>  }
> 
> +static void rockchip_irq_bus_lock(struct irq_data *d)
> +{
> +	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
> +	struct rockchip_pin_bank *bank = gc->private;
> +
> +	clk_enable(bank->clk);
> +	mutex_lock(&bank->irq_lock);
> +}
> +
> +static void rockchip_irq_bus_sync_unlock(struct irq_data *d)
> +{
> +	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
> +	struct rockchip_pin_bank *bank = gc->private;
> +
> +	while (bank->new_irqs) {
> +		unsigned int irq = __ffs(bank->new_irqs);
> +		int ret;
> +
> +		ret = rockchip_set_mux(bank, irq, RK_FUNC_GPIO);
> +		WARN_ON(ret < 0);
> +
> +		bank->new_irqs &= ~BIT(irq);
> +	}
> +
> +	mutex_unlock(&bank->irq_lock);
> +	clk_disable(bank->clk);
> +}
> +
>  static int rockchip_interrupts_register(struct platform_device *pdev,
>  						struct rockchip_pinctrl *info)
>  {
> @@ -2225,6 +2257,9 @@ static int rockchip_interrupts_register(struct
> platform_device *pdev, gc->chip_types[0].chip.irq_suspend =
> rockchip_irq_suspend;
>  		gc->chip_types[0].chip.irq_resume = rockchip_irq_resume;
>  		gc->chip_types[0].chip.irq_set_type = rockchip_irq_set_type;
> +		gc->chip_types[0].chip.irq_bus_lock = rockchip_irq_bus_lock;
> +		gc->chip_types[0].chip.irq_bus_sync_unlock =
> +						rockchip_irq_bus_sync_unlock;
>  		gc->wake_enabled = IRQ_MSK(bank->nr_pins);
> 
>  		irq_set_chained_handler_and_data(bank->irq,
> @@ -2398,6 +2433,7 @@ static struct rockchip_pin_ctrl
> *rockchip_pinctrl_get_soc_data( int bank_pins = 0;
> 
>  		raw_spin_lock_init(&bank->slock);
> +		mutex_init(&bank->irq_lock);
>  		bank->drvdata = d;
>  		bank->pin_base = ctrl->nr_pins;
>  		ctrl->nr_pins += bank->nr_pins;

[RFC PATCH 4/4] pinctrl: rockchip: avoid hardirq-unsafe functions in irq_chip

[Heiko Stuebner]

Am Montag, 13. M?rz 2017, 18:38:13 CET schrieb John Keeping:
> With real-time preemption, regmap functions cannot be used in the
> implementation of irq_chip since they use spinlocks which may sleep.
> 
> Move the setting of the mux for IRQs to an irq_bus_sync_unlock handler
> where we are allowed to sleep.
> 
> Signed-off-by: John Keeping <john@metanate.com>

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

> ---
>  drivers/pinctrl/pinctrl-rockchip.c | 44
> ++++++++++++++++++++++++++++++++++---- 1 file changed, 40 insertions(+), 4
> deletions(-)
> 
> diff --git a/drivers/pinctrl/pinctrl-rockchip.c
> b/drivers/pinctrl/pinctrl-rockchip.c index 52e70c4aef7c..80f7c23d12d4
> 100644
> --- a/drivers/pinctrl/pinctrl-rockchip.c
> +++ b/drivers/pinctrl/pinctrl-rockchip.c
> @@ -143,6 +143,9 @@ struct rockchip_drv {
>   * @gpio_chip: gpiolib chip
>   * @grange: gpio range
>   * @slock: spinlock for the gpio bank
> + * @irq_lock: bus lock for irq chip
> + * @new_irqs: newly configured irqs which must be muxed as GPIOs in
> + *	irq_bus_sync_unlock()
>   */
>  struct rockchip_pin_bank {
>  	void __iomem			*reg_base;
> @@ -165,6 +168,8 @@ struct rockchip_pin_bank {
>  	struct pinctrl_gpio_range	grange;
>  	raw_spinlock_t			slock;
>  	u32				toggle_edge_mode;
> +	struct mutex			irq_lock;
> +	u32				new_irqs;
>  };
> 
>  #define PIN_BANK(id, pins, label)			\
> @@ -2045,11 +2050,12 @@ static int rockchip_irq_set_type(struct irq_data *d,
> unsigned int type) int ret;
> 
>  	/* make sure the pin is configured as gpio input */
> -	ret = rockchip_set_mux(bank, d->hwirq, RK_FUNC_GPIO);
> +	ret = rockchip_verify_mux(bank, d->hwirq, RK_FUNC_GPIO);
>  	if (ret < 0)
>  		return ret;
> 
> -	clk_enable(bank->clk);
> +	bank->new_irqs |= mask;
> +
>  	raw_spin_lock_irqsave(&bank->slock, flags);
> 
>  	data = readl_relaxed(bank->reg_base + GPIO_SWPORT_DDR);
> @@ -2107,7 +2113,6 @@ static int rockchip_irq_set_type(struct irq_data *d,
> unsigned int type) default:
>  		irq_gc_unlock(gc);
>  		raw_spin_unlock_irqrestore(&bank->slock, flags);
> -		clk_disable(bank->clk);
>  		return -EINVAL;
>  	}
> 
> @@ -2116,7 +2121,6 @@ static int rockchip_irq_set_type(struct irq_data *d,
> unsigned int type)
> 
>  	irq_gc_unlock(gc);
>  	raw_spin_unlock_irqrestore(&bank->slock, flags);
> -	clk_disable(bank->clk);
> 
>  	return 0;
>  }
> @@ -2160,6 +2164,34 @@ static void rockchip_irq_gc_mask_set_bit(struct
> irq_data *d) clk_disable(bank->clk);
>  }
> 
> +static void rockchip_irq_bus_lock(struct irq_data *d)
> +{
> +	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
> +	struct rockchip_pin_bank *bank = gc->private;
> +
> +	clk_enable(bank->clk);
> +	mutex_lock(&bank->irq_lock);
> +}
> +
> +static void rockchip_irq_bus_sync_unlock(struct irq_data *d)
> +{
> +	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
> +	struct rockchip_pin_bank *bank = gc->private;
> +
> +	while (bank->new_irqs) {
> +		unsigned int irq = __ffs(bank->new_irqs);
> +		int ret;
> +
> +		ret = rockchip_set_mux(bank, irq, RK_FUNC_GPIO);
> +		WARN_ON(ret < 0);
> +
> +		bank->new_irqs &= ~BIT(irq);
> +	}
> +
> +	mutex_unlock(&bank->irq_lock);
> +	clk_disable(bank->clk);
> +}
> +
>  static int rockchip_interrupts_register(struct platform_device *pdev,
>  						struct rockchip_pinctrl *info)
>  {
> @@ -2225,6 +2257,9 @@ static int rockchip_interrupts_register(struct
> platform_device *pdev, gc->chip_types[0].chip.irq_suspend =
> rockchip_irq_suspend;
>  		gc->chip_types[0].chip.irq_resume = rockchip_irq_resume;
>  		gc->chip_types[0].chip.irq_set_type = rockchip_irq_set_type;
> +		gc->chip_types[0].chip.irq_bus_lock = rockchip_irq_bus_lock;
> +		gc->chip_types[0].chip.irq_bus_sync_unlock =
> +						rockchip_irq_bus_sync_unlock;
>  		gc->wake_enabled = IRQ_MSK(bank->nr_pins);
> 
>  		irq_set_chained_handler_and_data(bank->irq,
> @@ -2398,6 +2433,7 @@ static struct rockchip_pin_ctrl
> *rockchip_pinctrl_get_soc_data( int bank_pins = 0;
> 
>  		raw_spin_lock_init(&bank->slock);
> +		mutex_init(&bank->irq_lock);
>  		bank->drvdata = d;
>  		bank->pin_base = ctrl->nr_pins;
>  		ctrl->nr_pins += bank->nr_pins;

Re: [RFC PATCH 0/4] pinctrl: rockchip: PREEMPT_RT_FULL fixes

[Linus Walleij]

On Mon, Mar 13, 2017 at 7:38 PM, John Keeping <john@metanate.com> wrote:

> As described in Documentation/gpio/driver.txt, we should not be using
> sleepable APIs in the irqchip implementation.  Since this includes the
> regmap API, this patch series ends up moving the mux setup for IRQs into
> an irq_bus_sync_unlock() handler which may result in the IRQ being
> configured before the port has been muxed as a GPIO.
>
> I've marked the series as RFC because I'm not sure if this is the best
> way to accomplish this or if there is another approach that is cleaner.
> Also, the first patch may not be correct on RK3399 because I originally
> wrote the patch for RK3288 on top of v4.4 where all drive updates only
> affect a single register.  We don't need locking in this case because
> regmap_update_bits() takes a lock on the regmap internally, but if these
> two registers need to be updated atomically then another lock will
> be required here - slock cannot be used if it is converted to a raw
> spinlock since with full RT preemption the regmap's spinlock may sleep.

Nice work! It all looks good to me, let's see what Heiko says.

Please keep Julia Cartwright on the CC for this patch series, she is
doing some coccinelle-based rewrites to use raw spinlocks as we
speak, and she knows this stuff.

She has not targeted the Rockchip driver yet, I guess because of
its complexity.

Yours,
Linus Walleij

[RFC PATCH 0/4] pinctrl: rockchip: PREEMPT_RT_FULL fixes

[Linus Walleij]

On Mon, Mar 13, 2017 at 7:38 PM, John Keeping <john@metanate.com> wrote:

> As described in Documentation/gpio/driver.txt, we should not be using
> sleepable APIs in the irqchip implementation.  Since this includes the
> regmap API, this patch series ends up moving the mux setup for IRQs into
> an irq_bus_sync_unlock() handler which may result in the IRQ being
> configured before the port has been muxed as a GPIO.
>
> I've marked the series as RFC because I'm not sure if this is the best
> way to accomplish this or if there is another approach that is cleaner.
> Also, the first patch may not be correct on RK3399 because I originally
> wrote the patch for RK3288 on top of v4.4 where all drive updates only
> affect a single register.  We don't need locking in this case because
> regmap_update_bits() takes a lock on the regmap internally, but if these
> two registers need to be updated atomically then another lock will
> be required here - slock cannot be used if it is converted to a raw
> spinlock since with full RT preemption the regmap's spinlock may sleep.

Nice work! It all looks good to me, let's see what Heiko says.

Please keep Julia Cartwright on the CC for this patch series, she is
doing some coccinelle-based rewrites to use raw spinlocks as we
speak, and she knows this stuff.

She has not targeted the Rockchip driver yet, I guess because of
its complexity.

Yours,
Linus Walleij

Re: [RFC PATCH 0/4] pinctrl: rockchip: PREEMPT_RT_FULL fixes

[Julia Cartwright]

[-- Attachment #1.1: Type: text/plain, Size: 1779 bytes --]

On Wed, Mar 15, 2017 at 02:12:39PM +0100, Linus Walleij wrote:
> On Mon, Mar 13, 2017 at 7:38 PM, John Keeping <john@metanate.com> wrote:
> 
> > As described in Documentation/gpio/driver.txt, we should not be using
> > sleepable APIs in the irqchip implementation.  Since this includes the
> > regmap API, this patch series ends up moving the mux setup for IRQs into
> > an irq_bus_sync_unlock() handler which may result in the IRQ being
> > configured before the port has been muxed as a GPIO.
> >
> > I've marked the series as RFC because I'm not sure if this is the best
> > way to accomplish this or if there is another approach that is cleaner.
> > Also, the first patch may not be correct on RK3399 because I originally
> > wrote the patch for RK3288 on top of v4.4 where all drive updates only
> > affect a single register.  We don't need locking in this case because
> > regmap_update_bits() takes a lock on the regmap internally, but if these
> > two registers need to be updated atomically then another lock will
> > be required here - slock cannot be used if it is converted to a raw
> > spinlock since with full RT preemption the regmap's spinlock may sleep.
> 
> Nice work! It all looks good to me, let's see what Heiko says.
> 
> Please keep Julia Cartwright on the CC for this patch series, she is
> doing some coccinelle-based rewrites to use raw spinlocks as we
> speak, and she knows this stuff.
> 
> She has not targeted the Rockchip driver yet, I guess because of
> its complexity.

I haven't really given much thought to how we might generically solve
raw_spinlock problems for those drivers which make of irq_chip_generic
just yet.  I don't imagine it would be too difficult, just more work.

Thanks for the CC.

   Julia

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH 0/4] pinctrl: rockchip: PREEMPT_RT_FULL fixes

[Julia Cartwright]

[-- Attachment #1: Type: text/plain, Size: 1779 bytes --]

On Wed, Mar 15, 2017 at 02:12:39PM +0100, Linus Walleij wrote:
> On Mon, Mar 13, 2017 at 7:38 PM, John Keeping <john@metanate.com> wrote:
> 
> > As described in Documentation/gpio/driver.txt, we should not be using
> > sleepable APIs in the irqchip implementation.  Since this includes the
> > regmap API, this patch series ends up moving the mux setup for IRQs into
> > an irq_bus_sync_unlock() handler which may result in the IRQ being
> > configured before the port has been muxed as a GPIO.
> >
> > I've marked the series as RFC because I'm not sure if this is the best
> > way to accomplish this or if there is another approach that is cleaner.
> > Also, the first patch may not be correct on RK3399 because I originally
> > wrote the patch for RK3288 on top of v4.4 where all drive updates only
> > affect a single register.  We don't need locking in this case because
> > regmap_update_bits() takes a lock on the regmap internally, but if these
> > two registers need to be updated atomically then another lock will
> > be required here - slock cannot be used if it is converted to a raw
> > spinlock since with full RT preemption the regmap's spinlock may sleep.
> 
> Nice work! It all looks good to me, let's see what Heiko says.
> 
> Please keep Julia Cartwright on the CC for this patch series, she is
> doing some coccinelle-based rewrites to use raw spinlocks as we
> speak, and she knows this stuff.
> 
> She has not targeted the Rockchip driver yet, I guess because of
> its complexity.

I haven't really given much thought to how we might generically solve
raw_spinlock problems for those drivers which make of irq_chip_generic
just yet.  I don't imagine it would be too difficult, just more work.

Thanks for the CC.

   Julia

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[RFC PATCH 0/4] pinctrl: rockchip: PREEMPT_RT_FULL fixes

[Julia Cartwright]

On Wed, Mar 15, 2017 at 02:12:39PM +0100, Linus Walleij wrote:
> On Mon, Mar 13, 2017 at 7:38 PM, John Keeping <john@metanate.com> wrote:
> 
> > As described in Documentation/gpio/driver.txt, we should not be using
> > sleepable APIs in the irqchip implementation.  Since this includes the
> > regmap API, this patch series ends up moving the mux setup for IRQs into
> > an irq_bus_sync_unlock() handler which may result in the IRQ being
> > configured before the port has been muxed as a GPIO.
> >
> > I've marked the series as RFC because I'm not sure if this is the best
> > way to accomplish this or if there is another approach that is cleaner.
> > Also, the first patch may not be correct on RK3399 because I originally
> > wrote the patch for RK3288 on top of v4.4 where all drive updates only
> > affect a single register.  We don't need locking in this case because
> > regmap_update_bits() takes a lock on the regmap internally, but if these
> > two registers need to be updated atomically then another lock will
> > be required here - slock cannot be used if it is converted to a raw
> > spinlock since with full RT preemption the regmap's spinlock may sleep.
> 
> Nice work! It all looks good to me, let's see what Heiko says.
> 
> Please keep Julia Cartwright on the CC for this patch series, she is
> doing some coccinelle-based rewrites to use raw spinlocks as we
> speak, and she knows this stuff.
> 
> She has not targeted the Rockchip driver yet, I guess because of
> its complexity.

I haven't really given much thought to how we might generically solve
raw_spinlock problems for those drivers which make of irq_chip_generic
just yet.  I don't imagine it would be too difficult, just more work.

Thanks for the CC.

   Julia
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170315/c3ed4b8f/attachment.sig>

Re: [RFC PATCH 0/4] pinctrl: rockchip: PREEMPT_RT_FULL fixes

[Heiko Stuebner]

Am Montag, 13. März 2017, 18:38:09 CET schrieb John Keeping:
> As described in Documentation/gpio/driver.txt, we should not be using
> sleepable APIs in the irqchip implementation.  Since this includes the
> regmap API, this patch series ends up moving the mux setup for IRQs into
> an irq_bus_sync_unlock() handler which may result in the IRQ being
> configured before the port has been muxed as a GPIO.
> 
> I've marked the series as RFC because I'm not sure if this is the best
> way to accomplish this or if there is another approach that is cleaner.
> Also, the first patch may not be correct on RK3399 because I originally
> wrote the patch for RK3288 on top of v4.4 where all drive updates only
> affect a single register.  We don't need locking in this case because
> regmap_update_bits() takes a lock on the regmap internally, but if these
> two registers need to be updated atomically then another lock will
> be required here - slock cannot be used if it is converted to a raw
> spinlock since with full RT preemption the regmap's spinlock may sleep.

With the issue in patch2 fixed and top of the current head of Linus T's tree 
(4.11-rc2 + some patches), at least rk3036, rk3288, rk3368 and rk3399 come up 
nicely including multiple (1 to 3 per board) gpio-interrupts, so this series

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

[RFC PATCH 0/4] pinctrl: rockchip: PREEMPT_RT_FULL fixes

[Heiko Stuebner]

Am Montag, 13. M?rz 2017, 18:38:09 CET schrieb John Keeping:
> As described in Documentation/gpio/driver.txt, we should not be using
> sleepable APIs in the irqchip implementation.  Since this includes the
> regmap API, this patch series ends up moving the mux setup for IRQs into
> an irq_bus_sync_unlock() handler which may result in the IRQ being
> configured before the port has been muxed as a GPIO.
> 
> I've marked the series as RFC because I'm not sure if this is the best
> way to accomplish this or if there is another approach that is cleaner.
> Also, the first patch may not be correct on RK3399 because I originally
> wrote the patch for RK3288 on top of v4.4 where all drive updates only
> affect a single register.  We don't need locking in this case because
> regmap_update_bits() takes a lock on the regmap internally, but if these
> two registers need to be updated atomically then another lock will
> be required here - slock cannot be used if it is converted to a raw
> spinlock since with full RT preemption the regmap's spinlock may sleep.

With the issue in patch2 fixed and top of the current head of Linus T's tree 
(4.11-rc2 + some patches), at least rk3036, rk3288, rk3368 and rk3399 come up 
nicely including multiple (1 to 3 per board) gpio-interrupts, so this series

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