[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.