[v3,2/2] clocksource: Add support for Xilinx AXI Timer

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

On 5/11/21 3: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
> 
> diff --git a/arch/microblaze/kernel/Makefile b/arch/microblaze/kernel/Makefile
> index 15a20eb814ce..986b1f21d90e 100644
> --- a/arch/microblaze/kernel/Makefile
> +++ b/arch/microblaze/kernel/Makefile
> @@ -17,7 +17,7 @@ extra-y := head.o vmlinux.lds
>   obj-y += dma.o exceptions.o \
>   	hw_exception_handler.o irq.o \
>   	process.o prom.o ptrace.o \
> -	reset.o setup.o signal.o sys_microblaze.o timer.o traps.o unwind.o
> +	reset.o setup.o signal.o sys_microblaze.o traps.o unwind.o
>   
>   obj-y += cpu/
>   
> diff --git a/arch/microblaze/kernel/timer.c b/arch/microblaze/kernel/timer.c
> deleted file mode 100644
> index f8832cf49384..000000000000
> --- a/arch/microblaze/kernel/timer.c
> +++ /dev/null
> @@ -1,326 +0,0 @@
> -/*
> - * Copyright (C) 2007-2013 Michal Simek <monstr@monstr.eu>
> - * Copyright (C) 2012-2013 Xilinx, Inc.
> - * Copyright (C) 2007-2009 PetaLogix
> - * Copyright (C) 2006 Atmark Techno, Inc.
> - *
> - * This file is subject to the terms and conditions of the GNU General Public
> - * License. See the file "COPYING" in the main directory of this archive
> - * for more details.
> - */
> -
> -#include <linux/interrupt.h>
> -#include <linux/delay.h>
> -#include <linux/sched.h>
> -#include <linux/sched/clock.h>
> -#include <linux/sched_clock.h>
> -#include <linux/clk.h>
> -#include <linux/clockchips.h>
> -#include <linux/of_address.h>
> -#include <linux/of_irq.h>
> -#include <linux/timecounter.h>
> -#include <asm/cpuinfo.h>
> -
> -static void __iomem *timer_baseaddr;
> -
> -static unsigned int freq_div_hz;
> -static unsigned int timer_clock_freq;
> -
> -#define TCSR0	(0x00)
> -#define TLR0	(0x04)
> -#define TCR0	(0x08)
> -#define TCSR1	(0x10)
> -#define TLR1	(0x14)
> -#define TCR1	(0x18)
> -
> -#define TCSR_MDT	(1<<0)
> -#define TCSR_UDT	(1<<1)
> -#define TCSR_GENT	(1<<2)
> -#define TCSR_CAPT	(1<<3)
> -#define TCSR_ARHT	(1<<4)
> -#define TCSR_LOAD	(1<<5)
> -#define TCSR_ENIT	(1<<6)
> -#define TCSR_ENT	(1<<7)
> -#define TCSR_TINT	(1<<8)
> -#define TCSR_PWMA	(1<<9)
> -#define TCSR_ENALL	(1<<10)
> -
> -static unsigned int (*read_fn)(void __iomem *);
> -static void (*write_fn)(u32, void __iomem *);
> -
> -static void timer_write32(u32 val, void __iomem *addr)
> -{
> -	iowrite32(val, addr);
> -}
> -
> -static unsigned int timer_read32(void __iomem *addr)
> -{
> -	return ioread32(addr);
> -}
> -
> -static void timer_write32_be(u32 val, void __iomem *addr)
> -{
> -	iowrite32be(val, addr);
> -}
> -
> -static unsigned int timer_read32_be(void __iomem *addr)
> -{
> -	return ioread32be(addr);
> -}
> -
> -static inline void xilinx_timer0_stop(void)
> -{
> -	write_fn(read_fn(timer_baseaddr + TCSR0) & ~TCSR_ENT,
> -		 timer_baseaddr + TCSR0);
> -}
> -
> -static inline void xilinx_timer0_start_periodic(unsigned long load_val)
> -{
> -	if (!load_val)
> -		load_val = 1;
> -	/* loading value to timer reg */
> -	write_fn(load_val, timer_baseaddr + TLR0);
> -
> -	/* load the initial value */
> -	write_fn(TCSR_LOAD, timer_baseaddr + TCSR0);
> -
> -	/* see timer data sheet for detail
> -	 * !ENALL - don't enable 'em all
> -	 * !PWMA - disable pwm
> -	 * TINT - clear interrupt status
> -	 * ENT- enable timer itself
> -	 * ENIT - enable interrupt
> -	 * !LOAD - clear the bit to let go
> -	 * ARHT - auto reload
> -	 * !CAPT - no external trigger
> -	 * !GENT - no external signal
> -	 * UDT - set the timer as down counter
> -	 * !MDT0 - generate mode
> -	 */
> -	write_fn(TCSR_TINT|TCSR_ENIT|TCSR_ENT|TCSR_ARHT|TCSR_UDT,
> -		 timer_baseaddr + TCSR0);
> -}
> -
> -static inline void xilinx_timer0_start_oneshot(unsigned long load_val)
> -{
> -	if (!load_val)
> -		load_val = 1;
> -	/* loading value to timer reg */
> -	write_fn(load_val, timer_baseaddr + TLR0);
> -
> -	/* load the initial value */
> -	write_fn(TCSR_LOAD, timer_baseaddr + TCSR0);
> -
> -	write_fn(TCSR_TINT|TCSR_ENIT|TCSR_ENT|TCSR_ARHT|TCSR_UDT,
> -		 timer_baseaddr + TCSR0);
> -}
> -
> -static int xilinx_timer_set_next_event(unsigned long delta,
> -					struct clock_event_device *dev)
> -{
> -	pr_debug("%s: next event, delta %x\n", __func__, (u32)delta);
> -	xilinx_timer0_start_oneshot(delta);
> -	return 0;
> -}
> -
> -static int xilinx_timer_shutdown(struct clock_event_device *evt)
> -{
> -	pr_info("%s\n", __func__);
> -	xilinx_timer0_stop();
> -	return 0;
> -}
> -
> -static int xilinx_timer_set_periodic(struct clock_event_device *evt)
> -{
> -	pr_info("%s\n", __func__);
> -	xilinx_timer0_start_periodic(freq_div_hz);
> -	return 0;
> -}
> -
> -static struct clock_event_device clockevent_xilinx_timer = {
> -	.name			= "xilinx_clockevent",
> -	.features		= CLOCK_EVT_FEAT_ONESHOT |
> -				  CLOCK_EVT_FEAT_PERIODIC,
> -	.shift			= 8,
> -	.rating			= 300,
> -	.set_next_event		= xilinx_timer_set_next_event,
> -	.set_state_shutdown	= xilinx_timer_shutdown,
> -	.set_state_periodic	= xilinx_timer_set_periodic,
> -};
> -
> -static inline void timer_ack(void)
> -{
> -	write_fn(read_fn(timer_baseaddr + TCSR0), timer_baseaddr + TCSR0);
> -}
> -
> -static irqreturn_t timer_interrupt(int irq, void *dev_id)
> -{
> -	struct clock_event_device *evt = &clockevent_xilinx_timer;
> -	timer_ack();
> -	evt->event_handler(evt);
> -	return IRQ_HANDLED;
> -}
> -
> -static __init int xilinx_clockevent_init(void)
> -{
> -	clockevent_xilinx_timer.mult =
> -		div_sc(timer_clock_freq, NSEC_PER_SEC,
> -				clockevent_xilinx_timer.shift);
> -	clockevent_xilinx_timer.max_delta_ns =
> -		clockevent_delta2ns((u32)~0, &clockevent_xilinx_timer);
> -	clockevent_xilinx_timer.max_delta_ticks = (u32)~0;
> -	clockevent_xilinx_timer.min_delta_ns =
> -		clockevent_delta2ns(1, &clockevent_xilinx_timer);
> -	clockevent_xilinx_timer.min_delta_ticks = 1;
> -	clockevent_xilinx_timer.cpumask = cpumask_of(0);
> -	clockevents_register_device(&clockevent_xilinx_timer);
> -
> -	return 0;
> -}
> -
> -static u64 xilinx_clock_read(void)
> -{
> -	return read_fn(timer_baseaddr + TCR1);
> -}
> -
> -static u64 xilinx_read(struct clocksource *cs)
> -{
> -	/* reading actual value of timer 1 */
> -	return (u64)xilinx_clock_read();
> -}
> -
> -static struct timecounter xilinx_tc = {
> -	.cc = NULL,
> -};
> -
> -static u64 xilinx_cc_read(const struct cyclecounter *cc)
> -{
> -	return xilinx_read(NULL);
> -}
> -
> -static struct cyclecounter xilinx_cc = {
> -	.read = xilinx_cc_read,
> -	.mask = CLOCKSOURCE_MASK(32),
> -	.shift = 8,
> -};
> -
> -static int __init init_xilinx_timecounter(void)
> -{
> -	xilinx_cc.mult = div_sc(timer_clock_freq, NSEC_PER_SEC,
> -				xilinx_cc.shift);
> -
> -	timecounter_init(&xilinx_tc, &xilinx_cc, sched_clock());
> -
> -	return 0;
> -}
> -
> -static struct clocksource clocksource_microblaze = {
> -	.name		= "xilinx_clocksource",
> -	.rating		= 300,
> -	.read		= xilinx_read,
> -	.mask		= CLOCKSOURCE_MASK(32),
> -	.flags		= CLOCK_SOURCE_IS_CONTINUOUS,
> -};
> -
> -static int __init xilinx_clocksource_init(void)
> -{
> -	int ret;
> -
> -	ret = clocksource_register_hz(&clocksource_microblaze,
> -				      timer_clock_freq);
> -	if (ret) {
> -		pr_err("failed to register clocksource");
> -		return ret;
> -	}
> -
> -	/* stop timer1 */
> -	write_fn(read_fn(timer_baseaddr + TCSR1) & ~TCSR_ENT,
> -		 timer_baseaddr + TCSR1);
> -	/* start timer1 - up counting without interrupt */
> -	write_fn(TCSR_TINT|TCSR_ENT|TCSR_ARHT, timer_baseaddr + TCSR1);
> -
> -	/* register timecounter - for ftrace support */
> -	return init_xilinx_timecounter();
> -}
> -
> -static int __init xilinx_timer_init(struct device_node *timer)
> -{
> -	struct clk *clk;
> -	static int initialized;
> -	u32 irq;
> -	u32 timer_num = 1;
> -	int ret;
> -
> -	if (initialized)
> -		return -EINVAL;
> -
> -	initialized = 1;
> -
> -	timer_baseaddr = of_iomap(timer, 0);
> -	if (!timer_baseaddr) {
> -		pr_err("ERROR: invalid timer base address\n");
> -		return -ENXIO;
> -	}
> -
> -	write_fn = timer_write32;
> -	read_fn = timer_read32;
> -
> -	write_fn(TCSR_MDT, timer_baseaddr + TCSR0);
> -	if (!(read_fn(timer_baseaddr + TCSR0) & TCSR_MDT)) {
> -		write_fn = timer_write32_be;
> -		read_fn = timer_read32_be;
> -	}
> -
> -	irq = irq_of_parse_and_map(timer, 0);
> -	if (irq <= 0) {
> -		pr_err("Failed to parse and map irq");
> -		return -EINVAL;
> -	}
> -
> -	of_property_read_u32(timer, "xlnx,one-timer-only", &timer_num);
> -	if (timer_num) {
> -		pr_err("Please enable two timers in HW\n");
> -		return -EINVAL;
> -	}
> -
> -	pr_info("%pOF: irq=%d\n", timer, irq);
> -
> -	clk = of_clk_get(timer, 0);
> -	if (IS_ERR(clk)) {
> -		pr_err("ERROR: timer CCF input clock not found\n");
> -		/* If there is clock-frequency property than use it */
> -		of_property_read_u32(timer, "clock-frequency",
> -				    &timer_clock_freq);
> -	} else {
> -		timer_clock_freq = clk_get_rate(clk);
> -	}
> -
> -	if (!timer_clock_freq) {
> -		pr_err("ERROR: Using CPU clock frequency\n");
> -		timer_clock_freq = cpuinfo.cpu_clock_freq;
> -	}
> -
> -	freq_div_hz = timer_clock_freq / HZ;
> -
> -	ret = request_irq(irq, timer_interrupt, IRQF_TIMER, "timer",
> -			  &clockevent_xilinx_timer);
> -	if (ret) {
> -		pr_err("Failed to setup IRQ");
> -		return ret;
> -	}
> -
> -	ret = xilinx_clocksource_init();
> -	if (ret)
> -		return ret;
> -
> -	ret = xilinx_clockevent_init();
> -	if (ret)
> -		return ret;
> -
> -	sched_clock_register(xilinx_clock_read, 32, timer_clock_freq);
> -
> -	return 0;
> -}
> -
> -TIMER_OF_DECLARE(xilinx_timer, "xlnx,xps-timer-1.00.a",
> -		       xilinx_timer_init);
> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> index 39aa21d01e05..35c95671d242 100644
> --- a/drivers/clocksource/Kconfig
> +++ b/drivers/clocksource/Kconfig
> @@ -693,4 +693,19 @@ config MICROCHIP_PIT64B
>   	  modes and high resolution. It is used as a clocksource
>   	  and a clockevent.
>   
> +config XILINX_TIMER
> +	tristate "Xilinx AXI Timer support"
> +	depends on HAS_IOMEM && COMMON_CLK
> +	default y if MICROBLAZE
> +	help
> +	  Clocksource, clockevent, and PWM drivers for Xilinx LogiCORE
> +	  IP AXI Timers. This timer is typically a soft core which may
> +	  be present in Xilinx FPGAs. This device may also be present in
> +	  Microblaze soft processors. If you don't have this IP in your
> +	  design, choose N.
> +
> +	  To use this device as the primary clocksource for your system,
> +	  choose Y here. Otherwise, this driver will not be available
> +	  early enough during boot. To compile this driver as a module,
> +	  choose M here: the module will be called timer-xilinx.
>   endmenu
> diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
> index c17ee32a7151..717f01c0ac41 100644
> --- a/drivers/clocksource/Makefile
> +++ b/drivers/clocksource/Makefile
> @@ -58,6 +58,7 @@ obj-$(CONFIG_MILBEAUT_TIMER)	+= timer-milbeaut.o
>   obj-$(CONFIG_SPRD_TIMER)	+= timer-sprd.o
>   obj-$(CONFIG_NPCM7XX_TIMER)	+= timer-npcm7xx.o
>   obj-$(CONFIG_RDA_TIMER)		+= timer-rda.o
> +obj-$(CONFIG_XILINX_TIMER)	+= timer-xilinx.o
>   
>   obj-$(CONFIG_ARC_TIMERS)		+= arc_timer.o
>   obj-$(CONFIG_ARM_ARCH_TIMER)		+= arm_arch_timer.o
> diff --git a/drivers/clocksource/timer-xilinx.c b/drivers/clocksource/timer-xilinx.c
> new file mode 100644
> index 000000000000..b410c6af9c63
> --- /dev/null
> +++ b/drivers/clocksource/timer-xilinx.c
> @@ -0,0 +1,650 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2021 Sean Anderson <sean.anderson@seco.com>
> + *
> + * For Xilinx LogiCORE IP AXI Timer documentation, refer to DS764:
> + * https://www.xilinx.com/support/documentation/ip_documentation/axi_timer/v1_03_a/axi_timer_ds764.pdf
> + *
> + * Hardware limitations:
> + * - When in cascade mode we cannot read the full 64-bit counter in one go
> + * - When changing both duty cycle and period, we may end up with one cycle
> + *   with the old duty cycle and the new period.
> + * - Cannot produce 100% duty cycle.
> + * - Only produces "normal" output.
> + */
> +#include <linux/bitfield.h>
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/clockchips.h>
> +#include <linux/clocksource.h>
> +#include <linux/device.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/sched_clock.h>
> +#include <asm/io.h>
> +#if IS_ENABLED(CONFIG_MICROBLAZE)
> +#include <asm/cpuinfo.h>
> +#endif
> +
> +/* A replacement for dev_err_probe, since we don't always have a device */
> +#define xilinx_timer_err(np, err, fmt, ...) ({ \
> +	pr_err("%pOF: error %d: " fmt, (np), (int)(err), ##__VA_ARGS__); \
> +	err; \
> +})
> +
> +#define TCSR0	0x00
> +#define TLR0	0x04
> +#define TCR0	0x08
> +#define TCSR1	0x10
> +#define TLR1	0x14
> +#define TCR1	0x18
> +
> +#define TCSR_MDT	BIT(0)
> +#define TCSR_UDT	BIT(1)
> +#define TCSR_GENT	BIT(2)
> +#define TCSR_CAPT	BIT(3)
> +#define TCSR_ARHT	BIT(4)
> +#define TCSR_LOAD	BIT(5)
> +#define TCSR_ENIT	BIT(6)
> +#define TCSR_ENT	BIT(7)
> +#define TCSR_TINT	BIT(8)
> +#define TCSR_PWMA	BIT(9)
> +#define TCSR_ENALL	BIT(10)
> +#define TCSR_CASC	BIT(11)
> +
> +/*
> + * The idea here is to capture whether the PWM is actually running (e.g.
> + * because we or the bootloader set it up) and we need to be careful to ensure
> + * we don't cause a glitch. According to the device data sheet, to enable the
> + * PWM we need to
> + *
> + * - Set both timers to generate mode (MDT=1)
> + * - Set both timers to PWM mode (PWMA=1)
> + * - Enable the generate out signals (GENT=1)
> + *
> + * In addition,
> + *
> + * - The timer must be running (ENT=1)
> + * - The timer must auto-reload TLR into TCR (ARHT=1)
> + * - We must not be in the process of loading TLR into TCR (LOAD=0)
> + * - Cascade mode must be disabled (CASC=0)
> + *
> + * If any of these differ from usual, then the PWM is either disabled, or is
> + * running in a mode that this driver does not support.
> + */
> +#define TCSR_PWM_SET (TCSR_GENT | TCSR_ARHT | TCSR_ENT | TCSR_PWMA)
> +#define TCSR_PWM_CLEAR (TCSR_MDT | TCSR_LOAD)
> +#define TCSR_PWM_MASK (TCSR_PWM_SET | TCSR_PWM_CLEAR)
> +
> +/**
> + * struct xilinx_timer_priv - Private data for Xilinx AXI timer driver
> + * @cs: Clocksource device
> + * @ce: Clockevent device
> + * @pwm: PWM controller chip
> + * @clk: Parent clock
> + * @regs: Base address of this device
> + * @width: Width of the counters, in bits
> + * @XILINX_TIMER_ONE: We have only one timer.
> + * @XILINX_TIMER_PWM: Configured as a PWM.
> + * @XILINX_TIMER_CLK: We were missing a device tree clock and created our own
> + * @flags: Flags for what type of device we are
> + */
> +struct xilinx_timer_priv {
> +	union {
> +		struct {
> +			struct clocksource cs;
> +			struct clock_event_device ce;
> +		};
> +		struct pwm_chip pwm;
> +	};
> +	struct clk *clk;
> +	void __iomem *regs;
> +	u32 (*read)(const volatile void __iomem *addr);
> +	void (*write)(u32 value, volatile void __iomem *addr);
> +	unsigned int width;
> +	enum {
> +		XILINX_TIMER_ONE = BIT(0),
> +		XILINX_TIMER_PWM = BIT(1),
> +		XILINX_TIMER_CLK = BIT(2),
> +	} flags;
> +};
> +
> +static inline struct xilinx_timer_priv
> +*xilinx_pwm_chip_to_priv(struct pwm_chip *chip)
> +{
> +	return container_of(chip, struct xilinx_timer_priv, pwm);
> +}
> +
> +static inline struct xilinx_timer_priv
> +*xilinx_clocksource_to_priv(struct clocksource *cs)
> +{
> +	return container_of(cs, struct xilinx_timer_priv, cs);
> +}
> +
> +static inline struct xilinx_timer_priv
> +*xilinx_clockevent_to_priv(struct clock_event_device *ce)
> +{
> +	return container_of(ce, struct xilinx_timer_priv, ce);
> +}
> +
> +static u32 xilinx_ioread32be(const volatile void __iomem *addr)
> +{
> +	return ioread32be(addr);
> +}
> +
> +static void xilinx_iowrite32be(u32 value, volatile void __iomem *addr)
> +{
> +	iowrite32be(value, addr);
> +}
> +
> +static inline u32 xilinx_timer_read(struct xilinx_timer_priv *priv,
> +				    int offset)
> +{
> +	return priv->read(priv->regs + offset);
> +}
> +
> +static inline void xilinx_timer_write(struct xilinx_timer_priv *priv,
> +				      u32 value, int offset)
> +{
> +	priv->write(value, priv->regs + offset);
> +}
> +
> +static inline u64 xilinx_timer_max(struct xilinx_timer_priv *priv)
> +{
> +	return BIT_ULL(priv->width) - 1;
> +}
> +
> +static int xilinx_timer_tlr_cycles(struct xilinx_timer_priv *priv, u32 *tlr,
> +				   u32 tcsr, u64 cycles)
> +{
> +	u64 max_count = xilinx_timer_max(priv);
> +
> +	if (cycles < 2 || cycles > max_count + 2)
> +		return -ERANGE;
> +
> +	if (tcsr & TCSR_UDT)
> +		*tlr = cycles - 2;
> +	else
> +		*tlr = max_count - cycles + 2;
> +
> +	return 0;
> +}
> +
> +static bool xilinx_timer_pwm_enabled(u32 tcsr0, u32 tcsr1)
> +{
> +	return ((TCSR_PWM_MASK | TCSR_CASC) & tcsr0) == TCSR_PWM_SET &&
> +		(TCSR_PWM_MASK & tcsr1) == TCSR_PWM_SET;
> +}
> +
> +static int xilinx_timer_tlr_period(struct xilinx_timer_priv *priv, u32 *tlr,
> +				   u32 tcsr, unsigned int period)
> +{
> +	u64 cycles = DIV_ROUND_DOWN_ULL((u64)period * clk_get_rate(priv->clk),
> +					NSEC_PER_SEC);
> +
> +	return xilinx_timer_tlr_cycles(priv, tlr, tcsr, cycles);
> +}
> +
> +static unsigned int xilinx_timer_get_period(struct xilinx_timer_priv *priv,
> +					    u32 tlr, u32 tcsr)
> +{
> +	u64 cycles;
> +
> +	if (tcsr & TCSR_UDT)
> +		cycles = tlr + 2;
> +	else
> +		cycles = xilinx_timer_max(priv) - tlr + 2;
> +
> +	return DIV_ROUND_UP_ULL(cycles * NSEC_PER_SEC,
> +				clk_get_rate(priv->clk));
> +}
> +
> +static int xilinx_pwm_apply(struct pwm_chip *chip, struct pwm_device *unused,
> +			    const struct pwm_state *state)
> +{
> +	int ret;
> +	struct xilinx_timer_priv *priv = xilinx_pwm_chip_to_priv(chip);
> +	u32 tlr0, tlr1;
> +	u32 tcsr0 = xilinx_timer_read(priv, TCSR0);
> +	u32 tcsr1 = xilinx_timer_read(priv, TCSR1);
> +	bool enabled = xilinx_timer_pwm_enabled(tcsr0, tcsr1);
> +
> +	if (state->polarity != PWM_POLARITY_NORMAL)
> +		return -EINVAL;
> +
> +	ret = xilinx_timer_tlr_period(priv, &tlr0, tcsr0, state->period);
> +	if (ret)
> +		return ret;
> +
> +	ret = xilinx_timer_tlr_period(priv, &tlr1, tcsr1, state->duty_cycle);
> +	if (ret)
> +		return ret;
> +
> +	xilinx_timer_write(priv, tlr0, TLR0);
> +	xilinx_timer_write(priv, tlr1, TLR1);
> +
> +	if (state->enabled) {
> +		/* Only touch the TCSRs if we aren't already running */
> +		if (!enabled) {
> +			/* Load TLR into TCR */
> +			xilinx_timer_write(priv, tcsr0 | TCSR_LOAD, TCSR0);
> +			xilinx_timer_write(priv, tcsr1 | TCSR_LOAD, TCSR1);
> +			/* Enable timers all at once with ENALL */
> +			tcsr0 = (TCSR_PWM_SET & ~TCSR_ENT) | (tcsr0 & TCSR_UDT);
> +			tcsr1 = TCSR_PWM_SET | TCSR_ENALL | (tcsr1 & TCSR_UDT);
> +			xilinx_timer_write(priv, tcsr0, TCSR0);
> +			xilinx_timer_write(priv, tcsr1, TCSR1);
> +		}
> +	} else {
> +		xilinx_timer_write(priv, 0, TCSR0);
> +		xilinx_timer_write(priv, 0, TCSR1);
> +	}
> +
> +	return 0;
> +}
> +
> +static void xilinx_pwm_get_state(struct pwm_chip *chip,
> +				 struct pwm_device *unused,
> +				 struct pwm_state *state)
> +{
> +	struct xilinx_timer_priv *priv = xilinx_pwm_chip_to_priv(chip);
> +	u32 tlr0 = xilinx_timer_read(priv, TLR0);
> +	u32 tlr1 = xilinx_timer_read(priv, TLR1);
> +	u32 tcsr0 = xilinx_timer_read(priv, TCSR0);
> +	u32 tcsr1 = xilinx_timer_read(priv, TCSR1);
> +
> +	state->period = xilinx_timer_get_period(priv, tlr0, tcsr0);
> +	state->duty_cycle = xilinx_timer_get_period(priv, tlr1, tcsr1);
> +	state->enabled = xilinx_timer_pwm_enabled(tcsr0, tcsr1);
> +	state->polarity = PWM_POLARITY_NORMAL;
> +}
> +
> +static const struct pwm_ops xilinx_pwm_ops = {
> +	.apply = xilinx_pwm_apply,
> +	.get_state = xilinx_pwm_get_state,
> +	.owner = THIS_MODULE,
> +};
> +
> +static int xilinx_pwm_init(struct device *dev,
> +			   struct xilinx_timer_priv *priv)
> +{
> +	int ret;
> +
> +	if (!dev)
> +		return -EPROBE_DEFER;
> +
> +	priv->pwm.dev = dev;
> +	priv->pwm.ops = &xilinx_pwm_ops;
> +	priv->pwm.npwm = 1;
> +	ret = pwmchip_add(&priv->pwm);
> +	if (ret)
> +		xilinx_timer_err(dev->of_node, ret,
> +				 "could not register pwm chip\n");
> +	return ret;
> +}
> +
> +static irqreturn_t xilinx_timer_handler(int irq, void *dev)
> +{
> +	struct xilinx_timer_priv *priv = dev;
> +	u32 tcsr1 = xilinx_timer_read(priv, TCSR1);
> +
> +	/* Acknowledge interrupt */
> +	xilinx_timer_write(priv, tcsr1 | TCSR_TINT, TCSR1);
> +	priv->ce.event_handler(&priv->ce);
> +	return IRQ_HANDLED;
> +}
> +
> +static int xilinx_clockevent_next_event(unsigned long evt,
> +					struct clock_event_device *ce)
> +{
> +	struct xilinx_timer_priv *priv = xilinx_clockevent_to_priv(ce);
> +
> +	xilinx_timer_write(priv, evt, TLR1);
> +	xilinx_timer_write(priv, TCSR_LOAD, TCSR1);
> +	xilinx_timer_write(priv, TCSR_ENIT | TCSR_ENT, TCSR1);
> +	return 0;
> +}
> +
> +static int xilinx_clockevent_state_periodic(struct clock_event_device *ce)
> +{
> +	int ret;
> +	u32 tlr1;
> +	struct xilinx_timer_priv *priv = xilinx_clockevent_to_priv(ce);
> +
> +	ret = xilinx_timer_tlr_cycles(priv, &tlr1, 0,
> +				      clk_get_rate(priv->clk) / HZ);
> +	if (ret)
> +		return ret;
> +
> +	xilinx_timer_write(priv, tlr1, TLR1);
> +	xilinx_timer_write(priv, TCSR_LOAD, TCSR1);
> +	xilinx_timer_write(priv, TCSR_ARHT | TCSR_ENIT | TCSR_ENT, TCSR1);
> +	return 0;
> +}
> +
> +static int xilinx_clockevent_shutdown(struct clock_event_device *ce)
> +{
> +	xilinx_timer_write(xilinx_clockevent_to_priv(ce), 0, TCSR1);
> +	return 0;
> +}
> +
> +static const struct clock_event_device xilinx_clockevent_base = {
> +	.name = "xilinx_clockevent",
> +	.features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT,
> +	.set_next_event = xilinx_clockevent_next_event,
> +	.set_state_periodic = xilinx_clockevent_state_periodic,
> +	.set_state_shutdown = xilinx_clockevent_shutdown,
> +	.rating = 300,
> +	.cpumask = cpu_possible_mask,
> +	.owner = THIS_MODULE,
> +};
> +
> +static int xilinx_clockevent_init(struct device_node *np,
> +				  struct xilinx_timer_priv *priv)
> +{
> +	int ret = of_irq_get(np, 0);
> +
> +	if (ret < 0)
> +		return xilinx_timer_err(np, ret, "could not get irq\n");
> +
> +	ret = request_irq(ret, xilinx_timer_handler, IRQF_TIMER,
> +			  np->full_name, priv);
> +	if (ret)
> +		return xilinx_timer_err(np, ret, "could not request irq\n");
> +
> +	memcpy(&priv->ce, &xilinx_clockevent_base, sizeof(priv->ce));
> +	clockevents_config_and_register(&priv->ce,
> +					clk_get_rate(priv->clk), 2,
> +					min_t(u64,
> +					      xilinx_timer_max(priv) + 2,
> +					      ULONG_MAX));
> +	return 0;
> +}
> +
> +static u64 xilinx_clocksource_read(struct clocksource *cs)
> +{
> +	return xilinx_timer_read(xilinx_clocksource_to_priv(cs), TCR0);
> +}
> +
> +static const struct clocksource xilinx_clocksource_base = {
> +	.read = xilinx_clocksource_read,
> +	.name = "xilinx_clocksource",
> +	.rating = 300,
> +	.flags = CLOCK_SOURCE_IS_CONTINUOUS,
> +	.owner = THIS_MODULE,
> +};
> +
> +static int xilinx_clocksource_init(struct xilinx_timer_priv *priv)
> +{
> +	xilinx_timer_write(priv, 0, TLR0);
> +	/* Load TLR and clear any interrupts */
> +	xilinx_timer_write(priv, TCSR_LOAD | TCSR_TINT, TCSR0);
> +	/* Start the timer counting up with auto-reload */
> +	xilinx_timer_write(priv, TCSR_ARHT | TCSR_ENT, TCSR0);
> +
> +	memcpy(&priv->cs, &xilinx_clocksource_base, sizeof(priv->cs));
> +	priv->cs.mask = xilinx_timer_max(priv);
> +	return clocksource_register_hz(&priv->cs, clk_get_rate(priv->clk));
> +}
> +
> +static struct clk *xilinx_timer_clock_init(struct device_node *np,
> +					   struct xilinx_timer_priv *priv)
> +{
> +	int ret;
> +	u32 freq;
> +	struct clk_hw *hw;
> +	struct clk *clk = of_clk_get_by_name(np, "s_axi_aclk");
> +
> +	if (!IS_ERR(clk) || PTR_ERR(clk) == -EPROBE_DEFER)
> +		return clk;
> +
> +	pr_warn("%pOF: missing s_axi_aclk, falling back to clock-frequency\n",
> +		np);
> +	ret = of_property_read_u32(np, "clock-frequency", &freq);
> +	if (ret) {
> +#if IS_ENABLED(CONFIG_MICROBLAZE)
> +		pr_warn("%pOF: missing clock-frequency, falling back to /cpus/timebase-frequency\n",
> +			np);
> +		freq = cpuinfo.cpu_clock_freq;
> +#else
> +		return ERR_PTR(ret);
> +#endif
> +	}
> +
> +	priv->flags |= XILINX_TIMER_CLK;
> +	hw = __clk_hw_register_fixed_rate(NULL, np, "s_axi_aclk", NULL, NULL,
> +					  NULL, 0, freq, 0, 0);
> +	if (IS_ERR(hw))
> +		return ERR_CAST(hw);
> +	return hw->clk;
> +}
> +
> +static struct xilinx_timer_priv *xilinx_timer_init(struct device *dev,
> +						   struct device_node *np)
> +{
> +	bool pwm;
> +	int i, ret;
> +	struct xilinx_timer_priv *priv;
> +	u32 one_timer, tcsr0;
> +
> +	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return ERR_PTR(-ENOMEM);
> +
> +	priv->regs = of_iomap(np, 0);
> +	if (!priv->regs) {
> +		ret = -ENXIO;
> +		goto err_priv;
> +	} else if (IS_ERR(priv->regs)) {
> +		ret = PTR_ERR(priv->regs);
> +		goto err_priv;
> +	}
> +
> +	priv->read = ioread32;
> +	priv->write = iowrite32;
> +	/*
> +	 * We aren't using the interrupts yet, so use ENIT to detect endianness
> +	 */
> +	tcsr0 = xilinx_timer_read(priv, TCSR0);
> +	if (swab32(tcsr0) & TCSR_ENIT) {
> +		ret = xilinx_timer_err(np, -EOPNOTSUPP,
> +				       "cannot determine endianness\n");
> +		goto err_priv;
> +	}
> +
> +	xilinx_timer_write(priv, tcsr0 | TCSR_ENIT, TCSR0);
> +	if (!(xilinx_timer_read(priv, TCSR0) & TCSR_ENIT)) {
> +		priv->read = xilinx_ioread32be;
> +		priv->write = xilinx_iowrite32be;
> +	}
> +
> +	/*
> +	 * For backwards compatibility, allow xlnx,one-timer-only = <bool>;
> +	 * However, the preferred way is to use the xlnx,single-timer flag.
> +	 */
> +	one_timer = of_property_read_bool(np, "xlnx,single-timer");
> +	if (!one_timer) {
> +		ret = of_property_read_u32(np, "xlnx,one-timer-only", &one_timer);
> +		if (ret) {
> +			ret = xilinx_timer_err(np, ret, "xlnx,one-timer-only");
> +			goto err_priv;
> +		}
> +	}
> +
> +	pwm = of_property_read_bool(np, "xlnx,pwm");
> +	if (one_timer && pwm) {
> +		ret = xilinx_timer_err(np, -EINVAL,
> +				       "pwm mode not possible with one timer\n");
> +		goto err_priv;
> +	}
> +
> +	priv->flags = FIELD_PREP(XILINX_TIMER_ONE, one_timer) |
> +		      FIELD_PREP(XILINX_TIMER_PWM, pwm);
> +
> +	for (i = 0; pwm && i < 2; i++) {
> +		char int_fmt[] = "xlnx,gen%u-assert";
> +		char bool_fmt[] = "xlnx,gen%u-active-low";
> +		char buf[max(sizeof(int_fmt), sizeof(bool_fmt))];
> +		u32 gen;
> +
> +		/*
> +		 * Allow xlnx,gen?-assert = <bool>; for backwards
> +		 * compatibility. However, the preferred way is to use the
> +		 * xlnx,gen?-active-low flag.
> +		 */
> +		snprintf(buf, sizeof(buf), bool_fmt, i);
> +		gen = !of_property_read_bool(np, buf);
> +		if (gen) {
> +			snprintf(buf, sizeof(buf), int_fmt, i);
> +			ret = of_property_read_u32(np, buf, &gen);
> +			if (ret && ret != -EINVAL) {
> +				xilinx_timer_err(np, ret, "%s\n", buf);
> +				goto err_priv;
> +			}
> +		}
> +
> +		if (!gen) {
> +			ret = xilinx_timer_err(np, -EINVAL,
> +					       "generateout%u must be active high\n",
> +					       i);
> +			goto err_priv;
> +		}
> +	}
> +
> +	ret = of_property_read_u32(np, "xlnx,count-width", &priv->width);
> +	if (ret) {
> +		xilinx_timer_err(np, ret, "xlnx,count-width\n");
> +		goto err_priv;
> +	} else if (priv->width < 8 || priv->width > 32) {
> +		ret = xilinx_timer_err(np, -EINVAL, "invalid counter width\n");
> +		goto err_priv;
> +	}
> +
> +	priv->clk = xilinx_timer_clock_init(np, priv);
> +	if (IS_ERR(priv->clk)) {
> +		ret = xilinx_timer_err(np, PTR_ERR(priv->clk), "clock\n");
> +		goto err_priv;
> +	}
> +
> +	ret = clk_prepare_enable(priv->clk);
> +	if (ret) {
> +		xilinx_timer_err(np, ret, "clock enable failed\n");
> +		goto err_clk;
> +	}
> +	clk_rate_exclusive_get(priv->clk);
> +
> +	if (pwm) {
> +		ret = xilinx_pwm_init(dev, priv);
> +	} else {
> +		ret = xilinx_clocksource_init(priv);
> +		if (!ret && !one_timer) {
> +			ret = xilinx_clockevent_init(np, priv);
> +			if (ret)
> +				priv->flags |= XILINX_TIMER_ONE;
> +		}
> +	}
> +
> +	if (!ret)
> +		return priv;
> +
> +	clk_rate_exclusive_put(priv->clk);
> +	clk_disable_unprepare(priv->clk);
> +err_clk:
> +	if (priv->flags & XILINX_TIMER_CLK)
> +		clk_unregister_fixed_rate(priv->clk);
> +	else
> +		clk_put(priv->clk);
> +err_priv:
> +	kfree(priv);
> +	return ERR_PTR(ret);
> +}
> +
> +static int xilinx_timer_probe(struct platform_device *pdev)
> +{
> +	struct xilinx_timer_priv *priv =
> +		xilinx_timer_init(&pdev->dev, pdev->dev.of_node);
> +
> +	if (IS_ERR(priv))
> +		return PTR_ERR(priv);
> +
> +	platform_set_drvdata(pdev, priv);
> +	return 0;
> +}
> +
> +static int xilinx_timer_remove(struct platform_device *pdev)
> +{
> +	struct xilinx_timer_priv *priv = platform_get_drvdata(pdev);
> +
> +	if (IS_ENABLED(CONFIG_XILINX_PWM) && priv->flags & XILINX_TIMER_PWM) {
> +		pwmchip_remove(&priv->pwm);
> +	} else {
> +		if (!(priv->flags & XILINX_TIMER_ONE)) {
> +			int cpu;
> +
> +			for_each_cpu(cpu, priv->ce.cpumask)
> +				clockevents_unbind_device(&priv->ce, cpu);
> +		}
> +		clocksource_unregister(&priv->cs);
> +	}
> +
> +	clk_rate_exclusive_put(priv->clk);
> +	clk_disable_unprepare(priv->clk);
> +	if (priv->flags & XILINX_TIMER_CLK)
> +		clk_unregister_fixed_rate(priv->clk);
> +	else
> +		clk_put(priv->clk);
> +	return 0;
> +}
> +
> +static const struct of_device_id xilinx_timer_of_match[] = {
> +	{ .compatible = "xlnx,xps-timer-1.00.a", },
> +	{ .compatible = "xlnx,axi-timer-2.0" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, xilinx_timer_of_match);
> +
> +static struct platform_driver xilinx_timer_driver = {
> +	.probe = xilinx_timer_probe,
> +	.remove = xilinx_timer_remove,
> +	.driver = {
> +		.name = "xilinx-timer",
> +		.of_match_table = of_match_ptr(xilinx_timer_of_match),
> +	},
> +};
> +module_platform_driver(xilinx_timer_driver);
> +
> +static struct xilinx_timer_priv *xilinx_sched = (void *)-EAGAIN;
> +
> +static u64 xilinx_sched_read(void)
> +{
> +	return xilinx_timer_read(xilinx_sched, TCSR0);

This should be TCR0.

--Sean

> +}
> +
> +static int __init xilinx_timer_register(struct device_node *np)
> +{
> +	struct xilinx_timer_priv *priv;
> +
> +	if (xilinx_sched != ERR_PTR(-EAGAIN))
> +		return -EPROBE_DEFER;
> +
> +	priv = xilinx_timer_init(NULL, np);
> +	if (IS_ERR(priv))
> +		return PTR_ERR(priv);
> +	of_node_set_flag(np, OF_POPULATED);
> +
> +	xilinx_sched = priv;
> +	sched_clock_register(xilinx_sched_read, priv->width,
> +			     clk_get_rate(priv->clk));
> +	return 0;
> +}
> +
> +TIMER_OF_DECLARE(xilinx_xps_timer, "xlnx,xps-timer-1.00.a", xilinx_timer_register);
> +TIMER_OF_DECLARE(xilinx_axi_timer, "xlnx,axi-timer-2.0", xilinx_timer_register);
> +
> +MODULE_ALIAS("platform:xilinx-timer");
> +MODULE_DESCRIPTION("Xilinx LogiCORE IP AXI Timer driver");
> +MODULE_LICENSE("GPL v2");
>

Hi Sean,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on tip/timers/core]
[also build test ERROR on pwm/for-next linux/master linus/master v5.13-rc1 next-20210511]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Sean-Anderson/dt-bindings-pwm-Add-Xilinx-AXI-Timer/20210512-031347
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git 2d036dfa5f10df9782f5278fc591d79d283c1fad
config: mips-randconfig-r012-20210512 (attached as .config)
compiler: mips64el-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/8e9195e8da4d8ea7fd88d14fd95d95ba21008aef
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Sean-Anderson/dt-bindings-pwm-Add-Xilinx-AXI-Timer/20210512-031347
        git checkout 8e9195e8da4d8ea7fd88d14fd95d95ba21008aef
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross W=1 ARCH=mips 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All error/warnings (new ones prefixed by >>):

   drivers/clocksource/timer-xilinx.c: In function 'xilinx_ioread32be':
>> drivers/clocksource/timer-xilinx.c:136:20: warning: passing argument 1 of 'ioread32be' discards 'volatile' qualifier from pointer target type [-Wdiscarded-qualifiers]

     136 |  return ioread32be(addr);
         |                    ^~~~
   In file included from arch/mips/include/asm/io.h:28,
                    from arch/mips/include/asm/mmiowb.h:5,
                    from include/linux/spinlock.h:61,
                    from include/linux/rwsem.h:16,
                    from include/linux/notifier.h:15,
                    from include/linux/clk.h:14,
                    from drivers/clocksource/timer-xilinx.c:16:
   include/asm-generic/iomap.h:33:32: note: expected 'const void *' but argument is of type 'const volatile void *'
      33 | extern unsigned int ioread32be(const void __iomem *);
         |                                ^~~~~~~~~~~~~~~~~~~~
   drivers/clocksource/timer-xilinx.c: In function 'xilinx_iowrite32be':
>> drivers/clocksource/timer-xilinx.c:141:21: warning: passing argument 2 of 'iowrite32be' discards 'volatile' qualifier from pointer target type [-Wdiscarded-qualifiers]

     141 |  iowrite32be(value, addr);
         |                     ^~~~
   In file included from arch/mips/include/asm/io.h:28,
                    from arch/mips/include/asm/mmiowb.h:5,
                    from include/linux/spinlock.h:61,
                    from include/linux/rwsem.h:16,
                    from include/linux/notifier.h:15,
                    from include/linux/clk.h:14,
                    from drivers/clocksource/timer-xilinx.c:16:
   include/asm-generic/iomap.h:54:30: note: expected 'void *' but argument is of type 'volatile void *'
      54 | extern void iowrite32be(u32, void __iomem *);
         |                              ^~~~~~~~~~~~~~
   drivers/clocksource/timer-xilinx.c: In function 'xilinx_timer_init':
>> drivers/clocksource/timer-xilinx.c:447:13: error: assignment to 'u32 (*)(const volatile void *)' {aka 'unsigned int (*)(const volatile void *)'} from incompatible pointer type 'unsigned int (*)(const void *)' [-Werror=incompatible-pointer-types]

     447 |  priv->read = ioread32;
         |             ^
>> drivers/clocksource/timer-xilinx.c:448:14: error: assignment to 'void (*)(u32,  volatile void *)' {aka 'void (*)(unsigned int,  volatile void *)'} from incompatible pointer type 'void (*)(u32,  void *)' {aka 'void (*)(unsigned int,  void *)'} [-Werror=incompatible-pointer-types]

     448 |  priv->write = iowrite32;
         |              ^
   cc1: some warnings being treated as errors


vim +447 drivers/clocksource/timer-xilinx.c

   425	
   426	static struct xilinx_timer_priv *xilinx_timer_init(struct device *dev,
   427							   struct device_node *np)
   428	{
   429		bool pwm;
   430		int i, ret;
   431		struct xilinx_timer_priv *priv;
   432		u32 one_timer, tcsr0;
   433	
   434		priv = kzalloc(sizeof(*priv), GFP_KERNEL);
   435		if (!priv)
   436			return ERR_PTR(-ENOMEM);
   437	
   438		priv->regs = of_iomap(np, 0);
   439		if (!priv->regs) {
   440			ret = -ENXIO;
   441			goto err_priv;
   442		} else if (IS_ERR(priv->regs)) {
   443			ret = PTR_ERR(priv->regs);
   444			goto err_priv;
   445		}
   446	
 > 447		priv->read = ioread32;

 > 448		priv->write = iowrite32;

   449		/*
   450		 * We aren't using the interrupts yet, so use ENIT to detect endianness
   451		 */
   452		tcsr0 = xilinx_timer_read(priv, TCSR0);
   453		if (swab32(tcsr0) & TCSR_ENIT) {
   454			ret = xilinx_timer_err(np, -EOPNOTSUPP,
   455					       "cannot determine endianness\n");
   456			goto err_priv;
   457		}
   458	
   459		xilinx_timer_write(priv, tcsr0 | TCSR_ENIT, TCSR0);
   460		if (!(xilinx_timer_read(priv, TCSR0) & TCSR_ENIT)) {
   461			priv->read = xilinx_ioread32be;
   462			priv->write = xilinx_iowrite32be;
   463		}
   464	
   465		/*
   466		 * For backwards compatibility, allow xlnx,one-timer-only = <bool>;
   467		 * However, the preferred way is to use the xlnx,single-timer flag.
   468		 */
   469		one_timer = of_property_read_bool(np, "xlnx,single-timer");
   470		if (!one_timer) {
   471			ret = of_property_read_u32(np, "xlnx,one-timer-only", &one_timer);
   472			if (ret) {
   473				ret = xilinx_timer_err(np, ret, "xlnx,one-timer-only");
   474				goto err_priv;
   475			}
   476		}
   477	
   478		pwm = of_property_read_bool(np, "xlnx,pwm");
   479		if (one_timer && pwm) {
   480			ret = xilinx_timer_err(np, -EINVAL,
   481					       "pwm mode not possible with one timer\n");
   482			goto err_priv;
   483		}
   484	
   485		priv->flags = FIELD_PREP(XILINX_TIMER_ONE, one_timer) |
   486			      FIELD_PREP(XILINX_TIMER_PWM, pwm);
   487	
   488		for (i = 0; pwm && i < 2; i++) {
   489			char int_fmt[] = "xlnx,gen%u-assert";
   490			char bool_fmt[] = "xlnx,gen%u-active-low";
   491			char buf[max(sizeof(int_fmt), sizeof(bool_fmt))];
   492			u32 gen;
   493	
   494			/*
   495			 * Allow xlnx,gen?-assert = <bool>; for backwards
   496			 * compatibility. However, the preferred way is to use the
   497			 * xlnx,gen?-active-low flag.
   498			 */
   499			snprintf(buf, sizeof(buf), bool_fmt, i);
   500			gen = !of_property_read_bool(np, buf);
   501			if (gen) {
   502				snprintf(buf, sizeof(buf), int_fmt, i);
   503				ret = of_property_read_u32(np, buf, &gen);
   504				if (ret && ret != -EINVAL) {
   505					xilinx_timer_err(np, ret, "%s\n", buf);
   506					goto err_priv;
   507				}
   508			}
   509	
   510			if (!gen) {
   511				ret = xilinx_timer_err(np, -EINVAL,
   512						       "generateout%u must be active high\n",
   513						       i);
   514				goto err_priv;
   515			}
   516		}
   517	
   518		ret = of_property_read_u32(np, "xlnx,count-width", &priv->width);
   519		if (ret) {
   520			xilinx_timer_err(np, ret, "xlnx,count-width\n");
   521			goto err_priv;
   522		} else if (priv->width < 8 || priv->width > 32) {
   523			ret = xilinx_timer_err(np, -EINVAL, "invalid counter width\n");
   524			goto err_priv;
   525		}
   526	
   527		priv->clk = xilinx_timer_clock_init(np, priv);
   528		if (IS_ERR(priv->clk)) {
   529			ret = xilinx_timer_err(np, PTR_ERR(priv->clk), "clock\n");
   530			goto err_priv;
   531		}
   532	
   533		ret = clk_prepare_enable(priv->clk);
   534		if (ret) {
   535			xilinx_timer_err(np, ret, "clock enable failed\n");
   536			goto err_clk;
   537		}
   538		clk_rate_exclusive_get(priv->clk);
   539	
   540		if (pwm) {
   541			ret = xilinx_pwm_init(dev, priv);
   542		} else {
   543			ret = xilinx_clocksource_init(priv);
   544			if (!ret && !one_timer) {
   545				ret = xilinx_clockevent_init(np, priv);
   546				if (ret)
   547					priv->flags |= XILINX_TIMER_ONE;
   548			}
   549		}
   550	
   551		if (!ret)
   552			return priv;
   553	
   554		clk_rate_exclusive_put(priv->clk);
   555		clk_disable_unprepare(priv->clk);
   556	err_clk:
   557		if (priv->flags & XILINX_TIMER_CLK)
   558			clk_unregister_fixed_rate(priv->clk);
   559		else
   560			clk_put(priv->clk);
   561	err_priv:
   562		kfree(priv);
   563		return ERR_PTR(ret);
   564	}
   565	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

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

On 5/14/21 4:59 AM, Michal Simek wrote:
 >

 >

 > 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.


What parts would you like to see split? Fundamentally, this current
patch is a reimplementation of the driver. I think the only reasonable
split would be to add PWM support in a separate patch.

I do not think that genericizing the microblaze timer driver is an
integral part of adding PWM support. This is especially since you seem
opposed to using existing devicetree properties to inform the driver. I
am inclined to just add a patch adding a check for '#-pwm-cells' to the
existing driver and otherwise leave it untouched.

 > 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.


I have not copy-pasted any code from the original driver. All of this
was written by consulting with the datasheet and other timer drivers in
the Linux kernel. In some instances I have referred to the original
driver (such as when discovering the need for detecting endianness) but
none of the original code was re-used. As it happens, since these
drivers are accomplishing the same task, some code is necessarily going
to be similar. Therefore, I have not added the copyright lines from the
original driver.

--Sean

 >

 > Thanks,

 > Michal

 >

On 5/14/21 4:40 PM, Sean Anderson wrote:
> 

> 

> On 5/14/21 4:59 AM, Michal Simek wrote:

>>

>>

>> 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.

> 

> What parts would you like to see split? Fundamentally, this current

> patch is a reimplementation of the driver. I think the only reasonable

> split would be to add PWM support in a separate patch.

> 

> I do not think that genericizing the microblaze timer driver is an

> integral part of adding PWM support. This is especially since you seem

> opposed to using existing devicetree properties to inform the driver. I

> am inclined to just add a patch adding a check for '#-pwm-cells' to the

> existing driver and otherwise leave it untouched.


As I said I think the patches should be like this.
1. Cover existing DT binding based on current code.
2. Move time out of arch/microblaze to drivers/clocksource/ and even
enable it via Kconfig just for Microblaze.
3. Remove dependency on Microblaze and enable build for others. I have
seen at least one cpuinfo.cpu_clock_freq assignment. This code can be
likely completely removed or deprecate.
4. Make driver as module
5. Do whatever changes you want before adding pwm support
6. Extend DT binding doc for PWM support
7. Add PWM support

I expect you know that some time ago we have also added support for
Microblaze SMP and this code has never been sent upstream. You should
just be aware about it.
https://github.com/Xilinx/linux-xlnx/blob/master/arch/microblaze/kernel/timer.c

Thanks,
Michal

On 5/17/21 3:54 AM, Michal Simek wrote:
 >

 >

 > On 5/14/21 4:40 PM, Sean Anderson wrote:

 >>

 >>

 >> On 5/14/21 4:59 AM, Michal Simek wrote:

 >>>

 >>>

 >>> 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.

 >>

 >> What parts would you like to see split? Fundamentally, this current

 >> patch is a reimplementation of the driver. I think the only reasonable

 >> split would be to add PWM support in a separate patch.

 >>

 >> I do not think that genericizing the microblaze timer driver is an

 >> integral part of adding PWM support. This is especially since you seem

 >> opposed to using existing devicetree properties to inform the driver. I

 >> am inclined to just add a patch adding a check for '#-pwm-cells' to the

 >> existing driver and otherwise leave it untouched.

 >

 > As I said I think the patches should be like this.

 > 1. Cover existing DT binding based on current code.

 > 2. Move time out of arch/microblaze to drivers/clocksource/ and even

 > enable it via Kconfig just for Microblaze.

 > 3. Remove dependency on Microblaze and enable build for others. I have

 > seen at least one cpuinfo.cpu_clock_freq assignment. This code can be

 > likely completely removed or deprecate.


This could be deprecated, but cannot be removed since existing device
trees (e.g. qemu) have neither clocks nor clock-frequency properties.

 > 4. Make driver as module

 > 5. Do whatever changes you want before adding pwm support

 > 6. Extend DT binding doc for PWM support

 > 7. Add PWM support


Frankly, I am inclined to just leave the microblaze timer as-is. The PWM
driver is completely independent. I have already put too much effort into
this driver, and I don't have the energy to continue working on the
microblaze timer.

--Sean

 > I expect you know that some time ago we have also added support for

 > Microblaze SMP and this code has never been sent upstream. You should

 > just be aware about it.

 > https://github.com/Xilinx/linux-xlnx/blob/master/arch/microblaze/kernel/timer.c

 >

 > Thanks,

 > Michal

 >

On 5/18/21 12:15 AM, Sean Anderson wrote:
> 

> 

> On 5/17/21 3:54 AM, Michal Simek wrote:

>>

>>

>> On 5/14/21 4:40 PM, Sean Anderson wrote:

>>>

>>>

>>> On 5/14/21 4:59 AM, Michal Simek wrote:

>>>>

>>>>

>>>> 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.

>>>

>>> What parts would you like to see split? Fundamentally, this current

>>> patch is a reimplementation of the driver. I think the only reasonable

>>> split would be to add PWM support in a separate patch.

>>>

>>> I do not think that genericizing the microblaze timer driver is an

>>> integral part of adding PWM support. This is especially since you seem

>>> opposed to using existing devicetree properties to inform the driver. I

>>> am inclined to just add a patch adding a check for '#-pwm-cells' to the

>>> existing driver and otherwise leave it untouched.

>>

>> As I said I think the patches should be like this.

>> 1. Cover existing DT binding based on current code.

>> 2. Move time out of arch/microblaze to drivers/clocksource/ and even

>> enable it via Kconfig just for Microblaze.

>> 3. Remove dependency on Microblaze and enable build for others. I have

>> seen at least one cpuinfo.cpu_clock_freq assignment. This code can be

>> likely completely removed or deprecate.

> 

> This could be deprecated, but cannot be removed since existing device

> trees (e.g. qemu) have neither clocks nor clock-frequency properties.


Rob: Do we have any obligation to keep properties for other projects?


>> 4. Make driver as module

>> 5. Do whatever changes you want before adding pwm support

>> 6. Extend DT binding doc for PWM support

>> 7. Add PWM support

> 

> Frankly, I am inclined to just leave the microblaze timer as-is. The PWM

> driver is completely independent. I have already put too much effort into

> this driver, and I don't have the energy to continue working on the

> microblaze timer.


I understand. I am actually using axi timer as pwm driver in one of my
project but never had time to upstream it because of couple of steps above.
We need to do it right based on steps listed above. If this is too much
work it will have to wait. I will NACK all attempts to add separate
driver for IP which we already support in the tree.

Thanks,
Michal

On 5/19/21 3:24 AM, Michal Simek wrote:
 >

 >

 > On 5/18/21 12:15 AM, Sean Anderson wrote:

 >>

 >>

 >> On 5/17/21 3:54 AM, Michal Simek wrote:

 >>>

 >>>

 >>> On 5/14/21 4:40 PM, Sean Anderson wrote:

 >>>>

 >>>>

 >>>> On 5/14/21 4:59 AM, Michal Simek wrote:

 >>>>>

 >>>>>

 >>>>> 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.

 >>>>

 >>>> What parts would you like to see split? Fundamentally, this current

 >>>> patch is a reimplementation of the driver. I think the only reasonable

 >>>> split would be to add PWM support in a separate patch.

 >>>>

 >>>> I do not think that genericizing the microblaze timer driver is an

 >>>> integral part of adding PWM support. This is especially since you seem

 >>>> opposed to using existing devicetree properties to inform the driver. I

 >>>> am inclined to just add a patch adding a check for '#-pwm-cells' to the

 >>>> existing driver and otherwise leave it untouched.

 >>>

 >>> As I said I think the patches should be like this.

 >>> 1. Cover existing DT binding based on current code.

 >>> 2. Move time out of arch/microblaze to drivers/clocksource/ and even

 >>> enable it via Kconfig just for Microblaze.

 >>> 3. Remove dependency on Microblaze and enable build for others. I have

 >>> seen at least one cpuinfo.cpu_clock_freq assignment. This code can be

 >>> likely completely removed or deprecate.

 >>

 >> This could be deprecated, but cannot be removed since existing device

 >> trees (e.g. qemu) have neither clocks nor clock-frequency properties.

 >

 > Rob: Do we have any obligation to keep properties for other projects?

 >

 >

 >>> 4. Make driver as module

 >>> 5. Do whatever changes you want before adding pwm support

 >>> 6. Extend DT binding doc for PWM support

 >>> 7. Add PWM support

 >>

 >> Frankly, I am inclined to just leave the microblaze timer as-is. The PWM

 >> driver is completely independent. I have already put too much effort into

 >> this driver, and I don't have the energy to continue working on the

 >> microblaze timer.

 >

 > I understand. I am actually using axi timer as pwm driver in one of my

 > project but never had time to upstream it because of couple of steps above.

 > We need to do it right based on steps listed above. If this is too much

 > work it will have to wait. I will NACK all attempts to add separate

 > driver for IP which we already support in the tree.


1. Many timers have separate clocksource and PWM drivers. E.g. samsung,
    renesas TPU, etc. It is completely reasonable to keep separate
    drivers for these purposes. There is no Linux requirement that each
    device have only one driver, especially if it has multiple functions
    or ways to be configured.
2. If you want to do work on a driver, I'm all for it. However, if you
    have not yet submitted that work to the list, you should not gate
    other work behind it. Saying that X feature must be gated behind Y
    *even if X works completely independently of Y* is just stifling
    development.
3. There is a clear desire for a PWM driver for this device. You, I, and
    Alvaro have all written separate drivers for this device because we
    want to use it as a PWM. By preventing merging this driver, you are
    encouraging duplicate effort by the next person who wants to use this
    device as a PWM, and sees that there is no driver in the tree.

--Sean

 >

 > Thanks,

 > Michal

 >

On 5/20/21 10:13 PM, Sean Anderson wrote:
> 

> 

> On 5/19/21 3:24 AM, Michal Simek wrote:

>>

>>

>> On 5/18/21 12:15 AM, Sean Anderson wrote:

>>>

>>>

>>> On 5/17/21 3:54 AM, Michal Simek wrote:

>>>>

>>>>

>>>> On 5/14/21 4:40 PM, Sean Anderson wrote:

>>>>>

>>>>>

>>>>> On 5/14/21 4:59 AM, Michal Simek wrote:

>>>>>>

>>>>>>

>>>>>> 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.

>>>>>

>>>>> What parts would you like to see split? Fundamentally, this current

>>>>> patch is a reimplementation of the driver. I think the only reasonable

>>>>> split would be to add PWM support in a separate patch.

>>>>>

>>>>> I do not think that genericizing the microblaze timer driver is an

>>>>> integral part of adding PWM support. This is especially since you seem

>>>>> opposed to using existing devicetree properties to inform the

> driver. I

>>>>> am inclined to just add a patch adding a check for '#-pwm-cells' to

> the

>>>>> existing driver and otherwise leave it untouched.

>>>>

>>>> As I said I think the patches should be like this.

>>>> 1. Cover existing DT binding based on current code.

>>>> 2. Move time out of arch/microblaze to drivers/clocksource/ and even

>>>> enable it via Kconfig just for Microblaze.

>>>> 3. Remove dependency on Microblaze and enable build for others. I have

>>>> seen at least one cpuinfo.cpu_clock_freq assignment. This code can be

>>>> likely completely removed or deprecate.

>>>

>>> This could be deprecated, but cannot be removed since existing device

>>> trees (e.g. qemu) have neither clocks nor clock-frequency properties.

>>

>> Rob: Do we have any obligation to keep properties for other projects?

>>

>>

>>>> 4. Make driver as module

>>>> 5. Do whatever changes you want before adding pwm support

>>>> 6. Extend DT binding doc for PWM support

>>>> 7. Add PWM support

>>>

>>> Frankly, I am inclined to just leave the microblaze timer as-is. The PWM

>>> driver is completely independent. I have already put too much effort

> into

>>> this driver, and I don't have the energy to continue working on the

>>> microblaze timer.

>>

>> I understand. I am actually using axi timer as pwm driver in one of my

>> project but never had time to upstream it because of couple of steps

> above.

>> We need to do it right based on steps listed above. If this is too much

>> work it will have to wait. I will NACK all attempts to add separate

>> driver for IP which we already support in the tree.

> 

> 1. Many timers have separate clocksource and PWM drivers. E.g. samsung,

>    renesas TPU, etc. It is completely reasonable to keep separate

>    drivers for these purposes. There is no Linux requirement that each

>    device have only one driver, especially if it has multiple functions

>    or ways to be configured.


It doesn't mean that it was done properly and correctly. Code
duplication is bad all the time.

> 2. If you want to do work on a driver, I'm all for it. However, if you

>    have not yet submitted that work to the list, you should not gate

>    other work behind it. Saying that X feature must be gated behind Y

>    *even if X works completely independently of Y* is just stifling

>    development.


I gave you guidance how I think this should be done. I am not gating you
from this work. Your patch is not working on Microblaze arch which is
what I maintain. And I don't want to go the route that we will have two
drivers for the same IP without integration. We were there in past and
it is just pain.
I am expecting that PWM guys will guide how this should be done
properly. I haven't heard any guidance on this yet.
Thierry/Uwe: Any comment?


> 3. There is a clear desire for a PWM driver for this device. You, I, and

>    Alvaro have all written separate drivers for this device because we

>    want to use it as a PWM. By preventing merging this driver, you are

>    encouraging duplicate effort by the next person who wants to use this

>    device as a PWM, and sees that there is no driver in the tree.


We should do it cleanly that it will be easy to maintain which is not by
creating two separate drivers or by switching to completely new driver.

Thanks,
Michal

On 5/24/21 3:00 AM, Michal Simek wrote:
 >

 >

 > On 5/20/21 10:13 PM, Sean Anderson wrote:

 >>

 >>

 >> On 5/19/21 3:24 AM, Michal Simek wrote:

 >>>

 >>>

 >>> On 5/18/21 12:15 AM, Sean Anderson wrote:

 >>>>

 >>>>

 >>>> On 5/17/21 3:54 AM, Michal Simek wrote:

 >>>>>

 >>>>>

 >>>>> On 5/14/21 4:40 PM, Sean Anderson wrote:

 >>>>>>

 >>>>>>

 >>>>>> On 5/14/21 4:59 AM, Michal Simek wrote:

 >>>>>>>

 >>>>>>>

 >>>>>>> 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.

 >>>>>>

 >>>>>> What parts would you like to see split? Fundamentally, this current

 >>>>>> patch is a reimplementation of the driver. I think the only reasonable

 >>>>>> split would be to add PWM support in a separate patch.

 >>>>>>

 >>>>>> I do not think that genericizing the microblaze timer driver is an

 >>>>>> integral part of adding PWM support. This is especially since you seem

 >>>>>> opposed to using existing devicetree properties to inform the

 >> driver. I

 >>>>>> am inclined to just add a patch adding a check for '#-pwm-cells' to

 >> the

 >>>>>> existing driver and otherwise leave it untouched.

 >>>>>

 >>>>> As I said I think the patches should be like this.

 >>>>> 1. Cover existing DT binding based on current code.

 >>>>> 2. Move time out of arch/microblaze to drivers/clocksource/ and even

 >>>>> enable it via Kconfig just for Microblaze.

 >>>>> 3. Remove dependency on Microblaze and enable build for others. I have

 >>>>> seen at least one cpuinfo.cpu_clock_freq assignment. This code can be

 >>>>> likely completely removed or deprecate.

 >>>>

 >>>> This could be deprecated, but cannot be removed since existing device

 >>>> trees (e.g. qemu) have neither clocks nor clock-frequency properties.

 >>>

 >>> Rob: Do we have any obligation to keep properties for other projects?

 >>>

 >>>

 >>>>> 4. Make driver as module

 >>>>> 5. Do whatever changes you want before adding pwm support

 >>>>> 6. Extend DT binding doc for PWM support

 >>>>> 7. Add PWM support

 >>>>

 >>>> Frankly, I am inclined to just leave the microblaze timer as-is. The PWM

 >>>> driver is completely independent. I have already put too much effort

 >> into

 >>>> this driver, and I don't have the energy to continue working on the

 >>>> microblaze timer.

 >>>

 >>> I understand. I am actually using axi timer as pwm driver in one of my

 >>> project but never had time to upstream it because of couple of steps

 >> above.

 >>> We need to do it right based on steps listed above. If this is too much

 >>> work it will have to wait. I will NACK all attempts to add separate

 >>> driver for IP which we already support in the tree.

 >>

 >> 1. Many timers have separate clocksource and PWM drivers. E.g. samsung,

 >>     renesas TPU, etc. It is completely reasonable to keep separate

 >>     drivers for these purposes. There is no Linux requirement that each

 >>     device have only one driver, especially if it has multiple functions

 >>     or ways to be configured.

 >

 > It doesn't mean that it was done properly and correctly. Code

 > duplication is bad all the time.


IMO after doing all this there is not too much which can be reused. We
can reuse the read/write functions, the TLR calculations and the
processing of xlnx,counter-width and xlnx,one-timer. The timer probe is
likely much more cleanly implemented with timer_of_init. And not having
a platform device greatly complicates the PWM probe.

 >

 >> 2. If you want to do work on a driver, I'm all for it. However, if you

 >>     have not yet submitted that work to the list, you should not gate

 >>     other work behind it. Saying that X feature must be gated behind Y

 >>     *even if X works completely independently of Y* is just stifling

 >>     development.

 >

 > I gave you guidance how I think this should be done. I am not gating you

 > from this work. Your patch is not working on Microblaze arch which is

 > what I maintain.


I tested this on Microblaze qemu. What problems do you see?

--Sean

 > And I don't want to go the route that we will have two

 > drivers for the same IP without integration. We were there in past and

 > it is just pain.

 > I am expecting that PWM guys will guide how this should be done

 > properly. I haven't heard any guidance on this yet.

 > Thierry/Uwe: Any comment?

 >

 >

 >> 3. There is a clear desire for a PWM driver for this device. You, I, and

 >>     Alvaro have all written separate drivers for this device because we

 >>     want to use it as a PWM. By preventing merging this driver, you are

 >>     encouraging duplicate effort by the next person who wants to use this

 >>     device as a PWM, and sees that there is no driver in the tree.

 >

 > We should do it cleanly that it will be easy to maintain which is not by

 > creating two separate drivers or by switching to completely new driver.

 >

 > Thanks,

 > Michal

 >

Hello Sean, hello Michal,

On Mon, May 24, 2021 at 09:00:51AM +0200, Michal Simek wrote:
> On 5/20/21 10:13 PM, Sean Anderson wrote:

> > On 5/19/21 3:24 AM, Michal Simek wrote:

> >> On 5/18/21 12:15 AM, Sean Anderson wrote:

> >>> This could be deprecated, but cannot be removed since existing device

> >>> trees (e.g. qemu) have neither clocks nor clock-frequency properties.

> >>

> >> Rob: Do we have any obligation to keep properties for other projects?


If a binding is in the wild and used to be documented, it has to stay.

> >>>> 4. Make driver as module

> >>>> 5. Do whatever changes you want before adding pwm support

> >>>> 6. Extend DT binding doc for PWM support

> >>>> 7. Add PWM support

> >>>

> >>> Frankly, I am inclined to just leave the microblaze timer as-is. The PWM

> >>> driver is completely independent. I have already put too much effort into

> >>> this driver, and I don't have the energy to continue working on the

> >>> microblaze timer.

> >>

> >> I understand. I am actually using axi timer as pwm driver in one of my

> >> project but never had time to upstream it because of couple of steps above.

> >> We need to do it right based on steps listed above. If this is too much

> >> work it will have to wait. I will NACK all attempts to add separate

> >> driver for IP which we already support in the tree.

> > 

> > 1. Many timers have separate clocksource and PWM drivers. E.g. samsung,

> >    renesas TPU, etc. It is completely reasonable to keep separate

> >    drivers for these purposes. There is no Linux requirement that each

> >    device have only one driver, especially if it has multiple functions

> >    or ways to be configured.

> 

> It doesn't mean that it was done properly and correctly. Code

> duplication is bad all the time.


IMHO it's not so much about code duplication. Yes, code duplication is
bad and should be prevented if possible. But it's more important to not
introduce surprises. So I think it should be obvious from reading the
device tree source which timer is used to provide the PWM. I don't care
much if this is from an extra property (like xilinx,provide-pwm),
overriding the compatible or some other explicit mechanism. IIUC in this
suggested patch the selection is implicit and so this isn't so nice.

> > 2. If you want to do work on a driver, I'm all for it. However, if you

> >    have not yet submitted that work to the list, you should not gate

> >    other work behind it. Saying that X feature must be gated behind Y

> >    *even if X works completely independently of Y* is just stifling

> >    development.

> 

> I gave you guidance how I think this should be done. I am not gating you

> from this work. Your patch is not working on Microblaze arch which is

> what I maintain. And I don't want to go the route that we will have two

> drivers for the same IP without integration. We were there in past and

> it is just pain.

> I am expecting that PWM guys will guide how this should be done

> properly. I haven't heard any guidance on this yet.

> Thierry/Uwe: Any comment?


Not sure I can and want to provide guidance here. This is not Perl, but
still TIMTOWTDI. If it was me who cared here, I'd look into the
auxiliary bus (Documentation/driver-api/auxiliary_bus.rst) to check if
it can help to solve this problem.
 
> > 3. There is a clear desire for a PWM driver for this device. You, I, and

> >    Alvaro have all written separate drivers for this device because we

> >    want to use it as a PWM. By preventing merging this driver, you are

> >    encouraging duplicate effort by the next person who wants to use this

> >    device as a PWM, and sees that there is no driver in the tree.

> 

> We should do it cleanly that it will be easy to maintain which is not by

> creating two separate drivers or by switching to completely new driver.


+1

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

On 5/25/21 2:11 AM, Uwe Kleine-König wrote:
 > Hello Sean, hello Michal,

 >

 > On Mon, May 24, 2021 at 09:00:51AM +0200, Michal Simek wrote:

 >> On 5/20/21 10:13 PM, Sean Anderson wrote:

 >>> On 5/19/21 3:24 AM, Michal Simek wrote:

 >>>> On 5/18/21 12:15 AM, Sean Anderson wrote:

 >>>>> This could be deprecated, but cannot be removed since existing device

 >>>>> trees (e.g. qemu) have neither clocks nor clock-frequency properties.

 >>>>

 >>>> Rob: Do we have any obligation to keep properties for other projects?

 >

 > If a binding is in the wild and used to be documented, it has to stay.

 >

 >>>>>> 4. Make driver as module

 >>>>>> 5. Do whatever changes you want before adding pwm support

 >>>>>> 6. Extend DT binding doc for PWM support

 >>>>>> 7. Add PWM support

 >>>>>

 >>>>> Frankly, I am inclined to just leave the microblaze timer as-is. The PWM

 >>>>> driver is completely independent. I have already put too much effort into

 >>>>> this driver, and I don't have the energy to continue working on the

 >>>>> microblaze timer.

 >>>>

 >>>> I understand. I am actually using axi timer as pwm driver in one of my

 >>>> project but never had time to upstream it because of couple of steps above.

 >>>> We need to do it right based on steps listed above. If this is too much

 >>>> work it will have to wait. I will NACK all attempts to add separate

 >>>> driver for IP which we already support in the tree.

 >>>

 >>> 1. Many timers have separate clocksource and PWM drivers. E.g. samsung,

 >>>     renesas TPU, etc. It is completely reasonable to keep separate

 >>>     drivers for these purposes. There is no Linux requirement that each

 >>>     device have only one driver, especially if it has multiple functions

 >>>     or ways to be configured.

 >>

 >> It doesn't mean that it was done properly and correctly. Code

 >> duplication is bad all the time.

 >

 > IMHO it's not so much about code duplication. Yes, code duplication is

 > bad and should be prevented if possible. But it's more important to not

 > introduce surprises. So I think it should be obvious from reading the

 > device tree source which timer is used to provide the PWM. I don't care

 > much if this is from an extra property (like xilinx,provide-pwm),

 > overriding the compatible or some other explicit mechanism. IIUC in this

 > suggested patch the selection is implicit and so this isn't so nice.


In this patch, the selection is by the presence of the xlnx,pwm
property. In the next revision, this will be changed to be the presence
of #pwm-cells (by the request of Rob).

 >>> 2. If you want to do work on a driver, I'm all for it. However, if you

 >>>     have not yet submitted that work to the list, you should not gate

 >>>     other work behind it. Saying that X feature must be gated behind Y

 >>>     *even if X works completely independently of Y* is just stifling

 >>>     development.

 >>

 >> I gave you guidance how I think this should be done. I am not gating you

 >> from this work. Your patch is not working on Microblaze arch which is

 >> what I maintain. And I don't want to go the route that we will have two

 >> drivers for the same IP without integration. We were there in past and

 >> it is just pain.

 >> I am expecting that PWM guys will guide how this should be done

 >> properly. I haven't heard any guidance on this yet.

 >> Thierry/Uwe: Any comment?

 >

 > Not sure I can and want to provide guidance here. This is not Perl, but

 > still TIMTOWTDI. If it was me who cared here, I'd look into the

 > auxiliary bus (Documentation/driver-api/auxiliary_bus.rst) to check if

 > it can help to solve this problem.


I don't think this is the correct solution here.

 > A key requirement for utilizing the auxiliary bus is that there is no

 > dependency on a physical bus, device, register accesses or regmap

 > support.


Since both PWM and timer drivers need register access, we cannot use the
auxiliary bus here. Further, timers must be initialized very early
during boot, before we even have devices, and cannot be unregistered.
Because of this, it only makes sense to bind one driver to the device.

 >

 >>> 3. There is a clear desire for a PWM driver for this device. You, I, and

 >>>     Alvaro have all written separate drivers for this device because we

 >>>     want to use it as a PWM. By preventing merging this driver, you are

 >>>     encouraging duplicate effort by the next person who wants to use this

 >>>     device as a PWM, and sees that there is no driver in the tree.

 >>

 >> We should do it cleanly that it will be easy to maintain which is not by

 >> creating two separate drivers or by switching to completely new driver.

 >

 > +1


Ok, then you would like me to continue my current approach where both
drivers live in the same file?

--Sean

 >

 > Best regards

 > Uwe

 >

Hi Uwe,

On 5/25/21 8:11 AM, Uwe Kleine-König wrote:
> Hello Sean, hello Michal,

> 

> On Mon, May 24, 2021 at 09:00:51AM +0200, Michal Simek wrote:

>> On 5/20/21 10:13 PM, Sean Anderson wrote:

>>> On 5/19/21 3:24 AM, Michal Simek wrote:

>>>> On 5/18/21 12:15 AM, Sean Anderson wrote:

>>>>> This could be deprecated, but cannot be removed since existing device

>>>>> trees (e.g. qemu) have neither clocks nor clock-frequency properties.

>>>>

>>>> Rob: Do we have any obligation to keep properties for other projects?

> 

> If a binding is in the wild and used to be documented, it has to stay.

> 

>>>>>> 4. Make driver as module

>>>>>> 5. Do whatever changes you want before adding pwm support

>>>>>> 6. Extend DT binding doc for PWM support

>>>>>> 7. Add PWM support

>>>>>

>>>>> Frankly, I am inclined to just leave the microblaze timer as-is. The PWM

>>>>> driver is completely independent. I have already put too much effort into

>>>>> this driver, and I don't have the energy to continue working on the

>>>>> microblaze timer.

>>>>

>>>> I understand. I am actually using axi timer as pwm driver in one of my

>>>> project but never had time to upstream it because of couple of steps above.

>>>> We need to do it right based on steps listed above. If this is too much

>>>> work it will have to wait. I will NACK all attempts to add separate

>>>> driver for IP which we already support in the tree.

>>>

>>> 1. Many timers have separate clocksource and PWM drivers. E.g. samsung,

>>>    renesas TPU, etc. It is completely reasonable to keep separate

>>>    drivers for these purposes. There is no Linux requirement that each

>>>    device have only one driver, especially if it has multiple functions

>>>    or ways to be configured.

>>

>> It doesn't mean that it was done properly and correctly. Code

>> duplication is bad all the time.

> 

> IMHO it's not so much about code duplication. Yes, code duplication is

> bad and should be prevented if possible. But it's more important to not

> introduce surprises. So I think it should be obvious from reading the

> device tree source which timer is used to provide the PWM. I don't care

> much if this is from an extra property (like xilinx,provide-pwm),

> overriding the compatible or some other explicit mechanism. IIUC in this

> suggested patch the selection is implicit and so this isn't so nice.

> 

>>> 2. If you want to do work on a driver, I'm all for it. However, if you

>>>    have not yet submitted that work to the list, you should not gate

>>>    other work behind it. Saying that X feature must be gated behind Y

>>>    *even if X works completely independently of Y* is just stifling

>>>    development.

>>

>> I gave you guidance how I think this should be done. I am not gating you

>> from this work. Your patch is not working on Microblaze arch which is

>> what I maintain. And I don't want to go the route that we will have two

>> drivers for the same IP without integration. We were there in past and

>> it is just pain.

>> I am expecting that PWM guys will guide how this should be done

>> properly. I haven't heard any guidance on this yet.

>> Thierry/Uwe: Any comment?

> 

> Not sure I can and want to provide guidance here. This is not Perl, but

> still TIMTOWTDI. If it was me who cared here, I'd look into the

> auxiliary bus (Documentation/driver-api/auxiliary_bus.rst) to check if

> it can help to solve this problem.


I recently got patches for cadence TTC driver
(drivers/clocksource/timer-cadence-ttc.c) for PWM support too. It is the
second and very similar case. This driver is used on Zynq as clock
source and can be also use as PWM. I can't believe that there are no
other examples how to deal with these timers which are used for PWM
generation.

Thanks,
Michal

On 6/16/21 8:12 AM, Michal Simek wrote:
 > Hi Uwe,

 >

 > On 5/25/21 8:11 AM, Uwe Kleine-König wrote:

 >> Hello Sean, hello Michal,

 >>

 >> On Mon, May 24, 2021 at 09:00:51AM +0200, Michal Simek wrote:

 >>> On 5/20/21 10:13 PM, Sean Anderson wrote:

 >>>> On 5/19/21 3:24 AM, Michal Simek wrote:

 >>>>> On 5/18/21 12:15 AM, Sean Anderson wrote:

 >>>>>> This could be deprecated, but cannot be removed since existing device

 >>>>>> trees (e.g. qemu) have neither clocks nor clock-frequency properties.

 >>>>>

 >>>>> Rob: Do we have any obligation to keep properties for other projects?

 >>

 >> If a binding is in the wild and used to be documented, it has to stay.

 >>

 >>>>>>> 4. Make driver as module

 >>>>>>> 5. Do whatever changes you want before adding pwm support

 >>>>>>> 6. Extend DT binding doc for PWM support

 >>>>>>> 7. Add PWM support

 >>>>>>

 >>>>>> Frankly, I am inclined to just leave the microblaze timer as-is. The PWM

 >>>>>> driver is completely independent. I have already put too much effort into

 >>>>>> this driver, and I don't have the energy to continue working on the

 >>>>>> microblaze timer.

 >>>>>

 >>>>> I understand. I am actually using axi timer as pwm driver in one of my

 >>>>> project but never had time to upstream it because of couple of steps above.

 >>>>> We need to do it right based on steps listed above. If this is too much

 >>>>> work it will have to wait. I will NACK all attempts to add separate

 >>>>> driver for IP which we already support in the tree.

 >>>>

 >>>> 1. Many timers have separate clocksource and PWM drivers. E.g. samsung,

 >>>>     renesas TPU, etc. It is completely reasonable to keep separate

 >>>>     drivers for these purposes. There is no Linux requirement that each

 >>>>     device have only one driver, especially if it has multiple functions

 >>>>     or ways to be configured.

 >>>

 >>> It doesn't mean that it was done properly and correctly. Code

 >>> duplication is bad all the time.

 >>

 >> IMHO it's not so much about code duplication. Yes, code duplication is

 >> bad and should be prevented if possible. But it's more important to not

 >> introduce surprises. So I think it should be obvious from reading the

 >> device tree source which timer is used to provide the PWM. I don't care

 >> much if this is from an extra property (like xilinx,provide-pwm),

 >> overriding the compatible or some other explicit mechanism. IIUC in this

 >> suggested patch the selection is implicit and so this isn't so nice.

 >>

 >>>> 2. If you want to do work on a driver, I'm all for it. However, if you

 >>>>     have not yet submitted that work to the list, you should not gate

 >>>>     other work behind it. Saying that X feature must be gated behind Y

 >>>>     *even if X works completely independently of Y* is just stifling

 >>>>     development.

 >>>

 >>> I gave you guidance how I think this should be done. I am not gating you

 >>> from this work. Your patch is not working on Microblaze arch which is

 >>> what I maintain. And I don't want to go the route that we will have two

 >>> drivers for the same IP without integration. We were there in past and

 >>> it is just pain.

 >>> I am expecting that PWM guys will guide how this should be done

 >>> properly. I haven't heard any guidance on this yet.

 >>> Thierry/Uwe: Any comment?

 >>

 >> Not sure I can and want to provide guidance here. This is not Perl, but

 >> still TIMTOWTDI. If it was me who cared here, I'd look into the

 >> auxiliary bus (Documentation/driver-api/auxiliary_bus.rst) to check if

 >> it can help to solve this problem.

 >

 > I recently got patches for cadence TTC driver

 > (drivers/clocksource/timer-cadence-ttc.c) for PWM support too. It is the

 > second and very similar case. This driver is used on Zynq as clock

 > source and can be also use as PWM. I can't believe that there are no

 > other examples how to deal with these timers which are used for PWM

 > generation.

 >


The approach I took in v4 is that probe functions and driver callbacks
live in drivers/timer and drivers/pwm, and common functions live in
drivers/mfd (although I may move them to drivers/timer since Lee Jones
doesn't like them there).

I would greatly appreciate if you could review v4. It has been on the
list for three weeks now with no comments from either you or Uwe.

--Sean

Hi,

On 6/18/21 11:24 PM, Sean Anderson wrote:
> 

> 

> On 6/16/21 8:12 AM, Michal Simek wrote:

>> Hi Uwe,

>>

>> On 5/25/21 8:11 AM, Uwe Kleine-König wrote:

>>> Hello Sean, hello Michal,

>>>

>>> On Mon, May 24, 2021 at 09:00:51AM +0200, Michal Simek wrote:

>>>> On 5/20/21 10:13 PM, Sean Anderson wrote:

>>>>> On 5/19/21 3:24 AM, Michal Simek wrote:

>>>>>> On 5/18/21 12:15 AM, Sean Anderson wrote:

>>>>>>> This could be deprecated, but cannot be removed since existing

> device

>>>>>>> trees (e.g. qemu) have neither clocks nor clock-frequency

> properties.

>>>>>>

>>>>>> Rob: Do we have any obligation to keep properties for other projects?

>>>

>>> If a binding is in the wild and used to be documented, it has to stay.

>>>

>>>>>>>> 4. Make driver as module

>>>>>>>> 5. Do whatever changes you want before adding pwm support

>>>>>>>> 6. Extend DT binding doc for PWM support

>>>>>>>> 7. Add PWM support

>>>>>>>

>>>>>>> Frankly, I am inclined to just leave the microblaze timer as-is.

> The PWM

>>>>>>> driver is completely independent. I have already put too much

> effort into

>>>>>>> this driver, and I don't have the energy to continue working on the

>>>>>>> microblaze timer.

>>>>>>

>>>>>> I understand. I am actually using axi timer as pwm driver in one

> of my

>>>>>> project but never had time to upstream it because of couple of

> steps above.

>>>>>> We need to do it right based on steps listed above. If this is too

> much

>>>>>> work it will have to wait. I will NACK all attempts to add separate

>>>>>> driver for IP which we already support in the tree.

>>>>>

>>>>> 1. Many timers have separate clocksource and PWM drivers. E.g.

> samsung,

>>>>>     renesas TPU, etc. It is completely reasonable to keep separate

>>>>>     drivers for these purposes. There is no Linux requirement that

> each

>>>>>     device have only one driver, especially if it has multiple

> functions

>>>>>     or ways to be configured.

>>>>

>>>> It doesn't mean that it was done properly and correctly. Code

>>>> duplication is bad all the time.

>>>

>>> IMHO it's not so much about code duplication. Yes, code duplication is

>>> bad and should be prevented if possible. But it's more important to not

>>> introduce surprises. So I think it should be obvious from reading the

>>> device tree source which timer is used to provide the PWM. I don't care

>>> much if this is from an extra property (like xilinx,provide-pwm),

>>> overriding the compatible or some other explicit mechanism. IIUC in this

>>> suggested patch the selection is implicit and so this isn't so nice.

>>>

>>>>> 2. If you want to do work on a driver, I'm all for it. However, if you

>>>>>     have not yet submitted that work to the list, you should not gate

>>>>>     other work behind it. Saying that X feature must be gated behind Y

>>>>>     *even if X works completely independently of Y* is just stifling

>>>>>     development.

>>>>

>>>> I gave you guidance how I think this should be done. I am not gating

> you

>>>> from this work. Your patch is not working on Microblaze arch which is

>>>> what I maintain. And I don't want to go the route that we will have two

>>>> drivers for the same IP without integration. We were there in past and

>>>> it is just pain.

>>>> I am expecting that PWM guys will guide how this should be done

>>>> properly. I haven't heard any guidance on this yet.

>>>> Thierry/Uwe: Any comment?

>>>

>>> Not sure I can and want to provide guidance here. This is not Perl, but

>>> still TIMTOWTDI. If it was me who cared here, I'd look into the

>>> auxiliary bus (Documentation/driver-api/auxiliary_bus.rst) to check if

>>> it can help to solve this problem.

>>

>> I recently got patches for cadence TTC driver

>> (drivers/clocksource/timer-cadence-ttc.c) for PWM support too. It is the

>> second and very similar case. This driver is used on Zynq as clock

>> source and can be also use as PWM. I can't believe that there are no

>> other examples how to deal with these timers which are used for PWM

>> generation.

>>

> 

> The approach I took in v4 is that probe functions and driver callbacks

> live in drivers/timer and drivers/pwm, and common functions live in

> drivers/mfd (although I may move them to drivers/timer since Lee Jones

> doesn't like them there).

> 

> I would greatly appreciate if you could review v4. It has been on the

> list for three weeks now with no comments from either you or Uwe.


I will take a look at it next week.

Thanks,
Michal