pwm: intel: Add PWM driver for a new SoC
Subsystems (2)
v3 cover pwm: intel: Add PWM driver for a new SoC
Patch 1 adds dt binding document in YAML format. Patch 2 add PWM fan controller driver for LGM SoC.
Patch series is baselined on linux 5.8-rc2.
v3:
- Address below review concerns from Uwe Kleine-K�nig.
* Remove fan rpm calibration task from the driver.
* Modify apply op as per the review feedback.
* Add roundup & round down where necessary.
* Address other misc code quality related review concerns.
* Use devm_reset_control_get_exclusive(). (Philipp Zabel)
* Improve dt binding document.
v2:
- Address below review concerns from Uwe Kleine-K�nig.
* Add notes and limitations about PWM HW.
* Rename all functions and structure to lgm_pwm_*
* Readjust space aligninment in structure fields to single space.
* Switch to using apply instead of config/enable/disable.
* Address other code quality related concerns.
* Rebase to 5.8-rc1.
- Address review concerns in dt binding YAML from Rob Herring.
v1:
- Initial version.
Rahul Tanwar (2):
Add DT bindings YAML schema for PWM fan controller of LGM SoC Add PWM fan controller driver for LGM SoC
.../devicetree/bindings/pwm/intel,lgm-pwm.yaml | 51 ++++ drivers/pwm/Kconfig | 9 + drivers/pwm/Makefile | 1 + drivers/pwm/pwm-intel-lgm.c | 265 +++++++++++++++++++++ 4 files changed, 326 insertions(+) create mode 100644 Documentation/devicetree/bindings/pwm/intel,lgm-pwm.yaml create mode 100644 drivers/pwm/pwm-intel-lgm.c
Signature · 1 line
2.11.0
PATCH v3 1/2 Add DT bindings YAML schema for PWM fan controller of LGM SoC
Intel's LGM(Lightning Mountain) SoC contains a PWM fan controller which is only used to control the fan attached to the system. This PWM controller does not have any other consumer other than fan. Add DT bindings documentation for this PWM fan controller.
--- .../devicetree/bindings/pwm/intel,lgm-pwm.yaml | 51 ++++++++++++++++++++++ 1 file changed, 51 insertions(+) create mode 100644 Documentation/devicetree/bindings/pwm/intel,lgm-pwm.yaml
Documentation/devicetree/bindings/pwm/intel,lgm-pwm.yaml new file +51 -0
@@ -0,0 +1,51 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/pwm/intel,lgm-pwm.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: LGM SoC PWM fan controller + +maintainers: + - Rahul Tanwar <[email protected]> + +properties: + compatible: + const: intel,lgm-pwm + + reg: + maxItems: 1 + + clocks: + maxItems: 1 + + resets: + maxItems: 1 + + intel,fan-wire: + $ref: '/schemas/types.yaml#/definitions/uint32' + description: Specifies fan mode. Default when unspecified is 2. + + intel,max-rpm: + $ref: '/schemas/types.yaml#/definitions/uint32' + description: + Specifies maximum RPM of fan attached to the system. + Default when unspecified is 4000. + +required: + - compatible + - reg + - 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>; + };
Signature · 1 line
2.11.0
Rob Herring
maintainerRe: [PATCH v3 1/2] Add DT bindings YAML schema for PWM fan controller of LGM SoC
Quoted 11 lines · Rahul Tanwar
Intel's LGM(Lightning Mountain) SoC contains a PWM fan controller which is only used to control the fan attached to the system. This PWM controller does not have any other consumer other than fan. Add DT bindings documentation for this PWM fan controller.
Signed-off-by Rahul Tanwar <[email protected]>--- .../devicetree/bindings/pwm/intel,lgm-pwm.yaml | 51 ++++++++++++++++++++++ 1 file changed, 51 insertions(+) create mode 100644 Documentation/devicetree/bindings/pwm/intel,lgm-pwm.yaml
My bot found errors running 'make dt_binding_check' on your patch:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/pwm/intel,lgm-pwm.example.dt.yaml: pwm@e0d00000: '#pwm-cells' does not match any of the regexes: 'pinctrl-[0-9]+' /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/reset/intel,rcu-gw.example.dt.yaml: pwm@e0d00000: '#pwm-cells' does not match any of the regexes: 'pinctrl-[0-9]+'
See https://patchwork.ozlabs.org/patch/1318996
If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure dt-schema is up to date:
pip3 install git+https://github.com/devicetree-org/dt-schema.git@master --upgrade
Please check and re-submit.
PATCH v3 2/2 Add PWM fan controller driver for LGM SoC
In reply to Rahul TanwarIntel Lightning Mountain(LGM) SoC contains a PWM fan controller. This PWM controller does not have any other consumer, it is a dedicated PWM controller for fan attached to the system. Add driver for this PWM fan controller.
--- drivers/pwm/Kconfig | 9 ++ drivers/pwm/Makefile | 1 + drivers/pwm/pwm-intel-lgm.c | 265 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 275 insertions(+) create mode 100644 drivers/pwm/pwm-intel-lgm.c
drivers/pwm/Kconfig +9 -0
@@ -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 fan controller driver for LGM SoC. + + To compile this driver as a module, choose M here: the module + will be called pwm-intel-lgm. + config PWM_IQS620A tristate "Azoteq IQS620A PWM support" depends on MFD_IQS62X || COMPILE_TEST
drivers/pwm/Makefile +1 -0
@@ -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_IQS620A) += pwm-iqs620a.o obj-$(CONFIG_PWM_JZ4740) += pwm-jz4740.o obj-$(CONFIG_PWM_LP3943) += pwm-lp3943.o
drivers/pwm/pwm-intel-lgm.c new file +265 -0
265 lines not on this page · raw message
Signature · 1 line
2.11.0
Re: [PATCH v3 2/2] Add PWM fan controller driver for LGM SoC
Hello Rahul,
Quoted 10 lines · Rahul Tanwar
drivers/pwm/pwm-intel-lgm.c new file +4
-0@@ -0,0 +1,265 @@ +// SPDX-License-Identifier: GPL-2.0 +/* +* Copyright (C) 2020 Intel Corporation. +*
drivers/pwm/pwm-intel-lgm.c line 14
Quoted 10 lines
drivers/pwm/pwm-intel-lgm.c new file +10
-0+* Notes & Limitations: +* - The hardware supports fixed period which is dependent on 2/3 or 4 +* wire fan mode. +* - Supports normal polarity. Does not support changing polarity. +* - When PWM is disabled, output of PWM will become 0(inactive). It doesn't +* keep track of running period. +* - When duty cycle is changed, PWM output may be a mix of previous setting +* and new setting for the first period. From second period, the output is +* based on new setting. +* - Supports 100% duty cycle.
This would be worth mentioning if it didn't support that. IMHO you can drop this one.
Quoted 15 lines
drivers/pwm/pwm-intel-lgm.c new file +15
-0+* - It is a dedicated PWM fan controller. There are no other consumers for +* this PWM controller. +*/ +#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)
drivers/pwm/pwm-intel-lgm.c line 39
Quoted 10 lines
drivers/pwm/pwm-intel-lgm.c new file +10
-0+#define PWM_FAN_MODE_2WIRE 0x0 +#define PWM_FAN_MODE_4WIRE 0x1 +#define PWM_FAN_MODE_MSK BIT(1) +#define PWM_FAN_DC_MSK GENMASK(23, 16) + +#define PWM_FAN_CON1 0x4 +#define PWM_FAN_MAX_RPM_MSK GENMASK(15, 0) + +#define MAX_RPM (BIT(16) - 1) +#define DFAULT_RPM 4000
DEFAULT_RPM?
drivers/pwm/pwm-intel-lgm.c line 45
Quoted 6 lines
drivers/pwm/pwm-intel-lgm.c new file +6
-0+#define MAX_DUTY_CYCLE (BIT(8) - 1) + +#define DC_BITS 8 + +#define PERIOD_2WIRE_NSECS 40000000 +#define PERIOD_4WIRE_NSECS 40000
Please add a common prefix to these definitions.
Quoted 16 lines
drivers/pwm/pwm-intel-lgm.c new file +16
-0+#define LGM_PWM_DIV_ROUND_DOWN (n, d) (((n) + ((d) / 2) - 1) / (d)) + +struct lgm_pwm_chip { +struct pwm_chip chip; +struct regmap *regmap; +struct clk *clk; +struct reset_control *rst; +u32 period; +}; + +static inline struct lgm_pwm_chip *to_lgm_pwm_chip (struct pwm_chip *chip) +{ + return container_of(chip, struct lgm_pwm_chip, chip); +} + +static int lgm_pwm_enable (struct pwm_chip *chip,bool enable)
drivers/pwm/pwm-intel-lgm.c line 72
Quoted 10 lines
drivers/pwm/pwm-intel-lgm.c new file +10
-0+{ +struct lgm_pwm_chip *pc =to_lgm_pwm_chip (chip); +struct regmap *regmap = pc->regmap; + +if (enable) +regmap_update_bits (regmap,PWM_FAN_CON0 , +PWM_FAN_EN_MSK ,PWM_FAN_EN_EN ); +else +regmap_update_bits (regmap,PWM_FAN_CON0 , +PWM_FAN_EN_MSK ,PWM_FAN_EN_DIS );
Is it easier to understand what happens here if you write this as:
+ regmap_update_bits(regmap, PWM_FAN_CON0, PWM_FAN_EN_MSK,
+ enable ? PWM_FAN_EN_EN : PWM_FAN_EN_DIS);
? (This is what I'd prefer, but maybe this is subjective?)
drivers/pwm/pwm-intel-lgm.c line 84
Quoted 12 lines
drivers/pwm/pwm-intel-lgm.c new file +12
-0+ +return 0 ; +} + +static int lgm_pwm_apply (struct pwm_chip *chip,struct pwm_device *pwm, +const struct pwm_state *state) +{ +struct lgm_pwm_chip *pc =to_lgm_pwm_chip (chip); +u32 duty_cycle, val; +unsigned int period; + + period = min_t(unsigned int, state->period, pc->period);
Given that state->period will most probably change type (from unsigned int to u64) soon, it would be nice to use this already here.
drivers/pwm/pwm-intel-lgm.c line 92
Quoted 7 lines
drivers/pwm/pwm-intel-lgm.c new file +7
-0+if (state->polarity !=PWM_POLARITY_NORMAL || + period < pc->period) +return -EINVAL ; + + duty_cycle =min_t (u32, state->duty_cycle, period); + +/* reg_value = duty_ns * MAX_REG_VAL(0xff) / period_ns */
s/MAX_REG_VAL/MAX_DUTY_CYCLE/
drivers/pwm/pwm-intel-lgm.c line 93
Quoted 1 line
drivers/pwm/pwm-intel-lgm.c new file +1
-0+ val =LGM_PWM_DIV_ROUND_DOWN (duty_cycle <<DC_BITS , period);
The rounding is wrong here, you have to round down. I think you need to do:
val = duty_cycle * MAX_DUTY_CYCLE / period;
drivers/pwm/pwm-intel-lgm.c line 94
Quoted 1 line
drivers/pwm/pwm-intel-lgm.c new file +1
-0+ val =min_t (u32, val,MAX_DUTY_CYCLE );
Then as duty_cycle <= period this is a noop and can be dropped.
drivers/pwm/pwm-intel-lgm.c line 101
Quoted 6 lines
drivers/pwm/pwm-intel-lgm.c new file +6
-0+regmap_update_bits (pc->regmap,PWM_FAN_CON0 ,PWM_FAN_DC_MSK , +FIELD_PREP (PWM_FAN_DC_MSK , val)); + +if (state->enabled !=regmap_test_bits (pc->regmap,PWM_FAN_CON0 , +PWM_FAN_EN_EN )) +lgm_pwm_enable (chip, state->enabled);
Here a spike can happen that you can prevent:
pwm_apply_state(pwm, { .enabled = 1, .duty_cycle = 0ms, .period = 40ms })
pwm_apply_state(pwm, { .enabled = 0, .duty_cycle = 40ms, .period = 40ms })As apply first configures the duty_cycle, the output goes high before disabling. So better do something like:
if (!state->enabled) {
lgm_pwm_enable(chip, 0);
return;
}
configure_duty_cycle();
if ( state->enabled)
lgm_pwm_enable(chip, 1);Quoted 7 lines
drivers/pwm/pwm-intel-lgm.c new file +7
-0+return 0 ; +} + +static void lgm_pwm_get_state (struct pwm_chip *chip,struct pwm_device *pwm, +struct pwm_state *state) +{ +struct lgm_pwm_chip *pc =to_lgm_pwm_chip (chip);
drivers/pwm/pwm-intel-lgm.c line 119
Quoted 10 lines
drivers/pwm/pwm-intel-lgm.c new file +10
-0+u32 duty, val; + + state->enabled =regmap_test_bits (pc->regmap,PWM_FAN_CON0 , +PWM_FAN_EN_EN ); + state->polarity =PWM_POLARITY_NORMAL ; + state->period = pc->period;/* fixed period */ + +regmap_read (pc->regmap,PWM_FAN_CON0 , &val); + duty =FIELD_GET (PWM_FAN_DC_MSK , val); + state->duty_cycle = duty * pc->period >>DC_BITS ;
If PWM_FAN_DC = 255 means 100% the calculation is wrong. You said in the v2 thread: 0 = disabled (0%) and 255 = 100%, so we need here:
state->duty_cycle = DIV_ROUND_UP(duty * pc->period, 255);
.
Quoted 26 lines
drivers/pwm/pwm-intel-lgm.c new file +26
-0+ state->duty_cycle =roundup_pow_of_two (state->duty_cycle); +} +[..] +static int lgm_pwm_probe (struct platform_device *pdev) +{ +struct lgm_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, &lgm_pwm_regmap_config); +if (IS_ERR (pc->regmap)) { + ret =PTR_ERR (pc->regmap); +dev_err (dev,"failed to init register map: %pe\n" , pc->regmap); +return ret; + } + + pc->clk =devm_clk_get (dev,NULL );
drivers/pwm/pwm-intel-lgm.c line 201
Quoted 10 lines
drivers/pwm/pwm-intel-lgm.c new file +10
-0+if (IS_ERR (pc->clk)) { + ret =PTR_ERR (pc->clk); +dev_err (dev,"failed to get clock: %pe\n" , pc->clk); +return ret; + } + + pc->rst =devm_reset_control_get_exclusive (dev,NULL ); +if (IS_ERR (pc->rst)) { + ret =PTR_ERR (pc->rst); +dev_err (dev,"failed to get reset control: %pe\n" , pc->rst);
Please skip the error messages if ret = -EPROBE_DEFER.
drivers/pwm/pwm-intel-lgm.c line 214
Quoted 13 lines
drivers/pwm/pwm-intel-lgm.c new file +13
-0+return ret; + } + + ret =reset_control_deassert (pc->rst); +if (ret) { +dev_err (dev,"cannot deassert reset control: %pe\n" , +ERR_PTR (ret)); +return ret; + } + + ret =clk_prepare_enable (pc->clk); +if (ret) { +dev_err (dev,"failed to enable clock\n" );
reset_control_assert(pc->rst);
drivers/pwm/pwm-intel-lgm.c line 166
Quoted 2 lines
drivers/pwm/pwm-intel-lgm.c new file +2
-0+return ret; + }
Best regards
Uwe
Signature · 2 lines
Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
Re: [PATCH v3 2/2] Add PWM fan controller driver for LGM SoC
In reply to Rahul TanwarHello,
drivers/pwm/Kconfig config PWM_IMX_TPM · line 237
Quoted 11 lines · Rahul Tanwar
drivers/pwm/Kconfig +3
-0@@ -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
Another thing I just noticed: You're using regmap, so I think you should have
select REGMAP_MMIO
here.
drivers/pwm/Kconfig config PWM_IMX_TPM · line 243
Quoted 6 lines
drivers/pwm/Kconfig +6
-0+ help + Generic PWM fan controller driver for LGM SoC. + + To compile this driver as a module, choose M here: the module + will be called pwm-intel-lgm. +
Best regards
Uwe
Signature · 2 lines
Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |