Add QorIQ platform clockgen driver.

[dgr_semihalf.com]

Summary

Tested with LS1046A SoC. Contains classes and functions that can be
used with various QorIQ Layerscape SoCs.

PLL nodes cannot be written. Additional divider nodes are created
on PLL outputs.

Uses built-in clk_mux nodes to represent CMUX and HWACCEL clock
nodes.

This is a preparation patch for NXP LS1046A SoC support.

[manu]

Thanks for those reviews, I'll try to find some time in the next few days to review (mostly on the clock framework usage as I don't have access to the docs).

[mmel]

With full respect, this is nothing that fake driver.
Faking chip clock structure with set of fixed clock nodes with statically computed frequency is not useful at all.
Please try again, this time so that the driver reflects the real clock tree structure.
And because I'm responsible for not having the documentation for clock framework , I offering maximum possible help.

sys/arm64/qoriq/clk/qoriq_clkgen.c

167 ↗

(On Diff #70369)

Modify operation should follow QORIQ_LITTLE_ENDIAN modifier.

[dgr_semihalf.com]

Pretty much a total rewrite. Now more closely represents the clock
node structure. For PLL nodes separate classes are now used, but
the PLL registers are read only, so there is no write functionality.
Other than that multiplexers are now represented by built-in classes
and PLL dividers are represented by fixed factor clocks.

The real driver using these classes (for LS1046A SoC) is still
contained in child revision but if it should be merged with this one
then I can do that.

[manu]

It's a bit hard to review without knowing the clock topology, is that something that you can share or can you describe it a bit ?
From what I understand of the code there is N pll based on the number of divider ?
Does all those PLLs really exists ?

sys/arm64/qoriq/clk/qoriq_clk_pll.c

82 ↗

(On Diff #71195)

recalc_freq is supposed to write the frequency of the clock in *freq, is the freq == parent_freq when the PLL have the "kill bit" ?

[dgr_semihalf.com]

Each PLL has multiple dividers. For each PLL single PLL node is created
and fixed factor clock nodes are created for each divider that the PLL has.
So only a single PLL node exists, but there are multiple divider nodes.

As for the clock topology - there is single platform PLL, which supplies
clocks for the peripheral bus and additional PLLs for CPU cores. There
can be multiple core PLLs (For example - LS1046A has two PLLs - CGAPLL1
and CGAPLL2). Each PLL has fixed dividers on output. The core PLLs
are not accessible from dts.

Cores are supplied clock by CMUX multiplexer nodes - they select
output of one of the core PLLs and its divider.
The same applies to HWACCEL multiplexer nodes, but the clock output
is being sent to some of the peripheral controllers.

The FMAN nodes function differently depending on SoC, in most SoCs
those don't exist, but in some cases they simply refer to other clock
node and from what I've seen in docs for some of the devices there
are additional registers to read from.

sys/arm64/qoriq/clk/qoriq_clk_pll.c

82 ↗

(On Diff #71195)

Yes, when the kill bit is set the PLL is bypassed.

[dgr_semihalf.com]

Hi, if you have any more comments or remarks, please let me know. Thanks.

[mmel]

This is much better implementation, thanks.
I see only one majoe issue (for me), fortunately it’s easy to fix.
Please, never use synthetic (generated) names for clock mapping, instead use a clock ID for this job. Names must be unique per system and you cannot guarantee this. As opposite, clock ID is property of given clock domain - so you can guarantee that these are unique within single clock domain.
Please combine type cell value and index cell value into unique number and the use clknode_find_by_name() inside qoriq_clkgen_ofw_mapper(). See [1]

Additionally, please use clock names from TRM where it’s possible. The clocks code is always hard and there is no reason to insert unnecessary obfuscation layer to it.

sys/arm64/qoriq/clk/qoriq_clkgen.c

81 ↗

(On Diff #71195)

[1]
Should be in qoriq_clkgen.h:

#define	QORIQ_CLK_ID(_type, _index) (((_type) << 8) +(_index))

and here :

/* Use clock node id for lookup. */
*clk = clknode_find_by_id(QORIQ_CLK_ID(cells[0], cells[1]));

126 ↗

(On Diff #71195)

Modify operation should follow QORIQ_LITTLE_ENDIAN flag.

178 ↗

(On Diff #71195)

this doesn't follow bindings (see /Documentation/devicetree/bindings/clock/qoriq-clock.txt
If "clock-frequency" is declared, you should create sysclk node itself as fixed clock with
given frequency. Otherwise , you should use 'sysclk' and (optionally) 'coreclk' as external as is defined in binding documentation.

196 ↗

(On Diff #71195)

All this is unnecessary. You should use resoved name of 'sysclk' as parent name for platform pll and resolved name of 'coreclk' for core plls.

sys/arm64/qoriq/clk/qoriq_clkgen.h

41 ↗

(On Diff #71195)

All these defines are not necessary. The clock name are SoC specific and you should not limit implementer to some synthetic name pattern.

[dgr_semihalf.com]

Modified driver to use clock id for lookup instead of names.
Modified the way sysclk and coreclk are created - currently
should follow bindings. The clock sources for PLLs are now
selected by driver automatically depending on the presence
of coreclk.

[dgr_semihalf.com]

sys/arm64/qoriq/clk/qoriq_clkgen.c

202 ↗

(On Diff #72071)

Here I left the code creating nodes in current clock domain. I think
that a better approach would be to save clock names in softc, but
this would require modifying clock interface as from what I've noticed
there is no way of accessing softc from ofw_mapper function. This would
only require adding device_t getter function for clkdom, however
I don't want to modify existing interfaces without someone more
experienced sharing their opinion.

[mmel]

LGTM.
I looking forward when my HoneyComb LX2K arrives, order was placed . I got offer (paid by this card :) ) to port FreeBSD to CEx7 LX2160A module. And because customer wants maximum control of clocks, clocks gating, inactive phys and so, only fully featured FDT base port is possible. So, please, be prepared to do some testing of new code on your board :)

sys/arm64/qoriq/clk/qoriq_clkgen.c

202 ↗

(On Diff #72071)

I don’t want to block you, so please take all bellow as discussion on implementation detail.
Please don’t map 2 different DT identifiers to one clock node, I think that this can create unexpected side effects on consumer side.
But, yeah, it seems that sysclk and/or coreclk can be referenced from other DT nodes when these are external, driven by other clock from other clock domain (see fsl-ls1043a.dtsi and clock definition for lpuart0 node). Then yes, these “phantom” 1:1 dividers are probably best solution. I did not noticed this previously, so sorry for noise. Only please, put this to short comment

[dgr_semihalf.com]

Added comment about 1:1 sysclk and coreclk divider nodes.

[mmel]

Thanks!