[PATCH] pwm: meson: add support for S4 chip family
neil.armstrong at linaro.org
neil.armstrong at linaro.org
Mon Mar 27 11:11:04 PDT 2023
On 27/03/2023 19:00, Heiner Kallweit wrote:
> 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 at 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.
Ok now I understand your point, it wasn't clear enough the pre-div was out aswell.
This should be clear documented in the code to outline the differences.
>
> 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;
>>
>
More information about the linux-arm-kernel
mailing list