> -----Original Message-----
> From: Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>
> Sent: Wednesday, November 10, 2021 5:50 PM
> To: Hammer Hsieh <hammerh0314@xxxxxxxxx>; gregkh@xxxxxxxxxxxxxxxxxxx;
> robh+dt@xxxxxxxxxx; linux-serial@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx; jirislaby@xxxxxxxxxx
> Cc: Tony Huang 黃懷厚 <tony.huang@xxxxxxxxxxx>; Wells Lu 呂芳騰
> <wells.lu@xxxxxxxxxxx>; Hammer Hsieh 謝宏孟
> <hammer.hsieh@xxxxxxxxxxx>
> 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:
> [...]
> > +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.
> [...]
> > +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.
> > + 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.
> > + 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.
>
> > + 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);
> > + 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) {
> > + 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.
> > + 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.
> > +
> > + 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.
> > +
> > + 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.
> [...]
> > +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
