From: Louis Chauvet <louis.chauvet@bootlin.com>
When the OTG USB port is used to power to SoC, configured as peripheral and
used in gadget mode, there is a disconnection about 6 seconds after the
gadget is configured and enumerated.
The problem was observed on a Radxa Rock Pi S board, which can only be
powered by the only USB-C connector. That connector is the only one usable
in gadget mode. This implies the USB cable is connected from before boot
and never disconnects while the kernel runs.
The problem happens because of the PHY driver code flow, summarized as:
* UDC start code (triggered via configfs at any time after boot)
-> phy_init
-> rockchip_usb2phy_init
-> schedule_delayed_work(otg_sm_work [A], 6 sec)
-> phy_power_on
-> rockchip_usb2phy_power_on
-> enable clock
-> rockchip_usb2phy_reset
* Now the gadget interface is up and running.
* 6 seconds later otg_sm_work starts [A]
-> rockchip_usb2phy_otg_sm_work():
if (B_IDLE state && VBUS present && ...):
schedule_delayed_work(&rport->chg_work [B], 0);
* immediately the chg_detect_work starts [B]
-> rockchip_chg_detect_work():
if chg_state is UNDEFINED:
if (!rport->suspended):
rockchip_usb2phy_power_off() <--- [X]
At [X], the PHY is powered off, causing a disconnection. This quickly
triggers a new connection and following re-enumeration, but any connection
that had been established during the 6 seconds is broken.
The code already checks for !rport->suspended, so add a guard for VBUS as
well to avoid a disconnection when a cable is connected.
Fixes: 98898f3bc83c ("phy: rockchip-inno-usb2: support otg-port for rk3399")
Closes: https://lore.kernel.org/lkml/20250414185458.7767aabc@booty/
Co-developed-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
---
drivers/phy/rockchip/phy-rockchip-inno-usb2.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
index b0f23690ec3002202c0f33a6988f5509622fa10e..0106d7b7ae24ead91d9c996daaa56671de02a39a 100644
--- a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
+++ b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
@@ -821,14 +821,16 @@ static void rockchip_chg_detect_work(struct work_struct *work)
container_of(work, struct rockchip_usb2phy_port, chg_work.work);
struct rockchip_usb2phy *rphy = dev_get_drvdata(rport->phy->dev.parent);
struct regmap *base = get_reg_base(rphy);
- bool is_dcd, tmout, vout;
+ bool is_dcd, tmout, vout, vbus_attach;
unsigned long delay;
+ vbus_attach = property_enabled(rphy->grf, &rport->port_cfg->utmi_bvalid);
+
dev_dbg(&rport->phy->dev, "chg detection work state = %d\n",
rphy->chg_state);
switch (rphy->chg_state) {
case USB_CHG_STATE_UNDEFINED:
- if (!rport->suspended)
+ if (!rport->suspended && !vbus_attach)
rockchip_usb2phy_power_off(rport->phy);
/* put the controller in non-driving mode */
property_enable(base, &rphy->phy_cfg->chg_det.opmode, false);
--
2.50.1Hello Luca,
On Tue Jul 22, 2025 at 10:43 AM CEST, Luca Ceresoli wrote:
> From: Louis Chauvet <louis.chauvet@bootlin.com>
>
> When the OTG USB port is used to power to SoC, configured as peripheral and
typo: is used to power *the* SoC
> used in gadget mode, there is a disconnection about 6 seconds after the
> gadget is configured and enumerated.
>
> The problem was observed on a Radxa Rock Pi S board, which can only be
> powered by the only USB-C connector. That connector is the only one usable
"which can only be powered by the only USB-C connector"
double "only" makes it a super exclusive connector!
> in gadget mode. This implies the USB cable is connected from before boot
> and never disconnects while the kernel runs.
>
> The problem happens because of the PHY driver code flow, summarized as:
>
> * UDC start code (triggered via configfs at any time after boot)
> -> phy_init
> -> rockchip_usb2phy_init
> -> schedule_delayed_work(otg_sm_work [A], 6 sec)
> -> phy_power_on
> -> rockchip_usb2phy_power_on
> -> enable clock
> -> rockchip_usb2phy_reset
>
> * Now the gadget interface is up and running.
>
> * 6 seconds later otg_sm_work starts [A]
> -> rockchip_usb2phy_otg_sm_work():
> if (B_IDLE state && VBUS present && ...):
> schedule_delayed_work(&rport->chg_work [B], 0);
>
> * immediately the chg_detect_work starts [B]
> -> rockchip_chg_detect_work():
> if chg_state is UNDEFINED:
> if (!rport->suspended):
> rockchip_usb2phy_power_off() <--- [X]
>
> At [X], the PHY is powered off, causing a disconnection. This quickly
> triggers a new connection and following re-enumeration, but any connection
> that had been established during the 6 seconds is broken.
>
> The code already checks for !rport->suspended, so add a guard for VBUS as
> well to avoid a disconnection when a cable is connected.
Your commit message was clear but I was missing one key point: what
rport->suspended means. It isn't what I first thought. Instead it means
phy is powered off. Naming is bad but unrelated to your series. Maybe
add a comment to your commit message like the following?
The code already checks for !rport->suspended (PHY is powered on), ...
> Fixes: 98898f3bc83c ("phy: rockchip-inno-usb2: support otg-port for rk3399")
> Closes: https://lore.kernel.org/lkml/20250414185458.7767aabc@booty/
> Co-developed-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
I believe there is an issue with your SoB ordering. It is meant to
signal the path's route from author into the kernel tree. In that
sense, it should start with author followed by you as submitter. Then
the maintainer will append its own.
Documentation/process/submitting-patches.rst is semi-explicitly writing
that out:
> Any further SoBs [...] following the author's SoB [...]
https://elixir.bootlin.com/linux/v6.17.9/source/Documentation/process/submitting-patches.rst#L449-L453
One good way to check is that all maintainers always append their SoB
rather than prepend.
With that,
Reviewed-by: Théo Lebrun <theo.lebrun@bootlin.com>
Thanks,
--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Hi Théo, thanks for reviewing this series! On Tue Nov 25, 2025 at 4:28 PM CET, Théo Lebrun wrote: [...] >> The problem was observed on a Radxa Rock Pi S board, which can only be >> powered by the only USB-C connector. That connector is the only one usable > > "which can only be powered by the only USB-C connector" > double "only" makes it a super exclusive connector! Hehe, indeed! :-) >> The code already checks for !rport->suspended, so add a guard for VBUS as >> well to avoid a disconnection when a cable is connected. > > Your commit message was clear but I was missing one key point: what > rport->suspended means. It isn't what I first thought. Instead it means > phy is powered off. Naming is bad but unrelated to your series. Maybe > add a comment to your commit message like the following? > > The code already checks for !rport->suspended (PHY is powered on), ... You are right. I have added a slightly longer text: The code already checks for !rport->suspended (which, somewhat counter-intuitively, means the PHY is powered on), ... Still worth your Reviewed-by? I fixed the other issues you have reported, for patch 2 as well. I also added the Cc: stable@vger.kernel.org line, which I noticed being missing. Luca -- Luca Ceresoli, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
On Thu Nov 27, 2025 at 10:22 AM CET, Luca Ceresoli wrote: > On Tue Nov 25, 2025 at 4:28 PM CET, Théo Lebrun wrote: >>> The code already checks for !rport->suspended, so add a guard for VBUS as >>> well to avoid a disconnection when a cable is connected. >> >> Your commit message was clear but I was missing one key point: what >> rport->suspended means. It isn't what I first thought. Instead it means >> phy is powered off. Naming is bad but unrelated to your series. Maybe >> add a comment to your commit message like the following? >> >> The code already checks for !rport->suspended (PHY is powered on), ... > > You are right. I have added a slightly longer text: > > The code already checks for !rport->suspended (which, somewhat > counter-intuitively, means the PHY is powered on), ... > > Still worth your Reviewed-by? Even more so. > I fixed the other issues you have reported, for patch 2 as well. > > I also added the Cc: stable@vger.kernel.org line, which I noticed being > missing. I never add that Cc trailer and only rely on `Fixes:`. I thought it used to be documented as an alternative to that Cc trailer but it does not show up in `git log -p Documentation/process/stable-kernel-rules.rst` There is one indirect mention of "scripts that look for commits containing a 'Fixes:' tag": https://elixir.bootlin.com/linux/v6.17.9/source/Documentation/process/stable-kernel-rules.rst#L132-L134 Anyway, you do right by explicitly tagging `Cc: stable@...`. Thanks, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
Hi Théo, On Thu Nov 27, 2025 at 10:48 AM CET, Théo Lebrun wrote: > On Thu Nov 27, 2025 at 10:22 AM CET, Luca Ceresoli wrote: >> On Tue Nov 25, 2025 at 4:28 PM CET, Théo Lebrun wrote: >>>> The code already checks for !rport->suspended, so add a guard for VBUS as >>>> well to avoid a disconnection when a cable is connected. >>> >>> Your commit message was clear but I was missing one key point: what >>> rport->suspended means. It isn't what I first thought. Instead it means >>> phy is powered off. Naming is bad but unrelated to your series. Maybe >>> add a comment to your commit message like the following? >>> >>> The code already checks for !rport->suspended (PHY is powered on), ... >> >> You are right. I have added a slightly longer text: >> >> The code already checks for !rport->suspended (which, somewhat >> counter-intuitively, means the PHY is powered on), ... >> >> Still worth your Reviewed-by? > > Even more so. Thanks, v2 on its way. >> I also added the Cc: stable@vger.kernel.org line, which I noticed being >> missing. > > I never add that Cc trailer and only rely on `Fixes:`. I thought it > used to be documented as an alternative to that Cc trailer but it does > not show up in `git log -p Documentation/process/stable-kernel-rules.rst` > > There is one indirect mention of "scripts that look for commits > containing a 'Fixes:' tag": > https://elixir.bootlin.com/linux/v6.17.9/source/Documentation/process/stable-kernel-rules.rst#L132-L134 > > Anyway, you do right by explicitly tagging `Cc: stable@...`. Theory says Cc: is needed: > Note: Attaching a Fixes: tag does not subvert the stable kernel rules > process nor the requirement to Cc: stable@vger.kernel.org on all stable > patch candidates. (https://docs.kernel.org/process/submitting-patches.html#reviewer-s-statement-of-oversight) But in the practice I happened to forget Cc: stable in the past, the patch got applied and the Fixes: tag was enough for correct cherry-pick in stable branches. Luca -- Luca Ceresoli, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
On Thu, Nov 27, 2025 at 11:18:45AM +0100, Luca Ceresoli wrote: > Hi Théo, > > On Thu Nov 27, 2025 at 10:48 AM CET, Théo Lebrun wrote: > > On Thu Nov 27, 2025 at 10:22 AM CET, Luca Ceresoli wrote: > >> On Tue Nov 25, 2025 at 4:28 PM CET, Théo Lebrun wrote: > >>>> The code already checks for !rport->suspended, so add a guard for VBUS as > >>>> well to avoid a disconnection when a cable is connected. > >>> > >>> Your commit message was clear but I was missing one key point: what > >>> rport->suspended means. It isn't what I first thought. Instead it means > >>> phy is powered off. Naming is bad but unrelated to your series. Maybe > >>> add a comment to your commit message like the following? > >>> > >>> The code already checks for !rport->suspended (PHY is powered on), ... > >> > >> You are right. I have added a slightly longer text: > >> > >> The code already checks for !rport->suspended (which, somewhat > >> counter-intuitively, means the PHY is powered on), ... > >> > >> Still worth your Reviewed-by? > > > > Even more so. > > Thanks, v2 on its way. > > >> I also added the Cc: stable@vger.kernel.org line, which I noticed being > >> missing. > > > > I never add that Cc trailer and only rely on `Fixes:`. I thought it > > used to be documented as an alternative to that Cc trailer but it does > > not show up in `git log -p Documentation/process/stable-kernel-rules.rst` > > > > There is one indirect mention of "scripts that look for commits > > containing a 'Fixes:' tag": > > https://elixir.bootlin.com/linux/v6.17.9/source/Documentation/process/stable-kernel-rules.rst#L132-L134 > > > > Anyway, you do right by explicitly tagging `Cc: stable@...`. > > Theory says Cc: is needed: > > > Note: Attaching a Fixes: tag does not subvert the stable kernel rules > > process nor the requirement to Cc: stable@vger.kernel.org on all stable > > patch candidates. > (https://docs.kernel.org/process/submitting-patches.html#reviewer-s-statement-of-oversight) > > But in the practice I happened to forget Cc: stable in the past, the patch > got applied and the Fixes: tag was enough for correct cherry-pick in stable > branches. That is never guaranteed, it is a "best effort only when the stable maintainers are bored" type of thing. Always be explicit, and use cc: stable, as the documentation has stated for the last 17+ years :) thanks, greg k-h
© 2016 - 2026 Red Hat, Inc.