All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] pwm: imx-tpm: reset module on probe
@ 2024-02-01 12:22 ` Viorel Suman (OSS)
  0 siblings, 0 replies; 6+ messages in thread
From: Viorel Suman (OSS) @ 2024-02-01 12:22 UTC (permalink / raw)
  To: Uwe Kleine-König, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
	linux-pwm, linux-arm-kernel, linux-kernel
  Cc: Viorel Suman, Viorel Suman

From: Viorel Suman <viorel.suman@nxp.com>

Reset Timer PWM module on probe so that the module
takes the default state after probe is finished.

Signed-off-by: Viorel Suman <viorel.suman@nxp.com>
Signed-off-by: Florin Leotescu <florin.leotescu@nxp.com>
---
 drivers/pwm/pwm-imx-tpm.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/pwm/pwm-imx-tpm.c b/drivers/pwm/pwm-imx-tpm.c
index 9fc290e647e1..27e6a5342693 100644
--- a/drivers/pwm/pwm-imx-tpm.c
+++ b/drivers/pwm/pwm-imx-tpm.c
@@ -33,6 +33,7 @@
 #define PWM_IMX_TPM_CnV(n)	(0x24 + (n) * 0x8)
 
 #define PWM_IMX_TPM_PARAM_CHAN			GENMASK(7, 0)
+#define PWM_IMX_TPM_GLOBAL_RST			BIT(1)
 
 #define PWM_IMX_TPM_SC_PS			GENMASK(2, 0)
 #define PWM_IMX_TPM_SC_CMOD			GENMASK(4, 3)
@@ -362,6 +363,10 @@ static int pwm_imx_tpm_probe(struct platform_device *pdev)
 	val = readl(tpm->base + PWM_IMX_TPM_PARAM);
 	tpm->chip.npwm = FIELD_GET(PWM_IMX_TPM_PARAM_CHAN, val);
 
+	/* Resets all internal logic and registers */
+	writel(PWM_IMX_TPM_GLOBAL_RST, tpm->base + PWM_IMX_TPM_GLOBAL);
+	writel(0, tpm->base + PWM_IMX_TPM_GLOBAL);
+
 	mutex_init(&tpm->lock);
 
 	ret = devm_pwmchip_add(&pdev->dev, &tpm->chip);
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [PATCH] pwm: imx-tpm: reset module on probe
@ 2024-02-01 12:22 ` Viorel Suman (OSS)
  0 siblings, 0 replies; 6+ messages in thread
From: Viorel Suman (OSS) @ 2024-02-01 12:22 UTC (permalink / raw)
  To: Uwe Kleine-König, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
	linux-pwm, linux-arm-kernel, linux-kernel
  Cc: Viorel Suman, Viorel Suman

From: Viorel Suman <viorel.suman@nxp.com>

Reset Timer PWM module on probe so that the module
takes the default state after probe is finished.

Signed-off-by: Viorel Suman <viorel.suman@nxp.com>
Signed-off-by: Florin Leotescu <florin.leotescu@nxp.com>
---
 drivers/pwm/pwm-imx-tpm.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/pwm/pwm-imx-tpm.c b/drivers/pwm/pwm-imx-tpm.c
index 9fc290e647e1..27e6a5342693 100644
--- a/drivers/pwm/pwm-imx-tpm.c
+++ b/drivers/pwm/pwm-imx-tpm.c
@@ -33,6 +33,7 @@
 #define PWM_IMX_TPM_CnV(n)	(0x24 + (n) * 0x8)
 
 #define PWM_IMX_TPM_PARAM_CHAN			GENMASK(7, 0)
+#define PWM_IMX_TPM_GLOBAL_RST			BIT(1)
 
 #define PWM_IMX_TPM_SC_PS			GENMASK(2, 0)
 #define PWM_IMX_TPM_SC_CMOD			GENMASK(4, 3)
@@ -362,6 +363,10 @@ static int pwm_imx_tpm_probe(struct platform_device *pdev)
 	val = readl(tpm->base + PWM_IMX_TPM_PARAM);
 	tpm->chip.npwm = FIELD_GET(PWM_IMX_TPM_PARAM_CHAN, val);
 
+	/* Resets all internal logic and registers */
+	writel(PWM_IMX_TPM_GLOBAL_RST, tpm->base + PWM_IMX_TPM_GLOBAL);
+	writel(0, tpm->base + PWM_IMX_TPM_GLOBAL);
+
 	mutex_init(&tpm->lock);
 
 	ret = devm_pwmchip_add(&pdev->dev, &tpm->chip);
-- 
2.34.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] pwm: imx-tpm: reset module on probe
  2024-02-01 12:22 ` Viorel Suman (OSS)
@ 2024-02-01 13:38   ` Uwe Kleine-König
  -1 siblings, 0 replies; 6+ messages in thread
From: Uwe Kleine-König @ 2024-02-01 13:38 UTC (permalink / raw)
  To: Viorel Suman (OSS),
	Florin Leotescu, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
	linux-pwm, linux-arm-kernel, linux-kernel, Viorel Suman

[-- Attachment #1: Type: text/plain, Size: 1985 bytes --]

Hello,

On Thu, Feb 01, 2024 at 02:22:42PM +0200, Viorel Suman (OSS) wrote:
> From: Viorel Suman <viorel.suman@nxp.com>
> 
> Reset Timer PWM module on probe so that the module
> takes the default state after probe is finished.
> 
> Signed-off-by: Viorel Suman <viorel.suman@nxp.com>
> Signed-off-by: Florin Leotescu <florin.leotescu@nxp.com>

The order of Signed-off-lines is supposed to be in the order that people
touched the patch during submission. So your S-o-b line should be last
and it should ideally use the email address you use for sending out the
patch.

> ---
>  drivers/pwm/pwm-imx-tpm.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/pwm/pwm-imx-tpm.c b/drivers/pwm/pwm-imx-tpm.c
> index 9fc290e647e1..27e6a5342693 100644
> --- a/drivers/pwm/pwm-imx-tpm.c
> +++ b/drivers/pwm/pwm-imx-tpm.c
> @@ -33,6 +33,7 @@
>  #define PWM_IMX_TPM_CnV(n)	(0x24 + (n) * 0x8)
>  
>  #define PWM_IMX_TPM_PARAM_CHAN			GENMASK(7, 0)
> +#define PWM_IMX_TPM_GLOBAL_RST			BIT(1)
>  
>  #define PWM_IMX_TPM_SC_PS			GENMASK(2, 0)
>  #define PWM_IMX_TPM_SC_CMOD			GENMASK(4, 3)
> @@ -362,6 +363,10 @@ static int pwm_imx_tpm_probe(struct platform_device *pdev)
>  	val = readl(tpm->base + PWM_IMX_TPM_PARAM);
>  	tpm->chip.npwm = FIELD_GET(PWM_IMX_TPM_PARAM_CHAN, val);
>  
> +	/* Resets all internal logic and registers */
> +	writel(PWM_IMX_TPM_GLOBAL_RST, tpm->base + PWM_IMX_TPM_GLOBAL);
> +	writel(0, tpm->base + PWM_IMX_TPM_GLOBAL);
> +

This opposes the use case that the bootloader setup the pwm-backlight to
show a splash screen that is simply taken over by Linux without
flickering, right?

Otherwise the commit log should motivate why "the module takes the
default state" is better than the status quo and what this default state
is.

Best regards
Uwe

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] pwm: imx-tpm: reset module on probe
@ 2024-02-01 13:38   ` Uwe Kleine-König
  0 siblings, 0 replies; 6+ messages in thread
From: Uwe Kleine-König @ 2024-02-01 13:38 UTC (permalink / raw)
  To: Viorel Suman (OSS),
	Florin Leotescu, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
	linux-pwm, linux-arm-kernel, linux-kernel, Viorel Suman


[-- Attachment #1.1: Type: text/plain, Size: 1985 bytes --]

Hello,

On Thu, Feb 01, 2024 at 02:22:42PM +0200, Viorel Suman (OSS) wrote:
> From: Viorel Suman <viorel.suman@nxp.com>
> 
> Reset Timer PWM module on probe so that the module
> takes the default state after probe is finished.
> 
> Signed-off-by: Viorel Suman <viorel.suman@nxp.com>
> Signed-off-by: Florin Leotescu <florin.leotescu@nxp.com>

The order of Signed-off-lines is supposed to be in the order that people
touched the patch during submission. So your S-o-b line should be last
and it should ideally use the email address you use for sending out the
patch.

> ---
>  drivers/pwm/pwm-imx-tpm.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/pwm/pwm-imx-tpm.c b/drivers/pwm/pwm-imx-tpm.c
> index 9fc290e647e1..27e6a5342693 100644
> --- a/drivers/pwm/pwm-imx-tpm.c
> +++ b/drivers/pwm/pwm-imx-tpm.c
> @@ -33,6 +33,7 @@
>  #define PWM_IMX_TPM_CnV(n)	(0x24 + (n) * 0x8)
>  
>  #define PWM_IMX_TPM_PARAM_CHAN			GENMASK(7, 0)
> +#define PWM_IMX_TPM_GLOBAL_RST			BIT(1)
>  
>  #define PWM_IMX_TPM_SC_PS			GENMASK(2, 0)
>  #define PWM_IMX_TPM_SC_CMOD			GENMASK(4, 3)
> @@ -362,6 +363,10 @@ static int pwm_imx_tpm_probe(struct platform_device *pdev)
>  	val = readl(tpm->base + PWM_IMX_TPM_PARAM);
>  	tpm->chip.npwm = FIELD_GET(PWM_IMX_TPM_PARAM_CHAN, val);
>  
> +	/* Resets all internal logic and registers */
> +	writel(PWM_IMX_TPM_GLOBAL_RST, tpm->base + PWM_IMX_TPM_GLOBAL);
> +	writel(0, tpm->base + PWM_IMX_TPM_GLOBAL);
> +

This opposes the use case that the bootloader setup the pwm-backlight to
show a splash screen that is simply taken over by Linux without
flickering, right?

Otherwise the commit log should motivate why "the module takes the
default state" is better than the status quo and what this default state
is.

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] pwm: imx-tpm: reset module on probe
       [not found]   ` <3d866664-d2fd-4d36-9e8f-5242327e41b9@nxp.com>
@ 2024-02-15 10:12       ` Uwe Kleine-König
  0 siblings, 0 replies; 6+ messages in thread
From: Uwe Kleine-König @ 2024-02-15 10:12 UTC (permalink / raw)
  To: Viorel Suman
  Cc: Viorel Suman (OSS),
	Florin Leotescu, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
	linux-pwm, linux-arm-kernel, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 1627 bytes --]

Hello,

On Thu, Feb 01, 2024 at 06:25:26PM +0200, Viorel Suman wrote:
> On 2/1/24 15:38, Uwe Kleine-König wrote:
> > > +	/* Resets all internal logic and registers */
> > > +	writel(PWM_IMX_TPM_GLOBAL_RST, tpm->base + PWM_IMX_TPM_GLOBAL);
> > > +	writel(0, tpm->base + PWM_IMX_TPM_GLOBAL);
> > > +
> > This opposes the use case that the bootloader setup the pwm-backlight to
> > show a splash screen that is simply taken over by Linux without
> > flickering, right?
> 
> Yes, I was not aware of such use case. Is it acceptable if I'll update
> 
> the patch in a such way so that the software reset happens as function
> 
> of a property in the related DTS node ?

That sounds wrong. Why do you reset at all? If at all, only reset if all
channels are disabled (if at all).

> > Otherwise the commit log should motivate why "the module takes the
> > default state" is better than the status quo and what this default state
> > is.
> 
> The default state above means IP internal logic being reset to the initial
> state and
> 
> registers values being set to their reset values. We're facing a situation
> on iMX95 when
> 
> A core may be reset independently from the rest of the SoC, this triggers a
> new
> 
> SPL->U-Boot->Linux boot, and in Linux probe phase PWM will inherit its state
> from
> 
> previous Linux runtime - this leads to some issues in suspend/resume
> functionality.

What kind of issues?

Best regards
Uwe

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] pwm: imx-tpm: reset module on probe
@ 2024-02-15 10:12       ` Uwe Kleine-König
  0 siblings, 0 replies; 6+ messages in thread
From: Uwe Kleine-König @ 2024-02-15 10:12 UTC (permalink / raw)
  To: Viorel Suman
  Cc: Viorel Suman (OSS),
	Florin Leotescu, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
	linux-pwm, linux-arm-kernel, linux-kernel


[-- Attachment #1.1: Type: text/plain, Size: 1627 bytes --]

Hello,

On Thu, Feb 01, 2024 at 06:25:26PM +0200, Viorel Suman wrote:
> On 2/1/24 15:38, Uwe Kleine-König wrote:
> > > +	/* Resets all internal logic and registers */
> > > +	writel(PWM_IMX_TPM_GLOBAL_RST, tpm->base + PWM_IMX_TPM_GLOBAL);
> > > +	writel(0, tpm->base + PWM_IMX_TPM_GLOBAL);
> > > +
> > This opposes the use case that the bootloader setup the pwm-backlight to
> > show a splash screen that is simply taken over by Linux without
> > flickering, right?
> 
> Yes, I was not aware of such use case. Is it acceptable if I'll update
> 
> the patch in a such way so that the software reset happens as function
> 
> of a property in the related DTS node ?

That sounds wrong. Why do you reset at all? If at all, only reset if all
channels are disabled (if at all).

> > Otherwise the commit log should motivate why "the module takes the
> > default state" is better than the status quo and what this default state
> > is.
> 
> The default state above means IP internal logic being reset to the initial
> state and
> 
> registers values being set to their reset values. We're facing a situation
> on iMX95 when
> 
> A core may be reset independently from the rest of the SoC, this triggers a
> new
> 
> SPL->U-Boot->Linux boot, and in Linux probe phase PWM will inherit its state
> from
> 
> previous Linux runtime - this leads to some issues in suspend/resume
> functionality.

What kind of issues?

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
end of thread, other threads:[~2024-02-15 10:13 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-02-01 12:22 [PATCH] pwm: imx-tpm: reset module on probe Viorel Suman (OSS)
2024-02-01 12:22 ` Viorel Suman (OSS)
2024-02-01 13:38 ` Uwe Kleine-König
2024-02-01 13:38   ` Uwe Kleine-König
     [not found]   ` <3d866664-d2fd-4d36-9e8f-5242327e41b9@nxp.com>
2024-02-15 10:12     ` Uwe Kleine-König
2024-02-15 10:12       ` Uwe Kleine-König

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.