On Tue, Dec 01, 2020 at 04:59:21PM +0800, luojiaxing wrote:
On 2020/11/30 19:22, Andy Shevchenko wrote:Hmm, that sounds like a problem, but the explanation is a bit unclear
On Mon, Nov 30, 2020 at 05:36:19PM +0800, Luo Jiaxing wrote:
The mask and unmask registers are not configured in dwapb_irq_enable() andSounds a bit like a papering over the issue which is slightly different.
dwapb_irq_disable(). In the following situations, the IRQ will be masked by
default after the IRQ is enabled:
mask IRQ -> disable IRQ -> enable IRQ
In this case, the IRQ status of GPIO controller is inconsistent with it's
irq_data too. For example, in __irq_enable(), IRQD_IRQ_DISABLED and
IRQD_IRQ_MASKED are both clear, but GPIO controller do not perform unmask.
Can you elaborate more, why ->irq_mask() / ->irq_unmask() are not being called?
Sure, The basic software invoking process is as follows:
Release IRQ:
free_irq() -> __free_irq() -> irq_shutdown() ->__irq_disable()
Disable IRQ:
disable_irq() -> __disable_irq_nosync() -> __disable_irq -> irq_disable ->
__irq_disable()
As shown before, both will call __irq_disable(). The code of it is as
follows:
if (irqd_irq_disabled(&desc->irq_data)) {
if (mask)
mask_irq(desc);
} else {
irq_state_set_disabled(desc);
if (desc->irq_data.chip->irq_disable) {
desc->irq_data.chip->irq_disable(&desc->irq_data);
irq_state_set_masked(desc);
} else if (mask) {
mask_irq(desc);
}
}
Because gpio-dwapb.c provides the hook function of irq_disable,
__irq_disable() will directly calls chip->irq_disable() instead of
mask_irq().
For irq_enable(), it's similar and the code is as follows:
if (!irqd_irq_disabled(&desc->irq_data)) {
unmask_irq(desc);
} else {
irq_state_clr_disabled(desc);
if (desc->irq_data.chip->irq_enable) {
desc->irq_data.chip->irq_enable(&desc->irq_data);
irq_state_clr_masked(desc);
} else {
unmask_irq(desc);
}
}
Similarly, because gpio-dwapb.c provides the hook function of irq_enable,
irq_enable() will directly calls chip->irq_enable() but does not call
unmask_irq().
Therefore, the current handle is as follows:
API of IRQ: | mask_irq() | disable_irq()
| enable_irq()
gpio-dwapb.c: | chip->irq_mask() | chip->irq_diable() |
chip->irq_enable()
I do not know why irq_enable() only calls chip->irq_enable(). However, the
code shows that irq_enable() clears the disable and masked flags in the
irq_data state.
Therefore, for gpio-dwapb.c, I thinks ->irq_enable also needs to clear the
disable and masked flags in the hardware register.
to me. AFAICS you are saying that the only callbacks which are
called during the IRQ request/release are the irq_enable(), right?
If
so then the only reason why we haven't got a problem reported due to
that so far is that the IRQs actually unmasked by default.
In anyway I'd suggest to join someone from the kernel IRQs-related
subsystem to this discussion to ask their opinion whether the IRQs
setup procedure is supposed to work like you say and the irq_enable
shall actually also unmask IRQs.
Thomas, Jason, Mark, could you give us your comment about the issue?
-Sergey
.