Re: [PATCH 1/2] phy: rockchip: inno-usb2: fix disconnection in gadget mode

["Théo Lebrun" <theo.lebrun@bootlin.com>]

Hello 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