| Message ID | 521B7A70.300@linaro.org |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Mon, 2013-08-26 at 16:55 +0100, Julien Grall wrote: > On 08/22/2013 08:23 AM, Ian Campbell wrote: > > On Wed, 2013-08-21 at 17:11 +0100, Julien Grall wrote: > >> On 08/13/2013 04:12 PM, Andre Przywara wrote: > >>> The PL011 IMSC register description is somehow fuzzy in the > >>> documentation; by comparing it with the Linux implementation one can > >>> see that the logic is actually reversed to Xen's implementation: > >>> A "0" in field means interrupt disabled, a "1" enables it. > >>> Therefore we enabled all interrupts instead of disabling them in the > >>> beginning and later on masked the wrong interrupts. > >>> Unclear how this worked on the Versatile Express, but this fix is > >>> needed to get Calxeda Midway running (and works on VExpress, too). > >> > >> On my Versatile Express, the keyboard is unusable with this patch. > >> Xen receives random keys and sometimes nothing is printed on the serial > >> port. > >> > >> By reverting this patch on my tree, I'm able to use correctly the serial > >> port. > > > > :-/ Andre did say this patch worked on vexpress for him. > > > > I'm pretty certain Andre's patch is correct in its own right. But the > > fact that it worked before does seem to imply that there are other > > issues with the pl011 driver, it's likely that this change has just > > exposed a latent one. > > > > Could this be related somehow to the baud rate setting change? I > > wouldn't have expected so but "random keys" and nothingness could be a > > symptom of incorrect baud too. > > > > Does anyone have time to look into this? > > > > If RTI interrupt are enabled (see small patch below), the UART works perfectly > on the versatile express. > > The PL011 documentation says the bit is used to mask/unmask receive interrupt > timeout. I don't understand why this interrupt is useful and the documentation > doesn't help me... Docs say: The receive timeout interrupt is asserted when the receive FIFO is not empty, and no more data is received during a 32-bit period. Perhaps at during boot we are not processing the interrupt fast enough (or at all) and this condition triggers? It's not clear to me if not handling this condition will block further operations or not. Is it possible that the vexpress firmware is enabling DMA RX? Perhaps draining the rx buffer in e.g. pl011_init_preirq would also help? Or perhaps clearing the RTI in postirq (where we also clear any pending errors) would be a good idea? Also in the footnote to table 3-15, which relates to this interrupt: "In this case the raw interrupt cannot be set unless the mask is set, this is because the mask acts as an enable for power saving. That is, the same status can be read from UARTMIS and UARTRIS for the receive timeout interrupt." Which makes me wonder if by not handling it we are causing the UART to hit some sort of low power state from which we never wake? Do you see this type of interrupt exactly once or do you keep seeing them? > > ==================================================================================== > diff --git a/xen/drivers/char/pl011.c b/xen/drivers/char/pl011.c > index 0e1eb64..e4bd702 100644 > --- a/xen/drivers/char/pl011.c > +++ b/xen/drivers/char/pl011.c > @@ -140,7 +140,7 @@ static void __init pl011_init_postirq(struct serial_port *port) > pl011_write(uart, ICR, OEI|BEI|PEI|FEI); > > /* Unmask interrupts */ > - pl011_write(uart, IMSC, OEI|BEI|PEI|FEI|TXI|RXI); > + pl011_write(uart, IMSC, RTI|OEI|BEI|PEI|FEI|TXI|RXI); > } > > static void pl011_suspend(struct serial_port *port) > ====================================================================================== >
On Tue, 2013-08-27 at 11:37 +0100, Ian Campbell wrote: > On Mon, 2013-08-26 at 16:55 +0100, Julien Grall wrote: > > On 08/22/2013 08:23 AM, Ian Campbell wrote: > > > On Wed, 2013-08-21 at 17:11 +0100, Julien Grall wrote: > > >> On 08/13/2013 04:12 PM, Andre Przywara wrote: > > >>> The PL011 IMSC register description is somehow fuzzy in the > > >>> documentation; by comparing it with the Linux implementation one can > > >>> see that the logic is actually reversed to Xen's implementation: > > >>> A "0" in field means interrupt disabled, a "1" enables it. > > >>> Therefore we enabled all interrupts instead of disabling them in the > > >>> beginning and later on masked the wrong interrupts. > > >>> Unclear how this worked on the Versatile Express, but this fix is > > >>> needed to get Calxeda Midway running (and works on VExpress, too). > > >> > > >> On my Versatile Express, the keyboard is unusable with this patch. > > >> Xen receives random keys and sometimes nothing is printed on the serial > > >> port. > > >> > > >> By reverting this patch on my tree, I'm able to use correctly the serial > > >> port. > > > > > > :-/ Andre did say this patch worked on vexpress for him. > > > > > > I'm pretty certain Andre's patch is correct in its own right. But the > > > fact that it worked before does seem to imply that there are other > > > issues with the pl011 driver, it's likely that this change has just > > > exposed a latent one. > > > > > > Could this be related somehow to the baud rate setting change? I > > > wouldn't have expected so but "random keys" and nothingness could be a > > > symptom of incorrect baud too. > > > > > > Does anyone have time to look into this? > > > > > > > If RTI interrupt are enabled (see small patch below), the UART works perfectly > > on the versatile express. > > > > The PL011 documentation says the bit is used to mask/unmask receive interrupt > > timeout. I don't understand why this interrupt is useful and the documentation > > doesn't help me... > > Docs say: > The receive timeout interrupt is asserted when the receive FIFO > is not empty, and no more data is received during a 32-bit > period. I see now that we do actually enable the FIFOs, so this functionality makes sense -- if your RX FIFO trigger level is e.g. 1/2 and the FIFO is 1/4 full when the sender stops then this interrupt provides a backstop such that you will receive those bytes without waiting for the sender to resume (which might be a long time). So I think enabling and handling these timeout interrupts makes sense. I cannot explain why it only affects vexpress though. Ian.
On 08/27/2013 11:45 AM, Ian Campbell wrote: > On Tue, 2013-08-27 at 11:37 +0100, Ian Campbell wrote: >> On Mon, 2013-08-26 at 16:55 +0100, Julien Grall wrote: >>> On 08/22/2013 08:23 AM, Ian Campbell wrote: >>>> On Wed, 2013-08-21 at 17:11 +0100, Julien Grall wrote: >>>>> On 08/13/2013 04:12 PM, Andre Przywara wrote: >>>>>> The PL011 IMSC register description is somehow fuzzy in the >>>>>> documentation; by comparing it with the Linux implementation one can >>>>>> see that the logic is actually reversed to Xen's implementation: >>>>>> A "0" in field means interrupt disabled, a "1" enables it. >>>>>> Therefore we enabled all interrupts instead of disabling them in the >>>>>> beginning and later on masked the wrong interrupts. >>>>>> Unclear how this worked on the Versatile Express, but this fix is >>>>>> needed to get Calxeda Midway running (and works on VExpress, too). >>>>> >>>>> On my Versatile Express, the keyboard is unusable with this patch. >>>>> Xen receives random keys and sometimes nothing is printed on the serial >>>>> port. >>>>> >>>>> By reverting this patch on my tree, I'm able to use correctly the serial >>>>> port. >>>> >>>> :-/ Andre did say this patch worked on vexpress for him. >>>> >>>> I'm pretty certain Andre's patch is correct in its own right. But the >>>> fact that it worked before does seem to imply that there are other >>>> issues with the pl011 driver, it's likely that this change has just >>>> exposed a latent one. >>>> >>>> Could this be related somehow to the baud rate setting change? I >>>> wouldn't have expected so but "random keys" and nothingness could be a >>>> symptom of incorrect baud too. >>>> >>>> Does anyone have time to look into this? >>>> >>> >>> If RTI interrupt are enabled (see small patch below), the UART works perfectly >>> on the versatile express. >>> >>> The PL011 documentation says the bit is used to mask/unmask receive interrupt >>> timeout. I don't understand why this interrupt is useful and the documentation >>> doesn't help me... >> >> Docs say: >> The receive timeout interrupt is asserted when the receive FIFO >> is not empty, and no more data is received during a 32-bit >> period. > > I see now that we do actually enable the FIFOs, so this functionality > makes sense -- if your RX FIFO trigger level is e.g. 1/2 and the FIFO is > 1/4 full when the sender stops then this interrupt provides a backstop > such that you will receive those bytes without waiting for the sender to > resume (which might be a long time). > > So I think enabling and handling these timeout interrupts makes sense. I > cannot explain why it only affects vexpress though. I only see the issue with the serial input. I guess this is needed on midway but we don't see anything because the serial console doesn't receive input. I will send a correct patch + commit message this afternoon.
On Tue, 2013-08-27 at 11:58 +0100, Julien Grall wrote: > On 08/27/2013 11:45 AM, Ian Campbell wrote: > > On Tue, 2013-08-27 at 11:37 +0100, Ian Campbell wrote: > >> On Mon, 2013-08-26 at 16:55 +0100, Julien Grall wrote: > >>> On 08/22/2013 08:23 AM, Ian Campbell wrote: > >>>> On Wed, 2013-08-21 at 17:11 +0100, Julien Grall wrote: > >>>>> On 08/13/2013 04:12 PM, Andre Przywara wrote: > >>>>>> The PL011 IMSC register description is somehow fuzzy in the > >>>>>> documentation; by comparing it with the Linux implementation one can > >>>>>> see that the logic is actually reversed to Xen's implementation: > >>>>>> A "0" in field means interrupt disabled, a "1" enables it. > >>>>>> Therefore we enabled all interrupts instead of disabling them in the > >>>>>> beginning and later on masked the wrong interrupts. > >>>>>> Unclear how this worked on the Versatile Express, but this fix is > >>>>>> needed to get Calxeda Midway running (and works on VExpress, too). > >>>>> > >>>>> On my Versatile Express, the keyboard is unusable with this patch. > >>>>> Xen receives random keys and sometimes nothing is printed on the serial > >>>>> port. > >>>>> > >>>>> By reverting this patch on my tree, I'm able to use correctly the serial > >>>>> port. > >>>> > >>>> :-/ Andre did say this patch worked on vexpress for him. > >>>> > >>>> I'm pretty certain Andre's patch is correct in its own right. But the > >>>> fact that it worked before does seem to imply that there are other > >>>> issues with the pl011 driver, it's likely that this change has just > >>>> exposed a latent one. > >>>> > >>>> Could this be related somehow to the baud rate setting change? I > >>>> wouldn't have expected so but "random keys" and nothingness could be a > >>>> symptom of incorrect baud too. > >>>> > >>>> Does anyone have time to look into this? > >>>> > >>> > >>> If RTI interrupt are enabled (see small patch below), the UART works perfectly > >>> on the versatile express. > >>> > >>> The PL011 documentation says the bit is used to mask/unmask receive interrupt > >>> timeout. I don't understand why this interrupt is useful and the documentation > >>> doesn't help me... > >> > >> Docs say: > >> The receive timeout interrupt is asserted when the receive FIFO > >> is not empty, and no more data is received during a 32-bit > >> period. > > > > I see now that we do actually enable the FIFOs, so this functionality > > makes sense -- if your RX FIFO trigger level is e.g. 1/2 and the FIFO is > > 1/4 full when the sender stops then this interrupt provides a backstop > > such that you will receive those bytes without waiting for the sender to > > resume (which might be a long time). > > > > So I think enabling and handling these timeout interrupts makes sense. I > > cannot explain why it only affects vexpress though. > > I only see the issue with the serial input. I guess this is needed on > midway but we don't see anything because the serial console doesn't > receive input. The IPMI console can take input too though and is a pl011 from the main processor's PoV > I will send a correct patch + commit message this afternoon. THanks! >
==================================================================================== diff --git a/xen/drivers/char/pl011.c b/xen/drivers/char/pl011.c index 0e1eb64..e4bd702 100644 --- a/xen/drivers/char/pl011.c +++ b/xen/drivers/char/pl011.c @@ -140,7 +140,7 @@ static void __init pl011_init_postirq(struct serial_port *port) pl011_write(uart, ICR, OEI|BEI|PEI|FEI); /* Unmask interrupts */ - pl011_write(uart, IMSC, OEI|BEI|PEI|FEI|TXI|RXI); + pl011_write(uart, IMSC, RTI|OEI|BEI|PEI|FEI|TXI|RXI); } static void pl011_suspend(struct serial_port *port)