Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <1652697510-30543-1-git-send-email-quic_vnivarth@quicinc.com>
In-Reply-To: <1652697510-30543-1-git-send-email-quic_vnivarth@quicinc.com>
From:   Doug Anderson <dianders@chromium.org>
Date:   Thu, 26 May 2022 10:07:37 -0700
Message-ID: <CAD=FV=WZQLpYAsdU4yTCGZCGp5+t+rJiadbSeppBaX0b6ZuUxg@mail.gmail.com>
Subject: Re: [V2] tty: serial: qcom-geni-serial: Remove uart frequency table.
 Instead, find suitable frequency with call to clk_round_rate.
To:     Vijaya Krishna Nivarthi <quic_vnivarth@quicinc.com>
Cc:     Andy Gross <agross@kernel.org>,
        Bjorn Andersson <bjorn.andersson@linaro.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Jiri Slaby <jirislaby@kernel.org>,
        linux-arm-msm <linux-arm-msm@vger.kernel.org>,
        linux-serial@vger.kernel.org, LKML <linux-kernel@vger.kernel.org>,
        quic_msavaliy@quicinc.com, Matthias Kaehlcke <mka@chromium.org>,
        Stephen Boyd <swboyd@chromium.org>,
        Satya Priya <quic_c_skakit@quicinc.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi,

On Mon, May 16, 2022 at 3:38 AM Vijaya Krishna Nivarthi
<quic_vnivarth@quicinc.com> wrote:
>
> 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.
>
> Signed-off-by: Vijaya Krishna Nivarthi <quic_vnivarth@quicinc.com>
> ---
> v2: loops through clk dividers to zero-in quickly
> v1: intial patch looped through available clk frequencies
> ---
>  drivers/tty/serial/qcom_geni_serial.c | 56 ++++++++++++++++++++++-------------
>  1 file changed, 36 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> index f496102..4733a23 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,25 +940,43 @@ 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;
> +       unsigned long div, maxdiv;
> +       int64_t mult;

Why is "mult" signed? Shouldn't it be type "u64" or something?

>
>         desired_clk = baud * sampling_rate;
> -       ser_clk = get_clk_cfg(desired_clk);
> +       if (!desired_clk) {
> +               pr_err("%s: Invalid frequency\n", __func__);

nit: IMO printing the __func__ in every printout is not a good
practice. It uglifies the kernel log buffer. If you personally truly
want the function in every printout then modify pr_err() to print it.


> +               return 0;
> +       }
> +
> +       maxdiv = CLK_DIV_MSK >> CLK_DIV_SHFT;
> +       prev = 0;
> +
> +       for (div = 1; div <= maxdiv; div++) {
> +               mult = div * desired_clk;
> +               if (mult > ULONG_MAX)
> +                       break;

I'm pretty sure your "if" test is always false because you didn't cast
properly. I even tested it for you:

{
    unsigned long a, b;
    unsigned long long c;

    printf("long size: %d, long long size: %d\n",
           (int)(sizeof(a)), (int)(sizeof(c)));

    a = 0xffffffff;
    b = 2;

    c = a * b;
    printf("c is %#llx\n", c);

    c = a * (unsigned long long)b;
    printf("c is %#llx\n", c);
}

That prints out:

long size: 4, long long size: 8
c is 0xfffffffe
c is 0x1fffffffe


> +
> +               freq = clk_round_rate(clk, (unsigned long)mult);
> +               if (!(freq % desired_clk)) {
> +                       ser_clk = freq;
> +                       break;
> +               }
> +
> +               if (!prev)
> +                       ser_clk = freq;

Instead of the above, why not just init ser_clk to "desired_clk"?
...or perhaps leave it initted to 0 so your error check after the loop
isn't dead code?


> +               else if (prev == freq)
> +                       break;

Are you sure about this exit condition? It seems wrong. I guess you're
assuming that clk_round_rate() will round up like it (almost always)
does for the Qualcomm clock driver. So I guess let's say we're trying
to make 10000 baud and your oversampling is 16. So "desired_clk" is
160000, right? Now let's imagine that the clock driver can make three
rates:

7372800, 14745600, 19200000

Your loop will run. The first time through we'll "round" 160000 and
get back 7372800. It's not a match. Prev will now be 7372800.

The second time through, we'll round (160000 * 2) and get back
7372800. It's not a match and prev will be equal to freq so we'll
break.

...but we _should_ have found 19200000

So I think this break condition is wrong.


> +
> +               prev = freq;
> +       }
> +
>         if (!ser_clk) {
>                 pr_err("%s: Can't find matching DFS entry for baud %d\n",
>                                                                 __func__, baud);

In the above, you _always_ init "ser_clk" to something, so how can
this error condition ever occur in your new code?


> @@ -972,6 +984,9 @@ static unsigned long get_clk_div_rate(unsigned int baud,
>         }
>
>         *clk_div = ser_clk / desired_clk;
> +       if (!(*clk_div))
> +               *clk_div = 1;

I _think_ this can be removed if you just don't allow inexact matches.
...if you do allow inexact matches, maybe you should put a warning in
the logs in that case?


> +
>         return ser_clk;
>  }
>
> @@ -1003,7 +1018,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);

IMO it would look better to just let the above line be 81 columns
rather than the ugly wrapping.