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

["Edgar E. Iglesias" <edgar.iglesias@gmail.com>]

On Mon, May 26, 2025 at 06:53:14AM +0200, Krzysztof Kozlowski wrote:
> 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.

Right.

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

Yeah, in my particular case, once the clock is ready it will never go
back to "unready" until cold restart.

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

Yes, I felt that this was simple enough to have a generic mechanism but perhaps not.


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

Fixed for v2.


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

Fixed.


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