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 | 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
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 <[email protected]>
Reviewed-by: Claudiu Beznea <[email protected]>
> ---
> 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;
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 <[email protected]>
> ---
> 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
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 <[email protected]>
>> ---
>> 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
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/ |
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 <[email protected]>