Jan,
On Wed, Aug 28, 2019 at 10:29:30PM +0200, Jan Kundrát wrote:
> On (some) of my boards, it appears that it takes up to three *checks* of
> this register's value for the external crystal to settle so that it is
> reported as "ready" by the chip. For example, on one of these boards it
> always succeeds upon a third try no matter if the individual waits are
> for 1ms or for 10ms. The original timeout of 10ms is therefore not ideal
> as it was generating false warnings on working HW for me. Let's solve
> this by retrying up to 20 times (i.e., 200ms).
>
> With this retrying in place, it is now also possible to fail device
> initialization altogether. A stable clock is really required in order to
> use this UART, so log an error message and bail out if the chip keeps
> saying "nope".
>
> Tested on several MAX14830 PCBs.
>
> Signed-off-by: Jan Kundrát <jan.kundrat@xxxxxxxxx>
> ---
> drivers/tty/serial/max310x.c | 15 +++++++++++----
> 1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
> index 0e0c2740ec7e..e8cd09d3e86f 100644
> --- a/drivers/tty/serial/max310x.c
> +++ b/drivers/tty/serial/max310x.c
> @@ -610,11 +610,14 @@ static int max310x_set_ref_clk(struct device *dev, struct max310x_port *s,
>
> /* Wait for crystal */
> if (xtal) {
> - unsigned int val;
> - msleep(10);
> - regmap_read(s->regmap, MAX310X_STS_IRQSTS_REG, &val);
> + unsigned int val, i;
> + for (i = 0; i < 20 && !(val & MAX310X_STS_CLKREADY_BIT); ++i) {
Don't you get a "'val' used uninitialized" compile-time warning here? Anyway
it must be zeroed before usage otherwise you might occasionally/permanently
end up with skipping this loop and the next conditional statement.
Though the former code was also buggy since in case if regmap_read() method
returned a non-zero value, the 'val' variable would be left uninitialized.
But this is a generic problem of the driver code, since it's a bit clumsy
at some places (I mean magic number literals and return values left untested).
I would also suggest to define and use the constants like MAX310X_XTAL_WAIT_RETRIES
and MAX310X_XTAL_WAIT_DELAY, so to parametrise this check-delay-loop.
Though it is up to you and the driver maintainer whether it's necessary to be
done.
-Sergey
> + msleep(10);
> + regmap_read(s->regmap, MAX310X_STS_IRQSTS_REG, &val);
> + }
> if (!(val & MAX310X_STS_CLKREADY_BIT)) {
> - dev_warn(dev, "clock is not stable yet\n");
> + dev_err(dev, "clock is not stable\n");
> + return -EAGAIN;
> }
> }
>
> @@ -1301,6 +1304,10 @@ static int max310x_probe(struct device *dev, struct max310x_devtype *devtype,
> }
>
> uartclk = max310x_set_ref_clk(dev, s, freq, xtal);
> + if (uartclk < 0) {
> + ret = uartclk;
> + goto out_uart;
> + }
> dev_dbg(dev, "Reference clock set to %i Hz\n", uartclk);
>
> for (i = 0; i < devtype->nr; i++) {
> --
> 2.21.0
>
>
