-----Original Message-----
From: Philipp Zabel <p.zabel@pengutronix.de>
Sent: Wednesday, November 10, 2021 5:50 PM
To: Hammer Hsieh <hammerh0314@gmail.com>; gregkh@linuxfoundation.org;
robh+dt@kernel.org; linux-serial@vger.kernel.org; devicetree@vger.kernel.org;
linux-kernel@vger.kernel.org; jirislaby@kernel.org
Cc: Tony Huang 黃懷厚 <redacted>; Wells Lu 呂芳騰
[off-list ref]; Hammer Hsieh 謝宏孟
[off-list ref]
Subject: Re: [PATCH v2 2/2] serial:sunplus-uart:Add Sunplus SoC UART Driver

Hi,

On Wed, 2021-11-10 at 15:51 +0800, Hammer Hsieh wrote:
[...]

quoted

+struct sunplus_uart_port {
+	char name[16];
+	struct uart_port uport;
+	struct sunplus_dma_info *uartdma_rx;
+	struct sunplus_dma_info *uartdma_tx;
+	struct clk *clk;
+	struct reset_control *rstc;
+	unsigned int pllsys_rate;
+	struct gpio_desc *rts_gpio;
+	struct hrtimer rts_check_tx_empty;
+	struct hrtimer rts_delay_before_send;
+	struct hrtimer rts_delay_after_send; }; struct sunplus_uart_port
+sunplus_uart_ports[UART_NR];

Does this have to be a global array? I would expect these to be allocated in the
probe function, one at a time.

With console, it uses global array.
  Such as:
  /drivers/tty/serial/ar933x_uart.c
  #ifdef CONFIG_SERIAL_AR933X_CONSOLE
  ar933x_uart_port * ar933x_console_port[ ];
With normal use, it should probe once at a time.
I will think about how to modify it.

[...]

quoted

+static int sunplus_uart_probe(struct platform_device *pdev) {
+	struct resource *res_mem;
+	struct uart_port *port;
+	struct clk *clk, *pllsys;
+	unsigned int pllsys_rate;
+	int ret, irq;
+	int idx_offset, idx;
+	int idx_which_uart;
+	char peri_name[16];
+
+	if (pdev->dev.of_node) {
+		pdev->id = of_alias_get_id(pdev->dev.of_node, "serial");
+		if (pdev->id < 0)
+			pdev->id = of_alias_get_id(pdev->dev.of_node, "uart");
+	}
+
+	idx_offset = -1;
+
+	if (IS_UARTDMARX_ID(pdev->id))
+		idx_offset = 0;
+	else if (IS_UARTDMATX_ID(pdev->id))
+		idx_offset = UART_DMARX_NR;
+
+	/* init txdma or rxdma */
+	if (idx_offset >= 0) {
+		clk = devm_clk_get(&pdev->dev, NULL);

Should this be requested by name? Looking at the binding, this could be
UADMA or HWUA?

Yes, it is better requested by name.
But , if request by name , I can't probe both dma_tx and dma_rx with one line.
I will think about how to modify it.

quoted

+		if (IS_ERR(clk))
+			return PTR_ERR(clk);
+
+		ret = clk_prepare_enable(clk);
+		if (ret)
+			return ret;

I suggest to move this down after all required resources are available.
Otherwise you'll have to either disable the clock in the error paths, or you are
left with a running clock if anything below fails.

OK, I will modify it.

quoted

+		if (idx_offset == 0)
+			idx = idx_offset + pdev->id - ID_BASE_DMARX;
+		else
+			idx = idx_offset + pdev->id - ID_BASE_DMATX;
+
+		res_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+		if (!res_mem)
+			return -ENODEV;
+
+		sprintf(peri_name, "PERI%d", (idx & 0x01));
+
+		clk = devm_clk_get(&pdev->dev, peri_name);
+		if (IS_ERR(clk))
+			return PTR_ERR(clk);
+
+		ret = clk_prepare_enable(clk);

Same as above.

OK, I will modify it.

quoted

+		if (ret)
+			return ret;
+
+		sunplus_uartdma[idx].addr_phy =
+			(unsigned long)(res_mem->start);
+		sunplus_uartdma[idx].membase =
+			devm_ioremap_resource(&pdev->dev, res_mem);
+
+		if (IS_ERR(sunplus_uartdma[idx].membase))
+			return PTR_ERR(sunplus_uartdma[idx].membase);
+
+		if (IS_UARTDMARX_ID(pdev->id)) {
+			irq = platform_get_irq(pdev, 0);
+			if (irq < 0)
+				return -ENODEV;
+
+			sunplus_uartdma[idx].irq = irq;
+		} else {
+			res_mem = platform_get_resource(pdev, IORESOURCE_MEM,

1);

quoted

+			if (!res_mem)
+				return -ENODEV;
+
+			sunplus_uartdma[idx].gdma_membase =
+				devm_ioremap_resource(&pdev->dev, res_mem);
+
+			if (IS_ERR(sunplus_uartdma[idx].gdma_membase))
+				return -EINVAL;
+		}
+
+		if (of_property_read_u32(pdev->dev.of_node, "which-uart",

&idx_which_uart) != 0) {

quoted

+			dev_err(&pdev->dev, "\"which-uart\" is not assigned.");
+			return -EINVAL;
+		}
+
+		if (idx_which_uart >= UART_NR) {
+			dev_err(&pdev->dev, "\"which-uart\" is not valid.");
+			return -EINVAL;
+		}
+
+		sunplus_uartdma[idx].which_uart = idx_which_uart;
+
+		return 0;
+	} else if (pdev->id < 0 || pdev->id >= UART_NR)
+		return -EINVAL;
+
+	/* init uart */
+	port = &sunplus_uart_ports[pdev->id].uport;
+	if (port->membase)
+		return -EBUSY;
+
+	memset(port, 0, sizeof(*port));
+
+	res_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res_mem)
+		return -ENODEV;
+
+	port->dev = &pdev->dev;
+	port->mapbase = res_mem->start;
+
+	port->membase = devm_ioremap_resource(&pdev->dev, res_mem);
+	if (IS_ERR(port->membase))
+		return PTR_ERR(port->membase);
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0)
+		return -ENODEV;
+
+	uart_get_rs485_mode(port);
+	sunplus_uart_ports[pdev->id].rts_gpio =
+		devm_gpiod_get(&pdev->dev, "rts", GPIOD_OUT_LOW);
+	port->rs485_config = sunplus_uart_config_rs485;
+	sunplus_uart_ports[pdev->id].rts_check_tx_empty.function = NULL;
+	sunplus_uart_ports[pdev->id].rts_delay_before_send.function = NULL;
+	sunplus_uart_ports[pdev->id].rts_delay_after_send.function = NULL;
+	if (port->rs485.flags & SER_RS485_ENABLED)
+		sunplus_uart_rs485_on(port);
+
+	sunplus_uart_ports[pdev->id].clk = devm_clk_get(&pdev->dev, NULL);

Here the same nameless clock as in the loop above is requested again.
Should this be UADMA or HWUA?

No, here should be UART clk, each uart didn't use "clock-names" , that's why we use NULL.
In dts UART only use clocks=<&clkc UA0> or <&clkc UA1> or <&clkc UA2>.
We did not use "clock-names".
uart_probe( ) , first init dma , then init uart.

quoted

+	if (IS_ERR(sunplus_uart_ports[pdev->id].clk))
+		return PTR_ERR(sunplus_uart_ports[pdev->id].clk);
+
+	ret = clk_prepare_enable(sunplus_uart_ports[pdev->id].clk);
+	if (ret)
+		return ret;

Same comment as above. Better to request the reset control before enabling
the clock, for example.

OK, I will modify it.

quoted

+
+	sunplus_uart_ports[pdev->id].rstc =
+		devm_reset_control_get(&pdev->dev, NULL);

Please use devm_reset_control_get_exclusive() instead.

OK, I will modify it.

quoted

+
+	if (IS_ERR(sunplus_uart_ports[pdev->id].rstc))
+		return PTR_ERR(sunplus_uart_ports[pdev->id].rstc);
+
+	ret = reset_control_deassert(sunplus_uart_ports[pdev->id].rstc);
+	if (ret)
+		return ret;
+
+	clk = sunplus_uart_ports[pdev->id].clk;
+	if (IS_ERR(clk))

This can't ever be true, the code above already returned in this case.

Indeed, I will modify it.

[...]

quoted

+static int sunplus_uart_remove(struct platform_device *pdev) { #ifdef
+CONFIG_PM_RUNTIME_UART
+	if (pdev->id != 0) {
+		pm_runtime_disable(&pdev->dev);
+		pm_runtime_set_suspended(&pdev->dev);
+	}
+#endif
+	uart_remove_one_port(&sunplus_uart_driver,
+		&sunplus_uart_ports[pdev->id].uport);
+
+	if (pdev->id < UART_NR) {
+		clk_disable_unprepare(sunplus_uart_ports[pdev->id].clk);
+		reset_control_assert(sunplus_uart_ports[pdev->id].rstc);
+	}

What about the PERI clocks? This seems to leave them enabled.

In case of dma enable, we should consider it.
I will modify it.

regards
Philipp