Re: [PATCH RFC] dt-bindings: clk: fixed-mmio-clock: Document mapping MMIO values to clock rates

Marek Behún
2021-09-03T21:42:47Z

On Fri, 03 Sep 2021 11:29:38 -0700
Stephen Boyd <sboyd@kernel.org> wrote:

> > This patch applies only after fixed-mmio-clock is converted to YAML by
> > dt-bindings: clk: fixed-mmio-clock: Convert to YAML
> >
> > This is a RFC and does not contain actual driver change. I would like
> > to hear your opinions.
>
> When it comes to masks and shifts in DT it's a NAK from me. I believe we
> don't have a good way to understand what endianess the mask is. Is it
> device order, or CPU order, or always big endian?

Several DT bindings already have little-endian/big-endian properties,
so this is solvable. Of course it is possible that such things are
frowned upon and I am not aware.

In this particular case though the register is part of the NB pinctrl
registers and that node is also a syscon node. In fact for the driver
I was considering to look at parent device whether it is a syscon node,
and if so, access the register via regmap. Regmaps have endianity
defined properly.

> It's also trending toward the one node per clk style of binding that we
> don't accept. I think this came up when the fixed-mmio binding was
> proposed. I hoped that nobody would use it outside of FPGAs.

I agree that one node per clk is not a good solution in most cases.

Yes, it would be possible to abuse this binding by for example using it
to define all peripheral clocks on 1 GHz variant of Armada 3720.

This is not the intended usage. The intended usage is system reference
clocks.

For reference clocks there _really is_ one crystal chip soldered on the
board. In this case I think one device node (per chip soldered on board)
is not something insane...

The reason why I did write this is that the current solution for A3720
seems weird to me: we have armada-37xx-xtal driver, which just looks at
one bit of one specific register registers fixed-rate clock with rate
depending on the value of that bit. This just looked like something
that should be generalized.

Also it makes more sense to me to have the possible frequencies of
reference clocks listed in the device tree instead of the driver, for
devices which allow several possible values. But others may of course
disagree...

> > + compatible = "marvell,armada-3700-xtal-clock", "fixed-mmio-clock";
> > + #clock-cells = <0>;
> > + reg = <0x8 0x4>;
>
> Because it has a reg property. Of course, a reg property that isn't
> aligned to a 4k page or so would imply that the clk is actually part of
> a larger hardware block that should have a binding for the whole device
> instead of picking the clk part out of the hardware and setting a node
> to be exactly the one register in there that is of interest.

I actually was thinking about whether I should also add parent node in
this example, because yes, in this case the register resides in
north-bridge pinctrl device register space. I decided against it in the
end, but I can do it in future version, if we decide that we want to do
what I am proposing.

Marek