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

[Oleksandr <olekstysh@gmail.com>]

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@epam.com>
>>
>> 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel