[PATCH v2 0/2] reset: rzg2l-usbphy-ctrl: Add suspend to RAM support

[PATCH v2 0/2] reset: rzg2l-usbphy-ctrl: Add suspend to RAM support

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Hi,

Series adds suspend to RAM support to the rzg2l-usbphy-ctrl driver.
Patch 1/2 is a preparatory change to avoid suppressing any
configuration errors.

Thank you,
Claudiu

Changes in v2:
- use pm_runtime_put_sync() in patch 2/2
- collected tags

Claudiu Beznea (2):
  reset: rzg2l-usbphy-ctrl: Propagate the return value of
    regmap_field_update_bits()
  reset: rzg2l-usbphy-ctrl: Add suspend/resume support

 drivers/reset/reset-rzg2l-usbphy-ctrl.c | 105 +++++++++++++++++++-----
 1 file changed, 86 insertions(+), 19 deletions(-)

-- 
2.43.0

[PATCH v2 1/2] reset: rzg2l-usbphy-ctrl: Propagate the return value of regmap_field_update_bits()

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Propagate the return value of regmap_field_update_bits() to avoid losing
any possible error. With this, the return type of
rzg2l_usbphy_ctrl_set_pwrrdy() was updated accordingly.

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v2:
- collected tags

 drivers/reset/reset-rzg2l-usbphy-ctrl.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/reset/reset-rzg2l-usbphy-ctrl.c b/drivers/reset/reset-rzg2l-usbphy-ctrl.c
index 4ecb9acb2641..9ce0c1f5d465 100644
--- a/drivers/reset/reset-rzg2l-usbphy-ctrl.c
+++ b/drivers/reset/reset-rzg2l-usbphy-ctrl.c
@@ -117,13 +117,13 @@ static const struct regmap_config rzg2l_usb_regconf = {
 	.max_register = 1,
 };
 
-static void rzg2l_usbphy_ctrl_set_pwrrdy(struct regmap_field *pwrrdy,
-					 bool power_on)
+static int rzg2l_usbphy_ctrl_set_pwrrdy(struct regmap_field *pwrrdy,
+					bool power_on)
 {
 	u32 val = power_on ? 0 : 1;
 
 	/* The initialization path guarantees that the mask is 1 bit long. */
-	regmap_field_update_bits(pwrrdy, 1, val);
+	return regmap_field_update_bits(pwrrdy, 1, val);
 }
 
 static void rzg2l_usbphy_ctrl_pwrrdy_off(void *data)
@@ -138,6 +138,7 @@ static int rzg2l_usbphy_ctrl_pwrrdy_init(struct device *dev)
 	struct regmap *regmap;
 	const int *data;
 	u32 args[2];
+	int ret;
 
 	data = device_get_match_data(dev);
 	if ((uintptr_t)data != RZG2L_USBPHY_CTRL_PWRRDY)
@@ -161,7 +162,9 @@ static int rzg2l_usbphy_ctrl_pwrrdy_init(struct device *dev)
 	if (IS_ERR(pwrrdy))
 		return PTR_ERR(pwrrdy);
 
-	rzg2l_usbphy_ctrl_set_pwrrdy(pwrrdy, true);
+	ret = rzg2l_usbphy_ctrl_set_pwrrdy(priv->pwrrdy, true);
+	if (ret)
+		return ret;
 
 	return devm_add_action_or_reset(dev, rzg2l_usbphy_ctrl_pwrrdy_off, pwrrdy);
 }
-- 
2.43.0

[PATCH v2 2/2] reset: rzg2l-usbphy-ctrl: Add suspend/resume support

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

The RZ/G2L USBPHY control driver is also used on the RZ/G3S SoC.
The RZ/G3S SoC supports a power-saving mode in which power to most USB
components (including the USBPHY control block) is turned off. Because of
this, the USBPHY control block needs to be reconfigured when returning
from power-saving mode.

Add suspend/resume support to handle runtime suspend/resume of the device,
assert/deassert the reset signal, and reinitialize the USBPHY control
block.

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v2:
- used pm_runtime_put_sync() in rzg2l_usbphy_ctrl_suspend()

 drivers/reset/reset-rzg2l-usbphy-ctrl.c | 94 +++++++++++++++++++++----
 1 file changed, 79 insertions(+), 15 deletions(-)

diff --git a/drivers/reset/reset-rzg2l-usbphy-ctrl.c b/drivers/reset/reset-rzg2l-usbphy-ctrl.c
index 9ce0c1f5d465..1a1581643bf3 100644
--- a/drivers/reset/reset-rzg2l-usbphy-ctrl.c
+++ b/drivers/reset/reset-rzg2l-usbphy-ctrl.c
@@ -36,6 +36,7 @@ struct rzg2l_usbphy_ctrl_priv {
 	struct reset_control *rstc;
 	void __iomem *base;
 	struct platform_device *vdev;
+	struct regmap_field *pwrrdy;
 
 	spinlock_t lock;
 };
@@ -92,6 +93,19 @@ static int rzg2l_usbphy_ctrl_status(struct reset_controller_dev *rcdev,
 	return !!(readl(priv->base + RESET) & port_mask);
 }
 
+/* put pll and phy into reset state */
+static void rzg2l_usbphy_ctrl_init(struct rzg2l_usbphy_ctrl_priv *priv)
+{
+	unsigned long flags;
+	u32 val;
+
+	spin_lock_irqsave(&priv->lock, flags);
+	val = readl(priv->base + RESET);
+	val |= RESET_SEL_PLLRESET | RESET_PLLRESET | PHY_RESET_PORT2 | PHY_RESET_PORT1;
+	writel(val, priv->base + RESET);
+	spin_unlock_irqrestore(&priv->lock, flags);
+}
+
 #define RZG2L_USBPHY_CTRL_PWRRDY	1
 
 static const struct of_device_id rzg2l_usbphy_ctrl_match_table[] = {
@@ -131,9 +145,9 @@ 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)
+static int rzg2l_usbphy_ctrl_pwrrdy_init(struct device *dev,
+					 struct rzg2l_usbphy_ctrl_priv *priv)
 {
-	struct regmap_field *pwrrdy;
 	struct reg_field field;
 	struct regmap *regmap;
 	const int *data;
@@ -158,15 +172,15 @@ static int rzg2l_usbphy_ctrl_pwrrdy_init(struct device *dev)
 	field.lsb = __ffs(args[1]);
 	field.msb = __fls(args[1]);
 
-	pwrrdy = devm_regmap_field_alloc(dev, regmap, field);
-	if (IS_ERR(pwrrdy))
-		return PTR_ERR(pwrrdy);
+	priv->pwrrdy = devm_regmap_field_alloc(dev, regmap, field);
+	if (IS_ERR(priv->pwrrdy))
+		return PTR_ERR(priv->pwrrdy);
 
 	ret = rzg2l_usbphy_ctrl_set_pwrrdy(priv->pwrrdy, true);
 	if (ret)
 		return ret;
 
-	return devm_add_action_or_reset(dev, rzg2l_usbphy_ctrl_pwrrdy_off, pwrrdy);
+	return devm_add_action_or_reset(dev, rzg2l_usbphy_ctrl_pwrrdy_off, priv->pwrrdy);
 }
 
 static int rzg2l_usbphy_ctrl_probe(struct platform_device *pdev)
@@ -175,9 +189,7 @@ static int rzg2l_usbphy_ctrl_probe(struct platform_device *pdev)
 	struct rzg2l_usbphy_ctrl_priv *priv;
 	struct platform_device *vdev;
 	struct regmap *regmap;
-	unsigned long flags;
 	int error;
-	u32 val;
 
 	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
@@ -191,7 +203,7 @@ static int rzg2l_usbphy_ctrl_probe(struct platform_device *pdev)
 	if (IS_ERR(regmap))
 		return PTR_ERR(regmap);
 
-	error = rzg2l_usbphy_ctrl_pwrrdy_init(dev);
+	error = rzg2l_usbphy_ctrl_pwrrdy_init(dev, priv);
 	if (error)
 		return error;
 
@@ -214,12 +226,7 @@ static int rzg2l_usbphy_ctrl_probe(struct platform_device *pdev)
 		goto err_pm_disable_reset_deassert;
 	}
 
-	/* put pll and phy into reset state */
-	spin_lock_irqsave(&priv->lock, flags);
-	val = readl(priv->base + RESET);
-	val |= RESET_SEL_PLLRESET | RESET_PLLRESET | PHY_RESET_PORT2 | PHY_RESET_PORT1;
-	writel(val, priv->base + RESET);
-	spin_unlock_irqrestore(&priv->lock, flags);
+	rzg2l_usbphy_ctrl_init(priv);
 
 	priv->rcdev.ops = &rzg2l_usbphy_ctrl_reset_ops;
 	priv->rcdev.of_reset_n_cells = 1;
@@ -266,10 +273,67 @@ static void rzg2l_usbphy_ctrl_remove(struct platform_device *pdev)
 	reset_control_assert(priv->rstc);
 }
 
+static int rzg2l_usbphy_ctrl_suspend(struct device *dev)
+{
+	struct rzg2l_usbphy_ctrl_priv *priv = dev_get_drvdata(dev);
+	int ret;
+
+	pm_runtime_put_sync(dev);
+
+	ret = reset_control_assert(priv->rstc);
+	if (ret)
+		goto rpm_resume;
+
+	ret = rzg2l_usbphy_ctrl_set_pwrrdy(priv->pwrrdy, false);
+	if (ret)
+		goto reset_deassert;
+
+	return 0;
+
+reset_deassert:
+	reset_control_deassert(priv->rstc);
+rpm_resume:
+	pm_runtime_resume_and_get(dev);
+	return ret;
+}
+
+static int rzg2l_usbphy_ctrl_resume(struct device *dev)
+{
+	struct rzg2l_usbphy_ctrl_priv *priv = dev_get_drvdata(dev);
+	int ret;
+
+	ret = rzg2l_usbphy_ctrl_set_pwrrdy(priv->pwrrdy, true);
+	if (ret)
+		return ret;
+
+	ret = reset_control_deassert(priv->rstc);
+	if (ret)
+		goto pwrrdy_off;
+
+	ret = pm_runtime_resume_and_get(dev);
+	if (ret)
+		goto reset_assert;
+
+	rzg2l_usbphy_ctrl_init(priv);
+
+	return 0;
+
+reset_assert:
+	reset_control_assert(priv->rstc);
+pwrrdy_off:
+	rzg2l_usbphy_ctrl_set_pwrrdy(priv->pwrrdy, false);
+	return ret;
+}
+
+static DEFINE_SIMPLE_DEV_PM_OPS(rzg2l_usbphy_ctrl_pm_ops,
+				rzg2l_usbphy_ctrl_suspend,
+				rzg2l_usbphy_ctrl_resume);
+
 static struct platform_driver rzg2l_usbphy_ctrl_driver = {
 	.driver = {
 		.name		= "rzg2l_usbphy_ctrl",
 		.of_match_table	= rzg2l_usbphy_ctrl_match_table,
+		.pm		= pm_ptr(&rzg2l_usbphy_ctrl_pm_ops),
 	},
 	.probe	= rzg2l_usbphy_ctrl_probe,
 	.remove = rzg2l_usbphy_ctrl_remove,
-- 
2.43.0

Re: [PATCH v2 0/2] reset: rzg2l-usbphy-ctrl: Add suspend to RAM support

[Claudiu Beznea]

Hi, Philipp,

Could you please let me know if there's anything I should do for this series?

Thank you,
Claudiu

On 11/10/25 15:27, Claudiu wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> Hi,
> 
> Series adds suspend to RAM support to the rzg2l-usbphy-ctrl driver.
> Patch 1/2 is a preparatory change to avoid suppressing any
> configuration errors.
> 
> Thank you,
> Claudiu
> 
> Changes in v2:
> - use pm_runtime_put_sync() in patch 2/2
> - collected tags
> 
> Claudiu Beznea (2):
>   reset: rzg2l-usbphy-ctrl: Propagate the return value of
>     regmap_field_update_bits()
>   reset: rzg2l-usbphy-ctrl: Add suspend/resume support
> 
>  drivers/reset/reset-rzg2l-usbphy-ctrl.c | 105 +++++++++++++++++++-----
>  1 file changed, 86 insertions(+), 19 deletions(-)
> 

RE: [PATCH v2 0/2] reset: rzg2l-usbphy-ctrl: Add suspend to RAM support

[Biju Das]

Hi Claudiu,

> -----Original Message-----
> From: Claudiu Beznea <claudiu.beznea@tuxon.dev>
> Sent: 04 December 2025 18:26
> Subject: Re: [PATCH v2 0/2] reset: rzg2l-usbphy-ctrl: Add suspend to RAM support
> 
> Hi, Philipp,
> 
> Could you please let me know if there's anything I should do for this series?

If rzg2l_usbphy_ctrl_suspend() fails, What is the probability that it will
suspend again without any issue as currently we abort system suspend
and enable clocks/deassert reset which keep draining the power.

Cheers,
Biju 

> 
> Thank you,
> Claudiu
> 
> On 11/10/25 15:27, Claudiu wrote:
> > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >
> > Hi,
> >
> > Series adds suspend to RAM support to the rzg2l-usbphy-ctrl driver.
> > Patch 1/2 is a preparatory change to avoid suppressing any
> > configuration errors.
> >
> > Thank you,
> > Claudiu
> >
> > Changes in v2:
> > - use pm_runtime_put_sync() in patch 2/2
> > - collected tags
> >
> > Claudiu Beznea (2):
> >   reset: rzg2l-usbphy-ctrl: Propagate the return value of
> >     regmap_field_update_bits()
> >   reset: rzg2l-usbphy-ctrl: Add suspend/resume support
> >
> >  drivers/reset/reset-rzg2l-usbphy-ctrl.c | 105
> > +++++++++++++++++++-----
> >  1 file changed, 86 insertions(+), 19 deletions(-)
> >
> 

Re: [PATCH v2 0/2] reset: rzg2l-usbphy-ctrl: Add suspend to RAM support

[Claudiu Beznea]

Hi, Biju,

On 12/5/25 10:53, Biju Das wrote:
> 
> 
> Hi Claudiu,
> 
>> -----Original Message-----
>> From: Claudiu Beznea <claudiu.beznea@tuxon.dev>
>> Sent: 04 December 2025 18:26
>> Subject: Re: [PATCH v2 0/2] reset: rzg2l-usbphy-ctrl: Add suspend to RAM support
>>
>> Hi, Philipp,
>>
>> Could you please let me know if there's anything I should do for this series?
> 
> If rzg2l_usbphy_ctrl_suspend() fails, What is the probability that it will
> suspend again without any issue

How can I measure this?

The idea with this code was the following: if any instruction of suspend
fails, the suspend is aborted, thus code in rzg2l_usbphy_ctrl_suspend() is
trying to restore the runtime state of the HW so that no runtime users of
it to be affected. This is also how core suspend code is doing, e.g.
suspend_devices_and_enter().

> as currently we abort system suspend
> and enable clocks/deassert reset which keep draining the power.
The code is restoring the clocks and resets to their previous runtime state
so that any users of it to not be affected. Later, at runtime, if any power
needs to be saved the runtime PM framework will do its job.

Thank you,
Claudiu

RE: [PATCH v2 0/2] reset: rzg2l-usbphy-ctrl: Add suspend to RAM support

[Biju Das]

> -----Original Message-----
> From: Claudiu Beznea <claudiu.beznea@tuxon.dev>
> Sent: 05 December 2025 10:00
> To: Biju Das <biju.das.jz@bp.renesas.com>; p.zabel@pengutronix.de
> Cc: linux-kernel@vger.kernel.org; linux-renesas-soc@vger.kernel.org; Claudiu Beznea
> <claudiu.beznea.uj@bp.renesas.com>
> Subject: Re: [PATCH v2 0/2] reset: rzg2l-usbphy-ctrl: Add suspend to RAM support
> 
> Hi, Biju,
> 
> On 12/5/25 10:53, Biju Das wrote:
> >
> >
> > Hi Claudiu,
> >
> >> -----Original Message-----
> >> From: Claudiu Beznea <claudiu.beznea@tuxon.dev>
> >> Sent: 04 December 2025 18:26
> >> Subject: Re: [PATCH v2 0/2] reset: rzg2l-usbphy-ctrl: Add suspend to
> >> RAM support
> >>
> >> Hi, Philipp,
> >>
> >> Could you please let me know if there's anything I should do for this series?
> >
> > If rzg2l_usbphy_ctrl_suspend() fails, What is the probability that it
> > will suspend again without any issue
> 
> How can I measure this?
> 
> The idea with this code was the following: if any instruction of suspend fails, the suspend is
> aborted, thus code in rzg2l_usbphy_ctrl_suspend() is trying to restore the runtime state of the HW so
> that no runtime users of it to be affected. This is also how core suspend code is doing, e.g.
> suspend_devices_and_enter().

The entire system suspend is aborted. See the case 2 in logs[1] and system keeps draining the power.

Again, if system tries to do shut down the same device will fail again in similar way and 
The system will never enter into suspend state.

[1] https://lore.kernel.org/all/TY3PR01MB11346A7B16CB3267F1A57302B86DBA@TY3PR01MB11346.jpnprd01.prod.outlook.com/



> 
> > as currently we abort system suspend
> > and enable clocks/deassert reset which keep draining the power.
> The code is restoring the clocks and resets to their previous runtime state so that any users of it to
> not be affected. Later, at runtime, if any power needs to be saved the runtime PM framework will do
> its job.

The system is enters into suspend state for saving power. Not to consume because of failure.
Our SoCs will power down during suspend and device reset will happen during wakeup
So everything will work as usual if there is device specific failures during suspend.
See case 3 in logs[1]

Cheers,
Biju

Re: [PATCH v2 0/2] reset: rzg2l-usbphy-ctrl: Add suspend to RAM support

[Claudiu Beznea]

On 12/5/25 12:17, Biju Das wrote:
> 
> 
>> -----Original Message-----
>> From: Claudiu Beznea <claudiu.beznea@tuxon.dev>
>> Sent: 05 December 2025 10:00
>> To: Biju Das <biju.das.jz@bp.renesas.com>; p.zabel@pengutronix.de
>> Cc: linux-kernel@vger.kernel.org; linux-renesas-soc@vger.kernel.org; Claudiu Beznea
>> <claudiu.beznea.uj@bp.renesas.com>
>> Subject: Re: [PATCH v2 0/2] reset: rzg2l-usbphy-ctrl: Add suspend to RAM support
>>
>> Hi, Biju,
>>
>> On 12/5/25 10:53, Biju Das wrote:
>>>
>>>
>>> Hi Claudiu,
>>>
>>>> -----Original Message-----
>>>> From: Claudiu Beznea <claudiu.beznea@tuxon.dev>
>>>> Sent: 04 December 2025 18:26
>>>> Subject: Re: [PATCH v2 0/2] reset: rzg2l-usbphy-ctrl: Add suspend to
>>>> RAM support
>>>>
>>>> Hi, Philipp,
>>>>
>>>> Could you please let me know if there's anything I should do for this series?
>>>
>>> If rzg2l_usbphy_ctrl_suspend() fails, What is the probability that it
>>> will suspend again without any issue
>>
>> How can I measure this?
>>
>> The idea with this code was the following: if any instruction of suspend fails, the suspend is
>> aborted, thus code in rzg2l_usbphy_ctrl_suspend() is trying to restore the runtime state of the HW so
>> that no runtime users of it to be affected. This is also how core suspend code is doing, e.g.
>> suspend_devices_and_enter().
> 
> The entire system suspend is aborted.

Isn't this the current general approach across different drivers?

Also, take into account that this code will still be executed for suspend
to idle, where power is not lost.

Also, for general case: if we ignore any failure, just because we may
resume from a power down state (where Linux state is preserved in RAM and
most of the SoC parts are powered off), there are resources that are
reference counted (e.g., clocks, some resets). Ignoring failures in those
cases wouldn't necessary make them work after resume just because the
system resumes from a power down state. The reference counters may not have
the right values for the proper registers to be updated.

> See the case 2 in logs[1] and system keeps draining the power.

Case 2 in the pointed logs seems related to resume, are we talk about
suspend, resume or both?

Also, case 2 points to a resume function that returns error w/o taking it
into account. The resume code proposed here takes into account any errors
on the resume path and put the HW in a power saving state as otherwise it
can't be runtime recovered.

> 
> Again, if system tries to do shut down

I guess, here you are talking about suspend with power cut.

> the same device will fail again in similar way and 
> The system will never enter into suspend state.

From my previous experience with suspend/resume implementations, I can say
restoring the system in failure cases in suspend/resume or not, is up to
the subsystem maintainer. So, I'll let Philipp to decide how he wants to go
with it in this driver.

> 
> [1] https://lore.kernel.org/all/TY3PR01MB11346A7B16CB3267F1A57302B86DBA@TY3PR01MB11346.jpnprd01.prod.outlook.com/
> 
> 
> 
>>
>>> as currently we abort system suspend
>>> and enable clocks/deassert reset which keep draining the power.
>> The code is restoring the clocks and resets to their previous runtime state so that any users of it to
>> not be affected. Later, at runtime, if any power needs to be saved the runtime PM framework will do
>> its job.
> 
> The system is enters into suspend state for saving power. Not to consume because of failure.
> Our SoCs will power down during suspend and device reset will happen during wakeup
> So everything will work as usual if there is device specific failures during suspend.
> See case 3 in logs[1]

They are still supporting suspend to idle, where power is maintained,
right? Shouldn't we cover this case?

Thank you,
Claudiu

RE: [PATCH v2 0/2] reset: rzg2l-usbphy-ctrl: Add suspend to RAM support

[Biju Das]

Hi Claudiu,

> -----Original Message-----
> From: Claudiu Beznea <claudiu.beznea@tuxon.dev>
> Sent: 05 December 2025 10:47
> Subject: Re: [PATCH v2 0/2] reset: rzg2l-usbphy-ctrl: Add suspend to RAM support
> 
> 
> 
> On 12/5/25 12:17, Biju Das wrote:
> >
> >
> >> -----Original Message-----
> >> From: Claudiu Beznea <claudiu.beznea@tuxon.dev>
> >> Sent: 05 December 2025 10:00
> >> To: Biju Das <biju.das.jz@bp.renesas.com>; p.zabel@pengutronix.de
> >> Cc: linux-kernel@vger.kernel.org; linux-renesas-soc@vger.kernel.org;
> >> Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >> Subject: Re: [PATCH v2 0/2] reset: rzg2l-usbphy-ctrl: Add suspend to
> >> RAM support
> >>
> >> Hi, Biju,
> >>
> >> On 12/5/25 10:53, Biju Das wrote:
> >>>
> >>>
> >>> Hi Claudiu,
> >>>
> >>>> -----Original Message-----
> >>>> From: Claudiu Beznea <claudiu.beznea@tuxon.dev>
> >>>> Sent: 04 December 2025 18:26
> >>>> Subject: Re: [PATCH v2 0/2] reset: rzg2l-usbphy-ctrl: Add suspend
> >>>> to RAM support
> >>>>
> >>>> Hi, Philipp,
> >>>>
> >>>> Could you please let me know if there's anything I should do for this series?
> >>>
> >>> If rzg2l_usbphy_ctrl_suspend() fails, What is the probability that
> >>> it will suspend again without any issue
> >>
> >> How can I measure this?
> >>
> >> The idea with this code was the following: if any instruction of
> >> suspend fails, the suspend is aborted, thus code in
> >> rzg2l_usbphy_ctrl_suspend() is trying to restore the runtime state of the HW so that no runtime
> users of it to be affected. This is also how core suspend code is doing, e.g.
> >> suspend_devices_and_enter().
> >
> > The entire system suspend is aborted.
> 
> Isn't this the current general approach across different drivers?

OK.

> 
> Also, take into account that this code will still be executed for suspend to idle, where power is not
> lost.

OK.

> 
> Also, for general case: if we ignore any failure, just because we may resume from a power down state
> (where Linux state is preserved in RAM and most of the SoC parts are powered off), there are resources
> that are reference counted (e.g., clocks, some resets). Ignoring failures in those cases wouldn't
> necessary make them work after resume just because the system resumes from a power down state. The
> reference counters may not have the right values for the proper registers to be updated.

OK.

> 
> > See the case 2 in logs[1] and system keeps draining the power.
> 
> Case 2 in the pointed logs seems related to resume, are we talk about suspend, resume or both?
> 
> Also, case 2 points to a resume function that returns error w/o taking it into account. The resume
> code proposed here takes into account any errors on the resume path and put the HW in a power saving
> state as otherwise it can't be runtime recovered.
> 
> >
> > Again, if system tries to do shut down
> 
> I guess, here you are talking about suspend with power cut.

It was a typo. System tries to suspend again.

> 
> > the same device will fail again in similar way and The system will
> > never enter into suspend state.
> 
> From my previous experience with suspend/resume implementations, I can say restoring the system in
> failure cases in suspend/resume or not, is up to the subsystem maintainer. So, I'll let Philipp to
> decide how he wants to go with it in this driver.
> 

Agreed.

> They are still supporting suspend to idle, where power is maintained, right? Shouldn't we cover this
> case?

Yes, I agree. Probably best thing is zero failures, if there is a failure in suspend
path, the same device will fail in similar fashion, and the system never enters suspend state.

So, report the failure and debug and fix the issue.


Cheers,
Biju

RE: [PATCH v2 0/2] reset: rzg2l-usbphy-ctrl: Add suspend to RAM support

[Biju Das]

Hi Claudiu,

> -----Original Message-----
> From: Biju Das
> Sent: 05 December 2025 10:57
> Subject: RE: [PATCH v2 0/2] reset: rzg2l-usbphy-ctrl: Add suspend to RAM support
> 
> 
> Hi Claudiu,
> 
> > -----Original Message-----
> > From: Claudiu Beznea <claudiu.beznea@tuxon.dev>
> > Sent: 05 December 2025 10:47
> > Subject: Re: [PATCH v2 0/2] reset: rzg2l-usbphy-ctrl: Add suspend to
> > RAM support
> >
> >
> >
> > On 12/5/25 12:17, Biju Das wrote:
> > >
> > >
> > >> -----Original Message-----
> > >> From: Claudiu Beznea <claudiu.beznea@tuxon.dev>
> > >> Sent: 05 December 2025 10:00
> > >> To: Biju Das <biju.das.jz@bp.renesas.com>; p.zabel@pengutronix.de
> > >> Cc: linux-kernel@vger.kernel.org;
> > >> linux-renesas-soc@vger.kernel.org;
> > >> Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> > >> Subject: Re: [PATCH v2 0/2] reset: rzg2l-usbphy-ctrl: Add suspend
> > >> to RAM support
> > >>
> > >> Hi, Biju,
> > >>
> > >> On 12/5/25 10:53, Biju Das wrote:
> > >>>
> > >>>
> > >>> Hi Claudiu,
> > >>>
> > >>>> -----Original Message-----
> > >>>> From: Claudiu Beznea <claudiu.beznea@tuxon.dev>
> > >>>> Sent: 04 December 2025 18:26
> > >>>> Subject: Re: [PATCH v2 0/2] reset: rzg2l-usbphy-ctrl: Add suspend
> > >>>> to RAM support
> > >>>>
> >
> > From my previous experience with suspend/resume implementations, I can
> > say restoring the system in failure cases in suspend/resume or not, is
> > up to the subsystem maintainer. So, I'll let Philipp to decide how he wants to go with it in this
> driver.
> >
> 
> Agreed.
> 
> > They are still supporting suspend to idle, where power is maintained,
> > right? Shouldn't we cover this case?
> 
> Yes, I agree. Probably best thing is zero failures, if there is a failure in suspend path, the same
> device will fail in similar fashion, and the system never enters suspend state.
> 
> So, report the failure and debug and fix the issue.

FYI, On your resume path, if the below call fails, then there is a pm imbalance for next suspend().

ret = pm_runtime_resume_and_get(dev);

Similarly, if reset_assert() fails for a shared reset.

Cheers,
Biju

Re: [PATCH v2 0/2] reset: rzg2l-usbphy-ctrl: Add suspend to RAM support

[Claudiu Beznea]

On 12/5/25 13:55, Biju Das wrote:
> Hi Claudiu,
> 
>> -----Original Message-----
>> From: Biju Das
>> Sent: 05 December 2025 10:57
>> Subject: RE: [PATCH v2 0/2] reset: rzg2l-usbphy-ctrl: Add suspend to RAM support
>>
>>
>> Hi Claudiu,
>>
>>> -----Original Message-----
>>> From: Claudiu Beznea <claudiu.beznea@tuxon.dev>
>>> Sent: 05 December 2025 10:47
>>> Subject: Re: [PATCH v2 0/2] reset: rzg2l-usbphy-ctrl: Add suspend to
>>> RAM support
>>>
>>>
>>>
>>> On 12/5/25 12:17, Biju Das wrote:
>>>>
>>>>
>>>>> -----Original Message-----
>>>>> From: Claudiu Beznea <claudiu.beznea@tuxon.dev>
>>>>> Sent: 05 December 2025 10:00
>>>>> To: Biju Das <biju.das.jz@bp.renesas.com>; p.zabel@pengutronix.de
>>>>> Cc: linux-kernel@vger.kernel.org;
>>>>> linux-renesas-soc@vger.kernel.org;
>>>>> Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>>> Subject: Re: [PATCH v2 0/2] reset: rzg2l-usbphy-ctrl: Add suspend
>>>>> to RAM support
>>>>>
>>>>> Hi, Biju,
>>>>>
>>>>> On 12/5/25 10:53, Biju Das wrote:
>>>>>>
>>>>>>
>>>>>> Hi Claudiu,
>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: Claudiu Beznea <claudiu.beznea@tuxon.dev>
>>>>>>> Sent: 04 December 2025 18:26
>>>>>>> Subject: Re: [PATCH v2 0/2] reset: rzg2l-usbphy-ctrl: Add suspend
>>>>>>> to RAM support
>>>>>>>
>>>
>>> From my previous experience with suspend/resume implementations, I can
>>> say restoring the system in failure cases in suspend/resume or not, is
>>> up to the subsystem maintainer. So, I'll let Philipp to decide how he wants to go with it in this
>> driver.
>>>
>>
>> Agreed.
>>
>>> They are still supporting suspend to idle, where power is maintained,
>>> right? Shouldn't we cover this case?
>>
>> Yes, I agree. Probably best thing is zero failures, if there is a failure in suspend path, the same
>> device will fail in similar fashion, and the system never enters suspend state.
>>
>> So, report the failure and debug and fix the issue.
> 
> FYI, On your resume path, if the below call fails, then there is a pm imbalance for next suspend().
> 
> ret = pm_runtime_resume_and_get(dev);
> 
> Similarly, if reset_assert() fails for a shared reset.

Wouldn't be the same if there will be no failure path code?

Thank you,
Claudiu

RE: [PATCH v2 0/2] reset: rzg2l-usbphy-ctrl: Add suspend to RAM support

[Biju Das]

> -----Original Message-----
> From: Claudiu Beznea <claudiu.beznea@tuxon.dev>
> Sent: 05 December 2025 13:30
> To: Biju Das <biju.das.jz@bp.renesas.com>; p.zabel@pengutronix.de
> Cc: linux-kernel@vger.kernel.org; linux-renesas-soc@vger.kernel.org; Claudiu Beznea
> <claudiu.beznea.uj@bp.renesas.com>
> Subject: Re: [PATCH v2 0/2] reset: rzg2l-usbphy-ctrl: Add suspend to RAM support
> 
> 
> 
> On 12/5/25 13:55, Biju Das wrote:
> > Hi Claudiu,
> >
> >> -----Original Message-----
> >> From: Biju Das
> >> Sent: 05 December 2025 10:57
> >> Subject: RE: [PATCH v2 0/2] reset: rzg2l-usbphy-ctrl: Add suspend to
> >> RAM support
> >>
> >>
> >> Hi Claudiu,
> >>
> >>> -----Original Message-----
> >>> From: Claudiu Beznea <claudiu.beznea@tuxon.dev>
> >>> Sent: 05 December 2025 10:47
> >>> Subject: Re: [PATCH v2 0/2] reset: rzg2l-usbphy-ctrl: Add suspend to
> >>> RAM support
> >>>
> >>>
> >>>
> >>> On 12/5/25 12:17, Biju Das wrote:
> >>>>
> >>>>
> >>>>> -----Original Message-----
> >>>>> From: Claudiu Beznea <claudiu.beznea@tuxon.dev>
> >>>>> Sent: 05 December 2025 10:00
> >>>>> To: Biju Das <biju.das.jz@bp.renesas.com>; p.zabel@pengutronix.de
> >>>>> Cc: linux-kernel@vger.kernel.org;
> >>>>> linux-renesas-soc@vger.kernel.org;
> >>>>> Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >>>>> Subject: Re: [PATCH v2 0/2] reset: rzg2l-usbphy-ctrl: Add suspend
> >>>>> to RAM support
> >>>>>
> >>>>> Hi, Biju,
> >>>>>
> >>>>> On 12/5/25 10:53, Biju Das wrote:
> >>>>>>
> >>>>>>
> >>>>>> Hi Claudiu,
> >>>>>>
> >>>>>>> -----Original Message-----
> >>>>>>> From: Claudiu Beznea <claudiu.beznea@tuxon.dev>
> >>>>>>> Sent: 04 December 2025 18:26
> >>>>>>> Subject: Re: [PATCH v2 0/2] reset: rzg2l-usbphy-ctrl: Add
> >>>>>>> suspend to RAM support
> >>>>>>>
> >>>
> >>> From my previous experience with suspend/resume implementations, I
> >>> can say restoring the system in failure cases in suspend/resume or
> >>> not, is up to the subsystem maintainer. So, I'll let Philipp to
> >>> decide how he wants to go with it in this
> >> driver.
> >>>
> >>
> >> Agreed.
> >>
> >>> They are still supporting suspend to idle, where power is
> >>> maintained, right? Shouldn't we cover this case?
> >>
> >> Yes, I agree. Probably best thing is zero failures, if there is a
> >> failure in suspend path, the same device will fail in similar fashion, and the system never enters
> suspend state.
> >>
> >> So, report the failure and debug and fix the issue.
> >
> > FYI, On your resume path, if the below call fails, then there is a pm imbalance for next suspend().
> >
> > ret = pm_runtime_resume_and_get(dev);
> >
> > Similarly, if reset_assert() fails for a shared reset.
> 
> Wouldn't be the same if there will be no failure path code?


Eg:
ret = reset_control_deassert(priv->rstc);
+	if (ret)
+		goto pwrrdy_off;

Here you are skipping pm_runtime_resume_and_get(), The subsequent suspend()
Will lead to pm underflow error.

Similarly, on suspend() you are checking the error code of reset_assert(),
If it fails, you deassert it. Surprisingly, there is no deassert operation
On resume().


Cheers,
Biju

Re: [PATCH v2 0/2] reset: rzg2l-usbphy-ctrl: Add suspend to RAM support

[Claudiu Beznea]

On 12/5/25 15:45, Biju Das wrote:
> 
> 
>> -----Original Message-----
>> From: Claudiu Beznea <claudiu.beznea@tuxon.dev>
>> Sent: 05 December 2025 13:30
>> To: Biju Das <biju.das.jz@bp.renesas.com>; p.zabel@pengutronix.de
>> Cc: linux-kernel@vger.kernel.org; linux-renesas-soc@vger.kernel.org; Claudiu Beznea
>> <claudiu.beznea.uj@bp.renesas.com>
>> Subject: Re: [PATCH v2 0/2] reset: rzg2l-usbphy-ctrl: Add suspend to RAM support
>>
>>
>>
>> On 12/5/25 13:55, Biju Das wrote:
>>> Hi Claudiu,
>>>
>>>> -----Original Message-----
>>>> From: Biju Das
>>>> Sent: 05 December 2025 10:57
>>>> Subject: RE: [PATCH v2 0/2] reset: rzg2l-usbphy-ctrl: Add suspend to
>>>> RAM support
>>>>
>>>>
>>>> Hi Claudiu,
>>>>
>>>>> -----Original Message-----
>>>>> From: Claudiu Beznea <claudiu.beznea@tuxon.dev>
>>>>> Sent: 05 December 2025 10:47
>>>>> Subject: Re: [PATCH v2 0/2] reset: rzg2l-usbphy-ctrl: Add suspend to
>>>>> RAM support
>>>>>
>>>>>
>>>>>
>>>>> On 12/5/25 12:17, Biju Das wrote:
>>>>>>
>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: Claudiu Beznea <claudiu.beznea@tuxon.dev>
>>>>>>> Sent: 05 December 2025 10:00
>>>>>>> To: Biju Das <biju.das.jz@bp.renesas.com>; p.zabel@pengutronix.de
>>>>>>> Cc: linux-kernel@vger.kernel.org;
>>>>>>> linux-renesas-soc@vger.kernel.org;
>>>>>>> Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>>>>> Subject: Re: [PATCH v2 0/2] reset: rzg2l-usbphy-ctrl: Add suspend
>>>>>>> to RAM support
>>>>>>>
>>>>>>> Hi, Biju,
>>>>>>>
>>>>>>> On 12/5/25 10:53, Biju Das wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> Hi Claudiu,
>>>>>>>>
>>>>>>>>> -----Original Message-----
>>>>>>>>> From: Claudiu Beznea <claudiu.beznea@tuxon.dev>
>>>>>>>>> Sent: 04 December 2025 18:26
>>>>>>>>> Subject: Re: [PATCH v2 0/2] reset: rzg2l-usbphy-ctrl: Add
>>>>>>>>> suspend to RAM support
>>>>>>>>>
>>>>>
>>>>> From my previous experience with suspend/resume implementations, I
>>>>> can say restoring the system in failure cases in suspend/resume or
>>>>> not, is up to the subsystem maintainer. So, I'll let Philipp to
>>>>> decide how he wants to go with it in this
>>>> driver.
>>>>>
>>>>
>>>> Agreed.
>>>>
>>>>> They are still supporting suspend to idle, where power is
>>>>> maintained, right? Shouldn't we cover this case?
>>>>
>>>> Yes, I agree. Probably best thing is zero failures, if there is a
>>>> failure in suspend path, the same device will fail in similar fashion, and the system never enters
>> suspend state.
>>>>
>>>> So, report the failure and debug and fix the issue.
>>>
>>> FYI, On your resume path, if the below call fails, then there is a pm imbalance for next suspend().
>>>
>>> ret = pm_runtime_resume_and_get(dev);
>>>
>>> Similarly, if reset_assert() fails for a shared reset.
>>
>> Wouldn't be the same if there will be no failure path code?

Could you please reply to this question as I may be wrong?

> 
> 
> Eg:
> ret = reset_control_deassert(priv->rstc);
> +	if (ret)
> +		goto pwrrdy_off;
> 
> Here you are skipping pm_runtime_resume_and_get(), The subsequent suspend()
> Will lead to pm underflow error.
> 
> Similarly, on suspend() you are checking the error code of reset_assert(),
> If it fails, you deassert it. Surprisingly, there is no deassert operation
> On resume().
Could you please share how would you like to look these functions? It looks
to me that you want to ignore any operation that might fail (as you
proposed in the case of resume from power off) and just re-enable
everything, unconditionally. If that's the case it wouldn't cover all the
cases, either. E.g., if resume looks like this:

static int rzg2l_usbphy_ctrl_resume(struct device *dev)
{
	struct rzg2l_usbphy_ctrl_priv *priv = dev_get_drvdata(dev);

	rzg2l_usbphy_ctrl_set_pwrrdy(priv->pwrrdy, true);
	reset_control_deassert(priv->rstc);
	pm_runtime_resume_and_get(dev);
	rzg2l_usbphy_ctrl_init(priv);

	return 0;
}

the rzg2l_usbphy_ctrl_set_pwrrdy(), reset_control_deassert(),
pm_runtime_resume_and_get() may still fail and may still lead to imbalance
refcounters for the next suspend execution or other scenarios.

Thank you,
Claudiu

RE: [PATCH v2 0/2] reset: rzg2l-usbphy-ctrl: Add suspend to RAM support

[Biju Das]

Hi Claudiu,

> -----Original Message-----
> From: Claudiu Beznea <claudiu.beznea@tuxon.dev>
> Sent: 05 December 2025 14:02
> Subject: Re: [PATCH v2 0/2] reset: rzg2l-usbphy-ctrl: Add suspend to RAM support
> 
> 
> 
> On 12/5/25 15:45, Biju Das wrote:
> >
> >
> >> -----Original Message-----
> >> From: Claudiu Beznea <claudiu.beznea@tuxon.dev>
> >> Sent: 05 December 2025 13:30
> >> To: Biju Das <biju.das.jz@bp.renesas.com>; p.zabel@pengutronix.de
> >> Cc: linux-kernel@vger.kernel.org; linux-renesas-soc@vger.kernel.org;
> >> Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >> Subject: Re: [PATCH v2 0/2] reset: rzg2l-usbphy-ctrl: Add suspend to
> >> RAM support
> >>
> >>
> >>
> >> On 12/5/25 13:55, Biju Das wrote:
> >>> Hi Claudiu,
> >>>
> >>>> -----Original Message-----
> >>>> From: Biju Das
> >>>> Sent: 05 December 2025 10:57
> >>>> Subject: RE: [PATCH v2 0/2] reset: rzg2l-usbphy-ctrl: Add suspend
> >>>> to RAM support
> >>>>
> >>>>
> >>>> Hi Claudiu,
> >>>>
> >>>>> -----Original Message-----
> >>>>> From: Claudiu Beznea <claudiu.beznea@tuxon.dev>
> >>>>> Sent: 05 December 2025 10:47
> >>>>> Subject: Re: [PATCH v2 0/2] reset: rzg2l-usbphy-ctrl: Add suspend
> >>>>> to RAM support
> >>>>>
> >>>>>
> >>>>>
> >>>>> On 12/5/25 12:17, Biju Das wrote:
> >>>>>>
> >>>>>>
> >>>>>>> -----Original Message-----
> >>>>>>> From: Claudiu Beznea <claudiu.beznea@tuxon.dev>
> >>>>>>> Sent: 05 December 2025 10:00
> >>>>>>> To: Biju Das <biju.das.jz@bp.renesas.com>;
> >>>>>>> p.zabel@pengutronix.de
> >>>>>>> Cc: linux-kernel@vger.kernel.org;
> >>>>>>> linux-renesas-soc@vger.kernel.org;
> >>>>>>> Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >>>>>>> Subject: Re: [PATCH v2 0/2] reset: rzg2l-usbphy-ctrl: Add
> >>>>>>> suspend to RAM support
> >>>>>>>
> >>>>>>> Hi, Biju,
> >>>>>>>
> >>>>>>> On 12/5/25 10:53, Biju Das wrote:
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> Hi Claudiu,
> >>>>>>>>
> >>>>>>>>> -----Original Message-----
> >>>>>>>>> From: Claudiu Beznea <claudiu.beznea@tuxon.dev>
> >>>>>>>>> Sent: 04 December 2025 18:26
> >>>>>>>>> Subject: Re: [PATCH v2 0/2] reset: rzg2l-usbphy-ctrl: Add
> >>>>>>>>> suspend to RAM support
> >>>>>>>>>
> >>>>>
> >>>>> From my previous experience with suspend/resume implementations, I
> >>>>> can say restoring the system in failure cases in suspend/resume or
> >>>>> not, is up to the subsystem maintainer. So, I'll let Philipp to
> >>>>> decide how he wants to go with it in this
> >>>> driver.
> >>>>>
> >>>>
> >>>> Agreed.
> >>>>
> >>>>> They are still supporting suspend to idle, where power is
> >>>>> maintained, right? Shouldn't we cover this case?
> >>>>
> >>>> Yes, I agree. Probably best thing is zero failures, if there is a
> >>>> failure in suspend path, the same device will fail in similar
> >>>> fashion, and the system never enters
> >> suspend state.
> >>>>
> >>>> So, report the failure and debug and fix the issue.
> >>>
> >>> FYI, On your resume path, if the below call fails, then there is a pm imbalance for next
> suspend().
> >>>
> >>> ret = pm_runtime_resume_and_get(dev);
> >>>
> >>> Similarly, if reset_assert() fails for a shared reset.
> >>
> >> Wouldn't be the same if there will be no failure path code?
> 
> Could you please reply to this question as I may be wrong?
> 
> >
> >
> > Eg:
> > ret = reset_control_deassert(priv->rstc);
> > +	if (ret)
> > +		goto pwrrdy_off;
> >
> > Here you are skipping pm_runtime_resume_and_get(), The subsequent
> > suspend() Will lead to pm underflow error.
> >
> > Similarly, on suspend() you are checking the error code of
> > reset_assert(), If it fails, you deassert it. Surprisingly, there is
> > no deassert operation On resume().
> Could you please share how would you like to look these functions? It looks to me that you want to
> ignore any operation that might fail (as you proposed in the case of resume from power off) and just
> re-enable everything, unconditionally. If that's the case it wouldn't cover all the cases, either.
> E.g., if resume looks like this:
> 
> static int rzg2l_usbphy_ctrl_resume(struct device *dev) {
> 	struct rzg2l_usbphy_ctrl_priv *priv = dev_get_drvdata(dev);
> 
> 	rzg2l_usbphy_ctrl_set_pwrrdy(priv->pwrrdy, true);
> 	reset_control_deassert(priv->rstc);
> 	pm_runtime_resume_and_get(dev);
> 	rzg2l_usbphy_ctrl_init(priv);
> 
> 	return 0;
> }
> 
> the rzg2l_usbphy_ctrl_set_pwrrdy(), reset_control_deassert(),
> pm_runtime_resume_and_get() may still fail and may still lead to imbalance refcounters for the next
> suspend execution or other scenarios.

There is no perfect solution for resume(). For suspend(), we can put back the state to previous state.
But resume don't know for the failure cases.

Cheers,
Biju

RE: [PATCH v2 0/2] reset: rzg2l-usbphy-ctrl: Add suspend to RAM support

[Biju Das]

> -----Original Message-----
> From: Claudiu Beznea <claudiu.beznea@tuxon.dev>
> Sent: 05 December 2025 14:02
> To: Biju Das <biju.das.jz@bp.renesas.com>; p.zabel@pengutronix.de
> Cc: linux-kernel@vger.kernel.org; linux-renesas-soc@vger.kernel.org; Claudiu Beznea
> <claudiu.beznea.uj@bp.renesas.com>
> Subject: Re: [PATCH v2 0/2] reset: rzg2l-usbphy-ctrl: Add suspend to RAM support
> 
> 
> 
> On 12/5/25 15:45, Biju Das wrote:
> >
> >
> >> -----Original Message-----
> >> From: Claudiu Beznea <claudiu.beznea@tuxon.dev>
> >> Sent: 05 December 2025 13:30
> >> To: Biju Das <biju.das.jz@bp.renesas.com>; p.zabel@pengutronix.de
> >> Cc: linux-kernel@vger.kernel.org; linux-renesas-soc@vger.kernel.org;
> >> Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >> Subject: Re: [PATCH v2 0/2] reset: rzg2l-usbphy-ctrl: Add suspend to
> >> RAM support
> >>
> >>
> >>
> >> On 12/5/25 13:55, Biju Das wrote:
> >>> Hi Claudiu,
> >>>
> >>>> -----Original Message-----
> >>>> From: Biju Das
> >>>> Sent: 05 December 2025 10:57
> >>>> Subject: RE: [PATCH v2 0/2] reset: rzg2l-usbphy-ctrl: Add suspend
> >>>> to RAM support
> >>>>
> >>>>
> >>>> Hi Claudiu,
> >>>>
> >>>>> -----Original Message-----
> >>>>> From: Claudiu Beznea <claudiu.beznea@tuxon.dev>
> >>>>> Sent: 05 December 2025 10:47
> >>>>> Subject: Re: [PATCH v2 0/2] reset: rzg2l-usbphy-ctrl: Add suspend
> >>>>> to RAM support
> >>>>>
> >>>>>
> >>>>>
> >>>>> On 12/5/25 12:17, Biju Das wrote:
> >>>>>>
> >>>>>>
> >>>>>>> -----Original Message-----
> >>>>>>> From: Claudiu Beznea <claudiu.beznea@tuxon.dev>
> >>>>>>> Sent: 05 December 2025 10:00
> >>>>>>> To: Biju Das <biju.das.jz@bp.renesas.com>;
> >>>>>>> p.zabel@pengutronix.de
> >>>>>>> Cc: linux-kernel@vger.kernel.org;
> >>>>>>> linux-renesas-soc@vger.kernel.org;
> >>>>>>> Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >>>>>>> Subject: Re: [PATCH v2 0/2] reset: rzg2l-usbphy-ctrl: Add
> >>>>>>> suspend to RAM support
> >>>>>>>
> >>>>>>> Hi, Biju,
> >>>>>>>
> >>>>>>> On 12/5/25 10:53, Biju Das wrote:
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> Hi Claudiu,
> >>>>>>>>
> >>>>>>>>> -----Original Message-----
> >>>>>>>>> From: Claudiu Beznea <claudiu.beznea@tuxon.dev>
> >>>>>>>>> Sent: 04 December 2025 18:26
> >>>>>>>>> Subject: Re: [PATCH v2 0/2] reset: rzg2l-usbphy-ctrl: Add
> >>>>>>>>> suspend to RAM support
> >>>>>>>>>
> >>>>>
> >>>>> From my previous experience with suspend/resume implementations, I
> >>>>> can say restoring the system in failure cases in suspend/resume or
> >>>>> not, is up to the subsystem maintainer. So, I'll let Philipp to
> >>>>> decide how he wants to go with it in this
> >>>> driver.
> >>>>>
> >>>>
> >>>> Agreed.
> >>>>
> >>>>> They are still supporting suspend to idle, where power is
> >>>>> maintained, right? Shouldn't we cover this case?
> >>>>
> >>>> Yes, I agree. Probably best thing is zero failures, if there is a
> >>>> failure in suspend path, the same device will fail in similar
> >>>> fashion, and the system never enters
> >> suspend state.
> >>>>
> >>>> So, report the failure and debug and fix the issue.
> >>>
> >>> FYI, On your resume path, if the below call fails, then there is a pm imbalance for next
> suspend().
> >>>
> >>> ret = pm_runtime_resume_and_get(dev);
> >>>
> >>> Similarly, if reset_assert() fails for a shared reset.
> >>
> >> Wouldn't be the same if there will be no failure path code?
> 
> Could you please reply to this question as I may be wrong?
> 
> >
> >
> > Eg:
> > ret = reset_control_deassert(priv->rstc);
> > +	if (ret)
> > +		goto pwrrdy_off;
> >
> > Here you are skipping pm_runtime_resume_and_get(), The subsequent
> > suspend() Will lead to pm underflow error.
> >
> > Similarly, on suspend() you are checking the error code of
> > reset_assert(), If it fails, you deassert it. Surprisingly, there is
> > no deassert operation On resume().
> Could you please share how would you like to look these functions? It looks to me that you want to
> ignore any operation that might fail (as you proposed in the case of resume from power off) and just
> re-enable everything, unconditionally. If that's the case it wouldn't cover all the cases, either.
> E.g., if resume looks like this:
> 
> static int rzg2l_usbphy_ctrl_resume(struct device *dev) {
> 	struct rzg2l_usbphy_ctrl_priv *priv = dev_get_drvdata(dev);
> 
> 	rzg2l_usbphy_ctrl_set_pwrrdy(priv->pwrrdy, true);
> 	reset_control_deassert(priv->rstc);
> 	pm_runtime_resume_and_get(dev);
> 	rzg2l_usbphy_ctrl_init(priv);
> 
> 	return 0;
> }

Maybe it is sensible to put the device into clk off,
reset_assert, power off of the device, if any of the calls fails during resume()
which saves the power for failed device.

With return error code, caller knows reset failed and caller won't try
any further operation with failed device.

Cheers,
Biju

RE: [PATCH v2 0/2] reset: rzg2l-usbphy-ctrl: Add suspend to RAM support

[Biju Das]

Hi Claudiu,

> -----Original Message-----
> From: Claudiu Beznea <claudiu.beznea@tuxon.dev>
> Sent: 05 December 2025 10:00
> Subject: Re: [PATCH v2 0/2] reset: rzg2l-usbphy-ctrl: Add suspend to RAM support
> 
> Hi, Biju,
> 
> On 12/5/25 10:53, Biju Das wrote:
> >
> >
> > Hi Claudiu,
> >
> >> -----Original Message-----
> >> From: Claudiu Beznea <claudiu.beznea@tuxon.dev>
> >> Sent: 04 December 2025 18:26
> >> Subject: Re: [PATCH v2 0/2] reset: rzg2l-usbphy-ctrl: Add suspend to
> >> RAM support
> >>
> >> Hi, Philipp,
> >>
> >> Could you please let me know if there's anything I should do for this series?
> >
> > If rzg2l_usbphy_ctrl_suspend() fails, What is the probability that it
> > will suspend again without any issue
> 
> How can I measure this?
> 
> The idea with this code was the following: if any instruction of suspend fails, the suspend is
> aborted, thus code in rzg2l_usbphy_ctrl_suspend() is trying to restore the runtime state of the HW so
> that no runtime users of it to be affected. This is also how core suspend code is doing, e.g.
> suspend_devices_and_enter().

After rechecking, the cleanup() in the suspend code making usage count unbalanced.

Eg:
 Suspend returns error with the following usage count incremented 

static int rzg2l_usbphy_ctrl_suspend(struct device *dev)
{
reset_deassert:
+	reset_control_deassert(priv->rstc);
+rpm_resume:
+	pm_runtime_resume_and_get(dev);
+	return ret;
}


The suspend error code invokes device resume[1] and in that you are again calling
reset_control_deassert() and pm_runtime_resume_and_get() which makes the usage
count unbalanced forever.

So, looks like the current logic in the Add suspend to RAM support patch is wrong.

[1]
https://elixir.bootlin.com/linux/v6.18-rc7/source/kernel/power/suspend.c#L519

static int rzg2l_usbphy_ctrl_resume(struct device *dev)
+{
+	ret = pm_runtime_resume_and_get(dev);
+
+	rzg2l_usbphy_ctrl_init(priv);
+}

Cheers,
Biju

Re: [PATCH v2 0/2] reset: rzg2l-usbphy-ctrl: Add suspend to RAM support

[Claudiu Beznea]

Hi, Biju,

On 12/7/25 13:02, Biju Das wrote:
> Hi Claudiu,
> 
>> -----Original Message-----
>> From: Claudiu Beznea <claudiu.beznea@tuxon.dev>
>> Sent: 05 December 2025 10:00
>> Subject: Re: [PATCH v2 0/2] reset: rzg2l-usbphy-ctrl: Add suspend to RAM support
>>
>> Hi, Biju,
>>
>> On 12/5/25 10:53, Biju Das wrote:
>>>
>>>
>>> Hi Claudiu,
>>>
>>>> -----Original Message-----
>>>> From: Claudiu Beznea <claudiu.beznea@tuxon.dev>
>>>> Sent: 04 December 2025 18:26
>>>> Subject: Re: [PATCH v2 0/2] reset: rzg2l-usbphy-ctrl: Add suspend to
>>>> RAM support
>>>>
>>>> Hi, Philipp,
>>>>
>>>> Could you please let me know if there's anything I should do for this series?
>>>
>>> If rzg2l_usbphy_ctrl_suspend() fails, What is the probability that it
>>> will suspend again without any issue
>>
>> How can I measure this?
>>
>> The idea with this code was the following: if any instruction of suspend fails, the suspend is
>> aborted, thus code in rzg2l_usbphy_ctrl_suspend() is trying to restore the runtime state of the HW so
>> that no runtime users of it to be affected. This is also how core suspend code is doing, e.g.
>> suspend_devices_and_enter().
> 
> After rechecking, the cleanup() in the suspend code making usage count unbalanced.
> 
> Eg:
>  Suspend returns error with the following usage count incremented 
> 
> static int rzg2l_usbphy_ctrl_suspend(struct device *dev)
> {
> reset_deassert:
> +	reset_control_deassert(priv->rstc);
> +rpm_resume:
> +	pm_runtime_resume_and_get(dev);
> +	return ret;
> }
> 
> 
> The suspend error code invokes device resume[1] and in that you are again calling
> reset_control_deassert() and pm_runtime_resume_and_get() which makes the usage
> count unbalanced forever.
> 
> So, looks like the current logic in the Add suspend to RAM support patch is wrong.

Seeing [1], [2], [3] being posted by you, and [2] following the same
pattern as proposed in this patch, are you still considering the approach
in this patch being wrong ?

Thank you,
Claudiu

[1]
https://lore.kernel.org/all/20251207124742.96526-1-biju.das.jz@bp.renesas.com
[2]
https://lore.kernel.org/all/20251208152133.269316-8-biju.das.jz@bp.renesas.com
[3]
https://lore.kernel.org/all/20251208101356.101379-1-biju.das.jz@bp.renesas.com

RE: [PATCH v2 0/2] reset: rzg2l-usbphy-ctrl: Add suspend to RAM support

[Biju Das]

Hi Claudiu,

> -----Original Message-----
> From: Claudiu Beznea <claudiu.beznea@tuxon.dev>
> Sent: 16 December 2025 09:04
> Subject: Re: [PATCH v2 0/2] reset: rzg2l-usbphy-ctrl: Add suspend to RAM support
> 
> Hi, Biju,
> 
> On 12/7/25 13:02, Biju Das wrote:
> > Hi Claudiu,
> >
> >> -----Original Message-----
> >> From: Claudiu Beznea <claudiu.beznea@tuxon.dev>
> >> Sent: 05 December 2025 10:00
> >> Subject: Re: [PATCH v2 0/2] reset: rzg2l-usbphy-ctrl: Add suspend to
> >> RAM support
> >>
> >> Hi, Biju,
> >>
> >> On 12/5/25 10:53, Biju Das wrote:
> >>>
> >>>
> >>> Hi Claudiu,
> >>>
> >>>> -----Original Message-----
> >>>> From: Claudiu Beznea <claudiu.beznea@tuxon.dev>
> >>>> Sent: 04 December 2025 18:26
> >>>> Subject: Re: [PATCH v2 0/2] reset: rzg2l-usbphy-ctrl: Add suspend
> >>>> to RAM support
> >>>>
> >>>> Hi, Philipp,
> >>>>
> >>>> Could you please let me know if there's anything I should do for this series?
> >>>
> >>> If rzg2l_usbphy_ctrl_suspend() fails, What is the probability that
> >>> it will suspend again without any issue
> >>
> >> How can I measure this?
> >>
> >> The idea with this code was the following: if any instruction of
> >> suspend fails, the suspend is aborted, thus code in
> >> rzg2l_usbphy_ctrl_suspend() is trying to restore the runtime state of the HW so that no runtime
> users of it to be affected. This is also how core suspend code is doing, e.g.
> >> suspend_devices_and_enter().
> >
> > After rechecking, the cleanup() in the suspend code making usage count unbalanced.
> >
> > Eg:
> >  Suspend returns error with the following usage count incremented
> >
> > static int rzg2l_usbphy_ctrl_suspend(struct device *dev) {
> > reset_deassert:
> > +	reset_control_deassert(priv->rstc);
> > +rpm_resume:
> > +	pm_runtime_resume_and_get(dev);
> > +	return ret;
> > }
> >
> >
> > The suspend error code invokes device resume[1] and in that you are
> > again calling
> > reset_control_deassert() and pm_runtime_resume_and_get() which makes
> > the usage count unbalanced forever.
> >
> > So, looks like the current logic in the Add suspend to RAM support patch is wrong.
> 
> Seeing [1], [2], [3] being posted by you, and [2] following the same pattern as proposed in this
> patch, are you still considering the approach in this patch being wrong ?

LGTM. as reset failures will be automatically taken care in patch[3].

Assert failure in suspend(): set to deassert state
Deassert failure in resume(): set to assert stae.

Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>

Cheers,
Biju

> 
> Thank you,
> Claudiu
> 
> [1]
> https://lore.kernel.org/all/20251207124742.96526-1-biju.das.jz@bp.renesas.com
> [2]
> https://lore.kernel.org/all/20251208152133.269316-8-biju.das.jz@bp.renesas.com
> [3]
> https://lore.kernel.org/all/20251208101356.101379-1-biju.das.jz@bp.renesas.com