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

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

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?

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

You could simplify this further by using a regmap_field instead of
rzg2l_usbphy_ctrl_pwrrdy.

> +
> +	regmap_update_bits(pwrrdy->regmap, pwrrdy->offset, pwrrdy->mask, val);
> +}
> +
> +static void rzg2l_usbphy_ctrl_pwrrdy_off(void *data)
> +{
> +	rzg2l_usbphy_ctrl_set_pwrrdy(data, false);
> +}
> +
> +static int rzg2l_usbphy_ctrl_pwrrdy_init(struct device *dev)
> +{
> +	struct rzg2l_usbphy_ctrl_pwrrdy *pwrrdy;
> +	struct regmap *regmap;
> +	const int *data;
> +	u32 args[2];
> +
> +	data = device_get_match_data(dev);
> +	if (data != (int *)RZG2L_USBPHY_CTRL_PWRRDY)

Better not to compare pointers here:

	if ((uintptr_t)data != RZG2L_USBPHY_CTRL_PWRRDY)

> +		return 0;
> +
> +	regmap = syscon_regmap_lookup_by_phandle_args(dev->of_node,
> +						      "renesas,sysc-pwrrdy",
> +						      ARRAY_SIZE(args), args);
> +	if (IS_ERR(regmap))
> +		return PTR_ERR(regmap);
> +
> +	pwrrdy = devm_kzalloc(dev, sizeof(*pwrrdy), GFP_KERNEL);
> +	if (!pwrrdy)
> +		return -ENOMEM;
> +
> +	pwrrdy->regmap = regmap;
> +	pwrrdy->offset = args[0];
> +	pwrrdy->mask = args[1];
> +
> +	rzg2l_usbphy_ctrl_set_pwrrdy(pwrrdy, true);
> +
> +	return devm_add_action_or_reset(dev, rzg2l_usbphy_ctrl_pwrrdy_off, pwrrdy);
> +}

So we deassert PWRRDY on probe and assert on remove. What about
suspend/resume ordering?

regards
Philipp

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy