diff mbox series

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

Message ID 20261005172538.398522-3-vinicius.eduardo.pedrosa@gmail.com (mailing list archive)
State New
Headers
Series serial: 8250_dw: Allwinner A733 UART support |

Commit Message

Vinicius Pedrosa Oct. 5, 2026, 5:25 p.m. UTC 
The Allwinner A733 UART is a DesignWare APB UART with two SoC-specific
differences. Its baud clock is the bus clock gate, which can't change
rate, and offset 0xc0 holds an RS485 control register instead of DLF.

Add a SoC-specific compatible, with "snps,dw-apb-uart" as the fallback.

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

Comments

sashiko-bot@kernel.org Oct. 5, 2026, 5:34 p.m. UTC | #1 
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] dt-bindings: Invalid snps,dw-apb-uart fallback for Allwinner A733
--

commit 51cd638ddbc823709bc6b8874d8c1f36411d5573
Author: Vinicius Pedrosa <vinicius.eduardo.pedrosa@gmail.com>

dt-bindings: serial: snps-dw-apb-uart: Add Allwinner A733

This commit adds the Allwinner A733 UART compatible string to the DesignWare
APB UART bindings, specifying "snps,dw-apb-uart" as a fallback. It mentions
that the baud clock cannot change rate and offset 0xc0 is an RS485 control
register rather than a DLF register.

> diff --git a/Documentation/devicetree/bindings/serial/snps-dw-apb-uart.yaml b/Documentation/devicetree/bindings/serial/snps-dw-apb-uart.yaml
> index 86dc259193112..aa8cab5ba3d33 100644
> --- a/Documentation/devicetree/bindings/serial/snps-dw-apb-uart.yaml
> +++ b/Documentation/devicetree/bindings/serial/snps-dw-apb-uart.yaml
> @@ -51,6 +51,7 @@ properties:
>            - const: renesas,rzn1-uart
>        - items:
>            - enum:
> +              - allwinner,sun60i-a733-uart

[Severity: High]
Is it safe to use "snps,dw-apb-uart" as a fallback for the A733?

When an OS binds to the generic "snps,dw-apb-uart" fallback, the 8250_dwlib
driver auto-probes offset 0xc0 (DW_UART_DLF) to detect DLF support:

drivers/tty/serial/8250/8250_dwlib.c:dw8250_setup_port() {
    ...
    old_dlf = dw8250_readl_ext(p, DW_UART_DLF);
    dw8250_writel_ext(p, DW_UART_DLF, ~0U);
    ...
}

Because 0xc0 is an active R/W RS485 register on the A733, the auto-probe will
falsely succeed. The generic driver will then set p->set_divisor =
dw8250_set_divisor, which blindly overwrites the RS485 bits with fractional
divisor calculations on every baud rate change.

Can this corrupt the RS485 configuration and potentially trigger unintended
hardware states (such as bus collisions) on the RS485 line?

Furthermore, the generic fallback lacks the DW_UART_QUIRK_SKIP_SET_RATE
quirk, so it will erroneously attempt to call clk_set_rate() on the bus clock
gate during baud rate changes:

drivers/tty/serial/8250/8250_dw.c:dw8250_set_termios() {
    ...
    ret = clk_set_rate(d->clk, newrate);
    ...
}

Given that the generic fallback driver actively misbehaves on this hardware,
should the compatible definition omit the fallback string so that unmodified
kernels do not attempt to drive the A733 incorrectly?
Conor Dooley Oct. 7, 2026, 10:36 a.m. UTC | #2 
Acked-by: Conor Dooley <conor.dooley@microchip.com>
pw-bot: not-applicable
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/serial/snps-dw-apb-uart.yaml b/Documentation/devicetree/bindings/serial/snps-dw-apb-uart.yaml
index 86dc25919311..aa8cab5ba3d3 100644
--- a/Documentation/devicetree/bindings/serial/snps-dw-apb-uart.yaml
+++ b/Documentation/devicetree/bindings/serial/snps-dw-apb-uart.yaml
@@ -51,6 +51,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