[PATCH v2 0/4] serial: 8250_dw: Allwinner A733 UART support

[PATCH v2 0/4] serial: 8250_dw: Allwinner A733 UART support

[Vinicius Pedrosa]

While testing the A733 UART, the console stopped taking interrupts at
the first termios change from userspace. Termios changes during console
output could also corrupt characters even when the baud rate did not
change.

Patch 1 is an independent fix for the DesignWare 8250 divisor path.
When DLF is detected, dw8250_setup_port() can replace the BUSY-safe
divisor hook with one that does not provide that protection. If the LCR
write is then dropped while the UART is busy, the retry path can try to
restore IER while DLAB is still selected, causing the value to be
written to DLH instead of IER.

Patch 2 takes Andre's suggestion to only accept a DLF readback when it
is a contiguous 2^n - 1 mask. On the A733, offset 0xc0 is the RS485
control register and reads back 0x19f; Andre observed 0x9f on the A523.

Patches 3 and 4 add the A733 compatible and select SKIP_SET_RATE for
it. Stopping the UART clock from set_termios while TX is active corrupts
output on this board. Skipping redundant reclocking in general was tried
in e5d6bd25f93d and reverted by 7dfae6cbadc1 after regressions on Bay
Trail and Cherry Trail, so the change stays specific to the A733.

Tested on a Radxa Cubie A7Z (PCB V1.11, 1 GiB) on top of tty-next
1184f87e2790e8af9be3116f608d2e86b4792d7d, using local A733 DT and
clock changes that are not part of this series.

With patch 1 alone and the generic fallback, repeated termios changes
no longer stalled the console and hardware IER remained equal to
up->ier, while the false DLF was still detected (dlf_size=9).

With the complete series and the A733 compatible, three clean runs of
both the CLOCAL and CSTOPB stress tests completed without corrupted
output. dlf_size remained 0 and offset 0xc0 remained 0 after the tests.
dlf_size and up->ier were read through a temporary debugfs test patch
which is not part of this series.

I do not have hardware with a real DLF, or Bay Trail/Cherry Trail, to
test those paths.

Changes in v2:
- rework patch 1 around a dwlib divisor helper and preserve an existing
  set_divisor hook, following Ilpo's review
- add a separate check for contiguous DLF readback, following Andre's
  suggestion
- drop the A733-specific NO_DLF quirk
- keep the generic DT fallback
- keep only SKIP_SET_RATE in the A733 platform data
- retain Conor's Acked-by on the binding
- rebase and retest on tty-next 1184f87e2790

Link to v1:
https://lore.kernel.org/all/20261005172538.398522-1-vinicius.eduardo.pedrosa@gmail.com/

Vinicius Pedrosa (4):
  serial: 8250_dw: Keep the BUSY-safe divisor hook when DLF is present
  serial: 8250_dw: Only accept a contiguous DLF readback
  dt-bindings: serial: snps-dw-apb-uart: Add Allwinner A733
  serial: 8250_dw: Add Allwinner A733 UART

 .../bindings/serial/snps-dw-apb-uart.yaml     |  1 +
 drivers/tty/serial/8250/8250_dw.c             |  3 +++
 drivers/tty/serial/8250/8250_dwlib.c          | 22 ++++++++++++++++---
 drivers/tty/serial/8250/8250_dwlib.h          |  1 +
 4 files changed, 24 insertions(+), 3 deletions(-)


base-commit: 1184f87e2790e8af9be3116f608d2e86b4792d7d

[PATCH v2 1/4] serial: 8250_dw: Keep the BUSY-safe divisor hook when DLF is present

[Vinicius Pedrosa]

dw8250_probe() installs dw8250_set_divisor(), which brackets the
DLAB/DLL/DLH sequence with dw8250_idle_enter() and dw8250_idle_exit()
so a BUSY UART cannot drop the divisor LCR write.

dw8250_setup_port() runs afterwards and replaces that hook with the
dwlib divisor hook whenever it detects DLF. The dwlib hook programs the
divisor through serial8250_do_set_divisor() without entering idle,
leaving the LCR|DLAB write exposed to BUSY.

When termios changes while the UART is transmitting, that LCR|DLAB
write can be ignored and dw8250_check_lcr() retries it between
dw8250_idle_enter() and dw8250_idle_exit(). dw8250_idle_exit() then
restores IER while DLAB is still set, so the value lands in DLH. The
real IER stays at the zero written by dw8250_idle_enter(), and the port
gets no further TX or RX interrupts while up->ier says they're enabled.

Do not replace a divisor hook the driver has already installed. Program
DLF from the BUSY-safe hook, inside the idle section, through a dwlib
helper that checks dlf_size.

Observed on an Allwinner A733 (Radxa Cubie A7Z), where
dw8250_setup_port() mistakes the RS485 control register for DLF (see the
next patch): the first interactive tcsetattr() during console output
left hardware IER at 0 and up->ier at 0x07. The shell never printed its
prompt or read input. With this change alone the same boot reached the
prompt and echoed input, and hardware IER matched up->ier afterwards.

Fixes: a7b9ce39fbe4 ("serial: 8250_dw: Ensure BUSY is deasserted")
Cc: stable@vger.kernel.org
Signed-off-by: Vinicius Pedrosa <vinicius.eduardo.pedrosa@gmail.com>
---
 drivers/tty/serial/8250/8250_dw.c    |  2 ++
 drivers/tty/serial/8250/8250_dwlib.c | 16 ++++++++++++++--
 drivers/tty/serial/8250/8250_dwlib.h |  1 +
 3 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index eca02e8bd6d1..4c86aaf9c121 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -197,6 +197,8 @@ static void dw8250_set_divisor(struct uart_port *p, unsigned int baud,
 	if (ret < 0)
 		return;
 
+	__dw8250_set_divisor(p, quot_frac);
+
 	serial_port_out(p, UART_LCR, up->lcr | UART_LCR_DLAB);
 	if (!(serial_port_in(p, UART_LCR) & UART_LCR_DLAB))
 		goto idle_failed;
diff --git a/drivers/tty/serial/8250/8250_dwlib.c b/drivers/tty/serial/8250/8250_dwlib.c
index 9bb02a4ab11f..325793f20256 100644
--- a/drivers/tty/serial/8250/8250_dwlib.c
+++ b/drivers/tty/serial/8250/8250_dwlib.c
@@ -36,10 +36,20 @@ static unsigned int dw8250_get_divisor(struct uart_port *p, unsigned int baud,
 	return quot;
 }
 
+/* Program the fractional part of the divisor, if the UART has one */
+void __dw8250_set_divisor(struct uart_port *p, unsigned int quot_frac)
+{
+	struct dw8250_port_data *d = p->private_data;
+
+	if (d->dlf_size)
+		dw8250_writel_ext(p, DW_UART_DLF, quot_frac);
+}
+EXPORT_SYMBOL_GPL(__dw8250_set_divisor);
+
 static void dw8250_set_divisor(struct uart_port *p, unsigned int baud,
 			       unsigned int quot, unsigned int quot_frac)
 {
-	dw8250_writel_ext(p, DW_UART_DLF, quot_frac);
+	__dw8250_set_divisor(p, quot_frac);
 	serial8250_do_set_divisor(p, baud, quot);
 }
 
@@ -218,7 +228,9 @@ void dw8250_setup_port(struct uart_port *p)
 	if (reg) {
 		pd->dlf_size = fls(reg);
 		p->get_divisor = dw8250_get_divisor;
-		p->set_divisor = dw8250_set_divisor;
+		/* Keep a hook the driver has already installed */
+		if (!p->set_divisor)
+			p->set_divisor = dw8250_set_divisor;
 	}
 
 	reg = dw8250_readl_ext(p, DW_UART_UCV);
diff --git a/drivers/tty/serial/8250/8250_dwlib.h b/drivers/tty/serial/8250/8250_dwlib.h
index 39fcb1b323ad..4bb0ccb3e8ef 100644
--- a/drivers/tty/serial/8250/8250_dwlib.h
+++ b/drivers/tty/serial/8250/8250_dwlib.h
@@ -99,6 +99,7 @@ struct dw8250_port_data {
 
 void dw8250_do_set_termios(struct uart_port *p, struct ktermios *termios, const struct ktermios *old);
 void dw8250_setup_port(struct uart_port *p);
+void __dw8250_set_divisor(struct uart_port *p, unsigned int quot_frac);
 
 static inline u32 dw8250_readl_ext(struct uart_port *p, int offset)
 {

[PATCH v2 2/4] serial: 8250_dw: Only accept a contiguous DLF readback

[Vinicius Pedrosa]

dw8250_setup_port() writes all ones to offset 0xc0 and treats any
nonzero readback as the DLF register. DLF is a bit field starting at
bit 0, so a real one reads back as a contiguous 2^n - 1 mask. On some
Allwinner SoCs offset 0xc0 is the RS485 control register instead
(UART_485_CTL in the A733 User Manual), whose implemented bits are not
contiguous. The probe still takes its readback for a DLF of fls(readback)
bits, and every later divisor change writes the fractional divisor into
the RS485 control register.

Only accept a readback of the form 2^n - 1.

The probe itself is unchanged: it still writes all ones to offset 0xc0
and then restores the old value, so this only changes how the readback
is interpreted.

The A733 reads back 0x19f, and Andre Przywara observed 0x9f on the
A523. Without this check the A733 RS485 control register reads 0xb after
boot; with it, the register stays 0 across 300 termios toggle and
restore cycles.

Suggested-by: Andre Przywara <andre.przywara@arm.com>
Signed-off-by: Vinicius Pedrosa <vinicius.eduardo.pedrosa@gmail.com>
---
 drivers/tty/serial/8250/8250_dwlib.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/8250/8250_dwlib.c b/drivers/tty/serial/8250/8250_dwlib.c
index 325793f20256..1b0192efe162 100644
--- a/drivers/tty/serial/8250/8250_dwlib.c
+++ b/drivers/tty/serial/8250/8250_dwlib.c
@@ -225,7 +225,11 @@ void dw8250_setup_port(struct uart_port *p)
 	reg = dw8250_readl_ext(p, DW_UART_DLF);
 	dw8250_writel_ext(p, DW_UART_DLF, old_dlf);
 
-	if (reg) {
+	/*
+	 * DLF is a bit field starting at bit 0, so a real one reads back as
+	 * 2^n - 1. Any other value means this offset is something else.
+	 */
+	if (reg && !(reg & (reg + 1))) {
 		pd->dlf_size = fls(reg);
 		p->get_divisor = dw8250_get_divisor;
 		/* Keep a hook the driver has already installed */

[PATCH v2 3/4] dt-bindings: serial: snps-dw-apb-uart: Add Allwinner A733

[Vinicius Pedrosa]

The Allwinner A733 UART is a DesignWare APB UART, but it needs
SoC-specific handling so the driver can avoid stopping its clock during
termios changes.

Add an A733-specific compatible, with "snps,dw-apb-uart" as the
fallback.

Signed-off-by: Vinicius Pedrosa <vinicius.eduardo.pedrosa@gmail.com>
Acked-by: Conor Dooley <conor.dooley@microchip.com>
---
 Documentation/devicetree/bindings/serial/snps-dw-apb-uart.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/serial/snps-dw-apb-uart.yaml b/Documentation/devicetree/bindings/serial/snps-dw-apb-uart.yaml
index e2ca126d58ba..532392dd54cf 100644
--- a/Documentation/devicetree/bindings/serial/snps-dw-apb-uart.yaml
+++ b/Documentation/devicetree/bindings/serial/snps-dw-apb-uart.yaml
@@ -60,6 +60,7 @@ properties:
           - const: renesas,rzn1-uart
       - items:
           - enum:
+              - allwinner,sun60i-a733-uart
               - anlogic,dr1v90-uart
               - brcm,bcm11351-dw-apb-uart
               - brcm,bcm21664-dw-apb-uart

[PATCH v2 4/4] serial: 8250_dw: Add Allwinner A733 UART

[Vinicius Pedrosa]

On the A733, dw8250_set_termios() stops the UART clock around
clk_set_rate() on every termios change that reaches the driver. If TX is
active at that moment, the character being shifted out is corrupted.

No baud rate change is required to trigger the problem. Toggling only
CLOCAL or CSTOPB 300 times during continuous console output corrupted
bytes in every run without the quirk and in none with it.

Use the existing SKIP_SET_RATE quirk, as the JH7100 and SG2044 UARTs do.

Signed-off-by: Vinicius Pedrosa <vinicius.eduardo.pedrosa@gmail.com>
---
 drivers/tty/serial/8250/8250_dw.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index 4c86aaf9c121..718ec44135e4 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -906,6 +906,7 @@ static const struct dw8250_platform_data dw8250_tda54 = {
 
 static const struct of_device_id dw8250_of_match[] = {
 	{ .compatible = "snps,dw-apb-uart", .data = &dw8250_dw_apb },
+	{ .compatible = "allwinner,sun60i-a733-uart", .data = &dw8250_skip_set_rate_data },
 	{ .compatible = "cavium,octeon-3860-uart", .data = &dw8250_octeon_3860_data },
 	{ .compatible = "marvell,armada-38x-uart", .data = &dw8250_armada_38x_data },
 	{ .compatible = "renesas,rzn1-uart", .data = &dw8250_renesas_rzn1_data },

Re: [PATCH v2 2/4] serial: 8250_dw: Only accept a contiguous DLF readback

[Jiri Slaby]

On 19. 10. 26, 22:06, Vinicius Pedrosa wrote:
> --- a/drivers/tty/serial/8250/8250_dwlib.c
> +++ b/drivers/tty/serial/8250/8250_dwlib.c
> @@ -225,7 +225,11 @@ void dw8250_setup_port(struct uart_port *p)
>   	reg = dw8250_readl_ext(p, DW_UART_DLF);
>   	dw8250_writel_ext(p, DW_UART_DLF, old_dlf);
>   
> -	if (reg) {
> +	/*
> +	 * DLF is a bit field starting at bit 0, so a real one reads back as
> +	 * 2^n - 1. Any other value means this offset is something else.
> +	 */
> +	if (reg && !(reg & (reg + 1))) {

Do I understand correctly, that this equivalent to:
   if (is_power_of_2(reg + 1))
?

thanks,
-- 
js
suse labs

Re: [PATCH v2 1/4] serial: 8250_dw: Keep the BUSY-safe divisor hook when DLF is present

[Andy Shevchenko]

On Mon, Oct 19, 2026 at 05:06:10PM -0300, Vinicius Pedrosa wrote:
> dw8250_probe() installs dw8250_set_divisor(), which brackets the
> DLAB/DLL/DLH sequence with dw8250_idle_enter() and dw8250_idle_exit()
> so a BUSY UART cannot drop the divisor LCR write.
> 
> dw8250_setup_port() runs afterwards and replaces that hook with the
> dwlib divisor hook whenever it detects DLF. The dwlib hook programs the
> divisor through serial8250_do_set_divisor() without entering idle,
> leaving the LCR|DLAB write exposed to BUSY.
> 
> When termios changes while the UART is transmitting, that LCR|DLAB
> write can be ignored and dw8250_check_lcr() retries it between
> dw8250_idle_enter() and dw8250_idle_exit(). dw8250_idle_exit() then
> restores IER while DLAB is still set, so the value lands in DLH. The
> real IER stays at the zero written by dw8250_idle_enter(), and the port
> gets no further TX or RX interrupts while up->ier says they're enabled.
> 
> Do not replace a divisor hook the driver has already installed. Program
> DLF from the BUSY-safe hook, inside the idle section, through a dwlib
> helper that checks dlf_size.
> 
> Observed on an Allwinner A733 (Radxa Cubie A7Z), where
> dw8250_setup_port() mistakes the RS485 control register for DLF (see the
> next patch): the first interactive tcsetattr() during console output
> left hardware IER at 0 and up->ier at 0x07. The shell never printed its
> prompt or read input. With this change alone the same boot reached the
> prompt and echoed input, and hardware IER matched up->ier afterwards.

...

> +/* Program the fractional part of the divisor, if the UART has one */
> +void __dw8250_set_divisor(struct uart_port *p, unsigned int quot_frac)

Let's reduce amount of double underscore functions. Let's call it
dw8250_update_dfl(). Also call the parameter just 'frac'.

void dw8250_update_dfl(struct uart_port *p, unsigned int frac)

...

> +++ b/drivers/tty/serial/8250/8250_dwlib.h

>  void dw8250_do_set_termios(struct uart_port *p, struct ktermios *termios, const struct ktermios *old);
>  void dw8250_setup_port(struct uart_port *p);
> +void __dw8250_set_divisor(struct uart_port *p, unsigned int quot_frac);

Put it rather after _writel_ext() implementation, because it's just a small
wrapper on top of it. Or even make it 'static inline' there.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/4] serial: 8250_dw: Only accept a contiguous DLF readback

[Andy Shevchenko]

On Tue, Oct 20, 2026 at 08:11:06AM +0200, Jiri Slaby wrote:
> On 19. 10. 26, 22:06, Vinicius Pedrosa wrote:

...

> > -	if (reg) {
> > +	/*
> > +	 * DLF is a bit field starting at bit 0, so a real one reads back as
> > +	 * 2^n - 1. Any other value means this offset is something else.
> > +	 */
> > +	if (reg && !(reg & (reg + 1))) {
> 
> Do I understand correctly, that this equivalent to:
>   if (is_power_of_2(reg + 1))
> ?

Yes, and since we check for reg to be "not zero", this will work the rest.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/4] serial: 8250_dw: Keep the BUSY-safe divisor hook when DLF is present

[Andy Shevchenko]

On Mon, Oct 19, 2026 at 05:06:10PM -0300, Vinicius Pedrosa wrote:

...

> @@ -218,7 +228,9 @@ void dw8250_setup_port(struct uart_port *p)
>  	if (reg) {
>  		pd->dlf_size = fls(reg);
>  		p->get_divisor = dw8250_get_divisor;

In the case of existing _set_divisor, do we want to keep the possible set
_get_divisor as well? Shouldn't we introduce a respective _get_divisor in 8250_dw.c?

> -		p->set_divisor = dw8250_set_divisor;
> +		/* Keep a hook the driver has already installed */
> +		if (!p->set_divisor)
> +			p->set_divisor = dw8250_set_divisor;
>  	}

On top of my previous reply, why is this hunk needed?

...

But now I see that we have same function name in two files. Can we rename those
in dwlib to, for example,
dw8250_get_divisor --> dw8250_get_dfl_divisor
dw8250_set_divisor --> dw8250_set_dfl_divisor

?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/4] serial: 8250_dw: Only accept a contiguous DLF readback

[Vinicius Pedrosa]

Hi Jiri,

On Tue, 20 Oct 2026, Jiri Slaby wrote:

> Do I understand correctly, that this equivalent to:
>   if (is_power_of_2(reg + 1))
> ?

Yes, given the existing reg != 0 check. I kept the explicit form in v2
to keep the change minimal, but I'll use

  reg && is_power_of_2(reg + 1)

in v3, it reads better.

The two forms only differ for U32_MAX, where reg + 1 wraps to 0. But that
isn't a valid DLF readback here.

Thanks,
Vinicius

Re: [PATCH v2 1/4] serial: 8250_dw: Keep the BUSY-safe divisor hook when DLF is present

[Vinicius Pedrosa]

Hi Andy,

On Tue, 20 Oct 2026, Andy Shevchenko wrote:

> > +		/* Keep a hook the driver has already installed */
> > +		if (!p->set_divisor)
> > +			p->set_divisor = dw8250_set_divisor;
>
> On top of my previous reply, why is this hunk needed?

dw8250_probe() installs the BUSY-safe dw8250_set_divisor() before
dw8250_setup_port() runs. Without the !p->set_divisor check, detecting
DLF replaces it with the dwlib setter, which is the BUSY problem
described in the commit message.

> In the case of existing _set_divisor, do we want to keep the possible set
> _get_divisor as well? Shouldn't we introduce a respective _get_divisor in
> 8250_dw.c?

8250_dw.c doesn't install a get_divisor, so there's no existing getter
to preserve. The dwlib getter is still needed when DLF is present,
because it computes the fractional part used by the preserved BUSY-safe
setter.

> But now I see that we have same function name in two files. Can we rename
> those in dwlib to, for example,
> dw8250_get_divisor --> dw8250_get_dfl_divisor
> dw8250_set_divisor --> dw8250_set_dfl_divisor

I'll also clean up the naming in v3: dw8250_update_dlf() with a frac
parameter, static inline after dw8250_writel_ext(), and the dwlib-local
helpers renamed to dw8250_get_dlf_divisor() and
dw8250_set_dlf_divisor().

Thanks,
Vinicius

Re: [PATCH v2 2/4] serial: 8250_dw: Only accept a contiguous DLF readback

[Ilpo Järvinen]

On Tue, 20 Oct 2026, Andy Shevchenko wrote:

> On Tue, Oct 20, 2026 at 08:11:06AM +0200, Jiri Slaby wrote:
> > On 19. 10. 26, 22:06, Vinicius Pedrosa wrote:
> 
> ...
> 
> > > -	if (reg) {
> > > +	/*
> > > +	 * DLF is a bit field starting at bit 0, so a real one reads back as
> > > +	 * 2^n - 1. Any other value means this offset is something else.
> > > +	 */
> > > +	if (reg && !(reg & (reg + 1))) {
> > 
> > Do I understand correctly, that this equivalent to:
> >   if (is_power_of_2(reg + 1))
> > ?
> 
> Yes, and since we check for reg to be "not zero", this will work the rest.

For is_power_of_2(), 0 is not a power of two. And it also wouldn't make 
sense to have zero bits in the bit field as it's same as no bit field so 
using just is_power_of_2() should be enough.

It could be that's what Andy meant, but the wording left too much room 
for interpretation.

-- 
 i.

Re: [PATCH v2 2/4] serial: 8250_dw: Only accept a contiguous DLF readback

[Vinicius Pedrosa]

Hi Ilpo,

On Tue, 20 Oct 2026, Ilpo Järvinen wrote:

> For is_power_of_2(), 0 is not a power of two. And it also wouldn't make
> sense to have zero bits in the bit field as it's same as no bit field so
> using just is_power_of_2() should be enough.

I think the reg != 0 check still needs to stay here, since the test is
is_power_of_2(reg + 1), rather than is_power_of_2(reg).

For reg == 0, reg + 1 is 1, and is_power_of_2(1) is true. So without
the explicit reg check, a zero readback, meaning no DLF, would be
accepted as a valid DLF.

I'll keep it as:

    reg && is_power_of_2(reg + 1)

in v3.

Thanks,
Vinicius

Re: [PATCH v2 2/4] serial: 8250_dw: Only accept a contiguous DLF readback

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 741 bytes --]

On Tue, 20 Oct 2026, Vinicius Pedrosa wrote:

> Hi Ilpo,
> 
> On Tue, 20 Oct 2026, Ilpo Järvinen wrote:
> 
> > For is_power_of_2(), 0 is not a power of two. And it also wouldn't make
> > sense to have zero bits in the bit field as it's same as no bit field so
> > using just is_power_of_2() should be enough.
> 
> I think the reg != 0 check still needs to stay here, since the test is
> is_power_of_2(reg + 1), rather than is_power_of_2(reg).
> 
> For reg == 0, reg + 1 is 1, and is_power_of_2(1) is true. So without
> the explicit reg check, a zero readback, meaning no DLF, would be
> accepted as a valid DLF.
> 
> I'll keep it as:
> 
>     reg && is_power_of_2(reg + 1)
> 
> in v3.

Ah, you're right.

-- 
 i.