Re: [PATCH v6 1/7] reset: mpfs: add non-auxiliary bus probing

[claudiu beznea <claudiu.beznea@tuxon.dev>]

Hi, Conor,

On 10/29/25 18:11, Conor Dooley wrote:
> From: Conor Dooley <conor.dooley@microchip.com>
> 
> While the auxiliary bus was a nice bandaid, and meant that re-writing
> the representation of the clock regions in devicetree was not required,
> it has run its course. The "mss_top_sysreg" region that contains the
> clock and reset regions, also contains pinctrl and an interrupt
> controller, so the time has come rewrite the devicetree and probe the
> reset controller from an mfd devicetree node, rather than implement
> those drivers using the auxiliary bus. Wanting to avoid propagating this
> naive/incorrect description of the hardware to the new pic64gx SoC is a
> major motivating factor here.
> 
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> ---
> v6:
> - depend on MFD_SYSCON
> - return regmap_update_bits() result directly instead of an additional
>    return 0
> 
> v4:
> - Only use driver specific lock for non-regmap writes
> 
> v2:
> - Implement the request to use regmap_update_bits(). I found that I then
>    hated the read/write helpers since they were just bloat, so I ripped
>    them out. I replaced the regular spin_lock_irqsave() stuff with a
>    guard(spinlock_irqsave), since that's a simpler way of handling the two
>    different paths through such a trivial pair of functions.
> ---
>   drivers/reset/Kconfig      |  1 +
>   drivers/reset/reset-mpfs.c | 79 ++++++++++++++++++++++++++++++--------
>   2 files changed, 63 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> index 78b7078478d4..0ec4b7cd08d6 100644
> --- a/drivers/reset/Kconfig
> +++ b/drivers/reset/Kconfig
> @@ -200,6 +200,7 @@ config RESET_PISTACHIO
>   config RESET_POLARFIRE_SOC
>   	bool "Microchip PolarFire SoC (MPFS) Reset Driver"
>   	depends on MCHP_CLK_MPFS
> +	depends on MFD_SYSCON
>   	select AUXILIARY_BUS
>   	default MCHP_CLK_MPFS
>   	help
> diff --git a/drivers/reset/reset-mpfs.c b/drivers/reset/reset-mpfs.c
> index f6fa10e03ea8..25de7df55301 100644
> --- a/drivers/reset/reset-mpfs.c
> +++ b/drivers/reset/reset-mpfs.c
> @@ -7,13 +7,16 @@
>    *
>    */
>   #include <linux/auxiliary_bus.h>
> +#include <linux/cleanup.h>
>   #include <linux/delay.h>
>   #include <linux/io.h>
> +#include <linux/mfd/syscon.h>
>   #include <linux/module.h>
>   #include <linux/of.h>
>   #include <linux/platform_device.h>
> -#include <linux/slab.h>
> +#include <linux/regmap.h>
>   #include <linux/reset-controller.h>
> +#include <linux/slab.h>
>   #include <dt-bindings/clock/microchip,mpfs-clock.h>
>   #include <soc/microchip/mpfs.h>
>   
> @@ -27,11 +30,14 @@
>   #define MPFS_SLEEP_MIN_US	100
>   #define MPFS_SLEEP_MAX_US	200
>   
> +#define REG_SUBBLK_RESET_CR	0x88u
> +
>   /* block concurrent access to the soft reset register */
>   static DEFINE_SPINLOCK(mpfs_reset_lock);
>   
>   struct mpfs_reset {
>   	void __iomem *base;
> +	struct regmap *regmap;
>   	struct reset_controller_dev rcdev;
>   };
>   
> @@ -46,41 +52,46 @@ static inline struct mpfs_reset *to_mpfs_reset(struct reset_controller_dev *rcde
>   static int mpfs_assert(struct reset_controller_dev *rcdev, unsigned long id)
>   {
>   	struct mpfs_reset *rst = to_mpfs_reset(rcdev);
> -	unsigned long flags;
>   	u32 reg;
>   
> -	spin_lock_irqsave(&mpfs_reset_lock, flags);
> +	if (rst->regmap)
> +		return regmap_update_bits(rst->regmap, REG_SUBBLK_RESET_CR, BIT(id), BIT(id));
> +
> +	guard(spinlock_irqsave)(&mpfs_reset_lock);
>   
>   	reg = readl(rst->base);
>   	reg |= BIT(id);
>   	writel(reg, rst->base);
>   
> -	spin_unlock_irqrestore(&mpfs_reset_lock, flags);
> -
>   	return 0;
>   }
>   
>   static int mpfs_deassert(struct reset_controller_dev *rcdev, unsigned long id)
>   {
>   	struct mpfs_reset *rst = to_mpfs_reset(rcdev);
> -	unsigned long flags;
>   	u32 reg;
>   
> -	spin_lock_irqsave(&mpfs_reset_lock, flags);
> +	if (rst->regmap)
> +		return regmap_update_bits(rst->regmap, REG_SUBBLK_RESET_CR, BIT(id), 0);
> +
> +	guard(spinlock_irqsave)(&mpfs_reset_lock);
>   
>   	reg = readl(rst->base);
>   	reg &= ~BIT(id);
>   	writel(reg, rst->base);
>   
> -	spin_unlock_irqrestore(&mpfs_reset_lock, flags);
> -
>   	return 0;
>   }
>   
>   static int mpfs_status(struct reset_controller_dev *rcdev, unsigned long id)
>   {
>   	struct mpfs_reset *rst = to_mpfs_reset(rcdev);
> -	u32 reg = readl(rst->base);
> +	u32 reg;
> +
> +	if (rst->regmap)
> +		regmap_read(rst->regmap, REG_SUBBLK_RESET_CR, &reg);
> +	else
> +		reg = readl(rst->base);
>   
>   	/*
>   	 * It is safe to return here as MPFS_NUM_RESETS makes sure the sign bit
> @@ -130,11 +141,45 @@ static int mpfs_reset_xlate(struct reset_controller_dev *rcdev,
>   	return index - MPFS_PERIPH_OFFSET;
>   }
>   
> -static int mpfs_reset_probe(struct auxiliary_device *adev,
> -			    const struct auxiliary_device_id *id)
> +static int mpfs_reset_mfd_probe(struct platform_device *pdev)
>   {
> -	struct device *dev = &adev->dev;
>   	struct reset_controller_dev *rcdev;
> +	struct device *dev = &pdev->dev;
> +	struct mpfs_reset *rst;
> +
> +	rst = devm_kzalloc(dev, sizeof(*rst), GFP_KERNEL);
> +	if (!rst)
> +		return -ENOMEM;
> +
> +	rcdev = &rst->rcdev;
> +	rcdev->dev = dev;
> +	rcdev->ops = &mpfs_reset_ops;
> +
> +	rcdev->of_node = pdev->dev.parent->of_node;
> +	rcdev->of_reset_n_cells = 1;
> +	rcdev->of_xlate = mpfs_reset_xlate;
> +	rcdev->nr_resets = MPFS_NUM_RESETS;
> +
> +	rst->regmap = device_node_to_regmap(pdev->dev.parent->of_node);
> +	if (IS_ERR(rst->regmap))
> +		dev_err_probe(dev, PTR_ERR(rst->regmap), "Failed to find syscon regmap\n");

Do you want to continue registering the reset controller here? rcdev->base is 
NULL, thus the reset controller ops will fail, if I'm not wrong.

Thank you,
Claudiu