clk: add initial frequency / gate config methods

[adrian]

Summary

Some clock node hardware (eg the qualcomm code I'm working on)
allows you to read the current frequency and gate configuration.

So:

• For root fixed clock nodes, we may as well configure their frequency at init time, as we don't need to read the clock frequency of any parent nodes to know what our frequency is.

• For gates that can read their own status, configure the gate status.

clk: track whether the clock is enabled or not, provide it out via sysctl

This is useful for gates that set their status at boot time but haven't
yet been programmed.

clk: add call for nodes to get the programmed/decided frequency passed back

The existing call can only really be used for a node wishing to
configure its parent, but as we don't pass in a pointer to the freq,
we can't set it to what it would be for a DRY_RUN pass.

So for clock nodes that wish to try setting parent frequencies to see
which would be the best for its own target frequency, we really do need
a way to call in and pass in a flag /and/ a pointer to freq so it can be
updated for us as the clock tree is recursed through.

[manu]

sys/dev/extres/clk/clk.c

786

I don't understand the point of this.
A clock should have a recalc method and this will be called by the clk framework.

796

I also don't understand this, a gate should use the clknode_gate method and the init function sets the ungated variable at init by reading the register.

[adrian]

sys/dev/extres/clk/clk.c

786

that only happens when a clock frequency change occurs. It doesn't happen otherwise, so when you look at the clock tree all of the fixed frequency sources show as 0 until something has tried recursing through the clock tree to set a frequency.

This lets me see all of the fixed frequency parent nodes correctly in sysctl.

796

but clknode is opaque to the nodes themselves, right? how does a gate node set its current state in its init function? i added this so i could see the state of the gates in sysctl before anything used them, so i could see how the clock tree was configured by the hw/bootloader.

[manu]

sys/dev/extres/clk/clk.c

786

Mhm no, the recalc_freq is also called in clknode_get_freq and this is what is used for dumping the clocks so we have the real freq at the first sysctl hw.clock if we didn't before.

796

See https://cgit.freebsd.org/src/tree/sys/arm64/rockchip/clk/rk_clk_gate.c#n74
We read the register and set sc->ungated variable according to this.

[mmel]

Sorry for the strong language, please don't take it the wrong way.
clknode_init_freq_value() - this function is completely unnecessary, and only complicates an already complicated interface. Moreover, it is only used here as papering over a real problem in sysctl dump code. Ii really don't want that.

clknode_init_gate_value() - this function does not work at all as intended. For example, a given clock can be gated on any parent. The enable_count represents the number of cunsumers requesting a given clock. The only thing that is certain is that when enable_count > 0 the given clock chain is ungated. The reverse is generally not true , enable_count == 0 says nothing about the state of the gate. Also we have systems where one bit in the one register enables multiple clock gates.If we want to display the gate state in a sysctl dump, we should implement a clknode_get_gate() method that returns the current state of the HW (if the HW allows it).

clknode_set_freq() - this change makes sense to me and the current implementation of clknode_set_freq() is probably the most problematic function in the current clock framework.
However, I must warn you that splitting a frequency change request into multiple nodes (like selecting the correct multiplexer branch and then requesting a clock change on the selected branch) does not work without additional SoC specific knowledge. For example, you need to be sure that for a live clock , the changes must be committed in the correct order so that the target device is not overclocked (even for a minimal amount of time).
But I would rather see the functionality split into clknode_set_freq() and clknode_try_freq(). What do you think?

[adrian]

In D33445#759706, @mmel wrote:

Sorry for the strong language, please don't take it the wrong way.

lol, don't be sorry!

clknode_init_freq_value() - this function is completely unnecessary, and only complicates an already complicated interface. Moreover, it is only used here as papering over a real problem in sysctl dump code. Ii really don't want that.

That's fine, I'll delete it!

clknode_init_gate_value() - this function does not work at all as intended. For example, a given clock can be gated on any parent. The enable_count represents the number of cunsumers requesting a given clock. The only thing that is certain is that when enable_count > 0 the given clock chain is ungated. The reverse is generally not true , enable_count == 0 says nothing about the state of the gate. Also we have systems where one bit in the one register enables multiple clock gates.If we want to display the gate state in a sysctl dump, we should implement a clknode_get_gate() method that returns the current state of the HW (if the HW allows it).

That's again fine, nothing I wrote depends upon it, so I'll delete it.

clknode_set_freq() - this change makes sense to me and the current implementation of clknode_set_freq() is probably the most problematic function in the current clock framework.
However, I must warn you that splitting a frequency change request into multiple nodes (like selecting the correct multiplexer branch and then requesting a clock change on the selected branch) does not work without additional SoC specific knowledge. For example, you need to be sure that for a live clock , the changes must be committed in the correct order so that the target device is not overclocked (even for a minimal amount of time).

Oh i agree. I'm only calling this inside my own clock driver path, and I can make sure that doesn't happen. We do need clock trees to do the right thing anyway!

But I would rather see the functionality split into clknode_set_freq() and clknode_try_freq(). What do you think?

I'll go and see if I can do that! Thanks!

[adrian]

update from mmels' feedback:

• remove the new functions, we'll work on it another way and it's not required for my qualcomm clock node work
• add a _try version of the freq API, rather than calling it freq2().

[mmel]

sys/dev/extres/clk/clk.c

1059

May I request last small change?

int
clknode_try_freq(struct clknode *clknode, uint64_t freq, int flags,
    int enablecnt, uint64_t *out_freq)
{
	int rv;
	rv = _clknode_set_freq(clknode, &freq, flags | CLK_SET_DRYRUN,
	    enablecnt);
	if (out_req != NULL)
		*out_freq = freq;
}

I believe that the combined IN/OUT argument is a bad practice for public interfaces.

[mmel]

You know I'm "creative", especially when it comes to booting SBC.
The failing sequence is:

netserver tftp://192.168.168.1/tegra/arm.armv7/sys/GENERIC/
set vfs.root.mountfrom="ufs:/dev/da0s2a"
boot net:kernel

This reminds me of another problem. Where should the configuration file be located if the actual loader is also loaded from u-boot using TFTP?
I know I only have trivial questions :)