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

[Heiner Kallweit <hkallweit1@gmail.com>]

On 25.03.2023 23:58, Heiner Kallweit wrote:
> On 25.03.2023 14:24, Jerome Brunet wrote:
>>
>> On Fri 24 Mar 2023 at 23:23, Heiner Kallweit <hkallweit1@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.
> 

In addition, that's what the relevant commit in the vendor driver says:

https://github.com/khadas/linux/commit/4f35972692b52a7f7b96f86b06c01feb7f3b3862

-> sc2 pwm controller moves clk to clktree


> - 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@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;
>>
> 


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