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

Benjamin Larsson <benjamin.larsson@xxxxxxxxxx> · Tue, 3 Sep 2024 13:58:30 +0200

Hi.

On 2024-09-03 12:46, Uwe Kleine-König wrote:
Hello,

On Sat, Aug 31, 2024 at 04:27:50PM +0200, Lorenzo Bianconi wrote:
From: Benjamin Larsson <benjamin.larsson@xxxxxxxxxx>

Introduce driver for PWM module available on EN7581 SoC.

Signed-off-by: Benjamin Larsson <benjamin.larsson@xxxxxxxxxx>
Co-developed-by: Lorenzo Bianconi <lorenzo@xxxxxxxxxx>
Signed-off-by: Lorenzo Bianconi <lorenzo@xxxxxxxxxx>
---
  drivers/pwm/Kconfig      |  10 ++
  drivers/pwm/Makefile     |   1 +
  drivers/pwm/pwm-airoha.c | 435 +++++++++++++++++++++++++++++++++++++++++++++++
  3 files changed, 446 insertions(+)

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 3e53838990f5..0a78bda0707d 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 0be4f3e6dd43..7ee61822d88d 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..54dc12d20da4
--- /dev/null
+++ b/drivers/pwm/pwm-airoha.c
@@ -0,0 +1,435 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2022 Markus Gothe <markus.gothe@xxxxxxxxxx>
+ */
Would you please add a "Limitations" paragraph here covering the
following questions:

  - How does the hardware behave on changes of configuration (does it
    complete the currently running period? Are there any glitches?)
  - How does the hardware behave on disabling?

Please stick to the format used in several other drivers such that

	sed -rn '/Limitations:/,/\*\/?$/p' drivers/pwm/*.c

emits the informations.


The answer to your questions are currently unknown. Is this information 
needed for a merge of the driver ?



+
+#include <linux/bitfield.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/mfd/airoha-en7581-mfd.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_CFG			0x0024
+#define REG_FLASH_CFG			0x0038
+#define REG_CYCLE_CFG			0x0098
+
+#define REG_SGPIO_LED_DATE		0x0000
+#define SGPIO_LED_SHIFT_FLAG		BIT(31)
+#define SGPIO_LED_DATA			GENMASK(16, 0)
Please make the bit fields's names start with the register name.

I noticed REG_SGPIO_LED_DATE

DATE should be DATA.


+#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;
+		u64 period_ns;
+		u64 duty_ns;
+		enum pwm_polarity polarity;
+	} 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.
How can 16 pins be mapped to 17 PWM channels?


The text is incorrect. There can be 17 pins in 17 slots.



+ * 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.
That's an information to add to the Limitations paragraph.

+ */
+#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;
+}
pc is unused here, please drop it.

+#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))
That does the right thing, but I'd consider

	#define airoha_pwm_sgpio_set(pc, offset, val)					\
		airoha_pwm_sgpio_rmw((pc), (offset), (val), (val))

to be more understandable (or ~0 instead of the last (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, u64 duty_ns,
+				   u64 period_ns)
Given that "waveform" will gain some specific semantic soon, but this
usage is different, I'd like to see this function renamed.

I suggest pwm_generator. Is that acceptable ?



+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(pc->bucket); i++) {
+		if (!pc->bucket[i].used)
+			continue;
+
+		if (duty_ns == pc->bucket[i].duty_ns &&
+		    period_ns == pc->bucket[i].period_ns)
+			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_ns == DUTY_FULL && pc->bucket[i].duty_ns == 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,
+				       u64 duty_ns, u64 period_ns,
+				       enum pwm_polarity polarity,
+				       unsigned int hwpwm)
+{
+	int id = airoha_pwm_get_waveform(pc, duty_ns, period_ns);
+
+	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_ns = period_ns;
+		pc->bucket[id].duty_ns = duty_ns;
+		pc->bucket[id].polarity = polarity;
+	}
One downside of the (nearly) maximal flexibility implemented here is
that if you have 9 (or more) requested pwm devices configuration is
quite limited.  So it might happen that a consumer changes the
configuration for pwm#2 from pwm_state A to pwm_state B but cannot
change back to A later.


Correct.



If you limit the number of requested pwm devices to 8, the code gets a
tad simpler (because you can map a fixed bucket to each pwm device and
don't need to search during .apply()) and each consumer has maximal
freedom to configure its device.


The main use for this solution is for led-dimming which uses the same 
timing among groups of leds. Most (of our) products have more then 8 
leds in total, with a limit of only 8 pwm devices it would not be 
possible to use the mainline driver with our hardware. I suggest that 
the current logic is kept but properly documented in the limitations 
section.



+
+	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,
+				       u64 duty_ns, u64 period_ns,
+				       enum pwm_polarity polarity)
+{
+	u32 period, duty, mask, val;
+
+	duty = clamp_val(div64_u64(DUTY_FULL * duty_ns, period_ns), 0,
+			 DUTY_FULL);
DUTY_FULL * duty_ns might overflow. Also the calculation is wrong.
For example if the following is requested:

	.period = 23999999,
	.duty_cycle = 8000000,

you're supposed to configure period = 16000000 ns and duty_cycle =
8000000 ns.

(I.e.: Pick the biggest possible period not bigger than the requested
period. For that pick the biggest possible duty_cycle not bigger than
the requested duty_cycle.)

If you implement .get_state() in a way to return the actually configured
state, enabling PWM_DEBUG and intensive testing helps to get this right.

+	if (polarity == PWM_POLARITY_INVERSED)
+		duty = DUTY_FULL - duty;
This alone doesn't switch the polarity of the signal. Please only claim
to be able to implement the settings that the hardware actually can do.

I am not sure I agree, but I will investigate this further.


MvH

Benjamin Larsson








