* Re: [PATCH] serial: msm: Configure correct working mode before starting earlycon
2025-04-08 17:22 [PATCH] serial: msm: Configure correct working mode before starting earlycon Stephan Gerhold
@ 2025-04-09 10:00 ` Mukesh Kumar Savaliya
2025-04-09 12:36 ` Stephan Gerhold
2025-04-09 12:37 ` neil.armstrong
2025-04-09 18:11 ` Stephen Boyd
2 siblings, 1 reply; 6+ messages in thread
From: Mukesh Kumar Savaliya @ 2025-04-09 10:00 UTC (permalink / raw)
To: Stephan Gerhold, Greg Kroah-Hartman, Jiri Slaby
Cc: Stephen Boyd, linux-arm-msm, linux-kernel, linux-serial, Sam Day,
Chandana Kishori Chiluveru
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,
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] serial: msm: Configure correct working mode before starting earlycon
2025-04-09 10:00 ` Mukesh Kumar Savaliya
@ 2025-04-09 12:36 ` Stephan Gerhold
0 siblings, 0 replies; 6+ messages in thread
From: Stephan Gerhold @ 2025-04-09 12:36 UTC (permalink / raw)
To: Mukesh Kumar Savaliya
Cc: Greg Kroah-Hartman, Jiri Slaby, Stephen Boyd, linux-arm-msm,
linux-kernel, linux-serial, Sam Day, Chandana Kishori Chiluveru
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] serial: msm: Configure correct working mode before starting earlycon
2025-04-08 17:22 [PATCH] serial: msm: Configure correct working mode before starting earlycon Stephan Gerhold
2025-04-09 10:00 ` Mukesh Kumar Savaliya
@ 2025-04-09 12:37 ` neil.armstrong
2025-04-09 18:11 ` Stephen Boyd
2 siblings, 0 replies; 6+ messages in thread
From: neil.armstrong @ 2025-04-09 12:37 UTC (permalink / raw)
To: Stephan Gerhold, Greg Kroah-Hartman, Jiri Slaby
Cc: Stephen Boyd, linux-arm-msm, linux-kernel, linux-serial, Sam Day
On 08/04/2025 19:22, 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.
>
> 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(+)
>
> 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,
I think it's safe the sanitize the mode in earlycon.
Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>
Thanks
Neil
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] serial: msm: Configure correct working mode before starting earlycon
2025-04-08 17:22 [PATCH] serial: msm: Configure correct working mode before starting earlycon Stephan Gerhold
2025-04-09 10:00 ` Mukesh Kumar Savaliya
2025-04-09 12:37 ` neil.armstrong
@ 2025-04-09 18:11 ` Stephen Boyd
2025-04-09 19:01 ` Stephan Gerhold
2 siblings, 1 reply; 6+ messages in thread
From: Stephen Boyd @ 2025-04-09 18:11 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby, Stephan Gerhold
Cc: linux-arm-msm, linux-kernel, linux-serial, Sam Day
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] serial: msm: Configure correct working mode before starting earlycon
2025-04-09 18:11 ` Stephen Boyd
@ 2025-04-09 19:01 ` Stephan Gerhold
0 siblings, 0 replies; 6+ messages in thread
From: Stephan Gerhold @ 2025-04-09 19:01 UTC (permalink / raw)
To: Stephen Boyd
Cc: Greg Kroah-Hartman, Jiri Slaby, linux-arm-msm, linux-kernel,
linux-serial, Sam Day
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
^ permalink raw reply [flat|nested] 6+ messages in thread