Re: [PATCHv2 5/5] clk: samsung: exynos5410: Added clocks DPLL, EPLL, IPLL, and VPLL

[Tomasz Figa]

On 31.07.2014 15:37, Humberto Naves wrote:
> Hi Tomasz,
> 
> I remember checking these rates on my calculator. You might notice the
> odd frequency of 45158401Hz (no pun intended) in the EPLL clock. This
> particular clock frequency was giving me a big headache in a previous
> project, since it was wrongly listed as 45158400. At first it seems
> innocuous, but whenever I would ask for one of the child clocks to
> change the rate, the driver would miscalculate the correct frequencies
> and errors would propagate up and down the clock tree.
> 
> Anyway, I would double check these tables. And if you want, I can
> write a separate patch for the rate table validation. I presume that
> you would like to add a new field, such as default_base_freq, to the
> structure samsung_pll_clock, and if that field is non-zero, you
> perform the validation of the table in _samsung_clk_register_pll?

I'm not sure I get the idea of the field you're suggesting. If I
understand correctly, your intention would be to provide a default
frequency if there is no table provided. I don't think there is a need
for it, because current code can read back current settings from
registers and calculate current rate.

As for the validation itself, one more thing that needs to be considered
is that the rate table must be sorted.

We once decided to rely on the fact that tables in SoC drivers have
rates explicitly specified and are correctly sorted, but now I'm
inclined to reconsider this, based on the fact that those rates often
are already incorrectly calculated in vendor code or even datasheets,
which are main information sources for patch authors.

Before mainlining PLL drivers (which was quite some time ago), we had a
bit different implementation in our internal tree, which did not use
explicitly specified rates at all (they could have been considered just
comments to improve table readability) and the _register_pll() function
simply calculated rates for all entries creating new table for internal
use of the PLL driver that was in addition explicitly sorted to make
sure that the order is correct. This kind of implementation is what I
would lean toward today.

Best regards,
Tomasz
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/