Message-ID: <13c9345f-b3e5-cc97-437b-c342777fcf3c@monstr.eu>
Date:   Wed, 30 Jun 2021 15:47:18 +0200
From:   Michal Simek <monstr@...str.eu>
To:     Sean Anderson <sean.anderson@...o.com>, linux-pwm@...r.kernel.org,
        devicetree@...r.kernel.org,
        Uwe Kleine-König <u.kleine-koenig@...gutronix.de>,
        Sascha Hauer <s.hauer@...gutronix.de>
Cc:     michal.simek@...inx.com, linux-kernel@...r.kernel.org,
        Alvaro Gamez <alvaro.gamez@...ent.com>,
        linux-arm-kernel@...ts.infradead.org, Rob Herring <robh@...nel.org>
Subject: Re: [PATCH v4 1/3] dt-bindings: pwm: Add Xilinx AXI Timer



On 5/28/21 11:45 PM, 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@...o.com>
> ---
> 
> Changes in v4:
> - Remove references to generate polarity so this can get merged
> - Predicate PWM driver on the presence of #pwm-cells
> - Make some properties optional for clocksource drivers
> 
> 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          | 85 +++++++++++++++++++
>  1 file changed, 85 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..48a280f96e63
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/xlnx,axi-timer.yaml

I don't think this is the right location for this.

I have done some grepping and I think this should be done in a different
way. I pretty much like solution around "ti,omap3430-timer" which is
calling dmtimer_systimer_select_best() and later dmtimer_is_preferred()
which in this case would allow us to get rid of cases which are not
suitable for clocksource and clockevent.

And there is drivers/pwm/pwm-omap-dmtimer.c which has link to timer
which is providing functions for it's functionality.

I have also looked at
Documentation/devicetree/bindings/timer/nxp,tpm-timer.yaml which is also
the same device.

And sort of curious if you look at
https://www.xilinx.com/support/documentation/ip_documentation/axi_timer/v2_0/pg079-axi-timer.pdf
( Figure 1-1)
that PWM is taking input from generate out 0 and generate out 1 which is
maybe can be modeled is any output and pwm driver can register inputs
for pwm driver.


> @@ -0,0 +1,85 @@
> +# 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@...o.com>
> +
> +properties:
> +  compatible:
> +    oneOf:
> +      - items:
> +         - const: xlnx,axi-timer-2.0
> +         - const: xlnx,xps-timer-1.00.a
> +      - items:
> +         - const: xlnx,xps-timer-1.00.a
> +
> +  clocks:
> +    maxItems: 1
> +
> +  clock-names:
> +    const: s_axi_aclk

Origin driver is not using this clock name and it is only one that's why
it shouldn't be listed.

> +
> +  interrupts:
> +    maxItems: 1
> +
> +  reg:
> +    maxItems: 1
> +
> +  xlnx,count-width:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 8
> +    maximum: 32
> +    default: 32

This is not accurate. It should be enum because only 8/16/32 are valid
values here.

> +    description:
> +      The width of the counter(s), in bits.
> +
> +  xlnx,one-timer-only:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [ 0, 1 ]
> +    description:
> +      Whether only one timer is present in this block.
> +
> +required:
> +  - compatible
> +  - reg
> +  - xlnx,one-timer-only
> +
> +allOf:
> +  - if:
> +      required:
> +        - '#pwm-cells'

Let's discussed this usage based on design.

> +    then:
> +      allOf:
> +        - required:
> +            - clocks
> +        - properties:
> +            xlnx,one-timer-only:
> +              const: 0
> +    else:
> +      required:
> +        - interrupts
> +  - if:
> +      required:
> +        - clocks
> +    then:
> +      required:
> +        - clock-names

And this checking should be removed too.

> +
> +additionalProperties: true
> +
> +examples:
> +  - |
> +    axi_timer_0: timer@...e0000 {

label is useless here and should be removed.

> +        #pwm-cells = <0>;
> +        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,one-timer-only = <0x0>;
> +    };
> 

I would list example without pwm-cells first as it is valid and reflect
current status.

Thanks,
Michal


-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Xilinx Microblaze
Maintainer of Linux kernel - Xilinx Zynq ARM and ZynqMP ARM64 SoCs
U-Boot custodian - Xilinx Microblaze/Zynq/ZynqMP/Versal SoCs

Powered by blists - more mailing lists