[3/3] serial: 8250_dw: Add Allwinner A733 UART

The A733 UART is clocked from its bus clock gate, which can't change
rate. dw8250_set_termios() still gates it around a no-op clk_set_rate()
on every termios change. That stalls the character being shifted out
and corrupts it on the wire. Use the existing SKIP_SET_RATE quirk, as
other SoCs with a fixed UART clock do.

Offset 0xc0 is the RS485 control register on this SoC (A733 User
Manual, UART_485_CTL), not DLF. dw8250_setup_port() writes all ones
there, reads back a nonzero 9-bit value and takes it for a 9-bit DLF.
Every later divisor change then writes the fractional divisor into the
RS485 control register. Add a NO_DLF quirk so dwlib skips that probe.

Signed-off-by: Vinicius Pedrosa <vinicius.eduardo.pedrosa@gmail.com>
---
 drivers/tty/serial/8250/8250_dw.c    | 13 +++++++++++++
 drivers/tty/serial/8250/8250_dwlib.c | 22 ++++++++++++----------
 drivers/tty/serial/8250/8250_dwlib.h |  1 +
 3 files changed, 26 insertions(+), 10 deletions(-)
  

> The A733 UART is clocked from its bus clock gate, which can't change
> rate. dw8250_set_termios() still gates it around a no-op clk_set_rate()
> on every termios change. That stalls the character being shifted out
> and corrupts it on the wire. Use the existing SKIP_SET_RATE quirk, as
> other SoCs with a fixed UART clock do.
> 
> Offset 0xc0 is the RS485 control register on this SoC (A733 User
> Manual, UART_485_CTL), not DLF. dw8250_setup_port() writes all ones
> there, reads back a nonzero 9-bit value and takes it for a 9-bit DLF.
> Every later divisor change then writes the fractional divisor into the
> RS485 control register. Add a NO_DLF quirk so dwlib skips that probe.
> 
> Signed-off-by: Vinicius Pedrosa <vinicius.eduardo.pedrosa@gmail.com>

Sashiko has reviewed this patch and found no issues. It looks great!
  

Hi Vinicius,

On 10/5/26 19:25, Vinicius Pedrosa wrote:
> The A733 UART is clocked from its bus clock gate, which can't change
> rate. dw8250_set_termios() still gates it around a no-op clk_set_rate()
> on every termios change. That stalls the character being shifted out
> and corrupts it on the wire. Use the existing SKIP_SET_RATE quirk, as
> other SoCs with a fixed UART clock do.
> 
> Offset 0xc0 is the RS485 control register on this SoC (A733 User
> Manual, UART_485_CTL), not DLF. dw8250_setup_port() writes all ones
> there, reads back a nonzero 9-bit value and takes it for a 9-bit DLF.
> Every later divisor change then writes the fractional divisor into the
> RS485 control register. Add a NO_DLF quirk so dwlib skips that probe.

many thanks for digging into this, and superficially it looks like the 
right thing to do. But looking into some manuals, it seems like 
Allwinner added this RS485 support on registers +0xc0, 0xc4 and 0xc8 
already a while ago: I find it in the H6, H616, A523 manuals, for 
instance. And while register 0x50004c0 ignores writes on the H616, the 
corresponding 0x25004c0 reacts on the A523, so at least that one is 
already broken.
So as Sashiko mentioned, we would technically need to drop the fallback 
compatible, and doing this also for the A523 would break all older 
kernels, so that isn't a good option.

Now when I write 0xffffffff into +0xc0 on the A523, I read 0x9f back, so 
there are zero bits in the middle, which doesn't match the expected 
contiguous bitmask for the fractional divider bits.
So I was wondering if we should refine the DLF detection instead? Only 
when the readback from 0xc0 returns some 2^n-1 value we assume DLF is 
implemented?

Alternatively making DLF a (negative?) DT/ACPI property instead, and 
keep the fallback compatible?
I guess this all depends a bit on what the snps,dw-apb-uart compatible 
string really covers, and if Allwinner hacked^Wchanged the IP, by adding 
their RS485 bits on top of on older Designware IP, or if this is a valid 
configuration.

Cheers,
Andre

> Signed-off-by: Vinicius Pedrosa <vinicius.eduardo.pedrosa@gmail.com>
> ---
>   drivers/tty/serial/8250/8250_dw.c    | 13 +++++++++++++
>   drivers/tty/serial/8250/8250_dwlib.c | 22 ++++++++++++----------
>   drivers/tty/serial/8250/8250_dwlib.h |  1 +
>   3 files changed, 26 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
> index ba414306c98a..ea33aa644cda 100644
> --- a/drivers/tty/serial/8250/8250_dw.c
> +++ b/drivers/tty/serial/8250/8250_dw.c
> @@ -52,6 +52,7 @@
>   #define DW_UART_QUIRK_CPR_VALUE		BIT(5)
>   #define DW_UART_QUIRK_IER_KICK		BIT(6)
>   #define DW_UART_QUIRK_SKIP_EMPTY_FIFO_READ	BIT(7)
> +#define DW_UART_QUIRK_NO_DLF		BIT(8)
>   
>   /*
>    * Number of consecutive IIR_NO_INT interrupts required to trigger interrupt
> @@ -606,6 +607,8 @@ static void dw8250_quirks(struct uart_port *p, struct dw8250_data *data)
>   		p->serial_out = dw8250_serial_out38x;
>   	if (quirks & DW_UART_QUIRK_SKIP_SET_RATE)
>   		p->set_termios = dw8250_do_set_termios;
> +	if (quirks & DW_UART_QUIRK_NO_DLF)
> +		data->data.no_dlf = true;
>   	if (quirks & DW_UART_QUIRK_IS_DMA_FC) {
>   		data->data.dma.txconf.device_fc = 1;
>   		data->data.dma.rxconf.device_fc = 1;
> @@ -892,6 +895,15 @@ static const struct dw8250_platform_data dw8250_skip_set_rate_data = {
>   	.quirks = DW_UART_QUIRK_SKIP_SET_RATE,
>   };
>   
> +/*
> + * The baud clock is the bus clock gate, whose rate cannot change, and offset
> + * 0xc0 is the RS485 control register rather than DLF.
> + */
> +static const struct dw8250_platform_data dw8250_sun60i_a733_data = {
> +	.usr_reg = DW_UART_USR,
> +	.quirks = DW_UART_QUIRK_SKIP_SET_RATE | DW_UART_QUIRK_NO_DLF,
> +};
> +
>   static const struct dw8250_platform_data dw8250_intc10ee = {
>   	.usr_reg = DW_UART_USR,
>   	.quirks = DW_UART_QUIRK_IER_KICK,
> @@ -913,6 +925,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_sun60i_a733_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 },
> diff --git a/drivers/tty/serial/8250/8250_dwlib.c b/drivers/tty/serial/8250/8250_dwlib.c
> index 9bb02a4ab11f..9c6f3d926ad5 100644
> --- a/drivers/tty/serial/8250/8250_dwlib.c
> +++ b/drivers/tty/serial/8250/8250_dwlib.c
> @@ -209,16 +209,18 @@ void dw8250_setup_port(struct uart_port *p)
>   	}
>   	up->capabilities |= UART_CAP_NOTEMT;
>   
> -	/* Preserve value written by firmware or bootloader  */
> -	old_dlf = dw8250_readl_ext(p, DW_UART_DLF);
> -	dw8250_writel_ext(p, DW_UART_DLF, ~0U);
> -	reg = dw8250_readl_ext(p, DW_UART_DLF);
> -	dw8250_writel_ext(p, DW_UART_DLF, old_dlf);
> -
> -	if (reg) {
> -		pd->dlf_size = fls(reg);
> -		p->get_divisor = dw8250_get_divisor;
> -		p->set_divisor = dw8250_set_divisor;
> +	if (!pd->no_dlf) {
> +		/* Preserve value written by firmware or bootloader  */
> +		old_dlf = dw8250_readl_ext(p, DW_UART_DLF);
> +		dw8250_writel_ext(p, DW_UART_DLF, ~0U);
> +		reg = dw8250_readl_ext(p, DW_UART_DLF);
> +		dw8250_writel_ext(p, DW_UART_DLF, old_dlf);
> +
> +		if (reg) {
> +			pd->dlf_size = fls(reg);
> +			p->get_divisor = dw8250_get_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 ee7a07fac0f6..ca0dfd6d056e 100644
> --- a/drivers/tty/serial/8250/8250_dwlib.h
> +++ b/drivers/tty/serial/8250/8250_dwlib.h
> @@ -88,6 +88,7 @@ struct dw8250_port_data {
>   	/* Hardware configuration */
>   	u32			cpr_value;
>   	u8			dlf_size;
> +	bool			no_dlf;		/* Offset 0xc0 is not DLF */
>   
>   	/* RS485 variables */
>   	bool			hw_rs485_support;
  

Hi Andre,

On 10/6/26 11:46, Andre Przywara wrote:
> So I was wondering if we should refine the DLF detection instead? Only
> when the readback from 0xc0 returns some 2^n-1 value we assume DLF is
> implemented?

Thanks, that's better than a per-SoC quirk. On the A7Z, writing all ones
to 0xc0 reads back 0x19f, so the same family as your 0x9f with bit 8 on
top. A 2^n - 1 check rejects it.

With the check on the board, dlf_size is 0 and 0xc0 is still 0 after the
termios stress loop. Without it, 0xc0 reads 0xb after boot. I don't have
a UART with a real DLF to try.

v2 puts the check in its own patch and keeps the fallback, with the A733
compatible only selecting SKIP_SET_RATE. On the fallback alone the same
loop leaves 28 garbage bytes in 3000 lines.

The probe itself still writes all ones to 0xc0 before restoring it. Would
you rather skip that with a DT property, or leave it?

Thanks,
Vinicius
  

Hi,

about the rate settting ...

On 10/5/26 19:25, Vinicius Pedrosa wrote:
> The A733 UART is clocked from its bus clock gate, which can't change
> rate. dw8250_set_termios() still gates it around a no-op clk_set_rate()
> on every termios change. That stalls the character being shifted out

So isn't that a bug somewhere else then? Why would it gate the clock to 
set the rate? Shouldn't this decision be left to the CCF, where we have 
CLK_SET_RATE_GATE and CLK_SET_RATE_UNGATE to describe the clock's 
requirement? For instance the A733 manual says:
"Note: The clock MUX/DIV provides glitch-free switching
and supports dynamic configuration."
So we can both change the mux and the divider without gating.

And secondly, why does the termios code gate the clock when it's 
apparently not safe to do so, because the UART is not quiescent?

And what is your use case here, exactly? Do you reconfigure the baud 
rate while characters are incoming? That would garble characters anyway?

One thing you could try: the binding and the code support separate bus 
gate and baud rate clocks. So can you just expose CLK_APB1 as the baud 
rate clock in the DT? This would allow to change the rate, which is also 
beneficial, because at least on the current H6 generation U-Boot code 
(clock_sun50i_h6.c) we set the UART APB to 24MHz, so limit ourselves to 
1.5Mpbs. With APB1 set to 100 or 160 MHz, we could get higher baud 
rates, and exposing the mux+divider clock should allow to set the rate. 
I think the CCF would still need to negotiate with the other users, 
which are I2C and PWM, but that's something to solve there.

Cheers,
Andre.

  > and corrupts it on the wire. Use the existing SKIP_SET_RATE quirk, as
> other SoCs with a fixed UART clock do.
> 
> Offset 0xc0 is the RS485 control register on this SoC (A733 User
> Manual, UART_485_CTL), not DLF. dw8250_setup_port() writes all ones
> there, reads back a nonzero 9-bit value and takes it for a 9-bit DLF.
> Every later divisor change then writes the fractional divisor into the
> RS485 control register. Add a NO_DLF quirk so dwlib skips that probe.
> 
> Signed-off-by: Vinicius Pedrosa <vinicius.eduardo.pedrosa@gmail.com>
> ---
>   drivers/tty/serial/8250/8250_dw.c    | 13 +++++++++++++
>   drivers/tty/serial/8250/8250_dwlib.c | 22 ++++++++++++----------
>   drivers/tty/serial/8250/8250_dwlib.h |  1 +
>   3 files changed, 26 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
> index ba414306c98a..ea33aa644cda 100644
> --- a/drivers/tty/serial/8250/8250_dw.c
> +++ b/drivers/tty/serial/8250/8250_dw.c
> @@ -52,6 +52,7 @@
>   #define DW_UART_QUIRK_CPR_VALUE		BIT(5)
>   #define DW_UART_QUIRK_IER_KICK		BIT(6)
>   #define DW_UART_QUIRK_SKIP_EMPTY_FIFO_READ	BIT(7)
> +#define DW_UART_QUIRK_NO_DLF		BIT(8)
>   
>   /*
>    * Number of consecutive IIR_NO_INT interrupts required to trigger interrupt
> @@ -606,6 +607,8 @@ static void dw8250_quirks(struct uart_port *p, struct dw8250_data *data)
>   		p->serial_out = dw8250_serial_out38x;
>   	if (quirks & DW_UART_QUIRK_SKIP_SET_RATE)
>   		p->set_termios = dw8250_do_set_termios;
> +	if (quirks & DW_UART_QUIRK_NO_DLF)
> +		data->data.no_dlf = true;
>   	if (quirks & DW_UART_QUIRK_IS_DMA_FC) {
>   		data->data.dma.txconf.device_fc = 1;
>   		data->data.dma.rxconf.device_fc = 1;
> @@ -892,6 +895,15 @@ static const struct dw8250_platform_data dw8250_skip_set_rate_data = {
>   	.quirks = DW_UART_QUIRK_SKIP_SET_RATE,
>   };
>   
> +/*
> + * The baud clock is the bus clock gate, whose rate cannot change, and offset
> + * 0xc0 is the RS485 control register rather than DLF.
> + */
> +static const struct dw8250_platform_data dw8250_sun60i_a733_data = {
> +	.usr_reg = DW_UART_USR,
> +	.quirks = DW_UART_QUIRK_SKIP_SET_RATE | DW_UART_QUIRK_NO_DLF,
> +};
> +
>   static const struct dw8250_platform_data dw8250_intc10ee = {
>   	.usr_reg = DW_UART_USR,
>   	.quirks = DW_UART_QUIRK_IER_KICK,
> @@ -913,6 +925,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_sun60i_a733_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 },
> diff --git a/drivers/tty/serial/8250/8250_dwlib.c b/drivers/tty/serial/8250/8250_dwlib.c
> index 9bb02a4ab11f..9c6f3d926ad5 100644
> --- a/drivers/tty/serial/8250/8250_dwlib.c
> +++ b/drivers/tty/serial/8250/8250_dwlib.c
> @@ -209,16 +209,18 @@ void dw8250_setup_port(struct uart_port *p)
>   	}
>   	up->capabilities |= UART_CAP_NOTEMT;
>   
> -	/* Preserve value written by firmware or bootloader  */
> -	old_dlf = dw8250_readl_ext(p, DW_UART_DLF);
> -	dw8250_writel_ext(p, DW_UART_DLF, ~0U);
> -	reg = dw8250_readl_ext(p, DW_UART_DLF);
> -	dw8250_writel_ext(p, DW_UART_DLF, old_dlf);
> -
> -	if (reg) {
> -		pd->dlf_size = fls(reg);
> -		p->get_divisor = dw8250_get_divisor;
> -		p->set_divisor = dw8250_set_divisor;
> +	if (!pd->no_dlf) {
> +		/* Preserve value written by firmware or bootloader  */
> +		old_dlf = dw8250_readl_ext(p, DW_UART_DLF);
> +		dw8250_writel_ext(p, DW_UART_DLF, ~0U);
> +		reg = dw8250_readl_ext(p, DW_UART_DLF);
> +		dw8250_writel_ext(p, DW_UART_DLF, old_dlf);
> +
> +		if (reg) {
> +			pd->dlf_size = fls(reg);
> +			p->get_divisor = dw8250_get_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 ee7a07fac0f6..ca0dfd6d056e 100644
> --- a/drivers/tty/serial/8250/8250_dwlib.h
> +++ b/drivers/tty/serial/8250/8250_dwlib.h
> @@ -88,6 +88,7 @@ struct dw8250_port_data {
>   	/* Hardware configuration */
>   	u32			cpr_value;
>   	u8			dlf_size;
> +	bool			no_dlf;		/* Offset 0xc0 is not DLF */
>   
>   	/* RS485 variables */
>   	bool			hw_rs485_support;
  

Hi Andre,

On 10/6/26 14:16, Andre Przywara wrote:
> So isn't that a bug somewhere else then? Why would it gate the clock to
> set the rate?

I think the bug is in dw8250_set_termios(). The gate comes from
4e26b134bd17 (2014, replacing the Baytrail hook) and runs on every
termios change that reaches the driver, with nothing draining the port
first. On LPSS the UART's clock ends in a gate on bit 31, the divider
update bit, so I'd guess the disable/enable pair is how a new rate gets
latched there. On the A733 the DT hands over only the bus gate, so that
is what gets pulsed.

> And what is your use case here, exactly? Do you reconfigure the baud
> rate while characters are incoming?

No baud change. I toggled only CLOCAL with TCSANOW, 300 times while the
console printed 3000 lines. With the gate alone every run lost 12 to 18
lines (3 runs). With round_rate and set_rate but no gate, all 3 were
clean. A CSTOPB toggle splits the same way over 6 runs each.

> So can you just expose CLK_APB1 as the baud rate clock in the DT?

On the A733 the UARTs aren't on APB1. They sit on APB_UART (0x538),
shared by the seven UARTs. I tried it as baudclk with the bus gate as
apb_pclk and the console was garbage. The driver asks for 16 x baud and
the CCU rounds down, so 115200 gets RC16M/9 = 1777778 and divisor 1
gives 111111 baud. 24M/13 is just above the target, so it isn't picked.
ccu-sun6i-rtc lists RC16M at 30% accuracy anyway.

It's also shared: 01569a29af76 removed the clock notifier in 7.2, so a
port that re-rates APB_UART would leave the other six with stale
divisors.

For v2 I'd drop SKIP_SET_RATE for the A733 and only gate and set the
rate when clk_round_rate() differs from clk_get_rate(). By my reading
the branch is never taken here, and that kernel was clean in all 13
runs. H6, H616 and A523 pass only a bus gate too, so they likely see the
same thing, but I can't test them.

Would you rather drop the gate altogether? I kept it because I can't
test LPSS.

Thanks,
Vinicius
  

Hi Andre,

Correction to my last mail: the v2 change I proposed there is wrong.

Gating only when clk_round_rate() differs from the current rate is
e5d6bd25f93d ("serial: 8250_dw: Do not reclock if already at correct
rate"). 7dfae6cbadc1 reverted it because it broke the dw UARTs on Bay
Trail and Cherry Trail. Mine compares against clk_get_rate() instead of
p->uartclk. Peter suggested that variant in the revert thread, and Hans
pointed out it's the same check, since uartclk comes from clk_get_rate()
at probe.

>From my reading of clk.c, the flag you mentioned seems to explain the
regression. The LPSS "-update" gate is registered with
CLK_SET_RATE_PARENT | CLK_SET_RATE_GATE in drivers/acpi/x86/lpss.c.
Preparing it rate-protects the gate and its divider parent, and while
they're protected clk_round_rate() returns the current rate. So the
check always says there's nothing to do. The divider never gets
reprogrammed. Hans's debug log in
<20240317214123.34482-1-hdegoede@redhat.com> has a request for 1843200
rounded to 153600, the rate already set. I can't test LPSS.

So on LPSS the CCF does ask for the gate, and the disable/enable in
dw8250_set_termios() is how the driver gets clk_set_rate() past it.

For v2 I'm inclined to keep SKIP_SET_RATE for the A733, like JH7100
(b0ad20a3b64b) and SG2044 (cad4dda82c7e). The A733 bus-uart gates carry
no rate flags, so the rate calls there are no-ops. Calling
clk_set_rate() with the clock running and gating only on -EBUSY might
work for everyone, but it needs BYT/CHT testing I can't do.

This is separate from DLF. v2 still uses your 2^n - 1 check for that,
with no A733 quirk.

Thanks,
Vinicius