| Message ID | 20250305-preset-capture-mode-microchip-tcb-capture-v1-1-632c95c6421e@kernel.org (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | counter: microchip-tcb-capture: Fix undefined counter channel state on probe | expand |
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
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
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
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,
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);
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(+) --- base-commit: 8744dcd4fc7800de2eb9369410470bb2930d4c14 change-id: 20250305-preset-capture-mode-microchip-tcb-capture-fa31550ef594 Best regards,