Re: [PATCH 1/2] clk: meson: s4: keep the PWM clocks off vid_pll

[Jerome Brunet <jbrunet@baylibre.com>]

On dim. 25 oct. 2026 at 20:18, christian.koever-draxl@student.uibk.ac.at wrote:

> From: Christian Stefan Kövér-Draxl <ckoever@outlook.de>
>
> The PWM driver asks the clock framework for the fastest input it can
> get (clk_round_rate() with roughly 0xffff/period), so on S4 every PWM
> channel ends up parented to vid_pll, the fastest input of the PWM mux.
> vid_pll is derived from the HDMI PLL and is reprogrammed on every
> display mode set, which silently changes the period and duty cycle of
> whatever PWM runs from it.

While I agree with the general principle of the change,
"The PWM driver asks the clock framework for the fastest input" is a
fairly inaccurate representation of what the PWM driver does and it gets
repeated. Please fix this.

You've disclose the usage AI, it is great. It would be even better if
you could summarize its output a little bit. This is very verbose
for a simple problem: vid_pll is dedicated to another usage in general
and should actually not be provided to PWM devices, eventhough the HW
allows it

>
> On boards that use a PWM regulator for the CPU supply this corrupts
> the CPU voltage: on an S905W2 board vddcpu read 699 mV at 1 GHz and
> the first cpufreq transition hung the SoC. Boards that feed the
> 32.768 kHz low power clock of an SDIO wifi chip from a PWM see the chip
> intermittently fail to enumerate ("Failed to initialize a
> non-removable card").
>
> Drop vid_pll from the PWM parents and keep the remaining inputs on
> their register values with a mux table. xtal, fclk_div4 and fclk_div3
> cover every PWM rate in use.
>
> Fixes: 57b55c76aaf1 ("clk: meson: S4: add support for Amlogic S4 SoC peripheral clock controller")
> Signed-off-by: Christian Stefan Kövér-Draxl <ckoever@outlook.de>
> ---
>  drivers/clk/meson/s4-peripherals.c | 38 ++++++++++++++++++++----------
>  1 file changed, 26 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/clk/meson/s4-peripherals.c b/drivers/clk/meson/s4-peripherals.c
> index ba41fcd90588..329cd526749e 100644
> --- a/drivers/clk/meson/s4-peripherals.c
> +++ b/drivers/clk/meson/s4-peripherals.c
> @@ -2588,51 +2588,65 @@ static struct clk_regmap s4_spicc0_en = {
>  	},
>  };
>  
> -/* PWM Clock */
> +/*
> + * PWM Clock
> + *
> + * vid_pll (mux input 1) is left out on purpose. The PWM driver asks for the
> + * fastest clock it can get, which made every PWM pick vid_pll - and vid_pll
> + * is the HDMI PLL, reprogrammed on every display mode set. That silently
> + * changed the period and duty of whatever ran from it: the PWM CPU supply
> + * (a cpufreq voltage change then hung the SoC) and the 32.768kHz clock of an
> + * SDIO wifi chip (which then failed to enumerate).
> + */
> +static u32 s4_pwm_parents_table[] = { 0, 2, 3 };
> +
>  static const struct clk_parent_data s4_pwm_parents[] = {
>  	{ .fw_name = "xtal", },
> -	{ .hw = &s4_vid_pll.hw },
>  	{ .fw_name = "fclk_div4", },
>  	{ .fw_name = "fclk_div3", },
>  };
>  
> -static S4_COMP_SEL(pwm_a, CLKCTRL_PWM_CLK_AB_CTRL, 9, 0x3, s4_pwm_parents);
> +#define S4_PWM_SEL(_name, _reg, _shift) \
> +	MESON_COMP_SEL(s4_, _name, _reg, _shift, 0x3, s4_pwm_parents, \
> +		       s4_pwm_parents_table, 0, 0)
> +
> +static S4_PWM_SEL(pwm_a, CLKCTRL_PWM_CLK_AB_CTRL, 9);
>  static S4_COMP_DIV(pwm_a, CLKCTRL_PWM_CLK_AB_CTRL, 0, 8);
>  static S4_COMP_GATE(pwm_a, CLKCTRL_PWM_CLK_AB_CTRL, 8);
>  
> -static S4_COMP_SEL(pwm_b, CLKCTRL_PWM_CLK_AB_CTRL, 25, 0x3, s4_pwm_parents);
> +static S4_PWM_SEL(pwm_b, CLKCTRL_PWM_CLK_AB_CTRL, 25);
>  static S4_COMP_DIV(pwm_b, CLKCTRL_PWM_CLK_AB_CTRL, 16, 8);
>  static S4_COMP_GATE(pwm_b, CLKCTRL_PWM_CLK_AB_CTRL, 24);
>  
> -static S4_COMP_SEL(pwm_c, CLKCTRL_PWM_CLK_CD_CTRL, 9, 0x3, s4_pwm_parents);
> +static S4_PWM_SEL(pwm_c, CLKCTRL_PWM_CLK_CD_CTRL, 9);
>  static S4_COMP_DIV(pwm_c, CLKCTRL_PWM_CLK_CD_CTRL, 0, 8);
>  static S4_COMP_GATE(pwm_c, CLKCTRL_PWM_CLK_CD_CTRL, 8);
>  
> -static S4_COMP_SEL(pwm_d, CLKCTRL_PWM_CLK_CD_CTRL, 25, 0x3, s4_pwm_parents);
> +static S4_PWM_SEL(pwm_d, CLKCTRL_PWM_CLK_CD_CTRL, 25);
>  static S4_COMP_DIV(pwm_d, CLKCTRL_PWM_CLK_CD_CTRL, 16, 8);
>  static S4_COMP_GATE(pwm_d, CLKCTRL_PWM_CLK_CD_CTRL, 24);
>  
> -static S4_COMP_SEL(pwm_e, CLKCTRL_PWM_CLK_EF_CTRL, 9, 0x3, s4_pwm_parents);
> +static S4_PWM_SEL(pwm_e, CLKCTRL_PWM_CLK_EF_CTRL, 9);
>  static S4_COMP_DIV(pwm_e, CLKCTRL_PWM_CLK_EF_CTRL, 0, 8);
>  static S4_COMP_GATE(pwm_e, CLKCTRL_PWM_CLK_EF_CTRL, 8);
>  
> -static S4_COMP_SEL(pwm_f, CLKCTRL_PWM_CLK_EF_CTRL, 25, 0x3, s4_pwm_parents);
> +static S4_PWM_SEL(pwm_f, CLKCTRL_PWM_CLK_EF_CTRL, 25);
>  static S4_COMP_DIV(pwm_f, CLKCTRL_PWM_CLK_EF_CTRL, 16, 8);
>  static S4_COMP_GATE(pwm_f, CLKCTRL_PWM_CLK_EF_CTRL, 24);
>  
> -static S4_COMP_SEL(pwm_g, CLKCTRL_PWM_CLK_GH_CTRL, 9, 0x3, s4_pwm_parents);
> +static S4_PWM_SEL(pwm_g, CLKCTRL_PWM_CLK_GH_CTRL, 9);
>  static S4_COMP_DIV(pwm_g, CLKCTRL_PWM_CLK_GH_CTRL, 0, 8);
>  static S4_COMP_GATE(pwm_g, CLKCTRL_PWM_CLK_GH_CTRL, 8);
>  
> -static S4_COMP_SEL(pwm_h, CLKCTRL_PWM_CLK_GH_CTRL, 25, 0x3, s4_pwm_parents);
> +static S4_PWM_SEL(pwm_h, CLKCTRL_PWM_CLK_GH_CTRL, 25);
>  static S4_COMP_DIV(pwm_h, CLKCTRL_PWM_CLK_GH_CTRL, 16, 8);
>  static S4_COMP_GATE(pwm_h, CLKCTRL_PWM_CLK_GH_CTRL, 24);
>  
> -static S4_COMP_SEL(pwm_i, CLKCTRL_PWM_CLK_IJ_CTRL, 9, 0x3, s4_pwm_parents);
> +static S4_PWM_SEL(pwm_i, CLKCTRL_PWM_CLK_IJ_CTRL, 9);
>  static S4_COMP_DIV(pwm_i, CLKCTRL_PWM_CLK_IJ_CTRL, 0, 8);
>  static S4_COMP_GATE(pwm_i, CLKCTRL_PWM_CLK_IJ_CTRL, 8);
>  
> -static S4_COMP_SEL(pwm_j, CLKCTRL_PWM_CLK_IJ_CTRL, 25, 0x3, s4_pwm_parents);
> +static S4_PWM_SEL(pwm_j, CLKCTRL_PWM_CLK_IJ_CTRL, 25);
>  static S4_COMP_DIV(pwm_j, CLKCTRL_PWM_CLK_IJ_CTRL, 16, 8);
>  static S4_COMP_GATE(pwm_j, CLKCTRL_PWM_CLK_IJ_CTRL, 24);
>  
> -- 
> 2.56.0
>

-- 
Jerome