| Message ID | 20210719221322.3723009-3-sean.anderson@seco.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v5,1/3] dt-bindings: pwm: Add Xilinx AXI Timer | expand |
Hi Sean,
I love your patch! Yet something to improve:
[auto build test ERROR on tip/timers/core]
[also build test ERROR on pwm/for-next linus/master v5.14-rc2 next-20210720]
[cannot apply to xlnx/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Sean-Anderson/dt-bindings-pwm-Add-Xilinx-AXI-Timer/20210720-144903
base: https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git 2d0a9eb23ccfdf11308bec6db0bc007585d919d2
config: m68k-allmodconfig (attached as .config)
compiler: m68k-linux-gcc (GCC) 10.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/338d061c42411ef012c4dce517355fd1d61c1cab
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Sean-Anderson/dt-bindings-pwm-Add-Xilinx-AXI-Timer/20210720-144903
git checkout 338d061c42411ef012c4dce517355fd1d61c1cab
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-10.3.0 make.cross ARCH=m68k
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>, old ones prefixed by <<):
>> ERROR: modpost: "xilinx_timer_tlr_cycles" [drivers/pwm/pwm-xilinx.ko] undefined!
>> ERROR: modpost: "xilinx_timer_common_init" [drivers/pwm/pwm-xilinx.ko] undefined!
>> ERROR: modpost: "xilinx_timer_get_period" [drivers/pwm/pwm-xilinx.ko] undefined!
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On 7/21/21 9:26 AM, kernel test robot wrote: > Hi Sean, > > I love your patch! Yet something to improve: > > [auto build test ERROR on tip/timers/core] > [also build test ERROR on pwm/for-next linus/master v5.14-rc2 next-20210720] > [cannot apply to xlnx/master] > [If your patch is applied to the wrong git tree, kindly drop us a note. > And when submitting patch, we suggest to use '--base' as documented in > https://git-scm.com/docs/git-format-patch] > > url: https://github.com/0day-ci/linux/commits/Sean-Anderson/dt-bindings-pwm-Add-Xilinx-AXI-Timer/20210720-144903 > base: https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git 2d0a9eb23ccfdf11308bec6db0bc007585d919d2 > config: m68k-allmodconfig (attached as .config) > compiler: m68k-linux-gcc (GCC) 10.3.0 > reproduce (this is a W=1 build): > wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross > chmod +x ~/bin/make.cross > # https://github.com/0day-ci/linux/commit/338d061c42411ef012c4dce517355fd1d61c1cab > git remote add linux-review https://github.com/0day-ci/linux > git fetch --no-tags linux-review Sean-Anderson/dt-bindings-pwm-Add-Xilinx-AXI-Timer/20210720-144903 > git checkout 338d061c42411ef012c4dce517355fd1d61c1cab > # save the attached .config to linux build tree > COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-10.3.0 make.cross ARCH=m68k > > If you fix the issue, kindly add following tag as appropriate > Reported-by: kernel test robot <lkp@intel.com> > > All errors (new ones prefixed by >>, old ones prefixed by <<): > >>> ERROR: modpost: "xilinx_timer_tlr_cycles" [drivers/pwm/pwm-xilinx.ko] undefined! >>> ERROR: modpost: "xilinx_timer_common_init" [drivers/pwm/pwm-xilinx.ko] undefined! >>> ERROR: modpost: "xilinx_timer_get_period" [drivers/pwm/pwm-xilinx.ko] undefined! Looks like I forgot my EXPORT_SYMBOLs. Will be fixed in v6. --Sean > > --- > 0-DAY CI Kernel Test Service, Intel Corporation > https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org >
ping? On 7/19/21 6:13 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> > --- > Uwe, I know we haven't finished our discussion on the previous series > wrt what checks/rounding apply_state should do, but I want to get some > feedback on the other changes I've made to this series in the meantime. > > Changes in v5: > - Elaborate on limitation section > - Rework duty-cycle and period calculations with feedback from Uwe > - Use more verbose error messages > - Allow non-zero #pwm-cells > - Remove xlnx,axi-timer-2.0 compatible string > - Correctly set duty_cycle in get_state when TLR0=TLR1 > - Perform some additional checks/rounding in apply_state > - Switch to regmap to abstract endianness issues > > Changes in v4: > - Remove references to properties which are not good enough for Linux. > - 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. > > 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 > > MAINTAINERS | 1 + > drivers/clocksource/Makefile | 5 +- > drivers/pwm/Kconfig | 13 ++ > drivers/pwm/Makefile | 1 + > drivers/pwm/pwm-xilinx.c | 268 +++++++++++++++++++++++++++++++++++ > 5 files changed, 287 insertions(+), 1 deletion(-) > create mode 100644 drivers/pwm/pwm-xilinx.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index 252d71addd18..6adafd3e7a09 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -20050,6 +20050,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..dcf434fdb726 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 c76adedd58c9..974774b7c987 100644 > --- a/drivers/pwm/Kconfig > +++ b/drivers/pwm/Kconfig > @@ -637,4 +637,17 @@ 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 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..291577be992f > --- /dev/null > +++ b/drivers/pwm/pwm-xilinx.c > @@ -0,0 +1,268 @@ > +// 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 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) > +{ > + bool enabled; > + 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); > + period_cycles = mul_u64_u32_div(state->period, rate, NSEC_PER_SEC); > + if (period_cycles - 2 > priv->max || period_cycles < 2) > + 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); > + > + enabled = xilinx_timer_pwm_enabled(tcsr0, tcsr1); > + if (state->enabled) { > + /* > + * If the PWM is already running, then the counters will be > + * reloaded at the end of the current cycle. > + */ > + if (!enabled) { > + /* 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, > + .disable_locking = true, /* The PWM subsystem handles locking */ > +}; > + > +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"); >
Hello Sean, sorry for having you let waiting so long. Now here some more feedback: On Mon, Jul 19, 2021 at 06:13:22PM -0400, Sean Anderson wrote: > +static int xilinx_pwm_apply(struct pwm_chip *chip, struct pwm_device *unused, > + const struct pwm_state *state) > +{ > + bool enabled; > + 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); > + period_cycles = mul_u64_u32_div(state->period, rate, NSEC_PER_SEC); cool, I didn't know mul_u64_u32_div. Hmm, we still have a problem here if state->period * rate > 1000000000 * U64_MAX. So to be entirely save, we either need: /* * To ensure that period * rate / NSEC_PER_SEC fits into an u64 * we need: * U64_MAX * NSEC_PER_SEC * period < ---------------------- * rate * * . If rate is not bigger than NSEC_PER_SEC this is true for * sure as the RHS is bigger than U64_MAX. Otherwise we can * calculate the RHS using mul_u64_u32_div. */ if (rate > NSEC_PER_SEC) period = min(state->period, mul_u64_u32_div(U64_MAX, NSEC_PER_SEC, rate); else period = state->period; or we go a step further and check the priv->max limit in the same step: period = min(state->period, ((u64)priv->max + 2) * NSEC_PER_SEC / rate) . The latter is simpler and it's safe as priv->max is an u32 and so there is no overflow. > + if (period_cycles - 2 > priv->max || period_cycles < 2) I'd check for period_cycles < 2 first, because otherwise period_cycles - 2 might underflow. Nothing bad happens in this case, but reading from left to right my first thought was I found a bug. Also please decrease period_cycles if it's bigger than priv->max + 2. (With the suggestion above you don't need to check for period_cycles - 2 > priv->max any more however.) > + 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); > + > + enabled = xilinx_timer_pwm_enabled(tcsr0, tcsr1); > + if (state->enabled) { > + /* > + * If the PWM is already running, then the counters will be > + * reloaded at the end of the current cycle. > + */ If state->enabled is false, $enabled isn't used, so you can move the assignment into the if body and also limit the scope of $enabled. > + if (!enabled) { > + /* 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); xilinx_timer_get_period rounds down, this is however wrong for .get_state(). > + state->duty_cycle = xilinx_timer_get_period(priv, tlr1, tcsr1); ditto for duty_cycle. > + state->enabled = xilinx_timer_pwm_enabled(tcsr0, tcsr1); > + state->polarity = PWM_POLARITY_NORMAL; > [...] > +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"); Please capitalize error messages. > [...] > + 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"); s/pwm/PWM/ > + } > + > + return 0; > +} Best regards Uwe
On 8/14/21 4:47 PM, Uwe Kleine-König wrote: > Hello Sean, > > sorry for having you let waiting so long. Now here some more feedback: > > On Mon, Jul 19, 2021 at 06:13:22PM -0400, Sean Anderson wrote: >> +static int xilinx_pwm_apply(struct pwm_chip *chip, struct pwm_device *unused, >> + const struct pwm_state *state) >> +{ >> + bool enabled; >> + 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); >> + period_cycles = mul_u64_u32_div(state->period, rate, NSEC_PER_SEC); > > cool, I didn't know mul_u64_u32_div. I didn't either. Alas, many useful functions like these have no documentation... > > Hmm, we still have a problem here if > > state->period * rate > 1000000000 * U64_MAX. Note that this can only occur with rate > 1GHz (and period = U64_MAX). The highest fmax in the datasheet is 300 MHz (on a very expensive FPGA). Maybe it is more prudent to do period = min(state->period, ULONG_MAX * NSEC_PER_SEC) I think a period of 136 years is adequate :) This comparison also has the advantage of being against const values. > So to be entirely save, we either need: > > /* > * To ensure that period * rate / NSEC_PER_SEC fits into an u64 > * we need: > * U64_MAX * NSEC_PER_SEC > * period < ---------------------- > * rate > * > * . If rate is not bigger than NSEC_PER_SEC this is true for > * sure as the RHS is bigger than U64_MAX. Otherwise we can > * calculate the RHS using mul_u64_u32_div. > */ > if (rate > NSEC_PER_SEC) > period = min(state->period, mul_u64_u32_div(U64_MAX, NSEC_PER_SEC, rate); > else > period = state->period; > > or we go a step further and check the priv->max limit in the same step: > > period = min(state->period, ((u64)priv->max + 2) * NSEC_PER_SEC / rate) > > . The latter is simpler and it's safe as priv->max is an u32 and so > there is no overflow. > >> + if (period_cycles - 2 > priv->max || period_cycles < 2) > > I'd check for period_cycles < 2 first, because otherwise period_cycles - > 2 might underflow. Nothing bad happens in this case, but reading from > left to right my first thought was I found a bug. Also please decrease > period_cycles if it's bigger than priv->max + 2. (With the suggestion > above you don't need to check for period_cycles - 2 > priv->max any more > however.) Ok, will swap. >> + 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); >> + >> + enabled = xilinx_timer_pwm_enabled(tcsr0, tcsr1); >> + if (state->enabled) { >> + /* >> + * If the PWM is already running, then the counters will be >> + * reloaded at the end of the current cycle. >> + */ > > If state->enabled is false, $enabled isn't used, so you can move the > assignment into the if body and also limit the scope of $enabled. Ok. >> + if (!enabled) { >> + /* 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); > > xilinx_timer_get_period rounds down, this is however wrong for > .get_state(). Why is this wrong? I thought get_state should return values which would not be rounded if passed to apply_state. >> + state->duty_cycle = xilinx_timer_get_period(priv, tlr1, tcsr1); > > ditto for duty_cycle. > >> + state->enabled = xilinx_timer_pwm_enabled(tcsr0, tcsr1); >> + state->polarity = PWM_POLARITY_NORMAL; >> [...] >> +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"); > > Please capitalize error messages. Ok. >> [...] >> + 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"); > > s/pwm/PWM/ Ok. Thanks for the review. --Sean
On Mon, Aug 16, 2021 at 07:51:17PM -0400, Sean Anderson wrote: > > > On 8/14/21 4:47 PM, Uwe Kleine-König wrote: > > Hello Sean, > > > > sorry for having you let waiting so long. Now here some more feedback: > > > > On Mon, Jul 19, 2021 at 06:13:22PM -0400, Sean Anderson wrote: > > > +static int xilinx_pwm_apply(struct pwm_chip *chip, struct pwm_device *unused, > > > + const struct pwm_state *state) > > > +{ > > > + bool enabled; > > > + 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); > > > + period_cycles = mul_u64_u32_div(state->period, rate, NSEC_PER_SEC); > > > > cool, I didn't know mul_u64_u32_div. > > I didn't either. Alas, many useful functions like these have no > documentation... > > > > > Hmm, we still have a problem here if > > > > state->period * rate > 1000000000 * U64_MAX. > > Note that this can only occur with rate > 1GHz (and period = U64_MAX). > The highest fmax in the datasheet is 300 MHz (on a very expensive FPGA). > > Maybe it is more prudent to do > > period = min(state->period, ULONG_MAX * NSEC_PER_SEC) Together with a check for rate being <= 300 MHz to be safe that's fine. > I think a period of 136 years is adequate :) This comparison also has > the advantage of being against const values. *nod* > > > +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); > > > > xilinx_timer_get_period rounds down, this is however wrong for > > .get_state(). > > Why is this wrong? I thought get_state should return values which would > not be rounded if passed to apply_state. Consider a PWM that yields a period of π * $regval ns when a certain register is programmed with the value $regval. Consider the HW is programmed with regval = 317. The exact period is 995.8848711879644. If now .get_state() rounds down and returns 995 ns and you feed that value back into .apply the new regval (assuming round down in .apply(), too) this yields regval = 316. If however .get_state() rounds up and returns 996, putting this value back into .apply() you get the desired 317. Best regards Uwe
On 8/17/21 2:04 PM, Uwe Kleine-König wrote: > On Mon, Aug 16, 2021 at 07:51:17PM -0400, Sean Anderson wrote: >> >> >> On 8/14/21 4:47 PM, Uwe Kleine-König wrote: >> > Hello Sean, >> > >> > sorry for having you let waiting so long. Now here some more feedback: >> > >> > On Mon, Jul 19, 2021 at 06:13:22PM -0400, Sean Anderson wrote: >> > > +static int xilinx_pwm_apply(struct pwm_chip *chip, struct pwm_device *unused, >> > > + const struct pwm_state *state) >> > > +{ >> > > + bool enabled; >> > > + 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); >> > > + period_cycles = mul_u64_u32_div(state->period, rate, NSEC_PER_SEC); >> > >> > cool, I didn't know mul_u64_u32_div. >> >> I didn't either. Alas, many useful functions like these have no >> documentation... >> >> > >> > Hmm, we still have a problem here if >> > >> > state->period * rate > 1000000000 * U64_MAX. >> >> Note that this can only occur with rate > 1GHz (and period = U64_MAX). >> The highest fmax in the datasheet is 300 MHz (on a very expensive FPGA). >> >> Maybe it is more prudent to do >> >> period = min(state->period, ULONG_MAX * NSEC_PER_SEC) > > Together with a check for rate being <= 300 MHz to be safe that's fine. That's what the ULONG_MAX is for; whatever we get back from clk_get_rate, it has to fit in a ulong. > >> I think a period of 136 years is adequate :) This comparison also has >> the advantage of being against const values. > > *nod* > >> > > +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); >> > >> > xilinx_timer_get_period rounds down, this is however wrong for >> > .get_state(). >> >> Why is this wrong? I thought get_state should return values which would >> not be rounded if passed to apply_state. > > Consider a PWM that yields a period of π * $regval ns when a certain > register is programmed with the value $regval. > > Consider the HW is programmed with regval = 317. The exact period is > 995.8848711879644. If now .get_state() rounds down and returns 995 ns and > you feed that value back into .apply the new regval (assuming round down > in .apply(), too) this yields regval = 316. If however .get_state() > rounds up and returns 996, putting this value back into .apply() you get > the desired 317. Will fix for v6, but please document this somewhere :) --Sean
diff --git a/MAINTAINERS b/MAINTAINERS index 252d71addd18..6adafd3e7a09 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -20050,6 +20050,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..dcf434fdb726 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 c76adedd58c9..974774b7c987 100644 --- a/drivers/pwm/Kconfig +++ b/drivers/pwm/Kconfig @@ -637,4 +637,17 @@ 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 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..291577be992f --- /dev/null +++ b/drivers/pwm/pwm-xilinx.c @@ -0,0 +1,268 @@ +// 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 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) +{ + bool enabled; + 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); + period_cycles = mul_u64_u32_div(state->period, rate, NSEC_PER_SEC); + if (period_cycles - 2 > priv->max || period_cycles < 2) + 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); + + enabled = xilinx_timer_pwm_enabled(tcsr0, tcsr1); + if (state->enabled) { + /* + * If the PWM is already running, then the counters will be + * reloaded at the end of the current cycle. + */ + if (!enabled) { + /* 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, + .disable_locking = true, /* The PWM subsystem handles locking */ +}; + +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");
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> --- Uwe, I know we haven't finished our discussion on the previous series wrt what checks/rounding apply_state should do, but I want to get some feedback on the other changes I've made to this series in the meantime. Changes in v5: - Elaborate on limitation section - Rework duty-cycle and period calculations with feedback from Uwe - Use more verbose error messages - Allow non-zero #pwm-cells - Remove xlnx,axi-timer-2.0 compatible string - Correctly set duty_cycle in get_state when TLR0=TLR1 - Perform some additional checks/rounding in apply_state - Switch to regmap to abstract endianness issues Changes in v4: - Remove references to properties which are not good enough for Linux. - 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. 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 MAINTAINERS | 1 + drivers/clocksource/Makefile | 5 +- drivers/pwm/Kconfig | 13 ++ drivers/pwm/Makefile | 1 + drivers/pwm/pwm-xilinx.c | 268 +++++++++++++++++++++++++++++++++++ 5 files changed, 287 insertions(+), 1 deletion(-) create mode 100644 drivers/pwm/pwm-xilinx.c