Re: [PATCH] tty: serial: qcom_geni_serial: Fix serial when not used as console

[Matthias Kaehlcke]

On Wed, Sep 05, 2018 at 01:11:46PM -0700, Douglas Anderson wrote:
> If you've got the "console" serial port setup to use just as a UART
> (AKA there is no "console=ttyMSMX" on the kernel command line) then
> certain initialization is skipped.  When userspace later tries to do
> something with the port then things go boom (specifically, on my
> system, some sort of exception hit that caused the system to reboot
> itself w/ no error messages).
> 
> Let's cleanup / refactor the init so that we always run the same init
> code regardless of whether we're using the console.
> 
> To make this work, we make rely on qcom_geni_serial_pm doing its job
> to turn resources on.
> 
> For the record, here is a trace of the order of things (after this
> patch) when console= is specified on the command line and we have an
> agetty on the port:
>   qcom_geni_serial_pm: 4 (undefined) => 0 (on)
>   qcom_geni_console_setup
>   qcom_geni_serial_port_setup
>   qcom_geni_serial_console_write
>   qcom_geni_serial_startup
>   qcom_geni_serial_start_tx
> 
> ...and here is the order of things (after this patch) when console= is
> _NOT_ specified on the command line and we have an agetty port:
>   qcom_geni_serial_pm: 4 => 0
>   qcom_geni_serial_pm: 0 => 3
>   qcom_geni_serial_pm: 3 => 0
>   qcom_geni_serial_startup
>   qcom_geni_serial_port_setup
>   qcom_geni_serial_pm: 0 => 3
>   qcom_geni_serial_pm: 3 => 0
>   qcom_geni_serial_startup
>   qcom_geni_serial_start_tx
> 
> Fixes: c4f528795d1a ("tty: serial: msm_geni_serial: Add serial driver support for GENI based QUP")
> Signed-off-by: Douglas Anderson <dianders@xxxxxxxxxxxx>
> ---
> 
>  drivers/tty/serial/qcom_geni_serial.c | 55 +++++++++++++--------------
>  1 file changed, 26 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> index 29ec34387246..99103c67e1dc 100644
> --- a/drivers/tty/serial/qcom_geni_serial.c
> +++ b/drivers/tty/serial/qcom_geni_serial.c
> @@ -851,6 +851,23 @@ static int qcom_geni_serial_port_setup(struct uart_port *uport)
>  {
>  	struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
>  	unsigned int rxstale = DEFAULT_BITS_PER_CHAR * STALE_TIMEOUT;
> +	u32 proto;
> +
> +	if (uart_console(uport))
> +		port->tx_bytes_pw = 1;
> +	else
> +		port->tx_bytes_pw = 4;
> +	port->rx_bytes_pw = RX_BYTES_PW;
> +
> +	proto = geni_se_read_proto(&port->se);
> +	if (proto != GENI_SE_UART) {
> +		dev_err(uport->dev, "Invalid FW loaded, proto: %d\n", proto);
> +		return -ENXIO;
> +	}
> +
> +	qcom_geni_serial_stop_rx(uport);

This wasn't done earlier in qcom_geni_serial_startup() (but it was in
qcom_geni_console_setup()). I guess this extra stop of RX for the
'uart' shouldn't do any harm.

> +
> +	get_tx_fifo_size(port);
>  
>  	set_rfr_wm(port);
>  	writel_relaxed(rxstale, uport->membase + SE_UART_RX_STALE_CNT);
> @@ -874,30 +891,19 @@ static int qcom_geni_serial_port_setup(struct uart_port *uport)
>  			return -ENOMEM;
>  	}
>  	port->setup = true;
> +
>  	return 0;
>  }
>  
>  static int qcom_geni_serial_startup(struct uart_port *uport)
>  {
>  	int ret;
> -	u32 proto;
>  	struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
>  
>  	scnprintf(port->name, sizeof(port->name),
>  		  "qcom_serial_%s%d",
>  		(uart_console(uport) ? "console" : "uart"), uport->line);
>  
> -	if (!uart_console(uport)) {
> -		port->tx_bytes_pw = 4;
> -		port->rx_bytes_pw = RX_BYTES_PW;
> -	}
> -	proto = geni_se_read_proto(&port->se);
> -	if (proto != GENI_SE_UART) {
> -		dev_err(uport->dev, "Invalid FW loaded, proto: %d\n", proto);
> -		return -ENXIO;
> -	}
> -
> -	get_tx_fifo_size(port);
>  	if (!port->setup) {
>  		ret = qcom_geni_serial_port_setup(uport);
>  		if (ret)
> @@ -1056,6 +1062,7 @@ static int __init qcom_geni_console_setup(struct console *co, char *options)
>  	int bits = 8;
>  	int parity = 'n';
>  	int flow = 'n';
> +	int ret;
>  
>  	if (co->index >= GENI_UART_CONS_PORTS  || co->index < 0)
>  		return -ENXIO;
> @@ -1071,21 +1078,10 @@ static int __init qcom_geni_console_setup(struct console *co, char *options)
>  	if (unlikely(!uport->membase))
>  		return -ENXIO;
>  
> -	if (geni_se_resources_on(&port->se)) {
> -		dev_err(port->se.dev, "Error turning on resources\n");
> -		return -ENXIO;
> -	}
> -
> -	if (unlikely(geni_se_read_proto(&port->se) != GENI_SE_UART)) {
> -		geni_se_resources_off(&port->se);
> -		return -ENXIO;
> -	}
> -
>  	if (!port->setup) {
> -		port->tx_bytes_pw = 1;
> -		port->rx_bytes_pw = RX_BYTES_PW;
> -		qcom_geni_serial_stop_rx(uport);
> -		qcom_geni_serial_port_setup(uport);
> +		ret = qcom_geni_serial_port_setup(uport);
> +		if (ret)
> +			return ret;
>  	}
>  
>  	if (options)
> @@ -1203,11 +1199,12 @@ static void qcom_geni_serial_pm(struct uart_port *uport,
>  {
>  	struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
>  
> +	/* If we've never been called, treat it as off */
> +	if (old_state == UART_PM_STATE_UNDEFINED)
> +		old_state = UART_PM_STATE_OFF;
> +
>  	if (new_state == UART_PM_STATE_ON && old_state == UART_PM_STATE_OFF)
>  		geni_se_resources_on(&port->se);
> -	else if (!uart_console(uport) && (new_state == UART_PM_STATE_ON &&
> -				old_state == UART_PM_STATE_UNDEFINED))
> -		geni_se_resources_on(&port->se);
>  	else if (new_state == UART_PM_STATE_OFF &&
>  			old_state == UART_PM_STATE_ON)
>  		geni_se_resources_off(&port->se);

Seems like a good refactoring, besides fixing the problem when booting
without 'console=ttyMSMx'.

Reviewed-by: Matthias Kaehlcke <mka@xxxxxxxxxxxx>