[v3] Add STM32MP25 LPTIM support: MFD, PWM, IIO, counter, clocksource

[PATCH v3 4/8] clocksource: stm32-lptimer: add support for stm32mp25

Posted by Fabrice Gasnier 11 months, 1 week ago

On stm32mp25, DIER (former IER) must only be modified when the lptimer
is enabled. On earlier SoCs, it must be only be modified when it is
disabled. Read the LPTIM_VERR register to properly manage the enable
state, before accessing IER.

Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
---
Changes in V2:
- rely on fallback compatible as no specific .data is associated to the
  driver. Use version data from MFD core.
- Added interrupt enable register access update in (missed in V1)
---
 drivers/clocksource/timer-stm32-lp.c | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/drivers/clocksource/timer-stm32-lp.c b/drivers/clocksource/timer-stm32-lp.c
index a4c95161cb22..96d975adf7a4 100644
--- a/drivers/clocksource/timer-stm32-lp.c
+++ b/drivers/clocksource/timer-stm32-lp.c
@@ -25,6 +25,7 @@ struct stm32_lp_private {
 	struct clock_event_device clkevt;
 	unsigned long period;
 	struct device *dev;
+	bool ier_wr_enabled;	/* Enables LPTIMER before writing into IER register */
 };
 
 static struct stm32_lp_private*
@@ -37,8 +38,15 @@ static int stm32_clkevent_lp_shutdown(struct clock_event_device *clkevt)
 {
 	struct stm32_lp_private *priv = to_priv(clkevt);
 
-	regmap_write(priv->reg, STM32_LPTIM_CR, 0);
+	/* Disable LPTIMER either before or after writing IER register (else, keep it enabled) */
+	if (!priv->ier_wr_enabled)
+		regmap_write(priv->reg, STM32_LPTIM_CR, 0);
+
 	regmap_write(priv->reg, STM32_LPTIM_IER, 0);
+
+	if (priv->ier_wr_enabled)
+		regmap_write(priv->reg, STM32_LPTIM_CR, 0);
+
 	/* clear pending flags */
 	regmap_write(priv->reg, STM32_LPTIM_ICR, STM32_LPTIM_ARRMCF);
 
@@ -51,12 +59,21 @@ static int stm32_clkevent_lp_set_timer(unsigned long evt,
 {
 	struct stm32_lp_private *priv = to_priv(clkevt);
 
-	/* disable LPTIMER to be able to write into IER register*/
-	regmap_write(priv->reg, STM32_LPTIM_CR, 0);
+	if (!priv->ier_wr_enabled) {
+		/* Disable LPTIMER to be able to write into IER register */
+		regmap_write(priv->reg, STM32_LPTIM_CR, 0);
+	} else {
+		/* Enable LPTIMER to be able to write into IER register */
+		regmap_write(priv->reg, STM32_LPTIM_CR, STM32_LPTIM_ENABLE);
+	}
+
 	/* enable ARR interrupt */
 	regmap_write(priv->reg, STM32_LPTIM_IER, STM32_LPTIM_ARRMIE);
+
 	/* enable LPTIMER to be able to write into ARR register */
-	regmap_write(priv->reg, STM32_LPTIM_CR, STM32_LPTIM_ENABLE);
+	if (!priv->ier_wr_enabled)
+		regmap_write(priv->reg, STM32_LPTIM_CR, STM32_LPTIM_ENABLE);
+
 	/* set next event counter */
 	regmap_write(priv->reg, STM32_LPTIM_ARR, evt);
 
@@ -151,6 +168,7 @@ static int stm32_clkevent_lp_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	priv->reg = ddata->regmap;
+	priv->ier_wr_enabled = ddata->version == STM32_LPTIM_VERR_23;
 	ret = clk_prepare_enable(ddata->clk);
 	if (ret)
 		return -EINVAL;
-- 
2.25.1

Re: [PATCH v3 4/8] clocksource: stm32-lptimer: add support for stm32mp25

Posted by Daniel Lezcano 11 months, 1 week ago

On 05/03/2025 10:49, Fabrice Gasnier wrote:
> On stm32mp25, DIER (former IER) must only be modified when the lptimer
> is enabled. On earlier SoCs, it must be only be modified when it is
> disabled. Read the LPTIM_VERR register to properly manage the enable
> state, before accessing IER.
> 
> Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
> ---
> Changes in V2:
> - rely on fallback compatible as no specific .data is associated to the
>    driver. Use version data from MFD core.
> - Added interrupt enable register access update in (missed in V1)
> ---
>   drivers/clocksource/timer-stm32-lp.c | 26 ++++++++++++++++++++++----
>   1 file changed, 22 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/clocksource/timer-stm32-lp.c b/drivers/clocksource/timer-stm32-lp.c
> index a4c95161cb22..96d975adf7a4 100644
> --- a/drivers/clocksource/timer-stm32-lp.c
> +++ b/drivers/clocksource/timer-stm32-lp.c
> @@ -25,6 +25,7 @@ struct stm32_lp_private {
>   	struct clock_event_device clkevt;
>   	unsigned long period;
>   	struct device *dev;
> +	bool ier_wr_enabled;	/* Enables LPTIMER before writing into IER register */
>   };
>   
>   static struct stm32_lp_private*
> @@ -37,8 +38,15 @@ static int stm32_clkevent_lp_shutdown(struct clock_event_device *clkevt)
>   {
>   	struct stm32_lp_private *priv = to_priv(clkevt);
>   
> -	regmap_write(priv->reg, STM32_LPTIM_CR, 0);
> +	/* Disable LPTIMER either before or after writing IER register (else, keep it enabled) */
> +	if (!priv->ier_wr_enabled)
> +		regmap_write(priv->reg, STM32_LPTIM_CR, 0);
> +
>   	regmap_write(priv->reg, STM32_LPTIM_IER, 0);
> +

Why not encapsulate the function ?

	regmap_write_ier(struct stm32_lp_private *priv, int value)
	{

		/* A comment ... */
		if (!priv->ier_wr_enabled)
			regmap_write(priv->reg, STM32_LPTIM_CR, 0);

		regmap_write(priv->reg, STM32_LPTIM_IER, value);

		if (!priv->ier_wr_enabled)
			regmap_write(priv->reg, STM32_LPTIM_CR, STM32_LPTIM_ENABLE);
	}


> +	if (priv->ier_wr_enabled)
> +		regmap_write(priv->reg, STM32_LPTIM_CR, 0);
> +

>   	/* clear pending flags */
>   	regmap_write(priv->reg, STM32_LPTIM_ICR, STM32_LPTIM_ARRMCF);
>   
> @@ -51,12 +59,21 @@ static int stm32_clkevent_lp_set_timer(unsigned long evt,
>   {
>   	struct stm32_lp_private *priv = to_priv(clkevt);
>   
> -	/* disable LPTIMER to be able to write into IER register*/
> -	regmap_write(priv->reg, STM32_LPTIM_CR, 0);
> +	if (!priv->ier_wr_enabled) {
> +		/* Disable LPTIMER to be able to write into IER register */
> +		regmap_write(priv->reg, STM32_LPTIM_CR, 0);
> +	} else {
> +		/* Enable LPTIMER to be able to write into IER register */
> +		regmap_write(priv->reg, STM32_LPTIM_CR, STM32_LPTIM_ENABLE);
> +	}
> +
>   	/* enable ARR interrupt */
>   	regmap_write(priv->reg, STM32_LPTIM_IER, STM32_LPTIM_ARRMIE);
> +
>   	/* enable LPTIMER to be able to write into ARR register */
> -	regmap_write(priv->reg, STM32_LPTIM_CR, STM32_LPTIM_ENABLE);
> +	if (!priv->ier_wr_enabled)
> +		regmap_write(priv->reg, STM32_LPTIM_CR, STM32_LPTIM_ENABLE);
> +
>   	/* set next event counter */
>   	regmap_write(priv->reg, STM32_LPTIM_ARR, evt);
>   
> @@ -151,6 +168,7 @@ static int stm32_clkevent_lp_probe(struct platform_device *pdev)
>   		return -ENOMEM;
>   
>   	priv->reg = ddata->regmap;
> +	priv->ier_wr_enabled = ddata->version == STM32_LPTIM_VERR_23;
>   	ret = clk_prepare_enable(ddata->clk);
>   	if (ret)
>   		return -EINVAL;


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH v3 4/8] clocksource: stm32-lptimer: add support for stm32mp25

Posted by Fabrice Gasnier 11 months ago

On 3/7/25 16:12, Daniel Lezcano wrote:
> On 05/03/2025 10:49, Fabrice Gasnier wrote:
>> On stm32mp25, DIER (former IER) must only be modified when the lptimer
>> is enabled. On earlier SoCs, it must be only be modified when it is
>> disabled. Read the LPTIM_VERR register to properly manage the enable
>> state, before accessing IER.
>>
>> Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
>> ---
>> Changes in V2:
>> - rely on fallback compatible as no specific .data is associated to the
>>    driver. Use version data from MFD core.
>> - Added interrupt enable register access update in (missed in V1)
>> ---
>>   drivers/clocksource/timer-stm32-lp.c | 26 ++++++++++++++++++++++----
>>   1 file changed, 22 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/clocksource/timer-stm32-lp.c
>> b/drivers/clocksource/timer-stm32-lp.c
>> index a4c95161cb22..96d975adf7a4 100644
>> --- a/drivers/clocksource/timer-stm32-lp.c
>> +++ b/drivers/clocksource/timer-stm32-lp.c
>> @@ -25,6 +25,7 @@ struct stm32_lp_private {
>>       struct clock_event_device clkevt;
>>       unsigned long period;
>>       struct device *dev;
>> +    bool ier_wr_enabled;    /* Enables LPTIMER before writing into
>> IER register */
>>   };
>>     static struct stm32_lp_private*
>> @@ -37,8 +38,15 @@ static int stm32_clkevent_lp_shutdown(struct
>> clock_event_device *clkevt)
>>   {
>>       struct stm32_lp_private *priv = to_priv(clkevt);
>>   -    regmap_write(priv->reg, STM32_LPTIM_CR, 0);
>> +    /* Disable LPTIMER either before or after writing IER register
>> (else, keep it enabled) */
>> +    if (!priv->ier_wr_enabled)
>> +        regmap_write(priv->reg, STM32_LPTIM_CR, 0);
>> +
>>       regmap_write(priv->reg, STM32_LPTIM_IER, 0);
>> +
> 
> Why not encapsulate the function ?
> 
>     regmap_write_ier(struct stm32_lp_private *priv, int value)
>     {
> 
>         /* A comment ... */
>         if (!priv->ier_wr_enabled)
>             regmap_write(priv->reg, STM32_LPTIM_CR, 0);
> 
>         regmap_write(priv->reg, STM32_LPTIM_IER, value);
> 
>         if (!priv->ier_wr_enabled)
>             regmap_write(priv->reg, STM32_LPTIM_CR, STM32_LPTIM_ENABLE);
>     }

Hi Daniel,

Thanks for your suggestion.

I've tried various factorization but can't really find something pretty
and easy to read.

I think I'll rather implement some dedicated ops in V4, for stm32mp25,
based on compatible data.
This would allow straight forward sequence, without dangling with enable
bit / flags. I also need to add some checks on new status flags. So this
will avoid to add complexity on existing routines.

Best Regards,
Fabrice

> 
> 
>> +    if (priv->ier_wr_enabled)
>> +        regmap_write(priv->reg, STM32_LPTIM_CR, 0);
>> +
> 
>>       /* clear pending flags */
>>       regmap_write(priv->reg, STM32_LPTIM_ICR, STM32_LPTIM_ARRMCF);
>>   @@ -51,12 +59,21 @@ static int stm32_clkevent_lp_set_timer(unsigned
>> long evt,
>>   {
>>       struct stm32_lp_private *priv = to_priv(clkevt);
>>   -    /* disable LPTIMER to be able to write into IER register*/
>> -    regmap_write(priv->reg, STM32_LPTIM_CR, 0);
>> +    if (!priv->ier_wr_enabled) {
>> +        /* Disable LPTIMER to be able to write into IER register */
>> +        regmap_write(priv->reg, STM32_LPTIM_CR, 0);
>> +    } else {
>> +        /* Enable LPTIMER to be able to write into IER register */
>> +        regmap_write(priv->reg, STM32_LPTIM_CR, STM32_LPTIM_ENABLE);
>> +    }
>> +
>>       /* enable ARR interrupt */
>>       regmap_write(priv->reg, STM32_LPTIM_IER, STM32_LPTIM_ARRMIE);
>> +
>>       /* enable LPTIMER to be able to write into ARR register */
>> -    regmap_write(priv->reg, STM32_LPTIM_CR, STM32_LPTIM_ENABLE);
>> +    if (!priv->ier_wr_enabled)
>> +        regmap_write(priv->reg, STM32_LPTIM_CR, STM32_LPTIM_ENABLE);
>> +
>>       /* set next event counter */
>>       regmap_write(priv->reg, STM32_LPTIM_ARR, evt);
>>   @@ -151,6 +168,7 @@ static int stm32_clkevent_lp_probe(struct
>> platform_device *pdev)
>>           return -ENOMEM;
>>         priv->reg = ddata->regmap;
>> +    priv->ier_wr_enabled = ddata->version == STM32_LPTIM_VERR_23;
>>       ret = clk_prepare_enable(ddata->clk);
>>       if (ret)
>>           return -EINVAL;
> 
>