| Message ID | 1438595198-44230-1-git-send-email-mika.westerberg@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Aug 3, 2015 at 11:46 AM, Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > There is a hardware issue in Intel Braswell/Cherryview where concurrent > GPIO register access might results reads of 0xffffffff and writes might get > dropped. > > Prevent this from happening by taking the serializing lock for all places > where it is possible that more than one thread might be accessing the > hardware concurrently. > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> Patch applied. When we are on the topic of this spinlock, I noticed that you take it in some IRQ functions: static void chv_gpio_irq_ack(struct irq_data *d) { struct gpio_chip *gc = irq_data_get_irq_chip_data(d); struct chv_pinctrl *pctrl = gpiochip_to_pinctrl(gc); int pin = chv_gpio_offset_to_pin(pctrl, irqd_to_hwirq(d)); u32 intr_line; spin_lock(&pctrl->lock); (...) The Realtime tree have recently started to push raw spinlocks into GPIO drivers that handle IRQs. Can you look into this for the Intel drivers, I think maybe this needs to be a raw spinlock to play nicely with realtime. Cf: http://marc.info/?l=linux-gpio&m=143749603401220&w=2 Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Aug 13, 2015 at 01:24:13PM +0200, Linus Walleij wrote: > On Mon, Aug 3, 2015 at 11:46 AM, Mika Westerberg > <mika.westerberg@linux.intel.com> wrote: > > > There is a hardware issue in Intel Braswell/Cherryview where concurrent > > GPIO register access might results reads of 0xffffffff and writes might get > > dropped. > > > > Prevent this from happening by taking the serializing lock for all places > > where it is possible that more than one thread might be accessing the > > hardware concurrently. > > > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> > > Patch applied. Thanks! > When we are on the topic of this spinlock, I noticed that you > take it in some IRQ functions: > > static void chv_gpio_irq_ack(struct irq_data *d) > { > struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > struct chv_pinctrl *pctrl = gpiochip_to_pinctrl(gc); > int pin = chv_gpio_offset_to_pin(pctrl, irqd_to_hwirq(d)); > u32 intr_line; > > spin_lock(&pctrl->lock); > (...) > > The Realtime tree have recently started to push raw spinlocks > into GPIO drivers that handle IRQs. Can you look into this > for the Intel drivers, I think maybe this needs to be a raw > spinlock to play nicely with realtime. > > Cf: > http://marc.info/?l=linux-gpio&m=143749603401220&w=2 Sure, I'll look into that. -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/pinctrl/intel/pinctrl-cherryview.c b/drivers/pinctrl/intel/pinctrl-cherryview.c index 3f737daa3fd2..65cfd4c0024f 100644 --- a/drivers/pinctrl/intel/pinctrl-cherryview.c +++ b/drivers/pinctrl/intel/pinctrl-cherryview.c @@ -1169,9 +1169,12 @@ static int chv_gpio_get(struct gpio_chip *chip, unsigned offset) { struct chv_pinctrl *pctrl = gpiochip_to_pinctrl(chip); int pin = chv_gpio_offset_to_pin(pctrl, offset); + unsigned long flags; u32 ctrl0, cfg; + spin_lock_irqsave(&pctrl->lock, flags); ctrl0 = readl(chv_padreg(pctrl, pin, CHV_PADCTRL0)); + spin_unlock_irqrestore(&pctrl->lock, flags); cfg = ctrl0 & CHV_PADCTRL0_GPIOCFG_MASK; cfg >>= CHV_PADCTRL0_GPIOCFG_SHIFT; @@ -1209,8 +1212,11 @@ static int chv_gpio_get_direction(struct gpio_chip *chip, unsigned offset) struct chv_pinctrl *pctrl = gpiochip_to_pinctrl(chip); unsigned pin = chv_gpio_offset_to_pin(pctrl, offset); u32 ctrl0, direction; + unsigned long flags; + spin_lock_irqsave(&pctrl->lock, flags); ctrl0 = readl(chv_padreg(pctrl, pin, CHV_PADCTRL0)); + spin_unlock_irqrestore(&pctrl->lock, flags); direction = ctrl0 & CHV_PADCTRL0_GPIOCFG_MASK; direction >>= CHV_PADCTRL0_GPIOCFG_SHIFT; @@ -1313,6 +1319,7 @@ static unsigned chv_gpio_irq_startup(struct irq_data *d) unsigned long flags; u32 intsel, value; + spin_lock_irqsave(&pctrl->lock, flags); intsel = readl(chv_padreg(pctrl, pin, CHV_PADCTRL0)); intsel &= CHV_PADCTRL0_INTSEL_MASK; intsel >>= CHV_PADCTRL0_INTSEL_SHIFT; @@ -1323,7 +1330,6 @@ static unsigned chv_gpio_irq_startup(struct irq_data *d) else handler = handle_edge_irq; - spin_lock_irqsave(&pctrl->lock, flags); if (!pctrl->intr_lines[intsel]) { __irq_set_handler_locked(d->irq, handler); pctrl->intr_lines[intsel] = offset;
There is a hardware issue in Intel Braswell/Cherryview where concurrent GPIO register access might results reads of 0xffffffff and writes might get dropped. Prevent this from happening by taking the serializing lock for all places where it is possible that more than one thread might be accessing the hardware concurrently. Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> --- drivers/pinctrl/intel/pinctrl-cherryview.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)