diff mbox series

[v3,1/2] dt-bindings: pwm: Add Xilinx AXI Timer

Message ID 20210511191239.774570-1-sean.anderson@seco.com (mailing list archive)
State New, archived
Headers show
Series [v3,1/2] dt-bindings: pwm: Add Xilinx AXI Timer | expand

Commit Message

Sean Anderson May 11, 2021, 7:12 p.m. UTC
This adds a binding for the Xilinx LogiCORE IP AXI Timer. This device is
a "soft" block, so it has many parameters which would not be
configurable in most hardware. This binding is usually automatically
generated by Xilinx's tools, so the names and values of some properties
must be kept as they are. Replacement properties have been provided for
new device trees.

Because we need to init timer devices so early in boot, the easiest way
to configure things is to use a device tree property. For the moment
this is 'xlnx,pwm', but this could be extended/renamed/etc. in the
future if these is a need for a generic property.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---
How should the clocking situation be documented? For the moment I have
just left clock as optional, but should clock-frequency be documented?

Changes in v3:
- Mark all boolean-as-int properties as deprecated
- Add xlnx,pwm and xlnx,gen?-active-low properties.
- Make newer replacement properties mutually-exclusive with what they
  replace
- Add an example with non-deprecated properties only.

Changes in v2:
- Use 32-bit addresses for example binding

 .../bindings/pwm/xlnx,axi-timer.yaml          | 142 ++++++++++++++++++
 1 file changed, 142 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/xlnx,axi-timer.yaml

Comments

Rob Herring (Arm) May 12, 2021, 6:35 p.m. UTC | #1
On Tue, 11 May 2021 15:12:37 -0400, Sean Anderson wrote:
> This adds a binding for the Xilinx LogiCORE IP AXI Timer. This device is
> a "soft" block, so it has many parameters which would not be
> configurable in most hardware. This binding is usually automatically
> generated by Xilinx's tools, so the names and values of some properties
> must be kept as they are. Replacement properties have been provided for
> new device trees.
> 
> Because we need to init timer devices so early in boot, the easiest way
> to configure things is to use a device tree property. For the moment
> this is 'xlnx,pwm', but this could be extended/renamed/etc. in the
> future if these is a need for a generic property.
> 
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> ---
> How should the clocking situation be documented? For the moment I have
> just left clock as optional, but should clock-frequency be documented?
> 
> Changes in v3:
> - Mark all boolean-as-int properties as deprecated
> - Add xlnx,pwm and xlnx,gen?-active-low properties.
> - Make newer replacement properties mutually-exclusive with what they
>   replace
> - Add an example with non-deprecated properties only.
> 
> Changes in v2:
> - Use 32-bit addresses for example binding
> 
>  .../bindings/pwm/xlnx,axi-timer.yaml          | 142 ++++++++++++++++++
>  1 file changed, 142 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/xlnx,axi-timer.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:
./Documentation/devicetree/bindings/pwm/xlnx,axi-timer.yaml:16:9: [warning] wrong indentation: expected 10 but found 8 (indentation)

dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/pwm/xlnx,axi-timer.example.dts:49.37-57.11: ERROR (duplicate_label): /example-1/timer@800e0000: Duplicate label 'axi_timer_0' on /example-1/timer@800e0000 and /example-0/timer@800e0000
ERROR: Input tree has errors, aborting (use -f to force output)
make[1]: *** [scripts/Makefile.lib:380: Documentation/devicetree/bindings/pwm/xlnx,axi-timer.example.dt.yaml] Error 2
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:1416: dt_binding_check] Error 2

See https://patchwork.ozlabs.org/patch/1477288

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.
Rob Herring (Arm) May 13, 2021, 2:16 a.m. UTC | #2
On Tue, May 11, 2021 at 03:12:37PM -0400, Sean Anderson wrote:
> This adds a binding for the Xilinx LogiCORE IP AXI Timer. This device is
> a "soft" block, so it has many parameters which would not be
> configurable in most hardware. This binding is usually automatically
> generated by Xilinx's tools, so the names and values of some properties
> must be kept as they are. Replacement properties have been provided for
> new device trees.

Because you have some tool generating properties is not a reason we have 
to accept them upstream. 'deprecated' is for what *we* have deprecated.

In this case, I don't really see the point in defining new properties 
just to have bool.

> 
> Because we need to init timer devices so early in boot, the easiest way
> to configure things is to use a device tree property. For the moment
> this is 'xlnx,pwm', but this could be extended/renamed/etc. in the
> future if these is a need for a generic property.

No...

> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> ---
> How should the clocking situation be documented? For the moment I have
> just left clock as optional, but should clock-frequency be documented?
> 
> Changes in v3:
> - Mark all boolean-as-int properties as deprecated
> - Add xlnx,pwm and xlnx,gen?-active-low properties.
> - Make newer replacement properties mutually-exclusive with what they
>   replace
> - Add an example with non-deprecated properties only.
> 
> Changes in v2:
> - Use 32-bit addresses for example binding
> 
>  .../bindings/pwm/xlnx,axi-timer.yaml          | 142 ++++++++++++++++++
>  1 file changed, 142 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/xlnx,axi-timer.yaml
> 
> diff --git a/Documentation/devicetree/bindings/pwm/xlnx,axi-timer.yaml b/Documentation/devicetree/bindings/pwm/xlnx,axi-timer.yaml
> new file mode 100644
> index 000000000000..a5e90658e31a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/xlnx,axi-timer.yaml
> @@ -0,0 +1,142 @@
> +# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pwm/xlnx,axi-timer.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Xilinx LogiCORE IP AXI Timer Device Tree Binding
> +
> +maintainers:
> +  - Sean Anderson <sean.anderson@seco.com>
> +
> +properties:
> +  compatible:
> +    oneOf:
> +      - items:
> +        - const: xlnx,axi-timer-2.0
> +        - const: xlnx,xps-timer-1.00.a
> +      - const: xlnx,xps-timer-1.00.a
> +
> +  clocks:
> +    maxItems: 1
> +
> +  clock-names:
> +    const: s_axi_aclk
> +
> +  reg:
> +    maxItems: 1
> +
> +  xlnx,count-width:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 8
> +    maximum: 32
> +    description:
> +      The width of the counter(s), in bits.
> +
> +  xlnx,gen0-assert:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [ 0, 1 ]
> +    default: 1
> +    deprecated: true
> +    description:
> +      The polarity of the generateout0 signal. 0 for active-low, 1 for active-high.
> +
> +  xlnx,gen0-active-low:
> +    $ref: /schemas/types.yaml#/definitions/flag
> +    description:
> +      The generate0 signal is active-low.
> +
> +  xlnx,gen1-assert:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [ 0, 1 ]
> +    default: 1
> +    deprecated: true
> +    description:
> +      The polarity of the generateout1 signal. 0 for active-low, 1 for active-high.
> +
> +  xlnx,gen1-active-low:
> +    $ref: /schemas/types.yaml#/definitions/flag
> +    description:
> +      The generate1 signal is active-low.
> +
> +  xlnx,one-timer-only:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [ 0, 1 ]
> +    deprecated: true
> +    description:
> +      Whether only one timer is present in this block.
> +
> +  xlnx,single-timer:
> +    $ref: /schemas/types.yaml#/definitions/flag
> +    description:
> +      Only one timer is present in this block.
> +
> +  xlnx,pwm:
> +    $ref: /schemas/types.yaml#/definitions/flag
> +    description:
> +      This timer should be configured as a PWM.

If a PWM, perhaps you want a '#pwm-cells' property which can serve as 
the hint to configure as a PWM.

> +
> +required:
> +  - compatible
> +  - reg
> +  - xlnx,count-width
> +
> +allOf:
> +  - if:
> +      required:
> +        - clocks
> +    then:
> +      required:
> +        - clock-names
> +
> +  - if:
> +      required:
> +        - xlnx,gen0-active-low
> +    then:
> +      not:
> +        required:
> +          - xlnx,gen0-assert
> +
> +  - if:
> +      required:
> +        - xlnx,gen0-active-low
> +    then:
> +      not:
> +        required:
> +          - xlnx,gen0-assert
> +
> +  - if:
> +      required:
> +        - xlnx,one-timer-only
> +    then:
> +      not:
> +        required:
> +          - xlnx,single-timer
> +
> +additionalProperties: true
> +
> +examples:
> +  - |
> +    axi_timer_0: timer@800e0000 {
> +        clock-names = "s_axi_aclk";
> +        clocks = <&zynqmp_clk 71>;
> +        compatible = "xlnx,axi-timer-2.0", "xlnx,xps-timer-1.00.a";
> +        reg = <0x800e0000 0x10000>;
> +        xlnx,count-width = <0x20>;
> +        xlnx,gen0-assert = <0x1>;
> +        xlnx,gen1-assert = <0x1>;
> +        xlnx,one-timer-only = <0x0>;
> +        xlnx,trig0-assert = <0x1>;
> +        xlnx,trig1-assert = <0x1>;
> +    };
> +
> +  - |
> +    axi_timer_0: timer@800e0000 {
> +        clock-names = "s_axi_aclk";
> +        clocks = <&zynqmp_clk 71>;
> +        compatible = "xlnx,axi-timer-2.0", "xlnx,xps-timer-1.00.a";
> +        reg = <0x800e0000 0x10000>;
> +        xlnx,count-width = <0x20>;
> +        xlnx,gen0-active-low;
> +        xlnx,single-timer;
> +    };
> -- 
> 2.25.1
>
Sean Anderson May 13, 2021, 2:33 p.m. UTC | #3
On 5/12/21 10:16 PM, Rob Herring wrote:
 > On Tue, May 11, 2021 at 03:12:37PM -0400, Sean Anderson wrote:
 >> This adds a binding for the Xilinx LogiCORE IP AXI Timer. This device is
 >> a "soft" block, so it has many parameters which would not be
 >> configurable in most hardware. This binding is usually automatically
 >> generated by Xilinx's tools, so the names and values of some properties
 >> must be kept as they are. Replacement properties have been provided for
 >> new device trees.
 >
 > Because you have some tool generating properties is not a reason we have
 > to accept them upstream.

These properties are already in arch/microblaze/boot/dts/system.dts and
in the devicetree supplied to Linux by qemu. Removing these properties
will break existing setups, which I would like to avoid.

 > 'deprecated' is for what *we* have deprecated.

Ok. I will remove that then.

 >
 > In this case, I don't really see the point in defining new properties
 > just to have bool.

I don't either, but it was requested, by Michal...

 >
 >>
 >> Because we need to init timer devices so early in boot, the easiest way
 >> to configure things is to use a device tree property. For the moment
 >> this is 'xlnx,pwm', but this could be extended/renamed/etc. in the
 >> future if these is a need for a generic property.
 >
 > No...
 >
 >> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
 >> ---
 >> How should the clocking situation be documented? For the moment I have
 >> just left clock as optional, but should clock-frequency be documented?
 >>
 >> Changes in v3:
 >> - Mark all boolean-as-int properties as deprecated
 >> - Add xlnx,pwm and xlnx,gen?-active-low properties.
 >> - Make newer replacement properties mutually-exclusive with what they
 >>    replace
 >> - Add an example with non-deprecated properties only.
 >>
 >> Changes in v2:
 >> - Use 32-bit addresses for example binding
 >>
 >>   .../bindings/pwm/xlnx,axi-timer.yaml          | 142 ++++++++++++++++++
 >>   1 file changed, 142 insertions(+)
 >>   create mode 100644 Documentation/devicetree/bindings/pwm/xlnx,axi-timer.yaml
 >>
 >> diff --git a/Documentation/devicetree/bindings/pwm/xlnx,axi-timer.yaml b/Documentation/devicetree/bindings/pwm/xlnx,axi-timer.yaml
 >> new file mode 100644
 >> index 000000000000..a5e90658e31a
 >> --- /dev/null
 >> +++ b/Documentation/devicetree/bindings/pwm/xlnx,axi-timer.yaml
 >> @@ -0,0 +1,142 @@
 >> +# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
 >> +%YAML 1.2
 >> +---
 >> +$id: http://devicetree.org/schemas/pwm/xlnx,axi-timer.yaml#
 >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
 >> +
 >> +title: Xilinx LogiCORE IP AXI Timer Device Tree Binding
 >> +
 >> +maintainers:
 >> +  - Sean Anderson <sean.anderson@seco.com>
 >> +
 >> +properties:
 >> +  compatible:
 >> +    oneOf:
 >> +      - items:
 >> +        - const: xlnx,axi-timer-2.0
 >> +        - const: xlnx,xps-timer-1.00.a
 >> +      - const: xlnx,xps-timer-1.00.a
 >> +
 >> +  clocks:
 >> +    maxItems: 1
 >> +
 >> +  clock-names:
 >> +    const: s_axi_aclk
 >> +
 >> +  reg:
 >> +    maxItems: 1
 >> +
 >> +  xlnx,count-width:
 >> +    $ref: /schemas/types.yaml#/definitions/uint32
 >> +    minimum: 8
 >> +    maximum: 32
 >> +    description:
 >> +      The width of the counter(s), in bits.
 >> +
 >> +  xlnx,gen0-assert:
 >> +    $ref: /schemas/types.yaml#/definitions/uint32
 >> +    enum: [ 0, 1 ]
 >> +    default: 1
 >> +    deprecated: true
 >> +    description:
 >> +      The polarity of the generateout0 signal. 0 for active-low, 1 for active-high.
 >> +
 >> +  xlnx,gen0-active-low:
 >> +    $ref: /schemas/types.yaml#/definitions/flag
 >> +    description:
 >> +      The generate0 signal is active-low.
 >> +
 >> +  xlnx,gen1-assert:
 >> +    $ref: /schemas/types.yaml#/definitions/uint32
 >> +    enum: [ 0, 1 ]
 >> +    default: 1
 >> +    deprecated: true
 >> +    description:
 >> +      The polarity of the generateout1 signal. 0 for active-low, 1 for active-high.
 >> +
 >> +  xlnx,gen1-active-low:
 >> +    $ref: /schemas/types.yaml#/definitions/flag
 >> +    description:
 >> +      The generate1 signal is active-low.
 >> +
 >> +  xlnx,one-timer-only:
 >> +    $ref: /schemas/types.yaml#/definitions/uint32
 >> +    enum: [ 0, 1 ]
 >> +    deprecated: true
 >> +    description:
 >> +      Whether only one timer is present in this block.
 >> +
 >> +  xlnx,single-timer:
 >> +    $ref: /schemas/types.yaml#/definitions/flag
 >> +    description:
 >> +      Only one timer is present in this block.
 >> +
 >> +  xlnx,pwm:
 >> +    $ref: /schemas/types.yaml#/definitions/flag
 >> +    description:
 >> +      This timer should be configured as a PWM.
 >
 > If a PWM, perhaps you want a '#pwm-cells' property which can serve as
 > the hint to configure as a PWM.

Ok, that's a good idea.

--Sean

 >
 >> +
 >> +required:
 >> +  - compatible
 >> +  - reg
 >> +  - xlnx,count-width
 >> +
 >> +allOf:
 >> +  - if:
 >> +      required:
 >> +        - clocks
 >> +    then:
 >> +      required:
 >> +        - clock-names
 >> +
 >> +  - if:
 >> +      required:
 >> +        - xlnx,gen0-active-low
 >> +    then:
 >> +      not:
 >> +        required:
 >> +          - xlnx,gen0-assert
 >> +
 >> +  - if:
 >> +      required:
 >> +        - xlnx,gen0-active-low
 >> +    then:
 >> +      not:
 >> +        required:
 >> +          - xlnx,gen0-assert
 >> +
 >> +  - if:
 >> +      required:
 >> +        - xlnx,one-timer-only
 >> +    then:
 >> +      not:
 >> +        required:
 >> +          - xlnx,single-timer
 >> +
 >> +additionalProperties: true
 >> +
 >> +examples:
 >> +  - |
 >> +    axi_timer_0: timer@800e0000 {
 >> +        clock-names = "s_axi_aclk";
 >> +        clocks = <&zynqmp_clk 71>;
 >> +        compatible = "xlnx,axi-timer-2.0", "xlnx,xps-timer-1.00.a";
 >> +        reg = <0x800e0000 0x10000>;
 >> +        xlnx,count-width = <0x20>;
 >> +        xlnx,gen0-assert = <0x1>;
 >> +        xlnx,gen1-assert = <0x1>;
 >> +        xlnx,one-timer-only = <0x0>;
 >> +        xlnx,trig0-assert = <0x1>;
 >> +        xlnx,trig1-assert = <0x1>;
 >> +    };
 >> +
 >> +  - |
 >> +    axi_timer_0: timer@800e0000 {
 >> +        clock-names = "s_axi_aclk";
 >> +        clocks = <&zynqmp_clk 71>;
 >> +        compatible = "xlnx,axi-timer-2.0", "xlnx,xps-timer-1.00.a";
 >> +        reg = <0x800e0000 0x10000>;
 >> +        xlnx,count-width = <0x20>;
 >> +        xlnx,gen0-active-low;
 >> +        xlnx,single-timer;
 >> +    };
 >> --
 >> 2.25.1
 >>
Sean Anderson May 13, 2021, 3:28 p.m. UTC | #4
On 5/13/21 10:33 AM, Sean Anderson wrote:
 >
 >
 > On 5/12/21 10:16 PM, Rob Herring wrote:
 >  > On Tue, May 11, 2021 at 03:12:37PM -0400, Sean Anderson wrote:
 >  >> This adds a binding for the Xilinx LogiCORE IP AXI Timer. This device is
 >  >> a "soft" block, so it has many parameters which would not be
 >  >> configurable in most hardware. This binding is usually automatically
 >  >> generated by Xilinx's tools, so the names and values of some properties
 >  >> must be kept as they are. Replacement properties have been provided for
 >  >> new device trees.
 >  >
 >  > Because you have some tool generating properties is not a reason we have
 >  > to accept them upstream.
 >
 > These properties are already in arch/microblaze/boot/dts/system.dts and
 > in the devicetree supplied to Linux by qemu. Removing these properties
 > will break existing setups, which I would like to avoid.
 >
 >  > 'deprecated' is for what *we* have deprecated.
 >
 > Ok. I will remove that then.
 >
 >  >
 >  > In this case, I don't really see the point in defining new properties
 >  > just to have bool.
 >
 > I don't either, but it was requested, by Michal...

Err, your comment on the original bindings was

 > Can't all these be boolean?

And Michal commented

 > I think in this case you should described what it is used by current
 > driver in Microblaze and these options are required. The rest are by
 > design optional.
 > If you want to change them to different value then current binding
 > should be deprecated and have any transition time with code alignment.

So that is what I tried to accomplish with this revision. I also tried
allowing something like

	xlnx,one-timer-only = <0>; /* two timers */
	xlnx,one-timer-only = <1>; /* one timer  */
	xlnx,one-timer-only; /* one timer */
	/* property absent means two timers */

but I was unable to figure out how to express this with json-schema. I
don't think it's the best design either...

--Sean

 >
 >  >
 >  >>
 >  >> Because we need to init timer devices so early in boot, the easiest way
 >  >> to configure things is to use a device tree property. For the moment
 >  >> this is 'xlnx,pwm', but this could be extended/renamed/etc. in the
 >  >> future if these is a need for a generic property.
 >  >
 >  > No...
 >  >
 >  >> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
 >  >> ---
 >  >> How should the clocking situation be documented? For the moment I have
 >  >> just left clock as optional, but should clock-frequency be documented?
 >  >>
 >  >> Changes in v3:
 >  >> - Mark all boolean-as-int properties as deprecated
 >  >> - Add xlnx,pwm and xlnx,gen?-active-low properties.
 >  >> - Make newer replacement properties mutually-exclusive with what they
 >  >>    replace
 >  >> - Add an example with non-deprecated properties only.
 >  >>
 >  >> Changes in v2:
 >  >> - Use 32-bit addresses for example binding
 >  >>
 >  >>   .../bindings/pwm/xlnx,axi-timer.yaml          | 142 ++++++++++++++++++
 >  >>   1 file changed, 142 insertions(+)
 >  >>   create mode 100644 Documentation/devicetree/bindings/pwm/xlnx,axi-timer.yaml
 >  >>
 >  >> diff --git a/Documentation/devicetree/bindings/pwm/xlnx,axi-timer.yaml b/Documentation/devicetree/bindings/pwm/xlnx,axi-timer.yaml
 >  >> new file mode 100644
 >  >> index 000000000000..a5e90658e31a
 >  >> --- /dev/null
 >  >> +++ b/Documentation/devicetree/bindings/pwm/xlnx,axi-timer.yaml
 >  >> @@ -0,0 +1,142 @@
 >  >> +# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
 >  >> +%YAML 1.2
 >  >> +---
 >  >> +$id: http://devicetree.org/schemas/pwm/xlnx,axi-timer.yaml#
 >  >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
 >  >> +
 >  >> +title: Xilinx LogiCORE IP AXI Timer Device Tree Binding
 >  >> +
 >  >> +maintainers:
 >  >> +  - Sean Anderson <sean.anderson@seco.com>
 >  >> +
 >  >> +properties:
 >  >> +  compatible:
 >  >> +    oneOf:
 >  >> +      - items:
 >  >> +        - const: xlnx,axi-timer-2.0
 >  >> +        - const: xlnx,xps-timer-1.00.a
 >  >> +      - const: xlnx,xps-timer-1.00.a
 >  >> +
 >  >> +  clocks:
 >  >> +    maxItems: 1
 >  >> +
 >  >> +  clock-names:
 >  >> +    const: s_axi_aclk
 >  >> +
 >  >> +  reg:
 >  >> +    maxItems: 1
 >  >> +
 >  >> +  xlnx,count-width:
 >  >> +    $ref: /schemas/types.yaml#/definitions/uint32
 >  >> +    minimum: 8
 >  >> +    maximum: 32
 >  >> +    description:
 >  >> +      The width of the counter(s), in bits.
 >  >> +
 >  >> +  xlnx,gen0-assert:
 >  >> +    $ref: /schemas/types.yaml#/definitions/uint32
 >  >> +    enum: [ 0, 1 ]
 >  >> +    default: 1
 >  >> +    deprecated: true
 >  >> +    description:
 >  >> +      The polarity of the generateout0 signal. 0 for active-low, 1 for active-high.
 >  >> +
 >  >> +  xlnx,gen0-active-low:
 >  >> +    $ref: /schemas/types.yaml#/definitions/flag
 >  >> +    description:
 >  >> +      The generate0 signal is active-low.
 >  >> +
 >  >> +  xlnx,gen1-assert:
 >  >> +    $ref: /schemas/types.yaml#/definitions/uint32
 >  >> +    enum: [ 0, 1 ]
 >  >> +    default: 1
 >  >> +    deprecated: true
 >  >> +    description:
 >  >> +      The polarity of the generateout1 signal. 0 for active-low, 1 for active-high.
 >  >> +
 >  >> +  xlnx,gen1-active-low:
 >  >> +    $ref: /schemas/types.yaml#/definitions/flag
 >  >> +    description:
 >  >> +      The generate1 signal is active-low.
 >  >> +
 >  >> +  xlnx,one-timer-only:
 >  >> +    $ref: /schemas/types.yaml#/definitions/uint32
 >  >> +    enum: [ 0, 1 ]
 >  >> +    deprecated: true
 >  >> +    description:
 >  >> +      Whether only one timer is present in this block.
 >  >> +
 >  >> +  xlnx,single-timer:
 >  >> +    $ref: /schemas/types.yaml#/definitions/flag
 >  >> +    description:
 >  >> +      Only one timer is present in this block.
 >  >> +
 >  >> +  xlnx,pwm:
 >  >> +    $ref: /schemas/types.yaml#/definitions/flag
 >  >> +    description:
 >  >> +      This timer should be configured as a PWM.
 >  >
 >  > If a PWM, perhaps you want a '#pwm-cells' property which can serve as
 >  > the hint to configure as a PWM.
 >
 > Ok, that's a good idea.
 >
 > --Sean
 >
 >  >
 >  >> +
 >  >> +required:
 >  >> +  - compatible
 >  >> +  - reg
 >  >> +  - xlnx,count-width
 >  >> +
 >  >> +allOf:
 >  >> +  - if:
 >  >> +      required:
 >  >> +        - clocks
 >  >> +    then:
 >  >> +      required:
 >  >> +        - clock-names
 >  >> +
 >  >> +  - if:
 >  >> +      required:
 >  >> +        - xlnx,gen0-active-low
 >  >> +    then:
 >  >> +      not:
 >  >> +        required:
 >  >> +          - xlnx,gen0-assert
 >  >> +
 >  >> +  - if:
 >  >> +      required:
 >  >> +        - xlnx,gen0-active-low
 >  >> +    then:
 >  >> +      not:
 >  >> +        required:
 >  >> +          - xlnx,gen0-assert
 >  >> +
 >  >> +  - if:
 >  >> +      required:
 >  >> +        - xlnx,one-timer-only
 >  >> +    then:
 >  >> +      not:
 >  >> +        required:
 >  >> +          - xlnx,single-timer
 >  >> +
 >  >> +additionalProperties: true
 >  >> +
 >  >> +examples:
 >  >> +  - |
 >  >> +    axi_timer_0: timer@800e0000 {
 >  >> +        clock-names = "s_axi_aclk";
 >  >> +        clocks = <&zynqmp_clk 71>;
 >  >> +        compatible = "xlnx,axi-timer-2.0", "xlnx,xps-timer-1.00.a";
 >  >> +        reg = <0x800e0000 0x10000>;
 >  >> +        xlnx,count-width = <0x20>;
 >  >> +        xlnx,gen0-assert = <0x1>;
 >  >> +        xlnx,gen1-assert = <0x1>;
 >  >> +        xlnx,one-timer-only = <0x0>;
 >  >> +        xlnx,trig0-assert = <0x1>;
 >  >> +        xlnx,trig1-assert = <0x1>;
 >  >> +    };
 >  >> +
 >  >> +  - |
 >  >> +    axi_timer_0: timer@800e0000 {
 >  >> +        clock-names = "s_axi_aclk";
 >  >> +        clocks = <&zynqmp_clk 71>;
 >  >> +        compatible = "xlnx,axi-timer-2.0", "xlnx,xps-timer-1.00.a";
 >  >> +        reg = <0x800e0000 0x10000>;
 >  >> +        xlnx,count-width = <0x20>;
 >  >> +        xlnx,gen0-active-low;
 >  >> +        xlnx,single-timer;
 >  >> +    };
 >  >> --
 >  >> 2.25.1
 >  >>
Rob Herring (Arm) May 13, 2021, 8:43 p.m. UTC | #5
On Thu, May 13, 2021 at 10:28 AM Sean Anderson <sean.anderson@seco.com> wrote:
>
>
>
> On 5/13/21 10:33 AM, Sean Anderson wrote:
>  >
>  >
>  > On 5/12/21 10:16 PM, Rob Herring wrote:
>  >  > On Tue, May 11, 2021 at 03:12:37PM -0400, Sean Anderson wrote:
>  >  >> This adds a binding for the Xilinx LogiCORE IP AXI Timer. This device is
>  >  >> a "soft" block, so it has many parameters which would not be
>  >  >> configurable in most hardware. This binding is usually automatically
>  >  >> generated by Xilinx's tools, so the names and values of some properties
>  >  >> must be kept as they are. Replacement properties have been provided for
>  >  >> new device trees.
>  >  >
>  >  > Because you have some tool generating properties is not a reason we have
>  >  > to accept them upstream.
>  >
>  > These properties are already in arch/microblaze/boot/dts/system.dts and
>  > in the devicetree supplied to Linux by qemu. Removing these properties
>  > will break existing setups, which I would like to avoid.

Already in use in upstream dts files is different than just
'automatically generated' by vendor tools.

>  >
>  >  > 'deprecated' is for what *we* have deprecated.
>  >
>  > Ok. I will remove that then.
>  >
>  >  >
>  >  > In this case, I don't really see the point in defining new properties
>  >  > just to have bool.
>  >
>  > I don't either, but it was requested, by Michal...
>
> Err, your comment on the original bindings was
>
>  > Can't all these be boolean?

With no other context, yes that's what I would ask. Now you've given
me some context, between using the existing ones and 2 sets of
properties to maintain, I choose the former.

> And Michal commented
>
>  > I think in this case you should described what it is used by current
>  > driver in Microblaze and these options are required. The rest are by
>  > design optional.
>  > If you want to change them to different value then current binding
>  > should be deprecated and have any transition time with code alignment.
>
> So that is what I tried to accomplish with this revision. I also tried
> allowing something like
>
>         xlnx,one-timer-only = <0>; /* two timers */
>         xlnx,one-timer-only = <1>; /* one timer  */
>         xlnx,one-timer-only; /* one timer */
>         /* property absent means two timers */
>
> but I was unable to figure out how to express this with json-schema. I
> don't think it's the best design either...

json-schema would certainly let you, but generally we don't want
properties to have more than 1 type.

Rob
Sean Anderson May 13, 2021, 9:01 p.m. UTC | #6
On 5/13/21 4:43 PM, Rob Herring wrote:
 > On Thu, May 13, 2021 at 10:28 AM Sean Anderson <sean.anderson@seco.com> wrote:
 >>
 >>
 >>
 >> On 5/13/21 10:33 AM, Sean Anderson wrote:
 >>   >
 >>   >
 >>   > On 5/12/21 10:16 PM, Rob Herring wrote:
 >>   >  > On Tue, May 11, 2021 at 03:12:37PM -0400, Sean Anderson wrote:
 >>   >  >> This adds a binding for the Xilinx LogiCORE IP AXI Timer. This device is
 >>   >  >> a "soft" block, so it has many parameters which would not be
 >>   >  >> configurable in most hardware. This binding is usually automatically
 >>   >  >> generated by Xilinx's tools, so the names and values of some properties
 >>   >  >> must be kept as they are. Replacement properties have been provided for
 >>   >  >> new device trees.
 >>   >  >
 >>   >  > Because you have some tool generating properties is not a reason we have
 >>   >  > to accept them upstream.
 >>   >
 >>   > These properties are already in arch/microblaze/boot/dts/system.dts and
 >>   > in the devicetree supplied to Linux by qemu. Removing these properties
 >>   > will break existing setups, which I would like to avoid.
 >
 > Already in use in upstream dts files is different than just
 > 'automatically generated' by vendor tools.
 >
 >>   >
 >>   >  > 'deprecated' is for what *we* have deprecated.
 >>   >
 >>   > Ok. I will remove that then.
 >>   >
 >>   >  >
 >>   >  > In this case, I don't really see the point in defining new properties
 >>   >  > just to have bool.
 >>   >
 >>   > I don't either, but it was requested, by Michal...
 >>
 >> Err, your comment on the original bindings was
 >>
 >>   > Can't all these be boolean?
 >
 > With no other context, yes that's what I would ask. Now you've given
 > me some context, between using the existing ones and 2 sets of
 > properties to maintain, I choose the former.

Ok, then I will go with that for v4.

I noticed some another patch regarding a similar situation [1].  Would
you prefer separate files like what is done there, or is a unified file
like this ok?

[1] https://lore.kernel.org/lkml/d36e3690ce8c5a1e53d054552e4fd8b90d6a5478.1620648868.git.geert+renesas@glider.be/T/

--Sean

 >
 >> And Michal commented
 >>
 >>   > I think in this case you should described what it is used by current
 >>   > driver in Microblaze and these options are required. The rest are by
 >>   > design optional.
 >>   > If you want to change them to different value then current binding
 >>   > should be deprecated and have any transition time with code alignment.
 >>
 >> So that is what I tried to accomplish with this revision. I also tried
 >> allowing something like
 >>
 >>          xlnx,one-timer-only = <0>; /* two timers */
 >>          xlnx,one-timer-only = <1>; /* one timer  */
 >>          xlnx,one-timer-only; /* one timer */
 >>          /* property absent means two timers */
 >>
 >> but I was unable to figure out how to express this with json-schema. I
 >> don't think it's the best design either...
 >
 > json-schema would certainly let you, but generally we don't want
 > properties to have more than 1 type.
 >
 > Rob
 >
Michal Simek May 14, 2021, 8:50 a.m. UTC | #7
On 5/13/21 10:43 PM, Rob Herring wrote:
> On Thu, May 13, 2021 at 10:28 AM Sean Anderson <sean.anderson@seco.com> wrote:
>>
>>
>>
>> On 5/13/21 10:33 AM, Sean Anderson wrote:
>>  >
>>  >
>>  > On 5/12/21 10:16 PM, Rob Herring wrote:
>>  >  > On Tue, May 11, 2021 at 03:12:37PM -0400, Sean Anderson wrote:
>>  >  >> This adds a binding for the Xilinx LogiCORE IP AXI Timer. This device is
>>  >  >> a "soft" block, so it has many parameters which would not be
>>  >  >> configurable in most hardware. This binding is usually automatically
>>  >  >> generated by Xilinx's tools, so the names and values of some properties
>>  >  >> must be kept as they are. Replacement properties have been provided for
>>  >  >> new device trees.
>>  >  >
>>  >  > Because you have some tool generating properties is not a reason we have
>>  >  > to accept them upstream.
>>  >
>>  > These properties are already in arch/microblaze/boot/dts/system.dts and
>>  > in the devicetree supplied to Linux by qemu. Removing these properties
>>  > will break existing setups, which I would like to avoid.
> 
> Already in use in upstream dts files is different than just
> 'automatically generated' by vendor tools.
> 
>>  >
>>  >  > 'deprecated' is for what *we* have deprecated.
>>  >
>>  > Ok. I will remove that then.
>>  >
>>  >  >
>>  >  > In this case, I don't really see the point in defining new properties
>>  >  > just to have bool.
>>  >
>>  > I don't either, but it was requested, by Michal...
>>
>> Err, your comment on the original bindings was
>>
>>  > Can't all these be boolean?
> 
> With no other context, yes that's what I would ask. Now you've given
> me some context, between using the existing ones and 2 sets of
> properties to maintain, I choose the former.
> 
>> And Michal commented
>>
>>  > I think in this case you should described what it is used by current
>>  > driver in Microblaze and these options are required. The rest are by
>>  > design optional.
>>  > If you want to change them to different value then current binding
>>  > should be deprecated and have any transition time with code alignment.
>>
>> So that is what I tried to accomplish with this revision. I also tried
>> allowing something like
>>
>>         xlnx,one-timer-only = <0>; /* two timers */
>>         xlnx,one-timer-only = <1>; /* one timer  */
>>         xlnx,one-timer-only; /* one timer */
>>         /* property absent means two timers */
>>
>> but I was unable to figure out how to express this with json-schema. I
>> don't think it's the best design either...
> 
> json-schema would certainly let you, but generally we don't want
> properties to have more than 1 type.

One thing is what it is in system.dts file which was committed in 2009
and there are just small alignments there. But none is really using it.
Maybe I should just delete it.
And this version was generated by Xilinx ancient tools at that time. All
parameters there are fully describing HW and they are not changing. Only
new one can be added.

From the current microblaze code you can see which properties are really
used.

reg
interrupts
xlnx,one-timer-only
clocks
clock-frequency

It means from my point of view these should be listed in the binding.
clock-frequency is optional by code when clock is defined.

All other properties listed in system.dts are from my perspective
optional and that's how it should be.

I think DT binding patch should reflect this state as patch itself.
And then PWM should be added on the top as separate patch.

Note: In past we were using only parameters and name we got from tools
but over years we were fine to use for example bool properties and we
just aligned Xilinx device tree generator to match it. That's why not a
problem to deprecate any property and move to new one. Xilinx DTG is
already prepared for it and it is easy to remap it.

Thanks,
Michal
Sean Anderson May 14, 2021, 5:13 p.m. UTC | #8
On 5/14/21 4:50 AM, Michal Simek wrote:
 >
 >
 > On 5/13/21 10:43 PM, Rob Herring wrote:
 >> On Thu, May 13, 2021 at 10:28 AM Sean Anderson <sean.anderson@seco.com> wrote:
 >>>
 >>>
 >>>
 >>> On 5/13/21 10:33 AM, Sean Anderson wrote:
 >>>   >
 >>>   >
 >>>   > On 5/12/21 10:16 PM, Rob Herring wrote:
 >>>   >  > On Tue, May 11, 2021 at 03:12:37PM -0400, Sean Anderson wrote:
 >>>   >  >> This adds a binding for the Xilinx LogiCORE IP AXI Timer. This device is
 >>>   >  >> a "soft" block, so it has many parameters which would not be
 >>>   >  >> configurable in most hardware. This binding is usually automatically
 >>>   >  >> generated by Xilinx's tools, so the names and values of some properties
 >>>   >  >> must be kept as they are. Replacement properties have been provided for
 >>>   >  >> new device trees.
 >>>   >  >
 >>>   >  > Because you have some tool generating properties is not a reason we have
 >>>   >  > to accept them upstream.
 >>>   >
 >>>   > These properties are already in arch/microblaze/boot/dts/system.dts and
 >>>   > in the devicetree supplied to Linux by qemu. Removing these properties
 >>>   > will break existing setups, which I would like to avoid.
 >>
 >> Already in use in upstream dts files is different than just
 >> 'automatically generated' by vendor tools.
 >>
 >>>   >
 >>>   >  > 'deprecated' is for what *we* have deprecated.
 >>>   >
 >>>   > Ok. I will remove that then.
 >>>   >
 >>>   >  >
 >>>   >  > In this case, I don't really see the point in defining new properties
 >>>   >  > just to have bool.
 >>>   >
 >>>   > I don't either, but it was requested, by Michal...
 >>>
 >>> Err, your comment on the original bindings was
 >>>
 >>>   > Can't all these be boolean?
 >>
 >> With no other context, yes that's what I would ask. Now you've given
 >> me some context, between using the existing ones and 2 sets of
 >> properties to maintain, I choose the former.
 >>
 >>> And Michal commented
 >>>
 >>>   > I think in this case you should described what it is used by current
 >>>   > driver in Microblaze and these options are required. The rest are by
 >>>   > design optional.
 >>>   > If you want to change them to different value then current binding
 >>>   > should be deprecated and have any transition time with code alignment.
 >>>
 >>> So that is what I tried to accomplish with this revision. I also tried
 >>> allowing something like
 >>>
 >>>          xlnx,one-timer-only = <0>; /* two timers */
 >>>          xlnx,one-timer-only = <1>; /* one timer  */
 >>>          xlnx,one-timer-only; /* one timer */
 >>>          /* property absent means two timers */
 >>>
 >>> but I was unable to figure out how to express this with json-schema. I
 >>> don't think it's the best design either...
 >>
 >> json-schema would certainly let you, but generally we don't want
 >> properties to have more than 1 type.
 >
 > One thing is what it is in system.dts file which was committed in 2009
 > and there are just small alignments there. But none is really using it.
 > Maybe I should just delete it.
 > And this version was generated by Xilinx ancient tools at that time. All
 > parameters there are fully describing HW and they are not changing. Only
 > new one can be added.
 >
 >  From the current microblaze code you can see which properties are really
 > used.
 >
 > reg
 > interrupts
 > xlnx,one-timer-only
 > clocks
 > clock-frequency

There is also an implicit dependency on xlnx,count-width. Several times
the existing driver assumes the counter width is 32, but this should
instead be discovered from the devicetree.

 > It means from my point of view these should be listed in the binding.
 > clock-frequency is optional by code when clock is defined.
 >
 > All other properties listed in system.dts are from my perspective
 > optional and that's how it should be.

Here is the situation as I understand it

* This device has existed for around 15 years (since 2006)
* Because it is a soft device, there are several configurable parameters
* Although all of these parameters must be known for a complete
   implementation of this device, some are unnecessary if onlu reduced
   functionality is needed.
* A de facto devicetree binding for this device has existed for at least
   12 years (since 2009), but likely for as long as the device itself has
   existed. This binding has not changed substantially during this time.
* This binding is present in devicetrees from the Linux kernel, from
   qemu, in other existing systems, and in devicetrees generated by
   Xilinx's toolset.
* Because the existing driver for this device does not implement all
   functionality for this device, not all properties in the devicetree
   binding are used. In fact, there is (as noted above) one property
   which should be in use but is not because the current driver
   (implicitly) does not support some hardware configurations.
* To support additional functionality, it is necessary to
   use hardware parameters which were not previously necessary.

Based on the above, we can classify the properties of this binding into
several categories.

* Those which are currently read by the driver.
   * compatible
   * reg
   * clocks
   * clock-frequency
   * interrupts
   * xlnx,one-timer-only

* Those which reflect hardware parameters which are currently explicitly
   or implicitly relied upon by the driver.
   * reg
   * clocks
   * clock-frequency
   * interrupts
   * xlnx,counter-width
   * xlnx,one-timer-only

* Those which are currently present in device trees.
   * compatible
   * reg
   * interrupts
   * clocks
   * clock-frequency
   * xlnx,count-width
   * xlnx,one-timer-only
   * xlnx,trig0-assert
   * xlnx,trig1-assert
   * xlnx,gen0-assert
   * xlnx,gen1-assert

When choosing what properties to use, we must consider what the impact
of our changes will be on not just the kernel but also on existing users
of this binding:

* To use properties currently present in device trees, we just need to
   modify the kernel driver.
* To add additional properties (such as e.g. '#pwm-cells'), we must
   modify the kernel driver. In addition, users who would like to use
   these new properties must add them to their device trees. This may be
   done in a mechanical way using e.g. overlays.
* To deprecate existing properties and introduce new properties to
   expose the same underlying hardware parameters, we must modify the
   kernel driver. However, this has a large impact on existing users.
   They must modify their tools to generate this information in a
   different format. When this information is generated by upstream tools
   this may require updating a core part of their build system. For many
   projects, this may happen very infrequently because of the risk that
   such an upgrade will break things. Even if you suggest that Xilinx can
   easily modify its tools to generate any sort of output, the time for
   this upgrade to be deployed/adopted may be significantly longer.

Note that while all three types of changes are similar from a kernel
point of view, the impact on existing users is much large in the latter
case. For this reason, I think that wherever possible we should use
properties which are already present in existing device trees.

 > I think DT binding patch should reflect this state as patch itself.
 > And then PWM should be added on the top as separate patch.

I have no preference here.

--Sean

 >
 > Note: In past we were using only parameters and name we got from tools
 > but over years we were fine to use for example bool properties and we
 > just aligned Xilinx device tree generator to match it. That's why not a
 > problem to deprecate any property and move to new one. Xilinx DTG is
 > already prepared for it and it is easy to remap it.
 >
 > Thanks,
 > Michal
Michal Simek May 17, 2021, 8:28 a.m. UTC | #9
On 5/14/21 7:13 PM, Sean Anderson wrote:
> 
> 
> On 5/14/21 4:50 AM, Michal Simek wrote:
>>
>>
>> On 5/13/21 10:43 PM, Rob Herring wrote:
>>> On Thu, May 13, 2021 at 10:28 AM Sean Anderson
> <sean.anderson@seco.com> wrote:
>>>>
>>>>
>>>>
>>>> On 5/13/21 10:33 AM, Sean Anderson wrote:
>>>>   >
>>>>   >
>>>>   > On 5/12/21 10:16 PM, Rob Herring wrote:
>>>>   >  > On Tue, May 11, 2021 at 03:12:37PM -0400, Sean Anderson wrote:
>>>>   >  >> This adds a binding for the Xilinx LogiCORE IP AXI Timer.
> This device is
>>>>   >  >> a "soft" block, so it has many parameters which would not be
>>>>   >  >> configurable in most hardware. This binding is usually
> automatically
>>>>   >  >> generated by Xilinx's tools, so the names and values of some
> properties
>>>>   >  >> must be kept as they are. Replacement properties have been
> provided for
>>>>   >  >> new device trees.
>>>>   >  >
>>>>   >  > Because you have some tool generating properties is not a
> reason we have
>>>>   >  > to accept them upstream.
>>>>   >
>>>>   > These properties are already in
> arch/microblaze/boot/dts/system.dts and
>>>>   > in the devicetree supplied to Linux by qemu. Removing these
> properties
>>>>   > will break existing setups, which I would like to avoid.
>>>
>>> Already in use in upstream dts files is different than just
>>> 'automatically generated' by vendor tools.
>>>
>>>>   >
>>>>   >  > 'deprecated' is for what *we* have deprecated.
>>>>   >
>>>>   > Ok. I will remove that then.
>>>>   >
>>>>   >  >
>>>>   >  > In this case, I don't really see the point in defining new
> properties
>>>>   >  > just to have bool.
>>>>   >
>>>>   > I don't either, but it was requested, by Michal...
>>>>
>>>> Err, your comment on the original bindings was
>>>>
>>>>   > Can't all these be boolean?
>>>
>>> With no other context, yes that's what I would ask. Now you've given
>>> me some context, between using the existing ones and 2 sets of
>>> properties to maintain, I choose the former.
>>>
>>>> And Michal commented
>>>>
>>>>   > I think in this case you should described what it is used by
> current
>>>>   > driver in Microblaze and these options are required. The rest
> are by
>>>>   > design optional.
>>>>   > If you want to change them to different value then current binding
>>>>   > should be deprecated and have any transition time with code
> alignment.
>>>>
>>>> So that is what I tried to accomplish with this revision. I also tried
>>>> allowing something like
>>>>
>>>>          xlnx,one-timer-only = <0>; /* two timers */
>>>>          xlnx,one-timer-only = <1>; /* one timer  */
>>>>          xlnx,one-timer-only; /* one timer */
>>>>          /* property absent means two timers */
>>>>
>>>> but I was unable to figure out how to express this with json-schema. I
>>>> don't think it's the best design either...
>>>
>>> json-schema would certainly let you, but generally we don't want
>>> properties to have more than 1 type.
>>
>> One thing is what it is in system.dts file which was committed in 2009
>> and there are just small alignments there. But none is really using it.
>> Maybe I should just delete it.
>> And this version was generated by Xilinx ancient tools at that time. All
>> parameters there are fully describing HW and they are not changing. Only
>> new one can be added.
>>
>>  From the current microblaze code you can see which properties are really
>> used.
>>
>> reg
>> interrupts
>> xlnx,one-timer-only
>> clocks
>> clock-frequency
> 
> There is also an implicit dependency on xlnx,count-width. Several times
> the existing driver assumes the counter width is 32, but this should
> instead be discovered from the devicetree.

For me it is important what it is used now. Which is not
xlnx,count-width. That's why if you want to add it you can as optional
property.

> 
>> It means from my point of view these should be listed in the binding.
>> clock-frequency is optional by code when clock is defined.
>>
>> All other properties listed in system.dts are from my perspective
>> optional and that's how it should be.
> 
> Here is the situation as I understand it
> 
> * This device has existed for around 15 years (since 2006)
> * Because it is a soft device, there are several configurable parameters
> * Although all of these parameters must be known for a complete
>   implementation of this device, some are unnecessary if onlu reduced
>   functionality is needed.
> * A de facto devicetree binding for this device has existed for at least
>   12 years (since 2009), but likely for as long as the device itself has
>   existed. This binding has not changed substantially during this time.

note: IP itself is even much older.

> * This binding is present in devicetrees from the Linux kernel, from
>   qemu, in other existing systems, and in devicetrees generated by
>   Xilinx's toolset.

Only from Linux. Qemu is trying to reuse the same properties but it can
also add own one. They are trying to be aligned as much as possible but
there are a lot of cases where Qemu requires much more information. (I
am not saying in this timer case but in general).


> * Because the existing driver for this device does not implement all
>   functionality for this device, not all properties in the devicetree
>   binding are used. In fact, there is (as noted above) one property
>   which should be in use but is not because the current driver
>   (implicitly) does not support some hardware configurations.
> * To support additional functionality, it is necessary to
>   use hardware parameters which were not previously necessary.
> 
> Based on the above, we can classify the properties of this binding into
> several categories.
> 
> * Those which are currently read by the driver.
>   * compatible
>   * reg
>   * clocks
>   * clock-frequency
>   * interrupts
>   * xlnx,one-timer-only
> 
> * Those which reflect hardware parameters which are currently explicitly
>   or implicitly relied upon by the driver.
>   * reg
>   * clocks
>   * clock-frequency
>   * interrupts
>   * xlnx,counter-width
>   * xlnx,one-timer-only
> 
> * Those which are currently present in device trees.
>   * compatible
>   * reg
>   * interrupts
>   * clocks
>   * clock-frequency
>   * xlnx,count-width
>   * xlnx,one-timer-only
>   * xlnx,trig0-assert
>   * xlnx,trig1-assert
>   * xlnx,gen0-assert
>   * xlnx,gen1-assert
> 
> When choosing what properties to use, we must consider what the impact
> of our changes will be on not just the kernel but also on existing users
> of this binding:

I don't think that this is valid. Rob is asking for adding #pwm-cells
which is purely Linux binding. We also don't know what properties are
used by others projects not just Linux or Qemu. Also required properties
in Linux doesn't need to be required in U-Boot for example even we are
trying to aligned all of them. Another case are others RTOSes, etc.


> * To use properties currently present in device trees, we just need to
>   modify the kernel driver.
> * To add additional properties (such as e.g. '#pwm-cells'), we must
>   modify the kernel driver. In addition, users who would like to use
>   these new properties must add them to their device trees. This may be
>   done in a mechanical way using e.g. overlays.
> * To deprecate existing properties and introduce new properties to
>   expose the same underlying hardware parameters, we must modify the
>   kernel driver. However, this has a large impact on existing users.
>   They must modify their tools to generate this information in a
>   different format. When this information is generated by upstream tools
>   this may require updating a core part of their build system. For many
>   projects, this may happen very infrequently because of the risk that
>   such an upgrade will break things. Even if you suggest that Xilinx can
>   easily modify its tools to generate any sort of output, the time for
>   this upgrade to be deployed/adopted may be significantly longer.

From Xilinx perspective it would be ideal to use only properties which
fully describe HW in the form how they are generated today. They are
stable for a lot of years and as I said only new one are added.
But this alignment wasn't accepted long time ago and we have been asked
to start to align these properties with similar HW done by others.
And truth is that in a lot of cases there is clear 1:1 mapping and
generic properties can be simply use. This mapping ends in Xilinx device
tree generator.
Back to your point. Required properties are required by Linux driver
only. This driver is around for quite a long time where certain policies
haven't been setup/used/enforced (Microblaze is 2nd architecture which
started to use device tree).
We should create DT binding doc at that time but in 2009 it wasn't
standard practice. In 2007 Grant was adding support for Xilinx PPC
platform also without any DT binding document too.

That's why we need to review current unwritten DT binding based on code
requirements and look at it how to fix it (if needed) and then add PWM
support on the top of it.
If something needs to be deprecated, let's deprecate it and have
transition time for a year or so to adapt to it.

Rob knows much better than I how this should be handled.

Based on your list:
  * compatible
  * reg
  * clocks
  * clock-frequency
  * interrupts
  * xlnx,one-timer-only

all of these are required property in new DT binding.

xlnx,counter-width is optional if you want to use.
#pwm-cells is optional for enabling PWM support (I would expect that
when this property is present this timer don't need be used as
clocksource/clockevent by Microblaze)
etc

And for properties which are generated out of Xilinx tools I would allow
in DT binding to add others optional properties that DT won't error out
if they are seen.

Thanks,
Michal
Sean Anderson May 17, 2021, 2:40 p.m. UTC | #10
On 5/17/21 4:28 AM, Michal Simek wrote:
 >
 >
 > On 5/14/21 7:13 PM, Sean Anderson wrote:
 >>
 >>
 >> On 5/14/21 4:50 AM, Michal Simek wrote:
 >>>
 >>>
 >>> On 5/13/21 10:43 PM, Rob Herring wrote:
 >>>> On Thu, May 13, 2021 at 10:28 AM Sean Anderson
 >> <sean.anderson@seco.com> wrote:
 >>>>>
 >>>>>
 >>>>>
 >>>>> On 5/13/21 10:33 AM, Sean Anderson wrote:
 >>>>>     >
 >>>>>     >
 >>>>>     > On 5/12/21 10:16 PM, Rob Herring wrote:
 >>>>>     >  > On Tue, May 11, 2021 at 03:12:37PM -0400, Sean Anderson wrote:
 >>>>>     >  >> This adds a binding for the Xilinx LogiCORE IP AXI Timer.
 >> This device is
 >>>>>     >  >> a "soft" block, so it has many parameters which would not be
 >>>>>     >  >> configurable in most hardware. This binding is usually
 >> automatically
 >>>>>     >  >> generated by Xilinx's tools, so the names and values of some
 >> properties
 >>>>>     >  >> must be kept as they are. Replacement properties have been
 >> provided for
 >>>>>     >  >> new device trees.
 >>>>>     >  >
 >>>>>     >  > Because you have some tool generating properties is not a
 >> reason we have
 >>>>>     >  > to accept them upstream.
 >>>>>     >
 >>>>>     > These properties are already in
 >> arch/microblaze/boot/dts/system.dts and
 >>>>>     > in the devicetree supplied to Linux by qemu. Removing these
 >> properties
 >>>>>     > will break existing setups, which I would like to avoid.
 >>>>
 >>>> Already in use in upstream dts files is different than just
 >>>> 'automatically generated' by vendor tools.
 >>>>
 >>>>>     >
 >>>>>     >  > 'deprecated' is for what *we* have deprecated.
 >>>>>     >
 >>>>>     > Ok. I will remove that then.
 >>>>>     >
 >>>>>     >  >
 >>>>>     >  > In this case, I don't really see the point in defining new
 >> properties
 >>>>>     >  > just to have bool.
 >>>>>     >
 >>>>>     > I don't either, but it was requested, by Michal...
 >>>>>
 >>>>> Err, your comment on the original bindings was
 >>>>>
 >>>>>     > Can't all these be boolean?
 >>>>
 >>>> With no other context, yes that's what I would ask. Now you've given
 >>>> me some context, between using the existing ones and 2 sets of
 >>>> properties to maintain, I choose the former.
 >>>>
 >>>>> And Michal commented
 >>>>>
 >>>>>     > I think in this case you should described what it is used by
 >> current
 >>>>>     > driver in Microblaze and these options are required. The rest
 >> are by
 >>>>>     > design optional.
 >>>>>     > If you want to change them to different value then current binding
 >>>>>     > should be deprecated and have any transition time with code
 >> alignment.
 >>>>>
 >>>>> So that is what I tried to accomplish with this revision. I also tried
 >>>>> allowing something like
 >>>>>
 >>>>>            xlnx,one-timer-only = <0>; /* two timers */
 >>>>>            xlnx,one-timer-only = <1>; /* one timer  */
 >>>>>            xlnx,one-timer-only; /* one timer */
 >>>>>            /* property absent means two timers */
 >>>>>
 >>>>> but I was unable to figure out how to express this with json-schema. I
 >>>>> don't think it's the best design either...
 >>>>
 >>>> json-schema would certainly let you, but generally we don't want
 >>>> properties to have more than 1 type.
 >>>
 >>> One thing is what it is in system.dts file which was committed in 2009
 >>> and there are just small alignments there. But none is really using it.
 >>> Maybe I should just delete it.
 >>> And this version was generated by Xilinx ancient tools at that time. All
 >>> parameters there are fully describing HW and they are not changing. Only
 >>> new one can be added.
 >>>
 >>>    From the current microblaze code you can see which properties are really
 >>> used.
 >>>
 >>> reg
 >>> interrupts
 >>> xlnx,one-timer-only
 >>> clocks
 >>> clock-frequency
 >>
 >> There is also an implicit dependency on xlnx,count-width. Several times
 >> the existing driver assumes the counter width is 32, but this should
 >> instead be discovered from the devicetree.
 >
 > For me it is important what it is used now. Which is not
 > xlnx,count-width. That's why if you want to add it you can as optional
 > property.

At the very least we should sanity check it. E.g. check that it is 32
and return -EINVAL if it is not.

 >
 >>
 >>> It means from my point of view these should be listed in the binding.
 >>> clock-frequency is optional by code when clock is defined.
 >>>
 >>> All other properties listed in system.dts are from my perspective
 >>> optional and that's how it should be.
 >>
 >> Here is the situation as I understand it
 >>
 >> * This device has existed for around 15 years (since 2006)
 >> * Because it is a soft device, there are several configurable parameters
 >> * Although all of these parameters must be known for a complete
 >>    implementation of this device, some are unnecessary if onlu reduced
 >>    functionality is needed.
 >> * A de facto devicetree binding for this device has existed for at least
 >>    12 years (since 2009), but likely for as long as the device itself has
 >>    existed. This binding has not changed substantially during this time.
 >
 > note: IP itself is even much older.
 >
 >> * This binding is present in devicetrees from the Linux kernel, from
 >>    qemu, in other existing systems, and in devicetrees generated by
 >>    Xilinx's toolset.
 >
 > Only from Linux. Qemu is trying to reuse the same properties but it can
 > also add own one. They are trying to be aligned as much as possible but
 > there are a lot of cases where Qemu requires much more information. (I
 > am not saying in this timer case but in general).
 >
 >
 >> * Because the existing driver for this device does not implement all
 >>    functionality for this device, not all properties in the devicetree
 >>    binding are used. In fact, there is (as noted above) one property
 >>    which should be in use but is not because the current driver
 >>    (implicitly) does not support some hardware configurations.
 >> * To support additional functionality, it is necessary to
 >>    use hardware parameters which were not previously necessary.
 >>
 >> Based on the above, we can classify the properties of this binding into
 >> several categories.
 >>
 >> * Those which are currently read by the driver.
 >>    * compatible
 >>    * reg
 >>    * clocks
 >>    * clock-frequency
 >>    * interrupts
 >>    * xlnx,one-timer-only
 >>
 >> * Those which reflect hardware parameters which are currently explicitly
 >>    or implicitly relied upon by the driver.
 >>    * reg
 >>    * clocks
 >>    * clock-frequency
 >>    * interrupts
 >>    * xlnx,counter-width
 >>    * xlnx,one-timer-only
 >>
 >> * Those which are currently present in device trees.
 >>    * compatible
 >>    * reg
 >>    * interrupts
 >>    * clocks
 >>    * clock-frequency
 >>    * xlnx,count-width
 >>    * xlnx,one-timer-only
 >>    * xlnx,trig0-assert
 >>    * xlnx,trig1-assert
 >>    * xlnx,gen0-assert
 >>    * xlnx,gen1-assert
 >>
 >> When choosing what properties to use, we must consider what the impact
 >> of our changes will be on not just the kernel but also on existing users
 >> of this binding:
 >
 > I don't think that this is valid. Rob is asking for adding #pwm-cells
 > which is purely Linux binding. We also don't know what properties are
 > used by others projects not just Linux or Qemu. Also required properties
 > in Linux doesn't need to be required in U-Boot for example even we are
 > trying to aligned all of them. Another case are others RTOSes, etc.

Here I do not see a way around this. Any way we do it we will need to
have some new binding. However, as noted below, adding a new binding for
configuration is easier than exposing properties in new ways.

 >> * To use properties currently present in device trees, we just need to
 >>    modify the kernel driver.
 >> * To add additional properties (such as e.g. '#pwm-cells'), we must
 >>    modify the kernel driver. In addition, users who would like to use
 >>    these new properties must add them to their device trees. This may be
 >>    done in a mechanical way using e.g. overlays.
 >> * To deprecate existing properties and introduce new properties to
 >>    expose the same underlying hardware parameters, we must modify the
 >>    kernel driver. However, this has a large impact on existing users.
 >>    They must modify their tools to generate this information in a
 >>    different format. When this information is generated by upstream tools
 >>    this may require updating a core part of their build system. For many
 >>    projects, this may happen very infrequently because of the risk that
 >>    such an upgrade will break things. Even if you suggest that Xilinx can
 >>    easily modify its tools to generate any sort of output, the time for
 >>    this upgrade to be deployed/adopted may be significantly longer.
 >
 >  From Xilinx perspective it would be ideal to use only properties which
 > fully describe HW in the form how they are generated today. They are
 > stable for a lot of years and as I said only new one are added.
 > But this alignment wasn't accepted long time ago and we have been asked
 > to start to align these properties with similar HW done by others.
 > And truth is that in a lot of cases there is clear 1:1 mapping and
 > generic properties can be simply use. This mapping ends in Xilinx device
 > tree generator.
 > Back to your point. Required properties are required by Linux driver
 > only. This driver is around for quite a long time where certain policies
 > haven't been setup/used/enforced (Microblaze is 2nd architecture which
 > started to use device tree).
 > We should create DT binding doc at that time but in 2009 it wasn't
 > standard practice. In 2007 Grant was adding support for Xilinx PPC
 > platform also without any DT binding document too.
 >
 > That's why we need to review current unwritten DT binding based on code
 > requirements and look at it how to fix it (if needed) and then add PWM
 > support on the top of it.
 > If something needs to be deprecated, let's deprecate it and have
 > transition time for a year or so to adapt to it.
 >
 > Rob knows much better than I how this should be handled.
 >
 > Based on your list:
 >    * compatible
 >    * reg
 >    * clocks
 >    * clock-frequency
 >    * interrupts
 >    * xlnx,one-timer-only
 >
 > all of these are required property in new DT binding.
 >
 > xlnx,counter-width is optional if you want to use.

Again, for the existing driver this should at least be sanity-checked so
we fail noisily instead of buggily.

This is the situation for the generate polarity as well. The driver will
work fine, but you will not have PWM output and there will be no
indication why. This is why I think we should start with supporting the
existing output. This is used to ensure the device will work as
configured. We can also add support for different properties exposing
the same information, but to support only new properties is not very
useful.

--Sean

 > #pwm-cells is optional for enabling PWM support (I would expect that
 > when this property is present this timer don't need be used as
 > clocksource/clockevent by Microblaze)
 > etc
 >
 > And for properties which are generated out of Xilinx tools I would allow
 > in DT binding to add others optional properties that DT won't error out
 > if they are seen.
 >
 > Thanks,
 > Michal
 >
Michal Simek May 17, 2021, 2:49 p.m. UTC | #11
On 5/17/21 4:40 PM, Sean Anderson wrote:
> 
> 
> On 5/17/21 4:28 AM, Michal Simek wrote:
>>
>>
>> On 5/14/21 7:13 PM, Sean Anderson wrote:
>>>
>>>
>>> On 5/14/21 4:50 AM, Michal Simek wrote:
>>>>
>>>>
>>>> On 5/13/21 10:43 PM, Rob Herring wrote:
>>>>> On Thu, May 13, 2021 at 10:28 AM Sean Anderson
>>> <sean.anderson@seco.com> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 5/13/21 10:33 AM, Sean Anderson wrote:
>>>>>>     >
>>>>>>     >
>>>>>>     > On 5/12/21 10:16 PM, Rob Herring wrote:
>>>>>>     >  > On Tue, May 11, 2021 at 03:12:37PM -0400, Sean Anderson
> wrote:
>>>>>>     >  >> This adds a binding for the Xilinx LogiCORE IP AXI Timer.
>>> This device is
>>>>>>     >  >> a "soft" block, so it has many parameters which would
> not be
>>>>>>     >  >> configurable in most hardware. This binding is usually
>>> automatically
>>>>>>     >  >> generated by Xilinx's tools, so the names and values of
> some
>>> properties
>>>>>>     >  >> must be kept as they are. Replacement properties have been
>>> provided for
>>>>>>     >  >> new device trees.
>>>>>>     >  >
>>>>>>     >  > Because you have some tool generating properties is not a
>>> reason we have
>>>>>>     >  > to accept them upstream.
>>>>>>     >
>>>>>>     > These properties are already in
>>> arch/microblaze/boot/dts/system.dts and
>>>>>>     > in the devicetree supplied to Linux by qemu. Removing these
>>> properties
>>>>>>     > will break existing setups, which I would like to avoid.
>>>>>
>>>>> Already in use in upstream dts files is different than just
>>>>> 'automatically generated' by vendor tools.
>>>>>
>>>>>>     >
>>>>>>     >  > 'deprecated' is for what *we* have deprecated.
>>>>>>     >
>>>>>>     > Ok. I will remove that then.
>>>>>>     >
>>>>>>     >  >
>>>>>>     >  > In this case, I don't really see the point in defining new
>>> properties
>>>>>>     >  > just to have bool.
>>>>>>     >
>>>>>>     > I don't either, but it was requested, by Michal...
>>>>>>
>>>>>> Err, your comment on the original bindings was
>>>>>>
>>>>>>     > Can't all these be boolean?
>>>>>
>>>>> With no other context, yes that's what I would ask. Now you've given
>>>>> me some context, between using the existing ones and 2 sets of
>>>>> properties to maintain, I choose the former.
>>>>>
>>>>>> And Michal commented
>>>>>>
>>>>>>     > I think in this case you should described what it is used by
>>> current
>>>>>>     > driver in Microblaze and these options are required. The rest
>>> are by
>>>>>>     > design optional.
>>>>>>     > If you want to change them to different value then current
> binding
>>>>>>     > should be deprecated and have any transition time with code
>>> alignment.
>>>>>>
>>>>>> So that is what I tried to accomplish with this revision. I also
> tried
>>>>>> allowing something like
>>>>>>
>>>>>>            xlnx,one-timer-only = <0>; /* two timers */
>>>>>>            xlnx,one-timer-only = <1>; /* one timer  */
>>>>>>            xlnx,one-timer-only; /* one timer */
>>>>>>            /* property absent means two timers */
>>>>>>
>>>>>> but I was unable to figure out how to express this with
> json-schema. I
>>>>>> don't think it's the best design either...
>>>>>
>>>>> json-schema would certainly let you, but generally we don't want
>>>>> properties to have more than 1 type.
>>>>
>>>> One thing is what it is in system.dts file which was committed in 2009
>>>> and there are just small alignments there. But none is really using it.
>>>> Maybe I should just delete it.
>>>> And this version was generated by Xilinx ancient tools at that time.
> All
>>>> parameters there are fully describing HW and they are not changing.
> Only
>>>> new one can be added.
>>>>
>>>>    From the current microblaze code you can see which properties are
> really
>>>> used.
>>>>
>>>> reg
>>>> interrupts
>>>> xlnx,one-timer-only
>>>> clocks
>>>> clock-frequency
>>>
>>> There is also an implicit dependency on xlnx,count-width. Several times
>>> the existing driver assumes the counter width is 32, but this should
>>> instead be discovered from the devicetree.
>>
>> For me it is important what it is used now. Which is not
>> xlnx,count-width. That's why if you want to add it you can as optional
>> property.
> 
> At the very least we should sanity check it. E.g. check that it is 32
> and return -EINVAL if it is not.

I have not a problem with it but make sure that the check is there only
when property is present not to break all current users.


>>
>>>
>>>> It means from my point of view these should be listed in the binding.
>>>> clock-frequency is optional by code when clock is defined.
>>>>
>>>> All other properties listed in system.dts are from my perspective
>>>> optional and that's how it should be.
>>>
>>> Here is the situation as I understand it
>>>
>>> * This device has existed for around 15 years (since 2006)
>>> * Because it is a soft device, there are several configurable parameters
>>> * Although all of these parameters must be known for a complete
>>>    implementation of this device, some are unnecessary if onlu reduced
>>>    functionality is needed.
>>> * A de facto devicetree binding for this device has existed for at least
>>>    12 years (since 2009), but likely for as long as the device itself
> has
>>>    existed. This binding has not changed substantially during this time.
>>
>> note: IP itself is even much older.
>>
>>> * This binding is present in devicetrees from the Linux kernel, from
>>>    qemu, in other existing systems, and in devicetrees generated by
>>>    Xilinx's toolset.
>>
>> Only from Linux. Qemu is trying to reuse the same properties but it can
>> also add own one. They are trying to be aligned as much as possible but
>> there are a lot of cases where Qemu requires much more information. (I
>> am not saying in this timer case but in general).
>>
>>
>>> * Because the existing driver for this device does not implement all
>>>    functionality for this device, not all properties in the devicetree
>>>    binding are used. In fact, there is (as noted above) one property
>>>    which should be in use but is not because the current driver
>>>    (implicitly) does not support some hardware configurations.
>>> * To support additional functionality, it is necessary to
>>>    use hardware parameters which were not previously necessary.
>>>
>>> Based on the above, we can classify the properties of this binding into
>>> several categories.
>>>
>>> * Those which are currently read by the driver.
>>>    * compatible
>>>    * reg
>>>    * clocks
>>>    * clock-frequency
>>>    * interrupts
>>>    * xlnx,one-timer-only
>>>
>>> * Those which reflect hardware parameters which are currently explicitly
>>>    or implicitly relied upon by the driver.
>>>    * reg
>>>    * clocks
>>>    * clock-frequency
>>>    * interrupts
>>>    * xlnx,counter-width
>>>    * xlnx,one-timer-only
>>>
>>> * Those which are currently present in device trees.
>>>    * compatible
>>>    * reg
>>>    * interrupts
>>>    * clocks
>>>    * clock-frequency
>>>    * xlnx,count-width
>>>    * xlnx,one-timer-only
>>>    * xlnx,trig0-assert
>>>    * xlnx,trig1-assert
>>>    * xlnx,gen0-assert
>>>    * xlnx,gen1-assert
>>>
>>> When choosing what properties to use, we must consider what the impact
>>> of our changes will be on not just the kernel but also on existing users
>>> of this binding:
>>
>> I don't think that this is valid. Rob is asking for adding #pwm-cells
>> which is purely Linux binding. We also don't know what properties are
>> used by others projects not just Linux or Qemu. Also required properties
>> in Linux doesn't need to be required in U-Boot for example even we are
>> trying to aligned all of them. Another case are others RTOSes, etc.
> 
> Here I do not see a way around this. Any way we do it we will need to
> have some new binding. However, as noted below, adding a new binding for
> configuration is easier than exposing properties in new ways.

please look below.


>>> * To use properties currently present in device trees, we just need to
>>>    modify the kernel driver.
>>> * To add additional properties (such as e.g. '#pwm-cells'), we must
>>>    modify the kernel driver. In addition, users who would like to use
>>>    these new properties must add them to their device trees. This may be
>>>    done in a mechanical way using e.g. overlays.
>>> * To deprecate existing properties and introduce new properties to
>>>    expose the same underlying hardware parameters, we must modify the
>>>    kernel driver. However, this has a large impact on existing users.
>>>    They must modify their tools to generate this information in a
>>>    different format. When this information is generated by upstream
> tools
>>>    this may require updating a core part of their build system. For many
>>>    projects, this may happen very infrequently because of the risk that
>>>    such an upgrade will break things. Even if you suggest that Xilinx
> can
>>>    easily modify its tools to generate any sort of output, the time for
>>>    this upgrade to be deployed/adopted may be significantly longer.
>>
>>  From Xilinx perspective it would be ideal to use only properties which
>> fully describe HW in the form how they are generated today. They are
>> stable for a lot of years and as I said only new one are added.
>> But this alignment wasn't accepted long time ago and we have been asked
>> to start to align these properties with similar HW done by others.
>> And truth is that in a lot of cases there is clear 1:1 mapping and
>> generic properties can be simply use. This mapping ends in Xilinx device
>> tree generator.
>> Back to your point. Required properties are required by Linux driver
>> only. This driver is around for quite a long time where certain policies
>> haven't been setup/used/enforced (Microblaze is 2nd architecture which
>> started to use device tree).
>> We should create DT binding doc at that time but in 2009 it wasn't
>> standard practice. In 2007 Grant was adding support for Xilinx PPC
>> platform also without any DT binding document too.
>>
>> That's why we need to review current unwritten DT binding based on code
>> requirements and look at it how to fix it (if needed) and then add PWM
>> support on the top of it.
>> If something needs to be deprecated, let's deprecate it and have
>> transition time for a year or so to adapt to it.
>>
>> Rob knows much better than I how this should be handled.
>>
>> Based on your list:
>>    * compatible
>>    * reg
>>    * clocks
>>    * clock-frequency
>>    * interrupts
>>    * xlnx,one-timer-only
>>
>> all of these are required property in new DT binding.
>>
>> xlnx,counter-width is optional if you want to use.
> 
> Again, for the existing driver this should at least be sanity-checked so
> we fail noisily instead of buggily.

Till now for 10+ years none reported any issue with it that's why I
don't think this is a big problem.

> This is the situation for the generate polarity as well. The driver will
> work fine, but you will not have PWM output and there will be no
> indication why. This is why I think we should start with supporting the
> existing output. This is used to ensure the device will work as
> configured. We can also add support for different properties exposing
> the same information, but to support only new properties is not very
> useful.

Binding can enforce properties to be required for PWM only and that's
totally okay. And because this is new feature and Rob don't need to like
that new properties they can be aligned. I also have preference to be as
much aligned with HW parameters but if they have to change then let's
change it and I will ensure that Xilinx DTG will generate them as they
were described to be aligned.

Thanks,
Michal
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/pwm/xlnx,axi-timer.yaml b/Documentation/devicetree/bindings/pwm/xlnx,axi-timer.yaml
new file mode 100644
index 000000000000..a5e90658e31a
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/xlnx,axi-timer.yaml
@@ -0,0 +1,142 @@ 
+# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pwm/xlnx,axi-timer.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Xilinx LogiCORE IP AXI Timer Device Tree Binding
+
+maintainers:
+  - Sean Anderson <sean.anderson@seco.com>
+
+properties:
+  compatible:
+    oneOf:
+      - items:
+        - const: xlnx,axi-timer-2.0
+        - const: xlnx,xps-timer-1.00.a
+      - const: xlnx,xps-timer-1.00.a
+
+  clocks:
+    maxItems: 1
+
+  clock-names:
+    const: s_axi_aclk
+
+  reg:
+    maxItems: 1
+
+  xlnx,count-width:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 8
+    maximum: 32
+    description:
+      The width of the counter(s), in bits.
+
+  xlnx,gen0-assert:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [ 0, 1 ]
+    default: 1
+    deprecated: true
+    description:
+      The polarity of the generateout0 signal. 0 for active-low, 1 for active-high.
+
+  xlnx,gen0-active-low:
+    $ref: /schemas/types.yaml#/definitions/flag
+    description:
+      The generate0 signal is active-low.
+
+  xlnx,gen1-assert:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [ 0, 1 ]
+    default: 1
+    deprecated: true
+    description:
+      The polarity of the generateout1 signal. 0 for active-low, 1 for active-high.
+
+  xlnx,gen1-active-low:
+    $ref: /schemas/types.yaml#/definitions/flag
+    description:
+      The generate1 signal is active-low.
+
+  xlnx,one-timer-only:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [ 0, 1 ]
+    deprecated: true
+    description:
+      Whether only one timer is present in this block.
+
+  xlnx,single-timer:
+    $ref: /schemas/types.yaml#/definitions/flag
+    description:
+      Only one timer is present in this block.
+
+  xlnx,pwm:
+    $ref: /schemas/types.yaml#/definitions/flag
+    description:
+      This timer should be configured as a PWM.
+
+required:
+  - compatible
+  - reg
+  - xlnx,count-width
+
+allOf:
+  - if:
+      required:
+        - clocks
+    then:
+      required:
+        - clock-names
+
+  - if:
+      required:
+        - xlnx,gen0-active-low
+    then:
+      not:
+        required:
+          - xlnx,gen0-assert
+
+  - if:
+      required:
+        - xlnx,gen0-active-low
+    then:
+      not:
+        required:
+          - xlnx,gen0-assert
+
+  - if:
+      required:
+        - xlnx,one-timer-only
+    then:
+      not:
+        required:
+          - xlnx,single-timer
+
+additionalProperties: true
+
+examples:
+  - |
+    axi_timer_0: timer@800e0000 {
+        clock-names = "s_axi_aclk";
+        clocks = <&zynqmp_clk 71>;
+        compatible = "xlnx,axi-timer-2.0", "xlnx,xps-timer-1.00.a";
+        reg = <0x800e0000 0x10000>;
+        xlnx,count-width = <0x20>;
+        xlnx,gen0-assert = <0x1>;
+        xlnx,gen1-assert = <0x1>;
+        xlnx,one-timer-only = <0x0>;
+        xlnx,trig0-assert = <0x1>;
+        xlnx,trig1-assert = <0x1>;
+    };
+
+  - |
+    axi_timer_0: timer@800e0000 {
+        clock-names = "s_axi_aclk";
+        clocks = <&zynqmp_clk 71>;
+        compatible = "xlnx,axi-timer-2.0", "xlnx,xps-timer-1.00.a";
+        reg = <0x800e0000 0x10000>;
+        xlnx,count-width = <0x20>;
+        xlnx,gen0-active-low;
+        xlnx,single-timer;
+    };