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

[sashiko-bot@kernel.org]

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?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261005172538.398522-1-vinicius.eduardo.pedrosa@gmail.com?part=2