* [PATCH v1 0/2] pwm: intel: Add PWM driver for new SoC @ 2020-05-22 7:41 Rahul Tanwar 2020-05-22 7:41 ` [PATCH v1 1/2] Add YAML schema for a new PWM driver Rahul Tanwar 2020-05-22 7:41 ` [PATCH v1 2/2] Add PWM driver for LGM Rahul Tanwar 0 siblings, 2 replies; 10+ messages in thread From: Rahul Tanwar @ 2020-05-22 7:41 UTC (permalink / raw) To: thierry.reding, u.kleine-koenig, p.zabel, linux-pwm, robh+dt Cc: linux-kernel, devicetree, andriy.shevchenko, songjun.Wu, cheol.yong.kim, qi-ming.wu, Rahul Tanwar Patch 1 adds dt binding document in YAML format. Patch 2 add PWM driver for LGM SoC. Rahul Tanwar (2): Add YAML schema for a new PWM driver Add PWM driver for LGM .../devicetree/bindings/pwm/pwm-intel-lgm.yaml | 43 +++ drivers/pwm/Kconfig | 9 + drivers/pwm/Makefile | 1 + drivers/pwm/pwm-intel-lgm.c | 356 +++++++++++++++++++++ 4 files changed, 409 insertions(+) create mode 100644 Documentation/devicetree/bindings/pwm/pwm-intel-lgm.yaml create mode 100644 drivers/pwm/pwm-intel-lgm.c -- 2.11.0 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v1 1/2] Add YAML schema for a new PWM driver 2020-05-22 7:41 [PATCH v1 0/2] pwm: intel: Add PWM driver for new SoC Rahul Tanwar @ 2020-05-22 7:41 ` Rahul Tanwar 2020-05-28 23:31 ` Rob Herring 2020-05-22 7:41 ` [PATCH v1 2/2] Add PWM driver for LGM Rahul Tanwar 1 sibling, 1 reply; 10+ messages in thread From: Rahul Tanwar @ 2020-05-22 7:41 UTC (permalink / raw) To: thierry.reding, u.kleine-koenig, p.zabel, linux-pwm, robh+dt Cc: linux-kernel, devicetree, andriy.shevchenko, songjun.Wu, cheol.yong.kim, qi-ming.wu, Rahul Tanwar Add DT bindings YAML schema for PWM controller driver of Lightning Mountain(LGM) SoC. Signed-off-by: Rahul Tanwar <rahul.tanwar@linux.intel.com> --- .../devicetree/bindings/pwm/pwm-intel-lgm.yaml | 43 ++++++++++++++++++++++ 1 file changed, 43 insertions(+) create mode 100644 Documentation/devicetree/bindings/pwm/pwm-intel-lgm.yaml diff --git a/Documentation/devicetree/bindings/pwm/pwm-intel-lgm.yaml b/Documentation/devicetree/bindings/pwm/pwm-intel-lgm.yaml new file mode 100644 index 000000000000..adb33265aa5e --- /dev/null +++ b/Documentation/devicetree/bindings/pwm/pwm-intel-lgm.yaml @@ -0,0 +1,43 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/pwm/pwm-intel-lgm.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: LGM SoC PWM controller + +maintainers: + - Rahul Tanwar <rahul.tanwar@intel.com> + +properties: + compatible: + const: intel,lgm-pwm + + reg: + maxItems: 1 + + "#pwm-cells": + const: 2 + + clocks: + maxItems: 1 + + resets: + maxItems: 1 + +required: + - compatible + - reg + - "#pwm-cells" + - clocks + - resets + +examples: + - | + pwm: pwm@e0d00000 { + compatible = "intel,lgm-pwm"; + reg = <0xe0d00000 0x30>; + #pwm-cells = <2>; + clocks = <&cgu0 126>; + resets = <&rcu0 0x30 21>; + }; -- 2.11.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v1 1/2] Add YAML schema for a new PWM driver 2020-05-22 7:41 ` [PATCH v1 1/2] Add YAML schema for a new PWM driver Rahul Tanwar @ 2020-05-28 23:31 ` Rob Herring 0 siblings, 0 replies; 10+ messages in thread From: Rob Herring @ 2020-05-28 23:31 UTC (permalink / raw) To: Rahul Tanwar Cc: thierry.reding, u.kleine-koenig, p.zabel, linux-pwm, linux-kernel, devicetree, andriy.shevchenko, songjun.Wu, cheol.yong.kim, qi-ming.wu On Fri, May 22, 2020 at 03:41:58PM +0800, Rahul Tanwar wrote: > Add DT bindings YAML schema for PWM controller driver of > Lightning Mountain(LGM) SoC. You need a better subject such as what h/w this is for. Bindings are for h/w blocks, not drivers. > > Signed-off-by: Rahul Tanwar <rahul.tanwar@linux.intel.com> > --- > .../devicetree/bindings/pwm/pwm-intel-lgm.yaml | 43 ++++++++++++++++++++++ Use the compatible string for filename. > 1 file changed, 43 insertions(+) > create mode 100644 Documentation/devicetree/bindings/pwm/pwm-intel-lgm.yaml > > diff --git a/Documentation/devicetree/bindings/pwm/pwm-intel-lgm.yaml b/Documentation/devicetree/bindings/pwm/pwm-intel-lgm.yaml > new file mode 100644 > index 000000000000..adb33265aa5e > --- /dev/null > +++ b/Documentation/devicetree/bindings/pwm/pwm-intel-lgm.yaml > @@ -0,0 +1,43 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/pwm/pwm-intel-lgm.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: LGM SoC PWM controller > + > +maintainers: > + - Rahul Tanwar <rahul.tanwar@intel.com> > + > +properties: > + compatible: > + const: intel,lgm-pwm > + > + reg: > + maxItems: 1 > + > + "#pwm-cells": > + const: 2 > + > + clocks: > + maxItems: 1 > + > + resets: > + maxItems: 1 > + > +required: > + - compatible > + - reg > + - "#pwm-cells" > + - clocks > + - resets additionalProperties: false > + > +examples: > + - | > + pwm: pwm@e0d00000 { > + compatible = "intel,lgm-pwm"; > + reg = <0xe0d00000 0x30>; > + #pwm-cells = <2>; > + clocks = <&cgu0 126>; > + resets = <&rcu0 0x30 21>; > + }; > -- > 2.11.0 > ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v1 2/2] Add PWM driver for LGM 2020-05-22 7:41 [PATCH v1 0/2] pwm: intel: Add PWM driver for new SoC Rahul Tanwar 2020-05-22 7:41 ` [PATCH v1 1/2] Add YAML schema for a new PWM driver Rahul Tanwar @ 2020-05-22 7:41 ` Rahul Tanwar 2020-05-22 8:56 ` Uwe Kleine-König 1 sibling, 1 reply; 10+ messages in thread From: Rahul Tanwar @ 2020-05-22 7:41 UTC (permalink / raw) To: thierry.reding, u.kleine-koenig, p.zabel, linux-pwm, robh+dt Cc: linux-kernel, devicetree, andriy.shevchenko, songjun.Wu, cheol.yong.kim, qi-ming.wu, Rahul Tanwar Add PWM controller driver for Intel's Lightning Mountain(LGM) SoC. Signed-off-by: Rahul Tanwar <rahul.tanwar@linux.intel.com> --- drivers/pwm/Kconfig | 9 ++ drivers/pwm/Makefile | 1 + drivers/pwm/pwm-intel-lgm.c | 356 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 366 insertions(+) create mode 100644 drivers/pwm/pwm-intel-lgm.c diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index eebbc917ac97..a582214f50b2 100644 --- a/drivers/pwm/Kconfig +++ b/drivers/pwm/Kconfig @@ -232,6 +232,15 @@ config PWM_IMX_TPM To compile this driver as a module, choose M here: the module will be called pwm-imx-tpm. +config PWM_INTEL_LGM + tristate "Intel LGM PWM support" + depends on X86 || COMPILE_TEST + help + Generic PWM framework driver for LGM SoC. + + To compile this driver as a module, choose M here: the module + will be called pwm-intel-lgm. + config PWM_JZ4740 tristate "Ingenic JZ47xx PWM support" depends on MACH_INGENIC diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile index 9a475073dafc..c16a972a101d 100644 --- a/drivers/pwm/Makefile +++ b/drivers/pwm/Makefile @@ -20,6 +20,7 @@ obj-$(CONFIG_PWM_IMG) += pwm-img.o obj-$(CONFIG_PWM_IMX1) += pwm-imx1.o obj-$(CONFIG_PWM_IMX27) += pwm-imx27.o obj-$(CONFIG_PWM_IMX_TPM) += pwm-imx-tpm.o +obj-$(CONFIG_PWM_INTEL_LGM) += pwm-intel-lgm.o obj-$(CONFIG_PWM_JZ4740) += pwm-jz4740.o obj-$(CONFIG_PWM_LP3943) += pwm-lp3943.o obj-$(CONFIG_PWM_LPC18XX_SCT) += pwm-lpc18xx-sct.o diff --git a/drivers/pwm/pwm-intel-lgm.c b/drivers/pwm/pwm-intel-lgm.c new file mode 100644 index 000000000000..e307fd2457df --- /dev/null +++ b/drivers/pwm/pwm-intel-lgm.c @@ -0,0 +1,356 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (C) 2020 Intel Corporation. + */ +#include <linux/bitfield.h> +#include <linux/clk.h> +#include <linux/module.h> +#include <linux/of_device.h> +#include <linux/pwm.h> +#include <linux/regmap.h> +#include <linux/reset.h> + +#define PWM_FAN_CON0 0x0 +#define PWM_FAN_EN_EN BIT(0) +#define PWM_FAN_EN_DIS 0x0 +#define PWM_FAN_EN_MSK BIT(0) +#define PWM_FAN_MODE_2WIRE 0x0 +#define PWM_FAN_MODE_4WIRE 0x1 +#define PWM_FAN_MODE_MSK BIT(1) +#define PWM_FAN_PWM_DIS_DIS 0x0 +#define PWM_FAN_PWM_DIS_MSK BIT(2) +#define PWM_TACH_EN_EN 0x1 +#define PWM_TACH_EN_MSK BIT(4) +#define PWM_TACH_PLUS_2 0x0 +#define PWM_TACH_PLUS_4 0x1 +#define PWM_TACH_PLUS_MSK BIT(5) +#define PWM_FAN_DC_MSK GENMASK(23, 16) + +#define PWM_FAN_CON1 0x4 +#define PWM_FAN_MAX_RPM_MSK GENMASK(15, 0) + +#define PWM_FAN_STAT 0x10 +#define PWM_FAN_TACH_MASK GENMASK(15, 0) + +#define MAX_RPM (BIT(16) - 1) +#define DFAULT_RPM 4000 +#define MAX_DUTY_CYCLE (BIT(8) - 1) + +#define FRAC_BITS 10 +#define TWO_TENTH 204 + +#define TWO_SECONDS 2000 +#define IGNORE_FIRST_ERR 1 +#define THIRTY_SECS_WINDOW 15 +#define ERR_CNT_THRESHOLD 6 + +struct intel_pwm_chip { + struct pwm_chip chip; + struct regmap *regmap; + struct clk *clk; + struct reset_control *rst; + u32 tach_en; + u32 max_rpm; + u32 set_rpm; + u32 set_dc; + struct delayed_work work; +}; + +static inline struct intel_pwm_chip *to_intel_pwm_chip(struct pwm_chip *chip) +{ + return container_of(chip, struct intel_pwm_chip, chip); +} + +static int pwm_update_dc(struct intel_pwm_chip *pc, u32 val) +{ + return regmap_update_bits(pc->regmap, PWM_FAN_CON0, PWM_FAN_DC_MSK, + FIELD_PREP(PWM_FAN_DC_MSK, val)); +} + +static int intel_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, + int duty_ns, int period_ns) +{ + struct intel_pwm_chip *pc = to_intel_pwm_chip(chip); + u32 val; + + val = DIV_ROUND_CLOSEST(duty_ns * MAX_DUTY_CYCLE, period_ns); + val = min_t(u32, val, MAX_DUTY_CYCLE); + + if (pc->tach_en) { + pc->set_dc = val; + pc->set_rpm = val * pc->max_rpm / MAX_DUTY_CYCLE; + } + + return pwm_update_dc(pc, val); +} + +static int intel_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm) +{ + struct intel_pwm_chip *pc = to_intel_pwm_chip(chip); + struct regmap *regmap = pc->regmap; + + regmap_update_bits(regmap, PWM_FAN_CON0, + PWM_FAN_EN_MSK, PWM_FAN_EN_EN); + + if (pc->tach_en) + schedule_delayed_work(&pc->work, msecs_to_jiffies(10000)); + + return 0; +} + +static void intel_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm) +{ + struct intel_pwm_chip *pc = to_intel_pwm_chip(chip); + struct regmap *regmap = pc->regmap; + + if (pc->tach_en) + cancel_delayed_work_sync(&pc->work); + + regmap_update_bits(regmap, PWM_FAN_CON0, + PWM_FAN_EN_MSK, PWM_FAN_EN_DIS); +} + +static const struct pwm_ops intel_pwm_ops = { + .config = intel_pwm_config, + .enable = intel_pwm_enable, + .disable = intel_pwm_disable, + .owner = THIS_MODULE, +}; + +static void tach_work(struct work_struct *work) +{ + struct intel_pwm_chip *pc = container_of(work, struct intel_pwm_chip, + work.work); + struct regmap *regmap = pc->regmap; + u32 fan_tach, fan_dc, val; + s32 diff; + static u32 fanspeed_err_cnt, time_window, delta_dc; + + /* + * Fan speed is tracked by reading the active duty cycle of PWM output + * from the active duty cycle register. Some variance in the duty cycle + * register value is expected. So we set a time window of 30 seconds and + * if we detect inaccurate fan speed 6 times within 30 seconds then we + * mark it as fan speed problem and fix it by readjusting the duty cycle. + */ + + if (fanspeed_err_cnt > IGNORE_FIRST_ERR) + /* + * Ignore first time we detect inaccurate fan speed + * because it is expected during bootup. + */ + time_window++; + + if (time_window == THIRTY_SECS_WINDOW) { + /* + * This work is scheduled every 2 seconds i.e. each time_window + * counter step roughly mean 2 seconds. When the time window + * reaches 30 seconds, reset all the counters/logic. + */ + fanspeed_err_cnt = 0; + delta_dc = 0; + time_window = 0; + } + + regmap_read(regmap, PWM_FAN_STAT, &fan_tach); + fan_tach &= PWM_FAN_TACH_MASK; + if (!fan_tach) + goto restart_work; + + val = DIV_ROUND_CLOSEST(pc->set_rpm << FRAC_BITS, fan_tach); + diff = val - BIT(FRAC_BITS); + + if (abs(diff) > TWO_TENTH) { + /* if duty cycle diff is more than two tenth, detect it as error */ + if (fanspeed_err_cnt > IGNORE_FIRST_ERR) + delta_dc += val; + fanspeed_err_cnt++; + } + + if (fanspeed_err_cnt == ERR_CNT_THRESHOLD) { + /* + * We detected fan speed errors 6 times with 30 seconds. + * Fix the error by readjusting duty cycle and reset + * our counters/logic. + */ + fan_dc = pc->set_dc * delta_dc >> (FRAC_BITS + 2); + fan_dc = min_t(u32, fan_dc, MAX_DUTY_CYCLE); + pwm_update_dc(pc, fan_dc); + fanspeed_err_cnt = 0; + delta_dc = 0; + time_window = 0; + } + +restart_work: + /* + * Fan speed doesn't need continous tracking. Schedule this work + * every two seconds so it doesn't steal too much cpu cycles. + */ + schedule_delayed_work(&pc->work, msecs_to_jiffies(TWO_SECONDS)); +} + +static void pwm_init(struct intel_pwm_chip *pc) +{ + struct device *dev = pc->chip.dev; + struct regmap *regmap = pc->regmap; + u32 max_rpm, fan_wire, tach_plus, con0_val, con0_mask; + + if (device_property_read_u32(dev, "intel,fan-wire", &fan_wire)) + fan_wire = 2; /* default is 2 wire mode */ + + con0_val = FIELD_PREP(PWM_FAN_PWM_DIS_MSK, PWM_FAN_PWM_DIS_DIS); + con0_mask = PWM_FAN_PWM_DIS_MSK | PWM_FAN_MODE_MSK; + + switch (fan_wire) { + case 2 ... 3: + con0_val |= FIELD_PREP(PWM_FAN_MODE_MSK, PWM_FAN_MODE_2WIRE); + break; + case 4: + con0_val |= FIELD_PREP(PWM_FAN_MODE_MSK, PWM_FAN_MODE_4WIRE) | + FIELD_PREP(PWM_TACH_EN_MSK, PWM_TACH_EN_EN); + con0_mask |= PWM_TACH_EN_MSK | PWM_TACH_PLUS_MSK; + pc->tach_en = 1; + break; + default: + /* default is 2wire mode */ + con0_val |= FIELD_PREP(PWM_FAN_MODE_MSK, PWM_FAN_MODE_2WIRE); + break; + } + + if (pc->tach_en) { + if (device_property_read_u32(dev, "intel,tach-plus", + &tach_plus)) + tach_plus = 2; + + switch (tach_plus) { + case 2: + con0_val |= FIELD_PREP(PWM_TACH_PLUS_MSK, + PWM_TACH_PLUS_2); + break; + case 4: + con0_val |= FIELD_PREP(PWM_TACH_PLUS_MSK, + PWM_TACH_PLUS_4); + break; + default: + con0_val |= FIELD_PREP(PWM_TACH_PLUS_MSK, + PWM_TACH_PLUS_2); + break; + } + + if (device_property_read_u32(dev, "intel,max-rpm", &max_rpm)) + max_rpm = DFAULT_RPM; + + max_rpm = min_t(u32, max_rpm, MAX_RPM); + if (max_rpm == 0) + max_rpm = DFAULT_RPM; + + pc->max_rpm = max_rpm; + INIT_DEFERRABLE_WORK(&pc->work, tach_work); + regmap_update_bits(regmap, PWM_FAN_CON1, + PWM_FAN_MAX_RPM_MSK, max_rpm); + } + + regmap_update_bits(regmap, PWM_FAN_CON0, con0_mask, con0_val); +} + +static const struct regmap_config pwm_regmap_config = { + .reg_bits = 32, + .reg_stride = 4, + .val_bits = 32, +}; + +static int intel_pwm_probe(struct platform_device *pdev) +{ + struct intel_pwm_chip *pc; + struct device *dev = &pdev->dev; + void __iomem *io_base; + int ret; + + pc = devm_kzalloc(dev, sizeof(*pc), GFP_KERNEL); + if (!pc) + return -ENOMEM; + + io_base = devm_platform_ioremap_resource(pdev, 0); + if (IS_ERR(io_base)) + return PTR_ERR(io_base); + + pc->regmap = devm_regmap_init_mmio(dev, io_base, &pwm_regmap_config); + if (IS_ERR(pc->regmap)) { + ret = PTR_ERR(pc->regmap); + dev_err(dev, "failed to init register map: %d\n", ret); + return ret; + } + + pc->clk = devm_clk_get(dev, NULL); + if (IS_ERR(pc->clk)) { + ret = PTR_ERR(pc->clk); + dev_err(dev, "failed to get clock: %d\n", ret); + return ret; + } + + pc->rst = devm_reset_control_get(dev, NULL); + if (IS_ERR(pc->rst)) { + ret = PTR_ERR(pc->rst); + dev_err(dev, "failed to get reset control: %d\n", ret); + return ret; + } + + ret = clk_prepare_enable(pc->clk); + if (ret) { + dev_err(dev, "failed to enable clock\n"); + return ret; + } + + reset_control_deassert(pc->rst); + + pc->chip.dev = dev; + pc->chip.ops = &intel_pwm_ops; + pc->chip.npwm = 1; + + pwm_init(pc); + + ret = pwmchip_add(&pc->chip); + if (ret < 0) { + dev_err(dev, "failed to add PWM chip: %d\n", ret); + clk_disable_unprepare(pc->clk); + return ret; + } + + platform_set_drvdata(pdev, pc); + return 0; +} + +static int intel_pwm_remove(struct platform_device *pdev) +{ + struct intel_pwm_chip *pc = platform_get_drvdata(pdev); + int ret; + + if (pc->tach_en) + cancel_delayed_work_sync(&pc->work); + + ret = pwmchip_remove(&pc->chip); + if (ret < 0) + return ret; + + reset_control_assert(pc->rst); + + clk_disable_unprepare(pc->clk); + + return 0; +} + +static const struct of_device_id intel_pwm_of_match[] = { + { .compatible = "intel,lgm-pwm" }, + { } +}; +MODULE_DEVICE_TABLE(of, intel_pwm_of_match); + +static struct platform_driver intel_pwm_driver = { + .driver = { + .name = "intel-pwm", + .of_match_table = intel_pwm_of_match, + }, + .probe = intel_pwm_probe, + .remove = intel_pwm_remove, +}; +module_platform_driver(intel_pwm_driver); -- 2.11.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v1 2/2] Add PWM driver for LGM 2020-05-22 7:41 ` [PATCH v1 2/2] Add PWM driver for LGM Rahul Tanwar @ 2020-05-22 8:56 ` Uwe Kleine-König 2020-05-22 9:18 ` Andy Shevchenko 2020-05-27 6:28 ` Tanwar, Rahul 0 siblings, 2 replies; 10+ messages in thread From: Uwe Kleine-König @ 2020-05-22 8:56 UTC (permalink / raw) To: Rahul Tanwar Cc: thierry.reding, p.zabel, linux-pwm, robh+dt, linux-kernel, devicetree, andriy.shevchenko, songjun.Wu, cheol.yong.kim, qi-ming.wu Hello, On Fri, May 22, 2020 at 03:41:59PM +0800, Rahul Tanwar wrote: > Add PWM controller driver for Intel's Lightning Mountain(LGM) SoC. > > Signed-off-by: Rahul Tanwar <rahul.tanwar@linux.intel.com> > --- > drivers/pwm/Kconfig | 9 ++ > drivers/pwm/Makefile | 1 + > drivers/pwm/pwm-intel-lgm.c | 356 ++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 366 insertions(+) > create mode 100644 drivers/pwm/pwm-intel-lgm.c > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > index eebbc917ac97..a582214f50b2 100644 > --- a/drivers/pwm/Kconfig > +++ b/drivers/pwm/Kconfig > @@ -232,6 +232,15 @@ config PWM_IMX_TPM > To compile this driver as a module, choose M here: the module > will be called pwm-imx-tpm. > > +config PWM_INTEL_LGM > + tristate "Intel LGM PWM support" > + depends on X86 || COMPILE_TEST > + help > + Generic PWM framework driver for LGM SoC. I wouldn't call this "framework". > + To compile this driver as a module, choose M here: the module > + will be called pwm-intel-lgm. > + > config PWM_JZ4740 > tristate "Ingenic JZ47xx PWM support" > depends on MACH_INGENIC > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile > index 9a475073dafc..c16a972a101d 100644 > --- a/drivers/pwm/Makefile > +++ b/drivers/pwm/Makefile > @@ -20,6 +20,7 @@ obj-$(CONFIG_PWM_IMG) += pwm-img.o > obj-$(CONFIG_PWM_IMX1) += pwm-imx1.o > obj-$(CONFIG_PWM_IMX27) += pwm-imx27.o > obj-$(CONFIG_PWM_IMX_TPM) += pwm-imx-tpm.o > +obj-$(CONFIG_PWM_INTEL_LGM) += pwm-intel-lgm.o > obj-$(CONFIG_PWM_JZ4740) += pwm-jz4740.o > obj-$(CONFIG_PWM_LP3943) += pwm-lp3943.o > obj-$(CONFIG_PWM_LPC18XX_SCT) += pwm-lpc18xx-sct.o > diff --git a/drivers/pwm/pwm-intel-lgm.c b/drivers/pwm/pwm-intel-lgm.c > new file mode 100644 > index 000000000000..e307fd2457df > --- /dev/null > +++ b/drivers/pwm/pwm-intel-lgm.c > @@ -0,0 +1,356 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (C) 2020 Intel Corporation. > + */ Please point out limitations of your driver here similar to e.g. drivers/pwm/pwm-sifive.c (and in the same format). You should mention the fixed period of the hardware here (assuming I interpreted this correctly below) and missing support for polarity. How does the PWM behave on disable? Does the output become inactive then? Does it complete the currently running period? What about reconfiguration? Does changing the duty cycle complete the currently running period? > +#include <linux/bitfield.h> > +#include <linux/clk.h> > +#include <linux/module.h> > +#include <linux/of_device.h> > +#include <linux/pwm.h> > +#include <linux/regmap.h> > +#include <linux/reset.h> > + > +#define PWM_FAN_CON0 0x0 > +#define PWM_FAN_EN_EN BIT(0) > +#define PWM_FAN_EN_DIS 0x0 > +#define PWM_FAN_EN_MSK BIT(0) > +#define PWM_FAN_MODE_2WIRE 0x0 > +#define PWM_FAN_MODE_4WIRE 0x1 > +#define PWM_FAN_MODE_MSK BIT(1) > +#define PWM_FAN_PWM_DIS_DIS 0x0 > +#define PWM_FAN_PWM_DIS_MSK BIT(2) > +#define PWM_TACH_EN_EN 0x1 > +#define PWM_TACH_EN_MSK BIT(4) > +#define PWM_TACH_PLUS_2 0x0 > +#define PWM_TACH_PLUS_4 0x1 > +#define PWM_TACH_PLUS_MSK BIT(5) > +#define PWM_FAN_DC_MSK GENMASK(23, 16) > + > +#define PWM_FAN_CON1 0x4 > +#define PWM_FAN_MAX_RPM_MSK GENMASK(15, 0) > + > +#define PWM_FAN_STAT 0x10 > +#define PWM_FAN_TACH_MASK GENMASK(15, 0) > + > +#define MAX_RPM (BIT(16) - 1) > +#define DFAULT_RPM 4000 > +#define MAX_DUTY_CYCLE (BIT(8) - 1) > + > +#define FRAC_BITS 10 > +#define TWO_TENTH 204 > + > +#define TWO_SECONDS 2000 > +#define IGNORE_FIRST_ERR 1 > +#define THIRTY_SECS_WINDOW 15 > +#define ERR_CNT_THRESHOLD 6 > + > +struct intel_pwm_chip { > + struct pwm_chip chip; > + struct regmap *regmap; > + struct clk *clk; > + struct reset_control *rst; > + u32 tach_en; > + u32 max_rpm; > + u32 set_rpm; > + u32 set_dc; > + struct delayed_work work; > +}; I don't like aligning the member names and prefer a single space here. (But I'm aware this is subjective and others like it the why you did it.) > + > +static inline struct intel_pwm_chip *to_intel_pwm_chip(struct pwm_chip *chip) > +{ > + return container_of(chip, struct intel_pwm_chip, chip); > +} > + > +static int pwm_update_dc(struct intel_pwm_chip *pc, u32 val) Please use a common function prefix for all functions in your driver. > +{ > + return regmap_update_bits(pc->regmap, PWM_FAN_CON0, PWM_FAN_DC_MSK, > + FIELD_PREP(PWM_FAN_DC_MSK, val)); > +} > + > +static int intel_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, > + int duty_ns, int period_ns) > +{ > + struct intel_pwm_chip *pc = to_intel_pwm_chip(chip); > + u32 val; > + > + val = DIV_ROUND_CLOSEST(duty_ns * MAX_DUTY_CYCLE, period_ns); > + val = min_t(u32, val, MAX_DUTY_CYCLE); The period is fixed for this hardware type? If so you're supposed to refuse requests for a period smaller than the actual period length. Also duty_cycle is supposed to round down. (That's something that PWM_DEBUG will tell you, too) > + if (pc->tach_en) { > + pc->set_dc = val; > + pc->set_rpm = val * pc->max_rpm / MAX_DUTY_CYCLE; > + } > + > + return pwm_update_dc(pc, val); > +} > + > +static int intel_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm) > +{ > + struct intel_pwm_chip *pc = to_intel_pwm_chip(chip); > + struct regmap *regmap = pc->regmap; > + > + regmap_update_bits(regmap, PWM_FAN_CON0, > + PWM_FAN_EN_MSK, PWM_FAN_EN_EN); > + > + if (pc->tach_en) > + schedule_delayed_work(&pc->work, msecs_to_jiffies(10000)); > + > + return 0; > +} > + > +static void intel_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm) > +{ > + struct intel_pwm_chip *pc = to_intel_pwm_chip(chip); > + struct regmap *regmap = pc->regmap; > + > + if (pc->tach_en) > + cancel_delayed_work_sync(&pc->work); > + > + regmap_update_bits(regmap, PWM_FAN_CON0, > + PWM_FAN_EN_MSK, PWM_FAN_EN_DIS); > +} > + > +static const struct pwm_ops intel_pwm_ops = { > + .config = intel_pwm_config, > + .enable = intel_pwm_enable, > + .disable = intel_pwm_disable, > + .owner = THIS_MODULE, Please implement .apply instead of .config/.enable/.disable. Please also enable PWM_DEBUG and fix the issues pointed out with it. > +}; > + > +static void tach_work(struct work_struct *work) > +{ > + struct intel_pwm_chip *pc = container_of(work, struct intel_pwm_chip, > + work.work); > + struct regmap *regmap = pc->regmap; > + u32 fan_tach, fan_dc, val; > + s32 diff; > + static u32 fanspeed_err_cnt, time_window, delta_dc; > + > + /* > + * Fan speed is tracked by reading the active duty cycle of PWM output > + * from the active duty cycle register. Some variance in the duty cycle > + * register value is expected. So we set a time window of 30 seconds and > + * if we detect inaccurate fan speed 6 times within 30 seconds then we > + * mark it as fan speed problem and fix it by readjusting the duty cycle. > + */ I'm a unhappy to have this in the PWM driver. The PWM driver is supposed to be generic and I think this belongs into a dedicated driver. > + > + if (fanspeed_err_cnt > IGNORE_FIRST_ERR) > + /* > + * Ignore first time we detect inaccurate fan speed > + * because it is expected during bootup. > + */ > + time_window++; > + > + if (time_window == THIRTY_SECS_WINDOW) { > + /* > + * This work is scheduled every 2 seconds i.e. each time_window > + * counter step roughly mean 2 seconds. When the time window > + * reaches 30 seconds, reset all the counters/logic. > + */ > + fanspeed_err_cnt = 0; > + delta_dc = 0; > + time_window = 0; > + } > + > + regmap_read(regmap, PWM_FAN_STAT, &fan_tach); > + fan_tach &= PWM_FAN_TACH_MASK; > + if (!fan_tach) > + goto restart_work; > + > + val = DIV_ROUND_CLOSEST(pc->set_rpm << FRAC_BITS, fan_tach); > + diff = val - BIT(FRAC_BITS); > + > + if (abs(diff) > TWO_TENTH) { > + /* if duty cycle diff is more than two tenth, detect it as error */ > + if (fanspeed_err_cnt > IGNORE_FIRST_ERR) > + delta_dc += val; > + fanspeed_err_cnt++; > + } > + > + if (fanspeed_err_cnt == ERR_CNT_THRESHOLD) { > + /* > + * We detected fan speed errors 6 times with 30 seconds. > + * Fix the error by readjusting duty cycle and reset > + * our counters/logic. > + */ > + fan_dc = pc->set_dc * delta_dc >> (FRAC_BITS + 2); > + fan_dc = min_t(u32, fan_dc, MAX_DUTY_CYCLE); > + pwm_update_dc(pc, fan_dc); > + fanspeed_err_cnt = 0; > + delta_dc = 0; > + time_window = 0; > + } > + > +restart_work: > + /* > + * Fan speed doesn't need continous tracking. Schedule this work > + * every two seconds so it doesn't steal too much cpu cycles. > + */ > + schedule_delayed_work(&pc->work, msecs_to_jiffies(TWO_SECONDS)); > +} > + > +static void pwm_init(struct intel_pwm_chip *pc) > +{ > + struct device *dev = pc->chip.dev; > + struct regmap *regmap = pc->regmap; > + u32 max_rpm, fan_wire, tach_plus, con0_val, con0_mask; > + > + if (device_property_read_u32(dev, "intel,fan-wire", &fan_wire)) > + fan_wire = 2; /* default is 2 wire mode */ > + > + con0_val = FIELD_PREP(PWM_FAN_PWM_DIS_MSK, PWM_FAN_PWM_DIS_DIS); > + con0_mask = PWM_FAN_PWM_DIS_MSK | PWM_FAN_MODE_MSK; > + > + switch (fan_wire) { > + case 2 ... 3: > + con0_val |= FIELD_PREP(PWM_FAN_MODE_MSK, PWM_FAN_MODE_2WIRE); > + break; This can be dropped as it matches the default case. > + case 4: > + con0_val |= FIELD_PREP(PWM_FAN_MODE_MSK, PWM_FAN_MODE_4WIRE) | > + FIELD_PREP(PWM_TACH_EN_MSK, PWM_TACH_EN_EN); > + con0_mask |= PWM_TACH_EN_MSK | PWM_TACH_PLUS_MSK; > + pc->tach_en = 1; > + break; > + default: > + /* default is 2wire mode */ > + con0_val |= FIELD_PREP(PWM_FAN_MODE_MSK, PWM_FAN_MODE_2WIRE); > + break; > + } > + > + if (pc->tach_en) { > + if (device_property_read_u32(dev, "intel,tach-plus", > + &tach_plus)) > + tach_plus = 2; > + > + switch (tach_plus) { > + case 2: > + con0_val |= FIELD_PREP(PWM_TACH_PLUS_MSK, > + PWM_TACH_PLUS_2); > + break; > + case 4: > + con0_val |= FIELD_PREP(PWM_TACH_PLUS_MSK, > + PWM_TACH_PLUS_4); > + break; > + default: > + con0_val |= FIELD_PREP(PWM_TACH_PLUS_MSK, > + PWM_TACH_PLUS_2); > + break; Similarily here, case 2: and default are identical. > + } > + > + if (device_property_read_u32(dev, "intel,max-rpm", &max_rpm)) > + max_rpm = DFAULT_RPM; > + > + max_rpm = min_t(u32, max_rpm, MAX_RPM); > + if (max_rpm == 0) > + max_rpm = DFAULT_RPM; > + > + pc->max_rpm = max_rpm; > + INIT_DEFERRABLE_WORK(&pc->work, tach_work); > + regmap_update_bits(regmap, PWM_FAN_CON1, > + PWM_FAN_MAX_RPM_MSK, max_rpm); > + } > + > + regmap_update_bits(regmap, PWM_FAN_CON0, con0_mask, con0_val); > +} > + > +static const struct regmap_config pwm_regmap_config = { > + .reg_bits = 32, > + .reg_stride = 4, > + .val_bits = 32, > +}; > + > +static int intel_pwm_probe(struct platform_device *pdev) > +{ > + struct intel_pwm_chip *pc; > + struct device *dev = &pdev->dev; > + void __iomem *io_base; > + int ret; > + > + pc = devm_kzalloc(dev, sizeof(*pc), GFP_KERNEL); > + if (!pc) > + return -ENOMEM; > + > + io_base = devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(io_base)) error message here? > + return PTR_ERR(io_base); > + > + pc->regmap = devm_regmap_init_mmio(dev, io_base, &pwm_regmap_config); > + if (IS_ERR(pc->regmap)) { > + ret = PTR_ERR(pc->regmap); > + dev_err(dev, "failed to init register map: %d\n", ret); Better use %pe for the error code to get a more descriptive message. > + return ret; > + } > + > + pc->clk = devm_clk_get(dev, NULL); > + if (IS_ERR(pc->clk)) { > + ret = PTR_ERR(pc->clk); > + dev_err(dev, "failed to get clock: %d\n", ret); > + return ret; > + } > + > + pc->rst = devm_reset_control_get(dev, NULL); > + if (IS_ERR(pc->rst)) { > + ret = PTR_ERR(pc->rst); > + dev_err(dev, "failed to get reset control: %d\n", ret); > + return ret; > + } > + > + ret = clk_prepare_enable(pc->clk); > + if (ret) { > + dev_err(dev, "failed to enable clock\n"); > + return ret; > + } > + > + reset_control_deassert(pc->rst); return value check is missing. > + pc->chip.dev = dev; > + pc->chip.ops = &intel_pwm_ops; > + pc->chip.npwm = 1; > + > + pwm_init(pc); > + > + ret = pwmchip_add(&pc->chip); > + if (ret < 0) { > + dev_err(dev, "failed to add PWM chip: %d\n", ret); > + clk_disable_unprepare(pc->clk); > + return ret; > + } > + > + platform_set_drvdata(pdev, pc); > + return 0; > +} > + > +static int intel_pwm_remove(struct platform_device *pdev) > +{ > + struct intel_pwm_chip *pc = platform_get_drvdata(pdev); > + int ret; > + > + if (pc->tach_en) > + cancel_delayed_work_sync(&pc->work); > + > + ret = pwmchip_remove(&pc->chip); > + if (ret < 0) > + return ret; > + > + reset_control_assert(pc->rst); > + > + clk_disable_unprepare(pc->clk); In .probe() you first release reset and then enable the clock. It's good style to do it the other way round in .remove(). Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ | ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v1 2/2] Add PWM driver for LGM 2020-05-22 8:56 ` Uwe Kleine-König @ 2020-05-22 9:18 ` Andy Shevchenko 2020-05-22 9:32 ` Uwe Kleine-König 2020-05-27 6:28 ` Tanwar, Rahul 1 sibling, 1 reply; 10+ messages in thread From: Andy Shevchenko @ 2020-05-22 9:18 UTC (permalink / raw) To: Uwe Kleine-König Cc: Rahul Tanwar, thierry.reding, p.zabel, linux-pwm, robh+dt, linux-kernel, devicetree, songjun.Wu, cheol.yong.kim, qi-ming.wu On Fri, May 22, 2020 at 10:56:13AM +0200, Uwe Kleine-König wrote: > On Fri, May 22, 2020 at 03:41:59PM +0800, Rahul Tanwar wrote: > > + io_base = devm_platform_ioremap_resource(pdev, 0); > > + if (IS_ERR(io_base)) > > error message here? platform core provides it. No need to duplicate (esp. taking into consideration that it can issue IIRC three different error messages depending on actual error). > > > + return PTR_ERR(io_base); -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v1 2/2] Add PWM driver for LGM 2020-05-22 9:18 ` Andy Shevchenko @ 2020-05-22 9:32 ` Uwe Kleine-König 0 siblings, 0 replies; 10+ messages in thread From: Uwe Kleine-König @ 2020-05-22 9:32 UTC (permalink / raw) To: Andy Shevchenko Cc: Rahul Tanwar, thierry.reding, p.zabel, linux-pwm, robh+dt, linux-kernel, devicetree, songjun.Wu, cheol.yong.kim, qi-ming.wu Hello Andy, On Fri, May 22, 2020 at 12:18:24PM +0300, Andy Shevchenko wrote: > On Fri, May 22, 2020 at 10:56:13AM +0200, Uwe Kleine-König wrote: > > On Fri, May 22, 2020 at 03:41:59PM +0800, Rahul Tanwar wrote: > > > > + io_base = devm_platform_ioremap_resource(pdev, 0); > > > + if (IS_ERR(io_base)) > > > > error message here? > > platform core provides it. No need to duplicate (esp. taking into consideration > that it can issue IIRC three different error messages depending on actual error). Ah, missed that. Indeed that's fine as is in the patch. Thanks Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ | ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v1 2/2] Add PWM driver for LGM 2020-05-22 8:56 ` Uwe Kleine-König 2020-05-22 9:18 ` Andy Shevchenko @ 2020-05-27 6:28 ` Tanwar, Rahul 2020-05-27 9:15 ` Andy Shevchenko 1 sibling, 1 reply; 10+ messages in thread From: Tanwar, Rahul @ 2020-05-27 6:28 UTC (permalink / raw) To: Uwe Kleine-König Cc: thierry.reding, p.zabel, linux-pwm, robh+dt, linux-kernel, devicetree, andriy.shevchenko, songjun.Wu, cheol.yong.kim, qi-ming.wu Hi Uwe, Thanks for review. On 22/5/2020 4:56 pm, Uwe Kleine-König wrote: > Hello, > > On Fri, May 22, 2020 at 03:41:59PM +0800, Rahul Tanwar wrote: >> Add PWM controller driver for Intel's Lightning Mountain(LGM) SoC. >> >> Signed-off-by: Rahul Tanwar <rahul.tanwar@linux.intel.com> >> --- >> drivers/pwm/Kconfig | 9 ++ >> drivers/pwm/Makefile | 1 + >> drivers/pwm/pwm-intel-lgm.c | 356 ++++++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 366 insertions(+) >> create mode 100644 drivers/pwm/pwm-intel-lgm.c >> >> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig >> index eebbc917ac97..a582214f50b2 100644 >> --- a/drivers/pwm/Kconfig >> +++ b/drivers/pwm/Kconfig >> @@ -232,6 +232,15 @@ config PWM_IMX_TPM >> To compile this driver as a module, choose M here: the module >> will be called pwm-imx-tpm. >> >> +config PWM_INTEL_LGM >> + tristate "Intel LGM PWM support" >> + depends on X86 || COMPILE_TEST >> + help >> + Generic PWM framework driver for LGM SoC. [...] >> +}; >> + >> +static void tach_work(struct work_struct *work) >> +{ >> + struct intel_pwm_chip *pc = container_of(work, struct intel_pwm_chip, >> + work.work); >> + struct regmap *regmap = pc->regmap; >> + u32 fan_tach, fan_dc, val; >> + s32 diff; >> + static u32 fanspeed_err_cnt, time_window, delta_dc; >> + >> + /* >> + * Fan speed is tracked by reading the active duty cycle of PWM output >> + * from the active duty cycle register. Some variance in the duty cycle >> + * register value is expected. So we set a time window of 30 seconds and >> + * if we detect inaccurate fan speed 6 times within 30 seconds then we >> + * mark it as fan speed problem and fix it by readjusting the duty cycle. >> + */ > I'm a unhappy to have this in the PWM driver. The PWM driver is supposed > to be generic and I think this belongs into a dedicated driver. Well noted about all other review concerns. I will rework the driver in v2. However, i am not very sure about the above point - of having a separate dedicated driver for tach_work because its logic is tightly coupled with this driver. Regards, Rahul ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v1 2/2] Add PWM driver for LGM 2020-05-27 6:28 ` Tanwar, Rahul @ 2020-05-27 9:15 ` Andy Shevchenko 2020-05-28 5:45 ` Tanwar, Rahul 0 siblings, 1 reply; 10+ messages in thread From: Andy Shevchenko @ 2020-05-27 9:15 UTC (permalink / raw) To: Tanwar, Rahul Cc: Uwe Kleine-König, thierry.reding, p.zabel, linux-pwm, robh+dt, linux-kernel, devicetree, songjun.Wu, cheol.yong.kim, qi-ming.wu On Wed, May 27, 2020 at 02:28:53PM +0800, Tanwar, Rahul wrote: > On 22/5/2020 4:56 pm, Uwe Kleine-König wrote: > > On Fri, May 22, 2020 at 03:41:59PM +0800, Rahul Tanwar wrote: ... > > I'm a unhappy to have this in the PWM driver. The PWM driver is supposed > > to be generic and I think this belongs into a dedicated driver. > > Well noted about all other review concerns. I will rework the driver in v2. > However, i am not very sure about the above point - of having a separate > dedicated driver for tach_work because its logic is tightly coupled with > this driver. Actually I agree with Uwe. Here is layering violation, i.e. provider and consumer in the same pot. It's not good from design perspective. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v1 2/2] Add PWM driver for LGM 2020-05-27 9:15 ` Andy Shevchenko @ 2020-05-28 5:45 ` Tanwar, Rahul 0 siblings, 0 replies; 10+ messages in thread From: Tanwar, Rahul @ 2020-05-28 5:45 UTC (permalink / raw) To: Andy Shevchenko Cc: Uwe Kleine-König, thierry.reding, p.zabel, linux-pwm, robh+dt, linux-kernel, devicetree, songjun.Wu, cheol.yong.kim, qi-ming.wu On 27/5/2020 5:15 pm, Andy Shevchenko wrote: > On Wed, May 27, 2020 at 02:28:53PM +0800, Tanwar, Rahul wrote: >> On 22/5/2020 4:56 pm, Uwe Kleine-König wrote: >>> On Fri, May 22, 2020 at 03:41:59PM +0800, Rahul Tanwar wrote: > ... > >>> I'm a unhappy to have this in the PWM driver. The PWM driver is supposed >>> to be generic and I think this belongs into a dedicated driver. >> Well noted about all other review concerns. I will rework the driver in v2. >> However, i am not very sure about the above point - of having a separate >> dedicated driver for tach_work because its logic is tightly coupled with >> this driver. > Actually I agree with Uwe. > Here is layering violation, i.e. provider and consumer in the same pot. It's > not good from design perspective. > Just to clarify, the PWM controller in our SoC serves just one purpose which is to control the fan. Its actually named as PWM Fan Controller. There is no other generic usage or any other consumer for this PWM driver. So separating out this part seems redundant to me. Also, if we separate it out as a dedicated driver, this will endup as a very small daemon which is going to be very hard to justify while upstreaming.. Regards, Rahul ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-05-28 23:52 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-05-22 7:41 [PATCH v1 0/2] pwm: intel: Add PWM driver for new SoC Rahul Tanwar 2020-05-22 7:41 ` [PATCH v1 1/2] Add YAML schema for a new PWM driver Rahul Tanwar 2020-05-28 23:31 ` Rob Herring 2020-05-22 7:41 ` [PATCH v1 2/2] Add PWM driver for LGM Rahul Tanwar 2020-05-22 8:56 ` Uwe Kleine-König 2020-05-22 9:18 ` Andy Shevchenko 2020-05-22 9:32 ` Uwe Kleine-König 2020-05-27 6:28 ` Tanwar, Rahul 2020-05-27 9:15 ` Andy Shevchenko 2020-05-28 5:45 ` Tanwar, Rahul
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).