| Message ID | 1460026796-10899-2-git-send-email-julien.grall@arm.com |
|---|---|
| State | Superseded |
| Headers | show |
Hi Shannon, Thank you for the review. On 07/04/16 13:30, Shannon Zhao wrote: > > > On 2016/4/7 18:59, Julien Grall wrote: >> The SPCR does not specify if the interrupt is edge or level triggered. >> So the configuration needs to be hardcoded in the code. >> >> Based on the PL011 TRM (see 2.2.8 in ARM DDI 0183G), the interrupt generated >> will be active high. This wording implies the interrupt should be high level >> triggered. > I think active high can stand rising edge triggered for edge triggered > interrupt. > > E.g. see "Table 5-118 Flag Definitions: Virtual Timer, EL2 timers, and > Secure & Non-Secure EL1 timers" in ACPI SPEC 6.0. I've spoken with multiple person about the wording and the consensus is "active high" would imply high level triggered. So it's very ambiguous. However, the PL011 is always using a high level triggered. You can look at the device tree bindings such as the one for the foundation model. Also, the SBSA (section 4.3.2 in ARM-DEN-0029 v2.3) states the PL011 implemented with a level triggered interrupt. Note, I wasn't able to get the serial console working on my platform with edge triggered interrupt. Regards,
On 2016年04月07日 21:41, Julien Grall wrote: > > On 07/04/16 13:30, Shannon Zhao wrote: >> >> >> On 2016/4/7 18:59, Julien Grall wrote: >>> The SPCR does not specify if the interrupt is edge or level triggered. >>> So the configuration needs to be hardcoded in the code. >>> >>> Based on the PL011 TRM (see 2.2.8 in ARM DDI 0183G), the interrupt >>> generated >>> will be active high. This wording implies the interrupt should be >>> high level >>> triggered. >> I think active high can stand rising edge triggered for edge triggered >> interrupt. >> >> E.g. see "Table 5-118 Flag Definitions: Virtual Timer, EL2 timers, and >> Secure & Non-Secure EL1 timers" in ACPI SPEC 6.0. > > I've spoken with multiple person about the wording and the consensus is > "active high" would imply high level triggered. So it's very ambiguous. > > However, the PL011 is always using a high level triggered. You can look > at the device tree bindings such as the one for the foundation model. > > Also, the SBSA (section 4.3.2 in ARM-DEN-0029 v2.3) states the PL011 > implemented with a level triggered interrupt. > > Note, I wasn't able to get the serial console working on my platform > with edge triggered interrupt. So how about IRQ_TYPE_LEVEL_HIGH instead of IRQ_TYPE_LEVEL_MASK? Thanks,
On 07/04/16 14:57, Shannon Zhao wrote: > On 2016年04月07日 21:41, Julien Grall wrote: >> >> On 07/04/16 13:30, Shannon Zhao wrote: >>> >>> >>> On 2016/4/7 18:59, Julien Grall wrote: >>>> The SPCR does not specify if the interrupt is edge or level triggered. >>>> So the configuration needs to be hardcoded in the code. >>>> >>>> Based on the PL011 TRM (see 2.2.8 in ARM DDI 0183G), the interrupt >>>> generated >>>> will be active high. This wording implies the interrupt should be >>>> high level >>>> triggered. >>> I think active high can stand rising edge triggered for edge triggered >>> interrupt. >>> >>> E.g. see "Table 5-118 Flag Definitions: Virtual Timer, EL2 timers, and >>> Secure & Non-Secure EL1 timers" in ACPI SPEC 6.0. >> >> I've spoken with multiple person about the wording and the consensus is >> "active high" would imply high level triggered. So it's very ambiguous. >> >> However, the PL011 is always using a high level triggered. You can look >> at the device tree bindings such as the one for the foundation model. >> >> Also, the SBSA (section 4.3.2 in ARM-DEN-0029 v2.3) states the PL011 >> implemented with a level triggered interrupt. >> >> Note, I wasn't able to get the serial console working on my platform >> with edge triggered interrupt. > > So how about IRQ_TYPE_LEVEL_HIGH instead of IRQ_TYPE_LEVEL_MASK? Good point. I will likely resend only this patch and update the commit message too. Regards,
diff --git a/xen/drivers/char/pl011.c b/xen/drivers/char/pl011.c index fa22edf..88d8488 100644 --- a/xen/drivers/char/pl011.c +++ b/xen/drivers/char/pl011.c @@ -327,7 +327,7 @@ static int __init pl011_acpi_uart_init(const void *data) } /* trigger/polarity information is not available in spcr */ - irq_set_type(spcr->interrupt, IRQ_TYPE_EDGE_BOTH); + irq_set_type(spcr->interrupt, IRQ_TYPE_LEVEL_MASK); res = pl011_uart_init(spcr->interrupt, spcr->serial_port.address, PAGE_SIZE);
The SPCR does not specify if the interrupt is edge or level triggered. So the configuration needs to be hardcoded in the code. Based on the PL011 TRM (see 2.2.8 in ARM DDI 0183G), the interrupt generated will be active high. This wording implies the interrupt should be high level triggered. Note that a rising edge triggered interrupt would be described as "high going edge". Signed-off-by: Julien Grall <julien.grall@arm.com> --- xen/drivers/char/pl011.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)