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/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?

[...]

> @@ -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?
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.

[...]

> +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.

Cheers,

--
Julien Grall

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