On 20/08/18 09:12, Amit Tomer wrote:
Hello,
Hi,
Thanks for having a look at it.
The spec does not seems to provide the offset register. Where did you find
them?
Actually, looked at couple of references from u-boot and Linux. These
headers are picked from there.
Please mention it in the commit message then.
[...]
+}
+
+static void __init meson_s905_uart_init_preirq(struct serial_port *port)
+{
+ struct meson_s905_uart *uart = port->uart;
+ uint32_t reg;
+
+ reg = meson_s905_read(uart, UART_CONTROL);
+ reg &= ~(UART_RX_RST | UART_TX_RST | UART_CLEAR_ERR);
I am not sure why you are clearing those bits. AFAIU, init_preirq will reset
the serials, so you want to set thoses bits. This seems to be confirmed by
Linux in meson_uart_reset.
Idea here is to set these bits to their default values(which is 0 ) and if you
look at other drivers in XEN, it seems to be done same thing(clear
those bits) with them.
Are you sure about this? RX_RST and TX_RST are bit to reset the
transmission and receive path. Looking at a couple of different drivers
(cache-uart.c and mvebu-uart.c), those 2 bits are set and I suspect be
cleared by the hardware once reset.
Regarding UART_CLEAR_ERR, you indeed needs to clear the potential errors
by zeroing it.
+ reg = meson_s905_read(uart, UART_CONTROL);
+ reg &= ~(UART_RX_INT_EN | UART_TX_INT_EN);
+ meson_s905_write(uart, UART_CONTROL, reg);
+}
+
+static void __init meson_s905_uart_init_postirq(struct serial_port *port)
+{
+ struct meson_s905_uart *uart = port->uart;
+ uint32_t reg;
+
+ uart->irqaction.handler = meson_s905_uart_interrupt;
+ uart->irqaction.name = "meson_s905_uart";
+ uart->irqaction.dev_id = port;
+
+ if ( setup_irq(uart->irq, 0, &uart->irqaction) != 0 )
+ {
+ printk("Failed to allocated meson_s905_uart IRQ %d\n",
uart->irq);
+ return;
+ }
+
+ /* Configure Rx/Tx interrupts based on bytes in FIFO */
+ reg = meson_s905_read(uart, UART_MISC);
You read UART_MISC here but ...
+ reg = (UART_RECV_IRQ_CNT_MASK & 1) |
... override the value here. You either want to drop reading UART_MISC or
add | here.
Sorry, missed "|" somehow.
+ (UART_XMIT_IRQ_CNT_MASK & ((TX_FIFO_SIZE / 2) << 8));
This is a bit difficult to read. It feels like you want to use a macro with
a parameter that will do the correct masking.
Ok, shall I take it from Linux ?
Yes please.
+
+static const struct dt_device_match meson_dt_match[] __initconst =
+{
+ DT_MATCH_COMPATIBLE("amlogic,meson-uart"),
Looking at Linux, this is considered as a legacy bindings. Would not it be
better to use stable bindings in Xen?
Yeah, I took it from u-boot source and didn't realize that there are
stable binding exists.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
