[v7,1/3] dt-bindings: pwm: Add Xilinx AXI Timer

This adds a binding for the Xilinx LogiCORE IP AXI Timer. This device is a
"soft" block, so it has some 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 should be kept as they
are, if possible. In addition, this binding is already in the kernel at
arch/microblaze/boot/dts/system.dts, and in user software such as QEMU.

The existing driver uses the clock-frequency property, or alternatively the
/cpus/timebase-frequency property as its frequency input. Because these
properties are deprecated, they have not been included with this schema.
All new bindings should use the clocks/clock-names properties to specify
the parent clock.

Because we need to init timer devices so early in boot, we determine if we
should use the PWM driver or the clocksource/clockevent driver by the
presence/absence, respectively, of #pwm-cells. Because both counters are
used by the PWM, there is no need for a separate property specifying which
counters are to be used for the PWM.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---

Changes in v7:
- Add #pwm-cells to properties
- Document why additionalProperties is true

Changes in v6:
- Enumerate possible counter widths
- Fix incorrect schema id

Changes in v5:
- Add example for timer binding
- Fix indentation lint
- Move schema into the timer directory
- Remove xlnx,axi-timer-2.0 compatible string
- Update commit message to reflect revisions

Changes in v4:
- Make some properties optional for clocksource drivers
- Predicate PWM driver on the presence of #pwm-cells
- Remove references to generate polarity so this can get merged

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

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

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

On Thu, 16 Sep 2021 14:05:41 -0400, Sean Anderson wrote:
> This adds a binding for the Xilinx LogiCORE IP AXI Timer. This device is a
> "soft" block, so it has some 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 should be kept as they
> are, if possible. In addition, this binding is already in the kernel at
> arch/microblaze/boot/dts/system.dts, and in user software such as QEMU.
> 
> The existing driver uses the clock-frequency property, or alternatively the
> /cpus/timebase-frequency property as its frequency input. Because these
> properties are deprecated, they have not been included with this schema.
> All new bindings should use the clocks/clock-names properties to specify
> the parent clock.
> 
> Because we need to init timer devices so early in boot, we determine if we
> should use the PWM driver or the clocksource/clockevent driver by the
> presence/absence, respectively, of #pwm-cells. Because both counters are
> used by the PWM, there is no need for a separate property specifying which
> counters are to be used for the PWM.
> 
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> ---
> 
> Changes in v7:
> - Add #pwm-cells to properties
> - Document why additionalProperties is true
> 
> Changes in v6:
> - Enumerate possible counter widths
> - Fix incorrect schema id
> 
> Changes in v5:
> - Add example for timer binding
> - Fix indentation lint
> - Move schema into the timer directory
> - Remove xlnx,axi-timer-2.0 compatible string
> - Update commit message to reflect revisions
> 
> Changes in v4:
> - Make some properties optional for clocksource drivers
> - Predicate PWM driver on the presence of #pwm-cells
> - Remove references to generate polarity so this can get merged
> 
> Changes in v3:
> - Add an example with non-deprecated properties only.
> - Add xlnx,pwm and xlnx,gen?-active-low properties.
> - Make newer replacement properties mutually-exclusive with what they
>   replace
> - Mark all boolean-as-int properties as deprecated
> 
> Changes in v2:
> - Use 32-bit addresses for example binding
> 
>  .../bindings/timer/xlnx,xps-timer.yaml        | 94 +++++++++++++++++++
>  1 file changed, 94 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/timer/xlnx,xps-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/timer/xlnx,xps-timer.yaml:71:111: [warning] line too long (154 > 110 characters) (line-length)

dtschema/dtc warnings/errors:

doc reference errors (make refcheckdocs):

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

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.

Hi,

I see that in patchwork this patch is marked as "Changes Requested" [1].
However, I have not received any feedback regarding the changes I need
to make. Please let me know if I have missed anything.

--Sean

[1] http://patchwork.ozlabs.org/project/linux-pwm/patch/20210916180544.2873770-3-sean.anderson@seco.com/

On 9/16/21 2:05 PM, Sean Anderson wrote:
> This adds PWM support for Xilinx LogiCORE IP AXI soft timers commonly
> found on Xilinx FPGAs. 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.
>
> 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>
> ---
>
> Changes in v7:
> - Add dependency on OF_ADDRESS
> - Fix period_cycles calculation
> - Fix typo in limitations
>
> Changes in v6:
> - Capitalize error messages
> - Don't disable regmap locking to allow inspection of registers via
>    debugfs
> - Prevent overflow when calculating period_cycles
> - Remove enabled variable from xilinx_pwm_apply
> - Swap order of period_cycle range comparisons
>
> Changes in v5:
> - Allow non-zero #pwm-cells
> - Correctly set duty_cycle in get_state when TLR0=TLR1
> - Elaborate on limitation section
> - Perform some additional checks/rounding in apply_state
> - Remove xlnx,axi-timer-2.0 compatible string
> - Rework duty-cycle and period calculations with feedback from Uwe
> - Switch to regmap to abstract endianness issues
> - Use more verbose error messages
>
> Changes in v4:
> - Don't use volatile in read/write replacements. Some arches have it and
>    some don't.
> - Put common timer properties into their own struct to better reuse
>    code.
> - Remove references to properties which are not good enough for Linux.
>
> Changes in v3:
> - Add clockevent and clocksource support
> - Remove old microblaze driver
> - 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!
>
> Changes in v2:
> - Add comment describing device
> - Add comment explaining why we depend on !MICROBLAZE
> - Add dependencies on COMMON_CLK and HAS_IOMEM
> - Cast dividends to u64 to avoid overflow
> - Check for over- and underflow when calculating TLR
> - Check range of xlnx,count-width
> - Don't compile this module by default for arm64
> - Don't set pwmchip.base to -1
> - Ensure the clock is always running when the pwm is registered
> - Remove debugfs file :l
> - Rename TCSR_(SET|CLEAR) to TCSR_RUN_(SET|CLEAR)
> - Report errors with dev_error_probe
> - Set xilinx_pwm_ops.owner
> - Use NSEC_TO_SEC instead of defining our own
> - Use TCSR_RUN_MASK to check if the PWM is enabled, as suggested by Uwe
>
>   MAINTAINERS                  |   1 +
>   drivers/clocksource/Makefile |   5 +-
>   drivers/pwm/Kconfig          |  14 ++
>   drivers/pwm/Makefile         |   1 +
>   drivers/pwm/pwm-xilinx.c     | 267 +++++++++++++++++++++++++++++++++++
>   5 files changed, 287 insertions(+), 1 deletion(-)
>   create mode 100644 drivers/pwm/pwm-xilinx.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 683cfc03b71d..328664194e62 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -20606,6 +20606,7 @@ XILINX TIMER/PWM DRIVER
>   M:	Sean Anderson <sean.anderson@seco.com>
>   S:	Maintained
>   F:	drivers/clocksource/timer-xilinx*
> +F:	drivers/pwm/pwm-xilinx.c
>   F:	include/clocksource/timer-xilinx.h
>
>   XILINX UARTLITE SERIAL DRIVER
> diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
> index a7cba6ef5782..36aa2e5ac1d5 100644
> --- a/drivers/clocksource/Makefile
> +++ b/drivers/clocksource/Makefile
> @@ -88,4 +88,7 @@ obj-$(CONFIG_CSKY_MP_TIMER)		+= timer-mp-csky.o
>   obj-$(CONFIG_GX6605S_TIMER)		+= timer-gx6605s.o
>   obj-$(CONFIG_HYPERV_TIMER)		+= hyperv_timer.o
>   obj-$(CONFIG_MICROCHIP_PIT64B)		+= timer-microchip-pit64b.o
> -obj-$(CONFIG_XILINX_TIMER)		+= timer-xilinx.o timer-xilinx-common.o
> +obj-$(CONFIG_XILINX_TIMER)		+= timer-xilinx.o
> +ifneq ($(CONFIG_XILINX_TIMER)$(CONFIG_PWM_XILINX),)
> +obj-y					+= timer-xilinx-common.o
> +endif
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index aa29841bbb79..47f25237754f 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -638,4 +638,18 @@ config PWM_VT8500
>   	  To compile this driver as a module, choose M here: the module
>   	  will be called pwm-vt8500.
>
> +config PWM_XILINX
> +	tristate "Xilinx AXI Timer PWM support"
> +	depends on OF_ADDRESS
> +	depends on COMMON_CLK
> +	select REGMAP_MMIO
> +	help
> +	  PWM driver 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 compile this driver as a module, choose M here: the module
> +	  will be called pwm-xilinx.
> +
>   endif
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 708840b7fba8..ea785480359b 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -60,3 +60,4 @@ obj-$(CONFIG_PWM_TWL)		+= pwm-twl.o
>   obj-$(CONFIG_PWM_TWL_LED)	+= pwm-twl-led.o
>   obj-$(CONFIG_PWM_VISCONTI)	+= pwm-visconti.o
>   obj-$(CONFIG_PWM_VT8500)	+= pwm-vt8500.o
> +obj-$(CONFIG_PWM_XILINX)	+= pwm-xilinx.o
> diff --git a/drivers/pwm/pwm-xilinx.c b/drivers/pwm/pwm-xilinx.c
> new file mode 100644
> index 000000000000..972cb65c9fe9
> --- /dev/null
> +++ b/drivers/pwm/pwm-xilinx.c
> @@ -0,0 +1,267 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2021 Sean Anderson <sean.anderson@seco.com>
> + *
> + * Limitations:
> + * - When changing both duty cycle and period, we may end up with one cycle
> + *   with the old duty cycle and the new period. This is because the counters
> + *   may only be reloaded by first stopping them, or by letting them be
> + *   automatically reloaded at the end of a cycle. If this automatic reload
> + *   happens after we set TLR0 but before we set TLR1 then we will have a
> + *   bad cycle. This could probably be fixed by reading TCR0 just before
> + *   reprogramming, but I think it would add complexity for little gain.
> + * - Cannot produce 100% duty cycle by configuring the TLRs. This might be
> + *   possible by stopping the counters at an appropriate point in the cycle,
> + *   but this is not (yet) implemented.
> + * - Only produces "normal" output.
> + * - Always produces low output if disabled.
> + */
> +
> +#include <clocksource/timer-xilinx.h>
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/regmap.h>
> +
> +/*
> + * 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 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_pwm_device {
> +	struct pwm_chip chip;
> +	struct xilinx_timer_priv priv;
> +};
> +
> +static inline struct xilinx_timer_priv
> +*xilinx_pwm_chip_to_priv(struct pwm_chip *chip)
> +{
> +	return &container_of(chip, struct xilinx_pwm_device, chip)->priv;
> +}
> +
> +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_pwm_apply(struct pwm_chip *chip, struct pwm_device *unused,
> +			    const struct pwm_state *state)
> +{
> +	struct xilinx_timer_priv *priv = xilinx_pwm_chip_to_priv(chip);
> +	u32 tlr0, tlr1, tcsr0, tcsr1;
> +	u64 period_cycles, duty_cycles;
> +	unsigned long rate;
> +
> +	if (state->polarity != PWM_POLARITY_NORMAL)
> +		return -EINVAL;
> +
> +	/*
> +	 * To be representable by TLR, cycles must be between 2 and
> +	 * priv->max + 2. To enforce this we can reduce the duty
> +	 * cycle, but we may not increase it.
> +	 */
> +	rate = clk_get_rate(priv->clk);
> +	/* Prevent overflow by clamping to the worst case of rate */
> +	period_cycles = min_t(u64, state->period, ULONG_MAX * NSEC_PER_SEC);
> +	period_cycles = mul_u64_u32_div(period_cycles, rate, NSEC_PER_SEC);
> +	if (period_cycles < 2 || period_cycles - 2 > priv->max)
> +		return -ERANGE;
> +	duty_cycles = mul_u64_u32_div(state->duty_cycle, rate, NSEC_PER_SEC);
> +
> +	/*
> +	 * If we specify 100% duty cycle, we will get 0% instead, so decrease
> +	 * the duty cycle count by one.
> +	 */
> +	if (period_cycles == duty_cycles)
> +		duty_cycles--;
> +
> +	/* Round down to 0% duty cycle for unrepresentable duty cycles */
> +	if (duty_cycles < 2)
> +		duty_cycles = period_cycles;
> +
> +	regmap_read(priv->map, TCSR0, &tcsr0);
> +	regmap_read(priv->map, TCSR1, &tcsr1);
> +	tlr0 = xilinx_timer_tlr_cycles(priv, tcsr0, period_cycles);
> +	tlr1 = xilinx_timer_tlr_cycles(priv, tcsr1, duty_cycles);
> +	regmap_write(priv->map, TLR0, tlr0);
> +	regmap_write(priv->map, TLR1, tlr1);
> +
> +	if (state->enabled) {
> +		/*
> +		 * If the PWM is already running, then the counters will be
> +		 * reloaded at the end of the current cycle.
> +		 */
> +		if (!xilinx_timer_pwm_enabled(tcsr0, tcsr1)) {
> +			/* Load TLR into TCR */
> +			regmap_write(priv->map, TCSR0, tcsr0 | TCSR_LOAD);
> +			regmap_write(priv->map, TCSR1, tcsr1 | TCSR_LOAD);
> +			/* 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);
> +			regmap_write(priv->map, TCSR0, tcsr0);
> +			regmap_write(priv->map, TCSR1, tcsr1);
> +		}
> +	} else {
> +		regmap_write(priv->map, TCSR0, 0);
> +		regmap_write(priv->map, TCSR1, 0);
> +	}
> +
> +	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, tlr1, tcsr0, tcsr1;
> +
> +	regmap_read(priv->map, TLR0, &tlr0);
> +	regmap_read(priv->map, TLR1, &tlr1);
> +	regmap_read(priv->map, TCSR0, &tcsr0);
> +	regmap_read(priv->map, TCSR1, &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;
> +
> +	/* 100% duty cycle results in constant low output */
> +	if (state->period == state->duty_cycle)
> +		state->duty_cycle = 0;
> +}
> +
> +static const struct pwm_ops xilinx_pwm_ops = {
> +	.apply = xilinx_pwm_apply,
> +	.get_state = xilinx_pwm_get_state,
> +	.owner = THIS_MODULE,
> +};
> +
> +static const struct regmap_config xilinx_pwm_regmap_config = {
> +	.reg_bits = 32,
> +	.reg_stride = 4,
> +	.val_bits = 32,
> +	.val_format_endian = REGMAP_ENDIAN_LITTLE,
> +	.max_register = TCR1,
> +};
> +
> +static int xilinx_timer_probe(struct platform_device *pdev)
> +{
> +	int ret;
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	struct xilinx_timer_priv *priv;
> +	struct xilinx_pwm_device *pwm;
> +	u32 pwm_cells, one_timer;
> +	void __iomem *regs;
> +
> +	ret = of_property_read_u32(np, "#pwm-cells", &pwm_cells);
> +	if (ret == -EINVAL)
> +		return -ENODEV;
> +	else if (ret)
> +		return dev_err_probe(dev, ret, "could not read #pwm-cells\n");
> +
> +	pwm = devm_kzalloc(dev, sizeof(*pwm), GFP_KERNEL);
> +	if (!pwm)
> +		return -ENOMEM;
> +	platform_set_drvdata(pdev, pwm);
> +	priv = &pwm->priv;
> +
> +	regs = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(regs))
> +		return PTR_ERR(regs);
> +
> +	priv->map = devm_regmap_init_mmio(dev, regs,
> +					  &xilinx_pwm_regmap_config);
> +	if (IS_ERR(priv->map))
> +		return dev_err_probe(dev, PTR_ERR(priv->map),
> +				     "Could not create regmap\n");
> +
> +	ret = xilinx_timer_common_init(np, priv, &one_timer);
> +	if (ret)
> +		return ret;
> +
> +	if (one_timer)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "Two timers required for PWM mode\n");
> +
> +	/*
> +	 * The polarity of the generate outputs must be active high for PWM
> +	 * mode to work. We could determine this from the device tree, but
> +	 * alas, such properties are not allowed to be used.
> +	 */
> +
> +	priv->clk = devm_clk_get(dev, "s_axi_aclk");
> +	if (IS_ERR(priv->clk))
> +		return dev_err_probe(dev, PTR_ERR(priv->clk),
> +				     "Could not get clock\n");
> +
> +	ret = clk_prepare_enable(priv->clk);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Clock enable failed\n");
> +	clk_rate_exclusive_get(priv->clk);
> +
> +	pwm->chip.dev = dev;
> +	pwm->chip.ops = &xilinx_pwm_ops;
> +	pwm->chip.npwm = 1;
> +	ret = pwmchip_add(&pwm->chip);
> +	if (ret) {
> +		clk_rate_exclusive_put(priv->clk);
> +		clk_disable_unprepare(priv->clk);
> +		return dev_err_probe(dev, ret, "Could not register PWM chip\n");
> +	}
> +
> +	return 0;
> +}
> +
> +static int xilinx_timer_remove(struct platform_device *pdev)
> +{
> +	struct xilinx_pwm_device *pwm = platform_get_drvdata(pdev);
> +
> +	pwmchip_remove(&pwm->chip);
> +	clk_rate_exclusive_put(pwm->priv.clk);
> +	clk_disable_unprepare(pwm->priv.clk);
> +	return 0;
> +}
> +
> +static const struct of_device_id xilinx_timer_of_match[] = {
> +	{ .compatible = "xlnx,xps-timer-1.00.a", },
> +	{},
> +};
> +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);
> +
> +MODULE_ALIAS("platform:xilinx-timer");
> +MODULE_DESCRIPTION("Xilinx LogiCORE IP AXI Timer driver");
> +MODULE_LICENSE("GPL v2");
>

On Thu, Sep 23, 2021 at 07:36:32PM -0400, Sean Anderson wrote:
> I see that in patchwork this patch is marked as "Changes Requested" [1].
> However, I have not received any feedback regarding the changes I need
> to make. Please let me know if I have missed anything.

I agree, this must be an error. I marked it as new again.

Best regards
Uwe

Dear Sean,

On 9/16/21 8:05 PM, Sean Anderson wrote:
> This rewrites the Xilinx AXI timer driver to be more platform agnostic.
> Some common code has been split off so it can be reused. These routines
> currently live in drivers/mfd. The largest changes are summarized below:
> 
> - We now support any number of timer devices, possibly with only one
>   counter each. The first counter will be used as a clocksource. Every
>   other counter will be used as a clockevent. This allocation scheme was
>   chosen arbitrarily.
> - We do not use timer_of_init because we need to perform some tasks in
>   between different stages. For example, we must ensure that ->read and
>   ->write are initialized before registering the irq. This can only happen
>   after we have gotten the register base (to detect endianness). We also
>   have a rather unusual clock initialization sequence in order to remain
>   backwards compatible. Due to this, it's ok for the initial clock request
>   to fail, and we do not want other initialization to be undone. Lastly, it
>   is more convenient to do one allocation for xilinx_clockevent_device than
>   to do one for timer_of and one for xilinx_timer_priv.
> - We now pay attention to xlnx,count-width and handle smaller width timers.
>   The default remains 32.
> - We access registers using regmap. This automatically deals with
>   endianness issues, so we no longer have to use our own wrappers. It
>   also provides locking for clockevents which have to worry about being
>   interrupted in the middle of a read/modify/write.
> 
> Note that while the existing timer driver always sets the cpumask to cpu
> 0, this version sets it to all possible CPUs. I believe this is correct
> for multiprocessor systems where the timer is not physically wired to a
> particular CPU's interrupt line. For uniprocessor systems (like most
> microblaze systems) this makes no difference.
> 
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> ---
> This has been tested on microblaze qemu.
> 
> Changes in v7:
> - Add dependency on OF_ADDRESS
> 
> Changes in v6:
> - Add __init* attributes
> - Export common symbols
> - Fix goto'ing incorrect label for cleanup
> - Remove duplicate regmap_config
> - Round to closest period in xilinx_timer_get_period to ensure proper
>   semantics for xilinx_pwm_get_state
> 
> Changes in v5:
> - Fix some overflows when setting the max value for clockevent and
>   sched_clock
> - Just use clk_register_fixed_rate instead of the "private" version
> - Remove duplicate register definitions
> - Remove xilinx_timer_tlr_period
> - Remove xlnx,axi-timer-2.0 compatible string
> - Require that callers check arguments to xilinx_timer_tlr_cycles
> - Use regmap to deal with endianness issues as suggested by Lee
> 
> Changes in v4:
> - Break out clock* drivers into their own file
> 
>  MAINTAINERS                               |   6 +
>  arch/microblaze/kernel/Makefile           |   3 +-
>  arch/microblaze/kernel/timer.c            | 326 ----------------------
>  drivers/clocksource/Kconfig               |  13 +
>  drivers/clocksource/Makefile              |   1 +
>  drivers/clocksource/timer-xilinx-common.c |  71 +++++
>  drivers/clocksource/timer-xilinx.c        | 323 +++++++++++++++++++++
>  include/clocksource/timer-xilinx.h        |  91 ++++++
>  8 files changed, 506 insertions(+), 328 deletions(-)
>  delete mode 100644 arch/microblaze/kernel/timer.c
>  create mode 100644 drivers/clocksource/timer-xilinx-common.c
>  create mode 100644 drivers/clocksource/timer-xilinx.c
>  create mode 100644 include/clocksource/timer-xilinx.h


I have said it couple of times. I won't accept this in this form.
I have no problem to move this driver out of microblaze. But I want to
see transition from current state to new state and check it with baby
steps which are bisectable if any problem happens.
Because in this style we end in this patch and it will take some time to
find out what it is failing.

That's why my NACK.

Thanks,
Michal

On 9/24/21 2:52 AM, Michal Simek wrote:
> Dear Sean,
>
> On 9/16/21 8:05 PM, Sean Anderson wrote:
>> This rewrites the Xilinx AXI timer driver to be more platform agnostic.
>> Some common code has been split off so it can be reused. These routines
>> currently live in drivers/mfd. The largest changes are summarized below:
>>
>> - We now support any number of timer devices, possibly with only one
>>   counter each. The first counter will be used as a clocksource. Every
>>   other counter will be used as a clockevent. This allocation scheme was
>>   chosen arbitrarily.
>> - We do not use timer_of_init because we need to perform some tasks in
>>   between different stages. For example, we must ensure that ->read and
>>   ->write are initialized before registering the irq. This can only happen
>>   after we have gotten the register base (to detect endianness). We also
>>   have a rather unusual clock initialization sequence in order to remain
>>   backwards compatible. Due to this, it's ok for the initial clock request
>>   to fail, and we do not want other initialization to be undone. Lastly, it
>>   is more convenient to do one allocation for xilinx_clockevent_device than
>>   to do one for timer_of and one for xilinx_timer_priv.
>> - We now pay attention to xlnx,count-width and handle smaller width timers.
>>   The default remains 32.
>> - We access registers using regmap. This automatically deals with
>>   endianness issues, so we no longer have to use our own wrappers. It
>>   also provides locking for clockevents which have to worry about being
>>   interrupted in the middle of a read/modify/write.
>>
>> Note that while the existing timer driver always sets the cpumask to cpu
>> 0, this version sets it to all possible CPUs. I believe this is correct
>> for multiprocessor systems where the timer is not physically wired to a
>> particular CPU's interrupt line. For uniprocessor systems (like most
>> microblaze systems) this makes no difference.
>>
>> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
>> ---
>> This has been tested on microblaze qemu.
>>
>> Changes in v7:
>> - Add dependency on OF_ADDRESS
>>
>> Changes in v6:
>> - Add __init* attributes
>> - Export common symbols
>> - Fix goto'ing incorrect label for cleanup
>> - Remove duplicate regmap_config
>> - Round to closest period in xilinx_timer_get_period to ensure proper
>>   semantics for xilinx_pwm_get_state
>>
>> Changes in v5:
>> - Fix some overflows when setting the max value for clockevent and
>>   sched_clock
>> - Just use clk_register_fixed_rate instead of the "private" version
>> - Remove duplicate register definitions
>> - Remove xilinx_timer_tlr_period
>> - Remove xlnx,axi-timer-2.0 compatible string
>> - Require that callers check arguments to xilinx_timer_tlr_cycles
>> - Use regmap to deal with endianness issues as suggested by Lee
>>
>> Changes in v4:
>> - Break out clock* drivers into their own file
>>
>>  MAINTAINERS                               |   6 +
>>  arch/microblaze/kernel/Makefile           |   3 +-
>>  arch/microblaze/kernel/timer.c            | 326 ----------------------
>>  drivers/clocksource/Kconfig               |  13 +
>>  drivers/clocksource/Makefile              |   1 +
>>  drivers/clocksource/timer-xilinx-common.c |  71 +++++
>>  drivers/clocksource/timer-xilinx.c        | 323 +++++++++++++++++++++
>>  include/clocksource/timer-xilinx.h        |  91 ++++++
>>  8 files changed, 506 insertions(+), 328 deletions(-)
>>  delete mode 100644 arch/microblaze/kernel/timer.c
>>  create mode 100644 drivers/clocksource/timer-xilinx-common.c
>>  create mode 100644 drivers/clocksource/timer-xilinx.c
>>  create mode 100644 include/clocksource/timer-xilinx.h
>
>
> I have said it couple of times. I won't accept this in this form.
> I have no problem to move this driver out of microblaze. But I want to
> see transition from current state to new state and check it with baby
> steps which are bisectable if any problem happens.
> Because in this style we end in this patch and it will take some time to
> find out what it is failing.

Unfortunately, I do not have the time do do this at the moment. Because
these drivers are independent in nature, I propose to drop these changes
to the timer driver, but leave the common functions split out. In the
future, I (or you) may come back and make the changes in this patch in
an incremental fashion. The only change necessary for this driver would
be to check for #pwm-cells.

--Sean