Re: [Xen-devel] [PATCH RFC] ARM: vPL011: use receive timeout interrupt

[Julien Grall <julien.grall@xxxxxxxxxx>]

Hi,

On 23/10/2017 17:01, Andre Przywara wrote:
> Hi,
>
> On 18/10/17 17:32, Bhupinder Thakur wrote:
> > Hi Andre,
> >
> > I verified this patch on qualcomm platform. It is working fine.
> >
> > On 18 October 2017 at 19:11, Andre Przywara <andre.przywara@xxxxxxx> wrote:
> > > Instead of asserting the receive interrupt (RXI) on the first character
> > > in the FIFO, lets (ab)use the receive timeout interrupt (RTI) for that
> > > purpose. That seems to be closer to the spec and what hardware does.
> > > Improve the readability of vpl011_data_avail() on the way.
> > >
> > > Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
> > > ---
> > > Hi,
> > >
> > > this one is the approach I mentioned in the email earlier today.
> > > It goes on top of Bhupinders v12 27/27, but should eventually be merged
> > > into this one once we agreed on the subject. I just carved it out here
> > > for clarity to make it clearer what has been changed.
> > > Would be good if someone could test it.
> > >
> > > Cheers,
> > > Andre.
> > >  xen/arch/arm/vpl011.c | 61 ++++++++++++++++++++++++---------------------------
> > >  1 file changed, 29 insertions(+), 32 deletions(-)
> > >
> > > diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
> > > index adf1711571..ae18bddd81 100644
> > > --- a/xen/arch/arm/vpl011.c
> > > +++ b/xen/arch/arm/vpl011.c
> > > @@ -105,9 +105,13 @@ static uint8_t vpl011_read_data(struct domain *d)
> > >          if ( fifo_level == 0 )
> > >          {
> > >              vpl011->uartfr |= RXFE;
> > > -            vpl011->uartris &= ~RXI;
> > > -            vpl011_update_interrupt_status(d);
> > > +            vpl011->uartris &= ~RTI;
> > >          }
> > > +
> > > +        if ( fifo_level < sizeof(intf->in) - SBSA_UART_FIFO_SIZE / 2 )
> > > +            vpl011->uartris &= ~RXI;
> > > +
> > > +        vpl011_update_interrupt_status(d);
> > I think we check if ( fifo_level < SBSA_UART_FIFO_SIZE / 2 ) which
> > should be a valid condition to clear the RX interrupt.
> Are you sure? My understanding is that the semantics of the return value
> of xencons_queued() differs between intf and outf:
> - For intf, Xen fills that buffer with incoming characters. The
> watermark is assumed to be (FIFO / 2), which translates into 16
> characters. Now for the SBSA vUART RX side that means: "Assert the RX
> interrupt if there is only room for 16 (or less) characters in the FIFO
> (read: intf buffer in our case). Since we (ab)use the Xen buffer for the
> FIFO, this means we warn if the number of queued characters exceeds
> (buffersize - 16).
> - For outf, the UART emulation fills the buffer. The SBSA vUART TX side
> demands that the TX interrupt is asserted if the fill level of the
> transmit FIFO is less than or equal to the 16 characters, which means:
> number of queued characters is less than 16.
>
> I think the key point is that our trigger level isn't symmetrical here,
> since we have to emulate the architected 32-byte FIFO semantics for the
> driver, but have a (secretly) much larger "FIFO" internally.
>
> Do you agree with this reasoning and do I have a thinko here? Could well
> be I am seriously misguided here.
xencons_queued calculates how many bytes are currently on the ring. So I 
think your description makes sense.
With (fifo_level < (SBSA_UART_FIFO_SIZE / 2)), you would only clear it 
when the ring has less than 16 bytes queued.
I have a few requests on those patches for the resender:
	- Please introduce a define for SBSA_UART_FIFO_SIZE / 2 and use it 
everywhere.
	- Please add a bit more documentation on top of the checks in 
vpl011_read_data function. The checks in vpl011_write_data looks 
well-documented.
Cheers,

--
Julien Grall

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