diff mbox series

[5/5] tty: serial: atmel: Make the driver aware of the existence of GCLK

Message ID 20220817075517.49575-6-sergiu.moga@microchip.com
State New
Headers show
Series Make atmel serial driver aware of GCLK | expand

Commit Message

Sergiu Moga Aug. 17, 2022, 7:55 a.m. UTC
Previously, the atmel serial driver did not take into account the
possibility of using the more customizable generic clock as its
baudrate generator. Unless there is a Fractional Part available to
increase accuracy, there is a high chance that we may be able to
generate a baudrate closer to the desired one by using the GCLK as the
clock source. Now, depending on the error rate between
the desired baudrate and the actual baudrate, the serial driver will
fallback on the generic clock. The generic clock must be provided
in the DT node of the serial that may need a more flexible clock source.

Signed-off-by: Sergiu Moga <sergiu.moga@microchip.com>
---
 drivers/tty/serial/atmel_serial.c | 52 ++++++++++++++++++++++++++++++-
 drivers/tty/serial/atmel_serial.h |  1 +
 2 files changed, 52 insertions(+), 1 deletion(-)

Comments

Richard Genoud Aug. 30, 2022, 5:29 p.m. UTC | #1
Le 17/08/2022 à 09:55, Sergiu Moga a écrit :
> Previously, the atmel serial driver did not take into account the
> possibility of using the more customizable generic clock as its
> baudrate generator. Unless there is a Fractional Part available to
> increase accuracy, there is a high chance that we may be able to
> generate a baudrate closer to the desired one by using the GCLK as the
> clock source. Now, depending on the error rate between
> the desired baudrate and the actual baudrate, the serial driver will
> fallback on the generic clock. The generic clock must be provided
> in the DT node of the serial that may need a more flexible clock source.
> 
> Signed-off-by: Sergiu Moga <sergiu.moga@microchip.com>
> ---
>   drivers/tty/serial/atmel_serial.c | 52 ++++++++++++++++++++++++++++++-
>   drivers/tty/serial/atmel_serial.h |  1 +
>   2 files changed, 52 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
> index 30ba9eef7b39..0a0b46ee0955 100644
> --- a/drivers/tty/serial/atmel_serial.c
> +++ b/drivers/tty/serial/atmel_serial.c
> @@ -15,6 +15,7 @@
>   #include <linux/init.h>
>   #include <linux/serial.h>
>   #include <linux/clk.h>
> +#include <linux/clk-provider.h>
>   #include <linux/console.h>
>   #include <linux/sysrq.h>
>   #include <linux/tty_flip.h>
> @@ -77,6 +78,8 @@ static void atmel_stop_rx(struct uart_port *port);
>   #endif
>   
>   #define ATMEL_ISR_PASS_LIMIT	256
> +#define ERROR_RATE(desired_value, actual_value) \
> +	((int)(100 - ((desired_value) * 100) / (actual_value)))
>   
>   struct atmel_dma_buffer {
>   	unsigned char	*buf;
> @@ -110,6 +113,7 @@ struct atmel_uart_char {
>   struct atmel_uart_port {
>   	struct uart_port	uart;		/* uart */
>   	struct clk		*clk;		/* uart clock */
> +	struct clk		*gclk;		/* uart generic clock */
>   	int			may_wakeup;	/* cached value of device_may_wakeup for times we need to disable it */
>   	u32			backup_imr;	/* IMR saved during suspend */
>   	int			break_active;	/* break being received */
> @@ -2115,6 +2119,8 @@ static void atmel_serial_pm(struct uart_port *port, unsigned int state,
>   		 * This is called on uart_close() or a suspend event.
>   		 */
>   		clk_disable_unprepare(atmel_port->clk);
> +		if (atmel_port->gclk && __clk_is_enabled(atmel_port->gclk))
> +			clk_disable_unprepare(atmel_port->gclk);
>   		break;
>   	default:
>   		dev_err(port->dev, "atmel_serial: unknown pm %d\n", state);
> @@ -2129,7 +2135,8 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
>   {
>   	struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
>   	unsigned long flags;
> -	unsigned int old_mode, mode, imr, quot, baud, div, cd, fp = 0;
> +	unsigned int old_mode, mode, imr, quot, div, cd, fp = 0;
> +	unsigned int baud, actual_baud, gclk_rate;
>   
>   	/* save the current mode register */
>   	mode = old_mode = atmel_uart_readl(port, ATMEL_US_MR);
> @@ -2288,6 +2295,37 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
>   		cd /= 8;
>   		mode |= ATMEL_US_USCLKS_MCK_DIV8;
>   	}
> +
> +	/*
> +	 * If there is no Fractional Part, there is a high chance that
> +	 * we may be able to generate a baudrate closer to the desired one
> +	 * if we use the GCLK as the clock source driving the baudrate
> +	 * generator.
> +	 */
> +	if (!fp && atmel_port->gclk) {
> +		if (__clk_is_enabled(atmel_port->gclk))
> +			clk_disable_unprepare(atmel_port->gclk);
> +		clk_set_rate(atmel_port->gclk, 16 * baud);
> +		gclk_rate = clk_get_rate(atmel_port->gclk);
> +		actual_baud = clk_get_rate(atmel_port->clk) / (16 * cd);
> +		if (abs(ERROR_RATE(baud, actual_baud)) >
> +		    abs(ERROR_RATE(baud, gclk_rate / 16))) {
> +			mode |= ATMEL_US_GCLK;
> +
> +			/*
> +			 * Set the Clock Divisor for GCLK to 1.
> +			 * Since we were able to generate the smallest
> +			 * multiple of the desired baudrate times 16,
> +			 * then we surely can generate a bigger multiple
> +			 * with the exact error rate for an equally increased
> +			 * CD. Thus no need to take into account
> +			 * a higher value for CD.
> +			 */
> +			cd = 1;
> +			clk_prepare_enable(atmel_port->gclk);
> +		}
> +	}
> +
>   	quot = cd | fp << ATMEL_US_FP_OFFSET;
>   
>   	if (!(port->iso7816.flags & SER_ISO7816_ENABLED))
> @@ -2883,6 +2921,16 @@ static int atmel_serial_probe(struct platform_device *pdev)
>   	if (ret)
>   		goto err;
>   
> +	atmel_port->gclk = devm_clk_get_optional(&pdev->dev, "gclk");
> +	if (atmel_port->gclk) {
> +		ret = clk_prepare_enable(atmel_port->gclk);
> +		if (ret) {
> +			atmel_port->gclk = NULL;
> +			return ret;
> +		}
> +		clk_disable_unprepare(atmel_port->gclk);
> +	}
> +
>   	ret = atmel_init_port(atmel_port, pdev);
>   	if (ret)
>   		goto err_clk_disable_unprepare;
> @@ -2929,6 +2977,8 @@ static int atmel_serial_probe(struct platform_device *pdev)
>   	 * is used
>   	 */
>   	clk_disable_unprepare(atmel_port->clk);
> +	if (atmel_port->gclk && __clk_is_enabled(atmel_port->gclk))
> +		clk_disable_unprepare(atmel_port->gclk);
>   
>   	return 0;
>   
> diff --git a/drivers/tty/serial/atmel_serial.h b/drivers/tty/serial/atmel_serial.h
> index 0d8a0f9cc5c3..fb718972f81a 100644
> --- a/drivers/tty/serial/atmel_serial.h
> +++ b/drivers/tty/serial/atmel_serial.h
> @@ -63,6 +63,7 @@
>   #define		ATMEL_US_PAR_MARK		(3 <<  9)
>   #define		ATMEL_US_PAR_NONE		(4 <<  9)
>   #define		ATMEL_US_PAR_MULTI_DROP		(6 <<  9)
> +#define ATMEL_US_GCLK                          BIT(12)
>   #define	ATMEL_US_NBSTOP		GENMASK(13, 12)	/* Number of Stop Bits */
>   #define		ATMEL_US_NBSTOP_1		(0 << 12)
>   #define		ATMEL_US_NBSTOP_1_5		(1 << 12)

Correct me if I'm wrong, but GCLK is selected by the bit 12 only in UART_MR, not in USART_MR.
In USART_MR, it seems to be controlled by bits 4-5 (and bit 12 is for stop bits, as we can see above, and in the datasheet).
cf https://ww1.microchip.com/downloads/aemDocuments/documents/MPU32/ProductDocuments/DataSheets/SAMA5D2-Series-Datasheet-DS60001476H.pdf
page 1637

Regards,
Richard.
Sergiu Moga Aug. 31, 2022, 8:49 a.m. UTC | #2
On 30.08.2022 20:29, Richard Genoud wrote:
> 
> Le 17/08/2022 à 09:55, Sergiu Moga a écrit :
>> Previously, the atmel serial driver did not take into account the
>> possibility of using the more customizable generic clock as its
>> baudrate generator. Unless there is a Fractional Part available to
>> increase accuracy, there is a high chance that we may be able to
>> generate a baudrate closer to the desired one by using the GCLK as the
>> clock source. Now, depending on the error rate between
>> the desired baudrate and the actual baudrate, the serial driver will
>> fallback on the generic clock. The generic clock must be provided
>> in the DT node of the serial that may need a more flexible clock source.
>>
>> Signed-off-by: Sergiu Moga <sergiu.moga@microchip.com>
>> ---
>>   drivers/tty/serial/atmel_serial.c | 52 ++++++++++++++++++++++++++++++-
>>   drivers/tty/serial/atmel_serial.h |  1 +
>>   2 files changed, 52 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/tty/serial/atmel_serial.c 
>> b/drivers/tty/serial/atmel_serial.c
>> index 30ba9eef7b39..0a0b46ee0955 100644
>> --- a/drivers/tty/serial/atmel_serial.c
>> +++ b/drivers/tty/serial/atmel_serial.c
>> @@ -15,6 +15,7 @@
>>   #include <linux/init.h>
>>   #include <linux/serial.h>
>>   #include <linux/clk.h>
>> +#include <linux/clk-provider.h>
>>   #include <linux/console.h>
>>   #include <linux/sysrq.h>
>>   #include <linux/tty_flip.h>
>> @@ -77,6 +78,8 @@ static void atmel_stop_rx(struct uart_port *port);
>>   #endif
>>
>>   #define ATMEL_ISR_PASS_LIMIT        256
>> +#define ERROR_RATE(desired_value, actual_value) \
>> +     ((int)(100 - ((desired_value) * 100) / (actual_value)))
>>
>>   struct atmel_dma_buffer {
>>       unsigned char   *buf;
>> @@ -110,6 +113,7 @@ struct atmel_uart_char {
>>   struct atmel_uart_port {
>>       struct uart_port        uart;           /* uart */
>>       struct clk              *clk;           /* uart clock */
>> +     struct clk              *gclk;          /* uart generic clock */
>>       int                     may_wakeup;     /* cached value of 
>> device_may_wakeup for times we need to disable it */
>>       u32                     backup_imr;     /* IMR saved during 
>> suspend */
>>       int                     break_active;   /* break being received */
>> @@ -2115,6 +2119,8 @@ static void atmel_serial_pm(struct uart_port 
>> *port, unsigned int state,
>>                * This is called on uart_close() or a suspend event.
>>                */
>>               clk_disable_unprepare(atmel_port->clk);
>> +             if (atmel_port->gclk && __clk_is_enabled(atmel_port->gclk))
>> +                     clk_disable_unprepare(atmel_port->gclk);
>>               break;
>>       default:
>>               dev_err(port->dev, "atmel_serial: unknown pm %d\n", state);
>> @@ -2129,7 +2135,8 @@ static void atmel_set_termios(struct uart_port 
>> *port, struct ktermios *termios,
>>   {
>>       struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
>>       unsigned long flags;
>> -     unsigned int old_mode, mode, imr, quot, baud, div, cd, fp = 0;
>> +     unsigned int old_mode, mode, imr, quot, div, cd, fp = 0;
>> +     unsigned int baud, actual_baud, gclk_rate;
>>
>>       /* save the current mode register */
>>       mode = old_mode = atmel_uart_readl(port, ATMEL_US_MR);
>> @@ -2288,6 +2295,37 @@ static void atmel_set_termios(struct uart_port 
>> *port, struct ktermios *termios,
>>               cd /= 8;
>>               mode |= ATMEL_US_USCLKS_MCK_DIV8;
>>       }
>> +
>> +     /*
>> +      * If there is no Fractional Part, there is a high chance that
>> +      * we may be able to generate a baudrate closer to the desired one
>> +      * if we use the GCLK as the clock source driving the baudrate
>> +      * generator.
>> +      */
>> +     if (!fp && atmel_port->gclk) {
>> +             if (__clk_is_enabled(atmel_port->gclk))
>> +                     clk_disable_unprepare(atmel_port->gclk);
>> +             clk_set_rate(atmel_port->gclk, 16 * baud);
>> +             gclk_rate = clk_get_rate(atmel_port->gclk);
>> +             actual_baud = clk_get_rate(atmel_port->clk) / (16 * cd);
>> +             if (abs(ERROR_RATE(baud, actual_baud)) >
>> +                 abs(ERROR_RATE(baud, gclk_rate / 16))) {
>> +                     mode |= ATMEL_US_GCLK;
>> +
>> +                     /*
>> +                      * Set the Clock Divisor for GCLK to 1.
>> +                      * Since we were able to generate the smallest
>> +                      * multiple of the desired baudrate times 16,
>> +                      * then we surely can generate a bigger multiple
>> +                      * with the exact error rate for an equally 
>> increased
>> +                      * CD. Thus no need to take into account
>> +                      * a higher value for CD.
>> +                      */
>> +                     cd = 1;
>> +                     clk_prepare_enable(atmel_port->gclk);
>> +             }
>> +     }
>> +
>>       quot = cd | fp << ATMEL_US_FP_OFFSET;
>>
>>       if (!(port->iso7816.flags & SER_ISO7816_ENABLED))
>> @@ -2883,6 +2921,16 @@ static int atmel_serial_probe(struct 
>> platform_device *pdev)
>>       if (ret)
>>               goto err;
>>
>> +     atmel_port->gclk = devm_clk_get_optional(&pdev->dev, "gclk");
>> +     if (atmel_port->gclk) {
>> +             ret = clk_prepare_enable(atmel_port->gclk);
>> +             if (ret) {
>> +                     atmel_port->gclk = NULL;
>> +                     return ret;
>> +             }
>> +             clk_disable_unprepare(atmel_port->gclk);
>> +     }
>> +
>>       ret = atmel_init_port(atmel_port, pdev);
>>       if (ret)
>>               goto err_clk_disable_unprepare;
>> @@ -2929,6 +2977,8 @@ static int atmel_serial_probe(struct 
>> platform_device *pdev)
>>        * is used
>>        */
>>       clk_disable_unprepare(atmel_port->clk);
>> +     if (atmel_port->gclk && __clk_is_enabled(atmel_port->gclk))
>> +             clk_disable_unprepare(atmel_port->gclk);
>>
>>       return 0;
>>
>> diff --git a/drivers/tty/serial/atmel_serial.h 
>> b/drivers/tty/serial/atmel_serial.h
>> index 0d8a0f9cc5c3..fb718972f81a 100644
>> --- a/drivers/tty/serial/atmel_serial.h
>> +++ b/drivers/tty/serial/atmel_serial.h
>> @@ -63,6 +63,7 @@
>>   #define             ATMEL_US_PAR_MARK               (3 <<  9)
>>   #define             ATMEL_US_PAR_NONE               (4 <<  9)
>>   #define             ATMEL_US_PAR_MULTI_DROP         (6 <<  9)
>> +#define ATMEL_US_GCLK                          BIT(12)
>>   #define     ATMEL_US_NBSTOP         GENMASK(13, 12) /* Number of 
>> Stop Bits */
>>   #define             ATMEL_US_NBSTOP_1               (0 << 12)
>>   #define             ATMEL_US_NBSTOP_1_5             (1 << 12)
> 
> Correct me if I'm wrong, but GCLK is selected by the bit 12 only in 
> UART_MR, not in USART_MR.
> In USART_MR, it seems to be controlled by bits 4-5 (and bit 12 is for 
> stop bits, as we can see above, and in the datasheet).
> cf 
> https://ww1.microchip.com/downloads/aemDocuments/documents/MPU32/ProductDocuments/DataSheets/SAMA5D2-Series-Datasheet-DS60001476H.pdf 
> 
> page 1637
> 
> Regards,
> Richard.

Yes, you are correct, this should have been called ATMEL_UA_GCLK 
instead. I think I will add both ATMEL_UA_GCLK and ATMEL_US_GCLK bits 
and an additional field in struct atmel_uart_port to hold ATMEL_UA_GCLK 
for UART or ATMEL_US_GCLK for USART. I guess this field should be set in 
atmel_get_ip_name(), the same place where the decision between 
ATMEL_UA_RTOR and ATMEL_US_RTOR is taken.

Thanks,
     Sergiu
Claudiu Beznea Aug. 31, 2022, 9:46 a.m. UTC | #3
On 17.08.2022 10:55, Sergiu Moga wrote:
> Previously, the atmel serial driver did not take into account the
> possibility of using the more customizable generic clock as its
> baudrate generator. Unless there is a Fractional Part available to
> increase accuracy, there is a high chance that we may be able to
> generate a baudrate closer to the desired one by using the GCLK as the
> clock source. Now, depending on the error rate between
> the desired baudrate and the actual baudrate, the serial driver will
> fallback on the generic clock. The generic clock must be provided
> in the DT node of the serial that may need a more flexible clock source.
> 
> Signed-off-by: Sergiu Moga <sergiu.moga@microchip.com>
> ---
>  drivers/tty/serial/atmel_serial.c | 52 ++++++++++++++++++++++++++++++-
>  drivers/tty/serial/atmel_serial.h |  1 +
>  2 files changed, 52 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
> index 30ba9eef7b39..0a0b46ee0955 100644
> --- a/drivers/tty/serial/atmel_serial.c
> +++ b/drivers/tty/serial/atmel_serial.c
> @@ -15,6 +15,7 @@
>  #include <linux/init.h>
>  #include <linux/serial.h>
>  #include <linux/clk.h>
> +#include <linux/clk-provider.h>
>  #include <linux/console.h>
>  #include <linux/sysrq.h>
>  #include <linux/tty_flip.h>
> @@ -77,6 +78,8 @@ static void atmel_stop_rx(struct uart_port *port);
>  #endif
>  
>  #define ATMEL_ISR_PASS_LIMIT	256
> +#define ERROR_RATE(desired_value, actual_value) \
> +	((int)(100 - ((desired_value) * 100) / (actual_value)))
>  
>  struct atmel_dma_buffer {
>  	unsigned char	*buf;
> @@ -110,6 +113,7 @@ struct atmel_uart_char {
>  struct atmel_uart_port {
>  	struct uart_port	uart;		/* uart */
>  	struct clk		*clk;		/* uart clock */
> +	struct clk		*gclk;		/* uart generic clock */
>  	int			may_wakeup;	/* cached value of device_may_wakeup for times we need to disable it */
>  	u32			backup_imr;	/* IMR saved during suspend */
>  	int			break_active;	/* break being received */
> @@ -2115,6 +2119,8 @@ static void atmel_serial_pm(struct uart_port *port, unsigned int state,
>  		 * This is called on uart_close() or a suspend event.
>  		 */
>  		clk_disable_unprepare(atmel_port->clk);
> +		if (atmel_port->gclk && __clk_is_enabled(atmel_port->gclk))

No need to check for atmel_port->gclk != NULL. clk APIs are already doing this.

> +			clk_disable_unprepare(atmel_port->gclk);
>  		break;
>  	default:
>  		dev_err(port->dev, "atmel_serial: unknown pm %d\n", state);
> @@ -2129,7 +2135,8 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
>  {
>  	struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
>  	unsigned long flags;
> -	unsigned int old_mode, mode, imr, quot, baud, div, cd, fp = 0;
> +	unsigned int old_mode, mode, imr, quot, div, cd, fp = 0;
> +	unsigned int baud, actual_baud, gclk_rate;
>  
>  	/* save the current mode register */
>  	mode = old_mode = atmel_uart_readl(port, ATMEL_US_MR);
> @@ -2288,6 +2295,37 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
>  		cd /= 8;
>  		mode |= ATMEL_US_USCLKS_MCK_DIV8;
>  	}
> +
> +	/*
> +	 * If there is no Fractional Part, there is a high chance that
> +	 * we may be able to generate a baudrate closer to the desired one
> +	 * if we use the GCLK as the clock source driving the baudrate
> +	 * generator.
> +	 */
> +	if (!fp && atmel_port->gclk) {
> +		if (__clk_is_enabled(atmel_port->gclk))
> +			clk_disable_unprepare(atmel_port->gclk);

You disabled it here, set new rate but re-enable it conditionally above, is
this intended? the below condition may fail.

> +		clk_set_rate(atmel_port->gclk, 16 * baud);
> +		gclk_rate = clk_get_rate(atmel_port->gclk);

You should be able to use clk_round_rate() here:
		gclk_rate = clk_round_rate(atmel_port->gclk,
					   16 * baudrate);

With this you can re-write all this block something like:

		gclk_rate = clk_round_rate(atmel_port->gclk,
					   16 * baudrate);
		actual_baud = gclk_rate / (16 * cd);
		if (abs(ERROR_RATE(baud, actual_baud)) >
		    abd(ERROR_RATE(baud, gclk_rate / 16))) {
			mode |= ATMEL_US_GCLK;
			cd = 1;
			if (__clk_is_enabled(atmel_port->gclk))
				clk_disable_unprepare(atmel_port->gclk)
			clk_set_rate(atmel_port->gclk, gclk_rate);
			clk_prepare_enable(atmel_port->gclk);
		}


> +		actual_baud = clk_get_rate(atmel_port->clk) / (16 * cd);
> +		if (abs(ERROR_RATE(baud, actual_baud)) >
> +		    abs(ERROR_RATE(baud, gclk_rate / 16))) {
> +			mode |= ATMEL_US_GCLK;
> +
> +			/*
> +			 * Set the Clock Divisor for GCLK to 1.
> +			 * Since we were able to generate the smallest
> +			 * multiple of the desired baudrate times 16,
> +			 * then we surely can generate a bigger multiple
> +			 * with the exact error rate for an equally increased
> +			 * CD. Thus no need to take into account
> +			 * a higher value for CD.
> +			 */
> +			cd = 1;
> +			clk_prepare_enable(atmel_port->gclk);
> +		}
> +	}
> +
>  	quot = cd | fp << ATMEL_US_FP_OFFSET;
>  
>  	if (!(port->iso7816.flags & SER_ISO7816_ENABLED))
> @@ -2883,6 +2921,16 @@ static int atmel_serial_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto err;
>  
> +	atmel_port->gclk = devm_clk_get_optional(&pdev->dev, "gclk");
> +	if (atmel_port->gclk) {
> +		ret = clk_prepare_enable(atmel_port->gclk);
> +		if (ret) {
> +			atmel_port->gclk = NULL;
> +			return ret;
> +		}
> +		clk_disable_unprepare(atmel_port->gclk);

Is there a reason you enable then disable the clock here?

> +	}
> +
>  	ret = atmel_init_port(atmel_port, pdev);
>  	if (ret)
>  		goto err_clk_disable_unprepare;
> @@ -2929,6 +2977,8 @@ static int atmel_serial_probe(struct platform_device *pdev)
>  	 * is used
>  	 */
>  	clk_disable_unprepare(atmel_port->clk);
> +	if (atmel_port->gclk && __clk_is_enabled(atmel_port->gclk))
> +		clk_disable_unprepare(atmel_port->gclk);

Is this due to the enable in atmel_set_termios()? Is that called on probe
path? Also, there is no need to check for atmel_port->gclk as clk APIs are
already doing this.

>  
>  	return 0;
>  
> diff --git a/drivers/tty/serial/atmel_serial.h b/drivers/tty/serial/atmel_serial.h
> index 0d8a0f9cc5c3..fb718972f81a 100644
> --- a/drivers/tty/serial/atmel_serial.h
> +++ b/drivers/tty/serial/atmel_serial.h
> @@ -63,6 +63,7 @@
>  #define		ATMEL_US_PAR_MARK		(3 <<  9)
>  #define		ATMEL_US_PAR_NONE		(4 <<  9)
>  #define		ATMEL_US_PAR_MULTI_DROP		(6 <<  9)
> +#define ATMEL_US_GCLK                          BIT(12)

It seems there are spaces here.

>  #define	ATMEL_US_NBSTOP		GENMASK(13, 12)	/* Number of Stop Bits */
>  #define		ATMEL_US_NBSTOP_1		(0 << 12)
>  #define		ATMEL_US_NBSTOP_1_5		(1 << 12)
Sergiu Moga Aug. 31, 2022, 11:32 a.m. UTC | #4
On 31.08.2022 12:46, Claudiu Beznea wrote:
> On 17.08.2022 10:55, Sergiu Moga wrote:
>> Previously, the atmel serial driver did not take into account the
>> possibility of using the more customizable generic clock as its
>> baudrate generator. Unless there is a Fractional Part available to
>> increase accuracy, there is a high chance that we may be able to
>> generate a baudrate closer to the desired one by using the GCLK as the
>> clock source. Now, depending on the error rate between
>> the desired baudrate and the actual baudrate, the serial driver will
>> fallback on the generic clock. The generic clock must be provided
>> in the DT node of the serial that may need a more flexible clock source.
>>
>> Signed-off-by: Sergiu Moga <sergiu.moga@microchip.com>
>> ---
>>   drivers/tty/serial/atmel_serial.c | 52 ++++++++++++++++++++++++++++++-
>>   drivers/tty/serial/atmel_serial.h |  1 +
>>   2 files changed, 52 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
>> index 30ba9eef7b39..0a0b46ee0955 100644
>> --- a/drivers/tty/serial/atmel_serial.c
>> +++ b/drivers/tty/serial/atmel_serial.c
>> @@ -15,6 +15,7 @@
>>   #include <linux/init.h>
>>   #include <linux/serial.h>
>>   #include <linux/clk.h>
>> +#include <linux/clk-provider.h>
>>   #include <linux/console.h>
>>   #include <linux/sysrq.h>
>>   #include <linux/tty_flip.h>
>> @@ -77,6 +78,8 @@ static void atmel_stop_rx(struct uart_port *port);
>>   #endif
>>   
>>   #define ATMEL_ISR_PASS_LIMIT	256
>> +#define ERROR_RATE(desired_value, actual_value) \
>> +	((int)(100 - ((desired_value) * 100) / (actual_value)))
>>   
>>   struct atmel_dma_buffer {
>>   	unsigned char	*buf;
>> @@ -110,6 +113,7 @@ struct atmel_uart_char {
>>   struct atmel_uart_port {
>>   	struct uart_port	uart;		/* uart */
>>   	struct clk		*clk;		/* uart clock */
>> +	struct clk		*gclk;		/* uart generic clock */
>>   	int			may_wakeup;	/* cached value of device_may_wakeup for times we need to disable it */
>>   	u32			backup_imr;	/* IMR saved during suspend */
>>   	int			break_active;	/* break being received */
>> @@ -2115,6 +2119,8 @@ static void atmel_serial_pm(struct uart_port *port, unsigned int state,
>>   		 * This is called on uart_close() or a suspend event.
>>   		 */
>>   		clk_disable_unprepare(atmel_port->clk);
>> +		if (atmel_port->gclk && __clk_is_enabled(atmel_port->gclk))
> 
> No need to check for atmel_port->gclk != NULL. clk APIs are already doing this.
> 
>> +			clk_disable_unprepare(atmel_port->gclk);
>>   		break;
>>   	default:
>>   		dev_err(port->dev, "atmel_serial: unknown pm %d\n", state);
>> @@ -2129,7 +2135,8 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
>>   {
>>   	struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
>>   	unsigned long flags;
>> -	unsigned int old_mode, mode, imr, quot, baud, div, cd, fp = 0;
>> +	unsigned int old_mode, mode, imr, quot, div, cd, fp = 0;
>> +	unsigned int baud, actual_baud, gclk_rate;
>>   
>>   	/* save the current mode register */
>>   	mode = old_mode = atmel_uart_readl(port, ATMEL_US_MR);
>> @@ -2288,6 +2295,37 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
>>   		cd /= 8;
>>   		mode |= ATMEL_US_USCLKS_MCK_DIV8;
>>   	}
>> +
>> +	/*
>> +	 * If there is no Fractional Part, there is a high chance that
>> +	 * we may be able to generate a baudrate closer to the desired one
>> +	 * if we use the GCLK as the clock source driving the baudrate
>> +	 * generator.
>> +	 */
>> +	if (!fp && atmel_port->gclk) {
>> +		if (__clk_is_enabled(atmel_port->gclk))
>> +			clk_disable_unprepare(atmel_port->gclk);
> 
> You disabled it here, set new rate but re-enable it conditionally above, is
> this intended? the below condition may fail.
> 

Yes, it is intended. Gclk should remain disabled if the below condition 
fails.


>> +		clk_set_rate(atmel_port->gclk, 16 * baud);
>> +		gclk_rate = clk_get_rate(atmel_port->gclk);
> 
> You should be able to use clk_round_rate() here:
> 		gclk_rate = clk_round_rate(atmel_port->gclk,
> 					   16 * baudrate);
> 
> With this you can re-write all this block something like:
> 
> 		gclk_rate = clk_round_rate(atmel_port->gclk,
> 					   16 * baudrate);
> 		actual_baud = gclk_rate / (16 * cd);
> 		if (abs(ERROR_RATE(baud, actual_baud)) >
> 		    abd(ERROR_RATE(baud, gclk_rate / 16))) {
> 			mode |= ATMEL_US_GCLK;
> 			cd = 1;
> 			if (__clk_is_enabled(atmel_port->gclk))
> 				clk_disable_unprepare(atmel_port->gclk)
> 			clk_set_rate(atmel_port->gclk, gclk_rate);
> 			clk_prepare_enable(atmel_port->gclk);
> 		}
> 
> 

Hmm, yes, you are right, it is much better with this clk_round_rate() 
since there would be no need to disable gclk before setting the new 
rate, I did not know of this function at that point. However, in this 
case, the if(abs...) will also need an else to disable the gclk in case 
the condition failed and it was somehow enabled before.


>> +		actual_baud = clk_get_rate(atmel_port->clk) / (16 * cd);
>> +		if (abs(ERROR_RATE(baud, actual_baud)) >
>> +		    abs(ERROR_RATE(baud, gclk_rate / 16))) {
>> +			mode |= ATMEL_US_GCLK;
>> +
>> +			/*
>> +			 * Set the Clock Divisor for GCLK to 1.
>> +			 * Since we were able to generate the smallest
>> +			 * multiple of the desired baudrate times 16,
>> +			 * then we surely can generate a bigger multiple
>> +			 * with the exact error rate for an equally increased
>> +			 * CD. Thus no need to take into account
>> +			 * a higher value for CD.
>> +			 */
>> +			cd = 1;
>> +			clk_prepare_enable(atmel_port->gclk);
>> +		}
>> +	}
>> +
>>   	quot = cd | fp << ATMEL_US_FP_OFFSET;
>>   
>>   	if (!(port->iso7816.flags & SER_ISO7816_ENABLED))
>> @@ -2883,6 +2921,16 @@ static int atmel_serial_probe(struct platform_device *pdev)
>>   	if (ret)
>>   		goto err;
>>   
>> +	atmel_port->gclk = devm_clk_get_optional(&pdev->dev, "gclk");
>> +	if (atmel_port->gclk) {
>> +		ret = clk_prepare_enable(atmel_port->gclk);
>> +		if (ret) {
>> +			atmel_port->gclk = NULL;
>> +			return ret;
>> +		}
>> +		clk_disable_unprepare(atmel_port->gclk);
> 
> Is there a reason you enable then disable the clock here?
> 


I think it's better to make sure in the probe method that enabling the 
gclk issues no errors, so that the error does not appear in 
set_termios(). Since the user must place the optional gclk in DT if they 
want a finer rate when missing the Fractional Part, I think they should 
know before even trying to open the port that it is first correctly 
setup in the clock tree as well by making the probe method fail from the 
very beginning in case it is not.

So, I first enable it and make sure there are no errors and then disable 
it because I do not see the point of having an enabled clock that might 
not be used.


>> +	}
>> +
>>   	ret = atmel_init_port(atmel_port, pdev);
>>   	if (ret)
>>   		goto err_clk_disable_unprepare;
>> @@ -2929,6 +2977,8 @@ static int atmel_serial_probe(struct platform_device *pdev)
>>   	 * is used
>>   	 */
>>   	clk_disable_unprepare(atmel_port->clk);
>> +	if (atmel_port->gclk && __clk_is_enabled(atmel_port->gclk))
>> +		clk_disable_unprepare(atmel_port->gclk);
> 
> Is this due to the enable in atmel_set_termios()? Is that called on probe
> path? Also, there is no need to check for atmel_port->gclk as clk APIs are
> already doing this.
> 


No, I guess this is not really needed, since it is disabled once it is 
claimed from DT if enabling it succeeds. I initially placed this code 
sequence wherever the peripheral clock is disabled as well.

>>   
>>   	return 0;
>>   
>> diff --git a/drivers/tty/serial/atmel_serial.h b/drivers/tty/serial/atmel_serial.h
>> index 0d8a0f9cc5c3..fb718972f81a 100644
>> --- a/drivers/tty/serial/atmel_serial.h
>> +++ b/drivers/tty/serial/atmel_serial.h
>> @@ -63,6 +63,7 @@
>>   #define		ATMEL_US_PAR_MARK		(3 <<  9)
>>   #define		ATMEL_US_PAR_NONE		(4 <<  9)
>>   #define		ATMEL_US_PAR_MULTI_DROP		(6 <<  9)
>> +#define ATMEL_US_GCLK                          BIT(12)
> 
> It seems there are spaces here.
> 
>>   #define	ATMEL_US_NBSTOP		GENMASK(13, 12)	/* Number of Stop Bits */
>>   #define		ATMEL_US_NBSTOP_1		(0 << 12)
>>   #define		ATMEL_US_NBSTOP_1_5		(1 << 12)
> 

Thanks,
	Sergiu
Claudiu Beznea Sept. 1, 2022, 6:36 a.m. UTC | #5
On 31.08.2022 14:32, Sergiu Moga - M68701 wrote:
> On 31.08.2022 12:46, Claudiu Beznea wrote:
>> On 17.08.2022 10:55, Sergiu Moga wrote:
>>> Previously, the atmel serial driver did not take into account the
>>> possibility of using the more customizable generic clock as its
>>> baudrate generator. Unless there is a Fractional Part available to
>>> increase accuracy, there is a high chance that we may be able to
>>> generate a baudrate closer to the desired one by using the GCLK as the
>>> clock source. Now, depending on the error rate between
>>> the desired baudrate and the actual baudrate, the serial driver will
>>> fallback on the generic clock. The generic clock must be provided
>>> in the DT node of the serial that may need a more flexible clock source.
>>>
>>> Signed-off-by: Sergiu Moga <sergiu.moga@microchip.com>
>>> ---
>>>   drivers/tty/serial/atmel_serial.c | 52 ++++++++++++++++++++++++++++++-
>>>   drivers/tty/serial/atmel_serial.h |  1 +
>>>   2 files changed, 52 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
>>> index 30ba9eef7b39..0a0b46ee0955 100644
>>> --- a/drivers/tty/serial/atmel_serial.c
>>> +++ b/drivers/tty/serial/atmel_serial.c
>>> @@ -15,6 +15,7 @@
>>>   #include <linux/init.h>
>>>   #include <linux/serial.h>
>>>   #include <linux/clk.h>
>>> +#include <linux/clk-provider.h>
>>>   #include <linux/console.h>
>>>   #include <linux/sysrq.h>
>>>   #include <linux/tty_flip.h>
>>> @@ -77,6 +78,8 @@ static void atmel_stop_rx(struct uart_port *port);
>>>   #endif
>>>   
>>>   #define ATMEL_ISR_PASS_LIMIT	256
>>> +#define ERROR_RATE(desired_value, actual_value) \
>>> +	((int)(100 - ((desired_value) * 100) / (actual_value)))
>>>   
>>>   struct atmel_dma_buffer {
>>>   	unsigned char	*buf;
>>> @@ -110,6 +113,7 @@ struct atmel_uart_char {
>>>   struct atmel_uart_port {
>>>   	struct uart_port	uart;		/* uart */
>>>   	struct clk		*clk;		/* uart clock */
>>> +	struct clk		*gclk;		/* uart generic clock */
>>>   	int			may_wakeup;	/* cached value of device_may_wakeup for times we need to disable it */
>>>   	u32			backup_imr;	/* IMR saved during suspend */
>>>   	int			break_active;	/* break being received */
>>> @@ -2115,6 +2119,8 @@ static void atmel_serial_pm(struct uart_port *port, unsigned int state,
>>>   		 * This is called on uart_close() or a suspend event.
>>>   		 */
>>>   		clk_disable_unprepare(atmel_port->clk);
>>> +		if (atmel_port->gclk && __clk_is_enabled(atmel_port->gclk))
>>
>> No need to check for atmel_port->gclk != NULL. clk APIs are already doing this.
>>
>>> +			clk_disable_unprepare(atmel_port->gclk);
>>>   		break;
>>>   	default:
>>>   		dev_err(port->dev, "atmel_serial: unknown pm %d\n", state);
>>> @@ -2129,7 +2135,8 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
>>>   {
>>>   	struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
>>>   	unsigned long flags;
>>> -	unsigned int old_mode, mode, imr, quot, baud, div, cd, fp = 0;
>>> +	unsigned int old_mode, mode, imr, quot, div, cd, fp = 0;
>>> +	unsigned int baud, actual_baud, gclk_rate;
>>>   
>>>   	/* save the current mode register */
>>>   	mode = old_mode = atmel_uart_readl(port, ATMEL_US_MR);
>>> @@ -2288,6 +2295,37 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
>>>   		cd /= 8;
>>>   		mode |= ATMEL_US_USCLKS_MCK_DIV8;
>>>   	}
>>> +
>>> +	/*
>>> +	 * If there is no Fractional Part, there is a high chance that
>>> +	 * we may be able to generate a baudrate closer to the desired one
>>> +	 * if we use the GCLK as the clock source driving the baudrate
>>> +	 * generator.
>>> +	 */
>>> +	if (!fp && atmel_port->gclk) {
>>> +		if (__clk_is_enabled(atmel_port->gclk))
>>> +			clk_disable_unprepare(atmel_port->gclk);
>>
>> You disabled it here, set new rate but re-enable it conditionally above, is
>> this intended? the below condition may fail.
>>
> 
> Yes, it is intended. Gclk should remain disabled if the below condition 
> fails.
> 
> 
>>> +		clk_set_rate(atmel_port->gclk, 16 * baud);
>>> +		gclk_rate = clk_get_rate(atmel_port->gclk);
>>
>> You should be able to use clk_round_rate() here:
>> 		gclk_rate = clk_round_rate(atmel_port->gclk,
>> 					   16 * baudrate);
>>
>> With this you can re-write all this block something like:
>>
>> 		gclk_rate = clk_round_rate(atmel_port->gclk,
>> 					   16 * baudrate);
>> 		actual_baud = gclk_rate / (16 * cd);
>> 		if (abs(ERROR_RATE(baud, actual_baud)) >
>> 		    abd(ERROR_RATE(baud, gclk_rate / 16))) {
>> 			mode |= ATMEL_US_GCLK;
>> 			cd = 1;
>> 			if (__clk_is_enabled(atmel_port->gclk))
>> 				clk_disable_unprepare(atmel_port->gclk)
>> 			clk_set_rate(atmel_port->gclk, gclk_rate);
>> 			clk_prepare_enable(atmel_port->gclk);
>> 		}
>>
>>
> 
> Hmm, yes, you are right, it is much better with this clk_round_rate() 
> since there would be no need to disable gclk before setting the new 
> rate, I did not know of this function at that point. However, in this 
> case, the if(abs...) will also need an else to disable the gclk in case 
> the condition failed and it was somehow enabled before.

Then you can keep the disable at the beginning of the block (as it
previously was) and use clk_round_rate() instead of:

		clk_set_rate();
		clk_get_rate();

And use clk_set_rate() only if the following is true:

+		if (abs(ERROR_RATE(baud, actual_baud)) >
+		    abs(ERROR_RATE(baud, gclk_rate / 16))) {

Having this, in case gclk cannot be used it is just disable w/o also having
the rate changed.

> 
> 
>>> +		actual_baud = clk_get_rate(atmel_port->clk) / (16 * cd);
>>> +		if (abs(ERROR_RATE(baud, actual_baud)) >
>>> +		    abs(ERROR_RATE(baud, gclk_rate / 16))) {
>>> +			mode |= ATMEL_US_GCLK;
>>> +
>>> +			/*
>>> +			 * Set the Clock Divisor for GCLK to 1.
>>> +			 * Since we were able to generate the smallest
>>> +			 * multiple of the desired baudrate times 16,
>>> +			 * then we surely can generate a bigger multiple
>>> +			 * with the exact error rate for an equally increased
>>> +			 * CD. Thus no need to take into account
>>> +			 * a higher value for CD.
>>> +			 */
>>> +			cd = 1;
>>> +			clk_prepare_enable(atmel_port->gclk);
>>> +		}
>>> +	}
>>> +
>>>   	quot = cd | fp << ATMEL_US_FP_OFFSET;
>>>   
>>>   	if (!(port->iso7816.flags & SER_ISO7816_ENABLED))
>>> @@ -2883,6 +2921,16 @@ static int atmel_serial_probe(struct platform_device *pdev)
>>>   	if (ret)
>>>   		goto err;
>>>   
>>> +	atmel_port->gclk = devm_clk_get_optional(&pdev->dev, "gclk");
>>> +	if (atmel_port->gclk) {
>>> +		ret = clk_prepare_enable(atmel_port->gclk);
>>> +		if (ret) {
>>> +			atmel_port->gclk = NULL;
>>> +			return ret;
>>> +		}
>>> +		clk_disable_unprepare(atmel_port->gclk);
>>
>> Is there a reason you enable then disable the clock here?
>>
> 
> 
> I think it's better to make sure in the probe method that enabling the 
> gclk issues no errors, so that the error does not appear in 
> set_termios(). Since the user must place the optional gclk in DT if they 
> want a finer rate when missing the Fractional Part, I think they should 
> know before even trying to open the port that it is first correctly 
> setup in the clock tree as well by making the probe method fail from the 
> very beginning in case it is not.

There is no guarantee that the disable/enable in set_termios() will not
fail. If you want to avoid failures you can just take into account the
returning code of clk_prepare_enable() in set_termios() and decide to use
generic clock for baud rate depending on this.

> 
> So, I first enable it and make sure there are no errors and then disable 
> it because I do not see the point of having an enabled clock that might 
> not be used.
> 
> 
>>> +	}
>>> +
>>>   	ret = atmel_init_port(atmel_port, pdev);
>>>   	if (ret)
>>>   		goto err_clk_disable_unprepare;
>>> @@ -2929,6 +2977,8 @@ static int atmel_serial_probe(struct platform_device *pdev)
>>>   	 * is used
>>>   	 */
>>>   	clk_disable_unprepare(atmel_port->clk);
>>> +	if (atmel_port->gclk && __clk_is_enabled(atmel_port->gclk))
>>> +		clk_disable_unprepare(atmel_port->gclk);
>>
>> Is this due to the enable in atmel_set_termios()? Is that called on probe
>> path? Also, there is no need to check for atmel_port->gclk as clk APIs are
>> already doing this.
>>
> 
> 
> No, I guess this is not really needed, since it is disabled once it is 
> claimed from DT if enabling it succeeds. I initially placed this code 
> sequence wherever the peripheral clock is disabled as well.
> 
>>>   
>>>   	return 0;
>>>   
>>> diff --git a/drivers/tty/serial/atmel_serial.h b/drivers/tty/serial/atmel_serial.h
>>> index 0d8a0f9cc5c3..fb718972f81a 100644
>>> --- a/drivers/tty/serial/atmel_serial.h
>>> +++ b/drivers/tty/serial/atmel_serial.h
>>> @@ -63,6 +63,7 @@
>>>   #define		ATMEL_US_PAR_MARK		(3 <<  9)
>>>   #define		ATMEL_US_PAR_NONE		(4 <<  9)
>>>   #define		ATMEL_US_PAR_MULTI_DROP		(6 <<  9)
>>> +#define ATMEL_US_GCLK                          BIT(12)
>>
>> It seems there are spaces here.
>>
>>>   #define	ATMEL_US_NBSTOP		GENMASK(13, 12)	/* Number of Stop Bits */
>>>   #define		ATMEL_US_NBSTOP_1		(0 << 12)
>>>   #define		ATMEL_US_NBSTOP_1_5		(1 << 12)
>>
> 
> Thanks,
> 	Sergiu
diff mbox series

Patch

diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
index 30ba9eef7b39..0a0b46ee0955 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -15,6 +15,7 @@ 
 #include <linux/init.h>
 #include <linux/serial.h>
 #include <linux/clk.h>
+#include <linux/clk-provider.h>
 #include <linux/console.h>
 #include <linux/sysrq.h>
 #include <linux/tty_flip.h>
@@ -77,6 +78,8 @@  static void atmel_stop_rx(struct uart_port *port);
 #endif
 
 #define ATMEL_ISR_PASS_LIMIT	256
+#define ERROR_RATE(desired_value, actual_value) \
+	((int)(100 - ((desired_value) * 100) / (actual_value)))
 
 struct atmel_dma_buffer {
 	unsigned char	*buf;
@@ -110,6 +113,7 @@  struct atmel_uart_char {
 struct atmel_uart_port {
 	struct uart_port	uart;		/* uart */
 	struct clk		*clk;		/* uart clock */
+	struct clk		*gclk;		/* uart generic clock */
 	int			may_wakeup;	/* cached value of device_may_wakeup for times we need to disable it */
 	u32			backup_imr;	/* IMR saved during suspend */
 	int			break_active;	/* break being received */
@@ -2115,6 +2119,8 @@  static void atmel_serial_pm(struct uart_port *port, unsigned int state,
 		 * This is called on uart_close() or a suspend event.
 		 */
 		clk_disable_unprepare(atmel_port->clk);
+		if (atmel_port->gclk && __clk_is_enabled(atmel_port->gclk))
+			clk_disable_unprepare(atmel_port->gclk);
 		break;
 	default:
 		dev_err(port->dev, "atmel_serial: unknown pm %d\n", state);
@@ -2129,7 +2135,8 @@  static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
 {
 	struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
 	unsigned long flags;
-	unsigned int old_mode, mode, imr, quot, baud, div, cd, fp = 0;
+	unsigned int old_mode, mode, imr, quot, div, cd, fp = 0;
+	unsigned int baud, actual_baud, gclk_rate;
 
 	/* save the current mode register */
 	mode = old_mode = atmel_uart_readl(port, ATMEL_US_MR);
@@ -2288,6 +2295,37 @@  static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
 		cd /= 8;
 		mode |= ATMEL_US_USCLKS_MCK_DIV8;
 	}
+
+	/*
+	 * If there is no Fractional Part, there is a high chance that
+	 * we may be able to generate a baudrate closer to the desired one
+	 * if we use the GCLK as the clock source driving the baudrate
+	 * generator.
+	 */
+	if (!fp && atmel_port->gclk) {
+		if (__clk_is_enabled(atmel_port->gclk))
+			clk_disable_unprepare(atmel_port->gclk);
+		clk_set_rate(atmel_port->gclk, 16 * baud);
+		gclk_rate = clk_get_rate(atmel_port->gclk);
+		actual_baud = clk_get_rate(atmel_port->clk) / (16 * cd);
+		if (abs(ERROR_RATE(baud, actual_baud)) >
+		    abs(ERROR_RATE(baud, gclk_rate / 16))) {
+			mode |= ATMEL_US_GCLK;
+
+			/*
+			 * Set the Clock Divisor for GCLK to 1.
+			 * Since we were able to generate the smallest
+			 * multiple of the desired baudrate times 16,
+			 * then we surely can generate a bigger multiple
+			 * with the exact error rate for an equally increased
+			 * CD. Thus no need to take into account
+			 * a higher value for CD.
+			 */
+			cd = 1;
+			clk_prepare_enable(atmel_port->gclk);
+		}
+	}
+
 	quot = cd | fp << ATMEL_US_FP_OFFSET;
 
 	if (!(port->iso7816.flags & SER_ISO7816_ENABLED))
@@ -2883,6 +2921,16 @@  static int atmel_serial_probe(struct platform_device *pdev)
 	if (ret)
 		goto err;
 
+	atmel_port->gclk = devm_clk_get_optional(&pdev->dev, "gclk");
+	if (atmel_port->gclk) {
+		ret = clk_prepare_enable(atmel_port->gclk);
+		if (ret) {
+			atmel_port->gclk = NULL;
+			return ret;
+		}
+		clk_disable_unprepare(atmel_port->gclk);
+	}
+
 	ret = atmel_init_port(atmel_port, pdev);
 	if (ret)
 		goto err_clk_disable_unprepare;
@@ -2929,6 +2977,8 @@  static int atmel_serial_probe(struct platform_device *pdev)
 	 * is used
 	 */
 	clk_disable_unprepare(atmel_port->clk);
+	if (atmel_port->gclk && __clk_is_enabled(atmel_port->gclk))
+		clk_disable_unprepare(atmel_port->gclk);
 
 	return 0;
 
diff --git a/drivers/tty/serial/atmel_serial.h b/drivers/tty/serial/atmel_serial.h
index 0d8a0f9cc5c3..fb718972f81a 100644
--- a/drivers/tty/serial/atmel_serial.h
+++ b/drivers/tty/serial/atmel_serial.h
@@ -63,6 +63,7 @@ 
 #define		ATMEL_US_PAR_MARK		(3 <<  9)
 #define		ATMEL_US_PAR_NONE		(4 <<  9)
 #define		ATMEL_US_PAR_MULTI_DROP		(6 <<  9)
+#define ATMEL_US_GCLK                          BIT(12)
 #define	ATMEL_US_NBSTOP		GENMASK(13, 12)	/* Number of Stop Bits */
 #define		ATMEL_US_NBSTOP_1		(0 << 12)
 #define		ATMEL_US_NBSTOP_1_5		(1 << 12)