Re: [PATCH v9 5/6] reset: rzv2h-usb2phy: Convert to regmap API

[Tommaso Merciai <tommaso.merciai.xr@bp.renesas.com>]

Hi Philipp,
Thanks for your comments.

On Wed, Apr 01, 2026 at 10:23:51AM +0200, Philipp Zabel wrote:
> On Mi, 2026-04-01 at 10:04 +0200, Tommaso Merciai wrote:
> > Hi Philipp,
> > Thanks for your review.
> > 
> > On Tue, Mar 31, 2026 at 06:36:45PM +0200, Philipp Zabel wrote:
> > > On Fr, 2026-03-27 at 19:08 +0100, Tommaso Merciai wrote:
> > > > Replace raw MMIO accesses (void __iomem *, readl/writel) with
> > > > regmap_read/regmap_write via devm_regmap_init_mmio(). Regmap
> > > > provides its own internal locking, so the manual spinlock and
> > > > scoped_guard() wrappers are no longer needed.
> > > > 
> > > > Signed-off-by: Tommaso Merciai <tommaso.merciai.xr@bp.renesas.com>
> > > > ---
> > > > v8->v9:
> > > >  - New patch
> > > > 
> > > >  drivers/reset/Kconfig               |  1 +
> > > >  drivers/reset/reset-rzv2h-usb2phy.c | 42 ++++++++++++++++-------------
> > > >  2 files changed, 24 insertions(+), 19 deletions(-)
> > > > 
> > > > diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> > > > index 5165006be693..c539ca88518f 100644
> > > > --- a/drivers/reset/Kconfig
> > > > +++ b/drivers/reset/Kconfig
> > > > @@ -257,6 +257,7 @@ config RESET_RZG2L_USBPHY_CTRL
> > > >  config RESET_RZV2H_USB2PHY
> > > >  	tristate "Renesas RZ/V2H(P) (and similar SoCs) USB2PHY Reset driver"
> > > >  	depends on ARCH_RENESAS || COMPILE_TEST
> > > > +	select REGMAP_MMIO
> > > >  	help
> > > >  	  Support for USB2PHY Port reset Control found on the RZ/V2H(P) SoC
> > > >  	  (and similar SoCs).
> > > > diff --git a/drivers/reset/reset-rzv2h-usb2phy.c b/drivers/reset/reset-rzv2h-usb2phy.c
> > > > index 5bdd39274612..4014eff0f017 100644
> > > > --- a/drivers/reset/reset-rzv2h-usb2phy.c
> > > > +++ b/drivers/reset/reset-rzv2h-usb2phy.c
> > > > @@ -5,13 +5,13 @@
> > > >   * Copyright (C) 2025 Renesas Electronics Corporation
> > > >   */
> > > >  
> > > > -#include <linux/cleanup.h>
> > > >  #include <linux/delay.h>
> > > >  #include <linux/io.h>
> > > >  #include <linux/module.h>
> > > >  #include <linux/of.h>
> > > >  #include <linux/platform_device.h>
> > > >  #include <linux/pm_runtime.h>
> > > > +#include <linux/regmap.h>
> > > >  #include <linux/reset.h>
> > > >  #include <linux/reset-controller.h>
> > > >  
> > > > @@ -37,10 +37,9 @@ struct rzv2h_usb2phy_reset_of_data {
> > > >  
> > > >  struct rzv2h_usb2phy_reset_priv {
> > > >  	const struct rzv2h_usb2phy_reset_of_data *data;
> > > > -	void __iomem *base;
> > > > +	struct regmap *regmap;
> > > >  	struct device *dev;
> > > >  	struct reset_controller_dev rcdev;
> > > > -	spinlock_t lock; /* protects register accesses */
> > > >  };
> > > >  
> > > >  static inline struct rzv2h_usb2phy_reset_priv
> > > > @@ -55,10 +54,8 @@ static int rzv2h_usbphy_reset_assert(struct reset_controller_dev *rcdev,
> > > >  	struct rzv2h_usb2phy_reset_priv *priv = rzv2h_usbphy_rcdev_to_priv(rcdev);
> > > >  	const struct rzv2h_usb2phy_reset_of_data *data = priv->data;
> > > >  
> > > > -	scoped_guard(spinlock, &priv->lock) {
> > > > -		writel(data->reset2_acquire_val, priv->base + data->reset2_reg);
> > > > -		writel(data->reset_assert_val, priv->base + data->reset_reg);
> > > > -	}
> > > > +	regmap_write(priv->regmap, data->reset2_reg, data->reset2_acquire_val);
> > > > +	regmap_write(priv->regmap, data->reset_reg, data->reset_assert_val);
> > > 
> > > What is the spinlock protecting? acquire/assert registers being set
> > > together, without another acquire/assert or deassert/release register
> > > access pair interleaving?
> > > In that case you still need the lock. Or use regmap_multi_reg_write().
> > > You could even directly store the sequences as struct reg_sequence in
> > > rzv2h_usb2phy_reset_of_data.
> > 
> > You are correct. Thank you.
> > As per your suggestion I'm planning to use regmap_multi_reg_write().
> > 
> > Plan is to have the:
> > 
> > static const struct reg_sequence rzv2h_init_seqs[] = {
> 
> Even though the struct is called req_sequence, the whole array is the
> sequence. Let's call these _seq, singular.

Ack.

> 
> > 	{ .reg = 0xc10, .def = 0x67c },
> > 	{ .reg = 0xc14, .def = 0x1f },
> 
> 0x01f for consistency?

Ouch, thank you!

> 
> > 	{ .reg = 0x600, .def = 0x909 },
> > };
> > 
> > static const struct reg_sequence rzv2h_assert_seqs[] = {
> > 	{ .reg = 0xb04, .def = 0x303 },
> > 	{ .reg = 0x000, .def = 0x206 },
> 
> Consider setting .delay_us = 11, see below.

Thanks for the hint.

> 
> > };
> > 
> > static const struct reg_sequence rzv2h_deassert_seqs[] = {
> > 	{ .reg = 0x000, .def = 0x200 },
> > 	{ .reg = 0xb04, .def = 0x003 },
> > 	{ .reg = 0x000, .def = 0x000 },
> > };
> > 
> > static const struct rzv2h_usb2phy_reset_of_data rzv2h_reset_of_data = {
> > 	.init_seqs = rzv2h_init_seqs,
> > 	.init_nseqs = ARRAY_SIZE(rzv2h_init_seqs),
> > 	.assert_seqs = rzv2h_assert_seqs,
> > 	.assert_nseqs = ARRAY_SIZE(rzv2h_assert_seqs),
> > 	.deassert_seqs = rzv2h_deassert_seqs,
> > 	.deassert_nseqs = ARRAY_SIZE(rzv2h_deassert_seqs),
> > 	.reset_reg = 0,
> > 	.reset_status_bits = BIT(2),
> > };
> > 
> > With that I can use:
> > 
> > static int rzv2h_usbphy_reset_assert(struct reset_controller_dev *rcdev,
> > 				     unsigned long id)
> > {
> > 	struct rzv2h_usb2phy_reset_priv *priv = rzv2h_usbphy_rcdev_to_priv(rcdev);
> > 	const struct rzv2h_usb2phy_reset_of_data *data = priv->data;
> > 	int ret;
> > 
> > 	ret = regmap_multi_reg_write(priv->regmap, data->assert_seqs,
> > 				     data->assert_nseqs);
> > 	if (ret)
> > 		return ret;
> > 
> > 	usleep_range(11, 20);
> 
> Specifying a delay in rzv2h_assert_seqs[] and setting
> rzv2h_usb2phy_reset_regconf.can_sleep = true would have the same
> effect.

Then we can have:

static const struct reg_sequence rzv2h_init_seq[] = {
	{ .reg = 0xc10, .def = 0x67c },
	{ .reg = 0xc14, .def = 0x01f },
	{ .reg = 0x600, .def = 0x909 },
};

static const struct reg_sequence rzv2h_assert_seq[] = {
	{ .reg = 0xb04, .def = 0x303 },
	{ .reg = 0x000, .def = 0x206, .delay_us = 20 },
};

static const struct reg_sequence rzv2h_deassert_seq[] = {
	{ .reg = 0x000, .def = 0x200 },
	{ .reg = 0xb04, .def = 0x003 },
	{ .reg = 0x000, .def = 0x000 },
};

static const struct rzv2h_usb2phy_reset_of_data rzv2h_reset_of_data = {
	.init_seq = rzv2h_init_seq,
	.init_nseq = ARRAY_SIZE(rzv2h_init_seq),
	.assert_seq = rzv2h_assert_seq,
	.assert_nseq = ARRAY_SIZE(rzv2h_assert_seq),
	.deassert_seq = rzv2h_deassert_seq,
	.deassert_nseq = ARRAY_SIZE(rzv2h_deassert_seq),
	.reset_reg = 0,
	.reset_status_bits = BIT(2),
};

And I'll add:

static const struct regmap_config rzv2h_usb2phy_reset_regconf = {
	.reg_bits = 32,
	.val_bits = 32,
	.reg_stride = 4,
	.can_sleep = true,
};

In this way as you suggested we can drop usleep_range() into 
rzv2h_usbphy_reset_assert() in thi way I think we can have
simply:

static int rzv2h_usbphy_reset_assert(struct reset_controller_dev *rcdev,
				     unsigned long id)
{
	struct rzv2h_usb2phy_reset_priv *priv = rzv2h_usbphy_rcdev_to_priv(rcdev);

	return regmap_multi_reg_write(priv->regmap, priv->data->assert_seq,
				      priv->data->assert_nseq);
}

What do you think? Thanks.

Kind Regards,
Tommaso



> 
> regards
> Philipp