| Message ID | 20220919150846.1148783-10-sergiu.moga@microchip.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v4,1/9] dt-bindings: mfd: atmel,sama5d2-flexcom: Add SPI child node ref binding | expand |
On 21.09.2022 08:51, Claudiu Beznea - M18063 wrote: > On 19.09.2022 18:08, 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> > > Reviewed-by: Claudiu Beznea <claudiu.beznea@microchip.com> Actually, I'm taking this back at the moment. > > >> --- >> >> >> v1 -> v2: >> - take into account the different placement of the baudrate clock source >> into the IP's Mode Register (USART vs UART) >> - don't check for atmel_port->gclk != NULL >> - use clk_round_rate instead of clk_set_rate + clk_get_rate >> - remove clk_disable_unprepare from the end of the probe method >> >> >> >> v2 -> v3: >> - add the error rate calculation function as an inline function instead of >> a macro definition >> - add `gclk_fail` goto >> - replace `goto err` with `goto err_clk_disable_unprepare;` >> >> >> >> v3 -> v4: >> - Nothing, this was previously [PATCH 14] >> >> >> drivers/tty/serial/atmel_serial.c | 59 ++++++++++++++++++++++++++++++- >> 1 file changed, 58 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c >> index c983798a4ab2..426f9d4f9a5a 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> >> @@ -110,6 +111,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 */ >> @@ -229,6 +231,11 @@ static inline int atmel_uart_is_half_duplex(struct uart_port *port) >> (port->iso7816.flags & SER_ISO7816_ENABLED); >> } >> >> +static inline int atmel_error_rate(int desired_value, int actual_value) >> +{ >> + return 100 - (desired_value * 100) / actual_value; >> +} >> + >> #ifdef CONFIG_SERIAL_ATMEL_PDC >> static bool atmel_use_pdc_rx(struct uart_port *port) >> { >> @@ -2117,6 +2124,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 (__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); >> @@ -2132,7 +2141,9 @@ static void atmel_set_termios(struct uart_port *port, >> { >> 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; >> + int ret; >> >> /* save the current mode register */ >> mode = old_mode = atmel_uart_readl(port, ATMEL_US_MR); >> @@ -2302,6 +2313,46 @@ static void atmel_set_termios(struct uart_port *port, >> cd = min_t(unsigned int, cd, ATMEL_US_CD); >> } >> >> + /* >> + * 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 (!atmel_port->has_frac_baudrate) { >> + if (__clk_is_enabled(atmel_port->gclk)) >> + clk_disable_unprepare(atmel_port->gclk); >> + gclk_rate = clk_round_rate(atmel_port->gclk, 16 * baud); >> + actual_baud = clk_get_rate(atmel_port->clk) / (16 * cd); >> + if (gclk_rate && abs(atmel_error_rate(baud, actual_baud)) > >> + abs(atmel_error_rate(baud, gclk_rate / 16))) { If this condition fails and you previously used GCLK for clock generation you should remove bits ATMEL_US_USCLKS or ATMEL_UA_BRSRCCK from mode variable, to avoid configuring MR with GCLK support and clock not being properly setup. >> + clk_set_rate(atmel_port->gclk, 16 * baud); >> + ret = clk_prepare_enable(atmel_port->gclk); >> + if (ret) >> + goto gclk_fail; >> + >> + if (atmel_port->is_usart) { >> + mode &= ~ATMEL_US_USCLKS; >> + mode |= ATMEL_US_USCLKS_GCLK; >> + } else { >> + mode &= ~ATMEL_UA_BRSRCCK; >> + mode |= ATMEL_UA_BRSRCCK_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; >> + } >> + } >> + >> +gclk_fail: >> quot = cd | fp << ATMEL_US_FP_OFFSET; >> >> if (!(port->iso7816.flags & SER_ISO7816_ENABLED)) >> @@ -2897,6 +2948,12 @@ static int atmel_serial_probe(struct platform_device *pdev) >> if (ret) >> goto err; >> >> + atmel_port->gclk = devm_clk_get_optional(&pdev->dev, "gclk"); >> + if (IS_ERR(atmel_port->gclk)) { >> + ret = PTR_ERR(atmel_port->gclk); >> + goto err_clk_disable_unprepare; >> + } >> + >> ret = atmel_init_port(atmel_port, pdev); >> if (ret) >> goto err_clk_disable_unprepare; >
On 21.09.2022 09:27, Claudiu Beznea - M18063 wrote: > On 21.09.2022 08:51, Claudiu Beznea - M18063 wrote: >> On 19.09.2022 18:08, 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> >> >> Reviewed-by: Claudiu Beznea <claudiu.beznea@microchip.com> > > Actually, I'm taking this back at the moment. > >> >> >>> --- >>> >>> >>> v1 -> v2: >>> - take into account the different placement of the baudrate clock source >>> into the IP's Mode Register (USART vs UART) >>> - don't check for atmel_port->gclk != NULL >>> - use clk_round_rate instead of clk_set_rate + clk_get_rate >>> - remove clk_disable_unprepare from the end of the probe method >>> >>> >>> >>> v2 -> v3: >>> - add the error rate calculation function as an inline function instead of >>> a macro definition >>> - add `gclk_fail` goto >>> - replace `goto err` with `goto err_clk_disable_unprepare;` >>> >>> >>> >>> v3 -> v4: >>> - Nothing, this was previously [PATCH 14] >>> >>> >>> drivers/tty/serial/atmel_serial.c | 59 ++++++++++++++++++++++++++++++- >>> 1 file changed, 58 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c >>> index c983798a4ab2..426f9d4f9a5a 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> >>> @@ -110,6 +111,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 */ >>> @@ -229,6 +231,11 @@ static inline int atmel_uart_is_half_duplex(struct uart_port *port) >>> (port->iso7816.flags & SER_ISO7816_ENABLED); >>> } >>> >>> +static inline int atmel_error_rate(int desired_value, int actual_value) >>> +{ >>> + return 100 - (desired_value * 100) / actual_value; >>> +} >>> + >>> #ifdef CONFIG_SERIAL_ATMEL_PDC >>> static bool atmel_use_pdc_rx(struct uart_port *port) >>> { >>> @@ -2117,6 +2124,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 (__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); >>> @@ -2132,7 +2141,9 @@ static void atmel_set_termios(struct uart_port *port, >>> { >>> 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; >>> + int ret; >>> >>> /* save the current mode register */ >>> mode = old_mode = atmel_uart_readl(port, ATMEL_US_MR); >>> @@ -2302,6 +2313,46 @@ static void atmel_set_termios(struct uart_port *port, >>> cd = min_t(unsigned int, cd, ATMEL_US_CD); >>> } >>> >>> + /* >>> + * 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 (!atmel_port->has_frac_baudrate) { >>> + if (__clk_is_enabled(atmel_port->gclk)) >>> + clk_disable_unprepare(atmel_port->gclk); >>> + gclk_rate = clk_round_rate(atmel_port->gclk, 16 * baud); >>> + actual_baud = clk_get_rate(atmel_port->clk) / (16 * cd); >>> + if (gclk_rate && abs(atmel_error_rate(baud, actual_baud)) > >>> + abs(atmel_error_rate(baud, gclk_rate / 16))) { > > If this condition fails and you previously used GCLK for clock generation > you should remove bits ATMEL_US_USCLKS or ATMEL_UA_BRSRCCK from mode > variable, to avoid configuring MR with GCLK support and clock not being > properly setup. > This will not happen, since the code block placed at the very start of atmel_set_termios() /* reset the mode, clock divisor, parity, stop bits and data size */ mode &= ~(ATMEL_US_USCLKS | ATMEL_US_CHRL | ATMEL_US_NBSTOP | ATMEL_US_PAR | ATMEL_US_USMODE); will still clear the ATMEL_UA_BRSRCCK bitfield since it is the same as the ATMEL_US_NBSTOP bitfield. It is ugly and it has been working so far just through the coincidence of the bitfields placement. I did not want to change those lines as I did not really think of them as worth being part of this patch series (PATCH 8 of this series is an exception as it does introduce the needed is_usart boolean). I will send a patch regarding this, but separately since it is more like a pure cleanup related PATCH, in my opinion. >>> + clk_set_rate(atmel_port->gclk, 16 * baud); >>> + ret = clk_prepare_enable(atmel_port->gclk); >>> + if (ret) >>> + goto gclk_fail; >>> + >>> + if (atmel_port->is_usart) { >>> + mode &= ~ATMEL_US_USCLKS; >>> + mode |= ATMEL_US_USCLKS_GCLK; >>> + } else { >>> + mode &= ~ATMEL_UA_BRSRCCK; >>> + mode |= ATMEL_UA_BRSRCCK_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; >>> + } >>> + } >>> + >>> +gclk_fail: >>> quot = cd | fp << ATMEL_US_FP_OFFSET; >>> >>> if (!(port->iso7816.flags & SER_ISO7816_ENABLED)) >>> @@ -2897,6 +2948,12 @@ static int atmel_serial_probe(struct platform_device *pdev) >>> if (ret) >>> goto err; >>> >>> + atmel_port->gclk = devm_clk_get_optional(&pdev->dev, "gclk"); >>> + if (IS_ERR(atmel_port->gclk)) { >>> + ret = PTR_ERR(atmel_port->gclk); >>> + goto err_clk_disable_unprepare; >>> + } >>> + >>> ret = atmel_init_port(atmel_port, pdev); >>> if (ret) >>> goto err_clk_disable_unprepare; >> >
diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c index c983798a4ab2..426f9d4f9a5a 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> @@ -110,6 +111,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 */ @@ -229,6 +231,11 @@ static inline int atmel_uart_is_half_duplex(struct uart_port *port) (port->iso7816.flags & SER_ISO7816_ENABLED); } +static inline int atmel_error_rate(int desired_value, int actual_value) +{ + return 100 - (desired_value * 100) / actual_value; +} + #ifdef CONFIG_SERIAL_ATMEL_PDC static bool atmel_use_pdc_rx(struct uart_port *port) { @@ -2117,6 +2124,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 (__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); @@ -2132,7 +2141,9 @@ static void atmel_set_termios(struct uart_port *port, { 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; + int ret; /* save the current mode register */ mode = old_mode = atmel_uart_readl(port, ATMEL_US_MR); @@ -2302,6 +2313,46 @@ static void atmel_set_termios(struct uart_port *port, cd = min_t(unsigned int, cd, ATMEL_US_CD); } + /* + * 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 (!atmel_port->has_frac_baudrate) { + if (__clk_is_enabled(atmel_port->gclk)) + clk_disable_unprepare(atmel_port->gclk); + gclk_rate = clk_round_rate(atmel_port->gclk, 16 * baud); + actual_baud = clk_get_rate(atmel_port->clk) / (16 * cd); + if (gclk_rate && abs(atmel_error_rate(baud, actual_baud)) > + abs(atmel_error_rate(baud, gclk_rate / 16))) { + clk_set_rate(atmel_port->gclk, 16 * baud); + ret = clk_prepare_enable(atmel_port->gclk); + if (ret) + goto gclk_fail; + + if (atmel_port->is_usart) { + mode &= ~ATMEL_US_USCLKS; + mode |= ATMEL_US_USCLKS_GCLK; + } else { + mode &= ~ATMEL_UA_BRSRCCK; + mode |= ATMEL_UA_BRSRCCK_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; + } + } + +gclk_fail: quot = cd | fp << ATMEL_US_FP_OFFSET; if (!(port->iso7816.flags & SER_ISO7816_ENABLED)) @@ -2897,6 +2948,12 @@ static int atmel_serial_probe(struct platform_device *pdev) if (ret) goto err; + atmel_port->gclk = devm_clk_get_optional(&pdev->dev, "gclk"); + if (IS_ERR(atmel_port->gclk)) { + ret = PTR_ERR(atmel_port->gclk); + goto err_clk_disable_unprepare; + } + ret = atmel_init_port(atmel_port, pdev); if (ret) goto err_clk_disable_unprepare;
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> --- v1 -> v2: - take into account the different placement of the baudrate clock source into the IP's Mode Register (USART vs UART) - don't check for atmel_port->gclk != NULL - use clk_round_rate instead of clk_set_rate + clk_get_rate - remove clk_disable_unprepare from the end of the probe method v2 -> v3: - add the error rate calculation function as an inline function instead of a macro definition - add `gclk_fail` goto - replace `goto err` with `goto err_clk_disable_unprepare;` v3 -> v4: - Nothing, this was previously [PATCH 14] drivers/tty/serial/atmel_serial.c | 59 ++++++++++++++++++++++++++++++- 1 file changed, 58 insertions(+), 1 deletion(-)