* [PATCH 0/4] Support pwm driver for aspeed ast26xx
@ 2021-04-12 9:54 ` Billy Tsai
0 siblings, 0 replies; 26+ messages in thread
From: Billy Tsai @ 2021-04-12 9:54 UTC (permalink / raw)
To: lee.jones, robh+dt, joel, andrew, thierry.reding,
u.kleine-koenig, p.zabel, billy_tasi, devicetree,
linux-arm-kernel, linux-aspeed, linux-kernel, linux-pwm
Cc: BMC-SW
The legacy driver of aspeed pwm is binding with tach controller and it
doesn't follow the pwm framworks usage. In addition, the pwm register
usage of the 6th generation of ast26xx has drastic change. So these
patch serials add the new aspeed pwm driver to fix up the problem above.
Billy Tsai (4):
dt-bindings: Add bindings for aspeed pwm-tach.
dt-bindings: Add bindings for aspeed pwm
pwm: Add Aspeed ast2600 PWM support
pwm: Add support for aspeed pwm controller
.../bindings/mfd/aspeed,ast2600-pwm-tach.yaml | 52 ++++
.../bindings/pwm/aspeed,ast2600-pwm.yaml | 47 +++
drivers/pwm/Kconfig | 6 +
drivers/pwm/Makefile | 1 +
drivers/pwm/pwm-aspeed-g6.c | 291 ++++++++++++++++++
5 files changed, 397 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mfd/aspeed,ast2600-pwm-tach.yaml
create mode 100644 Documentation/devicetree/bindings/pwm/aspeed,ast2600-pwm.yaml
create mode 100644 drivers/pwm/pwm-aspeed-g6.c
--
2.25.1
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 0/4] Support pwm driver for aspeed ast26xx
@ 2021-04-12 9:54 ` Billy Tsai
0 siblings, 0 replies; 26+ messages in thread
From: Billy Tsai @ 2021-04-12 9:54 UTC (permalink / raw)
To: lee.jones, robh+dt, joel, andrew, thierry.reding,
u.kleine-koenig, p.zabel, billy_tasi, devicetree,
linux-arm-kernel, linux-aspeed, linux-kernel, linux-pwm
Cc: BMC-SW
The legacy driver of aspeed pwm is binding with tach controller and it
doesn't follow the pwm framworks usage. In addition, the pwm register
usage of the 6th generation of ast26xx has drastic change. So these
patch serials add the new aspeed pwm driver to fix up the problem above.
Billy Tsai (4):
dt-bindings: Add bindings for aspeed pwm-tach.
dt-bindings: Add bindings for aspeed pwm
pwm: Add Aspeed ast2600 PWM support
pwm: Add support for aspeed pwm controller
.../bindings/mfd/aspeed,ast2600-pwm-tach.yaml | 52 ++++
.../bindings/pwm/aspeed,ast2600-pwm.yaml | 47 +++
drivers/pwm/Kconfig | 6 +
drivers/pwm/Makefile | 1 +
drivers/pwm/pwm-aspeed-g6.c | 291 ++++++++++++++++++
5 files changed, 397 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mfd/aspeed,ast2600-pwm-tach.yaml
create mode 100644 Documentation/devicetree/bindings/pwm/aspeed,ast2600-pwm.yaml
create mode 100644 drivers/pwm/pwm-aspeed-g6.c
--
2.25.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 1/4] dt-bindings: Add bindings for aspeed pwm-tach.
2021-04-12 9:54 ` Billy Tsai
@ 2021-04-12 9:54 ` Billy Tsai
-1 siblings, 0 replies; 26+ messages in thread
From: Billy Tsai @ 2021-04-12 9:54 UTC (permalink / raw)
To: lee.jones, robh+dt, joel, andrew, thierry.reding,
u.kleine-koenig, p.zabel, billy_tasi, devicetree,
linux-arm-kernel, linux-aspeed, linux-kernel, linux-pwm
Cc: BMC-SW
This patch adds device bindings for aspeed pwm-tach device which is a
multi-function device include pwn and tach function.
Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
---
.../bindings/mfd/aspeed,ast2600-pwm-tach.yaml | 52 +++++++++++++++++++
1 file changed, 52 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mfd/aspeed,ast2600-pwm-tach.yaml
diff --git a/Documentation/devicetree/bindings/mfd/aspeed,ast2600-pwm-tach.yaml b/Documentation/devicetree/bindings/mfd/aspeed,ast2600-pwm-tach.yaml
new file mode 100644
index 000000000000..73512ff71d23
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/aspeed,ast2600-pwm-tach.yaml
@@ -0,0 +1,52 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright (C) 2021 ASPEED, Inc.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mfd/aspeed,ast2600-pwn-tach.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: PWM Tach controller Device Tree Bindings
+
+description: |
+ The PWM Tach controller is represented as a multi-function device which includes:
+ PWM
+ Tach
+
+maintainers:
+ - Billy Tsai <billy_tasi@aspeedtech.com>
+
+properties:
+ compatible:
+ - items:
+ - enum:
+ - aspeed,ast2600-pwn-tach
+ - const: syscon
+ - const: simple-mfd
+ reg:
+ maxItems: 1
+
+required:
+ - compatible
+ - reg
+
+additionalProperties: false
+
+examples:
+ - |
+ pwm_tach: pwm_tach@1e610000 {
+ compatible = "aspeed,ast2600-pwm-tach", "simple-mfd", "syscon";
+ #address-cells = <1>;
+ #size-cells = <1>;
+ reg = <0x1e610000 0x100>;
+
+ pwm: pwm@0 {
+ compatible = "aspeed,ast2600-pwm";
+ #pwm-cells = <3>;
+ reg = <0x0 0x100>;
+ };
+
+ tach: tach@0 {
+ compatible = "aspeed,ast2600-tach";
+ reg = <0x0 0x100>;
+ };
+ };
--
2.25.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 1/4] dt-bindings: Add bindings for aspeed pwm-tach.
@ 2021-04-12 9:54 ` Billy Tsai
0 siblings, 0 replies; 26+ messages in thread
From: Billy Tsai @ 2021-04-12 9:54 UTC (permalink / raw)
To: lee.jones, robh+dt, joel, andrew, thierry.reding,
u.kleine-koenig, p.zabel, billy_tasi, devicetree,
linux-arm-kernel, linux-aspeed, linux-kernel, linux-pwm
Cc: BMC-SW
This patch adds device bindings for aspeed pwm-tach device which is a
multi-function device include pwn and tach function.
Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
---
.../bindings/mfd/aspeed,ast2600-pwm-tach.yaml | 52 +++++++++++++++++++
1 file changed, 52 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mfd/aspeed,ast2600-pwm-tach.yaml
diff --git a/Documentation/devicetree/bindings/mfd/aspeed,ast2600-pwm-tach.yaml b/Documentation/devicetree/bindings/mfd/aspeed,ast2600-pwm-tach.yaml
new file mode 100644
index 000000000000..73512ff71d23
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/aspeed,ast2600-pwm-tach.yaml
@@ -0,0 +1,52 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright (C) 2021 ASPEED, Inc.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mfd/aspeed,ast2600-pwn-tach.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: PWM Tach controller Device Tree Bindings
+
+description: |
+ The PWM Tach controller is represented as a multi-function device which includes:
+ PWM
+ Tach
+
+maintainers:
+ - Billy Tsai <billy_tasi@aspeedtech.com>
+
+properties:
+ compatible:
+ - items:
+ - enum:
+ - aspeed,ast2600-pwn-tach
+ - const: syscon
+ - const: simple-mfd
+ reg:
+ maxItems: 1
+
+required:
+ - compatible
+ - reg
+
+additionalProperties: false
+
+examples:
+ - |
+ pwm_tach: pwm_tach@1e610000 {
+ compatible = "aspeed,ast2600-pwm-tach", "simple-mfd", "syscon";
+ #address-cells = <1>;
+ #size-cells = <1>;
+ reg = <0x1e610000 0x100>;
+
+ pwm: pwm@0 {
+ compatible = "aspeed,ast2600-pwm";
+ #pwm-cells = <3>;
+ reg = <0x0 0x100>;
+ };
+
+ tach: tach@0 {
+ compatible = "aspeed,ast2600-tach";
+ reg = <0x0 0x100>;
+ };
+ };
--
2.25.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 2/4] dt-bindings: Add bindings for aspeed pwm
2021-04-12 9:54 ` Billy Tsai
@ 2021-04-12 9:54 ` Billy Tsai
-1 siblings, 0 replies; 26+ messages in thread
From: Billy Tsai @ 2021-04-12 9:54 UTC (permalink / raw)
To: lee.jones, robh+dt, joel, andrew, thierry.reding,
u.kleine-koenig, p.zabel, billy_tasi, devicetree,
linux-arm-kernel, linux-aspeed, linux-kernel, linux-pwm
Cc: BMC-SW
This patch adds device bindings for aspeed pwm device which should be
the sub-node of aspeed,ast2600-pwm-tach.
Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
---
.../bindings/pwm/aspeed,ast2600-pwm.yaml | 47 +++++++++++++++++++
1 file changed, 47 insertions(+)
create mode 100644 Documentation/devicetree/bindings/pwm/aspeed,ast2600-pwm.yaml
diff --git a/Documentation/devicetree/bindings/pwm/aspeed,ast2600-pwm.yaml b/Documentation/devicetree/bindings/pwm/aspeed,ast2600-pwm.yaml
new file mode 100644
index 000000000000..7a7ff0260d27
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/aspeed,ast2600-pwm.yaml
@@ -0,0 +1,47 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright (C) 2021 ASPEED, Inc.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pwm/aspeed,ast2600-pwm.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ASPEED AST2600 PWM controller
+
+maintainers:
+ - Billy Tsai <billy_tsai@aspeedtech.com>
+
+description: |
+ The ASPEED PWM controller can support upto 16 PWM outputs.
+
+properties:
+ compatible:
+ enum:
+ - aspeed,ast2600-pwm
+
+ "#pwm-cells":
+ const: 3
+
+pwm-cells:
+ - channel
+# period in terms of nanoseconds
+ - period
+# PWM_POLARITY_INVERTED
+ - flag
+
+additionalProperties: false
+
+examples:
+ - |
+ // The PWM should be a subnode of a "aspeed,ast2600-pwn-tach" compatible node.
+ pwm_tach@1e610000 {
+ compatible = "aspeed,ast2600-pwm-tach", "simple-mfd", "syscon";
+ #address-cells = <1>;
+ #size-cells = <1>;
+ reg = <0x1e610000 0x100>;
+ ranges = <0 0x1e610000 0x100>;
+ pwm: pwm@0 {
+ compatible = "aspeed,ast2600-pwm";
+ #pwm-cells = <3>;
+ reg = <0x0 0x100>;
+ };
+ };
--
2.25.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 2/4] dt-bindings: Add bindings for aspeed pwm
@ 2021-04-12 9:54 ` Billy Tsai
0 siblings, 0 replies; 26+ messages in thread
From: Billy Tsai @ 2021-04-12 9:54 UTC (permalink / raw)
To: lee.jones, robh+dt, joel, andrew, thierry.reding,
u.kleine-koenig, p.zabel, billy_tasi, devicetree,
linux-arm-kernel, linux-aspeed, linux-kernel, linux-pwm
Cc: BMC-SW
This patch adds device bindings for aspeed pwm device which should be
the sub-node of aspeed,ast2600-pwm-tach.
Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
---
.../bindings/pwm/aspeed,ast2600-pwm.yaml | 47 +++++++++++++++++++
1 file changed, 47 insertions(+)
create mode 100644 Documentation/devicetree/bindings/pwm/aspeed,ast2600-pwm.yaml
diff --git a/Documentation/devicetree/bindings/pwm/aspeed,ast2600-pwm.yaml b/Documentation/devicetree/bindings/pwm/aspeed,ast2600-pwm.yaml
new file mode 100644
index 000000000000..7a7ff0260d27
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/aspeed,ast2600-pwm.yaml
@@ -0,0 +1,47 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright (C) 2021 ASPEED, Inc.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pwm/aspeed,ast2600-pwm.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ASPEED AST2600 PWM controller
+
+maintainers:
+ - Billy Tsai <billy_tsai@aspeedtech.com>
+
+description: |
+ The ASPEED PWM controller can support upto 16 PWM outputs.
+
+properties:
+ compatible:
+ enum:
+ - aspeed,ast2600-pwm
+
+ "#pwm-cells":
+ const: 3
+
+pwm-cells:
+ - channel
+# period in terms of nanoseconds
+ - period
+# PWM_POLARITY_INVERTED
+ - flag
+
+additionalProperties: false
+
+examples:
+ - |
+ // The PWM should be a subnode of a "aspeed,ast2600-pwn-tach" compatible node.
+ pwm_tach@1e610000 {
+ compatible = "aspeed,ast2600-pwm-tach", "simple-mfd", "syscon";
+ #address-cells = <1>;
+ #size-cells = <1>;
+ reg = <0x1e610000 0x100>;
+ ranges = <0 0x1e610000 0x100>;
+ pwm: pwm@0 {
+ compatible = "aspeed,ast2600-pwm";
+ #pwm-cells = <3>;
+ reg = <0x0 0x100>;
+ };
+ };
--
2.25.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 3/4] pwm: Add Aspeed ast2600 PWM support
2021-04-12 9:54 ` Billy Tsai
@ 2021-04-12 9:54 ` Billy Tsai
-1 siblings, 0 replies; 26+ messages in thread
From: Billy Tsai @ 2021-04-12 9:54 UTC (permalink / raw)
To: lee.jones, robh+dt, joel, andrew, thierry.reding,
u.kleine-koenig, p.zabel, billy_tasi, devicetree,
linux-arm-kernel, linux-aspeed, linux-kernel, linux-pwm
Cc: BMC-SW
This patch add the support of PWM controller which can find at aspeed
ast2600 soc chip. This controller supoorts up to 16 channels.
Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
---
drivers/pwm/pwm-aspeed-g6.c | 291 ++++++++++++++++++++++++++++++++++++
1 file changed, 291 insertions(+)
create mode 100644 drivers/pwm/pwm-aspeed-g6.c
diff --git a/drivers/pwm/pwm-aspeed-g6.c b/drivers/pwm/pwm-aspeed-g6.c
new file mode 100644
index 000000000000..4bb4f97453c6
--- /dev/null
+++ b/drivers/pwm/pwm-aspeed-g6.c
@@ -0,0 +1,291 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) ASPEED Technology Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 or later as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/clk.h>
+#include <linux/errno.h>
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/of_platform.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/sysfs.h>
+#include <linux/reset.h>
+#include <linux/regmap.h>
+#include <linux/pwm.h>
+/* The channel number of Aspeed pwm controller */
+#define ASPEED_NR_PWMS 16
+/* PWM Control Register */
+#define ASPEED_PWM_CTRL_CH(ch) ((ch * 0x10) + 0x00)
+#define PWM_LOAD_SEL_AS_WDT BIT(19)
+#define LOAD_SEL_FALLING 0
+#define LOAD_SEL_RIGING 1
+#define PWM_DUTY_LOAD_AS_WDT_EN BIT(18)
+#define PWM_DUTY_SYNC_DIS BIT(17)
+#define PWM_CLK_ENABLE BIT(16)
+#define PWM_LEVEL_OUTPUT BIT(15)
+#define PWM_INVERSE BIT(14)
+#define PWM_OPEN_DRAIN_EN BIT(13)
+#define PWM_PIN_EN BIT(12)
+#define PWM_CLK_DIV_H_SHIFT 8
+#define PWM_CLK_DIV_H_MASK (0xf << PWM_CLK_DIV_H_SHIFT)
+#define PWM_CLK_DIV_L_SHIFT 0
+#define PWM_CLK_DIV_L_MASK (0xff << PWM_CLK_DIV_L_SHIFT)
+/* PWM Duty Cycle Register */
+#define ASPEED_PWM_DUTY_CYCLE_CH(ch) ((ch * 0x10) + 0x04)
+#define PWM_PERIOD_SHIFT (24)
+#define PWM_PERIOD_MASK (0xff << PWM_PERIOD_SHIFT)
+#define PWM_POINT_AS_WDT_SHIFT (16)
+#define PWM_POINT_AS_WDT_MASK (0xff << PWM_POINT_AS_WDT_SHIFT)
+#define PWM_FALLING_POINT_SHIFT (8)
+#define PWM_FALLING_POINT_MASK (0xffff << PWM_FALLING_POINT_SHIFT)
+#define PWM_RISING_POINT_SHIFT (0)
+#define PWM_RISING_POINT_MASK (0xffff << PWM_RISING_POINT_SHIFT)
+/* PWM default value */
+#define DEFAULT_PWM_PERIOD 0xff
+#define DEFAULT_TARGET_PWM_FREQ 25000
+#define DEFAULT_DUTY_PT 10
+#define DEFAULT_WDT_RELOAD_DUTY_PT 16
+
+struct aspeed_pwm_data {
+ struct pwm_chip chip;
+ struct regmap *regmap;
+ unsigned long clk_freq;
+ struct reset_control *reset;
+};
+/**
+ * struct aspeed_pwm - per-PWM driver data
+ * @freq: cached pwm freq
+ */
+struct aspeed_pwm {
+ u32 freq;
+};
+
+static void aspeed_set_pwm_channel_enable(struct regmap *regmap, u8 pwm_channel,
+ bool enable)
+{
+ regmap_update_bits(regmap, ASPEED_PWM_CTRL_CH(pwm_channel),
+ (PWM_CLK_ENABLE | PWM_PIN_EN),
+ enable ? (PWM_CLK_ENABLE | PWM_PIN_EN) : 0);
+}
+/*
+ * The PWM frequency = HCLK(200Mhz) / (clock division L bit *
+ * clock division H bit * (period bit + 1))
+ */
+static void aspeed_set_pwm_freq(struct aspeed_pwm_data *priv,
+ struct pwm_device *pwm, u32 freq)
+{
+ u32 target_div, cal_freq;
+ u32 tmp_div_h, tmp_div_l, diff, min_diff = INT_MAX;
+ u32 div_h = BIT(5) - 1, div_l = BIT(8) - 1;
+ u8 div_found;
+ u32 index = pwm->hwpwm;
+ struct aspeed_pwm *channel = pwm_get_chip_data(pwm);
+
+ cal_freq = priv->clk_freq / (DEFAULT_PWM_PERIOD + 1);
+ target_div = DIV_ROUND_UP(cal_freq, freq);
+ div_found = 0;
+ /* calculate for target frequence */
+ for (tmp_div_h = 0; tmp_div_h < 0x10; tmp_div_h++) {
+ tmp_div_l = target_div / BIT(tmp_div_h) - 1;
+
+ if (tmp_div_l < 0 || tmp_div_l > 255)
+ continue;
+
+ diff = freq - cal_freq / (BIT(tmp_div_h) * (tmp_div_l + 1));
+ if (abs(diff) < abs(min_diff)) {
+ min_diff = diff;
+ div_l = tmp_div_l;
+ div_h = tmp_div_h;
+ div_found = 1;
+ if (diff == 0)
+ break;
+ }
+ }
+ if (div_found == 0) {
+ pr_debug("target freq: %d too slow set minimal frequency\n",
+ freq);
+ }
+ channel->freq = cal_freq / (BIT(div_h) * (div_l + 1));
+ pr_debug("div h %x, l : %x pwm out clk %d\n", div_h, div_l,
+ channel->freq);
+ pr_debug("hclk %ld, target pwm freq %d, real pwm freq %d\n",
+ priv->clk_freq, freq, channel->freq);
+
+ regmap_update_bits(priv->regmap, ASPEED_PWM_CTRL_CH(index),
+ (PWM_CLK_DIV_H_MASK | PWM_CLK_DIV_L_MASK),
+ (div_h << PWM_CLK_DIV_H_SHIFT) |
+ (div_l << PWM_CLK_DIV_L_SHIFT));
+}
+
+static void aspeed_set_pwm_duty(struct aspeed_pwm_data *priv,
+ struct pwm_device *pwm, u32 duty_pt)
+{
+ u32 index = pwm->hwpwm;
+
+ if (duty_pt == 0) {
+ aspeed_set_pwm_channel_enable(priv->regmap, index, false);
+ } else {
+ regmap_update_bits(priv->regmap,
+ ASPEED_PWM_DUTY_CYCLE_CH(index),
+ PWM_FALLING_POINT_MASK,
+ duty_pt << PWM_FALLING_POINT_SHIFT);
+ aspeed_set_pwm_channel_enable(priv->regmap, index, true);
+ }
+}
+
+static void aspeed_set_pwm_polarity(struct aspeed_pwm_data *priv,
+ struct pwm_device *pwm, u8 polarity)
+{
+ u32 index = pwm->hwpwm;
+
+ regmap_update_bits(priv->regmap, ASPEED_PWM_CTRL_CH(index), PWM_INVERSE,
+ (polarity) ? PWM_INVERSE : 0);
+}
+
+static int aspeed_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+ struct device *dev = chip->dev;
+ struct aspeed_pwm_data *priv = dev_get_drvdata(dev);
+ struct aspeed_pwm *channel;
+ u32 index = pwm->hwpwm;
+ /*
+ * Fixed the period to the max value and rising point to 0
+ * for high resolution and \bsimplified frequency calculation.
+ */
+ regmap_update_bits(priv->regmap, ASPEED_PWM_DUTY_CYCLE_CH(index),
+ PWM_PERIOD_MASK,
+ DEFAULT_PWM_PERIOD << PWM_PERIOD_SHIFT);
+
+ regmap_update_bits(priv->regmap, ASPEED_PWM_DUTY_CYCLE_CH(index),
+ PWM_RISING_POINT_MASK, 0);
+
+ channel = devm_kzalloc(dev, sizeof(*channel), GFP_KERNEL);
+ if (!channel)
+ return -ENOMEM;
+
+ pwm_set_chip_data(pwm, channel);
+
+ return 0;
+}
+
+static void aspeed_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+ struct device *dev = chip->dev;
+ struct aspeed_pwm *channel = pwm_get_chip_data(pwm);
+
+ devm_kfree(dev, channel);
+}
+
+static int aspeed_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+ const struct pwm_state *state)
+{
+ struct device *dev = chip->dev;
+ struct aspeed_pwm_data *priv = dev_get_drvdata(dev);
+ struct pwm_state *cur_state = &pwm->state;
+ u32 freq = DIV_ROUND_UP_ULL(1000000000, state->period);
+ u32 duty_pt = DIV_ROUND_UP_ULL(
+ state->duty_cycle * (DEFAULT_PWM_PERIOD + 1), state->period);
+ dev_dbg(dev, "freq: %d, duty_pt: %d", freq, duty_pt);
+ if (state->enabled) {
+ aspeed_set_pwm_freq(priv, pwm, freq);
+ aspeed_set_pwm_duty(priv, pwm, duty_pt);
+ aspeed_set_pwm_polarity(priv, pwm, state->polarity);
+ } else {
+ aspeed_set_pwm_duty(priv, pwm, 0);
+ }
+ cur_state->period = state->period;
+ cur_state->duty_cycle = state->duty_cycle;
+ cur_state->polarity = state->polarity;
+ cur_state->enabled = state->enabled;
+
+ return 0;
+}
+static const struct pwm_ops aspeed_pwm_ops = {
+ .request = aspeed_pwm_request,
+ .free = aspeed_pwm_free,
+ .apply = aspeed_pwm_apply,
+ .owner = THIS_MODULE,
+};
+
+static int aspeed_pwm_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct clk *clk;
+ int ret;
+ struct aspeed_pwm_data *priv;
+ struct device_node *np;
+
+ priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ np = pdev->dev.parent->of_node;
+ if (!of_device_is_compatible(np, "aspeed,ast2600-pwm-tach")) {
+ dev_err(dev, "unsupported pwm device binding\n");
+ return -ENODEV;
+ }
+
+ priv->regmap = syscon_node_to_regmap(np);
+ if (IS_ERR(priv->regmap)) {
+ dev_err(dev, "Couldn't get regmap\n");
+ return -ENODEV;
+ }
+
+ clk = devm_clk_get(dev, NULL);
+ if (IS_ERR(clk))
+ return -ENODEV;
+ priv->clk_freq = clk_get_rate(clk);
+
+ priv->reset = reset_control_get_shared(&pdev->dev, NULL);
+ if (IS_ERR(priv->reset)) {
+ dev_err(&pdev->dev, "can't get aspeed_pwm_tacho reset\n");
+ return PTR_ERR(priv->reset);
+ }
+ reset_control_deassert(priv->reset);
+
+ priv->chip.dev = dev;
+ priv->chip.ops = &aspeed_pwm_ops;
+ priv->chip.base = -1;
+ priv->chip.npwm = ASPEED_NR_PWMS;
+ priv->chip.of_xlate = of_pwm_xlate_with_flags;
+ priv->chip.of_pwm_n_cells = 3;
+
+ ret = pwmchip_add(&priv->chip);
+ if (ret < 0) {
+ dev_err(&pdev->dev, "failed to add PWM chip: %d\n", ret);
+ return ret;
+ }
+ dev_set_drvdata(dev, priv);
+ return ret;
+}
+
+static const struct of_device_id of_pwm_match_table[] = {
+ {
+ .compatible = "aspeed,ast2600-pwm",
+ },
+ {},
+};
+MODULE_DEVICE_TABLE(of, of_pwm_match_table);
+
+static struct platform_driver aspeed_pwm_driver = {
+ .probe = aspeed_pwm_probe,
+ .driver = {
+ .name = "aspeed_pwm",
+ .of_match_table = of_pwm_match_table,
+ },
+};
+
+module_platform_driver(aspeed_pwm_driver);
+
+MODULE_AUTHOR("Billy Tsai <billy_tsai@aspeedtech.com>");
+MODULE_DESCRIPTION("ASPEED PWM device driver");
+MODULE_LICENSE("GPL v2");
--
2.25.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 3/4] pwm: Add Aspeed ast2600 PWM support
@ 2021-04-12 9:54 ` Billy Tsai
0 siblings, 0 replies; 26+ messages in thread
From: Billy Tsai @ 2021-04-12 9:54 UTC (permalink / raw)
To: lee.jones, robh+dt, joel, andrew, thierry.reding,
u.kleine-koenig, p.zabel, billy_tasi, devicetree,
linux-arm-kernel, linux-aspeed, linux-kernel, linux-pwm
Cc: BMC-SW
This patch add the support of PWM controller which can find at aspeed
ast2600 soc chip. This controller supoorts up to 16 channels.
Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
---
drivers/pwm/pwm-aspeed-g6.c | 291 ++++++++++++++++++++++++++++++++++++
1 file changed, 291 insertions(+)
create mode 100644 drivers/pwm/pwm-aspeed-g6.c
diff --git a/drivers/pwm/pwm-aspeed-g6.c b/drivers/pwm/pwm-aspeed-g6.c
new file mode 100644
index 000000000000..4bb4f97453c6
--- /dev/null
+++ b/drivers/pwm/pwm-aspeed-g6.c
@@ -0,0 +1,291 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) ASPEED Technology Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 or later as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/clk.h>
+#include <linux/errno.h>
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/of_platform.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/sysfs.h>
+#include <linux/reset.h>
+#include <linux/regmap.h>
+#include <linux/pwm.h>
+/* The channel number of Aspeed pwm controller */
+#define ASPEED_NR_PWMS 16
+/* PWM Control Register */
+#define ASPEED_PWM_CTRL_CH(ch) ((ch * 0x10) + 0x00)
+#define PWM_LOAD_SEL_AS_WDT BIT(19)
+#define LOAD_SEL_FALLING 0
+#define LOAD_SEL_RIGING 1
+#define PWM_DUTY_LOAD_AS_WDT_EN BIT(18)
+#define PWM_DUTY_SYNC_DIS BIT(17)
+#define PWM_CLK_ENABLE BIT(16)
+#define PWM_LEVEL_OUTPUT BIT(15)
+#define PWM_INVERSE BIT(14)
+#define PWM_OPEN_DRAIN_EN BIT(13)
+#define PWM_PIN_EN BIT(12)
+#define PWM_CLK_DIV_H_SHIFT 8
+#define PWM_CLK_DIV_H_MASK (0xf << PWM_CLK_DIV_H_SHIFT)
+#define PWM_CLK_DIV_L_SHIFT 0
+#define PWM_CLK_DIV_L_MASK (0xff << PWM_CLK_DIV_L_SHIFT)
+/* PWM Duty Cycle Register */
+#define ASPEED_PWM_DUTY_CYCLE_CH(ch) ((ch * 0x10) + 0x04)
+#define PWM_PERIOD_SHIFT (24)
+#define PWM_PERIOD_MASK (0xff << PWM_PERIOD_SHIFT)
+#define PWM_POINT_AS_WDT_SHIFT (16)
+#define PWM_POINT_AS_WDT_MASK (0xff << PWM_POINT_AS_WDT_SHIFT)
+#define PWM_FALLING_POINT_SHIFT (8)
+#define PWM_FALLING_POINT_MASK (0xffff << PWM_FALLING_POINT_SHIFT)
+#define PWM_RISING_POINT_SHIFT (0)
+#define PWM_RISING_POINT_MASK (0xffff << PWM_RISING_POINT_SHIFT)
+/* PWM default value */
+#define DEFAULT_PWM_PERIOD 0xff
+#define DEFAULT_TARGET_PWM_FREQ 25000
+#define DEFAULT_DUTY_PT 10
+#define DEFAULT_WDT_RELOAD_DUTY_PT 16
+
+struct aspeed_pwm_data {
+ struct pwm_chip chip;
+ struct regmap *regmap;
+ unsigned long clk_freq;
+ struct reset_control *reset;
+};
+/**
+ * struct aspeed_pwm - per-PWM driver data
+ * @freq: cached pwm freq
+ */
+struct aspeed_pwm {
+ u32 freq;
+};
+
+static void aspeed_set_pwm_channel_enable(struct regmap *regmap, u8 pwm_channel,
+ bool enable)
+{
+ regmap_update_bits(regmap, ASPEED_PWM_CTRL_CH(pwm_channel),
+ (PWM_CLK_ENABLE | PWM_PIN_EN),
+ enable ? (PWM_CLK_ENABLE | PWM_PIN_EN) : 0);
+}
+/*
+ * The PWM frequency = HCLK(200Mhz) / (clock division L bit *
+ * clock division H bit * (period bit + 1))
+ */
+static void aspeed_set_pwm_freq(struct aspeed_pwm_data *priv,
+ struct pwm_device *pwm, u32 freq)
+{
+ u32 target_div, cal_freq;
+ u32 tmp_div_h, tmp_div_l, diff, min_diff = INT_MAX;
+ u32 div_h = BIT(5) - 1, div_l = BIT(8) - 1;
+ u8 div_found;
+ u32 index = pwm->hwpwm;
+ struct aspeed_pwm *channel = pwm_get_chip_data(pwm);
+
+ cal_freq = priv->clk_freq / (DEFAULT_PWM_PERIOD + 1);
+ target_div = DIV_ROUND_UP(cal_freq, freq);
+ div_found = 0;
+ /* calculate for target frequence */
+ for (tmp_div_h = 0; tmp_div_h < 0x10; tmp_div_h++) {
+ tmp_div_l = target_div / BIT(tmp_div_h) - 1;
+
+ if (tmp_div_l < 0 || tmp_div_l > 255)
+ continue;
+
+ diff = freq - cal_freq / (BIT(tmp_div_h) * (tmp_div_l + 1));
+ if (abs(diff) < abs(min_diff)) {
+ min_diff = diff;
+ div_l = tmp_div_l;
+ div_h = tmp_div_h;
+ div_found = 1;
+ if (diff == 0)
+ break;
+ }
+ }
+ if (div_found == 0) {
+ pr_debug("target freq: %d too slow set minimal frequency\n",
+ freq);
+ }
+ channel->freq = cal_freq / (BIT(div_h) * (div_l + 1));
+ pr_debug("div h %x, l : %x pwm out clk %d\n", div_h, div_l,
+ channel->freq);
+ pr_debug("hclk %ld, target pwm freq %d, real pwm freq %d\n",
+ priv->clk_freq, freq, channel->freq);
+
+ regmap_update_bits(priv->regmap, ASPEED_PWM_CTRL_CH(index),
+ (PWM_CLK_DIV_H_MASK | PWM_CLK_DIV_L_MASK),
+ (div_h << PWM_CLK_DIV_H_SHIFT) |
+ (div_l << PWM_CLK_DIV_L_SHIFT));
+}
+
+static void aspeed_set_pwm_duty(struct aspeed_pwm_data *priv,
+ struct pwm_device *pwm, u32 duty_pt)
+{
+ u32 index = pwm->hwpwm;
+
+ if (duty_pt == 0) {
+ aspeed_set_pwm_channel_enable(priv->regmap, index, false);
+ } else {
+ regmap_update_bits(priv->regmap,
+ ASPEED_PWM_DUTY_CYCLE_CH(index),
+ PWM_FALLING_POINT_MASK,
+ duty_pt << PWM_FALLING_POINT_SHIFT);
+ aspeed_set_pwm_channel_enable(priv->regmap, index, true);
+ }
+}
+
+static void aspeed_set_pwm_polarity(struct aspeed_pwm_data *priv,
+ struct pwm_device *pwm, u8 polarity)
+{
+ u32 index = pwm->hwpwm;
+
+ regmap_update_bits(priv->regmap, ASPEED_PWM_CTRL_CH(index), PWM_INVERSE,
+ (polarity) ? PWM_INVERSE : 0);
+}
+
+static int aspeed_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+ struct device *dev = chip->dev;
+ struct aspeed_pwm_data *priv = dev_get_drvdata(dev);
+ struct aspeed_pwm *channel;
+ u32 index = pwm->hwpwm;
+ /*
+ * Fixed the period to the max value and rising point to 0
+ * for high resolution and \bsimplified frequency calculation.
+ */
+ regmap_update_bits(priv->regmap, ASPEED_PWM_DUTY_CYCLE_CH(index),
+ PWM_PERIOD_MASK,
+ DEFAULT_PWM_PERIOD << PWM_PERIOD_SHIFT);
+
+ regmap_update_bits(priv->regmap, ASPEED_PWM_DUTY_CYCLE_CH(index),
+ PWM_RISING_POINT_MASK, 0);
+
+ channel = devm_kzalloc(dev, sizeof(*channel), GFP_KERNEL);
+ if (!channel)
+ return -ENOMEM;
+
+ pwm_set_chip_data(pwm, channel);
+
+ return 0;
+}
+
+static void aspeed_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+ struct device *dev = chip->dev;
+ struct aspeed_pwm *channel = pwm_get_chip_data(pwm);
+
+ devm_kfree(dev, channel);
+}
+
+static int aspeed_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+ const struct pwm_state *state)
+{
+ struct device *dev = chip->dev;
+ struct aspeed_pwm_data *priv = dev_get_drvdata(dev);
+ struct pwm_state *cur_state = &pwm->state;
+ u32 freq = DIV_ROUND_UP_ULL(1000000000, state->period);
+ u32 duty_pt = DIV_ROUND_UP_ULL(
+ state->duty_cycle * (DEFAULT_PWM_PERIOD + 1), state->period);
+ dev_dbg(dev, "freq: %d, duty_pt: %d", freq, duty_pt);
+ if (state->enabled) {
+ aspeed_set_pwm_freq(priv, pwm, freq);
+ aspeed_set_pwm_duty(priv, pwm, duty_pt);
+ aspeed_set_pwm_polarity(priv, pwm, state->polarity);
+ } else {
+ aspeed_set_pwm_duty(priv, pwm, 0);
+ }
+ cur_state->period = state->period;
+ cur_state->duty_cycle = state->duty_cycle;
+ cur_state->polarity = state->polarity;
+ cur_state->enabled = state->enabled;
+
+ return 0;
+}
+static const struct pwm_ops aspeed_pwm_ops = {
+ .request = aspeed_pwm_request,
+ .free = aspeed_pwm_free,
+ .apply = aspeed_pwm_apply,
+ .owner = THIS_MODULE,
+};
+
+static int aspeed_pwm_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct clk *clk;
+ int ret;
+ struct aspeed_pwm_data *priv;
+ struct device_node *np;
+
+ priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ np = pdev->dev.parent->of_node;
+ if (!of_device_is_compatible(np, "aspeed,ast2600-pwm-tach")) {
+ dev_err(dev, "unsupported pwm device binding\n");
+ return -ENODEV;
+ }
+
+ priv->regmap = syscon_node_to_regmap(np);
+ if (IS_ERR(priv->regmap)) {
+ dev_err(dev, "Couldn't get regmap\n");
+ return -ENODEV;
+ }
+
+ clk = devm_clk_get(dev, NULL);
+ if (IS_ERR(clk))
+ return -ENODEV;
+ priv->clk_freq = clk_get_rate(clk);
+
+ priv->reset = reset_control_get_shared(&pdev->dev, NULL);
+ if (IS_ERR(priv->reset)) {
+ dev_err(&pdev->dev, "can't get aspeed_pwm_tacho reset\n");
+ return PTR_ERR(priv->reset);
+ }
+ reset_control_deassert(priv->reset);
+
+ priv->chip.dev = dev;
+ priv->chip.ops = &aspeed_pwm_ops;
+ priv->chip.base = -1;
+ priv->chip.npwm = ASPEED_NR_PWMS;
+ priv->chip.of_xlate = of_pwm_xlate_with_flags;
+ priv->chip.of_pwm_n_cells = 3;
+
+ ret = pwmchip_add(&priv->chip);
+ if (ret < 0) {
+ dev_err(&pdev->dev, "failed to add PWM chip: %d\n", ret);
+ return ret;
+ }
+ dev_set_drvdata(dev, priv);
+ return ret;
+}
+
+static const struct of_device_id of_pwm_match_table[] = {
+ {
+ .compatible = "aspeed,ast2600-pwm",
+ },
+ {},
+};
+MODULE_DEVICE_TABLE(of, of_pwm_match_table);
+
+static struct platform_driver aspeed_pwm_driver = {
+ .probe = aspeed_pwm_probe,
+ .driver = {
+ .name = "aspeed_pwm",
+ .of_match_table = of_pwm_match_table,
+ },
+};
+
+module_platform_driver(aspeed_pwm_driver);
+
+MODULE_AUTHOR("Billy Tsai <billy_tsai@aspeedtech.com>");
+MODULE_DESCRIPTION("ASPEED PWM device driver");
+MODULE_LICENSE("GPL v2");
--
2.25.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 4/4] pwm: Add support for aspeed pwm controller
2021-04-12 9:54 ` Billy Tsai
@ 2021-04-12 9:54 ` Billy Tsai
-1 siblings, 0 replies; 26+ messages in thread
From: Billy Tsai @ 2021-04-12 9:54 UTC (permalink / raw)
To: lee.jones, robh+dt, joel, andrew, thierry.reding,
u.kleine-koenig, p.zabel, billy_tasi, devicetree,
linux-arm-kernel, linux-aspeed, linux-kernel, linux-pwm
Cc: BMC-SW
Add support for the pwm controller which can be found at aspeed ast2600
soc. This driver is part function of multi-funciton of device "pwm-tach
controller".
Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
---
drivers/pwm/Kconfig | 6 ++++++
drivers/pwm/Makefile | 1 +
2 files changed, 7 insertions(+)
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 63be5362fd3a..947ed642debe 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -42,6 +42,12 @@ config PWM_DEBUG
It is expected to introduce some runtime overhead and diagnostic
output to the kernel log, so only enable while working on a driver.
+config PWM_ASPEED_G6
+ tristate "ASPEEDG6 PWM support"
+ help
+ This driver provides support for ASPEED G6 PWM controllers.
+
+
config PWM_AB8500
tristate "AB8500 PWM support"
depends on AB8500_CORE && ARCH_U8500
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index cbdcd55d69ee..4a74c68547bf 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -1,6 +1,7 @@
# SPDX-License-Identifier: GPL-2.0
obj-$(CONFIG_PWM) += core.o
obj-$(CONFIG_PWM_SYSFS) += sysfs.o
+obj-$(CONFIG_PWM_ASPEED_G6) += pwm-aspeed-g6.o
obj-$(CONFIG_PWM_AB8500) += pwm-ab8500.o
obj-$(CONFIG_PWM_ATMEL) += pwm-atmel.o
obj-$(CONFIG_PWM_ATMEL_HLCDC_PWM) += pwm-atmel-hlcdc.o
--
2.25.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 4/4] pwm: Add support for aspeed pwm controller
@ 2021-04-12 9:54 ` Billy Tsai
0 siblings, 0 replies; 26+ messages in thread
From: Billy Tsai @ 2021-04-12 9:54 UTC (permalink / raw)
To: lee.jones, robh+dt, joel, andrew, thierry.reding,
u.kleine-koenig, p.zabel, billy_tasi, devicetree,
linux-arm-kernel, linux-aspeed, linux-kernel, linux-pwm
Cc: BMC-SW
Add support for the pwm controller which can be found at aspeed ast2600
soc. This driver is part function of multi-funciton of device "pwm-tach
controller".
Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
---
drivers/pwm/Kconfig | 6 ++++++
drivers/pwm/Makefile | 1 +
2 files changed, 7 insertions(+)
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 63be5362fd3a..947ed642debe 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -42,6 +42,12 @@ config PWM_DEBUG
It is expected to introduce some runtime overhead and diagnostic
output to the kernel log, so only enable while working on a driver.
+config PWM_ASPEED_G6
+ tristate "ASPEEDG6 PWM support"
+ help
+ This driver provides support for ASPEED G6 PWM controllers.
+
+
config PWM_AB8500
tristate "AB8500 PWM support"
depends on AB8500_CORE && ARCH_U8500
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index cbdcd55d69ee..4a74c68547bf 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -1,6 +1,7 @@
# SPDX-License-Identifier: GPL-2.0
obj-$(CONFIG_PWM) += core.o
obj-$(CONFIG_PWM_SYSFS) += sysfs.o
+obj-$(CONFIG_PWM_ASPEED_G6) += pwm-aspeed-g6.o
obj-$(CONFIG_PWM_AB8500) += pwm-ab8500.o
obj-$(CONFIG_PWM_ATMEL) += pwm-atmel.o
obj-$(CONFIG_PWM_ATMEL_HLCDC_PWM) += pwm-atmel-hlcdc.o
--
2.25.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH 4/4] pwm: Add support for aspeed pwm controller
2021-04-12 9:54 ` Billy Tsai
@ 2021-04-12 11:14 ` Uwe Kleine-König
-1 siblings, 0 replies; 26+ messages in thread
From: Uwe Kleine-König @ 2021-04-12 11:14 UTC (permalink / raw)
To: Billy Tsai
Cc: lee.jones, robh+dt, joel, andrew, thierry.reding, p.zabel,
billy_tasi, devicetree, linux-arm-kernel, linux-aspeed,
linux-kernel, linux-pwm, BMC-SW
[-- Attachment #1: Type: text/plain, Size: 1054 bytes --]
Hello,
On Mon, Apr 12, 2021 at 05:54:57PM +0800, Billy Tsai wrote:
> Add support for the pwm controller which can be found at aspeed ast2600
> soc. This driver is part function of multi-funciton of device "pwm-tach
> controller".
please squash this into patch 3.
> Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
> ---
> drivers/pwm/Kconfig | 6 ++++++
> drivers/pwm/Makefile | 1 +
> 2 files changed, 7 insertions(+)
>
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 63be5362fd3a..947ed642debe 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -42,6 +42,12 @@ config PWM_DEBUG
> It is expected to introduce some runtime overhead and diagnostic
> output to the kernel log, so only enable while working on a driver.
>
> +config PWM_ASPEED_G6
> + tristate "ASPEEDG6 PWM support"
No depends?
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 4/4] pwm: Add support for aspeed pwm controller
@ 2021-04-12 11:14 ` Uwe Kleine-König
0 siblings, 0 replies; 26+ messages in thread
From: Uwe Kleine-König @ 2021-04-12 11:14 UTC (permalink / raw)
To: Billy Tsai
Cc: lee.jones, robh+dt, joel, andrew, thierry.reding, p.zabel,
billy_tasi, devicetree, linux-arm-kernel, linux-aspeed,
linux-kernel, linux-pwm, BMC-SW
[-- Attachment #1.1: Type: text/plain, Size: 1054 bytes --]
Hello,
On Mon, Apr 12, 2021 at 05:54:57PM +0800, Billy Tsai wrote:
> Add support for the pwm controller which can be found at aspeed ast2600
> soc. This driver is part function of multi-funciton of device "pwm-tach
> controller".
please squash this into patch 3.
> Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
> ---
> drivers/pwm/Kconfig | 6 ++++++
> drivers/pwm/Makefile | 1 +
> 2 files changed, 7 insertions(+)
>
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 63be5362fd3a..947ed642debe 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -42,6 +42,12 @@ config PWM_DEBUG
> It is expected to introduce some runtime overhead and diagnostic
> output to the kernel log, so only enable while working on a driver.
>
> +config PWM_ASPEED_G6
> + tristate "ASPEEDG6 PWM support"
No depends?
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 3/4] pwm: Add Aspeed ast2600 PWM support
2021-04-12 9:54 ` Billy Tsai
@ 2021-04-12 12:35 ` Uwe Kleine-König
-1 siblings, 0 replies; 26+ messages in thread
From: Uwe Kleine-König @ 2021-04-12 12:35 UTC (permalink / raw)
To: Billy Tsai
Cc: lee.jones, robh+dt, joel, andrew, thierry.reding, p.zabel,
billy_tasi, devicetree, linux-arm-kernel, linux-aspeed,
linux-kernel, linux-pwm, BMC-SW
[-- Attachment #1: Type: text/plain, Size: 12445 bytes --]
Hello Billy,
On Mon, Apr 12, 2021 at 05:54:56PM +0800, Billy Tsai wrote:
> This patch add the support of PWM controller which can find at aspeed
> ast2600 soc chip. This controller supoorts up to 16 channels.
>
> Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
> ---
> drivers/pwm/pwm-aspeed-g6.c | 291 ++++++++++++++++++++++++++++++++++++
> 1 file changed, 291 insertions(+)
> create mode 100644 drivers/pwm/pwm-aspeed-g6.c
>
> diff --git a/drivers/pwm/pwm-aspeed-g6.c b/drivers/pwm/pwm-aspeed-g6.c
> new file mode 100644
> index 000000000000..4bb4f97453c6
> --- /dev/null
> +++ b/drivers/pwm/pwm-aspeed-g6.c
> @@ -0,0 +1,291 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) ASPEED Technology Inc.
Don't you need to add a year here?
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 or later as
> + * published by the Free Software Foundation.
Hmm, the comment and the SPDX-License-Identifier contradict each other.
The idea of the latter is that the former isn't needed.
> + */
Is there documentation available in the internet for this hardware? If
yes, please mention a link here.
Also describe the hardware here similar to how e.g.
drivers/pwm/pwm-sifive.c does it. Please stick to the same format for
easy grepping.
> +
> +#include <linux/clk.h>
> +#include <linux/errno.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/module.h>
> +#include <linux/of_platform.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/sysfs.h>
> +#include <linux/reset.h>
> +#include <linux/regmap.h>
> +#include <linux/pwm.h>
> +/* The channel number of Aspeed pwm controller */
> +#define ASPEED_NR_PWMS 16
> +/* PWM Control Register */
> +#define ASPEED_PWM_CTRL_CH(ch) ((ch * 0x10) + 0x00)
#define ASPEED_PWM_CTRL_CH(ch) (((ch) * 0x10) + 0x00)
> +#define PWM_LOAD_SEL_AS_WDT BIT(19)
> +#define LOAD_SEL_FALLING 0
> +#define LOAD_SEL_RIGING 1
> +#define PWM_DUTY_LOAD_AS_WDT_EN BIT(18)
> +#define PWM_DUTY_SYNC_DIS BIT(17)
> +#define PWM_CLK_ENABLE BIT(16)
> +#define PWM_LEVEL_OUTPUT BIT(15)
> +#define PWM_INVERSE BIT(14)
> +#define PWM_OPEN_DRAIN_EN BIT(13)
> +#define PWM_PIN_EN BIT(12)
> +#define PWM_CLK_DIV_H_SHIFT 8
> +#define PWM_CLK_DIV_H_MASK (0xf << PWM_CLK_DIV_H_SHIFT)
> +#define PWM_CLK_DIV_L_SHIFT 0
> +#define PWM_CLK_DIV_L_MASK (0xff << PWM_CLK_DIV_L_SHIFT)
> +/* PWM Duty Cycle Register */
> +#define ASPEED_PWM_DUTY_CYCLE_CH(ch) ((ch * 0x10) + 0x04)
> +#define PWM_PERIOD_SHIFT (24)
> +#define PWM_PERIOD_MASK (0xff << PWM_PERIOD_SHIFT)
> +#define PWM_POINT_AS_WDT_SHIFT (16)
> +#define PWM_POINT_AS_WDT_MASK (0xff << PWM_POINT_AS_WDT_SHIFT)
> +#define PWM_FALLING_POINT_SHIFT (8)
> +#define PWM_FALLING_POINT_MASK (0xffff << PWM_FALLING_POINT_SHIFT)
> +#define PWM_RISING_POINT_SHIFT (0)
> +#define PWM_RISING_POINT_MASK (0xffff << PWM_RISING_POINT_SHIFT)
> +/* PWM default value */
> +#define DEFAULT_PWM_PERIOD 0xff
> +#define DEFAULT_TARGET_PWM_FREQ 25000
> +#define DEFAULT_DUTY_PT 10
> +#define DEFAULT_WDT_RELOAD_DUTY_PT 16
You could spend a few empty lines to make this better readable. Also
please use a consistent driver-specific prefix for your defines and
consider using the macros from <linux/bitfield.h>. Also defines
for bitfields should contain the register name.
> +struct aspeed_pwm_data {
> + struct pwm_chip chip;
> + struct regmap *regmap;
> + unsigned long clk_freq;
> + struct reset_control *reset;
> +};
> +/**
> + * struct aspeed_pwm - per-PWM driver data
> + * @freq: cached pwm freq
> + */
> +struct aspeed_pwm {
> + u32 freq;
> +};
This is actually unused, please drop it. (You save a value in it, but
make never use of it.)
> +static void aspeed_set_pwm_channel_enable(struct regmap *regmap, u8 pwm_channel,
> + bool enable)
> +{
> + regmap_update_bits(regmap, ASPEED_PWM_CTRL_CH(pwm_channel),
> + (PWM_CLK_ENABLE | PWM_PIN_EN),
> + enable ? (PWM_CLK_ENABLE | PWM_PIN_EN) : 0);
What is the semantic of PIN_EN?
> +}
> +/*
> + * The PWM frequency = HCLK(200Mhz) / (clock division L bit *
> + * clock division H bit * (period bit + 1))
> + */
> +static void aspeed_set_pwm_freq(struct aspeed_pwm_data *priv,
> + struct pwm_device *pwm, u32 freq)
> +{
> + u32 target_div, cal_freq;
> + u32 tmp_div_h, tmp_div_l, diff, min_diff = INT_MAX;
> + u32 div_h = BIT(5) - 1, div_l = BIT(8) - 1;
> + u8 div_found;
> + u32 index = pwm->hwpwm;
> + struct aspeed_pwm *channel = pwm_get_chip_data(pwm);
> +
> + cal_freq = priv->clk_freq / (DEFAULT_PWM_PERIOD + 1);
> + target_div = DIV_ROUND_UP(cal_freq, freq);
> + div_found = 0;
> + /* calculate for target frequence */
s/frequence/frequency/
> + for (tmp_div_h = 0; tmp_div_h < 0x10; tmp_div_h++) {
> + tmp_div_l = target_div / BIT(tmp_div_h) - 1;
> +
> + if (tmp_div_l < 0 || tmp_div_l > 255)
> + continue;
> +
> + diff = freq - cal_freq / (BIT(tmp_div_h) * (tmp_div_l + 1));
> + if (abs(diff) < abs(min_diff)) {
> + min_diff = diff;
> + div_l = tmp_div_l;
> + div_h = tmp_div_h;
> + div_found = 1;
> + if (diff == 0)
> + break;
> + }
> + }
If my understanding is right (i.e. H divides by a power of two and L by
an integer) this can be simplified.
> + if (div_found == 0) {
> + pr_debug("target freq: %d too slow set minimal frequency\n",
> + freq);
> + }
> + channel->freq = cal_freq / (BIT(div_h) * (div_l + 1));
> + pr_debug("div h %x, l : %x pwm out clk %d\n", div_h, div_l,
> + channel->freq);
> + pr_debug("hclk %ld, target pwm freq %d, real pwm freq %d\n",
> + priv->clk_freq, freq, channel->freq);
> +
> + regmap_update_bits(priv->regmap, ASPEED_PWM_CTRL_CH(index),
> + (PWM_CLK_DIV_H_MASK | PWM_CLK_DIV_L_MASK),
> + (div_h << PWM_CLK_DIV_H_SHIFT) |
> + (div_l << PWM_CLK_DIV_L_SHIFT));
> +}
> +
> +static void aspeed_set_pwm_duty(struct aspeed_pwm_data *priv,
> + struct pwm_device *pwm, u32 duty_pt)
> +{
> + u32 index = pwm->hwpwm;
> +
> + if (duty_pt == 0) {
> + aspeed_set_pwm_channel_enable(priv->regmap, index, false);
> + } else {
> + regmap_update_bits(priv->regmap,
> + ASPEED_PWM_DUTY_CYCLE_CH(index),
> + PWM_FALLING_POINT_MASK,
> + duty_pt << PWM_FALLING_POINT_SHIFT);
> + aspeed_set_pwm_channel_enable(priv->regmap, index, true);
> + }
> +}
> +
> +static void aspeed_set_pwm_polarity(struct aspeed_pwm_data *priv,
> + struct pwm_device *pwm, u8 polarity)
> +{
> + u32 index = pwm->hwpwm;
> +
> + regmap_update_bits(priv->regmap, ASPEED_PWM_CTRL_CH(index), PWM_INVERSE,
> + (polarity) ? PWM_INVERSE : 0);
> +}
> +
> +static int aspeed_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> + struct device *dev = chip->dev;
> + struct aspeed_pwm_data *priv = dev_get_drvdata(dev);
> + struct aspeed_pwm *channel;
> + u32 index = pwm->hwpwm;
> + /*
> + * Fixed the period to the max value and rising point to 0
> + * for high resolution and \bsimplified frequency calculation.
s/^H//
> + */
> + regmap_update_bits(priv->regmap, ASPEED_PWM_DUTY_CYCLE_CH(index),
> + PWM_PERIOD_MASK,
> + DEFAULT_PWM_PERIOD << PWM_PERIOD_SHIFT);
> +
> + regmap_update_bits(priv->regmap, ASPEED_PWM_DUTY_CYCLE_CH(index),
> + PWM_RISING_POINT_MASK, 0);
Only .apply is supposed to modify the PWM's configuration.
> + channel = devm_kzalloc(dev, sizeof(*channel), GFP_KERNEL);
> + if (!channel)
> + return -ENOMEM;
Don't use devm_kzalloc if freeing isn't done at device cleanup time.
> + pwm_set_chip_data(pwm, channel);
> +
> + return 0;
> +}
> +
> +static void aspeed_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> + struct device *dev = chip->dev;
> + struct aspeed_pwm *channel = pwm_get_chip_data(pwm);
> +
> + devm_kfree(dev, channel);
> +}
> +
> +static int aspeed_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> + const struct pwm_state *state)
> +{
> + struct device *dev = chip->dev;
> + struct aspeed_pwm_data *priv = dev_get_drvdata(dev);
Please consider using
priv = container_of(chip, struct aspeed_pwm_data, chip);
(preferably wrapped in a macro) which is more type safe and more
effective to calculate.
> + struct pwm_state *cur_state = &pwm->state;
> + u32 freq = DIV_ROUND_UP_ULL(1000000000, state->period);
> + u32 duty_pt = DIV_ROUND_UP_ULL(
> + state->duty_cycle * (DEFAULT_PWM_PERIOD + 1), state->period);
You're loosing precision here.
> + dev_dbg(dev, "freq: %d, duty_pt: %d", freq, duty_pt);
> + if (state->enabled) {
> + aspeed_set_pwm_freq(priv, pwm, freq);
> + aspeed_set_pwm_duty(priv, pwm, duty_pt);
> + aspeed_set_pwm_polarity(priv, pwm, state->polarity);
How does the hardware behave between these calls? E.g. can it happen
that it already emits a normal period when inversed polarity is
requested just before aspeed_set_pwm_polarity is called? Or there is a
period with the previous duty cycle and the new period?
> + } else {
> + aspeed_set_pwm_duty(priv, pwm, 0);
> + }
> + cur_state->period = state->period;
> + cur_state->duty_cycle = state->duty_cycle;
> + cur_state->polarity = state->polarity;
> + cur_state->enabled = state->enabled;
The driver is not supposed to modify pwm->state.
> + return 0;
> +}
From your code I understood: The period of the signal is
(PWM_PERIOD + 1) * (2 ** DIV_H) * (DIV_L + 1) / 200 MHz
. The duty cycle is
PWM_FALLING_POINT * (2 ** DIV_H) * (DIV_L + 1) / 200 MHz
. So the PWM cannot provide a 100% relative duty cycle.
Is this right?
> +static const struct pwm_ops aspeed_pwm_ops = {
> + .request = aspeed_pwm_request,
> + .free = aspeed_pwm_free,
> + .apply = aspeed_pwm_apply,
Please test your driver with PWM_DEBUG enabled.
> + .owner = THIS_MODULE,
> +};
> +
> +static int aspeed_pwm_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct clk *clk;
> + int ret;
> + struct aspeed_pwm_data *priv;
> + struct device_node *np;
> +
> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + np = pdev->dev.parent->of_node;
> + if (!of_device_is_compatible(np, "aspeed,ast2600-pwm-tach")) {
> + dev_err(dev, "unsupported pwm device binding\n");
> + return -ENODEV;
> + }
Is this pwm-tach an mfd?
> + priv->regmap = syscon_node_to_regmap(np);
> + if (IS_ERR(priv->regmap)) {
> + dev_err(dev, "Couldn't get regmap\n");
> + return -ENODEV;
> + }
> +
> + clk = devm_clk_get(dev, NULL);
> + if (IS_ERR(clk))
> + return -ENODEV;
> + priv->clk_freq = clk_get_rate(clk);
If you intend to use this clock, you have to enable it.
> + priv->reset = reset_control_get_shared(&pdev->dev, NULL);
> + if (IS_ERR(priv->reset)) {
> + dev_err(&pdev->dev, "can't get aspeed_pwm_tacho reset\n");
> + return PTR_ERR(priv->reset);
> + }
> + reset_control_deassert(priv->reset);
missing error checking
> + priv->chip.dev = dev;
> + priv->chip.ops = &aspeed_pwm_ops;
> + priv->chip.base = -1;
This isn't necessary since f9a8ee8c8bcd118e800d88772c6457381db45224,
please drop the assignment to base.
> + priv->chip.npwm = ASPEED_NR_PWMS;
> + priv->chip.of_xlate = of_pwm_xlate_with_flags;
> + priv->chip.of_pwm_n_cells = 3;
> +
> + ret = pwmchip_add(&priv->chip);
> + if (ret < 0) {
> + dev_err(&pdev->dev, "failed to add PWM chip: %d\n", ret);
Please use %pe to make the error messages better readable.
> + return ret;
> + }
> + dev_set_drvdata(dev, priv);
> + return ret;
> +}
> +
> +static const struct of_device_id of_pwm_match_table[] = {
> + {
> + .compatible = "aspeed,ast2600-pwm",
> + },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, of_pwm_match_table);
> +
> +static struct platform_driver aspeed_pwm_driver = {
> + .probe = aspeed_pwm_probe,
Please implement a .remove callback.
> + .driver = {
> + .name = "aspeed_pwm",
> + .of_match_table = of_pwm_match_table,
> + },
> +};
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 3/4] pwm: Add Aspeed ast2600 PWM support
@ 2021-04-12 12:35 ` Uwe Kleine-König
0 siblings, 0 replies; 26+ messages in thread
From: Uwe Kleine-König @ 2021-04-12 12:35 UTC (permalink / raw)
To: Billy Tsai
Cc: lee.jones, robh+dt, joel, andrew, thierry.reding, p.zabel,
billy_tasi, devicetree, linux-arm-kernel, linux-aspeed,
linux-kernel, linux-pwm, BMC-SW
[-- Attachment #1.1: Type: text/plain, Size: 12445 bytes --]
Hello Billy,
On Mon, Apr 12, 2021 at 05:54:56PM +0800, Billy Tsai wrote:
> This patch add the support of PWM controller which can find at aspeed
> ast2600 soc chip. This controller supoorts up to 16 channels.
>
> Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
> ---
> drivers/pwm/pwm-aspeed-g6.c | 291 ++++++++++++++++++++++++++++++++++++
> 1 file changed, 291 insertions(+)
> create mode 100644 drivers/pwm/pwm-aspeed-g6.c
>
> diff --git a/drivers/pwm/pwm-aspeed-g6.c b/drivers/pwm/pwm-aspeed-g6.c
> new file mode 100644
> index 000000000000..4bb4f97453c6
> --- /dev/null
> +++ b/drivers/pwm/pwm-aspeed-g6.c
> @@ -0,0 +1,291 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) ASPEED Technology Inc.
Don't you need to add a year here?
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 or later as
> + * published by the Free Software Foundation.
Hmm, the comment and the SPDX-License-Identifier contradict each other.
The idea of the latter is that the former isn't needed.
> + */
Is there documentation available in the internet for this hardware? If
yes, please mention a link here.
Also describe the hardware here similar to how e.g.
drivers/pwm/pwm-sifive.c does it. Please stick to the same format for
easy grepping.
> +
> +#include <linux/clk.h>
> +#include <linux/errno.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/module.h>
> +#include <linux/of_platform.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/sysfs.h>
> +#include <linux/reset.h>
> +#include <linux/regmap.h>
> +#include <linux/pwm.h>
> +/* The channel number of Aspeed pwm controller */
> +#define ASPEED_NR_PWMS 16
> +/* PWM Control Register */
> +#define ASPEED_PWM_CTRL_CH(ch) ((ch * 0x10) + 0x00)
#define ASPEED_PWM_CTRL_CH(ch) (((ch) * 0x10) + 0x00)
> +#define PWM_LOAD_SEL_AS_WDT BIT(19)
> +#define LOAD_SEL_FALLING 0
> +#define LOAD_SEL_RIGING 1
> +#define PWM_DUTY_LOAD_AS_WDT_EN BIT(18)
> +#define PWM_DUTY_SYNC_DIS BIT(17)
> +#define PWM_CLK_ENABLE BIT(16)
> +#define PWM_LEVEL_OUTPUT BIT(15)
> +#define PWM_INVERSE BIT(14)
> +#define PWM_OPEN_DRAIN_EN BIT(13)
> +#define PWM_PIN_EN BIT(12)
> +#define PWM_CLK_DIV_H_SHIFT 8
> +#define PWM_CLK_DIV_H_MASK (0xf << PWM_CLK_DIV_H_SHIFT)
> +#define PWM_CLK_DIV_L_SHIFT 0
> +#define PWM_CLK_DIV_L_MASK (0xff << PWM_CLK_DIV_L_SHIFT)
> +/* PWM Duty Cycle Register */
> +#define ASPEED_PWM_DUTY_CYCLE_CH(ch) ((ch * 0x10) + 0x04)
> +#define PWM_PERIOD_SHIFT (24)
> +#define PWM_PERIOD_MASK (0xff << PWM_PERIOD_SHIFT)
> +#define PWM_POINT_AS_WDT_SHIFT (16)
> +#define PWM_POINT_AS_WDT_MASK (0xff << PWM_POINT_AS_WDT_SHIFT)
> +#define PWM_FALLING_POINT_SHIFT (8)
> +#define PWM_FALLING_POINT_MASK (0xffff << PWM_FALLING_POINT_SHIFT)
> +#define PWM_RISING_POINT_SHIFT (0)
> +#define PWM_RISING_POINT_MASK (0xffff << PWM_RISING_POINT_SHIFT)
> +/* PWM default value */
> +#define DEFAULT_PWM_PERIOD 0xff
> +#define DEFAULT_TARGET_PWM_FREQ 25000
> +#define DEFAULT_DUTY_PT 10
> +#define DEFAULT_WDT_RELOAD_DUTY_PT 16
You could spend a few empty lines to make this better readable. Also
please use a consistent driver-specific prefix for your defines and
consider using the macros from <linux/bitfield.h>. Also defines
for bitfields should contain the register name.
> +struct aspeed_pwm_data {
> + struct pwm_chip chip;
> + struct regmap *regmap;
> + unsigned long clk_freq;
> + struct reset_control *reset;
> +};
> +/**
> + * struct aspeed_pwm - per-PWM driver data
> + * @freq: cached pwm freq
> + */
> +struct aspeed_pwm {
> + u32 freq;
> +};
This is actually unused, please drop it. (You save a value in it, but
make never use of it.)
> +static void aspeed_set_pwm_channel_enable(struct regmap *regmap, u8 pwm_channel,
> + bool enable)
> +{
> + regmap_update_bits(regmap, ASPEED_PWM_CTRL_CH(pwm_channel),
> + (PWM_CLK_ENABLE | PWM_PIN_EN),
> + enable ? (PWM_CLK_ENABLE | PWM_PIN_EN) : 0);
What is the semantic of PIN_EN?
> +}
> +/*
> + * The PWM frequency = HCLK(200Mhz) / (clock division L bit *
> + * clock division H bit * (period bit + 1))
> + */
> +static void aspeed_set_pwm_freq(struct aspeed_pwm_data *priv,
> + struct pwm_device *pwm, u32 freq)
> +{
> + u32 target_div, cal_freq;
> + u32 tmp_div_h, tmp_div_l, diff, min_diff = INT_MAX;
> + u32 div_h = BIT(5) - 1, div_l = BIT(8) - 1;
> + u8 div_found;
> + u32 index = pwm->hwpwm;
> + struct aspeed_pwm *channel = pwm_get_chip_data(pwm);
> +
> + cal_freq = priv->clk_freq / (DEFAULT_PWM_PERIOD + 1);
> + target_div = DIV_ROUND_UP(cal_freq, freq);
> + div_found = 0;
> + /* calculate for target frequence */
s/frequence/frequency/
> + for (tmp_div_h = 0; tmp_div_h < 0x10; tmp_div_h++) {
> + tmp_div_l = target_div / BIT(tmp_div_h) - 1;
> +
> + if (tmp_div_l < 0 || tmp_div_l > 255)
> + continue;
> +
> + diff = freq - cal_freq / (BIT(tmp_div_h) * (tmp_div_l + 1));
> + if (abs(diff) < abs(min_diff)) {
> + min_diff = diff;
> + div_l = tmp_div_l;
> + div_h = tmp_div_h;
> + div_found = 1;
> + if (diff == 0)
> + break;
> + }
> + }
If my understanding is right (i.e. H divides by a power of two and L by
an integer) this can be simplified.
> + if (div_found == 0) {
> + pr_debug("target freq: %d too slow set minimal frequency\n",
> + freq);
> + }
> + channel->freq = cal_freq / (BIT(div_h) * (div_l + 1));
> + pr_debug("div h %x, l : %x pwm out clk %d\n", div_h, div_l,
> + channel->freq);
> + pr_debug("hclk %ld, target pwm freq %d, real pwm freq %d\n",
> + priv->clk_freq, freq, channel->freq);
> +
> + regmap_update_bits(priv->regmap, ASPEED_PWM_CTRL_CH(index),
> + (PWM_CLK_DIV_H_MASK | PWM_CLK_DIV_L_MASK),
> + (div_h << PWM_CLK_DIV_H_SHIFT) |
> + (div_l << PWM_CLK_DIV_L_SHIFT));
> +}
> +
> +static void aspeed_set_pwm_duty(struct aspeed_pwm_data *priv,
> + struct pwm_device *pwm, u32 duty_pt)
> +{
> + u32 index = pwm->hwpwm;
> +
> + if (duty_pt == 0) {
> + aspeed_set_pwm_channel_enable(priv->regmap, index, false);
> + } else {
> + regmap_update_bits(priv->regmap,
> + ASPEED_PWM_DUTY_CYCLE_CH(index),
> + PWM_FALLING_POINT_MASK,
> + duty_pt << PWM_FALLING_POINT_SHIFT);
> + aspeed_set_pwm_channel_enable(priv->regmap, index, true);
> + }
> +}
> +
> +static void aspeed_set_pwm_polarity(struct aspeed_pwm_data *priv,
> + struct pwm_device *pwm, u8 polarity)
> +{
> + u32 index = pwm->hwpwm;
> +
> + regmap_update_bits(priv->regmap, ASPEED_PWM_CTRL_CH(index), PWM_INVERSE,
> + (polarity) ? PWM_INVERSE : 0);
> +}
> +
> +static int aspeed_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> + struct device *dev = chip->dev;
> + struct aspeed_pwm_data *priv = dev_get_drvdata(dev);
> + struct aspeed_pwm *channel;
> + u32 index = pwm->hwpwm;
> + /*
> + * Fixed the period to the max value and rising point to 0
> + * for high resolution and \bsimplified frequency calculation.
s/^H//
> + */
> + regmap_update_bits(priv->regmap, ASPEED_PWM_DUTY_CYCLE_CH(index),
> + PWM_PERIOD_MASK,
> + DEFAULT_PWM_PERIOD << PWM_PERIOD_SHIFT);
> +
> + regmap_update_bits(priv->regmap, ASPEED_PWM_DUTY_CYCLE_CH(index),
> + PWM_RISING_POINT_MASK, 0);
Only .apply is supposed to modify the PWM's configuration.
> + channel = devm_kzalloc(dev, sizeof(*channel), GFP_KERNEL);
> + if (!channel)
> + return -ENOMEM;
Don't use devm_kzalloc if freeing isn't done at device cleanup time.
> + pwm_set_chip_data(pwm, channel);
> +
> + return 0;
> +}
> +
> +static void aspeed_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> + struct device *dev = chip->dev;
> + struct aspeed_pwm *channel = pwm_get_chip_data(pwm);
> +
> + devm_kfree(dev, channel);
> +}
> +
> +static int aspeed_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> + const struct pwm_state *state)
> +{
> + struct device *dev = chip->dev;
> + struct aspeed_pwm_data *priv = dev_get_drvdata(dev);
Please consider using
priv = container_of(chip, struct aspeed_pwm_data, chip);
(preferably wrapped in a macro) which is more type safe and more
effective to calculate.
> + struct pwm_state *cur_state = &pwm->state;
> + u32 freq = DIV_ROUND_UP_ULL(1000000000, state->period);
> + u32 duty_pt = DIV_ROUND_UP_ULL(
> + state->duty_cycle * (DEFAULT_PWM_PERIOD + 1), state->period);
You're loosing precision here.
> + dev_dbg(dev, "freq: %d, duty_pt: %d", freq, duty_pt);
> + if (state->enabled) {
> + aspeed_set_pwm_freq(priv, pwm, freq);
> + aspeed_set_pwm_duty(priv, pwm, duty_pt);
> + aspeed_set_pwm_polarity(priv, pwm, state->polarity);
How does the hardware behave between these calls? E.g. can it happen
that it already emits a normal period when inversed polarity is
requested just before aspeed_set_pwm_polarity is called? Or there is a
period with the previous duty cycle and the new period?
> + } else {
> + aspeed_set_pwm_duty(priv, pwm, 0);
> + }
> + cur_state->period = state->period;
> + cur_state->duty_cycle = state->duty_cycle;
> + cur_state->polarity = state->polarity;
> + cur_state->enabled = state->enabled;
The driver is not supposed to modify pwm->state.
> + return 0;
> +}
From your code I understood: The period of the signal is
(PWM_PERIOD + 1) * (2 ** DIV_H) * (DIV_L + 1) / 200 MHz
. The duty cycle is
PWM_FALLING_POINT * (2 ** DIV_H) * (DIV_L + 1) / 200 MHz
. So the PWM cannot provide a 100% relative duty cycle.
Is this right?
> +static const struct pwm_ops aspeed_pwm_ops = {
> + .request = aspeed_pwm_request,
> + .free = aspeed_pwm_free,
> + .apply = aspeed_pwm_apply,
Please test your driver with PWM_DEBUG enabled.
> + .owner = THIS_MODULE,
> +};
> +
> +static int aspeed_pwm_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct clk *clk;
> + int ret;
> + struct aspeed_pwm_data *priv;
> + struct device_node *np;
> +
> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + np = pdev->dev.parent->of_node;
> + if (!of_device_is_compatible(np, "aspeed,ast2600-pwm-tach")) {
> + dev_err(dev, "unsupported pwm device binding\n");
> + return -ENODEV;
> + }
Is this pwm-tach an mfd?
> + priv->regmap = syscon_node_to_regmap(np);
> + if (IS_ERR(priv->regmap)) {
> + dev_err(dev, "Couldn't get regmap\n");
> + return -ENODEV;
> + }
> +
> + clk = devm_clk_get(dev, NULL);
> + if (IS_ERR(clk))
> + return -ENODEV;
> + priv->clk_freq = clk_get_rate(clk);
If you intend to use this clock, you have to enable it.
> + priv->reset = reset_control_get_shared(&pdev->dev, NULL);
> + if (IS_ERR(priv->reset)) {
> + dev_err(&pdev->dev, "can't get aspeed_pwm_tacho reset\n");
> + return PTR_ERR(priv->reset);
> + }
> + reset_control_deassert(priv->reset);
missing error checking
> + priv->chip.dev = dev;
> + priv->chip.ops = &aspeed_pwm_ops;
> + priv->chip.base = -1;
This isn't necessary since f9a8ee8c8bcd118e800d88772c6457381db45224,
please drop the assignment to base.
> + priv->chip.npwm = ASPEED_NR_PWMS;
> + priv->chip.of_xlate = of_pwm_xlate_with_flags;
> + priv->chip.of_pwm_n_cells = 3;
> +
> + ret = pwmchip_add(&priv->chip);
> + if (ret < 0) {
> + dev_err(&pdev->dev, "failed to add PWM chip: %d\n", ret);
Please use %pe to make the error messages better readable.
> + return ret;
> + }
> + dev_set_drvdata(dev, priv);
> + return ret;
> +}
> +
> +static const struct of_device_id of_pwm_match_table[] = {
> + {
> + .compatible = "aspeed,ast2600-pwm",
> + },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, of_pwm_match_table);
> +
> +static struct platform_driver aspeed_pwm_driver = {
> + .probe = aspeed_pwm_probe,
Please implement a .remove callback.
> + .driver = {
> + .name = "aspeed_pwm",
> + .of_match_table = of_pwm_match_table,
> + },
> +};
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/4] dt-bindings: Add bindings for aspeed pwm-tach.
2021-04-12 9:54 ` Billy Tsai
@ 2021-04-12 12:55 ` Uwe Kleine-König
-1 siblings, 0 replies; 26+ messages in thread
From: Uwe Kleine-König @ 2021-04-12 12:55 UTC (permalink / raw)
To: Billy Tsai
Cc: lee.jones, robh+dt, joel, andrew, thierry.reding, p.zabel,
billy_tasi, devicetree, linux-arm-kernel, linux-aspeed,
linux-kernel, linux-pwm, BMC-SW
[-- Attachment #1: Type: text/plain, Size: 378 bytes --]
Hello,
On Mon, Apr 12, 2021 at 05:54:54PM +0800, Billy Tsai wrote:
> + - Billy Tsai <billy_tasi@aspeedtech.com>
I object because the MTA at aspeedtech.com doesn't know this email
address.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/4] dt-bindings: Add bindings for aspeed pwm-tach.
@ 2021-04-12 12:55 ` Uwe Kleine-König
0 siblings, 0 replies; 26+ messages in thread
From: Uwe Kleine-König @ 2021-04-12 12:55 UTC (permalink / raw)
To: Billy Tsai
Cc: lee.jones, robh+dt, joel, andrew, thierry.reding, p.zabel,
billy_tasi, devicetree, linux-arm-kernel, linux-aspeed,
linux-kernel, linux-pwm, BMC-SW
[-- Attachment #1.1: Type: text/plain, Size: 378 bytes --]
Hello,
On Mon, Apr 12, 2021 at 05:54:54PM +0800, Billy Tsai wrote:
> + - Billy Tsai <billy_tasi@aspeedtech.com>
I object because the MTA at aspeedtech.com doesn't know this email
address.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 2/4] dt-bindings: Add bindings for aspeed pwm
2021-04-12 9:54 ` Billy Tsai
@ 2021-04-12 13:20 ` Rob Herring
-1 siblings, 0 replies; 26+ messages in thread
From: Rob Herring @ 2021-04-12 13:20 UTC (permalink / raw)
To: Billy Tsai
Cc: u.kleine-koenig, devicetree, linux-aspeed, andrew, linux-kernel,
billy_tasi, p.zabel, linux-pwm, joel, thierry.reding, robh+dt,
BMC-SW, linux-arm-kernel, lee.jones
On Mon, 12 Apr 2021 17:54:55 +0800, Billy Tsai wrote:
> This patch adds device bindings for aspeed pwm device which should be
> the sub-node of aspeed,ast2600-pwm-tach.
>
> Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
> ---
> .../bindings/pwm/aspeed,ast2600-pwm.yaml | 47 +++++++++++++++++++
> 1 file changed, 47 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/pwm/aspeed,ast2600-pwm.yaml
>
My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):
yamllint warnings/errors:
dtschema/dtc warnings/errors:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/pwm/aspeed,ast2600-pwm.yaml: Additional properties are not allowed ('pwm-cells' was unexpected)
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/pwm/aspeed,ast2600-pwm.yaml: Additional properties are not allowed ('pwm-cells' was unexpected)
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/pwm/aspeed,ast2600-pwm.yaml: ignoring, error in schema:
warning: no schema found in file: ./Documentation/devicetree/bindings/pwm/aspeed,ast2600-pwm.yaml
Documentation/devicetree/bindings/pwm/aspeed,ast2600-pwm.example.dt.yaml:0:0: /example-0/pwm_tach@1e610000: failed to match any schema with compatible: ['aspeed,ast2600-pwm-tach', 'simple-mfd', 'syscon']
Documentation/devicetree/bindings/pwm/aspeed,ast2600-pwm.example.dt.yaml:0:0: /example-0/pwm_tach@1e610000/pwm@0: failed to match any schema with compatible: ['aspeed,ast2600-pwm']
See https://patchwork.ozlabs.org/patch/1465116
This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.
If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:
pip3 install dtschema --upgrade
Please check and re-submit.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 2/4] dt-bindings: Add bindings for aspeed pwm
@ 2021-04-12 13:20 ` Rob Herring
0 siblings, 0 replies; 26+ messages in thread
From: Rob Herring @ 2021-04-12 13:20 UTC (permalink / raw)
To: Billy Tsai
Cc: u.kleine-koenig, devicetree, linux-aspeed, andrew, linux-kernel,
billy_tasi, p.zabel, linux-pwm, joel, thierry.reding, robh+dt,
BMC-SW, linux-arm-kernel, lee.jones
On Mon, 12 Apr 2021 17:54:55 +0800, Billy Tsai wrote:
> This patch adds device bindings for aspeed pwm device which should be
> the sub-node of aspeed,ast2600-pwm-tach.
>
> Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
> ---
> .../bindings/pwm/aspeed,ast2600-pwm.yaml | 47 +++++++++++++++++++
> 1 file changed, 47 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/pwm/aspeed,ast2600-pwm.yaml
>
My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):
yamllint warnings/errors:
dtschema/dtc warnings/errors:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/pwm/aspeed,ast2600-pwm.yaml: Additional properties are not allowed ('pwm-cells' was unexpected)
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/pwm/aspeed,ast2600-pwm.yaml: Additional properties are not allowed ('pwm-cells' was unexpected)
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/pwm/aspeed,ast2600-pwm.yaml: ignoring, error in schema:
warning: no schema found in file: ./Documentation/devicetree/bindings/pwm/aspeed,ast2600-pwm.yaml
Documentation/devicetree/bindings/pwm/aspeed,ast2600-pwm.example.dt.yaml:0:0: /example-0/pwm_tach@1e610000: failed to match any schema with compatible: ['aspeed,ast2600-pwm-tach', 'simple-mfd', 'syscon']
Documentation/devicetree/bindings/pwm/aspeed,ast2600-pwm.example.dt.yaml:0:0: /example-0/pwm_tach@1e610000/pwm@0: failed to match any schema with compatible: ['aspeed,ast2600-pwm']
See https://patchwork.ozlabs.org/patch/1465116
This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.
If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:
pip3 install dtschema --upgrade
Please check and re-submit.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 4/4] pwm: Add support for aspeed pwm controller
2021-04-12 11:14 ` Uwe Kleine-König
@ 2021-04-13 2:11 ` Billy Tsai
-1 siblings, 0 replies; 26+ messages in thread
From: Billy Tsai @ 2021-04-13 2:11 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: lee.jones, robh+dt, joel, andrew, thierry.reding, p.zabel,
billy_tasi, devicetree, linux-arm-kernel, linux-aspeed,
linux-kernel, linux-pwm, BMC-SW
Thanks for your review
Best Regards,
Billy Tsai
On 2021/4/12, 7:14 PM,Uwe Kleine-Königwrote:
>Hello,
>On Mon, Apr 12, 2021 at 05:54:57PM +0800, Billy Tsai wrote:
>> Add support for the pwm controller which can be found at aspeed ast2600
>> soc. This driver is part function of multi-funciton of device "pwm-tach
>> controller".
>please squash this into patch 3.
OK, I will squash it when sending v2.
>> Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
>> ---
>> drivers/pwm/Kconfig | 6 ++++++
>> drivers/pwm/Makefile | 1 +
>> 2 files changed, 7 insertions(+)
>>
>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>> index 63be5362fd3a..947ed642debe 100644
>> --- a/drivers/pwm/Kconfig
>> +++ b/drivers/pwm/Kconfig
>> @@ -42,6 +42,12 @@ config PWM_DEBUG
>> It is expected to introduce some runtime overhead and diagnostic
>> output to the kernel log, so only enable while working on a driver.
>>
>> +config PWM_ASPEED_G6
>> + tristate "ASPEEDG6 PWM support"
>No depends?
I will add "depends on (ARCH_ASPEED || COMPILE_TEST)" for this configure.
>Best regards
>Uwe
>--
>Pengutronix e.K. | Uwe Kleine-König |
>Industrial Linux Solutions | https://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 4/4] pwm: Add support for aspeed pwm controller
@ 2021-04-13 2:11 ` Billy Tsai
0 siblings, 0 replies; 26+ messages in thread
From: Billy Tsai @ 2021-04-13 2:11 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: lee.jones, robh+dt, joel, andrew, thierry.reding, p.zabel,
billy_tasi, devicetree, linux-arm-kernel, linux-aspeed,
linux-kernel, linux-pwm, BMC-SW
Thanks for your review
Best Regards,
Billy Tsai
On 2021/4/12, 7:14 PM,Uwe Kleine-Königwrote:
>Hello,
>On Mon, Apr 12, 2021 at 05:54:57PM +0800, Billy Tsai wrote:
>> Add support for the pwm controller which can be found at aspeed ast2600
>> soc. This driver is part function of multi-funciton of device "pwm-tach
>> controller".
>please squash this into patch 3.
OK, I will squash it when sending v2.
>> Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
>> ---
>> drivers/pwm/Kconfig | 6 ++++++
>> drivers/pwm/Makefile | 1 +
>> 2 files changed, 7 insertions(+)
>>
>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>> index 63be5362fd3a..947ed642debe 100644
>> --- a/drivers/pwm/Kconfig
>> +++ b/drivers/pwm/Kconfig
>> @@ -42,6 +42,12 @@ config PWM_DEBUG
>> It is expected to introduce some runtime overhead and diagnostic
>> output to the kernel log, so only enable while working on a driver.
>>
>> +config PWM_ASPEED_G6
>> + tristate "ASPEEDG6 PWM support"
>No depends?
I will add "depends on (ARCH_ASPEED || COMPILE_TEST)" for this configure.
>Best regards
>Uwe
>--
>Pengutronix e.K. | Uwe Kleine-König |
>Industrial Linux Solutions | https://www.pengutronix.de/ |
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/4] dt-bindings: Add bindings for aspeed pwm-tach.
2021-04-12 12:55 ` Uwe Kleine-König
@ 2021-04-13 6:27 ` Billy Tsai
-1 siblings, 0 replies; 26+ messages in thread
From: Billy Tsai @ 2021-04-13 6:27 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: lee.jones, robh+dt, joel, andrew, thierry.reding, p.zabel,
billy_tasi, devicetree, linux-arm-kernel, linux-aspeed,
linux-kernel, linux-pwm, BMC-SW
Hi,
Best Regards,
Billy Tsai
On 2021/4/12, 8:55 PM,Uwe Kleine-Königwrote:
> Hello,
On Mon, Apr 12, 2021 at 05:54:54PM +0800, Billy Tsai wrote:
> + - Billy Tsai <billy_tasi@aspeedtech.com>
> I object because the MTA at aspeedtech.com doesn't know this email
> address.
This is typo error, my email address is billy_tsai@aspeedtech.com
I will fix it at v2.
> Best regards
> Uwe
> --
> Pengutronix e.K. | Uwe Kleine-König |
> Industrial Linux Solutions | https://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/4] dt-bindings: Add bindings for aspeed pwm-tach.
@ 2021-04-13 6:27 ` Billy Tsai
0 siblings, 0 replies; 26+ messages in thread
From: Billy Tsai @ 2021-04-13 6:27 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: lee.jones, robh+dt, joel, andrew, thierry.reding, p.zabel,
billy_tasi, devicetree, linux-arm-kernel, linux-aspeed,
linux-kernel, linux-pwm, BMC-SW
Hi,
Best Regards,
Billy Tsai
On 2021/4/12, 8:55 PM,Uwe Kleine-Königwrote:
> Hello,
On Mon, Apr 12, 2021 at 05:54:54PM +0800, Billy Tsai wrote:
> + - Billy Tsai <billy_tasi@aspeedtech.com>
> I object because the MTA at aspeedtech.com doesn't know this email
> address.
This is typo error, my email address is billy_tsai@aspeedtech.com
I will fix it at v2.
> Best regards
> Uwe
> --
> Pengutronix e.K. | Uwe Kleine-König |
> Industrial Linux Solutions | https://www.pengutronix.de/ |
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 2/4] dt-bindings: Add bindings for aspeed pwm
2021-04-12 13:20 ` Rob Herring
@ 2021-04-13 7:27 ` Billy Tsai
-1 siblings, 0 replies; 26+ messages in thread
From: Billy Tsai @ 2021-04-13 7:27 UTC (permalink / raw)
To: Rob Herring
Cc: u.kleine-koenig, devicetree, linux-aspeed, andrew, linux-kernel,
p.zabel, linux-pwm, joel, thierry.reding, robh+dt, BMC-SW,
linux-arm-kernel, lee.jones
Hi Rob,
Best Regards,
Billy Tsai
On 2021/4/12, 9:20 PM,Rob Herringwrote:
On Mon, 12 Apr 2021 17:54:55 +0800, Billy Tsai wrote:
>> This patch adds device bindings for aspeed pwm device which should be
>> the sub-node of aspeed,ast2600-pwm-tach.
>>
>> Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
>> ---
>> .../bindings/pwm/aspeed,ast2600-pwm.yaml | 47 +++++++++++++++++++
>> 1 file changed, 47 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/pwm/aspeed,ast2600-pwm.yaml
>>
> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> on your patch (DT_CHECKER_FLAGS is new in v5.13):
> yamllint warnings/errors:
> dtschema/dtc warnings/errors:
> /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/pwm/aspeed,ast2600-pwm.yaml: Additional properties are not allowed ('pwm-cells' was unexpected)
> /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/pwm/aspeed,ast2600-pwm.yaml: Additional properties are not allowed ('pwm-cells' was unexpected)
> /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/pwm/aspeed,ast2600-pwm.yaml: ignoring, error in schema:
> warning: no schema found in file: ./Documentation/devicetree/bindings/pwm/aspeed,ast2600-pwm.yaml
> Documentation/devicetree/bindings/pwm/aspeed,ast2600-pwm.example.dt.yaml:0:0: /example-0/pwm_tach@1e610000: failed to match any schema with compatible: ['aspeed,ast2600-pwm-tach', 'simple-mfd', 'syscon']
> Documentation/devicetree/bindings/pwm/aspeed,ast2600-pwm.example.dt.yaml:0:0: /example-0/pwm_tach@1e610000/pwm@0: failed to match any schema with compatible: ['aspeed,ast2600-pwm']
The yaml file have dependencies with the first patch in these series. I will squash them.
> See https://patchwork.ozlabs.org/patch/1465116
> This check can fail if there are any dependencies. The base for a patch
> series is generally the most recent rc1.
> If you already ran 'make dt_binding_check' and didn't see the above
> error(s), then make sure 'yamllint' is installed and dt-schema is up to
> date:
> pip3 install dtschema --upgrade
> Please check and re-submit.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 2/4] dt-bindings: Add bindings for aspeed pwm
@ 2021-04-13 7:27 ` Billy Tsai
0 siblings, 0 replies; 26+ messages in thread
From: Billy Tsai @ 2021-04-13 7:27 UTC (permalink / raw)
To: Rob Herring
Cc: u.kleine-koenig, devicetree, linux-aspeed, andrew, linux-kernel,
p.zabel, linux-pwm, joel, thierry.reding, robh+dt, BMC-SW,
linux-arm-kernel, lee.jones
Hi Rob,
Best Regards,
Billy Tsai
On 2021/4/12, 9:20 PM,Rob Herringwrote:
On Mon, 12 Apr 2021 17:54:55 +0800, Billy Tsai wrote:
>> This patch adds device bindings for aspeed pwm device which should be
>> the sub-node of aspeed,ast2600-pwm-tach.
>>
>> Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
>> ---
>> .../bindings/pwm/aspeed,ast2600-pwm.yaml | 47 +++++++++++++++++++
>> 1 file changed, 47 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/pwm/aspeed,ast2600-pwm.yaml
>>
> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> on your patch (DT_CHECKER_FLAGS is new in v5.13):
> yamllint warnings/errors:
> dtschema/dtc warnings/errors:
> /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/pwm/aspeed,ast2600-pwm.yaml: Additional properties are not allowed ('pwm-cells' was unexpected)
> /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/pwm/aspeed,ast2600-pwm.yaml: Additional properties are not allowed ('pwm-cells' was unexpected)
> /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/pwm/aspeed,ast2600-pwm.yaml: ignoring, error in schema:
> warning: no schema found in file: ./Documentation/devicetree/bindings/pwm/aspeed,ast2600-pwm.yaml
> Documentation/devicetree/bindings/pwm/aspeed,ast2600-pwm.example.dt.yaml:0:0: /example-0/pwm_tach@1e610000: failed to match any schema with compatible: ['aspeed,ast2600-pwm-tach', 'simple-mfd', 'syscon']
> Documentation/devicetree/bindings/pwm/aspeed,ast2600-pwm.example.dt.yaml:0:0: /example-0/pwm_tach@1e610000/pwm@0: failed to match any schema with compatible: ['aspeed,ast2600-pwm']
The yaml file have dependencies with the first patch in these series. I will squash them.
> See https://patchwork.ozlabs.org/patch/1465116
> This check can fail if there are any dependencies. The base for a patch
> series is generally the most recent rc1.
> If you already ran 'make dt_binding_check' and didn't see the above
> error(s), then make sure 'yamllint' is installed and dt-schema is up to
> date:
> pip3 install dtschema --upgrade
> Please check and re-submit.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 3/4] pwm: Add Aspeed ast2600 PWM support
2021-04-12 12:35 ` Uwe Kleine-König
@ 2021-04-13 9:24 ` Billy Tsai
-1 siblings, 0 replies; 26+ messages in thread
From: Billy Tsai @ 2021-04-13 9:24 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: lee.jones, robh+dt, joel, andrew, thierry.reding, p.zabel,
devicetree, linux-arm-kernel, linux-aspeed, linux-kernel,
linux-pwm, BMC-SW
Thanks for your review
Best Regards,
Billy Tsai
On 2021/4/12, 8:35 PM,Uwe Kleine-Königwrote:
Hello Billy,
On Mon, Apr 12, 2021 at 05:54:56PM +0800, Billy Tsai wrote:
>> This patch add the support of PWM controller which can find at aspeed
>> ast2600 soc chip. This controller supoorts up to 16 channels.
>>
>> Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
>> ---
>> drivers/pwm/pwm-aspeed-g6.c | 291 ++++++++++++++++++++++++++++++++++++
>> 1 file changed, 291 insertions(+)
>> create mode 100644 drivers/pwm/pwm-aspeed-g6.c
>>
>> diff --git a/drivers/pwm/pwm-aspeed-g6.c b/drivers/pwm/pwm-aspeed-g6.c
>> new file mode 100644
>> index 000000000000..4bb4f97453c6
>> --- /dev/null
>> +++ b/drivers/pwm/pwm-aspeed-g6.c
>> @@ -0,0 +1,291 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (C) ASPEED Technology Inc.
> Don't you need to add a year here?
Got it.
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 or later as
>> + * published by the Free Software Foundation.
> Hmm, the comment and the SPDX-License-Identifier contradict each other.
> The idea of the latter is that the former isn't needed.
I will use "// SPDX-License-Identifier: GPL-2.0-or-later" for the license.
>> + */
> Is there documentation available in the internet for this hardware? If
> yes, please mention a link here.
Sorry we don't have the hardware document in the internet.
> Also describe the hardware here similar to how e.g.
> drivers/pwm/pwm-sifive.c does it. Please stick to the same format for
> easy grepping.
Got it.
>> +
>> +#include <linux/clk.h>
>> +#include <linux/errno.h>
>> +#include <linux/delay.h>
>> +#include <linux/io.h>
>> +#include <linux/kernel.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/module.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/of_device.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/sysfs.h>
>> +#include <linux/reset.h>
>> +#include <linux/regmap.h>
>> +#include <linux/pwm.h>
>> +/* The channel number of Aspeed pwm controller */
>> +#define ASPEED_NR_PWMS 16
>> +/* PWM Control Register */
>> +#define ASPEED_PWM_CTRL_CH(ch) ((ch * 0x10) + 0x00)
> #define ASPEED_PWM_CTRL_CH(ch) (((ch) * 0x10) + 0x00)
Got it.
>> +#define PWM_LOAD_SEL_AS_WDT BIT(19)
>> +#define LOAD_SEL_FALLING 0
>> +#define LOAD_SEL_RIGING 1
>> +#define PWM_DUTY_LOAD_AS_WDT_EN BIT(18)
>> +#define PWM_DUTY_SYNC_DIS BIT(17)
>> +#define PWM_CLK_ENABLE BIT(16)
>> +#define PWM_LEVEL_OUTPUT BIT(15)
>> +#define PWM_INVERSE BIT(14)
>> +#define PWM_OPEN_DRAIN_EN BIT(13)
>> +#define PWM_PIN_EN BIT(12)
>> +#define PWM_CLK_DIV_H_SHIFT 8
>> +#define PWM_CLK_DIV_H_MASK (0xf << PWM_CLK_DIV_H_SHIFT)
>> +#define PWM_CLK_DIV_L_SHIFT 0
>> +#define PWM_CLK_DIV_L_MASK (0xff << PWM_CLK_DIV_L_SHIFT)
>> +/* PWM Duty Cycle Register */
>> +#define ASPEED_PWM_DUTY_CYCLE_CH(ch) ((ch * 0x10) + 0x04)
>> +#define PWM_PERIOD_SHIFT (24)
>> +#define PWM_PERIOD_MASK (0xff << PWM_PERIOD_SHIFT)
>> +#define PWM_POINT_AS_WDT_SHIFT (16)
>> +#define PWM_POINT_AS_WDT_MASK (0xff << PWM_POINT_AS_WDT_SHIFT)
>> +#define PWM_FALLING_POINT_SHIFT (8)
>> +#define PWM_FALLING_POINT_MASK (0xffff << PWM_FALLING_POINT_SHIFT)
>> +#define PWM_RISING_POINT_SHIFT (0)
>> +#define PWM_RISING_POINT_MASK (0xffff << PWM_RISING_POINT_SHIFT)
>> +/* PWM default value */
>> +#define DEFAULT_PWM_PERIOD 0xff
>> +#define DEFAULT_TARGET_PWM_FREQ 25000
>> +#define DEFAULT_DUTY_PT 10
>> +#define DEFAULT_WDT_RELOAD_DUTY_PT 16
> You could spend a few empty lines to make this better readable. Also
> please use a consistent driver-specific prefix for your defines and
> consider using the macros from <linux/bitfield.h>. Also defines
> for bitfields should contain the register name.
Got it. I will use the bitfield method to write the hardware register.
>> +struct aspeed_pwm_data {
>> + struct pwm_chip chip;
>> + struct regmap *regmap;
>> + unsigned long clk_freq;
>> + struct reset_control *reset;
>> +};
>> +/**
>> + * struct aspeed_pwm - per-PWM driver data
>> + * @freq: cached pwm freq
>> + */
>> +struct aspeed_pwm {
>> + u32 freq;
>> +};
> This is actually unused, please drop it. (You save a value in it, but
> make never use of it.)
Got it.
>> +static void aspeed_set_pwm_channel_enable(struct regmap *regmap, u8 pwm_channel,
>> + bool enable)
>> +{
>> + regmap_update_bits(regmap, ASPEED_PWM_CTRL_CH(pwm_channel),
>> + (PWM_CLK_ENABLE | PWM_PIN_EN),
>> + enable ? (PWM_CLK_ENABLE | PWM_PIN_EN) : 0);
> What is the semantic of PIN_EN?
It means PIN_ENABLE. I will complete the defined name with PWM_PIN_ENABLE.
>> +}
>> +/*
>> + * The PWM frequency = HCLK(200Mhz) / (clock division L bit *
>> + * clock division H bit * (period bit + 1))
>> + */
>> +static void aspeed_set_pwm_freq(struct aspeed_pwm_data *priv,
>> + struct pwm_device *pwm, u32 freq)
>> +{
>> + u32 target_div, cal_freq;
>> + u32 tmp_div_h, tmp_div_l, diff, min_diff = INT_MAX;
>> + u32 div_h = BIT(5) - 1, div_l = BIT(8) - 1;
>> + u8 div_found;
>> + u32 index = pwm->hwpwm;
>> + struct aspeed_pwm *channel = pwm_get_chip_data(pwm);
>> +
>> + cal_freq = priv->clk_freq / (DEFAULT_PWM_PERIOD + 1);
>> + target_div = DIV_ROUND_UP(cal_freq, freq);
>> + div_found = 0;
>> + /* calculate for target frequence */
> s/frequence/frequency/
Got it.
>> + for (tmp_div_h = 0; tmp_div_h < 0x10; tmp_div_h++) {
>> + tmp_div_l = target_div / BIT(tmp_div_h) - 1;
>> +
>> + if (tmp_div_l < 0 || tmp_div_l >> 255)
>> + continue;
>> +
>> + diff = freq - cal_freq / (BIT(tmp_div_h) * (tmp_div_l + 1));
>> + if (abs(diff) < abs(min_diff)) {
>> + min_diff = diff;
>> + div_l = tmp_div_l;
>> + div_h = tmp_div_h;
>> + div_found = 1;
>> + if (diff == 0)
>> + break;
>> + }
>> + }
> If my understanding is right (i.e. H divides by a power of two and L by
> an integer) this can be simplified.
Yes, the formula of the frequency is:
HCLK / ((2 ** H divide) * L divide * PERIOD value)
I think the simplified way is using the bit shift, right?
>> + if (div_found == 0) {
>> + pr_debug("target freq: %d too slow set minimal frequency\n",
>> + freq);
>> + }
>> + channel->freq = cal_freq / (BIT(div_h) * (div_l + 1));
>> + pr_debug("div h %x, l : %x pwm out clk %d\n", div_h, div_l,
>> + channel->freq);
>> + pr_debug("hclk %ld, target pwm freq %d, real pwm freq %d\n",
>> + priv->clk_freq, freq, channel->freq);
>> +
>> + regmap_update_bits(priv->regmap, ASPEED_PWM_CTRL_CH(index),
>> + (PWM_CLK_DIV_H_MASK | PWM_CLK_DIV_L_MASK),
>> + (div_h << PWM_CLK_DIV_H_SHIFT) |
>> + (div_l << PWM_CLK_DIV_L_SHIFT));
>> +}
>> +
>> +static void aspeed_set_pwm_duty(struct aspeed_pwm_data *priv,
>> + struct pwm_device *pwm, u32 duty_pt)
>> +{
>> + u32 index = pwm->hwpwm;
>> +
>> + if (duty_pt == 0) {
>> + aspeed_set_pwm_channel_enable(priv->regmap, index, false);
>> + } else {
>> + regmap_update_bits(priv->regmap,
>> + ASPEED_PWM_DUTY_CYCLE_CH(index),
>> + PWM_FALLING_POINT_MASK,
>> + duty_pt << PWM_FALLING_POINT_SHIFT);
>> + aspeed_set_pwm_channel_enable(priv->regmap, index, true);
>> + }
>> +}
>> +
>> +static void aspeed_set_pwm_polarity(struct aspeed_pwm_data *priv,
>> + struct pwm_device *pwm, u8 polarity)
>> +{
>> + u32 index = pwm->hwpwm;
>> +
>> + regmap_update_bits(priv->regmap, ASPEED_PWM_CTRL_CH(index), PWM_INVERSE,
>> + (polarity) ? PWM_INVERSE : 0);
>> +}
>> +
>> +static int aspeed_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
>> +{
>> + struct device *dev = chip->dev;
>> + struct aspeed_pwm_data *priv = dev_get_drvdata(dev);
>> + struct aspeed_pwm *channel;
>> + u32 index = pwm->hwpwm;
>> + /*
>> + * Fixed the period to the max value and rising point to 0
>> + * for high resolution and simplified frequency calculation.
> s/^H//
Sorry, I don't understand this mean.
>> + */
>> + regmap_update_bits(priv->regmap, ASPEED_PWM_DUTY_CYCLE_CH(index),
>> + PWM_PERIOD_MASK,
>> + DEFAULT_PWM_PERIOD << PWM_PERIOD_SHIFT);
>> +
>> + regmap_update_bits(priv->regmap, ASPEED_PWM_DUTY_CYCLE_CH(index),
>> + PWM_RISING_POINT_MASK, 0);
> Only .apply is supposed to modify the PWM's configuration.
This is the initial value and the fixed(const) value for our pwm driver usage.
The value won't be modified, so I think I can initial it when pwm channel be requested.
>> + channel = devm_kzalloc(dev, sizeof(*channel), GFP_KERNEL);
>> + if (!channel)
>> + return -ENOMEM;
> Don't use devm_kzalloc if freeing isn't done at device cleanup time.
This doesn't depend on device, so I can use "kzalloc" to replace it?
>> + pwm_set_chip_data(pwm, channel);
>> +
>> + return 0;
>> +}
>> +
>> +static void aspeed_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
>> +{
>> + struct device *dev = chip->dev;
>> + struct aspeed_pwm *channel = pwm_get_chip_data(pwm);
>> +
>> + devm_kfree(dev, channel);
>> +}
When pwm free I need to use kfree to release the resources, right?
>> +
>> +static int aspeed_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>> + const struct pwm_state *state)
>> +{
>> + struct device *dev = chip->dev;
>> + struct aspeed_pwm_data *priv = dev_get_drvdata(dev);
>Please consider using
>
> priv = container_of(chip, struct aspeed_pwm_data, chip);
>
>(preferably wrapped in a macro) which is more type safe and more
>effective to calculate.
Got it.
>> + struct pwm_state *cur_state = &pwm->state;
>> + u32 freq = DIV_ROUND_UP_ULL(1000000000, state->period);
>> + u32 duty_pt = DIV_ROUND_UP_ULL(
>> + state->duty_cycle * (DEFAULT_PWM_PERIOD + 1), state->period);
> You're loosing precision here.
>> + dev_dbg(dev, "freq: %d, duty_pt: %d", freq, duty_pt);
>> + if (state->enabled) {
>> + aspeed_set_pwm_freq(priv, pwm, freq);
>> + aspeed_set_pwm_duty(priv, pwm, duty_pt);
>> + aspeed_set_pwm_polarity(priv, pwm, state->polarity);
> How does the hardware behave between these calls? E.g. can it happen
> that it already emits a normal period when inversed polarity is
> requested just before aspeed_set_pwm_polarity is called? Or there is a
> period with the previous duty cycle and the new period?
It will emits a normal period first and inversed the pwm duty instantly or wait one period
after aspeed_set_pwm_polarity is called depends on the bit PWM_DUTY_SYNC_DIS.
Does the pwm driver have expected behavior when apply polarity changed?
>> + } else {
>> + aspeed_set_pwm_duty(priv, pwm, 0);
>> + }
>> + cur_state->period = state->period;
>> + cur_state->duty_cycle = state->duty_cycle;
>> + cur_state->polarity = state->polarity;
>> + cur_state->enabled = state->enabled;
> The driver is not supposed to modify pwm->state.
Ok, I will remove it and use chip data to store it for .get_status api.
>> + return 0;
>> +}
> From your code I understood: The period of the signal is
>
> (PWM_PERIOD + 1) * (2 ** DIV_H) * (DIV_L + 1) / 200 MHz
> . The duty cycle is
>
> PWM_FALLING_POINT * (2 ** DIV_H) * (DIV_L + 1) / 200 MHz
> . So the PWM cannot provide a 100% relative duty cycle.
> Is this right?
No, If you want to set 100% duty cycle you can set the PWM_FALLING_POINT value
to same as PWM_RISING_POINT (we fixed it to 0).
>> +static const struct pwm_ops aspeed_pwm_ops = {
>> + .request = aspeed_pwm_request,
>> + .free = aspeed_pwm_free,
>> + .apply = aspeed_pwm_apply,
> Please test your driver with PWM_DEBUG enabled.
Got it.
>> + .owner = THIS_MODULE,
>> +};
>> +
>> +static int aspeed_pwm_probe(struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + struct clk *clk;
>> + int ret;
>> + struct aspeed_pwm_data *priv;
>> + struct device_node *np;
>> +
>> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>> + if (!priv)
>> + return -ENOMEM;
>> +
>> + np = pdev->dev.parent->of_node;
>> + if (!of_device_is_compatible(np, "aspeed,ast2600-pwm-tach")) {
>> + dev_err(dev, "unsupported pwm device binding\n");
>> + return -ENODEV;
>> + }
> Is this pwm-tach an mfd?
Yes, It is an mfd.
>> + priv->regmap = syscon_node_to_regmap(np);
>> + if (IS_ERR(priv->regmap)) {
>> + dev_err(dev, "Couldn't get regmap\n");
>> + return -ENODEV;
>> + }
>> +
>> + clk = devm_clk_get(dev, NULL);
>> + if (IS_ERR(clk))
>> + return -ENODEV;
>> + priv->clk_freq = clk_get_rate(clk);
> If you intend to use this clock, you have to enable it.
Got it.
>> + priv->reset = reset_control_get_shared(&pdev->dev, NULL);
>> + if (IS_ERR(priv->reset)) {
>> + dev_err(&pdev->dev, "can't get aspeed_pwm_tacho reset\n");
>> + return PTR_ERR(priv->reset);
>> + }
>> + reset_control_deassert(priv->reset);
> missing error checking
Got it.
>> + priv->chip.dev = dev;
>> + priv->chip.ops = &aspeed_pwm_ops;
>> + priv->chip.base = -1;
> This isn't necessary since f9a8ee8c8bcd118e800d88772c6457381db45224,
> please drop the assignment to base.
Got it.
>> + priv->chip.npwm = ASPEED_NR_PWMS;
>> + priv->chip.of_xlate = of_pwm_xlate_with_flags;
>> + priv->chip.of_pwm_n_cells = 3;
>> +
>> + ret = pwmchip_add(&priv->chip);
>> + if (ret < 0) {
>> + dev_err(&pdev->dev, "failed to add PWM chip: %d\n", ret);
> Please use %pe to make the error messages better readable.
Got it.
>> + return ret;
>> + }
>> + dev_set_drvdata(dev, priv);
>> + return ret;
>> +}
>> +
>> +static const struct of_device_id of_pwm_match_table[] = {
>> + {
>> + .compatible = "aspeed,ast2600-pwm",
>> + },
>> + {},
>> +};
>> +MODULE_DEVICE_TABLE(of, of_pwm_match_table);
>> +
>> +static struct platform_driver aspeed_pwm_driver = {
>> + .probe = aspeed_pwm_probe,
> Please implement a .remove callback.
Got it.
>> + .driver = {
>> + .name = "aspeed_pwm",
>> + .of_match_table = of_pwm_match_table,
>> + },
>> +};
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 3/4] pwm: Add Aspeed ast2600 PWM support
@ 2021-04-13 9:24 ` Billy Tsai
0 siblings, 0 replies; 26+ messages in thread
From: Billy Tsai @ 2021-04-13 9:24 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: lee.jones, robh+dt, joel, andrew, thierry.reding, p.zabel,
devicetree, linux-arm-kernel, linux-aspeed, linux-kernel,
linux-pwm, BMC-SW
Thanks for your review
Best Regards,
Billy Tsai
On 2021/4/12, 8:35 PM,Uwe Kleine-Königwrote:
Hello Billy,
On Mon, Apr 12, 2021 at 05:54:56PM +0800, Billy Tsai wrote:
>> This patch add the support of PWM controller which can find at aspeed
>> ast2600 soc chip. This controller supoorts up to 16 channels.
>>
>> Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
>> ---
>> drivers/pwm/pwm-aspeed-g6.c | 291 ++++++++++++++++++++++++++++++++++++
>> 1 file changed, 291 insertions(+)
>> create mode 100644 drivers/pwm/pwm-aspeed-g6.c
>>
>> diff --git a/drivers/pwm/pwm-aspeed-g6.c b/drivers/pwm/pwm-aspeed-g6.c
>> new file mode 100644
>> index 000000000000..4bb4f97453c6
>> --- /dev/null
>> +++ b/drivers/pwm/pwm-aspeed-g6.c
>> @@ -0,0 +1,291 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (C) ASPEED Technology Inc.
> Don't you need to add a year here?
Got it.
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 or later as
>> + * published by the Free Software Foundation.
> Hmm, the comment and the SPDX-License-Identifier contradict each other.
> The idea of the latter is that the former isn't needed.
I will use "// SPDX-License-Identifier: GPL-2.0-or-later" for the license.
>> + */
> Is there documentation available in the internet for this hardware? If
> yes, please mention a link here.
Sorry we don't have the hardware document in the internet.
> Also describe the hardware here similar to how e.g.
> drivers/pwm/pwm-sifive.c does it. Please stick to the same format for
> easy grepping.
Got it.
>> +
>> +#include <linux/clk.h>
>> +#include <linux/errno.h>
>> +#include <linux/delay.h>
>> +#include <linux/io.h>
>> +#include <linux/kernel.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/module.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/of_device.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/sysfs.h>
>> +#include <linux/reset.h>
>> +#include <linux/regmap.h>
>> +#include <linux/pwm.h>
>> +/* The channel number of Aspeed pwm controller */
>> +#define ASPEED_NR_PWMS 16
>> +/* PWM Control Register */
>> +#define ASPEED_PWM_CTRL_CH(ch) ((ch * 0x10) + 0x00)
> #define ASPEED_PWM_CTRL_CH(ch) (((ch) * 0x10) + 0x00)
Got it.
>> +#define PWM_LOAD_SEL_AS_WDT BIT(19)
>> +#define LOAD_SEL_FALLING 0
>> +#define LOAD_SEL_RIGING 1
>> +#define PWM_DUTY_LOAD_AS_WDT_EN BIT(18)
>> +#define PWM_DUTY_SYNC_DIS BIT(17)
>> +#define PWM_CLK_ENABLE BIT(16)
>> +#define PWM_LEVEL_OUTPUT BIT(15)
>> +#define PWM_INVERSE BIT(14)
>> +#define PWM_OPEN_DRAIN_EN BIT(13)
>> +#define PWM_PIN_EN BIT(12)
>> +#define PWM_CLK_DIV_H_SHIFT 8
>> +#define PWM_CLK_DIV_H_MASK (0xf << PWM_CLK_DIV_H_SHIFT)
>> +#define PWM_CLK_DIV_L_SHIFT 0
>> +#define PWM_CLK_DIV_L_MASK (0xff << PWM_CLK_DIV_L_SHIFT)
>> +/* PWM Duty Cycle Register */
>> +#define ASPEED_PWM_DUTY_CYCLE_CH(ch) ((ch * 0x10) + 0x04)
>> +#define PWM_PERIOD_SHIFT (24)
>> +#define PWM_PERIOD_MASK (0xff << PWM_PERIOD_SHIFT)
>> +#define PWM_POINT_AS_WDT_SHIFT (16)
>> +#define PWM_POINT_AS_WDT_MASK (0xff << PWM_POINT_AS_WDT_SHIFT)
>> +#define PWM_FALLING_POINT_SHIFT (8)
>> +#define PWM_FALLING_POINT_MASK (0xffff << PWM_FALLING_POINT_SHIFT)
>> +#define PWM_RISING_POINT_SHIFT (0)
>> +#define PWM_RISING_POINT_MASK (0xffff << PWM_RISING_POINT_SHIFT)
>> +/* PWM default value */
>> +#define DEFAULT_PWM_PERIOD 0xff
>> +#define DEFAULT_TARGET_PWM_FREQ 25000
>> +#define DEFAULT_DUTY_PT 10
>> +#define DEFAULT_WDT_RELOAD_DUTY_PT 16
> You could spend a few empty lines to make this better readable. Also
> please use a consistent driver-specific prefix for your defines and
> consider using the macros from <linux/bitfield.h>. Also defines
> for bitfields should contain the register name.
Got it. I will use the bitfield method to write the hardware register.
>> +struct aspeed_pwm_data {
>> + struct pwm_chip chip;
>> + struct regmap *regmap;
>> + unsigned long clk_freq;
>> + struct reset_control *reset;
>> +};
>> +/**
>> + * struct aspeed_pwm - per-PWM driver data
>> + * @freq: cached pwm freq
>> + */
>> +struct aspeed_pwm {
>> + u32 freq;
>> +};
> This is actually unused, please drop it. (You save a value in it, but
> make never use of it.)
Got it.
>> +static void aspeed_set_pwm_channel_enable(struct regmap *regmap, u8 pwm_channel,
>> + bool enable)
>> +{
>> + regmap_update_bits(regmap, ASPEED_PWM_CTRL_CH(pwm_channel),
>> + (PWM_CLK_ENABLE | PWM_PIN_EN),
>> + enable ? (PWM_CLK_ENABLE | PWM_PIN_EN) : 0);
> What is the semantic of PIN_EN?
It means PIN_ENABLE. I will complete the defined name with PWM_PIN_ENABLE.
>> +}
>> +/*
>> + * The PWM frequency = HCLK(200Mhz) / (clock division L bit *
>> + * clock division H bit * (period bit + 1))
>> + */
>> +static void aspeed_set_pwm_freq(struct aspeed_pwm_data *priv,
>> + struct pwm_device *pwm, u32 freq)
>> +{
>> + u32 target_div, cal_freq;
>> + u32 tmp_div_h, tmp_div_l, diff, min_diff = INT_MAX;
>> + u32 div_h = BIT(5) - 1, div_l = BIT(8) - 1;
>> + u8 div_found;
>> + u32 index = pwm->hwpwm;
>> + struct aspeed_pwm *channel = pwm_get_chip_data(pwm);
>> +
>> + cal_freq = priv->clk_freq / (DEFAULT_PWM_PERIOD + 1);
>> + target_div = DIV_ROUND_UP(cal_freq, freq);
>> + div_found = 0;
>> + /* calculate for target frequence */
> s/frequence/frequency/
Got it.
>> + for (tmp_div_h = 0; tmp_div_h < 0x10; tmp_div_h++) {
>> + tmp_div_l = target_div / BIT(tmp_div_h) - 1;
>> +
>> + if (tmp_div_l < 0 || tmp_div_l >> 255)
>> + continue;
>> +
>> + diff = freq - cal_freq / (BIT(tmp_div_h) * (tmp_div_l + 1));
>> + if (abs(diff) < abs(min_diff)) {
>> + min_diff = diff;
>> + div_l = tmp_div_l;
>> + div_h = tmp_div_h;
>> + div_found = 1;
>> + if (diff == 0)
>> + break;
>> + }
>> + }
> If my understanding is right (i.e. H divides by a power of two and L by
> an integer) this can be simplified.
Yes, the formula of the frequency is:
HCLK / ((2 ** H divide) * L divide * PERIOD value)
I think the simplified way is using the bit shift, right?
>> + if (div_found == 0) {
>> + pr_debug("target freq: %d too slow set minimal frequency\n",
>> + freq);
>> + }
>> + channel->freq = cal_freq / (BIT(div_h) * (div_l + 1));
>> + pr_debug("div h %x, l : %x pwm out clk %d\n", div_h, div_l,
>> + channel->freq);
>> + pr_debug("hclk %ld, target pwm freq %d, real pwm freq %d\n",
>> + priv->clk_freq, freq, channel->freq);
>> +
>> + regmap_update_bits(priv->regmap, ASPEED_PWM_CTRL_CH(index),
>> + (PWM_CLK_DIV_H_MASK | PWM_CLK_DIV_L_MASK),
>> + (div_h << PWM_CLK_DIV_H_SHIFT) |
>> + (div_l << PWM_CLK_DIV_L_SHIFT));
>> +}
>> +
>> +static void aspeed_set_pwm_duty(struct aspeed_pwm_data *priv,
>> + struct pwm_device *pwm, u32 duty_pt)
>> +{
>> + u32 index = pwm->hwpwm;
>> +
>> + if (duty_pt == 0) {
>> + aspeed_set_pwm_channel_enable(priv->regmap, index, false);
>> + } else {
>> + regmap_update_bits(priv->regmap,
>> + ASPEED_PWM_DUTY_CYCLE_CH(index),
>> + PWM_FALLING_POINT_MASK,
>> + duty_pt << PWM_FALLING_POINT_SHIFT);
>> + aspeed_set_pwm_channel_enable(priv->regmap, index, true);
>> + }
>> +}
>> +
>> +static void aspeed_set_pwm_polarity(struct aspeed_pwm_data *priv,
>> + struct pwm_device *pwm, u8 polarity)
>> +{
>> + u32 index = pwm->hwpwm;
>> +
>> + regmap_update_bits(priv->regmap, ASPEED_PWM_CTRL_CH(index), PWM_INVERSE,
>> + (polarity) ? PWM_INVERSE : 0);
>> +}
>> +
>> +static int aspeed_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
>> +{
>> + struct device *dev = chip->dev;
>> + struct aspeed_pwm_data *priv = dev_get_drvdata(dev);
>> + struct aspeed_pwm *channel;
>> + u32 index = pwm->hwpwm;
>> + /*
>> + * Fixed the period to the max value and rising point to 0
>> + * for high resolution and simplified frequency calculation.
> s/^H//
Sorry, I don't understand this mean.
>> + */
>> + regmap_update_bits(priv->regmap, ASPEED_PWM_DUTY_CYCLE_CH(index),
>> + PWM_PERIOD_MASK,
>> + DEFAULT_PWM_PERIOD << PWM_PERIOD_SHIFT);
>> +
>> + regmap_update_bits(priv->regmap, ASPEED_PWM_DUTY_CYCLE_CH(index),
>> + PWM_RISING_POINT_MASK, 0);
> Only .apply is supposed to modify the PWM's configuration.
This is the initial value and the fixed(const) value for our pwm driver usage.
The value won't be modified, so I think I can initial it when pwm channel be requested.
>> + channel = devm_kzalloc(dev, sizeof(*channel), GFP_KERNEL);
>> + if (!channel)
>> + return -ENOMEM;
> Don't use devm_kzalloc if freeing isn't done at device cleanup time.
This doesn't depend on device, so I can use "kzalloc" to replace it?
>> + pwm_set_chip_data(pwm, channel);
>> +
>> + return 0;
>> +}
>> +
>> +static void aspeed_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
>> +{
>> + struct device *dev = chip->dev;
>> + struct aspeed_pwm *channel = pwm_get_chip_data(pwm);
>> +
>> + devm_kfree(dev, channel);
>> +}
When pwm free I need to use kfree to release the resources, right?
>> +
>> +static int aspeed_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>> + const struct pwm_state *state)
>> +{
>> + struct device *dev = chip->dev;
>> + struct aspeed_pwm_data *priv = dev_get_drvdata(dev);
>Please consider using
>
> priv = container_of(chip, struct aspeed_pwm_data, chip);
>
>(preferably wrapped in a macro) which is more type safe and more
>effective to calculate.
Got it.
>> + struct pwm_state *cur_state = &pwm->state;
>> + u32 freq = DIV_ROUND_UP_ULL(1000000000, state->period);
>> + u32 duty_pt = DIV_ROUND_UP_ULL(
>> + state->duty_cycle * (DEFAULT_PWM_PERIOD + 1), state->period);
> You're loosing precision here.
>> + dev_dbg(dev, "freq: %d, duty_pt: %d", freq, duty_pt);
>> + if (state->enabled) {
>> + aspeed_set_pwm_freq(priv, pwm, freq);
>> + aspeed_set_pwm_duty(priv, pwm, duty_pt);
>> + aspeed_set_pwm_polarity(priv, pwm, state->polarity);
> How does the hardware behave between these calls? E.g. can it happen
> that it already emits a normal period when inversed polarity is
> requested just before aspeed_set_pwm_polarity is called? Or there is a
> period with the previous duty cycle and the new period?
It will emits a normal period first and inversed the pwm duty instantly or wait one period
after aspeed_set_pwm_polarity is called depends on the bit PWM_DUTY_SYNC_DIS.
Does the pwm driver have expected behavior when apply polarity changed?
>> + } else {
>> + aspeed_set_pwm_duty(priv, pwm, 0);
>> + }
>> + cur_state->period = state->period;
>> + cur_state->duty_cycle = state->duty_cycle;
>> + cur_state->polarity = state->polarity;
>> + cur_state->enabled = state->enabled;
> The driver is not supposed to modify pwm->state.
Ok, I will remove it and use chip data to store it for .get_status api.
>> + return 0;
>> +}
> From your code I understood: The period of the signal is
>
> (PWM_PERIOD + 1) * (2 ** DIV_H) * (DIV_L + 1) / 200 MHz
> . The duty cycle is
>
> PWM_FALLING_POINT * (2 ** DIV_H) * (DIV_L + 1) / 200 MHz
> . So the PWM cannot provide a 100% relative duty cycle.
> Is this right?
No, If you want to set 100% duty cycle you can set the PWM_FALLING_POINT value
to same as PWM_RISING_POINT (we fixed it to 0).
>> +static const struct pwm_ops aspeed_pwm_ops = {
>> + .request = aspeed_pwm_request,
>> + .free = aspeed_pwm_free,
>> + .apply = aspeed_pwm_apply,
> Please test your driver with PWM_DEBUG enabled.
Got it.
>> + .owner = THIS_MODULE,
>> +};
>> +
>> +static int aspeed_pwm_probe(struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + struct clk *clk;
>> + int ret;
>> + struct aspeed_pwm_data *priv;
>> + struct device_node *np;
>> +
>> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>> + if (!priv)
>> + return -ENOMEM;
>> +
>> + np = pdev->dev.parent->of_node;
>> + if (!of_device_is_compatible(np, "aspeed,ast2600-pwm-tach")) {
>> + dev_err(dev, "unsupported pwm device binding\n");
>> + return -ENODEV;
>> + }
> Is this pwm-tach an mfd?
Yes, It is an mfd.
>> + priv->regmap = syscon_node_to_regmap(np);
>> + if (IS_ERR(priv->regmap)) {
>> + dev_err(dev, "Couldn't get regmap\n");
>> + return -ENODEV;
>> + }
>> +
>> + clk = devm_clk_get(dev, NULL);
>> + if (IS_ERR(clk))
>> + return -ENODEV;
>> + priv->clk_freq = clk_get_rate(clk);
> If you intend to use this clock, you have to enable it.
Got it.
>> + priv->reset = reset_control_get_shared(&pdev->dev, NULL);
>> + if (IS_ERR(priv->reset)) {
>> + dev_err(&pdev->dev, "can't get aspeed_pwm_tacho reset\n");
>> + return PTR_ERR(priv->reset);
>> + }
>> + reset_control_deassert(priv->reset);
> missing error checking
Got it.
>> + priv->chip.dev = dev;
>> + priv->chip.ops = &aspeed_pwm_ops;
>> + priv->chip.base = -1;
> This isn't necessary since f9a8ee8c8bcd118e800d88772c6457381db45224,
> please drop the assignment to base.
Got it.
>> + priv->chip.npwm = ASPEED_NR_PWMS;
>> + priv->chip.of_xlate = of_pwm_xlate_with_flags;
>> + priv->chip.of_pwm_n_cells = 3;
>> +
>> + ret = pwmchip_add(&priv->chip);
>> + if (ret < 0) {
>> + dev_err(&pdev->dev, "failed to add PWM chip: %d\n", ret);
> Please use %pe to make the error messages better readable.
Got it.
>> + return ret;
>> + }
>> + dev_set_drvdata(dev, priv);
>> + return ret;
>> +}
>> +
>> +static const struct of_device_id of_pwm_match_table[] = {
>> + {
>> + .compatible = "aspeed,ast2600-pwm",
>> + },
>> + {},
>> +};
>> +MODULE_DEVICE_TABLE(of, of_pwm_match_table);
>> +
>> +static struct platform_driver aspeed_pwm_driver = {
>> + .probe = aspeed_pwm_probe,
> Please implement a .remove callback.
Got it.
>> + .driver = {
>> + .name = "aspeed_pwm",
>> + .of_match_table = of_pwm_match_table,
>> + },
>> +};
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2021-04-13 9:27 UTC | newest]
Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-12 9:54 [PATCH 0/4] Support pwm driver for aspeed ast26xx Billy Tsai
2021-04-12 9:54 ` Billy Tsai
2021-04-12 9:54 ` [PATCH 1/4] dt-bindings: Add bindings for aspeed pwm-tach Billy Tsai
2021-04-12 9:54 ` Billy Tsai
2021-04-12 12:55 ` Uwe Kleine-König
2021-04-12 12:55 ` Uwe Kleine-König
2021-04-13 6:27 ` Billy Tsai
2021-04-13 6:27 ` Billy Tsai
2021-04-12 9:54 ` [PATCH 2/4] dt-bindings: Add bindings for aspeed pwm Billy Tsai
2021-04-12 9:54 ` Billy Tsai
2021-04-12 13:20 ` Rob Herring
2021-04-12 13:20 ` Rob Herring
2021-04-13 7:27 ` Billy Tsai
2021-04-13 7:27 ` Billy Tsai
2021-04-12 9:54 ` [PATCH 3/4] pwm: Add Aspeed ast2600 PWM support Billy Tsai
2021-04-12 9:54 ` Billy Tsai
2021-04-12 12:35 ` Uwe Kleine-König
2021-04-12 12:35 ` Uwe Kleine-König
2021-04-13 9:24 ` Billy Tsai
2021-04-13 9:24 ` Billy Tsai
2021-04-12 9:54 ` [PATCH 4/4] pwm: Add support for aspeed pwm controller Billy Tsai
2021-04-12 9:54 ` Billy Tsai
2021-04-12 11:14 ` Uwe Kleine-König
2021-04-12 11:14 ` Uwe Kleine-König
2021-04-13 2:11 ` Billy Tsai
2021-04-13 2:11 ` Billy Tsai
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.