[v6] pwm: atmel: Enable clk when pwm already enabled in bootloader

[PATCH v6] pwm: atmel: Enable clk when pwm already enabled in bootloader

Posted by Guiting Shen 2 years, 6 months ago

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 <aarongt.shen@gmail.com>
---
 drivers/pwm/pwm-atmel.c | 47 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 45 insertions(+), 2 deletions(-)

diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
index cdbc23649032..fc89282db645 100644
--- a/drivers/pwm/pwm-atmel.c
+++ b/drivers/pwm/pwm-atmel.c
@@ -36,7 +36,7 @@
 #define PWM_SR			0x0C
 #define PWM_ISR			0x1C
 /* Bit field in SR */
-#define PWM_SR_ALL_CH_ON	0x0F
+#define PWM_SR_ALL_CH_MASK	0x0F
 
 /* The following register is PWM channel related registers */
 #define PWM_CH_REG_OFFSET	0x200
@@ -464,6 +464,42 @@ 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, bool on)
+{
+	unsigned int i, cnt = 0;
+	int ret = 0;
+	u32 sr;
+
+	sr = atmel_pwm_readl(atmel_pwm, PWM_SR) & PWM_SR_ALL_CH_MASK;
+	if (!sr)
+		return 0;
+
+	cnt = bitmap_weight((unsigned long *)&sr, atmel_pwm->chip.npwm);
+
+	if (!on)
+		goto disable_clk;
+
+	for (i = 0; i < cnt; i++) {
+		ret = clk_enable(atmel_pwm->clk);
+		if (ret) {
+			dev_err(atmel_pwm->chip.dev,
+				"failed to enable clock for pwm %pe\n",
+				ERR_PTR(ret));
+
+			cnt = i;
+			goto disable_clk;
+		}
+	}
+
+	return 0;
+
+disable_clk:
+	while (cnt--)
+		clk_disable(atmel_pwm->clk);
+
+	return ret;
+}
+
 static int atmel_pwm_probe(struct platform_device *pdev)
 {
 	struct atmel_pwm_chip *atmel_pwm;
@@ -496,16 +532,23 @@ static int atmel_pwm_probe(struct platform_device *pdev)
 	atmel_pwm->chip.ops = &atmel_pwm_ops;
 	atmel_pwm->chip.npwm = 4;
 
+	ret = atmel_pwm_enable_clk_if_on(atmel_pwm, true);
+	if (ret < 0)
+		goto unprepare_clk;
+
 	ret = pwmchip_add(&atmel_pwm->chip);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "failed to add PWM chip %d\n", ret);
-		goto unprepare_clk;
+		goto disable_clk;
 	}
 
 	platform_set_drvdata(pdev, atmel_pwm);
 
 	return ret;
 
+disable_clk:
+	atmel_pwm_enable_clk_if_on(atmel_pwm, false);
+
 unprepare_clk:
 	clk_unprepare(atmel_pwm->clk);
 	return ret;
-- 
2.25.1

Re: [PATCH v6] pwm: atmel: Enable clk when pwm already enabled in bootloader

Posted by Thierry Reding 2 years, 6 months ago

On Sun, 16 Jul 2023 10:06:52 +0800, Guiting Shen wrote:
> 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.
> 
> [...]

Applied, thanks!

[1/1] pwm: atmel: Enable clk when pwm already enabled in bootloader
      commit: 435ed5851458084b45f42df8689536d5f3d0e126

Best regards,
-- 
Thierry Reding <thierry.reding@gmail.com>

Re: [PATCH v6] pwm: atmel: Enable clk when pwm already enabled in bootloader

Posted by Thierry Reding 2 years, 6 months ago

On Sun, Jul 16, 2023 at 10:06:52AM +0800, Guiting Shen wrote:
> 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 <aarongt.shen@gmail.com>
> ---
>  drivers/pwm/pwm-atmel.c | 47 +++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 45 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
> index cdbc23649032..fc89282db645 100644
> --- a/drivers/pwm/pwm-atmel.c
> +++ b/drivers/pwm/pwm-atmel.c
> @@ -36,7 +36,7 @@
>  #define PWM_SR			0x0C
>  #define PWM_ISR			0x1C
>  /* Bit field in SR */
> -#define PWM_SR_ALL_CH_ON	0x0F
> +#define PWM_SR_ALL_CH_MASK	0x0F
>  
>  /* The following register is PWM channel related registers */
>  #define PWM_CH_REG_OFFSET	0x200
> @@ -464,6 +464,42 @@ 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, bool on)
> +{
> +	unsigned int i, cnt = 0;
> +	int ret = 0;
> +	u32 sr;
> +
> +	sr = atmel_pwm_readl(atmel_pwm, PWM_SR) & PWM_SR_ALL_CH_MASK;
> +	if (!sr)
> +		return 0;
> +
> +	cnt = bitmap_weight((unsigned long *)&sr, atmel_pwm->chip.npwm);

Tiny nit here: not sure if that cast is safe to do. You've got a 32-bit
variable, but if you cast &sr to unsigned long * on a 64-bit machine it
would cause hweight64() to get called and that would then read 64 bits
from a 32-bit variable. This probably works most of the time because we
don't read any of the upper bits, but it is strictly an illegal access
and could be unaligned as well.

Should we just turn sr into an unsigned long to be safe here? No need to
resend, I can make that change when I apply.

Thierry

Re: [PATCH v6] pwm: atmel: Enable clk when pwm already enabled in bootloader

Posted by Uwe Kleine-König 2 years, 6 months ago

On Thu, Jul 20, 2023 at 05:12:30PM +0200, Thierry Reding wrote:
> On Sun, Jul 16, 2023 at 10:06:52AM +0800, Guiting Shen wrote:
> > +static int atmel_pwm_enable_clk_if_on(struct atmel_pwm_chip *atmel_pwm, bool on)
> > +{
> > +	unsigned int i, cnt = 0;
> > +	int ret = 0;
> > +	u32 sr;
> > +
> > +	sr = atmel_pwm_readl(atmel_pwm, PWM_SR) & PWM_SR_ALL_CH_MASK;
> > +	if (!sr)
> > +		return 0;
> > +
> > +	cnt = bitmap_weight((unsigned long *)&sr, atmel_pwm->chip.npwm);
> 
> Tiny nit here: not sure if that cast is safe to do. You've got a 32-bit
> variable, but if you cast &sr to unsigned long * on a 64-bit machine it
> would cause hweight64() to get called and that would then read 64 bits
> from a 32-bit variable. This probably works most of the time because we
> don't read any of the upper bits, but it is strictly an illegal access
> and could be unaligned as well.

While relevance of BE systems ceases slowly, such a machine would
evaluate the wrong bits.
 
> Should we just turn sr into an unsigned long to be safe here?

yes please.

Best regards
Uwe

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

Re: [PATCH v6] pwm: atmel: Enable clk when pwm already enabled in bootloader

Posted by Guiting Shen 2 years, 6 months ago

On Thu, Jul 20, 2023 at 23:12:30PM GMT+8, Thierry Reding wrote:
> On Sun, Jul 16, 2023 at 10:06:52AM +0800, Guiting Shen wrote:
>> 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 <aarongt.shen@gmail.com>
>> ---
>>  drivers/pwm/pwm-atmel.c | 47 +++++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 45 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
>> index cdbc23649032..fc89282db645 100644
>> --- a/drivers/pwm/pwm-atmel.c
>> +++ b/drivers/pwm/pwm-atmel.c
>> @@ -36,7 +36,7 @@
>>  #define PWM_SR			0x0C
>>  #define PWM_ISR			0x1C
>>  /* Bit field in SR */
>> -#define PWM_SR_ALL_CH_ON	0x0F
>> +#define PWM_SR_ALL_CH_MASK	0x0F
>>  
>>  /* The following register is PWM channel related registers */
>>  #define PWM_CH_REG_OFFSET	0x200
>> @@ -464,6 +464,42 @@ 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, bool on)
>> +{
>> +	unsigned int i, cnt = 0;
>> +	int ret = 0;
>> +	u32 sr;
>> +
>> +	sr = atmel_pwm_readl(atmel_pwm, PWM_SR) & PWM_SR_ALL_CH_MASK;
>> +	if (!sr)
>> +		return 0;
>> +
>> +	cnt = bitmap_weight((unsigned long *)&sr, atmel_pwm->chip.npwm);
> 
> Tiny nit here: not sure if that cast is safe to do. You've got a 32-bit
> variable, but if you cast &sr to unsigned long * on a 64-bit machine it
> would cause hweight64() to get called and that would then read 64 bits
> from a 32-bit variable. This probably works most of the time because we
> don't read any of the upper bits, but it is strictly an illegal access
> and could be unaligned as well.

I found that the CONFIG_PWM_ATMEL was only in arch/arm/configs directory
which seems that the pwm-atmel driver only run in 32-bit soc machine.

> Should we just turn sr into an unsigned long to be safe here? No need to
> resend, I can make that change when I apply.

Yes, It is safer by doing this. Thank you.

-- 
Regards,
Guiting Shen

Re: [PATCH v6] pwm: atmel: Enable clk when pwm already enabled in bootloader

Posted by claudiu beznea 2 years, 6 months ago

On 16.07.2023 05:06, Guiting Shen wrote:
> 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 <aarongt.shen@gmail.com>

Reviewed-by: Claudiu Beznea <claudiu.beznea@tuxon.dev>

> ---
>   drivers/pwm/pwm-atmel.c | 47 +++++++++++++++++++++++++++++++++++++++--
>   1 file changed, 45 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
> index cdbc23649032..fc89282db645 100644
> --- a/drivers/pwm/pwm-atmel.c
> +++ b/drivers/pwm/pwm-atmel.c
> @@ -36,7 +36,7 @@
>   #define PWM_SR			0x0C
>   #define PWM_ISR			0x1C
>   /* Bit field in SR */
> -#define PWM_SR_ALL_CH_ON	0x0F
> +#define PWM_SR_ALL_CH_MASK	0x0F
>   
>   /* The following register is PWM channel related registers */
>   #define PWM_CH_REG_OFFSET	0x200
> @@ -464,6 +464,42 @@ 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, bool on)
> +{
> +	unsigned int i, cnt = 0;
> +	int ret = 0;
> +	u32 sr;
> +
> +	sr = atmel_pwm_readl(atmel_pwm, PWM_SR) & PWM_SR_ALL_CH_MASK;
> +	if (!sr)
> +		return 0;
> +
> +	cnt = bitmap_weight((unsigned long *)&sr, atmel_pwm->chip.npwm);
> +
> +	if (!on)
> +		goto disable_clk;
> +
> +	for (i = 0; i < cnt; i++) {
> +		ret = clk_enable(atmel_pwm->clk);
> +		if (ret) {
> +			dev_err(atmel_pwm->chip.dev,
> +				"failed to enable clock for pwm %pe\n",
> +				ERR_PTR(ret));
> +
> +			cnt = i;
> +			goto disable_clk;
> +		}
> +	}
> +
> +	return 0;
> +
> +disable_clk:
> +	while (cnt--)
> +		clk_disable(atmel_pwm->clk);
> +
> +	return ret;
> +}
> +
>   static int atmel_pwm_probe(struct platform_device *pdev)
>   {
>   	struct atmel_pwm_chip *atmel_pwm;
> @@ -496,16 +532,23 @@ static int atmel_pwm_probe(struct platform_device *pdev)
>   	atmel_pwm->chip.ops = &atmel_pwm_ops;
>   	atmel_pwm->chip.npwm = 4;
>   
> +	ret = atmel_pwm_enable_clk_if_on(atmel_pwm, true);
> +	if (ret < 0)
> +		goto unprepare_clk;
> +
>   	ret = pwmchip_add(&atmel_pwm->chip);
>   	if (ret < 0) {
>   		dev_err(&pdev->dev, "failed to add PWM chip %d\n", ret);
> -		goto unprepare_clk;
> +		goto disable_clk;
>   	}
>   
>   	platform_set_drvdata(pdev, atmel_pwm);
>   
>   	return ret;
>   
> +disable_clk:
> +	atmel_pwm_enable_clk_if_on(atmel_pwm, false);
> +
>   unprepare_clk:
>   	clk_unprepare(atmel_pwm->clk);
>   	return ret;