Re: [Xen-devel] [PATCH V3 2/5] xen/arm: drivers: scif: Extend driver to handle other interfaces

[Oleksandr <olekstysh@xxxxxxxxx>]

On 16.04.19 12:11, Julien Grall wrote:
> Hi Oleksandr,
Hi Julien


> On 4/15/19 12:30 PM, Oleksandr wrote:
> > On 14.04.19 19:55, Julien Grall wrote:
> > > Hi Oleksandr,
> > Hi Julien
> >
> >
> > > On 4/8/19 11:14 AM, Oleksandr Tyshchenko wrote:
> > > > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> > > >
> > > > Extend driver to be able to handle other SCIF(X) compatible
> > > > interfaces as well. These interfaces have lot in common,
> > > > but mostly differ in offsets and bits for some registers.
> > > Can you briefly explain in the commit message what differs?
> > Sure
> >
> >
> > > [...]
> > >
> > > > @@ -65,13 +100,16 @@ static void scif_uart_interrupt(int irq, void 
> > > > *data, struct cpu_user_regs *regs)
> > > >               serial_rx_interrupt(port, regs);
> > > >             /* Error Interrupt */
> > > > -        if ( status & SCIF_ERRORS )
> > > > -            scif_writew(uart, SCIF_SCFSR, ~SCIF_ERRORS);
> > > > -        if ( scif_readw(uart, SCIF_SCLSR) & SCLSR_ORER )
> > > > -            scif_writew(uart, SCIF_SCLSR, 0);
> > > > +        if ( status & params->error_mask )
> > > > +            scif_writew(uart, params->status_reg, 
> > > > ~params->error_mask);
> > > > +        if ( params->overrun_reg != params->status_reg )
> > > Below you will write the same register twice if overrun_reg == 
> > > status_reg (see scif_uart_init_preirq). Would there be any issue if 
> > > you do the same here?
> > I didn't expect any issue to write the same register twice during 
> > initialization to simplify the code, that why I agreed to remove the 
> > check in V1.
> > But I am not sure about doing so here. We could simplify the code by 
> > just removing "if ( params->overrun_reg != params->status_reg )",
> > but we would need to do extra operation for SCIFA which has 
> > overrun_reg == status_reg.
> > What way do you prefer?
> It is not about preference, it is about correctness. In my previous 
> reply, I pointed out that you define overrun_mask for SCIFA but it 
> will never be used. Without any explanation, the code looks pretty wrong.
> Looking at the next patch, you get away from any problem with SCIFA 
> because the overrun_mask is a subset of error_mask. But I still don't 
> see why trying to prevent to write a second time is an issue, the more 
> the Linux driver does not seem to do this dance.
> So at best, you need to explain in the commit message why you try to 
> skip the register when it is the same.
I got your point.


I can make the following changes:

1. Drop "if ( params->overrun_reg != params->status_reg )" check everywhere.

2. Remove overrun_bit (SCASSR_ORER) from error_mask for SCIFA.

This way we will get a much cleaner code where overrun_mask is used for 
both interfaces.
What do you think?


> Cheers,
--
Regards,

Oleksandr Tyshchenko


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel