Re: [PATCH 1/3] pwm: meson: Support constant and polarity bits

[neil.armstrong@linaro.org]

Hi,

On 07/10/2024 21:32, George Stark wrote:
> Newer meson PWM IPs support constant and polarity bits. Support them to
> correctly implement constant and inverted output levels.
> 
> Using constant bit allows to have truly stable low or high output level.
> Since hi and low regs internally increment its values by 1 just writing
> zero to any of them gives 1 clock count impulse. If constant bit is set
> zero value in hi and low regs is not incremented.
> 
> Using polarity bit instead of swapping hi and low reg values allows to
> correctly identify inversion in .get_state().
> 
> Signed-off-by: George Stark <gnstark@salutedevices.com>
> ---
>   drivers/pwm/pwm-meson.c | 75 +++++++++++++++++++++++++++++++++++++----
>   1 file changed, 69 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
> index 98e6c1533312..5d51404bdce3 100644
> --- a/drivers/pwm/pwm-meson.c
> +++ b/drivers/pwm/pwm-meson.c
> @@ -6,7 +6,7 @@
>    * PWM output is achieved by calculating a clock that permits calculating
>    * two periods (low and high). The counter then has to be set to switch after
>    * N cycles for the first half period.
> - * The hardware has no "polarity" setting. This driver reverses the period
> + * Partly the hardware has no "polarity" setting. This driver reverses the period
>    * cycles (the low length is inverted with the high length) for
>    * PWM_POLARITY_INVERSED. This means that .get_state cannot read the polarity
>    * from the hardware.
> @@ -56,6 +56,10 @@
>   #define MISC_B_CLK_SEL_SHIFT	6
>   #define MISC_A_CLK_SEL_SHIFT	4
>   #define MISC_CLK_SEL_MASK	0x3
> +#define MISC_B_CONSTANT_EN	BIT(29)
> +#define MISC_A_CONSTANT_EN	BIT(28)
> +#define MISC_B_INVERT_EN	BIT(27)
> +#define MISC_A_INVERT_EN	BIT(26)
>   #define MISC_B_EN		BIT(1)
>   #define MISC_A_EN		BIT(0)

Nice, seems I completely missed those 2 features!

> 
> @@ -68,6 +72,8 @@ static struct meson_pwm_channel_data {
>   	u8		clk_div_shift;
>   	u8		clk_en_shift;
>   	u32		pwm_en_mask;
> +	u32		const_en_mask;
> +	u32		inv_en_mask;
>   } meson_pwm_per_channel_data[MESON_NUM_PWMS] = {
>   	{
>   		.reg_offset	= REG_PWM_A,
> @@ -75,6 +81,8 @@ static struct meson_pwm_channel_data {
>   		.clk_div_shift	= MISC_A_CLK_DIV_SHIFT,
>   		.clk_en_shift	= MISC_A_CLK_EN_SHIFT,
>   		.pwm_en_mask	= MISC_A_EN,
> +		.const_en_mask	= MISC_A_CONSTANT_EN,
> +		.inv_en_mask	= MISC_A_INVERT_EN,
>   	},
>   	{
>   		.reg_offset	= REG_PWM_B,
> @@ -82,6 +90,8 @@ static struct meson_pwm_channel_data {
>   		.clk_div_shift	= MISC_B_CLK_DIV_SHIFT,
>   		.clk_en_shift	= MISC_B_CLK_EN_SHIFT,
>   		.pwm_en_mask	= MISC_B_EN,
> +		.const_en_mask	= MISC_B_CONSTANT_EN,
> +		.inv_en_mask	= MISC_B_INVERT_EN,
>   	}
>   };
> 
> @@ -99,6 +109,8 @@ struct meson_pwm_channel {
>   struct meson_pwm_data {
>   	const char *const parent_names[MESON_NUM_MUX_PARENTS];
>   	int (*channels_init)(struct pwm_chip *chip);
> +	bool has_constant;
> +	bool has_polarity;
>   };
> 
>   struct meson_pwm {
> @@ -160,7 +172,7 @@ static int meson_pwm_calc(struct pwm_chip *chip, struct pwm_device *pwm,
>   	 * Fixing this needs some care however as some machines might rely on
>   	 * this.
>   	 */
> -	if (state->polarity == PWM_POLARITY_INVERSED)
> +	if (state->polarity == PWM_POLARITY_INVERSED && !meson->data->has_polarity)
>   		duty = period - duty;
> 
>   	freq = div64_u64(NSEC_PER_SEC * 0xffffULL, period);
> @@ -204,6 +216,46 @@ static int meson_pwm_calc(struct pwm_chip *chip, struct pwm_device *pwm,
>   	return 0;
>   }
> 
> +static void meson_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
> +				   bool inverted)
> +{
> +	struct meson_pwm *meson = to_meson_pwm(chip);
> +	const struct meson_pwm_channel_data *channel_data;
> +	unsigned long flags;
> +	u32 value;
> +
> +	channel_data = &meson_pwm_per_channel_data[pwm->hwpwm];
> +
> +	spin_lock_irqsave(&meson->lock, flags);
> +	value = readl(meson->base + REG_MISC_AB);
> +	if (inverted)
> +		value |= channel_data->inv_en_mask;
> +	else
> +		value &= ~channel_data->inv_en_mask;
> +	writel(value, meson->base + REG_MISC_AB);
> +	spin_unlock_irqrestore(&meson->lock, flags);
> +}
> +
> +static void meson_pwm_set_constant(struct pwm_chip *chip, struct pwm_device *pwm,
> +				   bool enable)
> +{
> +	struct meson_pwm *meson = to_meson_pwm(chip);
> +	const struct meson_pwm_channel_data *channel_data;
> +	unsigned long flags;
> +	u32 value;
> +
> +	channel_data = &meson_pwm_per_channel_data[pwm->hwpwm];
> +
> +	spin_lock_irqsave(&meson->lock, flags);
> +	value = readl(meson->base + REG_MISC_AB);
> +	if (enable)
> +		value |= channel_data->const_en_mask;
> +	else
> +		value &= ~channel_data->const_en_mask;
> +	writel(value, meson->base + REG_MISC_AB);
> +	spin_unlock_irqrestore(&meson->lock, flags);
> +}

Those functions looks quite complicated, why can't they be part of
meson_pwm_enable/disable where we already write REG_MISC_AB ?

> +
>   static void meson_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
>   {
>   	struct meson_pwm *meson = to_meson_pwm(chip);
> @@ -255,9 +307,9 @@ static int meson_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>   	int err = 0;
> 
>   	if (!state->enabled) {
> -		if (state->polarity == PWM_POLARITY_INVERSED) {
> +		if (state->polarity == PWM_POLARITY_INVERSED && !meson->data->has_polarity) {
>   			/*
> -			 * This IP block revision doesn't have an "always high"
> +			 * Some of IP block revisions don't have an "always high"
>   			 * setting which we can use for "inverted disabled".
>   			 * Instead we achieve this by setting mux parent with
>   			 * highest rate and minimum divider value, resulting
> @@ -284,6 +336,14 @@ static int meson_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>   		meson_pwm_enable(chip, pwm);
>   	}
> 
> +	if (meson->data->has_constant)
> +		meson_pwm_set_constant(chip, pwm,
> +				       state->duty_cycle == state->period ||
> +				       !state->duty_cycle);
> +	if (meson->data->has_polarity)
> +		meson_pwm_set_polarity(chip, pwm,
> +				       !(state->polarity == PWM_POLARITY_NORMAL));
> +
>   	return 0;
>   }
> 
> @@ -318,6 +378,11 @@ static int meson_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
>   	value = readl(meson->base + REG_MISC_AB);
>   	state->enabled = value & channel_data->pwm_en_mask;
> 
> +	if (meson->data->has_polarity && (value & channel_data->inv_en_mask))
> +		state->polarity = PWM_POLARITY_INVERSED;
> +	else
> +		state->polarity = PWM_POLARITY_NORMAL;
> +
>   	value = readl(meson->base + channel_data->reg_offset);
>   	channel->lo = FIELD_GET(PWM_LOW_MASK, value);
>   	channel->hi = FIELD_GET(PWM_HIGH_MASK, value);
> @@ -325,8 +390,6 @@ static int meson_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
>   	state->period = meson_pwm_cnt_to_ns(chip, pwm, channel->lo + channel->hi);
>   	state->duty_cycle = meson_pwm_cnt_to_ns(chip, pwm, channel->hi);
> 
> -	state->polarity = PWM_POLARITY_NORMAL;
> -
>   	return 0;
>   }
> 
> --
> 2.25.1
>