[PATCH] counter: microchip-tcb-capture: Fix undefined counter channel state on probe

[PATCH] counter: microchip-tcb-capture: Fix undefined counter channel state on probe

[William Breathitt Gray]

Hardware initialize of the timer counter channel does not occur on probe
thus leaving the Count in an undefined state until the first
function_write() callback is executed. Fix this by performing the proper
hardware initialization during probe.

Fixes: 106b104137fd ("counter: Add microchip TCB capture counter")
Reported-by: Csókás Bence <csokas.bence@prolan.hu>
Closes: https://lore.kernel.org/all/bfa70e78-3cc3-4295-820b-3925c26135cb@prolan.hu/
Signed-off-by: William Breathitt Gray <wbg@kernel.org>
---
This should fix the issue where a user needs to set the count function
before they can use the counter. I don't have this hardware in person,
so please test this patch and let me know whether it works for you.
---
 drivers/counter/microchip-tcb-capture.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/counter/microchip-tcb-capture.c b/drivers/counter/microchip-tcb-capture.c
index 2f096a5b973d18edf5de5a2b33f2f72571deefb7..c391ac38b990939c6764a9120a4bd03289f68469 100644
--- a/drivers/counter/microchip-tcb-capture.c
+++ b/drivers/counter/microchip-tcb-capture.c
@@ -368,6 +368,25 @@ static int mchp_tc_probe(struct platform_device *pdev)
 			channel);
 	}
 
+	/* Disable Quadrature Decoder and position measure */
+	ret = regmap_update_bits(regmap, ATMEL_TC_BMR, ATMEL_TC_QDEN | ATMEL_TC_POSEN, 0);
+	if (ret)
+		return ret;
+
+	/* Setup the period capture mode */
+	ret = regmap_update_bits(regmap, ATMEL_TC_REG(priv->channel[0], CMR),
+				 ATMEL_TC_WAVE | ATMEL_TC_ABETRG | ATMEL_TC_CMR_MASK |
+				 ATMEL_TC_TCCLKS,
+				 ATMEL_TC_CMR_MASK);
+	if (ret)
+		return ret;
+
+	/* Enable clock and trigger counter */
+	ret = regmap_write(regmap, ATMEL_TC_REG(priv->channel[0], CCR),
+			   ATMEL_TC_CLKEN | ATMEL_TC_SWTRG);
+	if (ret)
+		return ret;
+
 	priv->tc_cfg = tcb_config;
 	priv->regmap = regmap;
 	counter->name = dev_name(&pdev->dev);

---
base-commit: 8744dcd4fc7800de2eb9369410470bb2930d4c14
change-id: 20250305-preset-capture-mode-microchip-tcb-capture-fa31550ef594

Best regards,
-- 
William Breathitt Gray <wbg@kernel.org>

Re: [PATCH] counter: microchip-tcb-capture: Fix undefined counter channel state on probe

[William Breathitt Gray]

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

On Wed, Mar 05, 2025 at 07:01:19PM +0900, William Breathitt Gray wrote:
> Hardware initialize of the timer counter channel does not occur on probe
> thus leaving the Count in an undefined state until the first
> function_write() callback is executed. Fix this by performing the proper
> hardware initialization during probe.
> 
> Fixes: 106b104137fd ("counter: Add microchip TCB capture counter")
> Reported-by: Csókás Bence <csokas.bence@prolan.hu>
> Closes: https://lore.kernel.org/all/bfa70e78-3cc3-4295-820b-3925c26135cb@prolan.hu/
> Signed-off-by: William Breathitt Gray <wbg@kernel.org>
> ---
> This should fix the issue where a user needs to set the count function
> before they can use the counter. I don't have this hardware in person,
> so please test this patch and let me know whether it works for you.

While developing this bug fix, I noticed the following code in the
mchp_tc_count_function_write() function:

	if (!priv->tc_cfg->has_gclk)
		cmr |= ATMEL_TC_TIMER_CLOCK2;
	else
		cmr |= ATMEL_TC_TIMER_CLOCK1;
	/* Setup the period capture mode */
	cmr |=  ATMEL_TC_CMR_MASK;
	cmr &= ~(ATMEL_TC_ABETRG | ATMEL_TC_XC0);

It looks like it's trying to choose the TCCLKS value by evaluating
has_gclk. However, a couple lines later the cmr value is masked by
ATMEL_TC_XC0 which will clobber the previous choice by resetting bit 0.

Is this a bug, or am I misunderstanding how the TCCLKS value is set by
these defines?

William Breathitt Gray

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

Re: [PATCH] counter: microchip-tcb-capture: Fix undefined counter channel state on probe

[Csókás Bence]

Hi,

thanks for fixing this up!

On 2025. 03. 05. 11:01, William Breathitt Gray wrote:
> Hardware initialize of the timer counter channel does not occur on probe
> thus leaving the Count in an undefined state until the first
> function_write() callback is executed. Fix this by performing the proper
> hardware initialization during probe.
> 
> Fixes: 106b104137fd ("counter: Add microchip TCB capture counter")
> Reported-by: Csókás Bence <csokas.bence@prolan.hu>
> Closes: https://lore.kernel.org/all/bfa70e78-3cc3-4295-820b-3925c26135cb@prolan.hu/
> Signed-off-by: William Breathitt Gray <wbg@kernel.org>
> ---
> This should fix the issue where a user needs to set the count function
> before they can use the counter. I don't have this hardware in person,
> so please test this patch and let me know whether it works for you.
> ---
>   drivers/counter/microchip-tcb-capture.c | 19 +++++++++++++++++++
>   1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/counter/microchip-tcb-capture.c b/drivers/counter/microchip-tcb-capture.c
> index 2f096a5b973d18edf5de5a2b33f2f72571deefb7..c391ac38b990939c6764a9120a4bd03289f68469 100644
> --- a/drivers/counter/microchip-tcb-capture.c
> +++ b/drivers/counter/microchip-tcb-capture.c
> @@ -368,6 +368,25 @@ static int mchp_tc_probe(struct platform_device *pdev)
>   			channel);
>   	}
>   
> +	/* Disable Quadrature Decoder and position measure */
> +	ret = regmap_update_bits(regmap, ATMEL_TC_BMR, ATMEL_TC_QDEN | ATMEL_TC_POSEN, 0);
> +	if (ret)
> +		return ret;
> +
> +	/* Setup the period capture mode */
> +	ret = regmap_update_bits(regmap, ATMEL_TC_REG(priv->channel[0], CMR),
> +				 ATMEL_TC_WAVE | ATMEL_TC_ABETRG | ATMEL_TC_CMR_MASK |
> +				 ATMEL_TC_TCCLKS,
> +				 ATMEL_TC_CMR_MASK);
> +	if (ret)
> +		return ret;
> +
> +	/* Enable clock and trigger counter */
> +	ret = regmap_write(regmap, ATMEL_TC_REG(priv->channel[0], CCR),
> +			   ATMEL_TC_CLKEN | ATMEL_TC_SWTRG);
> +	if (ret)
> +		return ret;
> +

This duplicates a lot of `mchp_tc_count_function_write()`. I'd much 
rather have this code in a separate function called something like 
`mchp_tc_setup_channels()`, that, depending on `priv->qdec_mode`, sets 
up the BMR, CCR and CMRs, and then have both probe() and 
function_write() call it. Or alternatively, have probe() call 
function_write() at the end, but that's not as nice.

> ---
> base-commit: 8744dcd4fc7800de2eb9369410470bb2930d4c14
> change-id: 20250305-preset-capture-mode-microchip-tcb-capture-fa31550ef594
> 
> Best regards,

Bence

Re: [PATCH] counter: microchip-tcb-capture: Fix undefined counter channel state on probe

[William Breathitt Gray]

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

On Wed, Mar 05, 2025 at 11:50:27AM +0100, Csókás Bence wrote:
> This duplicates a lot of `mchp_tc_count_function_write()`. I'd much rather
> have this code in a separate function called something like
> `mchp_tc_setup_channels()`, that, depending on `priv->qdec_mode`, sets up
> the BMR, CCR and CMRs, and then have both probe() and function_write() call
> it. Or alternatively, have probe() call function_write() at the end, but
> that's not as nice.

Hi Bence,

I agree, the mchp_tc_count_function_write() could be cleaned up and
divided into separate functions dedicated to configuring each mode
(perhaps regmap_update_bits() could be leveraged too), but that would be
a much more invasive update. For the sake of making backporting easy to
address this particular issue, I've kept the changes here localized to
just the probe() function. Once the fix is merged, someone can try
tackling a more proper refactor of the mchp_tc_count_function_write()
code.

William Breathitt Gray

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

Re: [PATCH] counter: microchip-tcb-capture: Fix undefined counter channel state on probe

[William Breathitt Gray]

On Wed, 05 Mar 2025 19:01:19 +0900, William Breathitt Gray wrote:
> Hardware initialize of the timer counter channel does not occur on probe
> thus leaving the Count in an undefined state until the first
> function_write() callback is executed. Fix this by performing the proper
> hardware initialization during probe.
> 
> 

Applied to counter-fixes.

[1/1] counter: microchip-tcb-capture: Fix undefined counter channel state on probe
      commit: c0c9c73434666dc99ee156b25e7e722150bee001

Best regards,
-- 
William Breathitt Gray <wbg@kernel.org>