From: Michal Simek <monstr@monstr.eu>
To: "Sean Anderson" <sean.anderson@seco.com>,
linux-pwm@vger.kernel.org, devicetree@vger.kernel.org,
"Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>,
"Sascha Hauer" <s.hauer@pengutronix.de>
Cc: michal.simek@xilinx.com, linux-kernel@vger.kernel.org,
Alvaro Gamez <alvaro.gamez@hazent.com>,
linux-arm-kernel@lists.infradead.org,
Rob Herring <robh@kernel.org>
Subject: Re: [PATCH v4 1/3] dt-bindings: pwm: Add Xilinx AXI Timer
Date: Wed, 30 Jun 2021 15:47:18 +0200 [thread overview]
Message-ID: <13c9345f-b3e5-cc97-437b-c342777fcf3c@monstr.eu> (raw)
In-Reply-To: <20210528214522.617435-1-sean.anderson@seco.com>
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
next prev parent reply other threads:[~2021-06-30 13:47 UTC|newest]
Thread overview: 39+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-05-28 21:45 [PATCH v4 1/3] dt-bindings: pwm: Add Xilinx AXI Timer Sean Anderson
2021-05-28 21:45 ` [PATCH v4 2/3] clocksource: Rewrite Xilinx AXI timer driver Sean Anderson
2021-06-01 8:47 ` Lee Jones
2021-06-01 14:24 ` Sean Anderson
2021-05-28 21:45 ` [PATCH v4 3/3] pwm: Add support for Xilinx AXI Timer Sean Anderson
2021-06-10 16:15 ` Sean Anderson
2021-06-25 6:19 ` Uwe Kleine-König
2021-06-25 15:13 ` Sean Anderson
2021-06-25 16:56 ` Uwe Kleine-König
2021-06-25 17:46 ` Sean Anderson
2021-06-25 17:46 ` Sean Anderson
2021-06-27 18:19 ` Uwe Kleine-König
2021-06-28 15:50 ` Sean Anderson
2021-06-28 16:24 ` Uwe Kleine-König
2021-06-28 16:35 ` Sean Anderson
2021-06-28 17:20 ` Uwe Kleine-König
2021-06-28 17:41 ` Sean Anderson
2021-06-29 8:31 ` Uwe Kleine-König
2021-06-29 18:01 ` Sean Anderson
2021-06-29 20:51 ` Uwe Kleine-König
2021-06-29 22:21 ` Sean Anderson
2021-06-29 22:26 ` Sean Anderson
2021-06-30 8:35 ` Uwe Kleine-König
2021-07-08 16:59 ` Sean Anderson
2021-07-08 19:43 ` Uwe Kleine-König
2021-07-12 16:26 ` Sean Anderson
2021-07-12 19:49 ` Uwe Kleine-König
2021-07-13 21:49 ` Sean Anderson
2021-06-01 13:32 ` [PATCH v4 1/3] dt-bindings: pwm: Add " Rob Herring
2021-06-01 16:47 ` Sean Anderson
2021-06-29 8:38 ` Uwe Kleine-König
2021-06-29 14:53 ` Sean Anderson
2021-06-30 13:47 ` Michal Simek [this message]
2021-06-30 13:58 ` Michal Simek
2021-07-01 15:38 ` Sean Anderson
2021-07-02 11:36 ` Michal Simek
2021-07-01 15:32 ` Sean Anderson
2021-07-02 12:40 ` Michal Simek
2021-07-02 17:31 ` Sean Anderson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=13c9345f-b3e5-cc97-437b-c342777fcf3c@monstr.eu \
--to=monstr@monstr.eu \
--cc=alvaro.gamez@hazent.com \
--cc=devicetree@vger.kernel.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pwm@vger.kernel.org \
--cc=michal.simek@xilinx.com \
--cc=robh@kernel.org \
--cc=s.hauer@pengutronix.de \
--cc=sean.anderson@seco.com \
--cc=u.kleine-koenig@pengutronix.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).