Messages in this thread | | | Date | Fri, 10 Jun 2022 15:43:15 +0200 | Subject | Re: [PATCH 2/3] tty: serial: atmel: improve clock management | From | Richard Genoud <> |
| |
Hi, Le 08/06/2022 à 10:18, Claudiu.Beznea@microchip.com a écrit : > Hi, > > On 02.06.2022 12:29, Richard Genoud wrote: >> >> Hi, >> >> Le 25/05/2022 à 15:37, Claudiu Beznea a écrit : >>> atmel_port->clk was requested in atmel_init_port() (which is called only >>> on probe path) only if atmel_port->clk was NULL. But atmel_port->clk is >>> NULL on probing path. Thus don't check this. Along with this the clock is >>> now requested with devm_clk_get() and the clock request has been moved in >>> atmel_serial_probe() function to avoid disabling/re-enabling it on probe >>> path for multiple times. All the checks of atmel_port->clk were removed >>> along with clk_put() and atmel_port->clk = NULL. Now, on probing time the >>> clock is enabled at the beginning and disabled at the end of probe. As >>> atmel_console_setup() is called in the middle of probe and clock is >>> already enabled at that time the clk_prepare_enable() in >>> atmel_console_setup() has also been removed. >> Could you split this patch into smaller steps ? >> I think it will be easier to read and review. > > I kept it as a single patch as it is all related to clock management. > > Having the clock enabled only at the beginning of probe and disabled at the > end of probe lead to removing the code in atmel_init_port(), also removing > the code under CONFIG_SERIAL_ATMEL_CONSOLE in probe. Same for the rest of > the removed code. > > With this, would you still want to split it in multiple patches?
I think that, at least, the switch from clk_get() to devm_clk_get() can be in a separate patch. But in general, I prefer when patches are self-explaining rather than a bigger patch with a longer explanation of what it does in the commit message.
Regards, Richard
> > Thank you, > Claudiu Beznea >>> >>> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com> >>> --- >>> drivers/tty/serial/atmel_serial.c | 65 +++++++------------------------ >>> 1 file changed, 15 insertions(+), 50 deletions(-) >>> >>> diff --git a/drivers/tty/serial/atmel_serial.c >>> b/drivers/tty/serial/atmel_serial.c >>> index 5c793a23dc54..2955b1012014 100644 >>> --- a/drivers/tty/serial/atmel_serial.c >>> +++ b/drivers/tty/serial/atmel_serial.c >>> @@ -2501,24 +2501,7 @@ static int atmel_init_port(struct atmel_uart_port >>> *atmel_port, >>> if (ret) >>> return ret; >>> >>> - /* for console, the clock could already be configured */ >>> - if (!atmel_port->clk) { >>> - atmel_port->clk = clk_get(&mpdev->dev, "usart"); >>> - if (IS_ERR(atmel_port->clk)) { >>> - ret = PTR_ERR(atmel_port->clk); >>> - atmel_port->clk = NULL; >>> - return ret; >>> - } >>> - ret = clk_prepare_enable(atmel_port->clk); >>> - if (ret) { >>> - clk_put(atmel_port->clk); >>> - atmel_port->clk = NULL; >>> - return ret; >>> - } >>> - port->uartclk = clk_get_rate(atmel_port->clk); >>> - clk_disable_unprepare(atmel_port->clk); >>> - /* only enable clock when USART is in use */ >>> - } >>> + port->uartclk = clk_get_rate(atmel_port->clk); >>> >>> /* >>> * Use TXEMPTY for interrupt when rs485 or ISO7816 else TXRDY or >>> @@ -2640,10 +2623,6 @@ static int __init atmel_console_setup(struct >>> console *co, char *options) >>> return -ENODEV; >>> } >>> >>> - ret = clk_prepare_enable(atmel_ports[co->index].clk); >>> - if (ret) >>> - return ret; >>> - >> Now, "int ret;" is unused, you can remove it. >> >>> atmel_uart_writel(port, ATMEL_US_IDR, -1); >>> atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_RSTSTA | >>> ATMEL_US_RSTRX); >>> atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXEN | ATMEL_US_RXEN); >>> @@ -2889,14 +2868,23 @@ static int atmel_serial_probe(struct >>> platform_device *pdev) >>> atomic_set(&atmel_port->tasklet_shutdown, 0); >>> spin_lock_init(&atmel_port->lock_suspended); >>> >>> + atmel_port->clk = devm_clk_get(&pdev->dev, "usart"); >>> + if (IS_ERR(atmel_port->clk)) { >>> + ret = PTR_ERR(atmel_port->clk); >>> + goto err; >>> + } >>> + ret = clk_prepare_enable(atmel_port->clk); >>> + if (ret) >>> + goto err; >>> + >>> ret = atmel_init_port(atmel_port, pdev); >>> if (ret) >>> - goto err_clear_bit; >>> + goto err_clk_disable_unprepare; >>> >>> atmel_port->gpios = mctrl_gpio_init(&atmel_port->uart, 0); >>> if (IS_ERR(atmel_port->gpios)) { >>> ret = PTR_ERR(atmel_port->gpios); >>> - goto err_clear_bit; >>> + goto err_clk_disable_unprepare; >>> } >>> >>> if (!atmel_use_pdc_rx(&atmel_port->uart)) { >>> @@ -2905,7 +2893,7 @@ static int atmel_serial_probe(struct >>> platform_device *pdev) >>> sizeof(struct atmel_uart_char), >>> GFP_KERNEL); >>> if (!data) >>> - goto err_alloc_ring; >>> + goto err_clk_disable_unprepare; >>> atmel_port->rx_ring.buf = data; >>> } >>> >>> @@ -2915,26 +2903,9 @@ static int atmel_serial_probe(struct >>> platform_device *pdev) >>> if (ret) >>> goto err_add_port; >>> >>> -#ifdef CONFIG_SERIAL_ATMEL_CONSOLE >>> - if (uart_console(&atmel_port->uart) >>> - && ATMEL_CONSOLE_DEVICE->flags & CON_ENABLED) { >>> - /* >>> - * The serial core enabled the clock for us, so undo >>> - * the clk_prepare_enable() in atmel_console_setup() >>> - */ >>> - clk_disable_unprepare(atmel_port->clk); >>> - } >>> -#endif >>> - >>> device_init_wakeup(&pdev->dev, 1); >>> platform_set_drvdata(pdev, atmel_port); >>> >>> - /* >>> - * The peripheral clock has been disabled by atmel_init_port(): >>> - * enable it before accessing I/O registers >>> - */ >>> - clk_prepare_enable(atmel_port->clk); >>> - >>> if (rs485_enabled) { >>> atmel_uart_writel(&atmel_port->uart, ATMEL_US_MR, >>> ATMEL_US_USMODE_NORMAL); >>> @@ -2958,12 +2929,8 @@ static int atmel_serial_probe(struct >>> platform_device *pdev) >>> err_add_port: >>> kfree(atmel_port->rx_ring.buf); >>> atmel_port->rx_ring.buf = NULL; >>> -err_alloc_ring: >>> - if (!uart_console(&atmel_port->uart)) { >>> - clk_put(atmel_port->clk); >>> - atmel_port->clk = NULL; >>> - } >>> -err_clear_bit: >>> +err_clk_disable_unprepare: >>> + clk_disable_unprepare(atmel_port->clk); >>> clear_bit(atmel_port->uart.line, atmel_ports_in_use); >>> err: >>> return ret; >>> @@ -2997,8 +2964,6 @@ static int atmel_serial_remove(struct >>> platform_device *pdev) >>> >>> clear_bit(port->line, atmel_ports_in_use); >>> >>> - clk_put(atmel_port->clk); >>> - atmel_port->clk = NULL; >>> pdev->dev.of_node = NULL; >>> >>> return ret; >> Thanks ! >> >> Regards, >> Richard >
| |