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

Re: [Xen-devel] [PATCH v1 2/2] xen/arm: Add MESON UART driver for Amlogic Meson SoCs


  • To: Amit Singh Tomar <amittomer25@xxxxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx
  • From: André Przywara <andre.przywara@xxxxxxx>
  • Date: Sun, 17 Mar 2019 15:16:57 +0000
  • Autocrypt: addr=andre.przywara@xxxxxxx; prefer-encrypt=mutual; keydata= mQINBFNPCKMBEAC+6GVcuP9ri8r+gg2fHZDedOmFRZPtcrMMF2Cx6KrTUT0YEISsqPoJTKld tPfEG0KnRL9CWvftyHseWTnU2Gi7hKNwhRkC0oBL5Er2hhNpoi8x4VcsxQ6bHG5/dA7ctvL6 kYvKAZw4X2Y3GTbAZIOLf+leNPiF9175S8pvqMPi0qu67RWZD5H/uT/TfLpvmmOlRzNiXMBm kGvewkBpL3R2clHquv7pB6KLoY3uvjFhZfEedqSqTwBVu/JVZZO7tvYCJPfyY5JG9+BjPmr+ REe2gS6w/4DJ4D8oMWKoY3r6ZpHx3YS2hWZFUYiCYovPxfj5+bOr78sg3JleEd0OB0yYtzTT esiNlQpCo0oOevwHR+jUiaZevM4xCyt23L2G+euzdRsUZcK/M6qYf41Dy6Afqa+PxgMEiDto ITEH3Dv+zfzwdeqCuNU0VOGrQZs/vrKOUmU/QDlYL7G8OIg5Ekheq4N+Ay+3EYCROXkstQnf YYxRn5F1oeVeqoh1LgGH7YN9H9LeIajwBD8OgiZDVsmb67DdF6EQtklH0ycBcVodG1zTCfqM AavYMfhldNMBg4vaLh0cJ/3ZXZNIyDlV372GmxSJJiidxDm7E1PkgdfCnHk+pD8YeITmSNyb 7qeU08Hqqh4ui8SSeUp7+yie9zBhJB5vVBJoO5D0MikZAODIDwARAQABtC1BbmRyZSBQcnp5 d2FyYSAoQVJNKSA8YW5kcmUucHJ6eXdhcmFAYXJtLmNvbT6JAjsEEwECACUCGwMGCwkIBwMC BhUIAgkKCwQWAgMBAh4BAheABQJTWSV8AhkBAAoJEAL1yD+ydue63REP/1tPqTo/f6StS00g NTUpjgVqxgsPWYWwSLkgkaUZn2z9Edv86BLpqTY8OBQZ19EUwfNehcnvR+Olw+7wxNnatyxo D2FG0paTia1SjxaJ8Nx3e85jy6l7N2AQrTCFCtFN9lp8Pc0LVBpSbjmP+Peh5Mi7gtCBNkpz KShEaJE25a/+rnIrIXzJHrsbC2GwcssAF3bd03iU41J1gMTalB6HCtQUwgqSsbG8MsR/IwHW XruOnVp0GQRJwlw07e9T3PKTLj3LWsAPe0LHm5W1Q+euoCLsZfYwr7phQ19HAxSCu8hzp43u zSw0+sEQsO+9wz2nGDgQCGepCcJR1lygVn2zwRTQKbq7Hjs+IWZ0gN2nDajScuR1RsxTE4WR lj0+Ne6VrAmPiW6QqRhliDO+e82riI75ywSWrJb9TQw0+UkIQ2DlNr0u0TwCUTcQNN6aKnru ouVt3qoRlcD5MuRhLH+ttAcmNITMg7GQ6RQajWrSKuKFrt6iuDbjgO2cnaTrLbNBBKPTG4oF D6kX8Zea0KvVBagBsaC1CDTDQQMxYBPDBSlqYCb/b2x7KHTvTAHUBSsBRL6MKz8wwruDodTM 4E4ToV9URl4aE/msBZ4GLTtEmUHBh4/AYwk6ACYByYKyx5r3PDG0iHnJ8bV0OeyQ9ujfgBBP B2t4oASNnIOeGEEcQ2rjuQINBFNPCKMBEACm7Xqafb1Dp1nDl06aw/3O9ixWsGMv1Uhfd2B6 it6wh1HDCn9HpekgouR2HLMvdd3Y//GG89irEasjzENZPsK82PS0bvkxxIHRFm0pikF4ljIb 6tca2sxFr/H7CCtWYZjZzPgnOPtnagN0qVVyEM7L5f7KjGb1/o5EDkVR2SVSSjrlmNdTL2Rd zaPqrBoxuR/y/n856deWqS1ZssOpqwKhxT1IVlF6S47CjFJ3+fiHNjkljLfxzDyQXwXCNoZn BKcW9PvAMf6W1DGASoXtsMg4HHzZ5fW+vnjzvWiC4pXrcP7Ivfxx5pB+nGiOfOY+/VSUlW/9 GdzPlOIc1bGyKc6tGREH5lErmeoJZ5k7E9cMJx+xzuDItvnZbf6RuH5fg3QsljQy8jLlr4S6 8YwxlObySJ5K+suPRzZOG2+kq77RJVqAgZXp3Zdvdaov4a5J3H8pxzjj0yZ2JZlndM4X7Msr P5tfxy1WvV4Km6QeFAsjcF5gM+wWl+mf2qrlp3dRwniG1vkLsnQugQ4oNUrx0ahwOSm9p6kM CIiTITo+W7O9KEE9XCb4vV0ejmLlgdDV8ASVUekeTJkmRIBnz0fa4pa1vbtZoi6/LlIdAEEt PY6p3hgkLLtr2GRodOW/Y3vPRd9+rJHq/tLIfwc58ZhQKmRcgrhtlnuTGTmyUqGSiMNfpwAR AQABiQIfBBgBAgAJBQJTTwijAhsMAAoJEAL1yD+ydue64BgP/33QKczgAvSdj9XTC14wZCGE U8ygZwkkyNf021iNMj+o0dpLU48PIhHIMTXlM2aiiZlPWgKVlDRjlYuc9EZqGgbOOuR/pNYA JX9vaqszyE34JzXBL9DBKUuAui8z8GcxRcz49/xtzzP0kH3OQbBIqZWuMRxKEpRptRT0wzBL O31ygf4FRxs68jvPCuZjTGKELIo656/Hmk17cmjoBAJK7JHfqdGkDXk5tneeHCkB411p9WJU vMO2EqsHjobjuFm89hI0pSxlUoiTL0Nuk9Edemjw70W4anGNyaQtBq+qu1RdjUPBvoJec7y/ EXJtoGxq9Y+tmm22xwApSiIOyMwUi9A1iLjQLmngLeUdsHyrEWTbEYHd2sAM2sqKoZRyBDSv ejRvZD6zwkY/9nRqXt02H1quVOP42xlkwOQU6gxm93o/bxd7S5tEA359Sli5gZRaucpNQkwd KLQdCvFdksD270r4jU/rwR2R/Ubi+txfy0dk2wGBjl1xpSf0Lbl/KMR5TQntELfLR4etizLq Xpd2byn96Ivi8C8u9zJruXTueHH8vt7gJ1oax3yKRGU5o2eipCRiKZ0s/T7fvkdq+8beg9ku fDO4SAgJMIl6H5awliCY2zQvLHysS/Wb8QuB09hmhLZ4AifdHyF1J5qeePEhgTA+BaUbiUZf i4aIXCH3Wv6K
  • Cc: julien.grall@xxxxxxx, sstabellini@xxxxxxxxxx
  • Delivery-date: Sun, 17 Mar 2019 15:19:07 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Openpgp: preference=signencrypt

On 26/01/2019 08:53, Amit Singh Tomar wrote:
> This patch adds driver for UART controller present on Amlogic Meson
> SoCs and it has been tested on Nanopi K2 board based on S905 SoC.
> 
> Controller registers defination is taken from Linux 4.20.
> 
> Signed-off-by: Amit Singh Tomar <amittomer25@xxxxxxxxx>
> ---
> Changes since RFC:
> 
>         * Removed S905 reference as other Amlogic SoCs
>           have this uart controller.
>         * Replaced meson_s905_read/write helper
>           with clrsetbit and friends helper.
>         * Followed proper UART reset sequence.
>         * List all UART compatible strings same as Linux
>           driver.
> ---
>  xen/drivers/char/Kconfig      |   8 ++
>  xen/drivers/char/Makefile     |   1 +
>  xen/drivers/char/meson-uart.c | 282 
> ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 291 insertions(+)
>  create mode 100644 xen/drivers/char/meson-uart.c
> 
> diff --git a/xen/drivers/char/Kconfig b/xen/drivers/char/Kconfig
> index b1f07f8..d4add7f 100644
> --- a/xen/drivers/char/Kconfig
> +++ b/xen/drivers/char/Kconfig
> @@ -20,6 +20,14 @@ config HAS_MVEBU
>         This selects the Marvell MVEBU UART. If you have a ARMADA 3700
>         based board, say Y.
>  
> +config HAS_MESON
> +        bool "Amlogic MESON UART driver"
> +        default y
> +        depends on ARM_64
> +        help
> +          This selects the Amlogic MESON UART. If you have a Amlogic based
> +          board, say Y.
> +
>  config HAS_PL011
>       bool "ARM PL011 UART driver"
>       default y
> diff --git a/xen/drivers/char/Makefile b/xen/drivers/char/Makefile
> index b68c330..7c646d7 100644
> --- a/xen/drivers/char/Makefile
> +++ b/xen/drivers/char/Makefile
> @@ -3,6 +3,7 @@ obj-$(CONFIG_HAS_NS16550) += ns16550.o
>  obj-$(CONFIG_HAS_CADENCE_UART) += cadence-uart.o
>  obj-$(CONFIG_HAS_PL011) += pl011.o
>  obj-$(CONFIG_HAS_EXYNOS4210) += exynos4210-uart.o
> +obj-$(CONFIG_HAS_MESON) += meson-uart.o
>  obj-$(CONFIG_HAS_MVEBU) += mvebu-uart.o
>  obj-$(CONFIG_HAS_OMAP) += omap-uart.o
>  obj-$(CONFIG_HAS_SCIF) += scif-uart.o
> diff --git a/xen/drivers/char/meson-uart.c b/xen/drivers/char/meson-uart.c
> new file mode 100644
> index 0000000..990ae95
> --- /dev/null
> +++ b/xen/drivers/char/meson-uart.c
> @@ -0,0 +1,282 @@
> +/*
> + * xen/drivers/char/meson-uart.c
> + *
> + * Driver for Amlogic MESON UART
> + *
> + * Copyright (c) 2019, Amit Singh Tomar <amittomer25@xxxxxxxxx>.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms and conditions of the GNU General Public
> + * License, version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public
> + * License along with this program; If not, see 
> <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <xen/irq.h>
> +#include <xen/serial.h>
> +#include <xen/vmap.h>
> +#include <asm/io.h>
> +
> +/* Register offsets */
> +#define AML_UART_WFIFO_REG              0x00
> +#define AML_UART_RFIFO_REG              0x04
> +#define AML_UART_CONTROL_REG            0x08
> +#define AML_UART_STATUS_REG             0x0c
> +#define AML_UART_MISC_REG               0x10
> +
> +/* UART_CONTROL bits */
> +#define AML_UART_TX_RST                 BIT(22)
> +#define AML_UART_RX_RST                 BIT(23)
> +#define AML_UART_CLEAR_ERR              BIT(24)
> +#define AML_UART_RX_INT_EN              BIT(27)
> +#define AML_UART_TX_INT_EN              BIT(28)
> +
> +/* UART_STATUS bits */
> +#define AML_UART_RX_FIFO_EMPTY          BIT(20)
> +#define AML_UART_TX_FIFO_FULL           BIT(21)
> +#define AML_UART_TX_FIFO_EMPTY          BIT(22)
> +#define AML_UART_TX_CNT_MASK            GENMASK(14, 8)
> +
> +/* AML_UART_MISC bits */
> +#define AML_UART_XMIT_IRQ(c)            (((c) & 0xff) << 8)
> +#define AML_UART_RECV_IRQ(c)            ((c) & 0xff)
> +
> +#define TX_FIFO_SIZE                    64
> +
> +#define setbits(addr, set)              writel((readl(addr) | (set)), (addr))
> +#define clrbits(addr, clear)            writel((readl(addr) & ~(clear)), 
> (addr))
> +#define clrsetbits(addr, clear, set)    writel(((readl(addr) & ~(clear)) \
> +                                        | (set)), (addr))

You don't seem to need this last one. Every user of it below sets "set"
to 0, so it's effectively just clrbits().

And keeping the other two as macros is probably fine then.

> +
> +static struct meson_uart {
> +    unsigned int irq;
> +    void __iomem *regs;
> +    struct irqaction irqaction;
> +    struct vuart_info vuart;
> +} meson_com;
> +
> +static void meson_uart_interrupt(int irq, void *data,
> +                                      struct cpu_user_regs *regs)

w/s

> +{
> +    struct serial_port *port = data;
> +    struct meson_uart *uart = port->uart;
> +    uint32_t st = readl(uart->regs + AML_UART_STATUS_REG);
> +
> +    if ( !(st & AML_UART_RX_FIFO_EMPTY) )
> +    {
> +        serial_rx_interrupt(port, regs);
> +    }
> +
> +    if ( !(st & AML_UART_TX_FIFO_FULL) )
> +    {
> +        serial_tx_interrupt(port, regs);
> +    }
> +}
> +
> +static void __init meson_uart_init_preirq(struct serial_port *port)
> +{
> +    struct meson_uart *uart = port->uart;
> +
> +    /* Reset UART */
> +    setbits(uart->regs + AML_UART_CONTROL_REG,
> +            (AML_UART_RX_RST | AML_UART_TX_RST | AML_UART_CLEAR_ERR));
> +
> +    clrbits(uart->regs + AML_UART_CONTROL_REG,
> +            (AML_UART_RX_RST | AML_UART_TX_RST | AML_UART_CLEAR_ERR));
> +
> +    /* Disable Rx/Tx interrupts */
> +    clrsetbits(uart->regs + AML_UART_CONTROL_REG,
> +               (AML_UART_RX_INT_EN | AML_UART_TX_INT_EN), 0);

This is just clrbits(), which also means you can merge those last two
calls into one.

> +}
> +
> +static void __init meson_uart_init_postirq(struct serial_port *port)
> +{
> +    struct meson_uart *uart = port->uart;
> +
> +    uart->irqaction.handler = meson_uart_interrupt;
> +    uart->irqaction.name    = "meson_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);

Seems like a 905 leftover. Just "Failed to allocate Meson UART IRQ"?

> +        return;
> +    }
> +
> +    /*
> +     * Configure Rx/Tx interrupts based on bytes in FIFO, these bits have
> +     * taken from Linux driver
> +     */
> +    writel((AML_UART_RECV_IRQ(1) | AML_UART_XMIT_IRQ(TX_FIFO_SIZE / 2)),
> +           uart->regs + AML_UART_MISC_REG);
> +
> +    /* Make sure Rx/Tx interrupts are enabled now */
> +    setbits(uart->regs + AML_UART_CONTROL_REG,
> +            (AML_UART_RX_INT_EN | AML_UART_TX_INT_EN));
> +}
> +
> +static void meson_uart_suspend(struct serial_port *port)
> +{
> +    BUG();
> +}
> +
> +static void meson_uart_resume(struct serial_port *port)
> +{
> +    BUG();
> +}
> +
> +static void meson_uart_putc(struct serial_port *port, char c)
> +{
> +    struct meson_uart *uart = port->uart;
> +
> +    writel(c, uart->regs + AML_UART_WFIFO_REG);
> +}
> +
> +static int meson_uart_getc(struct serial_port *port, char *c)
> +{
> +    struct meson_uart *uart = port->uart;
> +
> +    if ( (readl(uart->regs + AML_UART_STATUS_REG) & AML_UART_RX_FIFO_EMPTY) )
> +        return 0;
> +
> +    *c = readl(uart->regs + AML_UART_RFIFO_REG) & 0xff;
> +
> +    return 1;
> +}
> +
> +static int __init meson_irq(struct serial_port *port)
> +{
> +    struct meson_uart *uart = port->uart;
> +
> +    return uart->irq;
> +}
> +
> +static const struct vuart_info *meson_vuart_info(struct serial_port *port)
> +{
> +    struct meson_uart *uart = port->uart;
> +
> +    return &uart->vuart;
> +}
> +
> +static void meson_uart_stop_tx(struct serial_port *port)
> +{
> +    struct meson_uart *uart = port->uart;
> +
> +    clrsetbits(uart->regs + AML_UART_CONTROL_REG, AML_UART_TX_INT_EN, 0);

Just clrbits().

The rest seems fine to me.
Briefly tested by booting Xen (no Dom0) on an Odroid C2.
Both early printk and normale console output look alright.

Cheers,
Andre.

> +}
> +
> +static void meson_uart_start_tx(struct serial_port *port)
> +{
> +    struct meson_uart *uart = port->uart;
> +
> +    setbits(uart->regs + AML_UART_CONTROL_REG, AML_UART_TX_INT_EN);
> +}
> +
> +static int meson_uart_tx_ready(struct serial_port *port)
> +{
> +    struct meson_uart *uart = port->uart;
> +    uint32_t reg;
> +
> +    reg = readl(uart->regs + AML_UART_STATUS_REG);
> +
> +    if ( reg & AML_UART_TX_FIFO_EMPTY )
> +        return TX_FIFO_SIZE;
> +    if ( reg & AML_UART_TX_FIFO_FULL )
> +        return 0;
> +
> +    return (reg & AML_UART_TX_CNT_MASK) >> 8;
> +}
> +
> +static struct uart_driver __read_mostly meson_uart_driver = {
> +    .init_preirq  = meson_uart_init_preirq,
> +    .init_postirq = meson_uart_init_postirq,
> +    .endboot      = NULL,
> +    .suspend      = meson_uart_suspend,
> +    .resume       = meson_uart_resume,
> +    .putc         = meson_uart_putc,
> +    .getc         = meson_uart_getc,
> +    .tx_ready     = meson_uart_tx_ready,
> +    .stop_tx      = meson_uart_stop_tx,
> +    .start_tx     = meson_uart_start_tx,
> +    .irq          = meson_irq,
> +    .vuart_info   = meson_vuart_info,
> +};
> +
> +static int __init meson_uart_init(struct dt_device_node *dev, const void 
> *data)
> +{
> +    const char *config = data;
> +    struct meson_uart *uart;
> +    int res;
> +    u64 addr, size;
> +
> +    if ( strcmp(config, "") )
> +        printk("WARNING: UART configuration is not supported\n");
> +
> +    uart = &meson_com;
> +
> +    res = dt_device_get_address(dev, 0, &addr, &size);
> +    if ( res )
> +    {
> +        printk("meson: Unable to retrieve the base address of the UART\n");
> +        return res;
> +    }
> +
> +    res = platform_get_irq(dev, 0);
> +    if ( res < 0 )
> +    {
> +        printk("meson: Unable to retrieve the IRQ\n");
> +        return -EINVAL;
> +    }
> +
> +    uart->irq  = res;
> +
> +    uart->regs = ioremap_nocache(addr, size);
> +    if ( !uart->regs )
> +    {
> +        printk("meson: Unable to map the UART\n");
> +        return -ENOMEM;
> +    }
> +
> +    uart->vuart.base_addr = addr;
> +    uart->vuart.size = size;
> +    uart->vuart.data_off = AML_UART_WFIFO_REG;
> +    uart->vuart.status_off = AML_UART_STATUS_REG;
> +    uart->vuart.status = AML_UART_RX_FIFO_EMPTY | AML_UART_TX_FIFO_EMPTY;
> +
> +    /* Register with generic serial driver. */
> +    serial_register_uart(SERHND_DTUART, &meson_uart_driver, uart);
> +
> +    dt_device_set_used_by(dev, DOMID_XEN);
> +
> +    return 0;
> +}
> +
> +static const struct dt_device_match meson_dt_match[] __initconst =
> +{
> +    DT_MATCH_COMPATIBLE("amlogic,meson-uart"),
> +    DT_MATCH_COMPATIBLE("amlogic,meson6-uart"),
> +    DT_MATCH_COMPATIBLE("amlogic,meson8-uart"),
> +    DT_MATCH_COMPATIBLE("amlogic,meson8b-uart"),
> +    DT_MATCH_COMPATIBLE("amlogic,meson-gx-uart"),
> +    { /* sentinel */ },
> +};
> +
> +DT_DEVICE_START(meson, "Amlogic UART", DEVICE_SERIAL)
> +    .dt_match = meson_dt_match,
> +    .init = meson_uart_init,
> +DT_DEVICE_END
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> +*/
> 


_______________________________________________
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®.