serial: msm: Configure correct working mode before starting earlycon

The MSM UART DM controller supports different working modes, e.g. DMA or
the "single-character mode", where all reads/writes operate on a single
character rather than 4 chars (32-bit) at once. When using earlycon,
__msm_console_write() always writes 4 characters at a time, but we don't
know which mode the bootloader was using and we don't set the mode either.

This causes garbled output if the bootloader was using the single-character
mode, because only every 4th character appears in the serial console, e.g.

  "[ 00oni pi  000xf0[ 00i s 5rm9(l)l s 1  1 SPMTA 7:C 5[ 00A ade k d[
   00ano:ameoi .Q1B[ 00ac _idaM00080oo'"

If the bootloader was using the DMA ("DM") mode, output would likely fail
entirely. Later, when the full serial driver probes, the port is
re-initialized and output works as expected.

Fix this also for earlycon by clearing the DMEN register and
reset+re-enable the transmitter to apply the change. This ensures the
transmitter is in the expected state before writing any output.

Cc: stable@vger.kernel.org
Fixes: 0efe72963409 ("tty: serial: msm: Add earlycon support")
Signed-off-by: Stephan Gerhold <stephan.gerhold@linaro.org>
---
 drivers/tty/serial/msm_serial.c | 6 ++++++
 1 file changed, 6 insertions(+)


---
base-commit: 0af2f6be1b4281385b618cb86ad946eded089ac8
change-id: 20250408-msm-serial-earlycon-4c5dfa782496

Best regards,

On 4/8/2025 10:52 PM, Stephan Gerhold wrote:
> The MSM UART DM controller supports different working modes, e.g. DMA or
> the "single-character mode", where all reads/writes operate on a single
> character rather than 4 chars (32-bit) at once. When using earlycon,
> __msm_console_write() always writes 4 characters at a time, but we don't
> know which mode the bootloader was using and we don't set the mode either.
> 
Looks surprising. I haven't seen pre-kernel UART console ever works in 
DMA mode. It was always fixed to work in FIFO/PIO mode. From what i know.
> This causes garbled output if the bootloader was using the single-character
> mode, because only every 4th character appears in the serial console, e.g.
> 
>    "[ 00oni pi  000xf0[ 00i s 5rm9(l)l s 1  1 SPMTA 7:C 5[ 00A ade k d[
>     00ano:ameoi .Q1B[ 00ac _idaM00080oo'"
> 
> If the bootloader was using the DMA ("DM") mode, output would likely fail
> entirely. Later, when the full serial driver probes, the port is
> re-initialized and output works as expected.
> 
AFAIR, console UART was always configured in FIFO/PIO mode. For non 
Console application e.g. Bluetooth it works in DMA mode.
> Fix this also for earlycon by clearing the DMEN register and
> reset+re-enable the transmitter to apply the change. This ensures the
> transmitter is in the expected state before writing any output.
> 
> Cc: stable@vger.kernel.org
> Fixes: 0efe72963409 ("tty: serial: msm: Add earlycon support")
> Signed-off-by: Stephan Gerhold <stephan.gerhold@linaro.org>
> ---
>   drivers/tty/serial/msm_serial.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/tty/serial/msm_serial.c b/drivers/tty/serial/msm_serial.c
> index 1b137e06844425584afe5d3f647e9537c6e2d658..3449945493ceb42369d2acafca925350fccc4f82 100644
> --- a/drivers/tty/serial/msm_serial.c
> +++ b/drivers/tty/serial/msm_serial.c
> @@ -1746,6 +1746,12 @@ msm_serial_early_console_setup_dm(struct earlycon_device *device,
>   	if (!device->port.membase)
>   		return -ENODEV;
>   
> +	/* Disable DM / single-character modes */
> +	msm_write(&device->port, 0, UARTDM_DMEN);
> +	msm_write(&device->port, MSM_UART_CR_CMD_RESET_RX, MSM_UART_CR);
> +	msm_write(&device->port, MSM_UART_CR_CMD_RESET_TX, MSM_UART_CR);
> +	msm_write(&device->port, MSM_UART_CR_TX_ENABLE, MSM_UART_CR);
> +
>   	device->con->write = msm_serial_early_write_dm;
>   	return 0;
>   }
> 
> ---
> base-commit: 0af2f6be1b4281385b618cb86ad946eded089ac8
> change-id: 20250408-msm-serial-earlycon-4c5dfa782496
> 
> Best regards,

On Wed, Apr 09, 2025 at 03:30:02PM +0530, Mukesh Kumar Savaliya wrote:
> On 4/8/2025 10:52 PM, Stephan Gerhold wrote:
> > The MSM UART DM controller supports different working modes, e.g. DMA or
> > the "single-character mode", where all reads/writes operate on a single
> > character rather than 4 chars (32-bit) at once. When using earlycon,
> > __msm_console_write() always writes 4 characters at a time, but we don't
> > know which mode the bootloader was using and we don't set the mode either.
> > 
> Looks surprising. I haven't seen pre-kernel UART console ever works in DMA
> mode. It was always fixed to work in FIFO/PIO mode. From what i know.

I'm sure you're right, since it doesn't really make sense to implement
DMA mode for the UART console in earlier firmware or the bootloader.

DMA is just the side note here though. As I wrote in the patch
description, the real problem this patch fixes is the "single-character
mode". This is still FIFO/PIO mode, except that the register will
take/return just a single character rather than 4 chars at once.

This mode is used by firmware on some platforms and I'm planning to make
use of this mode in U-Boot to fix some edge cases. It's much more simple
to implement reliably for something minimal like U-Boot. With that
change in U-Boot, I get the garbled output shown below in the earlycon
serial console. This patch fixes it.

> > This causes garbled output if the bootloader was using the single-character
> > mode, because only every 4th character appears in the serial console, e.g.
> > 
> >    "[ 00oni pi  000xf0[ 00i s 5rm9(l)l s 1  1 SPMTA 7:C 5[ 00A ade k d[
> >     00ano:ameoi .Q1B[ 00ac _idaM00080oo'"
> > 
> > If the bootloader was using the DMA ("DM") mode, output would likely fail
> > entirely. Later, when the full serial driver probes, the port is
> > re-initialized and output works as expected.
> > 
> AFAIR, console UART was always configured in FIFO/PIO mode. For non Console
> application e.g. Bluetooth it works in DMA mode.

I think console UART in msm_serial does use DMA at least for receiving
at the moment, since we don't differentiate between "console" or
"non-console" use case in the upstream driver (as far as I can tell).
My patch doesn't change anything about this though, it just ensures the
UART controller is in the expected mode before starting earlycon.

Thanks,
Stephan

On Wed, Apr 09, 2025 at 11:11:00AM -0700, Stephen Boyd wrote:
> Quoting Stephan Gerhold (2025-04-08 10:22:47)
> > diff --git a/drivers/tty/serial/msm_serial.c b/drivers/tty/serial/msm_serial.c
> > index 1b137e06844425584afe5d3f647e9537c6e2d658..3449945493ceb42369d2acafca925350fccc4f82 100644
> > --- a/drivers/tty/serial/msm_serial.c
> > +++ b/drivers/tty/serial/msm_serial.c
> > @@ -1746,6 +1746,12 @@ msm_serial_early_console_setup_dm(struct earlycon_device *device,
> >         if (!device->port.membase)
> >                 return -ENODEV;
> >  
> > +       /* Disable DM / single-character modes */
> > +       msm_write(&device->port, 0, UARTDM_DMEN);
> > +       msm_write(&device->port, MSM_UART_CR_CMD_RESET_RX, MSM_UART_CR);
> > +       msm_write(&device->port, MSM_UART_CR_CMD_RESET_TX, MSM_UART_CR);
> > +       msm_write(&device->port, MSM_UART_CR_TX_ENABLE, MSM_UART_CR);
> 
> In msm_complete_tx_dma() these are under an if condition checking the
> version of uartdm. Do we need that here? Although I also see that
> MSM_UART_CR_CMD_RESET_TX is unconditionally written in msm_reset() but
> not MSM_UART_CR_TX_ENABLE so maybe the condition check is wrong or the
> bit doesn't exist in earlier versions of the hardware so it doesn't
> really matter.

msm_reset() is called from msm_set_baud_rate(), and that one does

  msm_write(port, MSM_UART_CR_TX_ENABLE | MSM_UART_CR_RX_ENABLE, MSM_UART_CR);

unconditionally immediately after, so what I'm doing here matches what
the driver anyway does for all the IP versions.

I'm not sure why we have version checks to perform the reset/enable in
*some* code paths only for *some* IP versions. All of this feels
obsolete at this point, since "msm_port->is_uartdm > UARTDM_1P3" covers
all SoCs we still support upstream. There are no users of the v1.1 and
v1.2 compatibles upstream, even MSM8660 has v1.3 already. We might as
well drop those conditions at some point.

Even better, we still have code for some super old controller before all
the DM variants (qcom,msm-uart instead of qcom,msm-uart*dm*). No users
of that upstream either. :-)

Thanks,
Stephan