| Message ID | 20240501083242.773305-1-qiujingbao.dlmu@gmail.com |
|---|---|
| Headers | show |
| Series | riscv: pwm: sophgo: add pwm support for CV1800 | expand |
Hi, Jingbao, This patchset should also be sent to the linux-riscv mailinglist. On 2024/5/1 16:32, Jingbao Qiu wrote: > The Sophgo CV1800 chip provides a set of four independent > PWM channel outputs. > This series adds PWM controller support for Sophgo cv1800. > > Changes since v6: > - add detailed Limitations > - using BIT(n) instead BIT(0) << n > - use 0 instead of disable macro > - modify OE judgment criteria > - add devm_regmap_init_mmio error message > - delete unused variable > > v6: https://lore.kernel.org/all/20240406063413.3334639-1-qiujingbao.dlmu@gmail.com/ > > Changes since v5: > - delete the OE function because we plan to use the counter subsystem > instead of capture, so there is no need to reuse this code. > - fix set polarity reverse error. > > v5: https://lore.kernel.org/all/20240314100131.323540-1-qiujingbao.dlmu@gmail.com/ > > Changes since v4: > - drop filename > - fix macro > - optimize cv1800_pwm_set_polarity() > - optimize cv1800_pwm_set_oe() > - add comment for cv1800_pwm_set_oe() > - use ticks replace tem > - fix duty_cycle larger than period_val > - use devm_clk_rate_exclusive_get() replace > clk_rate_exclusive_get() > - map linux polarity to register polarity > > v4: https://lore.kernel.org/all/20240304085933.1246964-1-qiujingbao.dlmu@gmail.com/ > > datasheet Link: https://github.com/milkv-duo/duo-files/blob/main/duo/datasheet/CV1800B-CV1801B-Preliminary-Datasheet-full-en.pdf > page 614 > > Changes since v3: > - use macro instead of npwm number > - add support for polarity feature > - add support for Output-Enable/OE feature > > v3: https://lore.kernel.org/all/20240223082014.109385-1-qiujingbao.dlmu@gmail.com/ > > Changes since v2: > - use 0x08 instead of macro > - split if statements based on conditions > - in order to round up, first calculate the > number of high-level cycles, then subtract > it from the PERIOD to obtain the number of HLPERIOD > - use new pwmchip_alloc() API instead of old style > > v2: https://lore.kernel.org/all/20240212121729.1086718-1-qiujingbao.dlmu@gmail.com/ > > Changes since v1: > - drop full stop from subject > - re-order maintainers and description > - pass checkpatch.pl --strict > - fix naming errors > - add "Limitations" section > - use a driver specific prefix for all defines > - using bool instead u32 in cv1800_pwm_enable > - check and set state->polarity > - use mul_u64_u64_div_u64 > - use clk_rate_exclusive_get(), balance with clk_rate_exclusive_put() > - using macro definitions instead of shift operations > - remove shift operation on 0 > - use priv replace cv_pwm > - hardcode npwm > - set atomic to true > - remove MODULE_ALIAS > > v1: https://lore.kernel.org/all/20240207055856.672184-1-qiujingbao.dlmu@gmail.com/ > > Jingbao Qiu (2): > dt-bindings: pwm: sophgo: add pwm for Sophgo CV1800 series SoC > pwm: sophgo: add pwm support for Sophgo CV1800 SoC > > .../bindings/pwm/sophgo,cv1800-pwm.yaml | 45 +++ > drivers/pwm/Kconfig | 10 + > drivers/pwm/Makefile | 1 + > drivers/pwm/pwm-cv1800.c | 293 ++++++++++++++++++ > 4 files changed, 349 insertions(+) > create mode 100644 Documentation/devicetree/bindings/pwm/sophgo,cv1800-pwm.yaml > create mode 100644 drivers/pwm/pwm-cv1800.c > > > base-commit: 32c44e1fa921aebf8a5ef9f778534a30aab39313
On Fri, May 3, 2024 at 9:47 AM Chen Wang <unicorn_wang@outlook.com> wrote: > > Hi, Jingbao, > > This patchset should also be sent to the linux-riscv mailinglist. I will do that. > > On 2024/5/1 16:32, Jingbao Qiu wrote: > > The Sophgo CV1800 chip provides a set of four independent > > PWM channel outputs. > > This series adds PWM controller support for Sophgo cv1800. > > > > Changes since v6: > > - add detailed Limitations > > - using BIT(n) instead BIT(0) << n > > - use 0 instead of disable macro > > - modify OE judgment criteria > > - add devm_regmap_init_mmio error message > > - delete unused variable > > > > v6: https://lore.kernel.org/all/20240406063413.3334639-1-qiujingbao.dlmu@gmail.com/ > > > > Changes since v5: > > - delete the OE function because we plan to use the counter subsystem > > instead of capture, so there is no need to reuse this code. > > - fix set polarity reverse error. > > > > v5: https://lore.kernel.org/all/20240314100131.323540-1-qiujingbao.dlmu@gmail.com/ > > > > Changes since v4: > > - drop filename > > - fix macro > > - optimize cv1800_pwm_set_polarity() > > - optimize cv1800_pwm_set_oe() > > - add comment for cv1800_pwm_set_oe() > > - use ticks replace tem > > - fix duty_cycle larger than period_val > > - use devm_clk_rate_exclusive_get() replace > > clk_rate_exclusive_get() > > - map linux polarity to register polarity > > > > v4: https://lore.kernel.org/all/20240304085933.1246964-1-qiujingbao.dlmu@gmail.com/ > > > > datasheet Link: https://github.com/milkv-duo/duo-files/blob/main/duo/datasheet/CV1800B-CV1801B-Preliminary-Datasheet-full-en.pdf > > page 614 > > > > Changes since v3: > > - use macro instead of npwm number > > - add support for polarity feature > > - add support for Output-Enable/OE feature > > > > v3: https://lore.kernel.org/all/20240223082014.109385-1-qiujingbao.dlmu@gmail.com/ > > > > Changes since v2: > > - use 0x08 instead of macro > > - split if statements based on conditions > > - in order to round up, first calculate the > > number of high-level cycles, then subtract > > it from the PERIOD to obtain the number of HLPERIOD > > - use new pwmchip_alloc() API instead of old style > > > > v2: https://lore.kernel.org/all/20240212121729.1086718-1-qiujingbao.dlmu@gmail.com/ > > > > Changes since v1: > > - drop full stop from subject > > - re-order maintainers and description > > - pass checkpatch.pl --strict > > - fix naming errors > > - add "Limitations" section > > - use a driver specific prefix for all defines > > - using bool instead u32 in cv1800_pwm_enable > > - check and set state->polarity > > - use mul_u64_u64_div_u64 > > - use clk_rate_exclusive_get(), balance with clk_rate_exclusive_put() > > - using macro definitions instead of shift operations > > - remove shift operation on 0 > > - use priv replace cv_pwm > > - hardcode npwm > > - set atomic to true > > - remove MODULE_ALIAS > > > > v1: https://lore.kernel.org/all/20240207055856.672184-1-qiujingbao.dlmu@gmail.com/ > > > > Jingbao Qiu (2): > > dt-bindings: pwm: sophgo: add pwm for Sophgo CV1800 series SoC > > pwm: sophgo: add pwm support for Sophgo CV1800 SoC > > > > .../bindings/pwm/sophgo,cv1800-pwm.yaml | 45 +++ > > drivers/pwm/Kconfig | 10 + > > drivers/pwm/Makefile | 1 + > > drivers/pwm/pwm-cv1800.c | 293 ++++++++++++++++++ > > 4 files changed, 349 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/pwm/sophgo,cv1800-pwm.yaml > > create mode 100644 drivers/pwm/pwm-cv1800.c > > > > > > base-commit: 32c44e1fa921aebf8a5ef9f778534a30aab39313 Best regards Jingbao Qiu
Hello, On Wed, May 01, 2024 at 04:32:42PM +0800, Jingbao Qiu wrote: > [...] > diff --git a/drivers/pwm/pwm-cv1800.c b/drivers/pwm/pwm-cv1800.c > new file mode 100644 > index 000000000000..d487af637198 > --- /dev/null > +++ b/drivers/pwm/pwm-cv1800.c > @@ -0,0 +1,293 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Sophgo CV1800 PWM driver > + * Author: Jingbao Qiu <qiujingbao.dlmu@gmail.com> > + * > + * Limitations: > + * - The hardware emits the inactive level when disabled. > + * - This pwm device supports dynamic loading of PWM parameters. When PWMSTART > + * is written from 0 to 1, the register value (HLPERIODn, PERIODn) will be > + * temporarily stored inside the PWM. If you want to dynamically change the > + * waveform during PWM output, after writing the new value to HLPERIODn and > + * PERIODn, write 1 and then 0 to PWMUPDATE[n] to make the new value effective. To change polarity, the hardware has to be stopped though. > + * - Supports up to Rate/2 output, and the lowest is about Rate/(2^30-1). Rate = input clock rate? > + * - By setting HLPERIODn to 0, can produce 100% duty cycle. > + * - This hardware could support inverted polarity. By default, the value of the > + * POLARITY register is 0x0. This means that HLPERIOD represents the number > + * of low level beats. > + * - This hardware supports input mode and output mode, implemented through the > + * Output-Enable/OE register. However, this driver has not yet implemented > + * capture callback. > + */ > + > +#include <linux/clk.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/platform_device.h> > +#include <linux/pwm.h> > +#include <linux/regmap.h> > + > +#define PWM_CV1800_HLPERIOD_BASE 0x00 > +#define PWM_CV1800_PERIOD_BASE 0x04 > +#define PWM_CV1800_POLARITY 0x40 > +#define PWM_CV1800_START 0x44 > +#define PWM_CV1800_DONE 0x48 > +#define PWM_CV1800_UPDATE 0x4c > +#define PWM_CV1800_OE 0xd0 > + > +#define PWM_CV1800_HLPERIOD(n) (PWM_CV1800_HLPERIOD_BASE + ((n)*0x08)) > +#define PWM_CV1800_PERIOD(n) (PWM_CV1800_PERIOD_BASE + ((n)*0x08)) I suggest to drop the ..._BASE defines and hardcode the 0 and 4 respectively in the definition of PWM_CV1800_HLPERIOD and PWM_CV1800_PERIOD. Also please use spaces around the multiplication *. > +#define PWM_CV1800_UPDATE_MASK(n) BIT(n) > +#define PWM_CV1800_OE_MASK(n) BIT(n) > +#define PWM_CV1800_START_MASK(n) BIT(n) > +#define PWM_CV1800_POLARITY_MASK(n) BIT(n) > + > +#define PWM_CV1800_MAXPERIOD 0x3fffffff > +#define PWM_CV1800_MINPERIOD 2 > +#define PWM_CV1800_CHANNELS 4 > +#define PWM_CV1800_PERIOD_RESET BIT(1) This is strange, the PWM_CV1800_PERIOD can be programmed with values >= PWM_CV1800_MINPERIOD; if you program 2, you use period == PWM_CV1800_PERIOD_RESET. > +#define PWM_CV1800_HLPERIOD_RESET BIT(0) > +#define PWM_CV1800_REG_ENABLE(n) BIT(n) > + > +struct cv1800_pwm { > + struct regmap *map; > + struct clk *clk; > + unsigned long clk_rate; > +}; > + > +static inline struct cv1800_pwm *to_cv1800_pwm_dev(struct pwm_chip *chip) > +{ > + return pwmchip_get_drvdata(chip); > +} > + > +static const struct regmap_config cv1800_pwm_regmap_config = { > + .reg_bits = 32, > + .val_bits = 32, > + .reg_stride = 4, > +}; > + > +static int cv1800_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm, > + bool enable) > +{ > + struct cv1800_pwm *priv = to_cv1800_pwm_dev(chip); > + u32 pwm_enable, state; > + > + regmap_read(priv->map, PWM_CV1800_START, &pwm_enable); > + pwm_enable &= PWM_CV1800_START_MASK(pwm->hwpwm); This value tells if the HW is enabled, right. Maybe rename the variable to pwm_enabled (or pwm_is_enabled). > + /* > + * If the parameters are changed during runtime, Register needs > + * to be updated to take effect. > + */ > + if (pwm_enable && enable) { > + regmap_update_bits(priv->map, PWM_CV1800_UPDATE, > + PWM_CV1800_UPDATE_MASK(pwm->hwpwm), > + PWM_CV1800_REG_ENABLE(pwm->hwpwm)); I would consider regmap_set_bits(priv->map, PWM_CV1800_UPDATE, BIT(pwm->hwpwm)); more readable. > + regmap_update_bits(priv->map, PWM_CV1800_UPDATE, > + PWM_CV1800_UPDATE_MASK(pwm->hwpwm), 0); Maybe add a comment about why you modify the same bit here twice. > + } else if (!pwm_enable && enable) { > + regmap_update_bits(priv->map, PWM_CV1800_START, > + PWM_CV1800_START_MASK(pwm->hwpwm), > + PWM_CV1800_REG_ENABLE(pwm->hwpwm)); > + } else if (pwm_enable && !enable) { > + regmap_update_bits(priv->map, PWM_CV1800_START, > + PWM_CV1800_START_MASK(pwm->hwpwm), 0); > + } The previous construct can be (IMO) simplified by doing: if (!enable) { if (pwm_enabled) regmap_clear_bits(...); return 0; } if (pwm_enabled) { ... } else { ... } (This slightly changes semantics, but that's fine.) > + > + /* check and set OE/Output-Enable mode */ > + regmap_read(priv->map, PWM_CV1800_OE, &state); > + > + if ((state & BIT(pwm->hwpwm)) && enable) > + regmap_update_bits(priv->map, PWM_CV1800_OE, > + PWM_CV1800_OE_MASK(pwm->hwpwm), > + PWM_CV1800_REG_ENABLE(pwm->hwpwm)); This looks strange. If BIT(hwpwm) is already set, set it again?! Also if you used the caching implemented in regmap, you don't need to make this conditional. > + return 0; > +} > + > +static void cv1800_pwm_set_polarity(struct pwm_chip *chip, > + struct pwm_device *pwm, > + enum pwm_polarity polarity) > +{ > + struct cv1800_pwm *priv = to_cv1800_pwm_dev(chip); > + u32 config_polarity = 0; > + > + if (pwm->state.enabled) > + cv1800_pwm_enable(chip, pwm, !pwm->state.enabled); Using false instead of !pwm->state.enabled would be more straight forward. > + if (polarity == PWM_POLARITY_NORMAL) > + config_polarity = PWM_CV1800_POLARITY_MASK(pwm->hwpwm); > + > + regmap_update_bits(priv->map, PWM_CV1800_POLARITY, > + PWM_CV1800_POLARITY_MASK(pwm->hwpwm), > + config_polarity); > +} > + > +static int cv1800_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > + const struct pwm_state *state) > +{ > + struct cv1800_pwm *priv = to_cv1800_pwm_dev(chip); > + u32 period_val, hlperiod_val; > + u64 ticks; > + > + if (state->polarity != pwm->state.polarity) > + cv1800_pwm_set_polarity(chip, pwm, state->polarity); > + > + /* > + * This hardware use PERIOD and HLPERIOD registers to represent PWM waves. > + * > + * The meaning of PERIOD is how many clock cycles (from the clock source) > + * are used to represent PWM waves. > + * PERIOD = rate(MHz) / target(MHz) > + * PERIOD = period(ns) * rate(Hz) / NSEC_PER_SEC > + */ > + ticks = mul_u64_u64_div_u64(state->period, priv->clk_rate, > + NSEC_PER_SEC); > + if (ticks < PWM_CV1800_MINPERIOD) > + return -EINVAL; If you check this before configuring the period, it won't happen that the hw state is modified before you notice you cannot fulfill the requested state. > + if (ticks > PWM_CV1800_MAXPERIOD) > + ticks = PWM_CV1800_MAXPERIOD; > + period_val = (u32)ticks; Maybe use period_ticks instead of period_val to have a consistent naming. > + > + /* > + * After mapping, hlperiod represents the same polarity as duty. > + * HLPERIOD = rate(MHz) / duty(MHz) > + * HLPERIOD = duty(ns) * rate(Hz) / NSEC_PER_SEC > + */ I don't understand that comment. > + ticks = mul_u64_u64_div_u64(state->duty_cycle, priv->clk_rate, > + NSEC_PER_SEC); > + if (ticks > period_val) > + ticks = period_val; > + hlperiod_val = (u32)ticks; > + > + regmap_write(priv->map, PWM_CV1800_PERIOD(pwm->hwpwm), period_val); > + regmap_write(priv->map, PWM_CV1800_HLPERIOD(pwm->hwpwm), hlperiod_val); > + > + cv1800_pwm_enable(chip, pwm, state->enabled); > + > + return 0; > +} > + > [...] > +static int cv1800_pwm_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct cv1800_pwm *priv; > + struct pwm_chip *chip; > + void __iomem *base; > + int ret; > + > + chip = devm_pwmchip_alloc(dev, PWM_CV1800_CHANNELS, sizeof(*priv)); PWM_CV1800_CHANNELS is only used here. I'd prefer a plain 4 here. This also makes this value easier to grep for. > + if (!chip) > + return PTR_ERR(chip); > + priv = to_cv1800_pwm_dev(chip); > + > + base = devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(base)) > + return PTR_ERR(base); > + > + priv->map = devm_regmap_init_mmio(&pdev->dev, base, > + &cv1800_pwm_regmap_config); > + if (IS_ERR(priv->map)) { > + dev_err(dev, "Couldn't create PWM regmap\n"); > + return PTR_ERR(priv->map); Use dev_err_probe please. > + } > + > + priv->clk = devm_clk_get_enabled(&pdev->dev, NULL); > + if (IS_ERR(priv->clk)) > + return dev_err_probe(&pdev->dev, PTR_ERR(priv->clk), > + "clk not found\n"); > + > + ret = devm_clk_rate_exclusive_get(dev, priv->clk); > + if (ret) > + return dev_err_probe(&pdev->dev, ret, > + "failed to get exclusive rate\n"); > + > + priv->clk_rate = clk_get_rate(priv->clk); > + if (!priv->clk_rate) > + return dev_err_probe(&pdev->dev, -EINVAL, > + "Invalid clock rate: %lu\n", > + priv->clk_rate); Please also error out if clk_rate > NSEC_PER_SEC, because otherwise you might get overflows for the calculations in .apply(). > + > + chip->ops = &cv1800_pwm_ops; > + > + ret = devm_pwmchip_add(dev, chip); > + if (ret) > + return dev_err_probe(dev, ret, "Failed to add PWM chip\n"); > + > + return 0; > +} > [...] Best regards Uwe