Hi,
On Fri, Jun 3, 2022 at 10:43 AM Vijaya Krishna Nivarthi
<quic_vnivarth@xxxxxxxxxxx> wrote:
>
> Hi,
>
>
> On 6/1/2022 9:03 PM, Doug Anderson wrote:
> > Hi,
> >
> > On Wed, Jun 1, 2022 at 3:46 AM Vijaya Krishna Nivarthi
> > <quic_vnivarth@xxxxxxxxxxx> wrote:
> >> Hi,
> >>
> >> On 6/1/2022 12:58 AM, Doug Anderson wrote:
> >>> Hi,
> >>>
> >>> On Tue, May 31, 2022 at 11:18 AM Vijaya Krishna Nivarthi
> >>> <quic_vnivarth@xxxxxxxxxxx> wrote:
> >>>> Add missing initialisation and correct type casting
> >>>>
> >>>> Signed-off-by: Vijaya Krishna Nivarthi <quic_vnivarth@xxxxxxxxxxx>
> >>>> ---
> >>>> drivers/tty/serial/qcom_geni_serial.c | 8 ++++----
> >>>> 1 file changed, 4 insertions(+), 4 deletions(-)
> >>>>
> >>>> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> >>>> index 4733a23..08f3ad4 100644
> >>>> --- a/drivers/tty/serial/qcom_geni_serial.c
> >>>> +++ b/drivers/tty/serial/qcom_geni_serial.c
> >>>> @@ -943,11 +943,11 @@ static int qcom_geni_serial_startup(struct uart_port *uport)
> >>>> 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 ser_clk = 0;
> >>> In this patch it's not at all obvious why you'd need to init to 0. I
> >>> think the "for loop" is guaranteed to run at least once because
> >>> "max_div" is known at compile time. ...and currently each time through
> >>> the "for" loop you'll always set "ser_clk".
> >> Ok, I realised we will never break out of for loop exceeding ULONG_MAX
> >> in 1st pass, so yes ser_clk will always be set.
> >>
> >>> I think in a future patch you'll want to _remove_ this from the for loop:
> >>>
> >>> if (!prev)
> >>> ser_clk = freq;
> >> Intent is to save (and use) 1st freq if we cannot find an exact divider.
> >>
> >> Isn't it ok?
> >>
> >> For example please find debug output for a required frequency of 51.2MHz.
> >>
> >> We try dividers 1, 2, 3 and end up with 52.1MHz the first result.
> >>
> >> [ 18.815432] 20220509 get_clk_div_rate desired_clk:51200000
> >> [ 18.821081] 20220509 get_clk_div_rate maxdiv:4095
> >> [ 18.825924] 20220509 get_clk_div_rate div:1
> >> [ 18.830239] 20220509 get_clk_div_rate freq:52174000
> >> [ 18.835288] 20220509 get_clk_div_rate div:2
> >> [ 18.839628] 20220509 get_clk_div_rate freq:100000000
> >> [ 18.844794] 20220509 get_clk_div_rate div:3
> >> [ 18.849119] 20220509 get_clk_div_rate freq:100000000
> >> [ 18.854254] 20220509 get_clk_div_rate reached max frequency breaking...
> >> [ 18.861072] 20220509 get_clk_div_rate clk_div=1, ser_clk=52174000
> >>
> >> The behaviour was same earlier too when root_freq table was present.
> > Are you certain about the behavior being the same earlier? Before
> > commit c2194bc999d4 ("tty: serial: qcom-geni-serial: Remove uart
> > frequency table..."), the behavior was that get_clk_cfg() would return
> > 0 if there was no exact match. Then get_clk_div_rate() would see this
> > 0 and print an error and return. Then the rest of
> > qcom_geni_serial_set_termios() would do nothing at all.
> >
> > Ah, or I guess what you're saying is that the table historically
> > contained "rounded" rates but that clk_round_rate() isn't returning
> > nice round rates. OK, but if we truly want to support an inexact
> > match, you'd want to pick the rate that reduces the error, not just
> > pick the first one. In other words, something like this (untested):
> >
> > freq = clk_round_rate(clk, mult);
> > diff = abs(((long)mult - freq) / div);
> > if (diff < best_diff) {
> > best_diff = diff;
> > ser_clk = freq;
> > best_div = div;
> > }
> I am not sure if its required that freq is a multiple of best_div now
> that we don't have a multiple of desired_clk anyway.
How about just this (untested):
freq = clk_round_rate(clk, mult);
candidate_div = max(1, DIV_ROUND_CLOSEST(freq, desired_clk));
candidate_freq = freq / candidate_div;
diff = abs((long)desired_clk - candidate_freq);
if (diff < best_diff) {
best_diff = diff;
ser_clk = freq;
best_div = candidate_div;
}
Here:
freq: a freq we can definitely make
candidate_div: the best number to divide freq by to get the desired clock.
candidate_freq: the frequency we'll end up if we divide freq by
candidate_div. We want this to be close to desired_clk.
diff: how far away the candidate_freq is away from what we want.
best_diff: how far away the best candidate was from what we wanted.
ser_clk: What we should pass to clk_set_rate() to get the best candidate.
best_div: What we should use as a divider to get the best candidate.
-Doug
