diff mbox series

[v3,1/2] dt-bindings: pwm: Add Xilinx AXI Timer

Message ID 20210511191239.774570-1-sean.anderson@seco.com
State Changes Requested
Headers show
Series [v3,1/2] dt-bindings: pwm: Add Xilinx AXI Timer | expand

Checks

Context Check Description
robh/checkpatch success
robh/dt-meta-schema fail build log

Commit Message

Sean Anderson May 11, 2021, 7:12 p.m. UTC
This adds a binding for the Xilinx LogiCORE IP AXI Timer. This device is
a "soft" block, so it has many parameters which would not be
configurable in most hardware. This binding is usually automatically
generated by Xilinx's tools, so the names and values of some properties
must be kept as they are. Replacement properties have been provided for
new device trees.

Because we need to init timer devices so early in boot, the easiest way
to configure things is to use a device tree property. For the moment
this is 'xlnx,pwm', but this could be extended/renamed/etc. in the
future if these is a need for a generic property.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---
How should the clocking situation be documented? For the moment I have
just left clock as optional, but should clock-frequency be documented?

Changes in v3:
- Mark all boolean-as-int properties as deprecated
- Add xlnx,pwm and xlnx,gen?-active-low properties.
- Make newer replacement properties mutually-exclusive with what they
  replace
- Add an example with non-deprecated properties only.

Changes in v2:
- Use 32-bit addresses for example binding

 .../bindings/pwm/xlnx,axi-timer.yaml          | 142 ++++++++++++++++++
 1 file changed, 142 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/xlnx,axi-timer.yaml

Comments

Sean Anderson May 11, 2021, 7:19 p.m. UTC | #1
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");
>
Rob Herring May 12, 2021, 6:35 p.m. UTC | #2
On Tue, 11 May 2021 15:12:37 -0400, Sean Anderson wrote:
> This adds a binding for the Xilinx LogiCORE IP AXI Timer. This device is
> a "soft" block, so it has many parameters which would not be
> configurable in most hardware. This binding is usually automatically
> generated by Xilinx's tools, so the names and values of some properties
> must be kept as they are. Replacement properties have been provided for
> new device trees.
> 
> Because we need to init timer devices so early in boot, the easiest way
> to configure things is to use a device tree property. For the moment
> this is 'xlnx,pwm', but this could be extended/renamed/etc. in the
> future if these is a need for a generic property.
> 
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> ---
> How should the clocking situation be documented? For the moment I have
> just left clock as optional, but should clock-frequency be documented?
> 
> Changes in v3:
> - Mark all boolean-as-int properties as deprecated
> - Add xlnx,pwm and xlnx,gen?-active-low properties.
> - Make newer replacement properties mutually-exclusive with what they
>   replace
> - Add an example with non-deprecated properties only.
> 
> Changes in v2:
> - Use 32-bit addresses for example binding
> 
>  .../bindings/pwm/xlnx,axi-timer.yaml          | 142 ++++++++++++++++++
>  1 file changed, 142 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/xlnx,axi-timer.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:
./Documentation/devicetree/bindings/pwm/xlnx,axi-timer.yaml:16:9: [warning] wrong indentation: expected 10 but found 8 (indentation)

dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/pwm/xlnx,axi-timer.example.dts:49.37-57.11: ERROR (duplicate_label): /example-1/timer@800e0000: Duplicate label 'axi_timer_0' on /example-1/timer@800e0000 and /example-0/timer@800e0000
ERROR: Input tree has errors, aborting (use -f to force output)
make[1]: *** [scripts/Makefile.lib:380: Documentation/devicetree/bindings/pwm/xlnx,axi-timer.example.dt.yaml] Error 2
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:1416: dt_binding_check] Error 2

See https://patchwork.ozlabs.org/patch/1477288

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.
Rob Herring May 13, 2021, 2:16 a.m. UTC | #3
On Tue, May 11, 2021 at 03:12:37PM -0400, Sean Anderson wrote:
> This adds a binding for the Xilinx LogiCORE IP AXI Timer. This device is
> a "soft" block, so it has many parameters which would not be
> configurable in most hardware. This binding is usually automatically
> generated by Xilinx's tools, so the names and values of some properties
> must be kept as they are. Replacement properties have been provided for
> new device trees.

Because you have some tool generating properties is not a reason we have 
to accept them upstream. 'deprecated' is for what *we* have deprecated.

In this case, I don't really see the point in defining new properties 
just to have bool.

> 
> Because we need to init timer devices so early in boot, the easiest way
> to configure things is to use a device tree property. For the moment
> this is 'xlnx,pwm', but this could be extended/renamed/etc. in the
> future if these is a need for a generic property.

No...

> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> ---
> How should the clocking situation be documented? For the moment I have
> just left clock as optional, but should clock-frequency be documented?
> 
> Changes in v3:
> - Mark all boolean-as-int properties as deprecated
> - Add xlnx,pwm and xlnx,gen?-active-low properties.
> - Make newer replacement properties mutually-exclusive with what they
>   replace
> - Add an example with non-deprecated properties only.
> 
> Changes in v2:
> - Use 32-bit addresses for example binding
> 
>  .../bindings/pwm/xlnx,axi-timer.yaml          | 142 ++++++++++++++++++
>  1 file changed, 142 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/xlnx,axi-timer.yaml
> 
> diff --git a/Documentation/devicetree/bindings/pwm/xlnx,axi-timer.yaml b/Documentation/devicetree/bindings/pwm/xlnx,axi-timer.yaml
> new file mode 100644
> index 000000000000..a5e90658e31a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/xlnx,axi-timer.yaml
> @@ -0,0 +1,142 @@
> +# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pwm/xlnx,axi-timer.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Xilinx LogiCORE IP AXI Timer Device Tree Binding
> +
> +maintainers:
> +  - Sean Anderson <sean.anderson@seco.com>
> +
> +properties:
> +  compatible:
> +    oneOf:
> +      - items:
> +        - const: xlnx,axi-timer-2.0
> +        - const: xlnx,xps-timer-1.00.a
> +      - const: xlnx,xps-timer-1.00.a
> +
> +  clocks:
> +    maxItems: 1
> +
> +  clock-names:
> +    const: s_axi_aclk
> +
> +  reg:
> +    maxItems: 1
> +
> +  xlnx,count-width:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 8
> +    maximum: 32
> +    description:
> +      The width of the counter(s), in bits.
> +
> +  xlnx,gen0-assert:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [ 0, 1 ]
> +    default: 1
> +    deprecated: true
> +    description:
> +      The polarity of the generateout0 signal. 0 for active-low, 1 for active-high.
> +
> +  xlnx,gen0-active-low:
> +    $ref: /schemas/types.yaml#/definitions/flag
> +    description:
> +      The generate0 signal is active-low.
> +
> +  xlnx,gen1-assert:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [ 0, 1 ]
> +    default: 1
> +    deprecated: true
> +    description:
> +      The polarity of the generateout1 signal. 0 for active-low, 1 for active-high.
> +
> +  xlnx,gen1-active-low:
> +    $ref: /schemas/types.yaml#/definitions/flag
> +    description:
> +      The generate1 signal is active-low.
> +
> +  xlnx,one-timer-only:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [ 0, 1 ]
> +    deprecated: true
> +    description:
> +      Whether only one timer is present in this block.
> +
> +  xlnx,single-timer:
> +    $ref: /schemas/types.yaml#/definitions/flag
> +    description:
> +      Only one timer is present in this block.
> +
> +  xlnx,pwm:
> +    $ref: /schemas/types.yaml#/definitions/flag
> +    description:
> +      This timer should be configured as a PWM.

If a PWM, perhaps you want a '#pwm-cells' property which can serve as 
the hint to configure as a PWM.

> +
> +required:
> +  - compatible
> +  - reg
> +  - xlnx,count-width
> +
> +allOf:
> +  - if:
> +      required:
> +        - clocks
> +    then:
> +      required:
> +        - clock-names
> +
> +  - if:
> +      required:
> +        - xlnx,gen0-active-low
> +    then:
> +      not:
> +        required:
> +          - xlnx,gen0-assert
> +
> +  - if:
> +      required:
> +        - xlnx,gen0-active-low
> +    then:
> +      not:
> +        required:
> +          - xlnx,gen0-assert
> +
> +  - if:
> +      required:
> +        - xlnx,one-timer-only
> +    then:
> +      not:
> +        required:
> +          - xlnx,single-timer
> +
> +additionalProperties: true
> +
> +examples:
> +  - |
> +    axi_timer_0: timer@800e0000 {
> +        clock-names = "s_axi_aclk";
> +        clocks = <&zynqmp_clk 71>;
> +        compatible = "xlnx,axi-timer-2.0", "xlnx,xps-timer-1.00.a";
> +        reg = <0x800e0000 0x10000>;
> +        xlnx,count-width = <0x20>;
> +        xlnx,gen0-assert = <0x1>;
> +        xlnx,gen1-assert = <0x1>;
> +        xlnx,one-timer-only = <0x0>;
> +        xlnx,trig0-assert = <0x1>;
> +        xlnx,trig1-assert = <0x1>;
> +    };
> +
> +  - |
> +    axi_timer_0: timer@800e0000 {
> +        clock-names = "s_axi_aclk";
> +        clocks = <&zynqmp_clk 71>;
> +        compatible = "xlnx,axi-timer-2.0", "xlnx,xps-timer-1.00.a";
> +        reg = <0x800e0000 0x10000>;
> +        xlnx,count-width = <0x20>;
> +        xlnx,gen0-active-low;
> +        xlnx,single-timer;
> +    };
> -- 
> 2.25.1
>
Sean Anderson May 13, 2021, 2:33 p.m. UTC | #4
On 5/12/21 10:16 PM, Rob Herring wrote:
 > On Tue, May 11, 2021 at 03:12:37PM -0400, Sean Anderson wrote:
 >> This adds a binding for the Xilinx LogiCORE IP AXI Timer. This device is
 >> a "soft" block, so it has many parameters which would not be
 >> configurable in most hardware. This binding is usually automatically
 >> generated by Xilinx's tools, so the names and values of some properties
 >> must be kept as they are. Replacement properties have been provided for
 >> new device trees.
 >
 > Because you have some tool generating properties is not a reason we have
 > to accept them upstream.

These properties are already in arch/microblaze/boot/dts/system.dts and
in the devicetree supplied to Linux by qemu. Removing these properties
will break existing setups, which I would like to avoid.

 > 'deprecated' is for what *we* have deprecated.

Ok. I will remove that then.

 >
 > In this case, I don't really see the point in defining new properties
 > just to have bool.

I don't either, but it was requested, by Michal...

 >
 >>
 >> Because we need to init timer devices so early in boot, the easiest way
 >> to configure things is to use a device tree property. For the moment
 >> this is 'xlnx,pwm', but this could be extended/renamed/etc. in the
 >> future if these is a need for a generic property.
 >
 > No...
 >
 >> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
 >> ---
 >> How should the clocking situation be documented? For the moment I have
 >> just left clock as optional, but should clock-frequency be documented?
 >>
 >> Changes in v3:
 >> - Mark all boolean-as-int properties as deprecated
 >> - Add xlnx,pwm and xlnx,gen?-active-low properties.
 >> - Make newer replacement properties mutually-exclusive with what they
 >>    replace
 >> - Add an example with non-deprecated properties only.
 >>
 >> Changes in v2:
 >> - Use 32-bit addresses for example binding
 >>
 >>   .../bindings/pwm/xlnx,axi-timer.yaml          | 142 ++++++++++++++++++
 >>   1 file changed, 142 insertions(+)
 >>   create mode 100644 Documentation/devicetree/bindings/pwm/xlnx,axi-timer.yaml
 >>
 >> diff --git a/Documentation/devicetree/bindings/pwm/xlnx,axi-timer.yaml b/Documentation/devicetree/bindings/pwm/xlnx,axi-timer.yaml
 >> new file mode 100644
 >> index 000000000000..a5e90658e31a
 >> --- /dev/null
 >> +++ b/Documentation/devicetree/bindings/pwm/xlnx,axi-timer.yaml
 >> @@ -0,0 +1,142 @@
 >> +# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
 >> +%YAML 1.2
 >> +---
 >> +$id: http://devicetree.org/schemas/pwm/xlnx,axi-timer.yaml#
 >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
 >> +
 >> +title: Xilinx LogiCORE IP AXI Timer Device Tree Binding
 >> +
 >> +maintainers:
 >> +  - Sean Anderson <sean.anderson@seco.com>
 >> +
 >> +properties:
 >> +  compatible:
 >> +    oneOf:
 >> +      - items:
 >> +        - const: xlnx,axi-timer-2.0
 >> +        - const: xlnx,xps-timer-1.00.a
 >> +      - const: xlnx,xps-timer-1.00.a
 >> +
 >> +  clocks:
 >> +    maxItems: 1
 >> +
 >> +  clock-names:
 >> +    const: s_axi_aclk
 >> +
 >> +  reg:
 >> +    maxItems: 1
 >> +
 >> +  xlnx,count-width:
 >> +    $ref: /schemas/types.yaml#/definitions/uint32
 >> +    minimum: 8
 >> +    maximum: 32
 >> +    description:
 >> +      The width of the counter(s), in bits.
 >> +
 >> +  xlnx,gen0-assert:
 >> +    $ref: /schemas/types.yaml#/definitions/uint32
 >> +    enum: [ 0, 1 ]
 >> +    default: 1
 >> +    deprecated: true
 >> +    description:
 >> +      The polarity of the generateout0 signal. 0 for active-low, 1 for active-high.
 >> +
 >> +  xlnx,gen0-active-low:
 >> +    $ref: /schemas/types.yaml#/definitions/flag
 >> +    description:
 >> +      The generate0 signal is active-low.
 >> +
 >> +  xlnx,gen1-assert:
 >> +    $ref: /schemas/types.yaml#/definitions/uint32
 >> +    enum: [ 0, 1 ]
 >> +    default: 1
 >> +    deprecated: true
 >> +    description:
 >> +      The polarity of the generateout1 signal. 0 for active-low, 1 for active-high.
 >> +
 >> +  xlnx,gen1-active-low:
 >> +    $ref: /schemas/types.yaml#/definitions/flag
 >> +    description:
 >> +      The generate1 signal is active-low.
 >> +
 >> +  xlnx,one-timer-only:
 >> +    $ref: /schemas/types.yaml#/definitions/uint32
 >> +    enum: [ 0, 1 ]
 >> +    deprecated: true
 >> +    description:
 >> +      Whether only one timer is present in this block.
 >> +
 >> +  xlnx,single-timer:
 >> +    $ref: /schemas/types.yaml#/definitions/flag
 >> +    description:
 >> +      Only one timer is present in this block.
 >> +
 >> +  xlnx,pwm:
 >> +    $ref: /schemas/types.yaml#/definitions/flag
 >> +    description:
 >> +      This timer should be configured as a PWM.
 >
 > If a PWM, perhaps you want a '#pwm-cells' property which can serve as
 > the hint to configure as a PWM.

Ok, that's a good idea.

--Sean

 >
 >> +
 >> +required:
 >> +  - compatible
 >> +  - reg
 >> +  - xlnx,count-width
 >> +
 >> +allOf:
 >> +  - if:
 >> +      required:
 >> +        - clocks
 >> +    then:
 >> +      required:
 >> +        - clock-names
 >> +
 >> +  - if:
 >> +      required:
 >> +        - xlnx,gen0-active-low
 >> +    then:
 >> +      not:
 >> +        required:
 >> +          - xlnx,gen0-assert
 >> +
 >> +  - if:
 >> +      required:
 >> +        - xlnx,gen0-active-low
 >> +    then:
 >> +      not:
 >> +        required:
 >> +          - xlnx,gen0-assert
 >> +
 >> +  - if:
 >> +      required:
 >> +        - xlnx,one-timer-only
 >> +    then:
 >> +      not:
 >> +        required:
 >> +          - xlnx,single-timer
 >> +
 >> +additionalProperties: true
 >> +
 >> +examples:
 >> +  - |
 >> +    axi_timer_0: timer@800e0000 {
 >> +        clock-names = "s_axi_aclk";
 >> +        clocks = <&zynqmp_clk 71>;
 >> +        compatible = "xlnx,axi-timer-2.0", "xlnx,xps-timer-1.00.a";
 >> +        reg = <0x800e0000 0x10000>;
 >> +        xlnx,count-width = <0x20>;
 >> +        xlnx,gen0-assert = <0x1>;
 >> +        xlnx,gen1-assert = <0x1>;
 >> +        xlnx,one-timer-only = <0x0>;
 >> +        xlnx,trig0-assert = <0x1>;
 >> +        xlnx,trig1-assert = <0x1>;
 >> +    };
 >> +
 >> +  - |
 >> +    axi_timer_0: timer@800e0000 {
 >> +        clock-names = "s_axi_aclk";
 >> +        clocks = <&zynqmp_clk 71>;
 >> +        compatible = "xlnx,axi-timer-2.0", "xlnx,xps-timer-1.00.a";
 >> +        reg = <0x800e0000 0x10000>;
 >> +        xlnx,count-width = <0x20>;
 >> +        xlnx,gen0-active-low;
 >> +        xlnx,single-timer;
 >> +    };
 >> --
 >> 2.25.1
 >>
Sean Anderson May 13, 2021, 3:28 p.m. UTC | #5
On 5/13/21 10:33 AM, Sean Anderson wrote:
 >
 >
 > On 5/12/21 10:16 PM, Rob Herring wrote:
 >  > On Tue, May 11, 2021 at 03:12:37PM -0400, Sean Anderson wrote:
 >  >> This adds a binding for the Xilinx LogiCORE IP AXI Timer. This device is
 >  >> a "soft" block, so it has many parameters which would not be
 >  >> configurable in most hardware. This binding is usually automatically
 >  >> generated by Xilinx's tools, so the names and values of some properties
 >  >> must be kept as they are. Replacement properties have been provided for
 >  >> new device trees.
 >  >
 >  > Because you have some tool generating properties is not a reason we have
 >  > to accept them upstream.
 >
 > These properties are already in arch/microblaze/boot/dts/system.dts and
 > in the devicetree supplied to Linux by qemu. Removing these properties
 > will break existing setups, which I would like to avoid.
 >
 >  > 'deprecated' is for what *we* have deprecated.
 >
 > Ok. I will remove that then.
 >
 >  >
 >  > In this case, I don't really see the point in defining new properties
 >  > just to have bool.
 >
 > I don't either, but it was requested, by Michal...

Err, your comment on the original bindings was

 > Can't all these be boolean?

And Michal commented

 > I think in this case you should described what it is used by current
 > driver in Microblaze and these options are required. The rest are by
 > design optional.
 > If you want to change them to different value then current binding
 > should be deprecated and have any transition time with code alignment.

So that is what I tried to accomplish with this revision. I also tried
allowing something like

	xlnx,one-timer-only = <0>; /* two timers */
	xlnx,one-timer-only = <1>; /* one timer  */
	xlnx,one-timer-only; /* one timer */
	/* property absent means two timers */

but I was unable to figure out how to express this with json-schema. I
don't think it's the best design either...

--Sean

 >
 >  >
 >  >>
 >  >> Because we need to init timer devices so early in boot, the easiest way
 >  >> to configure things is to use a device tree property. For the moment
 >  >> this is 'xlnx,pwm', but this could be extended/renamed/etc. in the
 >  >> future if these is a need for a generic property.
 >  >
 >  > No...
 >  >
 >  >> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
 >  >> ---
 >  >> How should the clocking situation be documented? For the moment I have
 >  >> just left clock as optional, but should clock-frequency be documented?
 >  >>
 >  >> Changes in v3:
 >  >> - Mark all boolean-as-int properties as deprecated
 >  >> - Add xlnx,pwm and xlnx,gen?-active-low properties.
 >  >> - Make newer replacement properties mutually-exclusive with what they
 >  >>    replace
 >  >> - Add an example with non-deprecated properties only.
 >  >>
 >  >> Changes in v2:
 >  >> - Use 32-bit addresses for example binding
 >  >>
 >  >>   .../bindings/pwm/xlnx,axi-timer.yaml          | 142 ++++++++++++++++++
 >  >>   1 file changed, 142 insertions(+)
 >  >>   create mode 100644 Documentation/devicetree/bindings/pwm/xlnx,axi-timer.yaml
 >  >>
 >  >> diff --git a/Documentation/devicetree/bindings/pwm/xlnx,axi-timer.yaml b/Documentation/devicetree/bindings/pwm/xlnx,axi-timer.yaml
 >  >> new file mode 100644
 >  >> index 000000000000..a5e90658e31a
 >  >> --- /dev/null
 >  >> +++ b/Documentation/devicetree/bindings/pwm/xlnx,axi-timer.yaml
 >  >> @@ -0,0 +1,142 @@
 >  >> +# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
 >  >> +%YAML 1.2
 >  >> +---
 >  >> +$id: http://devicetree.org/schemas/pwm/xlnx,axi-timer.yaml#
 >  >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
 >  >> +
 >  >> +title: Xilinx LogiCORE IP AXI Timer Device Tree Binding
 >  >> +
 >  >> +maintainers:
 >  >> +  - Sean Anderson <sean.anderson@seco.com>
 >  >> +
 >  >> +properties:
 >  >> +  compatible:
 >  >> +    oneOf:
 >  >> +      - items:
 >  >> +        - const: xlnx,axi-timer-2.0
 >  >> +        - const: xlnx,xps-timer-1.00.a
 >  >> +      - const: xlnx,xps-timer-1.00.a
 >  >> +
 >  >> +  clocks:
 >  >> +    maxItems: 1
 >  >> +
 >  >> +  clock-names:
 >  >> +    const: s_axi_aclk
 >  >> +
 >  >> +  reg:
 >  >> +    maxItems: 1
 >  >> +
 >  >> +  xlnx,count-width:
 >  >> +    $ref: /schemas/types.yaml#/definitions/uint32
 >  >> +    minimum: 8
 >  >> +    maximum: 32
 >  >> +    description:
 >  >> +      The width of the counter(s), in bits.
 >  >> +
 >  >> +  xlnx,gen0-assert:
 >  >> +    $ref: /schemas/types.yaml#/definitions/uint32
 >  >> +    enum: [ 0, 1 ]
 >  >> +    default: 1
 >  >> +    deprecated: true
 >  >> +    description:
 >  >> +      The polarity of the generateout0 signal. 0 for active-low, 1 for active-high.
 >  >> +
 >  >> +  xlnx,gen0-active-low:
 >  >> +    $ref: /schemas/types.yaml#/definitions/flag
 >  >> +    description:
 >  >> +      The generate0 signal is active-low.
 >  >> +
 >  >> +  xlnx,gen1-assert:
 >  >> +    $ref: /schemas/types.yaml#/definitions/uint32
 >  >> +    enum: [ 0, 1 ]
 >  >> +    default: 1
 >  >> +    deprecated: true
 >  >> +    description:
 >  >> +      The polarity of the generateout1 signal. 0 for active-low, 1 for active-high.
 >  >> +
 >  >> +  xlnx,gen1-active-low:
 >  >> +    $ref: /schemas/types.yaml#/definitions/flag
 >  >> +    description:
 >  >> +      The generate1 signal is active-low.
 >  >> +
 >  >> +  xlnx,one-timer-only:
 >  >> +    $ref: /schemas/types.yaml#/definitions/uint32
 >  >> +    enum: [ 0, 1 ]
 >  >> +    deprecated: true
 >  >> +    description:
 >  >> +      Whether only one timer is present in this block.
 >  >> +
 >  >> +  xlnx,single-timer:
 >  >> +    $ref: /schemas/types.yaml#/definitions/flag
 >  >> +    description:
 >  >> +      Only one timer is present in this block.
 >  >> +
 >  >> +  xlnx,pwm:
 >  >> +    $ref: /schemas/types.yaml#/definitions/flag
 >  >> +    description:
 >  >> +      This timer should be configured as a PWM.
 >  >
 >  > If a PWM, perhaps you want a '#pwm-cells' property which can serve as
 >  > the hint to configure as a PWM.
 >
 > Ok, that's a good idea.
 >
 > --Sean
 >
 >  >
 >  >> +
 >  >> +required:
 >  >> +  - compatible
 >  >> +  - reg
 >  >> +  - xlnx,count-width
 >  >> +
 >  >> +allOf:
 >  >> +  - if:
 >  >> +      required:
 >  >> +        - clocks
 >  >> +    then:
 >  >> +      required:
 >  >> +        - clock-names
 >  >> +
 >  >> +  - if:
 >  >> +      required:
 >  >> +        - xlnx,gen0-active-low
 >  >> +    then:
 >  >> +      not:
 >  >> +        required:
 >  >> +          - xlnx,gen0-assert
 >  >> +
 >  >> +  - if:
 >  >> +      required:
 >  >> +        - xlnx,gen0-active-low
 >  >> +    then:
 >  >> +      not:
 >  >> +        required:
 >  >> +          - xlnx,gen0-assert
 >  >> +
 >  >> +  - if:
 >  >> +      required:
 >  >> +        - xlnx,one-timer-only
 >  >> +    then:
 >  >> +      not:
 >  >> +        required:
 >  >> +          - xlnx,single-timer
 >  >> +
 >  >> +additionalProperties: true
 >  >> +
 >  >> +examples:
 >  >> +  - |
 >  >> +    axi_timer_0: timer@800e0000 {
 >  >> +        clock-names = "s_axi_aclk";
 >  >> +        clocks = <&zynqmp_clk 71>;
 >  >> +        compatible = "xlnx,axi-timer-2.0", "xlnx,xps-timer-1.00.a";
 >  >> +        reg = <0x800e0000 0x10000>;
 >  >> +        xlnx,count-width = <0x20>;
 >  >> +        xlnx,gen0-assert = <0x1>;
 >  >> +        xlnx,gen1-assert = <0x1>;
 >  >> +        xlnx,one-timer-only = <0x0>;
 >  >> +        xlnx,trig0-assert = <0x1>;
 >  >> +        xlnx,trig1-assert = <0x1>;
 >  >> +    };
 >  >> +
 >  >> +  - |
 >  >> +    axi_timer_0: timer@800e0000 {
 >  >> +        clock-names = "s_axi_aclk";
 >  >> +        clocks = <&zynqmp_clk 71>;
 >  >> +        compatible = "xlnx,axi-timer-2.0", "xlnx,xps-timer-1.00.a";
 >  >> +        reg = <0x800e0000 0x10000>;
 >  >> +        xlnx,count-width = <0x20>;
 >  >> +        xlnx,gen0-active-low;
 >  >> +        xlnx,single-timer;
 >  >> +    };
 >  >> --
 >  >> 2.25.1
 >  >>
Rob Herring May 13, 2021, 8:43 p.m. UTC | #6
On Thu, May 13, 2021 at 10:28 AM Sean Anderson <sean.anderson@seco.com> wrote:
>
>
>
> On 5/13/21 10:33 AM, Sean Anderson wrote:
>  >
>  >
>  > On 5/12/21 10:16 PM, Rob Herring wrote:
>  >  > On Tue, May 11, 2021 at 03:12:37PM -0400, Sean Anderson wrote:
>  >  >> This adds a binding for the Xilinx LogiCORE IP AXI Timer. This device is
>  >  >> a "soft" block, so it has many parameters which would not be
>  >  >> configurable in most hardware. This binding is usually automatically
>  >  >> generated by Xilinx's tools, so the names and values of some properties
>  >  >> must be kept as they are. Replacement properties have been provided for
>  >  >> new device trees.
>  >  >
>  >  > Because you have some tool generating properties is not a reason we have
>  >  > to accept them upstream.
>  >
>  > These properties are already in arch/microblaze/boot/dts/system.dts and
>  > in the devicetree supplied to Linux by qemu. Removing these properties
>  > will break existing setups, which I would like to avoid.

Already in use in upstream dts files is different than just
'automatically generated' by vendor tools.

>  >
>  >  > 'deprecated' is for what *we* have deprecated.
>  >
>  > Ok. I will remove that then.
>  >
>  >  >
>  >  > In this case, I don't really see the point in defining new properties
>  >  > just to have bool.
>  >
>  > I don't either, but it was requested, by Michal...
>
> Err, your comment on the original bindings was
>
>  > Can't all these be boolean?

With no other context, yes that's what I would ask. Now you've given
me some context, between using the existing ones and 2 sets of
properties to maintain, I choose the former.

> And Michal commented
>
>  > I think in this case you should described what it is used by current
>  > driver in Microblaze and these options are required. The rest are by
>  > design optional.
>  > If you want to change them to different value then current binding
>  > should be deprecated and have any transition time with code alignment.
>
> So that is what I tried to accomplish with this revision. I also tried
> allowing something like
>
>         xlnx,one-timer-only = <0>; /* two timers */
>         xlnx,one-timer-only = <1>; /* one timer  */
>         xlnx,one-timer-only; /* one timer */
>         /* property absent means two timers */
>
> but I was unable to figure out how to express this with json-schema. I
> don't think it's the best design either...

json-schema would certainly let you, but generally we don't want
properties to have more than 1 type.

Rob
Sean Anderson May 13, 2021, 9:01 p.m. UTC | #7
On 5/13/21 4:43 PM, Rob Herring wrote:
 > On Thu, May 13, 2021 at 10:28 AM Sean Anderson <sean.anderson@seco.com> wrote:
 >>
 >>
 >>
 >> On 5/13/21 10:33 AM, Sean Anderson wrote:
 >>   >
 >>   >
 >>   > On 5/12/21 10:16 PM, Rob Herring wrote:
 >>   >  > On Tue, May 11, 2021 at 03:12:37PM -0400, Sean Anderson wrote:
 >>   >  >> This adds a binding for the Xilinx LogiCORE IP AXI Timer. This device is
 >>   >  >> a "soft" block, so it has many parameters which would not be
 >>   >  >> configurable in most hardware. This binding is usually automatically
 >>   >  >> generated by Xilinx's tools, so the names and values of some properties
 >>   >  >> must be kept as they are. Replacement properties have been provided for
 >>   >  >> new device trees.
 >>   >  >
 >>   >  > Because you have some tool generating properties is not a reason we have
 >>   >  > to accept them upstream.
 >>   >
 >>   > These properties are already in arch/microblaze/boot/dts/system.dts and
 >>   > in the devicetree supplied to Linux by qemu. Removing these properties
 >>   > will break existing setups, which I would like to avoid.
 >
 > Already in use in upstream dts files is different than just
 > 'automatically generated' by vendor tools.
 >
 >>   >
 >>   >  > 'deprecated' is for what *we* have deprecated.
 >>   >
 >>   > Ok. I will remove that then.
 >>   >
 >>   >  >
 >>   >  > In this case, I don't really see the point in defining new properties
 >>   >  > just to have bool.
 >>   >
 >>   > I don't either, but it was requested, by Michal...
 >>
 >> Err, your comment on the original bindings was
 >>
 >>   > Can't all these be boolean?
 >
 > With no other context, yes that's what I would ask. Now you've given
 > me some context, between using the existing ones and 2 sets of
 > properties to maintain, I choose the former.

Ok, then I will go with that for v4.

I noticed some another patch regarding a similar situation [1].  Would
you prefer separate files like what is done there, or is a unified file
like this ok?

[1] https://lore.kernel.org/lkml/d36e3690ce8c5a1e53d054552e4fd8b90d6a5478.1620648868.git.geert+renesas@glider.be/T/

--Sean

 >
 >> And Michal commented
 >>
 >>   > I think in this case you should described what it is used by current
 >>   > driver in Microblaze and these options are required. The rest are by
 >>   > design optional.
 >>   > If you want to change them to different value then current binding
 >>   > should be deprecated and have any transition time with code alignment.
 >>
 >> So that is what I tried to accomplish with this revision. I also tried
 >> allowing something like
 >>
 >>          xlnx,one-timer-only = <0>; /* two timers */
 >>          xlnx,one-timer-only = <1>; /* one timer  */
 >>          xlnx,one-timer-only; /* one timer */
 >>          /* property absent means two timers */
 >>
 >> but I was unable to figure out how to express this with json-schema. I
 >> don't think it's the best design either...
 >
 > json-schema would certainly let you, but generally we don't want
 > properties to have more than 1 type.
 >
 > Rob
 >
Michal Simek May 14, 2021, 8:50 a.m. UTC | #8
On 5/13/21 10:43 PM, Rob Herring wrote:
> On Thu, May 13, 2021 at 10:28 AM Sean Anderson <sean.anderson@seco.com> wrote:
>>
>>
>>
>> On 5/13/21 10:33 AM, Sean Anderson wrote:
>>  >
>>  >
>>  > On 5/12/21 10:16 PM, Rob Herring wrote:
>>  >  > On Tue, May 11, 2021 at 03:12:37PM -0400, Sean Anderson wrote:
>>  >  >> This adds a binding for the Xilinx LogiCORE IP AXI Timer. This device is
>>  >  >> a "soft" block, so it has many parameters which would not be
>>  >  >> configurable in most hardware. This binding is usually automatically
>>  >  >> generated by Xilinx's tools, so the names and values of some properties
>>  >  >> must be kept as they are. Replacement properties have been provided for
>>  >  >> new device trees.
>>  >  >
>>  >  > Because you have some tool generating properties is not a reason we have
>>  >  > to accept them upstream.
>>  >
>>  > These properties are already in arch/microblaze/boot/dts/system.dts and
>>  > in the devicetree supplied to Linux by qemu. Removing these properties
>>  > will break existing setups, which I would like to avoid.
> 
> Already in use in upstream dts files is different than just
> 'automatically generated' by vendor tools.
> 
>>  >
>>  >  > 'deprecated' is for what *we* have deprecated.
>>  >
>>  > Ok. I will remove that then.
>>  >
>>  >  >
>>  >  > In this case, I don't really see the point in defining new properties
>>  >  > just to have bool.
>>  >
>>  > I don't either, but it was requested, by Michal...
>>
>> Err, your comment on the original bindings was
>>
>>  > Can't all these be boolean?
> 
> With no other context, yes that's what I would ask. Now you've given
> me some context, between using the existing ones and 2 sets of
> properties to maintain, I choose the former.
> 
>> And Michal commented
>>
>>  > I think in this case you should described what it is used by current
>>  > driver in Microblaze and these options are required. The rest are by
>>  > design optional.
>>  > If you want to change them to different value then current binding
>>  > should be deprecated and have any transition time with code alignment.
>>
>> So that is what I tried to accomplish with this revision. I also tried
>> allowing something like
>>
>>         xlnx,one-timer-only = <0>; /* two timers */
>>         xlnx,one-timer-only = <1>; /* one timer  */
>>         xlnx,one-timer-only; /* one timer */
>>         /* property absent means two timers */
>>
>> but I was unable to figure out how to express this with json-schema. I
>> don't think it's the best design either...
> 
> json-schema would certainly let you, but generally we don't want
> properties to have more than 1 type.

One thing is what it is in system.dts file which was committed in 2009
and there are just small alignments there. But none is really using it.
Maybe I should just delete it.
And this version was generated by Xilinx ancient tools at that time. All
parameters there are fully describing HW and they are not changing. Only
new one can be added.

From the current microblaze code you can see which properties are really
used.

reg
interrupts
xlnx,one-timer-only
clocks
clock-frequency

It means from my point of view these should be listed in the binding.
clock-frequency is optional by code when clock is defined.

All other properties listed in system.dts are from my perspective
optional and that's how it should be.

I think DT binding patch should reflect this state as patch itself.
And then PWM should be added on the top as separate patch.

Note: In past we were using only parameters and name we got from tools
but over years we were fine to use for example bool properties and we
just aligned Xilinx device tree generator to match it. That's why not a
problem to deprecate any property and move to new one. Xilinx DTG is
already prepared for it and it is easy to remap it.

Thanks,
Michal
Michal Simek May 14, 2021, 8:59 a.m. UTC | #9
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
Sean Anderson May 14, 2021, 2:40 p.m. UTC | #10
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
 >
Sean Anderson May 14, 2021, 5:13 p.m. UTC | #11
On 5/14/21 4:50 AM, Michal Simek wrote:
 >
 >
 > On 5/13/21 10:43 PM, Rob Herring wrote:
 >> On Thu, May 13, 2021 at 10:28 AM Sean Anderson <sean.anderson@seco.com> wrote:
 >>>
 >>>
 >>>
 >>> On 5/13/21 10:33 AM, Sean Anderson wrote:
 >>>   >
 >>>   >
 >>>   > On 5/12/21 10:16 PM, Rob Herring wrote:
 >>>   >  > On Tue, May 11, 2021 at 03:12:37PM -0400, Sean Anderson wrote:
 >>>   >  >> This adds a binding for the Xilinx LogiCORE IP AXI Timer. This device is
 >>>   >  >> a "soft" block, so it has many parameters which would not be
 >>>   >  >> configurable in most hardware. This binding is usually automatically
 >>>   >  >> generated by Xilinx's tools, so the names and values of some properties
 >>>   >  >> must be kept as they are. Replacement properties have been provided for
 >>>   >  >> new device trees.
 >>>   >  >
 >>>   >  > Because you have some tool generating properties is not a reason we have
 >>>   >  > to accept them upstream.
 >>>   >
 >>>   > These properties are already in arch/microblaze/boot/dts/system.dts and
 >>>   > in the devicetree supplied to Linux by qemu. Removing these properties
 >>>   > will break existing setups, which I would like to avoid.
 >>
 >> Already in use in upstream dts files is different than just
 >> 'automatically generated' by vendor tools.
 >>
 >>>   >
 >>>   >  > 'deprecated' is for what *we* have deprecated.
 >>>   >
 >>>   > Ok. I will remove that then.
 >>>   >
 >>>   >  >
 >>>   >  > In this case, I don't really see the point in defining new properties
 >>>   >  > just to have bool.
 >>>   >
 >>>   > I don't either, but it was requested, by Michal...
 >>>
 >>> Err, your comment on the original bindings was
 >>>
 >>>   > Can't all these be boolean?
 >>
 >> With no other context, yes that's what I would ask. Now you've given
 >> me some context, between using the existing ones and 2 sets of
 >> properties to maintain, I choose the former.
 >>
 >>> And Michal commented
 >>>
 >>>   > I think in this case you should described what it is used by current
 >>>   > driver in Microblaze and these options are required. The rest are by
 >>>   > design optional.
 >>>   > If you want to change them to different value then current binding
 >>>   > should be deprecated and have any transition time with code alignment.
 >>>
 >>> So that is what I tried to accomplish with this revision. I also tried
 >>> allowing something like
 >>>
 >>>          xlnx,one-timer-only = <0>; /* two timers */
 >>>          xlnx,one-timer-only = <1>; /* one timer  */
 >>>          xlnx,one-timer-only; /* one timer */
 >>>          /* property absent means two timers */
 >>>
 >>> but I was unable to figure out how to express this with json-schema. I
 >>> don't think it's the best design either...
 >>
 >> json-schema would certainly let you, but generally we don't want
 >> properties to have more than 1 type.
 >
 > One thing is what it is in system.dts file which was committed in 2009
 > and there are just small alignments there. But none is really using it.
 > Maybe I should just delete it.
 > And this version was generated by Xilinx ancient tools at that time. All
 > parameters there are fully describing HW and they are not changing. Only
 > new one can be added.
 >
 >  From the current microblaze code you can see which properties are really
 > used.
 >
 > reg
 > interrupts
 > xlnx,one-timer-only
 > clocks
 > clock-frequency

There is also an implicit dependency on xlnx,count-width. Several times
the existing driver assumes the counter width is 32, but this should
instead be discovered from the devicetree.

 > It means from my point of view these should be listed in the binding.
 > clock-frequency is optional by code when clock is defined.
 >
 > All other properties listed in system.dts are from my perspective
 > optional and that's how it should be.

Here is the situation as I understand it

* This device has existed for around 15 years (since 2006)
* Because it is a soft device, there are several configurable parameters
* Although all of these parameters must be known for a complete
   implementation of this device, some are unnecessary if onlu reduced
   functionality is needed.
* A de facto devicetree binding for this device has existed for at least
   12 years (since 2009), but likely for as long as the device itself has
   existed. This binding has not changed substantially during this time.
* This binding is present in devicetrees from the Linux kernel, from
   qemu, in other existing systems, and in devicetrees generated by
   Xilinx's toolset.
* Because the existing driver for this device does not implement all
   functionality for this device, not all properties in the devicetree
   binding are used. In fact, there is (as noted above) one property
   which should be in use but is not because the current driver
   (implicitly) does not support some hardware configurations.
* To support additional functionality, it is necessary to
   use hardware parameters which were not previously necessary.

Based on the above, we can classify the properties of this binding into
several categories.

* Those which are currently read by the driver.
   * compatible
   * reg
   * clocks
   * clock-frequency
   * interrupts
   * xlnx,one-timer-only

* Those which reflect hardware parameters which are currently explicitly
   or implicitly relied upon by the driver.
   * reg
   * clocks
   * clock-frequency
   * interrupts
   * xlnx,counter-width
   * xlnx,one-timer-only

* Those which are currently present in device trees.
   * compatible
   * reg
   * interrupts
   * clocks
   * clock-frequency
   * xlnx,count-width
   * xlnx,one-timer-only
   * xlnx,trig0-assert
   * xlnx,trig1-assert
   * xlnx,gen0-assert
   * xlnx,gen1-assert

When choosing what properties to use, we must consider what the impact
of our changes will be on not just the kernel but also on existing users
of this binding:

* To use properties currently present in device trees, we just need to
   modify the kernel driver.
* To add additional properties (such as e.g. '#pwm-cells'), we must
   modify the kernel driver. In addition, users who would like to use
   these new properties must add them to their device trees. This may be
   done in a mechanical way using e.g. overlays.
* To deprecate existing properties and introduce new properties to
   expose the same underlying hardware parameters, we must modify the
   kernel driver. However, this has a large impact on existing users.
   They must modify their tools to generate this information in a
   different format. When this information is generated by upstream tools
   this may require updating a core part of their build system. For many
   projects, this may happen very infrequently because of the risk that
   such an upgrade will break things. Even if you suggest that Xilinx can
   easily modify its tools to generate any sort of output, the time for
   this upgrade to be deployed/adopted may be significantly longer.

Note that while all three types of changes are similar from a kernel
point of view, the impact on existing users is much large in the latter
case. For this reason, I think that wherever possible we should use
properties which are already present in existing device trees.

 > I think DT binding patch should reflect this state as patch itself.
 > And then PWM should be added on the top as separate patch.

I have no preference here.

--Sean

 >
 > Note: In past we were using only parameters and name we got from tools
 > but over years we were fine to use for example bool properties and we
 > just aligned Xilinx device tree generator to match it. That's why not a
 > problem to deprecate any property and move to new one. Xilinx DTG is
 > already prepared for it and it is easy to remap it.
 >
 > Thanks,
 > Michal
Michal Simek May 17, 2021, 7:54 a.m. UTC | #12
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
Michal Simek May 17, 2021, 8:28 a.m. UTC | #13
On 5/14/21 7:13 PM, Sean Anderson wrote:
> 
> 
> On 5/14/21 4:50 AM, Michal Simek wrote:
>>
>>
>> On 5/13/21 10:43 PM, Rob Herring wrote:
>>> On Thu, May 13, 2021 at 10:28 AM Sean Anderson
> <sean.anderson@seco.com> wrote:
>>>>
>>>>
>>>>
>>>> On 5/13/21 10:33 AM, Sean Anderson wrote:
>>>>   >
>>>>   >
>>>>   > On 5/12/21 10:16 PM, Rob Herring wrote:
>>>>   >  > On Tue, May 11, 2021 at 03:12:37PM -0400, Sean Anderson wrote:
>>>>   >  >> This adds a binding for the Xilinx LogiCORE IP AXI Timer.
> This device is
>>>>   >  >> a "soft" block, so it has many parameters which would not be
>>>>   >  >> configurable in most hardware. This binding is usually
> automatically
>>>>   >  >> generated by Xilinx's tools, so the names and values of some
> properties
>>>>   >  >> must be kept as they are. Replacement properties have been
> provided for
>>>>   >  >> new device trees.
>>>>   >  >
>>>>   >  > Because you have some tool generating properties is not a
> reason we have
>>>>   >  > to accept them upstream.
>>>>   >
>>>>   > These properties are already in
> arch/microblaze/boot/dts/system.dts and
>>>>   > in the devicetree supplied to Linux by qemu. Removing these
> properties
>>>>   > will break existing setups, which I would like to avoid.
>>>
>>> Already in use in upstream dts files is different than just
>>> 'automatically generated' by vendor tools.
>>>
>>>>   >
>>>>   >  > 'deprecated' is for what *we* have deprecated.
>>>>   >
>>>>   > Ok. I will remove that then.
>>>>   >
>>>>   >  >
>>>>   >  > In this case, I don't really see the point in defining new
> properties
>>>>   >  > just to have bool.
>>>>   >
>>>>   > I don't either, but it was requested, by Michal...
>>>>
>>>> Err, your comment on the original bindings was
>>>>
>>>>   > Can't all these be boolean?
>>>
>>> With no other context, yes that's what I would ask. Now you've given
>>> me some context, between using the existing ones and 2 sets of
>>> properties to maintain, I choose the former.
>>>
>>>> And Michal commented
>>>>
>>>>   > I think in this case you should described what it is used by
> current
>>>>   > driver in Microblaze and these options are required. The rest
> are by
>>>>   > design optional.
>>>>   > If you want to change them to different value then current binding
>>>>   > should be deprecated and have any transition time with code
> alignment.
>>>>
>>>> So that is what I tried to accomplish with this revision. I also tried
>>>> allowing something like
>>>>
>>>>          xlnx,one-timer-only = <0>; /* two timers */
>>>>          xlnx,one-timer-only = <1>; /* one timer  */
>>>>          xlnx,one-timer-only; /* one timer */
>>>>          /* property absent means two timers */
>>>>
>>>> but I was unable to figure out how to express this with json-schema. I
>>>> don't think it's the best design either...
>>>
>>> json-schema would certainly let you, but generally we don't want
>>> properties to have more than 1 type.
>>
>> One thing is what it is in system.dts file which was committed in 2009
>> and there are just small alignments there. But none is really using it.
>> Maybe I should just delete it.
>> And this version was generated by Xilinx ancient tools at that time. All
>> parameters there are fully describing HW and they are not changing. Only
>> new one can be added.
>>
>>  From the current microblaze code you can see which properties are really
>> used.
>>
>> reg
>> interrupts
>> xlnx,one-timer-only
>> clocks
>> clock-frequency
> 
> There is also an implicit dependency on xlnx,count-width. Several times
> the existing driver assumes the counter width is 32, but this should
> instead be discovered from the devicetree.

For me it is important what it is used now. Which is not
xlnx,count-width. That's why if you want to add it you can as optional
property.

> 
>> It means from my point of view these should be listed in the binding.
>> clock-frequency is optional by code when clock is defined.
>>
>> All other properties listed in system.dts are from my perspective
>> optional and that's how it should be.
> 
> Here is the situation as I understand it
> 
> * This device has existed for around 15 years (since 2006)
> * Because it is a soft device, there are several configurable parameters
> * Although all of these parameters must be known for a complete
>   implementation of this device, some are unnecessary if onlu reduced
>   functionality is needed.
> * A de facto devicetree binding for this device has existed for at least
>   12 years (since 2009), but likely for as long as the device itself has
>   existed. This binding has not changed substantially during this time.

note: IP itself is even much older.

> * This binding is present in devicetrees from the Linux kernel, from
>   qemu, in other existing systems, and in devicetrees generated by
>   Xilinx's toolset.

Only from Linux. Qemu is trying to reuse the same properties but it can
also add own one. They are trying to be aligned as much as possible but
there are a lot of cases where Qemu requires much more information. (I
am not saying in this timer case but in general).


> * Because the existing driver for this device does not implement all
>   functionality for this device, not all properties in the devicetree
>   binding are used. In fact, there is (as noted above) one property
>   which should be in use but is not because the current driver
>   (implicitly) does not support some hardware configurations.
> * To support additional functionality, it is necessary to
>   use hardware parameters which were not previously necessary.
> 
> Based on the above, we can classify the properties of this binding into
> several categories.
> 
> * Those which are currently read by the driver.
>   * compatible
>   * reg
>   * clocks
>   * clock-frequency
>   * interrupts
>   * xlnx,one-timer-only
> 
> * Those which reflect hardware parameters which are currently explicitly
>   or implicitly relied upon by the driver.
>   * reg
>   * clocks
>   * clock-frequency
>   * interrupts
>   * xlnx,counter-width
>   * xlnx,one-timer-only
> 
> * Those which are currently present in device trees.
>   * compatible
>   * reg
>   * interrupts
>   * clocks
>   * clock-frequency
>   * xlnx,count-width
>   * xlnx,one-timer-only
>   * xlnx,trig0-assert
>   * xlnx,trig1-assert
>   * xlnx,gen0-assert
>   * xlnx,gen1-assert
> 
> When choosing what properties to use, we must consider what the impact
> of our changes will be on not just the kernel but also on existing users
> of this binding:

I don't think that this is valid. Rob is asking for adding #pwm-cells
which is purely Linux binding. We also don't know what properties are
used by others projects not just Linux or Qemu. Also required properties
in Linux doesn't need to be required in U-Boot for example even we are
trying to aligned all of them. Another case are others RTOSes, etc.


> * To use properties currently present in device trees, we just need to
>   modify the kernel driver.
> * To add additional properties (such as e.g. '#pwm-cells'), we must
>   modify the kernel driver. In addition, users who would like to use
>   these new properties must add them to their device trees. This may be
>   done in a mechanical way using e.g. overlays.
> * To deprecate existing properties and introduce new properties to
>   expose the same underlying hardware parameters, we must modify the
>   kernel driver. However, this has a large impact on existing users.
>   They must modify their tools to generate this information in a
>   different format. When this information is generated by upstream tools
>   this may require updating a core part of their build system. For many
>   projects, this may happen very infrequently because of the risk that
>   such an upgrade will break things. Even if you suggest that Xilinx can
>   easily modify its tools to generate any sort of output, the time for
>   this upgrade to be deployed/adopted may be significantly longer.

From Xilinx perspective it would be ideal to use only properties which
fully describe HW in the form how they are generated today. They are
stable for a lot of years and as I said only new one are added.
But this alignment wasn't accepted long time ago and we have been asked
to start to align these properties with similar HW done by others.
And truth is that in a lot of cases there is clear 1:1 mapping and
generic properties can be simply use. This mapping ends in Xilinx device
tree generator.
Back to your point. Required properties are required by Linux driver
only. This driver is around for quite a long time where certain policies
haven't been setup/used/enforced (Microblaze is 2nd architecture which
started to use device tree).
We should create DT binding doc at that time but in 2009 it wasn't
standard practice. In 2007 Grant was adding support for Xilinx PPC
platform also without any DT binding document too.

That's why we need to review current unwritten DT binding based on code
requirements and look at it how to fix it (if needed) and then add PWM
support on the top of it.
If something needs to be deprecated, let's deprecate it and have
transition time for a year or so to adapt to it.

Rob knows much better than I how this should be handled.

Based on your list:
  * compatible
  * reg
  * clocks
  * clock-frequency
  * interrupts
  * xlnx,one-timer-only

all of these are required property in new DT binding.

xlnx,counter-width is optional if you want to use.
#pwm-cells is optional for enabling PWM support (I would expect that
when this property is present this timer don't need be used as
clocksource/clockevent by Microblaze)
etc

And for properties which are generated out of Xilinx tools I would allow
in DT binding to add others optional properties that DT won't error out
if they are seen.

Thanks,
Michal
Sean Anderson May 17, 2021, 2:40 p.m. UTC | #14
On 5/17/21 4:28 AM, Michal Simek wrote:
 >
 >
 > On 5/14/21 7:13 PM, Sean Anderson wrote:
 >>
 >>
 >> On 5/14/21 4:50 AM, Michal Simek wrote:
 >>>
 >>>
 >>> On 5/13/21 10:43 PM, Rob Herring wrote:
 >>>> On Thu, May 13, 2021 at 10:28 AM Sean Anderson
 >> <sean.anderson@seco.com> wrote:
 >>>>>
 >>>>>
 >>>>>
 >>>>> On 5/13/21 10:33 AM, Sean Anderson wrote:
 >>>>>     >
 >>>>>     >
 >>>>>     > On 5/12/21 10:16 PM, Rob Herring wrote:
 >>>>>     >  > On Tue, May 11, 2021 at 03:12:37PM -0400, Sean Anderson wrote:
 >>>>>     >  >> This adds a binding for the Xilinx LogiCORE IP AXI Timer.
 >> This device is
 >>>>>     >  >> a "soft" block, so it has many parameters which would not be
 >>>>>     >  >> configurable in most hardware. This binding is usually
 >> automatically
 >>>>>     >  >> generated by Xilinx's tools, so the names and values of some
 >> properties
 >>>>>     >  >> must be kept as they are. Replacement properties have been
 >> provided for
 >>>>>     >  >> new device trees.
 >>>>>     >  >
 >>>>>     >  > Because you have some tool generating properties is not a
 >> reason we have
 >>>>>     >  > to accept them upstream.
 >>>>>     >
 >>>>>     > These properties are already in
 >> arch/microblaze/boot/dts/system.dts and
 >>>>>     > in the devicetree supplied to Linux by qemu. Removing these
 >> properties
 >>>>>     > will break existing setups, which I would like to avoid.
 >>>>
 >>>> Already in use in upstream dts files is different than just
 >>>> 'automatically generated' by vendor tools.
 >>>>
 >>>>>     >
 >>>>>     >  > 'deprecated' is for what *we* have deprecated.
 >>>>>     >
 >>>>>     > Ok. I will remove that then.
 >>>>>     >
 >>>>>     >  >
 >>>>>     >  > In this case, I don't really see the point in defining new
 >> properties
 >>>>>     >  > just to have bool.
 >>>>>     >
 >>>>>     > I don't either, but it was requested, by Michal...
 >>>>>
 >>>>> Err, your comment on the original bindings was
 >>>>>
 >>>>>     > Can't all these be boolean?
 >>>>
 >>>> With no other context, yes that's what I would ask. Now you've given
 >>>> me some context, between using the existing ones and 2 sets of
 >>>> properties to maintain, I choose the former.
 >>>>
 >>>>> And Michal commented
 >>>>>
 >>>>>     > I think in this case you should described what it is used by
 >> current
 >>>>>     > driver in Microblaze and these options are required. The rest
 >> are by
 >>>>>     > design optional.
 >>>>>     > If you want to change them to different value then current binding
 >>>>>     > should be deprecated and have any transition time with code
 >> alignment.
 >>>>>
 >>>>> So that is what I tried to accomplish with this revision. I also tried
 >>>>> allowing something like
 >>>>>
 >>>>>            xlnx,one-timer-only = <0>; /* two timers */
 >>>>>            xlnx,one-timer-only = <1>; /* one timer  */
 >>>>>            xlnx,one-timer-only; /* one timer */
 >>>>>            /* property absent means two timers */
 >>>>>
 >>>>> but I was unable to figure out how to express this with json-schema. I
 >>>>> don't think it's the best design either...
 >>>>
 >>>> json-schema would certainly let you, but generally we don't want
 >>>> properties to have more than 1 type.
 >>>
 >>> One thing is what it is in system.dts file which was committed in 2009
 >>> and there are just small alignments there. But none is really using it.
 >>> Maybe I should just delete it.
 >>> And this version was generated by Xilinx ancient tools at that time. All
 >>> parameters there are fully describing HW and they are not changing. Only
 >>> new one can be added.
 >>>
 >>>    From the current microblaze code you can see which properties are really
 >>> used.
 >>>
 >>> reg
 >>> interrupts
 >>> xlnx,one-timer-only
 >>> clocks
 >>> clock-frequency
 >>
 >> There is also an implicit dependency on xlnx,count-width. Several times
 >> the existing driver assumes the counter width is 32, but this should
 >> instead be discovered from the devicetree.
 >
 > For me it is important what it is used now. Which is not
 > xlnx,count-width. That's why if you want to add it you can as optional
 > property.

At the very least we should sanity check it. E.g. check that it is 32
and return -EINVAL if it is not.

 >
 >>
 >>> It means from my point of view these should be listed in the binding.
 >>> clock-frequency is optional by code when clock is defined.
 >>>
 >>> All other properties listed in system.dts are from my perspective
 >>> optional and that's how it should be.
 >>
 >> Here is the situation as I understand it
 >>
 >> * This device has existed for around 15 years (since 2006)
 >> * Because it is a soft device, there are several configurable parameters
 >> * Although all of these parameters must be known for a complete
 >>    implementation of this device, some are unnecessary if onlu reduced
 >>    functionality is needed.
 >> * A de facto devicetree binding for this device has existed for at least
 >>    12 years (since 2009), but likely for as long as the device itself has
 >>    existed. This binding has not changed substantially during this time.
 >
 > note: IP itself is even much older.
 >
 >> * This binding is present in devicetrees from the Linux kernel, from
 >>    qemu, in other existing systems, and in devicetrees generated by
 >>    Xilinx's toolset.
 >
 > Only from Linux. Qemu is trying to reuse the same properties but it can
 > also add own one. They are trying to be aligned as much as possible but
 > there are a lot of cases where Qemu requires much more information. (I
 > am not saying in this timer case but in general).
 >
 >
 >> * Because the existing driver for this device does not implement all
 >>    functionality for this device, not all properties in the devicetree
 >>    binding are used. In fact, there is (as noted above) one property
 >>    which should be in use but is not because the current driver
 >>    (implicitly) does not support some hardware configurations.
 >> * To support additional functionality, it is necessary to
 >>    use hardware parameters which were not previously necessary.
 >>
 >> Based on the above, we can classify the properties of this binding into
 >> several categories.
 >>
 >> * Those which are currently read by the driver.
 >>    * compatible
 >>    * reg
 >>    * clocks
 >>    * clock-frequency
 >>    * interrupts
 >>    * xlnx,one-timer-only
 >>
 >> * Those which reflect hardware parameters which are currently explicitly
 >>    or implicitly relied upon by the driver.
 >>    * reg
 >>    * clocks
 >>    * clock-frequency
 >>    * interrupts
 >>    * xlnx,counter-width
 >>    * xlnx,one-timer-only
 >>
 >> * Those which are currently present in device trees.
 >>    * compatible
 >>    * reg
 >>    * interrupts
 >>    * clocks
 >>    * clock-frequency
 >>    * xlnx,count-width
 >>    * xlnx,one-timer-only
 >>    * xlnx,trig0-assert
 >>    * xlnx,trig1-assert
 >>    * xlnx,gen0-assert
 >>    * xlnx,gen1-assert
 >>
 >> When choosing what properties to use, we must consider what the impact
 >> of our changes will be on not just the kernel but also on existing users
 >> of this binding:
 >
 > I don't think that this is valid. Rob is asking for adding #pwm-cells
 > which is purely Linux binding. We also don't know what properties are
 > used by others projects not just Linux or Qemu. Also required properties
 > in Linux doesn't need to be required in U-Boot for example even we are
 > trying to aligned all of them. Another case are others RTOSes, etc.

Here I do not see a way around this. Any way we do it we will need to
have some new binding. However, as noted below, adding a new binding for
configuration is easier than exposing properties in new ways.

 >> * To use properties currently present in device trees, we just need to
 >>    modify the kernel driver.
 >> * To add additional properties (such as e.g. '#pwm-cells'), we must
 >>    modify the kernel driver. In addition, users who would like to use
 >>    these new properties must add them to their device trees. This may be
 >>    done in a mechanical way using e.g. overlays.
 >> * To deprecate existing properties and introduce new properties to
 >>    expose the same underlying hardware parameters, we must modify the
 >>    kernel driver. However, this has a large impact on existing users.
 >>    They must modify their tools to generate this information in a
 >>    different format. When this information is generated by upstream tools
 >>    this may require updating a core part of their build system. For many
 >>    projects, this may happen very infrequently because of the risk that
 >>    such an upgrade will break things. Even if you suggest that Xilinx can
 >>    easily modify its tools to generate any sort of output, the time for
 >>    this upgrade to be deployed/adopted may be significantly longer.
 >
 >  From Xilinx perspective it would be ideal to use only properties which
 > fully describe HW in the form how they are generated today. They are
 > stable for a lot of years and as I said only new one are added.
 > But this alignment wasn't accepted long time ago and we have been asked
 > to start to align these properties with similar HW done by others.
 > And truth is that in a lot of cases there is clear 1:1 mapping and
 > generic properties can be simply use. This mapping ends in Xilinx device
 > tree generator.
 > Back to your point. Required properties are required by Linux driver
 > only. This driver is around for quite a long time where certain policies
 > haven't been setup/used/enforced (Microblaze is 2nd architecture which
 > started to use device tree).
 > We should create DT binding doc at that time but in 2009 it wasn't
 > standard practice. In 2007 Grant was adding support for Xilinx PPC
 > platform also without any DT binding document too.
 >
 > That's why we need to review current unwritten DT binding based on code
 > requirements and look at it how to fix it (if needed) and then add PWM
 > support on the top of it.
 > If something needs to be deprecated, let's deprecate it and have
 > transition time for a year or so to adapt to it.
 >
 > Rob knows much better than I how this should be handled.
 >
 > Based on your list:
 >    * compatible
 >    * reg
 >    * clocks
 >    * clock-frequency
 >    * interrupts
 >    * xlnx,one-timer-only
 >
 > all of these are required property in new DT binding.
 >
 > xlnx,counter-width is optional if you want to use.

Again, for the existing driver this should at least be sanity-checked so
we fail noisily instead of buggily.

This is the situation for the generate polarity as well. The driver will
work fine, but you will not have PWM output and there will be no
indication why. This is why I think we should start with supporting the
existing output. This is used to ensure the device will work as
configured. We can also add support for different properties exposing
the same information, but to support only new properties is not very
useful.

--Sean

 > #pwm-cells is optional for enabling PWM support (I would expect that
 > when this property is present this timer don't need be used as
 > clocksource/clockevent by Microblaze)
 > etc
 >
 > And for properties which are generated out of Xilinx tools I would allow
 > in DT binding to add others optional properties that DT won't error out
 > if they are seen.
 >
 > Thanks,
 > Michal
 >
Michal Simek May 17, 2021, 2:49 p.m. UTC | #15
On 5/17/21 4:40 PM, Sean Anderson wrote:
> 
> 
> On 5/17/21 4:28 AM, Michal Simek wrote:
>>
>>
>> On 5/14/21 7:13 PM, Sean Anderson wrote:
>>>
>>>
>>> On 5/14/21 4:50 AM, Michal Simek wrote:
>>>>
>>>>
>>>> On 5/13/21 10:43 PM, Rob Herring wrote:
>>>>> On Thu, May 13, 2021 at 10:28 AM Sean Anderson
>>> <sean.anderson@seco.com> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 5/13/21 10:33 AM, Sean Anderson wrote:
>>>>>>     >
>>>>>>     >
>>>>>>     > On 5/12/21 10:16 PM, Rob Herring wrote:
>>>>>>     >  > On Tue, May 11, 2021 at 03:12:37PM -0400, Sean Anderson
> wrote:
>>>>>>     >  >> This adds a binding for the Xilinx LogiCORE IP AXI Timer.
>>> This device is
>>>>>>     >  >> a "soft" block, so it has many parameters which would
> not be
>>>>>>     >  >> configurable in most hardware. This binding is usually
>>> automatically
>>>>>>     >  >> generated by Xilinx's tools, so the names and values of
> some
>>> properties
>>>>>>     >  >> must be kept as they are. Replacement properties have been
>>> provided for
>>>>>>     >  >> new device trees.
>>>>>>     >  >
>>>>>>     >  > Because you have some tool generating properties is not a
>>> reason we have
>>>>>>     >  > to accept them upstream.
>>>>>>     >
>>>>>>     > These properties are already in
>>> arch/microblaze/boot/dts/system.dts and
>>>>>>     > in the devicetree supplied to Linux by qemu. Removing these
>>> properties
>>>>>>     > will break existing setups, which I would like to avoid.
>>>>>
>>>>> Already in use in upstream dts files is different than just
>>>>> 'automatically generated' by vendor tools.
>>>>>
>>>>>>     >
>>>>>>     >  > 'deprecated' is for what *we* have deprecated.
>>>>>>     >
>>>>>>     > Ok. I will remove that then.
>>>>>>     >
>>>>>>     >  >
>>>>>>     >  > In this case, I don't really see the point in defining new
>>> properties
>>>>>>     >  > just to have bool.
>>>>>>     >
>>>>>>     > I don't either, but it was requested, by Michal...
>>>>>>
>>>>>> Err, your comment on the original bindings was
>>>>>>
>>>>>>     > Can't all these be boolean?
>>>>>
>>>>> With no other context, yes that's what I would ask. Now you've given
>>>>> me some context, between using the existing ones and 2 sets of
>>>>> properties to maintain, I choose the former.
>>>>>
>>>>>> And Michal commented
>>>>>>
>>>>>>     > I think in this case you should described what it is used by
>>> current
>>>>>>     > driver in Microblaze and these options are required. The rest
>>> are by
>>>>>>     > design optional.
>>>>>>     > If you want to change them to different value then current
> binding
>>>>>>     > should be deprecated and have any transition time with code
>>> alignment.
>>>>>>
>>>>>> So that is what I tried to accomplish with this revision. I also
> tried
>>>>>> allowing something like
>>>>>>
>>>>>>            xlnx,one-timer-only = <0>; /* two timers */
>>>>>>            xlnx,one-timer-only = <1>; /* one timer  */
>>>>>>            xlnx,one-timer-only; /* one timer */
>>>>>>            /* property absent means two timers */
>>>>>>
>>>>>> but I was unable to figure out how to express this with
> json-schema. I
>>>>>> don't think it's the best design either...
>>>>>
>>>>> json-schema would certainly let you, but generally we don't want
>>>>> properties to have more than 1 type.
>>>>
>>>> One thing is what it is in system.dts file which was committed in 2009
>>>> and there are just small alignments there. But none is really using it.
>>>> Maybe I should just delete it.
>>>> And this version was generated by Xilinx ancient tools at that time.
> All
>>>> parameters there are fully describing HW and they are not changing.
> Only
>>>> new one can be added.
>>>>
>>>>    From the current microblaze code you can see which properties are
> really
>>>> used.
>>>>
>>>> reg
>>>> interrupts
>>>> xlnx,one-timer-only
>>>> clocks
>>>> clock-frequency
>>>
>>> There is also an implicit dependency on xlnx,count-width. Several times
>>> the existing driver assumes the counter width is 32, but this should
>>> instead be discovered from the devicetree.
>>
>> For me it is important what it is used now. Which is not
>> xlnx,count-width. That's why if you want to add it you can as optional
>> property.
> 
> At the very least we should sanity check it. E.g. check that it is 32
> and return -EINVAL if it is not.

I have not a problem with it but make sure that the check is there only
when property is present not to break all current users.


>>
>>>
>>>> It means from my point of view these should be listed in the binding.
>>>> clock-frequency is optional by code when clock is defined.
>>>>
>>>> All other properties listed in system.dts are from my perspective
>>>> optional and that's how it should be.
>>>
>>> Here is the situation as I understand it
>>>
>>> * This device has existed for around 15 years (since 2006)
>>> * Because it is a soft device, there are several configurable parameters
>>> * Although all of these parameters must be known for a complete
>>>    implementation of this device, some are unnecessary if onlu reduced
>>>    functionality is needed.
>>> * A de facto devicetree binding for this device has existed for at least
>>>    12 years (since 2009), but likely for as long as the device itself
> has
>>>    existed. This binding has not changed substantially during this time.
>>
>> note: IP itself is even much older.
>>
>>> * This binding is present in devicetrees from the Linux kernel, from
>>>    qemu, in other existing systems, and in devicetrees generated by
>>>    Xilinx's toolset.
>>
>> Only from Linux. Qemu is trying to reuse the same properties but it can
>> also add own one. They are trying to be aligned as much as possible but
>> there are a lot of cases where Qemu requires much more information. (I
>> am not saying in this timer case but in general).
>>
>>
>>> * Because the existing driver for this device does not implement all
>>>    functionality for this device, not all properties in the devicetree
>>>    binding are used. In fact, there is (as noted above) one property
>>>    which should be in use but is not because the current driver
>>>    (implicitly) does not support some hardware configurations.
>>> * To support additional functionality, it is necessary to
>>>    use hardware parameters which were not previously necessary.
>>>
>>> Based on the above, we can classify the properties of this binding into
>>> several categories.
>>>
>>> * Those which are currently read by the driver.
>>>    * compatible
>>>    * reg
>>>    * clocks
>>>    * clock-frequency
>>>    * interrupts
>>>    * xlnx,one-timer-only
>>>
>>> * Those which reflect hardware parameters which are currently explicitly
>>>    or implicitly relied upon by the driver.
>>>    * reg
>>>    * clocks
>>>    * clock-frequency
>>>    * interrupts
>>>    * xlnx,counter-width
>>>    * xlnx,one-timer-only
>>>
>>> * Those which are currently present in device trees.
>>>    * compatible
>>>    * reg
>>>    * interrupts
>>>    * clocks
>>>    * clock-frequency
>>>    * xlnx,count-width
>>>    * xlnx,one-timer-only
>>>    * xlnx,trig0-assert
>>>    * xlnx,trig1-assert
>>>    * xlnx,gen0-assert
>>>    * xlnx,gen1-assert
>>>
>>> When choosing what properties to use, we must consider what the impact
>>> of our changes will be on not just the kernel but also on existing users
>>> of this binding:
>>
>> I don't think that this is valid. Rob is asking for adding #pwm-cells
>> which is purely Linux binding. We also don't know what properties are
>> used by others projects not just Linux or Qemu. Also required properties
>> in Linux doesn't need to be required in U-Boot for example even we are
>> trying to aligned all of them. Another case are others RTOSes, etc.
> 
> Here I do not see a way around this. Any way we do it we will need to
> have some new binding. However, as noted below, adding a new binding for
> configuration is easier than exposing properties in new ways.

please look below.


>>> * To use properties currently present in device trees, we just need to
>>>    modify the kernel driver.
>>> * To add additional properties (such as e.g. '#pwm-cells'), we must
>>>    modify the kernel driver. In addition, users who would like to use
>>>    these new properties must add them to their device trees. This may be
>>>    done in a mechanical way using e.g. overlays.
>>> * To deprecate existing properties and introduce new properties to
>>>    expose the same underlying hardware parameters, we must modify the
>>>    kernel driver. However, this has a large impact on existing users.
>>>    They must modify their tools to generate this information in a
>>>    different format. When this information is generated by upstream
> tools
>>>    this may require updating a core part of their build system. For many
>>>    projects, this may happen very infrequently because of the risk that
>>>    such an upgrade will break things. Even if you suggest that Xilinx
> can
>>>    easily modify its tools to generate any sort of output, the time for
>>>    this upgrade to be deployed/adopted may be significantly longer.
>>
>>  From Xilinx perspective it would be ideal to use only properties which
>> fully describe HW in the form how they are generated today. They are
>> stable for a lot of years and as I said only new one are added.
>> But this alignment wasn't accepted long time ago and we have been asked
>> to start to align these properties with similar HW done by others.
>> And truth is that in a lot of cases there is clear 1:1 mapping and
>> generic properties can be simply use. This mapping ends in Xilinx device
>> tree generator.
>> Back to your point. Required properties are required by Linux driver
>> only. This driver is around for quite a long time where certain policies
>> haven't been setup/used/enforced (Microblaze is 2nd architecture which
>> started to use device tree).
>> We should create DT binding doc at that time but in 2009 it wasn't
>> standard practice. In 2007 Grant was adding support for Xilinx PPC
>> platform also without any DT binding document too.
>>
>> That's why we need to review current unwritten DT binding based on code
>> requirements and look at it how to fix it (if needed) and then add PWM
>> support on the top of it.
>> If something needs to be deprecated, let's deprecate it and have
>> transition time for a year or so to adapt to it.
>>
>> Rob knows much better than I how this should be handled.
>>
>> Based on your list:
>>    * compatible
>>    * reg
>>    * clocks
>>    * clock-frequency
>>    * interrupts
>>    * xlnx,one-timer-only
>>
>> all of these are required property in new DT binding.
>>
>> xlnx,counter-width is optional if you want to use.
> 
> Again, for the existing driver this should at least be sanity-checked so
> we fail noisily instead of buggily.

Till now for 10+ years none reported any issue with it that's why I
don't think this is a big problem.

> This is the situation for the generate polarity as well. The driver will
> work fine, but you will not have PWM output and there will be no
> indication why. This is why I think we should start with supporting the
> existing output. This is used to ensure the device will work as
> configured. We can also add support for different properties exposing
> the same information, but to support only new properties is not very
> useful.

Binding can enforce properties to be required for PWM only and that's
totally okay. And because this is new feature and Rob don't need to like
that new properties they can be aligned. I also have preference to be as
much aligned with HW parameters but if they have to change then let's
change it and I will ensure that Xilinx DTG will generate them as they
were described to be aligned.

Thanks,
Michal
Sean Anderson May 17, 2021, 10:15 p.m. UTC | #16
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
 >
Michal Simek May 19, 2021, 7:24 a.m. UTC | #17
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
Sean Anderson May 20, 2021, 8:13 p.m. UTC | #18
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
 >
Michal Simek May 24, 2021, 7 a.m. UTC | #19
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
Sean Anderson May 24, 2021, 6:34 p.m. UTC | #20
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
 >
Uwe Kleine-König May 25, 2021, 6:11 a.m. UTC | #21
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
Sean Anderson May 25, 2021, 2:30 p.m. UTC | #22
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
 >
Michal Simek June 16, 2021, 12:12 p.m. UTC | #23
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
Sean Anderson June 18, 2021, 9:24 p.m. UTC | #24
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
Michal Simek June 24, 2021, 4:25 p.m. UTC | #25
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
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/pwm/xlnx,axi-timer.yaml b/Documentation/devicetree/bindings/pwm/xlnx,axi-timer.yaml
new file mode 100644
index 000000000000..a5e90658e31a
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/xlnx,axi-timer.yaml
@@ -0,0 +1,142 @@ 
+# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pwm/xlnx,axi-timer.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Xilinx LogiCORE IP AXI Timer Device Tree Binding
+
+maintainers:
+  - Sean Anderson <sean.anderson@seco.com>
+
+properties:
+  compatible:
+    oneOf:
+      - items:
+        - const: xlnx,axi-timer-2.0
+        - const: xlnx,xps-timer-1.00.a
+      - const: xlnx,xps-timer-1.00.a
+
+  clocks:
+    maxItems: 1
+
+  clock-names:
+    const: s_axi_aclk
+
+  reg:
+    maxItems: 1
+
+  xlnx,count-width:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 8
+    maximum: 32
+    description:
+      The width of the counter(s), in bits.
+
+  xlnx,gen0-assert:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [ 0, 1 ]
+    default: 1
+    deprecated: true
+    description:
+      The polarity of the generateout0 signal. 0 for active-low, 1 for active-high.
+
+  xlnx,gen0-active-low:
+    $ref: /schemas/types.yaml#/definitions/flag
+    description:
+      The generate0 signal is active-low.
+
+  xlnx,gen1-assert:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [ 0, 1 ]
+    default: 1
+    deprecated: true
+    description:
+      The polarity of the generateout1 signal. 0 for active-low, 1 for active-high.
+
+  xlnx,gen1-active-low:
+    $ref: /schemas/types.yaml#/definitions/flag
+    description:
+      The generate1 signal is active-low.
+
+  xlnx,one-timer-only:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [ 0, 1 ]
+    deprecated: true
+    description:
+      Whether only one timer is present in this block.
+
+  xlnx,single-timer:
+    $ref: /schemas/types.yaml#/definitions/flag
+    description:
+      Only one timer is present in this block.
+
+  xlnx,pwm:
+    $ref: /schemas/types.yaml#/definitions/flag
+    description:
+      This timer should be configured as a PWM.
+
+required:
+  - compatible
+  - reg
+  - xlnx,count-width
+
+allOf:
+  - if:
+      required:
+        - clocks
+    then:
+      required:
+        - clock-names
+
+  - if:
+      required:
+        - xlnx,gen0-active-low
+    then:
+      not:
+        required:
+          - xlnx,gen0-assert
+
+  - if:
+      required:
+        - xlnx,gen0-active-low
+    then:
+      not:
+        required:
+          - xlnx,gen0-assert
+
+  - if:
+      required:
+        - xlnx,one-timer-only
+    then:
+      not:
+        required:
+          - xlnx,single-timer
+
+additionalProperties: true
+
+examples:
+  - |
+    axi_timer_0: timer@800e0000 {
+        clock-names = "s_axi_aclk";
+        clocks = <&zynqmp_clk 71>;
+        compatible = "xlnx,axi-timer-2.0", "xlnx,xps-timer-1.00.a";
+        reg = <0x800e0000 0x10000>;
+        xlnx,count-width = <0x20>;
+        xlnx,gen0-assert = <0x1>;
+        xlnx,gen1-assert = <0x1>;
+        xlnx,one-timer-only = <0x0>;
+        xlnx,trig0-assert = <0x1>;
+        xlnx,trig1-assert = <0x1>;
+    };
+
+  - |
+    axi_timer_0: timer@800e0000 {
+        clock-names = "s_axi_aclk";
+        clocks = <&zynqmp_clk 71>;
+        compatible = "xlnx,axi-timer-2.0", "xlnx,xps-timer-1.00.a";
+        reg = <0x800e0000 0x10000>;
+        xlnx,count-width = <0x20>;
+        xlnx,gen0-active-low;
+        xlnx,single-timer;
+    };