On 12/7/20 5:14 AM, Damien Le Moal wrote:
On 2020/12/07 19:06, Geert Uytterhoeven wrote:
Hi Damien,
On Mon, Dec 7, 2020 at 10:55 AM Damien Le Moal <Damien.LeMoal@xxxxxxx> wrote:
On 2020/12/07 17:44, Geert Uytterhoeven wrote:
On Mon, Dec 7, 2020 at 4:52 AM Damien Le Moal <Damien.LeMoal@xxxxxxx> wrote:
I prepared a v5 series addressing your comments (and other comments).
I will post that later today after some more tests.
Thanks, already looking at k210-sysctl-v18...
On Fri, 2020-12-04 at 22:19 -0800, Stephen Boyd wrote:
--- /dev/null
+++ b/drivers/clk/clk-k210.c
+ in0_clk = of_clk_get(np, 0);
+ if (IS_ERR(in0_clk)) {
+ pr_warn("%pOFP: in0 oscillator not found\n", np);
+ hws[K210_CLK_IN0] =
+ clk_hw_register_fixed_rate(NULL, "in0", NULL,
+ 0, K210_IN0_RATE);
+ } else {
+ hws[K210_CLK_IN0] = __clk_get_hw(in0_clk);
+ }
+ if (IS_ERR(hws[K210_CLK_IN0])) {
+ pr_err("%pOFP: failed to get base oscillator\n", np);
+ goto err;
+ }
+
+ in0 = clk_hw_get_name(hws[K210_CLK_IN0]);
+ aclk_parents[0] = in0;
+ pll_parents[0] = in0;
+ mux_parents[0] = in0;
Can we use the new way of specifying clk parents so that we don't have
to use __clk_get_hw(), of_clk_get(), and clk_hw_get_name()? Hopefully
the core can handl that all instead of this driver.
I removed all this by adding:
clock-output-names = "in0";
to the DT fixed-rate oscillator clock node (and documented that too). Doing so,
clk_hw_get_name(), __clk_get_hw() and of_clk_get() are not needed anymore and
the parents clock names arrays do not need run-time update.
"clock-output-names" is deprecated for clocks with a single output:
the clock name will be taken from the node name.
Arg. I missed that.
However, relying on a clock name like this is fragile.
Instead, your driver should use the phandle from the clocks property,
using of_clk_get_by_name() or of_clk_get().
That is what all versions before V5 used. But Stephen mentioned that the driver
should not, if possible, use of_clk_get()/__clk_get_name(). Hence the change.
Easy to revert back.
Stephen: I'm a bit puzzled, as you suggest _not_ using of_clk_get()?
Another solution to this would be to simply remove the fixed-rate clock node
from the DT and have the k210 clock driver unconditionally create that clock
(that is one line !). That actually may be even more simple than the previous
version, albeit at the cost of having the DT not being a perfect description of
the hardware. I am fine with that though.
Thoughts ?
If there's an external crystal, DT should describe it.
Does the K210 support different crystal frequencies?
I am not 100% sure if this oscillator is part of the SoC or if it is an external
input to it. Probably not. Hard to tell by just looking at the boards.
It's an external crystal. For example, it is U6 on the maix bit. This is
why I put it as a separate clock instead of including it with the rest.
--Sean
I have
the boards drawings though, so I will check. The frequency seems to be fixed by
hardware: frequencies of the PLLs can be changed to change the CPU frequency,
but the Kendryte SDK does not point to any way allowing changing the base
frequency of the oscillator>
Anyway, I'm very interested in what the (new) proper way of handling this
is...
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
