Re: [PATCH] pwm: meson: external clock configuration for s4

Re: [PATCH] pwm: meson: external clock configuration for s4

[Martin Blumenstingl]

Hello,

I am adding my thoughts in this email which are in addition to Uwe's comments.

Please always Cc the linux-amlogic mailing list when you are sending patches.
Currently this patch cannot be found in the mailing list archives,
patchwork, ...

On Thu, Dec 23, 2021 at 6:58 AM Jiayi Zhou <Jiayi.Zhou@amlogic.com> wrote:
>
> From: "jiayi.zhou" <jiayi.zhou@amlogic.com>
>
> For PWM controller in the Meson-S4 SoC,
> PWM needs to obtain an external clock source.
> This patch tries to describe them in the DT compatible data.
I think this patch does more than what is described here.
It adds support for the constant high/low output, which is a feature
which I think has been introduced on the G12A SoC. I suggest having a
separate patch for this and also enabling it on supported SoCs (not
just the S4).

[...]
> +#define PWM_DISABLE            0
I don't see this being used anywhere so I think it can be dropped

[...]
> +       /*
> +        * duty_cycle equal 0% and 100%,constant should be enabled,
> +        * high and low count will not incease one;
typo: incease -> increase

[...]
> +       if (meson->data->extern_clk) {
> +               set_clk = clk_get_rate(channel->clk);
> +               if (set_clk == 0)
> +                       dev_err(meson->chip.dev, "invalid source clock frequency\n");
> +               set_clk /= (channel->pre_div + 1);
> +               err = clk_set_rate(channel->clk, set_clk);
> +               if (err)
> +                       dev_err(meson->chip.dev, "%s: error in setting pwm rate!\n", __func__);
> +       }
> +
>         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;
Why does this new controller version require clk_set_rate with a rate
that's calculated based on the pre-divider and then also configuring
the pre-divider in the MISC_AB register?
This seems odd, can't we use only one of them (either clk_set_rate or
setting MISC_AB)?

Also if clk_set_rate is the way to go here I suggest refactoring the
existing code so the pre-divider is a clock as well so we can use
clk_set_rate for both, the "old" and "new" IP version.

[...]
> +static const struct meson_pwm_data pwm_meson_data = {
> +       .extern_clk = true,
>  };

[...]
> +static int meson_pwm_v2_init_channels(struct meson_pwm *meson)
> +{
> +       struct meson_pwm_channel *channels = meson->channels;
> +       struct device *dev = meson->chip.dev;
> +       unsigned int i;
> +       char name[255];
> +
> +       for (i = 0; i < (meson->chip.npwm / 2); i++) {
> +               snprintf(name, sizeof(name), "clkin%u", i);
> +               (channels + i)->clk = devm_clk_get(dev, name);
> +               if (IS_ERR((channels + i)->clk)) {
> +                       dev_err(meson->chip.dev, "can't get device clock\n");
> +                       return PTR_ERR((channels + i)->clk);
> +               }
> +               (channels + i + 2)->clk = (channels + i)->clk;
> +       }
I think this is whole function is broken.
The vendor driver uses four channels per PWM controller on newer SoCs.
In the mainline driver we only support two PWM channels per controller.

Now the whole loop starts with npwm / 2 which is always 1 in the
mainline driver.
This is the first problem: this would never initialize the second PWM.
The second problem is that "channels + i + 2" would access channels[0
+ 2] which is out of bounds for this array (that array only holds two
values so only index 0 and 1 are valid).


Best regards,
Martin

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

Re: [PATCH] pwm: meson: external clock configuration for s4

[Jiayi Zhou]

在 2022/1/13 20:24, Jiayi Zhou 写道:
>
>
> 在 2021/12/25 2:24, Martin Blumenstingl 写道:
>> Hello,
>>
>> I am adding my thoughts in this email which are in addition to Uwe's comments.
>>
>> Please always Cc the linux-amlogic mailing list when you are sending patches.
>> Currently this patch cannot be found in the mailing list archives,
>> patchwork, ...
>     OK, I'll cc the linux-amlogic mailing list when I send the v2 release.
>> On Thu, Dec 23, 2021 at 6:58 AM Jiayi Zhou<Jiayi.Zhou@amlogic.com>  wrote:
>>> From: "jiayi.zhou"<jiayi.zhou@amlogic.com>
>>>
>>> For PWM controller in the Meson-S4 SoC,
>>> PWM needs to obtain an external clock source.
>>> This patch tries to describe them in the DT compatible data.
>> I think this patch does more than what is described here.
>> It adds support for the constant high/low output, which is a feature
>> which I think has been introduced on the G12A SoC. I suggest having a
>> separate patch for this and also enabling it on supported SoCs (not
>> just the S4).
>     The relevant code has been removed in the v2 version.
>> [...]
>>> +#define PWM_DISABLE            0
>> I don't see this being used anywhere so I think it can be dropped
>     code has been removed in v2 verison.
>> [...]
>>> +       /*
>>> +        * duty_cycle equal 0% and 100%,constant should be enabled,
>>> +        * high and low count will not incease one;
>> typo: incease -> increase
>     It has been modified in v2 version.
>> [...]
>>> +       if (meson->data->extern_clk) {
>>> +               set_clk = clk_get_rate(channel->clk);
>>> +               if (set_clk == 0)
>>> +                       dev_err(meson->chip.dev, "invalid source clock frequency\n");
>>> +               set_clk /= (channel->pre_div + 1);
>>> +               err = clk_set_rate(channel->clk, set_clk);
>>> +               if (err)
>>> +                       dev_err(meson->chip.dev, "%s: error in setting pwm rate!\n", __func__);
>>> +       }
>>> +
>>>          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;
>> Why does this new controller version require clk_set_rate with a rate
>> that's calculated based on the pre-divider and then also configuring
>> the pre-divider in the MISC_AB register?
>> This seems odd, can't we use only one of them (either clk_set_rate or
>> setting MISC_AB)?
>>
>> Also if clk_set_rate is the way to go here I suggest refactoring the
>> existing code so the pre-divider is a clock as well so we can use
>> clk_set_rate for both, the "old" and "new" IP version.
>
>  This part of the code was refactored in the v2 version, using 
> clk_set_rate to
>
>  differentiate the "old" and "new" IP versions.
>
>> [...]
>>> +static const struct meson_pwm_data pwm_meson_data = {
>>> +       .extern_clk = true,
>>>   };
>> [...]
>>> +static int meson_pwm_v2_init_channels(struct meson_pwm *meson)
>>> +{
>>> +       struct meson_pwm_channel *channels = meson->channels;
>>> +       struct device *dev = meson->chip.dev;
>>> +       unsigned int i;
>>> +       char name[255];
>>> +
>>> +       for (i = 0; i < (meson->chip.npwm / 2); i++) {
>>> +               snprintf(name, sizeof(name), "clkin%u", i);
>>> +               (channels + i)->clk = devm_clk_get(dev, name);
>>> +               if (IS_ERR((channels + i)->clk)) {
>>> +                       dev_err(meson->chip.dev, "can't get device clock\n");
>>> +                       return PTR_ERR((channels + i)->clk);
>>> +               }
>>> +               (channels + i + 2)->clk = (channels + i)->clk;
>>> +       }
>> I think this is whole function is broken.
>> The vendor driver uses four channels per PWM controller on newer SoCs.
>> In the mainline driver we only support two PWM channels per controller.
>>
>> Now the whole loop starts with npwm / 2 which is always 1 in the
>> mainline driver.
>> This is the first problem: this would never initialize the second PWM.
>> The second problem is that "channels + i + 2" would access channels[0
>> + 2] which is out of bounds for this array (that array only holds two
>> values so only index 0 and 1 are valid)..
>
>  This part of the code has been fixed in the v2 version.
>
>> Best regards,
>> Martin
>>
>> .

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