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

[Neil Armstrong <neil.armstrong@linaro.org>]

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

I'd use a better name for that, like: no_mux_blk, pwm_clk_input....

>   };
>   
>   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().

> +	}
> +
>   	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.

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;