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

[Julien Grall <julien.grall@xxxxxxx>]

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