Re: [Xen-devel] [PATCH 26/27 v8] xen/arm: vpl011: Correct the logic for asserting/de-asserting SBSA UART TX interrupt

[Bhupinder Thakur <bhupinder.thakur@xxxxxxxxxx>]

Hi Andre,


>> diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
>> index 56d9cbe..1e72fca 100644
>> --- a/xen/arch/arm/vpl011.c
>> +++ b/xen/arch/arm/vpl011.c
>> @@ -152,12 +152,20 @@ static void vpl011_write_data(struct domain *d, 
>> uint8_t data)
>>      else
>>          gprintk(XENLOG_ERR, "vpl011: Unexpected OUT ring buffer full\n");
>>
>> -    if ( xencons_queued(out_prod, out_cons, sizeof(intf->out)) ==
>> -         sizeof (intf->out) )
>> -    {
>> -        vpl011->uartfr |= TXFF;
>> +    /*
>> +     * Ensure that there is space for atleast 16 bytes before asserting the
>> +     * TXI interrupt status bit because the SBSA UART driver may write
>> +     * 16 bytes (i.e. half the SBSA UART fifo size of 32) on getting
>> +     * a TX interrupt.
>> +     */
>> +    if ( xencons_queued(out_prod, out_cons, sizeof(intf->out)) <=
>> +         (sizeof (intf->out) - 16) )
>> +        vpl011->uartris |= TXI;
>> +    else if ( xencons_queued(out_prod, out_cons, sizeof(intf->out)) !=
>> +              sizeof (intf->out) )
>
> Now this is really hard to read. Can't you use:
>
>     fifo_level = xencons_queued(out_prod, out_cons, sizeof(intf->out));

ok.

>
> Also I think you could start the patch a few lines above, where you
> check for any free space in the buffer.

ok. I will move the logic inside the case when there is space in the buffer.

>
>>          vpl011->uartris &= ~TXI;
>> -    }
>> +    else
>> +        vpl011->uartfr |= TXFF;
>
> And I believe we should separate the FIFO full condition from the
> interrupt condition. I think it should more look like:
>
>     vpl011->uartfr |= BUSY;
>     vpl011->uartfr &= ~TXFE;
>
>     if ( fifo_level == sizeof(intf->out) )
>         vpl011->uartfr |= TXFF;
>
>     if ( fifo_level >= sizeof(intf->out) - 16 )
>         vpl011->uartris &= ~TXI;
>
> Which is much easier to read and understand, also follows the spec
> closely. The "16" should be either expressed at FIFOSIZE / 2 or
> explained in a comment.
ok.

>
>>
>>      vpl011->uartfr |= BUSY;
>>
>> @@ -368,7 +376,16 @@ static void vpl011_data_avail(struct domain *d)
>>      if ( out_ring_qsize != sizeof(intf->out) )
>>      {
>>          vpl011->uartfr &= ~TXFF;
>> -        vpl011->uartris |= TXI;
>> +
>> +        /*
>> +         * Ensure that there is space for atleast 16 bytes before asserting 
>> the
>> +         * TXI interrupt status bit because the SBSA UART driver may write 
>> upto
>> +         * 16 bytes (i.e. half the SBSA UART fifo size of 32) on getting
>> +         * a TX interrupt.
>
> The comment sounds a bit like this is hack, where it actually is a
> totally legit spec requirement (the interrupt is asserted/deasserted
> around the *trigger level*, which is half way by default and always half
> for the SBSA).
ok. I will modify the comment to mention that TX interrupt is
asserted/de-aserted based
on the trigger level which is fifo_size/2.

>
> Also I think the same logic/fix needs to be applied to the receiving side.
>
That may delay the processing of incoming data. To verify that I
changed the code to assert the RX interrupt only
when the RX FIFO becomes half full. With that change, the input data
is delayed as the SBSA UART driver starts
processing the incoming data only when the RX interrupt is asserted.

> And while I see that Julien requested a follow-up patch, I believe this
> should eventually be squashed into 02/27, to not have wrong code in the
> repo. But can could be done at commit time, I guess.
>
ok.

Regards,
Bhupinder

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel