Quoting Marek Behun (2021-09-03 10:31:07) > The 'fixed-mmio-clock' binding currently only allows for the fixed-rate > clock frequency to be read directly from the MMIO register. > > There are, however, systems for which the value of a register uniquely > determines the frequency, but it is not encoded as a number in the > register. Rather the register may contain the latched values of the > strapping pins during system reset, and the clock rate can be determined > from the value of one strapping pin. > > For example on Armada 37xx, the GPIO1[9] pin must be brough low or high s/brough/brought/ > during system reset depending on whether the reference clock rate is > 25 MHz or 40 MHz. > > Extend this binding by adding two more properties: > - clock-rate-table-mask - if present, the register value will be masked > with the value of this property before mapping > - clock-rate-table - table mapping possible clock rates to register > values > [..] > 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? 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. > > The reason why I wrote this is that there are several clk drivers > reading one bit of a register and then registering fixed-rate clock > with frequency depending on that one bit. Most of them are drivers > also registering other clocks, but there is at least one, > armada-37xx-xtal, which only does this. > > I think that systems where the reference clock can have different rates > and the rate is encoded into value of a strapping pin during reset > should describe these reference clocks in device tree with a table > mapping these possible rates to values of strapping pins. > --- > .../bindings/clock/fixed-mmio-clock.yaml | 60 +++++++++++++++++-- > 1 file changed, 55 insertions(+), 5 deletions(-) > > diff --git a/Documentation/devicetree/bindings/clock/fixed-mmio-clock.yaml b/Documentation/devicetree/bindings/clock/fixed-mmio-clock.yaml > index 1453ac849a65..67fef63cdd8c 100644 > --- a/Documentation/devicetree/bindings/clock/fixed-mmio-clock.yaml > +++ b/Documentation/devicetree/bindings/clock/fixed-mmio-clock.yaml > @@ -4,16 +4,15 @@ > $id: http://devicetree.org/schemas/clock/fixed-mmio-clock.yaml# > $schema: http://devicetree.org/meta-schemas/core.yaml# > > -title: Binding for simple memory mapped IO fixed-rate clock sources > +title: Binding for memory mapped IO fixed-rate clock sources > > description: > This binding describes a fixed-rate clock for which the frequency can > - be read from a single 32-bit memory mapped I/O register. > - > - It was designed for test systems, like FPGA, not for complete, > - finished SoCs. > + be determined from value read from a single 32-bit memory mapped I/O > + register. > > maintainers: > + - Marek Behun <kabel@xxxxxxxxxx> > - Jan Kotas <jank@xxxxxxxxxxx> > > properties: > @@ -29,11 +28,50 @@ properties: > clock-output-names: > maxItems: 1 > > + clock-rate-table: > + $ref: /schemas/types.yaml#/definitions/uint32-matrix > + description: | > + If this property is present, it means that the MMIO register does > + not contain the clock rate itself, but rather that different > + values of this register (possibly masked, see the > + 'clock-rate-table-mask' property) correspond to different clock > + rates, and this property maps each possible clock rate to > + corresponding register value. > + > + Some SOCs, for example, allow for multiple possible frequencies of > + reference clocks, and the system can determine clock rate by the > + values of strapping pins during reset, which are latched into some > + MMIO registers. > + > + clock-rate-table-mask: > + description: > + Mask to be applied to the MMIO value before mapping the value to > + corresponding clock rate via 'clock-rate-table'. > + $ref: /schemas/types.yaml#/definitions/uint32 > + > + items: > + minItems: 2 > + maxItems: 2 > + items: > + - description: > + Clock rate in Hertz. > + - description: > + MMIO value (masked with value of the 'clock-rate-table-mask' > + property, if that property is present) corresponding to this > + clock rate. > + > required: > - compatible > - reg > - "#clock-cells" > > +if: > + required: > + - clock-rate-table-mask > +then: > + required: > + - clock-rate-table > + > additionalProperties: false > > examples: > @@ -44,4 +82,16 @@ examples: > reg = <0xfd020004 0x4>; > clock-output-names = "sysclk"; > }; > + > + - | > + xtalclk: xtal-clk { This needs a unit address. > + 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. > + clock-rate-table-mask = <0x200>; > + clock-rate-table = <25000000 0x000>, > + <40000000 0x200>; > + clock-output-names = "xtal"; > + };
