On 2020/12/6 6:15, Serge Semin wrote:
On Tue, Dec 01, 2020 at 04:59:21PM +0800, luojiaxing wrote:
On 2020/11/30 19:22, Andy Shevchenko wrote:
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() and
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.
Sounds a bit like a papering over the issue which is slightly different.
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.
Hmm, that sounds like a problem, but the explanation is a bit unclear
to me. AFAICS you are saying that the only callbacks which are
called during the IRQ request/release are the irq_enable(), right?
Yes, but one point needs to be clarified, for IRQ requests, it calls
irq_enable(); for IRQ release, it calls irq_disable().
Actually I am thinking that why only irq_enable()/irq_disable() is
called since the mask and enable flags of irq_data are both set.
Does IRQ subsystem expect irq_enable to set both mask and enable? If we
didn't do that, the state machine of the software is different from
hardware, at least for mask bit.
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.
yes, I think so, Common drivers do not mask the IRQ before releasing it.
But that's possible.
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
.
