On Fri, 03 Sep 2021 11:29:38 -0700 Stephen Boyd <sboyd@xxxxxxxxxx> 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
