| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <478259f2-b7ec-4664-95cb-3ab56ae6d529@linaro.org>
Date: Tue, 8 Oct 2024 09:30:33 +0200
From: neil.armstrong@...aro.org
To: George Stark <gnstark@...utedevices.com>, u.kleine-koenig@...gutronix.de,
khilman@...libre.com, jbrunet@...libre.com,
martin.blumenstingl@...glemail.com
Cc: linux-pwm@...r.kernel.org, linux-amlogic@...ts.infradead.org,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
kernel@...utedevices.com
Subject: Re: [PATCH 1/3] pwm: meson: Support constant and polarity bits
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@...utedevices.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
>
Powered by blists - more mailing lists