* [PATCH v6] pwm: atmel: Enable clk when pwm already enabled in bootloader
@ 2023-07-16 2:06 Guiting Shen
2023-07-16 13:17 ` claudiu beznea
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Guiting Shen @ 2023-07-16 2:06 UTC (permalink / raw)
To: claudiu.beznea
Cc: linux-pwm, alexandre.belloni, Guiting Shen, linux-kernel,
thierry.reding, u.kleine-koenig, linux-arm-kernel
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v6] pwm: atmel: Enable clk when pwm already enabled in bootloader
2023-07-16 2:06 [PATCH v6] pwm: atmel: Enable clk when pwm already enabled in bootloader Guiting Shen
@ 2023-07-16 13:17 ` claudiu beznea
2023-07-20 15:12 ` Thierry Reding
2023-07-21 12:39 ` Thierry Reding
2 siblings, 0 replies; 6+ messages in thread
From: claudiu beznea @ 2023-07-16 13:17 UTC (permalink / raw)
To: Guiting Shen, claudiu.beznea
Cc: linux-pwm, alexandre.belloni, linux-kernel, thierry.reding,
u.kleine-koenig, linux-arm-kernel
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;
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v6] pwm: atmel: Enable clk when pwm already enabled in bootloader
2023-07-16 2:06 [PATCH v6] pwm: atmel: Enable clk when pwm already enabled in bootloader Guiting Shen
2023-07-16 13:17 ` claudiu beznea
@ 2023-07-20 15:12 ` Thierry Reding
2023-07-21 3:23 ` Guiting Shen
2023-07-21 5:49 ` Uwe Kleine-König
2023-07-21 12:39 ` Thierry Reding
2 siblings, 2 replies; 6+ messages in thread
From: Thierry Reding @ 2023-07-20 15:12 UTC (permalink / raw)
To: Guiting Shen
Cc: linux-pwm, alexandre.belloni, linux-kernel, u.kleine-koenig,
claudiu.beznea, linux-arm-kernel
[-- Attachment #1.1: Type: text/plain, Size: 2053 bytes --]
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
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v6] pwm: atmel: Enable clk when pwm already enabled in bootloader
2023-07-20 15:12 ` Thierry Reding
@ 2023-07-21 3:23 ` Guiting Shen
2023-07-21 5:49 ` Uwe Kleine-König
1 sibling, 0 replies; 6+ messages in thread
From: Guiting Shen @ 2023-07-21 3:23 UTC (permalink / raw)
To: Thierry Reding
Cc: linux-pwm, alexandre.belloni, linux-kernel, u.kleine-koenig,
claudiu.beznea, linux-arm-kernel
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v6] pwm: atmel: Enable clk when pwm already enabled in bootloader
2023-07-20 15:12 ` Thierry Reding
2023-07-21 3:23 ` Guiting Shen
@ 2023-07-21 5:49 ` Uwe Kleine-König
1 sibling, 0 replies; 6+ messages in thread
From: Uwe Kleine-König @ 2023-07-21 5:49 UTC (permalink / raw)
To: Thierry Reding
Cc: linux-pwm, alexandre.belloni, Guiting Shen, linux-kernel,
claudiu.beznea, linux-arm-kernel
[-- Attachment #1.1: Type: text/plain, Size: 1251 bytes --]
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/ |
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v6] pwm: atmel: Enable clk when pwm already enabled in bootloader
2023-07-16 2:06 [PATCH v6] pwm: atmel: Enable clk when pwm already enabled in bootloader Guiting Shen
2023-07-16 13:17 ` claudiu beznea
2023-07-20 15:12 ` Thierry Reding
@ 2023-07-21 12:39 ` Thierry Reding
2 siblings, 0 replies; 6+ messages in thread
From: Thierry Reding @ 2023-07-21 12:39 UTC (permalink / raw)
To: claudiu.beznea, Guiting Shen
Cc: linux-pwm, alexandre.belloni, linux-kernel, u.kleine-koenig,
linux-arm-kernel
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>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-07-21 12:39 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-07-16 2:06 [PATCH v6] pwm: atmel: Enable clk when pwm already enabled in bootloader Guiting Shen
2023-07-16 13:17 ` claudiu beznea
2023-07-20 15:12 ` Thierry Reding
2023-07-21 3:23 ` Guiting Shen
2023-07-21 5:49 ` Uwe Kleine-König
2023-07-21 12:39 ` Thierry Reding
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).