[PATCH 0/2] Add PWM support to EN7581

[PATCH 0/2] Add PWM support to EN7581

[Lorenzo Bianconi]

Introduce driver for PWM module available on EN7581 SoC.

Benjamin Larsson (1):
  pwm: airoha: Add support for EN7581 SoC

Christian Marangi (1):
  dt-bindings: pwm: Document Airoha EN7581 PWM

 .../bindings/pwm/airoha,en7581-pwm.yaml       |  42 ++
 drivers/pwm/Kconfig                           |  10 +
 drivers/pwm/Makefile                          |   1 +
 drivers/pwm/pwm-airoha.c                      | 403 ++++++++++++++++++
 4 files changed, 456 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/airoha,en7581-pwm.yaml
 create mode 100644 drivers/pwm/pwm-airoha.c

-- 
2.46.0

[PATCH 1/2] dt-bindings: pwm: Document Airoha EN7581 PWM

[Lorenzo Bianconi]

From: Christian Marangi <ansuelsmth@gmail.com>

Document required property for the Airoha EN7581 PWM. The device
requires 3 different address for the sgpio, flash and cycle config.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 .../bindings/pwm/airoha,en7581-pwm.yaml       | 42 +++++++++++++++++++
 1 file changed, 42 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/airoha,en7581-pwm.yaml

diff --git a/Documentation/devicetree/bindings/pwm/airoha,en7581-pwm.yaml b/Documentation/devicetree/bindings/pwm/airoha,en7581-pwm.yaml
new file mode 100644
index 000000000000..52470668f90e
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/airoha,en7581-pwm.yaml
@@ -0,0 +1,42 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pwm/airoha,en7581-pwm.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Airoha EN7581 PWM
+
+maintainers:
+  - Christian Marangi <ansuelsmth@gmail.com>
+
+allOf:
+  - $ref: pwm.yaml#
+
+properties:
+  compatible:
+    const: airoha,en7581-pwm
+
+  reg:
+    items:
+      - description: sgpio config address
+      - description: flash config address
+      - description: cycle config address
+
+  "#pwm-cells":
+    const: 3
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    pwm@1fbf0224 {
+        compatible = "airoha,en7581-pwm";
+        reg = <0x1fbf0224 0x10>,
+              <0x1fbf0238 0x28>,
+              <0x1fbf0298 0x8>;
+        #pwm-cells = <3>;
+    };
-- 
2.46.0

[PATCH 2/2] pwm: airoha: Add support for EN7581 SoC

[Lorenzo Bianconi]

From: Benjamin Larsson <benjamin.larsson@genexis.eu>

Introduce driver for PWM module available on EN7581 SoC.

Signed-off-by: Benjamin Larsson <benjamin.larsson@genexis.eu>
Co-developed-by: Lorenzo Bianconi <lorenzo@kernel.org>
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 drivers/pwm/Kconfig      |  10 +
 drivers/pwm/Makefile     |   1 +
 drivers/pwm/pwm-airoha.c | 403 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 414 insertions(+)
 create mode 100644 drivers/pwm/pwm-airoha.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 1dd7921194f5..0d702b394ea7 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -47,6 +47,16 @@ config PWM_AB8500
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-ab8500.
 
+config PWM_AIROHA
+	tristate "Airoha PWM support"
+	depends on ARCH_AIROHA || COMPILE_TEST
+	depends on OF
+	help
+	  Generic PWM framework driver for Airoha SoC.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-airoha.
+
 config PWM_APPLE
 	tristate "Apple SoC PWM support"
 	depends on ARCH_APPLE || COMPILE_TEST
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 90913519f11a..cef216753dd6 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -1,6 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0
 obj-$(CONFIG_PWM)		+= core.o
 obj-$(CONFIG_PWM_AB8500)	+= pwm-ab8500.o
+obj-$(CONFIG_PWM_AIROHA)	+= pwm-airoha.o
 obj-$(CONFIG_PWM_APPLE)		+= pwm-apple.o
 obj-$(CONFIG_PWM_ATMEL)		+= pwm-atmel.o
 obj-$(CONFIG_PWM_ATMEL_HLCDC_PWM)	+= pwm-atmel-hlcdc.o
diff --git a/drivers/pwm/pwm-airoha.c b/drivers/pwm/pwm-airoha.c
new file mode 100644
index 000000000000..73ae73175e5c
--- /dev/null
+++ b/drivers/pwm/pwm-airoha.c
@@ -0,0 +1,403 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2022 Markus Gothe <markus.gothe@genexis.eu>
+ */
+
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/gpio.h>
+#include <linux/bitops.h>
+#include <asm/div64.h>
+
+#define REG_SGPIO_LED_DATE		0x0000
+#define SGPIO_LED_SHIFT_FLAG		BIT(31)
+#define SGPIO_LED_DATA			GENMASK(16, 0)
+
+#define REG_SGPIO_CLK_DIVR		0x0004
+#define REG_SGPIO_CLK_DLY		0x0008
+
+#define REG_SIPO_FLASH_MODE_CFG		0x000c
+#define SERIAL_GPIO_FLASH_MODE		BIT(1)
+#define SERIAL_GPIO_MODE		BIT(0)
+
+#define REG_GPIO_FLASH_PRD_SET(_n)	(0x0004 + ((_n) << 2))
+#define GPIO_FLASH_PRD_MASK(_n)		GENMASK(15 + ((_n) << 4), ((_n) << 4))
+
+#define REG_GPIO_FLASH_MAP(_n)		(0x0014 + ((_n) << 2))
+#define GPIO_FLASH_SETID_MASK(_n)	GENMASK(2 + ((_n) << 2), ((_n) << 2))
+#define GPIO_FLASH_EN(_n)		BIT(3 + ((_n) << 2))
+
+#define REG_SIPO_FLASH_MAP(_n)		(0x001c + ((_n) << 2))
+
+#define REG_CYCLE_CFG_VALUE(_n)		(0x0000 + ((_n) << 2))
+#define WAVE_GEN_CYCLE_MASK(_n)		GENMASK(7 + ((_n) << 3), ((_n) << 3))
+
+struct airoha_pwm {
+	void __iomem *sgpio_cfg;
+	void __iomem *flash_cfg;
+	void __iomem *cycle_cfg;
+
+	struct device_node *np;
+	u64 initialized;
+
+	struct {
+		/* Bitmask of PWM channels using this bucket */
+		u64 used;
+		/* Relative duty cycle, 0-255 */
+		u32 duty;
+		/* In 1/250 s, 1-250 permitted */
+		u32 period;
+	} bucket[8];
+};
+
+/*
+ * The first 16 GPIO pins, GPIO0-GPIO15, are mapped into 16 PWM channels, 0-15.
+ * The SIPO GPIO pins are 16 pins which are mapped into 17 PWM channels, 16-32.
+ * However, we've only got 8 concurrent waveform generators and can therefore
+ * only use up to 8 different combinations of duty cycle and period at a time.
+ */
+#define PWM_NUM_GPIO	16
+#define PWM_NUM_SIPO	17
+
+/* The PWM hardware supports periods between 4 ms and 1 s */
+#define PERIOD_MIN_NS	4000000
+#define PERIOD_MAX_NS	1000000000
+/* It is represented internally as 1/250 s between 1 and 250 */
+#define PERIOD_MIN	1
+#define PERIOD_MAX	250
+/* Duty cycle is relative with 255 corresponding to 100% */
+#define DUTY_FULL	255
+
+static u32 airoha_pwm_rmw(struct airoha_pwm *pc, void __iomem *addr,
+			  u32 mask, u32 val)
+{
+	val |= (readl(addr) & ~mask);
+	writel(val, addr);
+
+	return val;
+}
+
+#define airoha_pwm_sgpio_rmw(pc, offset, mask, val)				\
+	airoha_pwm_rmw((pc), (pc)->sgpio_cfg + (offset), (mask), (val))
+#define airoha_pwm_flash_rmw(pc, offset, mask, val)				\
+	airoha_pwm_rmw((pc), (pc)->flash_cfg + (offset), (mask), (val))
+#define airoha_pwm_cycle_rmw(pc, offset, mask, val)				\
+	airoha_pwm_rmw((pc), (pc)->cycle_cfg + (offset), (mask), (val))
+
+#define airoha_pwm_sgpio_set(pc, offset, val)					\
+	airoha_pwm_sgpio_rmw((pc), (offset), 0, (val))
+#define airoha_pwm_sgpio_clear(pc, offset, mask)				\
+	airoha_pwm_sgpio_rmw((pc), (offset), (mask), 0)
+#define airoha_pwm_flash_set(pc, offset, val)					\
+	airoha_pwm_flash_rmw((pc), (offset), 0, (val))
+#define airoha_pwm_flash_clear(pc, offset, mask)				\
+	airoha_pwm_flash_rmw((pc), (offset), (mask), 0)
+
+static int airoha_pwm_get_waveform(struct airoha_pwm *pc, u32 duty, u32 period)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(pc->bucket); i++) {
+		if (!pc->bucket[i].used)
+			continue;
+
+		if (duty == pc->bucket[i].duty &&
+		    period == pc->bucket[i].period)
+			return i;
+
+		/* Unlike duty cycle zero, which can be handled by
+		 * disabling PWM, a generator is needed for full duty
+		 * cycle but it can be reused regardless of period
+		 */
+		if (duty == DUTY_FULL && pc->bucket[i].duty == DUTY_FULL)
+			return i;
+	}
+
+	return -1;
+}
+
+static void airoha_pwm_free_waveform(struct airoha_pwm *pc, unsigned int hwpwm)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(pc->bucket); i++)
+		pc->bucket[i].used &= ~BIT_ULL(hwpwm);
+}
+
+static int airoha_pwm_consume_waveform(struct airoha_pwm *pc, u32 duty,
+				       u32 period, unsigned int hwpwm)
+{
+	int id = airoha_pwm_get_waveform(pc, duty, period);
+
+	if (id < 0) {
+		int i;
+
+		/* find an unused waveform generator */
+		for (i = 0; i < ARRAY_SIZE(pc->bucket); i++) {
+			if (!(pc->bucket[i].used & ~BIT_ULL(hwpwm))) {
+				id = i;
+				break;
+			}
+		}
+	}
+
+	if (id > 0) {
+		airoha_pwm_free_waveform(pc, hwpwm);
+		pc->bucket[id].used |= BIT_ULL(hwpwm);
+		pc->bucket[id].period = period;
+		pc->bucket[id].duty = duty;
+	}
+
+	return id;
+}
+
+static int airoha_pwm_sipo_init(struct airoha_pwm *pc)
+{
+	u32 clk_divr_val = 3, sipo_clock_delay = 1;
+	u32 val, sipo_clock_divisor = 32;
+
+	if (!(pc->initialized >> PWM_NUM_GPIO))
+		return 0;
+
+	/* Select the right shift register chip */
+	if (of_property_read_bool(pc->np, "hc74595"))
+		airoha_pwm_sgpio_set(pc, REG_SIPO_FLASH_MODE_CFG,
+				     SERIAL_GPIO_MODE);
+	else
+		airoha_pwm_sgpio_clear(pc, REG_SIPO_FLASH_MODE_CFG,
+				       SERIAL_GPIO_MODE);
+
+	if (!of_property_read_u32(pc->np, "sipo-clock-divisor",
+				  &sipo_clock_divisor)) {
+		switch (sipo_clock_divisor) {
+		case 4:
+			clk_divr_val = 0;
+			break;
+		case 8:
+			clk_divr_val = 1;
+			break;
+		case 16:
+			clk_divr_val = 2;
+			break;
+		case 32:
+			clk_divr_val = 3;
+			break;
+		default:
+			return -EINVAL;
+		}
+	}
+	/* Configure shift register timings */
+	writel(clk_divr_val, pc->sgpio_cfg + REG_SGPIO_CLK_DIVR);
+
+	of_property_read_u32(pc->np, "sipo-clock-delay", &sipo_clock_delay);
+	if (sipo_clock_delay < 1 || sipo_clock_delay > sipo_clock_divisor / 2)
+		return -EINVAL;
+
+	/* The actual delay is sclkdly + 1 so subtract 1 from
+	 * sipo-clock-delay to calculate the register value
+	 */
+	sipo_clock_delay--;
+	writel(sipo_clock_delay, pc->sgpio_cfg + REG_SGPIO_CLK_DLY);
+
+	/* It it necessary to after muxing explicitly shift out all
+	 * zeroes to initialize the shift register before enabling PWM
+	 * mode because in PWM mode SIPO will not start shifting until
+	 * it needs to output a non-zero value (bit 31 of led_data
+	 * indicates shifting in progress and it must return to zero
+	 * before led_data can be written or PWM mode can be set)
+	 */
+	if (readl_poll_timeout(pc->sgpio_cfg + REG_SGPIO_LED_DATE, val,
+			       !(val & SGPIO_LED_SHIFT_FLAG), 10,
+			       200 * USEC_PER_MSEC))
+		return -ETIMEDOUT;
+
+	airoha_pwm_sgpio_clear(pc, REG_SGPIO_LED_DATE, SGPIO_LED_DATA);
+	if (readl_poll_timeout(pc->sgpio_cfg + REG_SGPIO_LED_DATE, val,
+			       !(val & SGPIO_LED_SHIFT_FLAG), 10,
+			       200 * USEC_PER_MSEC))
+		return -ETIMEDOUT;
+
+	/* Set SIPO in PWM mode */
+	airoha_pwm_sgpio_set(pc, REG_SIPO_FLASH_MODE_CFG,
+			     SERIAL_GPIO_FLASH_MODE);
+
+	return 0;
+}
+
+static void airoha_pwm_config_waveform(struct airoha_pwm *pc, int index,
+				       u32 duty, u32 period)
+{
+	u32 mask, val;
+
+	/* Configure frequency divisor */
+	mask = WAVE_GEN_CYCLE_MASK(index % 4);
+	val = (period << __bf_shf(mask)) & mask;
+	airoha_pwm_cycle_rmw(pc, REG_CYCLE_CFG_VALUE(index / 4), mask, val);
+
+	/* Configure duty cycle */
+	duty = ((DUTY_FULL - duty) << 8) | duty;
+	mask = GPIO_FLASH_PRD_MASK(index % 2);
+	val = (duty << __bf_shf(mask)) & mask;
+	airoha_pwm_flash_rmw(pc, REG_GPIO_FLASH_PRD_SET(index / 2), mask, val);
+}
+
+static void airoha_pwm_config_flash_map(struct airoha_pwm *pc,
+					unsigned int hwpwm, int index)
+{
+	u32 addr, mask, val;
+
+	if (hwpwm < PWM_NUM_GPIO) {
+		addr = REG_GPIO_FLASH_MAP(hwpwm / 8);
+	} else {
+		addr = REG_SIPO_FLASH_MAP(hwpwm / 8);
+		hwpwm -= PWM_NUM_GPIO;
+	}
+
+	if (index < 0) {
+		/* Change of waveform takes effect immediately but
+		 * disabling has some delay so to prevent glitching
+		 * only the enable bit is touched when disabling
+		 */
+		airoha_pwm_flash_clear(pc, addr, GPIO_FLASH_EN(hwpwm % 8));
+		return;
+	}
+
+	mask = GPIO_FLASH_SETID_MASK(hwpwm % 8);
+	val = ((index & 7) << __bf_shf(mask)) & mask;
+	airoha_pwm_flash_rmw(pc, addr, mask, val);
+	airoha_pwm_flash_set(pc, addr, GPIO_FLASH_EN(hwpwm % 8));
+}
+
+static int airoha_pwm_config(struct airoha_pwm *pc, struct pwm_device *pwm,
+			     u64 duty_ns, u64 period_ns,
+			     enum pwm_polarity polarity)
+{
+	u32 duty, period;
+	int index = -1;
+
+	duty = clamp_val(div64_u64(DUTY_FULL * duty_ns, period_ns), 0,
+			 DUTY_FULL);
+	if (polarity == PWM_POLARITY_INVERSED)
+		duty = DUTY_FULL - duty;
+
+	period = clamp_val(div64_u64(25 * period_ns, 100000000), PERIOD_MIN,
+			   PERIOD_MAX);
+	if (duty) {
+		index = airoha_pwm_consume_waveform(pc, duty, period,
+						    pwm->hwpwm);
+		if (index < 0)
+			return -EBUSY;
+	}
+
+	if (!(pc->initialized & BIT_ULL(pwm->hwpwm)) &&
+	    pwm->hwpwm >= PWM_NUM_GPIO)
+		airoha_pwm_sipo_init(pc);
+
+	if (index >= 0) {
+		airoha_pwm_config_waveform(pc, index, duty, period);
+		airoha_pwm_config_flash_map(pc, pwm->hwpwm, index);
+	} else {
+		airoha_pwm_config_flash_map(pc, pwm->hwpwm, index);
+		airoha_pwm_free_waveform(pc, pwm->hwpwm);
+	}
+
+	pc->initialized |= BIT_ULL(pwm->hwpwm);
+
+	return 0;
+}
+
+static void airoha_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct airoha_pwm *pc = pwmchip_get_drvdata(chip);
+
+	/* Disable PWM and release the waveform */
+	airoha_pwm_config_flash_map(pc, pwm->hwpwm, -1);
+	airoha_pwm_free_waveform(pc, pwm->hwpwm);
+
+	pc->initialized &= ~BIT_ULL(pwm->hwpwm);
+	if (!(pc->initialized >> PWM_NUM_GPIO))
+		airoha_pwm_sgpio_clear(pc, REG_SIPO_FLASH_MODE_CFG,
+				       SERIAL_GPIO_FLASH_MODE);
+
+	/* Clear the state to force re-initialization the next time
+	 * this PWM channel is used since we cannot retain state in
+	 * hardware due to limited number of waveform generators
+	 */
+	memset(&pwm->state, 0, sizeof(pwm->state));
+}
+
+static int airoha_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+			    const struct pwm_state *state)
+{
+	struct airoha_pwm *pc = pwmchip_get_drvdata(chip);
+	u64 duty = state->enabled ? state->duty_cycle : 0;
+
+	if (state->period < PERIOD_MIN_NS || state->period > PERIOD_MAX_NS)
+		return -EINVAL;
+
+	return airoha_pwm_config(pc, pwm, duty, state->period,
+				 state->polarity);
+}
+
+static const struct pwm_ops airoha_pwm_ops = {
+	.apply = airoha_pwm_apply,
+	.free = airoha_pwm_free,
+};
+
+static int airoha_pwm_probe(struct platform_device *pdev)
+{
+	struct airoha_pwm *pc;
+	struct pwm_chip *chip;
+
+	chip = devm_pwmchip_alloc(&pdev->dev, PWM_NUM_GPIO + PWM_NUM_SIPO,
+				  sizeof(*pc));
+	if (IS_ERR(chip))
+		return PTR_ERR(chip);
+
+	pc = pwmchip_get_drvdata(chip);
+	pc->np = pdev->dev.of_node;
+
+	pc->sgpio_cfg = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(pc->sgpio_cfg))
+		return PTR_ERR(pc->sgpio_cfg);
+
+	pc->flash_cfg = devm_platform_ioremap_resource(pdev, 1);
+	if (IS_ERR(pc->flash_cfg))
+		return PTR_ERR(pc->flash_cfg);
+
+	pc->cycle_cfg = devm_platform_ioremap_resource(pdev, 2);
+	if (IS_ERR(pc->cycle_cfg))
+		return PTR_ERR(pc->cycle_cfg);
+
+	chip->ops = &airoha_pwm_ops;
+	chip->of_xlate = of_pwm_xlate_with_flags;
+
+	return devm_pwmchip_add(&pdev->dev, chip);
+}
+
+static const struct of_device_id airoha_pwm_of_match[] = {
+	{ .compatible = "airoha,en7581-pwm" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, airoha_pwm_of_match);
+
+static struct platform_driver airoha_pwm_driver = {
+	.driver = {
+		.name = "airoha-pwm",
+		.of_match_table = airoha_pwm_of_match,
+	},
+	.probe = airoha_pwm_probe,
+};
+module_platform_driver(airoha_pwm_driver);
+
+MODULE_ALIAS("platform:airoha-pwm");
+MODULE_AUTHOR("Markus Gothe <markus.gothe@genexis.eu>");
+MODULE_AUTHOR("Lorenzo Bianconi <lorenzo@kernel.org>");
+MODULE_AUTHOR("Benjamin Larsson <benjamin.larsson@genexis.eu>");
+MODULE_DESCRIPTION("Airoha EN7581 PWM driver");
+MODULE_LICENSE("GPL");
-- 
2.46.0

Re: [PATCH 2/2] pwm: airoha: Add support for EN7581 SoC

[Krzysztof Kozlowski]

On 10/08/2024 06:48, Lorenzo Bianconi wrote:
> From: Benjamin Larsson <benjamin.larsson@genexis.eu>
> 
> Introduce driver for PWM module available on EN7581 SoC.
> 
> Signed-off-by: Benjamin Larsson <benjamin.larsson@genexis.eu>
> Co-developed-by: Lorenzo Bianconi <lorenzo@kernel.org>
> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> ---


...

> +
> +static void airoha_pwm_config_flash_map(struct airoha_pwm *pc,
> +					unsigned int hwpwm, int index)
> +{
> +	u32 addr, mask, val;
> +
> +	if (hwpwm < PWM_NUM_GPIO) {
> +		addr = REG_GPIO_FLASH_MAP(hwpwm / 8);
> +	} else {
> +		addr = REG_SIPO_FLASH_MAP(hwpwm / 8);
> +		hwpwm -= PWM_NUM_GPIO;
> +	}
> +
> +	if (index < 0) {
> +		/* Change of waveform takes effect immediately but

This and other comments should be not netdev-style but general Linux style.

> +		 * disabling has some delay so to prevent glitching
> +		 * only the enable bit is touched when disabling
> +		 */
> +		airoha_pwm_flash_clear(pc, addr, GPIO_FLASH_EN(hwpwm % 8));
> +		return;

...


> +
> +static const struct of_device_id airoha_pwm_of_match[] = {
> +	{ .compatible = "airoha,en7581-pwm" },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, airoha_pwm_of_match);
> +
> +static struct platform_driver airoha_pwm_driver = {
> +	.driver = {
> +		.name = "airoha-pwm",
> +		.of_match_table = airoha_pwm_of_match,
> +	},
> +	.probe = airoha_pwm_probe,
> +};
> +module_platform_driver(airoha_pwm_driver);
> +
> +MODULE_ALIAS("platform:airoha-pwm");

You should not need MODULE_ALIAS() in normal cases. If you need it,
usually it means your device ID table is wrong (e.g. misses either
entries or MODULE_DEVICE_TABLE()). MODULE_ALIAS() is not a substitute
for incomplete ID table.

Especially that it does not match compatible, so you cannot use excuse
module autoloading does not work for given OF node...


Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: pwm: Document Airoha EN7581 PWM

[Krzysztof Kozlowski]

On 10/08/2024 06:48, Lorenzo Bianconi wrote:
> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    pwm@1fbf0224 {
> +        compatible = "airoha,en7581-pwm";
> +        reg = <0x1fbf0224 0x10>,
> +              <0x1fbf0238 0x28>,
> +              <0x1fbf0298 0x8>;

These look almost continuous, so I wonder what's in between? E.g.
between 0x1fbf0224+10=0x1fbf0234 and 0x1fbf0238?

Rest looks good.

Best regards,
Krzysztof

Re: [PATCH 2/2] pwm: airoha: Add support for EN7581 SoC

[Lorenzo Bianconi]

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

> On 10/08/2024 06:48, Lorenzo Bianconi wrote:
> > From: Benjamin Larsson <benjamin.larsson@genexis.eu>
> > 
> > Introduce driver for PWM module available on EN7581 SoC.
> > 
> > Signed-off-by: Benjamin Larsson <benjamin.larsson@genexis.eu>
> > Co-developed-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > ---
> 
> 
> ...
> 
> > +
> > +static void airoha_pwm_config_flash_map(struct airoha_pwm *pc,
> > +					unsigned int hwpwm, int index)
> > +{
> > +	u32 addr, mask, val;
> > +
> > +	if (hwpwm < PWM_NUM_GPIO) {
> > +		addr = REG_GPIO_FLASH_MAP(hwpwm / 8);
> > +	} else {
> > +		addr = REG_SIPO_FLASH_MAP(hwpwm / 8);
> > +		hwpwm -= PWM_NUM_GPIO;
> > +	}
> > +
> > +	if (index < 0) {
> > +		/* Change of waveform takes effect immediately but
> 
> This and other comments should be not netdev-style but general Linux style.

ack, I will fix them in v2.

> 
> > +		 * disabling has some delay so to prevent glitching
> > +		 * only the enable bit is touched when disabling
> > +		 */
> > +		airoha_pwm_flash_clear(pc, addr, GPIO_FLASH_EN(hwpwm % 8));
> > +		return;
> 
> ...
> 
> 
> > +
> > +static const struct of_device_id airoha_pwm_of_match[] = {
> > +	{ .compatible = "airoha,en7581-pwm" },
> > +	{ /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, airoha_pwm_of_match);
> > +
> > +static struct platform_driver airoha_pwm_driver = {
> > +	.driver = {
> > +		.name = "airoha-pwm",
> > +		.of_match_table = airoha_pwm_of_match,
> > +	},
> > +	.probe = airoha_pwm_probe,
> > +};
> > +module_platform_driver(airoha_pwm_driver);
> > +
> > +MODULE_ALIAS("platform:airoha-pwm");
> 
> You should not need MODULE_ALIAS() in normal cases. If you need it,
> usually it means your device ID table is wrong (e.g. misses either
> entries or MODULE_DEVICE_TABLE()). MODULE_ALIAS() is not a substitute
> for incomplete ID table.
> 
> Especially that it does not match compatible, so you cannot use excuse
> module autoloading does not work for given OF node...

ack, I will fix it in v2.

Regards,
Lorenzo

> 
> 
> Best regards,
> Krzysztof
> 

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

Re: [PATCH 1/2] dt-bindings: pwm: Document Airoha EN7581 PWM

[Lorenzo Bianconi]

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

On Aug 10, Krzysztof Kozlowski wrote:
> On 10/08/2024 06:48, Lorenzo Bianconi wrote:
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    pwm@1fbf0224 {
> > +        compatible = "airoha,en7581-pwm";
> > +        reg = <0x1fbf0224 0x10>,
> > +              <0x1fbf0238 0x28>,
> > +              <0x1fbf0298 0x8>;
> 
> These look almost continuous, so I wonder what's in between? E.g.
> between 0x1fbf0224+10=0x1fbf0234 and 0x1fbf0238?

register 0x1fbf0234 will be used by pinctrl driver (I will post it soon) for
pwm muxing. The issue here is clock, pinctrl, gpio, pwm and (future) serdes
registers are all interleaved in a non-regular fashion in the following
IO space:

- <0x1fa20000 - 0x1fa20384> 
- <0x1fb00000 - 0x1fb0096c>
- <0x1fbf0200 - 0x1fbf02bc>

So in order to avoid conflicts we need a sparse mapping.

Regards,
Lorenzo

> 
> Rest looks good.
> 
> Best regards,
> Krzysztof
> 

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

Re: [PATCH 2/2] pwm: airoha: Add support for EN7581 SoC

[kernel test robot]

Hi Lorenzo,

kernel test robot noticed the following build errors:

[auto build test ERROR on robh/for-next]
[also build test ERROR on linus/master v6.11-rc2 next-20240809]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Lorenzo-Bianconi/dt-bindings-pwm-Document-Airoha-EN7581-PWM/20240810-143716
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link:    https://lore.kernel.org/r/a03f5ea9291e39eab303696eb03fdd44cf04e8d9.1723264979.git.lorenzo%40kernel.org
patch subject: [PATCH 2/2] pwm: airoha: Add support for EN7581 SoC
config: alpha-allyesconfig (https://download.01.org/0day-ci/archive/20240811/202408110329.6YGwCzoM-lkp@intel.com/config)
compiler: alpha-linux-gcc (GCC) 13.3.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240811/202408110329.6YGwCzoM-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202408110329.6YGwCzoM-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/pwm/pwm-airoha.c: In function 'airoha_pwm_config_waveform':
>> drivers/pwm/pwm-airoha.c:239:26: error: implicit declaration of function '__bf_shf' [-Werror=implicit-function-declaration]
     239 |         val = (period << __bf_shf(mask)) & mask;
         |                          ^~~~~~~~
   cc1: some warnings being treated as errors


vim +/__bf_shf +239 drivers/pwm/pwm-airoha.c

   231	
   232	static void airoha_pwm_config_waveform(struct airoha_pwm *pc, int index,
   233					       u32 duty, u32 period)
   234	{
   235		u32 mask, val;
   236	
   237		/* Configure frequency divisor */
   238		mask = WAVE_GEN_CYCLE_MASK(index % 4);
 > 239		val = (period << __bf_shf(mask)) & mask;
   240		airoha_pwm_cycle_rmw(pc, REG_CYCLE_CFG_VALUE(index / 4), mask, val);
   241	
   242		/* Configure duty cycle */
   243		duty = ((DUTY_FULL - duty) << 8) | duty;
   244		mask = GPIO_FLASH_PRD_MASK(index % 2);
   245		val = (duty << __bf_shf(mask)) & mask;
   246		airoha_pwm_flash_rmw(pc, REG_GPIO_FLASH_PRD_SET(index / 2), mask, val);
   247	}
   248	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 2/2] pwm: airoha: Add support for EN7581 SoC

[kernel test robot]

Hi Lorenzo,

kernel test robot noticed the following build errors:

[auto build test ERROR on robh/for-next]
[also build test ERROR on linus/master thierry-reding-pwm/for-next v6.11-rc2 next-20240809]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Lorenzo-Bianconi/dt-bindings-pwm-Document-Airoha-EN7581-PWM/20240810-143716
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link:    https://lore.kernel.org/r/a03f5ea9291e39eab303696eb03fdd44cf04e8d9.1723264979.git.lorenzo%40kernel.org
patch subject: [PATCH 2/2] pwm: airoha: Add support for EN7581 SoC
config: hexagon-allmodconfig (https://download.01.org/0day-ci/archive/20240811/202408110920.KEW33sRE-lkp@intel.com/config)
compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project f86594788ce93b696675c94f54016d27a6c21d18)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240811/202408110920.KEW33sRE-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202408110920.KEW33sRE-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from drivers/pwm/pwm-airoha.c:7:
   In file included from include/linux/io.h:14:
   In file included from arch/hexagon/include/asm/io.h:328:
   include/asm-generic/io.h:548:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     548 |         val = __raw_readb(PCI_IOBASE + addr);
         |                           ~~~~~~~~~~ ^
   include/asm-generic/io.h:561:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     561 |         val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
         |                                                         ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
      37 | #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
         |                                                   ^
   In file included from drivers/pwm/pwm-airoha.c:7:
   In file included from include/linux/io.h:14:
   In file included from arch/hexagon/include/asm/io.h:328:
   include/asm-generic/io.h:574:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     574 |         val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
         |                                                         ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
      35 | #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
         |                                                   ^
   In file included from drivers/pwm/pwm-airoha.c:7:
   In file included from include/linux/io.h:14:
   In file included from arch/hexagon/include/asm/io.h:328:
   include/asm-generic/io.h:585:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     585 |         __raw_writeb(value, PCI_IOBASE + addr);
         |                             ~~~~~~~~~~ ^
   include/asm-generic/io.h:595:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     595 |         __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
         |                                                       ~~~~~~~~~~ ^
   include/asm-generic/io.h:605:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     605 |         __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
         |                                                       ~~~~~~~~~~ ^
>> drivers/pwm/pwm-airoha.c:239:19: error: call to undeclared function '__bf_shf'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
     239 |         val = (period << __bf_shf(mask)) & mask;
         |                          ^
   drivers/pwm/pwm-airoha.c:271:24: error: call to undeclared function '__bf_shf'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
     271 |         val = ((index & 7) << __bf_shf(mask)) & mask;
         |                               ^
   6 warnings and 2 errors generated.


vim +/__bf_shf +239 drivers/pwm/pwm-airoha.c

   231	
   232	static void airoha_pwm_config_waveform(struct airoha_pwm *pc, int index,
   233					       u32 duty, u32 period)
   234	{
   235		u32 mask, val;
   236	
   237		/* Configure frequency divisor */
   238		mask = WAVE_GEN_CYCLE_MASK(index % 4);
 > 239		val = (period << __bf_shf(mask)) & mask;
   240		airoha_pwm_cycle_rmw(pc, REG_CYCLE_CFG_VALUE(index / 4), mask, val);
   241	
   242		/* Configure duty cycle */
   243		duty = ((DUTY_FULL - duty) << 8) | duty;
   244		mask = GPIO_FLASH_PRD_MASK(index % 2);
   245		val = (duty << __bf_shf(mask)) & mask;
   246		airoha_pwm_flash_rmw(pc, REG_GPIO_FLASH_PRD_SET(index / 2), mask, val);
   247	}
   248	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 1/2] dt-bindings: pwm: Document Airoha EN7581 PWM

[Krzysztof Kozlowski]

On 10/08/2024 06:48, Lorenzo Bianconi wrote:
> From: Christian Marangi <ansuelsmth@gmail.com>
> 
> Document required property for the Airoha EN7581 PWM. The device
> requires 3 different address for the sgpio, flash and cycle config.
> 
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof