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

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

[John Keeping]

The aim of the series is to make the Rockchip pinctrl irq_chip
implementation safe for use with RT_FULL which requires that raw
spinlocks are used to avoid sleeping in hardirq context.

v4 is v3 rebased onto pinctrl/devel as of b9c6dcab265e ("pinctrl: rockchip:
rename RK1108 to RV1108").

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 | 149 +++++++++++++++++++++----------------
 1 file changed, 85 insertions(+), 64 deletions(-)

-- 
2.12.0.377.gf910686b23.dirty

[PATCH v4 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.

Signed-off-by: John Keeping <john@metanate.com>
Reviewed-by: Heiko Stuebner <heiko@sntech.de>
Tested-by: Heiko Stuebner <heiko@sntech.de>
---
v3: unchanged
v2.1:
- Remove RK2928 locking in rockchip_set_pull()
v2:
- Also remove locking in rockchip_set_schmitt()
---
 drivers/pinctrl/pinctrl-rockchip.c | 33 ++-------------------------------
 1 file changed, 2 insertions(+), 31 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c
index bd4b63f66220..6568c867bdcd 100644
--- a/drivers/pinctrl/pinctrl-rockchip.c
+++ b/drivers/pinctrl/pinctrl-rockchip.c
@@ -652,7 +652,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;
 
@@ -701,15 +700,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;
 }
 
@@ -1135,7 +1130,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;
@@ -1163,8 +1157,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:
@@ -1185,17 +1177,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 */
@@ -1203,7 +1192,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;
@@ -1215,7 +1203,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;
@@ -1227,7 +1214,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;
 }
@@ -1294,7 +1280,6 @@ static int rockchip_set_pull(struct rockchip_pin_bank *bank,
 	struct rockchip_pin_ctrl *ctrl = info->ctrl;
 	struct regmap *regmap;
 	int reg, ret, i, pull_type;
-	unsigned long flags;
 	u8 bit;
 	u32 data, rmask;
 
@@ -1309,14 +1294,10 @@ static int rockchip_set_pull(struct rockchip_pin_bank *bank,
 
 	switch (ctrl->type) {
 	case RK2928:
-		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);
 		break;
 	case RV1108:
 	case RK3188:
@@ -1339,16 +1320,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");
@@ -1408,7 +1385,6 @@ static int rockchip_set_schmitt(struct rockchip_pin_bank *bank,
 	struct rockchip_pin_ctrl *ctrl = info->ctrl;
 	struct regmap *regmap;
 	int reg, ret;
-	unsigned long flags;
 	u8 bit;
 	u32 data, rmask;
 
@@ -1419,16 +1395,11 @@ static int rockchip_set_schmitt(struct rockchip_pin_bank *bank,
 	if (ret)
 		return ret;
 
-	spin_lock_irqsave(&bank->slock, flags);
-
 	/* enable the write to the equivalent lower bits */
 	data = BIT(bit + 16) | (enable << bit);
 	rmask = BIT(bit + 16) | BIT(bit);
 
-	ret = regmap_update_bits(regmap, reg, rmask, data);
-	spin_unlock_irqrestore(&bank->slock, flags);
-
-	return ret;
+	return regmap_update_bits(regmap, reg, rmask, data);
 }
 
 /*
-- 
2.12.0.377.gf910686b23.dirty

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

[Julia Cartwright]

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

Hello John-

One quick question below.  Apologies if this has been covered, but just
want to be sure.

On Thu, Mar 23, 2017 at 10:59:28AM +0000, John Keeping wrote:
> 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.
> 
> Signed-off-by: John Keeping <john@metanate.com>
> Reviewed-by: Heiko Stuebner <heiko@sntech.de>
> Tested-by: Heiko Stuebner <heiko@sntech.de>
> ---
> v3: unchanged
> v2.1:
> - Remove RK2928 locking in rockchip_set_pull()
> v2:
> - Also remove locking in rockchip_set_schmitt()
> ---
>  drivers/pinctrl/pinctrl-rockchip.c | 33 ++-------------------------------
>  1 file changed, 2 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c
> index bd4b63f66220..6568c867bdcd 100644
> --- a/drivers/pinctrl/pinctrl-rockchip.c
> +++ b/drivers/pinctrl/pinctrl-rockchip.c
[..]
> @@ -1185,17 +1177,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);

Killing the lock here means the writes to to this pair of registers (reg
and reg + 4) can be observed non-atomically.  Have you convinced
yourself that this isn't a problem?

   Julia

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

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

[John Keeping]

On Thu, 23 Mar 2017 11:10:20 -0500, Julia Cartwright wrote:

> One quick question below.  Apologies if this has been covered, but just
> want to be sure.
> 
> On Thu, Mar 23, 2017 at 10:59:28AM +0000, John Keeping wrote:
> > 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.
> > 
> > Signed-off-by: John Keeping <john@metanate.com>
> > Reviewed-by: Heiko Stuebner <heiko@sntech.de>
> > Tested-by: Heiko Stuebner <heiko@sntech.de>
> > ---
> > v3: unchanged
> > v2.1:
> > - Remove RK2928 locking in rockchip_set_pull()
> > v2:
> > - Also remove locking in rockchip_set_schmitt()
> > ---
> >  drivers/pinctrl/pinctrl-rockchip.c | 33 ++-------------------------------
> >  1 file changed, 2 insertions(+), 31 deletions(-)
> > 
> > diff --git a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c
> > index bd4b63f66220..6568c867bdcd 100644
> > --- a/drivers/pinctrl/pinctrl-rockchip.c
> > +++ b/drivers/pinctrl/pinctrl-rockchip.c  
> [..]
> > @@ -1185,17 +1177,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);  
> 
> Killing the lock here means the writes to to this pair of registers (reg
> and reg + 4) can be observed non-atomically.  Have you convinced
> yourself that this isn't a problem?

I called it out in v1 [1] since this bit is new since v4.4 where I
originally wrote this patch, and didn't get any comments about it.

I've convinced myself that removing the lock doesn't cause any problems
for writing to the hardware: if the lock would prevent writes
interleaving then it means that two callers are trying to write
different drive strengths to the same pin, and even with a lock here one
of them will end up with the wrong drive strength.

But it does mean that a read via rockchip_get_drive_perpin() may see an
inconsistent state.  I think adding a new lock specifically for this
particular drive strength bit is overkill and I can't find a scenario
where this will actually matter; any driver setting a pinctrl config
must already be doing something to avoid racing two configurations
against each other, mustn't it?

[1] https://www.spinics.net/lists/arm-kernel/msg568925.html


John

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

[Heiko Stübner]

Am Donnerstag, 23. März 2017, 17:51:53 CET schrieb John Keeping:
> On Thu, 23 Mar 2017 11:10:20 -0500, Julia Cartwright wrote:
> > One quick question below.  Apologies if this has been covered, but just
> > want to be sure.
> > 
> > On Thu, Mar 23, 2017 at 10:59:28AM +0000, John Keeping wrote:
> > > 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.
> > > 
> > > Signed-off-by: John Keeping <john@metanate.com>
> > > Reviewed-by: Heiko Stuebner <heiko@sntech.de>
> > > Tested-by: Heiko Stuebner <heiko@sntech.de>
> > > ---
> > > v3: unchanged
> > > v2.1:
> > > - Remove RK2928 locking in rockchip_set_pull()
> > > v2:
> > > - Also remove locking in rockchip_set_schmitt()
> > > ---
> > > 
> > >  drivers/pinctrl/pinctrl-rockchip.c | 33
> > >  ++-------------------------------
> > >  1 file changed, 2 insertions(+), 31 deletions(-)
> > > 
> > > diff --git a/drivers/pinctrl/pinctrl-rockchip.c
> > > b/drivers/pinctrl/pinctrl-rockchip.c index bd4b63f66220..6568c867bdcd
> > > 100644
> > > --- a/drivers/pinctrl/pinctrl-rockchip.c
> > > +++ b/drivers/pinctrl/pinctrl-rockchip.c
> > 
> > [..]
> > 
> > > @@ -1185,17 +1177,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);
> > 
> > Killing the lock here means the writes to to this pair of registers (reg
> > and reg + 4) can be observed non-atomically.  Have you convinced
> > yourself that this isn't a problem?
> 
> I called it out in v1 [1] since this bit is new since v4.4 where I
> originally wrote this patch, and didn't get any comments about it.
> 
> I've convinced myself that removing the lock doesn't cause any problems
> for writing to the hardware: if the lock would prevent writes
> interleaving then it means that two callers are trying to write
> different drive strengths to the same pin, and even with a lock here one
> of them will end up with the wrong drive strength.
> 
> But it does mean that a read via rockchip_get_drive_perpin() may see an
> inconsistent state.  I think adding a new lock specifically for this
> particular drive strength bit is overkill and I can't find a scenario
> where this will actually matter; any driver setting a pinctrl config
> must already be doing something to avoid racing two configurations
> against each other, mustn't it?

also, pins can normally only be requested once - see drivers complaining if 
one of their pins is already held by a different driver. So if you really end 
up with two things writing to the same drive strength bits, the driver holding 
the pins must be really messed up anyway :-)


Heiko

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

[Julia Cartwright]

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

On Thu, Mar 23, 2017 at 06:55:50PM +0100, Heiko St?bner wrote:
> Am Donnerstag, 23. März 2017, 17:51:53 CET schrieb John Keeping:
> > On Thu, 23 Mar 2017 11:10:20 -0500, Julia Cartwright wrote:
[..]
> > > [..]
> > > 
> > > > @@ -1185,17 +1177,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);
> > > 
> > > Killing the lock here means the writes to to this pair of registers (reg
> > > and reg + 4) can be observed non-atomically.  Have you convinced
> > > yourself that this isn't a problem?
> > 
> > I called it out in v1 [1] since this bit is new since v4.4 where I
> > originally wrote this patch, and didn't get any comments about it.
> > 
> > I've convinced myself that removing the lock doesn't cause any problems
> > for writing to the hardware: if the lock would prevent writes
> > interleaving then it means that two callers are trying to write
> > different drive strengths to the same pin, and even with a lock here one
> > of them will end up with the wrong drive strength.
> > 
> > But it does mean that a read via rockchip_get_drive_perpin() may see an
> > inconsistent state.  I think adding a new lock specifically for this
> > particular drive strength bit is overkill and I can't find a scenario
> > where this will actually matter; any driver setting a pinctrl config
> > must already be doing something to avoid racing two configurations
> > against each other, mustn't it?
> 
> also, pins can normally only be requested once - see drivers complaining if 
> one of their pins is already held by a different driver. So if you really end 
> up with two things writing to the same drive strength bits, the driver holding 
> the pins must be really messed up anyway :-)

My concern would be if two independent pins' drive strength
configuration would walk on each other, because they happen to be
configured via the same registers.

If that's not possible, then great!

   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: [PATCH v4 1/4] pinctrl: rockchip: remove unnecessary locking

[Heiko Stübner]

Am Donnerstag, 23. März 2017, 13:29:10 CET schrieb Julia Cartwright:
> On Thu, Mar 23, 2017 at 06:55:50PM +0100, Heiko St?bner wrote:
> > Am Donnerstag, 23. März 2017, 17:51:53 CET schrieb John Keeping:
> > > On Thu, 23 Mar 2017 11:10:20 -0500, Julia Cartwright wrote:
> [..]
> 
> > > > [..]
> > > > 
> > > > > @@ -1185,17 +1177,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);
> > > > 
> > > > Killing the lock here means the writes to to this pair of registers
> > > > (reg
> > > > and reg + 4) can be observed non-atomically.  Have you convinced
> > > > yourself that this isn't a problem?
> > > 
> > > I called it out in v1 [1] since this bit is new since v4.4 where I
> > > originally wrote this patch, and didn't get any comments about it.
> > > 
> > > I've convinced myself that removing the lock doesn't cause any problems
> > > for writing to the hardware: if the lock would prevent writes
> > > interleaving then it means that two callers are trying to write
> > > different drive strengths to the same pin, and even with a lock here one
> > > of them will end up with the wrong drive strength.
> > > 
> > > But it does mean that a read via rockchip_get_drive_perpin() may see an
> > > inconsistent state.  I think adding a new lock specifically for this
> > > particular drive strength bit is overkill and I can't find a scenario
> > > where this will actually matter; any driver setting a pinctrl config
> > > must already be doing something to avoid racing two configurations
> > > against each other, mustn't it?
> > 
> > also, pins can normally only be requested once - see drivers complaining
> > if
> > one of their pins is already held by a different driver. So if you really
> > end up with two things writing to the same drive strength bits, the
> > driver holding the pins must be really messed up anyway :-)
> 
> My concern would be if two independent pins' drive strength
> configuration would walk on each other, because they happen to be
> configured via the same registers.
> 
> If that's not possible, then great!

ah sorry that we didn't make that clearer in the beginning, but no that also 
isn't possible. The registers use a "hiword-mask" scheme, so on each write to 
bit (x) the corresponding write-mask in bit (x+16) also needs to be set, so 
the each write will always only affect bits enabled in that way.

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

[Julia Cartwright]

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

On Thu, Mar 23, 2017 at 09:01:43PM +0100, Heiko St?bner wrote:
> Am Donnerstag, 23. März 2017, 13:29:10 CET schrieb Julia Cartwright:
> > On Thu, Mar 23, 2017 at 06:55:50PM +0100, Heiko St?bner wrote:
> > > Am Donnerstag, 23. März 2017, 17:51:53 CET schrieb John Keeping:
> > > > On Thu, 23 Mar 2017 11:10:20 -0500, Julia Cartwright wrote:
[..]
> > > > > > @@ -1185,17 +1177,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);
> > > > > 
> > > > > Killing the lock here means the writes to to this pair of registers
> > > > > (reg
> > > > > and reg + 4) can be observed non-atomically.  Have you convinced
> > > > > yourself that this isn't a problem?
> > > > 
> > > > I called it out in v1 [1] since this bit is new since v4.4 where I
> > > > originally wrote this patch, and didn't get any comments about it.
> > > > 
> > > > I've convinced myself that removing the lock doesn't cause any problems
> > > > for writing to the hardware: if the lock would prevent writes
> > > > interleaving then it means that two callers are trying to write
> > > > different drive strengths to the same pin, and even with a lock here one
> > > > of them will end up with the wrong drive strength.
> > > > 
> > > > But it does mean that a read via rockchip_get_drive_perpin() may see an
> > > > inconsistent state.  I think adding a new lock specifically for this
> > > > particular drive strength bit is overkill and I can't find a scenario
> > > > where this will actually matter; any driver setting a pinctrl config
> > > > must already be doing something to avoid racing two configurations
> > > > against each other, mustn't it?
> > > 
> > > also, pins can normally only be requested once - see drivers complaining
> > > if
> > > one of their pins is already held by a different driver. So if you really
> > > end up with two things writing to the same drive strength bits, the
> > > driver holding the pins must be really messed up anyway :-)
> > 
> > My concern would be if two independent pins' drive strength
> > configuration would walk on each other, because they happen to be
> > configured via the same registers.
> > 
> > If that's not possible, then great!
> 
> ah sorry that we didn't make that clearer in the beginning, but no that also 
> isn't possible. The registers use a "hiword-mask" scheme, so on each write to 
> bit (x) the corresponding write-mask in bit (x+16) also needs to be set, so 
> the each write will always only affect bits enabled in that way.

Awesome, thanks for clearing things up.

Thanks!
   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: [PATCH v4 1/4] pinctrl: rockchip: remove unnecessary locking

[Linus Walleij]

On Thu, Mar 23, 2017 at 11:59 AM, John Keeping <john@metanate.com> wrote:

> 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.
>
> Signed-off-by: John Keeping <john@metanate.com>
> Reviewed-by: Heiko Stuebner <heiko@sntech.de>
> Tested-by: Heiko Stuebner <heiko@sntech.de>
> ---
> v3: unchanged
> v2.1:
> - Remove RK2928 locking in rockchip_set_pull()
> v2:
> - Also remove locking in rockchip_set_schmitt()

Patch applied.

Yours,
Linus Walleij

[PATCH v4 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>
Reviewed-by: Heiko Stuebner <heiko@sntech.de>
Tested-by: Heiko Stuebner <heiko@sntech.de>
---
v3: unchanged
v2: unchanged
---
 drivers/pinctrl/pinctrl-rockchip.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c
index 6568c867bdcd..5d5a9c4c522d 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;
 };
 
@@ -1498,7 +1498,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 */
@@ -1508,7 +1508,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;
@@ -1958,7 +1958,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);
@@ -1966,7 +1966,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);
 }
 
@@ -2078,7 +2078,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);
@@ -2089,7 +2089,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 +
@@ -2120,20 +2120,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);
@@ -2176,7 +2176,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;
 	}
@@ -2185,7 +2185,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;
@@ -2468,7 +2468,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: [PATCH v4 2/4] pinctrl: rockchip: convert to raw spinlock

[Linus Walleij]

On Thu, Mar 23, 2017 at 11:59 AM, John Keeping <john@metanate.com> wrote:

> 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>
> Reviewed-by: Heiko Stuebner <heiko@sntech.de>
> Tested-by: Heiko Stuebner <heiko@sntech.de>
> ---
> v3: unchanged
> v2: unchanged

Patch applied.

Yours,
Linus Walleij

[PATCH v4 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>
Reviewed-by: Heiko Stuebner <heiko@sntech.de>
Tested-by: Heiko Stuebner <heiko@sntech.de>
---
v3: unchanged
v2: unchanged
---
 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 5d5a9c4c522d..9dd981ddbb17 100644
--- a/drivers/pinctrl/pinctrl-rockchip.c
+++ b/drivers/pinctrl/pinctrl-rockchip.c
@@ -632,6 +632,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.
  *
@@ -655,23 +680,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: [PATCH v4 3/4] pinctrl: rockchip: split out verification of mux settings

[Linus Walleij]

On Thu, Mar 23, 2017 at 11:59 AM, John Keeping <john@metanate.com> wrote:

> 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>
> Reviewed-by: Heiko Stuebner <heiko@sntech.de>
> Tested-by: Heiko Stuebner <heiko@sntech.de>
> ---
> v3: unchanged
> v2: unchanged

Patch applied.

Yours,
Linus Walleij

[PATCH v4 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>
Reviewed-by: Heiko Stuebner <heiko@sntech.de>
Tested-by: Heiko Stuebner <heiko@sntech.de>
---
v3: unchanged
v2: unchanged
---
 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 9dd981ddbb17..f141aa0430b1 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)			\
@@ -2129,11 +2134,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);
@@ -2191,7 +2197,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;
 	}
 
@@ -2200,7 +2205,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;
 }
@@ -2244,6 +2248,34 @@ static void rockchip_irq_disable(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)
 {
@@ -2310,6 +2342,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,
@@ -2483,6 +2518,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: [PATCH v4 4/4] pinctrl: rockchip: avoid hardirq-unsafe functions in irq_chip

[Linus Walleij]

On Thu, Mar 23, 2017 at 11:59 AM, John Keeping <john@metanate.com> wrote:

> 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>
> Reviewed-by: Heiko Stuebner <heiko@sntech.de>
> Tested-by: Heiko Stuebner <heiko@sntech.de>
> ---
> v3: unchanged
> v2: unchanged

Patch applied.

Yours,
Linus Walleij