lkml.org 
[lkml]   [2021]   [Jun]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH v4 1/3] dt-bindings: pwm: Add Xilinx AXI Timer
    Date


    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@seco.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@seco.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@800e0000 {

    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

    \
     
     \ /
      Last update: 2021-06-30 15:48    [W:3.111 / U:0.084 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site