On Thu 21 Jun 08:14 PDT 2018, Stephen Boyd wrote: > Quoting Bjorn Andersson (2018-06-19 23:45:09) > > On Mon 18 Jun 13:52 PDT 2018, Stephen Boyd wrote: > > > @@ -647,6 +660,10 @@ static void msm_gpio_irq_unmask(struct irq_data *d) > > > raw_spin_lock_irqsave(&pctrl->lock, flags); > > > > > > val = readl(pctrl->regs + g->intr_cfg_reg); > > > + if (irqd_get_trigger_type(d) & IRQ_TYPE_LEVEL_MASK) { > > > + val |= BIT(g->intr_raw_status_bit); > > > + writel(val, pctrl->regs + g->intr_cfg_reg); > > > + } > > > val |= BIT(g->intr_enable_bit); > > > writel(val, pctrl->regs + g->intr_cfg_reg); > > > > I looked at the TLMM documentation, which states that the status bit > > should be cleared after handling the interrupt and this driver used to > > do this. > > Nice! > > > > > But Timur managed to hit the race where we lost edge triggered > > interrupts with this behavior, so we changed it in the following commit: > > > > a6566710adaa ("pinctrl: qcom: Don't clear status bit on irq_unmask") > > > > > > But the reason that I had this in the driver originally is that msm-3.10 > > does this (clear status bit in unmask), so perhaps the appropriate way > > to solve is to follow the documentation and the downstream driver and > > ack the interrupt in unmask - but do so only for level triggered > > interrupts? > > > > Clearing the status bit (basically acking the gpio irq) can be done in > unmask for level triggered interrupts. That works and as you say it's > even documented. > > I didn't implement that because it felt better to prevent the status > from latching in the hardware while the interrupt is masked. My > understanding of irq mask semantics is that the interrupt shouldn't be > "pending" during the time between mask and unmask and clearing the raw > status allows us to do that properly without messing with the status bit > on the unmask path. It also means that the ack operation really does ack > the irq status bit and cause it to go away. I suppose there is one case > where I'm wrong though, and that is when the irq is unmasked on irq > startup where we don't want to see a spurious latched level interrupt > that occurred before we booted. > I took another pass through the irq code and I agree, while the late ack does solve the problem it's more intuitive if we can prevent the latching. > That problem may be possible with bad bootloaders that are leaving some > status bit latched in there, but also we would want to fix that for edge > type interrupts too, so we would need to clear the status bit regardless > of the level on irq startup and hope an edge isn't lost on startup. > I would prefer that we handle that case explicitly. There are some concerns from msm-3.10 regarding writing the intr-raw bit at the same time as the other bits, but I think that relates to the fact that the downstream driver used to configure and enabled raw-state in the same write. So please respin v2 as you planned. Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html