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

[Jiayi Zhou <Jiayi.Zhou@amlogic.com>]

在 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