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

Philipp Zabel
2025-10-30T13:40:45Z

On Mi, 2025-10-29 at 16:11 +0000, 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));

This could use regmap_set_bits().

> +
> + guard(spinlock_irqsave)(&mpfs_reset_lock);
>
> reg = readl(rst->base);
> reg |= BIT(id);
> writel(reg, rst->base);

Since I've just seen this in the i.MX8ULP series [1], it would be
cleaner to convert the aux driver to regmap as well. The readl/writel()
code paths could be dropped then.

[1] https://lore.kernel.org/lkml/20251029135229.890-1-laurentiumihalcea111@gmail.com/

[...]
> 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);

And this could use regmap_clear_bits().

regards
Philipp