|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v1 2/4] xen/arm: drivers: scif: Add support for SCIFA compatible UARTs
On Tue, Aug 7, 2018 at 4:43 PM, Julien Grall <julien.grall@xxxxxxx> wrote:
> Hi Oleksandr,
Hi, Julien
>
>
> On 06/08/18 19:35, Oleksandr Tyshchenko wrote:
>>
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
>>
>> Extend existing driver to be able to handle SCIFA interface as well.
>> SCIF and SCIFA have lot in common, though SCIFA has different
>> offsets and bits for some registers.
>>
>> Use compatible string to recognize what interface is present
>> on a target board.
>>
>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
>> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>> CC: Julien Grall <julien.grall@xxxxxxx>
>> ---
>> xen/drivers/char/scif-uart.c | 129
>> +++++++++++++++++++++++++++++++---------
>> xen/include/asm-arm/scif-uart.h | 44 ++++++++++++--
>> 2 files changed, 138 insertions(+), 35 deletions(-)
>>
>> diff --git a/xen/drivers/char/scif-uart.c b/xen/drivers/char/scif-uart.c
>> index 465fb34..d1eb713 100644
>> --- a/xen/drivers/char/scif-uart.c
>> +++ b/xen/drivers/char/scif-uart.c
>> @@ -1,7 +1,7 @@
>> /*
>> * xen/drivers/char/scif-uart.c
>> *
>> - * Driver for SCIF (Serial communication interface with FIFO)
>> + * Driver for SCIF(A) (Serial communication interface with FIFO (A))
>> * compatible UART.
>> *
>> * Oleksandr Tyshchenko <oleksandr.tyshchenko@xxxxxxxxxxxxxxx>
>> @@ -40,8 +40,57 @@ static struct scif_uart {
>> char __iomem *regs;
>> struct irqaction irqaction;
>> struct vuart_info vuart;
>> + const struct port_params *params;
>> } scif_com = {0};
>> +enum
>
>
> Should not this enum has a name?
I think, it should.
>
>
>> +{
>> + SCIF_PORT,
>> + SCIFA_PORT,
>> + NR_PORTS,
>> +};
>> +
>> +struct port_params
>> +{
>> + unsigned int status_reg;
>> + unsigned int tx_fifo_reg;
>> + unsigned int rx_fifo_reg;
>> + unsigned int overrun_reg;
>> + unsigned int overrun_mask;
>> + unsigned int error_mask;
>> + unsigned int irq_flags;
>> + unsigned int fifo_size;
>> +};
>> +
>> +static const struct port_params port_params[NR_PORTS] =
>> +{
>> + [SCIF_PORT] =
>> + {
>> + .status_reg = SCIF_SCFSR,
>> + .tx_fifo_reg = SCIF_SCFTDR,
>> + .rx_fifo_reg = SCIF_SCFRDR,
>> + .overrun_reg = SCIF_SCLSR,
>> + .overrun_mask = SCLSR_ORER,
>> + .error_mask = SCFSR_PER | SCFSR_FER | SCFSR_BRK | SCFSR_ER,
>> + .irq_flags = SCSCR_RIE | SCSCR_TIE | SCSCR_REIE,
>> + .fifo_size = 16,
>> + },
>> +
>> + [SCIFA_PORT] =
>> + {
>> + .status_reg = SCIFA_SCASSR,
>> + .tx_fifo_reg = SCIFA_SCAFTDR,
>> + .rx_fifo_reg = SCIFA_SCAFRDR,
>> + .overrun_reg = SCIFA_SCASSR,
>> + .overrun_mask = SCASSR_ORER,
>> + .error_mask = SCASSR_PER | SCASSR_FER | SCASSR_BRK | SCASSR_ER
>> |
>> + SCASSR_ORER,
>> + .irq_flags = SCASCR_RIE | SCASCR_TIE | SCASCR_DRIE |
>> SCASCR_ERIE |
>> + SCASCR_BRIE,
>> + .fifo_size = 64,
>> + },
>> +};
>> +
>> static void scif_uart_interrupt(int irq, void *data, struct
>> cpu_user_regs *regs)
>> {
>> struct serial_port *port = data;
>> @@ -49,7 +98,7 @@ static void scif_uart_interrupt(int irq, void *data,
>> struct cpu_user_regs *regs)
>> uint16_t status, ctrl;
>> ctrl = scif_readw(uart, SCIF_SCSCR);
>> - status = scif_readw(uart, SCIF_SCFSR) & ~SCFSR_TEND;
>> + status = scif_readw(uart, uart->params->status_reg) & ~SCFSR_TEND;
>
>
> I would prefer if you introduce a local variable for the params. This would
> avoid to write uart->params everywhere.
Agree. Do you want me to use this local variable only in interrupt
handler or everywhere in a file where uart->params is used?
>
>
>> /* Ignore next flag if TX Interrupt is disabled */
>> if ( !(ctrl & SCSCR_TIE) )
>> status &= ~SCFSR_TDFE;
>> @@ -65,13 +114,19 @@ 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 & uart->params->error_mask )
>> + scif_writew(uart, uart->params->status_reg,
>> + ~uart->params->error_mask);
>> + if ( uart->params->overrun_reg != uart->params->status_reg )
>> + {
>> + if ( scif_readw(uart, uart->params->overrun_reg) &
>> + uart->params->overrun_mask )
>> + scif_writew(uart, uart->params->overrun_reg,
>> + ~uart->params->overrun_mask);
>> + }
>> ctrl = scif_readw(uart, SCIF_SCSCR);
>> - status = scif_readw(uart, SCIF_SCFSR) & ~SCFSR_TEND;
>> + status = scif_readw(uart, uart->params->status_reg) &
>> ~SCFSR_TEND;
>> /* Ignore next flag if TX Interrupt is disabled */
>> if ( !(ctrl & SCSCR_TIE) )
>> status &= ~SCFSR_TDFE;
>> @@ -86,7 +141,7 @@ static void __init scif_uart_init_preirq(struct
>> serial_port *port)
>> * Wait until last bit has been transmitted. This is needed for a
>> smooth
>> * transition when we come from early printk
>> */
>> - while ( !(scif_readw(uart, SCIF_SCFSR) & SCFSR_TEND) );
>> + while ( !(scif_readw(uart, uart->params->status_reg) & SCFSR_TEND) );
>> /* Disable TX/RX parts and all interrupts */
>> scif_writew(uart, SCIF_SCSCR, 0);
>> @@ -95,10 +150,13 @@ static void __init scif_uart_init_preirq(struct
>> serial_port *port)
>> scif_writew(uart, SCIF_SCFCR, SCFCR_RFRST | SCFCR_TFRST);
>> /* Clear all errors and flags */
>> - scif_readw(uart, SCIF_SCFSR);
>> - scif_writew(uart, SCIF_SCFSR, 0);
>> - scif_readw(uart, SCIF_SCLSR);
>> - scif_writew(uart, SCIF_SCLSR, 0);
>> + scif_readw(uart, uart->params->status_reg);
>> + scif_writew(uart, uart->params->status_reg, 0);
>> + if ( uart->params->overrun_reg != uart->params->status_reg )
>
>
> I don't think there is any harm to reset the registers twice when
> overrun_reg != status_reg.
Agree.
>
>
>> + {
>> + scif_readw(uart, uart->params->overrun_reg);
>> + scif_writew(uart, uart->params->overrun_reg, 0);
>> + }
>> /* Setup trigger level for TX/RX FIFOs */
>> scif_writew(uart, SCIF_SCFCR, SCFCR_RTRG11 | SCFCR_TTRG11);
>> @@ -122,14 +180,19 @@ static void __init scif_uart_init_postirq(struct
>> serial_port *port)
>> uart->irq);
>> /* Clear all errors */
>> - if ( scif_readw(uart, SCIF_SCFSR) & SCIF_ERRORS )
>> - scif_writew(uart, SCIF_SCFSR, ~SCIF_ERRORS);
>> - if ( scif_readw(uart, SCIF_SCLSR) & SCLSR_ORER )
>> - scif_writew(uart, SCIF_SCLSR, 0);
>> + if ( scif_readw(uart, uart->params->status_reg) &
>> uart->params->error_mask )
>> + scif_writew(uart, uart->params->status_reg,
>> ~uart->params->error_mask);
>> + if ( uart->params->overrun_reg != uart->params->status_reg )
>> + {
>> + if ( scif_readw(uart, uart->params->overrun_reg) &
>> + uart->params->overrun_mask )
>> + scif_writew(uart, uart->params->overrun_reg,
>> + ~uart->params->overrun_mask);
>> + }
>> /* Enable TX/RX and Error Interrupts */
>> scif_writew(uart, SCIF_SCSCR, scif_readw(uart, SCIF_SCSCR) |
>> - SCSCR_TIE | SCSCR_RIE | SCSCR_REIE);
>> + uart->params->irq_flags);
>
>
> The indentation looks wrong here.
Will correct.
>
>
>> }
>> static void scif_uart_suspend(struct serial_port *port)
>> @@ -148,24 +211,25 @@ static int scif_uart_tx_ready(struct serial_port
>> *port)
>> uint16_t cnt;
>> /* Check for empty space in TX FIFO */
>> - if ( !(scif_readw(uart, SCIF_SCFSR) & SCFSR_TDFE) )
>> + if ( !(scif_readw(uart, uart->params->status_reg) & SCFSR_TDFE) )
>> return 0;
>> /* Check number of data bytes stored in TX FIFO */
>> cnt = scif_readw(uart, SCIF_SCFDR) >> 8;
>> - ASSERT( cnt >= 0 && cnt <= SCIF_FIFO_MAX_SIZE );
>> + ASSERT( cnt >= 0 && cnt <= uart->params->fifo_size );
>> - return (SCIF_FIFO_MAX_SIZE - cnt);
>> + return (uart->params->fifo_size - cnt);
>> }
>> static void scif_uart_putc(struct serial_port *port, char c)
>> {
>> struct scif_uart *uart = port->uart;
>> - scif_writeb(uart, SCIF_SCFTDR, c);
>> + scif_writeb(uart, uart->params->tx_fifo_reg, c);
>> /* Clear required TX flags */
>> - scif_writew(uart, SCIF_SCFSR, scif_readw(uart, SCIF_SCFSR) &
>> - ~(SCFSR_TEND | SCFSR_TDFE));
>> + scif_writew(uart, uart->params->status_reg,
>> + scif_readw(uart, uart->params->status_reg) &
>> + ~(SCFSR_TEND | SCFSR_TDFE));
>> }
>> static int scif_uart_getc(struct serial_port *port, char *pc)
>> @@ -173,15 +237,16 @@ static int scif_uart_getc(struct serial_port *port,
>> char *pc)
>> struct scif_uart *uart = port->uart;
>> /* Check for available data bytes in RX FIFO */
>> - if ( !(scif_readw(uart, SCIF_SCFSR) & (SCFSR_RDF | SCFSR_DR)) )
>> + if ( !(scif_readw(uart, uart->params->status_reg) &
>> + (SCFSR_RDF | SCFSR_DR)) )
>> return 0;
>> - *pc = scif_readb(uart, SCIF_SCFRDR);
>> + *pc = scif_readb(uart, uart->params->rx_fifo_reg);
>> /* dummy read */
>> - scif_readw(uart, SCIF_SCFSR);
>> + scif_readw(uart, uart->params->status_reg);
>> /* Clear required RX flags */
>> - scif_writew(uart, SCIF_SCFSR, ~(SCFSR_RDF | SCFSR_DR));
>> + scif_writew(uart, uart->params->status_reg, ~(SCFSR_RDF | SCFSR_DR));
>> return 1;
>> }
>> @@ -265,10 +330,15 @@ static int __init scif_uart_init(struct
>> dt_device_node *dev,
>> return -ENOMEM;
>> }
>> + if ( dt_device_is_compatible(dev, "renesas,scifa") )
>> + uart->params = &port_params[SCIFA_PORT];
>> + else
>> + uart->params = &port_params[SCIF_PORT];
>
>
> You can use the field .data in dt_device_match to store specific data and
> then use dt_match_node to find the correct node.
I understand. Will change.
>
>
>> +
>> uart->vuart.base_addr = addr;
>> uart->vuart.size = size;
>> - uart->vuart.data_off = SCIF_SCFTDR;
>> - uart->vuart.status_off = SCIF_SCFSR;
>> + uart->vuart.data_off = uart->params->tx_fifo_reg;
>> + uart->vuart.status_off = uart->params->status_reg;
>> uart->vuart.status = SCFSR_TDFE;
>> /* Register with generic serial driver */
>> @@ -282,6 +352,7 @@ static int __init scif_uart_init(struct dt_device_node
>> *dev,
>> static const struct dt_device_match scif_uart_dt_match[] __initconst =
>> {
>> DT_MATCH_COMPATIBLE("renesas,scif"),
>> + DT_MATCH_COMPATIBLE("renesas,scifa"),
>> { /* sentinel */ },
>> };
>> diff --git a/xen/include/asm-arm/scif-uart.h
>> b/xen/include/asm-arm/scif-uart.h
>> index 8137850..bce3404 100644
>> --- a/xen/include/asm-arm/scif-uart.h
>> +++ b/xen/include/asm-arm/scif-uart.h
>> @@ -2,7 +2,7 @@
>> * xen/include/asm-arm/scif-uart.h
>> *
>> * Common constant definition between early printk and the UART driver
>> - * for the SCIF compatible UART.
>> + * for the SCIF(A) compatible UART.
>> *
>> * Oleksandr Tyshchenko <oleksandr.tyshchenko@xxxxxxxxxxxxxxx>
>> * Copyright (C) 2014, Globallogic.
>> @@ -21,9 +21,7 @@
>> #ifndef __ASM_ARM_SCIF_UART_H
>> #define __ASM_ARM_SCIF_UART_H
>> -#define SCIF_FIFO_MAX_SIZE 16
>> -
>> -/* Register offsets */
>> +/* Register offsets (SCIF) */
>> #define SCIF_SCSMR (0x00) /* Serial mode register */
>> #define SCIF_SCBRR (0x04) /* Bit rate register */
>> #define SCIF_SCSCR (0x08) /* Serial control register */
>> @@ -57,8 +55,6 @@
>> #define SCFSR_RDF (1 << 1) /* Receive FIFO Data Full */
>> #define SCFSR_DR (1 << 0) /* Receive Data Ready */
>> -#define SCIF_ERRORS (SCFSR_PER | SCFSR_FER | SCFSR_ER | SCFSR_BRK)
>> -
>> /* Line Status Register (SCLSR) */
>> #define SCLSR_TO (1 << 2) /* Timeout */
>> #define SCLSR_ORER (1 << 0) /* Overrun Error */
>> @@ -83,6 +79,42 @@
>> #define SCFCR_TTRG10 (SCFCR_TTRG1)
>> #define SCFCR_TTRG11 (SCFCR_TTRG1 | SCFCR_TTRG0)
>> +/* Register offsets (SCIFA) */
>> +#define SCIFA_SCASMR (0x00) /* Serial mode register */
>> +#define SCIFA_SCABRR (0x04) /* Bit rate register */
>> +#define SCIFA_SCASCR (0x08) /* Serial control register */
>> +#define SCIFA_SCATDSR (0x0C) /* Transmit data stop register */
>> +#define SCIFA_SCAFER (0x10) /* FIFO error count register */
>> +#define SCIFA_SCASSR (0x14) /* Serial status register */
>> +#define SCIFA_SCAFCR (0x18) /* FIFO control register */
>> +#define SCIFA_SCAFDR (0x1C) /* FIFO data count register */
>> +#define SCIFA_SCAFTDR (0x20) /* Transmit FIFO data register */
>> +#define SCIFA_SCAFRDR (0x24) /* Receive FIFO data register */
>> +#define SCIFA_SCAPCR (0x30) /* Serial port control register */
>> +#define SCIFA_SCAPDR (0x34) /* Serial port data register */
>> +
>> +/* Serial Control Register (SCASCR) */
>> +#define SCASCR_ERIE (1 << 10) /* Receive Error Interrupt Enable */
>> +#define SCASCR_BRIE (1 << 9) /* Break Interrupt Enable */
>> +#define SCASCR_DRIE (1 << 8) /* Receive Data Ready Interrupt
>> Enable */
>> +#define SCASCR_TIE (1 << 7) /* Transmit Interrupt Enable */
>> +#define SCASCR_RIE (1 << 6) /* Receive Interrupt Enable */
>> +#define SCASCR_TE (1 << 5) /* Transmit Enable */
>> +#define SCASCR_RE (1 << 4) /* Receive Enable */
>> +#define SCASCR_CKE0 (1 << 0) /* Clock Enable 0 */
>> +
>> +/* Serial Status Register (SCASSR) */
>> +#define SCASSR_ORER (1 << 9) /* Overrun Error */
>> +#define SCASSR_TSF (1 << 8) /* Transmit Data Stop */
>> +#define SCASSR_ER (1 << 7) /* Receive Error */
>> +#define SCASSR_TEND (1 << 6) /* Transmission End */
>> +#define SCASSR_TDFE (1 << 5) /* Transmit FIFO Data Empty */
>> +#define SCASSR_BRK (1 << 4) /* Break Detect */
>> +#define SCASSR_FER (1 << 3) /* Framing Error */
>> +#define SCASSR_PER (1 << 2) /* Parity Error */
>> +#define SCASSR_RDF (1 << 1) /* Receive FIFO Data Full */
>> +#define SCASSR_DR (1 << 0) /* Receive Data Ready */
>> +
>> #endif /* __ASM_ARM_SCIF_UART_H */
>> /*
>>
>
> Cheers,
>
> --
> Julien Grall
--
Regards,
Oleksandr Tyshchenko
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |