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

[Paul Kocialkowski <paulk@sys-base.io>]

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

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.

> +
> +/* 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.

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.

> +#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(...)

> +
> +/* 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.

All the best,

Paul

-- 
Paul Kocialkowski,

Independent contractor - sys-base - https://www.sys-base.io/
Free software developer - https://www.paulk.fr/

Expert in multimedia, graphics and embedded hardware support with Linux.

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