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?
Thank you,
Claudiu Beznea
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea@xxxxxxxxxxxxx>
>> ---
>> 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
