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

[Oleksandr <olekstysh@xxxxxxxxx>]

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?


> Similarly, you seem to define overrun_mask for SCIFA (see next patch) 
> but it will never be used as, AFAICT, status_reg == overrun_reg.
> > +        {
> > +            if ( scif_readw(uart, params->overrun_reg) & 
> > params->overrun_mask )
> > +                scif_writew(uart, params->overrun_reg, 
> > ~params->overrun_mask); > +        }
> >             ctrl = scif_readw(uart, SCIF_SCSCR);
> > -        status = scif_readw(uart, SCIF_SCFSR) & ~SCFSR_TEND;
> > +        status = scif_readw(uart, params->status_reg) & ~SCFSR_TEND;
> >           /* Ignore next flag if TX Interrupt is disabled */
> >           if ( !(ctrl & SCSCR_TIE) )
> >               status &= ~SCFSR_TDFE;
> > @@ -81,12 +119,13 @@ static void scif_uart_interrupt(int irq, void 
> > *data, struct cpu_user_regs *regs)
> >   static void __init scif_uart_init_preirq(struct serial_port *port)
> >   {
> >       struct scif_uart *uart = port->uart;
> > +    const struct port_params *params = uart->params;
> >         /*
> >        * Wait until last bit has been transmitted. This is needed for 
> > a smooth
> >        * transition when we come from early printk
> >        */
> > -    while ( !(scif_readw(uart, SCIF_SCFSR) & SCFSR_TEND) );
> > +    while ( !(scif_readw(uart, params->status_reg) & SCFSR_TEND) );
> >         /* Disable TX/RX parts and all interrupts */
> >       scif_writew(uart, SCIF_SCSCR, 0);
> > @@ -95,10 +134,10 @@ static void __init scif_uart_init_preirq(struct 
> > serial_port *port)
> >       scif_writew(uart, SCIF_SCFCR, SCFCR_RFRST | SCFCR_TFRST);
> >         /* Clear all errors and flags */
> > -    scif_readw(uart, SCIF_SCFSR);
> > -    scif_writew(uart, SCIF_SCFSR, 0);
> > -    scif_readw(uart, SCIF_SCLSR);
> > -    scif_writew(uart, SCIF_SCLSR, 0);
> > +    scif_readw(uart, params->status_reg);
> > +    scif_writew(uart, params->status_reg, 0);
> > +    scif_readw(uart, params->overrun_reg);
> > +    scif_writew(uart, params->overrun_reg, 0);
> >         /* Setup trigger level for TX/RX FIFOs */
> >       scif_writew(uart, SCIF_SCFCR, SCFCR_RTRG11 | SCFCR_TTRG11);
> > @@ -111,6 +150,7 @@ static void __init scif_uart_init_preirq(struct 
> > serial_port *port)
> >   static void __init scif_uart_init_postirq(struct serial_port *port)
> >   {
> >       struct scif_uart *uart = port->uart;
> > +    const struct port_params *params = uart->params;
> >       int rc;
> >         uart->irqaction.handler = scif_uart_interrupt;
> > @@ -122,14 +162,17 @@ static void __init 
> > scif_uart_init_postirq(struct serial_port *port)
> >                   uart->irq);
> >         /* Clear all errors */
> > -    if ( scif_readw(uart, SCIF_SCFSR) & SCIF_ERRORS )
> > -        scif_writew(uart, SCIF_SCFSR, ~SCIF_ERRORS);
> > -    if ( scif_readw(uart, SCIF_SCLSR) & SCLSR_ORER )
> > -        scif_writew(uart, SCIF_SCLSR, 0);
> > +    if ( scif_readw(uart, params->status_reg) & params->error_mask )
> > +        scif_writew(uart, params->status_reg, ~params->error_mask);
> > +    if ( params->overrun_reg != params->status_reg )
> Same question here.
>
> [...]
Please see the answer above.


> > +static const struct dt_device_match scif_uart_dt_match[] __initconst =
> > +{
> > +    { .compatible = "renesas,scif",  .data = (void *)SCIF_PORT },
> > +    { /* sentinel */ },
> > +};
> > +
> >   static int __init scif_uart_init(struct dt_device_node *dev,
> >                                    const void *data)
> >   {
> > +    const struct dt_device_match *match;
> >       const char *config = data;
> >       struct scif_uart *uart;
> >       int res;
> > @@ -265,10 +319,13 @@ static int __init scif_uart_init(struct 
> > dt_device_node *dev,
> >           return -ENOMEM;
> >       }
> >   +    match = dt_match_node(scif_uart_dt_match, dev);
> Technically dt_match_node may return NULL here. This should never be 
> the case as you know it will always match.
> So I would add an ASSERT(match) here.
OK


> Cheers,
--
Regards,

Oleksandr Tyshchenko


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