Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1599684 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [RFC PATCH 0/4] pinctrl: rockchip: PREEMPT_RT_FULL fixes John Keeping <john@metanate.com> - 2017-03-13 19:40 +0100
    Re: [RFC PATCH 0/4] pinctrl: rockchip: PREEMPT_RT_FULL fixes Linus Walleij <linus.walleij@linaro.org> - 2017-03-15 14:20 +0100
      Re: [RFC PATCH 0/4] pinctrl: rockchip: PREEMPT_RT_FULL fixes Julia Cartwright <julia@ni.com> - 2017-03-15 19:30 +0100
    Re: [RFC PATCH 0/4] pinctrl: rockchip: PREEMPT_RT_FULL fixes Heiko Stuebner <heiko@sntech.de> - 2017-03-15 18:20 +0100

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

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

[toc] | [next] | [standalone]

#1601387

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

[toc] | [prev] | [next] | [standalone]

#1601628

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

#1601578

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>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web