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

[Heiner Kallweit <hkallweit1@gmail.com>]

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