| Message ID | 20210503214413.3145015-2-sean.anderson@seco.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] dt-bindings: pwm: Add Xilinx AXI Timer | expand |
Hi Sean,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on arm64/for-next/core]
[also build test ERROR on linux/master linus/master v5.12 next-20210503]
[cannot apply to pwm/for-next 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/20210504-054451
base: https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-next/core
config: sh-allmodconfig (attached as .config)
compiler: sh4-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/164c8df701eef0c9509d544501a1de5c406e93cb
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/20210504-054451
git checkout 164c8df701eef0c9509d544501a1de5c406e93cb
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross W=1 ARCH=sh
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: "__delay" [drivers/net/mdio/mdio-cavium.ko] undefined!
>> ERROR: modpost: "clk_rate_exclusive_get" [drivers/pwm/pwm-xilinx.ko] undefined!
>> ERROR: modpost: "clk_rate_exclusive_put" [drivers/pwm/pwm-xilinx.ko] undefined!
ERROR: modpost: "__udivdi3" [fs/btrfs/btrfs.ko] undefined!
ERROR: modpost: "__umoddi3" [fs/btrfs/btrfs.ko] undefined!
Kconfig warnings: (for reference only)
WARNING: unmet direct dependencies detected for SND_ATMEL_SOC_PDC
Depends on SOUND && !UML && SND && SND_SOC && SND_ATMEL_SOC && HAS_DMA
Selected by
- SND_ATMEL_SOC_SSC && SOUND && !UML && SND && SND_SOC && SND_ATMEL_SOC
- SND_ATMEL_SOC_SSC_PDC && SOUND && !UML && SND && SND_SOC && SND_ATMEL_SOC && ATMEL_SSC
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Hello, On Mon, May 03, 2021 at 05:44:13PM -0400, Sean Anderson wrote: > This adds PWM support for Xilinx LogiCORE IP AXI soft timers commonly > found on Xilinx FPGAs. There is another driver for this device located > at arch/microblaze/kernel/timer.c, but it is only used for timekeeping. > 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> > --- > > arch/arm64/configs/defconfig | 1 + > drivers/pwm/Kconfig | 11 ++ > drivers/pwm/Makefile | 1 + > drivers/pwm/pwm-xilinx.c | 322 +++++++++++++++++++++++++++++++++++ > 4 files changed, 335 insertions(+) > create mode 100644 drivers/pwm/pwm-xilinx.c > > diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig > index 08c6f769df9a..81794209f287 100644 > --- a/arch/arm64/configs/defconfig > +++ b/arch/arm64/configs/defconfig > @@ -1083,6 +1083,7 @@ CONFIG_PWM_SAMSUNG=y > CONFIG_PWM_SL28CPLD=m > CONFIG_PWM_SUN4I=m > CONFIG_PWM_TEGRA=m > +CONFIG_PWM_XILINX=m > CONFIG_SL28CPLD_INTC=y > CONFIG_QCOM_PDC=y > CONFIG_RESET_IMX7=y I think this should go into a separate patch once this driver is accepted. This can then go via the ARM people. > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > index d3371ac7b871..01e62928f4bf 100644 > --- a/drivers/pwm/Kconfig > +++ b/drivers/pwm/Kconfig > @@ -628,4 +628,15 @@ 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 !MICROBLAZE I don't understand this dependency. > + help > + PWM framework driver for Xilinx LogiCORE IP AXI Timers. This > + timer is typically a soft core which may be present in Xilinx > + FPGAs. 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 d3879619bd76..fc1bd6ccc9ed 100644 > --- a/drivers/pwm/Makefile > +++ b/drivers/pwm/Makefile > @@ -59,3 +59,4 @@ obj-$(CONFIG_PWM_TIEHRPWM) += pwm-tiehrpwm.o > obj-$(CONFIG_PWM_TWL) += pwm-twl.o > obj-$(CONFIG_PWM_TWL_LED) += pwm-twl-led.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..240bd2993f97 > --- /dev/null > +++ b/drivers/pwm/pwm-xilinx.c > @@ -0,0 +1,322 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * Copyright (C) 2021 Sean Anderson <sean.anderson@seco.com> > + */ Please add a link to a reference manual here (if available) and add a paragraph about hardware properties: Does the hardware complete cycles on reconfiguration or disable? Are there limitiation like "Cannot run with 100% duty cycle"? Please look into e.g. pwm-sifive how this is done and stick to the same format for easy grepping. > +#include <asm/io.h> > +#include <linux/clk.h> > +#include <linux/device.h> > +#include <linux/debugfs.h> > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/pwm.h> > + > +#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) I'd like to see a prefix for these defines, something like XILINX_PWM_ maybe? > +/* Bits we need to set/clear to enable PWM mode, not including CASC */ > +#define TCSR_SET (TCSR_GENT | TCSR_ARHT | TCSR_ENT | TCSR_PWMA) > +#define TCSR_CLEAR (TCSR_MDT | TCSR_LOAD) Maybe call this XILINX_PWM_TCSR_DEFAULT? > +#define NSEC_PER_SEC_ULL 1000000000ULL > + > +/** > + * struct xilinx_pwm_device - Driver data for Xilinx AXI timer PWM driver > + * @chip: PWM controller chip > + * @clk: Parent clock > + * @regs: Base address of this device > + * @width: Width of the counters, in bits > + */ > +struct xilinx_pwm_device { > + struct pwm_chip chip; > + struct clk *clk; > + void __iomem *regs; > + unsigned int width; > +}; > + > +static inline struct xilinx_pwm_device *xilinx_pwm_chip_to_device(struct pwm_chip *chip) > +{ > + return container_of(chip, struct xilinx_pwm_device, chip); > +} > + > +static bool xilinx_pwm_is_enabled(u32 tcsr0, u32 tcsr1) > +{ > + return !(~tcsr0 & TCSR_SET || tcsr0 & (TCSR_CLEAR | TCSR_CASC) || > + ~tcsr1 & TCSR_SET || tcsr1 & TCSR_CLEAR); > +} > + > +static u32 xilinx_pwm_calc_tlr(struct xilinx_pwm_device *pwm, u32 tcsr, > + unsigned int period) > +{ > + u64 cycles = DIV_ROUND_DOWN_ULL(period * clk_get_rate(pwm->clk), > + NSEC_PER_SEC_ULL); NSEC_PER_SEC should work just fine here. > + > + if (tcsr & TCSR_UDT) > + return cycles - 2; What happens if cycles <= 1? > + else > + return (BIT_ULL(pwm->width) - 1) - cycles + 2; What happens if cycles > BIT_ULL(pwm->width)? > +} > + > +static unsigned int xilinx_pwm_get_period(struct xilinx_pwm_device *pwm, > + u32 tlr, u32 tcsr) > +{ > + u64 cycles; > + > + if (tcsr & TCSR_UDT) > + cycles = tlr + 2; > + else > + cycles = (BIT_ULL(pwm->width) - 1) - tlr + 2; > + > + return DIV_ROUND_DOWN_ULL(cycles * NSEC_PER_SEC_ULL, > + clk_get_rate(pwm->clk)); Please round up here. The idea is that applying the result from .get_state() should not modify the configuration. You might want to enable PWM_DEBUG to test your driver. > +} > + > +static int xilinx_pwm_apply(struct pwm_chip *chip, struct pwm_device *unused, > + const struct pwm_state *state) > +{ > + struct xilinx_pwm_device *pwm = xilinx_pwm_chip_to_device(chip); > + u32 tlr0, tlr1; > + u32 tcsr0 = readl(pwm->regs + TCSR0); > + u32 tcsr1 = readl(pwm->regs + TCSR1); > + bool enabled = xilinx_pwm_is_enabled(tcsr0, tcsr1); > + > + if (state->polarity != PWM_POLARITY_NORMAL) > + return -EINVAL; > + > + if (!enabled && state->enabled) > + clk_rate_exclusive_get(pwm->clk); Missing error checking. > + > + tlr0 = xilinx_pwm_calc_tlr(pwm, tcsr0, state->period); Please care for overflowing. If state->period >= U64_MAX / clk_get_rate(pwm->clk) the multiplication in xilinx_pwm_calc_tlr is susceptible. Please do something like: ... calculate maximal period ... if (state->period > max_period) period = max_period else period = state->period; ... The algorithm to implement is: Configure the biggest possible period not bigger than state->period. With that period configure the biggest duty_cycle not bigger than state->duty_cycle. > + tlr1 = xilinx_pwm_calc_tlr(pwm, tcsr1, state->duty_cycle); > + writel(tlr0, pwm->regs + TLR0); > + writel(tlr1, pwm->regs + TLR1); How does the hardware behave here? E.g. can it happen that after writing TLR0 we can see a cycle with the new period but the old duty_cycle? Does it complete the currently running period? If state->enabled == 0 you want to disable first to prevent that the (maybe) new values for period and duty_cycle are emitted by the hardware. > + if (state->enabled) { > + /* Only touch the TCSRs if we aren't already running */ > + if (!enabled) { > + /* Load TLR into TCR */ > + writel(tcsr0 | TCSR_LOAD, pwm->regs + TCSR0); > + writel(tcsr1 | TCSR_LOAD, pwm->regs + TCSR1); > + /* Enable timers all at once with ENALL */ > + tcsr0 = (TCSR_SET & ~TCSR_ENT) | (tcsr0 & TCSR_UDT); > + tcsr1 = TCSR_SET | TCSR_ENALL | (tcsr1 & TCSR_UDT); > + writel(tcsr0, pwm->regs + TCSR0); > + writel(tcsr1, pwm->regs + TCSR1); > + } > + } else { > + writel(tcsr0 & ~TCSR_ENT, pwm->regs + TCSR0); > + writel(tcsr1 & ~TCSR_ENT, pwm->regs + TCSR1); How does the hardware behave on disable? Does it always emit a low level? Does it complete the currently running period? > + } > + > + if (enabled && !state->enabled) > + clk_rate_exclusive_put(pwm->clk); > + > + return 0; > +} > + > +static void xilinx_pwm_get_state(struct pwm_chip *chip, > + struct pwm_device *unused, > + struct pwm_state *state) > +{ > + struct xilinx_pwm_device *pwm = xilinx_pwm_chip_to_device(chip); > + u32 tlr0, tlr1, tcsr0, tcsr1; > + > + tlr0 = readl(pwm->regs + TLR0); > + tlr1 = readl(pwm->regs + TLR1); > + tcsr0 = readl(pwm->regs + TCSR0); > + tcsr1 = readl(pwm->regs + TCSR1); > + > + state->period = xilinx_pwm_get_period(pwm, tlr0, tcsr0); > + state->duty_cycle = xilinx_pwm_get_period(pwm, tlr1, tcsr1); > + state->enabled = xilinx_pwm_is_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 is missing > +}; > + > +static struct dentry *debug_dir; > + > +#define DEBUG_REG(reg) { .name = #reg, .offset = (reg), } > +static struct debugfs_reg32 xilinx_pwm_reg32[] = { > + DEBUG_REG(TCSR0), > + DEBUG_REG(TLR0), > + DEBUG_REG(TCR0), > + DEBUG_REG(TCSR1), > + DEBUG_REG(TLR1), > + DEBUG_REG(TCR1), > +}; > + > +static int xilinx_pwm_debug_create(struct xilinx_pwm_device *pwm) > +{ > + struct debugfs_regset32 *regset; > + > + if (!IS_ENABLED(CONFIG_DEBUG_FS)) > + return 0; > + > + regset = devm_kzalloc(pwm->chip.dev, sizeof(*regset), GFP_KERNEL); > + if (!pwm) > + return -ENOMEM; > + > + regset->regs = xilinx_pwm_reg32; > + regset->nregs = ARRAY_SIZE(xilinx_pwm_reg32); > + regset->base = pwm->regs; > + > + debugfs_create_regset32(dev_name(pwm->chip.dev), 0400, debug_dir, > + regset); debug_dir might not yet be initialized here. > + return 0; > +} This is unusual to have in a driver. Something like memtool[1] should work just fine, then you don't need this debugfs file. Together with pwm tracing this should be good enough. [1] https://github.com/pengutronix/memtool > +static int xilinx_pwm_probe(struct platform_device *pdev) > +{ > + bool enabled; > + int i, ret; > + struct device *dev = &pdev->dev; > + struct xilinx_pwm_device *pwm; > + u32 one_timer; > + > + ret = of_property_read_u32(dev->of_node, "xlnx,one-timer-only", > + &one_timer); > + if (ret || one_timer) { > + dev_err(dev, "two timers are needed for PWM mode\n"); > + return -EINVAL; > + } > + > + for (i = 0; i < 2; i++) { > + char fmt[] = "xlnx,gen%u-assert"; > + char buf[sizeof(fmt)]; > + u32 gen; > + > + snprintf(buf, sizeof(buf), fmt, i); > + ret = of_property_read_u32(dev->of_node, buf, &gen); > + if (ret || !gen) { > + dev_err(dev, "generateout%u must be active high\n", i); > + return -EINVAL; > + } > + } > + > + pwm = devm_kzalloc(&pdev->dev, sizeof(*pwm), GFP_KERNEL); > + if (!pwm) > + return -ENOMEM; > + platform_set_drvdata(pdev, pwm); > + > + pwm->chip.dev = &pdev->dev; > + pwm->chip.ops = &xilinx_pwm_ops; > + pwm->chip.base = -1; Please drop the assignment to .base. See commit f9a8ee8c8bcd118e800d88772c6457381db45224 in next. > + pwm->chip.npwm = 1; > + > + pwm->regs = devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(pwm->regs)) > + return PTR_ERR(pwm->regs); > + > + ret = of_property_read_u32(dev->of_node, "xlnx,count-width", &pwm->width); > + if (ret) { > + dev_err(dev, "missing counter width\n"); > + return -EINVAL; > + } Would it make sense to error out on some values of pwm->width? Values bigger than 32 don't make sense? > + pwm->clk = devm_clk_get(dev, NULL); > + if (IS_ERR(pwm->clk)) > + return dev_err_probe(dev, PTR_ERR(pwm->clk), "missing clock\n"); > + > + ret = clk_prepare_enable(pwm->clk); > + if (ret) { > + dev_err(dev, "clock enable failed\n"); > + return ret; You can use dev_err_probe here, too, for consistency. > + } > + > + ret = xilinx_pwm_debug_create(pwm); > + if (ret) { > + clk_disable_unprepare(pwm->clk); > + return ret; > + } > + > + ret = pwmchip_add(&pwm->chip); > + if (ret) { > + clk_disable_unprepare(pwm->clk); Error message please. > + return ret; > + } > + > + enabled = xilinx_pwm_is_enabled(readl(pwm->regs + TCSR0), > + readl(pwm->regs + TCSR1)); > + if (enabled) > + clk_rate_exclusive_get(pwm->clk); This needs to happen before calling pwmchip_add(), doesn't it? > + > + return ret; > +} > + > +static int xilinx_pwm_remove(struct platform_device *pdev) > +{ > + struct xilinx_pwm_device *pwm = platform_get_drvdata(pdev); > + bool enabled = xilinx_pwm_is_enabled(readl(pwm->regs + TCSR0), > + readl(pwm->regs + TCSR1)); > + > + if (enabled) > + clk_rate_exclusive_put(pwm->clk); This looks wrong. You should rely on the consumer that they disable the PWM. > + clk_disable_unprepare(pwm->clk); I assume this stops the PWM, so this must happen after calling pwmchip_remove. > + return pwmchip_remove(&pwm->chip); Note, pwmchip_remove always returns 0 and I plan to change it to return void instead. So please don't check the return value here. > +} > + Best regards Uwe
[ Sorry if you receive this twice, I think gmail is skipping the emails I send via mutt so I'm resending this via the web interface ] Hi, On Mon, May 03, 2021 at 05:44:13PM -0400, Sean Anderson wrote: > This adds PWM support for Xilinx LogiCORE IP AXI soft timers commonly > found on Xilinx FPGAs. There is another driver for this device located > at arch/microblaze/kernel/timer.c, but it is only used for timekeeping. > 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> > I wrote a patch to support this back in 2018, being the main difference that I did try to make this PWM driver available also for Microblaze systems. Of course, since a lot of time has passed since, even the PWM API seems to have changed, so my original patch is probably no longer applicable. You can see that old discussion here https://lore.kernel.org/patchwork/patch/803890/ https://lore.kernel.org/patchwork/patch/935728/ It didn't went much further because no one from the device tree list gave any feedback to my proposal, so it just kind of died there since I didn't have the time to push further. Also, I didn't really send a patch as such to devicetree list, since it looked like we needed some previous discussion about it. > --- > > arch/arm64/configs/defconfig | 1 + > drivers/pwm/Kconfig | 11 ++ > drivers/pwm/Makefile | 1 + > drivers/pwm/pwm-xilinx.c | 322 +++++++++++++++++++++++++++++++++++ > 4 files changed, 335 insertions(+) > create mode 100644 drivers/pwm/pwm-xilinx.c > > diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig > index 08c6f769df9a..81794209f287 100644 > --- a/arch/arm64/configs/defconfig > +++ b/arch/arm64/configs/defconfig > @@ -1083,6 +1083,7 @@ CONFIG_PWM_SAMSUNG=y > CONFIG_PWM_SL28CPLD=m > CONFIG_PWM_SUN4I=m > CONFIG_PWM_TEGRA=m > +CONFIG_PWM_XILINX=m > CONFIG_SL28CPLD_INTC=y > CONFIG_QCOM_PDC=y > CONFIG_RESET_IMX7=y > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > index d3371ac7b871..01e62928f4bf 100644 > --- a/drivers/pwm/Kconfig > +++ b/drivers/pwm/Kconfig > @@ -628,4 +628,15 @@ 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 !MICROBLAZE Here is probably the main difference between your patch and mine which instead of depending on !MICROBLAZE had a dependency on ARCH_ZYNQ || MICROBLAZE -apart from mine being too old to apply and useless now , of course- > + help > + PWM framework driver for Xilinx LogiCORE IP AXI Timers. This > + timer is typically a soft core which may be present in Xilinx > + FPGAs. 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 d3879619bd76..fc1bd6ccc9ed 100644 > --- a/drivers/pwm/Makefile > +++ b/drivers/pwm/Makefile > @@ -59,3 +59,4 @@ obj-$(CONFIG_PWM_TIEHRPWM) += pwm-tiehrpwm.o > obj-$(CONFIG_PWM_TWL) += pwm-twl.o > obj-$(CONFIG_PWM_TWL_LED) += pwm-twl-led.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..240bd2993f97 > --- /dev/null > +++ b/drivers/pwm/pwm-xilinx.c > @@ -0,0 +1,322 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * Copyright (C) 2021 Sean Anderson <sean.anderson@seco.com> > + */ > +#include <asm/io.h> > +#include <linux/clk.h> > +#include <linux/device.h> > +#include <linux/debugfs.h> > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/pwm.h> > + > +#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) > + > +/* Bits we need to set/clear to enable PWM mode, not including CASC */ > +#define TCSR_SET (TCSR_GENT | TCSR_ARHT | TCSR_ENT | TCSR_PWMA) > +#define TCSR_CLEAR (TCSR_MDT | TCSR_LOAD) > + > +#define NSEC_PER_SEC_ULL 1000000000ULL > + > +/** > + * struct xilinx_pwm_device - Driver data for Xilinx AXI timer PWM driver > + * @chip: PWM controller chip > + * @clk: Parent clock > + * @regs: Base address of this device > + * @width: Width of the counters, in bits > + */ > +struct xilinx_pwm_device { > + struct pwm_chip chip; > + struct clk *clk; > + void __iomem *regs; > + unsigned int width; > +}; > + > +static inline struct xilinx_pwm_device *xilinx_pwm_chip_to_device(struct pwm_chip *chip) > +{ > + return container_of(chip, struct xilinx_pwm_device, chip); > +} > + > +static bool xilinx_pwm_is_enabled(u32 tcsr0, u32 tcsr1) > +{ > + return !(~tcsr0 & TCSR_SET || tcsr0 & (TCSR_CLEAR | TCSR_CASC) || > + ~tcsr1 & TCSR_SET || tcsr1 & TCSR_CLEAR); > +} > + > +static u32 xilinx_pwm_calc_tlr(struct xilinx_pwm_device *pwm, u32 tcsr, > + unsigned int period) > +{ > + u64 cycles = DIV_ROUND_DOWN_ULL(period * clk_get_rate(pwm->clk), > + NSEC_PER_SEC_ULL); > + > + if (tcsr & TCSR_UDT) > + return cycles - 2; > + else > + return (BIT_ULL(pwm->width) - 1) - cycles + 2; > +} > + > +static unsigned int xilinx_pwm_get_period(struct xilinx_pwm_device *pwm, > + u32 tlr, u32 tcsr) > +{ > + u64 cycles; > + > + if (tcsr & TCSR_UDT) > + cycles = tlr + 2; > + else > + cycles = (BIT_ULL(pwm->width) - 1) - tlr + 2; > + > + return DIV_ROUND_DOWN_ULL(cycles * NSEC_PER_SEC_ULL, > + clk_get_rate(pwm->clk)); > +} > + > +static int xilinx_pwm_apply(struct pwm_chip *chip, struct pwm_device *unused, > + const struct pwm_state *state) > +{ > + struct xilinx_pwm_device *pwm = xilinx_pwm_chip_to_device(chip); > + u32 tlr0, tlr1; > + u32 tcsr0 = readl(pwm->regs + TCSR0); > + u32 tcsr1 = readl(pwm->regs + TCSR1); > + bool enabled = xilinx_pwm_is_enabled(tcsr0, tcsr1); > + > + if (state->polarity != PWM_POLARITY_NORMAL) > + return -EINVAL; > + > + if (!enabled && state->enabled) > + clk_rate_exclusive_get(pwm->clk); > + > + tlr0 = xilinx_pwm_calc_tlr(pwm, tcsr0, state->period); > + tlr1 = xilinx_pwm_calc_tlr(pwm, tcsr1, state->duty_cycle); > + writel(tlr0, pwm->regs + TLR0); > + writel(tlr1, pwm->regs + TLR1); > + > + if (state->enabled) { > + /* Only touch the TCSRs if we aren't already running */ > + if (!enabled) { > + /* Load TLR into TCR */ > + writel(tcsr0 | TCSR_LOAD, pwm->regs + TCSR0); > + writel(tcsr1 | TCSR_LOAD, pwm->regs + TCSR1); > + /* Enable timers all at once with ENALL */ > + tcsr0 = (TCSR_SET & ~TCSR_ENT) | (tcsr0 & TCSR_UDT); > + tcsr1 = TCSR_SET | TCSR_ENALL | (tcsr1 & TCSR_UDT); > + writel(tcsr0, pwm->regs + TCSR0); > + writel(tcsr1, pwm->regs + TCSR1); > + } > + } else { > + writel(tcsr0 & ~TCSR_ENT, pwm->regs + TCSR0); > + writel(tcsr1 & ~TCSR_ENT, pwm->regs + TCSR1); > + } > + > + if (enabled && !state->enabled) > + clk_rate_exclusive_put(pwm->clk); > + > + return 0; > +} > + > +static void xilinx_pwm_get_state(struct pwm_chip *chip, > + struct pwm_device *unused, > + struct pwm_state *state) > +{ > + struct xilinx_pwm_device *pwm = xilinx_pwm_chip_to_device(chip); > + u32 tlr0, tlr1, tcsr0, tcsr1; > + > + tlr0 = readl(pwm->regs + TLR0); > + tlr1 = readl(pwm->regs + TLR1); > + tcsr0 = readl(pwm->regs + TCSR0); > + tcsr1 = readl(pwm->regs + TCSR1); > + > + state->period = xilinx_pwm_get_period(pwm, tlr0, tcsr0); > + state->duty_cycle = xilinx_pwm_get_period(pwm, tlr1, tcsr1); > + state->enabled = xilinx_pwm_is_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, > +}; > + > +static struct dentry *debug_dir; > + > +#define DEBUG_REG(reg) { .name = #reg, .offset = (reg), } > +static struct debugfs_reg32 xilinx_pwm_reg32[] = { > + DEBUG_REG(TCSR0), > + DEBUG_REG(TLR0), > + DEBUG_REG(TCR0), > + DEBUG_REG(TCSR1), > + DEBUG_REG(TLR1), > + DEBUG_REG(TCR1), > +}; I didn't add any DEBUG_FS entries, so I really like this. It would have helped me while developing my own version of this and will probably be useful for people debugging their hardware. > + > +static int xilinx_pwm_debug_create(struct xilinx_pwm_device *pwm) > +{ > + struct debugfs_regset32 *regset; > + > + if (!IS_ENABLED(CONFIG_DEBUG_FS)) > + return 0; > + > + regset = devm_kzalloc(pwm->chip.dev, sizeof(*regset), GFP_KERNEL); > + if (!pwm) > + return -ENOMEM; > + > + regset->regs = xilinx_pwm_reg32; > + regset->nregs = ARRAY_SIZE(xilinx_pwm_reg32); > + regset->base = pwm->regs; > + > + debugfs_create_regset32(dev_name(pwm->chip.dev), 0400, debug_dir, > + regset); > + return 0; > +} > + > +static int xilinx_pwm_probe(struct platform_device *pdev) > +{ > + bool enabled; > + int i, ret; > + struct device *dev = &pdev->dev; > + struct xilinx_pwm_device *pwm; > + u32 one_timer; > + > + ret = of_property_read_u32(dev->of_node, "xlnx,one-timer-only", > + &one_timer); > + if (ret || one_timer) { > + dev_err(dev, "two timers are needed for PWM mode\n"); > + return -EINVAL; > + } > + > + for (i = 0; i < 2; i++) { > + char fmt[] = "xlnx,gen%u-assert"; > + char buf[sizeof(fmt)]; > + u32 gen; > + > + snprintf(buf, sizeof(buf), fmt, i); > + ret = of_property_read_u32(dev->of_node, buf, &gen); > + if (ret || !gen) { > + dev_err(dev, "generateout%u must be active high\n", i); > + return -EINVAL; > + } > + } > + > + pwm = devm_kzalloc(&pdev->dev, sizeof(*pwm), GFP_KERNEL); > + if (!pwm) > + return -ENOMEM; > + platform_set_drvdata(pdev, pwm); > + > + pwm->chip.dev = &pdev->dev; > + pwm->chip.ops = &xilinx_pwm_ops; > + pwm->chip.base = -1; > + pwm->chip.npwm = 1; > + > + pwm->regs = devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(pwm->regs)) > + return PTR_ERR(pwm->regs); > + > + ret = of_property_read_u32(dev->of_node, "xlnx,count-width", &pwm->width); > + if (ret) { > + dev_err(dev, "missing counter width\n"); > + return -EINVAL; > + } > + > + pwm->clk = devm_clk_get(dev, NULL); > + if (IS_ERR(pwm->clk)) > + return dev_err_probe(dev, PTR_ERR(pwm->clk), "missing clock\n"); > + > + ret = clk_prepare_enable(pwm->clk); > + if (ret) { > + dev_err(dev, "clock enable failed\n"); > + return ret; > + } > + > + ret = xilinx_pwm_debug_create(pwm); > + if (ret) { > + clk_disable_unprepare(pwm->clk); > + return ret; > + } > + > + ret = pwmchip_add(&pwm->chip); > + if (ret) { > + clk_disable_unprepare(pwm->clk); > + return ret; > + } > + > + enabled = xilinx_pwm_is_enabled(readl(pwm->regs + TCSR0), > + readl(pwm->regs + TCSR1)); > + if (enabled) > + clk_rate_exclusive_get(pwm->clk); > + > + return ret; > +} > + > +static int xilinx_pwm_remove(struct platform_device *pdev) > +{ > + struct xilinx_pwm_device *pwm = platform_get_drvdata(pdev); > + bool enabled = xilinx_pwm_is_enabled(readl(pwm->regs + TCSR0), > + readl(pwm->regs + TCSR1)); > + > + if (enabled) > + clk_rate_exclusive_put(pwm->clk); > + clk_disable_unprepare(pwm->clk); > + return pwmchip_remove(&pwm->chip); > +} > + > +static const struct of_device_id xilinx_pwm_of_match[] = { > + { .compatible = "xlnx,xps-timer-1.00.a" }, > + { .compatible = "xlnx,axi-timer-2.0" }, > + {}, > +}; > +MODULE_DEVICE_TABLE(of, xilinx_pwm_of_match); And here lies culprit of my patch not being accepted, since defining the driver to be "xlnx,axi-timer-2.0" compatible while still allowing MICROBLAZE to be set, did make the kernel to misconfigure the interrupt timer used by the processor. > + > +static struct platform_driver xilinx_pwm_driver = { > + .probe = xilinx_pwm_probe, > + .remove = xilinx_pwm_remove, > + .driver = { > + .name = "xilinx-pwm", > + .of_match_table = of_match_ptr(xilinx_pwm_of_match), > + }, > +}; > + > +static int __init xilinx_pwm_init(void) > +{ > + int ret = platform_driver_register(&xilinx_pwm_driver); > + > + if (ret) > + return ret; > + > + if (IS_ENABLED(CONFIG_DEBUG_FS)) { > + debug_dir = debugfs_create_dir(xilinx_pwm_driver.driver.name, NULL); > + if (IS_ERR(debug_dir)) { > + platform_driver_unregister(&xilinx_pwm_driver); > + return PTR_ERR(debug_dir); > + } > + } > + return 0; > +} > +module_init(xilinx_pwm_init); > + > +static void __exit xilinx_pwm_exit(void) > +{ > + platform_driver_unregister(&xilinx_pwm_driver); > + if (IS_ENABLED(CONFIG_DEBUG_FS)) > + debugfs_remove_recursive(debug_dir); > +} > +module_exit(xilinx_pwm_exit); > + > +MODULE_ALIAS("platform:xilinx-pwm"); > +MODULE_DESCRIPTION("Xilinx LogiCORE IP AXI Timer PWM driver"); > +MODULE_LICENSE("GPL v2"); > -- > 2.25.1 > All in all, I'd be very glad if this got accepted (I haven't tested it), but I'd be still happier if some compromise could be found within the device tree so that this driver can be used also for Microblaze systems without interfering with the main timer used by such systems. Best regards,
On 5/4/21 10:51 AM, Uwe Kleine-König wrote: > Hello, > > On Mon, May 03, 2021 at 05:44:13PM -0400, Sean Anderson wrote: >> This adds PWM support for Xilinx LogiCORE IP AXI soft timers commonly >> found on Xilinx FPGAs. There is another driver for this device located >> at arch/microblaze/kernel/timer.c, but it is only used for timekeeping. >> 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> >> --- >> >> arch/arm64/configs/defconfig | 1 + >> drivers/pwm/Kconfig | 11 ++ >> drivers/pwm/Makefile | 1 + >> drivers/pwm/pwm-xilinx.c | 322 +++++++++++++++++++++++++++++++++++ >> 4 files changed, 335 insertions(+) >> create mode 100644 drivers/pwm/pwm-xilinx.c >> >> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig >> index 08c6f769df9a..81794209f287 100644 >> --- a/arch/arm64/configs/defconfig >> +++ b/arch/arm64/configs/defconfig >> @@ -1083,6 +1083,7 @@ CONFIG_PWM_SAMSUNG=y >> CONFIG_PWM_SL28CPLD=m >> CONFIG_PWM_SUN4I=m >> CONFIG_PWM_TEGRA=m >> +CONFIG_PWM_XILINX=m >> CONFIG_SL28CPLD_INTC=y >> CONFIG_QCOM_PDC=y >> CONFIG_RESET_IMX7=y > > I think this should go into a separate patch once this driver is > accepted. This can then go via the ARM people. > >> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig >> index d3371ac7b871..01e62928f4bf 100644 >> --- a/drivers/pwm/Kconfig >> +++ b/drivers/pwm/Kconfig >> @@ -628,4 +628,15 @@ 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 !MICROBLAZE > > I don't understand this dependency. The dependency is clear here because microblaze has already driver for this timer here arch/microblaze/kernel/timer.c. And that's exactly pointing to the way how this should be done. IP itself is single or dual timer and in case of dual timer you can select if there is pwm output and use it for PWM generation. It means it is timer with PMW together. I didn't have a time but Uwe likely knows this better how to design it. I see that gpio-mvebu driver instantiate pwm driver. Maybe that's the way to go. Step first is move axi timer driver from microblaze to generic location. Figured it out how to add PWM (with DT flag) and then write support for it. Thanks, Michal
On 5/4/21 4:51 AM, Uwe Kleine-König wrote: > Hello, > > On Mon, May 03, 2021 at 05:44:13PM -0400, Sean Anderson wrote: >> This adds PWM support for Xilinx LogiCORE IP AXI soft timers commonly >> found on Xilinx FPGAs. There is another driver for this device located >> at arch/microblaze/kernel/timer.c, but it is only used for timekeeping. >> 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> >> --- >> >> arch/arm64/configs/defconfig | 1 + >> drivers/pwm/Kconfig | 11 ++ >> drivers/pwm/Makefile | 1 + >> drivers/pwm/pwm-xilinx.c | 322 +++++++++++++++++++++++++++++++++++ >> 4 files changed, 335 insertions(+) >> create mode 100644 drivers/pwm/pwm-xilinx.c >> >> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig >> index 08c6f769df9a..81794209f287 100644 >> --- a/arch/arm64/configs/defconfig >> +++ b/arch/arm64/configs/defconfig >> @@ -1083,6 +1083,7 @@ CONFIG_PWM_SAMSUNG=y >> CONFIG_PWM_SL28CPLD=m >> CONFIG_PWM_SUN4I=m >> CONFIG_PWM_TEGRA=m >> +CONFIG_PWM_XILINX=m >> CONFIG_SL28CPLD_INTC=y >> CONFIG_QCOM_PDC=y >> CONFIG_RESET_IMX7=y > > I think this should go into a separate patch once this driver is > accepted. This can then go via the ARM people. Sure. > >> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig >> index d3371ac7b871..01e62928f4bf 100644 >> --- a/drivers/pwm/Kconfig >> +++ b/drivers/pwm/Kconfig >> @@ -628,4 +628,15 @@ 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 !MICROBLAZE > > I don't understand this dependency. As noted in the commit message, there is already a driver for this device for microblaze systems. To prevent conflicts, this driver is disabled on microblaze. > >> + help >> + PWM framework driver for Xilinx LogiCORE IP AXI Timers. This >> + timer is typically a soft core which may be present in Xilinx >> + FPGAs. 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 d3879619bd76..fc1bd6ccc9ed 100644 >> --- a/drivers/pwm/Makefile >> +++ b/drivers/pwm/Makefile >> @@ -59,3 +59,4 @@ obj-$(CONFIG_PWM_TIEHRPWM) += pwm-tiehrpwm.o >> obj-$(CONFIG_PWM_TWL) += pwm-twl.o >> obj-$(CONFIG_PWM_TWL_LED) += pwm-twl-led.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..240bd2993f97 >> --- /dev/null >> +++ b/drivers/pwm/pwm-xilinx.c >> @@ -0,0 +1,322 @@ >> +// SPDX-License-Identifier: GPL-2.0+ >> +/* >> + * Copyright (C) 2021 Sean Anderson <sean.anderson@seco.com> >> + */ > > Please add a link to a reference manual here (if available) and add a > paragraph about hardware properties: Does the hardware complete cycles > on reconfiguration or disable? Are there limitiation like "Cannot run > with 100% duty cycle"? Please look into e.g. pwm-sifive how this is done > and stick to the same format for easy grepping. Ok, will add. > >> +#include <asm/io.h> >> +#include <linux/clk.h> >> +#include <linux/device.h> >> +#include <linux/debugfs.h> >> +#include <linux/module.h> >> +#include <linux/platform_device.h> >> +#include <linux/pwm.h> >> + >> +#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) > > I'd like to see a prefix for these defines, something like XILINX_PWM_ > maybe? I don't think that's necessary, since these defines are used only for this file. In particular, adding a prefix like that would significantly lengthen lines which add several flags. > >> +/* Bits we need to set/clear to enable PWM mode, not including CASC */ >> +#define TCSR_SET (TCSR_GENT | TCSR_ARHT | TCSR_ENT | TCSR_PWMA) >> +#define TCSR_CLEAR (TCSR_MDT | TCSR_LOAD) > > Maybe call this XILINX_PWM_TCSR_DEFAULT? > >> +#define NSEC_PER_SEC_ULL 1000000000ULL >> + >> +/** >> + * struct xilinx_pwm_device - Driver data for Xilinx AXI timer PWM driver >> + * @chip: PWM controller chip >> + * @clk: Parent clock >> + * @regs: Base address of this device >> + * @width: Width of the counters, in bits >> + */ >> +struct xilinx_pwm_device { >> + struct pwm_chip chip; >> + struct clk *clk; >> + void __iomem *regs; >> + unsigned int width; >> +}; >> + >> +static inline struct xilinx_pwm_device *xilinx_pwm_chip_to_device(struct pwm_chip *chip) >> +{ >> + return container_of(chip, struct xilinx_pwm_device, chip); >> +} >> + >> +static bool xilinx_pwm_is_enabled(u32 tcsr0, u32 tcsr1) >> +{ >> + return !(~tcsr0 & TCSR_SET || tcsr0 & (TCSR_CLEAR | TCSR_CASC) || >> + ~tcsr1 & TCSR_SET || tcsr1 & TCSR_CLEAR); >> +} >> + >> +static u32 xilinx_pwm_calc_tlr(struct xilinx_pwm_device *pwm, u32 tcsr, >> + unsigned int period) >> +{ >> + u64 cycles = DIV_ROUND_DOWN_ULL(period * clk_get_rate(pwm->clk), >> + NSEC_PER_SEC_ULL); > > NSEC_PER_SEC should work just fine here. Ok. > >> + >> + if (tcsr & TCSR_UDT) >> + return cycles - 2; > > What happens if cycles <= 1? Then we have a problem :) I will add a check for this. > >> + else >> + return (BIT_ULL(pwm->width) - 1) - cycles + 2; > > What happens if cycles > BIT_ULL(pwm->width)? ditto. > >> +} >> + >> +static unsigned int xilinx_pwm_get_period(struct xilinx_pwm_device *pwm, >> + u32 tlr, u32 tcsr) >> +{ >> + u64 cycles; >> + >> + if (tcsr & TCSR_UDT) >> + cycles = tlr + 2; >> + else >> + cycles = (BIT_ULL(pwm->width) - 1) - tlr + 2; >> + >> + return DIV_ROUND_DOWN_ULL(cycles * NSEC_PER_SEC_ULL, >> + clk_get_rate(pwm->clk)); > > Please round up here. The idea is that applying the result from > .get_state() should not modify the configuration. You might want to > enable PWM_DEBUG to test your driver. Ok. > >> +} >> + >> +static int xilinx_pwm_apply(struct pwm_chip *chip, struct pwm_device *unused, >> + const struct pwm_state *state) >> +{ >> + struct xilinx_pwm_device *pwm = xilinx_pwm_chip_to_device(chip); >> + u32 tlr0, tlr1; >> + u32 tcsr0 = readl(pwm->regs + TCSR0); >> + u32 tcsr1 = readl(pwm->regs + TCSR1); >> + bool enabled = xilinx_pwm_is_enabled(tcsr0, tcsr1); >> + >> + if (state->polarity != PWM_POLARITY_NORMAL) >> + return -EINVAL; >> + >> + if (!enabled && state->enabled) >> + clk_rate_exclusive_get(pwm->clk); > > Missing error checking. This cannot fail. > >> + >> + tlr0 = xilinx_pwm_calc_tlr(pwm, tcsr0, state->period); > > Please care for overflowing. If state->period >= U64_MAX / > clk_get_rate(pwm->clk) the multiplication in xilinx_pwm_calc_tlr is > susceptible. Please do something like: > > ... calculate maximal period ... > > if (state->period > max_period) > period = max_period > else > period = state->period; > > ... > > The algorithm to implement is: Configure the biggest possible period not > bigger than state->period. With that period configure the biggest > duty_cycle not bigger than state->duty_cycle. > >> + tlr1 = xilinx_pwm_calc_tlr(pwm, tcsr1, state->duty_cycle); >> + writel(tlr0, pwm->regs + TLR0); >> + writel(tlr1, pwm->regs + TLR1); > > How does the hardware behave here? E.g. can it happen that after writing > TLR0 we can see a cycle with the new period but the old duty_cycle? Does > it complete the currently running period? TLR is the value loaded into TCR when the counter resets. I believe this happens at the rising edge for TCR0 and at the falling edge for TCR1, but the exact behavior is not documented. Therefore, changing the value of TLR will not affect the current cycle. > > If state->enabled == 0 you want to disable first to prevent that the > (maybe) new values for period and duty_cycle are emitted by the > hardware. > >> + if (state->enabled) { >> + /* Only touch the TCSRs if we aren't already running */ >> + if (!enabled) { >> + /* Load TLR into TCR */ >> + writel(tcsr0 | TCSR_LOAD, pwm->regs + TCSR0); >> + writel(tcsr1 | TCSR_LOAD, pwm->regs + TCSR1); >> + /* Enable timers all at once with ENALL */ >> + tcsr0 = (TCSR_SET & ~TCSR_ENT) | (tcsr0 & TCSR_UDT); >> + tcsr1 = TCSR_SET | TCSR_ENALL | (tcsr1 & TCSR_UDT); >> + writel(tcsr0, pwm->regs + TCSR0); >> + writel(tcsr1, pwm->regs + TCSR1); >> + } >> + } else { >> + writel(tcsr0 & ~TCSR_ENT, pwm->regs + TCSR0); >> + writel(tcsr1 & ~TCSR_ENT, pwm->regs + TCSR1); > > How does the hardware behave on disable? Does it always emit a low > level? Does it complete the currently running period? I don't know. My suspicion is that the output stops at whatever its current value is. > >> + } >> + >> + if (enabled && !state->enabled) >> + clk_rate_exclusive_put(pwm->clk); >> + >> + return 0; >> +} >> + >> +static void xilinx_pwm_get_state(struct pwm_chip *chip, >> + struct pwm_device *unused, >> + struct pwm_state *state) >> +{ >> + struct xilinx_pwm_device *pwm = xilinx_pwm_chip_to_device(chip); >> + u32 tlr0, tlr1, tcsr0, tcsr1; >> + >> + tlr0 = readl(pwm->regs + TLR0); >> + tlr1 = readl(pwm->regs + TLR1); >> + tcsr0 = readl(pwm->regs + TCSR0); >> + tcsr1 = readl(pwm->regs + TCSR1); >> + >> + state->period = xilinx_pwm_get_period(pwm, tlr0, tcsr0); >> + state->duty_cycle = xilinx_pwm_get_period(pwm, tlr1, tcsr1); >> + state->enabled = xilinx_pwm_is_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 is missing OK, will add. > >> +}; >> + >> +static struct dentry *debug_dir; >> + >> +#define DEBUG_REG(reg) { .name = #reg, .offset = (reg), } >> +static struct debugfs_reg32 xilinx_pwm_reg32[] = { >> + DEBUG_REG(TCSR0), >> + DEBUG_REG(TLR0), >> + DEBUG_REG(TCR0), >> + DEBUG_REG(TCSR1), >> + DEBUG_REG(TLR1), >> + DEBUG_REG(TCR1), >> +}; >> + >> +static int xilinx_pwm_debug_create(struct xilinx_pwm_device *pwm) >> +{ >> + struct debugfs_regset32 *regset; >> + >> + if (!IS_ENABLED(CONFIG_DEBUG_FS)) >> + return 0; >> + >> + regset = devm_kzalloc(pwm->chip.dev, sizeof(*regset), GFP_KERNEL); >> + if (!pwm) >> + return -ENOMEM; >> + >> + regset->regs = xilinx_pwm_reg32; >> + regset->nregs = ARRAY_SIZE(xilinx_pwm_reg32); >> + regset->base = pwm->regs; >> + >> + debugfs_create_regset32(dev_name(pwm->chip.dev), 0400, debug_dir, >> + regset); > > debug_dir might not yet be initialized here. Ok, I will move the initialization of the directory before registering this driver in module init. > >> + return 0; >> +} > > This is unusual to have in a driver. Something like memtool[1] should > work just fine, then you don't need this debugfs file. Together with pwm > tracing this should be good enough. I didn't have memtool on my test system, so I added this. I can remove it from the next patch if you'd like. > > [1] https://github.com/pengutronix/memtool > >> +static int xilinx_pwm_probe(struct platform_device *pdev) >> +{ >> + bool enabled; >> + int i, ret; >> + struct device *dev = &pdev->dev; >> + struct xilinx_pwm_device *pwm; >> + u32 one_timer; >> + >> + ret = of_property_read_u32(dev->of_node, "xlnx,one-timer-only", >> + &one_timer); >> + if (ret || one_timer) { >> + dev_err(dev, "two timers are needed for PWM mode\n"); >> + return -EINVAL; >> + } >> + >> + for (i = 0; i < 2; i++) { >> + char fmt[] = "xlnx,gen%u-assert"; >> + char buf[sizeof(fmt)]; >> + u32 gen; >> + >> + snprintf(buf, sizeof(buf), fmt, i); >> + ret = of_property_read_u32(dev->of_node, buf, &gen); >> + if (ret || !gen) { >> + dev_err(dev, "generateout%u must be active high\n", i); >> + return -EINVAL; >> + } >> + } >> + >> + pwm = devm_kzalloc(&pdev->dev, sizeof(*pwm), GFP_KERNEL); >> + if (!pwm) >> + return -ENOMEM; >> + platform_set_drvdata(pdev, pwm); >> + >> + pwm->chip.dev = &pdev->dev; >> + pwm->chip.ops = &xilinx_pwm_ops; >> + pwm->chip.base = -1; > > Please drop the assignment to .base. See commit > f9a8ee8c8bcd118e800d88772c6457381db45224 in next. Ok. >> + pwm->chip.npwm = 1; >> + >> + pwm->regs = devm_platform_ioremap_resource(pdev, 0); >> + if (IS_ERR(pwm->regs)) >> + return PTR_ERR(pwm->regs); >> + >> + ret = of_property_read_u32(dev->of_node, "xlnx,count-width", &pwm->width); >> + if (ret) { >> + dev_err(dev, "missing counter width\n"); >> + return -EINVAL; >> + } > > Would it make sense to error out on some values of pwm->width? Values > bigger than 32 don't make sense? Yes. I will add a range check next revision. > >> + pwm->clk = devm_clk_get(dev, NULL); >> + if (IS_ERR(pwm->clk)) >> + return dev_err_probe(dev, PTR_ERR(pwm->clk), "missing clock\n"); >> + >> + ret = clk_prepare_enable(pwm->clk); >> + if (ret) { >> + dev_err(dev, "clock enable failed\n"); >> + return ret; > > You can use dev_err_probe here, too, for consistency. Ok. Though in this case we will never get -EPROBE_DEFER from clk_prepare_enable, so dev_err_probe is not as useful. > >> + } >> + >> + ret = xilinx_pwm_debug_create(pwm); >> + if (ret) { >> + clk_disable_unprepare(pwm->clk); >> + return ret; >> + } >> + >> + ret = pwmchip_add(&pwm->chip); >> + if (ret) { >> + clk_disable_unprepare(pwm->clk); > > Error message please. Ok. > >> + return ret; >> + } >> + >> + enabled = xilinx_pwm_is_enabled(readl(pwm->regs + TCSR0), >> + readl(pwm->regs + TCSR1)); >> + if (enabled) >> + clk_rate_exclusive_get(pwm->clk); > > This needs to happen before calling pwmchip_add(), doesn't it? Yes. > >> + >> + return ret; >> +} >> + >> +static int xilinx_pwm_remove(struct platform_device *pdev) >> +{ >> + struct xilinx_pwm_device *pwm = platform_get_drvdata(pdev); >> + bool enabled = xilinx_pwm_is_enabled(readl(pwm->regs + TCSR0), >> + readl(pwm->regs + TCSR1)); >> + >> + if (enabled) >> + clk_rate_exclusive_put(pwm->clk); > > This looks wrong. You should rely on the consumer that they disable the > PWM. What about a PWM regulator with always-on? This is mostly to match the exclusive_get in probe, in case there are misbehaving consumers. > >> + clk_disable_unprepare(pwm->clk); > > I assume this stops the PWM, so this must happen after calling > pwmchip_remove. Ok. > >> + return pwmchip_remove(&pwm->chip); > > Note, pwmchip_remove always returns 0 and I plan to change it to return > void instead. So please don't check the return value here. Ok. Thanks for the detailed review. --Sean > >> +} >> + > > Best regards > Uwe >
On 5/4/21 10:46 AM, Sean Anderson wrote: > On 5/4/21 4:51 AM, Uwe Kleine-König wrote: > >> +static int xilinx_pwm_remove(struct platform_device *pdev) > >> +{ > >> + struct xilinx_pwm_device *pwm = platform_get_drvdata(pdev); > >> + bool enabled = xilinx_pwm_is_enabled(readl(pwm->regs + TCSR0), > >> + readl(pwm->regs + TCSR1)); > >> + > >> + if (enabled) > >> + clk_rate_exclusive_put(pwm->clk); > > > > This looks wrong. You should rely on the consumer that they disable the > > PWM. > > What about a PWM regulator with always-on? > > This is mostly to match the exclusive_get in probe, in case there are > misbehaving consumers. Since we always get the rate exclusively, we must always put the rate exclusively here. So if the PWM is probed, but no one ever does anything with it (and therefore no one turns it off), we must release the exclusive rate in remove. --Sean
On 5/4/21 8:32 AM, Michal Simek wrote: > > > On 5/4/21 10:51 AM, Uwe Kleine-König wrote: >> Hello, >> >> On Mon, May 03, 2021 at 05:44:13PM -0400, Sean Anderson wrote: >>> This adds PWM support for Xilinx LogiCORE IP AXI soft timers commonly >>> found on Xilinx FPGAs. There is another driver for this device located >>> at arch/microblaze/kernel/timer.c, but it is only used for timekeeping. >>> 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> >>> --- >>> >>> arch/arm64/configs/defconfig | 1 + >>> drivers/pwm/Kconfig | 11 ++ >>> drivers/pwm/Makefile | 1 + >>> drivers/pwm/pwm-xilinx.c | 322 +++++++++++++++++++++++++++++++++++ >>> 4 files changed, 335 insertions(+) >>> create mode 100644 drivers/pwm/pwm-xilinx.c >>> >>> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig >>> index 08c6f769df9a..81794209f287 100644 >>> --- a/arch/arm64/configs/defconfig >>> +++ b/arch/arm64/configs/defconfig >>> @@ -1083,6 +1083,7 @@ CONFIG_PWM_SAMSUNG=y >>> CONFIG_PWM_SL28CPLD=m >>> CONFIG_PWM_SUN4I=m >>> CONFIG_PWM_TEGRA=m >>> +CONFIG_PWM_XILINX=m >>> CONFIG_SL28CPLD_INTC=y >>> CONFIG_QCOM_PDC=y >>> CONFIG_RESET_IMX7=y >> >> I think this should go into a separate patch once this driver is >> accepted. This can then go via the ARM people. >> >>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig >>> index d3371ac7b871..01e62928f4bf 100644 >>> --- a/drivers/pwm/Kconfig >>> +++ b/drivers/pwm/Kconfig >>> @@ -628,4 +628,15 @@ 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 !MICROBLAZE >> >> I don't understand this dependency. > > The dependency is clear here because microblaze has already driver for > this timer here arch/microblaze/kernel/timer.c. > > And that's exactly pointing to the way how this should be done. > IP itself is single or dual timer and in case of dual timer you can > select if there is pwm output and use it for PWM generation. > > It means it is timer with PMW together. > I didn't have a time but Uwe likely knows this better how to design it. > > I see that gpio-mvebu driver instantiate pwm driver. Maybe that's the > way to go. I think drivers/clocksource/samsung_pwm_timer.c and drivers/pwm/pwm-samsung.c provide another example for how to go about this. > Step first is move axi timer driver from microblaze to generic location. Yes. However, I don't have a microblaze setup, so I have just added the PWM driver. > Figured it out how to add PWM (with DT flag) and then write support for it. I would really like to see a standard way of doing this. Many timers also have PWM support (e.g. samsung, and also DW). I think it's unusual that this problem has not already been addressed. --Sean > > Thanks, > Michal >
Hello Sean, [Adding Rob to the list of recipents, as he for sure has a valuable opinion on this matter.] On Tue, May 04, 2021 at 11:57:20AM -0400, Sean Anderson wrote: > On 5/4/21 8:32 AM, Michal Simek wrote: > > On 5/4/21 10:51 AM, Uwe Kleine-König wrote: > >> On Mon, May 03, 2021 at 05:44:13PM -0400, Sean Anderson wrote: > >>> This adds PWM support for Xilinx LogiCORE IP AXI soft timers commonly > >>> found on Xilinx FPGAs. There is another driver for this device located > >>> at arch/microblaze/kernel/timer.c, but it is only used for timekeeping. > >>> 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> > >>> --- > >>> > >>> arch/arm64/configs/defconfig | 1 + > >>> drivers/pwm/Kconfig | 11 ++ > >>> drivers/pwm/Makefile | 1 + > >>> drivers/pwm/pwm-xilinx.c | 322 +++++++++++++++++++++++++++++++++++ > >>> 4 files changed, 335 insertions(+) > >>> create mode 100644 drivers/pwm/pwm-xilinx.c > >>> > >>> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig > >>> index 08c6f769df9a..81794209f287 100644 > >>> --- a/arch/arm64/configs/defconfig > >>> +++ b/arch/arm64/configs/defconfig > >>> @@ -1083,6 +1083,7 @@ CONFIG_PWM_SAMSUNG=y > >>> CONFIG_PWM_SL28CPLD=m > >>> CONFIG_PWM_SUN4I=m > >>> CONFIG_PWM_TEGRA=m > >>> +CONFIG_PWM_XILINX=m > >>> CONFIG_SL28CPLD_INTC=y > >>> CONFIG_QCOM_PDC=y > >>> CONFIG_RESET_IMX7=y > >> > >> I think this should go into a separate patch once this driver is > >> accepted. This can then go via the ARM people. > >> > >>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > >>> index d3371ac7b871..01e62928f4bf 100644 > >>> --- a/drivers/pwm/Kconfig > >>> +++ b/drivers/pwm/Kconfig > >>> @@ -628,4 +628,15 @@ 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 !MICROBLAZE > >> > >> I don't understand this dependency. > > > > The dependency is clear here because microblaze has already driver for > > this timer here arch/microblaze/kernel/timer.c. Then at least add a comment. > > And that's exactly pointing to the way how this should be done. > > IP itself is single or dual timer and in case of dual timer you can > > select if there is pwm output and use it for PWM generation. > > > > It means it is timer with PMW together. > > I didn't have a time but Uwe likely knows this better how to design it. > > > > I see that gpio-mvebu driver instantiate pwm driver. Maybe that's the > > way to go. > > I think drivers/clocksource/samsung_pwm_timer.c and > drivers/pwm/pwm-samsung.c provide another example for how to go about > this. I recently had a similar problem (with code that isn't (yet) in mainline), where a timer can be used as a counter. I chose to change the compatible. Transferred to this example this would mean to use e.g. static const struct of_device_id xilinx_pwm_of_match[] = { { .compatible = "xlnx,xps-timer-pwm-1.00.a" }, ... }; and if you want to use the hardware as a PWM, you overwrite the compatible in your machine.dts. Not sure however that this is nice enough to be accepted by the dt people?! Best regards Uwe
Hello Sean, On Tue, May 04, 2021 at 10:46:39AM -0400, Sean Anderson wrote: > On 5/4/21 4:51 AM, Uwe Kleine-König wrote: > > On Mon, May 03, 2021 at 05:44:13PM -0400, Sean Anderson wrote: > >> This adds PWM support for Xilinx LogiCORE IP AXI soft timers commonly > >> found on Xilinx FPGAs. There is another driver for this device located > >> at arch/microblaze/kernel/timer.c, but it is only used for timekeeping. > >> 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> > >> --- > >> > >> arch/arm64/configs/defconfig | 1 + > >> drivers/pwm/Kconfig | 11 ++ > >> drivers/pwm/Makefile | 1 + > >> drivers/pwm/pwm-xilinx.c | 322 +++++++++++++++++++++++++++++++++++ > >> 4 files changed, 335 insertions(+) > >> create mode 100644 drivers/pwm/pwm-xilinx.c > >> > >> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig > >> index 08c6f769df9a..81794209f287 100644 > >> --- a/arch/arm64/configs/defconfig > >> +++ b/arch/arm64/configs/defconfig > >> @@ -1083,6 +1083,7 @@ CONFIG_PWM_SAMSUNG=y > >> CONFIG_PWM_SL28CPLD=m > >> CONFIG_PWM_SUN4I=m > >> CONFIG_PWM_TEGRA=m > >> +CONFIG_PWM_XILINX=m > >> CONFIG_SL28CPLD_INTC=y > >> CONFIG_QCOM_PDC=y > >> CONFIG_RESET_IMX7=y > > > > I think this should go into a separate patch once this driver is > > accepted. This can then go via the ARM people. > > Sure. > > > > >> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > >> index d3371ac7b871..01e62928f4bf 100644 > >> --- a/drivers/pwm/Kconfig > >> +++ b/drivers/pwm/Kconfig > >> @@ -628,4 +628,15 @@ 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 !MICROBLAZE > > > > I don't understand this dependency. > > As noted in the commit message, there is already a driver for this > device for microblaze systems. To prevent conflicts, this driver is > disabled on microblaze. OK, already understood that after reading the other replies to this thread. As noted there, please add a comment describing the problem. You probably also need some more dependencies, at least COMMON_CLK for clk_rate_exclusive_get() and probably also HAS_IOMEM for readl/writel. > >> +#include <asm/io.h> > >> +#include <linux/clk.h> > >> +#include <linux/device.h> > >> +#include <linux/debugfs.h> > >> +#include <linux/module.h> > >> +#include <linux/platform_device.h> > >> +#include <linux/pwm.h> > >> + > >> +#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) > > > > I'd like to see a prefix for these defines, something like XILINX_PWM_ > > maybe? > > I don't think that's necessary, since these defines are used only for > this file. In particular, adding a prefix like that would significantly > lengthen lines which add several flags. This is something where Thierry and I don't agree, so you can at least expect support for your point. Still I see a value that is usually worth the additional horizontal space. The prefix would make it obvious that this is a local symbol. It also helps to jump to the right definition if you use ctags or something similar. The most affected code lines are probably: + return !(~tcsr0 & TCSR_SET || tcsr0 & (TCSR_CLEAR | TCSR_CASC) || + ~tcsr1 & TCSR_SET || tcsr1 & TCSR_CLEAR); which IMHO is already hard to read and adding a newline after each || doesn't hurt. Or write it as: #define XILINX_PWM_TCSR_RUN_SET \ (XILINX_PWM_TCSR_GENT | \ XILINX_PWM_TCSR_ARHT | \ XILINX_PWM_TCSR_ENT | \ XILINX_PWM_TCSR_PWMA) #define XILINX_PWM_TCSR_RUN_CLEAR \ (XILINX_PWM_TCSR_MDT | \ XILINX_PWM_TCSR_LOAD) #define XILINX_PWM_TCSR_RUN_MASK \ (XILINX_PWM_TCSR_RUN_SET | XILINX_PWM_TCSR_RUN_CLEAR) ... /* * Both TCSR registers must be setup according to the running * value, TCSR0 must also have the CASC bit set. */ if ((tcsr0 & XILINX_PWM_TCSR_RUN_MASK) != XILINX_PWM_TCSR_RUN_SET || !(tcsr0 & TCSR_CASC) || (tcsr1 & XILINX_PWM_TCSR_RUN_MASK) != XILINX_PWM_TCSR_RUN_SET) which IMHO is better understandable and still fits the line length. (Or simplify the check to only test for a single bit?) + tcsr0 = (TCSR_SET & ~TCSR_ENT) | (tcsr0 & TCSR_UDT); + tcsr1 = TCSR_SET | TCSR_ENALL | (tcsr1 & TCSR_UDT); is similar. I wonder if it makes sense to drop ENT from the default bit mask and if after that xilinx_pwm_is_enabled() yields true. I also don't understand the semantic of the UDT bit. You never modify it. Assuming it has a relevant meaning the driver's behaviour depends on the value the bootloader initialized that bit to? So these lines might need some explanation anyhow and having to write it over several lines doesn't actually hurt. bool enabled = xilinx_pwm_is_enabled(readl(pwm->regs + TCSR0), readl(pwm->regs + TCSR1)); could be rewritten as u32 tcsr0 = readl(pwm->regs + XILINX_PWM_TCSR0); u32 tcsr1 = readl(pwm->regs + XILINX_PWM_TCSR1); bool enabled = xilinx_pwm_is_enabled(tcsr0, tcsr1); which IMHO reads better and the required horizontal space is even less than with your lines even tough I have added a prefix. > >> +static int xilinx_pwm_apply(struct pwm_chip *chip, struct pwm_device *unused, > >> + const struct pwm_state *state) > >> +{ > >> + struct xilinx_pwm_device *pwm = xilinx_pwm_chip_to_device(chip); > >> + u32 tlr0, tlr1; > >> + u32 tcsr0 = readl(pwm->regs + TCSR0); > >> + u32 tcsr1 = readl(pwm->regs + TCSR1); > >> + bool enabled = xilinx_pwm_is_enabled(tcsr0, tcsr1); > >> + > >> + if (state->polarity != PWM_POLARITY_NORMAL) > >> + return -EINVAL; > >> + > >> + if (!enabled && state->enabled) > >> + clk_rate_exclusive_get(pwm->clk); > > > > Missing error checking. > > This cannot fail. Oh indeed. IMHO it should be changed to return void and the comment "Returns 0 on success, -EERROR otherwise" should be adapted to reality. > >> + tlr1 = xilinx_pwm_calc_tlr(pwm, tcsr1, state->duty_cycle); > >> + writel(tlr0, pwm->regs + TLR0); > >> + writel(tlr1, pwm->regs + TLR1); > > > > How does the hardware behave here? E.g. can it happen that after writing > > TLR0 we can see a cycle with the new period but the old duty_cycle? Does > > it complete the currently running period? > > TLR is the value loaded into TCR when the counter resets. I believe this > happens at the rising edge for TCR0 and at the falling edge for TCR1, > but the exact behavior is not documented. Therefore, changing the value > of TLR will not affect the current cycle. So we might still get a mixed period if the counter resets between the two writel commands, right? > > If state->enabled == 0 you want to disable first to prevent that the > > (maybe) new values for period and duty_cycle are emitted by the > > hardware. > > > >> + if (state->enabled) { > >> + /* Only touch the TCSRs if we aren't already running */ > >> + if (!enabled) { > >> + /* Load TLR into TCR */ > >> + writel(tcsr0 | TCSR_LOAD, pwm->regs + TCSR0); > >> + writel(tcsr1 | TCSR_LOAD, pwm->regs + TCSR1); > >> + /* Enable timers all at once with ENALL */ > >> + tcsr0 = (TCSR_SET & ~TCSR_ENT) | (tcsr0 & TCSR_UDT); > >> + tcsr1 = TCSR_SET | TCSR_ENALL | (tcsr1 & TCSR_UDT); > >> + writel(tcsr0, pwm->regs + TCSR0); > >> + writel(tcsr1, pwm->regs + TCSR1); > >> + } > >> + } else { > >> + writel(tcsr0 & ~TCSR_ENT, pwm->regs + TCSR0); > >> + writel(tcsr1 & ~TCSR_ENT, pwm->regs + TCSR1); > > > > How does the hardware behave on disable? Does it always emit a low > > level? Does it complete the currently running period? > > I don't know. My suspicion is that the output stops at whatever its > current value is. Would be great if you could find out that. Please document that in the Limitations paragraph at the top of the driver. > > This is unusual to have in a driver. Something like memtool[1] should > > work just fine, then you don't need this debugfs file. Together with pwm > > tracing this should be good enough. > > I didn't have memtool on my test system, so I added this. I can remove > it from the next patch if you'd like. Yes, please remove it. (And install memtool or one of its alternatives :-). > >> + pwm->clk = devm_clk_get(dev, NULL); > >> + if (IS_ERR(pwm->clk)) > >> + return dev_err_probe(dev, PTR_ERR(pwm->clk), "missing clock\n"); > >> + > >> + ret = clk_prepare_enable(pwm->clk); > >> + if (ret) { > >> + dev_err(dev, "clock enable failed\n"); > >> + return ret; > > > > You can use dev_err_probe here, too, for consistency. > > Ok. Though in this case we will never get -EPROBE_DEFER from > clk_prepare_enable, so dev_err_probe is not as useful. Everything it does here is useful, and the overhead is only the check against -EPROBE_DEFER. In return you get consistent error handling and the messages are also all in the same format. (And compared to your version the error message also gives a hint about the actual problem as it emits the error code.) > >> +static int xilinx_pwm_remove(struct platform_device *pdev) > >> +{ > >> + struct xilinx_pwm_device *pwm = platform_get_drvdata(pdev); > >> + bool enabled = xilinx_pwm_is_enabled(readl(pwm->regs + TCSR0), > >> + readl(pwm->regs + TCSR1)); > >> + > >> + if (enabled) > >> + clk_rate_exclusive_put(pwm->clk); Note that I misunderstood what clk_rate_exclusive_put does. This only drops the lock on the clock that prevents that others modify the clock rate. So that call is fine. > > This looks wrong. You should rely on the consumer that they disable the > > PWM. > > What about a PWM regulator with always-on? So in general: What do you think should happen? When the remove function is called the PWM regulator driver is already gone, so there is no way to get the information that the PWM should stay on ... > >> + clk_disable_unprepare(pwm->clk); ... and still you stop the PWM here (or does that only affect access to its registers?) So it's a delicate topic and there isn't the single right thing to do here. So usually I request to free all resources allocated in .probe() and otherwise leave the hardware alone. Now that I understood clk_rate_exclusive_put() the status quo is fine for me. Just call pwmchip_remove first and only then release the resources needed to operate the PWM. Best regards Uwe
On 5/3/21 10:24 PM, kernel test robot wrote: > Hi Sean, > > Thank you for the patch! Yet something to improve: > > [auto build test ERROR on arm64/for-next/core] > [also build test ERROR on linux/master linus/master v5.12 next-20210503] > [cannot apply to pwm/for-next 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/20210504-054451 > base: https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-next/core > config: sh-allmodconfig (attached as .config) > compiler: sh4-linux-gcc (GCC) 9.3.0 > reproduce (this is a W=1 build): > wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross > chmod +x ~/bin/make.cross > # https://github.com/0day-ci/linux/commit/164c8df701eef0c9509d544501a1de5c406e93cb > 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/20210504-054451 > git checkout 164c8df701eef0c9509d544501a1de5c406e93cb > # save the attached .config to linux build tree > COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross W=1 ARCH=sh > > 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: "__delay" [drivers/net/mdio/mdio-cavium.ko] undefined! >>> ERROR: modpost: "clk_rate_exclusive_get" [drivers/pwm/pwm-xilinx.ko] undefined! >>> ERROR: modpost: "clk_rate_exclusive_put" [drivers/pwm/pwm-xilinx.ko] undefined! These should be defined in drivers/sh/clk/core.c, because CONFIG_HAVE_CLK is selected (see the definition in include/linux/clk.h) but they are not. +CC SH maintainers --Sean > ERROR: modpost: "__udivdi3" [fs/btrfs/btrfs.ko] undefined! > ERROR: modpost: "__umoddi3" [fs/btrfs/btrfs.ko] undefined! > > Kconfig warnings: (for reference only) > WARNING: unmet direct dependencies detected for SND_ATMEL_SOC_PDC > Depends on SOUND && !UML && SND && SND_SOC && SND_ATMEL_SOC && HAS_DMA > Selected by > - SND_ATMEL_SOC_SSC && SOUND && !UML && SND && SND_SOC && SND_ATMEL_SOC > - SND_ATMEL_SOC_SSC_PDC && SOUND && !UML && SND && SND_SOC && SND_ATMEL_SOC && ATMEL_SSC > > --- > 0-DAY CI Kernel Test Service, Intel Corporation > https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org >
On 5/4/21 1:15 PM, Uwe Kleine-König wrote: > Hello Sean, > > On Tue, May 04, 2021 at 10:46:39AM -0400, Sean Anderson wrote: >> On 5/4/21 4:51 AM, Uwe Kleine-König wrote: >>> On Mon, May 03, 2021 at 05:44:13PM -0400, Sean Anderson wrote: >>>> This adds PWM support for Xilinx LogiCORE IP AXI soft timers commonly >>>> found on Xilinx FPGAs. There is another driver for this device located >>>> at arch/microblaze/kernel/timer.c, but it is only used for timekeeping. >>>> 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> >>>> --- >>>> >>>> arch/arm64/configs/defconfig | 1 + >>>> drivers/pwm/Kconfig | 11 ++ >>>> drivers/pwm/Makefile | 1 + >>>> drivers/pwm/pwm-xilinx.c | 322 +++++++++++++++++++++++++++++++++++ >>>> 4 files changed, 335 insertions(+) >>>> create mode 100644 drivers/pwm/pwm-xilinx.c >>>> >>>> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig >>>> index 08c6f769df9a..81794209f287 100644 >>>> --- a/arch/arm64/configs/defconfig >>>> +++ b/arch/arm64/configs/defconfig >>>> @@ -1083,6 +1083,7 @@ CONFIG_PWM_SAMSUNG=y >>>> CONFIG_PWM_SL28CPLD=m >>>> CONFIG_PWM_SUN4I=m >>>> CONFIG_PWM_TEGRA=m >>>> +CONFIG_PWM_XILINX=m >>>> CONFIG_SL28CPLD_INTC=y >>>> CONFIG_QCOM_PDC=y >>>> CONFIG_RESET_IMX7=y >>> >>> I think this should go into a separate patch once this driver is >>> accepted. This can then go via the ARM people. >> >> Sure. >> >>> >>>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig >>>> index d3371ac7b871..01e62928f4bf 100644 >>>> --- a/drivers/pwm/Kconfig >>>> +++ b/drivers/pwm/Kconfig >>>> @@ -628,4 +628,15 @@ 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 !MICROBLAZE >>> >>> I don't understand this dependency. >> >> As noted in the commit message, there is already a driver for this >> device for microblaze systems. To prevent conflicts, this driver is >> disabled on microblaze. > > OK, already understood that after reading the other replies to this > thread. As noted there, please add a comment describing the problem. Added. > You probably also need some more dependencies, at least COMMON_CLK for > clk_rate_exclusive_get() and probably also HAS_IOMEM for readl/writel. I will add a dependency on HAS_IOMEM. COMMON_CLK should not be necessary, because clk_exclusive_(get|put) should be defined by the platform if they define CONFIG_HAVE_CLK (otherwise we get a stub as define in the linux clock header). But of course, this is not the case on SH4... I wish this sort of thing were documented somewhere. I can add this if necessary. > >>>> +#include <asm/io.h> >>>> +#include <linux/clk.h> >>>> +#include <linux/device.h> >>>> +#include <linux/debugfs.h> >>>> +#include <linux/module.h> >>>> +#include <linux/platform_device.h> >>>> +#include <linux/pwm.h> >>>> + >>>> +#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) >>> >>> I'd like to see a prefix for these defines, something like XILINX_PWM_ >>> maybe? >> >> I don't think that's necessary, since these defines are used only for >> this file. In particular, adding a prefix like that would significantly >> lengthen lines which add several flags. > > This is something where Thierry and I don't agree, so you can at least > expect support for your point. Still I see a value that is usually worth > the additional horizontal space. > > The prefix would make it obvious that this is a local symbol. It also > helps to jump to the right definition if you use ctags or something > similar. > > The most affected code lines are probably: > > + return !(~tcsr0 & TCSR_SET || tcsr0 & (TCSR_CLEAR | TCSR_CASC) || > + ~tcsr1 & TCSR_SET || tcsr1 & TCSR_CLEAR); > > which IMHO is already hard to read and adding a newline after each || > doesn't hurt. This is done intentionally to highlight that we are doing the same checks for TCSR0 and TCSR1. > > Or write it as: > > #define XILINX_PWM_TCSR_RUN_SET \ > (XILINX_PWM_TCSR_GENT | \ > XILINX_PWM_TCSR_ARHT | \ > XILINX_PWM_TCSR_ENT | \ > XILINX_PWM_TCSR_PWMA) > > #define XILINX_PWM_TCSR_RUN_CLEAR \ > (XILINX_PWM_TCSR_MDT | \ > XILINX_PWM_TCSR_LOAD) > > #define XILINX_PWM_TCSR_RUN_MASK \ > (XILINX_PWM_TCSR_RUN_SET | XILINX_PWM_TCSR_RUN_CLEAR) > > > ... > > /* > * Both TCSR registers must be setup according to the running > * value, TCSR0 must also have the CASC bit set. > */ > if ((tcsr0 & XILINX_PWM_TCSR_RUN_MASK) != XILINX_PWM_TCSR_RUN_SET || > !(tcsr0 & TCSR_CASC) || > (tcsr1 & XILINX_PWM_TCSR_RUN_MASK) != XILINX_PWM_TCSR_RUN_SET) > > which IMHO is better understandable and still fits the line length. > (Or simplify the check to only test for a single bit?) > > + tcsr0 = (TCSR_SET & ~TCSR_ENT) | (tcsr0 & TCSR_UDT); > + tcsr1 = TCSR_SET | TCSR_ENALL | (tcsr1 & TCSR_UDT); > > is similar. I wonder if it makes sense to drop ENT from the default bit > mask and if after that xilinx_pwm_is_enabled() yields true. 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. > I also don't understand the semantic of the UDT bit. You never modify > it. Assuming it has a relevant meaning the driver's behaviour depends > on the value the bootloader initialized that bit to? UDT is whether the timer counts upward or downward. We keep whatever the bootloader (or the power-on reset) set so we don't cause an incorrect pulse between when we write TLR and when we write TCSR. > > So these lines might need some explanation anyhow and having to write it > over several lines doesn't actually hurt. > > bool enabled = xilinx_pwm_is_enabled(readl(pwm->regs + TCSR0), > readl(pwm->regs + TCSR1)); > > could be rewritten as > > u32 tcsr0 = readl(pwm->regs + XILINX_PWM_TCSR0); > u32 tcsr1 = readl(pwm->regs + XILINX_PWM_TCSR1); > bool enabled = xilinx_pwm_is_enabled(tcsr0, tcsr1); > > which IMHO reads better and the required horizontal space is even less > than with your lines even tough I have added a prefix. > >>>> +static int xilinx_pwm_apply(struct pwm_chip *chip, struct pwm_device *unused, >>>> + const struct pwm_state *state) >>>> +{ >>>> + struct xilinx_pwm_device *pwm = xilinx_pwm_chip_to_device(chip); >>>> + u32 tlr0, tlr1; >>>> + u32 tcsr0 = readl(pwm->regs + TCSR0); >>>> + u32 tcsr1 = readl(pwm->regs + TCSR1); >>>> + bool enabled = xilinx_pwm_is_enabled(tcsr0, tcsr1); >>>> + >>>> + if (state->polarity != PWM_POLARITY_NORMAL) >>>> + return -EINVAL; >>>> + >>>> + if (!enabled && state->enabled) >>>> + clk_rate_exclusive_get(pwm->clk); >>> >>> Missing error checking. >> >> This cannot fail. > > Oh indeed. IMHO it should be changed to return void and the comment > "Returns 0 on success, -EERROR otherwise" should be adapted to reality. > >>>> + tlr1 = xilinx_pwm_calc_tlr(pwm, tcsr1, state->duty_cycle); >>>> + writel(tlr0, pwm->regs + TLR0); >>>> + writel(tlr1, pwm->regs + TLR1); >>> >>> How does the hardware behave here? E.g. can it happen that after writing >>> TLR0 we can see a cycle with the new period but the old duty_cycle? Does >>> it complete the currently running period? >> >> TLR is the value loaded into TCR when the counter resets. I believe this >> happens at the rising edge for TCR0 and at the falling edge for TCR1, >> but the exact behavior is not documented. Therefore, changing the value >> of TLR will not affect the current cycle. > > So we might still get a mixed period if the counter resets between the > two writel commands, right? Yes. I have noted this in V2. >>> If state->enabled == 0 you want to disable first to prevent that the >>> (maybe) new values for period and duty_cycle are emitted by the >>> hardware. >>> >>>> + if (state->enabled) { >>>> + /* Only touch the TCSRs if we aren't already running */ >>>> + if (!enabled) { >>>> + /* Load TLR into TCR */ >>>> + writel(tcsr0 | TCSR_LOAD, pwm->regs + TCSR0); >>>> + writel(tcsr1 | TCSR_LOAD, pwm->regs + TCSR1); >>>> + /* Enable timers all at once with ENALL */ >>>> + tcsr0 = (TCSR_SET & ~TCSR_ENT) | (tcsr0 & TCSR_UDT); >>>> + tcsr1 = TCSR_SET | TCSR_ENALL | (tcsr1 & TCSR_UDT); >>>> + writel(tcsr0, pwm->regs + TCSR0); >>>> + writel(tcsr1, pwm->regs + TCSR1); >>>> + } >>>> + } else { >>>> + writel(tcsr0 & ~TCSR_ENT, pwm->regs + TCSR0); >>>> + writel(tcsr1 & ~TCSR_ENT, pwm->regs + TCSR1); >>> >>> How does the hardware behave on disable? Does it always emit a low >>> level? Does it complete the currently running period? >> >> I don't know. My suspicion is that the output stops at whatever its >> current value is. > > Would be great if you could find out that. Please document that in the > Limitations paragraph at the top of the driver. Well, as it turns out, just clearing ENT doesn't disable the PWM output (!). However, clearing everything in TCSR_SET does disable the PWM. The output is low when it is disabled. >>> This is unusual to have in a driver. Something like memtool[1] should >>> work just fine, then you don't need this debugfs file. Together with pwm >>> tracing this should be good enough. >> >> I didn't have memtool on my test system, so I added this. I can remove >> it from the next patch if you'd like. > > Yes, please remove it. (And install memtool or one of its alternatives > :-). I'll be sad to see it go. It is very convenient for debugging. >>>> + pwm->clk = devm_clk_get(dev, NULL); >>>> + if (IS_ERR(pwm->clk)) >>>> + return dev_err_probe(dev, PTR_ERR(pwm->clk), "missing clock\n"); >>>> + >>>> + ret = clk_prepare_enable(pwm->clk); >>>> + if (ret) { >>>> + dev_err(dev, "clock enable failed\n"); >>>> + return ret; >>> >>> You can use dev_err_probe here, too, for consistency. >> >> Ok. Though in this case we will never get -EPROBE_DEFER from >> clk_prepare_enable, so dev_err_probe is not as useful. > > Everything it does here is useful, and the overhead is only the check > against -EPROBE_DEFER. In return you get consistent error handling and > the messages are also all in the same format. (And compared to your > version the error message also gives a hint about the actual problem as > it emits the error code.) Shouldn't the error return be printed by whoever called this probe? >>>> +static int xilinx_pwm_remove(struct platform_device *pdev) >>>> +{ >>>> + struct xilinx_pwm_device *pwm = platform_get_drvdata(pdev); >>>> + bool enabled = xilinx_pwm_is_enabled(readl(pwm->regs + TCSR0), >>>> + readl(pwm->regs + TCSR1)); >>>> + >>>> + if (enabled) >>>> + clk_rate_exclusive_put(pwm->clk); > > Note that I misunderstood what clk_rate_exclusive_put does. This only > drops the lock on the clock that prevents that others modify the clock > rate. So that call is fine. > >>> This looks wrong. You should rely on the consumer that they disable the >>> PWM. >> >> What about a PWM regulator with always-on? > > So in general: What do you think should happen? When the remove function > is called the PWM regulator driver is already gone, so there is no way > to get the information that the PWM should stay on ... Well, we can still tell whether the PWM is on or off. If it should turn off, then someone should have disabled it. >>>> + clk_disable_unprepare(pwm->clk); > > ... and still you stop the PWM here (or does that only affect access to > its registers?) This will turn off the PWM if we are the last user of this clock and no one else has set e.g. CLK_IS_CRITICAL (which is what I would expect if this powered a critical regulator). --Sean > > So it's a delicate topic and there isn't the single right thing to do > here. So usually I request to free all resources allocated in .probe() > and otherwise leave the hardware alone. Now that I understood > clk_rate_exclusive_put() the status quo is fine for me. Just call > pwmchip_remove first and only then release the resources needed to > operate the PWM. > > Best regards > Uwe >
Hi, On 5/4/21 6:13 PM, Uwe Kleine-König wrote: > Hello Sean, > > [Adding Rob to the list of recipents, as he for sure has a valuable > opinion on this matter.] > > On Tue, May 04, 2021 at 11:57:20AM -0400, Sean Anderson wrote: >> On 5/4/21 8:32 AM, Michal Simek wrote: >>> On 5/4/21 10:51 AM, Uwe Kleine-König wrote: >>>> On Mon, May 03, 2021 at 05:44:13PM -0400, Sean Anderson wrote: >>>>> This adds PWM support for Xilinx LogiCORE IP AXI soft timers commonly >>>>> found on Xilinx FPGAs. There is another driver for this device located >>>>> at arch/microblaze/kernel/timer.c, but it is only used for timekeeping. >>>>> 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> >>>>> --- >>>>> >>>>> arch/arm64/configs/defconfig | 1 + >>>>> drivers/pwm/Kconfig | 11 ++ >>>>> drivers/pwm/Makefile | 1 + >>>>> drivers/pwm/pwm-xilinx.c | 322 +++++++++++++++++++++++++++++++++++ >>>>> 4 files changed, 335 insertions(+) >>>>> create mode 100644 drivers/pwm/pwm-xilinx.c >>>>> >>>>> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig >>>>> index 08c6f769df9a..81794209f287 100644 >>>>> --- a/arch/arm64/configs/defconfig >>>>> +++ b/arch/arm64/configs/defconfig >>>>> @@ -1083,6 +1083,7 @@ CONFIG_PWM_SAMSUNG=y >>>>> CONFIG_PWM_SL28CPLD=m >>>>> CONFIG_PWM_SUN4I=m >>>>> CONFIG_PWM_TEGRA=m >>>>> +CONFIG_PWM_XILINX=m >>>>> CONFIG_SL28CPLD_INTC=y >>>>> CONFIG_QCOM_PDC=y >>>>> CONFIG_RESET_IMX7=y >>>> >>>> I think this should go into a separate patch once this driver is >>>> accepted. This can then go via the ARM people. >>>> >>>>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig >>>>> index d3371ac7b871..01e62928f4bf 100644 >>>>> --- a/drivers/pwm/Kconfig >>>>> +++ b/drivers/pwm/Kconfig >>>>> @@ -628,4 +628,15 @@ 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 !MICROBLAZE >>>> >>>> I don't understand this dependency. >>> >>> The dependency is clear here because microblaze has already driver for >>> this timer here arch/microblaze/kernel/timer.c. > > Then at least add a comment. I don't think that comment will really solve this. We should never duplicate driver for the same IP to two locations. Maybe this should be MFD driver. > >>> And that's exactly pointing to the way how this should be done. >>> IP itself is single or dual timer and in case of dual timer you can >>> select if there is pwm output and use it for PWM generation. >>> >>> It means it is timer with PMW together. >>> I didn't have a time but Uwe likely knows this better how to design it. >>> >>> I see that gpio-mvebu driver instantiate pwm driver. Maybe that's the >>> way to go. >> >> I think drivers/clocksource/samsung_pwm_timer.c and >> drivers/pwm/pwm-samsung.c provide another example for how to go about >> this. > > I recently had a similar problem (with code that isn't (yet) in > mainline), where a timer can be used as a counter. I chose to change the > compatible. Transferred to this example this would mean to use e.g. > > static const struct of_device_id xilinx_pwm_of_match[] = { > { .compatible = "xlnx,xps-timer-pwm-1.00.a" }, > ... > }; > > and if you want to use the hardware as a PWM, you overwrite the > compatible in your machine.dts. It is HW selection inside that IP. It means you will get dt properly when PWM output is selected. I understand that this shortcut can work but I don't think it is proper design. > Not sure however that this is nice enough to be accepted by the dt > people?! up to Rob. Thanks, Michal
diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig index 08c6f769df9a..81794209f287 100644 --- a/arch/arm64/configs/defconfig +++ b/arch/arm64/configs/defconfig @@ -1083,6 +1083,7 @@ CONFIG_PWM_SAMSUNG=y CONFIG_PWM_SL28CPLD=m CONFIG_PWM_SUN4I=m CONFIG_PWM_TEGRA=m +CONFIG_PWM_XILINX=m CONFIG_SL28CPLD_INTC=y CONFIG_QCOM_PDC=y CONFIG_RESET_IMX7=y diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index d3371ac7b871..01e62928f4bf 100644 --- a/drivers/pwm/Kconfig +++ b/drivers/pwm/Kconfig @@ -628,4 +628,15 @@ 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 !MICROBLAZE + help + PWM framework driver for Xilinx LogiCORE IP AXI Timers. This + timer is typically a soft core which may be present in Xilinx + FPGAs. 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 d3879619bd76..fc1bd6ccc9ed 100644 --- a/drivers/pwm/Makefile +++ b/drivers/pwm/Makefile @@ -59,3 +59,4 @@ obj-$(CONFIG_PWM_TIEHRPWM) += pwm-tiehrpwm.o obj-$(CONFIG_PWM_TWL) += pwm-twl.o obj-$(CONFIG_PWM_TWL_LED) += pwm-twl-led.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..240bd2993f97 --- /dev/null +++ b/drivers/pwm/pwm-xilinx.c @@ -0,0 +1,322 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Copyright (C) 2021 Sean Anderson <sean.anderson@seco.com> + */ +#include <asm/io.h> +#include <linux/clk.h> +#include <linux/device.h> +#include <linux/debugfs.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/pwm.h> + +#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) + +/* Bits we need to set/clear to enable PWM mode, not including CASC */ +#define TCSR_SET (TCSR_GENT | TCSR_ARHT | TCSR_ENT | TCSR_PWMA) +#define TCSR_CLEAR (TCSR_MDT | TCSR_LOAD) + +#define NSEC_PER_SEC_ULL 1000000000ULL + +/** + * struct xilinx_pwm_device - Driver data for Xilinx AXI timer PWM driver + * @chip: PWM controller chip + * @clk: Parent clock + * @regs: Base address of this device + * @width: Width of the counters, in bits + */ +struct xilinx_pwm_device { + struct pwm_chip chip; + struct clk *clk; + void __iomem *regs; + unsigned int width; +}; + +static inline struct xilinx_pwm_device *xilinx_pwm_chip_to_device(struct pwm_chip *chip) +{ + return container_of(chip, struct xilinx_pwm_device, chip); +} + +static bool xilinx_pwm_is_enabled(u32 tcsr0, u32 tcsr1) +{ + return !(~tcsr0 & TCSR_SET || tcsr0 & (TCSR_CLEAR | TCSR_CASC) || + ~tcsr1 & TCSR_SET || tcsr1 & TCSR_CLEAR); +} + +static u32 xilinx_pwm_calc_tlr(struct xilinx_pwm_device *pwm, u32 tcsr, + unsigned int period) +{ + u64 cycles = DIV_ROUND_DOWN_ULL(period * clk_get_rate(pwm->clk), + NSEC_PER_SEC_ULL); + + if (tcsr & TCSR_UDT) + return cycles - 2; + else + return (BIT_ULL(pwm->width) - 1) - cycles + 2; +} + +static unsigned int xilinx_pwm_get_period(struct xilinx_pwm_device *pwm, + u32 tlr, u32 tcsr) +{ + u64 cycles; + + if (tcsr & TCSR_UDT) + cycles = tlr + 2; + else + cycles = (BIT_ULL(pwm->width) - 1) - tlr + 2; + + return DIV_ROUND_DOWN_ULL(cycles * NSEC_PER_SEC_ULL, + clk_get_rate(pwm->clk)); +} + +static int xilinx_pwm_apply(struct pwm_chip *chip, struct pwm_device *unused, + const struct pwm_state *state) +{ + struct xilinx_pwm_device *pwm = xilinx_pwm_chip_to_device(chip); + u32 tlr0, tlr1; + u32 tcsr0 = readl(pwm->regs + TCSR0); + u32 tcsr1 = readl(pwm->regs + TCSR1); + bool enabled = xilinx_pwm_is_enabled(tcsr0, tcsr1); + + if (state->polarity != PWM_POLARITY_NORMAL) + return -EINVAL; + + if (!enabled && state->enabled) + clk_rate_exclusive_get(pwm->clk); + + tlr0 = xilinx_pwm_calc_tlr(pwm, tcsr0, state->period); + tlr1 = xilinx_pwm_calc_tlr(pwm, tcsr1, state->duty_cycle); + writel(tlr0, pwm->regs + TLR0); + writel(tlr1, pwm->regs + TLR1); + + if (state->enabled) { + /* Only touch the TCSRs if we aren't already running */ + if (!enabled) { + /* Load TLR into TCR */ + writel(tcsr0 | TCSR_LOAD, pwm->regs + TCSR0); + writel(tcsr1 | TCSR_LOAD, pwm->regs + TCSR1); + /* Enable timers all at once with ENALL */ + tcsr0 = (TCSR_SET & ~TCSR_ENT) | (tcsr0 & TCSR_UDT); + tcsr1 = TCSR_SET | TCSR_ENALL | (tcsr1 & TCSR_UDT); + writel(tcsr0, pwm->regs + TCSR0); + writel(tcsr1, pwm->regs + TCSR1); + } + } else { + writel(tcsr0 & ~TCSR_ENT, pwm->regs + TCSR0); + writel(tcsr1 & ~TCSR_ENT, pwm->regs + TCSR1); + } + + if (enabled && !state->enabled) + clk_rate_exclusive_put(pwm->clk); + + return 0; +} + +static void xilinx_pwm_get_state(struct pwm_chip *chip, + struct pwm_device *unused, + struct pwm_state *state) +{ + struct xilinx_pwm_device *pwm = xilinx_pwm_chip_to_device(chip); + u32 tlr0, tlr1, tcsr0, tcsr1; + + tlr0 = readl(pwm->regs + TLR0); + tlr1 = readl(pwm->regs + TLR1); + tcsr0 = readl(pwm->regs + TCSR0); + tcsr1 = readl(pwm->regs + TCSR1); + + state->period = xilinx_pwm_get_period(pwm, tlr0, tcsr0); + state->duty_cycle = xilinx_pwm_get_period(pwm, tlr1, tcsr1); + state->enabled = xilinx_pwm_is_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, +}; + +static struct dentry *debug_dir; + +#define DEBUG_REG(reg) { .name = #reg, .offset = (reg), } +static struct debugfs_reg32 xilinx_pwm_reg32[] = { + DEBUG_REG(TCSR0), + DEBUG_REG(TLR0), + DEBUG_REG(TCR0), + DEBUG_REG(TCSR1), + DEBUG_REG(TLR1), + DEBUG_REG(TCR1), +}; + +static int xilinx_pwm_debug_create(struct xilinx_pwm_device *pwm) +{ + struct debugfs_regset32 *regset; + + if (!IS_ENABLED(CONFIG_DEBUG_FS)) + return 0; + + regset = devm_kzalloc(pwm->chip.dev, sizeof(*regset), GFP_KERNEL); + if (!pwm) + return -ENOMEM; + + regset->regs = xilinx_pwm_reg32; + regset->nregs = ARRAY_SIZE(xilinx_pwm_reg32); + regset->base = pwm->regs; + + debugfs_create_regset32(dev_name(pwm->chip.dev), 0400, debug_dir, + regset); + return 0; +} + +static int xilinx_pwm_probe(struct platform_device *pdev) +{ + bool enabled; + int i, ret; + struct device *dev = &pdev->dev; + struct xilinx_pwm_device *pwm; + u32 one_timer; + + ret = of_property_read_u32(dev->of_node, "xlnx,one-timer-only", + &one_timer); + if (ret || one_timer) { + dev_err(dev, "two timers are needed for PWM mode\n"); + return -EINVAL; + } + + for (i = 0; i < 2; i++) { + char fmt[] = "xlnx,gen%u-assert"; + char buf[sizeof(fmt)]; + u32 gen; + + snprintf(buf, sizeof(buf), fmt, i); + ret = of_property_read_u32(dev->of_node, buf, &gen); + if (ret || !gen) { + dev_err(dev, "generateout%u must be active high\n", i); + return -EINVAL; + } + } + + pwm = devm_kzalloc(&pdev->dev, sizeof(*pwm), GFP_KERNEL); + if (!pwm) + return -ENOMEM; + platform_set_drvdata(pdev, pwm); + + pwm->chip.dev = &pdev->dev; + pwm->chip.ops = &xilinx_pwm_ops; + pwm->chip.base = -1; + pwm->chip.npwm = 1; + + pwm->regs = devm_platform_ioremap_resource(pdev, 0); + if (IS_ERR(pwm->regs)) + return PTR_ERR(pwm->regs); + + ret = of_property_read_u32(dev->of_node, "xlnx,count-width", &pwm->width); + if (ret) { + dev_err(dev, "missing counter width\n"); + return -EINVAL; + } + + pwm->clk = devm_clk_get(dev, NULL); + if (IS_ERR(pwm->clk)) + return dev_err_probe(dev, PTR_ERR(pwm->clk), "missing clock\n"); + + ret = clk_prepare_enable(pwm->clk); + if (ret) { + dev_err(dev, "clock enable failed\n"); + return ret; + } + + ret = xilinx_pwm_debug_create(pwm); + if (ret) { + clk_disable_unprepare(pwm->clk); + return ret; + } + + ret = pwmchip_add(&pwm->chip); + if (ret) { + clk_disable_unprepare(pwm->clk); + return ret; + } + + enabled = xilinx_pwm_is_enabled(readl(pwm->regs + TCSR0), + readl(pwm->regs + TCSR1)); + if (enabled) + clk_rate_exclusive_get(pwm->clk); + + return ret; +} + +static int xilinx_pwm_remove(struct platform_device *pdev) +{ + struct xilinx_pwm_device *pwm = platform_get_drvdata(pdev); + bool enabled = xilinx_pwm_is_enabled(readl(pwm->regs + TCSR0), + readl(pwm->regs + TCSR1)); + + if (enabled) + clk_rate_exclusive_put(pwm->clk); + clk_disable_unprepare(pwm->clk); + return pwmchip_remove(&pwm->chip); +} + +static const struct of_device_id xilinx_pwm_of_match[] = { + { .compatible = "xlnx,xps-timer-1.00.a" }, + { .compatible = "xlnx,axi-timer-2.0" }, + {}, +}; +MODULE_DEVICE_TABLE(of, xilinx_pwm_of_match); + +static struct platform_driver xilinx_pwm_driver = { + .probe = xilinx_pwm_probe, + .remove = xilinx_pwm_remove, + .driver = { + .name = "xilinx-pwm", + .of_match_table = of_match_ptr(xilinx_pwm_of_match), + }, +}; + +static int __init xilinx_pwm_init(void) +{ + int ret = platform_driver_register(&xilinx_pwm_driver); + + if (ret) + return ret; + + if (IS_ENABLED(CONFIG_DEBUG_FS)) { + debug_dir = debugfs_create_dir(xilinx_pwm_driver.driver.name, NULL); + if (IS_ERR(debug_dir)) { + platform_driver_unregister(&xilinx_pwm_driver); + return PTR_ERR(debug_dir); + } + } + return 0; +} +module_init(xilinx_pwm_init); + +static void __exit xilinx_pwm_exit(void) +{ + platform_driver_unregister(&xilinx_pwm_driver); + if (IS_ENABLED(CONFIG_DEBUG_FS)) + debugfs_remove_recursive(debug_dir); +} +module_exit(xilinx_pwm_exit); + +MODULE_ALIAS("platform:xilinx-pwm"); +MODULE_DESCRIPTION("Xilinx LogiCORE IP AXI Timer PWM driver"); +MODULE_LICENSE("GPL v2");
This adds PWM support for Xilinx LogiCORE IP AXI soft timers commonly found on Xilinx FPGAs. There is another driver for this device located at arch/microblaze/kernel/timer.c, but it is only used for timekeeping. 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> --- arch/arm64/configs/defconfig | 1 + drivers/pwm/Kconfig | 11 ++ drivers/pwm/Makefile | 1 + drivers/pwm/pwm-xilinx.c | 322 +++++++++++++++++++++++++++++++++++ 4 files changed, 335 insertions(+) create mode 100644 drivers/pwm/pwm-xilinx.c