diff mbox series

[v9,13/15] serial: 8250_dw: add Ambarella CV75 quirks

Message ID 20261001-cv75-v5-v9-13-486cc289a4b8@ambarella.com
State New
Headers show
Series Ambarella CV75 SoC minimal bring-up | expand

Commit Message

Long Zhao via B4 Relay Oct. 1, 2026, 12:41 p.m. UTC 
From: Long Zhao <longzhao@ambarella.com>

The Ambarella CV75 UART is Ambarella IP, not a Synopsys DesignWare
licensed block. At the register level it is close enough to DW APB UART
that a full custom UART driver is unnecessary: reuse 8250_dw with a
small platform quirk.

Quirks:
- keep IER ETOI (bit 5) set for RX timeout behaviour
- skip set_rate; baud clock is managed outside the UART block

Reviewed-by: Linus Walleij <linusw@kernel.org>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Signed-off-by: Long Zhao <longzhao@ambarella.com>
---
 drivers/tty/serial/8250/8250_dw.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

Comments

Greg Kroah-Hartman Oct. 1, 2026, 12:52 p.m. UTC | #1 
On Thu, Oct 01, 2026 at 08:41:02PM +0800, Long Zhao via B4 Relay wrote:
> From: Long Zhao <longzhao@ambarella.com>
> 
> The Ambarella CV75 UART is Ambarella IP, not a Synopsys DesignWare
> licensed block. At the register level it is close enough to DW APB UART
> that a full custom UART driver is unnecessary: reuse 8250_dw with a
> small platform quirk.
> 
> Quirks:
> - keep IER ETOI (bit 5) set for RX timeout behaviour
> - skip set_rate; baud clock is managed outside the UART block
> 
> Reviewed-by: Linus Walleij <linusw@kernel.org>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Signed-off-by: Long Zhao <longzhao@ambarella.com>
> ---
>  drivers/tty/serial/8250/8250_dw.c | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)

When you submit lots of different subsystem patches all in one series,
it's hard, if not impossible, for subsystem maintainers to take them :(

thanks,

greg k-h
Linus Walleij Oct. 1, 2026, 7:20 p.m. UTC | #2 
On Thu, Oct 1, 2026 at 2:52 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Thu, Oct 01, 2026 at 08:41:02PM +0800, Long Zhao via B4 Relay wrote:

> > From: Long Zhao <longzhao@ambarella.com>
> >
> > The Ambarella CV75 UART is Ambarella IP, not a Synopsys DesignWare
> > licensed block. At the register level it is close enough to DW APB UART
> > that a full custom UART driver is unnecessary: reuse 8250_dw with a
> > small platform quirk.
> >
> > Quirks:
> > - keep IER ETOI (bit 5) set for RX timeout behaviour
> > - skip set_rate; baud clock is managed outside the UART block
> >
> > Reviewed-by: Linus Walleij <linusw@kernel.org>
> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > Signed-off-by: Long Zhao <longzhao@ambarella.com>
> > ---
> >  drivers/tty/serial/8250/8250_dw.c | 32 ++++++++++++++++++++++++++++++++
> >  1 file changed, 32 insertions(+)
>
> When you submit lots of different subsystem patches all in one series,
> it's hard, if not impossible, for subsystem maintainers to take them :(

In my experience people do this in cases like this it is because they
essentially dump out their "bring-up" tree, the thing they have
booting on the hardware so they know the pieces work together.

Long: can you split it up in topic branches and work on e.g. the
serial driver on a separate branch?

A better way to test the whole set of patces is to just create a
mixdown branch where you merge all the topic branched into
one "test" branch, then you boot and test this before sending
out each branch separately.

Each topic branch can be versioned separately with b4.

Yours,
Linus Walleij
zl020895 Oct. 2, 2026, 2:17 a.m. UTC | #3 
Hi Greg, Linus,

Thanks.

This series follows the new-vendor, new-SoC process and is sent
as one set, so the new Kconfig symbol and the drivers that use it
land together.

If it has to be split, the GPIO patches and the UART patches can
go out as their own series. The SoC patches and the CV75 clock
driver should stay in one series: the clock Kconfig depends on
ARCH_AMBARELLA, which is added by the arm64 patch.

Would that split be acceptable for you?

Best regards,
Long Zhao

At 2026-10-02 03:20:51, "Linus Walleij" <linusw@kernel.org> wrote:
>On Thu, Oct 1, 2026 at 2:52 PM Greg Kroah-Hartman
><gregkh@linuxfoundation.org> wrote:
>> On Thu, Oct 01, 2026 at 08:41:02PM +0800, Long Zhao via B4 Relay wrote:
>
>> > From: Long Zhao <longzhao@ambarella.com>
>> >
>> > The Ambarella CV75 UART is Ambarella IP, not a Synopsys DesignWare
>> > licensed block. At the register level it is close enough to DW APB UART
>> > that a full custom UART driver is unnecessary: reuse 8250_dw with a
>> > small platform quirk.
>> >
>> > Quirks:
>> > - keep IER ETOI (bit 5) set for RX timeout behaviour
>> > - skip set_rate; baud clock is managed outside the UART block
>> >
>> > Reviewed-by: Linus Walleij <linusw@kernel.org>
>> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>> > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
>> > Signed-off-by: Long Zhao <longzhao@ambarella.com>
>> > ---
>> >  drivers/tty/serial/8250/8250_dw.c | 32 ++++++++++++++++++++++++++++++++
>> >  1 file changed, 32 insertions(+)
>>
>> When you submit lots of different subsystem patches all in one series,
>> it's hard, if not impossible, for subsystem maintainers to take them :(
>
>In my experience people do this in cases like this it is because they
>essentially dump out their "bring-up" tree, the thing they have
>booting on the hardware so they know the pieces work together.
>
>Long: can you split it up in topic branches and work on e.g. the
>serial driver on a separate branch?
>
>A better way to test the whole set of patces is to just create a
>mixdown branch where you merge all the topic branched into
>one "test" branch, then you boot and test this before sending
>out each branch separately.
>
>Each topic branch can be versioned separately with b4.
>
>Yours,
>Linus Walleij
Jerome Brunet Oct. 2, 2026, 8:09 a.m. UTC | #4 
On ven. 02 oct. 2026 at 10:17, zl020895  <zl020895@163.com> wrote:

> Hi Greg, Linus,
>
> Thanks.
>
> This series follows the new-vendor, new-SoC process and is sent
> as one set, so the new Kconfig symbol and the drivers that use it
> land together.

What Greg and Linus say here is basically what I already suggested on
v6 [1]

Giving the whole context of a bring ups, mixing many trees, might be
nice at the RFC stage but then it makes a lot more difficult.

[1]: https://lore.kernel.org/all/1jld986j7m.fsf@starbuckisacylon.baylibre.com/

>
> If it has to be split, the GPIO patches and the UART patches can
> go out as their own series. The SoC patches and the CV75 clock
> driver should stay in one series: the clock Kconfig depends on
> ARCH_AMBARELLA, which is added by the arm64 patch.
>
> Would that split be acceptable for you?
>

I don't get why clock is any different here. Just state your
dependencies in the cover letter. `b4` can help you there as well.

> Best regards,
> Long Zhao
>
> At 2026-10-02 03:20:51, "Linus Walleij" <linusw@kernel.org> wrote:
>>On Thu, Oct 1, 2026 at 2:52 PM Greg Kroah-Hartman
>><gregkh@linuxfoundation.org> wrote:
>>> On Thu, Oct 01, 2026 at 08:41:02PM +0800, Long Zhao via B4 Relay wrote:
>>
>>> > From: Long Zhao <longzhao@ambarella.com>
>>> >
>>> > The Ambarella CV75 UART is Ambarella IP, not a Synopsys DesignWare
>>> > licensed block. At the register level it is close enough to DW APB UART
>>> > that a full custom UART driver is unnecessary: reuse 8250_dw with a
>>> > small platform quirk.
>>> >
>>> > Quirks:
>>> > - keep IER ETOI (bit 5) set for RX timeout behaviour
>>> > - skip set_rate; baud clock is managed outside the UART block
>>> >
>>> > Reviewed-by: Linus Walleij <linusw@kernel.org>
>>> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>>> > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
>>> > Signed-off-by: Long Zhao <longzhao@ambarella.com>
>>> > ---
>>> >  drivers/tty/serial/8250/8250_dw.c | 32 ++++++++++++++++++++++++++++++++
>>> >  1 file changed, 32 insertions(+)
>>>
>>> When you submit lots of different subsystem patches all in one series,
>>> it's hard, if not impossible, for subsystem maintainers to take them :(
>>
>>In my experience people do this in cases like this it is because they
>>essentially dump out their "bring-up" tree, the thing they have
>>booting on the hardware so they know the pieces work together.
>>
>>Long: can you split it up in topic branches and work on e.g. the
>>serial driver on a separate branch?
>>
>>A better way to test the whole set of patces is to just create a
>>mixdown branch where you merge all the topic branched into
>>one "test" branch, then you boot and test this before sending
>>out each branch separately.
>>
>>Each topic branch can be versioned separately with b4.
>>
>>Yours,
>>Linus Walleij
diff mbox series

Patch

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index 5fba913f3301..06f03e893346 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -43,6 +43,9 @@ 
 #define RZN1_UART_xDMACR_8_WORD_BURST	(2 << 1)
 #define RZN1_UART_xDMACR_BLK_SZ(x)	((x) << 3)
 
+/* Ambarella IER: receive timeout interrupt enable */
+#define AMBARELLA_UART_IER_ETOI		BIT(5)
+
 /* Quirks */
 #define DW_UART_QUIRK_OCTEON		BIT(0)
 #define DW_UART_QUIRK_ARMADA_38X	BIT(1)
@@ -60,6 +63,7 @@ 
 
 struct dw8250_platform_data {
 	u8 usr_reg;
+	u8 ier_mask;
 	u32 cpr_value;
 	unsigned int quirks;
 };
@@ -572,6 +576,25 @@  static void dw8250_prepare_rx_dma(struct uart_8250_port *p)
 	dw8250_writel_ext(up, RZN1_UART_RDMACR, val);
 }
 
+static int dw8250_ier_mask_startup(struct uart_port *p)
+{
+	struct dw8250_data *d = to_dw8250_data(p->private_data);
+	struct uart_8250_port *up = up_to_u8250p(p);
+	int ret;
+
+	ret = serial8250_do_startup(p);
+	if (ret)
+		return ret;
+
+	/*
+	 * Ambarella keeps variant IER bits (e.g. ETOI) set for correct RX
+	 * timeout behaviour.
+	 */
+	up->ier |= d->pdata->ier_mask;
+
+	return 0;
+}
+
 static void dw8250_quirks(struct uart_port *p, struct dw8250_data *data)
 {
 	unsigned int quirks = data->pdata->quirks;
@@ -606,6 +629,8 @@  static void dw8250_quirks(struct uart_port *p, struct dw8250_data *data)
 		p->serial_in = dw8250_serial_in32;
 		data->uart_16550_compatible = true;
 	}
+	if (data->pdata->ier_mask)
+		p->startup = dw8250_ier_mask_startup;
 }
 
 static void dw8250_reset_control_assert(void *data)
@@ -879,6 +904,12 @@  static const struct dw8250_platform_data dw8250_intc10ee = {
 	.quirks = DW_UART_QUIRK_IER_KICK,
 };
 
+static const struct dw8250_platform_data dw8250_ambarella_cv75_data = {
+	.usr_reg = DW_UART_USR,
+	.ier_mask = AMBARELLA_UART_IER_ETOI,
+	.quirks = DW_UART_QUIRK_SKIP_SET_RATE,
+};
+
 static const struct dw8250_platform_data dw8250_ultrarisc_dp1000_data = {
 	.usr_reg = DW_UART_USR,
 	.cpr_value = FIELD_PREP_CONST(DW_UART_CPR_ABP_DATA_WIDTH, 2) |
@@ -890,6 +921,7 @@  static const struct dw8250_platform_data dw8250_ultrarisc_dp1000_data = {
 
 static const struct of_device_id dw8250_of_match[] = {
 	{ .compatible = "snps,dw-apb-uart", .data = &dw8250_dw_apb },
+	{ .compatible = "ambarella,cv75-uart", .data = &dw8250_ambarella_cv75_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 },