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
--
2.11.0
Intel 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.
Signed-off-by: Rahul Tanwar <[email protected]>
---
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
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index cb8d739067d2..a3303e22d5fa 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 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
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index a59c710e98c7..db154a6b4f51 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_IQS620A) += pwm-iqs620a.o
obj-$(CONFIG_PWM_JZ4740) += pwm-jz4740.o
obj-$(CONFIG_PWM_LP3943) += pwm-lp3943.o
diff --git a/drivers/pwm/pwm-intel-lgm.c b/drivers/pwm/pwm-intel-lgm.c
new file mode 100644
index 000000000000..661fa7d9145d
--- /dev/null
+++ b/drivers/pwm/pwm-intel-lgm.c
@@ -0,0 +1,265 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2020 Intel Corporation.
+ *
+ * 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.
+ * - 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)
+#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
+#define MAX_DUTY_CYCLE (BIT(8) - 1)
+
+#define DC_BITS 8
+
+#define PERIOD_2WIRE_NSECS 40000000
+#define PERIOD_4WIRE_NSECS 40000
+
+#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)
+{
+ 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);
+
+ 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);
+
+ 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 */
+ val = LGM_PWM_DIV_ROUND_DOWN(duty_cycle << DC_BITS, period);
+ val = min_t(u32, val, MAX_DUTY_CYCLE);
+
+ 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);
+
+ 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);
+ 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;
+ state->duty_cycle = roundup_pow_of_two(state->duty_cycle);
+}
+
+static const struct pwm_ops lgm_pwm_ops = {
+ .get_state = lgm_pwm_get_state,
+ .apply = lgm_pwm_apply,
+ .owner = THIS_MODULE,
+};
+
+static void lgm_pwm_init(struct lgm_pwm_chip *pc)
+{
+ struct device *dev = pc->chip.dev;
+ struct regmap *regmap = pc->regmap;
+ u32 max_rpm, fan_wire, con0_val, con0_mask;
+
+ if (device_property_read_u32(dev, "intel,fan-wire", &fan_wire))
+ fan_wire = 2; /* default is 2 wire mode */
+
+ con0_mask = PWM_FAN_MODE_MSK;
+
+ switch (fan_wire) {
+ case 4:
+ con0_val = FIELD_PREP(PWM_FAN_MODE_MSK, PWM_FAN_MODE_4WIRE);
+ pc->period = PERIOD_4WIRE_NSECS;
+ break;
+ default:
+ /* default is 2wire mode */
+ con0_val = FIELD_PREP(PWM_FAN_MODE_MSK, PWM_FAN_MODE_2WIRE);
+ pc->period = PERIOD_2WIRE_NSECS;
+ 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;
+
+ 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 lgm_pwm_regmap_config = {
+ .reg_bits = 32,
+ .reg_stride = 4,
+ .val_bits = 32,
+};
+
+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);
+ 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);
+ 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");
+ return ret;
+ }
+
+ pc->chip.dev = dev;
+ pc->chip.ops = &lgm_pwm_ops;
+ pc->chip.npwm = 1;
+
+ lgm_pwm_init(pc);
+
+ ret = pwmchip_add(&pc->chip);
+ if (ret < 0) {
+ dev_err(dev, "failed to add PWM chip: %pe\n", ERR_PTR(ret));
+ clk_disable_unprepare(pc->clk);
+ reset_control_assert(pc->rst);
+ return ret;
+ }
+
+ platform_set_drvdata(pdev, pc);
+ return 0;
+}
+
+static int lgm_pwm_remove(struct platform_device *pdev)
+{
+ struct lgm_pwm_chip *pc = platform_get_drvdata(pdev);
+ int ret;
+
+ ret = pwmchip_remove(&pc->chip);
+ if (ret < 0)
+ return ret;
+
+ clk_disable_unprepare(pc->clk);
+ reset_control_assert(pc->rst);
+
+ return 0;
+}
+
+static const struct of_device_id lgm_pwm_of_match[] = {
+ { .compatible = "intel,lgm-pwm" },
+ { }
+};
+MODULE_DEVICE_TABLE(of, lgm_pwm_of_match);
+
+static struct platform_driver lgm_pwm_driver = {
+ .driver = {
+ .name = "intel-pwm",
+ .of_match_table = lgm_pwm_of_match,
+ },
+ .probe = lgm_pwm_probe,
+ .remove = lgm_pwm_remove,
+};
+module_platform_driver(lgm_pwm_driver);
--
2.11.0
Hello Rahul,
On Mon, Jun 29, 2020 at 05:03:47PM +0800, Rahul Tanwar wrote:
> diff --git a/drivers/pwm/pwm-intel-lgm.c b/drivers/pwm/pwm-intel-lgm.c
> new file mode 100644
> index 000000000000..661fa7d9145d
> --- /dev/null
> +++ b/drivers/pwm/pwm-intel-lgm.c
> @@ -0,0 +1,265 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2020 Intel Corporation.
> + *
> + * 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.
> + * - 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)
> +#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?
> +#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.
> +#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)
> +{
> + 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?)
> +
> + 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.
> + 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/
> + 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;
> + val = min_t(u32, val, MAX_DUTY_CYCLE);
Then as duty_cycle <= period this is a noop and can be dropped.
> + 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);
> + 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);
> + 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);
.
> + 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);
> + 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.
> + 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);
> + return ret;
> + }
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Hello,
On Mon, Jun 29, 2020 at 05:03:47PM +0800, Rahul Tanwar wrote:
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index cb8d739067d2..a3303e22d5fa 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
Another thing I just noticed: You're using regmap, so I think you should
have
select REGMAP_MMIO
here.
> + 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
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |