The driver would never call clk_enable() if the PWM channel was already
enabled in bootloader which lead to dump the warning message "the PWM
clock already disabled" when turning off the PWM channel.
Add atmel_pwm_enable_clk_if_on() in probe function to enable clock if
the PWM channel was already enabled in bootloader.
Signed-off-by: Guiting Shen <[email protected]>
---
drivers/pwm/pwm-atmel.c | 32 ++++++++++++++++++++++++++++++++
1 file changed, 32 insertions(+)
diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
index cdbc23649032..f8f1fbb8732d 100644
--- a/drivers/pwm/pwm-atmel.c
+++ b/drivers/pwm/pwm-atmel.c
@@ -464,6 +464,31 @@ static const struct of_device_id atmel_pwm_dt_ids[] = {
};
MODULE_DEVICE_TABLE(of, atmel_pwm_dt_ids);
+static int atmel_pwm_enable_clk_if_on(struct atmel_pwm_chip *atmel_pwm)
+{
+ unsigned int i;
+ int err;
+ u32 sr;
+
+ sr = atmel_pwm_readl(atmel_pwm, PWM_SR);
+ if (!sr)
+ return 0;
+
+ for (i = 0; i < atmel_pwm->chip.npwm; i++) {
+ if (!(sr & (1 << i)))
+ continue;
+
+ err = clk_enable(atmel_pwm->clk);
+ if (err) {
+ dev_err(atmel_pwm->chip.dev,
+ "failed to enable clock: %pe\n", ERR_PTR(err));
+ return err;
+ }
+ }
+
+ return 0;
+}
+
static int atmel_pwm_probe(struct platform_device *pdev)
{
struct atmel_pwm_chip *atmel_pwm;
@@ -504,8 +529,15 @@ static int atmel_pwm_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, atmel_pwm);
+ ret = atmel_pwm_enable_clk_if_on(atmel_pwm);
+ if (ret < 0)
+ goto remove_pwmchip;
+
return ret;
+remove_pwmchip:
+ pwmchip_remove(&atmel_pwm->chip);
+
unprepare_clk:
clk_unprepare(atmel_pwm->clk);
return ret;
--
2.25.1
Hello,
On Wed, Jul 12, 2023 at 04:09:05AM +0800, Guiting Shen wrote:
> +static int atmel_pwm_enable_clk_if_on(struct atmel_pwm_chip *atmel_pwm)
> +{
> + unsigned int i;
> + int err;
> + u32 sr;
> +
> + sr = atmel_pwm_readl(atmel_pwm, PWM_SR);
> + if (!sr)
> + return 0;
> +
> + for (i = 0; i < atmel_pwm->chip.npwm; i++) {
> + if (!(sr & (1 << i)))
> + continue;
> +
> + err = clk_enable(atmel_pwm->clk);
> + if (err) {
> + dev_err(atmel_pwm->chip.dev,
> + "failed to enable clock: %pe\n", ERR_PTR(err));
Here you leak possibly a few enables. While it's not likely that the
(say) third enable goes wrong, it's also not that hard to handle?!
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
On Wed, Jul 12, 2023 at 04:30:17AM GMT+8, Uwe Kleine-König wrote:
> Hello,
>
> On Wed, Jul 12, 2023 at 04:09:05AM +0800, Guiting Shen wrote:
>> +static int atmel_pwm_enable_clk_if_on(struct atmel_pwm_chip *atmel_pwm)
>> +{
>> + unsigned int i;
>> + int err;
>> + u32 sr;
>> +
>> + sr = atmel_pwm_readl(atmel_pwm, PWM_SR);
>> + if (!sr)
>> + return 0;
>> +
>> + for (i = 0; i < atmel_pwm->chip.npwm; i++) {
>> + if (!(sr & (1 << i)))
>> + continue;
>> +
>> + err = clk_enable(atmel_pwm->clk);
>> + if (err) {
>> + dev_err(atmel_pwm->chip.dev,
>> + "failed to enable clock: %pe\n", ERR_PTR(err));
>
> Here you leak possibly a few enables. While it's not likely that the
> (say) third enable goes wrong, it's also not that hard to handle?!
The driver used the enable_count member of struct clk_core to count the
PWM channels(4 channels). It will enable hardware clock only when one of
the PWM channels becomed on from all PWM channels off which maybe return
error. And in second/third/fourth times to clk_enable(), it just
increased the enable_count of struct clk_core which would never return
error.
It maybe confused at first time to view the code.
Do it need to add something like that: ?
for (i = 0; i < atmel_pwm->chip.npwm; i++) {
if (!(sr & (1 << i)))
continue;
err = clk_enable(atmel_pwm->clk);
if (err) {
dev_err(atmel_pwm->chip.dev,
"failed to enable clock: %pe\n", ERR_PTR(err));
for (i = 0; i < cnt; i++)
clk_disable(atmel_pwm->clk);
return err;
}
cnt++;
}
--
Regards,
Guiting Shen
On Wed, Jul 12, 2023 at 09:45:08AM +0800, Guiting Shen wrote:
> On Wed, Jul 12, 2023 at 04:30:17AM GMT+8, Uwe Kleine-K?nig wrote:
> > Hello,
> >
> > On Wed, Jul 12, 2023 at 04:09:05AM +0800, Guiting Shen wrote:
> >> +static int atmel_pwm_enable_clk_if_on(struct atmel_pwm_chip *atmel_pwm)
> >> +{
> >> + unsigned int i;
> >> + int err;
> >> + u32 sr;
> >> +
> >> + sr = atmel_pwm_readl(atmel_pwm, PWM_SR);
> >> + if (!sr)
> >> + return 0;
> >> +
> >> + for (i = 0; i < atmel_pwm->chip.npwm; i++) {
> >> + if (!(sr & (1 << i)))
> >> + continue;
> >> +
> >> + err = clk_enable(atmel_pwm->clk);
> >> + if (err) {
> >> + dev_err(atmel_pwm->chip.dev,
> >> + "failed to enable clock: %pe\n", ERR_PTR(err));
> >
> > Here you leak possibly a few enables. While it's not likely that the
> > (say) third enable goes wrong, it's also not that hard to handle?!
>
> The driver used the enable_count member of struct clk_core to count the
> PWM channels(4 channels). It will enable hardware clock only when one of
> the PWM channels becomed on from all PWM channels off which maybe return
> error. And in second/third/fourth times to clk_enable(), it just
> increased the enable_count of struct clk_core which would never return
> error.
>
> It maybe confused at first time to view the code.
> Do it need to add something like that: ?
>
> for (i = 0; i < atmel_pwm->chip.npwm; i++) {
> if (!(sr & (1 << i)))
> continue;
>
> err = clk_enable(atmel_pwm->clk);
> if (err) {
> dev_err(atmel_pwm->chip.dev,
> "failed to enable clock: %pe\n", ERR_PTR(err));
>
> for (i = 0; i < cnt; i++)
> clk_disable(atmel_pwm->clk);
> return err;
> }
> cnt++;
> }
Yes, that approx. what I thought of. Maybe also mention i in the error
message? (So something like:
"failed to enable clock for pwm #%d: %pe\n"
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
On Wed, Jul 12, 2023 at 09:45:08AM +0800, Guiting Shen wrote:
> On Wed, Jul 12, 2023 at 04:30:17AM GMT+8, Uwe Kleine-König wrote:
> > Hello,
> >
> > On Wed, Jul 12, 2023 at 04:09:05AM +0800, Guiting Shen wrote:
> >> +static int atmel_pwm_enable_clk_if_on(struct atmel_pwm_chip *atmel_pwm)
> >> +{
> >> + unsigned int i;
> >> + int err;
> >> + u32 sr;
> >> +
> >> + sr = atmel_pwm_readl(atmel_pwm, PWM_SR);
> >> + if (!sr)
> >> + return 0;
> >> +
> >> + for (i = 0; i < atmel_pwm->chip.npwm; i++) {
> >> + if (!(sr & (1 << i)))
> >> + continue;
> >> +
> >> + err = clk_enable(atmel_pwm->clk);
> >> + if (err) {
> >> + dev_err(atmel_pwm->chip.dev,
> >> + "failed to enable clock: %pe\n", ERR_PTR(err));
> >
> > Here you leak possibly a few enables. While it's not likely that the
> > (say) third enable goes wrong, it's also not that hard to handle?!
>
> The driver used the enable_count member of struct clk_core to count the
> PWM channels(4 channels). It will enable hardware clock only when one of
> the PWM channels becomed on from all PWM channels off which maybe return
> error. And in second/third/fourth times to clk_enable(), it just
> increased the enable_count of struct clk_core which would never return
> error.
>
> It maybe confused at first time to view the code.
> Do it need to add something like that: ?
>
> for (i = 0; i < atmel_pwm->chip.npwm; i++) {
> if (!(sr & (1 << i)))
> continue;
>
> err = clk_enable(atmel_pwm->clk);
> if (err) {
> dev_err(atmel_pwm->chip.dev,
> "failed to enable clock: %pe\n", ERR_PTR(err));
>
> for (i = 0; i < cnt; i++)
> clk_disable(atmel_pwm->clk);
> return err;
> }
> cnt++;
You can also achieve this by decrementing i back to zero, that way you
avoid the additional variable and you get a more natural unwinding of
what you did before.
So something like:
while (i--)
clk_disable(atmel_pwm->clk);
should do the same thing.
Thierry
On Wed, Jul 12, 2023 at 02:31:43PM +0200, Thierry Reding wrote:
> On Wed, Jul 12, 2023 at 09:45:08AM +0800, Guiting Shen wrote:
> > On Wed, Jul 12, 2023 at 04:30:17AM GMT+8, Uwe Kleine-K?nig wrote:
> > > Hello,
> > >
> > > On Wed, Jul 12, 2023 at 04:09:05AM +0800, Guiting Shen wrote:
> > >> +static int atmel_pwm_enable_clk_if_on(struct atmel_pwm_chip *atmel_pwm)
> > >> +{
> > >> + unsigned int i;
> > >> + int err;
> > >> + u32 sr;
> > >> +
> > >> + sr = atmel_pwm_readl(atmel_pwm, PWM_SR);
> > >> + if (!sr)
> > >> + return 0;
> > >> +
> > >> + for (i = 0; i < atmel_pwm->chip.npwm; i++) {
> > >> + if (!(sr & (1 << i)))
> > >> + continue;
> > >> +
> > >> + err = clk_enable(atmel_pwm->clk);
> > >> + if (err) {
> > >> + dev_err(atmel_pwm->chip.dev,
> > >> + "failed to enable clock: %pe\n", ERR_PTR(err));
> > >
> > > Here you leak possibly a few enables. While it's not likely that the
> > > (say) third enable goes wrong, it's also not that hard to handle?!
> >
> > The driver used the enable_count member of struct clk_core to count the
> > PWM channels(4 channels). It will enable hardware clock only when one of
> > the PWM channels becomed on from all PWM channels off which maybe return
> > error. And in second/third/fourth times to clk_enable(), it just
> > increased the enable_count of struct clk_core which would never return
> > error.
> >
> > It maybe confused at first time to view the code.
> > Do it need to add something like that: ?
> >
> > for (i = 0; i < atmel_pwm->chip.npwm; i++) {
> > if (!(sr & (1 << i)))
> > continue;
> >
> > err = clk_enable(atmel_pwm->clk);
> > if (err) {
> > dev_err(atmel_pwm->chip.dev,
> > "failed to enable clock: %pe\n", ERR_PTR(err));
> >
> > for (i = 0; i < cnt; i++)
> > clk_disable(atmel_pwm->clk);
> > return err;
> > }
> > cnt++;
>
> You can also achieve this by decrementing i back to zero, that way you
> avoid the additional variable and you get a more natural unwinding of
> what you did before.
>
> So something like:
>
> while (i--)
> clk_disable(atmel_pwm->clk);
You'd need something like:
while (i--) {
if (!(sr & (1 << i)))
continue;
clk_disable(atmel_pwm->clk);
}
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |