Re: [PATCH 2/9] mfd: stm32-timers: add support for stm32mp25

[Lee Jones <lee@kernel.org>]

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@foss.st.com>
> ---
>  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?

> +	if (!ddata->ipidr) {
> +		/* fallback to legacy method for probing counter width */

Sentences start with uppercase chars.

> +		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.

> +	if (val != ddata->ipidr) {
> +		dev_err(dev, "Unexpected identification: %u\n", val);

"Unexpected model number"?
"Unsupported device detected"?

> +		return -EINVAL;
> +	}
> +
> +	regmap_read(ddata->regmap, TIM_HWCFGR2, &val);

'/n' here.

> +	/* 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?

> +	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
> 

-- 
Lee Jones [李琼斯]