| Message ID | 1647934315-5189-1-git-send-email-quic_vnivarth@quicinc.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | drivers/tty/serial/qcom-geni-serial: Remove uart frequency table. Instead, find suitable frequency with call to clk_round_rate. | expand |
Quoting Vijaya Krishna Nivarthi (2022-03-22 00:31:55) > [Why] > This change is part of resolving feedback for an earlier > patch. The UART frequency table is to be replaced with a The first sentence is useless. Just say what you're doing and why you're doing it. Replace the UART frequency table 'root_freq[]' with logic around clk_round_rate() so that SoC details like the available clk frequencies can change and this driver still works. This reduces tight coupling between this UART driver and the SoC clk driver because we no longer have to update the 'root_freq[]' array for new SoCs. Instead the driver determines the available frequencies at runtime. > call to clk_round_rate so it would work regardless of > what the clk driver supports for the particular SoC. > > [How] > Try to find a frequency and divider that exactly matches > the required rate. If not found, return the closest > possible frequency and set divider to 1. > > Signed-off-by: Vijaya Krishna Nivarthi <quic_vnivarth@quicinc.com> > --- > drivers/tty/serial/qcom_geni_serial.c | 57 ++++++++++++++++++++--------------- > 1 file changed, 33 insertions(+), 24 deletions(-) > > diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c > index aedc388..5226673 100644 > --- a/drivers/tty/serial/qcom_geni_serial.c > +++ b/drivers/tty/serial/qcom_geni_serial.c > @@ -149,12 +149,6 @@ static unsigned int qcom_geni_serial_tx_empty(struct uart_port *port); > static void qcom_geni_serial_stop_rx(struct uart_port *uport); > static void qcom_geni_serial_handle_rx(struct uart_port *uport, bool drop); > > -static const unsigned long root_freq[] = {7372800, 14745600, 19200000, 29491200, > - 32000000, 48000000, 51200000, 64000000, > - 80000000, 96000000, 100000000, > - 102400000, 112000000, 120000000, > - 128000000}; > - > #define to_dev_port(ptr, member) \ > container_of(ptr, struct qcom_geni_serial_port, member) > > @@ -946,32 +940,46 @@ static int qcom_geni_serial_startup(struct uart_port *uport) > return 0; > } > > -static unsigned long get_clk_cfg(unsigned long clk_freq) > -{ > - int i; > - > - for (i = 0; i < ARRAY_SIZE(root_freq); i++) { > - if (!(root_freq[i] % clk_freq)) > - return root_freq[i]; > - } > - return 0; > -} > - > -static unsigned long get_clk_div_rate(unsigned int baud, > +static unsigned long get_clk_div_rate(struct clk *clk, unsigned int baud, > unsigned int sampling_rate, unsigned int *clk_div) > { > unsigned long ser_clk; > unsigned long desired_clk; > + unsigned long freq, prev, freq_first; > + > + if (!clk) { Please remove this nonsense check. When is clk going to be NULL? > + pr_err("%s: Invalid clock handle\n", __func__); > + return 0; > + } > > desired_clk = baud * sampling_rate; > - ser_clk = get_clk_cfg(desired_clk); > - if (!ser_clk) { > - pr_err("%s: Can't find matching DFS entry for baud %d\n", > - __func__, baud); > - return ser_clk; > + if (!desired_clk) { > + pr_err("%s: Invalid frequency\n", __func__); > + return 0; > } > > + freq_first = 0; > + prev = desired_clk; > + freq = desired_clk - 1; > + do { > + if (freq != (desired_clk - 1)) Does this ever happen after the first loop? Why not assign prev to freq before entering? > + prev = freq; > + > + freq = clk_round_rate(clk, (freq + 1)); Remove useless parenthesis. > + > + if (!freq_first) > + freq_first = freq; > + } while ((freq % desired_clk) && (freq > 0) && (freq != prev)); Remove useless parenthesis. In general, take a look at clk_divider_bestdiv() for how to handle this. This device only has so many possible divider values so it's better to loop through clk_round_rate() taking into account the possible divider values instead of adding 1 to the frequency returned from clk_round_rate(). According to the defines #define CLK_DIV_MSK GENMASK(15, 4) #define CLK_DIV_SHFT 4 it has 4095 divider values (I guess a divider of 0 is the same as divider of 1?). We should be able to multiply the desired rate by the divider and see if it matches the frequency of the 'ser_clk'. If it doesn't we can calculate the actual frequency that we can achieve and then do our own rounding here to figure out if it is acceptable. I guess it needs to be exactly divisible, so if the clk_round_rate() doesn't come back with the same frequency we wanted we keep trying bigger numbers with bigger dividers. The important thing is that we don't exceed the possible divider values by returning from this function with an invalid divider. I was thinking it may be easier to implement this divider as a clk in the clk tree but that looks complicated. Largely that's because the 'ser_clk' is set with dev_pm_opp_set_rate() and that would be the parent of this new divider clk. If OPP core could figure out that the parent clk is changing and that's the one with the voltage requirements it would work to call dev_pm_opp_set_rate() on the new divider clk (maybe call it baud_clk). This logic doesn't exist today though so implementing it here in this driver is OK with me for now. > + > + if (!(freq % desired_clk)) Flip these conditions to remove one more set of parenthesis. > + ser_clk = freq; > + else > + ser_clk = freq_first; > + > *clk_div = ser_clk / desired_clk; > + if ((ser_clk) && (!(*clk_div))) Remove useless parenthesis. > + *clk_div = 1; > + > return ser_clk; > } > > @@ -1003,7 +1011,8 @@ static void qcom_geni_serial_set_termios(struct uart_port *uport, > if (ver >= QUP_SE_VERSION_2_5) > sampling_rate /= 2; > > - clk_rate = get_clk_div_rate(baud, sampling_rate, &clk_div); > + clk_rate = get_clk_div_rate((port->se).clk, baud, Remove useless parenthesis. > + sampling_rate, &clk_div); > if (!clk_rate) > goto out_restart_rx;
diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c index aedc388..5226673 100644 --- a/drivers/tty/serial/qcom_geni_serial.c +++ b/drivers/tty/serial/qcom_geni_serial.c @@ -149,12 +149,6 @@ static unsigned int qcom_geni_serial_tx_empty(struct uart_port *port); static void qcom_geni_serial_stop_rx(struct uart_port *uport); static void qcom_geni_serial_handle_rx(struct uart_port *uport, bool drop); -static const unsigned long root_freq[] = {7372800, 14745600, 19200000, 29491200, - 32000000, 48000000, 51200000, 64000000, - 80000000, 96000000, 100000000, - 102400000, 112000000, 120000000, - 128000000}; - #define to_dev_port(ptr, member) \ container_of(ptr, struct qcom_geni_serial_port, member) @@ -946,32 +940,46 @@ static int qcom_geni_serial_startup(struct uart_port *uport) return 0; } -static unsigned long get_clk_cfg(unsigned long clk_freq) -{ - int i; - - for (i = 0; i < ARRAY_SIZE(root_freq); i++) { - if (!(root_freq[i] % clk_freq)) - return root_freq[i]; - } - return 0; -} - -static unsigned long get_clk_div_rate(unsigned int baud, +static unsigned long get_clk_div_rate(struct clk *clk, unsigned int baud, unsigned int sampling_rate, unsigned int *clk_div) { unsigned long ser_clk; unsigned long desired_clk; + unsigned long freq, prev, freq_first; + + if (!clk) { + pr_err("%s: Invalid clock handle\n", __func__); + return 0; + } desired_clk = baud * sampling_rate; - ser_clk = get_clk_cfg(desired_clk); - if (!ser_clk) { - pr_err("%s: Can't find matching DFS entry for baud %d\n", - __func__, baud); - return ser_clk; + if (!desired_clk) { + pr_err("%s: Invalid frequency\n", __func__); + return 0; } + freq_first = 0; + prev = desired_clk; + freq = desired_clk - 1; + do { + if (freq != (desired_clk - 1)) + prev = freq; + + freq = clk_round_rate(clk, (freq + 1)); + + if (!freq_first) + freq_first = freq; + } while ((freq % desired_clk) && (freq > 0) && (freq != prev)); + + if (!(freq % desired_clk)) + ser_clk = freq; + else + ser_clk = freq_first; + *clk_div = ser_clk / desired_clk; + if ((ser_clk) && (!(*clk_div))) + *clk_div = 1; + return ser_clk; } @@ -1003,7 +1011,8 @@ static void qcom_geni_serial_set_termios(struct uart_port *uport, if (ver >= QUP_SE_VERSION_2_5) sampling_rate /= 2; - clk_rate = get_clk_div_rate(baud, sampling_rate, &clk_div); + clk_rate = get_clk_div_rate((port->se).clk, baud, + sampling_rate, &clk_div); if (!clk_rate) goto out_restart_rx;
[Why] This change is part of resolving feedback for an earlier patch. The UART frequency table is to be replaced with a call to clk_round_rate so it would work regardless of what the clk driver supports for the particular SoC. [How] Try to find a frequency and divider that exactly matches the required rate. If not found, return the closest possible frequency and set divider to 1. Signed-off-by: Vijaya Krishna Nivarthi <quic_vnivarth@quicinc.com> --- drivers/tty/serial/qcom_geni_serial.c | 57 ++++++++++++++++++++--------------- 1 file changed, 33 insertions(+), 24 deletions(-)