mail archive of the barebox mailing list
 help / color / mirror / Atom feed
From: Bastian Krause <bst@pengutronix.de>
To: Ahmad Fatoum <a.fatoum@pengutronix.de>, barebox@lists.infradead.org
Cc: "Uwe Kleine-König" <ukl@pengutronix.de>,
	"Marco Felsch" <mfe@pengutronix.de>
Subject: Re: [PATCH master 2/2] pwm: imx: enable clocks during PWM register accesses
Date: Wed, 6 Sep 2023 14:42:24 +0200	[thread overview]
Message-ID: <d2c8ddf2-1e63-490a-859e-adf4806d4047@pengutronix.de> (raw)
In-Reply-To: <20230904154953.2367158-2-a.fatoum@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 |




  parent reply	other threads:[~2023-09-06 12:44 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-09-04 15:49 [PATCH master 1/2] clk: implement clk_get_optional helper Ahmad Fatoum
2023-09-04 15:49 ` [PATCH master 2/2] pwm: imx: enable clocks during PWM register accesses Ahmad Fatoum
2023-09-05 11:02   ` Marco Felsch
2023-09-06 12:42   ` Bastian Krause [this message]
2023-09-05 11:03 ` [PATCH master 1/2] clk: implement clk_get_optional helper Marco Felsch

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=d2c8ddf2-1e63-490a-859e-adf4806d4047@pengutronix.de \
    --to=bst@pengutronix.de \
    --cc=a.fatoum@pengutronix.de \
    --cc=barebox@lists.infradead.org \
    --cc=mfe@pengutronix.de \
    --cc=ukl@pengutronix.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox