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

2017-03-15 Thread John Keeping 
On Wed, 15 Mar 2017 13:23:09 -0500, Julia Cartwright wrote:
> On Wed, Mar 15, 2017 at 07:16:53PM +0100, Heiko Stuebner wrote:
> > Am Mittwoch, 15. März 2017, 18:08:06 CET schrieb John Keeping:  
> > > On Wed, 15 Mar 2017 13:01:37 -0500, Julia Cartwright wrote:  
> > > > On Wed, Mar 15, 2017 at 05:46:52PM +, John Keeping 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 
> > > > > Reviewed-by: Heiko Stuebner 
> > > > > Tested-by: Heiko Stuebner 
> > > > > ---
> > > > > v2: unchanged
> > > > > ---
> > > > > 
> > > > >  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 128c383ea7ba..8c1cae6d78d7
> > > > > 100644  
> [..]
> > > > > @@ -1295,14 +1295,14 @@ static int rockchip_set_pull(struct
> > > > > rockchip_pin_bank *bank,> > 
> > > > >   switch (ctrl->type) {
> > > > > 
> > > > >   case RK2928:
> > > > > - spin_lock_irqsave(>slock, flags);
> > > > > + raw_spin_lock_irqsave(>slock, flags);
> > > > > 
> > > > >   data = BIT(bit + 16);
> > > > >   if (pull == PIN_CONFIG_BIAS_DISABLE)
> > > > >   
> > > > >   data |= BIT(bit);  
> > > > 
> > > > This should be lifted out from under the lock.
> > > >   
> > > > >   ret = regmap_write(regmap, reg, data);  
> > > > 
> > > > How is this legal?  The regmap_write() here is going to end up acquiring
> > > > the regmap mutex.  
> > > 
> > > It's not, the spinlock can be deleted here.  I only have RK3288 hardware
> > > to test and I missed this when checking the uses of slock.  
> > 
> > That part could very well also use regmap_update_bits like the other parts.
> > Not really sure, why we use regmap_write here, but I'm also not sure, if it 
> > matters at all.  
> 
> regmap_update_bits also acquires the regmap lock, which would similarly
> be a problem here.[1]
> 
> But, if we could pull this entire operation out of the lock (and
> convince ourselves that it's okay to do so), then even better!

Yes, we can delete the lock here for the same reason as all of the
others that are removed in patch 1.

I don't think it makes much difference whether we use regmap_write or
regmap_update_bits here (although consistently using regmap_update_bits
might be nice) so I won't change it as part of this series, especially
since I don't have an RK2928 to test.


John

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

2017-03-15 Thread John Keeping 
On Wed, 15 Mar 2017 13:23:09 -0500, Julia Cartwright wrote:
> On Wed, Mar 15, 2017 at 07:16:53PM +0100, Heiko Stuebner wrote:
> > Am Mittwoch, 15. März 2017, 18:08:06 CET schrieb John Keeping:  
> > > On Wed, 15 Mar 2017 13:01:37 -0500, Julia Cartwright wrote:  
> > > > On Wed, Mar 15, 2017 at 05:46:52PM +, John Keeping 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 
> > > > > Reviewed-by: Heiko Stuebner 
> > > > > Tested-by: Heiko Stuebner 
> > > > > ---
> > > > > v2: unchanged
> > > > > ---
> > > > > 
> > > > >  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 128c383ea7ba..8c1cae6d78d7
> > > > > 100644  
> [..]
> > > > > @@ -1295,14 +1295,14 @@ static int rockchip_set_pull(struct
> > > > > rockchip_pin_bank *bank,> > 
> > > > >   switch (ctrl->type) {
> > > > > 
> > > > >   case RK2928:
> > > > > - spin_lock_irqsave(>slock, flags);
> > > > > + raw_spin_lock_irqsave(>slock, flags);
> > > > > 
> > > > >   data = BIT(bit + 16);
> > > > >   if (pull == PIN_CONFIG_BIAS_DISABLE)
> > > > >   
> > > > >   data |= BIT(bit);  
> > > > 
> > > > This should be lifted out from under the lock.
> > > >   
> > > > >   ret = regmap_write(regmap, reg, data);  
> > > > 
> > > > How is this legal?  The regmap_write() here is going to end up acquiring
> > > > the regmap mutex.  
> > > 
> > > It's not, the spinlock can be deleted here.  I only have RK3288 hardware
> > > to test and I missed this when checking the uses of slock.  
> > 
> > That part could very well also use regmap_update_bits like the other parts.
> > Not really sure, why we use regmap_write here, but I'm also not sure, if it 
> > matters at all.  
> 
> regmap_update_bits also acquires the regmap lock, which would similarly
> be a problem here.[1]
> 
> But, if we could pull this entire operation out of the lock (and
> convince ourselves that it's okay to do so), then even better!

Yes, we can delete the lock here for the same reason as all of the
others that are removed in patch 1.

I don't think it makes much difference whether we use regmap_write or
regmap_update_bits here (although consistently using regmap_update_bits
might be nice) so I won't change it as part of this series, especially
since I don't have an RK2928 to test.


John

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

2017-03-15 Thread Julia Cartwright 
On Wed, Mar 15, 2017 at 07:16:53PM +0100, Heiko Stuebner wrote:
> Am Mittwoch, 15. März 2017, 18:08:06 CET schrieb John Keeping:
> > On Wed, 15 Mar 2017 13:01:37 -0500, Julia Cartwright wrote:
> > > On Wed, Mar 15, 2017 at 05:46:52PM +, John Keeping 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 
> > > > Reviewed-by: Heiko Stuebner 
> > > > Tested-by: Heiko Stuebner 
> > > > ---
> > > > v2: unchanged
> > > > ---
> > > > 
> > > >  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 128c383ea7ba..8c1cae6d78d7
> > > > 100644
[..]
> > > > @@ -1295,14 +1295,14 @@ static int rockchip_set_pull(struct
> > > > rockchip_pin_bank *bank,> > 
> > > > switch (ctrl->type) {
> > > > 
> > > > case RK2928:
> > > > -   spin_lock_irqsave(>slock, flags);
> > > > +   raw_spin_lock_irqsave(>slock, flags);
> > > > 
> > > > data = BIT(bit + 16);
> > > > if (pull == PIN_CONFIG_BIAS_DISABLE)
> > > > 
> > > > data |= BIT(bit);
> > > 
> > > This should be lifted out from under the lock.
> > > 
> > > > ret = regmap_write(regmap, reg, data);
> > > 
> > > How is this legal?  The regmap_write() here is going to end up acquiring
> > > the regmap mutex.
> > 
> > It's not, the spinlock can be deleted here.  I only have RK3288 hardware
> > to test and I missed this when checking the uses of slock.
> 
> That part could very well also use regmap_update_bits like the other parts.
> Not really sure, why we use regmap_write here, but I'm also not sure, if it 
> matters at all.

regmap_update_bits also acquires the regmap lock, which would similarly
be a problem here.[1]

But, if we could pull this entire operation out of the lock (and
convince ourselves that it's okay to do so), then even better!

   Julia

1: Why is this a problem?  Because we're in the middle of a
   raw_spinlock_t protected critical region: if there were contention on
   the nested mutex (the "regmap mutex"), then we'd attempt to sleep in
   atomic context.


signature.asc
Description: PGP signature

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

2017-03-15 Thread Julia Cartwright 
On Wed, Mar 15, 2017 at 07:16:53PM +0100, Heiko Stuebner wrote:
> Am Mittwoch, 15. März 2017, 18:08:06 CET schrieb John Keeping:
> > On Wed, 15 Mar 2017 13:01:37 -0500, Julia Cartwright wrote:
> > > On Wed, Mar 15, 2017 at 05:46:52PM +, John Keeping 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 
> > > > Reviewed-by: Heiko Stuebner 
> > > > Tested-by: Heiko Stuebner 
> > > > ---
> > > > v2: unchanged
> > > > ---
> > > > 
> > > >  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 128c383ea7ba..8c1cae6d78d7
> > > > 100644
[..]
> > > > @@ -1295,14 +1295,14 @@ static int rockchip_set_pull(struct
> > > > rockchip_pin_bank *bank,> > 
> > > > switch (ctrl->type) {
> > > > 
> > > > case RK2928:
> > > > -   spin_lock_irqsave(>slock, flags);
> > > > +   raw_spin_lock_irqsave(>slock, flags);
> > > > 
> > > > data = BIT(bit + 16);
> > > > if (pull == PIN_CONFIG_BIAS_DISABLE)
> > > > 
> > > > data |= BIT(bit);
> > > 
> > > This should be lifted out from under the lock.
> > > 
> > > > ret = regmap_write(regmap, reg, data);
> > > 
> > > How is this legal?  The regmap_write() here is going to end up acquiring
> > > the regmap mutex.
> > 
> > It's not, the spinlock can be deleted here.  I only have RK3288 hardware
> > to test and I missed this when checking the uses of slock.
> 
> That part could very well also use regmap_update_bits like the other parts.
> Not really sure, why we use regmap_write here, but I'm also not sure, if it 
> matters at all.

regmap_update_bits also acquires the regmap lock, which would similarly
be a problem here.[1]

But, if we could pull this entire operation out of the lock (and
convince ourselves that it's okay to do so), then even better!

   Julia

1: Why is this a problem?  Because we're in the middle of a
   raw_spinlock_t protected critical region: if there were contention on
   the nested mutex (the "regmap mutex"), then we'd attempt to sleep in
   atomic context.


signature.asc
Description: PGP signature

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

2017-03-15 Thread Heiko Stuebner 
Am Mittwoch, 15. März 2017, 18:08:06 CET schrieb John Keeping:
> On Wed, 15 Mar 2017 13:01:37 -0500, Julia Cartwright wrote:
> > On Wed, Mar 15, 2017 at 05:46:52PM +, John Keeping 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 
> > > Reviewed-by: Heiko Stuebner 
> > > Tested-by: Heiko Stuebner 
> > > ---
> > > v2: unchanged
> > > ---
> > > 
> > >  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 128c383ea7ba..8c1cae6d78d7
> > > 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_chipgpio_chip;
> > >   struct pinctrl_gpio_range   grange;
> > > 
> > > - spinlock_t  slock;
> > > + raw_spinlock_t  slock;
> > > 
> > >   u32 toggle_edge_mode;
> > >  
> > >  };
> > > 
> > > @@ -1295,14 +1295,14 @@ static int rockchip_set_pull(struct
> > > rockchip_pin_bank *bank,> > 
> > >   switch (ctrl->type) {
> > > 
> > >   case RK2928:
> > > - spin_lock_irqsave(>slock, flags);
> > > + raw_spin_lock_irqsave(>slock, flags);
> > > 
> > >   data = BIT(bit + 16);
> > >   if (pull == PIN_CONFIG_BIAS_DISABLE)
> > >   
> > >   data |= BIT(bit);
> > 
> > This should be lifted out from under the lock.
> > 
> > >   ret = regmap_write(regmap, reg, data);
> > 
> > How is this legal?  The regmap_write() here is going to end up acquiring
> > the regmap mutex.
> 
> It's not, the spinlock can be deleted here.  I only have RK3288 hardware
> to test and I missed this when checking the uses of slock.

That part could very well also use regmap_update_bits like the other parts.
Not really sure, why we use regmap_write here, but I'm also not sure, if it 
matters at all.

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

2017-03-15 Thread Heiko Stuebner 
Am Mittwoch, 15. März 2017, 18:08:06 CET schrieb John Keeping:
> On Wed, 15 Mar 2017 13:01:37 -0500, Julia Cartwright wrote:
> > On Wed, Mar 15, 2017 at 05:46:52PM +, John Keeping 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 
> > > Reviewed-by: Heiko Stuebner 
> > > Tested-by: Heiko Stuebner 
> > > ---
> > > v2: unchanged
> > > ---
> > > 
> > >  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 128c383ea7ba..8c1cae6d78d7
> > > 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_chipgpio_chip;
> > >   struct pinctrl_gpio_range   grange;
> > > 
> > > - spinlock_t  slock;
> > > + raw_spinlock_t  slock;
> > > 
> > >   u32 toggle_edge_mode;
> > >  
> > >  };
> > > 
> > > @@ -1295,14 +1295,14 @@ static int rockchip_set_pull(struct
> > > rockchip_pin_bank *bank,> > 
> > >   switch (ctrl->type) {
> > > 
> > >   case RK2928:
> > > - spin_lock_irqsave(>slock, flags);
> > > + raw_spin_lock_irqsave(>slock, flags);
> > > 
> > >   data = BIT(bit + 16);
> > >   if (pull == PIN_CONFIG_BIAS_DISABLE)
> > >   
> > >   data |= BIT(bit);
> > 
> > This should be lifted out from under the lock.
> > 
> > >   ret = regmap_write(regmap, reg, data);
> > 
> > How is this legal?  The regmap_write() here is going to end up acquiring
> > the regmap mutex.
> 
> It's not, the spinlock can be deleted here.  I only have RK3288 hardware
> to test and I missed this when checking the uses of slock.

That part could very well also use regmap_update_bits like the other parts.
Not really sure, why we use regmap_write here, but I'm also not sure, if it 
matters at all.

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

2017-03-15 Thread 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 
Reviewed-by: Heiko Stuebner 
Tested-by: Heiko Stuebner 
---
v2: unchanged
---
 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 128c383ea7ba..8c1cae6d78d7 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_chipgpio_chip;
struct pinctrl_gpio_range   grange;
-   spinlock_t  slock;
+   raw_spinlock_t  slock;
u32 toggle_edge_mode;
 };
 
@@ -1295,14 +1295,14 @@ static int rockchip_set_pull(struct rockchip_pin_bank 
*bank,
 
switch (ctrl->type) {
case RK2928:
-   spin_lock_irqsave(>slock, flags);
+   raw_spin_lock_irqsave(>slock, flags);
 
data = BIT(bit + 16);
if (pull == PIN_CONFIG_BIAS_DISABLE)
data |= BIT(bit);
ret = regmap_write(regmap, reg, data);
 
-   spin_unlock_irqrestore(>slock, flags);
+   raw_spin_unlock_irqrestore(>slock, flags);
break;
case RK1108:
case RK3188:
@@ -1503,7 +1503,7 @@ static int _rockchip_pmx_gpio_set_direction(struct 
gpio_chip *chip,
return ret;
 
clk_enable(bank->clk);
-   spin_lock_irqsave(>slock, flags);
+   raw_spin_lock_irqsave(>slock, flags);
 
data = readl_relaxed(bank->reg_base + GPIO_SWPORT_DDR);
/* set bit to 1 for output, 0 for input */
@@ -1513,7 +1513,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(>slock, flags);
+   raw_spin_unlock_irqrestore(>slock, flags);
clk_disable(bank->clk);
 
return 0;
@@ -1963,7 +1963,7 @@ static void rockchip_gpio_set(struct gpio_chip *gc, 
unsigned offset, int value)
u32 data;
 
clk_enable(bank->clk);
-   spin_lock_irqsave(>slock, flags);
+   raw_spin_lock_irqsave(>slock, flags);
 
data = readl(reg);
data &= ~BIT(offset);
@@ -1971,7 +1971,7 @@ static void rockchip_gpio_set(struct gpio_chip *gc, 
unsigned offset, int value)
data |= BIT(offset);
writel(data, reg);
 
-   spin_unlock_irqrestore(>slock, flags);
+   raw_spin_unlock_irqrestore(>slock, flags);
clk_disable(bank->clk);
 }
 
@@ -2083,7 +2083,7 @@ static void rockchip_irq_demux(struct irq_desc *desc)
 
data = readl_relaxed(bank->reg_base + GPIO_EXT_PORT);
do {
-   spin_lock_irqsave(>slock, flags);
+   raw_spin_lock_irqsave(>slock, flags);
 
polarity = readl_relaxed(bank->reg_base +
 GPIO_INT_POLARITY);
@@ -2094,7 +2094,7 @@ static void rockchip_irq_demux(struct irq_desc *desc)
writel(polarity,
   bank->reg_base + GPIO_INT_POLARITY);
 
-   spin_unlock_irqrestore(>slock, flags);
+   raw_spin_unlock_irqrestore(>slock, flags);
 
data_old = data;
data = readl_relaxed(bank->reg_base +
@@ -2125,20 +2125,20 @@ static int rockchip_irq_set_type(struct irq_data *d, 
unsigned int type)
return ret;
 
clk_enable(bank->clk);
-   spin_lock_irqsave(>slock, flags);
+   raw_spin_lock_irqsave(>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(>slock, flags);
+   raw_spin_unlock_irqrestore(>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(>slock, flags);
+   raw_spin_lock_irqsave(>slock, flags);
irq_gc_lock(gc);
 
level = readl_relaxed(gc->reg_base + GPIO_INTTYPE_LEVEL);
@@ -2181,7 +2181,7 @@ static int rockchip_irq_set_type(struct irq_data *d, 
unsigned int type)
break;
default:
irq_gc_unlock(gc);
-   spin_unlock_irqrestore(>slock, flags);
+

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

2017-03-15 Thread 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 
Reviewed-by: Heiko Stuebner 
Tested-by: Heiko Stuebner 
---
v2: unchanged
---
 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 128c383ea7ba..8c1cae6d78d7 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_chipgpio_chip;
struct pinctrl_gpio_range   grange;
-   spinlock_t  slock;
+   raw_spinlock_t  slock;
u32 toggle_edge_mode;
 };
 
@@ -1295,14 +1295,14 @@ static int rockchip_set_pull(struct rockchip_pin_bank 
*bank,
 
switch (ctrl->type) {
case RK2928:
-   spin_lock_irqsave(>slock, flags);
+   raw_spin_lock_irqsave(>slock, flags);
 
data = BIT(bit + 16);
if (pull == PIN_CONFIG_BIAS_DISABLE)
data |= BIT(bit);
ret = regmap_write(regmap, reg, data);
 
-   spin_unlock_irqrestore(>slock, flags);
+   raw_spin_unlock_irqrestore(>slock, flags);
break;
case RK1108:
case RK3188:
@@ -1503,7 +1503,7 @@ static int _rockchip_pmx_gpio_set_direction(struct 
gpio_chip *chip,
return ret;
 
clk_enable(bank->clk);
-   spin_lock_irqsave(>slock, flags);
+   raw_spin_lock_irqsave(>slock, flags);
 
data = readl_relaxed(bank->reg_base + GPIO_SWPORT_DDR);
/* set bit to 1 for output, 0 for input */
@@ -1513,7 +1513,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(>slock, flags);
+   raw_spin_unlock_irqrestore(>slock, flags);
clk_disable(bank->clk);
 
return 0;
@@ -1963,7 +1963,7 @@ static void rockchip_gpio_set(struct gpio_chip *gc, 
unsigned offset, int value)
u32 data;
 
clk_enable(bank->clk);
-   spin_lock_irqsave(>slock, flags);
+   raw_spin_lock_irqsave(>slock, flags);
 
data = readl(reg);
data &= ~BIT(offset);
@@ -1971,7 +1971,7 @@ static void rockchip_gpio_set(struct gpio_chip *gc, 
unsigned offset, int value)
data |= BIT(offset);
writel(data, reg);
 
-   spin_unlock_irqrestore(>slock, flags);
+   raw_spin_unlock_irqrestore(>slock, flags);
clk_disable(bank->clk);
 }
 
@@ -2083,7 +2083,7 @@ static void rockchip_irq_demux(struct irq_desc *desc)
 
data = readl_relaxed(bank->reg_base + GPIO_EXT_PORT);
do {
-   spin_lock_irqsave(>slock, flags);
+   raw_spin_lock_irqsave(>slock, flags);
 
polarity = readl_relaxed(bank->reg_base +
 GPIO_INT_POLARITY);
@@ -2094,7 +2094,7 @@ static void rockchip_irq_demux(struct irq_desc *desc)
writel(polarity,
   bank->reg_base + GPIO_INT_POLARITY);
 
-   spin_unlock_irqrestore(>slock, flags);
+   raw_spin_unlock_irqrestore(>slock, flags);
 
data_old = data;
data = readl_relaxed(bank->reg_base +
@@ -2125,20 +2125,20 @@ static int rockchip_irq_set_type(struct irq_data *d, 
unsigned int type)
return ret;
 
clk_enable(bank->clk);
-   spin_lock_irqsave(>slock, flags);
+   raw_spin_lock_irqsave(>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(>slock, flags);
+   raw_spin_unlock_irqrestore(>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(>slock, flags);
+   raw_spin_lock_irqsave(>slock, flags);
irq_gc_lock(gc);
 
level = readl_relaxed(gc->reg_base + GPIO_INTTYPE_LEVEL);
@@ -2181,7 +2181,7 @@ static int rockchip_irq_set_type(struct irq_data *d, 
unsigned int type)
break;
default:
irq_gc_unlock(gc);
-   spin_unlock_irqrestore(>slock, flags);
+   raw_spin_unlock_irqrestore(>slock, flags);

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

2017-03-15 Thread John Keeping 
On Wed, 15 Mar 2017 13:01:37 -0500, Julia Cartwright wrote:
> On Wed, Mar 15, 2017 at 05:46:52PM +, John Keeping 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 
> > Reviewed-by: Heiko Stuebner 
> > Tested-by: Heiko Stuebner 
> > ---
> > v2: unchanged
> > ---
> >  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 128c383ea7ba..8c1cae6d78d7 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_chipgpio_chip;
> > struct pinctrl_gpio_range   grange;
> > -   spinlock_t  slock;
> > +   raw_spinlock_t  slock;
> > u32 toggle_edge_mode;
> >  };
> >  
> > @@ -1295,14 +1295,14 @@ static int rockchip_set_pull(struct 
> > rockchip_pin_bank *bank,
> >  
> > switch (ctrl->type) {
> > case RK2928:
> > -   spin_lock_irqsave(>slock, flags);
> > +   raw_spin_lock_irqsave(>slock, flags);
> >  
> > data = BIT(bit + 16);
> > if (pull == PIN_CONFIG_BIAS_DISABLE)
> > data |= BIT(bit);  
> 
> This should be lifted out from under the lock.
> 
> > ret = regmap_write(regmap, reg, data);  
> 
> How is this legal?  The regmap_write() here is going to end up acquiring
> the regmap mutex.

It's not, the spinlock can be deleted here.  I only have RK3288 hardware
to test and I missed this when checking the uses of slock.


John

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

2017-03-15 Thread John Keeping 
On Wed, 15 Mar 2017 13:01:37 -0500, Julia Cartwright wrote:
> On Wed, Mar 15, 2017 at 05:46:52PM +, John Keeping 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 
> > Reviewed-by: Heiko Stuebner 
> > Tested-by: Heiko Stuebner 
> > ---
> > v2: unchanged
> > ---
> >  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 128c383ea7ba..8c1cae6d78d7 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_chipgpio_chip;
> > struct pinctrl_gpio_range   grange;
> > -   spinlock_t  slock;
> > +   raw_spinlock_t  slock;
> > u32 toggle_edge_mode;
> >  };
> >  
> > @@ -1295,14 +1295,14 @@ static int rockchip_set_pull(struct 
> > rockchip_pin_bank *bank,
> >  
> > switch (ctrl->type) {
> > case RK2928:
> > -   spin_lock_irqsave(>slock, flags);
> > +   raw_spin_lock_irqsave(>slock, flags);
> >  
> > data = BIT(bit + 16);
> > if (pull == PIN_CONFIG_BIAS_DISABLE)
> > data |= BIT(bit);  
> 
> This should be lifted out from under the lock.
> 
> > ret = regmap_write(regmap, reg, data);  
> 
> How is this legal?  The regmap_write() here is going to end up acquiring
> the regmap mutex.

It's not, the spinlock can be deleted here.  I only have RK3288 hardware
to test and I missed this when checking the uses of slock.


John

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

2017-03-15 Thread Julia Cartwright 
Hello John-

On Wed, Mar 15, 2017 at 05:46:52PM +, John Keeping 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 
> Reviewed-by: Heiko Stuebner 
> Tested-by: Heiko Stuebner 
> ---
> v2: unchanged
> ---
>  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 128c383ea7ba..8c1cae6d78d7 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_chipgpio_chip;
>   struct pinctrl_gpio_range   grange;
> - spinlock_t  slock;
> + raw_spinlock_t  slock;
>   u32 toggle_edge_mode;
>  };
>  
> @@ -1295,14 +1295,14 @@ static int rockchip_set_pull(struct rockchip_pin_bank 
> *bank,
>  
>   switch (ctrl->type) {
>   case RK2928:
> - spin_lock_irqsave(>slock, flags);
> + raw_spin_lock_irqsave(>slock, flags);
>  
>   data = BIT(bit + 16);
>   if (pull == PIN_CONFIG_BIAS_DISABLE)
>   data |= BIT(bit);

This should be lifted out from under the lock.

>   ret = regmap_write(regmap, reg, data);

How is this legal?  The regmap_write() here is going to end up acquiring
the regmap mutex.

   Julia


signature.asc
Description: PGP signature

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

2017-03-15 Thread Julia Cartwright 
Hello John-

On Wed, Mar 15, 2017 at 05:46:52PM +, John Keeping 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 
> Reviewed-by: Heiko Stuebner 
> Tested-by: Heiko Stuebner 
> ---
> v2: unchanged
> ---
>  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 128c383ea7ba..8c1cae6d78d7 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_chipgpio_chip;
>   struct pinctrl_gpio_range   grange;
> - spinlock_t  slock;
> + raw_spinlock_t  slock;
>   u32 toggle_edge_mode;
>  };
>  
> @@ -1295,14 +1295,14 @@ static int rockchip_set_pull(struct rockchip_pin_bank 
> *bank,
>  
>   switch (ctrl->type) {
>   case RK2928:
> - spin_lock_irqsave(>slock, flags);
> + raw_spin_lock_irqsave(>slock, flags);
>  
>   data = BIT(bit + 16);
>   if (pull == PIN_CONFIG_BIAS_DISABLE)
>   data |= BIT(bit);

This should be lifted out from under the lock.

>   ret = regmap_write(regmap, reg, data);

How is this legal?  The regmap_write() here is going to end up acquiring
the regmap mutex.

   Julia


signature.asc
Description: PGP signature