linux-kernel - Re: [PATCH 1/1] Atmel-PWM: Fixed the PWM channel enable/disable issue which will cause the PWM output stop working after reenable. THe PWM channel status register is read to ensure the PWM channel is really enabled or disabled.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Fri, 12 Jun 2015 13:45:56 +0200
From:	Thierry Reding <thierry.reding@...il.com>
To:	robert yang <hachyang@...il.com>
Cc:	linux-pwm@...r.kernel.org, linux-kernel@...r.kernel.org,
	Nicolas Ferre <nicolas.ferre@...el.com>, ryang <ryang@...h.com>
Subject: Re: [PATCH 1/1] Atmel-PWM: Fixed the PWM channel enable/disable
 issue which will cause the PWM output stop working after reenable. THe PWM
 channel status register is read to ensure  the PWM channel is really enabled
 or disabled.

On Wed, Oct 01, 2014 at 03:08:30PM -0600, robert yang wrote:
> From: ryang <ryang@...h.com>
> 
> Signed-off-by: ryang <ryang@...h.com>
> ---
>  drivers/pwm/pwm-atmel.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)

Seems like I never got around to review this. Sorry for that.

First, you need to change the commit subject and message. The first line
should be a short description of what the patch does, something like
this:

	pwm: atmel: Ensure channel is enabled or disabled

Followed by a more verbose description of the change. What I don't
understand here is under what circumstances it can happen that the
channel isn't getting enabled. Is this because some error happened and
therefore the register needs to be written again? Or is it just that we
need to wait for some time for the PWM to become enabled and we use the
PWM enable register for that?

> diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
> index 6e700a5..bab933a 100644
> --- a/drivers/pwm/pwm-atmel.c
> +++ b/drivers/pwm/pwm-atmel.c
> @@ -237,7 +237,10 @@ static int atmel_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
>  	}
>  
>  	atmel_pwm_writel(atmel_pwm, PWM_ENA, 1 << pwm->hwpwm);
> -
> +	while (!(atmel_pwm_readl(atmel_pwm, PWM_SR) & (1 << pwm->hwpwm))) {
> +		atmel_pwm_writel(atmel_pwm, PWM_ENA, 1 << pwm->hwpwm);
> +		cpu_relax();
> +	}

So this indicates that the PWM_ENA register is used to enable multiple
PWM channels. Given that there is a PWM_DIS register too, I suspect that
this is a write-only register, so writing zeroes to an enable bit
doesn't actually cause the corresponding PWM to be disabled. That would
have been a nice explanation for why the original code could break.

>  	return 0;
>  }
>  
> @@ -246,7 +249,10 @@ static void atmel_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>  	struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip);
>  
>  	atmel_pwm_writel(atmel_pwm, PWM_DIS, 1 << pwm->hwpwm);
> -
> +	while (atmel_pwm_readl(atmel_pwm, PWM_SR) & (1 << pwm->hwpwm)) {
> +		atmel_pwm_writel(atmel_pwm, PWM_DIS, 1 << pwm->hwpwm);
> +		cpu_relax();
> +	}
>  	clk_disable(atmel_pwm->clk);
>  }
>  

Same here. I don't really understand why this change is necessary. Maybe
you can provide a more detailed description of the error you're seeing.
Also adding Nicolas to Cc, perhaps he can help find out what the issue
is with the current code.

Thierry

Content of type "application/pgp-signature" skipped