Re: [PATCH 2/2] serial: 8250_ingenic: Start the X1600 UART from 16 clocks a bit

[H. Nikolaus Schaller]

Hi,

> Am 24.10.2026 um 07:15 schrieb Olivier Louvignes <olivier@xxxxxxxxx>:
> 
> The X1600 UART samples each bit with UMR clocks, adjusted per bit by
> UACR, while the divisor the 8250 core computes assumes 16. The driver
> never sets them, so the UART keeps what ran before it: on a HiBy R1 the
> vendor kernel leaves UART0's UMR at 8, which by the register's meaning

So this patch adjusts a driver to a specific board and not the x1600 chip?

Then, this asks for a device tree property if it is really needed.

Or it should be fixed in U-Boot. Otherwise, assuming now that UMR = 16 is
correct for all x1600 boards would introduce a big issue for other boards
which suddenly stop working at the previously correct rate.

> doubles every rate set here (the line rate itself was not measured). The
> Bluetooth chip on that UART then never answers: "command 0xfc18 tx
> timeout" from hci_bcm.
> 
> Add an X1600 entry that writes UMR = 16 and UACR = 0 at probe. The
> offsets come from Rockbox's X1600 register headers and match the UMR
> read on the R1. The X1000, whose compatible the X1600's falls back to,
> may have the same registers, but I could not check one, so it is left
> alone.

Well, it should be checked...

... the x1000 has the same register.

Of course it does not seem to be a standard 8250 register. Anyways,
it should be checked is how other 8250 drivers handle special/private
register setup.

Or this should even become part of the clock setup?

> 
> Tested on a HiBy R1: UMR reads 16 and UACR 0 after probe, and the
> BCM43430A1 on UART0 comes up at 1.5 Mbaud and streams A2DP audio.
> 
> Signed-off-by: Olivier Louvignes <olivier@xxxxxxxxx>
> ---
> The test on the R1 ran with the same reset placed in the X1000 entry; the
> R1's device tree lists "ingenic,x1600-uart" first, so it takes the same
> path with this version. Build-tested with W=1 on tty-next.
> 
> drivers/tty/serial/8250/8250_ingenic.c | 18 ++++++++++++++++++
> 1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/tty/serial/8250/8250_ingenic.c b/drivers/tty/serial/8250/8250_ingenic.c
> index 0cdadacd7019..40307189021b 100644
> --- a/drivers/tty/serial/8250/8250_ingenic.c
> +++ b/drivers/tty/serial/8250/8250_ingenic.c
> @@ -25,6 +25,7 @@
> struct ingenic_uart_config {
> int tx_loadsz;
> int fifosize;
> + bool has_umr;
> };
> 
> struct ingenic_uart_data {
> @@ -40,6 +41,10 @@ static const struct of_device_id of_match[];
> #define UART_MCR_MDCE BIT(7)
> #define UART_MCR_FCM BIT(6)
> 
> +/* Clocks a bit, 16 for the divisor the 8250 core computes, and their per-bit adjustment. */
> +#define UART_UMR 9
> +#define UART_UACR 10
> +
> static struct earlycon_device *early_device;
> 
> static uint8_t early_in(struct uart_port *port, int offset)
> @@ -298,6 +303,12 @@ static int ingenic_uart_probe(struct platform_device *pdev)
> }
> uart.port.uartclk = clk_get_rate(data->clk_baud);
> 
> + /* Whatever ran before may have left another count: 8 on a HiBy R1. */
> + if (cdata->has_umr) {
> + writel(16, uart.port.membase + (UART_UMR << uart.port.regshift));
> + writel(0, uart.port.membase + (UART_UACR << uart.port.regshift));
> + }
> +
> data->uport = serial8250_register_8250_port(&uart);
> if (IS_ERR(data->uport)) {
> err = PTR_ERR(data->uport);
> @@ -344,6 +355,12 @@ static const struct ingenic_uart_config x1000_uart_config = {
> .fifosize = 64,
> };
> 
> +static const struct ingenic_uart_config x1600_uart_config = {
> + .tx_loadsz = 32,
> + .fifosize = 64,
> + .has_umr = true,

the x1000 has it as well. And even the jz4780. So correctly setting this
flag (as it is named) for more SoC variants will break more boards...

> +};
> +
> static const struct of_device_id of_match[] = {
> { .compatible = "ingenic,jz4740-uart", .data = &jz4740_uart_config },
> { .compatible = "ingenic,jz4750-uart", .data = &jz4760_uart_config },
> @@ -352,6 +369,7 @@ static const struct of_device_id of_match[] = {
> { .compatible = "ingenic,jz4775-uart", .data = &jz4760_uart_config },
> { .compatible = "ingenic,jz4780-uart", .data = &jz4780_uart_config },
> { .compatible = "ingenic,x1000-uart", .data = &x1000_uart_config },
> + { .compatible = "ingenic,x1600-uart", .data = &x1600_uart_config },

No, this is not special to the x1600, but a workaround of some peculiar
setup of your bootloader.

> { /* sentinel */ }
> };
> MODULE_DEVICE_TABLE(of, of_match);
> -- 
> 2.39.5

BR, H. Nikolaus Schaller <hns@xxxxxxxxxxxxx>