[PATCH v2] iio: trigger: stm32-timer: enable clock when in master mode

[PATCH v2] iio: trigger: stm32-timer: enable clock when in master mode

[Fabrice Gasnier]

Clock should be enabled as soon as using master modes, even before
enabling timer. Or, this may provoke bad behavior on the other end
(slave timer). Then, introduce 'clk_enabled' flag, instead of relying
on CR1 EN bit, to keep track of clock being enabled (balanced refcount).
Propagate this anywhere else in the driver.

Also add 'remove' routine to stop timer and disable clock in case it
has been left enabled. Enforce the user interface has been unregistered
in the remove routine, before disabling the hardware to avoid possible
race. So, remove use of devm_ variant to register triggers and unregister
them before the hardware gets disabled [1].
[1] https://patchwork.kernel.org/patch/9956247/

Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
---
Changes in v2:
- enforce the user interface has been unregistered in the remove routine,
  before disabling the hardware to avoid possible race.
---
 drivers/iio/trigger/stm32-timer-trigger.c | 98 ++++++++++++++++++++++++-------
 1 file changed, 76 insertions(+), 22 deletions(-)

diff --git a/drivers/iio/trigger/stm32-timer-trigger.c b/drivers/iio/trigger/stm32-timer-trigger.c
index a5dfe65..473853e 100644
--- a/drivers/iio/trigger/stm32-timer-trigger.c
+++ b/drivers/iio/trigger/stm32-timer-trigger.c
@@ -79,10 +79,13 @@ struct stm32_timer_trigger {
 	struct device *dev;
 	struct regmap *regmap;
 	struct clk *clk;
+	bool clk_enabled;
 	u32 max_arr;
 	const void *triggers;
 	const void *valids;
 	bool has_trgo2;
+	struct mutex lock; /* concurrent sysfs configuration */
+	struct list_head tr_list;
 };
 
 struct stm32_timer_trigger_cfg {
@@ -106,7 +109,7 @@ static int stm32_timer_start(struct stm32_timer_trigger *priv,
 {
 	unsigned long long prd, div;
 	int prescaler = 0;
-	u32 ccer, cr1;
+	u32 ccer;
 
 	/* Period and prescaler values depends of clock rate */
 	div = (unsigned long long)clk_get_rate(priv->clk);
@@ -136,9 +139,11 @@ static int stm32_timer_start(struct stm32_timer_trigger *priv,
 	if (ccer & TIM_CCER_CCXE)
 		return -EBUSY;
 
-	regmap_read(priv->regmap, TIM_CR1, &cr1);
-	if (!(cr1 & TIM_CR1_CEN))
+	mutex_lock(&priv->lock);
+	if (!priv->clk_enabled) {
+		priv->clk_enabled = true;
 		clk_enable(priv->clk);
+	}
 
 	regmap_write(priv->regmap, TIM_PSC, prescaler);
 	regmap_write(priv->regmap, TIM_ARR, prd - 1);
@@ -157,22 +162,20 @@ static int stm32_timer_start(struct stm32_timer_trigger *priv,
 
 	/* Enable controller */
 	regmap_update_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN, TIM_CR1_CEN);
+	mutex_unlock(&priv->lock);
 
 	return 0;
 }
 
 static void stm32_timer_stop(struct stm32_timer_trigger *priv)
 {
-	u32 ccer, cr1;
+	u32 ccer;
 
 	regmap_read(priv->regmap, TIM_CCER, &ccer);
 	if (ccer & TIM_CCER_CCXE)
 		return;
 
-	regmap_read(priv->regmap, TIM_CR1, &cr1);
-	if (cr1 & TIM_CR1_CEN)
-		clk_disable(priv->clk);
-
+	mutex_lock(&priv->lock);
 	/* Stop timer */
 	regmap_update_bits(priv->regmap, TIM_CR1, TIM_CR1_ARPE, 0);
 	regmap_update_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN, 0);
@@ -181,6 +184,12 @@ static void stm32_timer_stop(struct stm32_timer_trigger *priv)
 
 	/* Make sure that registers are updated */
 	regmap_update_bits(priv->regmap, TIM_EGR, TIM_EGR_UG, TIM_EGR_UG);
+
+	if (priv->clk_enabled) {
+		priv->clk_enabled = false;
+		clk_disable(priv->clk);
+	}
+	mutex_unlock(&priv->lock);
 }
 
 static ssize_t stm32_tt_store_frequency(struct device *dev,
@@ -295,11 +304,18 @@ static ssize_t stm32_tt_store_master_mode(struct device *dev,
 	for (i = 0; i <= master_mode_max; i++) {
 		if (!strncmp(master_mode_table[i], buf,
 			     strlen(master_mode_table[i]))) {
+			mutex_lock(&priv->lock);
+			if (!priv->clk_enabled) {
+				/* Clock should be enabled first */
+				priv->clk_enabled = true;
+				clk_enable(priv->clk);
+			}
 			regmap_update_bits(priv->regmap, TIM_CR2, mask,
 					   i << shift);
 			/* Make sure that registers are updated */
 			regmap_update_bits(priv->regmap, TIM_EGR,
 					   TIM_EGR_UG, TIM_EGR_UG);
+			mutex_unlock(&priv->lock);
 			return len;
 		}
 	}
@@ -357,11 +373,21 @@ static const struct attribute_group *stm32_trigger_attr_groups[] = {
 static const struct iio_trigger_ops timer_trigger_ops = {
 };
 
-static int stm32_setup_iio_triggers(struct stm32_timer_trigger *priv)
+static void stm32_unregister_iio_triggers(struct stm32_timer_trigger *priv)
+{
+	struct iio_trigger *tr;
+
+	list_for_each_entry(tr, &priv->tr_list, alloc_list)
+		iio_trigger_unregister(tr);
+}
+
+static int stm32_register_iio_triggers(struct stm32_timer_trigger *priv)
 {
 	int ret;
 	const char * const *cur = priv->triggers;
 
+	INIT_LIST_HEAD(&priv->tr_list);
+
 	while (cur && *cur) {
 		struct iio_trigger *trig;
 		bool cur_is_trgo = stm32_timer_is_trgo_name(*cur);
@@ -388,9 +414,13 @@ static int stm32_setup_iio_triggers(struct stm32_timer_trigger *priv)
 
 		iio_trigger_set_drvdata(trig, priv);
 
-		ret = devm_iio_trigger_register(priv->dev, trig);
-		if (ret)
+		ret = iio_trigger_register(trig);
+		if (ret) {
+			stm32_unregister_iio_triggers(priv);
 			return ret;
+		}
+
+		list_add_tail(&trig->alloc_list, &priv->tr_list);
 		cur++;
 	}
 
@@ -437,7 +467,6 @@ static int stm32_counter_write_raw(struct iio_dev *indio_dev,
 				   int val, int val2, long mask)
 {
 	struct stm32_timer_trigger *priv = iio_priv(indio_dev);
-	u32 dat;
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
@@ -448,19 +477,23 @@ static int stm32_counter_write_raw(struct iio_dev *indio_dev,
 		return -EINVAL;
 
 	case IIO_CHAN_INFO_ENABLE:
+		mutex_lock(&priv->lock);
 		if (val) {
-			regmap_read(priv->regmap, TIM_CR1, &dat);
-			if (!(dat & TIM_CR1_CEN))
+			if (!priv->clk_enabled) {
+				priv->clk_enabled = true;
 				clk_enable(priv->clk);
+			}
 			regmap_update_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN,
 					   TIM_CR1_CEN);
 		} else {
-			regmap_read(priv->regmap, TIM_CR1, &dat);
 			regmap_update_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN,
 					   0);
-			if (dat & TIM_CR1_CEN)
+			if (priv->clk_enabled) {
+				priv->clk_enabled = false;
 				clk_disable(priv->clk);
+			}
 		}
+		mutex_unlock(&priv->lock);
 		return 0;
 	}
 
@@ -556,7 +589,6 @@ static int stm32_set_enable_mode(struct iio_dev *indio_dev,
 {
 	struct stm32_timer_trigger *priv = iio_priv(indio_dev);
 	int sms = stm32_enable_mode2sms(mode);
-	u32 val;
 
 	if (sms < 0)
 		return sms;
@@ -564,11 +596,12 @@ static int stm32_set_enable_mode(struct iio_dev *indio_dev,
 	 * Triggered mode sets CEN bit automatically by hardware. So, first
 	 * enable counter clock, so it can use it. Keeps it in sync with CEN.
 	 */
-	if (sms == 6) {
-		regmap_read(priv->regmap, TIM_CR1, &val);
-		if (!(val & TIM_CR1_CEN))
-			clk_enable(priv->clk);
+	mutex_lock(&priv->lock);
+	if (sms == 6 && !priv->clk_enabled) {
+		clk_enable(priv->clk);
+		priv->clk_enabled = true;
 	}
+	mutex_unlock(&priv->lock);
 
 	regmap_update_bits(priv->regmap, TIM_SMCR, TIM_SMCR_SMS, sms);
 
@@ -752,8 +785,9 @@ static int stm32_timer_trigger_probe(struct platform_device *pdev)
 	priv->triggers = triggers_table[index];
 	priv->valids = cfg->valids_table[index];
 	stm32_timer_detect_trgo2(priv);
+	mutex_init(&priv->lock);
 
-	ret = stm32_setup_iio_triggers(priv);
+	ret = stm32_register_iio_triggers(priv);
 	if (ret)
 		return ret;
 
@@ -762,6 +796,25 @@ static int stm32_timer_trigger_probe(struct platform_device *pdev)
 	return 0;
 }
 
+static int stm32_timer_trigger_remove(struct platform_device *pdev)
+{
+	struct stm32_timer_trigger *priv = platform_get_drvdata(pdev);
+	u32 val;
+
+	/* Unregister triggers before everything can be safely turned off */
+	stm32_unregister_iio_triggers(priv);
+
+	/* Check if nobody else use the timer, then disable it */
+	regmap_read(priv->regmap, TIM_CCER, &val);
+	if (!(val & TIM_CCER_CCXE))
+		regmap_update_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN, 0);
+
+	if (priv->clk_enabled)
+		clk_disable(priv->clk);
+
+	return 0;
+}
+
 static const struct stm32_timer_trigger_cfg stm32_timer_trg_cfg = {
 	.valids_table = valids_table,
 	.num_valids_table = ARRAY_SIZE(valids_table),
@@ -786,6 +839,7 @@ MODULE_DEVICE_TABLE(of, stm32_trig_of_match);
 
 static struct platform_driver stm32_timer_trigger_driver = {
 	.probe = stm32_timer_trigger_probe,
+	.remove = stm32_timer_trigger_remove,
 	.driver = {
 		.name = "stm32-timer-trigger",
 		.of_match_table = stm32_trig_of_match,
-- 
2.7.4

Re: [PATCH v2] iio: trigger: stm32-timer: enable clock when in master mode

[Jonathan Cameron]

On Fri, 7 Feb 2020 17:30:31 +0100
Fabrice Gasnier <fabrice.gasnier@st.com> wrote:

> Clock should be enabled as soon as using master modes, even before
> enabling timer. Or, this may provoke bad behavior on the other end
> (slave timer). Then, introduce 'clk_enabled' flag, instead of relying
> on CR1 EN bit, to keep track of clock being enabled (balanced refcount).
> Propagate this anywhere else in the driver.
> 
> Also add 'remove' routine to stop timer and disable clock in case it
> has been left enabled. Enforce the user interface has been unregistered
> in the remove routine, before disabling the hardware to avoid possible
> race. So, remove use of devm_ variant to register triggers and unregister
> them before the hardware gets disabled [1].
> [1] https://patchwork.kernel.org/patch/9956247/
> 
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
Patch looks fine. So is this a fix for a known issue or a theoretical one?
Just a question of whether to line it up for an rc, or wait for next
merge window.  Ideally please give me a fixes tag as well if this
fixes a problem that is being seen in the wild.

Thanks,

Jonathan

> ---
> Changes in v2:
> - enforce the user interface has been unregistered in the remove routine,
>   before disabling the hardware to avoid possible race.
> ---
>  drivers/iio/trigger/stm32-timer-trigger.c | 98 ++++++++++++++++++++++++-------
>  1 file changed, 76 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/iio/trigger/stm32-timer-trigger.c b/drivers/iio/trigger/stm32-timer-trigger.c
> index a5dfe65..473853e 100644
> --- a/drivers/iio/trigger/stm32-timer-trigger.c
> +++ b/drivers/iio/trigger/stm32-timer-trigger.c
> @@ -79,10 +79,13 @@ struct stm32_timer_trigger {
>  	struct device *dev;
>  	struct regmap *regmap;
>  	struct clk *clk;
> +	bool clk_enabled;
>  	u32 max_arr;
>  	const void *triggers;
>  	const void *valids;
>  	bool has_trgo2;
> +	struct mutex lock; /* concurrent sysfs configuration */
> +	struct list_head tr_list;
>  };
>  
>  struct stm32_timer_trigger_cfg {
> @@ -106,7 +109,7 @@ static int stm32_timer_start(struct stm32_timer_trigger *priv,
>  {
>  	unsigned long long prd, div;
>  	int prescaler = 0;
> -	u32 ccer, cr1;
> +	u32 ccer;
>  
>  	/* Period and prescaler values depends of clock rate */
>  	div = (unsigned long long)clk_get_rate(priv->clk);
> @@ -136,9 +139,11 @@ static int stm32_timer_start(struct stm32_timer_trigger *priv,
>  	if (ccer & TIM_CCER_CCXE)
>  		return -EBUSY;
>  
> -	regmap_read(priv->regmap, TIM_CR1, &cr1);
> -	if (!(cr1 & TIM_CR1_CEN))
> +	mutex_lock(&priv->lock);
> +	if (!priv->clk_enabled) {
> +		priv->clk_enabled = true;
>  		clk_enable(priv->clk);
> +	}
>  
>  	regmap_write(priv->regmap, TIM_PSC, prescaler);
>  	regmap_write(priv->regmap, TIM_ARR, prd - 1);
> @@ -157,22 +162,20 @@ static int stm32_timer_start(struct stm32_timer_trigger *priv,
>  
>  	/* Enable controller */
>  	regmap_update_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN, TIM_CR1_CEN);
> +	mutex_unlock(&priv->lock);
>  
>  	return 0;
>  }
>  
>  static void stm32_timer_stop(struct stm32_timer_trigger *priv)
>  {
> -	u32 ccer, cr1;
> +	u32 ccer;
>  
>  	regmap_read(priv->regmap, TIM_CCER, &ccer);
>  	if (ccer & TIM_CCER_CCXE)
>  		return;
>  
> -	regmap_read(priv->regmap, TIM_CR1, &cr1);
> -	if (cr1 & TIM_CR1_CEN)
> -		clk_disable(priv->clk);
> -
> +	mutex_lock(&priv->lock);
>  	/* Stop timer */
>  	regmap_update_bits(priv->regmap, TIM_CR1, TIM_CR1_ARPE, 0);
>  	regmap_update_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN, 0);
> @@ -181,6 +184,12 @@ static void stm32_timer_stop(struct stm32_timer_trigger *priv)
>  
>  	/* Make sure that registers are updated */
>  	regmap_update_bits(priv->regmap, TIM_EGR, TIM_EGR_UG, TIM_EGR_UG);
> +
> +	if (priv->clk_enabled) {
> +		priv->clk_enabled = false;
> +		clk_disable(priv->clk);
> +	}
> +	mutex_unlock(&priv->lock);
>  }
>  
>  static ssize_t stm32_tt_store_frequency(struct device *dev,
> @@ -295,11 +304,18 @@ static ssize_t stm32_tt_store_master_mode(struct device *dev,
>  	for (i = 0; i <= master_mode_max; i++) {
>  		if (!strncmp(master_mode_table[i], buf,
>  			     strlen(master_mode_table[i]))) {
> +			mutex_lock(&priv->lock);
> +			if (!priv->clk_enabled) {
> +				/* Clock should be enabled first */
> +				priv->clk_enabled = true;
> +				clk_enable(priv->clk);
> +			}
>  			regmap_update_bits(priv->regmap, TIM_CR2, mask,
>  					   i << shift);
>  			/* Make sure that registers are updated */
>  			regmap_update_bits(priv->regmap, TIM_EGR,
>  					   TIM_EGR_UG, TIM_EGR_UG);
> +			mutex_unlock(&priv->lock);
>  			return len;
>  		}
>  	}
> @@ -357,11 +373,21 @@ static const struct attribute_group *stm32_trigger_attr_groups[] = {
>  static const struct iio_trigger_ops timer_trigger_ops = {
>  };
>  
> -static int stm32_setup_iio_triggers(struct stm32_timer_trigger *priv)
> +static void stm32_unregister_iio_triggers(struct stm32_timer_trigger *priv)
> +{
> +	struct iio_trigger *tr;
> +
> +	list_for_each_entry(tr, &priv->tr_list, alloc_list)
> +		iio_trigger_unregister(tr);
> +}
> +
> +static int stm32_register_iio_triggers(struct stm32_timer_trigger *priv)
>  {
>  	int ret;
>  	const char * const *cur = priv->triggers;
>  
> +	INIT_LIST_HEAD(&priv->tr_list);
> +
>  	while (cur && *cur) {
>  		struct iio_trigger *trig;
>  		bool cur_is_trgo = stm32_timer_is_trgo_name(*cur);
> @@ -388,9 +414,13 @@ static int stm32_setup_iio_triggers(struct stm32_timer_trigger *priv)
>  
>  		iio_trigger_set_drvdata(trig, priv);
>  
> -		ret = devm_iio_trigger_register(priv->dev, trig);
> -		if (ret)
> +		ret = iio_trigger_register(trig);
> +		if (ret) {
> +			stm32_unregister_iio_triggers(priv);
>  			return ret;
> +		}
> +
> +		list_add_tail(&trig->alloc_list, &priv->tr_list);
>  		cur++;
>  	}
>  
> @@ -437,7 +467,6 @@ static int stm32_counter_write_raw(struct iio_dev *indio_dev,
>  				   int val, int val2, long mask)
>  {
>  	struct stm32_timer_trigger *priv = iio_priv(indio_dev);
> -	u32 dat;
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
> @@ -448,19 +477,23 @@ static int stm32_counter_write_raw(struct iio_dev *indio_dev,
>  		return -EINVAL;
>  
>  	case IIO_CHAN_INFO_ENABLE:
> +		mutex_lock(&priv->lock);
>  		if (val) {
> -			regmap_read(priv->regmap, TIM_CR1, &dat);
> -			if (!(dat & TIM_CR1_CEN))
> +			if (!priv->clk_enabled) {
> +				priv->clk_enabled = true;
>  				clk_enable(priv->clk);
> +			}
>  			regmap_update_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN,
>  					   TIM_CR1_CEN);
>  		} else {
> -			regmap_read(priv->regmap, TIM_CR1, &dat);
>  			regmap_update_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN,
>  					   0);
> -			if (dat & TIM_CR1_CEN)
> +			if (priv->clk_enabled) {
> +				priv->clk_enabled = false;
>  				clk_disable(priv->clk);
> +			}
>  		}
> +		mutex_unlock(&priv->lock);
>  		return 0;
>  	}
>  
> @@ -556,7 +589,6 @@ static int stm32_set_enable_mode(struct iio_dev *indio_dev,
>  {
>  	struct stm32_timer_trigger *priv = iio_priv(indio_dev);
>  	int sms = stm32_enable_mode2sms(mode);
> -	u32 val;
>  
>  	if (sms < 0)
>  		return sms;
> @@ -564,11 +596,12 @@ static int stm32_set_enable_mode(struct iio_dev *indio_dev,
>  	 * Triggered mode sets CEN bit automatically by hardware. So, first
>  	 * enable counter clock, so it can use it. Keeps it in sync with CEN.
>  	 */
> -	if (sms == 6) {
> -		regmap_read(priv->regmap, TIM_CR1, &val);
> -		if (!(val & TIM_CR1_CEN))
> -			clk_enable(priv->clk);
> +	mutex_lock(&priv->lock);
> +	if (sms == 6 && !priv->clk_enabled) {
> +		clk_enable(priv->clk);
> +		priv->clk_enabled = true;
>  	}
> +	mutex_unlock(&priv->lock);
>  
>  	regmap_update_bits(priv->regmap, TIM_SMCR, TIM_SMCR_SMS, sms);
>  
> @@ -752,8 +785,9 @@ static int stm32_timer_trigger_probe(struct platform_device *pdev)
>  	priv->triggers = triggers_table[index];
>  	priv->valids = cfg->valids_table[index];
>  	stm32_timer_detect_trgo2(priv);
> +	mutex_init(&priv->lock);
>  
> -	ret = stm32_setup_iio_triggers(priv);
> +	ret = stm32_register_iio_triggers(priv);
>  	if (ret)
>  		return ret;
>  
> @@ -762,6 +796,25 @@ static int stm32_timer_trigger_probe(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +static int stm32_timer_trigger_remove(struct platform_device *pdev)
> +{
> +	struct stm32_timer_trigger *priv = platform_get_drvdata(pdev);
> +	u32 val;
> +
> +	/* Unregister triggers before everything can be safely turned off */
> +	stm32_unregister_iio_triggers(priv);
> +
> +	/* Check if nobody else use the timer, then disable it */
> +	regmap_read(priv->regmap, TIM_CCER, &val);
> +	if (!(val & TIM_CCER_CCXE))
> +		regmap_update_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN, 0);
> +
> +	if (priv->clk_enabled)
> +		clk_disable(priv->clk);
> +
> +	return 0;
> +}
> +
>  static const struct stm32_timer_trigger_cfg stm32_timer_trg_cfg = {
>  	.valids_table = valids_table,
>  	.num_valids_table = ARRAY_SIZE(valids_table),
> @@ -786,6 +839,7 @@ MODULE_DEVICE_TABLE(of, stm32_trig_of_match);
>  
>  static struct platform_driver stm32_timer_trigger_driver = {
>  	.probe = stm32_timer_trigger_probe,
> +	.remove = stm32_timer_trigger_remove,
>  	.driver = {
>  		.name = "stm32-timer-trigger",
>  		.of_match_table = stm32_trig_of_match,

Re: [PATCH v2] iio: trigger: stm32-timer: enable clock when in master mode

[Fabrice Gasnier]

On 2/14/20 3:20 PM, Jonathan Cameron wrote:
> On Fri, 7 Feb 2020 17:30:31 +0100
> Fabrice Gasnier <fabrice.gasnier@st.com> wrote:
> 
>> Clock should be enabled as soon as using master modes, even before
>> enabling timer. Or, this may provoke bad behavior on the other end
>> (slave timer). Then, introduce 'clk_enabled' flag, instead of relying
>> on CR1 EN bit, to keep track of clock being enabled (balanced refcount).
>> Propagate this anywhere else in the driver.
>>
>> Also add 'remove' routine to stop timer and disable clock in case it
>> has been left enabled. Enforce the user interface has been unregistered
>> in the remove routine, before disabling the hardware to avoid possible
>> race. So, remove use of devm_ variant to register triggers and unregister
>> them before the hardware gets disabled [1].
>> [1] https://patchwork.kernel.org/patch/9956247/
>>
>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
> Patch looks fine. So is this a fix for a known issue or a theoretical one?
> Just a question of whether to line it up for an rc, or wait for next
> merge window.  Ideally please give me a fixes tag as well if this
> fixes a problem that is being seen in the wild.

Hi Jonathan,

I got no complain for now. So I think this is fine to wait for the next
merge window.

Thanks,
Fabrice

> 
> Thanks,
> 
> Jonathan
> 
>> ---
>> Changes in v2:
>> - enforce the user interface has been unregistered in the remove routine,
>>   before disabling the hardware to avoid possible race.
>> ---
>>  drivers/iio/trigger/stm32-timer-trigger.c | 98 ++++++++++++++++++++++++-------
>>  1 file changed, 76 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/iio/trigger/stm32-timer-trigger.c b/drivers/iio/trigger/stm32-timer-trigger.c
>> index a5dfe65..473853e 100644
>> --- a/drivers/iio/trigger/stm32-timer-trigger.c
>> +++ b/drivers/iio/trigger/stm32-timer-trigger.c
>> @@ -79,10 +79,13 @@ struct stm32_timer_trigger {
>>  	struct device *dev;
>>  	struct regmap *regmap;
>>  	struct clk *clk;
>> +	bool clk_enabled;
>>  	u32 max_arr;
>>  	const void *triggers;
>>  	const void *valids;
>>  	bool has_trgo2;
>> +	struct mutex lock; /* concurrent sysfs configuration */
>> +	struct list_head tr_list;
>>  };
>>  
>>  struct stm32_timer_trigger_cfg {
>> @@ -106,7 +109,7 @@ static int stm32_timer_start(struct stm32_timer_trigger *priv,
>>  {
>>  	unsigned long long prd, div;
>>  	int prescaler = 0;
>> -	u32 ccer, cr1;
>> +	u32 ccer;
>>  
>>  	/* Period and prescaler values depends of clock rate */
>>  	div = (unsigned long long)clk_get_rate(priv->clk);
>> @@ -136,9 +139,11 @@ static int stm32_timer_start(struct stm32_timer_trigger *priv,
>>  	if (ccer & TIM_CCER_CCXE)
>>  		return -EBUSY;
>>  
>> -	regmap_read(priv->regmap, TIM_CR1, &cr1);
>> -	if (!(cr1 & TIM_CR1_CEN))
>> +	mutex_lock(&priv->lock);
>> +	if (!priv->clk_enabled) {
>> +		priv->clk_enabled = true;
>>  		clk_enable(priv->clk);
>> +	}
>>  
>>  	regmap_write(priv->regmap, TIM_PSC, prescaler);
>>  	regmap_write(priv->regmap, TIM_ARR, prd - 1);
>> @@ -157,22 +162,20 @@ static int stm32_timer_start(struct stm32_timer_trigger *priv,
>>  
>>  	/* Enable controller */
>>  	regmap_update_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN, TIM_CR1_CEN);
>> +	mutex_unlock(&priv->lock);
>>  
>>  	return 0;
>>  }
>>  
>>  static void stm32_timer_stop(struct stm32_timer_trigger *priv)
>>  {
>> -	u32 ccer, cr1;
>> +	u32 ccer;
>>  
>>  	regmap_read(priv->regmap, TIM_CCER, &ccer);
>>  	if (ccer & TIM_CCER_CCXE)
>>  		return;
>>  
>> -	regmap_read(priv->regmap, TIM_CR1, &cr1);
>> -	if (cr1 & TIM_CR1_CEN)
>> -		clk_disable(priv->clk);
>> -
>> +	mutex_lock(&priv->lock);
>>  	/* Stop timer */
>>  	regmap_update_bits(priv->regmap, TIM_CR1, TIM_CR1_ARPE, 0);
>>  	regmap_update_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN, 0);
>> @@ -181,6 +184,12 @@ static void stm32_timer_stop(struct stm32_timer_trigger *priv)
>>  
>>  	/* Make sure that registers are updated */
>>  	regmap_update_bits(priv->regmap, TIM_EGR, TIM_EGR_UG, TIM_EGR_UG);
>> +
>> +	if (priv->clk_enabled) {
>> +		priv->clk_enabled = false;
>> +		clk_disable(priv->clk);
>> +	}
>> +	mutex_unlock(&priv->lock);
>>  }
>>  
>>  static ssize_t stm32_tt_store_frequency(struct device *dev,
>> @@ -295,11 +304,18 @@ static ssize_t stm32_tt_store_master_mode(struct device *dev,
>>  	for (i = 0; i <= master_mode_max; i++) {
>>  		if (!strncmp(master_mode_table[i], buf,
>>  			     strlen(master_mode_table[i]))) {
>> +			mutex_lock(&priv->lock);
>> +			if (!priv->clk_enabled) {
>> +				/* Clock should be enabled first */
>> +				priv->clk_enabled = true;
>> +				clk_enable(priv->clk);
>> +			}
>>  			regmap_update_bits(priv->regmap, TIM_CR2, mask,
>>  					   i << shift);
>>  			/* Make sure that registers are updated */
>>  			regmap_update_bits(priv->regmap, TIM_EGR,
>>  					   TIM_EGR_UG, TIM_EGR_UG);
>> +			mutex_unlock(&priv->lock);
>>  			return len;
>>  		}
>>  	}
>> @@ -357,11 +373,21 @@ static const struct attribute_group *stm32_trigger_attr_groups[] = {
>>  static const struct iio_trigger_ops timer_trigger_ops = {
>>  };
>>  
>> -static int stm32_setup_iio_triggers(struct stm32_timer_trigger *priv)
>> +static void stm32_unregister_iio_triggers(struct stm32_timer_trigger *priv)
>> +{
>> +	struct iio_trigger *tr;
>> +
>> +	list_for_each_entry(tr, &priv->tr_list, alloc_list)
>> +		iio_trigger_unregister(tr);
>> +}
>> +
>> +static int stm32_register_iio_triggers(struct stm32_timer_trigger *priv)
>>  {
>>  	int ret;
>>  	const char * const *cur = priv->triggers;
>>  
>> +	INIT_LIST_HEAD(&priv->tr_list);
>> +
>>  	while (cur && *cur) {
>>  		struct iio_trigger *trig;
>>  		bool cur_is_trgo = stm32_timer_is_trgo_name(*cur);
>> @@ -388,9 +414,13 @@ static int stm32_setup_iio_triggers(struct stm32_timer_trigger *priv)
>>  
>>  		iio_trigger_set_drvdata(trig, priv);
>>  
>> -		ret = devm_iio_trigger_register(priv->dev, trig);
>> -		if (ret)
>> +		ret = iio_trigger_register(trig);
>> +		if (ret) {
>> +			stm32_unregister_iio_triggers(priv);
>>  			return ret;
>> +		}
>> +
>> +		list_add_tail(&trig->alloc_list, &priv->tr_list);
>>  		cur++;
>>  	}
>>  
>> @@ -437,7 +467,6 @@ static int stm32_counter_write_raw(struct iio_dev *indio_dev,
>>  				   int val, int val2, long mask)
>>  {
>>  	struct stm32_timer_trigger *priv = iio_priv(indio_dev);
>> -	u32 dat;
>>  
>>  	switch (mask) {
>>  	case IIO_CHAN_INFO_RAW:
>> @@ -448,19 +477,23 @@ static int stm32_counter_write_raw(struct iio_dev *indio_dev,
>>  		return -EINVAL;
>>  
>>  	case IIO_CHAN_INFO_ENABLE:
>> +		mutex_lock(&priv->lock);
>>  		if (val) {
>> -			regmap_read(priv->regmap, TIM_CR1, &dat);
>> -			if (!(dat & TIM_CR1_CEN))
>> +			if (!priv->clk_enabled) {
>> +				priv->clk_enabled = true;
>>  				clk_enable(priv->clk);
>> +			}
>>  			regmap_update_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN,
>>  					   TIM_CR1_CEN);
>>  		} else {
>> -			regmap_read(priv->regmap, TIM_CR1, &dat);
>>  			regmap_update_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN,
>>  					   0);
>> -			if (dat & TIM_CR1_CEN)
>> +			if (priv->clk_enabled) {
>> +				priv->clk_enabled = false;
>>  				clk_disable(priv->clk);
>> +			}
>>  		}
>> +		mutex_unlock(&priv->lock);
>>  		return 0;
>>  	}
>>  
>> @@ -556,7 +589,6 @@ static int stm32_set_enable_mode(struct iio_dev *indio_dev,
>>  {
>>  	struct stm32_timer_trigger *priv = iio_priv(indio_dev);
>>  	int sms = stm32_enable_mode2sms(mode);
>> -	u32 val;
>>  
>>  	if (sms < 0)
>>  		return sms;
>> @@ -564,11 +596,12 @@ static int stm32_set_enable_mode(struct iio_dev *indio_dev,
>>  	 * Triggered mode sets CEN bit automatically by hardware. So, first
>>  	 * enable counter clock, so it can use it. Keeps it in sync with CEN.
>>  	 */
>> -	if (sms == 6) {
>> -		regmap_read(priv->regmap, TIM_CR1, &val);
>> -		if (!(val & TIM_CR1_CEN))
>> -			clk_enable(priv->clk);
>> +	mutex_lock(&priv->lock);
>> +	if (sms == 6 && !priv->clk_enabled) {
>> +		clk_enable(priv->clk);
>> +		priv->clk_enabled = true;
>>  	}
>> +	mutex_unlock(&priv->lock);
>>  
>>  	regmap_update_bits(priv->regmap, TIM_SMCR, TIM_SMCR_SMS, sms);
>>  
>> @@ -752,8 +785,9 @@ static int stm32_timer_trigger_probe(struct platform_device *pdev)
>>  	priv->triggers = triggers_table[index];
>>  	priv->valids = cfg->valids_table[index];
>>  	stm32_timer_detect_trgo2(priv);
>> +	mutex_init(&priv->lock);
>>  
>> -	ret = stm32_setup_iio_triggers(priv);
>> +	ret = stm32_register_iio_triggers(priv);
>>  	if (ret)
>>  		return ret;
>>  
>> @@ -762,6 +796,25 @@ static int stm32_timer_trigger_probe(struct platform_device *pdev)
>>  	return 0;
>>  }
>>  
>> +static int stm32_timer_trigger_remove(struct platform_device *pdev)
>> +{
>> +	struct stm32_timer_trigger *priv = platform_get_drvdata(pdev);
>> +	u32 val;
>> +
>> +	/* Unregister triggers before everything can be safely turned off */
>> +	stm32_unregister_iio_triggers(priv);
>> +
>> +	/* Check if nobody else use the timer, then disable it */
>> +	regmap_read(priv->regmap, TIM_CCER, &val);
>> +	if (!(val & TIM_CCER_CCXE))
>> +		regmap_update_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN, 0);
>> +
>> +	if (priv->clk_enabled)
>> +		clk_disable(priv->clk);
>> +
>> +	return 0;
>> +}
>> +
>>  static const struct stm32_timer_trigger_cfg stm32_timer_trg_cfg = {
>>  	.valids_table = valids_table,
>>  	.num_valids_table = ARRAY_SIZE(valids_table),
>> @@ -786,6 +839,7 @@ MODULE_DEVICE_TABLE(of, stm32_trig_of_match);
>>  
>>  static struct platform_driver stm32_timer_trigger_driver = {
>>  	.probe = stm32_timer_trigger_probe,
>> +	.remove = stm32_timer_trigger_remove,
>>  	.driver = {
>>  		.name = "stm32-timer-trigger",
>>  		.of_match_table = stm32_trig_of_match,
> 

Re: [PATCH v2] iio: trigger: stm32-timer: enable clock when in master mode

[Jonathan Cameron]

On Fri, 14 Feb 2020 15:50:57 +0100
Fabrice Gasnier <fabrice.gasnier@st.com> wrote:

> On 2/14/20 3:20 PM, Jonathan Cameron wrote:
> > On Fri, 7 Feb 2020 17:30:31 +0100
> > Fabrice Gasnier <fabrice.gasnier@st.com> wrote:
> >   
> >> Clock should be enabled as soon as using master modes, even before
> >> enabling timer. Or, this may provoke bad behavior on the other end
> >> (slave timer). Then, introduce 'clk_enabled' flag, instead of relying
> >> on CR1 EN bit, to keep track of clock being enabled (balanced refcount).
> >> Propagate this anywhere else in the driver.
> >>
> >> Also add 'remove' routine to stop timer and disable clock in case it
> >> has been left enabled. Enforce the user interface has been unregistered
> >> in the remove routine, before disabling the hardware to avoid possible
> >> race. So, remove use of devm_ variant to register triggers and unregister
> >> them before the hardware gets disabled [1].
> >> [1] https://patchwork.kernel.org/patch/9956247/
> >>
> >> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>  
> > Patch looks fine. So is this a fix for a known issue or a theoretical one?
> > Just a question of whether to line it up for an rc, or wait for next
> > merge window.  Ideally please give me a fixes tag as well if this
> > fixes a problem that is being seen in the wild.  
> 
> Hi Jonathan,
> 
> I got no complain for now. So I think this is fine to wait for the next
> merge window.
Hi Fabrice,

Great.  Unfortunately this has crossed with other changes on the driver.
Would you mind doing a rebase on my testing branch?  It looked fairly
simple but I wasn't totally confident to do it without being able
to test!

Thanks,

Jonathan

> 
> Thanks,
> Fabrice
> 
> > 
> > Thanks,
> > 
> > Jonathan
> >   
> >> ---
> >> Changes in v2:
> >> - enforce the user interface has been unregistered in the remove routine,
> >>   before disabling the hardware to avoid possible race.
> >> ---
> >>  drivers/iio/trigger/stm32-timer-trigger.c | 98 ++++++++++++++++++++++++-------
> >>  1 file changed, 76 insertions(+), 22 deletions(-)
> >>
> >> diff --git a/drivers/iio/trigger/stm32-timer-trigger.c b/drivers/iio/trigger/stm32-timer-trigger.c
> >> index a5dfe65..473853e 100644
> >> --- a/drivers/iio/trigger/stm32-timer-trigger.c
> >> +++ b/drivers/iio/trigger/stm32-timer-trigger.c
> >> @@ -79,10 +79,13 @@ struct stm32_timer_trigger {
> >>  	struct device *dev;
> >>  	struct regmap *regmap;
> >>  	struct clk *clk;
> >> +	bool clk_enabled;
> >>  	u32 max_arr;
> >>  	const void *triggers;
> >>  	const void *valids;
> >>  	bool has_trgo2;
> >> +	struct mutex lock; /* concurrent sysfs configuration */
> >> +	struct list_head tr_list;
> >>  };
> >>  
> >>  struct stm32_timer_trigger_cfg {
> >> @@ -106,7 +109,7 @@ static int stm32_timer_start(struct stm32_timer_trigger *priv,
> >>  {
> >>  	unsigned long long prd, div;
> >>  	int prescaler = 0;
> >> -	u32 ccer, cr1;
> >> +	u32 ccer;
> >>  
> >>  	/* Period and prescaler values depends of clock rate */
> >>  	div = (unsigned long long)clk_get_rate(priv->clk);
> >> @@ -136,9 +139,11 @@ static int stm32_timer_start(struct stm32_timer_trigger *priv,
> >>  	if (ccer & TIM_CCER_CCXE)
> >>  		return -EBUSY;
> >>  
> >> -	regmap_read(priv->regmap, TIM_CR1, &cr1);
> >> -	if (!(cr1 & TIM_CR1_CEN))
> >> +	mutex_lock(&priv->lock);
> >> +	if (!priv->clk_enabled) {
> >> +		priv->clk_enabled = true;
> >>  		clk_enable(priv->clk);
> >> +	}
> >>  
> >>  	regmap_write(priv->regmap, TIM_PSC, prescaler);
> >>  	regmap_write(priv->regmap, TIM_ARR, prd - 1);
> >> @@ -157,22 +162,20 @@ static int stm32_timer_start(struct stm32_timer_trigger *priv,
> >>  
> >>  	/* Enable controller */
> >>  	regmap_update_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN, TIM_CR1_CEN);
> >> +	mutex_unlock(&priv->lock);
> >>  
> >>  	return 0;
> >>  }
> >>  
> >>  static void stm32_timer_stop(struct stm32_timer_trigger *priv)
> >>  {
> >> -	u32 ccer, cr1;
> >> +	u32 ccer;
> >>  
> >>  	regmap_read(priv->regmap, TIM_CCER, &ccer);
> >>  	if (ccer & TIM_CCER_CCXE)
> >>  		return;
> >>  
> >> -	regmap_read(priv->regmap, TIM_CR1, &cr1);
> >> -	if (cr1 & TIM_CR1_CEN)
> >> -		clk_disable(priv->clk);
> >> -
> >> +	mutex_lock(&priv->lock);
> >>  	/* Stop timer */
> >>  	regmap_update_bits(priv->regmap, TIM_CR1, TIM_CR1_ARPE, 0);
> >>  	regmap_update_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN, 0);
> >> @@ -181,6 +184,12 @@ static void stm32_timer_stop(struct stm32_timer_trigger *priv)
> >>  
> >>  	/* Make sure that registers are updated */
> >>  	regmap_update_bits(priv->regmap, TIM_EGR, TIM_EGR_UG, TIM_EGR_UG);
> >> +
> >> +	if (priv->clk_enabled) {
> >> +		priv->clk_enabled = false;
> >> +		clk_disable(priv->clk);
> >> +	}
> >> +	mutex_unlock(&priv->lock);
> >>  }
> >>  
> >>  static ssize_t stm32_tt_store_frequency(struct device *dev,
> >> @@ -295,11 +304,18 @@ static ssize_t stm32_tt_store_master_mode(struct device *dev,
> >>  	for (i = 0; i <= master_mode_max; i++) {
> >>  		if (!strncmp(master_mode_table[i], buf,
> >>  			     strlen(master_mode_table[i]))) {
> >> +			mutex_lock(&priv->lock);
> >> +			if (!priv->clk_enabled) {
> >> +				/* Clock should be enabled first */
> >> +				priv->clk_enabled = true;
> >> +				clk_enable(priv->clk);
> >> +			}
> >>  			regmap_update_bits(priv->regmap, TIM_CR2, mask,
> >>  					   i << shift);
> >>  			/* Make sure that registers are updated */
> >>  			regmap_update_bits(priv->regmap, TIM_EGR,
> >>  					   TIM_EGR_UG, TIM_EGR_UG);
> >> +			mutex_unlock(&priv->lock);
> >>  			return len;
> >>  		}
> >>  	}
> >> @@ -357,11 +373,21 @@ static const struct attribute_group *stm32_trigger_attr_groups[] = {
> >>  static const struct iio_trigger_ops timer_trigger_ops = {
> >>  };
> >>  
> >> -static int stm32_setup_iio_triggers(struct stm32_timer_trigger *priv)
> >> +static void stm32_unregister_iio_triggers(struct stm32_timer_trigger *priv)
> >> +{
> >> +	struct iio_trigger *tr;
> >> +
> >> +	list_for_each_entry(tr, &priv->tr_list, alloc_list)
> >> +		iio_trigger_unregister(tr);
> >> +}
> >> +
> >> +static int stm32_register_iio_triggers(struct stm32_timer_trigger *priv)
> >>  {
> >>  	int ret;
> >>  	const char * const *cur = priv->triggers;
> >>  
> >> +	INIT_LIST_HEAD(&priv->tr_list);
> >> +
> >>  	while (cur && *cur) {
> >>  		struct iio_trigger *trig;
> >>  		bool cur_is_trgo = stm32_timer_is_trgo_name(*cur);
> >> @@ -388,9 +414,13 @@ static int stm32_setup_iio_triggers(struct stm32_timer_trigger *priv)
> >>  
> >>  		iio_trigger_set_drvdata(trig, priv);
> >>  
> >> -		ret = devm_iio_trigger_register(priv->dev, trig);
> >> -		if (ret)
> >> +		ret = iio_trigger_register(trig);
> >> +		if (ret) {
> >> +			stm32_unregister_iio_triggers(priv);
> >>  			return ret;
> >> +		}
> >> +
> >> +		list_add_tail(&trig->alloc_list, &priv->tr_list);
> >>  		cur++;
> >>  	}
> >>  
> >> @@ -437,7 +467,6 @@ static int stm32_counter_write_raw(struct iio_dev *indio_dev,
> >>  				   int val, int val2, long mask)
> >>  {
> >>  	struct stm32_timer_trigger *priv = iio_priv(indio_dev);
> >> -	u32 dat;
> >>  
> >>  	switch (mask) {
> >>  	case IIO_CHAN_INFO_RAW:
> >> @@ -448,19 +477,23 @@ static int stm32_counter_write_raw(struct iio_dev *indio_dev,
> >>  		return -EINVAL;
> >>  
> >>  	case IIO_CHAN_INFO_ENABLE:
> >> +		mutex_lock(&priv->lock);
> >>  		if (val) {
> >> -			regmap_read(priv->regmap, TIM_CR1, &dat);
> >> -			if (!(dat & TIM_CR1_CEN))
> >> +			if (!priv->clk_enabled) {
> >> +				priv->clk_enabled = true;
> >>  				clk_enable(priv->clk);
> >> +			}
> >>  			regmap_update_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN,
> >>  					   TIM_CR1_CEN);
> >>  		} else {
> >> -			regmap_read(priv->regmap, TIM_CR1, &dat);
> >>  			regmap_update_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN,
> >>  					   0);
> >> -			if (dat & TIM_CR1_CEN)
> >> +			if (priv->clk_enabled) {
> >> +				priv->clk_enabled = false;
> >>  				clk_disable(priv->clk);
> >> +			}
> >>  		}
> >> +		mutex_unlock(&priv->lock);
> >>  		return 0;
> >>  	}
> >>  
> >> @@ -556,7 +589,6 @@ static int stm32_set_enable_mode(struct iio_dev *indio_dev,
> >>  {
> >>  	struct stm32_timer_trigger *priv = iio_priv(indio_dev);
> >>  	int sms = stm32_enable_mode2sms(mode);
> >> -	u32 val;
> >>  
> >>  	if (sms < 0)
> >>  		return sms;
> >> @@ -564,11 +596,12 @@ static int stm32_set_enable_mode(struct iio_dev *indio_dev,
> >>  	 * Triggered mode sets CEN bit automatically by hardware. So, first
> >>  	 * enable counter clock, so it can use it. Keeps it in sync with CEN.
> >>  	 */
> >> -	if (sms == 6) {
> >> -		regmap_read(priv->regmap, TIM_CR1, &val);
> >> -		if (!(val & TIM_CR1_CEN))
> >> -			clk_enable(priv->clk);
> >> +	mutex_lock(&priv->lock);
> >> +	if (sms == 6 && !priv->clk_enabled) {
> >> +		clk_enable(priv->clk);
> >> +		priv->clk_enabled = true;
> >>  	}
> >> +	mutex_unlock(&priv->lock);
> >>  
> >>  	regmap_update_bits(priv->regmap, TIM_SMCR, TIM_SMCR_SMS, sms);
> >>  
> >> @@ -752,8 +785,9 @@ static int stm32_timer_trigger_probe(struct platform_device *pdev)
> >>  	priv->triggers = triggers_table[index];
> >>  	priv->valids = cfg->valids_table[index];
> >>  	stm32_timer_detect_trgo2(priv);
> >> +	mutex_init(&priv->lock);
> >>  
> >> -	ret = stm32_setup_iio_triggers(priv);
> >> +	ret = stm32_register_iio_triggers(priv);
> >>  	if (ret)
> >>  		return ret;
> >>  
> >> @@ -762,6 +796,25 @@ static int stm32_timer_trigger_probe(struct platform_device *pdev)
> >>  	return 0;
> >>  }
> >>  
> >> +static int stm32_timer_trigger_remove(struct platform_device *pdev)
> >> +{
> >> +	struct stm32_timer_trigger *priv = platform_get_drvdata(pdev);
> >> +	u32 val;
> >> +
> >> +	/* Unregister triggers before everything can be safely turned off */
> >> +	stm32_unregister_iio_triggers(priv);
> >> +
> >> +	/* Check if nobody else use the timer, then disable it */
> >> +	regmap_read(priv->regmap, TIM_CCER, &val);
> >> +	if (!(val & TIM_CCER_CCXE))
> >> +		regmap_update_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN, 0);
> >> +
> >> +	if (priv->clk_enabled)
> >> +		clk_disable(priv->clk);
> >> +
> >> +	return 0;
> >> +}
> >> +
> >>  static const struct stm32_timer_trigger_cfg stm32_timer_trg_cfg = {
> >>  	.valids_table = valids_table,
> >>  	.num_valids_table = ARRAY_SIZE(valids_table),
> >> @@ -786,6 +839,7 @@ MODULE_DEVICE_TABLE(of, stm32_trig_of_match);
> >>  
> >>  static struct platform_driver stm32_timer_trigger_driver = {
> >>  	.probe = stm32_timer_trigger_probe,
> >> +	.remove = stm32_timer_trigger_remove,
> >>  	.driver = {
> >>  		.name = "stm32-timer-trigger",
> >>  		.of_match_table = stm32_trig_of_match,  
> >   

Re: [PATCH v2] iio: trigger: stm32-timer: enable clock when in master mode

[Fabrice Gasnier]

On 2/14/20 4:14 PM, Jonathan Cameron wrote:
> On Fri, 14 Feb 2020 15:50:57 +0100
> Fabrice Gasnier <fabrice.gasnier@st.com> wrote:
> 
>> On 2/14/20 3:20 PM, Jonathan Cameron wrote:
>>> On Fri, 7 Feb 2020 17:30:31 +0100
>>> Fabrice Gasnier <fabrice.gasnier@st.com> wrote:
>>>   
>>>> Clock should be enabled as soon as using master modes, even before
>>>> enabling timer. Or, this may provoke bad behavior on the other end
>>>> (slave timer). Then, introduce 'clk_enabled' flag, instead of relying
>>>> on CR1 EN bit, to keep track of clock being enabled (balanced refcount).
>>>> Propagate this anywhere else in the driver.
>>>>
>>>> Also add 'remove' routine to stop timer and disable clock in case it
>>>> has been left enabled. Enforce the user interface has been unregistered
>>>> in the remove routine, before disabling the hardware to avoid possible
>>>> race. So, remove use of devm_ variant to register triggers and unregister
>>>> them before the hardware gets disabled [1].
>>>> [1] https://patchwork.kernel.org/patch/9956247/
>>>>
>>>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>  
>>> Patch looks fine. So is this a fix for a known issue or a theoretical one?
>>> Just a question of whether to line it up for an rc, or wait for next
>>> merge window.  Ideally please give me a fixes tag as well if this
>>> fixes a problem that is being seen in the wild.  
>>
>> Hi Jonathan,
>>
>> I got no complain for now. So I think this is fine to wait for the next
>> merge window.
> Hi Fabrice,
> 
> Great.  Unfortunately this has crossed with other changes on the driver.
> Would you mind doing a rebase on my testing branch?  It looked fairly
> simple but I wasn't totally confident to do it without being able
> to test!

Yes, I'll do this shortly ;-)

Thanks,
Fabrice
> 
> Thanks,
> 
> Jonathan
> 
>>
>> Thanks,
>> Fabrice
>>
>>>
>>> Thanks,
>>>
>>> Jonathan
>>>   
>>>> ---
>>>> Changes in v2:
>>>> - enforce the user interface has been unregistered in the remove routine,
>>>>   before disabling the hardware to avoid possible race.
>>>> ---
>>>>  drivers/iio/trigger/stm32-timer-trigger.c | 98 ++++++++++++++++++++++++-------
>>>>  1 file changed, 76 insertions(+), 22 deletions(-)
>>>>
>>>> diff --git a/drivers/iio/trigger/stm32-timer-trigger.c b/drivers/iio/trigger/stm32-timer-trigger.c
>>>> index a5dfe65..473853e 100644
>>>> --- a/drivers/iio/trigger/stm32-timer-trigger.c
>>>> +++ b/drivers/iio/trigger/stm32-timer-trigger.c
>>>> @@ -79,10 +79,13 @@ struct stm32_timer_trigger {
>>>>  	struct device *dev;
>>>>  	struct regmap *regmap;
>>>>  	struct clk *clk;
>>>> +	bool clk_enabled;
>>>>  	u32 max_arr;
>>>>  	const void *triggers;
>>>>  	const void *valids;
>>>>  	bool has_trgo2;
>>>> +	struct mutex lock; /* concurrent sysfs configuration */
>>>> +	struct list_head tr_list;
>>>>  };
>>>>  
>>>>  struct stm32_timer_trigger_cfg {
>>>> @@ -106,7 +109,7 @@ static int stm32_timer_start(struct stm32_timer_trigger *priv,
>>>>  {
>>>>  	unsigned long long prd, div;
>>>>  	int prescaler = 0;
>>>> -	u32 ccer, cr1;
>>>> +	u32 ccer;
>>>>  
>>>>  	/* Period and prescaler values depends of clock rate */
>>>>  	div = (unsigned long long)clk_get_rate(priv->clk);
>>>> @@ -136,9 +139,11 @@ static int stm32_timer_start(struct stm32_timer_trigger *priv,
>>>>  	if (ccer & TIM_CCER_CCXE)
>>>>  		return -EBUSY;
>>>>  
>>>> -	regmap_read(priv->regmap, TIM_CR1, &cr1);
>>>> -	if (!(cr1 & TIM_CR1_CEN))
>>>> +	mutex_lock(&priv->lock);
>>>> +	if (!priv->clk_enabled) {
>>>> +		priv->clk_enabled = true;
>>>>  		clk_enable(priv->clk);
>>>> +	}
>>>>  
>>>>  	regmap_write(priv->regmap, TIM_PSC, prescaler);
>>>>  	regmap_write(priv->regmap, TIM_ARR, prd - 1);
>>>> @@ -157,22 +162,20 @@ static int stm32_timer_start(struct stm32_timer_trigger *priv,
>>>>  
>>>>  	/* Enable controller */
>>>>  	regmap_update_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN, TIM_CR1_CEN);
>>>> +	mutex_unlock(&priv->lock);
>>>>  
>>>>  	return 0;
>>>>  }
>>>>  
>>>>  static void stm32_timer_stop(struct stm32_timer_trigger *priv)
>>>>  {
>>>> -	u32 ccer, cr1;
>>>> +	u32 ccer;
>>>>  
>>>>  	regmap_read(priv->regmap, TIM_CCER, &ccer);
>>>>  	if (ccer & TIM_CCER_CCXE)
>>>>  		return;
>>>>  
>>>> -	regmap_read(priv->regmap, TIM_CR1, &cr1);
>>>> -	if (cr1 & TIM_CR1_CEN)
>>>> -		clk_disable(priv->clk);
>>>> -
>>>> +	mutex_lock(&priv->lock);
>>>>  	/* Stop timer */
>>>>  	regmap_update_bits(priv->regmap, TIM_CR1, TIM_CR1_ARPE, 0);
>>>>  	regmap_update_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN, 0);
>>>> @@ -181,6 +184,12 @@ static void stm32_timer_stop(struct stm32_timer_trigger *priv)
>>>>  
>>>>  	/* Make sure that registers are updated */
>>>>  	regmap_update_bits(priv->regmap, TIM_EGR, TIM_EGR_UG, TIM_EGR_UG);
>>>> +
>>>> +	if (priv->clk_enabled) {
>>>> +		priv->clk_enabled = false;
>>>> +		clk_disable(priv->clk);
>>>> +	}
>>>> +	mutex_unlock(&priv->lock);
>>>>  }
>>>>  
>>>>  static ssize_t stm32_tt_store_frequency(struct device *dev,
>>>> @@ -295,11 +304,18 @@ static ssize_t stm32_tt_store_master_mode(struct device *dev,
>>>>  	for (i = 0; i <= master_mode_max; i++) {
>>>>  		if (!strncmp(master_mode_table[i], buf,
>>>>  			     strlen(master_mode_table[i]))) {
>>>> +			mutex_lock(&priv->lock);
>>>> +			if (!priv->clk_enabled) {
>>>> +				/* Clock should be enabled first */
>>>> +				priv->clk_enabled = true;
>>>> +				clk_enable(priv->clk);
>>>> +			}
>>>>  			regmap_update_bits(priv->regmap, TIM_CR2, mask,
>>>>  					   i << shift);
>>>>  			/* Make sure that registers are updated */
>>>>  			regmap_update_bits(priv->regmap, TIM_EGR,
>>>>  					   TIM_EGR_UG, TIM_EGR_UG);
>>>> +			mutex_unlock(&priv->lock);
>>>>  			return len;
>>>>  		}
>>>>  	}
>>>> @@ -357,11 +373,21 @@ static const struct attribute_group *stm32_trigger_attr_groups[] = {
>>>>  static const struct iio_trigger_ops timer_trigger_ops = {
>>>>  };
>>>>  
>>>> -static int stm32_setup_iio_triggers(struct stm32_timer_trigger *priv)
>>>> +static void stm32_unregister_iio_triggers(struct stm32_timer_trigger *priv)
>>>> +{
>>>> +	struct iio_trigger *tr;
>>>> +
>>>> +	list_for_each_entry(tr, &priv->tr_list, alloc_list)
>>>> +		iio_trigger_unregister(tr);
>>>> +}
>>>> +
>>>> +static int stm32_register_iio_triggers(struct stm32_timer_trigger *priv)
>>>>  {
>>>>  	int ret;
>>>>  	const char * const *cur = priv->triggers;
>>>>  
>>>> +	INIT_LIST_HEAD(&priv->tr_list);
>>>> +
>>>>  	while (cur && *cur) {
>>>>  		struct iio_trigger *trig;
>>>>  		bool cur_is_trgo = stm32_timer_is_trgo_name(*cur);
>>>> @@ -388,9 +414,13 @@ static int stm32_setup_iio_triggers(struct stm32_timer_trigger *priv)
>>>>  
>>>>  		iio_trigger_set_drvdata(trig, priv);
>>>>  
>>>> -		ret = devm_iio_trigger_register(priv->dev, trig);
>>>> -		if (ret)
>>>> +		ret = iio_trigger_register(trig);
>>>> +		if (ret) {
>>>> +			stm32_unregister_iio_triggers(priv);
>>>>  			return ret;
>>>> +		}
>>>> +
>>>> +		list_add_tail(&trig->alloc_list, &priv->tr_list);
>>>>  		cur++;
>>>>  	}
>>>>  
>>>> @@ -437,7 +467,6 @@ static int stm32_counter_write_raw(struct iio_dev *indio_dev,
>>>>  				   int val, int val2, long mask)
>>>>  {
>>>>  	struct stm32_timer_trigger *priv = iio_priv(indio_dev);
>>>> -	u32 dat;
>>>>  
>>>>  	switch (mask) {
>>>>  	case IIO_CHAN_INFO_RAW:
>>>> @@ -448,19 +477,23 @@ static int stm32_counter_write_raw(struct iio_dev *indio_dev,
>>>>  		return -EINVAL;
>>>>  
>>>>  	case IIO_CHAN_INFO_ENABLE:
>>>> +		mutex_lock(&priv->lock);
>>>>  		if (val) {
>>>> -			regmap_read(priv->regmap, TIM_CR1, &dat);
>>>> -			if (!(dat & TIM_CR1_CEN))
>>>> +			if (!priv->clk_enabled) {
>>>> +				priv->clk_enabled = true;
>>>>  				clk_enable(priv->clk);
>>>> +			}
>>>>  			regmap_update_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN,
>>>>  					   TIM_CR1_CEN);
>>>>  		} else {
>>>> -			regmap_read(priv->regmap, TIM_CR1, &dat);
>>>>  			regmap_update_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN,
>>>>  					   0);
>>>> -			if (dat & TIM_CR1_CEN)
>>>> +			if (priv->clk_enabled) {
>>>> +				priv->clk_enabled = false;
>>>>  				clk_disable(priv->clk);
>>>> +			}
>>>>  		}
>>>> +		mutex_unlock(&priv->lock);
>>>>  		return 0;
>>>>  	}
>>>>  
>>>> @@ -556,7 +589,6 @@ static int stm32_set_enable_mode(struct iio_dev *indio_dev,
>>>>  {
>>>>  	struct stm32_timer_trigger *priv = iio_priv(indio_dev);
>>>>  	int sms = stm32_enable_mode2sms(mode);
>>>> -	u32 val;
>>>>  
>>>>  	if (sms < 0)
>>>>  		return sms;
>>>> @@ -564,11 +596,12 @@ static int stm32_set_enable_mode(struct iio_dev *indio_dev,
>>>>  	 * Triggered mode sets CEN bit automatically by hardware. So, first
>>>>  	 * enable counter clock, so it can use it. Keeps it in sync with CEN.
>>>>  	 */
>>>> -	if (sms == 6) {
>>>> -		regmap_read(priv->regmap, TIM_CR1, &val);
>>>> -		if (!(val & TIM_CR1_CEN))
>>>> -			clk_enable(priv->clk);
>>>> +	mutex_lock(&priv->lock);
>>>> +	if (sms == 6 && !priv->clk_enabled) {
>>>> +		clk_enable(priv->clk);
>>>> +		priv->clk_enabled = true;
>>>>  	}
>>>> +	mutex_unlock(&priv->lock);
>>>>  
>>>>  	regmap_update_bits(priv->regmap, TIM_SMCR, TIM_SMCR_SMS, sms);
>>>>  
>>>> @@ -752,8 +785,9 @@ static int stm32_timer_trigger_probe(struct platform_device *pdev)
>>>>  	priv->triggers = triggers_table[index];
>>>>  	priv->valids = cfg->valids_table[index];
>>>>  	stm32_timer_detect_trgo2(priv);
>>>> +	mutex_init(&priv->lock);
>>>>  
>>>> -	ret = stm32_setup_iio_triggers(priv);
>>>> +	ret = stm32_register_iio_triggers(priv);
>>>>  	if (ret)
>>>>  		return ret;
>>>>  
>>>> @@ -762,6 +796,25 @@ static int stm32_timer_trigger_probe(struct platform_device *pdev)
>>>>  	return 0;
>>>>  }
>>>>  
>>>> +static int stm32_timer_trigger_remove(struct platform_device *pdev)
>>>> +{
>>>> +	struct stm32_timer_trigger *priv = platform_get_drvdata(pdev);
>>>> +	u32 val;
>>>> +
>>>> +	/* Unregister triggers before everything can be safely turned off */
>>>> +	stm32_unregister_iio_triggers(priv);
>>>> +
>>>> +	/* Check if nobody else use the timer, then disable it */
>>>> +	regmap_read(priv->regmap, TIM_CCER, &val);
>>>> +	if (!(val & TIM_CCER_CCXE))
>>>> +		regmap_update_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN, 0);
>>>> +
>>>> +	if (priv->clk_enabled)
>>>> +		clk_disable(priv->clk);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>>  static const struct stm32_timer_trigger_cfg stm32_timer_trg_cfg = {
>>>>  	.valids_table = valids_table,
>>>>  	.num_valids_table = ARRAY_SIZE(valids_table),
>>>> @@ -786,6 +839,7 @@ MODULE_DEVICE_TABLE(of, stm32_trig_of_match);
>>>>  
>>>>  static struct platform_driver stm32_timer_trigger_driver = {
>>>>  	.probe = stm32_timer_trigger_probe,
>>>> +	.remove = stm32_timer_trigger_remove,
>>>>  	.driver = {
>>>>  		.name = "stm32-timer-trigger",
>>>>  		.of_match_table = stm32_trig_of_match,  
>>>   
>