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

[Julien Grall <julien.grall@xxxxxxx>]

Hi Oleksandr,

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

--
Julien Grall

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