public inbox for devicetree@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH v4 0/2] Airoha UART support
@ 2025-02-09 21:02 Benjamin Larsson
  2025-02-09 21:02 ` [PATCH 1/2] dt-bindings: serial: 8250: Add Airoha compatibles Benjamin Larsson
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Benjamin Larsson @ 2025-02-09 21:02 UTC (permalink / raw)
  To: linux-serial, devicetree; +Cc: ansuelsmth, lorenzo, gregkh, Benjamin Larsson

The Airoha familty of SoCs have a UART hardware that is 16550-compatible
with the exception of the baud rate settings.

This patch implements code for calculating the baud rate for the Airoha
UART and HSUART.

Signed-off-by: Benjamin Larsson <benjamin.larsson@genexis.eu>
---
Changes in v4:
- Removed COMPILE_TEST from Kconfig
- Removed Kconfig option to build as module

Changes in v3:
- Reworded commit message
- Restructured comment text
- Fixed kernel-doc warning
- Fixed kernel test robot build error and build warning

Changes in v2:
- Removed ifdef use in .c files
- Removed uart port defines from user-space headers
- Reworded commit message
- Added code documentation

Benjamin Larsson (2):
  dt-bindings: serial: 8250: Add Airoha compatibles
  serial: Airoha SoC UART and HSUART support

 .../devicetree/bindings/serial/8250.yaml      |  2 +
 drivers/tty/serial/8250/8250.h                | 15 ++++
 drivers/tty/serial/8250/8250_airoha.c         | 83 +++++++++++++++++++
 drivers/tty/serial/8250/8250_of.c             |  2 +
 drivers/tty/serial/8250/8250_port.c           | 27 ++++++
 drivers/tty/serial/8250/Kconfig               | 10 +++
 drivers/tty/serial/8250/Makefile              |  1 +
 7 files changed, 140 insertions(+)
 create mode 100644 drivers/tty/serial/8250/8250_airoha.c

-- 
2.34.1


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 1/2] dt-bindings: serial: 8250: Add Airoha compatibles
  2025-02-09 21:02 [PATCH v4 0/2] Airoha UART support Benjamin Larsson
@ 2025-02-09 21:02 ` Benjamin Larsson
  2025-02-09 21:02 ` [PATCH 2/2] serial: Airoha SoC UART and HSUART support Benjamin Larsson
  2025-02-10  6:14 ` [PATCH v4 0/2] Airoha UART support Greg KH
  2 siblings, 0 replies; 11+ messages in thread
From: Benjamin Larsson @ 2025-02-09 21:02 UTC (permalink / raw)
  To: linux-serial, devicetree
  Cc: ansuelsmth, lorenzo, gregkh, Benjamin Larsson,
	Krzysztof Kozlowski

The Airoha SoC family have a mostly 16550-compatible UART
and High-Speed UART hardware with the exception of custom
baud rate settings register.

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Benjamin Larsson <benjamin.larsson@genexis.eu>
---
 Documentation/devicetree/bindings/serial/8250.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/serial/8250.yaml b/Documentation/devicetree/bindings/serial/8250.yaml
index 692aa05500fd..2fbb972e5460 100644
--- a/Documentation/devicetree/bindings/serial/8250.yaml
+++ b/Documentation/devicetree/bindings/serial/8250.yaml
@@ -63,6 +63,8 @@ properties:
       - const: mrvl,pxa-uart
       - const: nuvoton,wpcm450-uart
       - const: nuvoton,npcm750-uart
+      - const: airoha,airoha-uart
+      - const: airoha,airoha-hsuart
       - const: nvidia,tegra20-uart
       - const: nxp,lpc3220-uart
       - items:
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 2/2] serial: Airoha SoC UART and HSUART support
  2025-02-09 21:02 [PATCH v4 0/2] Airoha UART support Benjamin Larsson
  2025-02-09 21:02 ` [PATCH 1/2] dt-bindings: serial: 8250: Add Airoha compatibles Benjamin Larsson
@ 2025-02-09 21:02 ` Benjamin Larsson
  2025-03-22 21:42   ` Andy Shevchenko
  2025-02-10  6:14 ` [PATCH v4 0/2] Airoha UART support Greg KH
  2 siblings, 1 reply; 11+ messages in thread
From: Benjamin Larsson @ 2025-02-09 21:02 UTC (permalink / raw)
  To: linux-serial, devicetree; +Cc: ansuelsmth, lorenzo, gregkh, Benjamin Larsson

Support for Airoha AN7581 SoC UART and HSUART baud rate
calculation routine.

Signed-off-by: Benjamin Larsson <benjamin.larsson@genexis.eu>
---
 drivers/tty/serial/8250/8250.h        | 15 +++++
 drivers/tty/serial/8250/8250_airoha.c | 81 +++++++++++++++++++++++++++
 drivers/tty/serial/8250/8250_of.c     |  2 +
 drivers/tty/serial/8250/8250_port.c   | 26 +++++++++
 drivers/tty/serial/8250/Kconfig       | 10 ++++
 drivers/tty/serial/8250/Makefile      |  1 +
 6 files changed, 135 insertions(+)
 create mode 100644 drivers/tty/serial/8250/8250_airoha.c

diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
index e5310c65cf52..dd762289fa25 100644
--- a/drivers/tty/serial/8250/8250.h
+++ b/drivers/tty/serial/8250/8250.h
@@ -314,6 +314,21 @@ static inline int serial8250_in_MCR(struct uart_8250_port *up)
 	return mctrl;
 }
 
+/* uart_config[] table port type defines */
+/* Airoha UART */
+#define PORT_AIROHA	124
+
+/* Airoha HSUART */
+#define PORT_AIROHA_HS	125
+
+#ifdef CONFIG_SERIAL_8250_AIROHA
+void airoha8250_set_baud_rate(struct uart_port *port,
+			     unsigned int baud, unsigned int hs);
+#else
+static inline void airoha8250_set_baud_rate(struct uart_port *port,
+			     unsigned int baud, unsigned int hs) { }
+#endif
+
 #ifdef CONFIG_SERIAL_8250_PNP
 int serial8250_pnp_init(void);
 void serial8250_pnp_exit(void);
diff --git a/drivers/tty/serial/8250/8250_airoha.c b/drivers/tty/serial/8250/8250_airoha.c
new file mode 100644
index 000000000000..51e675605741
--- /dev/null
+++ b/drivers/tty/serial/8250/8250_airoha.c
@@ -0,0 +1,81 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+/*
+ * Airoha UART baud rate calculation function
+ *
+ * Copyright (c) 2025 Genexis Sweden AB
+ * Author: Benjamin Larsson <benjamin.larsson@genexis.eu>
+ */
+
+#include "8250.h"
+
+/* The Airoha UART is 16550-compatible except for the baud rate calculation. */
+
+/* Airoha UART registers */
+#define UART_AIROHA_BRDL	0
+#define UART_AIROHA_BRDH	1
+#define UART_AIROHA_XINCLKDR	10
+#define UART_AIROHA_XYD		11
+
+#define XYD_Y 65000
+#define XINDIV_CLOCK 20000000
+#define UART_BRDL_20M 0x01
+#define UART_BRDH_20M 0x00
+
+static const int clock_div_tab[] = { 10, 4, 2};
+static const int clock_div_reg[] = {  4, 2, 1};
+
+/**
+ * airoha8250_set_baud_rate() - baud rate calculation routine
+ * @port: uart port
+ * @baud: requested uart baud rate
+ * @hs: uart type selector, 0 for regular uart and 1 for high-speed uart
+ *
+ * crystal_clock = 20 MHz (fixed frequency)
+ * xindiv_clock = crystal_clock / clock_div
+ * (x/y) = XYD, 32 bit register with 16 bits of x and then 16 bits of y
+ * clock_div = XINCLK_DIVCNT (default set to 10 (0x4)),
+ *           - 3 bit register [ 1, 2, 4, 8, 10, 12, 16, 20 ]
+ *
+ * baud_rate = ((xindiv_clock) * (x/y)) / ([BRDH,BRDL] * 16)
+ *
+ * Selecting divider needs to fulfill
+ * 1.8432 MHz <= xindiv_clk <= APB clock / 2
+ * The clocks are unknown but a divider of value 1 did not result in a valid
+ * waveform.
+ *
+ * XYD_y seems to need to be larger then XYD_x for proper waveform generation.
+ * Setting [BRDH,BRDL] to [0,1] and XYD_y to 65000 gives even values
+ * for usual baud rates.
+ */
+
+void airoha8250_set_baud_rate(struct uart_port *port,
+			     unsigned int baud, unsigned int hs)
+{
+	struct uart_8250_port *up = up_to_u8250p(port);
+	unsigned int xyd_x, nom, denom;
+	int i;
+
+	/* set DLAB to access the baud rate divider registers (BRDH, BRDL) */
+	serial_port_out(port, UART_LCR, up->lcr | UART_LCR_DLAB);
+	/* set baud rate calculation defaults */
+	/* set BRDIV ([BRDH,BRDL]) to 1 */
+	serial_port_out(port, UART_AIROHA_BRDL, UART_BRDL_20M);
+	serial_port_out(port, UART_AIROHA_BRDH, UART_BRDH_20M);
+	/* calculate XYD_x and XINCLKDR register by searching
+	 * through a table of crystal_clock divisors
+	 *
+	 * for the HSUART xyd_x needs to be scaled by a factor of 2
+	 */
+	for (i = 0 ; i < ARRAY_SIZE(clock_div_tab) ; i++) {
+		denom = (XINDIV_CLOCK/40) / clock_div_tab[i];
+		nom = baud * (XYD_Y/40);
+		xyd_x = ((nom/denom) << 4) >> hs;
+		if (xyd_x < XYD_Y)
+			break;
+	}
+	serial_port_out(port, UART_AIROHA_XINCLKDR, clock_div_reg[i]);
+	serial_port_out(port, UART_AIROHA_XYD, (xyd_x<<16) | XYD_Y);
+	/* unset DLAB */
+	serial_port_out(port, UART_LCR, up->lcr);
+}
diff --git a/drivers/tty/serial/8250/8250_of.c b/drivers/tty/serial/8250/8250_of.c
index 64aed7efc569..5315bc1bc06d 100644
--- a/drivers/tty/serial/8250/8250_of.c
+++ b/drivers/tty/serial/8250/8250_of.c
@@ -341,6 +341,8 @@ static const struct of_device_id of_platform_serial_table[] = {
 	{ .compatible = "ti,da830-uart", .data = (void *)PORT_DA830, },
 	{ .compatible = "nuvoton,wpcm450-uart", .data = (void *)PORT_NPCM, },
 	{ .compatible = "nuvoton,npcm750-uart", .data = (void *)PORT_NPCM, },
+	{ .compatible = "airoha,airoha-uart", .data = (void *)PORT_AIROHA, },
+	{ .compatible = "airoha,airoha-hsuart", .data = (void *)PORT_AIROHA_HS, },
 	{ /* end of list */ },
 };
 MODULE_DEVICE_TABLE(of, of_platform_serial_table);
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 1ea52fce9bf1..040659de35a2 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -319,6 +319,24 @@ static const struct serial8250_config uart_config[] = {
 		.rxtrig_bytes	= {1, 8, 16, 30},
 		.flags		= UART_CAP_FIFO | UART_CAP_AFE,
 	},
+	/* From here on after additional uart config port defines are placed in 8250.h
+	 */
+	[PORT_AIROHA] = {
+		.name		= "Airoha UART",
+		.fifo_size	= 8,
+		.tx_loadsz	= 1,
+		.fcr		= UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_01 | UART_FCR_CLEAR_RCVR,
+		.rxtrig_bytes	= {1, 4},
+		.flags		= UART_CAP_FIFO,
+	},
+	[PORT_AIROHA_HS] = {
+		.name		= "Airoha HSUART",
+		.fifo_size	= 128,
+		.tx_loadsz	= 128,
+		.fcr		= UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_01 | UART_FCR_CLEAR_RCVR,
+		.rxtrig_bytes	= {1, 4},
+		.flags		= UART_CAP_FIFO,
+	},
 };
 
 /* Uart divisor latch read */
@@ -2847,6 +2865,14 @@ serial8250_do_set_termios(struct uart_port *port, struct ktermios *termios,
 
 	serial8250_set_divisor(port, baud, quot, frac);
 
+	/*
+	 * Airoha SoCs have custom registers for baud rate settings
+	 */
+	if (port->type == PORT_AIROHA)
+		airoha8250_set_baud_rate(port, baud, 0);
+	if (port->type == PORT_AIROHA_HS)
+		airoha8250_set_baud_rate(port, baud, 1);
+
 	/*
 	 * LCR DLAB must be set to enable 64-byte FIFO mode. If the FCR
 	 * is written without DLAB set, this mode will be disabled.
diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
index 55d26d16df9b..9cf16d392aca 100644
--- a/drivers/tty/serial/8250/Kconfig
+++ b/drivers/tty/serial/8250/Kconfig
@@ -356,6 +356,16 @@ config SERIAL_8250_ACORN
 	  system, say Y to this option.  The driver can handle 1, 2, or 3 port
 	  cards.  If unsure, say N.
 
+config SERIAL_8250_AIROHA
+	bool "Airoha UART support"
+	depends on ARCH_AIROHA && OF && SERIAL_8250
+	help
+	  Selecting this option enables an Airoha SoC specific baud rate
+	  calculation routine on an otherwise 16550 compatible UART hardware.
+
+	  If you have an Airoha based board and want to use the serial port,
+	  say Y to this option. If unsure, say N.
+
 config SERIAL_8250_BCM2835AUX
 	tristate "BCM2835 auxiliar mini UART support"
 	depends on ARCH_BCM2835 || COMPILE_TEST
diff --git a/drivers/tty/serial/8250/Makefile b/drivers/tty/serial/8250/Makefile
index 1516de629b61..b7f07d5c4cca 100644
--- a/drivers/tty/serial/8250/Makefile
+++ b/drivers/tty/serial/8250/Makefile
@@ -20,6 +20,7 @@ obj-$(CONFIG_SERIAL_8250_CONSOLE)	+= 8250_early.o
 
 obj-$(CONFIG_SERIAL_8250_ACCENT)	+= 8250_accent.o
 obj-$(CONFIG_SERIAL_8250_ACORN)		+= 8250_acorn.o
+obj-$(CONFIG_SERIAL_8250_AIROHA)	+= 8250_airoha.o
 obj-$(CONFIG_SERIAL_8250_ASPEED_VUART)	+= 8250_aspeed_vuart.o
 obj-$(CONFIG_SERIAL_8250_BCM2835AUX)	+= 8250_bcm2835aux.o
 obj-$(CONFIG_SERIAL_8250_BCM7271)	+= 8250_bcm7271.o
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH v4 0/2] Airoha UART support
  2025-02-09 21:02 [PATCH v4 0/2] Airoha UART support Benjamin Larsson
  2025-02-09 21:02 ` [PATCH 1/2] dt-bindings: serial: 8250: Add Airoha compatibles Benjamin Larsson
  2025-02-09 21:02 ` [PATCH 2/2] serial: Airoha SoC UART and HSUART support Benjamin Larsson
@ 2025-02-10  6:14 ` Greg KH
  2025-03-21 20:37   ` Benjamin Larsson
  2 siblings, 1 reply; 11+ messages in thread
From: Greg KH @ 2025-02-10  6:14 UTC (permalink / raw)
  To: Benjamin Larsson; +Cc: linux-serial, devicetree, ansuelsmth, lorenzo

On Sun, Feb 09, 2025 at 10:02:39PM +0100, Benjamin Larsson wrote:
> The Airoha familty of SoCs have a UART hardware that is 16550-compatible
> with the exception of the baud rate settings.
> 
> This patch implements code for calculating the baud rate for the Airoha
> UART and HSUART.
> 
> Signed-off-by: Benjamin Larsson <benjamin.larsson@genexis.eu>
> ---
> Changes in v4:
> - Removed COMPILE_TEST from Kconfig

No, please don't do that.

> - Removed Kconfig option to build as module

No do that, you want your code as a module so that it can work in a
system that is built as a "generic system image" that does not force
your driver to be built into the main kernel image, wasting memory if
the hardware is not present.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v4 0/2] Airoha UART support
  2025-02-10  6:14 ` [PATCH v4 0/2] Airoha UART support Greg KH
@ 2025-03-21 20:37   ` Benjamin Larsson
  2025-03-22 21:45     ` Andy Shevchenko
  0 siblings, 1 reply; 11+ messages in thread
From: Benjamin Larsson @ 2025-03-21 20:37 UTC (permalink / raw)
  To: Greg KH; +Cc: linux-serial, devicetree, ansuelsmth, lorenzo

Hi, I seem to have missed that you commented on the patch.

On 10/02/2025 07:14, Greg KH wrote:
> On Sun, Feb 09, 2025 at 10:02:39PM +0100, Benjamin Larsson wrote:
>> The Airoha familty of SoCs have a UART hardware that is 16550-compatible
>> with the exception of the baud rate settings.
>>
>> This patch implements code for calculating the baud rate for the Airoha
>> UART and HSUART.
>>
>> Signed-off-by: Benjamin Larsson <benjamin.larsson@genexis.eu>
>> ---
>> Changes in v4:
>> - Removed COMPILE_TEST from Kconfig
> 
> No, please don't do that. >
>> - Removed Kconfig option to build as module
> 
> No do that, you want your code as a module so that it can work in a
> system that is built as a "generic system image" that does not force
> your driver to be built into the main kernel image, wasting memory if
> the hardware is not present.
> 
> thanks,
> 
> greg k-h

I would argue that I follow the current flow of the code. In 8250.h we have:

CONFIG_SERIAL_8250_PNP
CONFIG_SERIAL_8250_RSA
CONFIG_SERIAL_8250_FINTEK

none of those enables COMPILE_TEST or the option to compile as a module.

Neither the Airoha code or the other code is not intended to be its own 
separate module, it is to be part of the 8250-driver. The 8250-driver 
can be loaded as a module with or without the Airoha baud rate code.

Implementing COMPILE_TEST when the 8250-driver does not support it seems 
tricky. All the ways I could think of would result in messy code and 
logic. I came to the conclusion that a smaller patch that reuses the 
current logic was preferable. If that argument is not good enough then I 
need some guidance how to implement something what would be accepted.

MvH
Benjamin Larsson

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] serial: Airoha SoC UART and HSUART support
  2025-02-09 21:02 ` [PATCH 2/2] serial: Airoha SoC UART and HSUART support Benjamin Larsson
@ 2025-03-22 21:42   ` Andy Shevchenko
  2025-03-25 14:19     ` Benjamin Larsson
  0 siblings, 1 reply; 11+ messages in thread
From: Andy Shevchenko @ 2025-03-22 21:42 UTC (permalink / raw)
  To: Benjamin Larsson; +Cc: linux-serial, devicetree, ansuelsmth, lorenzo, gregkh

Sun, Feb 09, 2025 at 10:02:41PM +0100, Benjamin Larsson kirjoitti:
> Support for Airoha AN7581 SoC UART and HSUART baud rate
> calculation routine.

...

>  drivers/tty/serial/8250/8250_port.c   | 26 +++++++++

> +	/*
> +	 * Airoha SoCs have custom registers for baud rate settings
> +	 */
> +	if (port->type == PORT_AIROHA)
> +		airoha8250_set_baud_rate(port, baud, 0);
> +	if (port->type == PORT_AIROHA_HS)
> +		airoha8250_set_baud_rate(port, baud, 1);

Why is this here? Please, make it stay in your module.

...

> +config SERIAL_8250_AIROHA
> +	bool "Airoha UART support"

Why bool?

> +	depends on ARCH_AIROHA && OF && SERIAL_8250

What is the purpose of the OF dependency?

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v4 0/2] Airoha UART support
  2025-03-21 20:37   ` Benjamin Larsson
@ 2025-03-22 21:45     ` Andy Shevchenko
  2025-03-25 14:45       ` Benjamin Larsson
  0 siblings, 1 reply; 11+ messages in thread
From: Andy Shevchenko @ 2025-03-22 21:45 UTC (permalink / raw)
  To: Benjamin Larsson; +Cc: Greg KH, linux-serial, devicetree, ansuelsmth, lorenzo

Fri, Mar 21, 2025 at 09:37:09PM +0100, Benjamin Larsson kirjoitti:
> On 10/02/2025 07:14, Greg KH wrote:
> > On Sun, Feb 09, 2025 at 10:02:39PM +0100, Benjamin Larsson wrote:

...

> I would argue that I follow the current flow of the code. In 8250.h we have:
> 
> CONFIG_SERIAL_8250_PNP
> CONFIG_SERIAL_8250_RSA

These are historically parts of the main driver, RSA code theoretically
can be removed.

> CONFIG_SERIAL_8250_FINTEK

I would love to see this being not part of main driver.

> none of those enables COMPILE_TEST or the option to compile as a module.

They all together may be compiled as a main driver module.
Again, this is all historical and new code would need a very good justification
why it can be held in a separate module.

> Neither the Airoha code or the other code is not intended to be its own
> separate module,

Why not?

>  it is to be part of the 8250-driver. The 8250-driver can be
> loaded as a module with or without the Airoha baud rate code.
> 
> Implementing COMPILE_TEST when the 8250-driver does not support it seems
> tricky. All the ways I could think of would result in messy code and logic.
> I came to the conclusion that a smaller patch that reuses the current logic
> was preferable. If that argument is not good enough then I need some
> guidance how to implement something what would be accepted.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] serial: Airoha SoC UART and HSUART support
  2025-03-22 21:42   ` Andy Shevchenko
@ 2025-03-25 14:19     ` Benjamin Larsson
  2025-03-25 14:23       ` Andy Shevchenko
  0 siblings, 1 reply; 11+ messages in thread
From: Benjamin Larsson @ 2025-03-25 14:19 UTC (permalink / raw)
  To: Andy Shevchenko; +Cc: linux-serial, devicetree, ansuelsmth, lorenzo, gregkh

Hi.

On 2025-03-22 22:42, Andy Shevchenko wrote:
> [You don't often get email from andy.shevchenko@gmail.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> Sun, Feb 09, 2025 at 10:02:41PM +0100, Benjamin Larsson kirjoitti:
>> Support for Airoha AN7581 SoC UART and HSUART baud rate
>> calculation routine.
> ...
>
>>   drivers/tty/serial/8250/8250_port.c   | 26 +++++++++
>> +     /*
>> +      * Airoha SoCs have custom registers for baud rate settings
>> +      */
>> +     if (port->type == PORT_AIROHA)
>> +             airoha8250_set_baud_rate(port, baud, 0);
>> +     if (port->type == PORT_AIROHA_HS)
>> +             airoha8250_set_baud_rate(port, baud, 1);
> Why is this here? Please, make it stay in your module.

I dont add an extra module I extend the already existing one.


> ...
>
>> +config SERIAL_8250_AIROHA
>> +     bool "Airoha UART support"
> Why bool?

Because it is just an extension of an existing module.


>
>> +     depends on ARCH_AIROHA && OF && SERIAL_8250
> What is the purpose of the OF dependency?

I thought it was needed for dts support. I'll remove it.


>
> --
> With Best Regards,
> Andy Shevchenko
>
>
MvH

Benajmin Larsson


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] serial: Airoha SoC UART and HSUART support
  2025-03-25 14:19     ` Benjamin Larsson
@ 2025-03-25 14:23       ` Andy Shevchenko
  0 siblings, 0 replies; 11+ messages in thread
From: Andy Shevchenko @ 2025-03-25 14:23 UTC (permalink / raw)
  To: Benjamin Larsson; +Cc: linux-serial, devicetree, ansuelsmth, lorenzo, gregkh

On Tue, Mar 25, 2025 at 4:19 PM Benjamin Larsson
<benjamin.larsson@genexis.eu> wrote:
> On 2025-03-22 22:42, Andy Shevchenko wrote:
> > Sun, Feb 09, 2025 at 10:02:41PM +0100, Benjamin Larsson kirjoitti:

...

> >>   drivers/tty/serial/8250/8250_port.c   | 26 +++++++++
> >> +     /*
> >> +      * Airoha SoCs have custom registers for baud rate settings
> >> +      */
> >> +     if (port->type == PORT_AIROHA)
> >> +             airoha8250_set_baud_rate(port, baud, 0);
> >> +     if (port->type == PORT_AIROHA_HS)
> >> +             airoha8250_set_baud_rate(port, baud, 1);
> > Why is this here? Please, make it stay in your module.
>
> I dont add an extra module I extend the already existing one.

We are trying hard to get rid of custom quirks in 8250_pci.c. That's
how a few 8250_*.c appeared during the past several years. Please,
follow the same approach.

...

> >> +config SERIAL_8250_AIROHA
> >> +     bool "Airoha UART support"
> > Why bool?
>
> Because it is just an extension of an existing module.

Please, make a new real module instead.

> >> +     depends on ARCH_AIROHA && OF && SERIAL_8250
> > What is the purpose of the OF dependency?
>
> I thought it was needed for dts support. I'll remove it.

It's a runtime dependency and not a compile time, right? So, there is
no impediment to compile test on the configurations that have
CONFIG_OF=n.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v4 0/2] Airoha UART support
  2025-03-22 21:45     ` Andy Shevchenko
@ 2025-03-25 14:45       ` Benjamin Larsson
  2025-03-25 14:54         ` Andy Shevchenko
  0 siblings, 1 reply; 11+ messages in thread
From: Benjamin Larsson @ 2025-03-25 14:45 UTC (permalink / raw)
  To: Andy Shevchenko; +Cc: Greg KH, linux-serial, devicetree, ansuelsmth, lorenzo

On 2025-03-22 22:45, Andy Shevchenko wrote:
> [You don't often get email from andy.shevchenko@gmail.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> Fri, Mar 21, 2025 at 09:37:09PM +0100, Benjamin Larsson kirjoitti:
>> On 10/02/2025 07:14, Greg KH wrote:
>>> On Sun, Feb 09, 2025 at 10:02:39PM +0100, Benjamin Larsson wrote:
> ...
>
>> I would argue that I follow the current flow of the code. In 8250.h we have:
>>
>> CONFIG_SERIAL_8250_PNP
>> CONFIG_SERIAL_8250_RSA
> These are historically parts of the main driver, RSA code theoretically
> can be removed.
>
>> CONFIG_SERIAL_8250_FINTEK
> I would love to see this being not part of main driver.
>
>> none of those enables COMPILE_TEST or the option to compile as a module.
> They all together may be compiled as a main driver module.
> Again, this is all historical and new code would need a very good justification
> why it can be held in a separate module.

Hmm, ok I'll see if I understand you. I should create 8250_airoha.c by 
using 8250_fsl.c as a template. Adapt probe for Airoha by replacing 
.set_termios with a wrapper that calls the custom baud rate calculation 
code?

>
>> Neither the Airoha code or the other code is not intended to be its own
>> separate module,
> Why not?

Well I mean that was my intent.


>
>>   it is to be part of the 8250-driver. The 8250-driver can be
>> loaded as a module with or without the Airoha baud rate code.
>>
>> Implementing COMPILE_TEST when the 8250-driver does not support it seems
>> tricky. All the ways I could think of would result in messy code and logic.
>> I came to the conclusion that a smaller patch that reuses the current logic
>> was preferable. If that argument is not good enough then I need some
>> guidance how to implement something what would be accepted.
> --
> With Best Regards,
> Andy Shevchenko
>
>
MvH

Benjamin Larsson


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v4 0/2] Airoha UART support
  2025-03-25 14:45       ` Benjamin Larsson
@ 2025-03-25 14:54         ` Andy Shevchenko
  0 siblings, 0 replies; 11+ messages in thread
From: Andy Shevchenko @ 2025-03-25 14:54 UTC (permalink / raw)
  To: Benjamin Larsson; +Cc: Greg KH, linux-serial, devicetree, ansuelsmth, lorenzo

On Tue, Mar 25, 2025 at 4:45 PM Benjamin Larsson
<benjamin.larsson@genexis.eu> wrote:
> On 2025-03-22 22:45, Andy Shevchenko wrote:
> > [You don't often get email from andy.shevchenko@gmail.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> > Fri, Mar 21, 2025 at 09:37:09PM +0100, Benjamin Larsson kirjoitti:
> >> On 10/02/2025 07:14, Greg KH wrote:
> >>> On Sun, Feb 09, 2025 at 10:02:39PM +0100, Benjamin Larsson wrote:

...

> >> I would argue that I follow the current flow of the code. In 8250.h we have:
> >>
> >> CONFIG_SERIAL_8250_PNP
> >> CONFIG_SERIAL_8250_RSA
> > These are historically parts of the main driver, RSA code theoretically
> > can be removed.
> >
> >> CONFIG_SERIAL_8250_FINTEK
> > I would love to see this being not part of main driver.
> >
> >> none of those enables COMPILE_TEST or the option to compile as a module.
> > They all together may be compiled as a main driver module.
> > Again, this is all historical and new code would need a very good justification
> > why it can be held in a separate module.
>
> Hmm, ok I'll see if I understand you. I should create 8250_airoha.c by
> using 8250_fsl.c as a template. Adapt probe for Airoha by replacing
> .set_termios with a wrapper that calls the custom baud rate calculation
> code?

Not sure that _fsl is the best example (we have so far: 8250_exar,
8250_lpss, 8250_mid, ...), but overall yes, that's my expectation.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2025-03-25 14:55 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-02-09 21:02 [PATCH v4 0/2] Airoha UART support Benjamin Larsson
2025-02-09 21:02 ` [PATCH 1/2] dt-bindings: serial: 8250: Add Airoha compatibles Benjamin Larsson
2025-02-09 21:02 ` [PATCH 2/2] serial: Airoha SoC UART and HSUART support Benjamin Larsson
2025-03-22 21:42   ` Andy Shevchenko
2025-03-25 14:19     ` Benjamin Larsson
2025-03-25 14:23       ` Andy Shevchenko
2025-02-10  6:14 ` [PATCH v4 0/2] Airoha UART support Greg KH
2025-03-21 20:37   ` Benjamin Larsson
2025-03-22 21:45     ` Andy Shevchenko
2025-03-25 14:45       ` Benjamin Larsson
2025-03-25 14:54         ` Andy Shevchenko

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox