Re: [PATCH v4 4/4] pwm: meson: make full use of common clock framework

[neil.armstrong@linaro.org]

On 17/04/2023 11:53, Heiner Kallweit wrote:
> On 17.04.2023 09:23, Neil Armstrong wrote:
>> On 13/04/2023 07:54, Heiner Kallweit wrote:
>>> Newer versions of the PWM block use a core clock with external mux,
>>> divider, and gate. These components either don't exist any longer in
>>> the PWM block, or they are bypassed.
>>> To minimize needed changes for supporting the new version, the internal
>>> divider and gate should be handled by CCF too.
>>>
>>> I didn't see a good way to split the patch, therefore it's somewhat
>>> bigger. What it does:
>>>
>>> - The internal mux is handled by CCF already. Register also internal
>>>     divider and gate with CCF, so that we have one representation of the
>>>     input clock: [mux] parent of [divider] parent of [gate]
>>>     - Now that CCF selects an appropriate mux parent, we don't need the
>>>     DT-provided default parent any longer. Accordingly we can also omit
>>>     setting the mux parent directly in the driver.
>>>     - Instead of manually handling the pre-div divider value, let CCF
>>>     set the input clock. Targeted input clock frequency is
>>>     0xffff * 1/period for best precision.
>>>     - For the "inverted pwm disabled" scenario target an input clock
>>>     frequency of 1GHz. This ensures that the remaining low pulses
>>>     have minimum length.
>>>
>>> I don't have hw with the old PWM block, therefore I couldn't test this
>>> patch. With the not yet included extension for the new PWM block
>>> (channel->clk coming directly from get_clk(external_clk)) I didn't
>>> notice any problem. My system uses PWM for the CPU voltage regulator
>>> and for the SDIO 32kHz clock.
>>>
>>> Note: The clock gate in the old PWM block is permanently disabled.
>>> This seems to indicate that it's not used by the new PWM block.
>>>
>>> Tested-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>> ---
>>> Changes to RFT/RFC version:
>>> - use parent_hws instead of parent_names for div/gate clock
>>> - use devm_clk_hw_register where the struct clk * returned by
>>>     devm_clk_register isn't needed
>>>
>>> v2:
>>> - add patch 1
>>> - add patch 3
>>> - switch to using clk_parent_data in all relevant places
>>> v3:
>>> - add flag CLK_IGNORE_UNUSED
>>> v4:
>>> - remove variable tmp in meson_pwm_get_state
>>> - don't use deprecated function devm_clk_register
>>> ---
>>>    drivers/pwm/pwm-meson.c | 142 +++++++++++++++++++++++-----------------
>>>    1 file changed, 83 insertions(+), 59 deletions(-)
>>>
>>> diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
>>> index 40a8709ff..80ac71cbc 100644
>>> --- a/drivers/pwm/pwm-meson.c
>>> +++ b/drivers/pwm/pwm-meson.c
>>> @@ -51,7 +51,7 @@
>>>    #define REG_MISC_AB        0x8
>>>    #define MISC_B_CLK_EN        23
>>>    #define MISC_A_CLK_EN        15
>>> -#define MISC_CLK_DIV_MASK    0x7f
>>> +#define MISC_CLK_DIV_WIDTH    7
>>>    #define MISC_B_CLK_DIV_SHIFT    16
>>>    #define MISC_A_CLK_DIV_SHIFT    8
>>>    #define MISC_B_CLK_SEL_SHIFT    6
>>> @@ -87,12 +87,13 @@ static struct meson_pwm_channel_data {
>>>    };
>>>      struct meson_pwm_channel {
>>> +    unsigned long rate;
>>>        unsigned int hi;
>>>        unsigned int lo;
>>> -    u8 pre_div;
>>>    -    struct clk *clk_parent;
>>>        struct clk_mux mux;
>>> +    struct clk_divider div;
>>> +    struct clk_gate gate;
>>>        struct clk *clk;
>>>    };
>>>    @@ -125,16 +126,6 @@ static int meson_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
>>>        struct device *dev = chip->dev;
>>>        int err;
>>>    -    if (channel->clk_parent) {
>>> -        err = clk_set_parent(channel->clk, channel->clk_parent);
>>> -        if (err < 0) {
>>> -            dev_err(dev, "failed to set parent %s for %s: %d\n",
>>> -                __clk_get_name(channel->clk_parent),
>>> -                __clk_get_name(channel->clk), err);
>>> -            return err;
>>> -        }
>>> -    }
>>> -
>>>        err = clk_prepare_enable(channel->clk);
>>>        if (err < 0) {
>>>            dev_err(dev, "failed to enable clock %s: %d\n",
>>> @@ -157,8 +148,9 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm,
>>>                  const struct pwm_state *state)
>>>    {
>>>        struct meson_pwm_channel *channel = &meson->channels[pwm->hwpwm];
>>> -    unsigned int duty, period, pre_div, cnt, duty_cnt;
>>> +    unsigned int duty, period, cnt, duty_cnt;
>>>        unsigned long fin_freq;
>>> +    u64 freq;
>>>          duty = state->duty_cycle;
>>>        period = state->period;
>>> @@ -166,7 +158,11 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm,
>>>        if (state->polarity == PWM_POLARITY_INVERSED)
>>>            duty = period - duty;
>>>    -    fin_freq = clk_get_rate(channel->clk);
>>> +    freq = div64_u64(NSEC_PER_SEC * (u64)0xffff, period);
>>> +    if (freq > ULONG_MAX)
>>> +        freq = ULONG_MAX;
>>> +
>>> +    fin_freq = clk_round_rate(channel->clk, freq);
>>>        if (fin_freq == 0) {
>>>            dev_err(meson->chip.dev, "invalid source clock frequency\n");
>>>            return -EINVAL;
>>> @@ -174,46 +170,35 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm,
>>>          dev_dbg(meson->chip.dev, "fin_freq: %lu Hz\n", fin_freq);
>>>    -    pre_div = div64_u64(fin_freq * (u64)period, NSEC_PER_SEC * 0xffffLL);
>>> -    if (pre_div > MISC_CLK_DIV_MASK) {
>>> -        dev_err(meson->chip.dev, "unable to get period pre_div\n");
>>> -        return -EINVAL;
>>> -    }
>>> -
>>> -    cnt = div64_u64(fin_freq * (u64)period, NSEC_PER_SEC * (pre_div + 1));
>>> +    cnt = div64_u64(fin_freq * (u64)period, NSEC_PER_SEC);
>>>        if (cnt > 0xffff) {
>>>            dev_err(meson->chip.dev, "unable to get period cnt\n");
>>>            return -EINVAL;
>>>        }
>>>    -    dev_dbg(meson->chip.dev, "period=%u pre_div=%u cnt=%u\n", period,
>>> -        pre_div, cnt);
>>> +    dev_dbg(meson->chip.dev, "period=%u cnt=%u\n", period, cnt);
>>>          if (duty == period) {
>>> -        channel->pre_div = pre_div;
>>>            channel->hi = cnt;
>>>            channel->lo = 0;
>>>        } else if (duty == 0) {
>>> -        channel->pre_div = pre_div;
>>>            channel->hi = 0;
>>>            channel->lo = cnt;
>>>        } else {
>>> -        /* Then check is we can have the duty with the same pre_div */
>>> -        duty_cnt = div64_u64(fin_freq * (u64)duty,
>>> -                     NSEC_PER_SEC * (pre_div + 1));
>>> +        duty_cnt = div64_u64(fin_freq * (u64)duty, NSEC_PER_SEC);
>>>            if (duty_cnt > 0xffff) {
>>>                dev_err(meson->chip.dev, "unable to get duty cycle\n");
>>>                return -EINVAL;
>>>            }
>>>    -        dev_dbg(meson->chip.dev, "duty=%u pre_div=%u duty_cnt=%u\n",
>>> -            duty, pre_div, duty_cnt);
>>> +        dev_dbg(meson->chip.dev, "duty=%u duty_cnt=%u\n", duty, duty_cnt);
>>>    -        channel->pre_div = pre_div;
>>>            channel->hi = duty_cnt;
>>>            channel->lo = cnt - duty_cnt;
>>>        }
>>>    +    channel->rate = fin_freq;
>>> +
>>>        return 0;
>>>    }
>>>    @@ -223,16 +208,15 @@ static void meson_pwm_enable(struct meson_pwm *meson, struct pwm_device *pwm)
>>>        struct meson_pwm_channel_data *channel_data;
>>>        unsigned long flags;
>>>        u32 value;
>>> +    int err;
>>>          channel_data = &meson_pwm_per_channel_data[pwm->hwpwm];
>>>    -    spin_lock_irqsave(&meson->lock, flags);
>>> +    err = clk_set_rate(channel->clk, channel->rate);
>>> +    if (err)
>>> +        dev_err(meson->chip.dev, "setting clock rate failed\n");
>>>    -    value = readl(meson->base + REG_MISC_AB);
>>> -    value &= ~(MISC_CLK_DIV_MASK << channel_data->clk_div_shift);
>>> -    value |= channel->pre_div << channel_data->clk_div_shift;
>>> -    value |= BIT(channel_data->clk_en_bit);
>>> -    writel(value, meson->base + REG_MISC_AB);
>>> +    spin_lock_irqsave(&meson->lock, flags);
>>>          value = FIELD_PREP(PWM_HIGH_MASK, channel->hi) |
>>>            FIELD_PREP(PWM_LOW_MASK, channel->lo);
>>> @@ -271,16 +255,16 @@ static int meson_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>>>                /*
>>>                 * This IP block revision doesn't have an "always high"
>>>                 * setting which we can use for "inverted disabled".
>>> -             * Instead we achieve this using the same settings
>>> -             * that we use a pre_div of 0 (to get the shortest
>>> -             * possible duration for one "count") and
>>> +             * Instead we achieve this by setting an arbitrary,
>>> +             * very high frequency, resulting in the shortest
>>> +             * possible duration for one "count" and
>>>                 * "period == duty_cycle". This results in a signal
>>>                 * which is LOW for one "count", while being HIGH for
>>>                 * the rest of the (so the signal is HIGH for slightly
>>>                 * less than 100% of the period, but this is the best
>>>                 * we can achieve).
>>>                 */
>>> -            channel->pre_div = 0;
>>> +            channel->rate = 1000000000;
>>>                channel->hi = ~0;
>>>                channel->lo = 0;
>>
>> This looks like a really bad idea... please don't do that and instead introduce
>> some pinctrl states where we set the PWM pin as GPIO mode high/low state like we
>> did for SPI:
>> https://lore.kernel.org/r/20221004-up-aml-fix-spi-v4-2-0342d8e10c49@baylibre.com
>>
> 
> There's no behavior change in this patch set. So my understanding is that you'd
> like to change the current behavior independent of the CCF-related changes.

There's a behavior change since you change the calculation with a radically different
algorithm.

Neil

> 
> For the updated PWM block (at least for S905X3, not sure with which family Amlogic
> extended the PWM block) we have an integrated option to achieve constant low/high
> output. It's just not implemented yet, it's something I may do as next step.
> The extended PWM block added new bits pwm_A/B_constant_en that prevent the default
> increment of the hi/lo period. By supporting these bits we can achieve value 0
> for hi/lo.
> IMO that's easier than adding pinctrl handling. The remaining question would be
> whether it's worth it to add pinctrl handling just for the legacy version of the
> PWM block.
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel