Re: [PATCH v7 2/3] serial: 8250_dw: Simplify the ref clock rate setting procedure

[Serge Semin]

Hello Russell,

Thanks for your comments. My response is below.

On Sat, Jun 20, 2020 at 09:12:01AM +0100, Russell King - ARM Linux admin wrote:
> On Fri, Jun 19, 2020 at 11:02:50PM +0300, Serge Semin wrote:
> > Really instead of twice checking the clk_round_rate() return value
> > we could do it once, and if it isn't error the clock rate can be changed.
> > By doing so we decrease a number of ret-value tests and remove a weird
> > goto-based construction implemented in the dw8250_set_termios() method.
> 
> This doesn't look right to me - neither the before code nor the after
> code.
> 
> >  	clk_disable_unprepare(d->clk);
> >  	rate = clk_round_rate(d->clk, baud * 16);
> > -	if (rate < 0)
> > -		ret = rate;
> > -	else if (rate == 0)
> > -		ret = -ENOENT;
> > -	else
> > +	if (rate > 0) {
> >  		ret = clk_set_rate(d->clk, rate);
> > +		if (!ret)
> > +			p->uartclk = rate;
> > +	}
> >  	clk_prepare_enable(d->clk);
> >  
> > -	if (ret)
> > -		goto out;
> > -
> > -	p->uartclk = rate;
> 
> 	newrate = baud * 16;
> 
> 	clk_disable_unprepare(d->clk);

> 	rate = clk_round_rate(newrate);
> 	ret = clk_set_rate(d->clk, newrate);
> 	if (!ret)
> 		p->uartclk = rate;
> 
> 	ret = elk_prepare_enable(d->clk);
> 	/* check ret for failure, means the clock is no longer running */
> 
> is all that should be necessary: note that clk_round_rate() is required
> to return the rate that a successful call to clk_set_rate() would result
> in for that clock.

While I do understand your note regarding the newrate passing to both methods, I
don't fully get it why is it ok to skip checking the clk_round_rate() return
value? As I see it there is no point in calling clk_set_rate() if
clk_round_rate() has returned an error. From that perspective this patch
is full acceptable, right?

In addition to that in order to provide an optimization I'll have to check the
return value of the clk_round_rate() anyway in the next patch of this series
("serial: 8250_dw: Fix common clocks usage race condition") since I'll need to
set uartclk with that value before calling clk_set_rate() (see the patch and
notes there for details). So there is no point in removing the check here since
it will be got back in the next patch anyway.

One more note in favor of checking the clk_round_rate() return value is below.

> It is equivalent to:
> 

> 	ret = clk_set_rate(d->clk, newrate);
> 	if (ret == 0)
> 		p->uartclk = clk_get_rate(d->clk);

Alas neither this nor the suggested code above will work if the clock is
optional. If it is, then d->clk will be NULL and clk_round_rate(),
clk_set_rate() and clk_get_rate() will return zero. Thus in accordance with the
fixes suggested by you we'll rewrite a fixed uartclk value supplied by a
firmware.

To sum it up getting a positive value returned from clk_round_rate() will
mean not only an actual clock frequency, but also having a real reference clock
installed. So we can use that value to update the uartclk field of the UART port
descriptor.

> 
> The other commonly misunderstood thing about the clk API is that the
> rate you pass in to clk_round_rate() to discover the actual clock rate
> and the value passed in to clk_set_rate() _should_ be the same value.
> You should _not_ do clk_set_rate(clk, clk_round_rate(clk, newrate));

Agreed. Thanks for the comment. I'll fix it in the next version of the series.

-Sergey

> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!