Re: [PATCH v7 4/7] reset: rzg2l-usbphy-ctrl: Add support for USB PWRRDY

[Philipp Zabel <p.zabel@pengutronix.de>]

Hi Claudiu,

On Mi, 2025-10-08 at 12:29 +0300, Claudiu Beznea wrote:
> Hi, Philipp,
> 
> On 10/8/25 11:34, Philipp Zabel wrote:
> > Hi Claudiu,
> > 
> > On Do, 2025-09-25 at 13:02 +0300, Claudiu wrote:
> > > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> > > 
> > > On the Renesas RZ/G3S SoC, the USB PHY block has an input signal called
> > > PWRRDY. This signal is managed by the system controller and must be
> > > de-asserted after powering on the area where USB PHY resides and asserted
> > > before powering it off.
> > > 
> > > On power-on the USB PWRRDY signal need to be de-asserted before enabling
> > > clock and switching the module to normal state (through MSTOP support). The
> > > power-on configuration sequence
> > 
> > The wording makes me wonder, have you considered implementing this as a
> > power sequencing driver?
> 
> No, haven't tried as power sequencing. At the moment this was started I
> think the power sequencing support wasn't merged.
> 
> The approaches considered were:
> a/ power domain

Letting a power domain control a corresponding power ready signal would
have been my first instinct as well.

> b/ regulator
> c/ as a reference counted bit done through regmap read/writes APIs
> 
> a and b failed as a result of discussions in the previous posted versions.

Could you point me to the discussion related to a?

I see v2 and v3 tried to control the bit from the PHY drivers, and in
v4 we were are already back to the reset driver.

> c was abandoned by me after long discussions with Renesas HW team which
> revealed the block schema b/w individual USB blocks (presented in the patch
> description).
> 
> The point is that this bit doesn't actually power anything, at the moment
> the bit is set, the power to USB is already applied. Software just need to
> set the bit before/after setting the clocks and the associated MSTOP and
> applying any USB specific settings. Each clock has an MSTOP associated and
> the MSTOP is set though the clock driver when the clock is set.

I understand.

Apart from having to carry non-reset-related code in reset drivers, I
worry about the implicit ordering that the PHY driver depends on with
this:

The power-up (probe) order is guaranteed by probe order via the
reset_control_get() check in the PHY driver, and power-down (remove)
order is guaranteed by the reset controller device reference that the
reset control holds. That's all very hidden and indirect when the
actual dependency is between a bit being set in SYSC and the clock
handling in (either) PHY driver.

> > 
> > > must be:
> > > 
> > > 1/ PWRRDY=0
> > > 2/ CLK_ON=1
> > > 3/ MSTOP=0
> > > 
> > > On power-off the configuration sequence should be:
> > > 
> > > 1/ MSTOP=1
> > > 2/ CLK_ON=0
> > > 3/ PWRRDY=1
> > > 
> > > The CLK_ON and MSTOP functionalities are controlled by clock drivers.
> > > 
> > > After long discussions with the internal HW team, it has been confirmed
> > > that the HW connection b/w USB PHY block, the USB channels, the system
> > > controller, clock, MSTOP, PWRRDY signal is as follows:
> > > 
> > >                                ┌──────────────────────────────┐
> > >                                │                              │◄── CPG_CLKON_USB.CLK0_ON
> > >                                │     USB CH0                  │
> > > ┌──────────────────────────┐   │┌───────────────────────────┐ │◄── CPG_CLKON_USB.CLK2_ON
> > > │                 ┌────────┐   ││host controller registers  │ │
> > > │                 │        │   ││function controller registers│
> > > │                 │ PHY0   │◄──┤└───────────────────────────┘ │
> > > │     USB PHY     │        │   └────────────▲─────────────────┘
> > > │                 └────────┘                │
> > > │                          │    CPG_BUS_PERI_COM_MSTOP.MSTOP{6, 5}_ON
> > > │┌──────────────┐ ┌────────┐
> > > ││USHPHY control│ │        │
> > > ││  registers   │ │ PHY1   │   ┌──────────────────────────────┐
> > > │└──────────────┘ │        │◄──┤     USB CH1                  │
> > > │                 └────────┘   │┌───────────────────────────┐ │◄── CPG_CLKON_USB.CLK1_ON
> > > └─▲───────▲─────────▲──────┘   ││ host controller registers │ │
> > >   │       │         │          │└───────────────────────────┘ │
> > >   │       │         │          └────────────▲─────────────────┘
> > >   │       │         │                       │
> > >   │       │         │           CPG_BUS_PERI_COM_MSTOP.MSTOP7_ON
> > >   │PWRRDY │         │
> > >   │       │   CPG_CLK_ON_USB.CLK3_ON
> > >   │       │
> > >   │  CPG_BUS_PERI_COM_MSTOP.MSTOP4_ON
> > >   │
> > > ┌────┐
> > > │SYSC│
> > > └────┘
> > > 
> > > where:
> > > - CPG_CLKON_USB.CLK.CLKX_ON is the register bit controlling the clock X
> > >   of different USB blocks, X in {0, 1, 2, 3}
> > > - CPG_BUS_PERI_COM_MSTOP.MSTOPX_ON is the register bit controlling the
> > >   MSTOP of different USB blocks, X in {4, 5, 6, 7}
> > > - USB PHY is the USB PHY block exposing 2 ports, port0 and port1, used
> > >   by the USB CH0, USB CH1
> > > - SYSC is the system controller block controlling the PWRRDY signal
> > > - USB CHx are individual USB block with host and function capabilities
> > >   (USB CH0 have both host and function capabilities, USB CH1 has only
> > >   host capabilities)
> > > 
> > > The USBPHY control registers are controlled though the
> > > reset-rzg2l-usbphy-ctrl driver. The USB PHY ports are controlled by
> > > phy_rcar_gen3_usb2 (drivers/phy/renesas/phy-rcar-gen3-usb2.c file). The
> > > USB PHY ports request resets from the reset-rzg2l-usbphy-ctrl driver.
> > > 
> > > The connection b/w the system controller and the USB PHY CTRL driver is
> > > implemented through the renesas,sysc-pwrrdy device tree property
> > > proposed in this patch. This property specifies the register offset and the
> > > bitmask required to control the PWRRDY signal.
> > > 
> > > Since the USB PHY CTRL driver needs to be probed before any other
> > > USB-specific driver on RZ/G3S, control of PWRRDY is passed exclusively
> > > to it. This guarantees the correct configuration sequence between clocks,
> > > MSTOP bits, and the PWRRDY bit. At the same time, changes are kept minimal
> > > by avoiding modifications to the USB PHY driver to also handle the PWRRDY
> > > itself.
> > > 
> > > Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> > > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> > > ---
> > > 
> > > Changes in v7:
> > > - used proper regmap update value on rzg2l_usbphy_ctrl_set_pwrrdy()
> > > 
> > > Changes in v6:
> > > - used syscon_regmap_lookup_by_phandle_args() to simplify the code
> > > - collected tags
> > > 
> > > Changes in v5:
> > > - none
> > > 
> > > Changes in v4:
> > > - updated patch description
> > > - updated rzg2l_usbphy_ctrl_pwrrdy_init() to map directly the
> > >   "renesas,sysc-pwrrdy" as the SYSC signal abstraction was dropped
> > >   in this version, along with rz_sysc_get_signal_map()
> > > - dropped priv member of rzg2l_usbphy_ctrl_pwrrdy_init() as it is
> > >   not needed in this version
> > > - shift left !power_on with pwrrdy->mask as this is how the
> > >   regmap_update_bits() needs the last member to be
> > > - selected MFD_SYSCON
> > > 
> > > Changes in v3:
> > > - none; this patch is new
> > > 
> > > 
> > >  drivers/reset/Kconfig                   |  1 +
> > >  drivers/reset/reset-rzg2l-usbphy-ctrl.c | 62 +++++++++++++++++++++++++
> > >  2 files changed, 63 insertions(+)
> > > 
> > > diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> > > index 78b7078478d4..329730cbcfb9 100644
> > > --- a/drivers/reset/Kconfig
> > > +++ b/drivers/reset/Kconfig
> > > @@ -237,6 +237,7 @@ config RESET_RASPBERRYPI
> > >  config RESET_RZG2L_USBPHY_CTRL
> > >  	tristate "Renesas RZ/G2L USBPHY control driver"
> > >  	depends on ARCH_RZG2L || COMPILE_TEST
> > > +	select MFD_SYSCON
> > >  	help
> > >  	  Support for USBPHY Control found on RZ/G2L family. It mainly
> > >  	  controls reset and power down of the USB/PHY.
> > > diff --git a/drivers/reset/reset-rzg2l-usbphy-ctrl.c b/drivers/reset/reset-rzg2l-usbphy-ctrl.c
> > > index 8a7f167e405e..be315199e2b0 100644
> > > --- a/drivers/reset/reset-rzg2l-usbphy-ctrl.c
> > > +++ b/drivers/reset/reset-rzg2l-usbphy-ctrl.c
> > > @@ -13,6 +13,7 @@
> > >  #include <linux/regmap.h>
> > >  #include <linux/reset.h>
> > >  #include <linux/reset-controller.h>
> > > +#include <linux/mfd/syscon.h>
> > >  
> > >  #define RESET			0x000
> > >  #define VBENCTL			0x03c
> > > @@ -41,6 +42,18 @@ struct rzg2l_usbphy_ctrl_priv {
> > >  
> > >  #define rcdev_to_priv(x)	container_of(x, struct rzg2l_usbphy_ctrl_priv, rcdev)
> > >  
> > > +/**
> > > + * struct rzg2l_usbphy_ctrl_pwrrdy - SYSC PWRRDY signal descriptor
> > > + * @regmap: SYSC regmap
> > > + * @offset: offset into the SYSC address space for accessing PWRRDY
> > > + * @mask: mask into the register at offset for accessing PWRRDY
> > > + */
> > > +struct rzg2l_usbphy_ctrl_pwrrdy {
> > > +	struct regmap *regmap;
> > > +	u32 offset;
> > > +	u32 mask;
> > > +};
> > > +
> > >  static int rzg2l_usbphy_ctrl_assert(struct reset_controller_dev *rcdev,
> > >  				    unsigned long id)
> > >  {
> > > @@ -91,6 +104,8 @@ static int rzg2l_usbphy_ctrl_status(struct reset_controller_dev *rcdev,
> > >  	return !!(readl(priv->base + RESET) & port_mask);
> > >  }
> > >  
> > > +#define RZG2L_USBPHY_CTRL_PWRRDY	1
> > > +
> > >  static const struct of_device_id rzg2l_usbphy_ctrl_match_table[] = {
> > >  	{ .compatible = "renesas,rzg2l-usbphy-ctrl" },
> > >  	{ /* Sentinel */ }
> > > @@ -110,6 +125,49 @@ static const struct regmap_config rzg2l_usb_regconf = {
> > >  	.max_register = 1,
> > >  };
> > >  
> > > +static void rzg2l_usbphy_ctrl_set_pwrrdy(struct rzg2l_usbphy_ctrl_pwrrdy *pwrrdy,
> > > +					 bool power_on)
> > > +{
> > > +	u32 val = (!power_on << (ffs(pwrrdy->mask) - 1)) & pwrrdy->mask;
> > 
> > 
> > Why not just:
> > 
> > 	u32 val = power_on ? 0 : pwrrdy->mask;
> 
> This would work as well. I wanted to be sure it doesn't fail in case the
> mask is more than one bit (as it comes from device tree).

I'd just check this in rzg2l_usbphy_ctrl_pwrrdy_init() and fail if more
than one bit is set.

[...]
> 
> > So we deassert PWRRDY on probe and assert on remove. What about
> > suspend/resume ordering?
> 
> As this driver has no suspend/resume support yet, my intention was to take
> care of this bit on suspend/resume after it will be established how it is
> handled on probe/remove. Adding suspend/resume support to this series will
> make it bigger.

Ok, please add that to the commit description.
I'm just asking because the dt-bindings patch mentions how PWRRDY must
be set during suspend/resume, and then this patch doesn't do it.

regards
Philipp