[PATCH v2 1/3] dt-bindings: serial: Add Loongson UART controller

[PATCH v2 1/3] dt-bindings: serial: Add Loongson UART controller

[zhenghaowei]

From: Haowei Zheng <zhenghaowei@loongson.cn>

Add Loongson UART controller binding with DT schema format using
json-schema.

Signed-off-by: Haowei Zheng <zhenghaowei@loongson.cn>
---
 .../bindings/serial/loongson,ls7a-uart.yaml   | 74 +++++++++++++++++++
 MAINTAINERS                                   |  7 ++
 2 files changed, 81 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/serial/loongson,ls7a-uart.yaml

diff --git a/Documentation/devicetree/bindings/serial/loongson,ls7a-uart.yaml b/Documentation/devicetree/bindings/serial/loongson,ls7a-uart.yaml
new file mode 100644
index 000000000000..22d9cca5569e
--- /dev/null
+++ b/Documentation/devicetree/bindings/serial/loongson,ls7a-uart.yaml
@@ -0,0 +1,74 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/loongson,ls7a-uart.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Loongson UART
+
+maintainers:
+  - Haowei Zheng <zhenghaowei@loongson.cn>
+
+properties:
+  compatible:
+    const: loongson,ls7a-uart
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  fractional-division:
+    description: Enables fractional-N division. Currently,
+      only LS2K1500 and LS2K2000 support this feature.
+
+  rts-invert:
+    description: Inverts the RTS value in the MCR register.
+      This should be used on Loongson-3 series CPUs, Loongson-2K
+      series CPUs, and Loongson LS7A bridge chips.
+
+  dtr-invert:
+    description: Inverts the DTR value in the MCR register.
+      This should be used on Loongson-3 series CPUs, Loongson-2K
+      series CPUs, and Loongson LS7A bridge chips.
+
+  cts-invert:
+    description: Inverts the CTS value in the MSR register.
+      This should be used on Loongson-2K0500, Loongson-2K1000,
+      and Loongson LS7A bridge chips.
+
+  dsr-invert:
+    description: Inverts the DSR value in the MSR register.
+      This should be used on Loongson-2K0500, Loongson-2K1000,
+      and Loongson LS7A bridge chips.
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+
+allOf:
+  - $ref: serial.yaml
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/clock/loongson,ls2k-clk.h>
+
+    serial@1fe001e0 {
+        compatible = "loongson,ls7a-uart";
+        reg = <0x0 0x1fe001e0 0x0 0x10>;
+        clock-frequency = <100000000>;
+        interrupt-parent = <&liointc>;
+        interrupts = <10 IRQ_TYPE_LEVEL_HIGH>;
+        fractional-division;
+        rts-invert;
+        dtr-invert;
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index 8766f3e5e87e..a6306327dba5 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -13189,6 +13189,13 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/i2c/loongson,ls2x-i2c.yaml
 F:	drivers/i2c/busses/i2c-ls2x.c
 
+LOONGSON UART DRIVER
+M:	Haowei Zheng <zhenghaowei@loongson.cn>
+L:	linux-serial@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/serial/loongson,ls7a-uart.yaml
+F:	drivers/tty/serial/8250/8250_loongson.c
+
 LOONGSON-2 SOC SERIES CLOCK DRIVER
 M:	Yinbo Zhu <zhuyinbo@loongson.cn>
 L:	linux-clk@vger.kernel.org
-- 
2.43.0

[PATCH v2 2/3] tty: serial: 8250: Add loongson uart driver support

[zhenghaowei]

From: Haowei Zheng <zhenghaowei@loongson.cn>

Due to certain hardware design challenges, we have opted to
utilize a dedicated UART driver to probe the UART interface.

Presently, we have defined four parameters — 'fractional-division',
'invert-rts', 'invert-dtr', 'invert-cts', and 'invert-dsr' — which
will be employed as needed.

Signed-off-by: Haowei Zheng <zhenghaowei@loongson.cn>
---
 drivers/tty/serial/8250/8250_loongson.c | 208 ++++++++++++++++++++++++
 drivers/tty/serial/8250/8250_port.c     |   8 +
 drivers/tty/serial/8250/Kconfig         |   9 +
 drivers/tty/serial/8250/Makefile        |   1 +
 include/uapi/linux/serial_core.h        |   1 +
 5 files changed, 227 insertions(+)
 create mode 100644 drivers/tty/serial/8250/8250_loongson.c

diff --git a/drivers/tty/serial/8250/8250_loongson.c b/drivers/tty/serial/8250/8250_loongson.c
new file mode 100644
index 000000000000..eb16677f1dde
--- /dev/null
+++ b/drivers/tty/serial/8250/8250_loongson.c
@@ -0,0 +1,208 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2020-2024 Loongson Technology Corporation Limited
+ */
+
+#include <linux/acpi.h>
+#include <linux/clk.h>
+#include <linux/console.h>
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/reset.h>
+
+#include "8250.h"
+
+struct loongson_uart_data {
+	struct reset_control *rst;
+	int line;
+	int mcr_invert;
+	int msr_invert;
+};
+
+static unsigned int serial_fixup(struct uart_port *p, unsigned int offset, unsigned int val)
+{
+	struct loongson_uart_data *data = p->private_data;
+
+	if (offset == UART_MCR)
+		val ^= data->mcr_invert;
+	if (offset == UART_MSR)
+		val ^= data->msr_invert;
+
+	return val;
+}
+
+static unsigned int loongson_serial_in(struct uart_port *p, int offset)
+{
+	unsigned int val, offset0 = offset;
+
+	offset = offset << p->regshift;
+	val = readb(p->membase + offset);
+
+	return serial_fixup(p, offset0, val);
+}
+
+static void loongson_serial_out(struct uart_port *p, int offset, int value)
+{
+	offset = offset << p->regshift;
+	writeb(serial_fixup(p, offset, value), p->membase + offset);
+}
+
+static unsigned int loongson_frac_get_divisor(struct uart_port *port,
+		unsigned int baud,
+		unsigned int *frac)
+{
+	unsigned int quot;
+
+	quot = DIV_ROUND_CLOSEST((port->uartclk << 4), baud);
+	*frac = quot & 0xff;
+
+	return quot >> 8;
+}
+
+static void loongson_frac_set_divisor(struct uart_port *port, unsigned int baud,
+		unsigned int quot, unsigned int quot_frac)
+{
+	struct uart_8250_port *up = up_to_u8250p(port);
+
+	serial_port_out(port, UART_LCR, up->lcr | UART_LCR_DLAB);
+
+	serial_dl_write(up, quot);
+
+	serial_port_out(port, 0x2, quot_frac);
+}
+
+static int loongson_uart_probe(struct platform_device *pdev)
+{
+	struct uart_8250_port uart = {};
+	struct loongson_uart_data *data;
+	struct uart_port *port;
+	struct resource *res;
+	int ret;
+
+	port = &uart.port;
+	spin_lock_init(&port->lock);
+
+	port->flags		= UPF_SHARE_IRQ | UPF_FIXED_PORT | UPF_FIXED_TYPE;
+	port->iotype		= UPIO_MEM;
+	port->regshift		= 0;
+	port->dev		= &pdev->dev;
+	port->type		= (unsigned long)device_get_match_data(&pdev->dev);
+	port->serial_in		= loongson_serial_in;
+	port->serial_out	= loongson_serial_out;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res)
+		return -ENODEV;
+
+	port->membase = devm_ioremap(&pdev->dev, res->start, resource_size(res));
+	if (!port->membase)
+		return -ENOMEM;
+
+	port->mapbase = res->start;
+	port->mapsize = resource_size(res);
+
+	port->irq = platform_get_irq(pdev, 0);
+	if (port->irq < 0)
+		return -EINVAL;
+
+	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	port->private_data = data;
+
+	if (device_property_read_bool(&pdev->dev, "fractional-division")) {
+		port->get_divisor = loongson_frac_get_divisor;
+		port->set_divisor = loongson_frac_set_divisor;
+	}
+
+	if (device_property_read_bool(&pdev->dev, "rts-invert"))
+		data->mcr_invert |= UART_MCR_RTS;
+
+	if (device_property_read_bool(&pdev->dev, "dtr-invert"))
+		data->mcr_invert |= UART_MCR_DTR;
+
+	if (device_property_read_bool(&pdev->dev, "cts-invert"))
+		data->msr_invert |= UART_MSR_CTS;
+
+	if (device_property_read_bool(&pdev->dev, "dsr-invert"))
+		data->msr_invert |= UART_MSR_DSR;
+
+	data->rst = devm_reset_control_get_optional_shared(&pdev->dev, NULL);
+	if (IS_ERR(data->rst))
+		return PTR_ERR(data->rst);
+
+	device_property_read_u32(&pdev->dev, "clock-frequency", &port->uartclk);
+
+	ret = reset_control_deassert(data->rst);
+	if (ret)
+		goto err_unprepare;
+
+	ret = serial8250_register_8250_port(&uart);
+	if (ret < 0)
+		goto err_unprepare;
+
+	platform_set_drvdata(pdev, data);
+	data->line = ret;
+
+	return 0;
+
+err_unprepare:
+
+	return ret;
+}
+
+static void loongson_uart_remove(struct platform_device *pdev)
+{
+	struct loongson_uart_data *data = platform_get_drvdata(pdev);
+
+	serial8250_unregister_port(data->line);
+	reset_control_assert(data->rst);
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int loongson_uart_suspend(struct device *dev)
+{
+	struct loongson_uart_data *data = dev_get_drvdata(dev);
+
+	serial8250_suspend_port(data->line);
+
+	return 0;
+}
+
+static int loongson_uart_resume(struct device *dev)
+{
+	struct loongson_uart_data *data = dev_get_drvdata(dev);
+
+	serial8250_resume_port(data->line);
+
+	return 0;
+}
+#endif
+
+static const struct dev_pm_ops loongson_uart_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(loongson_uart_suspend, loongson_uart_resume)
+};
+
+static const struct of_device_id of_platform_serial_table[] = {
+	{.compatible = "loongson,ls7a-uart", .data = (void *)PORT_LOONGSON},
+	{},
+};
+MODULE_DEVICE_TABLE(of, of_platform_serial_table);
+
+static struct platform_driver loongson_uart_driver = {
+	.probe = loongson_uart_probe,
+	.remove = loongson_uart_remove,
+	.driver = {
+		.name = "ls7a-uart",
+		.pm = &loongson_uart_pm_ops,
+		.of_match_table = of_match_ptr(of_platform_serial_table),
+	},
+};
+
+module_platform_driver(loongson_uart_driver);
+
+MODULE_DESCRIPTION("LOONGSON 8250 Driver");
+MODULE_AUTHOR("Haowei Zheng <zhenghaowei@loongson.cn>");
+MODULE_LICENSE("GPL");
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 2786918aea98..60b72c785028 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -319,6 +319,14 @@ static const struct serial8250_config uart_config[] = {
 		.rxtrig_bytes	= {1, 8, 16, 30},
 		.flags		= UART_CAP_FIFO | UART_CAP_AFE,
 	},
+	[PORT_LOONGSON] = {
+		.name		= "Loongson",
+		.fifo_size	= 16,
+		.tx_loadsz	= 16,
+		.fcr		= UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_10,
+		.rxtrig_bytes   = {1, 4, 8, 14},
+		.flags		= UART_CAP_FIFO,
+	},
 };
 
 /* Uart divisor latch read */
diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
index 47ff50763c04..a696afc4f8a8 100644
--- a/drivers/tty/serial/8250/Kconfig
+++ b/drivers/tty/serial/8250/Kconfig
@@ -568,6 +568,15 @@ config SERIAL_8250_BCM7271
 	  including DMA support and high accuracy BAUD rates, say
 	  Y to this option. If unsure, say N.
 
+config SERIAL_8250_LOONGSON
+	tristate "Loongson 8250 serial port support"
+	default SERIAL_8250
+	depends on SERIAL_8250
+	depends on LOONGARCH || MIPS
+	help
+	  If you have machine with Loongson and want to use this serial driver,
+	  say Y to this option. If unsure, say N.
+
 config SERIAL_OF_PLATFORM
 	tristate "Devicetree based probing for 8250 ports"
 	depends on SERIAL_8250 && OF
diff --git a/drivers/tty/serial/8250/Makefile b/drivers/tty/serial/8250/Makefile
index 1516de629b61..e9587bf69f65 100644
--- a/drivers/tty/serial/8250/Makefile
+++ b/drivers/tty/serial/8250/Makefile
@@ -51,5 +51,6 @@ obj-$(CONFIG_SERIAL_8250_RT288X)	+= 8250_rt288x.o
 obj-$(CONFIG_SERIAL_8250_CS)		+= serial_cs.o
 obj-$(CONFIG_SERIAL_8250_UNIPHIER)	+= 8250_uniphier.o
 obj-$(CONFIG_SERIAL_8250_TEGRA)		+= 8250_tegra.o
+obj-$(CONFIG_SERIAL_8250_LOONGSON)	+= 8250_loongson.o
 
 CFLAGS_8250_ingenic.o += -I$(srctree)/scripts/dtc/libfdt
diff --git a/include/uapi/linux/serial_core.h b/include/uapi/linux/serial_core.h
index 9c007a106330..9e316b9295e5 100644
--- a/include/uapi/linux/serial_core.h
+++ b/include/uapi/linux/serial_core.h
@@ -31,6 +31,7 @@
 #define PORT_ALTR_16550_F128 28 /* Altera 16550 UART with 128 FIFOs */
 #define PORT_RT2880	29	/* Ralink RT2880 internal UART */
 #define PORT_16550A_FSL64 30	/* Freescale 16550 UART with 64 FIFOs */
+#define PORT_LOONGSON   31      /* Loongson 16550 UART*/
 
 /*
  * ARM specific type numbers.  These are not currently guaranteed
-- 
2.43.0

Re: [PATCH v2 2/3] tty: serial: 8250: Add loongson uart driver support

[Christophe JAILLET]

Le 04/08/2024 à 08:38, 
zhenghaowei-cXZgJK919ebM1kAEIRd3EQ@public.gmane.org a écrit :
> From: Haowei Zheng <zhenghaowei-cXZgJK919ebM1kAEIRd3EQ@public.gmane.org>
> 
> Due to certain hardware design challenges, we have opted to
> utilize a dedicated UART driver to probe the UART interface.
> 
> Presently, we have defined four parameters — 'fractional-division',
> 'invert-rts', 'invert-dtr', 'invert-cts', and 'invert-dsr' — which
> will be employed as needed.
> 
> Signed-off-by: Haowei Zheng <zhenghaowei-cXZgJK919ebM1kAEIRd3EQ@public.gmane.org>
> ---

Hi,

...

> +	data->rst = devm_reset_control_get_optional_shared(&pdev->dev, NULL);
> +	if (IS_ERR(data->rst))
> +		return PTR_ERR(data->rst);
> +
> +	device_property_read_u32(&pdev->dev, "clock-frequency", &port->uartclk);
> +
> +	ret = reset_control_deassert(data->rst);
> +	if (ret)
> +		goto err_unprepare;

Should reset_control_assert() be called if an error occurs later?

> +
> +	ret = serial8250_register_8250_port(&uart);
> +	if (ret < 0)
> +		goto err_unprepare;
> +
> +	platform_set_drvdata(pdev, data);
> +	data->line = ret;
> +
> +	return 0;
> +
> +err_unprepare:

What is this label for?

> +
> +	return ret;
> +}

CJ

Re: [PATCH v2 2/3] tty: serial: 8250: Add loongson uart driver support

[kernel test robot]

Hi,

kernel test robot noticed the following build errors:

[auto build test ERROR on tty/tty-testing]
[also build test ERROR on tty/tty-next tty/tty-linus robh/for-next usb/usb-testing usb/usb-next usb/usb-linus linus/master v6.11-rc1 next-20240802]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/zhenghaowei-loongson-cn/tty-serial-8250-Add-loongson-uart-driver-support/20240804-145047
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing
patch link:    https://lore.kernel.org/r/20240804063834.70022-2-zhenghaowei%40loongson.cn
patch subject: [PATCH v2 2/3] tty: serial: 8250: Add loongson uart driver support
config: mips-gpr_defconfig (https://download.01.org/0day-ci/archive/20240804/202408042241.zkkSuA60-lkp@intel.com/config)
compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project 423aec6573df4424f90555468128e17073ddc69e)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240804/202408042241.zkkSuA60-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202408042241.zkkSuA60-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from drivers/tty/serial/8250/8250_loongson.c:14:
   In file included from drivers/tty/serial/8250/8250.h:11:
   In file included from include/linux/serial_8250.h:11:
   In file included from include/linux/serial_core.h:16:
   In file included from include/linux/tty.h:11:
   In file included from include/linux/tty_port.h:5:
   In file included from include/linux/kfifo.h:40:
   In file included from include/linux/dma-mapping.h:11:
   In file included from include/linux/scatterlist.h:8:
   In file included from include/linux/mm.h:2228:
   include/linux/vmstat.h:514:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
     514 |         return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
         |                               ~~~~~~~~~~~ ^ ~~~
>> drivers/tty/serial/8250/8250_loongson.c:200:21: error: call to undeclared function 'of_match_ptr'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
     200 |                 .of_match_table = of_match_ptr(of_platform_serial_table),
         |                                   ^
>> drivers/tty/serial/8250/8250_loongson.c:200:21: error: incompatible integer to pointer conversion initializing 'const struct of_device_id *' with an expression of type 'int' [-Wint-conversion]
     200 |                 .of_match_table = of_match_ptr(of_platform_serial_table),
         |                                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/tty/serial/8250/8250_loongson.c:200:21: error: initializer element is not a compile-time constant
     200 |                 .of_match_table = of_match_ptr(of_platform_serial_table),
         |                                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   1 warning and 3 errors generated.


vim +/of_match_ptr +200 drivers/tty/serial/8250/8250_loongson.c

   193	
   194	static struct platform_driver loongson_uart_driver = {
   195		.probe = loongson_uart_probe,
   196		.remove = loongson_uart_remove,
   197		.driver = {
   198			.name = "ls7a-uart",
   199			.pm = &loongson_uart_pm_ops,
 > 200			.of_match_table = of_match_ptr(of_platform_serial_table),
   201		},
   202	};
   203	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v2 2/3] tty: serial: 8250: Add loongson uart driver support

[Krzysztof Kozlowski]

On 04/08/2024 08:38, zhenghaowei@loongson.cn wrote:
> From: Haowei Zheng <zhenghaowei@loongson.cn>
> 
> Due to certain hardware design challenges, we have opted to
> utilize a dedicated UART driver to probe the UART interface.
> 
> Presently, we have defined four parameters — 'fractional-division',
> 'invert-rts', 'invert-dtr', 'invert-cts', and 'invert-dsr' — which
> will be employed as needed.
> 
> Signed-off-by: Haowei Zheng <zhenghaowei@loongson.cn>
> ---
>  drivers/tty/serial/8250/8250_loongson.c | 208 ++++++++++++++++++++++++
>  drivers/tty/serial/8250/8250_port.c     |   8 +
>  drivers/tty/serial/8250/Kconfig         |   9 +
>  drivers/tty/serial/8250/Makefile        |   1 +
>  include/uapi/linux/serial_core.h        |   1 +
>  5 files changed, 227 insertions(+)
>  create mode 100644 drivers/tty/serial/8250/8250_loongson.c
> 
> diff --git a/drivers/tty/serial/8250/8250_loongson.c b/drivers/tty/serial/8250/8250_loongson.c
> new file mode 100644
> index 000000000000..eb16677f1dde
> --- /dev/null
> +++ b/drivers/tty/serial/8250/8250_loongson.c
> @@ -0,0 +1,208 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2020-2024 Loongson Technology Corporation Limited
> + */
> +
> +#include <linux/acpi.h>

How is this used?

> +#include <linux/clk.h>

And this?

> +#include <linux/console.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/reset.h>
> +
> +#include "8250.h"
> +
> +struct loongson_uart_data {
> +	struct reset_control *rst;
> +	int line;
> +	int mcr_invert;
> +	int msr_invert;
> +};

...

> +static int loongson_uart_probe(struct platform_device *pdev)
> +{
> +	struct uart_8250_port uart = {};
> +	struct loongson_uart_data *data;
> +	struct uart_port *port;
> +	struct resource *res;
> +	int ret;
> +
> +	port = &uart.port;
> +	spin_lock_init(&port->lock);
> +
> +	port->flags		= UPF_SHARE_IRQ | UPF_FIXED_PORT | UPF_FIXED_TYPE;
> +	port->iotype		= UPIO_MEM;
> +	port->regshift		= 0;
> +	port->dev		= &pdev->dev;
> +	port->type		= (unsigned long)device_get_match_data(&pdev->dev);
> +	port->serial_in		= loongson_serial_in;
> +	port->serial_out	= loongson_serial_out;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res)
> +		return -ENODEV;
> +
> +	port->membase = devm_ioremap(&pdev->dev, res->start, resource_size(res));
> +	if (!port->membase)
> +		return -ENOMEM;
> +

Use wrapper combining both calls.

> +	port->mapbase = res->start;
> +	port->mapsize = resource_size(res);
> +
> +	port->irq = platform_get_irq(pdev, 0);
> +	if (port->irq < 0)
> +		return -EINVAL;
> +
> +	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	port->private_data = data;
> +
> +	if (device_property_read_bool(&pdev->dev, "fractional-division")) {
> +		port->get_divisor = loongson_frac_get_divisor;
> +		port->set_divisor = loongson_frac_set_divisor;
> +	}
> +
> +	if (device_property_read_bool(&pdev->dev, "rts-invert"))
> +		data->mcr_invert |= UART_MCR_RTS;
> +
> +	if (device_property_read_bool(&pdev->dev, "dtr-invert"))
> +		data->mcr_invert |= UART_MCR_DTR;
> +
> +	if (device_property_read_bool(&pdev->dev, "cts-invert"))
> +		data->msr_invert |= UART_MSR_CTS;
> +
> +	if (device_property_read_bool(&pdev->dev, "dsr-invert"))
> +		data->msr_invert |= UART_MSR_DSR;
> +
> +	data->rst = devm_reset_control_get_optional_shared(&pdev->dev, NULL);
> +	if (IS_ERR(data->rst))
> +		return PTR_ERR(data->rst);
> +
> +	device_property_read_u32(&pdev->dev, "clock-frequency", &port->uartclk);
> +
> +	ret = reset_control_deassert(data->rst);
> +	if (ret)
> +		goto err_unprepare;
> +
> +	ret = serial8250_register_8250_port(&uart);
> +	if (ret < 0)
> +		goto err_unprepare;
> +
> +	platform_set_drvdata(pdev, data);
> +	data->line = ret;
> +
> +	return 0;
> +
> +err_unprepare:
> +
> +	return ret;
> +}
> +
> +static void loongson_uart_remove(struct platform_device *pdev)
> +{
> +	struct loongson_uart_data *data = platform_get_drvdata(pdev);
> +
> +	serial8250_unregister_port(data->line);
> +	reset_control_assert(data->rst);
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int loongson_uart_suspend(struct device *dev)
> +{
> +	struct loongson_uart_data *data = dev_get_drvdata(dev);
> +
> +	serial8250_suspend_port(data->line);
> +
> +	return 0;
> +}
> +
> +static int loongson_uart_resume(struct device *dev)
> +{
> +	struct loongson_uart_data *data = dev_get_drvdata(dev);
> +
> +	serial8250_resume_port(data->line);
> +
> +	return 0;
> +}
> +#endif
> +
> +static const struct dev_pm_ops loongson_uart_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(loongson_uart_suspend, loongson_uart_resume)
> +};
> +
> +static const struct of_device_id of_platform_serial_table[] = {
> +	{.compatible = "loongson,ls7a-uart", .data = (void *)PORT_LOONGSON},

Why do you need match data if there is no choice?

> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, of_platform_serial_table);
> +
> +static struct platform_driver loongson_uart_driver = {
> +	.probe = loongson_uart_probe,
> +	.remove = loongson_uart_remove,
> +	.driver = {
> +		.name = "ls7a-uart",
> +		.pm = &loongson_uart_pm_ops,
> +		.of_match_table = of_match_ptr(of_platform_serial_table),

Except that this does not build... drop of_match_ptr(), not needed and
causes warnings.

> +	},
> +};
> +
> +module_platform_driver(loongson_uart_driver);
> +
> +MODULE_DESCRIPTION("LOONGSON 8250 Driver");
> +MODULE_AUTHOR("Haowei Zheng <zhenghaowei@loongson.cn>");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index 2786918aea98..60b72c785028 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -319,6 +319,14 @@ static const struct serial8250_config uart_config[] = {
>  		.rxtrig_bytes	= {1, 8, 16, 30},
>  		.flags		= UART_CAP_FIFO | UART_CAP_AFE,
>  	},
> +	[PORT_LOONGSON] = {
> +		.name		= "Loongson",
> +		.fifo_size	= 16,
> +		.tx_loadsz	= 16,
> +		.fcr		= UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_10,
> +		.rxtrig_bytes   = {1, 4, 8, 14},
> +		.flags		= UART_CAP_FIFO,
> +	},
>  };
>  
>  /* Uart divisor latch read */
> diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
> index 47ff50763c04..a696afc4f8a8 100644
> --- a/drivers/tty/serial/8250/Kconfig
> +++ b/drivers/tty/serial/8250/Kconfig
> @@ -568,6 +568,15 @@ config SERIAL_8250_BCM7271
>  	  including DMA support and high accuracy BAUD rates, say
>  	  Y to this option. If unsure, say N.
>  
> +config SERIAL_8250_LOONGSON
> +	tristate "Loongson 8250 serial port support"
> +	default SERIAL_8250
> +	depends on SERIAL_8250
> +	depends on LOONGARCH || MIPS

MIPS? Why?

You also miss COMPILE_TEST.



Best regards,
Krzysztof

Re: [PATCH v2 2/3] tty: serial: 8250: Add loongson uart driver support

[郑豪威]

在 2024/8/4 23:33, Krzysztof Kozlowski 写道:
> On 04/08/2024 08:38,zhenghaowei@loongson.cn wrote:
>> From: Haowei Zheng<zhenghaowei@loongson.cn>
>>
>> Due to certain hardware design challenges, we have opted to
>> utilize a dedicated UART driver to probe the UART interface.
>>
>> Presently, we have defined four parameters — 'fractional-division',
>> 'invert-rts', 'invert-dtr', 'invert-cts', and 'invert-dsr' — which
>> will be employed as needed.
>>
>> Signed-off-by: Haowei Zheng<zhenghaowei@loongson.cn>
>> ---
>>   drivers/tty/serial/8250/8250_loongson.c | 208 ++++++++++++++++++++++++
>>   drivers/tty/serial/8250/8250_port.c     |   8 +
>>   drivers/tty/serial/8250/Kconfig         |   9 +
>>   drivers/tty/serial/8250/Makefile        |   1 +
>>   include/uapi/linux/serial_core.h        |   1 +
>>   5 files changed, 227 insertions(+)
>>   create mode 100644 drivers/tty/serial/8250/8250_loongson.c
>>
>> diff --git a/drivers/tty/serial/8250/8250_loongson.c b/drivers/tty/serial/8250/8250_loongson.c
>> new file mode 100644
>> index 000000000000..eb16677f1dde
>> --- /dev/null
>> +++ b/drivers/tty/serial/8250/8250_loongson.c
>> @@ -0,0 +1,208 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * Copyright (C) 2020-2024 Loongson Technology Corporation Limited
>> + */
>> +
>> +#include <linux/acpi.h>
> How is this used?

I forgot to drop it, Before this, when the kernel was booted in ACPI 
mode, we used acpi_match_table

for driver registration. To maintain code simplicity, now we use 
"PRP0001" for driver registration, so we

don't need 'acpi.h' anymore.

>> +#include <linux/clk.h>
> And this?
Currently, it doesn't seem to serve much purpose, and I will remove it 
in the next version.
>> +#include <linux/console.h>
>> +#include <linux/delay.h>
>> +#include <linux/io.h>
>> +#include <linux/module.h>
>> +#include <linux/reset.h>
>> +
>> +#include "8250.h"
>> +
>> +struct loongson_uart_data {
>> +	struct reset_control *rst;
>> +	int line;
>> +	int mcr_invert;
>> +	int msr_invert;
>> +};
> ...
>
>> +static int loongson_uart_probe(struct platform_device *pdev)
>> +{
>> +	struct uart_8250_port uart = {};
>> +	struct loongson_uart_data *data;
>> +	struct uart_port *port;
>> +	struct resource *res;
>> +	int ret;
>> +
>> +	port = &uart.port;
>> +	spin_lock_init(&port->lock);
>> +
>> +	port->flags		= UPF_SHARE_IRQ | UPF_FIXED_PORT | UPF_FIXED_TYPE;
>> +	port->iotype		= UPIO_MEM;
>> +	port->regshift		= 0;
>> +	port->dev		= &pdev->dev;
>> +	port->type		= (unsigned long)device_get_match_data(&pdev->dev);
>> +	port->serial_in		= loongson_serial_in;
>> +	port->serial_out	= loongson_serial_out;
>> +
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	if (!res)
>> +		return -ENODEV;
>> +
>> +	port->membase = devm_ioremap(&pdev->dev, res->start, resource_size(res));
>> +	if (!port->membase)
>> +		return -ENOMEM;
>> +
> Use wrapper combining both calls.

I got it, did you mean like this?

+    res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+    if (!res)
+        return -ENODEV;
+
+    port->mapbase = res->start;
+    port->mapsize = resource_size(res);
+
+    port->membase = devm_ioremap(&pdev->dev, port->mapbase, port->mapsize);
+    if (!port->membase)
  +       return -ENOMEM;

>> +	port->mapbase = res->start;
>> +	port->mapsize = resource_size(res);
>> +
>> +	port->irq = platform_get_irq(pdev, 0);
>> +	if (port->irq < 0)
>> +		return -EINVAL;
>> +
>> +	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
>> +	if (!data)
>> +		return -ENOMEM;
>> +
>> +	port->private_data = data;
>> +
>> +	if (device_property_read_bool(&pdev->dev, "fractional-division")) {
>> +		port->get_divisor = loongson_frac_get_divisor;
>> +		port->set_divisor = loongson_frac_set_divisor;
>> +	}
>> +
>> +	if (device_property_read_bool(&pdev->dev, "rts-invert"))
>> +		data->mcr_invert |= UART_MCR_RTS;
>> +
>> +	if (device_property_read_bool(&pdev->dev, "dtr-invert"))
>> +		data->mcr_invert |= UART_MCR_DTR;
>> +
>> +	if (device_property_read_bool(&pdev->dev, "cts-invert"))
>> +		data->msr_invert |= UART_MSR_CTS;
>> +
>> +	if (device_property_read_bool(&pdev->dev, "dsr-invert"))
>> +		data->msr_invert |= UART_MSR_DSR;
>> +
>> +	data->rst = devm_reset_control_get_optional_shared(&pdev->dev, NULL);
>> +	if (IS_ERR(data->rst))
>> +		return PTR_ERR(data->rst);
>> +
>> +	device_property_read_u32(&pdev->dev, "clock-frequency", &port->uartclk);
>> +
>> +	ret = reset_control_deassert(data->rst);
>> +	if (ret)
>> +		goto err_unprepare;
>> +
>> +	ret = serial8250_register_8250_port(&uart);
>> +	if (ret < 0)
>> +		goto err_unprepare;
>> +
>> +	platform_set_drvdata(pdev, data);
>> +	data->line = ret;
>> +
>> +	return 0;
>> +
>> +err_unprepare:
>> +
>> +	return ret;
>> +}
>> +
>> +static void loongson_uart_remove(struct platform_device *pdev)
>> +{
>> +	struct loongson_uart_data *data = platform_get_drvdata(pdev);
>> +
>> +	serial8250_unregister_port(data->line);
>> +	reset_control_assert(data->rst);
>> +}
>> +
>> +#ifdef CONFIG_PM_SLEEP
>> +static int loongson_uart_suspend(struct device *dev)
>> +{
>> +	struct loongson_uart_data *data = dev_get_drvdata(dev);
>> +
>> +	serial8250_suspend_port(data->line);
>> +
>> +	return 0;
>> +}
>> +
>> +static int loongson_uart_resume(struct device *dev)
>> +{
>> +	struct loongson_uart_data *data = dev_get_drvdata(dev);
>> +
>> +	serial8250_resume_port(data->line);
>> +
>> +	return 0;
>> +}
>> +#endif
>> +
>> +static const struct dev_pm_ops loongson_uart_pm_ops = {
>> +	SET_SYSTEM_SLEEP_PM_OPS(loongson_uart_suspend, loongson_uart_resume)
>> +};
>> +
>> +static const struct of_device_id of_platform_serial_table[] = {
>> +	{.compatible = "loongson,ls7a-uart", .data = (void *)PORT_LOONGSON},
> Why do you need match data if there is no choice?

Considering whether new port types might be added in the future.

Of course, currently it doesn't seem necessary to do so.

>> +	{},
>> +};
>> +MODULE_DEVICE_TABLE(of, of_platform_serial_table);
>> +
>> +static struct platform_driver loongson_uart_driver = {
>> +	.probe = loongson_uart_probe,
>> +	.remove = loongson_uart_remove,
>> +	.driver = {
>> +		.name = "ls7a-uart",
>> +		.pm = &loongson_uart_pm_ops,
>> +		.of_match_table = of_match_ptr(of_platform_serial_table),
> Except that this does not build... drop of_match_ptr(), not needed and
> causes warnings.
>
Ok, I got it.
>> +	},
>> +};
>> +
>> +module_platform_driver(loongson_uart_driver);
>> +
>> +MODULE_DESCRIPTION("LOONGSON 8250 Driver");
>> +MODULE_AUTHOR("Haowei Zheng<zhenghaowei@loongson.cn>");
>> +MODULE_LICENSE("GPL");
>> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
>> index 2786918aea98..60b72c785028 100644
>> --- a/drivers/tty/serial/8250/8250_port.c
>> +++ b/drivers/tty/serial/8250/8250_port.c
>> @@ -319,6 +319,14 @@ static const struct serial8250_config uart_config[] = {
>>   		.rxtrig_bytes	= {1, 8, 16, 30},
>>   		.flags		= UART_CAP_FIFO | UART_CAP_AFE,
>>   	},
>> +	[PORT_LOONGSON] = {
>> +		.name		= "Loongson",
>> +		.fifo_size	= 16,
>> +		.tx_loadsz	= 16,
>> +		.fcr		= UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_10,
>> +		.rxtrig_bytes   = {1, 4, 8, 14},
>> +		.flags		= UART_CAP_FIFO,
>> +	},
>>   };
>>   
>>   /* Uart divisor latch read */
>> diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
>> index 47ff50763c04..a696afc4f8a8 100644
>> --- a/drivers/tty/serial/8250/Kconfig
>> +++ b/drivers/tty/serial/8250/Kconfig
>> @@ -568,6 +568,15 @@ config SERIAL_8250_BCM7271
>>   	  including DMA support and high accuracy BAUD rates, say
>>   	  Y to this option. If unsure, say N.
>>   
>> +config SERIAL_8250_LOONGSON
>> +	tristate "Loongson 8250 serial port support"
>> +	default SERIAL_8250
>> +	depends on SERIAL_8250
>> +	depends on LOONGARCH || MIPS
> MIPS? Why?
>
> You also miss COMPILE_TEST.
>
>
>
> Best regards,
> Krzysztof

The addition of mips was intended to maintain compatibility with 
loongson-3a4000 and earlier chips.

Currently, it appears that this lacks sufficient validation, and I will 
remove it in the next version.


I compiled and verified it on the Loongson 3A6000 machine, and currently 
it seems to have issues.

I will fix the compilation problem in the next version.


Best regards,

Haowei Zheng

Re: [PATCH v2 2/3] tty: serial: 8250: Add loongson uart driver support

[Krzysztof Kozlowski]

On 07/08/2024 10:24, 郑豪威 wrote:
>>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>> +	if (!res)
>>> +		return -ENODEV;
>>> +
>>> +	port->membase = devm_ioremap(&pdev->dev, res->start, resource_size(res));
>>> +	if (!port->membase)
>>> +		return -ENOMEM;
>>> +
>> Use wrapper combining both calls.
> 
> I got it, did you mean like this?
> 
> +    res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +    if (!res)
> +        return -ENODEV;
> +
> +    port->mapbase = res->start;
> +    port->mapsize = resource_size(res);
> +
> +    port->membase = devm_ioremap(&pdev->dev, port->mapbase, port->mapsize);
> +    if (!port->membase)
>   +       return -ENOMEM;

I still see two calls, so how are they combined? Just open any other
file and see how it is done there.


Best regards,
Krzysztof

Re: [PATCH v2 2/3] tty: serial: 8250: Add loongson uart driver support

[kernel test robot]

Hi,

kernel test robot noticed the following build errors:

[auto build test ERROR on tty/tty-testing]
[also build test ERROR on tty/tty-next tty/tty-linus robh/for-next usb/usb-testing usb/usb-next usb/usb-linus linus/master v6.11-rc1 next-20240802]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/zhenghaowei-loongson-cn/tty-serial-8250-Add-loongson-uart-driver-support/20240804-145047
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing
patch link:    https://lore.kernel.org/r/20240804063834.70022-2-zhenghaowei%40loongson.cn
patch subject: [PATCH v2 2/3] tty: serial: 8250: Add loongson uart driver support
config: mips-lemote2f_defconfig (https://download.01.org/0day-ci/archive/20240805/202408050031.dYYkSqDM-lkp@intel.com/config)
compiler: mips64el-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240805/202408050031.dYYkSqDM-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202408050031.dYYkSqDM-lkp@intel.com/

All error/warnings (new ones prefixed by >>):

>> drivers/tty/serial/8250/8250_loongson.c:200:35: error: implicit declaration of function 'of_match_ptr' [-Werror=implicit-function-declaration]
     200 |                 .of_match_table = of_match_ptr(of_platform_serial_table),
         |                                   ^~~~~~~~~~~~
>> drivers/tty/serial/8250/8250_loongson.c:200:35: warning: initialization of 'const struct of_device_id *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
   drivers/tty/serial/8250/8250_loongson.c:200:35: note: (near initialization for 'loongson_uart_driver.driver.of_match_table')
>> drivers/tty/serial/8250/8250_loongson.c:200:35: error: initializer element is not constant
   drivers/tty/serial/8250/8250_loongson.c:200:35: note: (near initialization for 'loongson_uart_driver.driver.of_match_table')
   cc1: some warnings being treated as errors


vim +/of_match_ptr +200 drivers/tty/serial/8250/8250_loongson.c

   193	
   194	static struct platform_driver loongson_uart_driver = {
   195		.probe = loongson_uart_probe,
   196		.remove = loongson_uart_remove,
   197		.driver = {
   198			.name = "ls7a-uart",
   199			.pm = &loongson_uart_pm_ops,
 > 200			.of_match_table = of_match_ptr(of_platform_serial_table),
   201		},
   202	};
   203	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH v2 3/3] LoongArch: dts: Update UART driver to Loongson-2K0500, Loongson-2K1000 and Loongson-2K2000.

[zhenghaowei]

From: Haowei Zheng <zhenghaowei@loongson.cn>

Change to use the Loongson UART driver by default.

Signed-off-by: Haowei Zheng <zhenghaowei@loongson.cn>
---
 arch/loongarch/boot/dts/loongson-2k0500.dtsi | 6 +++++-
 arch/loongarch/boot/dts/loongson-2k1000.dtsi | 6 +++++-
 arch/loongarch/boot/dts/loongson-2k2000.dtsi | 5 ++++-
 3 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/arch/loongarch/boot/dts/loongson-2k0500.dtsi b/arch/loongarch/boot/dts/loongson-2k0500.dtsi
index 3b38ff8853a7..aba6c0991b36 100644
--- a/arch/loongarch/boot/dts/loongson-2k0500.dtsi
+++ b/arch/loongarch/boot/dts/loongson-2k0500.dtsi
@@ -220,12 +220,16 @@ tsensor: thermal-sensor@1fe11500 {
 		};
 
 		uart0: serial@1ff40800 {
-			compatible = "ns16550a";
+			compatible = "loongson,ls7a-uart";
 			reg = <0x0 0x1ff40800 0x0 0x10>;
 			clock-frequency = <100000000>;
 			interrupt-parent = <&eiointc>;
 			interrupts = <2>;
 			no-loopback-test;
+			rts-invert;
+			dtr-invert;
+			cts-invert;
+			dsr-invert;
 			status = "disabled";
 		};
 
diff --git a/arch/loongarch/boot/dts/loongson-2k1000.dtsi b/arch/loongarch/boot/dts/loongson-2k1000.dtsi
index 92180140eb56..44c57d2e5dc2 100644
--- a/arch/loongarch/boot/dts/loongson-2k1000.dtsi
+++ b/arch/loongarch/boot/dts/loongson-2k1000.dtsi
@@ -297,12 +297,16 @@ dma-controller@1fe00c40 {
 		};
 
 		uart0: serial@1fe20000 {
-			compatible = "ns16550a";
+			compatible = "loongson,ls7a-uart";
 			reg = <0x0 0x1fe20000 0x0 0x10>;
 			clock-frequency = <125000000>;
 			interrupt-parent = <&liointc0>;
 			interrupts = <0x0 IRQ_TYPE_LEVEL_HIGH>;
 			no-loopback-test;
+			rts-invert;
+			dtr-invert;
+			cts-invert;
+			dsr-invert;
 			status = "disabled";
 		};
 
diff --git a/arch/loongarch/boot/dts/loongson-2k2000.dtsi b/arch/loongarch/boot/dts/loongson-2k2000.dtsi
index 0953c5707825..394494aaa242 100644
--- a/arch/loongarch/boot/dts/loongson-2k2000.dtsi
+++ b/arch/loongarch/boot/dts/loongson-2k2000.dtsi
@@ -174,12 +174,15 @@ rtc0: rtc@100d0100 {
 		};
 
 		uart0: serial@1fe001e0 {
-			compatible = "ns16550a";
+			compatible = "loongson,ls7a-uart";
 			reg = <0x0 0x1fe001e0 0x0 0x10>;
 			clock-frequency = <100000000>;
 			interrupt-parent = <&liointc>;
 			interrupts = <10 IRQ_TYPE_LEVEL_HIGH>;
 			no-loopback-test;
+			fractional-division;
+			rts-invert;
+			dtr-invert;
 			status = "disabled";
 		};
 
-- 
2.43.0

Re: [PATCH v2 3/3] LoongArch: dts: Update UART driver to Loongson-2K0500, Loongson-2K1000 and Loongson-2K2000.

[Krzysztof Kozlowski]

On 04/08/2024 08:38, zhenghaowei@loongson.cn wrote:
> From: Haowei Zheng <zhenghaowei@loongson.cn>
> 
> Change to use the Loongson UART driver by default.
> 
> Signed-off-by: Haowei Zheng <zhenghaowei@loongson.cn>

No changelog? Nothing improved?

Best regards,
Krzysztof

Re: [PATCH v2 3/3] LoongArch: dts: Update UART driver to Loongson-2K0500, Loongson-2K1000 and Loongson-2K2000.

[郑豪威]

在 2024/8/4 16:40, Krzysztof Kozlowski 写道:
> On 04/08/2024 08:38,zhenghaowei@loongson.cn wrote:
>> From: Haowei Zheng<zhenghaowei@loongson.cn>
>>
>> Change to use the Loongson UART driver by default.
>>
>> Signed-off-by: Haowei Zheng<zhenghaowei@loongson.cn>
> No changelog? Nothing improved?
>
> Best regards,
> Krzysztof

Sorry , I will include the update from V1 to V2 in the next patch update.

Here is the content of the update from V1 to V2:

Changes in V2:

- The compatible property for the UART is changed from "ns16650,loongson"

    to "loongson,ls7a-uart".


Best regards,

Haowei Zheng

Re: [PATCH v2 1/3] dt-bindings: serial: Add Loongson UART controller

[Rob Herring (Arm)]

On Sun, 04 Aug 2024 14:38:32 +0800, zhenghaowei@loongson.cn wrote:
> From: Haowei Zheng <zhenghaowei@loongson.cn>
> 
> Add Loongson UART controller binding with DT schema format using
> json-schema.
> 
> Signed-off-by: Haowei Zheng <zhenghaowei@loongson.cn>
> ---
>  .../bindings/serial/loongson,ls7a-uart.yaml   | 74 +++++++++++++++++++
>  MAINTAINERS                                   |  7 ++
>  2 files changed, 81 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/serial/loongson,ls7a-uart.yaml
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/serial/loongson,ls7a-uart.yaml: fractional-division: missing type definition
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/serial/loongson,ls7a-uart.yaml: rts-invert: missing type definition
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/serial/loongson,ls7a-uart.yaml: dtr-invert: missing type definition
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/serial/loongson,ls7a-uart.yaml: cts-invert: missing type definition
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/serial/loongson,ls7a-uart.yaml: dsr-invert: missing type definition
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/serial/loongson,ls7a-uart.example.dtb: serial@1fe001e0: reg: [[0, 534774240], [0, 16]] is too long
	from schema $id: http://devicetree.org/schemas/loongson,ls7a-uart.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/serial/loongson,ls7a-uart.example.dtb: serial@1fe001e0: 'clocks' is a required property
	from schema $id: http://devicetree.org/schemas/loongson,ls7a-uart.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/serial/loongson,ls7a-uart.example.dtb: serial@1fe001e0: Unevaluated properties are not allowed ('clock-frequency', 'reg' were unexpected)
	from schema $id: http://devicetree.org/schemas/loongson,ls7a-uart.yaml#

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240804063834.70022-1-zhenghaowei@loongson.cn

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH v2 1/3] dt-bindings: serial: Add Loongson UART controller

[Krzysztof Kozlowski]

On 04/08/2024 08:38, zhenghaowei@loongson.cn wrote:
> From: Haowei Zheng <zhenghaowei@loongson.cn>
> 
> Add Loongson UART controller binding with DT schema format using
> json-schema.
> 

Where is the changelog? Are you sending the same patch again?

Best regards,
Krzysztof

Re: [PATCH v2 1/3] dt-bindings: serial: Add Loongson UART controller

[郑豪威]

在 2024/8/4 16:41, Krzysztof Kozlowski 写道:
> On 04/08/2024 08:38,zhenghaowei@loongson.cn wrote:
>> From: Haowei Zheng<zhenghaowei@loongson.cn>
>>
>> Add Loongson UART controller binding with DT schema format using
>> json-schema.
>>
> Where is the changelog? Are you sending the same patch again?
>
> Best regards,
> Krzysztof

Sorry, here are the change log from V1 to V2, and I will include the update

in the next patch update.

Changes in V2:

- Correct the schema formatting errors.

- file name changed from 'loongson-uart.yaml' to 'loongson,ls7a-uart.yaml'

- Replace 'loongson,loongson-uart' with 'loongson,ls7a-uart'.


Best regards,

Haowei Zheng

Re: [PATCH v2 1/3] dt-bindings: serial: Add Loongson UART controller

[Krzysztof Kozlowski]

On 04/08/2024 08:38, zhenghaowei@loongson.cn wrote:

Due to lack of changelog, I assume you send the same patch, so:

<form letter>
This is a friendly reminder during the review process.

It seems my or other reviewer's previous comments were not fully
addressed. Maybe the feedback got lost between the quotes, maybe you
just forgot to apply it. Please go back to the previous discussion and
either implement all requested changes or keep discussing them.

Thank you.
</form letter>

Also:

> +
> +  clocks:
> +    maxItems: 1
> +
> +  fractional-division:

Where are this and following defined? In which schema?

> +    description: Enables fractional-N division. Currently,
> +      only LS2K1500 and LS2K2000 support this feature.
> +
> +  rts-invert:
> +    description: Inverts the RTS value in the MCR register.
> +      This should be used on Loongson-3 series CPUs, Loongson-2K
> +      series CPUs, and Loongson LS7A bridge chips.
> +
> +  dtr-invert:
> +    description: Inverts the DTR value in the MCR register.
> +      This should be used on Loongson-3 series CPUs, Loongson-2K
> +      series CPUs, and Loongson LS7A bridge chips.
> +
> +  cts-invert:
> +    description: Inverts the CTS value in the MSR register.
> +      This should be used on Loongson-2K0500, Loongson-2K1000,
> +      and Loongson LS7A bridge chips.
> +
> +  dsr-invert:
> +    description: Inverts the DSR value in the MSR register.
> +      This should be used on Loongson-2K0500, Loongson-2K1000,
> +      and Loongson LS7A bridge chips.
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - clocks
> +
> +allOf:
> +  - $ref: serial.yaml
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    #include <dt-bindings/clock/loongson,ls2k-clk.h>
> +
> +    serial@1fe001e0 {
> +        compatible = "loongson,ls7a-uart";
> +        reg = <0x0 0x1fe001e0 0x0 0x10>;
> +        clock-frequency = <100000000>;
> +        interrupt-parent = <&liointc>;
> +        interrupts = <10 IRQ_TYPE_LEVEL_HIGH>;
> +        fractional-division;
> +        rts-invert;
> +        dtr-invert;
> +    };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 8766f3e5e87e..a6306327dba5 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -13189,6 +13189,13 @@ S:	Maintained
>  F:	Documentation/devicetree/bindings/i2c/loongson,ls2x-i2c.yaml
>  F:	drivers/i2c/busses/i2c-ls2x.c
>  
> +LOONGSON UART DRIVER
> +M:	Haowei Zheng <zhenghaowei@loongson.cn>
> +L:	linux-serial@vger.kernel.org
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/serial/loongson,ls7a-uart.yaml
> +F:	drivers/tty/serial/8250/8250_loongson.c

There is no such file.

Best regards,
Krzysztof

Re: [PATCH v2 1/3] dt-bindings: serial: Add Loongson UART controller

[郑豪威]

在 2024/8/4 16:43, Krzysztof Kozlowski 写道:
> On 04/08/2024 08:38,zhenghaowei@loongson.cn wrote:
>
> Due to lack of changelog, I assume you send the same patch, so:
>
> <form letter>
> This is a friendly reminder during the review process.
>
> It seems my or other reviewer's previous comments were not fully
> addressed. Maybe the feedback got lost between the quotes, maybe you
> just forgot to apply it. Please go back to the previous discussion and
> either implement all requested changes or keep discussing them.
>
> Thank you.
> </form letter>
>
> Also:
>
>> +
>> +  clocks:
>> +    maxItems: 1
>> +
>> +  fractional-division:
> Where are this and following defined? In which schema?
>
These and the ones below are new definitions, can I use them like this?

+  fractional-division:
+    description: Enables fractional-N division. Currently,
+      only LS2K1500 and LS2K2000 support this feature.
+    type: boolean

>> +    description: Enables fractional-N division. Currently,
>> +      only LS2K1500 and LS2K2000 support this feature.
>> +
>> +  rts-invert:
>> +    description: Inverts the RTS value in the MCR register.
>> +      This should be used on Loongson-3 series CPUs, Loongson-2K
>> +      series CPUs, and Loongson LS7A bridge chips.
>> +
>> +  dtr-invert:
>> +    description: Inverts the DTR value in the MCR register.
>> +      This should be used on Loongson-3 series CPUs, Loongson-2K
>> +      series CPUs, and Loongson LS7A bridge chips.
>> +
>> +  cts-invert:
>> +    description: Inverts the CTS value in the MSR register.
>> +      This should be used on Loongson-2K0500, Loongson-2K1000,
>> +      and Loongson LS7A bridge chips.
>> +
>> +  dsr-invert:
>> +    description: Inverts the DSR value in the MSR register.
>> +      This should be used on Loongson-2K0500, Loongson-2K1000,
>> +      and Loongson LS7A bridge chips.
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - interrupts
>> +  - clocks
>> +
>> +allOf:
>> +  - $ref: serial.yaml
>> +
>> +unevaluatedProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/interrupt-controller/irq.h>
>> +    #include <dt-bindings/clock/loongson,ls2k-clk.h>
>> +
>> +    serial@1fe001e0 {
>> +        compatible = "loongson,ls7a-uart";
>> +        reg = <0x0 0x1fe001e0 0x0 0x10>;
>> +        clock-frequency = <100000000>;
>> +        interrupt-parent = <&liointc>;
>> +        interrupts = <10 IRQ_TYPE_LEVEL_HIGH>;
>> +        fractional-division;
>> +        rts-invert;
>> +        dtr-invert;
>> +    };
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 8766f3e5e87e..a6306327dba5 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -13189,6 +13189,13 @@ S:	Maintained
>>   F:	Documentation/devicetree/bindings/i2c/loongson,ls2x-i2c.yaml
>>   F:	drivers/i2c/busses/i2c-ls2x.c
>>   
>> +LOONGSON UART DRIVER
>> +M:	Haowei Zheng<zhenghaowei@loongson.cn>
>> +L:	linux-serial@vger.kernel.org
>> +S:	Maintained
>> +F:	Documentation/devicetree/bindings/serial/loongson,ls7a-uart.yaml
>> +F:	drivers/tty/serial/8250/8250_loongson.c
> There is no such file.
>
> Best regards,
> Krzysztof

The file "drivers/tty/serial/8250/8250_loongson.c" will be created in 
the patch

"tty: serial: 8250: Add loongson uart driver support". Is it 
inappropriate to reference it here?


Best regards,

Haowei Zheng

Re: [PATCH v2 1/3] dt-bindings: serial: Add Loongson UART controller

[Xi Ruoyao]

On Wed, 2024-08-07 at 16:23 +0800, 郑豪威 wrote:
> The file "drivers/tty/serial/8250/8250_loongson.c" will be created in 
> the patch
> 
> "tty: serial: 8250: Add loongson uart driver support". Is it 
> inappropriate to reference it here?

You should add this line in the second patch then.  Separating a large
change into multiple patches in a series is not for formalities' sake. 
Each patch should be logically intact on its own.

-- 
Xi Ruoyao <xry111@xry111.site>
School of Aerospace Science and Technology, Xidian University

Re: [PATCH v2 1/3] dt-bindings: serial: Add Loongson UART controller

[郑豪威]

在 2024/8/7 16:39, Xi Ruoyao 写道:
> On Wed, 2024-08-07 at 16:23 +0800, 郑豪威 wrote:
>> The file "drivers/tty/serial/8250/8250_loongson.c" will be created in
>> the patch
>>
>> "tty: serial: 8250: Add loongson uart driver support". Is it
>> inappropriate to reference it here?
> You should add this line in the second patch then.  Separating a large
> change into multiple patches in a series is not for formalities' sake.
> Each patch should be logically intact on its own.
>
Thank you, I got it.


Best regards,

Haowei Zheng

Re: [PATCH v2 1/3] dt-bindings: serial: Add Loongson UART controller

[Krzysztof Kozlowski]

On 07/08/2024 10:23, 郑豪威 wrote:
> 
> 在 2024/8/4 16:43, Krzysztof Kozlowski 写道:
>> On 04/08/2024 08:38,zhenghaowei@loongson.cn wrote:
>>
>> Due to lack of changelog, I assume you send the same patch, so:
>>
>> <form letter>
>> This is a friendly reminder during the review process.
>>
>> It seems my or other reviewer's previous comments were not fully
>> addressed. Maybe the feedback got lost between the quotes, maybe you
>> just forgot to apply it. Please go back to the previous discussion and
>> either implement all requested changes or keep discussing them.
>>
>> Thank you.
>> </form letter>
>>
>> Also:
>>
>>> +
>>> +  clocks:
>>> +    maxItems: 1
>>> +
>>> +  fractional-division:
>> Where are this and following defined? In which schema?
>>
> These and the ones below are new definitions, can I use them like this?
> 
> +  fractional-division:
> +    description: Enables fractional-N division. Currently,
> +      only LS2K1500 and LS2K2000 support this feature.
> +    type: boolean
> 

Missing vendor prefix, but what's more important, why would this be
property of DT? Just enable it always...

>>> +    description: Enables fractional-N division. Currently,
>>> +      only LS2K1500 and LS2K2000 support this feature.
>>> +
>>> +  rts-invert:
>>> +    description: Inverts the RTS value in the MCR register.
>>> +      This should be used on Loongson-3 series CPUs, Loongson-2K
>>> +      series CPUs, and Loongson LS7A bridge chips.
>>> +
>>> +  dtr-invert:
>>> +    description: Inverts the DTR value in the MCR register.
>>> +      This should be used on Loongson-3 series CPUs, Loongson-2K
>>> +      series CPUs, and Loongson LS7A bridge chips.
>>> +
>>> +  cts-invert:
>>> +    description: Inverts the CTS value in the MSR register.
>>> +      This should be used on Loongson-2K0500, Loongson-2K1000,
>>> +      and Loongson LS7A bridge chips.
>>> +
>>> +  dsr-invert:
>>> +    description: Inverts the DSR value in the MSR register.
>>> +      This should be used on Loongson-2K0500, Loongson-2K1000,
>>> +      and Loongson LS7A bridge chips.

Same questions for all these. Why choosing invert is a board level
decision? If it "should be used" then why it is not used always?

>>> +
>>> +required:
>>> +  - compatible
>>> +  - reg
>>> +  - interrupts
>>> +  - clocks
>>> +
>>> +allOf:
>>> +  - $ref: serial.yaml
>>> +
>>> +unevaluatedProperties: false
>>> +
>>> +examples:
>>> +  - |
>>> +    #include <dt-bindings/interrupt-controller/irq.h>
>>> +    #include <dt-bindings/clock/loongson,ls2k-clk.h>
>>> +
>>> +    serial@1fe001e0 {
>>> +        compatible = "loongson,ls7a-uart";
>>> +        reg = <0x0 0x1fe001e0 0x0 0x10>;
>>> +        clock-frequency = <100000000>;
>>> +        interrupt-parent = <&liointc>;
>>> +        interrupts = <10 IRQ_TYPE_LEVEL_HIGH>;
>>> +        fractional-division;
>>> +        rts-invert;
>>> +        dtr-invert;
>>> +    };
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index 8766f3e5e87e..a6306327dba5 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -13189,6 +13189,13 @@ S:	Maintained
>>>   F:	Documentation/devicetree/bindings/i2c/loongson,ls2x-i2c.yaml
>>>   F:	drivers/i2c/busses/i2c-ls2x.c
>>>   
>>> +LOONGSON UART DRIVER
>>> +M:	Haowei Zheng<zhenghaowei@loongson.cn>
>>> +L:	linux-serial@vger.kernel.org
>>> +S:	Maintained
>>> +F:	Documentation/devicetree/bindings/serial/loongson,ls7a-uart.yaml
>>> +F:	drivers/tty/serial/8250/8250_loongson.c
>> There is no such file.
>>
>> Best regards,
>> Krzysztof
> 
> The file "drivers/tty/serial/8250/8250_loongson.c" will be created in 
> the patch
> 
> "tty: serial: 8250: Add loongson uart driver support". Is it 
> inappropriate to reference it here?

Apply this patch and run get_maintainers self tests. What do you see?

Of course it is not appropriate here. The file does not exist.

Best regards,
Krzysztof

Re: [PATCH v2 1/3] dt-bindings: serial: Add Loongson UART controller

[郑豪威]

在 2024/8/9 13:53, Krzysztof Kozlowski 写道:
> On 07/08/2024 10:23, 郑豪威 wrote:
>> 在 2024/8/4 16:43, Krzysztof Kozlowski 写道:
>>> On 04/08/2024 08:38,zhenghaowei@loongson.cn  wrote:
>>>
>>> Due to lack of changelog, I assume you send the same patch, so:
>>>
>>> <form letter>
>>> This is a friendly reminder during the review process.
>>>
>>> It seems my or other reviewer's previous comments were not fully
>>> addressed. Maybe the feedback got lost between the quotes, maybe you
>>> just forgot to apply it. Please go back to the previous discussion and
>>> either implement all requested changes or keep discussing them.
>>>
>>> Thank you.
>>> </form letter>
>>>
>>> Also:
>>>
>>>> +
>>>> +  clocks:
>>>> +    maxItems: 1
>>>> +
>>>> +  fractional-division:
>>> Where are this and following defined? In which schema?
>>>
>> These and the ones below are new definitions, can I use them like this?
>>
>> +  fractional-division:
>> +    description: Enables fractional-N division. Currently,
>> +      only LS2K1500 and LS2K2000 support this feature.
>> +    type: boolean
>>
> Missing vendor prefix, but what's more important, why would this be
> property of DT? Just enable it always...
>
>>>> +    description: Enables fractional-N division. Currently,
>>>> +      only LS2K1500 and LS2K2000 support this feature.
>>>> +
>>>> +  rts-invert:
>>>> +    description: Inverts the RTS value in the MCR register.
>>>> +      This should be used on Loongson-3 series CPUs, Loongson-2K
>>>> +      series CPUs, and Loongson LS7A bridge chips.
>>>> +
>>>> +  dtr-invert:
>>>> +    description: Inverts the DTR value in the MCR register.
>>>> +      This should be used on Loongson-3 series CPUs, Loongson-2K
>>>> +      series CPUs, and Loongson LS7A bridge chips.
>>>> +
>>>> +  cts-invert:
>>>> +    description: Inverts the CTS value in the MSR register.
>>>> +      This should be used on Loongson-2K0500, Loongson-2K1000,
>>>> +      and Loongson LS7A bridge chips.
>>>> +
>>>> +  dsr-invert:
>>>> +    description: Inverts the DSR value in the MSR register.
>>>> +      This should be used on Loongson-2K0500, Loongson-2K1000,
>>>> +      and Loongson LS7A bridge chips.
> Same questions for all these. Why choosing invert is a board level
> decision? If it "should be used" then why it is not used always?
>
Because these features are not applicable to all chips, such as 
'fractional-division',

which is currently supported only by 2K1500 and 2K2000, and for 
Loongson-3 series

CPUs,  'cts-invert' and 'dtr-invert' are not needed. More importantly, 
for future chip

designs, these issues may no longer exist.

>>>> +
>>>> +required:
>>>> +  - compatible
>>>> +  - reg
>>>> +  - interrupts
>>>> +  - clocks
>>>> +
>>>> +allOf:
>>>> +  - $ref: serial.yaml
>>>> +
>>>> +unevaluatedProperties: false
>>>> +
>>>> +examples:
>>>> +  - |
>>>> +    #include <dt-bindings/interrupt-controller/irq.h>
>>>> +    #include <dt-bindings/clock/loongson,ls2k-clk.h>
>>>> +
>>>> +    serial@1fe001e0 {
>>>> +        compatible = "loongson,ls7a-uart";
>>>> +        reg = <0x0 0x1fe001e0 0x0 0x10>;
>>>> +        clock-frequency = <100000000>;
>>>> +        interrupt-parent = <&liointc>;
>>>> +        interrupts = <10 IRQ_TYPE_LEVEL_HIGH>;
>>>> +        fractional-division;
>>>> +        rts-invert;
>>>> +        dtr-invert;
>>>> +    };
>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>> index 8766f3e5e87e..a6306327dba5 100644
>>>> --- a/MAINTAINERS
>>>> +++ b/MAINTAINERS
>>>> @@ -13189,6 +13189,13 @@ S:	Maintained
>>>>    F:	Documentation/devicetree/bindings/i2c/loongson,ls2x-i2c.yaml
>>>>    F:	drivers/i2c/busses/i2c-ls2x.c
>>>>    
>>>> +LOONGSON UART DRIVER
>>>> +M:	Haowei Zheng<zhenghaowei@loongson.cn>
>>>> +L:	linux-serial@vger.kernel.org
>>>> +S:	Maintained
>>>> +F:	Documentation/devicetree/bindings/serial/loongson,ls7a-uart.yaml
>>>> +F:	drivers/tty/serial/8250/8250_loongson.c
>>> There is no such file.
>>>
>>> Best regards,
>>> Krzysztof
>> The file "drivers/tty/serial/8250/8250_loongson.c" will be created in
>> the patch
>>
>> "tty: serial: 8250: Add loongson uart driver support". Is it
>> inappropriate to reference it here?
> Apply this patch and run get_maintainers self tests. What do you see?
>
> Of course it is not appropriate here. The file does not exist.
>
> Best regards,
> Krzysztof

I got it, I  will include it in the next patch.


Best regards,

Haowei Zheng

Re: [PATCH v2 1/3] dt-bindings: serial: Add Loongson UART controller

[Krzysztof Kozlowski]

On 09/08/2024 11:55, 郑豪威 wrote:
>>
>>>>> +    description: Enables fractional-N division. Currently,
>>>>> +      only LS2K1500 and LS2K2000 support this feature.
>>>>> +
>>>>> +  rts-invert:
>>>>> +    description: Inverts the RTS value in the MCR register.
>>>>> +      This should be used on Loongson-3 series CPUs, Loongson-2K
>>>>> +      series CPUs, and Loongson LS7A bridge chips.
>>>>> +
>>>>> +  dtr-invert:
>>>>> +    description: Inverts the DTR value in the MCR register.
>>>>> +      This should be used on Loongson-3 series CPUs, Loongson-2K
>>>>> +      series CPUs, and Loongson LS7A bridge chips.
>>>>> +
>>>>> +  cts-invert:
>>>>> +    description: Inverts the CTS value in the MSR register.
>>>>> +      This should be used on Loongson-2K0500, Loongson-2K1000,
>>>>> +      and Loongson LS7A bridge chips.
>>>>> +
>>>>> +  dsr-invert:
>>>>> +    description: Inverts the DSR value in the MSR register.
>>>>> +      This should be used on Loongson-2K0500, Loongson-2K1000,
>>>>> +      and Loongson LS7A bridge chips.
>> Same questions for all these. Why choosing invert is a board level
>> decision? If it "should be used" then why it is not used always?
>>
> Because these features are not applicable to all chips, such as 
> 'fractional-division',

Hm?

> 
> which is currently supported only by 2K1500 and 2K2000, and for 
> Loongson-3 series

These are SoCs. Compatible defines that. Please align with your
colleagues, because *we talked about this* already.

Best regards,
Krzysztof

Re: [PATCH v2 1/3] dt-bindings: serial: Add Loongson UART controller

[郑豪威]

在 2024/8/9 18:05, Krzysztof Kozlowski 写道:
> On 09/08/2024 11:55, 郑豪威 wrote:
>>>>>> +    description: Enables fractional-N division. Currently,
>>>>>> +      only LS2K1500 and LS2K2000 support this feature.
>>>>>> +
>>>>>> +  rts-invert:
>>>>>> +    description: Inverts the RTS value in the MCR register.
>>>>>> +      This should be used on Loongson-3 series CPUs, Loongson-2K
>>>>>> +      series CPUs, and Loongson LS7A bridge chips.
>>>>>> +
>>>>>> +  dtr-invert:
>>>>>> +    description: Inverts the DTR value in the MCR register.
>>>>>> +      This should be used on Loongson-3 series CPUs, Loongson-2K
>>>>>> +      series CPUs, and Loongson LS7A bridge chips.
>>>>>> +
>>>>>> +  cts-invert:
>>>>>> +    description: Inverts the CTS value in the MSR register.
>>>>>> +      This should be used on Loongson-2K0500, Loongson-2K1000,
>>>>>> +      and Loongson LS7A bridge chips.
>>>>>> +
>>>>>> +  dsr-invert:
>>>>>> +    description: Inverts the DSR value in the MSR register.
>>>>>> +      This should be used on Loongson-2K0500, Loongson-2K1000,
>>>>>> +      and Loongson LS7A bridge chips.
>>> Same questions for all these. Why choosing invert is a board level
>>> decision? If it "should be used" then why it is not used always?
>>>
>> Because these features are not applicable to all chips, such as
>> 'fractional-division',
> Hm?
>
>> which is currently supported only by 2K1500 and 2K2000, and for
>> Loongson-3 series
> These are SoCs. Compatible defines that. Please align with your
> colleagues, because *we talked about this* already.
>
> Best regards,
> Krzysztof

I consulted with my colleagues and would like to confirm with you. For 
the five

properties provided, fractional-division is offered as a new feature,  
supported by

2K1500 and 2K2000. As for the invert property, it is due to a bug in our 
controller,

and its usage may vary across different chips. Should we add different 
compatible

values to address these issues for different chips, whether they are new 
features or

controller bugs?


Best regards,

Haowei Zheng

Re: [PATCH v2 1/3] dt-bindings: serial: Add Loongson UART controller

[Krzysztof Kozlowski]

On 12/08/2024 10:09, 郑豪威 wrote:
> 
> 在 2024/8/9 18:05, Krzysztof Kozlowski 写道:
>> On 09/08/2024 11:55, 郑豪威 wrote:
>>>>>>> +    description: Enables fractional-N division. Currently,
>>>>>>> +      only LS2K1500 and LS2K2000 support this feature.
>>>>>>> +
>>>>>>> +  rts-invert:
>>>>>>> +    description: Inverts the RTS value in the MCR register.
>>>>>>> +      This should be used on Loongson-3 series CPUs, Loongson-2K
>>>>>>> +      series CPUs, and Loongson LS7A bridge chips.
>>>>>>> +
>>>>>>> +  dtr-invert:
>>>>>>> +    description: Inverts the DTR value in the MCR register.
>>>>>>> +      This should be used on Loongson-3 series CPUs, Loongson-2K
>>>>>>> +      series CPUs, and Loongson LS7A bridge chips.
>>>>>>> +
>>>>>>> +  cts-invert:
>>>>>>> +    description: Inverts the CTS value in the MSR register.
>>>>>>> +      This should be used on Loongson-2K0500, Loongson-2K1000,
>>>>>>> +      and Loongson LS7A bridge chips.
>>>>>>> +
>>>>>>> +  dsr-invert:
>>>>>>> +    description: Inverts the DSR value in the MSR register.
>>>>>>> +      This should be used on Loongson-2K0500, Loongson-2K1000,
>>>>>>> +      and Loongson LS7A bridge chips.
>>>> Same questions for all these. Why choosing invert is a board level
>>>> decision? If it "should be used" then why it is not used always?
>>>>
>>> Because these features are not applicable to all chips, such as
>>> 'fractional-division',
>> Hm?
>>
>>> which is currently supported only by 2K1500 and 2K2000, and for
>>> Loongson-3 series
>> These are SoCs. Compatible defines that. Please align with your
>> colleagues, because *we talked about this* already.
>>
>> Best regards,
>> Krzysztof
> 
> I consulted with my colleagues and would like to confirm with you. For 
> the five
> 
> properties provided, fractional-division is offered as a new feature,  
> supported by
> 
> 2K1500 and 2K2000. As for the invert property, it is due to a bug in our 
> controller,
> 
> and its usage may vary across different chips. Should we add different 
> compatible
> 
> values to address these issues for different chips, whether they are new 
> features or
> 
> controller bugs?

How did you align? We had already talks with you about this problem -
you need specific compatibles. How you explain above properties, all of
them are deducible from the compatible, so drop them.

Your entire argument above does not address at all my concerns, so
before you respond repeating the same, really talk with your colleagues.

One of many previous discussions:
https://lore.kernel.org/linux-devicetree/25c30964-6bd3-c7eb-640a-ba1f513b7675@linaro.org/

https://lore.kernel.org/linux-devicetree/20230526-dolly-reheat-06c4d5658415@wendy/

I wish we do not have to keep repeating the same to Loongson. Please
STORE the feedback for any future submissions, so you will not repeat
the same mistakes over and over.

Best regards,
Krzysztof

Re: [PATCH v2 1/3] dt-bindings: serial: Add Loongson UART controller

[郑豪威]

在 2024/8/12 16:25, Krzysztof Kozlowski 写道:
> On 12/08/2024 10:09, 郑豪威 wrote:
>> 在 2024/8/9 18:05, Krzysztof Kozlowski 写道:
>>> On 09/08/2024 11:55, 郑豪威 wrote:
>>>>>>>> +    description: Enables fractional-N division. Currently,
>>>>>>>> +      only LS2K1500 and LS2K2000 support this feature.
>>>>>>>> +
>>>>>>>> +  rts-invert:
>>>>>>>> +    description: Inverts the RTS value in the MCR register.
>>>>>>>> +      This should be used on Loongson-3 series CPUs, Loongson-2K
>>>>>>>> +      series CPUs, and Loongson LS7A bridge chips.
>>>>>>>> +
>>>>>>>> +  dtr-invert:
>>>>>>>> +    description: Inverts the DTR value in the MCR register.
>>>>>>>> +      This should be used on Loongson-3 series CPUs, Loongson-2K
>>>>>>>> +      series CPUs, and Loongson LS7A bridge chips.
>>>>>>>> +
>>>>>>>> +  cts-invert:
>>>>>>>> +    description: Inverts the CTS value in the MSR register.
>>>>>>>> +      This should be used on Loongson-2K0500, Loongson-2K1000,
>>>>>>>> +      and Loongson LS7A bridge chips.
>>>>>>>> +
>>>>>>>> +  dsr-invert:
>>>>>>>> +    description: Inverts the DSR value in the MSR register.
>>>>>>>> +      This should be used on Loongson-2K0500, Loongson-2K1000,
>>>>>>>> +      and Loongson LS7A bridge chips.
>>>>> Same questions for all these. Why choosing invert is a board level
>>>>> decision? If it "should be used" then why it is not used always?
>>>>>
>>>> Because these features are not applicable to all chips, such as
>>>> 'fractional-division',
>>> Hm?
>>>
>>>> which is currently supported only by 2K1500 and 2K2000, and for
>>>> Loongson-3 series
>>> These are SoCs. Compatible defines that. Please align with your
>>> colleagues, because *we talked about this* already.
>>>
>>> Best regards,
>>> Krzysztof
>> I consulted with my colleagues and would like to confirm with you. For
>> the five
>>
>> properties provided, fractional-division is offered as a new feature,
>> supported by
>>
>> 2K1500 and 2K2000. As for the invert property, it is due to a bug in our
>> controller,
>>
>> and its usage may vary across different chips. Should we add different
>> compatible
>>
>> values to address these issues for different chips, whether they are new
>> features or
>>
>> controller bugs?
> How did you align? We had already talks with you about this problem -
> you need specific compatibles. How you explain above properties, all of
> them are deducible from the compatible, so drop them.
>
> Your entire argument above does not address at all my concerns, so
> before you respond repeating the same, really talk with your colleagues.
>
> One of many previous discussions:
> https://lore.kernel.org/linux-devicetree/25c30964-6bd3-c7eb-640a-ba1f513b7675@linaro.org/
>
> https://lore.kernel.org/linux-devicetree/20230526-dolly-reheat-06c4d5658415@wendy/
>
> I wish we do not have to keep repeating the same to Loongson. Please
> STORE the feedback for any future submissions, so you will not repeat
> the same mistakes over and over.
>
> Best regards,
> Krzysztof

Hi:

I have been aligning with my colleagues over the past few days and

reviewing previous discussions. Based on these, I have made the

following modifications according to the differences in the controller:

+properties:
+  compatible:
+    oneOf:
+      - enum:
+          - loongson,ls7a-uart
+          - loongson,ls3a5000-uart
+          - loongson,ls2k2000-uart
+      - items:
+          - enum:
+              - loongson,ls2k1000-uart
+              - loongson,ls2k0500-uart
+          - const: loongson,ls7a-uart
+      - items:
+          - enum:
+              - loongson,ls2k1500-uart
+          - const: loongson,ls2k2000-uart
+      - items:
+          - enum:
+              - loongson,ls3a6000-uart
+          - const: loongson,ls3a5000-uart
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  clock-frequency: true
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clock-frequency
+
+allOf:
+  - $ref: serial.yaml
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/clock/loongson,ls2k-clk.h>
+
+    serial@1fe20000 {
+        compatible = "loongson,ls2k1000-uart", "loongson,ls7a-uart";
+        reg = <0x1fe20000 0x10>;
+        clock-frequency = <125000000>;
+        interrupt-parent = <&liointc0>;
+        interrupts = <0x0 IRQ_TYPE_LEVEL_HIGH>;
+    };

Does this modification meet the expectation?

Best regards,

Haowei Zheng

Re: [PATCH v2 1/3] dt-bindings: serial: Add Loongson UART controller

[Krzysztof Kozlowski]

On 25/08/2024 05:34, 郑豪威 wrote:
> 
> 在 2024/8/12 16:25, Krzysztof Kozlowski 写道:
>> On 12/08/2024 10:09, 郑豪威 wrote:
>>> 在 2024/8/9 18:05, Krzysztof Kozlowski 写道:
>>>> On 09/08/2024 11:55, 郑豪威 wrote:
>>>>>>>>> +    description: Enables fractional-N division. Currently,
>>>>>>>>> +      only LS2K1500 and LS2K2000 support this feature.
>>>>>>>>> +
>>>>>>>>> +  rts-invert:
>>>>>>>>> +    description: Inverts the RTS value in the MCR register.
>>>>>>>>> +      This should be used on Loongson-3 series CPUs, Loongson-2K
>>>>>>>>> +      series CPUs, and Loongson LS7A bridge chips.
>>>>>>>>> +
>>>>>>>>> +  dtr-invert:
>>>>>>>>> +    description: Inverts the DTR value in the MCR register.
>>>>>>>>> +      This should be used on Loongson-3 series CPUs, Loongson-2K
>>>>>>>>> +      series CPUs, and Loongson LS7A bridge chips.
>>>>>>>>> +
>>>>>>>>> +  cts-invert:
>>>>>>>>> +    description: Inverts the CTS value in the MSR register.
>>>>>>>>> +      This should be used on Loongson-2K0500, Loongson-2K1000,
>>>>>>>>> +      and Loongson LS7A bridge chips.
>>>>>>>>> +
>>>>>>>>> +  dsr-invert:
>>>>>>>>> +    description: Inverts the DSR value in the MSR register.
>>>>>>>>> +      This should be used on Loongson-2K0500, Loongson-2K1000,
>>>>>>>>> +      and Loongson LS7A bridge chips.
>>>>>> Same questions for all these. Why choosing invert is a board level
>>>>>> decision? If it "should be used" then why it is not used always?
>>>>>>
>>>>> Because these features are not applicable to all chips, such as
>>>>> 'fractional-division',
>>>> Hm?
>>>>
>>>>> which is currently supported only by 2K1500 and 2K2000, and for
>>>>> Loongson-3 series
>>>> These are SoCs. Compatible defines that. Please align with your
>>>> colleagues, because *we talked about this* already.
>>>>
>>>> Best regards,
>>>> Krzysztof
>>> I consulted with my colleagues and would like to confirm with you. For
>>> the five
>>>
>>> properties provided, fractional-division is offered as a new feature,
>>> supported by
>>>
>>> 2K1500 and 2K2000. As for the invert property, it is due to a bug in our
>>> controller,
>>>
>>> and its usage may vary across different chips. Should we add different
>>> compatible
>>>
>>> values to address these issues for different chips, whether they are new
>>> features or
>>>
>>> controller bugs?
>> How did you align? We had already talks with you about this problem -
>> you need specific compatibles. How you explain above properties, all of
>> them are deducible from the compatible, so drop them.
>>
>> Your entire argument above does not address at all my concerns, so
>> before you respond repeating the same, really talk with your colleagues.
>>
>> One of many previous discussions:
>> https://lore.kernel.org/linux-devicetree/25c30964-6bd3-c7eb-640a-ba1f513b7675@linaro.org/
>>
>> https://lore.kernel.org/linux-devicetree/20230526-dolly-reheat-06c4d5658415@wendy/
>>
>> I wish we do not have to keep repeating the same to Loongson. Please
>> STORE the feedback for any future submissions, so you will not repeat
>> the same mistakes over and over.
>>
>> Best regards,
>> Krzysztof
> 
> Hi:
> 
> I have been aligning with my colleagues over the past few days and
> 
> reviewing previous discussions. Based on these, I have made the
> 
> following modifications according to the differences in the controller:
> 
> +properties:
> +  compatible:
> +    oneOf:
> +      - enum:
> +          - loongson,ls7a-uart
> +          - loongson,ls3a5000-uart
> +          - loongson,ls2k2000-uart
> +      - items:
> +          - enum:
> +              - loongson,ls2k1000-uart
> +              - loongson,ls2k0500-uart
> +          - const: loongson,ls7a-uart
> +      - items:
> +          - enum:
> +              - loongson,ls2k1500-uart
> +          - const: loongson,ls2k2000-uart
> +      - items:
> +          - enum:
> +              - loongson,ls3a6000-uart
> +          - const: loongson,ls3a5000-uart
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clock-frequency: true
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - clock-frequency
> +
> +allOf:
> +  - $ref: serial.yaml
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    #include <dt-bindings/clock/loongson,ls2k-clk.h>
> +
> +    serial@1fe20000 {
> +        compatible = "loongson,ls2k1000-uart", "loongson,ls7a-uart";
> +        reg = <0x1fe20000 0x10>;
> +        clock-frequency = <125000000>;
> +        interrupt-parent = <&liointc0>;
> +        interrupts = <0x0 IRQ_TYPE_LEVEL_HIGH>;
> +    };
> 
> Does this modification meet the expectation?

Yes, assuming ls7a is a specific SoC, not a family of SoC.

Best regards,
Krzysztof

Re: [PATCH v2 1/3] dt-bindings: serial: Add Loongson UART controller

[郑豪威]

在 2024/8/25 14:55, Krzysztof Kozlowski 写道:
> On 25/08/2024 05:34, 郑豪威 wrote:
>> 在 2024/8/12 16:25, Krzysztof Kozlowski 写道:
>>> On 12/08/2024 10:09, 郑豪威 wrote:
>>>> 在 2024/8/9 18:05, Krzysztof Kozlowski 写道:
>>>>> On 09/08/2024 11:55, 郑豪威 wrote:
>>>>>>>>>> +    description: Enables fractional-N division. Currently,
>>>>>>>>>> +      only LS2K1500 and LS2K2000 support this feature.
>>>>>>>>>> +
>>>>>>>>>> +  rts-invert:
>>>>>>>>>> +    description: Inverts the RTS value in the MCR register.
>>>>>>>>>> +      This should be used on Loongson-3 series CPUs, Loongson-2K
>>>>>>>>>> +      series CPUs, and Loongson LS7A bridge chips.
>>>>>>>>>> +
>>>>>>>>>> +  dtr-invert:
>>>>>>>>>> +    description: Inverts the DTR value in the MCR register.
>>>>>>>>>> +      This should be used on Loongson-3 series CPUs, Loongson-2K
>>>>>>>>>> +      series CPUs, and Loongson LS7A bridge chips.
>>>>>>>>>> +
>>>>>>>>>> +  cts-invert:
>>>>>>>>>> +    description: Inverts the CTS value in the MSR register.
>>>>>>>>>> +      This should be used on Loongson-2K0500, Loongson-2K1000,
>>>>>>>>>> +      and Loongson LS7A bridge chips.
>>>>>>>>>> +
>>>>>>>>>> +  dsr-invert:
>>>>>>>>>> +    description: Inverts the DSR value in the MSR register.
>>>>>>>>>> +      This should be used on Loongson-2K0500, Loongson-2K1000,
>>>>>>>>>> +      and Loongson LS7A bridge chips.
>>>>>>> Same questions for all these. Why choosing invert is a board level
>>>>>>> decision? If it "should be used" then why it is not used always?
>>>>>>>
>>>>>> Because these features are not applicable to all chips, such as
>>>>>> 'fractional-division',
>>>>> Hm?
>>>>>
>>>>>> which is currently supported only by 2K1500 and 2K2000, and for
>>>>>> Loongson-3 series
>>>>> These are SoCs. Compatible defines that. Please align with your
>>>>> colleagues, because *we talked about this* already.
>>>>>
>>>>> Best regards,
>>>>> Krzysztof
>>>> I consulted with my colleagues and would like to confirm with you. For
>>>> the five
>>>>
>>>> properties provided, fractional-division is offered as a new feature,
>>>> supported by
>>>>
>>>> 2K1500 and 2K2000. As for the invert property, it is due to a bug in our
>>>> controller,
>>>>
>>>> and its usage may vary across different chips. Should we add different
>>>> compatible
>>>>
>>>> values to address these issues for different chips, whether they are new
>>>> features or
>>>>
>>>> controller bugs?
>>> How did you align? We had already talks with you about this problem -
>>> you need specific compatibles. How you explain above properties, all of
>>> them are deducible from the compatible, so drop them.
>>>
>>> Your entire argument above does not address at all my concerns, so
>>> before you respond repeating the same, really talk with your colleagues.
>>>
>>> One of many previous discussions:
>>> https://lore.kernel.org/linux-devicetree/25c30964-6bd3-c7eb-640a-ba1f513b7675@linaro.org/
>>>
>>> https://lore.kernel.org/linux-devicetree/20230526-dolly-reheat-06c4d5658415@wendy/
>>>
>>> I wish we do not have to keep repeating the same to Loongson. Please
>>> STORE the feedback for any future submissions, so you will not repeat
>>> the same mistakes over and over.
>>>
>>> Best regards,
>>> Krzysztof
>> Hi:
>>
>> I have been aligning with my colleagues over the past few days and
>>
>> reviewing previous discussions. Based on these, I have made the
>>
>> following modifications according to the differences in the controller:
>>
>> +properties:
>> +  compatible:
>> +    oneOf:
>> +      - enum:
>> +          - loongson,ls7a-uart
>> +          - loongson,ls3a5000-uart
>> +          - loongson,ls2k2000-uart
>> +      - items:
>> +          - enum:
>> +              - loongson,ls2k1000-uart
>> +              - loongson,ls2k0500-uart
>> +          - const: loongson,ls7a-uart
>> +      - items:
>> +          - enum:
>> +              - loongson,ls2k1500-uart
>> +          - const: loongson,ls2k2000-uart
>> +      - items:
>> +          - enum:
>> +              - loongson,ls3a6000-uart
>> +          - const: loongson,ls3a5000-uart
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +
>> +  clock-frequency: true
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - interrupts
>> +  - clock-frequency
>> +
>> +allOf:
>> +  - $ref: serial.yaml
>> +
>> +unevaluatedProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/interrupt-controller/irq.h>
>> +    #include <dt-bindings/clock/loongson,ls2k-clk.h>
>> +
>> +    serial@1fe20000 {
>> +        compatible = "loongson,ls2k1000-uart", "loongson,ls7a-uart";
>> +        reg = <0x1fe20000 0x10>;
>> +        clock-frequency = <125000000>;
>> +        interrupt-parent = <&liointc0>;
>> +        interrupts = <0x0 IRQ_TYPE_LEVEL_HIGH>;
>> +    };
>>
>> Does this modification meet the expectation?
> Yes, assuming ls7a is a specific SoC, not a family of SoC.
>
> Best regards,
> Krzysztof

I got it, thank you for your reply. I will update the discussion

content in the next version.

Best regards,

Haowei Zheng