[PATCH v3 5/5] pwm: airoha: Add support for EN7581 SoC
Benjamin Larsson
benjamin.larsson at genexis.eu
Tue Sep 3 04:58:30 PDT 2024
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 at genexis.eu>
>>
>> Introduce driver for PWM module available on EN7581 SoC.
>>
>> Signed-off-by: Benjamin Larsson <benjamin.larsson at genexis.eu>
>> Co-developed-by: Lorenzo Bianconi <lorenzo at kernel.org>
>> Signed-off-by: Lorenzo Bianconi <lorenzo at kernel.org>
>> ---
>> 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 at genexis.eu>
>> + */
> 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
More information about the Linux-mediatek
mailing list