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:0.351 / U:2.004 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site