Re: [PATCH v4 3/3] clocksource: Add Low Power STM32 timers driver

[Benjamin GAIGNARD <benjamin.gaignard@st.com>]

On 2/20/20 11:36 AM, Daniel Lezcano wrote:
> On 17/02/2020 14:45, Benjamin Gaignard wrote:
>> From: Benjamin Gaignard <benjamin.gaignard@linaro.org>
>>
>> Implement clock event driver using low power STM32 timers.
>> Low power timer counters running even when CPUs are stopped.
>> It could be used as clock event broadcaster to wake up CPUs but not like
>> a clocksource because each it rise an interrupt the counter restart from 0.
>>
>> Low power timers have a 16 bits counter and a prescaler which allow to
>> divide the clock per power of 2 to up 128 to target a 32KHz rate.
>>
>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
>> Signed-off-by: Pascal Paillet <p.paillet@st.com>
>> ---
>> version 4:
>> - move defines in mfd/stm32-lptimer.h
>> - change compatiblename
>> - reword commit message
>> - make driver Kconfig depends of MFD_STM32_LPTIMER
>> - remove useless include
>> - remove rate and clk fields from the private structure
>> - to add comments about the registers sequence in stm32_clkevent_lp_set_timer
>> - rework probe function and use devm_request_irq()
>> - do not allow module to be removed
>> - make sure that wakeup interrupt is set
>>
>>   drivers/clocksource/Kconfig          |   7 ++
>>   drivers/clocksource/Makefile         |   1 +
>>   drivers/clocksource/timer-stm32-lp.c | 213 +++++++++++++++++++++++++++++++++++
>>   3 files changed, 221 insertions(+)
>>   create mode 100644 drivers/clocksource/timer-stm32-lp.c
>>
>> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
>> index cc909e465823..9fc2b513db6f 100644
>> --- a/drivers/clocksource/Kconfig
>> +++ b/drivers/clocksource/Kconfig
>> @@ -292,6 +292,13 @@ config CLKSRC_STM32
>>   	select CLKSRC_MMIO
>>   	select TIMER_OF
>>   
>> +config CLKSRC_STM32_LP
>> +	bool "Low power clocksource for STM32 SoCs"
>> +	depends on MFD_STM32_LPTIMER || COMPILE_TEST
>> +	help
>> +	  This option enables support for STM32 low power clockevent available
>> +	  on STM32 SoCs
>> +
>>   config CLKSRC_MPS2
>>   	bool "Clocksource for MPS2 SoCs" if COMPILE_TEST
>>   	depends on GENERIC_SCHED_CLOCK
>> diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
>> index 713686faa549..c00fffbd4769 100644
>> --- a/drivers/clocksource/Makefile
>> +++ b/drivers/clocksource/Makefile
>> @@ -44,6 +44,7 @@ obj-$(CONFIG_BCM_KONA_TIMER)	+= bcm_kona_timer.o
>>   obj-$(CONFIG_CADENCE_TTC_TIMER)	+= timer-cadence-ttc.o
>>   obj-$(CONFIG_CLKSRC_EFM32)	+= timer-efm32.o
>>   obj-$(CONFIG_CLKSRC_STM32)	+= timer-stm32.o
>> +obj-$(CONFIG_CLKSRC_STM32_LP)	+= timer-stm32-lp.o
>>   obj-$(CONFIG_CLKSRC_EXYNOS_MCT)	+= exynos_mct.o
>>   obj-$(CONFIG_CLKSRC_LPC32XX)	+= timer-lpc32xx.o
>>   obj-$(CONFIG_CLKSRC_MPS2)	+= mps2-timer.o
>> diff --git a/drivers/clocksource/timer-stm32-lp.c b/drivers/clocksource/timer-stm32-lp.c
>> new file mode 100644
>> index 000000000000..50eecdb88216
>> --- /dev/null
>> +++ b/drivers/clocksource/timer-stm32-lp.c
>> @@ -0,0 +1,213 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) STMicroelectronics 2019 - All Rights Reserved
>> + * Authors: Benjamin Gaignard <benjamin.gaignard@st.com> for STMicroelectronics.
>> + *	    Pascal Paillet <p.paillet@st.com> for STMicroelectronics.
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/clockchips.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/mfd/stm32-lptimer.h>
>> +#include <linux/module.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_irq.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pm_wakeirq.h>
>> +
>> +#define CFGR_PSC_OFFSET		9
>> +#define STM32_LP_RATING		400
>> +#define STM32_TARGET_CLKRATE	(32000 * HZ)
>> +#define STM32_LP_MAX_PSC	7
>> +
>> +struct stm32_lp_private {
>> +	struct regmap *reg;
>> +	struct clock_event_device clkevt;
>> +	unsigned long period;
>> +};
>> +
>> +static struct stm32_lp_private*
>> +to_priv(struct clock_event_device *clkevt)
>> +{
>> +	return container_of(clkevt, struct stm32_lp_private, clkevt);
>> +}
>> +
>> +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);
>> +	regmap_write(priv->reg, STM32_LPTIM_IER, 0);
>> +	/* clear pending flags */
>> +	regmap_write(priv->reg, STM32_LPTIM_ICR, STM32_LPTIM_ARRMCF);
>> +
>> +	return 0;
>> +}
>> +
>> +static int stm32_clkevent_lp_set_timer(unsigned long evt,
>> +				       struct clock_event_device *clkevt,
>> +				       int is_periodic)
>> +{
>> +	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);
>> +	/* 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);
>> +	/* set next event counter */
>> +	regmap_write(priv->reg, STM32_LPTIM_ARR, evt);
>> +
>> +	/* start counter */
>> +	if (is_periodic)
>> +		regmap_write(priv->reg, STM32_LPTIM_CR,
>> +			     STM32_LPTIM_CNTSTRT | STM32_LPTIM_ENABLE);
>> +	else
>> +		regmap_write(priv->reg, STM32_LPTIM_CR,
>> +			     STM32_LPTIM_SNGSTRT | STM32_LPTIM_ENABLE);
> The regmap config in stm32-lptimer is not defined with the fast_io flag
> (on purpose or not?) that means we can potentially deadlock here as the
> lock is a mutex.
>
> Isn't it detected with the lock validation scheme?
It wasn't a problem with other parts of the mfd and I don't notice 
issues so I guess it is ok.
>
>> +	return 0;
>> +}
>> +static int stm32_clkevent_lp_remove(struct platform_device *pdev)
>> +{
>> +	return -EBUSY; /* cannot unregister clockevent */
>> +}
> Won't be the mfd into an inconsistent state here? The other subsystems
> will be removed but this one will prevent to unload the module leading
> to a situation where the mfd is partially removed but still there
> without a possible recovery, no?
We can't enable the timer part of the mfd at the same time than the 
other features.
It has be exclusive and that exclude the problem you describe above.

>
>> +static const struct of_device_id stm32_clkevent_lp_of_match[] = {
>> +	{ .compatible = "st,stm32-lptimer-timer", },
>> +	{},
>> +};
>> +MODULE_DEVICE_TABLE(of, stm32_clkevent_lp_of_match);
>> +
>> +static struct platform_driver stm32_clkevent_lp_driver = {
>> +	.probe	= stm32_clkevent_lp_probe,
>> +	.remove = stm32_clkevent_lp_remove,
>> +	.driver	= {
>> +		.name = "stm32-lptimer-timer",
>> +		.of_match_table = of_match_ptr(stm32_clkevent_lp_of_match),
>> +	},
>> +};
>