On 10/13/26 2:03 AM, Jerome Brunet wrote:
On lun. 12 oct. 2026 at 14:59, Alex Elder <[email protected]> wrote:
+ TC9564_CLOCK_INIT0(MAC0_TX, 7),
+ TC9564_CLOCK_INIT0(MAC0_RX, 14),
+ TC9564_CLOCK_INIT0(MAC0_125M, 29),
+ TC9564_CLOCK_INIT0(MAC0_312_5M, 30),
Are those really just providing the oscillator parent ?
That is what they will appear to do without a proper recalc_rate()
They are fixed clocks that can be gated, but their
rate cannot be changed.
It is not a problem if the rate cannot be changed but still
a consumer could call clk_get_rate() and change its behavior
based on the result, so it is better if it does reflect the
reality to some extent.
Just add fixed_factor clock elements on the path of clocks
where do know the output does not match the input osc
OK. And because I'm not sure whether it matches reality, I'll
do so in the code, and not expose these in devicetree. (If
this is not correct, please say so.)
Looking at __clk_core_init(), should I define a
recalc_rate() callback that just returns the
equivalent of get_rate()?
Their names suggest that maybe, it is not the osc coming out
of them ...
We have requested a clock tree that explains exactly
how these clock rates are derived from the 25 MHz
oscillator. They're all fixed rates, and although
we could easily speculate how they're derived, we
don't have the documentation.
If you know the input and output, you have enough to figure
out a fixed factor. If later on someone gets more information
than you, they'll just modify the clock tree.
OK.
If you no idea what the ouput rate is, at least clearly say
so in the comment so a future user or contributor can easily
understand the situation.
I'm pretty sure I know the output rate of all clocks, but
if not I'll make sure there is such a comment.
+ TC9564_CLOCK_INIT0(MAC0_ALL, 31),
[...]
+
+ clk = devm_clk_get(dev, NULL);
+ if (IS_ERR(clk))
+ return dev_err_probe(dev, PTR_ERR(clk),
+ "failed to get input clock\n");
Nothing is using this. This is not how you set the parent. See above
It will once I have the clocks get properly parented.
No it won't. You don't need a consumer clock reference. just drop it.
OK, yes, the parent(s) will be specified in the init data.
Sorry for this, and thank you for pointing it out.
+
+ clocks = tc9564_clk_init(dev);
+ if (IS_ERR(clocks))
+ return dev_err_probe(dev, PTR_ERR(clocks),
+ "failed to initialize clocks\n");
+
+ /* Force all clocks to be initially disabled */
+ tc9564_clock_disable_all(clocks);
Why is this needed ? In general a provider should never act the provided
clock on its own. Let the framework deal with the used clocks
The intent was to start with everything disabled--a known
state. Sashiko pointed out this races with a user that
could get the clock right away, so I intend to do this
by initially disabling all the clocks before they're
all registered.
We do not do that because (in general) there maybe devices already using
a clock provided by your device and setup at a prior stage (like the
bootloader). Doing so would break them.
Leave this alone, the framework will disable the unused clock for you
once the init is done and most devices have probed.
OK, I'll drop this. Thank you very much for your review.
-Alex
