linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
From: Michal Simek <michal.simek@xilinx.com>
To: Sean Anderson <sean.anderson@seco.com>,
	<linux-pwm@vger.kernel.org>, <devicetree@vger.kernel.org>
Cc: michal.simek@xilinx.com, "Alvaro Gamez" <alvaro.gamez@hazent.com>,
	linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	"Daniel Lezcano" <daniel.lezcano@linaro.org>,
	"Lee Jones" <lee.jones@linaro.org>,
	"Thierry Reding" <thierry.reding@gmail.com>,
	"Thomas Gleixner" <tglx@linutronix.de>,
	"Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
Subject: Re: [PATCH v3 2/2] clocksource: Add support for Xilinx AXI Timer
Date: Fri, 14 May 2021 10:59:33 +0200	[thread overview]
Message-ID: <d4bb7b5d-9f38-cf60-fb0b-18f8e0ca2b1e@xilinx.com> (raw)
In-Reply-To: <20210511191239.774570-2-sean.anderson@seco.com>



On 5/11/21 9:12 PM, Sean Anderson wrote:
> This adds generic clocksource and clockevent support for Xilinx LogiCORE IP
> AXI soft timers commonly found on Xilinx FPGAs. This timer is also the
> primary timer for Microblaze processors. This commit also adds support for
> configuring this timer as a PWM (though this could be split off if
> necessary). This whole driver lives in clocksource because it is primarily
> clocksource stuff now (even though it started out as a PWM driver). I think
> teasing apart the driver would not be worth it since they share so many
> functions.
> 
> This driver configures timer 0 (which is always present) as a clocksource,
> and timer 1 (which might be missing) as a clockevent. I don't know if this
> is the correct priority for these timers, or whether we should be using a
> more dynamic allocation scheme.
> 
> At the moment clock control is very basic: we just enable the clock during
> probe and pin the frequency. In the future, someone could add support for
> disabling the clock when not in use. Cascade mode is also unsupported.
> 
> This driver was written with reference to Xilinx DS764 for v1.03.a [1].
> 
> [1] https://www.xilinx.com/support/documentation/ip_documentation/axi_timer/v1_03_a/axi_timer_ds764.pdf
> 
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> ---
> Please let me know if I should organize this differently or if it should
> be broken up.
> 
> Changes in v3:
> - Add clockevent and clocksource support
> - Rewrite probe to only use a device_node, since timers may need to be
>   initialized before we have proper devices. This does bloat the code a bit
>   since we can no longer rely on helpers such as dev_err_probe. We also
>   cannot rely on device resources being free'd on failure, so we must free
>   them manually.
> - We now access registers through xilinx_timer_(read|write). This allows us
>   to deal with endianness issues, as originally seen in the microblaze
>   driver. CAVEAT EMPTOR: I have not tested this on big-endian!
> - Remove old microblaze driver
> 
> Changes in v2:
> - Don't compile this module by default for arm64
> - Add dependencies on COMMON_CLK and HAS_IOMEM
> - Add comment explaining why we depend on !MICROBLAZE
> - Add comment describing device
> - Rename TCSR_(SET|CLEAR) to TCSR_RUN_(SET|CLEAR)
> - Use NSEC_TO_SEC instead of defining our own
> - Use TCSR_RUN_MASK to check if the PWM is enabled, as suggested by Uwe
> - Cast dividends to u64 to avoid overflow
> - Check for over- and underflow when calculating TLR
> - Set xilinx_pwm_ops.owner
> - Don't set pwmchip.base to -1
> - Check range of xlnx,count-width
> - Ensure the clock is always running when the pwm is registered
> - Remove debugfs file :l
> - Report errors with dev_error_probe
> 
>  arch/microblaze/kernel/Makefile    |   2 +-
>  arch/microblaze/kernel/timer.c     | 326 ---------------
>  drivers/clocksource/Kconfig        |  15 +
>  drivers/clocksource/Makefile       |   1 +
>  drivers/clocksource/timer-xilinx.c | 650 +++++++++++++++++++++++++++++
>  5 files changed, 667 insertions(+), 327 deletions(-)
>  delete mode 100644 arch/microblaze/kernel/timer.c
>  create mode 100644 drivers/clocksource/timer-xilinx.c

I don't think this is the right way to go.
The first patch should be move current timer driver from microblaze to
generic location and then apply patches on the top based on what you are
adding/fixing to be able to review every change separately.
When any issue happens it can be bisected and exact patch is identified.
With this way we will end up in this patch and it will take a lot of
time to find where that problem is.

Another part of this is that you have c&p some parts from origin driver
and do not keep origin authors there which can be consider as license
violation.

Thanks,
Michal

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

  parent reply	other threads:[~2021-05-14  9:01 UTC|newest]

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-11 19:12 [PATCH v3 1/2] dt-bindings: pwm: Add Xilinx AXI Timer Sean Anderson
2021-05-11 19:12 ` [PATCH v3 2/2] clocksource: Add support for " Sean Anderson
2021-05-11 19:19   ` Sean Anderson
2021-05-12  8:31   ` kernel test robot
2021-05-14  8:59   ` Michal Simek [this message]
2021-05-14 14:40     ` Sean Anderson
2021-05-17  7:54       ` Michal Simek
2021-05-17 22:15         ` Sean Anderson
2021-05-19  7:24           ` Michal Simek
2021-05-20 20:13             ` Sean Anderson
2021-05-24  7:00               ` Michal Simek
2021-05-24 18:34                 ` Sean Anderson
2021-05-25  6:11                 ` Uwe Kleine-König
2021-05-25 14:30                   ` Sean Anderson
2021-06-16 12:12                   ` Michal Simek
2021-06-18 21:24                     ` Sean Anderson
2021-06-24 16:25                       ` Michal Simek
2021-05-12 18:35 ` [PATCH v3 1/2] dt-bindings: pwm: Add " Rob Herring
2021-05-13  2:16 ` Rob Herring
2021-05-13 14:33   ` Sean Anderson
2021-05-13 15:28     ` Sean Anderson
2021-05-13 20:43       ` Rob Herring
2021-05-13 21:01         ` Sean Anderson
2021-05-14  8:50         ` Michal Simek
2021-05-14 17:13           ` Sean Anderson
2021-05-17  8:28             ` Michal Simek
2021-05-17 14:40               ` Sean Anderson
2021-05-17 14:49                 ` Michal Simek

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=d4bb7b5d-9f38-cf60-fb0b-18f8e0ca2b1e@xilinx.com \
    --to=michal.simek@xilinx.com \
    --cc=alvaro.gamez@hazent.com \
    --cc=daniel.lezcano@linaro.org \
    --cc=devicetree@vger.kernel.org \
    --cc=lee.jones@linaro.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pwm@vger.kernel.org \
    --cc=sean.anderson@seco.com \
    --cc=tglx@linutronix.de \
    --cc=thierry.reding@gmail.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).