Re: [Xen-devel] [RFC PATCH 2/2] xen/arm: Add MESON UART driver for Amlogic S905 SoC

[Amit Tomer <amittomer25@xxxxxxxxx>]

Hello,

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.

> AFAIK, {0} is not necessary.

Ok.

>> +
>> +#define meson_s905_read(uart, off)           readl((uart)->regs + off)
>> +#define meson_s905_write(uart, off, val)     writel(val, (uart->regs) +
>> off)
>
>
> s/(uart->regs)/(uart)->regs/
>
>> +
>> +static void meson_s905_uart_interrupt(int irq, void *data,
>> +        struct cpu_user_regs *regs)
>
>
> The indentation looks wrong here.
>
>> +{
>> +    struct serial_port *port = data;
>> +    struct meson_s905_uart *uart = port->uart;
>> +    uint32_t st = meson_s905_read(uart, UART_STATUS);
>> +
>> +    if ( !(st & UART_RX_EMPTY) )
>> +    {
>> +        serial_rx_interrupt(port, regs);
>> +    }
>> +
>> +    if ( !(st & UART_TX_FULL) )
>> +    {
>> +        if ( st & UART_TX_INT_EN )
>> +            serial_tx_interrupt(port, regs);
>> +    }
>> +
>
>
> NIT: No need for this newline.
>
>> +}
>> +
>> +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.

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

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

Thanks
-Amit

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