Re: [PATCH v7 2/4] pwm: sun8i: Add H616 PWM support
James Hilliard <[email protected]>
| Newsgroups | org.kernel.vger.linux-pwm,dev.linux.lists.linux-sunxi,org.infradead.lists.linux-arm-kernel,org.kernel.vger.linux-clk,org.kernel.vger.linux-devicetree,org.kernel.vger.linux-kernel |
|---|---|
| Message-ID | <CADvTj4r=uBJyJdmK3T+jD_Nitg2NBs7fZgtE5p6wt9zwWgj6DQ@mail.gmail.com> |
On Tue, Aug 18, 2026 at 5:00 AM Uwe Kleine-König <[email protected]> wrote: > > Hello, > > On Fri, Jul 03, 2026 at 05:22:13PM +0200, Richard Genoud wrote: > > Add driver for Allwinner H616 PWM controller, supporting up to 6 > > channels. > > Those channels output can be either a PWM signal output or a clock > > output, thanks to the bypass. > > > > The channels are paired (0/1, 2/3 and 4/5) and each pair has a > > prescaler/mux/gate. > > Moreover, each channel has its own prescaler and bypass. > > > > The clock provider part of this driver is needed not only because the > > H616 PWM controller provides also clocks when bypass is enabled, but > > really because pwm-clock isn't fit to handle all cases here. > > pwm-clock would work if the 100MHz clock is requested, but if a lower > > clock is requested (like 24MHz), it will request a 42ns period to the > > PWM driver which will happily serve, with the 100MHz clock as input a > > 25MHz frequency and a duty cycle adjustable in the range [0-4]/4, > > because that is a sane thing to do for a PWM. > > The information missing is that a real clock is resquested, not a PWM. > > > > Tested-by: John Stultz <[email protected]> > > Tested-by: Joao Schim <[email protected]> > > Signed-off-by: Richard Genoud <[email protected]> > > --- > > drivers/pwm/Kconfig | 12 + > > drivers/pwm/Makefile | 1 + > > drivers/pwm/pwm-sun8i.c | 938 ++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 951 insertions(+) > > create mode 100644 drivers/pwm/pwm-sun8i.c > > > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > > index e8886a9b64d9..6e055a3a748c 100644 > > --- a/drivers/pwm/Kconfig > > +++ b/drivers/pwm/Kconfig > > @@ -748,6 +748,18 @@ config PWM_SUN4I > > To compile this driver as a module, choose M here: the module > > will be called pwm-sun4i. > > > > +config PWM_SUN8I > > + tristate "Allwinner sun8i/sun50i PWM support" > > + depends on ARCH_SUNXI || COMPILE_TEST > > + depends on HAS_IOMEM && COMMON_CLK > > + help > > + Generic PWM framework driver for Allwinner H616 SoCs. > > + It supports generic PWM, but can also provides a plain clock. > > + The AC300 PHY integrated in H616 SoC needs such a clock. > > + > > + To compile this driver as a module, choose M here: the module > > + will be called pwm-sun8i. > > + > > config PWM_SUNPLUS > > tristate "Sunplus PWM support" > > depends on ARCH_SUNPLUS || COMPILE_TEST > > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile > > index 5630a521a7cf..9c922b1fd0b4 100644 > > --- a/drivers/pwm/Makefile > > +++ b/drivers/pwm/Makefile > > @@ -68,6 +68,7 @@ obj-$(CONFIG_PWM_STM32) += pwm-stm32.o > > obj-$(CONFIG_PWM_STM32_LP) += pwm-stm32-lp.o > > obj-$(CONFIG_PWM_STMPE) += pwm-stmpe.o > > obj-$(CONFIG_PWM_SUN4I) += pwm-sun4i.o > > +obj-$(CONFIG_PWM_SUN8I) += pwm-sun8i.o > > obj-$(CONFIG_PWM_SUNPLUS) += pwm-sunplus.o > > obj-$(CONFIG_PWM_TEGRA) += pwm-tegra.o > > obj-$(CONFIG_PWM_TH1520) += pwm_th1520.o > > diff --git a/drivers/pwm/pwm-sun8i.c b/drivers/pwm/pwm-sun8i.c > > new file mode 100644 > > index 000000000000..8f1023e3a2e5 > > --- /dev/null > > +++ b/drivers/pwm/pwm-sun8i.c > > @@ -0,0 +1,938 @@ > > +// SPDX-License-Identifier: GPL-2.0-only > > +/* > > + * Driver for Allwinner sun8i Pulse Width Modulation Controller > > + * > > + * (C) Copyright 2025 Richard Genoud, Bootlin <[email protected]> > > + * > > + * Based on drivers/pwm/pwm-sun4i.c with Copyright: > > + * > > + * Copyright (C) 2014 Alexandre Belloni <[email protected]> > > + * > > + * Limitations: > > + * - As the channels are paired (0/1, 2/3, 4/5), they share the same clock > > + * source and prescaler(div_m), but they also have their own prescaler(div_k) > > + * and bypass. > > + * > > + */ > > + > > +#include <linux/bitfield.h> > > +#include <linux/bits.h> > > +#include <linux/clk.h> > > +#include <linux/clk-provider.h> > > +#include <linux/delay.h> > > +#include <linux/device.h> > > +#include <linux/err.h> > > +#include <linux/io.h> > > +#include <linux/limits.h> > > +#include <linux/math64.h> > > +#include <linux/module.h> > > +#include <linux/of.h> > > +#include <linux/platform_device.h> > > +#include <linux/pwm.h> > > +#include <linux/reset.h> > > +#include <linux/spinlock.h> > > +#include <linux/time.h> > > + > > +/* PWM IRQ Enable Register */ > > +#define SUN8I_PWM_PIER 0x0 > > + > > +/* PWM IRQ Status Register */ > > +#define SUN8I_PWM_PISR 0x4 > > + > > +/* PWM Capture IRQ Enable Register */ > > +#define SUN8I_PWM_CIER 0x10 > > + > > +/* PWM Capture IRQ Status Register */ > > +#define SUN8I_PWM_CISR 0x14 > > + > > +/* PWMCC Pairs Clock Configuration Registers */ > > +#define SUN8I_PWM_PCCR(pair) (0x20 + ((pair) * 0x4)) > > +#define SUN8I_PWM_PCCR_SRC_SHIFT 7 > > +#define SUN8I_PWM_PCCR_SRC_MASK 1 > > +#define SUN8I_PWM_PCCR_GATE_BIT 4 > > +#define SUN8I_PWM_PCCR_BYPASS_BIT(chan) ((chan) % 2 + 5) > > I'd prefer: > > #define SUN8I_PWM_PCCR_GATE BIT(4) > #define SUN8I_PWM_PCCR_BYPASS(chan) BIT((chan) % 2 + 5) > #define SUN8I_PWM_PCCR_SRC BIT(7) > > > +#define SUN8I_PWM_PCCR_DIV_M_SHIFT 0 > > + > > +/* PWMCC Pairs Dead Zone Control Registers */ > > +#define SUN8I_PWM_PDZCR(pair) (0x30 + ((pair) * 0x4)) > > + > > +/* PWM Enable Register */ > > +#define SUN8I_PWM_PER 0x40 > > +#define SUN8I_PWM_ENABLE(chan) BIT(chan) > > + > > +/* PWM Capture Enable Register */ > > +#define SUN8I_PWM_CER 0x44 > > + > > +/* PWM Control Register */ > > +#define SUN8I_PWM_PCR(chan) (0x60 + (chan) * 0x20) > > +#define SUN8I_PWM_PCR_PRESCAL_K_SHIFT 0 > > +#define SUN8I_PWM_PCR_PRESCAL_K_WIDTH 8 > > +#define SUN8I_PWM_PCR_ACTIVE_STATE BIT(8) > > + > > +/* PWM Period Register */ > > +#define SUN8I_PWM_PPR(chan) (0x64 + (chan) * 0x20) > > +#define SUN8I_PWM_PPR_PERIOD_MASK GENMASK(31, 16) > > +#define SUN8I_PWM_PPR_DUTY_MASK GENMASK(15, 0) > > +#define SUN8I_PWM_PPR_PERIOD_VALUE(reg) (FIELD_GET(SUN8I_PWM_PPR_PERIOD_MASK, reg) + 1) > > +#define SUN8I_PWM_PPR_DUTY_VALUE(reg) FIELD_GET(SUN8I_PWM_PPR_DUTY_MASK, reg) > > +#define SUN8I_PWM_PPR_PERIOD(prd) FIELD_PREP(SUN8I_PWM_PPR_PERIOD_MASK, (prd) - 1) > > +#define SUN8I_PWM_DUTY(dty) FIELD_PREP(SUN8I_PWM_PPR_DUTY_MASK, dty) > > +#define SUN8I_PWM_PPR_PERIOD_MAX (FIELD_MAX(SUN8I_PWM_PPR_PERIOD_MASK) + 1) > > + > > +/* PWM Count Register */ > > +#define SUN8I_PWM_PCNTR(chan) (0x68 + (chan) * 0x20) > > + > > +/* PWM Capture Control Register */ > > +#define SUN8I_PWM_CCR(chan) (0x6c + (chan) * 0x20) > > + > > +/* PWM Capture Rise Lock Register */ > > +#define SUN8I_PWM_CRLR(chan) (0x70 + (chan) * 0x20) > > + > > +/* PWM Capture Fall Lock Register */ > > +#define SUN8I_PWM_CFLR(chan) (0x74 + (chan) * 0x20) > > + > > +#define SUN8I_PWM_PAIR_IDX(chan) ((chan) >> 1) > > + > > +/* > > + * Block diagram of the PWM clock controller: > > + * > > + * _____ ______ ________ > > + * OSC24M --->| | | | | | > > + * APB1 ----->| Mux |--->| Gate |--->| /div_m |-----> SUN8I_PWM_clock_src_xy > > + * |_____| |______| |________| > > + * ________ > > + * | | > > + * +->| /div_k |---> SUN8I_PWM_clock_x > > + * | |________| > > + * | ______ > > + * | | | > > + * +-->| Gate |----> SUN8I_PWM_bypass_clock_x > > + * | |______| > > + * SUN8I_PWM_clock_src_xy ---+ ________ > > + * | | | > > + * +->| /div_k |---> SUN8I_PWM_clock_y > > + * | |________| > > + * | ______ > > + * | | | > > + * +-->| Gate |----> SUN8I_PWM_bypass_clock_y > > + * |______| > > + * > > + * NB: when the bypass is set, all the PWM logic is bypassed. > > + * So, the duty cycle and polarity can't be modified (we just have a clock). > > + * The bypass in PWM mode is used to achieve a 1/2 relative duty cycle with the > > + * fastest clock. > > + * > > + * SUN8I_PWM_clock_x/y serve for the PWM purpose. > > + * SUN8I_PWM_bypass_clock_x/y serve for the clock-provider purpose. > > + * > > + */ > > + > > +/* > > + * Table used for /div_m (diviser before obtaining SUN8I_PWM_clock_src_xy) > > + * It's actually CLK_DIVIDER_POWER_OF_TWO, but limited to /256 > > + */ > > +#define CLK_TABLE_DIV_M_ENTRY(i) { \ > > + .val = (i), .div = 1 << (i) \ > > +} > > + > > +static const struct clk_div_table clk_table_div_m[] = { > > + CLK_TABLE_DIV_M_ENTRY(0), > > + CLK_TABLE_DIV_M_ENTRY(1), > > + CLK_TABLE_DIV_M_ENTRY(2), > > + CLK_TABLE_DIV_M_ENTRY(3), > > + CLK_TABLE_DIV_M_ENTRY(4), > > + CLK_TABLE_DIV_M_ENTRY(5), > > + CLK_TABLE_DIV_M_ENTRY(6), > > + CLK_TABLE_DIV_M_ENTRY(7), > > + CLK_TABLE_DIV_M_ENTRY(8), > > + { /* sentinel */ } > > +}; > > + > > +#define SUN8I_PWM_XY_SRC_GATE(_pair, _reg) \ > > +struct clk_gate gate_xy_##_pair = { \ > > Could these be const? > > > + .reg = (void *)(_reg), \ > > + .bit_idx = SUN8I_PWM_PCCR_GATE_BIT, \ > > + .hw.init = &(struct clk_init_data){ \ > > + .ops = &clk_gate_ops, \ > > + } \ > > +} > > + > > +#define SUN8I_PWM_XY_SRC_MUX(_pair, _reg) \ > > +struct clk_mux mux_xy_##_pair = { \ > > + .reg = (void *)(_reg), \ > > + .shift = SUN8I_PWM_PCCR_SRC_SHIFT, \ > > + .mask = SUN8I_PWM_PCCR_SRC_MASK, \ > > + .flags = CLK_MUX_ROUND_CLOSEST, \ > > + .hw.init = &(struct clk_init_data){ \ > > + .ops = &clk_mux_ops, \ > > + } \ > > +} > > + > > +#define SUN8I_PWM_XY_SRC_DIV(_pair, _reg) \ > > +struct clk_divider rate_xy_##_pair = { \ > > + .reg = (void *)(_reg), \ > > + .shift = SUN8I_PWM_PCCR_DIV_M_SHIFT, \ > > + .table = clk_table_div_m, \ > > + .hw.init = &(struct clk_init_data){ \ > > + .ops = &clk_divider_ops, \ > > + } \ > > +} > > + > > +#define SUN8I_PWM_X_DIV(_idx, _reg) \ > > +struct clk_divider rate_x_##_idx = { \ > > + .reg = (void *)(_reg), \ > > + .shift = SUN8I_PWM_PCR_PRESCAL_K_SHIFT, \ > > + .width = SUN8I_PWM_PCR_PRESCAL_K_WIDTH, \ > > + .hw.init = &(struct clk_init_data){ \ > > + .ops = &clk_divider_ops, \ > > + } \ > > +} > > + > > +#define SUN8I_PWM_X_BYPASS_GATE(_idx) \ > > +struct clk_gate gate_x_bypass_##_idx = { \ > > + .reg = (void *)SUN8I_PWM_PER, \ > > + .bit_idx = _idx, \ > > + .hw.init = &(struct clk_init_data){ \ > > + .ops = &clk_gate_ops, \ > > + } \ > > +} > > + > > +#define SUN8I_PWM_XY_CLK_SRC(_pair, _reg) \ > > + static SUN8I_PWM_XY_SRC_MUX(_pair, _reg); \ > > + static SUN8I_PWM_XY_SRC_GATE(_pair, _reg); \ > > + static SUN8I_PWM_XY_SRC_DIV(_pair, _reg) > > + > > +#define SUN8I_PWM_X_CLK(_idx) \ > > + static SUN8I_PWM_X_DIV(_idx, SUN8I_PWM_PCR(_idx)) > > + > > +#define SUN8I_PWM_X_BYPASS_CLK(_idx) \ > > + SUN8I_PWM_X_BYPASS_GATE(_idx) > > + > > +#define REF_CLK_XY_SRC(_pair) \ > > + { \ > > + .name = "pwm-clk-src" #_pair, \ > > + .mux_hw = &mux_xy_##_pair.hw, \ > > + .gate_hw = &gate_xy_##_pair.hw, \ > > + .rate_hw = &rate_xy_##_pair.hw, \ > > No hard requirement, but I'd like spaces around ## > > > + } > > + > > +#define REF_CLK_X(_idx, _pair) \ > > + { \ > > + .name = "pwm-clk" #_idx, \ > > + .parent_names = (const char *[]){ "pwm-clk-src" #_pair }, \ > > + .num_parents = 1, \ > > + .rate_hw = &rate_x_##_idx.hw, \ > > + .flags = CLK_SET_RATE_PARENT, \ > > + } > > + > > +#define REF_CLK_BYPASS(_idx, _pair) \ > > + { \ > > + .name = "pwm-clk-bypass" #_idx, \ > > + .parent_names = (const char *[]){ "pwm-clk-src" #_pair }, \ > > + .num_parents = 1, \ > > + .gate_hw = &gate_x_bypass_##_idx.hw, \ > > + .flags = CLK_SET_RATE_PARENT, \ > > + } > > + > > +/* > > + * SUN8I_PWM_clock_src_xy generation: > > + * _____ ______ ________ > > + * OSC24M --->| | | | | | > > + * APB1 ----->| Mux |--->| Gate |--->| /div_m |-----> SUN8I_PWM_clock_src_xy > > + * |_____| |______| |________| > > + */ > > +SUN8I_PWM_XY_CLK_SRC(01, SUN8I_PWM_PCCR(0)); > > +SUN8I_PWM_XY_CLK_SRC(23, SUN8I_PWM_PCCR(1)); > > +SUN8I_PWM_XY_CLK_SRC(45, SUN8I_PWM_PCCR(2)); > > + > > +/* > > + * SUN8I_PWM_clock_x_div generation: > > + * ________ > > + * | | SUN8I_PWM_clock_x/y > > + * SUN8I_PWM_clock_src_xy -->| /div_k |---------------> > > + * |________| > > + */ > > +SUN8I_PWM_X_CLK(0); > > +SUN8I_PWM_X_CLK(1); > > +SUN8I_PWM_X_CLK(2); > > +SUN8I_PWM_X_CLK(3); > > +SUN8I_PWM_X_CLK(4); > > +SUN8I_PWM_X_CLK(5); > > + > > +/* > > + * SUN8I_PWM_bypass_clock_xy generation: > > + * ______ > > + * | | > > + * SUN8I_PWM_clock_src_xy --->| Gate |-------> SUN8I_PWM_bypass_clock_x > > + * |______| > > + * > > + * The gate is actually SUN8I_PWM_PER register. > > + */ > > +SUN8I_PWM_X_BYPASS_CLK(0); > > +SUN8I_PWM_X_BYPASS_CLK(1); > > +SUN8I_PWM_X_BYPASS_CLK(2); > > +SUN8I_PWM_X_BYPASS_CLK(3); > > +SUN8I_PWM_X_BYPASS_CLK(4); > > +SUN8I_PWM_X_BYPASS_CLK(5); > > + > > +struct clk_pwm_data { > > + const char *name; > > + const char **parent_names; > > + unsigned int num_parents; > > + struct clk_hw *mux_hw; > > + struct clk_hw *rate_hw; > > + struct clk_hw *gate_hw; > > + unsigned long flags; > > +}; > > + > > +/* Indexes of REF_CLK_BYPASS and REF_CLK_XY_SRC in the array */ > > +#define CLK_BYPASS_IDX(sun8i_chip, chan) ((sun8i_chip)->data->npwm + (chan)) > > +#define CLK_XY_SRC_IDX(sun8i_chip, chan) \ > > + ((sun8i_chip)->data->npwm * 2 + SUN8I_PWM_PAIR_IDX(chan)) > > +static struct clk_pwm_data pwmcc_data[] = { > > + REF_CLK_X(0, 01), > > + REF_CLK_X(1, 01), > > + REF_CLK_X(2, 23), > > + REF_CLK_X(3, 23), > > + REF_CLK_X(4, 45), > > + REF_CLK_X(5, 45), > > + REF_CLK_BYPASS(0, 01), > > + REF_CLK_BYPASS(1, 01), > > + REF_CLK_BYPASS(2, 23), > > + REF_CLK_BYPASS(3, 23), > > + REF_CLK_BYPASS(4, 45), > > + REF_CLK_BYPASS(5, 45), > > + REF_CLK_XY_SRC(01), > > + REF_CLK_XY_SRC(23), > > + REF_CLK_XY_SRC(45), > > + { /* sentinel */ } > > +}; > > For the massive clk-specific part it would be great to get feedback from > someone knowledgeable about clk. > > > +enum sun8i_pwm_mode { > > + SUN8I_PWM_MODE_NONE, > > + SUN8I_PWM_MODE_PWM, > > + SUN8I_PWM_MODE_CLK, > > +}; > > + > > +struct sun8i_pwm_data { > > + unsigned int npwm; > > +}; > > + > > +struct sun8i_pwm_channel { > > + struct clk *pwm_clk; > > + enum sun8i_pwm_mode mode; > > +}; > > + > > +struct clk_pwm_pdata { > > + struct clk_hw_onecell_data *hw_data; > > + spinlock_t lock; > > + void __iomem *reg; > > +}; > > + > > +struct sun8i_pwm_chip { > > + struct clk_pwm_pdata *clk_pdata; > > + struct sun8i_pwm_channel *channels; > > + struct clk *bus_clk; > > + struct reset_control *rst; > > + void __iomem *base; > > + const struct sun8i_pwm_data *data; > > +}; > > + > > +struct sun8i_pwm_waveform { > > + u8 enabled:1; > > + u8 active_state:1; > > + u8 bypass_en:1; > > + u16 duty_ticks; > > + u32 period_ticks; > > + unsigned long clk_rate; > > +}; > > + > > +static inline struct sun8i_pwm_chip *sun8i_pwm_from_chip(const struct pwm_chip *chip) > > +{ > > + return pwmchip_get_drvdata(chip); > > +} > > + > > +static inline u32 sun8i_pwm_readl(struct sun8i_pwm_chip *sun8i_chip, > > + unsigned long offset) > > +{ > > + return readl(sun8i_chip->base + offset); > > +} > > + > > +static inline void sun8i_pwm_writel(struct sun8i_pwm_chip *sun8i_chip, > > + u32 val, unsigned long offset) > > +{ > > + writel(val, sun8i_chip->base + offset); > > +} > > + > > +static void sun8i_pwm_set_bypass(struct sun8i_pwm_chip *sun8i_chip, > > + unsigned int idx, bool en_bypass) > > +{ > > + unsigned long flags, reg_offset; > > + u32 val; > > + > > + spin_lock_irqsave(&sun8i_chip->clk_pdata->lock, flags); > > + > > + reg_offset = SUN8I_PWM_PCCR(SUN8I_PWM_PAIR_IDX(idx)); > > + val = sun8i_pwm_readl(sun8i_chip, reg_offset); > > + if (en_bypass) > > + val |= BIT(SUN8I_PWM_PCCR_BYPASS_BIT(idx)); > > + else > > + val &= ~BIT(SUN8I_PWM_PCCR_BYPASS_BIT(idx)); > > + > > + sun8i_pwm_writel(sun8i_chip, val, reg_offset); > > + > > + spin_unlock_irqrestore(&sun8i_chip->clk_pdata->lock, flags); > > Use guard() here as you did in other functions? > > > +} > > + > > +static int sun8i_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm) > > +{ > > + struct sun8i_pwm_chip *sun8i_chip = sun8i_pwm_from_chip(chip); > > + struct sun8i_pwm_channel *chan = &sun8i_chip->channels[pwm->hwpwm]; > > + > > + scoped_guard(spinlock_irqsave, &sun8i_chip->clk_pdata->lock) { > > + if (chan->mode == SUN8I_PWM_MODE_CLK) > > + return -EBUSY; > > + chan->mode = SUN8I_PWM_MODE_PWM; > > + } > > + > > + return clk_prepare_enable(chan->pwm_clk); > > +} > > + > > +static void sun8i_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm) > > +{ > > + struct sun8i_pwm_chip *sun8i_chip = sun8i_pwm_from_chip(chip); > > + struct sun8i_pwm_channel *chan = &sun8i_chip->channels[pwm->hwpwm]; > > + > > + clk_disable_unprepare(chan->pwm_clk); > > + chan->mode = SUN8I_PWM_MODE_NONE; > > +} > > + > > +static int sun8i_pwm_read_waveform(struct pwm_chip *chip, > > + struct pwm_device *pwm, > > + void *_wfhw) > > +{ > > + struct sun8i_pwm_waveform *wfhw = _wfhw; > > + struct sun8i_pwm_chip *sun8i_chip = sun8i_pwm_from_chip(chip); > > + struct sun8i_pwm_channel *chan = &sun8i_chip->channels[pwm->hwpwm]; > > + u32 val; > > + > > + wfhw->clk_rate = clk_get_rate(chan->pwm_clk); > > clk_get_rate() is a sleeping call. Unless I miss something this is the > only sleeping call and if you query the clk rate in .request() after > calling clk_rate_exclusive_get() (which you should do anyhow to prevent > changes to the running PWM) you can mark the chip atomic. > > > + val = sun8i_pwm_readl(sun8i_chip, SUN8I_PWM_PER); > > + wfhw->enabled = !!(SUN8I_PWM_ENABLE(pwm->hwpwm) & val); > > + > > + val = sun8i_pwm_readl(sun8i_chip, SUN8I_PWM_PCCR(SUN8I_PWM_PAIR_IDX(pwm->hwpwm))); > > + wfhw->bypass_en = !!(val & BIT(SUN8I_PWM_PCCR_BYPASS_BIT(pwm->hwpwm))); > > + > > + val = sun8i_pwm_readl(sun8i_chip, SUN8I_PWM_PCR(pwm->hwpwm)); > > + wfhw->active_state = !!(val & SUN8I_PWM_PCR_ACTIVE_STATE); > > + > > + val = sun8i_pwm_readl(sun8i_chip, SUN8I_PWM_PPR(pwm->hwpwm)); > > + wfhw->duty_ticks = SUN8I_PWM_PPR_DUTY_VALUE(val); > > + wfhw->period_ticks = SUN8I_PWM_PPR_PERIOD_VALUE(val); > > + > > + dev_dbg(pwmchip_parent(chip), > > + "pwm%u: %s, bypass: %s, polarity: %s, clk_rate=%lu period_ticks=%u duty_ticks=%u\n", > > + pwm->hwpwm, > > + wfhw->enabled ? "enabled" : "disabled", > > + wfhw->bypass_en ? "enabled" : "disabled", > > + wfhw->active_state ? "normal" : "inversed", > > + wfhw->clk_rate, wfhw->period_ticks, wfhw->duty_ticks); > > + > > + return 0; > > +} > > + > > +static int sun8i_pwm_round_waveform_fromhw(struct pwm_chip *chip, > > + struct pwm_device *pwm, > > + const void *_wfhw, > > + struct pwm_waveform *wf) > > +{ > > + const struct sun8i_pwm_waveform *wfhw = _wfhw; > > + u64 tmp, resolution; > > + > > + dev_dbg(pwmchip_parent(chip), > > + "pwm%u: %s, bypass: %s, polarity: %s, clk_rate=%lu period_ticks=%u duty_ticks=%u\n", > > + pwm->hwpwm, > > + wfhw->enabled ? "enabled" : "disabled", > > + wfhw->bypass_en ? "enabled" : "disabled", > > + wfhw->active_state ? "normal" : "inversed", > > + wfhw->clk_rate, wfhw->period_ticks, wfhw->duty_ticks); > > + > > + wf->duty_offset_ns = 0; > > + > > + if (!wfhw->enabled || !wfhw->clk_rate) { > > + wf->period_length_ns = 0; > > + wf->duty_length_ns = 0; > > + return 0; > > + } > > + > > + if (wfhw->bypass_en) { > > + wf->period_length_ns = DIV_ROUND_UP_ULL(NSEC_PER_SEC, > > + wfhw->clk_rate); > > + wf->duty_length_ns = DIV_ROUND_UP_ULL(wf->period_length_ns, 2); > > It would be great to have a dev_dbg covering this branch, too. > > > + return 0; > > + } > > + > > + tmp = NSEC_PER_SEC * (u64)wfhw->period_ticks; > > + wf->period_length_ns = DIV_ROUND_UP_ULL(tmp, wfhw->clk_rate); > > + > > + tmp = NSEC_PER_SEC * (u64)wfhw->duty_ticks; > > + wf->duty_length_ns = DIV_ROUND_UP_ULL(tmp, wfhw->clk_rate); > > + if (!wfhw->active_state) { > > + /* > > + * For inverted polarity, we have to fix cases where > > + * computed duty_length_ns > requested duty_length_ns > > + * For that, we subtract the actual resolution of the PWM > > + * registers > > + */ > > + wf->duty_offset_ns = wf->duty_length_ns; > > + wf->duty_length_ns = wf->period_length_ns - wf->duty_length_ns; > > + > > + resolution = DIV_ROUND_UP_ULL(NSEC_PER_SEC, wfhw->clk_rate); > > + > > + if (wf->duty_offset_ns >= resolution) > > + wf->duty_offset_ns -= resolution; > > This is broken. Consider clk_rate = 1234567, period_ticks = 8000 and > duty_ticks = 7000 Then you calculate: > > period_length_ns = 6480005 > duty_length_ns = 6480005 - 5670005 = 810000 > > but the actual duty_length is 810000.5913004316 and thus 810001 should > be reported. Look at how the stm32 driver does it, that applies here, > too. > > > + } > > + > > + dev_dbg(pwmchip_parent(chip), > > + "pwm%u period_length_ns=%llu duty_length_ns=%llu duty_offset_ns=%llu\n", > > + pwm->hwpwm, wf->period_length_ns, wf->duty_length_ns, > > + wf->duty_offset_ns); > > A single dev_dbg() mentioning both the hardware specfic parts and *wf > please. Also use > > "... %lld/%lld [+%lld] ...", ..., > wf->duty_length_ns, wf->period_length_ns, wf->duty_offset_ns, > > to emit *wf. > > > + > > + return 0; > > +} > > + > > +static int sun8i_pwm_write_waveform(struct pwm_chip *chip, > > + struct pwm_device *pwm, const void *_wfhw) > > +{ > > + const struct sun8i_pwm_waveform *wfhw = _wfhw; > > + struct sun8i_pwm_chip *sun8i_chip = sun8i_pwm_from_chip(chip); > > + struct sun8i_pwm_channel *chan = &sun8i_chip->channels[pwm->hwpwm]; > > + unsigned long flags; > > + u32 val; > > + int ret; > > + > > + ret = clk_set_rate(chan->pwm_clk, wfhw->clk_rate); > > + if (ret) > > + return ret; > > You must check that this resulted indeed in setting the rate to exactly > wfhw->clk_rate. > > Also IIUC this might affect channel pwm->hwpwm ^ 1, so there needs to be > some guard to not modify that if it's running. > > > + sun8i_pwm_set_bypass(sun8i_chip, pwm->hwpwm, wfhw->bypass_en); > > + > > + val = sun8i_pwm_readl(sun8i_chip, SUN8I_PWM_PCR(pwm->hwpwm)); > > + if (wfhw->active_state) > > + val |= SUN8I_PWM_PCR_ACTIVE_STATE; > > + else > > + val &= ~SUN8I_PWM_PCR_ACTIVE_STATE; > > + sun8i_pwm_writel(sun8i_chip, val, SUN8I_PWM_PCR(pwm->hwpwm)); > > + > > + val = SUN8I_PWM_DUTY(wfhw->duty_ticks); > > + val |= SUN8I_PWM_PPR_PERIOD(wfhw->period_ticks); > > + sun8i_pwm_writel(sun8i_chip, val, SUN8I_PWM_PPR(pwm->hwpwm)); > > + > > + spin_lock_irqsave(&sun8i_chip->clk_pdata->lock, flags); > > + > > + val = sun8i_pwm_readl(sun8i_chip, SUN8I_PWM_PER); > > + if (wfhw->enabled) > > + val |= SUN8I_PWM_ENABLE(pwm->hwpwm); > > + else > > + val &= ~SUN8I_PWM_ENABLE(pwm->hwpwm); > > + sun8i_pwm_writel(sun8i_chip, val, SUN8I_PWM_PER); > > + > > + spin_unlock_irqrestore(&sun8i_chip->clk_pdata->lock, flags); > > + > > + return 0; > > +} > > + > > +static int sun8i_pwm_round_waveform_tohw(struct pwm_chip *chip, > > + struct pwm_device *pwm, > > + const struct pwm_waveform *wf, > > + void *_wfhw) > > +{ > > + struct sun8i_pwm_chip *sun8i_chip = sun8i_pwm_from_chip(chip); > > + struct sun8i_pwm_channel *chan = &sun8i_chip->channels[pwm->hwpwm]; > > + struct sun8i_pwm_waveform *wfhw = _wfhw; > > + unsigned long max_rate; > > + long calc_rate; > > + u64 period_ratio, double_duty_ratio, freq, duty_cycle; > > + > > + dev_dbg(pwmchip_parent(chip), > > + "pwm%u period_length_ns=%llu duty_length_ns=%llu duty_offset_ns=%llu\n", > > + pwm->hwpwm, wf->period_length_ns, wf->duty_length_ns, > > + wf->duty_offset_ns); > > + > > + if (wf->period_length_ns == 0) { > > + wfhw->enabled = 0; > > + return 0; > > + } > > + > > + wfhw->enabled = 1; > > + > > + duty_cycle = wf->duty_length_ns; > > + if (wf->duty_length_ns + wf->duty_offset_ns < wf->period_length_ns) > > + wfhw->active_state = 1; > > + else > > + wfhw->active_state = 0; > > That's wrong. The decision about polarity must be done in the tick > domain. See commit 5d087c485b6ecf200a9ebb2a032bf8571d330250 for a > similar issue in the stm32 driver. > > > + dev_dbg(pwmchip_parent(chip), "polarity: %s\n", > > + wfhw->active_state ? "normal" : "inversed"); > > + > > + /* > > + * Lowest possible period case: > > + * Without bypass, the lowest possible period is when: > > + * duty cycle = 1 and period cycle = 2 (0x10001 in period register) > > period cycle = 1 doesn't work? > > > + * E.g. if the input clock is 100MHz, we have a lowest period of 20ns. > > + * Now, with the bypass, the period register is ignored and we directly > > + * have the 100MHz clock as PWM output, that can act as a 10ns period > > + * with 5ns duty. > > + * So, to detect this lowest period case, just get the maximum possible > > + * rate from chan->pwm_clk and compare it with requested period and > > + * duty_cycle. > > + * > > + * But, to get the maximum possible rate, we have to use U32_MAX instead > > + * of (unsigned long)-1. > > + * This is because clk_round_rate() uses ultimately DIV_ROUND_UP_ULL() > > + * that in turn do_div(n,base). And base is uint32_t divisor. > > Sounds like a bug in the clk subsystem. > > > + */ > > + max_rate = clk_round_rate(chan->pwm_clk, U32_MAX); > > + > > + dev_dbg(pwmchip_parent(chip), "max_rate: %ld Hz\n", max_rate); > > + > > + period_ratio = mul_u64_u64_div_u64(wf->period_length_ns, > > + max_rate, NSEC_PER_SEC); > > This might overflow for max_rate > 1 GHz. > > > + double_duty_ratio = mul_u64_u64_div_u64(duty_cycle, (u64)max_rate * 2, > > + NSEC_PER_SEC); > > + if (period_ratio == 1) { > > + if (double_duty_ratio == 0) > > + /* requested period and duty are too small */ > > + return -EINVAL; > > period_ratio == 0 is ok? Also I fail to see the relevance of > double_duty_ratio, so this likely needs a comment. > > Also you're supposed to pick the smallest period if the requested period > is too small. > > > + /* > > + * If the requested period is to small to be generated by the > > + * PWM, but matches the highest clock with a > > + * duty_cycle >= period*2, just bypass the PWM logic > > + */ > > + freq = div64_u64(NSEC_PER_SEC, wf->period_length_ns); > > + wfhw->bypass_en = true; > > + } else { > > + wfhw->bypass_en = false; > > + freq = div64_u64(NSEC_PER_SEC * (u64)SUN8I_PWM_PPR_PERIOD_MAX, > > + wf->period_length_ns); > > + /* > > + * Same remark as above, this is to prevent a value to big for > > + * clk_round_rate() to handle > > + */ > > + if (freq > U32_MAX) > > + freq = U32_MAX; > > + } > > + > > + dev_dbg(pwmchip_parent(chip), "bypass: %s\n", > > + wfhw->bypass_en ? "enabled" : "disabled"); > > + > > + calc_rate = clk_round_rate(chan->pwm_clk, freq); > > It's a pity that there are no guarantees in the clk subsystem about how > clk_round_rate() works. If you ask for say freq=1000000 and the clock > can only do 950000 or 1050000, it's unspecified which of these > clk_round_rate() returns. And you might want to check the one that > wasn't returned as it might give better approximations. (And note it > depends on the purpose which of these is better. freq might be an upper > bound or a lower bound, and depending on the usecase you want to > minimise abs(freq - calc_rate) or abs(1 - freq/calc_rate) (or abs(1 - > calc_rate/freq)?), so a single function with the prototype of > clk_round_rate() cannot cover all use cases.) > > In general it's really hard to choose the optimal parent rate, even if > the clk framework gave you an easy way to explore the possibilities. > So it's unclear that NSEC_PER_SEC * SUN8I_PWM_PPR_PERIOD_MAX / > wf->period_length_ns is a good canditate at all. > > So I think it's impossible to get the tohw callback correct without > further guarantees by the clk framework (or implicit knowledge about > chan->pwm_clk) and even then it's hard. > > > + if (calc_rate <= 0) > > + return calc_rate ? calc_rate : -EINVAL; > > calc_rate ?: -EINVAL; > > > + > > + dev_dbg(pwmchip_parent(chip), "calc_rate: %ld Hz\n", calc_rate); > > + > > + wfhw->period_ticks = mul_u64_u64_div_u64(calc_rate, > > + wf->period_length_ns, > > + NSEC_PER_SEC); > > + if (wfhw->period_ticks > SUN8I_PWM_PPR_PERIOD_MAX) > > + wfhw->period_ticks = SUN8I_PWM_PPR_PERIOD_MAX; > > + > > + /* min value in period register is 1 */ > > + if (wfhw->period_ticks == 0) > > + return -EINVAL; > > As above, configure the smallest possible period in this case. > > > + wfhw->duty_ticks = mul_u64_u64_div_u64(calc_rate, duty_cycle, > > + NSEC_PER_SEC); > > + > > + if (wfhw->duty_ticks > wfhw->period_ticks) > > + wfhw->duty_ticks = wfhw->period_ticks; > > + > > + if (!wfhw->active_state) > > + wfhw->duty_ticks = wfhw->period_ticks - wfhw->duty_ticks; > > + > > + dev_dbg(pwmchip_parent(chip), > > + "pwm%u period_ticks=%u duty_cycle=%llu duty_ticks=%u\n", > > + pwm->hwpwm, wfhw->period_ticks, duty_cycle, wfhw->duty_ticks); > > + > > + wfhw->clk_rate = calc_rate; > > + > > + return 0; > > +} > > + > > +static const struct pwm_ops sun8i_pwm_ops = { > > + .request = sun8i_pwm_request, > > + .free = sun8i_pwm_free, > > + .sizeof_wfhw = sizeof(struct sun8i_pwm_waveform), > > + .round_waveform_tohw = sun8i_pwm_round_waveform_tohw, > > + .round_waveform_fromhw = sun8i_pwm_round_waveform_fromhw, > > + .read_waveform = sun8i_pwm_read_waveform, > > + .write_waveform = sun8i_pwm_write_waveform, > > +}; > > + > > +static struct clk_hw *sun8i_pwm_of_clk_get(struct of_phandle_args *clkspec, > > + void *data) > > +{ > > + struct sun8i_pwm_chip *sun8i_chip = data; > > + struct clk_hw_onecell_data *hw_data = sun8i_chip->clk_pdata->hw_data; > > + unsigned int idx = clkspec->args[0]; > > + struct sun8i_pwm_channel *chan; > > + struct clk_hw *ret_clk = NULL; > > + unsigned long flags; > > + > > + if (idx >= sun8i_chip->data->npwm) > > + return ERR_PTR(-EINVAL); > > + > > + chan = &sun8i_chip->channels[idx]; > > + > > + spin_lock_irqsave(&sun8i_chip->clk_pdata->lock, flags); > > + > > + if (chan->mode == SUN8I_PWM_MODE_PWM) { > > + ret_clk = ERR_PTR(-EBUSY); > > + } else { > > + chan->mode = SUN8I_PWM_MODE_CLK; > > + ret_clk = hw_data->hws[CLK_BYPASS_IDX(sun8i_chip, idx)]; > > + } > > + spin_unlock_irqrestore(&sun8i_chip->clk_pdata->lock, flags); > > + > > + if (IS_ERR(ret_clk)) > > + goto out; > > + > > + sun8i_pwm_set_bypass(sun8i_chip, idx, true); > > +out: > > If you switch to guard you can drop the goto. > > > + return ret_clk; > > +} > > + > > +static int sun8i_add_composite_clk(struct clk_pwm_data *data, > > + void __iomem *reg, spinlock_t *lock, > > + struct device *dev, struct clk_hw **hw) > > +{ > > + const struct clk_ops *mux_ops = NULL, *gate_ops = NULL, *rate_ops = NULL; > > + struct clk_hw *mux_hw = NULL, *gate_hw = NULL, *rate_hw = NULL; > > + struct device_node *node = dev->of_node; > > + > > + if (data->mux_hw) { > > + struct clk_mux *mux; > > + > > + mux_hw = data->mux_hw; > > + mux = to_clk_mux(mux_hw); > > + mux->lock = lock; > > + mux_ops = mux_hw->init->ops; > > + mux->reg = (uintptr_t)mux->reg + reg; > > + } > > + > > + if (data->gate_hw) { > > + struct clk_gate *gate; > > + > > + gate_hw = data->gate_hw; > > + gate = to_clk_gate(gate_hw); > > + gate->lock = lock; > > + gate_ops = gate_hw->init->ops; > > + gate->reg = (uintptr_t)gate->reg + reg; > > + } > > + > > + if (data->rate_hw) { > > + struct clk_divider *rate; > > + > > + rate_hw = data->rate_hw; > > + rate = to_clk_divider(rate_hw); > > + rate_ops = rate_hw->init->ops; > > + rate->lock = lock; > > + rate->reg = (uintptr_t)rate->reg + reg; > > + > > + if (rate->table) { > > + const struct clk_div_table *clkt; > > + int table_size = 0; > > + > > + for (clkt = rate->table; clkt->div; clkt++) > > + table_size++; > > + rate->width = order_base_2(table_size); > > + } > > + } > > + > > + /* > > + * Retrieve the parent clock names from DTS for pwm-clk-srcxy > > + */ > > + if (!data->parent_names) { > > + data->num_parents = of_clk_get_parent_count(node); > > + if (data->num_parents == 0) > > + return -ENOENT; > > + > > + data->parent_names = devm_kzalloc(dev, > > + sizeof(*data->parent_names), > > + GFP_KERNEL); > > + for (unsigned int i = 0; i < data->num_parents; i++) > > + data->parent_names[i] = of_clk_get_parent_name(node, i); > > + } > > + > > + *hw = clk_hw_register_composite(dev, data->name, data->parent_names, > > + data->num_parents, mux_hw, > > + mux_ops, rate_hw, rate_ops, > > + gate_hw, gate_ops, data->flags); > > + > > + return PTR_ERR_OR_ZERO(*hw); > > +} > > + > > +static int sun8i_pwm_init_clocks(struct platform_device *pdev, > > + struct sun8i_pwm_chip *sun8i_chip) > > +{ > > + struct clk_pwm_pdata *pdata; > > + struct device *dev = &pdev->dev; > > + int num_clocks = 0; > > + int ret; > > + > > + pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL); > > + if (!pdata) > > + return dev_err_probe(dev, -ENOMEM, > > + "Failed to allocate clk_pwm_pdata\n"); > > + > > + while (pwmcc_data[num_clocks].name) > > + num_clocks++; > > + > > + pdata->hw_data = devm_kzalloc(dev, struct_size(pdata->hw_data, hws, num_clocks), > > + GFP_KERNEL); > > + if (!pdata->hw_data) > > + return dev_err_probe(dev, -ENOMEM, > > + "Failed to allocate hw clocks\n"); > > + > > + pdata->hw_data->num = num_clocks; > > + pdata->reg = sun8i_chip->base; > > + > > + spin_lock_init(&pdata->lock); > > + > > + for (int i = 0; i < num_clocks; i++) { > > + struct clk_hw **hw = &pdata->hw_data->hws[i]; > > + > > + ret = sun8i_add_composite_clk(&pwmcc_data[i], pdata->reg, > > + &pdata->lock, dev, hw); > > + if (ret) { > > + dev_err_probe(dev, ret, > > + "Failed to register hw clock %s\n", > > + pwmcc_data[i].name); > > + for (i--; i >= 0; i--) > > + clk_hw_unregister_composite(pdata->hw_data->hws[i]); > > + return ret; > > + } > > + } > > + > > + sun8i_chip->clk_pdata = pdata; > > + > > + return 0; > > +} > > + > > +static void sun8i_pwm_unregister_clk(void *data) > > +{ > > + struct clk_hw_onecell_data *hw_data = data; > > + > > + for (unsigned int i = 0; i < hw_data->num; i++) > > + clk_hw_unregister_composite(hw_data->hws[i]); > > +} > > + > > +static int sun8i_pwm_probe(struct platform_device *pdev) > > +{ > > + const struct sun8i_pwm_data *data; > > + struct device *dev = &pdev->dev; > > + struct sun8i_pwm_chip *sun8i_chip; > > + struct pwm_chip *chip; > > + int ret; > > + > > + data = of_device_get_match_data(dev); > > + if (!data) > > + return dev_err_probe(dev, -ENODEV, > > + "Missing specific data structure\n"); > > + > > + chip = devm_pwmchip_alloc(dev, data->npwm, sizeof(*sun8i_chip)); > > + if (IS_ERR(chip)) > > + return dev_err_probe(dev, PTR_ERR(chip), > > + "Failed to allocate pwmchip\n"); > > + > > + sun8i_chip = sun8i_pwm_from_chip(chip); > > + sun8i_chip->data = data; > > + sun8i_chip->base = devm_platform_ioremap_resource(pdev, 0); > > + if (IS_ERR(sun8i_chip->base)) > > + return dev_err_probe(dev, PTR_ERR(sun8i_chip->base), > > + "Failed to get PWM base address\n"); > > + > > + sun8i_chip->bus_clk = devm_clk_get_enabled(dev, "bus"); > > + if (IS_ERR(sun8i_chip->bus_clk)) > > + return dev_err_probe(dev, PTR_ERR(sun8i_chip->bus_clk), > > + "Failed to get bus clock\n"); > > + > > + sun8i_chip->channels = devm_kmalloc_array(dev, data->npwm, > > + sizeof(*(sun8i_chip->channels)), > > + GFP_KERNEL); > > + if (!sun8i_chip->channels) > > + return dev_err_probe(dev, -ENOMEM, > > + "Failed to allocate %d channels array\n", > > + data->npwm); > > Can you use a flexible array for ->channels and allocate it together > with sun8i_chip to reduce memory fragmentation? > > > + chip->ops = &sun8i_pwm_ops; > > + > > + ret = sun8i_pwm_init_clocks(pdev, sun8i_chip); > > + if (ret) > > + return ret; > > + > > + for (unsigned int i = 0; i < data->npwm; i++) { > > + struct sun8i_pwm_channel *chan = &sun8i_chip->channels[i]; > > + struct clk_hw **hw = &sun8i_chip->clk_pdata->hw_data->hws[i]; > > + > > + chan->pwm_clk = devm_clk_hw_get_clk(dev, *hw, NULL); > > + if (IS_ERR(chan->pwm_clk)) { > > + ret = dev_err_probe(dev, PTR_ERR(chan->pwm_clk), > > + "Failed to register PWM clock %d\n", i); > > + return ret; > > + } > > + chan->mode = SUN8I_PWM_MODE_NONE; > > + } > > + > > + ret = devm_of_clk_add_hw_provider(dev, sun8i_pwm_of_clk_get, sun8i_chip); > > + if (ret) > > + return dev_err_probe(dev, ret, "Failed to add HW clock provider\n"); > > + > > + ret = devm_add_action_or_reset(dev, sun8i_pwm_unregister_clk, > > + sun8i_chip->clk_pdata->hw_data); > > + if (ret) > > + return dev_err_probe(dev, ret, "Failed to add devm action\n"); > > This undoes sun8i_pwm_init_clocks(), right? So this should be moved to > directly after the call to that function, otherwise it's skipped on the > error returns between the two calls? > > > + /* Deassert reset */ > > + sun8i_chip->rst = devm_reset_control_get_shared_deasserted(dev, NULL); > > + if (IS_ERR(sun8i_chip->rst)) > > + return dev_err_probe(dev, PTR_ERR(sun8i_chip->rst), > > + "Failed to get reset control\n"); > > Does this reset affect the clks registered above and thus should be > deasserted before? > > > + ret = devm_pwmchip_add(dev, chip); > > + if (ret < 0) > > + return dev_err_probe(dev, ret, "Failed to add PWM chip\n"); > > + > > + platform_set_drvdata(pdev, chip); > > This is unused I think? > > > + return 0; > > +} > > I didn't look at Sashiko's feedback > (https://lore.kernel.org/all/[email protected]) > but I would expect that not all 9 high-severity issues identified are > slop. Please look through these for the next revision. I sent a v8 that should have fixed all the sashiko issues along with a bunch of other issues: https://lore.kernel.org/all/[email protected]/T/ > > Best regards > Uwe