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

[Heiner Kallweit <hkallweit1@gmail.com>]

On 25.03.2023 09:20, Uwe Kleine-König wrote:
> Hello,
> 
> On Fri, Mar 24, 2023 at 11:23:09PM +0100, 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@gmail.com>
>> ---
>> Adding the amlogic,meson-s4-pwm compatible to the documentation was part
>> of the yaml conversion already.
> 
> This refers to
> https://lore.kernel.org/linux-pwm/3edc5ba6-bf3d-e45b-377a-9e7ece7642a7@gmail.com
> 
Right

> Does the external mux clk behave in the same way as the internal ones
> before? (I.e. it can select one of a few parents and has a single
> divider?)
> 
Yes, it's a standard clock with few parents, a mux, and a divider.

>> ---
>>  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 could be local to this block.
> 
OK

>> +		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;
>> +		}
>> +
> 
> This requires a binding change, right? Would it make sense to drop the
> ext_clk flag and determine that by trying to get the clk and if it's
> there, assume ext_clk would have been set?
> 
Clocks clkin0 and clkin1 are used already, therefore the binding doesn't change.
Just the type of clock usage is different. So far the clocks are the parents
for the internal mux/div, now it's the "final" clocks.

> Also I wonder if it would make sense to use the same name as used when
> the mux clk is internal, i.e. reuse name from the line below.
> 
IMO this name is ok when used internally, but wouldn't be nice if used in
the binding, e.g. because part of it is dev_name(dev). Therefore I'd prefer
to stick with the clkin0/clkin1 names.

>>  		snprintf(name, sizeof(name), "%s#mux%u", dev_name(dev), i);
> 
> Best regards
> Uwe
> 
Heiner