On 1/9/25 11:49, Lee Jones wrote:
> On Wed, 18 Dec 2024, Fabrice Gasnier wrote:
>
>> Add support for STM32MP25 SoC. Use newly introduced compatible, to handle
>> new features.
>> Identification and hardware configuration registers allow to read the
>> timer version and capabilities (counter width, number of channels...).
>> So, rework the probe to avoid touching ARR register by simply read the
>> counter width when available. This may avoid messing with a possibly
>> running timer.
>> Also add useful bit fields to stm32-timers header file.
>>
>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@xxxxxxxxxxx>
>> ---
>> drivers/mfd/stm32-timers.c | 32 +++++++++++++++++++++++++++++++-
>> include/linux/mfd/stm32-timers.h | 9 +++++++++
>> 2 files changed, 40 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mfd/stm32-timers.c b/drivers/mfd/stm32-timers.c
>> index 650724e19b88..6f217c32482c 100644
>> --- a/drivers/mfd/stm32-timers.c
>> +++ b/drivers/mfd/stm32-timers.c
>> @@ -9,6 +9,7 @@
>> #include <linux/module.h>
>> #include <linux/of_platform.h>
>> #include <linux/platform_device.h>
>> +#include <linux/property.h>
>> #include <linux/reset.h>
>>
>> #define STM32_TIMERS_MAX_REGISTERS 0x3fc
>> @@ -173,6 +174,32 @@ static void stm32_timers_get_arr_size(struct stm32_timers *ddata)
>> regmap_write(ddata->regmap, TIM_ARR, arr);
>> }
>>
>> +static int stm32_timers_probe_hwcfgr(struct device *dev, struct stm32_timers *ddata)
>> +{
>> + u32 val;
>> +
>> + ddata->ipidr = (uintptr_t)device_get_match_data(dev);
>
> Are you sure this cast is needed?
Hi Lee,
Yes, I can see a warning pops-up without it:
drivers/mfd/stm32-timers.c:181:22: warning: assignment to ‘u32’ {aka
‘unsigned int’} from ‘const void *’ makes integer from pointer without a
cast [-Wint-conversion]
181 | ddata->ipidr = device_get_match_data(dev);
| ^
>
>> + if (!ddata->ipidr) {
>> + /* fallback to legacy method for probing counter width */
>
> Sentences start with uppercase chars.
Ack
>
>> + stm32_timers_get_arr_size(ddata);
>> + return 0;
>> + }
>> +
>> + regmap_read(ddata->regmap, TIM_IPIDR, &val);
>> + /* Sanity check on IP identification register */
>
> This seems obvious, thus superfluous.
Ack
>
>> + if (val != ddata->ipidr) {
>> + dev_err(dev, "Unexpected identification: %u\n", val);
>
> "Unexpected model number"?
> "Unsupported device detected"?
Ack
>
>> + return -EINVAL;
>> + }
>> +
>> + regmap_read(ddata->regmap, TIM_HWCFGR2, &val);
>
> '/n' here.
Ack
>
>> + /* Counter width in bits, max reload value is BIT(width) - 1 */
>> + ddata->max_arr = BIT(FIELD_GET(TIM_HWCFGR2_CNT_WIDTH, val)) - 1;
>> + dev_dbg(dev, "TIM width: %ld\n", FIELD_GET(TIM_HWCFGR2_CNT_WIDTH, val));
>
> How useful is this now the driver has been developed?
Well... that's just debug. I'll remove it and send a V3.
Thanks for reviewing,
BR,
Fabrice
>
>> + return 0;
>> +}
>> +
>> static int stm32_timers_dma_probe(struct device *dev,
>> struct stm32_timers *ddata)
>> {
>> @@ -285,7 +312,9 @@ static int stm32_timers_probe(struct platform_device *pdev)
>> if (IS_ERR(ddata->clk))
>> return PTR_ERR(ddata->clk);
>>
>> - stm32_timers_get_arr_size(ddata);
>> + ret = stm32_timers_probe_hwcfgr(dev, ddata);
>> + if (ret)
>> + return ret;
>>
>> ret = stm32_timers_irq_probe(pdev, ddata);
>> if (ret)
>> @@ -320,6 +349,7 @@ static void stm32_timers_remove(struct platform_device *pdev)
>>
>> static const struct of_device_id stm32_timers_of_match[] = {
>> { .compatible = "st,stm32-timers", },
>> + { .compatible = "st,stm32mp25-timers", .data = (void *)STM32MP25_TIM_IPIDR },
>> { /* end node */ },
>> };
>> MODULE_DEVICE_TABLE(of, stm32_timers_of_match);
>> diff --git a/include/linux/mfd/stm32-timers.h b/include/linux/mfd/stm32-timers.h
>> index f09ba598c97a..23b0cae4a9f8 100644
>> --- a/include/linux/mfd/stm32-timers.h
>> +++ b/include/linux/mfd/stm32-timers.h
>> @@ -33,6 +33,9 @@
>> #define TIM_DCR 0x48 /* DMA control register */
>> #define TIM_DMAR 0x4C /* DMA register for transfer */
>> #define TIM_TISEL 0x68 /* Input Selection */
>> +#define TIM_HWCFGR2 0x3EC /* hardware configuration 2 Reg (MP25) */
>> +#define TIM_HWCFGR1 0x3F0 /* hardware configuration 1 Reg (MP25) */
>> +#define TIM_IPIDR 0x3F8 /* IP identification Reg (MP25) */
>>
>> #define TIM_CR1_CEN BIT(0) /* Counter Enable */
>> #define TIM_CR1_DIR BIT(4) /* Counter Direction */
>> @@ -100,6 +103,9 @@
>> #define TIM_BDTR_BKF(x) (0xf << (16 + (x) * 4))
>> #define TIM_DCR_DBA GENMASK(4, 0) /* DMA base addr */
>> #define TIM_DCR_DBL GENMASK(12, 8) /* DMA burst len */
>> +#define TIM_HWCFGR1_NB_OF_CC GENMASK(3, 0) /* Capture/compare channels */
>> +#define TIM_HWCFGR1_NB_OF_DT GENMASK(7, 4) /* Complementary outputs & dead-time generators */
>> +#define TIM_HWCFGR2_CNT_WIDTH GENMASK(15, 8) /* Counter width */
>>
>> #define MAX_TIM_PSC 0xFFFF
>> #define MAX_TIM_ICPSC 0x3
>> @@ -113,6 +119,8 @@
>> #define TIM_BDTR_BKF_MASK 0xF
>> #define TIM_BDTR_BKF_SHIFT(x) (16 + (x) * 4)
>>
>> +#define STM32MP25_TIM_IPIDR 0x00120002
>> +
>> enum stm32_timers_dmas {
>> STM32_TIMERS_DMA_CH1,
>> STM32_TIMERS_DMA_CH2,
>> @@ -151,6 +159,7 @@ struct stm32_timers_dma {
>>
>> struct stm32_timers {
>> struct clk *clk;
>> + u32 ipidr;
>> struct regmap *regmap;
>> u32 max_arr;
>> struct stm32_timers_dma dma; /* Only to be used by the parent */
>> --
>> 2.25.1
>>
>
