Re: [PATCH v4 2/4] pwm: sun50i: Add H616 PWM support

[Richard GENOUD <richard.genoud@bootlin.com>]

Hi Paul,
Le 09/04/2026 à 19:30, Paul Kocialkowski a écrit :
> Hi Richard,
> 
> On Thu 05 Mar 26, 10:19, Richard Genoud wrote:
>> +/* PWM IRQ Enable Register */
>> +#define H616_PWM_IER				0x0
> 
> I think it would make more sense to keep the full register names from the
> manual after the suffix and stick to them. It makes things easier when
> comparing the code with documentation or the reference implementation.
> 
> So something like SUN8I_PWM_PIER here.
Ok, that make sense.

> 
>> +
>> +/* PWM IRQ Status Register */
>> +#define H616_PWM_ISR				0x4
>> +
>> +/* PWM Capture IRQ Enable Register */
>> +#define H616_PWM_CIER				0x10
>> +
>> +/* PWM Capture IRQ Status Register */
>> +#define H616_PWM_CISR				0x14
>> +
>> +/* PWMCC Pairs Clock Configuration Registers */
>> +#define H616_PWM_XY_CLK_CR(pair)		(0x20 + ((pair) * 0x4))
>> +#define H616_PWM_XY_CLK_CR_SRC_SHIFT		7
>> +#define H616_PWM_XY_CLK_CR_SRC_MASK		1
>> +#define H616_PWM_XY_CLK_CR_GATE_BIT		4
>> +#define H616_PWM_XY_CLK_CR_BYPASS_BIT(chan)	((chan) % 2 + 5)
>> +#define H616_PWM_XY_CLK_CR_DIV_M_SHIFT		0
>> +
>> +/* PWMCC Pairs Dead Zone Control Registers */
>> +#define H616_PWM_XY_DZ(pair)			(0x30 + ((pair) * 0x4))
>> +
>> +/* PWM Enable Register */
>> +#define H616_PWM_ENR				0x40
>> +#define H616_PWM_ENABLE(x)			BIT(x)
>> +
>> +/* PWM Capture Enable Register */
>> +#define H616_PWM_CER				0x44
>> +
>> +/* PWM Control Register */
>> +#define H616_PWM_CTRL_REG(chan)		(0x60 + (chan) * 0x20)
> 
> You're sometimes calling the register offset _REG and sometimes not.
> Both options are fine but you need to keep it consistent across the whole
> definitions. I would be enclined to not use it after using the register names
> coming from the manual as suggested above.
I see what you mean, so H616_PWM_CTRL_REG() would become SUN8I_PWM_PCR()

> 
> Also you're sometimes using "chan", sometimes "ch" for the argument to the
> register macros. This is inconsistent and you might as well just use "c"
> everywhere so it doesn't take too much space.
Indeed.

> 
>> +#define H616_PWM_CTRL_PRESCAL_K_SHIFT	0
>> +#define H616_PWM_CTRL_PRESCAL_K_WIDTH	8
>> +#define H616_PWM_CTRL_ACTIVE_STATE	BIT(8)
>> +
>> +/* PWM Period Register */
>> +#define H616_PWM_PERIOD_REG(ch)		(0x64 + (ch) * 0x20)
>> +#define H616_PWM_PERIOD_MASK		GENMASK(31, 16)
>> +#define H616_PWM_DUTY_MASK		GENMASK(15, 0)
>> +#define H616_PWM_REG_PERIOD(reg)	(FIELD_GET(H616_PWM_PERIOD_MASK, reg) + 1)
>> +#define H616_PWM_REG_DUTY(reg)		FIELD_GET(H616_PWM_DUTY_MASK, reg)
>> +#define H616_PWM_PERIOD(prd)		FIELD_PREP(H616_PWM_PERIOD_MASK, (prd) - 1)
>> +#define H616_PWM_DUTY(dty)		FIELD_PREP(H616_PWM_DUTY_MASK, dty)
>> +#define H616_PWM_PERIOD_MAX		(FIELD_MAX(H616_PWM_PERIOD_MASK) + 1)
> 
> Using REG as a prefix feels a bit confusing here. I would rather see:
> #define SUN8I_PWM_PPR(c)		(0x64 + (c) * 0x20)
> #define SUN8I_PWM_PPR_PERIOD(p)		FIELD_PREP(...)
> #define SUN8I_PWM_PPR_PERIOD_VALUE(r)	FIELD_GET(...)
> #define SUN8I_PWN_PPR_PERIOD_MAX	FIELD_MAX(...)
> #define SUN8I_PWM_PPR_DUTY(d)		FIELD_PREP(...)
> #define SUN8I_PWM_PPR_DUTY_VALUE(r)	FIELD_GET(...)
That's right, that would be less confusing.

> 
>> +
>> +/* PWM Count Register */
>> +#define H616_PWM_CNT_REG(x)		(0x68 + (x) * 0x20)
>> +
>> +/* PWM Capture Control Register */
>> +#define H616_PWM_CCR(x)			(0x6c + (x) * 0x20)
>> +
>> +/* PWM Capture Rise Lock Register */
>> +#define H616_PWM_CRLR(x)		(0x70 + (x) * 0x20)
>> +
>> +/* PWM Capture Fall Lock Register */
>> +#define H616_PWM_CFLR(x)		(0x74 + (x) * 0x20)
>> +
>> +#define H616_PWM_PAIR_IDX(chan)		((chan) >> 2)
>> +
>> +/*
>> + * Block diagram of the PWM clock controller:
>> + *
>> + *             _____      ______      ________
>> + * OSC24M --->|     |    |      |    |        |
>> + * APB1 ----->| Mux |--->| Gate |--->| /div_m |-----> H616_PWM_clock_src_xy
>> + *            |_____|    |______|    |________|
>> + *                               ________
>> + *                              |        |
>> + *                           +->| /div_k |---> H616_PWM_clock_x
>> + *                           |  |________|
>> + *                           |    ______
>> + *                           |   |      |
>> + *                           +-->| Gate |----> H616_PWM_bypass_clock_x
>> + *                           |   |______|
>> + * H616_PWM_clock_src_xy ----+   ________
>> + *                           |  |        |
>> + *                           +->| /div_k |---> H616_PWM_clock_y
>> + *                           |  |________|
>> + *                           |    ______
>> + *                           |   |      |
>> + *                           +-->| Gate |----> H616_PWM_bypass_clock_y
>> + *                               |______|
>> + *
>> + * NB: when the bypass is set, all the PWM logic is bypassed.
>> + * So, the duty cycle and polarity can't be modified (we just have a clock).
>> + * The bypass in PWM mode is used to achieve a 1/2 relative duty cycle with the
>> + * fastest clock.
>> + *
>> + * H616_PWM_clock_x/y serve for the PWM purpose.
>> + * H616_PWM_bypass_clock_x/y serve for the clock-provider purpose.
>> + *
>> + */
>> +
>> +/*
>> + * Table used for /div_m (diviser before obtaining H616_PWM_clock_src_xy)
>> + * It's actually CLK_DIVIDER_POWER_OF_TWO, but limited to /256
>> + */
>> +#define CLK_TABLE_DIV_M_ENTRY(i) { \
>> +	.val = (i), .div = 1 << (i) \
>> +}
>> +
>> +static const struct clk_div_table clk_table_div_m[] = {
>> +	CLK_TABLE_DIV_M_ENTRY(0),
>> +	CLK_TABLE_DIV_M_ENTRY(1),
>> +	CLK_TABLE_DIV_M_ENTRY(2),
>> +	CLK_TABLE_DIV_M_ENTRY(3),
>> +	CLK_TABLE_DIV_M_ENTRY(4),
>> +	CLK_TABLE_DIV_M_ENTRY(5),
>> +	CLK_TABLE_DIV_M_ENTRY(6),
>> +	CLK_TABLE_DIV_M_ENTRY(7),
>> +	CLK_TABLE_DIV_M_ENTRY(8),
>> +	{ /* sentinel */ }
>> +};
>> +
>> +#define H616_PWM_XY_SRC_GATE(_pair, _reg)		\
>> +struct clk_gate gate_xy_##_pair = {			\
>> +	.reg = (void *)(_reg),				\
>> +	.bit_idx = H616_PWM_XY_CLK_CR_GATE_BIT,		\
>> +	.hw.init = &(struct clk_init_data){		\
>> +		.ops = &clk_gate_ops,			\
>> +	}						\
>> +}
>> +
>> +#define H616_PWM_XY_SRC_MUX(_pair, _reg)		\
>> +struct clk_mux mux_xy_##_pair = {			\
>> +	.reg = (void *)(_reg),				\
>> +	.shift = H616_PWM_XY_CLK_CR_SRC_SHIFT,		\
>> +	.mask = H616_PWM_XY_CLK_CR_SRC_MASK,		\
>> +	.flags = CLK_MUX_ROUND_CLOSEST,			\
>> +	.hw.init = &(struct clk_init_data){		\
>> +		.ops = &clk_mux_ops,			\
>> +	}						\
>> +}
>> +
>> +#define H616_PWM_XY_SRC_DIV(_pair, _reg)		\
>> +struct clk_divider rate_xy_##_pair = {			\
>> +	.reg = (void *)(_reg),				\
>> +	.shift = H616_PWM_XY_CLK_CR_DIV_M_SHIFT,	\
>> +	.table = clk_table_div_m,			\
>> +	.hw.init = &(struct clk_init_data){		\
>> +		.ops = &clk_divider_ops,		\
>> +	}						\
>> +}
>> +
>> +#define H616_PWM_X_DIV(_idx, _reg)			\
>> +struct clk_divider rate_x_##_idx = {			\
>> +	.reg = (void *)(_reg),				\
>> +	.shift = H616_PWM_CTRL_PRESCAL_K_SHIFT,		\
>> +	.width = H616_PWM_CTRL_PRESCAL_K_WIDTH,		\
>> +	.hw.init = &(struct clk_init_data){		\
>> +		.ops = &clk_divider_ops,		\
>> +	}						\
>> +}
>> +
>> +#define H616_PWM_X_BYPASS_GATE(_idx)			\
>> +struct clk_gate gate_x_bypass_##_idx = {		\
>> +	.reg = (void *)H616_PWM_ENR,			\
>> +	.bit_idx = _idx,				\
>> +	.hw.init = &(struct clk_init_data){		\
>> +		.ops = &clk_gate_ops,			\
>> +	}						\
>> +}
>> +
>> +#define H616_PWM_XY_CLK_SRC(_pair, _reg)			\
>> +	static H616_PWM_XY_SRC_MUX(_pair, _reg);		\
>> +	static H616_PWM_XY_SRC_GATE(_pair, _reg);		\
>> +	static H616_PWM_XY_SRC_DIV(_pair, _reg)
>> +
>> +#define H616_PWM_X_CLK(_idx)					\
>> +	static H616_PWM_X_DIV(_idx, H616_PWM_CTRL_REG(_idx))
>> +
>> +#define H616_PWM_X_BYPASS_CLK(_idx)				\
>> +	H616_PWM_X_BYPASS_GATE(_idx)
>> +
>> +#define REF_CLK_XY_SRC(_pair)						\
>> +	{								\
>> +		.name = "pwm-clk-src" #_pair,				\
>> +		.mux_hw = &mux_xy_##_pair.hw,				\
>> +		.gate_hw = &gate_xy_##_pair.hw,				\
>> +		.rate_hw = &rate_xy_##_pair.hw,				\
>> +	}
>> +
>> +#define REF_CLK_X(_idx, _pair)						\
>> +	{								\
>> +		.name = "pwm-clk" #_idx,				\
>> +		.parent_names = (const char *[]){ "pwm-clk-src" #_pair }, \
>> +		.num_parents = 1,					\
>> +		.rate_hw = &rate_x_##_idx.hw,				\
>> +		.flags = CLK_SET_RATE_PARENT,				\
>> +	}
>> +
>> +#define REF_CLK_BYPASS(_idx, _pair)					\
>> +	{								\
>> +		.name = "pwm-clk-bypass" #_idx,				\
>> +		.parent_names = (const char *[]){ "pwm-clk-src" #_pair }, \
>> +		.num_parents = 1,					\
>> +		.gate_hw = &gate_x_bypass_##_idx.hw,			\
>> +		.flags = CLK_SET_RATE_PARENT,	\
>> +	}
>> +
>> +/*
>> + * H616_PWM_clock_src_xy generation:
>> + *             _____      ______      ________
>> + * OSC24M --->|     |    |      |    |        |
>> + * APB1 ----->| Mux |--->| Gate |--->| /div_m |-----> H616_PWM_clock_src_xy
>> + *            |_____|    |______|    |________|
>> + */
>> +H616_PWM_XY_CLK_SRC(01, H616_PWM_XY_CLK_CR(0));
>> +H616_PWM_XY_CLK_SRC(23, H616_PWM_XY_CLK_CR(1));
>> +H616_PWM_XY_CLK_SRC(45, H616_PWM_XY_CLK_CR(2));
>> +
>> +/*
>> + * H616_PWM_clock_x_div generation:
>> + *                            ________
>> + *                           |        | H616_PWM_clock_x/y
>> + * H616_PWM_clock_src_xy --->| /div_k |--------------->
>> + *                           |________|
>> + */
>> +H616_PWM_X_CLK(0);
>> +H616_PWM_X_CLK(1);
>> +H616_PWM_X_CLK(2);
>> +H616_PWM_X_CLK(3);
>> +H616_PWM_X_CLK(4);
>> +H616_PWM_X_CLK(5);
>> +
>> +/*
>> + * H616_PWM_bypass_clock_xy generation:
>> + *                             ______
>> + *                            |      |
>> + * H616_PWM_clock_src_xy ---->| Gate |-------> H616_PWM_bypass_clock_x
>> + *                            |______|
>> + *
>> + * The gate is actually H616_PWM_ENR register.
>> + */
>> +H616_PWM_X_BYPASS_CLK(0);
>> +H616_PWM_X_BYPASS_CLK(1);
>> +H616_PWM_X_BYPASS_CLK(2);
>> +H616_PWM_X_BYPASS_CLK(3);
>> +H616_PWM_X_BYPASS_CLK(4);
>> +H616_PWM_X_BYPASS_CLK(5);
>> +
>> +struct clk_pwm_data {
>> +	const char *name;
>> +	const char **parent_names;
>> +	unsigned int num_parents;
>> +	struct clk_hw *mux_hw;
>> +	struct clk_hw *rate_hw;
>> +	struct clk_hw *gate_hw;
>> +	unsigned long flags;
>> +};
>> +
>> +#define CLK_BYPASS(h616chip, ch) ((h616chip)->data->npwm + (ch))
>> +#define CLK_XY_SRC_IDX(h616chip, ch) ((h616chip)->data->npwm * 2 + ((ch) >> 1))
>> +static struct clk_pwm_data pwmcc_data[] = {
>> +	REF_CLK_X(0, 01),
>> +	REF_CLK_X(1, 01),
>> +	REF_CLK_X(2, 23),
>> +	REF_CLK_X(3, 23),
>> +	REF_CLK_X(4, 45),
>> +	REF_CLK_X(5, 45),
>> +	REF_CLK_BYPASS(0, 01),
>> +	REF_CLK_BYPASS(1, 01),
>> +	REF_CLK_BYPASS(2, 23),
>> +	REF_CLK_BYPASS(3, 23),
>> +	REF_CLK_BYPASS(4, 45),
>> +	REF_CLK_BYPASS(5, 45),
>> +	REF_CLK_XY_SRC(01),
>> +	REF_CLK_XY_SRC(23),
>> +	REF_CLK_XY_SRC(45),
>> +	{ /* sentinel */ }
>> +};
> 
> We'll probably need a way to tie these static definitions to a particular
> instance of the unit for a given chip. But I guess that can be done later
> when adding more chips to the driver.
> 
> I'm not too versed in the clk and pwm APIs but the rest generally looks good
> to me.

Thanks!

> 
> All the best,
> 
> Paul
>