Re: [PATCH master 2/2] pwm: imx: enable clocks during PWM register accesses

[Bastian Krause <bst@pengutronix.de>]

On 9/4/23 17:49, Ahmad Fatoum wrote:
> This is a port of Linux commit 9f4c8f9607c3147d291b70c13dd01c738ed41faf:
> 
> | Author:     Anson Huang <anson.huang@nxp.com>
> | AuthorDate: Wed Dec 19 05:24:58 2018 +0000
> | Commit:     Thierry Reding <thierry.reding@gmail.com>
> | CommitDate: Mon Dec 24 12:06:56 2018 +0100
> |
> |     pwm: imx: Add ipg clock operation
> |
> |     i.MX PWM module's ipg_clk_s is for PWM register access, on most of i.MX
> |     SoCs, this ipg_clk_s is from system ipg clock or perclk which is always
> |     enabled, but on i.MX7D, the ipg_clk_s is from PWM1_CLK_ROOT which is
> |     controlled by CCGR132, that means the CCGR132 MUST be enabled first
> |     before accessing PWM registers on i.MX7D. This patch adds ipg clock
> |     operation to make sure register access successfully on i.MX7D and it
> |     fixes Linux kernel boot up hang during PWM driver probe.
> |
> |     Fixes: 4a23e6ee9f69 ("ARM: dts: imx7d-sdb: Restore pwm backlight support")
> |     Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> |     Signed-off-by: Thierry Reding <thierry.reding@gmail.com>
> 
> Unlike the Linux version, we make clk_ipg optional to reduce changes for
> older SoCs.
> 
> This fixes system hang during PWM access on i.MX8M and presumably i.MX7.
> 
> Reported-by: Bastian Krause <bst@pengutronix.de>
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

The port of the Linux commit looks good. I'm using this to enable some
PWM LEDs. Unfortunately the LEDs don't remain on with this, because
we're missing Linux commit 15d4dbd601591 ("pwm: imx27: Fix clock
handling in pwm_imx27_apply()").

See below for a suggested change.

> ---
>   drivers/pwm/pwm-imx.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 42 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> index 2dc7e4cfd64d..486ca962f96d 100644
> --- a/drivers/pwm/pwm-imx.c
> +++ b/drivers/pwm/pwm-imx.c
> @@ -36,7 +36,7 @@
>   #define MX3_PWMCR_EN              (1 << 0)
>   
>   struct imx_chip {
> -	struct clk	*clk_per;
> +	struct clk	*clk_per, *clk_ipg;
>   
>   	void __iomem	*mmio_base;
>   
> @@ -93,14 +93,42 @@ static void imx_pwm_set_enable_v1(struct pwm_chip *chip, bool enable)
>   	writel(val, imx->mmio_base + MX1_PWMC);
>   }
>   
> +static int imx_pwm_clk_enable_v2(struct imx_chip *imx)
> +{
> +	int ret;
> +
> +	ret = clk_enable(imx->clk_ipg);
> +	if (ret)
> +		return ret;
> +
> +	ret = clk_prepare_enable(imx->clk_per);
> +	if (ret) {
> +		clk_disable_unprepare(imx->clk_ipg);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static void imx_pwm_clk_disable_v2(struct imx_chip *imx)
> +{
> +	clk_disable_unprepare(imx->clk_per);
> +	clk_disable_unprepare(imx->clk_ipg);
> +}
> +
>   static int imx_pwm_config_v2(struct pwm_chip *chip,
>   		int duty_ns, int period_ns)
>   {
>   	struct imx_chip *imx = to_imx_chip(chip);
>   	unsigned long long c;
>   	unsigned long period_cycles, duty_cycles, prescale;
> +	int ret;
>   	u32 cr;
>   
> +	ret = imx_pwm_clk_enable_v2(imx);
> +	if (ret)
> +		return ret;
> +
>   	c = clk_get_rate(imx->clk_per);
>   	c = c * period_ns;
>   	do_div(c, 1000000000);
> @@ -134,6 +162,8 @@ static int imx_pwm_config_v2(struct pwm_chip *chip,
>   
>   	writel(cr, imx->mmio_base + MX3_PWMCR);
>   
> +	imx_pwm_clk_disable_v2(imx);
> +

I think this should be:

if (!chip->state.p_enable)
         imx_pwm_clk_disable_v2(imx);

>   	return 0;
>   }
>   
> @@ -141,6 +171,11 @@ static void imx_pwm_set_enable_v2(struct pwm_chip *chip, bool enable)
>   {
>   	struct imx_chip *imx = to_imx_chip(chip);
>   	u32 val;
> +	int ret;
> +
> +	ret = imx_pwm_clk_enable_v2(imx);
> +	if (WARN_ON(ret))
> +		return;
>   
>   	val = readl(imx->mmio_base + MX3_PWMCR);
>   
> @@ -150,6 +185,8 @@ static void imx_pwm_set_enable_v2(struct pwm_chip *chip, bool enable)
>   		val &= ~MX3_PWMCR_EN;
>   
>   	writel(val, imx->mmio_base + MX3_PWMCR);
> +
> +	imx_pwm_clk_disable_v2(imx);

I think this should be:

if (!enable)
         imx_pwm_clk_disable_v2(imx);

With these changes, the clocks remain on if the PWM is running and my
PWM LEDs keep their state.

With the suggested changes:

   Tested-by: Bastian Krause <bst@pengutronix.de>

Regards,
Bastian

>   }
>   
>   static int imx_pwm_apply(struct pwm_chip *chip, const struct pwm_state *state)
> @@ -215,6 +252,10 @@ static int imx_pwm_probe(struct device *dev)
>   
>   	imx = xzalloc(sizeof(*imx));
>   
> +	imx->clk_ipg = clk_get_optional(dev, "ipg");
> +	if (IS_ERR(imx->clk_ipg))
> +		return PTR_ERR(imx->clk_ipg);
> +
>   	imx->clk_per = clk_get(dev, "per");
>   	if (IS_ERR(imx->clk_per))
>   		return PTR_ERR(imx->clk_per);

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |