Re: [PATCH v5 2/2] Add PWM fan controller driver for LGM SoC
Hello, On Mon, Jul 27, 2020 at 03:30:16PM +0800, Tanwar, Rahul wrote: > On 27/7/2020 3:01 pm, Uwe Kleine-König wrote: > > In v4 you had: > > > > if (state->polarity != PWM_POLARITY_NORMAL || > > state->period < pc->period) > > return -EINVAL; > > > > That's the right thing to do (even though I was unsettled at one point > > and wrote it was wrong). The check in v5 with state->period != > > pc->period is wrong. > > Does that mean we should allow state->period >= pc->period cases? Yes, the driver is supposed to implement the longest period not longer than the requested one. This implies everything >= pc->period is fine. > If the state->period is greater than HW supported pc->period and > if we allow it then the duty cycle will again be evaluated to be > incorrect/higher than requested duty cycle. Am i missing something > else? Thanks. Yes, similar as with period you're supposed to implement the longest duty cycle your hardware supports and that is not longer than the requested duty cycle. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | signature.asc Description: PGP signature
Re: [PATCH v5 2/2] Add PWM fan controller driver for LGM SoC
Hi Uwe,
On 27/7/2020 3:01 pm, Uwe Kleine-König wrote:
> On Mon, Jul 27, 2020 at 02:04:56PM +0800, Tanwar, Rahul wrote:
>> Hi Uwe,
>>
>> On 24/7/2020 12:15 am, Uwe Kleine-König wrote:
>>> Hello,
>>>
>>> On Thu, Jul 23, 2020 at 03:44:18PM +0800, Rahul Tanwar wrote:
+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;
+ int ret;
+
+ if (!state->enabled) {
+ ret = lgm_pwm_enable(chip, 0);
+ return ret;
+ }
+
+ /*
+ * HW only supports NORMAL polarity
+ * HW supports fixed period which can not be changed/configured by user
+ */
+ if (state->polarity != PWM_POLARITY_NORMAL ||
+ state->period != pc->period)
+ return -EINVAL;
>>> At least for state->polarity you have to check before state->enabled, as
>>> the expectation on
>>>
>>> .enabled = false
>>> .polarity = PWM_POLARITY_INVERSED
>>>
>>> is that the output becomes constant high. Also as confirmed at the end
>>> of v4, state->period < pc->period was the right check to do.
>> For below case:
>>
>> .enabled = false
>> .polarity = PWM_POLARITY_INVERSED
>>
>> Since our HW does not support inversed polarity, the output for above case
>> is expected to be constant low. And if we disable PWM before checking for
>> polarity, the output becomes constant low. The code just does that. Sorry,
>> i could not understand what is wrong with the code. It looks correct to me.
> As your hardware can only support normal polarity, the code must have:
>
> if (state->polarity != PWM_POLARITY_NORMAL)
> return -EINVAL;
>
> if (!state->enabled) {
> ret = lgm_pwm_enable(chip, 0);
> return ret;
> }
>
> That's what I meant writing: "At least for state->polarity you have to
> check before state->enabled".
Ok, i understand your point now.
>> Given the fact that we support fixed period, if we allow
>> state->period < pc->period case then the duty cycle will be evaluated as
>> higher than the requested one because the state->period is lesser than
>> the actual fixed period supported by the HW. Can you please elaborate
>> on why you think we should allow state->period < pc->period case?
> You should not allow it. In v4 you had:
>
> if (state->polarity != PWM_POLARITY_NORMAL ||
> state->period < pc->period)
> return -EINVAL;
>
> That's the right thing to do (even though I was unsettled at one point
> and wrote it was wrong). The check in v5 with state->period !=
> pc->period is wrong.
>
Does that mean we should allow state->period >= pc->period cases?
If the state->period is greater than HW supported pc->period and
if we allow it then the duty cycle will again be evaluated to be
incorrect/higher than requested duty cycle. Am i missing something
else? Thanks.
Regards,
Rahul
Re: [PATCH v5 2/2] Add PWM fan controller driver for LGM SoC
On Mon, Jul 27, 2020 at 02:04:56PM +0800, Tanwar, Rahul wrote:
>
> Hi Uwe,
>
> On 24/7/2020 12:15 am, Uwe Kleine-König wrote:
> > Hello,
> >
> > On Thu, Jul 23, 2020 at 03:44:18PM +0800, Rahul Tanwar wrote:
> >> +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;
> >> + int ret;
> >> +
> >> + if (!state->enabled) {
> >> + ret = lgm_pwm_enable(chip, 0);
> >> + return ret;
> >> + }
> >> +
> >> + /*
> >> + * HW only supports NORMAL polarity
> >> + * HW supports fixed period which can not be changed/configured by user
> >> + */
> >> + if (state->polarity != PWM_POLARITY_NORMAL ||
> >> + state->period != pc->period)
> >> + return -EINVAL;
> > At least for state->polarity you have to check before state->enabled, as
> > the expectation on
> >
> > .enabled = false
> > .polarity = PWM_POLARITY_INVERSED
> >
> > is that the output becomes constant high. Also as confirmed at the end
> > of v4, state->period < pc->period was the right check to do.
>
> For below case:
>
> .enabled = false
> .polarity = PWM_POLARITY_INVERSED
>
> Since our HW does not support inversed polarity, the output for above case
> is expected to be constant low. And if we disable PWM before checking for
> polarity, the output becomes constant low. The code just does that. Sorry,
> i could not understand what is wrong with the code. It looks correct to me.
As your hardware can only support normal polarity, the code must have:
if (state->polarity != PWM_POLARITY_NORMAL)
return -EINVAL;
if (!state->enabled) {
ret = lgm_pwm_enable(chip, 0);
return ret;
}
That's what I meant writing: "At least for state->polarity you have to
check before state->enabled".
> Given the fact that we support fixed period, if we allow
> state->period < pc->period case then the duty cycle will be evaluated as
> higher than the requested one because the state->period is lesser than
> the actual fixed period supported by the HW. Can you please elaborate
> on why you think we should allow state->period < pc->period case?
You should not allow it. In v4 you had:
if (state->polarity != PWM_POLARITY_NORMAL ||
state->period < pc->period)
return -EINVAL;
That's the right thing to do (even though I was unsettled at one point
and wrote it was wrong). The check in v5 with state->period !=
pc->period is wrong.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |
signature.asc
Description: PGP signature
Re: [PATCH v5 2/2] Add PWM fan controller driver for LGM SoC
Hi Uwe,
On 24/7/2020 12:15 am, Uwe Kleine-König wrote:
> Hello,
>
> On Thu, Jul 23, 2020 at 03:44:18PM +0800, Rahul Tanwar wrote:
>> +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;
>> +int ret;
>> +
>> +if (!state->enabled) {
>> +ret = lgm_pwm_enable(chip, 0);
>> +return ret;
>> +}
>> +
>> +/*
>> + * HW only supports NORMAL polarity
>> + * HW supports fixed period which can not be changed/configured by user
>> + */
>> +if (state->polarity != PWM_POLARITY_NORMAL ||
>> +state->period != pc->period)
>> +return -EINVAL;
> At least for state->polarity you have to check before state->enabled, as
> the expectation on
>
> .enabled = false
> .polarity = PWM_POLARITY_INVERSED
>
> is that the output becomes constant high. Also as confirmed at the end
> of v4, state->period < pc->period was the right check to do.
For below case:
.enabled = false
.polarity = PWM_POLARITY_INVERSED
Since our HW does not support inversed polarity, the output for above case
is expected to be constant low. And if we disable PWM before checking for
polarity, the output becomes constant low. The code just does that. Sorry,
i could not understand what is wrong with the code. It looks correct to me.
Given the fact that we support fixed period, if we allow
state->period < pc->period case then the duty cycle will be evaluated as
higher than the requested one because the state->period is lesser than
the actual fixed period supported by the HW. Can you please elaborate
on why you think we should allow state->period < pc->period case?
Thanks,
Regards,
Rahul
Re: [PATCH v5 2/2] Add PWM fan controller driver for LGM SoC
Hello,
On Thu, Jul 23, 2020 at 03:44:18PM +0800, Rahul Tanwar wrote:
> +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;
> + int ret;
> +
> + if (!state->enabled) {
> + ret = lgm_pwm_enable(chip, 0);
> + return ret;
> + }
> +
> + /*
> + * HW only supports NORMAL polarity
> + * HW supports fixed period which can not be changed/configured by user
> + */
> + if (state->polarity != PWM_POLARITY_NORMAL ||
> + state->period != pc->period)
> + return -EINVAL;
At least for state->polarity you have to check before state->enabled, as
the expectation on
.enabled = false
.polarity = PWM_POLARITY_INVERSED
is that the output becomes constant high. Also as confirmed at the end
of v4, state->period < pc->period was the right check to do.
> + duty_cycle = min_t(u64, state->duty_cycle, state->period);
> + /* reg_value = duty_ns * LGM_PWM_MAX_DUTY_CYCLE(0xff) / period_ns */
> + val = duty_cycle * LGM_PWM_MAX_DUTY_CYCLE / state->period;
The comment adds only little compared to the code line below. Please
drop it.
Dividing by state->period is wrong. I think you need pc->period here.
> + ret = regmap_update_bits(pc->regmap, LGM_PWM_FAN_CON0,
> LGM_PWM_FAN_DC_MSK,
> + FIELD_PREP(LGM_PWM_FAN_DC_MSK, val));
> + if (ret)
> + return ret;
> +
> + if (state->enabled)
> + ret = lgm_pwm_enable(chip, 1);
> +
> + return ret;
> +}
> +
> +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, LGM_PWM_FAN_CON0,
> + LGM_PWM_FAN_EN_EN);
> + state->polarity = PWM_POLARITY_NORMAL;
> + state->period = pc->period; /* fixed period */
> +
> + regmap_read(pc->regmap, LGM_PWM_FAN_CON0, );
> + duty = FIELD_GET(LGM_PWM_FAN_DC_MSK, val);
> + state->duty_cycle = DIV_ROUND_UP(duty * pc->period,
> + LGM_PWM_MAX_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", _wire))
> + fan_wire = 2; /* default is 2 wire mode */
> +
> + con0_mask = LGM_PWM_FAN_MODE_MSK;
> +
> + switch (fan_wire) {
> + case 4:
> + con0_val = FIELD_PREP(LGM_PWM_FAN_MODE_MSK,
> LGM_PWM_FAN_MODE_4WIRE);
> + pc->period = LGM_PWM_PERIOD_4WIRE_NSECS;
> + break;
> + default:
> + /* default is 2wire mode */
> + con0_val = FIELD_PREP(LGM_PWM_FAN_MODE_MSK,
> LGM_PWM_FAN_MODE_2WIRE);
> + pc->period = LGM_PWM_PERIOD_2WIRE_NSECS;
> + break;
> + }
> +
> + if (device_property_read_u32(dev, "intel,max-rpm", _rpm))
> + max_rpm = LGM_PWM_DEFAULT_RPM;
This property isn't in the binding!?
> + max_rpm = min_t(u32, max_rpm, LGM_PWM_MAX_RPM);
> + if (max_rpm == 0)
> + max_rpm = LGM_PWM_DEFAULT_RPM;
> +
> + regmap_update_bits(regmap, LGM_PWM_FAN_CON1, LGM_PWM_FAN_MAX_RPM_MSK,
> max_rpm);
> + regmap_update_bits(regmap, LGM_PWM_FAN_CON0, con0_mask, con0_val);
> +}
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |
signature.asc
Description: PGP signature
[PATCH v5 2/2] Add PWM fan controller driver for LGM SoC
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
---
drivers/pwm/Kconfig | 11 ++
drivers/pwm/Makefile| 1 +
drivers/pwm/pwm-intel-lgm.c | 269
3 files changed, 281 insertions(+)
create mode 100644 drivers/pwm/pwm-intel-lgm.c
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index cb8d739067d2..3486edab09c4 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -232,6 +232,17 @@ 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 OF && HAS_IOMEM
+ depends on X86 || COMPILE_TEST
+ select REGMAP_MMIO
+ 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 ..7c6c21512d53
--- /dev/null
+++ b/drivers/pwm/pwm-intel-lgm.c
@@ -0,0 +1,269 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2020 Intel Corporation.
+ *
+ * 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.
+ * - It is a dedicated PWM fan controller. There are no other consumers for
+ * this PWM controller.
+ */
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define LGM_PWM_FAN_CON0 0x0
+#define LGM_PWM_FAN_EN_EN BIT(0)
+#define LGM_PWM_FAN_EN_DIS 0x0
+#define LGM_PWM_FAN_EN_MSK BIT(0)
+#define LGM_PWM_FAN_MODE_2WIRE 0x0
+#define LGM_PWM_FAN_MODE_4WIRE 0x1
+#define LGM_PWM_FAN_MODE_MSK BIT(1)
+#define LGM_PWM_FAN_DC_MSK GENMASK(23, 16)
+
+#define LGM_PWM_FAN_CON1 0x4
+#define LGM_PWM_FAN_MAX_RPM_MSKGENMASK(15, 0)
+
+#define LGM_PWM_MAX_RPM(BIT(16) - 1)
+#define LGM_PWM_DEFAULT_RPM4000
+#define LGM_PWM_MAX_DUTY_CYCLE (BIT(8) - 1)
+
+#define LGM_PWM_DC_BITS8
+
+#define LGM_PWM_PERIOD_2WIRE_NSECS 4000
+#define LGM_PWM_PERIOD_4WIRE_NSECS 4
+
+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;
+
+ return regmap_update_bits(regmap, LGM_PWM_FAN_CON0, LGM_PWM_FAN_EN_MSK,
+ enable ? LGM_PWM_FAN_EN_EN :
LGM_PWM_FAN_EN_DIS);
+}
+
+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;
+ int ret;
+
+ if (!state->enabled) {
+ ret = lgm_pwm_enable(chip, 0);
+ return ret;
+ }
+
+ /*
+* HW only supports NORMAL polarity
+* HW supports fixed period which can not be changed/configured by user
+*/
+ if (state->polarity != PWM_POLARITY_NORMAL ||
+ state->period != pc->period)
+ return -EINVAL;
+
+ duty_cycle = min_t(u64, state->duty_cycle, state->period);
+ /* reg_value = duty_ns * LGM_PWM_MAX_DUTY_CYCLE(0xff) / period_ns */
+ val =
