Received-SPF: pass (google.com: domain of linux-kernel+bounces-209003-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240604160123.2029413-1-dianders@chromium.org>
 <20240604090028.v3.2.I3e1968bbeee67e28fd4e15509950805b6665484a@changeid> <554749d5-5709-c740-b05d-bd4957d1e8d0@linux.intel.com>
In-Reply-To: <554749d5-5709-c740-b05d-bd4957d1e8d0@linux.intel.com>
From: Doug Anderson <dianders@chromium.org>
Date: Mon, 10 Jun 2024 15:26:15 -0700
Message-ID: <CAD=FV=VLa1pZv1gy7WONzThopx6RXf_8Uh3L0wCWdY6_Mo1KTA@mail.gmail.com>
Subject: Re: [PATCH v3 2/7] serial: qcom-geni: Fix the timeout in qcom_geni_serial_poll_bit()
To: =?UTF-8?Q?Ilpo_J=C3=A4rvinen?= <ilpo.jarvinen@linux.intel.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Jiri Slaby <jirislaby@kernel.org>, 
	=?UTF-8?Q?Uwe_Kleine=2DK=C3=B6nig?= <u.kleine-koenig@pengutronix.de>, 
	Andy Shevchenko <andriy.shevchenko@linux.intel.com>, linux-arm-msm@vger.kernel.org, 
	Konrad Dybcio <konrad.dybcio@linaro.org>, LKML <linux-kernel@vger.kernel.org>, 
	linux-serial <linux-serial@vger.kernel.org>, John Ogness <john.ogness@linutronix.de>, 
	Yicong Yang <yangyicong@hisilicon.com>, Tony Lindgren <tony@atomide.com>, 
	Stephen Boyd <swboyd@chromium.org>, Johan Hovold <johan+linaro@kernel.org>, 
	Bjorn Andersson <andersson@kernel.org>, Thomas Gleixner <tglx@linutronix.de>, 
	Vijaya Krishna Nivarthi <quic_vnivarth@quicinc.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi,

On Fri, Jun 7, 2024 at 12:50=E2=80=AFAM Ilpo J=C3=A4rvinen
<ilpo.jarvinen@linux.intel.com> wrote:
>
> > +     /*
> > +      * This function is used to poll bits, some of which (like CMD_DO=
NE)
> > +      * might take as long as it takes for the FIFO plus the temp regi=
ster
> > +      * on the geni side to drain. The Linux core calculates such a ti=
meout
> > +      * for us and we can get it from uart_fifo_timeout().
> > +      *
> > +      * It should be noted that during earlycon the variables that
> > +      * uart_fifo_timeout() makes use of in "uport" may not be setup y=
et.
> > +      * It's difficult to set things up for earlycon since it can't
> > +      * necessarily figure out the baud rate and reading the FIFO dept=
h
> > +      * from the wrapper means some extra MMIO maps that we don't get =
by
> > +      * default. This isn't a big problem, though, since uart_fifo_tim=
eout()
> > +      * gives back its "slop" of 20ms as a minimum and that should be
> > +      * plenty of time for earlycon unless we're running at an extreme=
ly
> > +      * low baud rate.
> > +      */
> > +     timeout_us =3D jiffies_to_usecs(uart_fifo_timeout(uport));
>
> Hi,
>
> While this is not exactly incorrect, the back and forth conversions nsecs
> -> jiffies -> usecs feels somewhat odd, perhaps reworking
> uart_fifo_timeout()'s return type from jiffies to e.g. usecs would be
> preferrable. As is, the jiffies as its return type seems a small obstacle
> for using uart_fifo_timeout() which has come up in other contexts too.

Sure. I'll change it to "ms" instead of "us". We don't need the
fidelity of "us" here given that the function is adding 20 ms of slop
anyway so might as well return ms so that callers don't need to do so
much math and don't need to work with u64.

This means that I'll have to add a "* USEC_PER_MSEC" in my driver, but
it still feels like the more correct thing to do. It also has the nice
side effect of allowing the driver to remove the awkward
"DIV_ROUND_UP(timeout_us, 10) * 10" because we know that the timeout
will always be a proper multiple.

I'll also add a new function with the _ms suffix instead of changing
the old one. The suffix makes it clear to the caller what the unit of
the returned value is and we might as well leave the old wrapper
there--otherwise we just need to move the jiffies conversion into the
existing callers.