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

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3424 bytes --]

On Wed, May 28, 2025 at 02:25:56AM +0000, Yao Zi wrote:
> 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.

With that,
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
I'd suggest you also send the driver patch to solve the issue on
platforms with whatever existing devicetree with both properties that
you're dealing with - or at the very least it'd open some discussion
about your problem.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]