| Message ID | 20220906135511.144725-13-sergiu.moga@microchip.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Make atmel serial driver aware of GCLK | expand |
On Tue, 6 Sep 2022, 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> > --- > > > > 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 > > > > drivers/tty/serial/atmel_serial.c | 52 ++++++++++++++++++++++++++++++- > 1 file changed, 51 insertions(+), 1 deletion(-) > > diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c > index 6aa01ca5489c..b2b6fd6ea2a5 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))) Make a function instead. Is percent accurate enough or would you perhaps want something slightly more accurate? Given you've abs() at the caller side, the error rate could be underestimated, is underestimating OK?
On 07.09.2022 12:36, Ilpo Järvinen wrote: > On Tue, 6 Sep 2022, 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> >> --- >> >> >> >> 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 >> >> >> >> drivers/tty/serial/atmel_serial.c | 52 ++++++++++++++++++++++++++++++- >> 1 file changed, 51 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c >> index 6aa01ca5489c..b2b6fd6ea2a5 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))) > > Make a function instead. > Alright, I see the point of making it an inline function instead. > Is percent accurate enough or would you perhaps want something slightly > more accurate? > It is accurate enough for the all the baudrates I have tested. It usually taps into the GCLK whenever high baudrates such as 921600 are used. For 115200 for example, the error rate was slightly better in the case of the peripheral clock and it acted accordingly, choosing the latter as its baudrate source clock. I do not think that a higher accuracy than this would be needed though. Say that using percent accuracy yields that the error rates are equal, but the gclk would have been better in this case by, say, a few 10 ^ -4, but the code logic does not see it so it proceeds using the peripheral clock. In that case, the error rate of the peripheral clock would still be low enough relative to the desired baudrate for the communication to function properly. The higher the baudrate, the lower the error rate must be in order for things to go smoothly. For example, for a baudrate of 57600 I noticed that even an error rate as big as 6% is still enough for the communication to work properly, while in the case of 921600 anything bigger than 2% and things do not go smoothly anymore. So I guess that it would be safe to say that, unless you go for baudrates as high as tens of millions, things should work well with just percent accuracy. A higher accuracy always definetely helps, but I believe it is not needed in this case. > Given you've abs() at the caller side, the error rate could be > underestimated, is underestimating OK? > Yes, this should be fine. While (both empirically and after looking stuff up) I noticed that in the case of negative error rates, their absolute value needs to be smaller than the one of positive error rates, it must be so by a very small margin that is negligible when estimating through percent accuracy. > -- > i. > >> 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 */ >> @@ -2117,6 +2121,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); >> @@ -2131,7 +2137,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); >> @@ -2297,6 +2304,43 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios, >> cd &= 65535; >> } >> >> + /* >> + * 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(ERROR_RATE(baud, actual_baud)) > >> + abs(ERROR_RATE(baud, gclk_rate / 16))) { >> + clk_set_rate(atmel_port->gclk, 16 * baud); >> + if (!clk_prepare_enable(atmel_port->gclk)) { >> + 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; >> + } >> + } >> + } >> + >> quot = cd | fp << ATMEL_US_FP_OFFSET; >> >> if (!(port->iso7816.flags & SER_ISO7816_ENABLED)) >> @@ -2892,6 +2936,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; >> + } >> + >> ret = atmel_init_port(atmel_port, pdev); >> if (ret) >> goto err_clk_disable_unprepare; >>
On Wed, 7 Sep 2022, Sergiu.Moga@microchip.com wrote: > On 07.09.2022 12:36, Ilpo Järvinen wrote: > > On Tue, 6 Sep 2022, 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> > >> --- > > Is percent accurate enough or would you perhaps want something slightly > > more accurate? > > > > > It is accurate enough for the all the baudrates I have tested. It > usually taps into the GCLK whenever high baudrates such as 921600 are > used. For 115200 for example, the error rate was slightly better in the > case of the peripheral clock and it acted accordingly, choosing the > latter as its baudrate source clock. I do not think that a higher > accuracy than this would be needed though. Say that using percent > accuracy yields that the error rates are equal, but the gclk would have > been better in this case by, say, a few 10 ^ -4, but the code logic does > not see it so it proceeds using the peripheral clock. In that case, the > error rate of the peripheral clock would still be low enough relative to > the desired baudrate for the communication to function properly. > > The higher the baudrate, the lower the error rate must be in order for > things to go smoothly. For example, for a baudrate of 57600 I noticed > that even an error rate as big as 6% is still enough for the > communication to work properly, while in the case of 921600 anything > bigger than 2% and things do not go smoothly anymore. So I guess that it > would be safe to say that, unless you go for baudrates as high as tens > of millions, things should work well with just percent accuracy. A > higher accuracy always definetely helps, but I believe it is not needed > in this case. > > > > Given you've abs() at the caller side, the error rate could be > > underestimated, is underestimating OK? > > > > > Yes, this should be fine. While (both empirically and after looking > stuff up) I noticed that in the case of negative error rates, their > absolute value needs to be smaller than the one of positive error rates, > it must be so by a very small margin that is negligible when estimating > through percent accuracy. OK. Thanks for checking.
On 06.09.2022 16: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> > --- > > > > 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 > > > > drivers/tty/serial/atmel_serial.c | 52 ++++++++++++++++++++++++++++++- > 1 file changed, 51 insertions(+), 1 deletion(-) > > diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c > index 6aa01ca5489c..b2b6fd6ea2a5 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 */ > @@ -2117,6 +2121,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); > @@ -2131,7 +2137,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); > @@ -2297,6 +2304,43 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios, > cd &= 65535; > } > > + /* > + * 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(ERROR_RATE(baud, actual_baud)) > > + abs(ERROR_RATE(baud, gclk_rate / 16))) { > + clk_set_rate(atmel_port->gclk, 16 * baud); This is a personal taste: I would do like this: ret = clk_prepare_enable(); if (ret) goto clk_prepare_failure; // and here the rest of the code... > + if (!clk_prepare_enable(atmel_port->gclk)) { > + 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; > + } > + } > + } > + clk_prepare_failure: // or other label name > quot = cd | fp << ATMEL_US_FP_OFFSET; > > if (!(port->iso7816.flags & SER_ISO7816_ENABLED)) > @@ -2892,6 +2936,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; This should be: goto err_clk_disable_unprepare; to disable the peripheral clock previously enabled. > + } > + > 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 6aa01ca5489c..b2b6fd6ea2a5 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 */ @@ -2117,6 +2121,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); @@ -2131,7 +2137,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); @@ -2297,6 +2304,43 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios, cd &= 65535; } + /* + * 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(ERROR_RATE(baud, actual_baud)) > + abs(ERROR_RATE(baud, gclk_rate / 16))) { + clk_set_rate(atmel_port->gclk, 16 * baud); + if (!clk_prepare_enable(atmel_port->gclk)) { + 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; + } + } + } + quot = cd | fp << ATMEL_US_FP_OFFSET; if (!(port->iso7816.flags & SER_ISO7816_ENABLED)) @@ -2892,6 +2936,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; + } + 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 drivers/tty/serial/atmel_serial.c | 52 ++++++++++++++++++++++++++++++- 1 file changed, 51 insertions(+), 1 deletion(-)