Message ID | 20230928222130.580487-1-festevam@gmail.com |
---|---|
State | Superseded |
Headers | show |
Series | [v3] dt-bindings: thermal: qoriq-thermal: Adjust fsl,tmu-range min/maxItems | expand |
Hi Conor, On Mon, Oct 2, 2023 at 9:27 AM Conor Dooley <conor@kernel.org> wrote: > tbh, this seems like a situation where per compatible constraints should > be added, since each of the devices listed above has different > requirements. Ok, I am trying to add the device constraints as suggested. For example: I am trying to describe that imx93 has 7 items for fsl,tmu-range: --- a/Documentation/devicetree/bindings/thermal/qoriq-thermal.yaml +++ b/Documentation/devicetree/bindings/thermal/qoriq-thermal.yaml @@ -21,6 +21,7 @@ properties: enum: - fsl,qoriq-tmu - fsl,imx8mq-tmu + - fsl,imx93-tmu reg: maxItems: 1 @@ -33,7 +34,15 @@ properties: description: | The values to be programmed into TTRnCR, as specified by the SoC reference manual. The first cell is TTR0CR, the second is TTR1CR, etc. - maxItems: 4 + items: + - description: TTR0CR + - description: TTR1CR + - description: TTR2CR + - description: TTR3CR + - description: TTR4CR + - description: TTR5CR + - description: TTR6CR + minItems: 4 fsl,tmu-calibration: $ref: /schemas/types.yaml#/definitions/uint32-matrix @@ -69,15 +78,33 @@ required: - fsl,tmu-calibration - '#thermal-sensor-cells' + +allOf: + - if: + properties: + compatible: + contains: + enum: + - fsl,imx93-tmu + then: + properties: + fsl,tmu-range: + minItems: 7 + maxItems: 7 + else: + properties: + fsl,tmu-range: + maxItems: 4 + additionalProperties: false examples: - | tmu@f0000 { - compatible = "fsl,qoriq-tmu"; + compatible = "fsl,imx93-tmu"; reg = <0xf0000 0x1000>; interrupts = <18 2 0 0>; - fsl,tmu-range = <0x000a0000 0x00090026 0x0008004a 0x0001006a>; + fsl,tmu-range = <0x000a0000 0x00090026 0x0008004a 0x0001006a 0 0 0>; fsl,tmu-calibration = <0x00000000 0x00000025>, <0x00000001 0x00000028>, <0x00000002 0x0000002d>, but dt_binding_check fails: $ make dt_binding_check DT_SCHEMA_FILES=qoriq-thermal.yaml -j12 LINT Documentation/devicetree/bindings DTEX Documentation/devicetree/bindings/thermal/qoriq-thermal.example.dts CHKDT Documentation/devicetree/bindings/processed-schema.json SCHEMA Documentation/devicetree/bindings/processed-schema.json DTC_CHK Documentation/devicetree/bindings/thermal/qoriq-thermal.example.dtb /home/fabio/linux-next/Documentation/devicetree/bindings/thermal/qoriq-thermal.example.dtb: tmu@f0000: fsl,tmu-range: [[655360, 589862, 524362, 65642, 0, 0, 0]] is too short from schema $id: http://devicetree.org/schemas/thermal/qoriq-thermal.yaml# What is wrong with the yaml changes to tell that imx93 has 7 items for fsl,tmu-range? Thanks
On Sat, Dec 09, 2023 at 04:22:31PM -0300, Fabio Estevam wrote: > Hi Conor, > > On Mon, Oct 2, 2023 at 9:27 AM Conor Dooley <conor@kernel.org> wrote: > > > tbh, this seems like a situation where per compatible constraints should > > be added, since each of the devices listed above has different > > requirements. > > Ok, I am trying to add the device constraints as suggested. > > For example: I am trying to describe that imx93 has 7 items for fsl,tmu-range: > > --- a/Documentation/devicetree/bindings/thermal/qoriq-thermal.yaml > +++ b/Documentation/devicetree/bindings/thermal/qoriq-thermal.yaml > @@ -21,6 +21,7 @@ properties: > enum: > - fsl,qoriq-tmu > - fsl,imx8mq-tmu > + - fsl,imx93-tmu > > reg: > maxItems: 1 > @@ -33,7 +34,15 @@ properties: > description: | > The values to be programmed into TTRnCR, as specified by the SoC > reference manual. The first cell is TTR0CR, the second is TTR1CR, etc. > - maxItems: 4 > + items: > + - description: TTR0CR > + - description: TTR1CR > + - description: TTR2CR > + - description: TTR3CR > + - description: TTR4CR > + - description: TTR5CR > + - description: TTR6CR > + minItems: 4 > > fsl,tmu-calibration: > $ref: /schemas/types.yaml#/definitions/uint32-matrix > @@ -69,15 +78,33 @@ required: > - fsl,tmu-calibration > - '#thermal-sensor-cells' > > + ^^ extra newline here I think. > +allOf: > + - if: > + properties: > + compatible: > + contains: > + enum: > + - fsl,imx93-tmu > + then: > + properties: > + fsl,tmu-range: > + minItems: 7 > + maxItems: 7 > + else: > + properties: > + fsl,tmu-range: > + maxItems: 4 > + > additionalProperties: false > > examples: > - | > tmu@f0000 { > - compatible = "fsl,qoriq-tmu"; > + compatible = "fsl,imx93-tmu"; > reg = <0xf0000 0x1000>; > interrupts = <18 2 0 0>; > - fsl,tmu-range = <0x000a0000 0x00090026 0x0008004a 0x0001006a>; > + fsl,tmu-range = <0x000a0000 0x00090026 0x0008004a 0x0001006a 0 0 0>; > fsl,tmu-calibration = <0x00000000 0x00000025>, > <0x00000001 0x00000028>, > <0x00000002 0x0000002d>, > > but dt_binding_check fails: > > $ make dt_binding_check DT_SCHEMA_FILES=qoriq-thermal.yaml -j12 > LINT Documentation/devicetree/bindings > DTEX Documentation/devicetree/bindings/thermal/qoriq-thermal.example.dts > CHKDT Documentation/devicetree/bindings/processed-schema.json > SCHEMA Documentation/devicetree/bindings/processed-schema.json > DTC_CHK Documentation/devicetree/bindings/thermal/qoriq-thermal.example.dtb > /home/fabio/linux-next/Documentation/devicetree/bindings/thermal/qoriq-thermal.example.dtb: > tmu@f0000: fsl,tmu-range: [[655360, 589862, 524362, 65642, 0, 0, 0]] > is too short > from schema $id: http://devicetree.org/schemas/thermal/qoriq-thermal.yaml# > > What is wrong with the yaml changes to tell that imx93 has 7 items for > fsl,tmu-range? You're adding the constraints and items at the wrong level AFAICT. I think something like the below better matches your constraints? diff --git a/Documentation/devicetree/bindings/thermal/qoriq-thermal.yaml b/Documentation/devicetree/bindings/thermal/qoriq-thermal.yaml index 145744027234..540169806cc8 100644 --- a/Documentation/devicetree/bindings/thermal/qoriq-thermal.yaml +++ b/Documentation/devicetree/bindings/thermal/qoriq-thermal.yaml @@ -21,6 +21,7 @@ properties: enum: - fsl,qoriq-tmu - fsl,imx8mq-tmu + - fsl,imx93-tmu reg: maxItems: 1 @@ -33,7 +34,17 @@ properties: description: | The values to be programmed into TTRnCR, as specified by the SoC reference manual. The first cell is TTR0CR, the second is TTR1CR, etc. - maxItems: 4 + items: + items: + - description: TTR0CR + - description: TTR1CR + - description: TTR2CR + - description: TTR3CR + - description: TTR4CR + - description: TTR5CR + - description: TTR6CR + minItems: 4 + maxItems: 1 fsl,tmu-calibration: $ref: /schemas/types.yaml#/definitions/uint32-matrix @@ -69,15 +80,34 @@ required: - fsl,tmu-calibration - '#thermal-sensor-cells' + +allOf: + - if: + properties: + compatible: + contains: + enum: + - fsl,imx93-tmu + then: + properties: + fsl,tmu-range: + items: + minItems: 7 + else: + properties: + fsl,tmu-range: + items: + maxItems: 4 + additionalProperties: false examples: - | tmu@f0000 { - compatible = "fsl,qoriq-tmu"; + compatible = "fsl,imx93-tmu"; reg = <0xf0000 0x1000>; interrupts = <18 2 0 0>; - fsl,tmu-range = <0x000a0000 0x00090026 0x0008004a 0x0001006a>; + fsl,tmu-range = <0x000a0000 0x00090026 0x0008004a 0x0001006a 0 0 0>; fsl,tmu-calibration = <0x00000000 0x00000025>, <0x00000001 0x00000028>, <0x00000002 0x0000002d>,
On Sat, Dec 09, 2023 at 05:59:17PM -0300, Fabio Estevam wrote: > Hi Conor, > > On 09/12/2023 17:23, Conor Dooley wrote: > > > You're adding the constraints and items at the wrong level AFAICT. > > I think something like the below better matches your constraints? > > Thanks for your example. > > With your change the fsl,imx93-tmu case works correctly: > if I pass the number of fsl,tmu-range entries different than 7, > dt_binding_check correctly complains. > > However, if I pass 7 entries to fsl,qoriq-tmu it should complain as it > expects 4, but it btw, unrelated - minItems seems (from a grep) like it needs to be 2 not 4. > does not. > > On top of your patch: > > --- a/Documentation/devicetree/bindings/thermal/qoriq-thermal.yaml > +++ b/Documentation/devicetree/bindings/thermal/qoriq-thermal.yaml > @@ -104,7 +104,7 @@ additionalProperties: false > examples: > - | > tmu@f0000 { > - compatible = "fsl,imx93-tmu"; > + compatible = "fsl,qoriq-tmu"; > reg = <0xf0000 0x1000>; > interrupts = <18 2 0 0>; > fsl,tmu-range = <0x000a0000 0x00090026 0x0008004a 0x0001006a 0 0 > 0>; > > make dt_binding_check DT_SCHEMA_FILES=qoriq-thermal.yaml > LINT Documentation/devicetree/bindings > DTEX > Documentation/devicetree/bindings/thermal/qoriq-thermal.example.dts > CHKDT Documentation/devicetree/bindings/processed-schema.json > SCHEMA Documentation/devicetree/bindings/processed-schema.json > DTC_CHK > Documentation/devicetree/bindings/thermal/qoriq-thermal.example.dtb > > Any suggestions? I dunno. I spent far far longer than I would like to admit trying to fix this. Firstly my suggestion here is crap I think and only applies to ___matrices___. I think it needs to be the way you had it in your diff, but I cannot figure out why it doesn't apply the maxItems constraint. Perhaps Rob or Krzysztof can figure out what we were overlooking. The diff in question was something like: diff --git a/Documentation/devicetree/bindings/thermal/qoriq-thermal.yaml b/Documentation/devicetree/bindings/thermal/qoriq-thermal.yaml index 145744027234..50787938d605 100644 --- a/Documentation/devicetree/bindings/thermal/qoriq-thermal.yaml +++ b/Documentation/devicetree/bindings/thermal/qoriq-thermal.yaml @@ -21,6 +21,7 @@ properties: enum: - fsl,qoriq-tmu - fsl,imx8mq-tmu + - fsl,imx93-tmu reg: maxItems: 1 @@ -33,7 +34,15 @@ properties: description: | The values to be programmed into TTRnCR, as specified by the SoC reference manual. The first cell is TTR0CR, the second is TTR1CR, etc. - maxItems: 4 + minItems: 2 + items: + - description: TTR0CR + - description: TTR1CR + - description: TTR2CR + - description: TTR3CR + - description: TTR4CR + - description: TTR5CR + - description: TTR6CR fsl,tmu-calibration: $ref: /schemas/types.yaml#/definitions/uint32-matrix @@ -69,15 +78,30 @@ required: - fsl,tmu-calibration - '#thermal-sensor-cells' +allOf: + - if: + properties: + compatible: + contains: + const: fsl,imx93-tmu + then: + properties: + fsl,tmu-range: + minItems: 7 + else: + properties: + fsl,tmu-range: + maxItems: 4 + additionalProperties: false examples: - | tmu@f0000 { - compatible = "fsl,qoriq-tmu"; + compatible = "fsl,imx8mq-tmu"; reg = <0xf0000 0x1000>; interrupts = <18 2 0 0>; - fsl,tmu-range = <0x000a0000 0x00090026 0x0008004a 0x0001006a>; + fsl,tmu-range = <0x000a0000 0x00090026 0x0008004a 0x0001006a 0x0 0x0>; fsl,tmu-calibration = <0x00000000 0x00000025>, <0x00000001 0x00000028>, <0x00000002 0x0000002d>,
On Sun, Dec 10, 2023 at 02:52:45PM +0000, Conor Dooley wrote: > On Sat, Dec 09, 2023 at 05:59:17PM -0300, Fabio Estevam wrote: > > Hi Conor, > > > > On 09/12/2023 17:23, Conor Dooley wrote: > > > > > You're adding the constraints and items at the wrong level AFAICT. > > > I think something like the below better matches your constraints? > > > > Thanks for your example. > > > > With your change the fsl,imx93-tmu case works correctly: > > if I pass the number of fsl,tmu-range entries different than 7, > > dt_binding_check correctly complains. > > > > However, if I pass 7 entries to fsl,qoriq-tmu it should complain as it > > expects 4, but it > > btw, unrelated - minItems seems (from a grep) like it needs to be 2 not > 4. > > > does not. > > > > On top of your patch: > > > > --- a/Documentation/devicetree/bindings/thermal/qoriq-thermal.yaml > > +++ b/Documentation/devicetree/bindings/thermal/qoriq-thermal.yaml > > @@ -104,7 +104,7 @@ additionalProperties: false > > examples: > > - | > > tmu@f0000 { > > - compatible = "fsl,imx93-tmu"; > > + compatible = "fsl,qoriq-tmu"; > > reg = <0xf0000 0x1000>; > > interrupts = <18 2 0 0>; > > fsl,tmu-range = <0x000a0000 0x00090026 0x0008004a 0x0001006a 0 0 > > 0>; > > > > make dt_binding_check DT_SCHEMA_FILES=qoriq-thermal.yaml > > LINT Documentation/devicetree/bindings > > DTEX > > Documentation/devicetree/bindings/thermal/qoriq-thermal.example.dts > > CHKDT Documentation/devicetree/bindings/processed-schema.json > > SCHEMA Documentation/devicetree/bindings/processed-schema.json > > DTC_CHK > > Documentation/devicetree/bindings/thermal/qoriq-thermal.example.dtb > > > > Any suggestions? > > I dunno. I spent far far longer than I would like to admit trying to fix > this. Firstly my suggestion here is crap I think and only applies to > ___matrices___. I think it needs to be the way you had it in your diff, > but I cannot figure out why it doesn't apply the maxItems constraint. > > Perhaps Rob or Krzysztof can figure out what we were overlooking. > The diff in question was something like: I suspect something is going afoul in the fixups. Will look at it tomorrow. Rob
On Sun, Dec 10, 2023 at 09:37:49AM -0600, Rob Herring wrote: > On Sun, Dec 10, 2023 at 02:52:45PM +0000, Conor Dooley wrote: > > On Sat, Dec 09, 2023 at 05:59:17PM -0300, Fabio Estevam wrote: > > > Hi Conor, > > > > > > On 09/12/2023 17:23, Conor Dooley wrote: > > > > > > > You're adding the constraints and items at the wrong level AFAICT. > > > > I think something like the below better matches your constraints? > > > > > > Thanks for your example. > > > > > > With your change the fsl,imx93-tmu case works correctly: > > > if I pass the number of fsl,tmu-range entries different than 7, > > > dt_binding_check correctly complains. > > > > > > However, if I pass 7 entries to fsl,qoriq-tmu it should complain as it > > > expects 4, but it > > > > btw, unrelated - minItems seems (from a grep) like it needs to be 2 not > > 4. > > > > > does not. > > > > > > On top of your patch: > > > > > > --- a/Documentation/devicetree/bindings/thermal/qoriq-thermal.yaml > > > +++ b/Documentation/devicetree/bindings/thermal/qoriq-thermal.yaml > > > @@ -104,7 +104,7 @@ additionalProperties: false > > > examples: > > > - | > > > tmu@f0000 { > > > - compatible = "fsl,imx93-tmu"; > > > + compatible = "fsl,qoriq-tmu"; > > > reg = <0xf0000 0x1000>; > > > interrupts = <18 2 0 0>; > > > fsl,tmu-range = <0x000a0000 0x00090026 0x0008004a 0x0001006a 0 0 > > > 0>; > > > > > > make dt_binding_check DT_SCHEMA_FILES=qoriq-thermal.yaml > > > LINT Documentation/devicetree/bindings > > > DTEX > > > Documentation/devicetree/bindings/thermal/qoriq-thermal.example.dts > > > CHKDT Documentation/devicetree/bindings/processed-schema.json > > > SCHEMA Documentation/devicetree/bindings/processed-schema.json > > > DTC_CHK > > > Documentation/devicetree/bindings/thermal/qoriq-thermal.example.dtb > > > > > > Any suggestions? > > > > I dunno. I spent far far longer than I would like to admit trying to fix > > this. Firstly my suggestion here is crap I think and only applies to > > ___matrices___. I think it needs to be the way you had it in your diff, > > but I cannot figure out why it doesn't apply the maxItems constraint. > > > > Perhaps Rob or Krzysztof can figure out what we were overlooking. > > The diff in question was something like: > > I suspect something is going afoul in the fixups. Will look at it > tomorrow. Did you manage to figure out what was causing this btw?
On Thu, Dec 14, 2023 at 10:13 AM Conor Dooley <conor@kernel.org> wrote: > > On Sun, Dec 10, 2023 at 09:37:49AM -0600, Rob Herring wrote: > > On Sun, Dec 10, 2023 at 02:52:45PM +0000, Conor Dooley wrote: > > > On Sat, Dec 09, 2023 at 05:59:17PM -0300, Fabio Estevam wrote: > > > > Hi Conor, > > > > > > > > On 09/12/2023 17:23, Conor Dooley wrote: > > > > > > > > > You're adding the constraints and items at the wrong level AFAICT. > > > > > I think something like the below better matches your constraints? > > > > > > > > Thanks for your example. > > > > > > > > With your change the fsl,imx93-tmu case works correctly: > > > > if I pass the number of fsl,tmu-range entries different than 7, > > > > dt_binding_check correctly complains. > > > > > > > > However, if I pass 7 entries to fsl,qoriq-tmu it should complain as it > > > > expects 4, but it > > > > > > btw, unrelated - minItems seems (from a grep) like it needs to be 2 not > > > 4. > > > > > > > does not. > > > > > > > > On top of your patch: > > > > > > > > --- a/Documentation/devicetree/bindings/thermal/qoriq-thermal.yaml > > > > +++ b/Documentation/devicetree/bindings/thermal/qoriq-thermal.yaml > > > > @@ -104,7 +104,7 @@ additionalProperties: false > > > > examples: > > > > - | > > > > tmu@f0000 { > > > > - compatible = "fsl,imx93-tmu"; > > > > + compatible = "fsl,qoriq-tmu"; > > > > reg = <0xf0000 0x1000>; > > > > interrupts = <18 2 0 0>; > > > > fsl,tmu-range = <0x000a0000 0x00090026 0x0008004a 0x0001006a 0 0 > > > > 0>; > > > > > > > > make dt_binding_check DT_SCHEMA_FILES=qoriq-thermal.yaml > > > > LINT Documentation/devicetree/bindings > > > > DTEX > > > > Documentation/devicetree/bindings/thermal/qoriq-thermal.example.dts > > > > CHKDT Documentation/devicetree/bindings/processed-schema.json > > > > SCHEMA Documentation/devicetree/bindings/processed-schema.json > > > > DTC_CHK > > > > Documentation/devicetree/bindings/thermal/qoriq-thermal.example.dtb > > > > > > > > Any suggestions? > > > > > > I dunno. I spent far far longer than I would like to admit trying to fix > > > this. Firstly my suggestion here is crap I think and only applies to > > > ___matrices___. I think it needs to be the way you had it in your diff, > > > but I cannot figure out why it doesn't apply the maxItems constraint. > > > > > > Perhaps Rob or Krzysztof can figure out what we were overlooking. > > > The diff in question was something like: > > > > I suspect something is going afoul in the fixups. Will look at it > > tomorrow. > > Did you manage to figure out what was causing this btw? Yes. There's not really an immediate fix I see. The issue is in the if/then schemas we don't have enough information to know the type of fsl,tmu-range. To work correctly, it needs to be transformed to: fsl,tmu-range: items: minItems: 7 maxItems: 7 This goes back to everything gets encoded into a 2 dim matrix, but the schemas try to hide this encoding. My plan here is to eventually drop doing that and decode properties to their correct type. That will drop a lot of the fixups. I have patches to do that, but then it has other corner cases. So short term, I'd just leave things such that they don't warn or just drop the conditional. Rob
On Wed, Jan 3, 2024 at 6:39 PM Rob Herring <robh@kernel.org> wrote: > Yes. There's not really an immediate fix I see. The issue is in the > if/then schemas we don't have enough information to know the type of > fsl,tmu-range. To work correctly, it needs to be transformed to: > > fsl,tmu-range: > items: > minItems: 7 > maxItems: 7 Is this a typo? minItems cannot be 7. - lx2160a has two fsl,tmu-range entries - imx8mq has four fsl,tmu-range entries. > This goes back to everything gets encoded into a 2 dim matrix, but the > schemas try to hide this encoding. My plan here is to eventually drop > doing that and decode properties to their correct type. That will drop > a lot of the fixups. I have patches to do that, but then it has other > corner cases. > > So short term, I'd just leave things such that they don't warn or just > drop the conditional. Is my v3 proposal good then? https://patchwork.kernel.org/project/linux-pm/patch/20230928222130.580487-1-festevam@gmail.com/ It fixes all fsl,tmu-range warnings
diff --git a/Documentation/devicetree/bindings/thermal/qoriq-thermal.yaml b/Documentation/devicetree/bindings/thermal/qoriq-thermal.yaml index ee4780f186f9..60b9d79e7543 100644 --- a/Documentation/devicetree/bindings/thermal/qoriq-thermal.yaml +++ b/Documentation/devicetree/bindings/thermal/qoriq-thermal.yaml @@ -36,7 +36,8 @@ properties: description: | The values to be programmed into TTRnCR, as specified by the SoC reference manual. The first cell is TTR0CR, the second is TTR1CR, etc. - minItems: 4 + minItems: 2 + maxItems: 7 fsl,tmu-calibration: $ref: /schemas/types.yaml#/definitions/uint32-matrix