| Message ID | 20250524105602.53949-1-ziyao@disroot.org |
|---|---|
| State | New |
| Headers | show |
| Series | dt-bindings: serial: 8250: Make clocks and clock-frequency exclusive | expand |
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. > > 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. > > 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. Cheers, Conor. > + - required: [ clock-frequency ] > + - properties: { clocks: false } > + - allOf: > + - required: [ clocks ] > + - properties: { clock-frequency: false } > > properties: > compatible: > -- > 2.49.0 >
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
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.
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: + - required: [ clock-frequency ] + - properties: { clocks: false } + - allOf: + - required: [ clocks ] + - properties: { clock-frequency: false } properties: compatible:
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. 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. 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(-)