[v6 0/4] Support pwm/tach driver for aspeed ast26xx

[v6 0/4] Support pwm/tach driver for aspeed ast26xx

[Billy Tsai]

Unlike the old design that the register setting of the TACH should based
on the configure of the PWM. In ast26xx, the dependency between pwm and
tach controller is eliminated and becomes a separate hardware block. One
is used to provide pwm output and another is used to monitor the frequency
of the input. Therefore, this patch serials implements them by writing the
two driver "pwm-aspeed-ast2600.c" and "tach-aspeed-ast2600.c". The former
is following the pwm subsystem which can apply the existed driver to
controller the fan(pwm-fan.c), beeper(pwm-beeper.c) and so on. The latter
is following the sysfs interface of hwmon to creat the node for fan
monitor.

Changes since v5:
- pwm/tach:
  - Remove the utilization of common resources from the parent node.
  - Change the concept to 16 PWM/TACH controllers, each with one channel,
  instead of 1 PWM/TACH controller with 16 channels.
- dt-binding:
  - Eliminate the usage of simple-mfd.

Changes since v4:
- pwm:
  - Fix the return type of get_status function.
- tach:
  - read clk source once and re-use it
  - Remove the constants variables
  - Allocate tach_channel as array
  - Use dev->parent
- dt-binding:
  - Fix the order of the patches
  - Add example and description for tach child node
  - Remove pwm extension property

Changes since v3:
- pwm:
  - Remove unnecessary include header
  - Fix warning Prefer "GPL" over "GPL v2"
- tach:
  - Remove the paremeter min_rpm and max_rpm and return the tach value 
  directly without any polling or delay.
  - Fix warning Prefer "GPL" over "GPL v2"
- dt-binding:
  - Replace underscore in node names with dashes
  - Split per subsystem

Changes since v2:
- pwm:
  - Use devm_* api to simplify the error cleanup
  - Fix the multi-line alignment problem
- tach:
  - Add tach-aspeed-ast2600 to index.rst
  - Fix the multi-line alignment problem
  - Remove the tach enable/disable when read the rpm
  - Fix some coding format issue

Changes since v1:
- tach:
  - Add the document tach-aspeed-ast2600.rst
  - Use devm_* api to simplify the error cleanup.
  - Change hwmon register api to devm_hwmon_device_register_with_info

Billy Tsai (4):
  dt-bindings: pwm: Add ASPEED PWM Control documentation
  dt-bindings: hwmon: Add ASPEED TACH Control documentation
  pwm: Add Aspeed ast2600 PWM support
  hwmon: Add Aspeed ast2600 TACH support

 .../bindings/hwmon/aspeed,ast2600-tach.yaml   |  32 ++
 .../bindings/pwm/aspeed,ast2600-pwm.yaml      |  38 +++
 Documentation/hwmon/index.rst                 |   1 +
 Documentation/hwmon/tach-aspeed-ast2600.rst   |  25 ++
 drivers/hwmon/Kconfig                         |  10 +
 drivers/hwmon/Makefile                        |   1 +
 drivers/hwmon/tach-aspeed-ast2600.c           | 305 +++++++++++++++++
 drivers/pwm/Kconfig                           |  10 +
 drivers/pwm/Makefile                          |   1 +
 drivers/pwm/pwm-aspeed-ast2600.c              | 309 ++++++++++++++++++
 10 files changed, 732 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/aspeed,ast2600-tach.yaml
 create mode 100644 Documentation/devicetree/bindings/pwm/aspeed,ast2600-pwm.yaml
 create mode 100644 Documentation/hwmon/tach-aspeed-ast2600.rst
 create mode 100644 drivers/hwmon/tach-aspeed-ast2600.c
 create mode 100644 drivers/pwm/pwm-aspeed-ast2600.c

-- 
2.25.1

[v6 1/4] dt-bindings: pwm: Add ASPEED PWM Control documentation

[Billy Tsai]

Document the compatible for aspeed,ast2600-pwm device.

Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
---
 .../bindings/pwm/aspeed,ast2600-pwm.yaml      | 38 +++++++++++++++++++
 1 file changed, 38 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..a9e040263578
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/aspeed,ast2600-pwm.yaml
@@ -0,0 +1,38 @@
+# 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 supports up to 1 PWM outputs.
+
+allOf:
+  - $ref: pwm.yaml#
+
+properties:
+  compatible:
+    enum:
+      - aspeed,ast2600-pwm
+
+  "#pwm-cells":
+    const: 3
+
+  clocks:
+    maxItems: 1
+
+  resets:
+    maxItems: 1
+
+required:
+  - compatible
+  - clocks
+  - resets
+
+additionalProperties: false
-- 
2.25.1

[v6 2/4] dt-bindings: hwmon: Add ASPEED TACH Control documentation

[Billy Tsai]

Document the compatible for aspeed,ast2600-tach device.

Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
---
 .../bindings/hwmon/aspeed,ast2600-tach.yaml   | 32 +++++++++++++++++++
 1 file changed, 32 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/aspeed,ast2600-tach.yaml

diff --git a/Documentation/devicetree/bindings/hwmon/aspeed,ast2600-tach.yaml b/Documentation/devicetree/bindings/hwmon/aspeed,ast2600-tach.yaml
new file mode 100644
index 000000000000..627aa00f2e92
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/aspeed,ast2600-tach.yaml
@@ -0,0 +1,32 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright (C) 2021 Aspeed, Inc.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/hwmon/aspeed,ast2600-tach.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Aspeed Ast2600 Tach controller
+
+maintainers:
+  - Billy Tsai <billy_tsai@aspeedtech.com>
+
+description: |
+  The Aspeed Tach controller can support upto 1 fan input.
+
+properties:
+  compatible:
+    enum:
+      - aspeed,ast2600-tach
+
+  clocks:
+    maxItems: 1
+
+  resets:
+    maxItems: 1
+
+required:
+  - compatible
+  - clocks
+  - resets
+
+additionalProperties: false
-- 
2.25.1

[v6 3/4] pwm: Add Aspeed ast2600 PWM support

[Billy Tsai]

Add the support of PWM controller which can be found in the Aspeed AST2600
SoC. This particular SoC features 16 PWM controllers, with each controller
capable of supporting up to 1 PWM output.

Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/Kconfig              |  10 +
 drivers/pwm/Makefile             |   1 +
 drivers/pwm/pwm-aspeed-ast2600.c | 309 +++++++++++++++++++++++++++++++
 3 files changed, 320 insertions(+)
 create mode 100644 drivers/pwm/pwm-aspeed-ast2600.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 60d13a949bc5..54915185d918 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -51,6 +51,16 @@ config PWM_AB8500
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-ab8500.
 
+config PWM_ASPEED_AST2600
+	tristate "Aspeed ast2600 PWM support"
+	depends on ARCH_ASPEED || COMPILE_TEST
+	depends on HAVE_CLK && HAS_IOMEM
+	help
+	  This driver provides support for Aspeed ast2600 PWM controllers.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-aspeed-ast2600.
+
 config PWM_ATMEL
 	tristate "Atmel PWM support"
 	depends on ARCH_AT91 || COMPILE_TEST
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 7bf1a29f02b8..5169c34056e6 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -2,6 +2,7 @@
 obj-$(CONFIG_PWM)		+= core.o
 obj-$(CONFIG_PWM_SYSFS)		+= sysfs.o
 obj-$(CONFIG_PWM_AB8500)	+= pwm-ab8500.o
+obj-$(CONFIG_PWM_ASPEED_AST2600)	+= pwm-aspeed-ast2600.o
 obj-$(CONFIG_PWM_ATMEL)		+= pwm-atmel.o
 obj-$(CONFIG_PWM_ATMEL_HLCDC_PWM)	+= pwm-atmel-hlcdc.o
 obj-$(CONFIG_PWM_ATMEL_TCB)	+= pwm-atmel-tcb.o
diff --git a/drivers/pwm/pwm-aspeed-ast2600.c b/drivers/pwm/pwm-aspeed-ast2600.c
new file mode 100644
index 000000000000..ae6eb197e473
--- /dev/null
+++ b/drivers/pwm/pwm-aspeed-ast2600.c
@@ -0,0 +1,309 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2021 Aspeed Technology Inc.
+ *
+ * PWM controller driver for Aspeed ast2600 SoCs.
+ * This drivers doesn't support earlier version of the IP.
+ *
+ * The hardware operates in time quantities of length
+ * Q := (DIV_L + 1) << DIV_H / input-clk
+ * The length of a PWM period is (DUTY_CYCLE_PERIOD + 1) * Q.
+ * The maximal value for DUTY_CYCLE_PERIOD is used here to provide
+ * a fine grained selection for the duty cycle.
+ *
+ * This driver uses DUTY_CYCLE_RISING_POINT = 0, so from the start of a
+ * period the output is active until DUTY_CYCLE_FALLING_POINT * Q. Note
+ * that if DUTY_CYCLE_RISING_POINT = DUTY_CYCLE_FALLING_POINT the output is
+ * always active.
+ *
+ * Register usage:
+ * PIN_ENABLE: When it is unset the pwm controller will emit inactive level to the external.
+ * Use to determine whether the PWM channel is enabled or disabled
+ * CLK_ENABLE: When it is unset the pwm controller will assert the duty counter reset and
+ * emit inactive level to the PIN_ENABLE mux after that the driver can still change the pwm period
+ * and duty and the value will apply when CLK_ENABLE be set again.
+ * Use to determine whether duty_cycle bigger than 0.
+ * PWM_ASPEED_CTRL_INVERSE: When it is toggled the output value will inverse immediately.
+ * PWM_ASPEED_DUTY_CYCLE_FALLING_POINT/PWM_ASPEED_DUTY_CYCLE_RISING_POINT: When these two
+ * values are equal it means the duty cycle = 100%.
+ *
+ * The glitch may generate at:
+ * - Enabled changing when the duty_cycle bigger than 0% and less than 100%.
+ * - Polarity changing when the duty_cycle bigger than 0% and less than 100%.
+ *
+ * Limitations:
+ * - When changing both duty cycle and period, we cannot prevent in
+ *   software that the output might produce a period with mixed
+ *   settings.
+ * - Disabling the PWM doesn't complete the current period.
+ *
+ * Improvements:
+ * - When only changing one of duty cycle or period, our pwm controller will not
+ *   generate the glitch, the configure will change at next cycle of pwm.
+ *   This improvement can disable/enable through PWM_ASPEED_CTRL_DUTY_SYNC_DISABLE.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/errno.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/math64.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/reset.h>
+#include <linux/sysfs.h>
+
+/* PWM Control Register */
+#define PWM_ASPEED_CTRL				(0x00)
+#define PWM_ASPEED_CTRL_LOAD_SEL_RISING_AS_WDT	BIT(19)
+#define PWM_ASPEED_CTRL_DUTY_LOAD_AS_WDT_ENABLE	BIT(18)
+#define PWM_ASPEED_CTRL_DUTY_SYNC_DISABLE	BIT(17)
+#define PWM_ASPEED_CTRL_CLK_ENABLE		BIT(16)
+#define PWM_ASPEED_CTRL_LEVEL_OUTPUT		BIT(15)
+#define PWM_ASPEED_CTRL_INVERSE			BIT(14)
+#define PWM_ASPEED_CTRL_OPEN_DRAIN_ENABLE	BIT(13)
+#define PWM_ASPEED_CTRL_PIN_ENABLE		BIT(12)
+#define PWM_ASPEED_CTRL_CLK_DIV_H		GENMASK(11, 8)
+#define PWM_ASPEED_CTRL_CLK_DIV_L		GENMASK(7, 0)
+
+/* PWM Duty Cycle Register */
+#define PWM_ASPEED_DUTY_CYCLE			(0x04)
+#define PWM_ASPEED_DUTY_CYCLE_PERIOD		GENMASK(31, 24)
+#define PWM_ASPEED_DUTY_CYCLE_POINT_AS_WDT	GENMASK(23, 16)
+#define PWM_ASPEED_DUTY_CYCLE_FALLING_POINT	GENMASK(15, 8)
+#define PWM_ASPEED_DUTY_CYCLE_RISING_POINT	GENMASK(7, 0)
+
+/* PWM fixed value */
+#define PWM_ASPEED_FIXED_PERIOD			FIELD_MAX(PWM_ASPEED_DUTY_CYCLE_PERIOD)
+
+struct aspeed_pwm_data {
+	struct pwm_chip chip;
+	struct clk *clk;
+	void __iomem *base;
+	struct reset_control *reset;
+	unsigned long clk_source;
+};
+
+static inline struct aspeed_pwm_data *
+aspeed_pwm_chip_to_data(struct pwm_chip *chip)
+{
+	return container_of(chip, struct aspeed_pwm_data, chip);
+}
+
+static int aspeed_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
+				struct pwm_state *state)
+{
+	struct device *dev = chip->dev;
+	struct aspeed_pwm_data *priv = aspeed_pwm_chip_to_data(chip);
+	bool polarity,	pin_en, clk_en;
+	u32 duty_pt, val;
+	u64 div_h, div_l, duty_cycle_period, dividend;
+
+	val = readl(priv->base + PWM_ASPEED_CTRL);
+	polarity = FIELD_GET(PWM_ASPEED_CTRL_INVERSE, val);
+	pin_en = FIELD_GET(PWM_ASPEED_CTRL_PIN_ENABLE, val);
+	clk_en = FIELD_GET(PWM_ASPEED_CTRL_CLK_ENABLE, val);
+	div_h = FIELD_GET(PWM_ASPEED_CTRL_CLK_DIV_H, val);
+	div_l = FIELD_GET(PWM_ASPEED_CTRL_CLK_DIV_L, val);
+	val = readl(priv->base + PWM_ASPEED_DUTY_CYCLE);
+	duty_pt = FIELD_GET(PWM_ASPEED_DUTY_CYCLE_FALLING_POINT, val);
+	duty_cycle_period = FIELD_GET(PWM_ASPEED_DUTY_CYCLE_PERIOD, val);
+
+	/*
+	 * This multiplication doesn't overflow, the upper bound is
+	 * 1000000000 * 256 * 256 << 15 = 0x1dcd650000000000
+	 */
+	dividend = (u64)NSEC_PER_SEC * (div_l + 1) * (duty_cycle_period + 1)
+		       << div_h;
+	state->period = DIV_ROUND_UP_ULL(dividend, priv->clk_source);
+
+	if (clk_en && duty_pt) {
+		dividend = (u64)NSEC_PER_SEC * (div_l + 1) * duty_pt
+				 << div_h;
+		state->duty_cycle =
+			DIV_ROUND_UP_ULL(dividend, priv->clk_source);
+	} else {
+		state->duty_cycle = clk_en ? state->period : 0;
+	}
+	state->polarity = polarity ? PWM_POLARITY_INVERSED : PWM_POLARITY_NORMAL;
+	state->enabled = pin_en;
+	dev_dbg(dev, "get period: %lldns, duty_cycle: %lldns", state->period,
+		state->duty_cycle);
+	return 0;
+}
+
+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 = aspeed_pwm_chip_to_data(chip);
+	u32 duty_pt;
+	u64 div_h, div_l, divisor, expect_period;
+	bool clk_en;
+
+	expect_period = min(div64_u64(ULLONG_MAX, (u64)priv->clk_source),
+			    state->period);
+	dev_dbg(dev, "expect period: %lldns, duty_cycle: %lldns", expect_period,
+		state->duty_cycle);
+	/*
+	 * Pick the smallest value for div_h so that div_l can be the biggest
+	 * which results in a finer resolution near the target period value.
+	 */
+	divisor = (u64)NSEC_PER_SEC * (PWM_ASPEED_FIXED_PERIOD + 1) *
+		  (FIELD_MAX(PWM_ASPEED_CTRL_CLK_DIV_L) + 1);
+	div_h = order_base_2(DIV64_U64_ROUND_UP(priv->clk_source * expect_period, divisor));
+	if (div_h > 0xf)
+		div_h = 0xf;
+
+	divisor = ((u64)NSEC_PER_SEC * (PWM_ASPEED_FIXED_PERIOD + 1)) << div_h;
+	div_l = div64_u64(priv->clk_source * expect_period, divisor);
+
+	if (div_l == 0)
+		return -ERANGE;
+
+	div_l -= 1;
+
+	if (div_l > 255)
+		div_l = 255;
+
+	dev_dbg(dev, "clk source: %ld div_h %lld, div_l : %lld\n",
+		priv->clk_source, div_h, div_l);
+	/* duty_pt = duty_cycle * (PERIOD + 1) / period */
+	duty_pt = div64_u64(state->duty_cycle * priv->clk_source,
+			    (u64)NSEC_PER_SEC * (div_l + 1) << div_h);
+	dev_dbg(dev, "duty_cycle = %lld, duty_pt = %d\n", state->duty_cycle,
+		duty_pt);
+
+	/*
+	 * Fixed DUTY_CYCLE_PERIOD to its max value to get a
+	 * fine-grained resolution for duty_cycle at the expense of a
+	 * coarser period resolution.
+	 */
+	writel((readl(priv->base + PWM_ASPEED_DUTY_CYCLE) &
+		~(PWM_ASPEED_DUTY_CYCLE_PERIOD)) |
+		       FIELD_PREP(PWM_ASPEED_DUTY_CYCLE_PERIOD,
+				  PWM_ASPEED_FIXED_PERIOD),
+	       priv->base + PWM_ASPEED_DUTY_CYCLE);
+
+	if (duty_pt == 0) {
+		/* emit inactive level and assert the duty counter reset */
+		clk_en = 0;
+	} else {
+		clk_en = 1;
+		if (duty_pt >= (PWM_ASPEED_FIXED_PERIOD + 1))
+			duty_pt = 0;
+		writel((readl(priv->base + PWM_ASPEED_DUTY_CYCLE) &
+			~(PWM_ASPEED_DUTY_CYCLE_RISING_POINT |
+			  PWM_ASPEED_DUTY_CYCLE_FALLING_POINT)) |
+			       FIELD_PREP(PWM_ASPEED_DUTY_CYCLE_FALLING_POINT,
+					  duty_pt),
+		       priv->base + PWM_ASPEED_DUTY_CYCLE);
+	}
+
+	writel((readl(priv->base + PWM_ASPEED_CTRL) &
+		~(PWM_ASPEED_CTRL_CLK_DIV_H | PWM_ASPEED_CTRL_CLK_DIV_L |
+		  PWM_ASPEED_CTRL_PIN_ENABLE | PWM_ASPEED_CTRL_CLK_ENABLE |
+		  PWM_ASPEED_CTRL_INVERSE)) |
+		       FIELD_PREP(PWM_ASPEED_CTRL_CLK_DIV_H, div_h) |
+		       FIELD_PREP(PWM_ASPEED_CTRL_CLK_DIV_L, div_l) |
+		       FIELD_PREP(PWM_ASPEED_CTRL_PIN_ENABLE, state->enabled) |
+		       FIELD_PREP(PWM_ASPEED_CTRL_CLK_ENABLE, clk_en) |
+		       FIELD_PREP(PWM_ASPEED_CTRL_INVERSE, state->polarity),
+	       priv->base + PWM_ASPEED_CTRL);
+
+	return 0;
+}
+
+static const struct pwm_ops aspeed_pwm_ops = {
+	.apply = aspeed_pwm_apply,
+	.get_state = aspeed_pwm_get_state,
+	.owner = THIS_MODULE,
+};
+
+static void aspeed_pwm_reset_assert(void *data)
+{
+	struct reset_control *rst = data;
+
+	reset_control_assert(rst);
+}
+
+static void aspeed_pwm_chip_remove(void *data)
+{
+	struct pwm_chip *chip = data;
+
+	pwmchip_remove(chip);
+}
+
+static int aspeed_pwm_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	int ret;
+	struct aspeed_pwm_data *priv;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(priv->base))
+		return PTR_ERR(priv->base);
+
+	priv->clk = devm_clk_get_enabled(dev, NULL);
+	if (IS_ERR(priv->clk))
+		return dev_err_probe(dev, PTR_ERR(priv->clk),
+				     "Couldn't get clock\n");
+	priv->clk_source = clk_get_rate(priv->clk);
+	priv->reset = devm_reset_control_get_shared(dev, NULL);
+	if (IS_ERR(priv->reset))
+		return dev_err_probe(dev, PTR_ERR(priv->reset),
+				     "Couldn't get reset control\n");
+
+	ret = reset_control_deassert(priv->reset);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "Couldn't deassert reset control\n");
+
+	ret = devm_add_action_or_reset(dev, aspeed_pwm_reset_assert,
+				       priv->reset);
+	if (ret)
+		return ret;
+
+	priv->chip.dev = dev;
+	priv->chip.ops = &aspeed_pwm_ops;
+	priv->chip.npwm = 1;
+
+	ret = pwmchip_add(&priv->chip);
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "Failed to add PWM chip\n");
+	ret = devm_add_action_or_reset(dev, aspeed_pwm_chip_remove,
+				       &priv->chip);
+	if (ret)
+		return ret;
+	return 0;
+}
+
+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 ast2600 PWM device driver");
+MODULE_LICENSE("GPL");
-- 
2.25.1

[v6 4/4] hwmon: Add Aspeed ast2600 TACH support

[Billy Tsai]

Add the support of Tachometer which can use to monitor the frequency of
the input. In Aspeed AST2600 SoC features 16 TACH controllers, with each
controller capable of supporting up to 1 input.

Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
---
 Documentation/hwmon/index.rst               |   1 +
 Documentation/hwmon/tach-aspeed-ast2600.rst |  25 ++
 drivers/hwmon/Kconfig                       |  10 +
 drivers/hwmon/Makefile                      |   1 +
 drivers/hwmon/tach-aspeed-ast2600.c         | 305 ++++++++++++++++++++
 5 files changed, 342 insertions(+)
 create mode 100644 Documentation/hwmon/tach-aspeed-ast2600.rst
 create mode 100644 drivers/hwmon/tach-aspeed-ast2600.c

diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
index ddff3c5713d7..4c3dd74675ef 100644
--- a/Documentation/hwmon/index.rst
+++ b/Documentation/hwmon/index.rst
@@ -194,6 +194,7 @@ Hardware Monitoring Kernel Drivers
    sparx5-temp
    stpddc60
    sy7636a-hwmon
+   tach-aspeed-ast2600
    tc654
    tc74
    thmc50
diff --git a/Documentation/hwmon/tach-aspeed-ast2600.rst b/Documentation/hwmon/tach-aspeed-ast2600.rst
new file mode 100644
index 000000000000..b08c73a4237f
--- /dev/null
+++ b/Documentation/hwmon/tach-aspeed-ast2600.rst
@@ -0,0 +1,25 @@
+Kernel driver tach-aspeed-ast2600
+=================================
+
+Supported chips:
+	ASPEED AST2600
+
+Authors:
+	<billy_tsai@aspeedtech.com>
+
+Description:
+------------
+This driver implements support for ASPEED AST2600 Fan Tacho controller.
+The controller supports up to 1 tachometer inputs.
+
+The driver provides the following sensor accesses in sysfs:
+
+=============== ======= ======================================================
+fanX_input	ro	provide current fan rotation value in RPM as reported
+			by the fan to the device.
+fanX_div	rw	Fan divisor: Supported value are power of 4 (1, 4, 16
+                        64, ... 4194304)
+                        The larger divisor, the less rpm accuracy and the less
+                        affected by fan signal glitch.
+fanX_pulses	rw      Fan pulses per resolution.
+=============== ======= ======================================================
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index a5253abb7ea7..5948a63e44e7 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -411,6 +411,16 @@ config SENSORS_ASPEED
 	  This driver can also be built as a module. If so, the module
 	  will be called aspeed_pwm_tacho.
 
+config SENSORS_TACH_ASPEED_AST2600
+	tristate "ASPEED ast2600 Tachometer support"
+	depends on ARCH_ASPEED || COMPILE_TEST
+	depends on HAVE_CLK && HAS_IOMEM
+	help
+	  This driver provides support for Aspeed ast2600 Tachometer.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called tach-aspeed-ast2600.
+
 config SENSORS_ATXP1
 	tristate "Attansic ATXP1 VID controller"
 	depends on I2C
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index c5cd7e3a67ff..a3bf5b438e0f 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -53,6 +53,7 @@ obj-$(CONFIG_SENSORS_ARM_SCMI)	+= scmi-hwmon.o
 obj-$(CONFIG_SENSORS_ARM_SCPI)	+= scpi-hwmon.o
 obj-$(CONFIG_SENSORS_AS370)	+= as370-hwmon.o
 obj-$(CONFIG_SENSORS_ASC7621)	+= asc7621.o
+obj-$(CONFIG_SENSORS_TACH_ASPEED_AST2600) += tach-aspeed-ast2600.o
 obj-$(CONFIG_SENSORS_ASPEED)	+= aspeed-pwm-tacho.o
 obj-$(CONFIG_SENSORS_ATXP1)	+= atxp1.o
 obj-$(CONFIG_SENSORS_AXI_FAN_CONTROL) += axi-fan-control.o
diff --git a/drivers/hwmon/tach-aspeed-ast2600.c b/drivers/hwmon/tach-aspeed-ast2600.c
new file mode 100644
index 000000000000..8be66ee25a31
--- /dev/null
+++ b/drivers/hwmon/tach-aspeed-ast2600.c
@@ -0,0 +1,305 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) ASPEED Technology Inc.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/errno.h>
+#include <linux/hwmon.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/reset.h>
+#include <linux/sysfs.h>
+
+/* TACH Control Register */
+#define TACH_ASPEED_CTRL		(0x00)
+#define TACH_ASPEED_IER			BIT(31)
+#define TACH_ASPEED_INVERS_LIMIT	BIT(30)
+#define TACH_ASPEED_LOOPBACK		BIT(29)
+#define TACH_ASPEED_ENABLE		BIT(28)
+#define TACH_ASPEED_DEBOUNCE_MASK	GENMASK(27, 26)
+#define TACH_ASPEED_DEBOUNCE_BIT	26
+#define TACH_ASPEED_IO_EDGE_MASK	GENMASK(25, 24)
+#define TACH_ASPEED_IO_EDGE_BIT		24
+#define TACH_ASPEED_CLK_DIV_T_MASK	GENMASK(23, 20)
+#define TACH_ASPEED_CLK_DIV_BIT		20
+#define TACH_ASPEED_THRESHOLD_MASK	GENMASK(19, 0)
+/* [27:26] */
+#define DEBOUNCE_3_CLK			0x00
+#define DEBOUNCE_2_CLK			0x01
+#define DEBOUNCE_1_CLK			0x02
+#define DEBOUNCE_0_CLK			0x03
+/* [25:24] */
+#define F2F_EDGES			0x00
+#define R2R_EDGES			0x01
+#define BOTH_EDGES			0x02
+/* [23:20] */
+/* divisor = 4 to the nth power, n = register value */
+#define DEFAULT_TACH_DIV		1024
+#define DIV_TO_REG(divisor)		(ilog2(divisor) >> 1)
+
+/* TACH Status Register */
+#define TACH_ASPEED_STS			(0x04)
+
+/*PWM_TACH_STS */
+#define TACH_ASPEED_ISR			BIT(31)
+#define TACH_ASPEED_PWM_OUT		BIT(25)
+#define TACH_ASPEED_PWM_OEN		BIT(24)
+#define TACH_ASPEED_DEB_INPUT		BIT(23)
+#define TACH_ASPEED_RAW_INPUT		BIT(22)
+#define TACH_ASPEED_VALUE_UPDATE	BIT(21)
+#define TACH_ASPEED_FULL_MEASUREMENT	BIT(20)
+#define TACH_ASPEED_VALUE_MASK		GENMASK(19, 0)
+/**********************************************************
+ * Software setting
+ *********************************************************/
+#define DEFAULT_FAN_PULSE_PR		2
+
+struct aspeed_tach_channel_params {
+	u8 pulse_pr;
+	u32 divisor;
+};
+
+struct aspeed_tach_data {
+	struct device *dev;
+	void __iomem *base;
+	struct clk *clk;
+	struct reset_control *reset;
+	bool tach_present;
+	struct aspeed_tach_channel_params tach_channel;
+	unsigned long clk_source;
+};
+
+static void aspeed_tach_ch_enable(struct aspeed_tach_data *priv, bool enable)
+{
+	if (enable)
+		writel(readl(priv->base + TACH_ASPEED_CTRL) |
+			       (TACH_ASPEED_ENABLE),
+		       priv->base + TACH_ASPEED_CTRL);
+	else
+		writel(readl(priv->base + TACH_ASPEED_CTRL) &
+			       ~(TACH_ASPEED_ENABLE),
+		       priv->base + TACH_ASPEED_CTRL);
+}
+
+static u64 aspeed_tach_val_to_rpm(struct aspeed_tach_data *priv, u32 tach_val)
+{
+	u64 rpm;
+	u32 tach_div;
+
+	tach_div = tach_val * (priv->tach_channel.divisor) *
+		   (priv->tach_channel.pulse_pr);
+
+	dev_dbg(priv->dev, "clk %ld, tach_val %d , tach_div %d\n",
+		priv->clk_source, tach_val, tach_div);
+
+	rpm = (u64)priv->clk_source * 60;
+	do_div(rpm, tach_div);
+
+	return rpm;
+}
+
+static int aspeed_get_fan_tach_ch_rpm(struct aspeed_tach_data *priv)
+{
+	u32 val;
+	u64 rpm;
+
+	val = readl(priv->base + TACH_ASPEED_STS);
+
+	if (!(val & TACH_ASPEED_FULL_MEASUREMENT))
+		return 0;
+	rpm = aspeed_tach_val_to_rpm(priv, val & TACH_ASPEED_VALUE_MASK);
+
+	return rpm;
+}
+
+static int aspeed_tach_hwmon_read(struct device *dev,
+				  enum hwmon_sensor_types type, u32 attr,
+				  int channel, long *val)
+{
+	struct aspeed_tach_data *priv = dev_get_drvdata(dev);
+	u32 reg_val;
+	int ret;
+
+	switch (attr) {
+	case hwmon_fan_input:
+		ret = aspeed_get_fan_tach_ch_rpm(priv);
+		if (ret < 0)
+			return ret;
+		*val = ret;
+		break;
+	case hwmon_fan_div:
+		reg_val = readl(priv->base + TACH_ASPEED_CTRL);
+		reg_val = FIELD_GET(TACH_ASPEED_CLK_DIV_T_MASK, reg_val);
+		*val = BIT(reg_val << 1);
+		break;
+	case hwmon_fan_pulses:
+		*val = priv->tach_channel.pulse_pr;
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+	return 0;
+}
+
+static int aspeed_tach_hwmon_write(struct device *dev,
+				   enum hwmon_sensor_types type, u32 attr,
+				   int channel, long val)
+{
+	struct aspeed_tach_data *priv = dev_get_drvdata(dev);
+
+	switch (attr) {
+	case hwmon_fan_div:
+		if (!is_power_of_2(val) || (ilog2(val) % 2))
+			return -EINVAL;
+		else if (DIV_TO_REG(val) > 0xb)
+			return -ERANGE;
+		priv->tach_channel.divisor = val;
+		writel((readl(priv->base + TACH_ASPEED_CTRL) &
+			~(TACH_ASPEED_CLK_DIV_T_MASK)) |
+			       (DIV_TO_REG(priv->tach_channel.divisor)
+				<< TACH_ASPEED_CLK_DIV_BIT),
+		       priv->base + TACH_ASPEED_CTRL);
+		break;
+	case hwmon_fan_pulses:
+		priv->tach_channel.pulse_pr = val;
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	return 0;
+}
+
+static umode_t aspeed_tach_dev_is_visible(const void *drvdata,
+					  enum hwmon_sensor_types type,
+					  u32 attr, int channel)
+{
+	const struct aspeed_tach_data *priv = drvdata;
+
+	if (!priv->tach_present)
+		return 0;
+	switch (attr) {
+	case hwmon_fan_input:
+		return 0444;
+	case hwmon_fan_div:
+	case hwmon_fan_pulses:
+		return 0644;
+	}
+	return 0;
+}
+
+static const struct hwmon_ops aspeed_tach_ops = {
+	.is_visible = aspeed_tach_dev_is_visible,
+	.read = aspeed_tach_hwmon_read,
+	.write = aspeed_tach_hwmon_write,
+};
+
+static const struct hwmon_channel_info *aspeed_tach_info[] = {
+	HWMON_CHANNEL_INFO(fan, HWMON_F_INPUT | HWMON_F_DIV | HWMON_F_PULSES),
+	NULL
+};
+
+static const struct hwmon_chip_info aspeed_tach_chip_info = {
+	.ops = &aspeed_tach_ops,
+	.info = aspeed_tach_info,
+};
+
+static void aspeed_present_fan_tach(struct aspeed_tach_data *priv)
+{
+	priv->tach_present = true;
+	priv->tach_channel.divisor = DEFAULT_TACH_DIV;
+	priv->tach_channel.pulse_pr = DEFAULT_FAN_PULSE_PR;
+
+	writel((readl(priv->base + TACH_ASPEED_CTRL) &
+		~(TACH_ASPEED_INVERS_LIMIT | TACH_ASPEED_DEBOUNCE_MASK |
+		  TACH_ASPEED_IO_EDGE_MASK | TACH_ASPEED_CLK_DIV_T_MASK |
+		  TACH_ASPEED_THRESHOLD_MASK)) |
+		       ((DEBOUNCE_3_CLK << TACH_ASPEED_DEBOUNCE_BIT) |
+			F2F_EDGES |
+			(DIV_TO_REG(priv->tach_channel.divisor)
+			 << TACH_ASPEED_CLK_DIV_BIT)),
+	       priv->base + TACH_ASPEED_CTRL);
+
+	aspeed_tach_ch_enable(priv, true);
+}
+
+static void aspeed_tach_reset_assert(void *data)
+{
+	struct reset_control *rst = data;
+
+	reset_control_assert(rst);
+}
+
+static int aspeed_tach_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct aspeed_tach_data *priv;
+	struct device *hwmon;
+	int ret;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+	priv->dev = &pdev->dev;
+	priv->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(priv->base))
+		return PTR_ERR(priv->base);
+	priv->clk = devm_clk_get_enabled(dev, NULL);
+	if (IS_ERR(priv->clk))
+		return dev_err_probe(dev, PTR_ERR(priv->clk),
+				     "Couldn't get clock\n");
+
+	priv->clk_source = clk_get_rate(priv->clk);
+	priv->reset = devm_reset_control_get_shared(dev, NULL);
+	if (IS_ERR(priv->reset))
+		return dev_err_probe(dev, PTR_ERR(priv->reset),
+				     "Couldn't get reset control\n");
+
+	ret = reset_control_deassert(priv->reset);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "Couldn't deassert reset control\n");
+
+	ret = devm_add_action_or_reset(dev, aspeed_tach_reset_assert,
+				       priv->reset);
+	if (ret)
+		return ret;
+
+	aspeed_present_fan_tach(priv);
+
+	hwmon = devm_hwmon_device_register_with_info(dev, "aspeed_tach", priv,
+						     &aspeed_tach_chip_info, NULL);
+	ret = PTR_ERR_OR_ZERO(hwmon);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "Failed to register hwmon device\n");
+	return 0;
+}
+
+static const struct of_device_id of_stach_match_table[] = {
+	{
+		.compatible = "aspeed,ast2600-tach",
+	},
+	{},
+};
+MODULE_DEVICE_TABLE(of, of_stach_match_table);
+
+static struct platform_driver aspeed_tach_driver = {
+	.probe		= aspeed_tach_probe,
+	.driver		= {
+		.name	= "aspeed_tach",
+		.of_match_table = of_stach_match_table,
+	},
+};
+
+module_platform_driver(aspeed_tach_driver);
+
+MODULE_AUTHOR("Billy Tsai <billy_tsai@aspeedtech.com>");
+MODULE_DESCRIPTION("Aspeed ast2600 TACH device driver");
+MODULE_LICENSE("GPL");
-- 
2.25.1

Re: [v6 2/4] dt-bindings: hwmon: Add ASPEED TACH Control documentation

[Guenter Roeck]

On 6/7/23 19:18, Billy Tsai wrote:
> Document the compatible for aspeed,ast2600-tach device.
> 
> Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
> ---
>   .../bindings/hwmon/aspeed,ast2600-tach.yaml   | 32 +++++++++++++++++++
>   1 file changed, 32 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/hwmon/aspeed,ast2600-tach.yaml
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/aspeed,ast2600-tach.yaml b/Documentation/devicetree/bindings/hwmon/aspeed,ast2600-tach.yaml
> new file mode 100644
> index 000000000000..627aa00f2e92
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/aspeed,ast2600-tach.yaml
> @@ -0,0 +1,32 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright (C) 2021 Aspeed, Inc.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/hwmon/aspeed,ast2600-tach.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Aspeed Ast2600 Tach controller
> +
> +maintainers:
> +  - Billy Tsai <billy_tsai@aspeedtech.com>
> +
> +description: |
> +  The Aspeed Tach controller can support upto 1 fan input.
> +

The code says:

In Aspeed AST2600 SoC features 16 TACH controllers, with each
controller capable of supporting up to 1 input.

which is a bit different. I guess there are no examples anymore,
but I'd really like to see how this looks like in the devicetree file,
and how the driver is supposed to distinguish/select the 16 inputs.

> +properties:
> +  compatible:
> +    enum:
> +      - aspeed,ast2600-tach
> +
> +  clocks:
> +    maxItems: 1
> +
> +  resets:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - clocks
> +  - resets
> +
> +additionalProperties: false

Re: [v6 4/4] hwmon: Add Aspeed ast2600 TACH support

[Guenter Roeck]

On 6/7/23 19:18, Billy Tsai wrote:
> Add the support of Tachometer which can use to monitor the frequency of
> the input. In Aspeed AST2600 SoC features 16 TACH controllers, with each
> controller capable of supporting up to 1 input.
> 

You mean there will be 16 instances of this driver ? That is weird,
confusing, and warrants a detailed explanation.

> Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
> ---
>   Documentation/hwmon/index.rst               |   1 +
>   Documentation/hwmon/tach-aspeed-ast2600.rst |  25 ++
>   drivers/hwmon/Kconfig                       |  10 +
>   drivers/hwmon/Makefile                      |   1 +
>   drivers/hwmon/tach-aspeed-ast2600.c         | 305 ++++++++++++++++++++
>   5 files changed, 342 insertions(+)
>   create mode 100644 Documentation/hwmon/tach-aspeed-ast2600.rst
>   create mode 100644 drivers/hwmon/tach-aspeed-ast2600.c
> 
> diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
> index ddff3c5713d7..4c3dd74675ef 100644
> --- a/Documentation/hwmon/index.rst
> +++ b/Documentation/hwmon/index.rst
> @@ -194,6 +194,7 @@ Hardware Monitoring Kernel Drivers
>      sparx5-temp
>      stpddc60
>      sy7636a-hwmon
> +   tach-aspeed-ast2600
>      tc654
>      tc74
>      thmc50
> diff --git a/Documentation/hwmon/tach-aspeed-ast2600.rst b/Documentation/hwmon/tach-aspeed-ast2600.rst
> new file mode 100644
> index 000000000000..b08c73a4237f
> --- /dev/null
> +++ b/Documentation/hwmon/tach-aspeed-ast2600.rst
> @@ -0,0 +1,25 @@
> +Kernel driver tach-aspeed-ast2600
> +=================================
> +
> +Supported chips:
> +	ASPEED AST2600
> +
> +Authors:
> +	<billy_tsai@aspeedtech.com>
> +
> +Description:
> +------------
> +This driver implements support for ASPEED AST2600 Fan Tacho controller.
> +The controller supports up to 1 tachometer inputs.
> +
> +The driver provides the following sensor accesses in sysfs:
> +
> +=============== ======= ======================================================
> +fanX_input	ro	provide current fan rotation value in RPM as reported
> +			by the fan to the device.
> +fanX_div	rw	Fan divisor: Supported value are power of 4 (1, 4, 16
> +                        64, ... 4194304)
> +                        The larger divisor, the less rpm accuracy and the less
> +                        affected by fan signal glitch.
> +fanX_pulses	rw      Fan pulses per resolution.

Presumably, as implemented, that would be 16 instances, each with fan1_{input,div}.

> +=============== ======= ======================================================
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index a5253abb7ea7..5948a63e44e7 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -411,6 +411,16 @@ config SENSORS_ASPEED
>   	  This driver can also be built as a module. If so, the module
>   	  will be called aspeed_pwm_tacho.
>   
> +config SENSORS_TACH_ASPEED_AST2600
> +	tristate "ASPEED ast2600 Tachometer support"
> +	depends on ARCH_ASPEED || COMPILE_TEST
> +	depends on HAVE_CLK && HAS_IOMEM
> +	help
> +	  This driver provides support for Aspeed ast2600 Tachometer.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called tach-aspeed-ast2600.
> +
>   config SENSORS_ATXP1
>   	tristate "Attansic ATXP1 VID controller"
>   	depends on I2C
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index c5cd7e3a67ff..a3bf5b438e0f 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -53,6 +53,7 @@ obj-$(CONFIG_SENSORS_ARM_SCMI)	+= scmi-hwmon.o
>   obj-$(CONFIG_SENSORS_ARM_SCPI)	+= scpi-hwmon.o
>   obj-$(CONFIG_SENSORS_AS370)	+= as370-hwmon.o
>   obj-$(CONFIG_SENSORS_ASC7621)	+= asc7621.o
> +obj-$(CONFIG_SENSORS_TACH_ASPEED_AST2600) += tach-aspeed-ast2600.o
>   obj-$(CONFIG_SENSORS_ASPEED)	+= aspeed-pwm-tacho.o
>   obj-$(CONFIG_SENSORS_ATXP1)	+= atxp1.o
>   obj-$(CONFIG_SENSORS_AXI_FAN_CONTROL) += axi-fan-control.o
> diff --git a/drivers/hwmon/tach-aspeed-ast2600.c b/drivers/hwmon/tach-aspeed-ast2600.c
> new file mode 100644
> index 000000000000..8be66ee25a31
> --- /dev/null
> +++ b/drivers/hwmon/tach-aspeed-ast2600.c
> @@ -0,0 +1,305 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) ASPEED Technology Inc.
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/errno.h>
> +#include <linux/hwmon.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/reset.h>
> +#include <linux/sysfs.h>
> +
> +/* TACH Control Register */
> +#define TACH_ASPEED_CTRL		(0x00)
> +#define TACH_ASPEED_IER			BIT(31)
> +#define TACH_ASPEED_INVERS_LIMIT	BIT(30)
> +#define TACH_ASPEED_LOOPBACK		BIT(29)
> +#define TACH_ASPEED_ENABLE		BIT(28)
> +#define TACH_ASPEED_DEBOUNCE_MASK	GENMASK(27, 26)
> +#define TACH_ASPEED_DEBOUNCE_BIT	26
> +#define TACH_ASPEED_IO_EDGE_MASK	GENMASK(25, 24)
> +#define TACH_ASPEED_IO_EDGE_BIT		24
> +#define TACH_ASPEED_CLK_DIV_T_MASK	GENMASK(23, 20)
> +#define TACH_ASPEED_CLK_DIV_BIT		20
> +#define TACH_ASPEED_THRESHOLD_MASK	GENMASK(19, 0)
> +/* [27:26] */
> +#define DEBOUNCE_3_CLK			0x00
> +#define DEBOUNCE_2_CLK			0x01
> +#define DEBOUNCE_1_CLK			0x02
> +#define DEBOUNCE_0_CLK			0x03
> +/* [25:24] */
> +#define F2F_EDGES			0x00
> +#define R2R_EDGES			0x01
> +#define BOTH_EDGES			0x02
> +/* [23:20] */
> +/* divisor = 4 to the nth power, n = register value */
> +#define DEFAULT_TACH_DIV		1024
> +#define DIV_TO_REG(divisor)		(ilog2(divisor) >> 1)
> +
> +/* TACH Status Register */
> +#define TACH_ASPEED_STS			(0x04)

Unnecesarry ()

> +
> +/*PWM_TACH_STS */
> +#define TACH_ASPEED_ISR			BIT(31)
> +#define TACH_ASPEED_PWM_OUT		BIT(25)
> +#define TACH_ASPEED_PWM_OEN		BIT(24)
> +#define TACH_ASPEED_DEB_INPUT		BIT(23)
> +#define TACH_ASPEED_RAW_INPUT		BIT(22)
> +#define TACH_ASPEED_VALUE_UPDATE	BIT(21)
> +#define TACH_ASPEED_FULL_MEASUREMENT	BIT(20)
> +#define TACH_ASPEED_VALUE_MASK		GENMASK(19, 0)
> +/**********************************************************
> + * Software setting
> + *********************************************************/
> +#define DEFAULT_FAN_PULSE_PR		2
> +
> +struct aspeed_tach_channel_params {
> +	u8 pulse_pr;
> +	u32 divisor;
> +};
> +
> +struct aspeed_tach_data {
> +	struct device *dev;
> +	void __iomem *base;
> +	struct clk *clk;
> +	struct reset_control *reset;
> +	bool tach_present;
> +	struct aspeed_tach_channel_params tach_channel;

Assuming that all channels have the same divisor, this structure
is really unnecessary (especially since pulse_pr should be dropped -
see below).

> +	unsigned long clk_source;
> +};
> +
> +static void aspeed_tach_ch_enable(struct aspeed_tach_data *priv, bool enable)
> +{
> +	if (enable)
> +		writel(readl(priv->base + TACH_ASPEED_CTRL) |
> +			       (TACH_ASPEED_ENABLE),
> +		       priv->base + TACH_ASPEED_CTRL);
> +	else
> +		writel(readl(priv->base + TACH_ASPEED_CTRL) &
> +			       ~(TACH_ASPEED_ENABLE),

Unnecessary () around TACH_ASPEED_ENABLE.

> +		       priv->base + TACH_ASPEED_CTRL);
> +}
> +
> +static u64 aspeed_tach_val_to_rpm(struct aspeed_tach_data *priv, u32 tach_val)
> +{
> +	u64 rpm;
> +	u32 tach_div;
> +
> +	tach_div = tach_val * (priv->tach_channel.divisor) *
> +		   (priv->tach.channel.pulse_pr);
> +

Unnecessary () around priv->tach_channel.pulse_pr and priv->tach_channel.divisor.
I am not going to look further for those. Please make sure you drop them all.

> +	dev_dbg(priv->dev, "clk %ld, tach_val %d , tach_div %d\n",
> +		priv->clk_source, tach_val, tach_div);
> +
> +	rpm = (u64)priv->clk_source * 60;
> +	do_div(rpm, tach_div);
> +
> +	return rpm;

This function should really return an integer.

> +}
> +
> +static int aspeed_get_fan_tach_ch_rpm(struct aspeed_tach_data *priv)
> +{
> +	u32 val;
> +	u64 rpm;
> +
> +	val = readl(priv->base + TACH_ASPEED_STS);
> +
> +	if (!(val & TACH_ASPEED_FULL_MEASUREMENT))
> +		return 0;
> +	rpm = aspeed_tach_val_to_rpm(priv, val & TACH_ASPEED_VALUE_MASK);
> +
> +	return rpm;

The 'rpm' variable is unnecessary.

> +}
> +
> +static int aspeed_tach_hwmon_read(struct device *dev,
> +				  enum hwmon_sensor_types type, u32 attr,
> +				  int channel, long *val)
> +{
> +	struct aspeed_tach_data *priv = dev_get_drvdata(dev);
> +	u32 reg_val;
> +	int ret;
> +
> +	switch (attr) {
> +	case hwmon_fan_input:
> +		ret = aspeed_get_fan_tach_ch_rpm(priv);
> +		if (ret < 0)
> +			return ret;

Why ? The called code never returns a negative value.

> +		*val = ret;
> +		break;
> +	case hwmon_fan_div:
> +		reg_val = readl(priv->base + TACH_ASPEED_CTRL);
> +		reg_val = FIELD_GET(TACH_ASPEED_CLK_DIV_T_MASK, reg_val);
> +		*val = BIT(reg_val << 1);
> +		break;
> +	case hwmon_fan_pulses:
> +		*val = priv->tach_channel.pulse_pr;
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +	return 0;
> +}
> +
> +static int aspeed_tach_hwmon_write(struct device *dev,
> +				   enum hwmon_sensor_types type, u32 attr,
> +				   int channel, long val)
> +{
> +	struct aspeed_tach_data *priv = dev_get_drvdata(dev);
> +
> +	switch (attr) {
> +	case hwmon_fan_div:
> +		if (!is_power_of_2(val) || (ilog2(val) % 2))
> +			return -EINVAL;
> +		else if (DIV_TO_REG(val) > 0xb)

Else after return is unnecessary

> +			return -ERANGE;

Math result not representable ? Not really.

> +		priv->tach_channel.divisor = val;
> +		writel((readl(priv->base + TACH_ASPEED_CTRL) &
> +			~(TACH_ASPEED_CLK_DIV_T_MASK)) |

Unnecessary () around TACH_ASPEED_CLK_DIV_T_MASK.

> +			       (DIV_TO_REG(priv->tach_channel.divisor)
> +				<< TACH_ASPEED_CLK_DIV_BIT),
> +		       priv->base + TACH_ASPEED_CTRL);
> +		break;
> +	case hwmon_fan_pulses:
> +		priv->tach_channel.pulse_pr = val;


The ABI says for the pulses atribute:

                 Should only be created if the chip has a register to configure
                 the number of pulses. In the absence of such a register (and
                 thus attribute) the value assumed by all devices is 2 pulses
                 per fan revolution.

I see no evidence that there is a chip register for the number of pulses.
Please drop this attribute.

> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	return 0;
> +}
> +
> +static umode_t aspeed_tach_dev_is_visible(const void *drvdata,
> +					  enum hwmon_sensor_types type,
> +					  u32 attr, int channel)
> +{
> +	const struct aspeed_tach_data *priv = drvdata;
> +
> +	if (!priv->tach_present)
> +		return 0;

What is the point of instantiating the driver in this case ?

> +	switch (attr) {
> +	case hwmon_fan_input:
> +		return 0444;
> +	case hwmon_fan_div:
> +	case hwmon_fan_pulses:
> +		return 0644;
> +	}
> +	return 0;
> +}
> +
> +static const struct hwmon_ops aspeed_tach_ops = {
> +	.is_visible = aspeed_tach_dev_is_visible,
> +	.read = aspeed_tach_hwmon_read,
> +	.write = aspeed_tach_hwmon_write,
> +};
> +
> +static const struct hwmon_channel_info *aspeed_tach_info[] = {
> +	HWMON_CHANNEL_INFO(fan, HWMON_F_INPUT | HWMON_F_DIV | HWMON_F_PULSES),
> +	NULL
> +};
> +
> +static const struct hwmon_chip_info aspeed_tach_chip_info = {
> +	.ops = &aspeed_tach_ops,
> +	.info = aspeed_tach_info,
> +};
> +
> +static void aspeed_present_fan_tach(struct aspeed_tach_data *priv)
> +{
> +	priv->tach_present = true;
> +	priv->tach_channel.divisor = DEFAULT_TACH_DIV;
> +	priv->tach_channel.pulse_pr = DEFAULT_FAN_PULSE_PR;
> +
> +	writel((readl(priv->base + TACH_ASPEED_CTRL) &
> +		~(TACH_ASPEED_INVERS_LIMIT | TACH_ASPEED_DEBOUNCE_MASK |
> +		  TACH_ASPEED_IO_EDGE_MASK | TACH_ASPEED_CLK_DIV_T_MASK |
> +		  TACH_ASPEED_THRESHOLD_MASK)) |
> +		       ((DEBOUNCE_3_CLK << TACH_ASPEED_DEBOUNCE_BIT) |
> +			F2F_EDGES |
> +			(DIV_TO_REG(priv->tach_channel.divisor)
> +			 << TACH_ASPEED_CLK_DIV_BIT)),
> +	       priv->base + TACH_ASPEED_CTRL);
> +
> +	aspeed_tach_ch_enable(priv, true);
> +}
> +
> +static void aspeed_tach_reset_assert(void *data)
> +{
> +	struct reset_control *rst = data;
> +
> +	reset_control_assert(rst);
> +}
> +
> +static int aspeed_tach_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct aspeed_tach_data *priv;
> +	struct device *hwmon;
> +	int ret;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +	priv->dev = &pdev->dev;
> +	priv->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(priv->base))
> +		return PTR_ERR(priv->base);
> +	priv->clk = devm_clk_get_enabled(dev, NULL);
> +	if (IS_ERR(priv->clk))
> +		return dev_err_probe(dev, PTR_ERR(priv->clk),
> +				     "Couldn't get clock\n");
> +
> +	priv->clk_source = clk_get_rate(priv->clk);
> +	priv->reset = devm_reset_control_get_shared(dev, NULL);
> +	if (IS_ERR(priv->reset))
> +		return dev_err_probe(dev, PTR_ERR(priv->reset),
> +				     "Couldn't get reset control\n");
> +
> +	ret = reset_control_deassert(priv->reset);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Couldn't deassert reset control\n");
> +
> +	ret = devm_add_action_or_reset(dev, aspeed_tach_reset_assert,
> +				       priv->reset);
> +	if (ret)
> +		return ret;
> +
> +	aspeed_present_fan_tach(priv);
> +
> +	hwmon = devm_hwmon_device_register_with_info(dev, "aspeed_tach", priv,
> +						     &aspeed_tach_chip_info, NULL);
> +	ret = PTR_ERR_OR_ZERO(hwmon);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Failed to register hwmon device\n");
> +	return 0;
> +}
> +
> +static const struct of_device_id of_stach_match_table[] = {
> +	{
> +		.compatible = "aspeed,ast2600-tach",
> +	},
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, of_stach_match_table);
> +
> +static struct platform_driver aspeed_tach_driver = {
> +	.probe		= aspeed_tach_probe,
> +	.driver		= {
> +		.name	= "aspeed_tach",
> +		.of_match_table = of_stach_match_table,
> +	},
> +};
> +
> +module_platform_driver(aspeed_tach_driver);
> +
> +MODULE_AUTHOR("Billy Tsai <billy_tsai@aspeedtech.com>");
> +MODULE_DESCRIPTION("Aspeed ast2600 TACH device driver");
> +MODULE_LICENSE("GPL");

Re: [v6 0/4] Support pwm/tach driver for aspeed ast26xx

[Krzysztof Kozlowski]

On 08/06/2023 04:18, Billy Tsai wrote:
> Unlike the old design that the register setting of the TACH should based
> on the configure of the PWM. In ast26xx, the dependency between pwm and
> tach controller is eliminated and becomes a separate hardware block. One
> is used to provide pwm output and another is used to monitor the frequency
> of the input. Therefore, this patch serials implements them by writing the
> two driver "pwm-aspeed-ast2600.c" and "tach-aspeed-ast2600.c". The former
> is following the pwm subsystem which can apply the existed driver to
> controller the fan(pwm-fan.c), beeper(pwm-beeper.c) and so on. The latter
> is following the sysfs interface of hwmon to creat the node for fan
> monitor.

You like to ignore my comments... How did you implement them? Go one by
one - answer to v4 emails.

Best regards,
Krzysztof

Re: [v6 1/4] dt-bindings: pwm: Add ASPEED PWM Control documentation

[Krzysztof Kozlowski]

On 08/06/2023 04:18, Billy Tsai wrote:
> Document the compatible for aspeed,ast2600-pwm device.

This is a friendly reminder during the review process.

It seems my previous comments were not fully addressed. Maybe my
feedback got lost between the quotes, maybe you just forgot to apply it.
Please go back to the previous discussion and either implement all
requested changes or keep discussing them.

Thank you.

I don't understand why you make the same mistakes, even though I pointed
them out two times already.

I am not going to point third time. Sorry, it's a waste of my time.

NAK.


Best regards,
Krzysztof

Re: [v6 2/4] dt-bindings: hwmon: Add ASPEED TACH Control documentation

[Krzysztof Kozlowski]

On 08/06/2023 04:18, Billy Tsai wrote:
> Document the compatible for aspeed,ast2600-tach device.

This is a friendly reminder during the review process.

It seems my previous comments were not fully addressed. Maybe my
feedback got lost between the quotes, maybe you just forgot to apply it.
Please go back to the previous discussion and either implement all
requested changes or keep discussing them.

Thank you.

> 
> Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
> ---
>  .../bindings/hwmon/aspeed,ast2600-tach.yaml   | 32 +++++++++++++++++++
>  1 file changed, 32 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/aspeed,ast2600-tach.yaml
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/aspeed,ast2600-tach.yaml b/Documentation/devicetree/bindings/hwmon/aspeed,ast2600-tach.yaml
> new file mode 100644
> index 000000000000..627aa00f2e92
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/aspeed,ast2600-tach.yaml
> @@ -0,0 +1,32 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright (C) 2021 Aspeed, Inc.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/hwmon/aspeed,ast2600-tach.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Aspeed Ast2600 Tach controller
> +
> +maintainers:
> +  - Billy Tsai <billy_tsai@aspeedtech.com>
> +
> +description: |
> +  The Aspeed Tach controller can support upto 1 fan input.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - aspeed,ast2600-tach
> +
> +  clocks:
> +    maxItems: 1
> +
> +  resets:
> +    maxItems: 1

NAK, not true based on previous discussions. Device does not come with
resets and clocks.


Best regards,
Krzysztof

Re: [v6 1/4] dt-bindings: pwm: Add ASPEED PWM Control documentation

[Krzysztof Kozlowski]

On 08/06/2023 04:18, Billy Tsai wrote:
> Document the compatible for aspeed,ast2600-pwm device.
> 
> Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
> ---
>  .../bindings/pwm/aspeed,ast2600-pwm.yaml      | 38 +++++++++++++++++++
>  1 file changed, 38 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..a9e040263578
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/aspeed,ast2600-pwm.yaml
> @@ -0,0 +1,38 @@
> +# 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 supports up to 1 PWM outputs.

This does not look right.

> +
> +allOf:
> +  - $ref: pwm.yaml#
> +
> +properties:
> +  compatible:
> +    enum:
> +      - aspeed,ast2600-pwm
> +
> +  "#pwm-cells":
> +    const: 3

3 cells? For one PWM? What are they?

> +
> +  clocks:
> +    maxItems: 1
> +
> +  resets:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - clocks
> +  - resets
> +
> +additionalProperties: false

Missing examples. All bindings need examples.

Best regards,
Krzysztof

Re: [v6 1/4] dt-bindings: pwm: Add ASPEED PWM Control documentation

[Krzysztof Kozlowski]

On 08/06/2023 09:47, Billy Tsai wrote:
> 
>   >> +
>   >> +allOf:
>   >> +  - $ref: pwm.yaml#
>   >> +
>   >> +properties:
>   >> +  compatible:
>   >> +    enum:
>   >> +      - aspeed,ast2600-pwm
>   >> +
>   >> +  "#pwm-cells":
>   >> +    const: 3
> 
>   > 3 cells? For one PWM? What are they?
> 
> channel, period and polarity.

Don't cut my responses. You wrote you have one PWM output, so only one
channel. What do you put then in the channel?

I will start NAKing such patches without DTS user. It's like reviewing
fake code for some unknown solution and trying to get from you piece of
answers one by one, because you do not want to share entire part.

Best regards,
Krzysztof

Re: [v6 1/4] dt-bindings: pwm: Add ASPEED PWM Control documentation

[Krzysztof Kozlowski]

On 08/06/2023 10:21, Billy Tsai wrote:
>         On 08/06/2023 09:47, Billy Tsai wrote:
>         >>
>         >>   >> +
>         >>   >> +allOf:
>         >>   >> +  - $ref: pwm.yaml#
>         >>   >> +
>         >>   >> +properties:
>         >>   >> +  compatible:
>         >>   >> +    enum:
>         >>   >> +      - aspeed,ast2600-pwm
>         >>   >> +
>         >>   >> +  "#pwm-cells":
>         >>   >> +    const: 3
>         >>
>         >>   > 3 cells? For one PWM? What are they?
>         >>
>         >> channel, period and polarity.
> 
>         > Don't cut my responses. You wrote you have one PWM output, so only one
>         > channel. What do you put then in the channel?
> 
> You need to put 0 in the cell of the channel, the example of the dts usage will like following:

If you always put 0 isn't this a proof that it's wrong?



Best regards,
Krzysztof

Re: [v6 1/4] dt-bindings: pwm: Add ASPEED PWM Control documentation

[Krzysztof Kozlowski]

On 08/06/2023 10:21, Billy Tsai wrote:
>         On 08/06/2023 09:47, Billy Tsai wrote:
>         >>
>         >>   >> +
>         >>   >> +allOf:
>         >>   >> +  - $ref: pwm.yaml#
>         >>   >> +
>         >>   >> +properties:
>         >>   >> +  compatible:
>         >>   >> +    enum:
>         >>   >> +      - aspeed,ast2600-pwm
>         >>   >> +
>         >>   >> +  "#pwm-cells":
>         >>   >> +    const: 3
>         >>
>         >>   > 3 cells? For one PWM? What are they?
>         >>
>         >> channel, period and polarity.
> 
>         > Don't cut my responses. You wrote you have one PWM output, so only one
>         > channel. What do you put then in the channel?
> 
> You need to put 0 in the cell of the channel, the example of the dts usage will like following:
> 
> pwm0: pwm0@1e610000 {
>         compatible = "aspeed,ast2600-pwm";
>         reg = <0x1e610000 0x8>;
>         #pwm-cells = <3>;
>         #address-cells = <1>;
>         #size-cells = <0>;
>         pinctrl-names = "default";
>         pinctrl-0 = <&pinctrl_pwm0_default>;
>         clocks = <&syscon ASPEED_CLK_AHB>;
>         resets = <&syscon ASPEED_RESET_PWM>;
>         status = "okay";
> };
> 
> pwm1: pwm1@1e610010 {
>         compatible = "aspeed,ast2600-pwm";
>         reg = <0x1e610010 0x8>;
>         #pwm-cells = <3>;
>         #address-cells = <1>;
>         #size-cells = <0>;
>         pinctrl-names = "default";
>         pinctrl-0 = <&pinctrl_pwm1_default>;
>         clocks = <&syscon ASPEED_CLK_AHB>;
>         resets = <&syscon ASPEED_RESET_PWM>;
>         status = "okay";

BTW, these are not two PWM devices but one. I don't understand why you
changed previous design into something like this, but this is not
representing your hardware.

Best regards,
Krzysztof

Re: [v6 1/4] dt-bindings: pwm: Add ASPEED PWM Control documentation

[Krzysztof Kozlowski]

On 08/06/2023 10:57, Billy Tsai wrote:
> On 08/06/2023 10:21, Billy Tsai wrote:
>         >>         On 08/06/2023 09:47, Billy Tsai wrote:
>         >>         >>
>         >>         >>   >> +
>         >>         >>   >> +allOf:
>         >>         >>   >> +  - $ref: pwm.yaml#
>         >>         >>   >> +
>         >>         >>   >> +properties:
>         >>         >>   >> +  compatible:
>         >>         >>   >> +    enum:
>         >>         >>   >> +      - aspeed,ast2600-pwm
>         >>         >>   >> +
>         >>         >>   >> +  "#pwm-cells":
>         >>         >>   >> +    const: 3
>         >>         >>
>         >>         >>   > 3 cells? For one PWM? What are they?
>         >>         >>
>         >>         >> channel, period and polarity.
>         >>
>         >>         > Don't cut my responses. You wrote you have one PWM output, so only one
>         >>         > channel. What do you put then in the channel?
>         >>
>         >> You need to put 0 in the cell of the channel, the example of the dts usage will like following:
> 
>         > If you always put 0 isn't this a proof that it's wrong?
> 
> No, if your PWM controller only has one pwm output, then it should only be configured as 0.
> This is the usage of the pwm-cells property.
> https://github.com/torvalds/linux/blob/master/drivers/pwm/core.c#L129-L158

This is only when you use generic of_xlate. You do not have to use
generic of_xlate if it does not suite you. Again you speak about the
drivers, but we talk about bindings:

https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/pwm/pwm.txt#L13

"controller specific"

> https://github.com/torvalds/linux/blob/master/include/linux/pwm.h#LL299C20-L299C20
> All of the pwm driver with npwm = 1 will has the same usage.

So it seems many simplified their drivers...

Best regards,
Krzysztof

Re: [v6 1/4] dt-bindings: pwm: Add ASPEED PWM Control documentation

[Krzysztof Kozlowski]

On 08/06/2023 11:15, Billy Tsai wrote:
> On 08/06/2023 10:21, Billy Tsai wrote:
>         >>         On 08/06/2023 09:47, Billy Tsai wrote:
>         >>         >>
>         >>         >>   >> +
>         >>         >>   >> +allOf:
>         >>         >>   >> +  - $ref: pwm.yaml#
>         >>         >>   >> +
>         >>         >>   >> +properties:
>         >>         >>   >> +  compatible:
>         >>         >>   >> +    enum:
>         >>         >>   >> +      - aspeed,ast2600-pwm
>         >>         >>   >> +
>         >>         >>   >> +  "#pwm-cells":
>         >>         >>   >> +    const: 3
>         >>         >>
>         >>         >>   > 3 cells? For one PWM? What are they?
>         >>         >>
>         >>         >> channel, period and polarity.
>         >>
>         >>         > Don't cut my responses. You wrote you have one PWM output, so only one
>         >>         > channel. What do you put then in the channel?
>         >>
>         >> You need to put 0 in the cell of the channel, the example of the dts usage will like following:
>         >>
>         >> pwm0: pwm0@1e610000 {
>         >>         compatible = "aspeed,ast2600-pwm";
>         >>         reg = <0x1e610000 0x8>;
>         >>         #pwm-cells = <3>;
>         >>         #address-cells = <1>;
>         >>         #size-cells = <0>;
>         >>         pinctrl-names = "default";
>         >>         pinctrl-0 = <&pinctrl_pwm0_default>;
>         >>         clocks = <&syscon ASPEED_CLK_AHB>;
>         >>         resets = <&syscon ASPEED_RESET_PWM>;
>         >>         status = "okay";
>         >> };
>         >>
>         >> pwm1: pwm1@1e610010 {
>         >>         compatible = "aspeed,ast2600-pwm";
>         >>         reg = <0x1e610010 0x8>;
>         >>         #pwm-cells = <3>;
>         >>         #address-cells = <1>;
>         >>         #size-cells = <0>;
>         >>         pinctrl-names = "default";
>         >>         pinctrl-0 = <&pinctrl_pwm1_default>;
>         >>         clocks = <&syscon ASPEED_CLK_AHB>;
>         >>         resets = <&syscon ASPEED_RESET_PWM>;
>         >>         status = "okay";
> 
>         > BTW, these are not two PWM devices but one. I don't understand why you
>         > changed previous design into something like this, but this is not
>         > representing your hardware.
> 
> The previous design of my patch treated our PWM controller as having 16 PWM channels.
> However, from a hardware perspective, it consists of 16 individual PWM chips, each
> with its own set of two 4-byte control registers. These chips operate independently
> and are not affected by each other.

They are affected by each other - you use the same clock and reset line.
I really doubt you have 16 PWM controllers. Anyway, I cannot judge.
Either your previous submissions were totally bogus or this one is.

Best regards,
Krzysztof

Re: [v6 2/4] dt-bindings: hwmon: Add ASPEED TACH Control documentation

[Guenter Roeck]

On 6/7/23 23:21, Billy Tsai wrote:
>          > The code says:
> 
>          > In Aspeed AST2600 SoC features 16 TACH controllers, with each
> 
>          > controller capable of supporting up to 1 input.
> 
>          > which is a bit different. I guess there are no examples anymore,
> 
>          > but I'd really like to see how this looks like in the devicetree file,
> 
>          > and how the driver is supposed to distinguish/select the 16 inputs.
> 
> Hi Roeck,
> 
> The node in the devicetree file will looks like following:
> 
> tach0: tach0@1e610008 {
> 
>          compatible = "aspeed,ast2600-tach";
> 
>          reg = <0x1e610008 0x8>;
> 
>          #address-cells = <1>;
> 
>          #size-cells = <0>;
> 
>          pinctrl-names = "default";
> 
>          pinctrl-0 = <&pinctrl_tach0_default>;
> 
>          clocks = <&syscon ASPEED_CLK_AHB>;
> 
>          resets = <&syscon ASPEED_RESET_PWM>;
> 
>          status = "disabled";
> 
> };
> 

Neither reg nor pinctrl is mentioned in the bindings. Maybe that is not needed nowadays,
but I find it confusing.

Either case, it is highly unusual that there would be 16 instances of this device
instead of one. Why is this done ? It doesn't really make sense to me.

Guenter

Re: [v6 3/4] pwm: Add Aspeed ast2600 PWM support

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 8717 bytes --]

On Thu, Jun 08, 2023 at 10:18:38AM +0800, Billy Tsai wrote:
[...]
> diff --git a/drivers/pwm/pwm-aspeed-ast2600.c b/drivers/pwm/pwm-aspeed-ast2600.c
[...]
> +/* PWM Control Register */
> +#define PWM_ASPEED_CTRL				(0x00)
[...]
> +#define PWM_ASPEED_DUTY_CYCLE			(0x04)

Guenther already mentioned this, but these parentheses are unnecessary.

> +struct aspeed_pwm_data {
> +	struct pwm_chip chip;
> +	struct clk *clk;
> +	void __iomem *base;
> +	struct reset_control *reset;
> +	unsigned long clk_source;

The name is a bit unfortunate. Looking at the code this represent the
rate of the parent clock, so something like clk_rate would be more
appropriate.

> +};
> +
> +static inline struct aspeed_pwm_data *
> +aspeed_pwm_chip_to_data(struct pwm_chip *chip)
> +{
> +	return container_of(chip, struct aspeed_pwm_data, chip);
> +}
> +
> +static int aspeed_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> +				struct pwm_state *state)
> +{
> +	struct device *dev = chip->dev;

You seem to use this exactly once, in a debug message, so having the
extra local variable seems a bit overkill. No strong objection, though.

> +	struct aspeed_pwm_data *priv = aspeed_pwm_chip_to_data(chip);
> +	bool polarity,	pin_en, clk_en;

A tab seems to have snuck in here.

> +	u32 duty_pt, val;
> +	u64 div_h, div_l, duty_cycle_period, dividend;
> +
> +	val = readl(priv->base + PWM_ASPEED_CTRL);
> +	polarity = FIELD_GET(PWM_ASPEED_CTRL_INVERSE, val);
> +	pin_en = FIELD_GET(PWM_ASPEED_CTRL_PIN_ENABLE, val);
> +	clk_en = FIELD_GET(PWM_ASPEED_CTRL_CLK_ENABLE, val);
> +	div_h = FIELD_GET(PWM_ASPEED_CTRL_CLK_DIV_H, val);
> +	div_l = FIELD_GET(PWM_ASPEED_CTRL_CLK_DIV_L, val);
> +	val = readl(priv->base + PWM_ASPEED_DUTY_CYCLE);
> +	duty_pt = FIELD_GET(PWM_ASPEED_DUTY_CYCLE_FALLING_POINT, val);
> +	duty_cycle_period = FIELD_GET(PWM_ASPEED_DUTY_CYCLE_PERIOD, val);
> +
> +	/*
> +	 * This multiplication doesn't overflow, the upper bound is
> +	 * 1000000000 * 256 * 256 << 15 = 0x1dcd650000000000
> +	 */
> +	dividend = (u64)NSEC_PER_SEC * (div_l + 1) * (duty_cycle_period + 1)
> +		       << div_h;
> +	state->period = DIV_ROUND_UP_ULL(dividend, priv->clk_source);
> +
> +	if (clk_en && duty_pt) {
> +		dividend = (u64)NSEC_PER_SEC * (div_l + 1) * duty_pt
> +				 << div_h;
> +		state->duty_cycle =
> +			DIV_ROUND_UP_ULL(dividend, priv->clk_source);
> +	} else {
> +		state->duty_cycle = clk_en ? state->period : 0;
> +	}
> +	state->polarity = polarity ? PWM_POLARITY_INVERSED : PWM_POLARITY_NORMAL;
> +	state->enabled = pin_en;
> +	dev_dbg(dev, "get period: %lldns, duty_cycle: %lldns", state->period,
> +		state->duty_cycle);

How likely are you to ever use this again? And how useful will that be?
We've got debugfs support that will show this information and more.

> +	return 0;
> +}
> +
> +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 = aspeed_pwm_chip_to_data(chip);
> +	u32 duty_pt;
> +	u64 div_h, div_l, divisor, expect_period;
> +	bool clk_en;
> +
> +	expect_period = min(div64_u64(ULLONG_MAX, (u64)priv->clk_source),
> +			    state->period);
> +	dev_dbg(dev, "expect period: %lldns, duty_cycle: %lldns", expect_period,
> +		state->duty_cycle);
> +	/*
> +	 * Pick the smallest value for div_h so that div_l can be the biggest
> +	 * which results in a finer resolution near the target period value.
> +	 */
> +	divisor = (u64)NSEC_PER_SEC * (PWM_ASPEED_FIXED_PERIOD + 1) *
> +		  (FIELD_MAX(PWM_ASPEED_CTRL_CLK_DIV_L) + 1);
> +	div_h = order_base_2(DIV64_U64_ROUND_UP(priv->clk_source * expect_period, divisor));
> +	if (div_h > 0xf)
> +		div_h = 0xf;
> +
> +	divisor = ((u64)NSEC_PER_SEC * (PWM_ASPEED_FIXED_PERIOD + 1)) << div_h;
> +	div_l = div64_u64(priv->clk_source * expect_period, divisor);
> +
> +	if (div_l == 0)
> +		return -ERANGE;
> +
> +	div_l -= 1;
> +
> +	if (div_l > 255)
> +		div_l = 255;
> +
> +	dev_dbg(dev, "clk source: %ld div_h %lld, div_l : %lld\n",
> +		priv->clk_source, div_h, div_l);
> +	/* duty_pt = duty_cycle * (PERIOD + 1) / period */
> +	duty_pt = div64_u64(state->duty_cycle * priv->clk_source,
> +			    (u64)NSEC_PER_SEC * (div_l + 1) << div_h);
> +	dev_dbg(dev, "duty_cycle = %lld, duty_pt = %d\n", state->duty_cycle,
> +		duty_pt);
> +
> +	/*
> +	 * Fixed DUTY_CYCLE_PERIOD to its max value to get a
> +	 * fine-grained resolution for duty_cycle at the expense of a
> +	 * coarser period resolution.
> +	 */
> +	writel((readl(priv->base + PWM_ASPEED_DUTY_CYCLE) &
> +		~(PWM_ASPEED_DUTY_CYCLE_PERIOD)) |
> +		       FIELD_PREP(PWM_ASPEED_DUTY_CYCLE_PERIOD,
> +				  PWM_ASPEED_FIXED_PERIOD),
> +	       priv->base + PWM_ASPEED_DUTY_CYCLE);

This is completely unreadable. Use a temporary variable to split this
up.

> +
> +	if (duty_pt == 0) {
> +		/* emit inactive level and assert the duty counter reset */
> +		clk_en = 0;
> +	} else {
> +		clk_en = 1;
> +		if (duty_pt >= (PWM_ASPEED_FIXED_PERIOD + 1))
> +			duty_pt = 0;
> +		writel((readl(priv->base + PWM_ASPEED_DUTY_CYCLE) &
> +			~(PWM_ASPEED_DUTY_CYCLE_RISING_POINT |
> +			  PWM_ASPEED_DUTY_CYCLE_FALLING_POINT)) |
> +			       FIELD_PREP(PWM_ASPEED_DUTY_CYCLE_FALLING_POINT,
> +					  duty_pt),
> +		       priv->base + PWM_ASPEED_DUTY_CYCLE);

Same here ...

> +	}
> +
> +	writel((readl(priv->base + PWM_ASPEED_CTRL) &
> +		~(PWM_ASPEED_CTRL_CLK_DIV_H | PWM_ASPEED_CTRL_CLK_DIV_L |
> +		  PWM_ASPEED_CTRL_PIN_ENABLE | PWM_ASPEED_CTRL_CLK_ENABLE |
> +		  PWM_ASPEED_CTRL_INVERSE)) |
> +		       FIELD_PREP(PWM_ASPEED_CTRL_CLK_DIV_H, div_h) |
> +		       FIELD_PREP(PWM_ASPEED_CTRL_CLK_DIV_L, div_l) |
> +		       FIELD_PREP(PWM_ASPEED_CTRL_PIN_ENABLE, state->enabled) |
> +		       FIELD_PREP(PWM_ASPEED_CTRL_CLK_ENABLE, clk_en) |
> +		       FIELD_PREP(PWM_ASPEED_CTRL_INVERSE, state->polarity),
> +	       priv->base + PWM_ASPEED_CTRL);

... and here.

> +
> +	return 0;
> +}
> +
> +static const struct pwm_ops aspeed_pwm_ops = {
> +	.apply = aspeed_pwm_apply,
> +	.get_state = aspeed_pwm_get_state,
> +	.owner = THIS_MODULE,
> +};
> +
> +static void aspeed_pwm_reset_assert(void *data)
> +{
> +	struct reset_control *rst = data;
> +
> +	reset_control_assert(rst);
> +}
> +
> +static void aspeed_pwm_chip_remove(void *data)
> +{
> +	struct pwm_chip *chip = data;
> +
> +	pwmchip_remove(chip);
> +}

Erm... no.

> +static int aspeed_pwm_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	int ret;
> +	struct aspeed_pwm_data *priv;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(priv->base))
> +		return PTR_ERR(priv->base);
> +
> +	priv->clk = devm_clk_get_enabled(dev, NULL);
> +	if (IS_ERR(priv->clk))
> +		return dev_err_probe(dev, PTR_ERR(priv->clk),
> +				     "Couldn't get clock\n");
> +	priv->clk_source = clk_get_rate(priv->clk);
> +	priv->reset = devm_reset_control_get_shared(dev, NULL);
> +	if (IS_ERR(priv->reset))
> +		return dev_err_probe(dev, PTR_ERR(priv->reset),
> +				     "Couldn't get reset control\n");
> +
> +	ret = reset_control_deassert(priv->reset);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Couldn't deassert reset control\n");
> +
> +	ret = devm_add_action_or_reset(dev, aspeed_pwm_reset_assert,
> +				       priv->reset);
> +	if (ret)
> +		return ret;

So now you need that extra callback that you defined earlier plus these
four lines of code in order to ...

> +
> +	priv->chip.dev = dev;
> +	priv->chip.ops = &aspeed_pwm_ops;
> +	priv->chip.npwm = 1;
> +
> +	ret = pwmchip_add(&priv->chip);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "Failed to add PWM chip\n");

... avoid calling reset_control_assert() once here? These device-
managed functions are meant to help simplify things, but there's nothing
complicated about it in this driver, so don't do it.

> +	ret = devm_add_action_or_reset(dev, aspeed_pwm_chip_remove,
> +				       &priv->chip);
> +	if (ret)
> +		return ret;

Why not just use the driver's .remove() callback? There's nothing here
that would fail afterwards, so this will effectively get called only
during driver removal, so might as well use the idiomatic infrastructure
that exists for this.

> +	return 0;
> +}
> +
> +static const struct of_device_id of_pwm_match_table[] = {

That's a suboptimal name. Use a driver-specific prefix.

> +	{
> +		.compatible = "aspeed,ast2600-pwm",
> +	},
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, of_pwm_match_table);
> +
> +static struct platform_driver aspeed_pwm_driver = {
> +	.probe = aspeed_pwm_probe,
> +	.driver	= {

There's another tab that doesn't belong here.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [v6 2/4] dt-bindings: hwmon: Add ASPEED TACH Control documentation

[蔡承達]

Thierry Reding <thierry.reding@gmail.com> 於 2023年7月18日 週二 下午3:14寫道：
>
> On Mon, Jul 17, 2023 at 11:54:26PM -0700, Guenter Roeck wrote:
> > On 7/17/23 23:39, Thierry Reding wrote:
> > > On Tue, Jul 18, 2023 at 08:04:24AM +0200, Krzysztof Kozlowski wrote:
> > > > On 18/07/2023 06:01, 蔡承達 wrote:
> > > > > >
> > > > > > On 17/07/2023 11:01, 蔡承達 wrote:
> > > > > > > Guenter Roeck <linux@roeck-us.net> 於 2023年7月17日 週一 上午1:00寫道：
> > > > > > > >
> > > > > > > > On 7/16/23 09:08, Krzysztof Kozlowski wrote:
> > > > > > > >
> > > > > > > > [ ... ]
> > > > > > > >
> > > > > > > > > >
> > > > > > > > > > This patch serial doesn't use to binding the fan control h/w. It is
> > > > > > > > > > used to binding the two independent h/w blocks.
> > > > > > > > > > One is used to provide pwm output and another is used to monitor the
> > > > > > > > > > speed of the input.
> > > > > > > > > > My patch is used to point out that the pwm and the tach is the
> > > > > > > > > > different function and don't need to
> > > > > > > > > > bind together. You can not only combine them as the fan usage but also
> > > > > > > > > > treat them as the individual module for
> > > > > > > > > > use. For example: the pwm can use to be the beeper (pwm-beeper.c), the
> > > > > > > > > > tach can be used to monitor the heart beat signal.
> > > > > > > > >
> > > > > > > > > Isn't this exactly the same as in every other SoC? PWMs can be used in
> > > > > > > > > different ways?
> > > > > > > > >
> > > > > > > >
> > > > > > > > ... and in every fan controller. Not that it really makes sense because
> > > > > > > > normally the pwm controller part of such chips is tied to the fan input,
> > > > > > > > to enable automatic fan control, but it is technically possible.
> > > > > > > > In many cases this is also the case in SoCs, for example, in ast2500.
> > > > > > > > Apparently this was redesigned in ast2600 where they two blocks are
> > > > > > > > only lightly coupled (there are two pwm status bits in the fan status
> > > > > > > > register, but I have no idea what those mean). If the blocks are tightly
> > > > > > > > coupled, separate drivers don't really make sense.
> > > > > > > >
> > > > > > > > There are multiple ways to separate the pwm controller part from the
> > > > > > > > fan inputs if that is really necessary. One would be to provide a
> > > > > > > > sequence of address mappings, the other would be to pass the memory
> > > > > > > > region from an mfd driver. It is not necessary to have N instances
> > > > > > > > of the fan controller, even if the address space is not continuous.
> > > > > > > >
> > > > > > >
> > > > > > > Hi Guenter,
> > > > > > >
> > > > > > > May I ask about the meaning of the sequence of address mappings? It appears
> > > > > > > to consist of multiple tuples within the 'reg' property, indicating
> > > > > > > the usage of PWM/Tach
> > > > > > > registers within a single instance. After that I can use the dts like following:
> > > > > > >
> > > > > > > pwm: pwm@1e610000 {
> > > > > > > ...
> > > > > > > reg = <0x1e610000 0x8
> > > > > > > 0x1e610010 0x8
> > > > > > > 0x1e610020 0x8
> > > > > > > 0x1e610030 0x8
> > > > > > > 0x1e610040 0x8
> > > > > > > 0x1e610050 0x8
> > > > > > > 0x1e610060 0x8
> > > > > > > 0x1e610070 0x8
> > > > > > > 0x1e610080 0x8
> > > > > > > 0x1e610090 0x8
> > > > > > > 0x1e6100A0 0x8
> > > > > > > 0x1e6100B0 0x8
> > > > > > > 0x1e6100C0 0x8
> > > > > > > 0x1e6100D0 0x8
> > > > > > > 0x1e6100E0 0x8
> > > > > > > 0x1e6100F0 0x8>;
> > > > > >
> > > > > >
> > > > > > Uh, no... I mean, why? We keep pointing out that this should not be done
> > > > > > differently than any other SoC. Open any other SoC PWM controller and
> > > > > > tell me why this is different? Why this cannot be one address space?
> > > > >
> > > > > Hi Krzysztof,
> > > > >
> > > > > This is because the register layout for PWM and Tach is not continuous.
> > > > > Each PWM/Tach instance has its own set of controller registers, and they
> > > > > are independent of each other.
> > > >
> > > > Register layout is not continuous in many other devices, so again - why
> > > > this must be different?
> > > >
> > > > >
> > > > > For example:
> > > > > PWM0 uses registers 0x0 and 0x4, while Tach0 uses registers 0x8 and 0xc.
> > > > > PWM1 uses registers 0x10 and 0x14, while Tach1 uses registers 0x18 and 0x1c.
> > > > > ...
> > > > >
> > > > > To separate the PWM controller part from the fan inputs, Guenter has
> > > > > provided two methods.
> > > > > The first method involves passing the memory region from an MFD
> > > > > driver, which was the
> > > >
> > > > I have no clue how can you pass memory region
> > > > (Documentation/devicetree/bindings/reserved-memory/) from MFD and why
> > > > does it make sense here.
> > > >
> > > > > initial method I intended to use. However, it seems that this method
> > > > > does not make sense to you.
> > > > >
> > > > > Therefore, I would like to explore the second method suggested by
> > > > > Guenter, which involves providing
> > > > > a sequence of address mappings.
> > >
> > > At the risk of saying what others have said: given that there's a single
> > > reset line and a single clock line controlling all of these channels and
> > > given what I recall of how address demuxers work in chips, everything
> > > indicates that this is a single hardware block/device.
> > >
> > > So the way that this should be described in DT is:
> > >
> > >     pwm@1e610000 {
> > >             reg = <0x1e610000 0x100>;
> > >             clocks = ...;
> > >             resets = ...
> > >     };
> > >
> > > That'd be the most accurate representation of this hardware in DT. It is
> > > then up to the driver to expose this in any way you see fit. For Linux
> > > it may make sense to expose this as 16 PWM channels and 16 hardware
> > > monitoring devices. Other operating systems using the same DT may choose
> >
> > It is single chip. It should be a single hardware monitoring device with
> > 16 channels. I don't even want to think about the mess we'd get if people
> > start modeling a single chip as N hardware monitoring devices, one for
> > each monitoring channel supported by that chip. It would be even more messy
> > if the driver supporting those N devices would be marked for asynchronous
> > probe, which would result in random hwmon device assignments.
>
> Sorry, I badly worded it. What I meant to say was: one hardware
> monitoring device with 16 channels.
>
> > > to expose this differently, depending on their frameworks, etc. A simple
> > > operating system may not expose this as separate resources at all but
> > > instead directly program individual registers from this block.
> > >
> > > I'd also like to add that I think trying to split this up into multiple
> > > drivers in Linux is a bit overkill. In my opinion, though I know not
> > > everyone shares this view, it's perfectly fine for one driver to expose
> > > multiple types of resources. There's plenty of use-cases across the
> > > kernel where tightly coupled devices like this have a single driver that
> > > registers with multiple subsystems. Going through MFD only because this
> > > particular hardware doesn't split registers nicely along Linux subsystem
> > > boundaries.
> > >
> > > So FWIW, I'm fine carrying hwmon code in a PWM driver and I'm equally
> > > fine if PWM code ends up in a hwmon driver (or any other subsystem
> > > really) if that makes sense for a given hardware.
> > >
> >
> > I am fine either way as well, as long as we are talking about a single
> > hwmon device and not 16 of them.
>
> Excellent. Should make it pretty clear in which direction this should
> go.
>
Hi Thierry,

Thank you for organizing and providing guidance on this patch series.
I'd like to
confirm my understanding of the next steps. Based on the previous discussions,
I should proceed by combining the PWM and TACH drivers into a single driver.
This consolidated driver would then be exposed to both the hwmon and
pwm subsystems.
Base on this driver the dts node of this would like following:

pwm_tach: pwm-tach-controller@1e610000 {
compatible = "aspeed,ast2600-pwm-tach"
reg = <0x1e610000 0x100>;
clocks = <&syscon ASPEED_CLK_AHB>;
resets = <&syscon ASPEED_RESET_PWM>;
#pwm-cells = <3>;
#address-cells = <1>;
#size-cells = <0>;
fan@0 {
aspeed,fan-tach-ch = /bits/ 8 <0x00>;
};
fan@1 {
aspeed,fan-tach-ch = /bits/ 8 <0x01, 0x02>;
};
};

The subnode of the fan is utilized to create the hwmon interface,
which facilitates monitoring
the speed of the input tach pin.
If users wish to employ PWM for fan or beeper control, they can create
the necessary configuration
through the following DTS declaration:
fan0: pwm-fan0 {
compatible = "pwm-fan";
pwms = <&pwm_tach 0 40000 0>; /* Target freq:25 kHz */
cooling-min-state = <0>;
cooling-max-state = <3>;
#cooling-cells = <2>;
cooling-levels = <0 15 128 255>;
};

buzzer {
compatible = "pwm-beeper";
pwms = <&pwm_tach 1 500000 0>;
};

Thanks
Best Regards,
Billy Tsai

Re: [v6 2/4] dt-bindings: hwmon: Add ASPEED TACH Control documentation

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 7487 bytes --]

On Mon, Jul 17, 2023 at 11:54:26PM -0700, Guenter Roeck wrote:
> On 7/17/23 23:39, Thierry Reding wrote:
> > On Tue, Jul 18, 2023 at 08:04:24AM +0200, Krzysztof Kozlowski wrote:
> > > On 18/07/2023 06:01, 蔡承達 wrote:
> > > > > 
> > > > > On 17/07/2023 11:01, 蔡承達 wrote:
> > > > > > Guenter Roeck <linux@roeck-us.net> 於 2023年7月17日 週一 上午1:00寫道：
> > > > > > > 
> > > > > > > On 7/16/23 09:08, Krzysztof Kozlowski wrote:
> > > > > > > 
> > > > > > > [ ... ]
> > > > > > > 
> > > > > > > > > 
> > > > > > > > > This patch serial doesn't use to binding the fan control h/w. It is
> > > > > > > > > used to binding the two independent h/w blocks.
> > > > > > > > > One is used to provide pwm output and another is used to monitor the
> > > > > > > > > speed of the input.
> > > > > > > > > My patch is used to point out that the pwm and the tach is the
> > > > > > > > > different function and don't need to
> > > > > > > > > bind together. You can not only combine them as the fan usage but also
> > > > > > > > > treat them as the individual module for
> > > > > > > > > use. For example: the pwm can use to be the beeper (pwm-beeper.c), the
> > > > > > > > > tach can be used to monitor the heart beat signal.
> > > > > > > > 
> > > > > > > > Isn't this exactly the same as in every other SoC? PWMs can be used in
> > > > > > > > different ways?
> > > > > > > > 
> > > > > > > 
> > > > > > > ... and in every fan controller. Not that it really makes sense because
> > > > > > > normally the pwm controller part of such chips is tied to the fan input,
> > > > > > > to enable automatic fan control, but it is technically possible.
> > > > > > > In many cases this is also the case in SoCs, for example, in ast2500.
> > > > > > > Apparently this was redesigned in ast2600 where they two blocks are
> > > > > > > only lightly coupled (there are two pwm status bits in the fan status
> > > > > > > register, but I have no idea what those mean). If the blocks are tightly
> > > > > > > coupled, separate drivers don't really make sense.
> > > > > > > 
> > > > > > > There are multiple ways to separate the pwm controller part from the
> > > > > > > fan inputs if that is really necessary. One would be to provide a
> > > > > > > sequence of address mappings, the other would be to pass the memory
> > > > > > > region from an mfd driver. It is not necessary to have N instances
> > > > > > > of the fan controller, even if the address space is not continuous.
> > > > > > > 
> > > > > > 
> > > > > > Hi Guenter,
> > > > > > 
> > > > > > May I ask about the meaning of the sequence of address mappings? It appears
> > > > > > to consist of multiple tuples within the 'reg' property, indicating
> > > > > > the usage of PWM/Tach
> > > > > > registers within a single instance. After that I can use the dts like following:
> > > > > > 
> > > > > > pwm: pwm@1e610000 {
> > > > > > ...
> > > > > > reg = <0x1e610000 0x8
> > > > > > 0x1e610010 0x8
> > > > > > 0x1e610020 0x8
> > > > > > 0x1e610030 0x8
> > > > > > 0x1e610040 0x8
> > > > > > 0x1e610050 0x8
> > > > > > 0x1e610060 0x8
> > > > > > 0x1e610070 0x8
> > > > > > 0x1e610080 0x8
> > > > > > 0x1e610090 0x8
> > > > > > 0x1e6100A0 0x8
> > > > > > 0x1e6100B0 0x8
> > > > > > 0x1e6100C0 0x8
> > > > > > 0x1e6100D0 0x8
> > > > > > 0x1e6100E0 0x8
> > > > > > 0x1e6100F0 0x8>;
> > > > > 
> > > > > 
> > > > > Uh, no... I mean, why? We keep pointing out that this should not be done
> > > > > differently than any other SoC. Open any other SoC PWM controller and
> > > > > tell me why this is different? Why this cannot be one address space?
> > > > 
> > > > Hi Krzysztof,
> > > > 
> > > > This is because the register layout for PWM and Tach is not continuous.
> > > > Each PWM/Tach instance has its own set of controller registers, and they
> > > > are independent of each other.
> > > 
> > > Register layout is not continuous in many other devices, so again - why
> > > this must be different?
> > > 
> > > > 
> > > > For example:
> > > > PWM0 uses registers 0x0 and 0x4, while Tach0 uses registers 0x8 and 0xc.
> > > > PWM1 uses registers 0x10 and 0x14, while Tach1 uses registers 0x18 and 0x1c.
> > > > ...
> > > > 
> > > > To separate the PWM controller part from the fan inputs, Guenter has
> > > > provided two methods.
> > > > The first method involves passing the memory region from an MFD
> > > > driver, which was the
> > > 
> > > I have no clue how can you pass memory region
> > > (Documentation/devicetree/bindings/reserved-memory/) from MFD and why
> > > does it make sense here.
> > > 
> > > > initial method I intended to use. However, it seems that this method
> > > > does not make sense to you.
> > > > 
> > > > Therefore, I would like to explore the second method suggested by
> > > > Guenter, which involves providing
> > > > a sequence of address mappings.
> > 
> > At the risk of saying what others have said: given that there's a single
> > reset line and a single clock line controlling all of these channels and
> > given what I recall of how address demuxers work in chips, everything
> > indicates that this is a single hardware block/device.
> > 
> > So the way that this should be described in DT is:
> > 
> > 	pwm@1e610000 {
> > 		reg = <0x1e610000 0x100>;
> > 		clocks = ...;
> > 		resets = ...
> > 	};
> > 
> > That'd be the most accurate representation of this hardware in DT. It is
> > then up to the driver to expose this in any way you see fit. For Linux
> > it may make sense to expose this as 16 PWM channels and 16 hardware
> > monitoring devices. Other operating systems using the same DT may choose
> 
> It is single chip. It should be a single hardware monitoring device with
> 16 channels. I don't even want to think about the mess we'd get if people
> start modeling a single chip as N hardware monitoring devices, one for
> each monitoring channel supported by that chip. It would be even more messy
> if the driver supporting those N devices would be marked for asynchronous
> probe, which would result in random hwmon device assignments.

Sorry, I badly worded it. What I meant to say was: one hardware
monitoring device with 16 channels.

> > to expose this differently, depending on their frameworks, etc. A simple
> > operating system may not expose this as separate resources at all but
> > instead directly program individual registers from this block.
> > 
> > I'd also like to add that I think trying to split this up into multiple
> > drivers in Linux is a bit overkill. In my opinion, though I know not
> > everyone shares this view, it's perfectly fine for one driver to expose
> > multiple types of resources. There's plenty of use-cases across the
> > kernel where tightly coupled devices like this have a single driver that
> > registers with multiple subsystems. Going through MFD only because this
> > particular hardware doesn't split registers nicely along Linux subsystem
> > boundaries.
> > 
> > So FWIW, I'm fine carrying hwmon code in a PWM driver and I'm equally
> > fine if PWM code ends up in a hwmon driver (or any other subsystem
> > really) if that makes sense for a given hardware.
> > 
> 
> I am fine either way as well, as long as we are talking about a single
> hwmon device and not 16 of them.

Excellent. Should make it pretty clear in which direction this should
go.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [v6 2/4] dt-bindings: hwmon: Add ASPEED TACH Control documentation

[Guenter Roeck]

On 7/17/23 23:39, Thierry Reding wrote:
> On Tue, Jul 18, 2023 at 08:04:24AM +0200, Krzysztof Kozlowski wrote:
>> On 18/07/2023 06:01, 蔡承達 wrote:
>>>>
>>>> On 17/07/2023 11:01, 蔡承達 wrote:
>>>>> Guenter Roeck <linux@roeck-us.net> 於 2023年7月17日 週一 上午1:00寫道：
>>>>>>
>>>>>> On 7/16/23 09:08, Krzysztof Kozlowski wrote:
>>>>>>
>>>>>> [ ... ]
>>>>>>
>>>>>>>>
>>>>>>>> This patch serial doesn't use to binding the fan control h/w. It is
>>>>>>>> used to binding the two independent h/w blocks.
>>>>>>>> One is used to provide pwm output and another is used to monitor the
>>>>>>>> speed of the input.
>>>>>>>> My patch is used to point out that the pwm and the tach is the
>>>>>>>> different function and don't need to
>>>>>>>> bind together. You can not only combine them as the fan usage but also
>>>>>>>> treat them as the individual module for
>>>>>>>> use. For example: the pwm can use to be the beeper (pwm-beeper.c), the
>>>>>>>> tach can be used to monitor the heart beat signal.
>>>>>>>
>>>>>>> Isn't this exactly the same as in every other SoC? PWMs can be used in
>>>>>>> different ways?
>>>>>>>
>>>>>>
>>>>>> ... and in every fan controller. Not that it really makes sense because
>>>>>> normally the pwm controller part of such chips is tied to the fan input,
>>>>>> to enable automatic fan control, but it is technically possible.
>>>>>> In many cases this is also the case in SoCs, for example, in ast2500.
>>>>>> Apparently this was redesigned in ast2600 where they two blocks are
>>>>>> only lightly coupled (there are two pwm status bits in the fan status
>>>>>> register, but I have no idea what those mean). If the blocks are tightly
>>>>>> coupled, separate drivers don't really make sense.
>>>>>>
>>>>>> There are multiple ways to separate the pwm controller part from the
>>>>>> fan inputs if that is really necessary. One would be to provide a
>>>>>> sequence of address mappings, the other would be to pass the memory
>>>>>> region from an mfd driver. It is not necessary to have N instances
>>>>>> of the fan controller, even if the address space is not continuous.
>>>>>>
>>>>>
>>>>> Hi Guenter,
>>>>>
>>>>> May I ask about the meaning of the sequence of address mappings? It appears
>>>>> to consist of multiple tuples within the 'reg' property, indicating
>>>>> the usage of PWM/Tach
>>>>> registers within a single instance. After that I can use the dts like following:
>>>>>
>>>>> pwm: pwm@1e610000 {
>>>>> ...
>>>>> reg = <0x1e610000 0x8
>>>>> 0x1e610010 0x8
>>>>> 0x1e610020 0x8
>>>>> 0x1e610030 0x8
>>>>> 0x1e610040 0x8
>>>>> 0x1e610050 0x8
>>>>> 0x1e610060 0x8
>>>>> 0x1e610070 0x8
>>>>> 0x1e610080 0x8
>>>>> 0x1e610090 0x8
>>>>> 0x1e6100A0 0x8
>>>>> 0x1e6100B0 0x8
>>>>> 0x1e6100C0 0x8
>>>>> 0x1e6100D0 0x8
>>>>> 0x1e6100E0 0x8
>>>>> 0x1e6100F0 0x8>;
>>>>
>>>>
>>>> Uh, no... I mean, why? We keep pointing out that this should not be done
>>>> differently than any other SoC. Open any other SoC PWM controller and
>>>> tell me why this is different? Why this cannot be one address space?
>>>
>>> Hi Krzysztof,
>>>
>>> This is because the register layout for PWM and Tach is not continuous.
>>> Each PWM/Tach instance has its own set of controller registers, and they
>>> are independent of each other.
>>
>> Register layout is not continuous in many other devices, so again - why
>> this must be different?
>>
>>>
>>> For example:
>>> PWM0 uses registers 0x0 and 0x4, while Tach0 uses registers 0x8 and 0xc.
>>> PWM1 uses registers 0x10 and 0x14, while Tach1 uses registers 0x18 and 0x1c.
>>> ...
>>>
>>> To separate the PWM controller part from the fan inputs, Guenter has
>>> provided two methods.
>>> The first method involves passing the memory region from an MFD
>>> driver, which was the
>>
>> I have no clue how can you pass memory region
>> (Documentation/devicetree/bindings/reserved-memory/) from MFD and why
>> does it make sense here.
>>
>>> initial method I intended to use. However, it seems that this method
>>> does not make sense to you.
>>>
>>> Therefore, I would like to explore the second method suggested by
>>> Guenter, which involves providing
>>> a sequence of address mappings.
> 
> At the risk of saying what others have said: given that there's a single
> reset line and a single clock line controlling all of these channels and
> given what I recall of how address demuxers work in chips, everything
> indicates that this is a single hardware block/device.
> 
> So the way that this should be described in DT is:
> 
> 	pwm@1e610000 {
> 		reg = <0x1e610000 0x100>;
> 		clocks = ...;
> 		resets = ...
> 	};
> 
> That'd be the most accurate representation of this hardware in DT. It is
> then up to the driver to expose this in any way you see fit. For Linux
> it may make sense to expose this as 16 PWM channels and 16 hardware
> monitoring devices. Other operating systems using the same DT may choose

It is single chip. It should be a single hardware monitoring device with
16 channels. I don't even want to think about the mess we'd get if people
start modeling a single chip as N hardware monitoring devices, one for
each monitoring channel supported by that chip. It would be even more messy
if the driver supporting those N devices would be marked for asynchronous
probe, which would result in random hwmon device assignments.

> to expose this differently, depending on their frameworks, etc. A simple
> operating system may not expose this as separate resources at all but
> instead directly program individual registers from this block.
> 
> I'd also like to add that I think trying to split this up into multiple
> drivers in Linux is a bit overkill. In my opinion, though I know not
> everyone shares this view, it's perfectly fine for one driver to expose
> multiple types of resources. There's plenty of use-cases across the
> kernel where tightly coupled devices like this have a single driver that
> registers with multiple subsystems. Going through MFD only because this
> particular hardware doesn't split registers nicely along Linux subsystem
> boundaries.
> 
> So FWIW, I'm fine carrying hwmon code in a PWM driver and I'm equally
> fine if PWM code ends up in a hwmon driver (or any other subsystem
> really) if that makes sense for a given hardware.
> 

I am fine either way as well, as long as we are talking about a single
hwmon device and not 16 of them.

Guenter

Re: [v6 2/4] dt-bindings: hwmon: Add ASPEED TACH Control documentation

[Guenter Roeck]

On 7/17/23 23:04, Krzysztof Kozlowski wrote:

[ ... ]

>> The first method involves passing the memory region from an MFD
>> driver, which was the
> 
> I have no clue how can you pass memory region
> (Documentation/devicetree/bindings/reserved-memory/) from MFD and why
> does it make sense here.
> 

It is quite common for mfd drivers to pass regmap information to
child drivers. Child drivers typically call dev_get_regmap() with
the parent device as parameter. Granted, that is typically used
for spi or i2c devices, but I don't see why it would not work
here as well.

Then there s syscon, but I don't know if that works with overlapping
memory regions.

Guenter

Re: [v6 2/4] dt-bindings: hwmon: Add ASPEED TACH Control documentation

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 5866 bytes --]

On Tue, Jul 18, 2023 at 08:04:24AM +0200, Krzysztof Kozlowski wrote:
> On 18/07/2023 06:01, 蔡承達 wrote:
> >>
> >> On 17/07/2023 11:01, 蔡承達 wrote:
> >>> Guenter Roeck <linux@roeck-us.net> 於 2023年7月17日 週一 上午1:00寫道：
> >>>>
> >>>> On 7/16/23 09:08, Krzysztof Kozlowski wrote:
> >>>>
> >>>> [ ... ]
> >>>>
> >>>>>>
> >>>>>> This patch serial doesn't use to binding the fan control h/w. It is
> >>>>>> used to binding the two independent h/w blocks.
> >>>>>> One is used to provide pwm output and another is used to monitor the
> >>>>>> speed of the input.
> >>>>>> My patch is used to point out that the pwm and the tach is the
> >>>>>> different function and don't need to
> >>>>>> bind together. You can not only combine them as the fan usage but also
> >>>>>> treat them as the individual module for
> >>>>>> use. For example: the pwm can use to be the beeper (pwm-beeper.c), the
> >>>>>> tach can be used to monitor the heart beat signal.
> >>>>>
> >>>>> Isn't this exactly the same as in every other SoC? PWMs can be used in
> >>>>> different ways?
> >>>>>
> >>>>
> >>>> ... and in every fan controller. Not that it really makes sense because
> >>>> normally the pwm controller part of such chips is tied to the fan input,
> >>>> to enable automatic fan control, but it is technically possible.
> >>>> In many cases this is also the case in SoCs, for example, in ast2500.
> >>>> Apparently this was redesigned in ast2600 where they two blocks are
> >>>> only lightly coupled (there are two pwm status bits in the fan status
> >>>> register, but I have no idea what those mean). If the blocks are tightly
> >>>> coupled, separate drivers don't really make sense.
> >>>>
> >>>> There are multiple ways to separate the pwm controller part from the
> >>>> fan inputs if that is really necessary. One would be to provide a
> >>>> sequence of address mappings, the other would be to pass the memory
> >>>> region from an mfd driver. It is not necessary to have N instances
> >>>> of the fan controller, even if the address space is not continuous.
> >>>>
> >>>
> >>> Hi Guenter,
> >>>
> >>> May I ask about the meaning of the sequence of address mappings? It appears
> >>> to consist of multiple tuples within the 'reg' property, indicating
> >>> the usage of PWM/Tach
> >>> registers within a single instance. After that I can use the dts like following:
> >>>
> >>> pwm: pwm@1e610000 {
> >>> ...
> >>> reg = <0x1e610000 0x8
> >>> 0x1e610010 0x8
> >>> 0x1e610020 0x8
> >>> 0x1e610030 0x8
> >>> 0x1e610040 0x8
> >>> 0x1e610050 0x8
> >>> 0x1e610060 0x8
> >>> 0x1e610070 0x8
> >>> 0x1e610080 0x8
> >>> 0x1e610090 0x8
> >>> 0x1e6100A0 0x8
> >>> 0x1e6100B0 0x8
> >>> 0x1e6100C0 0x8
> >>> 0x1e6100D0 0x8
> >>> 0x1e6100E0 0x8
> >>> 0x1e6100F0 0x8>;
> >>
> >>
> >> Uh, no... I mean, why? We keep pointing out that this should not be done
> >> differently than any other SoC. Open any other SoC PWM controller and
> >> tell me why this is different? Why this cannot be one address space?
> > 
> > Hi Krzysztof,
> > 
> > This is because the register layout for PWM and Tach is not continuous.
> > Each PWM/Tach instance has its own set of controller registers, and they
> > are independent of each other.
> 
> Register layout is not continuous in many other devices, so again - why
> this must be different?
> 
> > 
> > For example:
> > PWM0 uses registers 0x0 and 0x4, while Tach0 uses registers 0x8 and 0xc.
> > PWM1 uses registers 0x10 and 0x14, while Tach1 uses registers 0x18 and 0x1c.
> > ...
> > 
> > To separate the PWM controller part from the fan inputs, Guenter has
> > provided two methods.
> > The first method involves passing the memory region from an MFD
> > driver, which was the
> 
> I have no clue how can you pass memory region
> (Documentation/devicetree/bindings/reserved-memory/) from MFD and why
> does it make sense here.
> 
> > initial method I intended to use. However, it seems that this method
> > does not make sense to you.
> > 
> > Therefore, I would like to explore the second method suggested by
> > Guenter, which involves providing
> > a sequence of address mappings.

At the risk of saying what others have said: given that there's a single
reset line and a single clock line controlling all of these channels and
given what I recall of how address demuxers work in chips, everything
indicates that this is a single hardware block/device.

So the way that this should be described in DT is:

	pwm@1e610000 {
		reg = <0x1e610000 0x100>;
		clocks = ...;
		resets = ...
	};

That'd be the most accurate representation of this hardware in DT. It is
then up to the driver to expose this in any way you see fit. For Linux
it may make sense to expose this as 16 PWM channels and 16 hardware
monitoring devices. Other operating systems using the same DT may choose
to expose this differently, depending on their frameworks, etc. A simple
operating system may not expose this as separate resources at all but
instead directly program individual registers from this block.

I'd also like to add that I think trying to split this up into multiple
drivers in Linux is a bit overkill. In my opinion, though I know not
everyone shares this view, it's perfectly fine for one driver to expose
multiple types of resources. There's plenty of use-cases across the
kernel where tightly coupled devices like this have a single driver that
registers with multiple subsystems. Going through MFD only because this
particular hardware doesn't split registers nicely along Linux subsystem
boundaries.

So FWIW, I'm fine carrying hwmon code in a PWM driver and I'm equally
fine if PWM code ends up in a hwmon driver (or any other subsystem
really) if that makes sense for a given hardware.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [v6 2/4] dt-bindings: hwmon: Add ASPEED TACH Control documentation

[Krzysztof Kozlowski]

On 18/07/2023 06:01, 蔡承達 wrote:
>>
>> On 17/07/2023 11:01, 蔡承達 wrote:
>>> Guenter Roeck <linux@roeck-us.net> 於 2023年7月17日 週一 上午1:00寫道：
>>>>
>>>> On 7/16/23 09:08, Krzysztof Kozlowski wrote:
>>>>
>>>> [ ... ]
>>>>
>>>>>>
>>>>>> This patch serial doesn't use to binding the fan control h/w. It is
>>>>>> used to binding the two independent h/w blocks.
>>>>>> One is used to provide pwm output and another is used to monitor the
>>>>>> speed of the input.
>>>>>> My patch is used to point out that the pwm and the tach is the
>>>>>> different function and don't need to
>>>>>> bind together. You can not only combine them as the fan usage but also
>>>>>> treat them as the individual module for
>>>>>> use. For example: the pwm can use to be the beeper (pwm-beeper.c), the
>>>>>> tach can be used to monitor the heart beat signal.
>>>>>
>>>>> Isn't this exactly the same as in every other SoC? PWMs can be used in
>>>>> different ways?
>>>>>
>>>>
>>>> ... and in every fan controller. Not that it really makes sense because
>>>> normally the pwm controller part of such chips is tied to the fan input,
>>>> to enable automatic fan control, but it is technically possible.
>>>> In many cases this is also the case in SoCs, for example, in ast2500.
>>>> Apparently this was redesigned in ast2600 where they two blocks are
>>>> only lightly coupled (there are two pwm status bits in the fan status
>>>> register, but I have no idea what those mean). If the blocks are tightly
>>>> coupled, separate drivers don't really make sense.
>>>>
>>>> There are multiple ways to separate the pwm controller part from the
>>>> fan inputs if that is really necessary. One would be to provide a
>>>> sequence of address mappings, the other would be to pass the memory
>>>> region from an mfd driver. It is not necessary to have N instances
>>>> of the fan controller, even if the address space is not continuous.
>>>>
>>>
>>> Hi Guenter,
>>>
>>> May I ask about the meaning of the sequence of address mappings? It appears
>>> to consist of multiple tuples within the 'reg' property, indicating
>>> the usage of PWM/Tach
>>> registers within a single instance. After that I can use the dts like following:
>>>
>>> pwm: pwm@1e610000 {
>>> ...
>>> reg = <0x1e610000 0x8
>>> 0x1e610010 0x8
>>> 0x1e610020 0x8
>>> 0x1e610030 0x8
>>> 0x1e610040 0x8
>>> 0x1e610050 0x8
>>> 0x1e610060 0x8
>>> 0x1e610070 0x8
>>> 0x1e610080 0x8
>>> 0x1e610090 0x8
>>> 0x1e6100A0 0x8
>>> 0x1e6100B0 0x8
>>> 0x1e6100C0 0x8
>>> 0x1e6100D0 0x8
>>> 0x1e6100E0 0x8
>>> 0x1e6100F0 0x8>;
>>
>>
>> Uh, no... I mean, why? We keep pointing out that this should not be done
>> differently than any other SoC. Open any other SoC PWM controller and
>> tell me why this is different? Why this cannot be one address space?
> 
> Hi Krzysztof,
> 
> This is because the register layout for PWM and Tach is not continuous.
> Each PWM/Tach instance has its own set of controller registers, and they
> are independent of each other.

Register layout is not continuous in many other devices, so again - why
this must be different?

> 
> For example:
> PWM0 uses registers 0x0 and 0x4, while Tach0 uses registers 0x8 and 0xc.
> PWM1 uses registers 0x10 and 0x14, while Tach1 uses registers 0x18 and 0x1c.
> ...
> 
> To separate the PWM controller part from the fan inputs, Guenter has
> provided two methods.
> The first method involves passing the memory region from an MFD
> driver, which was the

I have no clue how can you pass memory region
(Documentation/devicetree/bindings/reserved-memory/) from MFD and why
does it make sense here.

> initial method I intended to use. However, it seems that this method
> does not make sense to you.
> 
> Therefore, I would like to explore the second method suggested by
> Guenter, which involves providing
> a sequence of address mappings.

Best regards,
Krzysztof

Re: [v6 2/4] dt-bindings: hwmon: Add ASPEED TACH Control documentation

[蔡承達]

>
> On 17/07/2023 11:01, 蔡承達 wrote:
> > Guenter Roeck <linux@roeck-us.net> 於 2023年7月17日 週一 上午1:00寫道：
> >>
> >> On 7/16/23 09:08, Krzysztof Kozlowski wrote:
> >>
> >> [ ... ]
> >>
> >>>>
> >>>> This patch serial doesn't use to binding the fan control h/w. It is
> >>>> used to binding the two independent h/w blocks.
> >>>> One is used to provide pwm output and another is used to monitor the
> >>>> speed of the input.
> >>>> My patch is used to point out that the pwm and the tach is the
> >>>> different function and don't need to
> >>>> bind together. You can not only combine them as the fan usage but also
> >>>> treat them as the individual module for
> >>>> use. For example: the pwm can use to be the beeper (pwm-beeper.c), the
> >>>> tach can be used to monitor the heart beat signal.
> >>>
> >>> Isn't this exactly the same as in every other SoC? PWMs can be used in
> >>> different ways?
> >>>
> >>
> >> ... and in every fan controller. Not that it really makes sense because
> >> normally the pwm controller part of such chips is tied to the fan input,
> >> to enable automatic fan control, but it is technically possible.
> >> In many cases this is also the case in SoCs, for example, in ast2500.
> >> Apparently this was redesigned in ast2600 where they two blocks are
> >> only lightly coupled (there are two pwm status bits in the fan status
> >> register, but I have no idea what those mean). If the blocks are tightly
> >> coupled, separate drivers don't really make sense.
> >>
> >> There are multiple ways to separate the pwm controller part from the
> >> fan inputs if that is really necessary. One would be to provide a
> >> sequence of address mappings, the other would be to pass the memory
> >> region from an mfd driver. It is not necessary to have N instances
> >> of the fan controller, even if the address space is not continuous.
> >>
> >
> > Hi Guenter,
> >
> > May I ask about the meaning of the sequence of address mappings? It appears
> > to consist of multiple tuples within the 'reg' property, indicating
> > the usage of PWM/Tach
> > registers within a single instance. After that I can use the dts like following:
> >
> > pwm: pwm@1e610000 {
> > ...
> > reg = <0x1e610000 0x8
> > 0x1e610010 0x8
> > 0x1e610020 0x8
> > 0x1e610030 0x8
> > 0x1e610040 0x8
> > 0x1e610050 0x8
> > 0x1e610060 0x8
> > 0x1e610070 0x8
> > 0x1e610080 0x8
> > 0x1e610090 0x8
> > 0x1e6100A0 0x8
> > 0x1e6100B0 0x8
> > 0x1e6100C0 0x8
> > 0x1e6100D0 0x8
> > 0x1e6100E0 0x8
> > 0x1e6100F0 0x8>;
>
>
> Uh, no... I mean, why? We keep pointing out that this should not be done
> differently than any other SoC. Open any other SoC PWM controller and
> tell me why this is different? Why this cannot be one address space?

Hi Krzysztof,

This is because the register layout for PWM and Tach is not continuous.
Each PWM/Tach instance has its own set of controller registers, and they
are independent of each other.

For example:
PWM0 uses registers 0x0 and 0x4, while Tach0 uses registers 0x8 and 0xc.
PWM1 uses registers 0x10 and 0x14, while Tach1 uses registers 0x18 and 0x1c.
...

To separate the PWM controller part from the fan inputs, Guenter has
provided two methods.
The first method involves passing the memory region from an MFD
driver, which was the
initial method I intended to use. However, it seems that this method
does not make sense to you.

Therefore, I would like to explore the second method suggested by
Guenter, which involves providing
a sequence of address mappings.

Thanks

Best Regards,
Billy Tsai

Re: [v6 2/4] dt-bindings: hwmon: Add ASPEED TACH Control documentation

[Krzysztof Kozlowski]

On 17/07/2023 11:01, 蔡承達 wrote:
> Guenter Roeck <linux@roeck-us.net> 於 2023年7月17日 週一 上午1:00寫道：
>>
>> On 7/16/23 09:08, Krzysztof Kozlowski wrote:
>>
>> [ ... ]
>>
>>>>
>>>> This patch serial doesn't use to binding the fan control h/w. It is
>>>> used to binding the two independent h/w blocks.
>>>> One is used to provide pwm output and another is used to monitor the
>>>> speed of the input.
>>>> My patch is used to point out that the pwm and the tach is the
>>>> different function and don't need to
>>>> bind together. You can not only combine them as the fan usage but also
>>>> treat them as the individual module for
>>>> use. For example: the pwm can use to be the beeper (pwm-beeper.c), the
>>>> tach can be used to monitor the heart beat signal.
>>>
>>> Isn't this exactly the same as in every other SoC? PWMs can be used in
>>> different ways?
>>>
>>
>> ... and in every fan controller. Not that it really makes sense because
>> normally the pwm controller part of such chips is tied to the fan input,
>> to enable automatic fan control, but it is technically possible.
>> In many cases this is also the case in SoCs, for example, in ast2500.
>> Apparently this was redesigned in ast2600 where they two blocks are
>> only lightly coupled (there are two pwm status bits in the fan status
>> register, but I have no idea what those mean). If the blocks are tightly
>> coupled, separate drivers don't really make sense.
>>
>> There are multiple ways to separate the pwm controller part from the
>> fan inputs if that is really necessary. One would be to provide a
>> sequence of address mappings, the other would be to pass the memory
>> region from an mfd driver. It is not necessary to have N instances
>> of the fan controller, even if the address space is not continuous.
>>
> 
> Hi Guenter,
> 
> May I ask about the meaning of the sequence of address mappings? It appears
> to consist of multiple tuples within the 'reg' property, indicating
> the usage of PWM/Tach
> registers within a single instance. After that I can use the dts like following:
> 
> pwm: pwm@1e610000 {
> ...
> reg = <0x1e610000 0x8
> 0x1e610010 0x8
> 0x1e610020 0x8
> 0x1e610030 0x8
> 0x1e610040 0x8
> 0x1e610050 0x8
> 0x1e610060 0x8
> 0x1e610070 0x8
> 0x1e610080 0x8
> 0x1e610090 0x8
> 0x1e6100A0 0x8
> 0x1e6100B0 0x8
> 0x1e6100C0 0x8
> 0x1e6100D0 0x8
> 0x1e6100E0 0x8
> 0x1e6100F0 0x8>;


Uh, no... I mean, why? We keep pointing out that this should not be done
differently than any other SoC. Open any other SoC PWM controller and
tell me why this is different? Why this cannot be one address space?

Best regards,
Krzysztof

Re: [v6 2/4] dt-bindings: hwmon: Add ASPEED TACH Control documentation

[蔡承達]

Guenter Roeck <linux@roeck-us.net> 於 2023年7月17日 週一 上午1:00寫道：
>
> On 7/16/23 09:08, Krzysztof Kozlowski wrote:
>
> [ ... ]
>
> >>
> >> This patch serial doesn't use to binding the fan control h/w. It is
> >> used to binding the two independent h/w blocks.
> >> One is used to provide pwm output and another is used to monitor the
> >> speed of the input.
> >> My patch is used to point out that the pwm and the tach is the
> >> different function and don't need to
> >> bind together. You can not only combine them as the fan usage but also
> >> treat them as the individual module for
> >> use. For example: the pwm can use to be the beeper (pwm-beeper.c), the
> >> tach can be used to monitor the heart beat signal.
> >
> > Isn't this exactly the same as in every other SoC? PWMs can be used in
> > different ways?
> >
>
> ... and in every fan controller. Not that it really makes sense because
> normally the pwm controller part of such chips is tied to the fan input,
> to enable automatic fan control, but it is technically possible.
> In many cases this is also the case in SoCs, for example, in ast2500.
> Apparently this was redesigned in ast2600 where they two blocks are
> only lightly coupled (there are two pwm status bits in the fan status
> register, but I have no idea what those mean). If the blocks are tightly
> coupled, separate drivers don't really make sense.
>
> There are multiple ways to separate the pwm controller part from the
> fan inputs if that is really necessary. One would be to provide a
> sequence of address mappings, the other would be to pass the memory
> region from an mfd driver. It is not necessary to have N instances
> of the fan controller, even if the address space is not continuous.
>

Hi Guenter,

May I ask about the meaning of the sequence of address mappings? It appears
to consist of multiple tuples within the 'reg' property, indicating
the usage of PWM/Tach
registers within a single instance. After that I can use the dts like following:

pwm: pwm@1e610000 {
...
reg = <0x1e610000 0x8
0x1e610010 0x8
0x1e610020 0x8
0x1e610030 0x8
0x1e610040 0x8
0x1e610050 0x8
0x1e610060 0x8
0x1e610070 0x8
0x1e610080 0x8
0x1e610090 0x8
0x1e6100A0 0x8
0x1e6100B0 0x8
0x1e6100C0 0x8
0x1e6100D0 0x8
0x1e6100E0 0x8
0x1e6100F0 0x8>;
...
};

tach: tach@
...
reg = <0x1e610008 0x8
0x1e610018 0x8
0x1e610028 0x8
0x1e610038 0x8
0x1e610048 0x8
0x1e610058 0x8
0x1e610068 0x8
0x1e610078 0x8
0x1e610088 0x8
0x1e610098 0x8
0x1e6100A8 0x8
0x1e6100B8 0x8
0x1e6100C8 0x8
0x1e6100D8 0x8
0x1e6100E8 0x8
0x1e6100F8 0x8>;
...
};

correct?

Thanks

> Guenter
>
> > Anyway, it is tricky to keep the discussion since you avoid posting
> > entire DTS. I already said:
> >
> > "I will start NAKing such patches without DTS user. It's like reviewing
> > fake code for some unknown solution and trying to get from you piece of
> > answers one by one, because you do not want to share entire part."
> >

Hi Krzysztof,

Do you mean the DTS example of the usage in the binding, like the
following right?
PWM:
pwm0: pwm0@1e610000 {
compatible = "aspeed,ast2600-pwm";
reg = <0x1e610000 0x8>;
#pwm-cells = <3>;
#address-cells = <1>;
#size-cells = <0>;
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_pwm0_default>;
clocks = <&syscon ASPEED_CLK_AHB>;
resets = <&syscon ASPEED_RESET_PWM>;
};

TACH:
examples:
- |
tach0: tach0@1e610008 {
        compatible = "aspeed,ast2600-tach";
        reg = <0x1e610008 0x8>;
        #address-cells = <1>;
        #size-cells = <0>;
        pinctrl-names = "default";
        pinctrl-0 = <&pinctrl_tach0_default>;
        clocks = <&syscon ASPEED_CLK_AHB>;
        resets = <&syscon ASPEED_RESET_PWM>;
};

Thanks

> >
> >
> > Best regards,
> > Krzysztof
> >
>

Re: [v6 2/4] dt-bindings: hwmon: Add ASPEED TACH Control documentation

[Guenter Roeck]

On 7/16/23 09:08, Krzysztof Kozlowski wrote:

[ ... ]

>>
>> This patch serial doesn't use to binding the fan control h/w. It is
>> used to binding the two independent h/w blocks.
>> One is used to provide pwm output and another is used to monitor the
>> speed of the input.
>> My patch is used to point out that the pwm and the tach is the
>> different function and don't need to
>> bind together. You can not only combine them as the fan usage but also
>> treat them as the individual module for
>> use. For example: the pwm can use to be the beeper (pwm-beeper.c), the
>> tach can be used to monitor the heart beat signal.
> 
> Isn't this exactly the same as in every other SoC? PWMs can be used in
> different ways?
> 

... and in every fan controller. Not that it really makes sense because
normally the pwm controller part of such chips is tied to the fan input,
to enable automatic fan control, but it is technically possible.
In many cases this is also the case in SoCs, for example, in ast2500.
Apparently this was redesigned in ast2600 where they two blocks are
only lightly coupled (there are two pwm status bits in the fan status
register, but I have no idea what those mean). If the blocks are tightly
coupled, separate drivers don't really make sense.

There are multiple ways to separate the pwm controller part from the
fan inputs if that is really necessary. One would be to provide a
sequence of address mappings, the other would be to pass the memory
region from an mfd driver. It is not necessary to have N instances
of the fan controller, even if the address space is not continuous.

Guenter

> Anyway, it is tricky to keep the discussion since you avoid posting
> entire DTS. I already said:
> 
> "I will start NAKing such patches without DTS user. It's like reviewing
> fake code for some unknown solution and trying to get from you piece of
> answers one by one, because you do not want to share entire part."
> 
> 
> 
> Best regards,
> Krzysztof
> 

Re: [v6 2/4] dt-bindings: hwmon: Add ASPEED TACH Control documentation

[Krzysztof Kozlowski]

On 14/07/2023 13:17, 蔡承達 wrote:
> Guenter Roeck <linux@roeck-us.net> 於 2023年7月14日 週五 下午6:26寫道：
>>
>> On 7/14/23 03:18, 蔡承達 wrote:
>>> Guenter Roeck <linux@roeck-us.net> 於 2023年7月14日 週五 下午5:59寫道：
>>>>
>>>> On 7/14/23 00:13, Krzysztof Kozlowski wrote:
>>>>> On 14/07/2023 09:04, 蔡承達 wrote:
>>>>>
>>>>>>           > This is because our register layout for PWM and Tach is not
>>>>>> continuous.
>>>>>>
>>>>>>           > PWM0 used 0x0 0x4, Tach0 used 0x8 0xc
>>>>>>
>>>>>>           > PWM1 used 0x10 0x14, Tach1 used 0x18 0x1c
>>>>>>
>>>>>>           > ...
>>>>>>
>>>>>>           > Each PWM/Tach instance has its own controller register and is not
>>>>>> dependent on others.
>>>>>
>>>>> Your email reply quoting style is very difficult to read.
>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> Hi Guenter,
>>>>>>
>>>>>>
>>>>>>
>>>>>> Did you receive a response to my previous email?
>>>>>>
>>>>>> I would like to inquire if you have any further concerns regarding the PWM
>>>>>> and Tach with 16 instances.
>>>>>
>>>>> But isn't like this in all PWMs in all SoCs?
>>>>>
>>>>
>>>> Correct, pretty much every fan controller is implemented that way.
>>>> I don't understand the logic.
>>>>
>>>
>>> Hi Krzysztof and Guenter,
>>>
>>> Apologies for any confusion earlier.
>>> So, you think that the implementation with 16 instances of TACH/PWM
>>> device nodes in dts instead of one is ok to you, right?
>>>
>>
>> Did I say that ? No, it is not ok with me. It is confusing and doesn't make
>> sense to me. This is one fan controller with 16 channels, not 16 separate
>> controllers.
>>
> 
> This patch serial doesn't use to binding the fan control h/w. It is
> used to binding the two independent h/w blocks.
> One is used to provide pwm output and another is used to monitor the
> speed of the input.
> My patch is used to point out that the pwm and the tach is the
> different function and don't need to
> bind together. You can not only combine them as the fan usage but also
> treat them as the individual module for
> use. For example: the pwm can use to be the beeper (pwm-beeper.c), the
> tach can be used to monitor the heart beat signal.

Isn't this exactly the same as in every other SoC? PWMs can be used in
different ways?

Anyway, it is tricky to keep the discussion since you avoid posting
entire DTS. I already said:

"I will start NAKing such patches without DTS user. It's like reviewing
fake code for some unknown solution and trying to get from you piece of
answers one by one, because you do not want to share entire part."



Best regards,
Krzysztof

Re: [v6 2/4] dt-bindings: hwmon: Add ASPEED TACH Control documentation

[蔡承達]

Guenter Roeck <linux@roeck-us.net> 於 2023年7月14日 週五 下午6:26寫道：
>
> On 7/14/23 03:18, 蔡承達 wrote:
> > Guenter Roeck <linux@roeck-us.net> 於 2023年7月14日 週五 下午5:59寫道：
> >>
> >> On 7/14/23 00:13, Krzysztof Kozlowski wrote:
> >>> On 14/07/2023 09:04, 蔡承達 wrote:
> >>>
> >>>>           > This is because our register layout for PWM and Tach is not
> >>>> continuous.
> >>>>
> >>>>           > PWM0 used 0x0 0x4, Tach0 used 0x8 0xc
> >>>>
> >>>>           > PWM1 used 0x10 0x14, Tach1 used 0x18 0x1c
> >>>>
> >>>>           > ...
> >>>>
> >>>>           > Each PWM/Tach instance has its own controller register and is not
> >>>> dependent on others.
> >>>
> >>> Your email reply quoting style is very difficult to read.
> >>>
> >>>>
> >>>>
> >>>>
> >>>> Hi Guenter,
> >>>>
> >>>>
> >>>>
> >>>> Did you receive a response to my previous email?
> >>>>
> >>>> I would like to inquire if you have any further concerns regarding the PWM
> >>>> and Tach with 16 instances.
> >>>
> >>> But isn't like this in all PWMs in all SoCs?
> >>>
> >>
> >> Correct, pretty much every fan controller is implemented that way.
> >> I don't understand the logic.
> >>
> >
> > Hi Krzysztof and Guenter,
> >
> > Apologies for any confusion earlier.
> > So, you think that the implementation with 16 instances of TACH/PWM
> > device nodes in dts instead of one is ok to you, right?
> >
>
> Did I say that ? No, it is not ok with me. It is confusing and doesn't make
> sense to me. This is one fan controller with 16 channels, not 16 separate
> controllers.
>

This patch serial doesn't use to binding the fan control h/w. It is
used to binding the two independent h/w blocks.
One is used to provide pwm output and another is used to monitor the
speed of the input.
My patch is used to point out that the pwm and the tach is the
different function and don't need to
bind together. You can not only combine them as the fan usage but also
treat them as the individual module for
use. For example: the pwm can use to be the beeper (pwm-beeper.c), the
tach can be used to monitor the heart beat signal.

Thanks

Re: [v6 2/4] dt-bindings: hwmon: Add ASPEED TACH Control documentation

[Guenter Roeck]

On 7/14/23 03:18, 蔡承達 wrote:
> Guenter Roeck <linux@roeck-us.net> 於 2023年7月14日 週五 下午5:59寫道：
>>
>> On 7/14/23 00:13, Krzysztof Kozlowski wrote:
>>> On 14/07/2023 09:04, 蔡承達 wrote:
>>>
>>>>           > This is because our register layout for PWM and Tach is not
>>>> continuous.
>>>>
>>>>           > PWM0 used 0x0 0x4, Tach0 used 0x8 0xc
>>>>
>>>>           > PWM1 used 0x10 0x14, Tach1 used 0x18 0x1c
>>>>
>>>>           > ...
>>>>
>>>>           > Each PWM/Tach instance has its own controller register and is not
>>>> dependent on others.
>>>
>>> Your email reply quoting style is very difficult to read.
>>>
>>>>
>>>>
>>>>
>>>> Hi Guenter,
>>>>
>>>>
>>>>
>>>> Did you receive a response to my previous email?
>>>>
>>>> I would like to inquire if you have any further concerns regarding the PWM
>>>> and Tach with 16 instances.
>>>
>>> But isn't like this in all PWMs in all SoCs?
>>>
>>
>> Correct, pretty much every fan controller is implemented that way.
>> I don't understand the logic.
>>
> 
> Hi Krzysztof and Guenter,
> 
> Apologies for any confusion earlier.
> So, you think that the implementation with 16 instances of TACH/PWM
> device nodes in dts instead of one is ok to you, right?
> 

Did I say that ? No, it is not ok with me. It is confusing and doesn't make
sense to me. This is one fan controller with 16 channels, not 16 separate
controllers.

Guenter

Re: [v6 2/4] dt-bindings: hwmon: Add ASPEED TACH Control documentation

[蔡承達]

Guenter Roeck <linux@roeck-us.net> 於 2023年7月14日 週五 下午5:59寫道：
>
> On 7/14/23 00:13, Krzysztof Kozlowski wrote:
> > On 14/07/2023 09:04, 蔡承達 wrote:
> >
> >>          > This is because our register layout for PWM and Tach is not
> >> continuous.
> >>
> >>          > PWM0 used 0x0 0x4, Tach0 used 0x8 0xc
> >>
> >>          > PWM1 used 0x10 0x14, Tach1 used 0x18 0x1c
> >>
> >>          > ...
> >>
> >>          > Each PWM/Tach instance has its own controller register and is not
> >> dependent on others.
> >
> > Your email reply quoting style is very difficult to read.
> >
> >>
> >>
> >>
> >> Hi Guenter,
> >>
> >>
> >>
> >> Did you receive a response to my previous email?
> >>
> >> I would like to inquire if you have any further concerns regarding the PWM
> >> and Tach with 16 instances.
> >
> > But isn't like this in all PWMs in all SoCs?
> >
>
> Correct, pretty much every fan controller is implemented that way.
> I don't understand the logic.
>

Hi Krzysztof and Guenter,

Apologies for any confusion earlier.
So, you think that the implementation with 16 instances of TACH/PWM
device nodes in dts instead of one is ok to you, right?

Thanks

Best regards,
Billy Tsai

Re: [v6 2/4] dt-bindings: hwmon: Add ASPEED TACH Control documentation

[Guenter Roeck]

On 7/14/23 00:13, Krzysztof Kozlowski wrote:
> On 14/07/2023 09:04, 蔡承達 wrote:
> 
>>          > This is because our register layout for PWM and Tach is not
>> continuous.
>>
>>          > PWM0 used 0x0 0x4, Tach0 used 0x8 0xc
>>
>>          > PWM1 used 0x10 0x14, Tach1 used 0x18 0x1c
>>
>>          > ...
>>
>>          > Each PWM/Tach instance has its own controller register and is not
>> dependent on others.
> 
> Your email reply quoting style is very difficult to read.
> 
>>
>>
>>
>> Hi Guenter,
>>
>>
>>
>> Did you receive a response to my previous email?
>>
>> I would like to inquire if you have any further concerns regarding the PWM
>> and Tach with 16 instances.
> 
> But isn't like this in all PWMs in all SoCs?
> 

Correct, pretty much every fan controller is implemented that way.
I don't understand the logic.

Guenter

Re: [v6 2/4] dt-bindings: hwmon: Add ASPEED TACH Control documentation

[Krzysztof Kozlowski]

On 14/07/2023 09:04, 蔡承達 wrote:

>         > This is because our register layout for PWM and Tach is not
> continuous.
> 
>         > PWM0 used 0x0 0x4, Tach0 used 0x8 0xc
> 
>         > PWM1 used 0x10 0x14, Tach1 used 0x18 0x1c
> 
>         > ...
> 
>         > Each PWM/Tach instance has its own controller register and is not
> dependent on others.

Your email reply quoting style is very difficult to read.

> 
> 
> 
> Hi Guenter,
> 
> 
> 
> Did you receive a response to my previous email?
> 
> I would like to inquire if you have any further concerns regarding the PWM
> and Tach with 16 instances.

But isn't like this in all PWMs in all SoCs?

Best regards,
Krzysztof