Re: [PATCH v1 1/2] dt-bindings: clk: fixed-mmio-clock: Add optional ready reg

[Krzysztof Kozlowski <krzk@kernel.org>]

On 25/05/2025 21:08, Edgar E. Iglesias wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@amd.com>
> 
> Add an optional ready register and properties describing bitfields
> that signal when the clock is ready. This can for example be useful
> to describe PLL lock bits.
> 
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@amd.com>
> ---
>  .../bindings/clock/fixed-mmio-clock.yaml      | 38 ++++++++++++++++++-
>  1 file changed, 37 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/clock/fixed-mmio-clock.yaml b/Documentation/devicetree/bindings/clock/fixed-mmio-clock.yaml
> index e22fc272d023..90033ba389e8 100644
> --- a/Documentation/devicetree/bindings/clock/fixed-mmio-clock.yaml
> +++ b/Documentation/devicetree/bindings/clock/fixed-mmio-clock.yaml
> @@ -10,6 +10,11 @@ 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.
>  
> +  An optional ready register can be specified in a second reg entry.
> +  The ready register will be polled until it signals ready prior to reading
> +  the fixed rate. This is useful for example to optionally wait for a PLL
> +  to lock.
> +
>    It was designed for test systems, like FPGA, not for complete,
>    finished SoCs.
>  
> @@ -21,7 +26,10 @@ properties:
>      const: fixed-mmio-clock
>  
>    reg:
> -    maxItems: 1
> +    minItems: 1
> +    items:
> +      - description: Fixed rate register
> +      - description: Optional clock ready register
>  

I am not convinced we actually want this. If you have more complicated
clocks which need more than one register, then maybe this is too complex
for generic device and you should just make this part of clock controller.

Also I wonder how a clock, which is not controllable, cannot be gated,
can be ready or not. Issue is easily visible in your driver:
1. Probe the driver
2. Clock is not ready - you wait...
3. and wait and entire probe is waiting and busy-looping
4. Probed.
5. Unbind device
6. Rebind and again we check if clock is ready? Why? Nothing changed in
the hardware, clock was not disabled.

Although above is maybe better question for driver design, but it still
makes me wonder whether you are just putting driver complexity into DT.

>    "#clock-cells":
>      const: 0
> @@ -29,6 +37,25 @@ properties:
>    clock-output-names:
>      maxItems: 1
>  
> +  ready-timeout:
> +    description:
> +      Optional timeout in micro-seconds when polling for clock readiness.
> +      0 means no timeout.

Use a proper unit suffix.
https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/property-units.yaml

> +    $ref: /schemas/types.yaml#/definitions/uint32

Drop

> +    default: 0
> +
> +  ready-mask:
> +    description:
> +      Optional mask to apply when reading the ready register.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    default: 0xffffffff
> +
> +  ready-value:
> +    description:
> +      When a ready register is specified in reg, poll the ready reg until
> +      ready-reg & ready-mask == ready-value.
> +    $ref: /schemas/types.yaml#/definitions/uint32


> +
>  required:
>    - compatible
>    - reg
> @@ -44,4 +71,13 @@ examples:
>        reg = <0xfd020004 0x4>;
>        clock-output-names = "sysclk";
>      };
> +  - |
> +    pclk: pclk@fd040000 {

clock@

And drop unused label

> +      compatible = "fixed-mmio-clock";
> +      #clock-cells = <0>;
> +      reg = <0xfd040000 0x4 0xfd040004 0x4>;
> +      ready-mask = <1>;
> +      ready-value = <1>;
> +      clock-output-names = "pclk";
> +    };
>  ...


Best regards,
Krzysztof