Hi Stephen,

On Tue, 3 Nov 2020 at 06:45, Stephen Boyd [off-list ref] wrote:

Quoting Daniel Palmer (2020-10-30 04:52:31)

quoted

As there is no documentation for this thing and I'm not sure what the
logical output numbers are or even if I know all of them I was
considering making the number of clock cells 2 and having the first be
the first divider (i.e. the divide by 2 output would be 2) and the
second cell the chained divider or 0 for no divider.

Does the PLL need to be expressed anywhere in the binding?

The PLL frequency doesn't seem to need to be exposed to the outside.
It's configured to generate 864MHz from the 24MHz input clock but
nothing else has 864MHz as an input clock.
At the moment I register a clk for the PLL but only the dividers are
accessible from OF.
The only thing that runs with a clock anywhere near 864MHz is the CPU
and it has its own PLL that's fed from the divide by 2 output from
this PLL.
So basically output 0 from the PLL block will be the first divider.

Here's a condensed version of clk_summary:

 xtal24                               2        2        0    24000000
        0     0  50000
   mpll                              4        4        0   864000000
       0     0  50000
      mpll_div_5                     1        1        0   172800000
       0     0  50000
         mpll_172_gate               2        2        0   172800000
       0     0  50000
            uart0                    1        1        0   172800000
       0     0  50000

xtal24 is the external input clk. mpll is this PLL, mpll_div_5 is an
output from this PLL, mpll_172_gate (old naming from before I knew how
to calculate the mpll frequency) is a clock gate in a block that
coalesces various PLL outputs before they go into more dividers and
muxes to peripherals.

I did think about making just the PLL frequency an output and then
having the dividers expressed in the DT but the PLL block does have
enable bits for each of the outputs so they should logically be
handled by this driver I think.

quoted

If I should just decide the order of the outputs and come up with
indexes for them would it still be ok to nest them like the first cell
is the index of the divider and then the second cell is the index of
the chained divider?

Generally we try to encourage one-cell or zero-cell bindings because
they're simple. A two cell binding is possible if you really want but
I'd say do a one-cell binding and make up some numbering scheme for the
different clks. A one cell binding also makes it easy to populate an
array of clk_hw pointers that the DT xlate function can pick from. When
it gets to two cells or more it gets complicated, but still doable.

Ok. I've done that now. I decided that the second divider doesn't need
to be handled in the PLL driver itself as the divider is actually on
the other side of the gate described above.
There only needs to be one cell now. I have the primary dividers
ordered from smallest to largest and those indexes become the output
numbers.

It would be great if clk hardware started numbering the clks instead of
naming them specific names but this almost never happens. It would
certainly make life easier if the datasheet said "Use clk #25 for the
uart" but usually SoC hardware engineers give them names and don't
consider a pinout scheme. I'd say this is mostly because the engineers
control the connections between their clk controller and the consumers.

The PLL seems to have enable bits for each of the dividers going from
smallest to highest so going in that order for the indexes made sense
to me.
What I didn't want to happen is to find out there is actually another
divider somewhere in the middle of the ones I know about and then have
to work out how to add that in.
So having the first cell being the divisor instead of an index seem to
make some sense.

Finally, if you don't have a datasheet then think about the person who
has to write the DT. Are they going to know the details of the clk tree
inside the PLL unit to know that they need the third divider underneath
the second divider? Or are they going to know they need the "uart clk"
and that can be some friendly #define that hides this detail from them?

I'm thinking of listing the divisors in the DT so that which divider
is on which output is clear just from looking at the DT.
Something like mstar,mpll-divisors = <2>, <3>,... This would also help
with the issue above as the driver would just create clks for whatever
dividers are listed in the DT it was given.
For this PLL there is only one consumer of the outputs as all of the
outputs from this PLL and some others are feed into a set of gates
before going to the muxes so the wiring in the
DT isn't very complicated.

For some background my very rough notes about the clocks in the chip
I'm working on are here:
http://linux-chenxing.org/ip/clks.html

Thanks for your comments.

Cheers,

Daniel