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

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
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

Signed-off-by: Marek Behún <kabel@kernel.org>
---
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.

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(-)

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@kernel.org>
>    - Jan Kotas <jank@cadence.com>
>  
>  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";
> +    };

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