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

[Jerome Brunet]

On Sat 25 Mar 2023 at 23:58, Heiner Kallweit <hkallweit1 at gmail.com> wrote:

> On 25.03.2023 14:24, Jerome Brunet wrote:
>> 
>> On Fri 24 Mar 2023 at 23:23, Heiner Kallweit <hkallweit1 at gmail.com> 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.
>> 
>> As far as the documentation is concerned this is not true.
>> There is a input multiplexer with the xtal, vid_pll, fdiv3 and fdiv4
>> 
>> I'm not sure the differences mentionned here actually exists.
>> 
>
> I don't have access to a S905X4 datasheet, just to the one for S905X3.
> What makes me think that the hw is different:
>
> - In the clock drivers for families before s4 I see no hint that
>   dedicated pwm clocks exist. From s4 there are CLKID_PWM_... clocks.

That the clock driver concern, not the PWM one.

The fact is the whole PWM clock block is still there.
It still has 4 inputs, which may be improperly documented AFAICT.

As previously stated, the doc says: xtal, vid_pll, fdiv3 and fdiv4

Since Amlogic does not bother to intialize the "SEL" value anymore, you
may assume the PWM clock is connected to the input number maching the
register field value. (most likely 0 - so replacing the XTAL)

Since you have been able to test this - change this SEL field, you'll see
what it does and should be able to determine which clock is behind each
of the mux inputs.

>
> - In the S905X3 datasheet I see no hint that the pwm block can be fed
>   from an external clock (except the standard 4 mux parents).
>
>>>
>>> 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
>> 
>> AFAICT, this looks more like a choice in vendor SDK to not use the input
>> mux. IOW, just SW decision.
>> 
>
> - If it would be a sw decision to use internal mux/div or an external
>   clock, then there should be a way to configure whether to use option a or b.
>   I see no such switch in the vendor driver code. Maybe you can check in the
>   datasheet whether there's such a switch.
>
>> I don't think such change makes sense in mainline if the HW has not
>> actually changed.
>> 
>>>
>>> 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;
>>>  };
>>>  
>>>  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;
>>> +		}
>>> +	}
>>> +
>>>  	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",
>>> +		.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;
>>