* [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>
---
| 2 ++
1 file changed, 2 insertions(+)
--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>
---
| 15 +++++
| 81 +++++++++++++++++++++++++++
| 2 +
| 26 +++++++++
| 10 ++++
| 1 +
6 files changed, 135 insertions(+)
create mode 100644 drivers/tty/serial/8250/8250_airoha.c
--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);
--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);
+}
--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);
--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.
--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
--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