[PATCH v2] pwm: rockchip: Keep enabled PWMs running while probing

[PATCH v2] pwm: rockchip: Keep enabled PWMs running while probing

[Simon South]

Following commit cfc4c189bc70 ("pwm: Read initial hardware state at
request time") the Rockchip PWM driver can no longer assume a device's
pwm_state structure has been populated after a call to pwmchip_add().
Consequently, the test in rockchip_pwm_probe() intended to prevent the
driver from stopping PWM devices already enabled by the bootloader no
longer functions reliably and this can lead to the kernel hanging
during startup, particularly on devices like the Pinebook Pro that use
a PWM-controlled backlight for their display.

Avoid this by querying the device directly at probe time to determine
whether or not it is enabled.

Fixes: cfc4c189bc70 ("pwm: Read initial hardware state at request time")
Signed-off-by: Simon South <simon@simonsouth.net>
---
 drivers/pwm/pwm-rockchip.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
index eb8c9cb645a6..098e94335cb5 100644
--- a/drivers/pwm/pwm-rockchip.c
+++ b/drivers/pwm/pwm-rockchip.c
@@ -288,6 +288,7 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
 	const struct of_device_id *id;
 	struct rockchip_pwm_chip *pc;
 	struct resource *r;
+	u32 enable_conf, ctrl;
 	int ret, count;
 
 	id = of_match_device(rockchip_pwm_dt_ids, &pdev->dev);
@@ -362,7 +363,9 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
 	}
 
 	/* Keep the PWM clk enabled if the PWM appears to be up and running. */
-	if (!pwm_is_enabled(pc->chip.pwms))
+	enable_conf = pc->data->enable_conf;
+	ctrl = readl_relaxed(pc->base + pc->data->regs.ctrl);
+	if ((ctrl & enable_conf) != enable_conf)
 		clk_disable(pc->clk);
 
 	return 0;
-- 
2.28.0

Re: [PATCH v2] pwm: rockchip: Keep enabled PWMs running while probing

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2756 bytes --]

Hello,

On Sat, Sep 19, 2020 at 03:33:06PM -0400, Simon South wrote:
> Following commit cfc4c189bc70 ("pwm: Read initial hardware state at
> request time") the Rockchip PWM driver can no longer assume a device's
> pwm_state structure has been populated after a call to pwmchip_add().
> Consequently, the test in rockchip_pwm_probe() intended to prevent the
> driver from stopping PWM devices already enabled by the bootloader no
> longer functions reliably and this can lead to the kernel hanging
> during startup, particularly on devices like the Pinebook Pro that use
> a PWM-controlled backlight for their display.
> 
> Avoid this by querying the device directly at probe time to determine
> whether or not it is enabled.
> 
> Fixes: cfc4c189bc70 ("pwm: Read initial hardware state at request time")
> Signed-off-by: Simon South <simon@simonsouth.net>
> ---
>  drivers/pwm/pwm-rockchip.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
> index eb8c9cb645a6..098e94335cb5 100644
> --- a/drivers/pwm/pwm-rockchip.c
> +++ b/drivers/pwm/pwm-rockchip.c
> @@ -288,6 +288,7 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
>  	const struct of_device_id *id;
>  	struct rockchip_pwm_chip *pc;
>  	struct resource *r;
> +	u32 enable_conf, ctrl;
>  	int ret, count;
>  
>  	id = of_match_device(rockchip_pwm_dt_ids, &pdev->dev);
> @@ -362,7 +363,9 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
>  	}
>  
>  	/* Keep the PWM clk enabled if the PWM appears to be up and running. */
> -	if (!pwm_is_enabled(pc->chip.pwms))
> +	enable_conf = pc->data->enable_conf;
> +	ctrl = readl_relaxed(pc->base + pc->data->regs.ctrl);
> +	if ((ctrl & enable_conf) != enable_conf)
>  		clk_disable(pc->clk);

In my book a pwm driver should never call pwm_get_state() (or its
wrappers).

Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

I looked at the other drivers for similar problems. I found a few
issues, but they are all independently of cfc4c189bc70:

 - pwm-lpc18xx-sct.c does some allocation that the .request
   callback depends on. pwm-sti.c does allocation that the irq handler
   depends on quite late.
 - some drivers modify the hardware after pwmchip_add()
   [pwm-fsl-ftm.c, pwm-hibvt.c, pwm-lpc18xx-sct.c, pwm-lpc32xx.c,
   pwm-mtk-disp.c, pwm-mxs.c]
 - pwm-lpss.c, pwm-pca9685.c, pwm-tiehrpwm.c and cpwm-tiecap.c do some
   pm_runtime stuff which should better be done before pwmchip_add()?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2] pwm: rockchip: Keep enabled PWMs running while probing

[Heiko Stübner]

Am Samstag, 19. September 2020, 21:33:06 CEST schrieb Simon South:
> Following commit cfc4c189bc70 ("pwm: Read initial hardware state at
> request time") the Rockchip PWM driver can no longer assume a device's
> pwm_state structure has been populated after a call to pwmchip_add().
> Consequently, the test in rockchip_pwm_probe() intended to prevent the
> driver from stopping PWM devices already enabled by the bootloader no
> longer functions reliably and this can lead to the kernel hanging
> during startup, particularly on devices like the Pinebook Pro that use
> a PWM-controlled backlight for their display.
> 
> Avoid this by querying the device directly at probe time to determine
> whether or not it is enabled.
> 
> Fixes: cfc4c189bc70 ("pwm: Read initial hardware state at request time")
> Signed-off-by: Simon South <simon@simonsouth.net>

Reviewed-by: Heiko Stuebner <heiko@sntech.de>

> ---
>  drivers/pwm/pwm-rockchip.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
> index eb8c9cb645a6..098e94335cb5 100644
> --- a/drivers/pwm/pwm-rockchip.c
> +++ b/drivers/pwm/pwm-rockchip.c
> @@ -288,6 +288,7 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
>  	const struct of_device_id *id;
>  	struct rockchip_pwm_chip *pc;
>  	struct resource *r;
> +	u32 enable_conf, ctrl;
>  	int ret, count;
>  
>  	id = of_match_device(rockchip_pwm_dt_ids, &pdev->dev);
> @@ -362,7 +363,9 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
>  	}
>  
>  	/* Keep the PWM clk enabled if the PWM appears to be up and running. */
> -	if (!pwm_is_enabled(pc->chip.pwms))
> +	enable_conf = pc->data->enable_conf;
> +	ctrl = readl_relaxed(pc->base + pc->data->regs.ctrl);
> +	if ((ctrl & enable_conf) != enable_conf)
>  		clk_disable(pc->clk);
>  
>  	return 0;
> 

Re: [PATCH v2] pwm: rockchip: Keep enabled PWMs running while probing

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 992 bytes --]

On Sat, Sep 19, 2020 at 03:33:06PM -0400, Simon South wrote:
> Following commit cfc4c189bc70 ("pwm: Read initial hardware state at
> request time") the Rockchip PWM driver can no longer assume a device's
> pwm_state structure has been populated after a call to pwmchip_add().
> Consequently, the test in rockchip_pwm_probe() intended to prevent the
> driver from stopping PWM devices already enabled by the bootloader no
> longer functions reliably and this can lead to the kernel hanging
> during startup, particularly on devices like the Pinebook Pro that use
> a PWM-controlled backlight for their display.
> 
> Avoid this by querying the device directly at probe time to determine
> whether or not it is enabled.
> 
> Fixes: cfc4c189bc70 ("pwm: Read initial hardware state at request time")
> Signed-off-by: Simon South <simon@simonsouth.net>
> ---
>  drivers/pwm/pwm-rockchip.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)

Applied, thanks.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2] pwm: rockchip: Keep enabled PWMs running while probing

[Trent Piepho]

On Saturday, September 19, 2020 12:33:06 PM PST Simon South wrote:
> Following commit cfc4c189bc70 ("pwm: Read initial hardware state at
> request time") the Rockchip PWM driver can no longer assume a device's
> pwm_state structure has been populated after a call to pwmchip_add().
> Consequently, the test in rockchip_pwm_probe() intended to prevent the

> 
> @@ -362,7 +363,9 @@ static int rockchip_pwm_probe(struct platform_device 
*pdev)
...
        ret = pwmchip_add(&pc->chip);
...
>  	}
>  
>  	/* Keep the PWM clk enabled if the PWM appears to be up and 
running. */
> -	if (!pwm_is_enabled(pc->chip.pwms))
> +	enable_conf = pc->data->enable_conf;
> +	ctrl = readl_relaxed(pc->base + pc->data->regs.ctrl);
> +	if ((ctrl & enable_conf) != enable_conf)
>  		clk_disable(pc->clk);
>  

I came across this while trying to get a PBP working better.  It seems like 
the issue is the driver was calling pwm_is_enabled() without first requesting 
the pwm with pwm_get().  Which wouldn't even be possible normally, how would 
one get the pwm_chip to call pwm_is_enabled on, but the driver already has the 
pointer.

Anyway, it seems like this solution has a race.  Isn't the pwm live and 
requestable as soon as pwmchip_add() returns?  Which would mean that disabling 
the clock here could race with other code requesting and enabling the pwm.

Seems like it would be safer to check the initial state and turn off the clock 
before calling pwmchip_add.

Re: [PATCH v2] pwm: rockchip: Keep enabled PWMs running while probing

[Simon South]

Trent Piepho <tpiepho@gmail.com> writes:
> Anyway, it seems like this solution has a race.  Isn't the pwm live
> and requestable as soon as pwmchip_add() returns?  Which would mean
> that disabling the clock here could race with other code requesting
> and enabling the pwm.

Yes, I think that's true. Glad you caught this.

> Seems like it would be safer to check the initial state and turn off
> the clock before calling pwmchip_add.

Yes. I tested this and it works, but on further consideration it seems
to me the code is actually doing things backwards: Instead of enabling
every PWM's clock and then turning off the clocks for PWMs that are not
themselves enabled, it should enable only the clocks for PWMs that
appear to be in use and leave the remaining clocks in their default
(presumably disabled) state. This should produce the same end result but
without the driver enabling clocks indiscriminately and without creating
a race condition.

I'll follow up with a patch for review that implements this change. I've
tested it as best I can on my own Pinebook Pro; everything works as it
did previously, with the display backlight on, no kernel hang and no
other apparent side effects.

> It seems like the issue is the driver was calling pwm_is_enabled()
> without first requesting the pwm with pwm_get().

This used to work because pwmchip_add() happened to invoke
rockchip_pwm_get_state(), which populates the PWM's pwm_state structure
from which pwm_is_enabled() reads. And I think that's why the code was
written the way it was originally: By waiting until pwmchip_add()
returned the PWM's state could be queried in a convenient manner,
without resorting to peeking at the hardware's registers.

Commit cfc4c189bc70 ("pwm: Read initial hardware state at request time")
changed this and pwmchip_add() no longer has the side effect of
populating the state structure, so the call to pwm_is_enabled() no
longer worked reliably. That's what I fixed with the patch you're
replying to, though now I wish I'd seen the larger issue.

As to why this code is needed in the first place (as I wondered recently
while working on it again), it seems to be a somewhat hacky way of
initializing the enable_count reference counter (see drivers/clk/clk.c)
of the already running clock to 1 instead of 0. This is necessary
because on SoCs like the RK3399 that use only a single clock for each
PWM, the driver treats the "APB" and "bus" clocks as referring to the
same physical device ("pc->pclk = pc->clk"), and without it functions
like rockchip_pwm_get_state() that enable the APB clock on entry and
disable it on exit would end up halting the PWM entirely.

-- 
Simon South
simon@simonsouth.net

[PATCH] pwm: rockchip: Eliminate potential race condition when probing

[Simon South]

Commit 48cf973cae33 ("pwm: rockchip: Avoid glitches on already running
PWMs") introduced a potential race condition in rockchip_pwm_probe() by
having it disable the clock of a PWM already registered through a call to
pwmchip_add().

Eliminate this possibility by calling clk_enable() for a probed PWM's clock
only when it appears the PWM itself has already been enabled (by a
bootloader, presumably), instead of always enabling the clock and then
disabling it after registration for non-enabled PWMs.

Fixes: 48cf973cae33 ("pwm: rockchip: Avoid glitches on already running PWMs")
Fixes: 457f74abbed0 ("pwm: rockchip: Keep enabled PWMs running while probing")
Reported-by: Trent Piepho <tpiepho@gmail.com>
Signed-off-by: Simon South <simon@simonsouth.net>
---
 drivers/pwm/pwm-rockchip.c | 45 ++++++++++++++++++++++++++------------
 1 file changed, 31 insertions(+), 14 deletions(-)

diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
index 77c23a2c6d71..7efba1d0adb4 100644
--- a/drivers/pwm/pwm-rockchip.c
+++ b/drivers/pwm/pwm-rockchip.c
@@ -289,6 +289,7 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
 	struct rockchip_pwm_chip *pc;
 	struct resource *r;
 	u32 enable_conf, ctrl;
+	bool enabled;
 	int ret, count;
 
 	id = of_match_device(rockchip_pwm_dt_ids, &pdev->dev);
@@ -299,6 +300,8 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
 	if (!pc)
 		return -ENOMEM;
 
+	pc->data = id->data;
+
 	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	pc->base = devm_ioremap_resource(&pdev->dev, r);
 	if (IS_ERR(pc->base))
@@ -326,21 +329,38 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	ret = clk_prepare_enable(pc->clk);
+	ret = clk_prepare(pc->clk);
 	if (ret) {
-		dev_err(&pdev->dev, "Can't prepare enable bus clk: %d\n", ret);
+		dev_err(&pdev->dev, "Can't prepare bus clk: %d\n", ret);
 		return ret;
 	}
 
+	/*
+	 * If it appears the PWM has already been enabled, perhaps by a
+	 * bootloader, re-enable its clock to increment the clock's enable
+	 * counter and ensure it is kept running (particularly in the case
+	 * where there is no separate APB clock).
+	 */
+	enable_conf = pc->data->enable_conf;
+	ctrl = readl_relaxed(pc->base + pc->data->regs.ctrl);
+	enabled = (ctrl & enable_conf) == enable_conf;
+	if (enabled) {
+		ret = clk_enable(pc->clk);
+		if (ret) {
+			dev_err(&pdev->dev, "Can't re-enable bus clk: %d\n",
+				ret);
+			goto err_clk_prepared;
+		}
+	}
+
 	ret = clk_prepare(pc->pclk);
 	if (ret) {
 		dev_err(&pdev->dev, "Can't prepare APB clk: %d\n", ret);
-		goto err_clk;
+		goto err_clk_enabled;
 	}
 
 	platform_set_drvdata(pdev, pc);
 
-	pc->data = id->data;
 	pc->chip.dev = &pdev->dev;
 	pc->chip.ops = &rockchip_pwm_ops;
 	pc->chip.base = -1;
@@ -355,21 +375,18 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
 	if (ret < 0) {
 		clk_unprepare(pc->clk);
 		dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret);
-		goto err_pclk;
+		goto err_pclk_prepared;
 	}
 
-	/* Keep the PWM clk enabled if the PWM appears to be up and running. */
-	enable_conf = pc->data->enable_conf;
-	ctrl = readl_relaxed(pc->base + pc->data->regs.ctrl);
-	if ((ctrl & enable_conf) != enable_conf)
-		clk_disable(pc->clk);
-
 	return 0;
 
-err_pclk:
+err_pclk_prepared:
 	clk_unprepare(pc->pclk);
-err_clk:
-	clk_disable_unprepare(pc->clk);
+err_clk_enabled:
+	if (enabled)
+		clk_disable(pc->clk);
+err_clk_prepared:
+	clk_unprepare(pc->clk);
 
 	return ret;
 }
-- 
2.29.2

Re: [PATCH] pwm: rockchip: Eliminate potential race condition when probing

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2979 bytes --]

On Sun, Nov 29, 2020 at 07:44:19PM -0500, Simon South wrote:
> Commit 48cf973cae33 ("pwm: rockchip: Avoid glitches on already running
> PWMs") introduced a potential race condition in rockchip_pwm_probe() by
> having it disable the clock of a PWM already registered through a call to
> pwmchip_add().
> 
> Eliminate this possibility by calling clk_enable() for a probed PWM's clock
> only when it appears the PWM itself has already been enabled (by a
> bootloader, presumably), instead of always enabling the clock and then
> disabling it after registration for non-enabled PWMs.
> 
> Fixes: 48cf973cae33 ("pwm: rockchip: Avoid glitches on already running PWMs")
> Fixes: 457f74abbed0 ("pwm: rockchip: Keep enabled PWMs running while probing")
> Reported-by: Trent Piepho <tpiepho@gmail.com>
> Signed-off-by: Simon South <simon@simonsouth.net>
> ---
>  drivers/pwm/pwm-rockchip.c | 45 ++++++++++++++++++++++++++------------
>  1 file changed, 31 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
> index 77c23a2c6d71..7efba1d0adb4 100644
> --- a/drivers/pwm/pwm-rockchip.c
> +++ b/drivers/pwm/pwm-rockchip.c
> @@ -289,6 +289,7 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
>  	struct rockchip_pwm_chip *pc;
>  	struct resource *r;
>  	u32 enable_conf, ctrl;
> +	bool enabled;
>  	int ret, count;
>  
>  	id = of_match_device(rockchip_pwm_dt_ids, &pdev->dev);
> @@ -299,6 +300,8 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
>  	if (!pc)
>  		return -ENOMEM;
>  
> +	pc->data = id->data;
> +
>  	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	pc->base = devm_ioremap_resource(&pdev->dev, r);
>  	if (IS_ERR(pc->base))
> @@ -326,21 +329,38 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> -	ret = clk_prepare_enable(pc->clk);
> +	ret = clk_prepare(pc->clk);
>  	if (ret) {
> -		dev_err(&pdev->dev, "Can't prepare enable bus clk: %d\n", ret);
> +		dev_err(&pdev->dev, "Can't prepare bus clk: %d\n", ret);
>  		return ret;
>  	}
>  
> +	/*
> +	 * If it appears the PWM has already been enabled, perhaps by a
> +	 * bootloader, re-enable its clock to increment the clock's enable
> +	 * counter and ensure it is kept running (particularly in the case
> +	 * where there is no separate APB clock).
> +	 */
> +	enable_conf = pc->data->enable_conf;
> +	ctrl = readl_relaxed(pc->base + pc->data->regs.ctrl);
> +	enabled = (ctrl & enable_conf) == enable_conf;

Given that we don't enable the bus clock before this, is it even safe to
access registers on the bus if the clock is disabled? I've seen a lot of
cases where accesses to an unclocked bus either lead to silent hangs or
very noisy crashes, and I would expect something like that (or something
in between) to happen on Rockchip SoCs.

Have you tested this for cases where the bus clock is initially
disabled?

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] pwm: rockchip: Eliminate potential race condition when probing

[Trent Piepho]

On Thursday, December 10, 2020 9:48:30 AM PST Thierry Reding wrote:
> On Sun, Nov 29, 2020 at 07:44:19PM -0500, Simon South wrote:
> > @@ -326,21 +329,38 @@ static int rockchip_pwm_probe(struct
> > platform_device *pdev)> 
> >  		return ret;
> >  	
> >  	}
> > 
> > -	ret = clk_prepare_enable(pc->clk);
> > +	ret = clk_prepare(pc->clk);
> > 
> >  	if (ret) {
> > 
> > -		dev_err(&pdev->dev, "Can't prepare enable bus clk: %d\n", ret);
> > +		dev_err(&pdev->dev, "Can't prepare bus clk: %d\n", ret);
> > 
> >  		return ret;
> >  	
> >  	}
> > 
> > +	/*
> > +	 * If it appears the PWM has already been enabled, perhaps by a
> > +	 * bootloader, re-enable its clock to increment the clock's enable
> > +	 * counter and ensure it is kept running (particularly in the case
> > +	 * where there is no separate APB clock).
> > +	 */
> > +	enable_conf = pc->data->enable_conf;
> > +	ctrl = readl_relaxed(pc->base + pc->data->regs.ctrl);
> > +	enabled = (ctrl & enable_conf) == enable_conf;
> 
> Given that we don't enable the bus clock before this, is it even safe to
> access registers on the bus if the clock is disabled? I've seen a lot of
> cases where accesses to an unclocked bus either lead to silent hangs or
> very noisy crashes, and I would expect something like that (or something
> in between) to happen on Rockchip SoCs.

I would also assume register access with the clock disabled would hang or
otherwise fail.  There are possibly two clocks, one called "bus clock" and
the other "APB clock".  APB being Advanced Peripheral Bus.  Not the greatest
choice of names.  I assume the APB clock is needed for register access and
the "bus clock" is used to generate the PWM signal and does not need to be
enabled for register access.  Unfortunately the RK3399 docs do not have a
clock diagram for the PWM or include details such as these.

There is a low power mode bit in the control register that disables the PWM
signal's clock.  And which clock does that disabled, the "ABP clock" or the
"bus clock"?  I quote §18.6.4, "the APB bus clock … is gated…"  It's like
they're being intentional ambiguous.

Anyway, from the existing code, it seems clear that pc->pclk needs to be
enabled for register access and pc->clk to generate a signal.  The call to
clk_prepare(pc->pclk) should become clk_prepare_enable(pc->pclk) and moved
to before the enabled_conf check.  Then clk_disable(pc->pclk) afterward.
The existing code will disable pclk even if the PWM is enabled, so unless
that is also a bug, it should be ok to disable pc->pclk after enabling
pc->clk.

Re: [PATCH] pwm: rockchip: Eliminate potential race condition when probing

[Robin Murphy]

On 2020-12-10 21:00, Trent Piepho wrote:
> On Thursday, December 10, 2020 9:48:30 AM PST Thierry Reding wrote:
>> On Sun, Nov 29, 2020 at 07:44:19PM -0500, Simon South wrote:
>>> @@ -326,21 +329,38 @@ static int rockchip_pwm_probe(struct
>>> platform_device *pdev)>
>>>   		return ret;
>>>   	
>>>   	}
>>>
>>> -	ret = clk_prepare_enable(pc->clk);
>>> +	ret = clk_prepare(pc->clk);
>>>
>>>   	if (ret) {
>>>
>>> -		dev_err(&pdev->dev, "Can't prepare enable bus clk: %d\n", ret);
>>> +		dev_err(&pdev->dev, "Can't prepare bus clk: %d\n", ret);
>>>
>>>   		return ret;
>>>   	
>>>   	}
>>>
>>> +	/*
>>> +	 * If it appears the PWM has already been enabled, perhaps by a
>>> +	 * bootloader, re-enable its clock to increment the clock's enable
>>> +	 * counter and ensure it is kept running (particularly in the case
>>> +	 * where there is no separate APB clock).
>>> +	 */
>>> +	enable_conf = pc->data->enable_conf;
>>> +	ctrl = readl_relaxed(pc->base + pc->data->regs.ctrl);
>>> +	enabled = (ctrl & enable_conf) == enable_conf;
>>
>> Given that we don't enable the bus clock before this, is it even safe to
>> access registers on the bus if the clock is disabled? I've seen a lot of
>> cases where accesses to an unclocked bus either lead to silent hangs or
>> very noisy crashes, and I would expect something like that (or something
>> in between) to happen on Rockchip SoCs.
> 
> I would also assume register access with the clock disabled would hang or
> otherwise fail.  There are possibly two clocks, one called "bus clock" and
> the other "APB clock".  APB being Advanced Peripheral Bus.  Not the greatest
> choice of names.  I assume the APB clock is needed for register access and
> the "bus clock" is used to generate the PWM signal and does not need to be
> enabled for register access.  Unfortunately the RK3399 docs do not have a
> clock diagram for the PWM or include details such as these.
> 
> There is a low power mode bit in the control register that disables the PWM
> signal's clock.  And which clock does that disabled, the "ABP clock" or the
> "bus clock"?  I quote §18.6.4, "the APB bus clock … is gated…"  It's like
> they're being intentional ambiguous.

FWIW I think it becomes clear enough if you read the DT binding in 
parallel with the code - if devm_clk_get(&pdev->dev, "pwm") fails, the 
driver falls back to assuming the RK3399-or-earlier case of a single 
unnamed clock, so "Can't get bus clk" is referring specifically to the 
devm_clk_get(&pdev->dev, NULL) call where that clock *is* also the APB 
clock.

Possibly the driver could do with a slightly clearer structure here, but 
compatibility fallbacks are inevitably messy to some degree.

Robin.

> Anyway, from the existing code, it seems clear that pc->pclk needs to be
> enabled for register access and pc->clk to generate a signal.  The call to
> clk_prepare(pc->pclk) should become clk_prepare_enable(pc->pclk) and moved
> to before the enabled_conf check.  Then clk_disable(pc->pclk) afterward.
> The existing code will disable pclk even if the PWM is enabled, so unless
> that is also a bug, it should be ok to disable pc->pclk after enabling
> pc->clk.
> 
> 
> 
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip
> 

Re: [PATCH] pwm: rockchip: Eliminate potential race condition when probing

[Simon South]

Thierry, Trent and Robin, thanks for reviewing this and for your
comments. I think I understand a lot better now how this code needs to
work.

Trent Piepho <tpiepho@gmail.com> writes:
> Anyway, from the existing code, it seems clear that pc->pclk needs to
> be enabled for register access and pc->clk to generate a signal.  The
> call to clk_prepare(pc->pclk) should become
> clk_prepare_enable(pc->pclk) and moved to before the enabled_conf
> check.  Then clk_disable(pc->pclk) afterward.

I'll follow up with a revised set of patches that implement this. (I've
split it into multiple changes as they seem logically distinct and I
find the progression easier to see this way.)

-- 
Simon South
simon@simonsouth.net