Hello, On Sat, Jul 18, 2020 at 10:47:07PM +0800, vineetha.g.jaya.kumaran@intel.com wrote: > From: "Lai, Poey Seng" > > Enable PWM support for the Intel Keem Bay SoC. > > Signed-off-by: Lai, Poey Seng > Signed-off-by: Vineetha G. Jaya Kumaran > --- > drivers/pwm/Kconfig | 9 ++ > drivers/pwm/Makefile | 1 + > drivers/pwm/pwm-keembay.c | 239 ++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 249 insertions(+) > create mode 100644 drivers/pwm/pwm-keembay.c > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > index cb8d739..2b0419b 100644 > --- a/drivers/pwm/Kconfig > +++ b/drivers/pwm/Kconfig > @@ -569,4 +569,13 @@ config PWM_ZX > To compile this driver as a module, choose M here: the module > will be called pwm-zx. > > +config PWM_KEEMBAY > + tristate "Intel Keem Bay PWM driver" > + depends on ARM64 || COMPILE_TEST > + help > + The platform driver for Intel Keem Bay PWM controller. > + > + To compile this driver as a module, choose M here: the module > + will be called pwm-keembay. > + > endif > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile > index a59c710..0c84ff2 100644 > --- a/drivers/pwm/Makefile > +++ b/drivers/pwm/Makefile > @@ -55,3 +55,4 @@ 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_ZX) += pwm-zx.o > +obj-$(CONFIG_PWM_KEEMBAY) += pwm-keembay.o > diff --git a/drivers/pwm/pwm-keembay.c b/drivers/pwm/pwm-keembay.c > new file mode 100644 > index 00000000..fa5fe95 > --- /dev/null > +++ b/drivers/pwm/pwm-keembay.c > @@ -0,0 +1,239 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Intel Keem Bay PWM driver > + * > + * Copyright (C) 2020 Intel Corporation > + * Authors: Lai Poey Seng > + * Vineetha G. Jaya Kumaran > + */ If possible, please add a link here to documentation for this chip. > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#define KMB_TOTAL_PWM_CHANNELS 6 > +#define KMB_PWM_COUNT_MAX 65535 If you write this as hexadecimal constant it is more obvious. > +#define KMB_PWM_EN_BIT BIT(31) > + > +/* Mask */ > +#define KMB_PWM_HIGH_MASK GENMASK(31, 16) > +#define KMB_PWM_LOW_MASK GENMASK(15, 0) > +#define KMB_PWM_COUNT_MASK GENMASK(31, 0) > + > +/* PWM Register offset */ > +#define KMB_PWM_LEADIN0_OFFSET 0x00 > +#define KMB_PWM_LEADIN1_OFFSET 0x04 > +#define KMB_PWM_LEADIN2_OFFSET 0x08 > +#define KMB_PWM_LEADIN3_OFFSET 0x0c > +#define KMB_PWM_LEADIN4_OFFSET 0x10 > +#define KMB_PWM_LEADIN5_OFFSET 0x14 All but ..LEADIN0.. are unused. Is this maybe more useful to write as: #define KMB_PWM_LEADIN_OFFSET(ch) (0x00 + 4 * (ch)) ? > +#define KMB_PWM_HIGHLOW0_OFFSET 0x20 > +#define KMB_PWM_HIGHLOW1_OFFSET 0x24 > +#define KMB_PWM_HIGHLOW2_OFFSET 0x28 > +#define KMB_PWM_HIGHLOW3_OFFSET 0x2c > +#define KMB_PWM_HIGHLOW4_OFFSET 0x30 > +#define KMB_PWM_HIGHLOW5_OFFSET 0x34 ditto. > +struct keembay_pwm { > + struct pwm_chip chip; > + struct device *dev; > + struct clk *clk; > + void __iomem *base; > +}; > + > +static inline struct keembay_pwm *to_keembay_pwm_dev(struct pwm_chip *chip) > +{ > + return container_of(chip, struct keembay_pwm, chip); > +} > + > +static inline void keembay_pwm_update_bits(struct keembay_pwm *priv, u32 mask, > + u32 val, u32 reg, int ch) > +{ > + u32 buff, offset, tmp; > + void __iomem *address; > + > + offset = reg + ch * 4; > + address = priv->base + offset; > + buff = readl(address); > + tmp = buff & ~mask; > + tmp |= FIELD_PREP(mask, val); > + writel(tmp, address); > +} > + > +static void keembay_pwm_enable(struct keembay_pwm *priv, int ch) > +{ > + keembay_pwm_update_bits(priv, KMB_PWM_EN_BIT, 1, > + KMB_PWM_LEADIN0_OFFSET, ch); > +} > + > +static void keembay_pwm_disable(struct keembay_pwm *priv, int ch) > +{ > + keembay_pwm_update_bits(priv, KMB_PWM_EN_BIT, 0, > + KMB_PWM_LEADIN0_OFFSET, ch); > +} > + > +static void keembay_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm, > + struct pwm_state *state) > +{ > + struct keembay_pwm *priv = to_keembay_pwm_dev(chip); > + unsigned long long pwm_h_count, pwm_l_count; > + unsigned long clk_rate; > + u32 buff; > + > + clk_rate = clk_get_rate(priv->clk); > + > + /* Read channel enabled status */ > + buff = readl(priv->base + KMB_PWM_LEADIN0_OFFSET + pwm->hwpwm * 4); > + if (buff & KMB_PWM_EN_BIT) > + state->enabled = true; > + else > + state->enabled = false; > + > + /* Read period and duty cycle*/ Missing ' ' before closing */ > + buff = readl(priv->base + KMB_PWM_HIGHLOW0_OFFSET + pwm->hwpwm * 4); > + pwm_l_count = (buff & KMB_PWM_LOW_MASK) * NSEC_PER_SEC; > + pwm_h_count = ((buff & KMB_PWM_HIGH_MASK) >> 16) * NSEC_PER_SEC; pwm_h_count = FIELD_GET(KMB_PWM_HIGH_MASK, buff) * NSEC_PER_SEC > + state->duty_cycle = pwm_h_count / clk_rate; > + state->period = (pwm_h_count + pwm_l_count) / clk_rate; Please round up both values. > +} > + > +static int keembay_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > + const struct pwm_state *state) > +{ > + struct keembay_pwm *priv = to_keembay_pwm_dev(chip); > + struct pwm_state current_state; > + u16 pwm_h_count, pwm_l_count; > + unsigned long long div; > + unsigned long clk_rate; > + u32 pwm_count = 0; > + > + pwm_get_state(pwm, ¤t_state); Please check the hardware state, not the value cached in the PWM framework. > + if (!state->enabled && current_state.enabled) { > + keembay_pwm_disable(priv, pwm->hwpwm); > + return 0; > + } > + > + if (state->polarity != PWM_POLARITY_NORMAL) > + return -ENOSYS; This must be checked before .enabled. That's because the expectation on .enabled = false .polarity = PWM_POLARITY_INVERSED is that the output gets constant high. > + /* > + * The upper 16 bits of the KMB_PWM_HIGHLOWx_OFFSET register contain > + * the high time of the waveform, while the last 16 bits contain > + * the low time of the waveform, in terms of clock cycles. Just to be sure: Each period starts with the high time, right? > + * high time = clock rate * duty cycle / NSEC_PER_SEC > + * low time = clock rate * (period - duty cycle) / NSEC_PER_SEC > + * > + * e.g. For period 50000ns, duty cycle 30000ns, and clock rate 500MHz: > + * high time = (500000000 * 30000) / 1000000000 = 0x3A98 > + * low time = (500000000 * 20000) / 1000000000 = 0x2710 > + * Value written to KMB_PWM_HIGHLOWx_OFFSET = 0x3A982710 For period = 50000ns, duty_cycle = 30000ns and clock rate 266666666 Hz you have to configure: high = 7999 low = 5334 > + */ > + > + clk_rate = clk_get_rate(priv->clk); > + > + /* Configure waveform high time */ > + div = clk_rate * state->duty_cycle; > + do_div(div, NSEC_PER_SEC); Can this overflow? > + if (div > KMB_PWM_COUNT_MAX) > + return -ERANGE; > + > + pwm_h_count = (u16)div; No need for this cast. > + /* Configure waveform low time */ > + div = clk_rate * (state->period - state->duty_cycle); > + do_div(div, NSEC_PER_SEC); Here the rounding is wrong. (See above example, currently you use low = 5333 here) > + if (div > KMB_PWM_COUNT_MAX) > + return -ERANGE; > + > + pwm_l_count = (u16)div; > + > + pwm_count |= pwm_h_count << 16; > + pwm_count |= pwm_l_count; pwm_count = FIELD_PREP(KMB_PWM_HIGH_MASK, pwm_h_count) | FIELD_PREP(KMB_PWM_LOW_MASK, pwm_l_count); > + > + keembay_pwm_update_bits(priv, KMB_PWM_COUNT_MASK, > + pwm_count, KMB_PWM_HIGHLOW0_OFFSET, pwm->hwpwm); here the rwm-procedure is not necessary as all 32 bits are written anyhow. > + if (state->enabled && !current_state.enabled) > + keembay_pwm_enable(priv, pwm->hwpwm); > + > + return 0; > +} > [...] > +MODULE_ALIAS("platform:keembay"); This has to match your driver's name, so use: MODULE_ALIAS("platform:pwm-keembay"); > +MODULE_DESCRIPTION("Intel Keem Bay PWM driver"); > +MODULE_LICENSE("GPL v2"); In v1 you told that on reconfiguration the hardware completes the currently running period. Please document this in the driver, similar to e.g. pwm-sifive.c. You only ever write the enable bit in the leadin register. If there is something != 0 at boot this influences the correct behaviour of the driver, right? Further things to note there: - What is the behaviour on disable (usual candidates: freezes at current value, completes period and emits low, changes to High-Z)? - Can it do 0% and 100% duty ratio? Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |