Add functions to sys/dev/extres/clk

[oskar.holmlund_ohdata.se]

Summary

Related to https://reviews.freebsd.org/D25118

clknode_default_ofw_map():
Ti SoC has "ti,clkctrl" that i deal with as a common clock. ti,clkctrl has two cells (offset and index) described in
Documentation/devicetree/bindings/clock/ti-clkctrl.txt

the new function clk_mux():
To handle the index I use the clknode_set_mux interface see D25118 sys/arm/ti/clk/ti_clk_clkctrl.c.
Example usage are in sys/gnu/dts/arm/am33xx-l4.dtsi to enable gdbclk field (bit 18) see TRM 8.1.12.1.30 CM_PER_GPIO2_CLKCTRL
1302 clocks = <&l4ls_clkctrl AM3_L4LS_GPIO2_CLKCTRL 0>,
1303 <&l4ls_clkctrl AM3_L4LS_GPIO2_CLKCTRL 18>;

[mmel]

sys/dev/extres/clk/clk.c

380

This is default mapping function. So it should be valid for majority of possible usage cases. I'm pretty sure that ignoring half of 'address' (second cell) cannot be taken as common usage. And i think that this is wrong also for case of TI clocks.

1274

This is not right. Direct usage of CLKNODE_SET_MUX() leaves rest of framework out of sync. By changing of clock topology, you must adjust clock enable count for original and new parent , you must recompute cached frequency for child clock nodes or so. Which is exactly what clknode_set_parent...() do.
You wrote:
The new function clk_mux():
To handle the index I use the clknode_set_mux interface to enable gdbclk field (bit 18)
clocks = <&l4ls_clkctrl AM3_L4LS_GPIO2_CLKCTRL 0>,

<&l4ls_clkctrl AM3_L4LS_GPIO2_CLKCTRL 18>;

Problem is that this references 2 different clock nodes, so your l4ls_clkctrl implementation should create it.
The “l4ls_clkctrl AM3_L4LS_GPIO2_CLKCTRL 18” exposes appropriate functional clock gated by gdbclk bit, and “l4ls_clkctrl AM3_L4LS_GPIO2_CLKCTRL 0” is another clock (interface) gated by 1:0 bits.
(for more details, please see subsequent review on D25118)

[oskar.holmlund_ohdata.se]

sys/dev/extres/clk/clk.c

1274

Thank you for your comments!

I agree that your proposal makes a much better implementation if i can create two clocks and i think no changes will be necessary at all in the dev/extres/clk.

But i cant see how i can get the information from the DTS.
For example:
in am33xx-clocks.dtsi (devicetree documentation at the end)
539 per_cm: per-cm@0 {
540 compatible = "ti,omap4-cm";
541 reg = <0x0 0x400>;
542 #address-cells = <1>;
543 #size-cells = <1>;
544 ranges = <0 0x0 0x400>;
546 l4ls_clkctrl: l4ls-clkctrl@38 {
547 compatible = "ti,clkctrl";
548 reg = <0x38 0x2c>, <0x6c 0x28>, <0xac 0xc>, <0xc0 0x1c>, <0xec 0xc>, <0x10c 0x8>, <0x130 0x4>;
549 #clock-cells = <2>;
550 };

<0x38 0x2c> only tells me that the first clkctrl is at offset 0x38, next 0x3c and so on until 0x64. To relate to the TRM see chapter 8.1.12.1
0x38 = CM_PER_UART5_CLKCTRL
0x3C = CM_PER_MMC0_CLKCTRL
...and so on.

The range that is interesting are actually <0xac 0xc> for the GPIO1-3, nothing about the gdbclk functional clock. So then i create the clocks/clkctrl i dont know about the functional clock at bit 18.

On the other side in the consumer in am33xx-l4.dtsi (mentioned before)
1302 clocks = <&l4ls_clkctrl AM3_L4LS_GPIO2_CLKCTRL 0>,
1303 <&l4ls_clkctrl AM3_L4LS_GPIO2_CLKCTRL 18>;

Now I know there is something at bit 18. should i create a new clock while i process "ti,sysc" node?

I appreciate all suggestion how i can get around the problem. Maybe the solution is to update the devicetree with the information somehow?

Documentation/devicetree/bindings/arm/omap/prcm.txt
Documentation/devicetree/bindings/clock/ti-clkctrl.txt

[mmel]

Oskar,
Firstly, it seems to have forgotten to thank you for your efforts. I apologize for my discourtesy.
Also allow me to continue here..

Implementation of clocks (and its description in DT) was significantly evolved in past. Its evolved from list of linked clock nodes provides basic functions (like old Allwinner clocks) through mixed implementation (where DT nodes are mixed with table based information in driver) to newest one, where given clock domain is represented as single block and internal clock structure is provided only by tables in driver (like Tegra and many others).

So in your particular case, you may add some tables to 'ti,clkctrl' driver. Something like “for ti,clkctrl with name 'l4ls-clkctr' expose additional clock with name ‘xyz’ as clock gate driven by bit 18”. This seems like the easiest solution for me.

Alternatively you may use clock nodes from DT only for clock mapping and do switch to full table based implementation...
In any case, you must define 2 different clocks in above case, the whole clock framework doesn’t allow other solution.

[oskar.holmlund_ohdata.se]

Thank you @mmel for the background and direction.

I have updated the ti,clkctrl implementation in review https://reviews.freebsd.org/D25118. No need to update dev/extres/clk at all.