[05/15] tty: serial: Add 8250-core based omap driver

Commit Message

Heikki Krogerus Aug. 18, 2014, 1:46 p.m. UTC

Hi,

On Fri, Aug 15, 2014 at 07:42:33PM +0200, Sebastian Andrzej Siewior wrote:
> diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
> index cc90c19..ab003b6 100644
> --- a/drivers/tty/serial/8250/8250_core.c
> +++ b/drivers/tty/serial/8250/8250_core.c
> @@ -264,6 +264,12 @@ static const struct serial8250_config uart_config[] = {
>  		.fcr		= UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_10,
>  		.flags		= UART_CAP_FIFO | UART_CAP_AFE,
>  	},
> +	[PORT_OMAP_16750] = {
> +		.name		= "OMAP",
> +		.fifo_size	= 64,
> +		.tx_loadsz	= 64,
> +		.flags		= UART_CAP_FIFO | UART_CAP_EFR | UART_CAP_SLEEP,
> +	},

I don't think you need this. Reasons below...

>  	[PORT_TEGRA] = {
>  		.name		= "Tegra",
>  		.fifo_size	= 32,
> @@ -1390,6 +1396,8 @@ static void serial8250_stop_rx(struct uart_port *port)
>  	serial8250_rpm_get(up);
>  
>  	up->ier &= ~UART_IER_RLSI;
> +	if (port->type == PORT_OMAP_16750)
> +		up->ier &= ~UART_IER_RDI;

I don't see any reason why this could not be always cleared regardless
of the type:

        up->ier &= ~(UART_IER_RLSI | UART_IRE_RDI);

>  	up->port.read_status_mask &= ~UART_LSR_DR;
>  	serial_port_out(port, UART_IER, up->ier);
>  
> diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
> new file mode 100644
> index 0000000..368e9d8
> --- /dev/null
> +++ b/drivers/tty/serial/8250/8250_omap.c
> @@ -0,0 +1,911 @@
> +/*
> + * 8250-core based driver for the OMAP internal UART
> + *
> + *  Copyright (C) 2014 Sebastian Andrzej Siewior
> + *
> + */

[cut]

> +static int omap8250_probe(struct platform_device *pdev)
> +{
> +	struct resource *regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	struct resource *irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> +	struct omap8250_priv *priv;
> +	struct uart_8250_port up;
> +	int ret;
> +	void __iomem *membase;
> +
> +	if (!regs || !irq) {
> +		dev_err(&pdev->dev, "missing registers or irq\n");
> +		return -EINVAL;
> +	}
> +
> +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv) {
> +		dev_err(&pdev->dev, "unable to allocate private data\n");
> +		return -ENOMEM;
> +	}
> +	membase = devm_ioremap_nocache(&pdev->dev, regs->start,
> +			resource_size(regs));
> +	if (!membase)
> +		return -ENODEV;
> +
> +	memset(&up, 0, sizeof(up));
> +	up.port.dev = &pdev->dev;
> +	up.port.mapbase = regs->start;
> +	up.port.membase = membase;
> +	up.port.irq = irq->start;
> +	/*
> +	 * It claims to be 16C750 compatible however it is a little different.
> +	 * It has EFR and has no FCR7_64byte bit. The AFE which it claims to
> +	 * have is enabled via EFR instead of MCR.
> +	 */
> +	up.port.type = PORT_OMAP_16750;

Since you are not calling serial8250_do_set_termios, 8250_core.c newer
overrides the FCR set in this driver. However, if the FCR is a
problem, since Yoshihiro added the member for it to struct
uart_8250_port (commit aef9a7bd9), just make it possible for the probe
drivers to provide also it's value:


So instead of using PORT_OMAP_16750:
        up.port.type = PORT_16750;
        up.capabilities = UART_CAP_FIFO | UART_CAP_EFR | UART_CAP_SLEEP;

and the fcr if needed.
        up.fcr = ...

> +	up.port.iotype = UPIO_MEM32;
> +	up.port.flags = UPF_BOOT_AUTOCONF | UPF_FIXED_PORT | UPF_FIXED_TYPE |
> +		UPF_SOFT_FLOW | UPF_HARD_FLOW;
> +	up.port.private_data = priv;
> +
> +	up.port.regshift = 2;
> +	up.port.fifosize = 64;

You don't need to set the fifosize unless you want to replace the
value you get from uart_config array.

> +	up.port.set_termios = omap_8250_set_termios;
> +	up.port.pm = omap_8250_pm;
> +	up.port.startup = omap_8250_startup;
> +	up.port.shutdown = omap_8250_shutdown;
> +	up.port.throttle = omap_8250_throttle;
> +	up.port.unthrottle = omap_8250_unthrottle;
> +
> +	if (pdev->dev.of_node) {
> +		up.port.line = of_alias_get_id(pdev->dev.of_node, "serial");
> +		of_property_read_u32(pdev->dev.of_node, "clock-frequency",
> +				&up.port.uartclk);
> +		priv->wakeirq = irq_of_parse_and_map(pdev->dev.of_node, 1);
> +	} else {
> +		up.port.line = pdev->id;
> +	}
> +
> +	if (up.port.line < 0) {
> +		dev_err(&pdev->dev, "failed to get alias/pdev id, errno %d\n",
> +				up.port.line);
> +		return -ENODEV;
> +	}
> +	if (!up.port.uartclk) {
> +		up.port.uartclk = DEFAULT_CLK_SPEED;
> +		dev_warn(&pdev->dev,
> +				"No clock speed specified: using default: %d\n",
> +				DEFAULT_CLK_SPEED);
> +	}
> +
> +#ifdef CONFIG_PM_RUNTIME
> +	up.capabilities |= UART_CAP_RPM;
> +#endif

By setting this here, you will not get the capabilities from the
uart_config[type].flags if runtime PM is enabled in any case, right?

[cut]

> diff --git a/drivers/tty/serial/8250/Makefile b/drivers/tty/serial/8250/Makefile
> index 36d68d0..4bac392 100644
> --- a/drivers/tty/serial/8250/Makefile
> +++ b/drivers/tty/serial/8250/Makefile
> @@ -20,3 +20,4 @@ obj-$(CONFIG_SERIAL_8250_HUB6)		+= 8250_hub6.o
>  obj-$(CONFIG_SERIAL_8250_FSL)		+= 8250_fsl.o
>  obj-$(CONFIG_SERIAL_8250_DW)		+= 8250_dw.o
>  obj-$(CONFIG_SERIAL_8250_EM)		+= 8250_em.o
> +obj-$(CONFIG_SERIAL_8250_OMAP)		+= 8250_omap.o
> diff --git a/include/uapi/linux/serial_core.h b/include/uapi/linux/serial_core.h
> index 5820269..74f9b11 100644
> --- a/include/uapi/linux/serial_core.h
> +++ b/include/uapi/linux/serial_core.h
> @@ -54,7 +54,8 @@
>  #define PORT_ALTR_16550_F32 26	/* Altera 16550 UART with 32 FIFOs */
>  #define PORT_ALTR_16550_F64 27	/* Altera 16550 UART with 64 FIFOs */
>  #define PORT_ALTR_16550_F128 28 /* Altera 16550 UART with 128 FIFOs */
> -#define PORT_MAX_8250	28	/* max port ID */
> +#define PORT_OMAP_16750	29	/* TI's OMAP internal 16C750 compatible UART */
> +#define PORT_MAX_8250	29	/* max port ID */

So in this case I see no reason for new type.

Cheers,

Comments

Sebastian Andrzej Siewior Sept. 1, 2014, 1:31 p.m. UTC | #1

On 08/18/2014 03:46 PM, Heikki Krogerus wrote:
> On Fri, Aug 15, 2014 at 07:42:33PM +0200, Sebastian Andrzej Siewior wrote:
>> diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
>> index cc90c19..ab003b6 100644
>> --- a/drivers/tty/serial/8250/8250_core.c
>> +++ b/drivers/tty/serial/8250/8250_core.c
>> @@ -264,6 +264,12 @@ static const struct serial8250_config uart_config[] = {
>>  		.fcr		= UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_10,
>>  		.flags		= UART_CAP_FIFO | UART_CAP_AFE,
>>  	},
>> +	[PORT_OMAP_16750] = {
>> +		.name		= "OMAP",
>> +		.fifo_size	= 64,
>> +		.tx_loadsz	= 64,
>> +		.flags		= UART_CAP_FIFO | UART_CAP_EFR | UART_CAP_SLEEP,
>> +	},
> 
> I don't think you need this. Reasons below...

For those it works. However I have to this value to something because
it can't stay PORT_UNKNOWN. So for now I took PORT_8250 because it is
not used anywhere in the code. The only side effect of this is that I
can't specify the name. I can live with this…

> 
>>  	[PORT_TEGRA] = {
>>  		.name		= "Tegra",
>>  		.fifo_size	= 32,
>> @@ -1390,6 +1396,8 @@ static void serial8250_stop_rx(struct uart_port *port)
>>  	serial8250_rpm_get(up);
>>  
>>  	up->ier &= ~UART_IER_RLSI;
>> +	if (port->type == PORT_OMAP_16750)
>> +		up->ier &= ~UART_IER_RDI;
> 
> I don't see any reason why this could not be always cleared regardless
> of the type:
> 
>         up->ier &= ~(UART_IER_RLSI | UART_IRE_RDI);
> 

I remember you brought that up recently asking Alan if it is okay doing
so. Since it looks sane to revert that bit on RX-stop, I will drop that
omap check here.

> [cut]
> 
> Since you are not calling serial8250_do_set_termios, 8250_core.c newer
> overrides the FCR set in this driver. However, if the FCR is a
> problem, since Yoshihiro added the member for it to struct
> uart_8250_port (commit aef9a7bd9), just make it possible for the probe
> drivers to provide also it's value:
> 
>  static int
> 
> So instead of using PORT_OMAP_16750:
>         up.port.type = PORT_16750;
>         up.capabilities = UART_CAP_FIFO | UART_CAP_EFR | UART_CAP_SLEEP;
> 
> and the fcr if needed.
>         up.fcr = ...
> 

That fcr value looks nice so I can't drop my private copy of it.  But
this FCR works different for RX trigger (the way it is used). Which
means to support user configurable RX-level I would need to overwrite
that callback. However since PORT_8250 does not supply any FCR values,
I can just ignore it for now.

>> +	up.port.iotype = UPIO_MEM32;
>> +	up.port.flags = UPF_BOOT_AUTOCONF | UPF_FIXED_PORT | UPF_FIXED_TYPE |
>> +		UPF_SOFT_FLOW | UPF_HARD_FLOW;
>> +	up.port.private_data = priv;
>> +
>> +	up.port.regshift = 2;
>> +	up.port.fifosize = 64;
> 
> You don't need to set the fifosize unless you want to replace the
> value you get from uart_config array.
Since you made me drop my uart_config array entry I keep this and add
the other values, too

>> +	up.port.set_termios = omap_8250_set_termios;
>> +	up.port.pm = omap_8250_pm;
>> +	up.port.startup = omap_8250_startup;
>> +	up.port.shutdown = omap_8250_shutdown;
>> +	up.port.throttle = omap_8250_throttle;
>> +	up.port.unthrottle = omap_8250_unthrottle;
>> +
>> +	if (pdev->dev.of_node) {
>> +		up.port.line = of_alias_get_id(pdev->dev.of_node, "serial");
>> +		of_property_read_u32(pdev->dev.of_node, "clock-frequency",
>> +				&up.port.uartclk);
>> +		priv->wakeirq = irq_of_parse_and_map(pdev->dev.of_node, 1);
>> +	} else {
>> +		up.port.line = pdev->id;
>> +	}
>> +
>> +	if (up.port.line < 0) {
>> +		dev_err(&pdev->dev, "failed to get alias/pdev id, errno %d\n",
>> +				up.port.line);
>> +		return -ENODEV;
>> +	}
>> +	if (!up.port.uartclk) {
>> +		up.port.uartclk = DEFAULT_CLK_SPEED;
>> +		dev_warn(&pdev->dev,
>> +				"No clock speed specified: using default: %d\n",
>> +				DEFAULT_CLK_SPEED);
>> +	}
>> +
>> +#ifdef CONFIG_PM_RUNTIME
>> +	up.capabilities |= UART_CAP_RPM;
>> +#endif
> 
> By setting this here, you will not get the capabilities from the
> uart_config[type].flags if runtime PM is enabled in any case, right?

Yes. It was not plan to behave like this and is fixed, thanks.

> [cut]
> 
>> diff --git a/drivers/tty/serial/8250/Makefile b/drivers/tty/serial/8250/Makefile
>> index 36d68d0..4bac392 100644
>> --- a/drivers/tty/serial/8250/Makefile
>> +++ b/drivers/tty/serial/8250/Makefile
>> @@ -20,3 +20,4 @@ obj-$(CONFIG_SERIAL_8250_HUB6)		+= 8250_hub6.o
>>  obj-$(CONFIG_SERIAL_8250_FSL)		+= 8250_fsl.o
>>  obj-$(CONFIG_SERIAL_8250_DW)		+= 8250_dw.o
>>  obj-$(CONFIG_SERIAL_8250_EM)		+= 8250_em.o
>> +obj-$(CONFIG_SERIAL_8250_OMAP)		+= 8250_omap.o
>> diff --git a/include/uapi/linux/serial_core.h b/include/uapi/linux/serial_core.h
>> index 5820269..74f9b11 100644
>> --- a/include/uapi/linux/serial_core.h
>> +++ b/include/uapi/linux/serial_core.h
>> @@ -54,7 +54,8 @@
>>  #define PORT_ALTR_16550_F32 26	/* Altera 16550 UART with 32 FIFOs */
>>  #define PORT_ALTR_16550_F64 27	/* Altera 16550 UART with 64 FIFOs */
>>  #define PORT_ALTR_16550_F128 28 /* Altera 16550 UART with 128 FIFOs */
>> -#define PORT_MAX_8250	28	/* max port ID */
>> +#define PORT_OMAP_16750	29	/* TI's OMAP internal 16C750 compatible UART */
>> +#define PORT_MAX_8250	29	/* max port ID */
> 
> So in this case I see no reason for new type.

gone.

> 
> Cheers,
> 


Sebastian

Patch

--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -2829,7 +2829,9 @@  static void serial8250_config_port(struct uart_port *port, int flags)
                port->handle_irq = exar_handle_irq;
 
        register_dev_spec_attr_grp(up);
-       up->fcr = uart_config[up->port.type].fcr;
+
+       if (!up->fcr)
+               up->fcr = uart_config[up->port.type].fcr;
 }
 
 static int