Re: [PATCH] dt-bindings: serial: 8250: Make clocks and clock-frequency exclusive

[Yao Zi <ziyao@disroot.org>]

On Tue, May 27, 2025 at 04:24:11PM +0100, Conor Dooley wrote:
> On Sat, May 24, 2025 at 10:56:02AM +0000, Yao Zi wrote:
> > The 8250 binding before converting to json-schema states,
> > 
> >   - clock-frequency : the input clock frequency for the UART
> >   	or
> >   - clocks phandle to refer to the clk used as per Documentation/devicetree
> > 
> > for clock-related properties, where "or" indicates these properties
> > shouldn't exist at the same time.
> > 
> > Additionally, the behavior of Linux's driver is strange when both clocks
> > and clock-frequency are specified: it ignores clocks and obtains the
> > frequency from clock-frequency, left the specified clocks unclaimed. It
> > may even be disabled, which is undesired most of the time.
> 
> That sounds like an issue in the driver itself, no? If the clock phandle
> is present it sounds like the driver should be claiming the clock
> whether a frequency is specified or not. If so, that should be fixed
> whether this patch gets applied or not.

Agree.

> > 
> > But "anyOf" doesn't prevent these two properties from coexisting, as it
> > considers the object valid as long as there's at LEAST one match.
> > 
> > Let's switch to "oneOf" and disallows the other property if one exists,
> > exclusively matching the original binding and avoid future confusion on
> > the driver's behavior.
> 
> Have you checked whether or not there are devices that have both
> in-tree? If there are, can you fix them up as part of the change, rather
> than adding new warnings.

Had taken a brief search, seems all UARTs ship both clock-frqeuency and
clocks properties are snps,dw-apb-uart variants, which are not related
to the generic 8250 binding. So I think it shouldn't cause new warnings.

> > 
> > Fixes: e69f5dc623f9 ("dt-bindings: serial: Convert 8250 to json-schema")
> > Signed-off-by: Yao Zi <ziyao@disroot.org>
> > ---
> >  Documentation/devicetree/bindings/serial/8250.yaml | 10 +++++++---
> >  1 file changed, 7 insertions(+), 3 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/serial/8250.yaml b/Documentation/devicetree/bindings/serial/8250.yaml
> > index dc0d52920575..4322394f5b8f 100644
> > --- a/Documentation/devicetree/bindings/serial/8250.yaml
> > +++ b/Documentation/devicetree/bindings/serial/8250.yaml
> > @@ -45,9 +45,13 @@ allOf:
> >                    - ns16550
> >                    - ns16550a
> >      then:
> > -      anyOf:
> > -        - required: [ clock-frequency ]
> > -        - required: [ clocks ]
> > +      oneOf:
> > +        - allOf:
> 
> Why is the allOf needed here? Does
> oneOf:
>   - required: foo
>   - required: bar
> not work? There's a bunch of bindings doing that, so not sure why it
> doesn't work in your case.

Oops, you're right, it does work here and emits an "... is valid under
each of ..." error. Will change to this form in v2.

> Cheers,
> Conor.
> 
> > +            - required: [ clock-frequency ]
> > +            - properties: { clocks: false }
> > +        - allOf:
> > +            - required: [ clocks ]
> > +            - properties: { clock-frequency: false }
> >  
> >  properties:
> >    compatible:
> > -- 
> > 2.49.0
> > 

Best regards,
Yao Zi