Re: [PATCH] pwm: meson: add support for S4 chip family

[Heiner Kallweit <hkallweit1@gmail.com>]

On 27.03.2023 09:33, Neil Armstrong wrote:
> On 24/03/2023 23:23, Heiner Kallweit wrote:
>> This adds pwm support for (at least) the s4 chip family. The extension
>> is based on the vendor driver that can be found at [0]. There the
>> version with the new clock handling is called meson-v2-pwm.
>> Central change is that the clock is now fully provided by the SoC clock
>> core. The multiplexer isn't any longer part of the pwm block.
>>
>> This was tested on a sc2-based system that uses the same pwm block.
>>
>> [0] https://github.com/khadas/linux/blob/khadas-vims-5.4.y/drivers/pwm/pwm-meson.c
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>> Adding the amlogic,meson-s4-pwm compatible to the documentation was part
>> of the yaml conversion already.
>> ---
>>   drivers/pwm/pwm-meson.c | 38 ++++++++++++++++++++++++++++++++++----
>>   1 file changed, 34 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
>> index 16d79ca5d..7a93fdada 100644
>> --- a/drivers/pwm/pwm-meson.c
>> +++ b/drivers/pwm/pwm-meson.c
>> @@ -98,6 +98,7 @@ struct meson_pwm_channel {
>>   struct meson_pwm_data {
>>       const char * const *parent_names;
>>       unsigned int num_parents;
>> +    unsigned int ext_clk:1;
> 
> Drop the :1, the compiler will align the struct size to the register size, so simply use unsigned int or bool.
> 
Having one flag only it doesn't really make a difference, right.
I chose this single bit because in future there may come another flag,
then having them in one bitfield would be advantageous.

Also https://www.kernel.org/doc/Documentation/process/coding-style.rst
Part "17) Using bool" prefers bits for bool values in structs, even though
the mentioned use cases like multiple flags don't apply here.

> I'd use a better name for that, like: no_mux_blk, pwm_clk_input....
> 
OK

>>   };
>>     struct meson_pwm {
>> @@ -158,6 +159,7 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm,
>>       struct meson_pwm_channel *channel = &meson->channels[pwm->hwpwm];
>>       unsigned int duty, period, pre_div, cnt, duty_cnt;
>>       unsigned long fin_freq;
>> +    int err;
>>         duty = state->duty_cycle;
>>       period = state->period;
>> @@ -165,6 +167,14 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm,
>>       if (state->polarity == PWM_POLARITY_INVERSED)
>>           duty = period - duty;
>>   +    if (meson->data->ext_clk) {
>> +        err = clk_set_rate(channel->clk, 0xffffUL * NSEC_PER_SEC / period);
>> +        if (err) {
>> +            dev_err(meson->chip.dev, "failed to set pwm clock rate\n");
>> +            return err;
>> +        }
> 
> If the same MUX architecture has been moved out of the PWM registers, then why would you call
> set_rate ? we don't call set_rate today, so why should we ?
> 
> Today the MUX selection is static depending on the clk names specified in the bindings (yes it's
> really not the good way to do that,....) but now the MUX is outside the PWM registers block
> we should stick to the same architecture and mark the MUX as NO_REPARENT and instead of using
> a bad bindings specify the MUX parent & rate in dt using assigned-clk-parent/rate.
> 
> So please drop this set_rate().
> 
Implicitly there's a set_rate() also today, by setting the pre_div divider value.
By the way: Not sure why the divider isn't handled via CCF, like e.g. in meson-gx-mmc.

The best (for achieving best precision) input frequency is 0xffff / period.
So we should do our best to come as close as possible to this frequency.
By using set_rate() CCF will choose the optimal mux input and divider value.
Not sure why we should restrict ourselves to one mux parent only.
E.g. for a very low duty cycle a higher input frequency than the xtal 24MHz may be preferable.

Not only the mux is outside the PWM block now, also the divider (8 bit wide instead of 7 bit
as of today). So we need a set_rate() anyway to set the divider value.
What I can't say is whether the internal divider still exists (then external and internal
divider would be cascaded) or is removed or bypassed.
It seems like when adding the external PWM clock feature Amlogic forgot to update
the PWM block documentation, as there's no reference at all to the now external input clock
(except in the clocks section).

>> +    }
>> +
>>       fin_freq = clk_get_rate(channel->clk);
>>       if (fin_freq == 0) {
>>           dev_err(meson->chip.dev, "invalid source clock frequency\n");
>> @@ -173,10 +183,14 @@ 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;
>> +    if (meson->data->ext_clk) {
>> +        pre_div = 0;
>> +    } else {
>> +        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));
>> @@ -445,6 +459,10 @@ static const struct meson_pwm_data pwm_g12a_ee_data = {
>>       .num_parents = ARRAY_SIZE(pwm_g12a_ee_parent_names),
>>   };
>>   +static const struct meson_pwm_data pwm_s4_data = {
>> +    .ext_clk = 1,
>> +};
>> +
>>   static const struct of_device_id meson_pwm_matches[] = {
>>       {
>>           .compatible = "amlogic,meson8b-pwm",
>> @@ -478,6 +496,10 @@ static const struct of_device_id meson_pwm_matches[] = {
>>           .compatible = "amlogic,meson-g12a-ao-pwm-cd",
>>           .data = &pwm_g12a_ao_cd_data
>>       },
>> +    {
>> +        .compatible = "amlogic,meson-s4-pwm",
> 
> This deserved a bindings update, and we should perhaps start a new bindings by using
> new clock-names (!= clkin0/1) to differentiate from the old buggy bindings.
> 
We could simply name the clocks pwm_a, pwm_b. If you think this could be confusing
for pwm PWM_C/PWM_D, then we may use pwm0 and pwm1.

The changed binding would have to reflect that now both input clocks for a pwm
are required.

> Neil
> 
>> +        .data = &pwm_s4_data
>> +    },
>>       {},
>>   };
>>   MODULE_DEVICE_TABLE(of, meson_pwm_matches);
>> @@ -493,6 +515,14 @@ static int meson_pwm_init_channels(struct meson_pwm *meson)
>>       for (i = 0; i < meson->chip.npwm; i++) {
>>           struct meson_pwm_channel *channel = &meson->channels[i];
>>   +        if (meson->data->ext_clk) {
>> +            snprintf(name, sizeof(name), "clkin%u", i);
>> +            channel->clk = devm_clk_get(dev, name);
>> +            if (IS_ERR(channel->clk))
>> +                return PTR_ERR(channel->clk);
>> +            continue;
>> +        }
>> +
>>           snprintf(name, sizeof(name), "%s#mux%u", dev_name(dev), i);
>>             init.name = name;
>