Xen devel - Aug 2013 - [PATCH] PL011: fix reverse logic for interrupt mask register

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).

Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
---
 xen/drivers/char/pl011.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/xen/drivers/char/pl011.c b/xen/drivers/char/pl011.c
index 05d034f..8e90520 100644
--- a/xen/drivers/char/pl011.c
+++ b/xen/drivers/char/pl011.c
@@ -87,7 +87,7 @@ static void __init pl011_init_preirq(struct serial_port *port)
     unsigned int divisor;
 
     /* No interrupts, please. */
-    pl011_write(uart, IMSC, ALLI);
+    pl011_write(uart, IMSC, 0);
 
     /* Definitely no DMA */
     pl011_write(uart, DMACR, 0x0);
@@ -115,7 +115,7 @@ static void __init pl011_init_preirq(struct serial_port
*port)
     pl011_write(uart, RSR, 0);
 
     /* Mask and clear the interrupts */
-    pl011_write(uart, IMSC, ALLI);
+    pl011_write(uart, IMSC, 0);
     pl011_write(uart, ICR, ALLI);
 
     /* Enable the UART for RX and TX; no flow ctrl */
@@ -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, RTI|DSRMI|DCDMI|CTSMI|RIMI);
+    pl011_write(uart, IMSC, OEI|BEI|PEI|FEI|TXI|RXI);
 }
 
 static void pl011_suspend(struct serial_port *port)
-- 
1.7.12.1

On 08/13/2013 05:12 PM, Andre Przywara wrote:

Ian,

what about this one?
This is a real showstopper on Midway.

Thanks,
Andre.
> 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).
>
> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
> ---
>   xen/drivers/char/pl011.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/xen/drivers/char/pl011.c b/xen/drivers/char/pl011.c
> index 05d034f..8e90520 100644
> --- a/xen/drivers/char/pl011.c
> +++ b/xen/drivers/char/pl011.c
> @@ -87,7 +87,7 @@ static void __init pl011_init_preirq(struct serial_port
*port)
>       unsigned int divisor;
>
>       /* No interrupts, please. */
> -    pl011_write(uart, IMSC, ALLI);
> +    pl011_write(uart, IMSC, 0);
>
>       /* Definitely no DMA */
>       pl011_write(uart, DMACR, 0x0);
> @@ -115,7 +115,7 @@ static void __init pl011_init_preirq(struct serial_port
*port)
>       pl011_write(uart, RSR, 0);
>
>       /* Mask and clear the interrupts */
> -    pl011_write(uart, IMSC, ALLI);
> +    pl011_write(uart, IMSC, 0);
>       pl011_write(uart, ICR, ALLI);
>
>       /* Enable the UART for RX and TX; no flow ctrl */
> @@ -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, RTI|DSRMI|DCDMI|CTSMI|RIMI);
> +    pl011_write(uart, IMSC, OEI|BEI|PEI|FEI|TXI|RXI);
>   }
>
>   static void pl011_suspend(struct serial_port *port)
>

Sorry, I''ve gotten behind while traveling. I cleared a bunch of backlog
this afternoon but didn''t manage to get to this one. I hope I can
review it tomorrow.

Ian. (Sorry for top post, on my phone)

Andre Przywara <andre.przywara@linaro.org> wrote:
>On 08/13/2013 05:12 PM, Andre Przywara wrote:
>
>Ian,
>
>what about this one?
>This is a real showstopper on Midway.
>
>Thanks,
>Andre.
>
>> 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).
>>
>> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
>> ---
>>   xen/drivers/char/pl011.c | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/xen/drivers/char/pl011.c b/xen/drivers/char/pl011.c
>> index 05d034f..8e90520 100644
>> --- a/xen/drivers/char/pl011.c
>> +++ b/xen/drivers/char/pl011.c
>> @@ -87,7 +87,7 @@ static void __init pl011_init_preirq(struct
serial_port *port)
>>       unsigned int divisor;
>>
>>       /* No interrupts, please. */
>> -    pl011_write(uart, IMSC, ALLI);
>> +    pl011_write(uart, IMSC, 0);
>>
>>       /* Definitely no DMA */
>>       pl011_write(uart, DMACR, 0x0);
>> @@ -115,7 +115,7 @@ static void __init pl011_init_preirq(struct
serial_port *port)
>>       pl011_write(uart, RSR, 0);
>>
>>       /* Mask and clear the interrupts */
>> -    pl011_write(uart, IMSC, ALLI);
>> +    pl011_write(uart, IMSC, 0);
>>       pl011_write(uart, ICR, ALLI);
>>
>>       /* Enable the UART for RX and TX; no flow ctrl */
>> @@ -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, RTI|DSRMI|DCDMI|CTSMI|RIMI);
>> +    pl011_write(uart, IMSC, OEI|BEI|PEI|FEI|TXI|RXI);
>>   }
>>
>>   static void pl011_suspend(struct serial_port *port)
>>
>

On Tue, 2013-08-13 at 17:12 +0200, Andre Przywara wrote:
> The PL011 IMSC register description is somehow fuzzy in the
> documentation;
OMG pl011 docs r1p5 issue g says:
        On a read this register returns the current value of the mask on
        the relevant interrupt. On a write of 1 to the particular bit,
        it sets the corresponding mask of that interrupt. A write of 0
        clears the corresponding mask.
        
Which is perfectly obvious if you read it understanding "mask an
interrupt" in the normal way, but in the introduction to the section on
interrupts it says:

        You can enable or disable the individual interrupts by changing
        the mask bits in the Interrupt Mask Set/Clear Register, UARTIMSC
        on page 3-17. Setting the appropriate mask bit HIGH enables the
        interrupt.
        
So IMSC is actually the "mask" of the interrupts which are enabled.

What crappy docs!
>  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).
How on earth we got away with this I''ve idea!
> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
Ack + applied, thanks!

On 08/21/2013 12:04 PM, Ian Campbell wrote:
> On Tue, 2013-08-13 at 17:12 +0200, Andre Przywara wrote:
>> The PL011 IMSC register description is somehow fuzzy in the
>> documentation;
>
> OMG pl011 docs r1p5 issue g says:
>          On a read this register returns the current value of the mask on
>          the relevant interrupt. On a write of 1 to the particular bit,
>          it sets the corresponding mask of that interrupt. A write of 0
>          clears the corresponding mask.
>
> Which is perfectly obvious if you read it understanding "mask an
> interrupt" in the normal way, but in the introduction to the section
on
> interrupts it says:
>
>          You can enable or disable the individual interrupts by changing
>          the mask bits in the Interrupt Mask Set/Clear Register, UARTIMSC
>          on page 3-17. Setting the appropriate mask bit HIGH enables the
>          interrupt.
>
> So IMSC is actually the "mask" of the interrupts which are
enabled.
>
> What crappy docs!
I agree, I stumbled on this by reading IMSC and wondered how it could be 
just 0 in the first place, then looking at the Linux code...
And honestly I didn''t even find the first paragraph, but was wondering 
why they repeat that "1 sets the mask, 0 clears it" all over the place
instead of explicitly stating the meaning for the interrupt ;-)

Regards,
Andre.
>>   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).
>
> How on earth we got away with this I''ve idea!
>
>> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
>
> Ack + applied, thanks!
>
>

On Wed, 2013-08-21 at 15:58 +0200, Andre Przywara wrote:
> On 08/21/2013 12:04 PM, Ian Campbell wrote:
> > On Tue, 2013-08-13 at 17:12 +0200, Andre Przywara wrote:
> >> The PL011 IMSC register description is somehow fuzzy in the
> >> documentation;
> >
> > OMG pl011 docs r1p5 issue g says:
> >          On a read this register returns the current value of the mask
on
> >          the relevant interrupt. On a write of 1 to the particular
bit,
> >          it sets the corresponding mask of that interrupt. A write of
0
> >          clears the corresponding mask.
> >
> > Which is perfectly obvious if you read it understanding "mask an
> > interrupt" in the normal way, but in the introduction to the
section on
> > interrupts it says:
> >
> >          You can enable or disable the individual interrupts by
changing
> >          the mask bits in the Interrupt Mask Set/Clear Register,
UARTIMSC
> >          on page 3-17. Setting the appropriate mask bit HIGH enables
the
> >          interrupt.
> >
> > So IMSC is actually the "mask" of the interrupts which are
enabled.
> >
> > What crappy docs!
> 
> I agree, I stumbled on this by reading IMSC and wondered how it could be 
> just 0 in the first place,
I wondered that too, but dismissed it as "mad hardware" without
thinking
carefully enough!

Ian.

On Wed, 2013-08-21 at 15:58 +0200, Andre Przywara wrote:
> I stumbled on this by reading IMSC and wondered how it could be 
> just 0 in the first place,
I wondered that too, but dismissed it as "mad hardware" without
thinking
carefully enough!

Ian.

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.
> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
> ---
>  xen/drivers/char/pl011.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/drivers/char/pl011.c b/xen/drivers/char/pl011.c
> index 05d034f..8e90520 100644
> --- a/xen/drivers/char/pl011.c
> +++ b/xen/drivers/char/pl011.c
> @@ -87,7 +87,7 @@ static void __init pl011_init_preirq(struct serial_port
*port)
>      unsigned int divisor;
>  
>      /* No interrupts, please. */
> -    pl011_write(uart, IMSC, ALLI);
> +    pl011_write(uart, IMSC, 0);
>  
>      /* Definitely no DMA */
>      pl011_write(uart, DMACR, 0x0);
> @@ -115,7 +115,7 @@ static void __init pl011_init_preirq(struct serial_port
*port)
>      pl011_write(uart, RSR, 0);
>  
>      /* Mask and clear the interrupts */
> -    pl011_write(uart, IMSC, ALLI);
> +    pl011_write(uart, IMSC, 0);
>      pl011_write(uart, ICR, ALLI);
>  
>      /* Enable the UART for RX and TX; no flow ctrl */
> @@ -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, RTI|DSRMI|DCDMI|CTSMI|RIMI);
> +    pl011_write(uart, IMSC, OEI|BEI|PEI|FEI|TXI|RXI);
>  }
>  
>  static void pl011_suspend(struct serial_port *port)
> 

-- 
Julien Grall

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?

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...

===================================================================================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)
=====================================================================================
-- 
Julien Grall

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.

-- 
Julien Grall

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!
>